[PATCH v4 00/10] rockchip: binman: Use a template for FIT and other improvements

[PATCH v4 00/10] rockchip: binman: Use a template for FIT and other improvements

[Jonas Karlman]

This series split out generic Rockchip binman image related commits from
Simons "VBE serial part H: Implement VBE on Rockchip RK3399" [1] series.

These are generic binman image patches fixing an existing issue [2][3]
and that other series [4] already depends on.

They are being split out to ease continued work for other U-Boot
developers as the main parts of the VBE series have not yet been tested
using the mainline U-Boot tree, so cannot yet be seen as ready.

Changes in v4:
- Patch 1-7 matches corresponding patch in Simons series, possible with
  minor updates in commit messages.
- Patch 8 drops changes to defconfig-files.
- Patch 9 adds support for using crc32 as checksum algo.
- Patch 10 adds a default value for SPL_PAD_TO.

This series holds off on any defconfig changes to let it be handled by
next "configs: Resync with savedefconfig". Changing SPL_PAD_TO to be
storage media relative has also been held off and is something that can
be picked up in a series for a future release after this has landed in a
release.

See [5] for a copy of this series on top of patch 1-2 from [3]. This
series does not depend on those two patches, but are related for a full
cleanup of use of u-boot.itb in Rockchip binman images.

[1] https://lore.kernel.org/u-boot/20250328153522.3555472-1-sjg@chromium.org/
[2] https://lore.kernel.org/u-boot/20250129132529.807031-3-naoki@radxa.com/
[3] https://lore.kernel.org/u-boot/20250220-has_rom-u-boot-rockchip-spi-bin-v2-3-d1768ee87808@cherry.de/
[4] https://lore.kernel.org/u-boot/20250220231358.432367-1-jonas@kwiboo.se/
[5] https://source.denx.de/u-boot/contributors/kwiboo/u-boot/-/commits/binman-image-refactor

Jonas Karlman (2):
  rockchip: binman: Support use of crc32 for SPL_FIT_SIGNATURE
  rockchip: Add SPL_PAD_TO Kconfig default value

Simon Glass (8):
  rockchip: binman: Correct the OS prop for U-Boot
  rockchip: binman: Factor out arch and compression
  rockchip: binman: Add an fdtmap
  rockchip: binman: Create a template for the FIT
  rockchip: binman: Un-indent the FIT template
  rockchip: binman: Use the FIT template in the SPI image
  rockchip: binman: Include a compatible string in each configuration
  rockchip: binman: Use the skip-at-start prop in simple-bin image

 arch/arm/dts/rockchip-u-boot.dtsi | 296 ++++++++++++++++--------------
 common/spl/Kconfig                |   1 +
 2 files changed, 162 insertions(+), 135 deletions(-)

-- 
2.49.0

[PATCH v4 01/10] rockchip: binman: Correct the OS prop for U-Boot

[Jonas Karlman]

From: Simon Glass <sjg@chromium.org>

The U-Boot image is currently being identified as an invalid OS in
spl_fit_image_get_os() due to case sensitive compare.

Use the correct lower-case value to fix this.

Fixes: e0c0efff2a02 ("rockchip: Support building the all output files in binman")
Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v4:
- Update commit message
- Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
---
 arch/arm/dts/rockchip-u-boot.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index c8c928c7e508..e9ed1d4b5738 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -50,7 +50,7 @@
 				u-boot {
 					description = "U-Boot";
 					type = "standalone";
-					os = "U-Boot";
+					os = "u-boot";
 #ifdef CONFIG_ARM64
 					arch = "arm64";
 #else
-- 
2.49.0

[PATCH v4 02/10] rockchip: binman: Factor out arch and compression

[Jonas Karlman]

From: Simon Glass <sjg@chromium.org>

Declare arch and compression at the top of the file to avoid needing
ifdefs in every usage.

Add a few comments to help with the remaining #ifdefs.

Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v4:
- Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
---
 arch/arm/dts/rockchip-u-boot.dtsi | 44 +++++++++++++++----------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index e9ed1d4b5738..2b01dc660562 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -5,6 +5,20 @@
 
 #include <config.h>
 
+#ifdef CONFIG_ARM64
+#define ARCH	"arm64"
+#else
+#define ARCH	"arm"
+#endif
+
+#if defined(CONFIG_SPL_GZIP)
+#define COMP	"gzip"
+#elif defined(CONFIG_SPL_LZMA)
+#define COMP	"lzma"
+#else
+#define COMP	"none"
+#endif
+
 / {
 	binman: binman {
 		multiple-images;
@@ -51,26 +65,12 @@
 					description = "U-Boot";
 					type = "standalone";
 					os = "u-boot";
-#ifdef CONFIG_ARM64
-					arch = "arm64";
-#else
-					arch = "arm";
-#endif
-#if defined(CONFIG_SPL_GZIP)
-					compression = "gzip";
-#elif defined(CONFIG_SPL_LZMA)
-					compression = "lzma";
-#else
-					compression = "none";
-#endif
+					arch = ARCH;
+					compression = COMP;
 					load = <CONFIG_TEXT_BASE>;
 					entry = <CONFIG_TEXT_BASE>;
 					u-boot-nodtb {
-#if defined(CONFIG_SPL_GZIP)
-					compress = "gzip";
-#elif defined(CONFIG_SPL_LZMA)
-					compress = "lzma";
-#endif
+					compress = COMP;
 					};
 #ifdef CONFIG_SPL_FIT_SIGNATURE
 					hash {
@@ -84,7 +84,7 @@
 					fit,operation = "split-elf";
 					description = "ARM Trusted Firmware";
 					type = "firmware";
-					arch = "arm64";
+					arch = ARCH;
 					os = "arm-trusted-firmware";
 					compression = "none";
 					fit,load;
@@ -103,7 +103,7 @@
 					fit,operation = "split-elf";
 					description = "TEE";
 					type = "tee";
-					arch = "arm64";
+					arch = ARCH;
 					os = "tee";
 					compression = "none";
 					fit,load;
@@ -119,11 +119,11 @@
 					};
 #endif
 				};
-#else
+#else /* !CONFIG_ARM64 */
 				op-tee {
 					description = "OP-TEE";
 					type = "tee";
-					arch = "arm";
+					arch = ARCH;
 					os = "tee";
 					compression = "none";
 					load = <(CFG_SYS_SDRAM_BASE + 0x8400000)>;
@@ -137,7 +137,7 @@
 					};
 #endif
 				};
-#endif
+#endif /* CONFIG_ARM64 */
 
 				@fdt-SEQ {
 					description = "fdt-NAME";
-- 
2.49.0

[PATCH v4 03/10] rockchip: binman: Add an fdtmap

[Jonas Karlman]

From: Simon Glass <sjg@chromium.org>

Add an fdtmap so it is possible to look at the image with 'binman ls'.

Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v4:
- Split from "VBE serial part H: Implement VBE on Rockchip RK3399"

Changes in v3:
- Add blank lines before the node
---
 arch/arm/dts/rockchip-u-boot.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index 2b01dc660562..8aea2e6f5713 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -170,6 +170,9 @@
 			offset = <CONFIG_SPL_PAD_TO>;
 		};
 #endif
+
+		fdtmap {
+		};
 	};
 
 #ifdef CONFIG_ROCKCHIP_SPI_IMAGE
@@ -203,6 +206,9 @@
 			/* Sync with u-boot,spl-payload-offset if present */
 			offset = <CONFIG_SYS_SPI_U_BOOT_OFFS>;
 		};
+
+		fdtmap {
+		};
 	};
 #endif /* CONFIG_ROCKCHIP_SPI_IMAGE */
 };
-- 
2.49.0

[PATCH v4 04/10] rockchip: binman: Create a template for the FIT

[Jonas Karlman]

From: Simon Glass <sjg@chromium.org>

Move the FIT description into a template so that it can (later) be used
in multiple places in the image.

Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v4:
- Rename template label to fit_template
- Split from "VBE serial part H: Implement VBE on Rockchip RK3399"

Changes in v3:
- Use HAS_FIT for the SPI node also
- Leave fit { node open within #ifdef HAS_FIT
- Move placement of CONFIG_SPL_PAD_TO
- Keep the FIT filename
---
 arch/arm/dts/rockchip-u-boot.dtsi | 61 ++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 26 deletions(-)

diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index 8aea2e6f5713..9ef1ca6cd13a 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -19,6 +19,10 @@
 #define COMP	"none"
 #endif
 
+#if defined(CONFIG_SPL_FIT) && (defined(CONFIG_ARM64) || defined(CONFIG_SPL_OPTEE_IMAGE))
+#define HAS_FIT
+#endif
+
 / {
 	binman: binman {
 		multiple-images;
@@ -27,28 +31,9 @@
 
 #ifdef CONFIG_SPL
 &binman {
-	simple-bin {
-		filename = "u-boot-rockchip.bin";
-		pad-byte = <0xff>;
-
-		mkimage {
-			filename = "idbloader.img";
-			args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
-			multiple-data-files;
-
-#ifdef CONFIG_ROCKCHIP_EXTERNAL_TPL
-			rockchip-tpl {
-			};
-#elif defined(CONFIG_TPL)
-			u-boot-tpl {
-			};
-#endif
-			u-boot-spl {
-			};
-		};
-
-#if defined(CONFIG_SPL_FIT) && (defined(CONFIG_ARM64) || defined(CONFIG_SPL_OPTEE_IMAGE))
-		fit: fit {
+#ifdef HAS_FIT
+	fit_template: template-1 {
+			type = "fit";
 #ifdef CONFIG_ARM64
 			description = "FIT image for U-Boot with bl31 (TF-A)";
 #else
@@ -56,10 +41,8 @@
 #endif
 			#address-cells = <1>;
 			fit,fdt-list = "of-list";
-			filename = "u-boot.itb";
 			fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>;
 			fit,align = <512>;
-			offset = <CONFIG_SPL_PAD_TO>;
 			images {
 				u-boot {
 					description = "U-Boot";
@@ -164,12 +147,38 @@
 					fit,loadables;
 				};
 			};
+	};
+#endif /* HAS_FIT */
+
+	simple-bin {
+		filename = "u-boot-rockchip.bin";
+		pad-byte = <0xff>;
+
+		mkimage {
+			filename = "idbloader.img";
+			args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
+			multiple-data-files;
+
+#ifdef CONFIG_ROCKCHIP_EXTERNAL_TPL
+			rockchip-tpl {
+			};
+#elif defined(CONFIG_TPL)
+			u-boot-tpl {
+			};
+#endif
+			u-boot-spl {
+			};
 		};
+
+#ifdef HAS_FIT
+		fit {
+			filename = "u-boot.itb";
+			insert-template = <&fit_template>;
 #else
 		u-boot-img {
+#endif
 			offset = <CONFIG_SPL_PAD_TO>;
 		};
-#endif
 
 		fdtmap {
 		};
@@ -196,7 +205,7 @@
 			};
 		};
 
-#if defined(CONFIG_ARM64) || defined(CONFIG_SPL_OPTEE_IMAGE)
+#ifdef HAS_FIT
 		fit {
 			type = "blob";
 			filename = "u-boot.itb";
-- 
2.49.0

[PATCH v4 05/10] rockchip: binman: Un-indent the FIT template

[Jonas Karlman]

From: Simon Glass <sjg@chromium.org>

Fix the indentation on the template. This is done in a separate patch
so that it is easier to review.

Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v4:
- Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
---
 arch/arm/dts/rockchip-u-boot.dtsi | 176 +++++++++++++++---------------
 1 file changed, 88 insertions(+), 88 deletions(-)

diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index 9ef1ca6cd13a..f860bfc1ba72 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -33,120 +33,120 @@
 &binman {
 #ifdef HAS_FIT
 	fit_template: template-1 {
-			type = "fit";
+		type = "fit";
 #ifdef CONFIG_ARM64
-			description = "FIT image for U-Boot with bl31 (TF-A)";
+		description = "FIT image for U-Boot with bl31 (TF-A)";
 #else
-			description = "FIT image with OP-TEE";
+		description = "FIT image with OP-TEE";
 #endif
-			#address-cells = <1>;
-			fit,fdt-list = "of-list";
-			fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>;
-			fit,align = <512>;
-			images {
-				u-boot {
-					description = "U-Boot";
-					type = "standalone";
-					os = "u-boot";
-					arch = ARCH;
-					compression = COMP;
-					load = <CONFIG_TEXT_BASE>;
-					entry = <CONFIG_TEXT_BASE>;
-					u-boot-nodtb {
+		#address-cells = <1>;
+		fit,fdt-list = "of-list";
+		fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>;
+		fit,align = <512>;
+		images {
+			u-boot {
+				description = "U-Boot";
+				type = "standalone";
+				os = "u-boot";
+				arch = ARCH;
+				compression = COMP;
+				load = <CONFIG_TEXT_BASE>;
+				entry = <CONFIG_TEXT_BASE>;
+				u-boot-nodtb {
 					compress = COMP;
-					};
+				};
 #ifdef CONFIG_SPL_FIT_SIGNATURE
-					hash {
-						algo = "sha256";
-					};
-#endif
+				hash {
+					algo = "sha256";
 				};
+#endif
+			};
 
 #ifdef CONFIG_ARM64
-				@atf-SEQ {
-					fit,operation = "split-elf";
-					description = "ARM Trusted Firmware";
-					type = "firmware";
-					arch = ARCH;
-					os = "arm-trusted-firmware";
-					compression = "none";
-					fit,load;
-					fit,entry;
-					fit,data;
-
-					atf-bl31 {
-					};
+			@atf-SEQ {
+				fit,operation = "split-elf";
+				description = "ARM Trusted Firmware";
+				type = "firmware";
+				arch = ARCH;
+				os = "arm-trusted-firmware";
+				compression = "none";
+				fit,load;
+				fit,entry;
+				fit,data;
+
+				atf-bl31 {
+				};
 #ifdef CONFIG_SPL_FIT_SIGNATURE
-					hash {
-						algo = "sha256";
-					};
+				hash {
+					algo = "sha256";
+				};
 #endif
+			};
+			@tee-SEQ {
+				fit,operation = "split-elf";
+				description = "TEE";
+				type = "tee";
+				arch = ARCH;
+				os = "tee";
+				compression = "none";
+				fit,load;
+				fit,entry;
+				fit,data;
+
+				tee-os {
+					optional;
 				};
-				@tee-SEQ {
-					fit,operation = "split-elf";
-					description = "TEE";
-					type = "tee";
-					arch = ARCH;
-					os = "tee";
-					compression = "none";
-					fit,load;
-					fit,entry;
-					fit,data;
-
-					tee-os {
-						optional;
-					};
 #ifdef CONFIG_SPL_FIT_SIGNATURE
-					hash {
-						algo = "sha256";
-					};
-#endif
+				hash {
+					algo = "sha256";
 				};
+#endif
+			};
 #else /* !CONFIG_ARM64 */
-				op-tee {
-					description = "OP-TEE";
-					type = "tee";
-					arch = ARCH;
-					os = "tee";
-					compression = "none";
-					load = <(CFG_SYS_SDRAM_BASE + 0x8400000)>;
-					entry = <(CFG_SYS_SDRAM_BASE + 0x8400000)>;
-
-					tee-os {
-					};
+			op-tee {
+				description = "OP-TEE";
+				type = "tee";
+				arch = ARCH;
+				os = "tee";
+				compression = "none";
+				load = <(CFG_SYS_SDRAM_BASE + 0x8400000)>;
+				entry = <(CFG_SYS_SDRAM_BASE + 0x8400000)>;
+
+				tee-os {
+				};
 #ifdef CONFIG_SPL_FIT_SIGNATURE
-					hash {
-						algo = "sha256";
-					};
-#endif
+				hash {
+					algo = "sha256";
 				};
+#endif
+			};
 #endif /* CONFIG_ARM64 */
 
-				@fdt-SEQ {
-					description = "fdt-NAME";
-					compression = "none";
-					type = "flat_dt";
+			@fdt-SEQ {
+				description = "fdt-NAME";
+				compression = "none";
+				type = "flat_dt";
 #ifdef CONFIG_SPL_FIT_SIGNATURE
-					hash {
-						algo = "sha256";
-					};
-#endif
+				hash {
+					algo = "sha256";
 				};
+#endif
 			};
+		};
 
-			configurations {
-				default = "@config-DEFAULT-SEQ";
-				@config-SEQ {
-					description = "NAME.dtb";
-					fdt = "fdt-SEQ";
+		configurations {
+			default = "@config-DEFAULT-SEQ";
+			@config-SEQ {
+				description = "NAME.dtb";
+				fdt = "fdt-SEQ";
 #ifdef CONFIG_ARM64
-					fit,firmware = "atf-1", "u-boot";
+				fit,firmware = "atf-1", "u-boot";
 #else
-					fit,firmware = "op-tee", "u-boot";
+				fit,firmware = "op-tee", "u-boot";
 #endif
-					fit,loadables;
-				};
+				fit,loadables;
 			};
+		};
 	};
 #endif /* HAS_FIT */
 
-- 
2.49.0

[PATCH v4 06/10] rockchip: binman: Use the FIT template in the SPI image

[Jonas Karlman]

From: Simon Glass <sjg@chromium.org>

At present simple-bin-spi relies on the u-boot.itb file created by the
simple-bin image. Use the template to avoid this, since Binman may
change to process images in parallel in the future.

Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v4:
- Drop filename for the SPI FIT
- Split from "VBE serial part H: Implement VBE on Rockchip RK3399"

Changes in v3:
- Keep the filename for the SPI FIT

This is an alternative for the issue reported and fixed in [1] and [2].

[1] https://lore.kernel.org/u-boot/20250129132529.807031-3-naoki@radxa.com/
[2] https://lore.kernel.org/u-boot/20250220-has_rom-u-boot-rockchip-spi-bin-v2-3-d1768ee87808@cherry.de/
---
 arch/arm/dts/rockchip-u-boot.dtsi | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index f860bfc1ba72..73149b2d8bdd 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -207,8 +207,7 @@
 
 #ifdef HAS_FIT
 		fit {
-			type = "blob";
-			filename = "u-boot.itb";
+			insert-template = <&fit_template>;
 #else
 		u-boot-img {
 #endif
-- 
2.49.0

[PATCH v4 07/10] rockchip: binman: Include a compatible string in each configuration

[Jonas Karlman]

From: Simon Glass <sjg@chromium.org>

Provide a compatible string in the config nodes that U-Boot can use to
help decide which configuration to use.

Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v4:
- Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
---
 arch/arm/dts/rockchip-u-boot.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index 73149b2d8bdd..fb38b7b80c43 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -145,6 +145,7 @@
 				fit,firmware = "op-tee", "u-boot";
 #endif
 				fit,loadables;
+				fit,compatible;
 			};
 		};
 	};
-- 
2.49.0

[PATCH v4 08/10] rockchip: binman: Use the skip-at-start prop in simple-bin image

[Jonas Karlman]

From: Simon Glass <sjg@chromium.org>

The simple-bin image is normally written to MMC media at block 64, which
is a 32K offset from start of storage media.

Set the skip-at-start property to 0x8000 (32 KiB) so that fdtmap and
other embedded binman symbols in the output binary is referencing image
offsets correctly.

Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v4:
- Drop defconfig changes
- Split from "VBE serial part H: Implement VBE on Rockchip RK3399"

Changes in v2:
- Move this patch to the end of the series
- Drop 0x8000 offset for SPI
---
 arch/arm/dts/rockchip-u-boot.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index fb38b7b80c43..65b81bf58626 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -154,6 +154,7 @@
 	simple-bin {
 		filename = "u-boot-rockchip.bin";
 		pad-byte = <0xff>;
+		skip-at-start = <0x8000>;
 
 		mkimage {
 			filename = "idbloader.img";
@@ -178,7 +179,7 @@
 #else
 		u-boot-img {
 #endif
-			offset = <CONFIG_SPL_PAD_TO>;
+			offset = <(CONFIG_SPL_PAD_TO + 0x8000)>;
 		};
 
 		fdtmap {
-- 
2.49.0

[PATCH v4 09/10] rockchip: binman: Support use of crc32 for SPL_FIT_SIGNATURE

[Jonas Karlman]

Use of SHA256 checksum validation on ARMv7 SoCs can be very time
consuming compared to ARMv8 SoCs with Crypto Extensions.

Add support for use of the crc32 hash algo when SHA256 is not supported.
Also use a HAS_HASH to simplify the ifdefs when no known hash algo is
compiled.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v4:
- New patch
---
 arch/arm/dts/rockchip-u-boot.dtsi | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index 65b81bf58626..0dfd45bb9bed 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -19,10 +19,20 @@
 #define COMP	"none"
 #endif
 
+#if defined(CONFIG_SPL_SHA256)
+#define HASH	"sha256"
+#elif defined(CONFIG_SPL_CRC32)
+#define HASH	"crc32"
+#endif
+
 #if defined(CONFIG_SPL_FIT) && (defined(CONFIG_ARM64) || defined(CONFIG_SPL_OPTEE_IMAGE))
 #define HAS_FIT
 #endif
 
+#if defined(CONFIG_SPL_FIT_SIGNATURE) && defined(HASH)
+#define HAS_HASH
+#endif
+
 / {
 	binman: binman {
 		multiple-images;
@@ -55,9 +65,9 @@
 				u-boot-nodtb {
 					compress = COMP;
 				};
-#ifdef CONFIG_SPL_FIT_SIGNATURE
+#ifdef HAS_HASH
 				hash {
-					algo = "sha256";
+					algo = HASH;
 				};
 #endif
 			};
@@ -76,9 +86,9 @@
 
 				atf-bl31 {
 				};
-#ifdef CONFIG_SPL_FIT_SIGNATURE
+#ifdef HAS_HASH
 				hash {
-					algo = "sha256";
+					algo = HASH;
 				};
 #endif
 			};
@@ -96,9 +106,9 @@
 				tee-os {
 					optional;
 				};
-#ifdef CONFIG_SPL_FIT_SIGNATURE
+#ifdef HAS_HASH
 				hash {
-					algo = "sha256";
+					algo = HASH;
 				};
 #endif
 			};
@@ -114,9 +124,9 @@
 
 				tee-os {
 				};
-#ifdef CONFIG_SPL_FIT_SIGNATURE
+#ifdef HAS_HASH
 				hash {
-					algo = "sha256";
+					algo = HASH;
 				};
 #endif
 			};
@@ -126,9 +136,9 @@
 				description = "fdt-NAME";
 				compression = "none";
 				type = "flat_dt";
-#ifdef CONFIG_SPL_FIT_SIGNATURE
+#ifdef HAS_HASH
 				hash {
-					algo = "sha256";
+					algo = HASH;
 				};
 #endif
 			};
-- 
2.49.0

[PATCH v4 10/10] rockchip: Add SPL_PAD_TO Kconfig default value

[Jonas Karlman]

Almost all Rockchip boards use the same Kconfig value for SPL_PAD_TO,
0x7f8000.

Add this value as a default value for ARCH_ROCKCHIP.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v4:
- New patch

This patch does not drop SPL_PAD_TO from existing defconfigs that would
be affected by this change, something that can wait until next "configs:
Resync with savedefconfig".

Following boards does not set a SPL_PAD_TO due to not using SPL:
- elgin-rv1108_defconfig
- evb-rk3128_defconfig
- evb-rv1108_defconfig
- geekbox_defconfig
- sheep-rk3368_defconfig
---
 common/spl/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/common/spl/Kconfig b/common/spl/Kconfig
index 97f542fcc8a5..322fc3cbdced 100644
--- a/common/spl/Kconfig
+++ b/common/spl/Kconfig
@@ -97,6 +97,7 @@ config SPL_MAX_SIZE
 
 config SPL_PAD_TO
 	hex "Offset to which the SPL should be padded before appending the SPL payload"
+	default 0x7f8000 if ARCH_ROCKCHIP
 	default 0x31000 if ARCH_MX6 && MX6_OCRAM_256KB
 	default 0x11000 if ARCH_MX7 || (ARCH_MX6 && !MX6_OCRAM_256KB)
 	default 0x10000 if ARCH_KEYSTONE
-- 
2.49.0

Re: [PATCH v4 01/10] rockchip: binman: Correct the OS prop for U-Boot

[Kever Yang]

On 2025/3/29 23:06, Jonas Karlman wrote:
> From: Simon Glass <sjg@chromium.org>
>
> The U-Boot image is currently being identified as an invalid OS in
> spl_fit_image_get_os() due to case sensitive compare.
>
> Use the correct lower-case value to fix this.
>
> Fixes: e0c0efff2a02 ("rockchip: Support building the all output files in binman")
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
> Changes in v4:
> - Update commit message
> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
> ---
>   arch/arm/dts/rockchip-u-boot.dtsi | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> index c8c928c7e508..e9ed1d4b5738 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -50,7 +50,7 @@
>   				u-boot {
>   					description = "U-Boot";
>   					type = "standalone";
> -					os = "U-Boot";
> +					os = "u-boot";
>   #ifdef CONFIG_ARM64
>   					arch = "arm64";
>   #else

Re: [PATCH v4 02/10] rockchip: binman: Factor out arch and compression

[Kever Yang]

On 2025/3/29 23:06, Jonas Karlman wrote:
> From: Simon Glass <sjg@chromium.org>
>
> Declare arch and compression at the top of the file to avoid needing
> ifdefs in every usage.
>
> Add a few comments to help with the remaining #ifdefs.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
> Changes in v4:
> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
> ---
>   arch/arm/dts/rockchip-u-boot.dtsi | 44 +++++++++++++++----------------
>   1 file changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> index e9ed1d4b5738..2b01dc660562 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -5,6 +5,20 @@
>   
>   #include <config.h>
>   
> +#ifdef CONFIG_ARM64
> +#define ARCH	"arm64"
> +#else
> +#define ARCH	"arm"
> +#endif
> +
> +#if defined(CONFIG_SPL_GZIP)
> +#define COMP	"gzip"
> +#elif defined(CONFIG_SPL_LZMA)
> +#define COMP	"lzma"
> +#else
> +#define COMP	"none"
> +#endif
> +
>   / {
>   	binman: binman {
>   		multiple-images;
> @@ -51,26 +65,12 @@
>   					description = "U-Boot";
>   					type = "standalone";
>   					os = "u-boot";
> -#ifdef CONFIG_ARM64
> -					arch = "arm64";
> -#else
> -					arch = "arm";
> -#endif
> -#if defined(CONFIG_SPL_GZIP)
> -					compression = "gzip";
> -#elif defined(CONFIG_SPL_LZMA)
> -					compression = "lzma";
> -#else
> -					compression = "none";
> -#endif
> +					arch = ARCH;
> +					compression = COMP;
>   					load = <CONFIG_TEXT_BASE>;
>   					entry = <CONFIG_TEXT_BASE>;
>   					u-boot-nodtb {
> -#if defined(CONFIG_SPL_GZIP)
> -					compress = "gzip";
> -#elif defined(CONFIG_SPL_LZMA)
> -					compress = "lzma";
> -#endif
> +					compress = COMP;
>   					};
>   #ifdef CONFIG_SPL_FIT_SIGNATURE
>   					hash {
> @@ -84,7 +84,7 @@
>   					fit,operation = "split-elf";
>   					description = "ARM Trusted Firmware";
>   					type = "firmware";
> -					arch = "arm64";
> +					arch = ARCH;
>   					os = "arm-trusted-firmware";
>   					compression = "none";
>   					fit,load;
> @@ -103,7 +103,7 @@
>   					fit,operation = "split-elf";
>   					description = "TEE";
>   					type = "tee";
> -					arch = "arm64";
> +					arch = ARCH;
>   					os = "tee";
>   					compression = "none";
>   					fit,load;
> @@ -119,11 +119,11 @@
>   					};
>   #endif
>   				};
> -#else
> +#else /* !CONFIG_ARM64 */
>   				op-tee {
>   					description = "OP-TEE";
>   					type = "tee";
> -					arch = "arm";
> +					arch = ARCH;
>   					os = "tee";
>   					compression = "none";
>   					load = <(CFG_SYS_SDRAM_BASE + 0x8400000)>;
> @@ -137,7 +137,7 @@
>   					};
>   #endif
>   				};
> -#endif
> +#endif /* CONFIG_ARM64 */
>   
>   				@fdt-SEQ {
>   					description = "fdt-NAME";

Re: [PATCH v4 03/10] rockchip: binman: Add an fdtmap

[Kever Yang]

On 2025/3/29 23:06, Jonas Karlman wrote:
> From: Simon Glass <sjg@chromium.org>
>
> Add an fdtmap so it is possible to look at the image with 'binman ls'.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
> Changes in v4:
> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
>
> Changes in v3:
> - Add blank lines before the node
> ---
>   arch/arm/dts/rockchip-u-boot.dtsi | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> index 2b01dc660562..8aea2e6f5713 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -170,6 +170,9 @@
>   			offset = <CONFIG_SPL_PAD_TO>;
>   		};
>   #endif
> +
> +		fdtmap {
> +		};
>   	};
>   
>   #ifdef CONFIG_ROCKCHIP_SPI_IMAGE
> @@ -203,6 +206,9 @@
>   			/* Sync with u-boot,spl-payload-offset if present */
>   			offset = <CONFIG_SYS_SPI_U_BOOT_OFFS>;
>   		};
> +
> +		fdtmap {
> +		};
>   	};
>   #endif /* CONFIG_ROCKCHIP_SPI_IMAGE */
>   };

Re: [PATCH v4 04/10] rockchip: binman: Create a template for the FIT

[Kever Yang]

On 2025/3/29 23:06, Jonas Karlman wrote:
> From: Simon Glass <sjg@chromium.org>
>
> Move the FIT description into a template so that it can (later) be used
> in multiple places in the image.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
> Changes in v4:
> - Rename template label to fit_template
> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
>
> Changes in v3:
> - Use HAS_FIT for the SPI node also
> - Leave fit { node open within #ifdef HAS_FIT
> - Move placement of CONFIG_SPL_PAD_TO
> - Keep the FIT filename
> ---
>   arch/arm/dts/rockchip-u-boot.dtsi | 61 ++++++++++++++++++-------------
>   1 file changed, 35 insertions(+), 26 deletions(-)
>
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> index 8aea2e6f5713..9ef1ca6cd13a 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -19,6 +19,10 @@
>   #define COMP	"none"
>   #endif
>   
> +#if defined(CONFIG_SPL_FIT) && (defined(CONFIG_ARM64) || defined(CONFIG_SPL_OPTEE_IMAGE))
> +#define HAS_FIT
> +#endif
> +
>   / {
>   	binman: binman {
>   		multiple-images;
> @@ -27,28 +31,9 @@
>   
>   #ifdef CONFIG_SPL
>   &binman {
> -	simple-bin {
> -		filename = "u-boot-rockchip.bin";
> -		pad-byte = <0xff>;
> -
> -		mkimage {
> -			filename = "idbloader.img";
> -			args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
> -			multiple-data-files;
> -
> -#ifdef CONFIG_ROCKCHIP_EXTERNAL_TPL
> -			rockchip-tpl {
> -			};
> -#elif defined(CONFIG_TPL)
> -			u-boot-tpl {
> -			};
> -#endif
> -			u-boot-spl {
> -			};
> -		};
> -
> -#if defined(CONFIG_SPL_FIT) && (defined(CONFIG_ARM64) || defined(CONFIG_SPL_OPTEE_IMAGE))
> -		fit: fit {
> +#ifdef HAS_FIT
> +	fit_template: template-1 {
> +			type = "fit";
>   #ifdef CONFIG_ARM64
>   			description = "FIT image for U-Boot with bl31 (TF-A)";
>   #else
> @@ -56,10 +41,8 @@
>   #endif
>   			#address-cells = <1>;
>   			fit,fdt-list = "of-list";
> -			filename = "u-boot.itb";
>   			fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>;
>   			fit,align = <512>;
> -			offset = <CONFIG_SPL_PAD_TO>;
>   			images {
>   				u-boot {
>   					description = "U-Boot";
> @@ -164,12 +147,38 @@
>   					fit,loadables;
>   				};
>   			};
> +	};
> +#endif /* HAS_FIT */
> +
> +	simple-bin {
> +		filename = "u-boot-rockchip.bin";
> +		pad-byte = <0xff>;
> +
> +		mkimage {
> +			filename = "idbloader.img";
> +			args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
> +			multiple-data-files;
> +
> +#ifdef CONFIG_ROCKCHIP_EXTERNAL_TPL
> +			rockchip-tpl {
> +			};
> +#elif defined(CONFIG_TPL)
> +			u-boot-tpl {
> +			};
> +#endif
> +			u-boot-spl {
> +			};
>   		};
> +
> +#ifdef HAS_FIT
> +		fit {
> +			filename = "u-boot.itb";
> +			insert-template = <&fit_template>;
>   #else
>   		u-boot-img {
> +#endif
>   			offset = <CONFIG_SPL_PAD_TO>;
>   		};
> -#endif
>   
>   		fdtmap {
>   		};
> @@ -196,7 +205,7 @@
>   			};
>   		};
>   
> -#if defined(CONFIG_ARM64) || defined(CONFIG_SPL_OPTEE_IMAGE)
> +#ifdef HAS_FIT
>   		fit {
>   			type = "blob";
>   			filename = "u-boot.itb";

Re: [PATCH v4 05/10] rockchip: binman: Un-indent the FIT template

[Kever Yang]

On 2025/3/29 23:06, Jonas Karlman wrote:
> From: Simon Glass <sjg@chromium.org>
>
> Fix the indentation on the template. This is done in a separate patch
> so that it is easier to review.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
> Changes in v4:
> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
> ---
>   arch/arm/dts/rockchip-u-boot.dtsi | 176 +++++++++++++++---------------
>   1 file changed, 88 insertions(+), 88 deletions(-)
>
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> index 9ef1ca6cd13a..f860bfc1ba72 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -33,120 +33,120 @@
>   &binman {
>   #ifdef HAS_FIT
>   	fit_template: template-1 {
> -			type = "fit";
> +		type = "fit";
>   #ifdef CONFIG_ARM64
> -			description = "FIT image for U-Boot with bl31 (TF-A)";
> +		description = "FIT image for U-Boot with bl31 (TF-A)";
>   #else
> -			description = "FIT image with OP-TEE";
> +		description = "FIT image with OP-TEE";
>   #endif
> -			#address-cells = <1>;
> -			fit,fdt-list = "of-list";
> -			fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>;
> -			fit,align = <512>;
> -			images {
> -				u-boot {
> -					description = "U-Boot";
> -					type = "standalone";
> -					os = "u-boot";
> -					arch = ARCH;
> -					compression = COMP;
> -					load = <CONFIG_TEXT_BASE>;
> -					entry = <CONFIG_TEXT_BASE>;
> -					u-boot-nodtb {
> +		#address-cells = <1>;
> +		fit,fdt-list = "of-list";
> +		fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>;
> +		fit,align = <512>;
> +		images {
> +			u-boot {
> +				description = "U-Boot";
> +				type = "standalone";
> +				os = "u-boot";
> +				arch = ARCH;
> +				compression = COMP;
> +				load = <CONFIG_TEXT_BASE>;
> +				entry = <CONFIG_TEXT_BASE>;
> +				u-boot-nodtb {
>   					compress = COMP;
> -					};
> +				};
>   #ifdef CONFIG_SPL_FIT_SIGNATURE
> -					hash {
> -						algo = "sha256";
> -					};
> -#endif
> +				hash {
> +					algo = "sha256";
>   				};
> +#endif
> +			};
>   
>   #ifdef CONFIG_ARM64
> -				@atf-SEQ {
> -					fit,operation = "split-elf";
> -					description = "ARM Trusted Firmware";
> -					type = "firmware";
> -					arch = ARCH;
> -					os = "arm-trusted-firmware";
> -					compression = "none";
> -					fit,load;
> -					fit,entry;
> -					fit,data;
> -
> -					atf-bl31 {
> -					};
> +			@atf-SEQ {
> +				fit,operation = "split-elf";
> +				description = "ARM Trusted Firmware";
> +				type = "firmware";
> +				arch = ARCH;
> +				os = "arm-trusted-firmware";
> +				compression = "none";
> +				fit,load;
> +				fit,entry;
> +				fit,data;
> +
> +				atf-bl31 {
> +				};
>   #ifdef CONFIG_SPL_FIT_SIGNATURE
> -					hash {
> -						algo = "sha256";
> -					};
> +				hash {
> +					algo = "sha256";
> +				};
>   #endif
> +			};
> +			@tee-SEQ {
> +				fit,operation = "split-elf";
> +				description = "TEE";
> +				type = "tee";
> +				arch = ARCH;
> +				os = "tee";
> +				compression = "none";
> +				fit,load;
> +				fit,entry;
> +				fit,data;
> +
> +				tee-os {
> +					optional;
>   				};
> -				@tee-SEQ {
> -					fit,operation = "split-elf";
> -					description = "TEE";
> -					type = "tee";
> -					arch = ARCH;
> -					os = "tee";
> -					compression = "none";
> -					fit,load;
> -					fit,entry;
> -					fit,data;
> -
> -					tee-os {
> -						optional;
> -					};
>   #ifdef CONFIG_SPL_FIT_SIGNATURE
> -					hash {
> -						algo = "sha256";
> -					};
> -#endif
> +				hash {
> +					algo = "sha256";
>   				};
> +#endif
> +			};
>   #else /* !CONFIG_ARM64 */
> -				op-tee {
> -					description = "OP-TEE";
> -					type = "tee";
> -					arch = ARCH;
> -					os = "tee";
> -					compression = "none";
> -					load = <(CFG_SYS_SDRAM_BASE + 0x8400000)>;
> -					entry = <(CFG_SYS_SDRAM_BASE + 0x8400000)>;
> -
> -					tee-os {
> -					};
> +			op-tee {
> +				description = "OP-TEE";
> +				type = "tee";
> +				arch = ARCH;
> +				os = "tee";
> +				compression = "none";
> +				load = <(CFG_SYS_SDRAM_BASE + 0x8400000)>;
> +				entry = <(CFG_SYS_SDRAM_BASE + 0x8400000)>;
> +
> +				tee-os {
> +				};
>   #ifdef CONFIG_SPL_FIT_SIGNATURE
> -					hash {
> -						algo = "sha256";
> -					};
> -#endif
> +				hash {
> +					algo = "sha256";
>   				};
> +#endif
> +			};
>   #endif /* CONFIG_ARM64 */
>   
> -				@fdt-SEQ {
> -					description = "fdt-NAME";
> -					compression = "none";
> -					type = "flat_dt";
> +			@fdt-SEQ {
> +				description = "fdt-NAME";
> +				compression = "none";
> +				type = "flat_dt";
>   #ifdef CONFIG_SPL_FIT_SIGNATURE
> -					hash {
> -						algo = "sha256";
> -					};
> -#endif
> +				hash {
> +					algo = "sha256";
>   				};
> +#endif
>   			};
> +		};
>   
> -			configurations {
> -				default = "@config-DEFAULT-SEQ";
> -				@config-SEQ {
> -					description = "NAME.dtb";
> -					fdt = "fdt-SEQ";
> +		configurations {
> +			default = "@config-DEFAULT-SEQ";
> +			@config-SEQ {
> +				description = "NAME.dtb";
> +				fdt = "fdt-SEQ";
>   #ifdef CONFIG_ARM64
> -					fit,firmware = "atf-1", "u-boot";
> +				fit,firmware = "atf-1", "u-boot";
>   #else
> -					fit,firmware = "op-tee", "u-boot";
> +				fit,firmware = "op-tee", "u-boot";
>   #endif
> -					fit,loadables;
> -				};
> +				fit,loadables;
>   			};
> +		};
>   	};
>   #endif /* HAS_FIT */
>   

Re: [PATCH v4 06/10] rockchip: binman: Use the FIT template in the SPI image

[Kever Yang]

On 2025/3/29 23:06, Jonas Karlman wrote:
> From: Simon Glass <sjg@chromium.org>
>
> At present simple-bin-spi relies on the u-boot.itb file created by the
> simple-bin image. Use the template to avoid this, since Binman may
> change to process images in parallel in the future.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
> Changes in v4:
> - Drop filename for the SPI FIT
> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
>
> Changes in v3:
> - Keep the filename for the SPI FIT
>
> This is an alternative for the issue reported and fixed in [1] and [2].
>
> [1] https://lore.kernel.org/u-boot/20250129132529.807031-3-naoki@radxa.com/
> [2] https://lore.kernel.org/u-boot/20250220-has_rom-u-boot-rockchip-spi-bin-v2-3-d1768ee87808@cherry.de/
> ---
>   arch/arm/dts/rockchip-u-boot.dtsi | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> index f860bfc1ba72..73149b2d8bdd 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -207,8 +207,7 @@
>   
>   #ifdef HAS_FIT
>   		fit {
> -			type = "blob";
> -			filename = "u-boot.itb";
> +			insert-template = <&fit_template>;
>   #else
>   		u-boot-img {
>   #endif

Re: [PATCH v4 07/10] rockchip: binman: Include a compatible string in each configuration

[Kever Yang]

On 2025/3/29 23:06, Jonas Karlman wrote:
> From: Simon Glass <sjg@chromium.org>
>
> Provide a compatible string in the config nodes that U-Boot can use to
> help decide which configuration to use.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
> Changes in v4:
> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
> ---
>   arch/arm/dts/rockchip-u-boot.dtsi | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> index 73149b2d8bdd..fb38b7b80c43 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -145,6 +145,7 @@
>   				fit,firmware = "op-tee", "u-boot";
>   #endif
>   				fit,loadables;
> +				fit,compatible;
>   			};
>   		};
>   	};

Re: [PATCH v4 08/10] rockchip: binman: Use the skip-at-start prop in simple-bin image

[Kever Yang]

On 2025/3/29 23:06, Jonas Karlman wrote:
> From: Simon Glass <sjg@chromium.org>
>
> The simple-bin image is normally written to MMC media at block 64, which
> is a 32K offset from start of storage media.
>
> Set the skip-at-start property to 0x8000 (32 KiB) so that fdtmap and
> other embedded binman symbols in the output binary is referencing image
> offsets correctly.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
> Changes in v4:
> - Drop defconfig changes
> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
>
> Changes in v2:
> - Move this patch to the end of the series
> - Drop 0x8000 offset for SPI
> ---
>   arch/arm/dts/rockchip-u-boot.dtsi | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> index fb38b7b80c43..65b81bf58626 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -154,6 +154,7 @@
>   	simple-bin {
>   		filename = "u-boot-rockchip.bin";
>   		pad-byte = <0xff>;
> +		skip-at-start = <0x8000>;
>   
>   		mkimage {
>   			filename = "idbloader.img";
> @@ -178,7 +179,7 @@
>   #else
>   		u-boot-img {
>   #endif
> -			offset = <CONFIG_SPL_PAD_TO>;
> +			offset = <(CONFIG_SPL_PAD_TO + 0x8000)>;
>   		};
>   
>   		fdtmap {

Re: [PATCH v4 09/10] rockchip: binman: Support use of crc32 for SPL_FIT_SIGNATURE

[Kever Yang]

On 2025/3/29 23:06, Jonas Karlman wrote:
> Use of SHA256 checksum validation on ARMv7 SoCs can be very time
> consuming compared to ARMv8 SoCs with Crypto Extensions.
>
> Add support for use of the crc32 hash algo when SHA256 is not supported.
> Also use a HAS_HASH to simplify the ifdefs when no known hash algo is
> compiled.
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
> Changes in v4:
> - New patch
> ---
>   arch/arm/dts/rockchip-u-boot.dtsi | 30 ++++++++++++++++++++----------
>   1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> index 65b81bf58626..0dfd45bb9bed 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -19,10 +19,20 @@
>   #define COMP	"none"
>   #endif
>   
> +#if defined(CONFIG_SPL_SHA256)
> +#define HASH	"sha256"
> +#elif defined(CONFIG_SPL_CRC32)
> +#define HASH	"crc32"
> +#endif
> +
>   #if defined(CONFIG_SPL_FIT) && (defined(CONFIG_ARM64) || defined(CONFIG_SPL_OPTEE_IMAGE))
>   #define HAS_FIT
>   #endif
>   
> +#if defined(CONFIG_SPL_FIT_SIGNATURE) && defined(HASH)
> +#define HAS_HASH
> +#endif
> +
>   / {
>   	binman: binman {
>   		multiple-images;
> @@ -55,9 +65,9 @@
>   				u-boot-nodtb {
>   					compress = COMP;
>   				};
> -#ifdef CONFIG_SPL_FIT_SIGNATURE
> +#ifdef HAS_HASH
>   				hash {
> -					algo = "sha256";
> +					algo = HASH;
>   				};
>   #endif
>   			};
> @@ -76,9 +86,9 @@
>   
>   				atf-bl31 {
>   				};
> -#ifdef CONFIG_SPL_FIT_SIGNATURE
> +#ifdef HAS_HASH
>   				hash {
> -					algo = "sha256";
> +					algo = HASH;
>   				};
>   #endif
>   			};
> @@ -96,9 +106,9 @@
>   				tee-os {
>   					optional;
>   				};
> -#ifdef CONFIG_SPL_FIT_SIGNATURE
> +#ifdef HAS_HASH
>   				hash {
> -					algo = "sha256";
> +					algo = HASH;
>   				};
>   #endif
>   			};
> @@ -114,9 +124,9 @@
>   
>   				tee-os {
>   				};
> -#ifdef CONFIG_SPL_FIT_SIGNATURE
> +#ifdef HAS_HASH
>   				hash {
> -					algo = "sha256";
> +					algo = HASH;
>   				};
>   #endif
>   			};
> @@ -126,9 +136,9 @@
>   				description = "fdt-NAME";
>   				compression = "none";
>   				type = "flat_dt";
> -#ifdef CONFIG_SPL_FIT_SIGNATURE
> +#ifdef HAS_HASH
>   				hash {
> -					algo = "sha256";
> +					algo = HASH;
>   				};
>   #endif
>   			};

Re: [PATCH v4 10/10] rockchip: Add SPL_PAD_TO Kconfig default value

[Kever Yang]

On 2025/3/29 23:06, Jonas Karlman wrote:
> Almost all Rockchip boards use the same Kconfig value for SPL_PAD_TO,
> 0x7f8000.
>
> Add this value as a default value for ARCH_ROCKCHIP.
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
> Changes in v4:
> - New patch
>
> This patch does not drop SPL_PAD_TO from existing defconfigs that would
> be affected by this change, something that can wait until next "configs:
> Resync with savedefconfig".
>
> Following boards does not set a SPL_PAD_TO due to not using SPL:
> - elgin-rv1108_defconfig
> - evb-rk3128_defconfig
> - evb-rv1108_defconfig
> - geekbox_defconfig
> - sheep-rk3368_defconfig
> ---
>   common/spl/Kconfig | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> index 97f542fcc8a5..322fc3cbdced 100644
> --- a/common/spl/Kconfig
> +++ b/common/spl/Kconfig
> @@ -97,6 +97,7 @@ config SPL_MAX_SIZE
>   
>   config SPL_PAD_TO
>   	hex "Offset to which the SPL should be padded before appending the SPL payload"
> +	default 0x7f8000 if ARCH_ROCKCHIP
>   	default 0x31000 if ARCH_MX6 && MX6_OCRAM_256KB
>   	default 0x11000 if ARCH_MX7 || (ARCH_MX6 && !MX6_OCRAM_256KB)
>   	default 0x10000 if ARCH_KEYSTONE

Re: [PATCH v4 01/10] rockchip: binman: Correct the OS prop for U-Boot

[Quentin Schulz]

Hi Jonas, Simon,

On 3/29/25 4:06 PM, Jonas Karlman wrote:
> From: Simon Glass <sjg@chromium.org>
> 
> The U-Boot image is currently being identified as an invalid OS in
> spl_fit_image_get_os() due to case sensitive compare.
> 
> Use the correct lower-case value to fix this.
> 
> Fixes: e0c0efff2a02 ("rockchip: Support building the all output files in binman")
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> Changes in v4:
> - Update commit message
> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
> ---
>   arch/arm/dts/rockchip-u-boot.dtsi | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> index c8c928c7e508..e9ed1d4b5738 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -50,7 +50,7 @@
>   				u-boot {
>   					description = "U-Boot";
>   					type = "standalone";
> -					os = "U-Boot";
> +					os = "u-boot";

It seems like it's not the only place where this is wrong?

$ git grep 'os.*=.*U-Boot'
arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi: 
             os = "U-Boot";
arch/arm/dts/k3-am65-iot2050-boot-image.dtsi: 
             os = "U-Boot";
arch/arm/dts/k3-binman.dtsi:                                        os = 
"U-Boot";
arch/arm/dts/k3-binman.dtsi:                                        os = 
"U-Boot";
arch/arm/dts/k3-j721e-beagleboneai64-u-boot.dtsi: 
                 os = "U-Boot";
arch/arm/dts/socfpga_soc64_fit-u-boot.dtsi: 
           os = "U-Boot";
arch/riscv/dts/binman.dtsi:                                        os = 
"U-Boot";
tools/binman/entries.rst:                os = "U-Boot";
tools/binman/etype/fit.py:                    os = "U-Boot";
tools/binman/test/343_fit_encrypt_data.dts: 
           os = "U-Boot";
tools/binman/test/344_fit_encrypt_data_no_key.dts: 
                  os = "U-Boot";

So we should probably fix those as well.

Cheers,
Quentin

Re: [PATCH v4 02/10] rockchip: binman: Factor out arch and compression

[Quentin Schulz]

Hi Jonas, Simon,

On 3/29/25 4:06 PM, Jonas Karlman wrote:
> From: Simon Glass <sjg@chromium.org>
> 
> Declare arch and compression at the top of the file to avoid needing
> ifdefs in every usage.
> 
> Add a few comments to help with the remaining #ifdefs.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> Changes in v4:
> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
> ---
>   arch/arm/dts/rockchip-u-boot.dtsi | 44 +++++++++++++++----------------
>   1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> index e9ed1d4b5738..2b01dc660562 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -5,6 +5,20 @@
>   
>   #include <config.h>
>   
> +#ifdef CONFIG_ARM64
> +#define ARCH	"arm64"
> +#else
> +#define ARCH	"arm"
> +#endif
> +

I would refrain from using ARCH here as it's something we already use to 
specify the architecture to build (e.g. make ARCH=arm64 CROSS_COMPILE=...).

Maybe FIT_ARCH?

> +#if defined(CONFIG_SPL_GZIP)
> +#define COMP	"gzip"
> +#elif defined(CONFIG_SPL_LZMA)
> +#define COMP	"lzma"
> +#else
> +#define COMP	"none"
> +#endif
> +

This is specific to the U-Boot proper image node only, maybe we should 
have this constant named more appropriately, e.g. FIT_PROPER_COMP or 
something like that?

Reading the symbols also, I believe their help text is misleading or 
confusing (at least the _GZIP ones, _ZSTD seems okay).

Also, I'm wondering if it actually makes sense to force a specific 
U-Boot proper compression algorithm based on a symbol of what 
decompression algorithms are supported in U-Boot SPL. Like, why should 
gzip be picked if I also have lzma enabled? In any case, nothing related 
to this patch.

>   / {
>   	binman: binman {
>   		multiple-images;
> @@ -51,26 +65,12 @@
>   					description = "U-Boot";
>   					type = "standalone";
>   					os = "u-boot";
> -#ifdef CONFIG_ARM64
> -					arch = "arm64";
> -#else
> -					arch = "arm";
> -#endif
> -#if defined(CONFIG_SPL_GZIP)
> -					compression = "gzip";
> -#elif defined(CONFIG_SPL_LZMA)
> -					compression = "lzma";
> -#else
> -					compression = "none";
> -#endif
> +					arch = ARCH;
> +					compression = COMP;
>   					load = <CONFIG_TEXT_BASE>;
>   					entry = <CONFIG_TEXT_BASE>;
>   					u-boot-nodtb {
> -#if defined(CONFIG_SPL_GZIP)
> -					compress = "gzip";
> -#elif defined(CONFIG_SPL_LZMA)
> -					compress = "lzma";
> -#endif
> +					compress = COMP;
>   					};
>   #ifdef CONFIG_SPL_FIT_SIGNATURE
>   					hash {
> @@ -84,7 +84,7 @@
>   					fit,operation = "split-elf";
>   					description = "ARM Trusted Firmware";
>   					type = "firmware";
> -					arch = "arm64";
> +					arch = ARCH;
>   					os = "arm-trusted-firmware";
>   					compression = "none";
>   					fit,load;
> @@ -103,7 +103,7 @@
>   					fit,operation = "split-elf";
>   					description = "TEE";
>   					type = "tee";
> -					arch = "arm64";
> +					arch = ARCH;
>   					os = "tee";
>   					compression = "none";
>   					fit,load;
> @@ -119,11 +119,11 @@
>   					};
>   #endif
>   				};
> -#else
> +#else /* !CONFIG_ARM64 */
>   				op-tee {
>   					description = "OP-TEE";
>   					type = "tee";
> -					arch = "arm";
> +					arch = ARCH;
>   					os = "tee";
>   					compression = "none";
>   					load = <(CFG_SYS_SDRAM_BASE + 0x8400000)>;

Wondering if we couldn't put some of the Aarch32 and Aarch64 OP-TEE OS 
node(s) in common?

The change is fine, finding a more appropriate name for COMP and ARCH 
and it's all good.

Cheers,
Quentin

Re: [PATCH v4 03/10] rockchip: binman: Add an fdtmap

[Quentin Schulz]

Hi Jonas, Simon,

On 3/29/25 4:06 PM, Jonas Karlman wrote:
> From: Simon Glass <sjg@chromium.org>
> 
> Add an fdtmap so it is possible to look at the image with 'binman ls'.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH v4 04/10] rockchip: binman: Create a template for the FIT

[Quentin Schulz]

Hi Jonas, Simon,

On 3/29/25 4:06 PM, Jonas Karlman wrote:
[...]
> -#if defined(CONFIG_ARM64) || defined(CONFIG_SPL_OPTEE_IMAGE)
> +#ifdef HAS_FIT
>   		fit {
>   			type = "blob";
>   			filename = "u-boot.itb";

Use the template for the SPI here already instead of splitting it in a 
separate commit?

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH v4 05/10] rockchip: binman: Un-indent the FIT template

[Quentin Schulz]

Hi Jonas, Simon,

On 3/29/25 4:06 PM, Jonas Karlman wrote:
> From: Simon Glass <sjg@chromium.org>
> 
> Fix the indentation on the template. This is done in a separate patch
> so that it is easier to review.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH v4 06/10] rockchip: binman: Use the FIT template in the SPI image

[Quentin Schulz]

Hi Jonas, Simon,

On 3/29/25 4:06 PM, Jonas Karlman wrote:
> From: Simon Glass <sjg@chromium.org>
> 
> At present simple-bin-spi relies on the u-boot.itb file created by the
> simple-bin image. Use the template to avoid this, since Binman may
> change to process images in parallel in the future.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> Changes in v4:
> - Drop filename for the SPI FIT
> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
> 
> Changes in v3:
> - Keep the filename for the SPI FIT
> 
> This is an alternative for the issue reported and fixed in [1] and [2].
> 
> [1] https://lore.kernel.org/u-boot/20250129132529.807031-3-naoki@radxa.com/
> [2] https://lore.kernel.org/u-boot/20250220-has_rom-u-boot-rockchip-spi-bin-v2-3-d1768ee87808@cherry.de/

What about squashing with patch 04/10 ?

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH v4 07/10] rockchip: binman: Include a compatible string in each configuration

[Quentin Schulz]

Hi Jonas, Simon,

On 3/29/25 4:06 PM, Jonas Karlman wrote:
> From: Simon Glass <sjg@chromium.org>
> 
> Provide a compatible string in the config nodes that U-Boot can use to
> help decide which configuration to use.
> 

Can you tell us more about this?

I don't think mkimage -l/dumpimage -l actually provide that information 
and the docs are sparse as to what "so that things work correctly with 
FIT's configuration-matching algortihm." means.

Looking a bit into tools/binman/ftest.py it seems like it's a way to 
expose the DT compatible property from within the first entry in `fdt` 
array property (a DTB) into the configuration node in the fit image.

I think it'd make sense to update dumpimage/mkimage/etc... to dump that 
information as well?

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH v4 08/10] rockchip: binman: Use the skip-at-start prop in simple-bin image

[Quentin Schulz]

Hi Jonas, Simon,

On 3/29/25 4:06 PM, Jonas Karlman wrote:
> From: Simon Glass <sjg@chromium.org>
> 
> The simple-bin image is normally written to MMC media at block 64, which
> is a 32K offset from start of storage media.
> 
> Set the skip-at-start property to 0x8000 (32 KiB) so that fdtmap and
> other embedded binman symbols in the output binary is referencing image
> offsets correctly.
> 

Shouldn't we have this commit BEFORE we add the `fdtmap` node since we 
know it's wrong before this commit?

> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> Changes in v4:
> - Drop defconfig changes
> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
> 
> Changes in v2:
> - Move this patch to the end of the series
> - Drop 0x8000 offset for SPI
> ---
>   arch/arm/dts/rockchip-u-boot.dtsi | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> index fb38b7b80c43..65b81bf58626 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -154,6 +154,7 @@
>   	simple-bin {
>   		filename = "u-boot-rockchip.bin";
>   		pad-byte = <0xff>;
> +		skip-at-start = <0x8000>;
>   
>   		mkimage {
>   			filename = "idbloader.img";
> @@ -178,7 +179,7 @@
>   #else
>   		u-boot-img {
>   #endif
> -			offset = <CONFIG_SPL_PAD_TO>;
> +			offset = <(CONFIG_SPL_PAD_TO + 0x8000)>;

This is confusing. The documentation states:

"""
offset:
     This sets the offset of an entry within the image or section containing
     it.
"""

My understanding is that it should be relative to the beginning of the 
image but this now needs the knowledge of where it will be stored on the 
MMC device (via the value in skip-at-start).

Why is skip-at-start automatically deducted from offset?

Cheers,
Quentin

Re: [PATCH v4 09/10] rockchip: binman: Support use of crc32 for SPL_FIT_SIGNATURE

[Quentin Schulz]

Hi Jonas,

On 3/29/25 4:06 PM, Jonas Karlman wrote:
> Use of SHA256 checksum validation on ARMv7 SoCs can be very time
> consuming compared to ARMv8 SoCs with Crypto Extensions.
> 
> Add support for use of the crc32 hash algo when SHA256 is not supported.
> Also use a HAS_HASH to simplify the ifdefs when no known hash algo is
> compiled.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

I don't know enough about general security but this very much looks like 
a bad idea to me.

https://web.archive.org/web/20170210173741/http://www.derkeiler.com/Newsgroups/sci.crypt/2003-07/1451.html

"""
While properly designed CRC's are good at detecting random errors in
the data (due to e.g. line noise), the CRC is useless as a secure
indicator of intentional manipulation of the data. And this is
because it's not hard at all to modify the data to produce any CRC
you desire (e.g. the same CRC as the original data, to try to
disguise your data manipulation).
"""

(yes I took the "first" link my web search engine returned me, thanks 
confirmation bias!).

I don't want to give people a false sense of security. If it really 
comes down to it, I'd rather have an explicit Kconfig symbol to set this 
value (maybe have a `choice` even) and be very clear about security 
implications.

Cheers,
Quentin

Re: [PATCH v4 10/10] rockchip: Add SPL_PAD_TO Kconfig default value

[Quentin Schulz]

Hi Jonas,

On 3/29/25 4:06 PM, Jonas Karlman wrote:
> Almost all Rockchip boards use the same Kconfig value for SPL_PAD_TO,
> 0x7f8000.
> 

Would be nice to say why :)

https://opensource.rock-chips.com/wiki_Partitions

u-boot.itb is typically expected to be in loader2 partition by Rockchip, 
which is at offset 16384S (32KiB). But because the image is flashed at 
offset 64S (32KiB) typically, then we subtract it from the padding, 
which means it's (16384S - 64S) * 512 = 0x7f8000.

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH v4 01/10] rockchip: binman: Correct the OS prop for U-Boot

[Jonas Karlman]

Hi Quentin,

On 2025-04-09 11:15, Quentin Schulz wrote:
> Hi Jonas, Simon,
> 
> On 3/29/25 4:06 PM, Jonas Karlman wrote:
>> From: Simon Glass <sjg@chromium.org>
>>
>> The U-Boot image is currently being identified as an invalid OS in
>> spl_fit_image_get_os() due to case sensitive compare.
>>
>> Use the correct lower-case value to fix this.
>>
>> Fixes: e0c0efff2a02 ("rockchip: Support building the all output files in binman")
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>> Changes in v4:
>> - Update commit message
>> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
>> ---
>>   arch/arm/dts/rockchip-u-boot.dtsi | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
>> index c8c928c7e508..e9ed1d4b5738 100644
>> --- a/arch/arm/dts/rockchip-u-boot.dtsi
>> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
>> @@ -50,7 +50,7 @@
>>   				u-boot {
>>   					description = "U-Boot";
>>   					type = "standalone";
>> -					os = "U-Boot";
>> +					os = "u-boot";
> 
> It seems like it's not the only place where this is wrong?
> 
> $ git grep 'os.*=.*U-Boot'
> arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi: 
>              os = "U-Boot";
> arch/arm/dts/k3-am65-iot2050-boot-image.dtsi: 
>              os = "U-Boot";
> arch/arm/dts/k3-binman.dtsi:                                        os = 
> "U-Boot";
> arch/arm/dts/k3-binman.dtsi:                                        os = 
> "U-Boot";
> arch/arm/dts/k3-j721e-beagleboneai64-u-boot.dtsi: 
>                  os = "U-Boot";
> arch/arm/dts/socfpga_soc64_fit-u-boot.dtsi: 
>            os = "U-Boot";
> arch/riscv/dts/binman.dtsi:                                        os = 
> "U-Boot";
> tools/binman/entries.rst:                os = "U-Boot";
> tools/binman/etype/fit.py:                    os = "U-Boot";
> tools/binman/test/343_fit_encrypt_data.dts: 
>            os = "U-Boot";
> tools/binman/test/344_fit_encrypt_data_no_key.dts: 
>                   os = "U-Boot";
> 
> So we should probably fix those as well.

I agree, this should probably be fixed for all device tree files (in a
separate series).

The default handling in SPL is to do the same thing for unknown and
u-boot image type, so should not be any effect, unless there is a future
code change that may depend on correct image type.

(something I think the VBE part of original series may have done).

Regards,
Jonas

> 
> Cheers,
> Quentin

Re: [PATCH v4 02/10] rockchip: binman: Factor out arch and compression

[Jonas Karlman]

Hi Quentin,

On 2025-04-09 11:28, Quentin Schulz wrote:
> Hi Jonas, Simon,
> 
> On 3/29/25 4:06 PM, Jonas Karlman wrote:
>> From: Simon Glass <sjg@chromium.org>
>>
>> Declare arch and compression at the top of the file to avoid needing
>> ifdefs in every usage.
>>
>> Add a few comments to help with the remaining #ifdefs.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>> Changes in v4:
>> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
>> ---
>>   arch/arm/dts/rockchip-u-boot.dtsi | 44 +++++++++++++++----------------
>>   1 file changed, 22 insertions(+), 22 deletions(-)
>>
>> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
>> index e9ed1d4b5738..2b01dc660562 100644
>> --- a/arch/arm/dts/rockchip-u-boot.dtsi
>> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
>> @@ -5,6 +5,20 @@
>>   
>>   #include <config.h>
>>   
>> +#ifdef CONFIG_ARM64
>> +#define ARCH	"arm64"
>> +#else
>> +#define ARCH	"arm"
>> +#endif
>> +
> 
> I would refrain from using ARCH here as it's something we already use to 
> specify the architecture to build (e.g. make ARCH=arm64 CROSS_COMPILE=...).
> 
> Maybe FIT_ARCH?

sunxi-u-boot.dtsi is also using ARCH so I figured it was also safe here,
we can change to FIT_ARCH for a v5.

> 
>> +#if defined(CONFIG_SPL_GZIP)
>> +#define COMP	"gzip"
>> +#elif defined(CONFIG_SPL_LZMA)
>> +#define COMP	"lzma"
>> +#else
>> +#define COMP	"none"
>> +#endif
>> +
> 
> This is specific to the U-Boot proper image node only, maybe we should 
> have this constant named more appropriately, e.g. FIT_PROPER_COMP or 
> something like that?

Sounds good, will change in a v5.

> 
> Reading the symbols also, I believe their help text is misleading or 
> confusing (at least the _GZIP ones, _ZSTD seems okay).
> 
> Also, I'm wondering if it actually makes sense to force a specific 
> U-Boot proper compression algorithm based on a symbol of what 
> decompression algorithms are supported in U-Boot SPL. Like, why should 
> gzip be picked if I also have lzma enabled? In any case, nothing related 
> to this patch.

Agree, use of compressed image should probably be enabled using a
dedicated Kconfig symbol, something for a different series :-)

> 
>>   / {
>>   	binman: binman {
>>   		multiple-images;
>> @@ -51,26 +65,12 @@
>>   					description = "U-Boot";
>>   					type = "standalone";
>>   					os = "u-boot";
>> -#ifdef CONFIG_ARM64
>> -					arch = "arm64";
>> -#else
>> -					arch = "arm";
>> -#endif
>> -#if defined(CONFIG_SPL_GZIP)
>> -					compression = "gzip";
>> -#elif defined(CONFIG_SPL_LZMA)
>> -					compression = "lzma";
>> -#else
>> -					compression = "none";
>> -#endif
>> +					arch = ARCH;
>> +					compression = COMP;
>>   					load = <CONFIG_TEXT_BASE>;
>>   					entry = <CONFIG_TEXT_BASE>;
>>   					u-boot-nodtb {
>> -#if defined(CONFIG_SPL_GZIP)
>> -					compress = "gzip";
>> -#elif defined(CONFIG_SPL_LZMA)
>> -					compress = "lzma";
>> -#endif
>> +					compress = COMP;
>>   					};
>>   #ifdef CONFIG_SPL_FIT_SIGNATURE
>>   					hash {
>> @@ -84,7 +84,7 @@
>>   					fit,operation = "split-elf";
>>   					description = "ARM Trusted Firmware";
>>   					type = "firmware";
>> -					arch = "arm64";
>> +					arch = ARCH;
>>   					os = "arm-trusted-firmware";
>>   					compression = "none";
>>   					fit,load;
>> @@ -103,7 +103,7 @@
>>   					fit,operation = "split-elf";
>>   					description = "TEE";
>>   					type = "tee";
>> -					arch = "arm64";
>> +					arch = ARCH;
>>   					os = "tee";
>>   					compression = "none";
>>   					fit,load;
>> @@ -119,11 +119,11 @@
>>   					};
>>   #endif
>>   				};
>> -#else
>> +#else /* !CONFIG_ARM64 */
>>   				op-tee {
>>   					description = "OP-TEE";
>>   					type = "tee";
>> -					arch = "arm";
>> +					arch = ARCH;
>>   					os = "tee";
>>   					compression = "none";
>>   					load = <(CFG_SYS_SDRAM_BASE + 0x8400000)>;
> 
> Wondering if we couldn't put some of the Aarch32 and Aarch64 OP-TEE OS 
> node(s) in common?

Sounds like a good idea to maybe put op-tee in a template, personally I
never use op-tee so typically try to minimize any change/impact related
to op-tee.

The RK3506 does use op-tee so I may need to dig more into the op-tee
parts in a future RK3506 enablement series, initially [1] was enough.
Could look more into using a op-tee template in such future series.

[1] https://source.denx.de/u-boot/contributors/kwiboo/u-boot/-/commit/3d683f3b717de010fffeece8712373892a599905

> 
> The change is fine, finding a more appropriate name for COMP and ARCH 
> and it's all good.

I will use different names in a v5.

Regards,
Jonas

> 
> Cheers,
> Quentin

Re: [PATCH v4 04/10] rockchip: binman: Create a template for the FIT

[Jonas Karlman]

Hi Quentin,

On 2025-04-09 11:39, Quentin Schulz wrote:
> Hi Jonas, Simon,
> 
> On 3/29/25 4:06 PM, Jonas Karlman wrote:
> [...]
>> -#if defined(CONFIG_ARM64) || defined(CONFIG_SPL_OPTEE_IMAGE)
>> +#ifdef HAS_FIT
>>   		fit {
>>   			type = "blob";
>>   			filename = "u-boot.itb";
> 
> Use the template for the SPI here already instead of splitting it in a 
> separate commit?

I think the original patches was more involved, so I may have requested
a split. Could possible merge them in a v5.

Regards,
Jonas

> 
> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
> 
> Thanks!
> Quentin
> 

Re: [PATCH v4 02/10] rockchip: binman: Factor out arch and compression

[Quentin Schulz]

Hi Jonas,

On 4/9/25 1:56 PM, Jonas Karlman wrote:
> Hi Quentin,
> 
> On 2025-04-09 11:28, Quentin Schulz wrote:
>> Hi Jonas, Simon,
>>
>> On 3/29/25 4:06 PM, Jonas Karlman wrote:
>>> From: Simon Glass <sjg@chromium.org>
>>>
>>> Declare arch and compression at the top of the file to avoid needing
>>> ifdefs in every usage.
>>>
>>> Add a few comments to help with the remaining #ifdefs.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>> ---
>>> Changes in v4:
>>> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
>>> ---
>>>    arch/arm/dts/rockchip-u-boot.dtsi | 44 +++++++++++++++----------------
>>>    1 file changed, 22 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
>>> index e9ed1d4b5738..2b01dc660562 100644
>>> --- a/arch/arm/dts/rockchip-u-boot.dtsi
>>> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
>>> @@ -5,6 +5,20 @@
>>>    
>>>    #include <config.h>
>>>    
>>> +#ifdef CONFIG_ARM64
>>> +#define ARCH	"arm64"
>>> +#else
>>> +#define ARCH	"arm"
>>> +#endif
>>> +
>>
>> I would refrain from using ARCH here as it's something we already use to
>> specify the architecture to build (e.g. make ARCH=arm64 CROSS_COMPILE=...).
>>

Actually you don't need (or even shouldn't?) provide ARCH to the make 
command, got confused because I'm compiling the kernel right now :)

>> Maybe FIT_ARCH?
> 
> sunxi-u-boot.dtsi is also using ARCH so I figured it was also safe here,
> we can change to FIT_ARCH for a v5.
> 

Indeed. I would prefer something not clashing with other 
environment/make variables :)

[...]

>>> @@ -84,7 +84,7 @@
>>>    					fit,operation = "split-elf";
>>>    					description = "ARM Trusted Firmware";
>>>    					type = "firmware";
>>> -					arch = "arm64";
>>> +					arch = ARCH;
>>>    					os = "arm-trusted-firmware";
>>>    					compression = "none";
>>>    					fit,load;
>>> @@ -103,7 +103,7 @@
>>>    					fit,operation = "split-elf";
>>>    					description = "TEE";
>>>    					type = "tee";
>>> -					arch = "arm64";
>>> +					arch = ARCH;
>>>    					os = "tee";
>>>    					compression = "none";
>>>    					fit,load;
>>> @@ -119,11 +119,11 @@
>>>    					};
>>>    #endif
>>>    				};
>>> -#else
>>> +#else /* !CONFIG_ARM64 */
>>>    				op-tee {
>>>    					description = "OP-TEE";
>>>    					type = "tee";
>>> -					arch = "arm";
>>> +					arch = ARCH;
>>>    					os = "tee";
>>>    					compression = "none";
>>>    					load = <(CFG_SYS_SDRAM_BASE + 0x8400000)>;
>>
>> Wondering if we couldn't put some of the Aarch32 and Aarch64 OP-TEE OS
>> node(s) in common?
> 
> Sounds like a good idea to maybe put op-tee in a template, personally I
> never use op-tee so typically try to minimize any change/impact related
> to op-tee.
> 
> The RK3506 does use op-tee so I may need to dig more into the op-tee
> parts in a future RK3506 enablement series, initially [1] was enough.
> Could look more into using a op-tee template in such future series.
> 
> [1] https://source.denx.de/u-boot/contributors/kwiboo/u-boot/-/commit/3d683f3b717de010fffeece8712373892a599905
> 

Interesting, is it required for RK3506? Do they do things in secure 
world and since it's Aarch32, no TF-A loaded by U-Boot?

You can play with OP-TEE on RK3588 from master, c.f. 
https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/plat-rockchip/platform_rk3588.c

I haven't even built it but there's been some work on it since it was 
merged early December last year, so possibly people are using it.

Cheers,
Quentin

Re: [PATCH v4 08/10] rockchip: binman: Use the skip-at-start prop in simple-bin image

[Simon Glass]

Hi Quentin,

On Wed, 9 Apr 2025 at 04:57, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Jonas, Simon,
>
> On 3/29/25 4:06 PM, Jonas Karlman wrote:
> > From: Simon Glass <sjg@chromium.org>
> >
> > The simple-bin image is normally written to MMC media at block 64, which
> > is a 32K offset from start of storage media.
> >
> > Set the skip-at-start property to 0x8000 (32 KiB) so that fdtmap and
> > other embedded binman symbols in the output binary is referencing image
> > offsets correctly.
> >
>
> Shouldn't we have this commit BEFORE we add the `fdtmap` node since we
> know it's wrong before this commit?
>
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > ---
> > Changes in v4:
> > - Drop defconfig changes
> > - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
> >
> > Changes in v2:
> > - Move this patch to the end of the series
> > - Drop 0x8000 offset for SPI
> > ---
> >   arch/arm/dts/rockchip-u-boot.dtsi | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> > index fb38b7b80c43..65b81bf58626 100644
> > --- a/arch/arm/dts/rockchip-u-boot.dtsi
> > +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> > @@ -154,6 +154,7 @@
> >       simple-bin {
> >               filename = "u-boot-rockchip.bin";
> >               pad-byte = <0xff>;
> > +             skip-at-start = <0x8000>;
> >
> >               mkimage {
> >                       filename = "idbloader.img";
> > @@ -178,7 +179,7 @@
> >   #else
> >               u-boot-img {
> >   #endif
> > -                     offset = <CONFIG_SPL_PAD_TO>;
> > +                     offset = <(CONFIG_SPL_PAD_TO + 0x8000)>;
>
> This is confusing. The documentation states:
>
> """
> offset:
>      This sets the offset of an entry within the image or section containing
>      it.
> """
>
> My understanding is that it should be relative to the beginning of the
> image but this now needs the knowledge of where it will be stored on the
> MMC device (via the value in skip-at-start).
>
> Why is skip-at-start automatically deducted from offset?

This is how binman works[1]. We are trying to use the feature designs
for this purpose (dealing with implied / missing chunks at the start
of an image). At this point in the series CONFIG_SPL_PAD_TO is set to
0x78000 I believe, so we need to add another 0x8000 to get the offset
that binman uses.

Regards,
Simon

[1] https://docs.u-boot.org/en/latest/develop/package/binman.html#image-properties

Re: [PATCH v4 07/10] rockchip: binman: Include a compatible string in each configuration

[Simon Glass]

Hi Quentin,

On Wed, 9 Apr 2025 at 04:03, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Jonas, Simon,
>
> On 3/29/25 4:06 PM, Jonas Karlman wrote:
> > From: Simon Glass <sjg@chromium.org>
> >
> > Provide a compatible string in the config nodes that U-Boot can use to
> > help decide which configuration to use.
> >
>
> Can you tell us more about this?

See [1]

>
> I don't think mkimage -l/dumpimage -l actually provide that information
> and the docs are sparse as to what "so that things work correctly with
> FIT's configuration-matching algortihm." means.
>
> Looking a bit into tools/binman/ftest.py it seems like it's a way to
> expose the DT compatible property from within the first entry in `fdt`
> array property (a DTB) into the configuration node in the fit image.

Yes, the root compatible string is how we decide which devicetree to use.

>
> I think it'd make sense to update dumpimage/mkimage/etc... to dump that
> information as well?

Yes, I think that might be useful

>
> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
>
> Thanks!
> Quentin

[1] https://fitspec.osfw.foundation/#configuration-nodes

Re: [PATCH v4 08/10] rockchip: binman: Use the skip-at-start prop in simple-bin image

[Quentin Schulz]

Hi Simon,

On 4/9/25 3:22 PM, Simon Glass wrote:
> Hi Quentin,
> 
> On Wed, 9 Apr 2025 at 04:57, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>
>> Hi Jonas, Simon,
>>
>> On 3/29/25 4:06 PM, Jonas Karlman wrote:
>>> From: Simon Glass <sjg@chromium.org>
>>>
>>> The simple-bin image is normally written to MMC media at block 64, which
>>> is a 32K offset from start of storage media.
>>>
>>> Set the skip-at-start property to 0x8000 (32 KiB) so that fdtmap and
>>> other embedded binman symbols in the output binary is referencing image
>>> offsets correctly.
>>>
>>
>> Shouldn't we have this commit BEFORE we add the `fdtmap` node since we
>> know it's wrong before this commit?
>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>> ---
>>> Changes in v4:
>>> - Drop defconfig changes
>>> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
>>>
>>> Changes in v2:
>>> - Move this patch to the end of the series
>>> - Drop 0x8000 offset for SPI
>>> ---
>>>    arch/arm/dts/rockchip-u-boot.dtsi | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
>>> index fb38b7b80c43..65b81bf58626 100644
>>> --- a/arch/arm/dts/rockchip-u-boot.dtsi
>>> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
>>> @@ -154,6 +154,7 @@
>>>        simple-bin {
>>>                filename = "u-boot-rockchip.bin";
>>>                pad-byte = <0xff>;
>>> +             skip-at-start = <0x8000>;
>>>
>>>                mkimage {
>>>                        filename = "idbloader.img";
>>> @@ -178,7 +179,7 @@
>>>    #else
>>>                u-boot-img {
>>>    #endif
>>> -                     offset = <CONFIG_SPL_PAD_TO>;
>>> +                     offset = <(CONFIG_SPL_PAD_TO + 0x8000)>;
>>
>> This is confusing. The documentation states:
>>
>> """
>> offset:
>>       This sets the offset of an entry within the image or section containing
>>       it.
>> """
>>
>> My understanding is that it should be relative to the beginning of the
>> image but this now needs the knowledge of where it will be stored on the
>> MMC device (via the value in skip-at-start).
>>
>> Why is skip-at-start automatically deducted from offset?
> 
> This is how binman works[1]. We are trying to use the feature designs

Why is it deducted?

Cheers,
Quentin

Re: [PATCH v4 08/10] rockchip: binman: Use the skip-at-start prop in simple-bin image

[Simon Glass]

Hi Quentin,

On Wed, 9 Apr 2025 at 07:32, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Simon,
>
> On 4/9/25 3:22 PM, Simon Glass wrote:
> > Hi Quentin,
> >
> > On Wed, 9 Apr 2025 at 04:57, Quentin Schulz <quentin.schulz@cherry.de> wrote:
> >>
> >> Hi Jonas, Simon,
> >>
> >> On 3/29/25 4:06 PM, Jonas Karlman wrote:
> >>> From: Simon Glass <sjg@chromium.org>
> >>>
> >>> The simple-bin image is normally written to MMC media at block 64, which
> >>> is a 32K offset from start of storage media.
> >>>
> >>> Set the skip-at-start property to 0x8000 (32 KiB) so that fdtmap and
> >>> other embedded binman symbols in the output binary is referencing image
> >>> offsets correctly.
> >>>
> >>
> >> Shouldn't we have this commit BEFORE we add the `fdtmap` node since we
> >> know it's wrong before this commit?
> >>
> >>> Signed-off-by: Simon Glass <sjg@chromium.org>
> >>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> >>> ---
> >>> Changes in v4:
> >>> - Drop defconfig changes
> >>> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
> >>>
> >>> Changes in v2:
> >>> - Move this patch to the end of the series
> >>> - Drop 0x8000 offset for SPI
> >>> ---
> >>>    arch/arm/dts/rockchip-u-boot.dtsi | 3 ++-
> >>>    1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> >>> index fb38b7b80c43..65b81bf58626 100644
> >>> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> >>> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> >>> @@ -154,6 +154,7 @@
> >>>        simple-bin {
> >>>                filename = "u-boot-rockchip.bin";
> >>>                pad-byte = <0xff>;
> >>> +             skip-at-start = <0x8000>;
> >>>
> >>>                mkimage {
> >>>                        filename = "idbloader.img";
> >>> @@ -178,7 +179,7 @@
> >>>    #else
> >>>                u-boot-img {
> >>>    #endif
> >>> -                     offset = <CONFIG_SPL_PAD_TO>;
> >>> +                     offset = <(CONFIG_SPL_PAD_TO + 0x8000)>;
> >>
> >> This is confusing. The documentation states:
> >>
> >> """
> >> offset:
> >>       This sets the offset of an entry within the image or section containing
> >>       it.
> >> """
> >>
> >> My understanding is that it should be relative to the beginning of the
> >> image but this now needs the knowledge of where it will be stored on the
> >> MMC device (via the value in skip-at-start).
> >>
> >> Why is skip-at-start automatically deducted from offset?
> >
> > This is how binman works[1]. We are trying to use the feature designs
>
> Why is it deducted?

Are you asking why skip-at-start is deducted from the offset?

Regards,
Simon

Re: [PATCH v4 08/10] rockchip: binman: Use the skip-at-start prop in simple-bin image

[Quentin Schulz]

Hi Simon,

On 4/9/25 3:33 PM, Simon Glass wrote:
> Hi Quentin,
> 
> On Wed, 9 Apr 2025 at 07:32, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>
>> Hi Simon,
>>
>> On 4/9/25 3:22 PM, Simon Glass wrote:
>>> Hi Quentin,
>>>
>>> On Wed, 9 Apr 2025 at 04:57, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>>>
>>>> Hi Jonas, Simon,
>>>>
>>>> On 3/29/25 4:06 PM, Jonas Karlman wrote:
>>>>> From: Simon Glass <sjg@chromium.org>
>>>>>
>>>>> The simple-bin image is normally written to MMC media at block 64, which
>>>>> is a 32K offset from start of storage media.
>>>>>
>>>>> Set the skip-at-start property to 0x8000 (32 KiB) so that fdtmap and
>>>>> other embedded binman symbols in the output binary is referencing image
>>>>> offsets correctly.
>>>>>
>>>>
>>>> Shouldn't we have this commit BEFORE we add the `fdtmap` node since we
>>>> know it's wrong before this commit?
>>>>
>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>>> ---
>>>>> Changes in v4:
>>>>> - Drop defconfig changes
>>>>> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
>>>>>
>>>>> Changes in v2:
>>>>> - Move this patch to the end of the series
>>>>> - Drop 0x8000 offset for SPI
>>>>> ---
>>>>>     arch/arm/dts/rockchip-u-boot.dtsi | 3 ++-
>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
>>>>> index fb38b7b80c43..65b81bf58626 100644
>>>>> --- a/arch/arm/dts/rockchip-u-boot.dtsi
>>>>> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
>>>>> @@ -154,6 +154,7 @@
>>>>>         simple-bin {
>>>>>                 filename = "u-boot-rockchip.bin";
>>>>>                 pad-byte = <0xff>;
>>>>> +             skip-at-start = <0x8000>;
>>>>>
>>>>>                 mkimage {
>>>>>                         filename = "idbloader.img";
>>>>> @@ -178,7 +179,7 @@
>>>>>     #else
>>>>>                 u-boot-img {
>>>>>     #endif
>>>>> -                     offset = <CONFIG_SPL_PAD_TO>;
>>>>> +                     offset = <(CONFIG_SPL_PAD_TO + 0x8000)>;
>>>>
>>>> This is confusing. The documentation states:
>>>>
>>>> """
>>>> offset:
>>>>        This sets the offset of an entry within the image or section containing
>>>>        it.
>>>> """
>>>>
>>>> My understanding is that it should be relative to the beginning of the
>>>> image but this now needs the knowledge of where it will be stored on the
>>>> MMC device (via the value in skip-at-start).
>>>>
>>>> Why is skip-at-start automatically deducted from offset?
>>>
>>> This is how binman works[1]. We are trying to use the feature designs
>>
>> Why is it deducted?
> 
> Are you asking why skip-at-start is deducted from the offset?
> 

Yes

> Regards,
> Simon

Re: [PATCH v4 08/10] rockchip: binman: Use the skip-at-start prop in simple-bin image

[Simon Glass]

Hi Quentin,

On Wed, 9 Apr 2025 at 07:35, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Simon,
>
> On 4/9/25 3:33 PM, Simon Glass wrote:
> > Hi Quentin,
> >
> > On Wed, 9 Apr 2025 at 07:32, Quentin Schulz <quentin.schulz@cherry.de> wrote:
> >>
> >> Hi Simon,
> >>
> >> On 4/9/25 3:22 PM, Simon Glass wrote:
> >>> Hi Quentin,
> >>>
> >>> On Wed, 9 Apr 2025 at 04:57, Quentin Schulz <quentin.schulz@cherry.de> wrote:
> >>>>
> >>>> Hi Jonas, Simon,
> >>>>
> >>>> On 3/29/25 4:06 PM, Jonas Karlman wrote:
> >>>>> From: Simon Glass <sjg@chromium.org>
> >>>>>
> >>>>> The simple-bin image is normally written to MMC media at block 64, which
> >>>>> is a 32K offset from start of storage media.
> >>>>>
> >>>>> Set the skip-at-start property to 0x8000 (32 KiB) so that fdtmap and
> >>>>> other embedded binman symbols in the output binary is referencing image
> >>>>> offsets correctly.
> >>>>>
> >>>>
> >>>> Shouldn't we have this commit BEFORE we add the `fdtmap` node since we
> >>>> know it's wrong before this commit?
> >>>>
> >>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
> >>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> >>>>> ---
> >>>>> Changes in v4:
> >>>>> - Drop defconfig changes
> >>>>> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
> >>>>>
> >>>>> Changes in v2:
> >>>>> - Move this patch to the end of the series
> >>>>> - Drop 0x8000 offset for SPI
> >>>>> ---
> >>>>>     arch/arm/dts/rockchip-u-boot.dtsi | 3 ++-
> >>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> >>>>> index fb38b7b80c43..65b81bf58626 100644
> >>>>> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> >>>>> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> >>>>> @@ -154,6 +154,7 @@
> >>>>>         simple-bin {
> >>>>>                 filename = "u-boot-rockchip.bin";
> >>>>>                 pad-byte = <0xff>;
> >>>>> +             skip-at-start = <0x8000>;
> >>>>>
> >>>>>                 mkimage {
> >>>>>                         filename = "idbloader.img";
> >>>>> @@ -178,7 +179,7 @@
> >>>>>     #else
> >>>>>                 u-boot-img {
> >>>>>     #endif
> >>>>> -                     offset = <CONFIG_SPL_PAD_TO>;
> >>>>> +                     offset = <(CONFIG_SPL_PAD_TO + 0x8000)>;
> >>>>
> >>>> This is confusing. The documentation states:
> >>>>
> >>>> """
> >>>> offset:
> >>>>        This sets the offset of an entry within the image or section containing
> >>>>        it.
> >>>> """
> >>>>
> >>>> My understanding is that it should be relative to the beginning of the
> >>>> image but this now needs the knowledge of where it will be stored on the
> >>>> MMC device (via the value in skip-at-start).
> >>>>
> >>>> Why is skip-at-start automatically deducted from offset?
> >>>
> >>> This is how binman works[1]. We are trying to use the feature designs
> >>
> >> Why is it deducted?
> >
> > Are you asking why skip-at-start is deducted from the offset?
> >
>
> Yes

It is confusing, unfortunately.

When you use an offset (say 0x78000) then normally the entry will
start at that offset in the image.

When you use skip-at-start 0x8000, its value is added to all top-level
offsets in the image, so the offset becomes 0x80000

BUT the image built by binman does not contain the first 0x8000 bytes.
It is expected that the image is written to offset 0x8000 so that the
offsets will be correct when used within U-Boot itself.

Another way to think of it is that skip-at-start doesn't really change
any offsets; it just creates a hole at the start of the image. If you
didn't use skip-at-start you could manually set the first offset to
0x8000 and use dd to skip the first 0x8000 bytes when writing.

This feature was added for x86 to handle memory-mapped ROMs, but it
has proved useful for other cases now.

Regards,
Simon

Re: [PATCH v4 07/10] rockchip: binman: Include a compatible string in each configuration

[Jonas Karlman]

Hi Quentin,

On 2025-04-09 12:02, Quentin Schulz wrote:
> Hi Jonas, Simon,
> 
> On 3/29/25 4:06 PM, Jonas Karlman wrote:
>> From: Simon Glass <sjg@chromium.org>
>>
>> Provide a compatible string in the config nodes that U-Boot can use to
>> help decide which configuration to use.
>>
> 
> Can you tell us more about this?

I think the VBE can use this to determine what config/fdt would be used
in a multi-dtb FIT image.

This also sparked an idea to use this compatible for board selection in SPL
instead of the description (fdtfile) field. I have started to play around
with a board_fit_config_compatible_match() a few patches in at [1], e.g.:

- WIP: boot: fit: add board_fit_config_compatible_match()
- WIP: rockchip: implement fit config compatible match for boards

[1] https://source.denx.de/u-boot/contributors/kwiboo/u-boot/-/commits/for-next

That could also be used for having board specific TPL+SPL and use a 
single multi-dtb FIT with U-Boot proper and multiple fdt/config nodes,
one for each board.

> 
> I don't think mkimage -l/dumpimage -l actually provide that information 
> and the docs are sparse as to what "so that things work correctly with 
> FIT's configuration-matching algortihm." means.
> 
> Looking a bit into tools/binman/ftest.py it seems like it's a way to 
> expose the DT compatible property from within the first entry in `fdt` 
> array property (a DTB) into the configuration node in the fit image.
> 
> I think it'd make sense to update dumpimage/mkimage/etc... to dump that 
> information as well?

I think so too, I know there are a few load/entry addresses for some
image type that is also not shown by dumpimage. This is something that
can be improved in a different series.

For FIT you can always use: dtc -I dtb -O dts u-boot.itb

  [...]
        configurations {
                default = "config-1";

                config-1 {
                        compatible = "radxa,e20c\0rockchip,rk3528";
                        loadables = "u-boot\0atf-2\0atf-3";
                        firmware = "atf-1";
                        fdt = "fdt-1";
                        description = "rockchip/rk3528-radxa-e20c.dtb";
                };

                config-2 {
                        compatible = "radxa,rock-2a\0rockchip,rk3528";
                        loadables = "u-boot\0atf-2\0atf-3";
                        firmware = "atf-1";
                        fdt = "fdt-2";
                        description = "rockchip/rk3528-rock-2a.dtb";
                };

                config-3 {
                        compatible = "radxa,rock-2f\0rockchip,rk3528";
                        loadables = "u-boot\0atf-2\0atf-3";
                        firmware = "atf-1";
                        fdt = "fdt-3";
                        description = "rockchip/rk3528-rock-2f.dtb";
                };
        };
  [...]

Regards,
Jonas

> 
> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
> 
> Thanks!
> Quentin

Re: [PATCH v4 08/10] rockchip: binman: Use the skip-at-start prop in simple-bin image

[Jonas Karlman]

Hi Quentin,

On 2025-04-09 12:57, Quentin Schulz wrote:
> Hi Jonas, Simon,
> 
> On 3/29/25 4:06 PM, Jonas Karlman wrote:
>> From: Simon Glass <sjg@chromium.org>
>>
>> The simple-bin image is normally written to MMC media at block 64, which
>> is a 32K offset from start of storage media.
>>
>> Set the skip-at-start property to 0x8000 (32 KiB) so that fdtmap and
>> other embedded binman symbols in the output binary is referencing image
>> offsets correctly.
>>
> 
> Shouldn't we have this commit BEFORE we add the `fdtmap` node since we 
> know it's wrong before this commit?

Sure, this series could/should probably be reordered into, patches with:
- fixes
- no functional change
- functional change
- new features

My understanding is that the binman symbols is not really used by
Rockchip in current configuration, yet symbols is embedded into the
binary utput and possible also with the fdtmap. So the order has no
practical change/diff.

Will reorder patches in a v5.

> 
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>> Changes in v4:
>> - Drop defconfig changes
>> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
>>
>> Changes in v2:
>> - Move this patch to the end of the series
>> - Drop 0x8000 offset for SPI
>> ---
>>   arch/arm/dts/rockchip-u-boot.dtsi | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
>> index fb38b7b80c43..65b81bf58626 100644
>> --- a/arch/arm/dts/rockchip-u-boot.dtsi
>> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
>> @@ -154,6 +154,7 @@
>>   	simple-bin {
>>   		filename = "u-boot-rockchip.bin";
>>   		pad-byte = <0xff>;
>> +		skip-at-start = <0x8000>;
>>   
>>   		mkimage {
>>   			filename = "idbloader.img";
>> @@ -178,7 +179,7 @@
>>   #else
>>   		u-boot-img {
>>   #endif
>> -			offset = <CONFIG_SPL_PAD_TO>;
>> +			offset = <(CONFIG_SPL_PAD_TO + 0x8000)>;
> 
> This is confusing. The documentation states:
> 
> """
> offset:
>      This sets the offset of an entry within the image or section containing
>      it.
> """
> 
> My understanding is that it should be relative to the beginning of the 
> image but this now needs the knowledge of where it will be stored on the 
> MMC device (via the value in skip-at-start).

This was also my understanding but Simon has a different view and for
the simple-bin image that is mainly targeted MMC storage this could
possible be okay.

If there is any slight objection of this I would rather just drop this
patch for now and it can be part of Simon's next VBE part series.

Regards,
Jonas

> 
> Why is skip-at-start automatically deducted from offset?
> 
> Cheers,
> Quentin

Re: [PATCH v4 07/10] rockchip: binman: Include a compatible string in each configuration

[Quentin Schulz]

Hi Jonas, Simon,

On 4/9/25 5:05 PM, Jonas Karlman wrote:
> Hi Quentin,
> 
> On 2025-04-09 12:02, Quentin Schulz wrote:
>> Hi Jonas, Simon,
>>
>> On 3/29/25 4:06 PM, Jonas Karlman wrote:
>>> From: Simon Glass <sjg@chromium.org>
>>>
>>> Provide a compatible string in the config nodes that U-Boot can use to
>>> help decide which configuration to use.
>>>
>>
>> Can you tell us more about this?
> 
> I think the VBE can use this to determine what config/fdt would be used
> in a multi-dtb FIT image.
> 
> This also sparked an idea to use this compatible for board selection in SPL
> instead of the description (fdtfile) field. I have started to play around
> with a board_fit_config_compatible_match() a few patches in at [1], e.g.:
> 
> - WIP: boot: fit: add board_fit_config_compatible_match()
> - WIP: rockchip: implement fit config compatible match for boards
> 
> [1] https://source.denx.de/u-boot/contributors/kwiboo/u-boot/-/commits/for-next
> 
> That could also be used for having board specific TPL+SPL and use a
> single multi-dtb FIT with U-Boot proper and multiple fdt/config nodes,
> one for each board.
> 
>>
>> I don't think mkimage -l/dumpimage -l actually provide that information
>> and the docs are sparse as to what "so that things work correctly with
>> FIT's configuration-matching algortihm." means.
>>
>> Looking a bit into tools/binman/ftest.py it seems like it's a way to
>> expose the DT compatible property from within the first entry in `fdt`
>> array property (a DTB) into the configuration node in the fit image.
>>

Seems like it's a bit more involved than that, c.f. 
https://fitspec.osfw.foundation/#select-a-configuration-to-boot

Thanks both of you for the pointers.

>> I think it'd make sense to update dumpimage/mkimage/etc... to dump that
>> information as well?
> 
> I think so too, I know there are a few load/entry addresses for some
> image type that is also not shown by dumpimage. This is something that
> can be improved in a different series.
> 

I've sent something for this compatible conf node property: 
https://lore.kernel.org/u-boot/20250409-fit-compat-v1-0-56df89ef486a@cherry.de/T/#t

But indeed, seems like we're missing printing the "script" property and 
many others related to signing for example. There's also a few image 
properties that are only printed based on the type, e.g. "Architecture" 
only for kernel, standalone, ramdisk, firmware and fdt types, "OS" onylk 
for kernel, ramdisk and firmware. Not sure what the reasoning is.

Cheers,
Quentin

Re: [PATCH v4 09/10] rockchip: binman: Support use of crc32 for SPL_FIT_SIGNATURE

[Jonas Karlman]

Hi Quentin,

On 2025-04-09 13:06, Quentin Schulz wrote:
> Hi Jonas,
> 
> On 3/29/25 4:06 PM, Jonas Karlman wrote:
>> Use of SHA256 checksum validation on ARMv7 SoCs can be very time
>> consuming compared to ARMv8 SoCs with Crypto Extensions.
>>
>> Add support for use of the crc32 hash algo when SHA256 is not supported.
>> Also use a HAS_HASH to simplify the ifdefs when no known hash algo is
>> compiled.
>>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> 
> I don't know enough about general security but this very much looks like 
> a bad idea to me.
> 
> https://web.archive.org/web/20170210173741/http://www.derkeiler.com/Newsgroups/sci.crypt/2003-07/1451.html
> 
> """
> While properly designed CRC's are good at detecting random errors in
> the data (due to e.g. line noise), the CRC is useless as a secure
> indicator of intentional manipulation of the data. And this is
> because it's not hard at all to modify the data to produce any CRC
> you desire (e.g. the same CRC as the original data, to try to
> disguise your data manipulation).
> """
> 
> (yes I took the "first" link my web search engine returned me, thanks 
> confirmation bias!).

I am fully aware, and the fallback to use crc32, and current primarily
use of sha256, should not be considered a security feature. It is
intended purely for a checksum validation of the binary blob after it
has been loaded into memory and before U-Boot otherwise unconditionally
jumps to and execute the loaded blob of binary code.

> 
> I don't want to give people a false sense of security. If it really 
> comes down to it, I'd rather have an explicit Kconfig symbol to set this 
> value (maybe have a `choice` even) and be very clear about security 
> implications.

Prior to the addition of the hash { algo=sha256 }, there was no checksum
test and U-Boot SPL would unconditionally jump to the loaded data, even
if it had been corrupted, was only halfway written or otherwise
overwritten.

The addition of crc32 instead of sha256 is just to allow older board
variants with weaker SoCs to at least have some sort of checksum
validation in use without affecting the boot time too much.

On my rk3228 board, validation using sha256 could take a few seconds,
and with crc32 it could be measured in ms.

To me, having at least some default checksum validation is preferred
over none at all.

Regards,
Jonas

> 
> Cheers,
> Quentin

Re: [PATCH v4 02/10] rockchip: binman: Factor out arch and compression

[Jonas Karlman]

Hi Quentin,

On 2025-04-09 14:27, Quentin Schulz wrote:
> Hi Jonas,
> 
> On 4/9/25 1:56 PM, Jonas Karlman wrote:
>> Hi Quentin,
>>
>> On 2025-04-09 11:28, Quentin Schulz wrote:
>>> Hi Jonas, Simon,
>>>
>>> On 3/29/25 4:06 PM, Jonas Karlman wrote:
>>>> From: Simon Glass <sjg@chromium.org>
>>>>
>>>> Declare arch and compression at the top of the file to avoid needing
>>>> ifdefs in every usage.
>>>>
>>>> Add a few comments to help with the remaining #ifdefs.
>>>>
>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>> ---
>>>> Changes in v4:
>>>> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
>>>> ---
>>>>    arch/arm/dts/rockchip-u-boot.dtsi | 44 +++++++++++++++----------------
>>>>    1 file changed, 22 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
>>>> index e9ed1d4b5738..2b01dc660562 100644
>>>> --- a/arch/arm/dts/rockchip-u-boot.dtsi
>>>> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
>>>> @@ -5,6 +5,20 @@
>>>>    
>>>>    #include <config.h>
>>>>    
>>>> +#ifdef CONFIG_ARM64
>>>> +#define ARCH	"arm64"
>>>> +#else
>>>> +#define ARCH	"arm"
>>>> +#endif
>>>> +
>>>
>>> I would refrain from using ARCH here as it's something we already use to
>>> specify the architecture to build (e.g. make ARCH=arm64 CROSS_COMPILE=...).
>>>
> 
> Actually you don't need (or even shouldn't?) provide ARCH to the make 
> command, got confused because I'm compiling the kernel right now :)
> 
>>> Maybe FIT_ARCH?
>>
>> sunxi-u-boot.dtsi is also using ARCH so I figured it was also safe here,
>> we can change to FIT_ARCH for a v5.
>>
> 
> Indeed. I would prefer something not clashing with other 
> environment/make variables :)
> 
> [...]
> 
>>>> @@ -84,7 +84,7 @@
>>>>    					fit,operation = "split-elf";
>>>>    					description = "ARM Trusted Firmware";
>>>>    					type = "firmware";
>>>> -					arch = "arm64";
>>>> +					arch = ARCH;
>>>>    					os = "arm-trusted-firmware";
>>>>    					compression = "none";
>>>>    					fit,load;
>>>> @@ -103,7 +103,7 @@
>>>>    					fit,operation = "split-elf";
>>>>    					description = "TEE";
>>>>    					type = "tee";
>>>> -					arch = "arm64";
>>>> +					arch = ARCH;
>>>>    					os = "tee";
>>>>    					compression = "none";
>>>>    					fit,load;
>>>> @@ -119,11 +119,11 @@
>>>>    					};
>>>>    #endif
>>>>    				};
>>>> -#else
>>>> +#else /* !CONFIG_ARM64 */
>>>>    				op-tee {
>>>>    					description = "OP-TEE";
>>>>    					type = "tee";
>>>> -					arch = "arm";
>>>> +					arch = ARCH;
>>>>    					os = "tee";
>>>>    					compression = "none";
>>>>    					load = <(CFG_SYS_SDRAM_BASE + 0x8400000)>;
>>>
>>> Wondering if we couldn't put some of the Aarch32 and Aarch64 OP-TEE OS
>>> node(s) in common?
>>
>> Sounds like a good idea to maybe put op-tee in a template, personally I
>> never use op-tee so typically try to minimize any change/impact related
>> to op-tee.
>>
>> The RK3506 does use op-tee so I may need to dig more into the op-tee
>> parts in a future RK3506 enablement series, initially [1] was enough.
>> Could look more into using a op-tee template in such future series.
>>
>> [1] https://source.denx.de/u-boot/contributors/kwiboo/u-boot/-/commit/3d683f3b717de010fffeece8712373892a599905
>>
> 
> Interesting, is it required for RK3506? Do they do things in secure 
> world and since it's Aarch32, no TF-A loaded by U-Boot?

To my knowledge the vendor OP-TEE blob implement PSCI for cpu core mgmt
and it also does some sort of initialization for OTP.

Without starting OP-TEE before U-Boot proper, reading from OTP only
return 0x00 for each byte read.

So will probably be much easier to just run OP-TEE similar to what some
other Rockchip ARMv7 SoCs does/require.

> 
> You can play with OP-TEE on RK3588 from master, c.f. 
> https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/plat-rockchip/platform_rk3588.c
> 
> I haven't even built it but there's been some work on it since it was 
> merged early December last year, so possibly people are using it.

Thanks, will take a closer look at some point in future :-)

Regards,
Jonas

> 
> Cheers,
> Quentin

Re: [PATCH v4 09/10] rockchip: binman: Support use of crc32 for SPL_FIT_SIGNATURE

[Quentin Schulz]

Hi Jonas,

On 4/9/25 5:38 PM, Jonas Karlman wrote:
> Hi Quentin,
> 
> On 2025-04-09 13:06, Quentin Schulz wrote:
>> Hi Jonas,
>>
>> On 3/29/25 4:06 PM, Jonas Karlman wrote:
>>> Use of SHA256 checksum validation on ARMv7 SoCs can be very time
>>> consuming compared to ARMv8 SoCs with Crypto Extensions.
>>>
>>> Add support for use of the crc32 hash algo when SHA256 is not supported.
>>> Also use a HAS_HASH to simplify the ifdefs when no known hash algo is
>>> compiled.
>>>
>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>
>> I don't know enough about general security but this very much looks like
>> a bad idea to me.
>>
>> https://web.archive.org/web/20170210173741/http://www.derkeiler.com/Newsgroups/sci.crypt/2003-07/1451.html
>>
>> """
>> While properly designed CRC's are good at detecting random errors in
>> the data (due to e.g. line noise), the CRC is useless as a secure
>> indicator of intentional manipulation of the data. And this is
>> because it's not hard at all to modify the data to produce any CRC
>> you desire (e.g. the same CRC as the original data, to try to
>> disguise your data manipulation).
>> """
>>
>> (yes I took the "first" link my web search engine returned me, thanks
>> confirmation bias!).
> 
> I am fully aware, and the fallback to use crc32, and current primarily
> use of sha256, should not be considered a security feature. It is
> intended purely for a checksum validation of the binary blob after it
> has been loaded into memory and before U-Boot otherwise unconditionally
> jumps to and execute the loaded blob of binary code.
> 
>>
>> I don't want to give people a false sense of security. If it really
>> comes down to it, I'd rather have an explicit Kconfig symbol to set this
>> value (maybe have a `choice` even) and be very clear about security
>> implications.
> 
> Prior to the addition of the hash { algo=sha256 }, there was no checksum
> test and U-Boot SPL would unconditionally jump to the loaded data, even
> if it had been corrupted, was only halfway written or otherwise
> overwritten.
> 
> The addition of crc32 instead of sha256 is just to allow older board
> variants with weaker SoCs to at least have some sort of checksum
> validation in use without affecting the boot time too much.
> 
> On my rk3228 board, validation using sha256 could take a few seconds,
> and with crc32 it could be measured in ms.
> 
> To me, having at least some default checksum validation is preferred
> over none at all.
> 

Except if this confuses people into thinking they have a secure system 
except they use CRC32 instead of something more appropriate like SHA256.

It seems like the "hash" node presence doesn't mean a FIT conf or image 
node is signed. I also don't see many device trees with a signature 
node... which is kinda odd. Looking a bit into the signature node in the 
spec and binman tests, it's not clear to me if the hash node is reused 
by the signature node if if exists and if their algo should match and 
what exactly is checked by the signature node (like signing the hash 
string contained in the hash node(s) listed in sign-images property, or 
(re-)generating a hash of those and signing it at build time?

If
a) it is not possible to have a hash node's algo differ from a signature 
node's algo, then I'm fine with this as crc32 won't be allowed
b) the signature node regenerates a hash regardless of the hash in the 
hash node, meaning the signature will be "appropriately secure" 
regardless of the algorithm listed in the hash node, then I'm fine with 
it too,
c) it is possible to sign a crc32 hash, don't want it :)

@Simon, do you have some hints about this?

Cheers,
Quentin

Re: [PATCH v4 09/10] rockchip: binman: Support use of crc32 for SPL_FIT_SIGNATURE

[Simon Glass]

Hi Quentin,

On Wed, 9 Apr 2025 at 10:11, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Jonas,
>
> On 4/9/25 5:38 PM, Jonas Karlman wrote:
> > Hi Quentin,
> >
> > On 2025-04-09 13:06, Quentin Schulz wrote:
> >> Hi Jonas,
> >>
> >> On 3/29/25 4:06 PM, Jonas Karlman wrote:
> >>> Use of SHA256 checksum validation on ARMv7 SoCs can be very time
> >>> consuming compared to ARMv8 SoCs with Crypto Extensions.
> >>>
> >>> Add support for use of the crc32 hash algo when SHA256 is not supported.
> >>> Also use a HAS_HASH to simplify the ifdefs when no known hash algo is
> >>> compiled.
> >>>
> >>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> >>
> >> I don't know enough about general security but this very much looks like
> >> a bad idea to me.
> >>
> >> https://web.archive.org/web/20170210173741/http://www.derkeiler.com/Newsgroups/sci.crypt/2003-07/1451.html
> >>
> >> """
> >> While properly designed CRC's are good at detecting random errors in
> >> the data (due to e.g. line noise), the CRC is useless as a secure
> >> indicator of intentional manipulation of the data. And this is
> >> because it's not hard at all to modify the data to produce any CRC
> >> you desire (e.g. the same CRC as the original data, to try to
> >> disguise your data manipulation).
> >> """
> >>
> >> (yes I took the "first" link my web search engine returned me, thanks
> >> confirmation bias!).
> >
> > I am fully aware, and the fallback to use crc32, and current primarily
> > use of sha256, should not be considered a security feature. It is
> > intended purely for a checksum validation of the binary blob after it
> > has been loaded into memory and before U-Boot otherwise unconditionally
> > jumps to and execute the loaded blob of binary code.
> >
> >>
> >> I don't want to give people a false sense of security. If it really
> >> comes down to it, I'd rather have an explicit Kconfig symbol to set this
> >> value (maybe have a `choice` even) and be very clear about security
> >> implications.
> >
> > Prior to the addition of the hash { algo=sha256 }, there was no checksum
> > test and U-Boot SPL would unconditionally jump to the loaded data, even
> > if it had been corrupted, was only halfway written or otherwise
> > overwritten.
> >
> > The addition of crc32 instead of sha256 is just to allow older board
> > variants with weaker SoCs to at least have some sort of checksum
> > validation in use without affecting the boot time too much.
> >
> > On my rk3228 board, validation using sha256 could take a few seconds,
> > and with crc32 it could be measured in ms.
> >
> > To me, having at least some default checksum validation is preferred
> > over none at all.
> >
>
> Except if this confuses people into thinking they have a secure system
> except they use CRC32 instead of something more appropriate like SHA256.
>
> It seems like the "hash" node presence doesn't mean a FIT conf or image
> node is signed. I also don't see many device trees with a signature
> node... which is kinda odd. Looking a bit into the signature node in the
> spec and binman tests, it's not clear to me if the hash node is reused
> by the signature node if if exists and if their algo should match and
> what exactly is checked by the signature node (like signing the hash
> string contained in the hash node(s) listed in sign-images property, or
> (re-)generating a hash of those and signing it at build time?
>
> If
> a) it is not possible to have a hash node's algo differ from a signature
> node's algo, then I'm fine with this as crc32 won't be allowed
> b) the signature node regenerates a hash regardless of the hash in the
> hash node, meaning the signature will be "appropriately secure"
> regardless of the algorithm listed in the hash node, then I'm fine with
> it too,
> c) it is possible to sign a crc32 hash, don't want it :)
>
> @Simon, do you have some hints about this?

In fit.py :
                # The hash subnodes here are for mkimage, not binman.
                entry.SetUpdateHash(False)

so yes they are independent, so far as Binman is concerned.

But of course, configuration signing relies on hashes in the image
nodes, so at least one strong hash is needed.

Regards,
Simon

Re: [PATCH v4 09/10] rockchip: binman: Support use of crc32 for SPL_FIT_SIGNATURE

[Quentin Schulz]

Hi Simon,

On 4/9/25 6:35 PM, Simon Glass wrote:
> Hi Quentin,
> 
> On Wed, 9 Apr 2025 at 10:11, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>
>> Hi Jonas,
>>
>> On 4/9/25 5:38 PM, Jonas Karlman wrote:
>>> Hi Quentin,
>>>
>>> On 2025-04-09 13:06, Quentin Schulz wrote:
>>>> Hi Jonas,
>>>>
>>>> On 3/29/25 4:06 PM, Jonas Karlman wrote:
>>>>> Use of SHA256 checksum validation on ARMv7 SoCs can be very time
>>>>> consuming compared to ARMv8 SoCs with Crypto Extensions.
>>>>>
>>>>> Add support for use of the crc32 hash algo when SHA256 is not supported.
>>>>> Also use a HAS_HASH to simplify the ifdefs when no known hash algo is
>>>>> compiled.
>>>>>
>>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>>
>>>> I don't know enough about general security but this very much looks like
>>>> a bad idea to me.
>>>>
>>>> https://web.archive.org/web/20170210173741/http://www.derkeiler.com/Newsgroups/sci.crypt/2003-07/1451.html
>>>>
>>>> """
>>>> While properly designed CRC's are good at detecting random errors in
>>>> the data (due to e.g. line noise), the CRC is useless as a secure
>>>> indicator of intentional manipulation of the data. And this is
>>>> because it's not hard at all to modify the data to produce any CRC
>>>> you desire (e.g. the same CRC as the original data, to try to
>>>> disguise your data manipulation).
>>>> """
>>>>
>>>> (yes I took the "first" link my web search engine returned me, thanks
>>>> confirmation bias!).
>>>
>>> I am fully aware, and the fallback to use crc32, and current primarily
>>> use of sha256, should not be considered a security feature. It is
>>> intended purely for a checksum validation of the binary blob after it
>>> has been loaded into memory and before U-Boot otherwise unconditionally
>>> jumps to and execute the loaded blob of binary code.
>>>
>>>>
>>>> I don't want to give people a false sense of security. If it really
>>>> comes down to it, I'd rather have an explicit Kconfig symbol to set this
>>>> value (maybe have a `choice` even) and be very clear about security
>>>> implications.
>>>
>>> Prior to the addition of the hash { algo=sha256 }, there was no checksum
>>> test and U-Boot SPL would unconditionally jump to the loaded data, even
>>> if it had been corrupted, was only halfway written or otherwise
>>> overwritten.
>>>
>>> The addition of crc32 instead of sha256 is just to allow older board
>>> variants with weaker SoCs to at least have some sort of checksum
>>> validation in use without affecting the boot time too much.
>>>
>>> On my rk3228 board, validation using sha256 could take a few seconds,
>>> and with crc32 it could be measured in ms.
>>>
>>> To me, having at least some default checksum validation is preferred
>>> over none at all.
>>>
>>
>> Except if this confuses people into thinking they have a secure system
>> except they use CRC32 instead of something more appropriate like SHA256.
>>
>> It seems like the "hash" node presence doesn't mean a FIT conf or image
>> node is signed. I also don't see many device trees with a signature
>> node... which is kinda odd. Looking a bit into the signature node in the
>> spec and binman tests, it's not clear to me if the hash node is reused
>> by the signature node if if exists and if their algo should match and
>> what exactly is checked by the signature node (like signing the hash
>> string contained in the hash node(s) listed in sign-images property, or
>> (re-)generating a hash of those and signing it at build time?
>>
>> If
>> a) it is not possible to have a hash node's algo differ from a signature
>> node's algo, then I'm fine with this as crc32 won't be allowed
>> b) the signature node regenerates a hash regardless of the hash in the
>> hash node, meaning the signature will be "appropriately secure"
>> regardless of the algorithm listed in the hash node, then I'm fine with
>> it too,
>> c) it is possible to sign a crc32 hash, don't want it :)
>>
>> @Simon, do you have some hints about this?
> 
> In fit.py :
>                  # The hash subnodes here are for mkimage, not binman.
>                  entry.SetUpdateHash(False)
> 
> so yes they are independent, so far as Binman is concerned.
> 
> But of course, configuration signing relies on hashes in the image
> nodes, so at least one strong hash is needed.
> 

Would/Could/Should those image node hash subnodes be autogenerated 
during signing based on the selected algo? In which case, only the algo 
in the signature subnode would actually matter from security PoV.

Cheers,
Quentin

Re: [PATCH v4 09/10] rockchip: binman: Support use of crc32 for SPL_FIT_SIGNATURE

[Jonas Karlman]

Hi Quentin,

On 2025-04-09 18:11, Quentin Schulz wrote:
> Hi Jonas,
> 
> On 4/9/25 5:38 PM, Jonas Karlman wrote:
>> Hi Quentin,
>>
>> On 2025-04-09 13:06, Quentin Schulz wrote:
>>> Hi Jonas,
>>>
>>> On 3/29/25 4:06 PM, Jonas Karlman wrote:
>>>> Use of SHA256 checksum validation on ARMv7 SoCs can be very time
>>>> consuming compared to ARMv8 SoCs with Crypto Extensions.
>>>>
>>>> Add support for use of the crc32 hash algo when SHA256 is not supported.
>>>> Also use a HAS_HASH to simplify the ifdefs when no known hash algo is
>>>> compiled.
>>>>
>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>
>>> I don't know enough about general security but this very much looks like
>>> a bad idea to me.
>>>
>>> https://web.archive.org/web/20170210173741/http://www.derkeiler.com/Newsgroups/sci.crypt/2003-07/1451.html
>>>
>>> """
>>> While properly designed CRC's are good at detecting random errors in
>>> the data (due to e.g. line noise), the CRC is useless as a secure
>>> indicator of intentional manipulation of the data. And this is
>>> because it's not hard at all to modify the data to produce any CRC
>>> you desire (e.g. the same CRC as the original data, to try to
>>> disguise your data manipulation).
>>> """
>>>
>>> (yes I took the "first" link my web search engine returned me, thanks
>>> confirmation bias!).
>>
>> I am fully aware, and the fallback to use crc32, and current primarily
>> use of sha256, should not be considered a security feature. It is
>> intended purely for a checksum validation of the binary blob after it
>> has been loaded into memory and before U-Boot otherwise unconditionally
>> jumps to and execute the loaded blob of binary code.
>>
>>>
>>> I don't want to give people a false sense of security. If it really
>>> comes down to it, I'd rather have an explicit Kconfig symbol to set this
>>> value (maybe have a `choice` even) and be very clear about security
>>> implications.
>>
>> Prior to the addition of the hash { algo=sha256 }, there was no checksum
>> test and U-Boot SPL would unconditionally jump to the loaded data, even
>> if it had been corrupted, was only halfway written or otherwise
>> overwritten.
>>
>> The addition of crc32 instead of sha256 is just to allow older board
>> variants with weaker SoCs to at least have some sort of checksum
>> validation in use without affecting the boot time too much.
>>
>> On my rk3228 board, validation using sha256 could take a few seconds,
>> and with crc32 it could be measured in ms.
>>
>> To me, having at least some default checksum validation is preferred
>> over none at all.
>>
> 
> Except if this confuses people into thinking they have a secure system 
> except they use CRC32 instead of something more appropriate like SHA256.
> 
> It seems like the "hash" node presence doesn't mean a FIT conf or image 
> node is signed.

Correct, current use of a hash node does not indicate any type of
signature validation, only that a hash value should be calculated and
added in the final FIT output.

The signature node must be added separately and/or injected at a later
stage, and for that the algo prop use a {checksum_algo},{crypto_algo} 
format. Where checksum_algo = [sha1, sha256, sha384, sha512] and
crypto_algo = [rsa2048, rsa3072, rsa4096, ecdsa256, ecdsa384, secp521r1].
See tools/image-sig-host.c for supported algos.

> I also don't see many device trees with a signature 
> node... which is kinda odd. Looking a bit into the signature node in the 
> spec and binman tests, it's not clear to me if the hash node is reused 
> by the signature node if if exists and if their algo should match and 
> what exactly is checked by the signature node (like signing the hash 
> string contained in the hash node(s) listed in sign-images property, or 
> (re-)generating a hash of those and signing it at build time?

I have not looked into how signing and signature validation works, and
if the checksum in signature { algo } must match the one used in
hash { algo }.

My personal interest have only ever been to add bare minimum checksum
validation in order to avoid executing code that is known to be broken
(a basic checksum test fails).

> 
> If
> a) it is not possible to have a hash node's algo differ from a signature 
> node's algo, then I'm fine with this as crc32 won't be allowed
> b) the signature node regenerates a hash regardless of the hash in the 
> hash node, meaning the signature will be "appropriately secure" 
> regardless of the algorithm listed in the hash node, then I'm fine with 
> it too,
> c) it is possible to sign a crc32 hash, don't want it :)

To actually validate a signature in SPL you would need to enable support
for RSA or ECDSA in SPL, without it only checksum validation may happen.

So yes, to get a proper secure solution there is much more to do than
just select a checksum algo. You need to correctly handle a signing key,
sign  FIT, preferably also sign the rk boot image, burn public cert in
OTP and more.

Not sure what you want me to do here, in my mind the use of a hash algo
has never been an indication of security, and the default to use sha256
was only because it was supported by ARMv8 CE on the SoC being tested
when I added the original hash { algo } prop in commit 99e3a2cd4e74
("rockchip: Add sha256 hash to FIT images").

The option to use crc32 as hash algo fallback was only ever to at least
give a better default option than not doing any type of checksum test.

Regards,
Jonas

> 
> @Simon, do you have some hints about this?
> 
> Cheers,
> Quentin

Re: [PATCH v4 08/10] rockchip: binman: Use the skip-at-start prop in simple-bin image

[Quentin Schulz]

Hi Simon,

On 4/9/25 4:30 PM, Simon Glass wrote:
> Hi Quentin,
> 
> On Wed, 9 Apr 2025 at 07:35, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>
>> Hi Simon,
>>
>> On 4/9/25 3:33 PM, Simon Glass wrote:
>>> Hi Quentin,
>>>
>>> On Wed, 9 Apr 2025 at 07:32, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>>>
>>>> Hi Simon,
>>>>
>>>> On 4/9/25 3:22 PM, Simon Glass wrote:
>>>>> Hi Quentin,
>>>>>
>>>>> On Wed, 9 Apr 2025 at 04:57, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>>>>>
>>>>>> Hi Jonas, Simon,
>>>>>>
>>>>>> On 3/29/25 4:06 PM, Jonas Karlman wrote:
>>>>>>> From: Simon Glass <sjg@chromium.org>
>>>>>>>
>>>>>>> The simple-bin image is normally written to MMC media at block 64, which
>>>>>>> is a 32K offset from start of storage media.
>>>>>>>
>>>>>>> Set the skip-at-start property to 0x8000 (32 KiB) so that fdtmap and
>>>>>>> other embedded binman symbols in the output binary is referencing image
>>>>>>> offsets correctly.
>>>>>>>
>>>>>>
>>>>>> Shouldn't we have this commit BEFORE we add the `fdtmap` node since we
>>>>>> know it's wrong before this commit?
>>>>>>
>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>>>>> ---
>>>>>>> Changes in v4:
>>>>>>> - Drop defconfig changes
>>>>>>> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
>>>>>>>
>>>>>>> Changes in v2:
>>>>>>> - Move this patch to the end of the series
>>>>>>> - Drop 0x8000 offset for SPI
>>>>>>> ---
>>>>>>>      arch/arm/dts/rockchip-u-boot.dtsi | 3 ++-
>>>>>>>      1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
>>>>>>> index fb38b7b80c43..65b81bf58626 100644
>>>>>>> --- a/arch/arm/dts/rockchip-u-boot.dtsi
>>>>>>> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
>>>>>>> @@ -154,6 +154,7 @@
>>>>>>>          simple-bin {
>>>>>>>                  filename = "u-boot-rockchip.bin";
>>>>>>>                  pad-byte = <0xff>;
>>>>>>> +             skip-at-start = <0x8000>;
>>>>>>>
>>>>>>>                  mkimage {
>>>>>>>                          filename = "idbloader.img";
>>>>>>> @@ -178,7 +179,7 @@
>>>>>>>      #else
>>>>>>>                  u-boot-img {
>>>>>>>      #endif
>>>>>>> -                     offset = <CONFIG_SPL_PAD_TO>;
>>>>>>> +                     offset = <(CONFIG_SPL_PAD_TO + 0x8000)>;
>>>>>>
>>>>>> This is confusing. The documentation states:
>>>>>>
>>>>>> """
>>>>>> offset:
>>>>>>         This sets the offset of an entry within the image or section containing
>>>>>>         it.
>>>>>> """
>>>>>>
>>>>>> My understanding is that it should be relative to the beginning of the
>>>>>> image but this now needs the knowledge of where it will be stored on the
>>>>>> MMC device (via the value in skip-at-start).
>>>>>>
>>>>>> Why is skip-at-start automatically deducted from offset?
>>>>>
>>>>> This is how binman works[1]. We are trying to use the feature designs
>>>>
>>>> Why is it deducted?
>>>
>>> Are you asking why skip-at-start is deducted from the offset?
>>>
>>
>> Yes
> 
> It is confusing, unfortunately.
> 
> When you use an offset (say 0x78000) then normally the entry will
> start at that offset in the image.
> 
> When you use skip-at-start 0x8000, its value is added to all top-level
> offsets in the image, so the offset becomes 0x80000
> 
> BUT the image built by binman does not contain the first 0x8000 bytes.
> It is expected that the image is written to offset 0x8000 so that the
> offsets will be correct when used within U-Boot itself.
> 

I would assume all offsets are relative to the beginning of the image in 
the binary? Adding skip-at-start doesn't add 0x8000 bytes at the 
beginning of the binary file, so why would the offsets need to be modified?

Also, while 0x8000 is the typical address the image can be flashed, it 
is not necessarily where it will be as the BootROM tries a few other 
offsets if it cannot find one at 32KiB offset in the storage medium. 
This seems to me to be a case of "somewhat helps in one case, but makes 
things more confusing in others"? Will we need different offsets 
depending on where the FIT is flashed? What happens for A/B updates then?

I understand that we currently essentially have skip-at-start = 0; and 
that it is bad because it doesn't reflect the actual address, but how is 
that worse than hardcoding a different offset?

Cheers,
Quentin

Re: [PATCH v4 08/10] rockchip: binman: Use the skip-at-start prop in simple-bin image

[Simon Glass]

Hi Quentin,

On Mon, 14 Apr 2025 at 09:10, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Simon,
>
> On 4/9/25 4:30 PM, Simon Glass wrote:
> > Hi Quentin,
> >
> > On Wed, 9 Apr 2025 at 07:35, Quentin Schulz <quentin.schulz@cherry.de> wrote:
> >>
> >> Hi Simon,
> >>
> >> On 4/9/25 3:33 PM, Simon Glass wrote:
> >>> Hi Quentin,
> >>>
> >>> On Wed, 9 Apr 2025 at 07:32, Quentin Schulz <quentin.schulz@cherry.de> wrote:
> >>>>
> >>>> Hi Simon,
> >>>>
> >>>> On 4/9/25 3:22 PM, Simon Glass wrote:
> >>>>> Hi Quentin,
> >>>>>
> >>>>> On Wed, 9 Apr 2025 at 04:57, Quentin Schulz <quentin.schulz@cherry.de> wrote:
> >>>>>>
> >>>>>> Hi Jonas, Simon,
> >>>>>>
> >>>>>> On 3/29/25 4:06 PM, Jonas Karlman wrote:
> >>>>>>> From: Simon Glass <sjg@chromium.org>
> >>>>>>>
> >>>>>>> The simple-bin image is normally written to MMC media at block 64, which
> >>>>>>> is a 32K offset from start of storage media.
> >>>>>>>
> >>>>>>> Set the skip-at-start property to 0x8000 (32 KiB) so that fdtmap and
> >>>>>>> other embedded binman symbols in the output binary is referencing image
> >>>>>>> offsets correctly.
> >>>>>>>
> >>>>>>
> >>>>>> Shouldn't we have this commit BEFORE we add the `fdtmap` node since we
> >>>>>> know it's wrong before this commit?
> >>>>>>
> >>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
> >>>>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> >>>>>>> ---
> >>>>>>> Changes in v4:
> >>>>>>> - Drop defconfig changes
> >>>>>>> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
> >>>>>>>
> >>>>>>> Changes in v2:
> >>>>>>> - Move this patch to the end of the series
> >>>>>>> - Drop 0x8000 offset for SPI
> >>>>>>> ---
> >>>>>>>      arch/arm/dts/rockchip-u-boot.dtsi | 3 ++-
> >>>>>>>      1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> >>>>>>> index fb38b7b80c43..65b81bf58626 100644
> >>>>>>> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> >>>>>>> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> >>>>>>> @@ -154,6 +154,7 @@
> >>>>>>>          simple-bin {
> >>>>>>>                  filename = "u-boot-rockchip.bin";
> >>>>>>>                  pad-byte = <0xff>;
> >>>>>>> +             skip-at-start = <0x8000>;
> >>>>>>>
> >>>>>>>                  mkimage {
> >>>>>>>                          filename = "idbloader.img";
> >>>>>>> @@ -178,7 +179,7 @@
> >>>>>>>      #else
> >>>>>>>                  u-boot-img {
> >>>>>>>      #endif
> >>>>>>> -                     offset = <CONFIG_SPL_PAD_TO>;
> >>>>>>> +                     offset = <(CONFIG_SPL_PAD_TO + 0x8000)>;
> >>>>>>
> >>>>>> This is confusing. The documentation states:
> >>>>>>
> >>>>>> """
> >>>>>> offset:
> >>>>>>         This sets the offset of an entry within the image or section containing
> >>>>>>         it.
> >>>>>> """
> >>>>>>
> >>>>>> My understanding is that it should be relative to the beginning of the
> >>>>>> image but this now needs the knowledge of where it will be stored on the
> >>>>>> MMC device (via the value in skip-at-start).
> >>>>>>
> >>>>>> Why is skip-at-start automatically deducted from offset?
> >>>>>
> >>>>> This is how binman works[1]. We are trying to use the feature designs
> >>>>
> >>>> Why is it deducted?
> >>>
> >>> Are you asking why skip-at-start is deducted from the offset?
> >>>
> >>
> >> Yes
> >
> > It is confusing, unfortunately.
> >
> > When you use an offset (say 0x78000) then normally the entry will
> > start at that offset in the image.
> >
> > When you use skip-at-start 0x8000, its value is added to all top-level
> > offsets in the image, so the offset becomes 0x80000
> >
> > BUT the image built by binman does not contain the first 0x8000 bytes.
> > It is expected that the image is written to offset 0x8000 so that the
> > offsets will be correct when used within U-Boot itself.
> >
>
> I would assume all offsets are relative to the beginning of the image in
> the binary? Adding skip-at-start doesn't add 0x8000 bytes at the
> beginning of the binary file, so why would the offsets need to be modified?
>
> Also, while 0x8000 is the typical address the image can be flashed, it
> is not necessarily where it will be as the BootROM tries a few other
> offsets if it cannot find one at 32KiB offset in the storage medium.
> This seems to me to be a case of "somewhat helps in one case, but makes
> things more confusing in others"? Will we need different offsets
> depending on where the FIT is flashed? What happens for A/B updates then?
>
> I understand that we currently essentially have skip-at-start = 0; and
> that it is bad because it doesn't reflect the actual address, but how is
> that worse than hardcoding a different offset?

It's probably best that we discuss this on a call. You could also give
it a try, e.g. try accessing data in the U-Boot image from SPL and see
how it works for you.

Regards,
Simon