Hi Suzuki,
On Thu, 6 Jun 2019 at 13:55, Suzuki K Poulose suzuki.poulose@arm.com wrote:
On 01/05/2019 09:49, Mike Leach wrote:
Adds in required sysfs access for:-
- CoreSight management registers.
- CTI device specific registers.
- Channel Cross triggering API.
This provides the basic programming interface for the device.
Signed-off-by: Mike Leach mike.leach@linaro.org
.../hwtracing/coresight/coresight-cti-sysfs.c | 831 +++++++++++++++++-
Please could you split this patch into smaller chunks ? This is slightly bigger patch for a healthy review. I have noted down some minor issues below. Otherwise, I look forward to the next version.
This has been split into 3 chunks in the next version. Will map your comments across to v3 where they still apply.
Thanks
Mike.
drivers/hwtracing/coresight/coresight-cti.c | 188 +++- drivers/hwtracing/coresight/coresight-cti.h | 42 + drivers/hwtracing/coresight/coresight-priv.h | 2 + 4 files changed, 1053 insertions(+), 10 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c index 9b6749621dcb..ff8dad02a779 100644 --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c @@ -34,8 +34,8 @@ static ssize_t enable_show(struct device *dev, struct device_attribute *attr, char *buf) {
int enable_req;bool enabled, powered, cpuid;
int enable_req, cpuid;bool enabled, powered; struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); ssize_t size = 0;@@ -46,12 +46,18 @@ static ssize_t enable_show(struct device *dev, cpuid = drvdata->ctidev.cpu; spin_unlock(&drvdata->spinlock);
if (powered) {size = scnprintf(buf, PAGE_SIZE, "cti %s; cpu%d powered;\n",enabled ? "enabled" : "disabled", cpuid);} else if (cpuid >= 0) {size = scnprintf(buf, PAGE_SIZE, "cti %s; cpu%d unpowered;\n",enable_req ? "enable req" : "disable req", cpuid);
if (cpuid >= 0) {if (powered) {size = scnprintf(buf, PAGE_SIZE,"cti %s; cpu%d powered;\n",enabled ? "enabled" : "disabled",cpuid);} else {size = scnprintf(buf, PAGE_SIZE,"cti %s; cpu%d unpowered;\n",enable_req ? "enable req" : "disable req",cpuid);} } else {Could we have started off with the above change in the first patch ?
size = scnprintf(buf, PAGE_SIZE, "cti %s; no assoc cpu;\n", enabled ? "enabled" : "disabled");@@ -81,17 +87,826 @@ static ssize_t enable_store(struct device *dev, } static DEVICE_ATTR_RW(enable);
+static ssize_t ctmid_show(struct device *dev,
struct device_attribute *attr,char *buf)+{
struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);return scnprintf(buf, PAGE_SIZE, "0x%x\n", drvdata->ctidev.ctm_id);+} +static DEVICE_ATTR_RO(ctmid);
- /* attribute and group sysfs tables. */ static struct attribute *coresight_cti_attrs[] = { &dev_attr_enable.attr,
&dev_attr_ctmid.attr, NULL,+/* register based attributes */ +#define coresight_cti_reg(name, offset) \
coresight_simple_reg32(struct cti_drvdata, name, offset)+/*
- Show a simple 32 bit value. If pval is NULL then live read,
- otherwise read from supplied pointer only.
- */
+static ssize_t cti_reg32_show(struct device *dev, char *buf,
u32 *pval, int reg_offset)+{
unsigned long val = 0;struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);struct cti_config *config = &drvdata->config;spin_lock(&drvdata->spinlock);if (pval) {val = (unsigned long)*pval;} else if ((reg_offset >= 0) && CTI_PWR_ENA(config)) {CS_UNLOCK(drvdata->base);val = readl_relaxed(drvdata->base + reg_offset);CS_LOCK(drvdata->base);}spin_unlock(&drvdata->spinlock);return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);+}
+/*
- Store a simple 32 bit value.
- If pval 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 *pval,int reg_offset)+{
unsigned long val;struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);struct cti_config *config = &drvdata->config;if (kstrtoul(buf, 16, &val))Please use 0, instead of hardcoding 16.
return -EINVAL;spin_lock(&drvdata->spinlock);/* local store */if (pval)*pval = (u32)val;/* write through of offset and enabled */if ((reg_offset >= 0) && CTI_PWR_ENA(config))cti_write_single_reg(drvdata, reg_offset, val);spin_unlock(&drvdata->spinlock);return size;+}
+/* coresight management registers */ +coresight_cti_reg(devaff0, CTIDEVAFF0); +coresight_cti_reg(devaff1, CTIDEVAFF1); +coresight_cti_reg(authstatus, CORESIGHT_AUTHSTATUS); +coresight_cti_reg(devarch, CORESIGHT_DEVARCH); +coresight_cti_reg(devid, CORESIGHT_DEVID); +coresight_cti_reg(devtype, CORESIGHT_DEVTYPE); +coresight_cti_reg(pidr0, CORESIGHT_PERIPHIDR0); +coresight_cti_reg(pidr1, CORESIGHT_PERIPHIDR1); +coresight_cti_reg(pidr2, CORESIGHT_PERIPHIDR2); +coresight_cti_reg(pidr3, CORESIGHT_PERIPHIDR3); +coresight_cti_reg(pidr4, CORESIGHT_PERIPHIDR4);
Are the PIDs really needed ? If at all they are needed, could we expose the unified PID from these 4 registers ?
+static ssize_t itchout_show(struct device *dev,
struct device_attribute *attr,char *buf)+{
return cti_reg32_show(dev, buf, NULL, ITCHOUT);+}
Whats the purpose of the exposing integration test registers ?
+static ssize_t itctrl_show(struct device *dev,
struct device_attribute *attr,char *buf)+{
return cti_reg32_show(dev, buf, NULL, CORESIGHT_ITCTRL);+}
+static ssize_t itctrl_store(struct device *dev,
struct device_attribute *attr,const char *buf, size_t size)+{
return cti_reg32_store(dev, buf, size, NULL, CORESIGHT_ITCTRL);+} +static DEVICE_ATTR_RW(itctrl);
You may be able to use a macro to make the above more compact.
#define cti_reg32_attr(name, offset) \ static ssize_t name##_show(struct device *dev, \ struct device_attribute *attr, \ char *buf) \ { \ return cti_reg32_show(dev, buf, NULL, offset); \ } \ \ static ssize_t name##_store(struct device *dev, \ struct device_attribute *attr, \ const char *buf, size_t size) \ { \ return cti_reg32_store(dev, buf, size, NULL, \ offset); \ \ } \ static DEVICE_ATTR_RW(name);
cti_reg32(itctrl, CORESIGHT_ITCTRL); cti_reg32(...., );
Or even use the extended attribute to store the "Offset" and then use a common routine to store/show the reg at the offset.
+/* CTI channel x-trigger programming */ +static int +cti_trig_op_parse(struct device *dev, enum cti_chan_op op,
enum cti_trig_dir dir, const char *buf, size_t size)+{
u32 chan_idx;u32 trig_idx;int items, err = size;/* extract chan idx and trigger idx */items = sscanf(buf, "%d %d", &chan_idx, &trig_idx);if (items) {Should we not match items == 2 to make sure we read a trig_idx as well ?
err = cti_channel_trig_op(dev, op, dir, chan_idx, trig_idx);if (!err)err = size;} elseerr = -EINVAL;minor nit: Please use :
else { err = -EINVAL; }to match the coding style.
+static ssize_t trigout_attach_store(struct device *dev,
struct device_attribute *attr,const char *buf, size_t size)+{
return cti_trig_op_parse(dev, CTI_CHAN_ATTACH, CTI_TRIG_OUT,buf, size);+} +static DEVICE_ATTR_WO(trigout_attach);
+static ssize_t trigout_detach_store(struct device *dev,
struct device_attribute *attr,const char *buf, size_t size)+{
return cti_trig_op_parse(dev, CTI_CHAN_DETACH, CTI_TRIG_OUT,buf, size);+} +static DEVICE_ATTR_WO(trigout_detach);
Again, as above you may be able to use a macro toe make it more easier.
+static ssize_t reset_xtrigs_store(struct device *dev,
struct device_attribute *attr,const char *buf, size_t size)+{
int i;struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);struct cti_config *config = &drvdata->config;spin_lock(&drvdata->spinlock);/* clear the CTI trigger / channel programming registers */for (i = 0; i < config->nr_trig_max; i++) {config->ctiinen[i] = 0;config->ctiouten[i] = 0;}/* clear the other regs */config->ctigate = (0x1 << config->nr_ctm_channels) - 1;Please GENMASK();
config->asicctl = 0;config->ctiappset = 0;config->ctiinout_sel = 0;/* if enabled then write through */if (CTI_PWR_ENA(config))cti_write_all_hw_regs(drvdata);spin_unlock(&drvdata->spinlock);return size;+} +static DEVICE_ATTR_WO(reset_xtrigs);
+static ssize_t show_chan_sel_show(struct device *dev,
struct device_attribute *attr,char *buf)+{
u32 val;struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);val = (u32)drvdata->config.xtrig_rchan_sel;return scnprintf(buf, PAGE_SIZE, "%d\n", val);+}
+/* select a channel for the show_chan_xtrig function */ +static ssize_t show_chan_sel_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);if (kstrtoul(buf, 16, &val))same as above.
return -EINVAL;if (val > (drvdata->config.nr_ctm_channels-1))nit: coding style, missing space.
return -EINVAL;spin_lock(&drvdata->spinlock);drvdata->config.xtrig_rchan_sel = val;spin_unlock(&drvdata->spinlock);return size;+} +static DEVICE_ATTR_RW(show_chan_sel);
Suzuki