[U-Boot] [PATCH] imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode
Lukasz Majewski
lukma at denx.de
Mon Sep 9 08:02:46 UTC 2019
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 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 ...
>
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.
> 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
-------------- 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/20190909/b6a952d9/attachment.sig>
More information about the U-Boot
mailing list