HI Suzuki,
On Mon, 17 Jun 2019 at 12:26, Suzuki K Poulose suzuki.poulose@arm.com wrote:
Hi Mike,
On 05/06/2019 20:03, 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.
This patch looks fine to me. Some minor comments/suggestsions below.
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,
..
+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);
Should we be strict about specifying "all" precisely ? i.e, the above code now matches "all*", isn't it ? Given you have made sure the (size >= 3), we could use strcmp() instead of strncmp() to match it precisely.
} 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);
same comment as above.
} else {
if (kstrtoint(buf, 0, &channel))
return -EINVAL;
err = cti_channel_gate_op(dev, CTI_GATE_CHAN_DISABLE, channel);
}
return err ? err : size;
+}
+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");
minor nit: Since we only store 1 or 0, for filter_enable, may be you could skip the integer part.
Agreed - this is now a bool
+}
+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;
minor nit: You may also use : drvdata->config.trig_filter_enable = !!val;
spin_unlock(&drvdata->spinlock);
return size;
+}
+/* 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))
nit : missing space.
not sure where - checkpatch.pl normally gets annoyed about these things and it didn't trigger.
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: ");
nit : You may reuse bitmap_print_to_pagebuf() for printing bitmask to their indices list.
No, this will not work. we are comparing the chan_mask bit with multiple enable register bitmaps and printing per match.
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);
nit: missing space.
}
}
used += scnprintf(buf+used, buf_sz-used, "OUT: ");
nit: Do we need a new line before OUT ?
No - previous patches were rejected for multi-line sysfs output./
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);
...
}
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;
GENMASK ?
OK
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) {
Please reuse bitmap_to_pagebuf().
+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);
}
}
As above, use bitmap_to_pagebuf(), or open code scnprintf() using %pb[l] modifier.
used += scnprintf(buf+used, b_sz-used, "\n");
}
return used;
+} +static DEVICE_ATTR_RO(list_gate_ena);
nit: Please use clear/obvious names. does it stand for "enabled" ?
OK
+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,
+/** 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 = 0x1 << trigger_idx;
Use BIT(trigger_idx) instead everywhere ?
/* 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;
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]);
nit: you may drop the parantheses ^^
reg_value = (op == CTI_CHAN_ATTACH) ? reg_value | chan_bitmask :
reg_value & (~chan_bitmask);
I think this statement would be better readable with an if..else. i.e if (op == CTI_CHAN_ATTACH) reg_value |= chan_bitmask; else reg_valu &= ~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 = 0x1 << channel_idx;
nit: As above, if you are only tracking a single idx in a mask, you may use BIT(idx) everywhere to make it more obvious.
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;
GENMASK()
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)
+{
chan_bitmask = 0x1 << channel_idx;
BIT()
return err;
+}
Suzuki
BIT & GENMASK used as suggested
Thanks
Mike
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK