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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot