Skip to contentSkip to conversation

bad block aware read/write support

PATCH New Kyungmin Park 2 participants 5 messages Nov 4, 2008 – Nov 6, 2008

lore
Share & tools
lore

Kyungmin Park

author
#0 lore raw

PATCH 1/1 bad block aware read/write support

Update OneNAND command to support bad block awareness Also change the OneNAND command styel like NAND

Signed-off-by Kyungmin Park <[email protected]>

---

common/cmd_onenand.c +301 -94
index 8d87b78..1eca9b0 100644
@@ -1,7 +1,7 @@
 /*
  *  U-Boot command for OneNAND support
  *
- *  Copyright (C) 2005-2007 Samsung Electronics
+ *  Copyright (C) 2005-2008 Samsung Electronics
  *  Kyungmin Park <[email protected]>
  *
  * This program is free software; you can redistribute it and/or modify

@@ -18,12 +18,245 @@
 
 #include <asm/io.h>
 
-extern struct mtd_info onenand_mtd;
-extern struct onenand_chip onenand_chip;
+static struct mtd_info *mtd = &onenand_mtd;
+
+static loff_t next_ofs;
+static loff_t skip_ofs;
+
+static int onenand_block_read(loff_t from, size_t len,
+				size_t *retlen, u_char *buf, int oob)
+{
+	struct onenand_chip *this = mtd->priv;
+	int blocks = (int) len >> this->erase_shift;
+	int blocksize = (1 << this->erase_shift);
+	loff_t ofs = from;
+	struct mtd_oob_ops ops = {
+		.retlen		= 0,
+	};
+	int ret, dump = 0;
+
+	if (buf == NULL) {
+		buf = (u_char *) CONFIG_SYS_LOAD_ADDR;
+		dump = 1;
+	}
+
+	if (oob)
+		ops.ooblen = blocksize;
+	else
+		ops.len = blocksize;
+	
+	while (blocks) {
+		ret = mtd->block_isbad(mtd, ofs);
+		if (ret) {
+			printk("Bad blocks %d at 0x%x\n", (unsigned int) ofs >> this->erase_shift, (unsigned int) ofs);
+			ofs += blocksize;
+			continue;
+		}
+
+		if (oob) 
+			ops.oobbuf = buf;
+		else
+			ops.datbuf = buf;
+
+		ops.retlen = 0;
+		ret = mtd->read_oob(mtd, ofs, &ops);
+		if (ret) {
+			printk("Read failed 0x%x, %d", (unsigned int) ofs, ret);
+			mtd->block_markbad(mtd, ofs);
+			ofs += blocksize;
+			continue;
+		}
+		ofs += blocksize;
+		buf += blocksize;
+		blocks--;
+		*retlen += ops.retlen;
+
+		if (dump) {
+			int i;
+			unsigned int *p = (unsigned int *) CONFIG_SYS_LOAD_ADDR;
+			printf("Dump offset 0x%x (%d)\n", (unsigned int) ofs, (int) ofs >> this->erase_shift);
+			for (i = 0; i < 32; i++) {
+				printf("0x%08x ", *p++);
+				if (((i + 1) & (4 - 1)) == 0)
+					printf("\n");
+			}
+			buf = (u_char *) CONFIG_SYS_LOAD_ADDR;
+		}
+	}
+
+	printf("Read from 0x%x to 0x%x (%d + %d blocks)\n", (unsigned int) from, (unsigned int) ofs, (int) len >> this->erase_shift, (int) ((ofs - from) - len) >> this->erase_shift);
+
+	return 0;
+}
+
+static int onenand_block_write(loff_t to, size_t len,
+		  size_t *retlen, const u_char * buf)
+{
+	struct onenand_chip *this = mtd->priv;
+	int blocks = len >> this->erase_shift;
+	int blocksize = (1 << this->erase_shift);
+	loff_t ofs;
+	size_t _retlen = 0;
+	int ret;
+
+	if (to == next_ofs) {
+		next_ofs = to + len;
+		to += skip_ofs;
+	} else {
+		next_ofs = to + len;
+		skip_ofs = 0;
+	}
+	ofs = to;
+
+	while (blocks) {
+		ret = mtd->block_isbad(mtd, ofs);
+		if (ret) {
+			printk("Bad blocks %d at 0x%x\n", (unsigned int) ofs >> this->erase_shift, (unsigned int) ofs);
+			skip_ofs += blocksize;
+			goto next;
+		}
+#ifdef USE_ERASEWRITE
+		if ((ofs & (blocksize - 1)) == 0) {
+			struct erase_info instr = {
+				.callback	= NULL,
+				.addr		= ofs,
+				.len		= blocksize,
+				.priv		= 0,
+			};
+			ret = mtd->erase(mtd, &instr);
+			if (ret) {
+				printk("Erase failed 0x%x, %d", (unsigned int) ofs, ret);
+				mtd->block_markbad(mtd, ofs);
+				skip_ofs += blocksize;
+				goto next;
+			}
+		}
+#endif
+		ret = mtd->write(mtd, ofs, blocksize, &_retlen, buf);
+		if (ret) {
+			printk("Write failed 0x%x, %d", (unsigned int) ofs, ret);
+			mtd->block_markbad(mtd, ofs);
+			skip_ofs += blocksize;
+			goto next;
+		}
+
+		buf += blocksize;
+		blocks--;
+		*retlen += _retlen;
+next:
+		ofs += blocksize;
+	}
+
+	printf("Write from 0x%x to 0x%x (%d + %d blocks)\n", (unsigned int) to, (unsigned int) ofs, (int) len >> this->erase_shift, (int) ((ofs - to) - len) >> this->erase_shift);
+
+	return 0;
+}
+
+static int onenand_block_erase(int start, int end, int force)
+{
+	struct onenand_chip *this = mtd->priv;
+	struct erase_info instr = {
+		.callback	= NULL,
+	};
+	loff_t ofs;
+	int block, ret;
+	int blocksize = 1 << this->erase_shift;
+
+	for (block = start; block <= end; block++) {
+		ofs = block << this->erase_shift;
+		ret = mtd->block_isbad(mtd, ofs);
+		if (ret && !force) {
+			printf("Skip erase bad block %d at 0x%x\n", block, (unsigned int) block << this->erase_shift);
+			continue;
+		}
+
+		instr.addr = ofs;
+		instr.len = blocksize;
+		instr.priv = force;
+		ret = mtd->erase(mtd, &instr);
+		if (ret) {
+			printf("erase failed %d\n", block);
+			mtd->block_markbad(mtd, instr.addr);
+			continue;
+		}
+	}
+}
+
+static int onenand_badblocks(unsigned long start_address, unsigned long end_address)
+{
+	struct onenand_chip *this = mtd->priv;
+	struct erase_info instr = {
+		.callback	= NULL,
+		.priv		= 0,
+	};
+
+	int blocks;
+	loff_t ofs = 0;
+	int blocksize = 1 << this->erase_shift;
+	int start_block, end_block;
+	size_t retlen;
+	u_char *buf = (u_char *) CONFIG_SYS_LOAD_ADDR;
+	/* block size '512KiB' is enough */
+	u_char *verify_buf = (u_char *) CONFIG_SYS_LOAD_ADDR + 0x80000;
+	int ret;
+
+	start_block = start_address >> this->erase_shift;
+	end_block = end_address >> this->erase_shift;
+
+	/* Protect boot-loader from badblock testing */
+	if (start_block < 2) 
+		start_block = 2;
+
+	if (end_block > (mtd->size >> this->erase_shift))
+		end_block = mtd->size >> this->erase_shift;
+
+	blocks = start_block;
+	while (blocks < end_block) {
+		ret = mtd->block_isbad(mtd, ofs);
+		if (ret) {
+			ofs += blocksize;
+			continue;
+		}
+		instr.addr = ofs;
+		instr.len = blocksize;
+		ret = mtd->erase(mtd, &instr);
+		if (ret) {
+			printk("Erase failed 0x%x, %d", (unsigned int) ofs, ret);
+			mtd->block_markbad(mtd, ofs);
+			goto next;
+		}
+		ret = mtd->write(mtd, ofs, blocksize, &retlen, buf);
+		if (ret) {
+			printk("Write failed 0x%x, %d", (unsigned int) ofs, ret);
+			mtd->block_markbad(mtd, ofs);
+			goto next;
+		}
+		ret = mtd->read(mtd, ofs, blocksize, &retlen, verify_buf);
+		if (ret) {
+			printk("Read failed 0x%x, %d", (unsigned int) ofs, ret);
+			mtd->block_markbad(mtd, ofs);
+			goto next;
+		}
+		if (memcmp(buf, verify_buf, blocksize))
+			printk("Read/Write test failed at 0x%x", (unsigned int) ofs);
+
+		printf("\rTest block at 0x%x", (unsigned int) ofs);
+		
+next:
+		ofs += blocksize;
+		blocks++;
+	}
+	printf("...Done\n");
+
+	return 0;
+}
 
 int do_onenand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
 {
-	int ret = 0;
+	struct onenand_chip *this = mtd->priv;
+	int blocksize = (1 << this->erase_shift);
+	ulong addr, ofs;
+	size_t len, retlen = 0;
 
 	switch (argc) {
 	case 0:

@@ -32,129 +265,100 @@ int do_onenand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
 		return 1;
 
 	case 2:
-		if (strncmp(argv[1], "open", 4) == 0) {
-			onenand_init();
-			return 0;
-		}
-		printf("%s\n", onenand_mtd.name);
+		printf("%s\n", mtd->name);
 		return 0;
 
 	default:
 		/* At least 4 args */
 		if (strncmp(argv[1], "erase", 5) == 0) {
-			struct erase_info instr = {
-				.callback	= NULL,
-			};
-			ulong start, end;
-			ulong block;
-			char *endtail;
+			ulong start, end, len;
+			char *endtail = NULL;
+			int force = 0;
+
+			if (strncmp(argv[1], "erasef", 6) == 0)
+				force = 1;
 
 			if (strncmp(argv[2], "block", 5) == 0) {
 				start = simple_strtoul(argv[3], NULL, 10);
 				endtail = strchr(argv[3], '-');
-				end = simple_strtoul(endtail + 1, NULL, 10);
+				if (endtail)
+					end = simple_strtoul(endtail + 1, NULL, 10);
+				else
+					end = start;
 			} else {
-				start = simple_strtoul(argv[2], NULL, 10);
-				end = simple_strtoul(argv[3], NULL, 10);
+				start = simple_strtoul(argv[2], NULL, 16);
+				len = simple_strtoul(argv[3], NULL, 16);
 
-				start >>= onenand_chip.erase_shift;
-				end >>= onenand_chip.erase_shift;
+				start >>= this->erase_shift;
+				len >>= this->erase_shift;
 				/* Don't include the end block */
-				end--;
+				end = start + len - 1;
 			}
 
 			if (!end || end < 0)
 				end = start;
 
-			printf("Erase block from %lu to %lu\n", start, end);
+			/* Don't exceed the whole block size */
+			if (end >= mtd->size >> this->erase_shift)
+				end = (mtd->size >> this->erase_shift) - 1;
 
-			for (block = start; block <= end; block++) {
-				instr.addr = block << onenand_chip.erase_shift;
-				instr.len = 1 << onenand_chip.erase_shift;
-				ret = onenand_erase(&onenand_mtd, &instr);
-				if (ret) {
-					printf("erase failed %lu\n", block);
-					break;
-				}
-			}
+			printf("Erase block from %lu to %lu\n", start, end);
 
+			onenand_block_erase(start, end, force);
 			return 0;
 		}
 
 		if (strncmp(argv[1], "read", 4) == 0) {
-			ulong addr = simple_strtoul(argv[2], NULL, 16);
-			ulong ofs = simple_strtoul(argv[3], NULL, 16);
-			size_t len = simple_strtoul(argv[4], NULL, 16);
 			int oob = strncmp(argv[1], "read.oob", 8) ? 0 : 1;
-			struct mtd_oob_ops ops;
-
-			ops.mode = MTD_OOB_PLACE;
 
-			if (oob) {
-				ops.len = 0;
-				ops.datbuf = NULL;
-				ops.ooblen = len;
-				ops.oobbuf = (u_char *) addr;
-			} else {
-				ops.len = len;
-				ops.datbuf = (u_char *) addr;
-				ops.ooblen = 0;
-				ops.oobbuf = NULL;
-			}
-			ops.retlen = ops.oobretlen = 0;
+			addr = simple_strtoul(argv[2], NULL, 16);
+			ofs = simple_strtoul(argv[3], NULL, 16);
+			len = simple_strtoul(argv[4], NULL, 16);
 
-			onenand_mtd.read_oob(&onenand_mtd, ofs, &ops);
-			printf("Done\n");
+			onenand_block_read(ofs, len, &retlen, (u_char *) addr, oob);
 
 			return 0;
 		}
+		if (strncmp(argv[1], "badb", 4) == 0) {
+			/* Start address */
+			addr = simple_strtoul(argv[2], NULL, 16);
+			/* End address */
+			ofs = simple_strtoul(argv[3], NULL, 16);
 
-		if (strncmp(argv[1], "write", 5) == 0) {
-			ulong addr = simple_strtoul(argv[2], NULL, 16);
-			ulong ofs = simple_strtoul(argv[3], NULL, 16);
-			size_t len = simple_strtoul(argv[4], NULL, 16);
-			size_t retlen = 0;
-
-			onenand_write(&onenand_mtd, ofs, len, &retlen,
-				      (u_char *) addr);
-			printf("Done\n");
-
-			return 0;
+			return onenand_badblocks(addr, ofs);
 		}
 
-		if (strncmp(argv[1], "block", 5) == 0) {
-			ulong addr = simple_strtoul(argv[2], NULL, 16);
-			ulong block = simple_strtoul(argv[3], NULL, 10);
-			ulong page = simple_strtoul(argv[4], NULL, 10);
-			size_t len = simple_strtol(argv[5], NULL, 10);
-			ulong ofs;
-			int oob = strncmp(argv[1], "block.oob", 9) ? 0 : 1;
-			struct mtd_oob_ops ops;
-
-			ops.mode = MTD_OOB_PLACE;
+		if (strncmp(argv[1], "write", 5) == 0) {
+			addr = simple_strtoul(argv[2], NULL, 16);
+			ofs = simple_strtoul(argv[3], NULL, 16);
+			len = simple_strtoul(argv[4], NULL, 16);
 
+			return onenand_block_write(ofs, len, &retlen, (u_char *) addr);
+		}
 
-			ofs = block << onenand_chip.erase_shift;
-			if (page)
-				ofs += page << onenand_chip.page_shift;
+		if (strcmp(argv[1], "markbad") == 0) {
+			addr = (ulong)simple_strtoul(argv[2], NULL, 16);
 
-			if (!len) {
-				if (oob)
-					ops.ooblen = 64;
-				else
-					ops.len = 512;
-			}
-
-			if (oob) {
-				ops.datbuf = NULL;
-				ops.oobbuf = (u_char *) addr;
+			int ret = mtd->block_markbad(mtd, addr);
+			if (ret == 0) {
+				printf("block 0x%08lx successfully marked as bad\n",
+						(ulong) addr);
+				return 0;
 			} else {
-				ops.datbuf = (u_char *) addr;
-				ops.oobbuf = NULL;
+				printf("block 0x%08lx NOT marked as bad! ERROR %d\n",
+						(ulong) addr, ret);
 			}
-			ops.retlen = ops.oobretlen = 0;
+			return 1;
+		}
+
+		if (strncmp(argv[1], "dump", 4) == 0) {
+			ofs = simple_strtoul(argv[2], NULL, 16);
+			len = blocksize;
+
+			if (argc == 4)
+				len = simple_strtoul(argv[3], NULL, 16);
 
-			onenand_read_oob(&onenand_mtd, ofs, &ops);
+			onenand_block_read(ofs, len, &retlen, NULL, 0);
 			return 0;
 		}
 

@@ -168,9 +372,12 @@ U_BOOT_CMD(
 	onenand,	6,	1,	do_onenand,
 	"onenand - OneNAND sub-system\n",
 	"info   - show available OneNAND devices\n"
-	"onenand read[.oob] addr ofs len - read data at ofs with len to addr\n"
-	"onenand write addr ofs len - write data at ofs with len from addr\n"
-	"onenand erase saddr eaddr - erase block start addr to end addr\n"
-	"onenand block[.oob] addr block [page] [len] - "
-		"read data with (block [, page]) to addr"
+	"badb[locks] start_addr end_addr - Test & Mark bad blocks\n"
+	"onenand read[.oob] addr ofs len"
+		" - read data at ofs with len to addr\n"
+	"onenand write[.oob] addr ofs len"
+		" - write data at ofs with len from addr\n"
+	"onenand erase[force] [block] ofs len"
+		" - erase [block] at ofs with len\n"
+	"onenand markbad off - mark bad block at offset (UNSAFE)\n"
 );
#1 lore raw

PATCH 1/1 bad block aware read/write support

Hi Kyungmin,

Quoted 2 lines · Kyungmin Park

Update OneNAND command to support bad block awareness Also change the OneNAND command styel like NAND

I'm starting with OneNAND support for a MIPS platform right now and wasn't ware that the onenand commands were not bad block aware. So thanks for this patch. But I have some comments.

Quoted 16 lines
Signed-off-by Kyungmin Park <[email protected]>

---

common/cmd_onenand.c +1 -1
index 8d87b78..1eca9b0 100644
@@ -1,7 +1,7 @@
 /*
  *  U-Boot command for OneNAND support
  *
- *  Copyright (C) 2005-2007 Samsung Electronics
+ *  Copyright (C) 2005-2008 Samsung Electronics
  *  Kyungmin Park <[email protected]>
  *
  * This program is free software; you can redistribute it and/or modify

common/cmd_onenand.c line 21

Quoted 6 lines
common/cmd_onenand.c +1 -2
@@ -18,12 +18,245 @@
 
 #include <asm/io.h>
 
-extern struct mtd_info onenand_mtd;
-extern struct onenand_chip onenand_chip;
+static struct mtd_info *mtd = &onenand_mtd;

This does not work on platforms where the relocation is "broken", like on my MIPS platform (and PPC as well). We need to assign this value at runtime to work around this problem.

Quoted 10 lines
common/cmd_onenand.c +10 -0
+
+static loff_t next_ofs;
+static loff_t skip_ofs;
+
+static int onenand_block_read(loff_t from, size_t len,
+				size_t *retlen, u_char *buf, int oob)
+{
+	struct onenand_chip *this = mtd->priv;
+	int blocks = (int) len >> this->erase_shift;
+	int blocksize = (1 << this->erase_shift);

common/cmd_onenand.c line 41

Quoted 10 lines
common/cmd_onenand.c +10 -0
+	loff_t ofs = from;
+	struct mtd_oob_ops ops = {
+		.retlen		= 0,
+	};
+	int ret, dump = 0;
+
+	if (buf == NULL) {
+		buf = (u_char *) CONFIG_SYS_LOAD_ADDR;
+		dump = 1;
+	}

I don't think that it is a good idea to default "buf" to CONFIG_SYS_LOAD_ADDR. Some board might really want to load data to address 0. We should not restrict this here. I suggest to remove this check and assignment completely.

common/cmd_onenand.c

Quoted 11 lines
common/cmd_onenand.c +10 -0
+
+	if (oob)
+		ops.ooblen = blocksize;
+	else
+		ops.len = blocksize;
+
+	while (blocks) {
+		ret = mtd->block_isbad(mtd, ofs);
+		if (ret) {
+			printk("Bad blocks %d at 0x%x\n", (unsigned int) ofs >>
 this->erase_shift, (unsigned int) ofs);

There are multiple much too long lines here.

Quoted 65 lines
common/cmd_onenand.c +35 -0
+			ofs += blocksize; 
+			continue;
+		}
+
+		if (oob)
+			ops.oobbuf = buf;
+		else
+			ops.datbuf = buf;
+
+		ops.retlen = 0;
+		ret = mtd->read_oob(mtd, ofs, &ops);
+		if (ret) {
+			printk("Read failed 0x%x, %d", (unsigned int) ofs, ret);
+			mtd->block_markbad(mtd, ofs);
+			ofs += blocksize;
+			continue;
+		}
+		ofs += blocksize;
+		buf += blocksize;
+		blocks--;
+		*retlen += ops.retlen;
+
+		if (dump) {
+			int i;
+			unsigned int *p = (unsigned int *) CONFIG_SYS_LOAD_ADDR;
+			printf("Dump offset 0x%x (%d)\n", (unsigned int) ofs, (int) ofs >>
 this->erase_shift); +			for (i = 0; i < 32; i++) {
+				printf("0x%08x ", *p++);
+				if (((i + 1) & (4 - 1)) == 0)
+					printf("\n");
+			}
+			buf = (u_char *) CONFIG_SYS_LOAD_ADDR;
+		}
+	}
+
+	printf("Read from 0x%x to 0x%x (%d + %d blocks)\n", (unsigned int) from,

(unsigned int) ofs, (int) len >> this->erase_shift, (int) ((ofs - from) - len) >> this->erase_shift); +

common/cmd_onenand.c +26 -0
+	return 0;
+}
+
+static int onenand_block_write(loff_t to, size_t len,
+		  size_t *retlen, const u_char * buf)
+{
+	struct onenand_chip *this = mtd->priv;
+	int blocks = len >> this->erase_shift;
+	int blocksize = (1 << this->erase_shift);
+	loff_t ofs;
+	size_t _retlen = 0;
+	int ret;
+
+	if (to == next_ofs) {
+		next_ofs = to + len;
+		to += skip_ofs;
+	} else {
+		next_ofs = to + len;
+		skip_ofs = 0;
+	}
+	ofs = to;
+
+	while (blocks) {
+		ret = mtd->block_isbad(mtd, ofs);
+		if (ret) {
+			printk("Bad blocks %d at 0x%x\n", (unsigned int) ofs >>

this->erase_shift, (unsigned int) ofs); + skip_ofs += blocksize;

common/cmd_onenand.c line 118

Quoted 3 lines
common/cmd_onenand.c +3 -0
+			goto next;
+		}
+#ifdef USE_ERASEWRITE

Hmmm. So you have an implicit erase feature if this USE_ERASEWRITE is defined. I'm don't like this. Is this really needed? The user should always be able to first erase the blocks before writing to them. I would prefer to remove this "feature".

Quoted 73 lines
common/cmd_onenand.c +32 -0
+		if ((ofs & (blocksize - 1)) == 0) {
+			struct erase_info instr = {
+				.callback	= NULL,
+				.addr		= ofs,
+				.len		= blocksize,
+				.priv		= 0,
+			};
+			ret = mtd->erase(mtd, &instr);
+			if (ret) {
+				printk("Erase failed 0x%x, %d", (unsigned int) ofs, ret);
+				mtd->block_markbad(mtd, ofs);
+				skip_ofs += blocksize;
+				goto next;
+			}
+		}
+#endif
+		ret = mtd->write(mtd, ofs, blocksize, &_retlen, buf);
+		if (ret) {
+			printk("Write failed 0x%x, %d", (unsigned int) ofs, ret);
+			mtd->block_markbad(mtd, ofs);
+			skip_ofs += blocksize;
+			goto next;
+		}
+
+		buf += blocksize;
+		blocks--;
+		*retlen += _retlen;
+next:
+		ofs += blocksize;
+	}
+
+	printf("Write from 0x%x to 0x%x (%d + %d blocks)\n", (unsigned int) to,

(unsigned int) ofs, (int) len >> this->erase_shift, (int) ((ofs - to) - len) >> this->erase_shift); +

common/cmd_onenand.c +18 -0
+	return 0;
+}
+
+static int onenand_block_erase(int start, int end, int force)
+{
+	struct onenand_chip *this = mtd->priv;
+	struct erase_info instr = {
+		.callback	= NULL,
+	};
+	loff_t ofs;
+	int block, ret;
+	int blocksize = 1 << this->erase_shift;
+
+	for (block = start; block <= end; block++) {
+		ofs = block << this->erase_shift;
+		ret = mtd->block_isbad(mtd, ofs);
+		if (ret && !force) {
+			printf("Skip erase bad block %d at 0x%x\n", block, (unsigned int) block

<< this->erase_shift); + continue;

common/cmd_onenand.c +15 -0
+		}
+
+		instr.addr = ofs;
+		instr.len = blocksize;
+		instr.priv = force;
+		ret = mtd->erase(mtd, &instr);
+		if (ret) {
+			printf("erase failed %d\n", block);
+			mtd->block_markbad(mtd, instr.addr);
+			continue;
+		}
+	}
+}
+
+static int onenand_badblocks(unsigned long start_address, unsigned long

end_address) +{

common/cmd_onenand.c +4 -0
+	struct onenand_chip *this = mtd->priv;
+	struct erase_info instr = {
+		.callback	= NULL,
+		.priv		= 0,

common/cmd_onenand.c line 200

Quoted 10 lines
common/cmd_onenand.c +10 -0
+	};
+
+	int blocks;
+	loff_t ofs = 0;
+	int blocksize = 1 << this->erase_shift;
+	int start_block, end_block;
+	size_t retlen;
+	u_char *buf = (u_char *) CONFIG_SYS_LOAD_ADDR;
+	/* block size '512KiB' is enough */
+	u_char *verify_buf = (u_char *) CONFIG_SYS_LOAD_ADDR + 0x80000;

So you need a buffer with twice the block-size? I don't like putting this buffer right at CONFIG_SYS_LOAD_ADDR. If such a feature is really needed (check for bad blocks by writing, reading and verifying) then we should malloc those areas. Do you think such a feature is needed? For the current NAND commands we "only" print the blocks here that are already marked as bad.

Quoted 250 lines
common/cmd_onenand.c +112 -86
+	int ret;
+
+	start_block = start_address >> this->erase_shift;
+	end_block = end_address >> this->erase_shift;
+
+	/* Protect boot-loader from badblock testing */
+	if (start_block < 2)
+		start_block = 2;
+
+	if (end_block > (mtd->size >> this->erase_shift))
+		end_block = mtd->size >> this->erase_shift;
+
+	blocks = start_block;
+	while (blocks < end_block) {
+		ret = mtd->block_isbad(mtd, ofs);
+		if (ret) {
+			ofs += blocksize;
+			continue;
+		}
+		instr.addr = ofs;
+		instr.len = blocksize;
+		ret = mtd->erase(mtd, &instr);
+		if (ret) {
+			printk("Erase failed 0x%x, %d", (unsigned int) ofs, ret);
+			mtd->block_markbad(mtd, ofs);
+			goto next;
+		}
+		ret = mtd->write(mtd, ofs, blocksize, &retlen, buf);
+		if (ret) {
+			printk("Write failed 0x%x, %d", (unsigned int) ofs, ret);
+			mtd->block_markbad(mtd, ofs);
+			goto next;
+		}
+		ret = mtd->read(mtd, ofs, blocksize, &retlen, verify_buf);
+		if (ret) {
+			printk("Read failed 0x%x, %d", (unsigned int) ofs, ret);
+			mtd->block_markbad(mtd, ofs);
+			goto next;
+		}
+		if (memcmp(buf, verify_buf, blocksize))
+			printk("Read/Write test failed at 0x%x", (unsigned int) ofs);
+
+		printf("\rTest block at 0x%x", (unsigned int) ofs);
+
+next:
+		ofs += blocksize;
+		blocks++;
+	}
+	printf("...Done\n");
+
+	return 0;
+}
 
 int do_onenand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
 {
-	int ret = 0;
+	struct onenand_chip *this = mtd->priv;
+	int blocksize = (1 << this->erase_shift);
+	ulong addr, ofs;
+	size_t len, retlen = 0;
 
 	switch (argc) {
 	case 0:

@@ -32,129 +265,100 @@ int do_onenand(cmd_tbl_t * cmdtp, int flag, int
 argc, char *argv[]) return 1;
 
 	case 2:
-		if (strncmp(argv[1], "open", 4) == 0) {
-			onenand_init();
-			return 0;
-		}
-		printf("%s\n", onenand_mtd.name);
+		printf("%s\n", mtd->name);
 		return 0;
 
 	default:
 		/* At least 4 args */
 		if (strncmp(argv[1], "erase", 5) == 0) {
-			struct erase_info instr = {
-				.callback	= NULL,
-			};
-			ulong start, end;
-			ulong block;
-			char *endtail;
+			ulong start, end, len;
+			char *endtail = NULL;
+			int force = 0;
+
+			if (strncmp(argv[1], "erasef", 6) == 0)
+				force = 1;
 
 			if (strncmp(argv[2], "block", 5) == 0) {
 				start = simple_strtoul(argv[3], NULL, 10);
 				endtail = strchr(argv[3], '-');
-				end = simple_strtoul(endtail + 1, NULL, 10);
+				if (endtail)
+					end = simple_strtoul(endtail + 1, NULL, 10);
+				else
+					end = start;
 			} else {
-				start = simple_strtoul(argv[2], NULL, 10);
-				end = simple_strtoul(argv[3], NULL, 10);
+				start = simple_strtoul(argv[2], NULL, 16);
+				len = simple_strtoul(argv[3], NULL, 16);
 
-				start >>= onenand_chip.erase_shift;
-				end >>= onenand_chip.erase_shift;
+				start >>= this->erase_shift;
+				len >>= this->erase_shift;
 				/* Don't include the end block */
-				end--;
+				end = start + len - 1;
 			}
 
 			if (!end || end < 0)
 				end = start;
 
-			printf("Erase block from %lu to %lu\n", start, end);
+			/* Don't exceed the whole block size */
+			if (end >= mtd->size >> this->erase_shift)
+				end = (mtd->size >> this->erase_shift) - 1;
 
-			for (block = start; block <= end; block++) {
-				instr.addr = block << onenand_chip.erase_shift;
-				instr.len = 1 << onenand_chip.erase_shift;
-				ret = onenand_erase(&onenand_mtd, &instr);
-				if (ret) {
-					printf("erase failed %lu\n", block);
-					break;
-				}
-			}
+			printf("Erase block from %lu to %lu\n", start, end);
 
+			onenand_block_erase(start, end, force);
 			return 0;
 		}
 
 		if (strncmp(argv[1], "read", 4) == 0) {
-			ulong addr = simple_strtoul(argv[2], NULL, 16);
-			ulong ofs = simple_strtoul(argv[3], NULL, 16);
-			size_t len = simple_strtoul(argv[4], NULL, 16);
 			int oob = strncmp(argv[1], "read.oob", 8) ? 0 : 1;
-			struct mtd_oob_ops ops;
-
-			ops.mode = MTD_OOB_PLACE;
 
-			if (oob) {
-				ops.len = 0;
-				ops.datbuf = NULL;
-				ops.ooblen = len;
-				ops.oobbuf = (u_char *) addr;
-			} else {
-				ops.len = len;
-				ops.datbuf = (u_char *) addr;
-				ops.ooblen = 0;
-				ops.oobbuf = NULL;
-			}
-			ops.retlen = ops.oobretlen = 0;
+			addr = simple_strtoul(argv[2], NULL, 16);
+			ofs = simple_strtoul(argv[3], NULL, 16);
+			len = simple_strtoul(argv[4], NULL, 16);
 
-			onenand_mtd.read_oob(&onenand_mtd, ofs, &ops);
-			printf("Done\n");
+			onenand_block_read(ofs, len, &retlen, (u_char *) addr, oob);
 
 			return 0;
 		}
+		if (strncmp(argv[1], "badb", 4) == 0) {
+			/* Start address */
+			addr = simple_strtoul(argv[2], NULL, 16);
+			/* End address */
+			ofs = simple_strtoul(argv[3], NULL, 16);
 
-		if (strncmp(argv[1], "write", 5) == 0) {
-			ulong addr = simple_strtoul(argv[2], NULL, 16);
-			ulong ofs = simple_strtoul(argv[3], NULL, 16);
-			size_t len = simple_strtoul(argv[4], NULL, 16);
-			size_t retlen = 0;
-
-			onenand_write(&onenand_mtd, ofs, len, &retlen,
-				      (u_char *) addr);
-			printf("Done\n");
-
-			return 0;
+			return onenand_badblocks(addr, ofs);
 		}
 
-		if (strncmp(argv[1], "block", 5) == 0) {
-			ulong addr = simple_strtoul(argv[2], NULL, 16);
-			ulong block = simple_strtoul(argv[3], NULL, 10);
-			ulong page = simple_strtoul(argv[4], NULL, 10);
-			size_t len = simple_strtol(argv[5], NULL, 10);
-			ulong ofs;
-			int oob = strncmp(argv[1], "block.oob", 9) ? 0 : 1;
-			struct mtd_oob_ops ops;
-
-			ops.mode = MTD_OOB_PLACE;
+		if (strncmp(argv[1], "write", 5) == 0) {
+			addr = simple_strtoul(argv[2], NULL, 16);
+			ofs = simple_strtoul(argv[3], NULL, 16);
+			len = simple_strtoul(argv[4], NULL, 16);
 
+			return onenand_block_write(ofs, len, &retlen, (u_char *) addr);
+		}
 
-			ofs = block << onenand_chip.erase_shift;
-			if (page)
-				ofs += page << onenand_chip.page_shift;
+		if (strcmp(argv[1], "markbad") == 0) {
+			addr = (ulong)simple_strtoul(argv[2], NULL, 16);
 
-			if (!len) {
-				if (oob)
-					ops.ooblen = 64;
-				else
-					ops.len = 512;
-			}
-
-			if (oob) {
-				ops.datbuf = NULL;
-				ops.oobbuf = (u_char *) addr;
+			int ret = mtd->block_markbad(mtd, addr);
+			if (ret == 0) {
+				printf("block 0x%08lx successfully marked as bad\n",
+						(ulong) addr);
+				return 0;
 			} else {
-				ops.datbuf = (u_char *) addr;
-				ops.oobbuf = NULL;
+				printf("block 0x%08lx NOT marked as bad! ERROR %d\n",
+						(ulong) addr, ret);
 			}
-			ops.retlen = ops.oobretlen = 0;
+			return 1;
+		}
+
+		if (strncmp(argv[1], "dump", 4) == 0) {
+			ofs = simple_strtoul(argv[2], NULL, 16);
+			len = blocksize;
+
+			if (argc == 4)
+				len = simple_strtoul(argv[3], NULL, 16);
 
-			onenand_read_oob(&onenand_mtd, ofs, &ops);
+			onenand_block_read(ofs, len, &retlen, NULL, 0);
 			return 0;
 		}

common/cmd_onenand.c U_BOOT_CMD( · line 375

Quoted 9 lines
common/cmd_onenand.c +1 -5
@@ -168,9 +372,12 @@ U_BOOT_CMD(
 	onenand,	6,	1,	do_onenand,
 	"onenand - OneNAND sub-system\n",
 	"info   - show available OneNAND devices\n"
-	"onenand read[.oob] addr ofs len - read data at ofs with len to addr\n"
-	"onenand write addr ofs len - write data at ofs with len from addr\n"
-	"onenand erase saddr eaddr - erase block start addr to end addr\n"
-	"onenand block[.oob] addr block [page] [len] - "
-		"read data with (block [, page]) to addr"
+	"badb[locks] start_addr end_addr - Test & Mark bad blocks\n"

Again, do we really need a feature to test if a block is still ok? For NAND we "only" check the bad block marker here. If we really need this, then I suggest that we add a new command for it:

onenand test addr size

and use analog to the NAND implementation this for bad block status:

onenand bad

What do you think?

common/cmd_onenand.c U_BOOT_CMD( · line 377

Quoted 2 lines
common/cmd_onenand.c +2 -0
+	"onenand read[.oob] addr ofs len"
+		" - read data at ofs with len to addr\n"

I prefer to always use "off" instead of "ofs" here. This is how it is done in the NAND implementation.

common/cmd_onenand.c U_BOOT_CMD( · line 383

Quoted 6 lines
common/cmd_onenand.c +5 -0
+	"onenand write[.oob] addr ofs len"
+		" - write data at ofs with len from addr\n"
+	"onenand erase[force] [block] ofs len"
+		" - erase [block] at ofs with len\n"
+	"onenand markbad off - mark bad block at offset (UNSAFE)\n"
 );

I'm currently working on a version of this "bad block aware" OneNAND command support which resembles the NAND command style even more. I hope to have something ready till tomorrow that I can send to the list for review. I would really like to see some comments from you on this since you are much more experienced with the OneNAND stuff.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office at denx.de
=====================================================================

#2 lore raw

PATCH 1/1 bad block aware read/write support

Hi Kyungmin,

Quoted 6 lines · Stefan Roese
Quoted 2 lines

Update OneNAND command to support bad block awareness Also change the OneNAND command styel like NAND

I'm starting with OneNAND support for a MIPS platform right now and wasn't ware that the onenand commands were not bad block aware. So thanks for this patch. But I have some comments.

Some further comment below.

<snip>

Quoted 4 lines

common/cmd_onenand.c line 65

Quoted 4 lines
common/cmd_onenand.c +4 -0
+		ret = mtd->read_oob(mtd, ofs, &ops);
+		if (ret) {
+			printk("Read failed 0x%x, %d", (unsigned int) ofs, ret);
+			mtd->block_markbad(mtd, ofs);

You are marking all blocks as bad whenever a failure occurs. I'm not sure if we really should do it this way. Failures could have other reasons as well. I'm inclined to remove this marking in my patch version.

Any comments?

<snip>

Quoted 5 lines

I'm currently working on a version of this "bad block aware" OneNAND command support which resembles the NAND command style even more. I hope to have something ready till tomorrow that I can send to the list for review. I would really like to see some comments from you on this since you are much more experienced with the OneNAND stuff.

Do you already have some comments to the patch version I posted yesterday?

Thanks.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office at denx.de
=====================================================================

author
#3 lore raw

PATCH 1/1 bad block aware read/write support

Hi,

Quoted 19 lines · Stefan Roese

Hi Kyungmin,

Quoted 6 lines · Stefan Roese
Quoted 2 lines

Update OneNAND command to support bad block awareness Also change the OneNAND command styel like NAND

I'm starting with OneNAND support for a MIPS platform right now and wasn't ware that the onenand commands were not bad block aware. So thanks for this patch. But I have some comments.

Some further comment below.

<snip>

Quoted 4 lines

common/cmd_onenand.c line 65

Quoted 4 lines
common/cmd_onenand.c +4 -0
+           ret = mtd->read_oob(mtd, ofs, &ops);
+           if (ret) {
+                   printk("Read failed 0x%x, %d", (unsigned int) ofs, ret);
+                   mtd->block_markbad(mtd, ofs);
Quoted 5 lines

You are marking all blocks as bad whenever a failure occurs. I'm not sure if we really should do it this way. Failures could have other reasons as well. I'm inclined to remove this marking in my patch version.

Any comments?

Right, current implementation make a bad on all errors, but actually it need to try again in case read/write error. It will patch it later.

Quoted 10 lines

<snip>

Quoted 5 lines

I'm currently working on a version of this "bad block aware" OneNAND command support which resembles the NAND command style even more. I hope to have something ready till tomorrow that I can send to the list for review. I would really like to see some comments from you on this since you are much more experienced with the OneNAND stuff.

Do you already have some comments to the patch version I posted yesterday?

Looks good to me. No problem to commit your version.

Acked-by Kyungmin Park <[email protected]>

Thank you,
Kyungmin Park

#4 lore raw

PATCH 1/1 bad block aware read/write support

Quoted 5 lines · Kyungmin Park
Quoted 5 lines
Quoted 4 lines
Quoted 4 lines
common/cmd_onenand.c +3 -0
+           ret = mtd->read_oob(mtd, ofs, &ops);
+           if (ret) {
+                   printk("Read failed 0x%x, %d", (unsigned int) ofs,

ret); + mtd->block_markbad(mtd, ofs);

Quoted 8 lines
Quoted 5 lines

You are marking all blocks as bad whenever a failure occurs. I'm not sure if we really should do it this way. Failures could have other reasons as well. I'm inclined to remove this marking in my patch version.

Any comments?

Right, current implementation make a bad on all errors, but actually it need to try again in case read/write error. It will patch it later.

OK, I'll remove this bad block marking for now.

Quoted 8 lines
Quoted 8 lines

<snip>

Quoted 5 lines

I'm currently working on a version of this "bad block aware" OneNAND command support which resembles the NAND command style even more. I hope to have something ready till tomorrow that I can send to the list for review. I would really like to see some comments from you on this since you are much more experienced with the OneNAND stuff.

Quoted 6 lines
Quoted 2 lines

Do you already have some comments to the patch version I posted yesterday?

Looks good to me. No problem to commit your version.

Acked-by Kyungmin Park <[email protected]>

Thanks. I'll send an updated version today or tomorrow.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office at denx.de
=====================================================================