Replace make-fit-atf.py with binman. Was: migrate u-boot-rockchip.bin to binman and generate an image for SPI
Simon Glass
sjg at chromium.org
Mon Aug 1 21:13:27 CEST 2022
Hi Quentin,
On Mon, 1 Aug 2022 at 11:05, Quentin Schulz
<quentin.schulz at theobroma-systems.com> wrote:
>
> Hi Simon,
>
> On 7/31/22 03:27, Simon Glass wrote:
> > Hi Quentin,
> >
> > On Wed, 27 Jul 2022 at 04:34, Quentin Schulz
> > <quentin.schulz at theobroma-systems.com> wrote:
> >>
> >> Hi Simon,
> >>
> >> On 7/26/22 21:58, Simon Glass wrote:
> >>> Hi Quentin,
> >>>
> >>> On Tue, 26 Jul 2022 at 03:08, Quentin Schulz
> >>> <quentin.schulz at theobroma-systems.com> wrote:
> >>>>
> >>>> Hi Xavier,
> >>>>
> >>>> On 7/25/22 19:33, Xavier Drudis Ferran wrote:
> >>>>> El Mon, Jul 25, 2022 at 07:29:53PM +0200, Xavier Drudis Ferran deia:
> >>>>>>
> >>>>>> I copy here the rockchip-u-boot.dtsi file and then 2 patches on top of yours.
> >>>>>>
> >>>>>
> >>>>> Sorry I copied a dirty version that din't work. The patches were correct, the dtsi wasn't.
> >>>>>
> >>>
> >>> [..]
> >>>
> >>>>
> >>>>>> + };
> >>>>>> + };
> >>>>>> + };
> >>>>>> + };
> >>>>>> simple-bin {
> >>>>>> filename = "u-boot-rockchip.bin";
> >>>>>> pad-byte = <0xff>;
> >>>>>> @@ -62,6 +131,7 @@
> >>>>>> #ifdef CONFIG_ARM64
> >>>>>> blob {
> >>>>>> filename = "u-boot.itb";
> >>>>>> +
> >>>>
> >>>> This is unfortunately not possible since binman parallelizes the build
> >>>> of images in the binman DT node. This means there is no guarantee the
> >>>> u-boot.itb file is generated before this section is worked on by binman.
> >>>> Or maybe I misunderstood the docs.
> >>>
> >>> You are correct, but this is something that has bothered me.
> >>>
> >>> At the moment we handle this by embedding the definition for one file
> >>> into the one that uses it. This is on the theory that there is no need
> >>> to actually write the embedded file, since it is a temporary file. In
> >>> fact binman does generate temporary files though.
> >>>
> >>> Is that good enough? If not I'd like to understand the need better,
> >>> before we make any changes.
> >>>
> >>
> >> For Rockchip, with the patch series from
> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_u-2Dboot_20220722113505.3875669-2D1-2Dfoss-2Buboot-400leil.net_&d=DwIBaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=OevHoqt3vOXsWmXfEPFnvZ2KSNns-ZKBiiZBUovrynOXVCUZtFfK9jsk3C1r4Y_J&s=nXnNRN3hfa_mkkt_suH6wLGbwj06I7HmQKcagZ6JPE0&e= ,
> >> we now have two binaries:
> >> u-boot-rockchip.bin and u-boot-rockchip-spi.bin
> >>
> >> Both share the exact same u-boot.itb file (though at a different offset
> >> in the storage medium) embedded.
> >>
> >> This u-boot.itb is currently externally created via the Makefile +
> >> make_fit_atf.py prior to binman being called. This allows us to have a
> >> blob { filename = "u-boot.itb"; } in the binman DT node and that's
> >> enough for it to work fine.
> >>
> >> There's a desire to get rid of make_fit_atf.py in favor of binman. This
> >> means that we need to create this file in binman now.
> >>
> >> There's been some resistance to making binman not create idbloader.img
> >> image in the patch series mentioned above (it is *technically* created
> >> by binman but only in temporary files and then embedded in
> >> u-boot-rockchip*.bin). I assume the same resistance will be met for
> >> u-boot.itb.
> >>
> >> With how binman works currently and since we have u-boot.itb content
> >> embedded in at least two different images created by binman (might be
> >> even more once there's USB/NAND support?), we'd need to have the fit
> >> device tree node appear thrice (or more). One for u-boot.itb creation
> >> (because of people not wanting it disappear from binaries generated by
> >> U-Boot), one for embedding into u-boot-rockchip.bin and one for
> >> embedding into u-boot-rockchip-spi.bin.
> >> This increases the risk of not updating all fit device tree nodes at once.
> >> This is suboptimal in terms of build time because the image will
> >> effectively be created thrice (or more).
> >> This is also not the best for easy check of reproducibility since the
> >> content of the fit device tree node is technically not the same as
> >> u-boot.itb file (because it is the result of a different image built by
> >> binman).
> >>
> >> So I think the minimal implementation would be to allow to define an
> >> image/section (u-boot.itb) and to "#include" it inline in another
> >> section (where blob { filename = "u-boot.itb"; } currently is for
> >> u-boot-rockchip.bin and u-boot-rockchip-spi.bin). From reading the docs,
> >> I expected collection entry to be exactly this? But I couldn't make it work.
> >>
> >> Ideally, allowing binman to define a dependency from one image on
> >> another would mean we could still use the blob image/section, but
> >> safely, because the dependency is explicit so binman will know which
> >> image to build first. Phandles is what we're after on the Device Tree
> >> side I would assume. We could have something like: blob { image =
> >> <&u-boot-itb>; } for example. No idea how binman would create this
> >> dependency tree though :)
> >>
> >> Straight from my brain, lemme know if something needs to be clarified or
> >> rephrased.
> >
> > Collections should allow this. Can you try running with threading
> > disabled (-T0)?
> >
>
> Do they?
>
> What (I think) I want is basically the following:
>
> &binman {
> multiple-images;
> u_boot_itb: u-boot-itb {
> fit {};
> };
> simple-bin {
> [...]
> collection {
> content = <&u_boot_itb>;
> };
> };
> simple-bin-spi {
> [...]
> collection {
> content = <&u_boot_itb>;
> };
> };
> };
>
> or I guess something like:
> &binman {
> multiple-images;
> u-boot-itb {
> filename = "u-boot.itb";
> collection {
> content = <&u_boot_itb>;
> };
> };
> simple-bin {
> [...]
> u_boot_itb: fit {};
> };
> simple-bin-spi {
> [...]
> collection {
> content = <&u_boot_itb>;
> };
> };
> };
>
> However, I tried with:
> // SPDX-License-Identifier: GPL-2.0+
> /dts-v1/;
>
> / {
> #address-cells = <1>;
> #size-cells = <1>;
>
> binman {
> multiple-images;
> text2: foo {
> text1: text {
> text = "bl31.bin";
> };
> };
> bar {
> collection {
> content = <&text2>;
> };
> };
> };
> };
>
> and:
> // SPDX-License-Identifier: GPL-2.0+
> /dts-v1/;
>
> / {
> #address-cells = <1>;
> #size-cells = <1>;
>
> binman {
> multiple-images;
> text2: foo {
> text1: text {
> text = "bl31.bin";
> };
> };
> bar {
> collection {
> content = <&text1>;
> };
> };
> };
> };
>
> but tools/binman/binman build -d test.dts returns for both:
> binman: Node '/binman/bar/collection': Cannot find entry for node 'text'
> (or 'foo')
> I guess the issue is that this collection would be crossing the image
> boundary and this is not supported?
Yes, they need to be in the same section / image.
>
> I'm not sure to really understand the point of the collection section? I
> would assume wanting to get multiple times the same entries in the same
> image should be pretty rare?
Yes it should, but it does happen.
>
> I guess I could simply have a huge constant defining the u-boot-itb fit
> node and add it to the binman DT node in the correct places to avoid
> having to maintain multiple copies of the DT node, but it's still
> inefficient IMO since the image would be created as many times as it's
> used. Ideally it'd be a dependency on an image being created and one
> uses blob-ext or something similar to use this newly generated image. I
> don't know, throwing ideas right now.
>
> Am I completely lost or does what I want to do make some kind of sense?
Well I still feel that we should handle this properly in binman.
I don't even think we need to allow images to be incorporated in other
images. We can probably just allow a section to have a filename, a bit
like this patch [1].
Regards,
Simon
[1] https://patchwork.ozlabs.org/project/uboot/patch/20220801160610.2330151-3-foss+uboot@0leil.net/
More information about the U-Boot
mailing list