Re: [PATCH 6/8] coresight: etm4x: Add view comparator settings API to sysfs.

Hi Mike,

On Mon, Aug 19, 2019 at 09:57:18PM +0100, Mike Leach wrote:

Currently it is not possible to view the current settings of a given address comparator without knowing what type it is set to. For example, if a comparator is set as an addr_start comparator, attempting to read addr_stop for the same index will result in an error.

addr_cmp_view is added to allow the user to see the current settings of the indexed address comparator without resorting to trail and error when the set type is not known.

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

.../coresight/coresight-etm4x-sysfs.c | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c index baac5b48b7ac..483976074779 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c @@ -1272,6 +1272,56 @@ static ssize_t addr_exlevel_s_ns_store(struct device *dev, } static DEVICE_ATTR_RW(addr_exlevel_s_ns); +static const char * const addr_type_names[] = {

• "unused",
• "single",
• "range",
• "start",
• "stop"

+};

+static ssize_t addr_cmp_view_show(struct device *dev,

• 		  struct device_attribute *attr, char *buf)
  

+{

• u8 idx, addr_type;
• unsigned long addr_v, addr_v2, addr_ctrl;

Some nitpicks, if you disagree, just ignore them :)

nitpicks: maybe we can use 'addr_v1' and 'addr_v2', we even can use 'addr_l' and 'addr_h'.

• struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent);
• struct etmv4_config *config = &drvdata->config;
• int size = 0;
• bool exclude = false;
• spin_lock(&drvdata->spinlock);
• idx = config->addr_idx;
• addr_v = config->addr_val[idx];
• addr_ctrl = config->addr_acc[idx];
• addr_type = config->addr_type[idx];
• if (addr_type == ETM_ADDR_TYPE_RANGE) {

• if (idx%2) {
  

if (idx & 0x1)

• 	idx -= 1;
  

• 	addr_v2 = addr_v;
  

• 	addr_v = config->addr_val[idx];
  

• } else
  

• 	addr_v2 = config->addr_val[idx+1];
  

The code style doc [1] suggests to use braces for 'else' as well.

• exclude = config->viiectlr & BIT(idx / 2 + 16);
  

• }
• spin_unlock(&drvdata->spinlock);
• if (addr_type) {

• size = scnprintf(buf, PAGE_SIZE, "addr_cmp[%i] %s %#lx", idx,
  

• 		 addr_type_names[addr_type], addr_v);
  

• if (addr_type == ETM_ADDR_TYPE_RANGE) {
  

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

s/buf+size/buf + size s/PAGE_SIZE-size/PAGE_SIZE - size

• 			  " %#lx %s", addr_v2,
  

• 			  exclude ? "exclude" : "include");
  

• }
  

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

s/buf+size/buf + size

This patch logic is clear for me, so you could add my review tag:

Reviewed-by: Leo Yan leo.yan@linaro.org

Thanks, Leo Yan

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu...

• 		  " ctrl(%#lx)\n", addr_ctrl);
  

• } else {

• size = scnprintf(buf, PAGE_SIZE, "addr_cmp[%i] unused\n", idx);
  

• }
• return size;

+} +static DEVICE_ATTR_RO(addr_cmp_view);

static ssize_t vinst_pe_cmp_start_stop_show(struct device *dev, struct device_attribute *attr, char *buf) @@ -2117,6 +2167,7 @@ static struct attribute *coresight_etmv4_attrs[] = { &dev_attr_addr_ctxtype.attr, &dev_attr_addr_context.attr, &dev_attr_addr_exlevel_s_ns.attr,

• &dev_attr_addr_cmp_view.attr, &dev_attr_vinst_pe_cmp_start_stop.attr, &dev_attr_seq_idx.attr, &dev_attr_seq_state.attr,

-- 2.17.1

---

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