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