Re: [PATCH 02/29] mmc: env: Unify the U_BOOT_ENV_LOCATION conditions
On Sun, Nov 19, 2023 at 07:49:32AM -0700, Simon Glass wrote:
> Hi Heinrich,
>
> On Wed, 15 Nov 2023 at 03:02, Heinrich Schuchardt wrote:
> >
> > On 11/12/23 01:08, Simon Glass wrote:
> > > The declaration of U_BOOT_ENV_LOCATION() uses a different #ifdef
> > > condition from the code it calls. Use the same condition to avoid a
> > > build warning if CONFIG_CMD_SAVEENV is disabled.
> > >
> > > Signed-off-by: Simon Glass
> > > ---
> > >
> > > env/mmc.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/env/mmc.c b/env/mmc.c
> > > index cb14bbb58f13..da84cddd74f0 100644
> > > --- a/env/mmc.c
> > > +++ b/env/mmc.c
> > > @@ -495,7 +495,7 @@ U_BOOT_ENV_LOCATION(mmc) = {
> > > .location = ENVL_MMC,
> > > ENV_NAME("MMC")
> > > .load = env_mmc_load,
> > > -#ifndef CONFIG_SPL_BUILD
> > > +#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_SPL_BUILD)
> >
> > According to README CONFIG_SPL_BUILD is not defined for TPL builds.
> >
> > I assume that we don't want to have below fields in TPL either. Please, use
> >
> > #if CONFIG_IS_ENABLED(CMD_SAVEENV)
>
> I missed this comment in the new version. But note that
> CONFIG_SPL_BUILD covers TPL (and others) as well.
>
> It is a bit confusing. Perhaps we should introduce CONFIG_XPL_BUILD to
> mean anything other than U-Boot proper?
We can see if something like that makes more sense as we further
separate out "library" functionality from "command" functionality. We
might swing back to needing to save environment changes from the
non-cmdline use cases all the same (assorted canary type environment
variables, etc) and be back to needing to tweak this differently. So
what's here is fine with me for today.
Reviewed-by: Tom Rini
... and just put that on the next iteration of the series.
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH 02/29] mmc: env: Unify the U_BOOT_ENV_LOCATION conditions
Hi Heinrich,
On Wed, 15 Nov 2023 at 03:02, Heinrich Schuchardt wrote:
>
> On 11/12/23 01:08, Simon Glass wrote:
> > The declaration of U_BOOT_ENV_LOCATION() uses a different #ifdef
> > condition from the code it calls. Use the same condition to avoid a
> > build warning if CONFIG_CMD_SAVEENV is disabled.
> >
> > Signed-off-by: Simon Glass
> > ---
> >
> > env/mmc.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/env/mmc.c b/env/mmc.c
> > index cb14bbb58f13..da84cddd74f0 100644
> > --- a/env/mmc.c
> > +++ b/env/mmc.c
> > @@ -495,7 +495,7 @@ U_BOOT_ENV_LOCATION(mmc) = {
> > .location = ENVL_MMC,
> > ENV_NAME("MMC")
> > .load = env_mmc_load,
> > -#ifndef CONFIG_SPL_BUILD
> > +#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_SPL_BUILD)
>
> According to README CONFIG_SPL_BUILD is not defined for TPL builds.
>
> I assume that we don't want to have below fields in TPL either. Please, use
>
> #if CONFIG_IS_ENABLED(CMD_SAVEENV)
I missed this comment in the new version. But note that
CONFIG_SPL_BUILD covers TPL (and others) as well.
It is a bit confusing. Perhaps we should introduce CONFIG_XPL_BUILD to
mean anything other than U-Boot proper?
>
> Best regards
>
> Heinrich
>
> > .save = env_save_ptr(env_mmc_save),
> > .erase = ENV_ERASE_PTR(env_mmc_erase)
> > #endif
>
Regards,
Simon
Re: [PATCH 02/29] mmc: env: Unify the U_BOOT_ENV_LOCATION conditions
On 11/12/23 01:08, Simon Glass wrote:
The declaration of U_BOOT_ENV_LOCATION() uses a different #ifdef
condition from the code it calls. Use the same condition to avoid a
build warning if CONFIG_CMD_SAVEENV is disabled.
Signed-off-by: Simon Glass
---
env/mmc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/env/mmc.c b/env/mmc.c
index cb14bbb58f13..da84cddd74f0 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -495,7 +495,7 @@ U_BOOT_ENV_LOCATION(mmc) = {
.location = ENVL_MMC,
ENV_NAME("MMC")
.load = env_mmc_load,
-#ifndef CONFIG_SPL_BUILD
+#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_SPL_BUILD)
According to README CONFIG_SPL_BUILD is not defined for TPL builds.
I assume that we don't want to have below fields in TPL either. Please, use
#if CONFIG_IS_ENABLED(CMD_SAVEENV)
Best regards
Heinrich
.save = env_save_ptr(env_mmc_save),
.erase = ENV_ERASE_PTR(env_mmc_erase)
#endif
[PATCH 02/29] mmc: env: Unify the U_BOOT_ENV_LOCATION conditions
The declaration of U_BOOT_ENV_LOCATION() uses a different #ifdef
condition from the code it calls. Use the same condition to avoid a
build warning if CONFIG_CMD_SAVEENV is disabled.
Signed-off-by: Simon Glass
---
env/mmc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/env/mmc.c b/env/mmc.c
index cb14bbb58f13..da84cddd74f0 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -495,7 +495,7 @@ U_BOOT_ENV_LOCATION(mmc) = {
.location = ENVL_MMC,
ENV_NAME("MMC")
.load = env_mmc_load,
-#ifndef CONFIG_SPL_BUILD
+#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_SPL_BUILD)
.save = env_save_ptr(env_mmc_save),
.erase = ENV_ERASE_PTR(env_mmc_erase)
#endif
--
2.42.0.869.gea05f2083d-goog
