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

[Simon Goldschmidt]

On 07.02.2018 22:18, York Sun wrote:
> 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.

The change I proposed would be to restore the old behavior but kick out 
the byte-by-byte reading from eeprom and nvram. My understanding was you 
are using flash and were reading from environment in ram, not in nvram 
or eeprom.

Simon