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

[Quentin Schulz <quentin.schulz@theobroma-systems.com>]

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