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

[Nishanth Menon]

On 20:34-20230822, Simon Glass wrote:
> 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 used to do distro_boot as I recollect right.. This stuff is still in
the process of migration.. And I think this patch aids that ;)

> 
> >
> > 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
> 
> >V
> > > >
> > > > 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.


Is there anything I can do to improve this patch for your reviewed-by ?

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D