On Tue, 2 Sep 2008, Wolfgang Denk wrote:
In message Pine.LNX.4.64.0809011028550.4686@axis700.grange you wrote:
- 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 ... ).
- 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.
- 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?
- 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@denx.de