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

[Guennadi Liakhovetski]

On Sun, 31 Aug 2008, Wolfgang Denk wrote:

> Dear Guennadi Liakhovetski,
> 
> In message <Pine.LNX.4.64.0808271748520.6718 at 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