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

[Ben Wolsieffer <benwolsieffer@gmail.com>]

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.

> > +       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.

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-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?

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