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

[Simon Glass]

Hi Moteen,

On Fri, 9 May 2025 at 08:10, Moteen Shah <m-shah at ti.com> wrote:
>
> 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())

But why do you need to do that? It is the 'output' dtb which should be
packaged in the image. The 'u-boot.dtb' file is an input to binman,
not an output from it..

> >
> >
> > Regards,
> > Moteen
> >>       images = _ReadImageDesc(node, use_expanded)
> >>         if select_images:

Regards,
Simon