diff mbox series

[U-Boot,v2,2/2] env: Access Environment in SPI flashes before relocation

Message ID 20191115072737.3543518-3-hs@denx.de
State Needs Review / ACK
Delegated to: Tom Rini
Headers show
Series env: Access Environment in SPI flashes before relocation | expand

Commit Message

Heiko Schocher Nov. 15, 2019, 7:27 a.m. UTC
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@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(+)

Comments

Simon Glass Dec. 10, 2019, 12:39 p.m. UTC | #1
Hi Heiko,

On Fri, 15 Nov 2019 at 00:28, Heiko Schocher <hs@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@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
Heiko Schocher Jan. 15, 2020, 10:13 a.m. UTC | #2
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@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@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
Simon Glass Jan. 16, 2020, 12:10 a.m. UTC | #3
Hi Heiko,

On Wed, 15 Jan 2020 at 23:13, Heiko Schocher <hs@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@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@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
Heiko Schocher Jan. 16, 2020, 6:25 a.m. UTC | #4
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@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@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@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
diff mbox series

Patch

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