| Message ID | 20190909133214.30699-1-lukma@denx.de |
|---|---|
| State | Accepted |
| Commit | 772b55723bcbe8ebe84f579d9cdc831d8e18579d |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | [U-Boot,v2] imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode | expand |
Hello, On Mon, 2019-09-09 at 15:32 +0200, Lukasz Majewski wrote: > 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. I want to pick up this discussion (and the previous discussion about Anatolij's rejected patch [1]) again, because this does not seem correct to me. Also, through the addition of imx8 support, the state has worsened further and I'd like to have this become more consistent again. Digging deep into the history, the `boot_device` parameter to `spl_boot_mode` was introduced by Marek in commit 2b1cdafa9fdd ("common: Pass the boot device into spl_boot_mode()"). The intention was to fix exactly the problem which Anatolij encountered. For reference: common: Pass the boot device into spl_boot_mode() The SPL code already knows which boot device it calls the spl_boot_mode() on, so pass that information into the function. This allows the code of spl_boot_mode() avoid invoking spl_boot_device() again, but it also lets board_boot_order() correctly alter the behavior of the boot process. The later one is important, since in certain cases, it is desired that spl_boot_device() return value be overriden using board_boot_order(). It seems to me that using spl_boot_device() instead of the `boot_device` parameter cannot be correct here (If I am wrong about the following, please correct me!): spl_boot_mode() is essentially a lookup function which is used by the SPL MMC driver (here [2]) to find out the 'mode' of the currently attempted MMC device. That is, for each MMC device, it should tell the driver whether this device has a FAT/ext4 filesystem (MMCSD_MODE_FS), is using an eMMC boot-partition (MMCSD_MODE_EMMCBOOT), or should be accessed directly (MMCSD_MODE_RAW). spl_boot_device() returns the device which SPL was booted from. Now because in most cases U-Boot Proper is loaded from the same MMC device which the SPL was originally loaded from, the current code often mistakenly does the right thing. But when this is not the case (e.g. because a board_boot_order() was defined), it is obviously not correct to return the mode of the MMC device which SPL was loaded from instead of the mode of the device which the MMC driver is currently attempting to access. So, I think the function should in all circumstances use its `boot_device` parameter to behave correctly (and all other implementations do this, from what I can tell). It might make sense to rename it, though. It is not really about the 'spl boot mode', but much more about 'mmc device mode'. I'd send a patch-series but first I'd like some input whether I am correct about this ... [1]: https://patchwork.ozlabs.org/patch/796237/ [2]: https://gitlab.denx.de/u-boot/u-boot/-/blob/v2020.01/common/spl/spl_mmc.c#L346 > Signed-off-by: Lukasz Majewski <lukma@denx.de> > --- > > Changes in v2: > - Update/enhance the Kconfig description of the SPL_FORCE_MMC_BOOT. > > 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 | 9 +++++++++ > 2 files changed, 20 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..fe800840beb6 100644 > --- a/common/spl/Kconfig > +++ b/common/spl/Kconfig > @@ -607,6 +607,15 @@ 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 booting from MMC" > + depends on SPL_MMC_SUPPORT > + default n > + help > + Force SPL to use MMC device for Linux kernel booting even when the > + SoC ROM recognized boot medium is not eMMC/SD. This is crucial for > + factory or 'falcon mode' booting. > + > config SPL_MMC_TINY > bool "Tiny MMC framework in SPL" > depends on SPL_MMC_SUPPORT
On 4/8/20 2:42 PM, Harald Seiler wrote: > Hello, Hi, > On Mon, 2019-09-09 at 15:32 +0200, Lukasz Majewski wrote: >> 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. > > I want to pick up this discussion (and the previous discussion about > Anatolij's rejected patch [1]) again, because this Can you define "this" ? What is not correct, that the patch was rejected or this patch ? > does not seem correct > to me. Also, through the addition of imx8 support, the state has worsened > further and I'd like to have this become more consistent again. > > Digging deep into the history, the `boot_device` parameter to > `spl_boot_mode` was introduced by Marek in commit 2b1cdafa9fdd ("common: > Pass the boot device into spl_boot_mode()"). The intention was to fix > exactly the problem which Anatolij encountered. For reference: > > common: Pass the boot device into spl_boot_mode() > > The SPL code already knows which boot device it calls the spl_boot_mode() > on, so pass that information into the function. This allows the code of > spl_boot_mode() avoid invoking spl_boot_device() again, but it also lets > board_boot_order() correctly alter the behavior of the boot process. > > The later one is important, since in certain cases, it is desired that > spl_boot_device() return value be overriden using board_boot_order(). Note that the entire madness above was needed for 8997de292a8b to work. ARM: at91: ma5d4: Boot from MMC2 when using SAM-BA Continue loading U-Boot from MMC2 when the SPL was loaded using SAM-BA loader. This allows the board to boot system from the removable media instead of the eMMC, which is useful for commissioning purposes. When booting from the eMMC, always boot from it as it is not possible to boot from the SD interface directly. > It seems to me that using spl_boot_device() instead of the `boot_device` > parameter cannot be correct here (If I am wrong about the following, > please correct me!): > > spl_boot_mode() is essentially a lookup function which is used by the SPL > MMC driver (here [2]) to find out the 'mode' of the currently attempted > MMC device. That is, for each MMC device, it should tell the driver > whether this device has a FAT/ext4 filesystem (MMCSD_MODE_FS), is using an > eMMC boot-partition (MMCSD_MODE_EMMCBOOT), or should be accessed directly > (MMCSD_MODE_RAW). Yes > spl_boot_device() returns the device which SPL was booted from. Yes > Now because in most cases U-Boot Proper is loaded from the same MMC device > which the SPL was originally loaded from, the current code often > mistakenly does the right thing. But when this is not the case (e.g. > because a board_boot_order() was defined), it is obviously not correct to > return the mode of the MMC device which SPL was loaded from instead of the > mode of the device which the MMC driver is currently attempting to access. > > So, I think the function should in all circumstances use its `boot_device` > parameter to behave correctly (and all other implementations do this, from > what I can tell). It might make sense to rename it, though. It is not > really about the 'spl boot mode', but much more about 'mmc device mode'. > > I'd send a patch-series but first I'd like some input whether I am correct > about this ... > > [1]: https://patchwork.ozlabs.org/patch/796237/ > [2]: https://gitlab.denx.de/u-boot/u-boot/-/blob/v2020.01/common/spl/spl_mmc.c#L346 I think you are correct about this.
Hello Marek, On Wed, 2020-04-08 at 15:45 +0200, Marek Vasut wrote: > On 4/8/20 2:42 PM, Harald Seiler wrote: > > Hello, > > Hi, > > > On Mon, 2019-09-09 at 15:32 +0200, Lukasz Majewski wrote: > > > 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. > > > > I want to pick up this discussion (and the previous discussion about > > Anatolij's rejected patch [1]) again, because this > > Can you define "this" ? What is not correct, that the patch was rejected > or this patch ? Right, sorry. I'm talking about the use of spl_boot_device() in the switch-statement of spl_boot_mode(). That means, I think rejecting Anatolij's original patch was wrong and this patch should not have been necessary as what now would be CONFIG_SPL_FORCE_MMC_BOOT=y is the only correct behavior (but it is not the default). > > does not seem correct > > to me. Also, through the addition of imx8 support, the state has worsened > > further and I'd like to have this become more consistent again. > > > > Digging deep into the history, the `boot_device` parameter to > > `spl_boot_mode` was introduced by Marek in commit 2b1cdafa9fdd ("common: > > Pass the boot device into spl_boot_mode()"). The intention was to fix > > exactly the problem which Anatolij encountered. For reference: > > > > common: Pass the boot device into spl_boot_mode() > > > > The SPL code already knows which boot device it calls the spl_boot_mode() > > on, so pass that information into the function. This allows the code of > > spl_boot_mode() avoid invoking spl_boot_device() again, but it also lets > > board_boot_order() correctly alter the behavior of the boot process. > > > > The later one is important, since in certain cases, it is desired that > > spl_boot_device() return value be overriden using board_boot_order(). > > Note that the entire madness above was needed for 8997de292a8b to work. > > ARM: at91: ma5d4: Boot from MMC2 when using SAM-BA > > Continue loading U-Boot from MMC2 when the SPL was loaded using SAM-BA > loader. This allows the board to boot system from the removable media > instead of the eMMC, which is useful for commissioning purposes. When > booting from the eMMC, always boot from it as it is not possible to > boot from the SD interface directly. I see. Well, and trying to do the same thing on an IMX would not work at the moment, because of the issue I am trying to describe. > > It seems to me that using spl_boot_device() instead of the `boot_device` > > parameter cannot be correct here (If I am wrong about the following, > > please correct me!): > > > > spl_boot_mode() is essentially a lookup function which is used by the SPL > > MMC driver (here [2]) to find out the 'mode' of the currently attempted > > MMC device. That is, for each MMC device, it should tell the driver > > whether this device has a FAT/ext4 filesystem (MMCSD_MODE_FS), is using an > > eMMC boot-partition (MMCSD_MODE_EMMCBOOT), or should be accessed directly > > (MMCSD_MODE_RAW). > > Yes > > > spl_boot_device() returns the device which SPL was booted from. > > Yes > > > Now because in most cases U-Boot Proper is loaded from the same MMC device > > which the SPL was originally loaded from, the current code often > > mistakenly does the right thing. But when this is not the case (e.g. > > because a board_boot_order() was defined), it is obviously not correct to > > return the mode of the MMC device which SPL was loaded from instead of the > > mode of the device which the MMC driver is currently attempting to access. > > > > So, I think the function should in all circumstances use its `boot_device` > > parameter to behave correctly (and all other implementations do this, from > > what I can tell). It might make sense to rename it, though. It is not > > really about the 'spl boot mode', but much more about 'mmc device mode'. > > > > I'd send a patch-series but first I'd like some input whether I am correct > > about this ... > > > > [1]: https://patchwork.ozlabs.org/patch/796237/ > > [2]: https://gitlab.denx.de/u-boot/u-boot/-/blob/v2020.01/common/spl/spl_mmc.c#L346 > > I think you are correct about this.
On 4/8/20 4:09 PM, Harald Seiler wrote: > Hello Marek, Hi, > On Wed, 2020-04-08 at 15:45 +0200, Marek Vasut wrote: >> On 4/8/20 2:42 PM, Harald Seiler wrote: >>> Hello, >> >> Hi, >> >>> On Mon, 2019-09-09 at 15:32 +0200, Lukasz Majewski wrote: >>>> 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. >>> >>> I want to pick up this discussion (and the previous discussion about >>> Anatolij's rejected patch [1]) again, because this >> >> Can you define "this" ? What is not correct, that the patch was rejected >> or this patch ? > > Right, sorry. I'm talking about the use of spl_boot_device() in the > switch-statement of spl_boot_mode(). That means, I think rejecting > Anatolij's original patch was wrong and this patch should not have been > necessary as what now would be CONFIG_SPL_FORCE_MMC_BOOT=y is the only > correct behavior (but it is not the default). Right, you want to be able to override -- at board level -- the boot device used for the next stage. So Anatolij's patch was indeed OK and we shouldn't add extra config options for that. >>> does not seem correct >>> to me. Also, through the addition of imx8 support, the state has worsened >>> further and I'd like to have this become more consistent again. >>> >>> Digging deep into the history, the `boot_device` parameter to >>> `spl_boot_mode` was introduced by Marek in commit 2b1cdafa9fdd ("common: >>> Pass the boot device into spl_boot_mode()"). The intention was to fix >>> exactly the problem which Anatolij encountered. For reference: >>> >>> common: Pass the boot device into spl_boot_mode() >>> >>> The SPL code already knows which boot device it calls the spl_boot_mode() >>> on, so pass that information into the function. This allows the code of >>> spl_boot_mode() avoid invoking spl_boot_device() again, but it also lets >>> board_boot_order() correctly alter the behavior of the boot process. >>> >>> The later one is important, since in certain cases, it is desired that >>> spl_boot_device() return value be overriden using board_boot_order(). >> >> Note that the entire madness above was needed for 8997de292a8b to work. >> >> ARM: at91: ma5d4: Boot from MMC2 when using SAM-BA >> >> Continue loading U-Boot from MMC2 when the SPL was loaded using SAM-BA >> loader. This allows the board to boot system from the removable media >> instead of the eMMC, which is useful for commissioning purposes. When >> booting from the eMMC, always boot from it as it is not possible to >> boot from the SD interface directly. > > I see. Well, and trying to do the same thing on an IMX would not work at > the moment, because of the issue I am trying to describe. Yep, just adding some extra context here. >>> It seems to me that using spl_boot_device() instead of the `boot_device` >>> parameter cannot be correct here (If I am wrong about the following, >>> please correct me!): >>> >>> spl_boot_mode() is essentially a lookup function which is used by the SPL >>> MMC driver (here [2]) to find out the 'mode' of the currently attempted >>> MMC device. That is, for each MMC device, it should tell the driver >>> whether this device has a FAT/ext4 filesystem (MMCSD_MODE_FS), is using an >>> eMMC boot-partition (MMCSD_MODE_EMMCBOOT), or should be accessed directly >>> (MMCSD_MODE_RAW). >> >> Yes >> >>> spl_boot_device() returns the device which SPL was booted from. >> >> Yes >> >>> Now because in most cases U-Boot Proper is loaded from the same MMC device >>> which the SPL was originally loaded from, the current code often >>> mistakenly does the right thing. But when this is not the case (e.g. >>> because a board_boot_order() was defined), it is obviously not correct to >>> return the mode of the MMC device which SPL was loaded from instead of the >>> mode of the device which the MMC driver is currently attempting to access. >>> >>> So, I think the function should in all circumstances use its `boot_device` >>> parameter to behave correctly (and all other implementations do this, from >>> what I can tell). It might make sense to rename it, though. It is not >>> really about the 'spl boot mode', but much more about 'mmc device mode'. >>> >>> I'd send a patch-series but first I'd like some input whether I am correct >>> about this ... >>> >>> [1]: https://patchwork.ozlabs.org/patch/796237/ >>> [2]: https://gitlab.denx.de/u-boot/u-boot/-/blob/v2020.01/common/spl/spl_mmc.c#L346 >> >> I think you are correct about this.
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..fe800840beb6 100644 --- a/common/spl/Kconfig +++ b/common/spl/Kconfig @@ -607,6 +607,15 @@ 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 booting from MMC" + depends on SPL_MMC_SUPPORT + default n + help + Force SPL to use MMC device for Linux kernel booting even when the + SoC ROM recognized boot medium is not eMMC/SD. This is crucial for + factory or 'falcon mode' 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> --- Changes in v2: - Update/enhance the Kconfig description of the SPL_FORCE_MMC_BOOT. 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 | 9 +++++++++ 2 files changed, 20 insertions(+)