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

Hi Mike,

On Wed, Jun 05, 2019 at 08:03:01PM +0100, 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 | 371 ++++++++++++++++++ drivers/hwtracing/coresight/coresight-cti.c | 159 +++++++- drivers/hwtracing/coresight/coresight-cti.h | 34 ++ 3 files changed, 563 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c index caecf011f6e0..e1eacd2b0f6e 100644 --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c @@ -510,6 +510,371 @@ 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;
• /* extract chan idx and trigger idx */
• items = sscanf(buf, "%d %d", &chan_idx, &trig_idx);
• if (items) {

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

• if (!err)
  

• 	err = size;
  

• } else

• err = -EINVAL;
  

• return err;

+}

+static ssize_t trigin_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_IN,

• 		 buf, size);
  

+} +static DEVICE_ATTR_WO(trigin_attach);

+static ssize_t trigin_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_IN,

• 		 buf, size);
  

+} +static DEVICE_ATTR_WO(trigin_detach);

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

+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)) {

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

• }
• 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 int +chan_op_parse(struct device *dev, enum cti_chan_set_op op, const char *buf) +{

• int err = 0, channel = 0;
• if (kstrtoint(buf, 0, &channel))

• return -EINVAL;
  

• err = cti_channel_setop(dev, op, channel);
• return err;

+}

+static ssize_t chan_set_store(struct device *dev,

• 	      struct device_attribute *attr,
  

• 	      const char *buf, size_t size)
  

+{

• int err = chan_op_parse(dev, CTI_CHAN_SET, buf);
• return err ? err : size;

+} +static DEVICE_ATTR_WO(chan_set);

+static ssize_t chan_clear_store(struct device *dev,

• 		struct device_attribute *attr,
  

• 		const char *buf, size_t size)
  

+{

• int err = chan_op_parse(dev, CTI_CHAN_CLR, buf);
• return err ? err : size;

+} +static DEVICE_ATTR_WO(chan_clear);

+static ssize_t chan_pulse_store(struct device *dev,

• 		struct device_attribute *attr,
  

• 		const char *buf, size_t size)
  

+{

• int err = chan_op_parse(dev, CTI_CHAN_PULSE, buf);
• return err ? err : size;

+} +static DEVICE_ATTR_WO(chan_pulse);

+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, "%ld (%s)\n", val,

• 	 val ? "enabled" : "disabled");
  

+}

+static ssize_t trig_filter_enable_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))

• return -EINVAL;
  

• spin_lock(&drvdata->spinlock);
• drvdata->config.trig_filter_enable = val ? 1 : 0;

In patch 1/16 I was wondering whey config.trig_filter_enable was an 'int' and I still do.

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

+} +static DEVICE_ATTR_RW(trig_filter_enable);

+/* clear all xtrigger / channel programming */ +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;

GENMASK()

• config->asicctl = 0;
• config->ctiappset = 0;
• config->ctiinout_sel = 0;
• config->xtrig_rchan_sel = 0;
• /* if enabled then write through */
• if (CTI_PWR_ENA(config))

• cti_write_all_hw_regs(drvdata);
  

I am debating whether it is worth checking the claim tags before accessing the HW. CTI_PWR_ENA() is already checking ->hw_enabled, which can only happen if the claim tags have been acquired. The only thing it would do is interfere with an external agent that would have connected after we've claimed the tags, which would lead to all sort of failures anyway. The same applies to functions cti_channel_gate_op(), cti_channel_trig_op() and cti_channel_setop().

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

xtrig_rchan_sel is not documented in struct cti_config.

• 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, 10, &val))

• return -EINVAL;
  

• if (val > (drvdata->config.nr_ctm_channels-1))

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

+/* 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 = 0x1 << cfg->xtrig_rchan_sel;
• used += scnprintf(buf, buf_sz, "IN: ");
• for (reg_idx = 0;

•     reg_idx < drvdata->config.nr_trig_max;
  

•     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 used = 0, i;
• u32 inuse_bits = 0, chan_mask, chan_bit_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 = ((u32)(0x1 << config->nr_ctm_channels)) - 1;
• if (inuse_bits & chan_mask) {

• for (i = 0; i < config->nr_ctm_channels; i++) {
  

• 	chan_bit_mask = 0x1 << i;
  

• 	if (chan_bit_mask & inuse_bits) {
  

• 		used += scnprintf(buf+used, PAGE_SIZE-used,
  

• 				  "%d ", i);
  

• 	}
  

• }
  

• used += scnprintf(buf+used, PAGE_SIZE-used, "\n");
  

• } else {

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

• }
• return used;

+}

+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_ena_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 b_sz = PAGE_SIZE;
• u32 chan_mask;
• int chan, used = 0;
• if (cfg->ctigate == 0) {

• used += scnprintf(buf, b_sz, "none\n");
  

• } else {

• for (chan = 0; chan < cfg->nr_ctm_channels; chan++) {
  

• 	chan_mask = 0x1 << chan;
  

• 	if (chan_mask & cfg->ctigate) {
  

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

• 				  chan);
  

• 	}
  

• }
  

• used += scnprintf(buf+used, b_sz-used, "\n");
  

• }
• return used;

+} +static DEVICE_ATTR_RO(list_gate_ena);

+static struct attribute *coresight_cti_channel_attrs[] = {

• &dev_attr_trigin_attach.attr,
• &dev_attr_trigin_detach.attr,
• &dev_attr_trigout_attach.attr,
• &dev_attr_trigout_detach.attr,
• &dev_attr_gate_enable.attr,
• &dev_attr_gate_disable.attr,
• &dev_attr_chan_set.attr,
• &dev_attr_chan_clear.attr,
• &dev_attr_chan_pulse.attr,
• &dev_attr_trig_filter_enable.attr,
• &dev_attr_reset_xtrigs.attr,
• &dev_attr_show_chan_sel.attr,
• &dev_attr_show_chan_xtrigs.attr,
• &dev_attr_list_inuse.attr,
• &dev_attr_list_free.attr,
• &dev_attr_list_gate_ena.attr,
• NULL,

+};

/* sysfs groups */ static const struct attribute_group coresight_cti_group = { .attrs = coresight_cti_attrs, @@ -525,9 +890,15 @@ static const struct attribute_group coresight_cti_regs_group = { .name = "regs", }; +static const struct attribute_group coresight_cti_channels_group = {

• .attrs = coresight_cti_channel_attrs,
• .name = "channels",

+};

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 cd804b29df07..817b6f70ee23 100644 --- a/drivers/hwtracing/coresight/coresight-cti.c +++ b/drivers/hwtracing/coresight/coresight-cti.c @@ -42,7 +42,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) { struct cti_config *config = &drvdata->config; int i; @@ -314,6 +314,163 @@ int cti_add_default_connection(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)
  

Please rework the above parameter list to if as many in 80 characters. I know it doesn't make a different it will be flagged on the public mailing list.

+{

• 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 = 0x1 << 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 = 0x1 << 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]);
  

• reg_value = (op == CTI_CHAN_ATTACH) ? reg_value | chan_bitmask :

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

Here too.

+{

• 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 = 0x1 << 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 = (0x1 << config->nr_ctm_channels) - 1;
  

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

And here.

+{

• 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 = 0x1 << 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, void *__unused) { diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h index 215b4a4ac45d..33516bf378a8 100644 --- a/drivers/hwtracing/coresight/coresight-cti.h +++ b/drivers/hwtracing/coresight/coresight-cti.h @@ -200,13 +200,47 @@ struct cti_drvdata { struct cti_config config; }; +/*

• • 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 cti_drvdata *drvdata); int cti_enable(struct coresight_device *csdev, void *__unused); int cti_disable(struct coresight_device *csdev, void *__unused); +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); -- 2.20.1

---

CoreSight mailing list CoreSight@lists.linaro.org https://lists.linaro.org/mailman/listinfo/coresight