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

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

[Guido Classen]

Hello Ladis,
hello list,

the attached patch implements several improvements to the new NAND subsystem:

JFFS2 related commands:

  implement "nand erase clean" and old "nand erase"
  using both the new code which is able to skip bad blocks
  "nand erase clean" additionally writes JFFS2-cleanmarkers in the oob.

  "nand write.jffs2"
  like "nand write" but skip found bad eraseblocks

  "nand read.jffs2"
  like "nand read" but skip found bad eraseblocks

Miscellaneous and testing commands:
  "markbad [offset]"
  create an artificial bad block (for testing bad block handling)

  "scrub [offset length]"
  like "erase" but don't skip bad block. Instead erase them.
  DANGEROUS!!! Factory set bad blocks will be lost. Use only
  to remove artificial bad blocks created with the "markbad" command.


NAND locking command (for chips with active LOCKPRE pin)

  "nand lock"
  set NAND chip to lock state (all pages locked)

  "nand lock tight"
  set NAND chip to lock tight state (software can't change locking anymore)

  "nand lock status"
  displays current locking status of all pages

  "nand unlock [offset] [size]"
  unlock consecutive area (can be called multiple times for different areas)


I have tested the code with board containing 128MiB NAND large page chips
and 32MiB small page chips.

CHANGELOG:
* Several improvements to the new NAND subsystem: JFFS2 related commands
  implemented in mtd-utils style, support for bad blocks, bad block testing
  commands, NAND lock commands
  Patch by Guido Classen, 06 Oct 2006



Regards

  Guido
-------------- next part --------------
A non-text attachment was scrubbed...
Name: nand-jffs2.patch.gz
Type: application/x-gzip
Size: 8920 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20061006/49569b42/attachment.bin 

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

[Stefan Roese]

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

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

[Guido Classen]

Hello Stefan,

> 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:
>

thank you for reviewing the patch. Sorry for wrong coding style on some places.
Some code was as said ported from linux mtd-utils code.

I have followed your suggestion to rename the old NAND sub-system to
"legacy NAND sub-system". It was very important during my tests to
quickly identify which type of NAND sub-system was installed in the
different boards laying around. Thus I renamed the new one.

Attached a revised patch following your suggestions regarding coding style.

Regards
  Guido
-------------- next part --------------
A non-text attachment was scrubbed...
Name: nand-jffs2-revised.patch.gz
Type: application/x-gzip
Size: 9044 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20061007/84b45206/attachment.bin 

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

[Stefan Roese]

Hi Guido,

On Saturday 07 October 2006 16:20, Guido Classen wrote:
> > Looks promising. I'm not completely happy with the implementation though:
>
> thank you for reviewing the patch. Sorry for wrong coding style on some
> places. Some code was as said ported from linux mtd-utils code.

Understood. Thanks.

> I have followed your suggestion to rename the old NAND sub-system to
> "legacy NAND sub-system". It was very important during my tests to
> quickly identify which type of NAND sub-system was installed in the
> different boards laying around. Thus I renamed the new one.

OK.

> Attached a revised patch following your suggestions regarding coding style.

Nearly done. Here some further remarks:

> diff -Nrub --exclude='*~' --exclude=.depend
> u-boot-weiss-unpatched/common/cmd_nand.c u-boot-weiss/common/cmd_nand.c ---
> u-boot-weiss-unpatched/common/cmd_nand.c	2006-10-07 14:46:21.000000000
> +0200 +++ u-boot-weiss/common/cmd_nand.c	2006-10-07 15:57:14.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,68 @@
>  		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);
> +
>  		if (off == 0 && size == 0)
>  			return 1;

Indentation wrong here.

>
> -		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;
> +		opts.quiet  = 1;
> +
> +		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;
> @@ -228,37 +285,158 @@
>
>  	/* read write */
>  	if (strncmp(cmd, "read", 4) == 0 || strncmp(cmd, "write", 5) == 0) {
> +		int read;
> +
>  		if (argc < 4)
>  			goto usage;
> -/*
> -		s = strchr(cmd, '.');
> -		clean = CLEAN_NONE;
> -		if (s != NULL) {
> -			if (strcmp(s, ".jffs2") == 0 || strcmp(s, ".e") == 0
> -			    || strcmp(s, ".i"))
> -				clean = CLEAN_JFFS2;
> -		}
> -*/
> +
>  		addr = (ulong)simple_strtoul(argv[2], NULL, 16);
>
>  		arg_off_size(argc - 3, argv + 3, &off, &size, nand->size);
>  		if (off == 0 && size == 0)
>  			return 1;
>
> -		i = strncmp(cmd, "read", 4) == 0;	/* 1 = read, 0 = write */
> +		read = strncmp(cmd, "read", 4) == 0; /* 1 = read, 0 = write */
>  		printf("\nNAND %s: device %d offset %u, size %u ... ",
> -		       i ? "read" : "write", nand_curr_device, off, size);
> +		       read ? "read" : "write", nand_curr_device, off, size);
> +
> +		s = strchr(cmd, '.');
> +		if (s != NULL
> +		    && (!strcmp(s, ".jffs2") || !strcmp(s, ".e")
> +			|| !strcmp(s, ".i"))) {
> +			if (read) {
> +				/* read */
> +				nand_read_options_t opts;
> +				memset(&opts, 0, sizeof(opts));
> +				opts.buffer	= (u_char*) addr;
> +				opts.length	= size;
> +				opts.offset	= off;
> +				opts.quiet      = 1;
> +				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;
> +				opts.quiet      = 1;
> +				ret = nand_write_opts(nand, &opts);
> +			}
> +			printf("%s\n", ret ? "ERROR" : "OK");
> +			return ret == 0 ? 0 : 1;
> +		}
>
> -		if (i)
> +		if (read)
>  			ret = nand_read(nand, off, &size, (u_char *)addr);
>  		else
>  			ret = nand_write(nand, off, &size, (u_char *)addr);
>
>  		printf(" %d bytes %s: %s\n", size,
> -		       i ? "read" : "written", ret ? "ERROR" : "OK");
> +		       read ? "read" : "written", ret ? "ERROR" : "OK");
>
>  		return ret == 0 ? 0 : 1;
>  	}
> +
> +	/* 2006-09-28 gc: implement missing commands */
> +	if (strcmp(cmd, "markbad") == 0) {
> +		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 = -1;
> +
> +			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;
> +		       }
> +
> +		} else {
> +
> +			if (!nand_lock( nand, tight)) {
> +				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)) {
> +			printf ("NAND flash successfully unlocked\n");
> +		} else {
> +			printf ("Error unlocking NAND flash. "
> +				"Write and erase will probably fail\n");
> +			return 1;
> +		}
> +		return 0;
> +	}
> +
>  usage:
>  	printf("Usage:\n%s\n", cmdtp->usage);
>  	return 1;
> @@ -277,7 +455,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[])
>  {
> @@ -635,7 +815,7 @@
>
>  U_BOOT_CMD(
>  	nand,	5,	1,	do_nand,
> -	"nand    - NAND sub-system\n",
> +	"nand    - legacy NAND sub-system\n",
>  	"info  - show available NAND devices\n"
>  	"nand device [dev] - show or set current device\n"
>  	"nand read[.jffs2[s]]  addr off size\n"
> diff -Nrub --exclude='*~' --exclude=.depend
> u-boot-weiss-unpatched/drivers/nand/Makefile
> u-boot-weiss/drivers/nand/Makefile ---
> u-boot-weiss-unpatched/drivers/nand/Makefile	2006-10-07 14:47:02.000000000
> +0200 +++ u-boot-weiss/drivers/nand/Makefile	2006-10-07 15:57:14.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-unpatched/drivers/nand/nand_util.c
> u-boot-weiss/drivers/nand/nand_util.c ---
> u-boot-weiss-unpatched/drivers/nand/nand_util.c	1970-01-01
> 01:00:00.000000000 +0100 +++
> u-boot-weiss/drivers/nand/nand_util.c	2006-10-07 15:57:14.000000000 +0200
> @@ -0,0 +1,839 @@
> +/*
> + * 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
> + *
> + */
> +
> +#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;
> +
> +	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;
> +
> +	if (opts->jffs2) {
> +		cleanmarker.magic = cpu_to_je16 (JFFS2_MAGIC_BITMASK);
> +		cleanmarker.nodetype = cpu_to_je16 (JFFS2_NODETYPE_CLEANMARKER);
> +		if (isNAND) {
> +			struct nand_oobinfo *oobinfo = &meminfo->oobinfo;
> +
> +			/* 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 {
> +				/* 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;
> +				}
> +			}
> +
> +			cleanmarker.totlen = cpu_to_je32(8);
> +		} else {
> +			cleanmarker.totlen =
> +				cpu_to_je32 (sizeof(struct jffs2_unknown_node));
> +		}
> +		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 opening brace and before 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 */
> +static struct nand_oobinfo none_oobinfo = {
> +	.useecc = MTD_NANDECC_OFF,
> +};
> +
> +static struct nand_oobinfo jffs2_oobinfo = {
> +	.useecc = MTD_NANDECC_PLACE,
> +	.eccbytes = 6,
> +	.eccpos = { 0, 1, 2, 3, 6, 7 }
> +};
> +
> +static struct nand_oobinfo yaffs_oobinfo = {
> +	.useecc = MTD_NANDECC_PLACE,
> +	.eccbytes = 6,
> +	.eccpos = { 8, 9, 10, 13, 14, 15}
> +};
> +
> +static struct nand_oobinfo autoplace_oobinfo = {
> +	.useecc = MTD_NANDECC_AUTOPLACE
> +};
> +
> +
> +/**
> + * nand_write_opts: - write image to NAND flash with support for various
> options + *
> + * @param meminfo	NAND device to erase
> + * @param opts		write options (@see nand_write_options)
> + * @return		0 in case of success
> + *
> + * This code is ported from nandwrite.c from Linux mtd utils by
> + * Steven J. Hill and Thomas Gleixner.
> + */
> +int nand_write_opts(nand_info_t *meminfo, const nand_write_options_t
> *opts) +{
> +	int imglen = 0;
> +	int pagelen;
> +	int baderaseblock;
> +	int blockstart = -1;
> +	loff_t offs;
> +	int readlen;
> +	int oobinfochanged = 0;
> +	struct nand_oobinfo old_oobinfo;
> +	ulong mtdoffset = opts->offset;
> +	ulong erasesize_blockalign;
> +	u_char *buffer = opts->buffer;
> +	size_t written;
> +	int result;
> +
> +	if (opts->pad && opts->writeoob) {
> +		printf("Can't pad when oob data is present.\n");
> +		return -1;
> +	}
> +
> +	/* set erasesize to specified number of blocks - to match
> +	 * jffs2 (virtual) block size */
> +	if (opts->blockalign == 0) {
> +		erasesize_blockalign = meminfo->erasesize;
> +	} else {
> +		erasesize_blockalign = meminfo->erasesize * opts->blockalign;
> +	}
> +
> +	/* make sure device page sizes are valid */
> +	if (!(meminfo->oobsize == 16 && meminfo->oobblock == 512)
> +	    && !(meminfo->oobsize == 8 && meminfo->oobblock == 256)
> +	    && !(meminfo->oobsize == 64 && meminfo->oobblock == 2048)) {
> +		printf("Unknown flash (not normal NAND)\n");
> +		return -1;
> +	}
> +
> +	/* read the current oob info */
> +	memcpy(&old_oobinfo, &meminfo->oobinfo, sizeof(old_oobinfo));
> +
> +	/* write without ecc? */
> +	if (opts->noecc) {
> +		memcpy(&meminfo->oobinfo, &none_oobinfo,
> +		       sizeof(meminfo->oobinfo));
> +		oobinfochanged = 1;
> +	}
> +
> +	/* autoplace ECC? */
> +	if (opts->autoplace && (old_oobinfo.useecc != MTD_NANDECC_AUTOPLACE)) {
> +
> +		memcpy(&meminfo->oobinfo, &autoplace_oobinfo,
> +		       sizeof(meminfo->oobinfo));
> +		oobinfochanged = 1;
> +	}
> +
> +	/* force OOB layout for jffs2 or yaffs? */
> +	if (opts->forcejffs2 || opts->forceyaffs) {
> +		struct nand_oobinfo *oobsel =
> +			opts->forcejffs2 ? &jffs2_oobinfo : &yaffs_oobinfo;
> +
> +		if (meminfo->oobsize == 8) {
> +			if (opts->forceyaffs) {
> +				printf("YAFSS cannot operate on "
> +				       "256 Byte page size\n");
> +				goto restoreoob;
> +			}
> +			/* Adjust number of ecc bytes */
> +			jffs2_oobinfo.eccbytes = 3;
> +		}
> +
> +		memcpy(&meminfo->oobinfo, oobsel, sizeof(meminfo->oobinfo));
> +	}
> +
> +	/* get image length */
> +	imglen = opts->length;
> +	pagelen = meminfo->oobblock
> +		+ ((opts->writeoob != 0) ? meminfo->oobsize : 0);
> +
> +	/* check, if file is pagealigned */
> +	if ((!opts->pad) && ((imglen % pagelen) != 0)) {
> +		printf("Input block length is not page aligned\n");
> +		goto restoreoob;
> +	}
> +
> +	/* check, if length fits into device */
> +	if ( ((imglen / pagelen) * meminfo->oobblock)

No space after opening brace.

> +	     > (meminfo->size - opts->offset)) {
> +		printf("Image %d bytes, NAND page %d bytes, "
> +		       "OOB area %u bytes, device size %u bytes\n",
> +		       imglen, pagelen, meminfo->oobblock, meminfo->size);
> +		printf("Input block does not fit into device\n");
> +		goto restoreoob;
> +	}
> +
> +	if (!opts->quiet)
> +		printf("\n");
> +
> +	/* get data from input and write to the device */
> +	while (imglen && (mtdoffset < meminfo->size)) {
> +
> +		WATCHDOG_RESET ();
> +
> +		/*
> +		 * new eraseblock, check for bad block(s). Stay in the
> +		 * loop to be sure if the offset changes because of
> +		 * a bad block, that the next block that will be
> +		 * written to is also checked. Thus avoiding errors if
> +		 * the block(s) after the skipped block(s) is also bad
> +		 * (number of blocks depending on the blockalign
> +		 */
> +		while (blockstart != (mtdoffset & (~erasesize_blockalign+1))) {
> +			blockstart = mtdoffset & (~erasesize_blockalign+1);
> +			offs = blockstart;
> +			baderaseblock = 0;
> +			if (!opts->quiet)
> +				printf("Writing data to block %x \r",
> +				       blockstart);
> +
> +			/* check all the blocks in an erase block for
> +			 * bad blocks */
> +			do {
> +				int ret = meminfo->block_isbad(meminfo, offs);
> +
> +				if (ret < 0 ) {

No space before closing brace.

> +					printf("Bad block check failed\n");
> +					goto restoreoob;
> +				}
> +				if (ret == 1) {
> +					baderaseblock = 1;
> +					if (!opts->quiet)
> +						printf("Bad block at %lx, "
> +						       "%u block(s) from "
> +						       "%x will be skipped\n",
> +						       (long) offs,
> +						       opts->blockalign,
> +						       blockstart);
> +				}
> +
> +				if (baderaseblock) {
> +					mtdoffset = blockstart
> +						+ erasesize_blockalign;
> +				}
> +				offs +=	 erasesize_blockalign
> +					/ opts->blockalign;
> +			} while ( offs < blockstart + erasesize_blockalign );

Spaces in braces again.

> +		}
> +
> +		readlen = meminfo->oobblock;
> +		if (opts->pad && (imglen < readlen)) {
> +			readlen = imglen;
> +			memset(data_buf + readlen, 0xff,
> +			       meminfo->oobblock - readlen);
> +		}
> +
> +		/* read page data from input memory buffer */
> +		memcpy(data_buf, buffer, readlen);
> +		buffer += readlen;
> +
> +		if (opts->writeoob) {
> +			/* read OOB data from input memory block, exit
> +			 * on failure */
> +			memcpy(oob_buf, buffer, meminfo->oobsize);
> +			buffer += meminfo->oobsize;
> +
> +			/* write OOB data first, as ecc will be placed
> +			 * in there*/
> +			result = meminfo->write_oob(meminfo,
> +						    mtdoffset,
> +						    meminfo->oobsize,
> +						    &written,
> +						    (unsigned char *)
> +						    &oob_buf);
> +
> +			if (result != 0) {
> +				printf("\nMTD writeoob failure: %d\n",
> +				       result);
> +				goto restoreoob;
> +			}
> +			imglen -= meminfo->oobsize;
> +		}
> +
> +		/* write out the page data */
> +		result = meminfo->write(meminfo,
> +					mtdoffset,
> +					meminfo->oobblock,
> +					&written,
> +					(unsigned char *) &data_buf);
> +
> +		if (result != 0) {
> +			printf("writing NAND page at offset %lx failed\n",
> +			       mtdoffset);
> +			goto restoreoob;
> +		}
> +		imglen -= readlen;
> +		mtdoffset += meminfo->oobblock;
> +	}
> +
> +	if (!opts->quiet)
> +		printf("\n");
> +
> +restoreoob:
> +	if (oobinfochanged) {
> +		memcpy(&meminfo->oobinfo, &old_oobinfo,
> +		       sizeof(meminfo->oobinfo));
> +	}
> +
> +	if (imglen > 0) {
> +		printf("Data did not fit into device, due to bad blocks\n");
> +		return -1;
> +	}
> +
> +	/* return happy */
> +	return 0;
> +}
> +
> +
> +/**
> + * nand_read_opts: - read image from NAND flash with support for various
> options + *
> + * @param meminfo	NAND device to erase
> + * @param opts		read options (@see struct nand_read_options)
> + * @return		0 in case of success
> + *
> + */
> +int nand_read_opts(nand_info_t *meminfo, const nand_read_options_t *opts)
> +{
> +	int imglen = opts->length;
> +	int pagelen;
> +	int baderaseblock;
> +	int blockstart = -1;
> +	loff_t offs;
> +	size_t readlen;
> +	ulong mtdoffset = opts->offset;
> +	u_char *buffer = opts->buffer;
> +	int result;
> +
> +        printf("nand_read_opts\n");
> +	/* make sure device page sizes are valid */
> +	if (!(meminfo->oobsize == 16 && meminfo->oobblock == 512)
> +	    && !(meminfo->oobsize == 8 && meminfo->oobblock == 256)
> +	    && !(meminfo->oobsize == 64 && meminfo->oobblock == 2048)) {
> +		printf("Unknown flash (not normal NAND)\n");
> +		return -1;
> +	}
> +
> +	pagelen = meminfo->oobblock
> +		+ ((opts->readoob != 0) ? meminfo->oobsize : 0);
> +
> +	/* check, if length is not larger than device */
> +	if ( ((imglen / pagelen) * meminfo->oobblock)

Space in brace.

> +	     > (meminfo->size - opts->offset)) {
> +		printf("Image %d bytes, NAND page %d bytes, "
> +		       "OOB area %u bytes, device size %u bytes\n",
> +		       imglen, pagelen, meminfo->oobblock, meminfo->size);
> +		printf("Input block is larger than device\n");
> +		return -1;
> +	}
> +
> +	if (!opts->quiet)
> +		printf("\n");
> +
> +	/* get data from input and write to the device */
> +	while (imglen && (mtdoffset < meminfo->size)) {
> +
> +		WATCHDOG_RESET ();
> +
> +		/*
> +		 * new eraseblock, check for bad block(s). Stay in the
> +		 * loop to be sure if the offset changes because of
> +		 * a bad block, that the next block that will be
> +		 * written to is also checked. Thus avoiding errors if
> +		 * the block(s) after the skipped block(s) is also bad
> +		 * (number of blocks depending on the blockalign
> +		 */
> +		while (blockstart != (mtdoffset & (~meminfo->erasesize+1))) {
> +			blockstart = mtdoffset & (~meminfo->erasesize+1);
> +			offs = blockstart;
> +			baderaseblock = 0;
> +			if (!opts->quiet)
> +				printf("Reading data from block %x \r",
> +				       blockstart);
> +
> +			/* check all the blocks in an erase block for
> +			 * bad blocks */
> +			do {
> +				int ret = meminfo->block_isbad(meminfo, offs);
> +
> +				if (ret < 0 ) {

Space in brace.

> +					printf("Bad block check failed\n");
> +					return -1;
> +				}
> +				if (ret == 1) {
> +					baderaseblock = 1;
> +					if (!opts->quiet)
> +						printf("Bad block at %lx in "
> +						       "erase block from %x "
> +						       "will be skipped\n",
> +						       (long) offs,
> +						       blockstart);
> +				}
> +
> +				if (baderaseblock) {
> +					mtdoffset = blockstart
> +						+ meminfo->erasesize;
> +				}
> +				offs +=	 meminfo->erasesize;
> +
> +			} while (offs < blockstart + meminfo->erasesize);
> +		}
> +
> +
> +		/* read page data to memory buffer */
> +		result = meminfo->read(meminfo,
> +				       mtdoffset,
> +				       meminfo->oobblock,
> +				       &readlen,
> +				       (unsigned char *) &data_buf);
> +
> +		if (result != 0) {
> +			printf("reading NAND page at offset %lx failed\n",
> +			       mtdoffset);
> +			return -1;
> +		}
> +
> +		if (imglen < readlen) {
> +			readlen = imglen;
> +		}
> +
> +		memcpy(buffer, data_buf, readlen);
> +		buffer += readlen;
> +		imglen -= readlen;
> +
> +		if (opts->readoob) {
> +			result = meminfo->read_oob(meminfo,
> +						   mtdoffset,
> +						   meminfo->oobsize,
> +						   &readlen,
> +						   (unsigned char *)
> +						   &oob_buf);
> +
> +			if (result != 0) {
> +				printf("\nMTD readoob failure: %d\n",
> +				       result);
> +				return -1;
> +			}
> +
> +
> +			if (imglen < readlen) {
> +				readlen = imglen;
> +			}
> +
> +			memcpy(buffer, oob_buf, readlen);
> +
> +			buffer += readlen;
> +			imglen -= readlen;
> +		}
> +
> +		mtdoffset += meminfo->oobblock;
> +	}
> +
> +	if (!opts->quiet)
> +		printf("\n");
> +
> +	if (imglen > 0) {
> +		printf("Could not read entire image due to bad blocks\n");
> +		return -1;
> +	}
> +
> +	/* return happy */
> +	return 0;
> +}
> +
> +
> +/*************************************************************************
>***** + * Support for locking / unlocking operations of some NAND devices +
> ***************************************************************************
>**/ +
> +#define NAND_CMD_LOCK		0x2a
> +#define NAND_CMD_LOCK_TIGHT	0x2c
> +#define NAND_CMD_UNLOCK1	0x23
> +#define NAND_CMD_UNLOCK2	0x24
> +#define NAND_CMD_LOCK_STATUS	0x7a
> +
> +/**
> + * nand_lock: Set all pages of NAND flash chip to the LOCK or LOCK-TIGHT
> + *	      state
> + *
> + * @param meminfo	nand mtd instance
> + * @param tight		bring device in lock tight mode
> + *
> + * @return		0 on success, -1 in case of error
> + *
> + * The lock / lock-tight command only applies to the whole chip. To get
> some + * parts of the chip lock and others unlocked use the following
> sequence: + *
> + * - Lock all pages of the chip using nand_lock(mtd, 0) (or the lockpre
> pin) + * - Call nand_unlock() once for each consecutive area to be unlocked
> + * - If desired: Bring the chip to the lock-tight state using
> nand_lock(mtd, 1) + *
> + *   If the device is in lock-tight state software can't change the
> + *   current active lock/unlock state of all pages. nand_lock() /
> nand_unlock() + *   calls will fail. It is only posible to leave lock-tight
> state by + *   an hardware signal (low pulse on _WP pin) or by power down.
> + */
> +int nand_lock(nand_info_t *meminfo, int tight)
> +{
> +	int ret = 0;
> +	int status;
> +	struct nand_chip *this = meminfo->priv;
> +
> +	/* select the NAND device */
> +	this->select_chip(meminfo, 0);
> +
> +	this->cmdfunc(meminfo,
> +		      (tight ? NAND_CMD_LOCK_TIGHT : NAND_CMD_LOCK),
> +		      -1, -1);
> +
> +	/* call wait ready function */
> +	status = this->waitfunc(meminfo, this, FL_WRITING);
> +
> +	/* see if device thinks it succeeded */
> +	if (status & 0x01) {
> +		ret = -1;
> +	}
> +
> +	/* de-select the NAND device */
> +	this->select_chip(meminfo, -1);
> +	return ret;
> +}
> +
> +
> +/**
> + * nand_get_lock_status: - query current lock state from one page of NAND
> + *			   flash
> + *
> + * @param meminfo	nand mtd instance
> + * @param offset	page address to query (muss be page aligned!)
> + *
> + * @return		-1 in case of error
> + *			>0 lock status:
> + *			  bitfield with the following combinations:
> + *			  NAND_LOCK_STATUS_TIGHT: page in tight state
> + *			  NAND_LOCK_STATUS_LOCK:  page locked
> + *			  NAND_LOCK_STATUS_UNLOCK: page unlocked
> + *
> + */
> +int nand_get_lock_status(nand_info_t *meminfo, ulong offset)
> +{
> +	int ret = 0;
> +	int chipnr;
> +	int page;
> +	struct nand_chip *this = meminfo->priv;
> +
> +	/* select the NAND device */
> +	chipnr = (int)(offset >> this->chip_shift);
> +	this->select_chip(meminfo, chipnr);
> +
> +
> +	if ((offset & (meminfo->oobblock - 1)) != 0) {
> +		printf ("nand_get_lock_status: "
> +			"Start address must be beginning of "
> +			"nand page!\n");
> +		ret = -1;
> +		goto out;
> +	}
> +
> +	/* check the Lock Status */
> +	page = (int)(offset >> this->page_shift);
> +	this->cmdfunc(meminfo, NAND_CMD_LOCK_STATUS, -1, page & this->pagemask);
> +
> +	ret = this->read_byte(meminfo) & (NAND_LOCK_STATUS_TIGHT
> +					  | NAND_LOCK_STATUS_LOCK
> +					  | NAND_LOCK_STATUS_UNLOCK);
> +
> + out:
> +	/* de-select the NAND device */
> +	this->select_chip(meminfo, -1);
> +	return ret;
> +}
> +
> +/**
> + * nand_unlock: - Unlock area of NAND pages
> + *		  only one consecutive area can be unlocked at one time!
> + *
> + * @param meminfo	nand mtd instance
> + * @param start		start byte address
> + * @param length	number of bytes to unlock (must be a multiple of
> + *			page size nand->oobblock)
> + *
> + * @return		0 on success, -1 in case of error
> + */
> +int nand_unlock(nand_info_t *meminfo, ulong start, ulong length)
> +{
> +	int ret = 0;
> +	int chipnr;
> +	int status;
> +	int page;
> +	struct nand_chip *this = meminfo->priv;
> +	printf ("nand_unlock: start: %08x, length: %d!\n",
> +		(int)start, (int)length);
> +
> +	/* select the NAND device */
> +	chipnr = (int)(start >> this->chip_shift);
> +	this->select_chip(meminfo, chipnr);
> +
> +	/* check the WP bit */
> +	this->cmdfunc(meminfo, NAND_CMD_STATUS, -1, -1);
> +	if ((this->read_byte(meminfo) & 0x80) == 0) {
> +		printf ("nand_unlock: Device is write protected!\n");
> +		ret = -1;
> +		goto out;
> +	}
> +
> +	if ((start & (meminfo->oobblock - 1)) != 0) {
> +		printf ("nand_unlock: Start address must be beginning of "
> +			"nand page!\n");
> +		ret = -1;
> +		goto out;
> +	}
> +
> +	if (length == 0 || (length & (meminfo->oobblock - 1)) != 0) {
> +		printf ("nand_unlock: Length must be a multiple of nand page "
> +			"size!\n");
> +		ret = -1;
> +		goto out;
> +	}
> +
> +	/* submit address of first page to unlock */
> +	page = (int)(start >> this->page_shift);
> +	this->cmdfunc(meminfo, NAND_CMD_UNLOCK1, -1, page & this->pagemask);
> +
> +	/* submit ADDRESS of LAST page to unlock */
> +	page += (int)(length >> this->page_shift) - 1;
> +	this->cmdfunc(meminfo, NAND_CMD_UNLOCK2, -1, page & this->pagemask);
> +
> +	/* call wait ready function */
> +	status = this->waitfunc(meminfo, this, FL_WRITING);
> +	/* see if device thinks it succeeded */
> +	if (status & 0x01) {
> +		/* there was an error */
> +		ret = -1;
> +		goto out;
> +	}
> +
> + out:
> +	/* de-select the NAND device */
> +	this->select_chip(meminfo, -1);
> +	return ret;
> +}
> +
> +
> +#endif /* (CONFIG_COMMANDS & CFG_CMD_NAND) && !defined(CFG_NAND_LEGACY) */
> +
> +/*
> + *Local Variables:
> + * mode: c
> + * End:
> + */

Please remove this c-style paragraph.

> diff -Nrub --exclude='*~' --exclude=.depend
> u-boot-weiss-unpatched/include/nand.h u-boot-weiss/include/nand.h ---
> u-boot-weiss-unpatched/include/nand.h	2006-10-07 14:45:24.000000000 +0200
> +++ u-boot-weiss/include/nand.h	2006-10-07 15:57:14.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-unpatched/lib_generic/crc32.c u-boot-weiss/lib_generic/crc32.c
> --- u-boot-weiss-unpatched/lib_generic/crc32.c	2006-10-07
> 15:55:01.000000000 +0200 +++ u-boot-weiss/lib_generic/crc32.c	2006-10-07
> 15:58:13.000000000 +0200 @@ -171,7 +171,9 @@
>      return crc ^ 0xffffffffL;
>  }
>
> -#if (CONFIG_COMMANDS & CFG_CMD_JFFS2)
> +#if (CONFIG_COMMANDS & CFG_CMD_JFFS2)					\
> +	|| (CONFIG_COMMANDS & CFG_CMD_NAND) && !defined(CFG_NAND_LEGACY)
> +
>
>  /* No ones complement version. JFFS2 (and other things ?)
>   * don't use ones compliment in their CRC calculations.

Now it compiles clean for my testplatform. Thanks.

As it seems you have set the "quiet" option for all commands. Because of
this we don't see the skipping of bad blocks message for example. So please
disable the quiet option.

Thanks.

Best regards,
Stefan

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

[Guido Classen]

Hi Stefan,

thanks for your comments.

>> ... a lot of patch ...
>> +
>> +	for (;
>> +	     erase.addr < opts->offset + erase_length;
>> +	     erase.addr += meminfo->erasesize) {
>> +
>> +		WATCHDOG_RESET ();
>> +
>> +		if ( !opts->scrub && bbtest ) {
>
>No spaces after opening brace and before closing brace.
>

Now I finally know what you meant. AFAIK the round ones are called
parenthesis, not brace! Braces are the curly and square onse.


>> +
>> +
>> +#endif /* (CONFIG_COMMANDS & CFG_CMD_NAND) && !defined(CFG_NAND_LEGACY) */
>> +
>> +/*
>> + *Local Variables:
>> + * mode: c
>> + * End:
>> + */
>
>Please remove this c-style paragraph.
>
Okay

>Now it compiles clean for my testplatform. Thanks.
>
>As it seems you have set the "quiet" option for all commands. Because of
>this we don't see the skipping of bad blocks message for example. So please
>disable the quiet option.
>

You're right. I set the "quiet" option for all commands. I have done some
testing again and get an impression that the output of progress messages on
the serial console takes too much time. It seems that they slow down the
operations especially on small page devices (four times more messages). Hence
I think the best solution would be to make them switchable on command line, so
they can be switched on for testing and off for normal operation.

Maybe a "quiet" option at command line for the related commands (like "clean"
option) will be the right way to do it. What do you think about this?

I will rework the patch with your suggestions as soon as provisions to switch
on/off the "quiet" option and resubmit it the next days.

Regards

  Guido

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

[Stefan Roese]

Hi Guido,

On Sunday 08 October 2006 15:49, Guido Classen wrote:
> >> +		if ( !opts->scrub && bbtest ) {
> >
> >No spaces after opening brace and before closing brace.
>
> Now I finally know what you meant. AFAIK the round ones are called
> parenthesis, not brace! Braces are the curly and square onse.

Thanks for clarifying this.

> >Now it compiles clean for my testplatform. Thanks.
> >
> >As it seems you have set the "quiet" option for all commands. Because of
> >this we don't see the skipping of bad blocks message for example. So
> > please disable the quiet option.
>
> You're right. I set the "quiet" option for all commands. I have done some
> testing again and get an impression that the output of progress messages on
> the serial console takes too much time. It seems that they slow down the
> operations especially on small page devices (four times more messages).
> Hence I think the best solution would be to make them switchable on command
> line, so they can be switched on for testing and off for normal operation.

Hmmm. Not sure here. Perhaps you could try to enhance the output so that it 
there printend that much output. And I also noticed that the output is not 
correct:

=> nand erase 0 1000000

NAND erase: device 0 offset 0x0, size 0x1000000
Erasing 128 Kibyte at fe0000 --  1% complete.
OK

The 1% is for the complete FLASH size, but I was erasing just a small part of 
it. I would really like to see 100%, even when erasing a small part.

So this is another reason to modify the output of the routines, at least of 
the erase routine.

> Maybe a "quiet" option at command line for the related commands (like
> "clean" option) will be the right way to do it. What do you think about
> this?
>
> I will rework the patch with your suggestions as soon as provisions to
> switch on/off the "quiet" option and resubmit it the next days.

Please see my comment above. I don't want the commands to become too complex 
with lots of options (they are allready complex enough). So my suggestion is 
to change the code that it doesn't slow down the operation anymore.

Best regards,
Stefan

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

[Guido Classen]

Hi Stefan,

now the next try...

> Hmmm. Not sure here. Perhaps you could try to enhance the output so that it
> there printend that much output.

The relation between time spent on NAND operation and progress output may
depend on much things like NAND type and size, CPU and bus speeds, console
type and speed. So I have changed the patch in a way that the output is only
print for whole percent steps.  This should reduce the output at reasonable
amount independent from this circumstances.

> And I also noticed that the output is not
> correct:
>
> => nand erase 0 1000000
>
> NAND erase: device 0 offset 0x0, size 0x1000000
> Erasing 128 Kibyte at fe0000 --  1% complete.
> OK
>
> The 1% is for the complete FLASH size, but I was erasing just a small part of
> it. I would really like to see 100%, even when erasing a small part.
>

This is fixed

> > Maybe a "quiet" option at command line for the related commands (like
> > "clean" option) will be the right way to do it. What do you think about
> > this?
> >
> > I will rework the patch with your suggestions as soon as provisions to
> > switch on/off the "quiet" option and resubmit it the next days.
>
> Please see my comment above. I don't want the commands to become too complex
> with lots of options (they are already complex enough). So my suggestion is
> to change the code that it doesn't slow down the operation anymore.

Now I consider an environment variable "quiet" if set for activating the quiet
options. This even permits the usage of the quiet option with macros. To my
mind this is a very convenient to apply the option to a sequence of NAND
operations.

Furthermore the spelling and grammar fixes from Phil are applied! Phil thank
you very much for that!

Regards

  Guido
-------------- next part --------------
A non-text attachment was scrubbed...
Name: nand-jffs2-revised-2.patch.gz
Type: application/x-gzip
Size: 9409 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20061009/fbfd7895/attachment.bin 

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

[Stefan Roese]

Hi Guido,

On Monday 09 October 2006 18:47, Guido Classen wrote:
> now the next try...

;-)

> > Hmmm. Not sure here. Perhaps you could try to enhance the output so that
> > it there printend that much output.
>
> The relation between time spent on NAND operation and progress output may
> depend on much things like NAND type and size, CPU and bus speeds, console
> type and speed. So I have changed the patch in a way that the output is
> only print for whole percent steps.  This should reduce the output at
> reasonable amount independent from this circumstances.

Looks much better to me. Thanks.

> > And I also noticed that the output is not
> > correct:
> >
> > => nand erase 0 1000000
> >
> > NAND erase: device 0 offset 0x0, size 0x1000000
> > Erasing 128 Kibyte at fe0000 --  1% complete.
> > OK
> >
> > The 1% is for the complete FLASH size, but I was erasing just a small
> > part of it. I would really like to see 100%, even when erasing a small
> > part.
>
> This is fixed

Good. Thanks.

> > > Maybe a "quiet" option at command line for the related commands (like
> > > "clean" option) will be the right way to do it. What do you think about
> > > this?
> > >
> > > I will rework the patch with your suggestions as soon as provisions to
> > > switch on/off the "quiet" option and resubmit it the next days.
> >
> > Please see my comment above. I don't want the commands to become too
> > complex with lots of options (they are already complex enough). So my
> > suggestion is to change the code that it doesn't slow down the operation
> > anymore.
>
> Now I consider an environment variable "quiet" if set for activating the
> quiet options. This even permits the usage of the quiet option with macros.
> To my mind this is a very convenient to apply the option to a sequence of
> NAND operations.

Good idea. So the "normal" operation with "quiet" not set will give the 
complete output including the "Skipping bad block..." lines.

> Furthermore the spelling and grammar fixes from Phil are applied! Phil
> thank you very much for that!

Thanks.

If Wolfgang has no further issues we will commit your patch in the next few 
days. Thanks again.

Best regards,
Stefan

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

[Ladislav Michl]

Hi Guido,

I'm very happy to see nicely working patch, perhaps I owe you couple of
beers for saving so much time. Thanks!

On Tue, Oct 10, 2006 at 12:14:00PM +0200, Stefan Roese wrote:
[snip]
> > Furthermore the spelling and grammar fixes from Phil are applied! Phil
> > thank you very much for that!

"shure" vs "sure" is still here :-) "sure" is correct. Patch line 180
contains changelog entry :-) That can be easily fixed later.

> Thanks.
> 
> If Wolfgang has no further issues we will commit your patch in the next few 
> days. Thanks again.

Please make sure to drop all my pending NAND related patches. They will
definitely not apply after this change. I will rebase relevant changes on
the top of Guido's patch.

Best regards,
	ladis

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

[Guido Classen]

Hi Ladis,
hi Stefan,

> I'm very happy to see nicely working patch, perhaps I owe you couple of
> beers for saving so much time. Thanks!

Thank you for reviewing and the rest of work on new NAND subsystem!

> "shure" vs "sure" is still here :-) "sure" is correct. Patch line 180
> contains changelog entry :-) That can be easily fixed later.
>

Oh.... I have overseen that - thank you. Fixed at at attached patch.
I have removed line 180. Here the extended CHANGELOG entry

CHANGELOG:
* Several improvements to the new NAND subsystem: JFFS2 related commands
 implemented in mtd-utils style, support for bad blocks, bad block testing
 commands, NAND lock commands. Implements missing commands "nand markbad" and
 "nand scrub"
 Patch by Guido Classen, 06 Oct 2006


I couldn't implement the "biterr" command. I was not sure what you meant with
it? Maybe wrong ECC in OOB? So this may be an isue for the next patch...


Regards

  Guido
-------------- next part --------------
A non-text attachment was scrubbed...
Name: nand-jffs2-revised-3.patch.gz
Type: application/x-gzip
Size: 9411 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20061010/77f38adb/attachment.bin 

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

[Stefan Roese]

Hi Guido,

On Tuesday 10 October 2006 19:03, Guido Classen wrote:
> > I'm very happy to see nicely working patch, perhaps I owe you couple of
> > beers for saving so much time. Thanks!
>
> Thank you for reviewing and the rest of work on new NAND subsystem!

You're welcome.

Patch applied with little changes. Thanks again.

Best regards,
Stefan

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

[Guido Classen]

Hi Stefan,

>
> Patch applied with little changes. Thanks again.
>

thank you for applying!

There are two issues I want to remark:


> --- a/lib_generic/crc32.c
> +++ b/lib_generic/crc32.c
> @@ -171,9 +171,8 @@ uLong ZEXPORT crc32(crc, buf, len)
> return crc ^ 0xffffffffL;
> }
> -#if (CONFIG_COMMANDS & CFG_CMD_JFFS2) \
> - || (CONFIG_COMMANDS & CFG_CMD_NAND) && !defined(CFG_NAND_LEGACY)
> -
> +#if ((CONFIG_COMMANDS & CFG_CMD_JFFS2) || (CONFIG_COMMANDS & CFG_CMD_NAND)) \
> + && !defined(CFG_NAND_LEGACY)

This means you can't use the JFFS2 filesystem with legacy NAND subsystem.
Was this meant so? I am not sure here since I don't use the JFFS2 filesystem
in u-boot. Maybe wrong set parenthesis?

I think the right code should look like this:
#if (CONFIG_COMMANDS & CFG_CMD_JFFS2) || ((CONFIG_COMMANDS & CFG_CMD_NAND) \
 && !defined(CFG_NAND_LEGACY))

Spelling fix, cmd_nand.c line 254
"shure" => "sure"
is open. Stefan can you fix this?

Regards
  Guido

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

[Wolfgang Denk]

Dear Guido,

in message <5c01f02f0610120148i546bb178tb2f1f1b011f9d356@mail.gmail.com> you wrote:
> 
> > -#if (CONFIG_COMMANDS & CFG_CMD_JFFS2) \
> > - || (CONFIG_COMMANDS & CFG_CMD_NAND) && !defined(CFG_NAND_LEGACY)
> > -
> > +#if ((CONFIG_COMMANDS & CFG_CMD_JFFS2) || (CONFIG_COMMANDS & CFG_CMD_NAND)) \
> > + && !defined(CFG_NAND_LEGACY)

You are right -  this modification was wrong.

> This means you can't use the JFFS2 filesystem with legacy NAND subsystem.
> Was this meant so? I am not sure here since I don't use the JFFS2 filesystem
> in u-boot. Maybe wrong set parenthesis?

Well spotted. Fixed.

> Spelling fix, cmd_nand.c line 254
> "shure" => "sure"
> is open. Stefan can you fix this?

Done.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"I find this a nice feature but it is not according to  the  documen-
tation. Or is it a BUG?"   "Let's call it an accidental feature. :-)"
                       - Larry Wall in <6909@jpl-devvax.JPL.NASA.GOV>

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

[Wolfgang Denk]

In message <200610071235.18760.sr@denx.de> Stefan Roese wrote:
> 
...
> Indentation is incorrect here.
...
> Again, please not 2 empty lines.
...
> Opening brace in "if-statement" line please.
...
> > +		if (!nand_unlock( nand, off, size) )
> 
> No space before the colsing brace here.

etc. etc.

I strongly recommend to read the README, section Coding Style,  which
explicitely  explains the Coding Style requirements and shows ways to
correctly reformat corrupted code.


Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"This is a test of the Emergency Broadcast System. If this had been an
actual emergency, do you really think we'd stick around to tell you?"