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

[York Sun]

On 02/07/2018 12:45 PM, Goldschmidt Simon wrote:
> 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.

Actually I am not a big fun to using global data. It increases size for
everybody. But I don't see a way you can save the variable before
relocation.

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

Adding multiple environments seems to be an improvement. But this fell
through the cracks. I don't know if other boards also read env before
relocation. All my boards reads hwconfig before relocation. Having to
create a new function for all of them doesn't look appealing to me.

York