[PATCH V4 0/8] arm64: binman: use binman symbols for imx

[PATCH V4 0/8] arm64: binman: use binman symbols for imx

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

V4:
 Fix three boards build failure

V3:
 Add R-b/T-b
 Fix build warning

V2:
 resolve some CI failure
 include patch 7

binman symbol is a good feature, but only used on X86 for now. This patchset
is to use it for i.MX8M platform.

The current imx8m ddr phy firmware consumes lots of space, because we pad
them to the largest 32KB and 16KB for IMEM and DMEM.

With this patchset we use binman symbols to get firmware location and size,
we could save near 36KB with i.MX8MP-EVK.

Please help check and test


Peng Fan (8):
  spl: guard u_boot_any with X86
  arm: dts: imx8m: update binman ddr firmware node name
  imx: imx8mm-icore: migrate to use BINMAN
  armv8: u-boot-spl.lds: mark __image_copy_start as symbol
  tools: binman: section: replace @ with -
  ddr: imx8m: helper: load ddr firmware according to binman symbols
  arm: dts: imx8m: shrink ddr firmware size to actual file size
  binman_sym: guard with CONFIG_SPL_BINMAN_SYMBOLS

 arch/arm/cpu/armv8/u-boot-spl.lds             |  2 +-
 arch/arm/dts/imx8mm-u-boot.dtsi               | 16 +++---
 arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi    |  8 +--
 .../dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi  |  4 +-
 arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi      |  8 +--
 arch/arm/dts/imx8mn-evk-u-boot.dtsi           |  8 +--
 .../dts/imx8mn-var-som-symphony-u-boot.dtsi   | 16 +++---
 arch/arm/dts/imx8mn-venice-u-boot.dtsi        | 16 +++---
 arch/arm/dts/imx8mp-u-boot.dtsi               |  8 +--
 arch/arm/dts/imx8mq-cm-u-boot.dtsi            |  8 +--
 arch/arm/dts/imx8mq-u-boot.dtsi               | 16 +++---
 arch/arm/mach-imx/imx8m/Kconfig               |  1 +
 .../mach-imx/imx8m/imximage-8mm-lpddr4.cfg    | 10 +---
 common/spl/spl.c                              |  8 ++-
 common/spl/spl_ram.c                          |  4 ++
 configs/imx8mm-icore-mx8mm-ctouch2_defconfig  |  2 +-
 configs/imx8mm-icore-mx8mm-edimm2.2_defconfig |  2 +-
 drivers/ddr/imx/imx8m/helper.c                | 51 ++++++++++++++++---
 include/binman_sym.h                          |  2 +-
 tools/binman/etype/section.py                 |  2 +-
 tools/binman/test/u_boot_binman_syms.c        |  1 +
 tools/binman/test/u_boot_binman_syms_size.c   |  1 +
 22 files changed, 116 insertions(+), 78 deletions(-)

-- 
2.36.0

[PATCH V4 1/8] spl: guard u_boot_any with X86

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

set the symbol as weak not work if LTO is enabled. Since u_boot_any is
only used on X86 for now, so guard it with X86, otherwise build break
if we use BINMAN_SYMBOLS on i.MX.

Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 common/spl/spl.c     | 8 ++++++--
 common/spl/spl_ram.c | 4 ++++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/common/spl/spl.c b/common/spl/spl.c
index c8c463f80bd..4b28180467a 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -50,7 +50,7 @@ DECLARE_GLOBAL_DATA_PTR;
 
 u32 *boot_params_ptr = NULL;
 
-#if CONFIG_IS_ENABLED(BINMAN_SYMBOLS)
+#if CONFIG_IS_ENABLED(BINMAN_SYMBOLS) && CONFIG_IS_ENABLED(X86)
 /* See spl.h for information about this */
 binman_sym_declare(ulong, u_boot_any, image_pos);
 binman_sym_declare(ulong, u_boot_any, size);
@@ -148,7 +148,7 @@ void spl_fixup_fdt(void *fdt_blob)
 #endif
 }
 
-#if CONFIG_IS_ENABLED(BINMAN_SYMBOLS)
+#if CONFIG_IS_ENABLED(BINMAN_SYMBOLS) && CONFIG_IS_ENABLED(X86)
 ulong spl_get_image_pos(void)
 {
 #ifdef CONFIG_VPL
@@ -221,7 +221,11 @@ __weak struct image_header *spl_get_load_buffer(ssize_t offset, size_t size)
 
 void spl_set_header_raw_uboot(struct spl_image_info *spl_image)
 {
+#if CONFIG_IS_ENABLED(X86)
 	ulong u_boot_pos = binman_sym(ulong, u_boot_any, image_pos);
+#else
+	ulong u_boot_pos = BINMAN_SYM_MISSING;
+#endif
 
 	spl_image->size = CONFIG_SYS_MONITOR_LEN;
 
diff --git a/common/spl/spl_ram.c b/common/spl/spl_ram.c
index 82964592571..083b14102ee 100644
--- a/common/spl/spl_ram.c
+++ b/common/spl/spl_ram.c
@@ -70,7 +70,11 @@ static int spl_ram_load_image(struct spl_image_info *spl_image,
 		load.read = spl_ram_load_read;
 		spl_load_simple_fit(spl_image, &load, 0, header);
 	} else {
+#if CONFIG_IS_ENABLED(X86)
 		ulong u_boot_pos = binman_sym(ulong, u_boot_any, image_pos);
+#else
+		ulong u_boot_pos = BINMAN_SYM_MISSING;
+#endif
 
 		debug("Legacy image\n");
 		/*
-- 
2.36.0

[PATCH V4 2/8] arm: dts: imx8m: update binman ddr firmware node name

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

We are migrating to use BINMAN SYMBOLS, the current name is not
a valid binman type, so update to use blob-ext@[1,2,3,4].

Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 arch/arm/dts/imx8mm-u-boot.dtsi                   | 8 ++++----
 arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi | 4 ++--
 arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi  | 8 ++++----
 arch/arm/dts/imx8mn-venice-u-boot.dtsi            | 8 ++++----
 arch/arm/dts/imx8mq-u-boot.dtsi                   | 8 ++++----
 5 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/arch/arm/dts/imx8mm-u-boot.dtsi b/arch/arm/dts/imx8mm-u-boot.dtsi
index 9f66cdb65a9..5de55a2d80b 100644
--- a/arch/arm/dts/imx8mm-u-boot.dtsi
+++ b/arch/arm/dts/imx8mm-u-boot.dtsi
@@ -39,25 +39,25 @@
 			filename = "u-boot-spl.bin";
 		};
 
-		1d-imem {
+		imem_1d: blob-ext@1 {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
 			size = <0x8000>;
 			type = "blob-ext";
 		};
 
-		1d-dmem {
+		dmem_1d: blob-ext@2 {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
 			size = <0x4000>;
 			type = "blob-ext";
 		};
 
-		2d-imem {
+		imem_2d: blob-ext@3 {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
 			size = <0x8000>;
 			type = "blob-ext";
 		};
 
-		2d-dmem {
+		dmem_2d: blob-ext@4 {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
 			size = <0x4000>;
 			type = "blob-ext";
diff --git a/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi b/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi
index 46a9d7fd78b..5a52b73d7e9 100644
--- a/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi
+++ b/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi
@@ -111,13 +111,13 @@
 			filename = "u-boot-spl.bin";
 		};
 
-		1d-imem {
+		imem_1d: blob-ext@1 {
 			filename = "ddr3_imem_1d.bin";
 			size = <0x8000>;
 			type = "blob-ext";
 		};
 
-		1d_dmem {
+		dmem_1d: blob-ext@2 {
 			filename = "ddr3_dmem_1d.bin";
 			size = <0x4000>;
 			type = "blob-ext";
diff --git a/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi b/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
index 6e37622cca7..001e725f568 100644
--- a/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
+++ b/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
@@ -130,25 +130,25 @@
 			filename = "u-boot-spl.bin";
 		};
 
-		1d-imem {
+		blob_1: blob-ext@1 {
 			filename = "ddr4_imem_1d.bin";
 			size = <0x8000>;
 			type = "blob-ext";
 		};
 
-		1d_dmem {
+		blob_2: blob-ext@2 {
 			filename = "ddr4_dmem_1d.bin";
 			size = <0x4000>;
 			type = "blob-ext";
 		};
 
-		2d_imem {
+		blob_3: blob-ext@3 {
 			filename = "ddr4_imem_2d.bin";
 			size = <0x8000>;
 			type = "blob-ext";
 		};
 
-		2d_dmem {
+		blob_4: blob-ext@4 {
 			filename = "ddr4_dmem_2d.bin";
 			size = <0x4000>;
 			type = "blob-ext";
diff --git a/arch/arm/dts/imx8mn-venice-u-boot.dtsi b/arch/arm/dts/imx8mn-venice-u-boot.dtsi
index 35819553879..67922146963 100644
--- a/arch/arm/dts/imx8mn-venice-u-boot.dtsi
+++ b/arch/arm/dts/imx8mn-venice-u-boot.dtsi
@@ -126,25 +126,25 @@
 			filename = "u-boot-spl.bin";
 		};
 
-		1d-imem {
+		imem_1d: blob-ext@1 {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
 			size = <0x8000>;
 			type = "blob-ext";
 		};
 
-		1d_dmem {
+		dmem_1d: blob-ext@2 {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
 			size = <0x4000>;
 			type = "blob-ext";
 		};
 
-		2d_imem {
+		imem_2d: blob-ext@3 {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
 			size = <0x8000>;
 			type = "blob-ext";
 		};
 
-		2d_dmem {
+		dmem_2d: blob-ext@4 {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
 			size = <0x4000>;
 			type = "blob-ext";
diff --git a/arch/arm/dts/imx8mq-u-boot.dtsi b/arch/arm/dts/imx8mq-u-boot.dtsi
index 912a3d4a356..389414ad26f 100644
--- a/arch/arm/dts/imx8mq-u-boot.dtsi
+++ b/arch/arm/dts/imx8mq-u-boot.dtsi
@@ -46,25 +46,25 @@
 			filename = "u-boot-spl.bin";
 		};
 
-		1d-imem {
+		imem_1d: blob-ext@1 {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
 			size = <0x8000>;
 			type = "blob-ext";
 		};
 
-		1d-dmem {
+		dmem_1d: blob-ext@2 {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
 			size = <0x4000>;
 			type = "blob-ext";
 		};
 
-		2d-imem {
+		imem_2d: blob-ext@3 {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
 			size = <0x8000>;
 			type = "blob-ext";
 		};
 
-		2d-dmem {
+		dmem_2d: blob-ext@4 {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
 			size = <0x4000>;
 			type = "blob-ext";
-- 
2.36.0

[PATCH V4 3/8] imx: imx8mm-icore: migrate to use BINMAN

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Use BINMAN instead of imx specific packing method.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 arch/arm/mach-imx/imx8m/Kconfig                 |  1 +
 arch/arm/mach-imx/imx8m/imximage-8mm-lpddr4.cfg | 10 +---------
 configs/imx8mm-icore-mx8mm-ctouch2_defconfig    |  2 +-
 configs/imx8mm-icore-mx8mm-edimm2.2_defconfig   |  2 +-
 4 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/arch/arm/mach-imx/imx8m/Kconfig b/arch/arm/mach-imx/imx8m/Kconfig
index 24299ae037f..75ad7aab081 100644
--- a/arch/arm/mach-imx/imx8m/Kconfig
+++ b/arch/arm/mach-imx/imx8m/Kconfig
@@ -68,6 +68,7 @@ config TARGET_IMX8MM_EVK
 
 config TARGET_IMX8MM_ICORE_MX8MM
 	bool "Engicam i.Core MX8M Mini SOM"
+	select BINMAN
 	select IMX8MM
 	select SUPPORT_SPL
 	select IMX8M_LPDDR4
diff --git a/arch/arm/mach-imx/imx8m/imximage-8mm-lpddr4.cfg b/arch/arm/mach-imx/imx8m/imximage-8mm-lpddr4.cfg
index e06d53ef417..5dcb8ae72f0 100644
--- a/arch/arm/mach-imx/imx8m/imximage-8mm-lpddr4.cfg
+++ b/arch/arm/mach-imx/imx8m/imximage-8mm-lpddr4.cfg
@@ -3,13 +3,5 @@
  * Copyright 2019 NXP
  */
 
-
-FIT
 BOOT_FROM	sd
-LOADER		spl/u-boot-spl-ddr.bin	0x7E1000
-SECOND_LOADER	u-boot.itb		0x40200000 0x60000
-
-DDR_FW lpddr4_pmu_train_1d_imem.bin
-DDR_FW lpddr4_pmu_train_1d_dmem.bin
-DDR_FW lpddr4_pmu_train_2d_imem.bin
-DDR_FW lpddr4_pmu_train_2d_dmem.bin
+LOADER		u-boot-spl-ddr.bin	0x7E1000
diff --git a/configs/imx8mm-icore-mx8mm-ctouch2_defconfig b/configs/imx8mm-icore-mx8mm-ctouch2_defconfig
index d95a74a7237..dcb12e5d026 100644
--- a/configs/imx8mm-icore-mx8mm-ctouch2_defconfig
+++ b/configs/imx8mm-icore-mx8mm-ctouch2_defconfig
@@ -20,7 +20,7 @@ CONFIG_DISTRO_DEFAULTS=y
 CONFIG_FIT=y
 CONFIG_FIT_EXTERNAL_OFFSET=0x3000
 CONFIG_SPL_LOAD_FIT=y
-CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh"
+# CONFIG_USE_SPL_FIT_GENERATOR is not set
 CONFIG_OF_SYSTEM_SETUP=y
 CONFIG_DEFAULT_FDT_FILE="imx8mm-icore-mx8mm-ctouch2.dtb"
 CONFIG_SPL_BOARD_INIT=y
diff --git a/configs/imx8mm-icore-mx8mm-edimm2.2_defconfig b/configs/imx8mm-icore-mx8mm-edimm2.2_defconfig
index 43c697a39d8..22acf7317b4 100644
--- a/configs/imx8mm-icore-mx8mm-edimm2.2_defconfig
+++ b/configs/imx8mm-icore-mx8mm-edimm2.2_defconfig
@@ -20,7 +20,7 @@ CONFIG_DISTRO_DEFAULTS=y
 CONFIG_FIT=y
 CONFIG_FIT_EXTERNAL_OFFSET=0x3000
 CONFIG_SPL_LOAD_FIT=y
-CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh"
+# CONFIG_USE_SPL_FIT_GENERATOR is not set
 CONFIG_OF_SYSTEM_SETUP=y
 CONFIG_DEFAULT_FDT_FILE="imx8mm-icore-mx8mm-edimm2.2.dtb"
 CONFIG_SPL_BOARD_INIT=y
-- 
2.36.0

[PATCH V4 4/8] armv8: u-boot-spl.lds: mark __image_copy_start as symbol

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

In arch/arm/lib/sections.c there is below code:
char __image_copy_start[0] __section(".__image_copy_start");
But actually 'objdump -t spl/u-boot-spl' not able to find out
symbol '__image_copy_start' for binman update image-pos/size.

So update link file

Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 arch/arm/cpu/armv8/u-boot-spl.lds | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds
index 730eb93dbc3..9b1e7d46287 100644
--- a/arch/arm/cpu/armv8/u-boot-spl.lds
+++ b/arch/arm/cpu/armv8/u-boot-spl.lds
@@ -23,7 +23,7 @@ SECTIONS
 {
 	.text : {
 		. = ALIGN(8);
-		*(.__image_copy_start)
+		__image_copy_start = .;
 		CPUDIR/start.o (.text*)
 		*(.text*)
 	} >.sram
-- 
2.36.0

[PATCH V4 5/8] tools: binman: section: replace @ with -

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

In arch/arm/dts/imx8mp-u-boot.dtsi, there are blob-ext@1, blob-ext@2 and
etc which is for packing ddr phy firmware. However we could not declare
symbol name such as 'binman_sym_declare(ulong, blob_ext@1, image_pos)',
because '@' is not allowed, so we choose to declare the symbol
'binman_sym_declare(ulong, blob_ext_1, image_pos);' with '@' replaced with
'_'. It does not impact if there is no '@' in section name.

Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
Reviewed-by: Tom Rini <trini@konsulko.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 tools/binman/etype/section.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
index bd67238b919..e3f362b442b 100644
--- a/tools/binman/etype/section.py
+++ b/tools/binman/etype/section.py
@@ -875,7 +875,7 @@ class Entry_section(Entry):
                 entries[entry.GetPath()] = entry
             for entry in to_add.values():
                 self._CollectEntries(entries, entries_by_name, entry)
-        entries_by_name[add_entry.name] = add_entry
+        entries_by_name[add_entry.name.replace('@', '-')] = add_entry
 
     def MissingArgs(self, entry, missing):
         """Report a missing argument, if enabled
-- 
2.36.0

[PATCH V4 6/8] ddr: imx8m: helper: load ddr firmware according to binman symbols

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

By reading binman symbols, we no need hard coded IMEM_LEN/DMEM_LEN after
we update the binman dtsi to drop 0x8000/0x4000 length for the firmware.

And that could save binary size for many KBs.

Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/ddr/imx/imx8m/helper.c | 51 ++++++++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 8 deletions(-)

diff --git a/drivers/ddr/imx/imx8m/helper.c b/drivers/ddr/imx/imx8m/helper.c
index f23904bf712..b3bd57531b7 100644
--- a/drivers/ddr/imx/imx8m/helper.c
+++ b/drivers/ddr/imx/imx8m/helper.c
@@ -4,6 +4,7 @@
  */
 
 #include <common.h>
+#include <binman_sym.h>
 #include <log.h>
 #include <spl.h>
 #include <asm/global_data.h>
@@ -25,15 +26,30 @@ DECLARE_GLOBAL_DATA_PTR;
 #define DMEM_OFFSET_ADDR 0x00054000
 #define DDR_TRAIN_CODE_BASE_ADDR IP2APB_DDRPHY_IPS_BASE_ADDR(0)
 
+binman_sym_declare(ulong, blob_ext_1, image_pos);
+binman_sym_declare(ulong, blob_ext_1, size);
+
+binman_sym_declare(ulong, blob_ext_2, image_pos);
+binman_sym_declare(ulong, blob_ext_2, size);
+
+#if !IS_ENABLED(CONFIG_IMX8M_DDR3L)
+binman_sym_declare(ulong, blob_ext_3, image_pos);
+binman_sym_declare(ulong, blob_ext_3, size);
+
+binman_sym_declare(ulong, blob_ext_4, image_pos);
+binman_sym_declare(ulong, blob_ext_4, size);
+#endif
+
 /* We need PHY iMEM PHY is 32KB padded */
 void ddr_load_train_firmware(enum fw_type type)
 {
 	u32 tmp32, i;
 	u32 error = 0;
 	unsigned long pr_to32, pr_from32;
-	unsigned long fw_offset = type ? IMEM_2D_OFFSET : 0;
-	unsigned long imem_start = (unsigned long)&_end + fw_offset;
-	unsigned long dmem_start;
+	uint32_t fw_offset = type ? IMEM_2D_OFFSET : 0;
+	uint32_t imem_start = (unsigned long)&_end + fw_offset;
+	uint32_t dmem_start;
+	uint32_t imem_len = IMEM_LEN, dmem_len = DMEM_LEN;
 
 #ifdef CONFIG_SPL_OF_CONTROL
 	if (gd->fdt_blob && !fdt_check_header(gd->fdt_blob)) {
@@ -43,11 +59,30 @@ void ddr_load_train_firmware(enum fw_type type)
 	}
 #endif
 
-	dmem_start = imem_start + IMEM_LEN;
+	if (CONFIG_IS_ENABLED(BINMAN_SYMBOLS)) {
+		switch (type) {
+		case FW_1D_IMAGE:
+			imem_start = binman_sym(ulong, blob_ext_1, image_pos);
+			imem_len = binman_sym(ulong, blob_ext_1, size);
+			dmem_start = binman_sym(ulong, blob_ext_2, image_pos);
+			dmem_len = binman_sym(ulong, blob_ext_2, size);
+			break;
+		case FW_2D_IMAGE:
+#if !IS_ENABLED(CONFIG_IMX8M_DDR3L)
+			imem_start = binman_sym(ulong, blob_ext_3, image_pos);
+			imem_len = binman_sym(ulong, blob_ext_3, size);
+			dmem_start = binman_sym(ulong, blob_ext_4, image_pos);
+			dmem_len = binman_sym(ulong, blob_ext_4, size);
+#endif
+			break;
+		}
+	}
+
+	dmem_start = imem_start + imem_len;
 
 	pr_from32 = imem_start;
 	pr_to32 = DDR_TRAIN_CODE_BASE_ADDR + 4 * IMEM_OFFSET_ADDR;
-	for (i = 0x0; i < IMEM_LEN; ) {
+	for (i = 0x0; i < imem_len; ) {
 		tmp32 = readl(pr_from32);
 		writew(tmp32 & 0x0000ffff, pr_to32);
 		pr_to32 += 4;
@@ -59,7 +94,7 @@ void ddr_load_train_firmware(enum fw_type type)
 
 	pr_from32 = dmem_start;
 	pr_to32 = DDR_TRAIN_CODE_BASE_ADDR + 4 * DMEM_OFFSET_ADDR;
-	for (i = 0x0; i < DMEM_LEN; ) {
+	for (i = 0x0; i < dmem_len; ) {
 		tmp32 = readl(pr_from32);
 		writew(tmp32 & 0x0000ffff, pr_to32);
 		pr_to32 += 4;
@@ -72,7 +107,7 @@ void ddr_load_train_firmware(enum fw_type type)
 	debug("check ddr_pmu_train_imem code\n");
 	pr_from32 = imem_start;
 	pr_to32 = DDR_TRAIN_CODE_BASE_ADDR + 4 * IMEM_OFFSET_ADDR;
-	for (i = 0x0; i < IMEM_LEN; ) {
+	for (i = 0x0; i < imem_len; ) {
 		tmp32 = (readw(pr_to32) & 0x0000ffff);
 		pr_to32 += 4;
 		tmp32 += ((readw(pr_to32) & 0x0000ffff) << 16);
@@ -93,7 +128,7 @@ void ddr_load_train_firmware(enum fw_type type)
 	debug("check ddr4_pmu_train_dmem code\n");
 	pr_from32 = dmem_start;
 	pr_to32 = DDR_TRAIN_CODE_BASE_ADDR + 4 * DMEM_OFFSET_ADDR;
-	for (i = 0x0; i < DMEM_LEN;) {
+	for (i = 0x0; i < dmem_len;) {
 		tmp32 = (readw(pr_to32) & 0x0000ffff);
 		pr_to32 += 4;
 		tmp32 += ((readw(pr_to32) & 0x0000ffff) << 16);
-- 
2.36.0

[PATCH V4 7/8] arm: dts: imx8m: shrink ddr firmware size to actual file size

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

After we switch to use BINMAN_SYMBOLS, there is no need to pad
the file size to 0x8000 and 0x4000. After we use BINMAN_SYMBOLS,
the u-boot-spl-ddr.bin shrink about 36KB with i.MX8MP-EVK.

Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 arch/arm/dts/imx8mm-u-boot.dtsi                  | 8 ++++----
 arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi       | 8 ++++----
 arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi         | 8 ++++----
 arch/arm/dts/imx8mn-evk-u-boot.dtsi              | 8 ++++----
 arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi | 8 ++++----
 arch/arm/dts/imx8mn-venice-u-boot.dtsi           | 8 ++++----
 arch/arm/dts/imx8mp-u-boot.dtsi                  | 8 ++++----
 arch/arm/dts/imx8mq-cm-u-boot.dtsi               | 8 ++++----
 arch/arm/dts/imx8mq-u-boot.dtsi                  | 8 ++++----
 9 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/arch/arm/dts/imx8mm-u-boot.dtsi b/arch/arm/dts/imx8mm-u-boot.dtsi
index 5de55a2d80b..19a2da30f51 100644
--- a/arch/arm/dts/imx8mm-u-boot.dtsi
+++ b/arch/arm/dts/imx8mm-u-boot.dtsi
@@ -41,25 +41,25 @@
 
 		imem_1d: blob-ext@1 {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
-			size = <0x8000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		dmem_1d: blob-ext@2 {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
-			size = <0x4000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		imem_2d: blob-ext@3 {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
-			size = <0x8000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		dmem_2d: blob-ext@4 {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
-			size = <0x4000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 	};
diff --git a/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi b/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi
index eb1dd8debba..e1740fa31a6 100644
--- a/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi
+++ b/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi
@@ -149,22 +149,22 @@
 
 		blob_1: blob-ext@1 {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
-			size = <0x8000>;
+			align-end = <4>;
 		};
 
 		blob_2: blob-ext@2 {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
-			size = <0x4000>;
+			align-end = <4>;
 		};
 
 		blob_3: blob-ext@3 {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
-			size = <0x8000>;
+			align-end = <4>;
 		};
 
 		blob_4: blob-ext@4 {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
-			size = <0x4000>;
+			align-end = <4>;
 		};
 	};
 
diff --git a/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi b/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi
index 4d0ecb07d4f..1fe2d0fd507 100644
--- a/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi
+++ b/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi
@@ -157,22 +157,22 @@
 
 		blob_1: blob-ext@1 {
 			filename = "ddr4_imem_1d_201810.bin";
-			size = <0x8000>;
+			align-end = <4>;
 		};
 
 		blob_2: blob-ext@2 {
 			filename = "ddr4_dmem_1d_201810.bin";
-			size = <0x4000>;
+			align-end = <4>;
 		};
 
 		blob_3: blob-ext@3 {
 			filename = "ddr4_imem_2d_201810.bin";
-			size = <0x8000>;
+			align-end = <4>;
 		};
 
 		blob_4: blob-ext@4 {
 			filename = "ddr4_dmem_2d_201810.bin";
-			size = <0x4000>;
+			align-end = <4>;
 		};
 	};
 
diff --git a/arch/arm/dts/imx8mn-evk-u-boot.dtsi b/arch/arm/dts/imx8mn-evk-u-boot.dtsi
index 3db46d4cbcb..4f6dcf307b2 100644
--- a/arch/arm/dts/imx8mn-evk-u-boot.dtsi
+++ b/arch/arm/dts/imx8mn-evk-u-boot.dtsi
@@ -38,22 +38,22 @@
 
 		blob_1: blob-ext@1 {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
-			size = <0x8000>;
+			align-end = <4>;
 		};
 
 		blob_2: blob-ext@2 {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
-			size = <0x4000>;
+			align-end = <4>;
 		};
 
 		blob_3: blob-ext@3 {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
-			size = <0x8000>;
+			align-end = <4>;
 		};
 
 		blob_4: blob-ext@4 {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
-			size = <0x4000>;
+			align-end = <4>;
 		};
 	};
 
diff --git a/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi b/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
index 001e725f568..32ef7929288 100644
--- a/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
+++ b/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
@@ -132,25 +132,25 @@
 
 		blob_1: blob-ext@1 {
 			filename = "ddr4_imem_1d.bin";
-			size = <0x8000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		blob_2: blob-ext@2 {
 			filename = "ddr4_dmem_1d.bin";
-			size = <0x4000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		blob_3: blob-ext@3 {
 			filename = "ddr4_imem_2d.bin";
-			size = <0x8000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		blob_4: blob-ext@4 {
 			filename = "ddr4_dmem_2d.bin";
-			size = <0x4000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 	};
diff --git a/arch/arm/dts/imx8mn-venice-u-boot.dtsi b/arch/arm/dts/imx8mn-venice-u-boot.dtsi
index 67922146963..e3a0b170347 100644
--- a/arch/arm/dts/imx8mn-venice-u-boot.dtsi
+++ b/arch/arm/dts/imx8mn-venice-u-boot.dtsi
@@ -128,25 +128,25 @@
 
 		imem_1d: blob-ext@1 {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
-			size = <0x8000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		dmem_1d: blob-ext@2 {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
-			size = <0x4000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		imem_2d: blob-ext@3 {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
-			size = <0x8000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		dmem_2d: blob-ext@4 {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
-			size = <0x4000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 	};
diff --git a/arch/arm/dts/imx8mp-u-boot.dtsi b/arch/arm/dts/imx8mp-u-boot.dtsi
index 20edd90cfad..274515a010e 100644
--- a/arch/arm/dts/imx8mp-u-boot.dtsi
+++ b/arch/arm/dts/imx8mp-u-boot.dtsi
@@ -63,22 +63,22 @@
 
 		blob_1: blob-ext@1 {
 			filename = "lpddr4_pmu_train_1d_imem_202006.bin";
-			size = <0x8000>;
+			align-end = <4>;
 		};
 
 		blob_2: blob-ext@2 {
 			filename = "lpddr4_pmu_train_1d_dmem_202006.bin";
-			size = <0x4000>;
+			align-end = <4>;
 		};
 
 		blob_3: blob-ext@3 {
 			filename = "lpddr4_pmu_train_2d_imem_202006.bin";
-			size = <0x8000>;
+			align-end = <4>;
 		};
 
 		blob_4: blob-ext@4 {
 			filename = "lpddr4_pmu_train_2d_dmem_202006.bin";
-			size = <0x4000>;
+			align-end = <4>;
 		};
 	};
 
diff --git a/arch/arm/dts/imx8mq-cm-u-boot.dtsi b/arch/arm/dts/imx8mq-cm-u-boot.dtsi
index e2f4b0e740d..9538a04ed97 100644
--- a/arch/arm/dts/imx8mq-cm-u-boot.dtsi
+++ b/arch/arm/dts/imx8mq-cm-u-boot.dtsi
@@ -30,22 +30,22 @@
 
 		blob_1: blob-ext@1 {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
-			size = <0x8000>;
+			align-end = <4>;
 		};
 
 		blob_2: blob-ext@2 {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
-			size = <0x4000>;
+			align-end = <4>;
 		};
 
 		blob_3: blob-ext@3 {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
-			size = <0x8000>;
+			align-end = <4>;
 		};
 
 		blob_4: blob-ext@4 {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
-			size = <0x4000>;
+			align-end = <4>;
 		};
 	};
 
diff --git a/arch/arm/dts/imx8mq-u-boot.dtsi b/arch/arm/dts/imx8mq-u-boot.dtsi
index 389414ad26f..1495869fcd2 100644
--- a/arch/arm/dts/imx8mq-u-boot.dtsi
+++ b/arch/arm/dts/imx8mq-u-boot.dtsi
@@ -48,25 +48,25 @@
 
 		imem_1d: blob-ext@1 {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
-			size = <0x8000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		dmem_1d: blob-ext@2 {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
-			size = <0x4000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		imem_2d: blob-ext@3 {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
-			size = <0x8000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		dmem_2d: blob-ext@4 {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
-			size = <0x4000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 	};
-- 
2.36.0

[PATCH V4 8/8] binman_sym: guard with CONFIG_SPL_BINMAN_SYMBOLS

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

There is case that CONFIG_BINMAN is defined, but
CONFIG_SPL_BINMAN_SYMBOLS is not defined. In that case, there will be
build failure. So use CONFIG_SPL_BINMAN_SYMBOLS to guard the macros, and
define CONFIG_SPL_BINMAN_SYMBOLS in binman syms test.

Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 include/binman_sym.h                        | 2 +-
 tools/binman/test/u_boot_binman_syms.c      | 1 +
 tools/binman/test/u_boot_binman_syms_size.c | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/binman_sym.h b/include/binman_sym.h
index 72e6765fe52..548d8f5654c 100644
--- a/include/binman_sym.h
+++ b/include/binman_sym.h
@@ -13,7 +13,7 @@
 
 #define BINMAN_SYM_MISSING	(-1UL)
 
-#ifdef CONFIG_BINMAN
+#ifdef CONFIG_SPL_BINMAN_SYMBOLS
 
 /**
  * binman_symname() - Internal function to get a binman symbol name
diff --git a/tools/binman/test/u_boot_binman_syms.c b/tools/binman/test/u_boot_binman_syms.c
index 37fc339ce84..f4a4d1f6846 100644
--- a/tools/binman/test/u_boot_binman_syms.c
+++ b/tools/binman/test/u_boot_binman_syms.c
@@ -6,6 +6,7 @@
  */
 
 #define CONFIG_BINMAN
+#define CONFIG_SPL_BINMAN_SYMBOLS
 #include <binman_sym.h>
 
 binman_sym_declare(unsigned long, u_boot_spl_any, offset);
diff --git a/tools/binman/test/u_boot_binman_syms_size.c b/tools/binman/test/u_boot_binman_syms_size.c
index 7224bc1863c..3a01d8ca4be 100644
--- a/tools/binman/test/u_boot_binman_syms_size.c
+++ b/tools/binman/test/u_boot_binman_syms_size.c
@@ -6,6 +6,7 @@
  */
 
 #define CONFIG_BINMAN
+#define CONFIG_SPL_BINMAN_SYMBOLS
 #include <binman_sym.h>
 
 binman_sym_declare(char, u_boot_spl, pos);
-- 
2.36.0

Re: [PATCH V4 1/8] spl: guard u_boot_any with X86

[Tom Rini]

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

On Fri, May 20, 2022 at 10:10:40PM +0800, Peng Fan (OSS) wrote:

> From: Peng Fan <peng.fan@nxp.com>
> 
> set the symbol as weak not work if LTO is enabled. Since u_boot_any is
> only used on X86 for now, so guard it with X86, otherwise build break
> if we use BINMAN_SYMBOLS on i.MX.
> 
> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  common/spl/spl.c     | 8 ++++++--
>  common/spl/spl_ram.c | 4 ++++
>  2 files changed, 10 insertions(+), 2 deletions(-)

I think we long term need to figure this out and address it so LTO
works.  But for now can you please guard this with a test on LTO
instead, so it's clear where the problem is?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH V4 4/8] armv8: u-boot-spl.lds: mark __image_copy_start as symbol

[Tom Rini]

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

On Fri, May 20, 2022 at 10:10:43PM +0800, Peng Fan (OSS) wrote:

> From: Peng Fan <peng.fan@nxp.com>
> 
> In arch/arm/lib/sections.c there is below code:
> char __image_copy_start[0] __section(".__image_copy_start");
> But actually 'objdump -t spl/u-boot-spl' not able to find out
> symbol '__image_copy_start' for binman update image-pos/size.
> 
> So update link file
> 
> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> Signed-off-by: Peng Fan <peng.fan@nxp.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

RE: [PATCH V4 1/8] spl: guard u_boot_any with X86

[Peng Fan]

> Subject: Re: [PATCH V4 1/8] spl: guard u_boot_any with X86
> 
> On Fri, May 20, 2022 at 10:10:40PM +0800, Peng Fan (OSS) wrote:
> 
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > set the symbol as weak not work if LTO is enabled. Since u_boot_any is
> > only used on X86 for now, so guard it with X86, otherwise build break
> > if we use BINMAN_SYMBOLS on i.MX.
> >
> > Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  common/spl/spl.c     | 8 ++++++--
> >  common/spl/spl_ram.c | 4 ++++
> >  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> I think we long term need to figure this out and address it so LTO works.  But
> for now can you please guard this with a test on LTO instead, so it's clear
> where the problem is?

Sorry, I could not get your point about guard with a test on LTO.

Actually binman weak symbol will report a warning log if there is no u_boot_any
binman symbol. Since only X86 use it, I guard with X86.

Thanks,
Peng.

> 
> --
> Tom

Re: [PATCH V4 1/8] spl: guard u_boot_any with X86

[Tom Rini]

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

On Sat, May 21, 2022 at 08:33:56AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH V4 1/8] spl: guard u_boot_any with X86
> > 
> > On Fri, May 20, 2022 at 10:10:40PM +0800, Peng Fan (OSS) wrote:
> > 
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > set the symbol as weak not work if LTO is enabled. Since u_boot_any is
> > > only used on X86 for now, so guard it with X86, otherwise build break
> > > if we use BINMAN_SYMBOLS on i.MX.
> > >
> > > Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  common/spl/spl.c     | 8 ++++++--
> > >  common/spl/spl_ram.c | 4 ++++
> > >  2 files changed, 10 insertions(+), 2 deletions(-)
> > 
> > I think we long term need to figure this out and address it so LTO works.  But
> > for now can you please guard this with a test on LTO instead, so it's clear
> > where the problem is?
> 
> Sorry, I could not get your point about guard with a test on LTO.
> 
> Actually binman weak symbol will report a warning log if there is no u_boot_any
> binman symbol. Since only X86 use it, I guard with X86.

Why are you mentioning LTO in the commit message?  When I read the
commit message it sounds like you're saying the problem is that LTO
doesn't like how this symbol is handled, but if LTO was disabled,
everything would be fine.  If it's not LTO-related, please re-word the
message instead.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH V4 1/8] spl: guard u_boot_any with X86

[Alper Nebi Yasak]

On 20/05/2022 17:10, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> set the symbol as weak not work if LTO is enabled. Since u_boot_any is
> only used on X86 for now, so guard it with X86, otherwise build break
> if we use BINMAN_SYMBOLS on i.MX.
> 
> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  common/spl/spl.c     | 8 ++++++--
>  common/spl/spl_ram.c | 4 ++++
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> [...]

If I apply the series without this patch and try to build imx8mm-beacon,
I get the following error, and I'm assuming that's what you're trying to
solve here:

    binman: Section '/binman/u-boot-spl-ddr': \
      Symbol '_binman_u_boot_any_prop_image_pos' in entry \
      '/binman/u-boot-spl-ddr/u-boot-spl/u-boot-spl-nodtb': \
      Entry 'u-boot-any' not found in list (...)

The immediate cause for this is that your 'u-boot-spl-ddr' image has no
'u-boot'-like entry, meaning there's no reasonable value to write into
the non-optional 'u_boot_any' symbol.

(I think it might be OK to make this symbol optional, but LTO breaks
that as you found out. The rest of this email is about my thoughts
before I realized the LTO problem, but they're still relevant.)


It looks like binman images were designed to be monolithic instead of
modular, and it's assumed you'd have one fully-specified image for your
'flash.bin' where 'u-boot-spl' would know about (for example) a 'u-boot'
in a FIT entry. (I would like things to be modular eventually, though).

I tried to merge things into a single binman image, but mkimage is
trying to read 'u-boot-spl-ddr.bin' which would no longer exist. I see
this is because it's specified in 'spl/u-boot-spl.cfgout' as a LOADER
file in a later patch.

Maybe 'imx8mimage.c' can be changed to use mkimage's -d argument as the
LOADER instead? That's how binman passes the input data to mkimage, but
that seems to be ignored in this case. Then we can merge things into a
single image which will be able to set the 'u_boot_any' symbols without
error.

Otherwise, the mkimage file reading error can be solved by creating a
new binman 'imx8m-image' entry type that creates the 'cfgout' and calls
mkimage with it. (Binman images would still need to be merged).

Re: [PATCH V4 1/8] spl: guard u_boot_any with X86

[Alper Nebi Yasak]

On 21/05/2022 15:05, Tom Rini wrote:
> On Sat, May 21, 2022 at 08:33:56AM +0000, Peng Fan wrote:
>>> Subject: Re: [PATCH V4 1/8] spl: guard u_boot_any with X86
>>>
>>> On Fri, May 20, 2022 at 10:10:40PM +0800, Peng Fan (OSS) wrote:
>>>
>>>> From: Peng Fan <peng.fan@nxp.com>
>>>>
>>>> set the symbol as weak not work if LTO is enabled. Since u_boot_any is
>>>> only used on X86 for now, so guard it with X86, otherwise build break
>>>> if we use BINMAN_SYMBOLS on i.MX.
>>>>
>>>> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
>>>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>>>> ---
>>>>  common/spl/spl.c     | 8 ++++++--
>>>>  common/spl/spl_ram.c | 4 ++++
>>>>  2 files changed, 10 insertions(+), 2 deletions(-)
>>>
>>> I think we long term need to figure this out and address it so LTO works.  But
>>> for now can you please guard this with a test on LTO instead, so it's clear
>>> where the problem is?
>>
>> Sorry, I could not get your point about guard with a test on LTO.
>>
>> Actually binman weak symbol will report a warning log if there is no u_boot_any
>> binman symbol. Since only X86 use it, I guard with X86.
> 
> Why are you mentioning LTO in the commit message?  When I read the
> commit message it sounds like you're saying the problem is that LTO
> doesn't like how this symbol is handled, but if LTO was disabled,
> everything would be fine.  If it's not LTO-related, please re-word the
> message instead.

It looks like we should be able to change things in common/spl/spl.c to:

    #if CONFIG_IS_ENABLED(BINMAN_SYMBOLS)
    /* See spl.h for information about this */
    binman_sym_declare_optional(ulong, u_boot_any, image_pos);
    binman_sym_declare_optional(ulong, u_boot_any, size);
    #endif

which would mark the symbol as 'weak' and turn the error into a warning
on the binman side. But that is somehow being undone by LTO.

I'm trying to build for imx8mm-beacon with that change instead of this
patch. With CONFIG_LTO=y, build fails and spl/u-boot-spl.sym has:

> 00000000007fbe28 l     O .binman_sym	0000000000000008 _binman_u_boot_any_prop_image_pos

Looks like the size symbol is optimized out. With CONFIG_LTO unset, the
build succeeds and the same file has:

> 00000000007fe90c  w    O .binman_sym	0000000000000008 _binman_u_boot_any_prop_image_pos
> 00000000007fe904  w    O .binman_sym	0000000000000008 _binman_u_boot_any_prop_size

I don't know much about linking stuff, so this is as deep as I could dig...

Re: [PATCH V4 2/8] arm: dts: imx8m: update binman ddr firmware node name

[Alper Nebi Yasak]

On 20/05/2022 17:10, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> We are migrating to use BINMAN SYMBOLS, the current name is not
> a valid binman type, so update to use blob-ext@[1,2,3,4].
> 
> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  arch/arm/dts/imx8mm-u-boot.dtsi                   | 8 ++++----
>  arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi | 4 ++--
>  arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi  | 8 ++++----
>  arch/arm/dts/imx8mn-venice-u-boot.dtsi            | 8 ++++----
>  arch/arm/dts/imx8mq-u-boot.dtsi                   | 8 ++++----
>  5 files changed, 18 insertions(+), 18 deletions(-)

I think descriptive entry names are better in general, so I prefer the
old names to a bunch of 'blob-ext's. Symbol resolution is done using the
entry names anyway, I don't see why this change would be necessary.

In any case, the names and labels should be consistent across the
different dts files, assuming the blobs have the same purpose / meaning.
I see that labels starting with digits cause a dtc parse error, so the
entries could be `imem_1d: imem-1d { ... };` and so on.

Also, the blobs essentially look the same to me aside from their
filenames, just duplicated over different imx8m* variants and boards.
Maybe binman parts could be unified into a single dtsi file later on?
(Filename differences can be handled by new 'blob_named_by_arg's btw.)

> diff --git a/arch/arm/dts/imx8mm-u-boot.dtsi b/arch/arm/dts/imx8mm-u-boot.dtsi
> index 9f66cdb65a9..5de55a2d80b 100644
> --- a/arch/arm/dts/imx8mm-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mm-u-boot.dtsi
> @@ -39,25 +39,25 @@
>  			filename = "u-boot-spl.bin";
>  		};
>  
> -		1d-imem {
> +		imem_1d: blob-ext@1 {
>  			filename = "lpddr4_pmu_train_1d_imem.bin";
>  			size = <0x8000>;
>  			type = "blob-ext";
>  		};
>  
> -		1d-dmem {
> +		dmem_1d: blob-ext@2 {
>  			filename = "lpddr4_pmu_train_1d_dmem.bin";
>  			size = <0x4000>;
>  			type = "blob-ext";
>  		};
>  
> -		2d-imem {
> +		imem_2d: blob-ext@3 {
>  			filename = "lpddr4_pmu_train_2d_imem.bin";
>  			size = <0x8000>;
>  			type = "blob-ext";
>  		};
>  
> -		2d-dmem {
> +		dmem_2d: blob-ext@4 {
>  			filename = "lpddr4_pmu_train_2d_dmem.bin";
>  			size = <0x4000>;
>  			type = "blob-ext";
> diff --git a/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi b/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi
> index 46a9d7fd78b..5a52b73d7e9 100644
> --- a/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi
> +++ b/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi
> @@ -111,13 +111,13 @@
>  			filename = "u-boot-spl.bin";
>  		};
>  
> -		1d-imem {
> +		imem_1d: blob-ext@1 {
>  			filename = "ddr3_imem_1d.bin";
>  			size = <0x8000>;
>  			type = "blob-ext";
>  		};
>  
> -		1d_dmem {
> +		dmem_1d: blob-ext@2 {
>  			filename = "ddr3_dmem_1d.bin";
>  			size = <0x4000>;
>  			type = "blob-ext";
> diff --git a/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi b/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
> index 6e37622cca7..001e725f568 100644
> --- a/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
> @@ -130,25 +130,25 @@
>  			filename = "u-boot-spl.bin";
>  		};
>  
> -		1d-imem {
> +		blob_1: blob-ext@1 {
>  			filename = "ddr4_imem_1d.bin";
>  			size = <0x8000>;
>  			type = "blob-ext";
>  		};
>  
> -		1d_dmem {
> +		blob_2: blob-ext@2 {
>  			filename = "ddr4_dmem_1d.bin";
>  			size = <0x4000>;
>  			type = "blob-ext";
>  		};
>  
> -		2d_imem {
> +		blob_3: blob-ext@3 {
>  			filename = "ddr4_imem_2d.bin";
>  			size = <0x8000>;
>  			type = "blob-ext";
>  		};
>  
> -		2d_dmem {
> +		blob_4: blob-ext@4 {
>  			filename = "ddr4_dmem_2d.bin";
>  			size = <0x4000>;
>  			type = "blob-ext";
> diff --git a/arch/arm/dts/imx8mn-venice-u-boot.dtsi b/arch/arm/dts/imx8mn-venice-u-boot.dtsi
> index 35819553879..67922146963 100644
> --- a/arch/arm/dts/imx8mn-venice-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mn-venice-u-boot.dtsi
> @@ -126,25 +126,25 @@
>  			filename = "u-boot-spl.bin";
>  		};
>  
> -		1d-imem {
> +		imem_1d: blob-ext@1 {
>  			filename = "lpddr4_pmu_train_1d_imem.bin";
>  			size = <0x8000>;
>  			type = "blob-ext";
>  		};
>  
> -		1d_dmem {
> +		dmem_1d: blob-ext@2 {
>  			filename = "lpddr4_pmu_train_1d_dmem.bin";
>  			size = <0x4000>;
>  			type = "blob-ext";
>  		};
>  
> -		2d_imem {
> +		imem_2d: blob-ext@3 {
>  			filename = "lpddr4_pmu_train_2d_imem.bin";
>  			size = <0x8000>;
>  			type = "blob-ext";
>  		};
>  
> -		2d_dmem {
> +		dmem_2d: blob-ext@4 {
>  			filename = "lpddr4_pmu_train_2d_dmem.bin";
>  			size = <0x4000>;
>  			type = "blob-ext";
> diff --git a/arch/arm/dts/imx8mq-u-boot.dtsi b/arch/arm/dts/imx8mq-u-boot.dtsi
> index 912a3d4a356..389414ad26f 100644
> --- a/arch/arm/dts/imx8mq-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mq-u-boot.dtsi
> @@ -46,25 +46,25 @@
>  			filename = "u-boot-spl.bin";
>  		};
>  
> -		1d-imem {
> +		imem_1d: blob-ext@1 {
>  			filename = "lpddr4_pmu_train_1d_imem.bin";
>  			size = <0x8000>;
>  			type = "blob-ext";
>  		};
>  
> -		1d-dmem {
> +		dmem_1d: blob-ext@2 {
>  			filename = "lpddr4_pmu_train_1d_dmem.bin";
>  			size = <0x4000>;
>  			type = "blob-ext";
>  		};
>  
> -		2d-imem {
> +		imem_2d: blob-ext@3 {
>  			filename = "lpddr4_pmu_train_2d_imem.bin";
>  			size = <0x8000>;
>  			type = "blob-ext";
>  		};
>  
> -		2d-dmem {
> +		dmem_2d: blob-ext@4 {
>  			filename = "lpddr4_pmu_train_2d_dmem.bin";
>  			size = <0x4000>;
>  			type = "blob-ext";

Re: [PATCH V4 3/8] imx: imx8mm-icore: migrate to use BINMAN

[Alper Nebi Yasak]

On 20/05/2022 17:10, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Use BINMAN instead of imx specific packing method.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  arch/arm/mach-imx/imx8m/Kconfig                 |  1 +
>  arch/arm/mach-imx/imx8m/imximage-8mm-lpddr4.cfg | 10 +---------

Maybe this could be done for other imximage*.cfg files as well?

>  configs/imx8mm-icore-mx8mm-ctouch2_defconfig    |  2 +-
>  configs/imx8mm-icore-mx8mm-edimm2.2_defconfig   |  2 +-
>  4 files changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/imx8m/Kconfig b/arch/arm/mach-imx/imx8m/Kconfig
> index 24299ae037f..75ad7aab081 100644
> --- a/arch/arm/mach-imx/imx8m/Kconfig
> +++ b/arch/arm/mach-imx/imx8m/Kconfig
> @@ -68,6 +68,7 @@ config TARGET_IMX8MM_EVK
>  
>  config TARGET_IMX8MM_ICORE_MX8MM
>  	bool "Engicam i.Core MX8M Mini SOM"
> +	select BINMAN
>  	select IMX8MM
>  	select SUPPORT_SPL
>  	select IMX8M_LPDDR4
> diff --git a/arch/arm/mach-imx/imx8m/imximage-8mm-lpddr4.cfg b/arch/arm/mach-imx/imx8m/imximage-8mm-lpddr4.cfg
> index e06d53ef417..5dcb8ae72f0 100644
> --- a/arch/arm/mach-imx/imx8m/imximage-8mm-lpddr4.cfg
> +++ b/arch/arm/mach-imx/imx8m/imximage-8mm-lpddr4.cfg
> @@ -3,13 +3,5 @@
>   * Copyright 2019 NXP
>   */
>  
> -
> -FIT
>  BOOT_FROM	sd
> -LOADER		spl/u-boot-spl-ddr.bin	0x7E1000
> -SECOND_LOADER	u-boot.itb		0x40200000 0x60000
> -
> -DDR_FW lpddr4_pmu_train_1d_imem.bin
> -DDR_FW lpddr4_pmu_train_1d_dmem.bin
> -DDR_FW lpddr4_pmu_train_2d_imem.bin
> -DDR_FW lpddr4_pmu_train_2d_dmem.bin
> +LOADER		u-boot-spl-ddr.bin	0x7E1000
> diff --git a/configs/imx8mm-icore-mx8mm-ctouch2_defconfig b/configs/imx8mm-icore-mx8mm-ctouch2_defconfig
> index d95a74a7237..dcb12e5d026 100644
> --- a/configs/imx8mm-icore-mx8mm-ctouch2_defconfig
> +++ b/configs/imx8mm-icore-mx8mm-ctouch2_defconfig
> @@ -20,7 +20,7 @@ CONFIG_DISTRO_DEFAULTS=y
>  CONFIG_FIT=y
>  CONFIG_FIT_EXTERNAL_OFFSET=0x3000
>  CONFIG_SPL_LOAD_FIT=y
> -CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh"
> +# CONFIG_USE_SPL_FIT_GENERATOR is not set
>  CONFIG_OF_SYSTEM_SETUP=y
>  CONFIG_DEFAULT_FDT_FILE="imx8mm-icore-mx8mm-ctouch2.dtb"
>  CONFIG_SPL_BOARD_INIT=y
> diff --git a/configs/imx8mm-icore-mx8mm-edimm2.2_defconfig b/configs/imx8mm-icore-mx8mm-edimm2.2_defconfig
> index 43c697a39d8..22acf7317b4 100644
> --- a/configs/imx8mm-icore-mx8mm-edimm2.2_defconfig
> +++ b/configs/imx8mm-icore-mx8mm-edimm2.2_defconfig
> @@ -20,7 +20,7 @@ CONFIG_DISTRO_DEFAULTS=y
>  CONFIG_FIT=y
>  CONFIG_FIT_EXTERNAL_OFFSET=0x3000
>  CONFIG_SPL_LOAD_FIT=y
> -CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh"
> +# CONFIG_USE_SPL_FIT_GENERATOR is not set
>  CONFIG_OF_SYSTEM_SETUP=y
>  CONFIG_DEFAULT_FDT_FILE="imx8mm-icore-mx8mm-edimm2.2.dtb"
>  CONFIG_SPL_BOARD_INIT=y

Re: [PATCH V4 5/8] tools: binman: section: replace @ with -

[Alper Nebi Yasak]

On 20/05/2022 17:10, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> In arch/arm/dts/imx8mp-u-boot.dtsi, there are blob-ext@1, blob-ext@2 and
> etc which is for packing ddr phy firmware. However we could not declare
> symbol name such as 'binman_sym_declare(ulong, blob_ext@1, image_pos)',
> because '@' is not allowed, so we choose to declare the symbol
> 'binman_sym_declare(ulong, blob_ext_1, image_pos);' with '@' replaced with
> '_'. It does not impact if there is no '@' in section name.
> 
> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> Reviewed-by: Tom Rini <trini@konsulko.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  tools/binman/etype/section.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

This shouldn't be necessary if you keep the old names for the binman
entries and use `binman_sym_declare(ulong, imem_1d, image_pos);` etc.
for the symbols.

> diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
> index bd67238b919..e3f362b442b 100644
> --- a/tools/binman/etype/section.py
> +++ b/tools/binman/etype/section.py
> @@ -875,7 +875,7 @@ class Entry_section(Entry):
>                  entries[entry.GetPath()] = entry
>              for entry in to_add.values():
>                  self._CollectEntries(entries, entries_by_name, entry)
> -        entries_by_name[add_entry.name] = add_entry
> +        entries_by_name[add_entry.name.replace('@', '-')] = add_entry

The correct place to do this would be LookupSymbol() in
binman/etype/section.py, but I'm not convinced this should be done at
all. I'd say if an entry is important enough to have a symbol for it, it
should have a unique, descriptive, non-@ name.

>  
>      def MissingArgs(self, entry, missing):
>          """Report a missing argument, if enabled

Re: [PATCH V4 6/8] ddr: imx8m: helper: load ddr firmware according to binman symbols

[Alper Nebi Yasak]

On 20/05/2022 17:10, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> By reading binman symbols, we no need hard coded IMEM_LEN/DMEM_LEN after
> we update the binman dtsi to drop 0x8000/0x4000 length for the firmware.
> 
> And that could save binary size for many KBs.
> 
> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/ddr/imx/imx8m/helper.c | 51 ++++++++++++++++++++++++++++------
>  1 file changed, 43 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/ddr/imx/imx8m/helper.c b/drivers/ddr/imx/imx8m/helper.c
> index f23904bf712..b3bd57531b7 100644
> --- a/drivers/ddr/imx/imx8m/helper.c
> +++ b/drivers/ddr/imx/imx8m/helper.c
> @@ -4,6 +4,7 @@
>   */
>  
>  #include <common.h>
> +#include <binman_sym.h>
>  #include <log.h>
>  #include <spl.h>
>  #include <asm/global_data.h>
> @@ -25,15 +26,30 @@ DECLARE_GLOBAL_DATA_PTR;
>  #define DMEM_OFFSET_ADDR 0x00054000
>  #define DDR_TRAIN_CODE_BASE_ADDR IP2APB_DDRPHY_IPS_BASE_ADDR(0)
>  
> +binman_sym_declare(ulong, blob_ext_1, image_pos);
> +binman_sym_declare(ulong, blob_ext_1, size);
> +
> +binman_sym_declare(ulong, blob_ext_2, image_pos);
> +binman_sym_declare(ulong, blob_ext_2, size);
> +
> +#if !IS_ENABLED(CONFIG_IMX8M_DDR3L)
> +binman_sym_declare(ulong, blob_ext_3, image_pos);
> +binman_sym_declare(ulong, blob_ext_3, size);
> +
> +binman_sym_declare(ulong, blob_ext_4, image_pos);
> +binman_sym_declare(ulong, blob_ext_4, size);
> +#endif
> +

(Descriptive entry names would make these more meaningful.)

>  /* We need PHY iMEM PHY is 32KB padded */
>  void ddr_load_train_firmware(enum fw_type type)
>  {
>  	u32 tmp32, i;
>  	u32 error = 0;
>  	unsigned long pr_to32, pr_from32;
> -	unsigned long fw_offset = type ? IMEM_2D_OFFSET : 0;
> -	unsigned long imem_start = (unsigned long)&_end + fw_offset;
> -	unsigned long dmem_start;
> +	uint32_t fw_offset = type ? IMEM_2D_OFFSET : 0;
> +	uint32_t imem_start = (unsigned long)&_end + fw_offset;
> +	uint32_t dmem_start;
> +	uint32_t imem_len = IMEM_LEN, dmem_len = DMEM_LEN;
>  
>  #ifdef CONFIG_SPL_OF_CONTROL
>  	if (gd->fdt_blob && !fdt_check_header(gd->fdt_blob)) {
> @@ -43,11 +59,30 @@ void ddr_load_train_firmware(enum fw_type type)
>  	}
>  #endif
>  
> -	dmem_start = imem_start + IMEM_LEN;
> +	if (CONFIG_IS_ENABLED(BINMAN_SYMBOLS)) {
> +		switch (type) {
> +		case FW_1D_IMAGE:
> +			imem_start = binman_sym(ulong, blob_ext_1, image_pos);
> +			imem_len = binman_sym(ulong, blob_ext_1, size);
> +			dmem_start = binman_sym(ulong, blob_ext_2, image_pos);
> +			dmem_len = binman_sym(ulong, blob_ext_2, size);
> +			break;
> +		case FW_2D_IMAGE:
> +#if !IS_ENABLED(CONFIG_IMX8M_DDR3L)
> +			imem_start = binman_sym(ulong, blob_ext_3, image_pos);
> +			imem_len = binman_sym(ulong, blob_ext_3, size);
> +			dmem_start = binman_sym(ulong, blob_ext_4, image_pos);
> +			dmem_len = binman_sym(ulong, blob_ext_4, size);
> +#endif
> +			break;
> +		}
> +	}
> +
> +	dmem_start = imem_start + imem_len;

This overwrites the values from binman symbols, which looks wrong to me.
I think this line should appear before the if block.

>  
>  	pr_from32 = imem_start;
>  	pr_to32 = DDR_TRAIN_CODE_BASE_ADDR + 4 * IMEM_OFFSET_ADDR;
> -	for (i = 0x0; i < IMEM_LEN; ) {
> +	for (i = 0x0; i < imem_len; ) {
>  		tmp32 = readl(pr_from32);
>  		writew(tmp32 & 0x0000ffff, pr_to32);
>  		pr_to32 += 4;
> @@ -59,7 +94,7 @@ void ddr_load_train_firmware(enum fw_type type)
>  
>  	pr_from32 = dmem_start;
>  	pr_to32 = DDR_TRAIN_CODE_BASE_ADDR + 4 * DMEM_OFFSET_ADDR;
> -	for (i = 0x0; i < DMEM_LEN; ) {
> +	for (i = 0x0; i < dmem_len; ) {
>  		tmp32 = readl(pr_from32);
>  		writew(tmp32 & 0x0000ffff, pr_to32);
>  		pr_to32 += 4;
> @@ -72,7 +107,7 @@ void ddr_load_train_firmware(enum fw_type type)
>  	debug("check ddr_pmu_train_imem code\n");
>  	pr_from32 = imem_start;
>  	pr_to32 = DDR_TRAIN_CODE_BASE_ADDR + 4 * IMEM_OFFSET_ADDR;
> -	for (i = 0x0; i < IMEM_LEN; ) {
> +	for (i = 0x0; i < imem_len; ) {
>  		tmp32 = (readw(pr_to32) & 0x0000ffff);
>  		pr_to32 += 4;
>  		tmp32 += ((readw(pr_to32) & 0x0000ffff) << 16);
> @@ -93,7 +128,7 @@ void ddr_load_train_firmware(enum fw_type type)
>  	debug("check ddr4_pmu_train_dmem code\n");
>  	pr_from32 = dmem_start;
>  	pr_to32 = DDR_TRAIN_CODE_BASE_ADDR + 4 * DMEM_OFFSET_ADDR;
> -	for (i = 0x0; i < DMEM_LEN;) {
> +	for (i = 0x0; i < dmem_len;) {
>  		tmp32 = (readw(pr_to32) & 0x0000ffff);
>  		pr_to32 += 4;
>  		tmp32 += ((readw(pr_to32) & 0x0000ffff) << 16);

Re: [PATCH V4 8/8] binman_sym: guard with CONFIG_SPL_BINMAN_SYMBOLS

[Alper Nebi Yasak]

On 20/05/2022 17:10, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> There is case that CONFIG_BINMAN is defined, but
> CONFIG_SPL_BINMAN_SYMBOLS is not defined. In that case, there will be
> build failure. So use CONFIG_SPL_BINMAN_SYMBOLS to guard the macros, and
> define CONFIG_SPL_BINMAN_SYMBOLS in binman syms test.

I guess they should be CONFIG_IS_ENABLED(BINMAN_SYMBOLS) instead, as
there's also a CONFIG_TPL_BINMAN_SYMBOLS.

> 
> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  include/binman_sym.h                        | 2 +-
>  tools/binman/test/u_boot_binman_syms.c      | 1 +
>  tools/binman/test/u_boot_binman_syms_size.c | 1 +
>  3 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/binman_sym.h b/include/binman_sym.h
> index 72e6765fe52..548d8f5654c 100644
> --- a/include/binman_sym.h
> +++ b/include/binman_sym.h
> @@ -13,7 +13,7 @@
>  
>  #define BINMAN_SYM_MISSING	(-1UL)
>  
> -#ifdef CONFIG_BINMAN
> +#ifdef CONFIG_SPL_BINMAN_SYMBOLS
>  
>  /**
>   * binman_symname() - Internal function to get a binman symbol name
> diff --git a/tools/binman/test/u_boot_binman_syms.c b/tools/binman/test/u_boot_binman_syms.c
> index 37fc339ce84..f4a4d1f6846 100644
> --- a/tools/binman/test/u_boot_binman_syms.c
> +++ b/tools/binman/test/u_boot_binman_syms.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #define CONFIG_BINMAN
> +#define CONFIG_SPL_BINMAN_SYMBOLS
>  #include <binman_sym.h>
>  
>  binman_sym_declare(unsigned long, u_boot_spl_any, offset);
> diff --git a/tools/binman/test/u_boot_binman_syms_size.c b/tools/binman/test/u_boot_binman_syms_size.c
> index 7224bc1863c..3a01d8ca4be 100644
> --- a/tools/binman/test/u_boot_binman_syms_size.c
> +++ b/tools/binman/test/u_boot_binman_syms_size.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #define CONFIG_BINMAN
> +#define CONFIG_SPL_BINMAN_SYMBOLS
>  #include <binman_sym.h>
>  
>  binman_sym_declare(char, u_boot_spl, pos);

Re: [PATCH V4 1/8] spl: guard u_boot_any with X86

[Tom Rini]

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

On Sun, May 22, 2022 at 04:56:08PM +0300, Alper Nebi Yasak wrote:
> On 21/05/2022 15:05, Tom Rini wrote:
> > On Sat, May 21, 2022 at 08:33:56AM +0000, Peng Fan wrote:
> >>> Subject: Re: [PATCH V4 1/8] spl: guard u_boot_any with X86
> >>>
> >>> On Fri, May 20, 2022 at 10:10:40PM +0800, Peng Fan (OSS) wrote:
> >>>
> >>>> From: Peng Fan <peng.fan@nxp.com>
> >>>>
> >>>> set the symbol as weak not work if LTO is enabled. Since u_boot_any is
> >>>> only used on X86 for now, so guard it with X86, otherwise build break
> >>>> if we use BINMAN_SYMBOLS on i.MX.
> >>>>
> >>>> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> >>>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> >>>> ---
> >>>>  common/spl/spl.c     | 8 ++++++--
> >>>>  common/spl/spl_ram.c | 4 ++++
> >>>>  2 files changed, 10 insertions(+), 2 deletions(-)
> >>>
> >>> I think we long term need to figure this out and address it so LTO works.  But
> >>> for now can you please guard this with a test on LTO instead, so it's clear
> >>> where the problem is?
> >>
> >> Sorry, I could not get your point about guard with a test on LTO.
> >>
> >> Actually binman weak symbol will report a warning log if there is no u_boot_any
> >> binman symbol. Since only X86 use it, I guard with X86.
> > 
> > Why are you mentioning LTO in the commit message?  When I read the
> > commit message it sounds like you're saying the problem is that LTO
> > doesn't like how this symbol is handled, but if LTO was disabled,
> > everything would be fine.  If it's not LTO-related, please re-word the
> > message instead.
> 
> It looks like we should be able to change things in common/spl/spl.c to:
> 
>     #if CONFIG_IS_ENABLED(BINMAN_SYMBOLS)
>     /* See spl.h for information about this */
>     binman_sym_declare_optional(ulong, u_boot_any, image_pos);
>     binman_sym_declare_optional(ulong, u_boot_any, size);
>     #endif
> 
> which would mark the symbol as 'weak' and turn the error into a warning
> on the binman side. But that is somehow being undone by LTO.

So, looking at binman_sym_declare_optional we tell the linker that it's
weak and might even be unused.  LTO gets very aggressive about finding
things that aren't used in the resulting binary and discarding them.
Typically we have the problem of a function that is used but it's hard
for LTO to see it, so we give it the "used" attribute.  But for
something we're already saying is "unused" this would be wrong.  So why
do we mark things as unused here?  I assume it's marked weak as it could
be overridden at link time by a definition elsewhere.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

RE: [PATCH V4 1/8] spl: guard u_boot_any with X86

[Peng Fan (OSS)]

> Subject: Re: [PATCH V4 1/8] spl: guard u_boot_any with X86
> 
> On 21/05/2022 15:05, Tom Rini wrote:
> > On Sat, May 21, 2022 at 08:33:56AM +0000, Peng Fan wrote:
> >>> Subject: Re: [PATCH V4 1/8] spl: guard u_boot_any with X86
> >>>
> >>> On Fri, May 20, 2022 at 10:10:40PM +0800, Peng Fan (OSS) wrote:
> >>>
> >>>> From: Peng Fan <peng.fan@nxp.com>
> >>>>
> >>>> set the symbol as weak not work if LTO is enabled. Since u_boot_any
> >>>> is only used on X86 for now, so guard it with X86, otherwise build
> >>>> break if we use BINMAN_SYMBOLS on i.MX.
> >>>>
> >>>> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> >>>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> >>>> ---
> >>>>  common/spl/spl.c     | 8 ++++++--
> >>>>  common/spl/spl_ram.c | 4 ++++
> >>>>  2 files changed, 10 insertions(+), 2 deletions(-)
> >>>
> >>> I think we long term need to figure this out and address it so LTO
> >>> works.  But for now can you please guard this with a test on LTO
> >>> instead, so it's clear where the problem is?
> >>
> >> Sorry, I could not get your point about guard with a test on LTO.
> >>
> >> Actually binman weak symbol will report a warning log if there is no
> >> u_boot_any binman symbol. Since only X86 use it, I guard with X86.
> >
> > Why are you mentioning LTO in the commit message?  When I read the
> > commit message it sounds like you're saying the problem is that LTO
> > doesn't like how this symbol is handled, but if LTO was disabled,
> > everything would be fine.  If it's not LTO-related, please re-word the
> > message instead.
> 
> It looks like we should be able to change things in common/spl/spl.c to:
> 
>     #if CONFIG_IS_ENABLED(BINMAN_SYMBOLS)
>     /* See spl.h for information about this */
>     binman_sym_declare_optional(ulong, u_boot_any, image_pos);
>     binman_sym_declare_optional(ulong, u_boot_any, size);
>     #endif
> 
> which would mark the symbol as 'weak' and turn the error into a warning on
> the binman side. But that is somehow being undone by LTO.
> 
> I'm trying to build for imx8mm-beacon with that change instead of this patch.
> With CONFIG_LTO=y, build fails and spl/u-boot-spl.sym has:
> 
> > 00000000007fbe28 l     O .binman_sym	0000000000000008
> _binman_u_boot_any_prop_image_pos
> 
> Looks like the size symbol is optimized out. With CONFIG_LTO unset, the build
> succeeds and the same file has:
> 
> > 00000000007fe90c  w    O .binman_sym	0000000000000008
> _binman_u_boot_any_prop_image_pos
> > 00000000007fe904  w    O .binman_sym	0000000000000008
> _binman_u_boot_any_prop_size
> 
> I don't know much about linking stuff, so this is as deep as I could dig...

Thanks for the detailed sharing. Yes, this is the issue I try to work around. Some
i.MX boards has LTO set, this leads me to use X86 as an extra guard.

Thanks,
Peng.

RE: [PATCH V4 1/8] spl: guard u_boot_any with X86

[Peng Fan (OSS)]

> Subject: Re: [PATCH V4 1/8] spl: guard u_boot_any with X86
> 
> On Sat, May 21, 2022 at 08:33:56AM +0000, Peng Fan wrote:
> > > Subject: Re: [PATCH V4 1/8] spl: guard u_boot_any with X86
> > >
> > > On Fri, May 20, 2022 at 10:10:40PM +0800, Peng Fan (OSS) wrote:
> > >
> > > > From: Peng Fan <peng.fan@nxp.com>
> > > >
> > > > set the symbol as weak not work if LTO is enabled. Since
> > > > u_boot_any is only used on X86 for now, so guard it with X86,
> > > > otherwise build break if we use BINMAN_SYMBOLS on i.MX.
> > > >
> > > > Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > ---
> > > >  common/spl/spl.c     | 8 ++++++--
> > > >  common/spl/spl_ram.c | 4 ++++
> > > >  2 files changed, 10 insertions(+), 2 deletions(-)
> > >
> > > I think we long term need to figure this out and address it so LTO
> > > works.  But for now can you please guard this with a test on LTO
> > > instead, so it's clear where the problem is?
> >
> > Sorry, I could not get your point about guard with a test on LTO.
> >
> > Actually binman weak symbol will report a warning log if there is no
> > u_boot_any binman symbol. Since only X86 use it, I guard with X86.
> 
> Why are you mentioning LTO in the commit message?  When I read the
> commit message it sounds like you're saying the problem is that LTO doesn't
> like how this symbol is handled, but if LTO was disabled, everything would be
> fine.  If it's not LTO-related, please re-word the message instead.

Sorry, I could reword the commit message, but currently I have no better
idea to address the issue unless use X86 as a guard in the code as this
patch does. If you agree the code in this patch, I could reword commit msg
and send v5.

Thanks,
Peng.

> 
> --
> Tom

RE: [PATCH V4 2/8] arm: dts: imx8m: update binman ddr firmware node name

[Peng Fan (OSS)]

> Subject: Re: [PATCH V4 2/8] arm: dts: imx8m: update binman ddr firmware
> node name
> 
> On 20/05/2022 17:10, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > We are migrating to use BINMAN SYMBOLS, the current name is not a
> > valid binman type, so update to use blob-ext@[1,2,3,4].
> >
> > Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  arch/arm/dts/imx8mm-u-boot.dtsi                   | 8 ++++----
> >  arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi | 4 ++--
> > arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi  | 8 ++++----
> >  arch/arm/dts/imx8mn-venice-u-boot.dtsi            | 8 ++++----
> >  arch/arm/dts/imx8mq-u-boot.dtsi                   | 8 ++++----
> >  5 files changed, 18 insertions(+), 18 deletions(-)
> 
> I think descriptive entry names are better in general, so I prefer the old names
> to a bunch of 'blob-ext's. Symbol resolution is done using the entry names
> anyway, I don't see why this change would be necessary.

Binman understand blob-ext, it not understand 1d-imem. If keep id-imem, or else,
need to add several new binman types which I would not prefer.

Thanks,
Peng.

> 
> In any case, the names and labels should be consistent across the different dts
> files, assuming the blobs have the same purpose / meaning.
> I see that labels starting with digits cause a dtc parse error, so the entries could
> be `imem_1d: imem-1d { ... };` and so on.
> 
> Also, the blobs essentially look the same to me aside from their filenames, just
> duplicated over different imx8m* variants and boards.
> Maybe binman parts could be unified into a single dtsi file later on?
> (Filename differences can be handled by new 'blob_named_by_arg's btw.)
> 
> > diff --git a/arch/arm/dts/imx8mm-u-boot.dtsi
> > b/arch/arm/dts/imx8mm-u-boot.dtsi index 9f66cdb65a9..5de55a2d80b
> > 100644
> > --- a/arch/arm/dts/imx8mm-u-boot.dtsi
> > +++ b/arch/arm/dts/imx8mm-u-boot.dtsi
> > @@ -39,25 +39,25 @@
> >  			filename = "u-boot-spl.bin";
> >  		};
> >
> > -		1d-imem {
> > +		imem_1d: blob-ext@1 {
> >  			filename = "lpddr4_pmu_train_1d_imem.bin";
> >  			size = <0x8000>;
> >  			type = "blob-ext";
> >  		};
> >
> > -		1d-dmem {
> > +		dmem_1d: blob-ext@2 {
> >  			filename = "lpddr4_pmu_train_1d_dmem.bin";
> >  			size = <0x4000>;
> >  			type = "blob-ext";
> >  		};
> >
> > -		2d-imem {
> > +		imem_2d: blob-ext@3 {
> >  			filename = "lpddr4_pmu_train_2d_imem.bin";
> >  			size = <0x8000>;
> >  			type = "blob-ext";
> >  		};
> >
> > -		2d-dmem {
> > +		dmem_2d: blob-ext@4 {
> >  			filename = "lpddr4_pmu_train_2d_dmem.bin";
> >  			size = <0x4000>;
> >  			type = "blob-ext";
> > diff --git a/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi
> > b/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi
> > index 46a9d7fd78b..5a52b73d7e9 100644
> > --- a/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi
> > +++ b/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi
> > @@ -111,13 +111,13 @@
> >  			filename = "u-boot-spl.bin";
> >  		};
> >
> > -		1d-imem {
> > +		imem_1d: blob-ext@1 {
> >  			filename = "ddr3_imem_1d.bin";
> >  			size = <0x8000>;
> >  			type = "blob-ext";
> >  		};
> >
> > -		1d_dmem {
> > +		dmem_1d: blob-ext@2 {
> >  			filename = "ddr3_dmem_1d.bin";
> >  			size = <0x4000>;
> >  			type = "blob-ext";
> > diff --git a/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
> > b/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
> > index 6e37622cca7..001e725f568 100644
> > --- a/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
> > +++ b/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
> > @@ -130,25 +130,25 @@
> >  			filename = "u-boot-spl.bin";
> >  		};
> >
> > -		1d-imem {
> > +		blob_1: blob-ext@1 {
> >  			filename = "ddr4_imem_1d.bin";
> >  			size = <0x8000>;
> >  			type = "blob-ext";
> >  		};
> >
> > -		1d_dmem {
> > +		blob_2: blob-ext@2 {
> >  			filename = "ddr4_dmem_1d.bin";
> >  			size = <0x4000>;
> >  			type = "blob-ext";
> >  		};
> >
> > -		2d_imem {
> > +		blob_3: blob-ext@3 {
> >  			filename = "ddr4_imem_2d.bin";
> >  			size = <0x8000>;
> >  			type = "blob-ext";
> >  		};
> >
> > -		2d_dmem {
> > +		blob_4: blob-ext@4 {
> >  			filename = "ddr4_dmem_2d.bin";
> >  			size = <0x4000>;
> >  			type = "blob-ext";
> > diff --git a/arch/arm/dts/imx8mn-venice-u-boot.dtsi
> > b/arch/arm/dts/imx8mn-venice-u-boot.dtsi
> > index 35819553879..67922146963 100644
> > --- a/arch/arm/dts/imx8mn-venice-u-boot.dtsi
> > +++ b/arch/arm/dts/imx8mn-venice-u-boot.dtsi
> > @@ -126,25 +126,25 @@
> >  			filename = "u-boot-spl.bin";
> >  		};
> >
> > -		1d-imem {
> > +		imem_1d: blob-ext@1 {
> >  			filename = "lpddr4_pmu_train_1d_imem.bin";
> >  			size = <0x8000>;
> >  			type = "blob-ext";
> >  		};
> >
> > -		1d_dmem {
> > +		dmem_1d: blob-ext@2 {
> >  			filename = "lpddr4_pmu_train_1d_dmem.bin";
> >  			size = <0x4000>;
> >  			type = "blob-ext";
> >  		};
> >
> > -		2d_imem {
> > +		imem_2d: blob-ext@3 {
> >  			filename = "lpddr4_pmu_train_2d_imem.bin";
> >  			size = <0x8000>;
> >  			type = "blob-ext";
> >  		};
> >
> > -		2d_dmem {
> > +		dmem_2d: blob-ext@4 {
> >  			filename = "lpddr4_pmu_train_2d_dmem.bin";
> >  			size = <0x4000>;
> >  			type = "blob-ext";
> > diff --git a/arch/arm/dts/imx8mq-u-boot.dtsi
> > b/arch/arm/dts/imx8mq-u-boot.dtsi index 912a3d4a356..389414ad26f
> > 100644
> > --- a/arch/arm/dts/imx8mq-u-boot.dtsi
> > +++ b/arch/arm/dts/imx8mq-u-boot.dtsi
> > @@ -46,25 +46,25 @@
> >  			filename = "u-boot-spl.bin";
> >  		};
> >
> > -		1d-imem {
> > +		imem_1d: blob-ext@1 {
> >  			filename = "lpddr4_pmu_train_1d_imem.bin";
> >  			size = <0x8000>;
> >  			type = "blob-ext";
> >  		};
> >
> > -		1d-dmem {
> > +		dmem_1d: blob-ext@2 {
> >  			filename = "lpddr4_pmu_train_1d_dmem.bin";
> >  			size = <0x4000>;
> >  			type = "blob-ext";
> >  		};
> >
> > -		2d-imem {
> > +		imem_2d: blob-ext@3 {
> >  			filename = "lpddr4_pmu_train_2d_imem.bin";
> >  			size = <0x8000>;
> >  			type = "blob-ext";
> >  		};
> >
> > -		2d-dmem {
> > +		dmem_2d: blob-ext@4 {
> >  			filename = "lpddr4_pmu_train_2d_dmem.bin";
> >  			size = <0x4000>;
> >  			type = "blob-ext";

RE: [PATCH V4 3/8] imx: imx8mm-icore: migrate to use BINMAN

[Peng Fan (OSS)]

> Subject: Re: [PATCH V4 3/8] imx: imx8mm-icore: migrate to use BINMAN
> 
> On 20/05/2022 17:10, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Use BINMAN instead of imx specific packing method.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  arch/arm/mach-imx/imx8m/Kconfig                 |  1 +
> >  arch/arm/mach-imx/imx8m/imximage-8mm-lpddr4.cfg | 10 +---------
> 
> Maybe this could be done for other imximage*.cfg files as well?

Yes, there are follow up patches to clean up the cfg files, this patch
is for binman and avoid build break for these two boards.

Thanks,
Peng.

> 
> >  configs/imx8mm-icore-mx8mm-ctouch2_defconfig    |  2 +-
> >  configs/imx8mm-icore-mx8mm-edimm2.2_defconfig   |  2 +-
> >  4 files changed, 4 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/arm/mach-imx/imx8m/Kconfig
> > b/arch/arm/mach-imx/imx8m/Kconfig index 24299ae037f..75ad7aab081
> > 100644
> > --- a/arch/arm/mach-imx/imx8m/Kconfig
> > +++ b/arch/arm/mach-imx/imx8m/Kconfig
> > @@ -68,6 +68,7 @@ config TARGET_IMX8MM_EVK
> >
> >  config TARGET_IMX8MM_ICORE_MX8MM
> >  	bool "Engicam i.Core MX8M Mini SOM"
> > +	select BINMAN
> >  	select IMX8MM
> >  	select SUPPORT_SPL
> >  	select IMX8M_LPDDR4
> > diff --git a/arch/arm/mach-imx/imx8m/imximage-8mm-lpddr4.cfg
> > b/arch/arm/mach-imx/imx8m/imximage-8mm-lpddr4.cfg
> > index e06d53ef417..5dcb8ae72f0 100644
> > --- a/arch/arm/mach-imx/imx8m/imximage-8mm-lpddr4.cfg
> > +++ b/arch/arm/mach-imx/imx8m/imximage-8mm-lpddr4.cfg
> > @@ -3,13 +3,5 @@
> >   * Copyright 2019 NXP
> >   */
> >
> > -
> > -FIT
> >  BOOT_FROM	sd
> > -LOADER		spl/u-boot-spl-ddr.bin	0x7E1000
> > -SECOND_LOADER	u-boot.itb		0x40200000 0x60000
> > -
> > -DDR_FW lpddr4_pmu_train_1d_imem.bin
> > -DDR_FW lpddr4_pmu_train_1d_dmem.bin
> > -DDR_FW lpddr4_pmu_train_2d_imem.bin
> > -DDR_FW lpddr4_pmu_train_2d_dmem.bin
> > +LOADER		u-boot-spl-ddr.bin	0x7E1000
> > diff --git a/configs/imx8mm-icore-mx8mm-ctouch2_defconfig
> > b/configs/imx8mm-icore-mx8mm-ctouch2_defconfig
> > index d95a74a7237..dcb12e5d026 100644
> > --- a/configs/imx8mm-icore-mx8mm-ctouch2_defconfig
> > +++ b/configs/imx8mm-icore-mx8mm-ctouch2_defconfig
> > @@ -20,7 +20,7 @@ CONFIG_DISTRO_DEFAULTS=y  CONFIG_FIT=y
> >  CONFIG_FIT_EXTERNAL_OFFSET=0x3000
> >  CONFIG_SPL_LOAD_FIT=y
> > -CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh"
> > +# CONFIG_USE_SPL_FIT_GENERATOR is not set
> >  CONFIG_OF_SYSTEM_SETUP=y
> >  CONFIG_DEFAULT_FDT_FILE="imx8mm-icore-mx8mm-ctouch2.dtb"
> >  CONFIG_SPL_BOARD_INIT=y
> > diff --git a/configs/imx8mm-icore-mx8mm-edimm2.2_defconfig
> > b/configs/imx8mm-icore-mx8mm-edimm2.2_defconfig
> > index 43c697a39d8..22acf7317b4 100644
> > --- a/configs/imx8mm-icore-mx8mm-edimm2.2_defconfig
> > +++ b/configs/imx8mm-icore-mx8mm-edimm2.2_defconfig
> > @@ -20,7 +20,7 @@ CONFIG_DISTRO_DEFAULTS=y  CONFIG_FIT=y
> >  CONFIG_FIT_EXTERNAL_OFFSET=0x3000
> >  CONFIG_SPL_LOAD_FIT=y
> > -CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh"
> > +# CONFIG_USE_SPL_FIT_GENERATOR is not set
> >  CONFIG_OF_SYSTEM_SETUP=y
> >  CONFIG_DEFAULT_FDT_FILE="imx8mm-icore-mx8mm-edimm2.2.dtb"
> >  CONFIG_SPL_BOARD_INIT=y

RE: [PATCH V4 5/8] tools: binman: section: replace @ with -

[Peng Fan (OSS)]

> Subject: Re: [PATCH V4 5/8] tools: binman: section: replace @ with -
> 
> On 20/05/2022 17:10, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > In arch/arm/dts/imx8mp-u-boot.dtsi, there are blob-ext@1, blob-ext@2
> > and etc which is for packing ddr phy firmware. However we could not
> > declare symbol name such as 'binman_sym_declare(ulong, blob_ext@1,
> > image_pos)', because '@' is not allowed, so we choose to declare the
> > symbol 'binman_sym_declare(ulong, blob_ext_1, image_pos);' with '@'
> > replaced with '_'. It does not impact if there is no '@' in section name.
> >
> > Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> > Reviewed-by: Tom Rini <trini@konsulko.com>
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  tools/binman/etype/section.py | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> This shouldn't be necessary if you keep the old names for the binman entries
> and use `binman_sym_declare(ulong, imem_1d, image_pos);` etc.
> for the symbols.

No, as I replied in the other mail, imem_1d is not a binman type.

> 
> > diff --git a/tools/binman/etype/section.py
> > b/tools/binman/etype/section.py index bd67238b919..e3f362b442b 100644
> > --- a/tools/binman/etype/section.py
> > +++ b/tools/binman/etype/section.py
> > @@ -875,7 +875,7 @@ class Entry_section(Entry):
> >                  entries[entry.GetPath()] = entry
> >              for entry in to_add.values():
> >                  self._CollectEntries(entries, entries_by_name, entry)
> > -        entries_by_name[add_entry.name] = add_entry
> > +        entries_by_name[add_entry.name.replace('@', '-')] = add_entry
> 
> The correct place to do this would be LookupSymbol() in
> binman/etype/section.py, but I'm not convinced this should be done at all. I'd
> say if an entry is important enough to have a symbol for it, it should have a
> unique, descriptive, non-@ name.

Since blob_ext@[1,2,3] is used, this is to replace and generate symbols as
blob_ext_[1,2,3]

Thanks,
Peng.
> 
> >
> >      def MissingArgs(self, entry, missing):
> >          """Report a missing argument, if enabled

RE: [PATCH V4 6/8] ddr: imx8m: helper: load ddr firmware according to binman symbols

[Peng Fan (OSS)]

> Subject: Re: [PATCH V4 6/8] ddr: imx8m: helper: load ddr firmware according
> to binman symbols
> 
> On 20/05/2022 17:10, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > By reading binman symbols, we no need hard coded IMEM_LEN/DMEM_LEN
> > after we update the binman dtsi to drop 0x8000/0x4000 length for the
> firmware.
> >
> > And that could save binary size for many KBs.
> >
> > Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/ddr/imx/imx8m/helper.c | 51
> > ++++++++++++++++++++++++++++------
> >  1 file changed, 43 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/ddr/imx/imx8m/helper.c
> > b/drivers/ddr/imx/imx8m/helper.c index f23904bf712..b3bd57531b7
> 100644
> > --- a/drivers/ddr/imx/imx8m/helper.c
> > +++ b/drivers/ddr/imx/imx8m/helper.c
> > @@ -4,6 +4,7 @@
> >   */
> >
> >  #include <common.h>
> > +#include <binman_sym.h>
> >  #include <log.h>
> >  #include <spl.h>
> >  #include <asm/global_data.h>
> > @@ -25,15 +26,30 @@ DECLARE_GLOBAL_DATA_PTR;  #define
> DMEM_OFFSET_ADDR
> > 0x00054000  #define DDR_TRAIN_CODE_BASE_ADDR
> > IP2APB_DDRPHY_IPS_BASE_ADDR(0)
> >
> > +binman_sym_declare(ulong, blob_ext_1, image_pos);
> > +binman_sym_declare(ulong, blob_ext_1, size);
> > +
> > +binman_sym_declare(ulong, blob_ext_2, image_pos);
> > +binman_sym_declare(ulong, blob_ext_2, size);
> > +
> > +#if !IS_ENABLED(CONFIG_IMX8M_DDR3L)
> > +binman_sym_declare(ulong, blob_ext_3, image_pos);
> > +binman_sym_declare(ulong, blob_ext_3, size);
> > +
> > +binman_sym_declare(ulong, blob_ext_4, image_pos);
> > +binman_sym_declare(ulong, blob_ext_4, size); #endif
> > +
> 
> (Descriptive entry names would make these more meaningful.)
> 
> >  /* We need PHY iMEM PHY is 32KB padded */  void
> > ddr_load_train_firmware(enum fw_type type)  {
> >  	u32 tmp32, i;
> >  	u32 error = 0;
> >  	unsigned long pr_to32, pr_from32;
> > -	unsigned long fw_offset = type ? IMEM_2D_OFFSET : 0;
> > -	unsigned long imem_start = (unsigned long)&_end + fw_offset;
> > -	unsigned long dmem_start;
> > +	uint32_t fw_offset = type ? IMEM_2D_OFFSET : 0;
> > +	uint32_t imem_start = (unsigned long)&_end + fw_offset;
> > +	uint32_t dmem_start;
> > +	uint32_t imem_len = IMEM_LEN, dmem_len = DMEM_LEN;
> >
> >  #ifdef CONFIG_SPL_OF_CONTROL
> >  	if (gd->fdt_blob && !fdt_check_header(gd->fdt_blob)) { @@ -43,11
> > +59,30 @@ void ddr_load_train_firmware(enum fw_type type)
> >  	}
> >  #endif
> >
> > -	dmem_start = imem_start + IMEM_LEN;
> > +	if (CONFIG_IS_ENABLED(BINMAN_SYMBOLS)) {
> > +		switch (type) {
> > +		case FW_1D_IMAGE:
> > +			imem_start = binman_sym(ulong, blob_ext_1, image_pos);
> > +			imem_len = binman_sym(ulong, blob_ext_1, size);
> > +			dmem_start = binman_sym(ulong, blob_ext_2, image_pos);
> > +			dmem_len = binman_sym(ulong, blob_ext_2, size);
> > +			break;
> > +		case FW_2D_IMAGE:
> > +#if !IS_ENABLED(CONFIG_IMX8M_DDR3L)
> > +			imem_start = binman_sym(ulong, blob_ext_3, image_pos);
> > +			imem_len = binman_sym(ulong, blob_ext_3, size);
> > +			dmem_start = binman_sym(ulong, blob_ext_4, image_pos);
> > +			dmem_len = binman_sym(ulong, blob_ext_4, size); #endif
> > +			break;
> > +		}
> > +	}
> > +
> > +	dmem_start = imem_start + imem_len;
> 
> This overwrites the values from binman symbols, which looks wrong to me.
> I think this line should appear before the if block.

It does not matter actually. Put it before if block is ok.

Regards,
Peng.

> 
> >
> >  	pr_from32 = imem_start;
> >  	pr_to32 = DDR_TRAIN_CODE_BASE_ADDR + 4 * IMEM_OFFSET_ADDR;
> > -	for (i = 0x0; i < IMEM_LEN; ) {
> > +	for (i = 0x0; i < imem_len; ) {
> >  		tmp32 = readl(pr_from32);
> >  		writew(tmp32 & 0x0000ffff, pr_to32);
> >  		pr_to32 += 4;
> > @@ -59,7 +94,7 @@ void ddr_load_train_firmware(enum fw_type type)
> >
> >  	pr_from32 = dmem_start;
> >  	pr_to32 = DDR_TRAIN_CODE_BASE_ADDR + 4 * DMEM_OFFSET_ADDR;
> > -	for (i = 0x0; i < DMEM_LEN; ) {
> > +	for (i = 0x0; i < dmem_len; ) {
> >  		tmp32 = readl(pr_from32);
> >  		writew(tmp32 & 0x0000ffff, pr_to32);
> >  		pr_to32 += 4;
> > @@ -72,7 +107,7 @@ void ddr_load_train_firmware(enum fw_type type)
> >  	debug("check ddr_pmu_train_imem code\n");
> >  	pr_from32 = imem_start;
> >  	pr_to32 = DDR_TRAIN_CODE_BASE_ADDR + 4 * IMEM_OFFSET_ADDR;
> > -	for (i = 0x0; i < IMEM_LEN; ) {
> > +	for (i = 0x0; i < imem_len; ) {
> >  		tmp32 = (readw(pr_to32) & 0x0000ffff);
> >  		pr_to32 += 4;
> >  		tmp32 += ((readw(pr_to32) & 0x0000ffff) << 16); @@ -93,7 +128,7
> @@
> > void ddr_load_train_firmware(enum fw_type type)
> >  	debug("check ddr4_pmu_train_dmem code\n");
> >  	pr_from32 = dmem_start;
> >  	pr_to32 = DDR_TRAIN_CODE_BASE_ADDR + 4 * DMEM_OFFSET_ADDR;
> > -	for (i = 0x0; i < DMEM_LEN;) {
> > +	for (i = 0x0; i < dmem_len;) {
> >  		tmp32 = (readw(pr_to32) & 0x0000ffff);
> >  		pr_to32 += 4;
> >  		tmp32 += ((readw(pr_to32) & 0x0000ffff) << 16);

RE: [PATCH V4 8/8] binman_sym: guard with CONFIG_SPL_BINMAN_SYMBOLS

[Peng Fan (OSS)]

> Subject: Re: [PATCH V4 8/8] binman_sym: guard with
> CONFIG_SPL_BINMAN_SYMBOLS
> 
> On 20/05/2022 17:10, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > There is case that CONFIG_BINMAN is defined, but
> > CONFIG_SPL_BINMAN_SYMBOLS is not defined. In that case, there will be
> > build failure. So use CONFIG_SPL_BINMAN_SYMBOLS to guard the macros,
> > and define CONFIG_SPL_BINMAN_SYMBOLS in binman syms test.
> 
> I guess they should be CONFIG_IS_ENABLED(BINMAN_SYMBOLS) instead, as
> there's also a CONFIG_TPL_BINMAN_SYMBOLS.

No. CONFIG_IS_ENABLED not work, because there is no defconfig generated.

Regards,
Peng.

> 
> >
> > Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  include/binman_sym.h                        | 2 +-
> >  tools/binman/test/u_boot_binman_syms.c      | 1 +
> >  tools/binman/test/u_boot_binman_syms_size.c | 1 +
> >  3 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/binman_sym.h b/include/binman_sym.h index
> > 72e6765fe52..548d8f5654c 100644
> > --- a/include/binman_sym.h
> > +++ b/include/binman_sym.h
> > @@ -13,7 +13,7 @@
> >
> >  #define BINMAN_SYM_MISSING	(-1UL)
> >
> > -#ifdef CONFIG_BINMAN
> > +#ifdef CONFIG_SPL_BINMAN_SYMBOLS
> >
> >  /**
> >   * binman_symname() - Internal function to get a binman symbol name
> > diff --git a/tools/binman/test/u_boot_binman_syms.c
> > b/tools/binman/test/u_boot_binman_syms.c
> > index 37fc339ce84..f4a4d1f6846 100644
> > --- a/tools/binman/test/u_boot_binman_syms.c
> > +++ b/tools/binman/test/u_boot_binman_syms.c
> > @@ -6,6 +6,7 @@
> >   */
> >
> >  #define CONFIG_BINMAN
> > +#define CONFIG_SPL_BINMAN_SYMBOLS
> >  #include <binman_sym.h>
> >
> >  binman_sym_declare(unsigned long, u_boot_spl_any, offset); diff --git
> > a/tools/binman/test/u_boot_binman_syms_size.c
> > b/tools/binman/test/u_boot_binman_syms_size.c
> > index 7224bc1863c..3a01d8ca4be 100644
> > --- a/tools/binman/test/u_boot_binman_syms_size.c
> > +++ b/tools/binman/test/u_boot_binman_syms_size.c
> > @@ -6,6 +6,7 @@
> >   */
> >
> >  #define CONFIG_BINMAN
> > +#define CONFIG_SPL_BINMAN_SYMBOLS
> >  #include <binman_sym.h>
> >
> >  binman_sym_declare(char, u_boot_spl, pos);

Re: [PATCH V4 1/8] spl: guard u_boot_any with X86

[Tom Rini]

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

On Mon, May 23, 2022 at 06:28:44AM +0000, Peng Fan (OSS) wrote:
> > Subject: Re: [PATCH V4 1/8] spl: guard u_boot_any with X86
> > 
> > On Sat, May 21, 2022 at 08:33:56AM +0000, Peng Fan wrote:
> > > > Subject: Re: [PATCH V4 1/8] spl: guard u_boot_any with X86
> > > >
> > > > On Fri, May 20, 2022 at 10:10:40PM +0800, Peng Fan (OSS) wrote:
> > > >
> > > > > From: Peng Fan <peng.fan@nxp.com>
> > > > >
> > > > > set the symbol as weak not work if LTO is enabled. Since
> > > > > u_boot_any is only used on X86 for now, so guard it with X86,
> > > > > otherwise build break if we use BINMAN_SYMBOLS on i.MX.
> > > > >
> > > > > Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > > ---
> > > > >  common/spl/spl.c     | 8 ++++++--
> > > > >  common/spl/spl_ram.c | 4 ++++
> > > > >  2 files changed, 10 insertions(+), 2 deletions(-)
> > > >
> > > > I think we long term need to figure this out and address it so LTO
> > > > works.  But for now can you please guard this with a test on LTO
> > > > instead, so it's clear where the problem is?
> > >
> > > Sorry, I could not get your point about guard with a test on LTO.
> > >
> > > Actually binman weak symbol will report a warning log if there is no
> > > u_boot_any binman symbol. Since only X86 use it, I guard with X86.
> > 
> > Why are you mentioning LTO in the commit message?  When I read the
> > commit message it sounds like you're saying the problem is that LTO doesn't
> > like how this symbol is handled, but if LTO was disabled, everything would be
> > fine.  If it's not LTO-related, please re-word the message instead.
> 
> Sorry, I could reword the commit message, but currently I have no better
> idea to address the issue unless use X86 as a guard in the code as this
> patch does. If you agree the code in this patch, I could reword commit msg
> and send v5.

Well, lets see what Alper says in the other part of the thread.  I'd
really like to solve this not work around this.  But I'll take
documenting the problem for the person that has X86 && LTO as good
enough for the moment.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH V4 1/8] spl: guard u_boot_any with X86

[Alper Nebi Yasak]

On 22/05/2022 17:50, Tom Rini wrote:
> On Sun, May 22, 2022 at 04:56:08PM +0300, Alper Nebi Yasak wrote:
>> It looks like we should be able to change things in common/spl/spl.c to:
>>
>>     #if CONFIG_IS_ENABLED(BINMAN_SYMBOLS)
>>     /* See spl.h for information about this */
>>     binman_sym_declare_optional(ulong, u_boot_any, image_pos);
>>     binman_sym_declare_optional(ulong, u_boot_any, size);
>>     #endif
>>
>> which would mark the symbol as 'weak' and turn the error into a warning
>> on the binman side. But that is somehow being undone by LTO.
> 
> So, looking at binman_sym_declare_optional we tell the linker that it's
> weak and might even be unused.  LTO gets very aggressive about finding
> things that aren't used in the resulting binary and discarding them.
> Typically we have the problem of a function that is used but it's hard
> for LTO to see it, so we give it the "used" attribute.  But for
> something we're already saying is "unused" this would be wrong.  So why
> do we mark things as unused here?  I assume it's marked weak as it could
> be overridden at link time by a definition elsewhere.

I don't know, but the GCC manual says marking things 'unused' will
suppress a warning if the variable is actually unused. I guess binman
symbols may become unused due to some other config options being unset,
and it's done for those cases?

I think the optional symbols are marked weak just to signal the python
side that they are optional. If I understand it right, binman:

1. Packs an image using the finalized 'spl/u-boot-spl.bin'
2. Figures out the image-pos and size values of every entry
3. Inspects the 'spl/u-boot-spl' ELF file for declared binman symbols
4. Raises an error if any non-'weak' symbol's value is undetermined
5. Updates the u-boot-spl.bin (in memory) with the calculated values
6. Recreates the final image based on that updated bin file.

Re: [PATCH V4 1/8] spl: guard u_boot_any with X86

[Alper Nebi Yasak]

On 23/05/2022 17:10, Tom Rini wrote:
> On Mon, May 23, 2022 at 06:28:44AM +0000, Peng Fan (OSS) wrote:
>>> Subject: Re: [PATCH V4 1/8] spl: guard u_boot_any with X86
>>>
>>> Why are you mentioning LTO in the commit message?  When I read the
>>> commit message it sounds like you're saying the problem is that LTO doesn't
>>> like how this symbol is handled, but if LTO was disabled, everything would be
>>> fine.  If it's not LTO-related, please re-word the message instead.
>>
>> Sorry, I could reword the commit message, but currently I have no better
>> idea to address the issue unless use X86 as a guard in the code as this
>> patch does. If you agree the code in this patch, I could reword commit msg
>> and send v5.
> 
> Well, lets see what Alper says in the other part of the thread.  I'd
> really like to solve this not work around this.  But I'll take
> documenting the problem for the person that has X86 && LTO as good
> enough for the moment.

Alternatively, I think we can decide that all binman symbols are
optional, and not raise an error when we can't find a value for them.
They are de-facto optional anyway: people can use the non-binman-image
build artifacts (which binman doesn't update wrt. symbols), thus the
code that uses binman symbols should assume their values can be missing.

Re: [PATCH V4 2/8] arm: dts: imx8m: update binman ddr firmware node name

[Alper Nebi Yasak]

On 23/05/2022 10:01, Peng Fan (OSS) wrote:
>> Subject: Re: [PATCH V4 2/8] arm: dts: imx8m: update binman ddr firmware
>> node name
>>
>> On 20/05/2022 17:10, Peng Fan (OSS) wrote:
>>> From: Peng Fan <peng.fan@nxp.com>
>>>
>>> We are migrating to use BINMAN SYMBOLS, the current name is not a
>>> valid binman type, so update to use blob-ext@[1,2,3,4].
>>>
>>> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
>>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>>> ---
>>>  arch/arm/dts/imx8mm-u-boot.dtsi                   | 8 ++++----
>>>  arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi | 4 ++--
>>> arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi  | 8 ++++----
>>>  arch/arm/dts/imx8mn-venice-u-boot.dtsi            | 8 ++++----
>>>  arch/arm/dts/imx8mq-u-boot.dtsi                   | 8 ++++----
>>>  5 files changed, 18 insertions(+), 18 deletions(-)
>>
>> I think descriptive entry names are better in general, so I prefer the old names
>> to a bunch of 'blob-ext's. Symbol resolution is done using the entry names
>> anyway, I don't see why this change would be necessary.
> 
> Binman understand blob-ext, it not understand 1d-imem. If keep id-imem, or else,
> need to add several new binman types which I would not prefer.

It would work as long as you set `type = "blob-ext";` in the entry block.

Please test this diff on top of your series:

diff --git a/arch/arm/dts/imx8mm-u-boot.dtsi b/arch/arm/dts/imx8mm-u-boot.dtsi
index 19a2da30f512..8c48678625d9 100644
--- a/arch/arm/dts/imx8mm-u-boot.dtsi
+++ b/arch/arm/dts/imx8mm-u-boot.dtsi
@@ -39,25 +39,25 @@
 			filename = "u-boot-spl.bin";
 		};
 
-		imem_1d: blob-ext@1 {
+		ddr-1d-imem-fw {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
 			align-end = <4>;
 			type = "blob-ext";
 		};
 
-		dmem_1d: blob-ext@2 {
+		ddr-1d-dmem-fw {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
 			align-end = <4>;
 			type = "blob-ext";
 		};
 
-		imem_2d: blob-ext@3 {
+		ddr-2d-imem-fw {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
 			align-end = <4>;
 			type = "blob-ext";
 		};
 
-		dmem_2d: blob-ext@4 {
+		ddr-2d-dmem-fw {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
 			align-end = <4>;
 			type = "blob-ext";
diff --git a/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi b/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi
index 0f89c8f677fa..dc7afc05e8f9 100644
--- a/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi
+++ b/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi
@@ -143,24 +143,28 @@
 			align-end = <4>;
 		};
 
-		blob_1: blob-ext@1 {
+		ddr-1d-imem-fw {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
 			align-end = <4>;
+			type = "blob-ext";
 		};
 
-		blob_2: blob-ext@2 {
+		ddr-1d-dmem-fw {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
 			align-end = <4>;
+			type = "blob-ext";
 		};
 
-		blob_3: blob-ext@3 {
+		ddr-2d-imem-fw {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
 			align-end = <4>;
+			type = "blob-ext";
 		};
 
-		blob_4: blob-ext@4 {
+		ddr-2d-dmem-fw {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
 			align-end = <4>;
+			type = "blob-ext";
 		};
 	};
 
diff --git a/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi b/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi
index 5a52b73d7e9c..dc4cec250efa 100644
--- a/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi
+++ b/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi
@@ -111,13 +111,13 @@
 			filename = "u-boot-spl.bin";
 		};
 
-		imem_1d: blob-ext@1 {
+		ddr-1d-imem-fw {
 			filename = "ddr3_imem_1d.bin";
 			size = <0x8000>;
 			type = "blob-ext";
 		};
 
-		dmem_1d: blob-ext@2 {
+		ddr-1d-dmem-fw {
 			filename = "ddr3_dmem_1d.bin";
 			size = <0x4000>;
 			type = "blob-ext";
diff --git a/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi b/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi
index 6f3d5cfe3c18..3f02b4aac135 100644
--- a/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi
+++ b/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi
@@ -151,24 +151,28 @@
 			align-end = <4>;
 		};
 
-		blob_1: blob-ext@1 {
+		ddr-1d-imem-fw {
 			filename = "ddr4_imem_1d_201810.bin";
 			align-end = <4>;
+			type = "blob-ext";
 		};
 
-		blob_2: blob-ext@2 {
+		ddr-1d-dmem-fw {
 			filename = "ddr4_dmem_1d_201810.bin";
 			align-end = <4>;
+			type = "blob-ext";
 		};
 
-		blob_3: blob-ext@3 {
+		ddr-2d-imem-fw {
 			filename = "ddr4_imem_2d_201810.bin";
 			align-end = <4>;
+			type = "blob-ext";
 		};
 
-		blob_4: blob-ext@4 {
+		ddr-2d-dmem-fw {
 			filename = "ddr4_dmem_2d_201810.bin";
 			align-end = <4>;
+			type = "blob-ext";
 		};
 	};
 
diff --git a/arch/arm/dts/imx8mn-evk-u-boot.dtsi b/arch/arm/dts/imx8mn-evk-u-boot.dtsi
index 4f6dcf307b28..4d55905736d0 100644
--- a/arch/arm/dts/imx8mn-evk-u-boot.dtsi
+++ b/arch/arm/dts/imx8mn-evk-u-boot.dtsi
@@ -36,24 +36,28 @@
 			align-end = <4>;
 		};
 
-		blob_1: blob-ext@1 {
+		ddr-1d-imem-fw {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
 			align-end = <4>;
+			type = "blob-ext";
 		};
 
-		blob_2: blob-ext@2 {
+		ddr-1d-dmem-fw {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
 			align-end = <4>;
+			type = "blob-ext";
 		};
 
-		blob_3: blob-ext@3 {
+		ddr-2d-imem-fw {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
 			align-end = <4>;
+			type = "blob-ext";
 		};
 
-		blob_4: blob-ext@4 {
+		ddr-2d-dmem-fw {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
 			align-end = <4>;
+			type = "blob-ext";
 		};
 	};
 
diff --git a/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi b/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
index 32ef79292886..ed1ab10ded37 100644
--- a/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
+++ b/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
@@ -130,25 +130,25 @@
 			filename = "u-boot-spl.bin";
 		};
 
-		blob_1: blob-ext@1 {
+		ddr-1d-imem-fw {
 			filename = "ddr4_imem_1d.bin";
 			align-end = <4>;
 			type = "blob-ext";
 		};
 
-		blob_2: blob-ext@2 {
+		ddr-1d-dmem-fw {
 			filename = "ddr4_dmem_1d.bin";
 			align-end = <4>;
 			type = "blob-ext";
 		};
 
-		blob_3: blob-ext@3 {
+		ddr-2d-imem-fw {
 			filename = "ddr4_imem_2d.bin";
 			align-end = <4>;
 			type = "blob-ext";
 		};
 
-		blob_4: blob-ext@4 {
+		ddr-2d-dmem-fw {
 			filename = "ddr4_dmem_2d.bin";
 			align-end = <4>;
 			type = "blob-ext";
diff --git a/arch/arm/dts/imx8mn-venice-u-boot.dtsi b/arch/arm/dts/imx8mn-venice-u-boot.dtsi
index 03a9eb8516f9..6d6535fdbfeb 100644
--- a/arch/arm/dts/imx8mn-venice-u-boot.dtsi
+++ b/arch/arm/dts/imx8mn-venice-u-boot.dtsi
@@ -122,25 +122,25 @@
 			filename = "u-boot-spl.bin";
 		};
 
-		imem_1d: blob-ext@1 {
+		ddr-1d-imem-fw {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
 			align-end = <4>;
 			type = "blob-ext";
 		};
 
-		dmem_1d: blob-ext@2 {
+		ddr-1d-dmem-fw {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
 			align-end = <4>;
 			type = "blob-ext";
 		};
 
-		imem_2d: blob-ext@3 {
+		ddr-2d-imem-fw {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
 			align-end = <4>;
 			type = "blob-ext";
 		};
 
-		dmem_2d: blob-ext@4 {
+		ddr-2d-dmem-fw {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
 			align-end = <4>;
 			type = "blob-ext";
diff --git a/arch/arm/dts/imx8mp-u-boot.dtsi b/arch/arm/dts/imx8mp-u-boot.dtsi
index 274515a010ee..6fd4de2c9615 100644
--- a/arch/arm/dts/imx8mp-u-boot.dtsi
+++ b/arch/arm/dts/imx8mp-u-boot.dtsi
@@ -61,24 +61,28 @@
 			align-end = <4>;
 		};
 
-		blob_1: blob-ext@1 {
+		ddr-1d-imem-fw {
 			filename = "lpddr4_pmu_train_1d_imem_202006.bin";
 			align-end = <4>;
+			type = "blob-ext";
 		};
 
-		blob_2: blob-ext@2 {
+		ddr-1d-dmem-fw {
 			filename = "lpddr4_pmu_train_1d_dmem_202006.bin";
 			align-end = <4>;
+			type = "blob-ext";
 		};
 
-		blob_3: blob-ext@3 {
+		ddr-2d-imem-fw {
 			filename = "lpddr4_pmu_train_2d_imem_202006.bin";
 			align-end = <4>;
+			type = "blob-ext";
 		};
 
-		blob_4: blob-ext@4 {
+		ddr-2d-dmem-fw {
 			filename = "lpddr4_pmu_train_2d_dmem_202006.bin";
 			align-end = <4>;
+			type = "blob-ext";
 		};
 	};
 
diff --git a/arch/arm/dts/imx8mq-cm-u-boot.dtsi b/arch/arm/dts/imx8mq-cm-u-boot.dtsi
index 1b66e76ee1dc..81063083bd40 100644
--- a/arch/arm/dts/imx8mq-cm-u-boot.dtsi
+++ b/arch/arm/dts/imx8mq-cm-u-boot.dtsi
@@ -20,24 +20,28 @@
 			align-end = <4>;
 		};
 
-		blob_1: blob-ext@1 {
+		ddr-1d-imem-fw {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
 			align-end = <4>;
+			type = "blob-ext";
 		};
 
-		blob_2: blob-ext@2 {
+		ddr-1d-dmem-fw {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
 			align-end = <4>;
+			type = "blob-ext";
 		};
 
-		blob_3: blob-ext@3 {
+		ddr-2d-imem-fw {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
 			align-end = <4>;
+			type = "blob-ext";
 		};
 
-		blob_4: blob-ext@4 {
+		ddr-2d-dmem-fw {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
 			align-end = <4>;
+			type = "blob-ext";
 		};
 	};
 
diff --git a/arch/arm/dts/imx8mq-u-boot.dtsi b/arch/arm/dts/imx8mq-u-boot.dtsi
index e683e9184725..000ee545ee7f 100644
--- a/arch/arm/dts/imx8mq-u-boot.dtsi
+++ b/arch/arm/dts/imx8mq-u-boot.dtsi
@@ -22,25 +22,25 @@
 			filename = "u-boot-spl.bin";
 		};
 
-		imem_1d: blob-ext@1 {
+		ddr-1d-imem-fw {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
 			align-end = <4>;
 			type = "blob-ext";
 		};
 
-		dmem_1d: blob-ext@2 {
+		ddr-1d-dmem-fw {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
 			align-end = <4>;
 			type = "blob-ext";
 		};
 
-		imem_2d: blob-ext@3 {
+		ddr-2d-imem-fw {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
 			align-end = <4>;
 			type = "blob-ext";
 		};
 
-		dmem_2d: blob-ext@4 {
+		ddr-2d-dmem-fw {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
 			align-end = <4>;
 			type = "blob-ext";
diff --git a/drivers/ddr/imx/imx8m/helper.c b/drivers/ddr/imx/imx8m/helper.c
index b3bd57531b76..3764cffabc90 100644
--- a/drivers/ddr/imx/imx8m/helper.c
+++ b/drivers/ddr/imx/imx8m/helper.c
@@ -26,18 +26,18 @@ DECLARE_GLOBAL_DATA_PTR;
 #define DMEM_OFFSET_ADDR 0x00054000
 #define DDR_TRAIN_CODE_BASE_ADDR IP2APB_DDRPHY_IPS_BASE_ADDR(0)
 
-binman_sym_declare(ulong, blob_ext_1, image_pos);
-binman_sym_declare(ulong, blob_ext_1, size);
+binman_sym_declare(ulong, ddr_1d_imem_fw, image_pos);
+binman_sym_declare(ulong, ddr_1d_imem_fw, size);
 
-binman_sym_declare(ulong, blob_ext_2, image_pos);
-binman_sym_declare(ulong, blob_ext_2, size);
+binman_sym_declare(ulong, ddr_1d_dmem_fw, image_pos);
+binman_sym_declare(ulong, ddr_1d_dmem_fw, size);
 
 #if !IS_ENABLED(CONFIG_IMX8M_DDR3L)
-binman_sym_declare(ulong, blob_ext_3, image_pos);
-binman_sym_declare(ulong, blob_ext_3, size);
+binman_sym_declare(ulong, ddr_2d_imem_fw, image_pos);
+binman_sym_declare(ulong, ddr_2d_imem_fw, size);
 
-binman_sym_declare(ulong, blob_ext_4, image_pos);
-binman_sym_declare(ulong, blob_ext_4, size);
+binman_sym_declare(ulong, ddr_2d_dmem_fw, image_pos);
+binman_sym_declare(ulong, ddr_2d_dmem_fw, size);
 #endif
 
 /* We need PHY iMEM PHY is 32KB padded */
@@ -59,27 +59,27 @@ void ddr_load_train_firmware(enum fw_type type)
 	}
 #endif
 
+	dmem_start = imem_start + imem_len;
+
 	if (CONFIG_IS_ENABLED(BINMAN_SYMBOLS)) {
 		switch (type) {
 		case FW_1D_IMAGE:
-			imem_start = binman_sym(ulong, blob_ext_1, image_pos);
-			imem_len = binman_sym(ulong, blob_ext_1, size);
-			dmem_start = binman_sym(ulong, blob_ext_2, image_pos);
-			dmem_len = binman_sym(ulong, blob_ext_2, size);
+			imem_start = binman_sym(ulong, ddr_1d_imem_fw, image_pos);
+			imem_len = binman_sym(ulong, ddr_1d_imem_fw, size);
+			dmem_start = binman_sym(ulong, ddr_1d_dmem_fw, image_pos);
+			dmem_len = binman_sym(ulong, ddr_1d_dmem_fw, size);
 			break;
 		case FW_2D_IMAGE:
 #if !IS_ENABLED(CONFIG_IMX8M_DDR3L)
-			imem_start = binman_sym(ulong, blob_ext_3, image_pos);
-			imem_len = binman_sym(ulong, blob_ext_3, size);
-			dmem_start = binman_sym(ulong, blob_ext_4, image_pos);
-			dmem_len = binman_sym(ulong, blob_ext_4, size);
+			imem_start = binman_sym(ulong, ddr_1d_dmem_fw, image_pos);
+			imem_len = binman_sym(ulong, ddr_1d_dmem_fw, size);
+			dmem_start = binman_sym(ulong, ddr_2d_dmem_fw, image_pos);
+			dmem_len = binman_sym(ulong, ddr_2d_dmem_fw, size);
 #endif
 			break;
 		}
 	}
 
-	dmem_start = imem_start + imem_len;
-
 	pr_from32 = imem_start;
 	pr_to32 = DDR_TRAIN_CODE_BASE_ADDR + 4 * IMEM_OFFSET_ADDR;
 	for (i = 0x0; i < imem_len; ) {
diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
index e3f362b442b8..bd67238b9199 100644
--- a/tools/binman/etype/section.py
+++ b/tools/binman/etype/section.py
@@ -875,7 +875,7 @@ def _CollectEntries(self, entries, entries_by_name, add_entry):
                 entries[entry.GetPath()] = entry
             for entry in to_add.values():
                 self._CollectEntries(entries, entries_by_name, entry)
-        entries_by_name[add_entry.name.replace('@', '-')] = add_entry
+        entries_by_name[add_entry.name] = add_entry
 
     def MissingArgs(self, entry, missing):
         """Report a missing argument, if enabled

Re: [PATCH V4 7/8] arm: dts: imx8m: shrink ddr firmware size to actual file size

[Alper Nebi Yasak]

On 20/05/2022 17:10, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> After we switch to use BINMAN_SYMBOLS, there is no need to pad
> the file size to 0x8000 and 0x4000. After we use BINMAN_SYMBOLS,
> the u-boot-spl-ddr.bin shrink about 36KB with i.MX8MP-EVK.
> 
> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  arch/arm/dts/imx8mm-u-boot.dtsi                  | 8 ++++----
>  arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi       | 8 ++++----
>  arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi         | 8 ++++----
>  arch/arm/dts/imx8mn-evk-u-boot.dtsi              | 8 ++++----
>  arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi | 8 ++++----
>  arch/arm/dts/imx8mn-venice-u-boot.dtsi           | 8 ++++----
>  arch/arm/dts/imx8mp-u-boot.dtsi                  | 8 ++++----
>  arch/arm/dts/imx8mq-cm-u-boot.dtsi               | 8 ++++----
>  arch/arm/dts/imx8mq-u-boot.dtsi                  | 8 ++++----

Probably can be done for 'imx8mn-bsh-smm-s2-u-boot-common.dtsi' as well.

>  9 files changed, 36 insertions(+), 36 deletions(-)
> 
> diff --git a/arch/arm/dts/imx8mm-u-boot.dtsi b/arch/arm/dts/imx8mm-u-boot.dtsi
> index 5de55a2d80b..19a2da30f51 100644
> --- a/arch/arm/dts/imx8mm-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mm-u-boot.dtsi
> @@ -41,25 +41,25 @@
>  
>  		imem_1d: blob-ext@1 {
>  			filename = "lpddr4_pmu_train_1d_imem.bin";
> -			size = <0x8000>;
> +			align-end = <4>;
>  			type = "blob-ext";
>  		};
>  
>  		dmem_1d: blob-ext@2 {
>  			filename = "lpddr4_pmu_train_1d_dmem.bin";
> -			size = <0x4000>;
> +			align-end = <4>;
>  			type = "blob-ext";
>  		};
>  
>  		imem_2d: blob-ext@3 {
>  			filename = "lpddr4_pmu_train_2d_imem.bin";
> -			size = <0x8000>;
> +			align-end = <4>;
>  			type = "blob-ext";
>  		};
>  
>  		dmem_2d: blob-ext@4 {
>  			filename = "lpddr4_pmu_train_2d_dmem.bin";
> -			size = <0x4000>;
> +			align-end = <4>;
>  			type = "blob-ext";
>  		};
>  	};
> diff --git a/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi b/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi
> index eb1dd8debba..e1740fa31a6 100644
> --- a/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi
> @@ -149,22 +149,22 @@
>  
>  		blob_1: blob-ext@1 {
>  			filename = "lpddr4_pmu_train_1d_imem.bin";
> -			size = <0x8000>;
> +			align-end = <4>;
>  		};
>  
>  		blob_2: blob-ext@2 {
>  			filename = "lpddr4_pmu_train_1d_dmem.bin";
> -			size = <0x4000>;
> +			align-end = <4>;
>  		};
>  
>  		blob_3: blob-ext@3 {
>  			filename = "lpddr4_pmu_train_2d_imem.bin";
> -			size = <0x8000>;
> +			align-end = <4>;
>  		};
>  
>  		blob_4: blob-ext@4 {
>  			filename = "lpddr4_pmu_train_2d_dmem.bin";
> -			size = <0x4000>;
> +			align-end = <4>;
>  		};
>  	};
>  
> diff --git a/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi b/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi
> index 4d0ecb07d4f..1fe2d0fd507 100644
> --- a/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi
> @@ -157,22 +157,22 @@
>  
>  		blob_1: blob-ext@1 {
>  			filename = "ddr4_imem_1d_201810.bin";
> -			size = <0x8000>;
> +			align-end = <4>;
>  		};
>  
>  		blob_2: blob-ext@2 {
>  			filename = "ddr4_dmem_1d_201810.bin";
> -			size = <0x4000>;
> +			align-end = <4>;
>  		};
>  
>  		blob_3: blob-ext@3 {
>  			filename = "ddr4_imem_2d_201810.bin";
> -			size = <0x8000>;
> +			align-end = <4>;
>  		};
>  
>  		blob_4: blob-ext@4 {
>  			filename = "ddr4_dmem_2d_201810.bin";
> -			size = <0x4000>;
> +			align-end = <4>;
>  		};
>  	};
>  
> diff --git a/arch/arm/dts/imx8mn-evk-u-boot.dtsi b/arch/arm/dts/imx8mn-evk-u-boot.dtsi
> index 3db46d4cbcb..4f6dcf307b2 100644
> --- a/arch/arm/dts/imx8mn-evk-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mn-evk-u-boot.dtsi
> @@ -38,22 +38,22 @@
>  
>  		blob_1: blob-ext@1 {
>  			filename = "lpddr4_pmu_train_1d_imem.bin";
> -			size = <0x8000>;
> +			align-end = <4>;
>  		};
>  
>  		blob_2: blob-ext@2 {
>  			filename = "lpddr4_pmu_train_1d_dmem.bin";
> -			size = <0x4000>;
> +			align-end = <4>;
>  		};
>  
>  		blob_3: blob-ext@3 {
>  			filename = "lpddr4_pmu_train_2d_imem.bin";
> -			size = <0x8000>;
> +			align-end = <4>;
>  		};
>  
>  		blob_4: blob-ext@4 {
>  			filename = "lpddr4_pmu_train_2d_dmem.bin";
> -			size = <0x4000>;
> +			align-end = <4>;
>  		};
>  	};
>  
> diff --git a/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi b/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
> index 001e725f568..32ef7929288 100644
> --- a/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
> @@ -132,25 +132,25 @@
>  
>  		blob_1: blob-ext@1 {
>  			filename = "ddr4_imem_1d.bin";
> -			size = <0x8000>;
> +			align-end = <4>;
>  			type = "blob-ext";
>  		};
>  
>  		blob_2: blob-ext@2 {
>  			filename = "ddr4_dmem_1d.bin";
> -			size = <0x4000>;
> +			align-end = <4>;
>  			type = "blob-ext";
>  		};
>  
>  		blob_3: blob-ext@3 {
>  			filename = "ddr4_imem_2d.bin";
> -			size = <0x8000>;
> +			align-end = <4>;
>  			type = "blob-ext";
>  		};
>  
>  		blob_4: blob-ext@4 {
>  			filename = "ddr4_dmem_2d.bin";
> -			size = <0x4000>;
> +			align-end = <4>;
>  			type = "blob-ext";
>  		};
>  	};
> diff --git a/arch/arm/dts/imx8mn-venice-u-boot.dtsi b/arch/arm/dts/imx8mn-venice-u-boot.dtsi
> index 67922146963..e3a0b170347 100644
> --- a/arch/arm/dts/imx8mn-venice-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mn-venice-u-boot.dtsi
> @@ -128,25 +128,25 @@
>  
>  		imem_1d: blob-ext@1 {
>  			filename = "lpddr4_pmu_train_1d_imem.bin";
> -			size = <0x8000>;
> +			align-end = <4>;
>  			type = "blob-ext";
>  		};
>  
>  		dmem_1d: blob-ext@2 {
>  			filename = "lpddr4_pmu_train_1d_dmem.bin";
> -			size = <0x4000>;
> +			align-end = <4>;
>  			type = "blob-ext";
>  		};
>  
>  		imem_2d: blob-ext@3 {
>  			filename = "lpddr4_pmu_train_2d_imem.bin";
> -			size = <0x8000>;
> +			align-end = <4>;
>  			type = "blob-ext";
>  		};
>  
>  		dmem_2d: blob-ext@4 {
>  			filename = "lpddr4_pmu_train_2d_dmem.bin";
> -			size = <0x4000>;
> +			align-end = <4>;
>  			type = "blob-ext";
>  		};
>  	};
> diff --git a/arch/arm/dts/imx8mp-u-boot.dtsi b/arch/arm/dts/imx8mp-u-boot.dtsi
> index 20edd90cfad..274515a010e 100644
> --- a/arch/arm/dts/imx8mp-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mp-u-boot.dtsi
> @@ -63,22 +63,22 @@
>  
>  		blob_1: blob-ext@1 {
>  			filename = "lpddr4_pmu_train_1d_imem_202006.bin";
> -			size = <0x8000>;
> +			align-end = <4>;
>  		};
>  
>  		blob_2: blob-ext@2 {
>  			filename = "lpddr4_pmu_train_1d_dmem_202006.bin";
> -			size = <0x4000>;
> +			align-end = <4>;
>  		};
>  
>  		blob_3: blob-ext@3 {
>  			filename = "lpddr4_pmu_train_2d_imem_202006.bin";
> -			size = <0x8000>;
> +			align-end = <4>;
>  		};
>  
>  		blob_4: blob-ext@4 {
>  			filename = "lpddr4_pmu_train_2d_dmem_202006.bin";
> -			size = <0x4000>;
> +			align-end = <4>;
>  		};
>  	};
>  
> diff --git a/arch/arm/dts/imx8mq-cm-u-boot.dtsi b/arch/arm/dts/imx8mq-cm-u-boot.dtsi
> index e2f4b0e740d..9538a04ed97 100644
> --- a/arch/arm/dts/imx8mq-cm-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mq-cm-u-boot.dtsi
> @@ -30,22 +30,22 @@
>  
>  		blob_1: blob-ext@1 {
>  			filename = "lpddr4_pmu_train_1d_imem.bin";
> -			size = <0x8000>;
> +			align-end = <4>;
>  		};
>  
>  		blob_2: blob-ext@2 {
>  			filename = "lpddr4_pmu_train_1d_dmem.bin";
> -			size = <0x4000>;
> +			align-end = <4>;
>  		};
>  
>  		blob_3: blob-ext@3 {
>  			filename = "lpddr4_pmu_train_2d_imem.bin";
> -			size = <0x8000>;
> +			align-end = <4>;
>  		};
>  
>  		blob_4: blob-ext@4 {
>  			filename = "lpddr4_pmu_train_2d_dmem.bin";
> -			size = <0x4000>;
> +			align-end = <4>;
>  		};
>  	};
>  
> diff --git a/arch/arm/dts/imx8mq-u-boot.dtsi b/arch/arm/dts/imx8mq-u-boot.dtsi
> index 389414ad26f..1495869fcd2 100644
> --- a/arch/arm/dts/imx8mq-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mq-u-boot.dtsi
> @@ -48,25 +48,25 @@
>  
>  		imem_1d: blob-ext@1 {
>  			filename = "lpddr4_pmu_train_1d_imem.bin";
> -			size = <0x8000>;
> +			align-end = <4>;
>  			type = "blob-ext";
>  		};
>  
>  		dmem_1d: blob-ext@2 {
>  			filename = "lpddr4_pmu_train_1d_dmem.bin";
> -			size = <0x4000>;
> +			align-end = <4>;
>  			type = "blob-ext";
>  		};
>  
>  		imem_2d: blob-ext@3 {
>  			filename = "lpddr4_pmu_train_2d_imem.bin";
> -			size = <0x8000>;
> +			align-end = <4>;
>  			type = "blob-ext";
>  		};
>  
>  		dmem_2d: blob-ext@4 {
>  			filename = "lpddr4_pmu_train_2d_dmem.bin";
> -			size = <0x4000>;
> +			align-end = <4>;
>  			type = "blob-ext";
>  		};
>  	};

Re: [PATCH V4 7/8] arm: dts: imx8m: shrink ddr firmware size to actual file size

[Michael Nazzareno Trimarchi]

Hi

Il lun 23 mag 2022, 23:13 Alper Nebi Yasak <alpernebiyasak@gmail.com> ha
scritto:

> On 20/05/2022 17:10, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > After we switch to use BINMAN_SYMBOLS, there is no need to pad
> > the file size to 0x8000 and 0x4000. After we use BINMAN_SYMBOLS,
> > the u-boot-spl-ddr.bin shrink about 36KB with i.MX8MP-EVK.
> >
> > Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  arch/arm/dts/imx8mm-u-boot.dtsi                  | 8 ++++----
> >  arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi       | 8 ++++----
> >  arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi         | 8 ++++----
> >  arch/arm/dts/imx8mn-evk-u-boot.dtsi              | 8 ++++----
> >  arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi | 8 ++++----
> >  arch/arm/dts/imx8mn-venice-u-boot.dtsi           | 8 ++++----
> >  arch/arm/dts/imx8mp-u-boot.dtsi                  | 8 ++++----
> >  arch/arm/dts/imx8mq-cm-u-boot.dtsi               | 8 ++++----
> >  arch/arm/dts/imx8mq-u-boot.dtsi                  | 8 ++++----
>
> Probably can be done for 'imx8mn-bsh-smm-s2-u-boot-common.dtsi' as well.
>

Let us test.

I did not find time

Michael

>
> >  9 files changed, 36 insertions(+), 36 deletions(-)
> >
> > diff --git a/arch/arm/dts/imx8mm-u-boot.dtsi
> b/arch/arm/dts/imx8mm-u-boot.dtsi
> > index 5de55a2d80b..19a2da30f51 100644
> > --- a/arch/arm/dts/imx8mm-u-boot.dtsi
> > +++ b/arch/arm/dts/imx8mm-u-boot.dtsi
> > @@ -41,25 +41,25 @@
> >
> >               imem_1d: blob-ext@1 {
> >                       filename = "lpddr4_pmu_train_1d_imem.bin";
> > -                     size = <0x8000>;
> > +                     align-end = <4>;
> >                       type = "blob-ext";
> >               };
> >
> >               dmem_1d: blob-ext@2 {
> >                       filename = "lpddr4_pmu_train_1d_dmem.bin";
> > -                     size = <0x4000>;
> > +                     align-end = <4>;
> >                       type = "blob-ext";
> >               };
> >
> >               imem_2d: blob-ext@3 {
> >                       filename = "lpddr4_pmu_train_2d_imem.bin";
> > -                     size = <0x8000>;
> > +                     align-end = <4>;
> >                       type = "blob-ext";
> >               };
> >
> >               dmem_2d: blob-ext@4 {
> >                       filename = "lpddr4_pmu_train_2d_dmem.bin";
> > -                     size = <0x4000>;
> > +                     align-end = <4>;
> >                       type = "blob-ext";
> >               };
> >       };
> > diff --git a/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi
> b/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi
> > index eb1dd8debba..e1740fa31a6 100644
> > --- a/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi
> > +++ b/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi
> > @@ -149,22 +149,22 @@
> >
> >               blob_1: blob-ext@1 {
> >                       filename = "lpddr4_pmu_train_1d_imem.bin";
> > -                     size = <0x8000>;
> > +                     align-end = <4>;
> >               };
> >
> >               blob_2: blob-ext@2 {
> >                       filename = "lpddr4_pmu_train_1d_dmem.bin";
> > -                     size = <0x4000>;
> > +                     align-end = <4>;
> >               };
> >
> >               blob_3: blob-ext@3 {
> >                       filename = "lpddr4_pmu_train_2d_imem.bin";
> > -                     size = <0x8000>;
> > +                     align-end = <4>;
> >               };
> >
> >               blob_4: blob-ext@4 {
> >                       filename = "lpddr4_pmu_train_2d_dmem.bin";
> > -                     size = <0x4000>;
> > +                     align-end = <4>;
> >               };
> >       };
> >
> > diff --git a/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi
> b/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi
> > index 4d0ecb07d4f..1fe2d0fd507 100644
> > --- a/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi
> > +++ b/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi
> > @@ -157,22 +157,22 @@
> >
> >               blob_1: blob-ext@1 {
> >                       filename = "ddr4_imem_1d_201810.bin";
> > -                     size = <0x8000>;
> > +                     align-end = <4>;
> >               };
> >
> >               blob_2: blob-ext@2 {
> >                       filename = "ddr4_dmem_1d_201810.bin";
> > -                     size = <0x4000>;
> > +                     align-end = <4>;
> >               };
> >
> >               blob_3: blob-ext@3 {
> >                       filename = "ddr4_imem_2d_201810.bin";
> > -                     size = <0x8000>;
> > +                     align-end = <4>;
> >               };
> >
> >               blob_4: blob-ext@4 {
> >                       filename = "ddr4_dmem_2d_201810.bin";
> > -                     size = <0x4000>;
> > +                     align-end = <4>;
> >               };
> >       };
> >
> > diff --git a/arch/arm/dts/imx8mn-evk-u-boot.dtsi
> b/arch/arm/dts/imx8mn-evk-u-boot.dtsi
> > index 3db46d4cbcb..4f6dcf307b2 100644
> > --- a/arch/arm/dts/imx8mn-evk-u-boot.dtsi
> > +++ b/arch/arm/dts/imx8mn-evk-u-boot.dtsi
> > @@ -38,22 +38,22 @@
> >
> >               blob_1: blob-ext@1 {
> >                       filename = "lpddr4_pmu_train_1d_imem.bin";
> > -                     size = <0x8000>;
> > +                     align-end = <4>;
> >               };
> >
> >               blob_2: blob-ext@2 {
> >                       filename = "lpddr4_pmu_train_1d_dmem.bin";
> > -                     size = <0x4000>;
> > +                     align-end = <4>;
> >               };
> >
> >               blob_3: blob-ext@3 {
> >                       filename = "lpddr4_pmu_train_2d_imem.bin";
> > -                     size = <0x8000>;
> > +                     align-end = <4>;
> >               };
> >
> >               blob_4: blob-ext@4 {
> >                       filename = "lpddr4_pmu_train_2d_dmem.bin";
> > -                     size = <0x4000>;
> > +                     align-end = <4>;
> >               };
> >       };
> >
> > diff --git a/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
> b/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
> > index 001e725f568..32ef7929288 100644
> > --- a/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
> > +++ b/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
> > @@ -132,25 +132,25 @@
> >
> >               blob_1: blob-ext@1 {
> >                       filename = "ddr4_imem_1d.bin";
> > -                     size = <0x8000>;
> > +                     align-end = <4>;
> >                       type = "blob-ext";
> >               };
> >
> >               blob_2: blob-ext@2 {
> >                       filename = "ddr4_dmem_1d.bin";
> > -                     size = <0x4000>;
> > +                     align-end = <4>;
> >                       type = "blob-ext";
> >               };
> >
> >               blob_3: blob-ext@3 {
> >                       filename = "ddr4_imem_2d.bin";
> > -                     size = <0x8000>;
> > +                     align-end = <4>;
> >                       type = "blob-ext";
> >               };
> >
> >               blob_4: blob-ext@4 {
> >                       filename = "ddr4_dmem_2d.bin";
> > -                     size = <0x4000>;
> > +                     align-end = <4>;
> >                       type = "blob-ext";
> >               };
> >       };
> > diff --git a/arch/arm/dts/imx8mn-venice-u-boot.dtsi
> b/arch/arm/dts/imx8mn-venice-u-boot.dtsi
> > index 67922146963..e3a0b170347 100644
> > --- a/arch/arm/dts/imx8mn-venice-u-boot.dtsi
> > +++ b/arch/arm/dts/imx8mn-venice-u-boot.dtsi
> > @@ -128,25 +128,25 @@
> >
> >               imem_1d: blob-ext@1 {
> >                       filename = "lpddr4_pmu_train_1d_imem.bin";
> > -                     size = <0x8000>;
> > +                     align-end = <4>;
> >                       type = "blob-ext";
> >               };
> >
> >               dmem_1d: blob-ext@2 {
> >                       filename = "lpddr4_pmu_train_1d_dmem.bin";
> > -                     size = <0x4000>;
> > +                     align-end = <4>;
> >                       type = "blob-ext";
> >               };
> >
> >               imem_2d: blob-ext@3 {
> >                       filename = "lpddr4_pmu_train_2d_imem.bin";
> > -                     size = <0x8000>;
> > +                     align-end = <4>;
> >                       type = "blob-ext";
> >               };
> >
> >               dmem_2d: blob-ext@4 {
> >                       filename = "lpddr4_pmu_train_2d_dmem.bin";
> > -                     size = <0x4000>;
> > +                     align-end = <4>;
> >                       type = "blob-ext";
> >               };
> >       };
> > diff --git a/arch/arm/dts/imx8mp-u-boot.dtsi
> b/arch/arm/dts/imx8mp-u-boot.dtsi
> > index 20edd90cfad..274515a010e 100644
> > --- a/arch/arm/dts/imx8mp-u-boot.dtsi
> > +++ b/arch/arm/dts/imx8mp-u-boot.dtsi
> > @@ -63,22 +63,22 @@
> >
> >               blob_1: blob-ext@1 {
> >                       filename = "lpddr4_pmu_train_1d_imem_202006.bin";
> > -                     size = <0x8000>;
> > +                     align-end = <4>;
> >               };
> >
> >               blob_2: blob-ext@2 {
> >                       filename = "lpddr4_pmu_train_1d_dmem_202006.bin";
> > -                     size = <0x4000>;
> > +                     align-end = <4>;
> >               };
> >
> >               blob_3: blob-ext@3 {
> >                       filename = "lpddr4_pmu_train_2d_imem_202006.bin";
> > -                     size = <0x8000>;
> > +                     align-end = <4>;
> >               };
> >
> >               blob_4: blob-ext@4 {
> >                       filename = "lpddr4_pmu_train_2d_dmem_202006.bin";
> > -                     size = <0x4000>;
> > +                     align-end = <4>;
> >               };
> >       };
> >
> > diff --git a/arch/arm/dts/imx8mq-cm-u-boot.dtsi
> b/arch/arm/dts/imx8mq-cm-u-boot.dtsi
> > index e2f4b0e740d..9538a04ed97 100644
> > --- a/arch/arm/dts/imx8mq-cm-u-boot.dtsi
> > +++ b/arch/arm/dts/imx8mq-cm-u-boot.dtsi
> > @@ -30,22 +30,22 @@
> >
> >               blob_1: blob-ext@1 {
> >                       filename = "lpddr4_pmu_train_1d_imem.bin";
> > -                     size = <0x8000>;
> > +                     align-end = <4>;
> >               };
> >
> >               blob_2: blob-ext@2 {
> >                       filename = "lpddr4_pmu_train_1d_dmem.bin";
> > -                     size = <0x4000>;
> > +                     align-end = <4>;
> >               };
> >
> >               blob_3: blob-ext@3 {
> >                       filename = "lpddr4_pmu_train_2d_imem.bin";
> > -                     size = <0x8000>;
> > +                     align-end = <4>;
> >               };
> >
> >               blob_4: blob-ext@4 {
> >                       filename = "lpddr4_pmu_train_2d_dmem.bin";
> > -                     size = <0x4000>;
> > +                     align-end = <4>;
> >               };
> >       };
> >
> > diff --git a/arch/arm/dts/imx8mq-u-boot.dtsi
> b/arch/arm/dts/imx8mq-u-boot.dtsi
> > index 389414ad26f..1495869fcd2 100644
> > --- a/arch/arm/dts/imx8mq-u-boot.dtsi
> > +++ b/arch/arm/dts/imx8mq-u-boot.dtsi
> > @@ -48,25 +48,25 @@
> >
> >               imem_1d: blob-ext@1 {
> >                       filename = "lpddr4_pmu_train_1d_imem.bin";
> > -                     size = <0x8000>;
> > +                     align-end = <4>;
> >                       type = "blob-ext";
> >               };
> >
> >               dmem_1d: blob-ext@2 {
> >                       filename = "lpddr4_pmu_train_1d_dmem.bin";
> > -                     size = <0x4000>;
> > +                     align-end = <4>;
> >                       type = "blob-ext";
> >               };
> >
> >               imem_2d: blob-ext@3 {
> >                       filename = "lpddr4_pmu_train_2d_imem.bin";
> > -                     size = <0x8000>;
> > +                     align-end = <4>;
> >                       type = "blob-ext";
> >               };
> >
> >               dmem_2d: blob-ext@4 {
> >                       filename = "lpddr4_pmu_train_2d_dmem.bin";
> > -                     size = <0x4000>;
> > +                     align-end = <4>;
> >                       type = "blob-ext";
> >               };
> >       };
>