Hi Suzuki,
On Wed, 27 Nov 2019 at 18:26, Suzuki Kuruppassery Poulose suzuki.poulose@arm.com wrote:
On 19/11/2019 23:19, Mike Leach wrote:
Adds in sysfs programming support for the CTI function register sets. Allows direct manipulation of channel / trigger association registers.
Reviewed-by: Mathieu Poirier mathieu.poirier@linaro.org Signed-off-by: Mike Leach mike.leach@linaro.org
.../hwtracing/coresight/coresight-cti-sysfs.c | 362 ++++++++++++++++++ drivers/hwtracing/coresight/coresight-cti.c | 19 + drivers/hwtracing/coresight/coresight-cti.h | 5 + 3 files changed, 386 insertions(+)
diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c index 507f8eb487fe..02d3ee0c1278 100644 --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c @@ -109,6 +109,362 @@ static struct attribute *coresight_cti_mgmt_attrs[] = { NULL, };
+/* CTI low level programming registers */
+/*
- Show a simple 32 bit value if enabled and powered.
- If inaccessible & pcached_val not NULL then show cached value.
- */
Also I am not sure if it makes sense to mention that the value is cached.
+static ssize_t cti_reg32_show(struct device *dev, char *buf,
u32 *pcached_val, int reg_offset)+{
u32 val = 0;
- char *state = "";
struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);struct cti_config *config = &drvdata->config;spin_lock(&drvdata->spinlock);if ((reg_offset >= 0) && CTI_PWR_ENA(config)) {minor nit: Personally I don't like the naming here. This could simply be: cti_accessible(config) , may be defined as a static inline function instead of a macro:
static inline bool cti_accessible(struct cti_drvdata *drvdata) { struct cti_config *cfg = &drvdata->config;
return cfg->hw_powered && cfg->hw_enabled;}
Since this is a generic access function used throughout the file - the cached pointer is an indicator used by the callee that there is a value available if the CTI is unpowered / disabled - so the function can show an appropriate value which will be taken from the config structure.
So I don't think it is relevant to show that a "cached" value is being used to show the user. If you look at similar functions in the ETM drivers for example, quite often a show function simple shows that stored value from a config structure without ever looking at the register in the device.
As to naming - the name is chosen to represent a specific state - both powered and enabled. The sysfs interface is accessible in any state - powered / unpowered , enabled /disabled - so I am being specific. Unlike the ETM, this hardware can have registers programmed while enabled - and for some such as apppulse this is the only time it makes sense to use them.
I don't mind either way between macro / inline function - though it still has to be declared in the header as it is used in multiple .c files. I'd be inclined to call it cti_active() if preferred to cti_pwr_ena - active implies that the CTI is in operation.
Thanks
Mike
CS_UNLOCK(drvdata->base);val = readl_relaxed(drvdata->base + reg_offset);if (pcached_val)*pcached_val = val;CS_LOCK(drvdata->base);} else if (pcached_val) {val = *pcached_val;
state = " (cached)";
}spin_unlock(&drvdata->spinlock);return scnprintf(buf, PAGE_SIZE, "%#x\n", val);
- return scnprintf(buf, PAGE_SIZE, "%#x%s\n", val, state);
+}
+/*
- Store a simple 32 bit value.
- If pcached_val not NULL, then copy to here too,
- if reg_offset >= 0 then write through if enabled.
- */
+static ssize_t cti_reg32_store(struct device *dev, const char *buf,
size_t size, u32 *pcached_val, int reg_offset)+static ssize_t appclear_store(struct device *dev,
struct device_attribute *attr,const char *buf, size_t size)+{
unsigned long val;struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);struct cti_config *config = &drvdata->config;if (kstrtoul(buf, 0, &val))return -EINVAL;spin_lock(&drvdata->spinlock);/* a 1'b1 in appclr clears down the same bit in appset*/nit: a 0b1 ?
Syntax is <bitwidth>'<radix><value> - a habit picked up from verilog.
config->ctiappset &= ~val;/* write through if enabled */if (CTI_PWR_ENA(config))cti_write_single_reg(drvdata, CTIAPPCLEAR, val);spin_unlock(&drvdata->spinlock);return size;+} +static DEVICE_ATTR_WO(appclear);
Otherwise looks good to me.
Suzuki