* [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv
@ 2008-08-27 15:52 Guennadi Liakhovetski
2008-08-27 15:52 ` [U-Boot] [PATCH 1/6] Convert fw_env.c to use a single environment image union Guennadi Liakhovetski
` (6 more replies)
0 siblings, 7 replies; 40+ messages in thread
From: Guennadi Liakhovetski @ 2008-08-27 15:52 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 1/6] Convert fw_env.c to use a single environment image union
2008-08-27 15:52 [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv Guennadi Liakhovetski
@ 2008-08-27 15:52 ` Guennadi Liakhovetski
2008-08-31 14:36 ` Wolfgang Denk
2008-08-27 15:52 ` [U-Boot] [PATCH 2/6] Separate flash read and write operations Guennadi Liakhovetski
` (5 subsequent siblings)
6 siblings, 1 reply; 40+ messages in thread
From: Guennadi Liakhovetski @ 2008-08-27 15:52 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 2/6] Separate flash read and write operations
2008-08-27 15:52 [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv Guennadi Liakhovetski
2008-08-27 15:52 ` [U-Boot] [PATCH 1/6] Convert fw_env.c to use a single environment image union Guennadi Liakhovetski
@ 2008-08-27 15:52 ` Guennadi Liakhovetski
2008-08-31 14:58 ` Wolfgang Denk
2008-08-27 15:52 ` [U-Boot] [PATCH 3/6] "return" is not a function Guennadi Liakhovetski
` (4 subsequent siblings)
6 siblings, 1 reply; 40+ messages in thread
From: Guennadi Liakhovetski @ 2008-08-27 15:52 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 3/6] "return" is not a function
2008-08-27 15:52 [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv Guennadi Liakhovetski
2008-08-27 15:52 ` [U-Boot] [PATCH 1/6] Convert fw_env.c to use a single environment image union Guennadi Liakhovetski
2008-08-27 15:52 ` [U-Boot] [PATCH 2/6] Separate flash read and write operations Guennadi Liakhovetski
@ 2008-08-27 15:52 ` Guennadi Liakhovetski
2008-08-31 14:59 ` Wolfgang Denk
2008-08-27 15:52 ` [U-Boot] [PATCH 4/6] Unify active vs. redundant environment variable naming Guennadi Liakhovetski
` (3 subsequent siblings)
6 siblings, 1 reply; 40+ messages in thread
From: Guennadi Liakhovetski @ 2008-08-27 15:52 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 4/6] Unify active vs. redundant environment variable naming
2008-08-27 15:52 [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv Guennadi Liakhovetski
` (2 preceding siblings ...)
2008-08-27 15:52 ` [U-Boot] [PATCH 3/6] "return" is not a function Guennadi Liakhovetski
@ 2008-08-27 15:52 ` Guennadi Liakhovetski
2008-08-31 15:04 ` Wolfgang Denk
2008-08-27 15:52 ` [U-Boot] [PATCH 5/6] Support environment anywhere within erase area Guennadi Liakhovetski
` (2 subsequent siblings)
6 siblings, 1 reply; 40+ messages in thread
From: Guennadi Liakhovetski @ 2008-08-27 15:52 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 5/6] Support environment anywhere within erase area
2008-08-27 15:52 [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv Guennadi Liakhovetski
` (3 preceding siblings ...)
2008-08-27 15:52 ` [U-Boot] [PATCH 4/6] Unify active vs. redundant environment variable naming Guennadi Liakhovetski
@ 2008-08-27 15:52 ` Guennadi Liakhovetski
2008-08-31 18:57 ` Wolfgang Denk
2008-08-27 15:52 ` [U-Boot] [PATCH 6/6] Support environment in NAND Guennadi Liakhovetski
2008-08-31 20:21 ` [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv Wolfgang Denk
6 siblings, 1 reply; 40+ messages in thread
From: Guennadi Liakhovetski @ 2008-08-27 15:52 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 6/6] Support environment in NAND
2008-08-27 15:52 [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv Guennadi Liakhovetski
` (4 preceding siblings ...)
2008-08-27 15:52 ` [U-Boot] [PATCH 5/6] Support environment anywhere within erase area Guennadi Liakhovetski
@ 2008-08-27 15:52 ` Guennadi Liakhovetski
2008-08-29 9:29 ` [U-Boot] [PATCH 6/6 v2] " Guennadi Liakhovetski
2008-08-31 18:57 ` [U-Boot] [PATCH 6/6] " Wolfgang Denk
2008-08-31 20:21 ` [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv Wolfgang Denk
6 siblings, 2 replies; 40+ messages in thread
From: Guennadi Liakhovetski @ 2008-08-27 15:52 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 6/6 v2] Support environment in NAND
2008-08-27 15:52 ` [U-Boot] [PATCH 6/6] Support environment in NAND Guennadi Liakhovetski
@ 2008-08-29 9:29 ` Guennadi Liakhovetski
2008-08-31 18:57 ` [U-Boot] [PATCH 6/6] " Wolfgang Denk
1 sibling, 0 replies; 40+ messages in thread
From: Guennadi Liakhovetski @ 2008-08-29 9:29 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 1/6] Convert fw_env.c to use a single environment image union
2008-08-27 15:52 ` [U-Boot] [PATCH 1/6] Convert fw_env.c to use a single environment image union Guennadi Liakhovetski
@ 2008-08-31 14:36 ` Wolfgang Denk
2008-08-31 15:57 ` Guennadi Liakhovetski
0 siblings, 1 reply; 40+ messages in thread
From: Wolfgang Denk @ 2008-08-31 14:36 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 2/6] Separate flash read and write operations
2008-08-27 15:52 ` [U-Boot] [PATCH 2/6] Separate flash read and write operations Guennadi Liakhovetski
@ 2008-08-31 14:58 ` Wolfgang Denk
2008-08-31 16:04 ` Guennadi Liakhovetski
0 siblings, 1 reply; 40+ messages in thread
From: Wolfgang Denk @ 2008-08-31 14:58 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 3/6] "return" is not a function
2008-08-27 15:52 ` [U-Boot] [PATCH 3/6] "return" is not a function Guennadi Liakhovetski
@ 2008-08-31 14:59 ` Wolfgang Denk
2008-08-31 16:10 ` Guennadi Liakhovetski
0 siblings, 1 reply; 40+ messages in thread
From: Wolfgang Denk @ 2008-08-31 14:59 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 4/6] Unify active vs. redundant environment variable naming
2008-08-27 15:52 ` [U-Boot] [PATCH 4/6] Unify active vs. redundant environment variable naming Guennadi Liakhovetski
@ 2008-08-31 15:04 ` Wolfgang Denk
2008-08-31 16:18 ` Guennadi Liakhovetski
0 siblings, 1 reply; 40+ messages in thread
From: Wolfgang Denk @ 2008-08-31 15:04 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 1/6] Convert fw_env.c to use a single environment image union
2008-08-31 14:36 ` Wolfgang Denk
@ 2008-08-31 15:57 ` Guennadi Liakhovetski
2008-08-31 18:57 ` Wolfgang Denk
0 siblings, 1 reply; 40+ messages in thread
From: Guennadi Liakhovetski @ 2008-08-31 15:57 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 2/6] Separate flash read and write operations
2008-08-31 14:58 ` Wolfgang Denk
@ 2008-08-31 16:04 ` Guennadi Liakhovetski
2008-08-31 18:57 ` Wolfgang Denk
0 siblings, 1 reply; 40+ messages in thread
From: Guennadi Liakhovetski @ 2008-08-31 16:04 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 3/6] "return" is not a function
2008-08-31 14:59 ` Wolfgang Denk
@ 2008-08-31 16:10 ` Guennadi Liakhovetski
2008-08-31 18:57 ` Wolfgang Denk
0 siblings, 1 reply; 40+ messages in thread
From: Guennadi Liakhovetski @ 2008-08-31 16:10 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 4/6] Unify active vs. redundant environment variable naming
2008-08-31 15:04 ` Wolfgang Denk
@ 2008-08-31 16:18 ` Guennadi Liakhovetski
2008-08-31 18:57 ` Wolfgang Denk
0 siblings, 1 reply; 40+ messages in thread
From: Guennadi Liakhovetski @ 2008-08-31 16:18 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 3/6] "return" is not a function
2008-08-31 16:10 ` Guennadi Liakhovetski
@ 2008-08-31 18:57 ` Wolfgang Denk
2008-08-31 19:17 ` Guennadi Liakhovetski
0 siblings, 1 reply; 40+ messages in thread
From: Wolfgang Denk @ 2008-08-31 18:57 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 4/6] Unify active vs. redundant environment variable naming
2008-08-31 16:18 ` Guennadi Liakhovetski
@ 2008-08-31 18:57 ` Wolfgang Denk
2008-08-31 19:27 ` Guennadi Liakhovetski
0 siblings, 1 reply; 40+ messages in thread
From: Wolfgang Denk @ 2008-08-31 18:57 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 5/6] Support environment anywhere within erase area
2008-08-27 15:52 ` [U-Boot] [PATCH 5/6] Support environment anywhere within erase area Guennadi Liakhovetski
@ 2008-08-31 18:57 ` Wolfgang Denk
2008-08-31 19:39 ` Guennadi Liakhovetski
0 siblings, 1 reply; 40+ messages in thread
From: Wolfgang Denk @ 2008-08-31 18:57 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 2/6] Separate flash read and write operations
2008-08-31 16:04 ` Guennadi Liakhovetski
@ 2008-08-31 18:57 ` Wolfgang Denk
2008-08-31 19:45 ` Guennadi Liakhovetski
0 siblings, 1 reply; 40+ messages in thread
From: Wolfgang Denk @ 2008-08-31 18:57 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 1/6] Convert fw_env.c to use a single environment image union
2008-08-31 15:57 ` Guennadi Liakhovetski
@ 2008-08-31 18:57 ` Wolfgang Denk
0 siblings, 0 replies; 40+ messages in thread
From: Wolfgang Denk @ 2008-08-31 18:57 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 6/6] Support environment in NAND
2008-08-27 15:52 ` [U-Boot] [PATCH 6/6] Support environment in NAND Guennadi Liakhovetski
2008-08-29 9:29 ` [U-Boot] [PATCH 6/6 v2] " Guennadi Liakhovetski
@ 2008-08-31 18:57 ` Wolfgang Denk
2008-08-31 21:53 ` Guennadi Liakhovetski
1 sibling, 1 reply; 40+ messages in thread
From: Wolfgang Denk @ 2008-08-31 18:57 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 3/6] "return" is not a function
2008-08-31 18:57 ` Wolfgang Denk
@ 2008-08-31 19:17 ` Guennadi Liakhovetski
0 siblings, 0 replies; 40+ messages in thread
From: Guennadi Liakhovetski @ 2008-08-31 19:17 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 4/6] Unify active vs. redundant environment variable naming
2008-08-31 18:57 ` Wolfgang Denk
@ 2008-08-31 19:27 ` Guennadi Liakhovetski
2008-08-31 19:44 ` Wolfgang Denk
0 siblings, 1 reply; 40+ messages in thread
From: Guennadi Liakhovetski @ 2008-08-31 19:27 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 5/6] Support environment anywhere within erase area
2008-08-31 18:57 ` Wolfgang Denk
@ 2008-08-31 19:39 ` Guennadi Liakhovetski
2008-08-31 19:53 ` Wolfgang Denk
0 siblings, 1 reply; 40+ messages in thread
From: Guennadi Liakhovetski @ 2008-08-31 19:39 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 4/6] Unify active vs. redundant environment variable naming
2008-08-31 19:27 ` Guennadi Liakhovetski
@ 2008-08-31 19:44 ` Wolfgang Denk
0 siblings, 0 replies; 40+ messages in thread
From: Wolfgang Denk @ 2008-08-31 19:44 UTC (permalink / raw)
To: u-boot
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_
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 2/6] Separate flash read and write operations
2008-08-31 18:57 ` Wolfgang Denk
@ 2008-08-31 19:45 ` Guennadi Liakhovetski
2008-08-31 19:56 ` Wolfgang Denk
0 siblings, 1 reply; 40+ messages in thread
From: Guennadi Liakhovetski @ 2008-08-31 19:45 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 5/6] Support environment anywhere within erase area
2008-08-31 19:39 ` Guennadi Liakhovetski
@ 2008-08-31 19:53 ` Wolfgang Denk
0 siblings, 0 replies; 40+ messages in thread
From: Wolfgang Denk @ 2008-08-31 19:53 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 2/6] Separate flash read and write operations
2008-08-31 19:45 ` Guennadi Liakhovetski
@ 2008-08-31 19:56 ` Wolfgang Denk
0 siblings, 0 replies; 40+ messages in thread
From: Wolfgang Denk @ 2008-08-31 19:56 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv
2008-08-27 15:52 [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv Guennadi Liakhovetski
` (5 preceding siblings ...)
2008-08-27 15:52 ` [U-Boot] [PATCH 6/6] Support environment in NAND Guennadi Liakhovetski
@ 2008-08-31 20:21 ` Wolfgang Denk
2008-08-31 20:37 ` Guennadi Liakhovetski
2008-09-02 16:00 ` Guennadi Liakhovetski
6 siblings, 2 replies; 40+ messages in thread
From: Wolfgang Denk @ 2008-08-31 20:21 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv
2008-08-31 20:21 ` [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv Wolfgang Denk
@ 2008-08-31 20:37 ` Guennadi Liakhovetski
2008-08-31 20:55 ` Wolfgang Denk
2008-09-02 16:00 ` Guennadi Liakhovetski
1 sibling, 1 reply; 40+ messages in thread
From: Guennadi Liakhovetski @ 2008-08-31 20:37 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv
2008-08-31 20:37 ` Guennadi Liakhovetski
@ 2008-08-31 20:55 ` Wolfgang Denk
2008-09-01 9:08 ` Guennadi Liakhovetski
0 siblings, 1 reply; 40+ messages in thread
From: Wolfgang Denk @ 2008-08-31 20:55 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 6/6] Support environment in NAND
2008-08-31 18:57 ` [U-Boot] [PATCH 6/6] " Wolfgang Denk
@ 2008-08-31 21:53 ` Guennadi Liakhovetski
0 siblings, 0 replies; 40+ messages in thread
From: Guennadi Liakhovetski @ 2008-08-31 21:53 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv
2008-08-31 20:55 ` Wolfgang Denk
@ 2008-09-01 9:08 ` Guennadi Liakhovetski
2008-09-01 9:31 ` Guennadi Liakhovetski
2008-09-01 22:41 ` Wolfgang Denk
0 siblings, 2 replies; 40+ messages in thread
From: Guennadi Liakhovetski @ 2008-09-01 9:08 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv
2008-09-01 9:08 ` Guennadi Liakhovetski
@ 2008-09-01 9:31 ` Guennadi Liakhovetski
2008-09-01 22:42 ` Wolfgang Denk
2008-09-01 22:41 ` Wolfgang Denk
1 sibling, 1 reply; 40+ messages in thread
From: Guennadi Liakhovetski @ 2008-09-01 9:31 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv
2008-09-01 9:08 ` Guennadi Liakhovetski
2008-09-01 9:31 ` Guennadi Liakhovetski
@ 2008-09-01 22:41 ` Wolfgang Denk
2008-09-01 23:33 ` Guennadi Liakhovetski
1 sibling, 1 reply; 40+ messages in thread
From: Wolfgang Denk @ 2008-09-01 22:41 UTC (permalink / raw)
To: u-boot
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"
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv
2008-09-01 9:31 ` Guennadi Liakhovetski
@ 2008-09-01 22:42 ` Wolfgang Denk
0 siblings, 0 replies; 40+ messages in thread
From: Wolfgang Denk @ 2008-09-01 22:42 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv
2008-09-01 22:41 ` Wolfgang Denk
@ 2008-09-01 23:33 ` Guennadi Liakhovetski
2008-09-02 0:13 ` Wolfgang Denk
0 siblings, 1 reply; 40+ messages in thread
From: Guennadi Liakhovetski @ 2008-09-01 23:33 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv
2008-09-01 23:33 ` Guennadi Liakhovetski
@ 2008-09-02 0:13 ` Wolfgang Denk
0 siblings, 0 replies; 40+ messages in thread
From: Wolfgang Denk @ 2008-09-02 0:13 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv
2008-08-31 20:21 ` [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv Wolfgang Denk
2008-08-31 20:37 ` Guennadi Liakhovetski
@ 2008-09-02 16:00 ` Guennadi Liakhovetski
1 sibling, 0 replies; 40+ messages in thread
From: Guennadi Liakhovetski @ 2008-09-02 16:00 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2008-09-02 16:00 UTC | newest]
Thread overview: 40+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-08-27 15:52 [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv Guennadi Liakhovetski
2008-08-27 15:52 ` [U-Boot] [PATCH 1/6] Convert fw_env.c to use a single environment image union Guennadi Liakhovetski
2008-08-31 14:36 ` Wolfgang Denk
2008-08-31 15:57 ` Guennadi Liakhovetski
2008-08-31 18:57 ` Wolfgang Denk
2008-08-27 15:52 ` [U-Boot] [PATCH 2/6] Separate flash read and write operations Guennadi Liakhovetski
2008-08-31 14:58 ` Wolfgang Denk
2008-08-31 16:04 ` Guennadi Liakhovetski
2008-08-31 18:57 ` Wolfgang Denk
2008-08-31 19:45 ` Guennadi Liakhovetski
2008-08-31 19:56 ` Wolfgang Denk
2008-08-27 15:52 ` [U-Boot] [PATCH 3/6] "return" is not a function Guennadi Liakhovetski
2008-08-31 14:59 ` Wolfgang Denk
2008-08-31 16:10 ` Guennadi Liakhovetski
2008-08-31 18:57 ` Wolfgang Denk
2008-08-31 19:17 ` Guennadi Liakhovetski
2008-08-27 15:52 ` [U-Boot] [PATCH 4/6] Unify active vs. redundant environment variable naming Guennadi Liakhovetski
2008-08-31 15:04 ` Wolfgang Denk
2008-08-31 16:18 ` Guennadi Liakhovetski
2008-08-31 18:57 ` Wolfgang Denk
2008-08-31 19:27 ` Guennadi Liakhovetski
2008-08-31 19:44 ` Wolfgang Denk
2008-08-27 15:52 ` [U-Boot] [PATCH 5/6] Support environment anywhere within erase area Guennadi Liakhovetski
2008-08-31 18:57 ` Wolfgang Denk
2008-08-31 19:39 ` Guennadi Liakhovetski
2008-08-31 19:53 ` Wolfgang Denk
2008-08-27 15:52 ` [U-Boot] [PATCH 6/6] Support environment in NAND Guennadi Liakhovetski
2008-08-29 9:29 ` [U-Boot] [PATCH 6/6 v2] " Guennadi Liakhovetski
2008-08-31 18:57 ` [U-Boot] [PATCH 6/6] " Wolfgang Denk
2008-08-31 21:53 ` Guennadi Liakhovetski
2008-08-31 20:21 ` [U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv Wolfgang Denk
2008-08-31 20:37 ` Guennadi Liakhovetski
2008-08-31 20:55 ` Wolfgang Denk
2008-09-01 9:08 ` Guennadi Liakhovetski
2008-09-01 9:31 ` Guennadi Liakhovetski
2008-09-01 22:42 ` Wolfgang Denk
2008-09-01 22:41 ` Wolfgang Denk
2008-09-01 23:33 ` Guennadi Liakhovetski
2008-09-02 0:13 ` Wolfgang Denk
2008-09-02 16:00 ` Guennadi Liakhovetski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.