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

[Heiko Schocher]

Hello Simon,

Am 10.12.2019 um 13:39 schrieb 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?

Ok, added.

>> +{
>> +       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?

I think yes.

>> +       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);

Hmm... if porting to "if(IS_ENABLED...)" may here is CONFIG_ENV_OFFSET_REDUND
not defined ... I will see on world build ...

>> +       ret = env_check_redund((char *)tmp_env1, read1_fail,
>> +                              (char *)tmp_env2, read2_fail);
>> +
>> +       if ((ret == -EIO) || (ret == -ENOMSG))
> 
> Can drop extra brackets

Without this bracket I get:

/home/hs/data/Entwicklung/abb/uboot-rework/u-boot/env/sf.c: In function ‘env_sf_init_early’:
/home/hs/data/Entwicklung/abb/uboot-rework/u-boot/env/sf.c:355:20: error: expected expression before 
‘||’ token
    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;
>> +#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?

Hmm.. currently the define is REDUNDAND ... but you are correct ...

>> +       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.

Ok, I can change this, but this leads into change, that we now always
call ".init" ...

in env/env.c env_init():

         for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {
                 if (!drv->init || !(ret = drv->init()))
                         env_set_inited(drv->location);

So, if env_sf_init() return 0 in case nothing ToDo, it is OK to change
this. I added a comment in env_sf_init()

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