Re: [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict
On Wed, May 7, 2014 at 10:12 AM, Bjorn Andersson wrote:
> On Tue, May 6, 2014 at 6:32 PM, Rob Herring wrote:
>> On Tue, May 6, 2014 at 7:48 PM, Frank Rowand 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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict
On Tue, May 6, 2014 at 6:32 PM, Rob Herring wrote:
> On Tue, May 6, 2014 at 7:48 PM, Frank Rowand 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 _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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict
On Wed, May 7, 2014 at 2:32 AM, Rob Herring wrote:
> On Tue, May 6, 2014 at 7:48 PM, Frank Rowand 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 _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//drivers and /sys/bus//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,
Re: [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict
On Wed, May 7, 2014 at 2:32 AM, Rob Herring robherri...@gmail.com wrote:
On Tue, May 6, 2014 at 7:48 PM, Frank Rowand frowand.l...@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,
Re: [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict
On Tue, May 6, 2014 at 6:32 PM, Rob Herring robherri...@gmail.com wrote:
On Tue, May 6, 2014 at 7:48 PM, Frank Rowand frowand.l...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict
On Wed, May 7, 2014 at 10:12 AM, Bjorn Andersson bj...@kryo.se wrote:
On Tue, May 6, 2014 at 6:32 PM, Rob Herring robherri...@gmail.com wrote:
On Tue, May 6, 2014 at 7:48 PM, Frank Rowand frowand.l...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict
On 5/6/2014 6:32 PM, Rob Herring wrote:
> On Tue, May 6, 2014 at 7:48 PM, Frank Rowand 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, _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.19gpios.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.19gpios.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.20fd484000.hwlock
6000.qcom,rtc.18 fd51.pinctrl
alarmtimer fd8c.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.24pm8841-s2.7
f900.interrupt-controller pm8941-l3.11
f9011000.smem
Re: [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict
On Tue, May 6, 2014 at 7:48 PM, Frank Rowand 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 _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, _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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict
On Tue, May 6, 2014 at 7:48 PM, Frank Rowand frowand.l...@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...)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/3] devicetree, qcomm PMIC: fix node name conflict
On 5/6/2014 6:32 PM, Rob Herring wrote:
On Tue, May 6, 2014 at 7:48 PM, Frank Rowand frowand.l...@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.19gpios.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.19gpios.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.20fd484000.hwlock
6000.qcom,rtc.18 fd51.pinctrl
alarmtimer fd8c.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.24pm8841-s2.7
f900.interrupt-controller pm8941-l3.11
f9011000.smem pm8941-l6.12
f9012000.regulator
