[U-Boot] [PATCH] imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode
Heiko Schocher
hs at denx.de
Thu Sep 5 10:18:30 UTC 2019
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 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).
>>
>> 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 ...
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
>
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs at denx.de
More information about the U-Boot
mailing list