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

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

[Guennadi Liakhovetski]

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.

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

[U-Boot] [PATCH 1/6] Convert fw_env.c to use a single environment image union

[Guennadi Liakhovetski]

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

Signed-off-by: Guennadi Liakhovetski <lg@denx.de>
---
 tools/env/fw_env.c |  148 +++++++++++++++++++++++++++++-----------------------
 1 files changed, 82 insertions(+), 66 deletions(-)

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index b8bca91..35783c5 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -63,16 +63,30 @@ static int curdev;
 
 #define ENV_SIZE      getenvsize()
 
-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;
+};
+
+struct environment {
+	union env_image	*image;
+	char		*data;	/* shortcut to data */
+};
 
-static env_t environment;
+static struct environment environment;
 
 static int HaveRedundEnv = 0;
 
+#define ENV_FLAGS(e) e.image->redund.flags
+
 static unsigned char active_flag = 1;
 static unsigned char obsolete_flag = 0;
 
@@ -156,7 +170,7 @@ static char default_environment[] = {
 #ifdef  CONFIG_EXTRA_ENV_SETTINGS
 	CONFIG_EXTRA_ENV_SETTINGS
 #endif
-	"\0"			/* Termimate env_t data with 2 NULs */
+	"\0"			/* Termimate struct environment data with 2 NULs */
 };
 
 static int flash_io (int mode);
@@ -382,8 +396,12 @@ int fw_setenv (int argc, char *argv[])
 
   WRITE_FLASH:
 
-	/* 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);
 
 	/* write environment back to flash */
 	if (flash_io (O_RDWR)) {
@@ -396,7 +414,7 @@ int fw_setenv (int argc, char *argv[])
 
 static int flash_io (int mode)
 {
-	int fd, fdr, rc, otherdev, len, resid;
+	int fd, fdr, rc, otherdev, resid;
 	erase_info_t erase;
 	char *data = NULL;
 
@@ -407,11 +425,6 @@ static int flash_io (int mode)
 		return (-1);
 	}
 
-	len = sizeof (environment.crc);
-	if (HaveRedundEnv) {
-		len += sizeof (environment.flags);
-	}
-
 	if (mode == O_RDWR) {
 		if (HaveRedundEnv) {
 			/* switch to next partition for writing */
@@ -436,7 +449,7 @@ static int flash_io (int mode)
 			erase.length = DEVESIZE (curdev);
 			erase.start = DEVOFFSET (curdev);
 			ioctl (fd, MEMUNLOCK, &erase);
-			environment.flags = active_flag;
+			ENV_FLAGS(environment) = active_flag;
 		}
 
 		printf ("Done\n");
@@ -485,18 +498,15 @@ static int flash_io (int mode)
 				DEVNAME (otherdev), strerror (errno));
 			return (-1);
 		}
-		if (write (fdr, &environment, len) != len) {
-			fprintf (stderr,
-				"CRC write error on %s: %s\n",
-				DEVNAME (otherdev), strerror (errno));
-			return (-1);
-		}
-		if (write (fdr, environment.data, ENV_SIZE) != ENV_SIZE) {
+
+		if (write (fdr, environment.image, CFG_ENV_SIZE) !=
+		    CFG_ENV_SIZE) {
 			fprintf (stderr,
 				"Write error on %s: %s\n",
 				DEVNAME (otherdev), strerror (errno));
 			return (-1);
 		}
+
 		if (resid) {
 			if (write (fdr, data, resid) != resid) {
 				fprintf (stderr,
@@ -548,13 +558,8 @@ static int flash_io (int mode)
 				DEVNAME (curdev), strerror (errno));
 			return (-1);
 		}
-		if (read (fd, &environment, len) != len) {
-			fprintf (stderr,
-				"CRC read error on %s: %s\n",
-				DEVNAME (curdev), strerror (errno));
-			return (-1);
-		}
-		if ((rc = read (fd, environment.data, ENV_SIZE)) != ENV_SIZE) {
+		if (read (fd, environment.image, CFG_ENV_SIZE) !=
+		    CFG_ENV_SIZE) {
 			fprintf (stderr,
 				"Read error on %s: %s\n",
 				DEVNAME (curdev), strerror (errno));
@@ -604,22 +609,24 @@ static int env_init (void)
 	if (parse_config ())		/* should fill envdevices */
 		return 1;
 
-	if ((addr1 = calloc (1, ENV_SIZE)) == NULL) {
+	if ((addr1 = calloc (1, CFG_ENV_SIZE)) == NULL) {
 		fprintf (stderr,
 			"Not enough memory for environment (%ld bytes)\n",
-			ENV_SIZE);
+			CFG_ENV_SIZE);
 		return (errno);
 	}
 
 	/* read environment from FLASH to local buffer */
-	environment.data = addr1;
+	environment.image = (union env_image *)addr1;
+	environment.data = HaveRedundEnv ? environment.image->redund.data :
+		environment.image->single.data;
 	curdev = 0;
 	if (flash_io (O_RDONLY)) {
 		return (errno);
 	}
 
-	crc1_ok = ((crc1 = crc32 (0, (uint8_t *) environment.data, ENV_SIZE))
-			   == environment.crc);
+	crc1 = crc32 (0, (uint8_t *) environment.data, ENV_SIZE);
+	crc1_ok = (crc1 == environment.image->single.crc);
 	if (!HaveRedundEnv) {
 		if (!crc1_ok) {
 			fprintf (stderr,
@@ -627,74 +634,83 @@ static int env_init (void)
 			memcpy(environment.data, default_environment, sizeof default_environment);
 		}
 	} else {
-		flag1 = environment.flags;
+		flag1 = ENV_FLAGS(environment);
 
 		curdev = 1;
-		if ((addr2 = calloc (1, ENV_SIZE)) == NULL) {
+		if ((addr2 = calloc (1, CFG_ENV_SIZE)) == NULL) {
 			fprintf (stderr,
 				"Not enough memory for environment (%ld bytes)\n",
-				ENV_SIZE);
+				CFG_ENV_SIZE);
 			return (errno);
 		}
-		environment.data = addr2;
+		environment.image = (union env_image *)addr2;
 
 		if (flash_io (O_RDONLY)) {
 			return (errno);
 		}
 
-		crc2_ok = ((crc2 = crc32 (0, (uint8_t *) environment.data, ENV_SIZE))
-				   == environment.crc);
-		flag2 = environment.flags;
+		crc2 = crc32 (0, (uint8_t *) environment.image->redund.data,
+			      ENV_SIZE);
+		crc2_ok = (crc2 == environment.image->redund.crc);
+		flag2 = ENV_FLAGS(environment);
 
 		if (crc1_ok && !crc2_ok) {
-			environment.data = addr1;
-			environment.flags = flag1;
-			environment.crc = crc1;
+			environment.image = (union env_image *)addr1;
+			ENV_FLAGS(environment) = flag1;
+			environment.image->redund.crc = crc1;
 			curdev = 0;
 			free (addr2);
 		} else if (!crc1_ok && crc2_ok) {
-			environment.data = addr2;
-			environment.flags = flag2;
-			environment.crc = crc2;
+			environment.image = (union env_image *)addr2;
+			ENV_FLAGS(environment) = flag2;
+			environment.image->redund.crc = crc2;
 			curdev = 1;
 			free (addr1);
 		} else if (!crc1_ok && !crc2_ok) {
 			fprintf (stderr,
 				"Warning: Bad CRC, using default environment\n");
-			memcpy(environment.data, default_environment, sizeof default_environment);
+			memcpy(environment.image->redund.data,
+			       default_environment, sizeof default_environment);
 			curdev = 0;
 			free (addr1);
+		/* From here: both CRCs correct */
 		} else if (flag1 == active_flag && flag2 == obsolete_flag) {
-			environment.data = addr1;
-			environment.flags = flag1;
-			environment.crc = crc1;
+			environment.image = (union env_image *)addr1;
+			ENV_FLAGS(environment) = flag1;
+			environment.image->redund.crc = crc1;
 			curdev = 0;
 			free (addr2);
 		} else if (flag1 == obsolete_flag && flag2 == active_flag) {
-			environment.data = addr2;
-			environment.flags = flag2;
-			environment.crc = crc2;
+			environment.image = (union env_image *)addr2;
+			ENV_FLAGS(environment) = flag2;
+			environment.image->redund.crc = crc2;
 			curdev = 1;
 			free (addr1);
+		/* From here: invalid flag configuration */
 		} else if (flag1 == flag2) {
-			environment.data = addr1;
-			environment.flags = flag1;
-			environment.crc = crc1;
+			environment.image = (union env_image *)addr1;
+			ENV_FLAGS(environment) = flag1;
+			environment.image->redund.crc = crc1;
 			curdev = 0;
 			free (addr2);
-		} else if (flag1 == 0xFF) {
-			environment.data = addr1;
-			environment.flags = flag1;
-			environment.crc = crc1;
+		} else if (flag1 == obsolete_flag || flag1 == active_flag) {
+			/* flag1 valid, update flag2 */
+			environment.image = (union env_image *)addr1;
+			ENV_FLAGS(environment) = flag1;
+			environment.image->redund.crc = crc1;
 			curdev = 0;
 			free (addr2);
-		} else if (flag2 == 0xFF) {
-			environment.data = addr2;
-			environment.flags = flag2;
-			environment.crc = crc2;
+		} else {
+			if (flag2 != obsolete_flag && flag2 != active_flag)
+				fprintf (stderr, "Both CRCs valid, but both "
+					"flags invalid, will use the 1st\n");
+			environment.image = (union env_image *)addr2;
+			ENV_FLAGS(environment) = flag2;
+			environment.image->redund.crc = crc2;
 			curdev = 1;
 			free (addr1);
 		}
+		environment.data = environment.image->redund.data;
 	}
 	return (0);
 }
-- 
1.5.4

[U-Boot] [PATCH 1/6] Convert fw_env.c to use a single environment image union

[Wolfgang Denk]

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.

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").

> +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.

>  static int HaveRedundEnv = 0;
>  
> +#define ENV_FLAGS(e) e.image->redund.flags
> +
>  static unsigned char active_flag = 1;
>  static unsigned char obsolete_flag = 0;
>  
> @@ -156,7 +170,7 @@ static char default_environment[] = {
>  #ifdef  CONFIG_EXTRA_ENV_SETTINGS
>  	CONFIG_EXTRA_ENV_SETTINGS
>  #endif
> -	"\0"			/* Termimate env_t data with 2 NULs */
> +	"\0"			/* Termimate struct environment data with 2 NULs */
>  };
>  
>  static int flash_io (int mode);
> @@ -382,8 +396,12 @@ int fw_setenv (int argc, char *argv[])
>  
>    WRITE_FLASH:
>  
> -	/* 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.

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


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.

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: wd at 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] [PATCH 1/6] Convert fw_env.c to use a single environment image union

[Guennadi Liakhovetski]

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

[U-Boot] [PATCH 1/6] Convert fw_env.c to use a single environment image union

[Wolfgang Denk]

Dear Guennadi Liakhovetski,

In message <Pine.LNX.4.64.0808311740070.3747@axis700.grange> you wrote:
> 
> 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.

This is not correct. You are creating this issue. So far, U-Boot and
fw_env used to use the same (or at least equivalent) definitions.

And I have to admit that I'm not a friend of using unions. They are a
great way to obfuscate code.

> > 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?

If there is no real need to change it now, then leave it unchanged?

> > Please stick with the typedef.
...
> 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.

Ah! And why didn't you mention this in the patch?

For a reviewer it is very important to understand why you are
modifying code.


> > > -	/* 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, 

Yes, you are right.

> 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.

And we see the obfuscation caused by using a union.

> > 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.

This is ugly, plain ugly.

> > 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 

Hm... why do you need to do this in a single read operation?  Where's
the  difference  between reading it all at once or in several smaller
chunks?

> this is also an advantage even on NOR - no need for two or three syscalls, 
> the whole image is read / written with one syscall.

A big win, really ;-)

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: wd at denx.de
How long does it take a  DEC  field  service  engineer  to  change  a
lightbulb?       It depends on how many bad ones he brought with him.

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

[Guennadi Liakhovetski]

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.

Signed-off-by: Guennadi Liakhovetski <lg@denx.de>
---
 tools/env/fw_env.c |  280 ++++++++++++++++++++++++++++------------------------
 1 files changed, 149 insertions(+), 131 deletions(-)

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index 35783c5..6f7fdb2 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -173,7 +173,8 @@ static char default_environment[] = {
 	"\0"			/* Termimate struct environment data with 2 NULs */
 };
 
-static int flash_io (int mode);
+static int flash_write (void);
+static int flash_read (void);
 static char *envmatch (char * s1, char * s2);
 static int env_init (void);
 static int parse_config (void);
@@ -404,7 +405,7 @@ int fw_setenv (int argc, char *argv[])
 					       ENV_SIZE);
 
 	/* write environment back to flash */
-	if (flash_io (O_RDWR)) {
+	if (flash_write ()) {
 		fprintf (stderr, "Error: can't write fw_env to flash\n");
 		return (-1);
 	}
@@ -412,165 +413,182 @@ int fw_setenv (int argc, char *argv[])
 	return (0);
 }
 
-static int flash_io (int mode)
+static int flash_write (void)
 {
 	int fd, fdr, rc, otherdev, resid;
 	erase_info_t erase;
 	char *data = NULL;
 
-	if ((fd = open (DEVNAME (curdev), mode)) < 0) {
+	if ((fd = open (DEVNAME (curdev), O_RDWR)) < 0) {
 		fprintf (stderr,
-			"Can't open %s: %s\n",
-			DEVNAME (curdev), strerror (errno));
+			 "Can't open %s: %s\n",
+			 DEVNAME (curdev), strerror (errno));
 		return (-1);
 	}
 
-	if (mode == O_RDWR) {
-		if (HaveRedundEnv) {
-			/* switch to next partition for writing */
-			otherdev = !curdev;
-			if ((fdr = open (DEVNAME (otherdev), mode)) < 0) {
-				fprintf (stderr,
-					"Can't open %s: %s\n",
-					DEVNAME (otherdev),
-					strerror (errno));
-				return (-1);
-			}
-		} else {
-			otherdev = curdev;
-			fdr = fd;
-		}
-		printf ("Unlocking flash...\n");
-		erase.length = DEVESIZE (otherdev);
-		erase.start = DEVOFFSET (otherdev);
-		ioctl (fdr, MEMUNLOCK, &erase);
-
-		if (HaveRedundEnv) {
-			erase.length = DEVESIZE (curdev);
-			erase.start = DEVOFFSET (curdev);
-			ioctl (fd, MEMUNLOCK, &erase);
-			ENV_FLAGS(environment) = active_flag;
+	if (HaveRedundEnv) {
+		/* switch to next partition for writing */
+		otherdev = !curdev;
+		if ((fdr = open (DEVNAME (otherdev), O_RDWR)) < 0) {
+			fprintf (stderr,
+				 "Can't open %s: %s\n",
+				 DEVNAME (otherdev),
+				 strerror (errno));
+			return (-1);
 		}
+	} else {
+		otherdev = curdev;
+		fdr = fd;
+	}
+	printf ("Unlocking flash...\n");
+	erase.length = DEVESIZE (otherdev);
+	erase.start = DEVOFFSET (otherdev);
+	ioctl (fdr, MEMUNLOCK, &erase);
+
+	if (HaveRedundEnv) {
+		erase.length = DEVESIZE (curdev);
+		erase.start = DEVOFFSET (curdev);
+		ioctl (fd, MEMUNLOCK, &erase);
+		ENV_FLAGS(environment) = active_flag;
+	}
 
-		printf ("Done\n");
-		resid = DEVESIZE (otherdev) - CFG_ENV_SIZE;
-		if (resid) {
-			if ((data = malloc (resid)) == NULL) {
-				fprintf (stderr,
-					"Cannot malloc %d bytes: %s\n",
-					resid,
-					strerror (errno));
-				return (-1);
-			}
-			if (lseek (fdr, DEVOFFSET (otherdev) + CFG_ENV_SIZE, SEEK_SET)
-				== -1) {
-				fprintf (stderr, "seek error on %s: %s\n",
-					DEVNAME (otherdev),
-					strerror (errno));
-				return (-1);
-			}
-			if ((rc = read (fdr, data, resid)) != resid) {
-				fprintf (stderr,
-					"read error on %s: %s\n",
-					DEVNAME (otherdev),
-					strerror (errno));
-				return (-1);
-			}
+	printf ("Done\n");
+	resid = DEVESIZE (otherdev) - CFG_ENV_SIZE;
+	if (resid) {
+		if ((data = malloc (resid)) == NULL) {
+			fprintf (stderr,
+				 "Cannot malloc %d bytes: %s\n",
+				 resid,
+				 strerror (errno));
+			return (-1);
 		}
-
-		printf ("Erasing old environment...\n");
-
-		erase.length = DEVESIZE (otherdev);
-		erase.start = DEVOFFSET (otherdev);
-		if (ioctl (fdr, MEMERASE, &erase) != 0) {
-			fprintf (stderr, "MTD erase error on %s: %s\n",
-				DEVNAME (otherdev),
-				strerror (errno));
+		if (lseek (fdr, DEVOFFSET (otherdev) + CFG_ENV_SIZE, SEEK_SET)
+		    == -1) {
+			fprintf (stderr, "seek error on %s: %s\n",
+				 DEVNAME (otherdev),
+				 strerror (errno));
 			return (-1);
 		}
-
-		printf ("Done\n");
-
-		printf ("Writing environment to %s...\n", DEVNAME (otherdev));
-		if (lseek (fdr, DEVOFFSET (otherdev), SEEK_SET) == -1) {
+		if ((rc = read (fdr, data, resid)) != resid) {
 			fprintf (stderr,
-				"seek error on %s: %s\n",
-				DEVNAME (otherdev), strerror (errno));
+				 "read error on %s: %s\n",
+				 DEVNAME (otherdev),
+				 strerror (errno));
 			return (-1);
 		}
+	}
+
+	printf ("Erasing old environment...\n");
+
+	erase.length = DEVESIZE (otherdev);
+	erase.start = DEVOFFSET (otherdev);
+	if (ioctl (fdr, MEMERASE, &erase) != 0) {
+		fprintf (stderr, "MTD erase error on %s: %s\n",
+			 DEVNAME (otherdev),
+			 strerror (errno));
+		return (-1);
+	}
+
+	printf ("Done\n");
+
+	printf ("Writing environment to %s...\n", DEVNAME (otherdev));
+	if (lseek (fdr, DEVOFFSET (otherdev), SEEK_SET) == -1) {
+		fprintf (stderr,
+			 "seek error on %s: %s\n",
+			 DEVNAME (otherdev), strerror (errno));
+		return (-1);
+	}
+
+	if (write (fdr, environment.image, CFG_ENV_SIZE) != CFG_ENV_SIZE) {
+		fprintf (stderr,
+			 "Write error on %s: %s\n",
+			 DEVNAME (otherdev), strerror (errno));
+		return (-1);
+	}
 
-		if (write (fdr, environment.image, CFG_ENV_SIZE) !=
-		    CFG_ENV_SIZE) {
+	if (resid) {
+		if (write (fdr, data, resid) != resid) {
 			fprintf (stderr,
-				"Write error on %s: %s\n",
-				DEVNAME (otherdev), strerror (errno));
+				 "write error on %s: %s\n",
+				 DEVNAME (curdev), strerror (errno));
 			return (-1);
 		}
-
-		if (resid) {
-			if (write (fdr, data, resid) != resid) {
-				fprintf (stderr,
-					"write error on %s: %s\n",
-					DEVNAME (curdev), strerror (errno));
-				return (-1);
-			}
-			free (data);
-		}
-		if (HaveRedundEnv) {
-			/* change flag on current active env partition */
-			if (lseek (fd, DEVOFFSET (curdev) + sizeof (ulong), SEEK_SET)
-				== -1) {
-				fprintf (stderr, "seek error on %s: %s\n",
-					DEVNAME (curdev), strerror (errno));
-				return (-1);
-			}
-			if (write (fd, &obsolete_flag, sizeof (obsolete_flag)) !=
-				sizeof (obsolete_flag)) {
-				fprintf (stderr,
-					"Write error on %s: %s\n",
-					DEVNAME (curdev), strerror (errno));
-				return (-1);
-			}
-		}
-		printf ("Done\n");
-		printf ("Locking ...\n");
-		erase.length = DEVESIZE (otherdev);
-		erase.start = DEVOFFSET (otherdev);
-		ioctl (fdr, MEMLOCK, &erase);
-		if (HaveRedundEnv) {
-			erase.length = DEVESIZE (curdev);
-			erase.start = DEVOFFSET (curdev);
-			ioctl (fd, MEMLOCK, &erase);
-			if (close (fdr)) {
-				fprintf (stderr,
-					"I/O error on %s: %s\n",
-					DEVNAME (otherdev),
-					strerror (errno));
-				return (-1);
-			}
+		free (data);
+	}
+	if (HaveRedundEnv) {
+		/* change flag on current active env partition */
+		if (lseek (fd, DEVOFFSET (curdev) + sizeof (ulong), SEEK_SET)
+		    == -1) {
+			fprintf (stderr, "seek error on %s: %s\n",
+				 DEVNAME (curdev), strerror (errno));
+			return (-1);
 		}
-		printf ("Done\n");
-	} else {
-
-		if (lseek (fd, DEVOFFSET (curdev), SEEK_SET) == -1) {
+		if (write (fd, &obsolete_flag, sizeof (obsolete_flag)) !=
+		    sizeof (obsolete_flag)) {
 			fprintf (stderr,
-				"seek error on %s: %s\n",
-				DEVNAME (curdev), strerror (errno));
+				 "Write error on %s: %s\n",
+				 DEVNAME (curdev), strerror (errno));
 			return (-1);
 		}
-		if (read (fd, environment.image, CFG_ENV_SIZE) !=
-		    CFG_ENV_SIZE) {
+	}
+	printf ("Done\n");
+	printf ("Locking ...\n");
+	erase.length = DEVESIZE (otherdev);
+	erase.start = DEVOFFSET (otherdev);
+	ioctl (fdr, MEMLOCK, &erase);
+	if (HaveRedundEnv) {
+		erase.length = DEVESIZE (curdev);
+		erase.start = DEVOFFSET (curdev);
+		ioctl (fd, MEMLOCK, &erase);
+		if (close (fdr)) {
 			fprintf (stderr,
-				"Read error on %s: %s\n",
-				DEVNAME (curdev), strerror (errno));
+				 "I/O error on %s: %s\n",
+				 DEVNAME (otherdev),
+				 strerror (errno));
 			return (-1);
 		}
 	}
+	printf ("Done\n");
+
+	if (close (fd)) {
+		fprintf (stderr,
+			 "I/O error on %s: %s\n",
+			 DEVNAME (curdev), strerror (errno));
+		return (-1);
+	}
+
+	/* everything ok */
+	return (0);
+}
+
+static int flash_read (void)
+{
+	int fd;
+
+	if ((fd = open (DEVNAME (curdev), O_RDONLY)) < 0) {
+		fprintf (stderr,
+			 "Can't open %s: %s\n",
+			 DEVNAME (curdev), strerror (errno));
+		return (-1);
+	}
+
+	if (lseek (fd, DEVOFFSET (curdev), SEEK_SET) == -1) {
+		fprintf (stderr,
+			 "seek error on %s: %s\n",
+			 DEVNAME (curdev), strerror (errno));
+		return (-1);
+	}
+	if (read (fd, environment.image, CFG_ENV_SIZE) != CFG_ENV_SIZE) {
+		fprintf (stderr,
+			 "Read error on %s: %s\n",
+			 DEVNAME (curdev), strerror (errno));
+		return (-1);
+	}
 
 	if (close (fd)) {
 		fprintf (stderr,
-			"I/O error on %s: %s\n",
-			DEVNAME (curdev), strerror (errno));
+			 "I/O error on %s: %s\n",
+			 DEVNAME (curdev), strerror (errno));
 		return (-1);
 	}
 
@@ -621,7 +639,7 @@ static int env_init (void)
 	environment.data = HaveRedundEnv ? environment.image->redund.data :
 		environment.image->single.data;
 	curdev = 0;
-	if (flash_io (O_RDONLY)) {
+	if (flash_read ()) {
 		return (errno);
 	}
 
@@ -645,7 +663,7 @@ static int env_init (void)
 		}
 		environment.image = (union env_image *)addr2;
 
-		if (flash_io (O_RDONLY)) {
+		if (flash_read ()) {
 			return (errno);
 		}
 
-- 
1.5.4

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

[Wolfgang Denk]

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.

> 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?

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: wd at denx.de
Perfection is reached, not when there is no longer anything  to  add,
but when there is no longer anything to take away.
                                           - Antoine de Saint-Exupery

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

[Guennadi Liakhovetski]

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

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

[Wolfgang Denk]

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.

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: wd at denx.de
My brother sent me a postcard the other day with this  big  sattelite
photo  of the entire earth on it. On the back it said: "Wish you were
here".                                                - Steven Wright

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

[Guennadi Liakhovetski]

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

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

[Wolfgang Denk]

Dear Guennadi Liakhovetski,

In message <Pine.LNX.4.64.0808312139100.6742@axis700.grange> you wrote:
> 
> Yes, the original code was:
...
> int flash_io(int mode)
> {
> 	fd = open(path, mode);
> 
> 	if (mode == O_RDONLY) {

That's not true. You intentionally omit the error handling part here.

> 	close(fd);
> }

Ditto.

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

...and all of the error handling.

> 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...

Indeed, I disagree. Didn't I write that before?

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: wd at denx.de
If something is different, it's either better or worse,  and  usually
both.                                                    - Larry Wall

[U-Boot] [PATCH 3/6] "return" is not a function

[Guennadi Liakhovetski]

Signed-off-by: Guennadi Liakhovetski <lg@denx.de>
---
 tools/env/fw_env.c |   88 ++++++++++++++++++++++++++--------------------------
 1 files changed, 44 insertions(+), 44 deletions(-)

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index 6f7fdb2..2c82970 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -200,7 +200,7 @@ char *fw_getenv (char *name)
 	char *env, *nxt;
 
 	if (env_init ())
-		return (NULL);
+		return NULL;
 
 	for (env = environment.data; *env; env = nxt + 1) {
 		char *val;
@@ -209,15 +209,15 @@ char *fw_getenv (char *name)
 			if (nxt >= &environment.data[ENV_SIZE]) {
 				fprintf (stderr, "## Error: "
 					"environment not terminated\n");
-				return (NULL);
+				return NULL;
 			}
 		}
 		val = envmatch (name, env);
 		if (!val)
 			continue;
-		return (val);
+		return val;
 	}
-	return (NULL);
+	return NULL;
 }
 
 /*
@@ -231,7 +231,7 @@ int fw_printenv (int argc, char *argv[])
 	int rc = 0;
 
 	if (env_init ())
-		return (-1);
+		return -1;
 
 	if (argc == 1) {		/* Print all env variables  */
 		for (env = environment.data; *env; env = nxt + 1) {
@@ -239,13 +239,13 @@ int fw_printenv (int argc, char *argv[])
 				if (nxt >= &environment.data[ENV_SIZE]) {
 					fprintf (stderr, "## Error: "
 						"environment not terminated\n");
-					return (-1);
+					return -1;
 				}
 			}
 
 			printf ("%s\n", env);
 		}
-		return (0);
+		return 0;
 	}
 
 	if (strcmp (argv[1], "-n") == 0) {
@@ -255,7 +255,7 @@ int fw_printenv (int argc, char *argv[])
 		if (argc != 2) {
 			fprintf (stderr, "## Error: "
 				"`-n' option requires exactly one argument\n");
-			return (-1);
+			return -1;
 		}
 	} else {
 		n_flag = 0;
@@ -271,7 +271,7 @@ int fw_printenv (int argc, char *argv[])
 				if (nxt >= &environment.data[ENV_SIZE]) {
 					fprintf (stderr, "## Error: "
 						"environment not terminated\n");
-					return (-1);
+					return -1;
 				}
 			}
 			val = envmatch (name, env);
@@ -290,7 +290,7 @@ int fw_printenv (int argc, char *argv[])
 		}
 	}
 
-	return (rc);
+	return rc;
 }
 
 /*
@@ -309,11 +309,11 @@ int fw_setenv (int argc, char *argv[])
 	char *name;
 
 	if (argc < 2) {
-		return (EINVAL);
+		return EINVAL;
 	}
 
 	if (env_init ())
-		return (errno);
+		return errno;
 
 	name = argv[1];
 
@@ -325,7 +325,7 @@ int fw_setenv (int argc, char *argv[])
 			if (nxt >= &environment.data[ENV_SIZE]) {
 				fprintf (stderr, "## Error: "
 					"environment not terminated\n");
-				return (EINVAL);
+				return EINVAL;
 			}
 		}
 		if ((oldval = envmatch (name, env)) != NULL)
@@ -342,7 +342,7 @@ int fw_setenv (int argc, char *argv[])
 		if ((strcmp (name, "ethaddr") == 0) ||
 			(strcmp (name, "serial#") == 0)) {
 			fprintf (stderr, "Can't overwrite \"%s\"\n", name);
-			return (EROFS);
+			return EROFS;
 		}
 
 		if (*++nxt == '\0') {
@@ -381,7 +381,7 @@ int fw_setenv (int argc, char *argv[])
 		fprintf (stderr,
 			"Error: environment overflow, \"%s\" deleted\n",
 			name);
-		return (-1);
+		return -1;
 	}
 	while ((*env = *name++) != '\0')
 		env++;
@@ -407,10 +407,10 @@ int fw_setenv (int argc, char *argv[])
 	/* write environment back to flash */
 	if (flash_write ()) {
 		fprintf (stderr, "Error: can't write fw_env to flash\n");
-		return (-1);
+		return -1;
 	}
 
-	return (0);
+	return 0;
 }
 
 static int flash_write (void)
@@ -423,7 +423,7 @@ static int flash_write (void)
 		fprintf (stderr,
 			 "Can't open %s: %s\n",
 			 DEVNAME (curdev), strerror (errno));
-		return (-1);
+		return -1;
 	}
 
 	if (HaveRedundEnv) {
@@ -434,7 +434,7 @@ static int flash_write (void)
 				 "Can't open %s: %s\n",
 				 DEVNAME (otherdev),
 				 strerror (errno));
-			return (-1);
+			return -1;
 		}
 	} else {
 		otherdev = curdev;
@@ -460,21 +460,21 @@ static int flash_write (void)
 				 "Cannot malloc %d bytes: %s\n",
 				 resid,
 				 strerror (errno));
-			return (-1);
+			return -1;
 		}
 		if (lseek (fdr, DEVOFFSET (otherdev) + CFG_ENV_SIZE, SEEK_SET)
 		    == -1) {
 			fprintf (stderr, "seek error on %s: %s\n",
 				 DEVNAME (otherdev),
 				 strerror (errno));
-			return (-1);
+			return -1;
 		}
 		if ((rc = read (fdr, data, resid)) != resid) {
 			fprintf (stderr,
 				 "read error on %s: %s\n",
 				 DEVNAME (otherdev),
 				 strerror (errno));
-			return (-1);
+			return -1;
 		}
 	}
 
@@ -486,7 +486,7 @@ static int flash_write (void)
 		fprintf (stderr, "MTD erase error on %s: %s\n",
 			 DEVNAME (otherdev),
 			 strerror (errno));
-		return (-1);
+		return -1;
 	}
 
 	printf ("Done\n");
@@ -496,14 +496,14 @@ static int flash_write (void)
 		fprintf (stderr,
 			 "seek error on %s: %s\n",
 			 DEVNAME (otherdev), strerror (errno));
-		return (-1);
+		return -1;
 	}
 
 	if (write (fdr, environment.image, CFG_ENV_SIZE) != CFG_ENV_SIZE) {
 		fprintf (stderr,
 			 "Write error on %s: %s\n",
 			 DEVNAME (otherdev), strerror (errno));
-		return (-1);
+		return -1;
 	}
 
 	if (resid) {
@@ -511,7 +511,7 @@ static int flash_write (void)
 			fprintf (stderr,
 				 "write error on %s: %s\n",
 				 DEVNAME (curdev), strerror (errno));
-			return (-1);
+			return -1;
 		}
 		free (data);
 	}
@@ -521,14 +521,14 @@ static int flash_write (void)
 		    == -1) {
 			fprintf (stderr, "seek error on %s: %s\n",
 				 DEVNAME (curdev), strerror (errno));
-			return (-1);
+			return -1;
 		}
 		if (write (fd, &obsolete_flag, sizeof (obsolete_flag)) !=
 		    sizeof (obsolete_flag)) {
 			fprintf (stderr,
 				 "Write error on %s: %s\n",
 				 DEVNAME (curdev), strerror (errno));
-			return (-1);
+			return -1;
 		}
 	}
 	printf ("Done\n");
@@ -545,7 +545,7 @@ static int flash_write (void)
 				 "I/O error on %s: %s\n",
 				 DEVNAME (otherdev),
 				 strerror (errno));
-			return (-1);
+			return -1;
 		}
 	}
 	printf ("Done\n");
@@ -554,11 +554,11 @@ static int flash_write (void)
 		fprintf (stderr,
 			 "I/O error on %s: %s\n",
 			 DEVNAME (curdev), strerror (errno));
-		return (-1);
+		return -1;
 	}
 
 	/* everything ok */
-	return (0);
+	return 0;
 }
 
 static int flash_read (void)
@@ -569,31 +569,31 @@ static int flash_read (void)
 		fprintf (stderr,
 			 "Can't open %s: %s\n",
 			 DEVNAME (curdev), strerror (errno));
-		return (-1);
+		return -1;
 	}
 
 	if (lseek (fd, DEVOFFSET (curdev), SEEK_SET) == -1) {
 		fprintf (stderr,
 			 "seek error on %s: %s\n",
 			 DEVNAME (curdev), strerror (errno));
-		return (-1);
+		return -1;
 	}
 	if (read (fd, environment.image, CFG_ENV_SIZE) != CFG_ENV_SIZE) {
 		fprintf (stderr,
 			 "Read error on %s: %s\n",
 			 DEVNAME (curdev), strerror (errno));
-		return (-1);
+		return -1;
 	}
 
 	if (close (fd)) {
 		fprintf (stderr,
 			 "I/O error on %s: %s\n",
 			 DEVNAME (curdev), strerror (errno));
-		return (-1);
+		return -1;
 	}
 
 	/* everything ok */
-	return (0);
+	return 0;
 }
 
 /*
@@ -607,10 +607,10 @@ static char *envmatch (char * s1, char * s2)
 
 	while (*s1 == *s2++)
 		if (*s1++ == '=')
-			return (s2);
+			return s2;
 	if (*s1 == '\0' && *(s2 - 1) == '=')
-		return (s2);
-	return (NULL);
+		return s2;
+	return NULL;
 }
 
 /*
@@ -631,7 +631,7 @@ static int env_init (void)
 		fprintf (stderr,
 			"Not enough memory for environment (%ld bytes)\n",
 			CFG_ENV_SIZE);
-		return (errno);
+		return errno;
 	}
 
 	/* read environment from FLASH to local buffer */
@@ -640,7 +640,7 @@ static int env_init (void)
 		environment.image->single.data;
 	curdev = 0;
 	if (flash_read ()) {
-		return (errno);
+		return errno;
 	}
 
 	crc1 = crc32 (0, (uint8_t *) environment.data, ENV_SIZE);
@@ -659,12 +659,12 @@ static int env_init (void)
 			fprintf (stderr,
 				"Not enough memory for environment (%ld bytes)\n",
 				CFG_ENV_SIZE);
-			return (errno);
+			return errno;
 		}
 		environment.image = (union env_image *)addr2;
 
 		if (flash_read ()) {
-			return (errno);
+			return errno;
 		}
 
 		crc2 = crc32 (0, (uint8_t *) environment.image->redund.data,
@@ -730,7 +730,7 @@ static int env_init (void)
 		}
 		environment.data = environment.image->redund.data;
 	}
-	return (0);
+	return 0;
 }
 
 
-- 
1.5.4

[U-Boot] [PATCH 3/6] "return" is not a function

[Wolfgang Denk]

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.

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: wd at denx.de
Any sufficiently advanced technology is indistinguishable from magic.
                                                   - Arthur C. Clarke

[U-Boot] [PATCH 3/6] "return" is not a function

[Guennadi Liakhovetski]

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

[U-Boot] [PATCH 3/6] "return" is not a function

[Wolfgang Denk]

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.

> 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. 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.)

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: wd at denx.de
A day without sunshine is like night.

[U-Boot] [PATCH 3/6] "return" is not a function

[Guennadi Liakhovetski]

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

[U-Boot] [PATCH 4/6] Unify active vs. redundant environment variable naming

[Guennadi Liakhovetski]

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

Signed-off-by: Guennadi Liakhovetski <lg@denx.de>
---
 tools/env/fw_env.c |  129 +++++++++++++++++++++++++--------------------------
 1 files changed, 63 insertions(+), 66 deletions(-)

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index 2c82970..556aa85 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -52,14 +52,14 @@ typedef struct envdev_s {
 } envdev_t;
 
 static envdev_t envdevices[2];
-static int curdev;
+static int dev_current;
 
 #define DEVNAME(i)    envdevices[(i)].devname
 #define DEVOFFSET(i)  envdevices[(i)].devoff
 #define ENVSIZE(i)    envdevices[(i)].env_size
 #define DEVESIZE(i)   envdevices[(i)].erase_size
 
-#define CFG_ENV_SIZE ENVSIZE(curdev)
+#define CFG_ENV_SIZE ENVSIZE(dev_current)
 
 #define ENV_SIZE      getenvsize()
 
@@ -415,45 +415,47 @@ int fw_setenv (int argc, char *argv[])
 
 static int flash_write (void)
 {
-	int fd, fdr, rc, otherdev, resid;
-	erase_info_t erase;
+	int fd_current, fd_target, rc, dev_target, resid;
+	erase_info_t erase_current = {}, erase_target;
 	char *data = NULL;
 
-	if ((fd = open (DEVNAME (curdev), O_RDWR)) < 0) {
+	/* dev_current: fd_current, erase_current */
+	if ((fd_current = open (DEVNAME (dev_current), O_RDWR)) < 0) {
 		fprintf (stderr,
 			 "Can't open %s: %s\n",
-			 DEVNAME (curdev), strerror (errno));
+			 DEVNAME (dev_current), strerror (errno));
 		return -1;
 	}
 
 	if (HaveRedundEnv) {
 		/* switch to next partition for writing */
-		otherdev = !curdev;
-		if ((fdr = open (DEVNAME (otherdev), O_RDWR)) < 0) {
+		dev_target = !dev_current;
+		/* dev_target: fd_target, erase_target */
+		if ((fd_target = open (DEVNAME (dev_target), O_RDWR)) < 0) {
 			fprintf (stderr,
 				 "Can't open %s: %s\n",
-				 DEVNAME (otherdev),
+				 DEVNAME (dev_target),
 				 strerror (errno));
 			return -1;
 		}
 	} else {
-		otherdev = curdev;
-		fdr = fd;
+		dev_target = dev_current;
+		fd_target = fd_current;
 	}
 	printf ("Unlocking flash...\n");
-	erase.length = DEVESIZE (otherdev);
-	erase.start = DEVOFFSET (otherdev);
-	ioctl (fdr, MEMUNLOCK, &erase);
+	erase_target.length = DEVESIZE (dev_target);
+	erase_target.start = DEVOFFSET (dev_target);
+	ioctl (fd_target, MEMUNLOCK, &erase_target);
 
 	if (HaveRedundEnv) {
-		erase.length = DEVESIZE (curdev);
-		erase.start = DEVOFFSET (curdev);
-		ioctl (fd, MEMUNLOCK, &erase);
+		erase_current.length = DEVESIZE (dev_current);
+		erase_current.start = DEVOFFSET (dev_current);
+		ioctl (fd_current, MEMUNLOCK, &erase_current);
 		ENV_FLAGS(environment) = active_flag;
 	}
 
 	printf ("Done\n");
-	resid = DEVESIZE (otherdev) - CFG_ENV_SIZE;
+	resid = DEVESIZE (dev_target) - CFG_ENV_SIZE;
 	if (resid) {
 		if ((data = malloc (resid)) == NULL) {
 			fprintf (stderr,
@@ -462,17 +464,17 @@ static int flash_write (void)
 				 strerror (errno));
 			return -1;
 		}
-		if (lseek (fdr, DEVOFFSET (otherdev) + CFG_ENV_SIZE, SEEK_SET)
-		    == -1) {
+		if (lseek (fd_target, DEVOFFSET (dev_target) + CFG_ENV_SIZE,
+			   SEEK_SET) == -1) {
 			fprintf (stderr, "seek error on %s: %s\n",
-				 DEVNAME (otherdev),
+				 DEVNAME (dev_target),
 				 strerror (errno));
 			return -1;
 		}
-		if ((rc = read (fdr, data, resid)) != resid) {
+		if ((rc = read (fd_target, data, resid)) != resid) {
 			fprintf (stderr,
 				 "read error on %s: %s\n",
-				 DEVNAME (otherdev),
+				 DEVNAME (dev_target),
 				 strerror (errno));
 			return -1;
 		}
@@ -480,80 +482,75 @@ static int flash_write (void)
 
 	printf ("Erasing old environment...\n");
 
-	erase.length = DEVESIZE (otherdev);
-	erase.start = DEVOFFSET (otherdev);
-	if (ioctl (fdr, MEMERASE, &erase) != 0) {
+	if (ioctl (fd_target, MEMERASE, &erase_target) != 0) {
 		fprintf (stderr, "MTD erase error on %s: %s\n",
-			 DEVNAME (otherdev),
+			 DEVNAME (dev_target),
 			 strerror (errno));
 		return -1;
 	}
 
 	printf ("Done\n");
 
-	printf ("Writing environment to %s...\n", DEVNAME (otherdev));
-	if (lseek (fdr, DEVOFFSET (otherdev), SEEK_SET) == -1) {
+	printf ("Writing environment to %s...\n", DEVNAME (dev_target));
+	if (lseek (fd_target, DEVOFFSET (dev_target), SEEK_SET) == -1) {
 		fprintf (stderr,
 			 "seek error on %s: %s\n",
-			 DEVNAME (otherdev), strerror (errno));
+			 DEVNAME (dev_target), strerror (errno));
 		return -1;
 	}
 
-	if (write (fdr, environment.image, CFG_ENV_SIZE) != CFG_ENV_SIZE) {
+	if (write (fd_target, environment.image, CFG_ENV_SIZE) !=
+	    CFG_ENV_SIZE) {
 		fprintf (stderr,
 			 "Write error on %s: %s\n",
-			 DEVNAME (otherdev), strerror (errno));
+			 DEVNAME (dev_target), strerror (errno));
 		return -1;
 	}
 
 	if (resid) {
-		if (write (fdr, data, resid) != resid) {
+		if (write (fd_target, data, resid) != resid) {
 			fprintf (stderr,
 				 "write error on %s: %s\n",
-				 DEVNAME (curdev), strerror (errno));
+				 DEVNAME (dev_current), strerror (errno));
 			return -1;
 		}
 		free (data);
 	}
 	if (HaveRedundEnv) {
 		/* change flag on current active env partition */
-		if (lseek (fd, DEVOFFSET (curdev) + sizeof (ulong), SEEK_SET)
-		    == -1) {
+		if (lseek (fd_current, DEVOFFSET (dev_current) + sizeof (ulong),
+			   SEEK_SET) == -1) {
 			fprintf (stderr, "seek error on %s: %s\n",
-				 DEVNAME (curdev), strerror (errno));
+				 DEVNAME (dev_current), strerror (errno));
 			return -1;
 		}
-		if (write (fd, &obsolete_flag, sizeof (obsolete_flag)) !=
-		    sizeof (obsolete_flag)) {
+		if (write (fd_current, &obsolete_flag,
+			   sizeof (obsolete_flag)) != sizeof (obsolete_flag)) {
 			fprintf (stderr,
 				 "Write error on %s: %s\n",
-				 DEVNAME (curdev), strerror (errno));
+				 DEVNAME (dev_current), strerror (errno));
 			return -1;
 		}
 	}
 	printf ("Done\n");
 	printf ("Locking ...\n");
-	erase.length = DEVESIZE (otherdev);
-	erase.start = DEVOFFSET (otherdev);
-	ioctl (fdr, MEMLOCK, &erase);
+	ioctl (fd_target, MEMLOCK, &erase_target);
 	if (HaveRedundEnv) {
-		erase.length = DEVESIZE (curdev);
-		erase.start = DEVOFFSET (curdev);
-		ioctl (fd, MEMLOCK, &erase);
-		if (close (fdr)) {
+		ioctl (fd_current, MEMLOCK, &erase_current);
+		if (close (fd_target)) {
 			fprintf (stderr,
 				 "I/O error on %s: %s\n",
-				 DEVNAME (otherdev),
+				 DEVNAME (dev_target),
 				 strerror (errno));
 			return -1;
 		}
 	}
 	printf ("Done\n");
 
-	if (close (fd)) {
+	if (close (fd_current)) {
 		fprintf (stderr,
 			 "I/O error on %s: %s\n",
-			 DEVNAME (curdev), strerror (errno));
+			 DEVNAME (dev_current), strerror (errno));
 		return -1;
 	}
 
@@ -565,30 +562,30 @@ static int flash_read (void)
 {
 	int fd;
 
-	if ((fd = open (DEVNAME (curdev), O_RDONLY)) < 0) {
+	if ((fd = open (DEVNAME (dev_current), O_RDONLY)) < 0) {
 		fprintf (stderr,
 			 "Can't open %s: %s\n",
-			 DEVNAME (curdev), strerror (errno));
+			 DEVNAME (dev_current), strerror (errno));
 		return -1;
 	}
 
-	if (lseek (fd, DEVOFFSET (curdev), SEEK_SET) == -1) {
+	if (lseek (fd, DEVOFFSET (dev_current), SEEK_SET) == -1) {
 		fprintf (stderr,
 			 "seek error on %s: %s\n",
-			 DEVNAME (curdev), strerror (errno));
+			 DEVNAME (dev_current), strerror (errno));
 		return -1;
 	}
 	if (read (fd, environment.image, CFG_ENV_SIZE) != CFG_ENV_SIZE) {
 		fprintf (stderr,
 			 "Read error on %s: %s\n",
-			 DEVNAME (curdev), strerror (errno));
+			 DEVNAME (dev_current), strerror (errno));
 		return -1;
 	}
 
 	if (close (fd)) {
 		fprintf (stderr,
 			 "I/O error on %s: %s\n",
-			 DEVNAME (curdev), strerror (errno));
+			 DEVNAME (dev_current), strerror (errno));
 		return -1;
 	}
 
@@ -638,7 +635,7 @@ static int env_init (void)
 	environment.image = (union env_image *)addr1;
 	environment.data = HaveRedundEnv ? environment.image->redund.data :
 		environment.image->single.data;
-	curdev = 0;
+	dev_current = 0;
 	if (flash_read ()) {
 		return errno;
 	}
@@ -654,7 +651,7 @@ static int env_init (void)
 	} else {
 		flag1 = ENV_FLAGS(environment);
 
-		curdev = 1;
+		dev_current = 1;
 		if ((addr2 = calloc (1, CFG_ENV_SIZE)) == NULL) {
 			fprintf (stderr,
 				"Not enough memory for environment (%ld bytes)\n",
@@ -676,47 +673,47 @@ static int env_init (void)
 			environment.image = (union env_image *)addr1;
 			ENV_FLAGS(environment) = flag1;
 			environment.image->redund.crc = crc1;
-			curdev = 0;
+			dev_current = 0;
 			free (addr2);
 		} else if (!crc1_ok && crc2_ok) {
 			environment.image = (union env_image *)addr2;
 			ENV_FLAGS(environment) = flag2;
 			environment.image->redund.crc = crc2;
-			curdev = 1;
+			dev_current = 1;
 			free (addr1);
 		} else if (!crc1_ok && !crc2_ok) {
 			fprintf (stderr,
 				"Warning: Bad CRC, using default environment\n");
 			memcpy(environment.image->redund.data,
 			       default_environment, sizeof default_environment);
-			curdev = 0;
+			dev_current = 0;
 			free (addr1);
 		/* From here: both CRCs correct */
 		} else if (flag1 == active_flag && flag2 == obsolete_flag) {
 			environment.image = (union env_image *)addr1;
 			ENV_FLAGS(environment) = flag1;
 			environment.image->redund.crc = crc1;
-			curdev = 0;
+			dev_current = 0;
 			free (addr2);
 		} else if (flag1 == obsolete_flag && flag2 == active_flag) {
 			environment.image = (union env_image *)addr2;
 			ENV_FLAGS(environment) = flag2;
 			environment.image->redund.crc = crc2;
-			curdev = 1;
+			dev_current = 1;
 			free (addr1);
 		/* From here: invalid flag configuration */
 		} else if (flag1 == flag2) {
 			environment.image = (union env_image *)addr1;
 			ENV_FLAGS(environment) = flag1;
 			environment.image->redund.crc = crc1;
-			curdev = 0;
+			dev_current = 0;
 			free (addr2);
 		} else if (flag1 == obsolete_flag || flag1 == active_flag) {
 			/* flag1 valid, update flag2 */
 			environment.image = (union env_image *)addr1;
 			ENV_FLAGS(environment) = flag1;
 			environment.image->redund.crc = crc1;
-			curdev = 0;
+			dev_current = 0;
 			free (addr2);
 		} else {
 			if (flag2 != obsolete_flag && flag2 != active_flag)
@@ -725,7 +722,7 @@ static int env_init (void)
 			environment.image = (union env_image *)addr2;
 			ENV_FLAGS(environment) = flag2;
 			environment.image->redund.crc = crc2;
-			curdev = 1;
+			dev_current = 1;
 			free (addr1);
 		}
 		environment.data = environment.image->redund.data;
-- 
1.5.4

[U-Boot] [PATCH 4/6] Unify active vs. redundant environment variable naming

[Wolfgang Denk]

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

-- 
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: wd at denx.de
"It is better to have tried and failed than to have  failed  to  try,
but the result's the same."                           - Mike Dennison

[U-Boot] [PATCH 4/6] Unify active vs. redundant environment variable naming

[Guennadi Liakhovetski]

On Sun, 31 Aug 2008, Wolfgang Denk wrote:

> 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.

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 
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.

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

[U-Boot] [PATCH 4/6] Unify active vs. redundant environment variable naming

[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

-- 
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: wd at denx.de
If it happens once, it's a bug.
If it happens twice, it's a feature.
If it happens more than twice, it's a design philosophy.

[U-Boot] [PATCH 4/6] Unify active vs. redundant environment variable naming

[Guennadi Liakhovetski]

On Sun, 31 Aug 2008, Wolfgang Denk wrote:

> 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.

Well, as I worked on this patch series, it was pretty difficult to me to 
recognise which of the two environment copies the current code was dealing 
with. So, to help myself and any future developers, that will work with 
this code, I decided to make this distinction clearer and more consistent. 
Sorry, but to me it wasn't obvious, that fd was referring to devcur, and 
fdr to devother - just from the naming. Whereas using fd_current, 
dev_current and fd_target, dev_target makes it clearer, IMHO. Yes, this is 
longer to type, and produces longer lines. But I am prepared to pay this 
price:-)

As for the "functional" change - introducing an additional erase variable 
to avoid having to reuse and reinitialise one several times - it is a 
related change, it also separates handling of the two environment copies. 
So, I think, they belong to one patch.

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.

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

[U-Boot] [PATCH 4/6] Unify active vs. redundant environment variable naming

[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

-- 
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: wd at denx.de
If programming was easy, they wouldn't need something as  complicated
as a human being to do it, now would they?
                       - L. Wall & R. L. Schwartz, _Programming Perl_

[U-Boot] [PATCH 5/6] Support environment anywhere within erase area

[Guennadi Liakhovetski]

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.

Signed-off-by: Guennadi Liakhovetski <lg@denx.de>
---
 tools/env/fw_env.c |  125 +++++++++++++++++++++++++++++++--------------------
 1 files changed, 76 insertions(+), 49 deletions(-)

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index 556aa85..931e647 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -413,11 +413,37 @@ int fw_setenv (int argc, char *argv[])
 	return 0;
 }
 
+static int flash_read_buf (int dev, int fd, void *buf, size_t count,
+			   off_t offset)
+{
+	int rc;
+
+	rc = lseek (fd, offset, SEEK_SET);
+	if (rc == -1) {
+		fprintf (stderr,
+			 "seek error on %s: %s\n",
+			 DEVNAME (dev), strerror (errno));
+		return rc;
+	}
+
+	rc = read (fd, buf, count);
+	if (rc != count) {
+		fprintf (stderr,
+			 "Read error on %s: %s\n",
+			 DEVNAME (dev), strerror (errno));
+		return -1;
+	}
+
+	return rc;
+}
+
 static int flash_write (void)
 {
-	int fd_current, fd_target, rc, dev_target, resid;
+	int fd_current, fd_target, rc, dev_target;
 	erase_info_t erase_current = {}, erase_target;
 	char *data = NULL;
+	off_t erase_offset;
+	struct mtd_info_user mtdinfo_target;
 
 	/* dev_current: fd_current, erase_current */
 	if ((fd_current = open (DEVNAME (dev_current), O_RDWR)) < 0) {
@@ -442,6 +468,45 @@ static int flash_write (void)
 		dev_target = dev_current;
 		fd_target = fd_current;
 	}
+
+	/*
+	 * 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);
+	}
+
 	printf ("Unlocking flash...\n");
 	erase_target.length = DEVESIZE (dev_target);
 	erase_target.start = DEVOFFSET (dev_target);
@@ -455,30 +520,6 @@ static int flash_write (void)
 	}
 
 	printf ("Done\n");
-	resid = DEVESIZE (dev_target) - CFG_ENV_SIZE;
-	if (resid) {
-		if ((data = malloc (resid)) == NULL) {
-			fprintf (stderr,
-				 "Cannot malloc %d bytes: %s\n",
-				 resid,
-				 strerror (errno));
-			return -1;
-		}
-		if (lseek (fd_target, DEVOFFSET (dev_target) + CFG_ENV_SIZE,
-			   SEEK_SET) == -1) {
-			fprintf (stderr, "seek error on %s: %s\n",
-				 DEVNAME (dev_target),
-				 strerror (errno));
-			return -1;
-		}
-		if ((rc = read (fd_target, data, resid)) != resid) {
-			fprintf (stderr,
-				 "read error on %s: %s\n",
-				 DEVNAME (dev_target),
-				 strerror (errno));
-			return -1;
-		}
-	}
 
 	printf ("Erasing old environment...\n");
 
@@ -492,30 +533,24 @@ static int flash_write (void)
 	printf ("Done\n");
 
 	printf ("Writing environment to %s...\n", DEVNAME (dev_target));
-	if (lseek (fd_target, DEVOFFSET (dev_target), SEEK_SET) == -1) {
+	if (lseek (fd_target, erase_offset, SEEK_SET) == -1) {
 		fprintf (stderr,
 			 "seek error on %s: %s\n",
 			 DEVNAME (dev_target), strerror (errno));
 		return -1;
 	}
 
-	if (write (fd_target, environment.image, CFG_ENV_SIZE) !=
-	    CFG_ENV_SIZE) {
+	if (write (fd_target, data, DEVESIZE (dev_target)) !=
+	    DEVESIZE (dev_target)) {
 		fprintf (stderr,
 			 "Write error on %s: %s\n",
 			 DEVNAME (dev_target), strerror (errno));
 		return -1;
 	}
 
-	if (resid) {
-		if (write (fd_target, data, resid) != resid) {
-			fprintf (stderr,
-				 "write error on %s: %s\n",
-				 DEVNAME (dev_current), strerror (errno));
-			return -1;
-		}
+	if (DEVESIZE (dev_target) > CFG_ENV_SIZE)
 		free (data);
-	}
+
 	if (HaveRedundEnv) {
 		/* change flag on current active env partition */
 		if (lseek (fd_current, DEVOFFSET (dev_current) + sizeof (ulong),
@@ -560,7 +595,7 @@ static int flash_write (void)
 
 static int flash_read (void)
 {
-	int fd;
+	int fd, rc;
 
 	if ((fd = open (DEVNAME (dev_current), O_RDONLY)) < 0) {
 		fprintf (stderr,
@@ -569,18 +604,10 @@ static int flash_read (void)
 		return -1;
 	}
 
-	if (lseek (fd, DEVOFFSET (dev_current), SEEK_SET) == -1) {
-		fprintf (stderr,
-			 "seek error on %s: %s\n",
-			 DEVNAME (dev_current), strerror (errno));
-		return -1;
-	}
-	if (read (fd, environment.image, CFG_ENV_SIZE) != CFG_ENV_SIZE) {
-		fprintf (stderr,
-			 "Read error on %s: %s\n",
-			 DEVNAME (dev_current), strerror (errno));
-		return -1;
-	}
+	rc = flash_read_buf (dev_current, fd, environment.image, CFG_ENV_SIZE,
+			     DEVOFFSET (dev_current));
+	if (rc < 0)
+		return rc;
 
 	if (close (fd)) {
 		fprintf (stderr,
-- 
1.5.4

[U-Boot] [PATCH 5/6] Support environment anywhere within erase area

[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

-- 
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: wd at denx.de
      Bugs are by far the largest and  most successful class of
      entity, with nearly a million known species. In this res-
      pect they outnumber all the other  known  creatures about
      four to one.  -- Professor Snope's Encyclopedia of Animal

[U-Boot] [PATCH 5/6] Support environment anywhere within erase area

[Guennadi Liakhovetski]

On Sun, 31 Aug 2008, Wolfgang Denk wrote:

> 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" ?

Yes, at any offset. I hope, everybody will not start from tomorrow putting 
their environment at offset 17, but this is supported by this tool now, 
yes.

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.

> 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".

> > +	/*
> > +	 * 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...

> > +	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?

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

> 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.

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.

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

[U-Boot] [PATCH 5/6] Support environment anywhere within erase area

[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

-- 
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: wd at denx.de
If something is different, it's either better or worse,  and  usually
both.                                                    - Larry Wall

[U-Boot] [PATCH 6/6] Support environment in NAND

[Guennadi Liakhovetski]

Add support for environment in NAND with automatic recognition, including
unaligned environment, bad-block skipping, redundant environment copy.

Signed-off-by: Guennadi Liakhovetski <lg@denx.de>
---
 tools/env/fw_env.c |  344 +++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 231 insertions(+), 113 deletions(-)

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index 931e647..66422e3 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -44,6 +44,12 @@
 #define	CMD_GETENV	"fw_printenv"
 #define	CMD_SETENV	"fw_setenv"
 
+#define min(x, y) ({				\
+	typeof(x) _min1 = (x);			\
+	typeof(y) _min2 = (y);			\
+	(void) (&_min1 == &_min2);		\
+	_min1 < _min2 ? _min1 : _min2; })
+
 typedef struct envdev_s {
 	char devname[16];		/* Device name */
 	ulong devoff;			/* Device offset */
@@ -413,179 +419,290 @@ int fw_setenv (int argc, char *argv[])
 	return 0;
 }
 
+static int flash_bad_block (int dev, int fd, struct mtd_info_user *mtdinfo,
+			    loff_t *blockstart, size_t blocklen)
+{
+	if (mtdinfo->type == MTD_NANDFLASH) {
+		int badblock = ioctl (fd, MEMGETBADBLOCK, blockstart);
+
+		if (badblock < 0) {
+			perror ("Cannot read bad block mark");
+			return badblock;
+		}
+
+		if (badblock) {
+			fprintf (stderr, "Bad block at 0x%llx, "
+				 "skipping\n", *blockstart);
+			*blockstart += blocklen;
+			return badblock;
+		}
+	}
+
+	return 0;
+}
+
+/*
+ * We are called with count == 0 for backing up as much data from the
+ * range as possible
+ */
 static int flash_read_buf (int dev, int fd, void *buf, size_t count,
-			   off_t offset)
+			   off_t offset, size_t range)
 {
+	struct mtd_info_user mtdinfo;
+	size_t blocklen, processed = 0;
+	size_t readlen = count ? : range;
+	off_t erase_offset, block_seek;
+	loff_t blockstart;
 	int rc;
+	int backup_mode = !count;
 
-	rc = lseek (fd, offset, SEEK_SET);
-	if (rc == -1) {
-		fprintf (stderr,
-			 "seek error on %s: %s\n",
-			 DEVNAME (dev), strerror (errno));
+	if (!count)
+		count = range;
+
+	rc = ioctl (fd, MEMGETINFO, &mtdinfo);
+	if (rc < 0) {
+		perror ("Cannot get MTD information");
 		return rc;
 	}
 
-	rc = read (fd, buf, count);
-	if (rc != count) {
-		fprintf (stderr,
-			 "Read error on %s: %s\n",
-			 DEVNAME (dev), strerror (errno));
-		return -1;
+	/* Erase sector size is always a power of 2 */
+	erase_offset = offset & ~(mtdinfo.erasesize - 1);
+
+	blockstart = erase_offset;
+	/* Offset inside a block */
+	block_seek = offset - erase_offset;
+
+	if (mtdinfo.type == MTD_NANDFLASH) {
+		/*
+		 * NAND: calculate which blocks we are reading. We have
+		 * to read one block@a time to skip bad blocks.
+		 */
+		blocklen = mtdinfo.erasesize;
+		/* Limit to one block for the first read */
+		if (readlen > blocklen - block_seek)
+			readlen = blocklen - block_seek;
+	} else {
+		blocklen = 0;
 	}
 
-	return rc;
+	/* This only runs once for NOR flash */
+	while (processed < count) {
+		rc = flash_bad_block (dev, fd, &mtdinfo, &blockstart, blocklen);
+		if (rc < 0)
+			return -1;
+		else if (blockstart + block_seek + readlen > offset + range) {
+			/* End of range is reached */
+			if (backup_mode) {
+				return processed;
+			} else {
+				fprintf (stderr,
+					 "Too few good blocks within range\n");
+				return -1;
+			}
+		} else if (rc)
+			continue;
+
+		/*
+		 * If a block is bad, we retry in the next block
+		 * at the same offset - see common/env_nand.c::
+		 * writeenv()
+		 */
+		lseek (fd, blockstart + block_seek, SEEK_SET);
+
+		rc = read (fd, buf + processed, readlen);
+		if (rc != readlen) {
+			fprintf (stderr,
+				 "Read error on %s: %s\n",
+				 DEVNAME (dev), strerror (errno));
+			return -1;
+		}
+		processed += readlen;
+		readlen = min(blocklen, count - processed);
+		block_seek = 0;
+		blockstart += blocklen;
+	}
+
+	return processed;
 }
 
-static int flash_write (void)
+static int flash_write_buf (int dev, int fd, void *buf, size_t count,
+			    off_t offset)
 {
-	int fd_current, fd_target, rc, dev_target;
-	erase_info_t erase_current = {}, erase_target;
 	char *data = NULL;
-	off_t erase_offset;
-	struct mtd_info_user mtdinfo_target;
+	erase_info_t erase;
+	struct mtd_info_user mtdinfo;
+	size_t blocklen, erase_len, processed = 0;
+	size_t writelen, write_total = DEVESIZE (dev);
+	off_t erase_offset, block_seek;
+	loff_t blockstart;
+	int rc;
 
-	/* dev_current: fd_current, erase_current */
-	if ((fd_current = open (DEVNAME (dev_current), O_RDWR)) < 0) {
-		fprintf (stderr,
-			 "Can't open %s: %s\n",
-			 DEVNAME (dev_current), strerror (errno));
+	rc = ioctl (fd, MEMGETINFO, &mtdinfo);
+	if (rc < 0) {
+		perror ("Cannot get MTD information");
 		return -1;
 	}
 
-	if (HaveRedundEnv) {
-		/* switch to next partition for writing */
-		dev_target = !dev_current;
-		/* dev_target: fd_target, erase_target */
-		if ((fd_target = open (DEVNAME (dev_target), O_RDWR)) < 0) {
-			fprintf (stderr,
-				 "Can't open %s: %s\n",
-				 DEVNAME (dev_target),
-				 strerror (errno));
-			return -1;
-		}
-	} else {
-		dev_target = dev_current;
-		fd_target = fd_current;
-	}
+	/* Erase sector size is always a power of 2 */
+	erase_offset = offset & ~(mtdinfo.erasesize - 1);
+	/* Maximum area we may use */
+	erase_len = (offset - erase_offset + DEVESIZE (dev) +
+		     mtdinfo.erasesize - 1) & ~(mtdinfo.erasesize - 1);
+
+	blockstart = erase_offset;
+	/* Offset inside a block */
+	block_seek = offset - erase_offset;
 
 	/*
 	 * 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 (erase_len > DEVESIZE (dev)) {
+		data = malloc (erase_len);
 		if (!data) {
 			fprintf (stderr,
-				 "Cannot malloc %lu bytes: %s\n",
-				 DEVESIZE (dev_target),
-				 strerror (errno));
+				 "Cannot malloc %u bytes: %s\n",
+				 erase_len, strerror (errno));
 			return -1;
 		}
 
-		rc = ioctl (fd_target, MEMGETINFO, &mtdinfo_target);
-		if (rc < 0) {
-			perror ("Cannot get MTD information");
+		/*
+		 * This is different from a normal read. We have to read as much
+		 * as we can from a certain area, and it should be at least X
+		 * bytes, instead of having to read a fixed number of bytes as
+		 * usual. This also tells us how much data "fits" in the good
+		 * blocks in the area.
+		 */
+		write_total = flash_read_buf (dev, fd, data, 0,
+					      erase_offset, erase_len);
+		if (write_total < block_seek + CFG_ENV_SIZE)
 			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);
+		memcpy(data + block_seek, buf, count);
 	} else {
 		data = (char *)environment.image;
-		erase_offset = DEVOFFSET (dev_target);
 	}
 
-	printf ("Unlocking flash...\n");
-	erase_target.length = DEVESIZE (dev_target);
-	erase_target.start = DEVOFFSET (dev_target);
-	ioctl (fd_target, MEMUNLOCK, &erase_target);
-
-	if (HaveRedundEnv) {
-		erase_current.length = DEVESIZE (dev_current);
-		erase_current.start = DEVOFFSET (dev_current);
-		ioctl (fd_current, MEMUNLOCK, &erase_current);
-		ENV_FLAGS(environment) = active_flag;
+	if (mtdinfo.type == MTD_NANDFLASH) {
+		/*
+		 * NAND: calculate which blocks we are writing. We have
+		 * to write one block at a time to skip bad blocks.
+		 */
+		blocklen = mtdinfo.erasesize;
+		/* Limit to one block */
+		writelen = blocklen;
+	} else {
+		blocklen = erase_len;
+		writelen = erase_len;
 	}
 
-	printf ("Done\n");
+	erase.length = blocklen;
 
-	printf ("Erasing old environment...\n");
+	while (processed < write_total) {
+		rc = flash_bad_block (dev, fd, &mtdinfo, &blockstart, blocklen);
+		if (rc < 0)
+			return rc;
+		else if (rc)
+			continue;
 
-	if (ioctl (fd_target, MEMERASE, &erase_target) != 0) {
-		fprintf (stderr, "MTD erase error on %s: %s\n",
-			 DEVNAME (dev_target),
-			 strerror (errno));
-		return -1;
-	}
+		printf ("Unlocking flash at %llx...", blockstart);
 
-	printf ("Done\n");
+		erase.start = blockstart;
+		ioctl (fd, MEMUNLOCK, &erase);
 
-	printf ("Writing environment to %s...\n", DEVNAME (dev_target));
-	if (lseek (fd_target, erase_offset, SEEK_SET) == -1) {
-		fprintf (stderr,
-			 "seek error on %s: %s\n",
-			 DEVNAME (dev_target), strerror (errno));
-		return -1;
-	}
+		printf ("Done\n");
 
-	if (write (fd_target, data, DEVESIZE (dev_target)) !=
-	    DEVESIZE (dev_target)) {
-		fprintf (stderr,
-			 "Write error on %s: %s\n",
-			 DEVNAME (dev_target), strerror (errno));
-		return -1;
-	}
+		printf ("Erasing old environment@%llx...", blockstart);
 
-	if (DEVESIZE (dev_target) > CFG_ENV_SIZE)
-		free (data);
+		if (ioctl (fd, MEMERASE, &erase) != 0) {
+			fprintf (stderr, "MTD erase error on %s: %s\n",
+				 DEVNAME (dev),
+				 strerror (errno));
+			return -1;
+		}
 
-	if (HaveRedundEnv) {
-		/* change flag on current active env partition */
-		if (lseek (fd_current, DEVOFFSET (dev_current) + sizeof (ulong),
-			   SEEK_SET) == -1) {
-			fprintf (stderr, "seek error on %s: %s\n",
-				 DEVNAME (dev_current), strerror (errno));
+		printf ("Done\n");
+
+		printf ("Writing %u bytes of environment to %s...", writelen,
+			DEVNAME (dev));
+		if (lseek (fd, blockstart, SEEK_SET) == -1) {
+			fprintf (stderr,
+				 "Seek error on %s: %s\n",
+				 DEVNAME (dev), strerror (errno));
 			return -1;
 		}
-		if (write (fd_current, &obsolete_flag,
-			   sizeof (obsolete_flag)) != sizeof (obsolete_flag)) {
+
+		if (write (fd, data + processed, writelen) != writelen) {
 			fprintf (stderr,
 				 "Write error on %s: %s\n",
-				 DEVNAME (dev_current), strerror (errno));
+				 DEVNAME (dev), strerror (errno));
 			return -1;
 		}
+		printf ("Done\n");
+
+		printf ("Locking ...");
+		ioctl (fd, MEMLOCK, &erase);
+		printf ("Done\n");
+
+		processed += writelen;
+		writelen = min(blocklen, count - processed);
+		block_seek = 0;
+		blockstart += blocklen;
 	}
-	printf ("Done\n");
-	printf ("Locking ...\n");
-	ioctl (fd_target, MEMLOCK, &erase_target);
+
+	if (erase_len > CFG_ENV_SIZE)
+		free (data);
+
+	return processed;
+}
+
+static int flash_write (void)
+{
+	int fd_current, fd_target, rc, dev_target;
+
+	/* dev_current: fd_current, erase_current */
+	if ((fd_current = open (DEVNAME (dev_current), O_RDWR)) < 0) {
+		fprintf (stderr,
+			 "Can't open %s: %s\n",
+			 DEVNAME (dev_current), strerror (errno));
+		return -1;
+	}
+
 	if (HaveRedundEnv) {
-		ioctl (fd_current, MEMLOCK, &erase_current);
-		if (close (fd_target)) {
+		/* switch to next partition for writing */
+		dev_target = !dev_current;
+		/* dev_target: fd_target, erase_target */
+		if ((fd_target = open (DEVNAME (dev_target), O_RDWR)) < 0) {
 			fprintf (stderr,
-				 "I/O error on %s: %s\n",
+				 "Can't open %s: %s\n",
 				 DEVNAME (dev_target),
 				 strerror (errno));
 			return -1;
 		}
+		ENV_FLAGS(environment) = active_flag;
+	} else {
+		dev_target = dev_current;
+		fd_target = fd_current;
 	}
-	printf ("Done\n");
 
-	if (close (fd_current)) {
+	rc = flash_write_buf (dev_target, fd_target, environment.image,
+			      CFG_ENV_SIZE, DEVOFFSET (dev_target));
+	if (rc < 0)
+		return rc;
+
+	if (HaveRedundEnv) {
+		off_t offset = DEVOFFSET (dev_current) +
+			offsetof(union env_image, redund.flags);
+		rc = flash_write_buf(dev_current, fd_current, &obsolete_flag,
+				     sizeof (obsolete_flag), offset);
+	}
+
+	if (close (fd_target)) {
 		fprintf (stderr,
 			 "I/O error on %s: %s\n",
-			 DEVNAME (dev_current), strerror (errno));
+			 DEVNAME (dev_target), strerror (errno));
 		return -1;
 	}
 
@@ -604,8 +721,9 @@ static int flash_read (void)
 		return -1;
 	}
 
+	/* Only try within CFG_ENV_RANGE */
 	rc = flash_read_buf (dev_current, fd, environment.image, CFG_ENV_SIZE,
-			     DEVOFFSET (dev_current));
+			     DEVOFFSET (dev_current), DEVESIZE (dev_current));
 	if (rc < 0)
 		return rc;
 
-- 
1.5.4

[U-Boot] [PATCH 6/6 v2] Support environment in NAND

[Guennadi Liakhovetski]

Add support for environment in NAND with automatic recognition, including
unaligned environment, bad-block skipping, redundant environment copy.
A new parameter is introduced to limit the number of sectors that may be
used on NAND, when skipping bad blocks, it is unused on NOR.

Take care to preserve backwards compatibility on NOR, including identical
erase / write behaviour.

Also fix some return codes.

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

I am not resending patches 1-5, they remain unchanged.

Changes since v1: instead of abusing the "erase sector size" parameter on 
NAND and retrieving it from the kernel, switched back to using 
user-provided erase size, and introduced a new parameter for the number of 
sectors, that can be used.

 tools/env/README        |    6 +
 tools/env/fw_env.c      |  504 +++++++++++++++++++++++++++++++++--------------
 tools/env/fw_env.config |    6 +-
 3 files changed, 365 insertions(+), 151 deletions(-)

diff --git a/tools/env/README b/tools/env/README
index f8a644e..f32f872 100644
--- a/tools/env/README
+++ b/tools/env/README
@@ -22,9 +22,11 @@ following lines are relevant:
 #define DEVICE1_OFFSET    0x0000
 #define ENV1_SIZE         0x4000
 #define DEVICE1_ESIZE     0x4000
+#define DEVICE1_ENVSECTORS     2
 #define DEVICE2_OFFSET    0x0000
 #define ENV2_SIZE         0x4000
 #define DEVICE2_ESIZE     0x4000
+#define DEVICE2_ENVSECTORS     2
 
 Current configuration matches the environment layout of the TRAB
 board.
@@ -46,3 +48,7 @@ then 1 sector.
 
 DEVICEx_ESIZE defines the size of the first sector in the flash
 partition where the environment resides.
+
+DEVICEx_ENVSECTORS defines the number of sectors that may be used for
+this environment instance. On NAND this is used to limit the range
+within which bad blocks are skipped, on NOR it is unused.
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index 931e647..9adfdad 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -44,11 +44,18 @@
 #define	CMD_GETENV	"fw_printenv"
 #define	CMD_SETENV	"fw_setenv"
 
+#define min(x, y) ({				\
+	typeof(x) _min1 = (x);			\
+	typeof(y) _min2 = (y);			\
+	(void) (&_min1 == &_min2);		\
+	_min1 < _min2 ? _min1 : _min2; })
+
 typedef struct envdev_s {
 	char devname[16];		/* Device name */
 	ulong devoff;			/* Device offset */
 	ulong env_size;			/* environment size */
 	ulong erase_size;		/* device erase size */
+	ulong env_sectors;		/* number of environment sectors */
 } envdev_t;
 
 static envdev_t envdevices[2];
@@ -58,6 +65,7 @@ static int dev_current;
 #define DEVOFFSET(i)  envdevices[(i)].devoff
 #define ENVSIZE(i)    envdevices[(i)].env_size
 #define DEVESIZE(i)   envdevices[(i)].erase_size
+#define ENVSECTORS(i) envdevices[(i)].env_sectors
 
 #define CFG_ENV_SIZE ENVSIZE(dev_current)
 
@@ -88,6 +96,7 @@ static int HaveRedundEnv = 0;
 #define ENV_FLAGS(e) e.image->redund.flags
 
 static unsigned char active_flag = 1;
+/* obsolete_flag must be 0 to efficiently set it on NOR flash without erasing */
 static unsigned char obsolete_flag = 0;
 
 
@@ -309,11 +318,12 @@ int fw_setenv (int argc, char *argv[])
 	char *name;
 
 	if (argc < 2) {
-		return EINVAL;
+		errno = EINVAL;
+		return -1;
 	}
 
 	if (env_init ())
-		return errno;
+		return -1;
 
 	name = argv[1];
 
@@ -325,7 +335,8 @@ int fw_setenv (int argc, char *argv[])
 			if (nxt >= &environment.data[ENV_SIZE]) {
 				fprintf (stderr, "## Error: "
 					"environment not terminated\n");
-				return EINVAL;
+				errno = EINVAL;
+				return -1;
 			}
 		}
 		if ((oldval = envmatch (name, env)) != NULL)
@@ -342,7 +353,8 @@ int fw_setenv (int argc, char *argv[])
 		if ((strcmp (name, "ethaddr") == 0) ||
 			(strcmp (name, "serial#") == 0)) {
 			fprintf (stderr, "Can't overwrite \"%s\"\n", name);
-			return EROFS;
+			errno = EROFS;
+			return -1;
 		}
 
 		if (*++nxt == '\0') {
@@ -413,179 +425,366 @@ int fw_setenv (int argc, char *argv[])
 	return 0;
 }
 
+static int flash_bad_block (int dev, int fd, struct mtd_info_user *mtdinfo,
+			    loff_t *blockstart, size_t blocklen)
+{
+	if (mtdinfo->type == MTD_NANDFLASH) {
+		int badblock = ioctl (fd, MEMGETBADBLOCK, blockstart);
+
+		if (badblock < 0) {
+			perror ("Cannot read bad block mark");
+			return badblock;
+		}
+
+		if (badblock) {
+#ifdef DEBUG
+			fprintf (stderr, "Bad block at 0x%llx, "
+				 "skipping\n", *blockstart);
+#endif
+			*blockstart += blocklen;
+			return badblock;
+		}
+	}
+
+	return 0;
+}
+
+/*
+ * We are called with count == 0 for backing up as much data from the
+ * range as possible
+ */
 static int flash_read_buf (int dev, int fd, void *buf, size_t count,
-			   off_t offset)
+			   off_t offset, size_t range)
 {
+	struct mtd_info_user mtdinfo;
+	/* erase / write length - one block on NAND, 0 on NOR */
+	size_t blocklen;
+	/* progress counter */
+	size_t processed = 0;
+	/* current read length */
+	size_t readlen = count ? : range;
+	/* offset to the first erase block (aligned) */
+	off_t erase_offset;
+	/* offset inside the erase block to the start of the data */
+	off_t block_seek;
+	/* running start of the current block - MEMGETBADBLOCK needs 64 bits */
+	loff_t blockstart;
+	/* mode used, when reading out all "good" blocks from the range */
+	int backup_mode = !count;
 	int rc;
 
-	rc = lseek (fd, offset, SEEK_SET);
-	if (rc == -1) {
-		fprintf (stderr,
-			 "seek error on %s: %s\n",
-			 DEVNAME (dev), strerror (errno));
+	if (!count)
+		count = range;
+
+	rc = ioctl (fd, MEMGETINFO, &mtdinfo);
+	if (rc < 0) {
+		perror ("Cannot get MTD information");
 		return rc;
 	}
 
-	rc = read (fd, buf, count);
-	if (rc != count) {
-		fprintf (stderr,
-			 "Read error on %s: %s\n",
-			 DEVNAME (dev), strerror (errno));
-		return -1;
+	/* Erase sector size is always a power of 2 */
+	erase_offset = offset & ~(DEVESIZE (dev) - 1);
+
+	blockstart = erase_offset;
+	/* Offset inside a block */
+	block_seek = offset - erase_offset;
+
+	if (mtdinfo.type == MTD_NANDFLASH) {
+		/*
+		 * NAND: calculate which blocks we are reading. We have
+		 * to read one block@a time to skip bad blocks.
+		 */
+		blocklen = DEVESIZE (dev);
+		/* Limit to one block for the first read */
+		if (readlen > blocklen - block_seek)
+			readlen = blocklen - block_seek;
+	} else {
+		blocklen = 0;
 	}
 
-	return rc;
+	/* This only runs once for NOR flash */
+	while (processed < count) {
+		rc = flash_bad_block (dev, fd, &mtdinfo, &blockstart, blocklen);
+		if (rc < 0) {
+			return -1;
+		} else if (blockstart + block_seek + readlen >
+			   erase_offset + range) {
+			/* End of range is reached */
+			if (backup_mode) {
+				return processed;
+			} else {
+				fprintf (stderr,
+					 "Too few good blocks within range\n");
+				return -1;
+			}
+		} else if (rc) {
+			continue;
+		}
+
+		/*
+		 * If a block is bad, we retry in the next block
+		 * at the same offset - see common/env_nand.c::
+		 * writeenv()
+		 */
+		lseek (fd, blockstart + block_seek, SEEK_SET);
+
+		rc = read (fd, buf + processed, readlen);
+		if (rc != readlen) {
+			fprintf (stderr, "Read error on %s: %s\n",
+				 DEVNAME (dev), strerror (errno));
+			return -1;
+		}
+#ifdef DEBUG
+		fprintf (stderr, "Read 0x%x bytes at 0x%llx\n",
+			 rc, blockstart + block_seek);
+#endif
+		processed += readlen;
+		readlen = min(blocklen, count - processed);
+		block_seek = 0;
+		blockstart += blocklen;
+	}
+
+	return processed;
 }
 
-static int flash_write (void)
+/*
+ * Write count bytes at offset, but stay within
+ * ENVSETCORS (dev) sectors of DEVOFFSET (dev)
+ */
+static int __flash_write_buf (int dev, int fd, void *buf, size_t count,
+			      off_t offset, struct mtd_info_user *mtdinfo)
 {
-	int fd_current, fd_target, rc, dev_target;
-	erase_info_t erase_current = {}, erase_target;
 	char *data = NULL;
+	erase_info_t erase;
+	/* erase / write length - one block on NAND, whole area on NOR */
+	size_t blocklen;
+	/* whole area that can be erased - may include bad blocks */
+	size_t erase_len;
+	/* length of erase sector */
+	size_t erasesize;
+	/* progress counter */
+	size_t processed = 0;
+	/* total size to actually write - excludinig bad blocks */
+	size_t write_total;
+	/* offset to the first erase block (aligned) below offset */
 	off_t erase_offset;
-	struct mtd_info_user mtdinfo_target;
+	/* offset inside the erase block to the start of the data */
+	off_t block_seek;
+	/* end of the last block we may use */
+	off_t top_of_range;
+	/* running start of the current block - MEMGETBADBLOCK needs 64 bits */
+	loff_t blockstart;
+	int rc;
 
-	/* dev_current: fd_current, erase_current */
-	if ((fd_current = open (DEVNAME (dev_current), O_RDWR)) < 0) {
-		fprintf (stderr,
-			 "Can't open %s: %s\n",
-			 DEVNAME (dev_current), strerror (errno));
-		return -1;
-	}
+	erasesize = DEVESIZE (dev);
 
-	if (HaveRedundEnv) {
-		/* switch to next partition for writing */
-		dev_target = !dev_current;
-		/* dev_target: fd_target, erase_target */
-		if ((fd_target = open (DEVNAME (dev_target), O_RDWR)) < 0) {
-			fprintf (stderr,
-				 "Can't open %s: %s\n",
-				 DEVNAME (dev_target),
-				 strerror (errno));
-			return -1;
-		}
-	} else {
-		dev_target = dev_current;
-		fd_target = fd_current;
-	}
+	/* Erase sector size is always a power of 2 */
+	top_of_range = (DEVOFFSET (dev) & ~(erasesize - 1)) +
+		ENVSECTORS (dev) * erasesize;
+
+	erase_offset = offset & ~(erasesize - 1);
+
+	/* Maximum area we may use */
+	erase_len = top_of_range - erase_offset;
+
+	blockstart = erase_offset;
+	/* Offset inside a block */
+	block_seek = offset - erase_offset;
 
 	/*
-	 * Support environment anywhere within erase sectors: read out the
-	 * complete area to be erased, replace the environment image, write
-	 * the whole block back again.
+	 * Support data 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 (erase_len > count) {
+		data = malloc (erase_len);
 		if (!data) {
 			fprintf (stderr,
-				 "Cannot malloc %lu bytes: %s\n",
-				 DEVESIZE (dev_target),
-				 strerror (errno));
+				 "Cannot malloc %u bytes: %s\n",
+				 erase_len, strerror (errno));
 			return -1;
 		}
 
-		rc = ioctl (fd_target, MEMGETINFO, &mtdinfo_target);
-		if (rc < 0) {
-			perror ("Cannot get MTD information");
+		/*
+		 * This is different from a normal read. We have to read as much
+		 * as we can from a certain area, and it should be@least X
+		 * bytes, instead of having to read a fixed number of bytes as
+		 * usual. This also tells us how much data "fits" in the good
+		 * blocks in the area.
+		 */
+		write_total = flash_read_buf (dev, fd, data, 0,
+					      erase_offset, erase_len);
+		if (write_total < block_seek + CFG_ENV_SIZE)
 			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);
+		memcpy(data + block_seek, buf, count);
 	} else {
+		if (erase_offset != offset || erase_len != count) {
+			fprintf (stderr, "Data doesn\'t fit in the sectors!\n");
+			return -1;
+		}
 		data = (char *)environment.image;
-		erase_offset = DEVOFFSET (dev_target);
+		write_total = count;
 	}
 
-	printf ("Unlocking flash...\n");
-	erase_target.length = DEVESIZE (dev_target);
-	erase_target.start = DEVOFFSET (dev_target);
-	ioctl (fd_target, MEMUNLOCK, &erase_target);
+	if (mtdinfo->type == MTD_NANDFLASH)
+		/*
+		 * NAND: calculate which blocks we are writing. We have
+		 * to write one block at a time to skip bad blocks.
+		 */
+		blocklen = erasesize;
+	else
+		blocklen = erase_len;
 
-	if (HaveRedundEnv) {
-		erase_current.length = DEVESIZE (dev_current);
-		erase_current.start = DEVOFFSET (dev_current);
-		ioctl (fd_current, MEMUNLOCK, &erase_current);
-		ENV_FLAGS(environment) = active_flag;
+	erase.length = blocklen;
+
+	while (processed < write_total) {
+		rc = flash_bad_block (dev, fd, mtdinfo, &blockstart, blocklen);
+		if (rc < 0) {
+			return rc;
+		} else if (blockstart + blocklen > top_of_range) {
+			fprintf (stderr, "End of range reached, aborting\n");
+			return -1;
+		} else if (rc) {
+			continue;
+		}
+
+		erase.start = blockstart;
+		ioctl (fd, MEMUNLOCK, &erase);
+
+		if (ioctl (fd, MEMERASE, &erase) != 0) {
+			fprintf (stderr, "MTD erase error on %s: %s\n",
+				 DEVNAME (dev),
+				 strerror (errno));
+			return -1;
+		}
+
+		if (lseek (fd, blockstart, SEEK_SET) == -1) {
+			fprintf (stderr,
+				 "Seek error on %s: %s\n",
+				 DEVNAME (dev), strerror (errno));
+			return -1;
+		}
+
+#ifdef DEBUG
+		printf ("Writing 0x%x bytes at 0x%llx\n", blocklen, blockstart);
+#endif
+		if (write (fd, data + processed, blocklen) != blocklen) {
+			fprintf (stderr, "Write error on %s: %s\n",
+				 DEVNAME (dev), strerror (errno));
+			return -1;
+		}
+
+		ioctl (fd, MEMLOCK, &erase);
+
+		processed += blocklen;
+		block_seek = 0;
+		blockstart += blocklen;
 	}
 
-	printf ("Done\n");
+	if (erase_len > CFG_ENV_SIZE)
+		free (data);
+
+	return processed;
+}
 
-	printf ("Erasing old environment...\n");
+static int flash_write_buf (int dev, int fd, void *buf, size_t count,
+			    off_t offset)
+{
+	struct mtd_info_user mtdinfo;
+	int rc;
 
-	if (ioctl (fd_target, MEMERASE, &erase_target) != 0) {
-		fprintf (stderr, "MTD erase error on %s: %s\n",
-			 DEVNAME (dev_target),
-			 strerror (errno));
+	rc = ioctl (fd, MEMGETINFO, &mtdinfo);
+	if (rc < 0) {
+		perror ("Cannot get MTD information");
 		return -1;
 	}
 
-	printf ("Done\n");
+	return __flash_write_buf (dev, fd, buf, count, offset, &mtdinfo);
+}
 
-	printf ("Writing environment to %s...\n", DEVNAME (dev_target));
-	if (lseek (fd_target, erase_offset, SEEK_SET) == -1) {
-		fprintf (stderr,
-			 "seek error on %s: %s\n",
-			 DEVNAME (dev_target), strerror (errno));
+static int flash_flag_obsolete (int dev, int fd, off_t offset)
+{
+	struct mtd_info_user mtdinfo;
+	int rc;
+
+	rc = ioctl (fd, MEMGETINFO, &mtdinfo);
+	if (rc < 0) {
+		perror ("Cannot get MTD information");
 		return -1;
 	}
 
-	if (write (fd_target, data, DEVESIZE (dev_target)) !=
-	    DEVESIZE (dev_target)) {
-		fprintf (stderr,
-			 "Write error on %s: %s\n",
-			 DEVNAME (dev_target), strerror (errno));
-		return -1;
+	if (mtdinfo.type == MTD_NANDFLASH) {
+		/*
+		 * No luck on NAND - we could save the erase, but have to write
+		 * a whole erase block anyway
+		 */
+		rc = __flash_write_buf (dev, fd, &obsolete_flag,
+					sizeof (obsolete_flag), offset,
+					&mtdinfo);
+	} else {
+		/* This relies on the fact, that obsolete_flag == 0 */
+		rc = lseek (fd, offset, SEEK_SET);
+		if (rc < 0) {
+			fprintf (stderr, "Cannot seek to set the flag on %s \n",
+				 DEVNAME (dev));
+			return rc;
+		}
+		rc = write (fd, &obsolete_flag, sizeof (obsolete_flag));
+		if (rc < 0)
+			perror ("Could not set obsolete flag");
 	}
+	return rc;
+}
 
-	if (DEVESIZE (dev_target) > CFG_ENV_SIZE)
-		free (data);
+static int flash_write (void)
+{
+	int fd_current, fd_target, rc, dev_target;
 
-	if (HaveRedundEnv) {
-		/* change flag on current active env partition */
-		if (lseek (fd_current, DEVOFFSET (dev_current) + sizeof (ulong),
-			   SEEK_SET) == -1) {
-			fprintf (stderr, "seek error on %s: %s\n",
-				 DEVNAME (dev_current), strerror (errno));
-			return -1;
-		}
-		if (write (fd_current, &obsolete_flag,
-			   sizeof (obsolete_flag)) != sizeof (obsolete_flag)) {
-			fprintf (stderr,
-				 "Write error on %s: %s\n",
-				 DEVNAME (dev_current), strerror (errno));
-			return -1;
-		}
+	/* dev_current: fd_current, erase_current */
+	if ((fd_current = open (DEVNAME (dev_current), O_RDWR)) < 0) {
+		fprintf (stderr,
+			 "Can't open %s: %s\n",
+			 DEVNAME (dev_current), strerror (errno));
+		return -1;
 	}
-	printf ("Done\n");
-	printf ("Locking ...\n");
-	ioctl (fd_target, MEMLOCK, &erase_target);
+
 	if (HaveRedundEnv) {
-		ioctl (fd_current, MEMLOCK, &erase_current);
-		if (close (fd_target)) {
+		/* switch to next partition for writing */
+		dev_target = !dev_current;
+		/* dev_target: fd_target, erase_target */
+		if ((fd_target = open (DEVNAME (dev_target), O_RDWR)) < 0) {
 			fprintf (stderr,
-				 "I/O error on %s: %s\n",
+				 "Can't open %s: %s\n",
 				 DEVNAME (dev_target),
 				 strerror (errno));
 			return -1;
 		}
+		ENV_FLAGS(environment) = active_flag;
+	} else {
+		dev_target = dev_current;
+		fd_target = fd_current;
+	}
+
+	printf ("Writing new environment at 0x%lx\n", DEVOFFSET (dev_target));
+	rc = flash_write_buf (dev_target, fd_target, environment.image,
+			      CFG_ENV_SIZE, DEVOFFSET (dev_target));
+	if (rc < 0)
+		return rc;
+
+	if (HaveRedundEnv) {
+		off_t offset = DEVOFFSET (dev_current) +
+			offsetof(union env_image, redund.flags);
+		printf ("Setting obsolete flag for environment@0x%lx\n",
+			DEVOFFSET (dev_current));
+		flash_flag_obsolete(dev_current, fd_current, offset);
 	}
-	printf ("Done\n");
 
-	if (close (fd_current)) {
+	if (close (fd_target)) {
 		fprintf (stderr,
 			 "I/O error on %s: %s\n",
-			 DEVNAME (dev_current), strerror (errno));
+			 DEVNAME (dev_target), strerror (errno));
 		return -1;
 	}
 
@@ -604,10 +803,10 @@ static int flash_read (void)
 		return -1;
 	}
 
+	/* Only try within CFG_ENV_RANGE */
 	rc = flash_read_buf (dev_current, fd, environment.image, CFG_ENV_SIZE,
-			     DEVOFFSET (dev_current));
-	if (rc < 0)
-		return rc;
+			     DEVOFFSET (dev_current),
+			     ENVSECTORS (dev_current) * DEVESIZE (dev_current));
 
 	if (close (fd)) {
 		fprintf (stderr,
@@ -616,8 +815,7 @@ static int flash_read (void)
 		return -1;
 	}
 
-	/* everything ok */
-	return 0;
+	return rc < 0 ? rc : 0;
 }
 
 /*
@@ -649,13 +847,13 @@ static int env_init (void)
 	char flag1, flag2, *addr2;
 
 	if (parse_config ())		/* should fill envdevices */
-		return 1;
+		return -1;
 
 	if ((addr1 = calloc (1, CFG_ENV_SIZE)) == NULL) {
 		fprintf (stderr,
 			"Not enough memory for environment (%ld bytes)\n",
 			CFG_ENV_SIZE);
-		return errno;
+		return -1;
 	}
 
 	/* read environment from FLASH to local buffer */
@@ -663,9 +861,8 @@ static int env_init (void)
 	environment.data = HaveRedundEnv ? environment.image->redund.data :
 		environment.image->single.data;
 	dev_current = 0;
-	if (flash_read ()) {
-		return errno;
-	}
+	if (flash_read ())
+		return -1;
 
 	crc1 = crc32 (0, (uint8_t *) environment.data, ENV_SIZE);
 	crc1_ok = (crc1 == environment.image->single.crc);
@@ -683,13 +880,12 @@ static int env_init (void)
 			fprintf (stderr,
 				"Not enough memory for environment (%ld bytes)\n",
 				CFG_ENV_SIZE);
-			return errno;
+			return -1;
 		}
 		environment.image = (union env_image *)addr2;
 
-		if (flash_read ()) {
-			return errno;
-		}
+		if (flash_read ())
+			return -1;
 
 		crc2 = crc32 (0, (uint8_t *) environment.image->redund.data,
 			      ENV_SIZE);
@@ -767,18 +963,20 @@ static int parse_config ()
 	if (get_config (CONFIG_FILE)) {
 		fprintf (stderr,
 			"Cannot parse config file: %s\n", strerror (errno));
-		return 1;
+		return -1;
 	}
 #else
 	strcpy (DEVNAME (0), DEVICE1_NAME);
 	DEVOFFSET (0) = DEVICE1_OFFSET;
 	ENVSIZE (0) = ENV1_SIZE;
 	DEVESIZE (0) = DEVICE1_ESIZE;
+	ENVSECTORS (0) = DEVICE1_ENVSECTORS;
 #ifdef HAVE_REDUND
 	strcpy (DEVNAME (1), DEVICE2_NAME);
 	DEVOFFSET (1) = DEVICE2_OFFSET;
 	ENVSIZE (1) = ENV2_SIZE;
 	DEVESIZE (1) = DEVICE2_ESIZE;
+	ENVSECTORS (1) = DEVICE2_ENVSECTORS;
 	HaveRedundEnv = 1;
 #endif
 #endif
@@ -786,14 +984,14 @@ static int parse_config ()
 		fprintf (stderr,
 			"Cannot access MTD device %s: %s\n",
 			DEVNAME (0), strerror (errno));
-		return 1;
+		return -1;
 	}
 
 	if (HaveRedundEnv && stat (DEVNAME (1), &st)) {
 		fprintf (stderr,
 			"Cannot access MTD device %s: %s\n",
 			DEVNAME (1), strerror (errno));
-		return 1;
+		return -1;
 	}
 	return 0;
 }
@@ -806,21 +1004,27 @@ static int get_config (char *fname)
 	int rc;
 	char dump[128];
 
-	if ((fp = fopen (fname, "r")) == NULL) {
-		return 1;
-	}
-
-	while ((i < 2) && ((rc = fscanf (fp, "%s %lx %lx %lx",
-				  DEVNAME (i),
-				  &DEVOFFSET (i),
-				  &ENVSIZE (i),
-				  &DEVESIZE (i)  )) != EOF)) {
+	if ((fp = fopen (fname, "r")) == NULL)
+		return -1;
 
+	while (i < 2 && fgets (dump, sizeof (dump), fp)) {
 		/* Skip incomplete conversions and comment strings */
-		if ((rc < 3) || (*DEVNAME (i) == '#')) {
-			fgets (dump, sizeof (dump), fp);	/* Consume till end */
+		if (dump[0] == '#')
 			continue;
-		}
+
+		rc = sscanf (dump, "%s %lx %lx %lx %lx",
+			     DEVNAME (i),
+			     &DEVOFFSET (i),
+			     &ENVSIZE (i),
+			     &DEVESIZE (i),
+			     &ENVSECTORS (i));
+
+		if (rc < 4)
+			continue;
+
+		if (rc < 5)
+			/* Default - 1 sector */
+			ENVSECTORS (i) = 1;
 
 		i++;
 	}
@@ -829,7 +1033,7 @@ static int get_config (char *fname)
 	HaveRedundEnv = i - 1;
 	if (!i) {			/* No valid entries found */
 		errno = EINVAL;
-		return 1;
+		return -1;
 	} else
 		return 0;
 }
diff --git a/tools/env/fw_env.config b/tools/env/fw_env.config
index 2432bd8..0fe37c9 100644
--- a/tools/env/fw_env.config
+++ b/tools/env/fw_env.config
@@ -1,7 +1,11 @@
 # Configuration file for fw_(printenv/saveenv) utility.
 # Up to two entries are valid, in this case the redundand
 # environment sector is assumed present.
+# Notice, that the "Number of sectors" is ignored on NOR.
 
-# MTD device name	Device offset	Env. size	Flash sector size
+# MTD device name	Device offset	Env. size	Flash sector size	Number of sectors
 /dev/mtd1		0x0000		0x4000		0x4000
 /dev/mtd2		0x0000		0x4000		0x4000
+
+# NAND example
+#/dev/mtd0		0x4000		0x4000		0x20000			2
-- 
1.5.4

[U-Boot] [PATCH 6/6] Support environment in NAND

[Wolfgang Denk]

Dear Guennadi Liakhovetski,

In message <Pine.LNX.4.64.0808271748520.6718@axis700.grange> you wrote:
>
> --- a/tools/env/fw_env.c
> +++ b/tools/env/fw_env.c
> @@ -44,6 +44,12 @@
>  #define	CMD_GETENV	"fw_printenv"
>  #define	CMD_SETENV	"fw_setenv"
>  
> +#define min(x, y) ({				\
> +	typeof(x) _min1 = (x);			\
> +	typeof(y) _min2 = (y);			\
> +	(void) (&_min1 == &_min2);		\

What does this do?

> +	_min1 < _min2 ? _min1 : _min2; })


>  typedef struct envdev_s {
>  	char devname[16];		/* Device name */
>  	ulong devoff;			/* Device offset */
> @@ -413,179 +419,290 @@ int fw_setenv (int argc, char *argv[])
>  	return 0;
>  }
>  
> +static int flash_bad_block (int dev, int fd, struct mtd_info_user *mtdinfo,
> +			    loff_t *blockstart, size_t blocklen)
> +{
> +	if (mtdinfo->type == MTD_NANDFLASH) {
> +		int badblock = ioctl (fd, MEMGETBADBLOCK, blockstart);
> +
> +		if (badblock < 0) {
> +			perror ("Cannot read bad block mark");

It would be probably helpful to print the block address.

> +			return badblock;
> +		}
> +
> +		if (badblock) {
> +			fprintf (stderr, "Bad block at 0x%llx, "
> +				 "skipping\n", *blockstart);
> +			*blockstart += blocklen;
> +			return badblock;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * We are called with count == 0 for backing up as much data from the
> + * range as possible
> + */

Backing up?

>  static int flash_read_buf (int dev, int fd, void *buf, size_t count,
> -			   off_t offset)
> +			   off_t offset, size_t range)
>  {
> +	struct mtd_info_user mtdinfo;
> +	size_t blocklen, processed = 0;
> +	size_t readlen = count ? : range;
> +	off_t erase_offset, block_seek;
> +	loff_t blockstart;
>  	int rc;
> +	int backup_mode = !count;

backup_mode ?

I think there should be an explanation what exactly you are trying to
do.

> -	rc = lseek (fd, offset, SEEK_SET);
> -	if (rc == -1) {
> -		fprintf (stderr,
> -			 "seek error on %s: %s\n",
> -			 DEVNAME (dev), strerror (errno));
> +	if (!count)
> +		count = range;
> +
> +	rc = ioctl (fd, MEMGETINFO, &mtdinfo);
> +	if (rc < 0) {
> +		perror ("Cannot get MTD information");
>  		return rc;
>  	}

Did you verify that the code still builds when MTD_OLD is set?

>  
> -	rc = read (fd, buf, count);
> -	if (rc != count) {
> -		fprintf (stderr,
> -			 "Read error on %s: %s\n",
> -			 DEVNAME (dev), strerror (errno));
> -		return -1;
> +	/* Erase sector size is always a power of 2 */
> +	erase_offset = offset & ~(mtdinfo.erasesize - 1);

Please explain this logic.

> +	blockstart = erase_offset;
> +	/* Offset inside a block */
> +	block_seek = offset - erase_offset;
> +
> +	if (mtdinfo.type == MTD_NANDFLASH) {
> +		/*
> +		 * NAND: calculate which blocks we are reading. We have
> +		 * to read one block at a time to skip bad blocks.
> +		 */
> +		blocklen = mtdinfo.erasesize;
> +		/* Limit to one block for the first read */
> +		if (readlen > blocklen - block_seek)
> +			readlen = blocklen - block_seek;
> +	} else {
> +		blocklen = 0;
>  	}
>  
> -	return rc;
> +	/* This only runs once for NOR flash */
> +	while (processed < count) {
> +		rc = flash_bad_block (dev, fd, &mtdinfo, &blockstart, blocklen);

But - NOR flash does not have bad block, so all of this is not needed
at all?

> +		if (rc < 0)
> +			return -1;
> +		else if (blockstart + block_seek + readlen > offset + range) {

I do not understand what you are doing here. Comment?

> +			/* End of range is reached */
> +			if (backup_mode) {
> +				return processed;
> +			} else {
> +				fprintf (stderr,
> +					 "Too few good blocks within range\n");
> +				return -1;
> +			}
> +		} else if (rc)
> +			continue;
> +
> +		/*
> +		 * If a block is bad, we retry in the next block
> +		 * at the same offset - see common/env_nand.c::
> +		 * writeenv()
> +		 */
> +		lseek (fd, blockstart + block_seek, SEEK_SET);

I don't see that you remember which blocks were bad. Does that mean
that you will attemopt to write the environment to known bad blocks?
Sonds not like a good idea to me.

> +		rc = read (fd, buf + processed, readlen);
> +		if (rc != readlen) {
> +			fprintf (stderr,
> +				 "Read error on %s: %s\n",
> +				 DEVNAME (dev), strerror (errno));
> +			return -1;
> +		}
> +		processed += readlen;
> +		readlen = min(blocklen, count - processed);
> +		block_seek = 0;
> +		blockstart += blocklen;
> +	}
> +
> +	return processed;
>  }
>  
> -static int flash_write (void)
> +static int flash_write_buf (int dev, int fd, void *buf, size_t count,
> +			    off_t offset)
>  {
> -	int fd_current, fd_target, rc, dev_target;
> -	erase_info_t erase_current = {}, erase_target;
>  	char *data = NULL;
> -	off_t erase_offset;
> -	struct mtd_info_user mtdinfo_target;
> +	erase_info_t erase;
> +	struct mtd_info_user mtdinfo;
> +	size_t blocklen, erase_len, processed = 0;
> +	size_t writelen, write_total = DEVESIZE (dev);
> +	off_t erase_offset, block_seek;
> +	loff_t blockstart;
> +	int rc;
>  
> -	/* dev_current: fd_current, erase_current */
> -	if ((fd_current = open (DEVNAME (dev_current), O_RDWR)) < 0) {
> -		fprintf (stderr,
> -			 "Can't open %s: %s\n",
> -			 DEVNAME (dev_current), strerror (errno));
> +	rc = ioctl (fd, MEMGETINFO, &mtdinfo);
> +	if (rc < 0) {
> +		perror ("Cannot get MTD information");
>  		return -1;
>  	}
>  
> -	if (HaveRedundEnv) {
> -		/* switch to next partition for writing */
> -		dev_target = !dev_current;
> -		/* dev_target: fd_target, erase_target */
> -		if ((fd_target = open (DEVNAME (dev_target), O_RDWR)) < 0) {
> -			fprintf (stderr,
> -				 "Can't open %s: %s\n",
> -				 DEVNAME (dev_target),
> -				 strerror (errno));
> -			return -1;
> -		}
> -	} else {
> -		dev_target = dev_current;
> -		fd_target = fd_current;
> -	}
> +	/* Erase sector size is always a power of 2 */
> +	erase_offset = offset & ~(mtdinfo.erasesize - 1);
> +	/* Maximum area we may use */
> +	erase_len = (offset - erase_offset + DEVESIZE (dev) +
> +		     mtdinfo.erasesize - 1) & ~(mtdinfo.erasesize - 1);
> +
> +	blockstart = erase_offset;
> +	/* Offset inside a block */
> +	block_seek = offset - erase_offset;
>  
>  	/*
>  	 * 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 (erase_len > DEVESIZE (dev)) {
> +		data = malloc (erase_len);
>  		if (!data) {
>  			fprintf (stderr,
> -				 "Cannot malloc %lu bytes: %s\n",
> -				 DEVESIZE (dev_target),
> -				 strerror (errno));
> +				 "Cannot malloc %u bytes: %s\n",
> +				 erase_len, strerror (errno));
>  			return -1;
>  		}
>  
> -		rc = ioctl (fd_target, MEMGETINFO, &mtdinfo_target);
> -		if (rc < 0) {
> -			perror ("Cannot get MTD information");
> +		/*
> +		 * This is different from a normal read. We have to read as much
> +		 * as we can from a certain area, and it should be at least X
> +		 * bytes, instead of having to read a fixed number of bytes as
> +		 * usual. This also tells us how much data "fits" in the good
> +		 * blocks in the area.
> +		 */
> +		write_total = flash_read_buf (dev, fd, data, 0,
> +					      erase_offset, erase_len);
> +		if (write_total < block_seek + CFG_ENV_SIZE)

Ummm...this is flash_write_buf(), and we start reading data?

Please explain your code.

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: wd@denx.de
It would seem that evil retreats when forcibly confronted
	-- Yarnek of Excalbia, "The Savage Curtain", stardate 5906.5

[U-Boot] [PATCH 6/6] Support environment in NAND

[Guennadi Liakhovetski]

On Sun, 31 Aug 2008, Wolfgang Denk wrote:

> Dear Guennadi Liakhovetski,
> 
> In message <Pine.LNX.4.64.0808271748520.6718@axis700.grange> you wrote:
> >
> > --- a/tools/env/fw_env.c
> > +++ b/tools/env/fw_env.c
> > @@ -44,6 +44,12 @@
> >  #define	CMD_GETENV	"fw_printenv"
> >  #define	CMD_SETENV	"fw_setenv"
> >  
> > +#define min(x, y) ({				\
> > +	typeof(x) _min1 = (x);			\
> > +	typeof(y) _min2 = (y);			\
> > +	(void) (&_min1 == &_min2);		\
> 
> What does this do?

This min definition is copied from Linux. This "useless" comparison 
operation forces the compiler to verify type compatibility of the two 
parameters.

> > +	_min1 < _min2 ? _min1 : _min2; })
> 
> 
> >  typedef struct envdev_s {
> >  	char devname[16];		/* Device name */
> >  	ulong devoff;			/* Device offset */
> > @@ -413,179 +419,290 @@ int fw_setenv (int argc, char *argv[])
> >  	return 0;
> >  }
> >  
> > +static int flash_bad_block (int dev, int fd, struct mtd_info_user *mtdinfo,
> > +			    loff_t *blockstart, size_t blocklen)
> > +{
> > +	if (mtdinfo->type == MTD_NANDFLASH) {
> > +		int badblock = ioctl (fd, MEMGETBADBLOCK, blockstart);
> > +
> > +		if (badblock < 0) {
> > +			perror ("Cannot read bad block mark");
> 
> It would be probably helpful to print the block address.

Ok, can do.

> 
> > +			return badblock;
> > +		}
> > +
> > +		if (badblock) {
> > +			fprintf (stderr, "Bad block at 0x%llx, "
> > +				 "skipping\n", *blockstart);
> > +			*blockstart += blocklen;
> > +			return badblock;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * We are called with count == 0 for backing up as much data from the
> > + * range as possible
> > + */
> 
> Backing up?

As explained before - I am preserving the data outside of the environment 
and I call this procedure backing-up.

> >  static int flash_read_buf (int dev, int fd, void *buf, size_t count,
> > -			   off_t offset)
> > +			   off_t offset, size_t range)
> >  {
> > +	struct mtd_info_user mtdinfo;
> > +	size_t blocklen, processed = 0;
> > +	size_t readlen = count ? : range;
> > +	off_t erase_offset, block_seek;
> > +	loff_t blockstart;
> >  	int rc;
> > +	int backup_mode = !count;
> 
> backup_mode ?
> 
> I think there should be an explanation what exactly you are trying to
> do.

I'll try to improve comments.

> > -	rc = lseek (fd, offset, SEEK_SET);
> > -	if (rc == -1) {
> > -		fprintf (stderr,
> > -			 "seek error on %s: %s\n",
> > -			 DEVNAME (dev), strerror (errno));
> > +	if (!count)
> > +		count = range;
> > +
> > +	rc = ioctl (fd, MEMGETINFO, &mtdinfo);
> > +	if (rc < 0) {
> > +		perror ("Cannot get MTD information");
> >  		return rc;
> >  	}
> 
> Did you verify that the code still builds when MTD_OLD is set?

No. If we separate the NAND tool - does it still have to build with this 
flag? Will anyone want to build it with older kernels?

> > -	rc = read (fd, buf, count);
> > -	if (rc != count) {
> > -		fprintf (stderr,
> > -			 "Read error on %s: %s\n",
> > -			 DEVNAME (dev), strerror (errno));
> > -		return -1;
> > +	/* Erase sector size is always a power of 2 */
> > +	erase_offset = offset & ~(mtdinfo.erasesize - 1);
> 
> Please explain this logic.

Ok, will do.

> > +	blockstart = erase_offset;
> > +	/* Offset inside a block */
> > +	block_seek = offset - erase_offset;
> > +
> > +	if (mtdinfo.type == MTD_NANDFLASH) {
> > +		/*
> > +		 * NAND: calculate which blocks we are reading. We have
> > +		 * to read one block at a time to skip bad blocks.
> > +		 */
> > +		blocklen = mtdinfo.erasesize;
> > +		/* Limit to one block for the first read */
> > +		if (readlen > blocklen - block_seek)
> > +			readlen = blocklen - block_seek;
> > +	} else {
> > +		blocklen = 0;
> >  	}
> >  
> > -	return rc;
> > +	/* This only runs once for NOR flash */
> > +	while (processed < count) {
> > +		rc = flash_bad_block (dev, fd, &mtdinfo, &blockstart, blocklen);
> 
> But - NOR flash does not have bad block, so all of this is not needed
> at all?

See function implementation. It just returns 0 in non-NAND case. There are 
two possibilities: either

	if (NAND)
		verify_bad_block();

or

	verify_bad_block();

int verify_bad_block()
{
	if (!NAND)
		return 0;

	...
}

in Linux the latter is generally preferred, as it doesn't clutter the 
caller's flow.

> > +		if (rc < 0)
> > +			return -1;
> > +		else if (blockstart + block_seek + readlen > offset + range) {
> 
> I do not understand what you are doing here. Comment?

The comment is one line below:

> > +			/* End of range is reached */

If this is not enough, I can try to improve it.

> > +			if (backup_mode) {
> > +				return processed;
> > +			} else {
> > +				fprintf (stderr,
> > +					 "Too few good blocks within range\n");
> > +				return -1;
> > +			}
> > +		} else if (rc)
> > +			continue;
> > +
> > +		/*
> > +		 * If a block is bad, we retry in the next block
> > +		 * at the same offset - see common/env_nand.c::
> > +		 * writeenv()
> > +		 */
> > +		lseek (fd, blockstart + block_seek, SEEK_SET);
> 
> I don't see that you remember which blocks were bad. Does that mean
> that you will attemopt to write the environment to known bad blocks?
> Sonds not like a good idea to me.

I don't have to remember it. It is the "else if (rc)" case above - the bad 
block is just skipped.

> > -		rc = ioctl (fd_target, MEMGETINFO, &mtdinfo_target);
> > -		if (rc < 0) {
> > -			perror ("Cannot get MTD information");
> > +		/*
> > +		 * This is different from a normal read. We have to read as much
> > +		 * as we can from a certain area, and it should be at least X
> > +		 * bytes, instead of having to read a fixed number of bytes as
> > +		 * usual. This also tells us how much data "fits" in the good
> > +		 * blocks in the area.
> > +		 */
> > +		write_total = flash_read_buf (dev, fd, data, 0,
> > +					      erase_offset, erase_len);
> > +		if (write_total < block_seek + CFG_ENV_SIZE)
> 
> Ummm...this is flash_write_buf(), and we start reading data?
> 
> Please explain your code.

That's exactly what the comment above is rying to do. Will try to improve 
it.

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

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

[Wolfgang Denk]

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
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:20: error: parse error before "uint32_t"
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:20: warning: no semicolon at end of struct or union
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:21: warning: type defaults to `int' in declaration of `length'
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:21: warning: data definition has no type or storage class
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:23: error: parse error before '}' token
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:57: error: parse error before "uint8_t"
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:57: warning: no semicolon at end of struct or union
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:58: warning: type defaults to `int' in declaration of `flags'
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:58: warning: data definition has no type or storage class
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:59: error: parse error before "size"
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:59: warning: type defaults to `int' in declaration of `size'
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:59: warning: data definition has no type or storage class
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:60: error: parse error before "erasesize"
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:60: warning: type defaults to `int' in declaration of `erasesize'
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:60: warning: data definition has no type or storage class
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:61: error: parse error before "writesize"
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:61: warning: type defaults to `int' in declaration of `writesize'
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:61: warning: data definition has no type or storage class
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:62: error: parse error before "oobsize"
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:62: warning: type defaults to `int' in declaration of `oobsize'
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:62: warning: data definition has no type or storage class
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:65: error: parse error before "ecctype"
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:65: warning: type defaults to `int' in declaration of `ecctype'
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:65: warning: data definition has no type or storage class
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:66: error: parse error before "eccsize"
/home/wd/git/u-boot/tmp/include/linux/mtd/mtd-abi.h:66: warning: type defaults to `int' in declaration of `eccsize'
...

etc. etc.

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: wd at denx.de
Two wrongs don't make a right, but three rights make a left.

[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.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 at denx.de

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

[Wolfgang Denk]

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.


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.

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: wd at denx.de
Copy from one, it's plagiarism; copy from two, it's research.

[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@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

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

[Guennadi Liakhovetski]

On Mon, 1 Sep 2008, Guennadi Liakhovetski wrote:

> > 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.

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.

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

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

[Wolfgang Denk]

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

-- 
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: wd at denx.de
I don't see any direct evidence ...  but, then, my crystal ball is in
dire need of an ectoplasmic upgrade. :-)              -- Howard Smith

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

[Wolfgang Denk]

Dear Guennadi Liakhovetski,

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.

> 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.

> 5. clarify back-up mode
> 
> This is actually a comment improvement, can do.

Actually the whole implementation needs to be explained.

> 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.

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: wd at denx.de
"Plan to throw one away. You will anyway."
                              - Fred Brooks, "The Mythical Man Month"

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

[Guennadi Liakhovetski]

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

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

[Wolfgang Denk]

Dear Guennadi Liakhovetski,

In message <Pine.LNX.4.64.0809020104080.8933@axis700.grange> you wrote:
> 
> 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 

Agreed so far.

> 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

Agreed.

> (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

Agreed that this is even worse than a union.

> (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 

None of them, I think.

> is a fourth possibility I am still overseeing.

The  two  cases  (redundant  versus  non-redundant  env)   are   well
separated,  and  known  early  (after  parsing the config file, i. e.
before any processing of environment data).

How about defining two structs,  one  without  the  flag  byte  (non-
redundant   env),  and  another  one  with  the  flag  byte  included
(redundant env). Then just use a pointer of the correct type (either
first or second struct) to access the data.

> 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.

But it's bogus. Now you have data[] in the union, *plus* in the
struct. You have it twice.

> > > 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?

Do we really need to ifdef this? 

> 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 

It will not. See my previous statistics. It will be  less  than  half
the size of your split patches.


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: wd at denx.de
This is now.  Later is later.

[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.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 at denx.de