Message ID | 20240909182103.3667296-3-niklas.cassel@wdc.com |
---|---|
State | Changes Requested |
Headers | show |
Series | rock5b quality of life improvements | expand |
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
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 + } + + partition uboot-env { + in-partition-table = "false" + image = "uboot-env.bin" + offset = 16M + size = 64K } partition rootfs { diff --git a/board/radxa/rock5b/u-boot.fragment b/board/radxa/rock5b/u-boot.fragment new file mode 100644 index 0000000000..e332bc0c79 --- /dev/null +++ b/board/radxa/rock5b/u-boot.fragment @@ -0,0 +1,5 @@ +CONFIG_SYS_MMC_ENV_DEV=1 +CONFIG_ENV_SIZE=0x10000 +CONFIG_ENV_OFFSET=0x1000000 +# CONFIG_ENV_IS_NOWHERE is not set +CONFIG_ENV_IS_IN_MMC=y 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" +BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SIZE="65536" BR2_SYSTEM_DHCP="eth0" BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_MDEV=y BR2_ROOTFS_POST_BUILD_SCRIPT="board/radxa/rock5b/post-build.sh"