[2/2] configs/rock5b_defconfig: enable uboot-env on the SD card

From: Niklas Cassel <cassel@kernel.org>

Having the uboot environment defined on the SD card allows the user to
use the uboot setenv and saveenv commands to make persistent changes
(e.g. to change the boot order, or to set a server IP for PXE boot).

Since the SD card environment is empty by default, initialize it using
the BR2_TARGET_UBOOT_INITIAL_ENV (output/target/etc/u-boot-initial-env),
which contains the default environment for our board (extracted from the
built uboot binary).

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 board/radxa/rock5b/genimage.cfg    | 8 ++++++++
 board/radxa/rock5b/u-boot.fragment | 5 +++++
 configs/rock5b_defconfig           | 6 ++++++
 3 files changed, 19 insertions(+)
 create mode 100644 board/radxa/rock5b/u-boot.fragment

Hello Niklas,

On Mon,  9 Sep 2024 20:21:03 +0200
Niklas Cassel via buildroot <buildroot@buildroot.org> wrote:

> From: Niklas Cassel <cassel@kernel.org>
> 
> Having the uboot environment defined on the SD card allows the user to
> use the uboot setenv and saveenv commands to make persistent changes
> (e.g. to change the boot order, or to set a server IP for PXE boot).
> 
> Since the SD card environment is empty by default, initialize it using
> the BR2_TARGET_UBOOT_INITIAL_ENV (output/target/etc/u-boot-initial-env),
> which contains the default environment for our board (extracted from the
> built uboot binary).
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Thanks for this patch! Comments below.

> diff --git a/board/radxa/rock5b/genimage.cfg b/board/radxa/rock5b/genimage.cfg
> index 43bb65bdd9..cd6c4e2dd4 100644
> --- a/board/radxa/rock5b/genimage.cfg
> +++ b/board/radxa/rock5b/genimage.cfg
> @@ -9,6 +9,14 @@ image sdcard.img {
>  		in-partition-table = "false"
>  		image = "u-boot-rockchip.bin"
>  		offset = 32K
> +		size = 16352K # 16MB - 32KB

I believe this size parameter is not really useful. Indeed, the next
partition has offset = 16 MB, so if the image in the previous partition
is too large, genimage will complain.

And in fact, I believe we don't care about the offset of the env
partition, see below.

> diff --git a/configs/rock5b_defconfig b/configs/rock5b_defconfig
> index b88d8c5da3..50f58d379d 100644
> --- a/configs/rock5b_defconfig
> +++ b/configs/rock5b_defconfig
> @@ -15,6 +15,8 @@ BR2_TARGET_UBOOT_BUILD_SYSTEM_KCONFIG=y
>  BR2_TARGET_UBOOT_CUSTOM_VERSION=y
>  BR2_TARGET_UBOOT_CUSTOM_VERSION_VALUE="2024.01"
>  BR2_TARGET_UBOOT_BOARD_DEFCONFIG="rock5b-rk3588"
> +BR2_TARGET_UBOOT_CONFIG_FRAGMENT_FILES="board/radxa/rock5b/u-boot.fragment"
> +BR2_TARGET_UBOOT_INITIAL_ENV=y
>  BR2_TARGET_UBOOT_NEEDS_PYLIBFDT=y
>  BR2_TARGET_UBOOT_NEEDS_OPENSSL=y
>  BR2_TARGET_UBOOT_NEEDS_PYELFTOOLS=y
> @@ -46,6 +48,10 @@ BR2_PACKAGE_HOST_DOSFSTOOLS=y
>  BR2_PACKAGE_HOST_DTC=y
>  BR2_PACKAGE_HOST_GENIMAGE=y
>  BR2_PACKAGE_HOST_MTOOLS=y
> +BR2_PACKAGE_HOST_UBOOT_TOOLS=y
> +BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE=y
> +BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SOURCE="output/target/etc/u-boot-initial-env"

This won't work because the output directory is not always output/. But
I believe this is useless, because you're creating an image of what is
the default environment. So basically, you should instead not create an
environment image at all: just tell U-Boot that the environment is at
offset XYZ (or even better, in partition X). The environment partition
will contain nothing or garbage, so at the first boot, U-Boot will use
its built-in environment itself, and at the first "saveenv", it will
save the environment to the designated partition.

This will simplify your changes while preserving the features you're
looking for. Could you have a look at implementing those suggestions?

Thanks a lot!

Thomas

On Sat, Sep 14, 2024 at 10:20:32PM +0200, Thomas Petazzoni wrote:
> > diff --git a/board/radxa/rock5b/genimage.cfg b/board/radxa/rock5b/genimage.cfg
> > index 43bb65bdd9..cd6c4e2dd4 100644
> > --- a/board/radxa/rock5b/genimage.cfg
> > +++ b/board/radxa/rock5b/genimage.cfg
> > @@ -9,6 +9,14 @@ image sdcard.img {
> >  		in-partition-table = "false"
> >  		image = "u-boot-rockchip.bin"
> >  		offset = 32K
> > +		size = 16352K # 16MB - 32KB
> 
> I believe this size parameter is not really useful. Indeed, the next
> partition has offset = 16 MB, so if the image in the previous partition
> is too large, genimage will complain.

You are right, genimage appears to still be able to figure this out and
give an error. Will remove in v2.


> 
> And in fact, I believe we don't care about the offset of the env
> partition, see below.
> 
> > diff --git a/configs/rock5b_defconfig b/configs/rock5b_defconfig
> > index b88d8c5da3..50f58d379d 100644
> > --- a/configs/rock5b_defconfig
> > +++ b/configs/rock5b_defconfig
> > @@ -15,6 +15,8 @@ BR2_TARGET_UBOOT_BUILD_SYSTEM_KCONFIG=y
> >  BR2_TARGET_UBOOT_CUSTOM_VERSION=y
> >  BR2_TARGET_UBOOT_CUSTOM_VERSION_VALUE="2024.01"
> >  BR2_TARGET_UBOOT_BOARD_DEFCONFIG="rock5b-rk3588"
> > +BR2_TARGET_UBOOT_CONFIG_FRAGMENT_FILES="board/radxa/rock5b/u-boot.fragment"
> > +BR2_TARGET_UBOOT_INITIAL_ENV=y
> >  BR2_TARGET_UBOOT_NEEDS_PYLIBFDT=y
> >  BR2_TARGET_UBOOT_NEEDS_OPENSSL=y
> >  BR2_TARGET_UBOOT_NEEDS_PYELFTOOLS=y
> > @@ -46,6 +48,10 @@ BR2_PACKAGE_HOST_DOSFSTOOLS=y
> >  BR2_PACKAGE_HOST_DTC=y
> >  BR2_PACKAGE_HOST_GENIMAGE=y
> >  BR2_PACKAGE_HOST_MTOOLS=y
> > +BR2_PACKAGE_HOST_UBOOT_TOOLS=y
> > +BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE=y
> > +BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SOURCE="output/target/etc/u-boot-initial-env"
> 
> This won't work because the output directory is not always output/. But
> I believe this is useless, because you're creating an image of what is
> the default environment. So basically, you should instead not create an
> environment image at all: just tell U-Boot that the environment is at
> offset XYZ (or even better, in partition X). The environment partition
> will contain nothing or garbage, so at the first boot, U-Boot will use
> its built-in environment itself, and at the first "saveenv", it will
> save the environment to the designated partition.

Well, what happens if you simply point to an offset or partition that is
empty, is that uboot will see an empty environment, so it will see bootcmd
as empty, so it will just enter the uboot command prompt without booting
anything at all.

Sure, if you have serial console connected, you can just type:
env default -f -a; saveenv; run bootcmd
but not everyone has a serial console connected, so not booting
anything by default seems like a major regression.

I do however have a uboot script that will check if the env is empty,
and if so, write the default env and run bootcmd.
(So it will simply write the environment on first boot, otherwise
it will just run bootcmd.)

I thought that solution was a bit more hacky, but at least it will not
require us using a source file in "output/" (which apparently can also
have a different name than "output/").

I will submit v2 using this solution so that you can have a look.


Kind regards,
Niklas

On Tue, Sep 17, 2024 at 10:35:16AM +0200, Niklas Cassel wrote:
> I will submit v2 using this solution so that you can have a look.

Hello Thomas,

You can find v2 here:
https://lore.kernel.org/buildroot/20240917084203.998272-1-niklas.cassel@wdc.com/T/#u

(I forgot to put you in To: or Cc:)


Kind regards,
Niklas