On 04/09/19 12:35, Lukasz Majewski wrote: > On Wed, 4 Sep 2019 11:54:40 +0200 > Lukasz Majewski <lu...@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 <lu...@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: >>>> lu...@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: >> lu...@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: lu...@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: sba...@denx.de ===================================================================== _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot