Re: [PATCH v4 05/10] coresight: tmc: Introduce tmc_read_ops to wrap read operations

On Wed, 6 Aug 2025 at 07:30, Jie Gan [off-list ref] wrote:

On 8/5/2025 6:55 PM, Mike Leach wrote:

quoted

Hi

On Fri, 25 Jul 2025 at 11:08, Jie Gan [off-list ref] wrote:

quoted

Introduce tmc_read_ops as a wrapper, wrap read operations, for reading
trace data from the TMC buffer.

Signed-off-by: Jie Gan <redacted>
---
  .../hwtracing/coresight/coresight-tmc-core.c  | 50 +++++++++----------
  drivers/hwtracing/coresight/coresight-tmc.h   | 17 +++++++
  2 files changed, 40 insertions(+), 27 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
index 4d249af93097..f668047c5df4 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-core.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-core.c

@@ -232,17 +232,10 @@ static int tmc_read_prepare(struct tmc_drvdata *drvdata)
  {
         int ret = 0;

-       switch (drvdata->config_type) {
-       case TMC_CONFIG_TYPE_ETB:
-       case TMC_CONFIG_TYPE_ETF:
-               ret = tmc_read_prepare_etb(drvdata);
-               break;
-       case TMC_CONFIG_TYPE_ETR:
-               ret = tmc_read_prepare_etr(drvdata);
-               break;
-       default:
+       if (drvdata->read_ops)
+               ret = drvdata->read_ops->read_prepare(drvdata);
+       else
                 ret = -EINVAL;
-       }

         if (!ret)
                 dev_dbg(&drvdata->csdev->dev, "TMC read start\n");

@@ -254,17 +247,10 @@ static int tmc_read_unprepare(struct tmc_drvdata *drvdata)
  {
         int ret = 0;

-       switch (drvdata->config_type) {
-       case TMC_CONFIG_TYPE_ETB:
-       case TMC_CONFIG_TYPE_ETF:
-               ret = tmc_read_unprepare_etb(drvdata);
-               break;
-       case TMC_CONFIG_TYPE_ETR:
-               ret = tmc_read_unprepare_etr(drvdata);
-               break;
-       default:
+       if (drvdata->read_ops)
+               ret = drvdata->read_ops->read_unprepare(drvdata);
+       else
                 ret = -EINVAL;
-       }

         if (!ret)
                 dev_dbg(&drvdata->csdev->dev, "TMC read end\n");

@@ -291,13 +277,8 @@ static int tmc_open(struct inode *inode, struct file *file)
  static ssize_t tmc_get_sysfs_trace(struct tmc_drvdata *drvdata, loff_t pos, size_t len,
                                    char **bufpp)
  {
-       switch (drvdata->config_type) {
-       case TMC_CONFIG_TYPE_ETB:
-       case TMC_CONFIG_TYPE_ETF:
-               return tmc_etb_get_sysfs_trace(drvdata, pos, len, bufpp);
-       case TMC_CONFIG_TYPE_ETR:
-               return tmc_etr_get_sysfs_trace(drvdata, pos, len, bufpp);
-       }
+       if (drvdata->read_ops)
+               return drvdata->read_ops->get_trace_data(drvdata, pos, len, bufpp);

         return -EINVAL;
  }

@@ -769,6 +750,18 @@ static void register_crash_dev_interface(struct tmc_drvdata *drvdata,
                         "Valid crash tracedata found\n");
  }

+static const struct tmc_read_ops tmc_etb_read_ops = {
+       .read_prepare   = tmc_read_prepare_etb,
+       .read_unprepare = tmc_read_unprepare_etb,
+       .get_trace_data = tmc_etb_get_sysfs_trace,
+};
+
+static const struct tmc_read_ops tmc_etr_read_ops = {
+       .read_prepare   = tmc_read_prepare_etr,
+       .read_unprepare = tmc_read_unprepare_etr,
+       .get_trace_data = tmc_etr_get_sysfs_trace,
+};
+
  static int __tmc_probe(struct device *dev, struct resource *res)
  {
         int ret = 0;

@@ -818,6 +811,7 @@ static int __tmc_probe(struct device *dev, struct resource *res)
                 desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
                 desc.ops = &tmc_etb_cs_ops;
                 dev_list = &etb_devs;
+               drvdata->read_ops = &tmc_etb_read_ops;
                 break;
         case TMC_CONFIG_TYPE_ETR:
                 desc.groups = coresight_etr_groups;

@@ -831,6 +825,7 @@ static int __tmc_probe(struct device *dev, struct resource *res)
                 mutex_init(&drvdata->idr_mutex);
                 dev_list = &etr_devs;
                 INIT_LIST_HEAD(&drvdata->etr_buf_list);
+               drvdata->read_ops = &tmc_etr_read_ops;
                 break;
         case TMC_CONFIG_TYPE_ETF:
                 desc.groups = coresight_etf_groups;

@@ -839,6 +834,7 @@ static int __tmc_probe(struct device *dev, struct resource *res)
                 desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_FIFO;
                 desc.ops = &tmc_etf_cs_ops;
                 dev_list = &etf_devs;
+               drvdata->read_ops = &tmc_etb_read_ops;
                 break;
         default:
                 pr_err("%s: Unsupported TMC config\n", desc.name);

diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index 3cb8ba9f88f5..2ad8e288c94b 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h

@@ -223,6 +223,8 @@ struct etr_buf_node {
         struct list_head        node;
  };

+struct tmc_read_ops;
+

declare the entire structure here rather than later.

There is an issue of declare the entire structure here because the
function pointer needs use the struct tmc_drvdata which is not defined
at this point and it will cause a compile error.

Understood.

Mike

Thanks,
Jie

quoted

quoted

  /**
   * struct tmc_drvdata - specifics associated to an TMC component
   * @pclk:      APB clock if present, otherwise NULL

@@ -259,6 +261,7 @@ struct etr_buf_node {
   *              Used by ETR/ETF.
   * @etr_buf_list: List that is used to manage allocated etr_buf.
   * @reading_node: Available buffer for byte-cntr reading.
+ * @tmc_read_ops: Read operations for TMC device.
   */
  struct tmc_drvdata {
         struct clk              *pclk;

@@ -290,6 +293,20 @@ struct tmc_drvdata {
         struct tmc_resrv_buf    crash_mdata;
         struct list_head        etr_buf_list;
         struct etr_buf_node     *reading_node;
+       const struct tmc_read_ops       *read_ops;

probably should be named sysfs_read_ops to be consistent with the
perf/sysfs differentiation within the rest of the structure

quoted

+};
+
+/**
+ * struct tmc_read_ops - read operations for TMC and its helper devices
+ * @read_prepare:      prepare operation.
+ * @read_unprepare:    unprepare operation.
+ * @get_trace_data:    read operation.
+ */
+struct tmc_read_ops {
+       int (*read_prepare)(struct tmc_drvdata *drvdata);
+       int (*read_unprepare)(struct tmc_drvdata *drvdata);
+       ssize_t (*get_trace_data)(struct tmc_drvdata *drvdata, loff_t pos,
+                                 size_t len, char **bufpp);
  };

  struct etr_buf_operations {
--
2.34.1

with the above changes:-

Reviewed-by: Mike Leach <redacted>

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