[meta-rockchip][PATCH v2 1/2] enable stored U-Boot environment

[meta-rockchip][PATCH v2 1/2] enable stored U-Boot environment

[Trevor Woerner]

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)}"
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)}"
+
 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 \
+	' '}"
 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
+
 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 --
+SRC_URI:append:rock-pi-e = " ${@bb.utils.contains('MACHINE_FEATURES', 'rk-u-boot-env', '', ' \
 	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)"
+	echo "/dev/mmcblk${UBOOT_MMC_DEV}p5 0x0000 ${UBOOT_ENV_SIZE}" > ${WORKDIR}/fw_env.config
+}
+
+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}
+}
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
+#
+# 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 [ ! -e "$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 a9e5508d3ff5..febb826bccc7 100644
--- a/wic/rockchip.wks
+++ b/wic/rockchip.wks
@@ -22,7 +22,7 @@ part loader1   --offset 64s    --fixed-size 3552K --fstype=none --source rawcopy
 part v_storage --offset 7168s  --fixed-size 256K  --fstype=none
 part reserved  --offset 7680s  --fixed-size 192K  --fstype=none
 part reserved1 --offset 8064s  --fixed-size 32K   --fstype=none
-part uboot_env --offset 8128s  --fixed-size 32K   --fstype=none
+part uboot_env --offset 8128s  --fixed-size 32K   --fstype=none ${RK_UBOOT_ENV}
 part reserved2 --offset 8192s  --fixed-size 4096K --fstype=none
 part loader2   --offset 16384s --fixed-size 4096K --fstype=none --source rawcopy --sourceparams="file=u-boot.${UBOOT_SUFFIX}"
 part atf       --offset 24576s --fixed-size 4096K --fstype=none
-- 
2.44.0.501.g19981daefd7c

[meta-rockchip][PATCH v2 2/2] fw_env.config: use partition name/label

[Trevor Woerner]

A filesystem label (/dev/disk/by-label) is a property of, and stored in, the
filesystem itself. Since the U-Boot environment partition is not formatted (it
is a raw copy of the generated, binary U-Boot environment) it is not possible
to assign it a label.

However, if GPT partitioning is being used, GPT supports the notion of
assigning labels/names to the partitions which are stored as part of the
GPT partition table itself (instead of being stored in the filesystem
in the partition). The naming is a bit confusing (different tools use
different names) but `wic` calls this "--part-name", `lsblk` calls this
"PARTLABEL", and `parted` calls this "name".

In Linux user-space these partition labels are referenced via
/dev/disk/by-partlabel and provide an excellent way of finding the GPT
partition containing the U-Boot environment regardless of which backing
device is actually being used (e.g. mmcblk0, mmcblk1... i.e. emmc,
sdcard...).

Tested in both systemd and sysvinit.
Tested on rock-5b (both emmc and sdcard).

Signed-off-by: Trevor Woerner <twoerner@gmail.com>
---
v2 changes:
- new
---
 recipes-bsp/u-boot/u-boot_%.bbappend | 3 +--
 wic/rockchip.wks                     | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/recipes-bsp/u-boot/u-boot_%.bbappend b/recipes-bsp/u-boot/u-boot_%.bbappend
index 18702428a787..a1dc25449229 100644
--- a/recipes-bsp/u-boot/u-boot_%.bbappend
+++ b/recipes-bsp/u-boot/u-boot_%.bbappend
@@ -59,8 +59,7 @@ do_compile:append:rock2-square () {
 
 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)"
-	echo "/dev/mmcblk${UBOOT_MMC_DEV}p5 0x0000 ${UBOOT_ENV_SIZE}" > ${WORKDIR}/fw_env.config
+	echo "/dev/disk/by-partlabel/uboot_env 0x0000 ${UBOOT_ENV_SIZE}" > ${WORKDIR}/fw_env.config
 }
 
 do_deploy:append:rk-u-boot-env() {
diff --git a/wic/rockchip.wks b/wic/rockchip.wks
index febb826bccc7..f7e501cbd25e 100644
--- a/wic/rockchip.wks
+++ b/wic/rockchip.wks
@@ -22,7 +22,7 @@ part loader1   --offset 64s    --fixed-size 3552K --fstype=none --source rawcopy
 part v_storage --offset 7168s  --fixed-size 256K  --fstype=none
 part reserved  --offset 7680s  --fixed-size 192K  --fstype=none
 part reserved1 --offset 8064s  --fixed-size 32K   --fstype=none
-part uboot_env --offset 8128s  --fixed-size 32K   --fstype=none ${RK_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 loader2   --offset 16384s --fixed-size 4096K --fstype=none --source rawcopy --sourceparams="file=u-boot.${UBOOT_SUFFIX}"
 part atf       --offset 24576s --fixed-size 4096K --fstype=none
-- 
2.44.0.501.g19981daefd7c

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

[Quentin Schulz]

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.

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.

 >   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.

 > +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"
?

 > +	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.

 > +}
 > +
 > +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?

 > +}
 > 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?

Cheers,
Quentin

Re: [yocto-patches] [meta-rockchip][PATCH v2 2/2] fw_env.config: use partition name/label

[Quentin Schulz]

Hi Trevor,

On 4/11/24 03:16, Trevor Woerner via lists.yoctoproject.org wrote:
> A filesystem label (/dev/disk/by-label) is a property of, and stored in, the
> filesystem itself. Since the U-Boot environment partition is not formatted (it
> is a raw copy of the generated, binary U-Boot environment) it is not possible
> to assign it a label.
> 
> However, if GPT partitioning is being used, GPT supports the notion of
> assigning labels/names to the partitions which are stored as part of the
> GPT partition table itself (instead of being stored in the filesystem
> in the partition). The naming is a bit confusing (different tools use
> different names) but `wic` calls this "--part-name", `lsblk` calls this
> "PARTLABEL", and `parted` calls this "name".
> 
> In Linux user-space these partition labels are referenced via
> /dev/disk/by-partlabel and provide an excellent way of finding the GPT
> partition containing the U-Boot environment regardless of which backing
> device is actually being used (e.g. mmcblk0, mmcblk1... i.e. emmc,
> sdcard...).
> 
> Tested in both systemd and sysvinit.
> Tested on rock-5b (both emmc and sdcard).
> 
> Signed-off-by: Trevor Woerner <twoerner@gmail.com>
> ---
> v2 changes:
> - new
> ---
>   recipes-bsp/u-boot/u-boot_%.bbappend | 3 +--
>   wic/rockchip.wks                     | 2 +-
>   2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/recipes-bsp/u-boot/u-boot_%.bbappend b/recipes-bsp/u-boot/u-boot_%.bbappend
> index 18702428a787..a1dc25449229 100644
> --- a/recipes-bsp/u-boot/u-boot_%.bbappend
> +++ b/recipes-bsp/u-boot/u-boot_%.bbappend
> @@ -59,8 +59,7 @@ do_compile:append:rock2-square () {
>   
>   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)"
> -	echo "/dev/mmcblk${UBOOT_MMC_DEV}p5 0x0000 ${UBOOT_ENV_SIZE}" > ${WORKDIR}/fw_env.config
> +	echo "/dev/disk/by-partlabel/uboot_env 0x0000 ${UBOOT_ENV_SIZE}" > ${WORKDIR}/fw_env.config

If you move this patch first in the series, you don't need this diff as 
it would be squashed with the second patch (the one that is currently 
first in the series).

>   }
>   
>   do_deploy:append:rk-u-boot-env() {
> diff --git a/wic/rockchip.wks b/wic/rockchip.wks
> index febb826bccc7..f7e501cbd25e 100644
> --- a/wic/rockchip.wks
> +++ b/wic/rockchip.wks
> @@ -22,7 +22,7 @@ part loader1   --offset 64s    --fixed-size 3552K --fstype=none --source rawcopy
>   part v_storage --offset 7168s  --fixed-size 256K  --fstype=none
>   part reserved  --offset 7680s  --fixed-size 192K  --fstype=none
>   part reserved1 --offset 8064s  --fixed-size 32K   --fstype=none
> -part uboot_env --offset 8128s  --fixed-size 32K   --fstype=none ${RK_UBOOT_ENV}
> +part uboot_env --offset 8128s  --fixed-size 32K   --fstype=none --part-name uboot_env ${RK_UBOOT_ENV}

You could simply have --part-name uboot_env here no? if it was the first 
patch in the series I mean?

 From the commit log, it seems this is required if it is rawcopy but 
otherwise not valid? If true, considering that RK_UBOOT_ENV may be empty 
(and thus not have --source rawcopy), is this actually working for 
setups where MACHINE_FEATURES doesn't have rk-u-boot-env or 
RK_IMAGE_INCLUDES_UBOOT_ENV is not 1?

Cheers,
Quentin

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

[Trevor Woerner]

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

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

[Quentin Schulz]

Hi Trevor,

On 4/17/24 16:19, Trevor Woerner via lists.yoctoproject.org wrote:
[...]
>>> 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.
> 

The bbappend doesn't, but the original recipe does.

meta/recipes-bsp/u-boot/u-boot_2024.04.bb
-> meta/recipes-bsp/u-boot/u-boot.inc

inherit deploy

and there's a do_deploy there already, so we don't need to re-inherit it 
again in the bbappend I think?

>>>    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".
> 

I should start reading comments with more care again, sorry for the 
noise. The next release is 2024.07, c.f. 
https://docs.u-boot.org/en/latest/develop/release_cycle.html

> 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.
> 

Up to you.

>>> +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.
> 

https://www.shellcheck.net/wiki/SC2002 /me shrugs

>>> +	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.
> 

Reading the code, I guess you meant to say do_install instead. Thanks 
for pointing at this. This also means that nobody will be able to 
override this with their own fw_env.config from SRC_URI for example 
which I assume is the reason for reading it from WORKDIR. Not sure it's 
desirable but maybe?

Looking even more at the code, it seems WORKDIR is also used for the 
do_deploy of u-boot.inc. I'm actually wondering if this isn't breaking 
the sstate already? SHouldn't the do_deploy use the file from ${D} instead?

bitbake-getvar -r u-boot -f sstate-inputdirs --value do_install

returns None... so i'm certainly wrong somewhere :)

Been a long time since I played with sstate so I wouldn't be surprised 
if I completely missed on something or misunderstood/misremembered :)

>>
>>> +}
>>> +
>>> +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?
> 

Not sure to follow what's the concern about sstate? do_deploy sstate 
monitors everything in DEPLOYDIR so everything you install in there 
shall be under sstate?

For me do_deploy is for moving a file from one location to another, in 
DEPLOYDIR, nothing else. Everything that involves 
compilation/configuration is in do_compile/do_configure, so it's a bit 
odd to me to see a call to mkenvimage in do_deploy.

I guess you could have an issue with sstate if you use WORKDIR for 
u-boot.env since this wouldn't be under do_compile's sstate? But then 
just use ${B} for that instead?

Can you tell me a bit more about your concern here, I'm not getting the 
big picture yet :/

Cheers,
Quentin

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

[Trevor Woerner]

On Thu 2024-04-18 @ 10:43:10 AM, Quentin Schulz wrote:
> Hi Trevor,
> 
> On 4/17/24 16:19, Trevor Woerner via lists.yoctoproject.org wrote:
> [...]
> > > > 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.
> > 
> 
> The bbappend doesn't, but the original recipe does.
> 
> meta/recipes-bsp/u-boot/u-boot_2024.04.bb
> -> meta/recipes-bsp/u-boot/u-boot.inc
> 
> inherit deploy
> 
> and there's a do_deploy there already, so we don't need to re-inherit it
> again in the bbappend I think?
> 
> > > >    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".
> > 
> 
> I should start reading comments with more care again, sorry for the noise.
> The next release is 2024.07, c.f.
> https://docs.u-boot.org/en/latest/develop/release_cycle.html
> 
> > 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.
> > 
> 
> Up to you.
> 
> > > > +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.
> > 
> 
> https://www.shellcheck.net/wiki/SC2002 /me shrugs
> 
> > > > +	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.
> > 
> 
> Reading the code, I guess you meant to say do_install instead. Thanks for
> pointing at this. This also means that nobody will be able to override this
> with their own fw_env.config from SRC_URI for example which I assume is the
> reason for reading it from WORKDIR. Not sure it's desirable but maybe?
> 
> Looking even more at the code, it seems WORKDIR is also used for the
> do_deploy of u-boot.inc. I'm actually wondering if this isn't breaking the
> sstate already? SHouldn't the do_deploy use the file from ${D} instead?
> 
> bitbake-getvar -r u-boot -f sstate-inputdirs --value do_install
> 
> returns None... so i'm certainly wrong somewhere :)
> 
> Been a long time since I played with sstate so I wouldn't be surprised if I
> completely missed on something or misunderstood/misremembered :)
> 
> > > 
> > > > +}
> > > > +
> > > > +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?
> > 
> 
> Not sure to follow what's the concern about sstate? do_deploy sstate
> monitors everything in DEPLOYDIR so everything you install in there shall be
> under sstate?
> 
> For me do_deploy is for moving a file from one location to another, in
> DEPLOYDIR, nothing else. Everything that involves compilation/configuration
> is in do_compile/do_configure, so it's a bit odd to me to see a call to
> mkenvimage in do_deploy.
> 
> I guess you could have an issue with sstate if you use WORKDIR for
> u-boot.env since this wouldn't be under do_compile's sstate? But then just
> use ${B} for that instead?
> 
> Can you tell me a bit more about your concern here, I'm not getting the big
> picture yet :/

Firstly I want these files handled correctly by the oe-core U-Boot packaging
so they get included in the image/sstate correctly.

Second, I'm pretty sure that anything you want inserted into a wic image (i.e.
all the "--sourceparams=..." that are specified in the wks file) need to be placed in
${WORKDIR} for wic to find them when assembling the image. I might be wrong
about this, or there might be other directories that are searched, but I do
know that putting these artifacts in ${WORKDIR} will get them included in the
wic file as expected.

Thirdly, the default U-Boot environment is placed in "u-boot-initial-env" (a
text file). Later "u-boot-initial-env" is converted into a U-Boot environment
file (i.e. apparently it has a checksum prepended to it) via `mkenvimage`.
If all of that takes place in do_compile(), then the user will never have an
opportunity to inject any variables into the U-Boot environment before it is
converted to its binary form. So by doing this operation in do_deploy(),
it gives the user an opportunity to inject things between the creation of
"u-boot-initial-env" in do_compile(), and it's conversion to its binary form
(i.e. the form that is including into the wic image) in do_deploy().

I've had rauc working with my rockchip boards for about 5-6 months now, and I
know when I put that together initially I wanted to inject extra variables
during the build. I'll have to revisit whether or not I still need to do that,
but even if I don't, doesn't mean it wouldn't be a nice feature for a user.

Best regards,
	Trevor

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

[Quentin Schulz]

Hi Trevor,

On 4/24/24 19:43, Trevor Woerner via lists.yoctoproject.org wrote:
> On Thu 2024-04-18 @ 10:43:10 AM, Quentin Schulz wrote:
>> Hi Trevor,
>>
>> On 4/17/24 16:19, Trevor Woerner via lists.yoctoproject.org wrote:
>> [...]
>>>>
>>>>> +}
>>>>> +
>>>>> +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?
>>>
>>
>> Not sure to follow what's the concern about sstate? do_deploy sstate
>> monitors everything in DEPLOYDIR so everything you install in there shall be
>> under sstate?
>>
>> For me do_deploy is for moving a file from one location to another, in
>> DEPLOYDIR, nothing else. Everything that involves compilation/configuration
>> is in do_compile/do_configure, so it's a bit odd to me to see a call to
>> mkenvimage in do_deploy.
>>
>> I guess you could have an issue with sstate if you use WORKDIR for
>> u-boot.env since this wouldn't be under do_compile's sstate? But then just
>> use ${B} for that instead?
>>
>> Can you tell me a bit more about your concern here, I'm not getting the big
>> picture yet :/
> 
> Firstly I want these files handled correctly by the oe-core U-Boot packaging
> so they get included in the image/sstate correctly.
> 

Using WORKDIR as a way to pass files between tasks is a terrible idea, 
WORKDIR isn't properly cleaned. However... this is an issue in the 
"upstream" U-Boot recipe, not something you can deal with in your 
bbappend, so let's ignore this issue here.

I'm also not entirely sure how the sstate works for do_compile... 
Couldn't find any sstate-indirs and sstate-outdirs for 
do_compile/do_install with bitbake-getvar or a quick check with grep. Do 
you have any idea how/where this is handled?

> Second, I'm pretty sure that anything you want inserted into a wic image (i.e.
> all the "--sourceparams=..." that are specified in the wks file) need to be placed in
> ${WORKDIR} for wic to find them when assembling the image. I might be wrong
> about this, or there might be other directories that are searched, but I do
> know that putting these artifacts in ${WORKDIR} will get them included in the
> wic file as expected.
> 

I hope you're wrong about this. wic is run as part of the image recipe. 
It should therefore not assume anything about the location of WORKDIR of 
the u-boot package recipe. The recommended way for exchanging data 
between recipes is to use either SYSROOT_DIRS (and DEPENDS) or DEPLOYDIR 
(and DEPLOY_DIR_IMAGE from the other recipe; + do_task[depends] += 
"recipea:do_deploy"), so I assume this is what's used by wic.

The `wic create` command called in do_image_wic (IMAGE_CMD:wic) seems to 
be trying to get data from three directories: IMAGE_ROOTFS, 
DEPLOY_DIR_IMAGE, STAGING_DATADIR (and BUILDDIR passed as an environment 
variable).

U-Boot upstream recipe takes fw_env.config from WORKDIR and puts it into 
DEPLOYDIR as part of the do_deploy task. Therefore, we need to put it 
there (WORKDIR) now, so that we don't mess with the current 
implementation of U-Boot recipe.

However I don't think there's a reason for putting u-boot.env into the 
WORKDIR before installing it into DEPLOYDIR.

> Thirdly, the default U-Boot environment is placed in "u-boot-initial-env" (a
> text file). Later "u-boot-initial-env" is converted into a U-Boot environment
> file (i.e. apparently it has a checksum prepended to it) via `mkenvimage`.
> If all of that takes place in do_compile(), then the user will never have an
> opportunity to inject any variables into the U-Boot environment before it is
> converted to its binary form. So by doing this operation in do_deploy(),
> it gives the user an opportunity to inject things between the creation of
> "u-boot-initial-env" in do_compile(), and it's conversion to its binary form
> (i.e. the form that is including into the wic image) in do_deploy().
> 
> I've had rauc working with my rockchip boards for about 5-6 months now, and I
> know when I put that together initially I wanted to inject extra variables
> during the build. I'll have to revisit whether or not I still need to do that,
> but even if I don't, doesn't mean it wouldn't be a nice feature for a user.
> 

Agreed that's a nice thing to have.

We could use a do_compile postfunc for the mkenvimage part though. And 
then you could always do_compile:append to modify u-boot-initial-env 
file before your postfunc to create the mkenvimage runs.

We could also simply have another variable called e.g. 
UBOOT_INITIAL_ENV_EXTRA_VARS whose content is added at the end of 
fw_env.config in the upstream recipe (or in your bbappend before calling 
mkenvimage). That would allow to extend it.

I think it'd make sense to add support for creating the u-boot env "fs" 
(via mkenvimage) to the upstream recipe as I see this as something 
people could reuse on other vendors?

Thanks for taking the time to list the reasons, I see better what you're 
attempting to do in this series now :)

Cheers,
Quentin

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

[Trevor Woerner]

Gosh darn! Yes you're right. I kept saying WORKDIR over and over but I meant
DEPLOYDIR. I must have been distracted or something because in my mind, the
whole time, I was thinking $TMPDIR/deploy/images/$MACHINE but I kept writing
WORKDIR for some unknown reason.

On Thu 2024-04-25 @ 02:21:57 PM, Quentin Schulz via lists.yoctoproject.org wrote:
> Hi Trevor,
> 
> On 4/24/24 19:43, Trevor Woerner via lists.yoctoproject.org wrote:
> > On Thu 2024-04-18 @ 10:43:10 AM, Quentin Schulz wrote:
> > > Hi Trevor,
> > > 
> > > On 4/17/24 16:19, Trevor Woerner via lists.yoctoproject.org wrote:
> > > [...]
> > > > > 
> > > > > > +}
> > > > > > +
> > > > > > +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?
> > > > 
> > > 
> > > Not sure to follow what's the concern about sstate? do_deploy sstate
> > > monitors everything in DEPLOYDIR so everything you install in there shall be
> > > under sstate?
> > > 
> > > For me do_deploy is for moving a file from one location to another, in
> > > DEPLOYDIR, nothing else. Everything that involves compilation/configuration
> > > is in do_compile/do_configure, so it's a bit odd to me to see a call to
> > > mkenvimage in do_deploy.
> > > 
> > > I guess you could have an issue with sstate if you use WORKDIR for
> > > u-boot.env since this wouldn't be under do_compile's sstate? But then just
> > > use ${B} for that instead?
> > > 
> > > Can you tell me a bit more about your concern here, I'm not getting the big
> > > picture yet :/
> > 
> > Firstly I want these files handled correctly by the oe-core U-Boot packaging
> > so they get included in the image/sstate correctly.
> > 
> 
> Using WORKDIR as a way to pass files between tasks is a terrible idea,
> WORKDIR isn't properly cleaned. However... this is an issue in the
> "upstream" U-Boot recipe, not something you can deal with in your bbappend,
> so let's ignore this issue here.

Yep, agreed.

> I'm also not entirely sure how the sstate works for do_compile... Couldn't
> find any sstate-indirs and sstate-outdirs for do_compile/do_install with
> bitbake-getvar or a quick check with grep. Do you have any idea how/where
> this is handled?
> 
> > Second, I'm pretty sure that anything you want inserted into a wic image (i.e.
> > all the "--sourceparams=..." that are specified in the wks file) need to be placed in
> > ${WORKDIR} for wic to find them when assembling the image. I might be wrong
> > about this, or there might be other directories that are searched, but I do
> > know that putting these artifacts in ${WORKDIR} will get them included in the
> > wic file as expected.
> > 
> 
> I hope you're wrong about this.

Yep entirely. I was totally thinking DEPLOYDIR the whole time, but writing
WORKDIR.

> wic is run as part of the image recipe. It
> should therefore not assume anything about the location of WORKDIR of the
> u-boot package recipe. The recommended way for exchanging data between
> recipes is to use either SYSROOT_DIRS (and DEPENDS) or DEPLOYDIR (and
> DEPLOY_DIR_IMAGE from the other recipe; + do_task[depends] +=
> "recipea:do_deploy"), so I assume this is what's used by wic.
> 
> The `wic create` command called in do_image_wic (IMAGE_CMD:wic) seems to be
> trying to get data from three directories: IMAGE_ROOTFS, DEPLOY_DIR_IMAGE,
> STAGING_DATADIR (and BUILDDIR passed as an environment variable).
> 
> U-Boot upstream recipe takes fw_env.config from WORKDIR and puts it into
> DEPLOYDIR as part of the do_deploy task. Therefore, we need to put it there
> (WORKDIR) now, so that we don't mess with the current implementation of
> U-Boot recipe.
> 
> However I don't think there's a reason for putting u-boot.env into the
> WORKDIR before installing it into DEPLOYDIR.

Sounds good.

> > Thirdly, the default U-Boot environment is placed in "u-boot-initial-env" (a
> > text file). Later "u-boot-initial-env" is converted into a U-Boot environment
> > file (i.e. apparently it has a checksum prepended to it) via `mkenvimage`.
> > If all of that takes place in do_compile(), then the user will never have an
> > opportunity to inject any variables into the U-Boot environment before it is
> > converted to its binary form. So by doing this operation in do_deploy(),
> > it gives the user an opportunity to inject things between the creation of
> > "u-boot-initial-env" in do_compile(), and it's conversion to its binary form
> > (i.e. the form that is including into the wic image) in do_deploy().
> > 
> > I've had rauc working with my rockchip boards for about 5-6 months now, and I
> > know when I put that together initially I wanted to inject extra variables
> > during the build. I'll have to revisit whether or not I still need to do that,
> > but even if I don't, doesn't mean it wouldn't be a nice feature for a user.
> > 
> 
> Agreed that's a nice thing to have.
> 
> We could use a do_compile postfunc for the mkenvimage part though. And then
> you could always do_compile:append to modify u-boot-initial-env file before
> your postfunc to create the mkenvimage runs.

Okay.

> We could also simply have another variable called e.g.
> UBOOT_INITIAL_ENV_EXTRA_VARS whose content is added at the end of
> fw_env.config in the upstream recipe (or in your bbappend before calling
> mkenvimage). That would allow to extend it.
> 
> I think it'd make sense to add support for creating the u-boot env "fs" (via
> mkenvimage) to the upstream recipe as I see this as something people could
> reuse on other vendors?

There was something else strange that I think I noticed in the upstream
recipe. If I'm not mistaken, you can do an environment or you could do a
script, but not both. I'll have to double-check that and maybe take a stab at
fixing that as well (if I'm correct in my reading of the recipe).

> Thanks for taking the time to list the reasons, I see better what you're
> attempting to do in this series now :)
> 
> Cheers,
> Quentin
> 
> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#63): https://lists.yoctoproject.org/g/yocto-patches/message/63
> Mute This Topic: https://lists.yoctoproject.org/mt/105454848/900817
> Group Owner: yocto-patches+owner@lists.yoctoproject.org
> Unsubscribe: https://lists.yoctoproject.org/g/yocto-patches/leave/13168745/900817/63955952/xyzzy [twoerner@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
> 
>