Re: [U-Boot] [PATCH v3 09/15] env: Support multiple environments
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
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.
York
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
participants (3)
-
Goldschmidt Simon -
Simon Goldschmidt
-
York Sun