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

[Rasmus Villemoes <rasmus.villemoes@prevas.dk>]

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