[RFC/PATCH 2/3] NAND multiple plane feature

[RFC/PATCH 2/3] NAND multiple plane feature

[Alexey Korolev]

As NAND multiple plane architecture assumes simultaneous write/erase of
several pages/blocks at the same time, we have to modify page/eraseblock
sizes and report modified size to upper layers. In other words physical
erase block size/ page size != reported erase block size/ page size.
For example if we have dual plane device we have to extend erase block
size and page size in 2 times. 

Also this patch keep track on bad block management. NAND datasheets
alows us combine only pair of neibour blocks into dual/multiple plane
operations. So if we have multiple plane device and one block is bad,
reported extended block should be considered as bad. 

Signed-off-by: Vasiliy Leonenko <vasiliy.leonenko@gmail.com>
Signed-off-by: Alexey Korolev <akorolev@infradead.org>
--------
 drivers/mtd/nand/diskonchip.c |    8 ++---
 drivers/mtd/nand/nand_base.c  |   60 ++++++++++++++++++++++++++----------------
 drivers/mtd/nand/nand_bbt.c   |   27 ++++++++++++++----
 include/linux/mtd/nand.h      |   14 ++++++---
 4 files changed, 71 insertions(+), 38 deletions(-)

diff -Nurp linux-2.6.24.3/drivers/mtd/nand/diskonchip.c linux-2.6.24.3-dp/drivers/mtd/nand/diskonchip.c
--- linux-2.6.24.3/drivers/mtd/nand/diskonchip.c	2008-02-26 03:20:20.000000000 +0300
+++ linux-2.6.24.3-dp/drivers/mtd/nand/diskonchip.c	2008-05-28 14:18:43.000000000 +0400
@@ -1142,7 +1142,7 @@ static inline int __init nftl_partscan(s
 		mh->FirstPhysicalEUN, mh->FormattedSize,
 		mh->UnitSizeFactor);
 
-	blocks = mtd->size >> this->phys_erase_shift;
+	blocks = mtd->size >> this->erase_shift;
 	maxblocks = min(32768U, mtd->erasesize - psize);
 
 	if (mh->UnitSizeFactor == 0x00) {
@@ -1226,7 +1226,7 @@ static inline int __init inftl_partscan(
 	int end = mtd->size;
 
 	if (inftl_bbt_write)
-		end -= (INFTL_BBT_RESERVED_BLOCKS << this->phys_erase_shift);
+		end -= (INFTL_BBT_RESERVED_BLOCKS << this->erase_shift);
 
 	buf = kmalloc(mtd->writesize, GFP_KERNEL);
 	if (!buf) {
@@ -1264,7 +1264,7 @@ static inline int __init inftl_partscan(
 		((unsigned char *) &mh->OsakVersion)[3] & 0xf,
 		mh->PercentUsed);
 
-	vshift = this->phys_erase_shift + mh->BlockMultiplierBits;
+	vshift = this->erase_shift + mh->BlockMultiplierBits;
 
 	blocks = mtd->size >> vshift;
 	if (blocks > 32768) {
@@ -1272,7 +1272,7 @@ static inline int __init inftl_partscan(
 		goto out;
 	}
 
-	blocks = doc->chips_per_floor << (this->chip_shift - this->phys_erase_shift);
+	blocks = doc->chips_per_floor << (this->chip_shift - this->erase_shift);
 	if (inftl_bbt_write && (blocks > mtd->erasesize)) {
 		printk(KERN_ERR "Writeable BBTs spanning more than one erase block are not yet supported.  FIX ME!\n");
 		goto out;
diff -Nurp linux-2.6.24.3/drivers/mtd/nand/nand_base.c linux-2.6.24.3-dp/drivers/mtd/nand/nand_base.c
--- linux-2.6.24.3/drivers/mtd/nand/nand_base.c	2008-05-28 14:04:59.000000000 +0400
+++ linux-2.6.24.3-dp/drivers/mtd/nand/nand_base.c	2008-05-28 14:43:15.000000000 +0400
@@ -554,7 +554,7 @@ static void nand_command_lp(struct mtd_i
 
 	/* Emulate NAND_CMD_READOOB */
 	if (command == NAND_CMD_READOOB) {
-		column += mtd->writesize;
+		column += chip->page_phys_size;
 		command = NAND_CMD_READ0;
 	}
 
@@ -752,8 +752,8 @@ static int nand_wait(struct mtd_info *mt
 static int nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
 			      uint8_t *buf)
 {
-	chip->read_buf(mtd, buf, mtd->writesize);
-	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
+	chip->read_buf(mtd, buf, chip->page_phys_size);
+	chip->read_buf(mtd, chip->oob_poi, chip->oob_phys_size);
 	return 0;
 }
 
@@ -958,7 +958,7 @@ static int nand_do_read_ops(struct mtd_i
 	int chipnr, page, realpage, col, bytes, aligned;
 	struct nand_chip *chip = mtd->priv;
 	struct mtd_ecc_stats stats;
-	int blkcheck = (1 << (chip->phys_erase_shift - chip->page_shift)) - 1;
+	int blkcheck = (1 << (chip->erase_shift - chip->page_shift)) - 1;
 	int sndcmd = 1;
 	int ret = 0;
 	uint32_t readlen = ops->len;
@@ -1267,7 +1267,7 @@ static int nand_do_read_oob(struct mtd_i
 {
 	int page, realpage, chipnr, sndcmd = 1;
 	struct nand_chip *chip = mtd->priv;
-	int blkcheck = (1 << (chip->phys_erase_shift - chip->page_shift)) - 1;
+	int blkcheck = (1 << (chip->erase_shift - chip->page_shift)) - 1;
 	int readlen = ops->ooblen;
 	int len;
 	uint8_t *buf = ops->oobbuf;
@@ -1402,8 +1402,8 @@ static int nand_read_oob(struct mtd_info
 static void nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
 				const uint8_t *buf)
 {
-	chip->write_buf(mtd, buf, mtd->writesize);
-	chip->write_buf(mtd, chip->oob_poi, mtd->oobsize);
+	chip->write_buf(mtd, buf, chip->page_phys_size);
+	chip->write_buf(mtd, chip->oob_poi, chip->oob_phys_size);
 }
 
 /**
@@ -1655,7 +1655,7 @@ static int nand_do_write_ops(struct mtd_
 
 	realpage = (int)(to >> chip->page_shift);
 	page = realpage & chip->pagemask;
-	blockmask = (1 << (chip->phys_erase_shift - chip->page_shift)) - 1;
+	blockmask = (1 << (chip->erase_shift - chip->page_shift)) - 1;
 
 	/* Invalidate the page cache, when we write to the cached page */
 	if (to <= (chip->pagebuf << chip->page_shift) &&
@@ -1939,13 +1939,13 @@ int nand_erase_nand(struct mtd_info *mtd
 	      (unsigned int)instr->addr, (unsigned int)instr->len);
 
 	/* Start address must align on block boundary */
-	if (instr->addr & ((1 << chip->phys_erase_shift) - 1)) {
+	if (instr->addr & ((1 << chip->erase_shift) - 1)) {
 		DEBUG(MTD_DEBUG_LEVEL0, "nand_erase: Unaligned address\n");
 		return -EINVAL;
 	}
 
 	/* Length must align on block boundary */
-	if (instr->len & ((1 << chip->phys_erase_shift) - 1)) {
+	if (instr->len & ((1 << chip->erase_shift) - 1)) {
 		DEBUG(MTD_DEBUG_LEVEL0, "nand_erase: "
 		      "Length not block aligned\n");
 		return -EINVAL;
@@ -1968,7 +1968,7 @@ int nand_erase_nand(struct mtd_info *mtd
 	chipnr = (int)(instr->addr >> chip->chip_shift);
 
 	/* Calculate pages in each block */
-	pages_per_block = 1 << (chip->phys_erase_shift - chip->page_shift);
+	pages_per_block = 1 << (chip->erase_shift - chip->page_shift);
 
 	/* Select the NAND device */
 	chip->select_chip(mtd, chipnr);
@@ -2045,7 +2045,7 @@ int nand_erase_nand(struct mtd_info *mtd
 			    rewrite_bbt[chipnr] = (page << chip->page_shift);
 
 		/* Increment page address and decrement length */
-		len -= (1 << chip->phys_erase_shift);
+		len -= (1 << chip->erase_shift);
 		page += pages_per_block;
 
 		/* Check, if we cross a chip boundary */
@@ -2269,13 +2269,13 @@ static struct nand_flash_dev *nand_get_f
 		/* Number of planes*/
 		chip->numplanes = 1 << ((planeid >> 2) & 0x03);
 		/* Calc pagesize */
-		mtd->writesize = 1024 << (extid & 0x3);
+		mtd->writesize = (1024 << (extid & 0x3)) * chip->numplanes;
 		extid >>= 2;
 		/* Calc oobsize */
 		mtd->oobsize = (8 << (extid & 0x01)) * (mtd->writesize >> 9);
 		extid >>= 2;
 		/* Calc blocksize. Blocksize is multiples of 64KiB */
-		mtd->erasesize = (64 * 1024) << (extid & 0x03);
+		mtd->erasesize = ((64 * 1024) << (extid & 0x03)) * chip->numplanes;
 		extid >>= 2;
 		/* Get buswidth information */
 		busw = (extid & 0x01) ? NAND_BUSWIDTH_16 : 0;
@@ -2290,6 +2290,8 @@ static struct nand_flash_dev *nand_get_f
 		busw = type->options & NAND_BUSWIDTH_16;
 	}
 
+	chip->oob_phys_size = mtd->oobsize / chip->numplanes;
+
 	/* Try to identify manufacturer */
 	for (maf_idx = 0; nand_manuf_ids[maf_idx].id != 0x0; maf_idx++) {
 		if (nand_manuf_ids[maf_idx].id == *maf_id)
@@ -2312,10 +2314,11 @@ static struct nand_flash_dev *nand_get_f
 
 	/* Calculate the address shift from the page size */
 	chip->page_shift = ffs(mtd->writesize) - 1;
+	chip->page_phys_size = mtd->writesize / chip->numplanes;
 	/* Convert chipsize to number of pages per chip -1. */
 	chip->pagemask = (chip->chipsize >> chip->page_shift) - 1;
 
-	chip->bbt_erase_shift = chip->phys_erase_shift =
+	chip->bbt_erase_shift = chip->erase_shift =
 		ffs(mtd->erasesize) - 1;
 	chip->chip_shift = ffs(chip->chipsize) - 1;
 
@@ -2417,7 +2420,8 @@ int nand_scan_ident(struct mtd_info *mtd
  */
 int nand_scan_tail(struct mtd_info *mtd)
 {
-	int i;
+	int i, j;
+	int oobfree_len;
 	struct nand_chip *chip = mtd->priv;
 
 	if (!(chip->options & NAND_OWN_BUFFERS))
@@ -2432,7 +2436,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 	 * If no default placement scheme is given, select an appropriate one
 	 */
 	if (!chip->ecc.layout) {
-		switch (mtd->oobsize) {
+		switch (chip->oob_phys_size) {
 		case 8:
 			chip->ecc.layout = &nand_oob_8;
 			break;
@@ -2449,9 +2453,6 @@ int nand_scan_tail(struct mtd_info *mtd)
 		}
 	}
 
-	if (!chip->write_page)
-		chip->write_page = nand_write_page;
-
 	/*
 	 * check ECC mode, default to software if 3byte/512byte hardware ECC is
 	 * selected and we have 256 byte pagesize fallback to software ECC
@@ -2539,8 +2540,8 @@ int nand_scan_tail(struct mtd_info *mtd)
 	 * Set the number of read / write steps for one page depending on ECC
 	 * mode
 	 */
-	chip->ecc.steps = mtd->writesize / chip->ecc.size;
-	if(chip->ecc.steps * chip->ecc.size != mtd->writesize) {
+	chip->ecc.steps = chip->page_phys_size / chip->ecc.size;
+	if (chip->ecc.steps * chip->ecc.size != chip->page_phys_size) {
 		printk(KERN_WARNING "Invalid ecc parameters\n");
 		BUG();
 	}
@@ -2593,6 +2594,21 @@ int nand_scan_tail(struct mtd_info *mtd)
 
 	/* propagate ecc.layout to mtd_info */
 	mtd->ecclayout = chip->ecc.layout;
+	if (chip->numplanes > 1) {
+		for (oobfree_len = 0; chip->ecc.layout->oobfree[oobfree_len].length; oobfree_len++)
+			;
+		i = 0;
+		while (i < chip->numplanes) {
+			for (j = 0; j < oobfree_len ; j++) {
+				chip->ecc.layout->oobfree[j + i * oobfree_len].length = chip->ecc.layout->oobfree[j].length;
+				chip->ecc.layout->oobfree[j + i * oobfree_len].offset = chip->ecc.layout->oobfree[j].offset + chip->oob_phys_size * i;
+			}
+			for (j = 0; j < chip->ecc.layout->eccbytes; j++)
+				chip->ecc.layout->eccpos[j + i * chip->ecc.layout->eccbytes] = chip->ecc.layout->eccpos[j] + chip->oob_phys_size * i;
+			i++;
+		}
+		chip->ecc.layout->eccbytes *= chip->numplanes;
+	}
 
 	/* Check, if we should skip the bad block table scan */
 	if (chip->options & NAND_SKIP_BBTSCAN)
diff -Nurp linux-2.6.24.3/drivers/mtd/nand/nand_bbt.c linux-2.6.24.3-dp/drivers/mtd/nand/nand_bbt.c
--- linux-2.6.24.3/drivers/mtd/nand/nand_bbt.c	2008-02-26 03:20:20.000000000 +0300
+++ linux-2.6.24.3-dp/drivers/mtd/nand/nand_bbt.c	2008-05-28 14:57:56.000000000 +0400
@@ -75,12 +75,15 @@
  * pattern area contain 0xff
  *
 */
-static int check_pattern(uint8_t *buf, int len, int paglen, struct nand_bbt_descr *td)
+static int check_pattern(struct mtd_info *mtd, uint8_t *buf, int len, int paglen, struct nand_bbt_descr *td)
 {
 	int i, end = 0;
+	struct nand_chip *this = mtd->priv;
+	int plane_num = 0;
 	uint8_t *p = buf;
 
-	end = paglen + td->offs;
+	do {
+	end = this->page_phys_size + td->offs;
 	if (td->options & NAND_BBT_SCANEMPTY) {
 		for (i = 0; i < end; i++) {
 			if (p[i] != 0xff)
@@ -103,6 +106,10 @@ static int check_pattern(uint8_t *buf, i
 				return -1;
 		}
 	}
+	p += len - end;
+	plane_num++;
+	}
+	while (plane_num < this->numplanes);
 	return 0;
 }
 
@@ -116,16 +123,22 @@ static int check_pattern(uint8_t *buf, i
  * no optional empty check
  *
 */
-static int check_short_pattern(uint8_t *buf, struct nand_bbt_descr *td)
+static int check_short_pattern(struct mtd_info *mtd, uint8_t *buf, struct nand_bbt_descr *td)
 {
 	int i;
 	uint8_t *p = buf;
 
+	struct nand_chip *this = mtd->priv;
+	int plane_num = 0;
 	/* Compare the pattern */
+	do {
 	for (i = 0; i < td->len; i++) {
-		if (p[td->offs + i] != td->pattern[i])
+		if (p[this->oob_phys_size * plane_num + td->offs + i] != td->pattern[i])
 			return -1;
 	}
+	plane_num++;
+	}
+	while (plane_num < this->numplanes);
 	return 0;
 }
 
@@ -318,7 +331,7 @@ static int scan_block_full(struct mtd_in
 		return ret;
 
 	for (j = 0; j < len; j++, buf += scanlen) {
-		if (check_pattern(buf, scanlen, mtd->writesize, bd))
+		if (check_pattern(mtd, buf, scanlen, mtd->writesize, bd))
 			return 1;
 	}
 	return 0;
@@ -349,7 +362,7 @@ static int scan_block_fast(struct mtd_in
 		if (ret)
 			return ret;
 
-		if (check_short_pattern(buf, bd))
+		if (check_short_pattern(mtd, buf, bd))
 			return 1;
 
 		offs += mtd->writesize;
@@ -501,7 +514,7 @@ static int search_bbt(struct mtd_info *m
 
 			/* Read first page */
 			scan_read_raw(mtd, buf, offs, mtd->writesize);
-			if (!check_pattern(buf, scanlen, mtd->writesize, td)) {
+			if (!check_pattern(mtd, buf, scanlen, mtd->writesize, td)) {
 				td->pages[i] = actblock << blocktopage;
 				if (td->options & NAND_BBT_VERSION) {
 					td->version[i] = buf[mtd->writesize + td->veroffs];
diff -Nurp linux-2.6.24.3/include/linux/mtd/nand.h linux-2.6.24.3-dp/include/linux/mtd/nand.h
--- linux-2.6.24.3/include/linux/mtd/nand.h	2008-05-28 14:04:59.000000000 +0400
+++ linux-2.6.24.3-dp/include/linux/mtd/nand.h	2008-05-28 15:02:26.000000000 +0400
@@ -45,8 +45,8 @@ extern void nand_wait_ready(struct mtd_i
  * is supported now. If you add a chip with bigger oobsize/page
  * adjust this accordingly.
  */
-#define NAND_MAX_OOBSIZE	64
-#define NAND_MAX_PAGESIZE	2048
+#define NAND_MAX_OOBSIZE	128
+#define NAND_MAX_PAGESIZE	4096
 
 /*
  * Constants for hardware specific CLE/ALE/NCE function
@@ -332,8 +332,8 @@ struct nand_buffers {
  * @wq:			[INTERN] wait queue to sleep on if a NAND operation is in progress
  * @state:		[INTERN] the current state of the NAND device
  * @oob_poi:		poison value buffer
- * @page_shift:		[INTERN] number of address bits in a page (column address bits)
- * @phys_erase_shift:	[INTERN] number of address bits in a physical eraseblock
+ * @page_shift:		[INTERN] number of address bits in a page reported to upper layer drivers 
+ * @erase_shift:	[INTERN] number of address bits in eraseblock reported to upper layer drivers
  * @bbt_erase_shift:	[INTERN] number of address bits in a bbt entry
  * @chip_shift:		[INTERN] number of address bits in one chip
  * @datbuf:		[INTERN] internal buffer for one page + oob
@@ -344,6 +344,8 @@ struct nand_buffers {
  *			special functionality. See the defines for further explanation
  * @badblockpos:	[INTERN] position of the bad block marker in the oob area
  * @numplanes:		[INTERN] number of ident planes on NAND device
+ * @oob_phys_size:	[INTERN] physical size of NAND oob area 
+ * @page_phys_size:	[INTERN] physical size of NAND page 
  * @cellinfo:		[INTERN] MLC/multichip data from chip ident
  * @numchips:		[INTERN] number of physical chips
  * @chipsize:		[INTERN] the size of one chip for multichip arrays
@@ -390,7 +392,7 @@ struct nand_chip {
 	unsigned int	options;
 
 	int		page_shift;
-	int		phys_erase_shift;
+	int		erase_shift;
 	int		bbt_erase_shift;
 	int		chip_shift;
 	int		numchips;
@@ -401,6 +403,8 @@ struct nand_chip {
 	uint8_t		cellinfo;
 	int		badblockpos;
 	int		numplanes;
+	int		oob_phys_size;
+	int		page_phys_size;
 
 	nand_state_t	state;
 

Re: [RFC/PATCH 2/3] NAND multiple plane feature

[Jörn Engel]

On Wed, 28 May 2008 14:08:01 +0100, Alexey Korolev wrote:
> 
> As NAND multiple plane architecture assumes simultaneous write/erase of
> several pages/blocks at the same time, we have to modify page/eraseblock
> sizes and report modified size to upper layers. In other words physical
> erase block size/ page size != reported erase block size/ page size.
> For example if we have dual plane device we have to extend erase block
> size and page size in 2 times. 

Before actually reading the datasheets (just now) I had hoped that
manufacturers would provide us several independent read/write/erase
units per chip and allow software to deal with each plane as if it was a
seperate chip.  _That_ would have been really useful.  And for NOR
flashes, Intel has already shown how to do it.

But hoping for manufacturers to get it right rarely works - it certainly
didn't work in this case.  As it seems, we can either program two planes
in a weird lock-step process or ignore the feature.  And the lock-step
variant isn't useful for much more than doubling/quadrupling the
erasesize and writesize.  With all the disadvantages that brings. :(

Speaking about the disadvantages, if the dual plane feature is
enabled/disabled across reboots and erase size or write size changes,
we're in for a lot of fun from the filesystem size.  F.e. JFFS2 will
experience data loss when erase size isn't stable.

Jörn

-- 
All models are wrong. Some models are useful.
-- George Box

Re: [RFC/PATCH 2/3] NAND multiple plane feature

[Jamie Lokier]

Jörn Engel wrote:
> But hoping for manufacturers to get it right rarely works - it certainly
> didn't work in this case.  As it seems, we can either program two planes
> in a weird lock-step process or ignore the feature.  And the lock-step
> variant isn't useful for much more than doubling/quadrupling the
> erasesize and writesize.  With all the disadvantages that brings. :(

Did you see if the dual plane feature at least allows erasing the next
block while writing the current one, so you can do continuous
streaming log writes without big pauses?

-- Jamie

Re: [RFC/PATCH 2/3] NAND multiple plane feature

[Jörn Engel]

On Mon, 2 June 2008 12:28:07 +0100, Jamie Lokier wrote:
> 
> Did you see if the dual plane feature at least allows erasing the next
> block while writing the current one, so you can do continuous
> streaming log writes without big pauses?

No, it simply allows the same operation to be done for two seperate
blocks.

Jörn

-- 
If you're willing to restrict the flexibility of your approach,
you can almost always do something better.
-- John Carmack

Re: [RFC/PATCH 2/3] NAND multiple plane feature

[Alexey Korolev]

Hi,

> Before actually reading the datasheets (just now) I had hoped that
> manufacturers would provide us several independent read/write/erase
> units per chip and allow software to deal with each plane as if it was a
> seperate chip.  _That_ would have been really useful.  And for NOR
> flashes, Intel has already shown how to do it.
>
> But hoping for manufacturers to get it right rarely works - it certainly
> didn't work in this case.  As it seems, we can either program two planes
> in a weird lock-step process or ignore the feature.  And the lock-step
> variant isn't useful for much more than doubling/quadrupling the
> erasesize and writesize.  With all the disadvantages that brings. :(
> 
I don't think that situation with NAND would change. This feature is
standardized (see ONFI). We live in imperfect world where everything has
its drawbacks. I think there should be a lot of people who need to
increase write performance and pay increased size for that and many
people for whom erase and write sizes are critical and they could pay
performance in order to save erase/write size. 

It would be nice to let people choose. Adding support of this
feature will make it possible. I believe that current implementation
might be improved more. So I posted the patch. 

We did some performance testing for this feature: we did not find a test
case where JFFS2 on NAND with dual-plane has lower performance than
JFFS2 on NAND without dual-plane features. (note: subpage read feature
was enabled for both cases)

> Speaking about the disadvantages, if the dual plane feature is
> enabled/disabled across reboots and erase size or write size changes,
> we're in for a lot of fun from the filesystem size.  F.e. JFFS2 will
> experience data loss when erase size isn't stable.
> 
What is the use-case for that? Who will need to manage low level feature
across reboots. I can't remember/image anything similar for the case of
cell phones. 

Thanks,
Alexey

-------
But there, everything has its drawbacks, as the man said when his
mother-in-law died, and they came down upon him for the funeral
expenses. 
Jerome K. Jerome

Re: [RFC/PATCH 2/3] NAND multiple plane feature

[Jörn Engel]

On Tue, 3 June 2008 17:57:35 +0100, Alexey Korolev wrote:
>
> We did some performance testing for this feature: we did not find a test
> case where JFFS2 on NAND with dual-plane has lower performance than
> JFFS2 on NAND without dual-plane features. (note: subpage read feature
> was enabled for both cases)

An encouraging result.

> > Speaking about the disadvantages, if the dual plane feature is
> > enabled/disabled across reboots and erase size or write size changes,
> > we're in for a lot of fun from the filesystem size.  F.e. JFFS2 will
> > experience data loss when erase size isn't stable.
>
> What is the use-case for that? Who will need to manage low level feature
> across reboots. I can't remember/image anything similar for the case of
> cell phones. 

There is no sane usecase for it.  It can only cause problems.  So we
should be reasonably confident that noone will change it by accident.
At least not easily.

Jörn

-- 
Prosperity makes friends, adversity tries them.
-- Publilius Syrus

Re: [RFC/PATCH 2/3] NAND multiple plane feature

[Jörn Engel]

On Wed, 28 May 2008 14:08:01 +0100, Alexey Korolev wrote:
>
> @@ -2449,9 +2453,6 @@ int nand_scan_tail(struct mtd_info *mtd)
>  		}
>  	}
>  
> -	if (!chip->write_page)
> -		chip->write_page = nand_write_page;
> -

Why is this removed?

> @@ -2593,6 +2594,21 @@ int nand_scan_tail(struct mtd_info *mtd)
>  
>  	/* propagate ecc.layout to mtd_info */
>  	mtd->ecclayout = chip->ecc.layout;
> +	if (chip->numplanes > 1) {
> +		for (oobfree_len = 0; chip->ecc.layout->oobfree[oobfree_len].length; oobfree_len++)
> +			;

Empty loop body.  And the header doesn't seem to have a side-effect
either.  If it does, it can sure use a comment.

> +		i = 0;
> +		while (i < chip->numplanes) {

		for (i = 0; i < chip->numplanes; i++) ?

> +			for (j = 0; j < oobfree_len ; j++) {
> +				chip->ecc.layout->oobfree[j + i * oobfree_len].length = chip->ecc.layout->oobfree[j].length;
> +				chip->ecc.layout->oobfree[j + i * oobfree_len].offset = chip->ecc.layout->oobfree[j].offset + chip->oob_phys_size * i;

Even if I sound like Andrew Morton, what is the meaning of i and j?
Maybe rename the variable so future readers don't ask that question?
Add a helper variable for chip->ecc.layout->oobfree and I might even be
able to understand what this code does. ;)

> +			}
> +			for (j = 0; j < chip->ecc.layout->eccbytes; j++)
> +				chip->ecc.layout->eccpos[j + i * chip->ecc.layout->eccbytes] = chip->ecc.layout->eccpos[j] + chip->oob_phys_size * i;
> +			i++;
> +		}
> +		chip->ecc.layout->eccbytes *= chip->numplanes;
> +	}
>  
>  	/* Check, if we should skip the bad block table scan */
>  	if (chip->options & NAND_SKIP_BBTSCAN)
> diff -Nurp linux-2.6.24.3/drivers/mtd/nand/nand_bbt.c linux-2.6.24.3-dp/drivers/mtd/nand/nand_bbt.c
> --- linux-2.6.24.3/drivers/mtd/nand/nand_bbt.c	2008-02-26 03:20:20.000000000 +0300
> +++ linux-2.6.24.3-dp/drivers/mtd/nand/nand_bbt.c	2008-05-28 14:57:56.000000000 +0400
> @@ -75,12 +75,15 @@
>   * pattern area contain 0xff
>   *
>  */
> -static int check_pattern(uint8_t *buf, int len, int paglen, struct nand_bbt_descr *td)
> +static int check_pattern(struct mtd_info *mtd, uint8_t *buf, int len, int paglen, struct nand_bbt_descr *td)
>  {
>  	int i, end = 0;
> +	struct nand_chip *this = mtd->priv;
> +	int plane_num = 0;
>  	uint8_t *p = buf;
>  
> -	end = paglen + td->offs;
> +	do {
> +	end = this->page_phys_size + td->offs;

The loop wants indentation.

>  	if (td->options & NAND_BBT_SCANEMPTY) {
>  		for (i = 0; i < end; i++) {
>  			if (p[i] != 0xff)
> @@ -103,6 +106,10 @@ static int check_pattern(uint8_t *buf, i
>  				return -1;
>  		}
>  	}
> +	p += len - end;
> +	plane_num++;
> +	}
> +	while (plane_num < this->numplanes);

	} while (plane_num < this->numplanes);

> -		if (p[td->offs + i] != td->pattern[i])
> +		if (p[this->oob_phys_size * plane_num + td->offs + i] != td->pattern[i])

This is beyond me.  Without context I have no clue what it does.  And
even with context it is hard work to figure it out.  Not good.

My first thought was to move the calculation of
"p[this->oob_phys_size * plane_num + td->offs + i]" to an inline
function.  But I doubt whether that is much better.  Ideas?

>  			return -1;
>  	}
> +	plane_num++;
> +	}
> +	while (plane_num < this->numplanes);

Add indentation, remove newline.

> -#define NAND_MAX_OOBSIZE	64
> -#define NAND_MAX_PAGESIZE	2048
> +#define NAND_MAX_OOBSIZE	128
> +#define NAND_MAX_PAGESIZE	4096

Are there chips with four planes around already?

Jörn

-- 
He who knows that enough is enough will always have enough.
-- Lao Tsu

Re: [RFC/PATCH 2/3] NAND multiple plane feature

[Alexey Korolev]

Hi,

> > -	if (!chip->write_page)
> > -		chip->write_page = nand_write_page;
> > -
> 
> Why is this removed?
>
Oh. May be it would be better to make stand alone patch in order to
clean up NAND subsystem. Using chip->write_page is redundend here. It
is set once it is set for write procedure only. There is no chip read
page call like that. IMHO it is better to use static call here. Please
let me know if you are agree with this. 
> > @@ -2593,6 +2594,21 @@ int nand_scan_tail(struct mtd_info *mtd)
> >  
> >  	/* propagate ecc.layout to mtd_info */
> >  	mtd->ecclayout = chip->ecc.layout;
> > +	if (chip->numplanes > 1) {
> > +			;
> > +		for (oobfree_len = 0; chip->ecc.layout->oobfree[oobfree_len].length; oobfree_len++)
> 
> Empty loop body.  And the header doesn't seem to have a side-effect
> either.  If it does, it can sure use a comment.
This is a very short way to calculate oobfree_len. It increases counter
until chip->ecc.layout->oobfree[_oobfree_len_].length is not 0. Comment
will be added.
> > +		i = 0;
> > +		while (i < chip->numplanes) {
> 
> 		for (i = 0; i < chip->numplanes; i++) ?
>
Oh thanks. I missed this substitution in code clean up. Will be fixed. 
> > +			for (j = 0; j < oobfree_len ; j++) {
> > +				chip->ecc.layout->oobfree[j + i * oobfree_len].length = chip->ecc.layout->oobfree[j].length;
> > +				chip->ecc.layout->oobfree[j + i * oobfree_len].offset = chip->ecc.layout->oobfree[j].offset + chip->oob_phys_size * i;
> 
> Even if I sound like Andrew Morton, what is the meaning of i and j?
> Maybe rename the variable so future readers don't ask that question?
> Add a helper variable for chip->ecc.layout->oobfree and I might even be
> able to understand what this code does. ;)
> 
Making code human readible is a hard task. It seems we need to do some
more work here. I'll send out a possible variant for this. 
> >  */
> > -static int check_pattern(uint8_t *buf, int len, int paglen, struct nand_bbt_descr *td)
> > +static int check_pattern(struct mtd_info *mtd, uint8_t *buf, int len, int paglen, struct nand_bbt_descr *td)
> >  {
> >  	int i, end = 0;
> > +	struct nand_chip *this = mtd->priv;
> > +	int plane_num = 0;
> >  	uint8_t *p = buf;
> >  
> > -	end = paglen + td->offs;
> > +	do {
> > +	end = this->page_phys_size + td->offs;
> 
> The loop wants indentation.
> 
Acked. 

> >  	if (td->options & NAND_BBT_SCANEMPTY) {
> >  		for (i = 0; i < end; i++) {
> >  			if (p[i] != 0xff)
> > @@ -103,6 +106,10 @@ static int check_pattern(uint8_t *buf, i
> >  				return -1;
> >  		}
> >  	}
> > +	p += len - end;
> > +	plane_num++;
> > +	}
> > +	while (plane_num < this->numplanes);
> 
> 	} while (plane_num < this->numplanes);
> 
> > -		if (p[td->offs + i] != td->pattern[i])
> > +		if (p[this->oob_phys_size * plane_num + td->offs + i] != td->pattern[i])
> 
> This is beyond me.  Without context I have no clue what it does.  And
> even with context it is hard work to figure it out.  Not good.
> 
> My first thought was to move the calculation of
> "p[this->oob_phys_size * plane_num + td->offs + i]" to an inline
> function.  But I doubt whether that is much better.  Ideas?
> 
I think it would be better to add some new variables to make it more
clear. We will post a possible resolution for this hard place. 
> >  			return -1;
> >  	}
> 
> > -#define NAND_MAX_OOBSIZE	64
> > -#define NAND_MAX_PAGESIZE	2048
> > +#define NAND_MAX_OOBSIZE	128
> > +#define NAND_MAX_PAGESIZE	4096
> 
> Are there chips with four planes around already?
>
No. AFAIK there are plans only for those chips. I don't know how soon
they will appear, may be next year.


Thanks,
Alexey 

Re: [RFC/PATCH 2/3] NAND multiple plane feature

[Jörn Engel]

On Thu, 5 June 2008 17:58:14 +0100, Alexey Korolev wrote:
> 
> > > -	if (!chip->write_page)
> > > -		chip->write_page = nand_write_page;
> > > -
> > 
> > Why is this removed?
> >
> Oh. May be it would be better to make stand alone patch in order to
> clean up NAND subsystem.

Yes, please. :)

> Using chip->write_page is redundend here. It
> is set once it is set for write procedure only. There is no chip read
> page call like that. IMHO it is better to use static call here. Please
> let me know if you are agree with this. 

Setting it just once is clearly better.  So long as it actually happens
and we don't introduce a bug, I like it.

Jörn

-- 
When I am working on a problem I never think about beauty.  I think
only how to solve the problem.  But when I have finished, if the
solution is not beautiful, I know it is wrong.
-- R. Buckminster Fuller

Re: [RFC/PATCH 2/3] NAND multiple plane feature

[Thomas Gleixner]

On Thu, 5 Jun 2008, Alexey Korolev wrote:
> > > -	if (!chip->write_page)
> > > -		chip->write_page = nand_write_page;
> > > -
> > 
> > Why is this removed?
> >
> Oh. May be it would be better to make stand alone patch in order to
> clean up NAND subsystem. Using chip->write_page is redundend here. It
> is set once it is set for write procedure only. There is no chip read
> page call like that. IMHO it is better to use static call here. Please
> let me know if you are agree with this. 

No, it's definitely not better to use a static call here. Cared to look
at the existing drivers, whether one provides it's own function ?

drivers/mtd/nand/cafe_nand.c:	cafe->nand.write_page = cafe_nand_write_page;

Thanks,
	tglx

Re: [RFC/PATCH 2/3] NAND multiple plane feature

[Thomas Gleixner]

On Wed, 28 May 2008, Alexey Korolev wrote:

> As NAND multiple plane architecture assumes simultaneous write/erase of
> several pages/blocks at the same time, we have to modify page/eraseblock
> sizes and report modified size to upper layers. In other words physical
> erase block size/ page size != reported erase block size/ page size.
>
> For example if we have dual plane device we have to extend erase block
> size and page size in 2 times. 

And why the heck do you have to do all those phys_erase_shift changes ?

mtd->erase_size is completely independent of the nand internal
information already. All it needs is to adjust the erase_size which is
reported to the mtd layer.

> Also this patch keep track on bad block management. NAND datasheets
> alows us combine only pair of neibour blocks into dual/multiple plane
> operations. So if we have multiple plane device and one block is bad,
> reported extended block should be considered as bad. 

Please do this in a separate patch if it's even necessary at all.

> @@ -2269,13 +2269,13 @@ static struct nand_flash_dev *nand_get_f
>  		/* Number of planes*/
>  		chip->numplanes = 1 << ((planeid >> 2) & 0x03);

		planeid is magically declared and initialized, right ?

>  		/* Calc pagesize */
> -		mtd->writesize = 1024 << (extid & 0x3);
> +		mtd->writesize = (1024 << (extid & 0x3)) * chip->numplanes;
>  		extid >>= 2;
>  		/* Calc oobsize */
>  		mtd->oobsize = (8 << (extid & 0x01)) * (mtd->writesize >> 9);
>  		extid >>= 2;
>  		/* Calc blocksize. Blocksize is multiples of 64KiB */
> -		mtd->erasesize = (64 * 1024) << (extid & 0x03);
> +		mtd->erasesize = ((64 * 1024) << (extid & 0x03)) * chip->numplanes;
>  		extid >>= 2;
>  		/* Get buswidth information */
>  		busw = (extid & 0x01) ? NAND_BUSWIDTH_16 : 0;
> @@ -2290,6 +2290,8 @@ static struct nand_flash_dev *nand_get_f
>  		busw = type->options & NAND_BUSWIDTH_16;
>  	}
>  
> +	chip->oob_phys_size = mtd->oobsize / chip->numplanes;

What the heck initializes chip->numplanes in the else path of 

        if (!type->pagesize) {


>  int nand_scan_tail(struct mtd_info *mtd)
>  {
> -	int i;
> +	int i, j;
> +	int oobfree_len;

One line

>  
> -	if (!chip->write_page)
> -		chip->write_page = nand_write_page;
> -

1) Change has nothing to do with the patch description

2) removing this will break existing code

> +	if (chip->numplanes > 1) {


Introduce new feature in a separate patch

> +		for (oobfree_len = 0; chip->ecc.layout->oobfree[oobfree_len].length; oobfree_len++)
> +			;
> +		i = 0;
> +		while (i < chip->numplanes) {
> +			for (j = 0; j < oobfree_len ; j++) {
> +				chip->ecc.layout->oobfree[j + i * oobfree_len].length = chip->ecc.layout->oobfree[j].length;
> +				chip->ecc.layout->oobfree[j + i * oobfree_len].offset = chip->ecc.layout->oobfree[j].offset + chip->oob_phys_size * i;
> +			}
> +			for (j = 0; j < chip->ecc.layout->eccbytes; j++)
> +				chip->ecc.layout->eccpos[j + i * chip->ecc.layout->eccbytes] = chip->ecc.layout->eccpos[j] + chip->oob_phys_size * i;

Screenwidth 280 characters ?

> +			i++;
> +		}
> +		chip->ecc.layout->eccbytes *= chip->numplanes;
> +	}
>  
>  */
> -static int check_pattern(uint8_t *buf, int len, int paglen, struct nand_bbt_descr *td)
> +static int check_pattern(struct mtd_info *mtd, uint8_t *buf, int len, int paglen, struct nand_bbt_descr *td)
>  {
>  	int i, end = 0;
> +	struct nand_chip *this = mtd->priv;
> +	int plane_num = 0;
>
>  	uint8_t *p = buf;
>  
> -	end = paglen + td->offs;
> +	do {
> +	end = this->page_phys_size + td->offs;
>  	if (td->options & NAND_BBT_SCANEMPTY) {
>  		for (i = 0; i < end; i++) {
>  			if (p[i] != 0xff)
> @@ -103,6 +106,10 @@ static int check_pattern(uint8_t *buf, i
>  				return -1;
>  		}
>  	}
> +	p += len - end;
> +	plane_num++;
> +	}
> +	while (plane_num < this->numplanes);
>  	return 0;
>  }

This change is completely useless. If you have to scan a buffer double
the size, then the code can be called twice. Create an inline function
which does the numplanes magic and call that.
  
> @@ -116,16 +123,22 @@ static int check_pattern(uint8_t *buf, i
>   * no optional empty check
>   *
>  */
> -static int check_short_pattern(uint8_t *buf, struct nand_bbt_descr *td)
> +static int check_short_pattern(struct mtd_info *mtd, uint8_t *buf, struct nand_bbt_descr *td)
>  {
>  	int i;
>  	uint8_t *p = buf;
>  
> +	struct nand_chip *this = mtd->priv;
> +	int plane_num = 0;
>  	/* Compare the pattern */
> +	do {
>  	for (i = 0; i < td->len; i++) {
> -		if (p[td->offs + i] != td->pattern[i])
> +		if (p[this->oob_phys_size * plane_num + td->offs + i] != td->pattern[i])
>  			return -1;
>  	}
> +	plane_num++;
> +	}
> +	while (plane_num < this->numplanes);
>  	return 0;
>  }

Same here
 
> @@ -318,7 +331,7 @@ static int scan_block_full(struct mtd_in
>  		return ret;
>  
>  	for (j = 0; j < len; j++, buf += scanlen) {
> -		if (check_pattern(buf, scanlen, mtd->writesize, bd))
> +		if (check_pattern(mtd, buf, scanlen, mtd->writesize, bd))
>  			return 1;
>  	}
>  	return 0;
> @@ -349,7 +362,7 @@ static int scan_block_fast(struct mtd_in
>  		if (ret)
>  			return ret;
>  
> -		if (check_short_pattern(buf, bd))
> +		if (check_short_pattern(mtd, buf, bd))
>  			return 1;
>  
>  		offs += mtd->writesize;
> @@ -501,7 +514,7 @@ static int search_bbt(struct mtd_info *m
>  
>  			/* Read first page */
>  			scan_read_raw(mtd, buf, offs, mtd->writesize);
> -			if (!check_pattern(buf, scanlen, mtd->writesize, td)) {
> +			if (!check_pattern(mtd, buf, scanlen, mtd->writesize, td)) {

And what puts the BBT magic pattern into both erase blocks ?

Thanks,
	tglx

Re: [RFC/PATCH 2/3] NAND multiple plane feature

[Jörn Engel]

On Tue, 3 June 2008 17:57:35 +0100, Alexey Korolev wrote:
> 
> We did some performance testing for this feature: we did not find a test
> case where JFFS2 on NAND with dual-plane has lower performance than
> JFFS2 on NAND without dual-plane features. (note: subpage read feature
> was enabled for both cases)

Maybe I have a testcase for you.  The "evil workload".

1. Write a file until you get -ENOSPC.
2. Replace 1000 page-sized page-aligned blocks at random offsets.

The time for 2 should roughly double with bigger eraseblocks.

[ And yes, I originally got this testcase from you. ;) ]

Jörn

-- 
Time? What's that? Time is only worth what you do with it.
-- Theo de Raadt

Re: [RFC/PATCH 2/3] NAND multiple plane feature

[Alexey Korolev]

Hi,
> On Thu, 5 Jun 2008, Alexey Korolev wrote:
> > > > -	if (!chip->write_page)
> > > > -		chip->write_page = nand_write_page;
> > > > -
> > > 
> > > Why is this removed?
> > >
> > Oh. May be it would be better to make stand alone patch in order to
> > clean up NAND subsystem. Using chip->write_page is redundend here. It
> > is set once it is set for write procedure only. There is no chip read
> > page call like that. IMHO it is better to use static call here. Please
> > let me know if you are agree with this. 
> 
> No, it's definitely not better to use a static call here. Cared to look
> at the existing drivers, whether one provides it's own function ?
> 
> drivers/mtd/nand/cafe_nand.c:	cafe->nand.write_page = cafe_nand_write_page;
>
It make sence to remove because of:
1. nand_chip structure looks ugly with that function. It contains write_page but it does not contain read_page.
2. existence of such redefinition is redundend. There is definetely no need to redefine it. 

The only reasignment of write_page call in whole nand subsystem exists in cafe driver. 
But the purpose of this reasigment looks rather unclear.
Try to find at least one line difference between cafe_nand_write_page and nand_write_page. I wonder how this code get included ?

Thanks,
Alexey


-------
static int cafe_nand_write_page(struct mtd_info *mtd, struct nand_chip *chip,
				const uint8_t *buf, int page, int cached, int raw)
{
	int status;

	chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);

	if (unlikely(raw))
		chip->ecc.write_page_raw(mtd, chip, buf);
	else
		chip->ecc.write_page(mtd, chip, buf);

	/*
	 * Cached progamming disabled for now, Not sure if its worth the
	 * trouble. The speed gain is not very impressive. (2.3->2.6Mib/s)
	 */
	cached = 0;

	if (!cached || !(chip->options & NAND_CACHEPRG)) {

		chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
		status = chip->waitfunc(mtd, chip);
		/*
		 * See if operation failed and additional status checks are
		 * available
		 */
		if ((status & NAND_STATUS_FAIL) && (chip->errstat))
			status = chip->errstat(mtd, chip, FL_WRITING, status,
					       page);

		if (status & NAND_STATUS_FAIL)
			return -EIO;
	} else {
		chip->cmdfunc(mtd, NAND_CMD_CACHEDPROG, -1, -1);
		status = chip->waitfunc(mtd, chip);
	}

#ifdef CONFIG_MTD_NAND_VERIFY_WRITE
	/* Send command to read back the data */
	chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);

	if (chip->verify_buf(mtd, buf, mtd->writesize))
		return -EIO;
#endif
	return 0;
}
------

static int nand_write_page(struct mtd_info *mtd, struct nand_chip *chip,
			   const uint8_t *buf, int page, int cached, int raw)
{
	int status;

	chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);

	if (unlikely(raw))
		chip->ecc.write_page_raw(mtd, chip, buf);
	else
		chip->ecc.write_page(mtd, chip, buf);

	/*
	 * Cached progamming disabled for now, Not sure if its worth the
	 * trouble. The speed gain is not very impressive. (2.3->2.6Mib/s)
	 */
	cached = 0;

	if (!cached || !(chip->options & NAND_CACHEPRG)) {

		chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
		status = chip->waitfunc(mtd, chip);
		/*
		 * See if operation failed and additional status checks are
		 * available
		 */
		if ((status & NAND_STATUS_FAIL) && (chip->errstat))
			status = chip->errstat(mtd, chip, FL_WRITING, status,
					       page);

		if (status & NAND_STATUS_FAIL)
			return -EIO;
	} else {
		chip->cmdfunc(mtd, NAND_CMD_CACHEDPROG, -1, -1);
		status = chip->waitfunc(mtd, chip);
	}

#ifdef CONFIG_MTD_NAND_VERIFY_WRITE
	/* Send command to read back the data */
	chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);

	if (chip->verify_buf(mtd, buf, mtd->writesize))
		return -EIO;
#endif
	return 0;
}

Re: [RFC/PATCH 2/3] NAND multiple plane feature

[Thomas Gleixner]

Hi,

Can you please use a sane mail client, which does line breaks around
column 78 ?

On Fri, 6 Jun 2008, Alexey Korolev wrote:
> Hi,
> > On Thu, 5 Jun 2008, Alexey Korolev wrote:
> > > > > -	if (!chip->write_page)
> > > > > -		chip->write_page = nand_write_page;
> > > > > -
> > > > 
> > > > Why is this removed?
> > > >
> > > Oh. May be it would be better to make stand alone patch in order to
> > > clean up NAND subsystem. Using chip->write_page is redundend here. It
> > > is set once it is set for write procedure only. There is no chip read
> > > page call like that. IMHO it is better to use static call here. Please
> > > let me know if you are agree with this. 
> > 
> > No, it's definitely not better to use a static call here. Cared to look
> > at the existing drivers, whether one provides it's own function ?
> > 
> > drivers/mtd/nand/cafe_nand.c:	cafe->nand.write_page = cafe_nand_write_page;
> >
> It make sence to remove because of:
> 1. nand_chip structure looks ugly with that function. It contains
  write_page but it does not contain read_page.

Oh yeah, "looks ugly" is definitely a perfect technical reason.

> 2. existence of such redefinition is redundend. There is definetely
> no need to redefine it.

-ENOPARSE

> The only reasignment of write_page call in whole nand subsystem
> exists in cafe driver.  But the purpose of this reasigment looks
> rather unclear.

> Try to find at least one line difference between
> cafe_nand_write_page and nand_write_page. I wonder how this code get
> included ?

You need to check the history why this is the case. And if it is the
same then a separate patch needs to be provide which removes this.

Thanks,
	tglx

Re: [RFC/PATCH 2/3] NAND multiple plane feature

[Alexey Korolev]

Hi,

> > > > > > -	if (!chip->write_page)
> > > > > > -		chip->write_page = nand_write_page;
> > > > > > -
> > > > > 
> > > > > Why is this removed?
> > > > >
> > > > Oh. May be it would be better to make stand alone patch in order to
> > > > clean up NAND subsystem. Using chip->write_page is redundend here. It
> > > > is set once it is set for write procedure only. There is no chip read
> > > > page call like that. IMHO it is better to use static call here. Please
> > > > let me know if you are agree with this. 
> > > 
> > > No, it's definitely not better to use a static call here. Cared to look
> > > at the existing drivers, whether one provides it's own function ?
> > > 
> > > drivers/mtd/nand/cafe_nand.c:	cafe->nand.write_page = cafe_nand_write_page;
> > >
> > It make sence to remove because of:
> > 1. nand_chip structure looks ugly with that function. It contains
>   write_page but it does not contain read_page.
> 
> Oh yeah, "looks ugly" is definitely a perfect technical reason.
> 
> > 2. existence of such redefinition is redundend. There is definetely
> > no need to redefine it.
> 
> -ENOPARSE
>
Ok. Can you show me a reason why write page
should be in nand_chip structure? Please consider the fact that nobody redefines
write_page (cafe_nand_write_page and nand_write_page are identical). 

Re: [RFC/PATCH 2/3] NAND multiple plane feature

[Alexey Korolev]

Hi,

> On Tue, 3 June 2008 17:57:35 +0100, Alexey Korolev wrote:
> > 
> > We did some performance testing for this feature: we did not find a test
> > case where JFFS2 on NAND with dual-plane has lower performance than
> > JFFS2 on NAND without dual-plane features. (note: subpage read feature
> > was enabled for both cases)
> 
> Maybe I have a testcase for you.  The "evil workload".
> 
> 1. Write a file until you get -ENOSPC.
> 2. Replace 1000 page-sized page-aligned blocks at random offsets.
> 
> The time for 2 should roughly double with bigger eraseblocks.
> 
> [ And yes, I originally got this testcase from you. ;) ]
We just tried to check your scenario. It is not
possible to overwrite something if JFFS2 returned -ENOSPC until
something got deleted. 

In fact our test suite do not include this test case. We have another.
Write file1 at about 5% of whole space, write file2 for rest of space.
Delete file1. Do step 2. 
In this not so good case we got the following numbers:

single-plane:
73 sec 424541 usec

dual-plane:
69 sec 428637 usec

Thanks,
Alexey 

Re: [RFC/PATCH 2/3] NAND multiple plane feature

[Thomas Gleixner]

On Fri, 6 Jun 2008, Alexey Korolev wrote:
> > > 2. existence of such redefinition is redundend. There is definetely
> > > no need to redefine it.
> > 
> > -ENOPARSE
> >
> Ok. Can you show me a reason why write page
> should be in nand_chip structure? Please consider the fact that nobody redefines
> write_page (cafe_nand_write_page and nand_write_page are identical). 

That's not the point. If these functions are the same then of course
there is no need to have it duplicated, but just silently deleting
stuff in a (according to the changelog) unnrelated patch w/o fixing up
what breaks is not how it works.

You want others to review your changes, then please provide them in a
form which do not cause the reviewer to figure out that the function
in cafe.c is identical.

It would have been entirely clear when the patch series would have
contained:

[1/n] remove duplicated write_page function from cafe.c
[2/n] remove now unused write_page function pointer from struct nand_chip

I have only restricted time to do such reviews and I definitely have
no interest to investigate why the heck a change which is completely
unrelated might be correct or useful.

Thanks,

	tglx

Re: [RFC/PATCH 2/3] NAND multiple plane feature

[Alexey Korolev]

On Fri, 6 Jun 2008, Thomas Gleixner wrote:

> On Fri, 6 Jun 2008, Alexey Korolev wrote:
> > > > 2. existence of such redefinition is redundend. There is definetely
> > > > no need to redefine it.
> > > 
> > > -ENOPARSE
> > >
> > Ok. Can you show me a reason why write page
> > should be in nand_chip structure? Please consider the fact that nobody redefines
> > write_page (cafe_nand_write_page and nand_write_page are identical). 
> 
> That's not the point. If these functions are the same then of course
> there is no need to have it duplicated, but just silently deleting
> stuff in a (according to the changelog) unnrelated patch w/o fixing up
> what breaks is not how it works.
Good this was a thing I wanted to know. Now I don't need to worry if I
can remove write_page from nand_chip and cafe.c
> 
> You want others to review your changes, then please provide them in a
> form which do not cause the reviewer to figure out that the function
> in cafe.c is identical.
> 
I didn't know it as well before today. Thanks for catching a potential
problem with nand_cafe. 
> It would have been entirely clear when the patch series would have
> contained:
> 
> [1/n] remove duplicated write_page function from cafe.c
> [2/n] remove now unused write_page function pointer from struct nand_chip
>
> I have only restricted time to do such reviews and I definitely have
> no interest to investigate why the heck a change which is completely
> unrelated might be correct or useful.


Thanks,
Alexey

Re: [RFC/PATCH 2/3] NAND multiple plane feature

[Jörn Engel]

On Fri, 6 June 2008 15:01:05 +0100, Alexey Korolev wrote:
>
> We just tried to check your scenario. It is not
> possible to overwrite something if JFFS2 returned -ENOSPC until
> something got deleted. 
> 
> In fact our test suite do not include this test case. We have another.
> Write file1 at about 5% of whole space, write file2 for rest of space.
> Delete file1. Do step 2. 
> In this not so good case we got the following numbers:
> 
> single-plane:
> 73 sec 424541 usec
> 
> dual-plane:
> 69 sec 428637 usec

An improvement instead of the expected regression.  Impressive.

Jörn

-- 
tglx1 thinks that joern should get a (TM) for "Thinking Is Hard"
-- Thomas Gleixner

Re: [RFC/PATCH 2/3] NAND multiple plane feature

[Alexey Korolev]

Hi,
> 
> > As NAND multiple plane architecture assumes simultaneous write/erase of
> > several pages/blocks at the same time, we have to modify page/eraseblock
> > sizes and report modified size to upper layers. In other words physical
> > erase block size/ page size != reported erase block size/ page size.
> >
> > For example if we have dual plane device we have to extend erase block
> > size and page size in 2 times. 
> 
> And why the heck do you have to do all those phys_erase_shift changes ?
> 
> mtd->erase_size is completely independent of the nand internal
> information already. All it needs is to adjust the erase_size which is
> reported to the mtd layer.
> 

erase_shift is additional variable to handle cases when we need to
operate bit-shift representation of erase size. It makes sence since it
is used often in nand_base. 
Since there are many cases when we need to have bit-shift representation
for extended erase-size we decided to keep variable responsible for
extended erase size. 

> > Also this patch keep track on bad block management. NAND datasheets
> > alows us combine only pair of neibour blocks into dual/multiple plane
> > operations. So if we have multiple plane device and one block is bad,
> > reported extended block should be considered as bad. 
> 
> Please do this in a separate patch if it's even necessary at all.
> 
> > @@ -2269,13 +2269,13 @@ static struct nand_flash_dev *nand_get_f
> >  		/* Number of planes*/
> >  		chip->numplanes = 1 << ((planeid >> 2) & 0x03);
> 
> 		planeid is magically declared and initialized, right ?
> 
In patch 1/3
> >  		/* Calc pagesize */
> > -		mtd->writesize = 1024 << (extid & 0x3);
> > +		mtd->writesize = (1024 << (extid & 0x3)) * chip->numplanes;
> >  		extid >>= 2;
> >  		/* Calc oobsize */
> >  		mtd->oobsize = (8 << (extid & 0x01)) * (mtd->writesize >> 9);
> >  		extid >>= 2;
> >  		/* Calc blocksize. Blocksize is multiples of 64KiB */
> > -		mtd->erasesize = (64 * 1024) << (extid & 0x03);
> > +		mtd->erasesize = ((64 * 1024) << (extid & 0x03)) * chip->numplanes;
> >  		extid >>= 2;
> >  		/* Get buswidth information */
> >  		busw = (extid & 0x01) ? NAND_BUSWIDTH_16 : 0;
> > @@ -2290,6 +2290,8 @@ static struct nand_flash_dev *nand_get_f
> >  		busw = type->options & NAND_BUSWIDTH_16;
> >  	}
> >  
> > +	chip->oob_phys_size = mtd->oobsize / chip->numplanes;
> 
> What the heck initializes chip->numplanes in the else path of 
> 
>         if (!type->pagesize) {
> 
> 
Correct, it will be fixed. 
> >  int nand_scan_tail(struct mtd_info *mtd)
> >  {
> > -	int i;
> > +	int i, j;
> > +	int oobfree_len;
> 
> One line
> 
Thanks
> >  			if (p[i] != 0xff)
> > @@ -103,6 +106,10 @@ static int check_pattern(uint8_t *buf, i
> >  				return -1;
> >  		}
> >  	}
> > +	p += len - end;
> > +	plane_num++;
> > +	}
> > +	while (plane_num < this->numplanes);
> >  	return 0;
> >  }
> 
> This change is completely useless. If you have to scan a buffer double
> the size, then the code can be called twice. Create an inline function
> which does the numplanes magic and call that.
Acked. It is already discussed with Joern. Code multiplication will be
removed. 
> > @@ -501,7 +514,7 @@ static int search_bbt(struct mtd_info *m
> >  
> >  			/* Read first page */
> >  			scan_read_raw(mtd, buf, offs, mtd->writesize);
> > -			if (!check_pattern(buf, scanlen, mtd->writesize, td)) {
> > +			if (!check_pattern(mtd, buf, scanlen, mtd->writesize, td)) {
> 
> And what puts the BBT magic pattern into both erase blocks ?
>
BBT operates extended blocks, no need to put pattern into each of both blocks.

Thanks,
Alexey