Dear Guennadi Liakhovetski, In message <pine.lnx.4.64.0902022310070.8...@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: w...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot