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

Stefano Babic
2019-09-05T08:08:05Z

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

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