Hi Moteen,
On Wed, 2 Apr 2025 at 22:01, Moteen Shah <m-s...@ti.com> wrote:
>
> Hey Simon,
>
> On 29/03/25 05:17, Simon Glass wrote:
> > Hi Moteen,
> >
> > On Thu, 27 Mar 2025 at 08:06, Moteen Shah <m-s...@ti.com> wrote:
> >> Add a function to scan through all the nodes in the device-tree
> >> recusively for bootph-* property. If found, propagate it to all
> >> of its parent nodes up the hierarchy.
> >>
> >> Signed-off-by: Moteen Shah <m-s...@ti.com>
> >> ---
> >> tools/binman/control.py | 35 ++++++++++++++++++++++++++++++++++-
> >> 1 file changed, 34 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/tools/binman/control.py b/tools/binman/control.py
> >> index e73c598298c..e739949d366 100644
> >> --- a/tools/binman/control.py
> >> +++ b/tools/binman/control.py
> >> @@ -526,6 +526,35 @@ def _RemoveTemplates(parent):
> >> if node.name.startswith('template'):
> >> node.Delete()
> >>
> >> +def prop_bootph_to_parent(node, prop, dtb):
> > I think the 'prop_' is a bit confusing, since you are dealing with
> > properties! How about 'add_' as the prefix?
>
> 'add_' should be more descriptive, will add that in v3.
>
> >
> >> + """Propagates bootph-* property to all the parent
> >> + nodes up the hierarchy
> > First line should be a summary
> >
> > then blank line
> >
> > then describe the args...you can see this if you look at a few other
> > functions.
>
>
> Noted, will rectify in v3.
>
> >> + """
> >> + parent = node.parent
> >> + if parent == None or parent.props.get(prop):
> > if not parent or ...
>
>
> Noted.
>
>
> >
> >> + return
> >> +
> >> + while parent:
> >> + parent.AddEmptyProp(prop, 0)
> >> + parent = parent.parent
> >> +
> >> +def scan_and_prop_bootph(node, dtb):
> >> + """Scan the device tree and set the bootph-* property if its present
> >> + in subnode
> >> +
> >> + 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.
> > comment style again
> >
> >> + """
> >> + bootph_prop = ['bootph-all', 'bootph-some-ram', 'bootph-pre-ram',
> >> 'bootph-pre-sram']
> > From my understanding the only ones that matter are bootph-all and
> > bootph-some-ram, since the SPL ones are handled by fdtgrep.
>
>
> Noted.
>
>
> >
> >> +
> >> + for props in bootph_prop:
> > for prop
> >
> > (since it is just one)
> >
> >> + if node.props.get(props):
> >> + prop_bootph_to_parent(node, props, dtb)
> >> +
> >> + for subnode in node.subnodes:
> >> + scan_and_prop_bootph(subnode, dtb)
> >> +
> >> def PrepareImagesAndDtbs(dtb_fname, select_images, update_fdt,
> >> use_expanded, indir):
> >> """Prepare the images to be processed and select the device tree
> >>
> >> @@ -563,7 +592,11 @@ 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')
> >> - tools.write_file(fname, tools.read_file(dtb_fname))
> >> + dtb = fdt.FdtScan(dtb_fname)
> >> + scan_and_prop_bootph(dtb.GetRoot(), dtb)
> >> + dtb.Sync(True)
> >> + tools.write_file(dtb_fname, dtb.GetContents())
> >> + tools.write_file(fname, dtb.GetContents())
> > You shouldn't write back to the file created by the build. Do you need this?
>
>
> Had the same thought, but the build fails in non clean builds[0]. Did a
> workaround[1] but then some essential template nodes ends up getting
> deleted from u-boot.dtb. Finally, had to write back to the file
> to resolve the issue.
Can you push a tree somewhere. This could be a bug that I could fix.
>
> >> dtb = fdt.FdtScan(fname)
> >>
> >> node = _FindBinmanNode(dtb)
> >> --
> >> 2.34.1
> >>
> > The code looks fine to me apart from the nits.
> >
> > This addition needs a test - see ftest.py for some examples there. But
> > basically just create a dts that has some props in it, then check that
> > they got added.
>
>
> Included a test as well in the patchset[3]. Do let me know if there are
> some changes required in it.
Somehow I'm not seeing that in patchwork. It looks good but please try
to keep lines <=80 cols.
>
> >
> > I think testSimpleFitEncryptedData() could be a good example?
> >
> > Regards,
> > Simon
>
> References:
> [0]
> https://gist.github.com/Jamm02/2478a4f1186f3ba4a8cd7dbf1fb11e2f#file-non-clean-build
> [1]
> https://gist.github.com/Jamm02/2478a4f1186f3ba4a8cd7dbf1fb11e2f#file-patch-tools-dtdoc-fdt-py
> [3] https://lore.kernel.org/u-boot/20250327080642.2269856-3-m-s...@ti.com/
>
> Thanks for the review.
Regards,
Simon
