| Message ID | 20230822184135.2328409-2-nm@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | board: ti: Add support for BeaglePlay | expand |
Hi Nishanth, On Tue, 22 Aug 2023 at 12:41, Nishanth Menon <nm@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. > > 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@ti.com> > --- > Cc: Simon Glass <sjg@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 > "\0" > #else /* CONFIG_USE_DEFAULT_ENV_FILE */ > -- > 2.40.0 > Regards, SImon
On 17:16-20230822, Simon Glass wrote: > Hi Nishanth, > > On Tue, 22 Aug 2023 at 12:41, Nishanth Menon <nm@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. 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. > > > > 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@ti.com> > > --- > > Cc: Simon Glass <sjg@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? [1] https://u-boot.readthedocs.io/en/latest/develop/bootstd.html
Hi Nishanth, On Tue, 22 Aug 2023 at 17:34, Nishanth Menon <nm@ti.com> wrote: > > On 17:16-20230822, Simon Glass wrote: > > Hi Nishanth, > > > > On Tue, 22 Aug 2023 at 12:41, Nishanth Menon <nm@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@ti.com> > > > --- > > > Cc: Simon Glass <sjg@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
On mar., août 22, 2023 at 13:41, Nishanth Menon <nm@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) > > 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@ti.com> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> > --- > Cc: Simon Glass <sjg@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 > "\0" > #else /* CONFIG_USE_DEFAULT_ENV_FILE */ > -- > 2.40.0
On 20:34-20230822, Simon Glass wrote: > Hi Nishanth, > > On Tue, 22 Aug 2023 at 17:34, Nishanth Menon <nm@ti.com> wrote: > > > > On 17:16-20230822, Simon Glass wrote: > > > Hi Nishanth, > > > > > > On Tue, 22 Aug 2023 at 12:41, Nishanth Menon <nm@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@ti.com> > > > > --- > > > > Cc: Simon Glass <sjg@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 ?
On Tue, Aug 22, 2023 at 01:41:28PM -0500, Nishanth Menon 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) > > 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@ti.com> > --- > Cc: Simon Glass <sjg@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 > "\0" > #else /* CONFIG_USE_DEFAULT_ENV_FILE */ The problem is that I believe we intend for CFG_EXTRA_ENV_SETTINGS to be where we can override things in a more final manner.
On 10:42-20230823, Tom Rini wrote: [...] > > 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 > > "\0" > > #else /* CONFIG_USE_DEFAULT_ENV_FILE */ > > The problem is that I believe we intend for CFG_EXTRA_ENV_SETTINGS to be > where we can override things in a more final manner. I am confused - if the intent is to move to text env, it should have priority over the extra_env settings.
On Wed, Aug 23, 2023 at 10:06:58AM -0500, Nishanth Menon wrote: > On 10:42-20230823, Tom Rini wrote: > [...] > > > > 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 > > > "\0" > > > #else /* CONFIG_USE_DEFAULT_ENV_FILE */ > > > > The problem is that I believe we intend for CFG_EXTRA_ENV_SETTINGS to be > > where we can override things in a more final manner. > > I am confused - if the intent is to move to text env, it should have > priority over the extra_env settings. If you have a text environment you really shouldn't have anything defined. But that restriction meant that almost nothing could be migrated (since we didn't have the boostd stuff far enough along), so I made it so we would at least end up merging the two.
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 "\0" #else /* CONFIG_USE_DEFAULT_ENV_FILE */
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) 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@ti.com> --- Cc: Simon Glass <sjg@chromium.org> New patch include/env_default.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)