| 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
Hello Niklas, It was nice meeting you during Linux Plumbers last week! On Tue, 17 Sep 2024 08:35:16 +0000 Niklas Cassel <Niklas.Cassel@wdc.com> wrote: > > 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. This is not what should happen. Indeed, if the environment on the non-volatile storage is empty or corrupted, U-Boot should ignore it, and load its built-in environment (which is compiled into the U-Boot binary). Why isn't this working for you? Best regards, Thomas
On Wed, Sep 25, 2024 at 10:26:13PM +0200, Thomas Petazzoni wrote: > Hello Niklas, > > It was nice meeting you during Linux Plumbers last week! > > On Tue, 17 Sep 2024 08:35:16 +0000 > Niklas Cassel <Niklas.Cassel@wdc.com> wrote: > > > > 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. > > This is not what should happen. Indeed, if the environment on the > non-volatile storage is empty or corrupted, U-Boot should ignore it, > and load its built-in environment (which is compiled into the U-Boot > binary). Why isn't this working for you? My first appoach: It turns out that is you use: BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SOURCE="board/radxa/rock5b/boot-env.txt" Even if that file is empty, if will have a correct CRC, so when uboot loads the environment, it will successfully load an empty environment. Ok, so this is no good. Second approach: I tried to create an empty partition: partition uboot-env { offset = 16M size = 64K } Then I can see that uboot uses the default environment: Loading Environment from MMC... *** Warning - bad CRC, using default environment However, it then continues trying to PXE boot. It doesn't try to boot using extlinux from mmc partition 2. Looking at why, it seems that it doesn't even find the boot flow. $ bootflow scan mmc $ bootflow list <nothing> If I don't have the uboot-env partition: $ bootflow scan mmc $ bootflow list Showing all bootflows Seq Method State Uclass Part Name Filename --- ----------- ------ -------- ---- ------------------------ ---------------- 0 extlinux ready mmc 1 mmc@fe2c0000.bootdev.part /boot/extlinux/extlinux.conf --- ----------- ------ -------- ---- ------------------------ ---------------- (1 bootflow, 1 valid) So for some reason, bootflow does not find the extlinux if it is not on partition 1... I've looked at the bootflow code in uboot, an it appears to be able to scan multiple partitions on a device, so I'm not sure why it is not working. My third approach was to use: partition uboot-env { in-partition-table = "false" offset = 16M size = 64K } However, genimage does not allow you to use '"in-partition-table = "false"' without also using "image =". I guess we could add code to the rockchip post-build.sh to do "dd if=/dev/zero of="$BINARIES_DIR/uboot-env.img" bs=64k count=1 and then use "image = uboot-env.img", but that seems ugly. The fourth appoarch is to do: partition rootfs { offset = 16448K # 16MB (uboot env offset) + 64KB (uboot env size) partition-type-uuid = root-arm64 image = "rootfs.ext2" } and simply skip having the uboot-env as a partition. This works, but is not the most beautiful either. Will submit a patch using this appoarch and see what you think. Kind regards, Niklas
On Wed, Sep 25, 2024 at 10:26:13PM +0200, Thomas Petazzoni wrote: > Hello Niklas, > > It was nice meeting you during Linux Plumbers last week! Nice meeting you too btw :) Kind regards, Niklas
On Thu, Oct 03, 2024 at 03:45:13PM +0200, Niklas Cassel wrote: > > Second approach: > > I tried to create an empty partition: > partition uboot-env { > offset = 16M > size = 64K > } > > Then I can see that uboot uses the default environment: > Loading Environment from MMC... *** Warning - bad CRC, using default environment > > However, it then continues trying to PXE boot. > > It doesn't try to boot using extlinux from mmc partition 2. > > Looking at why, it seems that it doesn't even find the boot flow. > > > $ bootflow scan mmc > $ bootflow list > <nothing> > > If I don't have the uboot-env partition: > > $ bootflow scan mmc > $ bootflow list > Showing all bootflows > Seq Method State Uclass Part Name Filename > --- ----------- ------ -------- ---- ------------------------ ---------------- > 0 extlinux ready mmc 1 mmc@fe2c0000.bootdev.part /boot/extlinux/extlinux.conf > --- ----------- ------ -------- ---- ------------------------ ---------------- > (1 bootflow, 1 valid) > > > So for some reason, bootflow does not find the extlinux if it is not on > partition 1... I've looked at the bootflow code in uboot, an it appears > to be able to scan multiple partitions on a device, so I'm not sure why > it is not working. I found out why uboot bootflow scan did not find this partition: The partition was not marked bootable. Apparently uboot doesn't care about the partition being marked bootable if it is the first partition, but if it is not on the first partition, it has to be marked bootable. Will submit a V2 with this approach, as it is much cleaner. 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"