[PATCH v2 0/4] Coresight: Add Coresight Control Unit driver

[PATCH v2 0/4] Coresight: Add Coresight Control Unit driver

[Jie Gan]

The Coresight Control Unit(CCU) device hosts miscellaneous configuration
registers to control various features related to TMC ETR device.

The CCU device works as a helper device physically connected to the TMC ETR device.
---------------------------------------------------------
             |ETR0|             |ETR1|
              . \                 / .
              .  \               /  .
              .   \             /   .
              .    \           /    .
---------------------------------------------------
ETR0ATID0-ETR0ATID3     CCU     ETR1ATID0-ETR1ATID3
---------------------------------------------------
Each ETR has four ATID registers with 128 bits long in total.
e.g. ETR0ATID0-ETR0ATID3 registers are used by ETR0 device.

Based on the trace id which is programed in CCU ATID register of
specific ETR, trace data with that trace id can get into ETR's buffer
while other trace data gets ignored. The number of CCU ATID registers
depends on the number of defined TMC ETR devices. For example, two TMC
ETR devices need eight ATID registers. ETR0 with ETR0ATID0-ETR0ATID3
and ETR1 with ETR1ATID0-ETRATID3.

The significant challenge in enabling the data filter function is how
to collect the trace ID of the source device. The introduction of
trace_id callback function addresses this challenge. The callback function
collects trace ID of the device and return it directly. The trace ID will be
stored in the structure called cs_sink_data and transmitted to helper
and sink devices.

The cs_sink_data structure is created to address how to transmit
parameters needs by coresight_enable_path/coresight_disbale_path
functions.

Here is an example of the struct cs_sink_data:
struct cs_sink_data {
        struct perf_output_handle  *handle; //used by perf mode
        struct coresight_device    *sink;   //used to retrieve atid_offset
        u32                        traceid; //traceid needed by CCU
};

The atid_offset mentioned before is the offset to ATID register in CCU
device.

Enabling the source device will configure one bit in the ATID register based
on its trace ID.
Disabling the source devices will reset the bit in the AITD register
based on its trace ID.

Useage:
Enable:
STM device with trace ID 5 and ETR0 is activated.
Bitmap before the enablement:
ETR0ATID0:
31..................543210
==========================
0000000000000000000000...0
==========================

Bitmap after the enablement:
31..................543210
==========================
0000000000000...0000100000
==========================

The bit 5 of the ETR0ATID0 register is configured to 1 when enabling the
STM device.

Disable:
STM device with trace ID 5 and ETR0 is activated.
Bitmap before the disablement:
ETR0ATID0:
31................6543210
=========================
000000000010111...0100000
=========================

Bitmap after the disablement
ETR0ATID0:
31................6543210
=========================
000000000010111...0000000
=========================

The bit 5 of the ETR0ATID0 register is reset to 0 when disabling the STM
device.

Previous discussion for V1:

https://lore.kernel.org/lkml/20240618072726.3767974-1-quic_jiegan@quicinc.com/T/#t

V1->V2:
1. Rename the device to Coresight Control Unit.
2. Introduce the trace_id function pointer to address the challeng how to
properly collect the trace ID of the device.
3. Introduce a new way to define the qcom,ccu-atid-offset property in
device tree.
4. Disabling the filter function blocked on acquiring the ATID-offset,
which will be addressed in a separate patch once it’s ready.

Jie Gan (4):
  Coresight: Add trace_id function to collect trace ID
  dt-bindings: arm: Add binding document for Coresight Control Unit
    device.
  Coresight: Add Coresight Control Unit driver
  arm64: dts: qcom: Add CCU and ETR nodes for SA8775p

 .../bindings/arm/qcom,coresight-ccu.yaml      |  87 ++++++
 arch/arm64/boot/dts/qcom/sa8775p.dtsi         | 163 ++++++++++
 drivers/hwtracing/coresight/Kconfig           |   6 +
 drivers/hwtracing/coresight/Makefile          |   1 +
 drivers/hwtracing/coresight/coresight-ccu.c   | 290 ++++++++++++++++++
 drivers/hwtracing/coresight/coresight-ccu.h   |  18 ++
 drivers/hwtracing/coresight/coresight-core.c  |  53 +++-
 drivers/hwtracing/coresight/coresight-etb10.c |   3 +-
 .../hwtracing/coresight/coresight-etm-perf.c  |  34 +-
 .../coresight/coresight-etm3x-core.c          |  14 +
 .../coresight/coresight-etm4x-core.c          |  13 +
 drivers/hwtracing/coresight/coresight-priv.h  |  12 +-
 drivers/hwtracing/coresight/coresight-stm.c   |  13 +
 drivers/hwtracing/coresight/coresight-sysfs.c |  24 +-
 .../hwtracing/coresight/coresight-tmc-etf.c   |   3 +-
 .../hwtracing/coresight/coresight-tmc-etr.c   |   6 +-
 drivers/hwtracing/coresight/coresight-tpda.c  |  13 +
 drivers/hwtracing/coresight/coresight-trbe.c  |   4 +-
 drivers/hwtracing/coresight/ultrasoc-smb.c    |   3 +-
 include/linux/coresight.h                     |   4 +
 20 files changed, 739 insertions(+), 25 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
 create mode 100644 drivers/hwtracing/coresight/coresight-ccu.c
 create mode 100644 drivers/hwtracing/coresight/coresight-ccu.h

-- 
2.34.1

[PATCH v2 1/4] Coresight: Add trace_id function to collect trace ID

[Jie Gan]

Add 'trace_id' function pointer in ops. It's responsible for collect the
trace ID of the device.

Add 'struct cs_sink_data' to store the data used by coresight_enable_path/
coresight_disable_path. The structure will be transmitted to the helper and
sink device.

Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
---
 drivers/hwtracing/coresight/coresight-core.c  | 53 +++++++++++++++----
 drivers/hwtracing/coresight/coresight-etb10.c |  3 +-
 .../hwtracing/coresight/coresight-etm-perf.c  | 34 ++++++++++--
 .../coresight/coresight-etm3x-core.c          | 14 +++++
 .../coresight/coresight-etm4x-core.c          | 13 +++++
 drivers/hwtracing/coresight/coresight-priv.h  | 12 ++++-
 drivers/hwtracing/coresight/coresight-stm.c   | 13 +++++
 drivers/hwtracing/coresight/coresight-sysfs.c | 24 +++++++--
 .../hwtracing/coresight/coresight-tmc-etf.c   |  3 +-
 .../hwtracing/coresight/coresight-tmc-etr.c   |  6 ++-
 drivers/hwtracing/coresight/coresight-tpda.c  | 13 +++++
 drivers/hwtracing/coresight/coresight-trbe.c  |  4 +-
 drivers/hwtracing/coresight/ultrasoc-smb.c    |  3 +-
 include/linux/coresight.h                     |  4 ++
 14 files changed, 174 insertions(+), 25 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index 9fc6f6b863e0..f414e66f4cda 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -297,12 +297,12 @@ static int coresight_enable_helper(struct coresight_device *csdev,
 	return helper_ops(csdev)->enable(csdev, mode, data);
 }
 
-static void coresight_disable_helper(struct coresight_device *csdev)
+static void coresight_disable_helper(struct coresight_device *csdev, void *data)
 {
-	helper_ops(csdev)->disable(csdev, NULL);
+	helper_ops(csdev)->disable(csdev, data);
 }
 
-static void coresight_disable_helpers(struct coresight_device *csdev)
+static void coresight_disable_helpers(struct coresight_device *csdev, void *data)
 {
 	int i;
 	struct coresight_device *helper;
@@ -310,7 +310,7 @@ static void coresight_disable_helpers(struct coresight_device *csdev)
 	for (i = 0; i < csdev->pdata->nr_outconns; ++i) {
 		helper = csdev->pdata->out_conns[i]->dest_dev;
 		if (helper && coresight_is_helper(helper))
-			coresight_disable_helper(helper);
+			coresight_disable_helper(helper, data);
 	}
 }
 
@@ -327,7 +327,7 @@ static void coresight_disable_helpers(struct coresight_device *csdev)
 void coresight_disable_source(struct coresight_device *csdev, void *data)
 {
 	source_ops(csdev)->disable(csdev, data);
-	coresight_disable_helpers(csdev);
+	coresight_disable_helpers(csdev, NULL);
 }
 EXPORT_SYMBOL_GPL(coresight_disable_source);
 
@@ -337,7 +337,8 @@ EXPORT_SYMBOL_GPL(coresight_disable_source);
  * disabled.
  */
 static void coresight_disable_path_from(struct list_head *path,
-					struct coresight_node *nd)
+					struct coresight_node *nd,
+					void *sink_data)
 {
 	u32 type;
 	struct coresight_device *csdev, *parent, *child;
@@ -382,13 +383,13 @@ static void coresight_disable_path_from(struct list_head *path,
 		}
 
 		/* Disable all helpers adjacent along the path last */
-		coresight_disable_helpers(csdev);
+		coresight_disable_helpers(csdev, sink_data);
 	}
 }
 
-void coresight_disable_path(struct list_head *path)
+void coresight_disable_path(struct list_head *path, void *sink_data)
 {
-	coresight_disable_path_from(path, NULL);
+	coresight_disable_path_from(path, NULL, sink_data);
 }
 EXPORT_SYMBOL_GPL(coresight_disable_path);
 
@@ -468,10 +469,42 @@ int coresight_enable_path(struct list_head *path, enum cs_mode mode,
 out:
 	return ret;
 err:
-	coresight_disable_path_from(path, nd);
+	coresight_disable_path_from(path, nd, sink_data);
 	goto out;
 }
 
+int coresight_read_traceid(struct list_head *path)
+{
+	int trace_id, type;
+	struct coresight_device *csdev;
+	struct coresight_node *nd;
+
+	list_for_each_entry(nd, path, link) {
+		csdev = nd->csdev;
+		type = csdev->type;
+
+		switch(type) {
+			case CORESIGHT_DEV_TYPE_SOURCE:
+				if (source_ops(csdev)->trace_id != NULL) {
+					trace_id = source_ops(csdev)->trace_id(csdev);
+					if (trace_id > 0)
+						return trace_id;
+				}
+				break;
+			case CORESIGHT_DEV_TYPE_LINK:
+				if (link_ops(csdev)->trace_id != NULL) {
+					trace_id = link_ops(csdev)->trace_id(csdev);
+					if (trace_id > 0)
+						return trace_id;
+				}
+				break;
+			default:
+				break;
+		}
+	}
+	return -EINVAL;
+}
+
 struct coresight_device *coresight_get_sink(struct list_head *path)
 {
 	struct coresight_device *csdev;
diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
index 7edd3f1d0d46..05e620529c14 100644
--- a/drivers/hwtracing/coresight/coresight-etb10.c
+++ b/drivers/hwtracing/coresight/coresight-etb10.c
@@ -173,7 +173,8 @@ static int etb_enable_perf(struct coresight_device *csdev, void *data)
 	pid_t pid;
 	unsigned long flags;
 	struct etb_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
-	struct perf_output_handle *handle = data;
+	struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
+	struct perf_output_handle *handle = sink_data->handle;
 	struct cs_buffers *buf = etm_perf_sink_config(handle);
 
 	spin_lock_irqsave(&drvdata->spinlock, flags);
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index c0c60e6a1703..8b155765b959 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -452,6 +452,7 @@ static void etm_event_start(struct perf_event *event, int flags)
 	struct perf_output_handle *handle = &ctxt->handle;
 	struct coresight_device *sink, *csdev = per_cpu(csdev_src, cpu);
 	struct list_head *path;
+	struct cs_sink_data *sink_data = NULL;
 	u64 hw_id;
 
 	if (!csdev)
@@ -490,9 +491,18 @@ static void etm_event_start(struct perf_event *event, int flags)
 	if (WARN_ON_ONCE(!sink))
 		goto fail_end_stop;
 
+	sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL);
+	if (!sink_data)
+		goto fail_end_stop;
+
+	sink_data->sink = sink;
+	sink_data->traceid = coresight_read_traceid(path);
+	sink_data->handle = handle;
 	/* Nothing will happen without a path */
-	if (coresight_enable_path(path, CS_MODE_PERF, handle))
+	if (coresight_enable_path(path, CS_MODE_PERF, sink_data)) {
+		kfree(sink_data);
 		goto fail_end_stop;
+	}
 
 	/* Finally enable the tracer */
 	if (source_ops(csdev)->enable(csdev, event, CS_MODE_PERF))
@@ -511,6 +521,7 @@ static void etm_event_start(struct perf_event *event, int flags)
 		perf_report_aux_output_id(event, hw_id);
 	}
 
+	kfree(sink_data);
 out:
 	/* Tell the perf core the event is alive */
 	event->hw.state = 0;
@@ -519,7 +530,8 @@ static void etm_event_start(struct perf_event *event, int flags)
 	return;
 
 fail_disable_path:
-	coresight_disable_path(path);
+	coresight_disable_path(path, sink_data);
+	kfree(sink_data);
 fail_end_stop:
 	/*
 	 * Check if the handle is still associated with the event,
@@ -544,6 +556,7 @@ static void etm_event_stop(struct perf_event *event, int mode)
 	struct perf_output_handle *handle = &ctxt->handle;
 	struct etm_event_data *event_data;
 	struct list_head *path;
+	struct cs_sink_data *sink_data = NULL;
 
 	/*
 	 * If we still have access to the event_data via handle,
@@ -588,6 +601,10 @@ static void etm_event_stop(struct perf_event *event, int mode)
 	if (!sink)
 		return;
 
+	sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL);
+	if (!sink_data)
+		return;
+
 	/* stop tracer */
 	coresight_disable_source(csdev, event);
 
@@ -601,12 +618,16 @@ static void etm_event_stop(struct perf_event *event, int mode)
 	 * have to do anything here.
 	 */
 	if (handle->event && (mode & PERF_EF_UPDATE)) {
-		if (WARN_ON_ONCE(handle->event != event))
+		if (WARN_ON_ONCE(handle->event != event)) {
+			kfree(sink_data);
 			return;
+		}
 
 		/* update trace information */
-		if (!sink_ops(sink)->update_buffer)
+		if (!sink_ops(sink)->update_buffer) {
+			kfree(sink_data);
 			return;
+		}
 
 		size = sink_ops(sink)->update_buffer(sink, handle,
 					      event_data->snk_config);
@@ -627,8 +648,11 @@ static void etm_event_stop(struct perf_event *event, int mode)
 			WARN_ON(size);
 	}
 
+	sink_data->sink = sink;
+	sink_data->traceid = coresight_read_traceid(path);
 	/* Disabling the path make its elements available to other sessions */
-	coresight_disable_path(path);
+	coresight_disable_path(path, sink_data);
+	kfree(sink_data);
 }
 
 static int etm_event_add(struct perf_event *event, int mode)
diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
index 8b362605d242..27e973749050 100644
--- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
@@ -696,10 +696,24 @@ static void etm_disable(struct coresight_device *csdev,
 		coresight_set_mode(csdev, CS_MODE_DISABLED);
 }
 
+static int etm_trace_id(struct coresight_device *csdev)
+{
+	struct etm_drvdata *drvdata;
+
+	if (csdev == NULL)
+		return -EINVAL;
+
+	drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	return etm_read_alloc_trace_id(drvdata);
+}
+
+
 static const struct coresight_ops_source etm_source_ops = {
 	.cpu_id		= etm_cpu_id,
 	.enable		= etm_enable,
 	.disable	= etm_disable,
+	.trace_id	= etm_trace_id,
 };
 
 static const struct coresight_ops etm_cs_ops = {
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index bf01f01964cf..8c3e9bfb9a9c 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1024,10 +1024,23 @@ static void etm4_disable(struct coresight_device *csdev,
 		coresight_set_mode(csdev, CS_MODE_DISABLED);
 }
 
+static int etm4_trace_id(struct coresight_device *csdev)
+{
+	struct etmv4_drvdata *drvdata;
+
+	if (csdev == NULL)
+		return -EINVAL;
+
+	drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	return etm4_read_alloc_trace_id(drvdata);
+}
+
 static const struct coresight_ops_source etm4_source_ops = {
 	.cpu_id		= etm4_cpu_id,
 	.enable		= etm4_enable,
 	.disable	= etm4_disable,
+	.trace_id	= etm4_trace_id,
 };
 
 static const struct coresight_ops etm4_cs_ops = {
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index 61a46d3bdcc8..e2576531f796 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -105,6 +105,15 @@ struct cs_buffers {
 	void			**data_pages;
 };
 
+/**
+ * struct cs_sink_data - data used by coresight_enable_path/coresight_disable_path
+ */
+struct cs_sink_data {
+	struct perf_output_handle	*handle;
+	struct coresight_device		*sink;
+	u32				traceid;
+};
+
 static inline void coresight_insert_barrier_packet(void *buf)
 {
 	if (buf)
@@ -129,9 +138,10 @@ static inline void CS_UNLOCK(void __iomem *addr)
 	} while (0);
 }
 
-void coresight_disable_path(struct list_head *path);
+void coresight_disable_path(struct list_head *path, void *sink_data);
 int coresight_enable_path(struct list_head *path, enum cs_mode mode,
 			  void *sink_data);
+int coresight_read_traceid(struct list_head *path);
 struct coresight_device *coresight_get_sink(struct list_head *path);
 struct coresight_device *coresight_get_sink_by_id(u32 id);
 struct coresight_device *
diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
index 117dbb484543..3817743fc0c6 100644
--- a/drivers/hwtracing/coresight/coresight-stm.c
+++ b/drivers/hwtracing/coresight/coresight-stm.c
@@ -280,9 +280,22 @@ static void stm_disable(struct coresight_device *csdev,
 	}
 }
 
+static int stm_trace_id(struct coresight_device *csdev)
+{
+	struct stm_drvdata *drvdata;
+
+	if (csdev == NULL)
+		return -EINVAL;
+
+	drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	return drvdata->traceid;
+}
+
 static const struct coresight_ops_source stm_source_ops = {
 	.enable		= stm_enable,
 	.disable	= stm_disable,
+	.trace_id	= stm_trace_id,
 };
 
 static const struct coresight_ops stm_cs_ops = {
diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c
index 1e67cc7758d7..a95afc890587 100644
--- a/drivers/hwtracing/coresight/coresight-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-sysfs.c
@@ -167,6 +167,7 @@ int coresight_enable_sysfs(struct coresight_device *csdev)
 	int cpu, ret = 0;
 	struct coresight_device *sink;
 	struct list_head *path;
+	struct cs_sink_data *sink_data;
 	enum coresight_dev_subtype_source subtype;
 	u32 hash;
 
@@ -208,7 +209,14 @@ int coresight_enable_sysfs(struct coresight_device *csdev)
 		goto out;
 	}
 
-	ret = coresight_enable_path(path, CS_MODE_SYSFS, NULL);
+	sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL);
+	if (!sink_data) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	sink_data->traceid = coresight_read_traceid(path);
+	sink_data->sink = sink;
+	ret = coresight_enable_path(path, CS_MODE_SYSFS, sink_data);
 	if (ret)
 		goto err_path;
 
@@ -245,15 +253,17 @@ int coresight_enable_sysfs(struct coresight_device *csdev)
 		break;
 	}
 
+	kfree(sink_data);
 out:
 	mutex_unlock(&coresight_mutex);
 	return ret;
 
 err_source:
-	coresight_disable_path(path);
+	coresight_disable_path(path, sink_data);
 
 err_path:
 	coresight_release_path(path);
+	kfree(sink_data);
 	goto out;
 }
 EXPORT_SYMBOL_GPL(coresight_enable_sysfs);
@@ -262,6 +272,7 @@ void coresight_disable_sysfs(struct coresight_device *csdev)
 {
 	int cpu, ret;
 	struct list_head *path = NULL;
+	struct cs_sink_data *sink_data = NULL;
 	u32 hash;
 
 	mutex_lock(&coresight_mutex);
@@ -273,6 +284,10 @@ void coresight_disable_sysfs(struct coresight_device *csdev)
 	if (!coresight_disable_source_sysfs(csdev, NULL))
 		goto out;
 
+	sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL);
+	if (!sink_data)
+		goto out;
+
 	switch (csdev->subtype.source_subtype) {
 	case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC:
 		cpu = source_ops(csdev)->cpu_id(csdev);
@@ -296,8 +311,11 @@ void coresight_disable_sysfs(struct coresight_device *csdev)
 		break;
 	}
 
-	coresight_disable_path(path);
+	sink_data->sink = coresight_find_activated_sysfs_sink(csdev);
+	sink_data->traceid = coresight_read_traceid(path);
+	coresight_disable_path(path, sink_data);
 	coresight_release_path(path);
+	kfree(sink_data);
 
 out:
 	mutex_unlock(&coresight_mutex);
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index d4f641cd9de6..7dc536eba3e2 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -250,7 +250,8 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, void *data)
 	pid_t pid;
 	unsigned long flags;
 	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
-	struct perf_output_handle *handle = data;
+	struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
+	struct perf_output_handle *handle = sink_data->handle;
 	struct cs_buffers *buf = etm_perf_sink_config(handle);
 
 	spin_lock_irqsave(&drvdata->spinlock, flags);
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index e75428fa1592..0c24520645e2 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -1253,7 +1253,8 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
 struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,
 				   enum cs_mode mode, void *data)
 {
-	struct perf_output_handle *handle = data;
+	struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
+	struct perf_output_handle *handle = sink_data->handle;
 	struct etr_perf_buffer *etr_perf;
 
 	switch (mode) {
@@ -1647,7 +1648,8 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
 	pid_t pid;
 	unsigned long flags;
 	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
-	struct perf_output_handle *handle = data;
+	struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
+	struct perf_output_handle *handle = sink_data->handle;
 	struct etr_perf_buffer *etr_perf = etm_perf_sink_config(handle);
 
 	spin_lock_irqsave(&drvdata->spinlock, flags);
diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
index bfca103f9f84..20f0ab73159c 100644
--- a/drivers/hwtracing/coresight/coresight-tpda.c
+++ b/drivers/hwtracing/coresight/coresight-tpda.c
@@ -232,9 +232,22 @@ static void tpda_disable(struct coresight_device *csdev,
 	dev_dbg(drvdata->dev, "TPDA inport %d disabled\n", in->dest_port);
 }
 
+static int tpda_trace_id(struct coresight_device *csdev)
+{
+	struct tpda_drvdata *drvdata;
+
+	if (csdev == NULL)
+		return -EINVAL;
+
+	drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	return drvdata->atid;
+}
+
 static const struct coresight_ops_link tpda_link_ops = {
 	.enable		= tpda_enable,
 	.disable	= tpda_disable,
+	.trace_id	= tpda_trace_id,
 };
 
 static const struct coresight_ops tpda_cs_ops = {
diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index 96a32b213669..7f4560b067a8 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -21,6 +21,7 @@
 
 #include "coresight-self-hosted-trace.h"
 #include "coresight-trbe.h"
+#include "coresight-priv.h"
 
 #define PERF_IDX2OFF(idx, buf) ((idx) % ((buf)->nr_pages << PAGE_SHIFT))
 
@@ -1012,7 +1013,8 @@ static int arm_trbe_enable(struct coresight_device *csdev, enum cs_mode mode,
 {
 	struct trbe_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
 	struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
-	struct perf_output_handle *handle = data;
+	struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
+	struct perf_output_handle *handle = sink_data->handle;
 	struct trbe_buf *buf = etm_perf_sink_config(handle);
 
 	WARN_ON(cpudata->cpu != smp_processor_id());
diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
index f9ebf20c91e6..92d8a9fb844e 100644
--- a/drivers/hwtracing/coresight/ultrasoc-smb.c
+++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
@@ -217,7 +217,8 @@ static void smb_enable_sysfs(struct coresight_device *csdev)
 static int smb_enable_perf(struct coresight_device *csdev, void *data)
 {
 	struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
-	struct perf_output_handle *handle = data;
+	struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
+	struct perf_output_handle *handle = sink_data->handle;
 	struct cs_buffers *buf = etm_perf_sink_config(handle);
 	pid_t pid;
 
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index f09ace92176e..fb1c225076a5 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -344,6 +344,7 @@ struct coresight_ops_sink {
  * Operations available for links.
  * @enable:	enables flow between iport and oport.
  * @disable:	disables flow between iport and oport.
+ * @trace_id:	Collect the traceid.
  */
 struct coresight_ops_link {
 	int (*enable)(struct coresight_device *csdev,
@@ -352,6 +353,7 @@ struct coresight_ops_link {
 	void (*disable)(struct coresight_device *csdev,
 			struct coresight_connection *in,
 			struct coresight_connection *out);
+	int (*trace_id)(struct coresight_device *csdev);
 };
 
 /**
@@ -361,6 +363,7 @@ struct coresight_ops_link {
  *		is associated to.
  * @enable:	enables tracing for a source.
  * @disable:	disables tracing for a source.
+ * @trace_id:	collect the traceid.
  */
 struct coresight_ops_source {
 	int (*cpu_id)(struct coresight_device *csdev);
@@ -368,6 +371,7 @@ struct coresight_ops_source {
 		      enum cs_mode mode);
 	void (*disable)(struct coresight_device *csdev,
 			struct perf_event *event);
+	int (*trace_id)(struct coresight_device *csdev);
 };
 
 /**
-- 
2.34.1

[PATCH v2 2/4] dt-bindings: arm: Add binding document for Coresight Control Unit device.

[Jie Gan]

Add binding document for Coresight Control Unit device.

Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
---
 .../bindings/arm/qcom,coresight-ccu.yaml      | 87 +++++++++++++++++++
 1 file changed, 87 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml

diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
new file mode 100644
index 000000000000..9bb8ced393a7
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
@@ -0,0 +1,87 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/qcom,coresight-ccu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: CoreSight Control Unit
+
+maintainers:
+  - Yuanfang Zhang <quic_yuanfang@quicinc.com>
+  - Mao Jinlong <quic_jinlmao@quicinc.com>
+  - Jie Gan <quic_jiegan@quicinc.com>
+
+description:
+  The Coresight Control unit controls various Coresight behaviors.
+  Used to enable/disable ETR’s data filter function based on trace ID.
+
+properties:
+  compatible:
+    const: qcom,coresight-ccu
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: apb_pclk
+
+  reg-names:
+    items:
+      - const: ccu-base
+
+  in-ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    unevaluatedProperties:
+      patternProperties:
+        '^port(@[0-7])?$':
+          description: Input connections from CoreSight Trace bus
+          $ref: /schemas/graph.yaml#/properties/port
+
+          properties:
+            qcom,ccu-atid-offset:
+              description:
+                Offset to the Coresight Control Unit component's ATID register
+                that is used by specific TMC ETR. The ATID register can be programed based
+                on the trace id to filter out specific trace data which gets into ETR buffer.
+              $ref: /schemas/types.yaml#/definitions/uint32
+
+required:
+  - compatible
+  - reg
+  - in-ports
+
+additionalProperties: false
+
+examples:
+  - |
+    syscon@1001000 {
+        compatible = "qcom,coresight-ccu";
+        reg = <0x1001000 0x1000>;
+        reg-names = "ccu-base";
+
+        in-ports {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            port@0 {
+                reg = <0>;
+                ccu_in_port0: endpoint {
+                    qcom,ccu-atid-offset = <0x1f>;
+                    remote-endpoint = <&etr0_out_port>;
+                };
+            };
+
+            port@1 {
+                reg = <1>;
+                ccu_in_port1: endpoint {
+                    qcom,ccu-atid-offset = <0x2f>;
+                    remote-endpoint = <&etr1_out_port>;
+                };
+            };
+        };
+    };
-- 
2.34.1

[PATCH v2 3/4] Coresight: Add Coresight Control Unit driver

[Jie Gan]

The Coresight Control Unit hosts miscellaneous configuration registers
which control various features related to TMC ETR sink.

Based on the trace ID, which is programmed in the related CCU ATID register
of a specific ETR, trace data with that trace ID gets into the ETR buffer,
while other trace data gets dropped.

Enabling source device sets one bit of the ATID register based on source
device's trace ID.
Disabling source device resets the bit according to the source device's trace ID.

Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
---
 drivers/hwtracing/coresight/Kconfig         |   6 +
 drivers/hwtracing/coresight/Makefile        |   1 +
 drivers/hwtracing/coresight/coresight-ccu.c | 290 ++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-ccu.h |  18 ++
 4 files changed, 315 insertions(+)
 create mode 100644 drivers/hwtracing/coresight/coresight-ccu.c
 create mode 100644 drivers/hwtracing/coresight/coresight-ccu.h

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index 06f0a7594169..a36934daa505 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -133,6 +133,12 @@ config CORESIGHT_STM
 	  To compile this driver as a module, choose M here: the
 	  module will be called coresight-stm.
 
+config CORESIGHT_CCU
+	tristate "CoreSight Control Unit driver"
+	help
+	  This driver provides support for CoreSight Control Unit block
+	  that hosts miscellaneous configuration registers.
+
 config CORESIGHT_CPU_DEBUG
 	tristate "CoreSight CPU Debug driver"
 	depends on ARM || ARM64
diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index 4ba478211b31..a668cdba926f 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -51,3 +51,4 @@ coresight-cti-y := coresight-cti-core.o	coresight-cti-platform.o \
 		   coresight-cti-sysfs.o
 obj-$(CONFIG_ULTRASOC_SMB) += ultrasoc-smb.o
 obj-$(CONFIG_CORESIGHT_DUMMY) += coresight-dummy.o
+obj-$(CONFIG_CORESIGHT_CCU) += coresight-ccu.o
diff --git a/drivers/hwtracing/coresight/coresight-ccu.c b/drivers/hwtracing/coresight/coresight-ccu.c
new file mode 100644
index 000000000000..30faf87b1a28
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-ccu.c
@@ -0,0 +1,290 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/clk.h>
+#include <linux/coresight.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/coresight-stm.h>
+
+#include "coresight-ccu.h"
+#include "coresight-priv.h"
+#include "coresight-tmc.h"
+#include "coresight-trace-id.h"
+
+DEFINE_CORESIGHT_DEVLIST(ccu_devs, "ccu");
+
+#define ccu_writel(drvdata, val, offset)	__raw_writel((val), drvdata->base + offset)
+#define ccu_readl(drvdata, offset)		__raw_readl(drvdata->base + offset)
+
+/* The Coresight Control Unit uses four ATID registers to control the data filter function based
+ * on the trace ID for each TMC ETR sink. The length of each ATID register is 32 bits. Therefore,
+ * the ETR has a related field in CCU that is 128 bits long. Each trace ID is represented by one bit in that filed.
+ * e.g. ETR0ATID0 layout, set bit 5 for traceid 5
+ *                                           bit5
+ * ------------------------------------------------------
+ * |   |28|   |24|   |20|   |16|   |12|   |8|  1|4|   |0|
+ * ------------------------------------------------------
+ *
+ * e.g. ETR0:
+ * 127                     0 from ATID_offset for ETR0ATID0
+ * -------------------------
+ * |ATID3|ATID2|ATID1|ATID0|
+ *
+ */
+#define CCU_ATID_REG_OFFSET(traceid, atid_offset) \
+		((traceid / 32) * 4 + atid_offset)
+
+#define CCU_ATID_REG_BIT(traceid)	(traceid % 32)
+#define CCU_ATID_REG_SIZE		0x10
+
+/*
+ * __ccu_set_etr_traceid: Set bit in the ATID register based on trace ID when enable is true.
+ * Reset the bit of the ATID register based on trace ID when enable is false.
+ *
+ * @csdev:	coresight_device struct related to the device
+ * @traceid:	trace ID of the source tracer.
+ * @enable:	True for set bit and false for reset bit.
+ *
+ * Returns 0 indicates success. Non-zero result means failure.
+ */
+static int __ccu_set_etr_traceid(struct coresight_device *csdev,
+			uint32_t traceid, bool enable)
+{
+	uint32_t atid_offset = 0;
+	struct ccu_drvdata *drvdata;
+	unsigned long flags;
+	uint32_t reg_offset;
+	int bit;
+	uint32_t val;
+
+	drvdata = dev_get_drvdata(csdev->dev.parent);
+	if (IS_ERR_OR_NULL(drvdata))
+		return -EINVAL;
+
+	atid_offset = drvdata->atid_offset;
+	if (((traceid < 0) && (traceid >= CORESIGHT_TRACE_IDS_MAX)) || atid_offset <= 0)
+		return -EINVAL;
+
+	spin_lock_irqsave(&drvdata->spin_lock, flags);
+	CS_UNLOCK(drvdata->base);
+
+	reg_offset = CCU_ATID_REG_OFFSET(traceid, atid_offset);
+	bit = CCU_ATID_REG_BIT(traceid);
+	if (reg_offset - atid_offset >= CCU_ATID_REG_SIZE
+		|| bit >= CORESIGHT_TRACE_IDS_MAX) {
+		CS_LOCK(drvdata);
+		spin_unlock_irqrestore(&drvdata->spin_lock, flags);
+		return -EINVAL;
+	}
+
+	val = ccu_readl(drvdata, reg_offset);
+	if (enable)
+		val = val | BIT(bit);
+	else
+		val = val & ~BIT(bit);
+	ccu_writel(drvdata, val, reg_offset);
+
+	CS_LOCK(drvdata->base);
+	spin_unlock_irqrestore(&drvdata->spin_lock, flags);
+	return 0;
+}
+
+/*
+ * ccu_set_atid_offset: Retrieve the offset of the CCU ATID register and store it in the driver data of ETR.
+ *
+ * Returns 0 indicates success. If the result is less than zero, it means
+ * failure.
+ */
+static int __ccu_set_atid_offset(struct device_node *helper_node, struct coresight_device *helper, int port)
+{
+	int atid_offset = 0;
+	struct device_node *node = helper_node;
+	struct device_node *child_node = NULL;
+	struct fwnode_handle *child_fwnode = NULL;
+	struct ccu_drvdata *drvdata;
+
+	if (!helper_node || !helper)
+		return -EINVAL;
+
+	drvdata = dev_get_drvdata(helper->dev.parent);
+	if (IS_ERR_OR_NULL(drvdata))
+		return -EINVAL;
+
+	child_node = of_get_child_by_name(node, "in-ports");
+	if (!child_node)
+		return -EINVAL;
+
+	child_fwnode = fwnode_graph_get_endpoint_by_id(&child_node->fwnode, port, 0, 0);
+	if (!child_fwnode)
+		return -EINVAL;
+
+	fwnode_property_read_u32(child_fwnode, "qcom,ccu-atid-offset", &atid_offset);
+	drvdata->atid_offset = atid_offset;
+	dev_dbg(&helper->dev, "atid_offset:0x%x\n", atid_offset);
+
+	return 0;
+}
+
+static int ccu_set_atid_offset(struct coresight_device *sink, struct coresight_device *helper)
+{
+	int port, i, ret = 0;
+	struct device_node *node;
+
+	for (i = 0; i < sink->pdata->nr_outconns; ++i) {
+		if (sink->pdata->out_conns[i]->dest_dev) {
+			port = sink->pdata->out_conns[i]->dest_port;
+			node = sink->pdata->out_conns[i]->dest_fwnode->dev->of_node;
+			ret = __ccu_set_atid_offset(node, helper, port);
+			return ret;
+		}
+	}
+
+	return -EINVAL;
+}
+
+/*
+ * ccu_set_etr_traceid: Retrieve the ATID offset and trace ID.
+ *
+ * Returns 0 indicates success. None-zero result means failure.
+ */
+static int ccu_set_etr_traceid(struct coresight_device *csdev, struct cs_sink_data *sink_data, bool enable)
+{
+	if ((sink_data->traceid < 0) || (csdev == NULL) || (sink_data->sink == NULL)) {
+		dev_dbg(&csdev->dev, "Invalid parameters\n");
+		return -EINVAL;
+	}
+
+	if (ccu_set_atid_offset(sink_data->sink, csdev))
+		return -EINVAL;
+
+	dev_dbg(&csdev->dev, "traceid is %d\n", sink_data->traceid);
+
+	return __ccu_set_etr_traceid(csdev, sink_data->traceid, enable);
+}
+
+static int ccu_enable(struct coresight_device *csdev, enum cs_mode mode,
+		       void *data)
+{
+	int ret = 0;
+	struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
+
+	ret = ccu_set_etr_traceid(csdev, sink_data, true);
+	if (ret)
+		dev_dbg(&csdev->dev,"enable data filter failed\n");
+
+	return 0;
+}
+
+static int ccu_disable(struct coresight_device *csdev, void *data)
+{
+	int ret = 0;
+	struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
+
+	ret = ccu_set_etr_traceid(csdev, sink_data, false);
+	if (ret)
+		dev_dbg(&csdev->dev,"disable data filter failed\n");
+
+	return 0;
+}
+
+static const struct coresight_ops_helper ccu_helper_ops = {
+	.enable = ccu_enable,
+	.disable = ccu_disable,
+};
+
+static const struct coresight_ops ccu_ops = {
+	.helper_ops = &ccu_helper_ops,
+};
+
+static int ccu_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct coresight_platform_data *pdata;
+	struct ccu_drvdata *drvdata;
+	struct coresight_desc desc = { 0 };
+	struct resource *res;
+
+	desc.name = coresight_alloc_device_name(&ccu_devs, dev);
+	if (!desc.name)
+		return -ENOMEM;
+	pdata = coresight_get_platform_data(dev);
+	if (IS_ERR(pdata))
+		return PTR_ERR(pdata);
+	pdev->dev.platform_data = pdata;
+
+	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+	drvdata->dev = &pdev->dev;
+	drvdata->atid_offset = 0;
+	platform_set_drvdata(pdev, drvdata);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ccu-base");
+	if (!res)
+		return -ENODEV;
+	drvdata->pbase = res->start;
+
+	drvdata->base = devm_ioremap(dev, res->start, resource_size(res));
+	if (!drvdata->base)
+		return -ENOMEM;
+
+	desc.type = CORESIGHT_DEV_TYPE_HELPER;
+	desc.pdata = pdev->dev.platform_data;
+	desc.dev = &pdev->dev;
+	desc.ops = &ccu_ops;
+
+	drvdata->csdev = coresight_register(&desc);
+	if (IS_ERR(drvdata->csdev))
+		return PTR_ERR(drvdata->csdev);
+
+	dev_dbg(dev, "CCU initialized: %s\n", desc.name);
+	return 0;
+}
+
+static void ccu_remove(struct platform_device *pdev)
+{
+	struct ccu_drvdata *drvdata = platform_get_drvdata(pdev);
+
+	coresight_unregister(drvdata->csdev);
+}
+
+static const struct of_device_id ccu_match[] = {
+	{.compatible = "qcom,coresight-ccu"},
+	{}
+};
+
+static struct platform_driver ccu_driver = {
+	.probe          = ccu_probe,
+	.remove         = ccu_remove,
+	.driver         = {
+		.name   = "coresight-ccu",
+		.of_match_table = ccu_match,
+		.suppress_bind_attrs = true,
+	},
+};
+
+static int __init ccu_init(void)
+{
+	return platform_driver_register(&ccu_driver);
+}
+module_init(ccu_init);
+
+static void __exit ccu_exit(void)
+{
+	platform_driver_unregister(&ccu_driver);
+}
+module_exit(ccu_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("CoreSight Control Unit driver");
diff --git a/drivers/hwtracing/coresight/coresight-ccu.h b/drivers/hwtracing/coresight/coresight-ccu.h
new file mode 100644
index 000000000000..d2895d288c88
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-ccu.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef _CORESIGHT_CCU_H
+#define _CORESIGHT_CCU_H
+
+struct ccu_drvdata {
+	void __iomem		*base;
+	phys_addr_t		pbase;
+	struct device		*dev;
+	struct coresight_device	*csdev;
+	spinlock_t		spin_lock;
+	uint32_t		atid_offset;
+};
+
+#endif
-- 
2.34.1

[PATCH v2 4/4] arm64: dts: qcom: Add CCU and ETR nodes for SA8775p

[Jie Gan]

Add CCU and ETR device tree nodes to enable related functions.

Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sa8775p.dtsi | 163 ++++++++++++++++++++++++++
 1 file changed, 163 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
index 23f1b2e5e624..ef4df5e59ab3 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
@@ -1664,6 +1664,38 @@ ice: crypto@1d88000 {
 			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
 		};
 
+		ccu@4001000 {
+			compatible = "qcom,coresight-ccu";
+			reg = <0x0 0x4001000 0x0 0x1000>;
+			reg-names = "ccu-base";
+
+			clocks = <&aoss_qmp>;
+			clock-names = "apb_pclk";
+
+			in-ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				port@0 {
+					reg = <0>;
+					ccu_in0: endpoint {
+						qcom,ccu-atid-offset = <0xf8>;
+						remote-endpoint =
+						<&etr0_out>;
+					};
+				};
+
+				port@1 {
+					reg = <1>;
+					ccu_in1: endpoint {
+						qcom,ccu-atid-offset = <0x108>;
+						remote-endpoint =
+						<&etr1_out>;
+					};
+				};
+			};
+		};
+
 		stm: stm@4002000 {
 			compatible = "arm,coresight-stm", "arm,primecell";
 			reg = <0x0 0x4002000 0x0 0x1000>,
@@ -1867,6 +1899,129 @@ qdss_funnel_in1: endpoint {
 			};
 		};
 
+		replicator@4046000 {
+			compatible = "arm,coresight-dynamic-replicator", "arm,primecell";
+			reg = <0x0 0x4046000 0x0 0x1000>;
+
+			clocks = <&aoss_qmp>;
+			clock-names = "apb_pclk";
+
+			out-ports {
+				port {
+					qdss_rep_out0: endpoint {
+						remote-endpoint =
+						<&etr_rep_in>;
+					};
+				};
+			};
+
+			in-ports {
+				port {
+					qdss_rep_in: endpoint {
+						remote-endpoint =
+						<&swao_rep_out0>;
+					};
+				};
+			};
+		};
+
+		tmc_etr: tmc@4048000 {
+			compatible = "arm,coresight-tmc", "arm,primecell";
+			reg = <0x0 0x4048000 0x0 0x1000>;
+
+			clocks = <&aoss_qmp>;
+			clock-names = "apb_pclk";
+			iommus = <&apps_smmu 0x04c0 0x00>;
+
+			arm,scatter-gather;
+
+			out-ports {
+				port {
+					etr0_out: endpoint {
+						remote-endpoint =
+						<&ccu_in0>;
+					};
+				};
+			};
+
+			in-ports {
+				port {
+					etr0_in: endpoint {
+						remote-endpoint =
+						<&etr_rep_out0>;
+					};
+				};
+			};
+		};
+
+		replicator@404e000 {
+			compatible = "arm,coresight-dynamic-replicator", "arm,primecell";
+			reg = <0x0 0x404e000 0x0 0x1000>;
+
+			clocks = <&aoss_qmp>;
+			clock-names = "apb_pclk";
+
+			out-ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				port@0 {
+					reg = <0>;
+					etr_rep_out0: endpoint {
+						remote-endpoint =
+						<&etr0_in>;
+					};
+				};
+
+				port@1 {
+					reg = <1>;
+					etr_rep_out1: endpoint {
+						remote-endpoint =
+						<&etr1_in>;
+					};
+				};
+			};
+
+			in-ports {
+				port {
+					etr_rep_in: endpoint {
+						remote-endpoint =
+						<&qdss_rep_out0>;
+					};
+				};
+			};
+		};
+
+		tmc_etr1: tmc@404f000 {
+			compatible = "arm,coresight-tmc", "arm,primecell";
+			reg = <0x0 0x404f000 0x0 0x1000>;
+
+			clocks = <&aoss_qmp>;
+			clock-names = "apb_pclk";
+			iommus = <&apps_smmu 0x04a0 0x40>;
+
+			arm,scatter-gather;
+			arm,buffer-size = <0x400000>;
+
+			out-ports {
+				port {
+					etr1_out: endpoint {
+						remote-endpoint =
+						<&ccu_in1>;
+					};
+				};
+			};
+
+			in-ports {
+				port {
+					etr1_in: endpoint {
+						remote-endpoint =
+						<&etr_rep_out1>;
+					};
+				};
+			};
+		};
+
 		funnel@4b04000 {
 			compatible = "arm,coresight-dynamic-funnel", "arm,primecell";
 			reg = <0x0 0x4b04000 0x0 0x1000>;
@@ -1942,6 +2097,14 @@ out-ports {
 				#address-cells = <1>;
 				#size-cells = <0>;
 
+				port@0 {
+					reg = <0>;
+					swao_rep_out0: endpoint {
+						remote-endpoint =
+						<&qdss_rep_in>;
+					};
+				};
+
 				port@1 {
 					reg = <1>;
 					swao_rep_out1: endpoint {
-- 
2.34.1

Re: [PATCH v2 2/4] dt-bindings: arm: Add binding document for Coresight Control Unit device.

[Krzysztof Kozlowski]

On 05/07/2024 11:00, Jie Gan wrote:
> Add binding document for Coresight Control Unit device.

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC (and consider --no-git-fallback argument). It might
happen, that command when run on an older kernel, gives you outdated
entries. Therefore please be sure you base your patches on recent Linux
kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.
</form letter>

Or stop developing on some old tree. It's some sort of weird pattern in
entire Qualcomm Coresight - everything developed on old kernels.

You must work on latest mainline or maintainer or linux-next tree, not
some old Qualcomm tree. Your v5.15, v5.19, v6.4 or v6.8 or whatever you
have there: BIG NOPE.

> 
> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
> ---

Subject: it never ends with full stop.

A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

>  .../bindings/arm/qcom,coresight-ccu.yaml      | 87 +++++++++++++++++++
>  1 file changed, 87 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> 
> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> new file mode 100644
> index 000000000000..9bb8ced393a7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/qcom,coresight-ccu.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: CoreSight Control Unit
> +
> +maintainers:
> +  - Yuanfang Zhang <quic_yuanfang@quicinc.com>
> +  - Mao Jinlong <quic_jinlmao@quicinc.com>
> +  - Jie Gan <quic_jiegan@quicinc.com>
> +
> +description:
> +  The Coresight Control unit controls various Coresight behaviors.
> +  Used to enable/disable ETR’s data filter function based on trace ID.
> +
> +properties:
> +  compatible:
> +    const: qcom,coresight-ccu
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: apb_pclk

Drop _pclk

> +
> +  reg-names:

Please follow DTS coding style about order of properties.

> +    items:
> +      - const: ccu-base
> +
> +  in-ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    unevaluatedProperties:

This was never tested and it cannot reliably work.

Sorry, this is waste of our time.


> +      patternProperties:
> +        '^port(@[0-7])?$':
> +          description: Input connections from CoreSight Trace bus
> +          $ref: /schemas/graph.yaml#/properties/port
> +
> +          properties:
> +            qcom,ccu-atid-offset:
> +              description:
> +                Offset to the Coresight Control Unit component's ATID register
> +                that is used by specific TMC ETR. The ATID register can be programed based
> +                on the trace id to filter out specific trace data which gets into ETR buffer.
> +              $ref: /schemas/types.yaml#/definitions/uint32
> +
> +required:
> +  - compatible
> +  - reg
> +  - in-ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    syscon@1001000 {

That's not a syscon.

> +        compatible = "qcom,coresight-ccu";
> +        reg = <0x1001000 0x1000>;
> +        reg-names = "ccu-base";
> +

Best regards,
Krzysztof

Re: [PATCH v2 4/4] arm64: dts: qcom: Add CCU and ETR nodes for SA8775p

[Krzysztof Kozlowski]

On 05/07/2024 11:00, Jie Gan wrote:
> Add CCU and ETR device tree nodes to enable related functions.
> 
> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sa8775p.dtsi | 163 ++++++++++++++++++++++++++
>  1 file changed, 163 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> index 23f1b2e5e624..ef4df5e59ab3 100644
> --- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> @@ -1664,6 +1664,38 @@ ice: crypto@1d88000 {
>  			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
>  		};
>  
> +		ccu@4001000 {
> +			compatible = "qcom,coresight-ccu";
> +			reg = <0x0 0x4001000 0x0 0x1000>;
> +			reg-names = "ccu-base";

NAK, not tested.

Follow your own internal guidelines - they are precise in what testing
you must peform.

Best regards,
Krzysztof

Re: [PATCH v2 3/4] Coresight: Add Coresight Control Unit driver

[Krzysztof Kozlowski]

On 05/07/2024 11:00, Jie Gan wrote:
> The Coresight Control Unit hosts miscellaneous configuration registers
> which control various features related to TMC ETR sink.
> 
> Based on the trace ID, which is programmed in the related CCU ATID register
> of a specific ETR, trace data with that trace ID gets into the ETR buffer,

....

> +static int ccu_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct coresight_platform_data *pdata;
> +	struct ccu_drvdata *drvdata;
> +	struct coresight_desc desc = { 0 };
> +	struct resource *res;
> +
> +	desc.name = coresight_alloc_device_name(&ccu_devs, dev);
> +	if (!desc.name)
> +		return -ENOMEM;
> +	pdata = coresight_get_platform_data(dev);
> +	if (IS_ERR(pdata))
> +		return PTR_ERR(pdata);
> +	pdev->dev.platform_data = pdata;
> +
> +	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +	drvdata->dev = &pdev->dev;

Use stored dev variable.

> +	drvdata->atid_offset = 0;
> +	platform_set_drvdata(pdev, drvdata);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ccu-base");
> +	if (!res)
> +		return -ENODEV;
> +	drvdata->pbase = res->start;

Drop.

> +
> +	drvdata->base = devm_ioremap(dev, res->start, resource_size(res));

Use proper wrapper for this two.

> +	if (!drvdata->base)
> +		return -ENOMEM;
> +
> +	desc.type = CORESIGHT_DEV_TYPE_HELPER;
> +	desc.pdata = pdev->dev.platform_data;
> +	desc.dev = &pdev->dev;
> +	desc.ops = &ccu_ops;
> +
> +	drvdata->csdev = coresight_register(&desc);
> +	if (IS_ERR(drvdata->csdev))
> +		return PTR_ERR(drvdata->csdev);
> +
> +	dev_dbg(dev, "CCU initialized: %s\n", desc.name);

Drop.

> +	return 0;
> +}
> +
> +static void ccu_remove(struct platform_device *pdev)
> +{
> +	struct ccu_drvdata *drvdata = platform_get_drvdata(pdev);
> +
> +	coresight_unregister(drvdata->csdev);
> +}
> +
> +static const struct of_device_id ccu_match[] = {
> +	{.compatible = "qcom,coresight-ccu"},
> +	{}
> +};
> +
> +static struct platform_driver ccu_driver = {
> +	.probe          = ccu_probe,
> +	.remove         = ccu_remove,
> +	.driver         = {
> +		.name   = "coresight-ccu",
> +		.of_match_table = ccu_match,
> +		.suppress_bind_attrs = true,

Why?

> +	},
> +};
> +
> +static int __init ccu_init(void)
> +{
> +	return platform_driver_register(&ccu_driver);
> +}
> +module_init(ccu_init);
> +
> +static void __exit ccu_exit(void)
> +{
> +	platform_driver_unregister(&ccu_driver);
> +}
> +module_exit(ccu_exit);

Why this is not just module platform driver?

Best regards,
Krzysztof

Re: [PATCH v2 2/4] dt-bindings: arm: Add binding document for Coresight Control Unit device.

[Rob Herring (Arm)]

On Fri, 05 Jul 2024 17:00:47 +0800, Jie Gan wrote:
> Add binding document for Coresight Control Unit device.
> 
> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
> ---
>  .../bindings/arm/qcom,coresight-ccu.yaml      | 87 +++++++++++++++++++
>  1 file changed, 87 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml: unevaluatedProperties: Missing additionalProperties/unevaluatedProperties constraint

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240705090049.1656986-3-quic_jiegan@quicinc.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v2 2/4] dt-bindings: arm: Add binding document for Coresight Control Unit device.

[JieGan]

On Fri, Jul 05, 2024 at 11:07:22AM +0200, Krzysztof Kozlowski wrote:
> On 05/07/2024 11:00, Jie Gan wrote:
> > Add binding document for Coresight Control Unit device.
> 
> <form letter>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC (and consider --no-git-fallback argument). It might
> happen, that command when run on an older kernel, gives you outdated
> entries. Therefore please be sure you base your patches on recent Linux
> kernel.
> 
> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> people, so fix your workflow. Tools might also fail if you work on some
> ancient tree (don't, instead use mainline) or work on fork of kernel
> (don't, instead use mainline). Just use b4 and everything should be
> fine, although remember about `b4 prep --auto-to-cc` if you added new
> patches to the patchset.
> </form letter>
> 
> Or stop developing on some old tree. It's some sort of weird pattern in
> entire Qualcomm Coresight - everything developed on old kernels.
> 
> You must work on latest mainline or maintainer or linux-next tree, not
> some old Qualcomm tree. Your v5.15, v5.19, v6.4 or v6.8 or whatever you
> have there: BIG NOPE.
> 
> > 
> > Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
> > ---
> 
> Subject: it never ends with full stop.
> 
> A nit, subject: drop second/last, redundant "bindings". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> 
> > +    items:
> > +      - const: apb_pclk
> 
> Drop _pclk
> 
> > +
> > +  reg-names:
> 
> Please follow DTS coding style about order of properties.
> 
> > +    items:
> > +      - const: ccu-base
> > +
> > +  in-ports:
> > +    $ref: /schemas/graph.yaml#/properties/ports
> > +
> > +    unevaluatedProperties:
> 
> This was never tested and it cannot reliably work.
> 
> Sorry, this is waste of our time.
> 
>
Sorry about that. I will re-check the dt-binding file.
 
> > +      patternProperties:
> > +        '^port(@[0-7])?$':
> > +          description: Input connections from CoreSight Trace bus
> > +          $ref: /schemas/graph.yaml#/properties/port
> > +
> > +          properties:
> > +            qcom,ccu-atid-offset:
> > +              description:
> > +                Offset to the Coresight Control Unit component's ATID register
> > +                that is used by specific TMC ETR. The ATID register can be programed based
> > +                on the trace id to filter out specific trace data which gets into ETR buffer.
> > +              $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - in-ports
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    syscon@1001000 {
> 
> That's not a syscon.
> 
> > +        compatible = "qcom,coresight-ccu";
> > +        reg = <0x1001000 0x1000>;
> > +        reg-names = "ccu-base";
> > +
> 
> Best regards,
> Krzysztof
> 

Thanks,
Jie

Re: [PATCH v2 3/4] Coresight: Add Coresight Control Unit driver

[JieGan]

On Fri, Jul 05, 2024 at 11:11:19AM +0200, Krzysztof Kozlowski wrote:
> On 05/07/2024 11:00, Jie Gan wrote:
> > The Coresight Control Unit hosts miscellaneous configuration registers
> > which control various features related to TMC ETR sink.
> > 
> > Based on the trace ID, which is programmed in the related CCU ATID register
> > of a specific ETR, trace data with that trace ID gets into the ETR buffer,
> 
> ....
> 
> > +static int ccu_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct coresight_platform_data *pdata;
> > +	struct ccu_drvdata *drvdata;
> > +	struct coresight_desc desc = { 0 };
> > +	struct resource *res;
> > +
> > +	desc.name = coresight_alloc_device_name(&ccu_devs, dev);
> > +	if (!desc.name)
> > +		return -ENOMEM;
> > +	pdata = coresight_get_platform_data(dev);
> > +	if (IS_ERR(pdata))
> > +		return PTR_ERR(pdata);
> > +	pdev->dev.platform_data = pdata;
> > +
> > +	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> > +	if (!drvdata)
> > +		return -ENOMEM;
> > +	drvdata->dev = &pdev->dev;
> 
> Use stored dev variable.
pdev->dev replaced by dev variable.


> 
> > +	drvdata->atid_offset = 0;
> > +	platform_set_drvdata(pdev, drvdata);
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ccu-base");
> > +	if (!res)
> > +		return -ENODEV;
> > +	drvdata->pbase = res->start;
> 
> Drop.
Dropped.

> 
> > +
> > +	drvdata->base = devm_ioremap(dev, res->start, resource_size(res));
> 
> Use proper wrapper for this two.
Replaced by:
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
drvdata->base = devm_ioremap_resource(dev, res);

> 
> > +	if (!drvdata->base)
> > +		return -ENOMEM;
> > +
> > +	desc.type = CORESIGHT_DEV_TYPE_HELPER;
> > +	desc.pdata = pdev->dev.platform_data;
> > +	desc.dev = &pdev->dev;
> > +	desc.ops = &ccu_ops;
> > +
> > +	drvdata->csdev = coresight_register(&desc);
> > +	if (IS_ERR(drvdata->csdev))
> > +		return PTR_ERR(drvdata->csdev);
> > +
> > +	dev_dbg(dev, "CCU initialized: %s\n", desc.name);
> 
> Drop.
Dropped.

> 
> > +	return 0;
> > +}
> > +
> > +static void ccu_remove(struct platform_device *pdev)
> > +{
> > +	struct ccu_drvdata *drvdata = platform_get_drvdata(pdev);
> > +
> > +	coresight_unregister(drvdata->csdev);
> > +}
> > +
> > +static const struct of_device_id ccu_match[] = {
> > +	{.compatible = "qcom,coresight-ccu"},
> > +	{}
> > +};
> > +
> > +static struct platform_driver ccu_driver = {
> > +	.probe          = ccu_probe,
> > +	.remove         = ccu_remove,
> > +	.driver         = {
> > +		.name   = "coresight-ccu",
> > +		.of_match_table = ccu_match,
> > +		.suppress_bind_attrs = true,
> 
> Why?
Sorry, I dont get the point here.
We dont need automatic bind/unbind, so the suppress_bind_attrs sets to true.
We need configure some settings before we register the device.

> 
> > +	},
> > +};
> > +
> > +static int __init ccu_init(void)
> > +{
> > +	return platform_driver_register(&ccu_driver);
> > +}
> > +module_init(ccu_init);
> > +
> > +static void __exit ccu_exit(void)
> > +{
> > +	platform_driver_unregister(&ccu_driver);
> > +}
> > +module_exit(ccu_exit);
> 
> Why this is not just module platform driver?
Replaced by module_platform_driver(ccu_driver);

> 
> Best regards,
> Krzysztof
> 


Thanks,
Jie

Re: [PATCH v2 2/4] dt-bindings: arm: Add binding document for Coresight Control Unit device.

[Suzuki K Poulose]

On 05/07/2024 10:00, Jie Gan wrote:
> Add binding document for Coresight Control Unit device.

nit: This is again too generic ? corsight-tmc-control-unit ? After all
thats what it is and not a *generic* coresight control unit ?

> 
> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
> ---
>   .../bindings/arm/qcom,coresight-ccu.yaml      | 87 +++++++++++++++++++
>   1 file changed, 87 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> 
> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> new file mode 100644
> index 000000000000..9bb8ced393a7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/qcom,coresight-ccu.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: CoreSight Control Unit
> +
> +maintainers:
> +  - Yuanfang Zhang <quic_yuanfang@quicinc.com>
> +  - Mao Jinlong <quic_jinlmao@quicinc.com>
> +  - Jie Gan <quic_jiegan@quicinc.com>
> +
> +description:
> +  The Coresight Control unit controls various Coresight behaviors.
> +  Used to enable/disable ETR’s data filter function based on trace ID.
> +
> +properties:
> +  compatible:
> +    const: qcom,coresight-ccu
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: apb_pclk
> +
> +  reg-names:
> +    items:
> +      - const: ccu-base
> +
> +  in-ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    unevaluatedProperties:
> +      patternProperties:
> +        '^port(@[0-7])?$':
> +          description: Input connections from CoreSight Trace bus
> +          $ref: /schemas/graph.yaml#/properties/port
> +
> +          properties:
> +            qcom,ccu-atid-offset:

Why do we need this atid offset ? Couldn't this be mapped to the "port"
number ?

e.g, input-port 0 on CCU => Offset x
      input-port 1 on CCU => (Offset x + Size of 1 region)

I believe I mentioned this in the previous posting too ?

Suzuki

Re: [PATCH v2 2/4] dt-bindings: arm: Add binding document for Coresight Control Unit device.

[JieGan]

On Mon, Jul 08, 2024 at 10:41:55AM +0100, Suzuki K Poulose wrote:
> On 05/07/2024 10:00, Jie Gan wrote:
> > Add binding document for Coresight Control Unit device.
> 
> nit: This is again too generic ? corsight-tmc-control-unit ? After all
> thats what it is and not a *generic* coresight control unit ?
>
coresight-tmc-control-unit is much better. We will check it.
 
> > 
> > Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
> > ---
> >   .../bindings/arm/qcom,coresight-ccu.yaml      | 87 +++++++++++++++++++
> >   1 file changed, 87 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> > new file mode 100644
> > index 000000000000..9bb8ced393a7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> > @@ -0,0 +1,87 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/arm/qcom,coresight-ccu.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: CoreSight Control Unit
> > +
> > +maintainers:
> > +  - Yuanfang Zhang <quic_yuanfang@quicinc.com>
> > +  - Mao Jinlong <quic_jinlmao@quicinc.com>
> > +  - Jie Gan <quic_jiegan@quicinc.com>
> > +
> > +description:
> > +  The Coresight Control unit controls various Coresight behaviors.
> > +  Used to enable/disable ETR’s data filter function based on trace ID.
> > +
> > +properties:
> > +  compatible:
> > +    const: qcom,coresight-ccu
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    items:
> > +      - const: apb_pclk
> > +
> > +  reg-names:
> > +    items:
> > +      - const: ccu-base
> > +
> > +  in-ports:
> > +    $ref: /schemas/graph.yaml#/properties/ports
> > +
> > +    unevaluatedProperties:
> > +      patternProperties:
> > +        '^port(@[0-7])?$':
> > +          description: Input connections from CoreSight Trace bus
> > +          $ref: /schemas/graph.yaml#/properties/port
> > +
> > +          properties:
> > +            qcom,ccu-atid-offset:
> 
> Why do we need this atid offset ? Couldn't this be mapped to the "port"
> number ?
> 
> e.g, input-port 0 on CCU => Offset x
>      input-port 1 on CCU => (Offset x + Size of 1 region)
If the first ATID offset remains constant, it appears to be feasible.
We will consider the possibility of this solution.

>
> I believe I mentioned this in the previous posting too ?
Yes, you mentioned before. I moved it from TMC filed to CCU filed.

> 
> Suzuki
> 

Re: [PATCH v2 2/4] dt-bindings: arm: Add binding document for Coresight Control Unit device.

[JieGan]

On Mon, Jul 08, 2024 at 06:10:28PM +0800, JieGan wrote:
> On Mon, Jul 08, 2024 at 10:41:55AM +0100, Suzuki K Poulose wrote:
> > On 05/07/2024 10:00, Jie Gan wrote:
> > > Add binding document for Coresight Control Unit device.
> > 
> > nit: This is again too generic ? corsight-tmc-control-unit ? After all
> > thats what it is and not a *generic* coresight control unit ?
> >
> coresight-tmc-control-unit is much better. We will check it.
>  
> > > 
> > > Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
> > > ---
> > >   .../bindings/arm/qcom,coresight-ccu.yaml      | 87 +++++++++++++++++++
> > >   1 file changed, 87 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> > > new file mode 100644
> > > index 000000000000..9bb8ced393a7
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> > > @@ -0,0 +1,87 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/arm/qcom,coresight-ccu.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: CoreSight Control Unit
> > > +
> > > +maintainers:
> > > +  - Yuanfang Zhang <quic_yuanfang@quicinc.com>
> > > +  - Mao Jinlong <quic_jinlmao@quicinc.com>
> > > +  - Jie Gan <quic_jiegan@quicinc.com>
> > > +
> > > +description:
> > > +  The Coresight Control unit controls various Coresight behaviors.
> > > +  Used to enable/disable ETR’s data filter function based on trace ID.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: qcom,coresight-ccu
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    maxItems: 1
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: apb_pclk
> > > +
> > > +  reg-names:
> > > +    items:
> > > +      - const: ccu-base
> > > +
> > > +  in-ports:
> > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > +
> > > +    unevaluatedProperties:
> > > +      patternProperties:
> > > +        '^port(@[0-7])?$':
> > > +          description: Input connections from CoreSight Trace bus
> > > +          $ref: /schemas/graph.yaml#/properties/port
> > > +
> > > +          properties:
> > > +            qcom,ccu-atid-offset:
> > 
> > Why do we need this atid offset ? Couldn't this be mapped to the "port"
> > number ?
> > 
> > e.g, input-port 0 on CCU => Offset x
> >      input-port 1 on CCU => (Offset x + Size of 1 region)
> If the first ATID offset remains constant, it appears to be feasible.
> We will consider the possibility of this solution.
We just checked the ATID offset varies across different hardware platforms.
It defined as 0xf4 on some platforms, and some others defined as 0xf8.

So I think it should be better to define it in device tree node.

> 
> >
> > I believe I mentioned this in the previous posting too ?
> Yes, you mentioned before. I moved it from TMC filed to CCU filed.
> 
> > 
> > Suzuki
> > 
> 

Re: [PATCH v2 3/4] Coresight: Add Coresight Control Unit driver

[Krzysztof Kozlowski]

On 08/07/2024 05:16, JieGan wrote:
> 
>>
>>> +
>>> +	drvdata->base = devm_ioremap(dev, res->start, resource_size(res));
>>
>> Use proper wrapper for this two.
> Replaced by:
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> drvdata->base = devm_ioremap_resource(dev, res);

Why?

Use the wrapper.


...

>>> +
>>> +static struct platform_driver ccu_driver = {
>>> +	.probe          = ccu_probe,
>>> +	.remove         = ccu_remove,
>>> +	.driver         = {
>>> +		.name   = "coresight-ccu",
>>> +		.of_match_table = ccu_match,
>>> +		.suppress_bind_attrs = true,
>>
>> Why?
> Sorry, I dont get the point here.

You do not get the point why I am asking "why?"?

Why do you need it?

> We dont need automatic bind/unbind, so the suppress_bind_attrs sets to true.

But I need it...

> We need configure some settings before we register the device.

Hm, is this expected for coresight devices?

Best regards,
Krzysztof

Re: [PATCH v2 1/4] Coresight: Add trace_id function to collect trace ID

[Markus Elfring]

> Add 'trace_id' function pointer in ops. It's responsible for collect the
…

                                          It is?               collecting?


…
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
…
> +	sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL);
> +	if (!sink_data)
> +		goto fail_end_stop;
…
> +	if (coresight_enable_path(path, CS_MODE_PERF, sink_data)) {
> +		kfree(sink_data);
>  		goto fail_end_stop;
> +	}
…

Would you like to to benefit any more from further applications of scope-based
resource management by using an attribute like “__free(kfree)”?
https://elixir.bootlin.com/linux/v6.10-rc7/source/include/linux/slab.h#L282

Regards,
Markus

Re: [PATCH v2 2/4] dt-bindings: arm: Add binding document for Coresight Control Unit device.

[Suzuki K Poulose]

On 08/07/2024 11:25, JieGan wrote:
> On Mon, Jul 08, 2024 at 06:10:28PM +0800, JieGan wrote:
>> On Mon, Jul 08, 2024 at 10:41:55AM +0100, Suzuki K Poulose wrote:
>>> On 05/07/2024 10:00, Jie Gan wrote:
>>>> Add binding document for Coresight Control Unit device.
>>>
>>> nit: This is again too generic ? corsight-tmc-control-unit ? After all
>>> thats what it is and not a *generic* coresight control unit ?
>>>
>> coresight-tmc-control-unit is much better. We will check it.
>>   
>>>>
>>>> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
>>>> ---
>>>>    .../bindings/arm/qcom,coresight-ccu.yaml      | 87 +++++++++++++++++++
>>>>    1 file changed, 87 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
>>>> new file mode 100644
>>>> index 000000000000..9bb8ced393a7
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
>>>> @@ -0,0 +1,87 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/arm/qcom,coresight-ccu.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: CoreSight Control Unit
>>>> +
>>>> +maintainers:
>>>> +  - Yuanfang Zhang <quic_yuanfang@quicinc.com>
>>>> +  - Mao Jinlong <quic_jinlmao@quicinc.com>
>>>> +  - Jie Gan <quic_jiegan@quicinc.com>
>>>> +
>>>> +description:
>>>> +  The Coresight Control unit controls various Coresight behaviors.
>>>> +  Used to enable/disable ETR’s data filter function based on trace ID.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: qcom,coresight-ccu
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  clocks:
>>>> +    maxItems: 1
>>>> +
>>>> +  clock-names:
>>>> +    items:
>>>> +      - const: apb_pclk
>>>> +
>>>> +  reg-names:
>>>> +    items:
>>>> +      - const: ccu-base
>>>> +
>>>> +  in-ports:
>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>> +
>>>> +    unevaluatedProperties:
>>>> +      patternProperties:
>>>> +        '^port(@[0-7])?$':
>>>> +          description: Input connections from CoreSight Trace bus
>>>> +          $ref: /schemas/graph.yaml#/properties/port
>>>> +
>>>> +          properties:
>>>> +            qcom,ccu-atid-offset:
>>>
>>> Why do we need this atid offset ? Couldn't this be mapped to the "port"
>>> number ?
>>>
>>> e.g, input-port 0 on CCU => Offset x
>>>       input-port 1 on CCU => (Offset x + Size of 1 region)
>> If the first ATID offset remains constant, it appears to be feasible.
>> We will consider the possibility of this solution.
> We just checked the ATID offset varies across different hardware platforms.
> It defined as 0xf4 on some platforms, and some others defined as 0xf8.

What do you mean ? The offset where you apply the filter changes across
different platforms ? or different "tmc-control-unit" implementations ?
Is this discoverable from the device ? We could use different
compatibles for different "types" of the "devices". Simply adding
something in the DT is not the right way.

> 
> So I think it should be better to define it in device tree node.

No. See above.

Suzuki

> 
>>
>>>
>>> I believe I mentioned this in the previous posting too ?
>> Yes, you mentioned before. I moved it from TMC filed to CCU filed.
>>
>>>
>>> Suzuki
>>>
>>

Re: [PATCH v2 3/4] Coresight: Add Coresight Control Unit driver

[Markus Elfring]

…
> Disabling source device resets the bit according to the source device's trace ID.

How do you think about to improve such a change description with imperative wordings?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc7#n94


…
> +++ b/drivers/hwtracing/coresight/coresight-ccu.c
> @@ -0,0 +1,290 @@
…
> +static int __ccu_set_etr_traceid(struct coresight_device *csdev,
> +			uint32_t traceid, bool enable)
> +{
…
> +	spin_lock_irqsave(&drvdata->spin_lock, flags);
> +	CS_UNLOCK(drvdata->base);
…
> +	ccu_writel(drvdata, val, reg_offset);
> +
> +	CS_LOCK(drvdata->base);
> +	spin_unlock_irqrestore(&drvdata->spin_lock, flags);
> +	return 0;
> +}
…

Under which circumstances would you become interested to apply a statement
like “guard(spinlock_irqsave)(&drvdata->spin_lock);”?
https://elixir.bootlin.com/linux/v6.10-rc7/source/include/linux/spinlock.h#L574

Regards,
Markus

Re: [PATCH v2 3/4] Coresight: Add Coresight Control Unit driver

[JieGan]

On Mon, Jul 08, 2024 at 12:44:43PM +0200, Krzysztof Kozlowski wrote:
> On 08/07/2024 05:16, JieGan wrote:
> > 
> >>
> >>> +
> >>> +	drvdata->base = devm_ioremap(dev, res->start, resource_size(res));
> >>
> >> Use proper wrapper for this two.
> > Replaced by:
> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > drvdata->base = devm_ioremap_resource(dev, res);
> 
> Why?
> 
> Use the wrapper.
Sorry, I misunderstood the "wrapper" before.
Just wrapped drvdata->base by void __iomem *base;

> 
> 
> ...
> 
> >>> +
> >>> +static struct platform_driver ccu_driver = {
> >>> +	.probe          = ccu_probe,
> >>> +	.remove         = ccu_remove,
> >>> +	.driver         = {
> >>> +		.name   = "coresight-ccu",
> >>> +		.of_match_table = ccu_match,
> >>> +		.suppress_bind_attrs = true,
> >>
> >> Why?
> > Sorry, I dont get the point here.
> 
> You do not get the point why I am asking "why?"?
> 
> Why do you need it?
> 
> > We dont need automatic bind/unbind, so the suppress_bind_attrs sets to true.
> 
> But I need it...
> 
> > We need configure some settings before we register the device.
> 
> Hm, is this expected for coresight devices?
The coresight device cannot be unbinded independently. As we known, 
The coresight devices collaborate to form a path, from source, link, to sink.
If an unexpected unbinding occurs for a coresight device that is part of the path,
it will disrupt the entire path.

I think it's the reason why the coresight device driver does not need automatic bind/unbind.
Here is the previous discussion for suppress_bind_attrs:
https://lore.kernel.org/all/1453753248-1716-1-git-send-email-mathieu.poirier@linaro.org/#r

> 
> Best regards,
> Krzysztof
> 

Thanks,
Jie

Re: [PATCH v2 2/4] dt-bindings: arm: Add binding document for Coresight Control Unit device.

[JieGan]

On Mon, Jul 08, 2024 at 01:50:11PM +0100, Suzuki K Poulose wrote:
> On 08/07/2024 11:25, JieGan wrote:
> > On Mon, Jul 08, 2024 at 06:10:28PM +0800, JieGan wrote:
> > > On Mon, Jul 08, 2024 at 10:41:55AM +0100, Suzuki K Poulose wrote:
> > > > On 05/07/2024 10:00, Jie Gan wrote:
> > > > > Add binding document for Coresight Control Unit device.
> > > > 
> > > > nit: This is again too generic ? corsight-tmc-control-unit ? After all
> > > > thats what it is and not a *generic* coresight control unit ?
> > > > 
> > > coresight-tmc-control-unit is much better. We will check it.
> > > > > 
> > > > > Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
> > > > > ---
> > > > >    .../bindings/arm/qcom,coresight-ccu.yaml      | 87 +++++++++++++++++++
> > > > >    1 file changed, 87 insertions(+)
> > > > >    create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..9bb8ced393a7
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> > > > > @@ -0,0 +1,87 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/arm/qcom,coresight-ccu.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: CoreSight Control Unit
> > > > > +
> > > > > +maintainers:
> > > > > +  - Yuanfang Zhang <quic_yuanfang@quicinc.com>
> > > > > +  - Mao Jinlong <quic_jinlmao@quicinc.com>
> > > > > +  - Jie Gan <quic_jiegan@quicinc.com>
> > > > > +
> > > > > +description:
> > > > > +  The Coresight Control unit controls various Coresight behaviors.
> > > > > +  Used to enable/disable ETR’s data filter function based on trace ID.
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    const: qcom,coresight-ccu
> > > > > +
> > > > > +  reg:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  clocks:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  clock-names:
> > > > > +    items:
> > > > > +      - const: apb_pclk
> > > > > +
> > > > > +  reg-names:
> > > > > +    items:
> > > > > +      - const: ccu-base
> > > > > +
> > > > > +  in-ports:
> > > > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > > > +
> > > > > +    unevaluatedProperties:
> > > > > +      patternProperties:
> > > > > +        '^port(@[0-7])?$':
> > > > > +          description: Input connections from CoreSight Trace bus
> > > > > +          $ref: /schemas/graph.yaml#/properties/port
> > > > > +
> > > > > +          properties:
> > > > > +            qcom,ccu-atid-offset:
> > > > 
> > > > Why do we need this atid offset ? Couldn't this be mapped to the "port"
> > > > number ?
> > > > 
> > > > e.g, input-port 0 on CCU => Offset x
> > > >       input-port 1 on CCU => (Offset x + Size of 1 region)
> > > If the first ATID offset remains constant, it appears to be feasible.
> > > We will consider the possibility of this solution.
> > We just checked the ATID offset varies across different hardware platforms.
> > It defined as 0xf4 on some platforms, and some others defined as 0xf8.
> 
> What do you mean ? The offset where you apply the filter changes across
> different platforms ? or different "tmc-control-unit" implementations ?
> Is this discoverable from the device ? We could use different
> compatibles for different "types" of the "devices". Simply adding
> something in the DT is not the right way.

I got your point here. I believe we should consult our hardware engineers first to check it.
We need to figure out the design of ATID offset from hardware perspective. Then we can
propose an alternative approach, such as associating the offset with a compitable value,
to resolve this issue.

> 
> > 
> > So I think it should be better to define it in device tree node.
> 
> No. See above.
> 
> Suzuki
> 
> > 
> > > 
> > > > 
> > > > I believe I mentioned this in the previous posting too ?
> > > Yes, you mentioned before. I moved it from TMC filed to CCU filed.
> > > 
> > > > 
> > > > Suzuki
> > > > 
> > > 
> 

Thanks,
Jie

Re: [PATCH v2 2/4] dt-bindings: arm: Add binding document for Coresight Control Unit device.

[JieGan]

On Tue, Jul 09, 2024 at 02:00:25PM +0800, JieGan wrote:
> On Mon, Jul 08, 2024 at 01:50:11PM +0100, Suzuki K Poulose wrote:
> > On 08/07/2024 11:25, JieGan wrote:
> > > On Mon, Jul 08, 2024 at 06:10:28PM +0800, JieGan wrote:
> > > > On Mon, Jul 08, 2024 at 10:41:55AM +0100, Suzuki K Poulose wrote:
> > > > > On 05/07/2024 10:00, Jie Gan wrote:
> > > > > > Add binding document for Coresight Control Unit device.
> > > > > 
> > > > > nit: This is again too generic ? corsight-tmc-control-unit ? After all
> > > > > thats what it is and not a *generic* coresight control unit ?
> > > > > 
> > > > coresight-tmc-control-unit is much better. We will check it.
> > > > > > 
> > > > > > Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
> > > > > > ---
> > > > > >    .../bindings/arm/qcom,coresight-ccu.yaml      | 87 +++++++++++++++++++
> > > > > >    1 file changed, 87 insertions(+)
> > > > > >    create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> > > > > > new file mode 100644
> > > > > > index 000000000000..9bb8ced393a7
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> > > > > > @@ -0,0 +1,87 @@
> > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > > +%YAML 1.2
> > > > > > +---
> > > > > > +$id: http://devicetree.org/schemas/arm/qcom,coresight-ccu.yaml#
> > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > +
> > > > > > +title: CoreSight Control Unit
> > > > > > +
> > > > > > +maintainers:
> > > > > > +  - Yuanfang Zhang <quic_yuanfang@quicinc.com>
> > > > > > +  - Mao Jinlong <quic_jinlmao@quicinc.com>
> > > > > > +  - Jie Gan <quic_jiegan@quicinc.com>
> > > > > > +
> > > > > > +description:
> > > > > > +  The Coresight Control unit controls various Coresight behaviors.
> > > > > > +  Used to enable/disable ETR’s data filter function based on trace ID.
> > > > > > +
> > > > > > +properties:
> > > > > > +  compatible:
> > > > > > +    const: qcom,coresight-ccu
> > > > > > +
> > > > > > +  reg:
> > > > > > +    maxItems: 1
> > > > > > +
> > > > > > +  clocks:
> > > > > > +    maxItems: 1
> > > > > > +
> > > > > > +  clock-names:
> > > > > > +    items:
> > > > > > +      - const: apb_pclk
> > > > > > +
> > > > > > +  reg-names:
> > > > > > +    items:
> > > > > > +      - const: ccu-base
> > > > > > +
> > > > > > +  in-ports:
> > > > > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > > > > +
> > > > > > +    unevaluatedProperties:
> > > > > > +      patternProperties:
> > > > > > +        '^port(@[0-7])?$':
> > > > > > +          description: Input connections from CoreSight Trace bus
> > > > > > +          $ref: /schemas/graph.yaml#/properties/port
> > > > > > +
> > > > > > +          properties:
> > > > > > +            qcom,ccu-atid-offset:
> > > > > 
> > > > > Why do we need this atid offset ? Couldn't this be mapped to the "port"
> > > > > number ?
> > > > > 
> > > > > e.g, input-port 0 on CCU => Offset x
> > > > >       input-port 1 on CCU => (Offset x + Size of 1 region)
> > > > If the first ATID offset remains constant, it appears to be feasible.
> > > > We will consider the possibility of this solution.
> > > We just checked the ATID offset varies across different hardware platforms.
> > > It defined as 0xf4 on some platforms, and some others defined as 0xf8.
> > 
> > What do you mean ? The offset where you apply the filter changes across
> > different platforms ? or different "tmc-control-unit" implementations ?
> > Is this discoverable from the device ? We could use different
> > compatibles for different "types" of the "devices". Simply adding
> > something in the DT is not the right way.
> 
> I got your point here. I believe we should consult our hardware engineers first to check it.
> We need to figure out the design of ATID offset from hardware perspective. Then we can
> propose an alternative approach, such as associating the offset with a compitable value,
> to resolve this issue.
> 
> > 
> > > 
> > > So I think it should be better to define it in device tree node.
> > 
> > No. See above.


Hi Suzuki

There is a new solution for CCU device. We would like to separate one CCU device into several helper
devices, each responsible for one feature of the CCU device.

For the data filter feature, we will define the address of the AITD Register that included by CCU in DT
as base address of the helper node. So the driver code can easily program the register field with the base address.
With this solution, we may need define several helper nodes in DT file to support different features for CCU and each
helper device needs a driver to support its behavior.

for example, ATID register for ETR0 with base address 0x10000f8: (tmp name used, TDFU for tmc data filter unit)

TDFU@10000f8 {
...
}

ATID register for ETR1 with base address 0x1000108:
TDFU@1000108 {
...
}

The CCU device supports various features and the data filter feature for ETR is one of those features. How to support
those features with one helper_enable function is a serious challenge. That's why we would like to separate it.
Meanwhile, This solution can also resolve the offset issue.

We are looking forward your opinions with this proposal.

Thanks!

> > 
> > Suzuki
> > 
> > > 
> > > > 
> > > > > 
> > > > > I believe I mentioned this in the previous posting too ?
> > > > Yes, you mentioned before. I moved it from TMC filed to CCU filed.
> > > > 
> > > > > 
> > > > > Suzuki
> > > > > 
> > > > 
> > 
> 
> Thanks,
> Jie
> 

Thanks,
Jie

Re: [PATCH v2 2/4] dt-bindings: arm: Add binding document for Coresight Control Unit device.

[Suzuki K Poulose]

On 11/07/2024 09:36, JieGan wrote:
> On Tue, Jul 09, 2024 at 02:00:25PM +0800, JieGan wrote:
>> On Mon, Jul 08, 2024 at 01:50:11PM +0100, Suzuki K Poulose wrote:
>>> On 08/07/2024 11:25, JieGan wrote:
>>>> On Mon, Jul 08, 2024 at 06:10:28PM +0800, JieGan wrote:
>>>>> On Mon, Jul 08, 2024 at 10:41:55AM +0100, Suzuki K Poulose wrote:
>>>>>> On 05/07/2024 10:00, Jie Gan wrote:
>>>>>>> Add binding document for Coresight Control Unit device.
>>>>>>
>>>>>> nit: This is again too generic ? corsight-tmc-control-unit ? After all
>>>>>> thats what it is and not a *generic* coresight control unit ?
>>>>>>
>>>>> coresight-tmc-control-unit is much better. We will check it.
>>>>>>>
>>>>>>> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
>>>>>>> ---
>>>>>>>     .../bindings/arm/qcom,coresight-ccu.yaml      | 87 +++++++++++++++++++
>>>>>>>     1 file changed, 87 insertions(+)
>>>>>>>     create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..9bb8ced393a7
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
>>>>>>> @@ -0,0 +1,87 @@
>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>> +%YAML 1.2
>>>>>>> +---
>>>>>>> +$id: http://devicetree.org/schemas/arm/qcom,coresight-ccu.yaml#
>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>> +
>>>>>>> +title: CoreSight Control Unit
>>>>>>> +
>>>>>>> +maintainers:
>>>>>>> +  - Yuanfang Zhang <quic_yuanfang@quicinc.com>
>>>>>>> +  - Mao Jinlong <quic_jinlmao@quicinc.com>
>>>>>>> +  - Jie Gan <quic_jiegan@quicinc.com>
>>>>>>> +
>>>>>>> +description:
>>>>>>> +  The Coresight Control unit controls various Coresight behaviors.
>>>>>>> +  Used to enable/disable ETR’s data filter function based on trace ID.
>>>>>>> +
>>>>>>> +properties:
>>>>>>> +  compatible:
>>>>>>> +    const: qcom,coresight-ccu
>>>>>>> +
>>>>>>> +  reg:
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  clocks:
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  clock-names:
>>>>>>> +    items:
>>>>>>> +      - const: apb_pclk
>>>>>>> +
>>>>>>> +  reg-names:
>>>>>>> +    items:
>>>>>>> +      - const: ccu-base
>>>>>>> +
>>>>>>> +  in-ports:
>>>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>>>>> +
>>>>>>> +    unevaluatedProperties:
>>>>>>> +      patternProperties:
>>>>>>> +        '^port(@[0-7])?$':
>>>>>>> +          description: Input connections from CoreSight Trace bus
>>>>>>> +          $ref: /schemas/graph.yaml#/properties/port
>>>>>>> +
>>>>>>> +          properties:
>>>>>>> +            qcom,ccu-atid-offset:
>>>>>>
>>>>>> Why do we need this atid offset ? Couldn't this be mapped to the "port"
>>>>>> number ?
>>>>>>
>>>>>> e.g, input-port 0 on CCU => Offset x
>>>>>>        input-port 1 on CCU => (Offset x + Size of 1 region)
>>>>> If the first ATID offset remains constant, it appears to be feasible.
>>>>> We will consider the possibility of this solution.
>>>> We just checked the ATID offset varies across different hardware platforms.
>>>> It defined as 0xf4 on some platforms, and some others defined as 0xf8.
>>>
>>> What do you mean ? The offset where you apply the filter changes across
>>> different platforms ? or different "tmc-control-unit" implementations ?
>>> Is this discoverable from the device ? We could use different
>>> compatibles for different "types" of the "devices". Simply adding
>>> something in the DT is not the right way.
>>
>> I got your point here. I believe we should consult our hardware engineers first to check it.
>> We need to figure out the design of ATID offset from hardware perspective. Then we can
>> propose an alternative approach, such as associating the offset with a compitable value,
>> to resolve this issue.
>>
>>>
>>>>
>>>> So I think it should be better to define it in device tree node.
>>>
>>> No. See above.
> 
> 
> Hi Suzuki
> 
> There is a new solution for CCU device. We would like to separate one CCU device into several helper
> devices, each responsible for one feature of the CCU device.
> 
> For the data filter feature, we will define the address of the AITD Register that included by CCU in DT
> as base address of the helper node. So the driver code can easily program the register field with the base address.
> With this solution, we may need define several helper nodes in DT file to support different features for CCU and each
> helper device needs a driver to support its behavior.
> 
> for example, ATID register for ETR0 with base address 0x10000f8: (tmp name used, TDFU for tmc data filter unit)

That looks like a hack to me and don't prefer that and there would be
multiple mappings for a single device and that could complicate device
resource accounting.

> 
> TDFU@10000f8 {
> ...
> }
> 
> ATID register for ETR1 with base address 0x1000108:
> TDFU@1000108 {
> ...
> }
> 
> The CCU device supports various features and the data filter feature for ETR is one of those features. How to support
> those features with one helper_enable function is a serious challenge. That's why we would like to separate it.
> Meanwhile, This solution can also resolve the offset issue.
> 
> We are looking forward your opinions with this proposal.

We need to know what other functionalities are supported and how that
will be used ?

In the worst case, you could define register bases for each port in the
CCU device node.

TDFU@.. {
	reg = <base-address 4K>

	<I don't know the standard property name for> =
	List of {
	<port number>, <Offset from base>,
	}
}

Suzuki

Re: [PATCH v2 2/4] dt-bindings: arm: Add binding document for Coresight Control Unit device.

[JieGan]

On Thu, Jul 11, 2024 at 10:32:17AM +0100, Suzuki K Poulose wrote:
> On 11/07/2024 09:36, JieGan wrote:
> > On Tue, Jul 09, 2024 at 02:00:25PM +0800, JieGan wrote:
> > > On Mon, Jul 08, 2024 at 01:50:11PM +0100, Suzuki K Poulose wrote:
> > > > On 08/07/2024 11:25, JieGan wrote:
> > > > > On Mon, Jul 08, 2024 at 06:10:28PM +0800, JieGan wrote:
> > > > > > On Mon, Jul 08, 2024 at 10:41:55AM +0100, Suzuki K Poulose wrote:
> > > > > > > On 05/07/2024 10:00, Jie Gan wrote:
> > > > > > > > Add binding document for Coresight Control Unit device.
> > > > > > > 
> > > > > > > nit: This is again too generic ? corsight-tmc-control-unit ? After all
> > > > > > > thats what it is and not a *generic* coresight control unit ?
> > > > > > > 
> > > > > > coresight-tmc-control-unit is much better. We will check it.
> > > > > > > > 
> > > > > > > > Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
> > > > > > > > ---
> > > > > > > >     .../bindings/arm/qcom,coresight-ccu.yaml      | 87 +++++++++++++++++++
> > > > > > > >     1 file changed, 87 insertions(+)
> > > > > > > >     create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> > > > > > > > 
> > > > > > > > diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..9bb8ced393a7
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> > > > > > > > @@ -0,0 +1,87 @@
> > > > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > > > > +%YAML 1.2
> > > > > > > > +---
> > > > > > > > +$id: http://devicetree.org/schemas/arm/qcom,coresight-ccu.yaml#
> > > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > > +
> > > > > > > > +title: CoreSight Control Unit
> > > > > > > > +
> > > > > > > > +maintainers:
> > > > > > > > +  - Yuanfang Zhang <quic_yuanfang@quicinc.com>
> > > > > > > > +  - Mao Jinlong <quic_jinlmao@quicinc.com>
> > > > > > > > +  - Jie Gan <quic_jiegan@quicinc.com>
> > > > > > > > +
> > > > > > > > +description:
> > > > > > > > +  The Coresight Control unit controls various Coresight behaviors.
> > > > > > > > +  Used to enable/disable ETR’s data filter function based on trace ID.
> > > > > > > > +
> > > > > > > > +properties:
> > > > > > > > +  compatible:
> > > > > > > > +    const: qcom,coresight-ccu
> > > > > > > > +
> > > > > > > > +  reg:
> > > > > > > > +    maxItems: 1
> > > > > > > > +
> > > > > > > > +  clocks:
> > > > > > > > +    maxItems: 1
> > > > > > > > +
> > > > > > > > +  clock-names:
> > > > > > > > +    items:
> > > > > > > > +      - const: apb_pclk
> > > > > > > > +
> > > > > > > > +  reg-names:
> > > > > > > > +    items:
> > > > > > > > +      - const: ccu-base
> > > > > > > > +
> > > > > > > > +  in-ports:
> > > > > > > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > > > > > > +
> > > > > > > > +    unevaluatedProperties:
> > > > > > > > +      patternProperties:
> > > > > > > > +        '^port(@[0-7])?$':
> > > > > > > > +          description: Input connections from CoreSight Trace bus
> > > > > > > > +          $ref: /schemas/graph.yaml#/properties/port
> > > > > > > > +
> > > > > > > > +          properties:
> > > > > > > > +            qcom,ccu-atid-offset:
> > > > > > > 
> > > > > > > Why do we need this atid offset ? Couldn't this be mapped to the "port"
> > > > > > > number ?
> > > > > > > 
> > > > > > > e.g, input-port 0 on CCU => Offset x
> > > > > > >        input-port 1 on CCU => (Offset x + Size of 1 region)
> > > > > > If the first ATID offset remains constant, it appears to be feasible.
> > > > > > We will consider the possibility of this solution.
> > > > > We just checked the ATID offset varies across different hardware platforms.
> > > > > It defined as 0xf4 on some platforms, and some others defined as 0xf8.
> > > > 
> > > > What do you mean ? The offset where you apply the filter changes across
> > > > different platforms ? or different "tmc-control-unit" implementations ?
> > > > Is this discoverable from the device ? We could use different
> > > > compatibles for different "types" of the "devices". Simply adding
> > > > something in the DT is not the right way.
> > > 
> > > I got your point here. I believe we should consult our hardware engineers first to check it.
> > > We need to figure out the design of ATID offset from hardware perspective. Then we can
> > > propose an alternative approach, such as associating the offset with a compitable value,
> > > to resolve this issue.
> > > 
> > > > 
> > > > > 
> > > > > So I think it should be better to define it in device tree node.
> > > > 
> > > > No. See above.
> > 
> > 
> > Hi Suzuki
> > 
> > There is a new solution for CCU device. We would like to separate one CCU device into several helper
> > devices, each responsible for one feature of the CCU device.
> > 
> > For the data filter feature, we will define the address of the AITD Register that included by CCU in DT
> > as base address of the helper node. So the driver code can easily program the register field with the base address.
> > With this solution, we may need define several helper nodes in DT file to support different features for CCU and each
> > helper device needs a driver to support its behavior.
> > 
> > for example, ATID register for ETR0 with base address 0x10000f8: (tmp name used, TDFU for tmc data filter unit)
> 
> That looks like a hack to me and don't prefer that and there would be
> multiple mappings for a single device and that could complicate device
> resource accounting.

This solution dropped. We will consider another proper solution to address the issue.

> 
> > 
> > TDFU@10000f8 {
> > ...
> > }
> > 
> > ATID register for ETR1 with base address 0x1000108:
> > TDFU@1000108 {
> > ...
> > }
> > 
> > The CCU device supports various features and the data filter feature for ETR is one of those features. How to support
> > those features with one helper_enable function is a serious challenge. That's why we would like to separate it.
> > Meanwhile, This solution can also resolve the offset issue.
> > 
> > We are looking forward your opinions with this proposal.
> 
> We need to know what other functionalities are supported and how that
> will be used ?
>
Sorry for the late reply.
I will introduce the features expected to be merged into the community.

Excepts for data filter feature for ETR, there are three more major features supported by CCU:
1. Byte counter: It is responsible for moving trace data from ETR buffer to main memory(e.g. main DDR RAM or SD card) to prevent
older trace data from being overwritten by newer data. There is a register named QDSS_CS_QDSSCSR_ETRIRQCTRL in CCU device that
controls this feature. The value of this register defines the number of bytes that when moved by the ETR AXI interface. The system will trigger
an interrupt to transfer the data when the data amount exceeds the specified value. ETR must be enabled when use this interrupt function.

for example, When the value 1000 is set in the register (in binary), it indicates that the data amount is 64 bytes.
The interrupt is triggered when the data amount exceeds 64 bytes.
(Binary 1000 equals 8 in decimal. Multiplying this by 8 gives a data amount of 64 bytes.) 

Here is the link for the patch series for Byte counter feature.
https://lore.kernel.org/linux-arm-msm/20230813151253.38128-1-quic_jinlmao@quicinc.com/

2. MSR for TPDM: QC introduced a feature called Switched Always on Debug(SWAO). The component AOSS that utilize the SWAO have designed with 4 CMB TPDM
and 1 DSB TPDM units. Control of bus selection that will reach TPDM CMB are done through CCU.
The registers that designed to control the bus selection:
CS_SWAOCSR_TPDM_PRIO0_CMB_CONTROL
CS_SWAOCSR_TPDM_PRIO1_CMB_CONTROL
CS_SWAOCSR_TPDM_PRIO2_CMB_CONTROL
CS_SWAOCSR_TPDM_PRIO3_CMB_CONTROL

for example, the value programmed into the register represents:
00000 for BCM
00100 for DDRAUX
01000 for VRM
...

3. Direct to USB(D2USB): There is a register called CS_QDSSCSR_QDSS_DIR2USB in CCU to control the D2USB feature. When sets, the data in ETR will directly
output to the USB over the existing system bus infrastructure.

Besides, the CCU also support some features like timestamp, Hardware Event Control for STM.

> In the worst case, you could define register bases for each port in the
> CCU device node.
We will consider this solution as plan b that we do not anticipate implementing.

> 
> TDFU@.. {
> 	reg = <base-address 4K>
> 
> 	<I don't know the standard property name for> =
> 	List of {
> 	<port number>, <Offset from base>,
> 	}
> }
> 
> Suzuki
> 

Thanks,
Jie

Re: [PATCH v2 2/4] dt-bindings: arm: Add binding document for Coresight Control Unit device.

[JieGan]

On Fri, Jul 05, 2024 at 11:07:22AM +0200, Krzysztof Kozlowski wrote:
> On 05/07/2024 11:00, Jie Gan wrote:
> > Add binding document for Coresight Control Unit device.
> 
> <form letter>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC (and consider --no-git-fallback argument). It might
> happen, that command when run on an older kernel, gives you outdated
> entries. Therefore please be sure you base your patches on recent Linux
> kernel.
> 
> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> people, so fix your workflow. Tools might also fail if you work on some
> ancient tree (don't, instead use mainline) or work on fork of kernel
> (don't, instead use mainline). Just use b4 and everything should be
> fine, although remember about `b4 prep --auto-to-cc` if you added new
> patches to the patchset.
> </form letter>
> 
> Or stop developing on some old tree. It's some sort of weird pattern in
> entire Qualcomm Coresight - everything developed on old kernels.
> 
> You must work on latest mainline or maintainer or linux-next tree, not
> some old Qualcomm tree. Your v5.15, v5.19, v6.4 or v6.8 or whatever you
> have there: BIG NOPE.
> 
> > 
> > Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
> > ---
> 
> Subject: it never ends with full stop.
> 
> A nit, subject: drop second/last, redundant "bindings". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> 
> >  .../bindings/arm/qcom,coresight-ccu.yaml      | 87 +++++++++++++++++++
> >  1 file changed, 87 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> > new file mode 100644
> > index 000000000000..9bb8ced393a7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-ccu.yaml
> > @@ -0,0 +1,87 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/arm/qcom,coresight-ccu.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: CoreSight Control Unit
> > +
> > +maintainers:
> > +  - Yuanfang Zhang <quic_yuanfang@quicinc.com>
> > +  - Mao Jinlong <quic_jinlmao@quicinc.com>
> > +  - Jie Gan <quic_jiegan@quicinc.com>
> > +
> > +description:
> > +  The Coresight Control unit controls various Coresight behaviors.
> > +  Used to enable/disable ETR’s data filter function based on trace ID.
> > +
> > +properties:
> > +  compatible:
> > +    const: qcom,coresight-ccu
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    items:
> > +      - const: apb_pclk
> 
> Drop _pclk
Hi, Suzuki,
As Krzysztof proposed, we have to change the clock name from apb_pclk to apb.
According to the new clock name, I updated the inline function coresight_get_enable_apb_pclk
to vote the clock with the new name.

Is that ok with the change? Or any other suggestions?

static inline struct clk *coresight_get_enable_apb_pclk(struct device *dev)
{
        struct clk *pclk;
        int ret;

        pclk = clk_get(dev, "apb_pclk");
        if (IS_ERR(pclk)) {
                pclk = clk_get(dev, "apb");    //added line
                if (IS_ERR(pclk))
                        return NULL;
        }

        ret = clk_prepare_enable(pclk);
        if (ret) {
                clk_put(pclk);
                return ERR_PTR(ret);
        }
        return pclk;
}

> 
> > +
> > +  reg-names:
> 
> Please follow DTS coding style about order of properties.
> 
> > +    items:
> > +      - const: ccu-base
> > +
> > +  in-ports:
> > +    $ref: /schemas/graph.yaml#/properties/ports
> > +
> > +    unevaluatedProperties:
> 
> This was never tested and it cannot reliably work.
> 
> Sorry, this is waste of our time.
> 
> 
> > +      patternProperties:
> > +        '^port(@[0-7])?$':
> > +          description: Input connections from CoreSight Trace bus
> > +          $ref: /schemas/graph.yaml#/properties/port
> > +
> > +          properties:
> > +            qcom,ccu-atid-offset:
> > +              description:
> > +                Offset to the Coresight Control Unit component's ATID register
> > +                that is used by specific TMC ETR. The ATID register can be programed based
> > +                on the trace id to filter out specific trace data which gets into ETR buffer.
> > +              $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - in-ports
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    syscon@1001000 {
> 
> That's not a syscon.
> 
> > +        compatible = "qcom,coresight-ccu";
> > +        reg = <0x1001000 0x1000>;
> > +        reg-names = "ccu-base";
> > +
> 
> Best regards,
> Krzysztof
> 

Thanks,
Jie

Re: [PATCH v2 1/4] Coresight: Add trace_id function to collect trace ID

[Mike Leach]

Hi,

This patch has a number of issues:-

1) The new dynamic trace ID patchset to use per sink ID maps makes
this set unusable. perf supplies a trace ID map for each sink used.
 - see https://lists.linaro.org/archives/list/coresight@lists.linaro.org/thread/JEK7M7HRS57XK4B7CVTVFSHFAFBX4SFG/

2) See etm4_enable_perf() - in the perf context a locked version of
the read trace ID cannot be used - therefore any path that calls
etm4_read_alloc_trace_id() (or equivalent for other sources) in perf
mode may result in lockdep issues.

3) on enable: given a cpu  number, the trace ID can be read from the
id maps rather than needing a new function in ops

4) on disable: trace id can be read directly from the source driver
data - again removing need for a new trace_id function in ops.

Regards

Mike


On Fri, 5 Jul 2024 at 10:01, Jie Gan <quic_jiegan@quicinc.com> wrote:
>
> Add 'trace_id' function pointer in ops. It's responsible for collect the
> trace ID of the device.
>
> Add 'struct cs_sink_data' to store the data used by coresight_enable_path/
> coresight_disable_path. The structure will be transmitted to the helper and
> sink device.
>
> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
> ---
>  drivers/hwtracing/coresight/coresight-core.c  | 53 +++++++++++++++----
>  drivers/hwtracing/coresight/coresight-etb10.c |  3 +-
>  .../hwtracing/coresight/coresight-etm-perf.c  | 34 ++++++++++--
>  .../coresight/coresight-etm3x-core.c          | 14 +++++
>  .../coresight/coresight-etm4x-core.c          | 13 +++++
>  drivers/hwtracing/coresight/coresight-priv.h  | 12 ++++-
>  drivers/hwtracing/coresight/coresight-stm.c   | 13 +++++
>  drivers/hwtracing/coresight/coresight-sysfs.c | 24 +++++++--
>  .../hwtracing/coresight/coresight-tmc-etf.c   |  3 +-
>  .../hwtracing/coresight/coresight-tmc-etr.c   |  6 ++-
>  drivers/hwtracing/coresight/coresight-tpda.c  | 13 +++++
>  drivers/hwtracing/coresight/coresight-trbe.c  |  4 +-
>  drivers/hwtracing/coresight/ultrasoc-smb.c    |  3 +-
>  include/linux/coresight.h                     |  4 ++
>  14 files changed, 174 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index 9fc6f6b863e0..f414e66f4cda 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -297,12 +297,12 @@ static int coresight_enable_helper(struct coresight_device *csdev,
>         return helper_ops(csdev)->enable(csdev, mode, data);
>  }
>
> -static void coresight_disable_helper(struct coresight_device *csdev)
> +static void coresight_disable_helper(struct coresight_device *csdev, void *data)
>  {
> -       helper_ops(csdev)->disable(csdev, NULL);
> +       helper_ops(csdev)->disable(csdev, data);
>  }
>
> -static void coresight_disable_helpers(struct coresight_device *csdev)
> +static void coresight_disable_helpers(struct coresight_device *csdev, void *data)
>  {
>         int i;
>         struct coresight_device *helper;
> @@ -310,7 +310,7 @@ static void coresight_disable_helpers(struct coresight_device *csdev)
>         for (i = 0; i < csdev->pdata->nr_outconns; ++i) {
>                 helper = csdev->pdata->out_conns[i]->dest_dev;
>                 if (helper && coresight_is_helper(helper))
> -                       coresight_disable_helper(helper);
> +                       coresight_disable_helper(helper, data);
>         }
>  }
>
> @@ -327,7 +327,7 @@ static void coresight_disable_helpers(struct coresight_device *csdev)
>  void coresight_disable_source(struct coresight_device *csdev, void *data)
>  {
>         source_ops(csdev)->disable(csdev, data);
> -       coresight_disable_helpers(csdev);
> +       coresight_disable_helpers(csdev, NULL);
>  }
>  EXPORT_SYMBOL_GPL(coresight_disable_source);
>
> @@ -337,7 +337,8 @@ EXPORT_SYMBOL_GPL(coresight_disable_source);
>   * disabled.
>   */
>  static void coresight_disable_path_from(struct list_head *path,
> -                                       struct coresight_node *nd)
> +                                       struct coresight_node *nd,
> +                                       void *sink_data)
>  {
>         u32 type;
>         struct coresight_device *csdev, *parent, *child;
> @@ -382,13 +383,13 @@ static void coresight_disable_path_from(struct list_head *path,
>                 }
>
>                 /* Disable all helpers adjacent along the path last */
> -               coresight_disable_helpers(csdev);
> +               coresight_disable_helpers(csdev, sink_data);
>         }
>  }
>
> -void coresight_disable_path(struct list_head *path)
> +void coresight_disable_path(struct list_head *path, void *sink_data)
>  {
> -       coresight_disable_path_from(path, NULL);
> +       coresight_disable_path_from(path, NULL, sink_data);
>  }
>  EXPORT_SYMBOL_GPL(coresight_disable_path);
>
> @@ -468,10 +469,42 @@ int coresight_enable_path(struct list_head *path, enum cs_mode mode,
>  out:
>         return ret;
>  err:
> -       coresight_disable_path_from(path, nd);
> +       coresight_disable_path_from(path, nd, sink_data);
>         goto out;
>  }
>
> +int coresight_read_traceid(struct list_head *path)
> +{
> +       int trace_id, type;
> +       struct coresight_device *csdev;
> +       struct coresight_node *nd;
> +
> +       list_for_each_entry(nd, path, link) {
> +               csdev = nd->csdev;
> +               type = csdev->type;
> +
> +               switch(type) {
> +                       case CORESIGHT_DEV_TYPE_SOURCE:
> +                               if (source_ops(csdev)->trace_id != NULL) {
> +                                       trace_id = source_ops(csdev)->trace_id(csdev);
> +                                       if (trace_id > 0)
> +                                               return trace_id;
> +                               }
> +                               break;
> +                       case CORESIGHT_DEV_TYPE_LINK:
> +                               if (link_ops(csdev)->trace_id != NULL) {
> +                                       trace_id = link_ops(csdev)->trace_id(csdev);
> +                                       if (trace_id > 0)
> +                                               return trace_id;
> +                               }
> +                               break;
> +                       default:
> +                               break;
> +               }
> +       }
> +       return -EINVAL;
> +}
> +
>  struct coresight_device *coresight_get_sink(struct list_head *path)
>  {
>         struct coresight_device *csdev;
> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
> index 7edd3f1d0d46..05e620529c14 100644
> --- a/drivers/hwtracing/coresight/coresight-etb10.c
> +++ b/drivers/hwtracing/coresight/coresight-etb10.c
> @@ -173,7 +173,8 @@ static int etb_enable_perf(struct coresight_device *csdev, void *data)
>         pid_t pid;
>         unsigned long flags;
>         struct etb_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> -       struct perf_output_handle *handle = data;
> +       struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
> +       struct perf_output_handle *handle = sink_data->handle;
>         struct cs_buffers *buf = etm_perf_sink_config(handle);
>
>         spin_lock_irqsave(&drvdata->spinlock, flags);
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index c0c60e6a1703..8b155765b959 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -452,6 +452,7 @@ static void etm_event_start(struct perf_event *event, int flags)
>         struct perf_output_handle *handle = &ctxt->handle;
>         struct coresight_device *sink, *csdev = per_cpu(csdev_src, cpu);
>         struct list_head *path;
> +       struct cs_sink_data *sink_data = NULL;
>         u64 hw_id;
>
>         if (!csdev)
> @@ -490,9 +491,18 @@ static void etm_event_start(struct perf_event *event, int flags)
>         if (WARN_ON_ONCE(!sink))
>                 goto fail_end_stop;
>
> +       sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL);
> +       if (!sink_data)
> +               goto fail_end_stop;
> +
> +       sink_data->sink = sink;
> +       sink_data->traceid = coresight_read_traceid(path);
> +       sink_data->handle = handle;
>         /* Nothing will happen without a path */
> -       if (coresight_enable_path(path, CS_MODE_PERF, handle))
> +       if (coresight_enable_path(path, CS_MODE_PERF, sink_data)) {
> +               kfree(sink_data);
>                 goto fail_end_stop;
> +       }
>
>         /* Finally enable the tracer */
>         if (source_ops(csdev)->enable(csdev, event, CS_MODE_PERF))
> @@ -511,6 +521,7 @@ static void etm_event_start(struct perf_event *event, int flags)
>                 perf_report_aux_output_id(event, hw_id);
>         }
>
> +       kfree(sink_data);
>  out:
>         /* Tell the perf core the event is alive */
>         event->hw.state = 0;
> @@ -519,7 +530,8 @@ static void etm_event_start(struct perf_event *event, int flags)
>         return;
>
>  fail_disable_path:
> -       coresight_disable_path(path);
> +       coresight_disable_path(path, sink_data);
> +       kfree(sink_data);
>  fail_end_stop:
>         /*
>          * Check if the handle is still associated with the event,
> @@ -544,6 +556,7 @@ static void etm_event_stop(struct perf_event *event, int mode)
>         struct perf_output_handle *handle = &ctxt->handle;
>         struct etm_event_data *event_data;
>         struct list_head *path;
> +       struct cs_sink_data *sink_data = NULL;
>
>         /*
>          * If we still have access to the event_data via handle,
> @@ -588,6 +601,10 @@ static void etm_event_stop(struct perf_event *event, int mode)
>         if (!sink)
>                 return;
>
> +       sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL);
> +       if (!sink_data)
> +               return;
> +
>         /* stop tracer */
>         coresight_disable_source(csdev, event);
>
> @@ -601,12 +618,16 @@ static void etm_event_stop(struct perf_event *event, int mode)
>          * have to do anything here.
>          */
>         if (handle->event && (mode & PERF_EF_UPDATE)) {
> -               if (WARN_ON_ONCE(handle->event != event))
> +               if (WARN_ON_ONCE(handle->event != event)) {
> +                       kfree(sink_data);
>                         return;
> +               }
>
>                 /* update trace information */
> -               if (!sink_ops(sink)->update_buffer)
> +               if (!sink_ops(sink)->update_buffer) {
> +                       kfree(sink_data);
>                         return;
> +               }
>
>                 size = sink_ops(sink)->update_buffer(sink, handle,
>                                               event_data->snk_config);
> @@ -627,8 +648,11 @@ static void etm_event_stop(struct perf_event *event, int mode)
>                         WARN_ON(size);
>         }
>
> +       sink_data->sink = sink;
> +       sink_data->traceid = coresight_read_traceid(path);
>         /* Disabling the path make its elements available to other sessions */
> -       coresight_disable_path(path);
> +       coresight_disable_path(path, sink_data);
> +       kfree(sink_data);
>  }
>
>  static int etm_event_add(struct perf_event *event, int mode)
> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> index 8b362605d242..27e973749050 100644
> --- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> @@ -696,10 +696,24 @@ static void etm_disable(struct coresight_device *csdev,
>                 coresight_set_mode(csdev, CS_MODE_DISABLED);
>  }
>
> +static int etm_trace_id(struct coresight_device *csdev)
> +{
> +       struct etm_drvdata *drvdata;
> +
> +       if (csdev == NULL)
> +               return -EINVAL;
> +
> +       drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> +       return etm_read_alloc_trace_id(drvdata);
> +}
> +
> +
>  static const struct coresight_ops_source etm_source_ops = {
>         .cpu_id         = etm_cpu_id,
>         .enable         = etm_enable,
>         .disable        = etm_disable,
> +       .trace_id       = etm_trace_id,
>  };
>
>  static const struct coresight_ops etm_cs_ops = {
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index bf01f01964cf..8c3e9bfb9a9c 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -1024,10 +1024,23 @@ static void etm4_disable(struct coresight_device *csdev,
>                 coresight_set_mode(csdev, CS_MODE_DISABLED);
>  }
>
> +static int etm4_trace_id(struct coresight_device *csdev)
> +{
> +       struct etmv4_drvdata *drvdata;
> +
> +       if (csdev == NULL)
> +               return -EINVAL;
> +
> +       drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> +       return etm4_read_alloc_trace_id(drvdata);
> +}
> +
>  static const struct coresight_ops_source etm4_source_ops = {
>         .cpu_id         = etm4_cpu_id,
>         .enable         = etm4_enable,
>         .disable        = etm4_disable,
> +       .trace_id       = etm4_trace_id,
>  };
>
>  static const struct coresight_ops etm4_cs_ops = {
> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> index 61a46d3bdcc8..e2576531f796 100644
> --- a/drivers/hwtracing/coresight/coresight-priv.h
> +++ b/drivers/hwtracing/coresight/coresight-priv.h
> @@ -105,6 +105,15 @@ struct cs_buffers {
>         void                    **data_pages;
>  };
>
> +/**
> + * struct cs_sink_data - data used by coresight_enable_path/coresight_disable_path
> + */
> +struct cs_sink_data {
> +       struct perf_output_handle       *handle;
> +       struct coresight_device         *sink;
> +       u32                             traceid;
> +};
> +
>  static inline void coresight_insert_barrier_packet(void *buf)
>  {
>         if (buf)
> @@ -129,9 +138,10 @@ static inline void CS_UNLOCK(void __iomem *addr)
>         } while (0);
>  }
>
> -void coresight_disable_path(struct list_head *path);
> +void coresight_disable_path(struct list_head *path, void *sink_data);
>  int coresight_enable_path(struct list_head *path, enum cs_mode mode,
>                           void *sink_data);
> +int coresight_read_traceid(struct list_head *path);
>  struct coresight_device *coresight_get_sink(struct list_head *path);
>  struct coresight_device *coresight_get_sink_by_id(u32 id);
>  struct coresight_device *
> diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
> index 117dbb484543..3817743fc0c6 100644
> --- a/drivers/hwtracing/coresight/coresight-stm.c
> +++ b/drivers/hwtracing/coresight/coresight-stm.c
> @@ -280,9 +280,22 @@ static void stm_disable(struct coresight_device *csdev,
>         }
>  }
>
> +static int stm_trace_id(struct coresight_device *csdev)
> +{
> +       struct stm_drvdata *drvdata;
> +
> +       if (csdev == NULL)
> +               return -EINVAL;
> +
> +       drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> +       return drvdata->traceid;
> +}
> +
>  static const struct coresight_ops_source stm_source_ops = {
>         .enable         = stm_enable,
>         .disable        = stm_disable,
> +       .trace_id       = stm_trace_id,
>  };
>
>  static const struct coresight_ops stm_cs_ops = {
> diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c
> index 1e67cc7758d7..a95afc890587 100644
> --- a/drivers/hwtracing/coresight/coresight-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-sysfs.c
> @@ -167,6 +167,7 @@ int coresight_enable_sysfs(struct coresight_device *csdev)
>         int cpu, ret = 0;
>         struct coresight_device *sink;
>         struct list_head *path;
> +       struct cs_sink_data *sink_data;
>         enum coresight_dev_subtype_source subtype;
>         u32 hash;
>
> @@ -208,7 +209,14 @@ int coresight_enable_sysfs(struct coresight_device *csdev)
>                 goto out;
>         }
>
> -       ret = coresight_enable_path(path, CS_MODE_SYSFS, NULL);
> +       sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL);
> +       if (!sink_data) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }
> +       sink_data->traceid = coresight_read_traceid(path);
> +       sink_data->sink = sink;
> +       ret = coresight_enable_path(path, CS_MODE_SYSFS, sink_data);
>         if (ret)
>                 goto err_path;
>
> @@ -245,15 +253,17 @@ int coresight_enable_sysfs(struct coresight_device *csdev)
>                 break;
>         }
>
> +       kfree(sink_data);
>  out:
>         mutex_unlock(&coresight_mutex);
>         return ret;
>
>  err_source:
> -       coresight_disable_path(path);
> +       coresight_disable_path(path, sink_data);
>
>  err_path:
>         coresight_release_path(path);
> +       kfree(sink_data);
>         goto out;
>  }
>  EXPORT_SYMBOL_GPL(coresight_enable_sysfs);
> @@ -262,6 +272,7 @@ void coresight_disable_sysfs(struct coresight_device *csdev)
>  {
>         int cpu, ret;
>         struct list_head *path = NULL;
> +       struct cs_sink_data *sink_data = NULL;
>         u32 hash;
>
>         mutex_lock(&coresight_mutex);
> @@ -273,6 +284,10 @@ void coresight_disable_sysfs(struct coresight_device *csdev)
>         if (!coresight_disable_source_sysfs(csdev, NULL))
>                 goto out;
>
> +       sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL);
> +       if (!sink_data)
> +               goto out;
> +
>         switch (csdev->subtype.source_subtype) {
>         case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC:
>                 cpu = source_ops(csdev)->cpu_id(csdev);
> @@ -296,8 +311,11 @@ void coresight_disable_sysfs(struct coresight_device *csdev)
>                 break;
>         }
>
> -       coresight_disable_path(path);
> +       sink_data->sink = coresight_find_activated_sysfs_sink(csdev);
> +       sink_data->traceid = coresight_read_traceid(path);
> +       coresight_disable_path(path, sink_data);
>         coresight_release_path(path);
> +       kfree(sink_data);
>
>  out:
>         mutex_unlock(&coresight_mutex);
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> index d4f641cd9de6..7dc536eba3e2 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> @@ -250,7 +250,8 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, void *data)
>         pid_t pid;
>         unsigned long flags;
>         struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> -       struct perf_output_handle *handle = data;
> +       struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
> +       struct perf_output_handle *handle = sink_data->handle;
>         struct cs_buffers *buf = etm_perf_sink_config(handle);
>
>         spin_lock_irqsave(&drvdata->spinlock, flags);
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index e75428fa1592..0c24520645e2 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -1253,7 +1253,8 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
>  struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,
>                                    enum cs_mode mode, void *data)
>  {
> -       struct perf_output_handle *handle = data;
> +       struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
> +       struct perf_output_handle *handle = sink_data->handle;
>         struct etr_perf_buffer *etr_perf;
>
>         switch (mode) {
> @@ -1647,7 +1648,8 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
>         pid_t pid;
>         unsigned long flags;
>         struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> -       struct perf_output_handle *handle = data;
> +       struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
> +       struct perf_output_handle *handle = sink_data->handle;
>         struct etr_perf_buffer *etr_perf = etm_perf_sink_config(handle);
>
>         spin_lock_irqsave(&drvdata->spinlock, flags);
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
> index bfca103f9f84..20f0ab73159c 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.c
> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
> @@ -232,9 +232,22 @@ static void tpda_disable(struct coresight_device *csdev,
>         dev_dbg(drvdata->dev, "TPDA inport %d disabled\n", in->dest_port);
>  }
>
> +static int tpda_trace_id(struct coresight_device *csdev)
> +{
> +       struct tpda_drvdata *drvdata;
> +
> +       if (csdev == NULL)
> +               return -EINVAL;
> +
> +       drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> +       return drvdata->atid;
> +}
> +
>  static const struct coresight_ops_link tpda_link_ops = {
>         .enable         = tpda_enable,
>         .disable        = tpda_disable,
> +       .trace_id       = tpda_trace_id,
>  };
>
>  static const struct coresight_ops tpda_cs_ops = {
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index 96a32b213669..7f4560b067a8 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -21,6 +21,7 @@
>
>  #include "coresight-self-hosted-trace.h"
>  #include "coresight-trbe.h"
> +#include "coresight-priv.h"
>
>  #define PERF_IDX2OFF(idx, buf) ((idx) % ((buf)->nr_pages << PAGE_SHIFT))
>
> @@ -1012,7 +1013,8 @@ static int arm_trbe_enable(struct coresight_device *csdev, enum cs_mode mode,
>  {
>         struct trbe_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>         struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
> -       struct perf_output_handle *handle = data;
> +       struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
> +       struct perf_output_handle *handle = sink_data->handle;
>         struct trbe_buf *buf = etm_perf_sink_config(handle);
>
>         WARN_ON(cpudata->cpu != smp_processor_id());
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> index f9ebf20c91e6..92d8a9fb844e 100644
> --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
> @@ -217,7 +217,8 @@ static void smb_enable_sysfs(struct coresight_device *csdev)
>  static int smb_enable_perf(struct coresight_device *csdev, void *data)
>  {
>         struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
> -       struct perf_output_handle *handle = data;
> +       struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
> +       struct perf_output_handle *handle = sink_data->handle;
>         struct cs_buffers *buf = etm_perf_sink_config(handle);
>         pid_t pid;
>
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index f09ace92176e..fb1c225076a5 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -344,6 +344,7 @@ struct coresight_ops_sink {
>   * Operations available for links.
>   * @enable:    enables flow between iport and oport.
>   * @disable:   disables flow between iport and oport.
> + * @trace_id:  Collect the traceid.
>   */
>  struct coresight_ops_link {
>         int (*enable)(struct coresight_device *csdev,
> @@ -352,6 +353,7 @@ struct coresight_ops_link {
>         void (*disable)(struct coresight_device *csdev,
>                         struct coresight_connection *in,
>                         struct coresight_connection *out);
> +       int (*trace_id)(struct coresight_device *csdev);
>  };
>
>  /**
> @@ -361,6 +363,7 @@ struct coresight_ops_link {
>   *             is associated to.
>   * @enable:    enables tracing for a source.
>   * @disable:   disables tracing for a source.
> + * @trace_id:  collect the traceid.
>   */
>  struct coresight_ops_source {
>         int (*cpu_id)(struct coresight_device *csdev);
> @@ -368,6 +371,7 @@ struct coresight_ops_source {
>                       enum cs_mode mode);
>         void (*disable)(struct coresight_device *csdev,
>                         struct perf_event *event);
> +       int (*trace_id)(struct coresight_device *csdev);
>  };
>
>  /**
> --
> 2.34.1
>


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

Re: [PATCH v2 1/4] Coresight: Add trace_id function to collect trace ID

[JieGan]

On Tue, Jul 23, 2024 at 11:30:02AM +0100, Mike Leach wrote:
> Hi,
> 
> This patch has a number of issues:-
> 
> 1) The new dynamic trace ID patchset to use per sink ID maps makes
> this set unusable. perf supplies a trace ID map for each sink used.
>  - see https://lists.linaro.org/archives/list/coresight@lists.linaro.org/thread/JEK7M7HRS57XK4B7CVTVFSHFAFBX4SFG/
> 
> 2) See etm4_enable_perf() - in the perf context a locked version of
> the read trace ID cannot be used - therefore any path that calls
> etm4_read_alloc_trace_id() (or equivalent for other sources) in perf
> mode may result in lockdep issues.
> 
> 3) on enable: given a cpu  number, the trace ID can be read from the
> id maps rather than needing a new function in ops
> 
> 4) on disable: trace id can be read directly from the source driver
> data - again removing need for a new trace_id function in ops.
> 
> Regards
> 
> Mike
Hi Mike

Thanks for comment. I will check James's patchset.
If possible, I will use the per sink ID maps to retrieving the trace ID.

Thanks,
Jie

> 
> 
> On Fri, 5 Jul 2024 at 10:01, Jie Gan <quic_jiegan@quicinc.com> wrote:
> >
> > Add 'trace_id' function pointer in ops. It's responsible for collect the
> > trace ID of the device.
> >
> > Add 'struct cs_sink_data' to store the data used by coresight_enable_path/
> > coresight_disable_path. The structure will be transmitted to the helper and
> > sink device.
> >
> > Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
> > ---
> >  drivers/hwtracing/coresight/coresight-core.c  | 53 +++++++++++++++----
> >  drivers/hwtracing/coresight/coresight-etb10.c |  3 +-
> >  .../hwtracing/coresight/coresight-etm-perf.c  | 34 ++++++++++--
> >  .../coresight/coresight-etm3x-core.c          | 14 +++++
> >  .../coresight/coresight-etm4x-core.c          | 13 +++++
> >  drivers/hwtracing/coresight/coresight-priv.h  | 12 ++++-
> >  drivers/hwtracing/coresight/coresight-stm.c   | 13 +++++
> >  drivers/hwtracing/coresight/coresight-sysfs.c | 24 +++++++--
> >  .../hwtracing/coresight/coresight-tmc-etf.c   |  3 +-
> >  .../hwtracing/coresight/coresight-tmc-etr.c   |  6 ++-
> >  drivers/hwtracing/coresight/coresight-tpda.c  | 13 +++++
> >  drivers/hwtracing/coresight/coresight-trbe.c  |  4 +-
> >  drivers/hwtracing/coresight/ultrasoc-smb.c    |  3 +-
> >  include/linux/coresight.h                     |  4 ++
> >  14 files changed, 174 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> > index 9fc6f6b863e0..f414e66f4cda 100644
> > --- a/drivers/hwtracing/coresight/coresight-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-core.c
> > @@ -297,12 +297,12 @@ static int coresight_enable_helper(struct coresight_device *csdev,
> >         return helper_ops(csdev)->enable(csdev, mode, data);
> >  }
> >
> > -static void coresight_disable_helper(struct coresight_device *csdev)
> > +static void coresight_disable_helper(struct coresight_device *csdev, void *data)
> >  {
> > -       helper_ops(csdev)->disable(csdev, NULL);
> > +       helper_ops(csdev)->disable(csdev, data);
> >  }
> >
> > -static void coresight_disable_helpers(struct coresight_device *csdev)
> > +static void coresight_disable_helpers(struct coresight_device *csdev, void *data)
> >  {
> >         int i;
> >         struct coresight_device *helper;
> > @@ -310,7 +310,7 @@ static void coresight_disable_helpers(struct coresight_device *csdev)
> >         for (i = 0; i < csdev->pdata->nr_outconns; ++i) {
> >                 helper = csdev->pdata->out_conns[i]->dest_dev;
> >                 if (helper && coresight_is_helper(helper))
> > -                       coresight_disable_helper(helper);
> > +                       coresight_disable_helper(helper, data);
> >         }
> >  }
> >
> > @@ -327,7 +327,7 @@ static void coresight_disable_helpers(struct coresight_device *csdev)
> >  void coresight_disable_source(struct coresight_device *csdev, void *data)
> >  {
> >         source_ops(csdev)->disable(csdev, data);
> > -       coresight_disable_helpers(csdev);
> > +       coresight_disable_helpers(csdev, NULL);
> >  }
> >  EXPORT_SYMBOL_GPL(coresight_disable_source);
> >
> > @@ -337,7 +337,8 @@ EXPORT_SYMBOL_GPL(coresight_disable_source);
> >   * disabled.
> >   */
> >  static void coresight_disable_path_from(struct list_head *path,
> > -                                       struct coresight_node *nd)
> > +                                       struct coresight_node *nd,
> > +                                       void *sink_data)
> >  {
> >         u32 type;
> >         struct coresight_device *csdev, *parent, *child;
> > @@ -382,13 +383,13 @@ static void coresight_disable_path_from(struct list_head *path,
> >                 }
> >
> >                 /* Disable all helpers adjacent along the path last */
> > -               coresight_disable_helpers(csdev);
> > +               coresight_disable_helpers(csdev, sink_data);
> >         }
> >  }
> >
> > -void coresight_disable_path(struct list_head *path)
> > +void coresight_disable_path(struct list_head *path, void *sink_data)
> >  {
> > -       coresight_disable_path_from(path, NULL);
> > +       coresight_disable_path_from(path, NULL, sink_data);
> >  }
> >  EXPORT_SYMBOL_GPL(coresight_disable_path);
> >
> > @@ -468,10 +469,42 @@ int coresight_enable_path(struct list_head *path, enum cs_mode mode,
> >  out:
> >         return ret;
> >  err:
> > -       coresight_disable_path_from(path, nd);
> > +       coresight_disable_path_from(path, nd, sink_data);
> >         goto out;
> >  }
> >
> > +int coresight_read_traceid(struct list_head *path)
> > +{
> > +       int trace_id, type;
> > +       struct coresight_device *csdev;
> > +       struct coresight_node *nd;
> > +
> > +       list_for_each_entry(nd, path, link) {
> > +               csdev = nd->csdev;
> > +               type = csdev->type;
> > +
> > +               switch(type) {
> > +                       case CORESIGHT_DEV_TYPE_SOURCE:
> > +                               if (source_ops(csdev)->trace_id != NULL) {
> > +                                       trace_id = source_ops(csdev)->trace_id(csdev);
> > +                                       if (trace_id > 0)
> > +                                               return trace_id;
> > +                               }
> > +                               break;
> > +                       case CORESIGHT_DEV_TYPE_LINK:
> > +                               if (link_ops(csdev)->trace_id != NULL) {
> > +                                       trace_id = link_ops(csdev)->trace_id(csdev);
> > +                                       if (trace_id > 0)
> > +                                               return trace_id;
> > +                               }
> > +                               break;
> > +                       default:
> > +                               break;
> > +               }
> > +       }
> > +       return -EINVAL;
> > +}
> > +
> >  struct coresight_device *coresight_get_sink(struct list_head *path)
> >  {
> >         struct coresight_device *csdev;
> > diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
> > index 7edd3f1d0d46..05e620529c14 100644
> > --- a/drivers/hwtracing/coresight/coresight-etb10.c
> > +++ b/drivers/hwtracing/coresight/coresight-etb10.c
> > @@ -173,7 +173,8 @@ static int etb_enable_perf(struct coresight_device *csdev, void *data)
> >         pid_t pid;
> >         unsigned long flags;
> >         struct etb_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> > -       struct perf_output_handle *handle = data;
> > +       struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
> > +       struct perf_output_handle *handle = sink_data->handle;
> >         struct cs_buffers *buf = etm_perf_sink_config(handle);
> >
> >         spin_lock_irqsave(&drvdata->spinlock, flags);
> > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > index c0c60e6a1703..8b155765b959 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > @@ -452,6 +452,7 @@ static void etm_event_start(struct perf_event *event, int flags)
> >         struct perf_output_handle *handle = &ctxt->handle;
> >         struct coresight_device *sink, *csdev = per_cpu(csdev_src, cpu);
> >         struct list_head *path;
> > +       struct cs_sink_data *sink_data = NULL;
> >         u64 hw_id;
> >
> >         if (!csdev)
> > @@ -490,9 +491,18 @@ static void etm_event_start(struct perf_event *event, int flags)
> >         if (WARN_ON_ONCE(!sink))
> >                 goto fail_end_stop;
> >
> > +       sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL);
> > +       if (!sink_data)
> > +               goto fail_end_stop;
> > +
> > +       sink_data->sink = sink;
> > +       sink_data->traceid = coresight_read_traceid(path);
> > +       sink_data->handle = handle;
> >         /* Nothing will happen without a path */
> > -       if (coresight_enable_path(path, CS_MODE_PERF, handle))
> > +       if (coresight_enable_path(path, CS_MODE_PERF, sink_data)) {
> > +               kfree(sink_data);
> >                 goto fail_end_stop;
> > +       }
> >
> >         /* Finally enable the tracer */
> >         if (source_ops(csdev)->enable(csdev, event, CS_MODE_PERF))
> > @@ -511,6 +521,7 @@ static void etm_event_start(struct perf_event *event, int flags)
> >                 perf_report_aux_output_id(event, hw_id);
> >         }
> >
> > +       kfree(sink_data);
> >  out:
> >         /* Tell the perf core the event is alive */
> >         event->hw.state = 0;
> > @@ -519,7 +530,8 @@ static void etm_event_start(struct perf_event *event, int flags)
> >         return;
> >
> >  fail_disable_path:
> > -       coresight_disable_path(path);
> > +       coresight_disable_path(path, sink_data);
> > +       kfree(sink_data);
> >  fail_end_stop:
> >         /*
> >          * Check if the handle is still associated with the event,
> > @@ -544,6 +556,7 @@ static void etm_event_stop(struct perf_event *event, int mode)
> >         struct perf_output_handle *handle = &ctxt->handle;
> >         struct etm_event_data *event_data;
> >         struct list_head *path;
> > +       struct cs_sink_data *sink_data = NULL;
> >
> >         /*
> >          * If we still have access to the event_data via handle,
> > @@ -588,6 +601,10 @@ static void etm_event_stop(struct perf_event *event, int mode)
> >         if (!sink)
> >                 return;
> >
> > +       sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL);
> > +       if (!sink_data)
> > +               return;
> > +
> >         /* stop tracer */
> >         coresight_disable_source(csdev, event);
> >
> > @@ -601,12 +618,16 @@ static void etm_event_stop(struct perf_event *event, int mode)
> >          * have to do anything here.
> >          */
> >         if (handle->event && (mode & PERF_EF_UPDATE)) {
> > -               if (WARN_ON_ONCE(handle->event != event))
> > +               if (WARN_ON_ONCE(handle->event != event)) {
> > +                       kfree(sink_data);
> >                         return;
> > +               }
> >
> >                 /* update trace information */
> > -               if (!sink_ops(sink)->update_buffer)
> > +               if (!sink_ops(sink)->update_buffer) {
> > +                       kfree(sink_data);
> >                         return;
> > +               }
> >
> >                 size = sink_ops(sink)->update_buffer(sink, handle,
> >                                               event_data->snk_config);
> > @@ -627,8 +648,11 @@ static void etm_event_stop(struct perf_event *event, int mode)
> >                         WARN_ON(size);
> >         }
> >
> > +       sink_data->sink = sink;
> > +       sink_data->traceid = coresight_read_traceid(path);
> >         /* Disabling the path make its elements available to other sessions */
> > -       coresight_disable_path(path);
> > +       coresight_disable_path(path, sink_data);
> > +       kfree(sink_data);
> >  }
> >
> >  static int etm_event_add(struct perf_event *event, int mode)
> > diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> > index 8b362605d242..27e973749050 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> > @@ -696,10 +696,24 @@ static void etm_disable(struct coresight_device *csdev,
> >                 coresight_set_mode(csdev, CS_MODE_DISABLED);
> >  }
> >
> > +static int etm_trace_id(struct coresight_device *csdev)
> > +{
> > +       struct etm_drvdata *drvdata;
> > +
> > +       if (csdev == NULL)
> > +               return -EINVAL;
> > +
> > +       drvdata = dev_get_drvdata(csdev->dev.parent);
> > +
> > +       return etm_read_alloc_trace_id(drvdata);
> > +}
> > +
> > +
> >  static const struct coresight_ops_source etm_source_ops = {
> >         .cpu_id         = etm_cpu_id,
> >         .enable         = etm_enable,
> >         .disable        = etm_disable,
> > +       .trace_id       = etm_trace_id,
> >  };
> >
> >  static const struct coresight_ops etm_cs_ops = {
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > index bf01f01964cf..8c3e9bfb9a9c 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > @@ -1024,10 +1024,23 @@ static void etm4_disable(struct coresight_device *csdev,
> >                 coresight_set_mode(csdev, CS_MODE_DISABLED);
> >  }
> >
> > +static int etm4_trace_id(struct coresight_device *csdev)
> > +{
> > +       struct etmv4_drvdata *drvdata;
> > +
> > +       if (csdev == NULL)
> > +               return -EINVAL;
> > +
> > +       drvdata = dev_get_drvdata(csdev->dev.parent);
> > +
> > +       return etm4_read_alloc_trace_id(drvdata);
> > +}
> > +
> >  static const struct coresight_ops_source etm4_source_ops = {
> >         .cpu_id         = etm4_cpu_id,
> >         .enable         = etm4_enable,
> >         .disable        = etm4_disable,
> > +       .trace_id       = etm4_trace_id,
> >  };
> >
> >  static const struct coresight_ops etm4_cs_ops = {
> > diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> > index 61a46d3bdcc8..e2576531f796 100644
> > --- a/drivers/hwtracing/coresight/coresight-priv.h
> > +++ b/drivers/hwtracing/coresight/coresight-priv.h
> > @@ -105,6 +105,15 @@ struct cs_buffers {
> >         void                    **data_pages;
> >  };
> >
> > +/**
> > + * struct cs_sink_data - data used by coresight_enable_path/coresight_disable_path
> > + */
> > +struct cs_sink_data {
> > +       struct perf_output_handle       *handle;
> > +       struct coresight_device         *sink;
> > +       u32                             traceid;
> > +};
> > +
> >  static inline void coresight_insert_barrier_packet(void *buf)
> >  {
> >         if (buf)
> > @@ -129,9 +138,10 @@ static inline void CS_UNLOCK(void __iomem *addr)
> >         } while (0);
> >  }
> >
> > -void coresight_disable_path(struct list_head *path);
> > +void coresight_disable_path(struct list_head *path, void *sink_data);
> >  int coresight_enable_path(struct list_head *path, enum cs_mode mode,
> >                           void *sink_data);
> > +int coresight_read_traceid(struct list_head *path);
> >  struct coresight_device *coresight_get_sink(struct list_head *path);
> >  struct coresight_device *coresight_get_sink_by_id(u32 id);
> >  struct coresight_device *
> > diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
> > index 117dbb484543..3817743fc0c6 100644
> > --- a/drivers/hwtracing/coresight/coresight-stm.c
> > +++ b/drivers/hwtracing/coresight/coresight-stm.c
> > @@ -280,9 +280,22 @@ static void stm_disable(struct coresight_device *csdev,
> >         }
> >  }
> >
> > +static int stm_trace_id(struct coresight_device *csdev)
> > +{
> > +       struct stm_drvdata *drvdata;
> > +
> > +       if (csdev == NULL)
> > +               return -EINVAL;
> > +
> > +       drvdata = dev_get_drvdata(csdev->dev.parent);
> > +
> > +       return drvdata->traceid;
> > +}
> > +
> >  static const struct coresight_ops_source stm_source_ops = {
> >         .enable         = stm_enable,
> >         .disable        = stm_disable,
> > +       .trace_id       = stm_trace_id,
> >  };
> >
> >  static const struct coresight_ops stm_cs_ops = {
> > diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c
> > index 1e67cc7758d7..a95afc890587 100644
> > --- a/drivers/hwtracing/coresight/coresight-sysfs.c
> > +++ b/drivers/hwtracing/coresight/coresight-sysfs.c
> > @@ -167,6 +167,7 @@ int coresight_enable_sysfs(struct coresight_device *csdev)
> >         int cpu, ret = 0;
> >         struct coresight_device *sink;
> >         struct list_head *path;
> > +       struct cs_sink_data *sink_data;
> >         enum coresight_dev_subtype_source subtype;
> >         u32 hash;
> >
> > @@ -208,7 +209,14 @@ int coresight_enable_sysfs(struct coresight_device *csdev)
> >                 goto out;
> >         }
> >
> > -       ret = coresight_enable_path(path, CS_MODE_SYSFS, NULL);
> > +       sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL);
> > +       if (!sink_data) {
> > +               ret = -ENOMEM;
> > +               goto out;
> > +       }
> > +       sink_data->traceid = coresight_read_traceid(path);
> > +       sink_data->sink = sink;
> > +       ret = coresight_enable_path(path, CS_MODE_SYSFS, sink_data);
> >         if (ret)
> >                 goto err_path;
> >
> > @@ -245,15 +253,17 @@ int coresight_enable_sysfs(struct coresight_device *csdev)
> >                 break;
> >         }
> >
> > +       kfree(sink_data);
> >  out:
> >         mutex_unlock(&coresight_mutex);
> >         return ret;
> >
> >  err_source:
> > -       coresight_disable_path(path);
> > +       coresight_disable_path(path, sink_data);
> >
> >  err_path:
> >         coresight_release_path(path);
> > +       kfree(sink_data);
> >         goto out;
> >  }
> >  EXPORT_SYMBOL_GPL(coresight_enable_sysfs);
> > @@ -262,6 +272,7 @@ void coresight_disable_sysfs(struct coresight_device *csdev)
> >  {
> >         int cpu, ret;
> >         struct list_head *path = NULL;
> > +       struct cs_sink_data *sink_data = NULL;
> >         u32 hash;
> >
> >         mutex_lock(&coresight_mutex);
> > @@ -273,6 +284,10 @@ void coresight_disable_sysfs(struct coresight_device *csdev)
> >         if (!coresight_disable_source_sysfs(csdev, NULL))
> >                 goto out;
> >
> > +       sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL);
> > +       if (!sink_data)
> > +               goto out;
> > +
> >         switch (csdev->subtype.source_subtype) {
> >         case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC:
> >                 cpu = source_ops(csdev)->cpu_id(csdev);
> > @@ -296,8 +311,11 @@ void coresight_disable_sysfs(struct coresight_device *csdev)
> >                 break;
> >         }
> >
> > -       coresight_disable_path(path);
> > +       sink_data->sink = coresight_find_activated_sysfs_sink(csdev);
> > +       sink_data->traceid = coresight_read_traceid(path);
> > +       coresight_disable_path(path, sink_data);
> >         coresight_release_path(path);
> > +       kfree(sink_data);
> >
> >  out:
> >         mutex_unlock(&coresight_mutex);
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > index d4f641cd9de6..7dc536eba3e2 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > @@ -250,7 +250,8 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, void *data)
> >         pid_t pid;
> >         unsigned long flags;
> >         struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> > -       struct perf_output_handle *handle = data;
> > +       struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
> > +       struct perf_output_handle *handle = sink_data->handle;
> >         struct cs_buffers *buf = etm_perf_sink_config(handle);
> >
> >         spin_lock_irqsave(&drvdata->spinlock, flags);
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > index e75428fa1592..0c24520645e2 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > @@ -1253,7 +1253,8 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
> >  struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,
> >                                    enum cs_mode mode, void *data)
> >  {
> > -       struct perf_output_handle *handle = data;
> > +       struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
> > +       struct perf_output_handle *handle = sink_data->handle;
> >         struct etr_perf_buffer *etr_perf;
> >
> >         switch (mode) {
> > @@ -1647,7 +1648,8 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
> >         pid_t pid;
> >         unsigned long flags;
> >         struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> > -       struct perf_output_handle *handle = data;
> > +       struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
> > +       struct perf_output_handle *handle = sink_data->handle;
> >         struct etr_perf_buffer *etr_perf = etm_perf_sink_config(handle);
> >
> >         spin_lock_irqsave(&drvdata->spinlock, flags);
> > diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
> > index bfca103f9f84..20f0ab73159c 100644
> > --- a/drivers/hwtracing/coresight/coresight-tpda.c
> > +++ b/drivers/hwtracing/coresight/coresight-tpda.c
> > @@ -232,9 +232,22 @@ static void tpda_disable(struct coresight_device *csdev,
> >         dev_dbg(drvdata->dev, "TPDA inport %d disabled\n", in->dest_port);
> >  }
> >
> > +static int tpda_trace_id(struct coresight_device *csdev)
> > +{
> > +       struct tpda_drvdata *drvdata;
> > +
> > +       if (csdev == NULL)
> > +               return -EINVAL;
> > +
> > +       drvdata = dev_get_drvdata(csdev->dev.parent);
> > +
> > +       return drvdata->atid;
> > +}
> > +
> >  static const struct coresight_ops_link tpda_link_ops = {
> >         .enable         = tpda_enable,
> >         .disable        = tpda_disable,
> > +       .trace_id       = tpda_trace_id,
> >  };
> >
> >  static const struct coresight_ops tpda_cs_ops = {
> > diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> > index 96a32b213669..7f4560b067a8 100644
> > --- a/drivers/hwtracing/coresight/coresight-trbe.c
> > +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> > @@ -21,6 +21,7 @@
> >
> >  #include "coresight-self-hosted-trace.h"
> >  #include "coresight-trbe.h"
> > +#include "coresight-priv.h"
> >
> >  #define PERF_IDX2OFF(idx, buf) ((idx) % ((buf)->nr_pages << PAGE_SHIFT))
> >
> > @@ -1012,7 +1013,8 @@ static int arm_trbe_enable(struct coresight_device *csdev, enum cs_mode mode,
> >  {
> >         struct trbe_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> >         struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
> > -       struct perf_output_handle *handle = data;
> > +       struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
> > +       struct perf_output_handle *handle = sink_data->handle;
> >         struct trbe_buf *buf = etm_perf_sink_config(handle);
> >
> >         WARN_ON(cpudata->cpu != smp_processor_id());
> > diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> > index f9ebf20c91e6..92d8a9fb844e 100644
> > --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
> > +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
> > @@ -217,7 +217,8 @@ static void smb_enable_sysfs(struct coresight_device *csdev)
> >  static int smb_enable_perf(struct coresight_device *csdev, void *data)
> >  {
> >         struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
> > -       struct perf_output_handle *handle = data;
> > +       struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
> > +       struct perf_output_handle *handle = sink_data->handle;
> >         struct cs_buffers *buf = etm_perf_sink_config(handle);
> >         pid_t pid;
> >
> > diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> > index f09ace92176e..fb1c225076a5 100644
> > --- a/include/linux/coresight.h
> > +++ b/include/linux/coresight.h
> > @@ -344,6 +344,7 @@ struct coresight_ops_sink {
> >   * Operations available for links.
> >   * @enable:    enables flow between iport and oport.
> >   * @disable:   disables flow between iport and oport.
> > + * @trace_id:  Collect the traceid.
> >   */
> >  struct coresight_ops_link {
> >         int (*enable)(struct coresight_device *csdev,
> > @@ -352,6 +353,7 @@ struct coresight_ops_link {
> >         void (*disable)(struct coresight_device *csdev,
> >                         struct coresight_connection *in,
> >                         struct coresight_connection *out);
> > +       int (*trace_id)(struct coresight_device *csdev);
> >  };
> >
> >  /**
> > @@ -361,6 +363,7 @@ struct coresight_ops_link {
> >   *             is associated to.
> >   * @enable:    enables tracing for a source.
> >   * @disable:   disables tracing for a source.
> > + * @trace_id:  collect the traceid.
> >   */
> >  struct coresight_ops_source {
> >         int (*cpu_id)(struct coresight_device *csdev);
> > @@ -368,6 +371,7 @@ struct coresight_ops_source {
> >                       enum cs_mode mode);
> >         void (*disable)(struct coresight_device *csdev,
> >                         struct perf_event *event);
> > +       int (*trace_id)(struct coresight_device *csdev);
> >  };
> >
> >  /**
> > --
> > 2.34.1
> >
> 
> 
> -- 
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK
> 

Re: [PATCH v2 1/4] Coresight: Add trace_id function to collect trace ID

[JieGan]

On Tue, Jul 23, 2024 at 11:30:02AM +0100, Mike Leach wrote:

Hi, Mike
Sorry for the late reply, I checked James' patchset and I have updated
patches based on James' patchset.

> Hi,
> 
> This patch has a number of issues:-
> 
> 1) The new dynamic trace ID patchset to use per sink ID maps makes
> this set unusable. perf supplies a trace ID map for each sink used.
>  - see https://lists.linaro.org/archives/list/coresight@lists.linaro.org/thread/JEK7M7HRS57XK4B7CVTVFSHFAFBX4SFG/
> 
For perf mode, I added a option in trace_id function, to read trace ID via
perf_sink_id_map.

> 2) See etm4_enable_perf() - in the perf context a locked version of
> the read trace ID cannot be used - therefore any path that calls
> etm4_read_alloc_trace_id() (or equivalent for other sources) in perf
> mode may result in lockdep issues.
Dropped etm4_read_alloc_trace_id() and etm_read_alloc_trace_id() for perf mode session.
Add a option for perf mode, here is the sample code:

static int etm4_trace_id(struct coresight_device *csdev,
                         enum cs_mode mode,
                         struct coresight_trace_id_map *id_map)
{
        int trace_id;
        struct etmv4_drvdata *drvdata;

        if (csdev == NULL)
                return -EINVAL;

        drvdata = dev_get_drvdata(csdev->dev.parent);
        switch (mode) {
        case CS_MODE_SYSFS:
                trace_id = etm4_read_alloc_trace_id(drvdata);
                break;
        case CS_MODE_PERF:
                trace_id = coresight_trace_id_read_cpu_id_map(drvdata->cpu, id_map);
                if (IS_VALID_CS_TRACE_ID(trace_id))
                        drvdata->trcid = (u8)trace_id;
                break;
        default:
                trace_id = -EINVAL;
                break;
        }

        return trace_id;
}


> 
> 3) on enable: given a cpu  number, the trace ID can be read from the
> id maps rather than needing a new function in ops
For perf mode, the trace ID can be directly read from the perf_sink_id_map, but I
have still encapsulated these codes within trace_id function, is that acceptable?

For sysfs mode, I think we still need the trace_id function to retrieve the
trace ID of the source or link device(TPDA for TPDM) for enable session.

> 
> 4) on disable: trace id can be read directly from the source driver
> data - again removing need for a new trace_id function in ops.
In perf mode, the source device can be either etm3x or etm4x, we cannot directly read
from the source driver data unless we know  the exact source type(etm4x
or etm3x). This is because we need retrieve the driver data based on coreisght_device
according to the source type in etm_event_stop(). Therefore, I still to use the
trace_id function to handle this scenario, is that acceptable? Or I can directly call
coresight_trace_id_read_cpu_id_map() to read the trace ID.

For sysfs mode, I think we still need the trace_id function to retrieving the
trace ID of the source or link device(TPDA for TPDM) for disable session.

> 
> Regards
> 
> Mike

Thanks,
Jie

> 
> 
> On Fri, 5 Jul 2024 at 10:01, Jie Gan <quic_jiegan@quicinc.com> wrote:
> >
> > Add 'trace_id' function pointer in ops. It's responsible for collect the
> > trace ID of the device.
> >
> > Add 'struct cs_sink_data' to store the data used by coresight_enable_path/
> > coresight_disable_path. The structure will be transmitted to the helper and
> > sink device.
> >
> > Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
> > ---
> >  drivers/hwtracing/coresight/coresight-core.c  | 53 +++++++++++++++----
> >  drivers/hwtracing/coresight/coresight-etb10.c |  3 +-
> >  .../hwtracing/coresight/coresight-etm-perf.c  | 34 ++++++++++--
> >  .../coresight/coresight-etm3x-core.c          | 14 +++++
> >  .../coresight/coresight-etm4x-core.c          | 13 +++++
> >  drivers/hwtracing/coresight/coresight-priv.h  | 12 ++++-
> >  drivers/hwtracing/coresight/coresight-stm.c   | 13 +++++
> >  drivers/hwtracing/coresight/coresight-sysfs.c | 24 +++++++--
> >  .../hwtracing/coresight/coresight-tmc-etf.c   |  3 +-
> >  .../hwtracing/coresight/coresight-tmc-etr.c   |  6 ++-
> >  drivers/hwtracing/coresight/coresight-tpda.c  | 13 +++++
> >  drivers/hwtracing/coresight/coresight-trbe.c  |  4 +-
> >  drivers/hwtracing/coresight/ultrasoc-smb.c    |  3 +-
> >  include/linux/coresight.h                     |  4 ++
> >  14 files changed, 174 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> > index 9fc6f6b863e0..f414e66f4cda 100644
> > --- a/drivers/hwtracing/coresight/coresight-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-core.c
> > @@ -297,12 +297,12 @@ static int coresight_enable_helper(struct coresight_device *csdev,
> >         return helper_ops(csdev)->enable(csdev, mode, data);
> >  }
> >
> > -static void coresight_disable_helper(struct coresight_device *csdev)
> > +static void coresight_disable_helper(struct coresight_device *csdev, void *data)
> >  {
> > -       helper_ops(csdev)->disable(csdev, NULL);
> > +       helper_ops(csdev)->disable(csdev, data);
> >  }
> >
> > -static void coresight_disable_helpers(struct coresight_device *csdev)
> > +static void coresight_disable_helpers(struct coresight_device *csdev, void *data)
> >  {
> >         int i;
> >         struct coresight_device *helper;
> > @@ -310,7 +310,7 @@ static void coresight_disable_helpers(struct coresight_device *csdev)
> >         for (i = 0; i < csdev->pdata->nr_outconns; ++i) {
> >                 helper = csdev->pdata->out_conns[i]->dest_dev;
> >                 if (helper && coresight_is_helper(helper))
> > -                       coresight_disable_helper(helper);
> > +                       coresight_disable_helper(helper, data);
> >         }
> >  }
> >
> > @@ -327,7 +327,7 @@ static void coresight_disable_helpers(struct coresight_device *csdev)
> >  void coresight_disable_source(struct coresight_device *csdev, void *data)
> >  {
> >         source_ops(csdev)->disable(csdev, data);
> > -       coresight_disable_helpers(csdev);
> > +       coresight_disable_helpers(csdev, NULL);
> >  }
> >  EXPORT_SYMBOL_GPL(coresight_disable_source);
> >
> > @@ -337,7 +337,8 @@ EXPORT_SYMBOL_GPL(coresight_disable_source);
> >   * disabled.
> >   */
> >  static void coresight_disable_path_from(struct list_head *path,
> > -                                       struct coresight_node *nd)
> > +                                       struct coresight_node *nd,
> > +                                       void *sink_data)
> >  {
> >         u32 type;
> >         struct coresight_device *csdev, *parent, *child;
> > @@ -382,13 +383,13 @@ static void coresight_disable_path_from(struct list_head *path,
> >                 }
> >
> >                 /* Disable all helpers adjacent along the path last */
> > -               coresight_disable_helpers(csdev);
> > +               coresight_disable_helpers(csdev, sink_data);
> >         }
> >  }
> >
> > -void coresight_disable_path(struct list_head *path)
> > +void coresight_disable_path(struct list_head *path, void *sink_data)
> >  {
> > -       coresight_disable_path_from(path, NULL);
> > +       coresight_disable_path_from(path, NULL, sink_data);
> >  }
> >  EXPORT_SYMBOL_GPL(coresight_disable_path);
> >
> > @@ -468,10 +469,42 @@ int coresight_enable_path(struct list_head *path, enum cs_mode mode,
> >  out:
> >         return ret;
> >  err:
> > -       coresight_disable_path_from(path, nd);
> > +       coresight_disable_path_from(path, nd, sink_data);
> >         goto out;
> >  }
> >
> > +int coresight_read_traceid(struct list_head *path)
> > +{
> > +       int trace_id, type;
> > +       struct coresight_device *csdev;
> > +       struct coresight_node *nd;
> > +
> > +       list_for_each_entry(nd, path, link) {
> > +               csdev = nd->csdev;
> > +               type = csdev->type;
> > +
> > +               switch(type) {
> > +                       case CORESIGHT_DEV_TYPE_SOURCE:
> > +                               if (source_ops(csdev)->trace_id != NULL) {
> > +                                       trace_id = source_ops(csdev)->trace_id(csdev);
> > +                                       if (trace_id > 0)
> > +                                               return trace_id;
> > +                               }
> > +                               break;
> > +                       case CORESIGHT_DEV_TYPE_LINK:
> > +                               if (link_ops(csdev)->trace_id != NULL) {
> > +                                       trace_id = link_ops(csdev)->trace_id(csdev);
> > +                                       if (trace_id > 0)
> > +                                               return trace_id;
> > +                               }
> > +                               break;
> > +                       default:
> > +                               break;
> > +               }
> > +       }
> > +       return -EINVAL;
> > +}
> > +
> >  struct coresight_device *coresight_get_sink(struct list_head *path)
> >  {
> >         struct coresight_device *csdev;
> > diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
> > index 7edd3f1d0d46..05e620529c14 100644
> > --- a/drivers/hwtracing/coresight/coresight-etb10.c
> > +++ b/drivers/hwtracing/coresight/coresight-etb10.c
> > @@ -173,7 +173,8 @@ static int etb_enable_perf(struct coresight_device *csdev, void *data)
> >         pid_t pid;
> >         unsigned long flags;
> >         struct etb_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> > -       struct perf_output_handle *handle = data;
> > +       struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
> > +       struct perf_output_handle *handle = sink_data->handle;
> >         struct cs_buffers *buf = etm_perf_sink_config(handle);
> >
> >         spin_lock_irqsave(&drvdata->spinlock, flags);
> > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > index c0c60e6a1703..8b155765b959 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > @@ -452,6 +452,7 @@ static void etm_event_start(struct perf_event *event, int flags)
> >         struct perf_output_handle *handle = &ctxt->handle;
> >         struct coresight_device *sink, *csdev = per_cpu(csdev_src, cpu);
> >         struct list_head *path;
> > +       struct cs_sink_data *sink_data = NULL;
> >         u64 hw_id;
> >
> >         if (!csdev)
> > @@ -490,9 +491,18 @@ static void etm_event_start(struct perf_event *event, int flags)
> >         if (WARN_ON_ONCE(!sink))
> >                 goto fail_end_stop;
> >
> > +       sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL);
> > +       if (!sink_data)
> > +               goto fail_end_stop;
> > +
> > +       sink_data->sink = sink;
> > +       sink_data->traceid = coresight_read_traceid(path);
> > +       sink_data->handle = handle;
> >         /* Nothing will happen without a path */
> > -       if (coresight_enable_path(path, CS_MODE_PERF, handle))
> > +       if (coresight_enable_path(path, CS_MODE_PERF, sink_data)) {
> > +               kfree(sink_data);
> >                 goto fail_end_stop;
> > +       }
> >
> >         /* Finally enable the tracer */
> >         if (source_ops(csdev)->enable(csdev, event, CS_MODE_PERF))
> > @@ -511,6 +521,7 @@ static void etm_event_start(struct perf_event *event, int flags)
> >                 perf_report_aux_output_id(event, hw_id);
> >         }
> >
> > +       kfree(sink_data);
> >  out:
> >         /* Tell the perf core the event is alive */
> >         event->hw.state = 0;
> > @@ -519,7 +530,8 @@ static void etm_event_start(struct perf_event *event, int flags)
> >         return;
> >
> >  fail_disable_path:
> > -       coresight_disable_path(path);
> > +       coresight_disable_path(path, sink_data);
> > +       kfree(sink_data);
> >  fail_end_stop:
> >         /*
> >          * Check if the handle is still associated with the event,
> > @@ -544,6 +556,7 @@ static void etm_event_stop(struct perf_event *event, int mode)
> >         struct perf_output_handle *handle = &ctxt->handle;
> >         struct etm_event_data *event_data;
> >         struct list_head *path;
> > +       struct cs_sink_data *sink_data = NULL;
> >
> >         /*
> >          * If we still have access to the event_data via handle,
> > @@ -588,6 +601,10 @@ static void etm_event_stop(struct perf_event *event, int mode)
> >         if (!sink)
> >                 return;
> >
> > +       sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL);
> > +       if (!sink_data)
> > +               return;
> > +
> >         /* stop tracer */
> >         coresight_disable_source(csdev, event);
> >
> > @@ -601,12 +618,16 @@ static void etm_event_stop(struct perf_event *event, int mode)
> >          * have to do anything here.
> >          */
> >         if (handle->event && (mode & PERF_EF_UPDATE)) {
> > -               if (WARN_ON_ONCE(handle->event != event))
> > +               if (WARN_ON_ONCE(handle->event != event)) {
> > +                       kfree(sink_data);
> >                         return;
> > +               }
> >
> >                 /* update trace information */
> > -               if (!sink_ops(sink)->update_buffer)
> > +               if (!sink_ops(sink)->update_buffer) {
> > +                       kfree(sink_data);
> >                         return;
> > +               }
> >
> >                 size = sink_ops(sink)->update_buffer(sink, handle,
> >                                               event_data->snk_config);
> > @@ -627,8 +648,11 @@ static void etm_event_stop(struct perf_event *event, int mode)
> >                         WARN_ON(size);
> >         }
> >
> > +       sink_data->sink = sink;
> > +       sink_data->traceid = coresight_read_traceid(path);
> >         /* Disabling the path make its elements available to other sessions */
> > -       coresight_disable_path(path);
> > +       coresight_disable_path(path, sink_data);
> > +       kfree(sink_data);
> >  }
> >
> >  static int etm_event_add(struct perf_event *event, int mode)
> > diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> > index 8b362605d242..27e973749050 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> > @@ -696,10 +696,24 @@ static void etm_disable(struct coresight_device *csdev,
> >                 coresight_set_mode(csdev, CS_MODE_DISABLED);
> >  }
> >
> > +static int etm_trace_id(struct coresight_device *csdev)
> > +{
> > +       struct etm_drvdata *drvdata;
> > +
> > +       if (csdev == NULL)
> > +               return -EINVAL;
> > +
> > +       drvdata = dev_get_drvdata(csdev->dev.parent);
> > +
> > +       return etm_read_alloc_trace_id(drvdata);
> > +}
> > +
> > +
> >  static const struct coresight_ops_source etm_source_ops = {
> >         .cpu_id         = etm_cpu_id,
> >         .enable         = etm_enable,
> >         .disable        = etm_disable,
> > +       .trace_id       = etm_trace_id,
> >  };
> >
> >  static const struct coresight_ops etm_cs_ops = {
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > index bf01f01964cf..8c3e9bfb9a9c 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > @@ -1024,10 +1024,23 @@ static void etm4_disable(struct coresight_device *csdev,
> >                 coresight_set_mode(csdev, CS_MODE_DISABLED);
> >  }
> >
> > +static int etm4_trace_id(struct coresight_device *csdev)
> > +{
> > +       struct etmv4_drvdata *drvdata;
> > +
> > +       if (csdev == NULL)
> > +               return -EINVAL;
> > +
> > +       drvdata = dev_get_drvdata(csdev->dev.parent);
> > +
> > +       return etm4_read_alloc_trace_id(drvdata);
> > +}
> > +
> >  static const struct coresight_ops_source etm4_source_ops = {
> >         .cpu_id         = etm4_cpu_id,
> >         .enable         = etm4_enable,
> >         .disable        = etm4_disable,
> > +       .trace_id       = etm4_trace_id,
> >  };
> >
> >  static const struct coresight_ops etm4_cs_ops = {
> > diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> > index 61a46d3bdcc8..e2576531f796 100644
> > --- a/drivers/hwtracing/coresight/coresight-priv.h
> > +++ b/drivers/hwtracing/coresight/coresight-priv.h
> > @@ -105,6 +105,15 @@ struct cs_buffers {
> >         void                    **data_pages;
> >  };
> >
> > +/**
> > + * struct cs_sink_data - data used by coresight_enable_path/coresight_disable_path
> > + */
> > +struct cs_sink_data {
> > +       struct perf_output_handle       *handle;
> > +       struct coresight_device         *sink;
> > +       u32                             traceid;
> > +};
> > +
> >  static inline void coresight_insert_barrier_packet(void *buf)
> >  {
> >         if (buf)
> > @@ -129,9 +138,10 @@ static inline void CS_UNLOCK(void __iomem *addr)
> >         } while (0);
> >  }
> >
> > -void coresight_disable_path(struct list_head *path);
> > +void coresight_disable_path(struct list_head *path, void *sink_data);
> >  int coresight_enable_path(struct list_head *path, enum cs_mode mode,
> >                           void *sink_data);
> > +int coresight_read_traceid(struct list_head *path);
> >  struct coresight_device *coresight_get_sink(struct list_head *path);
> >  struct coresight_device *coresight_get_sink_by_id(u32 id);
> >  struct coresight_device *
> > diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
> > index 117dbb484543..3817743fc0c6 100644
> > --- a/drivers/hwtracing/coresight/coresight-stm.c
> > +++ b/drivers/hwtracing/coresight/coresight-stm.c
> > @@ -280,9 +280,22 @@ static void stm_disable(struct coresight_device *csdev,
> >         }
> >  }
> >
> > +static int stm_trace_id(struct coresight_device *csdev)
> > +{
> > +       struct stm_drvdata *drvdata;
> > +
> > +       if (csdev == NULL)
> > +               return -EINVAL;
> > +
> > +       drvdata = dev_get_drvdata(csdev->dev.parent);
> > +
> > +       return drvdata->traceid;
> > +}
> > +
> >  static const struct coresight_ops_source stm_source_ops = {
> >         .enable         = stm_enable,
> >         .disable        = stm_disable,
> > +       .trace_id       = stm_trace_id,
> >  };
> >
> >  static const struct coresight_ops stm_cs_ops = {
> > diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c
> > index 1e67cc7758d7..a95afc890587 100644
> > --- a/drivers/hwtracing/coresight/coresight-sysfs.c
> > +++ b/drivers/hwtracing/coresight/coresight-sysfs.c
> > @@ -167,6 +167,7 @@ int coresight_enable_sysfs(struct coresight_device *csdev)
> >         int cpu, ret = 0;
> >         struct coresight_device *sink;
> >         struct list_head *path;
> > +       struct cs_sink_data *sink_data;
> >         enum coresight_dev_subtype_source subtype;
> >         u32 hash;
> >
> > @@ -208,7 +209,14 @@ int coresight_enable_sysfs(struct coresight_device *csdev)
> >                 goto out;
> >         }
> >
> > -       ret = coresight_enable_path(path, CS_MODE_SYSFS, NULL);
> > +       sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL);
> > +       if (!sink_data) {
> > +               ret = -ENOMEM;
> > +               goto out;
> > +       }
> > +       sink_data->traceid = coresight_read_traceid(path);
> > +       sink_data->sink = sink;
> > +       ret = coresight_enable_path(path, CS_MODE_SYSFS, sink_data);
> >         if (ret)
> >                 goto err_path;
> >
> > @@ -245,15 +253,17 @@ int coresight_enable_sysfs(struct coresight_device *csdev)
> >                 break;
> >         }
> >
> > +       kfree(sink_data);
> >  out:
> >         mutex_unlock(&coresight_mutex);
> >         return ret;
> >
> >  err_source:
> > -       coresight_disable_path(path);
> > +       coresight_disable_path(path, sink_data);
> >
> >  err_path:
> >         coresight_release_path(path);
> > +       kfree(sink_data);
> >         goto out;
> >  }
> >  EXPORT_SYMBOL_GPL(coresight_enable_sysfs);
> > @@ -262,6 +272,7 @@ void coresight_disable_sysfs(struct coresight_device *csdev)
> >  {
> >         int cpu, ret;
> >         struct list_head *path = NULL;
> > +       struct cs_sink_data *sink_data = NULL;
> >         u32 hash;
> >
> >         mutex_lock(&coresight_mutex);
> > @@ -273,6 +284,10 @@ void coresight_disable_sysfs(struct coresight_device *csdev)
> >         if (!coresight_disable_source_sysfs(csdev, NULL))
> >                 goto out;
> >
> > +       sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL);
> > +       if (!sink_data)
> > +               goto out;
> > +
> >         switch (csdev->subtype.source_subtype) {
> >         case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC:
> >                 cpu = source_ops(csdev)->cpu_id(csdev);
> > @@ -296,8 +311,11 @@ void coresight_disable_sysfs(struct coresight_device *csdev)
> >                 break;
> >         }
> >
> > -       coresight_disable_path(path);
> > +       sink_data->sink = coresight_find_activated_sysfs_sink(csdev);
> > +       sink_data->traceid = coresight_read_traceid(path);
> > +       coresight_disable_path(path, sink_data);
> >         coresight_release_path(path);
> > +       kfree(sink_data);
> >
> >  out:
> >         mutex_unlock(&coresight_mutex);
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > index d4f641cd9de6..7dc536eba3e2 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > @@ -250,7 +250,8 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, void *data)
> >         pid_t pid;
> >         unsigned long flags;
> >         struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> > -       struct perf_output_handle *handle = data;
> > +       struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
> > +       struct perf_output_handle *handle = sink_data->handle;
> >         struct cs_buffers *buf = etm_perf_sink_config(handle);
> >
> >         spin_lock_irqsave(&drvdata->spinlock, flags);
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > index e75428fa1592..0c24520645e2 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > @@ -1253,7 +1253,8 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
> >  struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,
> >                                    enum cs_mode mode, void *data)
> >  {
> > -       struct perf_output_handle *handle = data;
> > +       struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
> > +       struct perf_output_handle *handle = sink_data->handle;
> >         struct etr_perf_buffer *etr_perf;
> >
> >         switch (mode) {
> > @@ -1647,7 +1648,8 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
> >         pid_t pid;
> >         unsigned long flags;
> >         struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> > -       struct perf_output_handle *handle = data;
> > +       struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
> > +       struct perf_output_handle *handle = sink_data->handle;
> >         struct etr_perf_buffer *etr_perf = etm_perf_sink_config(handle);
> >
> >         spin_lock_irqsave(&drvdata->spinlock, flags);
> > diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
> > index bfca103f9f84..20f0ab73159c 100644
> > --- a/drivers/hwtracing/coresight/coresight-tpda.c
> > +++ b/drivers/hwtracing/coresight/coresight-tpda.c
> > @@ -232,9 +232,22 @@ static void tpda_disable(struct coresight_device *csdev,
> >         dev_dbg(drvdata->dev, "TPDA inport %d disabled\n", in->dest_port);
> >  }
> >
> > +static int tpda_trace_id(struct coresight_device *csdev)
> > +{
> > +       struct tpda_drvdata *drvdata;
> > +
> > +       if (csdev == NULL)
> > +               return -EINVAL;
> > +
> > +       drvdata = dev_get_drvdata(csdev->dev.parent);
> > +
> > +       return drvdata->atid;
> > +}
> > +
> >  static const struct coresight_ops_link tpda_link_ops = {
> >         .enable         = tpda_enable,
> >         .disable        = tpda_disable,
> > +       .trace_id       = tpda_trace_id,
> >  };
> >
> >  static const struct coresight_ops tpda_cs_ops = {
> > diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> > index 96a32b213669..7f4560b067a8 100644
> > --- a/drivers/hwtracing/coresight/coresight-trbe.c
> > +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> > @@ -21,6 +21,7 @@
> >
> >  #include "coresight-self-hosted-trace.h"
> >  #include "coresight-trbe.h"
> > +#include "coresight-priv.h"
> >
> >  #define PERF_IDX2OFF(idx, buf) ((idx) % ((buf)->nr_pages << PAGE_SHIFT))
> >
> > @@ -1012,7 +1013,8 @@ static int arm_trbe_enable(struct coresight_device *csdev, enum cs_mode mode,
> >  {
> >         struct trbe_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> >         struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
> > -       struct perf_output_handle *handle = data;
> > +       struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
> > +       struct perf_output_handle *handle = sink_data->handle;
> >         struct trbe_buf *buf = etm_perf_sink_config(handle);
> >
> >         WARN_ON(cpudata->cpu != smp_processor_id());
> > diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> > index f9ebf20c91e6..92d8a9fb844e 100644
> > --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
> > +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
> > @@ -217,7 +217,8 @@ static void smb_enable_sysfs(struct coresight_device *csdev)
> >  static int smb_enable_perf(struct coresight_device *csdev, void *data)
> >  {
> >         struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
> > -       struct perf_output_handle *handle = data;
> > +       struct cs_sink_data *sink_data = (struct cs_sink_data *)data;
> > +       struct perf_output_handle *handle = sink_data->handle;
> >         struct cs_buffers *buf = etm_perf_sink_config(handle);
> >         pid_t pid;
> >
> > diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> > index f09ace92176e..fb1c225076a5 100644
> > --- a/include/linux/coresight.h
> > +++ b/include/linux/coresight.h
> > @@ -344,6 +344,7 @@ struct coresight_ops_sink {
> >   * Operations available for links.
> >   * @enable:    enables flow between iport and oport.
> >   * @disable:   disables flow between iport and oport.
> > + * @trace_id:  Collect the traceid.
> >   */
> >  struct coresight_ops_link {
> >         int (*enable)(struct coresight_device *csdev,
> > @@ -352,6 +353,7 @@ struct coresight_ops_link {
> >         void (*disable)(struct coresight_device *csdev,
> >                         struct coresight_connection *in,
> >                         struct coresight_connection *out);
> > +       int (*trace_id)(struct coresight_device *csdev);
> >  };
> >
> >  /**
> > @@ -361,6 +363,7 @@ struct coresight_ops_link {
> >   *             is associated to.
> >   * @enable:    enables tracing for a source.
> >   * @disable:   disables tracing for a source.
> > + * @trace_id:  collect the traceid.
> >   */
> >  struct coresight_ops_source {
> >         int (*cpu_id)(struct coresight_device *csdev);
> > @@ -368,6 +371,7 @@ struct coresight_ops_source {
> >                       enum cs_mode mode);
> >         void (*disable)(struct coresight_device *csdev,
> >                         struct perf_event *event);
> > +       int (*trace_id)(struct coresight_device *csdev);
> >  };
> >
> >  /**
> > --
> > 2.34.1
> >
> 
> 
> -- 
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK
>