Hi Moteen,
On Thu, 10 Apr 2025 at 05:39, Moteen Shah <m-s...@ti.com> wrote:
>
> 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-s...@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-s...@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-s...@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've finally got back to this, but I can't repeat it.
For me:
crosfw j7200_evm_r5 -F
cmd: make -j32
'CROSS_COMPILE=/home/sglass/.buildman-toolchains/gcc-13.2.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-'
--no-print-directory 'HOSTSTRIP=true' 'QEMU_ARCH='
'KCONFIG_NOSILENTUPDATE=1' 'O=/tmp/b/j7200_evm_r5' -s 'BUILD_ROM=1'
all
Image 'tiboot3-j7200-hs-evm.bin' has faked external blobs and is
non-functional: ti-fs-firmware-j7200-hs-enc.bin
ti-fs-firmware-j7200-hs-cert.bin
Image 'tiboot3-j7200-hs-evm.bin' is missing optional external blobs
but is still functional: ti-fs-enc.bin sysfw-inner-cert
/binman/tiboot3-j7200-hs-evm.bin/ti-fs-enc.bin
(ti-sysfw/ti-fs-firmware-j7200-hs-enc.bin):
Missing blob
/binman/tiboot3-j7200-hs-evm.bin/sysfw-inner-cert
(ti-sysfw/ti-fs-firmware-j7200-hs-cert.bin):
Missing blob
Image 'tiboot3-j7200_sr2-hs-evm.bin' has faked external blobs and is
non-functional: ti-fs-firmware-j7200_sr2-hs-enc.bin
ti-fs-firmware-j7200_sr2-hs-cert.bin
Image 'tiboot3-j7200_sr2-hs-evm.bin' is missing optional external
blobs but is still functional: ti-fs-enc.bin sysfw-inner-cert
/binman/tiboot3-j7200_sr2-hs-evm.bin/ti-fs-enc.bin
(ti-sysfw/ti-fs-firmware-j7200_sr2-hs-enc.bin):
Missing blob
/binman/tiboot3-j7200_sr2-hs-evm.bin/sysfw-inner-cert
(ti-sysfw/ti-fs-firmware-j7200_sr2-hs-cert.bin):
Missing blob
Image 'tiboot3-j7200-hs-fs-evm.bin' has faked external blobs and is
non-functional: ti-fs-firmware-j7200-hs-fs-enc.bin
ti-fs-firmware-j7200-hs-fs-cert.bin
Image 'tiboot3-j7200-hs-fs-evm.bin' is missing optional external blobs
but is still functional: ti-fs-enc.bin sysfw-inner-cert
/binman/tiboot3-j7200-hs-fs-evm.bin/ti-fs-enc.bin
(ti-sysfw/ti-fs-firmware-j7200-hs-fs-enc.bin):
Missing blob
/binman/tiboot3-j7200-hs-fs-evm.bin/sysfw-inner-cert
(ti-sysfw/ti-fs-firmware-j7200-hs-fs-cert.bin):
Missing blob
Image 'tiboot3-j7200_sr2-hs-fs-evm.bin' has faked external blobs and
is non-functional: ti-fs-firmware-j7200_sr2-hs-fs-enc.bin
ti-fs-firmware-j7200_sr2-hs-fs-cert.bin
Image 'tiboot3-j7200_sr2-hs-fs-evm.bin' is missing optional external
blobs but is still functional: ti-fs-enc.bin sysfw-inner-cert
/binman/tiboot3-j7200_sr2-hs-fs-evm.bin/ti-fs-enc.bin
(ti-sysfw/ti-fs-firmware-j7200_sr2-hs-fs-enc.bin):
Missing blob
/binman/tiboot3-j7200_sr2-hs-fs-evm.bin/sysfw-inner-cert
(ti-sysfw/ti-fs-firmware-j7200_sr2-hs-fs-cert.bin):
Missing blob
Image 'tiboot3-j7200-gp-evm.bin' has faked external blobs and is
non-functional: ti-fs-firmware-j7200-gp.bin
Image 'tiboot3-j7200-gp-evm.bin' is missing optional external blobs
but is still functional: ti-fs-gp.bin
/binman/tiboot3-j7200-gp-evm.bin/ti-fs-gp.bin
(ti-sysfw/ti-fs-firmware-j7200-gp.bin):
Missing blob
Some images are invalid
make[1]: *** [/scratch/sglass/cosarm/src/third_party/u-boot/files/Makefile:1135:
.binman_stamp] Error 103
make: *** [Makefile:177: sub-make] Error 2
I wonder if I need some blobs?
> >
> >>
> >>
> >>>> 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-s...@ti.com/
> >>>
> >>>
> >>> Thanks for the review.
Regards,
SImon
