Message ID | 20190903214358.5127-1-lukma@denx.de |
---|---|
State | Changes Requested |
Delegated to: | Stefano Babic |
Headers | show |
Series | [U-Boot] imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode | expand |
Hello Lukasz, added Stefano to cc as he is the imx custodian. 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 <lukma@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 ... 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) and boots U-Boot and system from sd card. Than all software gets installed fully automated with the help of swupdate ... > 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? 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() 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 ? > /* 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. > + > config SPL_MMC_TINY > bool "Tiny MMC framework in SPL" > depends on SPL_MMC_SUPPORT > bye, Heiko
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 <lukma@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. > > > /* 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: lukma@denx.de
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 <lukma@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). Why do you need to force it in this way ? 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: lukma@denx.de >
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 <lukma@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. > > 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: > > lukma@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: lukma@denx.de
On Wed, 4 Sep 2019 11:54:40 +0200 Lukasz Majewski <lukma@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 <lukma@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: > > > lukma@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: > lukma@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: lukma@denx.de
On 04/09/19 12:35, Lukasz Majewski wrote: > On Wed, 4 Sep 2019 11:54:40 +0200 > Lukasz Majewski <lukma@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 <lukma@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: >>>> lukma@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: >> lukma@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: lukma@denx.de >
Hi Stefano, > On 04/09/19 12:35, Lukasz Majewski wrote: > > On Wed, 4 Sep 2019 11:54:40 +0200 > > Lukasz Majewski <lukma@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 <lukma@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: lukma@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: > >> lukma@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: > > lukma@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: lukma@denx.de
Hello Stefano, Lukasz, Am 05.09.2019 um 10:08 schrieb Stefano Babic: > On 04/09/19 12:35, Lukasz Majewski wrote: >> On Wed, 4 Sep 2019 11:54:40 +0200 >> Lukasz Majewski <lukma@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 <lukma@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 ? I am fine with a weak function only. bye, Heiko
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 <lukma@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 <lukma@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 ... 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: lukma@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: >>>> lukma@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: >>> lukma@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: lukma@denx.de >
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 <lukma@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 <lukma@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: lukma@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: lukma@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: > >>> lukma@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: > > lukma@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: lukma@denx.de
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 <lukma@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 <lukma@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: lukma@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: lukma@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: >>>>> lukma@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: >>> lukma@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: lukma@denx.de >
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 /* 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. + config SPL_MMC_TINY bool "Tiny MMC framework in SPL" depends on SPL_MMC_SUPPORT
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 <lukma@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(+)