[PATCH v2 0/3] Add support to configure Coresight Dummy subunit

[PATCH v2 0/3] Add support to configure Coresight Dummy subunit

[Hao Zhang]

Introduction of Coresight Dummy subunit
The Coresight Dummy subunit is for Coresight Dummy component, there are some
specific Coresight devices that HLOS don't have permission to access. Such as
some TPDMs, they would be configured in NON-HLOS side, but it's necessary to
build Coresight path for it to debug. So there need driver to register dummy
devices as Coresight devices.

Commit link:
https://git.codelinaro.org/clo/linux-kernel/coresight/-/tree/coresight-dummy-v2

Changes in V2:
1. Declare dummy_init and dummy_exit as static to fix missing-prototypes
warnings. -- kernel test robot <lkp@intel.com>
2. Fix the errors of coresight-dummy yaml file. -- Rob Herring <robh@kernel.org>

Hao Zhang (3):
  Coresight: Add coresight dummy driver
  dt-bindings: arm: Add Coresight Dummy Trace YAML schema
  Documentation: trace: Add documentation for Coresight Dummy Trace

 .../bindings/arm/qcom,coresight-dummy.yaml    | 118 ++++++++++++
 .../trace/coresight/coresight-dummy.rst       |  58 ++++++
 drivers/hwtracing/coresight/Kconfig           |  11 ++
 drivers/hwtracing/coresight/Makefile          |   1 +
 drivers/hwtracing/coresight/coresight-dummy.c | 176 ++++++++++++++++++
 5 files changed, 364 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
 create mode 100644 Documentation/trace/coresight/coresight-dummy.rst
 create mode 100644 drivers/hwtracing/coresight/coresight-dummy.c

-- 
2.17.1

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

[Hao Zhang]

Some Coresight devices that HLOS don't have permission to access
or configure. Such as Coresight sink EUD, some TPDMs etc. So there
need driver to register dummy devices as Coresight devices. 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@quicinc.com>
---
 drivers/hwtracing/coresight/Kconfig           |  11 ++
 drivers/hwtracing/coresight/Makefile          |   1 +
 drivers/hwtracing/coresight/coresight-dummy.c | 176 ++++++++++++++++++
 3 files changed, 188 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..2d4eb3e546eb
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-dummy.c
@@ -0,0 +1,176 @@
+// 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>
+
+#include "coresight-priv.h"
+#include "coresight-trace-id.h"
+
+struct dummy_drvdata {
+	struct device			*dev;
+	struct coresight_device		*csdev;
+	int				traceid;
+};
+
+DEFINE_CORESIGHT_DEVLIST(dummy_devs, "dummy");
+
+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_info(drvdata->dev, "Dummy source enabled\n");
+
+	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_info(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_info(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_info(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_sink dummy_sink_ops = {
+	.enable		= dummy_sink_enable,
+	.disable	= dummy_sink_disable,
+};
+
+static const struct coresight_ops dummy_cs_ops = {
+	.source_ops	= &dummy_source_ops,
+	.sink_ops	= &dummy_sink_ops,
+};
+
+static int dummy_probe(struct platform_device *pdev)
+{
+	int ret, trace_id;
+	struct device *dev = &pdev->dev;
+	struct coresight_platform_data *pdata;
+	struct dummy_drvdata *drvdata;
+	struct coresight_desc desc = { 0 };
+
+	desc.name = coresight_alloc_device_name(&dummy_devs, dev);
+	if (!desc.name)
+		return -ENOMEM;
+
+	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);
+
+	if (of_property_read_bool(pdev->dev.of_node, "qcom,dummy-source")) {
+		desc.type = CORESIGHT_DEV_TYPE_SOURCE;
+		desc.subtype.source_subtype =
+					CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS;
+	} else if (of_property_read_bool(pdev->dev.of_node,
+					 "qcom,dummy-sink")) {
+		desc.type = CORESIGHT_DEV_TYPE_SINK;
+		desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
+	} else {
+		dev_info(dev, "Device type not set\n");
+		return -EINVAL;
+	}
+
+	desc.ops = &dummy_cs_ops;
+	desc.pdata = pdev->dev.platform_data;
+	desc.dev = &pdev->dev;
+	drvdata->csdev = coresight_register(&desc);
+	if (IS_ERR(drvdata->csdev))
+		return PTR_ERR(drvdata->csdev);
+
+	trace_id = coresight_trace_id_get_system_id();
+	if (trace_id < 0) {
+		ret = trace_id;
+		goto cs_unregister;
+	}
+	drvdata->traceid = (u8)trace_id;
+
+	pm_runtime_enable(dev);
+	dev_info(dev, "Dummy device initialized\n");
+
+	return 0;
+
+cs_unregister:
+	coresight_unregister(drvdata->csdev);
+
+	return ret;
+}
+
+static int dummy_remove(struct platform_device *pdev)
+{
+	struct dummy_drvdata *drvdata = platform_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+
+	coresight_trace_id_put_system_id(drvdata->traceid);
+	pm_runtime_disable(dev);
+	coresight_unregister(drvdata->csdev);
+	return 0;
+}
+
+static const struct of_device_id dummy_match[] = {
+	{.compatible = "qcom,coresight-dummy"},
+	{},
+};
+
+static struct platform_driver dummy_driver = {
+	.probe	= dummy_probe,
+	.remove	= dummy_remove,
+	.driver	= {
+		.name   = "coresight-dummy",
+		.of_match_table = dummy_match,
+	},
+};
+
+static int __init dummy_init(void)
+{
+	return platform_driver_register(&dummy_driver);
+}
+module_init(dummy_init);
+
+static void __exit dummy_exit(void)
+{
+	platform_driver_unregister(&dummy_driver);
+}
+module_exit(dummy_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("CoreSight dummy source driver");
-- 
2.17.1

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

[Suzuki K Poulose]

On 24/03/2023 06:16, Hao Zhang wrote:
> Some Coresight devices that HLOS don't have permission to access
> or configure. Such as Coresight sink EUD, some TPDMs etc. So there
> need driver to register dummy devices as Coresight devices. 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@quicinc.com>
> ---
>   drivers/hwtracing/coresight/Kconfig           |  11 ++
>   drivers/hwtracing/coresight/Makefile          |   1 +
>   drivers/hwtracing/coresight/coresight-dummy.c | 176 ++++++++++++++++++
>   3 files changed, 188 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..2d4eb3e546eb
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-dummy.c
> @@ -0,0 +1,176 @@
> +// 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>
> +
> +#include "coresight-priv.h"
> +#include "coresight-trace-id.h"
> +
> +struct dummy_drvdata {
> +	struct device			*dev;
> +	struct coresight_device		*csdev;
> +	int				traceid;
> +};
> +
> +DEFINE_CORESIGHT_DEVLIST(dummy_devs, "dummy");
> +
> +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_info(drvdata->dev, "Dummy source enabled\n");

Please use dev_dbg everywher.


> +
> +	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_info(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_info(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_info(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_sink dummy_sink_ops = {
> +	.enable		= dummy_sink_enable,
> +	.disable	= dummy_sink_disable,
> +};
> +
> +static const struct coresight_ops dummy_cs_ops = {
> +	.source_ops	= &dummy_source_ops,
> +	.sink_ops	= &dummy_sink_ops,
> +};
> +
> +static int dummy_probe(struct platform_device *pdev)
> +{
> +	int ret, trace_id;
> +	struct device *dev = &pdev->dev;
> +	struct coresight_platform_data *pdata;
> +	struct dummy_drvdata *drvdata;
> +	struct coresight_desc desc = { 0 };
> +
> +	desc.name = coresight_alloc_device_name(&dummy_devs, dev);
> +	if (!desc.name)
> +		return -ENOMEM;
> +
> +	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);
> +
> +	if (of_property_read_bool(pdev->dev.of_node, "qcom,dummy-source")) {

I don't see any reason why this should be qcom,...

Please could we use : "arm,coresight-", everywhere including the "dt"
compatible ?

> +		desc.type = CORESIGHT_DEV_TYPE_SOURCE;
> +		desc.subtype.source_subtype =
> +					CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS;
> +	} else if (of_property_read_bool(pdev->dev.of_node,
> +					 "qcom,dummy-sink")) {
> +		desc.type = CORESIGHT_DEV_TYPE_SINK;
> +		desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
> +	} else {
> +		dev_info(dev, "Device type not set\n");
> +		return -EINVAL;
> +	}
> +
> +	desc.ops = &dummy_cs_ops;
> +	desc.pdata = pdev->dev.platform_data;
> +	desc.dev = &pdev->dev;
> +	drvdata->csdev = coresight_register(&desc);
> +	if (IS_ERR(drvdata->csdev))
> +		return PTR_ERR(drvdata->csdev);
> +
> +	trace_id = coresight_trace_id_get_system_id();
> +	if (trace_id < 0) {
> +		ret = trace_id;
> +		goto cs_unregister;
> +	}
> +	drvdata->traceid = (u8)trace_id;
> +
> +	pm_runtime_enable(dev);
> +	dev_info(dev, "Dummy device initialized\n");
> +
> +	return 0;
> +
> +cs_unregister:
> +	coresight_unregister(drvdata->csdev);
> +
> +	return ret;
> +}
> +
> +static int dummy_remove(struct platform_device *pdev)
> +{
> +	struct dummy_drvdata *drvdata = platform_get_drvdata(pdev);
> +	struct device *dev = &pdev->dev;
> +
> +	coresight_trace_id_put_system_id(drvdata->traceid);
> +	pm_runtime_disable(dev);
> +	coresight_unregister(drvdata->csdev);
> +	return 0;
> +}
> +
> +static const struct of_device_id dummy_match[] = {
> +	{.compatible = "qcom,coresight-dummy"},

As mentioned above, "arm,coresight-dummy-device" ? This has
nothing to do with qcom IP. qcom has a use for this. So, I would
like to keep this "coresight" subsystem specific compatibles.

May be we could even add other types too : i.e,

arm,coresight-dummy-link-split, arm,coresight-dummy-link-merge


Suzuki

> +	{},
> +};
> +
> +static struct platform_driver dummy_driver = {
> +	.probe	= dummy_probe,
> +	.remove	= dummy_remove,
> +	.driver	= {
> +		.name   = "coresight-dummy",
> +		.of_match_table = dummy_match,
> +	},
> +};
> +
> +static int __init dummy_init(void)
> +{
> +	return platform_driver_register(&dummy_driver);
> +}
> +module_init(dummy_init);
> +
> +static void __exit dummy_exit(void)
> +{
> +	platform_driver_unregister(&dummy_driver);
> +}
> +module_exit(dummy_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("CoreSight dummy source driver");

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

[Hao Zhang]

Hi Suzuki,

On 3/24/2023 6:44 PM, Suzuki K Poulose wrote:
> On 24/03/2023 06:16, Hao Zhang wrote:
>> Some Coresight devices that HLOS don't have permission to access
>> or configure. Such as Coresight sink EUD, some TPDMs etc. So there
>> need driver to register dummy devices as Coresight devices. 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@quicinc.com>
>> ---
>>   drivers/hwtracing/coresight/Kconfig           |  11 ++
>>   drivers/hwtracing/coresight/Makefile          |   1 +
>>   drivers/hwtracing/coresight/coresight-dummy.c | 176 ++++++++++++++++++
>>   3 files changed, 188 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..2d4eb3e546eb
>> --- /dev/null
>> +++ b/drivers/hwtracing/coresight/coresight-dummy.c
>> @@ -0,0 +1,176 @@
>> +// 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>
>> +
>> +#include "coresight-priv.h"
>> +#include "coresight-trace-id.h"
>> +
>> +struct dummy_drvdata {
>> +    struct device            *dev;
>> +    struct coresight_device        *csdev;
>> +    int                traceid;
>> +};
>> +
>> +DEFINE_CORESIGHT_DEVLIST(dummy_devs, "dummy");
>> +
>> +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_info(drvdata->dev, "Dummy source enabled\n");
> 
> Please use dev_dbg everywher.

Sure, I will change to dev_dbg in the next patch series.

> 
>> +
>> +    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_info(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_info(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_info(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_sink dummy_sink_ops = {
>> +    .enable        = dummy_sink_enable,
>> +    .disable    = dummy_sink_disable,
>> +};
>> +
>> +static const struct coresight_ops dummy_cs_ops = {
>> +    .source_ops    = &dummy_source_ops,
>> +    .sink_ops    = &dummy_sink_ops,
>> +};
>> +
>> +static int dummy_probe(struct platform_device *pdev)
>> +{
>> +    int ret, trace_id;
>> +    struct device *dev = &pdev->dev;
>> +    struct coresight_platform_data *pdata;
>> +    struct dummy_drvdata *drvdata;
>> +    struct coresight_desc desc = { 0 };
>> +
>> +    desc.name = coresight_alloc_device_name(&dummy_devs, dev);
>> +    if (!desc.name)
>> +        return -ENOMEM;
>> +
>> +    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);
>> +
>> +    if (of_property_read_bool(pdev->dev.of_node, "qcom,dummy-source")) {
> 
> I don't see any reason why this should be qcom,...
> 
> Please could we use : "arm,coresight-", everywhere including the "dt"
> compatible ?

It's not only for qcom device, I will update this according to your 
advice in the next patch series.

>> +        desc.type = CORESIGHT_DEV_TYPE_SOURCE;
>> +        desc.subtype.source_subtype =
>> +                    CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS;
>> +    } else if (of_property_read_bool(pdev->dev.of_node,
>> +                     "qcom,dummy-sink")) {
>> +        desc.type = CORESIGHT_DEV_TYPE_SINK;
>> +        desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
>> +    } else {
>> +        dev_info(dev, "Device type not set\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    desc.ops = &dummy_cs_ops;
>> +    desc.pdata = pdev->dev.platform_data;
>> +    desc.dev = &pdev->dev;
>> +    drvdata->csdev = coresight_register(&desc);
>> +    if (IS_ERR(drvdata->csdev))
>> +        return PTR_ERR(drvdata->csdev);
>> +
>> +    trace_id = coresight_trace_id_get_system_id();
>> +    if (trace_id < 0) {
>> +        ret = trace_id;
>> +        goto cs_unregister;
>> +    }
>> +    drvdata->traceid = (u8)trace_id;
>> +
>> +    pm_runtime_enable(dev);
>> +    dev_info(dev, "Dummy device initialized\n");
>> +
>> +    return 0;
>> +
>> +cs_unregister:
>> +    coresight_unregister(drvdata->csdev);
>> +
>> +    return ret;
>> +}
>> +
>> +static int dummy_remove(struct platform_device *pdev)
>> +{
>> +    struct dummy_drvdata *drvdata = platform_get_drvdata(pdev);
>> +    struct device *dev = &pdev->dev;
>> +
>> +    coresight_trace_id_put_system_id(drvdata->traceid);
>> +    pm_runtime_disable(dev);
>> +    coresight_unregister(drvdata->csdev);
>> +    return 0;
>> +}
>> +
>> +static const struct of_device_id dummy_match[] = {
>> +    {.compatible = "qcom,coresight-dummy"},
> 
> As mentioned above, "arm,coresight-dummy-device" ? This has
> nothing to do with qcom IP. qcom has a use for this. So, I would
> like to keep this "coresight" subsystem specific compatibles.
> 
> May be we could even add other types too : i.e,
> 
> arm,coresight-dummy-link-split, arm,coresight-dummy-link-merge
> 
> Suzuki
>

I will change this it to "arm,coresight-dummy-device" in the next patch 
series.

Thanks,
Hao

>> +    {},
>> +};
>> +
>> +static struct platform_driver dummy_driver = {
>> +    .probe    = dummy_probe,
>> +    .remove    = dummy_remove,
>> +    .driver    = {
>> +        .name   = "coresight-dummy",
>> +        .of_match_table = dummy_match,
>> +    },
>> +};
>> +
>> +static int __init dummy_init(void)
>> +{
>> +    return platform_driver_register(&dummy_driver);
>> +}
>> +module_init(dummy_init);
>> +
>> +static void __exit dummy_exit(void)
>> +{
>> +    platform_driver_unregister(&dummy_driver);
>> +}
>> +module_exit(dummy_exit);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("CoreSight dummy source driver");
> 

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

[Mike Leach]

Hi,

On Fri, 24 Mar 2023 at 06:16, Hao Zhang <quic_hazha@quicinc.com> wrote:
>
> Some Coresight devices that HLOS don't have permission to access
> or configure. Such as Coresight sink EUD, some TPDMs etc. So there
> need driver to register dummy devices as Coresight devices. 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@quicinc.com>
> ---
>  drivers/hwtracing/coresight/Kconfig           |  11 ++
>  drivers/hwtracing/coresight/Makefile          |   1 +
>  drivers/hwtracing/coresight/coresight-dummy.c | 176 ++++++++++++++++++
>  3 files changed, 188 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..2d4eb3e546eb
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-dummy.c
> @@ -0,0 +1,176 @@
> +// 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>
> +
> +#include "coresight-priv.h"
> +#include "coresight-trace-id.h"
> +
> +struct dummy_drvdata {
> +       struct device                   *dev;
> +       struct coresight_device         *csdev;
> +       int                             traceid;
> +};
> +
> +DEFINE_CORESIGHT_DEVLIST(dummy_devs, "dummy");
> +
> +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_info(drvdata->dev, "Dummy source enabled\n");
> +
> +       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_info(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_info(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_info(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_sink dummy_sink_ops = {
> +       .enable         = dummy_sink_enable,
> +       .disable        = dummy_sink_disable,
> +};
> +
> +static const struct coresight_ops dummy_cs_ops = {
> +       .source_ops     = &dummy_source_ops,
> +       .sink_ops       = &dummy_sink_ops,
> +};
> +
> +static int dummy_probe(struct platform_device *pdev)
> +{
> +       int ret, trace_id;
> +       struct device *dev = &pdev->dev;
> +       struct coresight_platform_data *pdata;
> +       struct dummy_drvdata *drvdata;
> +       struct coresight_desc desc = { 0 };
> +
> +       desc.name = coresight_alloc_device_name(&dummy_devs, dev);
> +       if (!desc.name)
> +               return -ENOMEM;
> +
> +       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);
> +
> +       if (of_property_read_bool(pdev->dev.of_node, "qcom,dummy-source")) {
> +               desc.type = CORESIGHT_DEV_TYPE_SOURCE;
> +               desc.subtype.source_subtype =
> +                                       CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS;
> +       } else if (of_property_read_bool(pdev->dev.of_node,
> +                                        "qcom,dummy-sink")) {
> +               desc.type = CORESIGHT_DEV_TYPE_SINK;
> +               desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;

This will break the automatic sink selection on a system where perf is
looking for a default sink and the dummy sink is closest  / first
discovered.

i.e. when perf record -e cs_etm// <options>
is used to trace a program in linux, a dummy sink appearing in the
coresight tree with this designation may be selected.

This needs to be corrected, probably with a unique sub-type that
appears before the CORESIGHT_DEV_SUBTYPE_SINK_BUFFER value in the enum
as the selection is based on >= CORESIGHT_DEV_SUBTYPE_SINK_BUFFER.

By implication adding a new value - will possibly affect other code
using the enum values so will need to be checked

Regards

Mike



> +       } else {
> +               dev_info(dev, "Device type not set\n");
> +               return -EINVAL;
> +       }
> +
> +       desc.ops = &dummy_cs_ops;
> +       desc.pdata = pdev->dev.platform_data;
> +       desc.dev = &pdev->dev;
> +       drvdata->csdev = coresight_register(&desc);
> +       if (IS_ERR(drvdata->csdev))
> +               return PTR_ERR(drvdata->csdev);
> +
> +       trace_id = coresight_trace_id_get_system_id();
> +       if (trace_id < 0) {
> +               ret = trace_id;
> +               goto cs_unregister;
> +       }
> +       drvdata->traceid = (u8)trace_id;
> +
> +       pm_runtime_enable(dev);
> +       dev_info(dev, "Dummy device initialized\n");
> +
> +       return 0;
> +
> +cs_unregister:
> +       coresight_unregister(drvdata->csdev);
> +
> +       return ret;
> +}
> +
> +static int dummy_remove(struct platform_device *pdev)
> +{
> +       struct dummy_drvdata *drvdata = platform_get_drvdata(pdev);
> +       struct device *dev = &pdev->dev;
> +
> +       coresight_trace_id_put_system_id(drvdata->traceid);
> +       pm_runtime_disable(dev);
> +       coresight_unregister(drvdata->csdev);
> +       return 0;
> +}
> +
> +static const struct of_device_id dummy_match[] = {
> +       {.compatible = "qcom,coresight-dummy"},
> +       {},
> +};
> +
> +static struct platform_driver dummy_driver = {
> +       .probe  = dummy_probe,
> +       .remove = dummy_remove,
> +       .driver = {
> +               .name   = "coresight-dummy",
> +               .of_match_table = dummy_match,
> +       },
> +};
> +
> +static int __init dummy_init(void)
> +{
> +       return platform_driver_register(&dummy_driver);
> +}
> +module_init(dummy_init);
> +
> +static void __exit dummy_exit(void)
> +{
> +       platform_driver_unregister(&dummy_driver);
> +}
> +module_exit(dummy_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("CoreSight dummy source driver");
> --
> 2.17.1
>


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

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

[Hao Zhang]

Hi Mike,

On 3/27/2023 11:58 PM, Mike Leach wrote:
> Hi,
> 
> On Fri, 24 Mar 2023 at 06:16, Hao Zhang <quic_hazha@quicinc.com> wrote:
>>
>> Some Coresight devices that HLOS don't have permission to access
>> or configure. Such as Coresight sink EUD, some TPDMs etc. So there
>> need driver to register dummy devices as Coresight devices. 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@quicinc.com>
>> ---
>>   drivers/hwtracing/coresight/Kconfig           |  11 ++
>>   drivers/hwtracing/coresight/Makefile          |   1 +
>>   drivers/hwtracing/coresight/coresight-dummy.c | 176 ++++++++++++++++++
>>   3 files changed, 188 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..2d4eb3e546eb
>> --- /dev/null
>> +++ b/drivers/hwtracing/coresight/coresight-dummy.c
>> @@ -0,0 +1,176 @@
>> +// 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>
>> +
>> +#include "coresight-priv.h"
>> +#include "coresight-trace-id.h"
>> +
>> +struct dummy_drvdata {
>> +       struct device                   *dev;
>> +       struct coresight_device         *csdev;
>> +       int                             traceid;
>> +};
>> +
>> +DEFINE_CORESIGHT_DEVLIST(dummy_devs, "dummy");
>> +
>> +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_info(drvdata->dev, "Dummy source enabled\n");
>> +
>> +       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_info(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_info(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_info(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_sink dummy_sink_ops = {
>> +       .enable         = dummy_sink_enable,
>> +       .disable        = dummy_sink_disable,
>> +};
>> +
>> +static const struct coresight_ops dummy_cs_ops = {
>> +       .source_ops     = &dummy_source_ops,
>> +       .sink_ops       = &dummy_sink_ops,
>> +};
>> +
>> +static int dummy_probe(struct platform_device *pdev)
>> +{
>> +       int ret, trace_id;
>> +       struct device *dev = &pdev->dev;
>> +       struct coresight_platform_data *pdata;
>> +       struct dummy_drvdata *drvdata;
>> +       struct coresight_desc desc = { 0 };
>> +
>> +       desc.name = coresight_alloc_device_name(&dummy_devs, dev);
>> +       if (!desc.name)
>> +               return -ENOMEM;
>> +
>> +       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);
>> +
>> +       if (of_property_read_bool(pdev->dev.of_node, "qcom,dummy-source")) {
>> +               desc.type = CORESIGHT_DEV_TYPE_SOURCE;
>> +               desc.subtype.source_subtype =
>> +                                       CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS;
>> +       } else if (of_property_read_bool(pdev->dev.of_node,
>> +                                        "qcom,dummy-sink")) {
>> +               desc.type = CORESIGHT_DEV_TYPE_SINK;
>> +               desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
> 
> This will break the automatic sink selection on a system where perf is
> looking for a default sink and the dummy sink is closest  / first
> discovered.
> 
> i.e. when perf record -e cs_etm// <options>
> is used to trace a program in linux, a dummy sink appearing in the
> coresight tree with this designation may be selected.
> 
> This needs to be corrected, probably with a unique sub-type that
> appears before the CORESIGHT_DEV_SUBTYPE_SINK_BUFFER value in the enum
> as the selection is based on >= CORESIGHT_DEV_SUBTYPE_SINK_BUFFER.
> 
> By implication adding a new value - will possibly affect other code
> using the enum values so will need to be checked
> 
> Regards
> 
> Mike
> 

Thanks for your comments, I will add a new sub-type for dummy sink and 
check the impact of it.

Thanks,
Hao

> 
>> +       } else {
>> +               dev_info(dev, "Device type not set\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       desc.ops = &dummy_cs_ops;
>> +       desc.pdata = pdev->dev.platform_data;
>> +       desc.dev = &pdev->dev;
>> +       drvdata->csdev = coresight_register(&desc);
>> +       if (IS_ERR(drvdata->csdev))
>> +               return PTR_ERR(drvdata->csdev);
>> +
>> +       trace_id = coresight_trace_id_get_system_id();
>> +       if (trace_id < 0) {
>> +               ret = trace_id;
>> +               goto cs_unregister;
>> +       }
>> +       drvdata->traceid = (u8)trace_id;
>> +
>> +       pm_runtime_enable(dev);
>> +       dev_info(dev, "Dummy device initialized\n");
>> +
>> +       return 0;
>> +
>> +cs_unregister:
>> +       coresight_unregister(drvdata->csdev);
>> +
>> +       return ret;
>> +}
>> +
>> +static int dummy_remove(struct platform_device *pdev)
>> +{
>> +       struct dummy_drvdata *drvdata = platform_get_drvdata(pdev);
>> +       struct device *dev = &pdev->dev;
>> +
>> +       coresight_trace_id_put_system_id(drvdata->traceid);
>> +       pm_runtime_disable(dev);
>> +       coresight_unregister(drvdata->csdev);
>> +       return 0;
>> +}
>> +
>> +static const struct of_device_id dummy_match[] = {
>> +       {.compatible = "qcom,coresight-dummy"},
>> +       {},
>> +};
>> +
>> +static struct platform_driver dummy_driver = {
>> +       .probe  = dummy_probe,
>> +       .remove = dummy_remove,
>> +       .driver = {
>> +               .name   = "coresight-dummy",
>> +               .of_match_table = dummy_match,
>> +       },
>> +};
>> +
>> +static int __init dummy_init(void)
>> +{
>> +       return platform_driver_register(&dummy_driver);
>> +}
>> +module_init(dummy_init);
>> +
>> +static void __exit dummy_exit(void)
>> +{
>> +       platform_driver_unregister(&dummy_driver);
>> +}
>> +module_exit(dummy_exit);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("CoreSight dummy source driver");
>> --
>> 2.17.1
>>
> 
> 

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

[Suzuki K Poulose]

On 28/03/2023 08:22, Hao Zhang wrote:
> Hi Mike,
> 
> On 3/27/2023 11:58 PM, Mike Leach wrote:
>> Hi,
>>
>> On Fri, 24 Mar 2023 at 06:16, Hao Zhang <quic_hazha@quicinc.com> wrote:
>>>
>>> Some Coresight devices that HLOS don't have permission to access
>>> or configure. Such as Coresight sink EUD, some TPDMs etc. So there
>>> need driver to register dummy devices as Coresight devices. 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@quicinc.com>
>>> ---
>>>   drivers/hwtracing/coresight/Kconfig           |  11 ++
>>>   drivers/hwtracing/coresight/Makefile          |   1 +
>>>   drivers/hwtracing/coresight/coresight-dummy.c | 176 ++++++++++++++++++
>>>   3 files changed, 188 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..2d4eb3e546eb
>>> --- /dev/null
>>> +++ b/drivers/hwtracing/coresight/coresight-dummy.c
>>> @@ -0,0 +1,176 @@
>>> +// 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>
>>> +
>>> +#include "coresight-priv.h"
>>> +#include "coresight-trace-id.h"
>>> +
>>> +struct dummy_drvdata {
>>> +       struct device                   *dev;
>>> +       struct coresight_device         *csdev;
>>> +       int                             traceid;
>>> +};
>>> +
>>> +DEFINE_CORESIGHT_DEVLIST(dummy_devs, "dummy");
>>> +
>>> +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_info(drvdata->dev, "Dummy source enabled\n");
>>> +
>>> +       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_info(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_info(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_info(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_sink dummy_sink_ops = {
>>> +       .enable         = dummy_sink_enable,
>>> +       .disable        = dummy_sink_disable,
>>> +};
>>> +
>>> +static const struct coresight_ops dummy_cs_ops = {
>>> +       .source_ops     = &dummy_source_ops,
>>> +       .sink_ops       = &dummy_sink_ops,
>>> +};
>>> +
>>> +static int dummy_probe(struct platform_device *pdev)
>>> +{
>>> +       int ret, trace_id;
>>> +       struct device *dev = &pdev->dev;
>>> +       struct coresight_platform_data *pdata;
>>> +       struct dummy_drvdata *drvdata;
>>> +       struct coresight_desc desc = { 0 };
>>> +
>>> +       desc.name = coresight_alloc_device_name(&dummy_devs, dev);
>>> +       if (!desc.name)
>>> +               return -ENOMEM;
>>> +
>>> +       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);
>>> +
>>> +       if (of_property_read_bool(pdev->dev.of_node, 
>>> "qcom,dummy-source")) {
>>> +               desc.type = CORESIGHT_DEV_TYPE_SOURCE;
>>> +               desc.subtype.source_subtype =
>>> +                                       
>>> CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS;
>>> +       } else if (of_property_read_bool(pdev->dev.of_node,
>>> +                                        "qcom,dummy-sink")) {
>>> +               desc.type = CORESIGHT_DEV_TYPE_SINK;
>>> +               desc.subtype.sink_subtype = 
>>> CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
>>
>> This will break the automatic sink selection on a system where perf is
>> looking for a default sink and the dummy sink is closest  / first
>> discovered.
>>
>> i.e. when perf record -e cs_etm// <options>
>> is used to trace a program in linux, a dummy sink appearing in the
>> coresight tree with this designation may be selected.
>>
>> This needs to be corrected, probably with a unique sub-type that
>> appears before the CORESIGHT_DEV_SUBTYPE_SINK_BUFFER value in the enum
>> as the selection is based on >= CORESIGHT_DEV_SUBTYPE_SINK_BUFFER.
>>

Good point Mike.

>> By implication adding a new value - will possibly affect other code
>> using the enum values so will need to be checked
>>
>> Regards
>>
>> Mike
>>
> 
> Thanks for your comments, I will add a new sub-type for dummy sink and 
> check the impact of it.

Please keep this as the lowest priority, something like:

  enum coresight_dev_subtype_sink {
+	CORESIGHT_DEV_SUBTYPE_SINK_DUMMY,
         CORESIGHT_DEV_SUBTYPE_SINK_PORT,
         CORESIGHT_DEV_SUBTYPE_SINK_BUFFER,
         CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM,
         CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM,
};

This should be fine without any impact on the existing code, as we
expect the driver modules to be updated with the new core module.

Suzuki



> 
> Thanks,
> Hao
> 
>>
>>> +       } else {
>>> +               dev_info(dev, "Device type not set\n");
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       desc.ops = &dummy_cs_ops;
>>> +       desc.pdata = pdev->dev.platform_data;
>>> +       desc.dev = &pdev->dev;
>>> +       drvdata->csdev = coresight_register(&desc);
>>> +       if (IS_ERR(drvdata->csdev))
>>> +               return PTR_ERR(drvdata->csdev);
>>> +
>>> +       trace_id = coresight_trace_id_get_system_id();
>>> +       if (trace_id < 0) {
>>> +               ret = trace_id;
>>> +               goto cs_unregister;
>>> +       }
>>> +       drvdata->traceid = (u8)trace_id;
>>> +
>>> +       pm_runtime_enable(dev);
>>> +       dev_info(dev, "Dummy device initialized\n");
>>> +
>>> +       return 0;
>>> +
>>> +cs_unregister:
>>> +       coresight_unregister(drvdata->csdev);
>>> +
>>> +       return ret;
>>> +}
>>> +
>>> +static int dummy_remove(struct platform_device *pdev)
>>> +{
>>> +       struct dummy_drvdata *drvdata = platform_get_drvdata(pdev);
>>> +       struct device *dev = &pdev->dev;
>>> +
>>> +       coresight_trace_id_put_system_id(drvdata->traceid);
>>> +       pm_runtime_disable(dev);
>>> +       coresight_unregister(drvdata->csdev);
>>> +       return 0;
>>> +}
>>> +
>>> +static const struct of_device_id dummy_match[] = {
>>> +       {.compatible = "qcom,coresight-dummy"},
>>> +       {},
>>> +};
>>> +
>>> +static struct platform_driver dummy_driver = {
>>> +       .probe  = dummy_probe,
>>> +       .remove = dummy_remove,
>>> +       .driver = {
>>> +               .name   = "coresight-dummy",
>>> +               .of_match_table = dummy_match,
>>> +       },
>>> +};
>>> +
>>> +static int __init dummy_init(void)
>>> +{
>>> +       return platform_driver_register(&dummy_driver);
>>> +}
>>> +module_init(dummy_init);
>>> +
>>> +static void __exit dummy_exit(void)
>>> +{
>>> +       platform_driver_unregister(&dummy_driver);
>>> +}
>>> +module_exit(dummy_exit);
>>> +
>>> +MODULE_LICENSE("GPL");
>>> +MODULE_DESCRIPTION("CoreSight dummy source driver");
>>> -- 
>>> 2.17.1
>>>
>>
>>

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

[Hao Zhang]

Hi Suzuki,

On 3/28/2023 4:35 PM, Suzuki K Poulose wrote:
> On 28/03/2023 08:22, Hao Zhang wrote:
>> Hi Mike,
>>
>> On 3/27/2023 11:58 PM, Mike Leach wrote:
>>> Hi,
>>>
>>> On Fri, 24 Mar 2023 at 06:16, Hao Zhang <quic_hazha@quicinc.com> wrote:
>>>>
>>>> Some Coresight devices that HLOS don't have permission to access
>>>> or configure. Such as Coresight sink EUD, some TPDMs etc. So there
>>>> need driver to register dummy devices as Coresight devices. 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@quicinc.com>
>>>> ---
>>>>   drivers/hwtracing/coresight/Kconfig           |  11 ++
>>>>   drivers/hwtracing/coresight/Makefile          |   1 +
>>>>   drivers/hwtracing/coresight/coresight-dummy.c | 176 
>>>> ++++++++++++++++++
>>>>   3 files changed, 188 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..2d4eb3e546eb
>>>> --- /dev/null
>>>> +++ b/drivers/hwtracing/coresight/coresight-dummy.c
>>>> @@ -0,0 +1,176 @@
>>>> +// 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>
>>>> +
>>>> +#include "coresight-priv.h"
>>>> +#include "coresight-trace-id.h"
>>>> +
>>>> +struct dummy_drvdata {
>>>> +       struct device                   *dev;
>>>> +       struct coresight_device         *csdev;
>>>> +       int                             traceid;
>>>> +};
>>>> +
>>>> +DEFINE_CORESIGHT_DEVLIST(dummy_devs, "dummy");
>>>> +
>>>> +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_info(drvdata->dev, "Dummy source enabled\n");
>>>> +
>>>> +       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_info(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_info(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_info(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_sink dummy_sink_ops = {
>>>> +       .enable         = dummy_sink_enable,
>>>> +       .disable        = dummy_sink_disable,
>>>> +};
>>>> +
>>>> +static const struct coresight_ops dummy_cs_ops = {
>>>> +       .source_ops     = &dummy_source_ops,
>>>> +       .sink_ops       = &dummy_sink_ops,
>>>> +};
>>>> +
>>>> +static int dummy_probe(struct platform_device *pdev)
>>>> +{
>>>> +       int ret, trace_id;
>>>> +       struct device *dev = &pdev->dev;
>>>> +       struct coresight_platform_data *pdata;
>>>> +       struct dummy_drvdata *drvdata;
>>>> +       struct coresight_desc desc = { 0 };
>>>> +
>>>> +       desc.name = coresight_alloc_device_name(&dummy_devs, dev);
>>>> +       if (!desc.name)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       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);
>>>> +
>>>> +       if (of_property_read_bool(pdev->dev.of_node, 
>>>> "qcom,dummy-source")) {
>>>> +               desc.type = CORESIGHT_DEV_TYPE_SOURCE;
>>>> +               desc.subtype.source_subtype =
>>>> + CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS;
>>>> +       } else if (of_property_read_bool(pdev->dev.of_node,
>>>> +                                        "qcom,dummy-sink")) {
>>>> +               desc.type = CORESIGHT_DEV_TYPE_SINK;
>>>> +               desc.subtype.sink_subtype = 
>>>> CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
>>>
>>> This will break the automatic sink selection on a system where perf is
>>> looking for a default sink and the dummy sink is closest  / first
>>> discovered.
>>>
>>> i.e. when perf record -e cs_etm// <options>
>>> is used to trace a program in linux, a dummy sink appearing in the
>>> coresight tree with this designation may be selected.
>>>
>>> This needs to be corrected, probably with a unique sub-type that
>>> appears before the CORESIGHT_DEV_SUBTYPE_SINK_BUFFER value in the enum
>>> as the selection is based on >= CORESIGHT_DEV_SUBTYPE_SINK_BUFFER.
>>>
> 
> Good point Mike.
> 
>>> By implication adding a new value - will possibly affect other code
>>> using the enum values so will need to be checked
>>>
>>> Regards
>>>
>>> Mike
>>>
>>
>> Thanks for your comments, I will add a new sub-type for dummy sink and 
>> check the impact of it.
> 
> Please keep this as the lowest priority, something like:
> 
>   enum coresight_dev_subtype_sink {
> +    CORESIGHT_DEV_SUBTYPE_SINK_DUMMY,
>          CORESIGHT_DEV_SUBTYPE_SINK_PORT,
>          CORESIGHT_DEV_SUBTYPE_SINK_BUFFER,
>          CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM,
>          CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM,
> };
> 
> This should be fine without any impact on the existing code, as we
> expect the driver modules to be updated with the new core module.
> 
> Suzuki
> 

Sure, I will take your advice in the next version of patch.

Thanks,
Hao

> 
>>
>> Thanks,
>> Hao
>>
>>>
>>>> +       } else {
>>>> +               dev_info(dev, "Device type not set\n");
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       desc.ops = &dummy_cs_ops;
>>>> +       desc.pdata = pdev->dev.platform_data;
>>>> +       desc.dev = &pdev->dev;
>>>> +       drvdata->csdev = coresight_register(&desc);
>>>> +       if (IS_ERR(drvdata->csdev))
>>>> +               return PTR_ERR(drvdata->csdev);
>>>> +
>>>> +       trace_id = coresight_trace_id_get_system_id();
>>>> +       if (trace_id < 0) {
>>>> +               ret = trace_id;
>>>> +               goto cs_unregister;
>>>> +       }
>>>> +       drvdata->traceid = (u8)trace_id;
>>>> +
>>>> +       pm_runtime_enable(dev);
>>>> +       dev_info(dev, "Dummy device initialized\n");
>>>> +
>>>> +       return 0;
>>>> +
>>>> +cs_unregister:
>>>> +       coresight_unregister(drvdata->csdev);
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +
>>>> +static int dummy_remove(struct platform_device *pdev)
>>>> +{
>>>> +       struct dummy_drvdata *drvdata = platform_get_drvdata(pdev);
>>>> +       struct device *dev = &pdev->dev;
>>>> +
>>>> +       coresight_trace_id_put_system_id(drvdata->traceid);
>>>> +       pm_runtime_disable(dev);
>>>> +       coresight_unregister(drvdata->csdev);
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static const struct of_device_id dummy_match[] = {
>>>> +       {.compatible = "qcom,coresight-dummy"},
>>>> +       {},
>>>> +};
>>>> +
>>>> +static struct platform_driver dummy_driver = {
>>>> +       .probe  = dummy_probe,
>>>> +       .remove = dummy_remove,
>>>> +       .driver = {
>>>> +               .name   = "coresight-dummy",
>>>> +               .of_match_table = dummy_match,
>>>> +       },
>>>> +};
>>>> +
>>>> +static int __init dummy_init(void)
>>>> +{
>>>> +       return platform_driver_register(&dummy_driver);
>>>> +}
>>>> +module_init(dummy_init);
>>>> +
>>>> +static void __exit dummy_exit(void)
>>>> +{
>>>> +       platform_driver_unregister(&dummy_driver);
>>>> +}
>>>> +module_exit(dummy_exit);
>>>> +
>>>> +MODULE_LICENSE("GPL");
>>>> +MODULE_DESCRIPTION("CoreSight dummy source driver");
>>>> -- 
>>>> 2.17.1
>>>>
>>>
>>>
> 

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

[Mike Leach]

Hi,

A few additional comments....

On Tue, 28 Mar 2023 at 10:24, Hao Zhang <quic_hazha@quicinc.com> wrote:
>
> Hi Suzuki,
>
> On 3/28/2023 4:35 PM, Suzuki K Poulose wrote:
> > On 28/03/2023 08:22, Hao Zhang wrote:
> >> Hi Mike,
> >>
> >> On 3/27/2023 11:58 PM, Mike Leach wrote:
> >>> Hi,
> >>>
> >>> On Fri, 24 Mar 2023 at 06:16, Hao Zhang <quic_hazha@quicinc.com> wrote:
> >>>>
> >>>> Some Coresight devices that HLOS don't have permission to access
> >>>> or configure. Such as Coresight sink EUD, some TPDMs etc. So there
> >>>> need driver to register dummy devices as Coresight devices. 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@quicinc.com>
> >>>> ---
> >>>>   drivers/hwtracing/coresight/Kconfig           |  11 ++
> >>>>   drivers/hwtracing/coresight/Makefile          |   1 +
> >>>>   drivers/hwtracing/coresight/coresight-dummy.c | 176
> >>>> ++++++++++++++++++
> >>>>   3 files changed, 188 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..2d4eb3e546eb
> >>>> --- /dev/null
> >>>> +++ b/drivers/hwtracing/coresight/coresight-dummy.c
> >>>> @@ -0,0 +1,176 @@
> >>>> +// 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>
> >>>> +
> >>>> +#include "coresight-priv.h"
> >>>> +#include "coresight-trace-id.h"
> >>>> +
> >>>> +struct dummy_drvdata {
> >>>> +       struct device                   *dev;
> >>>> +       struct coresight_device         *csdev;
> >>>> +       int                             traceid;
> >>>> +};
> >>>> +
> >>>> +DEFINE_CORESIGHT_DEVLIST(dummy_devs, "dummy");
> >>>> +

minor nit: can we have dummy_source and dummy_sink as the device names
to make it clear at the first level what these are without having to
look at the attributes?

> >>>> +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_info(drvdata->dev, "Dummy source enabled\n");
> >>>> +
> >>>> +       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_info(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_info(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_info(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_sink dummy_sink_ops = {
> >>>> +       .enable         = dummy_sink_enable,
> >>>> +       .disable        = dummy_sink_disable,
> >>>> +};
> >>>> +
> >>>> +static const struct coresight_ops dummy_cs_ops = {
> >>>> +       .source_ops     = &dummy_source_ops,
> >>>> +       .sink_ops       = &dummy_sink_ops,
> >>>> +};
> >>>> +
> >>>> +static int dummy_probe(struct platform_device *pdev)
> >>>> +{
> >>>> +       int ret, trace_id;
> >>>> +       struct device *dev = &pdev->dev;
> >>>> +       struct coresight_platform_data *pdata;
> >>>> +       struct dummy_drvdata *drvdata;
> >>>> +       struct coresight_desc desc = { 0 };
> >>>> +
> >>>> +       desc.name = coresight_alloc_device_name(&dummy_devs, dev);
> >>>> +       if (!desc.name)
> >>>> +               return -ENOMEM;
> >>>> +
> >>>> +       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);
> >>>> +
> >>>> +       if (of_property_read_bool(pdev->dev.of_node,
> >>>> "qcom,dummy-source")) {
> >>>> +               desc.type = CORESIGHT_DEV_TYPE_SOURCE;
> >>>> +               desc.subtype.source_subtype =
> >>>> + CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS;
> >>>> +       } else if (of_property_read_bool(pdev->dev.of_node,
> >>>> +                                        "qcom,dummy-sink")) {

It would simplify things if the compatibles were
arm,coresight-dummy-source and arm,coresight-dummy-sink - and drop the
two additional attributes, using of_device_is_compatible() here.

> >>>> +               desc.type = CORESIGHT_DEV_TYPE_SINK;
> >>>> +               desc.subtype.sink_subtype =
> >>>> CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
> >>>
> >>> This will break the automatic sink selection on a system where perf is
> >>> looking for a default sink and the dummy sink is closest  / first
> >>> discovered.
> >>>
> >>> i.e. when perf record -e cs_etm// <options>
> >>> is used to trace a program in linux, a dummy sink appearing in the
> >>> coresight tree with this designation may be selected.
> >>>
> >>> This needs to be corrected, probably with a unique sub-type that
> >>> appears before the CORESIGHT_DEV_SUBTYPE_SINK_BUFFER value in the enum
> >>> as the selection is based on >= CORESIGHT_DEV_SUBTYPE_SINK_BUFFER.
> >>>
> >
> > Good point Mike.
> >
> >>> By implication adding a new value - will possibly affect other code
> >>> using the enum values so will need to be checked
> >>>
> >>> Regards
> >>>
> >>> Mike
> >>>
> >>
> >> Thanks for your comments, I will add a new sub-type for dummy sink and
> >> check the impact of it.
> >
> > Please keep this as the lowest priority, something like:
> >
> >   enum coresight_dev_subtype_sink {
> > +    CORESIGHT_DEV_SUBTYPE_SINK_DUMMY,
> >          CORESIGHT_DEV_SUBTYPE_SINK_PORT,
> >          CORESIGHT_DEV_SUBTYPE_SINK_BUFFER,
> >          CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM,
> >          CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM,
> > };
> >
> > This should be fine without any impact on the existing code, as we
> > expect the driver modules to be updated with the new core module.
> >
> > Suzuki
> >
>
> Sure, I will take your advice in the next version of patch.
>
> Thanks,
> Hao
>
> >
> >>
> >> Thanks,
> >> Hao
> >>
> >>>
> >>>> +       } else {
> >>>> +               dev_info(dev, "Device type not set\n");
> >>>> +               return -EINVAL;
> >>>> +       }
> >>>> +
> >>>> +       desc.ops = &dummy_cs_ops;
> >>>> +       desc.pdata = pdev->dev.platform_data;
> >>>> +       desc.dev = &pdev->dev;
> >>>> +       drvdata->csdev = coresight_register(&desc);
> >>>> +       if (IS_ERR(drvdata->csdev))
> >>>> +               return PTR_ERR(drvdata->csdev);
> >>>> +
> >>>> +       trace_id = coresight_trace_id_get_system_id();
> >>>> +       if (trace_id < 0) {
> >>>> +               ret = trace_id;
> >>>> +               goto cs_unregister;
> >>>> +       }
> >>>> +       drvdata->traceid = (u8)trace_id;
> >>>> +

Number of issues here:-
1) Why are sinks being given a trace ID? - they do not need them.
2) how is the trace ID communicated to the underlying hardware system?
- there appears to be no way of doing this here. Without this you
cannot guarantee that there will not be clashes.
Although your use case may mitigate against this - for this to be a
generic module there must be a way to ensure the IDs can be discovered
externally.
3) Trace IDs are a limited resource - most sources allocate on enable,
release on disable  / reset - this would be preferable.


Regards

Mike

> >>>> +       pm_runtime_enable(dev);
> >>>> +       dev_info(dev, "Dummy device initialized\n");
> >>>> +
> >>>> +       return 0;
> >>>> +
> >>>> +cs_unregister:
> >>>> +       coresight_unregister(drvdata->csdev);
> >>>> +
> >>>> +       return ret;
> >>>> +}
> >>>> +
> >>>> +static int dummy_remove(struct platform_device *pdev)
> >>>> +{
> >>>> +       struct dummy_drvdata *drvdata = platform_get_drvdata(pdev);
> >>>> +       struct device *dev = &pdev->dev;
> >>>> +
> >>>> +       coresight_trace_id_put_system_id(drvdata->traceid);
> >>>> +       pm_runtime_disable(dev);
> >>>> +       coresight_unregister(drvdata->csdev);
> >>>> +       return 0;
> >>>> +}
> >>>> +
> >>>> +static const struct of_device_id dummy_match[] = {
> >>>> +       {.compatible = "qcom,coresight-dummy"},
> >>>> +       {},
> >>>> +};
> >>>> +
> >>>> +static struct platform_driver dummy_driver = {
> >>>> +       .probe  = dummy_probe,
> >>>> +       .remove = dummy_remove,
> >>>> +       .driver = {
> >>>> +               .name   = "coresight-dummy",
> >>>> +               .of_match_table = dummy_match,
> >>>> +       },
> >>>> +};
> >>>> +
> >>>> +static int __init dummy_init(void)
> >>>> +{
> >>>> +       return platform_driver_register(&dummy_driver);
> >>>> +}
> >>>> +module_init(dummy_init);
> >>>> +
> >>>> +static void __exit dummy_exit(void)
> >>>> +{
> >>>> +       platform_driver_unregister(&dummy_driver);
> >>>> +}
> >>>> +module_exit(dummy_exit);
> >>>> +
> >>>> +MODULE_LICENSE("GPL");
> >>>> +MODULE_DESCRIPTION("CoreSight dummy source driver");
> >>>> --
> >>>> 2.17.1
> >>>>
> >>>
> >>>
> >



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

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

[Hao Zhang]

Hi Mike,

On 3/28/2023 6:06 PM, Mike Leach wrote:
> Hi,
> 
> A few additional comments....
> 
> On Tue, 28 Mar 2023 at 10:24, Hao Zhang <quic_hazha@quicinc.com> wrote:
>>
>> Hi Suzuki,
>>
>> On 3/28/2023 4:35 PM, Suzuki K Poulose wrote:
>>> On 28/03/2023 08:22, Hao Zhang wrote:
>>>> Hi Mike,
>>>>
>>>> On 3/27/2023 11:58 PM, Mike Leach wrote:
>>>>> Hi,
>>>>>
>>>>> On Fri, 24 Mar 2023 at 06:16, Hao Zhang <quic_hazha@quicinc.com> wrote:
>>>>>>
>>>>>> Some Coresight devices that HLOS don't have permission to access
>>>>>> or configure. Such as Coresight sink EUD, some TPDMs etc. So there
>>>>>> need driver to register dummy devices as Coresight devices. 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@quicinc.com>
>>>>>> ---
>>>>>>    drivers/hwtracing/coresight/Kconfig           |  11 ++
>>>>>>    drivers/hwtracing/coresight/Makefile          |   1 +
>>>>>>    drivers/hwtracing/coresight/coresight-dummy.c | 176
>>>>>> ++++++++++++++++++
>>>>>>    3 files changed, 188 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..2d4eb3e546eb
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/hwtracing/coresight/coresight-dummy.c
>>>>>> @@ -0,0 +1,176 @@
>>>>>> +// 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>
>>>>>> +
>>>>>> +#include "coresight-priv.h"
>>>>>> +#include "coresight-trace-id.h"
>>>>>> +
>>>>>> +struct dummy_drvdata {
>>>>>> +       struct device                   *dev;
>>>>>> +       struct coresight_device         *csdev;
>>>>>> +       int                             traceid;
>>>>>> +};
>>>>>> +
>>>>>> +DEFINE_CORESIGHT_DEVLIST(dummy_devs, "dummy");
>>>>>> +
> 
> minor nit: can we have dummy_source and dummy_sink as the device names
> to make it clear at the first level what these are without having to
> look at the attributes?
> 

This is a good advice, dummy_source and dummy_sink are two different 
components, so it's better to separate it at the first level. I will 
take your advice in the next version of patch.

>>>>>> +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_info(drvdata->dev, "Dummy source enabled\n");
>>>>>> +
>>>>>> +       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_info(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_info(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_info(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_sink dummy_sink_ops = {
>>>>>> +       .enable         = dummy_sink_enable,
>>>>>> +       .disable        = dummy_sink_disable,
>>>>>> +};
>>>>>> +
>>>>>> +static const struct coresight_ops dummy_cs_ops = {
>>>>>> +       .source_ops     = &dummy_source_ops,
>>>>>> +       .sink_ops       = &dummy_sink_ops,
>>>>>> +};
>>>>>> +
>>>>>> +static int dummy_probe(struct platform_device *pdev)
>>>>>> +{
>>>>>> +       int ret, trace_id;
>>>>>> +       struct device *dev = &pdev->dev;
>>>>>> +       struct coresight_platform_data *pdata;
>>>>>> +       struct dummy_drvdata *drvdata;
>>>>>> +       struct coresight_desc desc = { 0 };
>>>>>> +
>>>>>> +       desc.name = coresight_alloc_device_name(&dummy_devs, dev);
>>>>>> +       if (!desc.name)
>>>>>> +               return -ENOMEM;
>>>>>> +
>>>>>> +       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);
>>>>>> +
>>>>>> +       if (of_property_read_bool(pdev->dev.of_node,
>>>>>> "qcom,dummy-source")) {
>>>>>> +               desc.type = CORESIGHT_DEV_TYPE_SOURCE;
>>>>>> +               desc.subtype.source_subtype =
>>>>>> + CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS;
>>>>>> +       } else if (of_property_read_bool(pdev->dev.of_node,
>>>>>> +                                        "qcom,dummy-sink")) {
> 
> It would simplify things if the compatibles were
> arm,coresight-dummy-source and arm,coresight-dummy-sink - and drop the
> two additional attributes, using of_device_is_compatible() here.
> 

Yes, I will update it in the next version of patch.

>>>>>> +               desc.type = CORESIGHT_DEV_TYPE_SINK;
>>>>>> +               desc.subtype.sink_subtype =
>>>>>> CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
>>>>>
>>>>> This will break the automatic sink selection on a system where perf is
>>>>> looking for a default sink and the dummy sink is closest  / first
>>>>> discovered.
>>>>>
>>>>> i.e. when perf record -e cs_etm// <options>
>>>>> is used to trace a program in linux, a dummy sink appearing in the
>>>>> coresight tree with this designation may be selected.
>>>>>
>>>>> This needs to be corrected, probably with a unique sub-type that
>>>>> appears before the CORESIGHT_DEV_SUBTYPE_SINK_BUFFER value in the enum
>>>>> as the selection is based on >= CORESIGHT_DEV_SUBTYPE_SINK_BUFFER.
>>>>>
>>>
>>> Good point Mike.
>>>
>>>>> By implication adding a new value - will possibly affect other code
>>>>> using the enum values so will need to be checked
>>>>>
>>>>> Regards
>>>>>
>>>>> Mike
>>>>>
>>>>
>>>> Thanks for your comments, I will add a new sub-type for dummy sink and
>>>> check the impact of it.
>>>
>>> Please keep this as the lowest priority, something like:
>>>
>>>    enum coresight_dev_subtype_sink {
>>> +    CORESIGHT_DEV_SUBTYPE_SINK_DUMMY,
>>>           CORESIGHT_DEV_SUBTYPE_SINK_PORT,
>>>           CORESIGHT_DEV_SUBTYPE_SINK_BUFFER,
>>>           CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM,
>>>           CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM,
>>> };
>>>
>>> This should be fine without any impact on the existing code, as we
>>> expect the driver modules to be updated with the new core module.
>>>
>>> Suzuki
>>>
>>
>> Sure, I will take your advice in the next version of patch.
>>
>> Thanks,
>> Hao
>>
>>>
>>>>
>>>> Thanks,
>>>> Hao
>>>>
>>>>>
>>>>>> +       } else {
>>>>>> +               dev_info(dev, "Device type not set\n");
>>>>>> +               return -EINVAL;
>>>>>> +       }
>>>>>> +
>>>>>> +       desc.ops = &dummy_cs_ops;
>>>>>> +       desc.pdata = pdev->dev.platform_data;
>>>>>> +       desc.dev = &pdev->dev;
>>>>>> +       drvdata->csdev = coresight_register(&desc);
>>>>>> +       if (IS_ERR(drvdata->csdev))
>>>>>> +               return PTR_ERR(drvdata->csdev);
>>>>>> +
>>>>>> +       trace_id = coresight_trace_id_get_system_id();
>>>>>> +       if (trace_id < 0) {
>>>>>> +               ret = trace_id;
>>>>>> +               goto cs_unregister;
>>>>>> +       }
>>>>>> +       drvdata->traceid = (u8)trace_id;
>>>>>> +
> 
> Number of issues here:-
> 1) Why are sinks being given a trace ID? - they do not need them.
> 2) how is the trace ID communicated to the underlying hardware system?
> - there appears to be no way of doing this here. Without this you
> cannot guarantee that there will not be clashes.
> Although your use case may mitigate against this - for this to be a
> generic module there must be a way to ensure the IDs can be discovered
> externally.
> 3) Trace IDs are a limited resource - most sources allocate on enable,
> release on disable  / reset - this would be preferable.
> 
> 
> Regards
> 
> Mike

1. It should not be given a trace ID for sink, I will correct it in the 
next version of patch.
2. There are other patches to transmit the trace ID to sub-processor. 
But We have an upstream dependency on QMI project. We will sync with 
them for the other related patches.
3. The trace ID of dummy source need to be communicated to the 
sub-processor, it's better to be allocated on probe, that would reduce 
communications costs. On the other hand, there will be few dummy 
sources. I'd perfer to allocate it on probe function.

Thanks,
Hao

> 
>>>>>> +       pm_runtime_enable(dev);
>>>>>> +       dev_info(dev, "Dummy device initialized\n");
>>>>>> +
>>>>>> +       return 0;
>>>>>> +
>>>>>> +cs_unregister:
>>>>>> +       coresight_unregister(drvdata->csdev);
>>>>>> +
>>>>>> +       return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int dummy_remove(struct platform_device *pdev)
>>>>>> +{
>>>>>> +       struct dummy_drvdata *drvdata = platform_get_drvdata(pdev);
>>>>>> +       struct device *dev = &pdev->dev;
>>>>>> +
>>>>>> +       coresight_trace_id_put_system_id(drvdata->traceid);
>>>>>> +       pm_runtime_disable(dev);
>>>>>> +       coresight_unregister(drvdata->csdev);
>>>>>> +       return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static const struct of_device_id dummy_match[] = {
>>>>>> +       {.compatible = "qcom,coresight-dummy"},
>>>>>> +       {},
>>>>>> +};
>>>>>> +
>>>>>> +static struct platform_driver dummy_driver = {
>>>>>> +       .probe  = dummy_probe,
>>>>>> +       .remove = dummy_remove,
>>>>>> +       .driver = {
>>>>>> +               .name   = "coresight-dummy",
>>>>>> +               .of_match_table = dummy_match,
>>>>>> +       },
>>>>>> +};
>>>>>> +
>>>>>> +static int __init dummy_init(void)
>>>>>> +{
>>>>>> +       return platform_driver_register(&dummy_driver);
>>>>>> +}
>>>>>> +module_init(dummy_init);
>>>>>> +
>>>>>> +static void __exit dummy_exit(void)
>>>>>> +{
>>>>>> +       platform_driver_unregister(&dummy_driver);
>>>>>> +}
>>>>>> +module_exit(dummy_exit);
>>>>>> +
>>>>>> +MODULE_LICENSE("GPL");
>>>>>> +MODULE_DESCRIPTION("CoreSight dummy source driver");
>>>>>> --
>>>>>> 2.17.1
>>>>>>
>>>>>
>>>>>
>>>
> 
> 
> 

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

[Suzuki K Poulose]

On 28/03/2023 12:25, Hao Zhang wrote:
> Hi Mike,
> 
> On 3/28/2023 6:06 PM, Mike Leach wrote:
>> Hi,
>>
>> A few additional comments....
>>
>> On Tue, 28 Mar 2023 at 10:24, Hao Zhang <quic_hazha@quicinc.com> wrote:
>>>
>>> Hi Suzuki,
>>>
>>> On 3/28/2023 4:35 PM, Suzuki K Poulose wrote:
>>>> On 28/03/2023 08:22, Hao Zhang wrote:
>>>>> Hi Mike,
>>>>>
>>>>> On 3/27/2023 11:58 PM, Mike Leach wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Fri, 24 Mar 2023 at 06:16, Hao Zhang <quic_hazha@quicinc.com> 
>>>>>> wrote:
>>>>>>>
>>>>>>> Some Coresight devices that HLOS don't have permission to access
>>>>>>> or configure. Such as Coresight sink EUD, some TPDMs etc. So there
>>>>>>> need driver to register dummy devices as Coresight devices. 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@quicinc.com>
>>>>>>> ---
>>>>>>>    drivers/hwtracing/coresight/Kconfig           |  11 ++
>>>>>>>    drivers/hwtracing/coresight/Makefile          |   1 +
>>>>>>>    drivers/hwtracing/coresight/coresight-dummy.c | 176
>>>>>>> ++++++++++++++++++
>>>>>>>    3 files changed, 188 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..2d4eb3e546eb
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/hwtracing/coresight/coresight-dummy.c
>>>>>>> @@ -0,0 +1,176 @@
>>>>>>> +// 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>
>>>>>>> +
>>>>>>> +#include "coresight-priv.h"
>>>>>>> +#include "coresight-trace-id.h"
>>>>>>> +
>>>>>>> +struct dummy_drvdata {
>>>>>>> +       struct device                   *dev;
>>>>>>> +       struct coresight_device         *csdev;
>>>>>>> +       int                             traceid;
>>>>>>> +};
>>>>>>> +
>>>>>>> +DEFINE_CORESIGHT_DEVLIST(dummy_devs, "dummy");
>>>>>>> +
>>
>> minor nit: can we have dummy_source and dummy_sink as the device names
>> to make it clear at the first level what these are without having to
>> look at the attributes?
>>
> 
> This is a good advice, dummy_source and dummy_sink are two different 
> components, so it's better to separate it at the first level. I will 
> take your advice in the next version of patch.
> 
>>>>>>> +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_info(drvdata->dev, "Dummy source enabled\n");
>>>>>>> +
>>>>>>> +       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_info(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_info(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_info(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_sink dummy_sink_ops = {
>>>>>>> +       .enable         = dummy_sink_enable,
>>>>>>> +       .disable        = dummy_sink_disable,
>>>>>>> +};
>>>>>>> +
>>>>>>> +static const struct coresight_ops dummy_cs_ops = {
>>>>>>> +       .source_ops     = &dummy_source_ops,
>>>>>>> +       .sink_ops       = &dummy_sink_ops,
>>>>>>> +};
>>>>>>> +
>>>>>>> +static int dummy_probe(struct platform_device *pdev)
>>>>>>> +{
>>>>>>> +       int ret, trace_id;
>>>>>>> +       struct device *dev = &pdev->dev;
>>>>>>> +       struct coresight_platform_data *pdata;
>>>>>>> +       struct dummy_drvdata *drvdata;
>>>>>>> +       struct coresight_desc desc = { 0 };
>>>>>>> +
>>>>>>> +       desc.name = coresight_alloc_device_name(&dummy_devs, dev);
>>>>>>> +       if (!desc.name)
>>>>>>> +               return -ENOMEM;
>>>>>>> +
>>>>>>> +       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);
>>>>>>> +
>>>>>>> +       if (of_property_read_bool(pdev->dev.of_node,
>>>>>>> "qcom,dummy-source")) {
>>>>>>> +               desc.type = CORESIGHT_DEV_TYPE_SOURCE;
>>>>>>> +               desc.subtype.source_subtype =
>>>>>>> + CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS;
>>>>>>> +       } else if (of_property_read_bool(pdev->dev.of_node,
>>>>>>> +                                        "qcom,dummy-sink")) {
>>
>> It would simplify things if the compatibles were
>> arm,coresight-dummy-source and arm,coresight-dummy-sink - and drop the
>> two additional attributes, using of_device_is_compatible() here.
>>
> 
> Yes, I will update it in the next version of patch.
> 
>>>>>>> +               desc.type = CORESIGHT_DEV_TYPE_SINK;
>>>>>>> +               desc.subtype.sink_subtype =
>>>>>>> CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
>>>>>>
>>>>>> This will break the automatic sink selection on a system where 
>>>>>> perf is
>>>>>> looking for a default sink and the dummy sink is closest  / first
>>>>>> discovered.
>>>>>>
>>>>>> i.e. when perf record -e cs_etm// <options>
>>>>>> is used to trace a program in linux, a dummy sink appearing in the
>>>>>> coresight tree with this designation may be selected.
>>>>>>
>>>>>> This needs to be corrected, probably with a unique sub-type that
>>>>>> appears before the CORESIGHT_DEV_SUBTYPE_SINK_BUFFER value in the 
>>>>>> enum
>>>>>> as the selection is based on >= CORESIGHT_DEV_SUBTYPE_SINK_BUFFER.
>>>>>>
>>>>
>>>> Good point Mike.
>>>>
>>>>>> By implication adding a new value - will possibly affect other code
>>>>>> using the enum values so will need to be checked
>>>>>>
>>>>>> Regards
>>>>>>
>>>>>> Mike
>>>>>>
>>>>>
>>>>> Thanks for your comments, I will add a new sub-type for dummy sink and
>>>>> check the impact of it.
>>>>
>>>> Please keep this as the lowest priority, something like:
>>>>
>>>>    enum coresight_dev_subtype_sink {
>>>> +    CORESIGHT_DEV_SUBTYPE_SINK_DUMMY,
>>>>           CORESIGHT_DEV_SUBTYPE_SINK_PORT,
>>>>           CORESIGHT_DEV_SUBTYPE_SINK_BUFFER,
>>>>           CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM,
>>>>           CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM,
>>>> };
>>>>
>>>> This should be fine without any impact on the existing code, as we
>>>> expect the driver modules to be updated with the new core module.
>>>>
>>>> Suzuki
>>>>
>>>
>>> Sure, I will take your advice in the next version of patch.
>>>
>>> Thanks,
>>> Hao
>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>> Hao
>>>>>
>>>>>>
>>>>>>> +       } else {
>>>>>>> +               dev_info(dev, "Device type not set\n");
>>>>>>> +               return -EINVAL;
>>>>>>> +       }
>>>>>>> +
>>>>>>> +       desc.ops = &dummy_cs_ops;
>>>>>>> +       desc.pdata = pdev->dev.platform_data;
>>>>>>> +       desc.dev = &pdev->dev;
>>>>>>> +       drvdata->csdev = coresight_register(&desc);
>>>>>>> +       if (IS_ERR(drvdata->csdev))
>>>>>>> +               return PTR_ERR(drvdata->csdev);
>>>>>>> +
>>>>>>> +       trace_id = coresight_trace_id_get_system_id();
>>>>>>> +       if (trace_id < 0) {
>>>>>>> +               ret = trace_id;
>>>>>>> +               goto cs_unregister;
>>>>>>> +       }
>>>>>>> +       drvdata->traceid = (u8)trace_id;
>>>>>>> +
>>
>> Number of issues here:-
>> 1) Why are sinks being given a trace ID? - they do not need them.
>> 2) how is the trace ID communicated to the underlying hardware system?
>> - there appears to be no way of doing this here. Without this you
>> cannot guarantee that there will not be clashes.
>> Although your use case may mitigate against this - for this to be a
>> generic module there must be a way to ensure the IDs can be discovered
>> externally.
>> 3) Trace IDs are a limited resource - most sources allocate on enable,
>> release on disable  / reset - this would be preferable.
>>
>>
>> Regards
>>
>> Mike

Good points Mike.

> 
> 1. It should not be given a trace ID for sink, I will correct it in the 
> next version of patch.
> 2. There are other patches to transmit the trace ID to sub-processor. 
> But We have an upstream dependency on QMI project. We will sync with 
> them for the other related patches.
> 3. The trace ID of dummy source need to be communicated to the 
> sub-processor, it's better to be allocated on probe, that would reduce 
> communications costs. On the other hand, there will be few dummy 
> sources. I'd perfer to allocate it on probe function.

Could that be delayed to dynamic allocation when the device is enabled ?
Also, do we need a property for the dummy-source to "allocate" a
traceID?

i.e., add a "property" (not compatible) 
"arm,coresight-dummy-source-traceid" ?

Suzuki


> 
> Thanks,
> Hao
> 
>>
>>>>>>> +       pm_runtime_enable(dev);
>>>>>>> +       dev_info(dev, "Dummy device initialized\n");
>>>>>>> +
>>>>>>> +       return 0;
>>>>>>> +
>>>>>>> +cs_unregister:
>>>>>>> +       coresight_unregister(drvdata->csdev);
>>>>>>> +
>>>>>>> +       return ret;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int dummy_remove(struct platform_device *pdev)
>>>>>>> +{
>>>>>>> +       struct dummy_drvdata *drvdata = platform_get_drvdata(pdev);
>>>>>>> +       struct device *dev = &pdev->dev;
>>>>>>> +
>>>>>>> +       coresight_trace_id_put_system_id(drvdata->traceid);
>>>>>>> +       pm_runtime_disable(dev);
>>>>>>> +       coresight_unregister(drvdata->csdev);
>>>>>>> +       return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static const struct of_device_id dummy_match[] = {
>>>>>>> +       {.compatible = "qcom,coresight-dummy"},
>>>>>>> +       {},
>>>>>>> +};
>>>>>>> +
>>>>>>> +static struct platform_driver dummy_driver = {
>>>>>>> +       .probe  = dummy_probe,
>>>>>>> +       .remove = dummy_remove,
>>>>>>> +       .driver = {
>>>>>>> +               .name   = "coresight-dummy",
>>>>>>> +               .of_match_table = dummy_match,
>>>>>>> +       },
>>>>>>> +};
>>>>>>> +
>>>>>>> +static int __init dummy_init(void)
>>>>>>> +{
>>>>>>> +       return platform_driver_register(&dummy_driver);
>>>>>>> +}
>>>>>>> +module_init(dummy_init);
>>>>>>> +
>>>>>>> +static void __exit dummy_exit(void)
>>>>>>> +{
>>>>>>> +       platform_driver_unregister(&dummy_driver);
>>>>>>> +}
>>>>>>> +module_exit(dummy_exit);
>>>>>>> +
>>>>>>> +MODULE_LICENSE("GPL");
>>>>>>> +MODULE_DESCRIPTION("CoreSight dummy source driver");
>>>>>>> -- 
>>>>>>> 2.17.1
>>>>>>>
>>>>>>
>>>>>>
>>>>
>>
>>
>>

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

[Mike Leach]

Hi,

On Tue, 28 Mar 2023 at 13:21, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> On 28/03/2023 12:25, Hao Zhang wrote:
> > Hi Mike,
> >
> > On 3/28/2023 6:06 PM, Mike Leach wrote:
> >> Hi,
> >>
> >> A few additional comments....
> >>
> >> On Tue, 28 Mar 2023 at 10:24, Hao Zhang <quic_hazha@quicinc.com> wrote:
> >>>
> >>> Hi Suzuki,
> >>>
> >>> On 3/28/2023 4:35 PM, Suzuki K Poulose wrote:
> >>>> On 28/03/2023 08:22, Hao Zhang wrote:
> >>>>> Hi Mike,
> >>>>>
> >>>>> On 3/27/2023 11:58 PM, Mike Leach wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> On Fri, 24 Mar 2023 at 06:16, Hao Zhang <quic_hazha@quicinc.com>
> >>>>>> wrote:
> >>>>>>>
> >>>>>>> Some Coresight devices that HLOS don't have permission to access
> >>>>>>> or configure. Such as Coresight sink EUD, some TPDMs etc. So there
> >>>>>>> need driver to register dummy devices as Coresight devices. 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@quicinc.com>
> >>>>>>> ---
> >>>>>>>    drivers/hwtracing/coresight/Kconfig           |  11 ++
> >>>>>>>    drivers/hwtracing/coresight/Makefile          |   1 +
> >>>>>>>    drivers/hwtracing/coresight/coresight-dummy.c | 176
> >>>>>>> ++++++++++++++++++
> >>>>>>>    3 files changed, 188 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..2d4eb3e546eb
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/drivers/hwtracing/coresight/coresight-dummy.c
> >>>>>>> @@ -0,0 +1,176 @@
> >>>>>>> +// 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>
> >>>>>>> +
> >>>>>>> +#include "coresight-priv.h"
> >>>>>>> +#include "coresight-trace-id.h"
> >>>>>>> +
> >>>>>>> +struct dummy_drvdata {
> >>>>>>> +       struct device                   *dev;
> >>>>>>> +       struct coresight_device         *csdev;
> >>>>>>> +       int                             traceid;
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> +DEFINE_CORESIGHT_DEVLIST(dummy_devs, "dummy");
> >>>>>>> +
> >>
> >> minor nit: can we have dummy_source and dummy_sink as the device names
> >> to make it clear at the first level what these are without having to
> >> look at the attributes?
> >>
> >
> > This is a good advice, dummy_source and dummy_sink are two different
> > components, so it's better to separate it at the first level. I will
> > take your advice in the next version of patch.
> >
> >>>>>>> +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_info(drvdata->dev, "Dummy source enabled\n");
> >>>>>>> +
> >>>>>>> +       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_info(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_info(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_info(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_sink dummy_sink_ops = {
> >>>>>>> +       .enable         = dummy_sink_enable,
> >>>>>>> +       .disable        = dummy_sink_disable,
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> +static const struct coresight_ops dummy_cs_ops = {
> >>>>>>> +       .source_ops     = &dummy_source_ops,
> >>>>>>> +       .sink_ops       = &dummy_sink_ops,
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> +static int dummy_probe(struct platform_device *pdev)
> >>>>>>> +{
> >>>>>>> +       int ret, trace_id;
> >>>>>>> +       struct device *dev = &pdev->dev;
> >>>>>>> +       struct coresight_platform_data *pdata;
> >>>>>>> +       struct dummy_drvdata *drvdata;
> >>>>>>> +       struct coresight_desc desc = { 0 };
> >>>>>>> +
> >>>>>>> +       desc.name = coresight_alloc_device_name(&dummy_devs, dev);
> >>>>>>> +       if (!desc.name)
> >>>>>>> +               return -ENOMEM;
> >>>>>>> +
> >>>>>>> +       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);
> >>>>>>> +
> >>>>>>> +       if (of_property_read_bool(pdev->dev.of_node,
> >>>>>>> "qcom,dummy-source")) {
> >>>>>>> +               desc.type = CORESIGHT_DEV_TYPE_SOURCE;
> >>>>>>> +               desc.subtype.source_subtype =
> >>>>>>> + CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS;
> >>>>>>> +       } else if (of_property_read_bool(pdev->dev.of_node,
> >>>>>>> +                                        "qcom,dummy-sink")) {
> >>
> >> It would simplify things if the compatibles were
> >> arm,coresight-dummy-source and arm,coresight-dummy-sink - and drop the
> >> two additional attributes, using of_device_is_compatible() here.
> >>
> >
> > Yes, I will update it in the next version of patch.
> >
> >>>>>>> +               desc.type = CORESIGHT_DEV_TYPE_SINK;
> >>>>>>> +               desc.subtype.sink_subtype =
> >>>>>>> CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
> >>>>>>
> >>>>>> This will break the automatic sink selection on a system where
> >>>>>> perf is
> >>>>>> looking for a default sink and the dummy sink is closest  / first
> >>>>>> discovered.
> >>>>>>
> >>>>>> i.e. when perf record -e cs_etm// <options>
> >>>>>> is used to trace a program in linux, a dummy sink appearing in the
> >>>>>> coresight tree with this designation may be selected.
> >>>>>>
> >>>>>> This needs to be corrected, probably with a unique sub-type that
> >>>>>> appears before the CORESIGHT_DEV_SUBTYPE_SINK_BUFFER value in the
> >>>>>> enum
> >>>>>> as the selection is based on >= CORESIGHT_DEV_SUBTYPE_SINK_BUFFER.
> >>>>>>
> >>>>
> >>>> Good point Mike.
> >>>>
> >>>>>> By implication adding a new value - will possibly affect other code
> >>>>>> using the enum values so will need to be checked
> >>>>>>
> >>>>>> Regards
> >>>>>>
> >>>>>> Mike
> >>>>>>
> >>>>>
> >>>>> Thanks for your comments, I will add a new sub-type for dummy sink and
> >>>>> check the impact of it.
> >>>>
> >>>> Please keep this as the lowest priority, something like:
> >>>>
> >>>>    enum coresight_dev_subtype_sink {
> >>>> +    CORESIGHT_DEV_SUBTYPE_SINK_DUMMY,
> >>>>           CORESIGHT_DEV_SUBTYPE_SINK_PORT,
> >>>>           CORESIGHT_DEV_SUBTYPE_SINK_BUFFER,
> >>>>           CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM,
> >>>>           CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM,
> >>>> };
> >>>>
> >>>> This should be fine without any impact on the existing code, as we
> >>>> expect the driver modules to be updated with the new core module.
> >>>>
> >>>> Suzuki
> >>>>
> >>>
> >>> Sure, I will take your advice in the next version of patch.
> >>>
> >>> Thanks,
> >>> Hao
> >>>
> >>>>
> >>>>>
> >>>>> Thanks,
> >>>>> Hao
> >>>>>
> >>>>>>
> >>>>>>> +       } else {
> >>>>>>> +               dev_info(dev, "Device type not set\n");
> >>>>>>> +               return -EINVAL;
> >>>>>>> +       }
> >>>>>>> +
> >>>>>>> +       desc.ops = &dummy_cs_ops;
> >>>>>>> +       desc.pdata = pdev->dev.platform_data;
> >>>>>>> +       desc.dev = &pdev->dev;
> >>>>>>> +       drvdata->csdev = coresight_register(&desc);
> >>>>>>> +       if (IS_ERR(drvdata->csdev))
> >>>>>>> +               return PTR_ERR(drvdata->csdev);
> >>>>>>> +
> >>>>>>> +       trace_id = coresight_trace_id_get_system_id();
> >>>>>>> +       if (trace_id < 0) {
> >>>>>>> +               ret = trace_id;
> >>>>>>> +               goto cs_unregister;
> >>>>>>> +       }
> >>>>>>> +       drvdata->traceid = (u8)trace_id;
> >>>>>>> +
> >>
> >> Number of issues here:-
> >> 1) Why are sinks being given a trace ID? - they do not need them.
> >> 2) how is the trace ID communicated to the underlying hardware system?
> >> - there appears to be no way of doing this here. Without this you
> >> cannot guarantee that there will not be clashes.
> >> Although your use case may mitigate against this - for this to be a
> >> generic module there must be a way to ensure the IDs can be discovered
> >> externally.
> >> 3) Trace IDs are a limited resource - most sources allocate on enable,
> >> release on disable  / reset - this would be preferable.
> >>
> >>
> >> Regards
> >>
> >> Mike
>
> Good points Mike.
>
> >
> > 1. It should not be given a trace ID for sink, I will correct it in the
> > next version of patch.
> > 2. There are other patches to transmit the trace ID to sub-processor.
> > But We have an upstream dependency on QMI project. We will sync with
> > them for the other related patches.

OK - but this set is not complete without the API for the external
system to access the allocated ID.
We need to review and agree this to ensure it fits the generic case.

> > 3. The trace ID of dummy source need to be communicated to the
> > sub-processor, it's better to be allocated on probe, that would reduce
> > communications costs. On the other hand, there will be few dummy
> > sources. I'd perfer to allocate it on probe function.
>
> Could that be delayed to dynamic allocation when the device is enabled ?

If  there are very few (low single digit) dummy devices, then it is
perhaps reasonable to allocate on probe - as per the STM

> Also, do we need a property for the dummy-source to "allocate" a
> traceID?
>

Any source must always allocate an ID, so we probably do not need this
just to enable the allocation. However, if we get into a situation
where there are many dummy devices then a property to force alloc on
enable could be used.
This could be addressed in future should the demand arise,

Regards

Mike

> i.e., add a "property" (not compatible)
> "arm,coresight-dummy-source-traceid" ?
>
> Suzuki
>
>
> >
> > Thanks,
> > Hao
> >
> >>
> >>>>>>> +       pm_runtime_enable(dev);
> >>>>>>> +       dev_info(dev, "Dummy device initialized\n");
> >>>>>>> +
> >>>>>>> +       return 0;
> >>>>>>> +
> >>>>>>> +cs_unregister:
> >>>>>>> +       coresight_unregister(drvdata->csdev);
> >>>>>>> +
> >>>>>>> +       return ret;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static int dummy_remove(struct platform_device *pdev)
> >>>>>>> +{
> >>>>>>> +       struct dummy_drvdata *drvdata = platform_get_drvdata(pdev);
> >>>>>>> +       struct device *dev = &pdev->dev;
> >>>>>>> +
> >>>>>>> +       coresight_trace_id_put_system_id(drvdata->traceid);
> >>>>>>> +       pm_runtime_disable(dev);
> >>>>>>> +       coresight_unregister(drvdata->csdev);
> >>>>>>> +       return 0;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static const struct of_device_id dummy_match[] = {
> >>>>>>> +       {.compatible = "qcom,coresight-dummy"},
> >>>>>>> +       {},
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> +static struct platform_driver dummy_driver = {
> >>>>>>> +       .probe  = dummy_probe,
> >>>>>>> +       .remove = dummy_remove,
> >>>>>>> +       .driver = {
> >>>>>>> +               .name   = "coresight-dummy",
> >>>>>>> +               .of_match_table = dummy_match,
> >>>>>>> +       },
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> +static int __init dummy_init(void)
> >>>>>>> +{
> >>>>>>> +       return platform_driver_register(&dummy_driver);
> >>>>>>> +}
> >>>>>>> +module_init(dummy_init);
> >>>>>>> +
> >>>>>>> +static void __exit dummy_exit(void)
> >>>>>>> +{
> >>>>>>> +       platform_driver_unregister(&dummy_driver);
> >>>>>>> +}
> >>>>>>> +module_exit(dummy_exit);
> >>>>>>> +
> >>>>>>> +MODULE_LICENSE("GPL");
> >>>>>>> +MODULE_DESCRIPTION("CoreSight dummy source driver");
> >>>>>>> --
> >>>>>>> 2.17.1
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>
> >>
> >>
> >>
>


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

[PATCH v2 2/3] dt-bindings: arm: Add Coresight Dummy Trace YAML schema

[Hao Zhang]

Add new coresight-dummy.yaml file describing the bindings required
to define coresight dummy trace in the device trees.

Signed-off-by: Hao Zhang <quic_hazha@quicinc.com>
---
 .../bindings/arm/qcom,coresight-dummy.yaml    | 118 ++++++++++++++++++
 1 file changed, 118 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml

diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
new file mode 100644
index 000000000000..7b719b084d72
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
@@ -0,0 +1,118 @@
+# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
+# Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/qcom,coresight-dummy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: QCOM Coresight Dummy component
+
+description: |
+  The Coresight Dummy component is for the specific devices that HLOS don't have
+  permission to access or configure. Such as Coresight sink EUD, some TPDMs etc.
+  So there need driver to register dummy devices as Coresight devices. 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.
+
+  The primary use case of the coresight dummy is to build path for dummy sink or
+  dummy source.
+
+maintainers:
+  - Mao Jinlong <quic_jinlmao@quicinc.com>
+  - Tao Zhang <quic_taozha@quicinc.com>
+  - Hao Zhang <quic_hazha@quicinc.com>
+
+select:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - qcom,coresight-dummy
+  required:
+    - compatible
+
+properties:
+  $nodename:
+    pattern: "^dummy_.*(sink|source)_[0-9]+.*$"
+  compatible:
+    items:
+      - const: qcom,coresight-dummy
+
+  qcom,dummy-sink:
+    type: boolean
+    description:
+      Indicates that the type of this coresight node is dummy sink.
+
+  qcom,dummy-source:
+    type: boolean
+    description:
+      Indicates that the type of this coresight node is dummy source.
+
+  out-ports:
+    description: |
+      Output connections from the dummy source to Coresight Trace bus.
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    properties:
+      port:
+        description: Output connection from the dummy source to Coresight
+            Trace bus.
+        $ref: /schemas/graph.yaml#/properties/port
+
+  in-ports:
+    description: |
+      Input connections from the CoreSight Trace bus to dummy sink.
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    properties:
+      port:
+        description: Input connection from the Coresight Trace bus to
+            dummy sink.
+        $ref: /schemas/graph.yaml#/properties/port
+
+required:
+  - compatible
+
+additionalProperties: false
+
+oneOf:
+  - required:
+      - qcom,dummy-sink
+  - required:
+      - qcom,dummy-source
+
+examples:
+  # minimum dummy sink definition. dummy sink connect to coresight replicator.
+  - |
+    dummy_sink_1 {
+      compatible = "qcom,coresight-dummy";
+      qcom,dummy-sink;
+
+      in-ports {
+        port {
+          eud_in_replicator_swao: endpoint {
+            remote-endpoint =
+              <&replicator_swao_out_eud>;
+          };
+        };
+      };
+    };
+
+  # minimum dummy source definition. dummy source connect to coresight funnel.
+  - |
+    dummy_source_1 {
+      compatible = "qcom,coresight-dummy";
+      qcom,dummy-source;
+
+      out-ports {
+        port {
+          dummy_riscv_out_funnel_swao: endpoint {
+            remote-endpoint =
+              <&funnel_swao_in_dummy_riscv>;
+          };
+        };
+      };
+    };
+
+...
-- 
2.17.1

Re: [PATCH v2 2/3] dt-bindings: arm: Add Coresight Dummy Trace YAML schema

[Suzuki K Poulose]

On 24/03/2023 06:16, Hao Zhang wrote:
> Add new coresight-dummy.yaml file describing the bindings required
> to define coresight dummy trace in the device trees.
> 
> Signed-off-by: Hao Zhang <quic_hazha@quicinc.com>
> ---
>   .../bindings/arm/qcom,coresight-dummy.yaml    | 118 ++++++++++++++++++
>   1 file changed, 118 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
> 
> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
> new file mode 100644
> index 000000000000..7b719b084d72
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
> @@ -0,0 +1,118 @@
> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> +# Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/qcom,coresight-dummy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: QCOM Coresight Dummy component

As mentioned in the previous email, please make this Arm CoreSight. This
is not specific to Qcom, rather something the CoreSight driver exposes
as a dummy framework. Otherwise looks good to me.

Suzuki

Re: [PATCH v2 2/3] dt-bindings: arm: Add Coresight Dummy Trace YAML schema

[Hao Zhang]

Hi Suzuki,

On 3/24/2023 6:47 PM, Suzuki K Poulose wrote:
> On 24/03/2023 06:16, Hao Zhang wrote:
>> Add new coresight-dummy.yaml file describing the bindings required
>> to define coresight dummy trace in the device trees.
>>
>> Signed-off-by: Hao Zhang <quic_hazha@quicinc.com>
>> ---
>>   .../bindings/arm/qcom,coresight-dummy.yaml    | 118 ++++++++++++++++++
>>   1 file changed, 118 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml 
>> b/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
>> new file mode 100644
>> index 000000000000..7b719b084d72
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
>> @@ -0,0 +1,118 @@
>> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
>> +# Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights 
>> reserved.
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/arm/qcom,coresight-dummy.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: QCOM Coresight Dummy component
> 
> As mentioned in the previous email, please make this Arm CoreSight. This
> is not specific to Qcom, rather something the CoreSight driver exposes
> as a dummy framework. Otherwise looks good to me.
> 
> Suzuki
> 

Thanks for your comment, I will take your advice and update it in the 
next version of patch.

Thanks,
Hao

Re: [PATCH v2 2/3] dt-bindings: arm: Add Coresight Dummy Trace YAML schema

[Krzysztof Kozlowski]

On 24/03/2023 07:16, Hao Zhang wrote:
> Add new coresight-dummy.yaml file describing the bindings required
> to define coresight dummy trace in the device trees.
> 

Subject: drop second/last, redundant "YAML schema". The "dt-bindings"
prefix is already stating that these are bindings and all new must be DT
schema. You cannot add anything else, so this is redundant.


> Signed-off-by: Hao Zhang <quic_hazha@quicinc.com>
> ---
>  .../bindings/arm/qcom,coresight-dummy.yaml    | 118 ++++++++++++++++++
>  1 file changed, 118 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
> 
> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
> new file mode 100644
> index 000000000000..7b719b084d72
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
> @@ -0,0 +1,118 @@
> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> +# Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/qcom,coresight-dummy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: QCOM Coresight Dummy component
> +
> +description: |
> +  The Coresight Dummy component is for the specific devices that HLOS don't have
> +  permission to access or configure. Such as Coresight sink EUD, some TPDMs etc.
> +  So there need driver to register dummy devices as Coresight devices. 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.
> +
> +  The primary use case of the coresight dummy is to build path for dummy sink or
> +  dummy source.
> +
> +maintainers:
> +  - Mao Jinlong <quic_jinlmao@quicinc.com>
> +  - Tao Zhang <quic_taozha@quicinc.com>
> +  - Hao Zhang <quic_hazha@quicinc.com>
> +
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +          - qcom,coresight-dummy
> +  required:
> +    - compatible

Why do you need the select?

> +
> +properties:
> +  $nodename:
> +    pattern: "^dummy_.*(sink|source)_[0-9]+.*$"

We do not enforce node names in individual bindings. Why do you need it?
Plus underscore is not even proper character...

> +  compatible:
> +    items:

Drop items. You have only one item, so no need for list.

> +      - const: qcom,coresight-dummy
> +
> +  qcom,dummy-sink:
> +    type: boolean
> +    description:
> +      Indicates that the type of this coresight node is dummy sink.

You just duplicated property name. Write something useful.

> +
> +  qcom,dummy-source:
> +    type: boolean
> +    description:
> +      Indicates that the type of this coresight node is dummy source.

You just duplicated property name. Write something useful.

> +
> +  out-ports:
> +    description: |

No need for |

> +      Output connections from the dummy source to Coresight Trace bus.
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port:
> +        description: Output connection from the dummy source to Coresight
> +            Trace bus.
> +        $ref: /schemas/graph.yaml#/properties/port
> +
> +  in-ports:
> +    description: |

Ditto

> +      Input connections from the CoreSight Trace bus to dummy sink.
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port:
> +        description: Input connection from the Coresight Trace bus to
> +            dummy sink.
> +        $ref: /schemas/graph.yaml#/properties/port
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +oneOf:
> +  - required:
> +      - qcom,dummy-sink
> +  - required:
> +      - qcom,dummy-source
> +
> +examples:
> +  # minimum dummy sink definition. dummy sink connect to coresight replicator.
> +  - |
> +    dummy_sink_1 {

Node names should be generic, so "sink"
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +      compatible = "qcom,coresight-dummy";
> +      qcom,dummy-sink;
> +
> +      in-ports {
> +        port {
> +          eud_in_replicator_swao: endpoint {
> +            remote-endpoint =
> +              <&replicator_swao_out_eud>;

Why line break after =?

> +          };
> +        };
> +      };
> +    };
> +
> +  # minimum dummy source definition. dummy source connect to coresight funnel.

If you use sentences, then start with capital letter.

> +  - |
> +    dummy_source_1 {
> +      compatible = "qcom,coresight-dummy";
> +      qcom,dummy-source;
> +
> +      out-ports {
> +        port {
> +          dummy_riscv_out_funnel_swao: endpoint {
> +            remote-endpoint =
> +              <&funnel_swao_in_dummy_riscv>;

Why line break?
> +          };
> +        };
> +      };
> +    };
> +
> +...

Best regards,
Krzysztof

Re: [PATCH v2 2/3] dt-bindings: arm: Add Coresight Dummy Trace YAML schema

[Hao Zhang]

Hi Krzysztof,

On 3/25/2023 7:49 PM, Krzysztof Kozlowski wrote:
> On 24/03/2023 07:16, Hao Zhang wrote:
>> Add new coresight-dummy.yaml file describing the bindings required
>> to define coresight dummy trace in the device trees.
>>
> 
> Subject: drop second/last, redundant "YAML schema". The "dt-bindings"
> prefix is already stating that these are bindings and all new must be DT
> schema. You cannot add anything else, so this is redundant.
> 
I will take your advice to drop redundant part of title in the next 
version of patch.
> 
>> Signed-off-by: Hao Zhang <quic_hazha@quicinc.com>
>> ---
>>   .../bindings/arm/qcom,coresight-dummy.yaml    | 118 ++++++++++++++++++
>>   1 file changed, 118 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
>> new file mode 100644
>> index 000000000000..7b719b084d72
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
>> @@ -0,0 +1,118 @@
>> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
>> +# Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/arm/qcom,coresight-dummy.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: QCOM Coresight Dummy component
>> +
>> +description: |
>> +  The Coresight Dummy component is for the specific devices that HLOS don't have
>> +  permission to access or configure. Such as Coresight sink EUD, some TPDMs etc.
>> +  So there need driver to register dummy devices as Coresight devices. 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.
>> +
>> +  The primary use case of the coresight dummy is to build path for dummy sink or
>> +  dummy source.
>> +
>> +maintainers:
>> +  - Mao Jinlong <quic_jinlmao@quicinc.com>
>> +  - Tao Zhang <quic_taozha@quicinc.com>
>> +  - Hao Zhang <quic_hazha@quicinc.com>
>> +
>> +select:
>> +  properties:
>> +    compatible:
>> +      contains:
>> +        enum:
>> +          - qcom,coresight-dummy
>> +  required:
>> +    - compatible
> 
> Why do you need the select?
> 
This is a mistake, will remove it in the next version of patch.
>> +
>> +properties:
>> +  $nodename:
>> +    pattern: "^dummy_.*(sink|source)_[0-9]+.*$"
> 
> We do not enforce node names in individual bindings. Why do you need it?
> Plus underscore is not even proper character...
> 
I will remove this node.

>> +  compatible:
>> +    items:
> 
> Drop items. You have only one item, so no need for list.

I will take your advice and update it in the next version of patch.

>> +      - const: qcom,coresight-dummy
>> +
>> +  qcom,dummy-sink:
>> +    type: boolean
>> +    description:
>> +      Indicates that the type of this coresight node is dummy sink.
> 
> You just duplicated property name. Write something useful.
> 
>> +
>> +  qcom,dummy-source:
>> +    type: boolean
>> +    description:
>> +      Indicates that the type of this coresight node is dummy source.
> 
> You just duplicated property name. Write something useful.
> 

Sure, I will add more details for it.

>> +
>> +  out-ports:
>> +    description: |
> 
> No need for |
> 
>> +      Output connections from the dummy source to Coresight Trace bus.
>> +    $ref: /schemas/graph.yaml#/properties/ports
>> +
>> +    properties:
>> +      port:
>> +        description: Output connection from the dummy source to Coresight
>> +            Trace bus.
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +
>> +  in-ports:
>> +    description: |
> 
> Ditto
> 
I will remove it in the next version of patch.

>> +      Input connections from the CoreSight Trace bus to dummy sink.
>> +    $ref: /schemas/graph.yaml#/properties/ports
>> +
>> +    properties:
>> +      port:
>> +        description: Input connection from the Coresight Trace bus to
>> +            dummy sink.
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +
>> +required:
>> +  - compatible
>> +
>> +additionalProperties: false
>> +
>> +oneOf:
>> +  - required:
>> +      - qcom,dummy-sink
>> +  - required:
>> +      - qcom,dummy-source
>> +
>> +examples:
>> +  # minimum dummy sink definition. dummy sink connect to coresight replicator.
>> +  - |
>> +    dummy_sink_1 {
> 
> Node names should be generic, so "sink"
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 
>> +      compatible = "qcom,coresight-dummy";
>> +      qcom,dummy-sink;
>> +
>> +      in-ports {
>> +        port {
>> +          eud_in_replicator_swao: endpoint {
>> +            remote-endpoint =
>> +              <&replicator_swao_out_eud>;
> 
> Why line break after =?
> 

>> +          };
>> +        };
>> +      };
>> +    };
>> +
>> +  # minimum dummy source definition. dummy source connect to coresight funnel.
> 
> If you use sentences, then start with capital letter.
>

I will update it according to your advice in the next version of patch.

>> +  - |
>> +    dummy_source_1 {
>> +      compatible = "qcom,coresight-dummy";
>> +      qcom,dummy-source;
>> +
>> +      out-ports {
>> +        port {
>> +          dummy_riscv_out_funnel_swao: endpoint {
>> +            remote-endpoint =
>> +              <&funnel_swao_in_dummy_riscv>;
> 
> Why line break?

I copy it from device tree and keep the original format, will correct 
the format in the next version of patch.

Thanks,
Hao

>> +          };
>> +        };
>> +      };
>> +    };
>> +
>> +...
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 2/3] dt-bindings: arm: Add Coresight Dummy Trace YAML schema

[Mike Leach]

Hi,

As per my comments in the previous patch in this set....

On Mon, 27 Mar 2023 at 08:38, Hao Zhang <quic_hazha@quicinc.com> wrote:
>
> Hi Krzysztof,
>
> On 3/25/2023 7:49 PM, Krzysztof Kozlowski wrote:
> > On 24/03/2023 07:16, Hao Zhang wrote:
> >> Add new coresight-dummy.yaml file describing the bindings required
> >> to define coresight dummy trace in the device trees.
> >>
> >
> > Subject: drop second/last, redundant "YAML schema". The "dt-bindings"
> > prefix is already stating that these are bindings and all new must be DT
> > schema. You cannot add anything else, so this is redundant.
> >
> I will take your advice to drop redundant part of title in the next
> version of patch.
> >
> >> Signed-off-by: Hao Zhang <quic_hazha@quicinc.com>
> >> ---
> >>   .../bindings/arm/qcom,coresight-dummy.yaml    | 118 ++++++++++++++++++
> >>   1 file changed, 118 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
> >> new file mode 100644
> >> index 000000000000..7b719b084d72
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
> >> @@ -0,0 +1,118 @@
> >> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> >> +# Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/arm/qcom,coresight-dummy.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: QCOM Coresight Dummy component
> >> +
> >> +description: |
> >> +  The Coresight Dummy component is for the specific devices that HLOS don't have
> >> +  permission to access or configure. Such as Coresight sink EUD, some TPDMs etc.
> >> +  So there need driver to register dummy devices as Coresight devices. 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.
> >> +
> >> +  The primary use case of the coresight dummy is to build path for dummy sink or
> >> +  dummy source.
> >> +
> >> +maintainers:
> >> +  - Mao Jinlong <quic_jinlmao@quicinc.com>
> >> +  - Tao Zhang <quic_taozha@quicinc.com>
> >> +  - Hao Zhang <quic_hazha@quicinc.com>
> >> +
> >> +select:
> >> +  properties:
> >> +    compatible:
> >> +      contains:
> >> +        enum:
> >> +          - qcom,coresight-dummy

Can we have coresight-dummy-source and coresight-dummy-sink?

> >> +  required:
> >> +    - compatible
> >
> > Why do you need the select?
> >
> This is a mistake, will remove it in the next version of patch.
> >> +
> >> +properties:
> >> +  $nodename:
> >> +    pattern: "^dummy_.*(sink|source)_[0-9]+.*$"
> >
> > We do not enforce node names in individual bindings. Why do you need it?
> > Plus underscore is not even proper character...
> >
> I will remove this node.
>
> >> +  compatible:
> >> +    items:
> >
> > Drop items. You have only one item, so no need for list.
>
> I will take your advice and update it in the next version of patch.
>
> >> +      - const: qcom,coresight-dummy
> >> +
> >> +  qcom,dummy-sink:
> >> +    type: boolean
> >> +    description:
> >> +      Indicates that the type of this coresight node is dummy sink.
> >
> > You just duplicated property name. Write something useful.
> >
> >> +
> >> +  qcom,dummy-source:
> >> +    type: boolean
> >> +    description:
> >> +      Indicates that the type of this coresight node is dummy source.
> >
> > You just duplicated property name. Write something useful.
> >
>

These properties not required if the compatible name is more specific

> Sure, I will add more details for it.
>
> >> +
> >> +  out-ports:
> >> +    description: |
> >
> > No need for |
> >
> >> +      Output connections from the dummy source to Coresight Trace bus.
> >> +    $ref: /schemas/graph.yaml#/properties/ports
> >> +
> >> +    properties:
> >> +      port:
> >> +        description: Output connection from the dummy source to Coresight
> >> +            Trace bus.
> >> +        $ref: /schemas/graph.yaml#/properties/port
> >> +
> >> +  in-ports:
> >> +    description: |
> >
> > Ditto
> >
> I will remove it in the next version of patch.
>
> >> +      Input connections from the CoreSight Trace bus to dummy sink.
> >> +    $ref: /schemas/graph.yaml#/properties/ports
> >> +
> >> +    properties:
> >> +      port:
> >> +        description: Input connection from the Coresight Trace bus to
> >> +            dummy sink.
> >> +        $ref: /schemas/graph.yaml#/properties/port
> >> +
> >> +required:
> >> +  - compatible
> >> +

The binding should constrain out ports to dummy-source only, and in
ports to dummy sink only.

Regards

Mike

> >> +additionalProperties: false
> >> +
> >> +oneOf:
> >> +  - required:
> >> +      - qcom,dummy-sink
> >> +  - required:
> >> +      - qcom,dummy-source
> >> +
> >> +examples:
> >> +  # minimum dummy sink definition. dummy sink connect to coresight replicator.
> >> +  - |
> >> +    dummy_sink_1 {
> >
> > Node names should be generic, so "sink"
> > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> >
> >> +      compatible = "qcom,coresight-dummy";
> >> +      qcom,dummy-sink;
> >> +
> >> +      in-ports {
> >> +        port {
> >> +          eud_in_replicator_swao: endpoint {
> >> +            remote-endpoint =
> >> +              <&replicator_swao_out_eud>;
> >
> > Why line break after =?
> >
>
> >> +          };
> >> +        };
> >> +      };
> >> +    };
> >> +
> >> +  # minimum dummy source definition. dummy source connect to coresight funnel.
> >
> > If you use sentences, then start with capital letter.
> >
>
> I will update it according to your advice in the next version of patch.
>
> >> +  - |
> >> +    dummy_source_1 {
> >> +      compatible = "qcom,coresight-dummy";
> >> +      qcom,dummy-source;
> >> +
> >> +      out-ports {
> >> +        port {
> >> +          dummy_riscv_out_funnel_swao: endpoint {
> >> +            remote-endpoint =
> >> +              <&funnel_swao_in_dummy_riscv>;
> >
> > Why line break?
>
> I copy it from device tree and keep the original format, will correct
> the format in the next version of patch.
>
> Thanks,
> Hao
>
> >> +          };
> >> +        };
> >> +      };
> >> +    };
> >> +
> >> +...
> >
> > Best regards,
> > Krzysztof
> >



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

Re: [PATCH v2 2/3] dt-bindings: arm: Add Coresight Dummy Trace YAML schema

[Hao Zhang]

Hi Mike,

On 3/28/2023 6:12 PM, Mike Leach wrote:
> Hi,
> 
> As per my comments in the previous patch in this set....
> 
> On Mon, 27 Mar 2023 at 08:38, Hao Zhang <quic_hazha@quicinc.com> wrote:
>>
>> Hi Krzysztof,
>>
>> On 3/25/2023 7:49 PM, Krzysztof Kozlowski wrote:
>>> On 24/03/2023 07:16, Hao Zhang wrote:
>>>> Add new coresight-dummy.yaml file describing the bindings required
>>>> to define coresight dummy trace in the device trees.
>>>>
>>>
>>> Subject: drop second/last, redundant "YAML schema". The "dt-bindings"
>>> prefix is already stating that these are bindings and all new must be DT
>>> schema. You cannot add anything else, so this is redundant.
>>>
>> I will take your advice to drop redundant part of title in the next
>> version of patch.
>>>
>>>> Signed-off-by: Hao Zhang <quic_hazha@quicinc.com>
>>>> ---
>>>>    .../bindings/arm/qcom,coresight-dummy.yaml    | 118 ++++++++++++++++++
>>>>    1 file changed, 118 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
>>>> new file mode 100644
>>>> index 000000000000..7b719b084d72
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
>>>> @@ -0,0 +1,118 @@
>>>> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
>>>> +# Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/arm/qcom,coresight-dummy.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: QCOM Coresight Dummy component
>>>> +
>>>> +description: |
>>>> +  The Coresight Dummy component is for the specific devices that HLOS don't have
>>>> +  permission to access or configure. Such as Coresight sink EUD, some TPDMs etc.
>>>> +  So there need driver to register dummy devices as Coresight devices. 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.
>>>> +
>>>> +  The primary use case of the coresight dummy is to build path for dummy sink or
>>>> +  dummy source.
>>>> +
>>>> +maintainers:
>>>> +  - Mao Jinlong <quic_jinlmao@quicinc.com>
>>>> +  - Tao Zhang <quic_taozha@quicinc.com>
>>>> +  - Hao Zhang <quic_hazha@quicinc.com>
>>>> +
>>>> +select:
>>>> +  properties:
>>>> +    compatible:
>>>> +      contains:
>>>> +        enum:
>>>> +          - qcom,coresight-dummy
> 
> Can we have coresight-dummy-source and coresight-dummy-sink?

Sure, I will take your advice in the next version of patch.

> 
>>>> +  required:
>>>> +    - compatible
>>>
>>> Why do you need the select?
>>>
>> This is a mistake, will remove it in the next version of patch.
>>>> +
>>>> +properties:
>>>> +  $nodename:
>>>> +    pattern: "^dummy_.*(sink|source)_[0-9]+.*$"
>>>
>>> We do not enforce node names in individual bindings. Why do you need it?
>>> Plus underscore is not even proper character...
>>>
>> I will remove this node.
>>
>>>> +  compatible:
>>>> +    items:
>>>
>>> Drop items. You have only one item, so no need for list.
>>
>> I will take your advice and update it in the next version of patch.
>>
>>>> +      - const: qcom,coresight-dummy
>>>> +
>>>> +  qcom,dummy-sink:
>>>> +    type: boolean
>>>> +    description:
>>>> +      Indicates that the type of this coresight node is dummy sink.
>>>
>>> You just duplicated property name. Write something useful.
>>>
>>>> +
>>>> +  qcom,dummy-source:
>>>> +    type: boolean
>>>> +    description:
>>>> +      Indicates that the type of this coresight node is dummy source.
>>>
>>> You just duplicated property name. Write something useful.
>>>
>>
> 
> These properties not required if the compatible name is more specific
> 

I will update it in the next version of patch.

>> Sure, I will add more details for it.
>>
>>>> +
>>>> +  out-ports:
>>>> +    description: |
>>>
>>> No need for |
>>>
>>>> +      Output connections from the dummy source to Coresight Trace bus.
>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>> +
>>>> +    properties:
>>>> +      port:
>>>> +        description: Output connection from the dummy source to Coresight
>>>> +            Trace bus.
>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>> +
>>>> +  in-ports:
>>>> +    description: |
>>>
>>> Ditto
>>>
>> I will remove it in the next version of patch.
>>
>>>> +      Input connections from the CoreSight Trace bus to dummy sink.
>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>> +
>>>> +    properties:
>>>> +      port:
>>>> +        description: Input connection from the Coresight Trace bus to
>>>> +            dummy sink.
>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +
> 
> The binding should constrain out ports to dummy-source only, and in
> ports to dummy sink only.
> 
> Regards
> 
> Mike
> 

I will update it according to your advice in the next version of patch.

Thanks,
Hao

>>>> +additionalProperties: false
>>>> +
>>>> +oneOf:
>>>> +  - required:
>>>> +      - qcom,dummy-sink
>>>> +  - required:
>>>> +      - qcom,dummy-source
>>>> +
>>>> +examples:
>>>> +  # minimum dummy sink definition. dummy sink connect to coresight replicator.
>>>> +  - |
>>>> +    dummy_sink_1 {
>>>
>>> Node names should be generic, so "sink"
>>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>>>
>>>> +      compatible = "qcom,coresight-dummy";
>>>> +      qcom,dummy-sink;
>>>> +
>>>> +      in-ports {
>>>> +        port {
>>>> +          eud_in_replicator_swao: endpoint {
>>>> +            remote-endpoint =
>>>> +              <&replicator_swao_out_eud>;
>>>
>>> Why line break after =?
>>>
>>
>>>> +          };
>>>> +        };
>>>> +      };
>>>> +    };
>>>> +
>>>> +  # minimum dummy source definition. dummy source connect to coresight funnel.
>>>
>>> If you use sentences, then start with capital letter.
>>>
>>
>> I will update it according to your advice in the next version of patch.
>>
>>>> +  - |
>>>> +    dummy_source_1 {
>>>> +      compatible = "qcom,coresight-dummy";
>>>> +      qcom,dummy-source;
>>>> +
>>>> +      out-ports {
>>>> +        port {
>>>> +          dummy_riscv_out_funnel_swao: endpoint {
>>>> +            remote-endpoint =
>>>> +              <&funnel_swao_in_dummy_riscv>;
>>>
>>> Why line break?
>>
>> I copy it from device tree and keep the original format, will correct
>> the format in the next version of patch.
>>
>> Thanks,
>> Hao
>>
>>>> +          };
>>>> +        };
>>>> +      };
>>>> +    };
>>>> +
>>>> +...
>>>
>>> Best regards,
>>> Krzysztof
>>>
> 
> 
> 

Re: [PATCH v2 2/3] dt-bindings: arm: Add Coresight Dummy Trace YAML schema

[Rob Herring]

On Fri, Mar 24, 2023 at 02:16:07PM +0800, Hao Zhang wrote:
> Add new coresight-dummy.yaml file describing the bindings required
> to define coresight dummy trace in the device trees.

The diff tells me all this. Please explain why this is needed and needs 
to be in DT here.

> 
> Signed-off-by: Hao Zhang <quic_hazha@quicinc.com>
> ---
>  .../bindings/arm/qcom,coresight-dummy.yaml    | 118 ++++++++++++++++++
>  1 file changed, 118 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
> 
> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
> new file mode 100644
> index 000000000000..7b719b084d72
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
> @@ -0,0 +1,118 @@
> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> +# Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/qcom,coresight-dummy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: QCOM Coresight Dummy component
> +
> +description: |
> +  The Coresight Dummy component is for the specific devices that HLOS don't have
> +  permission to access or configure. Such as Coresight sink EUD, some TPDMs etc.

EUD? TPDM?

I don't really love 'dummy' used here. Maybe the OS still wants/needs to 
know where the sink goes to even if not configurable.

You *can* have multiple compatibles for a single generic driver if those 
compatibles might be useful some day.

> +  So there need driver to register dummy devices as Coresight devices. 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.
> +
> +  The primary use case of the coresight dummy is to build path for dummy sink or
> +  dummy source.
> +
> +maintainers:
> +  - Mao Jinlong <quic_jinlmao@quicinc.com>
> +  - Tao Zhang <quic_taozha@quicinc.com>
> +  - Hao Zhang <quic_hazha@quicinc.com>
> +
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +          - qcom,coresight-dummy
> +  required:
> +    - compatible
> +
> +properties:
> +  $nodename:
> +    pattern: "^dummy_.*(sink|source)_[0-9]+.*$"

Don't use '_' in node names.

Convention for multiple instances without 'reg' is '-[0-9]+' on the end, 
but you are allowing anything after that.

> +  compatible:
> +    items:
> +      - const: qcom,coresight-dummy
> +
> +  qcom,dummy-sink:
> +    type: boolean
> +    description:
> +      Indicates that the type of this coresight node is dummy sink.
> +
> +  qcom,dummy-source:

Incorporate source or sink into the compatible strings.

It's also somewhat redundant with 'in-ports' vs. 'out-ports'.

> +    type: boolean
> +    description:
> +      Indicates that the type of this coresight node is dummy source.
> +
> +  out-ports:
> +    description: |

Don't need '|' unless you need to preserve formatting.

> +      Output connections from the dummy source to Coresight Trace bus.
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port:
> +        description: Output connection from the dummy source to Coresight
> +            Trace bus.
> +        $ref: /schemas/graph.yaml#/properties/port
> +
> +  in-ports:
> +    description: |
> +      Input connections from the CoreSight Trace bus to dummy sink.
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port:
> +        description: Input connection from the Coresight Trace bus to
> +            dummy sink.
> +        $ref: /schemas/graph.yaml#/properties/port
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +oneOf:
> +  - required:
> +      - qcom,dummy-sink
> +  - required:
> +      - qcom,dummy-source
> +
> +examples:
> +  # minimum dummy sink definition. dummy sink connect to coresight replicator.
> +  - |
> +    dummy_sink_1 {
> +      compatible = "qcom,coresight-dummy";
> +      qcom,dummy-sink;
> +
> +      in-ports {
> +        port {
> +          eud_in_replicator_swao: endpoint {
> +            remote-endpoint =
> +              <&replicator_swao_out_eud>;
> +          };
> +        };
> +      };
> +    };
> +
> +  # minimum dummy source definition. dummy source connect to coresight funnel.
> +  - |
> +    dummy_source_1 {
> +      compatible = "qcom,coresight-dummy";
> +      qcom,dummy-source;
> +
> +      out-ports {
> +        port {
> +          dummy_riscv_out_funnel_swao: endpoint {
> +            remote-endpoint =
> +              <&funnel_swao_in_dummy_riscv>;
> +          };
> +        };
> +      };
> +    };
> +
> +...
> -- 
> 2.17.1
> 

Re: [PATCH v2 2/3] dt-bindings: arm: Add Coresight Dummy Trace YAML schema

[Hao Zhang]

Hi Rob,

On 4/1/2023 2:47 AM, Rob Herring wrote:
> On Fri, Mar 24, 2023 at 02:16:07PM +0800, Hao Zhang wrote:
>> Add new coresight-dummy.yaml file describing the bindings required
>> to define coresight dummy trace in the device trees.
> 
> The diff tells me all this. Please explain why this is needed and needs
> to be in DT here.
> 
Sure, will add more details to describe it.
>>
>> Signed-off-by: Hao Zhang <quic_hazha@quicinc.com>
>> ---
>>   .../bindings/arm/qcom,coresight-dummy.yaml    | 118 ++++++++++++++++++
>>   1 file changed, 118 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
>> new file mode 100644
>> index 000000000000..7b719b084d72
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-dummy.yaml
>> @@ -0,0 +1,118 @@
>> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
>> +# Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/arm/qcom,coresight-dummy.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: QCOM Coresight Dummy component
>> +
>> +description: |
>> +  The Coresight Dummy component is for the specific devices that HLOS don't have
>> +  permission to access or configure. Such as Coresight sink EUD, some TPDMs etc.
> 
> EUD? TPDM?

The term EUD stand for Embedded USB debugger, it will be connected to 
coresight replicator component to receive and store coresight data. It 
would be configured by NON-HLOS, and need HLOS(Kernel) to configure the 
last coresight components. So we will use dummy sink to replace it in 
kernel side for building the whole path(from source to sink).

The TPDM is Trace Profiling and Diagnostics Monitor, it is a coresight 
trace source which could get hardware events from the IP subsystem.

> 
> I don't really love 'dummy' used here. Maybe the OS still wants/needs to
> know where the sink goes to even if not configurable.
> 
> You *can* have multiple compatibles for a single generic driver if those
> compatibles might be useful some day.
> 

Yes, we want to take it as a generic framework for coresight dummy sink 
and source. I think we could add one more compatible to indicate the 
type of it.

>> +  So there need driver to register dummy devices as Coresight devices. 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.
>> +
>> +  The primary use case of the coresight dummy is to build path for dummy sink or
>> +  dummy source.
>> +
>> +maintainers:
>> +  - Mao Jinlong <quic_jinlmao@quicinc.com>
>> +  - Tao Zhang <quic_taozha@quicinc.com>
>> +  - Hao Zhang <quic_hazha@quicinc.com>
>> +
>> +select:
>> +  properties:
>> +    compatible:
>> +      contains:
>> +        enum:
>> +          - qcom,coresight-dummy
>> +  required:
>> +    - compatible
>> +
>> +properties:
>> +  $nodename:
>> +    pattern: "^dummy_.*(sink|source)_[0-9]+.*$"
> 
> Don't use '_' in node names.
> 
> Convention for multiple instances without 'reg' is '-[0-9]+' on the end,
> but you are allowing anything after that.
> 

OK, I will update it in the next version of patch.

>> +  compatible:
>> +    items:
>> +      - const: qcom,coresight-dummy
>> +
>> +  qcom,dummy-sink:
>> +    type: boolean
>> +    description:
>> +      Indicates that the type of this coresight node is dummy sink.
>> +
>> +  qcom,dummy-source:
> 
> Incorporate source or sink into the compatible strings.
> 

OK, I will update it in the next version of patch.

> It's also somewhat redundant with 'in-ports' vs. 'out-ports'.

I think I could add more details to describe it.

> 
>> +    type: boolean
>> +    description:
>> +      Indicates that the type of this coresight node is dummy source.
>> +
>> +  out-ports:
>> +    description: |
> 
> Don't need '|' unless you need to preserve formatting.

I will remove it.

Thanks for your comments, I will take your advice and update it in the 
next version of patch.

Thanks,
Hao

> 
>> +      Output connections from the dummy source to Coresight Trace bus.
>> +    $ref: /schemas/graph.yaml#/properties/ports
>> +
>> +    properties:
>> +      port:
>> +        description: Output connection from the dummy source to Coresight
>> +            Trace bus.
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +
>> +  in-ports:
>> +    description: |
>> +      Input connections from the CoreSight Trace bus to dummy sink.
>> +    $ref: /schemas/graph.yaml#/properties/ports
>> +
>> +    properties:
>> +      port:
>> +        description: Input connection from the Coresight Trace bus to
>> +            dummy sink.
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +
>> +required:
>> +  - compatible
>> +
>> +additionalProperties: false
>> +
>> +oneOf:
>> +  - required:
>> +      - qcom,dummy-sink
>> +  - required:
>> +      - qcom,dummy-source
>> +
>> +examples:
>> +  # minimum dummy sink definition. dummy sink connect to coresight replicator.
>> +  - |
>> +    dummy_sink_1 {
>> +      compatible = "qcom,coresight-dummy";
>> +      qcom,dummy-sink;
>> +
>> +      in-ports {
>> +        port {
>> +          eud_in_replicator_swao: endpoint {
>> +            remote-endpoint =
>> +              <&replicator_swao_out_eud>;
>> +          };
>> +        };
>> +      };
>> +    };
>> +
>> +  # minimum dummy source definition. dummy source connect to coresight funnel.
>> +  - |
>> +    dummy_source_1 {
>> +      compatible = "qcom,coresight-dummy";
>> +      qcom,dummy-source;
>> +
>> +      out-ports {
>> +        port {
>> +          dummy_riscv_out_funnel_swao: endpoint {
>> +            remote-endpoint =
>> +              <&funnel_swao_in_dummy_riscv>;
>> +          };
>> +        };
>> +      };
>> +    };
>> +
>> +...
>> -- 
>> 2.17.1
>>

[PATCH v2 3/3] Documentation: trace: Add documentation for Coresight Dummy Trace

[Hao Zhang]

Add documentation for Coresight Dummy Trace under trace/coresight.

Signed-off-by: Hao Zhang <quic_hazha@quicinc.com>
---
 .../trace/coresight/coresight-dummy.rst       | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 Documentation/trace/coresight/coresight-dummy.rst

diff --git a/Documentation/trace/coresight/coresight-dummy.rst b/Documentation/trace/coresight/coresight-dummy.rst
new file mode 100644
index 000000000000..819cabab8623
--- /dev/null
+++ b/Documentation/trace/coresight/coresight-dummy.rst
@@ -0,0 +1,58 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=============================
+Coresight Dummy Trace Module
+=============================
+
+    :Author:   Hao Zhang <quic_hazha@quicinc.com>
+    :Date:     March 2023
+
+Introduction
+---------------------------
+
+Coresight Dummy Trace Module is for the specific devices that HLOS don't
+have permission to access or configure. Such as Coresight sink EUD, some
+TPDMs etc. So there need driver to register dummy devices as Coresight
+devices. 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.
+
+Sysfs files and directories
+---------------------------
+
+Root: ``/sys/bus/coresight/devices/dummy<N>``
+
+----
+
+:File:            ``enable_source`` (RW)
+:Notes:
+    - > 0 : enable the datasets of dummy source.
+
+    - = 0 : disable the datasets of dummy source.
+
+:Syntax:
+    ``echo 1 > enable_source``
+
+----
+
+:File:            ``enable_sink`` (RW)
+:Notes:
+    - > 0 : enable the datasets of dummy sink.
+
+    - = 0 : disable the datasets of dummy sink.
+
+:Syntax:
+    ``echo 1 > enable_sink``
+
+----
+
+Config details
+---------------------------
+
+There are two types of nodes, dummy sink and dummy source. The nodes
+should be observed at the coresight path
+"/sys/bus/coresight/devices".
+e.g.
+/sys/bus/coresight/devices # ls -l | grep dummy
+dummy0 -> ../../../devices/platform/soc@0/soc@0:dummy_source/dummy0
+dummy1 -> ../../../devices/platform/soc@0/soc@0:dummy_sink/dummy1
-- 
2.17.1

Re: [PATCH v2 3/3] Documentation: trace: Add documentation for Coresight Dummy Trace

[Suzuki K Poulose]

On 24/03/2023 06:16, Hao Zhang wrote:
> Add documentation for Coresight Dummy Trace under trace/coresight.
> 
> Signed-off-by: Hao Zhang <quic_hazha@quicinc.com>
> ---
>   .../trace/coresight/coresight-dummy.rst       | 58 +++++++++++++++++++
>   1 file changed, 58 insertions(+)
>   create mode 100644 Documentation/trace/coresight/coresight-dummy.rst
> 
> diff --git a/Documentation/trace/coresight/coresight-dummy.rst b/Documentation/trace/coresight/coresight-dummy.rst
> new file mode 100644
> index 000000000000..819cabab8623
> --- /dev/null
> +++ b/Documentation/trace/coresight/coresight-dummy.rst
> @@ -0,0 +1,58 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=============================
> +Coresight Dummy Trace Module
> +=============================
> +
> +    :Author:   Hao Zhang <quic_hazha@quicinc.com>
> +    :Date:     March 2023
> +
> +Introduction
> +---------------------------
> +
> +Coresight Dummy Trace Module is for the specific devices that HLOS don't

Please do not use cryptic abbreviations, please use "kernel"


> +have permission to access or configure. 

Such as Coresight sink EUD, some
> +TPDMs etc. 
Say "e.g., CoreSight TPDMs on Qualcomm platforms.:

So there need driver to register dummy devices as Coresight
> +devices.

Add:

"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.


I think the following content may not be needed as they are part
of the standard source/sink type devices, nothing specific to
dummy devices.

--- vvvvv ---
> +
> +Sysfs files and directories
> +---------------------------
> +
> +Root: ``/sys/bus/coresight/devices/dummy<N>``
> +
> +----
> +
> +:File:            ``enable_source`` (RW)
> +:Notes:
> +    - > 0 : enable the datasets of dummy source.
> +
> +    - = 0 : disable the datasets of dummy source.
> +
> +:Syntax:
> +    ``echo 1 > enable_source``
> +
> +----
> +
> +:File:            ``enable_sink`` (RW)
> +:Notes:
> +    - > 0 : enable the datasets of dummy sink.
> +
> +    - = 0 : disable the datasets of dummy sink.
> +
> +:Syntax:
> +    ``echo 1 > enable_sink``
> +
> +----
> +

--- You may remove the above ^^^ ----

> +Config details
> +---------------------------
> +
> +There are two types of nodes, dummy sink and dummy source. The nodes
> +should be observed at the coresight path
> +"/sys/bus/coresight/devices".
> +e.g.
> +/sys/bus/coresight/devices # ls -l | grep dummy
> +dummy0 -> ../../../devices/platform/soc@0/soc@0:dummy_source/dummy0
> +dummy1 -> ../../../devices/platform/soc@0/soc@0:dummy_sink/dummy1

Suzuki

Re: [PATCH v2 3/3] Documentation: trace: Add documentation for Coresight Dummy Trace

[Hao Zhang]

Hi Suzuki,

On 3/24/2023 7:00 PM, Suzuki K Poulose wrote:
> On 24/03/2023 06:16, Hao Zhang wrote:
>> Add documentation for Coresight Dummy Trace under trace/coresight.
>>
>> Signed-off-by: Hao Zhang <quic_hazha@quicinc.com>
>> ---
>>   .../trace/coresight/coresight-dummy.rst       | 58 +++++++++++++++++++
>>   1 file changed, 58 insertions(+)
>>   create mode 100644 Documentation/trace/coresight/coresight-dummy.rst
>>
>> diff --git a/Documentation/trace/coresight/coresight-dummy.rst 
>> b/Documentation/trace/coresight/coresight-dummy.rst
>> new file mode 100644
>> index 000000000000..819cabab8623
>> --- /dev/null
>> +++ b/Documentation/trace/coresight/coresight-dummy.rst
>> @@ -0,0 +1,58 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +=============================
>> +Coresight Dummy Trace Module
>> +=============================
>> +
>> +    :Author:   Hao Zhang <quic_hazha@quicinc.com>
>> +    :Date:     March 2023
>> +
>> +Introduction
>> +---------------------------
>> +
>> +Coresight Dummy Trace Module is for the specific devices that HLOS don't
> 
> Please do not use cryptic abbreviations, please use "kernel"
> 
Sure, I will change it to "kernel".
> 
>> +have permission to access or configure. 
> 
> Such as Coresight sink EUD, some
>> +TPDMs etc. 
> Say "e.g., CoreSight TPDMs on Qualcomm platforms.:
> 
> So there need driver to register dummy devices as Coresight
>> +devices.
> 
> Add:
> 
> "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.
> 
I will take your advice in the next version of patch.
> 
> I think the following content may not be needed as they are part
> of the standard source/sink type devices, nothing specific to
> dummy devices.
> 
> --- vvvvv ---
>> +
>> +Sysfs files and directories
>> +---------------------------
>> +
>> +Root: ``/sys/bus/coresight/devices/dummy<N>``
>> +
>> +----
>> +
>> +:File:            ``enable_source`` (RW)
>> +:Notes:
>> +    - > 0 : enable the datasets of dummy source.
>> +
>> +    - = 0 : disable the datasets of dummy source.
>> +
>> +:Syntax:
>> +    ``echo 1 > enable_source``
>> +
>> +----
>> +
>> +:File:            ``enable_sink`` (RW)
>> +:Notes:
>> +    - > 0 : enable the datasets of dummy sink.
>> +
>> +    - = 0 : disable the datasets of dummy sink.
>> +
>> +:Syntax:
>> +    ``echo 1 > enable_sink``
>> +
>> +----
>> +
> 
> --- You may remove the above ^^^ ----
>
I will remove the above notes in the next version of patch.

Thanks,
Hao

>> +Config details
>> +---------------------------
>> +
>> +There are two types of nodes, dummy sink and dummy source. The nodes
>> +should be observed at the coresight path
>> +"/sys/bus/coresight/devices".
>> +e.g.
>> +/sys/bus/coresight/devices # ls -l | grep dummy
>> +dummy0 -> ../../../devices/platform/soc@0/soc@0:dummy_source/dummy0
>> +dummy1 -> ../../../devices/platform/soc@0/soc@0:dummy_sink/dummy1
> 
> Suzuki
> 

Re: [PATCH v2 3/3] Documentation: trace: Add documentation for Coresight Dummy Trace

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 1431 bytes --]

On Fri, Mar 24, 2023 at 02:16:08PM +0800, Hao Zhang wrote:
> +Sysfs files and directories
> +---------------------------
> +
> +Root: ``/sys/bus/coresight/devices/dummy<N>``
> +
> +----
> +
> +:File:            ``enable_source`` (RW)
> +:Notes:
> +    - > 0 : enable the datasets of dummy source.
> +
> +    - = 0 : disable the datasets of dummy source.
> +
> +:Syntax:
> +    ``echo 1 > enable_source``
> +
> +----
> +
> +:File:            ``enable_sink`` (RW)
> +:Notes:
> +    - > 0 : enable the datasets of dummy sink.
> +
> +    - = 0 : disable the datasets of dummy sink.
> +
> +:Syntax:
> +    ``echo 1 > enable_sink``

Is enable_{source,sink} integer-valued or bit (0/1)? In other words, is
it OK to `echo 2` to both sysfs files?

> +
> +----
> +
> +Config details
> +---------------------------
> +
> +There are two types of nodes, dummy sink and dummy source. The nodes
> +should be observed at the coresight path
> +"/sys/bus/coresight/devices".

For consistency, inline this sysfs also (thus
``/sys/bus/coresight/devices``.

> +e.g.
e.g.:: (make the shell snippet below code block)
> +/sys/bus/coresight/devices # ls -l | grep dummy
IMO I prefer `PS1=\$` (that is, omit the directory). Alternatively,
you can write `ls -l /sys/bus/coresight/devices | grep dummy` (specify
the directory to `ls`).

Thanks.

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 3/3] Documentation: trace: Add documentation for Coresight Dummy Trace

[Hao Zhang]

Hi Bagas,

On 3/27/2023 5:06 PM, Bagas Sanjaya wrote:
> On Fri, Mar 24, 2023 at 02:16:08PM +0800, Hao Zhang wrote:
>> +Sysfs files and directories
>> +---------------------------
>> +
>> +Root: ``/sys/bus/coresight/devices/dummy<N>``
>> +
>> +----
>> +
>> +:File:            ``enable_source`` (RW)
>> +:Notes:
>> +    - > 0 : enable the datasets of dummy source.
>> +
>> +    - = 0 : disable the datasets of dummy source.
>> +
>> +:Syntax:
>> +    ``echo 1 > enable_source``
>> +
>> +----
>> +
>> +:File:            ``enable_sink`` (RW)
>> +:Notes:
>> +    - > 0 : enable the datasets of dummy sink.
>> +
>> +    - = 0 : disable the datasets of dummy sink.
>> +
>> +:Syntax:
>> +    ``echo 1 > enable_sink``
> 
> Is enable_{source,sink} integer-valued or bit (0/1)? In other words, is
> it OK to `echo 2` to both sysfs files?

It should be the integer value, that is OK to "echo 2" to the sysfs files.

>> +
>> +----
>> +
>> +Config details
>> +---------------------------
>> +
>> +There are two types of nodes, dummy sink and dummy source. The nodes
>> +should be observed at the coresight path
>> +"/sys/bus/coresight/devices".
> 
> For consistency, inline this sysfs also (thus
> ``/sys/bus/coresight/devices``.
> 
>> +e.g.
> e.g.:: (make the shell snippet below code block)

I will correct it in the next version of patch.

>> +/sys/bus/coresight/devices # ls -l | grep dummy
> IMO I prefer `PS1=\$` (that is, omit the directory). Alternatively,
> you can write `ls -l /sys/bus/coresight/devices | grep dummy` (specify
> the directory to `ls`).
> 
> Thanks.
> 

Thanks for your comments, I will take your advice in the next version of 
patch.

Thanks,
Hao

Re: [PATCH v2 3/3] Documentation: trace: Add documentation for Coresight Dummy Trace

[Greg Kroah-Hartman]

On Fri, Mar 24, 2023 at 02:16:08PM +0800, Hao Zhang wrote:
> Add documentation for Coresight Dummy Trace under trace/coresight.
> 
> Signed-off-by: Hao Zhang <quic_hazha@quicinc.com>
> ---
>  .../trace/coresight/coresight-dummy.rst       | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
>  create mode 100644 Documentation/trace/coresight/coresight-dummy.rst
> 
> diff --git a/Documentation/trace/coresight/coresight-dummy.rst b/Documentation/trace/coresight/coresight-dummy.rst
> new file mode 100644
> index 000000000000..819cabab8623
> --- /dev/null
> +++ b/Documentation/trace/coresight/coresight-dummy.rst
> @@ -0,0 +1,58 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=============================
> +Coresight Dummy Trace Module
> +=============================
> +
> +    :Author:   Hao Zhang <quic_hazha@quicinc.com>
> +    :Date:     March 2023
> +
> +Introduction
> +---------------------------
> +
> +Coresight Dummy Trace Module is for the specific devices that HLOS don't
> +have permission to access or configure. Such as Coresight sink EUD, some
> +TPDMs etc. So there need driver to register dummy devices as Coresight
> +devices. 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.
> +
> +Sysfs files and directories
> +---------------------------
> +
> +Root: ``/sys/bus/coresight/devices/dummy<N>``

sysfs files are documented in Documentation/ABI/ not in random .rst
files, sorry.  Please use the correct format described there, not a
random one like this :)

thanks,

greg k-h

Re: [PATCH v2 3/3] Documentation: trace: Add documentation for Coresight Dummy Trace

[Hao Zhang]

Hi Greg,

On 3/27/2023 5:13 PM, Greg Kroah-Hartman wrote:
> On Fri, Mar 24, 2023 at 02:16:08PM +0800, Hao Zhang wrote:
>> Add documentation for Coresight Dummy Trace under trace/coresight.
>>
>> Signed-off-by: Hao Zhang <quic_hazha@quicinc.com>
>> ---
>>   .../trace/coresight/coresight-dummy.rst       | 58 +++++++++++++++++++
>>   1 file changed, 58 insertions(+)
>>   create mode 100644 Documentation/trace/coresight/coresight-dummy.rst
>>
>> diff --git a/Documentation/trace/coresight/coresight-dummy.rst b/Documentation/trace/coresight/coresight-dummy.rst
>> new file mode 100644
>> index 000000000000..819cabab8623
>> --- /dev/null
>> +++ b/Documentation/trace/coresight/coresight-dummy.rst
>> @@ -0,0 +1,58 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +=============================
>> +Coresight Dummy Trace Module
>> +=============================
>> +
>> +    :Author:   Hao Zhang <quic_hazha@quicinc.com>
>> +    :Date:     March 2023
>> +
>> +Introduction
>> +---------------------------
>> +
>> +Coresight Dummy Trace Module is for the specific devices that HLOS don't
>> +have permission to access or configure. Such as Coresight sink EUD, some
>> +TPDMs etc. So there need driver to register dummy devices as Coresight
>> +devices. 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.
>> +
>> +Sysfs files and directories
>> +---------------------------
>> +
>> +Root: ``/sys/bus/coresight/devices/dummy<N>``
> 
> sysfs files are documented in Documentation/ABI/ not in random .rst
> files, sorry.  Please use the correct format described there, not a
> random one like this :)
> 
> thanks,
> 
> greg k-h

Thanks for your review, I will correct the format in the next version of 
patch.

Thanks,
Hao