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

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

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
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot