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