Patchwork [U-Boot,4/4,v3] cmd_nand: add nand write.trimffs command

login
register
mail settings
Submitter Ben Gardiner
Date May 24, 2011, 2:18 p.m.
Message ID <f9a94a1f4b1aadc2a94821cf4148d6bf4273758f.1306246651.git.bengardiner@nanometrics.ca>
Download mbox | patch
Permalink /patch/97164/
State Changes Requested
Headers show

Comments

Ben Gardiner - May 24, 2011, 2:18 p.m.
Add another nand write. variant, trimffs. This command will request of
nand_write_skip_bad() that all trailing all-0xff pages will be
dropped from eraseblocks when they are written to flash as-per the
reccommended behaviour of the UBI FAQ [1].

The function that implements this timming is the drop_ffs() function
by Artem Bityutskiy, ported from the mtd-utils tree.

[1] http://www.linux-mtd.infradead.org/doc/ubi.html#L_flasher_algo

Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
CC: Artem Bityutskiy <dedekind1@gmail.com>
CC: Detlev Zundel <dzu@denx.de>

---

Changes since v2:
 * added nand write.trimffs to the README.nand file
 * moved the nand_util copyright header addition to patch 'nand_util:
   drop trailing all-0xff pages if requested'
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")
 * renamed the command variant to '.trimffs' from '.ubi' (Detlev Zundel)
 * added attribution to mtd-utils and Artem Bityutskiy in both the source
   comments and commit message
 * wrapped the new command in a new ifdef, CONFIG_CMD_NAND_TRIMFFS, to
   reduce the size impact of this new feature
---
 common/cmd_nand.c |   16 ++++++++++++++++
 doc/README.nand   |   10 ++++++++++
 2 files changed, 26 insertions(+), 0 deletions(-)
Detlev Zundel - May 27, 2011, 9:41 a.m.
Hi Ben,

> Add another nand write. variant, trimffs. This command will request of
> nand_write_skip_bad() that all trailing all-0xff pages will be
> dropped from eraseblocks when they are written to flash as-per the
> reccommended behaviour of the UBI FAQ [1].
>
> The function that implements this timming is the drop_ffs() function
> by Artem Bityutskiy, ported from the mtd-utils tree.
>
> [1] http://www.linux-mtd.infradead.org/doc/ubi.html#L_flasher_algo
>
> Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
> CC: Artem Bityutskiy <dedekind1@gmail.com>
> CC: Detlev Zundel <dzu@denx.de>

Thanks - looks good now.

Acked-by: Detlev Zundel <dzu@denx.de>

Cheers
  Detlev
Scott Wood - June 6, 2011, 9 p.m.
On Tue, May 24, 2011 at 10:18:37AM -0400, Ben Gardiner wrote:
> diff --git a/common/cmd_nand.c b/common/cmd_nand.c
> index e7db4c9..786f179 100644
> --- a/common/cmd_nand.c
> +++ b/common/cmd_nand.c
> @@ -575,6 +575,16 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
>  			else
>  				ret = nand_write_skip_bad(nand, off, &rwsize,
>  							  (u_char *)addr, 0);
> +#ifdef CONFIG_CMD_NAND_TRIMFFS
> +		} else if (!strcmp(s, ".trimffs")) {
> +			if (read) {
> +				printf("Unknown nand command suffix '%s'\n", s);
> +				return 1;
> +			}
> +			ret = nand_write_skip_bad(nand, off, &rwsize,
> +						(u_char *)addr,
> +						WITH_DROP_FFS);
> +#endif
>  #ifdef CONFIG_CMD_NAND_YAFFS
>  		} else if (!strcmp(s, ".yaffs")) {

Should these really be modes rather than flags?

> +   nand write.trimffs addr ofs|partition size
> +      Enabled by the CONFIG_CMD_NAND_TRIMFFS macro. This command will write to
> +      the NAND flash in a manner identical to the 'nand write' command described
> +      above -- with the additional check that all pages at the end of
> +      eraseblocks which contain only 0xff data will not be written to the NAND
> +      flash. This behaviour is required when flashing UBI images containing
> +       UBIFS volumes as per the UBI FAQ[1].

Please wrap at 80 columns.

-Scott
Ben Gardiner - June 7, 2011, 1:09 p.m.
Hi Scott,

Thanks for the reviews.

On Mon, Jun 6, 2011 at 5:00 PM, Scott Wood <scottwood@freescale.com> wrote:
> On Tue, May 24, 2011 at 10:18:37AM -0400, Ben Gardiner wrote:
>> diff --git a/common/cmd_nand.c b/common/cmd_nand.c
>> index e7db4c9..786f179 100644
>> --- a/common/cmd_nand.c
>> +++ b/common/cmd_nand.c
>> @@ -575,6 +575,16 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
>>                       else
>>                               ret = nand_write_skip_bad(nand, off, &rwsize,
>>                                                         (u_char *)addr, 0);
>> +#ifdef CONFIG_CMD_NAND_TRIMFFS
>> +             } else if (!strcmp(s, ".trimffs")) {
>> +                     if (read) {
>> +                             printf("Unknown nand command suffix '%s'\n", s);
>> +                             return 1;
>> +                     }
>> +                     ret = nand_write_skip_bad(nand, off, &rwsize,
>> +                                             (u_char *)addr,
>> +                                             WITH_DROP_FFS);
>> +#endif
>>  #ifdef CONFIG_CMD_NAND_YAFFS
>>               } else if (!strcmp(s, ".yaffs")) {
>
> Should these really be modes rather than flags?

For the two currently possible values of 'int flags' -- yes. But
that's because the WITH_YAFFS_OOB causes a specific exuction path
which is mutually exclusive to the usual path; whereas the
WITH_DROP_FFS option does an operation in addition to the usual
execution. So the latter is (to me at least) a flag whereas the former
is a mode.

I see you have already applied the patches which convert to a flag --
so I will leave as is in v4. I will, as noted in the previous email --
add a return -EINVAL if WITH_YAFFS_OOB is used with any other flags.

>> +   nand write.trimffs addr ofs|partition size
>> +      Enabled by the CONFIG_CMD_NAND_TRIMFFS macro. This command will write to
>> +      the NAND flash in a manner identical to the 'nand write' command described
>> +      above -- with the additional check that all pages at the end of
>> +      eraseblocks which contain only 0xff data will not be written to the NAND
>> +      flash. This behaviour is required when flashing UBI images containing
>> +       UBIFS volumes as per the UBI FAQ[1].
>
> Please wrap at 80 columns.

Ok. Will do in v4.

checkpatch.pl did not complain about "     the NAND flash in a manner
identical to the 'nand write' command described" -- which is 81
characters including the \n but it did not complain about a line over
85 characters in that README either so I guess READMEs aren't enforced
by that script.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

Patch

diff --git a/common/cmd_nand.c b/common/cmd_nand.c
index e7db4c9..786f179 100644
--- a/common/cmd_nand.c
+++ b/common/cmd_nand.c
@@ -575,6 +575,16 @@  int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 			else
 				ret = nand_write_skip_bad(nand, off, &rwsize,
 							  (u_char *)addr, 0);
+#ifdef CONFIG_CMD_NAND_TRIMFFS
+		} else if (!strcmp(s, ".trimffs")) {
+			if (read) {
+				printf("Unknown nand command suffix '%s'\n", s);
+				return 1;
+			}
+			ret = nand_write_skip_bad(nand, off, &rwsize,
+						(u_char *)addr,
+						WITH_DROP_FFS);
+#endif
 #ifdef CONFIG_CMD_NAND_YAFFS
 		} else if (!strcmp(s, ".yaffs")) {
 			if (read) {
@@ -689,6 +699,12 @@  U_BOOT_CMD(
 	"nand write - addr off|partition size\n"
 	"    read/write 'size' bytes starting at offset 'off'\n"
 	"    to/from memory address 'addr', skipping bad blocks.\n"
+#ifdef CONFIG_CMD_NAND_TRIMFFS
+	"nand write.trimffs - addr off|partition size\n"
+	"    write 'size' bytes starting at offset 'off' from memory address\n"
+	"    'addr', skipping bad blocks and dropping any pages at the end\n"
+	"    of eraseblocks that contain only 0xFF\n"
+#endif
 #ifdef CONFIG_CMD_NAND_YAFFS
 	"nand write.yaffs - addr off|partition size\n"
 	"    write 'size' bytes starting at offset 'off' with yaffs format\n"
diff --git a/doc/README.nand b/doc/README.nand
index 8eedb6c..ca62f00 100644
--- a/doc/README.nand
+++ b/doc/README.nand
@@ -78,6 +78,16 @@  Commands:
       should work well, but loading an image copied from another flash is
       going to be trouble if there are any bad blocks.
 
+   nand write.trimffs addr ofs|partition size
+      Enabled by the CONFIG_CMD_NAND_TRIMFFS macro. This command will write to
+      the NAND flash in a manner identical to the 'nand write' command described
+      above -- with the additional check that all pages at the end of
+      eraseblocks which contain only 0xff data will not be written to the NAND
+      flash. This behaviour is required when flashing UBI images containing
+       UBIFS volumes as per the UBI FAQ[1].
+
+      [1] http://www.linux-mtd.infradead.org/doc/ubi.html#L_flasher_algo
+
    nand write.oob addr ofs|partition size
       Write `size' bytes from `addr' to the out-of-band data area
       corresponding to `ofs' in NAND flash. This is limited to the 16 bytes