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 <h...@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
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: h...@denx.de
