[PATCH v4 2/3] env: Access Environment in SPI flashes before relocation
Heiko Schocher
hs at denx.de
Mon Nov 2 06:20:29 CET 2020
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 at 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: hs at denx.de
More information about the U-Boot
mailing list