[PATCH 0/5] spl: binman: Fixes for BINMAN_SYMBOLS
There's some trouble with an i.MX8M series [1] trying to use binman symbols. The crux of it is the 'u_boot_any' symbols BINMAN_SYMBOLS configs declare, and the boards creating partial binman images including an SPL without a U-Boot the symbol is referring to.
Normally this should be easy to resolve by disabling BINMAN_SYMBOLS configs, but that causes a build error. Apparently some parts of the SPL code (RAW_IMAGE_SUPPORT, RAM_DEVICE) use the symbols directly without guarding them by BINMAN_SYMBOLS, implicitly requiring it.
The first patch fixes the issue above, the rest are minor things I tinkered with while trying to understand the issue. These apply onto u-boot/next. I have also triggered an Azure CI run [2] via a Github pull request.
[1] arm64: binman: use binman symbols for imx https://lore.kernel.org/u-boot/20220603071715.15212-1-peng.fan@oss.nxp.com/
[2] #20220610.3 spl: binman: Fixes for BINMAN_SYMBOLS https://dev.azure.com/u-boot/u-boot/_build/results?buildId=4431&view=res...
Alper Nebi Yasak (5): spl: binman: Fix use of undeclared u_boot_any symbols spl: binman: Make TPL_BINMAN_SYMBOLS depend on TPL_FRAMEWORK spl: binman: Declare extern symbols for VPL as well spl: binman: Let u-boot-spl/vpl symbol declarations be disabled spl: binman: Add a config option for binman symbols in VPL
common/spl/Kconfig | 12 ++++++------ common/spl/Kconfig.tpl | 14 +++++++------- common/spl/Kconfig.vpl | 12 ++++++++++++ common/spl/spl.c | 13 +++++++++---- common/spl/spl_ram.c | 2 +- include/spl.h | 2 ++ 6 files changed, 37 insertions(+), 18 deletions(-)
Some SPL functions directly use the binman 'u_boot_any' symbols to get U-Boot's binman image position. These symbols are declared by the SPL/TPL_BINMAN_SYMBOLS configs, but they are accessed by macros defined by just CONFIG_BINMAN. So when BINMAN is enabled and BINMAN_SYMBOLS is disabled, the code tries to use undeclared symbols and we get an error.
Therefore, any use of 'u_boot_any' symbols in the code is an implicit dependency on SPL/TPL_BINMAN_SYMBOLS. However, in the current uses they are meant to be the next phase's values, where that happens to be U-Boot. In the meantime, helper funcions spl_get_image_pos/size() were introduced to get these values.
Convert all uses of u_boot_any symbols to these functions, so we only access these symbols at one place. Make sure they will not use these symbols when the BINMAN_SYMBOLS configs are disabled, by returning early in those cases.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com ---
common/spl/spl.c | 10 +++++++--- common/spl/spl_ram.c | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/common/spl/spl.c b/common/spl/spl.c index 2a69a7c9324d..5630dcdb5c1e 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -149,9 +149,11 @@ void spl_fixup_fdt(void *fdt_blob) #endif }
-#if CONFIG_IS_ENABLED(BINMAN_SYMBOLS) ulong spl_get_image_pos(void) { + if (!CONFIG_IS_ENABLED(BINMAN_SYMBOLS)) + return BINMAN_SYM_MISSING; + #ifdef CONFIG_VPL if (spl_next_phase() == PHASE_VPL) return binman_sym(ulong, u_boot_vpl, image_pos); @@ -163,6 +165,9 @@ ulong spl_get_image_pos(void)
ulong spl_get_image_size(void) { + if (!CONFIG_IS_ENABLED(BINMAN_SYMBOLS)) + return BINMAN_SYM_MISSING; + #ifdef CONFIG_VPL if (spl_next_phase() == PHASE_VPL) return binman_sym(ulong, u_boot_vpl, size); @@ -171,7 +176,6 @@ ulong spl_get_image_size(void) binman_sym(ulong, u_boot_spl, size) : binman_sym(ulong, u_boot_any, size); } -#endif /* BINMAN_SYMBOLS */
ulong spl_get_image_text_base(void) { @@ -222,7 +226,7 @@ __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) { - ulong u_boot_pos = binman_sym(ulong, u_boot_any, image_pos); + ulong u_boot_pos = spl_get_image_pos();
spl_image->size = CONFIG_SYS_MONITOR_LEN;
diff --git a/common/spl/spl_ram.c b/common/spl/spl_ram.c index 829645925718..d64710878cf2 100644 --- a/common/spl/spl_ram.c +++ b/common/spl/spl_ram.c @@ -70,7 +70,7 @@ 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 { - ulong u_boot_pos = binman_sym(ulong, u_boot_any, image_pos); + ulong u_boot_pos = spl_get_image_pos();
debug("Legacy image\n"); /*
TPL_BINMAN_SYMBOLS depends on SPL_FRAMEWORK. The code this enables is compiled by checking CONFIG_$(SPL_TPL_)FRAMEWORK, so it should depend on TPL_FRAMEWORK instead (which in turn depends on SPL_FRAMEWORK). This was most likely a typo due to copy-pasting the config's SPL version, fix it.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com ---
common/spl/Kconfig.tpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/spl/Kconfig.tpl b/common/spl/Kconfig.tpl index 9a0e719cf949..834cb6b6dd82 100644 --- a/common/spl/Kconfig.tpl +++ b/common/spl/Kconfig.tpl @@ -10,7 +10,7 @@ config TPL_SIZE_LIMIT
config TPL_BINMAN_SYMBOLS bool "Declare binman symbols in TPL" - depends on SPL_FRAMEWORK && BINMAN + depends on TPL_FRAMEWORK && BINMAN default y help This enables use of symbols in TPL which refer to U-Boot, enabling TPL
The binman extern symbol declarations in spl.h are missing the VPL symbols recently added to spl.c, add them like the others.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com ---
include/spl.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/include/spl.h b/include/spl.h index 83ac583e0b49..1778e0f53686 100644 --- a/include/spl.h +++ b/include/spl.h @@ -288,6 +288,8 @@ binman_sym_extern(ulong, u_boot_any, image_pos); binman_sym_extern(ulong, u_boot_any, size); binman_sym_extern(ulong, u_boot_spl, image_pos); binman_sym_extern(ulong, u_boot_spl, size); +binman_sym_extern(ulong, u_boot_vpl, image_pos); +binman_sym_extern(ulong, u_boot_vpl, size);
/** * spl_get_image_pos() - get the image position of the next phase
The SPL/TPL_BINMAN_SYMBOLS config only disables the u_boot_any symbol. Extend its #if directive to cover declarations for all phases. Update the Kconfig prompt and help message to make it clearer about this.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com ---
common/spl/Kconfig | 12 ++++++------ common/spl/Kconfig.tpl | 12 ++++++------ common/spl/spl.c | 3 ++- 3 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/common/spl/Kconfig b/common/spl/Kconfig index 2ad2351c6eb3..f32548b8c9a7 100644 --- a/common/spl/Kconfig +++ b/common/spl/Kconfig @@ -186,16 +186,16 @@ config SPL_SHOW_ERRORS This adds a small amount to SPL code size, perhaps 100 bytes.
config SPL_BINMAN_SYMBOLS - bool "Declare binman symbols in SPL" + bool "Declare binman symbols for U-Boot phases in SPL" depends on SPL_FRAMEWORK && BINMAN default y help - This enables use of symbols in SPL which refer to U-Boot, enabling SPL - to obtain the location of U-Boot simply by calling spl_get_image_pos() - and spl_get_image_size(). + This enables use of symbols in SPL which refer to U-Boot phases, + enabling SPL to obtain the location and size of its next phase simply + by calling spl_get_image_pos() and spl_get_image_size().
- For this to work, you must have a U-Boot image in the binman image, so - binman can update SPL with the location of it. + For this to work, you must have all U-Boot phases in the same binman + image, so binman can update SPL with the locations of everything.
source "common/spl/Kconfig.nxp"
diff --git a/common/spl/Kconfig.tpl b/common/spl/Kconfig.tpl index 834cb6b6dd82..3a97487c9983 100644 --- a/common/spl/Kconfig.tpl +++ b/common/spl/Kconfig.tpl @@ -9,16 +9,16 @@ config TPL_SIZE_LIMIT If this value is zero, it is ignored.
config TPL_BINMAN_SYMBOLS - bool "Declare binman symbols in TPL" + bool "Declare binman symbols for U-Boot phases in TPL" depends on TPL_FRAMEWORK && BINMAN default y help - This enables use of symbols in TPL which refer to U-Boot, enabling TPL - to obtain the location of U-Boot simply by calling spl_get_image_pos() - and spl_get_image_size(). + This enables use of symbols in TPL which refer to U-Boot phases, + enabling TPL to obtain the location and size of its next phase simply + by calling spl_get_image_pos() and spl_get_image_size().
- For this to work, you must have a U-Boot image in the binman image, so - binman can update TPL with the location of it. + For this to work, you must have all U-Boot phases in the same binman + image, so binman can update TPL with the locations of everything.
config TPL_FRAMEWORK bool "Support TPL based upon the common SPL framework" diff --git a/common/spl/spl.c b/common/spl/spl.c index 5630dcdb5c1e..4579289f9d83 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -55,7 +55,6 @@ u32 *boot_params_ptr = NULL; /* See spl.h for information about this */ binman_sym_declare(ulong, u_boot_any, image_pos); binman_sym_declare(ulong, u_boot_any, size); -#endif
#ifdef CONFIG_TPL binman_sym_declare(ulong, u_boot_spl, image_pos); @@ -67,6 +66,8 @@ binman_sym_declare(ulong, u_boot_vpl, image_pos); binman_sym_declare(ulong, u_boot_vpl, size); #endif
+#endif /* BINMAN_SYMBOLS */ + /* Define board data structure */ static struct bd_info bdata __attribute__ ((section(".data")));
The SPL code declares binman symbols for U-Boot phases depending on CONFIG_IS_ENABLED(BINMAN_SYMBOLS). This config exists for SPL and TPL, also add a version for VPL.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com ---
common/spl/Kconfig.vpl | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/common/spl/Kconfig.vpl b/common/spl/Kconfig.vpl index ba1ea6075b94..29fa1cf400ba 100644 --- a/common/spl/Kconfig.vpl +++ b/common/spl/Kconfig.vpl @@ -198,4 +198,16 @@ config VPL_TEXT_BASE help The address in memory that VPL will be running from.
+config VPL_BINMAN_SYMBOLS + bool "Declare binman symbols for U-Boot phases in VPL" + depends on VPL_FRAMEWORK && BINMAN + default y + help + This enables use of symbols in VPL which refer to U-Boot phases, + enabling VPL to obtain the location and size of its next phase simply + by calling spl_get_image_pos() and spl_get_image_size(). + + For this to work, you must have all U-Boot phases in the same binman + image, so binman can update VPL with the locations of everything. + endmenu
On 6/10/22 12:58, Alper Nebi Yasak wrote:
There's some trouble with an i.MX8M series [1] trying to use binman symbols. The crux of it is the 'u_boot_any' symbols BINMAN_SYMBOLS configs declare, and the boards creating partial binman images including an SPL without a U-Boot the symbol is referring to.
Nice ! Thanks !
participants (2)
-
Alper Nebi Yasak -
Marek Vasut