[U-Boot-Users] [PATCH] New NAND subsystem: mtd like commands support jffs2 and bad blocks
Stefan Roese
sr at denx.de
Sat Oct 7 12:35:18 CEST 2006
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 at 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
More information about the U-Boot
mailing list