devicetree, qcomm PMIC: fix node name conflict
Subsystems (2)
cover devicetree, qcomm PMIC: fix node name conflict
An issue with the path of SPMI nodes under /sys/bus/... was reported in
https://lkml.org/lkml/2014/4/23/312. The symptom is that two different grandchild nodes of the spmi with the same node-name@unit-address will result in attempting to create duplicate links at /sys/bus/platform/devices/unit-address.node-name. It turns out that the specific example provided might not be an expected configuration for current hardware, but the reported trap remains an issue.
I have been poking at the problem, trying to figure out how to cleanly fix the issue without breaking devicetree device creation.
The first patch in the series is the one that may be a very bad idea. Or it may help show the way forward to deal with what I think is the major underlying problem. I have not finished investigating the possible negative side effects. And I am still thinking whether this is a conceptually good approach, or whether it is simply an expediant hack that hides the underlying problem. But I am throwing this out prematurely because I have mentioned it to several people, and I want to make it visible to everyone involved.
The underlying architectural problem (in my opinion) is that a lot of devices are created by the device tree infrastructure as platform devices, when they truly should not be platform devices. They should not be platform devices because they are not physically on a platform bus, they are instead somewhere below some other bus. The first patch in this series is a hack which results in the devices still being represented by "struct platform_device" objects, but with a link to their parent's "struct bus_type" instead of to &platform_bus_type.
The second patch does not require the first patch. The second patch provides a mechanism to allow subsystems to provide a method of naming devices to avoid name collisions.
The third patch provides an example of a subsystem using the new feature provided by the second patch.
The resulting device naming and soft links from applying all three patches, or just the second and third patches are:
===== no patches applied:
$ ls /sys/devices/ ARMv7 Krait cpu-pmu.1 platform software tracepoint breakpoint cpus.0 soc.2 system virtual
$ ls /sys/devices/soc.2/ f9000000.interrupt-controller fc4281d0.qcom,mpm f9011000.smem fc4ab000.restart f9012000.regulator fc4cf000.qcom,spmi f9020000.timer fd484000.hwlock f9088000.clock-controller fd510000.pinctrl f9098000.clock-controller fd8c0000.clock-controller f90a8000.clock-controller gpio_keys.5 f90b8000.clock-controller iio-thermal.4 f9824900.sdhc modalias f991e000.serial power f9924000.i2c2 subsystem f9928000.i2c6 timer.3 f9bff000.rng uevent fc400000.clock-controller
$ ls /sys/devices/soc.2/fc4cf000.qcom,spmi/ driver modalias power spmi-0 subsystem uevent
$ ls /sys/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/ 0-00 0-01 0-04 power subsystem uevent
$ ls /sys/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-00/ 100.qcom,revid driver subsystem 3100.qcom,pm8x41-adc-usr gpios.18 uevent 6000.qcom,rtc power
$ ls /sys/bus/platform/devices/soc.2/ f9000000.interrupt-controller fc4281d0.qcom,mpm f9011000.smem fc4ab000.restart f9012000.regulator fc4cf000.qcom,spmi f9020000.timer fd484000.hwlock f9088000.clock-controller fd510000.pinctrl f9098000.clock-controller fd8c0000.clock-controller f90a8000.clock-controller gpio_keys.5 f90b8000.clock-controller iio-thermal.4 f9824900.sdhc modalias f991e000.serial power f9924000.i2c2 subsystem f9928000.i2c6 timer.3 f9bff000.rng uevent fc400000.clock-controller
$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/ driver modalias power spmi-0 subsystem uevent
$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/ 0-00 0-01 0-04 power subsystem uevent
$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-00/ 100.qcom,revid driver subsystem 3100.qcom,pm8x41-adc-usr gpios.18 uevent 6000.qcom,rtc power
$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-01/ b040.pm8xxx-pwm driver uevent d000.pm8xxx-pwm-led power d800.pm8xxx-wled subsystem
$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-04/ driver power subsystem uevent
$ ls /sys/bus/spmi/devices/ 0-00 0-01 0-04 spmi-0
$ ls /sys/bus/platform/devices/ 100.qcom,revid fc4cf000.qcom,spmi 3100.qcom,pm8x41-adc-usr fd484000.hwlock 6000.qcom,rtc fd510000.pinctrl alarmtimer fd8c0000.clock-controller b040.pm8xxx-pwm gpio_keys.5 cpu-pmu.1 gpios.18 cpus.0 iio-thermal.4 d000.pm8xxx-pwm-led pm8841-s1.6 d800.pm8xxx-wled pm8841-s2.7 f9000000.interrupt-controller pm8941-l3.11 f9011000.smem pm8941-l6.12 f9012000.regulator reg-dummy f9020000.timer regulator-l11.14 f9088000.clock-controller regulator-l19.15 f9098000.clock-controller regulator-l20.16 f90a8000.clock-controller regulator-l22.17 f90b8000.clock-controller regulator-l9.13 f9824900.sdhc regulator-s1.8 f991e000.serial regulator-s2.9 f9924000.i2c2 regulator-s3.10 f9928000.i2c6 regulatory.0 f9bff000.rng snd-soc-dummy fc400000.clock-controller soc.2 fc4281d0.qcom,mpm timer.3 fc4ab000.restart
===== all three patches applied:
$ ls /sys/devices/ ARMv7 Krait cpu-pmu.1 platform software tracepoint breakpoint cpus.0 soc.2 system virtual
$ ls /sys/devices/soc.2/ f9000000.interrupt-controller fc4281d0.qcom,mpm f9011000.smem fc4ab000.restart f9012000.regulator fc4cf000.qcom,spmi f9020000.timer fd484000.hwlock f9088000.clock-controller fd510000.pinctrl f9098000.clock-controller fd8c0000.clock-controller f90a8000.clock-controller gpio_keys.5 f90b8000.clock-controller iio-thermal.4 f9824900.sdhc modalias f991e000.serial power f9924000.i2c2 subsystem f9928000.i2c6 timer.3 f9bff000.rng uevent fc400000.clock-controller
$ ls /sys/devices/soc.2/fc4cf000.qcom,spmi/ driver modalias power spmi-0 subsystem uevent
$ ls /sys/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/ 0-00 0-01 0-04 power subsystem uevent
$ ls /sys/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-00/ 0-00:100.qcom,revid driver 0-00:3100.qcom,pm8x41-adc-usr power 0-00:6000.qcom,rtc subsystem 0-00:gpios uevent
$ ls /sys/bus/platform/devices/soc.2/ f9000000.interrupt-controller fc4281d0.qcom,mpm f9011000.smem fc4ab000.restart f9012000.regulator fc4cf000.qcom,spmi f9020000.timer fd484000.hwlock f9088000.clock-controller fd510000.pinctrl f9098000.clock-controller fd8c0000.clock-controller f90a8000.clock-controller gpio_keys.5 f90b8000.clock-controller iio-thermal.4 f9824900.sdhc modalias f991e000.serial power f9924000.i2c2 subsystem f9928000.i2c6 timer.3 f9bff000.rng uevent fc400000.clock-controller
$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/ driver modalias power spmi-0 subsystem uevent
$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/ 0-00 0-01 0-04 power subsystem uevent
$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-00/ 0-00:100.qcom,revid driver 0-00:3100.qcom,pm8x41-adc-usr power 0-00:6000.qcom,rtc subsystem 0-00:gpios uevent
$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-01/ 0-01:b040.pm8xxx-pwm driver uevent 0-01:d000.pm8xxx-pwm-led power 0-01:d800.pm8xxx-wled subsystem
$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-04/ 0-04:100.qcom,revid power uevent driver subsystem
$ ls /sys/bus/spmi/devices/ 0-00 0-01:b040.pm8xxx-pwm 0-00:100.qcom,revid 0-01:d000.pm8xxx-pwm-led 0-00:3100.qcom,pm8x41-adc-usr 0-01:d800.pm8xxx-wled 0-00:6000.qcom,rtc 0-04 0-00:gpios 0-04:100.qcom,revid 0-01 spmi-0
$ ls /sys/bus/platform/devices/ alarmtimer f9bff000.rng cpu-pmu.1 fc400000.clock-controller cpus.0 fc4281d0.qcom,mpm f9000000.interrupt-controller fc4ab000.restart f9011000.smem fc4cf000.qcom,spmi f9012000.regulator fd484000.hwlock f9020000.timer fd510000.pinctrl f9088000.clock-controller fd8c0000.clock-controller f9098000.clock-controller gpio_keys.5 f90a8000.clock-controller iio-thermal.4 f90b8000.clock-controller reg-dummy f9824900.sdhc regulatory.0 f991e000.serial snd-soc-dummy f9924000.i2c2 soc.2 f9928000.i2c6 timer.3
===== patches 2 and 3 applied:
$ ls /sys/devices/ ARMv7 Krait cpu-pmu.1 platform software tracepoint breakpoint cpus.0 soc.2 system virtual
$ ls /sys/devices/soc.2/ f9000000.interrupt-controller fc4281d0.qcom,mpm f9011000.smem fc4ab000.restart f9012000.regulator fc4cf000.qcom,spmi f9020000.timer fd484000.hwlock f9088000.clock-controller fd510000.pinctrl f9098000.clock-controller fd8c0000.clock-controller f90a8000.clock-controller gpio_keys.5 f90b8000.clock-controller iio-thermal.4 f9824900.sdhc modalias f991e000.serial power f9924000.i2c2 subsystem f9928000.i2c6 timer.3 f9bff000.rng uevent fc400000.clock-controller
$ ls /sys/devices/soc.2/fc4cf000.qcom,spmi/ driver modalias power spmi-0 subsystem uevent
$ ls /sys/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/ 0-00 0-01 0-04 power subsystem uevent
$ ls /sys/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-00/ 0-00:100.qcom,revid driver 0-00:3100.qcom,pm8x41-adc-usr power 0-00:6000.qcom,rtc subsystem 0-00:gpios uevent
$ ls /sys/bus/platform/devices/soc.2/ f9000000.interrupt-controller fc4281d0.qcom,mpm f9011000.smem fc4ab000.restart f9012000.regulator fc4cf000.qcom,spmi f9020000.timer fd484000.hwlock f9088000.clock-controller fd510000.pinctrl f9098000.clock-controller fd8c0000.clock-controller f90a8000.clock-controller gpio_keys.5 f90b8000.clock-controller iio-thermal.4 f9824900.sdhc modalias f991e000.serial power f9924000.i2c2 subsystem f9928000.i2c6 timer.3 f9bff000.rng uevent fc400000.clock-controller
$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/ driver modalias power spmi-0 subsystem uevent
$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/ 0-00 0-01 0-04 power subsystem uevent
$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-00/ 0-00:100.qcom,revid driver 0-00:3100.qcom,pm8x41-adc-usr power 0-00:6000.qcom,rtc subsystem 0-00:gpios uevent
$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-01/ 0-01:b040.pm8xxx-pwm driver uevent 0-01:d000.pm8xxx-pwm-led power 0-01:d800.pm8xxx-wled subsystem
$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-04/ 0-04:100.qcom,revid power uevent driver subsystem
$ ls /sys/bus/spmi/devices/ 0-00 0-01 0-04 spmi-0
$ ls /sys/bus/platform/devices/ 0-00:100.qcom,revid fc4281d0.qcom,mpm 0-00:3100.qcom,pm8x41-adc-usr fc4ab000.restart 0-00:6000.qcom,rtc fc4cf000.qcom,spmi 0-00:gpios fd484000.hwlock 0-01:b040.pm8xxx-pwm fd510000.pinctrl 0-01:d000.pm8xxx-pwm-led fd8c0000.clock-controller 0-01:d800.pm8xxx-wled gpio_keys.5 0-04:100.qcom,revid iio-thermal.4 alarmtimer pm8841-s1.6 cpu-pmu.1 pm8841-s2.7 cpus.0 pm8941-l3.11 f9000000.interrupt-controller pm8941-l6.12 f9011000.smem reg-dummy f9012000.regulator regulator-l11.14 f9020000.timer regulator-l19.15 f9088000.clock-controller regulator-l20.16 f9098000.clock-controller regulator-l22.17 f90a8000.clock-controller regulator-l9.13 f90b8000.clock-controller regulator-s1.8 f9824900.sdhc regulator-s2.9 f991e000.serial regulator-s3.10 f9924000.i2c2 regulatory.0 f9928000.i2c6 snd-soc-dummy f9bff000.rng soc.2 fc400000.clock-controller timer.3
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
RFC PATCH 1/3 devicetree: set bus type same as parent
From: Frank Rowand <frank.rowand-/[email protected]>
This is a somewhat scary patch since it touches a path that is central to device creation based on the device tree. It should not be applied without careful consideration.
I am not sure if this patch is a good idea, even if it does not break anything.
An issue with the path of SPMI nodes under /sys/bus/... was reported in
https://lkml.org/lkml/2014/4/23/312. The symptom is that two different grandchild nodes of the spmi with the same node-name@unit-address will result in attempting to create duplicate links at /sys/bus/platform/devices/unit-address.node-name. It turns out that the specific example provided might not be an expected configuration for current hardware, but the reported trap remains an issue.
The common pattern exposed is a driver probe function calling of_platform_populate() to create child devices. As the reporting email noted, the devices are created with dev.bus set to platform_bus_type. Thus all devices created via this pattern will result in a link in /sys/bus/platform/devices/, with the risk that a name collision will occur.
This patch reduces the scope of possible name collisions to devices on the same bus type. This is still not ideal, because a legal device tree source file can result in run time errors. In the case of SPMI nodes, the collisions will occur in /bus/spmi/devices/.
I have not investigated whether other drivers would be negatively impacted by this change - there are 26 drivers in tree that call of_platform_populate().
--- drivers/of/platform.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/of/platform.c +4 -1
@@ -217,7 +217,10 @@ static struct platform_device *of_platfo dev->dev.coherent_dma_mask =DMA_BIT_MASK (32 );if (!dev->dev.dma_mask) dev->dev.dma_mask = &dev->dev.coherent_dma_mask; - dev->dev.bus = &platform_bus_type; +if (parent && parent->bus) + dev->dev.bus = parent->bus; +else + dev->dev.bus = &platform_bus_type; dev->dev.platform_data = platform_data;/* We do not fill the DMA ops for platform devices by default.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/3] devicetree: set bus type same as parent
Quoted 42 lines · Frank Rowand
From: Frank Rowand <frank.rowand-/[email protected]>
This is a somewhat scary patch since it touches a path that is central to device creation based on the device tree. It should not be applied without careful consideration.
I am not sure if this patch is a good idea, even if it does not break anything.
An issue with the path of SPMI nodes under
/sys/bus/... was reported in
https://lkml.org/lkml/2014/4/23/312. The symptom is that two different grandchild nodes of the spmi with the same node-name@unit-address will result in attempting to create duplicate links at/sys/bus/platform/devices/unit-address.node-name. It turns out that the specific example provided might not be an expected configuration for current hardware, but the reported trap remains an issue.The common pattern exposed is a driver probe function calling
of_platform_populate()to create child devices. As the reporting email noted, the devices are created with dev.bus set toplatform_bus_type. Thus all devices created via this pattern will result in a link in/sys/bus/platform/devices/, with the risk that a name collision will occur.This patch reduces the scope of possible name collisions to devices on the same bus type. This is still not ideal, because a legal device tree source file can result in run time errors. In the case of SPMI nodes, the collisions will occur in /bus/spmi/devices/.
I have not investigated whether other drivers would be negatively impacted by this change - there are 26 drivers in tree that call
of_platform_populate().Signed-off-by Frank Rowand <frank.rowand-/[email protected]>--- drivers/of/platform.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)Index:
b/drivers/of/platform.c
===================================================================
drivers/of/platform.c static struct platform_device *of_platfo · line 223
Quoted 8 lines
drivers/of/platform.c +4
-1@@ -217,7 +217,10 @@ static struct platform_device *of_platfo dev->dev.coherent_dma_mask =DMA_BIT_MASK (32 );if (!dev->dev.dma_mask) dev->dev.dma_mask = &dev->dev.coherent_dma_mask; - dev->dev.bus = &platform_bus_type; +if (parent && parent->bus) + dev->dev.bus = parent->bus; +else + dev->dev.bus = &platform_bus_type;
Yeah, this is a very wrong thing to do. It makes the device accessible to the other bus types behaviour and most likely will result in container_of() being used on the struct device to get the other bus types container structure. It is a very bad idea.
I also suspect that you're conflating the concept of a bus_type and the instance of a bus (like the platform bus) in the /sys/devices/ hierarchy. Among other things, a bus_type contains a list of drivers and a list of devices that can potentially be bound together because they use the same interface.
"platform_bus" on the other hand isn't really a device at all. It is a bare struct device with no bus type and no behaviour. It exists merely as a container for other devices. By default, any platform_device that gets registered will be added as a child of platform_bus unless the parent pointer is already assigned to something else.
The device hierarchy can pretty much be anything. A child can have any bus type regardless of the bus type of the parent. Individual subsystems will put restrictions on this (ie. all children of i2c bus instances will use the i2c bus type), but the driver core is very flexible.
The reason we use platform_bus_type so extensively is that there are a very large number of devices that don't have any bus behaviour expectations. No hot plug. No auto detection. Usually memory mapped. The device just exists and is immediately accessible by the system.
g.
drivers/of/platform.c static struct platform_device *of_platfo · line 226
Quoted 3 lines
drivers/of/platform.c +0
-0dev->dev.platform_data = platform_data;/* We do not fill the DMA ops for platform devices by default.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
RFC PATCH 2/3 devicetree: provide hook to allow setting devicetree device name
In reply to Frank RowandFrom: Frank Rowand <[email protected]>
Optionally push devicetree device naming into a function called dynamically by of_device_alloc().
TODO:
Change made to of_device_alloc() could also be made to of_amba_device_create()
--- drivers/of/platform.c | 2 ++ include/linux/of.h | 2 ++ 3 files changed, 43 insertions(+)
drivers/of/platform.c +2 -0
@@ -179,6 +179,8 @@ struct platform_device *of_device_alloc(if (bus_id)dev_set_name (&dev->dev,"%s" , bus_id); +else if (np->parent->of_device_make_bus_id) + np->parent->of_device_make_bus_id (&dev->dev);else of_device_make_bus_id (&dev->dev);
include/linux/of.h +2 -0
@@ -17,6 +17,7 @@*/ #include <linux/types.h> #include <linux/bitops.h> +#include <linux/device.h> #include <linux/errno.h> #include <linux/kobject.h> #include <linux/mod_devicetable.h>
@@ -60,6 +61,7 @@ struct device_node {struct kobject kobj;unsigned long _flags;void *data; +void (*of_device_make_bus_id)(struct device *dev);#if defined(CONFIG_SPARC )const char *path_component_name;unsigned int unique_id;
Re: [RFC PATCH 2/3] devicetree: provide hook to allow setting devicetree device name
Quoted 20 lines · Frank Rowand
From: Frank Rowand <frank.rowand-/[email protected]>
Optionally push devicetree device naming into a function called dynamically by
of_device_alloc().TODO:
Change made toof_device_alloc()could also be made toof_amba_device_create()Signed-off-by Frank Rowand <frank.rowand-/[email protected]>--- drivers/of/platform.c | 2 ++ include/linux/of.h | 2 ++ 3 files changed, 43 insertions(+)Index:
b/drivers/of/platform.c
===================================================================
drivers/of/platform.c struct platform_device *of_device_alloc( · line 183
Quoted 5 lines
drivers/of/platform.c +2
-0@@ -179,6 +179,8 @@ struct platform_device *of_device_alloc(if (bus_id)dev_set_name (&dev->dev,"%s" , bus_id); +else if (np->parent->of_device_make_bus_id) + np->parent->of_device_make_bus_id (&dev->dev);
This isn't dangerous in the way the first patch is, but I don't want to expose control over the naming scheme outside of the core code. It would be too easy to get wrong and mess up other drivers. The problem that is being hit is clearly a bug in the core DT code. It should be naming devices so that there isn't a conflict. I think Rob's suggested patch is the best fix.
g.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
RFC PATCH 3/3 devicetree, qcomm PMIC: use new hook to make PMIC device names unique
In reply to Frank RowandFrom: Frank Rowand <[email protected]>
The previous patch in the series does:
Optionally push device naming into a function called dynamically by of_device_alloc().
This patch adds an example of using that capability.
--- drivers/mfd/pm8x41.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)
drivers/mfd/pm8x41.c +39 -0
@@ -13,6 +13,7 @@#include <linux/module.h> #include <linux/spmi.h> #include <linux/regmap.h> +#include <linux/of_address.h> #include <linux/of_platform.h> static const struct regmap_config pm8x41_regmap_config = {
@@ -32,6 +33,43 @@ static void pm8x41_remove(struct spmi_dedevice_for_each_child (&sdev->dev,NULL , pm8x41_remove_child); } +static void spmi_of_device_make_bus_id (struct device *dev) +{ +struct device_node *node = dev->of_node; +const __be32 *reg; +u64 addr; +const __be32 *addrp; +struct spmi_device *sdev; + + sdev = container_of(dev->parent, struct spmi_device, dev); + +/* +* For MMIO, get the physical address +*/ + reg =of_get_property (node,"reg" ,NULL ); +if (reg) { +if (of_can_translate_address (node)) { + addr =of_translate_address (node, reg); + }else { + addrp =of_get_address (node,0 ,NULL ,NULL ); +if (addrp) + addr =of_read_number (addrp,1 ); +else + addr =OF_BAD_ADDR ; + } +if (addr !=OF_BAD_ADDR ) { +dev_set_name (dev,"%d-%02x:%llx.%s" , + sdev->ctrl->nr, sdev->usid, + (unsigned long long )addr, node->name); +return ; + } + } + +dev_set_name (dev,"%d-%02x:%s" , + sdev->ctrl->nr, sdev->usid, + node->name); +} +static int pm8x41_probe (struct spmi_device *sdev) {struct regmap *regmap;
@@ -42,6 +80,7 @@ static int pm8x41_probe(struct spmi_devireturn PTR_ERR (regmap); } + sdev->dev.of_node->of_device_make_bus_id = spmi_of_device_make_bus_id;return of_platform_populate (sdev->dev.of_node,NULL ,NULL , &sdev->dev); }
Quoted 28 lines · Frank Rowand
An issue with the path of SPMI nodes under
/sys/bus/... was reported in
https://lkml.org/lkml/2014/4/23/312. The symptom is that two different grandchild nodes of the spmi with the same node-name@unit-address will result in attempting to create duplicate links at/sys/bus/platform/devices/unit-address.node-name. It turns out that the specific example provided might not be an expected configuration for current hardware, but the reported trap remains an issue.I have been poking at the problem, trying to figure out how to cleanly fix the issue without breaking devicetree device creation.
The first patch in the series is the one that may be a very bad idea. Or it may help show the way forward to deal with what I think is the major underlying problem. I have not finished investigating the possible negative side effects. And I am still thinking whether this is a conceptually good approach, or whether it is simply an expediant hack that hides the underlying problem. But I am throwing this out prematurely because I have mentioned it to several people, and I want to make it visible to everyone involved.
The underlying architectural problem (in my opinion) is that a lot of devices are created by the device tree infrastructure as platform devices, when they truly should not be platform devices. They should not be platform devices because they are not physically on a platform bus, they are instead somewhere below some other bus. The first patch in this series is a hack which results in the devices still being represented by "struct
platform_device" objects, but with a link to their parent's "structbus_type" instead of to&platform_bus_type.
Quoted 7 lines
The second patch does not require the first patch. The second patch provides a mechanism to allow subsystems to provide a method of naming devices to avoid name collisions.
The third patch provides an example of a subsystem using the new feature provided by the second patch.
I think the primary question to ask is there any added benefit to having the additional hierarchy of devices. I don't think there is much support to have more hierarchy from what I have seen of past discussions.
Another approach could be to support having multiple platform bus instances. Then drivers can easily create a new instance for each set of sub-devices.
This can be solved in a much less invasive way just in the DT naming algorithm. This is slightly different from what I had suggested of just dropping the unit address. It keeps the unit address, but adds the unique index on untranslate-able addresses. The diff is bigger due to refactoring to reduce the indentation levels. It is untested and whitespace corrupted:
drivers/of/platform.c +25 -15
@@ -105,23 +105,33 @@ void of_device_make_bus_id(struct device *dev)* For MMIO, get the physical address */ reg =of_get_property (node,"reg" ,NULL ); -if (reg) { -if (of_can_translate_address (node)) { - addr =of_translate_address (node, reg); - }else { - addrp =of_get_address (node,0 ,NULL ,NULL ); -if (addrp) - addr =of_read_number (addrp,1 ); -else - addr =OF_BAD_ADDR ; - } -if (addr !=OF_BAD_ADDR ) { -dev_set_name (dev,"%llx.%s" , - (unsigned long long )addr, node->name); -return ; - } +if (!reg) + goto no_bus_id; + +if (of_can_translate_address (node)) { + addr =of_translate_address (node, reg); +if (addr ==OF_BAD_ADDR ) + goto no_bus_id; + +dev_set_name (dev,"%llx.%s" , + (unsigned long long )addr, node->name); +return ; } + addrp =of_get_address (node,0 ,NULL ,NULL ); +if (!addrp) + goto no_bus_id; + + addr =of_read_number (addrp,1 ); +if (addr ==OF_BAD_ADDR ) + goto no_bus_id; + + magic =atomic_add_return (1 , &bus_no_reg_magic); +dev_set_name (dev,"%llx.%s.%d" , (unsigned long long )addr, + node->name, magic -1 ); +return ; + +no_bus_id:/* * No BusID, use the node name and add a globally incremented * counter (and pray...)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 3 lines · Rob Herring
Quoted 2 lines · Frank Rowand
An issue with the path of SPMI nodes under
/sys/bus/... was reported in
https://lkml.org/lkml/2014/4/23/312. The symptom is that two different
< snip >
Quoted 6 lines
Quoted 1 line
I think the primary question to ask is there any added benefit to having the additional hierarchy of devices. I don't think there is much support to have more hierarchy from what I have seen of past discussions.
The hierarchy avoids the name space conflict.
It also maps the physical reality better than pretending all devices are on the platform bus.
It follows the model that non-device tree systems use. For example, why should a usb device show up under /sys/bus/platform/ instead of under /sys/bus/usb/ ? (I'm not positive this actually happens, but let me pretend it would.)
Quoted 55 lines
Another approach could be to support having multiple platform bus instances. Then drivers can easily create a new instance for each set of sub-devices.
This can be solved in a much less invasive way just in the DT naming algorithm. This is slightly different from what I had suggested of just dropping the unit address. It keeps the unit address, but adds the unique index on untranslate-able addresses. The diff is bigger due to refactoring to reduce the indentation levels. It is untested and whitespace corrupted:
drivers/of/platform.c +18
-15@@ -105,23 +105,33 @@ void of_device_make_bus_id(struct device *dev)* For MMIO, get the physical address */ reg =of_get_property (node,"reg" ,NULL ); -if (reg) { -if (of_can_translate_address (node)) { - addr =of_translate_address (node, reg); - }else { - addrp =of_get_address (node,0 ,NULL ,NULL ); -if (addrp) - addr =of_read_number (addrp,1 ); -else - addr =OF_BAD_ADDR ; - } -if (addr !=OF_BAD_ADDR ) { -dev_set_name (dev,"%llx.%s" , - (unsigned long long )addr, node->name); -return ; - } +if (!reg) + goto no_bus_id; + +if (of_can_translate_address (node)) { + addr =of_translate_address (node, reg); +if (addr ==OF_BAD_ADDR ) + goto no_bus_id; + +dev_set_name (dev,"%llx.%s" , + (unsigned long long )addr, node->name); +return ; } + addrp =of_get_address (node,0 ,NULL ,NULL ); +if (!addrp) + goto no_bus_id; + + addr =of_read_number (addrp,1 ); +if (addr ==OF_BAD_ADDR ) + goto no_bus_id;
drivers/of/platform.c void of_device_make_bus_id(struct device *dev) · line 137
Quoted 10 lines
drivers/of/platform.c +7
-0+ + magic =atomic_add_return (1 , &bus_no_reg_magic); +dev_set_name (dev,"%llx.%s.%d" , (unsigned long long )addr, + node->name, magic -1 ); +return ; + +no_bus_id:/* * No BusID, use the node name and add a globally incremented * counter (and pray...)
I think the refactored code is easier to read. (End of bike shed...)
The result of that patch is an even uglier set of device names. I would love to get rid of the bus_no_reg_magic instead of making more extensive use of it. The new names for the system that started this discussion are:
$ ls /sys/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-00/ 100.qcom,revid.19 gpios.21 3100.qcom,pm8x41-adc-usr.20 power 6000.qcom,rtc.18 subsystem driver uevent
$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-00/ 100.qcom,revid.19 gpios.21 3100.qcom,pm8x41-adc-usr.20 power 6000.qcom,rtc.18 subsystem driver uevent
$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-01/ b040.pm8xxx-pwm.22 driver uevent d000.pm8xxx-pwm-led.23 power d800.pm8xxx-wled.24 subsystem
$ ls /sys/bus/platform/devices/soc.2/fc4cf000.qcom,spmi/spmi-0/0-04/ 100.qcom,revid.25 power uevent driver subsystem
$ ls /sys/bus/platform/devices/ 100.qcom,revid.19 fc4ab000.restart 100.qcom,revid.25 fc4cf000.qcom,spmi 3100.qcom,pm8x41-adc-usr.20 fd484000.hwlock 6000.qcom,rtc.18 fd510000.pinctrl alarmtimer fd8c0000.clock-controller b040.pm8xxx-pwm.22 gpio_keys.5 cpu-pmu.1 gpios.21 cpus.0 iio-thermal.4 d000.pm8xxx-pwm-led.23 pm8841-s1.6 d800.pm8xxx-wled.24 pm8841-s2.7 f9000000.interrupt-controller pm8941-l3.11 f9011000.smem pm8941-l6.12 f9012000.regulator reg-dummy f9020000.timer regulator-l11.14 f9088000.clock-controller regulator-l19.15 f9098000.clock-controller regulator-l20.16 f90a8000.clock-controller regulator-l22.17 f90b8000.clock-controller regulator-l9.13 f9824900.sdhc regulator-s1.8 f991e000.serial regulator-s2.9 f9924000.i2c2 regulator-s3.10 f9928000.i2c6 regulatory.0 f9bff000.rng snd-soc-dummy fc400000.clock-controller soc.2 fc4281d0.qcom,mpm timer.3
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 37 lines · Rob Herring
Quoted 28 lines · Frank Rowand
An issue with the path of SPMI nodes under
/sys/bus/... was reported in
https://lkml.org/lkml/2014/4/23/312. The symptom is that two different grandchild nodes of the spmi with the same node-name@unit-address will result in attempting to create duplicate links at/sys/bus/platform/devices/unit-address.node-name. It turns out that the specific example provided might not be an expected configuration for current hardware, but the reported trap remains an issue.I have been poking at the problem, trying to figure out how to cleanly fix the issue without breaking devicetree device creation.
The first patch in the series is the one that may be a very bad idea. Or it may help show the way forward to deal with what I think is the major underlying problem. I have not finished investigating the possible negative side effects. And I am still thinking whether this is a conceptually good approach, or whether it is simply an expediant hack that hides the underlying problem. But I am throwing this out prematurely because I have mentioned it to several people, and I want to make it visible to everyone involved.
The underlying architectural problem (in my opinion) is that a lot of devices are created by the device tree infrastructure as platform devices, when they truly should not be platform devices. They should not be platform devices because they are not physically on a platform bus, they are instead somewhere below some other bus. The first patch in this series is a hack which results in the devices still being represented by "struct
platform_device" objects, but with a link to their parent's "structbus_type" instead of to&platform_bus_type.Quoted 7 lines
The second patch does not require the first patch. The second patch provides a mechanism to allow subsystems to provide a method of naming devices to avoid name collisions.
The third patch provides an example of a subsystem using the new feature provided by the second patch.
Quoted 9 lines
I think the primary question to ask is there any added benefit to having the additional hierarchy of devices. I don't think there is much support to have more hierarchy from what I have seen of past discussions.
Another approach could be to support having multiple platform bus instances. Then drivers can easily create a new instance for each set of sub-devices.
I fear that it will be more invasive that you think it will be. Right now the bus_type abstraction is a mechanism for matching drivers and devices. The bus type contains a bag of device drivers, and it tries to match one of them to a device when a device gets registered to that device (or when a driver gets registered, try to match it to one of the devices it already knows about). You can see this in the /sys/bus/<type>/drivers and /sys/bus/<type>/devices directories.
Splitting the platform bus type into multiple instances is not trivial because the drivers will only be available to one instance. You'd need to figure out how to make a device driver available to multiple bus_type instances (ideally without having to manually add the driver to each bus_type at module load time).
Quoted 47 lines
This can be solved in a much less invasive way just in the DT naming algorithm. This is slightly different from what I had suggested of just dropping the unit address. It keeps the unit address, but adds the unique index on untranslate-able addresses. The diff is bigger due to refactoring to reduce the indentation levels. It is untested and whitespace corrupted:
drivers/of/platform.c +15
-15@@ -105,23 +105,33 @@ void of_device_make_bus_id(struct device *dev)* For MMIO, get the physical address */ reg =of_get_property (node,"reg" ,NULL ); -if (reg) { -if (of_can_translate_address (node)) { - addr =of_translate_address (node, reg); - }else { - addrp =of_get_address (node,0 ,NULL ,NULL ); -if (addrp) - addr =of_read_number (addrp,1 ); -else - addr =OF_BAD_ADDR ; - } -if (addr !=OF_BAD_ADDR ) { -dev_set_name (dev,"%llx.%s" , - (unsigned long long )addr, node->name); -return ; - } +if (!reg) + goto no_bus_id; + +if (of_can_translate_address (node)) { + addr =of_translate_address (node, reg); +if (addr ==OF_BAD_ADDR ) + goto no_bus_id; + +dev_set_name (dev,"%llx.%s" , + (unsigned long long )addr, node->name); +return ; } + addrp =of_get_address (node,0 ,NULL ,NULL ); +if (!addrp) + goto no_bus_id; +
drivers/of/platform.c void of_device_make_bus_id(struct device *dev) · line 134
Quoted 10 lines
drivers/of/platform.c +10
-0+ addr =of_read_number (addrp,1 ); +if (addr ==OF_BAD_ADDR ) + goto no_bus_id; + + magic =atomic_add_return (1 , &bus_no_reg_magic); +dev_set_name (dev,"%llx.%s.%d" , (unsigned long long )addr, + node->name, magic -1 ); +return ; + +no_bus_id:
Looks like a reasonable change to me.
g.
Quoted 3 lines
drivers/of/platform.c +0
-0/* * No BusID, use the node name and add a globally incremented * counter (and pray...)
Quoted 46 lines · Rob Herring
Quoted 28 lines · Frank Rowand
An issue with the path of SPMI nodes under
/sys/bus/... was reported in
https://lkml.org/lkml/2014/4/23/312. The symptom is that two different grandchild nodes of the spmi with the same node-name@unit-address will result in attempting to create duplicate links at/sys/bus/platform/devices/unit-address.node-name. It turns out that the specific example provided might not be an expected configuration for current hardware, but the reported trap remains an issue.I have been poking at the problem, trying to figure out how to cleanly fix the issue without breaking devicetree device creation.
The first patch in the series is the one that may be a very bad idea. Or it may help show the way forward to deal with what I think is the major underlying problem. I have not finished investigating the possible negative side effects. And I am still thinking whether this is a conceptually good approach, or whether it is simply an expediant hack that hides the underlying problem. But I am throwing this out prematurely because I have mentioned it to several people, and I want to make it visible to everyone involved.
The underlying architectural problem (in my opinion) is that a lot of devices are created by the device tree infrastructure as platform devices, when they truly should not be platform devices. They should not be platform devices because they are not physically on a platform bus, they are instead somewhere below some other bus. The first patch in this series is a hack which results in the devices still being represented by "struct
platform_device" objects, but with a link to their parent's "structbus_type" instead of to&platform_bus_type.Quoted 7 lines
The second patch does not require the first patch. The second patch provides a mechanism to allow subsystems to provide a method of naming devices to avoid name collisions.
The third patch provides an example of a subsystem using the new feature provided by the second patch.
I think the primary question to ask is there any added benefit to having the additional hierarchy of devices. I don't think there is much support to have more hierarchy from what I have seen of past discussions.
Another approach could be to support having multiple platform bus instances. Then drivers can easily create a new instance for each set of sub-devices.
Quoted 6 lines
This can be solved in a much less invasive way just in the DT naming algorithm. This is slightly different from what I had suggested of just dropping the unit address. It keeps the unit address, but adds the unique index on untranslate-able addresses. The diff is bigger due to refactoring to reduce the indentation levels. It is untested and whitespace corrupted:
The unique index leads to an interesting dependency between the order of nodes in the DTB and userspace; paths to e.g. our the path to our block devices contains soc.X where X changes now and then. Fortunately soc.X won't change that often, but forcing more peripheral nodes to use the same schema would show the problem quite often.
Does translatable/untranslatable refer to if this is an address translatable my the cpu or that it's just a translatable address on this specific bus? As far as I can see it's the latter and in our case (revid { reg = <0x100, 0x100>; })
seem to be translatable with the current implementation.
Regards,
Bjorn
Quoted 9 lines · Bjorn Andersson
Quoted 8 lines · Rob Herring
Quoted 7 lines · Frank Rowand
An issue with the path of SPMI nodes under
/sys/bus/... was reported in
https://lkml.org/lkml/2014/4/23/312. The symptom is that two different grandchild nodes of the spmi with the same node-name@unit-address will result in attempting to create duplicate links at/sys/bus/platform/devices/unit-address.node-name. It turns out that the specific example provided might not be an expected configuration for current hardware, but the reported trap remains an issue.
[...]
Quoted 7 lines
Quoted 6 lines
This can be solved in a much less invasive way just in the DT naming algorithm. This is slightly different from what I had suggested of just dropping the unit address. It keeps the unit address, but adds the unique index on untranslate-able addresses. The diff is bigger due to refactoring to reduce the indentation levels. It is untested and whitespace corrupted:
Quoted 5 lines
The unique index leads to an interesting dependency between the order of nodes in the DTB and userspace; paths to e.g. our the path to our block devices contains soc.X where X changes now and then. Fortunately soc.X won't change that often, but forcing more peripheral nodes to use the same schema would show the problem quite often.
Userspace depending on the device names is broken and device names are not considered part of the sysfs ABI. So devices having randomish names is a feature. The names and location change when platforms convert to DT. The location changes when someone decides to add an soc device to a platform as well.
Quoted 5 lines
Does translatable/untranslatable refer to if this is an address translatable my the cpu or that it's just a translatable address on this specific bus? As far as I can see it's the latter and in our case (revid { reg = <
0x100,0x100>; })
seem to be translatable with the current implementation.
It should be the former. If the current behavior is the latter, then the problem is in a different spot.
Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html