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

[Moteen Shah]

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: