Re: [docs] [PATCH v2 1/2] ref-manual: cover UBOOT_ENV variables

["Antonin Godard" <antonin.godard@bootlin.com>]

Hi Adrian,

On Mon Feb 24, 2025 at 11:14 PM CET, Adrian Freihofer wrote:
> Hi Antonin
>
> I will just send a v3 which fixes all your findings, except this one
> needs clarification:
>
> snip
>> > +      -  Adding a script ``boot.cmd`` as an uImage to ``/boot``::
>> > +
>> > +            UBOOT_ENV = "boot"
>> > +            UBOOT_ENV_SUFFIX = "scr"
>> 
>> Or shouldn't you set UBOOT_ENV_SRC_SUFFIX to "cmd" here too? I'm a
>> little
>> confused as why you refer to boot.cmd yet "cmd" is not defined here.
>
> UBOOT_ENV_SRC_SUFFIX ?= "cmd" is the default.

Okay so if I recapitulate, what is happening in your example is:

UBOOT_ENV = "boot"
UBOOT_ENV_SUFFIX = "scr"
UBOOT_ENV_SRC_SUFFIX = "cmd"
SRC_URI += "file://${UBOOT_ENV}.${UBOOT_ENV_SRC_SUFFIX}"

Which makes sense.

I wasn't sure of the default value for UBOOT_ENV_SRC_SUFFIX, which is why I
suggested setting all the variables in the example to avoid confusion.

> I have spent many hours trying to understand the code that handles the
> UBOOT_ENV* variables. It's super complicated. In uboot.inc, there is:
>
> do_compile () {
> if [ -n "${UBOOT_ENV}" ] && [ "${UBOOT_ENV_SUFFIX}" = "scr" ]; then
> ${UBOOT_MKIMAGE} -C none -A ${UBOOT_ARCH} -T script \
> -d ${UNPACKDIR}/${UBOOT_ENV_SRC} ${B}/${UBOOT_ENV_BINARY}
> fi
> }
>
> do_install () {
> if [ -n "${UBOOT_ENV}" ]; then
> install -m 644 ${B}/${UBOOT_ENV_BINARY} \
> ${D}/boot/${UBOOT_ENV_IMAGE}
> fi
> }
>
> Depending on the file ending respectively the UBOOT_ENV_SUFFIX
> variable, something different happens: Either
>  * a script which can be loaded by U-Boot's source command
>    or
>  * an image including a script which can be handled by U-Boot's load
>    command
>    ends up in /boot.
>
> In u-boot-config.bbclass there are more variables defined:
>  * Documented variables:
>    UBOOT_ENV_SUFFIX ?= "txt"
>    UBOOT_ENV ?= ""
>    UBOOT_ENV_SRC_SUFFIX ?= "cmd"
>  * Variables which are considered as internal and hence not documented
>    on purpose:
>    UBOOT_ENV_SRC ?= "${UBOOT_ENV}.${UBOOT_ENV_SRC_SUFFIX}"
>    UBOOT_ENV_BINARY ?= "${UBOOT_ENV}.${UBOOT_ENV_SUFFIX}"
>    ...two more...
>
> So my patch tries to keep it as simple as possible but still cover the
> 2 relevant use cases. To make it more clear, I added:
> "It defaults to ``cmd``."
> to the UBOOT_ENV_SRC_SUFFIX section.

Thanks, this will make it clearer too.

>
> I hope this makes it more understandable without a need for dropping
> all these details on the users.

I see your point, yes this code is far from intuitive.

> Note: It was even much more complicated when the kernel-
> fitimage.bbclass was in use as well. The kernel-fitimage.bbclass is now
> decoupled which simplifies (or maybe I should say fixes) it quite a
> bit.
>
>> 
> snip
>
> Thank you for the review!
> Adrian

Thanks for the efforts!

Antonin

-- 
Antonin Godard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com