[RFC PATCH 1/2 v2] tools: binman: control.py: Propagate bootph-* properties to supernodes

[Moteen Shah]

Hey Simon,

Is the problem discussed in the thread an actual bug or am I missing 
something in the implementation?

Regards,
Moteen

On 03/04/25 11:51, Moteen Shah wrote:
> Hey Simon,
>
> On 03/04/25 00:52, Simon Glass wrote:
>> Hi Moteen,
>>
>> On Wed, 2 Apr 2025 at 22:01, Moteen Shah <m-shah at ti.com> wrote:
>>> Hey Simon,
>>>
>>> On 29/03/25 05:17, Simon Glass wrote:
>>>> Hi Moteen,
>>>>
>>>> On Thu, 27 Mar 2025 at 08:06, Moteen Shah <m-shah at ti.com> wrote:
>>>>> Add a function to scan through all the nodes in the device-tree
>>>>> recusively for bootph-* property. If found, propagate it to all
>>>>> of its parent nodes up the hierarchy.
>>>>>
>>>>> Signed-off-by: Moteen Shah <m-shah at ti.com>
>>>>> ---
>>>>>    tools/binman/control.py | 35 ++++++++++++++++++++++++++++++++++-
>>>>>    1 file changed, 34 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/tools/binman/control.py b/tools/binman/control.py
>>>>> index e73c598298c..e739949d366 100644
>>>>> --- a/tools/binman/control.py
>>>>> +++ b/tools/binman/control.py
>>>>> @@ -526,6 +526,35 @@ def _RemoveTemplates(parent):
>>>>>            if node.name.startswith('template'):
>>>>>                node.Delete()
>>>>>
>>>>> +def prop_bootph_to_parent(node, prop, dtb):
>>>> I think the 'prop_' is a bit confusing, since you are dealing with
>>>> properties! How about 'add_' as the prefix?
>>> 'add_' should be more descriptive, will add that in v3.
>>>
>>>>> +    """Propagates bootph-* property to all the parent
>>>>> +    nodes up the hierarchy
>>>> First line should be a summary
>>>>
>>>> then blank line
>>>>
>>>> then describe the args...you can see this if you look at a few 
>>>> other functions.
>>>
>>> Noted, will rectify in v3.
>>>
>>>>> +    """
>>>>> +    parent = node.parent
>>>>> +    if parent == None or parent.props.get(prop):
>>>> if not parent or ...
>>>
>>> Noted.
>>>
>>>
>>>>> +       return
>>>>> +
>>>>> +    while parent:
>>>>> +        parent.AddEmptyProp(prop, 0)
>>>>> +        parent = parent.parent
>>>>> +
>>>>> +def scan_and_prop_bootph(node, dtb):
>>>>> +    """Scan the device tree and set the bootph-* property if its 
>>>>> present
>>>>> +    in subnode
>>>>> +
>>>>> +    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.
>>>> comment style again
>>>>
>>>>> +    """
>>>>> +    bootph_prop = ['bootph-all', 'bootph-some-ram', 
>>>>> 'bootph-pre-ram', 'bootph-pre-sram']
>>>>   From my understanding the only ones that matter are bootph-all and
>>>> bootph-some-ram, since the SPL ones are handled by fdtgrep.
>>>
>>> Noted.
>>>
>>>
>>>>> +
>>>>> +    for props in bootph_prop:
>>>> for prop
>>>>
>>>> (since it is just one)
>>>>
>>>>> +        if node.props.get(props):
>>>>> +            prop_bootph_to_parent(node, props, dtb)
>>>>> +
>>>>> +    for subnode in node.subnodes:
>>>>> +       scan_and_prop_bootph(subnode, dtb)
>>>>> +
>>>>>    def PrepareImagesAndDtbs(dtb_fname, select_images, update_fdt, 
>>>>> use_expanded, indir):
>>>>>        """Prepare the images to be processed and select the device 
>>>>> tree
>>>>>
>>>>> @@ -563,7 +592,11 @@ 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')
>>>>> -    tools.write_file(fname, tools.read_file(dtb_fname))
>>>>> +    dtb = fdt.FdtScan(dtb_fname)
>>>>> +    scan_and_prop_bootph(dtb.GetRoot(), dtb)
>>>>> +    dtb.Sync(True)
>>>>> +    tools.write_file(dtb_fname, dtb.GetContents())
>>>>> +    tools.write_file(fname, dtb.GetContents())
>>>> You shouldn't write back to the file created by the build. Do you 
>>>> need this?
>>>
>>> Had the same thought, but the build fails in non clean builds[0]. Did a
>>> workaround[1] but then some essential template nodes ends up getting
>>> deleted from u-boot.dtb. Finally, had to write back to the file
>>> to resolve the issue.
>> Can you push a tree somewhere. This could be a bug that I could fix.
>
> You can use the tree here to recreate the non clean build issue 
> mentioned:
> https://github.com/Jamm02/U-Boot-patchwork/tree/bootph-patch-test
>
>>
>>>>>        dtb = fdt.FdtScan(fname)
>>>>>
>>>>>        node = _FindBinmanNode(dtb)
>>>>> -- 
>>>>> 2.34.1
>>>>>
>>>> The code looks fine to me apart from the nits.
>>>>
>>>> This addition needs a test - see ftest.py for some examples there. But
>>>> basically just create a dts that has some props in it, then check that
>>>> they got added.
>>>
>>> Included a test as well in the patchset[3]. Do let me know if there are
>>> some changes required in it.
>> Somehow I'm not seeing that in patchwork. It looks good but please try
>> to keep lines <=80 cols.
>
>
> Sure, will fix up in v3. Should I push a v3 or wait for you to fix the 
> issue discussed above?
>
>>
>>
>>>> I think testSimpleFitEncryptedData() could be a good example?
>>>>
>>>> Regards,
>>>> Simon
>>> References:
>>> [0]https://gist.github.com/Jamm02/2478a4f1186f3ba4a8cd7dbf1fb11e2f#file-non-clean-build 
>>>
>>> [1]https://gist.github.com/Jamm02/2478a4f1186f3ba4a8cd7dbf1fb11e2f#file-patch-tools-dtdoc-fdt-py 
>>>
>>> [3]https://lore.kernel.org/u-boot/20250327080642.2269856-3-m-shah@ti.com/ 
>>>
>>>
>>> Thanks for the review.
>> Regards,
>> Simon
>
>
> Regards,
> Moteen
>