[PATCH v1 0/3] coresight: Add reserve trace id support

[PATCH v1 0/3] coresight: Add reserve 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
"trace-id = <xxx>", and call coresight_trace_id_reserve_system_id in
device probe function. The id will be reserved for the HW all the time
if the device is probed.

Mao Jinlong (3):
  dt-bindings: arm: Add trace-id for coresight dummy source
  coresight: Add reserve trace id support
  coresight: dummy: Add reserve atid support for dummy source

 .../sysfs-bus-coresight-devices-dummy-source  | 15 +++++
 .../arm/arm,coresight-dummy-source.yaml       |  6 ++
 drivers/hwtracing/coresight/coresight-dummy.c | 56 +++++++++++++++++--
 .../hwtracing/coresight/coresight-platform.c  | 26 +++++++++
 .../hwtracing/coresight/coresight-trace-id.c  | 24 ++++++++
 .../hwtracing/coresight/coresight-trace-id.h  | 11 ++++
 include/linux/coresight.h                     |  1 +
 7 files changed, 135 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-dummy-source

-- 
2.41.0

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

[Mao Jinlong]

Add trace-id for static id support to coresight dummy source.

Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
---
 .../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 6745b4cc8f1c..9adf34ea450e 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
 
+  trace-id:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      If dummy source needs static id support, use this to set trace id.
+      The range is 1 to 127.
+
   out-ports:
     $ref: /schemas/graph.yaml#/properties/ports
 
-- 
2.41.0

[PATCH v1 2/3] coresight: Add reserve trace id support

[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. Reserve trace id
for this kind of hardware source.

Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
---
 .../hwtracing/coresight/coresight-platform.c  | 26 +++++++++++++++++++
 .../hwtracing/coresight/coresight-trace-id.c  | 24 +++++++++++++++++
 .../hwtracing/coresight/coresight-trace-id.h  | 11 ++++++++
 include/linux/coresight.h                     |  1 +
 4 files changed, 62 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
index 9d550f5697fa..d3e22a2608df 100644
--- a/drivers/hwtracing/coresight/coresight-platform.c
+++ b/drivers/hwtracing/coresight/coresight-platform.c
@@ -183,6 +183,17 @@ static int of_coresight_get_cpu(struct device *dev)
 	return cpu;
 }
 
+/*
+ * of_coresight_get_trace_id: Get the atid of a source device.
+ *
+ * Returns 0 on success.
+ */
+static int of_coresight_get_trace_id(struct device *dev, u32 *id)
+{
+
+	return of_property_read_u32(dev->of_node, "trace-id", id);
+}
+
 /*
  * of_coresight_parse_endpoint : Parse the given output endpoint @ep
  * and fill the connection information in @pdata->out_conns
@@ -315,6 +326,12 @@ static inline int of_coresight_get_cpu(struct device *dev)
 {
 	return -ENODEV;
 }
+
+static int of_coresight_get_trace_id(struct device *dev, u32 *id)
+{
+	return -ENODEV;
+}
+
 #endif
 
 #ifdef CONFIG_ACPI
@@ -794,6 +811,15 @@ int coresight_get_cpu(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(coresight_get_cpu);
 
+int coresight_get_trace_id(struct device *dev, u32 *id)
+{
+	if (!is_of_node(dev->fwnode))
+		return -EINVAL;
+
+	return of_coresight_get_trace_id(dev, id);
+}
+EXPORT_SYMBOL_GPL(coresight_get_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 af5b4ef59cea..536a34e9de6f 100644
--- a/drivers/hwtracing/coresight/coresight-trace-id.c
+++ b/drivers/hwtracing/coresight/coresight-trace-id.c
@@ -110,6 +110,24 @@ static int coresight_trace_id_alloc_new_id(struct coresight_trace_id_map *id_map
 	return id;
 }
 
+static int coresight_trace_id_set(int id, struct coresight_trace_id_map *id_map)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&id_map_lock, flags);
+
+	if (WARN(!IS_VALID_CS_TRACE_ID(id), "Invalid Trace ID %d\n", id))
+		return -EINVAL;
+	if (WARN(test_bit(id, id_map->used_ids), "ID is already used: %d\n", id))
+		return -EINVAL;
+	set_bit(id, id_map->used_ids);
+
+	DUMP_ID_MAP(id_map);
+
+	spin_unlock_irqrestore(&id_map_lock, flags);
+	return 0;
+}
+
 static void coresight_trace_id_free(int id, struct coresight_trace_id_map *id_map)
 {
 	if (WARN(!IS_VALID_CS_TRACE_ID(id), "Invalid Trace ID %d\n", id))
@@ -275,6 +293,12 @@ int coresight_trace_id_get_system_id(void)
 }
 EXPORT_SYMBOL_GPL(coresight_trace_id_get_system_id);
 
+int coresight_trace_id_reserve_system_id(int id)
+{
+	return coresight_trace_id_set(id, &id_map_default);
+}
+EXPORT_SYMBOL_GPL(coresight_trace_id_reserve_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 3797777d367e..255716887051 100644
--- a/drivers/hwtracing/coresight/coresight-trace-id.h
+++ b/drivers/hwtracing/coresight/coresight-trace-id.h
@@ -122,6 +122,17 @@ int coresight_trace_id_read_cpu_id(int cpu);
  */
 int coresight_trace_id_get_system_id(void);
 
+/**
+ * Reserve trace id for a system component.
+ *
+ * Reserve the trace id if system component needs a static id for the trace.
+ *
+ * @id: value of trace ID.
+ *
+ * return: 0 if reserve successfully or -EINVAL if fail.
+ */
+int coresight_trace_id_reserve_system_id(int id);
+
 /**
  * Release an allocated system trace ID.
  *
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index f09ace92176e..f65dc20ca76e 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -643,6 +643,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_trace_id(struct device *dev, u32 *id);
 
 struct coresight_platform_data *coresight_get_platform_data(struct device *dev);
 struct coresight_connection *
-- 
2.41.0

[PATCH v1 3/3] coresight: dummy: Add reserve atid 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 | 56 +++++++++++++++++--
 2 files changed, 67 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..608ea6d0f39a
--- /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:		May 2024
+KernelVersion:	6.9
+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:		May 2024
+KernelVersion:	6.9
+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 ac70c0b491be..2564e1c8ae26 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");
@@ -67,6 +69,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;
@@ -74,6 +102,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")) {
 
@@ -85,6 +118,22 @@ 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_trace_id(dev, &trace_id);
+		if (!ret) {
+			ret = coresight_trace_id_reserve_system_id(trace_id);
+			if (ret)
+				return ret;
+		} else {
+			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)
@@ -103,10 +152,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);
 
@@ -126,7 +171,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 (of_device_is_compatible(node, "arm,coresight-dummy-source"))
+		coresight_trace_id_put_system_id(drvdata->traceid);
 	pm_runtime_disable(dev);
 	coresight_unregister(drvdata->csdev);
 }
-- 
2.41.0

Re: [PATCH v1 2/3] coresight: Add reserve trace id support

[James Clark]

On 16/05/2024 04:56, 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. Reserve trace id
> for this kind of hardware source.
> 
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>  .../hwtracing/coresight/coresight-platform.c  | 26 +++++++++++++++++++
>  .../hwtracing/coresight/coresight-trace-id.c  | 24 +++++++++++++++++
>  .../hwtracing/coresight/coresight-trace-id.h  | 11 ++++++++
>  include/linux/coresight.h                     |  1 +
>  4 files changed, 62 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
> index 9d550f5697fa..d3e22a2608df 100644
> --- a/drivers/hwtracing/coresight/coresight-platform.c
> +++ b/drivers/hwtracing/coresight/coresight-platform.c
> @@ -183,6 +183,17 @@ static int of_coresight_get_cpu(struct device *dev)
>  	return cpu;
>  }
>  
> +/*
> + * of_coresight_get_trace_id: Get the atid of a source device.
> + *
> + * Returns 0 on success.
> + */
> +static int of_coresight_get_trace_id(struct device *dev, u32 *id)
> +{
> +
> +	return of_property_read_u32(dev->of_node, "trace-id", id);
> +}
> +
>  /*
>   * of_coresight_parse_endpoint : Parse the given output endpoint @ep
>   * and fill the connection information in @pdata->out_conns
> @@ -315,6 +326,12 @@ static inline int of_coresight_get_cpu(struct device *dev)
>  {
>  	return -ENODEV;
>  }
> +
> +static int of_coresight_get_trace_id(struct device *dev, u32 *id)
> +{
> +	return -ENODEV;
> +}
> +
>  #endif
>  
>  #ifdef CONFIG_ACPI
> @@ -794,6 +811,15 @@ int coresight_get_cpu(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(coresight_get_cpu);
>  
> +int coresight_get_trace_id(struct device *dev, u32 *id)
> +{
> +	if (!is_of_node(dev->fwnode))
> +		return -EINVAL;
> +
> +	return of_coresight_get_trace_id(dev, id);
> +}
> +EXPORT_SYMBOL_GPL(coresight_get_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 af5b4ef59cea..536a34e9de6f 100644
> --- a/drivers/hwtracing/coresight/coresight-trace-id.c
> +++ b/drivers/hwtracing/coresight/coresight-trace-id.c
> @@ -110,6 +110,24 @@ static int coresight_trace_id_alloc_new_id(struct coresight_trace_id_map *id_map
>  	return id;
>  }
>  
> +static int coresight_trace_id_set(int id, struct coresight_trace_id_map *id_map)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&id_map_lock, flags);
> +
> +	if (WARN(!IS_VALID_CS_TRACE_ID(id), "Invalid Trace ID %d\n", id))
> +		return -EINVAL;
> +	if (WARN(test_bit(id, id_map->used_ids), "ID is already used: %d\n", id))
> +		return -EINVAL;

Do these returns not skip unlocking the spinlock?

It might be slightly fewer changes if we update the existing
coresight_trace_id_alloc_new_id() to add a new "only_preferred" option.

Then use the existing system id allocator which already handles the lock
and unlock properly:

  static int coresight_trace_id_map_get_system_id(struct
                             coresight_trace_id_map *id_map, int id,

                             bool only_preferred)
  {
  ...
	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, id, true,
                                             only_preferred);
        spin_unlock_irqrestore(&id_map_lock, flags);
  ...

I suppose the end result is the same as your implementation, but it
trades making one existing function slightly more complicated instead of
adding some new ones.

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

[James Clark]

On 16/05/2024 04:56, Mao Jinlong wrote:
> Add trace-id for static id support to coresight dummy source.
> 
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>  .../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 6745b4cc8f1c..9adf34ea450e 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
>  
> +  trace-id:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      If dummy source needs static id support, use this to set trace id.
> +      The range is 1 to 127.
> +

The max is CORESIGHT_TRACE_ID_RES_TOP, so this would be 1 to 111 (inclusive)

>    out-ports:
>      $ref: /schemas/graph.yaml#/properties/ports
>  

Re: [PATCH v1 2/3] coresight: Add reserve trace id support

[James Clark]

On 16/05/2024 15:23, James Clark wrote:
> 
> 
> On 16/05/2024 04:56, 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. Reserve trace id
>> for this kind of hardware source.
>>
>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>> ---
>>  .../hwtracing/coresight/coresight-platform.c  | 26 +++++++++++++++++++
>>  .../hwtracing/coresight/coresight-trace-id.c  | 24 +++++++++++++++++
>>  .../hwtracing/coresight/coresight-trace-id.h  | 11 ++++++++
>>  include/linux/coresight.h                     |  1 +
>>  4 files changed, 62 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
>> index 9d550f5697fa..d3e22a2608df 100644
>> --- a/drivers/hwtracing/coresight/coresight-platform.c
>> +++ b/drivers/hwtracing/coresight/coresight-platform.c
>> @@ -183,6 +183,17 @@ static int of_coresight_get_cpu(struct device *dev)
>>  	return cpu;
>>  }
>>  
>> +/*
>> + * of_coresight_get_trace_id: Get the atid of a source device.
>> + *
>> + * Returns 0 on success.
>> + */
>> +static int of_coresight_get_trace_id(struct device *dev, u32 *id)
>> +{
>> +
>> +	return of_property_read_u32(dev->of_node, "trace-id", id);
>> +}
>> +
>>  /*
>>   * of_coresight_parse_endpoint : Parse the given output endpoint @ep
>>   * and fill the connection information in @pdata->out_conns
>> @@ -315,6 +326,12 @@ static inline int of_coresight_get_cpu(struct device *dev)
>>  {
>>  	return -ENODEV;
>>  }
>> +
>> +static int of_coresight_get_trace_id(struct device *dev, u32 *id)
>> +{
>> +	return -ENODEV;
>> +}
>> +
>>  #endif
>>  
>>  #ifdef CONFIG_ACPI
>> @@ -794,6 +811,15 @@ int coresight_get_cpu(struct device *dev)
>>  }
>>  EXPORT_SYMBOL_GPL(coresight_get_cpu);
>>  
>> +int coresight_get_trace_id(struct device *dev, u32 *id)
>> +{
>> +	if (!is_of_node(dev->fwnode))
>> +		return -EINVAL;
>> +
>> +	return of_coresight_get_trace_id(dev, id);

Can we somehow make this function name distinct from the trace ID
functions. It's a bit hard to read that it's called
coresight_get_trace_id() but it doesn't actually get an ID from the
existing trace ID stuff.

>> +}
>> +EXPORT_SYMBOL_GPL(coresight_get_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 af5b4ef59cea..536a34e9de6f 100644
>> --- a/drivers/hwtracing/coresight/coresight-trace-id.c
>> +++ b/drivers/hwtracing/coresight/coresight-trace-id.c
>> @@ -110,6 +110,24 @@ static int coresight_trace_id_alloc_new_id(struct coresight_trace_id_map *id_map
>>  	return id;
>>  }
>>  
>> +static int coresight_trace_id_set(int id, struct coresight_trace_id_map *id_map)
>> +{
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&id_map_lock, flags);
>> +
>> +	if (WARN(!IS_VALID_CS_TRACE_ID(id), "Invalid Trace ID %d\n", id))
>> +		return -EINVAL;
>> +	if (WARN(test_bit(id, id_map->used_ids), "ID is already used: %d\n", id))
>> +		return -EINVAL;
> 
> Do these returns not skip unlocking the spinlock?
> 
> It might be slightly fewer changes if we update the existing
> coresight_trace_id_alloc_new_id() to add a new "only_preferred" option.
> 
> Then use the existing system id allocator which already handles the lock
> and unlock properly:
> 
>   static int coresight_trace_id_map_get_system_id(struct
>                              coresight_trace_id_map *id_map, int id,
> 
>                              bool only_preferred)
>   {
>   ...
> 	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, id, true,
>                                              only_preferred);
>         spin_unlock_irqrestore(&id_map_lock, flags);
>   ...
> 
> I suppose the end result is the same as your implementation, but it
> trades making one existing function slightly more complicated instead of
> adding some new ones.

It's also not that obvious that there is the new reserve function, but
you still free the ID with the same coresight_trace_id_put_system_id().

Another benefit of adding arguments to the existing functions is that we
keep just ...get...() and ...put...(). 'Reserve' implies some other new
mechanism, but it's really a normal get. I think we should do one of
these two options for the top level API:

#1 (when id != 0, then it's an "only preferred" preferred ID:
  coresight_trace_id_get_system_id(int id)
  coresight_trace_id_put_system_id(int id)

#2
  coresight_trace_id_get_system_id()
  coresight_trace_id_get_system_id_resrv(int id)
  coresight_trace_id_put_system_id(int id)

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

[Krzysztof Kozlowski]

On 16/05/2024 04:56, Mao Jinlong wrote:
> Add trace-id for static id support to coresight dummy source.
> 
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>  .../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 6745b4cc8f1c..9adf34ea450e 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
>  
> +  trace-id:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      If dummy source needs static id support, use this to set trace id.
> +      The range is 1 to 127.

Missing constraints... and then no need to repeat constraints in free
form text.

Best regards,
Krzysztof

Re: [PATCH v1 2/3] coresight: Add reserve trace id support

[Jinlong Mao]

Hi James,

On 2024/5/16 21:23, James Clark wrote:
> 
> 
> On 16/05/2024 04:56, 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. Reserve trace id
>> for this kind of hardware source.
>>
>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>> ---
>>   .../hwtracing/coresight/coresight-platform.c  | 26 +++++++++++++++++++
>>   .../hwtracing/coresight/coresight-trace-id.c  | 24 +++++++++++++++++
>>   .../hwtracing/coresight/coresight-trace-id.h  | 11 ++++++++
>>   include/linux/coresight.h                     |  1 +
>>   4 files changed, 62 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
>> index 9d550f5697fa..d3e22a2608df 100644
>> --- a/drivers/hwtracing/coresight/coresight-platform.c
>> +++ b/drivers/hwtracing/coresight/coresight-platform.c
>> @@ -183,6 +183,17 @@ static int of_coresight_get_cpu(struct device *dev)
>>   	return cpu;
>>   }
>>   
>> +/*
>> + * of_coresight_get_trace_id: Get the atid of a source device.
>> + *
>> + * Returns 0 on success.
>> + */
>> +static int of_coresight_get_trace_id(struct device *dev, u32 *id)
>> +{
>> +
>> +	return of_property_read_u32(dev->of_node, "trace-id", id);
>> +}
>> +
>>   /*
>>    * of_coresight_parse_endpoint : Parse the given output endpoint @ep
>>    * and fill the connection information in @pdata->out_conns
>> @@ -315,6 +326,12 @@ static inline int of_coresight_get_cpu(struct device *dev)
>>   {
>>   	return -ENODEV;
>>   }
>> +
>> +static int of_coresight_get_trace_id(struct device *dev, u32 *id)
>> +{
>> +	return -ENODEV;
>> +}
>> +
>>   #endif
>>   
>>   #ifdef CONFIG_ACPI
>> @@ -794,6 +811,15 @@ int coresight_get_cpu(struct device *dev)
>>   }
>>   EXPORT_SYMBOL_GPL(coresight_get_cpu);
>>   
>> +int coresight_get_trace_id(struct device *dev, u32 *id)
>> +{
>> +	if (!is_of_node(dev->fwnode))
>> +		return -EINVAL;
>> +
>> +	return of_coresight_get_trace_id(dev, id);
>> +}
>> +EXPORT_SYMBOL_GPL(coresight_get_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 af5b4ef59cea..536a34e9de6f 100644
>> --- a/drivers/hwtracing/coresight/coresight-trace-id.c
>> +++ b/drivers/hwtracing/coresight/coresight-trace-id.c
>> @@ -110,6 +110,24 @@ static int coresight_trace_id_alloc_new_id(struct coresight_trace_id_map *id_map
>>   	return id;
>>   }
>>   
>> +static int coresight_trace_id_set(int id, struct coresight_trace_id_map *id_map)
>> +{
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&id_map_lock, flags);
>> +
>> +	if (WARN(!IS_VALID_CS_TRACE_ID(id), "Invalid Trace ID %d\n", id))
>> +		return -EINVAL;
>> +	if (WARN(test_bit(id, id_map->used_ids), "ID is already used: %d\n", id))
>> +		return -EINVAL;
> 
> Do these returns not skip unlocking the spinlock?

Yes. Missing the unlocking the spinlock here.

> 
> It might be slightly fewer changes if we update the existing
> coresight_trace_id_alloc_new_id() to add a new "only_preferred" option.
> 
> Then use the existing system id allocator which already handles the lock
> and unlock properly:
> 
>    static int coresight_trace_id_map_get_system_id(struct
>                               coresight_trace_id_map *id_map, int id,
> 
>                               bool only_preferred)
>    {
>    ...
> 	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, id, true,
>                                               only_preferred);
>          spin_unlock_irqrestore(&id_map_lock, flags);
>    ...
> 
> I suppose the end result is the same as your implementation, but it
> trades making one existing function slightly more complicated instead of
> adding some new ones.
yes. Your suggestion looks better. I will think carefully.