From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Roese Date: Sat, 7 Oct 2006 12:35:18 +0200 Subject: [U-Boot-Users] [PATCH] New NAND subsystem: mtd like commands support jffs2 and bad blocks In-Reply-To: <5c01f02f0610060809i163b13ex878e5aeadec738f9@mail.gmail.com> References: <5c01f02f0610060802o429760efib1ccc8c936f4509a@mail.gmail.com> <5c01f02f0610060809i163b13ex878e5aeadec738f9@mail.gmail.com> Message-ID: <200610071235.18760.sr@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Guido, On Friday 06 October 2006 17:09, Guido Classen wrote: > the attached patch implements several improvements to the new NAND > subsystem: Looks promising. I'm not completely happy with the implementation though: > diff -Nrub --exclude='*~' --exclude=.depend > u-boot-weiss-org/common/cmd_nand.c u-boot-weiss/common/cmd_nand.c --- > u-boot-weiss-org/common/cmd_nand.c 2006-09-22 11:50:07.000000000 +0200 +++ > u-boot-weiss/common/cmd_nand.c 2006-10-06 16:35:57.000000000 +0200 @@ > -178,7 +178,10 @@ > > if (strcmp(cmd, "bad") != 0 && strcmp(cmd, "erase") != 0 && > strncmp(cmd, "dump", 4) != 0 && > - strncmp(cmd, "read", 4) != 0 && strncmp(cmd, "write", 5) != 0) > + strncmp(cmd, "read", 4) != 0 && strncmp(cmd, "write", 5) != 0 && > + strcmp(cmd, "scrub") != 0 && strcmp(cmd, "markbad") != 0 && > + strcmp(cmd, "biterr") != 0 && > + strcmp(cmd, "lock") != 0 && strcmp(cmd, "unlock") != 0 ) > goto usage; > > /* the following commands operate on the current device */ > @@ -197,14 +200,69 @@ > return 0; > } > > - if (strcmp(cmd, "erase") == 0) { > - arg_off_size(argc - 2, argv + 2, &off, &size, nand->size); > + if (strcmp(cmd, "erase") == 0 > + || strcmp(cmd, "scrub") == 0) { > + nand_erase_options_t opts; > + int clean = argc >= 3 && !strcmp("clean", argv[2]); > + int rest_argc = argc - 2; > + char **rest_argv = argv + 2; > + int scrub = !strcmp(cmd, "scrub"); > + > + if (clean) { > + rest_argc--; > + rest_argv++; > + } > + > + if (rest_argc == 0) { > + > + printf("\nNAND %s: device %d whole chip\n", > + cmd, > + nand_curr_device); > + > + off = size = 0; > + } else { > + arg_off_size(rest_argc, rest_argv, &off, &size, > + nand->size); > + > + Don't add two empty lines. > if (off == 0 && size == 0) Indentation is incorrect here. > return 1; > > - printf("\nNAND erase: device %d offset 0x%x, size 0x%x ", > + printf("\nNAND %s: device %d offset 0x%x, " > + "size 0x%x\n", > + cmd, > nand_curr_device, off, size); > - ret = nand_erase(nand, off, size); > + } > + > + > + memset(&opts, 0, sizeof(opts)); > + opts.offset = off; > + opts.length = size; > + opts.jffs2 = clean; > + > + if (scrub) { > + > + printf("Warning: " > + "scrub option will erase all factory set " > + "bad blocks!\n" > + " " > + "There is now reliable way to bring them back.\n" > + " " > + "Use this command only for testing purposes " > + "if you\n" > + " " > + "are shure what you are doing!\n" > + "\nRealy scrub this NAND flash? \n" > + ); > + > + if (getc() == 'y' && getc() == '\r') { > + opts.scrub = 1; > + } else { > + printf ("scrub aborted\n"); > + return -1; > + } > + } > + ret = nand_erase_opts(nand, &opts); > printf("%s\n", ret ? "ERROR" : "OK"); > > return ret == 0 ? 0 : 1; > @@ -249,6 +307,35 @@ > printf("\nNAND %s: device %d offset %u, size %u ... ", > i ? "read" : "write", nand_curr_device, off, size); > > + Again, please not 2 empty lines. > + s = strchr(cmd, '.'); > + if (s != NULL > + && (!strcmp(s, ".jffs2") || !strcmp(s, ".e") > + || !strcmp(s, ".i"))) { > + if (i) { > + /* read */ > + nand_read_options_t opts; > + memset(&opts, 0, sizeof(opts)); > + opts.buffer = (u_char*) addr; > + opts.length = size; > + opts.offset = off; > + ret = nand_read_opts(nand, &opts); > + } else { > + /* write */ > + nand_write_options_t opts; > + memset(&opts, 0, sizeof(opts)); > + opts.buffer = (u_char*) addr; > + opts.length = size; > + opts.offset = off; > + /* opts.forcejffs2 = 1; */ > + opts.pad = 1; > + opts.blockalign = 1; > + ret = nand_write_opts(nand, &opts); > + } > + printf("%s\n", ret ? "ERROR" : "OK"); > + return ret == 0 ? 0 : 1; > + } > + > if (i) > ret = nand_read(nand, off, &size, (u_char *)addr); > else > @@ -259,13 +346,112 @@ > > return ret == 0 ? 0 : 1; > } > + > + /* 2006-09-28 gc: implement missing commands */ > + if (strcmp(cmd, "markbad") == 0) { > + /* todo */ > + addr = (ulong)simple_strtoul(argv[2], NULL, 16); > + > + int ret = nand->block_markbad(nand, addr); > + if (ret == 0) { > + printf("block 0x%08lx successfully marked as bad\n", > + (ulong) addr); > + return 0; > + } else { > + printf("block 0x%08lx NOT marked as bad! ERROR %d\n", > + (ulong) addr, ret); > + } > + return 1; > + } > + if (strcmp(cmd, "biterr") == 0) { > + /* todo */ > + return 1; > + } > + > + if (strcmp(cmd, "lock") == 0) { > + int tight = 0; > + int status = 0; > + if (argc == 3) { > + if (!strcmp("tight", argv[2])) > + tight = 1; > + if (!strcmp("status", argv[2])) > + status = 1; > + } > + > + if (status) { > + ulong block_start = 0; > + ulong off; > + int last_status; > + > + struct nand_chip *nand_chip = nand->priv; > + /* Check the WP bit */ > + nand_chip->cmdfunc (nand, NAND_CMD_STATUS, -1, -1); > + printf("device is %swrite protected\n", > + (nand_chip->read_byte(nand) & 0x80 ? > + "NOT " : "" ) ); > + > + for (off = 0; off < nand->size; off += nand->oobblock) { > + int s = nand_get_lock_status( nand, off); > + > + /* print message only if status has changed > + * or at end of chip > + */ > + if (off == nand->size - nand->oobblock > + || (s != last_status && off != 0)) { > + > + printf("%08x - %08x: %8d pages %s%s%s\n", > + block_start, > + off-1, > + (off-block_start)/nand->oobblock, > + ((last_status & NAND_LOCK_STATUS_TIGHT) ? "TIGHT " : ""), > + ((last_status & NAND_LOCK_STATUS_LOCK) ? "LOCK " : ""), > + ((last_status & NAND_LOCK_STATUS_UNLOCK) ? "UNLOCK " : "") > + ); > + } > + > + last_status = s; > + } > + > + Two empty lines. > + } else { > + > + if (!nand_lock( nand, tight) ) > + { Opening brace in "if-statement" line please. > + printf ("NAND flash successfully locked\n"); > + } else { > + printf ("Error locking NAND flash. \n"); > + return 1; > + } > + } > + return 0; > + } > + if (strcmp(cmd, "unlock") == 0) { > + > + if (argc == 2) { > + off = 0; > + size = nand->size; > + } else { > + arg_off_size(argc - 2, argv + 2, &off, &size, > + nand->size); > + } > + > + if (!nand_unlock( nand, off, size) ) No space before the colsing brace here. > + { Opening brace in "if-statement" line please. > + printf ("NAND flash successfully unlocked\n"); Indentation wrong. > + } else { > + printf ("Error unlocking NAND flash. " > + "Write and erase will probably fail\n"); > + return 1; Again indentation. > + } > + return 0; > + } > usage: > printf("Usage:\n%s\n", cmdtp->usage); > return 1; > } > > U_BOOT_CMD(nand, 5, 1, do_nand, > - "nand - NAND sub-system\n", > + "nand - new NAND sub-system\n", Hmmm. Please don't name this "new" NAND sub-system. Better would be to rename the "old" NAND sub-system to "legacy". > "info - show available NAND devices\n" > "nand device [dev] - show or set current device\n" > "nand read[.jffs2] - addr off size\n" > @@ -277,7 +463,9 @@ > "nand dump[.oob] off - dump page\n" > "nand scrub - really clean NAND erasing bad blocks (UNSAFE)\n" > "nand markbad off - mark bad block at offset (UNSAFE)\n" > - "nand biterr off - make a bit error at offset (UNSAFE)\n"); > + "nand biterr off - make a bit error at offset (UNSAFE)\n" > + "nand lock [tight] [status] - bring nand to lock state or display locked > pages\n" + "nand unlock [offset] [size] - unlock section\n"); > > int do_nandboot(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[]) > { > diff -Nrub --exclude='*~' --exclude=.depend u-boot-weiss-org/include/nand.h > u-boot-weiss/include/nand.h --- u-boot-weiss-org/include/nand.h 2006-09-22 > 11:50:15.000000000 +0200 +++ u-boot-weiss/include/nand.h 2006-10-06 > 16:16:18.000000000 +0200 @@ -60,4 +60,61 @@ > return info->erase(info, &instr); > } > > + > +/***************************************************************************** > + * declarations from nand_util.c > + > ****************************************************************************/ > + > + > +struct nand_write_options { > + u_char *buffer; /* memory block containing image to write */ > + ulong length; /* number of bytes to write */ > + ulong offset; /* start address in NAND */ > + int quiet; /* don't display progress messages */ > + int autoplace; /* if true use auto oob layout */ > + int forcejffs2; /* force jffs2 oob layout */ > + int forceyaffs; /* force yaffs oob layout */ > + int noecc; /* write without ecc */ > + int writeoob; /* image contains oob data */ > + int pad; /* pad to page size */ > + int blockalign; /* 1|2|4 set multiple of eraseblocks > + * to align to */ > +}; > + > +typedef struct nand_write_options nand_write_options_t; > + > +struct nand_read_options { > + u_char *buffer; /* memory block in which read image is written*/ > + ulong length; /* number of bytes to read */ > + ulong offset; /* start address in NAND */ > + int quiet; /* don't display progress messages */ > + int readoob; /* put oob data in image */ > +}; > + > +typedef struct nand_read_options nand_read_options_t; > + > +struct nand_erase_options { > + ulong length; /* number of bytes to erase */ > + ulong offset; /* first address in NAND to erase */ > + int quiet; /* don't display progress messages */ > + int jffs2; /* if true: format for jffs2 usage > + * (write appropriate cleanmarker blocks) */ > + int scrub; /* if true, really clean NAND by erasing > + * bad blocks (UNSAFE) */ > +}; > + > +typedef struct nand_erase_options nand_erase_options_t; > + > +int nand_write_opts(nand_info_t *meminfo, const nand_write_options_t > *opts); + > +int nand_read_opts(nand_info_t *meminfo, const nand_read_options_t *opts); > +int nand_erase_opts(nand_info_t *meminfo, const nand_erase_options_t > *opts); + > +#define NAND_LOCK_STATUS_TIGHT 0x01 > +#define NAND_LOCK_STATUS_LOCK 0x02 > +#define NAND_LOCK_STATUS_UNLOCK 0x04 > + > +int nand_lock( nand_info_t *meminfo, int tight ); > +int nand_unlock( nand_info_t *meminfo, ulong start, ulong length ); > +int nand_get_lock_status(nand_info_t *meminfo, ulong offset); > #endif > diff -Nrub --exclude='*~' --exclude=.depend > u-boot-weiss-org/drivers/nand/Makefile u-boot-weiss/drivers/nand/Makefile > --- u-boot-weiss-org/drivers/nand/Makefile 2006-09-22 11:50:09.000000000 > +0200 +++ u-boot-weiss/drivers/nand/Makefile 2006-10-05 17:19:20.000000000 > +0200 @@ -25,7 +25,7 @@ > > LIB := $(obj)libnand.a > > -COBJS := nand.o nand_base.o nand_ids.o nand_ecc.o nand_bbt.o > +COBJS := nand.o nand_base.o nand_ids.o nand_ecc.o nand_bbt.o nand_util.o > > SRCS := $(COBJS:.o=.c) > OBJS := $(addprefix $(obj),$(COBJS)) > diff -Nrub --exclude='*~' --exclude=.depend > u-boot-weiss-org/drivers/nand/nand_util.c > u-boot-weiss/drivers/nand/nand_util.c --- > u-boot-weiss-org/drivers/nand/nand_util.c 1970-01-01 01:00:00.000000000 > +0100 +++ u-boot-weiss/drivers/nand/nand_util.c 2006-10-06 > 16:27:49.000000000 +0200 @@ -0,0 +1,863 @@ > +/* > + * drivers/nand/nand_util.c > + * > + * Copyright (C) 2006 by Weiss-Electronic GmbH. > + * All rights reserved. > + * > + * @author: Guido Classen > + * @descr: NAND Flash support > + * @references: borrowed heavily from Linux mtd-utils code: > + * flash_eraseall.c by Arcom Control System Ltd > + * nandwrite.c by Steven J. Hill (sjhill at realitydiluted.com) > + * and Thomas Gleixner (tglx at linutronix.de) > + * > + * See file CREDITS for list of people who contributed to this > + * project. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License version > + * 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, > + * MA 02111-1307 USA > + * > + * @par Modification History: > + * 2006-09-22 gc: initial version No changelog in file itself. > + */ > + > +#include > + > +#if (CONFIG_COMMANDS & CFG_CMD_NAND) && !defined(CFG_NAND_LEGACY) > + > +#include > +#include > +#include > + > +#include > +#include > + > +typedef struct erase_info erase_info_t; > +typedef struct mtd_info mtd_info_t; > + > +/* support only for native endian JFFS2 */ > +#define cpu_to_je16(x) (x) > +#define cpu_to_je32(x) (x) > + > +/*****************************************************************************/ > +static int nand_block_bad_scrub(struct mtd_info *mtd, loff_t ofs, int getchip) > +{ > + return 0; > +} > + > +/** > + * nand_erase_opts: - erase NAND flash with support for various options > + * (jffs2 formating) > + * > + * @param meminfo NAND device to erase > + * @param opts options, @see struct nand_erase_options > + * @return 0 in case of success > + * > + * This code is ported from flash_eraseall.c from Linux mtd utils by > + * Arcom Control System Ltd. > + */ > +int nand_erase_opts(nand_info_t *meminfo, const nand_erase_options_t *opts) > +{ > + struct jffs2_unknown_node cleanmarker; > + int clmpos = 0; > + int clmlen = 8; > + erase_info_t erase; > + ulong erase_length; > + int isNAND; > + int bbtest = 1; > + int result; > + int (*nand_block_bad_old)(struct mtd_info *, loff_t, int) = NULL; > + const char *mtd_device = meminfo->name; > + > + 2 empty lines. > + memset(&erase, 0, sizeof(erase)); > + > + erase.mtd = meminfo; > + erase.len = meminfo->erasesize; > + if (opts->offset == 0 && opts->length == 0) { > + /* erase complete chip */ > + erase.addr = 0; > + erase_length = meminfo->size; > + } else { > + /* erase specified region */ > + erase.addr = opts->offset; > + erase_length = opts->length; > + } > + > + isNAND = meminfo->type == MTD_NANDFLASH ? 1 : 0; > + > + /* > + * printf("%s: length: %d, isNAND: %d\n", > + * mtd_device, erase_length, isNAND); > + */ Please use only tabs for indentation. > + > + if (opts->jffs2) { > + cleanmarker.magic = cpu_to_je16 (JFFS2_MAGIC_BITMASK); > + cleanmarker.nodetype = cpu_to_je16 (JFFS2_NODETYPE_CLEANMARKER); > + if (!isNAND) > + cleanmarker.totlen = > + cpu_to_je32 (sizeof(struct jffs2_unknown_node)); > + else { When the else statement has braces, please also use them in the if statement. > + struct nand_oobinfo *oobinfo = &meminfo->oobinfo; > + > +#if 1 Why "#if 1"? Either a "good" comment here, or remove the #if. > + /* this seems not to work in u-boot... why??? */ > + /* Check for autoplacement */ > + if (oobinfo->useecc == MTD_NANDECC_AUTOPLACE) { > + /* Get the position of the free bytes */ > + if (!oobinfo->oobfree[0][1]) { > + printf(" Eeep. Autoplacement selected " > + "and no empty space in oob\n"); > + return -1; > + } > + clmpos = oobinfo->oobfree[0][0]; > + clmlen = oobinfo->oobfree[0][1]; > + if (clmlen > 8) > + clmlen = 8; > + } else > +#endif > + { > + /* Legacy mode */ > + switch (meminfo->oobsize) { > + case 8: > + clmpos = 6; > + clmlen = 2; > + break; > + case 16: > + clmpos = 8; > + clmlen = 8; > + break; > + case 64: > + clmpos = 16; > + clmlen = 8; > + break; > + } > + } > + > + /* > + * printf("oobsize =%d\nclmpos = %d\nclmlen = > %d\n", + * meminfo->oobsize, > + * clmpos, clmlen); > + */ Only tabs for indentation. > + cleanmarker.totlen = cpu_to_je32(8); > + } > + cleanmarker.hdr_crc = cpu_to_je32( > + crc32_no_comp(0, (unsigned char *) &cleanmarker, > + sizeof(struct jffs2_unknown_node) - 4)); > + } > + > + /* scrub option allows to erase badblock. To prevent internal > + * check from erase() method, set block check method to dummy > + * and disable bad block table while erasing. > + */ > + if (opts->scrub) { > + struct nand_chip *priv_nand = meminfo->priv; > + > + nand_block_bad_old = priv_nand->block_bad; > + priv_nand->block_bad = nand_block_bad_scrub; > + /* we don't need the bad block table anymore... > + * after scrub, there are no bad blocks left! > + */ > + if (priv_nand->bbt) { > + kfree(priv_nand->bbt); > + } > + priv_nand->bbt = NULL; > + } > + > + for (; > + erase.addr < opts->offset + erase_length; > + erase.addr += meminfo->erasesize) { > + > + WATCHDOG_RESET (); > + > + if ( !opts->scrub && bbtest ) { No spaces after the opening brace and before the closing brace. > + int ret = meminfo->block_isbad(meminfo, erase.addr); > + if (ret > 0) { > + if (!opts->quiet) > + printf ("\rSkipping bad block at " > + "0x%08x " > + " \n", > + erase.addr); > + continue; > + } else if (ret < 0) { > + printf("\n%s: MTD get bad block failed: %d\n", > + mtd_device, > + ret); > + return -1; > + } > + } > + > + if (!opts->quiet) { > + printf > + ("\rErasing %d Kibyte at %x -- %2lu%% complete.", > + meminfo->erasesize >> 10, erase.addr, > + (unsigned long) > + ((unsigned long long) > + erase.addr * 100 / meminfo->size)); > + } > + > + result = meminfo->erase(meminfo, &erase); > + if (result != 0) { > + printf("\n%s: MTD Erase failure: %d\n", > + mtd_device, result); > + continue; > + } > + > + /* format for JFFS2 ? */ > + if (!opts->jffs2) > + continue; > + > + /* write cleanmarker */ > + if (isNAND) { > + size_t written; > + result = meminfo->write_oob(meminfo, > + erase.addr + clmpos, > + clmlen, > + &written, > + (unsigned char *) > + &cleanmarker); > + if (result != 0) { > + printf("\n%s: MTD writeoob failure: %d\n", > + mtd_device, result); > + continue; > + } > + } else { > + printf("\n%s: this erase routine only supports" > + " NAND devices!\n", > + mtd_device); > + > + } > + if (!opts->quiet) > + printf(" Cleanmarker written at %x.", erase.addr); > + } > + if (!opts->quiet) > + printf("\n"); > + > + if (nand_block_bad_old) { > + struct nand_chip *priv_nand = meminfo->priv; > + > + priv_nand->block_bad = nand_block_bad_old; > + priv_nand->scan_bbt(meminfo); > + } > + > + return 0; > +} > + > + > +#define MAX_PAGE_SIZE 2048 > +#define MAX_OOB_SIZE 64 > + > +/* > + * Buffer array used for writing data > + */ > +static unsigned char data_buf[MAX_PAGE_SIZE]; > +static unsigned char oob_buf[MAX_OOB_SIZE]; > + > +// oob layouts to pass into the kernel as default No C++ style comment please. I'm stopping here for now. Please check your code again. > +/* > + *Local Variables: > + * mode: c > + * c-file-style: "linux" > + * End: > + */ Please drop this c-style paragraph. And finally I get some warnings and a compilation error: nand_util.c: In function 'nand_get_lock_status': nand_util.c:748: warning: unused variable 'status' nand_util.c: In function 'nand_read_opts': nand_util.c:546: warning: 'pagelen' is used uninitialized in this function cmd_nand.c: In function 'do_nand': cmd_nand.c:384: warning: 'last_status' may be used uninitialized in this function drivers/nand/libnand.a(nand_util.o): In function `nand_erase_opts': nand_util.c:155: undefined reference to `crc32_no_comp' Please fix the above mentioned issues and resubmit a new patch. Thanks. Best regards, Stefan