[Buildroot] [RFC v2] boot/uboot: add option to install custom environment file

[Heiko Thiery]

Hi Yann,

Am Do., 3. Aug. 2023 um 21:41 Uhr schrieb Yann E. MORIN
<yann.morin.1998 at free.fr>:
>
> Heiko, All,
>
> On 2023-08-03 14:10 +0200, Heiko Thiery spake thusly:
> > U-Boot has the capability to set the environment variables via a text file.
> > The text file has to be located in the U-Boot board source directory and
> > selected via the CONFIG_ENV_SOURCE_FILE option. The CONFIG_ENV_SOURCE_FILE
> > must only contain the filename without the '.env' suffix.
> >
> > Thus the buildroot option BR2_TARGET_UBOOT_CUSTOM_ENVIRONMENT is added
> > that needs the information about the source of the file in the buildroot
> > environment (BR2_TARGET_UBOOT_CUSTOM_ENVIRONMENT_SOURCE).
> >
> > Since the environment file must be located in the U-Boot board source
> > directory. This directory is <SRC>/board/<VENDOR>/<BOARDNAME>.
> >
> > Thes information about vendor name and board name are available in the
> > U-Boot .config file and can be extracted from there to determine the
> > destination directoy.
> >
> > Cc: Michael Walle <michael at walle.cc>
> > Signed-off-by: Heiko Thiery <heiko.thiery at gmail.com>
> > ---
> [--SNIP--]
> > diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
> > index 8b726eaa57..894a0bd3b2 100644
> > --- a/boot/uboot/Config.in
> > +++ b/boot/uboot/Config.in
> > @@ -607,6 +607,22 @@ config BR2_TARGET_UBOOT_CUSTOM_DTS_PATH
> >
> >  endif
> >
> > +config BR2_TARGET_UBOOT_CUSTOM_ENVIRONMENT
> > +     bool "custom environment"
> > +     help
> > +       Provide a custom u-boot environment file. This will be
> > +       copied to the U-Boot source path and enabled via the
> > +       U-Boot config option CONFIG_ENV_SOURCE_FILE. The target
> > +       path will be determined based on the U-Boot configuration.
> > +
> > +if BR2_TARGET_UBOOT_CUSTOM_ENVIRONMENT
> > +config BR2_TARGET_UBOOT_CUSTOM_ENVIRONMENT_SOURCE
> > +     string "Custom environment source file"
> > +     help
> > +       Path to U-Boot custom environment file.
> > +
> > +endif
>
> We don't really need a boolean option to guard a single string option. I
> know we tend to do that a lot, but I find that to be an anti-pattern.
>
> In this case, the empty string is as good as saying "no" to the boolean
> option, so we can just live with the sting option, and then (see below)...
>
> (of course, if the empty string _has_ a special meaning, then we'd need
> a boolean, but that's usually not the case in such situations).

Ok, makes sense. Previously I had 2 config options for source and
destination and removed the destination in v2.

> >  config BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS
> >       string "Custom make options"
> >       help
> > diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
> > index b3d26b16fe..35e26ade2d 100644
> > --- a/boot/uboot/uboot.mk
> > +++ b/boot/uboot/uboot.mk
> > @@ -181,6 +181,26 @@ UBOOT_PRE_BUILD_HOOKS += UBOOT_COPY_ATF_FIRMWARE
> >  endif
> >  endif
> >
> > +#
> > +# Prepare for custom environment
> > +#
> > +ifeq ($(BR2_TARGET_UBOOT_CUSTOM_ENVIRONMENT),y)
> > +ifeq ($(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_ENVIRONMENT_SOURCE)),)
> > +$(error No custom environment source file specified, check your BR2_TARGET_UBOOT_CUSTOM_ENVIRONMENT_SOURCE setting)
> > +endif
>
> ... we don't need to check the sanity of the settings: empty means don't
> use a custom env file, set means use that file as custom env file.

Ok

>
> > +define UBOOT_COPY_CUSTOM_ENVIRONMENT_FILE
> > +     cp -f $(BR2_TARGET_UBOOT_CUSTOM_ENVIRONMENT_SOURCE) $(@D)/board/$(shell grep CONFIG_SYS_VENDOR $(@D)/.config | sed 's/.*=//' | sed 's/"//g')/$(shell grep CONFIG_SYS_BOARD $(@D)/.config | sed 's/.*=//' | sed 's/"//g')/
>
> Please split this line into easier-to-parse construct, see below...
>
> > +endef
> > +UBOOT_PRE_BUILD_HOOKS += UBOOT_COPY_CUSTOM_ENVIRONMENT_FILE
>
> Is it a pre-build or a post-configure hook? I would think it should be a
> post-configure one...

I wanted to do it like the other steps, copy the files to the source
folder before building.

- UBOOT_COPY_ATF_FIRMWARE
- UBOOT_COPY_IMX_FW_FILES

>
> > +
> > +
> > +UBOOT_ENV_FILE_NAME=$(subst .env,,$(notdir $(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_ENVIRONMENT_SOURCE))))
>
> $(patsubst %.env,%,$(notdir $(UBOOT_CUSTOM_ENVIRONMENT_SOURCE)))
>
> > +define UBOOT_KCONFIG_CUSTOM_ENV_SOURCE
> > +     $(call KCONFIG_SET_OPT,CONFIG_ENV_SOURCE_FILE,"$(UBOOT_ENV_FILE_NAME)")
> > +endef
> > +endif
>
> So, the .mk code would look like;
>
>     UBOOT_CUSTOM_ENVIRONMENT_SOURCE = $(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_ENVIRONMENT_SOURCE)))
>     UBOOT_ENV_FILE_NAME=$(patsubst %.env,%,$(notdir $(UBOOT_CUSTOM_ENVIRONMENT_SOURCE)))
>     ifneq ($(UBOOT_CUSTOM_ENVIRONMENT_SOURCE),)
>     define UBOOT_COPY_CUSTOM_ENVIRONMENT_FILE
>         sys_config=$( ./utils/config --file $(@D)/.config -s CONFIG_SYS_VENDOR ); \

Creating a subshell to get the output of uitls/config does not work
that way. I have to do $(shell ...). But with that it works ;-)

>         sys_board=$( ./utils/config --file $(@D)/.config -s CONFIG_SYS_BOARD ); \
>         cp -f $(UBOOT_CUSTOM_ENVIRONMENT_SOURCE) $(@D)/board/$${sys_config}/$${sys_board}
>     endef
>     UBOOT_POST_CONFIGURE_HOOKS += UBOOT_COPY_CUSTOM_ENVIRONMENT_FILE
>
>     define UBOOT_KCONFIG_CUSTOM_ENV_SOURCE
>         $(call KCONFIG_SET_OPT,CONFIG_ENV_SOURCE_FILE,"$(UBOOT_ENV_FILE_NAME)")
>     endef
>     endif  # UBOOT_CUSTOM_ENVIRONMENT_SOURCE != ""
>
> Regards,
> Yann E. MORIN.

Many thanks for your comments! I will retest and prepare a v3.

-- 
Heiko