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

[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?

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

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

Regards,
Simon