[PATCH 0/2] allow environment to be updated from dtb

[PATCH 0/2] allow environment to be updated from dtb

[Rasmus Villemoes]

Patch 1 adds a config option for updating the environment, after it
has been loaded from its persistent location, with a set of key/value
pairs from a node in the control DTB.

This is useful to create a few different images based on the same
U-Boot binary, e.g. one for normal use, one for development and one
for bootstrapping the board. It's also useful when SPL loads U-Boot
from a FIT image and decides between multiple possible control DTBs;
those control DTBs can then contain tweaks of the environment suitable
for that particular board variant.

Once those tweaks have been applied, one may no longer wish for the
DTB settings to be applied on subsequent boots; this is easily
achieved by clearing the fdt_env_path variable and saving the
environment.

In a previous version, I used a single property whose value was
expected to have the form "foo=1\0bar=234\0". But using a node and
individual property/value pairs is both more readable and more
flexible - for example, the individual control dtbs can easily be
prepared from each other using the fdtput utility, or with suitable
includes in the .dts. Combining fragments in that way is not possible
with a single property.

Patch 2 simply adds a small test case to the sandbox.

Rasmus Villemoes (2):
  env: allow environment to be amended from control dtb
  sandbox: add test of CONFIG_ENV_IMPORT_FDT

 arch/sandbox/dts/test.dts   |  7 +++++++
 common/board_r.c            |  2 ++
 configs/sandbox64_defconfig |  1 +
 configs/sandbox_defconfig   |  1 +
 env/Kconfig                 | 18 ++++++++++++++++++
 env/common.c                | 23 +++++++++++++++++++++++
 include/env.h               | 15 +++++++++++++++
 include/env_default.h       |  3 +++
 test/env/Makefile           |  1 +
 test/env/fdt.c              | 20 ++++++++++++++++++++
 10 files changed, 91 insertions(+)
 create mode 100644 test/env/fdt.c

-- 
2.29.2

[PATCH 1/2] env: allow environment to be amended from control dtb

[Rasmus Villemoes]

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(+)

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;
+	offset = fdt_path_offset(blob, path);
+	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

[PATCH 2/2] sandbox: add test of CONFIG_ENV_IMPORT_FDT

[Rasmus Villemoes]

Check that a variable defined in /config/environment is found in the
run-time environment, and that clearing fdt_env_path from within that
node works.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 arch/sandbox/dts/test.dts   |  7 +++++++
 configs/sandbox64_defconfig |  1 +
 configs/sandbox_defconfig   |  1 +
 test/env/Makefile           |  1 +
 test/env/fdt.c              | 20 ++++++++++++++++++++
 5 files changed, 30 insertions(+)
 create mode 100644 test/env/fdt.c

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 899e75f260..442f269a6c 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -51,6 +51,13 @@
 		osd0 = "/osd";
 	};
 
+	config {
+		environment {
+			from_fdt = "yes";
+			fdt_env_path = "";
+		};
+	};
+
 	audio: audio-codec {
 		compatible = "sandbox,audio-codec";
 		#sound-dai-cells = <1>;
diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
index 4648808d51..b193a83b5c 100644
--- a/configs/sandbox64_defconfig
+++ b/configs/sandbox64_defconfig
@@ -91,6 +91,7 @@ CONFIG_ENV_IS_NOWHERE=y
 CONFIG_ENV_IS_IN_EXT4=y
 CONFIG_ENV_EXT4_INTERFACE="host"
 CONFIG_ENV_EXT4_DEVICE_AND_PART="0:0"
+CONFIG_ENV_IMPORT_FDT=y
 CONFIG_BOOTP_SEND_HOSTNAME=y
 CONFIG_NETCONSOLE=y
 CONFIG_IP_DEFRAG=y
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index 5da8d1679e..25baabb6a1 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -106,6 +106,7 @@ CONFIG_ENV_IS_NOWHERE=y
 CONFIG_ENV_IS_IN_EXT4=y
 CONFIG_ENV_EXT4_INTERFACE="host"
 CONFIG_ENV_EXT4_DEVICE_AND_PART="0:0"
+CONFIG_ENV_IMPORT_FDT=y
 CONFIG_BOOTP_SEND_HOSTNAME=y
 CONFIG_NETCONSOLE=y
 CONFIG_IP_DEFRAG=y
diff --git a/test/env/Makefile b/test/env/Makefile
index 5c8eae31b0..170bece3d6 100644
--- a/test/env/Makefile
+++ b/test/env/Makefile
@@ -5,3 +5,4 @@
 obj-y += cmd_ut_env.o
 obj-y += attr.o
 obj-y += hashtable.o
+obj-y += fdt.o
diff --git a/test/env/fdt.c b/test/env/fdt.c
new file mode 100644
index 0000000000..30bfa88c35
--- /dev/null
+++ b/test/env/fdt.c
@@ -0,0 +1,20 @@
+#include <common.h>
+#include <command.h>
+#include <env_attr.h>
+#include <test/env.h>
+#include <test/ut.h>
+
+static int env_test_fdt_import(struct unit_test_state *uts)
+{
+	const char *val;
+
+	val = env_get("from_fdt");
+	ut_assertnonnull(val);
+	ut_asserteq_str("yes", val);
+
+	val = env_get("fdt_env_path");
+	ut_assertnull(val);
+
+	return 0;
+}
+ENV_TEST(env_test_fdt_import, 0);
-- 
2.29.2

[PATCH 1/2] env: allow environment to be amended from control dtb

[Simon Glass]

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

[PATCH 2/2] sandbox: add test of CONFIG_ENV_IMPORT_FDT

[Simon Glass]

On Wed, 14 Apr 2021 at 10:43, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Check that a variable defined in /config/environment is found in the
> run-time environment, and that clearing fdt_env_path from within that
> node works.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  arch/sandbox/dts/test.dts   |  7 +++++++
>  configs/sandbox64_defconfig |  1 +
>  configs/sandbox_defconfig   |  1 +
>  test/env/Makefile           |  1 +
>  test/env/fdt.c              | 20 ++++++++++++++++++++
>  5 files changed, 30 insertions(+)
>  create mode 100644 test/env/fdt.c

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 1/2] env: allow environment to be amended from control dtb

[Rasmus Villemoes]

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?

Thanks,
Rasmus

[PATCH 1/2] env: allow environment to be amended from control dtb

[Rasmus Villemoes]

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

[PATCH v2 0/2] allow environment to be updated from dtb

[Rasmus Villemoes]

Patch 1 adds a config option for updating the environment, after it
has been loaded from its persistent location, with a set of key/value
pairs from a node in the control DTB.

This is useful to create a few different images based on the same
U-Boot binary, e.g. one for normal use, one for development and one
for bootstrapping the board. It's also useful when SPL loads U-Boot
from a FIT image and decides between multiple possible control DTBs;
those control DTBs can then contain tweaks of the environment suitable
for that particular board variant.

Once those tweaks have been applied, one may no longer wish for the
DTB settings to be applied on subsequent boots; this is easily
achieved by clearing the fdt_env_path variable and saving the
environment.

In a previous version, I used a single property whose value was
expected to have the form "foo=1\0bar=234\0". But using a node and
individual property/value pairs is both more readable and more
flexible - for example, the individual control dtbs can easily be
prepared from each other using the fdtput utility, or with suitable
includes in the .dts. Combining fragments in that way is not possible
with a single property.

Patch 2 simply adds a small test case to the sandbox.

v2: Add Simon's R-b tags, use livetree/ofnode API.

Rasmus Villemoes (2):
  env: allow environment to be amended from control dtb
  sandbox: add test of CONFIG_ENV_IMPORT_FDT

 arch/sandbox/dts/test.dts   |  7 +++++++
 common/board_r.c            |  2 ++
 configs/sandbox64_defconfig |  1 +
 configs/sandbox_defconfig   |  1 +
 env/Kconfig                 | 18 ++++++++++++++++++
 env/common.c                | 30 ++++++++++++++++++++++++++++++
 include/env.h               | 15 +++++++++++++++
 include/env_default.h       |  3 +++
 test/env/Makefile           |  1 +
 test/env/fdt.c              | 20 ++++++++++++++++++++
 10 files changed, 98 insertions(+)
 create mode 100644 test/env/fdt.c

-- 
2.29.2

[PATCH v2 1/2] env: allow environment to be amended from control dtb

[Rasmus Villemoes]

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

[PATCH v2 2/2] sandbox: add test of CONFIG_ENV_IMPORT_FDT

[Rasmus Villemoes]

Check that a variable defined in /config/environment is found in the
run-time environment, and that clearing fdt_env_path from within that
node works.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 arch/sandbox/dts/test.dts   |  7 +++++++
 configs/sandbox64_defconfig |  1 +
 configs/sandbox_defconfig   |  1 +
 test/env/Makefile           |  1 +
 test/env/fdt.c              | 20 ++++++++++++++++++++
 5 files changed, 30 insertions(+)
 create mode 100644 test/env/fdt.c

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 48240aa26f..8dfeab8562 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -53,6 +53,13 @@
 		osd0 = "/osd";
 	};
 
+	config {
+		environment {
+			from_fdt = "yes";
+			fdt_env_path = "";
+		};
+	};
+
 	audio: audio-codec {
 		compatible = "sandbox,audio-codec";
 		#sound-dai-cells = <1>;
diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
index 4648808d51..b193a83b5c 100644
--- a/configs/sandbox64_defconfig
+++ b/configs/sandbox64_defconfig
@@ -91,6 +91,7 @@ CONFIG_ENV_IS_NOWHERE=y
 CONFIG_ENV_IS_IN_EXT4=y
 CONFIG_ENV_EXT4_INTERFACE="host"
 CONFIG_ENV_EXT4_DEVICE_AND_PART="0:0"
+CONFIG_ENV_IMPORT_FDT=y
 CONFIG_BOOTP_SEND_HOSTNAME=y
 CONFIG_NETCONSOLE=y
 CONFIG_IP_DEFRAG=y
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index 6038dcba95..832a7fbf3e 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -106,6 +106,7 @@ CONFIG_ENV_IS_NOWHERE=y
 CONFIG_ENV_IS_IN_EXT4=y
 CONFIG_ENV_EXT4_INTERFACE="host"
 CONFIG_ENV_EXT4_DEVICE_AND_PART="0:0"
+CONFIG_ENV_IMPORT_FDT=y
 CONFIG_BOOTP_SEND_HOSTNAME=y
 CONFIG_NETCONSOLE=y
 CONFIG_IP_DEFRAG=y
diff --git a/test/env/Makefile b/test/env/Makefile
index 5c8eae31b0..170bece3d6 100644
--- a/test/env/Makefile
+++ b/test/env/Makefile
@@ -5,3 +5,4 @@
 obj-y += cmd_ut_env.o
 obj-y += attr.o
 obj-y += hashtable.o
+obj-y += fdt.o
diff --git a/test/env/fdt.c b/test/env/fdt.c
new file mode 100644
index 0000000000..30bfa88c35
--- /dev/null
+++ b/test/env/fdt.c
@@ -0,0 +1,20 @@
+#include <common.h>
+#include <command.h>
+#include <env_attr.h>
+#include <test/env.h>
+#include <test/ut.h>
+
+static int env_test_fdt_import(struct unit_test_state *uts)
+{
+	const char *val;
+
+	val = env_get("from_fdt");
+	ut_assertnonnull(val);
+	ut_asserteq_str("yes", val);
+
+	val = env_get("fdt_env_path");
+	ut_assertnull(val);
+
+	return 0;
+}
+ENV_TEST(env_test_fdt_import, 0);
-- 
2.29.2

[PATCH v2 1/2] env: allow environment to be amended from control dtb

[Joe Hershberger]

On Wed, Apr 21, 2021 at 4:07 AM 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.
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Cool addition.

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[PATCH v2 2/2] sandbox: add test of CONFIG_ENV_IMPORT_FDT

[Joe Hershberger]

On Wed, Apr 21, 2021 at 4:07 AM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Check that a variable defined in /config/environment is found in the
> run-time environment, and that clearing fdt_env_path from within that
> node works.
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[PATCH v2 1/2] env: allow environment to be amended from control dtb

[Tom Rini]

On Wed, Apr 21, 2021 at 11:06:54AM +0200, 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.
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Acked-by: Joe Hershberger <joe.hershberger@ni.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210506/53393f6c/attachment.sig>

[PATCH v2 2/2] sandbox: add test of CONFIG_ENV_IMPORT_FDT

[Tom Rini]

On Wed, Apr 21, 2021 at 11:06:55AM +0200, Rasmus Villemoes wrote:

> Check that a variable defined in /config/environment is found in the
> run-time environment, and that clearing fdt_env_path from within that
> node works.
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Acked-by: Joe Hershberger <joe.hershberger@ni.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210506/671ceade/attachment.sig>