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

[Marek Vasut]

On 4/8/20 4:09 PM, Harald Seiler wrote:
> Hello Marek,

Hi,

> On Wed, 2020-04-08 at 15:45 +0200, Marek Vasut wrote:
>> 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 ?
> 
> Right, sorry.  I'm talking about the use of spl_boot_device() in the
> switch-statement of spl_boot_mode().  That means, I think rejecting
> Anatolij's original patch was wrong and this patch should not have been
> necessary as what now would be CONFIG_SPL_FORCE_MMC_BOOT=y is the only
> correct behavior (but it is not the default).

Right, you want to be able to override -- at board level -- the boot
device used for the next stage. So Anatolij's patch was indeed OK and we
shouldn't add extra config options for that.

>>> 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.
> 
> I see.  Well, and trying to do the same thing on an IMX would not work at
> the moment, because of the issue I am trying to describe.

Yep, just adding some extra context here.

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