| Message ID | AANLkTi=DmECiCVyZ61Cc0=66VANyrQV9xwzYjDceA8=M@mail.gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
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
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!
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
> 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.
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
> 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.
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
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 :-)
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
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.
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