[PATCH 0/2] MTD: OneNAND: multiblock erase support

[PATCH 0/2] MTD: OneNAND: multiblock erase support

[Mika Korhonen]

(Sorry about CCing this twice - I made a typo in the mailing list address.
 Please reply to this.)

This patch series is an updated version of
http://lists.infradead.org/pipermail/linux-mtd/2009-June/026130.html

I split the previous patch in two for readability: the first part extracts
the execution of erase command to a separate function for easier integration
of different erase method. The second part implements the multiblock erase
function in case multiple blocks are requested to be erased and the chip is
not Flex.

This is useful for flashing applications that need to do their work as
fast as possible. For full 64 eraseblock case the erase speed is up to 30x
faster. (Samsung: 64 MB/s vs 2.1 GB/s, I got 1.4 GB/s on Linux kernel)


Mika Korhonen (2):
  MTD: OneNAND: move erase method to a separate function
  MTD: OneNAND: multiblock erase support

 drivers/mtd/onenand/omap2.c        |   22 +++-
 drivers/mtd/onenand/onenand_base.c |  274 +++++++++++++++++++++++++++++-------
 include/linux/mtd/onenand.h        |    2 +
 include/linux/mtd/onenand_regs.h   |    2 +
 4 files changed, 246 insertions(+), 54 deletions(-)

[PATCH 1/2] MTD: OneNAND: move erase method to a separate function

[Mika Korhonen]

Separate the actual execution of erase to a new function:
onenand_single_block_erase()

Signed-off-by: Mika Korhonen <ext-mika.2.korhonen@nokia.com>
---
 drivers/mtd/onenand/onenand_base.c |  139 +++++++++++++++++++++--------------
 1 files changed, 83 insertions(+), 56 deletions(-)

diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
index 6e82909..ce9f9a0 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -2141,77 +2141,43 @@ static int onenand_block_isbad_nolock(struct mtd_info *mtd, loff_t ofs, int allo
 }
 
 /**
- * onenand_erase - [MTD Interface] erase block(s)
+ * onenand_single_block_erase - [Internal] erase block(s) using regular erase
  * @param mtd		MTD device structure
  * @param instr		erase instruction
+ * @param region	erase region
  *
- * Erase one ore more blocks
+ * Erase one or more blocks one block at a time
  */
-static int onenand_erase(struct mtd_info *mtd, struct erase_info *instr)
+static int onenand_single_block_erase(struct mtd_info *mtd,
+				      struct erase_info *instr,
+				      struct mtd_erase_region_info *region)
 {
 	struct onenand_chip *this = mtd->priv;
-	unsigned int block_size;
 	loff_t addr = instr->addr;
-	loff_t len = instr->len;
-	int ret = 0, i;
-	struct mtd_erase_region_info *region = NULL;
+	int len = instr->len;
+	unsigned int block_size;
 	loff_t region_end = 0;
+	int ret = 0;
 
-	DEBUG(MTD_DEBUG_LEVEL3, "onenand_erase: start = 0x%012llx, len = %llu\n", (unsigned long long) instr->addr, (unsigned long long) instr->len);
-
-	/* Do not allow erase past end of device */
-	if (unlikely((len + addr) > mtd->size)) {
-		printk(KERN_ERR "onenand_erase: Erase past end of device\n");
-		return -EINVAL;
-	}
-
-	if (FLEXONENAND(this)) {
-		/* Find the eraseregion of this address */
-		i = flexonenand_region(mtd, addr);
-		region = &mtd->eraseregions[i];
-
+	if (region) {
+		/* region is set for Flex-OneNAND */
 		block_size = region->erasesize;
 		region_end = region->offset + region->erasesize * region->numblocks;
-
-		/* Start address within region must align on block boundary.
-		 * Erase region's start offset is always block start address.
-		 */
-		if (unlikely((addr - region->offset) & (block_size - 1))) {
-			printk(KERN_ERR "onenand_erase: Unaligned address\n");
-			return -EINVAL;
-		}
 	} else {
-		block_size = 1 << this->erase_shift;
-
-		/* Start address must align on block boundary */
-		if (unlikely(addr & (block_size - 1))) {
-			printk(KERN_ERR "onenand_erase: Unaligned address\n");
-			return -EINVAL;
-		}
-	}
-
-	/* Length must align on block boundary */
-	if (unlikely(len & (block_size - 1))) {
-		printk(KERN_ERR "onenand_erase: Length not block aligned\n");
-		return -EINVAL;
+		block_size = (1 << this->erase_shift);
 	}
 
-	instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
-
-	/* Grab the lock and see if the device is available */
-	onenand_get_device(mtd, FL_ERASING);
-
-	/* Loop throught the pages */
 	instr->state = MTD_ERASING;
 
+	/* Loop through the blocks */
 	while (len) {
 		cond_resched();
 
 		/* Check if we have a bad block, we do not erase bad blocks */
 		if (onenand_block_isbad_nolock(mtd, addr, 0)) {
-			printk (KERN_WARNING "onenand_erase: attempt to erase a bad block at addr 0x%012llx\n", (unsigned long long) addr);
+			printk(KERN_WARNING "onenand_erase: attempt to erase a bad block at addr 0x%012llx\n", (unsigned long long) addr);
 			instr->state = MTD_ERASE_FAILED;
-			goto erase_exit;
+			return -1;
 		}
 
 		this->command(mtd, ONENAND_CMD_ERASE, addr, block_size);
@@ -2222,10 +2188,10 @@ static int onenand_erase(struct mtd_info *mtd, struct erase_info *instr)
 		/* Check, if it is write protected */
 		if (ret) {
 			printk(KERN_ERR "onenand_erase: Failed erase, block %d\n",
-						 onenand_block(this, addr));
+			       onenand_block(this, addr));
 			instr->state = MTD_ERASE_FAILED;
 			instr->fail_addr = addr;
-			goto erase_exit;
+			return -1;
 		}
 
 		len -= block_size;
@@ -2235,24 +2201,85 @@ static int onenand_erase(struct mtd_info *mtd, struct erase_info *instr)
 			if (!len)
 				break;
 			region++;
-
 			block_size = region->erasesize;
 			region_end = region->offset + region->erasesize * region->numblocks;
 
 			if (len & (block_size - 1)) {
 				/* FIXME: This should be handled at MTD partitioning level. */
 				printk(KERN_ERR "onenand_erase: Unaligned address\n");
-				goto erase_exit;
+				return -1;
 			}
 		}
+	}
+
+	return 0;
+}
+
+
+/**
+ * onenand_erase - [MTD Interface] erase block(s)
+ * @param mtd		MTD device structure
+ * @param instr		erase instruction
+ *
+ * Erase one or more blocks
+ */
+static int onenand_erase(struct mtd_info *mtd, struct erase_info *instr)
+{
+	struct onenand_chip *this = mtd->priv;
+	unsigned int block_size;
+	loff_t addr = instr->addr;
+	loff_t len = instr->len;
+	int ret = 0;
+	struct mtd_erase_region_info *region = NULL;
+	loff_t region_offset = 0;
 
+	DEBUG(MTD_DEBUG_LEVEL3, "onenand_erase: start = 0x%012llx, len = %llu\n", (unsigned long long) instr->addr, (unsigned long long) instr->len);
+
+	/* Do not allow erase past end of device */
+	if (unlikely((len + addr) > mtd->size)) {
+		printk(KERN_ERR "onenand_erase: Erase past end of device\n");
+		return -EINVAL;
 	}
 
-	instr->state = MTD_ERASE_DONE;
+	if (FLEXONENAND(this)) {
+		/* Find the eraseregion of this address */
+		int i = flexonenand_region(mtd, addr);
+		region = &mtd->eraseregions[i];
 
-erase_exit:
+		block_size = region->erasesize;
 
-	ret = instr->state == MTD_ERASE_DONE ? 0 : -EIO;
+		/* Start address within region must align on block boundary.
+		 * Erase region's start offset is always block start address.
+		 */
+		region_offset = region->offset;
+	} else {
+		block_size = 1 << this->erase_shift;
+	}
+
+	/* Start address must align on block boundary */
+	if (unlikely((addr - region_offset) & (block_size - 1))) {
+		printk(KERN_ERR "onenand_erase: Unaligned address\n");
+		return -EINVAL;
+	}
+
+	/* Length must align on block boundary */
+	if (unlikely(len & (block_size - 1))) {
+		printk(KERN_ERR "onenand_erase: Length not block aligned\n");
+		return -EINVAL;
+	}
+
+	instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
+
+	/* Grab the lock and see if the device is available */
+	onenand_get_device(mtd, FL_ERASING);
+
+	ret = onenand_single_block_erase(mtd, instr, region);
+
+	if (ret) {
+		ret = -EIO;
+	} else {
+		instr->state = MTD_ERASE_DONE;
+	}
 
 	/* Deselect and wake up anyone waiting on the device */
 	onenand_release_device(mtd);
-- 
1.6.0.4

[PATCH 2/2] MTD: OneNAND: multiblock erase support

[Mika Korhonen]

Add support for multiblock erase command. OneNANDs (excluding Flex-OneNAND)
are capable of simultaneous erase of up to 64 eraseblocks which is much
faster.
This changes the erase requests for regions covering multiple eraseblocks
to be performed using multiblock erase.

Signed-off-by: Mika Korhonen <ext-mika.2.korhonen@nokia.com>
---
 drivers/mtd/onenand/omap2.c        |   22 ++++-
 drivers/mtd/onenand/onenand_base.c |  151 +++++++++++++++++++++++++++++++++++-
 include/linux/mtd/onenand.h        |    2 +
 include/linux/mtd/onenand_regs.h   |    2 +
 4 files changed, 171 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c
index 0108ed4..47ae815 100644
--- a/drivers/mtd/onenand/omap2.c
+++ b/drivers/mtd/onenand/omap2.c
@@ -112,10 +112,24 @@ static int omap2_onenand_wait(struct mtd_info *mtd, int state)
 	unsigned long timeout;
 	u32 syscfg;
 
-	if (state == FL_RESETING) {
-		int i;
+	if (state == FL_RESETING || state == FL_PREPARING_ERASE ||
+		state == FL_VERIFYING_ERASE) {
+
+		int i = 21;
+		unsigned int intr_flags = ONENAND_INT_MASTER;
+		switch (state) {
+		case FL_RESETING:
+			intr_flags |= ONENAND_INT_RESET;
+			break;
+		case FL_PREPARING_ERASE:
+			intr_flags |= ONENAND_INT_ERASE;
+			break;
+		case FL_VERIFYING_ERASE:
+			i = 51;
+			break;
+		}
 
-		for (i = 0; i < 20; i++) {
+		while (--i) {
 			udelay(1);
 			intr = read_reg(c, ONENAND_REG_INTERRUPT);
 			if (intr & ONENAND_INT_MASTER)
@@ -126,7 +140,7 @@ static int omap2_onenand_wait(struct mtd_info *mtd, int state)
 			wait_err("controller error", state, ctrl, intr);
 			return -EIO;
 		}
-		if (!(intr & ONENAND_INT_RESET)) {
+		if (!(intr & intr_flags)) {
 			wait_err("timeout", state, ctrl, intr);
 			return -EIO;
 		}
diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
index ce9f9a0..1fa9101 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -32,6 +32,12 @@
 
 #include <asm/io.h>
 
+/* Multiblock erase if number of blocks to erase is 2 or more.
+   Maximum number of blocks for simultaneous erase is 64. */
+#define MB_ERASE_MIN_BLK_COUNT 2
+#define MB_ERASE_MAX_BLK_COUNT 64
+
+
 /* Default Flex-OneNAND boundary and lock respectively */
 static int flex_bdry[MAX_DIES * 2] = { -1, 0, -1, 0 };
 
@@ -339,6 +345,8 @@ static int onenand_command(struct mtd_info *mtd, int cmd, loff_t addr, size_t le
 		break;
 
 	case ONENAND_CMD_ERASE:
+	case ONENAND_CMD_MULTIBLOCK_ERASE:
+	case ONENAND_CMD_ERASE_VERIFY:
 	case ONENAND_CMD_BUFFERRAM:
 	case ONENAND_CMD_OTP_ACCESS:
 		block = onenand_block(this, addr);
@@ -483,7 +491,7 @@ static int onenand_wait(struct mtd_info *mtd, int state)
 		if (interrupt & flags)
 			break;
 
-		if (state != FL_READING)
+		if (state != FL_READING && state != FL_PREPARING_ERASE)
 			cond_resched();
 	}
 	/* To get correct interrupt status in timeout case */
@@ -513,6 +521,18 @@ static int onenand_wait(struct mtd_info *mtd, int state)
 		return -EIO;
 	}
 
+	if (state == FL_PREPARING_ERASE && !(interrupt & ONENAND_INT_ERASE)) {
+		printk(KERN_ERR "onenand_wait: mb erase timeout! ctrl=0x%04x intr=0x%04x\n",
+			ctrl, interrupt);
+		return -EIO;
+	}
+
+	if (!(interrupt & ONENAND_INT_MASTER)) {
+		printk(KERN_ERR "onenand_wait: timeout! ctrl=0x%04x intr=0x%04x\n",
+			ctrl, interrupt);
+		return -EIO;
+	}
+
 	/* If there's controller error, it's a real error */
 	if (ctrl & ONENAND_CTRL_ERROR) {
 		printk(KERN_ERR "onenand_wait: controller error = 0x%04x\n",
@@ -2140,6 +2160,128 @@ static int onenand_block_isbad_nolock(struct mtd_info *mtd, loff_t ofs, int allo
 	return bbm->isbad_bbt(mtd, ofs, allowbbt);
 }
 
+
+static int onenand_multiblock_erase_verify(struct mtd_info *mtd,
+					   struct erase_info *instr)
+{
+	struct onenand_chip *this = mtd->priv;
+	loff_t addr = instr->addr;
+	int len = instr->len;
+	unsigned int block_size = (1 << this->erase_shift);
+	int ret = 0;
+
+	while (len) {
+		this->command(mtd, ONENAND_CMD_ERASE_VERIFY, addr, block_size);
+		ret = this->wait(mtd, FL_VERIFYING_ERASE);
+		if (ret) {
+			printk(KERN_ERR "onenand_multiblock_erase_verify: "
+			       "Failed verify, block %d\n", onenand_block(this, addr));
+			instr->state = MTD_ERASE_FAILED;
+			instr->fail_addr = addr;
+			return -1;
+		}
+		len -= block_size;
+		addr += block_size;
+	}
+	return 0;
+}
+
+/**
+ * onenand_multiblock_erase - [Internal] erase block(s) using multiblock erase
+ * @param mtd		MTD device structure
+ * @param instr		erase instruction
+ * @param region	erase region
+ *
+ * Erase one or more blocks up to 64 block at a time
+ */
+static int onenand_multiblock_erase(struct mtd_info *mtd,
+				    struct erase_info *instr)
+{
+	struct onenand_chip *this = mtd->priv;
+	loff_t addr = instr->addr;
+	int len = instr->len;
+	unsigned int block_size = (1 << this->erase_shift);
+	int eb_count = 0;
+	int ret = 0;
+
+	instr->state = MTD_ERASING;
+
+	/* Pre-check bbs */
+	while (len) {
+		/* Check if we have a bad block, we do not erase bad blocks */
+		if (onenand_block_isbad_nolock(mtd, addr, 0)) {
+			printk(KERN_WARNING "onenand_erase: attempt to erase a bad block at addr 0x%012llx\n", (unsigned long long) addr);
+			instr->state = MTD_ERASE_FAILED;
+			return -1;
+		}
+		len -= block_size;
+		addr += block_size;
+	}
+
+	len = instr->len;
+	addr = instr->addr;
+
+
+	/* loop over 64 eb batches */
+	while (len) {
+		struct erase_info verify_instr = *instr;
+		verify_instr.addr = addr;
+		verify_instr.len = 0;
+
+		eb_count = 0;
+
+		while (len > block_size &&
+		       eb_count < (MB_ERASE_MAX_BLK_COUNT - 1)) {
+			this->command(mtd, ONENAND_CMD_MULTIBLOCK_ERASE, addr, block_size);
+			onenand_invalidate_bufferram(mtd, addr, block_size);
+
+			ret = this->wait(mtd, FL_PREPARING_ERASE);
+			if (ret) {
+				printk(KERN_ERR "onenand_multiblock_erase: "
+				       "Failed multiblock erase, block %d\n",
+				       onenand_block(this, addr));
+				instr->state = MTD_ERASE_FAILED;
+				instr->fail_addr = addr;
+				return -1;
+			}
+
+			len -= block_size;
+			addr += block_size;
+			eb_count++;
+		}
+
+		/* last block of 64-eb series */
+		cond_resched();
+		this->command(mtd, ONENAND_CMD_ERASE, addr, block_size);
+		onenand_invalidate_bufferram(mtd, addr, block_size);
+
+		ret = this->wait(mtd, FL_ERASING);
+		/* Check, if it is write protected */
+		if (ret) {
+			printk(KERN_ERR "onenand_erase: Failed erase, block %d\n",
+			       onenand_block(this, addr));
+			instr->state = MTD_ERASE_FAILED;
+			instr->fail_addr = addr;
+			return -1;
+		}
+
+		len -= block_size;
+		addr += block_size;
+		eb_count++;
+
+		/* verify */
+		verify_instr.len = eb_count * block_size;
+		if (onenand_multiblock_erase_verify(mtd, &verify_instr)) {
+			instr->state = verify_instr.state;
+			instr->fail_addr = verify_instr.fail_addr;
+			return -1;
+		}
+
+	}
+	return 0;
+}
+
+
 /**
  * onenand_single_block_erase - [Internal] erase block(s) using regular erase
  * @param mtd		MTD device structure
@@ -2273,7 +2415,12 @@ static int onenand_erase(struct mtd_info *mtd, struct erase_info *instr)
 	/* Grab the lock and see if the device is available */
 	onenand_get_device(mtd, FL_ERASING);
 
-	ret = onenand_single_block_erase(mtd, instr, region);
+	if (region || instr->len < MB_ERASE_MIN_BLK_COUNT * block_size) {
+		/* region is set for Flex-OneNAND (no mb erase) */
+		ret = onenand_single_block_erase(mtd, instr, region);
+	} else {
+		ret = onenand_multiblock_erase(mtd, instr);
+	}
 
 	if (ret) {
 		ret = -EIO;
diff --git a/include/linux/mtd/onenand.h b/include/linux/mtd/onenand.h
index 8ed8733..42384f3 100644
--- a/include/linux/mtd/onenand.h
+++ b/include/linux/mtd/onenand.h
@@ -39,6 +39,8 @@ typedef enum {
 	FL_RESETING,
 	FL_OTPING,
 	FL_PM_SUSPENDED,
+	FL_PREPARING_ERASE,
+	FL_VERIFYING_ERASE
 } onenand_state_t;
 
 /**
diff --git a/include/linux/mtd/onenand_regs.h b/include/linux/mtd/onenand_regs.h
index 86a6bbe..bd346e5 100644
--- a/include/linux/mtd/onenand_regs.h
+++ b/include/linux/mtd/onenand_regs.h
@@ -131,6 +131,8 @@
 #define ONENAND_CMD_LOCK_TIGHT		(0x2C)
 #define ONENAND_CMD_UNLOCK_ALL		(0x27)
 #define ONENAND_CMD_ERASE		(0x94)
+#define ONENAND_CMD_MULTIBLOCK_ERASE	(0x95)
+#define ONENAND_CMD_ERASE_VERIFY	(0x71)
 #define ONENAND_CMD_RESET		(0xF0)
 #define ONENAND_CMD_OTP_ACCESS		(0x65)
 #define ONENAND_CMD_READID		(0x90)
-- 
1.6.0.4

Re: [PATCH 2/2] MTD: OneNAND: multiblock erase support

[Adrian Hunter]

Korhonen Mika.2 (EXT-Ardites/Oulu) wrote:
> Add support for multiblock erase command. OneNANDs (excluding Flex-OneNAND)
> are capable of simultaneous erase of up to 64 eraseblocks which is much
> faster.
> This changes the erase requests for regions covering multiple eraseblocks
> to be performed using multiblock erase.
> 
> Signed-off-by: Mika Korhonen <ext-mika.2.korhonen@nokia.com>
> ---

A few comments below.

>  drivers/mtd/onenand/omap2.c        |   22 ++++-
>  drivers/mtd/onenand/onenand_base.c |  151 +++++++++++++++++++++++++++++++++++-
>  include/linux/mtd/onenand.h        |    2 +
>  include/linux/mtd/onenand_regs.h   |    2 +
>  4 files changed, 171 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c
> index 0108ed4..47ae815 100644
> --- a/drivers/mtd/onenand/omap2.c
> +++ b/drivers/mtd/onenand/omap2.c
> @@ -112,10 +112,24 @@ static int omap2_onenand_wait(struct mtd_info *mtd, int state)
>  	unsigned long timeout;
>  	u32 syscfg;
>  
> -	if (state == FL_RESETING) {
> -		int i;
> +	if (state == FL_RESETING || state == FL_PREPARING_ERASE ||
> +		state == FL_VERIFYING_ERASE) {

Better to align like this

+	    state == FL_VERIFYING_ERASE) {


> +

Drop this blank line

> +		int i = 21;
> +		unsigned int intr_flags = ONENAND_INT_MASTER;

And add a blank line here

> +		switch (state) {
> +		case FL_RESETING:
> +			intr_flags |= ONENAND_INT_RESET;
> +			break;
> +		case FL_PREPARING_ERASE:
> +			intr_flags |= ONENAND_INT_ERASE;
> +			break;
> +		case FL_VERIFYING_ERASE:
> +			i = 51;

I see 100us for this in some versions.  Perhaps Kyungmin can suggest
a value.

> +			break;
> +		}
>  
> -		for (i = 0; i < 20; i++) {
> +		while (--i) {
>  			udelay(1);
>  			intr = read_reg(c, ONENAND_REG_INTERRUPT);
>  			if (intr & ONENAND_INT_MASTER)
> @@ -126,7 +140,7 @@ static int omap2_onenand_wait(struct mtd_info *mtd, int state)
>  			wait_err("controller error", state, ctrl, intr);
>  			return -EIO;
>  		}
> -		if (!(intr & ONENAND_INT_RESET)) {
> +		if (!(intr & intr_flags)) {

Since you have OR'd in the ONENAND_INT_MASTER flag, this doesn't check the individual
flags anymore.  Perhaps you want:

+		if ((intr & intr_flags) != intr_flags)) {

>  			wait_err("timeout", state, ctrl, intr);
>  			return -EIO;
>  		}
> diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
> index ce9f9a0..1fa9101 100644
> --- a/drivers/mtd/onenand/onenand_base.c
> +++ b/drivers/mtd/onenand/onenand_base.c
> @@ -32,6 +32,12 @@
>  
>  #include <asm/io.h>
>  
> +/* Multiblock erase if number of blocks to erase is 2 or more.
> +   Maximum number of blocks for simultaneous erase is 64. */

It is more usual to write multiline comments like this:

/*
 * Multiblock erase if number of blocks to erase is 2 or more.
 * Maximum number of blocks for simultaneous erase is 64.
 */

> +#define MB_ERASE_MIN_BLK_COUNT 2
> +#define MB_ERASE_MAX_BLK_COUNT 64
> +
> +
>  /* Default Flex-OneNAND boundary and lock respectively */
>  static int flex_bdry[MAX_DIES * 2] = { -1, 0, -1, 0 };
>  
> @@ -339,6 +345,8 @@ static int onenand_command(struct mtd_info *mtd, int cmd, loff_t addr, size_t le
>  		break;
>  
>  	case ONENAND_CMD_ERASE:
> +	case ONENAND_CMD_MULTIBLOCK_ERASE:
> +	case ONENAND_CMD_ERASE_VERIFY:
>  	case ONENAND_CMD_BUFFERRAM:
>  	case ONENAND_CMD_OTP_ACCESS:
>  		block = onenand_block(this, addr);
> @@ -483,7 +491,7 @@ static int onenand_wait(struct mtd_info *mtd, int state)
>  		if (interrupt & flags)
>  			break;
>  
> -		if (state != FL_READING)
> +		if (state != FL_READING && state != FL_PREPARING_ERASE)
>  			cond_resched();
>  	}
>  	/* To get correct interrupt status in timeout case */
> @@ -513,6 +521,18 @@ static int onenand_wait(struct mtd_info *mtd, int state)
>  		return -EIO;
>  	}
>  
> +	if (state == FL_PREPARING_ERASE && !(interrupt & ONENAND_INT_ERASE)) {
> +		printk(KERN_ERR "onenand_wait: mb erase timeout! ctrl=0x%04x intr=0x%04x\n",
> +			ctrl, interrupt);
> +		return -EIO;
> +	}
> +
> +	if (!(interrupt & ONENAND_INT_MASTER)) {
> +		printk(KERN_ERR "onenand_wait: timeout! ctrl=0x%04x intr=0x%04x\n",
> +			ctrl, interrupt);
> +		return -EIO;
> +	}
> +
>  	/* If there's controller error, it's a real error */
>  	if (ctrl & ONENAND_CTRL_ERROR) {
>  		printk(KERN_ERR "onenand_wait: controller error = 0x%04x\n",
> @@ -2140,6 +2160,128 @@ static int onenand_block_isbad_nolock(struct mtd_info *mtd, loff_t ofs, int allo
>  	return bbm->isbad_bbt(mtd, ofs, allowbbt);
>  }
>  
> +
> +static int onenand_multiblock_erase_verify(struct mtd_info *mtd,
> +					   struct erase_info *instr)
> +{
> +	struct onenand_chip *this = mtd->priv;
> +	loff_t addr = instr->addr;
> +	int len = instr->len;
> +	unsigned int block_size = (1 << this->erase_shift);
> +	int ret = 0;
> +
> +	while (len) {
> +		this->command(mtd, ONENAND_CMD_ERASE_VERIFY, addr, block_size);
> +		ret = this->wait(mtd, FL_VERIFYING_ERASE);
> +		if (ret) {
> +			printk(KERN_ERR "onenand_multiblock_erase_verify: "
> +			       "Failed verify, block %d\n", onenand_block(this, addr));
> +			instr->state = MTD_ERASE_FAILED;
> +			instr->fail_addr = addr;
> +			return -1;
> +		}
> +		len -= block_size;
> +		addr += block_size;
> +	}
> +	return 0;
> +}
> +
> +/**
> + * onenand_multiblock_erase - [Internal] erase block(s) using multiblock erase
> + * @param mtd		MTD device structure
> + * @param instr		erase instruction
> + * @param region	erase region
> + *
> + * Erase one or more blocks up to 64 block at a time
> + */
> +static int onenand_multiblock_erase(struct mtd_info *mtd,
> +				    struct erase_info *instr)
> +{
> +	struct onenand_chip *this = mtd->priv;
> +	loff_t addr = instr->addr;
> +	int len = instr->len;
> +	unsigned int block_size = (1 << this->erase_shift);
> +	int eb_count = 0;
> +	int ret = 0;
> +
> +	instr->state = MTD_ERASING;
> +
> +	/* Pre-check bbs */
> +	while (len) {
> +		/* Check if we have a bad block, we do not erase bad blocks */
> +		if (onenand_block_isbad_nolock(mtd, addr, 0)) {
> +			printk(KERN_WARNING "onenand_erase: attempt to erase a bad block at addr 0x%012llx\n", (unsigned long long) addr);
> +			instr->state = MTD_ERASE_FAILED;
> +			return -1;
> +		}
> +		len -= block_size;
> +		addr += block_size;
> +	}
> +
> +	len = instr->len;
> +	addr = instr->addr;
> +
> +
> +	/* loop over 64 eb batches */
> +	while (len) {
> +		struct erase_info verify_instr = *instr;
> +		verify_instr.addr = addr;
> +		verify_instr.len = 0;
> +
> +		eb_count = 0;
> +
> +		while (len > block_size &&
> +		       eb_count < (MB_ERASE_MAX_BLK_COUNT - 1)) {

According to the manual I have you cannot do a multiblock erase
across a chip boundary, so you must exit this loop at the boundary too.

> +			this->command(mtd, ONENAND_CMD_MULTIBLOCK_ERASE, addr, block_size);
> +			onenand_invalidate_bufferram(mtd, addr, block_size);
> +
> +			ret = this->wait(mtd, FL_PREPARING_ERASE);
> +			if (ret) {
> +				printk(KERN_ERR "onenand_multiblock_erase: "
> +				       "Failed multiblock erase, block %d\n",
> +				       onenand_block(this, addr));
> +				instr->state = MTD_ERASE_FAILED;
> +				instr->fail_addr = addr;
> +				return -1;
> +			}
> +
> +			len -= block_size;
> +			addr += block_size;
> +			eb_count++;
> +		}
> +
> +		/* last block of 64-eb series */
> +		cond_resched();
> +		this->command(mtd, ONENAND_CMD_ERASE, addr, block_size);
> +		onenand_invalidate_bufferram(mtd, addr, block_size);
> +
> +		ret = this->wait(mtd, FL_ERASING);
> +		/* Check, if it is write protected */
> +		if (ret) {
> +			printk(KERN_ERR "onenand_erase: Failed erase, block %d\n",
> +			       onenand_block(this, addr));
> +			instr->state = MTD_ERASE_FAILED;
> +			instr->fail_addr = addr;
> +			return -1;
> +		}
> +
> +		len -= block_size;
> +		addr += block_size;
> +		eb_count++;
> +
> +		/* verify */
> +		verify_instr.len = eb_count * block_size;
> +		if (onenand_multiblock_erase_verify(mtd, &verify_instr)) {
> +			instr->state = verify_instr.state;
> +			instr->fail_addr = verify_instr.fail_addr;
> +			return -1;
> +		}
> +
> +	}
> +	return 0;
> +}
> +
> +
>  /**
>   * onenand_single_block_erase - [Internal] erase block(s) using regular erase
>   * @param mtd		MTD device structure
> @@ -2273,7 +2415,12 @@ static int onenand_erase(struct mtd_info *mtd, struct erase_info *instr)
>  	/* Grab the lock and see if the device is available */
>  	onenand_get_device(mtd, FL_ERASING);
>  
> -	ret = onenand_single_block_erase(mtd, instr, region);
> +	if (region || instr->len < MB_ERASE_MIN_BLK_COUNT * block_size) {
> +		/* region is set for Flex-OneNAND (no mb erase) */
> +		ret = onenand_single_block_erase(mtd, instr, region);
> +	} else {
> +		ret = onenand_multiblock_erase(mtd, instr);
> +	}
>  
>  	if (ret) {
>  		ret = -EIO;
> diff --git a/include/linux/mtd/onenand.h b/include/linux/mtd/onenand.h
> index 8ed8733..42384f3 100644
> --- a/include/linux/mtd/onenand.h
> +++ b/include/linux/mtd/onenand.h
> @@ -39,6 +39,8 @@ typedef enum {
>  	FL_RESETING,
>  	FL_OTPING,
>  	FL_PM_SUSPENDED,
> +	FL_PREPARING_ERASE,
> +	FL_VERIFYING_ERASE

Last one should also have a comma

>  } onenand_state_t;
>  
>  /**
> diff --git a/include/linux/mtd/onenand_regs.h b/include/linux/mtd/onenand_regs.h
> index 86a6bbe..bd346e5 100644
> --- a/include/linux/mtd/onenand_regs.h
> +++ b/include/linux/mtd/onenand_regs.h
> @@ -131,6 +131,8 @@
>  #define ONENAND_CMD_LOCK_TIGHT		(0x2C)
>  #define ONENAND_CMD_UNLOCK_ALL		(0x27)
>  #define ONENAND_CMD_ERASE		(0x94)
> +#define ONENAND_CMD_MULTIBLOCK_ERASE	(0x95)
> +#define ONENAND_CMD_ERASE_VERIFY	(0x71)
>  #define ONENAND_CMD_RESET		(0xF0)
>  #define ONENAND_CMD_OTP_ACCESS		(0x65)
>  #define ONENAND_CMD_READID		(0x90)

Re: [PATCH 2/2] MTD: OneNAND: multiblock erase support

[Mika Korhonen]

Hunter Adrian (Nokia-D/Helsinki) wrote:
> Korhonen Mika.2 (EXT-Ardites/Oulu) wrote:
>   
>> Add support for multiblock erase command. OneNANDs (excluding Flex-OneNAND)
>> are capable of simultaneous erase of up to 64 eraseblocks which is much
>> faster.
>> This changes the erase requests for regions covering multiple eraseblocks
>> to be performed using multiblock erase.
>>
>> Signed-off-by: Mika Korhonen <ext-mika.2.korhonen@nokia.com>
>> ---
>>     
>
> A few comments below.
>
>   
[...]
> +		switch (state) {
> +		case FL_RESETING:
> +			intr_flags |= ONENAND_INT_RESET;
> +			break;
> +		case FL_PREPARING_ERASE:
> +			intr_flags |= ONENAND_INT_ERASE;
> +			break;
> +		case FL_VERIFYING_ERASE:
> +			i = 51;
>   
>
> I see 100us for this in some versions.  Perhaps Kyungmin can suggest
> a value.
>   
With the chip I had the actual time for 64-eb verify read was in the 
range of 20-30 us but you're right, there are chips for which the 
maximum time is specified to be 100 us. Is ok to just raise this to that 
value? Kyungmin?

[...]
>
>> +
>> +	/* loop over 64 eb batches */
>> +	while (len) {
>> +		struct erase_info verify_instr = *instr;
>> +		verify_instr.addr = addr;
>> +		verify_instr.len = 0;
>> +
>> +		eb_count = 0;
>> +
>> +		while (len > block_size &&
>> +		       eb_count < (MB_ERASE_MAX_BLK_COUNT - 1)) {
>>     
>
> According to the manual I have you cannot do a multiblock erase
> across a chip boundary, so you must exit this loop at the boundary too.
>   

Ok, does this apply DDP chips only?

br,
Mika

Re: [PATCH 2/2] MTD: OneNAND: multiblock erase support

[Kyungmin Park]

On Fri, Sep 18, 2009 at 2:03 PM, Mika Korhonen
<ext-mika.2.korhonen@nokia.com> wrote:
> Hunter Adrian (Nokia-D/Helsinki) wrote:
>>
>> Korhonen Mika.2 (EXT-Ardites/Oulu) wrote:
>>
>>>
>>> Add support for multiblock erase command. OneNANDs (excluding
>>> Flex-OneNAND)
>>> are capable of simultaneous erase of up to 64 eraseblocks which is much
>>> faster.
>>> This changes the erase requests for regions covering multiple eraseblocks
>>> to be performed using multiblock erase.
>>>
>>> Signed-off-by: Mika Korhonen <ext-mika.2.korhonen@nokia.com>
>>> ---
>>>
>>
>> A few comments below.
>>
>>
>
> [...]
>>
>> +               switch (state) {
>> +               case FL_RESETING:
>> +                       intr_flags |= ONENAND_INT_RESET;
>> +                       break;
>> +               case FL_PREPARING_ERASE:
>> +                       intr_flags |= ONENAND_INT_ERASE;
>> +                       break;
>> +               case FL_VERIFYING_ERASE:
>> +                       i = 51;
>>
>> I see 100us for this in some versions.  Perhaps Kyungmin can suggest
>> a value.
>>
>
> With the chip I had the actual time for 64-eb verify read was in the range
> of 20-30 us but you're right, there are chips for which the maximum time is
> specified to be 100 us. Is ok to just raise this to that value? Kyungmin?

To correctness, It should be 100 as Spec.

Just FYI:
In recent OneNAND chips has 4KiB pagesize doesn't support Multi block
erase feature at my spec.

Right, these chips are not yet support at MTD. After make a patch, we
have to check the multi block erase feature.

Thank you,
Kyungmin Park

Re: [PATCH 2/2] MTD: OneNAND: multiblock erase support

[Adrian Hunter]

Korhonen Mika.2 (EXT-Ardites/Oulu) wrote:
> Hunter Adrian (Nokia-D/Helsinki) wrote:
>> Korhonen Mika.2 (EXT-Ardites/Oulu) wrote:
>>   
>>> Add support for multiblock erase command. OneNANDs (excluding Flex-OneNAND)
>>> are capable of simultaneous erase of up to 64 eraseblocks which is much
>>> faster.
>>> This changes the erase requests for regions covering multiple eraseblocks
>>> to be performed using multiblock erase.
>>>
>>> Signed-off-by: Mika Korhonen <ext-mika.2.korhonen@nokia.com>
>>> ---
>>>     
>> A few comments below.
>>
>>   
> [...]
>> +		switch (state) {
>> +		case FL_RESETING:
>> +			intr_flags |= ONENAND_INT_RESET;
>> +			break;
>> +		case FL_PREPARING_ERASE:
>> +			intr_flags |= ONENAND_INT_ERASE;
>> +			break;
>> +		case FL_VERIFYING_ERASE:
>> +			i = 51;
>>   
>>
>> I see 100us for this in some versions.  Perhaps Kyungmin can suggest
>> a value.
>>   
> With the chip I had the actual time for 64-eb verify read was in the 
> range of 20-30 us but you're right, there are chips for which the 
> maximum time is specified to be 100 us. Is ok to just raise this to that 
> value? Kyungmin?
> 
> [...]
>>> +
>>> +	/* loop over 64 eb batches */
>>> +	while (len) {
>>> +		struct erase_info verify_instr = *instr;
>>> +		verify_instr.addr = addr;
>>> +		verify_instr.len = 0;
>>> +
>>> +		eb_count = 0;
>>> +
>>> +		while (len > block_size &&
>>> +		       eb_count < (MB_ERASE_MAX_BLK_COUNT - 1)) {
>>>     
>> According to the manual I have you cannot do a multiblock erase
>> across a chip boundary, so you must exit this loop at the boundary too.
>>   
> 
> Ok, does this apply DDP chips only?

Yes, AFAIK. Kyungmin?

Re: [PATCH 2/2] MTD: OneNAND: multiblock erase support

[Kyungmin Park]

On Fri, Sep 18, 2009 at 5:04 PM, Adrian Hunter <adrian.hunter@nokia.com> wrote:
> Korhonen Mika.2 (EXT-Ardites/Oulu) wrote:
>>
>> Hunter Adrian (Nokia-D/Helsinki) wrote:
>>>
>>> Korhonen Mika.2 (EXT-Ardites/Oulu) wrote:
>>>
>>>>
>>>> Add support for multiblock erase command. OneNANDs (excluding
>>>> Flex-OneNAND)
>>>> are capable of simultaneous erase of up to 64 eraseblocks which is much
>>>> faster.
>>>> This changes the erase requests for regions covering multiple
>>>> eraseblocks
>>>> to be performed using multiblock erase.
>>>>
>>>> Signed-off-by: Mika Korhonen <ext-mika.2.korhonen@nokia.com>
>>>> ---
>>>>
>>>
>>> A few comments below.
>>>
>>>
>>
>> [...]
>>>
>>> +               switch (state) {
>>> +               case FL_RESETING:
>>> +                       intr_flags |= ONENAND_INT_RESET;
>>> +                       break;
>>> +               case FL_PREPARING_ERASE:
>>> +                       intr_flags |= ONENAND_INT_ERASE;
>>> +                       break;
>>> +               case FL_VERIFYING_ERASE:
>>> +                       i = 51;
>>>
>>> I see 100us for this in some versions.  Perhaps Kyungmin can suggest
>>> a value.
>>>
>>
>> With the chip I had the actual time for 64-eb verify read was in the range
>> of 20-30 us but you're right, there are chips for which the maximum time is
>> specified to be 100 us. Is ok to just raise this to that value? Kyungmin?
>>
>> [...]
>>>>
>>>> +
>>>> +       /* loop over 64 eb batches */
>>>> +       while (len) {
>>>> +               struct erase_info verify_instr = *instr;
>>>> +               verify_instr.addr = addr;
>>>> +               verify_instr.len = 0;
>>>> +
>>>> +               eb_count = 0;
>>>> +
>>>> +               while (len > block_size &&
>>>> +                      eb_count < (MB_ERASE_MAX_BLK_COUNT - 1)) {
>>>>
>>>
>>> According to the manual I have you cannot do a multiblock erase
>>> across a chip boundary, so you must exit this loop at the boundary too.
>>>
>>
>> Ok, does this apply DDP chips only?
>
> Yes, AFAIK. Kyungmin?

Of course, Actually DDP has two chips. can't share the info between chips.
That's the reason to check DDP. :)

Thank you,
Kyungmin Park

Re: [PATCH 1/2] MTD: OneNAND: move erase method to a separate function

[Adrian Hunter]

Korhonen Mika.2 (EXT-Ardites/Oulu) wrote:
> Separate the actual execution of erase to a new function:
> onenand_single_block_erase()
> 
> Signed-off-by: Mika Korhonen <ext-mika.2.korhonen@nokia.com>
> ---

The patch is fine but needs some minor style changes.

Also the patch description should explain that this is in
preparation for adding multiblock erase support.

>  drivers/mtd/onenand/onenand_base.c |  139 +++++++++++++++++++++--------------
>  1 files changed, 83 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
> index 6e82909..ce9f9a0 100644
> --- a/drivers/mtd/onenand/onenand_base.c
> +++ b/drivers/mtd/onenand/onenand_base.c
> @@ -2141,77 +2141,43 @@ static int onenand_block_isbad_nolock(struct mtd_info *mtd, loff_t ofs, int allo
>  }
>  
>  /**
> - * onenand_erase - [MTD Interface] erase block(s)
> + * onenand_single_block_erase - [Internal] erase block(s) using regular erase

onenand_single_block_erase is a poor name because it sounds like it erases just one
block.  I suggest onenand_block_by_block_erase instead.

>   * @param mtd		MTD device structure
>   * @param instr		erase instruction
> + * @param region	erase region
>   *
> - * Erase one ore more blocks
> + * Erase one or more blocks one block at a time
>   */
> -static int onenand_erase(struct mtd_info *mtd, struct erase_info *instr)
> +static int onenand_single_block_erase(struct mtd_info *mtd,
> +				      struct erase_info *instr,
> +				      struct mtd_erase_region_info *region)
>  {
>  	struct onenand_chip *this = mtd->priv;
> -	unsigned int block_size;
>  	loff_t addr = instr->addr;
> -	loff_t len = instr->len;
> -	int ret = 0, i;
> -	struct mtd_erase_region_info *region = NULL;
> +	int len = instr->len;
> +	unsigned int block_size;
>  	loff_t region_end = 0;
> +	int ret = 0;
>  
> -	DEBUG(MTD_DEBUG_LEVEL3, "onenand_erase: start = 0x%012llx, len = %llu\n", (unsigned long long) instr->addr, (unsigned long long) instr->len);
> -
> -	/* Do not allow erase past end of device */
> -	if (unlikely((len + addr) > mtd->size)) {
> -		printk(KERN_ERR "onenand_erase: Erase past end of device\n");
> -		return -EINVAL;
> -	}
> -
> -	if (FLEXONENAND(this)) {
> -		/* Find the eraseregion of this address */
> -		i = flexonenand_region(mtd, addr);
> -		region = &mtd->eraseregions[i];
> -
> +	if (region) {
> +		/* region is set for Flex-OneNAND */
>  		block_size = region->erasesize;
>  		region_end = region->offset + region->erasesize * region->numblocks;
> -
> -		/* Start address within region must align on block boundary.
> -		 * Erase region's start offset is always block start address.
> -		 */
> -		if (unlikely((addr - region->offset) & (block_size - 1))) {
> -			printk(KERN_ERR "onenand_erase: Unaligned address\n");
> -			return -EINVAL;
> -		}
>  	} else {
> -		block_size = 1 << this->erase_shift;
> -
> -		/* Start address must align on block boundary */
> -		if (unlikely(addr & (block_size - 1))) {
> -			printk(KERN_ERR "onenand_erase: Unaligned address\n");
> -			return -EINVAL;
> -		}
> -	}
> -
> -	/* Length must align on block boundary */
> -	if (unlikely(len & (block_size - 1))) {
> -		printk(KERN_ERR "onenand_erase: Length not block aligned\n");
> -		return -EINVAL;
> +		block_size = (1 << this->erase_shift);

Better to pass block_size than to calculate it twice.

>  	}
>  
> -	instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
> -
> -	/* Grab the lock and see if the device is available */
> -	onenand_get_device(mtd, FL_ERASING);
> -
> -	/* Loop throught the pages */
>  	instr->state = MTD_ERASING;
>  
> +	/* Loop through the blocks */
>  	while (len) {
>  		cond_resched();
>  
>  		/* Check if we have a bad block, we do not erase bad blocks */
>  		if (onenand_block_isbad_nolock(mtd, addr, 0)) {
> -			printk (KERN_WARNING "onenand_erase: attempt to erase a bad block at addr 0x%012llx\n", (unsigned long long) addr);
> +			printk(KERN_WARNING "onenand_erase: attempt to erase a bad block at addr 0x%012llx\n", (unsigned long long) addr);

Messages refer to 'onenand_erase' but the function is now onenand_single_block_erase (or onenand_block_by_block_erase if you take my suggestion)

>  			instr->state = MTD_ERASE_FAILED;
> -			goto erase_exit;
> +			return -1;
>  		}
>  
>  		this->command(mtd, ONENAND_CMD_ERASE, addr, block_size);
> @@ -2222,10 +2188,10 @@ static int onenand_erase(struct mtd_info *mtd, struct erase_info *instr)
>  		/* Check, if it is write protected */
>  		if (ret) {
>  			printk(KERN_ERR "onenand_erase: Failed erase, block %d\n",
> -						 onenand_block(this, addr));
> +			       onenand_block(this, addr));
>  			instr->state = MTD_ERASE_FAILED;
>  			instr->fail_addr = addr;
> -			goto erase_exit;
> +			return -1;
>  		}
>  
>  		len -= block_size;
> @@ -2235,24 +2201,85 @@ static int onenand_erase(struct mtd_info *mtd, struct erase_info *instr)
>  			if (!len)
>  				break;
>  			region++;
> -
>  			block_size = region->erasesize;
>  			region_end = region->offset + region->erasesize * region->numblocks;
>  
>  			if (len & (block_size - 1)) {
>  				/* FIXME: This should be handled at MTD partitioning level. */
>  				printk(KERN_ERR "onenand_erase: Unaligned address\n");
> -				goto erase_exit;
> +				return -1;
>  			}
>  		}
> +	}
> +
> +	return 0;
> +}
> +
> +
> +/**
> + * onenand_erase - [MTD Interface] erase block(s)
> + * @param mtd		MTD device structure
> + * @param instr		erase instruction
> + *
> + * Erase one or more blocks
> + */
> +static int onenand_erase(struct mtd_info *mtd, struct erase_info *instr)
> +{
> +	struct onenand_chip *this = mtd->priv;
> +	unsigned int block_size;
> +	loff_t addr = instr->addr;
> +	loff_t len = instr->len;
> +	int ret = 0;
> +	struct mtd_erase_region_info *region = NULL;
> +	loff_t region_offset = 0;
>  
> +	DEBUG(MTD_DEBUG_LEVEL3, "onenand_erase: start = 0x%012llx, len = %llu\n", (unsigned long long) instr->addr, (unsigned long long) instr->len);
> +
> +	/* Do not allow erase past end of device */
> +	if (unlikely((len + addr) > mtd->size)) {
> +		printk(KERN_ERR "onenand_erase: Erase past end of device\n");
> +		return -EINVAL;
>  	}
>  
> -	instr->state = MTD_ERASE_DONE;
> +	if (FLEXONENAND(this)) {
> +		/* Find the eraseregion of this address */
> +		int i = flexonenand_region(mtd, addr);

A blank line is nice after declarations

> +		region = &mtd->eraseregions[i];
>  
> -erase_exit:
> +		block_size = region->erasesize;
>  
> -	ret = instr->state == MTD_ERASE_DONE ? 0 : -EIO;
> +		/* Start address within region must align on block boundary.
> +		 * Erase region's start offset is always block start address.
> +		 */
> +		region_offset = region->offset;
> +	} else {
> +		block_size = 1 << this->erase_shift;
> +	}
> +
> +	/* Start address must align on block boundary */
> +	if (unlikely((addr - region_offset) & (block_size - 1))) {
> +		printk(KERN_ERR "onenand_erase: Unaligned address\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Length must align on block boundary */
> +	if (unlikely(len & (block_size - 1))) {
> +		printk(KERN_ERR "onenand_erase: Length not block aligned\n");
> +		return -EINVAL;
> +	}
> +
> +	instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
> +
> +	/* Grab the lock and see if the device is available */
> +	onenand_get_device(mtd, FL_ERASING);
> +
> +	ret = onenand_single_block_erase(mtd, instr, region);
> +
> +	if (ret) {
> +		ret = -EIO;
> +	} else {
> +		instr->state = MTD_ERASE_DONE;
> +	}

{} not needed, although you could drop the whole thing if
onenand_single_block_erase returned -EIO on error instead
of -1 and set instr->state = MTD_ERASE_DONE before
returning 0.

>  
>  	/* Deselect and wake up anyone waiting on the device */
>  	onenand_release_device(mtd);

Re: [PATCH 1/2] MTD: OneNAND: move erase method to a separate function

[Mika Korhonen]

Hunter Adrian (Nokia-D/Helsinki) wrote:
> Korhonen Mika.2 (EXT-Ardites/Oulu) wrote:
>   
>> Separate the actual execution of erase to a new function:
>> onenand_single_block_erase()
>>
>> Signed-off-by: Mika Korhonen <ext-mika.2.korhonen@nokia.com>
>> ---
>>     
>
> The patch is fine but needs some minor style changes.
>
> Also the patch description should explain that this is in
> preparation for adding multiblock erase support.
>   
Thanks for the feedback, I'll rework the patches and resend them in near 
future.

br,
Mika

[PATCH 0/2] Revised: MTD: OneNAND: multiblock erase support

[Mika Korhonen]

This patch series is a reworked version of

http://lists.infradead.org/pipermail/linux-mtd/2009-September/027138.html
http://lists.infradead.org/pipermail/linux-mtd/2009-September/027139.html
http://lists.infradead.org/pipermail/linux-mtd/2009-September/027140.html

based on comments by Adrian Hunter and Kyungmin Park. This version should
address the issues pointed out by them and apply on the current mtd tree.

The original patch is:
http://lists.infradead.org/pipermail/linux-mtd/2009-June/026130.html

I split the original patch in two for readability: the first part extracts
the execution of erase command to a separate function for easier integration
of different erase method. The second part implements the multiblock erase
function in case multiple blocks are requested to be erased and the chip is
not Flex.

This is useful for flashing applications that need to do their work as
fast as possible. For full 64 eraseblock case the erase speed is up to 30x
faster. (Samsung: 64 MB/s vs 2.1 GB/s, I got 1.4 GB/s on Linux kernel)


Mika Korhonen (2):
  MTD: OneNAND: move erase method to a separate function
  MTD: OneNAND: multiblock erase support

 drivers/mtd/onenand/omap2.c        |   22 +++-
 drivers/mtd/onenand/onenand_base.c |  298 +++++++++++++++++++++++++++++------
 include/linux/mtd/flashchip.h      |    4 +-
 include/linux/mtd/onenand_regs.h   |    2 +
 4 files changed, 270 insertions(+), 56 deletions(-)

[PATCH 1/2] MTD: OneNAND: move erase method to a separate function

[Mika Korhonen]

Separate the actual execution of erase to a new function:
onenand_block_by_block_erase(). This is done in preparation for
the multiblock erase support.

Signed-off-by: Mika Korhonen <ext-mika.2.korhonen@nokia.com>
---
 drivers/mtd/onenand/onenand_base.c |  143 +++++++++++++++++++++---------------
 1 files changed, 84 insertions(+), 59 deletions(-)

diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
index ff66e43..13090f3 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -2141,77 +2141,43 @@ static int onenand_block_isbad_nolock(struct mtd_info *mtd, loff_t ofs, int allo
 }
 
 /**
- * onenand_erase - [MTD Interface] erase block(s)
+ * onenand_block_by_block_erase - [Internal] erase block(s) using regular erase
  * @param mtd		MTD device structure
  * @param instr		erase instruction
+ * @param region	erase region
+ * @param block_size	erase block size
  *
- * Erase one ore more blocks
+ * Erase one or more blocks one block at a time
  */
-static int onenand_erase(struct mtd_info *mtd, struct erase_info *instr)
+static int onenand_block_by_block_erase(struct mtd_info *mtd,
+					struct erase_info *instr,
+					struct mtd_erase_region_info *region,
+					unsigned int block_size)
 {
 	struct onenand_chip *this = mtd->priv;
-	unsigned int block_size;
 	loff_t addr = instr->addr;
-	loff_t len = instr->len;
-	int ret = 0, i;
-	struct mtd_erase_region_info *region = NULL;
+	int len = instr->len;
 	loff_t region_end = 0;
+	int ret = 0;
 
-	DEBUG(MTD_DEBUG_LEVEL3, "onenand_erase: start = 0x%012llx, len = %llu\n", (unsigned long long) instr->addr, (unsigned long long) instr->len);
-
-	/* Do not allow erase past end of device */
-	if (unlikely((len + addr) > mtd->size)) {
-		printk(KERN_ERR "onenand_erase: Erase past end of device\n");
-		return -EINVAL;
-	}
-
-	if (FLEXONENAND(this)) {
-		/* Find the eraseregion of this address */
-		i = flexonenand_region(mtd, addr);
-		region = &mtd->eraseregions[i];
-
-		block_size = region->erasesize;
+	if (region) {
+		/* region is set for Flex-OneNAND */
 		region_end = region->offset + region->erasesize * region->numblocks;
-
-		/* Start address within region must align on block boundary.
-		 * Erase region's start offset is always block start address.
-		 */
-		if (unlikely((addr - region->offset) & (block_size - 1))) {
-			printk(KERN_ERR "onenand_erase: Unaligned address\n");
-			return -EINVAL;
-		}
-	} else {
-		block_size = 1 << this->erase_shift;
-
-		/* Start address must align on block boundary */
-		if (unlikely(addr & (block_size - 1))) {
-			printk(KERN_ERR "onenand_erase: Unaligned address\n");
-			return -EINVAL;
-		}
-	}
-
-	/* Length must align on block boundary */
-	if (unlikely(len & (block_size - 1))) {
-		printk(KERN_ERR "onenand_erase: Length not block aligned\n");
-		return -EINVAL;
 	}
 
-	instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
-
-	/* Grab the lock and see if the device is available */
-	onenand_get_device(mtd, FL_ERASING);
-
-	/* Loop through the blocks */
 	instr->state = MTD_ERASING;
 
+	/* Loop through the blocks */
 	while (len) {
 		cond_resched();
 
 		/* Check if we have a bad block, we do not erase bad blocks */
 		if (onenand_block_isbad_nolock(mtd, addr, 0)) {
-			printk (KERN_WARNING "onenand_erase: attempt to erase a bad block at addr 0x%012llx\n", (unsigned long long) addr);
+			printk(KERN_WARNING "onenand_block_by_block_erase: "
+			       "attempt to erase a bad block at addr "
+			       "0x%012llx\n", (unsigned long long) addr);
 			instr->state = MTD_ERASE_FAILED;
-			goto erase_exit;
+			return -EIO;
 		}
 
 		this->command(mtd, ONENAND_CMD_ERASE, addr, block_size);
@@ -2221,11 +2187,12 @@ static int onenand_erase(struct mtd_info *mtd, struct erase_info *instr)
 		ret = this->wait(mtd, FL_ERASING);
 		/* Check, if it is write protected */
 		if (ret) {
-			printk(KERN_ERR "onenand_erase: Failed erase, block %d\n",
-						 onenand_block(this, addr));
+			printk(KERN_ERR "onenand_block_by_block_erase: "
+			       "Failed erase, block %d\n",
+			       onenand_block(this, addr));
 			instr->state = MTD_ERASE_FAILED;
 			instr->fail_addr = addr;
-			goto erase_exit;
+			return -EIO;
 		}
 
 		len -= block_size;
@@ -2241,24 +2208,82 @@ static int onenand_erase(struct mtd_info *mtd, struct erase_info *instr)
 
 			if (len & (block_size - 1)) {
 				/* FIXME: This should be handled at MTD partitioning level. */
-				printk(KERN_ERR "onenand_erase: Unaligned address\n");
-				goto erase_exit;
+				printk(KERN_ERR "onenand_block_by_block_erase: "
+				       "Unaligned address\n");
+				return -EIO;
 			}
 		}
+	}
+	return 0;
+}
+
+/**
+ * onenand_erase - [MTD Interface] erase block(s)
+ * @param mtd		MTD device structure
+ * @param instr		erase instruction
+ *
+ * Erase one or more blocks
+ */
+static int onenand_erase(struct mtd_info *mtd, struct erase_info *instr)
+{
+	struct onenand_chip *this = mtd->priv;
+	unsigned int block_size;
+	loff_t addr = instr->addr;
+	loff_t len = instr->len;
+	int ret = 0;
+	struct mtd_erase_region_info *region = NULL;
+	loff_t region_offset = 0;
+
+	DEBUG(MTD_DEBUG_LEVEL3, "onenand_erase: start=0x%012llx, len=%llu\n",
+	      (unsigned long long) instr->addr,
+	      (unsigned long long) instr->len);
+
+	/* Do not allow erase past end of device */
+	if (unlikely((len + addr) > mtd->size)) {
+		printk(KERN_ERR "onenand_erase: Erase past end of device\n");
+		return -EINVAL;
+	}
+
+	if (FLEXONENAND(this)) {
+		/* Find the eraseregion of this address */
+		int i = flexonenand_region(mtd, addr);
+
+		region = &mtd->eraseregions[i];
+		block_size = region->erasesize;
+
+		/* Start address within region must align on block boundary.
+		 * Erase region's start offset is always block start address.
+		 */
+		region_offset = region->offset;
+	} else {
+		block_size = 1 << this->erase_shift;
+	}
 
+	/* Start address must align on block boundary */
+	if (unlikely((addr - region_offset) & (block_size - 1))) {
+		printk(KERN_ERR "onenand_erase: Unaligned address\n");
+		return -EINVAL;
 	}
 
-	instr->state = MTD_ERASE_DONE;
+	/* Length must align on block boundary */
+	if (unlikely(len & (block_size - 1))) {
+		printk(KERN_ERR "onenand_erase: Length not block aligned\n");
+		return -EINVAL;
+	}
+
+	instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
 
-erase_exit:
+	/* Grab the lock and see if the device is available */
+	onenand_get_device(mtd, FL_ERASING);
 
-	ret = instr->state == MTD_ERASE_DONE ? 0 : -EIO;
+	ret = onenand_block_by_block_erase(mtd, instr, region, block_size);
 
 	/* Deselect and wake up anyone waiting on the device */
 	onenand_release_device(mtd);
 
 	/* Do call back function */
 	if (!ret)
+		instr->state = MTD_ERASE_DONE;
 		mtd_erase_callback(instr);
 
 	return ret;
-- 
1.6.0.4

Re: [PATCH 1/2] MTD: OneNAND: move erase method to a separate function

[Kyungmin Park]

On Tue, Oct 6, 2009 at 5:55 PM, Mika Korhonen
<ext-mika.2.korhonen@nokia.com> wrote:
> Separate the actual execution of erase to a new function:
> onenand_block_by_block_erase(). This is done in preparation for
> the multiblock erase support.
>
> Signed-off-by: Mika Korhonen <ext-mika.2.korhonen@nokia.com>
> ---
>  drivers/mtd/onenand/onenand_base.c |  143 +++++++++++++++++++++---------------
>  1 files changed, 84 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
> index ff66e43..13090f3 100644
> --- a/drivers/mtd/onenand/onenand_base.c
> +++ b/drivers/mtd/onenand/onenand_base.c
> @@ -2141,77 +2141,43 @@ static int onenand_block_isbad_nolock(struct mtd_info *mtd, loff_t ofs, int allo
>  }
>
>  /**
> - * onenand_erase - [MTD Interface] erase block(s)
> + * onenand_block_by_block_erase - [Internal] erase block(s) using regular erase
>  * @param mtd          MTD device structure
>  * @param instr                erase instruction
> + * @param region       erase region
> + * @param block_size   erase block size
>  *
> - * Erase one ore more blocks
> + * Erase one or more blocks one block at a time
>  */
> -static int onenand_erase(struct mtd_info *mtd, struct erase_info *instr)
> +static int onenand_block_by_block_erase(struct mtd_info *mtd,
> +                                       struct erase_info *instr,
> +                                       struct mtd_erase_region_info *region,
> +                                       unsigned int block_size)
>  {
>        struct onenand_chip *this = mtd->priv;
> -       unsigned int block_size;
>        loff_t addr = instr->addr;
> -       loff_t len = instr->len;
> -       int ret = 0, i;
> -       struct mtd_erase_region_info *region = NULL;
> +       int len = instr->len;
>        loff_t region_end = 0;
> +       int ret = 0;
>
> -       DEBUG(MTD_DEBUG_LEVEL3, "onenand_erase: start = 0x%012llx, len = %llu\n", (unsigned long long) instr->addr, (unsigned long long) instr->len);
> -
> -       /* Do not allow erase past end of device */
> -       if (unlikely((len + addr) > mtd->size)) {
> -               printk(KERN_ERR "onenand_erase: Erase past end of device\n");
> -               return -EINVAL;
> -       }
> -
> -       if (FLEXONENAND(this)) {
> -               /* Find the eraseregion of this address */
> -               i = flexonenand_region(mtd, addr);
> -               region = &mtd->eraseregions[i];
> -
> -               block_size = region->erasesize;
> +       if (region) {
> +               /* region is set for Flex-OneNAND */
>                region_end = region->offset + region->erasesize * region->numblocks;
> -
> -               /* Start address within region must align on block boundary.
> -                * Erase region's start offset is always block start address.
> -                */
> -               if (unlikely((addr - region->offset) & (block_size - 1))) {
> -                       printk(KERN_ERR "onenand_erase: Unaligned address\n");
> -                       return -EINVAL;
> -               }
> -       } else {
> -               block_size = 1 << this->erase_shift;
> -
> -               /* Start address must align on block boundary */
> -               if (unlikely(addr & (block_size - 1))) {
> -                       printk(KERN_ERR "onenand_erase: Unaligned address\n");
> -                       return -EINVAL;
> -               }
> -       }
> -
> -       /* Length must align on block boundary */
> -       if (unlikely(len & (block_size - 1))) {
> -               printk(KERN_ERR "onenand_erase: Length not block aligned\n");
> -               return -EINVAL;
>        }
>
> -       instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
> -
> -       /* Grab the lock and see if the device is available */
> -       onenand_get_device(mtd, FL_ERASING);
> -
> -       /* Loop through the blocks */
>        instr->state = MTD_ERASING;
>
> +       /* Loop through the blocks */
>        while (len) {
>                cond_resched();
>
>                /* Check if we have a bad block, we do not erase bad blocks */
>                if (onenand_block_isbad_nolock(mtd, addr, 0)) {
> -                       printk (KERN_WARNING "onenand_erase: attempt to erase a bad block at addr 0x%012llx\n", (unsigned long long) addr);
> +                       printk(KERN_WARNING "onenand_block_by_block_erase: "
> +                              "attempt to erase a bad block at addr "
> +                              "0x%012llx\n", (unsigned long long) addr);
>                        instr->state = MTD_ERASE_FAILED;
> -                       goto erase_exit;
> +                       return -EIO;
>                }
>
>                this->command(mtd, ONENAND_CMD_ERASE, addr, block_size);
> @@ -2221,11 +2187,12 @@ static int onenand_erase(struct mtd_info *mtd, struct erase_info *instr)
>                ret = this->wait(mtd, FL_ERASING);
>                /* Check, if it is write protected */
>                if (ret) {
> -                       printk(KERN_ERR "onenand_erase: Failed erase, block %d\n",
> -                                                onenand_block(this, addr));
> +                       printk(KERN_ERR "onenand_block_by_block_erase: "
> +                              "Failed erase, block %d\n",
> +                              onenand_block(this, addr));
>                        instr->state = MTD_ERASE_FAILED;
>                        instr->fail_addr = addr;
> -                       goto erase_exit;
> +                       return -EIO;
>                }
>
>                len -= block_size;
> @@ -2241,24 +2208,82 @@ static int onenand_erase(struct mtd_info *mtd, struct erase_info *instr)
>
>                        if (len & (block_size - 1)) {
>                                /* FIXME: This should be handled at MTD partitioning level. */
> -                               printk(KERN_ERR "onenand_erase: Unaligned address\n");
> -                               goto erase_exit;
> +                               printk(KERN_ERR "onenand_block_by_block_erase: "
> +                                      "Unaligned address\n");
> +                               return -EIO;
>                        }
>                }
> +       }
> +       return 0;
> +}
> +
> +/**
> + * onenand_erase - [MTD Interface] erase block(s)
> + * @param mtd          MTD device structure
> + * @param instr                erase instruction
> + *
> + * Erase one or more blocks
> + */
> +static int onenand_erase(struct mtd_info *mtd, struct erase_info *instr)
> +{
> +       struct onenand_chip *this = mtd->priv;
> +       unsigned int block_size;
> +       loff_t addr = instr->addr;
> +       loff_t len = instr->len;
> +       int ret = 0;
> +       struct mtd_erase_region_info *region = NULL;
> +       loff_t region_offset = 0;
> +
> +       DEBUG(MTD_DEBUG_LEVEL3, "onenand_erase: start=0x%012llx, len=%llu\n",
> +             (unsigned long long) instr->addr,
> +             (unsigned long long) instr->len);
> +
> +       /* Do not allow erase past end of device */
> +       if (unlikely((len + addr) > mtd->size)) {
> +               printk(KERN_ERR "onenand_erase: Erase past end of device\n");
> +               return -EINVAL;
> +       }
> +
> +       if (FLEXONENAND(this)) {
> +               /* Find the eraseregion of this address */
> +               int i = flexonenand_region(mtd, addr);
> +
> +               region = &mtd->eraseregions[i];
> +               block_size = region->erasesize;
> +
> +               /* Start address within region must align on block boundary.
> +                * Erase region's start offset is always block start address.
> +                */
> +               region_offset = region->offset;
> +       } else {
> +               block_size = 1 << this->erase_shift;
> +       }

No { } here.

>
> +       /* Start address must align on block boundary */
> +       if (unlikely((addr - region_offset) & (block_size - 1))) {
> +               printk(KERN_ERR "onenand_erase: Unaligned address\n");
> +               return -EINVAL;
>        }
>
> -       instr->state = MTD_ERASE_DONE;
> +       /* Length must align on block boundary */
> +       if (unlikely(len & (block_size - 1))) {
> +               printk(KERN_ERR "onenand_erase: Length not block aligned\n");
> +               return -EINVAL;
> +       }
> +
> +       instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
>
> -erase_exit:
> +       /* Grab the lock and see if the device is available */
> +       onenand_get_device(mtd, FL_ERASING);
>
> -       ret = instr->state == MTD_ERASE_DONE ? 0 : -EIO;
> +       ret = onenand_block_by_block_erase(mtd, instr, region, block_size);
>
>        /* Deselect and wake up anyone waiting on the device */
>        onenand_release_device(mtd);
>
>        /* Do call back function */
>        if (!ret)
> +               instr->state = MTD_ERASE_DONE;
>                mtd_erase_callback(instr);

Are you missing curly braces { } here?

Thank you
Kyungmin Park

Re: [PATCH 1/2] MTD: OneNAND: move erase method to a separate function

[Mika Korhonen]

ext Kyungmin Park wrote:
> On Tue, Oct 6, 2009 at 5:55 PM, Mika Korhonen
> <ext-mika.2.korhonen@nokia.com> wrote:
>   
>> Separate the actual execution of erase to a new function:
>> onenand_block_by_block_erase(). This is done in preparation for
>> the multiblock erase support.
>>
>> Signed-off-by: Mika Korhonen <ext-mika.2.korhonen@nokia.com>
>> ---
>>  drivers/mtd/onenand/onenand_base.c |  143 +++++++++++++++++++++---------------
>>  1 files changed, 84 insertions(+), 59 deletions(-)
>>     
[...]
>>        /* Do call back function */
>>        if (!ret)
>> +               instr->state = MTD_ERASE_DONE;
>>                mtd_erase_callback(instr);
>>     
>
> Are you missing curly braces { } here?
>
> Thank you
> Kyungmin Park
>   
Yes, I am. Thanks!

br
Mika