[PATCH v6 0/2] coresight: Add Coresight Trace Network On Chip driver

[PATCH v6 0/2] coresight: Add Coresight Trace Network On Chip driver

[Yuanfang Zhang]

The Trace Network On Chip (TNOC) is an integration hierarchy which is a
hardware component that integrates the functionalities of TPDA and
funnels. It collects trace from subsystems and transfers it to coresight
sink.

In addition to the generic TNOC mentioned above, there is also a special type
of TNOC called Interconnect TNOC. Unlike the generic TNOC, the Interconnect
TNOC doesn't need ATID. Its primary function is to connect the source of
subsystems to the Aggregator TNOC. Its driver is different from this patch and
will describe it and upstream its driver separately.

Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com>
---
Changes in v6:
- Add a newline after return statements.
- Use 'x &= foo' to replace 'x = x & foo'.
- Use 'x |= foo' to replace 'x = x | foo'.
- Link to v5: https://lore.kernel.org/r/20250512-trace-noc-v5-0-f2ef070baee5@quicinc.com

Changes in v5:
- update cover-letter to describe the Interconnect TNOC.
- Link to v4: https://lore.kernel.org/r/20250415-trace-noc-v4-0-979938fedfd8@quicinc.com

Changes in v4:
- Fix dt_binding warning.
- update mask of trace_noc amba_id.
- Modify driver comments.
- rename TRACE_NOC_SYN_VAL to TRACE_NOC_SYNC_INTERVAL.
- Link to v3: https://lore.kernel.org/r/20250411-trace-noc-v3-0-1f19ddf7699b@quicinc.com

Changes in v3:
- Remove unnecessary sysfs nodes.
- update commit messages.
- Use 'writel' instead of 'write_relaxed' when writing to the register for the last time.
- Add trace_id ops.
- Link to v2: https://lore.kernel.org/r/20250226-trace-noc-driver-v2-0-8afc6584afc5@quicinc.com

Changes in v2:
- Modified the format of DT binging file.
- Fix compile warnings.
- Link to v1: https://lore.kernel.org/r/46643089-b88d-49dc-be05-7bf0bb21f847@quicinc.com

---
Yuanfang Zhang (2):
      dt-bindings: arm: Add device Trace Network On Chip definition
      coresight: add coresight Trace Network On Chip driver

 .../bindings/arm/qcom,coresight-tnoc.yaml          | 111 ++++++++++++
 drivers/hwtracing/coresight/Kconfig                |  13 ++
 drivers/hwtracing/coresight/Makefile               |   1 +
 drivers/hwtracing/coresight/coresight-tnoc.c       | 192 +++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tnoc.h       |  34 ++++
 5 files changed, 351 insertions(+)
---
base-commit: a2cc6ff5ec8f91bc463fd3b0c26b61166a07eb11
change-id: 20250403-trace-noc-f8286b30408e

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

[PATCH v6 1/2] dt-bindings: arm: Add device Trace Network On Chip definition

[Yuanfang Zhang]

Add a new coresight-tnoc.yaml file to describe the bindings required to
define Trace Network On Chip (TNOC) in device trees. TNOC is an
integration hierarchy which is a hardware component that integrates the
functionalities of TPDA and funnels. It collects trace form subsystems
and transfers to coresight sink.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com>
---
 .../bindings/arm/qcom,coresight-tnoc.yaml          | 111 +++++++++++++++++++++
 1 file changed, 111 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..d07c6f2d7b949f69f9d8dd8de8664382eb39fac1
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tnoc.yaml
@@ -0,0 +1,111 @@
+# 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 Trace Network On Chip - TNOC
+
+maintainers:
+  - Yuanfang Zhang <quic_yuanfang@quicinc.com>
+
+description: >
+  The Trace Network On Chip (TNOC) is an integration hierarchy hardware
+  component that integrates the functionalities of TPDA and funnels.
+
+  It sits in the different subsystem of SOC and aggregates the trace and
+  transports it to Aggregation TNOC or to coresight trace sink eventually.
+  TNOC embeds bridges for all the interfaces APB, ATB, TPDA and NTS (Narrow
+  Time Stamp).
+
+  TNOC can take inputs from different trace sources i.e. ATB, TPDM.
+
+# 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:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: apb_pclk
+
+  clocks:
+    items:
+      - description: APB register access clock
+
+  in-ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    patternProperties:
+      '^port(@[0-9a-f]{1,2})?$':
+        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 = <0x109ab000 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

[PATCH v6 2/2] coresight: add coresight Trace Network On Chip driver

[Yuanfang Zhang]

Add a driver to support Coresight device Trace Network On Chip (TNOC),
which is an integration hierarchy integrating functionalities of TPDA
and funnels. It aggregates the trace and transports to coresight trace
bus.

Compared to current configuration, 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.

  +------------------------+                +-------------------------+
  | Video Subsystem        |                |Video Subsystem          |
  |       +-------------+  |                |       +------------+    |
  |       | Video TPDM  |  |                |       | Video TPDM |    |
  |       +-------------+  |                |       +------------+    |
  |            |           |                |              |          |
  |            v           |                |              v          |
  |   +---------------+    |                |        +-----------+    |
  |   | Video funnel  |    |                |        |Video TNOC |    |
  |   +---------------+    |                |        +-----------+    |
  +------------|-----------+                +------------|------------+
               |                                         |
               v-----+                                   |
+--------------------|---------+                         |
|  Multimedia        v         |                         |
|  Subsystem   +--------+      |                         |
|              |  TPDA  |      |                         v
|              +----|---+      |              +---------------------+
|                   |          |              |   Aggregator  TNOC  |
|                   |          |              +----------|----------+
|                   +--        |                         |
|                     |        |                         |
|                     |        |                         |
|              +------v-----+  |                         |
|              |  Funnel    |  |                         |
|              +------------+  |                         |
+----------------|-------------+                         |
                 |                                       |
                 v                                       v
      +--------------------+                    +------------------+
      |   Coresight Sink   |                    |  Coresight Sink  |
      +--------------------+                    +------------------+

       Current Configuration                            TNOC

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 | 192 +++++++++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tnoc.h |  34 +++++
 4 files changed, 240 insertions(+)

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index ecd7086a5b83e86b6bc8ea039d6d26a628334ed3..f20600d58f38568f8178f69d3f678c2df2cbca7e 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -259,4 +259,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 Network On Chip driver"
+	help
+	  This driver provides support for Trace Network On Chip (TNOC) component.
+	  TNOC is an interconnect used to collect traces from various subsystems
+	  and transport to a coresight 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 coresight 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 8e62c3150aebd1e82b445fafc97a0a9b44397b0e..880e9ed6bfe9c711492c6a2cd972751f56dd8010 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..5658501155d322d28e87ff820c7ba8b787eff7ce
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-tnoc.c
@@ -0,0 +1,192 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+ #include <linux/amba/bus.h>
+ #include <linux/coresight.h>
+ #include <linux/device.h>
+ #include <linux/io.h>
+ #include <linux/kernel.h>
+ #include <linux/module.h>
+ #include <linux/of.h>
+ #include <linux/platform_device.h>
+
+#include "coresight-priv.h"
+#include "coresight-tnoc.h"
+#include "coresight-trace-id.h"
+
+DEFINE_CORESIGHT_DEVLIST(trace_noc_devs, "traceNoc");
+
+static void trace_noc_enable_hw(struct trace_noc_drvdata *drvdata)
+{
+	u32 val;
+
+	/* Set ATID */
+	writel_relaxed(drvdata->atid, drvdata->base + TRACE_NOC_XLD);
+
+	/* Set the data word count between 'SYNC' packets */
+	writel_relaxed(TRACE_NOC_SYNC_INTERVAL, drvdata->base + TRACE_NOC_SYNCR);
+
+	/* Set the Control register:
+	 * - Set the FLAG packets to 'FLAG' packets
+	 * - Set the FREQ packets to 'FREQ_TS' packets
+	 * - Enable generation of output ATB traffic
+	 */
+
+	val = readl_relaxed(drvdata->base + TRACE_NOC_CTRL);
+
+	val &= ~TRACE_NOC_CTRL_FLAGTYPE;
+	val |= TRACE_NOC_CTRL_FREQTYPE;
+	val |= TRACE_NOC_CTRL_PORTEN;
+
+	writel(val, drvdata->base + TRACE_NOC_CTRL);
+}
+
+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);
+
+	dev_dbg(drvdata->dev, "Trace NOC is enabled\n");
+	return 0;
+}
+
+static void trace_noc_disable_hw(struct trace_noc_drvdata *drvdata)
+{
+	writel(0x0, drvdata->base + TRACE_NOC_CTRL);
+}
+
+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_dbg(drvdata->dev, "Trace NOC is disabled\n");
+}
+
+static int trace_noc_id(struct coresight_device *csdev, __maybe_unused enum cs_mode mode,
+			__maybe_unused struct coresight_device *sink)
+{
+	struct trace_noc_drvdata *drvdata;
+
+	drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	return drvdata->atid;
+}
+
+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 = {
+	.trace_id	= trace_noc_id,
+	.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;
+
+	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   = 0x00ffff00,
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(amba, trace_noc_ids);
+
+static struct amba_driver trace_noc_driver = {
+	.drv = {
+		.name   = "coresight-trace-noc",
+		.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..34c696965ec50e3664d55e04e22d5b854d6937a7
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-tnoc.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2025 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)
+/* Sets the type of issued ATB FLAG packets.*/
+#define TRACE_NOC_CTRL_FLAGTYPE BIT(7)
+/* Sets the type of issued ATB FREQ packet*/
+#define TRACE_NOC_CTRL_FREQTYPE BIT(8)
+
+#define TRACE_NOC_SYNC_INTERVAL	0xFFFF
+
+/*
+ * 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.
+ */
+struct trace_noc_drvdata {
+	void __iomem		*base;
+	struct device		*dev;
+	struct coresight_device	*csdev;
+	spinlock_t		spinlock; /* lock for the drvdata. */
+	u32			atid;
+};

-- 
2.34.1

Re: [PATCH v6 2/2] coresight: add coresight Trace Network On Chip driver

[Leo Yan]

Hi Yuanfang,

On Thu, May 22, 2025 at 04:07:52PM +0800, Yuanfang Zhang wrote:
> Add a driver to support Coresight device Trace Network On Chip (TNOC),
> which is an integration hierarchy integrating functionalities of TPDA
> and funnels. It aggregates the trace and transports to coresight trace
> bus.

The code quality is good for me. However, I still have several minor
comments.

> Compared to current configuration, 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.
> 
>   +------------------------+                +-------------------------+
>   | Video Subsystem        |                |Video Subsystem          |
>   |       +-------------+  |                |       +------------+    |
>   |       | Video TPDM  |  |                |       | Video TPDM |    |
>   |       +-------------+  |                |       +------------+    |
>   |            |           |                |              |          |
>   |            v           |                |              v          |
>   |   +---------------+    |                |        +-----------+    |
>   |   | Video funnel  |    |                |        |Video TNOC |    |
>   |   +---------------+    |                |        +-----------+    |
>   +------------|-----------+                +------------|------------+
>                |                                         |
>                v-----+                                   |
> +--------------------|---------+                         |
> |  Multimedia        v         |                         |
> |  Subsystem   +--------+      |                         |
> |              |  TPDA  |      |                         v
> |              +----|---+      |              +---------------------+
> |                   |          |              |   Aggregator  TNOC  |
> |                   |          |              +----------|----------+
> |                   +--        |                         |
> |                     |        |                         |
> |                     |        |                         |
> |              +------v-----+  |                         |
> |              |  Funnel    |  |                         |
> |              +------------+  |                         |
> +----------------|-------------+                         |
>                  |                                       |
>                  v                                       v
>       +--------------------+                    +------------------+
>       |   Coresight Sink   |                    |  Coresight Sink  |
>       +--------------------+                    +------------------+
> 
>        Current Configuration                            TNOC
> 
> 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 | 192 +++++++++++++++++++++++++++
>  drivers/hwtracing/coresight/coresight-tnoc.h |  34 +++++
>  4 files changed, 240 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index ecd7086a5b83e86b6bc8ea039d6d26a628334ed3..f20600d58f38568f8178f69d3f678c2df2cbca7e 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -259,4 +259,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 Network On Chip driver"
> +	help
> +	  This driver provides support for Trace Network On Chip (TNOC) component.
> +	  TNOC is an interconnect used to collect traces from various subsystems
> +	  and transport to a coresight 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 coresight 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 8e62c3150aebd1e82b445fafc97a0a9b44397b0e..880e9ed6bfe9c711492c6a2cd972751f56dd8010 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..5658501155d322d28e87ff820c7ba8b787eff7ce
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-tnoc.c
> @@ -0,0 +1,192 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> + #include <linux/amba/bus.h>
> + #include <linux/coresight.h>
> + #include <linux/device.h>
> + #include <linux/io.h>
> + #include <linux/kernel.h>
> + #include <linux/module.h>
> + #include <linux/of.h>
> + #include <linux/platform_device.h>
> +
> +#include "coresight-priv.h"
> +#include "coresight-tnoc.h"
> +#include "coresight-trace-id.h"
> +
> +DEFINE_CORESIGHT_DEVLIST(trace_noc_devs, "traceNoc");
> +
> +static void trace_noc_enable_hw(struct trace_noc_drvdata *drvdata)
> +{
> +	u32 val;
> +
> +	/* Set ATID */
> +	writel_relaxed(drvdata->atid, drvdata->base + TRACE_NOC_XLD);
> +
> +	/* Set the data word count between 'SYNC' packets */
> +	writel_relaxed(TRACE_NOC_SYNC_INTERVAL, drvdata->base + TRACE_NOC_SYNCR);
> +
> +	/* Set the Control register:
> +	 * - Set the FLAG packets to 'FLAG' packets
> +	 * - Set the FREQ packets to 'FREQ_TS' packets
> +	 * - Enable generation of output ATB traffic
> +	 */
> +
> +	val = readl_relaxed(drvdata->base + TRACE_NOC_CTRL);
> +
> +	val &= ~TRACE_NOC_CTRL_FLAGTYPE;
> +	val |= TRACE_NOC_CTRL_FREQTYPE;
> +	val |= TRACE_NOC_CTRL_PORTEN;
> +
> +	writel(val, drvdata->base + TRACE_NOC_CTRL);
> +}
> +
> +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);

        scoped_guard(spinlock, &drvdata->spinlock) {

        }

> +	if (csdev->refcnt == 0)
> +		trace_noc_enable_hw(drvdata);
> +
> +	csdev->refcnt++;
> +	spin_unlock(&drvdata->spinlock);
> +
> +	dev_dbg(drvdata->dev, "Trace NOC is enabled\n");
> +	return 0;
> +}
> +
> +static void trace_noc_disable_hw(struct trace_noc_drvdata *drvdata)
> +{
> +	writel(0x0, drvdata->base + TRACE_NOC_CTRL);
> +}

It is not necessary to use a function to encapsulate single operations.

Please drop the function trace_noc_disable_hw() and directly use
writel() in the call site.

> +
> +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_dbg(drvdata->dev, "Trace NOC is disabled\n");
> +}
> +
> +static int trace_noc_id(struct coresight_device *csdev, __maybe_unused enum cs_mode mode,
> +			__maybe_unused struct coresight_device *sink)
> +{
> +	struct trace_noc_drvdata *drvdata;
> +
> +	drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> +	return drvdata->atid;
> +}
> +
> +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 = {
> +	.trace_id	= trace_noc_id,
> +	.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;
> +
> +	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);

Invoke coresight_trace_id_put_system_id() for registration failure.

> +
> +	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   = 0x00ffff00,
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(amba, trace_noc_ids);
> +
> +static struct amba_driver trace_noc_driver = {
> +	.drv = {
> +		.name   = "coresight-trace-noc",
> +		.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..34c696965ec50e3664d55e04e22d5b854d6937a7
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-tnoc.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2025 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)
> +/* Sets the type of issued ATB FLAG packets.*/
> +#define TRACE_NOC_CTRL_FLAGTYPE BIT(7)
> +/* Sets the type of issued ATB FREQ packet*/
> +#define TRACE_NOC_CTRL_FREQTYPE BIT(8)
> +
> +#define TRACE_NOC_SYNC_INTERVAL	0xFFFF
> +
> +/*
> + * 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.

Change to "serialize enable/disable operations" ?

> + * @atid:	id for the trace packet.
> + */
> +struct trace_noc_drvdata {
> +	void __iomem		*base;
> +	struct device		*dev;
> +	struct coresight_device	*csdev;
> +	spinlock_t		spinlock; /* lock for the drvdata. */

Remove comment "lock for the drvdata" ?  As it has been described in
the head comments.

> +	u32			atid;
> +};

Please move the content into coresight-tnoc.c and drop the header.

The definitions are private for the driver, it is not necessary to
maintain them in a header.


> 
> -- 
> 2.34.1
> 
> _______________________________________________
> CoreSight mailing list -- coresight@lists.linaro.org
> To unsubscribe send an email to coresight-leave@lists.linaro.org

Re: [PATCH v6 2/2] coresight: add coresight Trace Network On Chip driver

[Yuanfang Zhang]

On 5/22/2025 9:05 PM, Leo Yan wrote:
> Hi Yuanfang,
> 
> On Thu, May 22, 2025 at 04:07:52PM +0800, Yuanfang Zhang wrote:
>> Add a driver to support Coresight device Trace Network On Chip (TNOC),
>> which is an integration hierarchy integrating functionalities of TPDA
>> and funnels. It aggregates the trace and transports to coresight trace
>> bus.
> 
> The code quality is good for me. However, I still have several minor
> comments.
> 
>> Compared to current configuration, 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.
>>
>>   +------------------------+                +-------------------------+
>>   | Video Subsystem        |                |Video Subsystem          |
>>   |       +-------------+  |                |       +------------+    |
>>   |       | Video TPDM  |  |                |       | Video TPDM |    |
>>   |       +-------------+  |                |       +------------+    |
>>   |            |           |                |              |          |
>>   |            v           |                |              v          |
>>   |   +---------------+    |                |        +-----------+    |
>>   |   | Video funnel  |    |                |        |Video TNOC |    |
>>   |   +---------------+    |                |        +-----------+    |
>>   +------------|-----------+                +------------|------------+
>>                |                                         |
>>                v-----+                                   |
>> +--------------------|---------+                         |
>> |  Multimedia        v         |                         |
>> |  Subsystem   +--------+      |                         |
>> |              |  TPDA  |      |                         v
>> |              +----|---+      |              +---------------------+
>> |                   |          |              |   Aggregator  TNOC  |
>> |                   |          |              +----------|----------+
>> |                   +--        |                         |
>> |                     |        |                         |
>> |                     |        |                         |
>> |              +------v-----+  |                         |
>> |              |  Funnel    |  |                         |
>> |              +------------+  |                         |
>> +----------------|-------------+                         |
>>                  |                                       |
>>                  v                                       v
>>       +--------------------+                    +------------------+
>>       |   Coresight Sink   |                    |  Coresight Sink  |
>>       +--------------------+                    +------------------+
>>
>>        Current Configuration                            TNOC
>>
>> 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 | 192 +++++++++++++++++++++++++++
>>  drivers/hwtracing/coresight/coresight-tnoc.h |  34 +++++
>>  4 files changed, 240 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>> index ecd7086a5b83e86b6bc8ea039d6d26a628334ed3..f20600d58f38568f8178f69d3f678c2df2cbca7e 100644
>> --- a/drivers/hwtracing/coresight/Kconfig
>> +++ b/drivers/hwtracing/coresight/Kconfig
>> @@ -259,4 +259,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 Network On Chip driver"
>> +	help
>> +	  This driver provides support for Trace Network On Chip (TNOC) component.
>> +	  TNOC is an interconnect used to collect traces from various subsystems
>> +	  and transport to a coresight 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 coresight 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 8e62c3150aebd1e82b445fafc97a0a9b44397b0e..880e9ed6bfe9c711492c6a2cd972751f56dd8010 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..5658501155d322d28e87ff820c7ba8b787eff7ce
>> --- /dev/null
>> +++ b/drivers/hwtracing/coresight/coresight-tnoc.c
>> @@ -0,0 +1,192 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> + #include <linux/amba/bus.h>
>> + #include <linux/coresight.h>
>> + #include <linux/device.h>
>> + #include <linux/io.h>
>> + #include <linux/kernel.h>
>> + #include <linux/module.h>
>> + #include <linux/of.h>
>> + #include <linux/platform_device.h>
>> +
>> +#include "coresight-priv.h"
>> +#include "coresight-tnoc.h"
>> +#include "coresight-trace-id.h"
>> +
>> +DEFINE_CORESIGHT_DEVLIST(trace_noc_devs, "traceNoc");
>> +
>> +static void trace_noc_enable_hw(struct trace_noc_drvdata *drvdata)
>> +{
>> +	u32 val;
>> +
>> +	/* Set ATID */
>> +	writel_relaxed(drvdata->atid, drvdata->base + TRACE_NOC_XLD);
>> +
>> +	/* Set the data word count between 'SYNC' packets */
>> +	writel_relaxed(TRACE_NOC_SYNC_INTERVAL, drvdata->base + TRACE_NOC_SYNCR);
>> +
>> +	/* Set the Control register:
>> +	 * - Set the FLAG packets to 'FLAG' packets
>> +	 * - Set the FREQ packets to 'FREQ_TS' packets
>> +	 * - Enable generation of output ATB traffic
>> +	 */
>> +
>> +	val = readl_relaxed(drvdata->base + TRACE_NOC_CTRL);
>> +
>> +	val &= ~TRACE_NOC_CTRL_FLAGTYPE;
>> +	val |= TRACE_NOC_CTRL_FREQTYPE;
>> +	val |= TRACE_NOC_CTRL_PORTEN;
>> +
>> +	writel(val, drvdata->base + TRACE_NOC_CTRL);
>> +}
>> +
>> +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);
> 
>         scoped_guard(spinlock, &drvdata->spinlock) {
> 
>         }
> 
Done in next patch.
>> +	if (csdev->refcnt == 0)
>> +		trace_noc_enable_hw(drvdata);
>> +
>> +	csdev->refcnt++;
>> +	spin_unlock(&drvdata->spinlock);
>> +
>> +	dev_dbg(drvdata->dev, "Trace NOC is enabled\n");
>> +	return 0;
>> +}
>> +
>> +static void trace_noc_disable_hw(struct trace_noc_drvdata *drvdata)
>> +{
>> +	writel(0x0, drvdata->base + TRACE_NOC_CTRL);
>> +}
> 
> It is not necessary to use a function to encapsulate single operations.
> 
> Please drop the function trace_noc_disable_hw() and directly use
> writel() in the call site.

Done in next patch.
> 
>> +
>> +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_dbg(drvdata->dev, "Trace NOC is disabled\n");
>> +}
>> +
>> +static int trace_noc_id(struct coresight_device *csdev, __maybe_unused enum cs_mode mode,
>> +			__maybe_unused struct coresight_device *sink)
>> +{
>> +	struct trace_noc_drvdata *drvdata;
>> +
>> +	drvdata = dev_get_drvdata(csdev->dev.parent);
>> +
>> +	return drvdata->atid;
>> +}
>> +
>> +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 = {
>> +	.trace_id	= trace_noc_id,
>> +	.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;
>> +
>> +	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);
> 
> Invoke coresight_trace_id_put_system_id() for registration failure.
> 
Done in next patch.
>> +
>> +	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   = 0x00ffff00,
>> +	},
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(amba, trace_noc_ids);
>> +
>> +static struct amba_driver trace_noc_driver = {
>> +	.drv = {
>> +		.name   = "coresight-trace-noc",
>> +		.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..34c696965ec50e3664d55e04e22d5b854d6937a7
>> --- /dev/null
>> +++ b/drivers/hwtracing/coresight/coresight-tnoc.h
>> @@ -0,0 +1,34 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (c) 2025 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)
>> +/* Sets the type of issued ATB FLAG packets.*/
>> +#define TRACE_NOC_CTRL_FLAGTYPE BIT(7)
>> +/* Sets the type of issued ATB FREQ packet*/
>> +#define TRACE_NOC_CTRL_FREQTYPE BIT(8)
>> +
>> +#define TRACE_NOC_SYNC_INTERVAL	0xFFFF
>> +
>> +/*
>> + * 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.
> 
> Change to "serialize enable/disable operations" ?
Done in next patch.
> 
>> + * @atid:	id for the trace packet.
>> + */
>> +struct trace_noc_drvdata {
>> +	void __iomem		*base;
>> +	struct device		*dev;
>> +	struct coresight_device	*csdev;
>> +	spinlock_t		spinlock; /* lock for the drvdata. */
> 
> Remove comment "lock for the drvdata" ?  As it has been described in
> the head comments.
it was suggested by "checkpatch.pl", removed in next patch. 
> 
>> +	u32			atid;
>> +};
> 
> Please move the content into coresight-tnoc.c and drop the header.
> 
> The definitions are private for the driver, it is not necessary to
> maintain them in a header.

Done in next patch.
> 
> 
>>
>> -- 
>> 2.34.1
>>
>> _______________________________________________
>> CoreSight mailing list -- coresight@lists.linaro.org
>> To unsubscribe send an email to coresight-leave@lists.linaro.org

Re: [PATCH v6 2/2] coresight: add coresight Trace Network On Chip driver

[Suzuki K Poulose]

On 22/05/2025 09:07, Yuanfang Zhang wrote:
> Add a driver to support Coresight device Trace Network On Chip (TNOC),
> which is an integration hierarchy integrating functionalities of TPDA
> and funnels. It aggregates the trace and transports to coresight trace
> bus.
> 
> Compared to current configuration, 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.
> 
>    +------------------------+                +-------------------------+
>    | Video Subsystem        |                |Video Subsystem          |
>    |       +-------------+  |                |       +------------+    |
>    |       | Video TPDM  |  |                |       | Video TPDM |    |
>    |       +-------------+  |                |       +------------+    |
>    |            |           |                |              |          |
>    |            v           |                |              v          |
>    |   +---------------+    |                |        +-----------+    |
>    |   | Video funnel  |    |                |        |Video TNOC |    |
>    |   +---------------+    |                |        +-----------+    |
>    +------------|-----------+                +------------|------------+
>                 |                                         |
>                 v-----+                                   |
> +--------------------|---------+                         |
> |  Multimedia        v         |                         |
> |  Subsystem   +--------+      |                         |
> |              |  TPDA  |      |                         v
> |              +----|---+      |              +---------------------+
> |                   |          |              |   Aggregator  TNOC  |
> |                   |          |              +----------|----------+
> |                   +--        |                         |
> |                     |        |                         |
> |                     |        |                         |
> |              +------v-----+  |                         |
> |              |  Funnel    |  |                         |
> |              +------------+  |                         |
> +----------------|-------------+                         |
>                   |                                       |
>                   v                                       v
>        +--------------------+                    +------------------+
>        |   Coresight Sink   |                    |  Coresight Sink  |
>        +--------------------+                    +------------------+
> 
>         Current Configuration                            TNOC
> 
> Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com>


> +
> +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;

Do you need to expose this via sysfs ? Otherwise, how can you map
a trace to a TNOC at decoding ?

Suzuki

Re: [PATCH v6 2/2] coresight: add coresight Trace Network On Chip driver

[Yuanfang Zhang]

On 5/22/2025 9:13 PM, Suzuki K Poulose wrote:
> On 22/05/2025 09:07, Yuanfang Zhang wrote:
>> Add a driver to support Coresight device Trace Network On Chip (TNOC),
>> which is an integration hierarchy integrating functionalities of TPDA
>> and funnels. It aggregates the trace and transports to coresight trace
>> bus.
>>
>> Compared to current configuration, 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.
>>
>>    +------------------------+                +-------------------------+
>>    | Video Subsystem        |                |Video Subsystem          |
>>    |       +-------------+  |                |       +------------+    |
>>    |       | Video TPDM  |  |                |       | Video TPDM |    |
>>    |       +-------------+  |                |       +------------+    |
>>    |            |           |                |              |          |
>>    |            v           |                |              v          |
>>    |   +---------------+    |                |        +-----------+    |
>>    |   | Video funnel  |    |                |        |Video TNOC |    |
>>    |   +---------------+    |                |        +-----------+    |
>>    +------------|-----------+                +------------|------------+
>>                 |                                         |
>>                 v-----+                                   |
>> +--------------------|---------+                         |
>> |  Multimedia        v         |                         |
>> |  Subsystem   +--------+      |                         |
>> |              |  TPDA  |      |                         v
>> |              +----|---+      |              +---------------------+
>> |                   |          |              |   Aggregator  TNOC  |
>> |                   |          |              +----------|----------+
>> |                   +--        |                         |
>> |                     |        |                         |
>> |                     |        |                         |
>> |              +------v-----+  |                         |
>> |              |  Funnel    |  |                         |
>> |              +------------+  |                         |
>> +----------------|-------------+                         |
>>                   |                                       |
>>                   v                                       v
>>        +--------------------+                    +------------------+
>>        |   Coresight Sink   |                    |  Coresight Sink  |
>>        +--------------------+                    +------------------+
>>
>>         Current Configuration                            TNOC
>>
>> Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com>
> 
> 
>> +
>> +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;
> 
> Do you need to expose this via sysfs ? Otherwise, how can you map
> a trace to a TNOC at decoding ?

yes, need to expose the atid via sysfs, but it better to expose it on source driver which connect with
this TNOC. so dont expose it on this driver.

> 
> Suzuki

Re: [PATCH v6 2/2] coresight: add coresight Trace Network On Chip driver

[Leo Yan]

On Fri, May 23, 2025 at 04:08:58PM +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;
> > 
> > Do you need to expose this via sysfs ? Otherwise, how can you map
> > a trace to a TNOC at decoding ?
> 
> yes, need to expose the atid via sysfs, but it better to expose it on source driver which connect with
> this TNOC. so dont expose it on this driver.

If so, why the ID is not maintained in coresight_path::trace_id?

A source device allocates ID and maintains in coresight path, then
this ID is passed (when enabling the link) to TNOC driver to consume it.

Thanks,
Leo

Re: [PATCH v6 2/2] coresight: add coresight Trace Network On Chip driver

[Suzuki K Poulose]

On 23/05/2025 09:56, Leo Yan wrote:
> On Fri, May 23, 2025 at 04:08:58PM +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;
>>>
>>> Do you need to expose this via sysfs ? Otherwise, how can you map
>>> a trace to a TNOC at decoding ?
>>
>> yes, need to expose the atid via sysfs, but it better to expose it on source driver which connect with
>> this TNOC. so dont expose it on this driver.

But why ? How does that work ? The packets that come via TNOC 
(irrespective of the source(s)) will have the same ATID as that of the 
TNOC. So :

1) How does it help if the source exports the ID that was allocated in 
the TNOC driver ?

2) How does the source driver know the TraceID for exposing via sysfs ?
Does it expose its own traceid ?

> 
> If so, why the ID is not maintained in coresight_path::trace_id?
> 
> A source device allocates ID and maintains in coresight path, then
> this ID is passed (when enabling the link) to TNOC driver to consume it.

Good question, since we have the "path" maintaining the TraceID, we
should use that here for the TNOC. But the other question is, can there 
be multiple sources connected to a single TNOC ? (I am guessing, yes!. 
And thus it may not work with what you are proposing.

Cheers
Suzuki


> 
> Thanks,
> Leo

Re: [PATCH v6 2/2] coresight: add coresight Trace Network On Chip driver

[Yuanfang Zhang]

On 5/27/2025 6:19 PM, Suzuki K Poulose wrote:
> On 23/05/2025 09:56, Leo Yan wrote:
>> On Fri, May 23, 2025 at 04:08:58PM +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;
>>>>
>>>> Do you need to expose this via sysfs ? Otherwise, how can you map
>>>> a trace to a TNOC at decoding ?
>>>
>>> yes, need to expose the atid via sysfs, but it better to expose it on source driver which connect with
>>> this TNOC. so dont expose it on this driver.
> 
> But why ? How does that work ? The packets that come via TNOC (irrespective of the source(s)) will have the same ATID as that of the TNOC. So :
> 
> 1) How does it help if the source exports the ID that was allocated in the TNOC driver ?
>
The sources connecting to TNOC will have the same ATID which allocate in TNOC.
This is convenient as users do not need to know which source is connected to this TNOC, 
to get ID can through reading the trace_id sysfs node in the source directory.

> 2) How does the source driver know the TraceID for exposing via sysfs ?
> Does it expose its own traceid ?
No, sources connecting to TNOC don't have their own traceid, it expose the ATID which allocated in TNOC.
TNOC will maintain the ID in coresight_path:: trace_id, when enable source, the source can get it from path.

Here is the patch to expose id in source:
https://patchwork.kernel.org/project/linux-arm-kernel/patch/20250530-showtraceid-v1-1-2761352cf7b4@quicinc.com/ 
> 
>>
>> If so, why the ID is not maintained in coresight_path::trace_id?
>>
>> A source device allocates ID and maintains in coresight path, then
>> this ID is passed (when enabling the link) to TNOC driver to consume it.
> 
This is because there can be multiple sources connected to one TNOC, and these sources share one Trace ID, so the ID is allocated in TNOC.

> Good question, since we have the "path" maintaining the TraceID, we
> should use that here for the TNOC. But the other question is, can there be multiple sources connected to a single TNOC ? (I am guessing, yes!. And thus it may not work with what you are proposing.
>
 
yes, there can be multiple sources connected to one TNOC, and these sources share one Trace ID which allocate in TNOC. 
To decode the scenario relay on TraceID + Inport number, TraceID identifies the TNOC, the decoder maintains a table that maps each TNOC inport to its corresponding source.

> Cheers
> Suzuki
> 
> 
>>
>> Thanks,
>> Leo
> 
> 
> 

Re: [PATCH v6 2/2] coresight: add coresight Trace Network On Chip driver

[Suzuki K Poulose]

On 30/05/2025 11:28, Yuanfang Zhang wrote:
> 
> 
> On 5/27/2025 6:19 PM, Suzuki K Poulose wrote:
>> On 23/05/2025 09:56, Leo Yan wrote:
>>> On Fri, May 23, 2025 at 04:08:58PM +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;
>>>>>
>>>>> Do you need to expose this via sysfs ? Otherwise, how can you map
>>>>> a trace to a TNOC at decoding ?
>>>>
>>>> yes, need to expose the atid via sysfs, but it better to expose it on source driver which connect with
>>>> this TNOC. so dont expose it on this driver.
>>
>> But why ? How does that work ? The packets that come via TNOC (irrespective of the source(s)) will have the same ATID as that of the TNOC. So :
>>
>> 1) How does it help if the source exports the ID that was allocated in the TNOC driver ?
>>
> The sources connecting to TNOC will have the same ATID which allocate in TNOC.
> This is convenient as users do not need to know which source is connected to this TNOC,
> to get ID can through reading the trace_id sysfs node in the source directory.
> 
>> 2) How does the source driver know the TraceID for exposing via sysfs ?
>> Does it expose its own traceid ?
> No, sources connecting to TNOC don't have their own traceid, it expose the ATID which allocated in TNOC.
> TNOC will maintain the ID in coresight_path:: trace_id, when enable source, the source can get it from path.
> 
> Here is the patch to expose id in source:
> https://patchwork.kernel.org/project/linux-arm-kernel/patch/20250530-showtraceid-v1-1-2761352cf7b4@quicinc.com/

Please don't do that. We don't have to fake a traceid for all sources.
It is only of use to the decoder, with manual input from the user. So,
someone using the TNOC based system must be aware of how to collect the
traceid and as such expose it from the TNOC and not all the other
sources connected to it.

Simply expose it on the TNOC device node


Suzuki


>>
>>>
>>> If so, why the ID is not maintained in coresight_path::trace_id?
>>>
>>> A source device allocates ID and maintains in coresight path, then
>>> this ID is passed (when enabling the link) to TNOC driver to consume it.
>>
> This is because there can be multiple sources connected to one TNOC, and these sources share one Trace ID, so the ID is allocated in TNOC.
> 
>> Good question, since we have the "path" maintaining the TraceID, we
>> should use that here for the TNOC. But the other question is, can there be multiple sources connected to a single TNOC ? (I am guessing, yes!. And thus it may not work with what you are proposing.
>>
>   
> yes, there can be multiple sources connected to one TNOC, and these sources share one Trace ID which allocate in TNOC.
> To decode the scenario relay on TraceID + Inport number, TraceID identifies the TNOC, the decoder maintains a table that maps each TNOC inport to its corresponding source.
> 
>> Cheers
>> Suzuki
>>
>>
>>>
>>> Thanks,
>>> Leo
>>
>>
>>
> 

Re: [PATCH v6 2/2] coresight: add coresight Trace Network On Chip driver

[Leo Yan]

On Fri, May 30, 2025 at 12:32:23PM +0100, Suzuki Kuruppassery Poulose wrote:

[...]

> > > 2) How does the source driver know the TraceID for exposing via sysfs ?
> > > Does it expose its own traceid ?
> > No, sources connecting to TNOC don't have their own traceid, it expose the ATID which allocated in TNOC.
> > TNOC will maintain the ID in coresight_path:: trace_id, when enable source, the source can get it from path.
> > 
> > Here is the patch to expose id in source:
> > https://patchwork.kernel.org/project/linux-arm-kernel/patch/20250530-showtraceid-v1-1-2761352cf7b4@quicinc.com/
> 
> Please don't do that. We don't have to fake a traceid for all sources.
> It is only of use to the decoder, with manual input from the user. So,
> someone using the TNOC based system must be aware of how to collect the
> traceid and as such expose it from the TNOC and not all the other
> sources connected to it.
> 
> Simply expose it on the TNOC device node

[...]

> > > Good question, since we have the "path" maintaining the TraceID, we
> > > should use that here for the TNOC. But the other question is, can there be multiple sources connected to a single TNOC ? (I am guessing, yes!. And thus it may not work with what you are proposing.
> > > 
> > yes, there can be multiple sources connected to one TNOC, and these sources share one Trace ID which allocate in TNOC.
> > To decode the scenario relay on TraceID + Inport number, TraceID identifies the TNOC, the decoder maintains a table that maps each TNOC inport to its corresponding source.

If the Trace ID is only used to identify a TNOC, I am just wandering
if can use self-contained method.

For example, you can generate a ID number based on the register base
address, something like:

   /* TNOC physical address */
   drvdata->paddr = res->start;

   if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT))
       drvdata->atid = (drvdata->paddr >> 32) ^ (drvdata->paddr & 0xffffffffUL);
   else
       drvdata->atid = drvdata->paddr;

Then, you can get a unique ID for each TNOC in the system, and the
ID is determined by the pyshical address and can be calculated
directly by decoder.

Leo

Re: [PATCH v6 2/2] coresight: add coresight Trace Network On Chip driver

[Yuanfang Zhang]

On 5/30/2025 8:59 PM, Leo Yan wrote:
> On Fri, May 30, 2025 at 12:32:23PM +0100, Suzuki Kuruppassery Poulose wrote:
> 
> [...]
> 
>>>> 2) How does the source driver know the TraceID for exposing via sysfs ?
>>>> Does it expose its own traceid ?
>>> No, sources connecting to TNOC don't have their own traceid, it expose the ATID which allocated in TNOC.
>>> TNOC will maintain the ID in coresight_path:: trace_id, when enable source, the source can get it from path.
>>>
>>> Here is the patch to expose id in source:
>>> https://patchwork.kernel.org/project/linux-arm-kernel/patch/20250530-showtraceid-v1-1-2761352cf7b4@quicinc.com/
>>
>> Please don't do that. We don't have to fake a traceid for all sources.
>> It is only of use to the decoder, with manual input from the user. So,
>> someone using the TNOC based system must be aware of how to collect the
>> traceid and as such expose it from the TNOC and not all the other
>> sources connected to it.
>>
>> Simply expose it on the TNOC device node
> 
> [...]
> 
>>>> Good question, since we have the "path" maintaining the TraceID, we
>>>> should use that here for the TNOC. But the other question is, can there be multiple sources connected to a single TNOC ? (I am guessing, yes!. And thus it may not work with what you are proposing.
>>>>
>>> yes, there can be multiple sources connected to one TNOC, and these sources share one Trace ID which allocate in TNOC.
>>> To decode the scenario relay on TraceID + Inport number, TraceID identifies the TNOC, the decoder maintains a table that maps each TNOC inport to its corresponding source.
> 
> If the Trace ID is only used to identify a TNOC, I am just wandering
> if can use self-contained method.
> 
> For example, you can generate a ID number based on the register base
> address, something like:
> 
>    /* TNOC physical address */
>    drvdata->paddr = res->start;
> 
>    if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT))
>        drvdata->atid = (drvdata->paddr >> 32) ^ (drvdata->paddr & 0xffffffffUL);
>    else
>        drvdata->atid = drvdata->paddr;
> 
> Then, you can get a unique ID for each TNOC in the system, and the
> ID is determined by the pyshical address and can be calculated
> directly by decoder.
> 
> Leo

Using both dynamic and self-contained IDs within the same system can result in ID conflicts, which may cause decoding failures.

Re: [PATCH v6 2/2] coresight: add coresight Trace Network On Chip driver

[Leo Yan]

On Tue, Jun 03, 2025 at 11:22:58AM +0800, Yuanfang Zhang wrote:

[...]

> Using both dynamic and self-contained IDs within the same system can result in ID conflicts, which may cause decoding failures.

If so, please follow up Suzuki's suggestion to expose Sysfs interface
in TNOC device node.

Leo

Re: [PATCH v6 2/2] coresight: add coresight Trace Network On Chip driver

[Yuanfang Zhang]

On 6/3/2025 8:25 PM, Leo Yan wrote:
> On Tue, Jun 03, 2025 at 11:22:58AM +0800, Yuanfang Zhang wrote:
> 
> [...]
> 
>> Using both dynamic and self-contained IDs within the same system can result in ID conflicts, which may cause decoding failures.
> 
> If so, please follow up Suzuki's suggestion to expose Sysfs interface
> in TNOC device node.
> 
> Leo

Done in patch V8.