Hi Rasmus,

On Wed, 14 Apr 2021 at 10:43, Rasmus Villemoes rasmus.villemoes@prevas.dk 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 rasmus.villemoes@prevas.dk

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 sjg@chromium.org

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?

•   offset = fdt_path_offset(blob, path);
  

Could we use the livetree API here (ofnode)?

•   if (offset < 0)
  

•           return;
  

•   fdt_for_each_property_offset(prop, blob, offset) {
  

•           const char *name, *val;
  

•           val = fdt_getprop_by_offset(blob, prop, &name, NULL);
  

•           env_set(name, val);
  

•   }
  

+} +#endif diff --git a/include/env.h b/include/env.h index c15339a93f..6296502595 100644 --- a/include/env.h +++ b/include/env.h @@ -377,4 +377,19 @@ int env_get_char(int index);

• This is used for those unfortunate archs with crappy toolchains

*/ void env_reloc(void);

+/**

• • env_import_fdt() - Import environment values from device tree blob
• • This uses the value of the environment variable "env_fdt_path" as a
• • path to an fdt node, whose property/value pairs are added to the
• • environment.
• */

+#ifdef CONFIG_ENV_IMPORT_FDT +void env_import_fdt(void); +#else +static inline void env_import_fdt(void) {} +#endif

#endif diff --git a/include/env_default.h b/include/env_default.h index ea31a8eddf..1ddd64ba8f 100644 --- a/include/env_default.h +++ b/include/env_default.h @@ -103,6 +103,9 @@ const uchar default_environment[] = { #ifdef CONFIG_SYS_SOC "soc=" CONFIG_SYS_SOC "\0" #endif +#ifdef CONFIG_ENV_IMPORT_FDT

•   "env_fdt_path=" CONFIG_ENV_FDT_PATH             "\0"
  

+#endif #endif #if defined(CONFIG_BOOTCOUNT_BOOTLIMIT) && (CONFIG_BOOTCOUNT_BOOTLIMIT > 0) "bootlimit=" __stringify(CONFIG_BOOTCOUNT_BOOTLIMIT)"\0" -- 2.29.2

Regards, Simon

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 rasmus.villemoes@prevas.dk 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 rasmus.villemoes@prevas.dk

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 sjg@chromium.org

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

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.

Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- common/board_r.c | 2 ++ env/Kconfig | 18 ++++++++++++++++++ env/common.c | 30 ++++++++++++++++++++++++++++++ include/env.h | 15 +++++++++++++++ include/env_default.h | 3 +++ 5 files changed, 68 insertions(+)

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 08e49c2a47..78c534344f 100644 --- a/env/Kconfig +++ b/env/Kconfig @@ -669,6 +669,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 49bbb05eec..81e9e0b2aa 100644 --- a/env/common.c +++ b/env/common.c @@ -20,6 +20,7 @@ #include <errno.h> #include <malloc.h> #include <u-boot/crc.h> +#include <dm/ofnode.h>

DECLARE_GLOBAL_DATA_PTR;

@@ -334,3 +335,32 @@ 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 char *path; + struct ofprop prop; + ofnode node; + int res; + + path = env_get("env_fdt_path"); + if (!path || !path[0]) + return; + + node = ofnode_path(path); + if (!ofnode_valid(node)) { + printf("Warning: device tree node '%s' not found\n", path); + return; + } + + for (res = ofnode_get_first_property(node, &prop); + !res; + res = ofnode_get_next_property(&prop)) { + const char *name, *val; + + val = ofnode_get_property_by_prop(&prop, &name, NULL); + env_set(name, val); + } +} +#endif diff --git a/include/env.h b/include/env.h index b5731e4b9a..d5e2bcb530 100644 --- a/include/env.h +++ b/include/env.h @@ -375,4 +375,19 @@ int env_get_char(int index); * This is used for those unfortunate archs with crappy toolchains */ void env_reloc(void); + + +/** + * env_import_fdt() - Import environment values from device tree blob + * + * This uses the value of the environment variable "env_fdt_path" as a + * path to an fdt node, whose property/value pairs are added to the + * environment. + */ +#ifdef CONFIG_ENV_IMPORT_FDT +void env_import_fdt(void); +#else +static inline void env_import_fdt(void) {} +#endif + #endif diff --git a/include/env_default.h b/include/env_default.h index ea31a8eddf..1ddd64ba8f 100644 --- a/include/env_default.h +++ b/include/env_default.h @@ -103,6 +103,9 @@ const uchar default_environment[] = { #ifdef CONFIG_SYS_SOC "soc=" CONFIG_SYS_SOC "\0" #endif +#ifdef CONFIG_ENV_IMPORT_FDT + "env_fdt_path=" CONFIG_ENV_FDT_PATH "\0" +#endif #endif #if defined(CONFIG_BOOTCOUNT_BOOTLIMIT) && (CONFIG_BOOTCOUNT_BOOTLIMIT > 0) "bootlimit=" __stringify(CONFIG_BOOTCOUNT_BOOTLIMIT)"\0"