Re: [PATCH] arm64: zynqmp: Use CONFIG_SPL_FS_LOAD_PAYLOAD_NAME in binman

[Michal Simek <michal.simek@amd.com>]

On 3/28/25 16:08, Quentin Schulz wrote:
> Hi Michal,
> 
> On 3/27/25 10:11 AM, Michal Simek wrote:
>> Hi,
>>
>> On 3/25/25 10:48, Quentin Schulz wrote:
>>> Hi Michal,
>>>
>>> On 3/19/25 2:08 PM, Michal Simek wrote:
>>>> u-boot.itb name is coming via CONFIG_SPL_FS_LOAD_PAYLOAD_NAME and it's
>>>
>>> They are unrelated as far as I can tell?
>>>
>>> Isn't CONFIG_SPL_FS_LOAD_PAYLOAD_NAME the name of the U-Boot FIT that the SPL 
>>> is going to load from some filesystem?
>>
>> yes.
>>
>>>
>>> u-boot.itb in the binman nodes below is the name of the generated U- Boot FIT 
>>> on your disk.
>>>
>>> Your disk FS != SPL FS, no?
>>>
>>> I can get behind wanting the same name for the file on both FS, but the issue 
>>> I have is the additional CONFIG_SPL_FS_LOAD_PAYLOAD_NAME #ifdef will prevent 
>>> the generation of this U-Boot FIT image if the symbol isn't defined anymore 
>>> (which is possible if SPL has no support for FS (ext4, fat, squashfs, 
>>> semihosting)). U- Boot likely work just fine by trying to load u-boot.itb 
>>> from another source no? (No experience with AMD, typically only Aarch64 for 
>>> me, where u-boot.itb is usually loaded from a specific offset in the storage 
>>> medium, outside of any FS).
>>>
>>> Basically, do we really need to NOT build U-Boot FIT if 
>>> SPL_FS_LOAD_PAYLOAD_NAME is not defined?
>>
>> There are couple of things here. AMD/Xilinx evaluation boards are capable to 
>> boot from multiple devices.
>>
>> On ZynqMP we have one official supported flow which is FSBL or alternative SPL.
>>
>> In FSBL boot mode FIT image is not handled and boot image contains just u- 
>> boot.elf and that's it and no matter which boot mode you use.
>>
>> In SPL boot mode boot image (boot.bin) has only SPL and that's it. Then FIT 
>> image should be created to be able to start TF-A, U-Boot, etc.
>>
>> Then based on boot device there are in general two options which are raw mode 
>> (for example SPI) and then filesystem one (SD or EMMC).
>>
> 
> So no raw mode for anything that is not SPI? E.g. no raw mode on SD or eMMC?

There is also NAND raw but it never gets popular.

SD/EMMC no raw boot mode on ZynqMP. (Versal and never has it).

> 
>> In RAW mode qspi image is created (maybe we can rename it because it can be 
>> also used for NAND for example) and image is expected at certain offset and 
>> name obviously doesn't matter.
>>
> 
> But you still need a u-boot.itb to be available during build (c.f. /binman/ 
> image/blob-ext@2 which points at u-boot.itb). Isn't this u-boot.itb generated by 
> the /binman/itb image node?

it is and the patch is addressing it.

> 
>> But in filesystem mode names matter and image should match 
>> CONFIG_SPL_FS_LOAD_PAYLOAD_NAME because that's the filename which SPL is 
>> looking for that's why these has to match.
>>
> 
> That's fine. I'm arguing about the fact that you are not only renaming the u- 
> boot.itb to the expected filename on the filesystem but also entirely skipping 
> building it if the filename is not provided, which is not what's done today.
> 
> Essentially, I'm wondering if you don't want:
> 
> """
> diff --git a/arch/arm/dts/zynqmp-binman-som.dts b/arch/arm/dts/zynqmp-binman- 
> som.dts
> index d5b63ef604b..e73f196611f 100644
> --- a/arch/arm/dts/zynqmp-binman-som.dts
> +++ b/arch/arm/dts/zynqmp-binman-som.dts
> @@ -9,6 +9,12 @@
> 
>   #include <config.h>
> 
> +#if defined(CONFIG_SPL_FS_LOAD_PAYLOAD_NAME)
> +#define U_BOOT_ITB_FILENAME CONFIG_SPL_FS_LOAD_PAYLOAD_NAME
> +#else
> +#define U_BOOT_ITB_FILENAME "u-boot.itb"
> +#endif
> +
>   /dts-v1/;
>   / {
>       binman: binman {
> @@ -105,7 +111,7 @@
> 
>           /* u-boot.itb generation in a static way */
>           itb {
> -            filename = "u-boot.itb";
> +            filename = U_BOOT_ITB_FILENAME;
>               pad-byte = <0>;
> 
>               fit {
> @@ -227,7 +233,7 @@
>               };
>               blob-ext@2 {
>                   offset = <CONFIG_SYS_SPI_U_BOOT_OFFS>;
> -                filename = "u-boot.itb";
> +                filename = U_BOOT_ITB_FILENAME;
>               };
>               fdtmap {
>               };
> diff --git a/arch/arm/dts/zynqmp-binman.dts b/arch/arm/dts/zynqmp-binman.dts
> index 252c2ad552b..89dfbf4e936 100644
> --- a/arch/arm/dts/zynqmp-binman.dts
> +++ b/arch/arm/dts/zynqmp-binman.dts
> @@ -9,6 +9,12 @@
> 
>   #include <config.h>
> 
> +#if defined(CONFIG_SPL_FS_LOAD_PAYLOAD_NAME)
> +#define U_BOOT_ITB_FILENAME CONFIG_SPL_FS_LOAD_PAYLOAD_NAME
> +#else
> +#define U_BOOT_ITB_FILENAME "u-boot.itb"
> +#endif
> +
>   /dts-v1/;
>   / {
>       binman: binman {
> @@ -17,7 +23,7 @@
>   #ifdef CONFIG_SPL
>           /* u-boot.itb generation in a static way */
>           itb {
> -            filename = "u-boot.itb";
> +            filename = CONFIG_SPL_FS_LOAD_PAYLOAD_NAME;
>               pad-byte = <0>;
> 
>               fit {
> @@ -196,7 +202,7 @@
>               };
>               blob-ext@2 {
>                   offset = <CONFIG_SYS_SPI_U_BOOT_OFFS>;
> -                filename = "u-boot.itb";
> +                filename = U_BOOT_ITB_FILENAME;
>               };
>               fdtmap {
>               };
> """

You are right that when FS functionality is disabled there is no reason not to 
build images for qspi which are not affected. I will send v2 to address it.

Thanks for review,
Michal