[U-Boot] [PATCH v3 2/3] env: introduce macro ENV_IS_IN_SOMEWHERE

[Simon Glass]

Hi Patrick,

On Mon, 30 Sep 2019 at 03:30, Patrick DELAUNAY <patrick.delaunay at st.com> wrote:
>
> Hi Simon,
>
> > From: Simon Glass <sjg at chromium.org>
> > Sent: vendredi 27 septembre 2019 03:49
> >
> > On Wed, 18 Sep 2019 at 03:30, Patrick Delaunay <patrick.delaunay at st.com>
> > wrote:
> > >
> > > This patch introduce a macro ENV_IS_IN_SOMEWHERE to check if the the
> > > environment can be saved somewhere, in a device or in a file system,
> > > without assumption on CONFIG$(SPL_TPL_)ENV_IS_NOWHERE.
> > >
> > > Since the commit 208bd2b85ecc ("env: allow ENV_IS_NOWHERE with other
> > > storage target"), is is allowed to activated ENV_IS_NOWHERE with other
> > > CONFIG_IS_IN...
> > > It is only the case for U-Boot but the restriction in Kconfig could
> > > also removed for SPL and TPL (depends on !SPL_ENV_IS_NOWHERE /
> > depends
> > > on !TPL_ENV_IS_NOWHERE).
> > >
> > > This macro ENV_IS_IN_DEVICE can be used in code to check if the
> > > environment for U-Boot / SPL / TPL is really managed (at least one
> > > CONFIG$(SPL_TPL_)ENV_IS_IN_.. is activated).
> > >
> > > Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com>
> > > ---
> > >
> > > Changes in v3: None
> > > Changes in v2: None
> > >
> > >  cmd/nvedit.c  | 29 +++++++----------------------  include/env.h | 13
> > > +++++++++++++
> > >  2 files changed, 20 insertions(+), 22 deletions(-)
> >
> > I feel this is a bit confusion as we have ENV_IS_NOWHERE and
> > ENV_IS_IN_SOMEWHERE which are opposites. Can they both be true?
>
> I am not completely satisfied by the name " ENV_IS_IN_SOMEWHERE ".
>
> Perhaps other name will be less confusing:
>  "ENV_IS_IN_XXX", "ENV_IS_IN_DEVICE",  "ENV_IS_IN_STORAGE_MEDIUM",  "ENV_SAVE_SUPPORT" ...
>
> But I don't found a perfect solution...

Actually I think 'save' is good.

Perhaps ENV_IS_STORED?

>
> It is not really the opposite:  ENV_IS_IN_SOMEWHERE  means at least one of the config  "ENV_IS_IN...." is activated.
> => at leats one not volatile TARGET (= a strorage medium) is supported to load and save the environment
>
> Yes, then can both be true: ENV can support several target in U-Boot
> (but it is not the case today in SPL due to dependency in Kconfig):
> - CONFIG_ENV_IS_IN_EXT4
> - CONFIG_ENV_IS_IN_FAT
> - CONFIG_ENV_IS_IN_NAND
> ....
>
> The arrays env_locations[] to defined the location supported
> => env_get_location = weak function select the location (can ENVL_NOWHERE)
>
> For me, U-Boot have no reason to refuse ENV_IS_NOWHERE when the other ENV_IS_IN_XXX are activated , as it I supported in env code.
> => ENVL_NOWHERE can be see as a fallback (no error) when no other ENV location is available (each init failed / not supported in device tree)...
>
> I plan to use this feature in stm32mp1 platform: U-Boot select the ENV location according the boot device
>
> Boot from
> - SD Card : eMMC => env in a file in ext4 file
> - NAND =>  env in a UBI file
> - NOR => env in SPI flash parttion (RAW)
> - other case (boot form USB/serial for STM32CubeProgrammer):
>    U-Boot don't known where found the environment , use default environment => fallback with NOWHERE returned by env_get_location()
>
> board/st/stm32mp1/stm32mp1.c:757
>
> enum env_location env_get_location(enum env_operation op, int prio)
> {
>         u32 bootmode = get_bootmode();
>
>         if (prio)
>                 return ENVL_UNKNOWN;
>
>         switch (bootmode & TAMP_BOOT_DEVICE_MASK) {
> #ifdef CONFIG_ENV_IS_IN_EXT4
>         case BOOT_FLASH_SD:
>         case BOOT_FLASH_EMMC:
>                 return ENVL_EXT4;
> #endif
> #ifdef CONFIG_ENV_IS_IN_UBI
>         case BOOT_FLASH_NAND:
>                 return ENVL_UBI;
> #endif
> #ifdef CONFIG_ENV_IS_IN_SPI_FLASH
>         case BOOT_FLASH_NOR:
>                 return ENVL_SPI_FLASH;
> #endif
>         default:
>                 return ENVL_NOWHERE;
>         }
> }
>
> > So how about adding a comment as to what your new ENV_IS_NOWHERE
> > means?
>
> I don't think I change the meaning of ENV_IS_NOWHERE
>
> In env/Kconfig
>
>           Define this if you don't want to or can't have an environment stored
>           on a storage medium. In this case the environment will still exist
>           while U-Boot is running, but once U-Boot exits it will not be
>           stored. U-Boot will therefore always start up with a default
>           environment.
>
> I only change the dependency
>
> > Also, how about ENV_IS_SOMEWHERE?
>
> Yes I need to add comment on the added macro

[..]

Regards,
Simon