Re: Replace make-fit-atf.py with binman. Was: migrate u-boot-rockchip.bin to binman and generate an image for SPI

Quentin Schulz
2022-07-27T10:34:19Z

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@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://lore.kernel.org/u-boot/20220722113505.3875669-1-foss+uboot@0leil.net/,
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.

Cheers,
Quentin