* [PATCH] rockchip: load env from boot MMC device
@ 2024-02-26 1:14 Ben Wolsieffer
2024-02-26 11:26 ` Quentin Schulz
0 siblings, 1 reply; 3+ messages in thread
From: Ben Wolsieffer @ 2024-02-26 1:14 UTC (permalink / raw)
To: u-boot
Cc: Matwey V. Kornilov, Tom Rini, Simon Glass, Philipp Tomsich,
Quentin Schulz, Klaus Goger, Kever Yang, Lin Huang,
Michael Trimarchi, Ben Wolsieffer
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 =
--
2.43.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] rockchip: load env from boot MMC device
2024-02-26 1:14 [PATCH] rockchip: load env from boot MMC device Ben Wolsieffer
@ 2024-02-26 11:26 ` Quentin Schulz
2024-03-08 2:50 ` Ben Wolsieffer
0 siblings, 1 reply; 3+ messages in thread
From: Quentin Schulz @ 2024-02-26 11:26 UTC (permalink / raw)
To: Ben Wolsieffer, u-boot
Cc: Matwey V. Kornilov, Tom Rini, Simon Glass, Philipp Tomsich,
Klaus Goger, Kever Yang, Lin Huang, Michael Trimarchi
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] rockchip: load env from boot MMC device
2024-02-26 11:26 ` Quentin Schulz
@ 2024-03-08 2:50 ` Ben Wolsieffer
0 siblings, 0 replies; 3+ messages in thread
From: Ben Wolsieffer @ 2024-03-08 2:50 UTC (permalink / raw)
To: Quentin Schulz
Cc: u-boot, Matwey V. Kornilov, Tom Rini, Simon Glass,
Philipp Tomsich, Klaus Goger, Kever Yang, Lin Huang,
Michael Trimarchi
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-03-08 2:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-26 1:14 [PATCH] rockchip: load env from boot MMC device Ben Wolsieffer
2024-02-26 11:26 ` Quentin Schulz
2024-03-08 2:50 ` Ben Wolsieffer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox