[PATCH v4 2/3] env: Access Environment in SPI flashes before relocation

[Michael Walle]

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.

-michael