On Sun, 31 Aug 2008, Wolfgang Denk wrote:
> Dear Guennadi Liakhovetski,
>
> In message <[EMAIL PROTECTED]> 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: [EMAIL PROTECTED]
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
