[U-Boot] [PATCH] [OneNAND] bad block aware read/write support
Stefan Roese
sr at denx.de
Tue Nov 4 10:48:49 CET 2008
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 at 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 at 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 at denx.de
=====================================================================
More information about the U-Boot
mailing list