Re: [U-Boot-Users] [PATCH] New NAND subsystem: mtd like commands support jffs2 and bad blocks

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? <y/N>\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 clagix@gmail.com
• • @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@realitydiluted.com)
    

• • 	       and Thomas Gleixner (tglx@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 <common.h>

+#if (CONFIG_COMMANDS & CFG_CMD_NAND) && !defined(CFG_NAND_LEGACY)

+#include <command.h> +#include <watchdog.h> +#include <malloc.h>

+#include <nand.h> +#include <jffs2/jffs2.h>

+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.

<snip>

+/*

• *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