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

[Harald Seiler]

Hello Marek,

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

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

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