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

[Moteen Shah]

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