[PATCH 3/3] wip: binman: Move processing after templates

[Moteen Shah]

Hey Simon,
A gentle ping on this.

Regards,
Moteen

On 28/04/25 16:53, Moteen Shah wrote:
> Hey Simon,
> Thanks for the suggestions and review.
>
> On 23/04/25 17:59, Simon Glass wrote:
>> Do the bootph processing after templates are handled, since templates
>> can change the tree structure quite a bit.
>>
>> Also avoid changing the u-boot.dtb file since that is an input to Binman
>>
>> Add some other suggested code tweaks:
>> - Only sync the dtb if something changed
>> - add more comments
>>
>> Note: This patch is just for illustration; please just combine it into
>> your existing work.
>>
>> Signed-off-by: Simon Glass <sjg at chromium.org>
>> ---
>>
>>   tools/binman/control.py | 70 +++++++++++++++++++++++++++--------------
>>   1 file changed, 46 insertions(+), 24 deletions(-)
>>
>> diff --git a/tools/binman/control.py b/tools/binman/control.py
>> index b703bdf482e..6b7c5507513 100644
>> --- a/tools/binman/control.py
>> +++ b/tools/binman/control.py
>> @@ -530,34 +530,55 @@ def _RemoveTemplates(parent):
>>       for node in del_nodes:
>>           node.Delete()
>>   -def prop_bootph_to_parent(node, prop):
>> -    """Propagates bootph-* property to all the parent
>> -    nodes up the hierarchy
>> -    """
>> -    parent = node.parent
>> -    if parent == None or parent.props.get(prop):
>> -       return
>> +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
>>   -    while parent:
>> -        parent.AddEmptyProp(prop, 0)
>> -        parent = parent.parent
>> +    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):
>> -    """Scan the device tree and set the bootph-* property if its 
>> present
>> -    in subnode
>> +    """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 subnodes of the parent
>> -    node.
>> +    "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', 
>> 'bootph-pre-ram', 'bootph-pre-sram']
>> +    bootph_prop = ['bootph-all', 'bootph-some-ram', 'bootph-pre-ram',
>> +                   'bootph-pre-sram']
>>   -    for props in bootph_prop:
>> -        if node.props.get(props):
>> -            prop_bootph_to_parent(node, props)
>> +    changed = False
>> +    for prop in bootph_prop:
>> +        if prop in node.props:
>> +            changed |= propagate_bootph(node.parent, prop)
>>         for subnode in node.subnodes:
>> -       scan_and_prop_bootph(subnode)
>> +        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
>> @@ -573,7 +594,7 @@ def PrepareImagesAndDtbs(dtb_fname, 
>> select_images, update_fdt, use_expanded, ind
>>           dtb_fname: Filename of the device tree file to use (.dts or 
>> .dtb)
>>           selected_images: List of images to output, or None for all
>>           update_fdt: True to update the FDT wth entry offsets, etc.
>> -        use_expanded: True to use expanded versions of entries, if 
>> available.
>> +        use_expanded: True to use expanded versions osf entries, if 
>> available.
>>               So if 'u-boot' is called for, we use 'u-boot-expanded' 
>> instead. This
>>               is needed if update_fdt is True (although tests may 
>> disable it)
>>           indir: List of directories where input files can be found
>> @@ -596,10 +617,8 @@ def PrepareImagesAndDtbs(dtb_fname, 
>> select_images, update_fdt, use_expanded, ind
>>           indir = []
>>       dtb_fname = fdt_util.EnsureCompiled(dtb_fname, indir=indir)
>>       fname = tools.get_output_filename('u-boot.dtb.out')
>> -    dtb = fdt.FdtScan(dtb_fname)
>> -    scan_and_prop_bootph(dtb.GetRoot())
>> -    dtb.Sync(True)
>> -    tools.write_file(dtb_fname, dtb.GetContents())
>> +    tools.write_file(fname, tools.read_file(dtb_fname))
>> +    dtb = fdt.FdtScan(fname)
>>         node = _FindBinmanNode(dtb)
>>       if not node:
>> @@ -620,6 +639,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)
>> +
>
> I applied the suggested changes, u-boot.dtb.out does shows the 
> bootph-* being propagated on reverse compiling, but
> on reverse compiling u-boot.dtb I cannot see the same behavior. I 
> think we might have to make an explicit call to sync
> the two of them somehow because the dtb.Sync() here as per my 
> understanding is syncing the changes done back to
> u-boot.dtb.out.
>
> Any pointers on how I can make the changes reflect in u-boot.dtb 
> without doing this:
>
> tools.write_file(dtb_fname, dtb.GetContents())
>
>
> Regards,
> Moteen
>>       images = _ReadImageDesc(node, use_expanded)
>>         if select_images: