On Tue, Nov 03, 2020 at 05:40:16AM +0100, Heiko Schocher wrote:
> Hello Michael,
>
> Am 02.11.2020 um 21:15 schrieb Michael Walle:
> > Am 2020-11-02 08:00, schrieb Heiko Schocher:
> > > Hello Michael,
> > >
> > > Am 01.11.2020 um 14:38 schrieb Michael Walle:
> > > > Commit 92765f45bb95 ("env: Access Environment in SPI flashes before
> > > > relocation") at least breaks the Kontron sl28 board. I guess it also
> > > > breaks others which use a (late) SPI environment.
> > > >
> > > > Unfortunately, we cannot set the .init op and fall back to the same
> > > > behavior as it would be unset. Thus guard the .init op by #if's.
> > > >
> > > > Fixes: 92765f45bb95 ("env: Access Environment in SPI flashes before
> > > > relocation")
> > > > Signed-off-by: Michael Walle <mich...@walle.cc>
> > > > ---
> > > > env/sf.c | 13 ++++++++++---
> > > > 1 file changed, 10 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/env/sf.c b/env/sf.c
> > > > index 2eb2de1a4e..18d44a7ddc 100644
> > > > --- a/env/sf.c
> > > > +++ b/env/sf.c
> > > > @@ -385,7 +385,7 @@ out:
> > > > }
> > > > #endif
> > > > -static int env_sf_init(void)
> > > > +static int __maybe_unused env_sf_init(void)
> > > > {
> > > > #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
> > > > return env_sf_init_addr();
> > > > @@ -393,8 +393,13 @@ static int env_sf_init(void)
> > > > return env_sf_init_early();
> > > > #endif
> > > > /*
> > > > - * we must return with 0 if there is nothing done,
> > > > - * else env_set_inited() get not called in env_init()
> > > > + * We shouldn't end up here. Unfortunately, there is no
> > > > + * way to return a value which yields the same behavior
> > > > + * as if the .init op wouldn't be set at all. See
> > > > + * env_init(); env_set_inited() is only called if we
> > > > + * return 0, but the default environment is only loaded
> > > > + * if -ENOENT is returned. Therefore, we need the ugly
> > > > + * ifdeferry for the .init op.
> > > > */
> > > > return 0;
> > > > }
> > > > @@ -404,5 +409,7 @@ U_BOOT_ENV_LOCATION(sf) = {
> > > > ENV_NAME("SPIFlash")
> > > > .load = env_sf_load,
> > > > .save = CONFIG_IS_ENABLED(SAVEENV) ?
> > > > ENV_SAVE_PTR(env_sf_save) : NULL,
> > > > +#if (defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)) ||
> > > > defined(CONFIG_ENV_SPI_EARLY)
> > > > .init = env_sf_init,
> > > > +#endif
> > > > };
> > > >
> > >
> > > Ok, tested this patch on an imx6 based board with SPI NOR and it works.
> > >
> > > But.... there is a problem with environment in spi nor and ENV_APPEND
> > > enabled, with current implementation (also before my patches applied):
> > >
> > > I enabled now ENV_APPEND on this board and
> > >
> > > CONFIG_ENV_IS_NOWHERE
> > > CONFIG_ENV_IS_IN_SPI_FLASH
> > >
> > > and the Environment from SPI NOR never loaded as gd->env_valid is
> > > always ENV_INVALID and env_load() never called from env_relocate().
> > >
> > > What do you think about following patch:
> > >
> > > diff --git a/env/sf.c b/env/sf.c
> > > index 2eb2de1a4e..7f3491b458 100644
> > > --- a/env/sf.c
> > > +++ b/env/sf.c
> > > @@ -393,9 +393,13 @@ static int env_sf_init(void)
> > > return env_sf_init_early();
> > > #endif
> > > /*
> > > - * we must return with 0 if there is nothing done,
> > > - * else env_set_inited() get not called in env_init()
> > > + * We must return with 0 if there is nothing done,
> > > + * to get inited bit set in env_init().
> > > + * We need to set env_valid to ENV_VALID, so later
> > > + * env_load() loads the Environment from SPI NOR.
> > > */
> > > + gd->env_addr = (ulong)&default_environment[0];
> > > + gd->env_valid = ENV_VALID;
> > > return 0;
> > > }
> > >
> > > Can you try it?
> >
> > This works for me...
> >
> > > Another option would be to reutrn -ENOENT and set init bit also
> > > when a init function returns -ENOENT:
> > >
> > > diff --git a/env/env.c b/env/env.c
> > > index 42c7d8155e..37b4b54cb7 100644
> > > --- a/env/env.c
> > > +++ b/env/env.c
> > > @@ -329,6 +329,8 @@ int env_init(void)
> > > for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio));
> > > prio++) {
> > > if (!drv->init || !(ret = drv->init()))
> > > env_set_inited(drv->location);
> > > + if (ret == -ENOENT)
> > > + env_set_inited(drv->location);
> > >
> > > debug("%s: Environment %s init done (ret=%d)\n", __func__,
> > > drv->name, ret);
> > > diff --git a/env/sf.c b/env/sf.c
> > > index 2eb2de1a4e..66279fb4f4 100644
> > > --- a/env/sf.c
> > > +++ b/env/sf.c
> > > @@ -396,7 +396,7 @@ static int env_sf_init(void)
> > > * we must return with 0 if there is nothing done,
> > > * else env_set_inited() get not called in env_init()
> > > */
> > > - return 0;
> > > + return -ENOENT;
> > > }
> > >
> > > But this may has impact on other environment drivers ... but may is
> > > the cleaner approach as env_init() later sets the default environment
> > > if ret is -ENOENT ...
> >
> > .. and also this.
> >
> > So we have four solutions
> > (1) revert the series
> > (2) apply my patch
> > (3) use the first solution from Heiko
> > (4) use the second solution from Heiko
> >
> > I'm fine with all four. If it will be (3) or (4) will you prepare a
> > patch, Heiko?
>
> I tend to implement solution [4] ... I can send a patch...
>
> Simon? Tom? Any suggestions?Lets go with #4 above then please, thanks! -- Tom
signature.asc
Description: PGP signature
