Hi Simon,

On 7/26/22 21:58, Simon Glass wrote:
Hi Quentin,

On Tue, 26 Jul 2022 at 03:08, Quentin Schulz
<quentin.sch...@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+ub...@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

Reply via email to