[PATCH v2 0/5] coresight: Add Coresight Trace NOC driver

[PATCH v2 0/5] coresight: Add Coresight Trace NOC driver

[Yuanfang Zhang]

The Trace NoC is an integration hierarchy which is a replacement of
Dragonlink configuration. It brings together debug component like TPDA,
funnel and interconnect Trace Noc which collects trace from subsystems
and transfers to QDSS sink.

Compared to DL, it has the following advantages:
1. Reduce wires between subsystems.
2. Continue cleaning the infrastructure.
3. Reduce Data overhead by transporting raw data from source to target.

    +--------------+                                         +-------------+     
    | SDCC5 TPDM   |                                         |  SDCC5 TPDM |     
    +--------------+                                         +-------------+     
           |                                                        |            
           |                                                        |            
+----------|-------------------+                                    |            
|          v                   |                                    |            
|  +----v----+     Dragon Link |                                    v            
|  |DLNT TPDA|     North       |                         +----------------------+
|  +---------+                 |                         |    TRACE NOC AG      |
|       |                      |                         |                      |
|       v-------------+        |                         +----------------------+
|                     |        |                                   |             
|              +------v-----+  |                                   |             
|              | DLNT Funnel|  |                                   |             
|              +------------+  |                                   |             
|                   |          |                                   |             
+-------------------|----------+                                   |             
              <-----+                                              |             
             |                                                     |             
             |                                                     |             
             v                                                     v             
    +----------------+                                      +---------------+    
    |     QDSS       |                                      |    QDSS       |    
    +----------------+                                      +---------------+
    

Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com>
---
---
Changes in v2:
1. Modified the format of DT binging file.
2. Fix compile warnings.
- Link to v1: https://lore.kernel.org/r/20250221-trace-noc-driver-v1-0-0a23fc643217@quicinc.com

---
Yuanfang Zhang (5):
      dt-bindings: arm: Add Coresight device Trace NOC definition
      coresight: add coresight Trace NOC driver
      coresight-tnoc: add nodes to configure flush
      coresight-tnoc: add node to configure flag type
      coresight-tnoc: add nodes to configure freq packet

 .../bindings/arm/qcom,coresight-tnoc.yaml          | 116 ++++++
 drivers/hwtracing/coresight/Kconfig                |  13 +
 drivers/hwtracing/coresight/Makefile               |   1 +
 drivers/hwtracing/coresight/coresight-tnoc.c       | 400 +++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tnoc.h       |  57 +++
 5 files changed, 587 insertions(+)
---
base-commit: 92514ef226f511f2ca1fb1b8752966097518edc0
change-id: 20250212-trace-noc-driver-9e75d78114fa

Best regards,
-- 
Yuanfang Zhang <quic_yuanfang@quicinc.com>

[PATCH v2 1/5] dt-bindings: arm: Add Coresight device Trace NOC definition

[Yuanfang Zhang]

Adds new coresight-tnoc.yaml file describing the bindings required
to define Trace NOC in the device trees.

Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com>
---
 .../bindings/arm/qcom,coresight-tnoc.yaml          | 116 +++++++++++++++++++++
 1 file changed, 116 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-tnoc.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-tnoc.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..2d806cc34c381d27b47dcce126ce5bcf468826a8
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tnoc.yaml
@@ -0,0 +1,116 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/qcom,coresight-tnoc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Ttrace NOC(Network On Chip)
+
+maintainers:
+  - yuanfang Zhang <quic_yuanfang@quicinc.com>
+
+description:
+  The Trace NoC is an integration hierarchy which is a replacement of Dragonlink
+  tile configuration. It brings together debug component like TPDA, funnel and
+  interconnect Trace Noc which collects trace from subsystems and transfers to
+  QDSS sink.
+
+  It sits in the different subsystem of SOC and aggregates the trace and
+  transports it to Aggregation TNoC or to QDSS trace sink eventually. Trace NoC
+  embeds bridges for all the interfaces(APB, ATB, QPMDA & NTS).
+
+  Trace NoC can take inputs from different trace sources i.e. ATB, QPMDA.
+
+# Need a custom select here or 'arm,primecell' will match on lots of nodes
+select:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - qcom,coresight-tnoc
+  required:
+    - compatible
+
+properties:
+  $nodename:
+    pattern: "^tn(@[0-9a-f]+)$"
+
+  compatible:
+    items:
+      - const: qcom,coresight-tnoc
+      - const: arm,primecell
+
+  reg:
+    minItems: 1
+    maxItems: 2
+    description:
+      Physical address space of the device.
+
+  clocks:
+    maxItems: 1
+    description:
+      Clock sources used by the device.
+
+  clock-names:
+    items:
+      - const: apb_pclk
+
+  in-ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    patternProperties:
+      '^port(@[0-9a-f]+)?$':
+        description: Input connections from CoreSight Trace bus
+        $ref: /schemas/graph.yaml#/properties/port
+
+  out-ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    additionalProperties: false
+
+    properties:
+      port:
+        description:
+          Output connection to CoreSight Trace bus
+        $ref: /schemas/graph.yaml#/properties/port
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - in-ports
+  - out-ports
+
+additionalProperties: false
+
+examples:
+  - |
+    tn@109ab000  {
+      compatible = "qcom,coresight-tnoc", "arm,primecell";
+      reg = <0x0 0x109ab000  0x0 0x4200>;
+
+      clocks = <&aoss_qmp>;
+      clock-names = "apb_pclk";
+
+      in-ports {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        port@0 {
+          reg = <0>;
+
+          tn_ag_in_tpdm_gcc: endpoint {
+            remote-endpoint = <&tpdm_gcc_out_tn_ag>;
+          };
+        };
+      };
+
+      out-ports {
+        port {
+          tn_ag_out_funnel_in1: endpoint {
+            remote-endpoint = <&funnel_in1_in_tn_ag>;
+          };
+        };
+      };
+    };
+...

-- 
2.34.1

Re: [PATCH v2 1/5] dt-bindings: arm: Add Coresight device Trace NOC definition

[Krzysztof Kozlowski]

On 26/02/2025 12:05, Yuanfang Zhang wrote:
> +
> +  compatible:
> +    items:
> +      - const: qcom,coresight-tnoc
> +      - const: arm,primecell
> +
> +  reg:
> +    minItems: 1
> +    maxItems: 2
> +    description:
> +      Physical address space of the device.
Not much improved - still items are not listed. Which binding did you
choose as an example as I asked to? (so I can fix it)

Best regards,
Krzysztof

Re: [PATCH v2 1/5] dt-bindings: arm: Add Coresight device Trace NOC definition

[Yuanfang Zhang]

On 2/26/2025 7:09 PM, Krzysztof Kozlowski wrote:
> On 26/02/2025 12:05, Yuanfang Zhang wrote:
>> +
>> +  compatible:
>> +    items:
>> +      - const: qcom,coresight-tnoc
>> +      - const: arm,primecell
>> +
>> +  reg:
>> +    minItems: 1
>> +    maxItems: 2
>> +    description:
>> +      Physical address space of the device.
> Not much improved - still items are not listed. Which binding did you
> choose as an example as I asked to? (so I can fix it)
> 
qcom,coresight-tpda.yaml

> Best regards,
> Krzysztof

Re: [PATCH v2 1/5] dt-bindings: arm: Add Coresight device Trace NOC definition

[Krzysztof Kozlowski]

On 26/02/2025 12:16, Yuanfang Zhang wrote:
> 
> 
> On 2/26/2025 7:09 PM, Krzysztof Kozlowski wrote:
>> On 26/02/2025 12:05, Yuanfang Zhang wrote:
>>> +
>>> +  compatible:
>>> +    items:
>>> +      - const: qcom,coresight-tnoc
>>> +      - const: arm,primecell
>>> +
>>> +  reg:
>>> +    minItems: 1
>>> +    maxItems: 2
>>> +    description:
>>> +      Physical address space of the device.
>> Not much improved - still items are not listed. Which binding did you
>> choose as an example as I asked to? (so I can fix it)
>>
> qcom,coresight-tpda.yaml
But there is no description there. About the items, I will fix it, thanks.

Best regards,
Krzysztof

[PATCH v2 2/5] coresight: add coresight Trace NOC driver

[Yuanfang Zhang]

Add driver to support Coresight device Trace NOC(Network On Chip).
Trace NOC is an integration hierarchy which is a replacement of
Dragonlink configuration. It brings together debug components like
TPDA, funnel and interconnect Trace Noc.

It sits in the different subsystem of SOC and aggregates the trace
and transports to QDSS trace bus.

Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com>
---
 drivers/hwtracing/coresight/Kconfig          |  13 ++
 drivers/hwtracing/coresight/Makefile         |   1 +
 drivers/hwtracing/coresight/coresight-tnoc.c | 190 +++++++++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tnoc.h |  53 ++++++++
 4 files changed, 257 insertions(+)

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index 06f0a7594169c5f03ca5f893b7debd294587de78..6cfd160f09d383ab5f5aa276fa57496a52c8f961 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -247,4 +247,17 @@ config CORESIGHT_DUMMY
 
 	  To compile this driver as a module, choose M here: the module will be
 	  called coresight-dummy.
+
+config CORESIGHT_TNOC
+	tristate "Coresight Trace Noc driver"
+	help
+	  This driver provides support for Trace NoC component.
+	  Trace NoC is a interconnect that is used to collect trace from
+	  various subsystems and transport it QDSS trace sink.It sits in
+	  the different tiles of SOC and aggregates the trace local to the
+	  tile and transports it another tile or to QDSS trace sink eventually.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called coresight-tnoc.
+
 endif
diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index 4ba478211b318ea5305f9f98dda40a041759f09f..60b729979f19c8f8848c77c290605132dba1a991 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_CORESIGHT_SINK_TPIU) += coresight-tpiu.o
 obj-$(CONFIG_CORESIGHT_SINK_ETBV10) += coresight-etb10.o
 obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o \
 					   coresight-replicator.o
+obj-$(CONFIG_CORESIGHT_TNOC) += coresight-tnoc.o
 obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x.o
 coresight-etm3x-y := coresight-etm3x-core.o coresight-etm-cp14.o \
 		     coresight-etm3x-sysfs.o
diff --git a/drivers/hwtracing/coresight/coresight-tnoc.c b/drivers/hwtracing/coresight/coresight-tnoc.c
new file mode 100644
index 0000000000000000000000000000000000000000..fad8e61f05ef25989aba1be342c547f835e8953a
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-tnoc.c
@@ -0,0 +1,190 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/amba/bus.h>
+#include <linux/io.h>
+#include <linux/coresight.h>
+#include <linux/of.h>
+
+#include "coresight-priv.h"
+#include "coresight-tnoc.h"
+#include "coresight-trace-id.h"
+
+static void trace_noc_enable_hw(struct trace_noc_drvdata *drvdata)
+{
+	u32 val;
+
+	/* Set ATID */
+	writel_relaxed(drvdata->atid, drvdata->base + TRACE_NOC_XLD);
+
+	/* Config sync CR */
+	writel_relaxed(0xffff, drvdata->base + TRACE_NOC_SYNCR);
+
+	/* Set frequency value */
+	writel_relaxed(drvdata->freq_req_val, drvdata->base + TRACE_NOC_FREQVAL);
+
+	/* Set Ctrl register */
+	val = readl_relaxed(drvdata->base + TRACE_NOC_CTRL);
+
+	if (drvdata->flag_type == FLAG_TS)
+		val = val | TRACE_NOC_CTRL_FLAGTYPE;
+	else
+		val = val & ~TRACE_NOC_CTRL_FLAGTYPE;
+
+	if (drvdata->freq_type == FREQ_TS)
+		val = val | TRACE_NOC_CTRL_FREQTYPE;
+	else
+		val = val & ~TRACE_NOC_CTRL_FREQTYPE;
+
+	val = val | TRACE_NOC_CTRL_PORTEN;
+	writel_relaxed(val, drvdata->base + TRACE_NOC_CTRL);
+
+	dev_dbg(drvdata->dev, "Trace NOC is enabled\n");
+}
+
+static int trace_noc_enable(struct coresight_device *csdev, struct coresight_connection *inport,
+			    struct coresight_connection *outport)
+{
+	struct trace_noc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	spin_lock(&drvdata->spinlock);
+	if (csdev->refcnt == 0)
+		trace_noc_enable_hw(drvdata);
+
+	csdev->refcnt++;
+	spin_unlock(&drvdata->spinlock);
+
+	return 0;
+}
+
+static void trace_noc_disable_hw(struct trace_noc_drvdata *drvdata)
+{
+	writel_relaxed(0x0, drvdata->base + TRACE_NOC_CTRL);
+	dev_dbg(drvdata->dev, "Trace NOC is disabled\n");
+}
+
+static void trace_noc_disable(struct coresight_device *csdev, struct coresight_connection *inport,
+			      struct coresight_connection *outport)
+{
+	struct trace_noc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	spin_lock(&drvdata->spinlock);
+	if (--csdev->refcnt == 0)
+		trace_noc_disable_hw(drvdata);
+
+	spin_unlock(&drvdata->spinlock);
+	dev_info(drvdata->dev, "Trace NOC is disabled\n");
+}
+
+static const struct coresight_ops_link trace_noc_link_ops = {
+	.enable		= trace_noc_enable,
+	.disable	= trace_noc_disable,
+};
+
+static const struct coresight_ops trace_noc_cs_ops = {
+	.link_ops	= &trace_noc_link_ops,
+};
+
+static int trace_noc_init_default_data(struct trace_noc_drvdata *drvdata)
+{
+	int atid;
+
+	atid = coresight_trace_id_get_system_id();
+	if (atid < 0)
+		return atid;
+
+	drvdata->atid = atid;
+
+	drvdata->freq_type = FREQ_TS;
+	drvdata->flag_type = FLAG;
+	drvdata->freq_req_val = 0;
+
+	return 0;
+}
+
+static int trace_noc_probe(struct amba_device *adev, const struct amba_id *id)
+{
+	struct device *dev = &adev->dev;
+	struct coresight_platform_data *pdata;
+	struct trace_noc_drvdata *drvdata;
+	struct coresight_desc desc = { 0 };
+	int ret;
+
+	desc.name = coresight_alloc_device_name(&trace_noc_devs, dev);
+	if (!desc.name)
+		return -ENOMEM;
+	pdata = coresight_get_platform_data(dev);
+	if (IS_ERR(pdata))
+		return PTR_ERR(pdata);
+	adev->dev.platform_data = pdata;
+
+	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+
+	drvdata->dev = &adev->dev;
+	dev_set_drvdata(dev, drvdata);
+
+	drvdata->base = devm_ioremap_resource(dev, &adev->res);
+	if (!drvdata->base)
+		return -ENOMEM;
+
+	spin_lock_init(&drvdata->spinlock);
+
+	ret = trace_noc_init_default_data(drvdata);
+	if (ret)
+		return ret;
+
+	desc.ops = &trace_noc_cs_ops;
+	desc.type = CORESIGHT_DEV_TYPE_LINK;
+	desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_MERG;
+	desc.pdata = adev->dev.platform_data;
+	desc.dev = &adev->dev;
+	desc.access = CSDEV_ACCESS_IOMEM(drvdata->base);
+	drvdata->csdev = coresight_register(&desc);
+	if (IS_ERR(drvdata->csdev))
+		return PTR_ERR(drvdata->csdev);
+
+	pm_runtime_put(&adev->dev);
+
+	return 0;
+}
+
+static void trace_noc_remove(struct amba_device *adev)
+{
+	struct trace_noc_drvdata *drvdata = dev_get_drvdata(&adev->dev);
+
+	coresight_trace_id_put_system_id(drvdata->atid);
+	coresight_unregister(drvdata->csdev);
+}
+
+static struct amba_id trace_noc_ids[] = {
+	{
+		.id     = 0x000f0c00,
+		.mask   = 0x000fff00,
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(amba, trace_noc_ids);
+
+static struct amba_driver trace_noc_driver = {
+	.drv = {
+		.name   = "coresight-trace-noc",
+		.owner	= THIS_MODULE,
+		.suppress_bind_attrs = true,
+	},
+	.probe          = trace_noc_probe,
+	.remove		= trace_noc_remove,
+	.id_table	= trace_noc_ids,
+};
+
+module_amba_driver(trace_noc_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Trace NOC driver");
diff --git a/drivers/hwtracing/coresight/coresight-tnoc.h b/drivers/hwtracing/coresight/coresight-tnoc.h
new file mode 100644
index 0000000000000000000000000000000000000000..b6bd1ef659897d8e0994c5e8514e8cbdd16eebd8
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-tnoc.h
@@ -0,0 +1,53 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#define TRACE_NOC_CTRL	0x008
+#define TRACE_NOC_XLD	0x010
+#define TRACE_NOC_FREQVAL	0x018
+#define TRACE_NOC_SYNCR	0x020
+
+/* Enable generation of output ATB traffic.*/
+#define TRACE_NOC_CTRL_PORTEN	BIT(0)
+/* Writing 1 to issue a FREQ or FREQ_TS packet*/
+#define TRACE_NOC_CTRL_FREQTSREQ	BIT(5)
+/* Sets the type of issued ATB FLAG packets. 0: 'FLAG' packets; 1: 'FLAG_TS' packets.*/
+#define TRACE_NOC_CTRL_FLAGTYPE		BIT(7)
+/* sets the type of issued ATB FREQ packets. 0: 'FREQ' packets; 1: 'FREQ_TS' packets.*/
+#define TRACE_NOC_CTRL_FREQTYPE		BIT(8)
+DEFINE_CORESIGHT_DEVLIST(trace_noc_devs, "traceNoc");
+
+/**
+ * struct trace_noc_drvdata - specifics associated to a trace noc component
+ * @base:	memory mapped base address for this component.
+ * @dev:	device node for trace_noc_drvdata.
+ * @csdev:	component vitals needed by the framework.
+ * @spinlock:	only one at a time pls.
+ * @atid:	id for the trace packet.
+ * @freqtype:	0: 'FREQ' packets; 1: 'FREQ_TS' packets.
+ * @flagtype:	0: 'FLAG' packets; 1: 'FLAG_TS' packets.
+ * @freq_req_val:	 set frequency values carried by 'FREQ' and 'FREQ_TS' packets.
+ */
+struct trace_noc_drvdata {
+	void __iomem		*base;
+	struct device		*dev;
+	struct coresight_device	*csdev;
+	spinlock_t		spinlock; /* lock for the drvdata. */
+	u32			atid;
+	u32			freq_type;
+	u32			flag_type;
+	u32			freq_req_val;
+};
+
+/* freq type */
+enum freq_type {
+	FREQ,
+	FREQ_TS,
+};
+
+/* flag type */
+enum flag_type {
+	FLAG,
+	FLAG_TS,
+};

-- 
2.34.1

Re: [PATCH v2 2/5] coresight: add coresight Trace NOC driver

[Leo Yan]

Hi Yuanfang,

On Wed, Feb 26, 2025 at 07:05:51PM +0800, Yuanfang Zhang wrote:
> 
> Add driver to support Coresight device Trace NOC(Network On Chip).
> Trace NOC is an integration hierarchy which is a replacement of
> Dragonlink configuration. It brings together debug components like
> TPDA, funnel and interconnect Trace Noc.
> 
> It sits in the different subsystem of SOC and aggregates the trace
> and transports to QDSS trace bus.
> 
> Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com>
> ---
>  drivers/hwtracing/coresight/Kconfig          |  13 ++
>  drivers/hwtracing/coresight/Makefile         |   1 +
>  drivers/hwtracing/coresight/coresight-tnoc.c | 190 +++++++++++++++++++++++++++
>  drivers/hwtracing/coresight/coresight-tnoc.h |  53 ++++++++
>  4 files changed, 257 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index 06f0a7594169c5f03ca5f893b7debd294587de78..6cfd160f09d383ab5f5aa276fa57496a52c8f961 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -247,4 +247,17 @@ config CORESIGHT_DUMMY
> 
>           To compile this driver as a module, choose M here: the module will be
>           called coresight-dummy.
> +
> +config CORESIGHT_TNOC
> +       tristate "Coresight Trace Noc driver"
> +       help
> +         This driver provides support for Trace NoC component.
> +         Trace NoC is a interconnect that is used to collect trace from
> +         various subsystems and transport it QDSS trace sink.It sits in

Trace NoC is an interconnect used to collect traces from various
subsystems and transport to a QDSS trace sink.

> +         the different tiles of SOC and aggregates the trace local to the
> +         tile and transports it another tile or to QDSS trace sink eventually.
> +
> +         To compile this driver as a module, choose M here: the module will be
> +         called coresight-tnoc.
> +
>  endif
> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> index 4ba478211b318ea5305f9f98dda40a041759f09f..60b729979f19c8f8848c77c290605132dba1a991 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -34,6 +34,7 @@ obj-$(CONFIG_CORESIGHT_SINK_TPIU) += coresight-tpiu.o
>  obj-$(CONFIG_CORESIGHT_SINK_ETBV10) += coresight-etb10.o
>  obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o \
>                                            coresight-replicator.o
> +obj-$(CONFIG_CORESIGHT_TNOC) += coresight-tnoc.o
>  obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x.o
>  coresight-etm3x-y := coresight-etm3x-core.o coresight-etm-cp14.o \
>                      coresight-etm3x-sysfs.o
> diff --git a/drivers/hwtracing/coresight/coresight-tnoc.c b/drivers/hwtracing/coresight/coresight-tnoc.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..fad8e61f05ef25989aba1be342c547f835e8953a
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-tnoc.c
> @@ -0,0 +1,190 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/amba/bus.h>
> +#include <linux/io.h>
> +#include <linux/coresight.h>
> +#include <linux/of.h>

Please include headers in alphabetical ordering.

> +#include "coresight-priv.h"
> +#include "coresight-tnoc.h"
> +#include "coresight-trace-id.h"
> +
> +static void trace_noc_enable_hw(struct trace_noc_drvdata *drvdata)
> +{
> +       u32 val;
> +
> +       /* Set ATID */
> +       writel_relaxed(drvdata->atid, drvdata->base + TRACE_NOC_XLD);
> +
> +       /* Config sync CR */
> +       writel_relaxed(0xffff, drvdata->base + TRACE_NOC_SYNCR);
> +
> +       /* Set frequency value */
> +       writel_relaxed(drvdata->freq_req_val, drvdata->base + TRACE_NOC_FREQVAL);
> +
> +       /* Set Ctrl register */
> +       val = readl_relaxed(drvdata->base + TRACE_NOC_CTRL);
> +
> +       if (drvdata->flag_type == FLAG_TS)
> +               val = val | TRACE_NOC_CTRL_FLAGTYPE;
> +       else
> +               val = val & ~TRACE_NOC_CTRL_FLAGTYPE;
> +
> +       if (drvdata->freq_type == FREQ_TS)
> +               val = val | TRACE_NOC_CTRL_FREQTYPE;
> +       else
> +               val = val & ~TRACE_NOC_CTRL_FREQTYPE;
> +
> +       val = val | TRACE_NOC_CTRL_PORTEN;
> +       writel_relaxed(val, drvdata->base + TRACE_NOC_CTRL);

It is fine for using writel_relaxed() for continuous writing into the
same component.  However, the last writing into TRACE_NOC_CTRL enales
the NOC but without any barrier guard, it might cause the out-of-order
issue with Arm CoreSight modules (or any other relevant endpoints).

I'd like suggest to use writel() for writing TRACE_NOC_CTRL.

> +       dev_dbg(drvdata->dev, "Trace NOC is enabled\n");
> +}
> +
> +static int trace_noc_enable(struct coresight_device *csdev, struct coresight_connection *inport,
> +                           struct coresight_connection *outport)
> +{
> +       struct trace_noc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> +       spin_lock(&drvdata->spinlock);
> +       if (csdev->refcnt == 0)

AFAICT csdev->refcnt is only used in sink drivers.  This driver is for
funnel, it should use `inport->dest_refcnt` as the counter.

> +               trace_noc_enable_hw(drvdata);
> +
> +       csdev->refcnt++;
> +       spin_unlock(&drvdata->spinlock);
> +
> +       return 0;
> +}
> +
> +static void trace_noc_disable_hw(struct trace_noc_drvdata *drvdata)
> +{
> +       writel_relaxed(0x0, drvdata->base + TRACE_NOC_CTRL);

Same with the comment above for using writel() to replace
writel_relaxed().

> +       dev_dbg(drvdata->dev, "Trace NOC is disabled\n");
> +}
> +
> +static void trace_noc_disable(struct coresight_device *csdev, struct coresight_connection *inport,
> +                             struct coresight_connection *outport)
> +{
> +       struct trace_noc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> +       spin_lock(&drvdata->spinlock);
> +       if (--csdev->refcnt == 0)
> +               trace_noc_disable_hw(drvdata);
> +
> +       spin_unlock(&drvdata->spinlock);
> +       dev_info(drvdata->dev, "Trace NOC is disabled\n");
> +}
> +
> +static const struct coresight_ops_link trace_noc_link_ops = {
> +       .enable         = trace_noc_enable,
> +       .disable        = trace_noc_disable,
> +};
> +
> +static const struct coresight_ops trace_noc_cs_ops = {
> +       .link_ops       = &trace_noc_link_ops,
> +};
> +
> +static int trace_noc_init_default_data(struct trace_noc_drvdata *drvdata)
> +{
> +       int atid;
> +
> +       atid = coresight_trace_id_get_system_id();
> +       if (atid < 0)
> +               return atid;
> +
> +       drvdata->atid = atid;
> +
> +       drvdata->freq_type = FREQ_TS;

I don't see anywhere uses FREQ.  Please remove the unused definitions
and related code.

> +       drvdata->flag_type = FLAG;

FLAG_TS is not used in the driver as well.  Remove it.

> +       drvdata->freq_req_val = 0;
> +
> +       return 0;
> +}
> +
> +static int trace_noc_probe(struct amba_device *adev, const struct amba_id *id)
> +{
> +       struct device *dev = &adev->dev;
> +       struct coresight_platform_data *pdata;
> +       struct trace_noc_drvdata *drvdata;
> +       struct coresight_desc desc = { 0 };
> +       int ret;
> +
> +       desc.name = coresight_alloc_device_name(&trace_noc_devs, dev);
> +       if (!desc.name)
> +               return -ENOMEM;
> +       pdata = coresight_get_platform_data(dev);
> +       if (IS_ERR(pdata))
> +               return PTR_ERR(pdata);
> +       adev->dev.platform_data = pdata;
> +
> +       drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> +       if (!drvdata)
> +               return -ENOMEM;
> +
> +       drvdata->dev = &adev->dev;
> +       dev_set_drvdata(dev, drvdata);
> +
> +       drvdata->base = devm_ioremap_resource(dev, &adev->res);
> +       if (!drvdata->base)
> +               return -ENOMEM;
> +
> +       spin_lock_init(&drvdata->spinlock);
> +
> +       ret = trace_noc_init_default_data(drvdata);
> +       if (ret)
> +               return ret;
> +
> +       desc.ops = &trace_noc_cs_ops;
> +       desc.type = CORESIGHT_DEV_TYPE_LINK;
> +       desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_MERG;
> +       desc.pdata = adev->dev.platform_data;
> +       desc.dev = &adev->dev;
> +       desc.access = CSDEV_ACCESS_IOMEM(drvdata->base);
> +       drvdata->csdev = coresight_register(&desc);
> +       if (IS_ERR(drvdata->csdev))
> +               return PTR_ERR(drvdata->csdev);
> +
> +       pm_runtime_put(&adev->dev);
> +
> +       return 0;
> +}
> +
> +static void trace_noc_remove(struct amba_device *adev)
> +{
> +       struct trace_noc_drvdata *drvdata = dev_get_drvdata(&adev->dev);
> +
> +       coresight_trace_id_put_system_id(drvdata->atid);
> +       coresight_unregister(drvdata->csdev);
> +}
> +
> +static struct amba_id trace_noc_ids[] = {
> +       {
> +               .id     = 0x000f0c00,
> +               .mask   = 0x000fff00,

Unlike Arm CoreSight drivers (the mask value is 0x000fffff in most
cases), could you remind me why here the mask is 0x000fff00?

> +       },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(amba, trace_noc_ids);
> +
> +static struct amba_driver trace_noc_driver = {
> +       .drv = {
> +               .name   = "coresight-trace-noc",
> +               .owner  = THIS_MODULE,

I don't think you need to explicitly set `THIS_MODULE`, as this will
be set default by amba_driver_register().

> +               .suppress_bind_attrs = true,
> +       },
> +       .probe          = trace_noc_probe,
> +       .remove         = trace_noc_remove,
> +       .id_table       = trace_noc_ids,
> +};
> +
> +module_amba_driver(trace_noc_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Trace NOC driver");
> diff --git a/drivers/hwtracing/coresight/coresight-tnoc.h b/drivers/hwtracing/coresight/coresight-tnoc.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..b6bd1ef659897d8e0994c5e8514e8cbdd16eebd8
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-tnoc.h
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#define TRACE_NOC_CTRL 0x008
> +#define TRACE_NOC_XLD  0x010
> +#define TRACE_NOC_FREQVAL      0x018
> +#define TRACE_NOC_SYNCR        0x020
> +
> +/* Enable generation of output ATB traffic.*/
> +#define TRACE_NOC_CTRL_PORTEN  BIT(0)
> +/* Writing 1 to issue a FREQ or FREQ_TS packet*/
> +#define TRACE_NOC_CTRL_FREQTSREQ       BIT(5)

This is not used.  Remove it.

> +/* Sets the type of issued ATB FLAG packets. 0: 'FLAG' packets; 1: 'FLAG_TS' packets.*/
> +#define TRACE_NOC_CTRL_FLAGTYPE                BIT(7)

> +/* sets the type of issued ATB FREQ packets. 0: 'FREQ' packets; 1: 'FREQ_TS' packets.*/
> +#define TRACE_NOC_CTRL_FREQTYPE                BIT(8)
> +DEFINE_CORESIGHT_DEVLIST(trace_noc_devs, "traceNoc");

It is good to move the definition into C file.

Thanks,
Leo

> +
> +/**
> + * struct trace_noc_drvdata - specifics associated to a trace noc component
> + * @base:      memory mapped base address for this component.
> + * @dev:       device node for trace_noc_drvdata.
> + * @csdev:     component vitals needed by the framework.
> + * @spinlock:  only one at a time pls.
> + * @atid:      id for the trace packet.
> + * @freqtype:  0: 'FREQ' packets; 1: 'FREQ_TS' packets.
> + * @flagtype:  0: 'FLAG' packets; 1: 'FLAG_TS' packets.
> + * @freq_req_val:       set frequency values carried by 'FREQ' and 'FREQ_TS' packets.
> + */
> +struct trace_noc_drvdata {
> +       void __iomem            *base;
> +       struct device           *dev;
> +       struct coresight_device *csdev;
> +       spinlock_t              spinlock; /* lock for the drvdata. */
> +       u32                     atid;
> +       u32                     freq_type;
> +       u32                     flag_type;
> +       u32                     freq_req_val;
> +};
> +
> +/* freq type */
> +enum freq_type {
> +       FREQ,
> +       FREQ_TS,
> +};
> +
> +/* flag type */
> +enum flag_type {
> +       FLAG,
> +       FLAG_TS,
> +};
> 
> --
> 2.34.1
> 
> _______________________________________________
> CoreSight mailing list -- coresight@lists.linaro.org
> To unsubscribe send an email to coresight-leave@lists.linaro.org

Re: [PATCH v2 2/5] coresight: add coresight Trace NOC driver

[Yuanfang Zhang]

On 2/27/2025 7:39 PM, Leo Yan wrote:
> Hi Yuanfang,
> 
> On Wed, Feb 26, 2025 at 07:05:51PM +0800, Yuanfang Zhang wrote:
>>
>> Add driver to support Coresight device Trace NOC(Network On Chip).
>> Trace NOC is an integration hierarchy which is a replacement of
>> Dragonlink configuration. It brings together debug components like
>> TPDA, funnel and interconnect Trace Noc.
>>
>> It sits in the different subsystem of SOC and aggregates the trace
>> and transports to QDSS trace bus.
>>
>> Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com>
>> ---
>>  drivers/hwtracing/coresight/Kconfig          |  13 ++
>>  drivers/hwtracing/coresight/Makefile         |   1 +
>>  drivers/hwtracing/coresight/coresight-tnoc.c | 190 +++++++++++++++++++++++++++
>>  drivers/hwtracing/coresight/coresight-tnoc.h |  53 ++++++++
>>  4 files changed, 257 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>> index 06f0a7594169c5f03ca5f893b7debd294587de78..6cfd160f09d383ab5f5aa276fa57496a52c8f961 100644
>> --- a/drivers/hwtracing/coresight/Kconfig
>> +++ b/drivers/hwtracing/coresight/Kconfig
>> @@ -247,4 +247,17 @@ config CORESIGHT_DUMMY
>>
>>           To compile this driver as a module, choose M here: the module will be
>>           called coresight-dummy.
>> +
>> +config CORESIGHT_TNOC
>> +       tristate "Coresight Trace Noc driver"
>> +       help
>> +         This driver provides support for Trace NoC component.
>> +         Trace NoC is a interconnect that is used to collect trace from
>> +         various subsystems and transport it QDSS trace sink.It sits in
> 
> Trace NoC is an interconnect used to collect traces from various
> subsystems and transport to a QDSS trace sink.
sure, will update in next version.
> 
>> +         the different tiles of SOC and aggregates the trace local to the
>> +         tile and transports it another tile or to QDSS trace sink eventually.
>> +
>> +         To compile this driver as a module, choose M here: the module will be
>> +         called coresight-tnoc.
>> +
>>  endif
>> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
>> index 4ba478211b318ea5305f9f98dda40a041759f09f..60b729979f19c8f8848c77c290605132dba1a991 100644
>> --- a/drivers/hwtracing/coresight/Makefile
>> +++ b/drivers/hwtracing/coresight/Makefile
>> @@ -34,6 +34,7 @@ obj-$(CONFIG_CORESIGHT_SINK_TPIU) += coresight-tpiu.o
>>  obj-$(CONFIG_CORESIGHT_SINK_ETBV10) += coresight-etb10.o
>>  obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o \
>>                                            coresight-replicator.o
>> +obj-$(CONFIG_CORESIGHT_TNOC) += coresight-tnoc.o
>>  obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x.o
>>  coresight-etm3x-y := coresight-etm3x-core.o coresight-etm-cp14.o \
>>                      coresight-etm3x-sysfs.o
>> diff --git a/drivers/hwtracing/coresight/coresight-tnoc.c b/drivers/hwtracing/coresight/coresight-tnoc.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..fad8e61f05ef25989aba1be342c547f835e8953a
>> --- /dev/null
>> +++ b/drivers/hwtracing/coresight/coresight-tnoc.c
>> @@ -0,0 +1,190 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/amba/bus.h>
>> +#include <linux/io.h>
>> +#include <linux/coresight.h>
>> +#include <linux/of.h>
> 
> Please include headers in alphabetical ordering.
sure, will update in next version.
> 
>> +#include "coresight-priv.h"
>> +#include "coresight-tnoc.h"
>> +#include "coresight-trace-id.h"
>> +
>> +static void trace_noc_enable_hw(struct trace_noc_drvdata *drvdata)
>> +{
>> +       u32 val;
>> +
>> +       /* Set ATID */
>> +       writel_relaxed(drvdata->atid, drvdata->base + TRACE_NOC_XLD);
>> +
>> +       /* Config sync CR */
>> +       writel_relaxed(0xffff, drvdata->base + TRACE_NOC_SYNCR);
>> +
>> +       /* Set frequency value */
>> +       writel_relaxed(drvdata->freq_req_val, drvdata->base + TRACE_NOC_FREQVAL);
>> +
>> +       /* Set Ctrl register */
>> +       val = readl_relaxed(drvdata->base + TRACE_NOC_CTRL);
>> +
>> +       if (drvdata->flag_type == FLAG_TS)
>> +               val = val | TRACE_NOC_CTRL_FLAGTYPE;
>> +       else
>> +               val = val & ~TRACE_NOC_CTRL_FLAGTYPE;
>> +
>> +       if (drvdata->freq_type == FREQ_TS)
>> +               val = val | TRACE_NOC_CTRL_FREQTYPE;
>> +       else
>> +               val = val & ~TRACE_NOC_CTRL_FREQTYPE;
>> +
>> +       val = val | TRACE_NOC_CTRL_PORTEN;
>> +       writel_relaxed(val, drvdata->base + TRACE_NOC_CTRL);
> 
> It is fine for using writel_relaxed() for continuous writing into the
> same component.  However, the last writing into TRACE_NOC_CTRL enales
> the NOC but without any barrier guard, it might cause the out-of-order
> issue with Arm CoreSight modules (or any other relevant endpoints).
sure, will update in next version.
> 
> I'd like suggest to use writel() for writing TRACE_NOC_CTRL.
> 
>> +       dev_dbg(drvdata->dev, "Trace NOC is enabled\n");
>> +}
>> +
>> +static int trace_noc_enable(struct coresight_device *csdev, struct coresight_connection *inport,
>> +                           struct coresight_connection *outport)
>> +{
>> +       struct trace_noc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +
>> +       spin_lock(&drvdata->spinlock);
>> +       if (csdev->refcnt == 0)
> 
> AFAICT csdev->refcnt is only used in sink drivers.  This driver is for
> funnel, it should use `inport->dest_refcnt` as the counter.
sure, will update in next version.
> 
>> +               trace_noc_enable_hw(drvdata);
>> +
>> +       csdev->refcnt++;
>> +       spin_unlock(&drvdata->spinlock);
>> +
>> +       return 0;
>> +}
>> +
>> +static void trace_noc_disable_hw(struct trace_noc_drvdata *drvdata)
>> +{
>> +       writel_relaxed(0x0, drvdata->base + TRACE_NOC_CTRL);
> 
> Same with the comment above for using writel() to replace
> writel_relaxed().
sure.
> 
>> +       dev_dbg(drvdata->dev, "Trace NOC is disabled\n");
>> +}
>> +
>> +static void trace_noc_disable(struct coresight_device *csdev, struct coresight_connection *inport,
>> +                             struct coresight_connection *outport)
>> +{
>> +       struct trace_noc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +
>> +       spin_lock(&drvdata->spinlock);
>> +       if (--csdev->refcnt == 0)
>> +               trace_noc_disable_hw(drvdata);
>> +
>> +       spin_unlock(&drvdata->spinlock);
>> +       dev_info(drvdata->dev, "Trace NOC is disabled\n");
>> +}
>> +
>> +static const struct coresight_ops_link trace_noc_link_ops = {
>> +       .enable         = trace_noc_enable,
>> +       .disable        = trace_noc_disable,
>> +};
>> +
>> +static const struct coresight_ops trace_noc_cs_ops = {
>> +       .link_ops       = &trace_noc_link_ops,
>> +};
>> +
>> +static int trace_noc_init_default_data(struct trace_noc_drvdata *drvdata)
>> +{
>> +       int atid;
>> +
>> +       atid = coresight_trace_id_get_system_id();
>> +       if (atid < 0)
>> +               return atid;
>> +
>> +       drvdata->atid = atid;
>> +
>> +       drvdata->freq_type = FREQ_TS;
> 
> I don't see anywhere uses FREQ.  Please remove the unused definitions
> and related code.
it is used in trace_noc_enable_hw().
> 
>> +       drvdata->flag_type = FLAG;
> 
> FLAG_TS is not used in the driver as well.  Remove it.
it is used in trace_noc_enable_hw().
> 
>> +       drvdata->freq_req_val = 0;
>> +
>> +       return 0;
>> +}
>> +
>> +static int trace_noc_probe(struct amba_device *adev, const struct amba_id *id)
>> +{
>> +       struct device *dev = &adev->dev;
>> +       struct coresight_platform_data *pdata;
>> +       struct trace_noc_drvdata *drvdata;
>> +       struct coresight_desc desc = { 0 };
>> +       int ret;
>> +
>> +       desc.name = coresight_alloc_device_name(&trace_noc_devs, dev);
>> +       if (!desc.name)
>> +               return -ENOMEM;
>> +       pdata = coresight_get_platform_data(dev);
>> +       if (IS_ERR(pdata))
>> +               return PTR_ERR(pdata);
>> +       adev->dev.platform_data = pdata;
>> +
>> +       drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>> +       if (!drvdata)
>> +               return -ENOMEM;
>> +
>> +       drvdata->dev = &adev->dev;
>> +       dev_set_drvdata(dev, drvdata);
>> +
>> +       drvdata->base = devm_ioremap_resource(dev, &adev->res);
>> +       if (!drvdata->base)
>> +               return -ENOMEM;
>> +
>> +       spin_lock_init(&drvdata->spinlock);
>> +
>> +       ret = trace_noc_init_default_data(drvdata);
>> +       if (ret)
>> +               return ret;
>> +
>> +       desc.ops = &trace_noc_cs_ops;
>> +       desc.type = CORESIGHT_DEV_TYPE_LINK;
>> +       desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_MERG;
>> +       desc.pdata = adev->dev.platform_data;
>> +       desc.dev = &adev->dev;
>> +       desc.access = CSDEV_ACCESS_IOMEM(drvdata->base);
>> +       drvdata->csdev = coresight_register(&desc);
>> +       if (IS_ERR(drvdata->csdev))
>> +               return PTR_ERR(drvdata->csdev);
>> +
>> +       pm_runtime_put(&adev->dev);
>> +
>> +       return 0;
>> +}
>> +
>> +static void trace_noc_remove(struct amba_device *adev)
>> +{
>> +       struct trace_noc_drvdata *drvdata = dev_get_drvdata(&adev->dev);
>> +
>> +       coresight_trace_id_put_system_id(drvdata->atid);
>> +       coresight_unregister(drvdata->csdev);
>> +}
>> +
>> +static struct amba_id trace_noc_ids[] = {
>> +       {
>> +               .id     = 0x000f0c00,
>> +               .mask   = 0x000fff00,
> 
> Unlike Arm CoreSight drivers (the mask value is 0x000fffff in most
> cases), could you remind me why here the mask is 0x000fff00?
Because bit 0-7 has different values, records the number of trace initiator NIUs implemented in the NoC. 
> 
>> +       },
>> +       {},
>> +};
>> +MODULE_DEVICE_TABLE(amba, trace_noc_ids);
>> +
>> +static struct amba_driver trace_noc_driver = {
>> +       .drv = {
>> +               .name   = "coresight-trace-noc",
>> +               .owner  = THIS_MODULE,
> 
> I don't think you need to explicitly set `THIS_MODULE`, as this will
> be set default by amba_driver_register().
will remove in next version.
> 
>> +               .suppress_bind_attrs = true,
>> +       },
>> +       .probe          = trace_noc_probe,
>> +       .remove         = trace_noc_remove,
>> +       .id_table       = trace_noc_ids,
>> +};
>> +
>> +module_amba_driver(trace_noc_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Trace NOC driver");
>> diff --git a/drivers/hwtracing/coresight/coresight-tnoc.h b/drivers/hwtracing/coresight/coresight-tnoc.h
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..b6bd1ef659897d8e0994c5e8514e8cbdd16eebd8
>> --- /dev/null
>> +++ b/drivers/hwtracing/coresight/coresight-tnoc.h
>> @@ -0,0 +1,53 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#define TRACE_NOC_CTRL 0x008
>> +#define TRACE_NOC_XLD  0x010
>> +#define TRACE_NOC_FREQVAL      0x018
>> +#define TRACE_NOC_SYNCR        0x020
>> +
>> +/* Enable generation of output ATB traffic.*/
>> +#define TRACE_NOC_CTRL_PORTEN  BIT(0)
>> +/* Writing 1 to issue a FREQ or FREQ_TS packet*/
>> +#define TRACE_NOC_CTRL_FREQTSREQ       BIT(5)
> 
> This is not used.  Remove it.
it is used in trace_noc_enable_hw().
> 
>> +/* Sets the type of issued ATB FLAG packets. 0: 'FLAG' packets; 1: 'FLAG_TS' packets.*/
>> +#define TRACE_NOC_CTRL_FLAGTYPE                BIT(7)
> 
>> +/* sets the type of issued ATB FREQ packets. 0: 'FREQ' packets; 1: 'FREQ_TS' packets.*/
>> +#define TRACE_NOC_CTRL_FREQTYPE                BIT(8)
>> +DEFINE_CORESIGHT_DEVLIST(trace_noc_devs, "traceNoc");
> 
> It is good to move the definition into C file.
will update in next version.
> 
> Thanks,
> Leo
> 
>> +
>> +/**
>> + * struct trace_noc_drvdata - specifics associated to a trace noc component
>> + * @base:      memory mapped base address for this component.
>> + * @dev:       device node for trace_noc_drvdata.
>> + * @csdev:     component vitals needed by the framework.
>> + * @spinlock:  only one at a time pls.
>> + * @atid:      id for the trace packet.
>> + * @freqtype:  0: 'FREQ' packets; 1: 'FREQ_TS' packets.
>> + * @flagtype:  0: 'FLAG' packets; 1: 'FLAG_TS' packets.
>> + * @freq_req_val:       set frequency values carried by 'FREQ' and 'FREQ_TS' packets.
>> + */
>> +struct trace_noc_drvdata {
>> +       void __iomem            *base;
>> +       struct device           *dev;
>> +       struct coresight_device *csdev;
>> +       spinlock_t              spinlock; /* lock for the drvdata. */
>> +       u32                     atid;
>> +       u32                     freq_type;
>> +       u32                     flag_type;
>> +       u32                     freq_req_val;
>> +};
>> +
>> +/* freq type */
>> +enum freq_type {
>> +       FREQ,
>> +       FREQ_TS,
>> +};
>> +
>> +/* flag type */
>> +enum flag_type {
>> +       FLAG,
>> +       FLAG_TS,
>> +};
>>
>> --
>> 2.34.1
>>
>> _______________________________________________
>> CoreSight mailing list -- coresight@lists.linaro.org
>> To unsubscribe send an email to coresight-leave@lists.linaro.org

Re: [PATCH v2 2/5] coresight: add coresight Trace NOC driver

[Leo Yan]

On Thu, Mar 06, 2025 at 04:22:20PM +0800, Yuanfang Zhang wrote:

[...]

> >> +static int trace_noc_init_default_data(struct trace_noc_drvdata *drvdata)
> >> +{
> >> +       int atid;
> >> +
> >> +       atid = coresight_trace_id_get_system_id();
> >> +       if (atid < 0)
> >> +               return atid;
> >> +
> >> +       drvdata->atid = atid;
> >> +
> >> +       drvdata->freq_type = FREQ_TS;
> > 
> > I don't see anywhere uses FREQ.  Please remove the unused definitions
> > and related code.
>
> it is used in trace_noc_enable_hw().

I understood some macros and definitions are used by seqential patches.

A good practice is code should be added only when they are used.  This
can allow every patch in neat way and easier for review.

Thanks,
Leo

> > 
> >> +       drvdata->flag_type = FLAG;
> > 
> > FLAG_TS is not used in the driver as well.  Remove it.
> it is used in trace_noc_enable_hw().

Re: [PATCH v2 2/5] coresight: add coresight Trace NOC driver

[Yuanfang Zhang]

On 3/6/2025 4:22 PM, Yuanfang Zhang wrote:
> 
> 
> On 2/27/2025 7:39 PM, Leo Yan wrote:
>> Hi Yuanfang,
>>
>> On Wed, Feb 26, 2025 at 07:05:51PM +0800, Yuanfang Zhang wrote:
>>>
>>> Add driver to support Coresight device Trace NOC(Network On Chip).
>>> Trace NOC is an integration hierarchy which is a replacement of
>>> Dragonlink configuration. It brings together debug components like
>>> TPDA, funnel and interconnect Trace Noc.
>>>
>>> It sits in the different subsystem of SOC and aggregates the trace
>>> and transports to QDSS trace bus.
>>>
>>> Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com>
>>> ---
>>>  drivers/hwtracing/coresight/Kconfig          |  13 ++
>>>  drivers/hwtracing/coresight/Makefile         |   1 +
>>>  drivers/hwtracing/coresight/coresight-tnoc.c | 190 +++++++++++++++++++++++++++
>>>  drivers/hwtracing/coresight/coresight-tnoc.h |  53 ++++++++
>>>  4 files changed, 257 insertions(+)
>>>
>>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>>> index 06f0a7594169c5f03ca5f893b7debd294587de78..6cfd160f09d383ab5f5aa276fa57496a52c8f961 100644
>>> --- a/drivers/hwtracing/coresight/Kconfig
>>> +++ b/drivers/hwtracing/coresight/Kconfig
>>> @@ -247,4 +247,17 @@ config CORESIGHT_DUMMY
>>>
>>>           To compile this driver as a module, choose M here: the module will be
>>>           called coresight-dummy.
>>> +
>>> +config CORESIGHT_TNOC
>>> +       tristate "Coresight Trace Noc driver"
>>> +       help
>>> +         This driver provides support for Trace NoC component.
>>> +         Trace NoC is a interconnect that is used to collect trace from
>>> +         various subsystems and transport it QDSS trace sink.It sits in
>>
>> Trace NoC is an interconnect used to collect traces from various
>> subsystems and transport to a QDSS trace sink.
> sure, will update in next version.
>>
>>> +         the different tiles of SOC and aggregates the trace local to the
>>> +         tile and transports it another tile or to QDSS trace sink eventually.
>>> +
>>> +         To compile this driver as a module, choose M here: the module will be
>>> +         called coresight-tnoc.
>>> +
>>>  endif
>>> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
>>> index 4ba478211b318ea5305f9f98dda40a041759f09f..60b729979f19c8f8848c77c290605132dba1a991 100644
>>> --- a/drivers/hwtracing/coresight/Makefile
>>> +++ b/drivers/hwtracing/coresight/Makefile
>>> @@ -34,6 +34,7 @@ obj-$(CONFIG_CORESIGHT_SINK_TPIU) += coresight-tpiu.o
>>>  obj-$(CONFIG_CORESIGHT_SINK_ETBV10) += coresight-etb10.o
>>>  obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o \
>>>                                            coresight-replicator.o
>>> +obj-$(CONFIG_CORESIGHT_TNOC) += coresight-tnoc.o
>>>  obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x.o
>>>  coresight-etm3x-y := coresight-etm3x-core.o coresight-etm-cp14.o \
>>>                      coresight-etm3x-sysfs.o
>>> diff --git a/drivers/hwtracing/coresight/coresight-tnoc.c b/drivers/hwtracing/coresight/coresight-tnoc.c
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..fad8e61f05ef25989aba1be342c547f835e8953a
>>> --- /dev/null
>>> +++ b/drivers/hwtracing/coresight/coresight-tnoc.c
>>> @@ -0,0 +1,190 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
>>> + */
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/device.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/amba/bus.h>
>>> +#include <linux/io.h>
>>> +#include <linux/coresight.h>
>>> +#include <linux/of.h>
>>
>> Please include headers in alphabetical ordering.
> sure, will update in next version.
>>
>>> +#include "coresight-priv.h"
>>> +#include "coresight-tnoc.h"
>>> +#include "coresight-trace-id.h"
>>> +
>>> +static void trace_noc_enable_hw(struct trace_noc_drvdata *drvdata)
>>> +{
>>> +       u32 val;
>>> +
>>> +       /* Set ATID */
>>> +       writel_relaxed(drvdata->atid, drvdata->base + TRACE_NOC_XLD);
>>> +
>>> +       /* Config sync CR */
>>> +       writel_relaxed(0xffff, drvdata->base + TRACE_NOC_SYNCR);
>>> +
>>> +       /* Set frequency value */
>>> +       writel_relaxed(drvdata->freq_req_val, drvdata->base + TRACE_NOC_FREQVAL);
>>> +
>>> +       /* Set Ctrl register */
>>> +       val = readl_relaxed(drvdata->base + TRACE_NOC_CTRL);
>>> +
>>> +       if (drvdata->flag_type == FLAG_TS)
>>> +               val = val | TRACE_NOC_CTRL_FLAGTYPE;
>>> +       else
>>> +               val = val & ~TRACE_NOC_CTRL_FLAGTYPE;
>>> +
>>> +       if (drvdata->freq_type == FREQ_TS)
>>> +               val = val | TRACE_NOC_CTRL_FREQTYPE;
>>> +       else
>>> +               val = val & ~TRACE_NOC_CTRL_FREQTYPE;
>>> +
>>> +       val = val | TRACE_NOC_CTRL_PORTEN;
>>> +       writel_relaxed(val, drvdata->base + TRACE_NOC_CTRL);
>>
>> It is fine for using writel_relaxed() for continuous writing into the
>> same component.  However, the last writing into TRACE_NOC_CTRL enales
>> the NOC but without any barrier guard, it might cause the out-of-order
>> issue with Arm CoreSight modules (or any other relevant endpoints).
> sure, will update in next version.
>>
>> I'd like suggest to use writel() for writing TRACE_NOC_CTRL.
>>
>>> +       dev_dbg(drvdata->dev, "Trace NOC is enabled\n");
>>> +}
>>> +
>>> +static int trace_noc_enable(struct coresight_device *csdev, struct coresight_connection *inport,
>>> +                           struct coresight_connection *outport)
>>> +{
>>> +       struct trace_noc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>>> +
>>> +       spin_lock(&drvdata->spinlock);
>>> +       if (csdev->refcnt == 0)
>>
>> AFAICT csdev->refcnt is only used in sink drivers.  This driver is for
>> funnel, it should use `inport->dest_refcnt` as the counter.
> sure, will update in next version.

TNOC is slightly different from other links in that it does not require port based enablement.
Only need to enable once, all ports will be activated. So i will still use csdev->refcnt as a counter
>>
>>> +               trace_noc_enable_hw(drvdata);
>>> +
>>> +       csdev->refcnt++;
>>> +       spin_unlock(&drvdata->spinlock);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static void trace_noc_disable_hw(struct trace_noc_drvdata *drvdata)
>>> +{
>>> +       writel_relaxed(0x0, drvdata->base + TRACE_NOC_CTRL);
>>
>> Same with the comment above for using writel() to replace
>> writel_relaxed().
> sure.
>>
>>> +       dev_dbg(drvdata->dev, "Trace NOC is disabled\n");
>>> +}
>>> +
>>> +static void trace_noc_disable(struct coresight_device *csdev, struct coresight_connection *inport,
>>> +                             struct coresight_connection *outport)
>>> +{
>>> +       struct trace_noc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>>> +
>>> +       spin_lock(&drvdata->spinlock);
>>> +       if (--csdev->refcnt == 0)
>>> +               trace_noc_disable_hw(drvdata);
>>> +
>>> +       spin_unlock(&drvdata->spinlock);
>>> +       dev_info(drvdata->dev, "Trace NOC is disabled\n");
>>> +}
>>> +
>>> +static const struct coresight_ops_link trace_noc_link_ops = {
>>> +       .enable         = trace_noc_enable,
>>> +       .disable        = trace_noc_disable,
>>> +};
>>> +
>>> +static const struct coresight_ops trace_noc_cs_ops = {
>>> +       .link_ops       = &trace_noc_link_ops,
>>> +};
>>> +
>>> +static int trace_noc_init_default_data(struct trace_noc_drvdata *drvdata)
>>> +{
>>> +       int atid;
>>> +
>>> +       atid = coresight_trace_id_get_system_id();
>>> +       if (atid < 0)
>>> +               return atid;
>>> +
>>> +       drvdata->atid = atid;
>>> +
>>> +       drvdata->freq_type = FREQ_TS;
>>
>> I don't see anywhere uses FREQ.  Please remove the unused definitions
>> and related code.
> it is used in trace_noc_enable_hw().
>>
>>> +       drvdata->flag_type = FLAG;
>>
>> FLAG_TS is not used in the driver as well.  Remove it.
> it is used in trace_noc_enable_hw().
>>
>>> +       drvdata->freq_req_val = 0;
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int trace_noc_probe(struct amba_device *adev, const struct amba_id *id)
>>> +{
>>> +       struct device *dev = &adev->dev;
>>> +       struct coresight_platform_data *pdata;
>>> +       struct trace_noc_drvdata *drvdata;
>>> +       struct coresight_desc desc = { 0 };
>>> +       int ret;
>>> +
>>> +       desc.name = coresight_alloc_device_name(&trace_noc_devs, dev);
>>> +       if (!desc.name)
>>> +               return -ENOMEM;
>>> +       pdata = coresight_get_platform_data(dev);
>>> +       if (IS_ERR(pdata))
>>> +               return PTR_ERR(pdata);
>>> +       adev->dev.platform_data = pdata;
>>> +
>>> +       drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>>> +       if (!drvdata)
>>> +               return -ENOMEM;
>>> +
>>> +       drvdata->dev = &adev->dev;
>>> +       dev_set_drvdata(dev, drvdata);
>>> +
>>> +       drvdata->base = devm_ioremap_resource(dev, &adev->res);
>>> +       if (!drvdata->base)
>>> +               return -ENOMEM;
>>> +
>>> +       spin_lock_init(&drvdata->spinlock);
>>> +
>>> +       ret = trace_noc_init_default_data(drvdata);
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       desc.ops = &trace_noc_cs_ops;
>>> +       desc.type = CORESIGHT_DEV_TYPE_LINK;
>>> +       desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_MERG;
>>> +       desc.pdata = adev->dev.platform_data;
>>> +       desc.dev = &adev->dev;
>>> +       desc.access = CSDEV_ACCESS_IOMEM(drvdata->base);
>>> +       drvdata->csdev = coresight_register(&desc);
>>> +       if (IS_ERR(drvdata->csdev))
>>> +               return PTR_ERR(drvdata->csdev);
>>> +
>>> +       pm_runtime_put(&adev->dev);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static void trace_noc_remove(struct amba_device *adev)
>>> +{
>>> +       struct trace_noc_drvdata *drvdata = dev_get_drvdata(&adev->dev);
>>> +
>>> +       coresight_trace_id_put_system_id(drvdata->atid);
>>> +       coresight_unregister(drvdata->csdev);
>>> +}
>>> +
>>> +static struct amba_id trace_noc_ids[] = {
>>> +       {
>>> +               .id     = 0x000f0c00,
>>> +               .mask   = 0x000fff00,
>>
>> Unlike Arm CoreSight drivers (the mask value is 0x000fffff in most
>> cases), could you remind me why here the mask is 0x000fff00?
> Because bit 0-7 has different values, records the number of trace initiator NIUs implemented in the NoC. 
>>
>>> +       },
>>> +       {},
>>> +};
>>> +MODULE_DEVICE_TABLE(amba, trace_noc_ids);
>>> +
>>> +static struct amba_driver trace_noc_driver = {
>>> +       .drv = {
>>> +               .name   = "coresight-trace-noc",
>>> +               .owner  = THIS_MODULE,
>>
>> I don't think you need to explicitly set `THIS_MODULE`, as this will
>> be set default by amba_driver_register().
> will remove in next version.
>>
>>> +               .suppress_bind_attrs = true,
>>> +       },
>>> +       .probe          = trace_noc_probe,
>>> +       .remove         = trace_noc_remove,
>>> +       .id_table       = trace_noc_ids,
>>> +};
>>> +
>>> +module_amba_driver(trace_noc_driver);
>>> +
>>> +MODULE_LICENSE("GPL");
>>> +MODULE_DESCRIPTION("Trace NOC driver");
>>> diff --git a/drivers/hwtracing/coresight/coresight-tnoc.h b/drivers/hwtracing/coresight/coresight-tnoc.h
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..b6bd1ef659897d8e0994c5e8514e8cbdd16eebd8
>>> --- /dev/null
>>> +++ b/drivers/hwtracing/coresight/coresight-tnoc.h
>>> @@ -0,0 +1,53 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>>> + */
>>> +
>>> +#define TRACE_NOC_CTRL 0x008
>>> +#define TRACE_NOC_XLD  0x010
>>> +#define TRACE_NOC_FREQVAL      0x018
>>> +#define TRACE_NOC_SYNCR        0x020
>>> +
>>> +/* Enable generation of output ATB traffic.*/
>>> +#define TRACE_NOC_CTRL_PORTEN  BIT(0)
>>> +/* Writing 1 to issue a FREQ or FREQ_TS packet*/
>>> +#define TRACE_NOC_CTRL_FREQTSREQ       BIT(5)
>>
>> This is not used.  Remove it.
> it is used in trace_noc_enable_hw().
>>
>>> +/* Sets the type of issued ATB FLAG packets. 0: 'FLAG' packets; 1: 'FLAG_TS' packets.*/
>>> +#define TRACE_NOC_CTRL_FLAGTYPE                BIT(7)
>>
>>> +/* sets the type of issued ATB FREQ packets. 0: 'FREQ' packets; 1: 'FREQ_TS' packets.*/
>>> +#define TRACE_NOC_CTRL_FREQTYPE                BIT(8)
>>> +DEFINE_CORESIGHT_DEVLIST(trace_noc_devs, "traceNoc");
>>
>> It is good to move the definition into C file.
> will update in next version.
>>
>> Thanks,
>> Leo
>>
>>> +
>>> +/**
>>> + * struct trace_noc_drvdata - specifics associated to a trace noc component
>>> + * @base:      memory mapped base address for this component.
>>> + * @dev:       device node for trace_noc_drvdata.
>>> + * @csdev:     component vitals needed by the framework.
>>> + * @spinlock:  only one at a time pls.
>>> + * @atid:      id for the trace packet.
>>> + * @freqtype:  0: 'FREQ' packets; 1: 'FREQ_TS' packets.
>>> + * @flagtype:  0: 'FLAG' packets; 1: 'FLAG_TS' packets.
>>> + * @freq_req_val:       set frequency values carried by 'FREQ' and 'FREQ_TS' packets.
>>> + */
>>> +struct trace_noc_drvdata {
>>> +       void __iomem            *base;
>>> +       struct device           *dev;
>>> +       struct coresight_device *csdev;
>>> +       spinlock_t              spinlock; /* lock for the drvdata. */
>>> +       u32                     atid;
>>> +       u32                     freq_type;
>>> +       u32                     flag_type;
>>> +       u32                     freq_req_val;
>>> +};
>>> +
>>> +/* freq type */
>>> +enum freq_type {
>>> +       FREQ,
>>> +       FREQ_TS,
>>> +};
>>> +
>>> +/* flag type */
>>> +enum flag_type {
>>> +       FLAG,
>>> +       FLAG_TS,
>>> +};
>>>
>>> --
>>> 2.34.1
>>>
>>> _______________________________________________
>>> CoreSight mailing list -- coresight@lists.linaro.org
>>> To unsubscribe send an email to coresight-leave@lists.linaro.org
> 
> 

Re: [PATCH v2 2/5] coresight: add coresight Trace NOC driver

[Mike Leach]

Hi,

On Wed, 26 Feb 2025 at 11:06, Yuanfang Zhang <quic_yuanfang@quicinc.com> wrote:
>
> Add driver to support Coresight device Trace NOC(Network On Chip).
> Trace NOC is an integration hierarchy which is a replacement of
> Dragonlink configuration. It brings together debug components like
> TPDA, funnel and interconnect Trace Noc.
>
> It sits in the different subsystem of SOC and aggregates the trace
> and transports to QDSS trace bus.
>
> Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com>
> ---
>  drivers/hwtracing/coresight/Kconfig          |  13 ++
>  drivers/hwtracing/coresight/Makefile         |   1 +
>  drivers/hwtracing/coresight/coresight-tnoc.c | 190 +++++++++++++++++++++++++++
>  drivers/hwtracing/coresight/coresight-tnoc.h |  53 ++++++++
>  4 files changed, 257 insertions(+)
>
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index 06f0a7594169c5f03ca5f893b7debd294587de78..6cfd160f09d383ab5f5aa276fa57496a52c8f961 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -247,4 +247,17 @@ config CORESIGHT_DUMMY
>
>           To compile this driver as a module, choose M here: the module will be
>           called coresight-dummy.
> +
> +config CORESIGHT_TNOC
> +       tristate "Coresight Trace Noc driver"
> +       help
> +         This driver provides support for Trace NoC component.
> +         Trace NoC is a interconnect that is used to collect trace from
> +         various subsystems and transport it QDSS trace sink.It sits in
> +         the different tiles of SOC and aggregates the trace local to the
> +         tile and transports it another tile or to QDSS trace sink eventually.
> +
> +         To compile this driver as a module, choose M here: the module will be
> +         called coresight-tnoc.
> +
>  endif
> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> index 4ba478211b318ea5305f9f98dda40a041759f09f..60b729979f19c8f8848c77c290605132dba1a991 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -34,6 +34,7 @@ obj-$(CONFIG_CORESIGHT_SINK_TPIU) += coresight-tpiu.o
>  obj-$(CONFIG_CORESIGHT_SINK_ETBV10) += coresight-etb10.o
>  obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o \
>                                            coresight-replicator.o
> +obj-$(CONFIG_CORESIGHT_TNOC) += coresight-tnoc.o
>  obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x.o
>  coresight-etm3x-y := coresight-etm3x-core.o coresight-etm-cp14.o \
>                      coresight-etm3x-sysfs.o
> diff --git a/drivers/hwtracing/coresight/coresight-tnoc.c b/drivers/hwtracing/coresight/coresight-tnoc.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..fad8e61f05ef25989aba1be342c547f835e8953a
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-tnoc.c
> @@ -0,0 +1,190 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/amba/bus.h>
> +#include <linux/io.h>
> +#include <linux/coresight.h>
> +#include <linux/of.h>
> +
> +#include "coresight-priv.h"
> +#include "coresight-tnoc.h"
> +#include "coresight-trace-id.h"
> +
> +static void trace_noc_enable_hw(struct trace_noc_drvdata *drvdata)
> +{
> +       u32 val;
> +
> +       /* Set ATID */
> +       writel_relaxed(drvdata->atid, drvdata->base + TRACE_NOC_XLD);
> +
> +       /* Config sync CR */
> +       writel_relaxed(0xffff, drvdata->base + TRACE_NOC_SYNCR);
> +
> +       /* Set frequency value */
> +       writel_relaxed(drvdata->freq_req_val, drvdata->base + TRACE_NOC_FREQVAL);
> +
> +       /* Set Ctrl register */
> +       val = readl_relaxed(drvdata->base + TRACE_NOC_CTRL);
> +
> +       if (drvdata->flag_type == FLAG_TS)
> +               val = val | TRACE_NOC_CTRL_FLAGTYPE;
> +       else
> +               val = val & ~TRACE_NOC_CTRL_FLAGTYPE;
> +
> +       if (drvdata->freq_type == FREQ_TS)
> +               val = val | TRACE_NOC_CTRL_FREQTYPE;
> +       else
> +               val = val & ~TRACE_NOC_CTRL_FREQTYPE;
> +
> +       val = val | TRACE_NOC_CTRL_PORTEN;
> +       writel_relaxed(val, drvdata->base + TRACE_NOC_CTRL);
> +
> +       dev_dbg(drvdata->dev, "Trace NOC is enabled\n");
> +}
> +
> +static int trace_noc_enable(struct coresight_device *csdev, struct coresight_connection *inport,
> +                           struct coresight_connection *outport)
> +{
> +       struct trace_noc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> +       spin_lock(&drvdata->spinlock);
> +       if (csdev->refcnt == 0)
> +               trace_noc_enable_hw(drvdata);
> +
> +       csdev->refcnt++;
> +       spin_unlock(&drvdata->spinlock);
> +
> +       return 0;
> +}
> +
> +static void trace_noc_disable_hw(struct trace_noc_drvdata *drvdata)
> +{
> +       writel_relaxed(0x0, drvdata->base + TRACE_NOC_CTRL);
> +       dev_dbg(drvdata->dev, "Trace NOC is disabled\n");
> +}
> +
> +static void trace_noc_disable(struct coresight_device *csdev, struct coresight_connection *inport,
> +                             struct coresight_connection *outport)
> +{
> +       struct trace_noc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> +       spin_lock(&drvdata->spinlock);
> +       if (--csdev->refcnt == 0)
> +               trace_noc_disable_hw(drvdata);
> +
> +       spin_unlock(&drvdata->spinlock);
> +       dev_info(drvdata->dev, "Trace NOC is disabled\n");
> +}
> +
> +static const struct coresight_ops_link trace_noc_link_ops = {
> +       .enable         = trace_noc_enable,
> +       .disable        = trace_noc_disable,
> +};
> +
> +static const struct coresight_ops trace_noc_cs_ops = {
> +       .link_ops       = &trace_noc_link_ops,
> +};
> +
> +static int trace_noc_init_default_data(struct trace_noc_drvdata *drvdata)
> +{
> +       int atid;
> +
> +       atid = coresight_trace_id_get_system_id();
> +       if (atid < 0)
> +               return atid;
> +
> +       drvdata->atid = atid;
> +
> +       drvdata->freq_type = FREQ_TS;
> +       drvdata->flag_type = FLAG;
> +       drvdata->freq_req_val = 0;
> +
> +       return 0;
> +}
> +
> +static int trace_noc_probe(struct amba_device *adev, const struct amba_id *id)
> +{
> +       struct device *dev = &adev->dev;
> +       struct coresight_platform_data *pdata;
> +       struct trace_noc_drvdata *drvdata;
> +       struct coresight_desc desc = { 0 };
> +       int ret;
> +
> +       desc.name = coresight_alloc_device_name(&trace_noc_devs, dev);
> +       if (!desc.name)
> +               return -ENOMEM;
> +       pdata = coresight_get_platform_data(dev);
> +       if (IS_ERR(pdata))
> +               return PTR_ERR(pdata);
> +       adev->dev.platform_data = pdata;
> +
> +       drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> +       if (!drvdata)
> +               return -ENOMEM;
> +
> +       drvdata->dev = &adev->dev;
> +       dev_set_drvdata(dev, drvdata);
> +
> +       drvdata->base = devm_ioremap_resource(dev, &adev->res);
> +       if (!drvdata->base)
> +               return -ENOMEM;
> +
> +       spin_lock_init(&drvdata->spinlock);
> +
> +       ret = trace_noc_init_default_data(drvdata);
> +       if (ret)
> +               return ret;
> +
> +       desc.ops = &trace_noc_cs_ops;
> +       desc.type = CORESIGHT_DEV_TYPE_LINK;
> +       desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_MERG;
> +       desc.pdata = adev->dev.platform_data;
> +       desc.dev = &adev->dev;
> +       desc.access = CSDEV_ACCESS_IOMEM(drvdata->base);
> +       drvdata->csdev = coresight_register(&desc);
> +       if (IS_ERR(drvdata->csdev))
> +               return PTR_ERR(drvdata->csdev);
> +
> +       pm_runtime_put(&adev->dev);
> +
> +       return 0;
> +}
> +
> +static void trace_noc_remove(struct amba_device *adev)
> +{
> +       struct trace_noc_drvdata *drvdata = dev_get_drvdata(&adev->dev);
> +
> +       coresight_trace_id_put_system_id(drvdata->atid);
> +       coresight_unregister(drvdata->csdev);
> +}
> +
> +static struct amba_id trace_noc_ids[] = {
> +       {
> +               .id     = 0x000f0c00,
> +               .mask   = 0x000fff00,
> +       },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(amba, trace_noc_ids);
> +
> +static struct amba_driver trace_noc_driver = {
> +       .drv = {
> +               .name   = "coresight-trace-noc",
> +               .owner  = THIS_MODULE,
> +               .suppress_bind_attrs = true,
> +       },
> +       .probe          = trace_noc_probe,
> +       .remove         = trace_noc_remove,
> +       .id_table       = trace_noc_ids,
> +};
> +
> +module_amba_driver(trace_noc_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Trace NOC driver");
> diff --git a/drivers/hwtracing/coresight/coresight-tnoc.h b/drivers/hwtracing/coresight/coresight-tnoc.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..b6bd1ef659897d8e0994c5e8514e8cbdd16eebd8
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-tnoc.h
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#define TRACE_NOC_CTRL 0x008
> +#define TRACE_NOC_XLD  0x010
> +#define TRACE_NOC_FREQVAL      0x018
> +#define TRACE_NOC_SYNCR        0x020
> +
> +/* Enable generation of output ATB traffic.*/



> +#define TRACE_NOC_CTRL_PORTEN  BIT(0)
> +/* Writing 1 to issue a FREQ or FREQ_TS packet*/
> +#define TRACE_NOC_CTRL_FREQTSREQ       BIT(5)
> +/* Sets the type of issued ATB FLAG packets. 0: 'FLAG' packets; 1: 'FLAG_TS' packets.*/
> +#define TRACE_NOC_CTRL_FLAGTYPE                BIT(7)
> +/* sets the type of issued ATB FREQ packets. 0: 'FREQ' packets; 1: 'FREQ_TS' packets.*/
> +#define TRACE_NOC_CTRL_FREQTYPE                BIT(8)
> +DEFINE_CORESIGHT_DEVLIST(trace_noc_devs, "traceNoc");

Coresight links do not generate their own packets - please explain
what these are.

As far as I am aware, frequency and flag packets are not part of the
ATB specification.

If the output bus for this device is not in fact an ATB bus, then it
should not be referred to as such.

Thanks and regards

 Mike

> +
> +/**
> + * struct trace_noc_drvdata - specifics associated to a trace noc component
> + * @base:      memory mapped base address for this component.
> + * @dev:       device node for trace_noc_drvdata.
> + * @csdev:     component vitals needed by the framework.
> + * @spinlock:  only one at a time pls.
> + * @atid:      id for the trace packet.
> + * @freqtype:  0: 'FREQ' packets; 1: 'FREQ_TS' packets.
> + * @flagtype:  0: 'FLAG' packets; 1: 'FLAG_TS' packets.
> + * @freq_req_val:       set frequency values carried by 'FREQ' and 'FREQ_TS' packets.
> + */
> +struct trace_noc_drvdata {
> +       void __iomem            *base;
> +       struct device           *dev;
> +       struct coresight_device *csdev;
> +       spinlock_t              spinlock; /* lock for the drvdata. */
> +       u32                     atid;
> +       u32                     freq_type;
> +       u32                     flag_type;
> +       u32                     freq_req_val;
> +};
> +
> +/* freq type */
> +enum freq_type {
> +       FREQ,
> +       FREQ_TS,
> +};
> +
> +/* flag type */
> +enum flag_type {
> +       FLAG,
> +       FLAG_TS,
> +};
>
> --
> 2.34.1
>


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

Re: [PATCH v2 2/5] coresight: add coresight Trace NOC driver

[Yuanfang Zhang]

On 4/7/2025 11:47 PM, Mike Leach wrote:
> Hi,
> 
> On Wed, 26 Feb 2025 at 11:06, Yuanfang Zhang <quic_yuanfang@quicinc.com> wrote:
>>
>> Add driver to support Coresight device Trace NOC(Network On Chip).
>> Trace NOC is an integration hierarchy which is a replacement of
>> Dragonlink configuration. It brings together debug components like
>> TPDA, funnel and interconnect Trace Noc.
>>
>> It sits in the different subsystem of SOC and aggregates the trace
>> and transports to QDSS trace bus.
>>
>> Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com>
>> ---
>>  drivers/hwtracing/coresight/Kconfig          |  13 ++
>>  drivers/hwtracing/coresight/Makefile         |   1 +
>>  drivers/hwtracing/coresight/coresight-tnoc.c | 190 +++++++++++++++++++++++++++
>>  drivers/hwtracing/coresight/coresight-tnoc.h |  53 ++++++++
>>  4 files changed, 257 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>> index 06f0a7594169c5f03ca5f893b7debd294587de78..6cfd160f09d383ab5f5aa276fa57496a52c8f961 100644
>> --- a/drivers/hwtracing/coresight/Kconfig
>> +++ b/drivers/hwtracing/coresight/Kconfig
>> @@ -247,4 +247,17 @@ config CORESIGHT_DUMMY
>>
>>           To compile this driver as a module, choose M here: the module will be
>>           called coresight-dummy.
>> +
>> +config CORESIGHT_TNOC
>> +       tristate "Coresight Trace Noc driver"
>> +       help
>> +         This driver provides support for Trace NoC component.
>> +         Trace NoC is a interconnect that is used to collect trace from
>> +         various subsystems and transport it QDSS trace sink.It sits in
>> +         the different tiles of SOC and aggregates the trace local to the
>> +         tile and transports it another tile or to QDSS trace sink eventually.
>> +
>> +         To compile this driver as a module, choose M here: the module will be
>> +         called coresight-tnoc.
>> +
>>  endif
>> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
>> index 4ba478211b318ea5305f9f98dda40a041759f09f..60b729979f19c8f8848c77c290605132dba1a991 100644
>> --- a/drivers/hwtracing/coresight/Makefile
>> +++ b/drivers/hwtracing/coresight/Makefile
>> @@ -34,6 +34,7 @@ obj-$(CONFIG_CORESIGHT_SINK_TPIU) += coresight-tpiu.o
>>  obj-$(CONFIG_CORESIGHT_SINK_ETBV10) += coresight-etb10.o
>>  obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o \
>>                                            coresight-replicator.o
>> +obj-$(CONFIG_CORESIGHT_TNOC) += coresight-tnoc.o
>>  obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x.o
>>  coresight-etm3x-y := coresight-etm3x-core.o coresight-etm-cp14.o \
>>                      coresight-etm3x-sysfs.o
>> diff --git a/drivers/hwtracing/coresight/coresight-tnoc.c b/drivers/hwtracing/coresight/coresight-tnoc.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..fad8e61f05ef25989aba1be342c547f835e8953a
>> --- /dev/null
>> +++ b/drivers/hwtracing/coresight/coresight-tnoc.c
>> @@ -0,0 +1,190 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/amba/bus.h>
>> +#include <linux/io.h>
>> +#include <linux/coresight.h>
>> +#include <linux/of.h>
>> +
>> +#include "coresight-priv.h"
>> +#include "coresight-tnoc.h"
>> +#include "coresight-trace-id.h"
>> +
>> +static void trace_noc_enable_hw(struct trace_noc_drvdata *drvdata)
>> +{
>> +       u32 val;
>> +
>> +       /* Set ATID */
>> +       writel_relaxed(drvdata->atid, drvdata->base + TRACE_NOC_XLD);
>> +
>> +       /* Config sync CR */
>> +       writel_relaxed(0xffff, drvdata->base + TRACE_NOC_SYNCR);
>> +
>> +       /* Set frequency value */
>> +       writel_relaxed(drvdata->freq_req_val, drvdata->base + TRACE_NOC_FREQVAL);
>> +
>> +       /* Set Ctrl register */
>> +       val = readl_relaxed(drvdata->base + TRACE_NOC_CTRL);
>> +
>> +       if (drvdata->flag_type == FLAG_TS)
>> +               val = val | TRACE_NOC_CTRL_FLAGTYPE;
>> +       else
>> +               val = val & ~TRACE_NOC_CTRL_FLAGTYPE;
>> +
>> +       if (drvdata->freq_type == FREQ_TS)
>> +               val = val | TRACE_NOC_CTRL_FREQTYPE;
>> +       else
>> +               val = val & ~TRACE_NOC_CTRL_FREQTYPE;
>> +
>> +       val = val | TRACE_NOC_CTRL_PORTEN;
>> +       writel_relaxed(val, drvdata->base + TRACE_NOC_CTRL);
>> +
>> +       dev_dbg(drvdata->dev, "Trace NOC is enabled\n");
>> +}
>> +
>> +static int trace_noc_enable(struct coresight_device *csdev, struct coresight_connection *inport,
>> +                           struct coresight_connection *outport)
>> +{
>> +       struct trace_noc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +
>> +       spin_lock(&drvdata->spinlock);
>> +       if (csdev->refcnt == 0)
>> +               trace_noc_enable_hw(drvdata);
>> +
>> +       csdev->refcnt++;
>> +       spin_unlock(&drvdata->spinlock);
>> +
>> +       return 0;
>> +}
>> +
>> +static void trace_noc_disable_hw(struct trace_noc_drvdata *drvdata)
>> +{
>> +       writel_relaxed(0x0, drvdata->base + TRACE_NOC_CTRL);
>> +       dev_dbg(drvdata->dev, "Trace NOC is disabled\n");
>> +}
>> +
>> +static void trace_noc_disable(struct coresight_device *csdev, struct coresight_connection *inport,
>> +                             struct coresight_connection *outport)
>> +{
>> +       struct trace_noc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +
>> +       spin_lock(&drvdata->spinlock);
>> +       if (--csdev->refcnt == 0)
>> +               trace_noc_disable_hw(drvdata);
>> +
>> +       spin_unlock(&drvdata->spinlock);
>> +       dev_info(drvdata->dev, "Trace NOC is disabled\n");
>> +}
>> +
>> +static const struct coresight_ops_link trace_noc_link_ops = {
>> +       .enable         = trace_noc_enable,
>> +       .disable        = trace_noc_disable,
>> +};
>> +
>> +static const struct coresight_ops trace_noc_cs_ops = {
>> +       .link_ops       = &trace_noc_link_ops,
>> +};
>> +
>> +static int trace_noc_init_default_data(struct trace_noc_drvdata *drvdata)
>> +{
>> +       int atid;
>> +
>> +       atid = coresight_trace_id_get_system_id();
>> +       if (atid < 0)
>> +               return atid;
>> +
>> +       drvdata->atid = atid;
>> +
>> +       drvdata->freq_type = FREQ_TS;
>> +       drvdata->flag_type = FLAG;
>> +       drvdata->freq_req_val = 0;
>> +
>> +       return 0;
>> +}
>> +
>> +static int trace_noc_probe(struct amba_device *adev, const struct amba_id *id)
>> +{
>> +       struct device *dev = &adev->dev;
>> +       struct coresight_platform_data *pdata;
>> +       struct trace_noc_drvdata *drvdata;
>> +       struct coresight_desc desc = { 0 };
>> +       int ret;
>> +
>> +       desc.name = coresight_alloc_device_name(&trace_noc_devs, dev);
>> +       if (!desc.name)
>> +               return -ENOMEM;
>> +       pdata = coresight_get_platform_data(dev);
>> +       if (IS_ERR(pdata))
>> +               return PTR_ERR(pdata);
>> +       adev->dev.platform_data = pdata;
>> +
>> +       drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>> +       if (!drvdata)
>> +               return -ENOMEM;
>> +
>> +       drvdata->dev = &adev->dev;
>> +       dev_set_drvdata(dev, drvdata);
>> +
>> +       drvdata->base = devm_ioremap_resource(dev, &adev->res);
>> +       if (!drvdata->base)
>> +               return -ENOMEM;
>> +
>> +       spin_lock_init(&drvdata->spinlock);
>> +
>> +       ret = trace_noc_init_default_data(drvdata);
>> +       if (ret)
>> +               return ret;
>> +
>> +       desc.ops = &trace_noc_cs_ops;
>> +       desc.type = CORESIGHT_DEV_TYPE_LINK;
>> +       desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_MERG;
>> +       desc.pdata = adev->dev.platform_data;
>> +       desc.dev = &adev->dev;
>> +       desc.access = CSDEV_ACCESS_IOMEM(drvdata->base);
>> +       drvdata->csdev = coresight_register(&desc);
>> +       if (IS_ERR(drvdata->csdev))
>> +               return PTR_ERR(drvdata->csdev);
>> +
>> +       pm_runtime_put(&adev->dev);
>> +
>> +       return 0;
>> +}
>> +
>> +static void trace_noc_remove(struct amba_device *adev)
>> +{
>> +       struct trace_noc_drvdata *drvdata = dev_get_drvdata(&adev->dev);
>> +
>> +       coresight_trace_id_put_system_id(drvdata->atid);
>> +       coresight_unregister(drvdata->csdev);
>> +}
>> +
>> +static struct amba_id trace_noc_ids[] = {
>> +       {
>> +               .id     = 0x000f0c00,
>> +               .mask   = 0x000fff00,
>> +       },
>> +       {},
>> +};
>> +MODULE_DEVICE_TABLE(amba, trace_noc_ids);
>> +
>> +static struct amba_driver trace_noc_driver = {
>> +       .drv = {
>> +               .name   = "coresight-trace-noc",
>> +               .owner  = THIS_MODULE,
>> +               .suppress_bind_attrs = true,
>> +       },
>> +       .probe          = trace_noc_probe,
>> +       .remove         = trace_noc_remove,
>> +       .id_table       = trace_noc_ids,
>> +};
>> +
>> +module_amba_driver(trace_noc_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Trace NOC driver");
>> diff --git a/drivers/hwtracing/coresight/coresight-tnoc.h b/drivers/hwtracing/coresight/coresight-tnoc.h
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..b6bd1ef659897d8e0994c5e8514e8cbdd16eebd8
>> --- /dev/null
>> +++ b/drivers/hwtracing/coresight/coresight-tnoc.h
>> @@ -0,0 +1,53 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#define TRACE_NOC_CTRL 0x008
>> +#define TRACE_NOC_XLD  0x010
>> +#define TRACE_NOC_FREQVAL      0x018
>> +#define TRACE_NOC_SYNCR        0x020
>> +
>> +/* Enable generation of output ATB traffic.*/
> 
> 
> 
>> +#define TRACE_NOC_CTRL_PORTEN  BIT(0)
>> +/* Writing 1 to issue a FREQ or FREQ_TS packet*/
>> +#define TRACE_NOC_CTRL_FREQTSREQ       BIT(5)
>> +/* Sets the type of issued ATB FLAG packets. 0: 'FLAG' packets; 1: 'FLAG_TS' packets.*/
>> +#define TRACE_NOC_CTRL_FLAGTYPE                BIT(7)
>> +/* sets the type of issued ATB FREQ packets. 0: 'FREQ' packets; 1: 'FREQ_TS' packets.*/
>> +#define TRACE_NOC_CTRL_FREQTYPE                BIT(8)
>> +DEFINE_CORESIGHT_DEVLIST(trace_noc_devs, "traceNoc");
> 
> Coresight links do not generate their own packets - please explain
> what these are.
> 
> As far as I am aware, frequency and flag packets are not part of the
> ATB specification.
> 
> If the output bus for this device is not in fact an ATB bus, then it
> should not be referred to as such.
> 
> Thanks and regards
> 
>  Mike
> 
Hi Mike

Frequency and flag packets are STPV2 packet formats, they are generated by FREQ,
FLAG & Sysnc Request Packet interface which is a component of Trace NOC.
 
TNOC is not just a link, it is an integrator that includes TPDA interface,
FREQ, FLAG & Sysnc Request Packet interface, etc. The data generated by these
interfaces will be packaged in STPV2 format and then output through ATB bus.

thanks,
Yuanfang.
>> +
>> +/**
>> + * struct trace_noc_drvdata - specifics associated to a trace noc component
>> + * @base:      memory mapped base address for this component.
>> + * @dev:       device node for trace_noc_drvdata.
>> + * @csdev:     component vitals needed by the framework.
>> + * @spinlock:  only one at a time pls.
>> + * @atid:      id for the trace packet.
>> + * @freqtype:  0: 'FREQ' packets; 1: 'FREQ_TS' packets.
>> + * @flagtype:  0: 'FLAG' packets; 1: 'FLAG_TS' packets.
>> + * @freq_req_val:       set frequency values carried by 'FREQ' and 'FREQ_TS' packets.
>> + */
>> +struct trace_noc_drvdata {
>> +       void __iomem            *base;
>> +       struct device           *dev;
>> +       struct coresight_device *csdev;
>> +       spinlock_t              spinlock; /* lock for the drvdata. */
>> +       u32                     atid;
>> +       u32                     freq_type;
>> +       u32                     flag_type;
>> +       u32                     freq_req_val;
>> +};
>> +
>> +/* freq type */
>> +enum freq_type {
>> +       FREQ,
>> +       FREQ_TS,
>> +};
>> +
>> +/* flag type */
>> +enum flag_type {
>> +       FLAG,
>> +       FLAG_TS,
>> +};
>>
>> --
>> 2.34.1
>>
> 
> 

[PATCH v2 3/5] coresight-tnoc: add nodes to configure flush

[Yuanfang Zhang]

Two nodes for configure flush are added here:
1. flush_req: write 1 to initiates a flush sequence.

2. flush_state: read this node to get flush status. 0: sequence in
progress; 1: sequence has been completed.

Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com>
---
 drivers/hwtracing/coresight/coresight-tnoc.c | 73 ++++++++++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tnoc.h |  4 ++
 2 files changed, 77 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tnoc.c b/drivers/hwtracing/coresight/coresight-tnoc.c
index fad8e61f05ef25989aba1be342c547f835e8953a..20231f28ddcb6a60d9b3c1ca3e0ca4d731dac39c 100644
--- a/drivers/hwtracing/coresight/coresight-tnoc.c
+++ b/drivers/hwtracing/coresight/coresight-tnoc.c
@@ -16,6 +16,78 @@
 #include "coresight-tnoc.h"
 #include "coresight-trace-id.h"
 
+static ssize_t flush_req_store(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf,
+			       size_t size)
+{
+	struct trace_noc_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	struct coresight_device	*csdev = drvdata->csdev;
+	unsigned long val;
+	u32 reg;
+
+	if (kstrtoul(buf, 10, &val))
+		return -EINVAL;
+
+	if (val != 1)
+		return -EINVAL;
+
+	spin_lock(&drvdata->spinlock);
+	if (csdev->refcnt == 0) {
+		spin_unlock(&drvdata->spinlock);
+		return -EPERM;
+	}
+
+	reg = readl_relaxed(drvdata->base + TRACE_NOC_CTRL);
+	reg = reg | TRACE_NOC_CTRL_FLUSHREQ;
+	writel_relaxed(reg, drvdata->base + TRACE_NOC_CTRL);
+
+	spin_unlock(&drvdata->spinlock);
+
+	return size;
+}
+static DEVICE_ATTR_WO(flush_req);
+
+/*
+ * flush-sequence status:
+ * value 0: sequence in progress;
+ * value 1: sequence has been completed.
+ */
+static ssize_t flush_status_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct trace_noc_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	struct coresight_device	*csdev = drvdata->csdev;
+	u32 val;
+
+	spin_lock(&drvdata->spinlock);
+	if (csdev->refcnt == 0) {
+		spin_unlock(&drvdata->spinlock);
+		return -EPERM;
+	}
+
+	val = readl_relaxed(drvdata->base + TRACE_NOC_CTRL);
+	spin_unlock(&drvdata->spinlock);
+	return sysfs_emit(buf, "%lu\n", BMVAL(val, 2, 2));
+}
+static DEVICE_ATTR_RO(flush_status);
+
+static struct attribute *trace_noc_attrs[] = {
+	&dev_attr_flush_req.attr,
+	&dev_attr_flush_status.attr,
+	NULL,
+};
+
+static struct attribute_group trace_noc_attr_grp = {
+	.attrs = trace_noc_attrs,
+};
+
+static const struct attribute_group *trace_noc_attr_grps[] = {
+	&trace_noc_attr_grp,
+	NULL,
+};
+
 static void trace_noc_enable_hw(struct trace_noc_drvdata *drvdata)
 {
 	u32 val;
@@ -142,6 +214,7 @@ static int trace_noc_probe(struct amba_device *adev, const struct amba_id *id)
 		return ret;
 
 	desc.ops = &trace_noc_cs_ops;
+	desc.groups = trace_noc_attr_grps;
 	desc.type = CORESIGHT_DEV_TYPE_LINK;
 	desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_MERG;
 	desc.pdata = adev->dev.platform_data;
diff --git a/drivers/hwtracing/coresight/coresight-tnoc.h b/drivers/hwtracing/coresight/coresight-tnoc.h
index b6bd1ef659897d8e0994c5e8514e8cbdd16eebd8..d0fe8f52709ff4147d66dbf90987595012cfaa4e 100644
--- a/drivers/hwtracing/coresight/coresight-tnoc.h
+++ b/drivers/hwtracing/coresight/coresight-tnoc.h
@@ -10,6 +10,10 @@
 
 /* Enable generation of output ATB traffic.*/
 #define TRACE_NOC_CTRL_PORTEN	BIT(0)
+/* Writing 1 to initiate a flush sequence.*/
+#define TRACE_NOC_CTRL_FLUSHREQ	BIT(1)
+/* 0: sequence in progress; 1: sequence has been completed.*/
+#define TRACE_NOC_CTRL_FLUSHSTATUS	BIT(2)
 /* Writing 1 to issue a FREQ or FREQ_TS packet*/
 #define TRACE_NOC_CTRL_FREQTSREQ	BIT(5)
 /* Sets the type of issued ATB FLAG packets. 0: 'FLAG' packets; 1: 'FLAG_TS' packets.*/

-- 
2.34.1

Re: [PATCH v2 3/5] coresight-tnoc: add nodes to configure flush

[Leo Yan]

On Wed, Feb 26, 2025 at 07:05:52PM +0800, Yuanfang Zhang wrote:
> 
> Two nodes for configure flush are added here:
> 1. flush_req: write 1 to initiates a flush sequence.
> 
> 2. flush_state: read this node to get flush status. 0: sequence in
> progress; 1: sequence has been completed.
> 
> Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com>
> ---
>  drivers/hwtracing/coresight/coresight-tnoc.c | 73 ++++++++++++++++++++++++++++
>  drivers/hwtracing/coresight/coresight-tnoc.h |  4 ++
>  2 files changed, 77 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tnoc.c b/drivers/hwtracing/coresight/coresight-tnoc.c
> index fad8e61f05ef25989aba1be342c547f835e8953a..20231f28ddcb6a60d9b3c1ca3e0ca4d731dac39c 100644
> --- a/drivers/hwtracing/coresight/coresight-tnoc.c
> +++ b/drivers/hwtracing/coresight/coresight-tnoc.c
> @@ -16,6 +16,78 @@
>  #include "coresight-tnoc.h"
>  #include "coresight-trace-id.h"
> 
> +static ssize_t flush_req_store(struct device *dev,
> +                              struct device_attribute *attr,
> +                              const char *buf,
> +                              size_t size)
> +{
> +       struct trace_noc_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +       struct coresight_device *csdev = drvdata->csdev;
> +       unsigned long val;
> +       u32 reg;
> +
> +       if (kstrtoul(buf, 10, &val))
> +               return -EINVAL;
> +
> +       if (val != 1)
> +               return -EINVAL;
> +
> +       spin_lock(&drvdata->spinlock);
> +       if (csdev->refcnt == 0) {
> +               spin_unlock(&drvdata->spinlock);
> +               return -EPERM;
> +       }
> +
> +       reg = readl_relaxed(drvdata->base + TRACE_NOC_CTRL);
> +       reg = reg | TRACE_NOC_CTRL_FLUSHREQ;
> +       writel_relaxed(reg, drvdata->base + TRACE_NOC_CTRL);

How can userspace determine when to trigger a flush?

Generally, a driver kicks off a flush operation for a hardware before
reading data from buffer or when disable a link path.  I don't know the
hardware mechanism of TNOC, but seems to me, it does not make sense to
let the userspace to trigger a hardware flush, given the userspace has
no knowledge for device's state.

Furthermore, based on my understanding for patch 02 and 03, the working
flow is also concerned me.  IIUC, you want to use the driver to create
a linkage and then use userspace program to poll state and trigger
flushing.  Could you explain why use this way for managing the device?

Thanks,
Leo

> +
> +       spin_unlock(&drvdata->spinlock);
> +
> +       return size;
> +}
> +static DEVICE_ATTR_WO(flush_req);
> +
> +/*
> + * flush-sequence status:
> + * value 0: sequence in progress;
> + * value 1: sequence has been completed.
> + */
> +static ssize_t flush_status_show(struct device *dev,
> +                                struct device_attribute *attr,
> +                                char *buf)
> +{
> +       struct trace_noc_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +       struct coresight_device *csdev = drvdata->csdev;
> +       u32 val;
> +
> +       spin_lock(&drvdata->spinlock);
> +       if (csdev->refcnt == 0) {
> +               spin_unlock(&drvdata->spinlock);
> +               return -EPERM;
> +       }
> +
> +       val = readl_relaxed(drvdata->base + TRACE_NOC_CTRL);
> +       spin_unlock(&drvdata->spinlock);
> +       return sysfs_emit(buf, "%lu\n", BMVAL(val, 2, 2));
> +}
> +static DEVICE_ATTR_RO(flush_status);
> +
> +static struct attribute *trace_noc_attrs[] = {
> +       &dev_attr_flush_req.attr,
> +       &dev_attr_flush_status.attr,
> +       NULL,
> +};
> +
> +static struct attribute_group trace_noc_attr_grp = {
> +       .attrs = trace_noc_attrs,
> +};
> +
> +static const struct attribute_group *trace_noc_attr_grps[] = {
> +       &trace_noc_attr_grp,
> +       NULL,
> +};
> +
>  static void trace_noc_enable_hw(struct trace_noc_drvdata *drvdata)
>  {
>         u32 val;
> @@ -142,6 +214,7 @@ static int trace_noc_probe(struct amba_device *adev, const struct amba_id *id)
>                 return ret;
> 
>         desc.ops = &trace_noc_cs_ops;
> +       desc.groups = trace_noc_attr_grps;
>         desc.type = CORESIGHT_DEV_TYPE_LINK;
>         desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_MERG;
>         desc.pdata = adev->dev.platform_data;
> diff --git a/drivers/hwtracing/coresight/coresight-tnoc.h b/drivers/hwtracing/coresight/coresight-tnoc.h
> index b6bd1ef659897d8e0994c5e8514e8cbdd16eebd8..d0fe8f52709ff4147d66dbf90987595012cfaa4e 100644
> --- a/drivers/hwtracing/coresight/coresight-tnoc.h
> +++ b/drivers/hwtracing/coresight/coresight-tnoc.h
> @@ -10,6 +10,10 @@
> 
>  /* Enable generation of output ATB traffic.*/
>  #define TRACE_NOC_CTRL_PORTEN  BIT(0)
> +/* Writing 1 to initiate a flush sequence.*/
> +#define TRACE_NOC_CTRL_FLUSHREQ        BIT(1)
> +/* 0: sequence in progress; 1: sequence has been completed.*/
> +#define TRACE_NOC_CTRL_FLUSHSTATUS     BIT(2)
>  /* Writing 1 to issue a FREQ or FREQ_TS packet*/
>  #define TRACE_NOC_CTRL_FREQTSREQ       BIT(5)
>  /* Sets the type of issued ATB FLAG packets. 0: 'FLAG' packets; 1: 'FLAG_TS' packets.*/
> 
> --
> 2.34.1
> 
> _______________________________________________
> CoreSight mailing list -- coresight@lists.linaro.org
> To unsubscribe send an email to coresight-leave@lists.linaro.org

Re: [PATCH v2 3/5] coresight-tnoc: add nodes to configure flush

[Yuanfang Zhang]

On 2/28/2025 12:23 AM, Leo Yan wrote:
> On Wed, Feb 26, 2025 at 07:05:52PM +0800, Yuanfang Zhang wrote:
>>
>> Two nodes for configure flush are added here:
>> 1. flush_req: write 1 to initiates a flush sequence.
>>
>> 2. flush_state: read this node to get flush status. 0: sequence in
>> progress; 1: sequence has been completed.
>>
>> Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com>
>> ---
>>  drivers/hwtracing/coresight/coresight-tnoc.c | 73 ++++++++++++++++++++++++++++
>>  drivers/hwtracing/coresight/coresight-tnoc.h |  4 ++
>>  2 files changed, 77 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tnoc.c b/drivers/hwtracing/coresight/coresight-tnoc.c
>> index fad8e61f05ef25989aba1be342c547f835e8953a..20231f28ddcb6a60d9b3c1ca3e0ca4d731dac39c 100644
>> --- a/drivers/hwtracing/coresight/coresight-tnoc.c
>> +++ b/drivers/hwtracing/coresight/coresight-tnoc.c
>> @@ -16,6 +16,78 @@
>>  #include "coresight-tnoc.h"
>>  #include "coresight-trace-id.h"
>>
>> +static ssize_t flush_req_store(struct device *dev,
>> +                              struct device_attribute *attr,
>> +                              const char *buf,
>> +                              size_t size)
>> +{
>> +       struct trace_noc_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +       struct coresight_device *csdev = drvdata->csdev;
>> +       unsigned long val;
>> +       u32 reg;
>> +
>> +       if (kstrtoul(buf, 10, &val))
>> +               return -EINVAL;
>> +
>> +       if (val != 1)
>> +               return -EINVAL;
>> +
>> +       spin_lock(&drvdata->spinlock);
>> +       if (csdev->refcnt == 0) {
>> +               spin_unlock(&drvdata->spinlock);
>> +               return -EPERM;
>> +       }
>> +
>> +       reg = readl_relaxed(drvdata->base + TRACE_NOC_CTRL);
>> +       reg = reg | TRACE_NOC_CTRL_FLUSHREQ;
>> +       writel_relaxed(reg, drvdata->base + TRACE_NOC_CTRL);
> 
> How can userspace determine when to trigger a flush?
It can be triggered under any circumstances.
> 
> Generally, a driver kicks off a flush operation for a hardware before
> reading data from buffer or when disable a link path.  I don't know the
> hardware mechanism of TNOC, but seems to me, it does not make sense to
> let the userspace to trigger a hardware flush, given the userspace has
> no knowledge for device's state.
TNOC supports the aforementioned flush operation, and it also adds this
flush functionality, allowing users to set the flush themselves.
> 
> Furthermore, based on my understanding for patch 02 and 03, the working
> flow is also concerned me.  IIUC, you want to use the driver to create
> a linkage and then use userspace program to poll state and trigger
> flushing.  Could you explain why use this way for managing the device?
> 
TNOC support flush just like other links. This interface simply provides
customers with an additional option to trigger the flush.

> Thanks,
> Leo
> 
>> +
>> +       spin_unlock(&drvdata->spinlock);
>> +
>> +       return size;
>> +}
>> +static DEVICE_ATTR_WO(flush_req);
>> +
>> +/*
>> + * flush-sequence status:
>> + * value 0: sequence in progress;
>> + * value 1: sequence has been completed.
>> + */
>> +static ssize_t flush_status_show(struct device *dev,
>> +                                struct device_attribute *attr,
>> +                                char *buf)
>> +{
>> +       struct trace_noc_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +       struct coresight_device *csdev = drvdata->csdev;
>> +       u32 val;
>> +
>> +       spin_lock(&drvdata->spinlock);
>> +       if (csdev->refcnt == 0) {
>> +               spin_unlock(&drvdata->spinlock);
>> +               return -EPERM;
>> +       }
>> +
>> +       val = readl_relaxed(drvdata->base + TRACE_NOC_CTRL);
>> +       spin_unlock(&drvdata->spinlock);
>> +       return sysfs_emit(buf, "%lu\n", BMVAL(val, 2, 2));
>> +}
>> +static DEVICE_ATTR_RO(flush_status);
>> +
>> +static struct attribute *trace_noc_attrs[] = {
>> +       &dev_attr_flush_req.attr,
>> +       &dev_attr_flush_status.attr,
>> +       NULL,
>> +};
>> +
>> +static struct attribute_group trace_noc_attr_grp = {
>> +       .attrs = trace_noc_attrs,
>> +};
>> +
>> +static const struct attribute_group *trace_noc_attr_grps[] = {
>> +       &trace_noc_attr_grp,
>> +       NULL,
>> +};
>> +
>>  static void trace_noc_enable_hw(struct trace_noc_drvdata *drvdata)
>>  {
>>         u32 val;
>> @@ -142,6 +214,7 @@ static int trace_noc_probe(struct amba_device *adev, const struct amba_id *id)
>>                 return ret;
>>
>>         desc.ops = &trace_noc_cs_ops;
>> +       desc.groups = trace_noc_attr_grps;
>>         desc.type = CORESIGHT_DEV_TYPE_LINK;
>>         desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_MERG;
>>         desc.pdata = adev->dev.platform_data;
>> diff --git a/drivers/hwtracing/coresight/coresight-tnoc.h b/drivers/hwtracing/coresight/coresight-tnoc.h
>> index b6bd1ef659897d8e0994c5e8514e8cbdd16eebd8..d0fe8f52709ff4147d66dbf90987595012cfaa4e 100644
>> --- a/drivers/hwtracing/coresight/coresight-tnoc.h
>> +++ b/drivers/hwtracing/coresight/coresight-tnoc.h
>> @@ -10,6 +10,10 @@
>>
>>  /* Enable generation of output ATB traffic.*/
>>  #define TRACE_NOC_CTRL_PORTEN  BIT(0)
>> +/* Writing 1 to initiate a flush sequence.*/
>> +#define TRACE_NOC_CTRL_FLUSHREQ        BIT(1)
>> +/* 0: sequence in progress; 1: sequence has been completed.*/
>> +#define TRACE_NOC_CTRL_FLUSHSTATUS     BIT(2)
>>  /* Writing 1 to issue a FREQ or FREQ_TS packet*/
>>  #define TRACE_NOC_CTRL_FREQTSREQ       BIT(5)
>>  /* Sets the type of issued ATB FLAG packets. 0: 'FLAG' packets; 1: 'FLAG_TS' packets.*/
>>
>> --
>> 2.34.1
>>
>> _______________________________________________
>> CoreSight mailing list -- coresight@lists.linaro.org
>> To unsubscribe send an email to coresight-leave@lists.linaro.org

Re: [PATCH v2 3/5] coresight-tnoc: add nodes to configure flush

[Leo Yan]

On Thu, Mar 06, 2025 at 04:39:27PM +0800, Yuanfang Zhang wrote:

[...]

> >> +static ssize_t flush_req_store(struct device *dev,
> >> +                              struct device_attribute *attr,
> >> +                              const char *buf,
> >> +                              size_t size)
> >> +{

...

> >> +       reg = readl_relaxed(drvdata->base + TRACE_NOC_CTRL);
> >> +       reg = reg | TRACE_NOC_CTRL_FLUSHREQ;
> >> +       writel_relaxed(reg, drvdata->base + TRACE_NOC_CTRL);
> > 
> > How can userspace determine when to trigger a flush?
> It can be triggered under any circumstances.
> > 
> > Generally, a driver kicks off a flush operation for a hardware before
> > reading data from buffer or when disable a link path.  I don't know the
> > hardware mechanism of TNOC, but seems to me, it does not make sense to
> > let the userspace to trigger a hardware flush, given the userspace has
> > no knowledge for device's state.
>
> TNOC supports the aforementioned flush operation, and it also adds this
> flush functionality, allowing users to set the flush themselves.

I am still not convinced for providing knobs to allow userspace to
directly control hardware.

A low level driver should have sufficient information to know when and
how it triggers a flush.  E.g., CoreSight ETF (coresight-tmc-etf.c) can
act as a link, in this case, it calls the tmc_flush_and_stop() function
to flush its buffer when it is stopped.  A flushing is triggered when a
session is terminated (either is a perf session or a Sysfs session).

Why not TNOC driver do the flushing same as other drivers?  It can flush
the data before a hardware link is to be disabled.  I don't think flush
operations are required at any time.

Seems to me, exposing APIs to userspace for flushing operations also
will introduce potential security risk.  A malicious software might
attack system with triggering tons of flushing in short time.

> > Furthermore, based on my understanding for patch 02 and 03, the working
> > flow is also concerned me.  IIUC, you want to use the driver to create
> > a linkage and then use userspace program to poll state and trigger
> > flushing.  Could you explain why use this way for managing the device?
> > 
> TNOC support flush just like other links. This interface simply provides
> customers with an additional option to trigger the flush.

This is not true for Arm CoreSight components.  My understanding is Arm
CoreSight drivers never provides an API to userspace to manually trigger
flush operations.

Thanks,
Leo

[PATCH v2 4/5] coresight-tnoc: add node to configure flag type

[Yuanfang Zhang]

flag_type：used to set the type of issued ATB FLAG packets.
0: 'FLAG' packets; 1: 'FLAG_TS' packets.

Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com>
---
 drivers/hwtracing/coresight/coresight-tnoc.c | 42 +++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-tnoc.c b/drivers/hwtracing/coresight/coresight-tnoc.c
index 20231f28ddcb6a60d9b3c1ca3e0ca4d731dac39c..ad973749250644760adc4dfd855240026d0a744c 100644
--- a/drivers/hwtracing/coresight/coresight-tnoc.c
+++ b/drivers/hwtracing/coresight/coresight-tnoc.c
@@ -26,7 +26,7 @@ static ssize_t flush_req_store(struct device *dev,
 	unsigned long val;
 	u32 reg;
 
-	if (kstrtoul(buf, 10, &val))
+	if (kstrtoul(buf, 0, &val))
 		return -EINVAL;
 
 	if (val != 1)
@@ -73,9 +73,49 @@ static ssize_t flush_status_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(flush_status);
 
+/*
+ * Sets the type of issued ATB FLAG packets:
+ * 0: 'FLAG' packets;
+ * 1: 'FLAG_TS' packets.
+ */
+static ssize_t flag_type_store(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf,
+			       size_t size)
+{
+	struct trace_noc_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+
+	if (kstrtoul(buf, 10, &val))
+		return -EINVAL;
+
+	if (val != 1 && val != 0)
+		return -EINVAL;
+
+	spin_lock(&drvdata->spinlock);
+	if (val)
+		drvdata->flag_type = FLAG_TS;
+	else
+		drvdata->flag_type = FLAG;
+	spin_unlock(&drvdata->spinlock);
+
+	return size;
+}
+
+static ssize_t flag_type_show(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct trace_noc_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	return sysfs_emit(buf, "%u\n", drvdata->flag_type);
+}
+static DEVICE_ATTR_RW(flag_type);
+
 static struct attribute *trace_noc_attrs[] = {
 	&dev_attr_flush_req.attr,
 	&dev_attr_flush_status.attr,
+	&dev_attr_flag_type.attr,
 	NULL,
 };
 

-- 
2.34.1

[PATCH v2 5/5] coresight-tnoc: add nodes to configure freq packet

[Yuanfang Zhang]

Three nodes for freq packet config are added here:

1. freq_type: used to set the type of issued ATB FREQ packets.
0: 'FREQ' packets; 1: 'FREQ_TS' packets.

2. freq_req_val: used to set frequency values carried by 'FREQ'
and 'FREQ_TS' packets.

3. freq_ts_req: writing '1' to issue a 'FREQ' or 'FREQ_TS' packet.

Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com>
---
 drivers/hwtracing/coresight/coresight-tnoc.c | 97 ++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tnoc.c b/drivers/hwtracing/coresight/coresight-tnoc.c
index ad973749250644760adc4dfd855240026d0a744c..24b1add4c921866b944d756e563d50b4172d583a 100644
--- a/drivers/hwtracing/coresight/coresight-tnoc.c
+++ b/drivers/hwtracing/coresight/coresight-tnoc.c
@@ -112,10 +112,107 @@ static ssize_t flag_type_show(struct device *dev,
 }
 static DEVICE_ATTR_RW(flag_type);
 
+static ssize_t freq_type_show(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct trace_noc_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	return sysfs_emit(buf, "%u\n", drvdata->freq_type);
+}
+
+static ssize_t freq_type_store(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf,
+			       size_t size)
+{
+	struct trace_noc_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+
+	if (kstrtoul(buf, 0, &val))
+		return -EINVAL;
+
+	if (val != 1 && val != 0)
+		return -EINVAL;
+
+	spin_lock(&drvdata->spinlock);
+	if (val)
+		drvdata->freq_type = FREQ_TS;
+	else
+		drvdata->freq_type = FREQ;
+	spin_unlock(&drvdata->spinlock);
+
+	return size;
+}
+static DEVICE_ATTR_RW(freq_type);
+
+static ssize_t freq_req_val_show(struct device *dev,
+				 struct device_attribute *attr,
+				char *buf)
+{
+	struct trace_noc_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	return sysfs_emit(buf, "%u\n", drvdata->freq_req_val);
+}
+
+static ssize_t freq_req_val_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf,
+				  size_t size)
+{
+	struct trace_noc_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+
+	if (kstrtoul(buf, 0, &val))
+		return -EINVAL;
+
+	if (val) {
+		spin_lock(&drvdata->spinlock);
+		drvdata->freq_req_val = val;
+		spin_unlock(&drvdata->spinlock);
+	}
+
+	return size;
+}
+static DEVICE_ATTR_RW(freq_req_val);
+
+static ssize_t freq_ts_req_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf,
+				 size_t size)
+{
+	struct trace_noc_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	struct coresight_device	*csdev = drvdata->csdev;
+	unsigned long val;
+	u32 reg;
+
+	if (kstrtoul(buf, 0, &val))
+		return -EINVAL;
+
+	spin_lock(&drvdata->spinlock);
+	if (csdev->refcnt == 0) {
+		spin_unlock(&drvdata->spinlock);
+		return -EPERM;
+	}
+
+	if (val) {
+		reg = readl_relaxed(drvdata->base + TRACE_NOC_CTRL);
+		reg = reg | TRACE_NOC_CTRL_FREQTSREQ;
+		writel_relaxed(reg, drvdata->base + TRACE_NOC_CTRL);
+	}
+	spin_unlock(&drvdata->spinlock);
+
+	return size;
+}
+static DEVICE_ATTR_WO(freq_ts_req);
+
 static struct attribute *trace_noc_attrs[] = {
 	&dev_attr_flush_req.attr,
 	&dev_attr_flush_status.attr,
 	&dev_attr_flag_type.attr,
+	&dev_attr_freq_type.attr,
+	&dev_attr_freq_req_val.attr,
+	&dev_attr_freq_ts_req.attr,
 	NULL,
 };
 

-- 
2.34.1

Re: [PATCH v2 5/5] coresight-tnoc: add nodes to configure freq packet

[Krzysztof Kozlowski]

On 26/02/2025 12:05, Yuanfang Zhang wrote:
> Three nodes for freq packet config are added here:
> 
> 1. freq_type: used to set the type of issued ATB FREQ packets.
> 0: 'FREQ' packets; 1: 'FREQ_TS' packets.
> 
> 2. freq_req_val: used to set frequency values carried by 'FREQ'
> and 'FREQ_TS' packets.
> 
> 3. freq_ts_req: writing '1' to issue a 'FREQ' or 'FREQ_TS' packet.
> 
> Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com>
> ---
>  drivers/hwtracing/coresight/coresight-tnoc.c | 97 ++++++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tnoc.c b/drivers/hwtracing/coresight/coresight-tnoc.c
> index ad973749250644760adc4dfd855240026d0a744c..24b1add4c921866b944d756e563d50b4172d583a 100644
> --- a/drivers/hwtracing/coresight/coresight-tnoc.c
> +++ b/drivers/hwtracing/coresight/coresight-tnoc.c
> @@ -112,10 +112,107 @@ static ssize_t flag_type_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(flag_type);
>  
> +static ssize_t freq_type_show(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct trace_noc_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +
> +	return sysfs_emit(buf, "%u\n", drvdata->freq_type);
> +}
> +
> +static ssize_t freq_type_store(struct device *dev,
> +			       struct device_attribute *attr,
> +			       const char *buf,
> +			       size_t size)

No improvements. You got here comments and you got also later my
complain that yo missed comments already.

You keep ignoring received feedback and that's a no go for me.

Maybe process needs to be improved, so reach to your colleagues and
learn how to interact with upstream. There is very comprehensive
internal guideline in Qualcomm, so follow it carefully.

I expect still to respond to my original comment or implement it fully.

Best regards,
Krzysztof