[PATCH v2] imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode

[Harald Seiler]

Hello,

On Mon, 2019-09-09 at 15:32 +0200, Lukasz Majewski wrote:
> This change tries to fix the following problem:
> 
> - The board boots (to be more precise - ROM loads SPL) from a slow SPI-NOR
>   memory.
>   As a result the spl_boot_device() will return SPI-NOR as a boot device
>   (which is correct).
> 
> - The problem is that in 'falcon boot' the eMMC is used as a boot medium to
>   load kernel from its partition.
>   Calling spl_boot_device() will break things as it returns SPI-NOR device.
> 
> To fix this issue the new CONFIG_SPL_FORCE_MMC_BOOT Kconfig flag is
> introduced to handle this special use case. By default it is not defined,
> so there is no change in the legacy code flow.

I want to pick up this discussion (and the previous discussion about
Anatolij's rejected patch [1]) again, because this does not seem correct
to me.  Also, through the addition of imx8 support, the state has worsened
further and I'd like to have this become more consistent again.

Digging deep into the history, the `boot_device` parameter to
`spl_boot_mode` was introduced by Marek in commit 2b1cdafa9fdd ("common:
Pass the boot device into spl_boot_mode()").  The intention was to fix
exactly the problem which Anatolij encountered.  For reference:

    common: Pass the boot device into spl_boot_mode()
   
    The SPL code already knows which boot device it calls the spl_boot_mode()
    on, so pass that information into the function. This allows the code of
    spl_boot_mode() avoid invoking spl_boot_device() again, but it also lets
    board_boot_order() correctly alter the behavior of the boot process.
   
    The later one is important, since in certain cases, it is desired that
    spl_boot_device() return value be overriden using board_boot_order().

It seems to me that using spl_boot_device() instead of the `boot_device`
parameter cannot be correct here (If I am wrong about the following,
please correct me!):

spl_boot_mode() is essentially a lookup function which is used by the SPL
MMC driver (here [2]) to find out the 'mode' of the currently attempted
MMC device.  That is, for each MMC device, it should tell the driver
whether this device has a FAT/ext4 filesystem (MMCSD_MODE_FS), is using an
eMMC boot-partition (MMCSD_MODE_EMMCBOOT), or should be accessed directly
(MMCSD_MODE_RAW).

spl_boot_device() returns the device which SPL was booted from.

Now because in most cases U-Boot Proper is loaded from the same MMC device
which the SPL was originally loaded from, the current code often
mistakenly does the right thing.  But when this is not the case (e.g.
because a board_boot_order() was defined), it is obviously not correct to
return the mode of the MMC device which SPL was loaded from instead of the
mode of the device which the MMC driver is currently attempting to access.

So, I think the function should in all circumstances use its `boot_device`
parameter to behave correctly (and all other implementations do this, from
what I can tell).  It might make sense to rename it, though.  It is not
really about the 'spl boot mode', but much more about 'mmc device mode'.

I'd send a patch-series but first I'd like some input whether I am correct
about this ...

[1]: https://patchwork.ozlabs.org/patch/796237/
[2]: https://gitlab.denx.de/u-boot/u-boot/-/blob/v2020.01/common/spl/spl_mmc.c#L346

> Signed-off-by: Lukasz Majewski <lukma at denx.de>
> ---
> 
> Changes in v2:
> - Update/enhance the Kconfig description of the SPL_FORCE_MMC_BOOT.
> 
> This patch is a follow up of previous discussion/fix:
> https://patchwork.ozlabs.org/patch/796237/
> 
> Travis-CI:
> https://travis-ci.org/lmajewski/u-boot-dfu/builds/580272414
> ---
>  arch/arm/mach-imx/spl.c | 11 +++++++++++
>  common/spl/Kconfig      |  9 +++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
> index 1f230aca3397..daa3d8f7ed94 100644
> --- a/arch/arm/mach-imx/spl.c
> +++ b/arch/arm/mach-imx/spl.c
> @@ -178,7 +178,18 @@ int g_dnl_bind_fixup(struct usb_device_descriptor *dev, const char *name)
>  /* called from spl_mmc to see type of boot mode for storage (RAW or FAT) */
>  u32 spl_boot_mode(const u32 boot_device)
>  {
> +/*
> + * 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 (spl_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 f467eca2be72..fe800840beb6 100644
> --- a/common/spl/Kconfig
> +++ b/common/spl/Kconfig
> @@ -607,6 +607,15 @@ 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

-- 
Harald