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

[Guennadi Liakhovetski]

Morning Wolfgang

On Tue, 3 Feb 2009, Wolfgang Denk wrote:

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

for (1) - "valid copy" - you're not suggesting to calculate crc32 in 
nand_spl to decide upon env validity, are you? I think, we should add as 
little as possible to nand_spl, remember, it has to fit in a very limited 
size like 16, or 4, or even 2K. So, probably, just add one (or two for 
redundant env) nand_load calls and let env_init() evaluate them? Or is 
this what you meant?

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

Correct me if I'm wrong, but I think code like

int env_init(void)
{
	/* SPI flash isn't usable before relocation */
	gd->env_addr = (ulong)&default_environment[0];
	gd->env_valid = 1;

	return 0;
}

does mean the specific environment limits itself to default until 
env_relocate()? Then, this snippet os from env_sf.c, env_onenand.c does 
exactly the same, env_nand.c if env is not embedded. The rest are actually 
reading the medium.

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

Why? Maybe there are users out there ATM that have environment in NAND and 
boot from another storage and are reasonably happy being only able to 
affect late configuration dynamically (e.g., Linux boot procedure) and 
having to recompile u-boot to change early configuration (console 
baudrate, etc.)?

> > > > > >  #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.

This all sounds good, but, as I see it, this memory map, and the location 
of the env copies read out by nand_spl are platform-dependent, so, every 
implementor will just have to take care of the specific configuration. 
besides, this location only has to survive until env_relocate(), where it 
anyway will be copied to a malloc()ed buffer. Although, if we reuse the 
ENV_IS_EMBEDDED case then we can keep the buffers and avoid the extra 
malloc() / memcpy(). But documenting is always good, sure:-)

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

in 4. - as a reason why you have to skip it.

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

We still have the other possibility - properly initialise NAND before 
calling env_init() and then read out env? This would be a preferred 
solution IMHO, because it leverages restrictions like "only the boot 
NAND," simplifies the choice of a location in RAM, leaves nand_spl code 
intact, thus avoiding even minimal size increment. NAND maintainer's 
opinion as to how feasible / difficult this would be would be appreciated. 
Or even better - a patch:-) I think, we could even move mem_malloc_init() 
before the initcall loop and use a malloc()ed buffer just as 
env_relocate() would do.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de