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

[Stefan Roese <sr@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? <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 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 <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