[U-Boot] [PATCH v3 09/15] env: Support multiple environments

[Maxime Ripard]

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.

> 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 :)

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180207/bcecb269/attachment.sig>