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

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