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 ...
I think you are correct about this.