Hi Quentin, On Mon, 1 Aug 2022 at 11:05, Quentin Schulz <quentin.sch...@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.sch...@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.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://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+ub...@0leil.net/