Re: [PATCH 5/6] OF: Introduce Device Tree resolve support.
Hi Grant, On Mar 19, 2013, at 7:18 PM, Grant Likely wrote: > On Tue, 19 Mar 2013 13:51:01 +0200, Pantelis Antoniou > wrote: >> Hi Grant, >> >> On Mar 16, 2013, at 11:24 AM, Grant Likely wrote: >> >>> On Wed, 23 Jan 2013 12:58:02 +0200, Pantelis Antoniou >>> wrote: Hi David, On Jan 23, 2013, at 6:40 AM, David Gibson wrote: > Ok. Nonetheless it's not hard to avoid a recursive approach here. How can I find the maximum phandle value of a subtree without using recursion. Note that the whole function is just 6 lines long. >>> >>> It's a failure in the existing kernel DT data structures. We need a hash >>> lookup for the phandles to eliminate the search entirely. Then you'd be >>> able to allocated new phandles on the fly easily and resolve phandles >>> without searching the whole tree (which has always been horrible). >>> >> >> Yes, it is pretty obvious that the in-kernel data structures are sub-optimal. >> But I was not after modifying them, since that's a different kind of problem. > > Think about it this way; fixing up that aspect of the data structure > makes the job you're trying to do a lot easier. I don't feel bad about > asking you to add a radix tree for phandle lookups when it makes your > patches a whole lot better. :-) > Oh that's going to be fun... maybe. >> Since we're having a 'sub-optimal' data structures, I'd like to point out >> that >> the usage of of_find_by_name(), mostly by drivers trying to find a child >> of their own node, works by a lucky accident of how the device nodes are >> instantiated >> by the flat tree loader. Most of the use cases should be replaced by a call >> to of_get_child_by_name() which does the right thing. > > It is true. In fact, calling of_find_node_by_name() when using .dtb is > most likely a bug since using node name to determine behaviour is > strongly discouraged. Maybe mark the function as deprecated then? Easier to get people to fix it. > >> Fair enough, but be warned that phandle resolution the overlay feature is >> mostly useless. >> >> In actual practice the amount of driver nodes that can be overlaid without a >> single case >> of referencing phandles outside (or within) their own blob is close to zero. > > That's not what I'm saying. I'm saying that (at least for now) we should > require the overlay to already know the phandles from the parent and to > refuse to load an overlay that defines phandles already in use in the > base. Overlays do become usable at that point. A mechanism for phandle > resolution so that conflicts can be found and resolved can be added > as a feature enhancement. By splitting it out you'll be able to get the > overlay feature merged even if we don't have agreement on the resolution > mechanism yet. > As long as we don't come up with something that's too difficult to use for non kernel developers. I would hate to punt and push all the complexity of phandle resolution to the driver/cape developer. FWIW, the resolution mechanism we use on the beaglebone seems to work just fine, and people seem to handle it just fine. The part that trips them is mostly how to install the updated dtc compiler that's required. > g. > Regards -- Pantelis -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/6] OF: Introduce Device Tree resolve support.
On Tue, 19 Mar 2013 13:51:01 +0200, Pantelis Antoniou wrote: > Hi Grant, > > On Mar 16, 2013, at 11:24 AM, Grant Likely wrote: > > > On Wed, 23 Jan 2013 12:58:02 +0200, Pantelis Antoniou > > wrote: > >> Hi David, > >> > >> On Jan 23, 2013, at 6:40 AM, David Gibson wrote: > >>> Ok. Nonetheless it's not hard to avoid a recursive approach here. > >> > >> How can I find the maximum phandle value of a subtree without using > >> recursion. > >> Note that the whole function is just 6 lines long. > > > > It's a failure in the existing kernel DT data structures. We need a hash > > lookup for the phandles to eliminate the search entirely. Then you'd be > > able to allocated new phandles on the fly easily and resolve phandles > > without searching the whole tree (which has always been horrible). > > > > Yes, it is pretty obvious that the in-kernel data structures are sub-optimal. > But I was not after modifying them, since that's a different kind of problem. Think about it this way; fixing up that aspect of the data structure makes the job you're trying to do a lot easier. I don't feel bad about asking you to add a radix tree for phandle lookups when it makes your patches a whole lot better. :-) > Since we're having a 'sub-optimal' data structures, I'd like to point out that > the usage of of_find_by_name(), mostly by drivers trying to find a child > of their own node, works by a lucky accident of how the device nodes are > instantiated > by the flat tree loader. Most of the use cases should be replaced by a call > to of_get_child_by_name() which does the right thing. It is true. In fact, calling of_find_node_by_name() when using .dtb is most likely a bug since using node name to determine behaviour is strongly discouraged. > Fair enough, but be warned that phandle resolution the overlay feature is > mostly useless. > > In actual practice the amount of driver nodes that can be overlaid without a > single case > of referencing phandles outside (or within) their own blob is close to zero. That's not what I'm saying. I'm saying that (at least for now) we should require the overlay to already know the phandles from the parent and to refuse to load an overlay that defines phandles already in use in the base. Overlays do become usable at that point. A mechanism for phandle resolution so that conflicts can be found and resolved can be added as a feature enhancement. By splitting it out you'll be able to get the overlay feature merged even if we don't have agreement on the resolution mechanism yet. g. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/6] OF: Introduce Device Tree resolve support.
Hi Grant, On Mar 16, 2013, at 11:24 AM, Grant Likely wrote: > On Wed, 23 Jan 2013 12:58:02 +0200, Pantelis Antoniou > wrote: >> Hi David, >> >> On Jan 23, 2013, at 6:40 AM, David Gibson wrote: >>> Ok. Nonetheless it's not hard to avoid a recursive approach here. >> >> How can I find the maximum phandle value of a subtree without using >> recursion. >> Note that the whole function is just 6 lines long. > > It's a failure in the existing kernel DT data structures. We need a hash > lookup for the phandles to eliminate the search entirely. Then you'd be > able to allocated new phandles on the fly easily and resolve phandles > without searching the whole tree (which has always been horrible). > Yes, it is pretty obvious that the in-kernel data structures are sub-optimal. But I was not after modifying them, since that's a different kind of problem. Since we're having a 'sub-optimal' data structures, I'd like to point out that the usage of of_find_by_name(), mostly by drivers trying to find a child of their own node, works by a lucky accident of how the device nodes are instantiated by the flat tree loader. Most of the use cases should be replaced by a call to of_get_child_by_name() which does the right thing. > That said, I'd like to punt on the whole phandle resolution thing. The > DT overlay support can be merged without the phandle resolution support > if the core rejects any overlays with phandle collisions. > Fair enough, but be warned that phandle resolution the overlay feature is mostly useless. In actual practice the amount of driver nodes that can be overlaid without a single case of referencing phandles outside (or within) their own blob is close to zero. > g. Regards -- Pantelis -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/6] OF: Introduce Device Tree resolve support.
On Wed, 23 Jan 2013 12:58:02 +0200, Pantelis Antoniou wrote: > Hi David, > > On Jan 23, 2013, at 6:40 AM, David Gibson wrote: > > Ok. Nonetheless it's not hard to avoid a recursive approach here. > > How can I find the maximum phandle value of a subtree without using recursion. > Note that the whole function is just 6 lines long. It's a failure in the existing kernel DT data structures. We need a hash lookup for the phandles to eliminate the search entirely. Then you'd be able to allocated new phandles on the fly easily and resolve phandles without searching the whole tree (which has always been horrible). That said, I'd like to punt on the whole phandle resolution thing. The DT overlay support can be merged without the phandle resolution support if the core rejects any overlays with phandle collisions. g. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/6] OF: Introduce Device Tree resolve support.
Hi David, On Jan 23, 2013, at 6:40 AM, David Gibson wrote: > On Tue, Jan 22, 2013 at 01:06:09PM +0200, Pantelis Antoniou wrote: >> Hi >> >> On Jan 22, 2013, at 6:05 AM, David Gibson wrote: >> >>> On Mon, Jan 21, 2013 at 12:59:15PM +0200, Pantelis Antoniou wrote: Hi David On Jan 21, 2013, at 6:48 AM, David Gibson wrote: > On Fri, Jan 04, 2013 at 09:31:09PM +0200, Pantelis Antoniou wrote: >> Introduce support for dynamic device tree resolution. >> Using it, it is possible to prepare a device tree that's >> been loaded on runtime to be modified and inserted at the kernel >> live tree. >> >> Signed-off-by: Pantelis Antoniou >> --- >> .../devicetree/dynamic-resolution-notes.txt| 25 ++ >> drivers/of/Kconfig | 9 + >> drivers/of/Makefile| 1 + >> drivers/of/resolver.c | 394 >> + >> include/linux/of.h | 17 + >> 5 files changed, 446 insertions(+) >> create mode 100644 Documentation/devicetree/dynamic-resolution-notes.txt >> create mode 100644 drivers/of/resolver.c >> >> diff --git a/Documentation/devicetree/dynamic-resolution-notes.txt >> b/Documentation/devicetree/dynamic-resolution-notes.txt >> new file mode 100644 >> index 000..0b396c4 >> --- /dev/null >> +++ b/Documentation/devicetree/dynamic-resolution-notes.txt >> @@ -0,0 +1,25 @@ >> +Device Tree Dynamic Resolver Notes >> +-- >> + >> +This document describes the implementation of the in-kernel >> +Device Tree resolver, residing in drivers/of/resolver.c and is a >> +companion document to Documentation/devicetree/dt-object-internal.txt[1] >> + >> +How the resolver works >> +-- >> + >> +The resolver is given as an input an arbitrary tree compiled with the >> +proper dtc option and having a /plugin/ tag. This generates the >> +appropriate __fixups__ & __local_fixups__ nodes as described in [1]. >> + >> +In sequence the resolver works by the following steps: >> + >> +1. Get the maximum device tree phandle value from the live tree + 1. >> +2. Adjust all the local phandles of the tree to resolve by that amount. >> +3. Using the __local__fixups__ node information adjust all local >> references >> + by the same amount. >> +4. For each property in the __fixups__ node locate the node it >> references >> + in the live tree. This is the label used to tag the node. >> +5. Retrieve the phandle of the target of the fixup. >> +5. For each fixup in the property locate the node:property:offset >> location >> + and replace it with the phandle value. > > Hrm. So, I'm really still not convinced by this approach. > > First, I think it's unwise to allow overlays to change > essentially anything in the base tree, rather than having the base > tree define sockets of some sort where things can be attached. > One could say that the labels define the sockets. It's not just things to be attached, properties might have to change, or something more complex, as we've found out in practice. >>> >>> Hrm. I know a number of these have come up previously in that big >>> thread, but can you summarise some of these cases here. If things >>> need modification in the base tree that still seems to me like the >>> base tree hasn't properly described the socket arrangement (I realise >>> that allowing such descriptions may require extensions to some of our >>> device tree conventions). >>> >> >> It would be pointless to number all the use-cases that Grant put in that >> long document, but I can summarize the cases that we've seen on the bone. >> >> * Addition of child device nodes to the ocp node, creates new platform >> devices of various kind (audio/video/pwms/timers) - almost any kind of >> platform device that the SoC supports. Removing the overlay unregisters >> the devices (but precious few drivers support that cleanly ATM). Since >> the capes don't support hotplug that's not a big deal. > > Ok, that's just adding nodes, which is straightforward. > >> * Addition of pinctrl configuration nodes. > > Ok, do you know where I can look to see how the pinctrl stuff works? > There's information about it in Documentation/pinctrl.txt, and there's the source in drivers/pinctrl. >> * Addition of i2c/spi etc device nodes and modification of the parent's >> node status property to "okay", > > Ok. I'm assuming this is basically just to enable the bus controller > which was previously disabled because it had nothing attached to it? > Yes, but the configuration of the device might need to be altered. For example the clock, or adding/removing some properties that affect the driver. >> c
Re: [PATCH 5/6] OF: Introduce Device Tree resolve support.
On Tue, Jan 22, 2013 at 01:06:09PM +0200, Pantelis Antoniou wrote: > Hi > > On Jan 22, 2013, at 6:05 AM, David Gibson wrote: > > > On Mon, Jan 21, 2013 at 12:59:15PM +0200, Pantelis Antoniou wrote: > >> Hi David > >> > >> On Jan 21, 2013, at 6:48 AM, David Gibson wrote: > >> > >>> On Fri, Jan 04, 2013 at 09:31:09PM +0200, Pantelis Antoniou wrote: > Introduce support for dynamic device tree resolution. > Using it, it is possible to prepare a device tree that's > been loaded on runtime to be modified and inserted at the kernel > live tree. > > Signed-off-by: Pantelis Antoniou > --- > .../devicetree/dynamic-resolution-notes.txt| 25 ++ > drivers/of/Kconfig | 9 + > drivers/of/Makefile| 1 + > drivers/of/resolver.c | 394 > + > include/linux/of.h | 17 + > 5 files changed, 446 insertions(+) > create mode 100644 Documentation/devicetree/dynamic-resolution-notes.txt > create mode 100644 drivers/of/resolver.c > > diff --git a/Documentation/devicetree/dynamic-resolution-notes.txt > b/Documentation/devicetree/dynamic-resolution-notes.txt > new file mode 100644 > index 000..0b396c4 > --- /dev/null > +++ b/Documentation/devicetree/dynamic-resolution-notes.txt > @@ -0,0 +1,25 @@ > +Device Tree Dynamic Resolver Notes > +-- > + > +This document describes the implementation of the in-kernel > +Device Tree resolver, residing in drivers/of/resolver.c and is a > +companion document to Documentation/devicetree/dt-object-internal.txt[1] > + > +How the resolver works > +-- > + > +The resolver is given as an input an arbitrary tree compiled with the > +proper dtc option and having a /plugin/ tag. This generates the > +appropriate __fixups__ & __local_fixups__ nodes as described in [1]. > + > +In sequence the resolver works by the following steps: > + > +1. Get the maximum device tree phandle value from the live tree + 1. > +2. Adjust all the local phandles of the tree to resolve by that amount. > +3. Using the __local__fixups__ node information adjust all local > references > + by the same amount. > +4. For each property in the __fixups__ node locate the node it > references > + in the live tree. This is the label used to tag the node. > +5. Retrieve the phandle of the target of the fixup. > +5. For each fixup in the property locate the node:property:offset > location > + and replace it with the phandle value. > >>> > >>> Hrm. So, I'm really still not convinced by this approach. > >>> > >>> First, I think it's unwise to allow overlays to change > >>> essentially anything in the base tree, rather than having the base > >>> tree define sockets of some sort where things can be attached. > >>> > >> > >> One could say that the labels define the sockets. It's not just things > >> to be attached, properties might have to change, or something more complex, > >> as we've found out in practice. > > > > Hrm. I know a number of these have come up previously in that big > > thread, but can you summarise some of these cases here. If things > > need modification in the base tree that still seems to me like the > > base tree hasn't properly described the socket arrangement (I realise > > that allowing such descriptions may require extensions to some of our > > device tree conventions). > > > > It would be pointless to number all the use-cases that Grant put in that > long document, but I can summarize the cases that we've seen on the bone. > > * Addition of child device nodes to the ocp node, creates new platform > devices of various kind (audio/video/pwms/timers) - almost any kind of > platform device that the SoC supports. Removing the overlay unregisters > the devices (but precious few drivers support that cleanly ATM). Since > the capes don't support hotplug that's not a big deal. Ok, that's just adding nodes, which is straightforward. > * Addition of pinctrl configuration nodes. Ok, do you know where I can look to see how the pinctrl stuff works? > * Addition of i2c/spi etc device nodes and modification of the parent's > node status property to "okay", Ok. I'm assuming this is basically just to enable the bus controller which was previously disabled because it had nothing attached to it? > creates the bus platform device & registers > the devices on the bus. Slight complication with i2c client devices which > are not platform devices need special handling. And this part is again just adding nodes. > * Modification of configuration parameters of a disabled device and subsequent > enablement. What sorts of modification are necessary to th
Re: [PATCH 5/6] OF: Introduce Device Tree resolve support.
Hi On Jan 22, 2013, at 6:05 AM, David Gibson wrote: > On Mon, Jan 21, 2013 at 12:59:15PM +0200, Pantelis Antoniou wrote: >> Hi David >> >> On Jan 21, 2013, at 6:48 AM, David Gibson wrote: >> >>> On Fri, Jan 04, 2013 at 09:31:09PM +0200, Pantelis Antoniou wrote: Introduce support for dynamic device tree resolution. Using it, it is possible to prepare a device tree that's been loaded on runtime to be modified and inserted at the kernel live tree. Signed-off-by: Pantelis Antoniou --- .../devicetree/dynamic-resolution-notes.txt| 25 ++ drivers/of/Kconfig | 9 + drivers/of/Makefile| 1 + drivers/of/resolver.c | 394 + include/linux/of.h | 17 + 5 files changed, 446 insertions(+) create mode 100644 Documentation/devicetree/dynamic-resolution-notes.txt create mode 100644 drivers/of/resolver.c diff --git a/Documentation/devicetree/dynamic-resolution-notes.txt b/Documentation/devicetree/dynamic-resolution-notes.txt new file mode 100644 index 000..0b396c4 --- /dev/null +++ b/Documentation/devicetree/dynamic-resolution-notes.txt @@ -0,0 +1,25 @@ +Device Tree Dynamic Resolver Notes +-- + +This document describes the implementation of the in-kernel +Device Tree resolver, residing in drivers/of/resolver.c and is a +companion document to Documentation/devicetree/dt-object-internal.txt[1] + +How the resolver works +-- + +The resolver is given as an input an arbitrary tree compiled with the +proper dtc option and having a /plugin/ tag. This generates the +appropriate __fixups__ & __local_fixups__ nodes as described in [1]. + +In sequence the resolver works by the following steps: + +1. Get the maximum device tree phandle value from the live tree + 1. +2. Adjust all the local phandles of the tree to resolve by that amount. +3. Using the __local__fixups__ node information adjust all local references + by the same amount. +4. For each property in the __fixups__ node locate the node it references + in the live tree. This is the label used to tag the node. +5. Retrieve the phandle of the target of the fixup. +5. For each fixup in the property locate the node:property:offset location + and replace it with the phandle value. >>> >>> Hrm. So, I'm really still not convinced by this approach. >>> >>> First, I think it's unwise to allow overlays to change >>> essentially anything in the base tree, rather than having the base >>> tree define sockets of some sort where things can be attached. >>> >> >> One could say that the labels define the sockets. It's not just things >> to be attached, properties might have to change, or something more complex, >> as we've found out in practice. > > Hrm. I know a number of these have come up previously in that big > thread, but can you summarise some of these cases here. If things > need modification in the base tree that still seems to me like the > base tree hasn't properly described the socket arrangement (I realise > that allowing such descriptions may require extensions to some of our > device tree conventions). > It would be pointless to number all the use-cases that Grant put in that long document, but I can summarize the cases that we've seen on the bone. * Addition of child device nodes to the ocp node, creates new platform devices of various kind (audio/video/pwms/timers) - almost any kind of platform device that the SoC supports. Removing the overlay unregisters the devices (but precious few drivers support that cleanly ATM). Since the capes don't support hotplug that's not a big deal. * Addition of pinctrl configuration nodes. * Addition of i2c/spi etc device nodes and modification of the parent's node status property to "okay", creates the bus platform device & registers the devices on the bus. Slight complication with i2c client devices which are not platform devices need special handling. * Modification of configuration parameters of a disabled device and subsequent enablement. >> As far as the unwise part, a good deal of care has been taken so that >> people that don't use the overlay functionality have absolutely no >> overhead, or anything modified in the way they use DT. > > Yeah, that's not what I'm concerned about. I'm concerned about hard > to debug problems because some subtle change in the base tree or the > overlay or both causes the overlay to alter something in the base tree > it really shouldn't. > Define shouldn't. Let's say someone inserts a bogus overlay that makes the system fail, or misbehave in some other manner. What is the different between using the overlay and insert
Re: [PATCH 5/6] OF: Introduce Device Tree resolve support.
On Mon, Jan 21, 2013 at 12:59:15PM +0200, Pantelis Antoniou wrote: > Hi David > > On Jan 21, 2013, at 6:48 AM, David Gibson wrote: > > > On Fri, Jan 04, 2013 at 09:31:09PM +0200, Pantelis Antoniou wrote: > >> Introduce support for dynamic device tree resolution. > >> Using it, it is possible to prepare a device tree that's > >> been loaded on runtime to be modified and inserted at the kernel > >> live tree. > >> > >> Signed-off-by: Pantelis Antoniou > >> --- > >> .../devicetree/dynamic-resolution-notes.txt| 25 ++ > >> drivers/of/Kconfig | 9 + > >> drivers/of/Makefile| 1 + > >> drivers/of/resolver.c | 394 > >> + > >> include/linux/of.h | 17 + > >> 5 files changed, 446 insertions(+) > >> create mode 100644 Documentation/devicetree/dynamic-resolution-notes.txt > >> create mode 100644 drivers/of/resolver.c > >> > >> diff --git a/Documentation/devicetree/dynamic-resolution-notes.txt > >> b/Documentation/devicetree/dynamic-resolution-notes.txt > >> new file mode 100644 > >> index 000..0b396c4 > >> --- /dev/null > >> +++ b/Documentation/devicetree/dynamic-resolution-notes.txt > >> @@ -0,0 +1,25 @@ > >> +Device Tree Dynamic Resolver Notes > >> +-- > >> + > >> +This document describes the implementation of the in-kernel > >> +Device Tree resolver, residing in drivers/of/resolver.c and is a > >> +companion document to Documentation/devicetree/dt-object-internal.txt[1] > >> + > >> +How the resolver works > >> +-- > >> + > >> +The resolver is given as an input an arbitrary tree compiled with the > >> +proper dtc option and having a /plugin/ tag. This generates the > >> +appropriate __fixups__ & __local_fixups__ nodes as described in [1]. > >> + > >> +In sequence the resolver works by the following steps: > >> + > >> +1. Get the maximum device tree phandle value from the live tree + 1. > >> +2. Adjust all the local phandles of the tree to resolve by that amount. > >> +3. Using the __local__fixups__ node information adjust all local > >> references > >> + by the same amount. > >> +4. For each property in the __fixups__ node locate the node it references > >> + in the live tree. This is the label used to tag the node. > >> +5. Retrieve the phandle of the target of the fixup. > >> +5. For each fixup in the property locate the node:property:offset location > >> + and replace it with the phandle value. > > > > Hrm. So, I'm really still not convinced by this approach. > > > > First, I think it's unwise to allow overlays to change > > essentially anything in the base tree, rather than having the base > > tree define sockets of some sort where things can be attached. > > > > One could say that the labels define the sockets. It's not just things > to be attached, properties might have to change, or something more complex, > as we've found out in practice. Hrm. I know a number of these have come up previously in that big thread, but can you summarise some of these cases here. If things need modification in the base tree that still seems to me like the base tree hasn't properly described the socket arrangement (I realise that allowing such descriptions may require extensions to some of our device tree conventions). > As far as the unwise part, a good deal of care has been taken so that > people that don't use the overlay functionality have absolutely no > overhead, or anything modified in the way they use DT. Yeah, that's not what I'm concerned about. I'm concerned about hard to debug problems because some subtle change in the base tree or the overlay or both causes the overlay to alter something in the base tree it really shouldn't. > > Second, even allowing overlays to change anything, I don't see > > a lot of reason to do this kind of resolution within the kernel and > > with data stored in the dtb itself, rather than doing the resolution > > in userspace from an annotated overlay dts or dtb, then inserting the > > fully resolved product into the kernel. In either case, the overlay > > needs to be constructed with pretty intimate knowledge of the base > > tree. > > Fair enough, but that's one more thing of user-space crud to drag along, which > will get enabled pretty late in the boot sequence. Meaning a whole bunch of > devices, > like consoles, and root filesystems on the devices that need an overlay to > operate > won't work easily enough. Hrm. But doesn't your scheme already require userspace to identify the hardware and load the overlay? So why is having it resolve the overlay significantly harder? AFAICT devices wanted early can be handled in several possible ways without having the resolved in kernel: an initramfs is the most obvious, but for things you want really early, it should be possible to do the resolution from the platform's bootloader update tool - so the p
Re: [PATCH 5/6] OF: Introduce Device Tree resolve support.
Hi David
On Jan 21, 2013, at 6:48 AM, David Gibson wrote:
> On Fri, Jan 04, 2013 at 09:31:09PM +0200, Pantelis Antoniou wrote:
>> Introduce support for dynamic device tree resolution.
>> Using it, it is possible to prepare a device tree that's
>> been loaded on runtime to be modified and inserted at the kernel
>> live tree.
>>
>> Signed-off-by: Pantelis Antoniou
>> ---
>> .../devicetree/dynamic-resolution-notes.txt| 25 ++
>> drivers/of/Kconfig | 9 +
>> drivers/of/Makefile| 1 +
>> drivers/of/resolver.c | 394
>> +
>> include/linux/of.h | 17 +
>> 5 files changed, 446 insertions(+)
>> create mode 100644 Documentation/devicetree/dynamic-resolution-notes.txt
>> create mode 100644 drivers/of/resolver.c
>>
>> diff --git a/Documentation/devicetree/dynamic-resolution-notes.txt
>> b/Documentation/devicetree/dynamic-resolution-notes.txt
>> new file mode 100644
>> index 000..0b396c4
>> --- /dev/null
>> +++ b/Documentation/devicetree/dynamic-resolution-notes.txt
>> @@ -0,0 +1,25 @@
>> +Device Tree Dynamic Resolver Notes
>> +--
>> +
>> +This document describes the implementation of the in-kernel
>> +Device Tree resolver, residing in drivers/of/resolver.c and is a
>> +companion document to Documentation/devicetree/dt-object-internal.txt[1]
>> +
>> +How the resolver works
>> +--
>> +
>> +The resolver is given as an input an arbitrary tree compiled with the
>> +proper dtc option and having a /plugin/ tag. This generates the
>> +appropriate __fixups__ & __local_fixups__ nodes as described in [1].
>> +
>> +In sequence the resolver works by the following steps:
>> +
>> +1. Get the maximum device tree phandle value from the live tree + 1.
>> +2. Adjust all the local phandles of the tree to resolve by that amount.
>> +3. Using the __local__fixups__ node information adjust all local references
>> + by the same amount.
>> +4. For each property in the __fixups__ node locate the node it references
>> + in the live tree. This is the label used to tag the node.
>> +5. Retrieve the phandle of the target of the fixup.
>> +5. For each fixup in the property locate the node:property:offset location
>> + and replace it with the phandle value.
>
> Hrm. So, I'm really still not convinced by this approach.
>
> First, I think it's unwise to allow overlays to change
> essentially anything in the base tree, rather than having the base
> tree define sockets of some sort where things can be attached.
>
One could say that the labels define the sockets. It's not just things
to be attached, properties might have to change, or something more complex,
as we've found out in practice.
As far as the unwise part, a good deal of care has been taken so that
people that don't use the overlay functionality have absolutely no
overhead, or anything modified in the way they use DT.
> Second, even allowing overlays to change anything, I don't see
> a lot of reason to do this kind of resolution within the kernel and
> with data stored in the dtb itself, rather than doing the resolution
> in userspace from an annotated overlay dts or dtb, then inserting the
> fully resolved product into the kernel. In either case, the overlay
> needs to be constructed with pretty intimate knowledge of the base
> tree.
>
Fair enough, but that's one more thing of user-space crud to drag along, which
will get enabled pretty late in the boot sequence. Meaning a whole bunch of
devices,
like consoles, and root filesystems on the devices that need an overlay to
operate
won't work easily enough.
> That said, I have some implementation comments below.
>
> [snip]
>> +/**
>> + * Find a subtree's maximum phandle value.
>> + */
>> +static phandle __of_get_tree_max_phandle(struct device_node *node,
>> +phandle max_phandle)
>> +{
>> +struct device_node *child;
>> +
>> +if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL &&
>> +node->phandle > max_phandle)
>> +max_phandle = node->phandle;
>> +
>> +__for_each_child_of_node(node, child)
>> +max_phandle = __of_get_tree_max_phandle(child, max_phandle);
>
> Recursion is best avoided given the kernel's limited stack space.
> This is also trivial to implement non-recursively, using the allnext
> pointer.
>
The caller passes a tree that's not yet been inserted in the live tree.
So there's no allnodes pointer yet. Care has been taken for the function
to not have excessive local variables. I would guess about 20-32 bytes for
the stack frame + the local variables, so with a 4K stack we would overflow at
a
nest level of 128, which has a pretty slim chance for a real system.
>> +
>> +return max_phandle;
>> +}
>> +
>> +/**
>> + * Find live tree's maximum phandle value.
>> + */
>> +static phandle of_get_tree_max_phandle(void
Re: [PATCH 5/6] OF: Introduce Device Tree resolve support.
On Fri, Jan 04, 2013 at 09:31:09PM +0200, Pantelis Antoniou wrote:
> Introduce support for dynamic device tree resolution.
> Using it, it is possible to prepare a device tree that's
> been loaded on runtime to be modified and inserted at the kernel
> live tree.
>
> Signed-off-by: Pantelis Antoniou
> ---
> .../devicetree/dynamic-resolution-notes.txt| 25 ++
> drivers/of/Kconfig | 9 +
> drivers/of/Makefile| 1 +
> drivers/of/resolver.c | 394
> +
> include/linux/of.h | 17 +
> 5 files changed, 446 insertions(+)
> create mode 100644 Documentation/devicetree/dynamic-resolution-notes.txt
> create mode 100644 drivers/of/resolver.c
>
> diff --git a/Documentation/devicetree/dynamic-resolution-notes.txt
> b/Documentation/devicetree/dynamic-resolution-notes.txt
> new file mode 100644
> index 000..0b396c4
> --- /dev/null
> +++ b/Documentation/devicetree/dynamic-resolution-notes.txt
> @@ -0,0 +1,25 @@
> +Device Tree Dynamic Resolver Notes
> +--
> +
> +This document describes the implementation of the in-kernel
> +Device Tree resolver, residing in drivers/of/resolver.c and is a
> +companion document to Documentation/devicetree/dt-object-internal.txt[1]
> +
> +How the resolver works
> +--
> +
> +The resolver is given as an input an arbitrary tree compiled with the
> +proper dtc option and having a /plugin/ tag. This generates the
> +appropriate __fixups__ & __local_fixups__ nodes as described in [1].
> +
> +In sequence the resolver works by the following steps:
> +
> +1. Get the maximum device tree phandle value from the live tree + 1.
> +2. Adjust all the local phandles of the tree to resolve by that amount.
> +3. Using the __local__fixups__ node information adjust all local references
> + by the same amount.
> +4. For each property in the __fixups__ node locate the node it references
> + in the live tree. This is the label used to tag the node.
> +5. Retrieve the phandle of the target of the fixup.
> +5. For each fixup in the property locate the node:property:offset location
> + and replace it with the phandle value.
Hrm. So, I'm really still not convinced by this approach.
First, I think it's unwise to allow overlays to change
essentially anything in the base tree, rather than having the base
tree define sockets of some sort where things can be attached.
Second, even allowing overlays to change anything, I don't see
a lot of reason to do this kind of resolution within the kernel and
with data stored in the dtb itself, rather than doing the resolution
in userspace from an annotated overlay dts or dtb, then inserting the
fully resolved product into the kernel. In either case, the overlay
needs to be constructed with pretty intimate knowledge of the base
tree.
That said, I have some implementation comments below.
[snip]
> +/**
> + * Find a subtree's maximum phandle value.
> + */
> +static phandle __of_get_tree_max_phandle(struct device_node *node,
> + phandle max_phandle)
> +{
> + struct device_node *child;
> +
> + if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL &&
> + node->phandle > max_phandle)
> + max_phandle = node->phandle;
> +
> + __for_each_child_of_node(node, child)
> + max_phandle = __of_get_tree_max_phandle(child, max_phandle);
Recursion is best avoided given the kernel's limited stack space.
This is also trivial to implement non-recursively, using the allnext
pointer.
> +
> + return max_phandle;
> +}
> +
> +/**
> + * Find live tree's maximum phandle value.
> + */
> +static phandle of_get_tree_max_phandle(void)
> +{
> + struct device_node *node;
> + phandle phandle;
> +
> + /* get root node */
> + node = of_find_node_by_path("/");
> + if (node == NULL)
> + return OF_PHANDLE_ILLEGAL;
> +
> + /* now search recursively */
> + read_lock(&devtree_lock);
> + phandle = __of_get_tree_max_phandle(node, 0);
> + read_unlock(&devtree_lock);
> +
> + of_node_put(node);
> +
> + return phandle;
> +}
> +
> +/**
> + * Adjust a subtree's phandle values by a given delta.
> + * Makes sure not to just adjust the device node's phandle value,
> + * but modify the phandle properties values as well.
> + */
> +static void __of_adjust_tree_phandles(struct device_node *node,
> + int phandle_delta)
> +{
> + struct device_node *child;
> + struct property *prop;
> + phandle phandle;
> +
> + /* first adjust the node's phandle direct value */
> + if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL)
> + node->phandle += phandle_delta;
You need to have some kind of check for overflow here, or the adjusted
phandle could be one of the illegal values (0 or -1) - or wrap around
and colllide with existing
[PATCH 5/6] OF: Introduce Device Tree resolve support.
Introduce support for dynamic device tree resolution.
Using it, it is possible to prepare a device tree that's
been loaded on runtime to be modified and inserted at the kernel
live tree.
Signed-off-by: Pantelis Antoniou
---
.../devicetree/dynamic-resolution-notes.txt| 25 ++
drivers/of/Kconfig | 9 +
drivers/of/Makefile| 1 +
drivers/of/resolver.c | 394 +
include/linux/of.h | 17 +
5 files changed, 446 insertions(+)
create mode 100644 Documentation/devicetree/dynamic-resolution-notes.txt
create mode 100644 drivers/of/resolver.c
diff --git a/Documentation/devicetree/dynamic-resolution-notes.txt
b/Documentation/devicetree/dynamic-resolution-notes.txt
new file mode 100644
index 000..0b396c4
--- /dev/null
+++ b/Documentation/devicetree/dynamic-resolution-notes.txt
@@ -0,0 +1,25 @@
+Device Tree Dynamic Resolver Notes
+--
+
+This document describes the implementation of the in-kernel
+Device Tree resolver, residing in drivers/of/resolver.c and is a
+companion document to Documentation/devicetree/dt-object-internal.txt[1]
+
+How the resolver works
+--
+
+The resolver is given as an input an arbitrary tree compiled with the
+proper dtc option and having a /plugin/ tag. This generates the
+appropriate __fixups__ & __local_fixups__ nodes as described in [1].
+
+In sequence the resolver works by the following steps:
+
+1. Get the maximum device tree phandle value from the live tree + 1.
+2. Adjust all the local phandles of the tree to resolve by that amount.
+3. Using the __local__fixups__ node information adjust all local references
+ by the same amount.
+4. For each property in the __fixups__ node locate the node it references
+ in the live tree. This is the label used to tag the node.
+5. Retrieve the phandle of the target of the fixup.
+5. For each fixup in the property locate the node:property:offset location
+ and replace it with the phandle value.
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index d37bfcf..f9a6193 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -83,4 +83,13 @@ config OF_MTD
depends on MTD
def_bool y
+config OF_RESOLVE
+ bool "OF Dynamic resolution support"
+ depends on OF
+ select OF_DYNAMIC
+ select OF_DEVICE
+ help
+ Enable OF dynamic resolution support. This allows you to
+ load Device Tree object fragments are run time.
+
endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index abcc08a..9a809c9 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_OF_MDIO) += of_mdio.o
obj-$(CONFIG_OF_PCI) += of_pci.o
obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o
obj-$(CONFIG_OF_MTD) += of_mtd.o
+obj-$(CONFIG_OF_RESOLVE) += resolver.o
diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
new file mode 100644
index 000..016d7da
--- /dev/null
+++ b/drivers/of/resolver.c
@@ -0,0 +1,394 @@
+/*
+ * Functions for dealing with DT resolution
+ *
+ * Copyright (C) 2012 Pantelis Antoniou
+ * Copyright (C) 2012 Texas Instruments Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/**
+ * Find a subtree's maximum phandle value.
+ */
+static phandle __of_get_tree_max_phandle(struct device_node *node,
+ phandle max_phandle)
+{
+ struct device_node *child;
+
+ if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL &&
+ node->phandle > max_phandle)
+ max_phandle = node->phandle;
+
+ __for_each_child_of_node(node, child)
+ max_phandle = __of_get_tree_max_phandle(child, max_phandle);
+
+ return max_phandle;
+}
+
+/**
+ * Find live tree's maximum phandle value.
+ */
+static phandle of_get_tree_max_phandle(void)
+{
+ struct device_node *node;
+ phandle phandle;
+
+ /* get root node */
+ node = of_find_node_by_path("/");
+ if (node == NULL)
+ return OF_PHANDLE_ILLEGAL;
+
+ /* now search recursively */
+ read_lock(&devtree_lock);
+ phandle = __of_get_tree_max_phandle(node, 0);
+ read_unlock(&devtree_lock);
+
+ of_node_put(node);
+
+ return phandle;
+}
+
+/**
+ * Adjust a subtree's phandle values by a given delta.
+ * Makes sure not to just adjust the device node's phandle value,
+ * but modify the phandle properties values as well.
+ */
+static void __of_adjust_tree_phandles(struct device_node *node,
+ int phandle_delta)
+{
+ struct device_node *child;
+ struct property *prop;
+ phandle ph
