Re: [PATCH v4 1/3] Coresight: Add coresight dummy driver

From: Hao Zhang
Date: Wed May 24 2023 - 02:13:20 EST




On 5/10/2023 10:56 AM, Hao Zhang wrote:
Hi Suzuki,

On 5/5/2023 5:57 PM, Suzuki K Poulose wrote:
On 05/05/2023 10:24, Hao Zhang wrote:
Some Coresight devices that kernel don't have permission to access or
configure. So there need driver to register dummy devices as Coresight
devices. It may also be used to define components that may not have
any programming interfaces (e.g, static links), so that paths can be
established in the driver. Provide Coresight API for dummy device
operations, such as enabling and disabling dummy devices. Build the
Coresight path for dummy sink or dummy source for debugging.

Signed-off-by: Hao Zhang <quic_hazha@xxxxxxxxxxx>
---
  drivers/hwtracing/coresight/Kconfig           |  11 ++
  drivers/hwtracing/coresight/Makefile          |   1 +
  drivers/hwtracing/coresight/coresight-dummy.c | 171 ++++++++++++++++++
  include/linux/coresight.h                     |   1 +
  4 files changed, 184 insertions(+)
  create mode 100644 drivers/hwtracing/coresight/coresight-dummy.c

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index 2b5bbfffbc4f..06f0a7594169 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -236,4 +236,15 @@ config CORESIGHT_TPDA
        To compile this driver as a module, choose M here: the module will be
        called coresight-tpda.
+
+config CORESIGHT_DUMMY
+    tristate "Dummy driver support"
+    help
+      Enables support for dummy driver. Dummy driver can be used for
+      CoreSight sources/sinks that are owned and configured by some
+      other subsystem and use Linux drivers to configure rest of trace
+      path > +
+      To compile this driver as a module, choose M here: the module will be
+      called coresight-dummy.
  endif
diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index 33bcc3f7b8ae..995d3b2c76df 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -30,3 +30,4 @@ obj-$(CONFIG_CORESIGHT_TPDA) += coresight-tpda.o
  coresight-cti-y := coresight-cti-core.o    coresight-cti-platform.o \
             coresight-cti-sysfs.o
  obj-$(CONFIG_ULTRASOC_SMB) += ultrasoc-smb.o
+obj-$(CONFIG_CORESIGHT_DUMMY) += coresight-dummy.o
diff --git a/drivers/hwtracing/coresight/coresight-dummy.c b/drivers/hwtracing/coresight/coresight-dummy.c
new file mode 100644
index 000000000000..ee9881ff4754
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-dummy.c
@@ -0,0 +1,171 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/coresight.h>
+#include <linux/of.h>
+#include <linux/pm_runtime.h>

Please follow the alphabetical order for the header files ^

Sure, I will update this in the next patch series.

+
+#include "coresight-priv.h"
+
+struct dummy_drvdata {
+    struct device            *dev;

nit: We don't need this really. And that completely removes the need for
drvdata too. See below.

+    struct coresight_device        *csdev;
+};
+
+DEFINE_CORESIGHT_DEVLIST(source_devs, "dummy_source");
+DEFINE_CORESIGHT_DEVLIST(sink_devs, "dummy_sink");
+
+static int dummy_source_enable(struct coresight_device *csdev,
+                   struct perf_event *event, u32 mode)
+{
+    struct dummy_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+    dev_dbg(drvdata->dev, "Dummy source enabled\n");

     dev_dbg(csdev->dev.parent, ..");

Similarly for all instances below.

+
+    return 0;
+}
+
+static void dummy_source_disable(struct coresight_device *csdev,
+                 struct perf_event *event)
+{
+    struct dummy_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+    dev_dbg(drvdata->dev, "Dummy source disabled\n");
+}
+
+static int dummy_sink_enable(struct coresight_device *csdev, u32 mode,
+                void *data)
+{
+    struct dummy_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+    dev_dbg(drvdata->dev, "Dummy sink enabled\n");
+
+    return 0;
+}
+
+static int dummy_sink_disable(struct coresight_device *csdev)
+{
+    struct dummy_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+    dev_dbg(drvdata->dev, "Dummy sink disabled\n");
+
+    return 0;
+}
+
+static const struct coresight_ops_source dummy_source_ops = {
+    .enable    = dummy_source_enable,
+    .disable = dummy_source_disable,
+};
+
+static const struct coresight_ops dummy_source_cs_ops = {
+    .source_ops = &dummy_source_ops,
+};
+
+static const struct coresight_ops_sink dummy_sink_ops = {
+    .enable    = dummy_sink_enable,
+    .disable = dummy_sink_disable,
+};
+
+static const struct coresight_ops dummy_sink_cs_ops = {
+    .sink_ops = &dummy_sink_ops,
+};
+
+static int dummy_probe(struct platform_device *pdev)
+{
+    struct device *dev = &pdev->dev;
+    struct device_node *node = dev->of_node;
+    struct coresight_platform_data *pdata;
+    struct dummy_drvdata *drvdata;
+    struct coresight_desc desc = { 0 };
+
+    if (of_device_is_compatible(node, "arm,coresight-dummy-source")) {
+
+        desc.name = coresight_alloc_device_name(&source_devs, dev);
+        if (!desc.name)
+            return -ENOMEM;
+
+        desc.type = CORESIGHT_DEV_TYPE_SOURCE;
+        desc.subtype.source_subtype =
+                    CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS;
+        desc.ops = &dummy_source_cs_ops;
+    } else if (of_device_is_compatible(node, "arm,coresight-dummy-sink")) {
+        desc.name = coresight_alloc_device_name(&sink_devs, dev);
+        if (!desc.name)
+            return -ENOMEM;
+
+        desc.type = CORESIGHT_DEV_TYPE_SINK;
+        desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_DUMMY;
+        desc.ops = &dummy_sink_cs_ops;
+    } else {
+        dev_err(dev, "Device type not set\n");
+        return -EINVAL;
+    }
+
+    pdata = coresight_get_platform_data(dev);
+    if (IS_ERR(pdata))
+        return PTR_ERR(pdata);
+    pdev->dev.platform_data = pdata;
+
+    drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+    if (!drvdata)
+        return -ENOMEM; > +
+    drvdata->dev = &pdev->dev;
+    platform_set_drvdata(pdev, drvdata);

As above, you may remove the drvdata entirely.

For some dummy sources, it's needed to allocate traceid for them in kernel driver, so this struct would be useful for that case.

I will remove it now and upstream it in the further.


Hi Suzuki,

After checking, I find the drvdata struct is required for csdev which would store the pointer of coresight_device, and it is also required for traceid in the further. So I will keep drvdata in the next patch series.

Thanks,
Hao

Thanks,
Hao

Otherwise looks good to me

Suzuki