[PATCH v8 2/2] coresight: Add label sysfs node support

Mao Jinlong posted 2 patches 3 months ago
There is a newer version of this series
[PATCH v8 2/2] coresight: Add label sysfs node support
Posted by Mao Jinlong 3 months ago
For some coresight components like CTI and TPDM, there could be
numerous of them. From the node name, we can only get the type and
register address of the component. We can't identify the HW or the
system the component belongs to. Add label sysfs node support for
showing the intuitive name of the device.

Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
Reviewed-by: Mike Leach <mike.leach@linaro.org>
---
 .../testing/sysfs-bus-coresight-devices-cti   |  6 ++++
 .../sysfs-bus-coresight-devices-funnel        |  6 ++++
 .../testing/sysfs-bus-coresight-devices-tpdm  |  6 ++++
 drivers/hwtracing/coresight/coresight-sysfs.c | 32 +++++++++++++++++++
 4 files changed, 50 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-cti b/Documentation/ABI/testing/sysfs-bus-coresight-devices-cti
index a97b70f588da..55367bbc696f 100644
--- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-cti
+++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-cti
@@ -239,3 +239,9 @@ Date:		March 2020
 KernelVersion:	5.7
 Contact:	Mike Leach or Mathieu Poirier
 Description:	(Write) Clear all channel / trigger programming.
+
+What:           /sys/bus/coresight/devices/<cti-name>/label
+Date:           Jul 2025
+KernelVersion   6.17
+Contact:        Mao Jinlong <quic_jinlmao@quicinc.com>
+Description:    (Read) Show hardware context information of device.
diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-funnel b/Documentation/ABI/testing/sysfs-bus-coresight-devices-funnel
index d75acda5e1b3..5578fa5f6f02 100644
--- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-funnel
+++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-funnel
@@ -10,3 +10,9 @@ Date:		November 2014
 KernelVersion:	3.19
 Contact:	Mathieu Poirier <mathieu.poirier@linaro.org>
 Description:	(RW) Defines input port priority order.
+
+What:           /sys/bus/coresight/devices/<memory_map>.funnel/label
+Date:           Jul 2025
+KernelVersion   6.17
+Contact:        Mao Jinlong <quic_jinlmao@quicinc.com>
+Description:    (Read) Show hardware context information of device.
diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
index a341b08ae70b..e6d935e83042 100644
--- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
+++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
@@ -272,3 +272,9 @@ KernelVersion	6.15
 Contact:	Jinlong Mao (QUIC) <quic_jinlmao@quicinc.com>, Tao Zhang (QUIC) <quic_taozha@quicinc.com>
 Description:
 		(RW) Set/Get the enablement of the individual lane.
+
+What:           /sys/bus/coresight/devices/<tpdm-name>/label
+Date:           Jul 2025
+KernelVersion   6.17
+Contact:        Mao Jinlong <quic_jinlmao@quicinc.com>
+Description:    (Read) Show hardware context information of device.
diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c
index feadaf065b53..e3d21c49814d 100644
--- a/drivers/hwtracing/coresight/coresight-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-sysfs.c
@@ -7,6 +7,7 @@
 #include <linux/device.h>
 #include <linux/idr.h>
 #include <linux/kernel.h>
+#include <linux/property.h>
 
 #include "coresight-priv.h"
 #include "coresight-trace-id.h"
@@ -371,18 +372,47 @@ static ssize_t enable_source_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(enable_source);
 
+static ssize_t label_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+
+	const char *str;
+	int ret = 0;
+
+	ret = fwnode_property_read_string(dev_fwnode(dev), "label", &str);
+	if (ret == 0)
+		return scnprintf(buf, PAGE_SIZE, "%s\n", str);
+	else
+		return ret;
+}
+static DEVICE_ATTR_RO(label);
+
 static struct attribute *coresight_sink_attrs[] = {
 	&dev_attr_enable_sink.attr,
+	&dev_attr_label.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(coresight_sink);
 
 static struct attribute *coresight_source_attrs[] = {
 	&dev_attr_enable_source.attr,
+	&dev_attr_label.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(coresight_source);
 
+static struct attribute *coresight_link_attrs[] = {
+	&dev_attr_label.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(coresight_link);
+
+static struct attribute *coresight_helper_attrs[] = {
+	&dev_attr_label.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(coresight_helper);
+
 const struct device_type coresight_dev_type[] = {
 	[CORESIGHT_DEV_TYPE_SINK] = {
 		.name = "sink",
@@ -390,6 +420,7 @@ const struct device_type coresight_dev_type[] = {
 	},
 	[CORESIGHT_DEV_TYPE_LINK] = {
 		.name = "link",
+		.groups = coresight_link_groups,
 	},
 	[CORESIGHT_DEV_TYPE_LINKSINK] = {
 		.name = "linksink",
@@ -401,6 +432,7 @@ const struct device_type coresight_dev_type[] = {
 	},
 	[CORESIGHT_DEV_TYPE_HELPER] = {
 		.name = "helper",
+		.groups = coresight_helper_groups,
 	}
 };
 /* Ensure the enum matches the names and groups */
-- 
2.17.1
Re: [PATCH v8 2/2] coresight: Add label sysfs node support
Posted by Leo Yan 3 months ago
On Thu, Jul 03, 2025 at 09:04:53PM +0800, Mao Jinlong wrote:

[...]

> +static ssize_t label_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +
> +	const char *str;
> +	int ret = 0;

No need to init ret to 0.

> +	ret = fwnode_property_read_string(dev_fwnode(dev), "label", &str);
> +	if (ret == 0)
> +		return scnprintf(buf, PAGE_SIZE, "%s\n", str);
> +	else
> +		return ret;
> +}
> +static DEVICE_ATTR_RO(label);
> +
>  static struct attribute *coresight_sink_attrs[] = {
>  	&dev_attr_enable_sink.attr,
> +	&dev_attr_label.attr,
>  	NULL,
>  };
>  ATTRIBUTE_GROUPS(coresight_sink);
>  
>  static struct attribute *coresight_source_attrs[] = {
>  	&dev_attr_enable_source.attr,
> +	&dev_attr_label.attr,
>  	NULL,
>  };
>  ATTRIBUTE_GROUPS(coresight_source);
>  
> +static struct attribute *coresight_link_attrs[] = {
> +	&dev_attr_label.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(coresight_link);
> +
> +static struct attribute *coresight_helper_attrs[] = {
> +	&dev_attr_label.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(coresight_helper);
> +

This change adds a 'label' entry for source, link, helper, and sink
components, but the documentation has only updated for three components:
CTI, funnel, and TPDM.

Should we also update the documentation for all relevant components,
such as ETM, ETR, etc.?

Additionally, patch 01 is missing the update to the ETM yaml file for
the new property. I checked patch v4 [1], which includes a change to
etm.yaml, but this change was dropped since v5. I briefly read the
v4 discussion thread and didn't see any mention of removing the ETM
related change. Did you see any particular issue when add label for
ETM devices?

Overall, this series is fine for me. Just please ensure that all
relevant components are covered for completeness.

Thanks,
Leo

[1] https://patchwork.kernel.org/project/linux-arm-msm/cover/20240703122340.26864-1-quic_jinlmao@quicinc.com/

>  const struct device_type coresight_dev_type[] = {
>  	[CORESIGHT_DEV_TYPE_SINK] = {
>  		.name = "sink",
> @@ -390,6 +420,7 @@ const struct device_type coresight_dev_type[] = {
>  	},
>  	[CORESIGHT_DEV_TYPE_LINK] = {
>  		.name = "link",
> +		.groups = coresight_link_groups,
>  	},
>  	[CORESIGHT_DEV_TYPE_LINKSINK] = {
>  		.name = "linksink",
> @@ -401,6 +432,7 @@ const struct device_type coresight_dev_type[] = {
>  	},
>  	[CORESIGHT_DEV_TYPE_HELPER] = {
>  		.name = "helper",
> +		.groups = coresight_helper_groups,
>  	}
>  };
>  /* Ensure the enum matches the names and groups */
> -- 
> 2.17.1
> 
> _______________________________________________
> CoreSight mailing list -- coresight@lists.linaro.org
> To unsubscribe send an email to coresight-leave@lists.linaro.org
Re: [PATCH v8 2/2] coresight: Add label sysfs node support
Posted by Jinlong Mao 2 months, 3 weeks ago

On 2025/7/3 22:19, Leo Yan wrote:
> On Thu, Jul 03, 2025 at 09:04:53PM +0800, Mao Jinlong wrote:
> 
> [...]
> 
>> +static ssize_t label_show(struct device *dev,
>> +		struct device_attribute *attr, char *buf)
>> +{
>> +
>> +	const char *str;
>> +	int ret = 0;
> 
> No need to init ret to 0.
> 
>> +	ret = fwnode_property_read_string(dev_fwnode(dev), "label", &str);
>> +	if (ret == 0)
>> +		return scnprintf(buf, PAGE_SIZE, "%s\n", str);
>> +	else
>> +		return ret;
>> +}
>> +static DEVICE_ATTR_RO(label);
>> +
>>   static struct attribute *coresight_sink_attrs[] = {
>>   	&dev_attr_enable_sink.attr,
>> +	&dev_attr_label.attr,
>>   	NULL,
>>   };
>>   ATTRIBUTE_GROUPS(coresight_sink);
>>   
>>   static struct attribute *coresight_source_attrs[] = {
>>   	&dev_attr_enable_source.attr,
>> +	&dev_attr_label.attr,
>>   	NULL,
>>   };
>>   ATTRIBUTE_GROUPS(coresight_source);
>>   
>> +static struct attribute *coresight_link_attrs[] = {
>> +	&dev_attr_label.attr,
>> +	NULL,
>> +};
>> +ATTRIBUTE_GROUPS(coresight_link);
>> +
>> +static struct attribute *coresight_helper_attrs[] = {
>> +	&dev_attr_label.attr,
>> +	NULL,
>> +};
>> +ATTRIBUTE_GROUPS(coresight_helper);
>> +
> 
> This change adds a 'label' entry for source, link, helper, and sink
> components, but the documentation has only updated for three components:
> CTI, funnel, and TPDM.
> 
> Should we also update the documentation for all relevant components,
> such as ETM, ETR, etc.?
> 
> Additionally, patch 01 is missing the update to the ETM yaml file for
> the new property. I checked patch v4 [1], which includes a change to
> etm.yaml, but this change was dropped since v5. I briefly read the
> v4 discussion thread and didn't see any mention of removing the ETM
> related change. Did you see any particular issue when add label for
> ETM devices?
> 
> Overall, this series is fine for me. Just please ensure that all
> relevant components are covered for completeness.
> 
> Thanks,
> Leo
> 

I will update all coresight docs.

Thanks
Jinlong Mao

> [1] https://patchwork.kernel.org/project/linux-arm-msm/cover/20240703122340.26864-1-quic_jinlmao@quicinc.com/
> 
>>   const struct device_type coresight_dev_type[] = {
>>   	[CORESIGHT_DEV_TYPE_SINK] = {
>>   		.name = "sink",
>> @@ -390,6 +420,7 @@ const struct device_type coresight_dev_type[] = {
>>   	},
>>   	[CORESIGHT_DEV_TYPE_LINK] = {
>>   		.name = "link",
>> +		.groups = coresight_link_groups,
>>   	},
>>   	[CORESIGHT_DEV_TYPE_LINKSINK] = {
>>   		.name = "linksink",
>> @@ -401,6 +432,7 @@ const struct device_type coresight_dev_type[] = {
>>   	},
>>   	[CORESIGHT_DEV_TYPE_HELPER] = {
>>   		.name = "helper",
>> +		.groups = coresight_helper_groups,
>>   	}
>>   };
>>   /* Ensure the enum matches the names and groups */
>> -- 
>> 2.17.1
>>
>> _______________________________________________
>> CoreSight mailing list -- coresight@lists.linaro.org
>> To unsubscribe send an email to coresight-leave@lists.linaro.org
Re: [PATCH v8 2/2] coresight: Add label sysfs node support
Posted by Mike Leach 2 months, 3 weeks ago
Hi,

On Wed, 16 Jul 2025 at 03:43, Jinlong Mao <quic_jinlmao@quicinc.com> wrote:
>
>
>
> On 2025/7/3 22:19, Leo Yan wrote:
> > On Thu, Jul 03, 2025 at 09:04:53PM +0800, Mao Jinlong wrote:
> >
> > [...]
> >
> >> +static ssize_t label_show(struct device *dev,
> >> +            struct device_attribute *attr, char *buf)
> >> +{
> >> +
> >> +    const char *str;
> >> +    int ret = 0;
> >
> > No need to init ret to 0.
> >
> >> +    ret = fwnode_property_read_string(dev_fwnode(dev), "label", &str);
> >> +    if (ret == 0)
> >> +            return scnprintf(buf, PAGE_SIZE, "%s\n", str);
> >> +    else
> >> +            return ret;
> >> +}
> >> +static DEVICE_ATTR_RO(label);
> >> +
> >>   static struct attribute *coresight_sink_attrs[] = {
> >>      &dev_attr_enable_sink.attr,
> >> +    &dev_attr_label.attr,
> >>      NULL,
> >>   };
> >>   ATTRIBUTE_GROUPS(coresight_sink);
> >>
> >>   static struct attribute *coresight_source_attrs[] = {
> >>      &dev_attr_enable_source.attr,
> >> +    &dev_attr_label.attr,
> >>      NULL,
> >>   };
> >>   ATTRIBUTE_GROUPS(coresight_source);
> >>
> >> +static struct attribute *coresight_link_attrs[] = {
> >> +    &dev_attr_label.attr,
> >> +    NULL,
> >> +};
> >> +ATTRIBUTE_GROUPS(coresight_link);
> >> +
> >> +static struct attribute *coresight_helper_attrs[] = {
> >> +    &dev_attr_label.attr,
> >> +    NULL,
> >> +};
> >> +ATTRIBUTE_GROUPS(coresight_helper);
> >> +
> >
> > This change adds a 'label' entry for source, link, helper, and sink
> > components, but the documentation has only updated for three components:
> > CTI, funnel, and TPDM.
> >
> > Should we also update the documentation for all relevant components,
> > such as ETM, ETR, etc.?
> >
> > Additionally, patch 01 is missing the update to the ETM yaml file for
> > the new property. I checked patch v4 [1], which includes a change to
> > etm.yaml, but this change was dropped since v5. I briefly read the
> > v4 discussion thread and didn't see any mention of removing the ETM
> > related change. Did you see any particular issue when add label for
> > ETM devices?
> >
> > Overall, this series is fine for me. Just please ensure that all
> > relevant components are covered for completeness.
> >
> > Thanks,
> > Leo
> >
>
> I will update all coresight docs.
>
> Thanks
> Jinlong Mao
>
> > [1] https://patchwork.kernel.org/project/linux-arm-msm/cover/20240703122340.26864-1-quic_jinlmao@quicinc.com/
> >
> >>   const struct device_type coresight_dev_type[] = {
> >>      [CORESIGHT_DEV_TYPE_SINK] = {
> >>              .name = "sink",
> >> @@ -390,6 +420,7 @@ const struct device_type coresight_dev_type[] = {
> >>      },
> >>      [CORESIGHT_DEV_TYPE_LINK] = {
> >>              .name = "link",
> >> +            .groups = coresight_link_groups,
> >>      },
> >>      [CORESIGHT_DEV_TYPE_LINKSINK] = {
> >>              .name = "linksink",
> >> @@ -401,6 +432,7 @@ const struct device_type coresight_dev_type[] = {
> >>      },
> >>      [CORESIGHT_DEV_TYPE_HELPER] = {
> >>              .name = "helper",
> >> +            .groups = coresight_helper_groups,
> >>      }
> >>   };
> >>   /* Ensure the enum matches the names and groups */
> >> --
> >> 2.17.1
> >>
> >> _______________________________________________
> >> CoreSight mailing list -- coresight@lists.linaro.org
> >> To unsubscribe send an email to coresight-leave@lists.linaro.org
>

Revisiting this - the label DT attribute is purely optional, and
provides context for the hardware instance.
This code as written appears to add a "label" file to all devices,
irrespective of if the label is set in the DT.or not, with blank
labels where  the attribute is not present.
The visibility of the sysfs attribute should be controlled so that it
only appears if label is present in the DT.

Mike

-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
Re: [PATCH v8 2/2] coresight: Add label sysfs node support
Posted by Jinlong Mao 2 months, 3 weeks ago

On 2025/7/16 18:45, Mike Leach wrote:
> Hi,
> 
> On Wed, 16 Jul 2025 at 03:43, Jinlong Mao <quic_jinlmao@quicinc.com> wrote:
>>
>>
>>
>> On 2025/7/3 22:19, Leo Yan wrote:
>>> On Thu, Jul 03, 2025 at 09:04:53PM +0800, Mao Jinlong wrote:
>>>
>>> [...]
>>>
>>>> +static ssize_t label_show(struct device *dev,
>>>> +            struct device_attribute *attr, char *buf)
>>>> +{
>>>> +
>>>> +    const char *str;
>>>> +    int ret = 0;
>>>
>>> No need to init ret to 0.
>>>
>>>> +    ret = fwnode_property_read_string(dev_fwnode(dev), "label", &str);
>>>> +    if (ret == 0)
>>>> +            return scnprintf(buf, PAGE_SIZE, "%s\n", str);
>>>> +    else
>>>> +            return ret;
>>>> +}
>>>> +static DEVICE_ATTR_RO(label);
>>>> +
>>>>    static struct attribute *coresight_sink_attrs[] = {
>>>>       &dev_attr_enable_sink.attr,
>>>> +    &dev_attr_label.attr,
>>>>       NULL,
>>>>    };
>>>>    ATTRIBUTE_GROUPS(coresight_sink);
>>>>
>>>>    static struct attribute *coresight_source_attrs[] = {
>>>>       &dev_attr_enable_source.attr,
>>>> +    &dev_attr_label.attr,
>>>>       NULL,
>>>>    };
>>>>    ATTRIBUTE_GROUPS(coresight_source);
>>>>
>>>> +static struct attribute *coresight_link_attrs[] = {
>>>> +    &dev_attr_label.attr,
>>>> +    NULL,
>>>> +};
>>>> +ATTRIBUTE_GROUPS(coresight_link);
>>>> +
>>>> +static struct attribute *coresight_helper_attrs[] = {
>>>> +    &dev_attr_label.attr,
>>>> +    NULL,
>>>> +};
>>>> +ATTRIBUTE_GROUPS(coresight_helper);
>>>> +
>>>
>>> This change adds a 'label' entry for source, link, helper, and sink
>>> components, but the documentation has only updated for three components:
>>> CTI, funnel, and TPDM.
>>>
>>> Should we also update the documentation for all relevant components,
>>> such as ETM, ETR, etc.?
>>>
>>> Additionally, patch 01 is missing the update to the ETM yaml file for
>>> the new property. I checked patch v4 [1], which includes a change to
>>> etm.yaml, but this change was dropped since v5. I briefly read the
>>> v4 discussion thread and didn't see any mention of removing the ETM
>>> related change. Did you see any particular issue when add label for
>>> ETM devices?
>>>
>>> Overall, this series is fine for me. Just please ensure that all
>>> relevant components are covered for completeness.
>>>
>>> Thanks,
>>> Leo
>>>
>>
>> I will update all coresight docs.
>>
>> Thanks
>> Jinlong Mao
>>
>>> [1] https://patchwork.kernel.org/project/linux-arm-msm/cover/20240703122340.26864-1-quic_jinlmao@quicinc.com/
>>>
>>>>    const struct device_type coresight_dev_type[] = {
>>>>       [CORESIGHT_DEV_TYPE_SINK] = {
>>>>               .name = "sink",
>>>> @@ -390,6 +420,7 @@ const struct device_type coresight_dev_type[] = {
>>>>       },
>>>>       [CORESIGHT_DEV_TYPE_LINK] = {
>>>>               .name = "link",
>>>> +            .groups = coresight_link_groups,
>>>>       },
>>>>       [CORESIGHT_DEV_TYPE_LINKSINK] = {
>>>>               .name = "linksink",
>>>> @@ -401,6 +432,7 @@ const struct device_type coresight_dev_type[] = {
>>>>       },
>>>>       [CORESIGHT_DEV_TYPE_HELPER] = {
>>>>               .name = "helper",
>>>> +            .groups = coresight_helper_groups,
>>>>       }
>>>>    };
>>>>    /* Ensure the enum matches the names and groups */
>>>> --
>>>> 2.17.1
>>>>
>>>> _______________________________________________
>>>> CoreSight mailing list -- coresight@lists.linaro.org
>>>> To unsubscribe send an email to coresight-leave@lists.linaro.org
>>
> 
> Revisiting this - the label DT attribute is purely optional, and
> provides context for the hardware instance.
> This code as written appears to add a "label" file to all devices,
> irrespective of if the label is set in the DT.or not, with blank
> labels where  the attribute is not present.
> The visibility of the sysfs attribute should be controlled so that it
> only appears if label is present in the DT.
> 
I will follow this.

Thanks
Jinlong Mao
> Mike
>