From mboxrd@z Thu Jan 1 00:00:00 1970 From: Goldschmidt Simon Date: Wed, 7 Feb 2018 20:45:45 +0000 Subject: [U-Boot] [PATCH v3 09/15] env: Support multiple environments Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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