From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rasmus Villemoes Date: Wed, 21 Apr 2021 10:28:18 +0200 Subject: [PATCH 1/2] env: allow environment to be amended from control dtb In-Reply-To: References: <20210413224345.2692591-1-rasmus.villemoes@prevas.dk> <20210413224345.2692591-2-rasmus.villemoes@prevas.dk> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 21/04/2021 10.02, Rasmus Villemoes wrote: > On 21/04/2021 09.14, Simon Glass wrote: >> Hi Rasmus, >> >> On Wed, 14 Apr 2021 at 10:43, Rasmus Villemoes >> wrote: >>> >>> It can be useful to use the same U-Boot binary for multiple purposes, >>> say the normal one, one for developers that allow breaking into the >>> U-Boot shell, and one for use during bootstrapping which runs a >>> special-purpose bootcmd. Or one can have several board variants that >>> can share almost all boot logic, but just needs a few tweaks in the >>> variables used by the boot script. >>> >>> To that end, allow the control dtb to contain a /config/enviroment >>> node (or whatever one puts in fdt_env_path variable), whose >>> property/value pairs are used to update the run-time environment after >>> it has been loaded from its persistent location. >>> >>> The indirection via fdt_env_path is for maximum flexibility - for >>> example, should the user wish (or board logic dictate) that the values >>> in the DTB should no longer be applied, one simply needs to delete the >>> fdt_env_path variable; that can even be done automatically by >>> including a >>> >>> fdt_env_path = ""; >>> >>> property in the DTB node. >>> >>> Signed-off-by: Rasmus Villemoes >>> --- >>> common/board_r.c | 2 ++ >>> env/Kconfig | 18 ++++++++++++++++++ >>> env/common.c | 23 +++++++++++++++++++++++ >>> include/env.h | 15 +++++++++++++++ >>> include/env_default.h | 3 +++ >>> 5 files changed, 61 insertions(+) >>> >> >> Reviewed-by: Simon Glass >> >> Some suggestions below >> >>> diff --git a/common/board_r.c b/common/board_r.c >>> index c835ff8e26..3f82404772 100644 >>> --- a/common/board_r.c >>> +++ b/common/board_r.c >>> @@ -459,6 +459,8 @@ static int initr_env(void) >>> else >>> env_set_default(NULL, 0); >>> >>> + env_import_fdt(); >>> + >>> if (IS_ENABLED(CONFIG_OF_CONTROL)) >>> env_set_hex("fdtcontroladdr", >>> (unsigned long)map_to_sysmem(gd->fdt_blob)); >>> diff --git a/env/Kconfig b/env/Kconfig >>> index b473d7cfe1..aa800e37ce 100644 >>> --- a/env/Kconfig >>> +++ b/env/Kconfig >>> @@ -647,6 +647,24 @@ config DELAY_ENVIRONMENT >>> later by U-Boot code. With CONFIG_OF_CONTROL this is instead >>> controlled by the value of /config/load-environment. >>> >>> +config ENV_IMPORT_FDT >>> + bool "Amend environment by FDT properties" >>> + depends on OF_CONTROL >>> + help >>> + If selected, after the environment has been loaded from its >>> + persistent location, the "env_fdt_path" variable is looked >>> + up and used as a path to a node in the control DTB. The >>> + property/value pairs in that node is then used to update the >>> + run-time environment. This can be useful to use the same >>> + U-Boot binary with different board variants. >>> + >>> +config ENV_FDT_PATH >>> + string "Default value for env_fdt_path variable" >>> + depends on ENV_IMPORT_FDT >>> + default "/config/environment" >>> + help >>> + The initial value of the env_fdt_path variable. >>> + >>> config ENV_APPEND >>> bool "Always append the environment with new data" >>> default n >>> diff --git a/env/common.c b/env/common.c >>> index 2ee423beb5..af45e561ce 100644 >>> --- a/env/common.c >>> +++ b/env/common.c >>> @@ -345,3 +345,26 @@ int env_complete(char *var, int maxv, char *cmdv[], int bufsz, char *buf, >>> return found; >>> } >>> #endif >>> + >>> +#ifdef CONFIG_ENV_IMPORT_FDT >>> +void env_import_fdt(void) >>> +{ >>> + const void *blob = gd->fdt_blob; >>> + const char *path; >>> + int offset, prop; >>> + >>> + path = env_get("env_fdt_path"); >>> + if (!path || !path[0]) >>> + return; >> >> How about returning an error code indicating what happened? > > I considered that, but I'm not sure what the (single) caller would do > with that. Not having env_fdt_path set is a supported use case as I > explain. So here it's not really an error. However, if env_fdt_path is > set, but there's no such node in the DT blob, it might be worth printing > a warning (i.e. in the "offset < 0" case below). > >>> + offset = fdt_path_offset(blob, path); >> >> Could we use the livetree API here (ofnode)? > > Dunno, what's that? Have U-Boot started deserializing the FDT blob like > the linux kernel does and build an in-memory representation that's > easier to traverse? Ah, ok, I see, something like node = ofnode_path(path); if (!ofnode_valid(node)) /* no such node */ But I can't find any ofnode_for_each_property, though I guess it's just as easy to open-code it like test/dm/ofread.c does. Thanks, I'll give it a try. Rasmus