| Message ID | 20230324061608.33609-2-quic_hazha@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support to configure Coresight Dummy subunit | expand |
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"); >
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 >
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 >> > >
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 >>> >> >>
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 >>>> >>> >>> >
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 > >>>> > >>> > >>> > >
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 >>>>>> >>>>> >>>>> >>> > > >
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 >>>>>>> >>>>>> >>>>>> >>>> >> >> >>
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 > >>>>>>> > >>>>>> > >>>>>> > >>>> > >> > >> > >> >
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");
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