[PATCH v4 0/3] env: Access Environment in SPI flashes before relocation
Add the possibility to access Environment stored in SPI NOR Flash before relocation. This is used in DM/DTS rework for the aristainetos boards. There mutliple DTS are packed into a FIT image and through a variable in the Environment the correct DTS will be selected. For this we need very early access to the Environment. Also may this is good for selecting the console baudrate again through Environment which I think currently does not work for any board based on DM/DTS and with Environment in SPI NOR flash.
Add this to aristainetos board, as aristainetos DM board updates are now in mainline.
travis build: https://travis-ci.org/github/hsdenx/u-boot-test/builds/733559949
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. - new in v3 as aristainetos board DM changes are in mainline now
Changes in v2: - patch "env: split env_import_redund into 2 functions" is new in version 2. Idea is to not duplicate too much code as Simon suggested.
Heiko Schocher (3): env: split env_import_redund() into 2 functions env: Access Environment in SPI flashes before relocation imx6: enable early spi environment access on aristainetos boards
configs/aristainetos2_defconfig | 1 + configs/aristainetos2b_defconfig | 1 + configs/aristainetos2bcsl_defconfig | 1 + configs/aristainetos2c_defconfig | 1 + env/Kconfig | 8 +++ env/common.c | 42 +++++++++--- env/sf.c | 100 +++++++++++++++++++++++++++- include/env.h | 18 +++++ 8 files changed, 159 insertions(+), 13 deletions(-)
split from env_import_redund() the part which checks which Environment is valid into a separate function called env_check_redund() and call it from env_import_redund().
So env_check_redund() can be used from places which also need to do this checks.
Signed-off-by: Heiko Schocher hs@denx.de
--- The return values from env_check_redund() for the cases only one read buffer is OK are not perfect, may we should find better return codes?
(no changes since v2)
Changes in v2: - patch "env: split env_import_redund into 2 functions" is new in version 2. Idea is to not duplicate too much code as Simon suggested.
env/common.c | 42 ++++++++++++++++++++++++++++++++---------- include/env.h | 18 ++++++++++++++++++ 2 files changed, 50 insertions(+), 10 deletions(-)
diff --git a/env/common.c b/env/common.c index ed18378000..6c32a9b479 100644 --- a/env/common.c +++ b/env/common.c @@ -141,12 +141,11 @@ int env_import(const char *buf, int check, int flags) #ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT static unsigned char env_flags;
-int env_import_redund(const char *buf1, int buf1_read_fail, - const char *buf2, int buf2_read_fail, - int flags) +int env_check_redund(const char *buf1, int buf1_read_fail, + const char *buf2, int buf2_read_fail) { int crc1_ok, crc2_ok; - env_t *ep, *tmp_env1, *tmp_env2; + env_t *tmp_env1, *tmp_env2;
tmp_env1 = (env_t *)buf1; tmp_env2 = (env_t *)buf2; @@ -159,14 +158,13 @@ int env_import_redund(const char *buf1, int buf1_read_fail, }
if (buf1_read_fail && buf2_read_fail) { - env_set_default("bad env area", 0); return -EIO; } else if (!buf1_read_fail && buf2_read_fail) { gd->env_valid = ENV_VALID; - return env_import((char *)tmp_env1, 1, flags); + return -EINVAL; } else if (buf1_read_fail && !buf2_read_fail) { gd->env_valid = ENV_REDUND; - return env_import((char *)tmp_env2, 1, flags); + return -ENOENT; }
crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) == @@ -175,7 +173,6 @@ int env_import_redund(const char *buf1, int buf1_read_fail, tmp_env2->crc;
if (!crc1_ok && !crc2_ok) { - env_set_default("bad CRC", 0); return -ENOMSG; /* needed for env_load() */ } else if (crc1_ok && !crc2_ok) { gd->env_valid = ENV_VALID; @@ -195,12 +192,37 @@ int env_import_redund(const char *buf1, int buf1_read_fail, gd->env_valid = ENV_VALID; }
+ return 0; +} + +int env_import_redund(const char *buf1, int buf1_read_fail, + const char *buf2, int buf2_read_fail, + int flags) +{ + env_t *ep; + int ret; + + ret = env_check_redund(buf1, buf1_read_fail, buf2, buf2_read_fail); + + if (ret == -EIO) { + env_set_default("bad env area", 0); + return -EIO; + } else if (ret == -EINVAL) { + return env_import((char *)buf1, 1, flags); + } else if (ret == -ENOENT) { + return env_import((char *)buf2, 1, flags); + } else if (ret == -ENOMSG) { + env_set_default("bad CRC", 0); + return -ENOMSG; + } + if (gd->env_valid == ENV_VALID) - ep = tmp_env1; + ep = (env_t *)buf1; else - ep = tmp_env2; + ep = (env_t *)buf2;
env_flags = ep->flags; + return env_import((char *)ep, 0, flags); } #endif /* CONFIG_SYS_REDUNDAND_ENVIRONMENT */ diff --git a/include/env.h b/include/env.h index af405955b0..c15339a93f 100644 --- a/include/env.h +++ b/include/env.h @@ -318,6 +318,24 @@ int env_import(const char *buf, int check, int flags); */ int env_export(struct environment_s *env_out);
+/** + * env_check_redund() - check the two redundant environments + * and find out, which is the valid one. + * + * @buf1: First environment (struct environemnt_s *) + * @buf1_read_fail: 0 if buf1 is valid, non-zero if invalid + * @buf2: Second environment (struct environemnt_s *) + * @buf2_read_fail: 0 if buf2 is valid, non-zero if invalid + * @return 0 if OK, + * -EIO if no environment is valid, + * -EINVAL if read of second entry is good + * -ENOENT if read of first entry is good + * -ENOMSG if the CRC was bad + */ + +int env_check_redund(const char *buf1, int buf1_read_fail, + const char *buf2, int buf2_read_fail); + /** * env_import_redund() - Select and import one of two redundant environments *
On Sat, 10 Oct 2020 at 02:28, Heiko Schocher hs@denx.de wrote:
split from env_import_redund() the part which checks which Environment is valid into a separate function called env_check_redund() and call it from env_import_redund().
So env_check_redund() can be used from places which also need to do this checks.
Signed-off-by: Heiko Schocher hs@denx.de
The return values from env_check_redund() for the cases only one read buffer is OK are not perfect, may we should find better return codes?
(no changes since v2)
Changes in v2:
- patch "env: split env_import_redund into 2 functions" is new in version 2. Idea is to not duplicate too much code as Simon suggested.
env/common.c | 42 ++++++++++++++++++++++++++++++++---------- include/env.h | 18 ++++++++++++++++++ 2 files changed, 50 insertions(+), 10 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Sat, Oct 10, 2020 at 10:28:04AM +0200, Heiko Schocher wrote:
split from env_import_redund() the part which checks which Environment is valid into a separate function called env_check_redund() and call it from env_import_redund().
So env_check_redund() can be used from places which also need to do this checks.
Signed-off-by: Heiko Schocher hs@denx.de Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
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 };
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 flashbefore relocation. Call env_init() and than you can useenv_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;elsegd->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 flashbefore relocation. Call env_init() and than you can useenv_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;elsegd->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 flashbefore relocation. Call env_init() and than you can useenv_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;elsegd->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
On aristianetos boards the display type is detected through "panel" environment variable. Dependend on the display type we detect the board type and this decides which DTB we have to use for the board.
So we need early spi environment access.
Signed-off-by: Heiko Schocher hs@denx.de
---
(no changes since v3)
Changes in v3: - new in v3 as aristainetos board DM changes are in mainline now
configs/aristainetos2_defconfig | 1 + configs/aristainetos2b_defconfig | 1 + configs/aristainetos2bcsl_defconfig | 1 + configs/aristainetos2c_defconfig | 1 + 4 files changed, 4 insertions(+)
diff --git a/configs/aristainetos2_defconfig b/configs/aristainetos2_defconfig index 241b3fea5e..23e5acec5e 100644 --- a/configs/aristainetos2_defconfig +++ b/configs/aristainetos2_defconfig @@ -58,6 +58,7 @@ CONFIG_DTB_RESELECT=y CONFIG_MULTI_DTB_FIT=y CONFIG_ENV_OVERWRITE=y CONFIG_ENV_IS_IN_SPI_FLASH=y +CONFIG_ENV_SPI_EARLY=y CONFIG_SYS_REDUNDAND_ENVIRONMENT=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y CONFIG_VERSION_VARIABLE=y diff --git a/configs/aristainetos2b_defconfig b/configs/aristainetos2b_defconfig index 25a712cfd9..b7530e9ffe 100644 --- a/configs/aristainetos2b_defconfig +++ b/configs/aristainetos2b_defconfig @@ -56,6 +56,7 @@ CONFIG_DTB_RESELECT=y CONFIG_MULTI_DTB_FIT=y CONFIG_ENV_OVERWRITE=y CONFIG_ENV_IS_IN_SPI_FLASH=y +CONFIG_ENV_SPI_EARLY=y CONFIG_SYS_REDUNDAND_ENVIRONMENT=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y CONFIG_VERSION_VARIABLE=y diff --git a/configs/aristainetos2bcsl_defconfig b/configs/aristainetos2bcsl_defconfig index 460a5cf0e7..034d2267fc 100644 --- a/configs/aristainetos2bcsl_defconfig +++ b/configs/aristainetos2bcsl_defconfig @@ -56,6 +56,7 @@ CONFIG_DTB_RESELECT=y CONFIG_MULTI_DTB_FIT=y CONFIG_ENV_OVERWRITE=y CONFIG_ENV_IS_IN_SPI_FLASH=y +CONFIG_ENV_SPI_EARLY=y CONFIG_SYS_REDUNDAND_ENVIRONMENT=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y CONFIG_VERSION_VARIABLE=y diff --git a/configs/aristainetos2c_defconfig b/configs/aristainetos2c_defconfig index 6c708b8e95..d0f4b99eb0 100644 --- a/configs/aristainetos2c_defconfig +++ b/configs/aristainetos2c_defconfig @@ -56,6 +56,7 @@ CONFIG_DTB_RESELECT=y CONFIG_MULTI_DTB_FIT=y CONFIG_ENV_OVERWRITE=y CONFIG_ENV_IS_IN_SPI_FLASH=y +CONFIG_ENV_SPI_EARLY=y CONFIG_SYS_REDUNDAND_ENVIRONMENT=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y CONFIG_VERSION_VARIABLE=y
On Sat, Oct 10, 2020 at 10:28:06AM +0200, Heiko Schocher wrote:
On aristianetos boards the display type is detected through "panel" environment variable. Dependend on the display type we detect the board type and this decides which DTB we have to use for the board.
So we need early spi environment access.
Signed-off-by: Heiko Schocher hs@denx.de
Applied to u-boot/master, thanks!
participants (4)
-
Heiko Schocher -
Michael Walle -
Simon Glass -
Tom Rini