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.

      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.


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,
Moteen

Reply via email to