Hi Moteen,
On 5/14/25 8:05 AM, Moteen Shah wrote:
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.
Yup, the issue is that the non-proper stages do not actually read the
bootph nodes as they are stripped by fdtgrep. The DTB for the stage is
of everything relevant to the stage, without the bootph nodes. This is
done to save precious space (instead of having a full DTB with
appropriate bootph nodes, we only keep the bootph nodes and we strip the
bootph properties from them as they are now implied). However, the
proper stage (and specifically the pre-relocation part of the proper
stage) uses the full DTB, with bootph properties. So we need to
propagate them so that pre-relocation works as expected since it'll
check whether bootph-all/bootph-some-ram property is part of the node
before probing the device.
[...]
+ 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"
Mmmm, true, I had in mind that recursive also works for defining "parent
of parent" and "parent of parent of parent", etc, but it really seems
the term is specific to recursion, which we don't use here.
Can suggest something like:
Node to propagate the property to. Its parents (and their parents, and
so on until the root node) are also propagated the property.
[...]
+ 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?
Something like: (not tested)
def propagate_bootph(node, props):
changed = False
while node:
to_write = props - set(node.props)
for prop in to_write:
node.AddEmptyProp(prop, 0)
changed = True
node = node.parent
return changed
def scan_and_prop_bootph(node):
bootph_prop = {'bootph-all', 'bootph-some-ram'}
changed = False
to_propagate = bootph_prop & set(node.props)
if to_propagate:
changed |= propagate_bootph(node.parent, to_propagate)
[...]
> 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
require? No :)
We can optimize later on if it really becomes annoying for build time
but I'm sure there are other places in code where optimizing would be
more beneficial :)
So no worries about the optimization ;) (also not sure it is necessarily
more optimized what I'm suggesting, just a gut feeling).
Cheers,
Quentin
