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())


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

Reply via email to