[PATCH 0/2] Rename spl_boot_mode() and spl_boot_partition()
TL;DR: The two functions are only used in the SPL MMC driver so I think their names should reflect that.
spl_boot_mode() has caused a bit of trouble to me and others because some of its implementations miss what it is meant to do in its current form. This is in part due to history and I suspect in part due to its name being a bit misleading.
As a quick summary: spl_boot_mode() is solely used by the SPL MMC driver to check how it should load U-Boot from the currently attempted boot-source. There are three possibilities:
1. MMCSD_MODE_FS: The MMC device contains a filesystem (FAT or EXT4). 2. MMCSD_MODE_EMMCBOOT: U-Boot should be loaded from an eMMC boot-partition. 3. MMCSD_MODE_RAW: U-Boot should be loaded from a raw offset or a partition on the MMC device.
spl_boot_mode() should, for each MMC boot device, return one of those 3 modes. The boot_device parameter tells which device is queried.
Historically, the boot_device parameter did not exist. Because of this, a lot of implementations relied on checking where SPL was loaded from (spl_boot_device()) as it is common to load U-Boot from the same device. However, nowadays it is possible to attempt loading U-Boot from multiple MMC devices (as a fallback), for example using board_boot_order().
This means, spl_boot_mode() needs to properly tell the MMC driver the mode of each device. This was the reason for introducing the boot_device parameter in commit 2b1cdafa9fdd ("common: Pass the boot device into spl_boot_mode()")).
I am bringing all this up because some existing implementations don't handle this properly and new code has been merged which also doesn't. To make it more clear what the function and its cousin, spl_boot_partition(), are supposed to do, I suggest renaming them to spl_mmc_boot_mode() and spl_mmc_boot_partition() respectively.
I will also send a second series to fix the implementation which is affecting me (arch/arm/mach-imx/spl.c).
Harald Seiler (2): spl: mmc: Rename spl_boot_mode() to spl_mmc_boot_mode() spl: mmc: Rename spl_boot_partition() to spl_mmc_boot_partition()
include/spl.h | 32 ++++++++++++++++++++++++-- common/spl/spl_mmc.c | 9 ++++---- arch/arm/mach-imx/spl.c | 2 +- arch/arm/mach-k3/am6_init.c | 2 +- arch/arm/mach-k3/j721e_init.c | 2 +- arch/arm/mach-omap2/boot-common.c | 2 +- arch/arm/mach-rockchip/spl.c | 2 +- arch/arm/mach-socfpga/spl_a10.c | 2 +- arch/arm/mach-socfpga/spl_agilex.c | 2 +- arch/arm/mach-socfpga/spl_gen5.c | 2 +- arch/arm/mach-socfpga/spl_s10.c | 2 +- arch/arm/mach-stm32mp/spl.c | 4 ++-- arch/arm/mach-uniphier/mmc-boot-mode.c | 2 +- 13 files changed, 46 insertions(+), 19 deletions(-)
The function's name is misleading as one might think it is used generally to select the boot-mode when in reality it is only used by the MMC driver to find out in what way it should try reading U-Boot Proper from a device (either using a filesystem, a raw sector/partition, or an eMMC boot partition).
Rename it to spl_mmc_boot_mode() to make it more obvious what this function is about.
Link: https://lists.denx.de/pipermail/u-boot/2020-April/405979.html Signed-off-by: Harald Seiler hws@denx.de --- include/spl.h | 18 +++++++++++++++++- common/spl/spl_mmc.c | 4 ++-- arch/arm/mach-imx/spl.c | 2 +- arch/arm/mach-k3/am6_init.c | 2 +- arch/arm/mach-k3/j721e_init.c | 2 +- arch/arm/mach-omap2/boot-common.c | 2 +- arch/arm/mach-rockchip/spl.c | 2 +- arch/arm/mach-socfpga/spl_a10.c | 2 +- arch/arm/mach-socfpga/spl_agilex.c | 2 +- arch/arm/mach-socfpga/spl_gen5.c | 2 +- arch/arm/mach-socfpga/spl_s10.c | 2 +- arch/arm/mach-stm32mp/spl.c | 2 +- arch/arm/mach-uniphier/mmc-boot-mode.c | 2 +- 13 files changed, 30 insertions(+), 14 deletions(-)
diff --git a/include/spl.h b/include/spl.h index 5d8d14dbf5cd..fffcc610bb2b 100644 --- a/include/spl.h +++ b/include/spl.h @@ -238,7 +238,23 @@ int spl_load_imx_container(struct spl_image_info *spl_image, /* SPL common functions */ void preloader_console_init(void); u32 spl_boot_device(void); -u32 spl_boot_mode(const u32 boot_device); + +/** + * spl_mmc_boot_mode() - Lookup function for the mode of an MMC boot source. + * @boot_device: ID of the device which the MMC driver wants to read + * from. Common values are e.g. BOOT_DEVICE_MMC1, + * BOOT_DEVICE_MMC2, BOOT_DEVICE_MMC2_2. + * + * This function should return one of MMCSD_MODE_FS, MMCSD_MODE_EMMCBOOT, or + * MMCSD_MODE_RAW for each MMC boot source which is defined for the target. The + * boot_device parameter tells which device the MMC driver is interested in. + * + * If not overridden, it is weakly defined in common/spl/spl_mmc.c. + * + * Note: It is important to use the boot_device parameter instead of e.g. + * spl_boot_device() as U-Boot is not always loaded from the same device as SPL. + */ +u32 spl_mmc_boot_mode(const u32 boot_device); int spl_boot_partition(const u32 boot_device); void spl_set_bd(void);
diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c index a2ea363e96a9..fb8ad5d54006 100644 --- a/common/spl/spl_mmc.c +++ b/common/spl/spl_mmc.c @@ -298,7 +298,7 @@ static int spl_mmc_do_fs_boot(struct spl_image_info *spl_image, struct mmc *mmc, } #endif
-u32 __weak spl_boot_mode(const u32 boot_device) +u32 __weak spl_mmc_boot_mode(const u32 boot_device) { #if defined(CONFIG_SPL_FS_FAT) || defined(CONFIG_SPL_FS_EXT4) return MMCSD_MODE_FS; @@ -350,7 +350,7 @@ int spl_mmc_load(struct spl_image_info *spl_image, } }
- boot_mode = spl_boot_mode(bootdev->boot_device); + boot_mode = spl_mmc_boot_mode(bootdev->boot_device); err = -EINVAL; switch (boot_mode) { case MMCSD_MODE_EMMCBOOT: diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c index 87dbdf3011ee..49bb3b928da1 100644 --- a/arch/arm/mach-imx/spl.c +++ b/arch/arm/mach-imx/spl.c @@ -189,7 +189,7 @@ int g_dnl_bind_fixup(struct usb_device_descriptor *dev, const char *name)
#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) +u32 spl_mmc_boot_mode(const u32 boot_device) { #if defined(CONFIG_MX7) || defined(CONFIG_IMX8M) || defined(CONFIG_IMX8) switch (get_boot_device()) { diff --git a/arch/arm/mach-k3/am6_init.c b/arch/arm/mach-k3/am6_init.c index 3768bccafa63..b692806352c8 100644 --- a/arch/arm/mach-k3/am6_init.c +++ b/arch/arm/mach-k3/am6_init.c @@ -199,7 +199,7 @@ void board_init_f(ulong dummy) #endif }
-u32 spl_boot_mode(const u32 boot_device) +u32 spl_mmc_boot_mode(const u32 boot_device) { #if defined(CONFIG_SUPPORT_EMMC_BOOT) u32 devstat = readl(CTRLMMR_MAIN_DEVSTAT); diff --git a/arch/arm/mach-k3/j721e_init.c b/arch/arm/mach-k3/j721e_init.c index f34090f9cc99..71fc20c30bfc 100644 --- a/arch/arm/mach-k3/j721e_init.c +++ b/arch/arm/mach-k3/j721e_init.c @@ -223,7 +223,7 @@ void board_init_f(ulong dummy) #endif }
-u32 spl_boot_mode(const u32 boot_device) +u32 spl_mmc_boot_mode(const u32 boot_device) { switch (boot_device) { case BOOT_DEVICE_MMC1: diff --git a/arch/arm/mach-omap2/boot-common.c b/arch/arm/mach-omap2/boot-common.c index 734fa9d9e6f7..753852372431 100644 --- a/arch/arm/mach-omap2/boot-common.c +++ b/arch/arm/mach-omap2/boot-common.c @@ -187,7 +187,7 @@ u32 spl_boot_device(void) return gd->arch.omap_boot_device; }
-u32 spl_boot_mode(const u32 boot_device) +u32 spl_mmc_boot_mode(const u32 boot_device) { return gd->arch.omap_boot_mode; } diff --git a/arch/arm/mach-rockchip/spl.c b/arch/arm/mach-rockchip/spl.c index 48ab0e60c636..0b76af6080c9 100644 --- a/arch/arm/mach-rockchip/spl.c +++ b/arch/arm/mach-rockchip/spl.c @@ -58,7 +58,7 @@ u32 spl_boot_device(void) return boot_device; }
-u32 spl_boot_mode(const u32 boot_device) +u32 spl_mmc_boot_mode(const u32 boot_device) { return MMCSD_MODE_RAW; } diff --git a/arch/arm/mach-socfpga/spl_a10.c b/arch/arm/mach-socfpga/spl_a10.c index b10be3326803..d2f52f2f2cae 100644 --- a/arch/arm/mach-socfpga/spl_a10.c +++ b/arch/arm/mach-socfpga/spl_a10.c @@ -92,7 +92,7 @@ u32 spl_boot_device(void) }
#ifdef CONFIG_SPL_MMC_SUPPORT -u32 spl_boot_mode(const u32 boot_device) +u32 spl_mmc_boot_mode(const u32 boot_device) { #if defined(CONFIG_SPL_FS_FAT) || defined(CONFIG_SPL_FS_EXT4) return MMCSD_MODE_FS; diff --git a/arch/arm/mach-socfpga/spl_agilex.c b/arch/arm/mach-socfpga/spl_agilex.c index ecc1a35c4973..aa9f3e646c3d 100644 --- a/arch/arm/mach-socfpga/spl_agilex.c +++ b/arch/arm/mach-socfpga/spl_agilex.c @@ -28,7 +28,7 @@ u32 spl_boot_device(void) }
#ifdef CONFIG_SPL_MMC_SUPPORT -u32 spl_boot_mode(const u32 boot_device) +u32 spl_mmc_boot_mode(const u32 boot_device) { #if defined(CONFIG_SPL_FS_FAT) || defined(CONFIG_SPL_FS_EXT4) return MMCSD_MODE_FS; diff --git a/arch/arm/mach-socfpga/spl_gen5.c b/arch/arm/mach-socfpga/spl_gen5.c index a01e2a5cb9f7..e9967ac450c6 100644 --- a/arch/arm/mach-socfpga/spl_gen5.c +++ b/arch/arm/mach-socfpga/spl_gen5.c @@ -49,7 +49,7 @@ u32 spl_boot_device(void) }
#ifdef CONFIG_SPL_MMC_SUPPORT -u32 spl_boot_mode(const u32 boot_device) +u32 spl_mmc_boot_mode(const u32 boot_device) { #if defined(CONFIG_SPL_FS_FAT) || defined(CONFIG_SPL_FS_EXT4) return MMCSD_MODE_FS; diff --git a/arch/arm/mach-socfpga/spl_s10.c b/arch/arm/mach-socfpga/spl_s10.c index d89151d90241..08427dd83e22 100644 --- a/arch/arm/mach-socfpga/spl_s10.c +++ b/arch/arm/mach-socfpga/spl_s10.c @@ -30,7 +30,7 @@ u32 spl_boot_device(void) }
#ifdef CONFIG_SPL_MMC_SUPPORT -u32 spl_boot_mode(const u32 boot_device) +u32 spl_mmc_boot_mode(const u32 boot_device) { #if defined(CONFIG_SPL_FS_FAT) || defined(CONFIG_SPL_FS_EXT4) return MMCSD_MODE_FS; diff --git a/arch/arm/mach-stm32mp/spl.c b/arch/arm/mach-stm32mp/spl.c index ca4231cd0df4..55ff97de2794 100644 --- a/arch/arm/mach-stm32mp/spl.c +++ b/arch/arm/mach-stm32mp/spl.c @@ -44,7 +44,7 @@ u32 spl_boot_device(void) return BOOT_DEVICE_MMC1; }
-u32 spl_boot_mode(const u32 boot_device) +u32 spl_mmc_boot_mode(const u32 boot_device) { return MMCSD_MODE_RAW; } diff --git a/arch/arm/mach-uniphier/mmc-boot-mode.c b/arch/arm/mach-uniphier/mmc-boot-mode.c index 19b4560494c7..b48495365c05 100644 --- a/arch/arm/mach-uniphier/mmc-boot-mode.c +++ b/arch/arm/mach-uniphier/mmc-boot-mode.c @@ -8,7 +8,7 @@ #include <mmc.h> #include <spl.h>
-u32 spl_boot_mode(const u32 boot_device) +u32 spl_mmc_boot_mode(const u32 boot_device) { struct mmc *mmc;
On Wed, 15 Apr 2020 at 03:35, Harald Seiler hws@denx.de wrote:
The function's name is misleading as one might think it is used generally to select the boot-mode when in reality it is only used by the MMC driver to find out in what way it should try reading U-Boot Proper from a device (either using a filesystem, a raw sector/partition, or an eMMC boot partition).
Rename it to spl_mmc_boot_mode() to make it more obvious what this function is about.
Link: https://lists.denx.de/pipermail/u-boot/2020-April/405979.html Signed-off-by: Harald Seiler hws@denx.de
include/spl.h | 18 +++++++++++++++++- common/spl/spl_mmc.c | 4 ++-- arch/arm/mach-imx/spl.c | 2 +- arch/arm/mach-k3/am6_init.c | 2 +- arch/arm/mach-k3/j721e_init.c | 2 +- arch/arm/mach-omap2/boot-common.c | 2 +- arch/arm/mach-rockchip/spl.c | 2 +- arch/arm/mach-socfpga/spl_a10.c | 2 +- arch/arm/mach-socfpga/spl_agilex.c | 2 +- arch/arm/mach-socfpga/spl_gen5.c | 2 +- arch/arm/mach-socfpga/spl_s10.c | 2 +- arch/arm/mach-stm32mp/spl.c | 2 +- arch/arm/mach-uniphier/mmc-boot-mode.c | 2 +- 13 files changed, 30 insertions(+), 14 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Hi Harald,
From: Harald Seiler hws@denx.de Sent: mercredi 15 avril 2020 11:34
The function's name is misleading as one might think it is used generally to select the boot-mode when in reality it is only used by the MMC driver to find out in what way it should try reading U-Boot Proper from a device (either using a filesystem, a raw sector/partition, or an eMMC boot partition).
Rename it to spl_mmc_boot_mode() to make it more obvious what this function is about.
Link: https://lists.denx.de/pipermail/u-boot/2020-April/405979.html Signed-off-by: Harald Seiler hws@denx.de
include/spl.h | 18 +++++++++++++++++- common/spl/spl_mmc.c | 4 ++-- arch/arm/mach-imx/spl.c | 2 +- arch/arm/mach-k3/am6_init.c | 2 +- arch/arm/mach-k3/j721e_init.c | 2 +- arch/arm/mach-omap2/boot-common.c | 2 +- arch/arm/mach-rockchip/spl.c | 2 +- arch/arm/mach-socfpga/spl_a10.c | 2 +- arch/arm/mach-socfpga/spl_agilex.c | 2 +- arch/arm/mach-socfpga/spl_gen5.c | 2 +- arch/arm/mach-socfpga/spl_s10.c | 2 +- arch/arm/mach-stm32mp/spl.c | 2 +- arch/arm/mach-uniphier/mmc-boot-mode.c | 2 +- 13 files changed, 30 insertions(+), 14 deletions(-)
Reviewed-by: Patrick Delaunay patrick.delaunay@st.com
Thanks
Patrick
This function is only relevant to the MMC driver so calling it spl_boot_partition() might be confusing. Rename it to spl_mmc_boot_partition() to make its purpose more clear (and bring it in line with spl_mmc_boot_mode()).
Signed-off-by: Harald Seiler hws@denx.de --- include/spl.h | 14 +++++++++++++- common/spl/spl_mmc.c | 5 ++--- arch/arm/mach-stm32mp/spl.c | 2 +- 3 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/include/spl.h b/include/spl.h index fffcc610bb2b..8b15cd4914ce 100644 --- a/include/spl.h +++ b/include/spl.h @@ -255,7 +255,19 @@ u32 spl_boot_device(void); * spl_boot_device() as U-Boot is not always loaded from the same device as SPL. */ u32 spl_mmc_boot_mode(const u32 boot_device); -int spl_boot_partition(const u32 boot_device); + +/** + * spl_mmc_boot_partition() - MMC partition to load U-Boot from. + * @boot_device: ID of the device which the MMC driver wants to load + * U-Boot from. + * + * This function should return the partition number which the SPL + * should load U-Boot from (on the given boot_device) when + * CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION is set. + * + * If not overridden, it is weakly defined in common/spl/spl_mmc.c. + */ +int spl_mmc_boot_partition(const u32 boot_device); void spl_set_bd(void);
/** diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c index fb8ad5d54006..a68cdec8dc8f 100644 --- a/common/spl/spl_mmc.c +++ b/common/spl/spl_mmc.c @@ -310,8 +310,7 @@ u32 __weak spl_mmc_boot_mode(const u32 boot_device) }
#ifdef CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION -__weak -int spl_boot_partition(const u32 boot_device) +int __weak spl_mmc_boot_partition(const u32 boot_device) { return CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION; } @@ -431,7 +430,7 @@ int spl_mmc_load_image(struct spl_image_info *spl_image, NULL, #endif #ifdef CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION - spl_boot_partition(bootdev->boot_device), + spl_mmc_boot_partition(bootdev->boot_device), #else 0, #endif diff --git a/arch/arm/mach-stm32mp/spl.c b/arch/arm/mach-stm32mp/spl.c index 55ff97de2794..f85391c6af2f 100644 --- a/arch/arm/mach-stm32mp/spl.c +++ b/arch/arm/mach-stm32mp/spl.c @@ -49,7 +49,7 @@ u32 spl_mmc_boot_mode(const u32 boot_device) return MMCSD_MODE_RAW; }
-int spl_boot_partition(const u32 boot_device) +int spl_mmc_boot_partition(const u32 boot_device) { switch (boot_device) { case BOOT_DEVICE_MMC1:
On Wed, 15 Apr 2020 at 03:35, Harald Seiler hws@denx.de wrote:
This function is only relevant to the MMC driver so calling it spl_boot_partition() might be confusing. Rename it to spl_mmc_boot_partition() to make its purpose more clear (and bring it in line with spl_mmc_boot_mode()).
Signed-off-by: Harald Seiler hws@denx.de
include/spl.h | 14 +++++++++++++- common/spl/spl_mmc.c | 5 ++--- arch/arm/mach-stm32mp/spl.c | 2 +- 3 files changed, 16 insertions(+), 5 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Dear Harald,
From: Harald Seiler hws@denx.de Sent: mercredi 15 avril 2020 11:34
This function is only relevant to the MMC driver so calling it spl_boot_partition() might be confusing. Rename it to spl_mmc_boot_partition() to make its purpose more clear (and bring it in line with spl_mmc_boot_mode()).
Signed-off-by: Harald Seiler hws@denx.de
include/spl.h | 14 +++++++++++++- common/spl/spl_mmc.c | 5 ++--- arch/arm/mach-stm32mp/spl.c | 2 +- 3 files changed, 16 insertions(+), 5 deletions(-)
Reviewed-by: Patrick Delaunay patrick.delaunay@st.com
Thanks
Patrick
participants (3)
-
Harald Seiler -
Patrick DELAUNAY -
Simon Glass