* [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict
@ 2014-05-07 0:48 ` Frank Rowand
0 siblings, 0 replies; 18+ messages in thread
From: Frank Rowand @ 2014-05-07 0:48 UTC (permalink / raw)
To: Grant Likely, Rob Herring, devicetree, Linux Kernel list,
Josh Cartwright, Courtney Cavin
Cc: Samuel Ortiz, Lee Jones, Greg Kroah-Hartman
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict
@ 2014-05-07 0:48 ` Frank Rowand
0 siblings, 0 replies; 18+ messages in thread
From: Frank Rowand @ 2014-05-07 0:48 UTC (permalink / raw)
To: Grant Likely, Rob Herring, devicetree-u79uwXL29TY76Z2rM5mHXA,
Linux Kernel list, Josh Cartwright, Courtney Cavin
Cc: Samuel Ortiz, Lee Jones, Greg Kroah-Hartman
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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
* [RFC PATCH 1/3] devicetree: set bus type same as parent
@ 2014-05-07 0:51 ` Frank Rowand
0 siblings, 0 replies; 18+ messages in thread
From: Frank Rowand @ 2014-05-07 0:51 UTC (permalink / raw)
To: Grant Likely, Rob Herring, devicetree, Linux Kernel list,
Josh Cartwright, Courtney Cavin
Cc: Samuel Ortiz, Lee Jones, Greg Kroah-Hartman
From: Frank Rowand <frank.rowand@sonymobile.com>
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().
Signed-off-by: Frank Rowand <frank.rowand@sonymobile.com>
---
drivers/of/platform.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Index: b/drivers/of/platform.c
===================================================================
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [RFC PATCH 1/3] devicetree: set bus type same as parent
@ 2014-05-07 0:51 ` Frank Rowand
0 siblings, 0 replies; 18+ messages in thread
From: Frank Rowand @ 2014-05-07 0:51 UTC (permalink / raw)
To: Grant Likely, Rob Herring, devicetree-u79uwXL29TY76Z2rM5mHXA,
Linux Kernel list, Josh Cartwright, Courtney Cavin
Cc: Samuel Ortiz, Lee Jones, Greg Kroah-Hartman
From: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
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().
Signed-off-by: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
---
drivers/of/platform.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Index: b/drivers/of/platform.c
===================================================================
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
* [RFC PATCH 2/3] devicetree: provide hook to allow setting devicetree device name
2014-05-07 0:48 ` Frank Rowand
(?)
(?)
@ 2014-05-07 0:52 ` Frank Rowand
2014-05-07 15:21 ` Grant Likely
-1 siblings, 1 reply; 18+ messages in thread
From: Frank Rowand @ 2014-05-07 0:52 UTC (permalink / raw)
To: Grant Likely, Rob Herring, devicetree, Linux Kernel list,
Josh Cartwright, Courtney Cavin
Cc: Samuel Ortiz, Lee Jones, Greg Kroah-Hartman
From: Frank Rowand <frank.rowand@sonymobile.com>
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()
Signed-off-by: Frank Rowand <frank.rowand@sonymobile.com>
---
drivers/of/platform.c | 2 ++
include/linux/of.h | 2 ++
3 files changed, 43 insertions(+)
Index: b/drivers/of/platform.c
===================================================================
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -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);
Index: b/include/linux/of.h
===================================================================
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -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;
^ permalink raw reply [flat|nested] 18+ messages in thread
* [RFC PATCH 3/3] devicetree, qcomm PMIC: use new hook to make PMIC device names unique
2014-05-07 0:48 ` Frank Rowand
` (2 preceding siblings ...)
(?)
@ 2014-05-07 0:53 ` Frank Rowand
-1 siblings, 0 replies; 18+ messages in thread
From: Frank Rowand @ 2014-05-07 0:53 UTC (permalink / raw)
To: Grant Likely, Rob Herring, devicetree, Linux Kernel list,
Josh Cartwright, Courtney Cavin
Cc: Samuel Ortiz, Lee Jones, Greg Kroah-Hartman
From: Frank Rowand <frank.rowand@sonymobile.com>
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.
Signed-off-by: Frank Rowand <frank.rowand@sonymobile.com>
---
drivers/mfd/pm8x41.c | 39 +++++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
Index: b/drivers/mfd/pm8x41.c
===================================================================
--- a/drivers/mfd/pm8x41.c
+++ b/drivers/mfd/pm8x41.c
@@ -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_de
device_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_devi
return 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);
}
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict
@ 2014-05-07 1:32 ` Rob Herring
0 siblings, 0 replies; 18+ messages in thread
From: Rob Herring @ 2014-05-07 1:32 UTC (permalink / raw)
To: Frank Rowand
Cc: Grant Likely, Rob Herring, devicetree, Linux Kernel list,
Josh Cartwright, Courtney Cavin, Samuel Ortiz, Lee Jones,
Greg Kroah-Hartman
On Tue, May 6, 2014 at 7:48 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> 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.
>
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:
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 404d1da..c77dd7a 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -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...)
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict
@ 2014-05-07 1:32 ` Rob Herring
0 siblings, 0 replies; 18+ messages in thread
From: Rob Herring @ 2014-05-07 1:32 UTC (permalink / raw)
To: Frank Rowand
Cc: Grant Likely, Rob Herring, devicetree-u79uwXL29TY76Z2rM5mHXA,
Linux Kernel list, Josh Cartwright, Courtney Cavin, Samuel Ortiz,
Lee Jones, Greg Kroah-Hartman
On Tue, May 6, 2014 at 7:48 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 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.
>
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:
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 404d1da..c77dd7a 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict
@ 2014-05-07 2:49 ` Frank Rowand
0 siblings, 0 replies; 18+ messages in thread
From: Frank Rowand @ 2014-05-07 2:49 UTC (permalink / raw)
To: Rob Herring
Cc: Grant Likely, Rob Herring, devicetree, Linux Kernel list,
Josh Cartwright, Courtney Cavin, Samuel Ortiz, Lee Jones,
Greg Kroah-Hartman
On 5/6/2014 6:32 PM, Rob Herring wrote:
> On Tue, May 6, 2014 at 7:48 PM, Frank Rowand <frowand.list@gmail.com> wrote:
>> 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 >
>>
>
> 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.)
> 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:
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 404d1da..c77dd7a 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -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...)
>
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict
@ 2014-05-07 2:49 ` Frank Rowand
0 siblings, 0 replies; 18+ messages in thread
From: Frank Rowand @ 2014-05-07 2:49 UTC (permalink / raw)
To: Rob Herring
Cc: Grant Likely, Rob Herring, devicetree-u79uwXL29TY76Z2rM5mHXA,
Linux Kernel list, Josh Cartwright, Courtney Cavin, Samuel Ortiz,
Lee Jones, Greg Kroah-Hartman
On 5/6/2014 6:32 PM, Rob Herring wrote:
> On Tue, May 6, 2014 at 7:48 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> 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 >
>>
>
> 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.)
> 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:
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 404d1da..c77dd7a 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -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...)
>
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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict
2014-05-07 1:32 ` Rob Herring
(?)
(?)
@ 2014-05-07 14:51 ` Grant Likely
-1 siblings, 0 replies; 18+ messages in thread
From: Grant Likely @ 2014-05-07 14:51 UTC (permalink / raw)
To: Rob Herring
Cc: Frank Rowand, Rob Herring, devicetree, Linux Kernel list,
Josh Cartwright, Courtney Cavin, Samuel Ortiz, Lee Jones,
Greg Kroah-Hartman
On Wed, May 7, 2014 at 2:32 AM, Rob Herring <robherring2@gmail.com> wrote:
> On Tue, May 6, 2014 at 7:48 PM, Frank Rowand <frowand.list@gmail.com> wrote:
>> 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.
>>
>
> 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).
> 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:
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 404d1da..c77dd7a 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -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:
Looks like a reasonable change to me.
g.
> /*
> * No BusID, use the node name and add a globally incremented
> * counter (and pray...)
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict
2014-05-07 1:32 ` Rob Herring
` (2 preceding siblings ...)
(?)
@ 2014-05-07 15:12 ` Bjorn Andersson
2014-05-07 16:08 ` Rob Herring
-1 siblings, 1 reply; 18+ messages in thread
From: Bjorn Andersson @ 2014-05-07 15:12 UTC (permalink / raw)
To: Rob Herring
Cc: Frank Rowand, Grant Likely, Rob Herring, devicetree,
Linux Kernel list, Josh Cartwright, Courtney Cavin, Samuel Ortiz,
Lee Jones, Greg Kroah-Hartman
On Tue, May 6, 2014 at 6:32 PM, Rob Herring <robherring2@gmail.com> wrote:
> On Tue, May 6, 2014 at 7:48 PM, Frank Rowand <frowand.list@gmail.com> wrote:
>> 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.
>>
>
> 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:
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC PATCH 1/3] devicetree: set bus type same as parent
@ 2014-05-07 15:17 ` Grant Likely
0 siblings, 0 replies; 18+ messages in thread
From: Grant Likely @ 2014-05-07 15:17 UTC (permalink / raw)
To: Frank Rowand
Cc: Rob Herring, devicetree, Linux Kernel list, Josh Cartwright,
Courtney Cavin, Samuel Ortiz, Lee Jones, Greg Kroah-Hartman
On Wed, May 7, 2014 at 1:51 AM, Frank Rowand <frowand.list@gmail.com> wrote:
> From: Frank Rowand <frank.rowand@sonymobile.com>
>
> 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().
>
> Signed-off-by: Frank Rowand <frank.rowand@sonymobile.com>
> ---
> drivers/of/platform.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> Index: b/drivers/of/platform.c
> ===================================================================
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -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.
> dev->dev.platform_data = platform_data;
>
> /* We do not fill the DMA ops for platform devices by default.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC PATCH 1/3] devicetree: set bus type same as parent
@ 2014-05-07 15:17 ` Grant Likely
0 siblings, 0 replies; 18+ messages in thread
From: Grant Likely @ 2014-05-07 15:17 UTC (permalink / raw)
To: Frank Rowand
Cc: Rob Herring, devicetree-u79uwXL29TY76Z2rM5mHXA,
Linux Kernel list, Josh Cartwright, Courtney Cavin, Samuel Ortiz,
Lee Jones, Greg Kroah-Hartman
On Wed, May 7, 2014 at 1:51 AM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
>
> 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().
>
> Signed-off-by: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
> ---
> drivers/of/platform.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> Index: b/drivers/of/platform.c
> ===================================================================
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -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.
> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC PATCH 2/3] devicetree: provide hook to allow setting devicetree device name
@ 2014-05-07 15:21 ` Grant Likely
0 siblings, 0 replies; 18+ messages in thread
From: Grant Likely @ 2014-05-07 15:21 UTC (permalink / raw)
To: Frank Rowand
Cc: Rob Herring, devicetree, Linux Kernel list, Josh Cartwright,
Courtney Cavin, Samuel Ortiz, Lee Jones, Greg Kroah-Hartman
On Wed, May 7, 2014 at 1:52 AM, Frank Rowand <frowand.list@gmail.com> wrote:
> From: Frank Rowand <frank.rowand@sonymobile.com>
>
> 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()
>
> Signed-off-by: Frank Rowand <frank.rowand@sonymobile.com>
> ---
> drivers/of/platform.c | 2 ++
> include/linux/of.h | 2 ++
> 3 files changed, 43 insertions(+)
>
> Index: b/drivers/of/platform.c
> ===================================================================
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC PATCH 2/3] devicetree: provide hook to allow setting devicetree device name
@ 2014-05-07 15:21 ` Grant Likely
0 siblings, 0 replies; 18+ messages in thread
From: Grant Likely @ 2014-05-07 15:21 UTC (permalink / raw)
To: Frank Rowand
Cc: Rob Herring, devicetree-u79uwXL29TY76Z2rM5mHXA,
Linux Kernel list, Josh Cartwright, Courtney Cavin, Samuel Ortiz,
Lee Jones, Greg Kroah-Hartman
On Wed, May 7, 2014 at 1:52 AM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
>
> 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()
>
> Signed-off-by: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
> ---
> drivers/of/platform.c | 2 ++
> include/linux/of.h | 2 ++
> 3 files changed, 43 insertions(+)
>
> Index: b/drivers/of/platform.c
> ===================================================================
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict
@ 2014-05-07 16:08 ` Rob Herring
0 siblings, 0 replies; 18+ messages in thread
From: Rob Herring @ 2014-05-07 16:08 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Frank Rowand, Grant Likely, Rob Herring, devicetree,
Linux Kernel list, Josh Cartwright, Courtney Cavin, Samuel Ortiz,
Lee Jones, Greg Kroah-Hartman
On Wed, May 7, 2014 at 10:12 AM, Bjorn Andersson <bjorn@kryo.se> wrote:
> On Tue, May 6, 2014 at 6:32 PM, Rob Herring <robherring2@gmail.com> wrote:
>> On Tue, May 6, 2014 at 7:48 PM, Frank Rowand <frowand.list@gmail.com> wrote:
>>> 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.
[...]
>> 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.
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.
> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict
@ 2014-05-07 16:08 ` Rob Herring
0 siblings, 0 replies; 18+ messages in thread
From: Rob Herring @ 2014-05-07 16:08 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Frank Rowand, Grant Likely, Rob Herring,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Kernel list,
Josh Cartwright, Courtney Cavin, Samuel Ortiz, Lee Jones,
Greg Kroah-Hartman
On Wed, May 7, 2014 at 10:12 AM, Bjorn Andersson <bjorn-UYDU3/A3LUY@public.gmane.org> wrote:
> On Tue, May 6, 2014 at 6:32 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Tue, May 6, 2014 at 7:48 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> 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.
[...]
>> 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.
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.
> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2014-05-07 16:08 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-07 0:48 [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict Frank Rowand
2014-05-07 0:48 ` Frank Rowand
2014-05-07 0:51 ` [RFC PATCH 1/3] devicetree: set bus type same as parent Frank Rowand
2014-05-07 0:51 ` Frank Rowand
2014-05-07 15:17 ` Grant Likely
2014-05-07 15:17 ` Grant Likely
2014-05-07 0:52 ` [RFC PATCH 2/3] devicetree: provide hook to allow setting devicetree device name Frank Rowand
2014-05-07 15:21 ` Grant Likely
2014-05-07 15:21 ` Grant Likely
2014-05-07 0:53 ` [RFC PATCH 3/3] devicetree, qcomm PMIC: use new hook to make PMIC device names unique Frank Rowand
2014-05-07 1:32 ` [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict Rob Herring
2014-05-07 1:32 ` Rob Herring
2014-05-07 2:49 ` Frank Rowand
2014-05-07 2:49 ` Frank Rowand
2014-05-07 14:51 ` Grant Likely
2014-05-07 15:12 ` Bjorn Andersson
2014-05-07 16:08 ` Rob Herring
2014-05-07 16:08 ` Rob Herring
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.