Re: [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation

2020-10-30 Thread Tom Rini 
On Sat, Oct 10, 2020 at 10:28:05AM +0200, Heiko Schocher 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 
> Reviewed-by: Simon Glass 

Applied to u-boot/master, thanks!

-- 
Tom


signature.asc
Description: PGP signature

Re: [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation

2020-10-30 Thread Michael Walle 

Hi Heiko, Hi Tom,

Am 2020-10-10 10:28, schrieb Heiko Schocher:

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

Changes in v4:
- rebased to current master 5dcf7cc590

Changes in v3:
- 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.
- add comment from Simon Glass
  - add missing function comments
  - use if(IS_ENABLED...)
  - drop extra brackets
  - let env_sf_init() decide, which function to call
add comment that it is necessary to return env_sf_init()
with 0.

 env/Kconfig |   8 +
 env/sf.c| 100 ++--
 2 files changed, 105 insertions(+), 3 deletions(-)

diff --git a/env/Kconfig b/env/Kconfig
index c6ba08878d..393b191a30 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -376,6 +376,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 937778aa37..2eb2de1a4e 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -287,7 +287,10 @@ __weak void *env_sf_get_env_addr(void)
 #endif

 #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
-static int env_sf_init(void)
+/*
+ * check if Environment on CONFIG_ENV_ADDR is valid.
+ */
+static int env_sf_init_addr(void)
 {
env_t *env_ptr = (env_t *)env_sf_get_env_addr();

@@ -303,12 +306,103 @@ static int env_sf_init(void)
 }
 #endif

+#if defined(CONFIG_ENV_SPI_EARLY)
+/*
+ * early load environment from SPI flash (before relocation)
+ * and check if it is valid.
+ */
+static int env_sf_init_early(void)
+{
+   int ret;
+   int read1_fail;
+   int read2_fail;
+   int crc1_ok;
+   env_t *tmp_env2 = NULL;
+   env_t *tmp_env1;
+
+   /*
+* if malloc is not ready yet, we cannot use
+* this part yet.
+*/
+   if (!gd->malloc_limit)
+   return -ENOENT;
+
+   tmp_env1 = (env_t *)memalign(ARCH_DMA_MINALIGN,
+   CONFIG_ENV_SIZE);
+   if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
+   tmp_env2 = (env_t *)memalign(ARCH_DMA_MINALIGN,
+CONFIG_ENV_SIZE);
+
+   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);
+
+   if (IS_ENABLED(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;
+   }
+
+   return 0;
+err_read:
+   spi_flash_free(env_flash);
+   env_flash = NULL;
+   free(tmp_env1);
+   if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
+   free(tmp_env2);
+out:
+   /* env is not valid. always return 0 */
+   gd->env_valid = ENV_INVALID;
+   return 0;
+}
+#endif
+
+static int env_sf_init(void)
+{
+#if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
+   return env_sf_init_addr();
+#elif defined(CONFIG_ENV_SPI_EARLY)
+   return env_sf_init_early();
+#endif
+   /*
+* we must return with 0 if there is nothing done,
+* else env_set_inited() get not called in env_init()
+*/
+

Re: [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation

2020-10-30 Thread Tom Rini 
On Sat, Oct 31, 2020 at 12:51:27AM +0100, Michael Walle wrote:
> Hi Heiko, Hi Tom,
> 
> Am 2020-10-10 10:28, schrieb Heiko Schocher:
> > 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 
> > ---
> > 
> > Changes in v4:
> > - rebased to current master 5dcf7cc590
> > 
> > Changes in v3:
> > - 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.
> > - add comment from Simon Glass
> >   - add missing function comments
> >   - use if(IS_ENABLED...)
> >   - drop extra brackets
> >   - let env_sf_init() decide, which function to call
> > add comment that it is necessary to return env_sf_init()
> > with 0.
> > 
> >  env/Kconfig |   8 +
> >  env/sf.c| 100 ++--
> >  2 files changed, 105 insertions(+), 3 deletions(-)
> > 
> > diff --git a/env/Kconfig b/env/Kconfig
> > index c6ba08878d..393b191a30 100644
> > --- a/env/Kconfig
> > +++ b/env/Kconfig
> > @@ -376,6 +376,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 937778aa37..2eb2de1a4e 100644
> > --- a/env/sf.c
> > +++ b/env/sf.c
> > @@ -287,7 +287,10 @@ __weak void *env_sf_get_env_addr(void)
> >  #endif
> > 
> >  #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
> > -static int env_sf_init(void)
> > +/*
> > + * check if Environment on CONFIG_ENV_ADDR is valid.
> > + */
> > +static int env_sf_init_addr(void)
> >  {
> > env_t *env_ptr = (env_t *)env_sf_get_env_addr();
> > 
> > @@ -303,12 +306,103 @@ static int env_sf_init(void)
> >  }
> >  #endif
> > 
> > +#if defined(CONFIG_ENV_SPI_EARLY)
> > +/*
> > + * early load environment from SPI flash (before relocation)
> > + * and check if it is valid.
> > + */
> > +static int env_sf_init_early(void)
> > +{
> > +   int ret;
> > +   int read1_fail;
> > +   int read2_fail;
> > +   int crc1_ok;
> > +   env_t *tmp_env2 = NULL;
> > +   env_t *tmp_env1;
> > +
> > +   /*
> > +* if malloc is not ready yet, we cannot use
> > +* this part yet.
> > +*/
> > +   if (!gd->malloc_limit)
> > +   return -ENOENT;
> > +
> > +   tmp_env1 = (env_t *)memalign(ARCH_DMA_MINALIGN,
> > +   CONFIG_ENV_SIZE);
> > +   if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
> > +   tmp_env2 = (env_t *)memalign(ARCH_DMA_MINALIGN,
> > +CONFIG_ENV_SIZE);
> > +
> > +   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);
> > +
> > +   if (IS_ENABLED(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;
> > +   }
> > +
> > +   return 0;
> > +err_read:
> > +   spi_flash_free(env_flash);
> > +   env_flash = NULL;
> > +   free(tmp_env1);
> > +   if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
> > +   free(tmp_env2);
> > +out:
> > +   /* env is not valid. always return 0 */
> > +   gd->env_valid = ENV_INVALID;
> >

Re: [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation

2020-10-30 Thread Michael Walle 

Hi,

Am 2020-10-31 01:07, schrieb Tom Rini:
[..]

> +static int env_sf_init(void)
> +{
> +#if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
> +  return env_sf_init_addr();
> +#elif defined(CONFIG_ENV_SPI_EARLY)
> +  return env_sf_init_early();
> +#endif
> +  /*
> +   * we must return with 0 if there is nothing done,
> +   * else env_set_inited() get not called in env_init()
> +   */
> +  return 0;
> +}
> +
>  U_BOOT_ENV_LOCATION(sf) = {
>.location   = ENVL_SPI_FLASH,
>ENV_NAME("SPIFlash")
>.load   = env_sf_load,
>.save   = CONFIG_IS_ENABLED(SAVEENV) ? ENV_SAVE_PTR(env_sf_save) :
> NULL,
> -#if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
>.init   = env_sf_init,
> -#endif

This will break my board (the new kontron sl28). Before the
change, the environment is loaded from SPI, after this patch
the environment won't be loaded anymore. If I delete the
.init callback, the behavior is the same as before.

The problem here is that returning 0 in env_sf_init() is not
enough because if the .init callback is not set, env_init() will
have ret defaulting to -ENOENT and in this case will load the
default environment and setting env_valid to ENV_VALID. I didn't
follow the whole call chain then. But this will then eventually
lead to loading the actual environment in a later stage.


Ugh.  The games we play in the env area really just need to be
rewritten.  So at this point I've merged the series, should I revert or
do you see an easy fix for your platform?  Thanks!


Mh, my board cannot be the only one with a late environment
from SPI flash, can it?

Returning 0 will call env_set_inited() and returning -ENOENT
will call the second part. I can't tell why it was done that
way. So I don't have a simple fix.

But I guess the following ugly ifdeffery could do it:

#if (defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)) || 
defined(CONFIG_ENV_SPI_EARLY)

.init   = env_sf_init,
#endif

I can try that tomorrow. Also the comment about the return
value should be removed (?).

-michael

Re: [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation

2020-11-01 Thread Heiko Schocher 

Hello Tom, Michael,

Am 31.10.2020 um 01:07 schrieb Tom Rini:

On Sat, Oct 31, 2020 at 12:51:27AM +0100, Michael Walle wrote:

Hi Heiko, Hi Tom,

Am 2020-10-10 10:28, schrieb Heiko Schocher:

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

Changes in v4:
- rebased to current master 5dcf7cc590

Changes in v3:
- 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.
- add comment from Simon Glass
   - add missing function comments
   - use if(IS_ENABLED...)
   - drop extra brackets
   - let env_sf_init() decide, which function to call
 add comment that it is necessary to return env_sf_init()
 with 0.

  env/Kconfig |   8 +
  env/sf.c| 100 ++--
  2 files changed, 105 insertions(+), 3 deletions(-)

diff --git a/env/Kconfig b/env/Kconfig
index c6ba08878d..393b191a30 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -376,6 +376,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 937778aa37..2eb2de1a4e 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -287,7 +287,10 @@ __weak void *env_sf_get_env_addr(void)
  #endif

  #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
-static int env_sf_init(void)
+/*
+ * check if Environment on CONFIG_ENV_ADDR is valid.
+ */
+static int env_sf_init_addr(void)
  {
env_t *env_ptr = (env_t *)env_sf_get_env_addr();

@@ -303,12 +306,103 @@ static int env_sf_init(void)
  }
  #endif

+#if defined(CONFIG_ENV_SPI_EARLY)
+/*
+ * early load environment from SPI flash (before relocation)
+ * and check if it is valid.
+ */
+static int env_sf_init_early(void)
+{
+   int ret;
+   int read1_fail;
+   int read2_fail;
+   int crc1_ok;
+   env_t *tmp_env2 = NULL;
+   env_t *tmp_env1;
+
+   /*
+* if malloc is not ready yet, we cannot use
+* this part yet.
+*/
+   if (!gd->malloc_limit)
+   return -ENOENT;
+
+   tmp_env1 = (env_t *)memalign(ARCH_DMA_MINALIGN,
+   CONFIG_ENV_SIZE);
+   if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
+   tmp_env2 = (env_t *)memalign(ARCH_DMA_MINALIGN,
+CONFIG_ENV_SIZE);
+
+   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);
+
+   if (IS_ENABLED(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;
+   }
+
+   return 0;
+err_read:
+   spi_flash_free(env_flash);
+   env_flash = NULL;
+   free(tmp_env1);
+   if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
+   free(tmp_env2);
+out:
+   /* env is not valid. always return 0 */
+   gd->env_valid = ENV_INVALID;
+   return 0;
+}
+#endif
+
+static int env_sf_init(void)
+{
+#if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
+   return env_sf_init_addr();
+#elif defined(CONFIG_ENV_SPI_EARLY)
+   return env_sf_init_early();
+#endif

Re: [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation

2020-10-11 Thread Simon Glass 
On Sat, 10 Oct 2020 at 02:28, Heiko Schocher  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 
> ---
>
> Changes in v4:
> - rebased to current master 5dcf7cc590
>
> Changes in v3:
> - 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.
> - add comment from Simon Glass
>   - add missing function comments
>   - use if(IS_ENABLED...)
>   - drop extra brackets
>   - let env_sf_init() decide, which function to call
> add comment that it is necessary to return env_sf_init()
> with 0.
>
>  env/Kconfig |   8 +
>  env/sf.c| 100 ++--
>  2 files changed, 105 insertions(+), 3 deletions(-)

Reviewed-by: Simon Glass