[PATCH v5 0/3] coresight: Add static trace id support

[PATCH v5 0/3] coresight: Add static trace id support

[Mao Jinlong]

Some HW has static trace id which cannot be changed via
software programming. For this case, configure the trace id
in device tree with "arm,static-trace-id = <xxx>", and
call coresight_trace_id_get_static_system_id with the trace id value
in device probe function. The id will be reserved for the HW
all the time if the device is probed.

Changes since V4:
1. Use fwnode_property_read_u32 in fwnode_property_read_u32.
2. Update date and version in sysfs-bus-coresight-devices-dummy-source

Changes since V3:
1. Adda new API function
int coresight_trace_id_get_system_static_id(int trace_id).
2. Use the term "static trace id" for these devices where
the hardware sets a non-programmable trace ID. 

Changes since V2:
1. Change "trace-id" to "arm,trace-id".
2. Add trace id flag for getting preferred id or ODD id.

Changes since V1:
1. Add argument to coresight_trace_id_get_system_id for preferred id
instead of adding new function coresight_trace_id_reserve_system_id.
2. Add constraint to trace-id in dt-binding file.

Mao Jinlong (3):
  dt-bindings: arm: Add arm,static-trace-id for coresight dummy source
  coresight: Add support to get static id for system trace sources
  coresight: dummy: Add static trace id support for dummy source

 .../sysfs-bus-coresight-devices-dummy-source  | 15 +++++
 .../arm/arm,coresight-dummy-source.yaml       |  6 ++
 drivers/hwtracing/coresight/coresight-dummy.c | 59 +++++++++++++++++--
 .../hwtracing/coresight/coresight-platform.c  |  9 +++
 .../hwtracing/coresight/coresight-trace-id.c  | 38 ++++++++----
 .../hwtracing/coresight/coresight-trace-id.h  |  9 +++
 include/linux/coresight.h                     |  1 +
 7 files changed, 123 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-dummy-source

-- 
2.17.1

[PATCH v5 1/3] dt-bindings: arm: Add arm,static-trace-id for coresight dummy source

[Mao Jinlong]

Some dummy source HW has static trace id which cannot be changed via
software programming. Add arm,static-trace-id for static id support to
coresight dummy source.

Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
 .../devicetree/bindings/arm/arm,coresight-dummy-source.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/arm,coresight-dummy-source.yaml b/Documentation/devicetree/bindings/arm/arm,coresight-dummy-source.yaml
index 04a8c37b4aff..742dc4e25d3b 100644
--- a/Documentation/devicetree/bindings/arm/arm,coresight-dummy-source.yaml
+++ b/Documentation/devicetree/bindings/arm/arm,coresight-dummy-source.yaml
@@ -38,6 +38,12 @@ properties:
     enum:
       - arm,coresight-dummy-source
 
+  arm,static-trace-id:
+    description: If dummy source needs static id support, use this to set trace id.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 111
+
   out-ports:
     $ref: /schemas/graph.yaml#/properties/ports
 
-- 
2.17.1

[PATCH v5 2/3] coresight: Add support to get static id for system trace sources

[Mao Jinlong]

Dynamic trace id was introduced in coresight subsystem, so trace id is
allocated dynamically. However, some hardware ATB source has static trace
id and it cannot be changed via software programming. For such source,
it can call coresight_get_static_trace_id to get the fixed trace id from
device node and pass id to coresight_trace_id_get_static_system_id to
reserve the id.

Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
---
 .../hwtracing/coresight/coresight-platform.c  |  9 +++++
 .../hwtracing/coresight/coresight-trace-id.c  | 38 ++++++++++++++-----
 .../hwtracing/coresight/coresight-trace-id.h  |  9 +++++
 include/linux/coresight.h                     |  1 +
 4 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
index 64e171eaad82..b03aa43d3cba 100644
--- a/drivers/hwtracing/coresight/coresight-platform.c
+++ b/drivers/hwtracing/coresight/coresight-platform.c
@@ -796,6 +796,15 @@ int coresight_get_cpu(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(coresight_get_cpu);
 
+int coresight_get_static_trace_id(struct device *dev, u32 *id)
+{
+	if (!is_of_node(dev->fwnode))
+		return -EINVAL;
+
+	return fwnode_property_read_u32(dev_fwnode(dev), "arm,static-trace-id", id);
+}
+EXPORT_SYMBOL_GPL(coresight_get_static_trace_id);
+
 struct coresight_platform_data *
 coresight_get_platform_data(struct device *dev)
 {
diff --git a/drivers/hwtracing/coresight/coresight-trace-id.c b/drivers/hwtracing/coresight/coresight-trace-id.c
index d98e12cb30ec..269a5f7b279f 100644
--- a/drivers/hwtracing/coresight/coresight-trace-id.c
+++ b/drivers/hwtracing/coresight/coresight-trace-id.c
@@ -12,6 +12,12 @@
 
 #include "coresight-trace-id.h"
 
+enum trace_id_flags {
+	TRACE_ID_ANY = 0x0,
+	TRACE_ID_PREFER_ODD = 0x1,
+	TRACE_ID_REQ_STATIC = 0x2,
+};
+
 /* Default trace ID map. Used in sysfs mode and for system sources */
 static DEFINE_PER_CPU(atomic_t, id_map_default_cpu_ids) = ATOMIC_INIT(0);
 static struct coresight_trace_id_map id_map_default = {
@@ -74,16 +80,19 @@ static int coresight_trace_id_find_odd_id(struct coresight_trace_id_map *id_map)
  * Otherwise allocate next available ID.
  */
 static int coresight_trace_id_alloc_new_id(struct coresight_trace_id_map *id_map,
-					   int preferred_id, bool prefer_odd_id)
+					   int preferred_id, unsigned int flags)
 {
 	int id = 0;
 
 	/* for backwards compatibility, cpu IDs may use preferred value */
-	if (IS_VALID_CS_TRACE_ID(preferred_id) &&
-	    !test_bit(preferred_id, id_map->used_ids)) {
-		id = preferred_id;
-		goto trace_id_allocated;
-	} else if (prefer_odd_id) {
+	if (IS_VALID_CS_TRACE_ID(preferred_id)) {
+		if (!test_bit(preferred_id, id_map->used_ids)) {
+			id = preferred_id;
+			goto trace_id_allocated;
+		} else if (WARN((flags & TRACE_ID_REQ_STATIC), "Trace ID %d is used.\n",
+					preferred_id))
+			return -EINVAL;
+	} else if (flags & TRACE_ID_PREFER_ODD) {
 	/* may use odd ids to avoid preferred legacy cpu IDs */
 		id = coresight_trace_id_find_odd_id(id_map);
 		if (id)
@@ -153,7 +162,7 @@ static int _coresight_trace_id_get_cpu_id(int cpu, struct coresight_trace_id_map
 	 */
 	id = coresight_trace_id_alloc_new_id(id_map,
 					     CORESIGHT_LEGACY_CPU_TRACE_ID(cpu),
-					     false);
+					     TRACE_ID_ANY);
 	if (!IS_VALID_CS_TRACE_ID(id))
 		goto get_cpu_id_out_unlock;
 
@@ -188,14 +197,15 @@ static void _coresight_trace_id_put_cpu_id(int cpu, struct coresight_trace_id_ma
 	DUMP_ID_MAP(id_map);
 }
 
-static int coresight_trace_id_map_get_system_id(struct coresight_trace_id_map *id_map)
+static int coresight_trace_id_map_get_system_id(struct coresight_trace_id_map *id_map,
+					int preferred_id, unsigned int traceid_flags)
 {
 	unsigned long flags;
 	int id;
 
 	spin_lock_irqsave(&id_map->lock, flags);
 	/* prefer odd IDs for system components to avoid legacy CPU IDS */
-	id = coresight_trace_id_alloc_new_id(id_map, 0, true);
+	id = coresight_trace_id_alloc_new_id(id_map, preferred_id, traceid_flags);
 	spin_unlock_irqrestore(&id_map->lock, flags);
 
 	DUMP_ID(id);
@@ -255,10 +265,18 @@ EXPORT_SYMBOL_GPL(coresight_trace_id_read_cpu_id_map);
 
 int coresight_trace_id_get_system_id(void)
 {
-	return coresight_trace_id_map_get_system_id(&id_map_default);
+	return coresight_trace_id_map_get_system_id(&id_map_default, 0,
+			TRACE_ID_PREFER_ODD);
 }
 EXPORT_SYMBOL_GPL(coresight_trace_id_get_system_id);
 
+int coresight_trace_id_get_static_system_id(int trace_id)
+{
+	return coresight_trace_id_map_get_system_id(&id_map_default,
+			trace_id, TRACE_ID_REQ_STATIC);
+}
+EXPORT_SYMBOL_GPL(coresight_trace_id_get_static_system_id);
+
 void coresight_trace_id_put_system_id(int id)
 {
 	coresight_trace_id_map_put_system_id(&id_map_default, id);
diff --git a/drivers/hwtracing/coresight/coresight-trace-id.h b/drivers/hwtracing/coresight/coresight-trace-id.h
index 9aae50a553ca..db68e1ec56b6 100644
--- a/drivers/hwtracing/coresight/coresight-trace-id.h
+++ b/drivers/hwtracing/coresight/coresight-trace-id.h
@@ -116,6 +116,15 @@ int coresight_trace_id_read_cpu_id_map(int cpu, struct coresight_trace_id_map *i
  */
 int coresight_trace_id_get_system_id(void);
 
+/**
+ * Allocate a CoreSight static trace ID for a system component.
+ *
+ * Used to allocate static IDs for system trace sources such as dummy source.
+ *
+ * return: Trace ID or -EINVAL if allocation is impossible.
+ */
+int coresight_trace_id_get_static_system_id(int id);
+
 /**
  * Release an allocated system trace ID.
  *
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index c13342594278..129795873072 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -662,6 +662,7 @@ void coresight_relaxed_write64(struct coresight_device *csdev,
 void coresight_write64(struct coresight_device *csdev, u64 val, u32 offset);
 
 extern int coresight_get_cpu(struct device *dev);
+extern int coresight_get_static_trace_id(struct device *dev, u32 *id);
 
 struct coresight_platform_data *coresight_get_platform_data(struct device *dev);
 struct coresight_connection *
-- 
2.17.1

[PATCH v5 3/3] coresight: dummy: Add static trace id support for dummy source

[Mao Jinlong]

Some dummy source has static trace id configured in HW and it cannot
be changed via software programming. Configure the trace id in device
tree and reserve the id when device probe.

Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
---
 .../sysfs-bus-coresight-devices-dummy-source  | 15 +++++
 drivers/hwtracing/coresight/coresight-dummy.c | 59 +++++++++++++++++--
 2 files changed, 70 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-dummy-source

diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-dummy-source b/Documentation/ABI/testing/sysfs-bus-coresight-devices-dummy-source
new file mode 100644
index 000000000000..c7d975e75d85
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-dummy-source
@@ -0,0 +1,15 @@
+What:		/sys/bus/coresight/devices/dummy_source<N>/enable_source
+Date:		Oct 2024
+KernelVersion:	6.13
+Contact:	Mao Jinlong <quic_jinlmao@quicinc.com>
+Description:	(RW) Enable/disable tracing of dummy source. A sink should be activated
+		before enabling the source. The path of coresight components linking
+		the source to the sink is configured and managed automatically by the
+		coresight framework.
+
+What:		/sys/bus/coresight/devices/dummy_source<N>/traceid
+Date:		Oct 2024
+KernelVersion:	6.13
+Contact:	Mao Jinlong <quic_jinlmao@quicinc.com>
+Description:	(R) Show the trace ID that will appear in the trace stream
+		coming from this trace entity.
diff --git a/drivers/hwtracing/coresight/coresight-dummy.c b/drivers/hwtracing/coresight/coresight-dummy.c
index bb85fa663ffc..602a7e89e311 100644
--- a/drivers/hwtracing/coresight/coresight-dummy.c
+++ b/drivers/hwtracing/coresight/coresight-dummy.c
@@ -11,10 +11,12 @@
 #include <linux/pm_runtime.h>
 
 #include "coresight-priv.h"
+#include "coresight-trace-id.h"
 
 struct dummy_drvdata {
 	struct device			*dev;
 	struct coresight_device		*csdev;
+	u8				traceid;
 };
 
 DEFINE_CORESIGHT_DEVLIST(source_devs, "dummy_source");
@@ -72,6 +74,32 @@ static const struct coresight_ops dummy_sink_cs_ops = {
 	.sink_ops = &dummy_sink_ops,
 };
 
+/* User can get the trace id of dummy source from this node. */
+static ssize_t traceid_show(struct device *dev,
+			    struct device_attribute *attr, char *buf)
+{
+	unsigned long val;
+	struct dummy_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	val = drvdata->traceid;
+	return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
+}
+static DEVICE_ATTR_RO(traceid);
+
+static struct attribute *coresight_dummy_attrs[] = {
+	&dev_attr_traceid.attr,
+	NULL,
+};
+
+static const struct attribute_group coresight_dummy_group = {
+	.attrs = coresight_dummy_attrs,
+};
+
+static const struct attribute_group *coresight_dummy_groups[] = {
+	&coresight_dummy_group,
+	NULL,
+};
+
 static int dummy_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -79,6 +107,11 @@ static int dummy_probe(struct platform_device *pdev)
 	struct coresight_platform_data *pdata;
 	struct dummy_drvdata *drvdata;
 	struct coresight_desc desc = { 0 };
+	int ret, trace_id;
+
+	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
 
 	if (of_device_is_compatible(node, "arm,coresight-dummy-source")) {
 
@@ -90,6 +123,25 @@ static int dummy_probe(struct platform_device *pdev)
 		desc.subtype.source_subtype =
 					CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS;
 		desc.ops = &dummy_source_cs_ops;
+		desc.groups = coresight_dummy_groups;
+
+		ret = coresight_get_static_trace_id(dev, &trace_id);
+		if (!ret) {
+			/* Get the static id if id is set in device tree. */
+			ret = coresight_trace_id_get_static_system_id(trace_id);
+			if (ret < 0)
+				return ret;
+
+		} else {
+			/* Get next available id if id is not set in device tree. */
+			trace_id = coresight_trace_id_get_system_id();
+			if (trace_id < 0) {
+				ret = trace_id;
+				return ret;
+			}
+		}
+		drvdata->traceid = (u8)trace_id;
+
 	} else if (of_device_is_compatible(node, "arm,coresight-dummy-sink")) {
 		desc.name = coresight_alloc_device_name(&sink_devs, dev);
 		if (!desc.name)
@@ -108,10 +160,6 @@ static int dummy_probe(struct platform_device *pdev)
 		return PTR_ERR(pdata);
 	pdev->dev.platform_data = pdata;
 
-	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
-	if (!drvdata)
-		return -ENOMEM;
-
 	drvdata->dev = &pdev->dev;
 	platform_set_drvdata(pdev, drvdata);
 
@@ -131,7 +179,10 @@ static void dummy_remove(struct platform_device *pdev)
 {
 	struct dummy_drvdata *drvdata = platform_get_drvdata(pdev);
 	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
 
+	if (IS_VALID_CS_TRACE_ID(drvdata->traceid))
+		coresight_trace_id_put_system_id(drvdata->traceid);
 	pm_runtime_disable(dev);
 	coresight_unregister(drvdata->csdev);
 }
-- 
2.17.1

Re: [PATCH v5 2/3] coresight: Add support to get static id for system trace sources

[Suzuki K Poulose]

On 18/10/2024 04:22, Mao Jinlong wrote:
> Dynamic trace id was introduced in coresight subsystem, so trace id is
> allocated dynamically. However, some hardware ATB source has static trace
> id and it cannot be changed via software programming. For such source,
> it can call coresight_get_static_trace_id to get the fixed trace id from
> device node and pass id to coresight_trace_id_get_static_system_id to
> reserve the id.
> 
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>   .../hwtracing/coresight/coresight-platform.c  |  9 +++++
>   .../hwtracing/coresight/coresight-trace-id.c  | 38 ++++++++++++++-----
>   .../hwtracing/coresight/coresight-trace-id.h  |  9 +++++
>   include/linux/coresight.h                     |  1 +
>   4 files changed, 47 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
> index 64e171eaad82..b03aa43d3cba 100644
> --- a/drivers/hwtracing/coresight/coresight-platform.c
> +++ b/drivers/hwtracing/coresight/coresight-platform.c
> @@ -796,6 +796,15 @@ int coresight_get_cpu(struct device *dev)
>   }
>   EXPORT_SYMBOL_GPL(coresight_get_cpu);
>   
> +int coresight_get_static_trace_id(struct device *dev, u32 *id)
> +{
> +	if (!is_of_node(dev->fwnode))
> +		return -EINVAL;
> +

You don't need this check, with the fwnode_property_read(). Rest looks 
fine to me.

Suzuki



> +	return fwnode_property_read_u32(dev_fwnode(dev), "arm,static-trace-id", id);
> +}
> +EXPORT_SYMBOL_GPL(coresight_get_static_trace_id);
> +
>   struct coresight_platform_data *
>   coresight_get_platform_data(struct device *dev)
>   {
> diff --git a/drivers/hwtracing/coresight/coresight-trace-id.c b/drivers/hwtracing/coresight/coresight-trace-id.c
> index d98e12cb30ec..269a5f7b279f 100644
> --- a/drivers/hwtracing/coresight/coresight-trace-id.c
> +++ b/drivers/hwtracing/coresight/coresight-trace-id.c
> @@ -12,6 +12,12 @@
>   
>   #include "coresight-trace-id.h"
>   
> +enum trace_id_flags {
> +	TRACE_ID_ANY = 0x0,
> +	TRACE_ID_PREFER_ODD = 0x1,
> +	TRACE_ID_REQ_STATIC = 0x2,
> +};
> +
>   /* Default trace ID map. Used in sysfs mode and for system sources */
>   static DEFINE_PER_CPU(atomic_t, id_map_default_cpu_ids) = ATOMIC_INIT(0);
>   static struct coresight_trace_id_map id_map_default = {
> @@ -74,16 +80,19 @@ static int coresight_trace_id_find_odd_id(struct coresight_trace_id_map *id_map)
>    * Otherwise allocate next available ID.
>    */
>   static int coresight_trace_id_alloc_new_id(struct coresight_trace_id_map *id_map,
> -					   int preferred_id, bool prefer_odd_id)
> +					   int preferred_id, unsigned int flags)
>   {
>   	int id = 0;
>   
>   	/* for backwards compatibility, cpu IDs may use preferred value */
> -	if (IS_VALID_CS_TRACE_ID(preferred_id) &&
> -	    !test_bit(preferred_id, id_map->used_ids)) {
> -		id = preferred_id;
> -		goto trace_id_allocated;
> -	} else if (prefer_odd_id) {
> +	if (IS_VALID_CS_TRACE_ID(preferred_id)) {
> +		if (!test_bit(preferred_id, id_map->used_ids)) {
> +			id = preferred_id;
> +			goto trace_id_allocated;
> +		} else if (WARN((flags & TRACE_ID_REQ_STATIC), "Trace ID %d is used.\n",
> +					preferred_id))
> +			return -EINVAL;
> +	} else if (flags & TRACE_ID_PREFER_ODD) {
>   	/* may use odd ids to avoid preferred legacy cpu IDs */
>   		id = coresight_trace_id_find_odd_id(id_map);
>   		if (id)
> @@ -153,7 +162,7 @@ static int _coresight_trace_id_get_cpu_id(int cpu, struct coresight_trace_id_map
>   	 */
>   	id = coresight_trace_id_alloc_new_id(id_map,
>   					     CORESIGHT_LEGACY_CPU_TRACE_ID(cpu),
> -					     false);
> +					     TRACE_ID_ANY);
>   	if (!IS_VALID_CS_TRACE_ID(id))
>   		goto get_cpu_id_out_unlock;
>   
> @@ -188,14 +197,15 @@ static void _coresight_trace_id_put_cpu_id(int cpu, struct coresight_trace_id_ma
>   	DUMP_ID_MAP(id_map);
>   }
>   
> -static int coresight_trace_id_map_get_system_id(struct coresight_trace_id_map *id_map)
> +static int coresight_trace_id_map_get_system_id(struct coresight_trace_id_map *id_map,
> +					int preferred_id, unsigned int traceid_flags)
>   {
>   	unsigned long flags;
>   	int id;
>   
>   	spin_lock_irqsave(&id_map->lock, flags);
>   	/* prefer odd IDs for system components to avoid legacy CPU IDS */
> -	id = coresight_trace_id_alloc_new_id(id_map, 0, true);
> +	id = coresight_trace_id_alloc_new_id(id_map, preferred_id, traceid_flags);
>   	spin_unlock_irqrestore(&id_map->lock, flags);
>   
>   	DUMP_ID(id);
> @@ -255,10 +265,18 @@ EXPORT_SYMBOL_GPL(coresight_trace_id_read_cpu_id_map);
>   
>   int coresight_trace_id_get_system_id(void)
>   {
> -	return coresight_trace_id_map_get_system_id(&id_map_default);
> +	return coresight_trace_id_map_get_system_id(&id_map_default, 0,
> +			TRACE_ID_PREFER_ODD);
>   }
>   EXPORT_SYMBOL_GPL(coresight_trace_id_get_system_id);
>   
> +int coresight_trace_id_get_static_system_id(int trace_id)
> +{
> +	return coresight_trace_id_map_get_system_id(&id_map_default,
> +			trace_id, TRACE_ID_REQ_STATIC);
> +}
> +EXPORT_SYMBOL_GPL(coresight_trace_id_get_static_system_id);
> +
>   void coresight_trace_id_put_system_id(int id)
>   {
>   	coresight_trace_id_map_put_system_id(&id_map_default, id);
> diff --git a/drivers/hwtracing/coresight/coresight-trace-id.h b/drivers/hwtracing/coresight/coresight-trace-id.h
> index 9aae50a553ca..db68e1ec56b6 100644
> --- a/drivers/hwtracing/coresight/coresight-trace-id.h
> +++ b/drivers/hwtracing/coresight/coresight-trace-id.h
> @@ -116,6 +116,15 @@ int coresight_trace_id_read_cpu_id_map(int cpu, struct coresight_trace_id_map *i
>    */
>   int coresight_trace_id_get_system_id(void);
>   
> +/**
> + * Allocate a CoreSight static trace ID for a system component.
> + *
> + * Used to allocate static IDs for system trace sources such as dummy source.
> + *
> + * return: Trace ID or -EINVAL if allocation is impossible.
> + */
> +int coresight_trace_id_get_static_system_id(int id);
> +
>   /**
>    * Release an allocated system trace ID.
>    *
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index c13342594278..129795873072 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -662,6 +662,7 @@ void coresight_relaxed_write64(struct coresight_device *csdev,
>   void coresight_write64(struct coresight_device *csdev, u64 val, u32 offset);
>   
>   extern int coresight_get_cpu(struct device *dev);
> +extern int coresight_get_static_trace_id(struct device *dev, u32 *id);
>   
>   struct coresight_platform_data *coresight_get_platform_data(struct device *dev);
>   struct coresight_connection *

Re: [PATCH v5 3/3] coresight: dummy: Add static trace id support for dummy source

[Suzuki K Poulose]

On 18/10/2024 04:22, Mao Jinlong wrote:
> Some dummy source has static trace id configured in HW and it cannot
> be changed via software programming. Configure the trace id in device
> tree and reserve the id when device probe.
> 
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>   .../sysfs-bus-coresight-devices-dummy-source  | 15 +++++
>   drivers/hwtracing/coresight/coresight-dummy.c | 59 +++++++++++++++++--
>   2 files changed, 70 insertions(+), 4 deletions(-)
>   create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-dummy-source
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-dummy-source b/Documentation/ABI/testing/sysfs-bus-coresight-devices-dummy-source
> new file mode 100644
> index 000000000000..c7d975e75d85
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-dummy-source
> @@ -0,0 +1,15 @@
> +What:		/sys/bus/coresight/devices/dummy_source<N>/enable_source
> +Date:		Oct 2024
> +KernelVersion:	6.13
> +Contact:	Mao Jinlong <quic_jinlmao@quicinc.com>
> +Description:	(RW) Enable/disable tracing of dummy source. A sink should be activated
> +		before enabling the source. The path of coresight components linking
> +		the source to the sink is configured and managed automatically by the
> +		coresight framework.
> +
> +What:		/sys/bus/coresight/devices/dummy_source<N>/traceid
> +Date:		Oct 2024
> +KernelVersion:	6.13
> +Contact:	Mao Jinlong <quic_jinlmao@quicinc.com>
> +Description:	(R) Show the trace ID that will appear in the trace stream
> +		coming from this trace entity.
> diff --git a/drivers/hwtracing/coresight/coresight-dummy.c b/drivers/hwtracing/coresight/coresight-dummy.c
> index bb85fa663ffc..602a7e89e311 100644
> --- a/drivers/hwtracing/coresight/coresight-dummy.c
> +++ b/drivers/hwtracing/coresight/coresight-dummy.c
> @@ -11,10 +11,12 @@
>   #include <linux/pm_runtime.h>
>   
>   #include "coresight-priv.h"
> +#include "coresight-trace-id.h"
>   
>   struct dummy_drvdata {
>   	struct device			*dev;
>   	struct coresight_device		*csdev;
> +	u8				traceid;
>   };
>   
>   DEFINE_CORESIGHT_DEVLIST(source_devs, "dummy_source");
> @@ -72,6 +74,32 @@ static const struct coresight_ops dummy_sink_cs_ops = {
>   	.sink_ops = &dummy_sink_ops,
>   };
>   
> +/* User can get the trace id of dummy source from this node. */
> +static ssize_t traceid_show(struct device *dev,
> +			    struct device_attribute *attr, char *buf)
> +{
> +	unsigned long val;
> +	struct dummy_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +
> +	val = drvdata->traceid;
> +	return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
> +}
> +static DEVICE_ATTR_RO(traceid);
> +
> +static struct attribute *coresight_dummy_attrs[] = {
> +	&dev_attr_traceid.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group coresight_dummy_group = {
> +	.attrs = coresight_dummy_attrs,
> +};
> +
> +static const struct attribute_group *coresight_dummy_groups[] = {
> +	&coresight_dummy_group,
> +	NULL,
> +};
> +
>   static int dummy_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> @@ -79,6 +107,11 @@ static int dummy_probe(struct platform_device *pdev)
>   	struct coresight_platform_data *pdata;
>   	struct dummy_drvdata *drvdata;
>   	struct coresight_desc desc = { 0 };
> +	int ret, trace_id;
> +
> +	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
>   
>   	if (of_device_is_compatible(node, "arm,coresight-dummy-source")) {
>   
> @@ -90,6 +123,25 @@ static int dummy_probe(struct platform_device *pdev)
>   		desc.subtype.source_subtype =
>   					CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS;
>   		desc.ops = &dummy_source_cs_ops;
> +		desc.groups = coresight_dummy_groups;
> +
> +		ret = coresight_get_static_trace_id(dev, &trace_id);
> +		if (!ret) {
> +			/* Get the static id if id is set in device tree. */
> +			ret = coresight_trace_id_get_static_system_id(trace_id);
> +			if (ret < 0)
> +				return ret;
> +
> +		} else {
> +			/* Get next available id if id is not set in device tree. */
> +			trace_id = coresight_trace_id_get_system_id();
> +			if (trace_id < 0) {
> +				ret = trace_id;
> +				return ret;
> +			}
> +		}
> +		drvdata->traceid = (u8)trace_id;
> +
>   	} else if (of_device_is_compatible(node, "arm,coresight-dummy-sink")) {
>   		desc.name = coresight_alloc_device_name(&sink_devs, dev);
>   		if (!desc.name)
> @@ -108,10 +160,6 @@ static int dummy_probe(struct platform_device *pdev)
>   		return PTR_ERR(pdata);
>   	pdev->dev.platform_data = pdata;
>   
> -	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> -	if (!drvdata)
> -		return -ENOMEM;
> -
>   	drvdata->dev = &pdev->dev;
>   	platform_set_drvdata(pdev, drvdata);
>   
> @@ -131,7 +179,10 @@ static void dummy_remove(struct platform_device *pdev)
>   {
>   	struct dummy_drvdata *drvdata = platform_get_drvdata(pdev);
>   	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;

^^ Why is this needed ? The rest looks fine to me

>   
> +	if (IS_VALID_CS_TRACE_ID(drvdata->traceid))
> +		coresight_trace_id_put_system_id(drvdata->traceid);
>   	pm_runtime_disable(dev);
>   	coresight_unregister(drvdata->csdev);
>   }

Re: [PATCH v5 3/3] coresight: dummy: Add static trace id support for dummy source

[Suzuki K Poulose]

On 21/10/2024 13:31, Suzuki K Poulose wrote:
> On 18/10/2024 04:22, Mao Jinlong wrote:
>> Some dummy source has static trace id configured in HW and it cannot
>> be changed via software programming. Configure the trace id in device
>> tree and reserve the id when device probe.
>>
>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>> ---
>>   .../sysfs-bus-coresight-devices-dummy-source  | 15 +++++
>>   drivers/hwtracing/coresight/coresight-dummy.c | 59 +++++++++++++++++--
>>   2 files changed, 70 insertions(+), 4 deletions(-)
>>   create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight- 
>> devices-dummy-source
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices- 
>> dummy-source b/Documentation/ABI/testing/sysfs-bus-coresight-devices- 
>> dummy-source
>> new file mode 100644
>> index 000000000000..c7d975e75d85
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-dummy-source
>> @@ -0,0 +1,15 @@
>> +What:        /sys/bus/coresight/devices/dummy_source<N>/enable_source
>> +Date:        Oct 2024
>> +KernelVersion:    6.13
>> +Contact:    Mao Jinlong <quic_jinlmao@quicinc.com>
>> +Description:    (RW) Enable/disable tracing of dummy source. A sink 
>> should be activated
>> +        before enabling the source. The path of coresight components 
>> linking
>> +        the source to the sink is configured and managed 
>> automatically by the
>> +        coresight framework.
>> +
>> +What:        /sys/bus/coresight/devices/dummy_source<N>/traceid
>> +Date:        Oct 2024
>> +KernelVersion:    6.13
>> +Contact:    Mao Jinlong <quic_jinlmao@quicinc.com>
>> +Description:    (R) Show the trace ID that will appear in the trace 
>> stream
>> +        coming from this trace entity.
>> diff --git a/drivers/hwtracing/coresight/coresight-dummy.c b/drivers/ 
>> hwtracing/coresight/coresight-dummy.c
>> index bb85fa663ffc..602a7e89e311 100644
>> --- a/drivers/hwtracing/coresight/coresight-dummy.c
>> +++ b/drivers/hwtracing/coresight/coresight-dummy.c
>> @@ -11,10 +11,12 @@
>>   #include <linux/pm_runtime.h>
>>   #include "coresight-priv.h"
>> +#include "coresight-trace-id.h"
>>   struct dummy_drvdata {
>>       struct device            *dev;
>>       struct coresight_device        *csdev;
>> +    u8                traceid;
>>   };
>>   DEFINE_CORESIGHT_DEVLIST(source_devs, "dummy_source");
>> @@ -72,6 +74,32 @@ static const struct coresight_ops dummy_sink_cs_ops 
>> = {
>>       .sink_ops = &dummy_sink_ops,
>>   };
>> +/* User can get the trace id of dummy source from this node. */
>> +static ssize_t traceid_show(struct device *dev,
>> +                struct device_attribute *attr, char *buf)
>> +{
>> +    unsigned long val;
>> +    struct dummy_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +
>> +    val = drvdata->traceid;
>> +    return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
>> +}
>> +static DEVICE_ATTR_RO(traceid);
>> +
>> +static struct attribute *coresight_dummy_attrs[] = {
>> +    &dev_attr_traceid.attr,
>> +    NULL,
>> +};
>> +
>> +static const struct attribute_group coresight_dummy_group = {
>> +    .attrs = coresight_dummy_attrs,
>> +};
>> +
>> +static const struct attribute_group *coresight_dummy_groups[] = {
>> +    &coresight_dummy_group,
>> +    NULL,
>> +};
>> +
>>   static int dummy_probe(struct platform_device *pdev)
>>   {
>>       struct device *dev = &pdev->dev;
>> @@ -79,6 +107,11 @@ static int dummy_probe(struct platform_device *pdev)
>>       struct coresight_platform_data *pdata;
>>       struct dummy_drvdata *drvdata;
>>       struct coresight_desc desc = { 0 };
>> +    int ret, trace_id;
>> +
>> +    drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>> +    if (!drvdata)
>> +        return -ENOMEM;
>>       if (of_device_is_compatible(node, "arm,coresight-dummy-source")) {
>> @@ -90,6 +123,25 @@ static int dummy_probe(struct platform_device *pdev)
>>           desc.subtype.source_subtype =
>>                       CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS;
>>           desc.ops = &dummy_source_cs_ops;
>> +        desc.groups = coresight_dummy_groups;
>> +
>> +        ret = coresight_get_static_trace_id(dev, &trace_id);
>> +        if (!ret) {
>> +            /* Get the static id if id is set in device tree. */
>> +            ret = coresight_trace_id_get_static_system_id(trace_id);

This may be worth an error message, it is a rare one. Othewise, there is
no clue on what caused the failure. Or have a specific error code as a
result ?

>> +            if (ret < 0)
>> +                return ret;

e.g., return -EBUSY ? /* Device or resource not available */

>> +
>> +        } else {
>> +            /* Get next available id if id is not set in device tree. */
>> +            trace_id = coresight_trace_id_get_system_id();
>> +            if (trace_id < 0) {
>> +                ret = trace_id;
>> +                return ret;
>> +            }
>> +        }
>> +        drvdata->traceid = (u8)trace_id;
>> +
>>       } else if (of_device_is_compatible(node, "arm,coresight-dummy- 
>> sink")) {
>>           desc.name = coresight_alloc_device_name(&sink_devs, dev);
>>           if (!desc.name)
>> @@ -108,10 +160,6 @@ static int dummy_probe(struct platform_device *pdev)
>>           return PTR_ERR(pdata);
>>       pdev->dev.platform_data = pdata;
>> -    drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>> -    if (!drvdata)
>> -        return -ENOMEM;
>> -
>>       drvdata->dev = &pdev->dev;
>>       platform_set_drvdata(pdev, drvdata);

Additionally we should drop the system_id if registering the coresight 
device fails.


Suzuki

>> @@ -131,7 +179,10 @@ static void dummy_remove(struct platform_device 
>> *pdev)
>>   {
>>       struct dummy_drvdata *drvdata = platform_get_drvdata(pdev);
>>       struct device *dev = &pdev->dev;
>> +    struct device_node *node = dev->of_node;
> 
> ^^ Why is this needed ? The rest looks fine to me
> 
>> +    if (IS_VALID_CS_TRACE_ID(drvdata->traceid))
>> +        coresight_trace_id_put_system_id(drvdata->traceid);
>>       pm_runtime_disable(dev);
>>       coresight_unregister(drvdata->csdev);
>>   }
> 

Re: [PATCH v5 2/3] coresight: Add support to get static id for system trace sources

[Mike Leach]

On Mon, 21 Oct 2024 at 13:09, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> On 18/10/2024 04:22, Mao Jinlong wrote:
> > Dynamic trace id was introduced in coresight subsystem, so trace id is
> > allocated dynamically. However, some hardware ATB source has static trace
> > id and it cannot be changed via software programming. For such source,
> > it can call coresight_get_static_trace_id to get the fixed trace id from
> > device node and pass id to coresight_trace_id_get_static_system_id to
> > reserve the id.
> >
> > Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> > ---
> >   .../hwtracing/coresight/coresight-platform.c  |  9 +++++
> >   .../hwtracing/coresight/coresight-trace-id.c  | 38 ++++++++++++++-----
> >   .../hwtracing/coresight/coresight-trace-id.h  |  9 +++++
> >   include/linux/coresight.h                     |  1 +
> >   4 files changed, 47 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
> > index 64e171eaad82..b03aa43d3cba 100644
> > --- a/drivers/hwtracing/coresight/coresight-platform.c
> > +++ b/drivers/hwtracing/coresight/coresight-platform.c
> > @@ -796,6 +796,15 @@ int coresight_get_cpu(struct device *dev)
> >   }
> >   EXPORT_SYMBOL_GPL(coresight_get_cpu);
> >
> > +int coresight_get_static_trace_id(struct device *dev, u32 *id)
> > +{
> > +     if (!is_of_node(dev->fwnode))
> > +             return -EINVAL;
> > +
>
> You don't need this check, with the fwnode_property_read(). Rest looks
> fine to me.
>
> Suzuki
>
>
>
> > +     return fwnode_property_read_u32(dev_fwnode(dev), "arm,static-trace-id", id);
> > +}
> > +EXPORT_SYMBOL_GPL(coresight_get_static_trace_id);
> > +
> >   struct coresight_platform_data *
> >   coresight_get_platform_data(struct device *dev)
> >   {
> > diff --git a/drivers/hwtracing/coresight/coresight-trace-id.c b/drivers/hwtracing/coresight/coresight-trace-id.c
> > index d98e12cb30ec..269a5f7b279f 100644
> > --- a/drivers/hwtracing/coresight/coresight-trace-id.c
> > +++ b/drivers/hwtracing/coresight/coresight-trace-id.c
> > @@ -12,6 +12,12 @@
> >
> >   #include "coresight-trace-id.h"
> >
> > +enum trace_id_flags {
> > +     TRACE_ID_ANY = 0x0,
> > +     TRACE_ID_PREFER_ODD = 0x1,
> > +     TRACE_ID_REQ_STATIC = 0x2,
> > +};
> > +
> >   /* Default trace ID map. Used in sysfs mode and for system sources */
> >   static DEFINE_PER_CPU(atomic_t, id_map_default_cpu_ids) = ATOMIC_INIT(0);
> >   static struct coresight_trace_id_map id_map_default = {
> > @@ -74,16 +80,19 @@ static int coresight_trace_id_find_odd_id(struct coresight_trace_id_map *id_map)
> >    * Otherwise allocate next available ID.
> >    */
> >   static int coresight_trace_id_alloc_new_id(struct coresight_trace_id_map *id_map,
> > -                                        int preferred_id, bool prefer_odd_id)
> > +                                        int preferred_id, unsigned int flags)
> >   {
> >       int id = 0;
> >
> >       /* for backwards compatibility, cpu IDs may use preferred value */
> > -     if (IS_VALID_CS_TRACE_ID(preferred_id) &&
> > -         !test_bit(preferred_id, id_map->used_ids)) {
> > -             id = preferred_id;
> > -             goto trace_id_allocated;
> > -     } else if (prefer_odd_id) {
> > +     if (IS_VALID_CS_TRACE_ID(preferred_id)) {
> > +             if (!test_bit(preferred_id, id_map->used_ids)) {
> > +                     id = preferred_id;
> > +                     goto trace_id_allocated;
> > +             } else if (WARN((flags & TRACE_ID_REQ_STATIC), "Trace ID %d is used.\n",

If another version of this set is done, then consider making this
message more specific - e.g.  "Requested Static Trace ID %d is not
available"

> > +                                     preferred_id))
> > +                     return -EINVAL;
> > +     } else if (flags & TRACE_ID_PREFER_ODD) {
> >       /* may use odd ids to avoid preferred legacy cpu IDs */
> >               id = coresight_trace_id_find_odd_id(id_map);
> >               if (id)
> > @@ -153,7 +162,7 @@ static int _coresight_trace_id_get_cpu_id(int cpu, struct coresight_trace_id_map
> >        */
> >       id = coresight_trace_id_alloc_new_id(id_map,
> >                                            CORESIGHT_LEGACY_CPU_TRACE_ID(cpu),
> > -                                          false);
> > +                                          TRACE_ID_ANY);
> >       if (!IS_VALID_CS_TRACE_ID(id))
> >               goto get_cpu_id_out_unlock;
> >
> > @@ -188,14 +197,15 @@ static void _coresight_trace_id_put_cpu_id(int cpu, struct coresight_trace_id_ma
> >       DUMP_ID_MAP(id_map);
> >   }
> >
> > -static int coresight_trace_id_map_get_system_id(struct coresight_trace_id_map *id_map)
> > +static int coresight_trace_id_map_get_system_id(struct coresight_trace_id_map *id_map,
> > +                                     int preferred_id, unsigned int traceid_flags)
> >   {
> >       unsigned long flags;
> >       int id;
> >
> >       spin_lock_irqsave(&id_map->lock, flags);
> >       /* prefer odd IDs for system components to avoid legacy CPU IDS */

This comment now belongs in coresight_trace_id_get_system_id()

> > -     id = coresight_trace_id_alloc_new_id(id_map, 0, true);
> > +     id = coresight_trace_id_alloc_new_id(id_map, preferred_id, traceid_flags);
> >       spin_unlock_irqrestore(&id_map->lock, flags);
> >
> >       DUMP_ID(id);
> > @@ -255,10 +265,18 @@ EXPORT_SYMBOL_GPL(coresight_trace_id_read_cpu_id_map);
> >
> >   int coresight_trace_id_get_system_id(void)
> >   {
> > -     return coresight_trace_id_map_get_system_id(&id_map_default);
> > +     return coresight_trace_id_map_get_system_id(&id_map_default, 0,
> > +                     TRACE_ID_PREFER_ODD);
> >   }
> >   EXPORT_SYMBOL_GPL(coresight_trace_id_get_system_id);
> >
> > +int coresight_trace_id_get_static_system_id(int trace_id)
> > +{
> > +     return coresight_trace_id_map_get_system_id(&id_map_default,
> > +                     trace_id, TRACE_ID_REQ_STATIC);
> > +}
> > +EXPORT_SYMBOL_GPL(coresight_trace_id_get_static_system_id);
> > +
> >   void coresight_trace_id_put_system_id(int id)
> >   {
> >       coresight_trace_id_map_put_system_id(&id_map_default, id);
> > diff --git a/drivers/hwtracing/coresight/coresight-trace-id.h b/drivers/hwtracing/coresight/coresight-trace-id.h
> > index 9aae50a553ca..db68e1ec56b6 100644
> > --- a/drivers/hwtracing/coresight/coresight-trace-id.h
> > +++ b/drivers/hwtracing/coresight/coresight-trace-id.h
> > @@ -116,6 +116,15 @@ int coresight_trace_id_read_cpu_id_map(int cpu, struct coresight_trace_id_map *i
> >    */
> >   int coresight_trace_id_get_system_id(void);
> >
> > +/**
> > + * Allocate a CoreSight static trace ID for a system component.
> > + *
> > + * Used to allocate static IDs for system trace sources such as dummy source.
> > + *
> > + * return: Trace ID or -EINVAL if allocation is impossible.
> > + */
> > +int coresight_trace_id_get_static_system_id(int id);
> > +
> >   /**
> >    * Release an allocated system trace ID.
> >    *
> > diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> > index c13342594278..129795873072 100644
> > --- a/include/linux/coresight.h
> > +++ b/include/linux/coresight.h
> > @@ -662,6 +662,7 @@ void coresight_relaxed_write64(struct coresight_device *csdev,
> >   void coresight_write64(struct coresight_device *csdev, u64 val, u32 offset);
> >
> >   extern int coresight_get_cpu(struct device *dev);
> > +extern int coresight_get_static_trace_id(struct device *dev, u32 *id);
> >
> >   struct coresight_platform_data *coresight_get_platform_data(struct device *dev);
> >   struct coresight_connection *
>

Other than minor issues above...

Reviewed-by: Mike Leach <mike.leach@linaro.org>

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

Re: [PATCH v5 2/3] coresight: Add support to get static id for system trace sources

[Suzuki K Poulose]

On 22/10/2024 10:42, Mike Leach wrote:
> On Mon, 21 Oct 2024 at 13:09, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>>
>> On 18/10/2024 04:22, Mao Jinlong wrote:
>>> Dynamic trace id was introduced in coresight subsystem, so trace id is
>>> allocated dynamically. However, some hardware ATB source has static trace
>>> id and it cannot be changed via software programming. For such source,
>>> it can call coresight_get_static_trace_id to get the fixed trace id from
>>> device node and pass id to coresight_trace_id_get_static_system_id to
>>> reserve the id.
>>>
>>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>>> ---
>>>    .../hwtracing/coresight/coresight-platform.c  |  9 +++++
>>>    .../hwtracing/coresight/coresight-trace-id.c  | 38 ++++++++++++++-----
>>>    .../hwtracing/coresight/coresight-trace-id.h  |  9 +++++
>>>    include/linux/coresight.h                     |  1 +
>>>    4 files changed, 47 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
>>> index 64e171eaad82..b03aa43d3cba 100644
>>> --- a/drivers/hwtracing/coresight/coresight-platform.c
>>> +++ b/drivers/hwtracing/coresight/coresight-platform.c
>>> @@ -796,6 +796,15 @@ int coresight_get_cpu(struct device *dev)
>>>    }
>>>    EXPORT_SYMBOL_GPL(coresight_get_cpu);
>>>
>>> +int coresight_get_static_trace_id(struct device *dev, u32 *id)
>>> +{
>>> +     if (!is_of_node(dev->fwnode))
>>> +             return -EINVAL;
>>> +
>>
>> You don't need this check, with the fwnode_property_read(). Rest looks
>> fine to me.
>>
>> Suzuki
>>
>>
>>
>>> +     return fwnode_property_read_u32(dev_fwnode(dev), "arm,static-trace-id", id);
>>> +}
>>> +EXPORT_SYMBOL_GPL(coresight_get_static_trace_id);
>>> +
>>>    struct coresight_platform_data *
>>>    coresight_get_platform_data(struct device *dev)
>>>    {
>>> diff --git a/drivers/hwtracing/coresight/coresight-trace-id.c b/drivers/hwtracing/coresight/coresight-trace-id.c
>>> index d98e12cb30ec..269a5f7b279f 100644
>>> --- a/drivers/hwtracing/coresight/coresight-trace-id.c
>>> +++ b/drivers/hwtracing/coresight/coresight-trace-id.c
>>> @@ -12,6 +12,12 @@
>>>
>>>    #include "coresight-trace-id.h"
>>>
>>> +enum trace_id_flags {
>>> +     TRACE_ID_ANY = 0x0,
>>> +     TRACE_ID_PREFER_ODD = 0x1,
>>> +     TRACE_ID_REQ_STATIC = 0x2,
>>> +};
>>> +
>>>    /* Default trace ID map. Used in sysfs mode and for system sources */
>>>    static DEFINE_PER_CPU(atomic_t, id_map_default_cpu_ids) = ATOMIC_INIT(0);
>>>    static struct coresight_trace_id_map id_map_default = {
>>> @@ -74,16 +80,19 @@ static int coresight_trace_id_find_odd_id(struct coresight_trace_id_map *id_map)
>>>     * Otherwise allocate next available ID.
>>>     */
>>>    static int coresight_trace_id_alloc_new_id(struct coresight_trace_id_map *id_map,
>>> -                                        int preferred_id, bool prefer_odd_id)
>>> +                                        int preferred_id, unsigned int flags)
>>>    {
>>>        int id = 0;
>>>
>>>        /* for backwards compatibility, cpu IDs may use preferred value */
>>> -     if (IS_VALID_CS_TRACE_ID(preferred_id) &&
>>> -         !test_bit(preferred_id, id_map->used_ids)) {
>>> -             id = preferred_id;
>>> -             goto trace_id_allocated;
>>> -     } else if (prefer_odd_id) {
>>> +     if (IS_VALID_CS_TRACE_ID(preferred_id)) {
>>> +             if (!test_bit(preferred_id, id_map->used_ids)) {
>>> +                     id = preferred_id;
>>> +                     goto trace_id_allocated;
>>> +             } else if (WARN((flags & TRACE_ID_REQ_STATIC), "Trace ID %d is used.\n",
> 
> If another version of this set is done, then consider making this
> message more specific - e.g.  "Requested Static Trace ID %d is not
> available"

Actually this reminds me, we shouldn't WARN() here, and that too for
such minor error case. WARN() could result in panic() and as such the 
system is functional with this error and should be avoided.
We should leave it to the user of the call to log the error, so that
it is easier to understand which device failed to probe and why.

Agree with the rest of the comments from Mike

Suzuki


> 
>>> +                                     preferred_id))
>>> +                     return -EINVAL;
>>> +     } else if (flags & TRACE_ID_PREFER_ODD) {
>>>        /* may use odd ids to avoid preferred legacy cpu IDs */
>>>                id = coresight_trace_id_find_odd_id(id_map);
>>>                if (id)
>>> @@ -153,7 +162,7 @@ static int _coresight_trace_id_get_cpu_id(int cpu, struct coresight_trace_id_map
>>>         */
>>>        id = coresight_trace_id_alloc_new_id(id_map,
>>>                                             CORESIGHT_LEGACY_CPU_TRACE_ID(cpu),
>>> -                                          false);
>>> +                                          TRACE_ID_ANY);
>>>        if (!IS_VALID_CS_TRACE_ID(id))
>>>                goto get_cpu_id_out_unlock;
>>>
>>> @@ -188,14 +197,15 @@ static void _coresight_trace_id_put_cpu_id(int cpu, struct coresight_trace_id_ma
>>>        DUMP_ID_MAP(id_map);
>>>    }
>>>
>>> -static int coresight_trace_id_map_get_system_id(struct coresight_trace_id_map *id_map)
>>> +static int coresight_trace_id_map_get_system_id(struct coresight_trace_id_map *id_map,
>>> +                                     int preferred_id, unsigned int traceid_flags)
>>>    {
>>>        unsigned long flags;
>>>        int id;
>>>
>>>        spin_lock_irqsave(&id_map->lock, flags);
>>>        /* prefer odd IDs for system components to avoid legacy CPU IDS */
> 
> This comment now belongs in coresight_trace_id_get_system_id()
> 
>>> -     id = coresight_trace_id_alloc_new_id(id_map, 0, true);
>>> +     id = coresight_trace_id_alloc_new_id(id_map, preferred_id, traceid_flags);
>>>        spin_unlock_irqrestore(&id_map->lock, flags);
>>>
>>>        DUMP_ID(id);
>>> @@ -255,10 +265,18 @@ EXPORT_SYMBOL_GPL(coresight_trace_id_read_cpu_id_map);
>>>
>>>    int coresight_trace_id_get_system_id(void)
>>>    {
>>> -     return coresight_trace_id_map_get_system_id(&id_map_default);
>>> +     return coresight_trace_id_map_get_system_id(&id_map_default, 0,
>>> +                     TRACE_ID_PREFER_ODD);
>>>    }
>>>    EXPORT_SYMBOL_GPL(coresight_trace_id_get_system_id);
>>>
>>> +int coresight_trace_id_get_static_system_id(int trace_id)
>>> +{
>>> +     return coresight_trace_id_map_get_system_id(&id_map_default,
>>> +                     trace_id, TRACE_ID_REQ_STATIC);
>>> +}
>>> +EXPORT_SYMBOL_GPL(coresight_trace_id_get_static_system_id);
>>> +
>>>    void coresight_trace_id_put_system_id(int id)
>>>    {
>>>        coresight_trace_id_map_put_system_id(&id_map_default, id);
>>> diff --git a/drivers/hwtracing/coresight/coresight-trace-id.h b/drivers/hwtracing/coresight/coresight-trace-id.h
>>> index 9aae50a553ca..db68e1ec56b6 100644
>>> --- a/drivers/hwtracing/coresight/coresight-trace-id.h
>>> +++ b/drivers/hwtracing/coresight/coresight-trace-id.h
>>> @@ -116,6 +116,15 @@ int coresight_trace_id_read_cpu_id_map(int cpu, struct coresight_trace_id_map *i
>>>     */
>>>    int coresight_trace_id_get_system_id(void);
>>>
>>> +/**
>>> + * Allocate a CoreSight static trace ID for a system component.
>>> + *
>>> + * Used to allocate static IDs for system trace sources such as dummy source.
>>> + *
>>> + * return: Trace ID or -EINVAL if allocation is impossible.
>>> + */
>>> +int coresight_trace_id_get_static_system_id(int id);
>>> +
>>>    /**
>>>     * Release an allocated system trace ID.
>>>     *
>>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>>> index c13342594278..129795873072 100644
>>> --- a/include/linux/coresight.h
>>> +++ b/include/linux/coresight.h
>>> @@ -662,6 +662,7 @@ void coresight_relaxed_write64(struct coresight_device *csdev,
>>>    void coresight_write64(struct coresight_device *csdev, u64 val, u32 offset);
>>>
>>>    extern int coresight_get_cpu(struct device *dev);
>>> +extern int coresight_get_static_trace_id(struct device *dev, u32 *id);
>>>
>>>    struct coresight_platform_data *coresight_get_platform_data(struct device *dev);
>>>    struct coresight_connection *
>>
> 
> Other than minor issues above...
> 
> Reviewed-by: Mike Leach <mike.leach@linaro.org>
> 

Re: [PATCH v5 3/3] coresight: dummy: Add static trace id support for dummy source

[Mike Leach]

On Mon, 21 Oct 2024 at 13:36, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> On 21/10/2024 13:31, Suzuki K Poulose wrote:
> > On 18/10/2024 04:22, Mao Jinlong wrote:
> >> Some dummy source has static trace id configured in HW and it cannot
> >> be changed via software programming. Configure the trace id in device
> >> tree and reserve the id when device probe.
> >>
> >> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> >> ---
> >>   .../sysfs-bus-coresight-devices-dummy-source  | 15 +++++
> >>   drivers/hwtracing/coresight/coresight-dummy.c | 59 +++++++++++++++++--
> >>   2 files changed, 70 insertions(+), 4 deletions(-)
> >>   create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-
> >> devices-dummy-source
> >>
> >> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-
> >> dummy-source b/Documentation/ABI/testing/sysfs-bus-coresight-devices-
> >> dummy-source
> >> new file mode 100644
> >> index 000000000000..c7d975e75d85
> >> --- /dev/null
> >> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-dummy-source
> >> @@ -0,0 +1,15 @@
> >> +What:        /sys/bus/coresight/devices/dummy_source<N>/enable_source
> >> +Date:        Oct 2024
> >> +KernelVersion:    6.13
> >> +Contact:    Mao Jinlong <quic_jinlmao@quicinc.com>
> >> +Description:    (RW) Enable/disable tracing of dummy source. A sink
> >> should be activated
> >> +        before enabling the source. The path of coresight components
> >> linking
> >> +        the source to the sink is configured and managed
> >> automatically by the
> >> +        coresight framework.
> >> +
> >> +What:        /sys/bus/coresight/devices/dummy_source<N>/traceid
> >> +Date:        Oct 2024
> >> +KernelVersion:    6.13
> >> +Contact:    Mao Jinlong <quic_jinlmao@quicinc.com>
> >> +Description:    (R) Show the trace ID that will appear in the trace
> >> stream
> >> +        coming from this trace entity.
> >> diff --git a/drivers/hwtracing/coresight/coresight-dummy.c b/drivers/
> >> hwtracing/coresight/coresight-dummy.c
> >> index bb85fa663ffc..602a7e89e311 100644
> >> --- a/drivers/hwtracing/coresight/coresight-dummy.c
> >> +++ b/drivers/hwtracing/coresight/coresight-dummy.c
> >> @@ -11,10 +11,12 @@
> >>   #include <linux/pm_runtime.h>
> >>   #include "coresight-priv.h"
> >> +#include "coresight-trace-id.h"
> >>   struct dummy_drvdata {
> >>       struct device            *dev;
> >>       struct coresight_device        *csdev;
> >> +    u8                traceid;
> >>   };
> >>   DEFINE_CORESIGHT_DEVLIST(source_devs, "dummy_source");
> >> @@ -72,6 +74,32 @@ static const struct coresight_ops dummy_sink_cs_ops
> >> = {
> >>       .sink_ops = &dummy_sink_ops,
> >>   };
> >> +/* User can get the trace id of dummy source from this node. */
> >> +static ssize_t traceid_show(struct device *dev,
> >> +                struct device_attribute *attr, char *buf)
> >> +{
> >> +    unsigned long val;
> >> +    struct dummy_drvdata *drvdata = dev_get_drvdata(dev->parent);
> >> +
> >> +    val = drvdata->traceid;
> >> +    return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);

sysfs_emit() is the convention moving forwards - this handles all the
PAGE_SIZE issues automatically

> >> +}
> >> +static DEVICE_ATTR_RO(traceid);
> >> +
> >> +static struct attribute *coresight_dummy_attrs[] = {
> >> +    &dev_attr_traceid.attr,
> >> +    NULL,
> >> +};
> >> +
> >> +static const struct attribute_group coresight_dummy_group = {
> >> +    .attrs = coresight_dummy_attrs,
> >> +};
> >> +
> >> +static const struct attribute_group *coresight_dummy_groups[] = {
> >> +    &coresight_dummy_group,
> >> +    NULL,
> >> +};
> >> +
> >>   static int dummy_probe(struct platform_device *pdev)
> >>   {
> >>       struct device *dev = &pdev->dev;
> >> @@ -79,6 +107,11 @@ static int dummy_probe(struct platform_device *pdev)
> >>       struct coresight_platform_data *pdata;
> >>       struct dummy_drvdata *drvdata;
> >>       struct coresight_desc desc = { 0 };
> >> +    int ret, trace_id;
> >> +
> >> +    drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> >> +    if (!drvdata)
> >> +        return -ENOMEM;
> >>       if (of_device_is_compatible(node, "arm,coresight-dummy-source")) {
> >> @@ -90,6 +123,25 @@ static int dummy_probe(struct platform_device *pdev)
> >>           desc.subtype.source_subtype =
> >>                       CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS;
> >>           desc.ops = &dummy_source_cs_ops;
> >> +        desc.groups = coresight_dummy_groups;
> >> +
> >> +        ret = coresight_get_static_trace_id(dev, &trace_id);
> >> +        if (!ret) {
> >> +            /* Get the static id if id is set in device tree. */
> >> +            ret = coresight_trace_id_get_static_system_id(trace_id);
>
> This may be worth an error message, it is a rare one. Othewise, there is
> no clue on what caused the failure. Or have a specific error code as a
> result ?
>

The called function does actually emit an error message - but a
comment to that effect would clarify this.

> >> +            if (ret < 0)
> >> +                return ret;
>
> e.g., return -EBUSY ? /* Device or resource not available */
>
> >> +
> >> +        } else {
> >> +            /* Get next available id if id is not set in device tree. */
> >> +            trace_id = coresight_trace_id_get_system_id();
> >> +            if (trace_id < 0) {
> >> +                ret = trace_id;
> >> +                return ret;
> >> +            }
> >> +        }
> >> +        drvdata->traceid = (u8)trace_id;
> >> +
> >>       } else if (of_device_is_compatible(node, "arm,coresight-dummy-
> >> sink")) {
> >>           desc.name = coresight_alloc_device_name(&sink_devs, dev);
> >>           if (!desc.name)
> >> @@ -108,10 +160,6 @@ static int dummy_probe(struct platform_device *pdev)
> >>           return PTR_ERR(pdata);
> >>       pdev->dev.platform_data = pdata;
> >> -    drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> >> -    if (!drvdata)
> >> -        return -ENOMEM;
> >> -
> >>       drvdata->dev = &pdev->dev;
> >>       platform_set_drvdata(pdev, drvdata);
>
> Additionally we should drop the system_id if registering the coresight
> device fails.
>
>
> Suzuki
>
> >> @@ -131,7 +179,10 @@ static void dummy_remove(struct platform_device
> >> *pdev)
> >>   {
> >>       struct dummy_drvdata *drvdata = platform_get_drvdata(pdev);
> >>       struct device *dev = &pdev->dev;
> >> +    struct device_node *node = dev->of_node;
> >
> > ^^ Why is this needed ? The rest looks fine to me
> >
> >> +    if (IS_VALID_CS_TRACE_ID(drvdata->traceid))
> >> +        coresight_trace_id_put_system_id(drvdata->traceid);
> >>       pm_runtime_disable(dev);
> >>       coresight_unregister(drvdata->csdev);
> >>   }
> >
>

Mike

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

Re: [PATCH v5 1/3] dt-bindings: arm: Add arm,static-trace-id for coresight dummy source

[Mike Leach]

On Fri, 18 Oct 2024 at 04:22, Mao Jinlong <quic_jinlmao@quicinc.com> wrote:
>
> Some dummy source HW has static trace id which cannot be changed via
> software programming. Add arm,static-trace-id for static id support to
> coresight dummy source.
>
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  .../devicetree/bindings/arm/arm,coresight-dummy-source.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/arm,coresight-dummy-source.yaml b/Documentation/devicetree/bindings/arm/arm,coresight-dummy-source.yaml
> index 04a8c37b4aff..742dc4e25d3b 100644
> --- a/Documentation/devicetree/bindings/arm/arm,coresight-dummy-source.yaml
> +++ b/Documentation/devicetree/bindings/arm/arm,coresight-dummy-source.yaml
> @@ -38,6 +38,12 @@ properties:
>      enum:
>        - arm,coresight-dummy-source
>
> +  arm,static-trace-id:
> +    description: If dummy source needs static id support, use this to set trace id.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    maximum: 111
> +
>    out-ports:
>      $ref: /schemas/graph.yaml#/properties/ports
>
> --
> 2.17.1
>

Reviewed-by: Mike Leach <mike.leach@linaro.org>

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