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.