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