Patchwork Faster flash_eraseall for NOR flash

login
register
mail settings
Submitter Øyvind Harboe
Date Dec. 14, 2010, 9:20 a.m.
Message ID <AANLkTi=DmECiCVyZ61Cc0=66VANyrQV9xwzYjDceA8=M@mail.gmail.com>
Download mbox | patch
Permalink /patch/75475/
State New
Headers show

Comments

Øyvind Harboe - Dec. 14, 2010, 9:20 a.m.
CFI erase performance can be 10x slower than write
performance on CFI flashes.

I've added an option to skip already erased CFI blocks. This
includes checking if the JFFS2 cleanmarker is written.


--
Øyvind Harboe

Can Zylin Consulting help on your project?

US toll free 1-866-980-3434 / International +47 51 63 25 00

http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
Mike Frysinger - Dec. 14, 2010, 2:52 p.m.
On Tue, Dec 14, 2010 at 04:20, Øyvind Harboe wrote:
> CFI erase performance can be 10x slower than write
> performance on CFI flashes.
>
> I've added an option to skip already erased CFI blocks. This
> includes checking if the JFFS2 cleanmarker is written.

i dont think -f/--fast is the right name.  how about something a bit
more explicit like --skip-erased ?

why cant this also be used for NAND ?  all the NAND devices i use have
0xff when erased.

other random problems:
 - the coding style of this patch is incorrect.  please review the
style found in the rest of the file.  such as brace placement and
spaces in function calls.
 - your patch is not against the master branch -- there is no
"flash_eraseall.c" file anymore.
 - once you update, your code will look vastly different as there are
helpers to do memory allocation/error output/etc...
 - better to use pread() rather than lseek()/read().
-mike
Øyvind Harboe - Dec. 14, 2010, 3:09 p.m.
Hi Mike,

thanks for the feedback!

>> I've added an option to skip already erased CFI blocks. This
>> includes checking if the JFFS2 cleanmarker is written.
>
> i dont think -f/--fast is the right name.  how about something a bit
> more explicit like --skip-erased ?

Sounds good to me.

> why cant this also be used for NAND ?  all the NAND devices i use have
> 0xff when erased.

I don't have any NAND to test with, nor am I terribly familiar with
NAND.

I think it would be good to limit this patch to NOR flash that I
can actually test and know something about. Also NAND flash
doesn't have the horrible erase performance issues, so it
might not be worth it from the point of view that it adds a new
code path.

Will it be acceptable to limit this patch to NOR flash and
pass the buck on NAND flash to someone
more familiar with NAND flash, who can test it and knows
whether or not it makes much sense?

> other random problems:
>  - the coding style of this patch is incorrect.  please review the
> style found in the rest of the file.  such as brace placement and
> spaces in function calls.

I did make an effort to follow the style, so any mismatch with
the existing style is just because I'm white-space blind. Much
like color blindness, it happens when you see enough different
formatting rules. The most annoying is of course code that
doesn't follow any rules, so I'll see about getting this fixed.

>  - your patch is not against the master branch -- there is no
> "flash_eraseall.c" file anymore.

k.

>  - once you update, your code will look vastly different as there are
> helpers to do memory allocation/error output/etc...

k.

>  - better to use pread() rather than lseek()/read().

Didn't know that one. Looks nice though!
Mike Frysinger - Dec. 14, 2010, 3:18 p.m.
On Tue, Dec 14, 2010 at 10:09, Øyvind Harboe wrote:
>> why cant this also be used for NAND ?  all the NAND devices i use have
>> 0xff when erased.
>
> I don't have any NAND to test with, nor am I terribly familiar with
> NAND.
>
> I think it would be good to limit this patch to NOR flash that I
> can actually test and know something about. Also NAND flash
> doesn't have the horrible erase performance issues, so it
> might not be worth it from the point of view that it adds a new
> code path.

i think it's fine to let the option operate on NAND devices.  if
someone has a problem with how it operates, then they can post a fix
for their use case.  i wouldnt worry about trying to come up with a
bullet proof solution at the first try.
-mike
Øyvind Harboe - Dec. 14, 2010, 3:26 p.m.
> i think it's fine to let the option operate on NAND devices.  if
> someone has a problem with how it operates, then they can post a fix
> for their use case.  i wouldnt worry about trying to come up with a
> bullet proof solution at the first try.

I'd rather pass the buck, because NAND is using a different code
path. Also this is an optimization that makes little difference on
NAND.
Mike Frysinger - Dec. 14, 2010, 3:33 p.m.
On Tue, Dec 14, 2010 at 10:26, Øyvind Harboe wrote:
>> i think it's fine to let the option operate on NAND devices.  if
>> someone has a problem with how it operates, then they can post a fix
>> for their use case.  i wouldnt worry about trying to come up with a
>> bullet proof solution at the first try.
>
> I'd rather pass the buck, because NAND is using a different code
> path. Also this is an optimization that makes little difference on
> NAND.

so pass the buck to me and say i told you to do it that way ;)

the only thing it changes in your original patch is to remove the type
checking ?
-mike
Øyvind Harboe - Dec. 14, 2010, 3:33 p.m.
> the only thing it changes in your original patch is to remove the type
> checking ?

No. There is a different code path for writing the cleanmarkers.
Mike Frysinger - Dec. 14, 2010, 3:38 p.m.
On Tue, Dec 14, 2010 at 10:33, Øyvind Harboe wrote:
>> the only thing it changes in your original patch is to remove the type
>> checking ?
>
> No. There is a different code path for writing the cleanmarkers.

OK, i missed that.  just change the code to issue a warning when
someone tries to use this option on a NAND device.  something like
"--skip-erased: sorry, no support (yet) for NAND devices".

in looking at the original patch, i noticed something else ... dont
mix variable decls and code.  i dont think mtd-utils has moved on to
newer C standards/styles yet and support that.  for example, the
"ssize_t i" decl after a bunch of if statements.
-mike
Øyvind Harboe - Dec. 14, 2010, 3:46 p.m.
On Tue, Dec 14, 2010 at 4:38 PM, Mike Frysinger <vapier.adi@gmail.com> wrote:
> On Tue, Dec 14, 2010 at 10:33, Øyvind Harboe wrote:
>>> the only thing it changes in your original patch is to remove the type
>>> checking ?
>>
>> No. There is a different code path for writing the cleanmarkers.
>
> OK, i missed that.  just change the code to issue a warning when
> someone tries to use this option on a NAND device.  something like
> "--skip-erased: sorry, no support (yet) for NAND devices".
>
> in looking at the original patch, i noticed something else ... dont
> mix variable decls and code.  i dont think mtd-utils has moved on to
> newer C standards/styles yet and support that.  for example, the
> "ssize_t i" decl after a bunch of if statements.

I'll see about getting this done. Don't know when though, if
someone beats me to the punch here, no loss :-)
Mike Frysinger - Dec. 14, 2010, 3:47 p.m.
On Tue, Dec 14, 2010 at 10:46, Øyvind Harboe wrote:
> I'll see about getting this done. Don't know when though, if
> someone beats me to the punch here, no loss :-)

np; take your time.  thanks for the improvements!
-mike
Øyvind Harboe - Dec. 29, 2010, 8:25 a.m.
I don't think this feature is going to work very well as-is.

To make this effective in real life, I think I'll have to add in an
option to read in a binary file, which is written to flash after
erase.

Only those erase blocks which changed should be
erased + written.

The cleanmarkers would be written for erase blocks after
the end of the binary file.

This fixes two things: even faster for blocks that don't change,
if the cleanmarker is not supposed to be there for certain
blocks, then it will currently fail.

Patch

From 066f501b253c84550e6959be527aa0aa75fff631 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=98yvind=20Harboe?= <oyvind.harboe@zylin.com>
Date: Fri, 10 Dec 2010 10:05:43 +0100
Subject: [PATCH] mtd-utils: add fast NOR flash erase
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

do not erase sector (which can be 10x slower than write's on
CFI's) if it is already erased. This includes a check for a
cleanmarker.

Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
---
 .../flash_eraseall.c                               |   60 +++++++++++++++++++-
 1 files changed, 59 insertions(+), 1 deletions(-)

diff --git a/user/mtd-utils/606f38a2221648ca5c5fa292c9f71d2ddd59fa66/flash_eraseall.c b/user/mtd-utils/606f38a2221648ca5c5fa292c9f71d2ddd59fa66/flash_eraseall.c
index a22fc49..f6da1ff 100644
--- a/user/mtd-utils/606f38a2221648ca5c5fa292c9f71d2ddd59fa66/flash_eraseall.c
+++ b/user/mtd-utils/606f38a2221648ca5c5fa292c9f71d2ddd59fa66/flash_eraseall.c
@@ -47,6 +47,7 @@  static const char *exe_name;
 static const char *mtd_device;
 static int quiet;		/* true -- don't output progress */
 static int jffs2;		// format for jffs2 usage
+static int fast;		// Fast - check if the block is erased before erasing
 
 static void process_options (int argc, char *argv[]);
 void show_progress (mtd_info_t *meminfo, erase_info_t *erase);
@@ -146,9 +147,61 @@  int main (int argc, char *argv[])
 			}
 		}
 
+		/* Can we skip this one if it is already erased?
+		 * Perhaps NAND performance could be improved by adding support for
+		 * those cleanmarkers as well?
+		 *
+		 * Currently this only works with JFFS2 NOR clean markers
+		 */
+		int skip = 0;
+		if (fast && !isNAND) {
+			if (lseek (fd, erase.start, SEEK_SET) < 0) {
+				fprintf(stderr, "\n%s: %s: MTD lseek failure: %s\n", exe_name, mtd_device, strerror(errno));
+				return 1;
+			}
+			uint8_t *tmp = malloc(meminfo.erasesize);
+			if (tmp == NULL)
+			{
+				fprintf(stderr, "Out of memory\n");
+				return 1;
+			}
+			ssize_t actual = read(fd, tmp, meminfo.erasesize);
+			if (actual != meminfo.erasesize) {
+				fprintf(stderr, "\n%s: %s: MTD read failure: %s\n", exe_name, mtd_device, strerror(errno));
+				return 1;
+			}
+			ssize_t i = 0;
+			int ok = 1;
+
+			if (jffs2) {
+				ok = memcmp(tmp, &cleanmarker, sizeof (cleanmarker)) == 0;
+				i = sizeof (cleanmarker);
+			}
+
+			if (ok)
+			{
+				for (; i < meminfo.erasesize; i++)
+				{
+					if (tmp[i] != 0xff)
+						break;
+				}
+				if (i == meminfo.erasesize)
+				{
+					/* Yup! we can skip! Here we could improve things by
+					 * adding support for NAND cleanmarkers
+					 */
+					skip = 1;
+				}
+			}
+			free(tmp);
+		}
+
 		if (!quiet)
 			show_progress(&meminfo, &erase);
 
+		if (skip)
+			continue;
+
 		if (ioctl(fd, MEMERASE, &erase) != 0) {
 			fprintf(stderr, "\n%s: %s: MTD Erase failure: %s\n", exe_name, mtd_device, strerror(errno));
 			continue;
@@ -198,11 +251,12 @@  void process_options (int argc, char *argv[])
 
 	for (;;) {
 		int option_index = 0;
-		static const char *short_options = "jq";
+		static const char *short_options = "jqf";
 		static const struct option long_options[] = {
 			{"help", no_argument, 0, 0},
 			{"version", no_argument, 0, 0},
 			{"jffs2", no_argument, 0, 'j'},
+			{"fast", no_argument, 0, 'f'},
 			{"quiet", no_argument, 0, 'q'},
 			{"silent", no_argument, 0, 'q'},
 
@@ -232,6 +286,9 @@  void process_options (int argc, char *argv[])
 			case 'j':
 				jffs2 = 1;
 				break;
+			case 'f':
+				fast = 1;
+				break;
 			case '?':
 				error = 1;
 				break;
@@ -265,6 +322,7 @@  void display_help (void)
 			"\n"
 			"  -j, --jffs2    format the device for jffs2\n"
 			"  -q, --quiet    don't display progress messages\n"
+			"  -f, --fast     do not re-erase sectors that are erased\n"
 			"      --silent   same as --quiet\n"
 			"      --help     display this help and exit\n"
 			"      --version  output version information and exit\n",
-- 
1.7.0.4