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

[York Sun]

On 02/07/2018 12:43 AM, Maxime Ripard wrote:
> Hi,
> 
> On Tue, Jan 30, 2018 at 11:02:49PM +0000, 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 would work for me.
> 
>>> 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?
>>>
>>
>> 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
>>
>>
>> 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;
> 
> And this too.

If you agree with this fix (actually revert your change earlier), I can
send out a patch.

> 
>> 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.
>>
>> Can Maxime post a quick post soon?
> 
> Given that you already made all the debugging, and the patches, and I
> have no way to test, I guess it would make more sense if you did it :)

Yes, I have tested on my boards. I will send out this patch.

York