| Message ID | 20250512115028.1783305-2-m-shah@ti.com |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | Propagate bootph-* properties to all supernodes | expand |
On Mon, 12 May 2025 at 13:50, Moteen Shah <m-shah@ti.com> wrote: > > Add a function to scan through all the nodes in the device-tree > for bootph-* property. If found, propagate it to all of its parent > nodes up the hierarchy. > > Signed-off-by: Moteen Shah <m-shah@ti.com> > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > tools/binman/control.py | 51 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) Reviewed-by: Simon Glass <sjg@chromium.org>
Hi Moteen, On 5/12/25 1:50 PM, Moteen Shah wrote: > Add a function to scan through all the nodes in the device-tree > for bootph-* property. If found, propagate it to all of its parent It's not scanning for bootph-* properties. It's scanning for two specific bootph properties, so try to update the commit log and comments so it's not misleading. The commit log is lacking information as to why we need to do this, what this helps with and why this is specific to bootph-all and bootph-some-ram. We need to be able to look at this patch in 5 years and have a clue of why this was required. > nodes up the hierarchy. > > Signed-off-by: Moteen Shah <m-shah@ti.com> > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > tools/binman/control.py | 51 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/tools/binman/control.py b/tools/binman/control.py > index 81f61e3e152..ccf081cb686 100644 > --- a/tools/binman/control.py > +++ b/tools/binman/control.py > @@ -530,6 +530,54 @@ def _RemoveTemplates(parent): > for node in del_nodes: > node.Delete() > > +def propagate_bootph(node, prop): > + """Propagate bootph-* property to all the parent nodes up the hierarchy > + Misleading, it's not propagating bootph properties, it's propagating the prop passed as an argument to the function, so rename to something like propagate_prop and update the comment. Be clear also that this is expected to be a boolean property (because of AddEmptyProp). > + Args: > + node (fdt.Node): Node to propagate the property to Specify that all its parents (recursively) will have this property propagated to. > + prop (str): Property to propagate > + > + Return: > + True if any change was made, else False > + """ > + changed = False > + while node: > + if prop not in node.props: > + node.AddEmptyProp(prop, 0) > + changed = True > + node = node.parent > + return changed > + > +def scan_and_prop_bootph(node): > + """Propagate bootph properties from children to parents > + > + The bootph schema indicates that bootph nodes in children should be implied s/nodes/properties/ > + in their parents, all the way up the hierarchy. This is expensive to > + implement in U-Boot before relocation, so this function explicitly I think the point here is "at runtime" which is missing. > + propagates these bootph properties upwards. at build time :) > + > + This is used to set the bootph-* property in the parent node if a > + "bootph-*" property is found in any of the parent's subnodes. > + This is all misleading, we aren't propagating all bootph properties > + Args: > + node (fdt.Node): Node to propagate the property to Same remarks as for the propagate_bootph method above. > + prop (str): Property name to propagate > + > + Return: > + True if any change was made, else False > + > + """ > + bootph_prop = ['bootph-all', 'bootph-some-ram'] > + Please explain why those only. > + changed = False > + for prop in bootph_prop: > + if prop in node.props: > + changed |= propagate_bootph(node.parent, prop) > + > + for subnode in node.subnodes: > + changed |= scan_and_prop_bootph(subnode) > + return changed > + I have a gut feeling this is suboptimal, though simple and easy to maintain. We currently go up to the root node for each and every found bootph-{all,some-ram} property. We could simply divide this by half (or more if there are more properties to propagate in the future) by passing the props to propagate instead of just one. We can keep track of which node has propagated the property already and, for its children, stop the propagation at that level instead of going up to the root node all the time. I'm not sure typical Device Trees are deep enough for us to care about that kind of optimization for now, especially since it'll be running at build time only? Cheers, Quentin
Hey Quentin, Thanks for the review. On 13/05/25 16:10, Quentin Schulz wrote: > Hi Moteen, > > On 5/12/25 1:50 PM, Moteen Shah wrote: >> Add a function to scan through all the nodes in the device-tree >> for bootph-* property. If found, propagate it to all of its parent > > It's not scanning for bootph-* properties. It's scanning for two > specific bootph properties, so try to update the commit log and > comments so it's not misleading. > I will update the commit message in the revision. > The commit log is lacking information as to why we need to do this, > what this helps with and why this is specific to bootph-all and > bootph-some-ram. > Why this is specific to bootph-all and bootph-some-ram The other ones are handled by fdtgrep as those are for the SPL stage. > > We need to be able to look at this patch in 5 years and have a clue of > why this was required. Noted. > >> nodes up the hierarchy. >> >> Signed-off-by: Moteen Shah <m-shah@ti.com> >> Signed-off-by: Simon Glass <sjg@chromium.org> >> --- >> tools/binman/control.py | 51 +++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 51 insertions(+) >> >> diff --git a/tools/binman/control.py b/tools/binman/control.py >> index 81f61e3e152..ccf081cb686 100644 >> --- a/tools/binman/control.py >> +++ b/tools/binman/control.py >> @@ -530,6 +530,54 @@ def _RemoveTemplates(parent): >> for node in del_nodes: >> node.Delete() >> +def propagate_bootph(node, prop): >> + """Propagate bootph-* property to all the parent nodes up the >> hierarchy >> + > > Misleading, it's not propagating bootph properties, it's propagating > the prop passed as an argument to the function, so rename to something > like propagate_prop and update the comment. Okay, will rename this to propagate_prop. > > Be clear also that this is expected to be a boolean property (because > of AddEmptyProp). Noted. > >> + Args: >> + node (fdt.Node): Node to propagate the property to > > Specify that all its parents (recursively) will have this property > propagated to. It is iterative not recursive. How about: "Node and all its parent nodes above it to propagate the property to iteratively" > >> + prop (str): Property to propagate >> + >> + Return: >> + True if any change was made, else False >> + """ >> + changed = False >> + while node: >> + if prop not in node.props: >> + node.AddEmptyProp(prop, 0) >> + changed = True >> + node = node.parent >> + return changed >> + >> +def scan_and_prop_bootph(node): >> + """Propagate bootph properties from children to parents >> + >> + The bootph schema indicates that bootph nodes in children should >> be implied > > s/nodes/properties/ Will change in the revision > >> + in their parents, all the way up the hierarchy. This is >> expensive to >> + implement in U-Boot before relocation, so this function explicitly > > I think the point here is "at runtime" which is missing. > Noted. >> + propagates these bootph properties upwards. > > at build time :) > Noted. >> + >> + This is used to set the bootph-* property in the parent node if a >> + "bootph-*" property is found in any of the parent's subnodes. >> + > > This is all misleading, we aren't propagating all bootph properties > >> + Args: >> + node (fdt.Node): Node to propagate the property to > > Same remarks as for the propagate_bootph method above. > >> + prop (str): Property name to propagate >> + >> + Return: >> + True if any change was made, else False >> + >> + """ >> + bootph_prop = ['bootph-all', 'bootph-some-ram'] >> + > > Please explain why those only. Answered above. > >> + changed = False >> + for prop in bootph_prop: >> + if prop in node.props: >> + changed |= propagate_bootph(node.parent, prop) >> + >> + for subnode in node.subnodes: >> + changed |= scan_and_prop_bootph(subnode) >> + return changed >> + > > I have a gut feeling this is suboptimal, though simple and easy to > maintain. > We currently go up to the root node for each and every found > bootph-{all,some-ram} property. We could simply divide this by half > (or more if there are more properties to propagate in the future) by > passing the props to propagate instead of just one. We can keep track > of which node has propagated the property already and, for its > children, stop the propagation at that level instead of going up to > the root node all the time. I'm not sure typical Device Trees are deep > enough for us to care about that kind of optimization for now, > especially since it'll be running at build time only? > > passing the props to propagate instead of just one Since we are only concerned with the post SPL stage with this patch since the SPL stages are handled by fdtgrep, I am not sure if adding this optimization would result in gains assuming that bootph-all is the superset of all bootph-* properties(this patch concerns only with some-ram and all) and adding anything with bootph-all in the same node will be redundant? > stop the propagation at that level instead of going up to the root node all the time This optimization will surely result in gains of about few microseconds I would assume, since we are running this on host PC. I am not sure if we require such optimizations in build time. It would be great if I can get some more opinions on this. I am not able to profile the time delta in build time with this patch since every build is having differences of few seconds between them on the same tree. I tried using pythons cprofiler on the function[0], didn't find any timing overheads as expected since we are running things on host PC which is way faster than the SoC's. [1]https://gist.github.com/Jamm02/6ce3e4eb1fd2442ad5e01ff89ead6e6e Regards, Moteen > Cheers, > Quentin
Hi Moteen, On 5/14/25 8:05 AM, Moteen Shah wrote: > Hey Quentin, > Thanks for the review. > > On 13/05/25 16:10, Quentin Schulz wrote: >> Hi Moteen, >> >> On 5/12/25 1:50 PM, Moteen Shah wrote: >>> Add a function to scan through all the nodes in the device-tree >>> for bootph-* property. If found, propagate it to all of its parent >> >> It's not scanning for bootph-* properties. It's scanning for two >> specific bootph properties, so try to update the commit log and >> comments so it's not misleading. >> > > I will update the commit message in the revision. > >> The commit log is lacking information as to why we need to do this, >> what this helps with and why this is specific to bootph-all and >> bootph-some-ram. > > > Why this is specific to bootph-all and bootph-some-ram > > The other ones are handled by fdtgrep as those are for the SPL stage. > Yup, the issue is that the non-proper stages do not actually read the bootph nodes as they are stripped by fdtgrep. The DTB for the stage is of everything relevant to the stage, without the bootph nodes. This is done to save precious space (instead of having a full DTB with appropriate bootph nodes, we only keep the bootph nodes and we strip the bootph properties from them as they are now implied). However, the proper stage (and specifically the pre-relocation part of the proper stage) uses the full DTB, with bootph properties. So we need to propagate them so that pre-relocation works as expected since it'll check whether bootph-all/bootph-some-ram property is part of the node before probing the device. [...] >>> + Args: >>> + node (fdt.Node): Node to propagate the property to >> >> Specify that all its parents (recursively) will have this property >> propagated to. > > It is iterative not recursive. How about: "Node and all its parent nodes > above it to propagate the property to iteratively" > Mmmm, true, I had in mind that recursive also works for defining "parent of parent" and "parent of parent of parent", etc, but it really seems the term is specific to recursion, which we don't use here. Can suggest something like: Node to propagate the property to. Its parents (and their parents, and so on until the root node) are also propagated the property. [...] >>> + changed = False >>> + for prop in bootph_prop: >>> + if prop in node.props: >>> + changed |= propagate_bootph(node.parent, prop) >>> + >>> + for subnode in node.subnodes: >>> + changed |= scan_and_prop_bootph(subnode) >>> + return changed >>> + >> >> I have a gut feeling this is suboptimal, though simple and easy to >> maintain. >> We currently go up to the root node for each and every found bootph- >> {all,some-ram} property. We could simply divide this by half (or more >> if there are more properties to propagate in the future) by passing >> the props to propagate instead of just one. We can keep track of which >> node has propagated the property already and, for its children, stop >> the propagation at that level instead of going up to the root node all >> the time. I'm not sure typical Device Trees are deep enough for us to >> care about that kind of optimization for now, especially since it'll >> be running at build time only? >> > > > passing the props to propagate instead of just one > > Since we are only concerned with the post SPL stage with this patch > since the SPL stages are handled by fdtgrep, I am not sure if adding > this optimization would result in gains assuming that bootph-all is the > superset of all bootph-* properties(this patch concerns only with some- > ram and all) and adding anything with bootph-all in the same node will > be redundant? > Something like: (not tested) def propagate_bootph(node, props): changed = False while node: to_write = props - set(node.props) for prop in to_write: node.AddEmptyProp(prop, 0) changed = True node = node.parent return changed def scan_and_prop_bootph(node): bootph_prop = {'bootph-all', 'bootph-some-ram'} changed = False to_propagate = bootph_prop & set(node.props) if to_propagate: changed |= propagate_bootph(node.parent, to_propagate) [...] > > > stop the propagation at that level instead of going up to the root > node all the time > > This optimization will surely result in gains of about few microseconds > I would assume, since we are running this on host PC. > I am not sure if we require such optimizations in build time. It would require? No :) We can optimize later on if it really becomes annoying for build time but I'm sure there are other places in code where optimizing would be more beneficial :) So no worries about the optimization ;) (also not sure it is necessarily more optimized what I'm suggesting, just a gut feeling). Cheers, Quentin
Hey Quentin, On 14/05/25 21:42, Quentin Schulz wrote: > Hi Moteen, > > On 5/14/25 8:05 AM, Moteen Shah wrote: >> Hey Quentin, >> Thanks for the review. >> >> On 13/05/25 16:10, Quentin Schulz wrote: >>> Hi Moteen, >>> >>> On 5/12/25 1:50 PM, Moteen Shah wrote: >>>> Add a function to scan through all the nodes in the device-tree >>>> for bootph-* property. If found, propagate it to all of its parent >>> >>> It's not scanning for bootph-* properties. It's scanning for two >>> specific bootph properties, so try to update the commit log and >>> comments so it's not misleading. >>> >> >> I will update the commit message in the revision. >> >>> The commit log is lacking information as to why we need to do this, >>> what this helps with and why this is specific to bootph-all and >>> bootph-some-ram. >> >> > Why this is specific to bootph-all and bootph-some-ram >> >> The other ones are handled by fdtgrep as those are for the SPL stage. >> > > Yup, the issue is that the non-proper stages do not actually read the > bootph nodes as they are stripped by fdtgrep. The DTB for the stage is > of everything relevant to the stage, without the bootph nodes. This is > done to save precious space (instead of having a full DTB with > appropriate bootph nodes, we only keep the bootph nodes and we strip > the bootph properties from them as they are now implied). However, the > proper stage (and specifically the pre-relocation part of the proper > stage) uses the full DTB, with bootph properties. So we need to > propagate them so that pre-relocation works as expected since it'll > check whether bootph-all/bootph-some-ram property is part of the node > before probing the device. > > [...] > >>>> + Args: >>>> + node (fdt.Node): Node to propagate the property to >>> >>> Specify that all its parents (recursively) will have this property >>> propagated to. >> >> It is iterative not recursive. How about: "Node and all its parent >> nodes above it to propagate the property to iteratively" >> > > Mmmm, true, I had in mind that recursive also works for defining > "parent of parent" and "parent of parent of parent", etc, but it > really seems the term is specific to recursion, which we don't use here. > > Can suggest something like: > > Node to propagate the property to. Its parents (and their parents, and > so on until the root node) are also propagated the property. > > [...] > >>>> + changed = False >>>> + for prop in bootph_prop: >>>> + if prop in node.props: >>>> + changed |= propagate_bootph(node.parent, prop) >>>> + >>>> + for subnode in node.subnodes: >>>> + changed |= scan_and_prop_bootph(subnode) >>>> + return changed >>>> + >>> >>> I have a gut feeling this is suboptimal, though simple and easy to >>> maintain. >>> We currently go up to the root node for each and every found bootph- >>> {all,some-ram} property. We could simply divide this by half (or >>> more if there are more properties to propagate in the future) by >>> passing the props to propagate instead of just one. We can keep >>> track of which node has propagated the property already and, for its >>> children, stop the propagation at that level instead of going up to >>> the root node all the time. I'm not sure typical Device Trees are >>> deep enough for us to care about that kind of optimization for now, >>> especially since it'll be running at build time only? >>> >> >> > passing the props to propagate instead of just one >> >> Since we are only concerned with the post SPL stage with this patch >> since the SPL stages are handled by fdtgrep, I am not sure if adding >> this optimization would result in gains assuming that bootph-all is >> the superset of all bootph-* properties(this patch concerns only with >> some- ram and all) and adding anything with bootph-all in the same >> node will be redundant? >> > > Something like: (not tested) > > def propagate_bootph(node, props): > changed = False > while node: > to_write = props - set(node.props) > for prop in to_write: > node.AddEmptyProp(prop, 0) > changed = True > node = node.parent > return changed > > def scan_and_prop_bootph(node): > bootph_prop = {'bootph-all', 'bootph-some-ram'} > > changed = False > to_propagate = bootph_prop & set(node.props) > if to_propagate: > changed |= propagate_bootph(node.parent, to_propagate) > [...] Thanks for the pseudo code, question on this again, can a node have bootph-all and bootph-some-ram together which we would require to propagate together. In my understanding having both of these property in the same node present will be a redundancy, and searching in current tree as well, I found no such examples of both the properties being used in the same node. If in future we are planning to add more bootph* properties in the proper stage(pre-relocation) this change does makes sense. Let me know if I should add this in my next revision. Regards, Moteen > >> >> > stop the propagation at that level instead of going up to the root >> node all the time >> >> This optimization will surely result in gains of about few >> microseconds I would assume, since we are running this on host PC. >> I am not sure if we require such optimizations in build time. It would > > require? No :) > > We can optimize later on if it really becomes annoying for build time > but I'm sure there are other places in code where optimizing would be > more beneficial :) > > So no worries about the optimization ;) (also not sure it is > necessarily more optimized what I'm suggesting, just a gut feeling). > > Cheers, > Quentin
Hi Moteen, On 5/15/25 11:01 AM, Moteen Shah wrote: > Hey Quentin, > > On 14/05/25 21:42, Quentin Schulz wrote: >> Hi Moteen, >> >> On 5/14/25 8:05 AM, Moteen Shah wrote: >>> Hey Quentin, >>> Thanks for the review. >>> >>> On 13/05/25 16:10, Quentin Schulz wrote: >>>> Hi Moteen, >>>> >>>> On 5/12/25 1:50 PM, Moteen Shah wrote: >>>>> Add a function to scan through all the nodes in the device-tree >>>>> for bootph-* property. If found, propagate it to all of its parent >>>> >>>> It's not scanning for bootph-* properties. It's scanning for two >>>> specific bootph properties, so try to update the commit log and >>>> comments so it's not misleading. >>>> >>> >>> I will update the commit message in the revision. >>> >>>> The commit log is lacking information as to why we need to do this, >>>> what this helps with and why this is specific to bootph-all and >>>> bootph-some-ram. >>> >>> > Why this is specific to bootph-all and bootph-some-ram >>> >>> The other ones are handled by fdtgrep as those are for the SPL stage. >>> >> >> Yup, the issue is that the non-proper stages do not actually read the >> bootph nodes as they are stripped by fdtgrep. The DTB for the stage is >> of everything relevant to the stage, without the bootph nodes. This is >> done to save precious space (instead of having a full DTB with >> appropriate bootph nodes, we only keep the bootph nodes and we strip >> the bootph properties from them as they are now implied). However, the >> proper stage (and specifically the pre-relocation part of the proper >> stage) uses the full DTB, with bootph properties. So we need to >> propagate them so that pre-relocation works as expected since it'll >> check whether bootph-all/bootph-some-ram property is part of the node >> before probing the device. >> >> [...] >> >>>>> + Args: >>>>> + node (fdt.Node): Node to propagate the property to >>>> >>>> Specify that all its parents (recursively) will have this property >>>> propagated to. >>> >>> It is iterative not recursive. How about: "Node and all its parent >>> nodes above it to propagate the property to iteratively" >>> >> >> Mmmm, true, I had in mind that recursive also works for defining >> "parent of parent" and "parent of parent of parent", etc, but it >> really seems the term is specific to recursion, which we don't use here. >> >> Can suggest something like: >> >> Node to propagate the property to. Its parents (and their parents, and >> so on until the root node) are also propagated the property. >> >> [...] >> >>>>> + changed = False >>>>> + for prop in bootph_prop: >>>>> + if prop in node.props: >>>>> + changed |= propagate_bootph(node.parent, prop) >>>>> + >>>>> + for subnode in node.subnodes: >>>>> + changed |= scan_and_prop_bootph(subnode) >>>>> + return changed >>>>> + >>>> >>>> I have a gut feeling this is suboptimal, though simple and easy to >>>> maintain. >>>> We currently go up to the root node for each and every found bootph- >>>> {all,some-ram} property. We could simply divide this by half (or >>>> more if there are more properties to propagate in the future) by >>>> passing the props to propagate instead of just one. We can keep >>>> track of which node has propagated the property already and, for its >>>> children, stop the propagation at that level instead of going up to >>>> the root node all the time. I'm not sure typical Device Trees are >>>> deep enough for us to care about that kind of optimization for now, >>>> especially since it'll be running at build time only? >>>> >>> >>> > passing the props to propagate instead of just one >>> >>> Since we are only concerned with the post SPL stage with this patch >>> since the SPL stages are handled by fdtgrep, I am not sure if adding >>> this optimization would result in gains assuming that bootph-all is >>> the superset of all bootph-* properties(this patch concerns only with >>> some- ram and all) and adding anything with bootph-all in the same >>> node will be redundant? >>> >> >> Something like: (not tested) >> >> def propagate_bootph(node, props): >> changed = False >> while node: >> to_write = props - set(node.props) >> for prop in to_write: >> node.AddEmptyProp(prop, 0) >> changed = True >> node = node.parent >> return changed >> >> def scan_and_prop_bootph(node): >> bootph_prop = {'bootph-all', 'bootph-some-ram'} >> >> changed = False >> to_propagate = bootph_prop & set(node.props) >> if to_propagate: >> changed |= propagate_bootph(node.parent, to_propagate) >> [...] > > Thanks for the pseudo code, question on this again, can a node have > bootph-all and bootph-some-ram together which we would require to > propagate together. In my understanding having both of these property in > the same node present will be a redundancy, and searching in current > tree as well, I found no such examples of both the properties being used > in the same node. If in future we are planning to add more bootph* > properties in the proper stage(pre-relocation) this change does makes By definition, bootph-all covers all stages so must cover bootph-some-ram. It doesn't make sense to have both bootph-some-ram and bootph-all, but I guess it could happen. That could also be another optimization, only look for bootph-some-ram if bootph-all wasn't found, otherwise simply propagate bootph-all and skip propagating bootph-some-ram for this node. > sense. Let me know if I should add this in my next revision. > I think your implementation is fine albeit *maybe* not optimized. But this is a host tool and I don't think that small optimization is going to matter. So up to you, whatever you are more comfortable with! Cheers, Quentin
diff --git a/tools/binman/control.py b/tools/binman/control.py index 81f61e3e152..ccf081cb686 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -530,6 +530,54 @@ def _RemoveTemplates(parent): for node in del_nodes: node.Delete() +def propagate_bootph(node, prop): + """Propagate bootph-* property to all the parent nodes up the hierarchy + + Args: + node (fdt.Node): Node to propagate the property to + prop (str): Property to propagate + + Return: + True if any change was made, else False + """ + changed = False + while node: + if prop not in node.props: + node.AddEmptyProp(prop, 0) + changed = True + node = node.parent + return changed + +def scan_and_prop_bootph(node): + """Propagate bootph properties from children to parents + + The bootph schema indicates that bootph nodes in children should be implied + in their parents, all the way up the hierarchy. This is expensive to + implement in U-Boot before relocation, so this function explicitly + propagates these bootph properties upwards. + + This is used to set the bootph-* property in the parent node if a + "bootph-*" property is found in any of the parent's subnodes. + + Args: + node (fdt.Node): Node to propagate the property to + prop (str): Property name to propagate + + Return: + True if any change was made, else False + + """ + bootph_prop = ['bootph-all', 'bootph-some-ram'] + + changed = False + for prop in bootph_prop: + if prop in node.props: + changed |= propagate_bootph(node.parent, prop) + + for subnode in node.subnodes: + changed |= scan_and_prop_bootph(subnode) + return changed + def PrepareImagesAndDtbs(dtb_fname, select_images, update_fdt, use_expanded, indir): """Prepare the images to be processed and select the device tree @@ -589,6 +637,9 @@ def PrepareImagesAndDtbs(dtb_fname, select_images, update_fdt, use_expanded, ind fname = tools.get_output_filename('u-boot.dtb.tmpl2') tools.write_file(fname, dtb.GetContents()) + if scan_and_prop_bootph(dtb.GetRoot()): + dtb.Sync(True) + images = _ReadImageDesc(node, use_expanded) if select_images: