| Message ID | 20240308030051.370824-1-benwolsieffer@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | [v2] rockchip: load env from boot MMC device | expand |
Hi Ben, On 3/8/24 04:00, 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> Minor (optional I believe) comments below but for the "logic" part: Reviewed-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> > --- > v2: > * Use #ifdef rather than if(CONFIG_IS_ENABLED(...)) > * Add a debug message if boot device is not found > > arch/arm/mach-rockchip/board.c | 31 ++++++++++++++++++++ > board/rockchip/tinker_rk3288/tinker-rk3288.c | 12 -------- > board/theobroma-systems/common/common.c | 30 ------------------- > 3 files changed, 31 insertions(+), 42 deletions(-) > > diff --git a/arch/arm/mach-rockchip/board.c b/arch/arm/mach-rockchip/board.c > index 2620530e03..8aa09f44b3 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> c.f. https://docs.u-boot.org/en/latest/develop/codingstyle.html#include-files """ In all cases, they should be listed in alphabetical order. First comes headers which are located directly in our top-level include diretory. This excludes the common.h header file which is to be removed. Second are headers within subdirectories, Finally directory-local includes should be listed. """ So it should be after asm/arch-rockchip/misc.h and before power/regulator.h > #include <efi_loader.h> > #include <fastboot.h> > #include <init.h> > @@ -349,3 +350,33 @@ __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; > + This rule isn't written in the coding style I think, but some kernel people like the reverse Christmas tree order. So, here it'd be: """ + const char *boot_device; + struct udevice *dev; + int devnum; """ Cheers, Quentin
On 2024/3/8 11:00, Ben Wolsieffer wrote: > 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> Reviewed-by: Kever Yang <kever.yang@rock-chips.com> Thanks, - Kever > --- > v2: > * Use #ifdef rather than if(CONFIG_IS_ENABLED(...)) > * Add a debug message if boot device is not found > > arch/arm/mach-rockchip/board.c | 31 ++++++++++++++++++++ > board/rockchip/tinker_rk3288/tinker-rk3288.c | 12 -------- > board/theobroma-systems/common/common.c | 30 ------------------- > 3 files changed, 31 insertions(+), 42 deletions(-) > > diff --git a/arch/arm/mach-rockchip/board.c b/arch/arm/mach-rockchip/board.c > index 2620530e03..8aa09f44b3 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,33 @@ __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; > + > +#ifdef CONFIG_SYS_MMC_ENV_DEV > + devnum = CONFIG_SYS_MMC_ENV_DEV; > +#else > + devnum = 0; > +#endif > + > + 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)) { > + debug("%s: no U-Boot device found for %s\n", __func__, boot_device); > + 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 =
diff --git a/arch/arm/mach-rockchip/board.c b/arch/arm/mach-rockchip/board.c index 2620530e03..8aa09f44b3 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,33 @@ __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; + +#ifdef CONFIG_SYS_MMC_ENV_DEV + devnum = CONFIG_SYS_MMC_ENV_DEV; +#else + devnum = 0; +#endif + + 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)) { + debug("%s: no U-Boot device found for %s\n", __func__, boot_device); + 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 =
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> --- v2: * Use #ifdef rather than if(CONFIG_IS_ENABLED(...)) * Add a debug message if boot device is not found arch/arm/mach-rockchip/board.c | 31 ++++++++++++++++++++ board/rockchip/tinker_rk3288/tinker-rk3288.c | 12 -------- board/theobroma-systems/common/common.c | 30 ------------------- 3 files changed, 31 insertions(+), 42 deletions(-)