Received-SPF: pass (google.com: domain of linux-kernel+bounces-14322-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20231228093321.5522-1-quic_jinlmao@quicinc.com> <12ce6e5d-6e4d-fb99-eb82-dece97423bfb@arm.com>
In-Reply-To: <12ce6e5d-6e4d-fb99-eb82-dece97423bfb@arm.com>
From: Mike Leach <mike.leach@linaro.org>
Date: Tue, 2 Jan 2024 12:04:29 +0000
Message-ID: <CAJ9a7Vgi=ELOhXNF97KrBtV5ef8khwWqzWKevrYW2RtBBtsppw@mail.gmail.com>
Subject: Re: [PATCH] coresight: Add coresight name support
To: James Clark <james.clark@arm.com>
Cc: Mao Jinlong <quic_jinlmao@quicinc.com>, Suzuki K Poulose <suzuki.poulose@arm.com>, 
	coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, 
	linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, 
	Tingwei Zhang <quic_tingweiz@quicinc.com>, Yuanfang Zhang <quic_yuanfang@quicinc.com>, 
	Tao Zhang <quic_taozha@quicinc.com>, Leo Yan <leo.yan@linaro.org>, 
	Alexander Shishkin <alexander.shishkin@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"

As James mentions this is clearly a V2 of a previous patch - please
mark as such in future.

Adding to what James has already said:-

1) Mapping between the canonical names used in the drivers and the
information as to the precise device is as easy as running 'ls' on
/sys/bus/coresight/devices:-

root@linaro-developer:/home/linaro/cs-mods# ls -al /sys/bus/coresight/devices/
total 0
drwxr-xr-x 2 root root 0 Jan  2 11:27 .
drwxr-xr-x 4 root root 0 Jan  2 11:27 ..
lrwxrwxrwx 1 root root 0 Jan  2 11:27 cti_cpu0 ->
../../../devices/platform/soc@0/858000.cti/cti_cpu0
lrwxrwxrwx 1 root root 0 Jan  2 11:27 cti_cpu1 ->
../../../devices/platform/soc@0/859000.cti/cti_cpu1
lrwxrwxrwx 1 root root 0 Jan  2 11:27 cti_cpu2 ->
../../../devices/platform/soc@0/85a000.cti/cti_cpu2
lrwxrwxrwx 1 root root 0 Jan  2 11:27 cti_cpu3 ->
../../../devices/platform/soc@0/85b000.cti/cti_cpu3
lrwxrwxrwx 1 root root 0 Jan  2 11:27 cti_sys0 ->
../../../devices/platform/soc@0/810000.cti/cti_sys0
lrwxrwxrwx 1 root root 0 Jan  2 11:27 cti_sys1 ->
../../../devices/platform/soc@0/811000.cti/cti_sys1
lrwxrwxrwx 1 root root 0 Jan  2 11:27 etm0 ->
../../../devices/platform/soc@0/85c000.etm/etm0
lrwxrwxrwx 1 root root 0 Jan  2 11:27 etm1 ->
../../../devices/platform/soc@0/85d000.etm/etm1
lrwxrwxrwx 1 root root 0 Jan  2 11:27 etm2 ->
../../../devices/platform/soc@0/85e000.etm/etm2
lrwxrwxrwx 1 root root 0 Jan  2 11:27 etm3 ->
../../../devices/platform/soc@0/85f000.etm/etm3
lrwxrwxrwx 1 root root 0 Jan  2 11:42 funnel0 ->
../../../devices/platform/soc@0/821000.funnel/funnel0
lrwxrwxrwx 1 root root 0 Jan  2 11:42 funnel1 ->
../../../devices/platform/soc@0/841000.funnel/funnel1
lrwxrwxrwx 1 root root 0 Jan  2 11:42 replicator0 ->
../../../devices/platform/soc@0/824000.replicator/replicator0
lrwxrwxrwx 1 root root 0 Jan  2 11:42 tmc_etf0 ->
../../../devices/platform/soc@0/825000.etf/tmc_etf0
lrwxrwxrwx 1 root root 0 Jan  2 11:42 tmc_etr0 ->
../../../devices/platform/soc@0/826000.etr/tmc_etr0


2) The patch set must contain the usage and specification in the .yaml
 file(s) of the property used.

However, there was a standard property called 'name' which is
deprecated - see
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html
section 2.3.11. I do not believe that adding another 'name' property
would be accepted by the DT maintainers.

3) the 'device_node' structure has a 'name' field that contains the
node name in the DT approved "node-name@unit-address" format.This
contains whatever node names you used in the dt.  Why not use this if
a change has to be made and find some conditional to activate it.

However, given point 1) above, the problem is solved and the patch
adds no new information not already available.

Regards

Mike

On Thu, 28 Dec 2023 at 11:26, James Clark <james.clark@arm.com> wrote:
>
>
>
> On 28/12/2023 09:33, Mao Jinlong wrote:
> > Add coresight name support for custom names which will be
> > easy to identify the device by the name.
> >
>
> I suppose this is more of a V2 because the subject is the same as the
> one sent earlier this year. But it looks like the discussion on the
> previous one wasn't resolved.
>
> With the main issues to solve being:
>
>  * It would be nice to use the existing root node name instead of adding
>    a new property. But at the same time DT nodes are supposed to have
>    generic names.
>
>  * This only works for DT and not ACPI
>
> To me it seems like adding the new property is just a "cheat" to get
> around not being allowed to have a specific name for the root node. But
> if we admit that we need a name I don't see the benefit of not putting
> the name where the node is already named.
>
> Using the root node name at this point would also undo the hard coded
> per-cpu naming of the CTI and ETM devices, so maybe it would be nice,
> but it's just too late. That means that a new field is necessary.
> Although that field could be a boolean like "use-root-name-for-display"
> or something like that. In the end it probably doesn't really make a
> difference whether it's that or a name string.
>
> And maybe the answer to the ACPI question is just that if anyone needs
> it, they can add it in the future. It doesn't seem like it would
> conflict with anything we do here.
>
> > Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> > ---
> >  .../hwtracing/coresight/coresight-cti-core.c  | 20 ++++++++------
> >  drivers/hwtracing/coresight/coresight-dummy.c | 10 ++++---
> >  .../hwtracing/coresight/coresight-platform.c  | 27 +++++++++++++++++++
> >  drivers/hwtracing/coresight/coresight-tpdm.c  | 10 ++++---
> >  include/linux/coresight.h                     |  1 +
> >  5 files changed, 53 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c
> > index 3999d0a2cb60..60a1e76064a9 100644
> > --- a/drivers/hwtracing/coresight/coresight-cti-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-cti-core.c
> > @@ -902,14 +902,18 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id)
> >       /* default to powered - could change on PM notifications */
> >       drvdata->config.hw_powered = true;
> >
> > -     /* set up device name - will depend if cpu bound or otherwise */
> > -     if (drvdata->ctidev.cpu >= 0)
> > -             cti_desc.name = devm_kasprintf(dev, GFP_KERNEL, "cti_cpu%d",
> > -                                            drvdata->ctidev.cpu);
> > -     else
> > -             cti_desc.name = coresight_alloc_device_name(&cti_sys_devs, dev);
>
> Can we put the new name stuff inside coresight_alloc_device_name()? Then
> it happens by default for every device.
>
> I know Suzuki said previously to do it per-device, but the new DT
> property is just "coresight-name", so it's generic. Rather than being
> specific like "cti-name". So I don't see the benefit of duplicating the
> code at this point if we do decide to do it.
>
> > -     if (!cti_desc.name)
> > -             return -ENOMEM;
> > +     cti_desc.name = coresight_get_device_name(dev);
> > +     if (!cti_desc.name) {
> > +             /* set up device name - will depend if cpu bound or otherwise */
> > +             if (drvdata->ctidev.cpu >= 0)
> > +                     cti_desc.name = devm_kasprintf(dev, GFP_KERNEL, "cti_cpu%d",
> > +                                                    drvdata->ctidev.cpu);
> > +             else {
> > +                     cti_desc.name = coresight_alloc_device_name(&cti_sys_devs, dev);
> > +                     if (!cti_desc.name)
> > +                             return -ENOMEM;
> > +             }
> > +     }
>
> >
> >       /* setup CPU power management handling for CPU bound CTI devices. */
> >       ret = cti_pm_setup(drvdata);
> > diff --git a/drivers/hwtracing/coresight/coresight-dummy.c b/drivers/hwtracing/coresight/coresight-dummy.c
> > index e4deafae7bc2..b19cd400df79 100644
> > --- a/drivers/hwtracing/coresight/coresight-dummy.c
> > +++ b/drivers/hwtracing/coresight/coresight-dummy.c
> > @@ -76,10 +76,12 @@ static int dummy_probe(struct platform_device *pdev)
> >       struct coresight_desc desc = { 0 };
> >
> >       if (of_device_is_compatible(node, "arm,coresight-dummy-source")) {
> > -
> > -             desc.name = coresight_alloc_device_name(&source_devs, dev);
> > -             if (!desc.name)
> > -                     return -ENOMEM;
> > +             desc.name = coresight_get_device_name(dev);
> > +             if (!desc.name) {
> > +                     desc.name = coresight_alloc_device_name(&source_devs, dev);
> > +                     if (!desc.name)
> > +                             return -ENOMEM;
> > +             }
> >
> >               desc.type = CORESIGHT_DEV_TYPE_SOURCE;
> >               desc.subtype.source_subtype =
> > diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
> > index 9d550f5697fa..284aa22a06b7 100644
> > --- a/drivers/hwtracing/coresight/coresight-platform.c
> > +++ b/drivers/hwtracing/coresight/coresight-platform.c
> > @@ -183,6 +183,18 @@ static int of_coresight_get_cpu(struct device *dev)
> >       return cpu;
> >  }
> >
> > +static const char *of_coresight_get_device_name(struct device *dev)
> > +{
> > +     const char *name = NULL;
> > +
> > +     if (!dev->of_node)
> > +             return NULL;
> > +
> > +     of_property_read_string(dev->of_node, "coresight-name", &name);
>
> Do you need to update the binding docs with this new property?
>
> Also a minor nit: Maybe "display-name" is better? "Coresight" is
> implied, and the node is already named, although that node name isn't
> used for display purposes, but this one is.
>
> Thanks
> James


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