Hi Moteen,
On Fri, 9 May 2025 at 08:10, Moteen Shah <m-s...@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 <s...@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
