[PATCH] arm64: zynqmp: Use CONFIG_SPL_FS_LOAD_PAYLOAD_NAME in binman

[Quentin Schulz]

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?

> 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 at 2 which points at u-boot.itb). Isn't this 
u-boot.itb generated by the /binman/itb image node?

> 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 at 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 at 2 {
  				offset = <CONFIG_SYS_SPI_U_BOOT_OFFS>;
-				filename = "u-boot.itb";
+				filename = U_BOOT_ITB_FILENAME;
  			};
  			fdtmap {
  			};
"""

Cheers,
Quentin