Re: [yocto-patches] [meta-rockchip][PATCH v2 1/2] enable stored U-Boot environment

["Trevor Woerner" <twoerner@gmail.com>]

On Wed 2024-04-17 @ 12:44:36 PM, Quentin Schulz wrote:
> Hi Trevor,
> 
> On 4/11/24 03:16, Trevor Woerner via lists.yoctoproject.org wrote:
> > U-Boot has the ability to store its environment variables to a permanent
> > storage device. Whether or not it does so for any one specific device
> > depends on whatever settings are enabled in that specific device's
> > defconfig. In order to definitively configure U-Boot to be able to store
> > its environment into the device from which it boots, for any device
> > supported in this BSP, simply add the following to MACHINE_FEATURES:
> >
> > 	rk-u-boot-env
> >
> > If enabled, there is now a second choice to make: should the build
> > also include the U-Boot environment in the image or not? The default
> > environment, as generated by U-Boot, can be included in the generated wic
> > image. If it is included, then flashing the image will also flash the
> > default U-Boot environment variables and settings, wiping out anything
> that
> > might have been there already. If it is not included then your device will
> > either continue using whatever environment happens to be there (if valid),
> > or will not use any stored environment if the stored environment has not
> > been set or is invalid. The variable which governs this behaviour is:
> >
> > 	RK_IMAGE_INCLUDES_UBOOT_ENV
> >
> > By default this is set to "0", meaning that by default the image does not
> > contain the U-Boot environment. To enable this behaviour, enable this
> > variable. This variable only takes effect if rk-u-boot-env is listed in
> > MACHINE_FEATURES, and has no effect otherwise.
> >
> > The script:
> >
> > 	scripts/dump-uboot-env-from-yocto-image.sh
> >
> > can be used on a rockchip wic image to see the contents of the U-Boot
> > environment partition at build time.
> >
> > Tested by booting the same image on both eMMC and SDcard with the
> following
> > devices, verifying the ability to read and write the U-Boot environment in
> > both U-Boot and Linux user-space, and that changes made in one are seen in
> > the other:
> > 	rock-3a
> > 	rock-5a
> > 	rock-5b
> > 	rock-pi-4b
> > 	rock-pi-e
> > 	rock64
> >
> > Signed-off-by: Trevor Woerner <twoerner@gmail.com>
> > ---
> > v2 changes:
> > - re-word the commit message and README for clarity
> > - use bash's built-in math handling instead of depending on `bc`
> > - in anticipation of the upcoming feature in U-Boot whereby rockchip
> >    devices can automatically select the environment storage device to be
> >    the same as the boot device, use a more recent SRCREV for builds that
> do
> >    environment handling, instead of hardcoding the environment storage
> device
> >    by SoC family
> > - handle RK_IMAGE_INCLUDES_UBOOT_ENV as a boolean
> > ---
> >   README                                        | 20 +++++++++++
> >   classes/rk-u-boot-env.bbclass                 |  1 +
> >   conf/machine/include/rockchip-wic.inc         | 11 ++++++
> >   .../rockchip-enable-environment-mmc.cfg       |  6 ++++
> >   recipes-bsp/u-boot/u-boot_%.bbappend          | 36 +++++++++++++++++--
> >   scripts/dump-uboot-env-from-yocto-image.sh    | 28 +++++++++++++++
> >   wic/rockchip.wks                              |  2 +-
> >   7 files changed, 100 insertions(+), 4 deletions(-)
> >   create mode 100644 classes/rk-u-boot-env.bbclass
> >   create mode 100644
> recipes-bsp/u-boot/files/rk-u-boot-env/rockchip-enable-environment-mmc.cfg
> >   create mode 100755 scripts/dump-uboot-env-from-yocto-image.sh
> >
> > diff --git a/README b/README
> > index 4c30f7529353..95e7dd2e4338 100644
> > --- a/README
> > +++ b/README
> > @@ -67,6 +67,26 @@ Notes:
> >   	
> >   	in the configuration (e.g. conf/local.conf).
> >
> > +U-Boot Environment:
> > +------------------
> > +	In order to configure U-Boot to be able to store its environment into
> the
> > +	device from which it was booted, for any device supported in this BSP,
> > +	simply add the following to MACHINE_FEATURES:
> > +
> > +		rk-u-boot-env
> > +
> > +	If enabled, to additionally have the U-Boot environment generated and
> > +	stored in the image, also enable the following variable (default: off):
> > +
> > +		RK_IMAGE_INCLUDES_UBOOT_ENV
> > +
> > +	The script:
> > +
> > +		scripts/dump-uboot-env-from-yocto-image.sh
> > +
> > +	can be used on a rockchip wic image to see the contents of the U-Boot
> > +	environment partition at build time.
> > +
> >   Maintenance:
> >   -----------
> >   	Please send pull requests, patches, comments, or questions to the
> > diff --git a/classes/rk-u-boot-env.bbclass b/classes/rk-u-boot-env.bbclass
> > new file mode 100644
> > index 000000000000..2de3a54d35c3
> > --- /dev/null
> > +++ b/classes/rk-u-boot-env.bbclass
> > @@ -0,0 +1 @@
> > +MACHINEOVERRIDES .= "${@bb.utils.contains('MACHINE_FEATURES',
> 'rk-u-boot-env', ':rk-u-boot-env', '', d)}"
> Please just add this line in the proper location in one of the machine
> include files, this way the :rk-u-boot-env OVERRIDES can be used anywhere,
> and not only in recipes where rk-u-boot-env.bbclass is used.

Sounds good, will do.

> Make sure the order makes sense here by checking with bitbake-getvar -r
> u-boot OVERRIDES for example.
> 
> > diff --git a/conf/machine/include/rockchip-wic.inc
> b/conf/machine/include/rockchip-wic.inc
> > index 147a36685d7d..375a169a4c5e 100644
> > --- a/conf/machine/include/rockchip-wic.inc
> > +++ b/conf/machine/include/rockchip-wic.inc
> > @@ -2,6 +2,11 @@
> >
> >   require conf/machine/include/rockchip-extlinux.inc
> >
> > +# u-boot environment
> > +# any MACHINE that is using wic is using U-Boot
> > +# if rk-u-boot-env is enabled, then include the u-boot-env and
> u-boot-fw-utils packages
> > +IMAGE_INSTALL:append = " ${@bb.utils.contains('MACHINE_FEATURES',
> 'rk-u-boot-env', 'u-boot-fw-utils u-boot-env', '', d)}"
> > +
> By doing the above, here you could do:
> 
> IMAGE_INSTALL:append:rk-u-boot-env = " u-boot-fw-utils u-boot-env"
> 
> >   SPL_BINARY ?= "idbloader.img"
> >
> >   IMAGE_FSTYPES += "wic wic.bmap"
> > @@ -11,7 +16,13 @@ WKS_FILE_DEPENDS ?= " \
> >   	virtual/bootloader \
> >   	"
> >
> > +RK_IMAGE_INCLUDES_UBOOT_ENV ?= "no"
> > +RK_UBOOT_ENV = "${@ '--source rawcopy --sourceparams=file=u-boot.env' if
> \
> > +	bb.utils.to_boolean(d.getVar('RK_IMAGE_INCLUDES_UBOOT_ENV'), False) and
> \
> > +	bb.utils.contains('MACHINE_FEATURES', 'rk-u-boot-env', True, False, d)
> else \
> > +	' '}"
> Same here:
> 
> RK_UBOOT_ENV = ""
> RK_UBOOT_ENV:rk-u-boot-env = "${@ '--source rawcopy
> --sourceparams=file=u-boot.env' if
> bb.utils.to_boolean(d.getVar('RK_IMAGE_INCLUDES_UBOOT_ENV'), False) else ''"
> 
> >   WICVARS:append = " \
> >   	SPL_BINARY \
> >   	UBOOT_SUFFIX \
> > +	RK_UBOOT_ENV \
> >   	"
> > diff --git
> a/recipes-bsp/u-boot/files/rk-u-boot-env/rockchip-enable-environment-mmc.cfg
> b/recipes-bsp/u-boot/files/rk-u-boot-env/rockchip-enable-environment-mmc.cfg
> > new file mode 100644
> > index 000000000000..778772d27767
> > --- /dev/null
> > +++
> b/recipes-bsp/u-boot/files/rk-u-boot-env/rockchip-enable-environment-mmc.cfg
> > @@ -0,0 +1,6 @@
> > +CONFIG_ENV_SIZE=0x8000
> > +CONFIG_ENV_OFFSET=0x3f8000
> > +# CONFIG_ENV_IS_NOWHERE is not set
> > +CONFIG_ENV_IS_IN_MMC=y
> > +CONFIG_DM_SEQ_ALIAS=y
> > +CONFIG_SPL_DM_SEQ_ALIAS=y
> > diff --git a/recipes-bsp/u-boot/u-boot_%.bbappend
> b/recipes-bsp/u-boot/u-boot_%.bbappend
> > index f8378d91ce68..18702428a787 100644
> > --- a/recipes-bsp/u-boot/u-boot_%.bbappend
> > +++ b/recipes-bsp/u-boot/u-boot_%.bbappend
> > @@ -1,13 +1,29 @@
> > +inherit rk-u-boot-env deploy
> > +
> Why is deploy suddenly added here? u-boot.inc already includes it so I don't
> think it's warranted here.

Because this bbappend currently doesn't have a do_deploy() clause, and I'm
about to add one, one that I want to be handled correctly wrt sstate and
deploying, etc.

> >   FILESEXTRAPATHS:prepend := "${THISDIR}/files:"
> > -SRC_URI:append:rock-pi-e = " \
> > +
> > +# -- REMOVE AFTER 2024.04 --
> > +# As of 2024.01 and 2024.04 U-Boot is unable to automatically read or
> write
> > +# its environment from/to the device from which it booted automatically.
> > +# But patches are in master for rockchip devices to assume the user wants
> > +# to use the boot media for the environment.
> > +# Set the SRCREV to something on master, but don't set it to master to
> > +# avoid churn.
> > +# -- REMOVE AFTER 2024.04 --
> U-Boot v2024.04 is merged in master, c.f. https://git.openembedded.org/openembedded-core/commit/meta/recipes-bsp/u-boot?id=c035655ed65b6333d87019677ba93d7899f42d9a
> 
> For scarthgap, you can use meta-lts-mixin scarthgap/u-boot branch instead.

U-Boot v2024.04 still doesn't contain all the patches I need to get this
working. The comment means "remove this with the release after 2024.04" not
"once 2024.04 is being used this can be deleted".

Trying to find all the needed patches and back-porting them successfully, and
testing all the builds and variants on the 6 boards that I have will take a
ridiculous amount of time just to throw it all away with the next release
after 2024.04? It's not worth it. I'll just set the SRCREV to something that I
know works, update it post-2024.04 and get on with things.

> > +SRC_URI:append:rock-pi-e = " ${@bb.utils.contains('MACHINE_FEATURES',
> 'rk-u-boot-env', '', ' \
> SRC_URI:append:rock-pi-e:rk-u-boot-env = ""
> 
> to avoid the bb.utils.contains which is difficult to read usually.
> 
> > file://PATCH_1-2_net_designware_Reset_eth_phy_before_phy_connect.patch \
> > - file://PATCH_2-2_rockchip_rk3328-rock-pi-e_Enable_DM_ETH_PHY_and_PHY_REALTEK.patch
> \
> > -	"
> > + file://PATCH_2-2_rockchip_rk3328-rock-pi-e_Enable_DM_ETH_PHY_and_PHY_REALTEK.patch
> ', \
> > +	d)} "
> > +SRCREV:rk-u-boot-env = "cdfcc37428e06f4730ab9a17cc084eeb7676ea1a"
> > +
> > +SRC_URI:append:rk-u-boot-env = " \
> > +	file://rockchip-enable-environment-mmc.cfg "
> >
> >   # various machines require the pyelftools library for parsing dtb files
> >   DEPENDS:append = " python3-pyelftools-native"
> >   DEPENDS:append:rk3308 = " u-boot-tools-native"
> >   DEPENDS:append:rock-pi-4 = " gnutls-native"
> > +DEPENDS:append:rk-u-boot-env = " u-boot-mkenvimage-native"
> >
> >   EXTRA_OEMAKE:append:px30 = " BL31=${DEPLOY_DIR_IMAGE}/bl31-px30.elf"
> >   EXTRA_OEMAKE:append:rk3308 = " \
> > @@ -40,3 +56,17 @@ do_compile:append:rock2-square () {
> >   		cp ${B}/spl/${SPL_BINARY} ${B}
> >   	fi
> >   }
> > +
> > +do_compile:append:rk-u-boot-env() {
> > +	UBOOT_ENV_SIZE="$(cat ${B}/.config | grep "^CONFIG_ENV_SIZE=" | cut
> -d'=' -f2)"
> > +	UBOOT_MMC_DEV="$(cat ${B}/.config | grep "^CONFIG_SYS_MMC_ENV_DEV=" |
> cut -d'=' -f2)"
> grep "pattern" file
> instead of
> cat file | grep "pattern"
> ?

That's just a matter of taste. I prefer to see the file I'm querying first,
then see what I'm doing with it. I guess that's just me.

> > +	echo "/dev/mmcblk${UBOOT_MMC_DEV}p5 0x0000 ${UBOOT_ENV_SIZE}" >
> ${WORKDIR}/fw_env.config
> I would really recommend to NOT use WORKDIR but B instead. The WORKDIR isn't
> cleaned by bitbake for now, so you can have leftover.

Putting the file in WORKDIR ensures it gets picked up by the do_compile() of
oe-core's u-boot.inc and therefore gets included in U-Boot's ${PN}-env
package. Otherwise I'd have to tweak the U-Boot packaging myself.

> 
> > +}
> > +
> > +do_deploy:append:rk-u-boot-env() {
> > +	UBOOT_ENV_SIZE="$(cat ${B}/.config | grep "^CONFIG_ENV_SIZE=" | cut
> -d'=' -f2)"
> > +	mkenvimage -s ${UBOOT_ENV_SIZE} ${B}/u-boot-initial-env -o
> ${WORKDIR}/u-boot.env
> > +
> > +	install -d ${DEPLOYDIR}
> > +	install -m 0644 ${WORKDIR}/u-boot.env ${DEPLOYDIR}
> Just install at the top and then write the file directly to DEPLOYDIR? Or,
> actually, create the file in do_compile if possible?

But I want this file handled correctly by do_deploy when sstate is being used?
So don't I need to do this in the do_deploy task?

> 
> > +}
> > diff --git a/scripts/dump-uboot-env-from-yocto-image.sh
> b/scripts/dump-uboot-env-from-yocto-image.sh
> > new file mode 100755
> > index 000000000000..94573e7a6dbc
> > --- /dev/null
> > +++ b/scripts/dump-uboot-env-from-yocto-image.sh
> > @@ -0,0 +1,28 @@
> > +#/bin/bash
> > +#
> Missing SPDX license in there?

Oops!

> 
> Cheers,
> Quentin