Hello Simon,

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

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

Reply via email to