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 michael@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.@@ -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?
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 ...
Thanks!
bye, Heiko