[PATCH v6 2/2] coresight: Add label sysfs node support
Mike Leach
mike.leach at linaro.org
Thu Dec 19 03:17:57 PST 2024
Hi,
On Wed, 18 Dec 2024 at 18:16, Suzuki K Poulose <suzuki.poulose at arm.com> wrote:
>
> Hi Mike
>
> On 18/12/2024 09:56, Mike Leach wrote:
> > Hi
> >
> >> -----Original Message-----
> >> From: Suzuki K Poulose <suzuki.poulose at arm.com>
> >> Sent: Wednesday, December 18, 2024 9:38 AM
> >> To: Mao Jinlong <quic_jinlmao at quicinc.com>; Mike Leach
> >> <mike.leach at linaro.org>; James Clark <James.Clark at arm.com>; Rob Herring
> >> <robh at kernel.org>; Krzysztof Kozlowski <krzk+dt at kernel.org>; Conor Dooley
> >> <conor+dt at kernel.org>; Mathieu Poirier <mathieu.poirier at linaro.org>; Bjorn
> >> Andersson <andersson at kernel.org>; Konrad Dybcio
> >> <konradybcio at kernel.org>
> >> Cc: coresight at lists.linaro.org; linux-arm-kernel at lists.infradead.org;
> >> devicetree at vger.kernel.org; linux-kernel at vger.kernel.org; linux-arm-
> >> msm at vger.kernel.org
> >> 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 at quicinc.com>
> >>> ---
> >>> .../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 at 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 at 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 at 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..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 at linaro.org>
> >>> 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 at 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 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 at quicinc.com>, 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 at 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 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 at lists.linaro.org To unsubscribe send an
> >> email to coresight-leave at lists.linaro.org
>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
More information about the linux-arm-kernel
mailing list