Re: [PATCH 1/4] arm64: dts: imx8mn: Fix FSPI booting

[Marek Vasut <marex@denx.de>]

[-- Attachment #1: Type: text/plain, Size: 2976 bytes --]

On 11/10/24 6:21 PM, Adam Ford wrote:
> On Sun, Nov 10, 2024 at 10:42 AM Marek Vasut <marex@denx.de> wrote:
>>
>> On 11/10/24 2:15 PM, Adam Ford wrote:
>>> On Sat, Nov 9, 2024 at 7:34 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 11/9/24 9:06 PM, Adam Ford wrote:
>>>>> When FSPI_CONF_HEADER is set, the binary needs to be built such
>>>>> that there is a configuration file located at 0x400 and the start
>>>>> of the file that would normally be flash.bin starts at 0x1000.
>>>>> This used to be done properly until the device tree was converted to
>>>>> nxp_imx8mimage.
>>>>>
>>>>> Building these with the offsets built into the binman device tree
>>>>> changes impacts how the actual image is built and the locations
>>>>> of the various blobs aren't fetched properly and booting fails.
>>>>>
>>>>> Fix this by building a standard image as if it were to boot from
>>>>> eMMC or SD, then use that image as the input for a second image
>>>>
>>>> This seems like a workaround for some broken offset calculation in binman ?
>>>
>>> This used to work until it was migrated to nxp_imx8mimage.
>>> The blobs appear to be at the proper offsets, but the contents of
>>> what's stored at those offsets are not the same.
>>
>> I know, this is what Lukasz reported too.
>>
>>> If you're going to claim there is a bug somewhere, I would argue that
>>> it's somewhere i nxp_imx8mimage
>>
>> I agree with that claim. Well, by extension, the problem might also be
>> in binman itself.
>>
>>> .  However, if you look at this series,
>>> the added benefit is the ability for Nano to be able to build both a
>>> SD/eMMC image and FSPI images with one config which allows for the
>>> elimination of extra defconfig files.  I am guessing Plus would have a
>>> similar benefit since they have similar bootloaders.
>> This I do not agree with. If the intent is to generate two images, then
>> there should be two full binman descriptors, one for each image (one for
>> flash-plain.bin and one for flash-fspi.bin or some such naming).
>>
>> Can you try and fix the FSPI generation first, so an FSPI compatible
> 
> I am not a python programmer, and I couldn't determine what was going on.
I am hoping Simon could offer some input here ...

Can you try the attached diff on MX8MM (use "git show -w" to view the 
diff better) ? It will generate two files, flash.bin and flash-fspi.bin 
, the later should have the fspi header and maybe even correct offsets?

Applies on top of 56accc56b9aa ("bios_emulator: fix first argument of 
pci_{read,write}_config_* function calls") .

I noticed that there is some dependency issue where fspi_header.bin may 
not be generated yet when binman is triggered -- that needs to be fixed. 
You can detect the failure by running 'hexdump -C flash-fspi.bin | head' 
, if there is no FCFB header then this failure occurred. The easiest way 
to work around this is to run 'rm flash.bin ; make flash.bin'. The real 
fix is likely a matter of some Makefile tweak.

[-- Attachment #2: fspi.diff --]
[-- Type: text/x-patch, Size: 7317 bytes --]

diff --git a/arch/arm/dts/imx8mm-u-boot.dtsi b/arch/arm/dts/imx8mm-u-boot.dtsi
index d31bc822532..74eac34b863 100644
--- a/arch/arm/dts/imx8mm-u-boot.dtsi
+++ b/arch/arm/dts/imx8mm-u-boot.dtsi
@@ -42,165 +42,165 @@
 };
 
 &binman {
-	filename = "flash.bin";
-	section {
-		pad-byte = <0x00>;
-
 #ifdef CONFIG_FSPI_CONF_HEADER
+	filename = "flash-fspi.bin";
+	section {
 		fspi_conf_block {
 			filename = CONFIG_FSPI_CONF_FILE;
 			type = "blob-ext";
 			size = <0x1000>;
 		};
-#endif
 
-#ifdef CONFIG_IMX_HAB
-		nxp-imx8mcst@0 {
-			filename = "u-boot-spl-mkimage.signed.bin";
-			nxp,loader-address = <CONFIG_SPL_TEXT_BASE>;
-			nxp,unlock;
-			args;	/* Needed by mkimage etype superclass */
+		section {
 #endif
+			filename = "flash.bin";
 
-			binman_imx_spl: nxp-imx8mimage {
-				filename = "u-boot-spl-mkimage.bin";
-				nxp,boot-from = "sd";
-				nxp,rom-version = <1>;
-				nxp,loader-address = <CONFIG_SPL_TEXT_BASE>;
-				args;	/* Needed by mkimage etype superclass */
-
-				section {
-					align = <4>;
-					align-size = <4>;
-					filename = "u-boot-spl-ddr.bin";
-					pad-byte = <0xff>;
-
-					u-boot-spl {
-						align-end = <4>;
-						filename = "u-boot-spl.bin";
-					};
-
-					ddr-1d-imem-fw {
-						filename = "lpddr4_pmu_train_1d_imem.bin";
-						align-end = <4>;
-						type = "blob-ext";
-					};
-
-					ddr-1d-dmem-fw {
-						filename = "lpddr4_pmu_train_1d_dmem.bin";
-						align-end = <4>;
-						type = "blob-ext";
-					};
+			section {
+				pad-byte = <0x00>;
 
-					ddr-2d-imem-fw {
-						filename = "lpddr4_pmu_train_2d_imem.bin";
-						align-end = <4>;
-						type = "blob-ext";
-					};
+#ifdef CONFIG_IMX_HAB
+				nxp-imx8mcst@0 {
+					filename = "u-boot-spl-mkimage.signed.bin";
+					nxp,loader-address = <CONFIG_SPL_TEXT_BASE>;
+					nxp,unlock;
+					args;	/* Needed by mkimage etype superclass */
+#endif
 
-					ddr-2d-dmem-fw {
-						filename = "lpddr4_pmu_train_2d_dmem.bin";
-						align-end = <4>;
-						type = "blob-ext";
+					binman_imx_spl: nxp-imx8mimage {
+						filename = "u-boot-spl-mkimage.bin";
+						nxp,boot-from = "sd";
+						nxp,rom-version = <1>;
+						nxp,loader-address = <CONFIG_SPL_TEXT_BASE>;
+						args;	/* Needed by mkimage etype superclass */
+
+						section {
+							align = <4>;
+							align-size = <4>;
+							filename = "u-boot-spl-ddr.bin";
+							pad-byte = <0xff>;
+
+							u-boot-spl {
+								align-end = <4>;
+								filename = "u-boot-spl.bin";
+							};
+
+							ddr-1d-imem-fw {
+								filename = "lpddr4_pmu_train_1d_imem.bin";
+								align-end = <4>;
+								type = "blob-ext";
+							};
+
+							ddr-1d-dmem-fw {
+								filename = "lpddr4_pmu_train_1d_dmem.bin";
+								align-end = <4>;
+								type = "blob-ext";
+							};
+
+							ddr-2d-imem-fw {
+								filename = "lpddr4_pmu_train_2d_imem.bin";
+								align-end = <4>;
+								type = "blob-ext";
+							};
+
+							ddr-2d-dmem-fw {
+								filename = "lpddr4_pmu_train_2d_dmem.bin";
+								align-end = <4>;
+								type = "blob-ext";
+							};
+						};
 					};
-				};
-			};
 #ifdef CONFIG_IMX_HAB
-		};
+				};
 
-		nxp-imx8mcst@1 {
-			filename = "u-boot-fit.signed.bin";
-			nxp,loader-address = <CONFIG_SPL_LOAD_FIT_ADDRESS>;
-#ifdef CONFIG_FSPI_CONF_HEADER
-			offset = <0x58C00>;
-#else
-			offset = <0x57c00>;
-#endif
+				nxp-imx8mcst@1 {
+					filename = "u-boot-fit.signed.bin";
+					nxp,loader-address = <CONFIG_SPL_LOAD_FIT_ADDRESS>;
+					offset = <0x57c00>;
 
-			args;	/* Needed by mkimage etype superclass */
+					args;	/* Needed by mkimage etype superclass */
 #endif
 
-			binman_imx_fit: fit {
-				description = "Configuration to load ATF before U-Boot";
-				filename = "u-boot.itb";
+					binman_imx_fit: fit {
+						description = "Configuration to load ATF before U-Boot";
+						filename = "u-boot.itb";
 #ifndef CONFIG_IMX_HAB
-				fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>;
+						fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>;
 #endif
-				fit,fdt-list = "of-list";
-				#address-cells = <1>;
-#ifdef CONFIG_FSPI_CONF_HEADER
-				offset = <0x58C00>;
-#else
-				offset = <0x57c00>;
-#endif
-
-				images {
-					uboot {
-						arch = "arm64";
-						compression = "none";
-						description = "U-Boot (64-bit)";
-						load = <CONFIG_TEXT_BASE>;
-						type = "standalone";
-
-						uboot-blob {
-							filename = "u-boot-nodtb.bin";
-							type = "blob-ext";
-						};
-					};
+						fit,fdt-list = "of-list";
+						#address-cells = <1>;
+						offset = <0x57c00>;
+
+						images {
+							uboot {
+								arch = "arm64";
+								compression = "none";
+								description = "U-Boot (64-bit)";
+								load = <CONFIG_TEXT_BASE>;
+								type = "standalone";
+
+								uboot-blob {
+									filename = "u-boot-nodtb.bin";
+									type = "blob-ext";
+								};
+							};
 
 #ifndef CONFIG_ARMV8_PSCI
-					atf {
-						arch = "arm64";
-						compression = "none";
-						description = "ARM Trusted Firmware";
-						entry = <0x920000>;
-						load = <0x920000>;
-						type = "firmware";
-
-						atf-blob {
-							filename = "bl31.bin";
-							type = "atf-bl31";
-						};
-					};
+							atf {
+								arch = "arm64";
+								compression = "none";
+								description = "ARM Trusted Firmware";
+								entry = <0x920000>;
+								load = <0x920000>;
+								type = "firmware";
+
+								atf-blob {
+									filename = "bl31.bin";
+									type = "atf-bl31";
+								};
+							};
 #endif
 
-					binman_fip: fip {
-						arch = "arm64";
-						compression = "none";
-						description = "Trusted Firmware FIP";
-						load = <0x40310000>;
-						type = "firmware";
-					};
-
-					@fdt-SEQ {
-						compression = "none";
-						description = "NAME";
-						type = "flat_dt";
-
-						uboot-fdt-blob {
-							filename = "u-boot.dtb";
-							type = "blob-ext";
+							binman_fip: fip {
+								arch = "arm64";
+								compression = "none";
+								description = "Trusted Firmware FIP";
+								load = <0x40310000>;
+								type = "firmware";
+							};
+
+							@fdt-SEQ {
+								compression = "none";
+								description = "NAME";
+								type = "flat_dt";
+
+								uboot-fdt-blob {
+									filename = "u-boot.dtb";
+									type = "blob-ext";
+								};
+							};
 						};
-					};
-				};
 
-				configurations {
-					default = "@config-DEFAULT-SEQ";
+						configurations {
+							default = "@config-DEFAULT-SEQ";
 
-					@config-SEQ {
-						description = "NAME";
-						fdt = "fdt-SEQ";
-						firmware = "uboot";
+							@config-SEQ {
+								description = "NAME";
+								fdt = "fdt-SEQ";
+								firmware = "uboot";
 #ifndef CONFIG_ARMV8_PSCI
-						loadables = "atf";
+								loadables = "atf";
 #endif
+							};
+						};
 					};
+#ifdef CONFIG_IMX_HAB
 				};
+#endif
 			};
-#ifdef CONFIG_IMX_HAB
+#ifdef CONFIG_FSPI_CONF_HEADER
 		};
-#endif
 	};
+#endif
 };
 
 &clk {
diff --git a/tools/binman/etype/nxp_imx8mimage.py b/tools/binman/etype/nxp_imx8mimage.py
index 8ad177b3b65..e57da68079f 100644
--- a/tools/binman/etype/nxp_imx8mimage.py
+++ b/tools/binman/etype/nxp_imx8mimage.py
@@ -72,4 +72,6 @@ class Entry_nxp_imx8mimage(Entry_mkimage):
                 return
             upto += entry.size
 
+        # FIXME: Maybe ?
+        image_pos = 0
         Entry_section.SetImagePos(self, image_pos)