Hi Suzuki,

Thanks a lot for your code review.

I will update the patch with your suggestions in the next release.

On 9/15/2022 6:14 PM, Suzuki K Poulose wrote:

Hi Tao

On 08/09/2022 09:44, Tao Zhang wrote:

...

Read the DSB element size from the device tree. Set the register bit that controls the DSB element size of the corresponding port.

Signed-off-by: Tao Zhang quic_taozha@quicinc.com

drivers/hwtracing/coresight/coresight-tpda.c | 62 ++++++++++++++++++++++++++++   drivers/hwtracing/coresight/coresight-tpda.h |  4 ++   2 files changed, 66 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c index c8bbc75..76636a1 100644 --- a/drivers/hwtracing/coresight/coresight-tpda.c +++ b/drivers/hwtracing/coresight/coresight-tpda.c @@ -37,6 +37,15 @@ static void tpda_enable_port(struct tpda_drvdata *drvdata, int port)       u32 val;         val = readl_relaxed(drvdata->base + TPDA_Pn_CR(port)); +    /* +     * Configure aggregator port n DSB data set element size +     * Set the bit to 0 if the size is 32 +     * Set the bit to 1 if the size is 64 +     */ +    if (drvdata->dsb_esize[port] == 32) +        val &= ~TPDA_Pn_CR_DSBSIZE; +    else if (drvdata->dsb_esize[port] == 64) +        val |= TPDA_Pn_CR_DSBSIZE;       /* Enable the port */       val |= TPDA_Pn_CR_ENA;       writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port)); @@ -105,6 +114,55 @@ static const struct coresight_ops tpda_cs_ops = {       .link_ops    = &tpda_link_ops,   };   +static int tpda_parse_dsb(struct tpda_drvdata *drvdata) +{ +    int len, port, i; +    const __be32 *prop; +    struct device_node *node = drvdata->dev->of_node;

+    /* Read the size of DSB element */ +    prop = of_get_property(node, "qcom,dsb-elem-size", &len); +    if (prop) { +        len /= sizeof(__be32); +        /* +         * The read set of data is port and size, so the number of data +         * is a multiple of two. And the number of data will not exceed +         * two times that of the TPDA inpurts number. +         */ +        if (len < 2 || len >= (2 * TPDA_MAX_INPORTS) || len % 2 != 0) { +            dev_err(drvdata->dev, +                "Dataset DSB width entries are wrong\n"); +            return -EINVAL; +        }

+        for (i = 0; i < len; i++) {

Please could we be explicit here that we are dealing with 2 entries in an iteration. i.e,

for (i = 0; i < len; i += 2) {

...

+            port = be32_to_cpu(prop[i++]);

port = be32_to_cpu(prop[i]);

...

+            if (port >= TPDA_MAX_INPORTS) { +                dev_err(drvdata->dev, +                    "Wrong port specified for DSB\n"); +                return -EINVAL; +            } +            /* Set DSB element size for corresponding port to dsb_esize*/ +            drvdata->dsb_esize[port] = be32_to_cpu(prop[i]);

drvdata->dsb_esize[port] = be32_to_cpu(prop[i + 1]);

Sure, I will update this part of the code in the next release.

...

+        } +    }

+    return 0; +}

+static int tpda_parse_of_data(struct tpda_drvdata *drvdata) +{ +    int ret;

+    ret = tpda_parse_dsb(drvdata); +    if (ret) { +        dev_err(drvdata->dev, "Fail to get DSB data set element size\n"); +        return -EINVAL; +    }

+    return 0; +}

static int tpda_init_default_data(struct tpda_drvdata *drvdata)   {       int atid; @@ -148,6 +206,10 @@ static int tpda_probe(struct amba_device *adev, const struct amba_id *id)         spin_lock_init(&drvdata->spinlock);   +    ret = tpda_parse_of_data(drvdata); +    if (ret) +        return ret;

ret = tpda_init_default_data(drvdata);       if (ret)           return ret; diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/hwtracing/coresight/coresight-tpda.h index 4beb332..ecc7869 100644 --- a/drivers/hwtracing/coresight/coresight-tpda.h +++ b/drivers/hwtracing/coresight/coresight-tpda.h @@ -10,6 +10,8 @@   #define TPDA_Pn_CR(n)        (0x004 + (n * 4))   /* Aggregator port enable bit */   #define TPDA_Pn_CR_ENA        BIT(0) +/* Aggregator port DSB data set element size bit */ +#define TPDA_Pn_CR_DSBSIZE        BIT(8)     #define TPDA_MAX_INPORTS    32   @@ -23,6 +25,7 @@    * @csdev:      component vitals needed by the framework.    * @spinlock:   lock for the drvdata value.    * @enable:     enable status of the component.

• • @dsb_esize   DSB element size

super minor nit: Missing ":", consistent with the other fields.

I will update in the next release.

...

*/   struct tpda_drvdata {       void __iomem        *base; @@ -30,6 +33,7 @@ struct tpda_drvdata {       struct coresight_device    *csdev;       spinlock_t        spinlock;       u8            atid; +    u32            dsb_esize[TPDA_MAX_INPORTS];   };     #endif  /* _CORESIGHT_CORESIGHT_TPDA_H */

Suzuki

Regards,

Tao

Hi

On 08/09/2022 09:44, Tao Zhang wrote:

DSB subunit need to be configured in enablement and disablement. A struct that specifics associated to dsb dataset is needed. It saves the configuration and parameters of the dsb datasets. This change is to add this struct and initialize the configuration of DSB subunit.

Signed-off-by: Tao Zhang quic_taozha@quicinc.com

drivers/hwtracing/coresight/coresight-tpdm.c | 44 ++++++++++++++++++++++++++-- drivers/hwtracing/coresight/coresight-tpdm.h | 17 +++++++++++ 2 files changed, 58 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c index 88df3e6..69ea453 100644 --- a/drivers/hwtracing/coresight/coresight-tpdm.c +++ b/drivers/hwtracing/coresight/coresight-tpdm.c @@ -24,6 +24,22 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata) { u32 val;

• val = readl_relaxed(drvdata->base + TPDM_DSB_TIER);
• /* Set trigger timestamp */
• if (drvdata->dsb->trig_ts)

What happens if this instance doesn't have a DSB set ? Have you tested this on a system without the DSB ?

Suzuki

On 26/10/2022 09:10, Tao Zhang wrote:

Hi Suzuki,

在 10/24/2022 6:02 PM, Suzuki K Poulose 写道:

...

Hi

On 08/09/2022 09:44, Tao Zhang wrote:

...

DSB subunit need to be configured in enablement and disablement. A struct that specifics associated to dsb dataset is needed. It saves the configuration and parameters of the dsb datasets. This change is to add this struct and initialize the configuration of DSB subunit.

Signed-off-by: Tao Zhang quic_taozha@quicinc.com

drivers/hwtracing/coresight/coresight-tpdm.c | 44 ++++++++++++++++++++++++++--   drivers/hwtracing/coresight/coresight-tpdm.h | 17 +++++++++++   2 files changed, 58 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c index 88df3e6..69ea453 100644 --- a/drivers/hwtracing/coresight/coresight-tpdm.c +++ b/drivers/hwtracing/coresight/coresight-tpdm.c @@ -24,6 +24,22 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)   {       u32 val;   +    val = readl_relaxed(drvdata->base + TPDM_DSB_TIER); +    /* Set trigger timestamp */ +    if (drvdata->dsb->trig_ts)

What happens if this instance doesn't have a DSB set ? Have you tested this on a system without the DSB ?

The function "tpdm_enable_dsb" will only be called when it is checked that the DSB dataset is present.

And only the TPDM that supports the DSB dataset will have the DSB TIER register.

If the TPDM doesn't support the DSB dataset, this instance should not be run. Otherwise, it will cause that the incorrect register is accessed.

Thanks, this is what happens when you send something that is not queued anywhwere. Please provide a reference tree in the future, for ease of reviewing such things

Suzuki

On 08/09/2022 09:45, Tao Zhang wrote:

TPDM device need a node to reset the configurations and status of it. This change provides a node to reset the configurations and disable the TPDM if it has been enabled.

It is not clear to me *why* this is needed. Please could you elaborate on the use case of this ? See my questions below.

Signed-off-by: Tao Zhang quic_taozha@quicinc.com

drivers/hwtracing/coresight/coresight-tpdm.c | 32 ++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c index 69ea453..74cc653 100644 --- a/drivers/hwtracing/coresight/coresight-tpdm.c +++ b/drivers/hwtracing/coresight/coresight-tpdm.c @@ -152,6 +152,37 @@ static void tpdm_init_default_data(struct tpdm_drvdata *drvdata) } } +static ssize_t reset_store(struct device *dev,

• 			  struct device_attribute *attr,
  

• 			  const char *buf,
  

• 			  size_t size)
  

minor nit: Alignment

+{

• int ret = 0;
• unsigned long val;
• struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
• ret = kstrtoul(buf, 10, &val);

So, any integer value written to the sysfs knob triggers the rest ? It may be better to restrict this to "1".

• if (ret)

• return ret;
  

• spin_lock(&drvdata->spinlock);
• /* Reset all datasets to ZERO */
• if (drvdata->dsb != NULL)

• memset(drvdata->dsb, 0, sizeof(struct dsb_dataset));
  

• /* Init the default data */
• tpdm_init_default_data(drvdata);

Why is this needed ? Does the DSB device configuration change on the fly ?

• spin_unlock(&drvdata->spinlock);
• /* Disable tpdm if enabled */
• if (drvdata->enable)

• coresight_disable(drvdata->csdev);
  

Why is this needed ? Isn't this supposed to be triggered from the "path" when the trace session is complete ?

Suzuki

• return size;

+} +static DEVICE_ATTR_WO(reset);

• /*
  • value 1: 64 bits test data
  • value 2: 32 bits test data

@@ -192,6 +223,7 @@ static ssize_t integration_test_store(struct device *dev, static DEVICE_ATTR_WO(integration_test); static struct attribute *tpdm_attrs[] = {

• &dev_attr_reset.attr, &dev_attr_integration_test.attr, NULL, };

Hi Suzuki,

在 10/24/2022 6:32 PM, Suzuki K Poulose 写道:

On 08/09/2022 09:45, Tao Zhang wrote:

...

The nodes are needed to set or show the trigger timestamp and trigger type. This change is to add these nodes to achieve these function.

Signed-off-by: Tao Zhang quic_taozha@quicinc.com

drivers/hwtracing/coresight/coresight-tpdm.c | 90 ++++++++++++++++++++++++++++   1 file changed, 90 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c index 74cc653..fae9963 100644 --- a/drivers/hwtracing/coresight/coresight-tpdm.c +++ b/drivers/hwtracing/coresight/coresight-tpdm.c @@ -232,8 +232,98 @@ static struct attribute_group tpdm_attr_grp = {       .attrs = tpdm_attrs,   };   +static ssize_t dsb_trig_type_show(struct device *dev, +                     struct device_attribute *attr, +                     char *buf) +{ +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);

+    if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB)) +        return -EPERM;

+    return scnprintf(buf, PAGE_SIZE, "%u\n", +             (unsigned int)drvdata->dsb->trig_type); +}

+/*

• • value 0: set trigger type as enablement
• • value 1: set trigger type as disablement
• */

+static ssize_t dsb_trig_type_store(struct device *dev, +                      struct device_attribute *attr, +                      const char *buf, +                      size_t size) +{ +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); +    unsigned long val;

+    if ((kstrtoul(buf, 16, &val)) || val < 0 || val > 1) +        return -EINVAL; +    if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB)) +        return -EPERM;

+    spin_lock(&drvdata->spinlock); +    if (val) +        drvdata->dsb->trig_type = true; +    else +        drvdata->dsb->trig_type = false; +    spin_unlock(&drvdata->spinlock); +    return size; +} +static DEVICE_ATTR_RW(dsb_trig_type);

+static ssize_t dsb_trig_ts_show(struct device *dev, +                     struct device_attribute *attr, +                     char *buf) +{ +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);

+    if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB)) +        return -EPERM;

+    return scnprintf(buf, PAGE_SIZE, "%u\n", +             (unsigned int)drvdata->dsb->trig_ts); +}

+/*

• • value 0: set trigger timestamp as enablement
• • value 1: set trigger timestamp as disablement
• */

+static ssize_t dsb_trig_ts_store(struct device *dev, +                      struct device_attribute *attr, +                      const char *buf, +                      size_t size) +{ +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); +    unsigned long val;

+    if ((kstrtoul(buf, 16, &val)) || val < 0 || val > 1) +        return -EINVAL; +    if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB)) +        return -EPERM;

+    spin_lock(&drvdata->spinlock); +    if (val) +        drvdata->dsb->trig_ts = true; +    else +        drvdata->dsb->trig_ts = false; +    spin_unlock(&drvdata->spinlock); +    return size; +} +static DEVICE_ATTR_RW(dsb_trig_ts); +static struct attribute *tpdm_dsb_attrs[] = { +    &dev_attr_dsb_trig_ts.attr, +    &dev_attr_dsb_trig_type.attr, +    NULL, +};

+static struct attribute_group tpdm_dsb_attr_grp = { +    .attrs = tpdm_dsb_attrs, +};

Please could we hide the "DSB" related knobs, if the the Data sets are missing using is_visible() hook ?

Make sense, I will update this in the next patch series.

Suzuki

Best Regards Tao

On 08/09/2022 09:45, Tao Zhang wrote:

Add node to set and show programming mode for TPDM DSB subunit. Once the DSB programming mode is set, it will be written to the register DSB_CR. Bit[10:9] of the DSB_CR register is used to set the DSB test mode.

Signed-off-by: Tao Zhang quic_taozha@quicinc.com

drivers/hwtracing/coresight/coresight-tpdm.c | 49 +++++++++++++++++++++++++++- drivers/hwtracing/coresight/coresight-tpdm.h | 10 ++++++ 2 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c index fae9963..7265793 100644 --- a/drivers/hwtracing/coresight/coresight-tpdm.c +++ b/drivers/hwtracing/coresight/coresight-tpdm.c @@ -22,7 +22,7 @@ DEFINE_CORESIGHT_DEVLIST(tpdm_devs, "tpdm"); static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata) {

• u32 val;

• u32 val, mode;

val = readl_relaxed(drvdata->base + TPDM_DSB_TIER); /* Set trigger timestamp */ @@ -42,6 +42,19 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata) /* Set the enable bit of DSB control register to 1 */ val = readl_relaxed(drvdata->base + TPDM_DSB_CR);

• /* Set the cycle accurate mode */
• mode = TPDM_DSB_MODE_CYCACC(drvdata->dsb->mode);
• val = val & ~(0x7 << 9);
• val = val | (mode << 9);

Please do not hard code numbers like that above. Please could you define proper masks for the fields in DSB_CR and use FIELD_GET, FIELD_PREP for setting the values.

• /* Set the byte lane for high-performance mode */
• mode = TPDM_DSB_MODE_HPBYTESEL(drvdata->dsb->mode);
• val = val & ~(0x1F << 2);
• val = val | (mode << 2);
• /* Set the performance mode */

Same as above.

• if (drvdata->dsb->mode & TPDM_DSB_MODE_PERF)

• val |= TPDM_DSB_MODE;
  

• else

• val &= ~TPDM_DSB_MODE;
  

  val |= TPDM_DSB_CR_ENA; writel_relaxed(val, drvdata->base + TPDM_DSB_CR); }

@@ -232,6 +245,39 @@ static struct attribute_group tpdm_attr_grp = { .attrs = tpdm_attrs, }; +static ssize_t dsb_mode_show(struct device *dev,

• 		  struct device_attribute *attr,
  

• 		  char *buf)
  

+{

• struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
• if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB))

• return -EPERM;
  

As mentioned earlier, use is_visble() instead of hard coding this in every function.

• return scnprintf(buf, PAGE_SIZE, "%lx\n",

• 	 (unsigned long)drvdata->dsb->mode);
  

+}

+static ssize_t dsb_mode_store(struct device *dev,

• 		   struct device_attribute *attr,
  

• 		   const char *buf,
  

• 		   size_t size)
  

+{

• struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
• unsigned long val;
• if ((kstrtoul(buf, 16, &val)) || val < 0 || val > 3)

Why not leave kstrtoul to detect the base instead of always forcing 16 ? Some other code had forced to base of 10. Please be flexible and avoid the inconsistencies.

• return -EINVAL;
  

• if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB))

• return -EPERM;
  

• spin_lock(&drvdata->spinlock);
• drvdata->dsb->mode = val & TPDM_MODE_ALL;
• spin_unlock(&drvdata->spinlock);
• return size;

+} +static DEVICE_ATTR_RW(dsb_mode);

• static ssize_t dsb_trig_type_show(struct device *dev, struct device_attribute *attr, char *buf)

@@ -312,6 +358,7 @@ static ssize_t dsb_trig_ts_store(struct device *dev, } static DEVICE_ATTR_RW(dsb_trig_ts); static struct attribute *tpdm_dsb_attrs[] = {

• &dev_attr_dsb_mode.attr, &dev_attr_dsb_trig_ts.attr, &dev_attr_dsb_trig_type.attr, NULL,

diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h index dd4a013..4d57488 100644 --- a/drivers/hwtracing/coresight/coresight-tpdm.h +++ b/drivers/hwtracing/coresight/coresight-tpdm.h @@ -19,6 +19,14 @@ #define TPDM_DSB_XTRIG_TSENAB BIT(1) /* Enable bit for DSB subunit trigger type */ #define TPDM_DSB_TRIG_TYPE BIT(12) +/* Enable bit for DSB subunit perfmance mode */ +#define TPDM_DSB_MODE BIT(1)

+/* DSB programming modes */ +#define TPDM_DSB_MODE_CYCACC(val) BMVAL(val, 0, 2) +#define TPDM_DSB_MODE_PERF BIT(3) +#define TPDM_DSB_MODE_HPBYTESEL(val) BMVAL(val, 4, 8)

Please could we use GENMASK to define the masks and use FIELD_GET/FILED_PREP macros for the dealing with the fields ?

Similarly for the DSB_CR register please.

Suzuki

Hi Suzuki,

On 10/24/2022 11:47 PM, Suzuki K Poulose wrote:

On 08/09/2022 09:45, Tao Zhang wrote:

...

Add the nodes to set value for DSB edge control and DSB edge control mask. Each DSB subunit TPDM has n(n<16) EDCR resgisters to configure edge control. And each DSB subunit TPDM has m(m<8) ECDMR registers to configure edge control mask.

Please could you describe what these values represent ? Size of the "value". Without the specs, it is really difficult to comprehend the code.

Please add comments

The registers EDCR control DSB edge detection, and the following values should be writen to them. 00 - Rising edge detection 01 - Falling edge detection 10 - Rising and falling edge detection (toggle detection) The registers EDCMR are eight 32-bit registers, and they provide DSB interface edge detection mask control.

...

Signed-off-by: Tao Zhang quic_taozha@quicinc.com

drivers/hwtracing/coresight/coresight-tpdm.c | 130 ++++++++++++++++++++++++++-   drivers/hwtracing/coresight/coresight-tpdm.h |  11 +++   2 files changed, 140 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c index 7265793..14bcf2b 100644 --- a/drivers/hwtracing/coresight/coresight-tpdm.c +++ b/drivers/hwtracing/coresight/coresight-tpdm.c @@ -22,7 +22,14 @@ DEFINE_CORESIGHT_DEVLIST(tpdm_devs, "tpdm");     static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)   { -    u32 val, mode; +    u32 val, mode, i;

+    for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) +        writel_relaxed(drvdata->dsb->edge_ctrl[i], +               drvdata->base + TPDM_DSB_EDCR(i)); +    for (i = 0; i < TPDM_DSB_MAX_EDCR / 2; i++)

Please use "TPDM_DSB_MAX_EDMCR" for consistency and ease of following the code.

Sure, I will update this in the next patch series.

...

• writel_relaxed(drvdata->dsb->edge_ctrl_mask[i],

+               drvdata->base + TPDM_DSB_EDCMR(i));         val = readl_relaxed(drvdata->base + TPDM_DSB_TIER);       /* Set trigger timestamp */ @@ -278,6 +285,125 @@ static ssize_t dsb_mode_store(struct device *dev,   }   static DEVICE_ATTR_RW(dsb_mode);   +static ssize_t dsb_edge_ctrl_show(struct device *dev, +                       struct device_attribute *attr, +                       char *buf) +{ +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); +    ssize_t size = 0; +    int i;

+    if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB)) +        return -EPERM;

+    spin_lock(&drvdata->spinlock); +    for (i = 0; i < TPDM_DSB_MAX_EDCR; i++) { +        size += scnprintf(buf + size, PAGE_SIZE - size, +                  "Index:0x%x Val:0x%x\n", i, +                  drvdata->dsb->edge_ctrl[i]); +    } +    spin_unlock(&drvdata->spinlock); +    return size; +}

+/*

• • value 1: Start EDCR register number
• • value 2: End EDCR register number
• • value 3: The value need to be written

What are the constraints on these inputs ? Please add a comment here. And may be also add a user scenario if possible to describe the values and its effects on the unit ?

The EDCR registers can include up to 16 32-bit registers, and each one can be configured to control up to 16 edge detections(2 bits control one edge detection). So a total 256 edge detections can be configured. So the starting number(value 1) and ending number(value 2) cannot be greater than 256, and value 1 should be less than value2. The following values(value 3) should be writen to EDCR registers. 00 - Rising edge detection 01 - Falling edge detection 10 - Rising and falling edge detection (toggle detection)

...

• */

+static ssize_t dsb_edge_ctrl_store(struct device *dev, +                    struct device_attribute *attr, +                    const char *buf, +                    size_t size) +{ +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); +    unsigned long start, end, edge_ctrl; +    uint32_t val; +    int i, bit, reg;

+    if (sscanf(buf, "%lx %lx %lx", &start, &end, &edge_ctrl) != 3) +        return -EINVAL; +    if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB) || +        (start >= TPDM_DSB_MAX_LINES) || (end >= TPDM_DSB_MAX_LINES) || +        edge_ctrl > 0x2) +        return -EPERM;

+    spin_lock(&drvdata->spinlock); +    for (i = start; i <= end; i++) { +        reg = i / (NUM_OF_BITS / 2); +        bit = i % (NUM_OF_BITS / 2);

NUM_OF_BITS in what ? That is too generic.

It means the number of bits of a register. The 32-bit register has 32 bits(NUM_OF_BITS), and each one can be configured to control 16(NUM_OF_BITS / 2) edge detectioins. I will update this in the next patch series.

...

+        bit = bit * 2;

minor nit: Name "bit" is confusing here. s/bit/{index or even field}?

It should be named "index" here. I will update this in the next patch series.

...

+        val = drvdata->dsb->edge_ctrl[reg]; +        val = val & ~GENMASK((bit + 1), bit); +        val = val | (edge_ctrl << bit); +        drvdata->dsb->edge_ctrl[reg] = val; +    } +    spin_unlock(&drvdata->spinlock);

+    return size; +} +static DEVICE_ATTR_RW(dsb_edge_ctrl);

+static ssize_t dsb_edge_ctrl_mask_show(struct device *dev, +                        struct device_attribute *attr, +                        char *buf) +{ +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); +    ssize_t size = 0; +    int i;

+    if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB)) +        return -EPERM;

+    spin_lock(&drvdata->spinlock); +    for (i = 0; i < TPDM_DSB_MAX_EDCR / 2; i++) { +        size += scnprintf(buf + size, PAGE_SIZE - size, +                  "Index:0x%x Val:0x%x\n", i, +                  drvdata->dsb->edge_ctrl_mask[i]); +    } +    spin_unlock(&drvdata->spinlock); +    return size; +}

+/*

• • value 1: Start EDCMR register number
• • value 2: End EDCMR register number
• • value 3: The value need to be written
• */

+static ssize_t dsb_edge_ctrl_mask_store(struct device *dev, +                         struct device_attribute *attr, +                         const char *buf, +                         size_t size) +{ +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); +    unsigned long start, end, val; +    u32 set; +    int i, bit, reg;

+    if (sscanf(buf, "%lx %lx %lx", &start, &end, &val) != 3) +        return -EINVAL; +    if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB) ||

Please use is_visible() hook.

Sure, I will update this in the next patch series.

...

+        (start >= TPDM_DSB_MAX_LINES) || (end >= TPDM_DSB_MAX_LINES)) +        return -EPERM;

No constraints on "val"? 0 or 1 ?

EDCMR registers are 32-bit registers, and every bit of them provides DSB interface edge detection mask control. So they can be configured to control up to 256 DSB interface edge detection masks. So the number of "start" and "end" should not be greater than 256 here.

...

+    spin_lock(&drvdata->spinlock); +    for (i = start; i <= end; i++) { +        reg = i / NUM_OF_BITS; +        bit = (i % NUM_OF_BITS);

+        set = drvdata->dsb->edge_ctrl_mask[reg]; +        if (val) +            set = set | BIT(bit);

minor nit:             set |= ..

Sure, I will update this in the next patch series.

...

+        else +            set = set & ~BIT(bit);

set &= .. ?

Sure, I will update this in the next patch series.

...

• drvdata->dsb->edge_ctrl_mask[reg] = set;

+    } +    spin_unlock(&drvdata->spinlock); +    return size; +} +static DEVICE_ATTR_RW(dsb_edge_ctrl_mask);

static ssize_t dsb_trig_type_show(struct device *dev,                        struct device_attribute *attr,                        char *buf) @@ -359,6 +485,8 @@ static ssize_t dsb_trig_ts_store(struct device *dev,   static DEVICE_ATTR_RW(dsb_trig_ts);   static struct attribute *tpdm_dsb_attrs[] = {       &dev_attr_dsb_mode.attr, +    &dev_attr_dsb_edge_ctrl.attr, +    &dev_attr_dsb_edge_ctrl_mask.attr,       &dev_attr_dsb_trig_ts.attr,       &dev_attr_dsb_trig_type.attr,       NULL, diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h index 4d57488..ed03c68 100644 --- a/drivers/hwtracing/coresight/coresight-tpdm.h +++ b/drivers/hwtracing/coresight/coresight-tpdm.h @@ -12,6 +12,8 @@   /* DSB Subunit Registers */   #define TPDM_DSB_CR        (0x780)   #define TPDM_DSB_TIER        (0x784) +#define TPDM_DSB_EDCR(n)    (0x808 + (n * 4)) +#define TPDM_DSB_EDCMR(n)    (0x848 + (n * 4))     /* Enable bit for DSB subunit */   #define TPDM_DSB_CR_ENA        BIT(0) @@ -28,6 +30,8 @@   #define TPDM_DSB_MODE_HPBYTESEL(val)    BMVAL(val, 4, 8)   #define TPDM_MODE_ALL            (0xFFFFFFF)   +#define NUM_OF_BITS        32

/* TPDM integration test registers */   #define TPDM_ITATBCNTRL        (0xEF0)   #define TPDM_ITCNTRL        (0xF00) @@ -54,14 +58,21 @@   #define TPDM_PIDR0_DS_IMPDEF    BIT(0)   #define TPDM_PIDR0_DS_DSB    BIT(1)   +#define TPDM_DSB_MAX_EDCR    16 > +#define TPDM_DSB_MAX_LINES    256

A comment here would help : e.g, EDCR has 2 bits for each lines. Thus we can support upto (16 * 32) / 2 lines.

May be, even use that math above instead of hard coding 256 ? And explain what each bits would do ?

I will add more comments here in the next patch series.

...

/**    * struct dsb_dataset - specifics associated to dsb dataset    * @mode:             DSB programming mode

• • @edge_ctrl:        Save value for edge control
• • @edge_ctrl_mask:   Save value for edge control mask

* @trig_ts:          Enable/Disable trigger timestamp.    * @trig_type:        Enable/Disable trigger type.    */   struct dsb_dataset {       u32                mode; +    u32                edge_ctrl[TPDM_DSB_MAX_EDCR]; +    u32                edge_ctrl_mask[TPDM_DSB_MAX_EDCR / 2];

Please use TPDM_DSB_MAX_EDCMR instead.

Sure. I will update this in the next patch series.

Suzuki

...

bool            trig_ts;       bool            trig_type;   };

Best Regards

Tao

Hi Suzuki,

On 10/25/2022 6:00 PM, Suzuki K Poulose wrote:

On 08/09/2022 09:45, Tao Zhang wrote:

...

Add nodes to configure the timestamp request based on input pattern match. Each TPDM that support DSB subunit has n(0-7) TPR registers to configure value for timestamp request based on input pattern match, and has m(0-7) TPMR registers to configure pattern mask for timestamp request. Add nodes to enable/disable pattern timestamp and set pattern timestamp type.

Signed-off-by: Tao Zhang quic_taozha@quicinc.com

drivers/hwtracing/coresight/coresight-tpdm.c | 189 +++++++++++++++++++++++++++   drivers/hwtracing/coresight/coresight-tpdm.h |  14 ++   2 files changed, 203 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c index 648bbe6..4212ff4 100644 --- a/drivers/hwtracing/coresight/coresight-tpdm.c +++ b/drivers/hwtracing/coresight/coresight-tpdm.c @@ -32,6 +32,13 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)                  drvdata->base + TPDM_DSB_EDCMR(i));         for (i = 0; i < TPDM_DSB_MAX_PATT; i++) { +        writel_relaxed(drvdata->dsb->patt_val[i], +                drvdata->base + TPDM_DSB_TPR(i)); +        writel_relaxed(drvdata->dsb->patt_mask[i], +                drvdata->base + TPDM_DSB_TPMR(i)); +    }

+    for (i = 0; i < TPDM_DSB_MAX_PATT; i++) {           writel_relaxed(drvdata->dsb->trig_patt_val[i],                   drvdata->base + TPDM_DSB_XPR(i));           writel_relaxed(drvdata->dsb->trig_patt_mask[i], @@ -39,6 +46,16 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)       }         val = readl_relaxed(drvdata->base + TPDM_DSB_TIER); +    /* Set pattern timestamp type and enablement */ +    if (drvdata->dsb->patt_ts) { +        val |= TPDM_DSB_PATT_TSENAB; +        if (drvdata->dsb->patt_type) +            val |= TPDM_DSB_PATT_TYPE; +        else +            val &= ~TPDM_DSB_PATT_TYPE; +    } else { +        val &= ~TPDM_DSB_PATT_TSENAB; +    }       /* Set trigger timestamp */       if (drvdata->dsb->trig_ts)           val |= TPDM_DSB_XTRIG_TSENAB; @@ -411,6 +428,174 @@ static ssize_t dsb_edge_ctrl_mask_store(struct device *dev,   }   static DEVICE_ATTR_RW(dsb_edge_ctrl_mask);   +static ssize_t dsb_patt_val_show(struct device *dev, +                      struct device_attribute *attr, +                      char *buf) +{ +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); +    ssize_t size = 0; +    int i = 0;

+    if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB)) +        return -EPERM;

+    spin_lock(&drvdata->spinlock); +    for (i = 0; i < TPDM_DSB_MAX_PATT; i++) { +        size += scnprintf(buf + size, PAGE_SIZE - size, +                  "Index: 0x%x Value: 0x%x\n", i, +                  drvdata->dsb->patt_val[i]); +    } +    spin_unlock(&drvdata->spinlock); +    return size; +}

+/*

• • value 1: Index of TPR register
• • value 2: Value need to be written
• */

+static ssize_t dsb_patt_val_store(struct device *dev, +                       struct device_attribute *attr, +                       const char *buf, +                       size_t size) +{ +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); +    unsigned long index, val;

+    if (sscanf(buf, "%lx %lx", &index, &val) != 2) +        return -EINVAL; +    if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB) || +        index >= TPDM_DSB_MAX_PATT) +        return -EPERM;

+    spin_lock(&drvdata->spinlock); +    drvdata->dsb->patt_val[index] = val; +    spin_unlock(&drvdata->spinlock); +    return size; +} +static DEVICE_ATTR_RW(dsb_patt_val);

+static ssize_t dsb_patt_mask_show(struct device *dev, +                       struct device_attribute *attr, +                       char *buf) +{ +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); +    ssize_t size = 0; +    int i = 0;

+    if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB)) +        return -EPERM;

+    spin_lock(&drvdata->spinlock); +    for (i = 0; i < TPDM_DSB_MAX_PATT; i++) { +        size += scnprintf(buf + size, PAGE_SIZE - size, +                  "Index: 0x%x Value: 0x%x\n", i, +                  drvdata->dsb->patt_mask[i]); +    } +    spin_unlock(&drvdata->spinlock); +    return size; +}

+/*

• • value 1: Index of TPMR register
• • value 2: Value need to be written
• */

+static ssize_t dsb_patt_mask_store(struct device *dev, +                    struct device_attribute *attr, +                    const char *buf, +                    size_t size) +{ +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); +    unsigned long index, val;

+    if (sscanf(buf, "%lx %lx", &index, &val) != 2) +        return -EINVAL; +    if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB) || +        index >= TPDM_DSB_MAX_PATT) +        return -EPERM;

+    spin_lock(&drvdata->spinlock); +    drvdata->dsb->patt_mask[index] = val; +    spin_unlock(&drvdata->spinlock); +    return size; +} +static DEVICE_ATTR_RW(dsb_patt_mask);

+static ssize_t dsb_patt_ts_show(struct device *dev, +                     struct device_attribute *attr, +                     char *buf) +{ +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);

+    if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB)) +        return -EPERM;

+    return scnprintf(buf, PAGE_SIZE, "%u\n", +             (unsigned int)drvdata->dsb->patt_ts);

Please use sysfs_emit() everywhere (in the previous patches too) for such operations.

Sure, I will update this in the next patch series.

I have finished reviewing this series.

Suzuki _______________________________________________ CoreSight mailing list -- coresight@lists.linaro.org To unsubscribe send an email to coresight-leave@lists.linaro.org