On Sun, 31 Aug 2008, Wolfgang Denk wrote:

Dear Guennadi Liakhovetski,

In message Pine.LNX.4.64.0808271745000.6718@axis700.grange you wrote:

...

Use a union to cover both with and without redundant environment cases.

...

...

-typedef struct environment_s {

• ulong crc; /* CRC32 over data bytes */
• unsigned char flags; /* active or obsolete */
• char *data;

-} env_t; +/* This union will occupy exactly CFG_ENV_SIZE bytes. */ +union env_image {

• struct {

• uint32_t	crc;	/* CRC32 over data bytes    */
  

• char		data[];
  

• } single;
• struct {

• uint32_t	crc;	/* CRC32 over data bytes    */
  

• unsigned char	flags;	/* active or obsolete */
  

• char		data[];
  

• } redund;

+};

Hm... You defione this union in the context of tools/env/fw_env.c, while "include/environment.h" uses a different typedef. I think this is extremly error-prone because the connection between the environment-handling code in U-Boot and that in the external tool gets lost.

This union replaces the typedef env_t, which was also defined in fw_env.c. Thus I was not fixing the issue you describe above, which I fully agree with - the tool and u-boot should ideally use the same definition from a common header. I just did not address this issue in this patch series.

I think both sets of functions should use the same set of definitions (yes, I am aware that this requires chnages to "include/environ- ment.h").

Would be good, yes, the question is - do we want to do this now and do we want to do this in the scope of this patch series?

...

+struct environment {

• union env_image *image;
• char *data; /* shortcut to data */

+};

-static env_t environment; +static struct environment environment;

Omitting the typedef and then changing "env_t" into "struct environment" makes no sense to me. It just makes for less readable code and more typing.

Please stick with the typedef.

My understanding until now, that U-Boot follows the same coding style as the Linux kernel with only one exception - a space between a function name and the opening parenthesis. This is also stated here:

http://www.denx.de/wiki/U-Boot/CodingStyle

and Linux explicitly discourages typedef. They also produce warnings by checkpatch.sh. If this is also different in U-Boot, no problem, can change back.

...

• /* Update CRC */
• environment.crc = crc32 (0, (uint8_t*) environment.data, ENV_SIZE);

• /*

• * Update CRC: it is at the same location with and without the
  

• * redundant environment
  

• */
  

• environment.image->single.crc = crc32 (0, (uint8_t *) environment.data,

• 			       ENV_SIZE);
  

The comment is wrong. Without redundant environment, the CRC is at offset 4 from the start of the environment storage, and with redundancy it's at offset 5. This is definitely not the same location.

I think, CRC is always at offset 0, then follows the (optional) flag byte, and then comes the data. I also see this with binary dumps of the environment. Otherwise what takes the first four bytes? Or did you mean the data which CRC is calculated is at offset 4 or 5? I think, the comment is correct.

Also, the code does not match the commend, since you access "single.crc" which has no relation to redundant environment at all.

That's exactly the reason - the comment explains, that the crc is at the same location, so, you can access it using single.crc in both cases.

Looking at the rest of the code I see no improvement in using the union versus what we did before - the code is even longer now, and (slightly) less readable to me.

I need it to be able to copy the whole environment image, including the crc and the optional flag with one read / write / memcpy operation. And this is also an advantage even on NOR - no need for two or three syscalls, the whole image is read / written with one syscall.

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

On Sun, 31 Aug 2008, Wolfgang Denk wrote:

Dear Guennadi Liakhovetski,

In message Pine.LNX.4.64.0808271746001.6718@axis700.grange you wrote:

Your

...

Subject: Re: [U-Boot] [PATCH 2/6] Separate flash read and write operations

does not make a good commit comment - you should at least add "fw_env:" or similar so eveybody know which "flash read and write operations" you are talking about.

ok

...

The flash_io function was used for both read and write operations, whereby very little code was shared between the two modes. By breaking this function we simplify the code and save one level of identation.

If I see this correctly, you did not implement any functional changes or bug fixes, i. e. it was just a code restructuring because you didn't like the current style and preferred another one?

This also applies to my previous reply. I probably failed to explain this clearly in the introductory 0/6 email. The patches 1-5 are preparatory for the main "add NAND support" patch. Thus also separating read and write makes the NAND conversion much easier. The previous flash_io function was pretty long, implemented read and write and they share _very_ little code - only open and close IIRC. With NAND extra code would be added to both read and write parts and would make the whole function much longer and much less readable yet. Apart from that it would add many more indentation levels, believe me, this would be ugly.

So, no, this is not because I didn't like somebody else's coding style. This is because with NAND addition this function would become an absolutely unreadable monster. So, I would consider this patch a readability improvement.

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

On Sun, 31 Aug 2008, Wolfgang Denk wrote:

Dear Guennadi Liakhovetski,

In message Pine.LNX.4.64.0808311757460.3747@axis700.grange you wrote:

...

So, no, this is not because I didn't like somebody else's coding style. This is because with NAND addition this function would become an absolutely unreadable monster. So, I would consider this patch a readability improvement.

But you are duplicating code. It may be just 20 lines or so, but the better approach would be to leave the common code as is and factor out two new functions being called from the common code.

Yes, the original code was:

... flash_io(O_RDONLY); ... flash_io(O_RDONLY); ... flash_io(O_RDWR);

int flash_io(int mode) { fd = open(path, mode);

if (mode == O_RDONLY) { ... } else { ... }

close(fd); }

I changed it to

... flash_read(); ... flash_read(); ... flash_write(); ...

int flash_read(void) { fd = open(path, O_RDONLY);

...

close(fd); }

int flash_write(void) { fd = open(path, O_RDWR);

...

close(fd); }

yes, I thus duplicate "open" and "close". We could do

... flash_io(O_RDONLY); ... flash_io(O_RDONLY); ... flash_io(O_RDWR);

int flash_io(int mode) { fd = open(path, mode);

if (mode == O_RDONLY) { do_flash_read(fd); } else { do_flash_write(fd); }

close(fd); }

but, honestly, I prefer my version. If you disagree, I can change it to variant 3, no problem. This will mean redoing all patches 2-6 though...

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

On Sun, 31 Aug 2008, Wolfgang Denk wrote:

Dear Guennadi Liakhovetski,

In message Pine.LNX.4.64.0808271746380.6718@axis700.grange you wrote:

...

Signed-off-by: Guennadi Liakhovetski lg@denx.de

I don't see any need to change this code. Patch rejected.

return (0) and similar produce warnings from checkpatch.pl. If I followed this local style and used parenthesis in all returns I added, all of them would cause checkpatch warnings. If I only added returns without parenthesis the mixed style would look terrible. So, I consider this a coding style clean up, just as well as any space vs. tab, or brace on the same line as if or for or...

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

On Sun, 31 Aug 2008, Wolfgang Denk wrote:

Dear Guennadi Liakhovetski,

In message Pine.LNX.4.64.0808311805020.3747@axis700.grange you wrote:

...

...

I don't see any need to change this code. Patch rejected.

return (0) and similar produce warnings from checkpatch.pl. If I followed

Ah! Then you should have at least mentioned in your comments that this was the motivation of your changes. As you posted it, it looked just like a change because you didn't like the style.

Sorry, will do in the next version, which, as it seems, will be necessary.

...

this local style and used parenthesis in all returns I added, all of them would cause checkpatch warnings. If I only added returns without parenthesis the mixed style would look terrible. So, I consider this a coding style clean up, just as well as any space vs. tab, or brace on the same line as if or for or...

Frankly, I don't understand what checkpatch is warning about. IMO this is really just a matter f style, and I'm still using habits learned from K&R.

In my K&R second edition, which I just skipped over again, I haven't seen a single occurrence of "return (x)". No, I haven't checked every single return statement in the book, but I did come across a couple of dozens of them, they all just did "return x".

If you had learned C from the Unix version 6 code like me you'd most probbaly write "return (value);", too. (Just for the fun of it I checked if my memory is with me - in the whole Unix v7 code, kernel + libraries + commands, I could find just 18 cases where "return value;" was used.)

ic.

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

Dear Guennadi Liakhovetski,

In message Pine.LNX.4.64.0808271747130.6718@axis700.grange you wrote:

Use consistent naming for active and redundant environment variables, remove redundant erase struct initialisation by using separate structs for the active and redundant environments.

This is another sylistic change that is IMHO not justified by any functional improvements or code fixes.

If you are woking on other people's code you will have to accept that these other people have other preferences for variable names and the like. Reformatting the code and renaming variables just to make it better match your own style is IMHO not justified.

Your new code gets longer, you even have to split a couple of lines because of that. That's not an improvement to me.

I reject this patch.

Best regards,

Wolfgang Denk

Dear Guennadi Liakhovetski,

In message Pine.LNX.4.64.0808311810110.3747@axis700.grange you wrote:

It's not purely stylistic. For example, previously the code had fd and fdr, curdev and otherdev. It used one erase struct for both main and redundant copies, thus they had to initialise it multiple times to one or another version. I separated it into two erase_current and erase_target thus removing the need for multiple initialisation. I think, having

Ah! So there was a functional change (but - what was this needed for?), which esceaped me because it was buried across all those variable renamings.

dev_target, erase_target, fd_target vs. dev_current, erase_current and fd_current is also a readability improvement.

...

I reject this patch.

Please, reconsider.

I still reject it, at least as is. Whether you call the variables curdev and otherdev versus dev_current and dev_target makes no significant change to me, except that the new names are longer, more difficult to type and to read. And any functional changes become completely invisible among all the renaming. This makes such patches unacceptable to me.

It is important that you can actually SEE what a patch is changing. With your patches, this is not the case. You change everything, and the significant modifications become invisible.

Best regards,

Wolfgang Denk

Dear Guennadi Liakhovetski,

In message Pine.LNX.4.64.0808312118130.6742@axis700.grange you wrote:

Would it suffice to change the patch description for this patch to be accepted, or do you still want this patch to be dropped / changes? We could use fdcur, fdtrg, devcur, devtrg, erasecur, erasetrg to save the typing, but, personally, I find dev_current easier to read.

When I look at the code resulting after applying all your 6 patches, I see little remainings of the origial code. I wonder if it makes sense to claim that this is was an evolutionary change. Let's face the facts, it ain't so. There is old code, and there is new code, with some parts in common, but the other parts are replaced, not changed.

You can see this easily from the statistics:

- Your patch series which claims it was evolutionary changes adds to 1932 lines of patches. - Diffing just the original and the new version gives a patch of only 865 lines. - The old file had 778 lines of code, the new file has 954.

I suggest we no longer attempt to make this look like small changes, step by step. It ain't so. It is one big change which ressults in an old file being replaced by a new one.

Best regards,

Wolfgang Denk

Dear Guennadi Liakhovetski,

In message Pine.LNX.4.64.0808271747470.6718@axis700.grange you wrote:

This will become more important with NAND support, in which case the minimum erase region is a block, which consists of several pages and can be 256KiB large.

Please explain.

What does "anywhere" mean? At offset 0, 1, 5, 17 or 42? Or what? And what exactly is the "erase area" ?

And where's the difference between NAND and NOR flash? For NOR, the minimum "erase region" is a "block", either, which also can be 256KiB large.

• /*

• * Support environment anywhere within erase sectors: read out the
  

• * complete area to be erased, replace the environment image, write
  

• * the whole block back again.
  

• */
  

• if (DEVESIZE (dev_target) > CFG_ENV_SIZE) {

• data = malloc (DEVESIZE (dev_target));
  

• if (!data) {
  

• 	fprintf (stderr,
  

• 		 "Cannot malloc %lu bytes: %s\n",
  

• 		 DEVESIZE (dev_target),
  

• 		 strerror (errno));
  

• 	return -1;
  

• }
  

• rc = ioctl (fd_target, MEMGETINFO, &mtdinfo_target);
  

• if (rc < 0) {
  

• 	perror ("Cannot get MTD information");
  

• 	return -1;
  

• }
  

• /* Erase sector size is always a power of 2 */
  

• erase_offset = DEVOFFSET (dev_target) &
  

• 	~(mtdinfo_target.erasesize - 1);
  

• rc = flash_read_buf (dev_target, fd_target, data,
  

• 		     DEVESIZE (dev_target), erase_offset);
  

• if (rc < 0)
  

• 	return rc;
  

• /* Overwrite the old environment */
  

• memcpy(DEVOFFSET (dev_target) - erase_offset + data,
  

•        environment.image, CFG_ENV_SIZE);
  

• } else {

• data = (char *)environment.image;
  

• erase_offset = DEVOFFSET (dev_target);
  

• }

You are talking about "several pages" above. Where is this refelected in the code?

Frankly, I don't understand what you are trying to do. Please explain your implementation.

Best regards,

Wolfgang Denk

Dear Guennadi Liakhovetski,

In message Pine.LNX.4.64.0808312127560.6742@axis700.grange you wrote:

Erase area - this is all we erase, as opposed to one erase sector. On NOR this is limited by the environment size, on NAND by the number of blocks - the fifth parameter in the configuration file. This area may contain other useful data, which is first read in, then the whole area is erased, the environment is replaced in the read-in data, and it is written back - this is what I call the back-up process in the code.

We should not do that. We should erase only those sectors / blocks that we are actually attemmpting to write to.

...

And where's the difference between NAND and NOR flash? For NOR, the minimum "erase region" is a "block", either, which also can be 256KiB large.

This patch enables this for NOR - NAND support comes first with patch 6. So, it just enables placing the actual environment at any offset in the "erase area".

I don't understand. Why would such an offset be needed? The envrionment always starts right at the beginning of a sector or erase unit or block or however the storage device might call the smallest unit it can handle.

...

...

• /*

• * Support environment anywhere within erase sectors: read out the
  

• * complete area to be erased, replace the environment image, write
  

• * the whole block back again.
  

• */
  

This comment should actually serve as an explanation...

But I don't understand it. What are "erase sectors"? How is the "area to be erased" defined? And what is the "block" (a flash block?) you are writing?

...

You are talking about "several pages" above. Where is this refelected in the code?

You mean in the commit comment? There I am talking about the future code - NAND case, which is not yet in this patch.

How shall anybody understand this, then? Description and code are not in sync - this is bad.

...

Frankly, I don't understand what you are trying to do. Please explain your implementation.

Hope, it is a bit clearer now. If not, please ask, will try to explain again.

You must provide a description of what you are actually doing. I do not understand your code.

Indeed, this patch series changes the programme in a non-trivial way, that's why I had to split this "NAND-support" into several patches, still some of them seem to be not clear enough.

Well, splitting complex things that obviously belong together into smaller pieces and confronting the reviewer with a puzzle of unrelated bits does not exactly make things "clear".

See previous message - I guess the whole splitting is just contra- productive.

Best regards,

Wolfgang Denk

On Sun, 31 Aug 2008, Wolfgang Denk wrote:

Dear Guennadi Liakhovetski,

In message Pine.LNX.4.64.0808271741250.6718@axis700.grange you wrote:

...

What follows is a patch series to support NAND environment under Linux, including bad blocks. In principle, this is just one logical change, but it is a big one... So I split it into 6 smaller patches, which should be easier to review. Tested with and without redundant environment, with an injected bad block, crossing block border, read and write.

I just tried building for older kernels - it doesn't work:

-> make env MTD_VERSION=old make -C tools/env all MTD_VERSION=old || exit 1 make[1]: Entering directory `/home/wd/git/u-boot/tmp/tools/env' ppc_8xx-gcc -Wall -DUSE_HOSTCC -I/home/wd/git/u-boot/tmp/include -DMTD_OLD crc32.c fw_env.c fw_env_main.c -o fw_printenv In file included from /home/wd/git/u-boot/tmp/include/linux/mtd/mtd.h:13, from fw_env.c:36: /home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:15: error: parse error before "uint32_t" /home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:15: warning: no semicolon at end of struct or union /home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:16: warning: type defaults to `int' in declaration of `length' /home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:16: warning: data definition has no type or storage class

...

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?

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

On Sun, 31 Aug 2008, Wolfgang Denk wrote:

Dear Guennadi Liakhovetski,

In message Pine.LNX.4.64.0808312235130.6742@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@denx.de

Dear Guennadi Liakhovetski,

In message Pine.LNX.4.64.0809011129240.4686@axis700.grange you wrote:

This seems to be also the case in the mainline version, and fixing it would take a bit more than a oneliner - just including <config.h> produces a couple of "redefeined" warnings.

Hm. This used to work before. Can you bisect it?

Best regards,

Wolfgang Denk

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@denx.de

On Sun, 31 Aug 2008, Wolfgang Denk wrote:

Dear Guennadi Liakhovetski,

In message Pine.LNX.4.64.0808271741250.6718@axis700.grange you wrote:

...

What follows is a patch series to support NAND environment under Linux, including bad blocks. In principle, this is just one logical change, but it is a big one... So I split it into 6 smaller patches, which should be easier to review. Tested with and without redundant environment, with an injected bad block, crossing block border, read and write.

I just tried building for older kernels - it doesn't work:

-> make env MTD_VERSION=old

I am not sure if I am doing this right - maybe I have to point U-Boot to older kernel headers too, or it has to be done with an older tolchain, in any case, this doesn't seem to work also with the current mainline version:

$ make env MTD_VERSION=old make -C tools/env all MTD_VERSION=old || exit 1 make[1]: Entering directory `/home/lyakh/project/17/src/u-boot/tools/env' ppc_4xx-gcc -Wall -DUSE_HOSTCC -I/home/lyakh/project/17/src/u-boot/include -DMTD_OLD crc32.c fw_env.c fw_env_main.c -o fw_printenv In file included from /home/lyakh/project/17/src/u-boot/include/linux/mtd/mtd.h:13, from fw_env.c:36: /home/lyakh/project/17/src/u-boot/include/linux/mtd/mtd-abi.h:15: error: expected specifier-qualifier-list before 'uint32_t' /home/lyakh/project/17/src/u-boot/include/linux/mtd/mtd-abi.h:20: error: expected specifier-qualifier-list before 'uint32_t' /home/lyakh/project/17/src/u-boot/include/linux/mtd/mtd-abi.h:57: error: expected specifier-qualifier-list before 'uint8_t' /home/lyakh/project/17/src/u-boot/include/linux/mtd/mtd-abi.h:70: error: expected specifier-qualifier-list before 'uint32_t' /home/lyakh/project/17/src/u-boot/include/linux/mtd/mtd-abi.h:78: error: expected specifier-qualifier-list before 'uint32_t' /home/lyakh/project/17/src/u-boot/include/linux/mtd/mtd-abi.h:108: error: expected specifier-qualifier-list before 'uint32_t' /home/lyakh/project/17/src/u-boot/include/linux/mtd/mtd-abi.h:115: error: expected specifier-qualifier-list before 'uint32_t' /home/lyakh/project/17/src/u-boot/include/linux/mtd/mtd-abi.h:125: error: expected specifier-qualifier-list before 'uint32_t' /home/lyakh/project/17/src/u-boot/include/linux/mtd/mtd-abi.h:140: error: expected specifier-qualifier-list before 'uint32_t' In file included from fw_env.c:36: /home/lyakh/project/17/src/u-boot/include/linux/mtd/mtd.h:107: error: expected specifier-qualifier-list before 'uint32_t' fw_env.c: In function 'fw_setenv': fw_env.c:386: error: 'uint8_t' undeclared (first use in this function) fw_env.c:386: error: (Each undeclared identifier is reported only once fw_env.c:386: error: for each function it appears in.) fw_env.c:386: error: expected expression before ')' token fw_env.c:386: error: too few arguments to function 'crc32' fw_env.c: In function 'flash_io': fw_env.c:400: error: 'erase_info_t' undeclared (first use in this function) fw_env.c:400: error: expected ';' before 'erase' fw_env.c:431: error: 'erase' undeclared (first use in this function) fw_env.c: In function 'env_init': fw_env.c:621: error: 'uint8_t' undeclared (first use in this function) fw_env.c:621: error: expected expression before ')' token fw_env.c:621: error: too few arguments to function 'crc32' fw_env.c:645: error: expected expression before ')' token fw_env.c:645: error: too few arguments to function 'crc32' make[1]: *** [fw_printenv] Error 1 make[1]: Leaving directory `/home/lyakh/project/17/src/u-boot/tools/env' make: *** [env] Error 1

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