| Message ID | 0ea56396c07f0fcb92175e7303f4bbda23724a00.1306246651.git.bengardiner@nanometrics.ca |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Ben, > Add a flag to nand_read_skip_bad() such that if true, any trailing > pages in an eraseblock whose contents are entirely 0xff will be > dropped. > > The implementation is via a new drop_ffs() function which is > based on the function of the same name from the ubiformat > utility by Artem Bityutskiy. > > This is as-per the reccomendations of the UBI FAQ [1] > > Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca> > CC: Artem Bityutskiy <dedekind1@gmail.com> > CC: Detlev Zundel <dzu@denx.de> > > [1] http://www.linux-mtd.infradead.org/doc/ubi.html#L_flasher_algo Looks good - Acked-by: Detlev Zundel <dzu@denx.de> Cheers Detlev
On Tue, May 24, 2011 at 10:18:36AM -0400, Ben Gardiner wrote: > +#ifdef CONFIG_CMD_NAND_TRIMFFS > +static size_t drop_ffs(const nand_info_t *nand, const u_char *buf, > + const size_t *len) > +{ > + size_t i, l = *len; > + > + for (i = l - 1; i >= 0; i--) > + if (((const uint8_t *)buf)[i] != 0xFF) > + break; This cast looks unnecessary. > + /* The resulting length must be aligned to the minimum flash I/O size */ > + l = i + 1; > + l = (l + nand->writesize - 1) / nand->writesize; > + l *= nand->writesize; > + return l; We allow unaligned lengths (the rest of the page gets padded with 0xff, see nand_do_page_write-ops). The input length might be unaligned -- this adjustment could cause you to read beyond the end of the supplied buffer. > +} > +#endif > + > /** > * nand_write_skip_bad: > * > @@ -499,7 +520,7 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length, > return -EINVAL; > } > > - if (!need_skip) { > + if (!need_skip && !(flags & WITH_DROP_FFS)) { > rval = nand_write (nand, offset, length, buffer); > if (rval == 0) > return 0; Why not call drop_ffs before this point? > @@ -512,7 +533,7 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length, > > while (left_to_write > 0) { > size_t block_offset = offset & (nand->erasesize - 1); > - size_t write_size; > + size_t write_size, truncated_write_size; > > WATCHDOG_RESET (); > > @@ -558,7 +579,15 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length, > else > #endif > { > - rval = nand_write (nand, offset, &write_size, p_buffer); > + truncated_write_size = write_size; > +#ifdef CONFIG_CMD_NAND_TRIMFFS > + if (flags & WITH_DROP_FFS) > + truncated_write_size = drop_ffs(nand, p_buffer, > + &write_size); > +#endif What if both WITH_DROP_FFS and WITH_YAFFS_OOB are specified? -Scott
Hi Scott, Thanks for the review. On Mon, Jun 6, 2011 at 4:58 PM, Scott Wood <scottwood@freescale.com> wrote: > On Tue, May 24, 2011 at 10:18:36AM -0400, Ben Gardiner wrote: >> +#ifdef CONFIG_CMD_NAND_TRIMFFS >> +static size_t drop_ffs(const nand_info_t *nand, const u_char *buf, >> + const size_t *len) >> +{ >> + size_t i, l = *len; >> + >> + for (i = l - 1; i >= 0; i--) >> + if (((const uint8_t *)buf)[i] != 0xFF) >> + break; > > This cast looks unnecessary. You're absolutely right. It will be gone in v4. >> + /* The resulting length must be aligned to the minimum flash I/O size */ >> + l = i + 1; >> + l = (l + nand->writesize - 1) / nand->writesize; >> + l *= nand->writesize; >> + return l; > > We allow unaligned lengths (the rest of the page gets padded with 0xff, > see nand_do_page_write-ops). The input length might be unaligned -- > this adjustment could cause you to read beyond the end of the supplied > buffer. Right. Sorry I missed that. In v4 I will drop also any trailling 0xff which do not make-up a full page since they would be padded out to trailing 0xff. >> +} >> +#endif >> + >> /** >> * nand_write_skip_bad: >> * >> @@ -499,7 +520,7 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length, >> return -EINVAL; >> } >> >> - if (!need_skip) { >> + if (!need_skip && !(flags & WITH_DROP_FFS)) { >> rval = nand_write (nand, offset, length, buffer); >> if (rval == 0) >> return 0; > > Why not call drop_ffs before this point? To achieve the desired effect, drop_ffs must be called on each eraseblock sized chunk being written; so it seemed the simplest way was to force a block-by-block pass with the !WITH_DROP_FFS to enter while (left_to_write > 0) { I'll leave this as-is in v4. >> @@ -512,7 +533,7 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length, >> >> while (left_to_write > 0) { >> size_t block_offset = offset & (nand->erasesize - 1); >> - size_t write_size; >> + size_t write_size, truncated_write_size; >> >> WATCHDOG_RESET (); >> >> @@ -558,7 +579,15 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length, >> else >> #endif >> { >> - rval = nand_write (nand, offset, &write_size, p_buffer); >> + truncated_write_size = write_size; >> +#ifdef CONFIG_CMD_NAND_TRIMFFS >> + if (flags & WITH_DROP_FFS) >> + truncated_write_size = drop_ffs(nand, p_buffer, >> + &write_size); >> +#endif > > What if both WITH_DROP_FFS and WITH_YAFFS_OOB are specified? I didn't plan for that or intend for it to be supported. Previous to my introduction of WITH_DROP_FFS; using the YAFFS oob mode was mutually exclusive with the 'usual' way of writing. The introduction of WITH_DROP_FFs respects this precedent. If both flags were set 1) cmd_nand.c would need to be changed ( :) ) and 2) the WITH_YAFFS_OOB behaviour would override. In v4 I will add a -EINVAL if WITH_YAFFS_OOB flag is used with any other flag. Best Regards, Ben Gardiner --- Nanometrics Inc. http://www.nanometrics.ca
On Tue, 7 Jun 2011 09:09:07 -0400 Ben Gardiner <bengardiner@nanometrics.ca> wrote: > > Why not call drop_ffs before this point? > > To achieve the desired effect, drop_ffs must be called on each > eraseblock sized chunk being written; so it seemed the simplest way > was to force a block-by-block pass with the !WITH_DROP_FFS to enter Ah, I missed that it was within each erase block. > In v4 I will add a -EINVAL if WITH_YAFFS_OOB flag is used with any other flag. OK, I just didn't want it silently ignored, with no documentation of the limitation. -Scott
This series adds a nand write variant which implements the procedure reccomended in the UBI FAQ [1] of dropping trailing pages of eraseblocks containing entirely 0xff's. [1] http://www.linux-mtd.infradead.org/doc/ubi.html#L_flasher_algo Changes since v3: * rebased to nand-flash/next where patches 1/4 and 2/4 [v3] were already applied * added patch 1/3 [v4] to treat WITH_YAFFS_OOB as a mode * renumbered 3/4 and 4/4 [v3] to 1/3 and 3/3 [v4], respectively * remove uneccessary cast * wrapped README at 80 columns * prevent access past input buffer end Changes since v2: * dropped WITH_DEFAULTS in favour of '0' * moved copyright header to nand_util patch * added write.trimffs variant to README.nand Changes since v1: * renamed to 'trimffs' from 'ubi' * wrapped the new feature in #ifdefs * don't make it default for jffs -- patch dropped * attribution of the drop_ffs() function from mtd-utils to Artem Ben Gardiner (3): [v4] nand_util: treat WITH_YAFFS_OOB as a mode [v4] nand_util: drop trailing all-0xff pages if requested [v4] cmd_nand: add nand write.trimffs command common/cmd_nand.c | 16 +++++++++++++++ doc/README.nand | 10 +++++++++ drivers/mtd/nand/nand_util.c | 43 +++++++++++++++++++++++++++++++++++++++-- include/nand.h | 5 +++- 4 files changed, 70 insertions(+), 4 deletions(-)
On Tue, Jun 14, 2011 at 04:35:04PM -0400, Ben Gardiner wrote: > This series adds a nand write variant which implements the procedure > reccomended in the UBI FAQ [1] of dropping trailing pages of eraseblocks > containing entirely 0xff's. > > [1] http://www.linux-mtd.infradead.org/doc/ubi.html#L_flasher_algo > > Changes since v3: > * rebased to nand-flash/next where patches 1/4 and 2/4 [v3] were already > applied > * added patch 1/3 [v4] to treat WITH_YAFFS_OOB as a mode > * renumbered 3/4 and 4/4 [v3] to 1/3 and 3/3 [v4], respectively > * remove uneccessary cast > * wrapped README at 80 columns > * prevent access past input buffer end Applied to u-boot-nand-flash next -Scott
diff --git a/drivers/mtd/nand/nand_util.c b/drivers/mtd/nand/nand_util.c index 762ac53..82b41a0 100644 --- a/drivers/mtd/nand/nand_util.c +++ b/drivers/mtd/nand/nand_util.c @@ -11,6 +11,9 @@ * nandwrite.c by Steven J. Hill (sjhill@realitydiluted.com) * and Thomas Gleixner (tglx@linutronix.de) * + * Copyright (C) 2008 Nokia Corporation: drop_ffs() function by + * Artem Bityutskiy <dedekind1@gmail.com> from mtd-utils + * * See file CREDITS for list of people who contributed to this * project. * @@ -436,6 +439,24 @@ static int check_skip_len(nand_info_t *nand, loff_t offset, size_t length) return ret; } +#ifdef CONFIG_CMD_NAND_TRIMFFS +static size_t drop_ffs(const nand_info_t *nand, const u_char *buf, + const size_t *len) +{ + size_t i, l = *len; + + for (i = l - 1; i >= 0; i--) + if (((const uint8_t *)buf)[i] != 0xFF) + break; + + /* The resulting length must be aligned to the minimum flash I/O size */ + l = i + 1; + l = (l + nand->writesize - 1) / nand->writesize; + l *= nand->writesize; + return l; +} +#endif + /** * nand_write_skip_bad: * @@ -499,7 +520,7 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length, return -EINVAL; } - if (!need_skip) { + if (!need_skip && !(flags & WITH_DROP_FFS)) { rval = nand_write (nand, offset, length, buffer); if (rval == 0) return 0; @@ -512,7 +533,7 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length, while (left_to_write > 0) { size_t block_offset = offset & (nand->erasesize - 1); - size_t write_size; + size_t write_size, truncated_write_size; WATCHDOG_RESET (); @@ -558,7 +579,15 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length, else #endif { - rval = nand_write (nand, offset, &write_size, p_buffer); + truncated_write_size = write_size; +#ifdef CONFIG_CMD_NAND_TRIMFFS + if (flags & WITH_DROP_FFS) + truncated_write_size = drop_ffs(nand, p_buffer, + &write_size); +#endif + + rval = nand_write(nand, offset, &truncated_write_size, + p_buffer); offset += write_size; p_buffer += write_size; } diff --git a/include/nand.h b/include/nand.h index b0a31b8..5c8a189 100644 --- a/include/nand.h +++ b/include/nand.h @@ -116,6 +116,7 @@ int nand_read_skip_bad(nand_info_t *nand, loff_t offset, size_t *length, u_char *buffer); #define WITH_YAFFS_OOB (1 << 0) /* whether write with yaffs format */ +#define WITH_DROP_FFS (1 << 1) /* drop trailing all-0xff pages */ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length, u_char *buffer, int flags);
Add a flag to nand_read_skip_bad() such that if true, any trailing pages in an eraseblock whose contents are entirely 0xff will be dropped. The implementation is via a new drop_ffs() function which is based on the function of the same name from the ubiformat utility by Artem Bityutskiy. This is as-per the reccomendations of the UBI FAQ [1] Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca> CC: Artem Bityutskiy <dedekind1@gmail.com> CC: Detlev Zundel <dzu@denx.de> [1] http://www.linux-mtd.infradead.org/doc/ubi.html#L_flasher_algo --- This behaviour was found to fix both UBI and JFFS2 images written to cleanly erased NAND partitions on da850evm. Changes since v2: * moved the copyright header addition of nand_util to this patch from patch 'cmd_nand: add nand write.trimffs command' * Did not add Detlev's Acked-by because of movement of copyright header Changes since v1: * rebased to HEAD of git://git.denx.de/u-boot-nand-flash.git : ff7b4a0 ("env_nand: zero-initialize variable nand_erase_options") * wrap the new functionality in a CONFIG_CMD_NAND_TRIMFFS ifdef to reduce size impact of new feature --- drivers/mtd/nand/nand_util.c | 35 ++++++++++++++++++++++++++++++++--- include/nand.h | 1 + 2 files changed, 33 insertions(+), 3 deletions(-)