Re: [RFC PATCH v2 05/13] drivers: coresight: cti: Enable CTI associated with devices.

Hi Mathieu,

On Tue, 14 May 2019 at 17:41, Mathieu Poirier mathieu.poirier@linaro.org wrote:

On Wed, May 01, 2019 at 09:49:37AM +0100, Mike Leach wrote:

...

The CoreSight subsystem enables a path of devices from source to sink. Any CTI devices associated with the path devices must be enabled at the same time.

This patch adds an associated device element to the main coresight device structure, and uses this to create associations between the CTI and other devices based on the device tree data. The assoc dev element is used to enable CTI in conjunction with the path elements. CTI are reference counted so where a single CTI is associated with multiple elements on the path, it will be enabled on the first associated device enable, and disabled with the last associated device disable.

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

drivers/hwtracing/coresight/coresight-cti.c | 90 ++++++++++++++++++++ drivers/hwtracing/coresight/coresight-priv.h | 3 + drivers/hwtracing/coresight/coresight.c | 39 ++++++++- include/linux/coresight.h | 4 + 4 files changed, 135 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c index 32d9bf0bf0b9..6d452ca3725c 100644 --- a/drivers/hwtracing/coresight/coresight-cti.c +++ b/drivers/hwtracing/coresight/coresight-cti.c @@ -464,6 +464,93 @@ void cti_write_intack(struct device *dev, u32 ackval) spin_unlock(&drvdata->spinlock); }

+/*

• • Look for a matching connection device name in the list of
• • connections. If found then swap in the csdev name and return
• • found.
• */

+static int +cti_match_con_name(struct cti_device *ctidev, const char *node_name,

•            const char *csdev_name)
  

+{

• int found = 0;
  

• struct cti_trig_con *trig_con;
  

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

•         if (trig_con->con_dev_name) {
  

•                 if (!strcmp(node_name, trig_con->con_dev_name)) {
  

•                         found = 1;
  

•                         /* match: so swap in csdev name */
  

•                         kfree(trig_con->con_dev_name);
  

•                         trig_con->con_dev_name =
  

•                                 kstrdup(csdev_name, GFP_KERNEL);
  

I'm not sure I understand why we have to swap here but I suspect it is related to device ordering and probing. I won't pay too much attention to it for now since it will likely have to be modified once you've rebased on Suzuki's ACPI V3 patchset.

Correct - it is to do with probing and matching ordering. This reflects the current "devicetree only" state of the art and will have to be revisited once the standardised naming schema for CS is introduced with ACPI.

...

•                         goto cti_con_name_match;
  

•                 }
  

•         }
  

• }
  

+cti_con_name_match:

• return found;
  

+}

+/*

• • Search the cti list to add an associated CTI into the supplied CS device
• • This will set the association if CTI declared before the CS device
• */

+int cti_add_assoc_to_csdev(struct coresight_device *csdev) +{

• struct ect_node *ect_item;
  

• struct cti_device *ctidev;
  

• const char *node_name = NULL, *csdev_name;
  

• /* exit early for no CTIs or current is an ECT device.*/
  

• if (!cti_count || (csdev->type == CORESIGHT_DEV_TYPE_ECT))
  

•         return 0;
  

• if (!csdev->dev.parent->of_node)
  

•         return 0;
  

• node_name = csdev->dev.parent->of_node->full_name;
  

• if (!node_name)
  

•         return 0;
  

• csdev_name = dev_name(&csdev->dev);
  

• /* for each CTI in list... */
  

• mutex_lock(&ect_mutex);
  

• list_for_each_entry(ect_item, &ect_net, next) {
  

•         ctidev = &ect_item->cti_drv->ctidev;
  

•         if (cti_match_con_name(ctidev, node_name, csdev_name)) {
  

•                 /*
  

•                  * if we found a matching name then update the
  

•                  * association pointers.
  

•                  */
  

•                 csdev->ect_dev = ect_item->cti_drv->csdev;
  

•                 goto cti_found;
  

•         }
  

• }
  

+cti_found:

• mutex_unlock(&ect_mutex);
  

• return 0;
  

+} +EXPORT_SYMBOL_GPL(cti_add_assoc_to_csdev);

+/*

• • Update the cross references where the associated device was found
• • while we were building the connection info. This will occur if the
• • assoc device was registered before the CTI.
• */

+static void cti_update_conn_xrefs(struct cti_drvdata *drvdata) +{

• struct cti_trig_con *tc;
  

• struct cti_device *ctidev = &drvdata->ctidev;
  

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

•         if (tc->con_dev)
  

•                 tc->con_dev->ect_dev = drvdata->csdev;
  

• }
  

+}

/** cti ect operations **/ int cti_enable(struct coresight_device *csdev, void *__unused) { @@ -609,6 +696,9 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id) } mutex_unlock(&ect_mutex);

• /* set any cross references */
  

• cti_update_conn_xrefs(drvdata);
  

• /* all done - dec pm refcount */
  pm_runtime_put(&adev->dev);
  if (drvdata->ctidev.cpu >= 0) {
  

diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index 9b497e2f8725..b1fca5c4bdb2 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -165,8 +165,11 @@ static inline int etm_writel_cp14(u32 off, u32 val) { return 0; }

#ifdef CONFIG_CORESIGHT_CTI extern void cti_device_release(struct device *dev); +extern int cti_add_assoc_to_csdev(struct coresight_device *csdev); #else static inline void cti_device_release(struct device *dev) {}; +static inline int cti_add_assoc_to_csdev(struct coresight_device *csdev) +{ return 0; } #endif

diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c index 8db3d1a5314f..9d56b3b32172 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight.c @@ -216,6 +216,31 @@ void coresight_disclaim_device(void __iomem *base) CS_LOCK(base); }

+/* enable or disable an associated CTI device of the supplied CS device */ +static int +coresight_control_assoc_ectdev(struct coresight_device *csdev, int enable) +{

• int ect_ret = 0;
  

• struct coresight_device *ect_csdev = csdev->ect_dev;
  

• if (!ect_csdev)
  

•         return 0;
  

• if (enable) {
  

•         if (ect_ops(ect_csdev)->enable)
  

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

• } else {
  

•         if (ect_ops(ect_csdev)->disable)
  

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

• }
  

• /* 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");
  

Yes, this could be useful.

...

• return ect_ret;
  

+}

static int coresight_enable_sink(struct coresight_device *csdev, u32 mode, void *data) { @@ -227,6 +252,9 @@ static int coresight_enable_sink(struct coresight_device *csdev, */ if (sink_ops(csdev)->enable) { ret = sink_ops(csdev)->enable(csdev, mode, data);

•         if (ret)
  

•                 return ret;
  

•         ret = coresight_control_assoc_ectdev(csdev, 1);
          if (ret)
                  return ret;
  

In case of error the sink is left enabled.

good point - I'll have to recheck this - though if an error is seen in the "enable path" code in the coresight enable path process, does the "disable path" not unwind everything anyway?

Also, wouldn't it be best to enable the ect device first and then the sink? Otherwise if the sink wrongly emits ECT signals when enabled we'll catch the condition all the time. In the case where the ECT is enabled after the sink we will never know if the wrongly emitted signals will be caught, leading to intermittent behavior.

May not have a choice - CTIs can be connected to multiple devices, so any given CTI - especially ones not bound to cores can be enabled by association with any of these devices. The CTI enable process has a refcount - enable on first enable request, disable on last disable request.

I would tend to favour a sink / associated device first - CTI second model as programming / enable of the associated device should in general be done in such a way as to not have triggers active on enable. If enabling a sink triggers some output signals - that are typically FULL or ACQCOMP, then that is either a sink programming error, or we may have not seen a valid signal the last time we were enabled.

...

          csdev->enable = true;

@@ -242,6 +270,7 @@ static void coresight_disable_sink(struct coresight_device *csdev) if (atomic_dec_return(csdev->refcnt) == 0) { if (sink_ops(csdev)->disable) { sink_ops(csdev)->disable(csdev);

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

@@ -275,6 +304,8 @@ static int coresight_enable_link(struct coresight_device *csdev, if (atomic_inc_return(&csdev->refcnt[refport]) == 1) { if (link_ops(csdev)->enable) { ret = link_ops(csdev)->enable(csdev, inport, outport);

•                 if (!ret)
  

•                         ret = coresight_control_assoc_ectdev(csdev, 1);
                  if (ret) {
                          atomic_dec(&csdev->refcnt[refport]);
                          return ret;
  

Same comment as above.

...

@@ -314,8 +345,10 @@ static void coresight_disable_link(struct coresight_device *csdev, }

  if (atomic_dec_return(&csdev->refcnt[refport]) == 0) {

•         if (link_ops(csdev)->disable)
  

•         if (link_ops(csdev)->disable) {
                  link_ops(csdev)->disable(csdev, inport, outport);
  

•                 coresight_control_assoc_ectdev(csdev, 0);
  

•         }
  }
  
  for (i = 0; i < nr_conns; i++)
  

@@ -338,6 +371,8 @@ static int coresight_enable_source(struct coresight_device *csdev, u32 mode) if (!csdev->enable) { if (source_ops(csdev)->enable) { ret = source_ops(csdev)->enable(csdev, NULL, mode);

•                 if (!ret)
  

•                         ret = coresight_control_assoc_ectdev(csdev, 1);
                  if (ret)
                          return ret;
  

Same comment as above.

...

          }

@@ -362,6 +397,7 @@ 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, 0);
          csdev->enable = false;
  }
  return !csdev->enable;
  

@@ -1238,6 +1274,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)

  coresight_fixup_device_conns(csdev);
  coresight_fixup_orphan_conns(csdev);

• cti_add_assoc_to_csdev(csdev);
  
  mutex_unlock(&coresight_mutex);
  
  

diff --git a/include/linux/coresight.h b/include/linux/coresight.h index d1c1ed17d2ca..50be479c35d1 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -167,6 +167,8 @@ struct coresight_connection {

•       activated but not yet enabled.  Enabling for a _sink_
  

•       appens when a source has been selected for that it.
  

• @ea: Device attribute for sink representation under PMU directory.

• • @ect_dev: Associated cross trigger device. Not part of the trace data

• •       path or connections.
    

  */

struct coresight_device { struct coresight_connection *conns; @@ -182,6 +184,8 @@ struct coresight_device { /* sink specific fields */ bool activated; /* true only if a sink is part of a path */ struct dev_ext_attribute *ea;

• /* cross trigger handling */
  

• struct coresight_device *ect_dev;
  

I am still not sure how to treat ECT devices, i.e should they become a special helper device or be considered a separate class of device. They are a separate class since they deal with signals rather than data, but they could easily re-use the infrastructure put in place for data devices. I haven't taken a position on this yet so just keep going with the current scheme. I expect the decision will get easier to take as this set is refined over iterations.

I considered them separate from the data devices as the topology is completely different. The trace data devices have an end to end, one way flow of data. This has limit programmability in the way that data flows - i.e. you can choose your sink, but not much else, and downstream components cannot affect upstream components. ECT is a star topology, with highly programmable flexibility - with the correct configuration an ETM trace event can trigger a sink to stop collecting trace, or a sink full event can stop generation. When you add that to the possibility of the hooks into STM hardware signal triggers, and none CS hardware such as ELA, system profilers & clocks (see the Juno .dtsi for connections such as these) it becomes clear that the functionality does not fit easily into the existing data flow infrastructure.

My initial development tried to re-use more of the existing infrastructure but it just got increasingly complex & difficult.

...

};

#define to_coresight_device(d) container_of(d, struct coresight_device, dev)

2.20.1

Regards

Mike