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

[Marek Vasut]

On 4/8/20 2:42 PM, Harald Seiler wrote:
> Hello,

Hi,

> 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

Can you define "this" ? What is not correct, that the patch was rejected
or this patch ?

> 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().

Note that the entire madness above was needed for 8997de292a8b to work.

ARM: at91: ma5d4: Boot from MMC2 when using SAM-BA

Continue loading U-Boot from MMC2 when the SPL was loaded using SAM-BA
loader. This allows the board to boot system from the removable media
instead of the eMMC, which is useful for commissioning purposes. When
booting from the eMMC, always boot from it as it is not possible to
boot from the SD interface directly.

> 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).

Yes

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

Yes

> 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

I think you are correct about this.