[PATCH v2 2/3] arm: dts: rockchip: Separate rockchip-binman.dtsi
Quentin Schulz
quentin.schulz at theobroma-systems.com
Mon Nov 7 16:26:30 CET 2022
Hi Jagan,
On 11/4/22 05:17, Jagan Teki wrote:
> Hi Quentin,
>
> On Thu, 3 Nov 2022 at 18:51, Quentin Schulz
> <quentin.schulz at theobroma-systems.com> wrote:
>>
>> Hi Jagan,
>>
>> On 11/3/22 13:37, Jagan Teki wrote:
>>> On Thu, 3 Nov 2022 at 15:32, Quentin Schulz
>>> <quentin.schulz at theobroma-systems.com> wrote:
>>>>
>>>> Hi Jagan,
>>>>
>>>> On 11/3/22 07:19, Jagan Teki wrote:
>>>>> rockchip-u-boot.dtsi has the FIT image for the final stage of
>>>>> binman image creation.
>>>>>
>>>>> If the actual binman node is part of this dtsi then there are
>>>>> build issues to use optee as input to this final stage binman
>>>>> image since optee is built via another binman image creation
>>>>> unlike ATF built via tools like make_fit_atf.py.
>>>>>
>>>>> binman: Filename 'u-boot.itb' not found in input path
>>>>>
>>>>> Fix this by separating binman FIT image in rockchip-binman.dtsi
>>>>>
>>>>
>>>> My understanding is that this is a work-around for something that should
>>>> be implemented in binman instead (e.g. dependency between images). If
>>>> i'm not mistaken, what you're suggesting is to not build
>>>> u-boot-rockchip.bin for some platforms? IIRC the plan for this binary
>>>> was that it would apply to all Rockchip platforms, and this patch makes
>>>> this "promise" go away.
>>>
>>> Not really, no functionality is changed. It is just that we cannot
>>> create the final binman image for optee. It is not possible to
>>> implement in binman alone however if you want to add optee binman
>>> prior to the final binman can be solvable but it makes unnecessary
>>> ifdefs and maintaining many binman node definitions in one file seems
>>> confusing and difficult to maintain.
>>>
>>
>> The project does not want us to use a separate script for building the
>> SPL fit image, c.f. the message printed when you build
>> (https://urldefense.com/v3/__https://source.denx.de/u-boot/u-boot/-/blob/master/Makefile*L1134-L1140__;Iw!!OOPJP91ZZw!gqogvHFe6_TxXIlTvQcIrfgTlGSKMX5Fbd3LsWUfHCqgJ9ozw5ShnQJkoV31NcAcL7hu-Cr1BFDYKySB1HZuJuvw40qJ1XI$ )
>> so we'll need to migrate to binman eventually.
>>
>> Patches or suggestions on how to make the binman nodes easier to
>> maintain welcome obviously. That's a different topic though.
>>
>>>>
>>>>> rockchip-u-boot.dtsi: binman node
>>>>> rockchip-binman.dtsi: binman FIT image node
>>>>>
>>>>> The inclusion of rockchip-binman.dtsi is always to be last in
>>>>> included files as it has a FIT image node for final image creation.
>>>>>
>>>>
>>>> You are not respecting this in your patch. Please update or remove this
>>>> section if not required. (I assume you have this limitation because you
>>>> use a binman phandle, meaning the node needs to be defined before).
>>>>
>>>> Also, rockchip-u-boot.dtsi content is now literally:
>>>> / {
>>>> binman: binman {
>>>> multiple-images;
>>>> };
>>>> };
>>>>
>>>> which is pretty much useless.
>>>>
>>>> Since you want to work around your build issue, why just not include
>>>> rockchip-u-boot.dtsi instead of moving part of it to another file
>>>> without any added benefit (at least at first glance, I may be missing
>>>> some context).
>>>>
>>>> BTW, we were discussing some months ago on moving away from
>>>> make_fit_atf.py to binman for all Rockchip platforms, c.f. the long
>>>> discussion here:
>>>> https://urldefense.com/v3/__https://lore.kernel.org/u-boot/20220725172953.GD2029@begut/__;!!OOPJP91ZZw!loeWJQdnvs4xp1KOPE_UekBxO1MVtI8zdMU2brPPR5vPO312JHwp5kdeK2xAzXnMrepRjers3vG5dmMKdVNqzWA2G5WTCZE$ So maybe we
>>>> should just do this and that might fix the problem you're trying to
>>>> work-around?
>>>>
>>>> In any case, can you provide a bit more context on the failing platform(s)?
>>>
>>> As I explained above, the functionality remains unchanged. Even if you
>>> build atf via binam dts files the final binman node has to be in the
>>> order of last since input files like bl31 and tee.bin have depended.
>>
>> Yes, that's something we discussed on the linked topic. Binman would
>> need to gain the ability to express dependencies between nodes.
>> Otherwise, one could also force binman to build images sequentially in
>> which case (AFAIK) the images are created top to bottom in the binman
>> node. It makes the image creation slower but you should get what you want.
>>
>> AFAIK, binman is what we're supposed to use to create U-Boot binaries
>> and binman uses FDT for how to generate them. If there's a better way to
>> configure the FDT without ifdef, feel free to suggest something.
>>
>>> Adding all the binman image creations and the final binman image
>>> creation in one file make it difficult to read and maintain and
>>> unnecessary ifdef.
>>>
>>
>> We'll eventually have to make this migration anyways.
>>
>> Back to the patch.
>>
>> Applying your patch, rockchip-u-boot.dtsi only contains:
>> / {
>> binman: binman {
>> multiple-images;
>> };
>> };
>> This makes very little sense since it is useless and meaningless on its own.
>>
>> You would need to move this node to the newly added rockchip-binman.dtsi
>> which would make this patch just about a file rename. All of this
>> because of a build issue for one platform/SoC (as per my understanding).
>> If you don't want to work on improving binman to support your use case
>> right now, just don't include rockchip-u-boot.dtsi for your platform
>> until what you want is supported?
>
> < answering all together here>
>
> As I said this is not about building issue on a specific platform.
>
> Let's consider the situation w/o this patch and assume u-boot.itb
> creation for tf-a was done via binam.
>
> The optee image creation is already part of binman in
> rockchip-optee.dtsi assumes it is optee-binman
It is not "the optee image" it is a fitImage which happens to contain
OP-TEE OS in it, but it is not *only* OP-TEE (you have u-boot-nodtb.bin
and u-boot fdt too). However, I concede that the name is confusing.
> The tf-a image creation can be part of binman in rockchip-tf-a.dtsi
> assumes it is tf-a-binam
> and final image u-boot-rockchip.bin creation is part of
> rockchip-binman.dtsi assumes it is final-binman.
>
> In order to build final-binman both the optee-binman and tf-a-binman
> have to be built prior.
>
1) This is not possible currently since binman compiles all images in
parallel and there's no inter-image dependency.
2) This also does not make much sense since U-Boot does not build OP-TEE
OS nor TF-A, it merely injects them into a fitImage loaded by the SPL. I
don't think we want a binman dtsi per possible combination of loadables
(one for tf-a, one for tf-a+op-tee, one for no tf-a (Aarch32) but
op-tee, one with none of them (Aarch32)).
Considering point 2), there seems to be a misunderstanding somewhere.
What am I missing?
> Solutions: 1
> - Keep maintaining individual binam in respective files.
> - Add binman node in rockchip-u-boot.dtsi (This is needed as the
> binman node is updating and used in later files or we can add this on
> individual binman image creation files - assume we do a later step)
> - Include the rockchip-binman.dtsi at last in the list as it is the
> final image creation.
>
> For example, a platform with tf-a
> #include "rockchip-tf-a.dtsi"
> #include "rockcihp-binman.dtsi"
>
> For examples, a platform with spl-optee
> #include "rockchip-optee.dtsi"
> #include "rockcihp-binman.dtsi"
>
> Solution:2
> Add all binman-related files in one file and add proper ifdef to control.
>
You can have a separate file per binman image if you want and include it
only when needed. That would be e.g. one binman dtsi for
u-boot-rockchip.bin and one for u-boot-rockchip-spi.bin.
FWIW, rockchip-optee.dtsi seems like a mistake to me and should be
merged with rockchip-u-boot.dtsi.
I agree that ifdefs don't make the dtsi easy to read and error-prone
when doing changes to it. The issue is that there's usually a lot in
common between dtsis when you split things to avoid ifdefs (and to have
some generic support, I guess the ifdef will just be added to the SoC
include list to know which binman dtsi to include?).
I feel like I don't understand what you're suggesting to fix and how.
Finally, if this patch series is to be continued, I believe you should
just not have a rockchip-u-boot.dtsi and have in rockchip-binman.dtsi:
/ {
binman: binman {
multiple-images;
whatever is in &binman currently
};
};
same for rockchip-optee.dtsi:
/ {
binman: binman {
multiple-images;
whatever is in &binman currently
};
};
The point being that the dtsi for just setting multiple-images and have
the node label is a bit silly. Also because it could be possible to
include rockchip-optee.dtsi without rockchip-u-boot.dtsi by just adding
a binman: binman {} node, which is incorrect since we need this
multiple-images property.
Cheers,
Quentin
More information about the U-Boot
mailing list