Hi,
On Wed, 18 Dec 2024 at 18:16, Suzuki K Poulose <suzuki.poulose@xxxxxxx> wrote:
>
> Hi Mike
>
> On 18/12/2024 09:56, Mike Leach wrote:
> > Hi
> >
> >> -----Original Message-----
> >> From: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> >> Sent: Wednesday, December 18, 2024 9:38 AM
> >> To: Mao Jinlong <quic_jinlmao@xxxxxxxxxxx>; Mike Leach
> >> <mike.leach@xxxxxxxxxx>; James Clark <James.Clark@xxxxxxx>; Rob Herring
> >> <robh@xxxxxxxxxx>; Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley
> >> <conor+dt@xxxxxxxxxx>; Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>; Bjorn
> >> Andersson <andersson@xxxxxxxxxx>; Konrad Dybcio
> >> <konradybcio@xxxxxxxxxx>
> >> Cc: coresight@xxxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> >> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-arm-
> >> msm@xxxxxxxxxxxxxxx
> >> Subject: Re: [PATCH v6 2/2] coresight: Add label sysfs node support
> >>
> >> On 17/12/2024 06:33, Mao Jinlong wrote:
> >>> 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@xxxxxxxxxxx>
> >>> ---
> >>> .../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(+)
> >>
> >> Do you think we need to name the devices using the label ?
> >>
> >
> > No - absolutely not. If we use label to name devices then we have to validate that the string is a correctly formed device name and that it has not been previously used.
>
> Anything that doesn't contain '/' can be a device name ? And it is very
> easy to find if the device name has been used in the coresight bus,
> after all these devices only appear there.
>
> It is as good as :
>
> bus_find_device_by_name(coresight_bus_type, NULL, name) == NULL
>
> Of course with coresight_mutex() held.
>
> Suzuki
>
DTS label property (DT spec 4.1.2) is a freeform string, specified to
be a human readable description of the device, e.g. :-
cti0@0x1000 {
reg = <0x1000>;
label = "main system CTI";
};
Arguably the label is completely unnecessary - as the @0x1000 tells
the knowledgeable user, with a hardware specification of the device
precisely what this CTI does and is related to.
The point of this patchset is to add context to the name@addr to make
the identification of the devices easier.
The DT compiler should ensure that the device tree is well formed.
Using driver selected names (cti_cpu0 ... etc) guarantees that every
device found in the DT has a unique representation in sysfs.
Once a freeform string is used, then not only are duplicates possible,
illegal device names are possible, all of which can result in missing
nodes or worse. This requires handling / complications that are
unnecessary for the purpose.
Yes of course it is easy to look for duplicate names, reject bad ones,
emit errors - but that could end up with a partially working system
with missing components.
Why add potential for breakage when it is not necessary?
Regards
Mike
>
> >
> > Using the canonical driver selected names works best as we are guaranteed a unique name and the information label can be used to provide flexible context information that best matches the users requirements.
> >
> > Mike
> >
> >> Or is this enough ?
> >
> >> Suzuki
> >>
> >>
> >>>
> >>> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-cti
> >>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-cti
> >>> index bf2869c413e7..909670e0451a 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: Dec 2024
> >>> +KernelVersion 6.14
> >>> +Contact: Mao Jinlong <quic_jinlmao@xxxxxxxxxxx>
> >>> +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..944aad879aeb 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@xxxxxxxxxx>
> >>> Description: (RW) Defines input port priority order.
> >>> +
> >>> +What: /sys/bus/coresight/devices/<memory_map>.funnel/label
> >>> +Date: Dec 2024
> >>> +KernelVersion 6.14
> >>> +Contact: Mao Jinlong <quic_jinlmao@xxxxxxxxxxx>
> >>> +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 bf710ea6e0ef..309802246398 100644
> >>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> >>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
> >>> @@ -257,3 +257,9 @@ Contact: Jinlong Mao (QUIC)
> >> <quic_jinlmao@xxxxxxxxxxx>, Tao Zhang (QUIC) <quic_t
> >>> Description:
> >>> (RW) Set/Get the MSR(mux select register) for the CMB
> >> subunit
> >>> TPDM.
> >>> +
> >>> +What: /sys/bus/coresight/devices/<tpdm-name>/label
> >>> +Date: Dec 2024
> >>> +KernelVersion 6.14
> >>> +Contact: Mao Jinlong <quic_jinlmao@xxxxxxxxxxx>
> >>> +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 a01c9e54e2ed..4af40cd7d75a 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"
> >>> @@ -366,18 +367,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",
> >>> @@ -385,6 +415,7 @@ const struct device_type coresight_dev_type[] = {
> >>> },
> >>> [CORESIGHT_DEV_TYPE_LINK] = {
> >>> .name = "link",
> >>> + .groups = coresight_link_groups,
> >>> },
> >>> [CORESIGHT_DEV_TYPE_LINKSINK] = {
> >>> .name = "linksink",
> >>> @@ -396,6 +427,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 */
> >>
> >> _______________________________________________
> >> CoreSight mailing list -- coresight@xxxxxxxxxxxxxxxx To unsubscribe send an
> >> email to coresight-leave@xxxxxxxxxxxxxxxx
>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
