[U-Boot] [PATCH] imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode

Wed Sep 4 10:35:54 UTC 2019

On Wed, 4 Sep 2019 11:54:40 +0200
Lukasz Majewski <lukma at denx.de> wrote:

> Hi Stefano, Heiko,
> 
> > On 04/09/19 10:46, Lukasz Majewski wrote:  
> > > Hi Heiko,
> > >     
> > >> Hello Lukasz,
> > >>
> > >> added Stefano to cc as he is the imx custodian.    
> > > 
> > > I've relied on patman ... Thanks for adding Stefano to CC.
> > >     
> > >>
> > >> Am 03.09.2019 um 23:43 schrieb Lukasz Majewski:    
> > >>> 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.
> > >>>
> > >>> Signed-off-by: Lukasz Majewski <lukma at denx.de>
> > >>>
> > >>>
> > >>> ---
> > >>>
> > >>> 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      |  7 +++++++
> > >>>   2 files changed, 18 insertions(+)      
> > >>
> > >> I have just similiar change for an imx6ull based board ...    
> > > 
> > > Also one more of my boards uses this trick (i.MX28 based one).
> > >     
> > >> just antoher usecase: In factory empty board boots into USB SDP
> > >> mode, and SPL gets loaded with usb_loader ... SPL detects a sd
> > >> card (there is no sd card slot mounted, mmc boot is disabled!
> > >> They attach only one for installing software)    
> > > 
> > > So there is no dedicated SD card slot (also the mmc is disabled on
> > > that point).
> > > Instead, in the factory the sd card is attached to pads - just for
> > > this time.
> > >     
> > >> and boots U-Boot and system from sd card.
> > >> Than all software gets installed fully automated with the help of
> > >> swupdate ...    
> > > 
> > > Ok.
> > >     
> > >>    
> > >>> 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      
> > >>
> > >> Just dummy question .. couldn;t we switch to use always
> > >> boot_device?    
> > > 
> > > Stefano has provided some rationale for the need of
> > > spl_boot_device() in the previous version of this fix [1]:
> > > 
> > > https://patchwork.ozlabs.org/patch/796237/
> > > 
> > >     
> > >>
> > >> In my case I had a board specific:
> > >>
> > >> void board_boot_order(u32 *spl_boot_list)
> > >> {
> > >>          spl_boot_list[0] = BOOT_DEVICE_MMC1;
> > >>          spl_boot_list[1] = BOOT_DEVICE_SPI;
> > >>          spl_boot_list[2] = BOOT_DEVICE_BOARD;
> > >>          spl_boot_list[3] = BOOT_DEVICE_NONE;
> > >> }
> > >>
> > >> which leads that BOOT_DEVICE_MMC1 is passed to spl_boot_mode()
> > >>  
> > > 
> > > Is your board normally booting from MMC or SPI-NOR (from where SoC
> > > ROM loads SPL) ?
> > >     
> > >>
> > >> If MMC1 is not found, it tries to boot U-Boot from SPI, and if
> > >> this is empty, as fallback board go into USB SDP mode.
> > >>
> > >> The weak function of board_boot_order is:
> > >>
> > >> __weak void board_boot_order(u32 *spl_boot_list)
> > >> {
> > >>          spl_boot_list[0] = spl_boot_device();
> > >> }
> > >>
> > >> which is exactly what we pass to spl_boot_mode() ... so instead
> > >> calling spl_boot_device() twice we can use the passed boot_device
> > >> ... and save the ifdef and new Kconfig option.
> > >>
> > >> But may I oversee something ?    
> > > 
> > > Please read the Stefano's reply from [1] - the spl_boot_device()
> > > has a valid use cases as well.    
> > 
> > Nevertheless, why do we need to add a new compiler switch if this
> > can be check into board code ? You just need that spl_boot_device()
> > returns the device you want (MMC for falcon boot).   
> 
> Current status:
> 
> git grep -n "spl_boot_mode" | grep weak
> common/spl/spl_mmc.c:287:u32 __weak spl_boot_mode(const u32
> boot_device)
> 
> git grep -n "board_boot_order" | grep weak
> common/spl/spl.c:491:__weak void board_boot_order(u32 *spl_boot_list)
> 
> 
> 
> 
> The spl_boot_device() is defined in arch/arm/mach-imx/spl.c [*] as a
> NON __weak function:
> git grep -n "spl_boot_device" | grep weak
> 
> 
> 
> Shall I make this function [*] as __weak and provide override for it
> in the board file?
> 
> The problem is in the call of spl_boot_mode() (which is overridden
> already in arch/arm/mach-imx/spl.c) at common/spl/spl_mmc.c
> 
> Which then calls spl_boot_device() [**], which may return (correctly)
> SPI-NOR for eMMC.
> 
> 
> As is now spl_boot_device() is not a __weak function.
> 
> > Why do you need to
> > force it in this way ?  
> 
> The option is to use the proposed patch to make spl_boot_device as a
> weak function.

The above text is a bit misleading - better version below.

The solution would be:

1. Use the proposed patch (with Kconfig option)

2. Define spl_boot_device as weak and override it in board file.
   However, it is not the preferred solution as spl_boot_device() on my
   setup correctly returns SPI-NOR (as SOC ROM loads SPL from SPI-NOR,
   not eMMC).

> 
> > 
> > Regards,
> > Stefano
> >   
> > >     
> > >>    
> > >>>   	/* 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..3460beb62d59 100644
> > >>> --- a/common/spl/Kconfig
> > >>> +++ b/common/spl/Kconfig
> > >>> @@ -607,6 +607,13 @@ 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's falcon boot from MMC"
> > >>> +	depends on SPL_MMC_SUPPORT
> > >>> +	default n
> > >>> +	help
> > >>> +	  Force SPL's falcon boot to use MMC device for Linux
> > >>> kernel booting.      
> > >>
> > >> Hmm... falcon boot is just one use case. I would drop "falcon
> > >> boot" It just to force boot from MMC.    
> > > 
> > > Ok. I will rewrite the help message.
> > >     
> > >>    
> > >>> +
> > >>>   config SPL_MMC_TINY
> > >>>   	bool "Tiny MMC framework in SPL"
> > >>>   	depends on SPL_MMC_SUPPORT
> > >>>       
> > >>
> > >> bye,
> > >> Heiko    
> > > 
> > > 
> > > 
> > > Best regards,
> > > 
> > > Lukasz Majewski
> > > 
> > > --
> > > 
> > > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> > > Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> > > lukma at denx.de   
> > 
> >   
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> lukma at denx.de

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190904/70f120a5/attachment.sig>