Re: [RFC PATCH v3 08/16] coresight: cti: Enable CTI associated with devices.

Hi Suzuki,

On Fri, 21 Jun 2019 at 16:25, Suzuki K Poulose suzuki.poulose@arm.com wrote:

On 05/06/2019 20:03, 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

.../coresight/coresight-cti-platform.c | 22 +++++ drivers/hwtracing/coresight/coresight-cti.c | 90 +++++++++++++++++++ drivers/hwtracing/coresight/coresight-cti.h | 1 + drivers/hwtracing/coresight/coresight-priv.h | 3 + drivers/hwtracing/coresight/coresight.c | 56 ++++++++++-- include/linux/coresight.h | 4 + 6 files changed, 171 insertions(+), 5 deletions(-)

...

struct coresight_platform_data * @@ -477,3 +488,14 @@ coresight_cti_get_platform_data(struct device *dev) error: return ERR_PTR(ret); }

+const char *coresight_cti_get_node_name(struct device *dev) +{

• const char *node_name = NULL;
  

• struct fwnode_handle *fwnode = dev_fwnode(dev);
  

• if (is_of_node(fwnode))
  

•         node_name = of_cti_get_node_name(dev);
  

• return node_name;
  

+}

nit: This is not specific to CTI and could be simple made coresight_get_fwnode_name() or something similar and moved to coresight-platform.c

Done.

...

diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c index a4e349984ab8..6fb924f70af2 100644 --- a/drivers/hwtracing/coresight/coresight-cti.c +++ b/drivers/hwtracing/coresight/coresight-cti.c @@ -471,6 +471,93 @@ int cti_channel_setop(struct device *dev, return err; }

+/*

• • 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);
  

•                         goto cti_con_name_match;
  

nit: Instead, return 1; ?

...

•                 }
  

•         }
  

• }
  

+cti_con_name_match:

• return found;
  

and return 0; here ?

Agreed - somewaht neater.

Thanks

Mike

...

+}

+/*

• • 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
• */

+void 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;
  

• /* protect the list */
  

• mutex_lock(&ect_mutex);
  

• /* exit if current is an ECT device.*/
  

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

•         goto cti_add_done;
  

• /* if we didn't find the csdev previously we used the fwnode name */
  

• node_name = coresight_cti_get_node_name(csdev->dev.parent);
  

• if (!node_name)
  

•         goto cti_add_done;
  

• /* this is the name we want to use for the association */
  

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

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

• 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_add_done;
  

•         }
  

• }
  

+cti_add_done:

• mutex_unlock(&ect_mutex);
  

+} +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) {

@@ -587,6 +674,9 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id) list_add(&ect_nd->next, &ect_net); 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-cti.h b/drivers/hwtracing/coresight/coresight-cti.h index b87cc67543e0..ab1be67eb704 100644 --- a/drivers/hwtracing/coresight/coresight-cti.h +++ b/drivers/hwtracing/coresight/coresight-cti.h @@ -250,6 +250,7 @@ int cti_channel_setop(struct device *dev, enum cti_chan_set_op op,

struct coresight_platform_data * coresight_cti_get_platform_data(struct device *dev); +const char *coresight_cti_get_node_name(struct device *dev);

/* cti powered and enabled */ #define CTI_PWR_ENA(p_cfg) (p_cfg->hw_enabled && p_cfg->hw_powered) diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index 64a888cc620f..9c4fd7eb56eb 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -164,8 +164,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 void 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 1c9ba2fd6879..74661fabe098 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");
  

• return ect_ret;
  

+}

• static int coresight_enable_sink(struct coresight_device *csdev, u32 mode, void *data) {

@@ -228,11 +253,15 @@ static int coresight_enable_sink(struct coresight_device *csdev, if (!sink_ops(csdev)->enable) return -EINVAL;

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

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

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

• if (ret) {
  

•         coresight_control_assoc_ectdev(csdev, 0);
  

•         return ret;
  

• }
  csdev->enable = true;
  

• return 0;
  

  }

@@ -246,6 +275,7 @@ static void coresight_disable_sink(struct coresight_device *csdev) ret = sink_ops(csdev)->disable(csdev); if (ret) return;

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

  }

@@ -276,7 +306,14 @@ 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);
  

•                 ret = coresight_control_assoc_ectdev(csdev, 1);
  

•                 if (!ret) {
  

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

•                                                       outport);
  

•                         if (ret)
  

•                                 coresight_control_assoc_ectdev(csdev,
  

•                                                                0);
  

•                 }
                  if (ret) {
                          atomic_dec(&csdev->refcnt[refport]);
                          return ret;
  

@@ -316,8 +353,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++)
  

@@ -339,9 +378,14 @@ 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);
  

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

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

•                 if (ret) {
  

•                         coresight_control_assoc_ectdev(csdev, 0);
  

•                         return ret;
  

•                 };
          }
          csdev->enable = true;
  }
  

Instead of spilling the call for sink/link/source call backs, could we not add this to the coresight_enable/disable_path() where these gets called from ? For each device we could add "enable" associated device. This could be done for the source even. Otherwise looks good to me.

Suzuki

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