[U-Boot] [ARM] Environment variables not available during console initialisation?

[Wolfgang Denk]

Dear Guennadi Liakhovetski,

In message <Pine.LNX.4.64.0902022310070.8602 at axis700.grange> you wrote:
> 
> > Such an explanation belongs into the commit message.
> 
> Sure, _if_ we decide, that this is the correct direction to fix this 
> problem.

We're iterating here to make this a usable patch, right? Such
description should be added to the next version.

> > We have two situations only:
> > 
> > 1) environment is on the same NAND chip as the U-Boot image; this is
> >    the case we can handle cleanly. But it does not play any role of
> >    the environment is embedded, or directly adjacent (either before
> >    or after) to the U-Boot image, or if it is on a completely
> >    different location on this NAND chip.
> 
> Well, there is a difference. If the environment is embedded, it is loaded 
> by nand_spl automatically with the image with a single nand_load() (that's 
> how it worked until now, I think). If the environment is not embedded but 
> on the same chip, we still can handle it, but we need an additional 
> nand_load(). And the copy of the environment from NAND as loaded into RAM 
> will be at a different location. Maybe env_nand.c doesn't have to know 
> about this difference, but I haven't gone that far to unify these two 
> cases.

Why not? The only thing env_nand.c needs to know is the address of the
environment in memory - it does not care if it's embedded or
elsewhere.

I think we should not artifically split this into two cases when it's
really one. There is two things that need to be done: (1) nand_spl must
locate a valid copy of the environment and load it into RAM, and (2)
the address must be passed to env_nand.c, but this is probably easy as
it can be statically defined, I think.

> > To me this means that 1) is the case we implement, without additional
> > #ifdef'fery, and 2) is a configuration error for which we should  add
> > appropriate #error handling.
> 
> Hm, as I briefly looked through other env_*.c, it looks like a few of them 
> resort to the default environment until env_relocate(). So, we can either 

Which ones? I think this is almost certainly broken, then.

> accept that and say "on those media you can store environment, but it will 
> be only available late in boot process," or we can start fixing them all 
> by shifting hardware initialisation before env_init()... Or are you 
> suggesting to declare them all as broken?

Not declare them as broken - just state what they are.

But that wa snot what I suggested above. I wrote that we should  bail
out  with  an  error  message  if  somebody  attempts  to  compile  a
configuration where the environment is on a different NAND chip  than
the U-Boot image - it's a non-supported configuration.

> > > > >  #define CFG_ENV_OFFSET		0x0040000
> > > > > +/* Leave enough space for bss, currently __bss_end == 0x57e74800 */
> > > > > +#define CFG_NAND_ENV_DST	(CFG_UBOOT_BASE + 0x80000)
> > > > 
> > > > ???? What's that? What has BSS to do with the environment storage ???
> > > 
> > > This is smdk6400 header, on this board I chose to place the environment 
> > > read in by nand_spl above bss, because, AFAIU, the area above __bss_end is 
> > 
> > What has BSS to do with the environment storage ???
> > 
> > BSS does not take any space in the U-Boot image, so why leave a
> > (eventually huge) gap unused?
> 
> I need a location in RAM for nand_spl to put a copy of the environment 
> into. I know that bss doesn't take space in the u-boot image. But you 
> cannot put environment there, because it will be nullified by u-boot 
> proper.

I think there is some problem with the memory map on this system. It's
next to impossible to understand it's design from the config file.
Maybe you should document this in the first place. No, please deleted
the "maybe".

Your patch refers to CFG_MAPPED_RAM_BASE  which  is  not  present  in
mainline. Then you define CFG_UBOOT_BASE. Using a fixed offset, i. e.
probably without dynamic memory sizing? Grrrgh. Ah, right, that's ARM
where this is broken anyway.

Well, I think it is fundamentally important here that you take off the
ARM goggles here. Please be aware that such code must run on PPC as
well, i. e. it must be usable on systems that do proper auto-sizing
etc.

If you need space for the environment copy, you  must  not  just  use
some  area  somewhere  "behind"  U-Boot,  where  it  will most likely
collide with protected ram, shared log buffer,  frame  buffer  memory
etc. Instead, you must design a place for it in the memory map.

Thus please start by documenting the intended memory map.

> > What has the location in RAM to do with the image structure on NAND?
> > We don't need to allocate any space for BSS on the NAND device, right?
> 
> 1. nand_spl copies u-boot proper at a location in RAM
> 
> 2. nand_spl copies environment at another location in RAM
> 
> 3. nand_spl jumps to u-boot proper
> 
> 4. u-boot nullifies bss
> 
> 5. u-boot looks for environment - either embedded, or default, or copied 
> by nand_spl in 2.

Agreed. I don't see any mention of BSS in your description, though -
as expected.

> ideally you would tell nand_spl to put environment directly after bss, but 
> I haven't find a way (without adding some ugly scripts) to do this.

No, that is definitely not "ideally", that's most likely buggy, as it
messes with the memory map we're using so far.

> Notice, 1) I am not an expert in U-Boot environment operation, so, there 
> well might be issues I'm overseeing, 2) redundant environment is not 
> handled yet, 3) would be nice to hear comments from nand(_spl) 
> maintainers.

I don't see what nand(_spl) maintainers should comment here  -  their
code  is  so  far  completley  unaffected.  We just agreed on needing
another call to the load routine.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If A equals success, then the formula is A = X + Y + Z. X is work.  Y
is play. Z is keep your mouth shut.                 - Albert Einstein