[PATCH 1/2] tools: binman: control.py: Propagate bootph-* properties to supernodes

[Moteen Shah]

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 at ti.com>
>> Signed-off-by: Simon Glass <sjg at 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