diff mbox series

[meta-rockchip,v7] enable stored U-Boot environment

Message ID 20240522225345.13339-1-twoerner@gmail.com
State New
Headers show
Series [meta-rockchip,v7] enable stored U-Boot environment | expand

Commit Message

Trevor Woerner May 22, 2024, 10:53 p.m. UTC
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>
---
v7 changes:
- move u-boot's extra image install to machine/include (it wasn't having
  any effect)
- only call u-boot's rk_generate_env() postfunc if the feature is in
  MACHINEOVERRIDES (in which it is only *conditionally* found, unlike its
  presence in MACHINE_FEATURES)
- add a warning if the user is trying to build an image whereby they've
  enabled the rk-u-boot-env mechanism but it isn't supported in a given
  specific build

v6 changes:
- separate out the MACHINEOVERRIDES handling into its own include so
  that it can be included more flexibly into different scenarios; i.e.
  rock2-square build was failing because it is not wic-based

v5 changes:
- fix a bug in v4 to handle RK_IMAGE_INCLUDES_UBOOT_ENV correctly in the
  bash test in postfuncs of u-boot bbappend
- add extra check in do_deploy() of u-boot bbappend to only copy the
  environment if RK_IMAGE_INCLUDES_UBOOT_ENV is true

v4 changes:
- sort WICVARS alphabetically
- tweak dump-uboot-env-from-yocto-image.sh script to not just check for
  the existence of the user-supplied parameter, but that it is a file
  specifically
- u-boot bbappend:
  - use a do_compile[postfuncs] for generating the fw_env.config instead of
    a do_compile:append
  - move the conversion of the U-Boot text-based environment to the
    required binary representation from a do_deploy:append to the newly-created
    do_compile[postfuncs] created above, this will allow the user to inject
    any extra U-Boot environment variables they might wish as part of a
    do_compile:append and have that occur before the binary is generated
    so that those additional user-supplied values are included
  - add a lot of extra checking (instead of assuming various resources exist)
    before making use of said resources

v3 changes:
- switch from using a bbclass so the `rk-u-boot-env` override can be used
  directly
- add SPDX header to script
- drop `inherit deploy` in U-Boot bbappend

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 +++++++++
 .../include/rockchip-rk-u-boot-env.inc        |  4 ++
 conf/machine/include/rockchip-wic.inc         |  7 +++
 conf/machine/rock2-square.conf                |  1 -
 .../rockchip-enable-environment-mmc.cfg       |  6 +++
 recipes-bsp/u-boot/u-boot_%.bbappend          | 45 +++++++++++++++++++
 scripts/dump-uboot-env-from-yocto-image.sh    | 29 ++++++++++++
 wic/rockchip.wks                              |  2 +-
 8 files changed, 112 insertions(+), 2 deletions(-)
 create mode 100644 conf/machine/include/rockchip-rk-u-boot-env.inc
 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

Comments

Quentin Schulz May 23, 2024, 1:03 p.m. UTC | #1
Hi Trevor,

On 5/23/24 12:53 AM, 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>
> ---
> v7 changes:
> - move u-boot's extra image install to machine/include (it wasn't having
>    any effect)

Couldn't find this in the diff between v6 and v7, are you sure you sent 
the proper version or is this outdated? You did add the inc file to the 
rock2-square though, is that what you meant?

> - only call u-boot's rk_generate_env() postfunc if the feature is in
>    MACHINEOVERRIDES (in which it is only *conditionally* found, unlike its
>    presence in MACHINE_FEATURES)
> - add a warning if the user is trying to build an image whereby they've
>    enabled the rk-u-boot-env mechanism but it isn't supported in a given
>    specific build
> 
> v6 changes:
> - separate out the MACHINEOVERRIDES handling into its own include so
>    that it can be included more flexibly into different scenarios; i.e.
>    rock2-square build was failing because it is not wic-based
> 
> v5 changes:
> - fix a bug in v4 to handle RK_IMAGE_INCLUDES_UBOOT_ENV correctly in the
>    bash test in postfuncs of u-boot bbappend
> - add extra check in do_deploy() of u-boot bbappend to only copy the
>    environment if RK_IMAGE_INCLUDES_UBOOT_ENV is true
> 
> v4 changes:
> - sort WICVARS alphabetically
> - tweak dump-uboot-env-from-yocto-image.sh script to not just check for
>    the existence of the user-supplied parameter, but that it is a file
>    specifically
> - u-boot bbappend:
>    - use a do_compile[postfuncs] for generating the fw_env.config instead of
>      a do_compile:append
>    - move the conversion of the U-Boot text-based environment to the
>      required binary representation from a do_deploy:append to the newly-created
>      do_compile[postfuncs] created above, this will allow the user to inject
>      any extra U-Boot environment variables they might wish as part of a
>      do_compile:append and have that occur before the binary is generated
>      so that those additional user-supplied values are included
>    - add a lot of extra checking (instead of assuming various resources exist)
>      before making use of said resources
> 
> v3 changes:
> - switch from using a bbclass so the `rk-u-boot-env` override can be used
>    directly
> - add SPDX header to script
> - drop `inherit deploy` in U-Boot bbappend
> 
> 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 +++++++++
>   .../include/rockchip-rk-u-boot-env.inc        |  4 ++
>   conf/machine/include/rockchip-wic.inc         |  7 +++
>   conf/machine/rock2-square.conf                |  1 -
>   .../rockchip-enable-environment-mmc.cfg       |  6 +++
>   recipes-bsp/u-boot/u-boot_%.bbappend          | 45 +++++++++++++++++++
>   scripts/dump-uboot-env-from-yocto-image.sh    | 29 ++++++++++++
>   wic/rockchip.wks                              |  2 +-
>   8 files changed, 112 insertions(+), 2 deletions(-)
>   create mode 100644 conf/machine/include/rockchip-rk-u-boot-env.inc
>   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 c76fc9131276..605773d4ecd3 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/conf/machine/include/rockchip-rk-u-boot-env.inc b/conf/machine/include/rockchip-rk-u-boot-env.inc
> new file mode 100644
> index 000000000000..4eb877f77c48
> --- /dev/null
> +++ b/conf/machine/include/rockchip-rk-u-boot-env.inc
> @@ -0,0 +1,4 @@
> +# 'rk-u-boot-env' indicates the user wants to be able to save their U-Boot
> +# environment back to the drive from which the device was booted
> +MACHINEOVERRIDES .= "${@bb.utils.contains('MACHINE_FEATURES', 'rk-u-boot-env', ':rk-u-boot-env', '', d)}"
> +IMAGE_INSTALL:append:rk-u-boot-env = " u-boot-fw-utils u-boot-env"
> diff --git a/conf/machine/include/rockchip-wic.inc b/conf/machine/include/rockchip-wic.inc
> index 147a36685d7d..b5ee6e0c2724 100644
> --- a/conf/machine/include/rockchip-wic.inc
> +++ b/conf/machine/include/rockchip-wic.inc
> @@ -1,6 +1,7 @@
>   # common meta-rockchip wic/wks items
>   
>   require conf/machine/include/rockchip-extlinux.inc
> +require conf/machine/include/rockchip-rk-u-boot-env.inc
>   
>   SPL_BINARY ?= "idbloader.img"
>   
> @@ -11,7 +12,13 @@ WKS_FILE_DEPENDS ?= " \
>   	virtual/bootloader \
>   	"
>   
> +RK_IMAGE_INCLUDES_UBOOT_ENV ?= "no"
> +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 = " \
> +	RK_UBOOT_ENV \
>   	SPL_BINARY \
>   	UBOOT_SUFFIX \
>   	"
> diff --git a/conf/machine/rock2-square.conf b/conf/machine/rock2-square.conf
> index 9468b9a6b559..b31eb574015f 100644
> --- a/conf/machine/rock2-square.conf
> +++ b/conf/machine/rock2-square.conf
> @@ -16,4 +16,3 @@ UBOOT_MACHINE = "rock2_defconfig"
>   # image class
>   IMAGE_FSTYPES += "rockchip-gpt-img"
>   IMAGE_CLASSES += "rockchip-gpt-img"
> -
> 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 a83179a9f007..33b521021a1c 100644
> --- a/recipes-bsp/u-boot/u-boot_%.bbappend
> +++ b/recipes-bsp/u-boot/u-boot_%.bbappend
> @@ -1,7 +1,13 @@
> +FILESEXTRAPATHS:prepend := "${THISDIR}/files:"
> +
> +SRC_URI:append:rk-u-boot-env = " file://rockchip-enable-environment-mmc.cfg"
> +SRCREV:rk-u-boot-env = "cdfcc37428e06f4730ab9a17cc084eeb7676ea1a"
> +
>   # 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 = " \
> @@ -34,3 +40,42 @@ do_compile:append:rock2-square () {
>   		cp ${B}/spl/${SPL_BINARY} ${B}
>   	fi
>   }
> +
> +python rk_no_env() {
> +    if bb.utils.contains('MACHINE_FEATURES', 'rk-u-boot-env', True, False, d):
> +        bb.warn("the rk-u-boot-env MACHINE_FEATURE is not supported for this build")
> +}
> +
> +rk_generate_env() {
> +	if [ ! -f "${B}/.config" ]; then
> +		echo "U-Boot .config not found, can't determine environment size"
> +		return 1
> +	fi
> +	cat ${B}/.config | grep "^CONFIG_ENV_SIZE=" > /dev/null
> +	if [ $? -ne 0 ]; then
> +		echo "can not find CONFIG_ENV_SIZE value in U-Boot .config"
> +		return 1
> +	fi
> +
> +	UBOOT_ENV_SIZE="$(cat ${B}/.config | grep "^CONFIG_ENV_SIZE=" | cut -d'=' -f2)"
> +
> +	# linux user-space U-Boot env config file
> +	echo "/dev/disk/by-partlabel/uboot_env 0x0000 ${UBOOT_ENV_SIZE}" > ${WORKDIR}/fw_env.config
> +
> +	# convert text-based environment to binary suitable for image
> +	if [ "${@bb.utils.to_boolean(d.getVar('RK_IMAGE_INCLUDES_UBOOT_ENV'), False)}" = "True" ]; then
> +		if [ ! -f ${B}/u-boot-initial-env ]; then
> +			echo "initial, text-formatted U-Boot environment file \"${B}/u-boot-initial-env\" not found"
> +			return 1
> +		fi
> +		mkenvimage -s ${UBOOT_ENV_SIZE} ${B}/u-boot-initial-env -o ${WORKDIR}/u-boot.env
> +	fi
> +}
> +do_compile[postfuncs] += "${@'rk_generate_env' if 'rk-u-boot-env' in d.getVar('MACHINEOVERRIDES').split(':') else 'rk_no_env'}"
> +

Could be simplified with:

"""
rk_generate_env() {
     if bb.utils.contains('MACHINE_FEATURES', 'rk-u-boot-env', True, 
False, d):
         bb.warn("the rk-u-boot-env MACHINE_FEATURE is not supported for 
this build")
}

rk_generate_env:rk-u-boot-env() {
[content of rk_generate_env in v7]
}

do_compile[postfuncs] += "rk_generate_env;"
"""

Cheers,
Quentin
Trevor Woerner May 23, 2024, 7:21 p.m. UTC | #2
On Thu 2024-05-23 @ 03:03:53 PM, Quentin Schulz via lists.yoctoproject.org wrote:
> Hi Trevor,
> 
> On 5/23/24 12:53 AM, 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>
> > ---
> > v7 changes:
> > - move u-boot's extra image install to machine/include (it wasn't having
> >    any effect)
> 
> Couldn't find this in the diff between v6 and v7, are you sure you sent the
> proper version or is this outdated? You did add the inc file to the
> rock2-square though, is that what you meant?

Oops, sorry. I have about 4 different versions of v7 and I probably got
confused between some of them. In any case the extra packages are listed in
the feature's inc file and it works there.

> > - only call u-boot's rk_generate_env() postfunc if the feature is in
> >    MACHINEOVERRIDES (in which it is only *conditionally* found, unlike its
> >    presence in MACHINE_FEATURES)
> > - add a warning if the user is trying to build an image whereby they've
> >    enabled the rk-u-boot-env mechanism but it isn't supported in a given
> >    specific build
> > 
> > v6 changes:
> > - separate out the MACHINEOVERRIDES handling into its own include so
> >    that it can be included more flexibly into different scenarios; i.e.
> >    rock2-square build was failing because it is not wic-based
> > 
> > v5 changes:
> > - fix a bug in v4 to handle RK_IMAGE_INCLUDES_UBOOT_ENV correctly in the
> >    bash test in postfuncs of u-boot bbappend
> > - add extra check in do_deploy() of u-boot bbappend to only copy the
> >    environment if RK_IMAGE_INCLUDES_UBOOT_ENV is true
> > 
> > v4 changes:
> > - sort WICVARS alphabetically
> > - tweak dump-uboot-env-from-yocto-image.sh script to not just check for
> >    the existence of the user-supplied parameter, but that it is a file
> >    specifically
> > - u-boot bbappend:
> >    - use a do_compile[postfuncs] for generating the fw_env.config instead of
> >      a do_compile:append
> >    - move the conversion of the U-Boot text-based environment to the
> >      required binary representation from a do_deploy:append to the newly-created
> >      do_compile[postfuncs] created above, this will allow the user to inject
> >      any extra U-Boot environment variables they might wish as part of a
> >      do_compile:append and have that occur before the binary is generated
> >      so that those additional user-supplied values are included
> >    - add a lot of extra checking (instead of assuming various resources exist)
> >      before making use of said resources
> > 
> > v3 changes:
> > - switch from using a bbclass so the `rk-u-boot-env` override can be used
> >    directly
> > - add SPDX header to script
> > - drop `inherit deploy` in U-Boot bbappend
> > 
> > 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 +++++++++
> >   .../include/rockchip-rk-u-boot-env.inc        |  4 ++
> >   conf/machine/include/rockchip-wic.inc         |  7 +++
> >   conf/machine/rock2-square.conf                |  1 -
> >   .../rockchip-enable-environment-mmc.cfg       |  6 +++
> >   recipes-bsp/u-boot/u-boot_%.bbappend          | 45 +++++++++++++++++++
> >   scripts/dump-uboot-env-from-yocto-image.sh    | 29 ++++++++++++
> >   wic/rockchip.wks                              |  2 +-
> >   8 files changed, 112 insertions(+), 2 deletions(-)
> >   create mode 100644 conf/machine/include/rockchip-rk-u-boot-env.inc
> >   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 c76fc9131276..605773d4ecd3 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/conf/machine/include/rockchip-rk-u-boot-env.inc b/conf/machine/include/rockchip-rk-u-boot-env.inc
> > new file mode 100644
> > index 000000000000..4eb877f77c48
> > --- /dev/null
> > +++ b/conf/machine/include/rockchip-rk-u-boot-env.inc
> > @@ -0,0 +1,4 @@
> > +# 'rk-u-boot-env' indicates the user wants to be able to save their U-Boot
> > +# environment back to the drive from which the device was booted
> > +MACHINEOVERRIDES .= "${@bb.utils.contains('MACHINE_FEATURES', 'rk-u-boot-env', ':rk-u-boot-env', '', d)}"
> > +IMAGE_INSTALL:append:rk-u-boot-env = " u-boot-fw-utils u-boot-env"
> > diff --git a/conf/machine/include/rockchip-wic.inc b/conf/machine/include/rockchip-wic.inc
> > index 147a36685d7d..b5ee6e0c2724 100644
> > --- a/conf/machine/include/rockchip-wic.inc
> > +++ b/conf/machine/include/rockchip-wic.inc
> > @@ -1,6 +1,7 @@
> >   # common meta-rockchip wic/wks items
> >   require conf/machine/include/rockchip-extlinux.inc
> > +require conf/machine/include/rockchip-rk-u-boot-env.inc
> >   SPL_BINARY ?= "idbloader.img"
> > @@ -11,7 +12,13 @@ WKS_FILE_DEPENDS ?= " \
> >   	virtual/bootloader \
> >   	"
> > +RK_IMAGE_INCLUDES_UBOOT_ENV ?= "no"
> > +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 = " \
> > +	RK_UBOOT_ENV \
> >   	SPL_BINARY \
> >   	UBOOT_SUFFIX \
> >   	"
> > diff --git a/conf/machine/rock2-square.conf b/conf/machine/rock2-square.conf
> > index 9468b9a6b559..b31eb574015f 100644
> > --- a/conf/machine/rock2-square.conf
> > +++ b/conf/machine/rock2-square.conf
> > @@ -16,4 +16,3 @@ UBOOT_MACHINE = "rock2_defconfig"
> >   # image class
> >   IMAGE_FSTYPES += "rockchip-gpt-img"
> >   IMAGE_CLASSES += "rockchip-gpt-img"
> > -
> > 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 a83179a9f007..33b521021a1c 100644
> > --- a/recipes-bsp/u-boot/u-boot_%.bbappend
> > +++ b/recipes-bsp/u-boot/u-boot_%.bbappend
> > @@ -1,7 +1,13 @@
> > +FILESEXTRAPATHS:prepend := "${THISDIR}/files:"
> > +
> > +SRC_URI:append:rk-u-boot-env = " file://rockchip-enable-environment-mmc.cfg"
> > +SRCREV:rk-u-boot-env = "cdfcc37428e06f4730ab9a17cc084eeb7676ea1a"
> > +
> >   # 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 = " \
> > @@ -34,3 +40,42 @@ do_compile:append:rock2-square () {
> >   		cp ${B}/spl/${SPL_BINARY} ${B}
> >   	fi
> >   }
> > +
> > +python rk_no_env() {
> > +    if bb.utils.contains('MACHINE_FEATURES', 'rk-u-boot-env', True, False, d):
> > +        bb.warn("the rk-u-boot-env MACHINE_FEATURE is not supported for this build")
> > +}
> > +
> > +rk_generate_env() {
> > +	if [ ! -f "${B}/.config" ]; then
> > +		echo "U-Boot .config not found, can't determine environment size"
> > +		return 1
> > +	fi
> > +	cat ${B}/.config | grep "^CONFIG_ENV_SIZE=" > /dev/null
> > +	if [ $? -ne 0 ]; then
> > +		echo "can not find CONFIG_ENV_SIZE value in U-Boot .config"
> > +		return 1
> > +	fi
> > +
> > +	UBOOT_ENV_SIZE="$(cat ${B}/.config | grep "^CONFIG_ENV_SIZE=" | cut -d'=' -f2)"
> > +
> > +	# linux user-space U-Boot env config file
> > +	echo "/dev/disk/by-partlabel/uboot_env 0x0000 ${UBOOT_ENV_SIZE}" > ${WORKDIR}/fw_env.config
> > +
> > +	# convert text-based environment to binary suitable for image
> > +	if [ "${@bb.utils.to_boolean(d.getVar('RK_IMAGE_INCLUDES_UBOOT_ENV'), False)}" = "True" ]; then
> > +		if [ ! -f ${B}/u-boot-initial-env ]; then
> > +			echo "initial, text-formatted U-Boot environment file \"${B}/u-boot-initial-env\" not found"
> > +			return 1
> > +		fi
> > +		mkenvimage -s ${UBOOT_ENV_SIZE} ${B}/u-boot-initial-env -o ${WORKDIR}/u-boot.env
> > +	fi
> > +}
> > +do_compile[postfuncs] += "${@'rk_generate_env' if 'rk-u-boot-env' in d.getVar('MACHINEOVERRIDES').split(':') else 'rk_no_env'}"
> > +
> 
> Could be simplified with:
> 
> """
> rk_generate_env() {
>     if bb.utils.contains('MACHINE_FEATURES', 'rk-u-boot-env', True, False,
> d):
>         bb.warn("the rk-u-boot-env MACHINE_FEATURE is not supported for this
> build")
> }
> 
> rk_generate_env:rk-u-boot-env() {
> [content of rk_generate_env in v7]
> }
> 
> do_compile[postfuncs] += "rk_generate_env;"
> """

"...eye of the beholder..." I guess. You've suggested this before as a
simplification, but I guess I've never quite gotten accustomed to bitbake's
override mechanism. To this day I still find it odd... and unique.

What I've proposed is a simple: "IF <string> IN <object> THEN <generate_env>
ELSE <no_env>", which is mostly plain English.

With overrides you're saying "always call this function... oh
wait! unless overrides are in place, then call this one or this other one...
wait! are overrides in place? what are the overrides?..."

In any case what you've proposed doesn't work. The non-override
rk_generate_env() function would need a "python" prefix and try as I might, it
seems you can't have a set of override functions with one being python and the
other one not. Without the "python" bitbake chokes if the non-override one is
called. With the "python" bitbake will still choke if the overridden version
is called:

Try making the change you suggest, both with and without the "python"
qualifier for the non-overridden version, then try building with both
MACHINE=rock2-square and MACHINE=rock-5a; one or the other won't work with or
without the "python".

In any case I like my version :-)
I'm going to apply it. If you can change my mind we can always take a later
patch.

Thanks for the review and the discussion!!
Quentin Schulz May 24, 2024, 8:37 a.m. UTC | #3
Hi Trevor,

On 5/23/24 9:21 PM, Trevor Woerner via lists.yoctoproject.org wrote:
> On Thu 2024-05-23 @ 03:03:53 PM, Quentin Schulz via lists.yoctoproject.org wrote:
>> Hi Trevor,
>>
>> On 5/23/24 12:53 AM, 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>
>>> ---
>>> v7 changes:
>>> - move u-boot's extra image install to machine/include (it wasn't having
>>>     any effect)
>>
>> Couldn't find this in the diff between v6 and v7, are you sure you sent the
>> proper version or is this outdated? You did add the inc file to the
>> rock2-square though, is that what you meant?
> 
> Oops, sorry. I have about 4 different versions of v7 and I probably got
> confused between some of them. In any case the extra packages are listed in
> the feature's inc file and it works there.
> 
>>> - only call u-boot's rk_generate_env() postfunc if the feature is in
>>>     MACHINEOVERRIDES (in which it is only *conditionally* found, unlike its
>>>     presence in MACHINE_FEATURES)
>>> - add a warning if the user is trying to build an image whereby they've
>>>     enabled the rk-u-boot-env mechanism but it isn't supported in a given
>>>     specific build
>>>
>>> v6 changes:
>>> - separate out the MACHINEOVERRIDES handling into its own include so
>>>     that it can be included more flexibly into different scenarios; i.e.
>>>     rock2-square build was failing because it is not wic-based
>>>
>>> v5 changes:
>>> - fix a bug in v4 to handle RK_IMAGE_INCLUDES_UBOOT_ENV correctly in the
>>>     bash test in postfuncs of u-boot bbappend
>>> - add extra check in do_deploy() of u-boot bbappend to only copy the
>>>     environment if RK_IMAGE_INCLUDES_UBOOT_ENV is true
>>>
>>> v4 changes:
>>> - sort WICVARS alphabetically
>>> - tweak dump-uboot-env-from-yocto-image.sh script to not just check for
>>>     the existence of the user-supplied parameter, but that it is a file
>>>     specifically
>>> - u-boot bbappend:
>>>     - use a do_compile[postfuncs] for generating the fw_env.config instead of
>>>       a do_compile:append
>>>     - move the conversion of the U-Boot text-based environment to the
>>>       required binary representation from a do_deploy:append to the newly-created
>>>       do_compile[postfuncs] created above, this will allow the user to inject
>>>       any extra U-Boot environment variables they might wish as part of a
>>>       do_compile:append and have that occur before the binary is generated
>>>       so that those additional user-supplied values are included
>>>     - add a lot of extra checking (instead of assuming various resources exist)
>>>       before making use of said resources
>>>
>>> v3 changes:
>>> - switch from using a bbclass so the `rk-u-boot-env` override can be used
>>>     directly
>>> - add SPDX header to script
>>> - drop `inherit deploy` in U-Boot bbappend
>>>
>>> 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 +++++++++
>>>    .../include/rockchip-rk-u-boot-env.inc        |  4 ++
>>>    conf/machine/include/rockchip-wic.inc         |  7 +++
>>>    conf/machine/rock2-square.conf                |  1 -
>>>    .../rockchip-enable-environment-mmc.cfg       |  6 +++
>>>    recipes-bsp/u-boot/u-boot_%.bbappend          | 45 +++++++++++++++++++
>>>    scripts/dump-uboot-env-from-yocto-image.sh    | 29 ++++++++++++
>>>    wic/rockchip.wks                              |  2 +-
>>>    8 files changed, 112 insertions(+), 2 deletions(-)
>>>    create mode 100644 conf/machine/include/rockchip-rk-u-boot-env.inc
>>>    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 c76fc9131276..605773d4ecd3 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/conf/machine/include/rockchip-rk-u-boot-env.inc b/conf/machine/include/rockchip-rk-u-boot-env.inc
>>> new file mode 100644
>>> index 000000000000..4eb877f77c48
>>> --- /dev/null
>>> +++ b/conf/machine/include/rockchip-rk-u-boot-env.inc
>>> @@ -0,0 +1,4 @@
>>> +# 'rk-u-boot-env' indicates the user wants to be able to save their U-Boot
>>> +# environment back to the drive from which the device was booted
>>> +MACHINEOVERRIDES .= "${@bb.utils.contains('MACHINE_FEATURES', 'rk-u-boot-env', ':rk-u-boot-env', '', d)}"
>>> +IMAGE_INSTALL:append:rk-u-boot-env = " u-boot-fw-utils u-boot-env"
>>> diff --git a/conf/machine/include/rockchip-wic.inc b/conf/machine/include/rockchip-wic.inc
>>> index 147a36685d7d..b5ee6e0c2724 100644
>>> --- a/conf/machine/include/rockchip-wic.inc
>>> +++ b/conf/machine/include/rockchip-wic.inc
>>> @@ -1,6 +1,7 @@
>>>    # common meta-rockchip wic/wks items
>>>    require conf/machine/include/rockchip-extlinux.inc
>>> +require conf/machine/include/rockchip-rk-u-boot-env.inc
>>>    SPL_BINARY ?= "idbloader.img"
>>> @@ -11,7 +12,13 @@ WKS_FILE_DEPENDS ?= " \
>>>    	virtual/bootloader \
>>>    	"
>>> +RK_IMAGE_INCLUDES_UBOOT_ENV ?= "no"
>>> +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 = " \
>>> +	RK_UBOOT_ENV \
>>>    	SPL_BINARY \
>>>    	UBOOT_SUFFIX \
>>>    	"
>>> diff --git a/conf/machine/rock2-square.conf b/conf/machine/rock2-square.conf
>>> index 9468b9a6b559..b31eb574015f 100644
>>> --- a/conf/machine/rock2-square.conf
>>> +++ b/conf/machine/rock2-square.conf
>>> @@ -16,4 +16,3 @@ UBOOT_MACHINE = "rock2_defconfig"
>>>    # image class
>>>    IMAGE_FSTYPES += "rockchip-gpt-img"
>>>    IMAGE_CLASSES += "rockchip-gpt-img"
>>> -
>>> 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 a83179a9f007..33b521021a1c 100644
>>> --- a/recipes-bsp/u-boot/u-boot_%.bbappend
>>> +++ b/recipes-bsp/u-boot/u-boot_%.bbappend
>>> @@ -1,7 +1,13 @@
>>> +FILESEXTRAPATHS:prepend := "${THISDIR}/files:"
>>> +
>>> +SRC_URI:append:rk-u-boot-env = " file://rockchip-enable-environment-mmc.cfg"
>>> +SRCREV:rk-u-boot-env = "cdfcc37428e06f4730ab9a17cc084eeb7676ea1a"
>>> +
>>>    # 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 = " \
>>> @@ -34,3 +40,42 @@ do_compile:append:rock2-square () {
>>>    		cp ${B}/spl/${SPL_BINARY} ${B}
>>>    	fi
>>>    }
>>> +
>>> +python rk_no_env() {
>>> +    if bb.utils.contains('MACHINE_FEATURES', 'rk-u-boot-env', True, False, d):
>>> +        bb.warn("the rk-u-boot-env MACHINE_FEATURE is not supported for this build")
>>> +}
>>> +
>>> +rk_generate_env() {
>>> +	if [ ! -f "${B}/.config" ]; then
>>> +		echo "U-Boot .config not found, can't determine environment size"
>>> +		return 1
>>> +	fi
>>> +	cat ${B}/.config | grep "^CONFIG_ENV_SIZE=" > /dev/null
>>> +	if [ $? -ne 0 ]; then
>>> +		echo "can not find CONFIG_ENV_SIZE value in U-Boot .config"
>>> +		return 1
>>> +	fi
>>> +
>>> +	UBOOT_ENV_SIZE="$(cat ${B}/.config | grep "^CONFIG_ENV_SIZE=" | cut -d'=' -f2)"
>>> +
>>> +	# linux user-space U-Boot env config file
>>> +	echo "/dev/disk/by-partlabel/uboot_env 0x0000 ${UBOOT_ENV_SIZE}" > ${WORKDIR}/fw_env.config
>>> +
>>> +	# convert text-based environment to binary suitable for image
>>> +	if [ "${@bb.utils.to_boolean(d.getVar('RK_IMAGE_INCLUDES_UBOOT_ENV'), False)}" = "True" ]; then
>>> +		if [ ! -f ${B}/u-boot-initial-env ]; then
>>> +			echo "initial, text-formatted U-Boot environment file \"${B}/u-boot-initial-env\" not found"
>>> +			return 1
>>> +		fi
>>> +		mkenvimage -s ${UBOOT_ENV_SIZE} ${B}/u-boot-initial-env -o ${WORKDIR}/u-boot.env
>>> +	fi
>>> +}
>>> +do_compile[postfuncs] += "${@'rk_generate_env' if 'rk-u-boot-env' in d.getVar('MACHINEOVERRIDES').split(':') else 'rk_no_env'}"
>>> +
>>
>> Could be simplified with:
>>
>> """
>> rk_generate_env() {
>>      if bb.utils.contains('MACHINE_FEATURES', 'rk-u-boot-env', True, False,
>> d):
>>          bb.warn("the rk-u-boot-env MACHINE_FEATURE is not supported for this
>> build")
>> }
>>
>> rk_generate_env:rk-u-boot-env() {
>> [content of rk_generate_env in v7]
>> }
>>
>> do_compile[postfuncs] += "rk_generate_env;"
>> """
> 
> "...eye of the beholder..." I guess. You've suggested this before as a
> simplification, but I guess I've never quite gotten accustomed to bitbake's
> override mechanism. To this day I still find it odd... and unique.
> 
> What I've proposed is a simple: "IF <string> IN <object> THEN <generate_env>
> ELSE <no_env>", which is mostly plain English.
> 
> With overrides you're saying "always call this function... oh
> wait! unless overrides are in place, then call this one or this other one...
> wait! are overrides in place? what are the overrides?..."
> 

Yeah, your version requires more (of my) brain parsing power to figure 
out what it's doing. Just different patterns in different people brains 
that's all :)

> In any case what you've proposed doesn't work. The non-override
> rk_generate_env() function would need a "python" prefix and try as I might, it
> seems you can't have a set of override functions with one being python and the
> other one not. Without the "python" bitbake chokes if the non-override one is
> called. With the "python" bitbake will still choke if the overridden version
> is called:
> 

That's... an interesting side effect. I don't know if this is 
expected/known? But thanks for testing and reporting.

> Try making the change you suggest, both with and without the "python"
> qualifier for the non-overridden version, then try building with both
> MACHINE=rock2-square and MACHINE=rock-5a; one or the other won't work with or
> without the "python".
> 

So, we can actually have this python code migrated to shell, if I'm not 
mistaken the following should work:

"""
rk_generate_env() {
     if [ "${@bb.utils.contains('MACHINE_FEATURES', 'rk-u-boot-env', 1, 
0, d)}" -eq 1 ]; then
         bbwarn "the rk-u-boot-env MACHINE_FEATURE is not supported for 
this build"
     fi
}
"""

> In any case I like my version :-)

And I like mine :D

Just a matter of taste in any case, nothing wrong with it, just 
different ways of coming to the same result :)

Cheers,
Quentin
diff mbox series

Patch

diff --git a/README b/README
index c76fc9131276..605773d4ecd3 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/conf/machine/include/rockchip-rk-u-boot-env.inc b/conf/machine/include/rockchip-rk-u-boot-env.inc
new file mode 100644
index 000000000000..4eb877f77c48
--- /dev/null
+++ b/conf/machine/include/rockchip-rk-u-boot-env.inc
@@ -0,0 +1,4 @@ 
+# 'rk-u-boot-env' indicates the user wants to be able to save their U-Boot
+# environment back to the drive from which the device was booted
+MACHINEOVERRIDES .= "${@bb.utils.contains('MACHINE_FEATURES', 'rk-u-boot-env', ':rk-u-boot-env', '', d)}"
+IMAGE_INSTALL:append:rk-u-boot-env = " u-boot-fw-utils u-boot-env"
diff --git a/conf/machine/include/rockchip-wic.inc b/conf/machine/include/rockchip-wic.inc
index 147a36685d7d..b5ee6e0c2724 100644
--- a/conf/machine/include/rockchip-wic.inc
+++ b/conf/machine/include/rockchip-wic.inc
@@ -1,6 +1,7 @@ 
 # common meta-rockchip wic/wks items
 
 require conf/machine/include/rockchip-extlinux.inc
+require conf/machine/include/rockchip-rk-u-boot-env.inc
 
 SPL_BINARY ?= "idbloader.img"
 
@@ -11,7 +12,13 @@  WKS_FILE_DEPENDS ?= " \
 	virtual/bootloader \
 	"
 
+RK_IMAGE_INCLUDES_UBOOT_ENV ?= "no"
+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 = " \
+	RK_UBOOT_ENV \
 	SPL_BINARY \
 	UBOOT_SUFFIX \
 	"
diff --git a/conf/machine/rock2-square.conf b/conf/machine/rock2-square.conf
index 9468b9a6b559..b31eb574015f 100644
--- a/conf/machine/rock2-square.conf
+++ b/conf/machine/rock2-square.conf
@@ -16,4 +16,3 @@  UBOOT_MACHINE = "rock2_defconfig"
 # image class
 IMAGE_FSTYPES += "rockchip-gpt-img"
 IMAGE_CLASSES += "rockchip-gpt-img"
-
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 a83179a9f007..33b521021a1c 100644
--- a/recipes-bsp/u-boot/u-boot_%.bbappend
+++ b/recipes-bsp/u-boot/u-boot_%.bbappend
@@ -1,7 +1,13 @@ 
+FILESEXTRAPATHS:prepend := "${THISDIR}/files:"
+
+SRC_URI:append:rk-u-boot-env = " file://rockchip-enable-environment-mmc.cfg"
+SRCREV:rk-u-boot-env = "cdfcc37428e06f4730ab9a17cc084eeb7676ea1a"
+
 # 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 = " \
@@ -34,3 +40,42 @@  do_compile:append:rock2-square () {
 		cp ${B}/spl/${SPL_BINARY} ${B}
 	fi
 }
+
+python rk_no_env() {
+    if bb.utils.contains('MACHINE_FEATURES', 'rk-u-boot-env', True, False, d):
+        bb.warn("the rk-u-boot-env MACHINE_FEATURE is not supported for this build")
+}
+
+rk_generate_env() {
+	if [ ! -f "${B}/.config" ]; then
+		echo "U-Boot .config not found, can't determine environment size"
+		return 1
+	fi
+	cat ${B}/.config | grep "^CONFIG_ENV_SIZE=" > /dev/null
+	if [ $? -ne 0 ]; then
+		echo "can not find CONFIG_ENV_SIZE value in U-Boot .config"
+		return 1
+	fi
+
+	UBOOT_ENV_SIZE="$(cat ${B}/.config | grep "^CONFIG_ENV_SIZE=" | cut -d'=' -f2)"
+
+	# linux user-space U-Boot env config file
+	echo "/dev/disk/by-partlabel/uboot_env 0x0000 ${UBOOT_ENV_SIZE}" > ${WORKDIR}/fw_env.config
+
+	# convert text-based environment to binary suitable for image
+	if [ "${@bb.utils.to_boolean(d.getVar('RK_IMAGE_INCLUDES_UBOOT_ENV'), False)}" = "True" ]; then
+		if [ ! -f ${B}/u-boot-initial-env ]; then
+			echo "initial, text-formatted U-Boot environment file \"${B}/u-boot-initial-env\" not found"
+			return 1
+		fi
+		mkenvimage -s ${UBOOT_ENV_SIZE} ${B}/u-boot-initial-env -o ${WORKDIR}/u-boot.env
+	fi
+}
+do_compile[postfuncs] += "${@'rk_generate_env' if 'rk-u-boot-env' in d.getVar('MACHINEOVERRIDES').split(':') else 'rk_no_env'}"
+
+do_deploy:append:rk-u-boot-env() {
+	if [ -f ${WORKDIR}/u-boot.env -a "${@bb.utils.to_boolean(d.getVar('RK_IMAGE_INCLUDES_UBOOT_ENV'),False)}" = "True" ]; then
+		install -d ${DEPLOYDIR}
+		install -m 0644 ${WORKDIR}/u-boot.env ${DEPLOYDIR}
+	fi
+}
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..05b7048f1734
--- /dev/null
+++ b/scripts/dump-uboot-env-from-yocto-image.sh
@@ -0,0 +1,29 @@ 
+#/bin/bash
+# SPDX-License-Identifier: OSL-3.0
+#
+# a program that can take a wic file and dump out the contents
+# of the U-Boot environment in canonical hex+ascii format
+# (assuming the "rockchip" layout specified in this layer's wic file)
+
+# check for programs
+check_pgm() {
+	$1 --help > /dev/null 2>&1
+	if [ $? -ne 0 ]; then
+		echo "required program \"$1\" not found"
+		exit 1
+	fi
+}
+check_pgm dd
+check_pgm hexdump
+
+if [ $# -ne 1 ]; then
+	echo "required param missing: yocto wic image"
+	exit 1
+fi
+if [ ! -f "$1" ]; then
+	echo "specified file \"$1\" not found"
+	exit 1
+fi
+
+SKIP=$(( 8128 * 512 ))
+dd if="$1" ibs=1 skip=$SKIP count=32k 2> /dev/null | hexdump -C
diff --git a/wic/rockchip.wks b/wic/rockchip.wks
index 9ba3352b51bb..bc65f73b0cf3 100644
--- a/wic/rockchip.wks
+++ b/wic/rockchip.wks
@@ -22,7 +22,7 @@  part loader1   --offset 64s    --fixed-size 3552K --fstype=none --part-name load
 part v_storage --offset 7168s  --fixed-size 256K  --fstype=none --part-name v_storage
 part reserved  --offset 7680s  --fixed-size 192K  --fstype=none --part-name reserved
 part reserved1 --offset 8064s  --fixed-size 32K   --fstype=none --part-name reserved1
-part uboot_env --offset 8128s  --fixed-size 32K   --fstype=none --part-name uboot_env
+part uboot_env --offset 8128s  --fixed-size 32K   --fstype=none --part-name uboot_env ${RK_UBOOT_ENV}
 part reserved2 --offset 8192s  --fixed-size 4096K --fstype=none --part-name reserved2
 part loader2   --offset 16384s --fixed-size 4096K --fstype=none --part-name loader2   --source rawcopy --sourceparams="file=u-boot.${UBOOT_SUFFIX}"
 part atf       --offset 24576s --fixed-size 4096K --fstype=none --part-name atf