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

[Quentin Schulz]

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