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

[Guennadi Liakhovetski]

On Sun, 31 Aug 2008, Wolfgang Denk wrote:

> Dear Guennadi Liakhovetski,
> 
> In message <Pine.LNX.4.64.0808312235130.6742 at axis700.grange> you wrote:
> > 
> > Ok, how about this: we leave the current fw_env.c as it is, I submit 
> > _exactly_ the state after applying my 6 patches as a new file, with 
> > suitable changes to the Makefile, fix building with MTD_VERSION=old, and 
> > try to improve comments in the code. Would this be accepted?
> 
> Just improvements of comments? I think there were comments about code
> as well. These shall be addressed, too.

Ok, I looked through all your comments today, and this is what I extracted 
from all of them as code improvement suggestions (sorry if I missed 
anything, please remind):

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:
 a) env_t in environment.h does indeed describe the environment layout on 
    the media, but, this is only possible thanks to the compile-time 
    decisions whether or not to include flags and how long is data.
 b) it wasn't done before, env_t in the original tool was not equivalent 
    with the environment.h definition: it couldn't decide at compile time 
    whether or not to have flags, so, the struct always included it, and 
    it had "char *data" in it, which is not the same as "char data[...]", 
    so you couldn't cast this type on the environment image.
 c) Now the union I proposed does describe the environment layout, can be 
    casted on the image, and I really don't see another clean way of doing 
    this.

2. do not use single.crc in redundant case

This is done only at two places, yes, I realise, this is not very clean, 
but in the present configuration with a comment explaining why this is 
correct, I would consider this acceptable. Otherwise, I could either 
introducs a reference to crc in struct environment similar to the pointer 
to data, or I could open code using the correct - single or redundant - 
crc at these two occasions, which, of course, is actually not needed.

3. use a type equivalent with the one from the environment.h header

See 1. above.

4. fix MTD_OLD

Would we still need this with NAND-only tool?

5. clarify back-up mode

This is actually a comment improvement, can do.

6. erase / write only as much as needed

Yes, this is important, I'll have a look at it.

7. in flash_bad_blocks do not modify blockstart

Can do this, good point.

Have I missed anything? Would the current v2 version _with_ these changes 
be accepted?

A couple more questions then:

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 
would not significantly simplify or reduce the size of the nand-version. 
In fact, I still think having one tool support both might be preferable 
from the maintenance point of view - all fixes, improvements, changes, 
that affect both versions would only have to be done once... So, maybe if 
we now add a new tool, which supports both, after we have sufficiently 
tested it, we could remove the original one?

How shall I name it? nand_env?

> New issue: I just noted that the default environment built  into  the
> fw_  tool  has not much to do with the default environment build into
> the U-Boot binary image; in theory both should  be  the  same.  Don;t
> know yet if this is a new or an old bug, though.

Will have a look.

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