From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Mon, 2 Nov 2020 06:20:29 +0100 Subject: [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation In-Reply-To: <20201031000703.GZ5340@bill-the-cat> References: <20201010082806.2260318-1-hs@denx.de> <20201010082806.2260318-3-hs@denx.de> <24cf63b0b269f6beb0c8470b57cab622@walle.cc> <20201031000703.GZ5340@bill-the-cat> Message-ID: <376c37e7-f82d-e412-fa3b-c71fe74ec163@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>> + /* >>> + * 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