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). 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: 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
pgpIwkw1JkyYP.pgp
Description: OpenPGP digital signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot