| Message ID | 20201010082806.2260318-3-hs@denx.de |
|---|---|
| State | Accepted |
| Commit | 92765f45bb950c24b8997752ab94f35c1fb4425f |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | env: Access Environment in SPI flashes before relocation | expand |
On Sat, 10 Oct 2020 at 02:28, Heiko Schocher <hs@denx.de> wrote: > > Enable the new Kconfig option ENV_SPI_EARLY if you want > to use Environment in SPI flash before relocation. > Call env_init() and than you can use env_get_f() for > accessing Environment variables. > > Signed-off-by: Heiko Schocher <hs@denx.de> > --- > > Changes in v4: > - rebased to current master 5dcf7cc590 > > Changes in v3: > - env_sf_init_early() always return 0 now. If we do not return > 0 in this function, env_set_inited() never get called, > which has the consequence that env_load/save/erase never > work, because they check if the init bit is set. > - add comment from Simon Glass > - add missing function comments > - use if(IS_ENABLED...) > - drop extra brackets > - let env_sf_init() decide, which function to call > add comment that it is necessary to return env_sf_init() > with 0. > > env/Kconfig | 8 +++++ > env/sf.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++-- > 2 files changed, 105 insertions(+), 3 deletions(-) Reviewed-by: Simon Glass <sjg@chromium.org>
On Sat, Oct 10, 2020 at 10:28:05AM +0200, Heiko Schocher wrote: > Enable the new Kconfig option ENV_SPI_EARLY if you want > to use Environment in SPI flash before relocation. > Call env_init() and than you can use env_get_f() for > accessing Environment variables. > > Signed-off-by: Heiko Schocher <hs@denx.de> > Reviewed-by: Simon Glass <sjg@chromium.org> Applied to u-boot/master, thanks!
Hi Heiko, Hi Tom, Am 2020-10-10 10:28, schrieb Heiko Schocher: > Enable the new Kconfig option ENV_SPI_EARLY if you want > to use Environment in SPI flash before relocation. > Call env_init() and than you can use env_get_f() for > accessing Environment variables. > > Signed-off-by: Heiko Schocher <hs@denx.de> > --- > > Changes in v4: > - rebased to current master 5dcf7cc590 > > Changes in v3: > - env_sf_init_early() always return 0 now. If we do not return > 0 in this function, env_set_inited() never get called, > which has the consequence that env_load/save/erase never > work, because they check if the init bit is set. > - add comment from Simon Glass > - add missing function comments > - use if(IS_ENABLED...) > - drop extra brackets > - let env_sf_init() decide, which function to call > add comment that it is necessary to return env_sf_init() > with 0. > > env/Kconfig | 8 +++++ > env/sf.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++-- > 2 files changed, 105 insertions(+), 3 deletions(-) > > diff --git a/env/Kconfig b/env/Kconfig > index c6ba08878d..393b191a30 100644 > --- a/env/Kconfig > +++ b/env/Kconfig > @@ -376,6 +376,14 @@ config ENV_SPI_MODE > Value of the SPI work mode for environment. > See include/spi.h for value. > > +config ENV_SPI_EARLY > + bool "Access Environment in SPI flashes before relocation" > + depends on ENV_IS_IN_SPI_FLASH > + help > + Enable this if you want to use Environment in SPI flash > + before relocation. Call env_init() and than you can use > + env_get_f() for accessing Environment variables. > + > config ENV_IS_IN_UBI > bool "Environment in a UBI volume" > depends on !CHAIN_OF_TRUST > diff --git a/env/sf.c b/env/sf.c > index 937778aa37..2eb2de1a4e 100644 > --- a/env/sf.c > +++ b/env/sf.c > @@ -287,7 +287,10 @@ __weak void *env_sf_get_env_addr(void) > #endif > > #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0) > -static int env_sf_init(void) > +/* > + * check if Environment on CONFIG_ENV_ADDR is valid. > + */ > +static int env_sf_init_addr(void) > { > env_t *env_ptr = (env_t *)env_sf_get_env_addr(); > > @@ -303,12 +306,103 @@ static int env_sf_init(void) > } > #endif > > +#if defined(CONFIG_ENV_SPI_EARLY) > +/* > + * early load environment from SPI flash (before relocation) > + * and check if it is valid. > + */ > +static int env_sf_init_early(void) > +{ > + int ret; > + int read1_fail; > + int read2_fail; > + int crc1_ok; > + env_t *tmp_env2 = NULL; > + env_t *tmp_env1; > + > + /* > + * if malloc is not ready yet, we cannot use > + * this part yet. > + */ > + if (!gd->malloc_limit) > + return -ENOENT; > + > + tmp_env1 = (env_t *)memalign(ARCH_DMA_MINALIGN, > + CONFIG_ENV_SIZE); > + if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) > + tmp_env2 = (env_t *)memalign(ARCH_DMA_MINALIGN, > + CONFIG_ENV_SIZE); > + > + if (!tmp_env1 || !tmp_env2) > + goto out; > + > + ret = setup_flash_device(); > + if (ret) > + goto out; > + > + read1_fail = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, > + CONFIG_ENV_SIZE, tmp_env1); > + > + if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) { > + read2_fail = spi_flash_read(env_flash, > + CONFIG_ENV_OFFSET_REDUND, > + CONFIG_ENV_SIZE, tmp_env2); > + ret = env_check_redund((char *)tmp_env1, read1_fail, > + (char *)tmp_env2, read2_fail); > + > + if (ret == -EIO || ret == -ENOMSG) > + goto err_read; > + > + if (gd->env_valid == ENV_VALID) > + gd->env_addr = (unsigned long)&tmp_env1->data; > + else > + gd->env_addr = (unsigned long)&tmp_env2->data; > + } else { > + if (read1_fail) > + goto err_read; > + > + crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) == > + tmp_env1->crc; > + if (!crc1_ok) > + goto err_read; > + > + /* if valid -> this is our env */ > + gd->env_valid = ENV_VALID; > + gd->env_addr = (unsigned long)&tmp_env1->data; > + } > + > + return 0; > +err_read: > + spi_flash_free(env_flash); > + env_flash = NULL; > + free(tmp_env1); > + if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) > + free(tmp_env2); > +out: > + /* env is not valid. always return 0 */ > + gd->env_valid = ENV_INVALID; > + return 0; > +} > +#endif > + > +static int env_sf_init(void) > +{ > +#if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0) > + return env_sf_init_addr(); > +#elif defined(CONFIG_ENV_SPI_EARLY) > + 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() > + */ > + return 0; > +} > + > U_BOOT_ENV_LOCATION(sf) = { > .location = ENVL_SPI_FLASH, > 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) > .init = env_sf_init, > -#endif This will break my board (the new kontron sl28). Before the change, the environment is loaded from SPI, after this patch the environment won't be loaded anymore. If I delete the .init callback, the behavior is the same as before. The problem here is that returning 0 in env_sf_init() is not enough because if the .init callback is not set, env_init() will have ret defaulting to -ENOENT and in this case will load the default environment and setting env_valid to ENV_VALID. I didn't follow the whole call chain then. But this will then eventually lead to loading the actual environment in a later stage. -michael
On Sat, Oct 31, 2020 at 12:51:27AM +0100, Michael Walle wrote: > Hi Heiko, Hi Tom, > > Am 2020-10-10 10:28, schrieb Heiko Schocher: > > Enable the new Kconfig option ENV_SPI_EARLY if you want > > to use Environment in SPI flash before relocation. > > Call env_init() and than you can use env_get_f() for > > accessing Environment variables. > > > > Signed-off-by: Heiko Schocher <hs@denx.de> > > --- > > > > Changes in v4: > > - rebased to current master 5dcf7cc590 > > > > Changes in v3: > > - env_sf_init_early() always return 0 now. If we do not return > > 0 in this function, env_set_inited() never get called, > > which has the consequence that env_load/save/erase never > > work, because they check if the init bit is set. > > - add comment from Simon Glass > > - add missing function comments > > - use if(IS_ENABLED...) > > - drop extra brackets > > - let env_sf_init() decide, which function to call > > add comment that it is necessary to return env_sf_init() > > with 0. > > > > env/Kconfig | 8 +++++ > > env/sf.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++-- > > 2 files changed, 105 insertions(+), 3 deletions(-) > > > > diff --git a/env/Kconfig b/env/Kconfig > > index c6ba08878d..393b191a30 100644 > > --- a/env/Kconfig > > +++ b/env/Kconfig > > @@ -376,6 +376,14 @@ config ENV_SPI_MODE > > Value of the SPI work mode for environment. > > See include/spi.h for value. > > > > +config ENV_SPI_EARLY > > + bool "Access Environment in SPI flashes before relocation" > > + depends on ENV_IS_IN_SPI_FLASH > > + help > > + Enable this if you want to use Environment in SPI flash > > + before relocation. Call env_init() and than you can use > > + env_get_f() for accessing Environment variables. > > + > > config ENV_IS_IN_UBI > > bool "Environment in a UBI volume" > > depends on !CHAIN_OF_TRUST > > diff --git a/env/sf.c b/env/sf.c > > index 937778aa37..2eb2de1a4e 100644 > > --- a/env/sf.c > > +++ b/env/sf.c > > @@ -287,7 +287,10 @@ __weak void *env_sf_get_env_addr(void) > > #endif > > > > #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0) > > -static int env_sf_init(void) > > +/* > > + * check if Environment on CONFIG_ENV_ADDR is valid. > > + */ > > +static int env_sf_init_addr(void) > > { > > env_t *env_ptr = (env_t *)env_sf_get_env_addr(); > > > > @@ -303,12 +306,103 @@ static int env_sf_init(void) > > } > > #endif > > > > +#if defined(CONFIG_ENV_SPI_EARLY) > > +/* > > + * early load environment from SPI flash (before relocation) > > + * and check if it is valid. > > + */ > > +static int env_sf_init_early(void) > > +{ > > + int ret; > > + int read1_fail; > > + int read2_fail; > > + int crc1_ok; > > + env_t *tmp_env2 = NULL; > > + env_t *tmp_env1; > > + > > + /* > > + * if malloc is not ready yet, we cannot use > > + * this part yet. > > + */ > > + if (!gd->malloc_limit) > > + return -ENOENT; > > + > > + tmp_env1 = (env_t *)memalign(ARCH_DMA_MINALIGN, > > + CONFIG_ENV_SIZE); > > + if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) > > + tmp_env2 = (env_t *)memalign(ARCH_DMA_MINALIGN, > > + CONFIG_ENV_SIZE); > > + > > + if (!tmp_env1 || !tmp_env2) > > + goto out; > > + > > + ret = setup_flash_device(); > > + if (ret) > > + goto out; > > + > > + read1_fail = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, > > + CONFIG_ENV_SIZE, tmp_env1); > > + > > + if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) { > > + read2_fail = spi_flash_read(env_flash, > > + CONFIG_ENV_OFFSET_REDUND, > > + CONFIG_ENV_SIZE, tmp_env2); > > + ret = env_check_redund((char *)tmp_env1, read1_fail, > > + (char *)tmp_env2, read2_fail); > > + > > + if (ret == -EIO || ret == -ENOMSG) > > + goto err_read; > > + > > + if (gd->env_valid == ENV_VALID) > > + gd->env_addr = (unsigned long)&tmp_env1->data; > > + else > > + gd->env_addr = (unsigned long)&tmp_env2->data; > > + } else { > > + if (read1_fail) > > + goto err_read; > > + > > + crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) == > > + tmp_env1->crc; > > + if (!crc1_ok) > > + goto err_read; > > + > > + /* if valid -> this is our env */ > > + gd->env_valid = ENV_VALID; > > + gd->env_addr = (unsigned long)&tmp_env1->data; > > + } > > + > > + return 0; > > +err_read: > > + spi_flash_free(env_flash); > > + env_flash = NULL; > > + free(tmp_env1); > > + if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) > > + free(tmp_env2); > > +out: > > + /* env is not valid. always return 0 */ > > + gd->env_valid = ENV_INVALID; > > + return 0; > > +} > > +#endif > > + > > +static int env_sf_init(void) > > +{ > > +#if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0) > > + return env_sf_init_addr(); > > +#elif defined(CONFIG_ENV_SPI_EARLY) > > + 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() > > + */ > > + return 0; > > +} > > + > > U_BOOT_ENV_LOCATION(sf) = { > > .location = ENVL_SPI_FLASH, > > 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) > > .init = env_sf_init, > > -#endif > > This will break my board (the new kontron sl28). Before the > change, the environment is loaded from SPI, after this patch > the environment won't be loaded anymore. If I delete the > .init callback, the behavior is the same as before. > > The problem here is that returning 0 in env_sf_init() is not > enough because if the .init callback is not set, env_init() will > have ret defaulting to -ENOENT and in this case will load the > default environment and setting env_valid to ENV_VALID. I didn't > follow the whole call chain then. But this will then eventually > lead to loading the actual environment in a later stage. Ugh. The games we play in the env area really just need to be rewritten. So at this point I've merged the series, should I revert or do you see an easy fix for your platform? Thanks!
Hi, Am 2020-10-31 01:07, schrieb Tom Rini: [..] >> > +static int env_sf_init(void) >> > +{ >> > +#if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0) >> > + return env_sf_init_addr(); >> > +#elif defined(CONFIG_ENV_SPI_EARLY) >> > + 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() >> > + */ >> > + return 0; >> > +} >> > + >> > U_BOOT_ENV_LOCATION(sf) = { >> > .location = ENVL_SPI_FLASH, >> > 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) >> > .init = env_sf_init, >> > -#endif >> >> This will break my board (the new kontron sl28). Before the >> change, the environment is loaded from SPI, after this patch >> the environment won't be loaded anymore. If I delete the >> .init callback, the behavior is the same as before. >> >> The problem here is that returning 0 in env_sf_init() is not >> enough because if the .init callback is not set, env_init() will >> have ret defaulting to -ENOENT and in this case will load the >> default environment and setting env_valid to ENV_VALID. I didn't >> follow the whole call chain then. But this will then eventually >> lead to loading the actual environment in a later stage. > > Ugh. The games we play in the env area really just need to be > rewritten. So at this point I've merged the series, should I revert or > do you see an easy fix for your platform? Thanks! Mh, my board cannot be the only one with a late environment from SPI flash, can it? Returning 0 will call env_set_inited() and returning -ENOENT will call the second part. I can't tell why it was done that way. So I don't have a simple fix. But I guess the following ugly ifdeffery could do it: #if (defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)) || defined(CONFIG_ENV_SPI_EARLY) .init = env_sf_init, #endif I can try that tomorrow. Also the comment about the return value should be removed (?). -michael
Hello Tom, Michael, Am 31.10.2020 um 01:07 schrieb Tom Rini: > On Sat, Oct 31, 2020 at 12:51:27AM +0100, Michael Walle wrote: >> Hi Heiko, Hi Tom, >> >> Am 2020-10-10 10:28, schrieb Heiko Schocher: >>> Enable the new Kconfig option ENV_SPI_EARLY if you want >>> to use Environment in SPI flash before relocation. >>> Call env_init() and than you can use env_get_f() for >>> accessing Environment variables. >>> >>> Signed-off-by: Heiko Schocher <hs@denx.de> >>> --- >>> >>> Changes in v4: >>> - rebased to current master 5dcf7cc590 >>> >>> Changes in v3: >>> - env_sf_init_early() always return 0 now. If we do not return >>> 0 in this function, env_set_inited() never get called, >>> which has the consequence that env_load/save/erase never >>> work, because they check if the init bit is set. >>> - add comment from Simon Glass >>> - add missing function comments >>> - use if(IS_ENABLED...) >>> - drop extra brackets >>> - let env_sf_init() decide, which function to call >>> add comment that it is necessary to return env_sf_init() >>> with 0. >>> >>> env/Kconfig | 8 +++++ >>> env/sf.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++-- >>> 2 files changed, 105 insertions(+), 3 deletions(-) >>> >>> diff --git a/env/Kconfig b/env/Kconfig >>> index c6ba08878d..393b191a30 100644 >>> --- a/env/Kconfig >>> +++ b/env/Kconfig >>> @@ -376,6 +376,14 @@ config ENV_SPI_MODE >>> Value of the SPI work mode for environment. >>> See include/spi.h for value. >>> >>> +config ENV_SPI_EARLY >>> + bool "Access Environment in SPI flashes before relocation" >>> + depends on ENV_IS_IN_SPI_FLASH >>> + help >>> + Enable this if you want to use Environment in SPI flash >>> + before relocation. Call env_init() and than you can use >>> + env_get_f() for accessing Environment variables. >>> + >>> config ENV_IS_IN_UBI >>> bool "Environment in a UBI volume" >>> depends on !CHAIN_OF_TRUST >>> diff --git a/env/sf.c b/env/sf.c >>> index 937778aa37..2eb2de1a4e 100644 >>> --- a/env/sf.c >>> +++ b/env/sf.c >>> @@ -287,7 +287,10 @@ __weak void *env_sf_get_env_addr(void) >>> #endif >>> >>> #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0) >>> -static int env_sf_init(void) >>> +/* >>> + * check if Environment on CONFIG_ENV_ADDR is valid. >>> + */ >>> +static int env_sf_init_addr(void) >>> { >>> env_t *env_ptr = (env_t *)env_sf_get_env_addr(); >>> >>> @@ -303,12 +306,103 @@ static int env_sf_init(void) >>> } >>> #endif >>> >>> +#if defined(CONFIG_ENV_SPI_EARLY) >>> +/* >>> + * early load environment from SPI flash (before relocation) >>> + * and check if it is valid. >>> + */ >>> +static int env_sf_init_early(void) >>> +{ >>> + int ret; >>> + int read1_fail; >>> + int read2_fail; >>> + int crc1_ok; >>> + env_t *tmp_env2 = NULL; >>> + env_t *tmp_env1; >>> + >>> + /* >>> + * if malloc is not ready yet, we cannot use >>> + * this part yet. >>> + */ >>> + if (!gd->malloc_limit) >>> + return -ENOENT; >>> + >>> + tmp_env1 = (env_t *)memalign(ARCH_DMA_MINALIGN, >>> + CONFIG_ENV_SIZE); >>> + if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) >>> + tmp_env2 = (env_t *)memalign(ARCH_DMA_MINALIGN, >>> + CONFIG_ENV_SIZE); >>> + >>> + if (!tmp_env1 || !tmp_env2) >>> + goto out; >>> + >>> + ret = setup_flash_device(); >>> + if (ret) >>> + goto out; >>> + >>> + read1_fail = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, >>> + CONFIG_ENV_SIZE, tmp_env1); >>> + >>> + if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) { >>> + read2_fail = spi_flash_read(env_flash, >>> + CONFIG_ENV_OFFSET_REDUND, >>> + CONFIG_ENV_SIZE, tmp_env2); >>> + ret = env_check_redund((char *)tmp_env1, read1_fail, >>> + (char *)tmp_env2, read2_fail); >>> + >>> + if (ret == -EIO || ret == -ENOMSG) >>> + goto err_read; >>> + >>> + if (gd->env_valid == ENV_VALID) >>> + gd->env_addr = (unsigned long)&tmp_env1->data; >>> + else >>> + gd->env_addr = (unsigned long)&tmp_env2->data; >>> + } else { >>> + if (read1_fail) >>> + goto err_read; >>> + >>> + crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) == >>> + tmp_env1->crc; >>> + if (!crc1_ok) >>> + goto err_read; >>> + >>> + /* if valid -> this is our env */ >>> + gd->env_valid = ENV_VALID; >>> + gd->env_addr = (unsigned long)&tmp_env1->data; >>> + } >>> + >>> + return 0; >>> +err_read: >>> + spi_flash_free(env_flash); >>> + env_flash = NULL; >>> + free(tmp_env1); >>> + if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) >>> + free(tmp_env2); >>> +out: >>> + /* env is not valid. always return 0 */ >>> + gd->env_valid = ENV_INVALID; >>> + return 0; >>> +} >>> +#endif >>> + >>> +static int env_sf_init(void) >>> +{ >>> +#if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0) >>> + return env_sf_init_addr(); >>> +#elif defined(CONFIG_ENV_SPI_EARLY) >>> + 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() >>> + */ >>> + return 0; >>> +} >>> + >>> U_BOOT_ENV_LOCATION(sf) = { >>> .location = ENVL_SPI_FLASH, >>> 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) >>> .init = env_sf_init, >>> -#endif >> >> This will break my board (the new kontron sl28). Before the >> change, the environment is loaded from SPI, after this patch >> the environment won't be loaded anymore. If I delete the >> .init callback, the behavior is the same as before. >> >> The problem here is that returning 0 in env_sf_init() is not >> enough because if the .init callback is not set, env_init() will >> have ret defaulting to -ENOENT and in this case will load the >> default environment and setting env_valid to ENV_VALID. I didn't >> follow the whole call chain then. But this will then eventually >> lead to loading the actual environment in a later stage. > > Ugh. The games we play in the env area really just need to be > rewritten. So at this point I've merged the series, should I revert or > do you see an easy fix for your platform? Thanks! > Autsch ... sorry ... Yes, env code is really ugly ... env/env.c: 323 int env_init(void) 324 { 325 struct env_driver *drv; 326 int ret = -ENOENT; 327 int prio; 328 329 for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) { 330 if (!drv->init || !(ret = drv->init())) 331 env_set_inited(drv->location); 332 333 debug("%s: Environment %s init done (ret=%d)\n", __func__, 334 drv->name, ret); 335 } 336 337 if (!prio) 338 return -ENODEV; 339 340 if (ret == -ENOENT) { 341 gd->env_addr = (ulong)&default_environment[0]; 342 gd->env_valid = ENV_VALID; 343 344 return 0; 345 } 346 347 return ret; So if now the init function returns 0, env_set_inited() sets the init bit, but gd->env_valid = ENV_VALID; never set ... which leads in the error Michael see... as in env/common.c 230 void env_relocate(void) [...] 237 if (gd->env_valid == ENV_INVALID) { 238 #if defined(CONFIG_ENV_IS_NOWHERE) || defined(CONFIG_SPL_BUILD) 239 /* Environment not changable */ 240 env_set_default(NULL, 0); 241 #else 242 bootstage_error(BOOTSTAGE_ID_NET_CHECKSUM); 243 env_set_default("bad CRC", 0); 244 #endif 245 } else { 246 env_load(); 247 } env_load() never called ... on the other hand in env_load() 181 int env_load(void) 182 { 183 struct env_driver *drv; 184 int best_prio = -1; 185 int prio; 186 187 for (prio = 0; (drv = env_driver_lookup(ENVOP_LOAD, prio)); prio++) { 188 int ret; 189 190 if (!env_has_inited(drv->location)) 191 continue; if the init bit is not set, it never loads from storage device here. :-( So I tend to say, we must prevent registering ".init" with some ifdeffery ... as I had it in v1 of this series ... :-( bye, Heiko
diff --git a/env/Kconfig b/env/Kconfig index c6ba08878d..393b191a30 100644 --- a/env/Kconfig +++ b/env/Kconfig @@ -376,6 +376,14 @@ config ENV_SPI_MODE Value of the SPI work mode for environment. See include/spi.h for value. +config ENV_SPI_EARLY + bool "Access Environment in SPI flashes before relocation" + depends on ENV_IS_IN_SPI_FLASH + help + Enable this if you want to use Environment in SPI flash + before relocation. Call env_init() and than you can use + env_get_f() for accessing Environment variables. + config ENV_IS_IN_UBI bool "Environment in a UBI volume" depends on !CHAIN_OF_TRUST diff --git a/env/sf.c b/env/sf.c index 937778aa37..2eb2de1a4e 100644 --- a/env/sf.c +++ b/env/sf.c @@ -287,7 +287,10 @@ __weak void *env_sf_get_env_addr(void) #endif #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0) -static int env_sf_init(void) +/* + * check if Environment on CONFIG_ENV_ADDR is valid. + */ +static int env_sf_init_addr(void) { env_t *env_ptr = (env_t *)env_sf_get_env_addr(); @@ -303,12 +306,103 @@ static int env_sf_init(void) } #endif +#if defined(CONFIG_ENV_SPI_EARLY) +/* + * early load environment from SPI flash (before relocation) + * and check if it is valid. + */ +static int env_sf_init_early(void) +{ + int ret; + int read1_fail; + int read2_fail; + int crc1_ok; + env_t *tmp_env2 = NULL; + env_t *tmp_env1; + + /* + * if malloc is not ready yet, we cannot use + * this part yet. + */ + if (!gd->malloc_limit) + return -ENOENT; + + tmp_env1 = (env_t *)memalign(ARCH_DMA_MINALIGN, + CONFIG_ENV_SIZE); + if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) + tmp_env2 = (env_t *)memalign(ARCH_DMA_MINALIGN, + CONFIG_ENV_SIZE); + + if (!tmp_env1 || !tmp_env2) + goto out; + + ret = setup_flash_device(); + if (ret) + goto out; + + read1_fail = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, + CONFIG_ENV_SIZE, tmp_env1); + + if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) { + read2_fail = spi_flash_read(env_flash, + CONFIG_ENV_OFFSET_REDUND, + CONFIG_ENV_SIZE, tmp_env2); + ret = env_check_redund((char *)tmp_env1, read1_fail, + (char *)tmp_env2, read2_fail); + + if (ret == -EIO || ret == -ENOMSG) + goto err_read; + + if (gd->env_valid == ENV_VALID) + gd->env_addr = (unsigned long)&tmp_env1->data; + else + gd->env_addr = (unsigned long)&tmp_env2->data; + } else { + if (read1_fail) + goto err_read; + + crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) == + tmp_env1->crc; + if (!crc1_ok) + goto err_read; + + /* if valid -> this is our env */ + gd->env_valid = ENV_VALID; + gd->env_addr = (unsigned long)&tmp_env1->data; + } + + return 0; +err_read: + spi_flash_free(env_flash); + env_flash = NULL; + free(tmp_env1); + if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) + free(tmp_env2); +out: + /* env is not valid. always return 0 */ + gd->env_valid = ENV_INVALID; + return 0; +} +#endif + +static int env_sf_init(void) +{ +#if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0) + return env_sf_init_addr(); +#elif defined(CONFIG_ENV_SPI_EARLY) + 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() + */ + return 0; +} + U_BOOT_ENV_LOCATION(sf) = { .location = ENVL_SPI_FLASH, 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) .init = env_sf_init, -#endif };
Enable the new Kconfig option ENV_SPI_EARLY if you want to use Environment in SPI flash before relocation. Call env_init() and than you can use env_get_f() for accessing Environment variables. Signed-off-by: Heiko Schocher <hs@denx.de> --- Changes in v4: - rebased to current master 5dcf7cc590 Changes in v3: - env_sf_init_early() always return 0 now. If we do not return 0 in this function, env_set_inited() never get called, which has the consequence that env_load/save/erase never work, because they check if the init bit is set. - add comment from Simon Glass - add missing function comments - use if(IS_ENABLED...) - drop extra brackets - let env_sf_init() decide, which function to call add comment that it is necessary to return env_sf_init() with 0. env/Kconfig | 8 +++++ env/sf.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 105 insertions(+), 3 deletions(-)