Message-ID: <CAGXv+5Fp__Hats=sNmdmg8dYVfNB2K0FP23aKA_7D3sz4EvzrA@mail.gmail.com>
Date: Tue, 2 Jan 2024 11:19:21 +0800
From: Chen-Yu Tsai <wenst@...omium.org>
To: Simon Glass <sjg@...omium.org>
Cc: Masahiro Yamada <masahiroy@...nel.org>, Geert Uytterhoeven <geert@...ux-m68k.org>, 
	Laurent Pinchart <laurent.pinchart@...asonboard.com>, linux-arm-kernel@...ts.infradead.org, 
	Ahmad Fatoum <a.fatoum@...gutronix.de>, U-Boot Mailing List <u-boot@...ts.denx.de>, 
	Nicolas Schier <nicolas@...sle.eu>, Tom Rini <trini@...sulko.com>, 
	Catalin Marinas <catalin.marinas@....com>, Jonathan Corbet <corbet@....net>, 
	Nathan Chancellor <nathan@...nel.org>, Nick Terrell <terrelln@...com>, Will Deacon <will@...nel.org>, 
	linux-doc@...r.kernel.org, linux-kbuild@...r.kernel.org, 
	linux-kernel@...r.kernel.org, workflows@...r.kernel.org
Subject: Re: [PATCH v9 2/2] arm64: boot: Support Flat Image Tree

On Fri, Dec 29, 2023 at 2:39 PM Simon Glass <sjg@...omium.org> wrote:
>
> Hi Masahiro,
>
> On Thu, Dec 14, 2023 at 7:34 AM Masahiro Yamada <masahiroy@...nel.org> wrote:
> >
> > On Thu, Dec 14, 2023 at 3:12 PM Masahiro Yamada <masahiroy@...nel.org> wrote:
> > >
> > > On Thu, Dec 14, 2023 at 1:03 PM Chen-Yu Tsai <wenst@...omium.org> wrote:
> > > >
> > > > On Sun, Dec 10, 2023 at 1:31 AM Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
> > > > >
> > > > > Hi Laurent,
> > > > >
> > > > > On Sat, Dec 9, 2023 at 4:29 PM Laurent Pinchart
> > > > > <laurent.pinchart@...asonboard.com> wrote:
> > > > > > On Sat, Dec 09, 2023 at 10:13:59PM +0900, Chen-Yu Tsai wrote:
> > > > > > > On Thu, Dec 7, 2023 at 11:38 PM Laurent Pinchart
> > > > > > > <laurent.pinchart@...asonboard.com> wrote:
> > > > > > > > On Thu, Dec 07, 2023 at 10:27:23PM +0800, Chen-Yu Tsai wrote:
> > > > > > > > > On Sun, Dec 03, 2023 at 05:34:01PM +0200, Laurent Pinchart wrote:
> > > > > > > > > > On Fri, Dec 01, 2023 at 08:54:42PM -0700, 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.
> > > > > > > > > > >
> > > > > > > > > > > Add a 'make image.fit' build target for arm64, as well. Use
> > > > > > > > > > > FIT_COMPRESSION to select a different algorithm.
> > > > > > > > > > >
> > > > > > > > > > > The FIT can be examined using 'dumpimage -l'.
> > > > > > > > > > >
> > > > > > > > > > > This features requires pylibfdt (use 'pip install libfdt'). It also
> > > > > > > > > > > requires compression utilities for the algorithm being used. Supported
> > > > > > > > > > > compression options are the same as the Image.xxx files. For now there
> > > > > > > > > > > is no way to change the compression other than by editing the rule for
> > > > > > > > > > > $(obj)/image.fit
> > > > > > > > > > >
> > > > > > > > > > > While FIT supports a ramdisk / initrd, no attempt is made to support
> > > > > > > > > > > this here, since it must be built separately from the Linux build.
> > > > > > > > > >
> > > > > > > > > > FIT images are very useful, so I think this is a very welcome addition
> > > > > > > > > > to the kernel build system. It can get tricky though: given the
> > > > > > > > > > versatile nature of FIT images, there can't be any
> > > > > > > > > > one-size-fits-them-all solution to build them, and striking the right
> > > > > > > > > > balance between what makes sense for the kernel and the features that
> > > > > > > > > > users may request will probably lead to bikeshedding. As we all love
> > > > > > > > > > bikeshedding, I thought I would start selfishly, with a personal use
> > > > > > > > > > case :-) This isn't a yak-shaving request though, I don't see any reason
> > > > > > > > > > to delay merging this series.
> > > > > > > > > >
> > > > > > > > > > Have you envisioned building FIT images with a subset of DTBs, or adding
> > > > > > > > > > DTBOs ? Both would be fairly trivial extensions to this script by
> > > > > > > > > > extending the supported command line arguments. It would perhaps be more
> > > > > > > > > > difficult to integrate in the kernel build system though. This leads me
> > > > > > > > > > to a second question: would you consider merging extensions to this
> > > > > > > > > > script if they are not used by the kernel build system, but meant for
> > > > > > > > > > users who manually invoke the script ? More generally, is the script
> > > > > > > > >
> > > > > > > > > We'd also be interested in some customization, though in a different way.
> > > > > > > > > We imagine having a rule file that says X compatible string should map
> > > > > > > > > to A base DTB, plus B and C DTBO for the configuration section. The base
> > > > > > > > > DTB would carry all common elements of some device, while the DTBOs
> > > > > > > > > carry all the possible second source components, like different display
> > > > > > > > > panels or MIPI cameras for instance. This could drastically reduce the
> > > > > > > > > size of FIT images in ChromeOS by deduplicating all the common stuff.
> > > > > > > >
> > > > > > > > Do you envision the "mapping" compatible string mapping to a config
> > > > > > > > section in the FIT image, that would bundle the base DTB and the DTBOs ?
> > > > > > >
> > > > > > > That's exactly the idea. The mapping compatible string could be untied
> > > > > > > from the base board's compatible string if needed (which we probably do).
> > > > > > >
> > > > > > > So something like:
> > > > > > >
> > > > > > > config {
> > > > > > >     config-1 {
> > > > > > >         compatible = "google,krane-sku0";
> > > > > > >         fdt = "krane-baseboard", "krane-sku0-overlay";
> > > > > > >     };
> > > > > > > };
> > > > > > >
> > > > > > > With "krane-sku0-overlay" being an overlay that holds the differences
> > > > > > > between the SKUs, in this case the display panel and MIPI camera (not
> > > > > > > upstreamed) that applies to SKU0 in particular.
> > > > > >
> > > > > > The kernel DT makefiles already contain information on what overlays to
> > > > > > apply to what base boards, in order to test the overlays and produce
> > > > > > "full" DTBs. Maybe that information could be leveraged to create the
> > > > > > configurations in the FIT image ?
> > > > >
> > > > > Although the "full" DTBs created may only be a subset of all possible
> > > > > combinations (I believe Rob just started with creating one "full" DTB
> > > > > for each overlay, cfr. the additions I made in commit a09c3e105a208580
> > > > > ("arm64: dts: renesas: Apply overlays to base dtbs")), that could
> > > > > definitely be a start.
> > > > >
> > > > > Now, since the kernel build system already creates "full" DTBs, does
> > > > > that mean that all of the base DTBs, overlays, and "full" DTBs will
> > > > > end up in the FIT image?
> > > >
> > > > I suppose we could add an option to the packing tool to be able to _not_
> > > > add the "full" DTBs if they can also be assembled with a base DTB and
> > > > overlays. Think of it as a firmware compatibility option: if the firmware
> > > > supports overlays, then you almost always want the deconstructed parts,
> > > > not the fully assembled ones. Vice versa.
> > > >
> > > > If we don't we could end up with two configurations that have the same
> > > > compatible string?
> > >
> > >
> > > Right.
> > >
> > > We would end up with such situations because applying
> > > an overlay does not change the compatible string.
> > >
> > >
> > >
> > > With this code in arch/arm64/boot/dts/ti/Makefile:
> > >
> > > k3-am642-tqma64xxl-mbax4xxl-sdcard-dtbs := \
> > >       k3-am642-tqma64xxl-mbax4xxl.dtb k3-am64-tqma64xxl-mbax4xxl-sdcard.dtbo
> > > k3-am642-tqma64xxl-mbax4xxl-wlan-dtbs := \
> > >       k3-am642-tqma64xxl-mbax4xxl.dtb k3-am64-tqma64xxl-mbax4xxl-wlan.dtbo
> > >
> > >
> > >
> > >
> > > $ fdtdump  arch/arm64/boot/dts/ti/k3-am642-tqma64xxl-mbax4xxl-sdcard.dtb
> > > 2>/dev/null| head -n15 | tail -n2
> > >     model = "TQ-Systems TQMa64xxL SoM on MBax4xxL carrier board";
> > >     compatible = "tq,am642-tqma6442l-mbax4xxl", "tq,am642-tqma6442l",
> > > "ti,am642";
> > >
> > >
> > > $ fdtdump  arch/arm64/boot/dts/ti/k3-am642-tqma64xxl-mbax4xxl-wlan.dtb
> > > 2>/dev/null| head -n15 | tail -n2
> > >     model = "TQ-Systems TQMa64xxL SoM on MBax4xxL carrier board";
> > >     compatible = "tq,am642-tqma6442l-mbax4xxl", "tq,am642-tqma6442l",
> > > "ti,am642";
> > >
> > >
> > >
> > >
> > >
> > > These two go into image.fit, but one of them is completely dead
> > > since there is no way to distinguish them.
> > >
> > >
> > > $ fdtdump  arch/arm64/boot/image.fit
> > >
> > >         ...
> > >
> > >         conf-10 {
> > >             compatible = "tq,am642-tqma6442l-mbax4xxl",
> > > "tq,am642-tqma6442l", "ti,am642";
> > >             description = "TQ-Systems TQMa64xxL SoM on MBax4xxL carrier board";
> > >             fdt = "fdt-10";
> > >             kernel = "kernel";
> > >         };
> > >
> > >         ...
> > >
> > >         conf-25 {
> > >             compatible = "tq,am642-tqma6442l-mbax4xxl",
> > > "tq,am642-tqma6442l", "ti,am642";
> > >             description = "TQ-Systems TQMa64xxL SoM on MBax4xxL carrier board";
> > >             fdt = "fdt-25";
> > >             kernel = "kernel";
> > >         };
> > >
> > >
> > >
> > >
> > >
> > > I agree with Chen-Yu.
> > >
> > > FIT should not include full DTBs.
> > >
> > > Bootloaders should assemble the final DTB
> > > from base and overlays on-the-fly.
> > >
> > >
> > > The FIT spec allows the "fdt" property to list
> > > multiple image nodes.
> > >
> > >
> > > o config-1
> > >  |- description = "configuration description"
> > >  |- kernel = "kernel sub-node unit name"
> > >  |- fdt = "fdt sub-node unit-name" [, "fdt overlay sub-node unit-name", ...]
> > >  |- loadables = "loadables sub-node unit-name"
> > >  |- script = "
> > >  |- compatible = "vendor
> >
> >
> >
> >
> >
> > This is a question for U-Boot (and barebox).
> >
> >
> >
> >
> >    images {
> >           base {
> >                 ...
> >           };
> >
> >           addon1 {
> >                 ...
> >           };
> >
> >           addon2 {
> >                 ...
> >           };
> >     };
> >
> >     configurations {
> >           ...
> >           fdt = "base", "addon1", "addon2";
> >     };
> >
> >
> >
> >
> > Is U-Boot's "bootm" command able to dynamically construct
> > the full DTB from "base" + "addon1" + "addon2"
> > and pass to the kernel?
> >
> >
> >
> > When I used overlay from U-Boot command line last time,
> > I typed complicated commands, following this manual:
> > https://docs.u-boot.org/en/latest/usage/fdt_overlays.html
> >
> >
>
> So far this is not possible with bootm, no. But if we can add
> extensions to the FIT spec, then it should be possible to implement
> this.

Isn't this already part of the FIT spec? There's nothing special here.
It's one configuration that lists a base DTB plus some addon overlays.

The FIT spec says:

  - fdt : Unit name of the corresponding fdt blob (component image node of a
    "fdt type"). Additional fdt overlay nodes can be supplied which signify
    that the resulting device tree blob is generated by the first base fdt
    blob with all subsequent overlays applied.

Are you saying that U-boot currently lacks a mechanism to select the config?
A quick skim over U-boot code suggests that boards need to implement
board_fit_config_name_match()?

> Is it (or will it be) possible to get Linux to build the DT + overlay
> combinations?

It is possible to build overlays separately, and have the build system
apply them. Taking an example from the Renesas tree (line wrap mine):

    dtb-$(CONFIG_ARCH_R8A779G0) += r8a779g0-white-hawk.dtb
    dtb-$(CONFIG_ARCH_R8A779G0) += r8a779g0-white-hawk-ard-audio-da7212.dtbo
    r8a779g0-white-hawk-ard-audio-da7212-dtbs := r8a779g0-white-hawk.dtb \
            r8a779g0-white-hawk-ard-audio-da7212.dtbo
    dtb-$(CONFIG_ARCH_R8A779G0) += r8a779g0-white-hawk-ard-audio-da7212.dtb

The overlays are applied using the fdtoverlay command from the device tree
compiler suite.

ChenYu

> > One more question to confirm if I can use this
> > for my practical use-cases.
> >
> > Is U-Boot able to handle FIT (includes kernel + DTs)
> > and a separate initrd?
> >
> >   # bootm  <fit-address>:<conf-name>  <ramdisk-address>
> >
> >
> > Presumably, it would be difficult to inject initramdisk
> > into image.fit later, so I am hoping bootm would work like that,
> > but I did not delve into U-Boot code.
> >
> >
>
> The ramdisk is handled by the FIT configuration. I suppose it would be
> possible to add a way to bypass the logic in select_ramdisk(), but I
> wonder what is the use case for this?
>
> >
> > If it works, is it possible to verify the integrity of initrd?
> > The kernel and DTs inside FIT will be verified, but not sure
> > if it is possible for ramdisk.
>
> I do have thoughts about a possible new FIT feature to allow external
> files, which could perhaps include an initrd.
>
> Regards,
> Simon

Powered by blists - more mailing lists