[e-news abonnieren]http://www.pepperl-fuchs.de/e-news [YouTube: Pepperl+Fuchs auf YouTube] http://www.youtube.com/user/PepperlFuchsGmbH [@PepperlFuchsDE folgen] https://twitter.com/PepperlFuchsDE [Pepperl+Fuchs auf XING] https://www.xing.com/companies/pepperl%252bfuchsgmbh

________________________________ [Pepperl+Fuchs GmbH]http://www.pepperl-fuchs.com

Denken Sie bitte an die Umwelt und prüfen Sie, ob diese E-Mail wirklich ausgedruckt werden muss.

SocialMedia2014

On 08.02.2018 09:47, Maxime Ripard wrote:

On Wed, Feb 07, 2018 at 02:17:12PM -0800, York Sun wrote:

Commit 8a3a7e2270b3 ("env: Pass additional parameters to the env lookup function") dropped the default action if driver doesn't have get_char() defined. This causes failure to get environmental variables from NOR flash. Add back this default action for now.

Signed-off-by: York Sun york.sun@nxp.commailto:york.sun@nxp.com CC: Maxime Ripard maxime.ripard@free-electrons.commailto:maxime.ripard@free-electrons.com --- Limited test on LS1043ARDB.

env/env.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/env/env.c b/env/env.c index edfb575..210bae2 100644 --- a/env/env.c +++ b/env/env.c @@ -159,7 +159,7 @@ int env_get_char(int index) int ret;

if (!drv->get_char) - continue; + return *(uchar *)(gd->env_addr + index);

Thinking more about this, I think this would break the case where the first environment in your list has no get_char method, but the second might.

How can we decide which way is wanted? With your patch below, we might end up loading chars from a low-prio environment (which is not CRC checked) in the early boot stage. Later, we load the environment from another env driver with higher priority.

That's why I suggested removing the 'get_char' callback from the env drivers :-) Early boot stage environment lookups would still work the old way reading from 'gd->env_addr + index'.

Simon

Would something like that work?

------------ 8< -------- diff --git a/env/env.c b/env/env.c index 9a89832c1aaf..391c9c0df2f0 100644 --- a/env/env.c +++ b/env/env.c @@ -152,6 +152,7 @@ static struct env_driver *env_driver_lookup(enum env_operation op, int prio) int env_get_char(int index) { struct env_driver *drv; + bool has_get_char = false; int prio;

if (gd->env_valid == ENV_INVALID) @@ -166,6 +167,7 @@ int env_get_char(int index) if (!env_has_inited(drv->location)) continue;

+ has_get_char = true; ret = drv->get_char(index); if (!ret) return 0; @@ -174,6 +176,9 @@ int env_get_char(int index) drv->name, ret); }

+ if (!has_get_char) + return *(uchar *)(gd->env_addr + index); + return -ENODEV; } ------ 8< -------

Thanks! Maxime

_______________________________________________ U-Boot mailing list U-Boot@lists.denx.demailto:U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot

On 08.02.2018 09:47, Maxime Ripard wrote:

On Wed, Feb 07, 2018 at 02:17:12PM -0800, York Sun wrote:

...

Commit 8a3a7e2270b3 ("env: Pass additional parameters to the env lookup function") dropped the default action if driver doesn't have get_char() defined. This causes failure to get environmental variables from NOR flash. Add back this default action for now.

Signed-off-by: York Sun york.sun@nxp.com CC: Maxime Ripard maxime.ripard@free-electrons.com

---

Limited test on LS1043ARDB.

env/env.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/env/env.c b/env/env.c index edfb575..210bae2 100644 --- a/env/env.c +++ b/env/env.c @@ -159,7 +159,7 @@ int env_get_char(int index) int ret;

if (!drv->get_char)

• 	continue;
  

• 	return *(uchar *)(gd->env_addr + index);
  

Thinking more about this, I think this would break the case where the first environment in your list has no get_char method, but the second might.

How can we decide which way is wanted? With your patch below, we might end up loading chars from a low-prio environment (which is not CRC checked) in the early boot stage. Later, we load the environment from another env driver with higher priority.

That's why I suggested removing the 'get_char' callback from the env drivers :-) Early boot stage environment lookups would still work the old way reading from 'gd->env_addr + index'.

Simon

Would something like that work?

------------ 8< -------- diff --git a/env/env.c b/env/env.c index 9a89832c1aaf..391c9c0df2f0 100644 --- a/env/env.c +++ b/env/env.c @@ -152,6 +152,7 @@ static struct env_driver *env_driver_lookup(enum env_operation op, int prio) int env_get_char(int index) { struct env_driver *drv;

•   bool has_get_char = false;
     int prio;
  
     if (gd->env_valid == ENV_INVALID)
  

@@ -166,6 +167,7 @@ int env_get_char(int index) if (!env_has_inited(drv->location)) continue;

•           has_get_char = true;
             ret = drv->get_char(index);
             if (!ret)
                     return 0;
  

@@ -174,6 +176,9 @@ int env_get_char(int index) drv->name, ret); }

•   if (!has_get_char)
  

•           return *(uchar *)(gd->env_addr + index);
  

•    return -ENODEV;
  

  }

------ 8< -------

Thanks! Maxime

---

U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot

Maxime, York,

I've just sent a patch that removes 'get_char' callback from the env drivers. This should restore the old behaviour we had before supporting multiple environment drivers (for all but eeprom, of course).

Simon

On 08.02.2018 23:10, Maxime Ripard wrote:

On Thu, Feb 08, 2018 at 10:52:20AM +0100, Simon Goldschmidt wrote:

...

On 08.02.2018 09:47, Maxime Ripard wrote:

...

On Wed, Feb 07, 2018 at 02:17:12PM -0800, York Sun wrote:

...

Commit 8a3a7e2270b3 ("env: Pass additional parameters to the env lookup function") dropped the default action if driver doesn't have get_char() defined. This causes failure to get environmental variables from NOR flash. Add back this default action for now.

Signed-off-by: York Sun york.sun@nxp.com CC: Maxime Ripard maxime.ripard@free-electrons.com

---

Limited test on LS1043ARDB.

env/env.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/env/env.c b/env/env.c index edfb575..210bae2 100644 --- a/env/env.c +++ b/env/env.c @@ -159,7 +159,7 @@ int env_get_char(int index) int ret; if (!drv->get_char)

• 	continue;
  

• 	return *(uchar *)(gd->env_addr + index);
  

Thinking more about this, I think this would break the case where the first environment in your list has no get_char method, but the second might.

How can we decide which way is wanted? With your patch below, we might end up loading chars from a low-prio environment (which is not CRC checked) in the early boot stage. Later, we load the environment from another env driver with higher priority.

Ah, right.

...

That's why I suggested removing the 'get_char' callback from the env drivers :-) Early boot stage environment lookups would still work the old way reading from 'gd->env_addr + index'.

If that works on York's board, I'm all in.

Maxime

On 08.02.2018 09:38, Maxime Ripard wrote:

Hi,

Thanks for your patch

On Wed, Feb 07, 2018 at 02:17:11PM -0800, York Sun wrote:

...

Commit 7d714a24d725 ("env: Support multiple environments") added static variable env_load_location. When saving environmental variables, this variable is presumed to have the value set before. In case the value was set before relocation and U-Boot runs from a NOR flash, this variable wasn't writable. This causes failure when saving the environment. To save this location, global data must be used instead.

Out of curiosity: which linker sections are affected of this issue? I assume only initialized data ('.data') is affected as it is left in place. Also, I assume that uninitialized data ('.bss') is not affected (as your linker can place this into ram)? If so, this issue could be fixed by changing ENVL_UNKNOWN to zero (which you already do in your patch) and leaving 'env_load_location' uninitialized, in which case it is initialized to zero (== ENVL_UNKNOWN) as defined by the C standard.

Another possibility to fix this would be to have your linker script put 'data' into ram but initialize it from rom. Something like this:

.data : {   <your section contents here> } >ram AT>rom

To me, both versions would be better than to add members to struct global_dat.

...

Signed-off-by: York Sun york.sun@nxp.com CC: Maxime Ripard maxime.ripard@free-electrons.com

---

Limited test on LS1043ARDB.

env/env.c | 8 +++----- include/asm-generic/global_data.h | 1 + include/environment.h | 2 +- 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/env/env.c b/env/env.c index 9a89832..edfb575 100644 --- a/env/env.c +++ b/env/env.c @@ -62,8 +62,6 @@ static enum env_location env_locations[] = { #endif };

-static enum env_location env_load_location = ENVL_UNKNOWN;

• static bool env_has_inited(enum env_location location) { return gd->env_has_init & BIT(location);

@@ -108,11 +106,11 @@ __weak enum env_location env_get_location(enum env_operation op, int prio) if (prio >= ARRAY_SIZE(env_locations)) return ENVL_UNKNOWN;

• env_load_location = env_locations[prio];
  

• return env_load_location;
  

• gd->env_load_location = env_locations[prio];
  

• return gd->env_load_location;
  
  

  case ENVOP_SAVE:

• return env_load_location;
  

• return gd->env_load_location;
  

  }

  return ENVL_UNKNOWN;

diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index fd8cd45..10f1441 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -51,6 +51,7 @@ typedef struct global_data { unsigned long env_addr; /* Address of Environment struct */ unsigned long env_valid; /* Environment valid? enum env_valid */ unsigned long env_has_init; /* Bitmask of boolean of struct env_location offsets */

• int env_load_location;

  unsigned long ram_top; /* Top address of RAM used by U-Boot */ unsigned long relocaddr; /* Start address of U-Boot in RAM */

diff --git a/include/environment.h b/include/environment.h index a406050..0f339da 100644 --- a/include/environment.h +++ b/include/environment.h @@ -188,6 +188,7 @@ enum env_valid { };

enum env_location {

• ENVL_UNKNOWN, ENVL_EEPROM, ENVL_EXT4, ENVL_FAT,

@@ -202,7 +203,6 @@ enum env_location { ENVL_NOWHERE,

ENVL_COUNT,

• ENVL_UNKNOWN,

Why did you need to change this? This looks a bit odd.

The global data struct is initialized to zero. I guess this change is meant to ensure 'gd->env_load_location' is initialized to ENV_UNKOWN (see my comments above).

Simon

Thanks! Maxime

---

U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot