Re: [RFC PATCH v4 04/15] coresight: cti: Add sysfs trigger / channel programming API

Hi Mike,

Looks good overall. I have some comments about how we mix integer and string output for write/read for sysfs handles.

On 19/08/2019 22:13, Mike Leach wrote:

Adds a user API to allow programming of CTI by trigger ID and channel number. This will take the channel and trigger ID supplied by the user and program the appropriate register values.

Signed-off-by: Mike Leach mike.leach@linaro.org

.../hwtracing/coresight/coresight-cti-sysfs.c | 355 ++++++++++++++++++ drivers/hwtracing/coresight/coresight-cti.c | 157 +++++++- drivers/hwtracing/coresight/coresight-cti.h | 34 ++ 3 files changed, 545 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c index 4055e392530d..dbbfd1de58b3 100644 --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c @@ -480,6 +480,355 @@ static struct attribute *coresight_cti_regs_attrs[] = { NULL, }; +/* 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;

nit: If we set this to -EINVAL, you could remove the else case below. We can overwrite the err with size only when we have parsed properly.

• /* extract chan idx and trigger idx */
• items = sscanf(buf, "%d %d", &chan_idx, &trig_idx);
• if (items) {

To make sure we have got two values parsed, the above check must be :

if (items == 2) {

• err = cti_channel_trig_op(dev, op, dir, chan_idx, trig_idx);
  

• if (!err)
  

• 	err = size;
  

• } else

• err = -EINVAL;
  

• return err;

+}

+static ssize_t gate_enable_store(struct device *dev,

• 		 struct device_attribute *attr,
  

• 		 const char *buf, size_t size)
  

+{

• int err = 0, channel = 0;
• if ((size >= 3) && (strncmp(buf, "all", 3) == 0)) {

Could we make it strict by making this (size == 4) ? But more on that below.

• err = cti_channel_gate_op(dev, CTI_GATE_CHAN_ENABLE_ALL, 0);
  

• } else {

• if (kstrtoint(buf, 0, &channel))
  

• 	return -EINVAL;
  

• err = cti_channel_gate_op(dev, CTI_GATE_CHAN_ENABLE, channel);
  

• }

In general I am a bit concerned over mixing string and integer as input for an ABI. I don't have a perfect solution for "all" option, other than using -1 ! What do others think ?

• return err ? err : size;

+} +static DEVICE_ATTR_WO(gate_enable);

+static ssize_t gate_disable_store(struct device *dev,

• 		  struct device_attribute *attr,
  

• 		  const char *buf, size_t size)
  

+{

• int err = 0, channel = 0;
• if ((size >= 3) && (strncmp(buf, "all", 3) == 0)) {

• err = cti_channel_gate_op(dev, CTI_GATE_CHAN_DISABLE_ALL, 0);
  

• } else {

• if (kstrtoint(buf, 0, &channel))
  

• 	return -EINVAL;
  

• err = cti_channel_gate_op(dev, CTI_GATE_CHAN_DISABLE, channel);
  

• }
• return err ? err : size;

+} +static DEVICE_ATTR_WO(gate_disable);

+static ssize_t trig_filter_enable_show(struct device *dev,

• 		       struct device_attribute *attr,
  

• 		       char *buf)
  

+{

• unsigned long val;
• struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
• spin_lock(&drvdata->spinlock);
• val = drvdata->config.trig_filter_enable;
• spin_unlock(&drvdata->spinlock);
• return scnprintf(buf, PAGE_SIZE, "%s\n", val ? "enabled" : "disabled");

nit: I am not sure if "enabled"/"disabled" is preferred over 1/0. We follow the latter for enable_sink/source etc. Also for the "store" below for this field. So it may be a good idea to keep this consistent with the rest. e.g this might be more helpful for scripting.

+/* for the selected channel - show the signals attached. */ +static ssize_t show_chan_xtrigs_show(struct device *dev,

• 		     struct device_attribute *attr,
  

• 		     char *buf)
  

+{

• struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
• struct cti_config *cfg = &drvdata->config;
• int used = 0, reg_idx;
• int buf_sz = PAGE_SIZE;
• u32 chan_mask = BIT(cfg->xtrig_rchan_sel);
• used += scnprintf(buf, buf_sz, "IN: ");
• for (reg_idx = 0;

•     reg_idx < drvdata->config.nr_trig_max;
  

To be on the safer side shouldn't we check we have enough space left in the buffer ?

i.e reg_idx < drvdata->config.nr_trig_mmax && used < buf_sz ?

and similar check everywhere below ?

•     reg_idx++) {
  

• if (chan_mask & cfg->ctiinen[reg_idx]) {
  

• 	used += scnprintf(buf + used, buf_sz - used, "%d ",
  

• 			  reg_idx);
  

• }
  

• }
• used += scnprintf(buf + used, buf_sz - used, "OUT: ");
• for (reg_idx = 0;

•     reg_idx < drvdata->config.nr_trig_max;
  

•     reg_idx++) {
  

• if (chan_mask & cfg->ctiouten[reg_idx]) {
  

• 	used += scnprintf(buf + used, buf_sz - used, "%d ",
  

• 			  reg_idx);
  

• }
  

• }
• used += scnprintf(buf + used, buf_sz - used, "\n");
• return used;

+} +static DEVICE_ATTR_RO(show_chan_xtrigs);

+static ssize_t print_chan_list(struct device *dev,

• 	       char *buf, bool inuse)
  

+{

• struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
• struct cti_config *config = &drvdata->config;
• int size, i;
• unsigned long inuse_bits = 0, chan_mask;
• /* scan regs to get bitmap of channels in use. */
• spin_lock(&drvdata->spinlock);
• for (i = 0; i < config->nr_trig_max; i++) {

• inuse_bits |= config->ctiinen[i];
  

• inuse_bits |= config->ctiouten[i];
  

• }
• spin_unlock(&drvdata->spinlock);
• /* inverse bits if printing free channels */
• if (!inuse)

• inuse_bits = ~inuse_bits;
  

• /* list of channels, or 'none' */
• chan_mask = GENMASK(config->nr_ctm_channels - 1, 0);
• if (inuse_bits & chan_mask)

• size = bitmap_print_to_pagebuf(true, buf, &inuse_bits,
  

• 			       config->nr_ctm_channels);
  

• else

• size = scnprintf(buf, PAGE_SIZE, "none\n");
  

Could we keep this empty rather than making "none" ? This could make the parsing easier and consistent (for scripts).

• return size;

+}

+static ssize_t list_inuse_show(struct device *dev,

• 	       struct device_attribute *attr,
  

• 	       char *buf)
  

+{

• return print_chan_list(dev, buf, true);

+} +static DEVICE_ATTR_RO(list_inuse);

+static ssize_t list_free_show(struct device *dev,

• 	 struct device_attribute *attr,
  

• 	 char *buf)
  

+{

• return print_chan_list(dev, buf, false);

+} +static DEVICE_ATTR_RO(list_free);

+static ssize_t list_gate_enable_show(struct device *dev,

• 		     struct device_attribute *attr,
  

• 		     char *buf)
  

+{

• struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
• struct cti_config *cfg = &drvdata->config;
• unsigned long ctigate_bitmask = cfg->ctigate;
• int size = 0;
• if (cfg->ctigate == 0)

• size = scnprintf(buf, PAGE_SIZE, "none\n");
  

same here, could we keep this empty ?

• else

• size = bitmap_print_to_pagebuf(true, buf, &ctigate_bitmask,
  

• 			       cfg->nr_ctm_channels);
  

• return size;

+} +static DEVICE_ATTR_RO(list_gate_enable);

const struct attribute_group *coresight_cti_groups[] = { &coresight_cti_group, &coresight_cti_mgmt_group, &coresight_cti_regs_group,

• &coresight_cti_channels_group, NULL, };

diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c index 0368fa7c6179..9b0441d8be61 100644 --- a/drivers/hwtracing/coresight/coresight-cti.c +++ b/drivers/hwtracing/coresight/coresight-cti.c @@ -36,7 +36,7 @@ static DEFINE_MUTEX(ect_mutex); DEFINE_CORESIGHT_DEVLIST(cti_sys_devs, "cti_sys"); /* write set of regs to hardware - call with spinlock claimed */ -static void cti_write_all_hw_regs(struct cti_drvdata *drvdata) +void cti_write_all_hw_regs(struct cti_drvdata *drvdata)

What triggers this change in this patch ? Seems like an unrelated change w.r.t the patch.

{ struct cti_config *config = &drvdata->config; int i; @@ -295,6 +295,161 @@ int cti_add_default_connection(struct device *dev, struct cti_drvdata *drvdata) return ret; } +/** cti channel api **/ +/* attach/detach channel from trigger - write through if enabled. */ +int cti_channel_trig_op(struct device *dev, enum cti_chan_op op,

• 	enum cti_trig_dir direction, u32 channel_idx,
  

• 	u32 trigger_idx)
  

+{

• struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
• struct cti_config *config = &drvdata->config;
• u32 trig_bitmask;
• u32 chan_bitmask;
• u32 reg_value;
• int reg_offset;
• /* ensure indexes in range */
• if ((channel_idx >= config->nr_ctm_channels) ||

•   (trigger_idx >= config->nr_trig_max))
  

• return -EINVAL;
  

• trig_bitmask = BIT(trigger_idx);
• /* ensure registered triggers and not out filtered */
• if (direction == CTI_TRIG_IN) {

• if (!(trig_bitmask & config->trig_in_use))
  

• 	return -EINVAL;
  

• } else {

• if (!(trig_bitmask & config->trig_out_use))
  

• 	return -EINVAL;
  

• if ((config->trig_filter_enable) &&
  

•     (config->trig_out_filter & trig_bitmask))
  

• 	return -EINVAL;
  

• }
• /* update the local register values */
• chan_bitmask = BIT(channel_idx);
• reg_offset = (direction == CTI_TRIG_IN ? CTIINEN(trigger_idx) :

•       CTIOUTEN(trigger_idx));
  

• spin_lock(&drvdata->spinlock);
• /* read - modify write - the trigger / channel enable value */
• reg_value = direction == CTI_TRIG_IN ? config->ctiinen[trigger_idx] :

•      config->ctiouten[trigger_idx];
  

• if (op == CTI_CHAN_ATTACH)

• reg_value |= chan_bitmask;
  

• else

• reg_value &= ~chan_bitmask;
  

• /* write local copy */
• if (direction == CTI_TRIG_IN)

• config->ctiinen[trigger_idx] = reg_value;
  

• else

• config->ctiouten[trigger_idx] = reg_value;
  

• /* write through if enabled */
• if (CTI_PWR_ENA(config))

• cti_write_single_reg(drvdata, reg_offset, reg_value);
  

• spin_unlock(&drvdata->spinlock);
• return 0;

+}

+int cti_channel_gate_op(struct device *dev, enum cti_chan_gate_op op,

• 	u32 channel_idx)
  

+{

• struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
• struct cti_config *config = &drvdata->config;
• u32 chan_bitmask;
• u32 reg_value;
• int err = 0;
• if (channel_idx >= config->nr_ctm_channels)

• return -EINVAL;
  

• chan_bitmask = BIT(channel_idx);
• spin_lock(&drvdata->spinlock);
• reg_value = config->ctigate;
• switch (op) {
• case CTI_GATE_CHAN_ENABLE:

• reg_value |= chan_bitmask;
  

• break;
  

• case CTI_GATE_CHAN_DISABLE:

• reg_value &= ~chan_bitmask;
  

• break;
  

• case CTI_GATE_CHAN_ENABLE_ALL:

• reg_value = GENMASK(config->nr_ctm_channels - 1, 0);
  

• break;
  

• case CTI_GATE_CHAN_DISABLE_ALL:

• reg_value = 0x0;
  

• break;
  

• default:

• err = -EINVAL;
  

• break;
  

• }
• if (err == 0) {

• config->ctigate = reg_value;
  

• if (CTI_PWR_ENA(config))
  

• 	cti_write_single_reg(drvdata, CTIGATE, reg_value);
  

• }
• spin_unlock(&drvdata->spinlock);
• return err;

+}

+int cti_channel_setop(struct device *dev, enum cti_chan_set_op op,

•       u32 channel_idx)
  

+{

• struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
• struct cti_config *config = &drvdata->config;
• u32 chan_bitmask;
• u32 reg_value;
• u32 reg_offset;
• int err = 0;
• if (channel_idx >= config->nr_ctm_channels)

• return -EINVAL;
  

• chan_bitmask = BIT(channel_idx);
• spin_lock(&drvdata->spinlock);
• reg_value = config->ctiappset;
• switch (op) {
• case CTI_CHAN_SET:

• config->ctiappset |= chan_bitmask;
  

• reg_value  = config->ctiappset;
  

• reg_offset = CTIAPPSET;
  

• break;
  

• case CTI_CHAN_CLR:

• config->ctiappset &= ~chan_bitmask;
  

• reg_value = chan_bitmask;
  

• reg_offset = CTIAPPCLEAR;
  

• break;
  

• case CTI_CHAN_PULSE:

• config->ctiappset &= ~chan_bitmask;
  

• reg_value = chan_bitmask;
  

• reg_offset = CTIAPPPULSE;
  

• break;
  

• default:

• err = -EINVAL;
  

• break;
  

• }
• if ((err == 0) && CTI_PWR_ENA(config))

• cti_write_single_reg(drvdata, reg_offset, reg_value);
  

• spin_unlock(&drvdata->spinlock);
• return err;

+}

• /** cti ect operations **/ int cti_enable(struct coresight_device *csdev) {

diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h index db8667a2691c..865d3d0ffb11 100644 --- a/drivers/hwtracing/coresight/coresight-cti.h +++ b/drivers/hwtracing/coresight/coresight-cti.h @@ -174,6 +174,32 @@ struct cti_drvdata { void (*csdev_release)(struct device *dev); }; +/*

• • Channel operation types.
• */

+enum cti_chan_op {

• CTI_CHAN_ATTACH,
• CTI_CHAN_DETACH,

+};

+enum cti_trig_dir {

• CTI_TRIG_IN,
• CTI_TRIG_OUT,

+};

+enum cti_chan_gate_op {

• CTI_GATE_CHAN_ENABLE,
• CTI_GATE_CHAN_DISABLE,
• CTI_GATE_CHAN_ENABLE_ALL,
• CTI_GATE_CHAN_DISABLE_ALL,

+};

+enum cti_chan_set_op {

• CTI_CHAN_SET,
• CTI_CHAN_CLR,
• CTI_CHAN_PULSE,

+};

• /* private cti driver fns & vars */ extern const struct attribute_group *coresight_cti_groups[]; int cti_add_default_connection(struct device *dev,

@@ -186,8 +212,16 @@ struct cti_trig_con *cti_allocate_trig_con(struct device *dev, int in_sigs, int out_sigs); int cti_enable(struct coresight_device *csdev); int cti_disable(struct coresight_device *csdev); +void cti_write_all_hw_regs(struct cti_drvdata *drvdata); void cti_write_intack(struct device *dev, u32 ackval); void cti_write_single_reg(struct cti_drvdata *drvdata, int offset, u32 value); +int cti_channel_trig_op(struct device *dev, enum cti_chan_op op,

• 	enum cti_trig_dir direction, u32 channel_idx,
  

• 	u32 trigger_idx);
  

+int cti_channel_gate_op(struct device *dev, enum cti_chan_gate_op op,

• 	u32 channel_idx);
  

+int cti_channel_setop(struct device *dev, enum cti_chan_set_op op,

•       u32 channel_idx);
  

  struct coresight_platform_data * coresight_cti_get_platform_data(struct device *dev);

Suzuki