From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Tue, 3 Nov 2020 07:30:50 -0500 Subject: [PATCH] env: env_sf: don't set .init op if not needed In-Reply-To: <7c65c870-cb35-8d73-211f-b40cad88cb31@denx.de> References: <20201101133812.18701-1-michael@walle.cc> <08ffa0cfde14971e4cd9aa35d66123dc@walle.cc> <7c65c870-cb35-8d73-211f-b40cad88cb31@denx.de> Message-ID: <20201103123050.GP5340@bill-the-cat> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > > > > --- > > > > ? 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 -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 659 bytes Desc: not available URL: