[PATCH 0/5] Fix spl_mmc_boot_mode() implementation for IMX

[PATCH 0/5] Fix spl_mmc_boot_mode() implementation for IMX

[Harald Seiler]

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#L192-251
[2]: https://patchwork.ozlabs.org/patch/796237/
[3]: https://gitlab.denx.de/u-boot/u-boot/-/commit/772b55723bcbe8ebe84f579d9cdc831d8e18579d
[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(-)

-- 
2.26.1

[PATCH 1/5] imx: spl: return boot mode for asked MMC device in spl_mmc_boot_mode()

[Harald Seiler]

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:
-- 
2.26.1

[PATCH 2/5] Revert "imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode"

[Harald Seiler]

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
-- 
2.26.1

[PATCH 3/5] Revert "imx: defconfig: Enable CONFIG_SPL_FORCE_MMC_BOOT"

[Harald Seiler]

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"
-- 
2.26.1

[PATCH 4/5] imx: spl: Remove ifdefs in spl_mmc_boot_mode()

[Harald Seiler]

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();
-- 
2.26.1

[PATCH 5/5] imx: spl: Fix use of removed SPL_FAT_SUPPORT config

[Harald Seiler]

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;
-- 
2.26.1

[PATCH 3/5] Revert "imx: defconfig: Enable CONFIG_SPL_FORCE_MMC_BOOT"

[sbabic at denx.de]

> 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

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[PATCH 1/5] imx: spl: return boot mode for asked MMC device in spl_mmc_boot_mode()

[sbabic at denx.de]

> 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

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[PATCH 4/5] imx: spl: Remove ifdefs in spl_mmc_boot_mode()

[sbabic at denx.de]

> 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

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[PATCH 2/5] Revert "imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode"

[sbabic at denx.de]

> 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

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[PATCH 5/5] imx: spl: Fix use of removed SPL_FAT_SUPPORT config

[sbabic at denx.de]

> 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

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================