Re: [RFC PATCH v3 03/16] coresight: cti: Add sysfs access to program function regs

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 ?

+{

• 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.

• 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);

+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 ?

• 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 ?

• 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() ?

Cheers Suzuki