On Thu, 16 Mar 2023 at 17:17, Suzuki K Poulose suzuki.poulose@arm.com wrote:

On 10/03/2023 16:06, James Clark wrote:

...

conns is actually for output connections. Change the name to make it clearer and so that we can add input connections later.

No functional changes.

Signed-off-by: James Clark james.clark@arm.com

drivers/hwtracing/coresight/coresight-core.c | 26 +++++++++---------- .../hwtracing/coresight/coresight-platform.c | 12 ++++----- .../hwtracing/coresight/coresight-tmc-etr.c | 2 +- include/linux/coresight.h | 2 +- 4 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index e3b63fd52b9c..c0eda7407fb8 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -119,7 +119,7 @@ static int coresight_find_link_inport(struct coresight_device *csdev, struct coresight_connection *conn;

  for (i = 0; i < parent->pdata->nr_outport; i++) {

•         conn = &parent->pdata->conns[i];
  

•         conn = &parent->pdata->out_conns[i];
          if (conn->child_dev == csdev)
                  return conn->child_port;
  }
  

@@ -137,7 +137,7 @@ static int coresight_find_link_outport(struct coresight_device *csdev, struct coresight_connection *conn;

  for (i = 0; i < csdev->pdata->nr_outport; i++) {

•         conn = &csdev->pdata->conns[i];
  

•         conn = &csdev->pdata->out_conns[i];
          if (conn->child_dev == child)
                  return conn->outport;
  }
  

@@ -606,7 +606,7 @@ coresight_find_enabled_sink(struct coresight_device *csdev) for (i = 0; i < csdev->pdata->nr_outport; i++) { struct coresight_device *child_dev;

•         child_dev = csdev->pdata->conns[i].child_dev;
  

•         child_dev = csdev->pdata->out_conns[i].child_dev;
          if (child_dev)
                  sink = coresight_find_enabled_sink(child_dev);
          if (sink)
  

@@ -722,7 +722,7 @@ static int coresight_grab_device(struct coresight_device *csdev) for (i = 0; i < csdev->pdata->nr_outport; i++) { struct coresight_device *child;

•         child  = csdev->pdata->conns[i].child_dev;
  

•         child = csdev->pdata->out_conns[i].child_dev;
          if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
                  if (!coresight_get_ref(child))
                          goto err;
  

@@ -733,7 +733,7 @@ static int coresight_grab_device(struct coresight_device *csdev) for (i--; i >= 0; i--) { struct coresight_device *child;

•         child  = csdev->pdata->conns[i].child_dev;
  

•         child = csdev->pdata->out_conns[i].child_dev;
          if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
                  coresight_put_ref(child);
  }
  

@@ -752,7 +752,7 @@ static void coresight_drop_device(struct coresight_device *csdev) for (i = 0; i < csdev->pdata->nr_outport; i++) { struct coresight_device *child;

•         child  = csdev->pdata->conns[i].child_dev;
  

•         child = csdev->pdata->out_conns[i].child_dev;
          if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
                  coresight_put_ref(child);
  }
  

@@ -794,7 +794,7 @@ static int _coresight_build_path(struct coresight_device *csdev, for (i = 0; i < csdev->pdata->nr_outport; i++) { struct coresight_device *child_dev;

•         child_dev = csdev->pdata->conns[i].child_dev;
  

•         child_dev = csdev->pdata->out_conns[i].child_dev;
          if (child_dev &&
              _coresight_build_path(child_dev, sink, path) == 0) {
                  found = true;
  

@@ -964,7 +964,7 @@ coresight_find_sink(struct coresight_device *csdev, int *depth) struct coresight_device *child_dev, *sink = NULL; int child_depth = curr_depth;

•         child_dev = csdev->pdata->conns[i].child_dev;
  

•         child_dev = csdev->pdata->out_conns[i].child_dev;
          if (child_dev)
                  sink = coresight_find_sink(child_dev, &child_depth);
  
  

@@ -1334,7 +1334,7 @@ static int coresight_orphan_match(struct device *dev, void *data) * an orphan connection whose name matches @csdev, link it. */ for (i = 0; i < i_csdev->pdata->nr_outport; i++) {

•         conn = &i_csdev->pdata->conns[i];
  

•         conn = &i_csdev->pdata->out_conns[i];
  
          /* Skip the port if FW doesn't describe it */
          if (!conn->child_fwnode)
  

@@ -1375,7 +1375,7 @@ static int coresight_fixup_device_conns(struct coresight_device *csdev) int i, ret = 0;

  for (i = 0; i < csdev->pdata->nr_outport; i++) {

•         struct coresight_connection *conn = &csdev->pdata->conns[i];
  

•         struct coresight_connection *conn = &csdev->pdata->out_conns[i];
  
          if (!conn->child_fwnode)
                  continue;
  

@@ -1412,7 +1412,7 @@ static int coresight_remove_match(struct device *dev, void *data) * a connection whose name matches @csdev, remove it. */ for (i = 0; i < iterator->pdata->nr_outport; i++) {

•         conn = &iterator->pdata->conns[i];
  

•         conn = &iterator->pdata->out_conns[i];
  
          if (conn->child_dev == NULL || conn->child_fwnode == NULL)
                  continue;
  

@@ -1548,7 +1548,7 @@ void coresight_release_platform_data(struct coresight_device *csdev, struct coresight_platform_data *pdata) { int i;

• struct coresight_connection *conns = pdata->conns;
  

• struct coresight_connection *conns = pdata->out_conns;
  
  for (i = 0; i < pdata->nr_outport; i++) {
          /* If we have made the links, remove them now */
  

@@ -1560,7 +1560,7 @@ void coresight_release_platform_data(struct coresight_device *csdev, */ if (conns[i].child_fwnode) { fwnode_handle_put(conns[i].child_fwnode);

•                 pdata->conns[i].child_fwnode = NULL;
  

•                 pdata->out_conns[i].child_fwnode = NULL;
          }
  }
  if (csdev)
  

diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c index 475899714104..5085525a32bb 100644 --- a/drivers/hwtracing/coresight/coresight-platform.c +++ b/drivers/hwtracing/coresight/coresight-platform.c @@ -27,9 +27,9 @@ static int coresight_alloc_conns(struct device *dev, struct coresight_platform_data *pdata) { if (pdata->nr_outport) {

•         pdata->conns = devm_kcalloc(dev, pdata->nr_outport,
  

•                                     sizeof(*pdata->conns), GFP_KERNEL);
  

•         if (!pdata->conns)
  

•         pdata->out_conns = devm_kcalloc(dev, pdata->nr_outport,
  

•                                     sizeof(*pdata->out_conns), GFP_KERNEL);
  

•         if (!pdata->out_conns)
                  return -ENOMEM;
  }
  
  

@@ -251,7 +251,7 @@ static int of_coresight_parse_endpoint(struct device *dev, break; }

•         conn = &pdata->conns[endpoint.port];
  

•         conn = &pdata->out_conns[endpoint.port];
          if (conn->child_fwnode) {
                  dev_warn(dev, "Duplicate output port %d\n",
                           endpoint.port);
  

@@ -744,8 +744,8 @@ static int acpi_coresight_parse_graph(struct acpi_device *adev, int port = conns[i].outport;

          /* Duplicate output port */

•         WARN_ON(pdata->conns[port].child_fwnode);
  

•         pdata->conns[port] = conns[i];
  

•         WARN_ON(pdata->out_conns[port].child_fwnode);
  

•         pdata->out_conns[port] = conns[i];
  }
  
  devm_kfree(&adev->dev, conns);
  

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index ed589cfff1b5..86d4a08aa833 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -782,7 +782,7 @@ tmc_etr_get_catu_device(struct tmc_drvdata *drvdata) return NULL;

  for (i = 0; i < etr->pdata->nr_outport; i++) {

•         tmp = etr->pdata->conns[i].child_dev;
  

•         tmp = etr->pdata->out_conns[i].child_dev;
          if (tmp && coresight_is_catu_device(tmp))
                  return tmp;
  }
  

diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 1f878e8ed8c4..322d7273e122 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -109,7 +109,7 @@ union coresight_dev_subtype { struct coresight_platform_data { int nr_inport; int nr_outport;

• struct coresight_connection *conns;
  

• struct coresight_connection *out_conns;
  

The comment above this structure might need to reflect the name change.

Otherwise looks good to me.

Suzuki

...

};

/**

Reviewed-by: Mike Leach mike.leach@linaro.org

-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

On 10/03/2023 16:06, James Clark wrote:

Rename to avoid confusion between port number and the index in the connection array. The port number is already stored in the connection, and in a later commit the connection array will be appended to, so the length of it will no longer reflect the number of ports.

No functional changes.

Signed-off-by: James Clark james.clark@arm.com

drivers/hwtracing/coresight/coresight-core.c | 32 ++++++++--------- .../hwtracing/coresight/coresight-platform.c | 34 +++++++++---------- .../hwtracing/coresight/coresight-tmc-etr.c | 2 +- include/linux/coresight.h | 10 +++--- 4 files changed, 39 insertions(+), 39 deletions(-)

...

diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 322d7273e122..cdf0d1def778 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -102,13 +102,13 @@ union coresight_dev_subtype {

• struct coresight_platform_data - data harvested from the firmware
• specification.

• • @nr_inport: Number of elements for the input connections.
• • @nr_outport: Number of elements for the output connections.
• • @conns: Sparse array of nr_outport connections from this component.

• • @nr_inconns: Number of elements for the input connections.
• • @nr_outconns: Number of elements for the output connections.
• • @conns: Sparse array of nr_outconns connections from this component.

s/conns/out_conns/

Otherwise looks good to me.

Suzuki

Hi James

On Thu, 16 Mar 2023 at 17:12, Suzuki K Poulose suzuki.poulose@arm.com wrote:

On 10/03/2023 16:06, James Clark wrote:

...

Add a function for adding connections dynamically. This also removes the 1:1 mapping between port number and the index into the connections array. The only place this mapping was used was in the warning for duplicate output ports, which has been replaced by a search. Other uses of the port number already use the port member variable.

Being able to dynamically add connections will allow other devices like CTI to re-use the connection mechanism despite not having explicit connections described in the DT.

Signed-off-by: James Clark james.clark@arm.com

.../hwtracing/coresight/coresight-platform.c | 77 ++++++++++++++----- include/linux/coresight.h | 7 +- 2 files changed, 64 insertions(+), 20 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c index c77238cdf448..16553f7dde12 100644 --- a/drivers/hwtracing/coresight/coresight-platform.c +++ b/drivers/hwtracing/coresight/coresight-platform.c @@ -27,8 +27,9 @@ static int coresight_alloc_conns(struct device *dev, struct coresight_platform_data *pdata) { if (pdata->nr_outconns) {

•         pdata->out_conns = devm_kcalloc(dev, pdata->nr_outconns,
  

•                                     sizeof(*pdata->out_conns), GFP_KERNEL);
  

•         pdata->out_conns = devm_krealloc_array(
  

•                 dev, pdata->out_conns, pdata->nr_outconns,
  

super minor nit: pdata->out_conns = devm_krealloc_array(dev,

...

•                 sizeof(*pdata->out_conns), GFP_KERNEL | __GFP_ZERO);
          if (!pdata->out_conns)
                  return -ENOMEM;
  }
  

@@ -36,6 +37,48 @@ static int coresight_alloc_conns(struct device *dev, return 0; }

+/*

• • Add a connection in the first free slot, or realloc
• • if there is no space. @conn's contents is copied into the new slot.
• • If the output port is already assigned on this device, return -EINVAL
• */

+int coresight_add_conn(struct device *dev,

•                struct coresight_platform_data *pdata,
  

•                const struct coresight_connection *conn)
  

+{

• int ret;
  

• struct coresight_connection *free_conn = NULL;
  

• struct coresight_connection *i;
  

• /*
  

•  * Search for a free slot, and while looking for one, warn
  

•  * on any existing duplicate output port.
  

•  */
  

• for (i = pdata->out_conns; i < pdata->out_conns + pdata->nr_outconns;
  

•      ++i) {
  

minor nit: I see why you have gone against using "i" as index into the array. But I think having that as the index is still better readable.

    for (i = 0; i < pdata->nr_outconns; i++) {
            struct coresight_connection *c = &pdata->out_conns[i];

...

•         if (i->remote_fwnode && conn->port != -1 &&
  

•             i->port == conn->port) {
  

•                 dev_warn(dev, "Duplicate output port %d\n", i->port);
  

•                 return -EINVAL;
  

•         }
  

This code assumes that slots are filled sequentially and that it is not possible to release slots out of order - i.e. if we find a free slot, there is not a match in a later slot. I can't think how this could happen but a comment to confirm this might be needed here.

When we had 1:1 port / array index then this check was guaranteed

Mike

...

•         if (!i->remote_fwnode && !free_conn)
  

•                 free_conn = i;
  

• }
  

• if (!free_conn) {
  

and: /* No free slots */ if (i == pdata->nr_outconns) {

...

•         pdata->nr_outconns++;
  

•         ret = coresight_alloc_conns(dev, pdata);
  

•         if (ret)
  

•                 return ret;
  

•         free_conn = &pdata->out_conns[pdata->nr_outconns - 1];
  

• }
  

and: pdata->out_conns[i] = *conn;

Otherwise looks good to me.

Suzuki

-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

On 16/03/2023 17:12, Suzuki K Poulose wrote:

On 10/03/2023 16:06, James Clark wrote:

...

Add a function for adding connections dynamically. This also removes the 1:1 mapping between port number and the index into the connections array. The only place this mapping was used was in the warning for duplicate output ports, which has been replaced by a search. Other uses of the port number already use the port member variable.

Being able to dynamically add connections will allow other devices like CTI to re-use the connection mechanism despite not having explicit connections described in the DT.

Signed-off-by: James Clark james.clark@arm.com

.../hwtracing/coresight/coresight-platform.c  | 77 ++++++++++++++-----   include/linux/coresight.h                     |  7 +-   2 files changed, 64 insertions(+), 20 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c index c77238cdf448..16553f7dde12 100644 --- a/drivers/hwtracing/coresight/coresight-platform.c +++ b/drivers/hwtracing/coresight/coresight-platform.c @@ -27,8 +27,9 @@ static int coresight_alloc_conns(struct device *dev,                    struct coresight_platform_data *pdata)   {       if (pdata->nr_outconns) { -        pdata->out_conns = devm_kcalloc(dev, pdata->nr_outconns, -                        sizeof(*pdata->out_conns), GFP_KERNEL); +        pdata->out_conns = devm_krealloc_array( +            dev, pdata->out_conns, pdata->nr_outconns,

super minor nit:         pdata->out_conns = devm_krealloc_array(dev,

This is actually clang-format's doing when using the kernel rules in the root of the repo. I started using it because of some other style comments I got before. Not sure if this time it's just done something bad or it's technically ok.

I formatted everything in V3 with it so it should at least all be consistent.

...

+            sizeof(*pdata->out_conns), GFP_KERNEL | __GFP_ZERO);           if (!pdata->out_conns)               return -ENOMEM;       } @@ -36,6 +37,48 @@ static int coresight_alloc_conns(struct device *dev,       return 0;   }   +/*

• • Add a connection in the first free slot, or realloc
• • if there is no space. @conn's contents is copied into the new slot.
• • If the output port is already assigned on this device, return -EINVAL
• */

+int coresight_add_conn(struct device *dev, +               struct coresight_platform_data *pdata, +               const struct coresight_connection *conn) +{ +    int ret; +    struct coresight_connection *free_conn = NULL; +    struct coresight_connection *i;

+    /* +     * Search for a free slot, and while looking for one, warn +     * on any existing duplicate output port. +     */ +    for (i = pdata->out_conns; i < pdata->out_conns + pdata->nr_outconns; +         ++i) {

minor nit: I see why you have gone against using "i" as index into the array. But I think having that as the index is still better readable.     for (i = 0; i < pdata->nr_outconns; i++) {         struct coresight_connection *c = &pdata->out_conns[i];

...

...

+        if (i->remote_fwnode && conn->port != -1 &&

+            i->port == conn->port) { +            dev_warn(dev, "Duplicate output port %d\n", i->port); +            return -EINVAL; +        } +        if (!i->remote_fwnode && !free_conn) +            free_conn = i; +    }

+    if (!free_conn) {

and:     /* No free slots */     if (i == pdata->nr_outconns) {

...

+        pdata->nr_outconns++; +        ret = coresight_alloc_conns(dev, pdata); +        if (ret) +            return ret; +        free_conn = &pdata->out_conns[pdata->nr_outconns - 1]; +    }

and:     pdata->out_conns[i] = *conn;

...

Otherwise looks good to me.

Suzuki

On 3/11/2023 12:06 AM, James Clark wrote:

This will allow CATU to get its associated ETR in a generic way where currently the enable path has some hard coded searches which avoid the need to store input connections.

Signed-off-by: James Clark james.clark@arm.com

drivers/hwtracing/coresight/coresight-core.c | 56 +++++++++++++++-- .../hwtracing/coresight/coresight-platform.c | 61 ++++++++++++++++--- drivers/hwtracing/coresight/coresight-sysfs.c | 1 - include/linux/coresight.h | 25 ++++++++ 4 files changed, 130 insertions(+), 13 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index f457914e445e..a8ba7493c09a 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -59,6 +59,7 @@ const u32 coresight_barrier_pkt[4] = {0x7fffffff, 0x7fffffff, 0x7fffffff, 0x7fff EXPORT_SYMBOL_GPL(coresight_barrier_pkt); static const struct cti_assoc_op *cti_assoc_ops; +static int coresight_fixup_inputs(struct coresight_device *csdev); ssize_t coresight_simple_show_pair(struct device *_dev, struct device_attribute *attr, char *buf) @@ -1369,6 +1370,35 @@ static int coresight_fixup_orphan_conns(struct coresight_device *csdev) csdev, coresight_orphan_match); } +/*

• • Device connections are discovered before one/both devices have been created,
• • so inputs must be added later.
• */

+static int coresight_fixup_inputs(struct coresight_device *csdev) +{

• int i, ret = 0;
• struct coresight_connection *out_conn;
• struct coresight_connection in_conn;
• for (i = 0; i < csdev->pdata->nr_outconns; i++) {

• out_conn = &csdev->pdata->out_conns[i];
  

• if (!out_conn->remote_dev || !out_conn->remote_dev->pdata)
  

• 	continue;
  

Hi James,

If out_conn->remote_dev is null here,  the in_conn of out_conn->remote_dev->pdata will never be set. For example, device A is connected to in_port 0 of device B. If device A is probed first, the in_conn of device B will not be set. Do we need to add Defer probe return here ? I tested with defer probe return, it works.

for (i = 0; i < csdev->pdata->nr_outconns; i++) {                 out_conn = &csdev->pdata->out_conns[i];                 if (!out_conn->remote_dev || !out_conn->remote_dev->pdata) -                       continue; +                        return -EPROBE_DEFER;

Thanks Jinlong Mao

• /* Reverse local/remote relationships for inputs */
  

• in_conn.remote_dev = csdev;
  

• in_conn.remote_port = out_conn->port;
  

• in_conn.port = out_conn->remote_port;
  

• in_conn.remote_fwnode = csdev->dev.fwnode;
  

• ret = coresight_add_in_conn(out_conn->remote_dev->dev.parent,
  

• 			    out_conn->remote_dev->pdata,
  

• 			    &in_conn);
  

• if (ret)
  

• 	return ret;
  

• }
• return 0;

+} static int coresight_fixup_device_conns(struct coresight_device *csdev) { @@ -1427,11 +1457,20 @@ static int coresight_remove_match(struct device *dev, void *data) */ fwnode_handle_put(conn->remote_fwnode); conn->remote_fwnode = NULL;

• 	conn->remote_dev = NULL;
  

• 	/* No need to continue */
  

• 	break;
  

• }
  

• }
• for (i = 0; i < iterator->pdata->nr_inconns; i++) {

• conn = &iterator->pdata->in_conns[i];
  

• if (csdev == conn->remote_dev) {
  

• 	conn->remote_fwnode = NULL;
  

• 	conn->remote_dev = NULL;
  /* No need to continue */
  break;
  

  } }

• /*
  • Returning '0' ensures that all known component on the
  • bus will be checked.

@@ -1552,21 +1591,28 @@ void coresight_release_platform_data(struct coresight_device *csdev, for (i = 0; i < pdata->nr_outconns; i++) { /* If we have made the links, remove them now */

• if (csdev && conns[i].remote_dev)
  

• if (csdev && conns[i].remote_dev) {
  coresight_remove_links(csdev, &conns[i]);
  

• 	conns[i].remote_dev = NULL;
  

• }
  

• /*
  • Drop the refcount and clear the handle as this device
  • is going away
  */ if (conns[i].remote_fwnode) { fwnode_handle_put(conns[i].remote_fwnode);

• 	pdata->out_conns[i].remote_fwnode = NULL;
  

• 	conns[i].remote_fwnode = NULL;
  

  } }
• for (i = 0; i < pdata->nr_inconns; i++) {

• pdata->in_conns[i].remote_dev = NULL;
  

• pdata->in_conns[i].remote_fwnode = NULL;
  

• }
• if (csdev) coresight_remove_conns_sysfs_group(csdev); }

• struct coresight_device *coresight_register(struct coresight_desc *desc) { int ret;

@@ -1659,6 +1705,8 @@ struct coresight_device *coresight_register(struct coresight_desc *desc) ret = coresight_create_conns_sysfs_group(csdev); if (!ret) ret = coresight_fixup_device_conns(csdev);

• if (!ret)

• ret = coresight_fixup_inputs(csdev);
  

  if (!ret) ret = coresight_fixup_orphan_conns(csdev);

diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c index 16553f7dde12..20e3351cbdc2 100644 --- a/drivers/hwtracing/coresight/coresight-platform.c +++ b/drivers/hwtracing/coresight/coresight-platform.c @@ -20,8 +20,7 @@ #include "coresight-priv.h" /*

• • coresight_alloc_conns: Allocate connections record for each output
• • port from the device.

• • coresight_alloc_conns: Allocate connections record for each input/output device.
  */ static int coresight_alloc_conns(struct device *dev, struct coresight_platform_data *pdata)

@@ -33,7 +32,14 @@ static int coresight_alloc_conns(struct device *dev, if (!pdata->out_conns) return -ENOMEM; }

• if (pdata->nr_inconns) {

• pdata->in_conns = devm_krealloc_array(dev, pdata->in_conns,
  

• 				      pdata->nr_inconns,
  

• 				      sizeof(*pdata->in_conns),
  

• 				      GFP_KERNEL | __GFP_ZERO);
  

• if (!pdata->in_conns)
  

• 	return -ENOMEM;
  

• } return 0; }

@@ -79,6 +85,45 @@ int coresight_add_conn(struct device *dev, } EXPORT_SYMBOL_GPL(coresight_add_conn); +/*

• • Add a connection in the first free slot, or realloc
• • if there is no space.
• • Do nothing if the connection already exists because inputs are
• • fixed up multiple times.
• */

+int coresight_add_in_conn(struct device *dev,

• 	  struct coresight_platform_data *pdata,
  

• 	  struct coresight_connection *conn)
  

+{

• int ret;
• struct coresight_connection *free_conn = NULL;
• int i;
• /* Search for a free slot or exit if a duplicate is found */
• if (pdata->in_conns) {

• for (i = 0; i < pdata->nr_inconns; ++i) {
  

• 	if (!free_conn && !pdata->in_conns[i].remote_fwnode)
  

• 		free_conn = &pdata->in_conns[i];
  

• 	if (pdata->in_conns[i].remote_fwnode ==
  

• 	    conn->remote_fwnode)
  

• 		return 0;
  

• }
  

• }
• if (!free_conn) {

• pdata->nr_inconns++;
  

• ret = coresight_alloc_conns(dev, pdata);
  

• if (ret)
  

• 	return ret;
  

• free_conn = &pdata->in_conns[pdata->nr_inconns - 1];
  

• }
• *free_conn = *conn;
• return 0;

+} +EXPORT_SYMBOL_GPL(coresight_add_in_conn);

• static struct device * coresight_find_device_by_fwnode(struct fwnode_handle *fwnode) {

@@ -249,7 +294,7 @@ static int of_coresight_get_cpu(struct device *dev) /*

• of_coresight_parse_endpoint : Parse the given output endpoint @ep

• • and fill the connection information in @conn

• • and fill the connection information in @in_conn and @out_conn
  • Parses the local port, remote device name and the remote port.

@@ -333,14 +378,14 @@ static int of_get_coresight_platform_data(struct device *dev, /* Get the number of input and output port for this component */ of_coresight_get_ports(node, &pdata->nr_inconns, &pdata->nr_outconns);

• /* If there are no output connections, we are done */
• if (!pdata->nr_outconns)

• return 0;
  

• ret = coresight_alloc_conns(dev, pdata); if (ret) return ret;

• /* If there are no output connections, we are done */
• if (!pdata->nr_outconns)

• return 0;
  

• parent = of_coresight_get_output_ports_node(node); /*
  • If the DT uses obsoleted bindings, the ports are listed

diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c index 3da9868d9237..2abf9639ac0f 100644 --- a/drivers/hwtracing/coresight/coresight-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-sysfs.c @@ -202,5 +202,4 @@ void coresight_remove_links(struct coresight_device *orig, devm_kfree(&orig->dev, conn->link->orig_name); devm_kfree(&orig->dev, conn->link); conn->link = NULL;

• conn->remote_dev = NULL; }

diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 47fa58d6981d..fd268b24c761 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -110,6 +110,7 @@ struct coresight_platform_data { int nr_inconns; int nr_outconns; struct coresight_connection *out_conns;

• struct coresight_connection *in_conns; };

/** @@ -177,6 +178,27 @@ struct coresight_desc {

•           @remote_fwnode once the remote's coresight_device has
  

•           been created.
  

• @link: Representation of the connection as a sysfs link.

• • The full connection structure looks like this, where in_conns store references to
• • the parent device in the same remote_dev member as output connections.

• •   +-----------------------------+            +-----------------------------+
    

• •   |coresight_device             |            |coresight_connection         |
    

• •   |-----------------------------|            |-----------------------------|
    

• • ---->| | | |
• • | | | | remote_dev*|------
• • | |pdata->out_conns[nr_outconns]|----------->| | |
• • | | | | | |
• • | +-----------------------------+ +-----------------------------+ |
• • | |
• • | +-----------------------------+ +-----------------------------+ |
• • | |coresight_connection | |coresight_device | |
• • | |-----------------------------| |------------------------------ |
• • | | | | |<-----
• • -----|remote_dev* | | |

• •   |                             |<-----------|pdata->in_conns[nr_inconns]  |
    

• •   |                             |            |                             |
    

• •   +-----------------------------+            +-----------------------------+
    

  */ struct coresight_connection { int port;

@@ -619,5 +641,8 @@ struct coresight_platform_data *coresight_get_platform_data(struct device *dev); int coresight_add_conn(struct device *dev, struct coresight_platform_data *pdata, const struct coresight_connection *conn); +int coresight_add_in_conn(struct device *dev,

• 	  struct coresight_platform_data *pdata,
  

• 	  struct coresight_connection *conn);
  

#endif /* _LINUX_COREISGHT_H */

On 10/03/2023 16:06, James Clark wrote:

This will allow CATU to get its associated ETR in a generic way where currently the enable path has some hard coded searches which avoid the need to store input connections.

Signed-off-by: James Clark james.clark@arm.com

drivers/hwtracing/coresight/coresight-core.c | 56 +++++++++++++++-- .../hwtracing/coresight/coresight-platform.c | 61 ++++++++++++++++--- drivers/hwtracing/coresight/coresight-sysfs.c | 1 - include/linux/coresight.h | 25 ++++++++ 4 files changed, 130 insertions(+), 13 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index f457914e445e..a8ba7493c09a 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -59,6 +59,7 @@ const u32 coresight_barrier_pkt[4] = {0x7fffffff, 0x7fffffff, 0x7fffffff, 0x7fff EXPORT_SYMBOL_GPL(coresight_barrier_pkt); static const struct cti_assoc_op *cti_assoc_ops; +static int coresight_fixup_inputs(struct coresight_device *csdev); ssize_t coresight_simple_show_pair(struct device *_dev, struct device_attribute *attr, char *buf) @@ -1369,6 +1370,35 @@ static int coresight_fixup_orphan_conns(struct coresight_device *csdev) csdev, coresight_orphan_match); } +/*

• • Device connections are discovered before one/both devices have been created,
• • so inputs must be added later.
• */

+static int coresight_fixup_inputs(struct coresight_device *csdev) +{

• int i, ret = 0;
• struct coresight_connection *out_conn;
• struct coresight_connection in_conn;
• for (i = 0; i < csdev->pdata->nr_outconns; i++) {

• out_conn = &csdev->pdata->out_conns[i];
  

• if (!out_conn->remote_dev || !out_conn->remote_dev->pdata)
  

• 	continue;
  

• /* Reverse local/remote relationships for inputs */
  

• in_conn.remote_dev = csdev;
  

• in_conn.remote_port = out_conn->port;
  

• in_conn.port = out_conn->remote_port;
  

• in_conn.remote_fwnode = csdev->dev.fwnode;
  

• ret = coresight_add_in_conn(out_conn->remote_dev->dev.parent,
  

• 			    out_conn->remote_dev->pdata,
  

• 			    &in_conn);
  

• if (ret)
  

• 	return ret;
  

• }
• return 0;

+} static int coresight_fixup_device_conns(struct coresight_device *csdev) { @@ -1427,11 +1457,20 @@ static int coresight_remove_match(struct device *dev, void *data)

Removing the connection should be much simpler now with the inports connection tracking.

i.e., coresight_remove_conns() need not iterate over all the devices on the coresight bus. We had to do that to find all the devices connecting to the removed device.

Instead with the in_conns we could readily get the csdev instance and remove the connection from those devices.

Similarly for the out_conns can give you the remove device for removing the in_conns on that device.

i.e,

static void coresight_remove_conns(csdev) { struct coresight_device *remote; for (i = 0; i < csdev->pdata->nr_outconns; i++) { remote = csdev->data->out_conns[i].remote_dev;

if (!remote) continue;

for (j = 0; j < remote->pdata->nr_inconns; j++) { if (remote->pdata->in_conns[i].remote_dev == csdev) /* Clear in_conns[i] on remote */ } }

Similarly for in_conns for (i = 0; i < csdev->pdata->nr_inconns; i++) { remote = csdev->data->in_conns[i].remote_dev;

if (!remote) continue;

for (j = 0; j < remote->pdata->nr_outconns; j++) { if (remote->pdata->out_conns[i].remote_dev == csdev) /* Clear out_conns[i] on remote */ }

	 */
	fwnode_handle_put(conn->remote_fwnode);
	conn->remote_fwnode = NULL;

• 	conn->remote_dev = NULL;
  

• 	/* No need to continue */
  

• 	break;
  

• }
  

• }
• for (i = 0; i < iterator->pdata->nr_inconns; i++) {

• conn = &iterator->pdata->in_conns[i];
  

• if (csdev == conn->remote_dev) {
  

• 	conn->remote_fwnode = NULL;
  

• 	conn->remote_dev = NULL;
  /* No need to continue */
  break;
  

  } }

• /*
  • Returning '0' ensures that all known component on the
  • bus will be checked.

@@ -1552,21 +1591,28 @@ void coresight_release_platform_data(struct coresight_device *csdev, for (i = 0; i < pdata->nr_outconns; i++) { /* If we have made the links, remove them now */

• if (csdev && conns[i].remote_dev)
  

• if (csdev && conns[i].remote_dev) {
  coresight_remove_links(csdev, &conns[i]);
  

• 	conns[i].remote_dev = NULL;
  

• }
  

• /*
  • Drop the refcount and clear the handle as this device
  • is going away
  */ if (conns[i].remote_fwnode) { fwnode_handle_put(conns[i].remote_fwnode);

• 	pdata->out_conns[i].remote_fwnode = NULL;
  

• 	conns[i].remote_fwnode = NULL;
  

  } }
• for (i = 0; i < pdata->nr_inconns; i++) {

• pdata->in_conns[i].remote_dev = NULL;
  

• pdata->in_conns[i].remote_fwnode = NULL;
  

• }
• if (csdev) coresight_remove_conns_sysfs_group(csdev); }

• struct coresight_device *coresight_register(struct coresight_desc *desc) { int ret;

@@ -1659,6 +1705,8 @@ struct coresight_device *coresight_register(struct coresight_desc *desc) ret = coresight_create_conns_sysfs_group(csdev); if (!ret) ret = coresight_fixup_device_conns(csdev);

• if (!ret)

• ret = coresight_fixup_inputs(csdev);
  

Don't we also need to fixup the "input" connections on this device ? It would be good move all of this connection stuff into a wrapper function.

if (!ret) ret = coresight_fixup_orphan_conns(csdev); diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c index 16553f7dde12..20e3351cbdc2 100644 --- a/drivers/hwtracing/coresight/coresight-platform.c +++ b/drivers/hwtracing/coresight/coresight-platform.c @@ -20,8 +20,7 @@ #include "coresight-priv.h" /*

• • coresight_alloc_conns: Allocate connections record for each output
• • port from the device.

• • coresight_alloc_conns: Allocate connections record for each input/output device.
  */ static int coresight_alloc_conns(struct device *dev, struct coresight_platform_data *pdata)

@@ -33,7 +32,14 @@ static int coresight_alloc_conns(struct device *dev, if (!pdata->out_conns) return -ENOMEM; }

• if (pdata->nr_inconns) {

• pdata->in_conns = devm_krealloc_array(dev, pdata->in_conns,
  

• 				      pdata->nr_inconns,
  

• 				      sizeof(*pdata->in_conns),
  

• 				      GFP_KERNEL | __GFP_ZERO);
  

• if (!pdata->in_conns)
  

• 	return -ENOMEM;
  

• }

devm_krealloc_array() skips any allocation if the requested new_size <= origin_size. So this should be fine for the out_conns, to not realloc the space again unnecessarily.

return 0; } @@ -79,6 +85,45 @@ int coresight_add_conn(struct device *dev, } EXPORT_SYMBOL_GPL(coresight_add_conn);

Should this be named as add_out_conn in the previous patch just to make it explicit and easier to read.

+/*

• • Add a connection in the first free slot, or realloc
• • if there is no space.
• • Do nothing if the connection already exists because inputs are
• • fixed up multiple times.
• */

+int coresight_add_in_conn(struct device *dev,

• 	  struct coresight_platform_data *pdata,
  

minor nit: Is there any reason why this can't simply be:

int coresight_add_in_conn(struct coresight_device *csdev, struct coresight_connection *conn) { struct device *dev = csdev->dev.parent; struct coresight_platform_data *pdata = csdev->pdata;

• 	  struct coresight_connection *conn)
  

Suzuki

+{

• int ret;
• struct coresight_connection *free_conn = NULL;
• int i;
• /* Search for a free slot or exit if a duplicate is found */
• if (pdata->in_conns) {

• for (i = 0; i < pdata->nr_inconns; ++i) {
  

• 	if (!free_conn && !pdata->in_conns[i].remote_fwnode)
  

• 		free_conn = &pdata->in_conns[i];
  

• 	if (pdata->in_conns[i].remote_fwnode ==
  

• 	    conn->remote_fwnode)
  

• 		return 0;
  

• }
  

• }
• if (!free_conn) {

• pdata->nr_inconns++;
  

• ret = coresight_alloc_conns(dev, pdata);
  

• if (ret)
  

• 	return ret;
  

• free_conn = &pdata->in_conns[pdata->nr_inconns - 1];
  

• }
• *free_conn = *conn;
• return 0;

+} +EXPORT_SYMBOL_GPL(coresight_add_in_conn);

• static struct device * coresight_find_device_by_fwnode(struct fwnode_handle *fwnode) {

@@ -249,7 +294,7 @@ static int of_coresight_get_cpu(struct device *dev) /*

• of_coresight_parse_endpoint : Parse the given output endpoint @ep

• • and fill the connection information in @conn

• • and fill the connection information in @in_conn and @out_conn
  • Parses the local port, remote device name and the remote port.

@@ -333,14 +378,14 @@ static int of_get_coresight_platform_data(struct device *dev, /* Get the number of input and output port for this component */ of_coresight_get_ports(node, &pdata->nr_inconns, &pdata->nr_outconns);

• /* If there are no output connections, we are done */
• if (!pdata->nr_outconns)

• return 0;
  

• ret = coresight_alloc_conns(dev, pdata); if (ret) return ret;

• /* If there are no output connections, we are done */
• if (!pdata->nr_outconns)

• return 0;
  

• parent = of_coresight_get_output_ports_node(node); /*
  • If the DT uses obsoleted bindings, the ports are listed

diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c index 3da9868d9237..2abf9639ac0f 100644 --- a/drivers/hwtracing/coresight/coresight-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-sysfs.c @@ -202,5 +202,4 @@ void coresight_remove_links(struct coresight_device *orig, devm_kfree(&orig->dev, conn->link->orig_name); devm_kfree(&orig->dev, conn->link); conn->link = NULL;

• conn->remote_dev = NULL; }

diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 47fa58d6981d..fd268b24c761 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -110,6 +110,7 @@ struct coresight_platform_data { int nr_inconns; int nr_outconns; struct coresight_connection *out_conns;

• struct coresight_connection *in_conns;

Please document the field above the structure.

Suzuki

}; /** @@ -177,6 +178,27 @@ struct coresight_desc {

•           @remote_fwnode once the remote's coresight_device has
  

•           been created.
  

• @link: Representation of the connection as a sysfs link.

• • The full connection structure looks like this, where in_conns store references to
• • the parent device in the same remote_dev member as output connections.

• •   +-----------------------------+            +-----------------------------+
    

• •   |coresight_device             |            |coresight_connection         |
    

• •   |-----------------------------|            |-----------------------------|
    

• • ---->| | | |
• • | | | | remote_dev*|------
• • | |pdata->out_conns[nr_outconns]|----------->| | |
• • | | | | | |
• • | +-----------------------------+ +-----------------------------+ |
• • | |
• • | +-----------------------------+ +-----------------------------+ |
• • | |coresight_connection | |coresight_device | |
• • | |-----------------------------| |------------------------------ |
• • | | | | |<-----
• • -----|remote_dev* | | |

• •   |                             |<-----------|pdata->in_conns[nr_inconns]  |
    

• •   |                             |            |                             |
    

• •   +-----------------------------+            +-----------------------------+
    

  */ struct coresight_connection { int port;

@@ -619,5 +641,8 @@ struct coresight_platform_data *coresight_get_platform_data(struct device *dev); int coresight_add_conn(struct device *dev, struct coresight_platform_data *pdata, const struct coresight_connection *conn); +int coresight_add_in_conn(struct device *dev,

• 	  struct coresight_platform_data *pdata,
  

• 	  struct coresight_connection *conn);
  

#endif /* _LINUX_COREISGHT_H */

On 16/03/2023 20:23, Suzuki K Poulose wrote:

On 10/03/2023 16:06, James Clark wrote:

...

This will allow CATU to get its associated ETR in a generic way where currently the enable path has some hard coded searches which avoid the need to store input connections.

Signed-off-by: James Clark james.clark@arm.com

drivers/hwtracing/coresight/coresight-core.c  | 56 +++++++++++++++--   .../hwtracing/coresight/coresight-platform.c  | 61 ++++++++++++++++---   drivers/hwtracing/coresight/coresight-sysfs.c |  1 -   include/linux/coresight.h                     | 25 ++++++++   4 files changed, 130 insertions(+), 13 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index f457914e445e..a8ba7493c09a 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -59,6 +59,7 @@ const u32 coresight_barrier_pkt[4] = {0x7fffffff, 0x7fffffff, 0x7fffffff, 0x7fff   EXPORT_SYMBOL_GPL(coresight_barrier_pkt);     static const struct cti_assoc_op *cti_assoc_ops; +static int coresight_fixup_inputs(struct coresight_device *csdev);     ssize_t coresight_simple_show_pair(struct device *_dev,                     struct device_attribute *attr, char *buf) @@ -1369,6 +1370,35 @@ static int coresight_fixup_orphan_conns(struct coresight_device *csdev)                csdev, coresight_orphan_match);   }   +/*

• • Device connections are discovered before one/both devices have

been created,

• • so inputs must be added later.
• */

+static int coresight_fixup_inputs(struct coresight_device *csdev) +{ +    int i, ret = 0; +    struct coresight_connection *out_conn; +    struct coresight_connection in_conn;

+    for (i = 0; i < csdev->pdata->nr_outconns; i++) { +        out_conn = &csdev->pdata->out_conns[i]; +        if (!out_conn->remote_dev || !out_conn->remote_dev->pdata) +            continue;

+        /* Reverse local/remote relationships for inputs */ +        in_conn.remote_dev = csdev; +        in_conn.remote_port = out_conn->port; +        in_conn.port = out_conn->remote_port; +        in_conn.remote_fwnode = csdev->dev.fwnode; +        ret = coresight_add_in_conn(out_conn->remote_dev->dev.parent, +                        out_conn->remote_dev->pdata, +                        &in_conn); +        if (ret) +            return ret; +    }

+    return 0; +}     static int coresight_fixup_device_conns(struct coresight_device *csdev)   { @@ -1427,11 +1457,20 @@ static int coresight_remove_match(struct device *dev, void *data)

Removing the connection should be much simpler now with the inports connection tracking.

i.e., coresight_remove_conns() need not iterate over all the devices on the coresight bus. We had to do that to find all the devices connecting to the removed device.

Done in v3

Instead with the in_conns we could readily get the csdev instance and remove the connection from those devices.

Similarly for the out_conns can give you the remove device for removing the in_conns on that device.

i.e,

static void coresight_remove_conns(csdev) {     struct coresight_device *remote;          for (i = 0; i < csdev->pdata->nr_outconns; i++) {         remote = csdev->data->out_conns[i].remote_dev;

if (!remote)             continue;

for (j = 0; j < remote->pdata->nr_inconns; j++) {             if (remote->pdata->in_conns[i].remote_dev ==  csdev)             /* Clear in_conns[i] on remote */                 }         }

Similarly for in_conns     for (i = 0; i < csdev->pdata->nr_inconns; i++) {         remote = csdev->data->in_conns[i].remote_dev;

if (!remote)             continue;

for (j = 0; j < remote->pdata->nr_outconns; j++) {             if (remote->pdata->out_conns[i].remote_dev ==  csdev)             /* Clear out_conns[i] on remote */ }

...

*/               fwnode_handle_put(conn->remote_fwnode);               conn->remote_fwnode = NULL; +            conn->remote_dev = NULL; +            /* No need to continue */ +            break; +        } +    } +    for (i = 0; i < iterator->pdata->nr_inconns; i++) { +        conn = &iterator->pdata->in_conns[i]; +        if (csdev == conn->remote_dev) { +            conn->remote_fwnode = NULL; +            conn->remote_dev = NULL;               /* No need to continue */               break;           }       }

/*        * Returning '0' ensures that all known component on the        * bus will be checked. @@ -1552,21 +1591,28 @@ void coresight_release_platform_data(struct coresight_device *csdev,         for (i = 0; i < pdata->nr_outconns; i++) {           /* If we have made the links, remove them now */ -        if (csdev && conns[i].remote_dev) +        if (csdev && conns[i].remote_dev) {               coresight_remove_links(csdev, &conns[i]); +            conns[i].remote_dev = NULL; +        }

/*            * Drop the refcount and clear the handle as this device            * is going away            */           if (conns[i].remote_fwnode) {               fwnode_handle_put(conns[i].remote_fwnode); -            pdata->out_conns[i].remote_fwnode = NULL; +            conns[i].remote_fwnode = NULL;           }       } +    for (i = 0; i < pdata->nr_inconns; i++) { +        pdata->in_conns[i].remote_dev = NULL; +        pdata->in_conns[i].remote_fwnode = NULL; +    }

if (csdev)           coresight_remove_conns_sysfs_group(csdev);   }

struct coresight_device *coresight_register(struct coresight_desc *desc)   {       int ret; @@ -1659,6 +1705,8 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)       ret = coresight_create_conns_sysfs_group(csdev);       if (!ret)           ret = coresight_fixup_device_conns(csdev); +    if (!ret) +        ret = coresight_fixup_inputs(csdev);

Don't we also need to fixup the "input" connections on this device ?

It would be good move all of this connection stuff into a wrapper function.

Should be simplified now with coresight_fixup_device_conns() dropped.

...

if (!ret)           ret = coresight_fixup_orphan_conns(csdev);   diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c index 16553f7dde12..20e3351cbdc2 100644 --- a/drivers/hwtracing/coresight/coresight-platform.c +++ b/drivers/hwtracing/coresight/coresight-platform.c @@ -20,8 +20,7 @@     #include "coresight-priv.h"   /*

• • coresight_alloc_conns: Allocate connections record for each output
• • port from the device.

• • coresight_alloc_conns: Allocate connections record for each

input/output device.    */   static int coresight_alloc_conns(struct device *dev,                    struct coresight_platform_data *pdata) @@ -33,7 +32,14 @@ static int coresight_alloc_conns(struct device *dev,           if (!pdata->out_conns)               return -ENOMEM;       }

+    if (pdata->nr_inconns) { +        pdata->in_conns = devm_krealloc_array(dev, pdata->in_conns, +                              pdata->nr_inconns, +                              sizeof(*pdata->in_conns), +                              GFP_KERNEL | __GFP_ZERO); +        if (!pdata->in_conns) +            return -ENOMEM; +    }

devm_krealloc_array() skips any allocation if the requested new_size <= origin_size. So this should be fine for the out_conns, to not realloc the space again unnecessarily.

Now out_conns is contiguous so adding always allocates.

...

return 0;   }   @@ -79,6 +85,45 @@ int coresight_add_conn(struct device *dev,   }   EXPORT_SYMBOL_GPL(coresight_add_conn);

Should this be named as add_out_conn in the previous patch just to make it explicit and easier to read.

Done

...

+/*

• • Add a connection in the first free slot, or realloc
• • if there is no space.
• • Do nothing if the connection already exists because inputs are
• • fixed up multiple times.
• */

+int coresight_add_in_conn(struct device *dev, +              struct coresight_platform_data *pdata,

minor nit: Is there any reason why this can't simply be:

int coresight_add_in_conn(struct coresight_device *csdev,                  struct coresight_connection *conn)    {         struct device *dev = csdev->dev.parent;         struct coresight_platform_data *pdata = csdev->pdata;

No I can change it. I was going for consistency with coresight_add_conn() which does need the device argument because it's called both before and after the coresight_device is created. But yeah it makes sense to simplify this one.

...

+              struct coresight_connection *conn)

Suzuki

...

+{ +    int ret; +    struct coresight_connection *free_conn = NULL; +    int i;

+    /* Search for a free slot or exit if a duplicate is found */ +    if (pdata->in_conns) { +        for (i = 0; i < pdata->nr_inconns; ++i) { +            if (!free_conn && !pdata->in_conns[i].remote_fwnode) +                free_conn = &pdata->in_conns[i]; +            if (pdata->in_conns[i].remote_fwnode == +                conn->remote_fwnode) +                return 0; +        } +    }

+    if (!free_conn) { +        pdata->nr_inconns++; +        ret = coresight_alloc_conns(dev, pdata); +        if (ret) +            return ret; +        free_conn = &pdata->in_conns[pdata->nr_inconns - 1]; +    }

+    *free_conn = *conn; +    return 0; +} +EXPORT_SYMBOL_GPL(coresight_add_in_conn);

static struct device *   coresight_find_device_by_fwnode(struct fwnode_handle *fwnode)   { @@ -249,7 +294,7 @@ static int of_coresight_get_cpu(struct device *dev)     /*    * of_coresight_parse_endpoint : Parse the given output endpoint @ep

• • and fill the connection information in @conn

• • and fill the connection information in @in_conn and @out_conn

*    * Parses the local port, remote device name and the remote port.    * @@ -333,14 +378,14 @@ static int of_get_coresight_platform_data(struct device *dev,       /* Get the number of input and output port for this component */       of_coresight_get_ports(node, &pdata->nr_inconns, &pdata->nr_outconns);   -    /* If there are no output connections, we are done */ -    if (!pdata->nr_outconns) -        return 0;

ret = coresight_alloc_conns(dev, pdata);       if (ret)           return ret;   +    /* If there are no output connections, we are done */ +    if (!pdata->nr_outconns) +        return 0;

parent = of_coresight_get_output_ports_node(node);       /*        * If the DT uses obsoleted bindings, the ports are listed diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c index 3da9868d9237..2abf9639ac0f 100644 --- a/drivers/hwtracing/coresight/coresight-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-sysfs.c @@ -202,5 +202,4 @@ void coresight_remove_links(struct coresight_device *orig,       devm_kfree(&orig->dev, conn->link->orig_name);       devm_kfree(&orig->dev, conn->link);       conn->link = NULL; -    conn->remote_dev = NULL;   } diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 47fa58d6981d..fd268b24c761 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -110,6 +110,7 @@ struct coresight_platform_data {       int nr_inconns;       int nr_outconns;       struct coresight_connection *out_conns; +    struct coresight_connection *in_conns;

Please document the field above the structure.

Suzuki

Done

...

};     /** @@ -177,6 +178,27 @@ struct coresight_desc {    *               @remote_fwnode once the remote's coresight_device has    *               been created.    * @link: Representation of the connection as a sysfs link.

• • The full connection structure looks like this, where in_conns

store references to

• • the parent device in the same remote_dev member as output

connections.

• *       +-----------------------------+

+-----------------------------+

• *       |coresight_device             |

|coresight_connection         |

• *       |-----------------------------|

|-----------------------------|

• *  ---->|                             |

|                             |

• *  |    |                             |

|                  remote_dev*|------

• *  |

|pdata->out_conns[nr_outconns]|----------->|                             |      |

• *  |    |                             |

|                             |      |

• *  |    +-----------------------------+

+-----------------------------+      |

• *

|                                                                                    |

• *  |    +-----------------------------+

+-----------------------------+      |

• *  |    |coresight_connection         |

|coresight_device             |      |

• *  |    |-----------------------------|

|------------------------------      |

• *  |    |                             |

|                             |<-----

• *  -----|remote_dev*                  |

|                             |

• *       |

|<-----------|pdata->in_conns[nr_inconns]  |

• *       |                             |

|                             |

• *       +-----------------------------+

+-----------------------------+    */   struct coresight_connection {       int port; @@ -619,5 +641,8 @@ struct coresight_platform_data *coresight_get_platform_data(struct device *dev);   int coresight_add_conn(struct device *dev,                  struct coresight_platform_data *pdata,                  const struct coresight_connection *conn); +int coresight_add_in_conn(struct device *dev, +              struct coresight_platform_data *pdata, +              struct coresight_connection *conn);     #endif        /* _LINUX_COREISGHT_H */

On Fri, 10 Mar 2023 at 16:07, James Clark james.clark@arm.com wrote:

When CATU is moved to the generic enable/disable path system in the next commit, it will need to call into ETR and get it to pre-allocate its buffer so add a function for it.

No functional changes

Signed-off-by: James Clark james.clark@arm.com

.../hwtracing/coresight/coresight-tmc-etr.c | 50 ++++++++++++++++--- drivers/hwtracing/coresight/coresight-tmc.h | 2 + 2 files changed, 45 insertions(+), 7 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index 223081dc3ad4..cb9621003aef 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1169,7 +1169,7 @@ void tmc_etr_disable_hw(struct tmc_drvdata *drvdata) drvdata->etr_buf = NULL; }

-static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) +static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev) { int ret = 0; unsigned long flags; @@ -1192,7 +1192,7 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) /* Allocate memory with the locks released */ free_buf = new_buf = tmc_etr_setup_sysfs_buf(drvdata); if (IS_ERR(new_buf))

•                   return PTR_ERR(new_buf);
  

•                   return new_buf;
  
            /* Let's try again */
            spin_lock_irqsave(&drvdata->spinlock, flags);
  

@@ -1223,17 +1223,33 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) drvdata->sysfs_buf = new_buf; }

•   ret = tmc_etr_enable_hw(drvdata, drvdata->sysfs_buf);
  

•   if (!ret) {
  

•           drvdata->mode = CS_MODE_SYSFS;
  

•           atomic_inc(csdev->refcnt);
  

•   }
  

out: spin_unlock_irqrestore(&drvdata->spinlock, flags);

    /* Free memory outside the spinlock if need be */
    if (free_buf)
            tmc_etr_free_sysfs_buf(free_buf);

•   return ret ? ERR_PTR(ret) : drvdata->sysfs_buf;
  

+}

+static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) +{

•   int ret;
  

•   unsigned long flags;
  

•   struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
  

•   struct etr_buf *sysfs_buf = tmc_etr_get_sysfs_buffer(csdev);
  

•   if (IS_ERR(sysfs_buf))
  

•           return PTR_ERR(sysfs_buf);
  

•   spin_lock_irqsave(&drvdata->spinlock, flags);
  

•   ret = tmc_etr_enable_hw(drvdata, sysfs_buf);
  

•   if (!ret) {
  

•           drvdata->mode = CS_MODE_SYSFS;
  

•           atomic_inc(csdev->refcnt);
  

•   }
  

•   spin_unlock_irqrestore(&drvdata->spinlock, flags);
  
    if (!ret)
            dev_dbg(&csdev->dev, "TMC-ETR enabled\n");
  

@@ -1241,6 +1257,26 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) return ret; }

+struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,

•                              enum cs_mode mode, void *data)
  

+{

•   struct perf_output_handle *handle = data;
  

•   struct etr_perf_buffer *etr_perf;
  

•   switch (mode) {
  

•   case CS_MODE_SYSFS:
  

•           return tmc_etr_get_sysfs_buffer(csdev);
  

•   case CS_MODE_PERF:
  

•           etr_perf = etm_perf_sink_config(handle);
  

•           if (WARN_ON(!etr_perf || !etr_perf->etr_buf))
  

•                   return ERR_PTR(-EINVAL);
  

•           return etr_perf->etr_buf;
  

•   default:
  

•           return ERR_PTR(-EINVAL);
  

•   }
  

+} +EXPORT_SYMBOL_GPL(tmc_etr_get_buffer);

/*

• alloc_etr_buf: Allocate ETR buffer for use by perf.
• The size of the hardware buffer is dependent on the size configured

diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index 01c0382a29c0..b97da39652d2 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -332,5 +332,7 @@ struct coresight_device *tmc_etr_get_catu_device(struct tmc_drvdata *drvdata);

void tmc_etr_set_catu_ops(const struct etr_buf_operations *catu); void tmc_etr_remove_catu_ops(void); +struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,

•                              enum cs_mode mode, void *data);
  
  

#endif

2.34.1

Reviewed by: Mike Leach mike.leach@linaro.org

On 10/03/2023 16:06, James Clark wrote:

Currently CATU is the only helper device, and its enable and disable calls are hard coded. To allow more helper devices to be added in a generic way, remove these hard coded calls and just enable and disable all helper devices.

This has to apply to helpers adjacent to the path, because they will never be in the path. CATU was already discovered in this way, so there is no change there.

One change that is needed is for CATU to call back into ETR to allocate the buffer. Because the enable call was previously hard coded, it was done at a point where the buffer was already allocated, but this is no longer the case.

Signed-off-by: James Clark james.clark@arm.com

drivers/hwtracing/coresight/coresight-catu.c | 34 ++++++++-- drivers/hwtracing/coresight/coresight-core.c | 68 ++++++++++++++++++- .../hwtracing/coresight/coresight-tmc-etr.c | 28 -------- include/linux/coresight.h | 3 +- 4 files changed, 99 insertions(+), 34 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c index bc90a03f478f..24a08a2b96b1 100644 --- a/drivers/hwtracing/coresight/coresight-catu.c +++ b/drivers/hwtracing/coresight/coresight-catu.c @@ -395,13 +395,32 @@ static inline int catu_wait_for_ready(struct catu_drvdata *drvdata) return coresight_timeout(csa, CATU_STATUS, CATU_STATUS_READY, 1); } -static int catu_enable_hw(struct catu_drvdata *drvdata, void *data) +static struct coresight_device * +catu_get_etr_device(struct coresight_device *csdev) +{

• int i;
• struct coresight_device *tmp;
• for (i = 0; i < csdev->pdata->nr_inconns; i++) {

• tmp = csdev->pdata->in_conns[i].remote_dev;
  

• if (tmp && tmp->type == CORESIGHT_DEV_TYPE_SINK &&
  

•     tmp->subtype.sink_subtype ==
  

• 	    CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM)
  

• 	return tmp;
  

• }
• return NULL;

+}

+static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode cs_mode,

• 	  void *data)
  

  { int rc; u32 control, mode;

• struct etr_buf *etr_buf = data;

• struct etr_buf *etr_buf = NULL; struct device *dev = &drvdata->csdev->dev; struct coresight_device *csdev = drvdata->csdev;
• struct coresight_device *etrdev;

if (catu_wait_for_ready(drvdata)) dev_warn(dev, "Timeout while waiting for READY\n"); @@ -416,6 +435,12 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, void *data) if (rc) return rc;

• etrdev = catu_get_etr_device(csdev);
• if (etrdev) {

• etr_buf = tmc_etr_get_buffer(etrdev, cs_mode, data);
  

• if (IS_ERR(etr_buf))
  

• 	return PTR_ERR(etr_buf);
  

• }

WARN_ON(!etrdev) ? We are not supposed to reach in the first place and return.

control |= BIT(CATU_CONTROL_ENABLE); if (etr_buf && etr_buf->mode == ETR_MODE_CATU) { @@ -441,13 +466,14 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, void *data) return 0; } -static int catu_enable(struct coresight_device *csdev, void *data) +static int catu_enable(struct coresight_device *csdev, enum cs_mode mode,

•        void *data)
  

  { int rc; struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev);

CS_UNLOCK(catu_drvdata->base);

• rc = catu_enable_hw(catu_drvdata, data);

• rc = catu_enable_hw(catu_drvdata, mode, data); CS_LOCK(catu_drvdata->base); return rc; }

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index a8ba7493c09a..3e6ccd9e8d4e 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -441,6 +441,34 @@ static int coresight_enable_source(struct coresight_device *csdev, return 0; } +static int coresight_enable_helper(struct coresight_device *csdev,

• 		   enum cs_mode mode, void *sink_data)
  

minor nit: s/sink_data/data/ ? Though it is always either sink_data (perf mode) or NULL (sysfs mode), for the core code it is simply an opaque data.

Rest looks fine to me.

Suzuki

On 17/03/2023 11:04, Suzuki K Poulose wrote:

On 10/03/2023 16:06, James Clark wrote:

...

Currently CATU is the only helper device, and its enable and disable calls are hard coded. To allow more helper devices to be added in a generic way, remove these hard coded calls and just enable and disable all helper devices.

This has to apply to helpers adjacent to the path, because they will never be in the path. CATU was already discovered in this way, so there is no change there.

One change that is needed is for CATU to call back into ETR to allocate the buffer. Because the enable call was previously hard coded, it was done at a point where the buffer was already allocated, but this is no longer the case.

Signed-off-by: James Clark james.clark@arm.com

drivers/hwtracing/coresight/coresight-catu.c  | 34 ++++++++--   drivers/hwtracing/coresight/coresight-core.c  | 68 ++++++++++++++++++-   .../hwtracing/coresight/coresight-tmc-etr.c   | 28 --------   include/linux/coresight.h                     |  3 +-   4 files changed, 99 insertions(+), 34 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c index bc90a03f478f..24a08a2b96b1 100644 --- a/drivers/hwtracing/coresight/coresight-catu.c +++ b/drivers/hwtracing/coresight/coresight-catu.c @@ -395,13 +395,32 @@ static inline int catu_wait_for_ready(struct catu_drvdata *drvdata)       return coresight_timeout(csa, CATU_STATUS, CATU_STATUS_READY, 1);   }   -static int catu_enable_hw(struct catu_drvdata *drvdata, void *data) +static struct coresight_device * +catu_get_etr_device(struct coresight_device *csdev) +{ +    int i; +    struct coresight_device *tmp;

+    for (i = 0; i < csdev->pdata->nr_inconns; i++) { +        tmp = csdev->pdata->in_conns[i].remote_dev; +        if (tmp && tmp->type == CORESIGHT_DEV_TYPE_SINK && +            tmp->subtype.sink_subtype == +                CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM) +            return tmp; +    }

+    return NULL; +}

+static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode cs_mode, +              void *data)   {       int rc;       u32 control, mode; -    struct etr_buf *etr_buf = data; +    struct etr_buf *etr_buf = NULL;       struct device *dev = &drvdata->csdev->dev;       struct coresight_device *csdev = drvdata->csdev; +    struct coresight_device *etrdev;         if (catu_wait_for_ready(drvdata))           dev_warn(dev, "Timeout while waiting for READY\n"); @@ -416,6 +435,12 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, void *data)       if (rc)           return rc;   +    etrdev = catu_get_etr_device(csdev); +    if (etrdev) { +        etr_buf = tmc_etr_get_buffer(etrdev, cs_mode, data); +        if (IS_ERR(etr_buf)) +            return PTR_ERR(etr_buf); +    }

WARN_ON(!etrdev) ? We are not supposed to reach in the first place and return.

I saw there was the pass-through mode below which I thought didn't need an ETR device. I think I followed the code through and there was a way for it to get there without an ETR in the existing version, but now I'm not sure. Or does it still need the ETR device but it just doesn't access the buffer?

...

control |= BIT(CATU_CONTROL_ENABLE);         if (etr_buf && etr_buf->mode == ETR_MODE_CATU) { @@ -441,13 +466,14 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, void *data)       return 0;   }   -static int catu_enable(struct coresight_device *csdev, void *data) +static int catu_enable(struct coresight_device *csdev, enum cs_mode mode, +               void *data)   {       int rc;       struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev);         CS_UNLOCK(catu_drvdata->base); -    rc = catu_enable_hw(catu_drvdata, data); +    rc = catu_enable_hw(catu_drvdata, mode, data);       CS_LOCK(catu_drvdata->base);       return rc;   } diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index a8ba7493c09a..3e6ccd9e8d4e 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -441,6 +441,34 @@ static int coresight_enable_source(struct coresight_device *csdev,       return 0;   }   +static int coresight_enable_helper(struct coresight_device *csdev, +                   enum cs_mode mode, void *sink_data)

minor nit: s/sink_data/data/ ? Though it is always either sink_data (perf mode) or NULL (sysfs mode), for the core code it is simply an opaque data.

Done

Rest looks fine to me.

Suzuki

On 29/03/2023 14:23, Suzuki K Poulose wrote:

On 29/03/2023 13:04, James Clark wrote:

...

On 17/03/2023 11:04, Suzuki K Poulose wrote:

...

On 10/03/2023 16:06, James Clark wrote:

...

Currently CATU is the only helper device, and its enable and disable calls are hard coded. To allow more helper devices to be added in a generic way, remove these hard coded calls and just enable and disable all helper devices.

This has to apply to helpers adjacent to the path, because they will never be in the path. CATU was already discovered in this way, so there is no change there.

One change that is needed is for CATU to call back into ETR to allocate the buffer. Because the enable call was previously hard coded, it was done at a point where the buffer was already allocated, but this is no longer the case.

Signed-off-by: James Clark james.clark@arm.com

drivers/hwtracing/coresight/coresight-catu.c  | 34 ++++++++--    drivers/hwtracing/coresight/coresight-core.c  | 68 ++++++++++++++++++-    .../hwtracing/coresight/coresight-tmc-etr.c   | 28 --------    include/linux/coresight.h                     |  3 +-    4 files changed, 99 insertions(+), 34 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c index bc90a03f478f..24a08a2b96b1 100644 --- a/drivers/hwtracing/coresight/coresight-catu.c +++ b/drivers/hwtracing/coresight/coresight-catu.c @@ -395,13 +395,32 @@ static inline int catu_wait_for_ready(struct catu_drvdata *drvdata)        return coresight_timeout(csa, CATU_STATUS, CATU_STATUS_READY, 1);    }    -static int catu_enable_hw(struct catu_drvdata *drvdata, void *data) +static struct coresight_device * +catu_get_etr_device(struct coresight_device *csdev) +{ +    int i; +    struct coresight_device *tmp;

+    for (i = 0; i < csdev->pdata->nr_inconns; i++) { +        tmp = csdev->pdata->in_conns[i].remote_dev; +        if (tmp && tmp->type == CORESIGHT_DEV_TYPE_SINK && +            tmp->subtype.sink_subtype == +                CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM) +            return tmp; +    }

+    return NULL; +}

+static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode cs_mode, +              void *data)    {        int rc;        u32 control, mode; -    struct etr_buf *etr_buf = data; +    struct etr_buf *etr_buf = NULL;        struct device *dev = &drvdata->csdev->dev;        struct coresight_device *csdev = drvdata->csdev; +    struct coresight_device *etrdev;          if (catu_wait_for_ready(drvdata))            dev_warn(dev, "Timeout while waiting for READY\n"); @@ -416,6 +435,12 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, void *data)        if (rc)            return rc;    +    etrdev = catu_get_etr_device(csdev); +    if (etrdev) { +        etr_buf = tmc_etr_get_buffer(etrdev, cs_mode, data); +        if (IS_ERR(etr_buf)) +            return PTR_ERR(etr_buf); +    }

WARN_ON(!etrdev) ? We are not supposed to reach in the first place and return.

I saw there was the pass-through mode below which I thought didn't need an ETR device. I think I followed the code through and there was a way for it to get there without an ETR in the existing version, but now I'm not sure.

...

Or does it still need the ETR device but it just doesn't access the buffer?

The first part is correct. Without an ETR, CATU wouldn't be a helper device, and wouldn't get here in "enable CATU" via the helper route. The CATU chooses the mode depending on the etr_buf mode.

Ok thanks I will add that warning then

Suzuki

On 10/03/2023 16:06, James Clark wrote:

The CTI module has some hard coded refcounting code that has a leak. For example running perf and then trying to unload it fails:

perf record -e cs_etm// -a -- ls rmmod coresight_cti

rmmod: ERROR: Module coresight_cti is in use

The coresight core already handles references of devices in use, so by making CTI a normal helper device, we get working refcounting for free.

Signed-off-by: James Clark james.clark@arm.com

drivers/hwtracing/coresight/coresight-core.c | 73 +++---------------- .../hwtracing/coresight/coresight-cti-core.c | 56 +++++++++----- .../hwtracing/coresight/coresight-cti-sysfs.c | 4 +- drivers/hwtracing/coresight/coresight-cti.h | 4 +- include/linux/coresight.h | 28 +------ 5 files changed, 53 insertions(+), 112 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index 3e6ccd9e8d4e..94d84404fb29 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -253,48 +253,6 @@ void coresight_disclaim_device(struct coresight_device *csdev) } EXPORT_SYMBOL_GPL(coresight_disclaim_device); -/* enable or disable an associated CTI device of the supplied CS device */ -static int -coresight_control_assoc_ectdev(struct coresight_device *csdev, bool enable) -{

• int ect_ret = 0;
• struct coresight_device *ect_csdev = csdev->ect_dev;
• struct module *mod;
• if (!ect_csdev)

• return 0;
  

• if ((!ect_ops(ect_csdev)->enable) || (!ect_ops(ect_csdev)->disable))

• return 0;
  

• mod = ect_csdev->dev.parent->driver->owner;
• if (enable) {

• if (try_module_get(mod)) {
  

• 	ect_ret = ect_ops(ect_csdev)->enable(ect_csdev);
  

• 	if (ect_ret) {
  

• 		module_put(mod);
  

• 	} else {
  

• 		get_device(ect_csdev->dev.parent);
  

• 		csdev->ect_enabled = true;
  

• 	}
  

• } else
  

• 	ect_ret = -ENODEV;
  

• } else {

• if (csdev->ect_enabled) {
  

• 	ect_ret = ect_ops(ect_csdev)->disable(ect_csdev);
  

• 	put_device(ect_csdev->dev.parent);
  

• 	module_put(mod);
  

• 	csdev->ect_enabled = false;
  

• }
  

• }
• /* output warning if ECT enable is preventing trace operation */
• if (ect_ret)

• dev_info(&csdev->dev, "Associated ECT device (%s) %s failed\n",
  

• 	 dev_name(&ect_csdev->dev),
  

• 	 enable ? "enable" : "disable");
  

• return ect_ret;

-}

• /*
  • Set the associated ect / cti device while holding the coresight_mutex
  • to avoid a race with coresight_enable that may try to use this value.

@@ -302,8 +260,14 @@ coresight_control_assoc_ectdev(struct coresight_device *csdev, bool enable) void coresight_set_assoc_ectdev_mutex(struct coresight_device *csdev, struct coresight_device *ect_csdev) {

• struct coresight_connection conn = {};
• mutex_lock(&coresight_mutex);

• csdev->ect_dev = ect_csdev;

• conn.remote_fwnode = fwnode_handle_get(dev_fwnode(&ect_csdev->dev));
• conn.remote_dev = ect_csdev;
• conn.remote_port = conn.port = -1;
• coresight_add_conn(csdev->dev.parent, csdev->pdata, &conn);

• coresight_fixup_inputs(csdev);

Can this cause duplicate entries to be created for the other "existing" output connections of csdev ? IIUC, we just need to create the input connection for the connection we are creating now and not all of the out_conns. They should have been populated at coresight_register(). Now you may not hit this, because we don't have any actual "defined" connections in the DT. But we need to be safe here.

mutex_unlock(&coresight_mutex); } EXPORT_SYMBOL_GPL(coresight_set_assoc_ectdev_mutex); @@ -320,12 +284,8 @@ static int coresight_enable_sink(struct coresight_device *csdev, if (!sink_ops(csdev)->enable) return -EINVAL;

• ret = coresight_control_assoc_ectdev(csdev, true);
• if (ret)

• return ret;
  

  ret = sink_ops(csdev)->enable(csdev, mode, data); if (ret) {

• coresight_control_assoc_ectdev(csdev, false);
  

  return ret; }

super minor nit: You could remove the { } too

csdev->enable = true; @@ -343,7 +303,6 @@ static void coresight_disable_sink(struct coresight_device *csdev) ret = sink_ops(csdev)->disable(csdev); if (ret) return;

• coresight_control_assoc_ectdev(csdev, false); csdev->enable = false; }

@@ -368,17 +327,11 @@ static int coresight_enable_link(struct coresight_device *csdev, return outport; if (link_ops(csdev)->enable) {

• ret = coresight_control_assoc_ectdev(csdev, true);
  

• if (!ret) {
  

• 	ret = link_ops(csdev)->enable(csdev, inport, outport);
  

• 	if (ret)
  

• 		coresight_control_assoc_ectdev(csdev, false);
  

• }
  

• ret = link_ops(csdev)->enable(csdev, inport, outport);
  

• if (!ret)
  

• 	csdev->enable = true;
  

  }

• if (!ret)

• csdev->enable = true;
  

• return ret; }

@@ -407,7 +360,6 @@ static void coresight_disable_link(struct coresight_device *csdev, if (link_ops(csdev)->disable) { link_ops(csdev)->disable(csdev, inport, outport);

• coresight_control_assoc_ectdev(csdev, false);
  

  }

super minor nit: You may remove the { } s.

for (i = 0; i < nr_conns; i++) @@ -424,12 +376,8 @@ static int coresight_enable_source(struct coresight_device *csdev, if (!csdev->enable) { if (source_ops(csdev)->enable) {

• 	ret = coresight_control_assoc_ectdev(csdev, true);
  

• 	if (ret)
  

• 		return ret;
  ret = source_ops(csdev)->enable(csdev, NULL, mode);
  if (ret) {
  

• 		coresight_control_assoc_ectdev(csdev, false);
  	return ret;
  }
  

  }

@@ -482,7 +430,6 @@ static bool coresight_disable_source(struct coresight_device *csdev) if (atomic_dec_return(csdev->refcnt) == 0) { if (source_ops(csdev)->disable) source_ops(csdev)->disable(csdev, NULL);

• coresight_control_assoc_ectdev(csdev, false);
  

  csdev->enable = false; } return !csdev->enable;

diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c index 277c890a1f1f..dbce6680759f 100644 --- a/drivers/hwtracing/coresight/coresight-cti-core.c +++ b/drivers/hwtracing/coresight/coresight-cti-core.c @@ -555,7 +555,10 @@ static void cti_add_assoc_to_csdev(struct coresight_device *csdev) mutex_lock(&ect_mutex); /* exit if current is an ECT device.*/

• if ((csdev->type == CORESIGHT_DEV_TYPE_ECT) || list_empty(&ect_net))

• if ((csdev->type == CORESIGHT_DEV_TYPE_HELPER &&

•     csdev->subtype.helper_subtype ==
  

•      CORESIGHT_DEV_SUBTYPE_HELPER_ECT_CTI) ||
  

Should we add a helper to check this : is_coresight_ctidev(csdev) ?

•    list_empty(&ect_net))
  

  goto cti_add_done;

/* if we didn't find the csdev previously we used the fwnode name */ @@ -580,6 +583,22 @@ static void cti_add_assoc_to_csdev(struct coresight_device *csdev) mutex_unlock(&ect_mutex); } +static struct coresight_device *cti__get_cti_device(struct coresight_device *csdev)

minor nit: no perf tool naming style please ;-)

+{

• int i;
• struct coresight_device *tmp;
• for (i = 0; i < csdev->pdata->nr_outconns; i++) {

• tmp = csdev->pdata->out_conns[i].remote_dev;
  

• if (tmp && tmp->type == CORESIGHT_DEV_TYPE_HELPER &&
  

•     tmp->subtype.helper_subtype ==
  

• 	    CORESIGHT_DEV_SUBTYPE_HELPER_ECT_CTI)
  

As mentioned above, a helper to check this would benefit reading.

• 	return tmp;
  

• }
• return NULL;

+}

• /*
  • Removing the associated devices is easier.
  • A CTI will not have a value for csdev->ect_dev.

@@ -588,20 +607,21 @@ static void cti_remove_assoc_from_csdev(struct coresight_device *csdev) { struct cti_drvdata *ctidrv; struct cti_trig_con *tc;

• struct coresight_device *cti_csdev = cti__get_cti_device(csdev); struct cti_device *ctidev;

• if (!cti_csdev)

• return;
  

• mutex_lock(&ect_mutex);

• if (csdev->ect_dev) {

• ctidrv = csdev_to_cti_drvdata(csdev->ect_dev);
  

• ctidev = &ctidrv->ctidev;
  

• list_for_each_entry(tc, &ctidev->trig_cons, node) {
  

• 	if (tc->con_dev == csdev) {
  

• 		cti_remove_sysfs_link(ctidrv, tc);
  

• 		tc->con_dev = NULL;
  

• 		break;
  

• 	}
  

• ctidrv = csdev_to_cti_drvdata(cti_csdev);
• ctidev = &ctidrv->ctidev;
• list_for_each_entry(tc, &ctidev->trig_cons, node) {

• if (tc->con_dev == csdev) {
  

• 	cti_remove_sysfs_link(ctidrv, tc);
  

• 	tc->con_dev = NULL;
  

• 	break;
  

  }

• csdev->ect_dev = NULL;
  

  } mutex_unlock(&ect_mutex); }

@@ -646,8 +666,6 @@ static void cti_remove_conn_xrefs(struct cti_drvdata *drvdata) list_for_each_entry(tc, &ctidev->trig_cons, node) { if (tc->con_dev) {

• 	coresight_set_assoc_ectdev_mutex(tc->con_dev,
  

• 					 NULL);
  cti_remove_sysfs_link(drvdata, tc);
  tc->con_dev = NULL;
  

  }

@@ -795,27 +813,27 @@ static void cti_pm_release(struct cti_drvdata *drvdata) } /** cti ect operations **/ -int cti_enable(struct coresight_device *csdev) +int cti_enable(struct coresight_device *csdev, enum cs_mode mode, void *data) { struct cti_drvdata *drvdata = csdev_to_cti_drvdata(csdev); return cti_enable_hw(drvdata); } -int cti_disable(struct coresight_device *csdev) +int cti_disable(struct coresight_device *csdev, void *data) { struct cti_drvdata *drvdata = csdev_to_cti_drvdata(csdev); return cti_disable_hw(drvdata); } -static const struct coresight_ops_ect cti_ops_ect = { +static const struct coresight_ops_helper cti_ops_ect = { .enable = cti_enable, .disable = cti_disable, }; static const struct coresight_ops cti_ops = {

• .ect_ops = &cti_ops_ect,

• .helper_ops = &cti_ops_ect, };

/* @@ -922,8 +940,8 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id) /* set up coresight component description */ cti_desc.pdata = pdata;

• cti_desc.type = CORESIGHT_DEV_TYPE_ECT;
• cti_desc.subtype.ect_subtype = CORESIGHT_DEV_SUBTYPE_ECT_CTI;

• cti_desc.type = CORESIGHT_DEV_TYPE_HELPER;
• cti_desc.subtype.helper_subtype = CORESIGHT_DEV_SUBTYPE_HELPER_ECT_CTI; cti_desc.ops = &cti_ops; cti_desc.groups = drvdata->ctidev.con_groups; cti_desc.dev = dev;

diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c index e528cff9d4e2..d25dd2737b49 100644 --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c @@ -112,11 +112,11 @@ static ssize_t enable_store(struct device *dev, ret = pm_runtime_resume_and_get(dev->parent); if (ret) return ret;

• ret = cti_enable(drvdata->csdev);
  

• ret = cti_enable(drvdata->csdev, CS_MODE_SYSFS, NULL);
  

  if (ret) pm_runtime_put(dev->parent); } else {

• ret = cti_disable(drvdata->csdev);
  

• ret = cti_disable(drvdata->csdev, NULL);
  

  if (!ret) pm_runtime_put(dev->parent); }

diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h index 8b106b13a244..cb9ee616d01f 100644 --- a/drivers/hwtracing/coresight/coresight-cti.h +++ b/drivers/hwtracing/coresight/coresight-cti.h @@ -215,8 +215,8 @@ int cti_add_connection_entry(struct device *dev, struct cti_drvdata *drvdata, const char *assoc_dev_name); 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); +int cti_enable(struct coresight_device *csdev, enum cs_mode mode, void *data); +int cti_disable(struct coresight_device *csdev, void *data); 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); diff --git a/include/linux/coresight.h b/include/linux/coresight.h index c6ee1634d813..cf7a8658ee88 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -40,8 +40,7 @@ enum coresight_dev_type { CORESIGHT_DEV_TYPE_LINK, CORESIGHT_DEV_TYPE_LINKSINK, CORESIGHT_DEV_TYPE_SOURCE,

• CORESIGHT_DEV_TYPE_HELPER,
• CORESIGHT_DEV_TYPE_ECT,

• CORESIGHT_DEV_TYPE_HELPER };

enum coresight_dev_subtype_sink { @@ -66,12 +65,7 @@ enum coresight_dev_subtype_source { enum coresight_dev_subtype_helper { CORESIGHT_DEV_SUBTYPE_HELPER_CATU, -};

-/* Embedded Cross Trigger (ECT) sub-types */ -enum coresight_dev_subtype_ect {

• CORESIGHT_DEV_SUBTYPE_ECT_NONE,
• CORESIGHT_DEV_SUBTYPE_ECT_CTI,

• CORESIGHT_DEV_SUBTYPE_HELPER_ECT_CTI };

/** @@ -84,8 +78,6 @@ enum coresight_dev_subtype_ect {

• 	by @coresight_dev_subtype_source.
  

• @helper_subtype: type of helper this component is, as defined

• 	by @coresight_dev_subtype_helper.
  

• • @ect_subtype: type of cross trigger this component is, as

• • 	defined by @coresight_dev_subtype_ect
    

  */ union coresight_dev_subtype { /* We have some devices which acts as LINK and SINK */

@@ -95,7 +87,6 @@ union coresight_dev_subtype { }; enum coresight_dev_subtype_source source_subtype; enum coresight_dev_subtype_helper helper_subtype;

• enum coresight_dev_subtype_ect ect_subtype; };

/** @@ -264,12 +255,9 @@ struct coresight_device { bool activated; /* true only if a sink is part of a path */ struct dev_ext_attribute *ea; struct coresight_device *def_sink;

• /* cross trigger handling */
• struct coresight_device *ect_dev; /* sysfs links between components */ int nr_links; bool has_conns_grp;
• bool ect_enabled; /* true only if associated ect device is enabled */

nit: Update comments above the struct too ?

Thanks Suzuki

/* system configuration and feature lists */ struct list_head feature_csdev_list; struct list_head config_csdev_list; @@ -377,23 +365,11 @@ struct coresight_ops_helper { int (*disable)(struct coresight_device *csdev, void *data); }; -/**

• • struct coresight_ops_ect - Ops for an embedded cross trigger device
• • @enable : Enable the device
• • @disable : Disable the device
• */

-struct coresight_ops_ect {

• int (*enable)(struct coresight_device *csdev);
• int (*disable)(struct coresight_device *csdev);

-};

• struct coresight_ops { const struct coresight_ops_sink *sink_ops; const struct coresight_ops_link *link_ops; const struct coresight_ops_source *source_ops; const struct coresight_ops_helper *helper_ops;
• const struct coresight_ops_ect *ect_ops; };

#if IS_ENABLED(CONFIG_CORESIGHT)