Hey Quentin,
Thanks for the review.
On 13/05/25 16:10, Quentin Schulz wrote:
Hi Moteen,
On 5/12/25 1:50 PM, Moteen Shah wrote:
Add a function to scan through all the nodes in the device-tree
for bootph-* property. If found, propagate it to all of its parent
It's not scanning for bootph-* properties. It's scanning for two
specific bootph properties, so try to update the commit log and
comments so it's not misleading.
I will update the commit message in the revision.
The commit log is lacking information as to why we need to do this,
what this helps with and why this is specific to bootph-all and
bootph-some-ram.
> Why this is specific to bootph-all and bootph-some-ram
The other ones are handled by fdtgrep as those are for the SPL stage.
We need to be able to look at this patch in 5 years and have a clue of
why this was required.
Noted.
nodes up the hierarchy.
Signed-off-by: Moteen Shah <m-s...@ti.com>
Signed-off-by: Simon Glass <s...@chromium.org>
---
tools/binman/control.py | 51 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 51 insertions(+)
diff --git a/tools/binman/control.py b/tools/binman/control.py
index 81f61e3e152..ccf081cb686 100644
--- a/tools/binman/control.py
+++ b/tools/binman/control.py
@@ -530,6 +530,54 @@ def _RemoveTemplates(parent):
for node in del_nodes:
node.Delete()
+def propagate_bootph(node, prop):
+ """Propagate bootph-* property to all the parent nodes up the
hierarchy
+
Misleading, it's not propagating bootph properties, it's propagating
the prop passed as an argument to the function, so rename to something
like propagate_prop and update the comment.
Okay, will rename this to propagate_prop.
Be clear also that this is expected to be a boolean property (because
of AddEmptyProp).
Noted.
+ Args:
+ node (fdt.Node): Node to propagate the property to
Specify that all its parents (recursively) will have this property
propagated to.
It is iterative not recursive. How about: "Node and all its parent nodes
above it to propagate the property to iteratively"
+ prop (str): Property to propagate
+
+ 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):
+ """Propagate bootph properties from children to parents
+
+ The bootph schema indicates that bootph nodes in children should
be implied
s/nodes/properties/
Will change in the revision
+ in their parents, all the way up the hierarchy. This is
expensive to
+ implement in U-Boot before relocation, so this function explicitly
I think the point here is "at runtime" which is missing.
Noted.
+ propagates these bootph properties upwards.
at build time :)
Noted.
+
+ This is used to set the bootph-* property in the parent node if a
+ "bootph-*" property is found in any of the parent's subnodes.
+
This is all misleading, we aren't propagating all bootph properties
+ Args:
+ node (fdt.Node): Node to propagate the property to
Same remarks as for the propagate_bootph method above.
+ prop (str): Property name to propagate
+
+ Return:
+ True if any change was made, else False
+
+ """
+ bootph_prop = ['bootph-all', 'bootph-some-ram']
+
Please explain why those only.
Answered above.
+ changed = False
+ for prop in bootph_prop:
+ if prop in node.props:
+ changed |= propagate_bootph(node.parent, prop)
+
+ for subnode in node.subnodes:
+ changed |= scan_and_prop_bootph(subnode)
+ return changed
+
I have a gut feeling this is suboptimal, though simple and easy to
maintain.
We currently go up to the root node for each and every found
bootph-{all,some-ram} property. We could simply divide this by half
(or more if there are more properties to propagate in the future) by
passing the props to propagate instead of just one. We can keep track
of which node has propagated the property already and, for its
children, stop the propagation at that level instead of going up to
the root node all the time. I'm not sure typical Device Trees are deep
enough for us to care about that kind of optimization for now,
especially since it'll be running at build time only?
> passing the props to propagate instead of just one
Since we are only concerned with the post SPL stage with this patch
since the SPL stages are handled by fdtgrep, I am not sure if adding
this optimization would result in gains assuming that bootph-all is the
superset of all bootph-* properties(this patch concerns only with
some-ram and all) and adding anything with bootph-all in the same node
will be redundant?
> stop the propagation at that level instead of going up to the root
node all the time
This optimization will surely result in gains of about few microseconds
I would assume, since we are running this on host PC.
I am not sure if we require such optimizations in build time. It would
be great if I can get some more opinions on this. I am not able to
profile the time delta in build time with this patch since every build
is having differences of few seconds between them on the same tree. I
tried using pythons cprofiler on the function[0], didn't find any timing
overheads as expected since we are running things on host PC which is
way faster than the SoC's.
[1]https://gist.github.com/Jamm02/6ce3e4eb1fd2442ad5e01ff89ead6e6e
Regards,
Moteen
Cheers,
Quentin
