[PATCH 0/5] Fix spl_mmc_boot_mode() implementation for IMX
The spl_mmc_boot_mode() (formerly spl_boot_mode()) implementation for IMX has grown quite big over time [1]. It has also started to steer away from what it is meant to do in its current form and breaks some use-cases of the SPL. Namely, the issue is that nowadays SPL can attempt loading U-Boot from multiple MMC devices and this function needs to return the correct mode for each one which it currently does not.
There have been multiple attempts to fix this in the past, starting with a (rejected) patch from Anatolij [2]. Most recently, a patch from Lukasz [3] was merged which introduces a config option CONFIG_SPL_FORCE_MMC_BOOT.
I think this is going in the wrong direction when the real solution lies in cutting the function back down as much as possible. IMX currently has the biggest implementation in the tree and I don't really see a reason why all that complexity is necessary. I have spend some time to find out the full story here and have summarized that in a previous mail [4].
To cut it as short as possible: I believe CONFIG_SPL_FORCE_MMC_BOOT is superfluous as enabling it is the only correct behavior and CONFIG_SPL_FORCE_MMC_BOOT=n is faulty and will lead to problems when customizing the SPL boot sequence e.g. using board_boot_order().
Thus, this series contains Anatolij's original patch, reverts the introduction of CONFIG_SPL_FORCE_MMC_BOOT and cleans up the implementation a bit. One thing which is left out, which still needs to happen in the future is getting rid of the big #if for IMX{7,8,8M}. I don't have hardware to test IMX7 or 8 right now and haven't looked too deep into what needs to be done here.
I'm pretty certain this series should not break any use-case and if it does anyway, a breakage caused by it indicates the need to fix SPL configuration for that board. I'm happy to help with that if the need arises.
[1]: https://gitlab.denx.de/u-boot/u-boot/-/blob/v2020.04/arch/arm/mach-imx/spl.c... [2]: https://patchwork.ozlabs.org/patch/796237/ [3]: https://gitlab.denx.de/u-boot/u-boot/-/commit/772b55723bcbe8ebe84f579d9cdc83... [4]: https://lists.denx.de/pipermail/u-boot/2020-April/405979.html
Anatolij Gustschin (1): imx: spl: return boot mode for asked MMC device in spl_mmc_boot_mode()
Harald Seiler (4): Revert "imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode" Revert "imx: defconfig: Enable CONFIG_SPL_FORCE_MMC_BOOT" imx: spl: Remove ifdefs in spl_mmc_boot_mode() imx: spl: Fix use of removed SPL_FAT_SUPPORT config
arch/arm/mach-imx/spl.c | 49 ++++++++++++------------------------- common/spl/Kconfig | 9 ------- configs/display5_defconfig | 1 - configs/imx28_xea_defconfig | 1 - 4 files changed, 16 insertions(+), 44 deletions(-)
From: Anatolij Gustschin agust@denx.de
Boards may extend or re-define the boot list in their board_boot_order() function by modifying spl_boot_list. E.g. a board might boot SPL from a slow SPI NOR flash and then load the U-Boot from an eMMC or SD-card. Or it might use additional MMC boot device in spl_boot_list for cases when the image in SPI NOR flash is not found, so it could fall back to eMMC, SD-card or another boot device.
Getting the MMC boot mode in spl_mmc will fail when we are trying to boot from an MMC device in the spl_boot_list and the original board boot mode (as returned by spl_boot_device()) is not an MMC boot mode. Fix it by checking the asked MMC boot device from the spl_mmc_boot_mode() argument.
Signed-off-by: Anatolij Gustschin agust@denx.de --- arch/arm/mach-imx/spl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c index 49bb3b928da1..e5835150a06d 100644 --- a/arch/arm/mach-imx/spl.c +++ b/arch/arm/mach-imx/spl.c @@ -229,7 +229,7 @@ u32 spl_mmc_boot_mode(const u32 boot_device) #ifdef CONFIG_SPL_FORCE_MMC_BOOT switch (boot_device) { #else - switch (spl_boot_device()) { + switch (boot_device) { #endif /* for MMC return either RAW or FAT mode */ case BOOT_DEVICE_MMC1:
From: Anatolij Gustschin agust@denx.de Boards may extend or re-define the boot list in their board_boot_order() function by modifying spl_boot_list. E.g. a board might boot SPL from a slow SPI NOR flash and then load the U-Boot from an eMMC or SD-card. Or it might use additional MMC boot device in spl_boot_list for cases when the image in SPI NOR flash is not found, so it could fall back to eMMC, SD-card or another boot device. Getting the MMC boot mode in spl_mmc will fail when we are trying to boot from an MMC device in the spl_boot_list and the original board boot mode (as returned by spl_boot_device()) is not an MMC boot mode. Fix it by checking the asked MMC boot device from the spl_mmc_boot_mode() argument. Signed-off-by: Anatolij Gustschin agust@denx.de
Applied to u-boot-imx, master, thanks !
Best regards, Stefano Babic
The CONFIG_SPL_FORCE_MMC_BOOT config flag is not needed as its behavior is the correct one in all cases; using spl_boot_device() instead of the boot_device parameter will lead to inconsistency issues, for example, when a board_boot_order() is defined. In fact, this is the reason the parameter was introduced in the first place, in commit 2b1cdafa9fdd ("common: Pass the boot device into spl_boot_mode()").
This reverts commit 772b55723bcbe8ebe84f579d9cdc831d8e18579d.
Link: https://lists.denx.de/pipermail/u-boot/2020-April/405979.html Signed-off-by: Harald Seiler hws@denx.de --- arch/arm/mach-imx/spl.c | 11 ----------- common/spl/Kconfig | 9 --------- 2 files changed, 20 deletions(-)
diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c index e5835150a06d..e3f51e45edf2 100644 --- a/arch/arm/mach-imx/spl.c +++ b/arch/arm/mach-imx/spl.c @@ -219,18 +219,7 @@ u32 spl_mmc_boot_mode(const u32 boot_device) hang(); } #else -/* - * When CONFIG_SPL_FORCE_MMC_BOOT is defined the 'boot_device' is used - * unconditionally to decide about device to use for booting. - * This is crucial for falcon boot mode, when board boots up (i.e. ROM - * loads SPL) from slow SPI-NOR memory and afterwards the SPL's 'falcon' boot - * mode is used to load Linux OS from eMMC partition. - */ -#ifdef CONFIG_SPL_FORCE_MMC_BOOT switch (boot_device) { -#else - switch (boot_device) { -#endif /* for MMC return either RAW or FAT mode */ case BOOT_DEVICE_MMC1: case BOOT_DEVICE_MMC2: diff --git a/common/spl/Kconfig b/common/spl/Kconfig index 9d52b75cb434..344fb8003f63 100644 --- a/common/spl/Kconfig +++ b/common/spl/Kconfig @@ -669,15 +669,6 @@ config SPL_MMC_SUPPORT this option to build the drivers in drivers/mmc as part of an SPL build.
-config SPL_FORCE_MMC_BOOT - bool "Force SPL booting from MMC" - depends on SPL_MMC_SUPPORT - default n - help - Force SPL to use MMC device for Linux kernel booting even when the - SoC ROM recognized boot medium is not eMMC/SD. This is crucial for - factory or 'falcon mode' booting. - config SPL_MMC_TINY bool "Tiny MMC framework in SPL" depends on SPL_MMC_SUPPORT
The CONFIG_SPL_FORCE_MMC_BOOT config flag is not needed as its behavior is the correct one in all cases; using spl_boot_device() instead of the boot_device parameter will lead to inconsistency issues, for example, when a board_boot_order() is defined. In fact, this is the reason the parameter was introduced in the first place, in commit 2b1cdafa9fdd ("common: Pass the boot device into spl_boot_mode()"). This reverts commit 772b55723bcbe8ebe84f579d9cdc831d8e18579d. Link: https://lists.denx.de/pipermail/u-boot/2020-April/405979.html Signed-off-by: Harald Seiler hws@denx.de
Applied to u-boot-imx, master, thanks !
Best regards, Stefano Babic
CONFIG_SPL_FORCE_MMC_BOOT was removed in a previous patch as its behavior is the correct one in all cases. Remove all uses of it from defconfigs.
This reverts commit 3201e5b444ae3a13aa31e8b5101ad38d7ff0640d and removes CONFIG_SPL_FORCE_MMC_BOOT from the imx28_xea defconfig.
Signed-off-by: Harald Seiler hws@denx.de --- configs/display5_defconfig | 1 - configs/imx28_xea_defconfig | 1 - 2 files changed, 2 deletions(-)
diff --git a/configs/display5_defconfig b/configs/display5_defconfig index 9026c17f3fb5..f982191f7f53 100644 --- a/configs/display5_defconfig +++ b/configs/display5_defconfig @@ -38,7 +38,6 @@ CONFIG_SPL_DMA=y CONFIG_SPL_ENV_SUPPORT=y CONFIG_SPL_SAVEENV=y CONFIG_SPL_I2C_SUPPORT=y -CONFIG_SPL_FORCE_MMC_BOOT=y CONFIG_SPL_OS_BOOT=y CONFIG_SPL_SPI_LOAD=y CONFIG_SYS_SPI_U_BOOT_OFFS=0x20000 diff --git a/configs/imx28_xea_defconfig b/configs/imx28_xea_defconfig index 2d49b664deae..79fa08bbab2b 100644 --- a/configs/imx28_xea_defconfig +++ b/configs/imx28_xea_defconfig @@ -28,7 +28,6 @@ CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR=y CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR=0 CONFIG_SUPPORT_EMMC_BOOT_OVERRIDE_PART_CONFIG=y CONFIG_SPL_DMA=y -CONFIG_SPL_FORCE_MMC_BOOT=y CONFIG_SPL_MMC_TINY=y CONFIG_SPL_OS_BOOT=y CONFIG_SPL_PAYLOAD="u-boot.img"
CONFIG_SPL_FORCE_MMC_BOOT was removed in a previous patch as its behavior is the correct one in all cases. Remove all uses of it from defconfigs. This reverts commit 3201e5b444ae3a13aa31e8b5101ad38d7ff0640d and removes CONFIG_SPL_FORCE_MMC_BOOT from the imx28_xea defconfig. Signed-off-by: Harald Seiler hws@denx.de
Applied to u-boot-imx, master, thanks !
Best regards, Stefano Babic
It is hard to read code which contains nested ifdef blocks. Replace them with normal if-blocks and the IS_ENABLED() macro. This is not only more readable but also helps as both arms are validated by the compiler in all cases.
Signed-off-by: Harald Seiler hws@denx.de --- arch/arm/mach-imx/spl.c | 38 ++++++++++++++++---------------------- 1 file changed, 16 insertions(+), 22 deletions(-)
diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c index e3f51e45edf2..32d78b799c36 100644 --- a/arch/arm/mach-imx/spl.c +++ b/arch/arm/mach-imx/spl.c @@ -197,23 +197,19 @@ u32 spl_mmc_boot_mode(const u32 boot_device) case SD1_BOOT: case SD2_BOOT: case SD3_BOOT: -#if defined(CONFIG_SPL_FAT_SUPPORT) - return MMCSD_MODE_FS; -#else - return MMCSD_MODE_RAW; -#endif - break; + if (IS_ENABLED(CONFIG_SPL_FAT_SUPPORT)) + return MMCSD_MODE_FS; + else + return MMCSD_MODE_RAW; case MMC1_BOOT: case MMC2_BOOT: case MMC3_BOOT: -#if defined(CONFIG_SPL_FAT_SUPPORT) - return MMCSD_MODE_FS; -#elif defined(CONFIG_SUPPORT_EMMC_BOOT) - return MMCSD_MODE_EMMCBOOT; -#else - return MMCSD_MODE_RAW; -#endif - break; + if (IS_ENABLED(CONFIG_SPL_FAT_SUPPORT)) + return MMCSD_MODE_FS; + else if (IS_ENABLED(CONFIG_SUPPORT_EMMC_BOOT)) + return MMCSD_MODE_EMMCBOOT; + else + return MMCSD_MODE_RAW; default: puts("spl: ERROR: unsupported device\n"); hang(); @@ -224,14 +220,12 @@ u32 spl_mmc_boot_mode(const u32 boot_device) case BOOT_DEVICE_MMC1: case BOOT_DEVICE_MMC2: case BOOT_DEVICE_MMC2_2: -#if defined(CONFIG_SPL_FS_FAT) - return MMCSD_MODE_FS; -#elif defined(CONFIG_SUPPORT_EMMC_BOOT) - return MMCSD_MODE_EMMCBOOT; -#else - return MMCSD_MODE_RAW; -#endif - break; + if (IS_ENABLED(CONFIG_SPL_FS_FAT)) + return MMCSD_MODE_FS; + else if (IS_ENABLED(CONFIG_SUPPORT_EMMC_BOOT)) + return MMCSD_MODE_EMMCBOOT; + else + return MMCSD_MODE_RAW; default: puts("spl: ERROR: unsupported device\n"); hang();
It is hard to read code which contains nested ifdef blocks. Replace them with normal if-blocks and the IS_ENABLED() macro. This is not only more readable but also helps as both arms are validated by the compiler in all cases. Signed-off-by: Harald Seiler hws@denx.de
Applied to u-boot-imx, master, thanks !
Best regards, Stefano Babic
CONFIG_SPL_FAT_SUPPORT was removed in commit 0c3a9ed409a5 ("spl: Kconfig: Replace CONFIG_SPL_FAT_SUPPORT with CONFIG_SPL_FS_FAT"). Fixup a leftover use of the symbol.
Fixes: 9d86dbd9cf9d ("imx: spl: implement spl_boot_mode for i.MX7/8/8M") Signed-off-by: Harald Seiler hws@denx.de --- arch/arm/mach-imx/spl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c index 32d78b799c36..fd3fa046002a 100644 --- a/arch/arm/mach-imx/spl.c +++ b/arch/arm/mach-imx/spl.c @@ -197,14 +197,14 @@ u32 spl_mmc_boot_mode(const u32 boot_device) case SD1_BOOT: case SD2_BOOT: case SD3_BOOT: - if (IS_ENABLED(CONFIG_SPL_FAT_SUPPORT)) + if (IS_ENABLED(CONFIG_SPL_FS_FAT)) return MMCSD_MODE_FS; else return MMCSD_MODE_RAW; case MMC1_BOOT: case MMC2_BOOT: case MMC3_BOOT: - if (IS_ENABLED(CONFIG_SPL_FAT_SUPPORT)) + if (IS_ENABLED(CONFIG_SPL_FS_FAT)) return MMCSD_MODE_FS; else if (IS_ENABLED(CONFIG_SUPPORT_EMMC_BOOT)) return MMCSD_MODE_EMMCBOOT;
CONFIG_SPL_FAT_SUPPORT was removed in commit 0c3a9ed409a5 ("spl: Kconfig: Replace CONFIG_SPL_FAT_SUPPORT with CONFIG_SPL_FS_FAT"). Fixup a leftover use of the symbol. Fixes: 9d86dbd9cf9d ("imx: spl: implement spl_boot_mode for i.MX7/8/8M") Signed-off-by: Harald Seiler hws@denx.de
Applied to u-boot-imx, master, thanks !
Best regards, Stefano Babic
participants (2)
-
Harald Seiler -
sbabic＠denx.de