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

[Stefano Babic <sbabic@denx.de>]

On 09/09/19 10:02, Lukasz Majewski wrote:
> Hi Heiko, Stefano
> 
>> Hello Lukasz,
>>
>> Am 05.09.2019 um 11:42 schrieb Lukasz Majewski:
>>> Hi Stefano,
>>>   
>>>> On 04/09/19 12:35, Lukasz Majewski wrote:  
>>>>> On Wed, 4 Sep 2019 11:54:40 +0200
>>>>> Lukasz Majewski <lukma@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@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).  
>>>>
>>>> Ok, but is a wek not thought for this purpose ?  
>>>
>>> Making the spl_boot_device() __weak would be a fix only for a
>>> specific appearance of spl_boot_device() in arch/arm/mach-imx/spl.c
>>> (L178): 
>>> ------------------>8--------------------------  
>>> #if defined(CONFIG_SPL_MMC_SUPPORT)
>>>
>>> /* called from spl_mmc to see type of boot mode for storage (RAW or
>>> FAT) */ u32 spl_boot_mode(const u32 boot_device)
>>> {
>>> 	switch (spl_boot_device()) {
>>> 	/* for MMC return either RAW or FAT mode */
>>> ------------------8<--------------------------
>>>
>>> The above code is only executed when SPL_MMC_SUPPORT is enabled and
>>> IMHO the spl_boot_device() call assumes that one boots up from eMMC
>>> (but not from SPI-NOR).  
>>
>> Indeed, it is not that easy ... so forget my previous commment.
>>
>>> I'm (in a first place) curious if this code is correct - the
>>> boot_device is passed as a parameter to this function, but then it
>>> is ignored.  
>>
>> Yes, that was also my first thought, so I simply replaced
>>
>> diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
>> index 4023f916ee..7fabec206b 100644
>> --- a/arch/arm/mach-imx/spl.c
>> +++ b/arch/arm/mach-imx/spl.c
>> @@ -178,7 +178,7 @@ 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) {
>> -       switch (spl_boot_device()) {
>> +       switch (boot_device) {
>>          /* for MMC return either RAW or FAT mode */
>>          case BOOT_DEVICE_MMC1:
>>          case BOOT_DEVICE_MMC2:
>>
>> but I see Stefanos arguments:
>> https://patchwork.ozlabs.org/patch/796237/#1735852
>>
>> So your current patch seems the best solution for me ...
>>
> 
> Can I assume that we do have a consensus here?
> 
> If yes - I will prepare v2 of this patch taking in the comments
> regarding Heiko's production flashing comments.

ok, fine.

Regards,
Stefano

> 
>> bye,
>> Heiko
>>
>>
>>>
>>>   
>>>> If it is weak, you can
>>>> change it and decide to return the device you want. You could also
>>>> check if Falco boot is enabled, at compile time with
>>>> CONFIG_SPL_OS_BOOT or at runtime with spl_start_uboot(). Should it
>>>> not be enough ?
>>>>
>>>> Regards,
>>>> Stefano
>>>>  
>>>>>      
>>>>>>     
>>>>>>>
>>>>>>> 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  
>>>>
>>>>  
>>>
>>>
>>>
>>> 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
> 


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================