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