Hi Jagan,

On 11/3/22 13:37, Jagan Teki wrote:

On Thu, 3 Nov 2022 at 15:32, Quentin Schulz quentin.schulz@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.GD... 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

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@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@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/mast... ) 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.GD... 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