Hi Jagan,
On 11/3/22 13:37, Jagan Teki wrote:
On Thu, 3 Nov 2022 at 15:32, Quentin Schulz
<quentin.sch...@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://source.denx.de/u-boot/u-boot/-/blob/master/Makefile#L1134-L1140)
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?
Cheers,
Quentin
