[PATCH 0/3] mtd: rawnand: Support bad block markers in first, second or last page

[PATCH 0/3] mtd: rawnand: Support bad block markers in first, second or last page

[Schrempf Frieder]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

Currently supported bad block marker positions within the block are:
* in first page only
* in last page only
* in first or second page

Some ESMT NANDs are known to have been shipped by the manufacturer
with bad block markers in the first or last page, instead of the
first or second page.

Also the datasheets for Cypress/Spansion/AMD NANDs claim that the
first, second *and* last page needs to be checked.

Therefore we make it possible to set NAND_BBT_SCAN2NDPAGE and
NAND_BBT_SCANLASTPAGE at the same time to scan/set all three pages.

This series also contains patches for AMD/Spansion/Cypress and ESMT
chips to enable both flags at the same time.

Frieder Schrempf (3):
  mtd: rawnand: Support bad block markers in first, second or last page
  mtd: rawnand: ESMT: Also use the last page for bad block markers
  mtd: rawnand: AMD: Also use the last page for bad block markers

 drivers/mtd/nand/raw/internals.h |  1 +
 drivers/mtd/nand/raw/nand_amd.c  |  8 +++-
 drivers/mtd/nand/raw/nand_base.c | 72 ++++++++++++++++++++++++++---------
 drivers/mtd/nand/raw/nand_bbt.c  | 30 +++++++--------
 drivers/mtd/nand/raw/nand_esmt.c |  9 ++++-
 5 files changed, 83 insertions(+), 37 deletions(-)

-- 
2.7.4

[PATCH 1/3] mtd: rawnand: Support bad block markers in first, second or last page

[Schrempf Frieder]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

Currently supported bad block marker positions within the block are:
* in first page only
* in last page only
* in first or second page

Some ESMT NANDs are known to have been shipped by the manufacturer
with bad block markers in the first or last page, instead of the
first or second page.

Also the datasheets for Cypress/Spansion/AMD NANDs claim that the
first, second *and* last page needs to be checked.

Therefore we make it possible to set NAND_BBT_SCAN2NDPAGE and
NAND_BBT_SCANLASTPAGE at the same time to scan/set all three pages.

To simplify the code, the logic to evaluate the flags is moved to a
a new function nand_bbm_page_offset().

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/mtd/nand/raw/internals.h |  1 +
 drivers/mtd/nand/raw/nand_base.c | 72 ++++++++++++++++++++++++++---------
 drivers/mtd/nand/raw/nand_bbt.c  | 30 +++++++--------
 3 files changed, 68 insertions(+), 35 deletions(-)

diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h
index 04c2cf7..8e4b168 100644
--- a/drivers/mtd/nand/raw/internals.h
+++ b/drivers/mtd/nand/raw/internals.h
@@ -76,6 +76,7 @@ extern const struct nand_manufacturer_ops toshiba_nand_manuf_ops;
 
 /* Core functions */
 const struct nand_manufacturer *nand_get_manufacturer(u8 id);
+int nand_bbm_page_offset(struct nand_chip *chip, int index);
 int nand_markbad_bbm(struct nand_chip *chip, loff_t ofs);
 int nand_erase_nand(struct nand_chip *chip, struct erase_info *instr,
 		    int allowbbt);
diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 71050a0..388d9ed 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -253,6 +253,45 @@ static void nand_release_device(struct mtd_info *mtd)
 }
 
 /**
+ * nand_bbm_page_offset - Get the page offsets for bad block markers
+ * @chip: NAND chip object
+ * @index: Index for the page offset
+ *
+ * Returns an integer that corresponds to the page offset within a block, for
+ * a page that is used to store bad block markers. If no more page offsets are
+ * available, -1 is returned.
+ */
+int nand_bbm_page_offset(struct nand_chip *chip, int index)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	int last_page = ((mtd->erasesize - mtd->writesize) >>
+			 chip->page_shift) & chip->pagemask;
+
+	switch (index) {
+	case 0:
+		if ((chip->bbt_options & NAND_BBT_SCANLASTPAGE) &&
+		    !(chip->bbt_options & NAND_BBT_SCAN2NDPAGE))
+			return last_page;
+		else
+			return 0;
+		break;
+	case 1:
+		if (chip->bbt_options & NAND_BBT_SCAN2NDPAGE)
+			return 1;
+		break;
+	case 2:
+		if ((chip->bbt_options & NAND_BBT_SCANLASTPAGE) &&
+		    (chip->bbt_options & NAND_BBT_SCAN2NDPAGE))
+			return last_page;
+		break;
+	default:
+		break;
+	}
+
+	return -1;
+}
+
+/**
  * nand_block_bad - [DEFAULT] Read bad block marker from the chip
  * @chip: NAND chip object
  * @ofs: offset from device start
@@ -261,18 +300,14 @@ static void nand_release_device(struct mtd_info *mtd)
  */
 static int nand_block_bad(struct nand_chip *chip, loff_t ofs)
 {
-	struct mtd_info *mtd = nand_to_mtd(chip);
-	int page, page_end, res;
+	int page_offset, i = 0;
+	int res, first_page = (int)(ofs >> chip->page_shift) & chip->pagemask;
 	u8 bad;
 
-	if (chip->bbt_options & NAND_BBT_SCANLASTPAGE)
-		ofs += mtd->erasesize - mtd->writesize;
+	page_offset = nand_bbm_page_offset(chip, 0);
 
-	page = (int)(ofs >> chip->page_shift) & chip->pagemask;
-	page_end = page + (chip->bbt_options & NAND_BBT_SCAN2NDPAGE ? 2 : 1);
-
-	for (; page < page_end; page++) {
-		res = chip->ecc.read_oob(chip, page);
+	do {
+		res = chip->ecc.read_oob(chip, first_page + page_offset);
 		if (res < 0)
 			return res;
 
@@ -284,7 +319,9 @@ static int nand_block_bad(struct nand_chip *chip, loff_t ofs)
 			res = hweight8(bad) < chip->badblockbits;
 		if (res)
 			return res;
-	}
+
+		page_offset = nand_bbm_page_offset(chip, ++i);
+	} while (page_offset != -1);
 
 	return 0;
 }
@@ -303,7 +340,7 @@ static int nand_default_block_markbad(struct nand_chip *chip, loff_t ofs)
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	struct mtd_oob_ops ops;
 	uint8_t buf[2] = { 0, 0 };
-	int ret = 0, res, i = 0;
+	int ret = 0, res, i = 0, page_offset;
 
 	memset(&ops, 0, sizeof(ops));
 	ops.oobbuf = buf;
@@ -316,17 +353,16 @@ static int nand_default_block_markbad(struct nand_chip *chip, loff_t ofs)
 	}
 	ops.mode = MTD_OPS_PLACE_OOB;
 
-	/* Write to first/last page(s) if necessary */
-	if (chip->bbt_options & NAND_BBT_SCANLASTPAGE)
-		ofs += mtd->erasesize - mtd->writesize;
+	page_offset = nand_bbm_page_offset(chip, 0);
+
 	do {
-		res = nand_do_write_oob(mtd, ofs, &ops);
+		res = nand_do_write_oob(mtd, ofs + page_offset * mtd->writesize,
+					&ops);
 		if (!ret)
 			ret = res;
 
-		i++;
-		ofs += mtd->writesize;
-	} while ((chip->bbt_options & NAND_BBT_SCAN2NDPAGE) && i < 2);
+		page_offset = nand_bbm_page_offset(chip, ++i);
+	} while (page_offset != -1);
 
 	return ret;
 }
diff --git a/drivers/mtd/nand/raw/nand_bbt.c b/drivers/mtd/nand/raw/nand_bbt.c
index 98a8268..b1424b3 100644
--- a/drivers/mtd/nand/raw/nand_bbt.c
+++ b/drivers/mtd/nand/raw/nand_bbt.c
@@ -412,10 +412,11 @@ static void read_abs_bbts(struct mtd_info *mtd, uint8_t *buf,
 
 /* Scan a given block partially */
 static int scan_block_fast(struct mtd_info *mtd, struct nand_bbt_descr *bd,
-			   loff_t offs, uint8_t *buf, int numpages)
+			   loff_t offs, uint8_t *buf)
 {
+	struct nand_chip *chip = mtd_to_nand(mtd);
 	struct mtd_oob_ops ops;
-	int j, ret;
+	int i = 0, ret, page_offset;
 
 	ops.ooblen = mtd->oobsize;
 	ops.oobbuf = buf;
@@ -423,12 +424,15 @@ static int scan_block_fast(struct mtd_info *mtd, struct nand_bbt_descr *bd,
 	ops.datbuf = NULL;
 	ops.mode = MTD_OPS_PLACE_OOB;
 
-	for (j = 0; j < numpages; j++) {
+	page_offset = nand_bbm_page_offset(chip, 0);
+
+	do {
 		/*
 		 * Read the full oob until read_oob is fixed to handle single
 		 * byte reads for 16 bit buswidth.
 		 */
-		ret = mtd_read_oob(mtd, offs, &ops);
+		ret = mtd_read_oob(mtd, offs + page_offset * mtd->writesize,
+				   &ops);
 		/* Ignore ECC errors when checking for BBM */
 		if (ret && !mtd_is_bitflip_or_eccerr(ret))
 			return ret;
@@ -436,8 +440,9 @@ static int scan_block_fast(struct mtd_info *mtd, struct nand_bbt_descr *bd,
 		if (check_short_pattern(buf, bd))
 			return 1;
 
-		offs += mtd->writesize;
-	}
+		page_offset = nand_bbm_page_offset(chip, ++i);
+	} while (page_offset != -1);
+
 	return 0;
 }
 
@@ -456,17 +461,11 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf,
 	struct nand_bbt_descr *bd, int chip)
 {
 	struct nand_chip *this = mtd_to_nand(mtd);
-	int i, numblocks, numpages;
-	int startblock;
+	int i, numblocks, startblock;
 	loff_t from;
 
 	pr_info("Scanning device for bad blocks\n");
 
-	if (bd->options & NAND_BBT_SCAN2NDPAGE)
-		numpages = 2;
-	else
-		numpages = 1;
-
 	if (chip == -1) {
 		numblocks = mtd->size >> this->bbt_erase_shift;
 		startblock = 0;
@@ -483,15 +482,12 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf,
 		from = (loff_t)startblock << this->bbt_erase_shift;
 	}
 
-	if (this->bbt_options & NAND_BBT_SCANLASTPAGE)
-		from += mtd->erasesize - (mtd->writesize * numpages);
-
 	for (i = startblock; i < numblocks; i++) {
 		int ret;
 
 		BUG_ON(bd->options & NAND_BBT_NO_OOB);
 
-		ret = scan_block_fast(mtd, bd, from, buf, numpages);
+		ret = scan_block_fast(mtd, bd, from, buf);
 		if (ret < 0)
 			return ret;
 
-- 
2.7.4

Re: [PATCH 1/3] mtd: rawnand: Support bad block markers in first, second or last page

[Boris Brezillon]

On Mon, 17 Dec 2018 15:49:07 +0000
Schrempf Frieder <frieder.schrempf@kontron.de> wrote:

> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> Currently supported bad block marker positions within the block are:
> * in first page only
> * in last page only
> * in first or second page
> 
> Some ESMT NANDs are known to have been shipped by the manufacturer
> with bad block markers in the first or last page, instead of the
> first or second page.
> 
> Also the datasheets for Cypress/Spansion/AMD NANDs claim that the
> first, second *and* last page needs to be checked.
> 
> Therefore we make it possible to set NAND_BBT_SCAN2NDPAGE and
> NAND_BBT_SCANLASTPAGE at the same time to scan/set all three pages.
> 
> To simplify the code, the logic to evaluate the flags is moved to a
> a new function nand_bbm_page_offset().
> 
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
>  drivers/mtd/nand/raw/internals.h |  1 +
>  drivers/mtd/nand/raw/nand_base.c | 72 ++++++++++++++++++++++++++---------
>  drivers/mtd/nand/raw/nand_bbt.c  | 30 +++++++--------
>  3 files changed, 68 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h
> index 04c2cf7..8e4b168 100644
> --- a/drivers/mtd/nand/raw/internals.h
> +++ b/drivers/mtd/nand/raw/internals.h
> @@ -76,6 +76,7 @@ extern const struct nand_manufacturer_ops toshiba_nand_manuf_ops;
>  
>  /* Core functions */
>  const struct nand_manufacturer *nand_get_manufacturer(u8 id);
> +int nand_bbm_page_offset(struct nand_chip *chip, int index);
>  int nand_markbad_bbm(struct nand_chip *chip, loff_t ofs);
>  int nand_erase_nand(struct nand_chip *chip, struct erase_info *instr,
>  		    int allowbbt);
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 71050a0..388d9ed 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -253,6 +253,45 @@ static void nand_release_device(struct mtd_info *mtd)
>  }
>  
>  /**
> + * nand_bbm_page_offset - Get the page offsets for bad block markers
> + * @chip: NAND chip object
> + * @index: Index for the page offset

Hm, the meaning of index is far from obvious. How about passing the
current page instead (and return 1 if there are more pages to scan 0
otherwise)?

Something like:

static int nand_bbm_get_next_page(struct nand_chip *chip, int page)
{
	struct mtd_info *mtd = nand_to_mtd(chip);
	int last_page = ((mtd->erasesize - mtd->writesize) >>
			 chip->page_shift) & chip->pagemask;

	if (page < 0 && chip->bbt_options & NAND_BBT_SCANFIRSTPAGE)
		return 0;
	else if (page < 1 && chip->bbt_options & NAND_BBT_SCAN2NDPAGE)
		return 1;
	else if (page < last_page &&
		 chip->bbt_options & NAND_BBT_SCANLASTPAGE)
		return last_page;

	return -1;
}

And yes, that means defining NAND_BBT_SCANFIRSTPAGE and setting it when
appropriate.

> + *
> + * Returns an integer that corresponds to the page offset within a block, for
> + * a page that is used to store bad block markers. If no more page offsets are
> + * available, -1 is returned.
> + */
> +int nand_bbm_page_offset(struct nand_chip *chip, int index)
> +{
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	int last_page = ((mtd->erasesize - mtd->writesize) >>
> +			 chip->page_shift) & chip->pagemask;
> +
> +	switch (index) {
> +	case 0:
> +		if ((chip->bbt_options & NAND_BBT_SCANLASTPAGE) &&
> +		    !(chip->bbt_options & NAND_BBT_SCAN2NDPAGE))
> +			return last_page;
> +		else
> +			return 0;
> +		break;
> +	case 1:
> +		if (chip->bbt_options & NAND_BBT_SCAN2NDPAGE)
> +			return 1;
> +		break;
> +	case 2:
> +		if ((chip->bbt_options & NAND_BBT_SCANLASTPAGE) &&
> +		    (chip->bbt_options & NAND_BBT_SCAN2NDPAGE))
> +			return last_page;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return -1;
> +}
> +
> +/**
>   * nand_block_bad - [DEFAULT] Read bad block marker from the chip
>   * @chip: NAND chip object
>   * @ofs: offset from device start
> @@ -261,18 +300,14 @@ static void nand_release_device(struct mtd_info *mtd)
>   */
>  static int nand_block_bad(struct nand_chip *chip, loff_t ofs)
>  {
> -	struct mtd_info *mtd = nand_to_mtd(chip);
> -	int page, page_end, res;
> +	int page_offset, i = 0;
> +	int res, first_page = (int)(ofs >> chip->page_shift) & chip->pagemask;
>  	u8 bad;
>  
> -	if (chip->bbt_options & NAND_BBT_SCANLASTPAGE)
> -		ofs += mtd->erasesize - mtd->writesize;
> +	page_offset = nand_bbm_page_offset(chip, 0);
>  
> -	page = (int)(ofs >> chip->page_shift) & chip->pagemask;
> -	page_end = page + (chip->bbt_options & NAND_BBT_SCAN2NDPAGE ? 2 : 1);
> -
> -	for (; page < page_end; page++) {
> -		res = chip->ecc.read_oob(chip, page);
> +	do {
> +		res = chip->ecc.read_oob(chip, first_page + page_offset);
>  		if (res < 0)
>  			return res;
>  
> @@ -284,7 +319,9 @@ static int nand_block_bad(struct nand_chip *chip, loff_t ofs)
>  			res = hweight8(bad) < chip->badblockbits;
>  		if (res)
>  			return res;
> -	}
> +
> +		page_offset = nand_bbm_page_offset(chip, ++i);
> +	} while (page_offset != -1);
>  
>  	return 0;
>  }
> @@ -303,7 +340,7 @@ static int nand_default_block_markbad(struct nand_chip *chip, loff_t ofs)
>  	struct mtd_info *mtd = nand_to_mtd(chip);
>  	struct mtd_oob_ops ops;
>  	uint8_t buf[2] = { 0, 0 };
> -	int ret = 0, res, i = 0;
> +	int ret = 0, res, i = 0, page_offset;
>  
>  	memset(&ops, 0, sizeof(ops));
>  	ops.oobbuf = buf;
> @@ -316,17 +353,16 @@ static int nand_default_block_markbad(struct nand_chip *chip, loff_t ofs)
>  	}
>  	ops.mode = MTD_OPS_PLACE_OOB;
>  
> -	/* Write to first/last page(s) if necessary */
> -	if (chip->bbt_options & NAND_BBT_SCANLASTPAGE)
> -		ofs += mtd->erasesize - mtd->writesize;
> +	page_offset = nand_bbm_page_offset(chip, 0);
> +
>  	do {
> -		res = nand_do_write_oob(mtd, ofs, &ops);
> +		res = nand_do_write_oob(mtd, ofs + page_offset * mtd->writesize,
> +					&ops);
>  		if (!ret)
>  			ret = res;
>  
> -		i++;
> -		ofs += mtd->writesize;
> -	} while ((chip->bbt_options & NAND_BBT_SCAN2NDPAGE) && i < 2);
> +		page_offset = nand_bbm_page_offset(chip, ++i);
> +	} while (page_offset != -1);
>  
>  	return ret;
>  }
> diff --git a/drivers/mtd/nand/raw/nand_bbt.c b/drivers/mtd/nand/raw/nand_bbt.c
> index 98a8268..b1424b3 100644
> --- a/drivers/mtd/nand/raw/nand_bbt.c
> +++ b/drivers/mtd/nand/raw/nand_bbt.c
> @@ -412,10 +412,11 @@ static void read_abs_bbts(struct mtd_info *mtd, uint8_t *buf,
>  
>  /* Scan a given block partially */
>  static int scan_block_fast(struct mtd_info *mtd, struct nand_bbt_descr *bd,
> -			   loff_t offs, uint8_t *buf, int numpages)
> +			   loff_t offs, uint8_t *buf)
>  {
> +	struct nand_chip *chip = mtd_to_nand(mtd);
>  	struct mtd_oob_ops ops;
> -	int j, ret;
> +	int i = 0, ret, page_offset;
>  
>  	ops.ooblen = mtd->oobsize;
>  	ops.oobbuf = buf;
> @@ -423,12 +424,15 @@ static int scan_block_fast(struct mtd_info *mtd, struct nand_bbt_descr *bd,
>  	ops.datbuf = NULL;
>  	ops.mode = MTD_OPS_PLACE_OOB;
>  
> -	for (j = 0; j < numpages; j++) {
> +	page_offset = nand_bbm_page_offset(chip, 0);
> +
> +	do {
>  		/*
>  		 * Read the full oob until read_oob is fixed to handle single
>  		 * byte reads for 16 bit buswidth.
>  		 */
> -		ret = mtd_read_oob(mtd, offs, &ops);
> +		ret = mtd_read_oob(mtd, offs + page_offset * mtd->writesize,
> +				   &ops);
>  		/* Ignore ECC errors when checking for BBM */
>  		if (ret && !mtd_is_bitflip_or_eccerr(ret))
>  			return ret;
> @@ -436,8 +440,9 @@ static int scan_block_fast(struct mtd_info *mtd, struct nand_bbt_descr *bd,
>  		if (check_short_pattern(buf, bd))
>  			return 1;
>  
> -		offs += mtd->writesize;
> -	}
> +		page_offset = nand_bbm_page_offset(chip, ++i);
> +	} while (page_offset != -1);
> +
>  	return 0;
>  }
>  
> @@ -456,17 +461,11 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf,
>  	struct nand_bbt_descr *bd, int chip)
>  {
>  	struct nand_chip *this = mtd_to_nand(mtd);
> -	int i, numblocks, numpages;
> -	int startblock;
> +	int i, numblocks, startblock;
>  	loff_t from;
>  
>  	pr_info("Scanning device for bad blocks\n");
>  
> -	if (bd->options & NAND_BBT_SCAN2NDPAGE)
> -		numpages = 2;
> -	else
> -		numpages = 1;
> -
>  	if (chip == -1) {
>  		numblocks = mtd->size >> this->bbt_erase_shift;
>  		startblock = 0;
> @@ -483,15 +482,12 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf,
>  		from = (loff_t)startblock << this->bbt_erase_shift;
>  	}
>  
> -	if (this->bbt_options & NAND_BBT_SCANLASTPAGE)
> -		from += mtd->erasesize - (mtd->writesize * numpages);
> -
>  	for (i = startblock; i < numblocks; i++) {
>  		int ret;
>  
>  		BUG_ON(bd->options & NAND_BBT_NO_OOB);
>  
> -		ret = scan_block_fast(mtd, bd, from, buf, numpages);
> +		ret = scan_block_fast(mtd, bd, from, buf);
>  		if (ret < 0)
>  			return ret;
>  

Re: [PATCH 1/3] mtd: rawnand: Support bad block markers in first, second or last page

[Boris Brezillon]

On Thu, 20 Dec 2018 14:59:54 +0100
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> >  
> >  /**
> > + * nand_bbm_page_offset - Get the page offsets for bad block markers
> > + * @chip: NAND chip object
> > + * @index: Index for the page offset  
> 
> Hm, the meaning of index is far from obvious. How about passing the
> current page instead (and return 1 if there are more pages to scan 0
> otherwise)?
> 
> Something like:
> 
> static int nand_bbm_get_next_page(struct nand_chip *chip, int page)
> {
> 	struct mtd_info *mtd = nand_to_mtd(chip);
> 	int last_page = ((mtd->erasesize - mtd->writesize) >>
> 			 chip->page_shift) & chip->pagemask;
> 
> 	if (page < 0 && chip->bbt_options & NAND_BBT_SCANFIRSTPAGE)
> 		return 0;
> 	else if (page < 1 && chip->bbt_options & NAND_BBT_SCAN2NDPAGE)
> 		return 1;
> 	else if (page < last_page &&
> 		 chip->bbt_options & NAND_BBT_SCANLASTPAGE)
> 		return last_page;
> 
> 	return -1;
> }
> 
> And yes, that means defining NAND_BBT_SCANFIRSTPAGE and setting it when
> appropriate.
> 

Oh, and you'll have to define NAND_BBT_SCANFIRST2PAGES like this

#define NAND_BBT_SCANFIRST2PAGES NAND_BBT_SCAN1STPAGE |	\
				 NAND_BBT_SCAN2NDPAGE

and then patch all users of NAND_BBT_SCAN2NDPAGE to use
NAND_BBT_SCANFIRST2PAGES instead.

Re: [PATCH 1/3] mtd: rawnand: Support bad block markers in first, second or last page

[Schrempf Frieder]

On 20.12.18 14:59, Boris Brezillon wrote:
> On Mon, 17 Dec 2018 15:49:07 +0000
> Schrempf Frieder <frieder.schrempf@kontron.de> wrote:
> 
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> Currently supported bad block marker positions within the block are:
>> * in first page only
>> * in last page only
>> * in first or second page
>>
>> Some ESMT NANDs are known to have been shipped by the manufacturer
>> with bad block markers in the first or last page, instead of the
>> first or second page.
>>
>> Also the datasheets for Cypress/Spansion/AMD NANDs claim that the
>> first, second *and* last page needs to be checked.
>>
>> Therefore we make it possible to set NAND_BBT_SCAN2NDPAGE and
>> NAND_BBT_SCANLASTPAGE at the same time to scan/set all three pages.
>>
>> To simplify the code, the logic to evaluate the flags is moved to a
>> a new function nand_bbm_page_offset().
>>
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>> ---
>>   drivers/mtd/nand/raw/internals.h |  1 +
>>   drivers/mtd/nand/raw/nand_base.c | 72 ++++++++++++++++++++++++++---------
>>   drivers/mtd/nand/raw/nand_bbt.c  | 30 +++++++--------
>>   3 files changed, 68 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h
>> index 04c2cf7..8e4b168 100644
>> --- a/drivers/mtd/nand/raw/internals.h
>> +++ b/drivers/mtd/nand/raw/internals.h
>> @@ -76,6 +76,7 @@ extern const struct nand_manufacturer_ops toshiba_nand_manuf_ops;
>>   
>>   /* Core functions */
>>   const struct nand_manufacturer *nand_get_manufacturer(u8 id);
>> +int nand_bbm_page_offset(struct nand_chip *chip, int index);
>>   int nand_markbad_bbm(struct nand_chip *chip, loff_t ofs);
>>   int nand_erase_nand(struct nand_chip *chip, struct erase_info *instr,
>>   		    int allowbbt);
>> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
>> index 71050a0..388d9ed 100644
>> --- a/drivers/mtd/nand/raw/nand_base.c
>> +++ b/drivers/mtd/nand/raw/nand_base.c
>> @@ -253,6 +253,45 @@ static void nand_release_device(struct mtd_info *mtd)
>>   }
>>   
>>   /**
>> + * nand_bbm_page_offset - Get the page offsets for bad block markers
>> + * @chip: NAND chip object
>> + * @index: Index for the page offset
> 
> Hm, the meaning of index is far from obvious. How about passing the
> current page instead (and return 1 if there are more pages to scan 0
> otherwise)?

Good idea.

> 
> Something like:
> 
> static int nand_bbm_get_next_page(struct nand_chip *chip, int page)
> {
> 	struct mtd_info *mtd = nand_to_mtd(chip);
> 	int last_page = ((mtd->erasesize - mtd->writesize) >>
> 			 chip->page_shift) & chip->pagemask;
> 
> 	if (page < 0 && chip->bbt_options & NAND_BBT_SCANFIRSTPAGE)
> 		return 0;
> 	else if (page < 1 && chip->bbt_options & NAND_BBT_SCAN2NDPAGE)
> 		return 1;
> 	else if (page < last_page &&
> 		 chip->bbt_options & NAND_BBT_SCANLASTPAGE)
> 		return last_page;
> 
> 	return -1;
> }
> 
> And yes, that means defining NAND_BBT_SCANFIRSTPAGE and setting it when
> appropriate.

I tried to keep the existing flags and their current meanings, but you 
are right. If we redefine the flags and add NAND_BBT_SCANFIRSTPAGE and 
NAND_BBT_SCANFIRST2PAGES this will be much easier to read.

Also maybe renaming the flags to NAND_BBM_XXX would be even cleaner, as 
we use them not only for scanning, but also for writing markers and they 
are not directly related to the bad block table (BBT)?

By the way, what are your plans for using the common NAND layer (that is 
used by the SPI NAND layer) for raw NAND?
I'm thinking of SPI NANDs that might require things like this, too. 
Currently they seem to have the markers in the first page only, but that 
could change easily and in that case it would be nice to share the code.

> 
>> + *
>> + * Returns an integer that corresponds to the page offset within a block, for
>> + * a page that is used to store bad block markers. If no more page offsets are
>> + * available, -1 is returned.
>> + */
>> +int nand_bbm_page_offset(struct nand_chip *chip, int index)
>> +{
>> +	struct mtd_info *mtd = nand_to_mtd(chip);
>> +	int last_page = ((mtd->erasesize - mtd->writesize) >>
>> +			 chip->page_shift) & chip->pagemask;
>> +
>> +	switch (index) {
>> +	case 0:
>> +		if ((chip->bbt_options & NAND_BBT_SCANLASTPAGE) &&
>> +		    !(chip->bbt_options & NAND_BBT_SCAN2NDPAGE))
>> +			return last_page;
>> +		else
>> +			return 0;
>> +		break;
>> +	case 1:
>> +		if (chip->bbt_options & NAND_BBT_SCAN2NDPAGE)
>> +			return 1;
>> +		break;
>> +	case 2:
>> +		if ((chip->bbt_options & NAND_BBT_SCANLASTPAGE) &&
>> +		    (chip->bbt_options & NAND_BBT_SCAN2NDPAGE))
>> +			return last_page;
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return -1;
>> +}
>> +
>> +/**
>>    * nand_block_bad - [DEFAULT] Read bad block marker from the chip
>>    * @chip: NAND chip object
>>    * @ofs: offset from device start
>> @@ -261,18 +300,14 @@ static void nand_release_device(struct mtd_info *mtd)
>>    */
>>   static int nand_block_bad(struct nand_chip *chip, loff_t ofs)
>>   {
>> -	struct mtd_info *mtd = nand_to_mtd(chip);
>> -	int page, page_end, res;
>> +	int page_offset, i = 0;
>> +	int res, first_page = (int)(ofs >> chip->page_shift) & chip->pagemask;
>>   	u8 bad;
>>   
>> -	if (chip->bbt_options & NAND_BBT_SCANLASTPAGE)
>> -		ofs += mtd->erasesize - mtd->writesize;
>> +	page_offset = nand_bbm_page_offset(chip, 0);
>>   
>> -	page = (int)(ofs >> chip->page_shift) & chip->pagemask;
>> -	page_end = page + (chip->bbt_options & NAND_BBT_SCAN2NDPAGE ? 2 : 1);
>> -
>> -	for (; page < page_end; page++) {
>> -		res = chip->ecc.read_oob(chip, page);
>> +	do {
>> +		res = chip->ecc.read_oob(chip, first_page + page_offset);
>>   		if (res < 0)
>>   			return res;
>>   
>> @@ -284,7 +319,9 @@ static int nand_block_bad(struct nand_chip *chip, loff_t ofs)
>>   			res = hweight8(bad) < chip->badblockbits;
>>   		if (res)
>>   			return res;
>> -	}
>> +
>> +		page_offset = nand_bbm_page_offset(chip, ++i);
>> +	} while (page_offset != -1);
>>   
>>   	return 0;
>>   }
>> @@ -303,7 +340,7 @@ static int nand_default_block_markbad(struct nand_chip *chip, loff_t ofs)
>>   	struct mtd_info *mtd = nand_to_mtd(chip);
>>   	struct mtd_oob_ops ops;
>>   	uint8_t buf[2] = { 0, 0 };
>> -	int ret = 0, res, i = 0;
>> +	int ret = 0, res, i = 0, page_offset;
>>   
>>   	memset(&ops, 0, sizeof(ops));
>>   	ops.oobbuf = buf;
>> @@ -316,17 +353,16 @@ static int nand_default_block_markbad(struct nand_chip *chip, loff_t ofs)
>>   	}
>>   	ops.mode = MTD_OPS_PLACE_OOB;
>>   
>> -	/* Write to first/last page(s) if necessary */
>> -	if (chip->bbt_options & NAND_BBT_SCANLASTPAGE)
>> -		ofs += mtd->erasesize - mtd->writesize;
>> +	page_offset = nand_bbm_page_offset(chip, 0);
>> +
>>   	do {
>> -		res = nand_do_write_oob(mtd, ofs, &ops);
>> +		res = nand_do_write_oob(mtd, ofs + page_offset * mtd->writesize,
>> +					&ops);
>>   		if (!ret)
>>   			ret = res;
>>   
>> -		i++;
>> -		ofs += mtd->writesize;
>> -	} while ((chip->bbt_options & NAND_BBT_SCAN2NDPAGE) && i < 2);
>> +		page_offset = nand_bbm_page_offset(chip, ++i);
>> +	} while (page_offset != -1);
>>   
>>   	return ret;
>>   }
>> diff --git a/drivers/mtd/nand/raw/nand_bbt.c b/drivers/mtd/nand/raw/nand_bbt.c
>> index 98a8268..b1424b3 100644
>> --- a/drivers/mtd/nand/raw/nand_bbt.c
>> +++ b/drivers/mtd/nand/raw/nand_bbt.c
>> @@ -412,10 +412,11 @@ static void read_abs_bbts(struct mtd_info *mtd, uint8_t *buf,
>>   
>>   /* Scan a given block partially */
>>   static int scan_block_fast(struct mtd_info *mtd, struct nand_bbt_descr *bd,
>> -			   loff_t offs, uint8_t *buf, int numpages)
>> +			   loff_t offs, uint8_t *buf)
>>   {
>> +	struct nand_chip *chip = mtd_to_nand(mtd);
>>   	struct mtd_oob_ops ops;
>> -	int j, ret;
>> +	int i = 0, ret, page_offset;
>>   
>>   	ops.ooblen = mtd->oobsize;
>>   	ops.oobbuf = buf;
>> @@ -423,12 +424,15 @@ static int scan_block_fast(struct mtd_info *mtd, struct nand_bbt_descr *bd,
>>   	ops.datbuf = NULL;
>>   	ops.mode = MTD_OPS_PLACE_OOB;
>>   
>> -	for (j = 0; j < numpages; j++) {
>> +	page_offset = nand_bbm_page_offset(chip, 0);
>> +
>> +	do {
>>   		/*
>>   		 * Read the full oob until read_oob is fixed to handle single
>>   		 * byte reads for 16 bit buswidth.
>>   		 */
>> -		ret = mtd_read_oob(mtd, offs, &ops);
>> +		ret = mtd_read_oob(mtd, offs + page_offset * mtd->writesize,
>> +				   &ops);
>>   		/* Ignore ECC errors when checking for BBM */
>>   		if (ret && !mtd_is_bitflip_or_eccerr(ret))
>>   			return ret;
>> @@ -436,8 +440,9 @@ static int scan_block_fast(struct mtd_info *mtd, struct nand_bbt_descr *bd,
>>   		if (check_short_pattern(buf, bd))
>>   			return 1;
>>   
>> -		offs += mtd->writesize;
>> -	}
>> +		page_offset = nand_bbm_page_offset(chip, ++i);
>> +	} while (page_offset != -1);
>> +
>>   	return 0;
>>   }
>>   
>> @@ -456,17 +461,11 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf,
>>   	struct nand_bbt_descr *bd, int chip)
>>   {
>>   	struct nand_chip *this = mtd_to_nand(mtd);
>> -	int i, numblocks, numpages;
>> -	int startblock;
>> +	int i, numblocks, startblock;
>>   	loff_t from;
>>   
>>   	pr_info("Scanning device for bad blocks\n");
>>   
>> -	if (bd->options & NAND_BBT_SCAN2NDPAGE)
>> -		numpages = 2;
>> -	else
>> -		numpages = 1;
>> -
>>   	if (chip == -1) {
>>   		numblocks = mtd->size >> this->bbt_erase_shift;
>>   		startblock = 0;
>> @@ -483,15 +482,12 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf,
>>   		from = (loff_t)startblock << this->bbt_erase_shift;
>>   	}
>>   
>> -	if (this->bbt_options & NAND_BBT_SCANLASTPAGE)
>> -		from += mtd->erasesize - (mtd->writesize * numpages);
>> -
>>   	for (i = startblock; i < numblocks; i++) {
>>   		int ret;
>>   
>>   		BUG_ON(bd->options & NAND_BBT_NO_OOB);
>>   
>> -		ret = scan_block_fast(mtd, bd, from, buf, numpages);
>> +		ret = scan_block_fast(mtd, bd, from, buf);
>>   		if (ret < 0)
>>   			return ret;
>>   
> 

Re: [PATCH 1/3] mtd: rawnand: Support bad block markers in first, second or last page

[Boris Brezillon]

Hi Frieder,

On Thu, 20 Dec 2018 14:35:05 +0000
Schrempf Frieder <frieder.schrempf@kontron.de> wrote:

> On 20.12.18 14:59, Boris Brezillon wrote:
> > On Mon, 17 Dec 2018 15:49:07 +0000
> > Schrempf Frieder <frieder.schrempf@kontron.de> wrote:
> >   
> >> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> >>
> >> Currently supported bad block marker positions within the block are:
> >> * in first page only
> >> * in last page only
> >> * in first or second page
> >>
> >> Some ESMT NANDs are known to have been shipped by the manufacturer
> >> with bad block markers in the first or last page, instead of the
> >> first or second page.
> >>
> >> Also the datasheets for Cypress/Spansion/AMD NANDs claim that the
> >> first, second *and* last page needs to be checked.
> >>
> >> Therefore we make it possible to set NAND_BBT_SCAN2NDPAGE and
> >> NAND_BBT_SCANLASTPAGE at the same time to scan/set all three pages.
> >>
> >> To simplify the code, the logic to evaluate the flags is moved to a
> >> a new function nand_bbm_page_offset().
> >>
> >> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> >> ---
> >>   drivers/mtd/nand/raw/internals.h |  1 +
> >>   drivers/mtd/nand/raw/nand_base.c | 72 ++++++++++++++++++++++++++---------
> >>   drivers/mtd/nand/raw/nand_bbt.c  | 30 +++++++--------
> >>   3 files changed, 68 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h
> >> index 04c2cf7..8e4b168 100644
> >> --- a/drivers/mtd/nand/raw/internals.h
> >> +++ b/drivers/mtd/nand/raw/internals.h
> >> @@ -76,6 +76,7 @@ extern const struct nand_manufacturer_ops toshiba_nand_manuf_ops;
> >>   
> >>   /* Core functions */
> >>   const struct nand_manufacturer *nand_get_manufacturer(u8 id);
> >> +int nand_bbm_page_offset(struct nand_chip *chip, int index);
> >>   int nand_markbad_bbm(struct nand_chip *chip, loff_t ofs);
> >>   int nand_erase_nand(struct nand_chip *chip, struct erase_info *instr,
> >>   		    int allowbbt);
> >> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> >> index 71050a0..388d9ed 100644
> >> --- a/drivers/mtd/nand/raw/nand_base.c
> >> +++ b/drivers/mtd/nand/raw/nand_base.c
> >> @@ -253,6 +253,45 @@ static void nand_release_device(struct mtd_info *mtd)
> >>   }
> >>   
> >>   /**
> >> + * nand_bbm_page_offset - Get the page offsets for bad block markers
> >> + * @chip: NAND chip object
> >> + * @index: Index for the page offset  
> > 
> > Hm, the meaning of index is far from obvious. How about passing the
> > current page instead (and return 1 if there are more pages to scan 0
> > otherwise)?  
> 
> Good idea.
> 
> > 
> > Something like:
> > 
> > static int nand_bbm_get_next_page(struct nand_chip *chip, int page)
> > {
> > 	struct mtd_info *mtd = nand_to_mtd(chip);
> > 	int last_page = ((mtd->erasesize - mtd->writesize) >>
> > 			 chip->page_shift) & chip->pagemask;
> > 
> > 	if (page < 0 && chip->bbt_options & NAND_BBT_SCANFIRSTPAGE)
> > 		return 0;
> > 	else if (page < 1 && chip->bbt_options & NAND_BBT_SCAN2NDPAGE)
> > 		return 1;
> > 	else if (page < last_page &&
> > 		 chip->bbt_options & NAND_BBT_SCANLASTPAGE)
> > 		return last_page;
> > 
> > 	return -1;
> > }
> > 
> > And yes, that means defining NAND_BBT_SCANFIRSTPAGE and setting it when
> > appropriate.  
> 
> I tried to keep the existing flags and their current meanings, but you 
> are right. If we redefine the flags and add NAND_BBT_SCANFIRSTPAGE and 
> NAND_BBT_SCANFIRST2PAGES this will be much easier to read.
> 
> Also maybe renaming the flags to NAND_BBM_XXX would be even cleaner, as 
> we use them not only for scanning, but also for writing markers and they 
> are not directly related to the bad block table (BBT)?

Yep, and maybe move them to chip->options too.

> 
> By the way, what are your plans for using the common NAND layer (that is 
> used by the SPI NAND layer) for raw NAND?

I'd still like to have this done at some point, just don't have the
time to do it myself ;-). I started working on that a few weeks back
[1], but didn't have time to finish it.

> I'm thinking of SPI NANDs that might require things like this, too. 

Yes, probably.

> Currently they seem to have the markers in the first page only, but that 
> could change easily and in that case it would be nice to share the code.

Yes. Actually, that's the whole BBT + BBM scanning logic we should make
generic. But I'd like to take this as an opportunity to
cleanup/simplify the bbt code instead of simply porting it to the
generic NAND layer.

If you have some time, feel free to finish what I started.

Regards,

Boris

[1]https://github.com/bbrezillon/linux/commits/nand/cleanup

[PATCH 2/3] mtd: rawnand: ESMT: Also use the last page for bad block markers

[Schrempf Frieder]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

It is known that some ESMT SLC NANDs have been shipped
with the factory bad block markers in the first or last page
of the block, instead of the first or second page. To be on
the safe side, let's check all three locations.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/mtd/nand/raw/nand_esmt.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/nand_esmt.c b/drivers/mtd/nand/raw/nand_esmt.c
index 96f039a..275bc8e 100644
--- a/drivers/mtd/nand/raw/nand_esmt.c
+++ b/drivers/mtd/nand/raw/nand_esmt.c
@@ -36,7 +36,14 @@ static void esmt_nand_decode_id(struct nand_chip *chip)
 static int esmt_nand_init(struct nand_chip *chip)
 {
 	if (nand_is_slc(chip))
-		chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
+		/*
+		 * It is known that some ESMT SLC NANDs have been shipped
+		 * with the factory bad block markers in the first or last page
+		 * of the block, instead of the first or second page. To be on
+		 * the safe side, let's check all three locations.
+		 */
+		chip->bbt_options |= NAND_BBT_SCAN2NDPAGE |
+				     NAND_BBT_SCANLASTPAGE;
 
 	return 0;
 }
-- 
2.7.4

[PATCH 3/3] mtd: rawnand: AMD: Also use the last page for bad block markers

[Schrempf Frieder]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

According to the datasheet of some Cypress SLC NANDs, the bad
block markers can be in the first, second or last page of a block.
So let's check all three locations.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/mtd/nand/raw/nand_amd.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/nand_amd.c b/drivers/mtd/nand/raw/nand_amd.c
index 890c5b4..aad5f14 100644
--- a/drivers/mtd/nand/raw/nand_amd.c
+++ b/drivers/mtd/nand/raw/nand_amd.c
@@ -40,7 +40,13 @@ static void amd_nand_decode_id(struct nand_chip *chip)
 static int amd_nand_init(struct nand_chip *chip)
 {
 	if (nand_is_slc(chip))
-		chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
+		/*
+		 * According to the datasheet of some Cypress SLC NANDs,
+		 * the bad block markers can be in the first, second or last
+		 * page of a block. So let's check all three locations.
+		 */
+		chip->bbt_options |= NAND_BBT_SCAN2NDPAGE |
+				     NAND_BBT_SCANLASTPAGE;
 
 	return 0;
 }
-- 
2.7.4