On 09/09/19 10:02, Lukasz Majewski wrote: > 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 <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 ? >>> >>> 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.
ok, fine. Regards, Stefano > >> 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: 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 >>>> >>>> >>> >>> >>> >>> 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