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

[Moteen Shah]

Hello Simon,

On 10/05/25 16:54, Simon Glass wrote:
> 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..

We are packaging the u-boot.dtb for K3 devices because of which,
I was not seeing the bootph-* being propagated.
I'll send this patch then, and send a follow up patch for K3 devices to
package u-boot.dtb.out instead.

Thanks and regards,
Moteen

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