Hey Quentin,
On 14/05/25 21:42, Quentin Schulz wrote:
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)
[...]
Thanks for the pseudo code, question on this again, can a node have
bootph-all and bootph-some-ram together which we would require to
propagate together. In my understanding having both of these property in
the same node present will be a redundancy, and searching in current
tree as well, I found no such examples of both the properties being used
in the same node. If in future we are planning to add more bootph*
properties in the proper stage(pre-relocation) this change does makes
sense. Let me know if I should add this in my next revision.
Regards,
Moteen
> 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
