[U-Boot] [PATCH] imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode
Lukasz Majewski
lukma at denx.de
Thu Sep 5 09:42:29 UTC 2019
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).
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.
> 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
-------------- 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/20190905/c8916b4d/attachment.sig>
More information about the U-Boot
mailing list