Re: [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation
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 > Reviewed-by: Simon Glass Applied to u-boot/master, thanks! -- Tom signature.asc Description: PGP signature
Re: [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation
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 --- 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() +*/ +
Re: [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation
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 > > --- > > > > 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; > >
Re: [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation
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
Re: [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation
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 --- 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
Re: [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation
On Sat, 10 Oct 2020 at 02:28, 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 > --- > > 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