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

[Simon Glass]

Hi Heiko,

On Fri, 15 Nov 2019 at 00:28, Heiko Schocher <hs at 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 at denx.de>
>
> ---
>
> Changes in v2:
> - 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.
>
>  env/Kconfig |  8 ++++++
>  env/sf.c    | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 89 insertions(+)
>
> diff --git a/env/Kconfig b/env/Kconfig
> index 090cc795f9..76e4f30839 100644
> --- a/env/Kconfig
> +++ b/env/Kconfig
> @@ -370,6 +370,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 590d0cedd8..be2e7fc01c 100644
> --- a/env/sf.c
> +++ b/env/sf.c
> @@ -308,6 +308,85 @@ static int env_sf_init(void)
>  }
>  #endif
>
> +#if defined(CONFIG_ENV_SPI_EARLY)
> +static int env_sf_init_early(void)

Function comment to explain what it does?

> +{
> +       int ret;
> +       int read1_fail;
> +#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
> +       int read2_fail;
> +#else
> +       int crc1_ok;
> +#endif
> +
> +       /*
> +        * if malloc is not ready yet, we cannot use
> +        * this part yet.
> +        */
> +       if (!gd->malloc_limit)
> +               return -ENOENT;
> +
> +       env_t *tmp_env2 = NULL;
> +       env_t *tmp_env1;
> +
> +       tmp_env1 = (env_t *)memalign(ARCH_DMA_MINALIGN,
> +                       CONFIG_ENV_SIZE);
> +#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT

Can the #ifdefs in this function be if(IS_ENABLED...) instead?

> +       tmp_env2 = (env_t *)memalign(ARCH_DMA_MINALIGN,
> +                       CONFIG_ENV_SIZE);
> +#endif
> +       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);
> +#ifdef 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))

Can drop extra brackets

> +               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;
> +#endif
> +
> +       return 0;
> +err_read:
> +       spi_flash_free(env_flash);
> +       env_flash = NULL;
> +       free(tmp_env1);
> +#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT

Unrelated to your patch, but should that be REDUNDANT?

> +       free(tmp_env2);
> +#endif
> +out:
> +       /* env is not valid. always return 0 */
> +       gd->env_valid = ENV_INVALID;
> +       return 0;
> +}
> +#endif
> +
>  U_BOOT_ENV_LOCATION(sf) = {
>         .location       = ENVL_SPI_FLASH,
>         ENV_NAME("SPI Flash")
> @@ -317,5 +396,7 @@ U_BOOT_ENV_LOCATION(sf) = {
>  #endif
>  #if defined(INITENV) && defined(CONFIG_ENV_ADDR)
>         .init           = env_sf_init,
> +#elif defined(CONFIG_ENV_SPI_EARLY)
> +       .init           = env_sf_init_early,
>  #endif

It seems like we should have two drivers here, only one of which is
enabled. Perhaps for testing sandbox would want to enable both?

Alternatively I think env_sf_init() should decide which init function to call.

Regards,
Simon