[U-Boot] [PATCH] imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode
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(+)
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
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 Linuxkernel 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 Linuxkernel 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 Linuxkernel 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 Linuxkernel 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:
Use the proposed patch (with Kconfig option)
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 Linuxkernel 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:
Use the proposed patch (with Kconfig option)
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 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:
Use the proposed patch (with Kconfig option)
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:
Use the proposed patch (with Kconfig option)
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:
Use the proposed patch (with Kconfig option)
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
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:
Use the proposed patch (with Kconfig option)
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
participants (3)
-
Heiko Schocher -
Lukasz Majewski -
Stefano Babic