[PATCH V4 1/8] env_default: Allow CONFIG_EXTRA_ENV_TEXT to override CFG_EXTRA_ENV_SETTINGS

[Simon Glass]

Hi Nishanth,

On Tue, 22 Aug 2023 at 17:34, Nishanth Menon <nm at ti.com> wrote:
>
> On 17:16-20230822, Simon Glass wrote:
> > Hi Nishanth,
> >
> > On Tue, 22 Aug 2023 at 12:41, Nishanth Menon <nm at ti.com> wrote:
> > >
> > > CFG_EXTRA_ENV_SETTINGS is set in common board config files, This allows
> > > for majority of the settings to be set in a common manner. However, the
> > > minor variations between various board can be addressed by the board.env
> > > files. The board.env files are converted into CONFIG_EXTRA_ENV_TEXT.
> > >
> > > However, this creates a minor problem. For example:
> > > distro_bootcmd.h and used by ti_armv7_common.h uses it as:
> > >  #define BOOT_TARGET_DEVICES(func) \
> > >         func(MMC, mmc, 0) \
> > >         func(MMC, mmc, 1)
> >
> > OK, but we should be using bootstd for new boards.
>
> Agreed, and I think I am using bootstd[1] unless I am missing something
> completely.

Then why do you have BOOT_TARGET_DEVICES() ?

>
> we still have a problem with EXTRA_ENV_SETTINGS and ENV_TEXT. This patch
> helps enforce priority of ENV_TEXT.
>
> But overall, I agree that we need to move off from EXTRA_ENV to text..
> probably in stages, I guess.

OK good

>
> > >
> > > Which in turn generates:
> > > boot_targets=mmc0 mmc1
> > >
> > > And this probably works fine for most boards, However when the
> > > boot_targets need to be reversed, the preferred behavior would have been
> > > to define it in board.env file as:
> > > boot_targets=mmc1 mmc0
> > >
> > > By changing the order of the inclusion, we allow for the
> > > CONFIG_EXTRA_ENV_TEXT to have a higher priority in the definition.
> > >
> > > Signed-off-by: Nishanth Menon <nm at ti.com>
> > > ---
> > > Cc: Simon Glass <sjg at chromium.org>
> > >
> > > New patch
> > >
> > >  include/env_default.h | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/env_default.h b/include/env_default.h
> > > index b16c22d5a28c..714dfa9e845e 100644
> > > --- a/include/env_default.h
> > > +++ b/include/env_default.h
> > > @@ -112,12 +112,12 @@ const char default_environment[] = {
> > >  #ifdef CONFIG_MTDPARTS_DEFAULT
> > >         "mtdparts="     CONFIG_MTDPARTS_DEFAULT         "\0"
> > >  #endif
> > > +#ifdef CFG_EXTRA_ENV_SETTINGS
> > > +       CFG_EXTRA_ENV_SETTINGS
> > > +#endif
> > >  #ifdef CONFIG_EXTRA_ENV_TEXT
> > >         /* This is created in the Makefile */
> > >         CONFIG_EXTRA_ENV_TEXT
> > > -#endif
> > > -#ifdef CFG_EXTRA_ENV_SETTINGS
> > > -       CFG_EXTRA_ENV_SETTINGS
> > >  #endif
> >
> > and text environment
>
> Could you clarify what I am missing?

I'm not sure about that. I have no objection to this patch.

>
> [1] https://u-boot.readthedocs.io/en/latest/develop/bootstd.html
> --
> Regards,
> Nishanth Menon
> Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Regards,
Simon