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