Message-ID: <CAGXv+5E1UsHyxier94=ah0P_g5QfdwLgNzc915QjdtXQqTN8ow@mail.gmail.com>
Date: Thu, 13 Jun 2024 14:06:25 +0800
From: Chen-Yu Tsai <wenst@...omium.org>
To: Masahiro Yamada <masahiroy@...nel.org>
Cc: Simon Glass <sjg@...omium.org>, Nathan Chancellor <nathan@...nel.org>, 
	Nicolas Schier <nicolas@...sle.eu>, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, linux-kbuild@...r.kernel.org
Subject: Re: [PATCH] scripts/make_fit: Support decomposing DTBs

On Tue, Jun 11, 2024 at 10:33 PM Masahiro Yamada <masahiroy@...nel.org> wrote:
>
> On Wed, Jun 5, 2024 at 6:48 PM Chen-Yu Tsai <wenst@...omium.org> wrote:
> >
> > The kernel tree builds some "composite" DTBs, where the final DTB is the
> > result of applying one or more DTB overlays on top of a base DTB with
> > fdtoverlay.
> >
> > The FIT image specification already supports configurations having one
> > base DTB and overlays applied on top. It is then up to the bootloader to
> > apply said overlays and either use or pass on the final result. This
> > allows the FIT image builder to reuse the same FDT images for multiple
> > configurations, if such cases exist.
> >
> > The decomposition function depends on the kernel build system, reading
> > back the .cmd files for the to-be-packaged DTB files to check for the
> > fdtoverlay command being called. This will not work outside the kernel
> > tree. The function is off by default to keep compatibility with possible
> > existing users.
> >
> > To facilitate the decomposition and keep the code clean, the model and
> > compatitble string extraction have been moved out of the output_dtb
> > function. The FDT image description is replaced with the base file name
> > of the included image.
> >
> > Signed-off-by: Chen-Yu Tsai <wenst@...omium.org>
> > ---
> > This is a feature I alluded to in my replies to Simon's original
> > submission of the make_fit.py script [1].
> >
> > This is again made a runtime argument as not all firmware out there
> > that boot FIT images support applying overlays. Like my previous
> > submission for disabling compression for included FDT images, the
> > bootloader found in RK3399 and MT8173 Chromebooks do not support
> > applying overlays. Another case of this is U-boot shipped by development
> > board vendors in binary form (without upstream) in an image or in
> > SPI flash on the board that were built with OF_LIBFDT_OVERLAY=n.
> > These would fail to boot FIT images with DT overlays. One such
> > example is my Hummingboard Pulse. In these cases the firmware is
> > either not upgradable or very hard to upgrade.
> >
> > I believe there is value in supporting these cases. A common script
> > shipped with the kernel source that can be shared by distros means
> > the distro people don't have to reimplement this in their downstream
> > repos or meta-packages. For ChromeOS this means reducing the amount
> > of package code we have in shell script.
> >
> > [1] https://lore.kernel.org/linux-kbuild/20231207142723.GA3187877@google.com/
> > [2]
> >
> >  scripts/Makefile.lib |  1 +
> >  scripts/make_fit.py  | 70 ++++++++++++++++++++++++++++++--------------
> >  2 files changed, 49 insertions(+), 22 deletions(-)
> >
> > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > index 9f06f6aaf7fc..d78b5d38beaa 100644
> > --- a/scripts/Makefile.lib
> > +++ b/scripts/Makefile.lib
> > @@ -522,6 +522,7 @@ quiet_cmd_fit = FIT     $@
> >        cmd_fit = $(MAKE_FIT) -o $@ --arch $(UIMAGE_ARCH) --os linux \
> >                 --name '$(UIMAGE_NAME)' \
> >                 $(if $(findstring 1,$(KBUILD_VERBOSE)),-v) \
> > +               $(if $(FIT_DECOMPOSE_DTBS),--decompose-dtbs) \
> >                 --compress $(FIT_COMPRESSION) -k $< @$(word 2,$^)
> >
> >  # XZ
> > diff --git a/scripts/make_fit.py b/scripts/make_fit.py
> > index 263147df80a4..120f13e1323c 100755
> > --- a/scripts/make_fit.py
> > +++ b/scripts/make_fit.py
> > @@ -22,6 +22,11 @@ the entire FIT.
> >  Use -c to compress the data, using bzip2, gzip, lz4, lzma, lzo and
> >  zstd algorithms.
> >
> > +Use -d to decompose "composite" DTBs into their base components and
> > +deduplicate the resulting base DTBs and DTB overlays. This requires the
> > +DTBs to be sourced from the kernel build directory, as the implementation
> > +looks at the .cmd files produced by the kernel build.
> > +
> >  The resulting FIT can be booted by bootloaders which support FIT, such
> >  as U-Boot, Linuxboot, Tianocore, etc.
> >
> > @@ -64,6 +69,8 @@ def parse_args():
> >            help='Specifies the architecture')
> >      parser.add_argument('-c', '--compress', type=str, default='none',
> >            help='Specifies the compression')
> > +    parser.add_argument('-d', '--decompose-dtbs', action='store_true',
> > +          help='Decompose composite DTBs into base DTB and overlays')
> >      parser.add_argument('-E', '--external', action='store_true',
> >            help='Convert the FIT to use external data')
> >      parser.add_argument('-n', '--name', type=str, required=True,
> > @@ -140,12 +147,12 @@ def finish_fit(fsw, entries):
> >      fsw.end_node()
> >      seq = 0
> >      with fsw.add_node('configurations'):
> > -        for model, compat in entries:
> > +        for model, compat, files in entries:
> >              seq += 1
> >              with fsw.add_node(f'conf-{seq}'):
> >                  fsw.property('compatible', bytes(compat))
> >                  fsw.property_string('description', model)
> > -                fsw.property_string('fdt', f'fdt-{seq}')
> > +                fsw.property('fdt', b''.join([b'fdt-%d\x00' % x for x in files]))
> >                  fsw.property_string('kernel', 'kernel')
> >      fsw.end_node()
> >
> > @@ -193,21 +200,9 @@ def output_dtb(fsw, seq, fname, arch, compress):
> >          fname (str): Filename containing the DTB
> >          arch: FIT architecture, e.g. 'arm64'
> >          compress (str): Compressed algorithm, e.g. 'gzip'
> > -
> > -    Returns:
> > -        tuple:
> > -            str: Model name
> > -            bytes: Compatible stringlist
> >      """
> >      with fsw.add_node(f'fdt-{seq}'):
> > -        # Get the compatible / model information
> > -        with open(fname, 'rb') as inf:
> > -            data = inf.read()
> > -        fdt = libfdt.FdtRo(data)
> > -        model = fdt.getprop(0, 'model').as_str()
> > -        compat = fdt.getprop(0, 'compatible')
> > -
> > -        fsw.property_string('description', model)
> > +        fsw.property_string('description', os.path.basename(fname))
> >          fsw.property_string('type', 'flat_dt')
> >          fsw.property_string('arch', arch)
> >          fsw.property_string('compression', compress)
> > @@ -215,7 +210,6 @@ def output_dtb(fsw, seq, fname, arch, compress):
> >          with open(fname, 'rb') as inf:
> >              compressed = compress_data(inf, compress)
> >          fsw.property('data', compressed)
> > -    return model, compat
> >
> >
> >  def build_fit(args):
> > @@ -235,6 +229,7 @@ def build_fit(args):
> >      fsw = libfdt.FdtSw()
> >      setup_fit(fsw, args.name)
> >      entries = []
> > +    fdts = collections.OrderedDict()
>
>
> I am fine with this patch.
>
> Just a nit.
>
> Is there any reason why you used OrderedDict() instead of
> the normal dictionary, "fdts = {}" ?

I had wanted to use it as the main list of entries; using OrderedDict()
preserves the order that the DTBs were given. That didn't pan out.

I'll replace it with the standard dictionary.


ChenYu

Powered by blists - more mailing lists