Re: [U-Boot] [PATCH] [OneNAND] bad block aware read/write support

Hi Kyungmin,

On Tuesday 04 November 2008, Kyungmin Park wrote:

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.

Signed-off-by: Kyungmin Park kyungmin.park@samsung.com

diff --git a/common/cmd_onenand.c b/common/cmd_onenand.c index 8d87b78..1eca9b0 100644 --- a/common/cmd_onenand.c +++ b/common/cmd_onenand.c @@ -1,7 +1,7 @@ /*

• U-Boot command for OneNAND support

• • Copyright (C) 2005-2007 Samsung Electronics

• • Copyright (C) 2005-2008 Samsung Electronics
  • Kyungmin Park kyungmin.park@samsung.com
  • 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;

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.

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

• }

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.

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

• 	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

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

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

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.

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

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?

• "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.

• "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@denx.de =====================================================================