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

[Heiko Schocher]

Hello Simon,

Am 16.01.2020 um 01:10 schrieb Simon Glass:
> Hi Heiko,
> 
> On Wed, 15 Jan 2020 at 23:13, Heiko Schocher <hs at denx.de> wrote:
>>
>> 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(+)
>>>>
> 
> [..]
> 
>>>> +       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 ...
> 
> OK maybe it is not in Kconfig yet?

No, both are in Kconfig, and ENV_OFFSET_REDUND depends on SYS_REDUNDAND_ENVIRONMENT
so all should be fine.

>>>> +       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)
>>                       ^~
> 
> No, I mean:
> 
>     if (ret == -EIO || ret == -ENOMSG)

of course, sorry for being so stupid!

>>>> +               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()
> 
> Yes I think it is better to have the function decide is anything is needed.

Ok, I rework it.

Thanks for your comments!

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