[PATCH v2 2/3] arm: dts: rockchip: Separate rockchip-binman.dtsi

[Jagan Teki]

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://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?

< 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
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.

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.

There are pros and cons to each solution but solution 1 seems to be
maintainable at this point in time and this patches doing that. I
understand more matureness to binman can simplify things but that can
be a separate discussion as of now.

Hope you understand.

Thanks,
Jagan.