Re: [PATCH] rockchip: load env from boot MMC device

[Quentin Schulz <quentin.schulz@theobroma-systems.com>]

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-systems.com/ 
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