[U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv

[Guennadi Liakhovetski <lg@denx.de>]

On Tue, 2 Sep 2008, Wolfgang Denk wrote:

> In message <Pine.LNX.4.64.0809011028550.4686@axis700.grange> you wrote:
> > 
> > 1. do not use the union
> > 
> > well, I would still prefer to use it and I hope I will be allowed to do so 
> > in a separate NAND-tool. I agree, it would be better to use the definition 
> > from the environment.h directly. But:
> 
> There is no such thing as a separate NAND tool - this mkes zero sense.
> There shall be one tool that supports both NOR and NAND (and soon
> probably DataFlash and OneNAND and ... ).
> 
> > 2. do not use single.crc in redundant case
> > 
> > This is done only at two places, yes, I realise, this is not very clean, 
> 
> Indeed. But the problem goes away automatically whenyou get rid of the
> union.

I'll try to explain again _why_ i introduced the union and why I still 
don't see a good replacement for it.

As you know, in the tool we have to decide at run-time whether we are 
dealing  with a single environment copy or with current / redundant 
configuration. With NAND support when _writing_ environment to NAND you 
have to write page at a time, which means, at this point we _must_ have 
the image in a contiguous buffer in RAM. And the image can have one of the 
two possible formats - with and without the "flags" byte. In principle I 
see only three possibilities to implement this:

(a) work with arbitrary non-contiguous data in RAM as before, and copy it 
into an additional buffer just before writing to NAND

advantage: can keep the current struct

disadvantage: extra malloc

(b) use a plain data buffer, and, if needed, use the first byte in it for 
flags

advantage: no extra malloc, no (explicit) union

disadvantage: confusing, have to work with byte-offsets instead of struct 
/ union members, and, in fact, this is the same as using a union, just 
implicit, calculating byte-offsets manually, instead of letting the 
compiler do it

(c) use a union

advantage: clean access to all environment fields without the use of 
byte-offsets

disadvantage: slightly more complex code

Please, just tell me which of these three you would prefer, or maybe there 
is a fourth possibility I am still overseeing.

You also asked about the extra "char *data" pointer in the struct 
environment, whether there is no danger that a different compiler version 
will break it. This pointer uses no magic - it is just a plain simple 
pointer, I use it to point to data inside the union to avoid having to 
check every time whether we have the redundant environment or not. So, I 
check it only once at initialisation time, set this pointer, and then just 
use it to access the data buffer inside the image (union). No magic here.

> > 4. fix MTD_OLD
> > 
> > Would we still need this with NAND-only tool?
> 
> Yes of course we need it. I will not accept such thing as a NAND-only tool.

Ok. But NAND-support is not needed with MTD_OLD? So, if it cannot be 
compiled with older kernels, we may just disable it per ifdef?

> > 5. clarify back-up mode
> > 
> > This is actually a comment improvement, can do.
> 
> Actually the whole implementation needs to be explained.

Ok.

> > Shall I keep support for NOR in the separate NAND version or completely 
> > remove it? The "type == MTD_NORFLASH" code is quite small, so, removing it 
> 
> I don't understand why you come up with such an idea. There shall  be
> just  the  one  tool we have now, just with extended functionality. I
> just wanted to get rid of the futile attempts to make  the  one  huge
> change looking like a series af several big but incremental changes.

How would you like to make such a replacement then? If I produce a patch 
just from the current state to the final state, I think, it will look 
worse than the broken-down patch-series. Otherwise we could remove the 
current file and add a new one in two patches? This wouldn't be very good 
either - you'd have to change Makefiles etc. to keep the tree compilable.

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