[U-Boot] [PATCH v3 09/15] env: Support multiple environments

[Simon Goldschmidt]

On 31.01.2018 00:02, York Sun wrote:
> On 01/30/2018 12:16 PM, York Sun wrote:
>> On 01/30/2018 11:40 AM, York Sun wrote:
>>> On 01/30/2018 12:19 AM, Simon Goldschmidt wrote:
>>>> On 23.01.2018 21:16, Maxime Ripard wrote:
>>>>> Now that we have everything in place to support multiple environment, let's
>>>>> make sure the current code can use it.
>>>>>
>>>>> The priority used between the various environment is the same one that was
>>>>> used in the code previously.
>>>>>
>>>>> At read / init times, the highest priority environment is going to be
>>>>> detected,
>>>> Does priority handling really work here? Most env drivers seem to ignore
>>>> the return value of env_import and may thus return success although
>>>> importing the environment failed (only reading the data from the device
>>>> succeeded).
>>>>
>>>> This is from reading the code, I haven't had a chance to test this, yet.
>>> It is broken on my LS1043ARDB with simply NOR flash. I am trying to
>>> determine what went wrong.
>>>
>> I found the problem. The variable "env_load_location" is static. It is
>> probably not write-able during booting from flash. It is expected to be
>> set during ENVOP_INIT. But if I print this variable, it has
>> ENVL_UNKNOWN. I can make it work by moving env_load_location to global
>> data structure.
>>
>> That being said, this addition of multiple environments really slows
>> down the booting process for me. I see every time env_get_char() is
>> called, env_driver_lookup() runs. A simple call of env_get_f() gets
>> slowed down dramatically. I didn't find out where the most time is spent
>> yet.
>>
>> Does anyone else experience this unbearable slowness?

Yes. Depending on CPU speed... :-)

On my board, that slowdown comes from the fact that env_get_f does not 
check the return value of env_get_char for an error. That leads to 
trying for CONFIG_ENV_SIZE times, which is of course slow.
I'll post a fix for that.

> I found the problem. In patch #3 in this series, the default get_char()
> was dropped so there is no driver for a plain NOR flash. A quick (and
> maybe dirty) fix is this

That patch #3 actually changed the behavior for all env drivers not 
providing .get_char (so all drivers except for eeprom and nvram) from 
returning what's behind gd->env_addr. Your patch below restores the old 
behaviour.

I can't tell what's the correct behaviour though: in my tests, env_get_f 
got called even after importing the environment, which is not what I 
would have expected...

> 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);
>
>                  if (!env_has_inited(drv->location))
>                          continue;
>
> With this temporary fix, my flash chip works again and I can boot all
> the way up in a timely manner. I still don't like to call
> env_driver_lookup() thousands of times to get a simple env variable.

That's not a thing Maxime has changed but a change that came in when 
adding environment drivers.

Simon

>
> Can Maxime post a quick post soon?
>
> York
>