Hi Suzuki
On Wed, 12 Jun 2019 at 11:52, Suzuki K Poulose suzuki.poulose@arm.com wrote:
On 05/06/2019 20:03, Mike Leach wrote:
Adds in sysfs programming support for the CTI function register sets. Allows direct manipulation of channel / trigger association registers.
Signed-off-by: Mike Leach mike.leach@linaro.org
.../hwtracing/coresight/coresight-cti-sysfs.c | 402 ++++++++++++++++++ drivers/hwtracing/coresight/coresight-cti.c | 19 + drivers/hwtracing/coresight/coresight-cti.h | 6 + 3 files changed, 427 insertions(+)
diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c index ad7c45390313..caecf011f6e0 100644 --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c @@ -115,6 +115,402 @@ static struct attribute *coresight_cti_mgmt_attrs[] = { NULL, };
+/* CTI low level programming registers */
+/*
- 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)nit: Please could you rename pval to *pcached ?
done.
+{
u32 val = 0;struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);struct cti_config *config = &drvdata->config;spin_lock(&drvdata->spinlock);if (pval) {val = *pval;} else if ((reg_offset >= 0) && CTI_PWR_ENA(config)) {nit: s/CTI_PWR_ENA/cti_available(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, "%#x\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 do not hard code 16. Use 0 instead, everywhere.
Fixed in all places
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;+}
+static ssize_t inout_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.ctiinout_sel;return scnprintf(buf, PAGE_SIZE, "%d\n", val);+}
I think we may be able to use dev_ext_attribute to expose the individual inen/inout attributes rather than using the inout_sel. I am not too particular about this, as it may follow the existing scheme for ETMs.
+static ssize_t inout_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, 10, &val))return -EINVAL;if (val > (CTIINOUTEN_MAX-1))return -EINVAL;spin_lock(&drvdata->spinlock);drvdata->config.ctiinout_sel = val;spin_unlock(&drvdata->spinlock);return size;+} +static DEVICE_ATTR_RW(inout_sel);
+static ssize_t inen_show(struct device *dev,
struct device_attribute *attr,char *buf)+{
unsigned long val;int index;struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);spin_lock(&drvdata->spinlock);index = drvdata->config.ctiinout_sel;val = drvdata->config.ctiinen[index];spin_unlock(&drvdata->spinlock);return scnprintf(buf, PAGE_SIZE, "INEN%d %#lx\n", index, val);+}
+static ssize_t inen_store(struct device *dev,
struct device_attribute *attr,const char *buf, size_t size)+{
unsigned long val;int index;struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);struct cti_config *config = &drvdata->config;if (kstrtoul(buf, 16, &val))return -EINVAL;spin_lock(&drvdata->spinlock);index = config->ctiinout_sel;config->ctiinen[index] = val;/* write through if enabled */if (CTI_PWR_ENA(config))cti_write_single_reg(drvdata, CTIINEN(index), val);spin_unlock(&drvdata->spinlock);return size;+} +static DEVICE_ATTR_RW(inen);
+static ssize_t outen_show(struct device *dev,
struct device_attribute *attr,char *buf)+{
unsigned long val;int index;struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);spin_lock(&drvdata->spinlock);index = drvdata->config.ctiinout_sel;val = drvdata->config.ctiouten[index];spin_unlock(&drvdata->spinlock);return scnprintf(buf, PAGE_SIZE, "OUTEN%d %#lx\n", index, val);+}
+static ssize_t outen_store(struct device *dev,
struct device_attribute *attr,const char *buf, size_t size)+{
unsigned long val;int index;struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);struct cti_config *config = &drvdata->config;if (kstrtoul(buf, 16, &val))return -EINVAL;spin_lock(&drvdata->spinlock);index = config->ctiinout_sel;config->ctiouten[index] = val;/* write through if enabled */if (CTI_PWR_ENA(config))cti_write_single_reg(drvdata, CTIOUTEN(index), val);spin_unlock(&drvdata->spinlock);return size;+} +static DEVICE_ATTR_RW(outen);
-- CUT here ---
+static ssize_t gate_show(struct device *dev,
struct device_attribute *attr,char *buf)+{
struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);return cti_reg32_show(dev, buf, &drvdata->config.ctigate, -1);+}
+static ssize_t gate_store(struct device *dev,
struct device_attribute *attr,const char *buf, size_t size)+{
struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);return cti_reg32_store(dev, buf, size,&drvdata->config.ctigate, CTIGATE);+} +static DEVICE_ATTR_RW(gate);
+static ssize_t asicctl_show(struct device *dev,
struct device_attribute *attr,char *buf)+{
struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);return cti_reg32_show(dev, buf, &drvdata->config.asicctl, -1);+}
+static ssize_t asicctl_store(struct device *dev,
struct device_attribute *attr,const char *buf, size_t size)+{
struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);return cti_reg32_store(dev, buf, size,&drvdata->config.asicctl, ASICCTL);+} +static DEVICE_ATTR_RW(asicctl);
+static ssize_t appset_show(struct device *dev,
struct device_attribute *attr,char *buf)+{
- struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
- return cti_reg32_show(dev, buf, &drvdata->config.ctiappset, -1);
+}
+static ssize_t appset_store(struct device *dev,
struct device_attribute *attr,const char *buf, size_t size)+{
- struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
- return cti_reg32_store(dev, buf, size,
&drvdata->config.ctiappset, CTIAPPSET);+} +static DEVICE_ATTR_RW(appset);
---- End here ---
You may be able to use a macro reduce the code a bit: something like:
#define cti_config_ reg32_rw(name, cfg, offset) \ static ssize_t #name_show(struct device *dev, \ struct device_attribute *attr, \ char *buf) \ { \ struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); \ \ return cti_reg32_show(dev, buf, &drvdata->config.##cfg, -1); \ } \ \ static ssize_t asicctl_store(struct device *dev, \ struct device_attribute *attr, \ const char *buf, size_t size) \ { \ struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); \ \ return cti_reg32_store(dev, buf, size, \ &drvdata->config.##cfg, offset); \ } \ static DEVICE_ATTR_RW(name)
cti_config_reg32_rw(appset, ctiappset, CTIAPPSET); cti_config_reg32_rw(asicctl, asicctl, ASICCTL); cti_config_reg32_rw(gate, ctigate, CTIGATE);
Macros implemented here and a couple of other places.
+static ssize_t intack_store(struct device *dev,
struct device_attribute *attr,const char *buf, size_t size)+{
unsigned long val;if (kstrtoul(buf, 16, &val))s/16/0
return -EINVAL;cti_write_intack(dev, val);return size;+} +static DEVICE_ATTR_WO(intack);
+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, 16, &val))s/16/0
return -EINVAL;spin_lock(&drvdata->spinlock);/* a 1'b1 in appclr clears down the same bit in appset*/config->ctiappset &= ~val;/* write through if enabled */if (CTI_PWR_ENA(config))cti_write_single_reg(drvdata, CTIAPPCLEAR, val);Should we return an error if the device wasn't turned on ?
No - if the device is not on then just the cached values are updated. Once the device is turned on then whatever the current value is will be applied to the system. This allows programming either before enable, while powered down, or while enabled. Unlike the ETM CTIs can be altered while live (appset / appclear are designed for just that).
spin_unlock(&drvdata->spinlock);return size;+} +static DEVICE_ATTR_WO(appclear);
+static ssize_t apppulse_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, 16, &val))return -EINVAL;s/16/0
spin_lock(&drvdata->spinlock);/** a 1'b1 in apppulse sets then clears the bit,* effectively clears down the same bit in appset*/config->ctiappset &= ~val;/* write through if enabled */if (CTI_PWR_ENA(config))cti_write_single_reg(drvdata, CTIAPPPULSE, val);spin_unlock(&drvdata->spinlock);return size;+} +static DEVICE_ATTR_WO(apppulse);
+/*
- #define CTI_DEBUG_INTEGRATION_CTRL to enable the access to the integration
- control registers. Normally only used to investigate connection data.
- */
+// #define CTI_DEBUG_INTEGRATION_CTRL
+#ifdef CTI_DEBUG_INTEGRATION_CTRL
+static ssize_t itchout_show(struct device *dev,
struct device_attribute *attr,char *buf)+{
return cti_reg32_show(dev, buf, NULL, ITCHOUT);+}
+static ssize_t itchout_store(struct device *dev,
struct device_attribute *attr,const char *buf, size_t size)+{
return cti_reg32_store(dev, buf, size, NULL, ITCHOUT);+} +static DEVICE_ATTR_RW(itchout);
+static ssize_t ittrigout_show(struct device *dev,
struct device_attribute *attr,char *buf)+{
return cti_reg32_show(dev, buf, NULL, ITTRIGOUT);+}
+static ssize_t ittrigout_store(struct device *dev,
struct device_attribute *attr,const char *buf, size_t size)+{
return cti_reg32_store(dev, buf, size, NULL, ITTRIGOUT);+} +static DEVICE_ATTR_RW(ittrigout);
+static ssize_t itchinack_store(struct device *dev,
struct device_attribute *attr,const char *buf, size_t size)+{
return cti_reg32_store(dev, buf, size, NULL, ITCHINACK);+} +static DEVICE_ATTR_WO(itchinack);
+static ssize_t ittriginack_store(struct device *dev,
struct device_attribute *attr,const char *buf, size_t size)+{
return cti_reg32_store(dev, buf, size, NULL, ITTRIGINACK);+} +static DEVICE_ATTR_WO(ittriginack);
+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);
Please use a macro to reduce the code, similar to above. Say: cti_reg32_rw(name, offset)
+coresight_cti_reg(ittrigin, ITTRIGIN); +coresight_cti_reg(itchin, ITCHIN); +coresight_cti_reg(itchoutack, ITCHOUTACK); +coresight_cti_reg(ittrigoutack, ITTRIGOUTACK);
+#endif /* CTI_DEBUG_INTEGRATION_CTRL */
+coresight_cti_reg(triginstatus, CTITRIGINSTATUS); +coresight_cti_reg(trigoutstatus, CTITRIGOUTSTATUS); +coresight_cti_reg(chinstatus, CTICHINSTATUS); +coresight_cti_reg(choutstatus, CTICHOUTSTATUS);
+static struct attribute *coresight_cti_regs_attrs[] = {
&dev_attr_inout_sel.attr,&dev_attr_inen.attr,&dev_attr_outen.attr,&dev_attr_gate.attr,&dev_attr_asicctl.attr,&dev_attr_intack.attr,&dev_attr_appset.attr,&dev_attr_appclear.attr,&dev_attr_apppulse.attr,&dev_attr_triginstatus.attr,&dev_attr_trigoutstatus.attr,&dev_attr_chinstatus.attr,&dev_attr_choutstatus.attr,+#ifdef CTI_DEBUG_INTEGRATION_CTRL
&dev_attr_itctrl.attr,&dev_attr_ittrigin.attr,&dev_attr_itchin.attr,&dev_attr_ittrigout.attr,&dev_attr_itchout.attr,&dev_attr_itchoutack.attr,&dev_attr_ittrigoutack.attr,&dev_attr_ittriginack.attr,&dev_attr_itchinack.attr,+#endif
NULL,+};
+/* sysfs groups */ static const struct attribute_group coresight_cti_group = { .attrs = coresight_cti_attrs, }; @@ -124,8 +520,14 @@ static const struct attribute_group coresight_cti_mgmt_group = { .name = "mgmt", };
+static const struct attribute_group coresight_cti_regs_group = {
.attrs = coresight_cti_regs_attrs,.name = "regs",+};
- const struct attribute_group *coresight_cti_groups[] = { &coresight_cti_group, &coresight_cti_mgmt_group,
&coresight_cti_regs_group, NULL,diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c index 4b3a5a91c4ff..cd804b29df07 100644 --- a/drivers/hwtracing/coresight/coresight-cti.c +++ b/drivers/hwtracing/coresight/coresight-cti.c @@ -158,6 +158,25 @@ static int cti_disable_hw(struct cti_drvdata *drvdata, bool update_refcount) return 0; }
+void cti_write_single_reg(struct cti_drvdata *drvdata, int offset, u32 value) +{
CS_UNLOCK(drvdata->base);Don't we need to check if we have claimed the device before wrting something here ? Or at least make sure that the caller has claimed it ?
This is always called from a context we know the device is enabled and powered. Claim tag regs are checked on enable, and enable will fail if external debugger has the device.
writel_relaxed(value, drvdata->base+offset);nit: coding style.
CS_LOCK(drvdata->base);+}
+void cti_write_intack(struct device *dev, u32 ackval) +{
struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);struct cti_config *config = &drvdata->config;spin_lock(&drvdata->spinlock);/* write if enabled */if (CTI_PWR_ENA(config))cti_write_single_reg(drvdata, CTIINTACK, ackval);spin_unlock(&drvdata->spinlock);+}
- static void cti_set_default_config(struct device *dev, struct cti_drvdata *drvdata) {
diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h index bc4c9bdf745e..215b4a4ac45d 100644 --- a/drivers/hwtracing/coresight/coresight-cti.h +++ b/drivers/hwtracing/coresight/coresight-cti.h @@ -205,7 +205,13 @@ extern const struct attribute_group *coresight_cti_groups[]; int cti_add_default_connection(struct cti_drvdata *drvdata); int cti_enable(struct coresight_device *csdev, void *__unused); int cti_disable(struct coresight_device *csdev, void *__unused); +void cti_write_intack(struct device *dev, u32 ackval); +void cti_write_single_reg(struct cti_drvdata *drvdata, int offset, u32 value);
- struct coresight_platform_data * coresight_cti_get_platform_data(struct device *dev);
+/* cti powered and enabled */ +#define CTI_PWR_ENA(p_cfg) (p_cfg->hw_enabled && p_cfg->hw_powered)
Please could we be more explicit with the name, say cti_available() ?
'available' is less explicit & coding style requires CAPS for macros. There are some cases - e.g. the management registers - where we need to read before the device is enabled - which just use the powered test.
Cheers Suzuki
Thanks for the feedback.
Mike
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK