[PATCH] rockchip: load env from boot MMC device
Currently, if the environment is stored on an MMC device, the device number is hardcoded by CONFIG_SYS_MMC_ENV_DEV. This is problematic because many boards can choose between booting from an SD card or a removable eMMC. For example, the Rock64 defconfig sets CONFIG_SYS_MMC_ENV_DEV=1, which corresponds to the SD card. If an eMMC is used as the boot device and no SD card is installed, it is impossible to save the environment.
To avoid this problem, we can choose the environment MMC device based on the boot device. The theobroma-systems boards already contain code to do this, so this commit simply moves it to the common Rockchip board file, with some refactoring. I also removed another implementation of mmc_get_env_dev() from tinker_rk3288 that performed MMC boot device detection by reading a bootrom register.
This has been tested on a Rock64v2.
Signed-off-by: Ben Wolsieffer benwolsieffer@gmail.com --- arch/arm/mach-rockchip/board.c | 28 ++++++++++++++++++ board/rockchip/tinker_rk3288/tinker-rk3288.c | 12 -------- board/theobroma-systems/common/common.c | 30 -------------------- 3 files changed, 28 insertions(+), 42 deletions(-)
diff --git a/arch/arm/mach-rockchip/board.c b/arch/arm/mach-rockchip/board.c index 2620530e03..04db809e97 100644 --- a/arch/arm/mach-rockchip/board.c +++ b/arch/arm/mach-rockchip/board.c @@ -6,6 +6,7 @@ #include <clk.h> #include <cpu_func.h> #include <dm.h> +#include <dm/uclass-internal.h> #include <efi_loader.h> #include <fastboot.h> #include <init.h> @@ -349,3 +350,30 @@ __weak int board_rng_seed(struct abuf *buf) return 0; } #endif + +int mmc_get_env_dev(void) +{ + int devnum; + const char *boot_device; + struct udevice *dev; + + if (IS_ENABLED(CONFIG_SYS_MMC_ENV_DEV)) + devnum = CONFIG_SYS_MMC_ENV_DEV; + else + devnum = 0; + + boot_device = ofnode_read_chosen_string("u-boot,spl-boot-device"); + if (!boot_device) { + debug("%s: /chosen/u-boot,spl-boot-device not set\n", __func__); + return devnum; + } + + debug("%s: booted from %s\n", __func__, boot_device); + + if (uclass_find_device_by_ofnode(UCLASS_MMC, ofnode_path(boot_device), &dev)) + return devnum; + + devnum = dev->seq_; + debug("%s: get MMC env from mmc%d\n", __func__, devnum); + return devnum; +} diff --git a/board/rockchip/tinker_rk3288/tinker-rk3288.c b/board/rockchip/tinker_rk3288/tinker-rk3288.c index f85209c649..eff3a00c30 100644 --- a/board/rockchip/tinker_rk3288/tinker-rk3288.c +++ b/board/rockchip/tinker_rk3288/tinker-rk3288.c @@ -11,8 +11,6 @@ #include <init.h> #include <net.h> #include <netdev.h> -#include <asm/arch-rockchip/bootrom.h> -#include <asm/io.h>
static int get_ethaddr_from_eeprom(u8 *addr) { @@ -38,13 +36,3 @@ int rk3288_board_late_init(void)
return 0; } - -int mmc_get_env_dev(void) -{ - u32 bootdevice_brom_id = readl(BROM_BOOTSOURCE_ID_ADDR); - - if (bootdevice_brom_id == BROM_BOOTSOURCE_EMMC) - return 0; - - return 1; -} diff --git a/board/theobroma-systems/common/common.c b/board/theobroma-systems/common/common.c index 864bcdd46f..585da43884 100644 --- a/board/theobroma-systems/common/common.c +++ b/board/theobroma-systems/common/common.c @@ -89,36 +89,6 @@ int setup_boottargets(void) return 0; }
-int mmc_get_env_dev(void) -{ - const char *boot_device = - ofnode_read_chosen_string("u-boot,spl-boot-device"); - struct udevice *devp; - - if (!boot_device) { - debug("%s: /chosen/u-boot,spl-boot-device not set\n", - __func__); -#ifdef CONFIG_SYS_MMC_ENV_DEV - return CONFIG_SYS_MMC_ENV_DEV; -#else - return 0; -#endif - } - - debug("%s: booted from %s\n", __func__, boot_device); - - if (uclass_find_device_by_ofnode(UCLASS_MMC, ofnode_path(boot_device), &devp)) -#ifdef CONFIG_SYS_MMC_ENV_DEV - return CONFIG_SYS_MMC_ENV_DEV; -#else - return 0; -#endif - - debug("%s: get MMC ENV from mmc%d\n", __func__, devp->seq_); - - return devp->seq_; -} - enum env_location arch_env_get_location(enum env_operation op, int prio) { const char *boot_device =
Hi Ben,
On 2/26/24 02:14, Ben Wolsieffer wrote:
[Some people who received this message don't often get email from benwolsieffer@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
Currently, if the environment is stored on an MMC device, the device number is hardcoded by CONFIG_SYS_MMC_ENV_DEV. This is problematic because many boards can choose between booting from an SD card or a removable eMMC. For example, the Rock64 defconfig sets CONFIG_SYS_MMC_ENV_DEV=1, which corresponds to the SD card. If an eMMC is used as the boot device and no SD card is installed, it is impossible to save the environment.
To avoid this problem, we can choose the environment MMC device based on the boot device. The theobroma-systems boards already contain code to do this, so this commit simply moves it to the common Rockchip board file, with some refactoring. I also removed another implementation of mmc_get_env_dev() from tinker_rk3288 that performed MMC boot device detection by reading a bootrom register.
This has been tested on a Rock64v2.
Signed-off-by: Ben Wolsieffer benwolsieffer@gmail.com
arch/arm/mach-rockchip/board.c | 28 ++++++++++++++++++ board/rockchip/tinker_rk3288/tinker-rk3288.c | 12 -------- board/theobroma-systems/common/common.c | 30 -------------------- 3 files changed, 28 insertions(+), 42 deletions(-)
diff --git a/arch/arm/mach-rockchip/board.c b/arch/arm/mach-rockchip/board.c index 2620530e03..04db809e97 100644 --- a/arch/arm/mach-rockchip/board.c +++ b/arch/arm/mach-rockchip/board.c @@ -6,6 +6,7 @@ #include <clk.h> #include <cpu_func.h> #include <dm.h> +#include <dm/uclass-internal.h> #include <efi_loader.h> #include <fastboot.h> #include <init.h> @@ -349,3 +350,30 @@ __weak int board_rng_seed(struct abuf *buf) return 0; } #endif
+int mmc_get_env_dev(void) +{
int devnum;const char *boot_device;struct udevice *dev;if (IS_ENABLED(CONFIG_SYS_MMC_ENV_DEV))devnum = CONFIG_SYS_MMC_ENV_DEV;
This sadly will not compile if CONFIG_SYS_MMC_ENV_DEV is not defined, so you need to use the #ifdef the same way we did in board/theobroma-systems/common/common.c
elsedevnum = 0;boot_device = ofnode_read_chosen_string("u-boot,spl-boot-device");if (!boot_device) {debug("%s: /chosen/u-boot,spl-boot-device not set\n", __func__);return devnum;}
Note that this would mean that mmc_get_env_dev called in any context before U-Boot proper is executed would result in this check failing. So this would return devnum in SPL for example. We don't have environment support in SPL for our Theobroma boards, so that was something I explicitly didn't have to handle.
debug("%s: booted from %s\n", __func__, boot_device);if (uclass_find_device_by_ofnode(UCLASS_MMC, ofnode_path(boot_device), &dev))
I know I didn't add it in board/theobroma-systems/common/common.c but I think it'd make sense to have a debug message here?
I think this may not work if mmc_get_env_dev is called before U-Boot proper is relocated on e.g. RK3588 and RK356x (the former will be fixed after https://lore.kernel.org/u-boot/20240221-jaguar-v3-15-1f256a82201b@theobroma-... is merged). So giving some hint at where this fails could be nice too.
Something like: """ debug("%s: no U-Boot device found for %s\n", __func__, boot_device); """
for example?
return devnum;devnum = dev->seq_;debug("%s: get MMC env from mmc%d\n", __func__, devnum);return devnum;+} diff --git a/board/rockchip/tinker_rk3288/tinker-rk3288.c b/board/rockchip/tinker_rk3288/tinker-rk3288.c index f85209c649..eff3a00c30 100644 --- a/board/rockchip/tinker_rk3288/tinker-rk3288.c +++ b/board/rockchip/tinker_rk3288/tinker-rk3288.c @@ -11,8 +11,6 @@ #include <init.h> #include <net.h> #include <netdev.h> -#include <asm/arch-rockchip/bootrom.h> -#include <asm/io.h>
static int get_ethaddr_from_eeprom(u8 *addr) { @@ -38,13 +36,3 @@ int rk3288_board_late_init(void)
return 0;}
-int mmc_get_env_dev(void) -{
u32 bootdevice_brom_id = readl(BROM_BOOTSOURCE_ID_ADDR);if (bootdevice_brom_id == BROM_BOOTSOURCE_EMMC)return 0;return 1;-}
This could be an issue. Indeed, /chosen/u-boot,spl-boot-device doesn't report the storage medium used to load TPL+SPL (as does BROM_BOOTSOURCE_ as far as I know?), but rather U-Boot proper, which may be loaded from a different storage medium than what was used for loading TPL+SPL. So this would be a change in behavior (which could be fine, it depends on what maintainers of the RK3288 TInker board would like to have). E.g. for u-boot,spl-boot-order = "same-as-spl", <sd>, <emmc>; if TPL+SPL is loaded from eMMC but U-Boot proper isn't found on the eMMC, it'll try to load it from SD card next. In that scenario /chosen/u-boot,spl-boot-device would be <sd> while the current implementation for mmc_get_env_dev would select eMMC instead.
Cheers, Quentin
Hi Quentin,
On Mon, Feb 26, 2024 at 12:26:59PM +0100, Quentin Schulz wrote:
Hi Ben,
On 2/26/24 02:14, Ben Wolsieffer wrote:
[Some people who received this message don't often get email from benwolsieffer@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
Currently, if the environment is stored on an MMC device, the device number is hardcoded by CONFIG_SYS_MMC_ENV_DEV. This is problematic because many boards can choose between booting from an SD card or a removable eMMC. For example, the Rock64 defconfig sets CONFIG_SYS_MMC_ENV_DEV=1, which corresponds to the SD card. If an eMMC is used as the boot device and no SD card is installed, it is impossible to save the environment.
To avoid this problem, we can choose the environment MMC device based on the boot device. The theobroma-systems boards already contain code to do this, so this commit simply moves it to the common Rockchip board file, with some refactoring. I also removed another implementation of mmc_get_env_dev() from tinker_rk3288 that performed MMC boot device detection by reading a bootrom register.
This has been tested on a Rock64v2.
Signed-off-by: Ben Wolsieffer benwolsieffer@gmail.com
arch/arm/mach-rockchip/board.c | 28 ++++++++++++++++++ board/rockchip/tinker_rk3288/tinker-rk3288.c | 12 -------- board/theobroma-systems/common/common.c | 30 -------------------- 3 files changed, 28 insertions(+), 42 deletions(-)
diff --git a/arch/arm/mach-rockchip/board.c b/arch/arm/mach-rockchip/board.c index 2620530e03..04db809e97 100644 --- a/arch/arm/mach-rockchip/board.c +++ b/arch/arm/mach-rockchip/board.c @@ -6,6 +6,7 @@ #include <clk.h> #include <cpu_func.h> #include <dm.h> +#include <dm/uclass-internal.h> #include <efi_loader.h> #include <fastboot.h> #include <init.h> @@ -349,3 +350,30 @@ __weak int board_rng_seed(struct abuf *buf) return 0; } #endif
+int mmc_get_env_dev(void) +{
int devnum;const char *boot_device;struct udevice *dev;if (IS_ENABLED(CONFIG_SYS_MMC_ENV_DEV))devnum = CONFIG_SYS_MMC_ENV_DEV;This sadly will not compile if CONFIG_SYS_MMC_ENV_DEV is not defined, so you need to use the #ifdef the same way we did in board/theobroma-systems/common/common.c
Sorry, I just blindly applied the checkpatch recommendation without thinking about it.
elsedevnum = 0;boot_device = ofnode_read_chosen_string("u-boot,spl-boot-device");if (!boot_device) {debug("%s: /chosen/u-boot,spl-boot-device not set\n", __func__);return devnum;}Note that this would mean that mmc_get_env_dev called in any context before U-Boot proper is executed would result in this check failing. So this would return devnum in SPL for example. We don't have environment support in SPL for our Theobroma boards, so that was something I explicitly didn't have to handle.
Not sure what to do about this. I assume the environment is loaded before SPL makes a decision about the boot device, so it is impossible to guarantee that SPL loads the env from the same device as U-Boot proper with this patch.
debug("%s: booted from %s\n", __func__, boot_device);if (uclass_find_device_by_ofnode(UCLASS_MMC, ofnode_path(boot_device), &dev))I know I didn't add it in board/theobroma-systems/common/common.c but I think it'd make sense to have a debug message here?
I think this may not work if mmc_get_env_dev is called before U-Boot proper is relocated on e.g. RK3588 and RK356x (the former will be fixed after https://lore.kernel.org/u-boot/20240221-jaguar-v3-15-1f256a82201b@theobroma-... is merged). So giving some hint at where this fails could be nice too.
Something like: """ debug("%s: no U-Boot device found for %s\n", __func__, boot_device); """
for example?
Ok, I'll add this in v2.
return devnum;devnum = dev->seq_;debug("%s: get MMC env from mmc%d\n", __func__, devnum);return devnum;+} diff --git a/board/rockchip/tinker_rk3288/tinker-rk3288.c b/board/rockchip/tinker_rk3288/tinker-rk3288.c index f85209c649..eff3a00c30 100644 --- a/board/rockchip/tinker_rk3288/tinker-rk3288.c +++ b/board/rockchip/tinker_rk3288/tinker-rk3288.c @@ -11,8 +11,6 @@ #include <init.h> #include <net.h> #include <netdev.h> -#include <asm/arch-rockchip/bootrom.h> -#include <asm/io.h>
static int get_ethaddr_from_eeprom(u8 *addr) { @@ -38,13 +36,3 @@ int rk3288_board_late_init(void)
return 0;}
-int mmc_get_env_dev(void) -{
u32 bootdevice_brom_id = readl(BROM_BOOTSOURCE_ID_ADDR);if (bootdevice_brom_id == BROM_BOOTSOURCE_EMMC)return 0;return 1;-}
This could be an issue. Indeed, /chosen/u-boot,spl-boot-device doesn't report the storage medium used to load TPL+SPL (as does BROM_BOOTSOURCE_ as far as I know?), but rather U-Boot proper, which may be loaded from a different storage medium than what was used for loading TPL+SPL. So this would be a change in behavior (which could be fine, it depends on what maintainers of the RK3288 TInker board would like to have). E.g. for u-boot,spl-boot-order = "same-as-spl", <sd>, <emmc>; if TPL+SPL is loaded from eMMC but U-Boot proper isn't found on the eMMC, it'll try to load it from SD card next. In that scenario /chosen/u-boot,spl-boot-device would be <sd> while the current implementation for mmc_get_env_dev would select eMMC instead.
If this behavior change is problematic, we could introduce a weak board_mmc_get_env_dev() function like some other vendors have to allow specific boards to implement custom behavior.
Cheers, Quentin
Thanks, Ben
participants (2)
-
Ben Wolsieffer -
Quentin Schulz