[U-Boot] [PATCH] fw_env: add NAND support
Wolfgang Denk
wd at denx.de
Tue Sep 2 22:32:22 CEST 2008
Dear Guennadi Liakhovetski,
In message <Pine.LNX.4.64.0809021840240.9447 at axis700.grange> you wrote:
> Add support for environment in NAND with automatic NOR / NAND recognition,
> including unaligned environment, bad-block skipping, redundant environment
> copy.
>
> Signed-off-by: Guennadi Liakhovetski <lg at denx.de>
> ---
>
> This is now a single patch. No union any more, instead a struct with
> pointers into a flat image buffer is used. Still, MTD_VERSION=old doesn't
> work, it looks it is also broken in the current version.
I wonder why you didn't even try to fix it.
See [PATCH] fw_env.c: fix build problems with MTD_VERSION=old
Please rebase your patch and resubmit.
> 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.
I think "...on NOR it is not used" would be clearer ?
> +static int flash_read_buf (int dev, int fd, void *buf, size_t count,
> + off_t offset)
> +{
> + 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;
> + /* end of the last block we may use */
> + off_t top_of_range;
> + /* offset inside the current block to the start of the data */
> + off_t block_seek;
> + /* running start of the current block - MEMGETBADBLOCK needs 64 bits */
> + loff_t blockstart;
> + int rc;
That's difficult to read. Please reformat (comments on smae line with
code, if necessary continued on next line).
> + /*
> + * Start of the first block to be read, relies on the fact, that
> + * erase sector size is always a power of 2
> + */
> + blockstart = offset & ~(DEVESIZE (dev) - 1);
Insert empty line here.
> + /* Offset inside a block */
> + block_seek = offset - blockstart;
...
> + /* Limit to one block for the first read */
Why?
> + if (readlen > blocklen - block_seek)
> + readlen = blocklen - block_seek;
> + } else {
> + blocklen = 0;
> + top_of_range = offset + count;
> + }
> +
> + /* This only runs once on NOR flash */
> + while (processed < count) {
> + rc = flash_bad_block (dev, fd, &mtdinfo, &blockstart);
> + if (rc < 0) {
> + return -1;
> + } else if (blockstart + block_seek + readlen > top_of_range) {
> + /* End of range is reached */
> fprintf (stderr,
> + "Too few good blocks within range\n");
> + return -1;
> + } else if (rc) {
> + blockstart += blocklen;
> + continue;
> }
Please rewrite as:
if (rc < 0) /* block test failed */
return -1;
if (blockstart + block_seek + readlen > top_of_range) {
/* End of range is reached */
fprintf (stderr,
"Too few good blocks within range\n");
return -1;
}
if (rc) { /* block is bad */
blockstart += blocklen;
continue;
}
I can much easier follow the flow when written that way.
> +
> + /*
> + * 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);
Hm... there was a "continue" for the badblock case just above, so do
we really try the _next_ block here?
> +static int __flash_write_buf (int dev, int fd, void *buf, size_t count,
> + off_t offset, struct mtd_info_user *mtdinfo)
> +{
> + char *data = NULL;
> + erase_info_t erase;
> + /* length of NAND block / NOR erase sector */
> + size_t blocklen;
> + /* whole area that can be erased - may include bad blocks */
> + size_t erase_len;
> + /* erase / write length - one block on NAND, whole area on NOR */
> + 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;
> + /* 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;
Not readable. Please reformat.
> + /*
> + * Data size we actually have to write: from the start of the block
> + * to the start of the data, then count bytes ob data, and to the
-------------------------------------------------------^ f
> + * end of the block
> + */
Hm... sounds as if this was always exactly one full block, then?
> + write_total = (block_seek + count + blocklen - 1) & ~(blocklen - 1);
> +
> + /*
> + * 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 (write_total > count) {
> + data = malloc (erase_len);
My understanding is, that erase_len can be > block size. Is this
correct?
I don't see where the actual size of the environment data is taken
into considration?
> + } else {
> + /* Offset is block-aligned by construction here */
What does "block-aligned by construction" mean?
> + if (mtdinfo->type == MTD_NANDFLASH)
Add ----------------------------------------> {
> + /*
> + * NAND: calculate which blocks we are writing. We have
> + * to write one block at a time to skip bad blocks.
> + */
> + erasesize = blocklen;
> + else
} else {
> + erasesize = erase_len;
}
> + erase.length = erasesize;
> +
> + /* This only runs once on NOR flash */
How comes that?
> + while (processed < write_total) {
> + rc = flash_bad_block (dev, fd, mtdinfo, &blockstart);
> + if (rc < 0) {
> + return rc;
> + } else if (blockstart + erasesize > top_of_range) {
> + fprintf (stderr, "End of range reached, aborting\n");
> + return -1;
> + } else if (rc) {
> + blockstart += blocklen;
> + continue;
> }
Please rewrite - see above.
> + processed += blocklen;
> + block_seek = 0;
> + blockstart += blocklen;
processed += blocklen;
blockstart += blocklen;
block_seek = 0;
> + printf ("Writing new environment at 0x%lx\n", DEVOFFSET (dev_target));
If you really want to keep this printf(), thenplease also print the
device name (i. e try to stick to the format of the output of the old
version and at least provide the same information).
You may also consider the comments posted before, that such informa-
tion should be printed only when the verbose option (argument -v) was
selected.
> 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;
> }
Please factor out the open()/close() as discussed several times
before.
> + if (rc < 0)
> + return rc;
> +
> + if (HaveRedundEnv) {
> + off_t offset = DEVOFFSET (dev_current) +
> + offsetof(struct env_image_redundant, flags);
> + printf ("Setting obsolete flag for environment at 0x%lx\n",
> + DEVOFFSET (dev_current));
Please make this a debug print only.
In general, please check printed output.
> + flash_flag_obsolete(dev_current, fd_current, offset);
> + }
> +
> + if (close (fd_target)) {
> fprintf (stderr,
> + "I/O error on %s: %s\n",
> + DEVNAME (dev_target), strerror (errno));
> + return -1;
> }
Please factor out the open()/close() as discussed several times
before.
> +static int flash_read (void)
> +{
> + int fd, rc;
> +
> + if ((fd = open (DEVNAME (dev_current), O_RDONLY)) < 0) {
> + fprintf (stderr,
> + "Can't open %s: %s\n",
> + DEVNAME (dev_current), strerror (errno));
> + return -1;
> + }
...
> + if (close (fd)) {
> + fprintf (stderr,
> + "I/O error on %s: %s\n",
> + DEVNAME (dev_current), strerror (errno));
> + return -1;
> + }
Please factor out the open()/close() as discussed several times
before.
> + return rc < 0 ? rc : 0;
return (rc < 0) ? rc : 0;
> @@ -596,107 +844,120 @@ static char *envmatch (char * s1, char * s2)
> static int env_init (void)
> {
> int crc1, crc1_ok;
> + char flag1, *addr1;
>
> int crc2, crc2_ok;
> + char flag2, *addr2;
I think you should number these 0 an 1, respectively here.
Because then you can read this code much better:
> if (crc1_ok && !crc2_ok) {
> + dev_current = 0;
> } else if (!crc1_ok && crc2_ok) {
> + dev_current = 1;
would become:
if (crc0_ok && !crc1_ok) {
dev_current = 0;
} else if (!crc0_ok && crc1_ok) {
dev_current = 1;
If "0" is OK, then use "0"; if "1" is ok then use "1".
Your version: if "1" is OK then use "0", if "2" is OK then use "1"
is more difficult to read.
[Or stick with the old identifiers, i. e. use 1 and 2 consistently,
but don't mix 0/1 here and 1/2 there.]
...
> + /* prepare for device 2 */
> + dev_current = 0;
> + /* From here: both CRCs correct */
The comment doesn't fit (indentation is wrong), and it's obvious anyway - omit it.
> + /* From here: invalid flag configuration */
Ditto.
> + /*
> + * If we are reading, we don't need the flag and the crc any
> + * more, if we are writing, we will set them before writing out
> + */
Note that this is a serious impairment compared to the old version.
The whole logic of writing the envrionment (and especially so when
redundancy is used) is based on separate logic steps that are per-
formed strictly sequentially, and only when the previous one was
successfully completed. This essential to maintain consistent data.
For the redundant case that means:
1) write the environment data to flash.
*After* this has completed, make the data valid by
2) writing the CRC checksum.
*After* this has completed, make this copy of the environment valid by
3) writing the valid flag to this copy.
*After* this has completed, make the other copy of the environment obsolete by
4) writing the obsolete flag to the other copy.
Your new implementation breaks this by writing out the whole buffer
at once. This is not a good ideas, because you may end up ith
situations that were impossible before, like having a valid flag byte
in flash even though the environment data were not completely
written.
I am aware that you have little options with NAND flash, but for NOR
this is IMHO a serious change for the worse.
Can we please avoid this, and re-establish the old mode of operation,
which was designed for reliability?
> + if ((fp = fopen (fname, "r")) == NULL)
> + return -1;
>
> + while (i < 2 && fgets (dump, sizeof (dump), fp)) {
> /* Skip incomplete conversions and comment strings */
> + if (dump[0] == '#')
> continue;
I think you must initialize ENVSECTORS(i) here...
> + 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;
Because if rc < 4, you already continued and left ENVSECTORS uninitialized.
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 became apparent that one reason why the Ice Giants were known as
the Ice Giants was because they were, well, giants. The other was
that they were made of ice. -Terry Pratchett, _Sourcery_
More information about the U-Boot
mailing list