Good day,

On Mon, Aug 17, 2020 at 07:49:16PM +0100, Mike Leach wrote:

Hi Mathieu,

On Mon, 17 Aug 2020 at 17:02, Mathieu Poirier
[off-list ref] wrote:

quoted

On Fri, Aug 07, 2020 at 07:11:45PM +0800, Tingwei Zhang wrote:

quoted

Add static cti_assoc_ops to coresight core driver. Let cti
driver register the add_assoc and remove_assoc call back.
Avoid coresight core driver to depend on cti driver.

Signed-off-by: Tingwei Zhang <redacted>
Tested-by: Mike Leach <redacted>
---
 drivers/hwtracing/coresight/coresight-cti.c  | 32 ++++++++++++++++++--
 drivers/hwtracing/coresight/coresight-priv.h | 14 ++++-----
 drivers/hwtracing/coresight/coresight.c      | 21 +++++++++++--
 3 files changed, 53 insertions(+), 14 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c
index 3ccc703dc940..1f470c47ba70 100644
--- a/drivers/hwtracing/coresight/coresight-cti.c
+++ b/drivers/hwtracing/coresight/coresight-cti.c

@@ -589,7 +589,6 @@ void cti_add_assoc_to_csdev(struct coresight_device *csdev)
 cti_add_done:
      mutex_unlock(&ect_mutex);
 }
-EXPORT_SYMBOL_GPL(cti_add_assoc_to_csdev);

 /*
  * Removing the associated devices is easier.

@@ -616,7 +615,15 @@ void cti_remove_assoc_from_csdev(struct coresight_device *csdev)
      }
      mutex_unlock(&ect_mutex);
 }
-EXPORT_SYMBOL_GPL(cti_remove_assoc_from_csdev);

Mike and Tingwei,

It seems to me that functions cti_add_assoc_to_csdev() and
cti_remote_assoc_from_csdev() don't have to be exported.  They could be called
in cti_probe() and cti_device_release() with the ect_mutex held, the same way
cti_update_conn_xrefs() and cti_remove_conn_xrefs() are.

The different functions handle two different initialisation & two
different teardown ordering scenarios
a) csdev - e.g. ETM is discovered & initialised before the associated
CTI. Therefore the CTI has to establish interconnections on creation -
calls cti_update_conn_xrefs() .
b) CTI is discovered before csdev - therefore the csdev calls
cti_add_assoc_to_csdev().
c) csdev is torn down before CTI - therefore csdev calls
cti_remove_assoc_from_csdev()
d) CTI is torn down before csdev - therefore calls cti_remove_conn_xrefs()

We can't call cti_add_assoc_to_csdev() in cti probe as the csdev does
not exist yet. Bear in mind that one CTI can actually be associated
with multiple different csdev devices - especially in the none CPU
bound infrastructure such as funnels and sinks.

Yes, I see your point now.  The problem needs to be viewed from other devices'
(than CTI) perspective.  And that part is done in
coresight_register/unregister().

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

This has always been the case since the first CTI set - but is even
more important now the modules can be removed / added as required.

Regards

Mike

quoted

If my assessment is correct this patch would be simpler.

Thanks,
Mathieu

quoted

+
+/*
+ * Operations to add and remove associated CTI.
+ * Register to coresight core driver as call back function.
+ */
+static struct cti_assoc_op cti_assoc_ops = {
+     .add = cti_add_assoc_to_csdev,
+     .remove = cti_remove_assoc_from_csdev
+};

 /*
  * Update the cross references where the associated device was found

@@ -972,4 +979,23 @@ static struct amba_driver cti_driver = {
      .probe          = cti_probe,
      .id_table       = cti_ids,
 };
-builtin_amba_driver(cti_driver);
+
+static int __init cti_init(void)
+{
+     int ret;
+
+     ret = amba_driver_register(&cti_driver);
+     if (ret)
+             pr_info("Error registering cti driver\n");
+     coresight_set_cti_ops(&cti_assoc_ops);
+     return ret;
+}
+
+static void __exit cti_exit(void)
+{
+     coresight_remove_cti_ops();
+     amba_driver_unregister(&cti_driver);
+}
+
+module_init(cti_init);
+module_exit(cti_exit);

diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index dcb8aeb6af62..6cde6cf42554 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h

@@ -173,15 +173,13 @@ static inline int etm_readl_cp14(u32 off, unsigned int *val) { return 0; }
 static inline int etm_writel_cp14(u32 off, u32 val) { return 0; }
 #endif

-#ifdef CONFIG_CORESIGHT_CTI
-extern void cti_add_assoc_to_csdev(struct coresight_device *csdev);
-extern void cti_remove_assoc_from_csdev(struct coresight_device *csdev);
+struct cti_assoc_op {
+     void (*add)(struct coresight_device *csdev);
+     void (*remove)(struct coresight_device *csdev);
+};

-#else
-static inline void cti_add_assoc_to_csdev(struct coresight_device *csdev) {}
-static inline void
-cti_remove_assoc_from_csdev(struct coresight_device *csdev) {}
-#endif
+extern void coresight_set_cti_ops(const struct cti_assoc_op *cti_op);
+extern void coresight_remove_cti_ops(void);

 /*
  * Macros and inline functions to handle CoreSight UCI data and driver

diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index 0b0e31577b9b..31b9ec8d3b9c 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c

@@ -57,6 +57,20 @@ const u32 coresight_barrier_pkt[4] = {
              0x7fffffff, 0x7fffffff, 0x7fffffff, 0x7fffffff};
 EXPORT_SYMBOL_GPL(coresight_barrier_pkt);

+static const struct cti_assoc_op *cti_assoc_ops;
+
+void coresight_set_cti_ops(const struct cti_assoc_op *cti_op)
+{
+     cti_assoc_ops = cti_op;
+}
+EXPORT_SYMBOL_GPL(coresight_set_cti_ops);
+
+void coresight_remove_cti_ops(void)
+{
+     cti_assoc_ops = NULL;
+}
+EXPORT_SYMBOL_GPL(coresight_remove_cti_ops);
+
 static int coresight_id_match(struct device *dev, void *data)
 {
      int trace_id, i_trace_id;

@@ -1242,7 +1256,8 @@ static void coresight_device_release(struct device *dev)
 {
      struct coresight_device *csdev = to_coresight_device(dev);

-     cti_remove_assoc_from_csdev(csdev);
+     if (cti_assoc_ops && cti_assoc_ops->remove)
+             cti_assoc_ops->remove(csdev);
      fwnode_handle_put(csdev->dev.fwnode);
      kfree(csdev->refcnt);
      kfree(csdev);

@@ -1553,8 +1568,8 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
              ret = coresight_fixup_device_conns(csdev);
      if (!ret)
              ret = coresight_fixup_orphan_conns(csdev);
-     if (!ret)
-             cti_add_assoc_to_csdev(csdev);
+     if (!ret && cti_assoc_ops && cti_assoc_ops->add)
+             cti_assoc_ops->add(csdev);

      mutex_unlock(&coresight_mutex);
      if (ret) {
--

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel