Re: [PATCH] rockchip: load env from boot MMC 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

•   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;
  

•   }
  

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