[RFC PATCH v2 0/8] Build Rockchip final images using binman

[RFC PATCH v2 0/8] Build Rockchip final images using binman

[Andrew Abbott]

My original goal was to produce SPI images for Rockchip platforms (specifically
for me, ROCKPro64, and in the future ROCK64). Looking into it, it seemed nicer
to just switch the SD/MMC image generation over to binman as well in the
process.

This is my attempt to move Rockchip final full image generation to binman,
adding the option to make full SPI images as well.

Other questions:

- I noticed that ATF generation for ARM64 Rockchip is done via a Python script
  instead of binman. I don't currently know how to change that over to binman,
  but is that something worth pursuing as part of this?

Please give me your feedback!

Changes in v2:
- Revert u-boot-rockchip-sdmmc.bin name to u-boot-rockchip.bin, to
  keep the name the same as before.
- Fix whitespace issues.
- Remove note from docs about different offsets in SPI flash for
  different SoCs - this was a bad assumption on my part, it doesn't work
  this way.
- Update name of SD/MMC image in the docs from u-boot-rockchip-sdmmc.bin
  to u-boot-rockchip.bin.

Andrew Abbott (8):
  binman: mkimage: Support ':'-separated inputs
  rockchip: Add binman definitions for final images
  soc: rockchip: Include common U-Boot dtsi file
  board: rockchip: Move SPI U-Boot offset to config
  rockchip: Remove obsolete Makefile targets
  rockchip: Enable binman for ARM64
  doc: rockchip: Update for new binman image generation
  board: rockpro64: Enable building SPI image

 Kconfig                                      |  4 +-
 Makefile                                     | 31 ++----------
 arch/arm/Kconfig                             |  2 +-
 arch/arm/dts/rk3308-u-boot.dtsi              |  2 +
 arch/arm/dts/rk3328-u-boot.dtsi              |  2 +
 arch/arm/dts/rk3368-lion-haikou-u-boot.dtsi  |  1 -
 arch/arm/dts/rk3368-u-boot.dtsi              |  1 +
 arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi |  4 --
 arch/arm/dts/rk3399-puma-haikou-u-boot.dtsi  |  1 -
 arch/arm/dts/rk3399-roc-pc-u-boot.dtsi       |  4 --
 arch/arm/dts/rk3399-rockpro64-u-boot.dtsi    |  4 --
 arch/arm/dts/rk3568-u-boot.dtsi              |  2 +
 arch/arm/dts/rockchip-u-boot.dtsi            | 53 ++++++++++++++++++--
 arch/arm/mach-rockchip/Kconfig               |  7 ++-
 arch/arm/mach-rockchip/rk3399/Kconfig        |  1 +
 configs/lion-rk3368_defconfig                |  1 +
 configs/pinebook-pro-rk3399_defconfig        |  1 +
 configs/puma-rk3399_defconfig                |  2 +-
 configs/roc-pc-rk3399_defconfig              |  1 +
 configs/rockpro64-rk3399_defconfig           |  1 +
 doc/board/rockchip/rockchip.rst              | 29 +++--------
 tools/binman/etype/mkimage.py                | 33 +++++++-----
 22 files changed, 100 insertions(+), 87 deletions(-)

-- 
2.36.0

[RFC PATCH v2 1/8] binman: mkimage: Support ':'-separated inputs

[Andrew Abbott]

mkimage supports combining multiple input binaries separating them
with colons (':'). This is used at least for Rockchip platforms to
encode payload offsets and sizes in the image header. It is required for
Rockchip SPI boot since for the rkspi format, after the input binary
combining, the entire image is spread across only the first 2K bytes of
each 4K block.

Previous to this change, multiple inputs to a binman mkimage node would
just be concatenated and the combined input would be passed as the -d
argument to mkimage. Now, the inputs are instead passed colon-separated.

Signed-off-by: Andrew Abbott <andrew@mirx.dev>
---
This is a bit of a messy implementation for now and would probably break
existing uses of mkimage that rely on the concatenation behaviour.

Questions:
- Should this be a separate entry type, or an option to the mkimage
  entry type that enables this behaviour?
- What kind of test(s) should I add?

(no changes since v1)

 tools/binman/etype/mkimage.py | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
index 5f6def2287..8cea618fbd 100644
--- a/tools/binman/etype/mkimage.py
+++ b/tools/binman/etype/mkimage.py
@@ -51,21 +51,30 @@ class Entry_mkimage(Entry):
         self.ReadEntries()
 
     def ObtainContents(self):
-        # Use a non-zero size for any fake files to keep mkimage happy
-        data, input_fname, uniq = self.collect_contents_to_file(
-            self._mkimage_entries.values(), 'mkimage', 1024)
-        if data is None:
-            return False
-        output_fname = tools.get_output_filename('mkimage-out.%s' % uniq)
-        if self.mkimage.run_cmd('-d', input_fname, *self._args,
-                                output_fname) is not None:
+        # For multiple inputs to mkimage, we want to separate them by colons.
+        # This is needed for eg. the rkspi format, which treats the first data
+        # file as the "init" and the second as "boot" and sets the image header
+        # accordingly, then makes the image so that only the first 2 KiB of each
+        # 4KiB block is used.
+
+        data_filenames = []
+        for entry in self._mkimage_entries.values():
+            # First get the input data and put it in a file. If any entry is not
+            # available, try later.
+            if not entry.ObtainContents():
+                return False
+
+            input_fname = tools.get_output_filename('mkimage-in.%s' % entry.GetUniqueName())
+            data_filenames.append(input_fname)
+            tools.write_file(input_fname, entry.GetData())
+
+        output_fname = tools.get_output_filename('mkimage-out.%s' % self.GetUniqueName())
+        if self.mkimage.run_cmd('-d', ":".join(data_filenames), *self._args, output_fname):
             self.SetContents(tools.read_file(output_fname))
+            return True
         else:
-            # Bintool is missing; just use the input data as the output
             self.record_missing_bintool(self.mkimage)
-            self.SetContents(data)
-
-        return True
+            return False
 
     def ReadEntries(self):
         """Read the subnodes to find out what should go in this image"""
-- 
2.36.0

[RFC PATCH v2 2/8] rockchip: Add binman definitions for final images

[Andrew Abbott]

Currently, building for Rockchip targets produces:

- idbloader.img
    - rksd-formatted TPL with SPL appended; or
    - rksd-formatted SPL
- u-boot.itb
    - U-Boot Proper FIT image
- u-boot-rockchip.bin
    - idbloader.img + u-boot.itb, padded the correct amount for SD/MMC
      usage.

For RK3399 targets:

- u-boot.rom
    - SPI image specific to the bob Chromebook target (see
      c4cea2bbf995764f325a907061c22ecd6768cf7b).

This commit adds binman definitions to produce these images:

- idbloader.img
    - rksd-formatted [TPL + ] SPL, as before.
- u-boot-rockchip.bin
    - [TPL + ] SPL all rksd-formatted + u-boot.itb padded for SD/MMC
      usage, as before.
- u-boot-rockchip-spi.bin
    - [TPL + ] SPL all rkspi-formatted + u-boot.itb padded for SPI
      usage.

This commit also generalizes the CONFIG_ROCKCHIP_SPI_IMAGE config
setting - it now means to generate a generic SPI flash image, in
addition to the generic SD/MMC image.

Signed-off-by: Andrew Abbott <andrew@mirx.dev>
---
Question: Does this break/not play nicely with rockchip-optee
generation? It creates u-boot.itb for rk3288 targets. That would need to
run before what I've implemented here?

Changes in v2:
- Revert u-boot-rockchip-sdmmc.bin name to u-boot-rockchip.bin, to
  keep the name the same as before.
- Fix whitespace issues.

 arch/arm/dts/rockchip-u-boot.dtsi | 53 +++++++++++++++++++++++++++++--
 arch/arm/mach-rockchip/Kconfig    |  7 ++--
 2 files changed, 53 insertions(+), 7 deletions(-)

diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index eae3ee715d..9354b0f5a7 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -13,17 +13,64 @@
 
 #ifdef CONFIG_SPL
 &binman {
-	simple-bin {
+	sdmmc-idbloader {
+		filename = "idbloader.img";
+
+		mkimage {
+			args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
+
+#ifdef CONFIG_TPL
+			u-boot-tpl {};
+#endif
+			u-boot-spl {};
+		};
+	};
+
+	sdmmc-image {
 		filename = "u-boot-rockchip.bin";
 		pad-byte = <0xff>;
 
-		blob {
+		idbloader {
 			filename = "idbloader.img";
+			type = "blob";
 		};
 
-		u-boot-img {
+#ifdef CONFIG_ARM64
+		u-boot-fit {
+			filename = "u-boot.itb";
+			type = "blob";
 			offset = <CONFIG_SPL_PAD_TO>;
 		};
+#else
+		u-boot-img {};
+#endif
 	};
 };
+
+#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
+&binman {
+	spi-image {
+		filename = "u-boot-rockchip-spi.bin";
+		pad-byte = <0xff>;
+
+		mkimage {
+			args = "-n", CONFIG_SYS_SOC, "-T", "rkspi";
+
+#ifdef CONFIG_TPL
+			u-boot-tpl {};
+#endif
+			u-boot-spl {};
+		};
+
+#ifdef CONFIG_ARM64
+		blob {
+			filename = "u-boot.itb";
+			offset = <CONFIG_SYS_SPI_U_BOOT_OFFS>;
+		};
+#else
+		u-boot-img {};
 #endif
+	};
+};
+#endif // CONFIG_ROCKCHIP_SPI_IMAGE
+#endif // CONFIG_SPL
diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
index 18aff5480b..7149b9a530 100644
--- a/arch/arm/mach-rockchip/Kconfig
+++ b/arch/arm/mach-rockchip/Kconfig
@@ -415,12 +415,11 @@ config SPL_MMC
 
 config ROCKCHIP_SPI_IMAGE
 	bool "Build a SPI image for rockchip"
-	depends on HAS_ROM
 	help
 	  Some Rockchip SoCs support booting from SPI flash. Enable this
-	  option to produce a 4MB SPI-flash image (called u-boot.rom)
-	  containing U-Boot. The image is built by binman. U-Boot sits near
-	  the start of the image.
+	  option to produce an SPI-flash image (called u-boot-rockchip-spi.bin)
+	  containing TPL (if enabled) and SPL, and U-Boot proper at the offset
+	  CONFIG_SYS_SPI_U_BOOT_OFFS. The image is built by binman.
 
 config LNX_KRNL_IMG_TEXT_OFFSET_BASE
 	default SYS_TEXT_BASE
-- 
2.36.0

[RFC PATCH v2 3/8] soc: rockchip: Include common U-Boot dtsi file

[Andrew Abbott]

This pulls in binman definitions so images can be built for all
Rockchip platforms.

Signed-off-by: Andrew Abbott <andrew@mirx.dev>
---

(no changes since v1)

 arch/arm/dts/rk3308-u-boot.dtsi | 2 ++
 arch/arm/dts/rk3328-u-boot.dtsi | 2 ++
 arch/arm/dts/rk3368-u-boot.dtsi | 1 +
 arch/arm/dts/rk3568-u-boot.dtsi | 2 ++
 4 files changed, 7 insertions(+)

diff --git a/arch/arm/dts/rk3308-u-boot.dtsi b/arch/arm/dts/rk3308-u-boot.dtsi
index 4bfad31fba..ab5bfc2ce9 100644
--- a/arch/arm/dts/rk3308-u-boot.dtsi
+++ b/arch/arm/dts/rk3308-u-boot.dtsi
@@ -3,6 +3,8 @@
  *(C) Copyright 2019 Rockchip Electronics Co., Ltd
  */
 
+#include "rockchip-u-boot.dtsi"
+
 / {
 	aliases {
 		mmc0 = &emmc;
diff --git a/arch/arm/dts/rk3328-u-boot.dtsi b/arch/arm/dts/rk3328-u-boot.dtsi
index 1633558264..d4a7540a92 100644
--- a/arch/arm/dts/rk3328-u-boot.dtsi
+++ b/arch/arm/dts/rk3328-u-boot.dtsi
@@ -3,6 +3,8 @@
  * (C) Copyright 2019 Rockchip Electronics Co., Ltd
  */
 
+#include "rockchip-u-boot.dtsi"
+
 / {
 	aliases {
 		mmc0 = &emmc;
diff --git a/arch/arm/dts/rk3368-u-boot.dtsi b/arch/arm/dts/rk3368-u-boot.dtsi
index 2767c2678d..b37da4e851 100644
--- a/arch/arm/dts/rk3368-u-boot.dtsi
+++ b/arch/arm/dts/rk3368-u-boot.dtsi
@@ -3,6 +3,7 @@
  * Copyright (c) 2020 Theobroma Systems Design und Consulting GmbH
  */
 
+#include "rockchip-u-boot.dtsi"
 #include <dt-bindings/memory/rk3368-dmc.h>
 
 / {
diff --git a/arch/arm/dts/rk3568-u-boot.dtsi b/arch/arm/dts/rk3568-u-boot.dtsi
index 5a80dda275..fa9b6ae23b 100644
--- a/arch/arm/dts/rk3568-u-boot.dtsi
+++ b/arch/arm/dts/rk3568-u-boot.dtsi
@@ -3,6 +3,8 @@
  * (C) Copyright 2021 Rockchip Electronics Co., Ltd
  */
 
+#include "rockchip-u-boot.dtsi"
+
 / {
 	aliases {
 		mmc0 = &sdhci;
-- 
2.36.0

[RFC PATCH v2 4/8] board: rockchip: Move SPI U-Boot offset to config

[Andrew Abbott]

This needs to be accessible to binman (via CONFIG_ preprocessor macros)
so it can build SPI images using the correct offset.

The documentation at 'doc/device-tree-bindings/config.txt' says that
the 'u-boot,spl-payload-offset' device tree option simply overrides
'CONFIG_SYS_SPI_U_BOOT_OFFS', so this change should not functionally
change the offset for any of the affected boards.

Signed-off-by: Andrew Abbott <andrew@mirx.dev>
---

(no changes since v1)

 arch/arm/dts/rk3368-lion-haikou-u-boot.dtsi  | 1 -
 arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi | 4 ----
 arch/arm/dts/rk3399-puma-haikou-u-boot.dtsi  | 1 -
 arch/arm/dts/rk3399-roc-pc-u-boot.dtsi       | 4 ----
 arch/arm/dts/rk3399-rockpro64-u-boot.dtsi    | 4 ----
 configs/lion-rk3368_defconfig                | 1 +
 configs/pinebook-pro-rk3399_defconfig        | 1 +
 configs/puma-rk3399_defconfig                | 2 +-
 configs/roc-pc-rk3399_defconfig              | 1 +
 configs/rockpro64-rk3399_defconfig           | 1 +
 10 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/arch/arm/dts/rk3368-lion-haikou-u-boot.dtsi b/arch/arm/dts/rk3368-lion-haikou-u-boot.dtsi
index 7826d1e70b..6840182f03 100644
--- a/arch/arm/dts/rk3368-lion-haikou-u-boot.dtsi
+++ b/arch/arm/dts/rk3368-lion-haikou-u-boot.dtsi
@@ -7,7 +7,6 @@
 
 / {
 	config {
-		u-boot,spl-payload-offset = <0x40000>; /* @ 256KB */
 		u-boot,mmc-env-offset = <0x4000>;      /* @  16KB */
 	};
 
diff --git a/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi b/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
index 2d87bea933..d1d0ac460d 100644
--- a/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
+++ b/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
@@ -10,10 +10,6 @@
 	chosen {
 		u-boot,spl-boot-order = "same-as-spl", &sdhci, &spiflash, &sdmmc;
 	};
-
-	config {
-		u-boot,spl-payload-offset = <0x60000>; /* @ 384KB */
-	};
 };
 
 &edp {
diff --git a/arch/arm/dts/rk3399-puma-haikou-u-boot.dtsi b/arch/arm/dts/rk3399-puma-haikou-u-boot.dtsi
index e0476ab25c..f3f8619716 100644
--- a/arch/arm/dts/rk3399-puma-haikou-u-boot.dtsi
+++ b/arch/arm/dts/rk3399-puma-haikou-u-boot.dtsi
@@ -14,7 +14,6 @@
 
 / {
 	config {
-		u-boot,spl-payload-offset = <0x40000>; /* @ 256KB */
 		u-boot,mmc-env-offset = <0x4000>;      /* @  16KB */
 		u-boot,efi-partition-entries-offset = <0x200000>; /* 2MB */
 		u-boot,boot-led = "module_led";
diff --git a/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi b/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi
index e3c9364e35..a54e554d8a 100644
--- a/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi
+++ b/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi
@@ -11,10 +11,6 @@
 		u-boot,spl-boot-order = "same-as-spl", &spi_flash, &sdhci, &sdmmc;
 	};
 
-	config {
-		u-boot,spl-payload-offset = <0x60000>; /* @ 384KB */
-	};
-
 	vcc_hub_en: vcc_hub_en-regulator {
 		compatible = "regulator-fixed";
 		enable-active-high;
diff --git a/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi b/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
index 37dff04adf..bd65496df3 100644
--- a/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
+++ b/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
@@ -9,10 +9,6 @@
 	chosen {
 		u-boot,spl-boot-order = "same-as-spl", &spi_flash, &sdmmc, &sdhci;
 	};
-
-	config {
-		u-boot,spl-payload-offset = <0x60000>; /* @ 384KB */
-	};
 };
 
 &spi1 {
diff --git a/configs/lion-rk3368_defconfig b/configs/lion-rk3368_defconfig
index 426913816b..91630c822b 100644
--- a/configs/lion-rk3368_defconfig
+++ b/configs/lion-rk3368_defconfig
@@ -17,6 +17,7 @@ CONFIG_DEBUG_UART_BASE=0xFF180000
 CONFIG_DEBUG_UART_CLOCK=24000000
 CONFIG_SPL_SPI_FLASH_SUPPORT=y
 CONFIG_SPL_SPI=y
+CONFIG_SYS_SPI_U_BOOT_OFFS=0x40000
 CONFIG_SYS_LOAD_ADDR=0x800800
 CONFIG_DEBUG_UART=y
 CONFIG_FIT=y
diff --git a/configs/pinebook-pro-rk3399_defconfig b/configs/pinebook-pro-rk3399_defconfig
index 8ca1d0708f..c5ebf62f02 100644
--- a/configs/pinebook-pro-rk3399_defconfig
+++ b/configs/pinebook-pro-rk3399_defconfig
@@ -24,6 +24,7 @@ CONFIG_SPL_STACK_R=y
 CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
 CONFIG_SPL_MTD_SUPPORT=y
 CONFIG_SPL_SPI_LOAD=y
+CONFIG_SYS_SPI_U_BOOT_OFFS=0x60000
 CONFIG_TPL=y
 CONFIG_CMD_BOOTZ=y
 CONFIG_CMD_GPIO=y
diff --git a/configs/puma-rk3399_defconfig b/configs/puma-rk3399_defconfig
index 7ce2dc0719..6b7898be49 100644
--- a/configs/puma-rk3399_defconfig
+++ b/configs/puma-rk3399_defconfig
@@ -27,7 +27,7 @@ CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR=0x200
 CONFIG_SPL_I2C=y
 CONFIG_SPL_POWER=y
 CONFIG_SPL_SPI_LOAD=y
-CONFIG_SYS_SPI_U_BOOT_OFFS=0x20000
+CONFIG_SYS_SPI_U_BOOT_OFFS=0x40000
 CONFIG_CMD_BOOTZ=y
 CONFIG_CMD_GPT=y
 CONFIG_CMD_I2C=y
diff --git a/configs/roc-pc-rk3399_defconfig b/configs/roc-pc-rk3399_defconfig
index 4684fa6e74..2d7e2ad563 100644
--- a/configs/roc-pc-rk3399_defconfig
+++ b/configs/roc-pc-rk3399_defconfig
@@ -25,6 +25,7 @@ CONFIG_SPL_STACK_R=y
 CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x20000
 CONFIG_SPL_ENV_SUPPORT=y
 CONFIG_SPL_SPI_LOAD=y
+CONFIG_SYS_SPI_U_BOOT_OFFS=0x60000
 CONFIG_TPL=y
 CONFIG_CMD_BOOTZ=y
 CONFIG_CMD_GPT=y
diff --git a/configs/rockpro64-rk3399_defconfig b/configs/rockpro64-rk3399_defconfig
index e6f7a8469a..8c45a6c575 100644
--- a/configs/rockpro64-rk3399_defconfig
+++ b/configs/rockpro64-rk3399_defconfig
@@ -23,6 +23,7 @@ CONFIG_MISC_INIT_R=y
 CONFIG_SPL_STACK_R=y
 CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
 CONFIG_SPL_SPI_LOAD=y
+CONFIG_SYS_SPI_U_BOOT_OFFS=0x60000
 CONFIG_TPL=y
 CONFIG_CMD_BOOTZ=y
 CONFIG_CMD_GPT=y
-- 
2.36.0

[RFC PATCH v2 5/8] rockchip: Remove obsolete Makefile targets

[Andrew Abbott]

These are obsoleted by a previous patch which added binman image
definitions for Rockchip SD/MMC and SPI images.

Signed-off-by: Andrew Abbott <andrew@mirx.dev>
---

(no changes since v1)

 Makefile | 31 +++----------------------------
 1 file changed, 3 insertions(+), 28 deletions(-)

diff --git a/Makefile b/Makefile
index e3ce287918..ea7a2031e7 100644
--- a/Makefile
+++ b/Makefile
@@ -991,16 +991,15 @@ INPUTS-y += u-boot-with-dtb.bin
 endif
 
 ifeq ($(CONFIG_ARCH_ROCKCHIP),y)
-# On ARM64 this target is produced by binman so we don't need this dep
 ifeq ($(CONFIG_ARM64),y)
 ifeq ($(CONFIG_SPL),y)
 # TODO: Get binman to generate this too
-INPUTS-y += u-boot-rockchip.bin
+INPUTS-y += u-boot.itb
 endif
 else
 ifeq ($(CONFIG_SPL),y)
-# Generate these inputs for binman which will create the output files
-INPUTS-y += idbloader.img u-boot.img
+# Generate this input for binman which will create the output files
+INPUTS-y += u-boot.img
 endif
 endif
 endif
@@ -1488,30 +1487,6 @@ OBJCOPYFLAGS_u-boot-with-spl.bin = -I binary -O binary \
 u-boot-with-spl.bin: $(SPL_IMAGE) $(SPL_PAYLOAD) FORCE
 	$(call if_changed,pad_cat)
 
-ifeq ($(CONFIG_ARCH_ROCKCHIP),y)
-
-# TPL + SPL
-ifeq ($(CONFIG_SPL)$(CONFIG_TPL),yy)
-MKIMAGEFLAGS_u-boot-tpl-rockchip.bin = -n $(CONFIG_SYS_SOC) -T rksd
-tpl/u-boot-tpl-rockchip.bin: tpl/u-boot-tpl.bin FORCE
-	$(call if_changed,mkimage)
-idbloader.img: tpl/u-boot-tpl-rockchip.bin spl/u-boot-spl.bin FORCE
-	$(call if_changed,cat)
-else
-MKIMAGEFLAGS_idbloader.img = -n $(CONFIG_SYS_SOC) -T rksd
-idbloader.img: spl/u-boot-spl.bin FORCE
-	$(call if_changed,mkimage)
-endif
-
-ifeq ($(CONFIG_ARM64),y)
-OBJCOPYFLAGS_u-boot-rockchip.bin = -I binary -O binary \
-	--pad-to=$(CONFIG_SPL_PAD_TO) --gap-fill=0xff
-u-boot-rockchip.bin: idbloader.img u-boot.itb FORCE
-	$(call if_changed,pad_cat)
-endif # CONFIG_ARM64
-
-endif # CONFIG_ARCH_ROCKCHIP
-
 ifeq ($(CONFIG_ARCH_LPC32XX)$(CONFIG_SPL),yy)
 MKIMAGEFLAGS_lpc32xx-spl.img = -T lpc32xximage -a $(CONFIG_SPL_TEXT_BASE)
 
-- 
2.36.0

[RFC PATCH v2 6/8] rockchip: Enable binman for ARM64

[Andrew Abbott]

Binman is now being used to build the final flashable images for
Rockchip devices, thus enabling it for all Rockchip targets here. But
it is not yet being used to generate the FIT image (u-boot.itb),
thus we need to force it to be built.

Signed-off-by: Andrew Abbott <andrew@mirx.dev>
---
Question: Will this causes issues with eg. Chromebook gru/bob, which build
u-boot.itb with binman already?

(no changes since v1)

 Kconfig          | 4 ++--
 arch/arm/Kconfig | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Kconfig b/Kconfig
index 797038b037..7226986830 100644
--- a/Kconfig
+++ b/Kconfig
@@ -414,8 +414,8 @@ config BUILD_TARGET
 	default "u-boot-with-spl.sfp" if TARGET_SOCFPGA_GEN5
 	default "u-boot-spl.kwb" if ARCH_MVEBU && SPL
 	default "u-boot-elf.srec" if RCAR_GEN3
-	default "u-boot.itb" if !BINMAN && SPL_LOAD_FIT && (ARCH_ROCKCHIP || \
-				ARCH_SUNXI || RISCV || ARCH_ZYNQMP)
+	default "u-boot.itb" if ARCH_ROCKCHIP || (!BINMAN && SPL_LOAD_FIT && \
+				(ARCH_SUNXI || RISCV || ARCH_ZYNQMP))
 	default "u-boot.kwb" if ARCH_KIRKWOOD
 	default "u-boot-with-spl.bin" if ARCH_AT91 && SPL_NAND_SUPPORT
 	default "u-boot-with-spl.imx" if ARCH_MX6 && SPL
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 0afec5155b..545bf9a8cc 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1967,7 +1967,7 @@ config ARCH_STM32MP
 config ARCH_ROCKCHIP
 	bool "Support Rockchip SoCs"
 	select BLK
-	select BINMAN if SPL_OPTEE || (SPL && !ARM64)
+	select BINMAN if SPL
 	select DM
 	select DM_GPIO
 	select DM_I2C
-- 
2.36.0

[RFC PATCH v2 7/8] doc: rockchip: Update for new binman image generation

[Andrew Abbott]

Update the Rockchip documentation for image generation with binman,
including the new automatically-created SPI image.

Signed-off-by: Andrew Abbott <andrew@mirx.dev>
---

Changes in v2:
- Remove note from docs about different offsets in SPI flash for
  different SoCs - this was a bad assumption on my part, it doesn't work
  this way.
- Update name of SD/MMC image in the docs from u-boot-rockchip-sdmmc.bin
  to u-boot-rockchip.bin.

 doc/board/rockchip/rockchip.rst | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/doc/board/rockchip/rockchip.rst b/doc/board/rockchip/rockchip.rst
index 4ca7b00b1f..a96ca0b582 100644
--- a/doc/board/rockchip/rockchip.rst
+++ b/doc/board/rockchip/rockchip.rst
@@ -172,8 +172,8 @@ Flashing
 SD Card
 ^^^^^^^
 
-All Rockchip platforms (except rk3128 which doesn't use SPL) are now
-supporting a single boot image using binman and pad_cat.
+All Rockchip platforms, (except rk3128 which doesn't use SPL) generate a
+SD/MMC full boot image using binman.
 
 To write an image that boots from a SD card (assumed to be /dev/sda):
 
@@ -224,31 +224,16 @@ is u-boot-dtb.img
 SPI
 ^^^
 
-The SPI boot method requires the generation of idbloader.img with help of the mkimage tool.
+Some platforms also generate a SPI flash full boot image, controlled by the
+CONFIG_ROCKCHIP_SPI_IMAGE configuration option.
 
-SPL-alone SPI boot image:
-
-.. code-block:: bash
-
-        ./tools/mkimage -n rk3399 -T rkspi -d spl/u-boot-spl.bin idbloader.img
-
-TPL+SPL SPI boot image:
-
-.. code-block:: bash
-
-        ./tools/mkimage -n rk3399 -T rkspi -d tpl/u-boot-tpl.bin:spl/u-boot-spl.bin idbloader.img
-
-Copy SPI boot images into SD card and boot from SD:
+This image can be copied onto an SD card and written to SPI flash:
 
 .. code-block:: bash
 
         sf probe
-        load mmc 1:1 $kernel_addr_r idbloader.img
-        sf erase 0 +$filesize
-        sf write $kernel_addr_r 0 ${filesize}
-        load mmc 1:1 ${kernel_addr_r} u-boot.itb
-        sf erase 0x60000 +$filesize
-        sf write $kernel_addr_r 0x60000 ${filesize}
+        load mmc 1:1 $kernel_addr_r u-boot-rockchip-spi.bin
+        sf update $kernel_addr_r 0 ${filesize}
 
 2. Package the image with Rockchip miniloader
 ---------------------------------------------
-- 
2.36.0

[RFC PATCH v2 8/8] board: rockpro64: Enable building SPI image

[Andrew Abbott]

Using the new SPI image generation, build an image for the Pine64
ROCKPro64 board.

Signed-off-by: Andrew Abbott <andrew@mirx.dev>
---

(no changes since v1)

 arch/arm/mach-rockchip/rk3399/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-rockchip/rk3399/Kconfig b/arch/arm/mach-rockchip/rk3399/Kconfig
index c1f251316c..4a049935df 100644
--- a/arch/arm/mach-rockchip/rk3399/Kconfig
+++ b/arch/arm/mach-rockchip/rk3399/Kconfig
@@ -84,6 +84,7 @@ config TARGET_ROCK960_RK3399
 
 config TARGET_ROCKPRO64_RK3399
 	bool "Pine64 Rockpro64 board"
+	select ROCKCHIP_SPI_IMAGE
 	help
 	  Rockro64 is SBC produced by Pine64. Key features:
 
-- 
2.36.0

Re: [RFC PATCH v2 0/8] Build Rockchip final images using binman

[Jerome Forissier]

Hi Andrew,

On 5/16/22 13:07, Andrew Abbott wrote:
> My original goal was to produce SPI images for Rockchip platforms (specifically
> for me, ROCKPro64, and in the future ROCK64). Looking into it, it seemed nicer
> to just switch the SD/MMC image generation over to binman as well in the
> process.
> 
> This is my attempt to move Rockchip final full image generation to binman,
> adding the option to make full SPI images as well.
> 
> Other questions:
> 
> - I noticed that ATF generation for ARM64 Rockchip is done via a Python script
>   instead of binman. I don't currently know how to change that over to binman,
>   but is that something worth pursuing as part of this?

I use this kind of configuration on ROCKPi 4B (U-Boot with TF-A BL31 and OP-TEE),
so I could at least run some tests. Please note that I recently sent a patch to
fix the OP-TEE support in make_fit_atf.py [1]. That would be needed in binman
too for me to able to test.

[1] https://lists.denx.de/pipermail/u-boot/2022-May/483917.html

> 
> Please give me your feedback!
> 
> Changes in v2:
> - Revert u-boot-rockchip-sdmmc.bin name to u-boot-rockchip.bin, to
>   keep the name the same as before.
> - Fix whitespace issues.
> - Remove note from docs about different offsets in SPI flash for
>   different SoCs - this was a bad assumption on my part, it doesn't work
>   this way.
> - Update name of SD/MMC image in the docs from u-boot-rockchip-sdmmc.bin
>   to u-boot-rockchip.bin.
> 
> Andrew Abbott (8):
>   binman: mkimage: Support ':'-separated inputs
>   rockchip: Add binman definitions for final images
>   soc: rockchip: Include common U-Boot dtsi file
>   board: rockchip: Move SPI U-Boot offset to config
>   rockchip: Remove obsolete Makefile targets
>   rockchip: Enable binman for ARM64
>   doc: rockchip: Update for new binman image generation
>   board: rockpro64: Enable building SPI image
> 
>  Kconfig                                      |  4 +-
>  Makefile                                     | 31 ++----------
>  arch/arm/Kconfig                             |  2 +-
>  arch/arm/dts/rk3308-u-boot.dtsi              |  2 +
>  arch/arm/dts/rk3328-u-boot.dtsi              |  2 +
>  arch/arm/dts/rk3368-lion-haikou-u-boot.dtsi  |  1 -
>  arch/arm/dts/rk3368-u-boot.dtsi              |  1 +
>  arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi |  4 --
>  arch/arm/dts/rk3399-puma-haikou-u-boot.dtsi  |  1 -
>  arch/arm/dts/rk3399-roc-pc-u-boot.dtsi       |  4 --
>  arch/arm/dts/rk3399-rockpro64-u-boot.dtsi    |  4 --
>  arch/arm/dts/rk3568-u-boot.dtsi              |  2 +
>  arch/arm/dts/rockchip-u-boot.dtsi            | 53 ++++++++++++++++++--
>  arch/arm/mach-rockchip/Kconfig               |  7 ++-
>  arch/arm/mach-rockchip/rk3399/Kconfig        |  1 +
>  configs/lion-rk3368_defconfig                |  1 +
>  configs/pinebook-pro-rk3399_defconfig        |  1 +
>  configs/puma-rk3399_defconfig                |  2 +-
>  configs/roc-pc-rk3399_defconfig              |  1 +
>  configs/rockpro64-rk3399_defconfig           |  1 +
>  doc/board/rockchip/rockchip.rst              | 29 +++--------
>  tools/binman/etype/mkimage.py                | 33 +++++++-----
>  22 files changed, 100 insertions(+), 87 deletions(-)
> 

Re: [RFC PATCH v2 0/8] Build Rockchip final images using binman

[Andrew Abbott]

Hi Jerome,

On Tue May 17, 2022 at 1:13 AM AEST, Jerome Forissier wrote:

> I use this kind of configuration on ROCKPi 4B (U-Boot with TF-A BL31 and OP-TEE),
> so I could at least run some tests. Please note that I recently sent a patch to
> fix the OP-TEE support in make_fit_atf.py [1]. That would be needed in binman
> too for me to able to test.
>
> [1] https://lists.denx.de/pipermail/u-boot/2022-May/483917.html

Thanks for your offer to test! I'm not actually very familiar with TF-A
and OP-TEE and the role they play, but I haven't changed how they are
generated/included into U-Boot in this patch set - I'm only using binman
to assemble SPL, TPL, and u-boot.itb into a single image, the latter
still being generated using make_fit_atf.py.

So unless I'm misunderstanding, you should be able to set
CONFIG_ROCKCHIP_SPI_IMAGE=y in the ROCKPi 4B defconfig and get a
u-boot-rockchip-spi.bin to be generated that would take your patch into
account as-is.

Thanks,
Andrew

Re: [RFC PATCH v2 0/8] Build Rockchip final images using binman

[Alper Nebi Yasak]

On 16/05/2022 14:07, Andrew Abbott wrote:
> My original goal was to produce SPI images for Rockchip platforms (specifically
> for me, ROCKPro64, and in the future ROCK64). Looking into it, it seemed nicer
> to just switch the SD/MMC image generation over to binman as well in the
> process.
> 
> This is my attempt to move Rockchip final full image generation to binman,
> adding the option to make full SPI images as well.
> 
> Other questions:
> 
> - I noticed that ATF generation for ARM64 Rockchip is done via a Python script
>   instead of binman. I don't currently know how to change that over to binman,
>   but is that something worth pursuing as part of this?

Simon was working on that in [1] (see patches 22-26 which weren't
applied), but images produced by that don't exactly work (see comments
on earlier versions [2]). Then, things got stalled/postponed because of
various issues in binman AFAICT.

The crux of the problem there is the binman-wise design of splitting
bl31.elf into parts and putting them into the FIT. I have some weird
ideas about generic mechanisms that would allow us to do it nicely, but
nothing I could flesh out.

Recently I've been thinking we might be able to sidestep it by putting
the unsplit ELF directly into FIT (as a new IH_TYPE_ELF for mkimage?)
and do the split-loading at runtime. I didn't have time to experiment on
that, though. I'd appreciate it if you tried, but don't worry about it
if you don't have the time.

[1] binman: rockchip: Migrate from rockchip SPL_FIT_GENERATOR script
https://lore.kernel.org/u-boot/20220306031917.3005215-1-sjg@chromium.org/

[2] rockchip: Support building the all output files in binman
https://lore.kernel.org/u-boot/CAPnjgZ37wnb4r7zkkBMfAeGDir147R4kxMwUWAE0nj6iSYdZBQ@mail.gmail.com/

> Please give me your feedback!
> 
> Changes in v2:
> - Revert u-boot-rockchip-sdmmc.bin name to u-boot-rockchip.bin, to
>   keep the name the same as before.
> - Fix whitespace issues.
> - Remove note from docs about different offsets in SPI flash for
>   different SoCs - this was a bad assumption on my part, it doesn't work
>   this way.
> - Update name of SD/MMC image in the docs from u-boot-rockchip-sdmmc.bin
>   to u-boot-rockchip.bin.
> 
> Andrew Abbott (8):
>   binman: mkimage: Support ':'-separated inputs
>   rockchip: Add binman definitions for final images
>   soc: rockchip: Include common U-Boot dtsi file
>   board: rockchip: Move SPI U-Boot offset to config
>   rockchip: Remove obsolete Makefile targets
>   rockchip: Enable binman for ARM64
>   doc: rockchip: Update for new binman image generation
>   board: rockpro64: Enable building SPI image
> 
>  Kconfig                                      |  4 +-
>  Makefile                                     | 31 ++----------
>  arch/arm/Kconfig                             |  2 +-
>  arch/arm/dts/rk3308-u-boot.dtsi              |  2 +
>  arch/arm/dts/rk3328-u-boot.dtsi              |  2 +
>  arch/arm/dts/rk3368-lion-haikou-u-boot.dtsi  |  1 -
>  arch/arm/dts/rk3368-u-boot.dtsi              |  1 +
>  arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi |  4 --
>  arch/arm/dts/rk3399-puma-haikou-u-boot.dtsi  |  1 -
>  arch/arm/dts/rk3399-roc-pc-u-boot.dtsi       |  4 --
>  arch/arm/dts/rk3399-rockpro64-u-boot.dtsi    |  4 --
>  arch/arm/dts/rk3568-u-boot.dtsi              |  2 +
>  arch/arm/dts/rockchip-u-boot.dtsi            | 53 ++++++++++++++++++--
>  arch/arm/mach-rockchip/Kconfig               |  7 ++-
>  arch/arm/mach-rockchip/rk3399/Kconfig        |  1 +
>  configs/lion-rk3368_defconfig                |  1 +
>  configs/pinebook-pro-rk3399_defconfig        |  1 +
>  configs/puma-rk3399_defconfig                |  2 +-
>  configs/roc-pc-rk3399_defconfig              |  1 +
>  configs/rockpro64-rk3399_defconfig           |  1 +
>  doc/board/rockchip/rockchip.rst              | 29 +++--------
>  tools/binman/etype/mkimage.py                | 33 +++++++-----
>  22 files changed, 100 insertions(+), 87 deletions(-)
> 

Re: [RFC PATCH v2 1/8] binman: mkimage: Support ':'-separated inputs

[Alper Nebi Yasak]

On 16/05/2022 14:07, Andrew Abbott wrote:
> mkimage supports combining multiple input binaries separating them
> with colons (':'). This is used at least for Rockchip platforms to
> encode payload offsets and sizes in the image header. It is required for
> Rockchip SPI boot since for the rkspi format, after the input binary
> combining, the entire image is spread across only the first 2K bytes of
> each 4K block.
> 
> Previous to this change, multiple inputs to a binman mkimage node would
> just be concatenated and the combined input would be passed as the -d
> argument to mkimage. Now, the inputs are instead passed colon-separated.
> 
> Signed-off-by: Andrew Abbott <andrew@mirx.dev>

Also see another attempt for this [1] and the comments to that for a
more complete picture, though I'll try writing all the points here anyway.

[1] binman: support mkimage separate files
https://lore.kernel.org/u-boot/20220304195641.1993688-1-pgwipeout@gmail.com/

> ---
> This is a bit of a messy implementation for now and would probably break
> existing uses of mkimage that rely on the concatenation behaviour.

I did a `git grep -C10 mkimage -- **/dts/*` and it doesn't look like
anything uses it yet. Except for binman/test/156_mkimage.dts, which
doesn't exactly test the concatenation.

> Questions:
> - Should this be a separate entry type, or an option to the mkimage
>   entry type that enables this behaviour?

You can add a 'separate-files' device-tree property like in [1]. I'm
actually OK with this separate-files being the default and only behavior
(concatenation can be done via an inner section), but I'm not sure Simon
would agree.

> - What kind of test(s) should I add?

At the minimum, a test using separate-files with multiple input entries.
You can do something like the _HandleGbbCommand in ftest.py to capture
and check the arguments that'll be passed to mkimage.

I think that'll be enough, but try to run `binman test -T` and check for
100% coverage with all tests succeeding.

> (no changes since v1)
> 
>  tools/binman/etype/mkimage.py | 33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
> index 5f6def2287..8cea618fbd 100644
> --- a/tools/binman/etype/mkimage.py
> +++ b/tools/binman/etype/mkimage.py
> @@ -51,21 +51,30 @@ class Entry_mkimage(Entry):

Expand the docstring with an explanation of the new behavior, and run
`binman entry-docs >tools/binman/entries.rst` to update it there as well.

>          self.ReadEntries()
>  
>      def ObtainContents(self):
> -        # Use a non-zero size for any fake files to keep mkimage happy
> -        data, input_fname, uniq = self.collect_contents_to_file(
> -            self._mkimage_entries.values(), 'mkimage', 1024)
> -        if data is None:
> -            return False
> -        output_fname = tools.get_output_filename('mkimage-out.%s' % uniq)
> -        if self.mkimage.run_cmd('-d', input_fname, *self._args,
> -                                output_fname) is not None:
> +        # For multiple inputs to mkimage, we want to separate them by colons.
> +        # This is needed for eg. the rkspi format, which treats the first data
> +        # file as the "init" and the second as "boot" and sets the image header
> +        # accordingly, then makes the image so that only the first 2 KiB of each
> +        # 4KiB block is used.
> +
> +        data_filenames = []
> +        for entry in self._mkimage_entries.values():
> +            # First get the input data and put it in a file. If any entry is not
> +            # available, try later.
> +            if not entry.ObtainContents():
> +                return False
> +
> +            input_fname = tools.get_output_filename('mkimage-in.%s' % entry.GetUniqueName())
> +            data_filenames.append(input_fname)
> +            tools.write_file(input_fname, entry.GetData())

Something like collect_contents_to_file([entry], f'mkimage-in-{idx}',
1024) would be better here. At least, the files must not be empty (or
mkimage exits with an error), where entry.GetData() can be b''.

> +
> +        output_fname = tools.get_output_filename('mkimage-out.%s' % self.GetUniqueName())

Should be an f-string.

> +        if self.mkimage.run_cmd('-d', ":".join(data_filenames), *self._args, output_fname):
>              self.SetContents(tools.read_file(output_fname))
> +            return True
>          else:
> -            # Bintool is missing; just use the input data as the output
>              self.record_missing_bintool(self.mkimage)
> -            self.SetContents(data)
> -
> -        return True
> +            return False

This case must set some dummy contents (I'd guess b'' is fine) and
return True. (False here roughly means "try again later".)

>  
>      def ReadEntries(self):
>          """Read the subnodes to find out what should go in this image"""

Re: [RFC PATCH v2 2/8] rockchip: Add binman definitions for final images

[Alper Nebi Yasak]

On 16/05/2022 14:07, Andrew Abbott wrote:
> Currently, building for Rockchip targets produces:
> 
> - idbloader.img
>     - rksd-formatted TPL with SPL appended; or
>     - rksd-formatted SPL
> - u-boot.itb
>     - U-Boot Proper FIT image
> - u-boot-rockchip.bin
>     - idbloader.img + u-boot.itb, padded the correct amount for SD/MMC
>       usage.
> 
> For RK3399 targets:
> 
> - u-boot.rom
>     - SPI image specific to the bob Chromebook target (see
>       c4cea2bbf995764f325a907061c22ecd6768cf7b).

I'm not sure we need anything special for the rk3399 chromebooks. I
think it's meant to be your 'u-boot-rockchip-spi.bin', but is just
incomplete (and partially broken, e.g. it should use u-boot.itb).

I'll try to test things on my chromebook_kevin later on, but probably
not before you can send a v3...

> This commit adds binman definitions to produce these images:
> 
> - idbloader.img
>     - rksd-formatted [TPL + ] SPL, as before.

Do we need the 'idbloader.img' as a build output, assuming we have a
working 'u-boot-rockchip.bin'? I'm asking because Simon was trying to
drop it in a similar patch [1].

[1] rockchip: Support building the all output files in binman
https://lore.kernel.org/u-boot/20220223230040.159317-24-sjg@chromium.org/

> - u-boot-rockchip.bin
>     - [TPL + ] SPL all rksd-formatted + u-boot.itb padded for SD/MMC
>       usage, as before.
> - u-boot-rockchip-spi.bin
>     - [TPL + ] SPL all rkspi-formatted + u-boot.itb padded for SPI
>       usage.
> 
> This commit also generalizes the CONFIG_ROCKCHIP_SPI_IMAGE config
> setting - it now means to generate a generic SPI flash image, in
> addition to the generic SD/MMC image.

With what I said above, I think you should rename this to 'u-boot.rom'
and remove the definitions in {rk3288,rk3399}-u-boot.dtsi.

> 
> Signed-off-by: Andrew Abbott <andrew@mirx.dev>
> ---
> Question: Does this break/not play nicely with rockchip-optee
> generation? It creates u-boot.itb for rk3288 targets. That would need to
> run before what I've implemented here?

The 'u-boot.itb' entries you add here are guarded with CONFIG_ARM64, so
don't apply to RK3288. I think that's fine until we can convert these
'u-boot.itb's to be completely binman-built as well.

OTOH, things appear work fine if I:

- Replace CONFIG_ARM64 with CONFIG_SPL_FIT below
- Include rockchip-optee before rockchip-u-boot in rk3288-u-boot.dtsi
- Move the empty binman node to rk3288-u-boot.dtsi above both includes

at least, until the patch that forces Makefile to build 'u-boot.itb'.

> 
> Changes in v2:
> - Revert u-boot-rockchip-sdmmc.bin name to u-boot-rockchip.bin, to
>   keep the name the same as before.
> - Fix whitespace issues.
> 
>  arch/arm/dts/rockchip-u-boot.dtsi | 53 +++++++++++++++++++++++++++++--
>  arch/arm/mach-rockchip/Kconfig    |  7 ++--
>  2 files changed, 53 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> index eae3ee715d..9354b0f5a7 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -13,17 +13,64 @@
>  
>  #ifdef CONFIG_SPL
>  &binman {
> -	simple-bin {
> +	sdmmc-idbloader {
> +		filename = "idbloader.img";
> +
> +		mkimage {
> +			args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
> +
> +#ifdef CONFIG_TPL
> +			u-boot-tpl {};
> +#endif
> +			u-boot-spl {};

The closing braces should be on their own line, more instances below as
well.

> +		};
> +	};
> +
> +	sdmmc-image {
>  		filename = "u-boot-rockchip.bin";
>  		pad-byte = <0xff>;
>  
> -		blob {
> +		idbloader {
>  			filename = "idbloader.img";
> +			type = "blob";
>  		};
>  
> -		u-boot-img {
> +#ifdef CONFIG_ARM64

Probably "#if defined(CONFIG_SPL_FIT) && defined(CONFIG_ARM64)" is
better, like in Simon's patch. Likewise in the SPI image below.

> +		u-boot-fit {
> +			filename = "u-boot.itb";
> +			type = "blob";
>  			offset = <CONFIG_SPL_PAD_TO>;
>  		};
> +#else
> +		u-boot-img {};

I think this still needs the "offset = <CONFIG_SPL_PAD_TO>;" which
shouldn't be removed.

> +#endif
>  	};
>  };
> +
> +#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
> +&binman {
> +	spi-image {
> +		filename = "u-boot-rockchip-spi.bin";
> +		pad-byte = <0xff>;
> +
> +		mkimage {
> +			args = "-n", CONFIG_SYS_SOC, "-T", "rkspi";
> +
> +#ifdef CONFIG_TPL
> +			u-boot-tpl {};
> +#endif
> +			u-boot-spl {};
> +		};
> +
> +#ifdef CONFIG_ARM64
> +		blob {

This should have the same name & type as the one in the other image.

> +			filename = "u-boot.itb";
> +			offset = <CONFIG_SYS_SPI_U_BOOT_OFFS>;
> +		};
> +#else
> +		u-boot-img {};

I think this also needs "offset = <CONFIG_SYS_SPI_U_BOOT_OFFS>;".

>  #endif
> +	};
> +};

Also consider adding a 'fdtmap' entry to both SPI and SD/MMC images,
it's useful for some interactive binman features.

> +#endif // CONFIG_ROCKCHIP_SPI_IMAGE
> +#endif // CONFIG_SPL
> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
> index 18aff5480b..7149b9a530 100644
> --- a/arch/arm/mach-rockchip/Kconfig
> +++ b/arch/arm/mach-rockchip/Kconfig
> @@ -415,12 +415,11 @@ config SPL_MMC
>  
>  config ROCKCHIP_SPI_IMAGE
>  	bool "Build a SPI image for rockchip"
> -	depends on HAS_ROM
>  	help
>  	  Some Rockchip SoCs support booting from SPI flash. Enable this
> -	  option to produce a 4MB SPI-flash image (called u-boot.rom)
> -	  containing U-Boot. The image is built by binman. U-Boot sits near
> -	  the start of the image.
> +	  option to produce an SPI-flash image (called u-boot-rockchip-spi.bin)
> +	  containing TPL (if enabled) and SPL, and U-Boot proper at the offset
> +	  CONFIG_SYS_SPI_U_BOOT_OFFS. The image is built by binman.

While renaming 'u-boot-rockchip-spi.bin' to 'u-boot.rom' you should drop
these Kconfig changes as well, and select HAS_ROM wherever you select
ROCKCHIP_SPI_IMAGE (i.e. in the last patch).

>  
>  config LNX_KRNL_IMG_TEXT_OFFSET_BASE
>  	default SYS_TEXT_BASE

Re: [RFC PATCH v2 3/8] soc: rockchip: Include common U-Boot dtsi file

[Alper Nebi Yasak]

On 16/05/2022 14:07, Andrew Abbott wrote:
> This pulls in binman definitions so images can be built for all
> Rockchip platforms.
> 
> Signed-off-by: Andrew Abbott <andrew@mirx.dev>
> ---

See Simon's patch [1] for this, which contains these changes and is more
complete. I think the 'imply BINMAN's there achieve the same thing as
your 'select BINMAN if SPL' in patch 6/8, but I'm not sure which is better.

[1] rockchip: Include binman script in 64-bit boards
https://lore.kernel.org/u-boot/20220306031917.3005215-24-sjg@chromium.org/

> 
> (no changes since v1)
> 
>  arch/arm/dts/rk3308-u-boot.dtsi | 2 ++
>  arch/arm/dts/rk3328-u-boot.dtsi | 2 ++
>  arch/arm/dts/rk3368-u-boot.dtsi | 1 +
>  arch/arm/dts/rk3568-u-boot.dtsi | 2 ++
>  4 files changed, 7 insertions(+)
> 
> diff --git a/arch/arm/dts/rk3308-u-boot.dtsi b/arch/arm/dts/rk3308-u-boot.dtsi
> index 4bfad31fba..ab5bfc2ce9 100644
> --- a/arch/arm/dts/rk3308-u-boot.dtsi
> +++ b/arch/arm/dts/rk3308-u-boot.dtsi
> @@ -3,6 +3,8 @@
>   *(C) Copyright 2019 Rockchip Electronics Co., Ltd
>   */
>  
> +#include "rockchip-u-boot.dtsi"
> +
>  / {
>  	aliases {
>  		mmc0 = &emmc;
> diff --git a/arch/arm/dts/rk3328-u-boot.dtsi b/arch/arm/dts/rk3328-u-boot.dtsi
> index 1633558264..d4a7540a92 100644
> --- a/arch/arm/dts/rk3328-u-boot.dtsi
> +++ b/arch/arm/dts/rk3328-u-boot.dtsi
> @@ -3,6 +3,8 @@
>   * (C) Copyright 2019 Rockchip Electronics Co., Ltd
>   */
>  
> +#include "rockchip-u-boot.dtsi"
> +
>  / {
>  	aliases {
>  		mmc0 = &emmc;
> diff --git a/arch/arm/dts/rk3368-u-boot.dtsi b/arch/arm/dts/rk3368-u-boot.dtsi
> index 2767c2678d..b37da4e851 100644
> --- a/arch/arm/dts/rk3368-u-boot.dtsi
> +++ b/arch/arm/dts/rk3368-u-boot.dtsi
> @@ -3,6 +3,7 @@
>   * Copyright (c) 2020 Theobroma Systems Design und Consulting GmbH
>   */
>  
> +#include "rockchip-u-boot.dtsi"
>  #include <dt-bindings/memory/rk3368-dmc.h>
>  
>  / {
> diff --git a/arch/arm/dts/rk3568-u-boot.dtsi b/arch/arm/dts/rk3568-u-boot.dtsi
> index 5a80dda275..fa9b6ae23b 100644
> --- a/arch/arm/dts/rk3568-u-boot.dtsi
> +++ b/arch/arm/dts/rk3568-u-boot.dtsi
> @@ -3,6 +3,8 @@
>   * (C) Copyright 2021 Rockchip Electronics Co., Ltd
>   */
>  
> +#include "rockchip-u-boot.dtsi"
> +
>  / {
>  	aliases {
>  		mmc0 = &sdhci;

Re: [RFC PATCH v2 4/8] board: rockchip: Move SPI U-Boot offset to config

[Alper Nebi Yasak]

On 16/05/2022 14:07, Andrew Abbott wrote:
> This needs to be accessible to binman (via CONFIG_ preprocessor macros)
> so it can build SPI images using the correct offset.
> 
> The documentation at 'doc/device-tree-bindings/config.txt' says that
> the 'u-boot,spl-payload-offset' device tree option simply overrides
> 'CONFIG_SYS_SPI_U_BOOT_OFFS', so this change should not functionally
> change the offset for any of the affected boards.
> 
> Signed-off-by: Andrew Abbott <andrew@mirx.dev>
> ---

This sounds reasonable, but I don't really understand how people started
using the device-tree property instead of changing the config value, so
can't exactly say which one should be preferred.

> 
> (no changes since v1)
> 
>  arch/arm/dts/rk3368-lion-haikou-u-boot.dtsi  | 1 -
>  arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi | 4 ----
>  arch/arm/dts/rk3399-puma-haikou-u-boot.dtsi  | 1 -
>  arch/arm/dts/rk3399-roc-pc-u-boot.dtsi       | 4 ----
>  arch/arm/dts/rk3399-rockpro64-u-boot.dtsi    | 4 ----

I guess this can be done for rk3399-gru-u-boot.dtsi as well, because I
did cargo-cult it there from a previous commit...

>  configs/lion-rk3368_defconfig                | 1 +
>  configs/pinebook-pro-rk3399_defconfig        | 1 +
>  configs/puma-rk3399_defconfig                | 2 +-
>  configs/roc-pc-rk3399_defconfig              | 1 +
>  configs/rockpro64-rk3399_defconfig           | 1 +
>  10 files changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/dts/rk3368-lion-haikou-u-boot.dtsi b/arch/arm/dts/rk3368-lion-haikou-u-boot.dtsi
> index 7826d1e70b..6840182f03 100644
> --- a/arch/arm/dts/rk3368-lion-haikou-u-boot.dtsi
> +++ b/arch/arm/dts/rk3368-lion-haikou-u-boot.dtsi
> @@ -7,7 +7,6 @@
>  
>  / {
>  	config {
> -		u-boot,spl-payload-offset = <0x40000>; /* @ 256KB */
>  		u-boot,mmc-env-offset = <0x4000>;      /* @  16KB */
>  	};
>  
> diff --git a/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi b/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
> index 2d87bea933..d1d0ac460d 100644
> --- a/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
> +++ b/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
> @@ -10,10 +10,6 @@
>  	chosen {
>  		u-boot,spl-boot-order = "same-as-spl", &sdhci, &spiflash, &sdmmc;
>  	};
> -
> -	config {
> -		u-boot,spl-payload-offset = <0x60000>; /* @ 384KB */
> -	};
>  };
>  
>  &edp {
> diff --git a/arch/arm/dts/rk3399-puma-haikou-u-boot.dtsi b/arch/arm/dts/rk3399-puma-haikou-u-boot.dtsi
> index e0476ab25c..f3f8619716 100644
> --- a/arch/arm/dts/rk3399-puma-haikou-u-boot.dtsi
> +++ b/arch/arm/dts/rk3399-puma-haikou-u-boot.dtsi
> @@ -14,7 +14,6 @@
>  
>  / {
>  	config {
> -		u-boot,spl-payload-offset = <0x40000>; /* @ 256KB */
>  		u-boot,mmc-env-offset = <0x4000>;      /* @  16KB */
>  		u-boot,efi-partition-entries-offset = <0x200000>; /* 2MB */
>  		u-boot,boot-led = "module_led";
> diff --git a/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi b/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi
> index e3c9364e35..a54e554d8a 100644
> --- a/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi
> +++ b/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi
> @@ -11,10 +11,6 @@
>  		u-boot,spl-boot-order = "same-as-spl", &spi_flash, &sdhci, &sdmmc;
>  	};
>  
> -	config {
> -		u-boot,spl-payload-offset = <0x60000>; /* @ 384KB */
> -	};
> -
>  	vcc_hub_en: vcc_hub_en-regulator {
>  		compatible = "regulator-fixed";
>  		enable-active-high;
> diff --git a/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi b/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
> index 37dff04adf..bd65496df3 100644
> --- a/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
> +++ b/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
> @@ -9,10 +9,6 @@
>  	chosen {
>  		u-boot,spl-boot-order = "same-as-spl", &spi_flash, &sdmmc, &sdhci;
>  	};
> -
> -	config {
> -		u-boot,spl-payload-offset = <0x60000>; /* @ 384KB */
> -	};
>  };
>  
>  &spi1 {
> diff --git a/configs/lion-rk3368_defconfig b/configs/lion-rk3368_defconfig
> index 426913816b..91630c822b 100644
> --- a/configs/lion-rk3368_defconfig
> +++ b/configs/lion-rk3368_defconfig
> @@ -17,6 +17,7 @@ CONFIG_DEBUG_UART_BASE=0xFF180000
>  CONFIG_DEBUG_UART_CLOCK=24000000
>  CONFIG_SPL_SPI_FLASH_SUPPORT=y
>  CONFIG_SPL_SPI=y
> +CONFIG_SYS_SPI_U_BOOT_OFFS=0x40000
>  CONFIG_SYS_LOAD_ADDR=0x800800
>  CONFIG_DEBUG_UART=y
>  CONFIG_FIT=y
> diff --git a/configs/pinebook-pro-rk3399_defconfig b/configs/pinebook-pro-rk3399_defconfig
> index 8ca1d0708f..c5ebf62f02 100644
> --- a/configs/pinebook-pro-rk3399_defconfig
> +++ b/configs/pinebook-pro-rk3399_defconfig
> @@ -24,6 +24,7 @@ CONFIG_SPL_STACK_R=y
>  CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
>  CONFIG_SPL_MTD_SUPPORT=y
>  CONFIG_SPL_SPI_LOAD=y
> +CONFIG_SYS_SPI_U_BOOT_OFFS=0x60000
>  CONFIG_TPL=y
>  CONFIG_CMD_BOOTZ=y
>  CONFIG_CMD_GPIO=y
> diff --git a/configs/puma-rk3399_defconfig b/configs/puma-rk3399_defconfig
> index 7ce2dc0719..6b7898be49 100644
> --- a/configs/puma-rk3399_defconfig
> +++ b/configs/puma-rk3399_defconfig
> @@ -27,7 +27,7 @@ CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR=0x200
>  CONFIG_SPL_I2C=y
>  CONFIG_SPL_POWER=y
>  CONFIG_SPL_SPI_LOAD=y
> -CONFIG_SYS_SPI_U_BOOT_OFFS=0x20000
> +CONFIG_SYS_SPI_U_BOOT_OFFS=0x40000
>  CONFIG_CMD_BOOTZ=y
>  CONFIG_CMD_GPT=y
>  CONFIG_CMD_I2C=y
> diff --git a/configs/roc-pc-rk3399_defconfig b/configs/roc-pc-rk3399_defconfig
> index 4684fa6e74..2d7e2ad563 100644
> --- a/configs/roc-pc-rk3399_defconfig
> +++ b/configs/roc-pc-rk3399_defconfig
> @@ -25,6 +25,7 @@ CONFIG_SPL_STACK_R=y
>  CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x20000
>  CONFIG_SPL_ENV_SUPPORT=y
>  CONFIG_SPL_SPI_LOAD=y
> +CONFIG_SYS_SPI_U_BOOT_OFFS=0x60000
>  CONFIG_TPL=y
>  CONFIG_CMD_BOOTZ=y
>  CONFIG_CMD_GPT=y
> diff --git a/configs/rockpro64-rk3399_defconfig b/configs/rockpro64-rk3399_defconfig
> index e6f7a8469a..8c45a6c575 100644
> --- a/configs/rockpro64-rk3399_defconfig
> +++ b/configs/rockpro64-rk3399_defconfig
> @@ -23,6 +23,7 @@ CONFIG_MISC_INIT_R=y
>  CONFIG_SPL_STACK_R=y
>  CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
>  CONFIG_SPL_SPI_LOAD=y
> +CONFIG_SYS_SPI_U_BOOT_OFFS=0x60000
>  CONFIG_TPL=y
>  CONFIG_CMD_BOOTZ=y
>  CONFIG_CMD_GPT=y

Re: [RFC PATCH v2 6/8] rockchip: Enable binman for ARM64

[Alper Nebi Yasak]

On 16/05/2022 14:07, Andrew Abbott wrote:
> Binman is now being used to build the final flashable images for
> Rockchip devices, thus enabling it for all Rockchip targets here. But
> it is not yet being used to generate the FIT image (u-boot.itb),
> thus we need to force it to be built.
> 
> Signed-off-by: Andrew Abbott <andrew@mirx.dev>
> ---
> Question: Will this causes issues with eg. Chromebook gru/bob, which build
> u-boot.itb with binman already?

They don't build u-boot.itb with binman. I don't think there would be a
issue with them, but didn't actually test (will test later as I said).

> (no changes since v1)
> 
>  Kconfig          | 4 ++--
>  arch/arm/Kconfig | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/Kconfig b/Kconfig
> index 797038b037..7226986830 100644
> --- a/Kconfig
> +++ b/Kconfig
> @@ -414,8 +414,8 @@ config BUILD_TARGET
>  	default "u-boot-with-spl.sfp" if TARGET_SOCFPGA_GEN5
>  	default "u-boot-spl.kwb" if ARCH_MVEBU && SPL
>  	default "u-boot-elf.srec" if RCAR_GEN3
> -	default "u-boot.itb" if !BINMAN && SPL_LOAD_FIT && (ARCH_ROCKCHIP || \
> -				ARCH_SUNXI || RISCV || ARCH_ZYNQMP)
> +	default "u-boot.itb" if ARCH_ROCKCHIP || (!BINMAN && SPL_LOAD_FIT && \
> +				(ARCH_SUNXI || RISCV || ARCH_ZYNQMP))

I can't see how this part is necessary, can you give a concrete example?

It also makes evb-rk3288, chromebook_jerry, chromebook_speedy,
evb-rk3036 fail to build (maybe more?).

>  	default "u-boot.kwb" if ARCH_KIRKWOOD
>  	default "u-boot-with-spl.bin" if ARCH_AT91 && SPL_NAND_SUPPORT
>  	default "u-boot-with-spl.imx" if ARCH_MX6 && SPL
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 0afec5155b..545bf9a8cc 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1967,7 +1967,7 @@ config ARCH_STM32MP
>  config ARCH_ROCKCHIP
>  	bool "Support Rockchip SoCs"
>  	select BLK
> -	select BINMAN if SPL_OPTEE || (SPL && !ARM64)
> +	select BINMAN if SPL
>  	select DM
>  	select DM_GPIO
>  	select DM_I2C

Re: [RFC PATCH v2 0/8] Build Rockchip final images using binman

[Andrew Abbott]

On Thu May 19, 2022 at 9:35 PM AEST, Alper Nebi Yasak wrote:

> > - I noticed that ATF generation for ARM64 Rockchip is done via a Python script
> >   instead of binman. I don't currently know how to change that over to binman,
> >   but is that something worth pursuing as part of this?
>
> Simon was working on that in [1] (see patches 22-26 which weren't
> applied), but images produced by that don't exactly work (see comments
> on earlier versions [2]). Then, things got stalled/postponed because of
> various issues in binman AFAICT.
>
> The crux of the problem there is the binman-wise design of splitting
> bl31.elf into parts and putting them into the FIT. I have some weird
> ideas about generic mechanisms that would allow us to do it nicely, but
> nothing I could flesh out.
>
> Recently I've been thinking we might be able to sidestep it by putting
> the unsplit ELF directly into FIT (as a new IH_TYPE_ELF for mkimage?)
> and do the split-loading at runtime. I didn't have time to experiment on
> that, though. I'd appreciate it if you tried, but don't worry about it
> if you don't have the time.
>
> [1] binman: rockchip: Migrate from rockchip SPL_FIT_GENERATOR script
> https://lore.kernel.org/u-boot/20220306031917.3005215-1-sjg@chromium.org/
>
> [2] rockchip: Support building the all output files in binman
> https://lore.kernel.org/u-boot/CAPnjgZ37wnb4r7zkkBMfAeGDir147R4kxMwUWAE0nj6iSYdZBQ@mail.gmail.com/

Thanks for the information and links to previous attempts.

Unfortunately, I don't think I have either the knowledge or the time to
be look into the FIT image generation to that extent in the near future
(which is why I didn't try to tackle it in this patch set).

Re: [RFC PATCH v2 1/8] binman: mkimage: Support ':'-separated inputs

[Andrew Abbott]

On Thu May 19, 2022 at 9:36 PM AEST, Alper Nebi Yasak wrote:
> Also see another attempt for this [1] and the comments to that for a
> more complete picture, though I'll try writing all the points here anyway.
>
> [1] binman: support mkimage separate files
> https://lore.kernel.org/u-boot/20220304195641.1993688-1-pgwipeout@gmail.com/
>
> > ---
> > This is a bit of a messy implementation for now and would probably break
> > existing uses of mkimage that rely on the concatenation behaviour.
>
> I did a `git grep -C10 mkimage -- **/dts/*` and it doesn't look like
> anything uses it yet. Except for binman/test/156_mkimage.dts, which
> doesn't exactly test the concatenation.

I did similar and came to the same conclusion, but I figured I'd ask
just in case it's used somewhere I hadn't found, or it's being relied
on outside of the U-Boot repository.

> You can add a 'separate-files' device-tree property like in [1]. I'm
> actually OK with this separate-files being the default and only behavior
> (concatenation can be done via an inner section), but I'm not sure Simon
> would agree.

Similar thoughts here, especially since we couldn't find anything
relying on the concatenation behaviour.

> > - What kind of test(s) should I add?
>
> At the minimum, a test using separate-files with multiple input entries.
> You can do something like the _HandleGbbCommand in ftest.py to capture
> and check the arguments that'll be passed to mkimage.
>
> I think that'll be enough, but try to run `binman test -T` and check for
> 100% coverage with all tests succeeding.
>
> > (no changes since v1)
> >
> >  tools/binman/etype/mkimage.py | 33 +++++++++++++++++++++------------
> >  1 file changed, 21 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
> > index 5f6def2287..8cea618fbd 100644
> > --- a/tools/binman/etype/mkimage.py
> > +++ b/tools/binman/etype/mkimage.py
> > @@ -51,21 +51,30 @@ class Entry_mkimage(Entry):
>
> Expand the docstring with an explanation of the new behavior, and run
> `binman entry-docs >tools/binman/entries.rst` to update it there as well.
>
> >          self.ReadEntries()
> >
> >      def ObtainContents(self):
> > -        # Use a non-zero size for any fake files to keep mkimage happy
> > -        data, input_fname, uniq = self.collect_contents_to_file(
> > -            self._mkimage_entries.values(), 'mkimage', 1024)
> > -        if data is None:
> > -            return False
> > -        output_fname = tools.get_output_filename('mkimage-out.%s' % uniq)
> > -        if self.mkimage.run_cmd('-d', input_fname, *self._args,
> > -                                output_fname) is not None:
> > +        # For multiple inputs to mkimage, we want to separate them by colons.
> > +        # This is needed for eg. the rkspi format, which treats the first data
> > +        # file as the "init" and the second as "boot" and sets the image header
> > +        # accordingly, then makes the image so that only the first 2 KiB of each
> > +        # 4KiB block is used.
> > +
> > +        data_filenames = []
> > +        for entry in self._mkimage_entries.values():
> > +            # First get the input data and put it in a file. If any entry is not
> > +            # available, try later.
> > +            if not entry.ObtainContents():
> > +                return False
> > +
> > +            input_fname = tools.get_output_filename('mkimage-in.%s' % entry.GetUniqueName())
> > +            data_filenames.append(input_fname)
> > +            tools.write_file(input_fname, entry.GetData())
>
> Something like collect_contents_to_file([entry], f'mkimage-in-{idx}',
> 1024) would be better here. At least, the files must not be empty (or
> mkimage exits with an error), where entry.GetData() can be b''.
>
> > +
> > +        output_fname = tools.get_output_filename('mkimage-out.%s' % self.GetUniqueName())
>
> Should be an f-string.
>
> > +        if self.mkimage.run_cmd('-d', ":".join(data_filenames), *self._args, output_fname):
> >              self.SetContents(tools.read_file(output_fname))
> > +            return True
> >          else:
> > -            # Bintool is missing; just use the input data as the output
> >              self.record_missing_bintool(self.mkimage)
> > -            self.SetContents(data)
> > -
> > -        return True
> > +            return False
>
> This case must set some dummy contents (I'd guess b'' is fine) and
> return True. (False here roughly means "try again later".)
>
> >
> >      def ReadEntries(self):
> >          """Read the subnodes to find out what should go in this image"""

Thanks for your review! I'll make these updates in the next version.

Re: [RFC PATCH v2 2/8] rockchip: Add binman definitions for final images

[Andrew Abbott]

On Thu May 19, 2022 at 9:36 PM AEST, Alper Nebi Yasak wrote:

> > For RK3399 targets:
> >
> > - u-boot.rom
> >     - SPI image specific to the bob Chromebook target (see
> >       c4cea2bbf995764f325a907061c22ecd6768cf7b).
>
> I'm not sure we need anything special for the rk3399 chromebooks. I
> think it's meant to be your 'u-boot-rockchip-spi.bin', but is just
> incomplete (and partially broken, e.g. it should use u-boot.itb).
>
> I'll try to test things on my chromebook_kevin later on, but probably
> not before you can send a v3...
>
> > This commit adds binman definitions to produce these images:
> >
> > - idbloader.img
> >     - rksd-formatted [TPL + ] SPL, as before.
>
> Do we need the 'idbloader.img' as a build output, assuming we have a
> working 'u-boot-rockchip.bin'? I'm asking because Simon was trying to
> drop it in a similar patch [1].

I was keeping it for backwards compatibility, mainly because it's
mentioned in 'rockchip.rst' and it implies that 'idbloader.img' goes
on a separate partition to 'u-boot.itb' for targets supporting Fastboot.
If we can drop it, then I'll gladly do so!

> [1] rockchip: Support building the all output files in binman
> https://lore.kernel.org/u-boot/20220223230040.159317-24-sjg@chromium.org/
>
> > - u-boot-rockchip.bin
> >     - [TPL + ] SPL all rksd-formatted + u-boot.itb padded for SD/MMC
> >       usage, as before.
> > - u-boot-rockchip-spi.bin
> >     - [TPL + ] SPL all rkspi-formatted + u-boot.itb padded for SPI
> >       usage.
> >
> > This commit also generalizes the CONFIG_ROCKCHIP_SPI_IMAGE config
> > setting - it now means to generate a generic SPI flash image, in
> > addition to the generic SD/MMC image.
>
> With what I said above, I think you should rename this to 'u-boot.rom'
> and remove the definitions in {rk3288,rk3399}-u-boot.dtsi.

Makes sense to me - I just wonder if the name 'u-boot.rom' is too
generic, since it will be an image specifically for Rockchip targets.
Then again, perhaps the original 'u-boot-rockchip.bin' name was
redundant, since you know what target you're building for by using a
specific defconfig in the first place.

> > Question: Does this break/not play nicely with rockchip-optee
> > generation? It creates u-boot.itb for rk3288 targets. That would need to
> > run before what I've implemented here?
>
> The 'u-boot.itb' entries you add here are guarded with CONFIG_ARM64, so
> don't apply to RK3288. I think that's fine until we can convert these
> 'u-boot.itb's to be completely binman-built as well.

Oh, good point.

> OTOH, things appear work fine if I:
>
> - Replace CONFIG_ARM64 with CONFIG_SPL_FIT below
> - Include rockchip-optee before rockchip-u-boot in rk3288-u-boot.dtsi
> - Move the empty binman node to rk3288-u-boot.dtsi above both includes
>
> at least, until the patch that forces Makefile to build 'u-boot.itb'.

In that case, I'll just leave it as is for now.

> > +#ifdef CONFIG_TPL
> > +			u-boot-tpl {};
> > +#endif
> > +			u-boot-spl {};
>
> The closing braces should be on their own line, more instances below as
> well.

> > -		u-boot-img {
> > +#ifdef CONFIG_ARM64
>
> Probably "#if defined(CONFIG_SPL_FIT) && defined(CONFIG_ARM64)" is
> better, like in Simon's patch. Likewise in the SPI image below.

> > +		u-boot-fit {
> > +			filename = "u-boot.itb";
> > +			type = "blob";
> >  			offset = <CONFIG_SPL_PAD_TO>;
> >  		};
> > +#else
> > +		u-boot-img {};
>
> I think this still needs the "offset = <CONFIG_SPL_PAD_TO>;" which
> shouldn't be removed.

> > +#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
> > +&binman {
> > +	spi-image {
> > +		filename = "u-boot-rockchip-spi.bin";
> > +		pad-byte = <0xff>;
> > +
> > +		mkimage {
> > +			args = "-n", CONFIG_SYS_SOC, "-T", "rkspi";
> > +
> > +#ifdef CONFIG_TPL
> > +			u-boot-tpl {};
> > +#endif
> > +			u-boot-spl {};
> > +		};
> > +
> > +#ifdef CONFIG_ARM64
> > +		blob {
>
> This should have the same name & type as the one in the other image.
>
> > +			filename = "u-boot.itb";
> > +			offset = <CONFIG_SYS_SPI_U_BOOT_OFFS>;
> > +		};
> > +#else
> > +		u-boot-img {};
>
> I think this also needs "offset = <CONFIG_SYS_SPI_U_BOOT_OFFS>;".
>
> >  #endif
> > +	};
> > +};

> Also consider adding a 'fdtmap' entry to both SPI and SD/MMC images,
> it's useful for some interactive binman features.

> > diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
> > index 18aff5480b..7149b9a530 100644
> > --- a/arch/arm/mach-rockchip/Kconfig
> > +++ b/arch/arm/mach-rockchip/Kconfig
> > @@ -415,12 +415,11 @@ config SPL_MMC
> >
> >  config ROCKCHIP_SPI_IMAGE
> >  	bool "Build a SPI image for rockchip"
> > -	depends on HAS_ROM
> >  	help
> >  	  Some Rockchip SoCs support booting from SPI flash. Enable this
> > -	  option to produce a 4MB SPI-flash image (called u-boot.rom)
> > -	  containing U-Boot. The image is built by binman. U-Boot sits near
> > -	  the start of the image.
> > +	  option to produce an SPI-flash image (called u-boot-rockchip-spi.bin)
> > +	  containing TPL (if enabled) and SPL, and U-Boot proper at the offset
> > +	  CONFIG_SYS_SPI_U_BOOT_OFFS. The image is built by binman.
>
> While renaming 'u-boot-rockchip-spi.bin' to 'u-boot.rom' you should drop
> these Kconfig changes as well, and select HAS_ROM wherever you select
> ROCKCHIP_SPI_IMAGE (i.e. in the last patch).

Thanks for your comments, I'll make the changes for the next version.

Re: [RFC PATCH v2 6/8] rockchip: Enable binman for ARM64

[Andrew Abbott]

On Thu May 19, 2022 at 9:37 PM AEST, Alper Nebi Yasak wrote:
> On 16/05/2022 14:07, Andrew Abbott wrote:
> > Binman is now being used to build the final flashable images for
> > Rockchip devices, thus enabling it for all Rockchip targets here. But
> > it is not yet being used to generate the FIT image (u-boot.itb),
> > thus we need to force it to be built.
> >
> > Signed-off-by: Andrew Abbott <andrew@mirx.dev>
> > ---
> > Question: Will this causes issues with eg. Chromebook gru/bob, which build
> > u-boot.itb with binman already?
>
> They don't build u-boot.itb with binman. I don't think there would be a
> issue with them, but didn't actually test (will test later as I said).

I think I got confused with the 'rockchip-optee.dtsi` stuff. With your
comment on 2/8 on how it's not built when selecting CONFIG_ARM64, it's
probably not a problem.

> > (no changes since v1)
> >
> >  Kconfig          | 4 ++--
> >  arch/arm/Kconfig | 2 +-
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/Kconfig b/Kconfig
> > index 797038b037..7226986830 100644
> > --- a/Kconfig
> > +++ b/Kconfig
> > @@ -414,8 +414,8 @@ config BUILD_TARGET
> >  	default "u-boot-with-spl.sfp" if TARGET_SOCFPGA_GEN5
> >  	default "u-boot-spl.kwb" if ARCH_MVEBU && SPL
> >  	default "u-boot-elf.srec" if RCAR_GEN3
> > -	default "u-boot.itb" if !BINMAN && SPL_LOAD_FIT && (ARCH_ROCKCHIP || \
> > -				ARCH_SUNXI || RISCV || ARCH_ZYNQMP)
> > +	default "u-boot.itb" if ARCH_ROCKCHIP || (!BINMAN && SPL_LOAD_FIT && \
> > +				(ARCH_SUNXI || RISCV || ARCH_ZYNQMP))
>
> I can't see how this part is necessary, can you give a concrete example?
>
> It also makes evb-rk3288, chromebook_jerry, chromebook_speedy,
> evb-rk3036 fail to build (maybe more?).

I thought the original condition meant that 'u-boot.itb' wouldn't be
built (via 'make_atf_fit.py') if CONFIG_BINMAN was selected. So I
selected CONFIG_BINMAN below, then updated this condition to try and
force 'u-boot.itb' generation.

I tried reverting just that change and it seems to still build
correctly, so I will leave it as it was for the next version.

> >  	default "u-boot.kwb" if ARCH_KIRKWOOD
> >  	default "u-boot-with-spl.bin" if ARCH_AT91 && SPL_NAND_SUPPORT
> >  	default "u-boot-with-spl.imx" if ARCH_MX6 && SPL
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index 0afec5155b..545bf9a8cc 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -1967,7 +1967,7 @@ config ARCH_STM32MP
> >  config ARCH_ROCKCHIP
> >  	bool "Support Rockchip SoCs"
> >  	select BLK
> > -	select BINMAN if SPL_OPTEE || (SPL && !ARM64)
> > +	select BINMAN if SPL
> >  	select DM
> >  	select DM_GPIO
> >  	select DM_I2C

Re: [RFC PATCH v2 2/8] rockchip: Add binman definitions for final images

[Alper Nebi Yasak]

On 22/05/2022 03:55, Andrew Abbott wrote:
> On Thu May 19, 2022 at 9:36 PM AEST, Alper Nebi Yasak wrote:
>> Do we need the 'idbloader.img' as a build output, assuming we have a
>> working 'u-boot-rockchip.bin'? I'm asking because Simon was trying to
>> drop it in a similar patch [1].
> 
> I was keeping it for backwards compatibility, mainly because it's
> mentioned in 'rockchip.rst' and it implies that 'idbloader.img' goes
> on a separate partition to 'u-boot.itb' for targets supporting Fastboot.
> If we can drop it, then I'll gladly do so!

Honestly, I don't know. I was hoping someone else would comment as well.
I'm inclined to say we don't need it, as we would ideally be able to
extract/replace the 'idbloader.img' from/in working images with binman
commands when needed.

>> With what I said above, I think you should rename this to 'u-boot.rom'
>> and remove the definitions in {rk3288,rk3399}-u-boot.dtsi.
> 
> Makes sense to me - I just wonder if the name 'u-boot.rom' is too
> generic, since it will be an image specifically for Rockchip targets.
> Then again, perhaps the original 'u-boot-rockchip.bin' name was
> redundant, since you know what target you're building for by using a
> specific defconfig in the first place.

I think it's meant to be a step towards unifying the build artifacts and
names across the board, which I'd like if it eventually happened. We
would have different definitions of the 'u-boot.rom' image for different
whatevers, but for every board "Build and write u-boot.rom to SPI flash
if it exists" would be valid advice.