On 02/08/2018 02:05 AM, Simon Goldschmidt wrote:
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.
Leaving it as ENVL_UNKNOWN doesn't fix the problem. The env_load_location needs to be valid after relocation. Leaving it as unknown doesn't let you write env.
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.
Not everything in initial RAM is copied to final RAM. After relocation, we shouldn't be using initial RAM at all. Actually the initial RAM may be used for other purpose.
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).
Yes. That was my intention.
York