Re: [PATCH v7 2/2] arm64: boot: Support Flat Image Tree

[Masahiro Yamada]

On Thu, Nov 30, 2023 at 5:26 PM Nicolas Schier <nicolas@xxxxxxxxx> wrote:
>
> Simon,
>
> thanks for the patch!  Below are some nitpicks and bike-shedding
> questions.
>
> On Wed 29 Nov 2023 10:21:53 GMT, Simon Glass wrote:
> > Add a script which produces a Flat Image Tree (FIT), a single file
> > containing the built kernel and associated devicetree files.
> > Compression defaults to gzip which gives a good balance of size and
> > performance.
> >
> > The files compress from about 86MB to 24MB using this approach.
> >
> > The FIT can be used by bootloaders which support it, such as U-Boot
> > and Linuxboot. It permits automatic selection of the correct
> > devicetree, matching the compatible string of the running board with
> > the closest compatible string in the FIT. There is no need for
> > filenames or other workarounds.
>
> Have you thought about updating the arch/mips ITB rules to also use the
> new scripts/make_fit.py?  Or is the FIT/ITB format for mips different
> from the one for arm64?



I recommend not touching MIPS at this moment
because this tool simply picks up *.dtb files
that exist under arch/*/boot/dts/, some of which
may be stale files.




Think of this scenario:


[1] Enable CONFIG_ARCH_FOO and build

   foo.dtb

will be created.


[2] Next, disable CONFIG_ARCH_FOO and
    enable CONFIG_ARCH_BAR, and build.

   bar.dtb

will be created.


This script will pick up both foo.dtb and bar.dtb
although foo.dtb is a left-over from the previous build.



Without cleaning, stale *.dtb will accumulate
and unwanted files will be included in image.fit.



Currently, MIPS hard-codes its files.
It always works in a deterministic way.




I do not request Simon to implement everything perfectly
because I know that would require much more effort.

We could do something like modules.order to list out the
dtb files from the current build, but I am not asking for it
in this patchset.



But, you are right. This tool is not arm64-specific at all
(and that is the reason why I think the MAINTAINERS
entry is a little odd)
Perhaps it can be applicable to MIPS after everything
works correctly.







> >  ARM ARCHITECTED TIMER DRIVER
> >  M:   Mark Rutland <mark.rutland@xxxxxxx>
> >  M:   Marc Zyngier <maz@xxxxxxxxxx>
> > diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> > index 1bd4fae6e806..18e092de7cdb 100644
> > --- a/arch/arm64/Makefile
> > +++ b/arch/arm64/Makefile
> > @@ -36,6 +36,8 @@ ifeq ($(CONFIG_BROKEN_GAS_INST),y)
> >  $(warning Detected assembler with broken .inst; disassembly will be unreliable)
> >  endif
> >
> > +KBUILD_DTBS      := dtbs
>
> Might you want to use tabs here as in the lines below?



This should not exist in the first place.


  image.fit: dtbs


is better.









--
Best Regards
Masahiro Yamada