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....@nxp.com>
>>> CC: Maxime Ripard <maxime.rip...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
