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

[Goldschmidt Simon]

On 02/07/2018 21:18, York Sun wrote:
> 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.

I still think we should get rid of the 'get_char' callback for
the env drivers. While that could have made sense for some boards
before the conversion to multiple environments (although I
doubt that, as the environment is *not* checked for
validity in this call), its meaning is totally lost when having
multiple env drivers active.

The whole purpose of multiple env drivers is that we select a
valid driver in the 'load' callback. How could we possibly know
that the 'get_char' callback of the highest prio env driver is
what we want?

I'd rather make 'env_get_char' weak and let boards decide if they
really want this behaviour.

A quick search through the current code base shows me *no* usage
of 'get_char' in nvram.c (CONFIG_SYS_NVRAM_ACCESS_ROUTINE is never
defined) and only 7 defconfigs that use ENV_IS_IN_EEPROM:

    imx31_phycore_defconfig
    imx31_phycore_eet_defconfig
    km_kirkwood_128m16_defconfig
    km_kirkwood_defconfig
    km_kirkwood_pci_defconfig
    mgcoge3un_defconfig
    portl2_defconfig

Are these seven boards worth keeping this feature?

Simon

>>
>>> 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