Add coresight name support for custom names which will be easy to identify the device by the name.
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); - 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); + + return name; +} + /* * of_coresight_parse_endpoint : Parse the given output endpoint @ep * and fill the connection information in @pdata->out_conns @@ -315,6 +327,12 @@ static inline int of_coresight_get_cpu(struct device *dev) { return -ENODEV; } + +static inline const char *of_coresight_get_device_name(struct device *dev) +{ + return NULL; +} + #endif
#ifdef CONFIG_ACPI @@ -794,6 +812,15 @@ int coresight_get_cpu(struct device *dev) } EXPORT_SYMBOL_GPL(coresight_get_cpu);
+const char *coresight_get_device_name(struct device *dev) +{ + if (is_of_node(dev->fwnode)) + return of_coresight_get_device_name(dev); + else + return NULL; +} +EXPORT_SYMBOL_GPL(coresight_get_device_name); + struct coresight_platform_data * coresight_get_platform_data(struct device *dev) { diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c index f4854af0431e..7735ff18c48e 100644 --- a/drivers/hwtracing/coresight/coresight-tpdm.c +++ b/drivers/hwtracing/coresight/coresight-tpdm.c @@ -201,9 +201,13 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id) drvdata->base = base;
/* Set up coresight component description */ - desc.name = coresight_alloc_device_name(&tpdm_devs, dev); - if (!desc.name) - return -ENOMEM; + desc.name = coresight_get_device_name(dev); + if (!desc.name) { + desc.name = coresight_alloc_device_name(&tpdm_devs, dev); + if (!desc.name) + return -ENOMEM; + } + desc.type = CORESIGHT_DEV_TYPE_SOURCE; desc.subtype.source_subtype = CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS; desc.ops = &tpdm_cs_ops; diff --git a/include/linux/coresight.h b/include/linux/coresight.h index a269fffaf991..caa17c8af865 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -675,6 +675,7 @@ static inline void coresight_write64(struct coresight_device *csdev, u64 val, u3 #endif /* IS_ENABLED(CONFIG_CORESIGHT) */
extern int coresight_get_cpu(struct device *dev); +extern const char *coresight_get_device_name(struct device *dev);
struct coresight_platform_data *coresight_get_platform_data(struct device *dev); struct coresight_connection *
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
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-devicetre... 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);elsecti_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
On 1/2/2024 8:04 PM, Mike Leach wrote:
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:-
- 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:-
For the components bounded with CPU, we can easily identify them by the number in the name. But for other components, we can only get the component type and registers address of it. We can't identify which system it belongs to from current name.
lrwxrwxrwx 1 root 0 0 Jan 1 00:01 cti_sys0 -> ../../../devices/platform/soc@0/138f0000.cti/cti_sys0 lrwxrwxrwx 1 root 0 0 Jan 1 00:01 cti_sys1 -> ../../../devices/platform/soc@0/13900000.cti/cti_sys1 lrwxrwxrwx 1 root 0 0 Jan 1 00:01 tpdm0 -> ../../../devices/platform/soc@0/10b0d000.tpdm/tpdm0 lrwxrwxrwx 1 root 0 0 Jan 1 00:01 tpdm1 -> ../../../devices/platform/soc@0/10c28000.tpdm/tpdm1 lrwxrwxrwx 1 root 0 0 Jan 1 00:01 tpdm2 -> ../../../devices/platform/soc@0/10c29000.tpdm/tpdm2
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
- The patch set must contain the usage and specification in the .yaml
file(s) of the property used.
I will add the usage in yaml.
However, there was a standard property called 'name' which is deprecated - see https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetre... section 2.3.11. I do not believe that adding another 'name' property would be accepted by the DT maintainers.
- 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.
The name in the DT only has the general class of device and registers address. It can't describe the subsystem it belongs to.
Thanks Jinlong Mao
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);elsecti_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
On 28/12/2023 11:26, James Clark 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.
The CTI and ETM can be handled as special cases, like they are already done and fall back to the nodename for the rest ? But, I thought the node names must be generic (e.g, cti) and doesn't really solve the naming requirements for naming CTIs. (e.g, <device>_tpda, etr_cti). Is there something I missed ?
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.
+1
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.
My suggestion was to name the device based on the specific device rather than following a generic rule for all device. e.g., A TPDM connected to modem, could be named as such based on the platform information. It could be any means, for e.g., tpdm nodes are always children nodes of the devices they are connected to ? or could have a phandle to point to the device they are monitoring etc. And the name could be created from the "monitoring device name" + tpdm. Also, we do this for CPU bound CTI and ETMs already, where we name them based on the CPU.
But then the "nodename" is something we explored and it looks like may not be an option.
- 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;}- }
For these special cases, i.e., CPU bound, we should handle them with priority.
if (drvdata->ctidev.cpu >= 0) name = devm_kasprintf(... "cti_cpu%d", .. cpu); else name = coresight_alloc_device_name(...);
/* 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.
On that front, the name is used as a "device" name and not simply display. So, even "device-name" sounds more appropriate.
Suzuki
Thanks James
On 1/3/2024 7:33 PM, Suzuki K Poulose wrote:
On 28/12/2023 11:26, James Clark 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.
The CTI and ETM can be handled as special cases, like they are already done and fall back to the nodename for the rest ? But, I thought the node names must be generic (e.g, cti) and doesn't really solve the naming requirements for naming CTIs. (e.g, <device>_tpda, etr_cti). Is there something I missed ?
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.
+1
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.
My suggestion was to name the device based on the specific device rather than following a generic rule for all device. e.g., A TPDM connected to modem, could be named as such based on the platform information. It could be any means, for e.g., tpdm nodes are always children nodes of the devices they are connected to ? or could have a phandle to point to the device they are monitoring etc. And the name could be created from the "monitoring device name" + tpdm. Also, we do this for CPU bound CTI and ETMs already, where we name them based on the CPU.
TPDM can only connect to the funnel or TPDA. The system TPDM is monitoring may not have the device node in DT.
But then the "nodename" is something we explored and it looks like may not be an option.
- 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; + } + }
For these special cases, i.e., CPU bound, we should handle them with priority.
if (drvdata->ctidev.cpu >= 0) name = devm_kasprintf(... "cti_cpu%d", .. cpu); else name = coresight_alloc_device_name(...);
/* 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.
On that front, the name is used as a "device" name and not simply display. So, even "device-name" sounds more appropriate.
I will use the device-name.
Thanks Jinlong Mao
Suzuki
Thanks James
On 12/28/2023 7:26 PM, James Clark 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.
I will make next patch as V2
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.
I will add it inside the coresight_alloc_device_name.
- 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?
Yes. I will update the binding doc.
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.
From Suzuki's comments, "device-name" should be better. What do you think ?
Thanks Jinlong Mao
-
James Clark -
Jinlong Mao -
Mao Jinlong
-
Mike Leach -
Suzuki K Poulose