[PATCH 0/3] mtd: Ingenic NAND fix for JZ4740

[PATCH 0/3] mtd: Ingenic NAND fix for JZ4740

[Paul Cercueil]

Hi,

Looks like NAND support has been broken on the JZ4740 SoC for a while;
it looks like it comes from the fact that the "hw_oob_first" mechanism
was dropped from the NAND core and moved to the Davinci driver.

It turns out the JZ4740 SoC needs it too; I didn't notice it when
writing the new ingenic-nand driver (to replace the old jz4740-nand
driver) most likely because my Device Tree had the "nand-ecc-mode" set
to "hw_oob_first".

I am not very sure about patch [1/3]; to me the original code does not
make sense, and it didn't work out-of-the-box on the JZ4740 without it.
By applying patch [1/3] the function nand_read_page_hwecc_oob_first()
can be reused for the JZ4740 SoC as well. But I did not test patch [1/3]
on Davinci.

Cheers,
-Paul

Paul Cercueil (3):
  mtd: rawnand/davinci: Don't calculate ECC when reading page
  mtd: rawnand: Export nand_read_page_hwecc_oob_first()
  mtd: rawnand/ingenic: JZ4740 needs 'oob_first' read page function

 drivers/mtd/nand/raw/davinci_nand.c           | 73 +------------------
 .../mtd/nand/raw/ingenic/ingenic_nand_drv.c   |  5 ++
 drivers/mtd/nand/raw/nand_base.c              | 69 ++++++++++++++++++
 include/linux/mtd/rawnand.h                   |  2 +
 4 files changed, 77 insertions(+), 72 deletions(-)

-- 
2.33.0

[PATCH 1/3] mtd: rawnand/davinci: Don't calculate ECC when reading page

[Paul Cercueil]

The function nand_davinci_read_page_hwecc_oob_first() does read the ECC
data from the OOB area. Therefore it does not need to calculate the ECC
as it is already available.

Cc: <stable@vger.kernel.org> # v5.2
Fixes: a0ac778eb82c ("mtd: rawnand: ingenic: Add support for the JZ4740")
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/mtd/nand/raw/davinci_nand.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/davinci_nand.c b/drivers/mtd/nand/raw/davinci_nand.c
index 118da9944e3b..89de24d3bb7a 100644
--- a/drivers/mtd/nand/raw/davinci_nand.c
+++ b/drivers/mtd/nand/raw/davinci_nand.c
@@ -394,7 +394,6 @@ static int nand_davinci_read_page_hwecc_oob_first(struct nand_chip *chip,
 	int eccsteps = chip->ecc.steps;
 	uint8_t *p = buf;
 	uint8_t *ecc_code = chip->ecc.code_buf;
-	uint8_t *ecc_calc = chip->ecc.calc_buf;
 	unsigned int max_bitflips = 0;
 
 	/* Read the OOB area first */
@@ -420,8 +419,6 @@ static int nand_davinci_read_page_hwecc_oob_first(struct nand_chip *chip,
 		if (ret)
 			return ret;
 
-		chip->ecc.calculate(chip, p, &ecc_calc[i]);
-
 		stat = chip->ecc.correct(chip, p, &ecc_code[i], NULL);
 		if (stat == -EBADMSG &&
 		    (chip->ecc.options & NAND_ECC_GENERIC_ERASED_CHECK)) {
-- 
2.33.0

[PATCH 2/3] mtd: rawnand: Export nand_read_page_hwecc_oob_first()

[Paul Cercueil]

Move the function nand_read_page_hwecc_oob_first() (previously
nand_davinci_read_page_hwecc_oob_first()) to nand_base.c, and export it
as a GPL symbol, so that it can be used by more modules.

Cc: <stable@vger.kernel.org> # v5.2
Fixes: a0ac778eb82c ("mtd: rawnand: ingenic: Add support for the JZ4740")
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/mtd/nand/raw/davinci_nand.c | 70 +----------------------------
 drivers/mtd/nand/raw/nand_base.c    | 69 ++++++++++++++++++++++++++++
 include/linux/mtd/rawnand.h         |  2 +
 3 files changed, 72 insertions(+), 69 deletions(-)

diff --git a/drivers/mtd/nand/raw/davinci_nand.c b/drivers/mtd/nand/raw/davinci_nand.c
index 89de24d3bb7a..45fec8c192ab 100644
--- a/drivers/mtd/nand/raw/davinci_nand.c
+++ b/drivers/mtd/nand/raw/davinci_nand.c
@@ -371,74 +371,6 @@ static int nand_davinci_correct_4bit(struct nand_chip *chip, u_char *data,
 	return corrected;
 }
 
-/**
- * nand_read_page_hwecc_oob_first - hw ecc, read oob first
- * @chip: nand chip info structure
- * @buf: buffer to store read data
- * @oob_required: caller requires OOB data read to chip->oob_poi
- * @page: page number to read
- *
- * Hardware ECC for large page chips, require OOB to be read first. For this
- * ECC mode, the write_page method is re-used from ECC_HW. These methods
- * read/write ECC from the OOB area, unlike the ECC_HW_SYNDROME support with
- * multiple ECC steps, follows the "infix ECC" scheme and reads/writes ECC from
- * the data area, by overwriting the NAND manufacturer bad block markings.
- */
-static int nand_davinci_read_page_hwecc_oob_first(struct nand_chip *chip,
-						  uint8_t *buf,
-						  int oob_required, int page)
-{
-	struct mtd_info *mtd = nand_to_mtd(chip);
-	int i, eccsize = chip->ecc.size, ret;
-	int eccbytes = chip->ecc.bytes;
-	int eccsteps = chip->ecc.steps;
-	uint8_t *p = buf;
-	uint8_t *ecc_code = chip->ecc.code_buf;
-	unsigned int max_bitflips = 0;
-
-	/* Read the OOB area first */
-	ret = nand_read_oob_op(chip, page, 0, chip->oob_poi, mtd->oobsize);
-	if (ret)
-		return ret;
-
-	ret = nand_read_page_op(chip, page, 0, NULL, 0);
-	if (ret)
-		return ret;
-
-	ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0,
-					 chip->ecc.total);
-	if (ret)
-		return ret;
-
-	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
-		int stat;
-
-		chip->ecc.hwctl(chip, NAND_ECC_READ);
-
-		ret = nand_read_data_op(chip, p, eccsize, false, false);
-		if (ret)
-			return ret;
-
-		stat = chip->ecc.correct(chip, p, &ecc_code[i], NULL);
-		if (stat == -EBADMSG &&
-		    (chip->ecc.options & NAND_ECC_GENERIC_ERASED_CHECK)) {
-			/* check for empty pages with bitflips */
-			stat = nand_check_erased_ecc_chunk(p, eccsize,
-							   &ecc_code[i],
-							   eccbytes, NULL, 0,
-							   chip->ecc.strength);
-		}
-
-		if (stat < 0) {
-			mtd->ecc_stats.failed++;
-		} else {
-			mtd->ecc_stats.corrected += stat;
-			max_bitflips = max_t(unsigned int, max_bitflips, stat);
-		}
-	}
-	return max_bitflips;
-}
-
 /*----------------------------------------------------------------------*/
 
 /* An ECC layout for using 4-bit ECC with small-page flash, storing
@@ -648,7 +580,7 @@ static int davinci_nand_attach_chip(struct nand_chip *chip)
 			} else if (chunks == 4 || chunks == 8) {
 				mtd_set_ooblayout(mtd,
 						  nand_get_large_page_ooblayout());
-				chip->ecc.read_page = nand_davinci_read_page_hwecc_oob_first;
+				chip->ecc.read_page = nand_read_page_hwecc_oob_first;
 			} else {
 				return -EIO;
 			}
diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 3d6c6e880520..cb5f343b9fa2 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -3160,6 +3160,75 @@ static int nand_read_page_hwecc(struct nand_chip *chip, uint8_t *buf,
 	return max_bitflips;
 }
 
+/**
+ * nand_read_page_hwecc_oob_first - Hardware ECC page read with ECC
+ *                                  data read from OOB area
+ * @chip: nand chip info structure
+ * @buf: buffer to store read data
+ * @oob_required: caller requires OOB data read to chip->oob_poi
+ * @page: page number to read
+ *
+ * Hardware ECC for large page chips, require OOB to be read first. For this
+ * ECC mode, the write_page method is re-used from ECC_HW. These methods
+ * read/write ECC from the OOB area, unlike the ECC_HW_SYNDROME support with
+ * multiple ECC steps, follows the "infix ECC" scheme and reads/writes ECC from
+ * the data area, by overwriting the NAND manufacturer bad block markings.
+ */
+int nand_read_page_hwecc_oob_first(struct nand_chip *chip, uint8_t *buf,
+				   int oob_required, int page)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	int i, eccsize = chip->ecc.size, ret;
+	int eccbytes = chip->ecc.bytes;
+	int eccsteps = chip->ecc.steps;
+	uint8_t *p = buf;
+	uint8_t *ecc_code = chip->ecc.code_buf;
+	unsigned int max_bitflips = 0;
+
+	/* Read the OOB area first */
+	ret = nand_read_oob_op(chip, page, 0, chip->oob_poi, mtd->oobsize);
+	if (ret)
+		return ret;
+
+	ret = nand_read_page_op(chip, page, 0, NULL, 0);
+	if (ret)
+		return ret;
+
+	ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0,
+					 chip->ecc.total);
+	if (ret)
+		return ret;
+
+	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
+		int stat;
+
+		chip->ecc.hwctl(chip, NAND_ECC_READ);
+
+		ret = nand_read_data_op(chip, p, eccsize, false, false);
+		if (ret)
+			return ret;
+
+		stat = chip->ecc.correct(chip, p, &ecc_code[i], NULL);
+		if (stat == -EBADMSG &&
+		    (chip->ecc.options & NAND_ECC_GENERIC_ERASED_CHECK)) {
+			/* check for empty pages with bitflips */
+			stat = nand_check_erased_ecc_chunk(p, eccsize,
+							   &ecc_code[i],
+							   eccbytes, NULL, 0,
+							   chip->ecc.strength);
+		}
+
+		if (stat < 0) {
+			mtd->ecc_stats.failed++;
+		} else {
+			mtd->ecc_stats.corrected += stat;
+			max_bitflips = max_t(unsigned int, max_bitflips, stat);
+		}
+	}
+	return max_bitflips;
+}
+EXPORT_SYMBOL_GPL(nand_read_page_hwecc_oob_first);
+
 /**
  * nand_read_page_syndrome - [REPLACEABLE] hardware ECC syndrome based page read
  * @chip: nand chip info structure
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index b2f9dd3cbd69..5b88cd51fadb 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -1539,6 +1539,8 @@ int nand_read_data_op(struct nand_chip *chip, void *buf, unsigned int len,
 		      bool force_8bit, bool check_only);
 int nand_write_data_op(struct nand_chip *chip, const void *buf,
 		       unsigned int len, bool force_8bit);
+int nand_read_page_hwecc_oob_first(struct nand_chip *chip, uint8_t *buf,
+				   int oob_required, int page);
 
 /* Scan and identify a NAND device */
 int nand_scan_with_ids(struct nand_chip *chip, unsigned int max_chips,
-- 
2.33.0

[PATCH 3/3] mtd: rawnand/ingenic: JZ4740 needs 'oob_first' read page function

[Paul Cercueil]

The ECC engine on the JZ4740 SoC requires the ECC data to be read before
the page; using the default page reading function does not work. Indeed,
the old JZ4740 NAND driver (removed in 5.4) did use the 'OOB first' flag
that existed back then.

Use the newly created nand_read_page_hwecc_oob_first() to address this
issue.

This issue was not found when the new ingenic-nand driver was developed,
most likely because the Device Tree used had the nand-ecc-mode set to
"hw_oob_first", which seems to not be supported anymore.

Cc: <stable@vger.kernel.org> # v5.2
Fixes: a0ac778eb82c ("mtd: rawnand: ingenic: Add support for the JZ4740")
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
index 0e9d426fe4f2..b18861bdcdc8 100644
--- a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
+++ b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
@@ -32,6 +32,7 @@ struct jz_soc_info {
 	unsigned long addr_offset;
 	unsigned long cmd_offset;
 	const struct mtd_ooblayout_ops *oob_layout;
+	bool oob_first;
 };
 
 struct ingenic_nand_cs {
@@ -240,6 +241,9 @@ static int ingenic_nand_attach_chip(struct nand_chip *chip)
 	if (chip->bbt_options & NAND_BBT_USE_FLASH)
 		chip->bbt_options |= NAND_BBT_NO_OOB;
 
+	if (nfc->soc_info->oob_first)
+		chip->ecc.read_page = nand_read_page_hwecc_oob_first;
+
 	/* For legacy reasons we use a different layout on the qi,lb60 board. */
 	if (of_machine_is_compatible("qi,lb60"))
 		mtd_set_ooblayout(mtd, &qi_lb60_ooblayout_ops);
@@ -534,6 +538,7 @@ static const struct jz_soc_info jz4740_soc_info = {
 	.data_offset = 0x00000000,
 	.cmd_offset = 0x00008000,
 	.addr_offset = 0x00010000,
+	.oob_first = true,
 };
 
 static const struct jz_soc_info jz4725b_soc_info = {
-- 
2.33.0

Re: [PATCH 2/3] mtd: rawnand: Export nand_read_page_hwecc_oob_first()

[Miquel Raynal]

Hi Paul,

paul@crapouillou.net wrote on Sat,  9 Oct 2021 20:49:51 +0200:

> Move the function nand_read_page_hwecc_oob_first() (previously
> nand_davinci_read_page_hwecc_oob_first()) to nand_base.c, and export it
> as a GPL symbol, so that it can be used by more modules.
> 
> Cc: <stable@vger.kernel.org> # v5.2
> Fixes: a0ac778eb82c ("mtd: rawnand: ingenic: Add support for the JZ4740")
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/mtd/nand/raw/davinci_nand.c | 70 +----------------------------
>  drivers/mtd/nand/raw/nand_base.c    | 69 ++++++++++++++++++++++++++++
>  include/linux/mtd/rawnand.h         |  2 +
>  3 files changed, 72 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/davinci_nand.c b/drivers/mtd/nand/raw/davinci_nand.c
> index 89de24d3bb7a..45fec8c192ab 100644
> --- a/drivers/mtd/nand/raw/davinci_nand.c
> +++ b/drivers/mtd/nand/raw/davinci_nand.c
> @@ -371,74 +371,6 @@ static int nand_davinci_correct_4bit(struct nand_chip *chip, u_char *data,
>  	return corrected;
>  }
>  
> -/**
> - * nand_read_page_hwecc_oob_first - hw ecc, read oob first
> - * @chip: nand chip info structure
> - * @buf: buffer to store read data
> - * @oob_required: caller requires OOB data read to chip->oob_poi
> - * @page: page number to read
> - *
> - * Hardware ECC for large page chips, require OOB to be read first. For this
> - * ECC mode, the write_page method is re-used from ECC_HW. These methods
> - * read/write ECC from the OOB area, unlike the ECC_HW_SYNDROME support with
> - * multiple ECC steps, follows the "infix ECC" scheme and reads/writes ECC from
> - * the data area, by overwriting the NAND manufacturer bad block markings.
> - */
> -static int nand_davinci_read_page_hwecc_oob_first(struct nand_chip *chip,
> -						  uint8_t *buf,
> -						  int oob_required, int page)
> -{
> -	struct mtd_info *mtd = nand_to_mtd(chip);
> -	int i, eccsize = chip->ecc.size, ret;
> -	int eccbytes = chip->ecc.bytes;
> -	int eccsteps = chip->ecc.steps;
> -	uint8_t *p = buf;
> -	uint8_t *ecc_code = chip->ecc.code_buf;
> -	unsigned int max_bitflips = 0;
> -
> -	/* Read the OOB area first */
> -	ret = nand_read_oob_op(chip, page, 0, chip->oob_poi, mtd->oobsize);
> -	if (ret)
> -		return ret;
> -
> -	ret = nand_read_page_op(chip, page, 0, NULL, 0);
> -	if (ret)
> -		return ret;
> -
> -	ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0,
> -					 chip->ecc.total);
> -	if (ret)
> -		return ret;
> -
> -	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
> -		int stat;
> -
> -		chip->ecc.hwctl(chip, NAND_ECC_READ);
> -
> -		ret = nand_read_data_op(chip, p, eccsize, false, false);
> -		if (ret)
> -			return ret;
> -
> -		stat = chip->ecc.correct(chip, p, &ecc_code[i], NULL);
> -		if (stat == -EBADMSG &&
> -		    (chip->ecc.options & NAND_ECC_GENERIC_ERASED_CHECK)) {
> -			/* check for empty pages with bitflips */
> -			stat = nand_check_erased_ecc_chunk(p, eccsize,
> -							   &ecc_code[i],
> -							   eccbytes, NULL, 0,
> -							   chip->ecc.strength);
> -		}
> -
> -		if (stat < 0) {
> -			mtd->ecc_stats.failed++;
> -		} else {
> -			mtd->ecc_stats.corrected += stat;
> -			max_bitflips = max_t(unsigned int, max_bitflips, stat);
> -		}
> -	}
> -	return max_bitflips;
> -}
> -
>  /*----------------------------------------------------------------------*/
>  
>  /* An ECC layout for using 4-bit ECC with small-page flash, storing
> @@ -648,7 +580,7 @@ static int davinci_nand_attach_chip(struct nand_chip *chip)
>  			} else if (chunks == 4 || chunks == 8) {
>  				mtd_set_ooblayout(mtd,
>  						  nand_get_large_page_ooblayout());
> -				chip->ecc.read_page = nand_davinci_read_page_hwecc_oob_first;
> +				chip->ecc.read_page = nand_read_page_hwecc_oob_first;
>  			} else {
>  				return -EIO;
>  			}
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 3d6c6e880520..cb5f343b9fa2 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -3160,6 +3160,75 @@ static int nand_read_page_hwecc(struct nand_chip *chip, uint8_t *buf,
>  	return max_bitflips;
>  }
>  
> +/**
> + * nand_read_page_hwecc_oob_first - Hardware ECC page read with ECC
> + *                                  data read from OOB area
> + * @chip: nand chip info structure
> + * @buf: buffer to store read data
> + * @oob_required: caller requires OOB data read to chip->oob_poi
> + * @page: page number to read
> + *
> + * Hardware ECC for large page chips, require OOB to be read first. For this

requires

With this ECC configuration?

> + * ECC mode, the write_page method is re-used from ECC_HW. These methods

I do not understand this sentence nor the next one about syndrome. I
believe it is related to your engine and should not leak into the core.

> + * read/write ECC from the OOB area, unlike the ECC_HW_SYNDROME support with
> + * multiple ECC steps, follows the "infix ECC" scheme and reads/writes ECC from
> + * the data area, by overwriting the NAND manufacturer bad block markings.

That's a sentence I don't like. What do you mean exactly?

What "Infix ECC" scheme is?

Do you mean that unlike the syndrome  mode it *does not* overwrite the
BBM ?

> + */
> +int nand_read_page_hwecc_oob_first(struct nand_chip *chip, uint8_t *buf,
> +				   int oob_required, int page)
> +{
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	int i, eccsize = chip->ecc.size, ret;
> +	int eccbytes = chip->ecc.bytes;
> +	int eccsteps = chip->ecc.steps;
> +	uint8_t *p = buf;
> +	uint8_t *ecc_code = chip->ecc.code_buf;
> +	unsigned int max_bitflips = 0;
> +
> +	/* Read the OOB area first */
> +	ret = nand_read_oob_op(chip, page, 0, chip->oob_poi, mtd->oobsize);
> +	if (ret)
> +		return ret;
> +
> +	ret = nand_read_page_op(chip, page, 0, NULL, 0);

Definitely not, your are requesting the chip to do the read_page
operation twice. You only need a nand_change_read_column I believe.

> +	if (ret)
> +		return ret;
> +
> +	ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0,
> +					 chip->ecc.total);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
> +		int stat;
> +
> +		chip->ecc.hwctl(chip, NAND_ECC_READ);
> +
> +		ret = nand_read_data_op(chip, p, eccsize, false, false);
> +		if (ret)
> +			return ret;
> +
> +		stat = chip->ecc.correct(chip, p, &ecc_code[i], NULL);
> +		if (stat == -EBADMSG &&
> +		    (chip->ecc.options & NAND_ECC_GENERIC_ERASED_CHECK)) {
> +			/* check for empty pages with bitflips */
> +			stat = nand_check_erased_ecc_chunk(p, eccsize,
> +							   &ecc_code[i],
> +							   eccbytes, NULL, 0,
> +							   chip->ecc.strength);
> +		}
> +
> +		if (stat < 0) {
> +			mtd->ecc_stats.failed++;
> +		} else {
> +			mtd->ecc_stats.corrected += stat;
> +			max_bitflips = max_t(unsigned int, max_bitflips, stat);
> +		}
> +	}
> +	return max_bitflips;
> +}
> +EXPORT_SYMBOL_GPL(nand_read_page_hwecc_oob_first);
> +
>  /**
>   * nand_read_page_syndrome - [REPLACEABLE] hardware ECC syndrome based page read
>   * @chip: nand chip info structure
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index b2f9dd3cbd69..5b88cd51fadb 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -1539,6 +1539,8 @@ int nand_read_data_op(struct nand_chip *chip, void *buf, unsigned int len,
>  		      bool force_8bit, bool check_only);
>  int nand_write_data_op(struct nand_chip *chip, const void *buf,
>  		       unsigned int len, bool force_8bit);
> +int nand_read_page_hwecc_oob_first(struct nand_chip *chip, uint8_t *buf,
> +				   int oob_required, int page);

You certainly want to add this symbol closer to the other read/write
page helpers?

>  
>  /* Scan and identify a NAND device */
>  int nand_scan_with_ids(struct nand_chip *chip, unsigned int max_chips,


Thanks,
Miquèl

Re: [PATCH 3/3] mtd: rawnand/ingenic: JZ4740 needs 'oob_first' read page function

[Miquel Raynal]

Hi Paul,

paul@crapouillou.net wrote on Sat,  9 Oct 2021 20:49:52 +0200:

> The ECC engine on the JZ4740 SoC requires the ECC data to be read before
> the page; using the default page reading function does not work. Indeed,
> the old JZ4740 NAND driver (removed in 5.4) did use the 'OOB first' flag
> that existed back then.
> 
> Use the newly created nand_read_page_hwecc_oob_first() to address this
> issue.
> 
> This issue was not found when the new ingenic-nand driver was developed,
> most likely because the Device Tree used had the nand-ecc-mode set to
> "hw_oob_first", which seems to not be supported anymore.

I would rename both the boolean and the helper "*_access_oob_first"
which seems more precise.

> 
> Cc: <stable@vger.kernel.org> # v5.2
> Fixes: a0ac778eb82c ("mtd: rawnand: ingenic: Add support for the JZ4740")
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
> index 0e9d426fe4f2..b18861bdcdc8 100644
> --- a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
> +++ b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
> @@ -32,6 +32,7 @@ struct jz_soc_info {
>  	unsigned long addr_offset;
>  	unsigned long cmd_offset;
>  	const struct mtd_ooblayout_ops *oob_layout;
> +	bool oob_first;
>  };
>  
>  struct ingenic_nand_cs {
> @@ -240,6 +241,9 @@ static int ingenic_nand_attach_chip(struct nand_chip *chip)
>  	if (chip->bbt_options & NAND_BBT_USE_FLASH)
>  		chip->bbt_options |= NAND_BBT_NO_OOB;
>  
> +	if (nfc->soc_info->oob_first)
> +		chip->ecc.read_page = nand_read_page_hwecc_oob_first;
> +
>  	/* For legacy reasons we use a different layout on the qi,lb60 board. */
>  	if (of_machine_is_compatible("qi,lb60"))
>  		mtd_set_ooblayout(mtd, &qi_lb60_ooblayout_ops);
> @@ -534,6 +538,7 @@ static const struct jz_soc_info jz4740_soc_info = {
>  	.data_offset = 0x00000000,
>  	.cmd_offset = 0x00008000,
>  	.addr_offset = 0x00010000,
> +	.oob_first = true,
>  };
>  
>  static const struct jz_soc_info jz4725b_soc_info = {


Thanks,
Miquèl

Re: [PATCH 2/3] mtd: rawnand: Export nand_read_page_hwecc_oob_first()

[Paul Cercueil]

Hi Miquel,

Le ven., oct. 15 2021 at 08:13:13 +0200, Miquel Raynal 
<miquel.raynal@bootlin.com> a écrit :
> Hi Paul,
> 
> paul@crapouillou.net wrote on Sat,  9 Oct 2021 20:49:51 +0200:
> 
>>  Move the function nand_read_page_hwecc_oob_first() (previously
>>  nand_davinci_read_page_hwecc_oob_first()) to nand_base.c, and 
>> export it
>>  as a GPL symbol, so that it can be used by more modules.
>> 
>>  Cc: <stable@vger.kernel.org> # v5.2
>>  Fixes: a0ac778eb82c ("mtd: rawnand: ingenic: Add support for the 
>> JZ4740")
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   drivers/mtd/nand/raw/davinci_nand.c | 70 
>> +----------------------------
>>   drivers/mtd/nand/raw/nand_base.c    | 69 
>> ++++++++++++++++++++++++++++
>>   include/linux/mtd/rawnand.h         |  2 +
>>   3 files changed, 72 insertions(+), 69 deletions(-)
>> 
>>  diff --git a/drivers/mtd/nand/raw/davinci_nand.c 
>> b/drivers/mtd/nand/raw/davinci_nand.c
>>  index 89de24d3bb7a..45fec8c192ab 100644
>>  --- a/drivers/mtd/nand/raw/davinci_nand.c
>>  +++ b/drivers/mtd/nand/raw/davinci_nand.c
>>  @@ -371,74 +371,6 @@ static int nand_davinci_correct_4bit(struct 
>> nand_chip *chip, u_char *data,
>>   	return corrected;
>>   }
>> 
>>  -/**
>>  - * nand_read_page_hwecc_oob_first - hw ecc, read oob first
>>  - * @chip: nand chip info structure
>>  - * @buf: buffer to store read data
>>  - * @oob_required: caller requires OOB data read to chip->oob_poi
>>  - * @page: page number to read
>>  - *
>>  - * Hardware ECC for large page chips, require OOB to be read 
>> first. For this
>>  - * ECC mode, the write_page method is re-used from ECC_HW. These 
>> methods
>>  - * read/write ECC from the OOB area, unlike the ECC_HW_SYNDROME 
>> support with
>>  - * multiple ECC steps, follows the "infix ECC" scheme and 
>> reads/writes ECC from
>>  - * the data area, by overwriting the NAND manufacturer bad block 
>> markings.
>>  - */
>>  -static int nand_davinci_read_page_hwecc_oob_first(struct nand_chip 
>> *chip,
>>  -						  uint8_t *buf,
>>  -						  int oob_required, int page)
>>  -{
>>  -	struct mtd_info *mtd = nand_to_mtd(chip);
>>  -	int i, eccsize = chip->ecc.size, ret;
>>  -	int eccbytes = chip->ecc.bytes;
>>  -	int eccsteps = chip->ecc.steps;
>>  -	uint8_t *p = buf;
>>  -	uint8_t *ecc_code = chip->ecc.code_buf;
>>  -	unsigned int max_bitflips = 0;
>>  -
>>  -	/* Read the OOB area first */
>>  -	ret = nand_read_oob_op(chip, page, 0, chip->oob_poi, 
>> mtd->oobsize);
>>  -	if (ret)
>>  -		return ret;
>>  -
>>  -	ret = nand_read_page_op(chip, page, 0, NULL, 0);
>>  -	if (ret)
>>  -		return ret;
>>  -
>>  -	ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0,
>>  -					 chip->ecc.total);
>>  -	if (ret)
>>  -		return ret;
>>  -
>>  -	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
>>  -		int stat;
>>  -
>>  -		chip->ecc.hwctl(chip, NAND_ECC_READ);
>>  -
>>  -		ret = nand_read_data_op(chip, p, eccsize, false, false);
>>  -		if (ret)
>>  -			return ret;
>>  -
>>  -		stat = chip->ecc.correct(chip, p, &ecc_code[i], NULL);
>>  -		if (stat == -EBADMSG &&
>>  -		    (chip->ecc.options & NAND_ECC_GENERIC_ERASED_CHECK)) {
>>  -			/* check for empty pages with bitflips */
>>  -			stat = nand_check_erased_ecc_chunk(p, eccsize,
>>  -							   &ecc_code[i],
>>  -							   eccbytes, NULL, 0,
>>  -							   chip->ecc.strength);
>>  -		}
>>  -
>>  -		if (stat < 0) {
>>  -			mtd->ecc_stats.failed++;
>>  -		} else {
>>  -			mtd->ecc_stats.corrected += stat;
>>  -			max_bitflips = max_t(unsigned int, max_bitflips, stat);
>>  -		}
>>  -	}
>>  -	return max_bitflips;
>>  -}
>>  -
>>   
>> /*----------------------------------------------------------------------*/
>> 
>>   /* An ECC layout for using 4-bit ECC with small-page flash, storing
>>  @@ -648,7 +580,7 @@ static int davinci_nand_attach_chip(struct 
>> nand_chip *chip)
>>   			} else if (chunks == 4 || chunks == 8) {
>>   				mtd_set_ooblayout(mtd,
>>   						  nand_get_large_page_ooblayout());
>>  -				chip->ecc.read_page = nand_davinci_read_page_hwecc_oob_first;
>>  +				chip->ecc.read_page = nand_read_page_hwecc_oob_first;
>>   			} else {
>>   				return -EIO;
>>   			}
>>  diff --git a/drivers/mtd/nand/raw/nand_base.c 
>> b/drivers/mtd/nand/raw/nand_base.c
>>  index 3d6c6e880520..cb5f343b9fa2 100644
>>  --- a/drivers/mtd/nand/raw/nand_base.c
>>  +++ b/drivers/mtd/nand/raw/nand_base.c
>>  @@ -3160,6 +3160,75 @@ static int nand_read_page_hwecc(struct 
>> nand_chip *chip, uint8_t *buf,
>>   	return max_bitflips;
>>   }
>> 
>>  +/**
>>  + * nand_read_page_hwecc_oob_first - Hardware ECC page read with ECC
>>  + *                                  data read from OOB area
>>  + * @chip: nand chip info structure
>>  + * @buf: buffer to store read data
>>  + * @oob_required: caller requires OOB data read to chip->oob_poi
>>  + * @page: page number to read
>>  + *
>>  + * Hardware ECC for large page chips, require OOB to be read 
>> first. For this
> 
> requires
> 
> With this ECC configuration?
> 
>>  + * ECC mode, the write_page method is re-used from ECC_HW. These 
>> methods
> 
> I do not understand this sentence nor the next one about syndrome. I
> believe it is related to your engine and should not leak into the 
> core.
> 
>>  + * read/write ECC from the OOB area, unlike the ECC_HW_SYNDROME 
>> support with
>>  + * multiple ECC steps, follows the "infix ECC" scheme and 
>> reads/writes ECC from
>>  + * the data area, by overwriting the NAND manufacturer bad block 
>> markings.
> 
> That's a sentence I don't like. What do you mean exactly?
> 
> What "Infix ECC" scheme is?
> 
> Do you mean that unlike the syndrome  mode it *does not* overwrite the
> BBM ?

I don't mean anything. I did not write that comment. I just moved the 
function verbatim with no changes. If something needs to be fixed, then 
it needs to be fixed before/after this patch.

>>  + */
>>  +int nand_read_page_hwecc_oob_first(struct nand_chip *chip, uint8_t 
>> *buf,
>>  +				   int oob_required, int page)
>>  +{
>>  +	struct mtd_info *mtd = nand_to_mtd(chip);
>>  +	int i, eccsize = chip->ecc.size, ret;
>>  +	int eccbytes = chip->ecc.bytes;
>>  +	int eccsteps = chip->ecc.steps;
>>  +	uint8_t *p = buf;
>>  +	uint8_t *ecc_code = chip->ecc.code_buf;
>>  +	unsigned int max_bitflips = 0;
>>  +
>>  +	/* Read the OOB area first */
>>  +	ret = nand_read_oob_op(chip, page, 0, chip->oob_poi, 
>> mtd->oobsize);
>>  +	if (ret)
>>  +		return ret;
>>  +
>>  +	ret = nand_read_page_op(chip, page, 0, NULL, 0);
> 
> Definitely not, your are requesting the chip to do the read_page
> operation twice. You only need a nand_change_read_column I believe.

Again, this code is just being moved around - don't shoot the messenger 
:)

>>  +	if (ret)
>>  +		return ret;
>>  +
>>  +	ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0,
>>  +					 chip->ecc.total);
>>  +	if (ret)
>>  +		return ret;
>>  +
>>  +	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
>>  +		int stat;
>>  +
>>  +		chip->ecc.hwctl(chip, NAND_ECC_READ);
>>  +
>>  +		ret = nand_read_data_op(chip, p, eccsize, false, false);
>>  +		if (ret)
>>  +			return ret;
>>  +
>>  +		stat = chip->ecc.correct(chip, p, &ecc_code[i], NULL);
>>  +		if (stat == -EBADMSG &&
>>  +		    (chip->ecc.options & NAND_ECC_GENERIC_ERASED_CHECK)) {
>>  +			/* check for empty pages with bitflips */
>>  +			stat = nand_check_erased_ecc_chunk(p, eccsize,
>>  +							   &ecc_code[i],
>>  +							   eccbytes, NULL, 0,
>>  +							   chip->ecc.strength);
>>  +		}
>>  +
>>  +		if (stat < 0) {
>>  +			mtd->ecc_stats.failed++;
>>  +		} else {
>>  +			mtd->ecc_stats.corrected += stat;
>>  +			max_bitflips = max_t(unsigned int, max_bitflips, stat);
>>  +		}
>>  +	}
>>  +	return max_bitflips;
>>  +}
>>  +EXPORT_SYMBOL_GPL(nand_read_page_hwecc_oob_first);
>>  +
>>   /**
>>    * nand_read_page_syndrome - [REPLACEABLE] hardware ECC syndrome 
>> based page read
>>    * @chip: nand chip info structure
>>  diff --git a/include/linux/mtd/rawnand.h 
>> b/include/linux/mtd/rawnand.h
>>  index b2f9dd3cbd69..5b88cd51fadb 100644
>>  --- a/include/linux/mtd/rawnand.h
>>  +++ b/include/linux/mtd/rawnand.h
>>  @@ -1539,6 +1539,8 @@ int nand_read_data_op(struct nand_chip *chip, 
>> void *buf, unsigned int len,
>>   		      bool force_8bit, bool check_only);
>>   int nand_write_data_op(struct nand_chip *chip, const void *buf,
>>   		       unsigned int len, bool force_8bit);
>>  +int nand_read_page_hwecc_oob_first(struct nand_chip *chip, uint8_t 
>> *buf,
>>  +				   int oob_required, int page);
> 
> You certainly want to add this symbol closer to the other read/write
> page helpers?

Where would that be? The other read/write page helpers are all "static" 
so they don't appear in any header.

Cheers,
-Paul

>> 
>>   /* Scan and identify a NAND device */
>>   int nand_scan_with_ids(struct nand_chip *chip, unsigned int 
>> max_chips,
> 
> 
> Thanks,
> Miquèl

Re: [PATCH 2/3] mtd: rawnand: Export nand_read_page_hwecc_oob_first()

[Miquel Raynal]

Hi Paul,

paul@crapouillou.net wrote on Fri, 15 Oct 2021 09:38:31 +0100:

> Hi Miquel,
> 
> Le ven., oct. 15 2021 at 08:13:13 +0200, Miquel Raynal <miquel.raynal@bootlin.com> a écrit :
> > Hi Paul,
> > 
> > paul@crapouillou.net wrote on Sat,  9 Oct 2021 20:49:51 +0200:
> >   
> >>  Move the function nand_read_page_hwecc_oob_first() (previously
> >>  nand_davinci_read_page_hwecc_oob_first()) to nand_base.c, and >> export it
> >>  as a GPL symbol, so that it can be used by more modules.  
> >> >>  Cc: <stable@vger.kernel.org> # v5.2  
> >>  Fixes: a0ac778eb82c ("mtd: rawnand: ingenic: Add support for the >> JZ4740")
> >>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> >>  ---
> >>   drivers/mtd/nand/raw/davinci_nand.c | 70 >> +----------------------------
> >>   drivers/mtd/nand/raw/nand_base.c    | 69 >> ++++++++++++++++++++++++++++
> >>   include/linux/mtd/rawnand.h         |  2 +
> >>   3 files changed, 72 insertions(+), 69 deletions(-)  
> >> >>  diff --git a/drivers/mtd/nand/raw/davinci_nand.c >> b/drivers/mtd/nand/raw/davinci_nand.c  
> >>  index 89de24d3bb7a..45fec8c192ab 100644
> >>  --- a/drivers/mtd/nand/raw/davinci_nand.c
> >>  +++ b/drivers/mtd/nand/raw/davinci_nand.c
> >>  @@ -371,74 +371,6 @@ static int nand_davinci_correct_4bit(struct >> nand_chip *chip, u_char *data,
> >>   	return corrected;
> >>   }  
> >> >>  -/**  
> >>  - * nand_read_page_hwecc_oob_first - hw ecc, read oob first
> >>  - * @chip: nand chip info structure
> >>  - * @buf: buffer to store read data
> >>  - * @oob_required: caller requires OOB data read to chip->oob_poi
> >>  - * @page: page number to read
> >>  - *
> >>  - * Hardware ECC for large page chips, require OOB to be read >> first. For this
> >>  - * ECC mode, the write_page method is re-used from ECC_HW. These >> methods
> >>  - * read/write ECC from the OOB area, unlike the ECC_HW_SYNDROME >> support with
> >>  - * multiple ECC steps, follows the "infix ECC" scheme and >> reads/writes ECC from
> >>  - * the data area, by overwriting the NAND manufacturer bad block >> markings.
> >>  - */
> >>  -static int nand_davinci_read_page_hwecc_oob_first(struct nand_chip >> *chip,
> >>  -						  uint8_t *buf,
> >>  -						  int oob_required, int page)
> >>  -{
> >>  -	struct mtd_info *mtd = nand_to_mtd(chip);
> >>  -	int i, eccsize = chip->ecc.size, ret;
> >>  -	int eccbytes = chip->ecc.bytes;
> >>  -	int eccsteps = chip->ecc.steps;
> >>  -	uint8_t *p = buf;
> >>  -	uint8_t *ecc_code = chip->ecc.code_buf;
> >>  -	unsigned int max_bitflips = 0;
> >>  -
> >>  -	/* Read the OOB area first */
> >>  -	ret = nand_read_oob_op(chip, page, 0, chip->oob_poi, >> mtd->oobsize);
> >>  -	if (ret)
> >>  -		return ret;
> >>  -
> >>  -	ret = nand_read_page_op(chip, page, 0, NULL, 0);
> >>  -	if (ret)
> >>  -		return ret;
> >>  -
> >>  -	ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0,
> >>  -					 chip->ecc.total);
> >>  -	if (ret)
> >>  -		return ret;
> >>  -
> >>  -	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
> >>  -		int stat;
> >>  -
> >>  -		chip->ecc.hwctl(chip, NAND_ECC_READ);
> >>  -
> >>  -		ret = nand_read_data_op(chip, p, eccsize, false, false);
> >>  -		if (ret)
> >>  -			return ret;
> >>  -
> >>  -		stat = chip->ecc.correct(chip, p, &ecc_code[i], NULL);
> >>  -		if (stat == -EBADMSG &&
> >>  -		    (chip->ecc.options & NAND_ECC_GENERIC_ERASED_CHECK)) {
> >>  -			/* check for empty pages with bitflips */
> >>  -			stat = nand_check_erased_ecc_chunk(p, eccsize,
> >>  -							   &ecc_code[i],
> >>  -							   eccbytes, NULL, 0,
> >>  -							   chip->ecc.strength);
> >>  -		}
> >>  -
> >>  -		if (stat < 0) {
> >>  -			mtd->ecc_stats.failed++;
> >>  -		} else {
> >>  -			mtd->ecc_stats.corrected += stat;
> >>  -			max_bitflips = max_t(unsigned int, max_bitflips, stat);
> >>  -		}
> >>  -	}
> >>  -	return max_bitflips;
> >>  -}
> >>  -  
> >>   >> /*----------------------------------------------------------------------  
> */
> >> >>   /* An ECC layout for using 4-bit ECC with small-page flash, storing  
> >>  @@ -648,7 +580,7 @@ static int davinci_nand_attach_chip(struct >> nand_chip *chip)
> >>   			} else if (chunks == 4 || chunks == 8) {
> >>   				mtd_set_ooblayout(mtd,
> >>   						  nand_get_large_page_ooblayout());
> >>  -				chip->ecc.read_page = nand_davinci_read_page_hwecc_oob_first;
> >>  +				chip->ecc.read_page = nand_read_page_hwecc_oob_first;
> >>   			} else {
> >>   				return -EIO;
> >>   			}
> >>  diff --git a/drivers/mtd/nand/raw/nand_base.c >> b/drivers/mtd/nand/raw/nand_base.c
> >>  index 3d6c6e880520..cb5f343b9fa2 100644
> >>  --- a/drivers/mtd/nand/raw/nand_base.c
> >>  +++ b/drivers/mtd/nand/raw/nand_base.c
> >>  @@ -3160,6 +3160,75 @@ static int nand_read_page_hwecc(struct >> nand_chip *chip, uint8_t *buf,
> >>   	return max_bitflips;
> >>   }  
> >> >>  +/**  
> >>  + * nand_read_page_hwecc_oob_first - Hardware ECC page read with ECC
> >>  + *                                  data read from OOB area
> >>  + * @chip: nand chip info structure
> >>  + * @buf: buffer to store read data
> >>  + * @oob_required: caller requires OOB data read to chip->oob_poi
> >>  + * @page: page number to read
> >>  + *
> >>  + * Hardware ECC for large page chips, require OOB to be read >> first. For this  
> > 
> > requires
> > 
> > With this ECC configuration?
> >   
> >>  + * ECC mode, the write_page method is re-used from ECC_HW. These >> methods  
> > 
> > I do not understand this sentence nor the next one about syndrome. I
> > believe it is related to your engine and should not leak into the > core.
> >   
> >>  + * read/write ECC from the OOB area, unlike the ECC_HW_SYNDROME >> support with
> >>  + * multiple ECC steps, follows the "infix ECC" scheme and >> reads/writes ECC from
> >>  + * the data area, by overwriting the NAND manufacturer bad block >> markings.  
> > 
> > That's a sentence I don't like. What do you mean exactly?
> > 
> > What "Infix ECC" scheme is?
> > 
> > Do you mean that unlike the syndrome  mode it *does not* overwrite the
> > BBM ?  
> 
> I don't mean anything. I did not write that comment. I just moved the function verbatim with no changes. If something needs to be fixed, then it needs to be fixed before/after this patch.

Well, this comment should be adapted because as-is I don't think it's
wise to move it around.

> 
> >>  + */
> >>  +int nand_read_page_hwecc_oob_first(struct nand_chip *chip, uint8_t >> *buf,
> >>  +				   int oob_required, int page)
> >>  +{
> >>  +	struct mtd_info *mtd = nand_to_mtd(chip);
> >>  +	int i, eccsize = chip->ecc.size, ret;
> >>  +	int eccbytes = chip->ecc.bytes;
> >>  +	int eccsteps = chip->ecc.steps;
> >>  +	uint8_t *p = buf;
> >>  +	uint8_t *ecc_code = chip->ecc.code_buf;
> >>  +	unsigned int max_bitflips = 0;
> >>  +
> >>  +	/* Read the OOB area first */
> >>  +	ret = nand_read_oob_op(chip, page, 0, chip->oob_poi, >> mtd->oobsize);
> >>  +	if (ret)
> >>  +		return ret;
> >>  +
> >>  +	ret = nand_read_page_op(chip, page, 0, NULL, 0);  
> > 
> > Definitely not, your are requesting the chip to do the read_page
> > operation twice. You only need a nand_change_read_column I believe.  
> 
> Again, this code is just being moved around - don't shoot the messenger :)

haha

Well, now you touch the core, so I need to be more careful, and the
code is definitely wrong, so even if we don't move that code off, you
definitely want to fix it in order to improve your performances.

> >>  +	if (ret)
> >>  +		return ret;
> >>  +
> >>  +	ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0,
> >>  +					 chip->ecc.total);
> >>  +	if (ret)
> >>  +		return ret;
> >>  +
> >>  +	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
> >>  +		int stat;
> >>  +
> >>  +		chip->ecc.hwctl(chip, NAND_ECC_READ);
> >>  +
> >>  +		ret = nand_read_data_op(chip, p, eccsize, false, false);
> >>  +		if (ret)
> >>  +			return ret;
> >>  +
> >>  +		stat = chip->ecc.correct(chip, p, &ecc_code[i], NULL);
> >>  +		if (stat == -EBADMSG &&
> >>  +		    (chip->ecc.options & NAND_ECC_GENERIC_ERASED_CHECK)) {
> >>  +			/* check for empty pages with bitflips */
> >>  +			stat = nand_check_erased_ecc_chunk(p, eccsize,
> >>  +							   &ecc_code[i],
> >>  +							   eccbytes, NULL, 0,
> >>  +							   chip->ecc.strength);
> >>  +		}
> >>  +
> >>  +		if (stat < 0) {
> >>  +			mtd->ecc_stats.failed++;
> >>  +		} else {
> >>  +			mtd->ecc_stats.corrected += stat;
> >>  +			max_bitflips = max_t(unsigned int, max_bitflips, stat);
> >>  +		}
> >>  +	}
> >>  +	return max_bitflips;
> >>  +}
> >>  +EXPORT_SYMBOL_GPL(nand_read_page_hwecc_oob_first);
> >>  +
> >>   /**
> >>    * nand_read_page_syndrome - [REPLACEABLE] hardware ECC syndrome >> based page read
> >>    * @chip: nand chip info structure
> >>  diff --git a/include/linux/mtd/rawnand.h >> b/include/linux/mtd/rawnand.h
> >>  index b2f9dd3cbd69..5b88cd51fadb 100644
> >>  --- a/include/linux/mtd/rawnand.h
> >>  +++ b/include/linux/mtd/rawnand.h
> >>  @@ -1539,6 +1539,8 @@ int nand_read_data_op(struct nand_chip *chip, >> void *buf, unsigned int len,
> >>   		      bool force_8bit, bool check_only);
> >>   int nand_write_data_op(struct nand_chip *chip, const void *buf,
> >>   		       unsigned int len, bool force_8bit);
> >>  +int nand_read_page_hwecc_oob_first(struct nand_chip *chip, uint8_t >> *buf,
> >>  +				   int oob_required, int page);  
> > 
> > You certainly want to add this symbol closer to the other read/write
> > page helpers?  
> 
> Where would that be? The other read/write page helpers are all "static" so they don't appear in any header.

I believe we should keep this header local as long as there are no
other users.


Thanks,
Miquèl

Re: [PATCH 2/3] mtd: rawnand: Export nand_read_page_hwecc_oob_first()

[Paul Cercueil]

Le ven., oct. 15 2021 at 10:51:46 +0200, Miquel Raynal 
<miquel.raynal@bootlin.com> a écrit :
> Hi Paul,
> 
> paul@crapouillou.net wrote on Fri, 15 Oct 2021 09:38:31 +0100:
> 
>>  Hi Miquel,
>> 
>>  Le ven., oct. 15 2021 at 08:13:13 +0200, Miquel Raynal 
>> <miquel.raynal@bootlin.com> a écrit :
>>  > Hi Paul,
>>  >
>>  > paul@crapouillou.net wrote on Sat,  9 Oct 2021 20:49:51 +0200:
>>  >
>>  >>  Move the function nand_read_page_hwecc_oob_first() (previously
>>  >>  nand_davinci_read_page_hwecc_oob_first()) to nand_base.c, and 
>> >> export it
>>  >>  as a GPL symbol, so that it can be used by more modules.
>>  >> >>  Cc: <stable@vger.kernel.org> # v5.2
>>  >>  Fixes: a0ac778eb82c ("mtd: rawnand: ingenic: Add support for 
>> the >> JZ4740")
>>  >>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  >>  ---
>>  >>   drivers/mtd/nand/raw/davinci_nand.c | 70 >> 
>> +----------------------------
>>  >>   drivers/mtd/nand/raw/nand_base.c    | 69 >> 
>> ++++++++++++++++++++++++++++
>>  >>   include/linux/mtd/rawnand.h         |  2 +
>>  >>   3 files changed, 72 insertions(+), 69 deletions(-)
>>  >> >>  diff --git a/drivers/mtd/nand/raw/davinci_nand.c >> 
>> b/drivers/mtd/nand/raw/davinci_nand.c
>>  >>  index 89de24d3bb7a..45fec8c192ab 100644
>>  >>  --- a/drivers/mtd/nand/raw/davinci_nand.c
>>  >>  +++ b/drivers/mtd/nand/raw/davinci_nand.c
>>  >>  @@ -371,74 +371,6 @@ static int 
>> nand_davinci_correct_4bit(struct >> nand_chip *chip, u_char *data,
>>  >>   	return corrected;
>>  >>   }
>>  >> >>  -/**
>>  >>  - * nand_read_page_hwecc_oob_first - hw ecc, read oob first
>>  >>  - * @chip: nand chip info structure
>>  >>  - * @buf: buffer to store read data
>>  >>  - * @oob_required: caller requires OOB data read to 
>> chip->oob_poi
>>  >>  - * @page: page number to read
>>  >>  - *
>>  >>  - * Hardware ECC for large page chips, require OOB to be read 
>> >> first. For this
>>  >>  - * ECC mode, the write_page method is re-used from ECC_HW. 
>> These >> methods
>>  >>  - * read/write ECC from the OOB area, unlike the 
>> ECC_HW_SYNDROME >> support with
>>  >>  - * multiple ECC steps, follows the "infix ECC" scheme and >> 
>> reads/writes ECC from
>>  >>  - * the data area, by overwriting the NAND manufacturer bad 
>> block >> markings.
>>  >>  - */
>>  >>  -static int nand_davinci_read_page_hwecc_oob_first(struct 
>> nand_chip >> *chip,
>>  >>  -						  uint8_t *buf,
>>  >>  -						  int oob_required, int page)
>>  >>  -{
>>  >>  -	struct mtd_info *mtd = nand_to_mtd(chip);
>>  >>  -	int i, eccsize = chip->ecc.size, ret;
>>  >>  -	int eccbytes = chip->ecc.bytes;
>>  >>  -	int eccsteps = chip->ecc.steps;
>>  >>  -	uint8_t *p = buf;
>>  >>  -	uint8_t *ecc_code = chip->ecc.code_buf;
>>  >>  -	unsigned int max_bitflips = 0;
>>  >>  -
>>  >>  -	/* Read the OOB area first */
>>  >>  -	ret = nand_read_oob_op(chip, page, 0, chip->oob_poi, >> 
>> mtd->oobsize);
>>  >>  -	if (ret)
>>  >>  -		return ret;
>>  >>  -
>>  >>  -	ret = nand_read_page_op(chip, page, 0, NULL, 0);
>>  >>  -	if (ret)
>>  >>  -		return ret;
>>  >>  -
>>  >>  -	ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, 
>> chip->oob_poi, 0,
>>  >>  -					 chip->ecc.total);
>>  >>  -	if (ret)
>>  >>  -		return ret;
>>  >>  -
>>  >>  -	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += 
>> eccsize) {
>>  >>  -		int stat;
>>  >>  -
>>  >>  -		chip->ecc.hwctl(chip, NAND_ECC_READ);
>>  >>  -
>>  >>  -		ret = nand_read_data_op(chip, p, eccsize, false, false);
>>  >>  -		if (ret)
>>  >>  -			return ret;
>>  >>  -
>>  >>  -		stat = chip->ecc.correct(chip, p, &ecc_code[i], NULL);
>>  >>  -		if (stat == -EBADMSG &&
>>  >>  -		    (chip->ecc.options & NAND_ECC_GENERIC_ERASED_CHECK)) {
>>  >>  -			/* check for empty pages with bitflips */
>>  >>  -			stat = nand_check_erased_ecc_chunk(p, eccsize,
>>  >>  -							   &ecc_code[i],
>>  >>  -							   eccbytes, NULL, 0,
>>  >>  -							   chip->ecc.strength);
>>  >>  -		}
>>  >>  -
>>  >>  -		if (stat < 0) {
>>  >>  -			mtd->ecc_stats.failed++;
>>  >>  -		} else {
>>  >>  -			mtd->ecc_stats.corrected += stat;
>>  >>  -			max_bitflips = max_t(unsigned int, max_bitflips, stat);
>>  >>  -		}
>>  >>  -	}
>>  >>  -	return max_bitflips;
>>  >>  -}
>>  >>  -
>>  >>   >> 
>> /*----------------------------------------------------------------------
>>  */
>>  >> >>   /* An ECC layout for using 4-bit ECC with small-page flash, 
>> storing
>>  >>  @@ -648,7 +580,7 @@ static int davinci_nand_attach_chip(struct 
>> >> nand_chip *chip)
>>  >>   			} else if (chunks == 4 || chunks == 8) {
>>  >>   				mtd_set_ooblayout(mtd,
>>  >>   						  nand_get_large_page_ooblayout());
>>  >>  -				chip->ecc.read_page = 
>> nand_davinci_read_page_hwecc_oob_first;
>>  >>  +				chip->ecc.read_page = nand_read_page_hwecc_oob_first;
>>  >>   			} else {
>>  >>   				return -EIO;
>>  >>   			}
>>  >>  diff --git a/drivers/mtd/nand/raw/nand_base.c >> 
>> b/drivers/mtd/nand/raw/nand_base.c
>>  >>  index 3d6c6e880520..cb5f343b9fa2 100644
>>  >>  --- a/drivers/mtd/nand/raw/nand_base.c
>>  >>  +++ b/drivers/mtd/nand/raw/nand_base.c
>>  >>  @@ -3160,6 +3160,75 @@ static int nand_read_page_hwecc(struct 
>> >> nand_chip *chip, uint8_t *buf,
>>  >>   	return max_bitflips;
>>  >>   }
>>  >> >>  +/**
>>  >>  + * nand_read_page_hwecc_oob_first - Hardware ECC page read 
>> with ECC
>>  >>  + *                                  data read from OOB area
>>  >>  + * @chip: nand chip info structure
>>  >>  + * @buf: buffer to store read data
>>  >>  + * @oob_required: caller requires OOB data read to 
>> chip->oob_poi
>>  >>  + * @page: page number to read
>>  >>  + *
>>  >>  + * Hardware ECC for large page chips, require OOB to be read 
>> >> first. For this
>>  >
>>  > requires
>>  >
>>  > With this ECC configuration?
>>  >
>>  >>  + * ECC mode, the write_page method is re-used from ECC_HW. 
>> These >> methods
>>  >
>>  > I do not understand this sentence nor the next one about 
>> syndrome. I
>>  > believe it is related to your engine and should not leak into the 
>> > core.
>>  >
>>  >>  + * read/write ECC from the OOB area, unlike the 
>> ECC_HW_SYNDROME >> support with
>>  >>  + * multiple ECC steps, follows the "infix ECC" scheme and >> 
>> reads/writes ECC from
>>  >>  + * the data area, by overwriting the NAND manufacturer bad 
>> block >> markings.
>>  >
>>  > That's a sentence I don't like. What do you mean exactly?
>>  >
>>  > What "Infix ECC" scheme is?
>>  >
>>  > Do you mean that unlike the syndrome  mode it *does not* 
>> overwrite the
>>  > BBM ?
>> 
>>  I don't mean anything. I did not write that comment. I just moved 
>> the function verbatim with no changes. If something needs to be 
>> fixed, then it needs to be fixed before/after this patch.
> 
> Well, this comment should be adapted because as-is I don't think it's
> wise to move it around.

OK.

I think it says that BBM can be overwritten with this configuration, 
but that would be if the OOB layout covers the BBM area.

>> 
>>  >>  + */
>>  >>  +int nand_read_page_hwecc_oob_first(struct nand_chip *chip, 
>> uint8_t >> *buf,
>>  >>  +				   int oob_required, int page)
>>  >>  +{
>>  >>  +	struct mtd_info *mtd = nand_to_mtd(chip);
>>  >>  +	int i, eccsize = chip->ecc.size, ret;
>>  >>  +	int eccbytes = chip->ecc.bytes;
>>  >>  +	int eccsteps = chip->ecc.steps;
>>  >>  +	uint8_t *p = buf;
>>  >>  +	uint8_t *ecc_code = chip->ecc.code_buf;
>>  >>  +	unsigned int max_bitflips = 0;
>>  >>  +
>>  >>  +	/* Read the OOB area first */
>>  >>  +	ret = nand_read_oob_op(chip, page, 0, chip->oob_poi, >> 
>> mtd->oobsize);
>>  >>  +	if (ret)
>>  >>  +		return ret;
>>  >>  +
>>  >>  +	ret = nand_read_page_op(chip, page, 0, NULL, 0);
>>  >
>>  > Definitely not, your are requesting the chip to do the read_page
>>  > operation twice. You only need a nand_change_read_column I 
>> believe.
>> 
>>  Again, this code is just being moved around - don't shoot the 
>> messenger :)
> 
> haha
> 
> Well, now you touch the core, so I need to be more careful, and the
> code is definitely wrong, so even if we don't move that code off, you
> definitely want to fix it in order to improve your performances.

I don't see the read_page being done twice?

There's one read_oob, one read_page, then read_data in the loop.

>>  >>  +	if (ret)
>>  >>  +		return ret;
>>  >>  +
>>  >>  +	ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, 
>> chip->oob_poi, 0,
>>  >>  +					 chip->ecc.total);
>>  >>  +	if (ret)
>>  >>  +		return ret;
>>  >>  +
>>  >>  +	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += 
>> eccsize) {
>>  >>  +		int stat;
>>  >>  +
>>  >>  +		chip->ecc.hwctl(chip, NAND_ECC_READ);
>>  >>  +
>>  >>  +		ret = nand_read_data_op(chip, p, eccsize, false, false);
>>  >>  +		if (ret)
>>  >>  +			return ret;
>>  >>  +
>>  >>  +		stat = chip->ecc.correct(chip, p, &ecc_code[i], NULL);
>>  >>  +		if (stat == -EBADMSG &&
>>  >>  +		    (chip->ecc.options & NAND_ECC_GENERIC_ERASED_CHECK)) {
>>  >>  +			/* check for empty pages with bitflips */
>>  >>  +			stat = nand_check_erased_ecc_chunk(p, eccsize,
>>  >>  +							   &ecc_code[i],
>>  >>  +							   eccbytes, NULL, 0,
>>  >>  +							   chip->ecc.strength);
>>  >>  +		}
>>  >>  +
>>  >>  +		if (stat < 0) {
>>  >>  +			mtd->ecc_stats.failed++;
>>  >>  +		} else {
>>  >>  +			mtd->ecc_stats.corrected += stat;
>>  >>  +			max_bitflips = max_t(unsigned int, max_bitflips, stat);
>>  >>  +		}
>>  >>  +	}
>>  >>  +	return max_bitflips;
>>  >>  +}
>>  >>  +EXPORT_SYMBOL_GPL(nand_read_page_hwecc_oob_first);
>>  >>  +
>>  >>   /**
>>  >>    * nand_read_page_syndrome - [REPLACEABLE] hardware ECC 
>> syndrome >> based page read
>>  >>    * @chip: nand chip info structure
>>  >>  diff --git a/include/linux/mtd/rawnand.h >> 
>> b/include/linux/mtd/rawnand.h
>>  >>  index b2f9dd3cbd69..5b88cd51fadb 100644
>>  >>  --- a/include/linux/mtd/rawnand.h
>>  >>  +++ b/include/linux/mtd/rawnand.h
>>  >>  @@ -1539,6 +1539,8 @@ int nand_read_data_op(struct nand_chip 
>> *chip, >> void *buf, unsigned int len,
>>  >>   		      bool force_8bit, bool check_only);
>>  >>   int nand_write_data_op(struct nand_chip *chip, const void *buf,
>>  >>   		       unsigned int len, bool force_8bit);
>>  >>  +int nand_read_page_hwecc_oob_first(struct nand_chip *chip, 
>> uint8_t >> *buf,
>>  >>  +				   int oob_required, int page);
>>  >
>>  > You certainly want to add this symbol closer to the other 
>> read/write
>>  > page helpers?
>> 
>>  Where would that be? The other read/write page helpers are all 
>> "static" so they don't appear in any header.
> 
> I believe we should keep this header local as long as there are no
> other users.

I'll move it to internal.h then.

Cheers,
-Paul

Re: [PATCH 2/3] mtd: rawnand: Export nand_read_page_hwecc_oob_first()

[Miquel Raynal]

Hi Paul,

> >>  */  
> >>  >> >>   /* An ECC layout for using 4-bit ECC with small-page flash, >> storing  
> >>  >>  @@ -648,7 +580,7 @@ static int davinci_nand_attach_chip(struct >> >> nand_chip *chip)
> >>  >>   			} else if (chunks == 4 || chunks == 8) {
> >>  >>   				mtd_set_ooblayout(mtd,
> >>  >>   						  nand_get_large_page_ooblayout());
> >>  >>  -				chip->ecc.read_page = >> nand_davinci_read_page_hwecc_oob_first;
> >>  >>  +				chip->ecc.read_page = nand_read_page_hwecc_oob_first;
> >>  >>   			} else {
> >>  >>   				return -EIO;
> >>  >>   			}
> >>  >>  diff --git a/drivers/mtd/nand/raw/nand_base.c >> >> b/drivers/mtd/nand/raw/nand_base.c
> >>  >>  index 3d6c6e880520..cb5f343b9fa2 100644
> >>  >>  --- a/drivers/mtd/nand/raw/nand_base.c
> >>  >>  +++ b/drivers/mtd/nand/raw/nand_base.c
> >>  >>  @@ -3160,6 +3160,75 @@ static int nand_read_page_hwecc(struct >> >> nand_chip *chip, uint8_t *buf,
> >>  >>   	return max_bitflips;
> >>  >>   }  
> >>  >> >>  +/**  
> >>  >>  + * nand_read_page_hwecc_oob_first - Hardware ECC page read >> with ECC
> >>  >>  + *                                  data read from OOB area
> >>  >>  + * @chip: nand chip info structure
> >>  >>  + * @buf: buffer to store read data
> >>  >>  + * @oob_required: caller requires OOB data read to >> chip->oob_poi
> >>  >>  + * @page: page number to read
> >>  >>  + *
> >>  >>  + * Hardware ECC for large page chips, require OOB to be read >> >> first. For this  
> >>  >
> >>  > requires
> >>  >
> >>  > With this ECC configuration?
> >>  >  
> >>  >>  + * ECC mode, the write_page method is re-used from ECC_HW. >> These >> methods  
> >>  >
> >>  > I do not understand this sentence nor the next one about >> syndrome. I
> >>  > believe it is related to your engine and should not leak into the >> > core.
> >>  >  
> >>  >>  + * read/write ECC from the OOB area, unlike the >> ECC_HW_SYNDROME >> support with
> >>  >>  + * multiple ECC steps, follows the "infix ECC" scheme and >> >> reads/writes ECC from
> >>  >>  + * the data area, by overwriting the NAND manufacturer bad >> block >> markings.  
> >>  >
> >>  > That's a sentence I don't like. What do you mean exactly?
> >>  >
> >>  > What "Infix ECC" scheme is?
> >>  >
> >>  > Do you mean that unlike the syndrome  mode it *does not* >> overwrite the
> >>  > BBM ?  
> >> >>  I don't mean anything. I did not write that comment. I just moved >> the function verbatim with no changes. If something needs to be >> fixed, then it needs to be fixed before/after this patch.  
> > 
> > Well, this comment should be adapted because as-is I don't think it's
> > wise to move it around.  
> 
> OK.
> 
> I think it says that BBM can be overwritten with this configuration, but that would be if the OOB layout covers the BBM area.

If the ooblayout prevents the BBM to be smatched I'm fine and this
sentence should disappear because it's misleading.

> >> >>  >>  + */  
> >>  >>  +int nand_read_page_hwecc_oob_first(struct nand_chip *chip, >> uint8_t >> *buf,
> >>  >>  +				   int oob_required, int page)
> >>  >>  +{
> >>  >>  +	struct mtd_info *mtd = nand_to_mtd(chip);
> >>  >>  +	int i, eccsize = chip->ecc.size, ret;
> >>  >>  +	int eccbytes = chip->ecc.bytes;
> >>  >>  +	int eccsteps = chip->ecc.steps;
> >>  >>  +	uint8_t *p = buf;
> >>  >>  +	uint8_t *ecc_code = chip->ecc.code_buf;
> >>  >>  +	unsigned int max_bitflips = 0;
> >>  >>  +
> >>  >>  +	/* Read the OOB area first */
> >>  >>  +	ret = nand_read_oob_op(chip, page, 0, chip->oob_poi, >> >> mtd->oobsize);
> >>  >>  +	if (ret)
> >>  >>  +		return ret;
> >>  >>  +
> >>  >>  +	ret = nand_read_page_op(chip, page, 0, NULL, 0);  
> >>  >
> >>  > Definitely not, your are requesting the chip to do the read_page
> >>  > operation twice. You only need a nand_change_read_column I >> believe.  
> >> >>  Again, this code is just being moved around - don't shoot the >> messenger :)  
> > 
> > haha
> > 
> > Well, now you touch the core, so I need to be more careful, and the
> > code is definitely wrong, so even if we don't move that code off, you
> > definitely want to fix it in order to improve your performances.  
> 
> I don't see the read_page being done twice?
> 
> There's one read_oob, one read_page, then read_data in the loop.

read_oob and read_page both end up sending READ0 and READSTART so
they do request the chip to perform an internal read twice. You
need this only once. The call to nand_read_page_op() should be a
nand_change_read_column() with no data requested. 

> >>  >>   /**
> >>  >>    * nand_read_page_syndrome - [REPLACEABLE] hardware ECC >> syndrome >> based page read
> >>  >>    * @chip: nand chip info structure
> >>  >>  diff --git a/include/linux/mtd/rawnand.h >> >> b/include/linux/mtd/rawnand.h
> >>  >>  index b2f9dd3cbd69..5b88cd51fadb 100644
> >>  >>  --- a/include/linux/mtd/rawnand.h
> >>  >>  +++ b/include/linux/mtd/rawnand.h
> >>  >>  @@ -1539,6 +1539,8 @@ int nand_read_data_op(struct nand_chip >> *chip, >> void *buf, unsigned int len,
> >>  >>   		      bool force_8bit, bool check_only);
> >>  >>   int nand_write_data_op(struct nand_chip *chip, const void *buf,
> >>  >>   		       unsigned int len, bool force_8bit);
> >>  >>  +int nand_read_page_hwecc_oob_first(struct nand_chip *chip, >> uint8_t >> *buf,
> >>  >>  +				   int oob_required, int page);  
> >>  >
> >>  > You certainly want to add this symbol closer to the other >> read/write
> >>  > page helpers?  
> >> >>  Where would that be? The other read/write page helpers are all >> "static" so they don't appear in any header.  
> > 
> > I believe we should keep this header local as long as there are no
> > other users.  
> 
> I'll move it to internal.h then.

Why do you want to put it there is there is only one user?

Thanks,
Miquèl

Re: [PATCH 2/3] mtd: rawnand: Export nand_read_page_hwecc_oob_first()

[Paul Cercueil]

Hi,

Le ven., oct. 15 2021 at 11:35:15 +0200, Miquel Raynal 
<miquel.raynal@bootlin.com> a écrit :
> Hi Paul,
> 
>>  >>  */
>>  >>  >> >>   /* An ECC layout for using 4-bit ECC with small-page 
>> flash, >> storing
>>  >>  >>  @@ -648,7 +580,7 @@ static int 
>> davinci_nand_attach_chip(struct >> >> nand_chip *chip)
>>  >>  >>   			} else if (chunks == 4 || chunks == 8) {
>>  >>  >>   				mtd_set_ooblayout(mtd,
>>  >>  >>   						  nand_get_large_page_ooblayout());
>>  >>  >>  -				chip->ecc.read_page = >> 
>> nand_davinci_read_page_hwecc_oob_first;
>>  >>  >>  +				chip->ecc.read_page = nand_read_page_hwecc_oob_first;
>>  >>  >>   			} else {
>>  >>  >>   				return -EIO;
>>  >>  >>   			}
>>  >>  >>  diff --git a/drivers/mtd/nand/raw/nand_base.c >> >> 
>> b/drivers/mtd/nand/raw/nand_base.c
>>  >>  >>  index 3d6c6e880520..cb5f343b9fa2 100644
>>  >>  >>  --- a/drivers/mtd/nand/raw/nand_base.c
>>  >>  >>  +++ b/drivers/mtd/nand/raw/nand_base.c
>>  >>  >>  @@ -3160,6 +3160,75 @@ static int 
>> nand_read_page_hwecc(struct >> >> nand_chip *chip, uint8_t *buf,
>>  >>  >>   	return max_bitflips;
>>  >>  >>   }
>>  >>  >> >>  +/**
>>  >>  >>  + * nand_read_page_hwecc_oob_first - Hardware ECC page read 
>> >> with ECC
>>  >>  >>  + *                                  data read from OOB area
>>  >>  >>  + * @chip: nand chip info structure
>>  >>  >>  + * @buf: buffer to store read data
>>  >>  >>  + * @oob_required: caller requires OOB data read to >> 
>> chip->oob_poi
>>  >>  >>  + * @page: page number to read
>>  >>  >>  + *
>>  >>  >>  + * Hardware ECC for large page chips, require OOB to be 
>> read >> >> first. For this
>>  >>  >
>>  >>  > requires
>>  >>  >
>>  >>  > With this ECC configuration?
>>  >>  >
>>  >>  >>  + * ECC mode, the write_page method is re-used from ECC_HW. 
>> >> These >> methods
>>  >>  >
>>  >>  > I do not understand this sentence nor the next one about >> 
>> syndrome. I
>>  >>  > believe it is related to your engine and should not leak into 
>> the >> > core.
>>  >>  >
>>  >>  >>  + * read/write ECC from the OOB area, unlike the >> 
>> ECC_HW_SYNDROME >> support with
>>  >>  >>  + * multiple ECC steps, follows the "infix ECC" scheme and 
>> >> >> reads/writes ECC from
>>  >>  >>  + * the data area, by overwriting the NAND manufacturer bad 
>> >> block >> markings.
>>  >>  >
>>  >>  > That's a sentence I don't like. What do you mean exactly?
>>  >>  >
>>  >>  > What "Infix ECC" scheme is?
>>  >>  >
>>  >>  > Do you mean that unlike the syndrome  mode it *does not* >> 
>> overwrite the
>>  >>  > BBM ?
>>  >> >>  I don't mean anything. I did not write that comment. I just 
>> moved >> the function verbatim with no changes. If something needs 
>> to be >> fixed, then it needs to be fixed before/after this patch.
>>  >
>>  > Well, this comment should be adapted because as-is I don't think 
>> it's
>>  > wise to move it around.
>> 
>>  OK.
>> 
>>  I think it says that BBM can be overwritten with this 
>> configuration, but that would be if the OOB layout covers the BBM 
>> area.
> 
> If the ooblayout prevents the BBM to be smatched I'm fine and this
> sentence should disappear because it's misleading.
> 
>>  >> >>  >>  + */
>>  >>  >>  +int nand_read_page_hwecc_oob_first(struct nand_chip *chip, 
>> >> uint8_t >> *buf,
>>  >>  >>  +				   int oob_required, int page)
>>  >>  >>  +{
>>  >>  >>  +	struct mtd_info *mtd = nand_to_mtd(chip);
>>  >>  >>  +	int i, eccsize = chip->ecc.size, ret;
>>  >>  >>  +	int eccbytes = chip->ecc.bytes;
>>  >>  >>  +	int eccsteps = chip->ecc.steps;
>>  >>  >>  +	uint8_t *p = buf;
>>  >>  >>  +	uint8_t *ecc_code = chip->ecc.code_buf;
>>  >>  >>  +	unsigned int max_bitflips = 0;
>>  >>  >>  +
>>  >>  >>  +	/* Read the OOB area first */
>>  >>  >>  +	ret = nand_read_oob_op(chip, page, 0, chip->oob_poi, >> 
>> >> mtd->oobsize);
>>  >>  >>  +	if (ret)
>>  >>  >>  +		return ret;
>>  >>  >>  +
>>  >>  >>  +	ret = nand_read_page_op(chip, page, 0, NULL, 0);
>>  >>  >
>>  >>  > Definitely not, your are requesting the chip to do the 
>> read_page
>>  >>  > operation twice. You only need a nand_change_read_column I >> 
>> believe.
>>  >> >>  Again, this code is just being moved around - don't shoot 
>> the >> messenger :)
>>  >
>>  > haha
>>  >
>>  > Well, now you touch the core, so I need to be more careful, and 
>> the
>>  > code is definitely wrong, so even if we don't move that code off, 
>> you
>>  > definitely want to fix it in order to improve your performances.
>> 
>>  I don't see the read_page being done twice?
>> 
>>  There's one read_oob, one read_page, then read_data in the loop.
> 
> read_oob and read_page both end up sending READ0 and READSTART so
> they do request the chip to perform an internal read twice. You
> need this only once. The call to nand_read_page_op() should be a
> nand_change_read_column() with no data requested.

OK.

> 
>>  >>  >>   /**
>>  >>  >>    * nand_read_page_syndrome - [REPLACEABLE] hardware ECC >> 
>> syndrome >> based page read
>>  >>  >>    * @chip: nand chip info structure
>>  >>  >>  diff --git a/include/linux/mtd/rawnand.h >> >> 
>> b/include/linux/mtd/rawnand.h
>>  >>  >>  index b2f9dd3cbd69..5b88cd51fadb 100644
>>  >>  >>  --- a/include/linux/mtd/rawnand.h
>>  >>  >>  +++ b/include/linux/mtd/rawnand.h
>>  >>  >>  @@ -1539,6 +1539,8 @@ int nand_read_data_op(struct 
>> nand_chip >> *chip, >> void *buf, unsigned int len,
>>  >>  >>   		      bool force_8bit, bool check_only);
>>  >>  >>   int nand_write_data_op(struct nand_chip *chip, const void 
>> *buf,
>>  >>  >>   		       unsigned int len, bool force_8bit);
>>  >>  >>  +int nand_read_page_hwecc_oob_first(struct nand_chip *chip, 
>> >> uint8_t >> *buf,
>>  >>  >>  +				   int oob_required, int page);
>>  >>  >
>>  >>  > You certainly want to add this symbol closer to the other >> 
>> read/write
>>  >>  > page helpers?
>>  >> >>  Where would that be? The other read/write page helpers are 
>> all >> "static" so they don't appear in any header.
>>  >
>>  > I believe we should keep this header local as long as there are no
>>  > other users.
>> 
>>  I'll move it to internal.h then.
> 
> Why do you want to put it there is there is only one user?

But there are two users: davinci_nand.c and (with patch [3/3]) 
ingenic/ingenic_nand_drv.c.

-Paul

Re: [PATCH 2/3] mtd: rawnand: Export nand_read_page_hwecc_oob_first()

[Miquel Raynal]

Hi Paul,

paul@crapouillou.net wrote on Fri, 15 Oct 2021 10:38:00 +0100:

> Hi,
> 
> Le ven., oct. 15 2021 at 11:35:15 +0200, Miquel Raynal <miquel.raynal@bootlin.com> a écrit :
> > Hi Paul,
> >   
> >>  >>  */  
> >>  >>  >> >>   /* An ECC layout for using 4-bit ECC with small-page >> flash, >> storing  
> >>  >>  >>  @@ -648,7 +580,7 @@ static int >> davinci_nand_attach_chip(struct >> >> nand_chip *chip)
> >>  >>  >>   			} else if (chunks == 4 || chunks == 8) {
> >>  >>  >>   				mtd_set_ooblayout(mtd,
> >>  >>  >>   						  nand_get_large_page_ooblayout());
> >>  >>  >>  -				chip->ecc.read_page = >> >> nand_davinci_read_page_hwecc_oob_first;
> >>  >>  >>  +				chip->ecc.read_page = nand_read_page_hwecc_oob_first;
> >>  >>  >>   			} else {
> >>  >>  >>   				return -EIO;
> >>  >>  >>   			}
> >>  >>  >>  diff --git a/drivers/mtd/nand/raw/nand_base.c >> >> >> b/drivers/mtd/nand/raw/nand_base.c
> >>  >>  >>  index 3d6c6e880520..cb5f343b9fa2 100644
> >>  >>  >>  --- a/drivers/mtd/nand/raw/nand_base.c
> >>  >>  >>  +++ b/drivers/mtd/nand/raw/nand_base.c
> >>  >>  >>  @@ -3160,6 +3160,75 @@ static int >> nand_read_page_hwecc(struct >> >> nand_chip *chip, uint8_t *buf,
> >>  >>  >>   	return max_bitflips;
> >>  >>  >>   }  
> >>  >>  >> >>  +/**  
> >>  >>  >>  + * nand_read_page_hwecc_oob_first - Hardware ECC page read >> >> with ECC
> >>  >>  >>  + *                                  data read from OOB area
> >>  >>  >>  + * @chip: nand chip info structure
> >>  >>  >>  + * @buf: buffer to store read data
> >>  >>  >>  + * @oob_required: caller requires OOB data read to >> >> chip->oob_poi
> >>  >>  >>  + * @page: page number to read
> >>  >>  >>  + *
> >>  >>  >>  + * Hardware ECC for large page chips, require OOB to be >> read >> >> first. For this  
> >>  >>  >
> >>  >>  > requires
> >>  >>  >
> >>  >>  > With this ECC configuration?
> >>  >>  >  
> >>  >>  >>  + * ECC mode, the write_page method is re-used from ECC_HW. >> >> These >> methods  
> >>  >>  >
> >>  >>  > I do not understand this sentence nor the next one about >> >> syndrome. I
> >>  >>  > believe it is related to your engine and should not leak into >> the >> > core.
> >>  >>  >  
> >>  >>  >>  + * read/write ECC from the OOB area, unlike the >> >> ECC_HW_SYNDROME >> support with
> >>  >>  >>  + * multiple ECC steps, follows the "infix ECC" scheme and >> >> >> reads/writes ECC from
> >>  >>  >>  + * the data area, by overwriting the NAND manufacturer bad >> >> block >> markings.  
> >>  >>  >
> >>  >>  > That's a sentence I don't like. What do you mean exactly?
> >>  >>  >
> >>  >>  > What "Infix ECC" scheme is?
> >>  >>  >
> >>  >>  > Do you mean that unlike the syndrome  mode it *does not* >> >> overwrite the
> >>  >>  > BBM ?  
> >>  >> >>  I don't mean anything. I did not write that comment. I just >> moved >> the function verbatim with no changes. If something needs >> to be >> fixed, then it needs to be fixed before/after this patch.  
> >>  >
> >>  > Well, this comment should be adapted because as-is I don't think >> it's
> >>  > wise to move it around.  
> >> >>  OK.
> >> >>  I think it says that BBM can be overwritten with this >> configuration, but that would be if the OOB layout covers the BBM >> area.  
> > 
> > If the ooblayout prevents the BBM to be smatched I'm fine and this
> > sentence should disappear because it's misleading.
> >   
> >>  >> >>  >>  + */  
> >>  >>  >>  +int nand_read_page_hwecc_oob_first(struct nand_chip *chip, >> >> uint8_t >> *buf,
> >>  >>  >>  +				   int oob_required, int page)
> >>  >>  >>  +{
> >>  >>  >>  +	struct mtd_info *mtd = nand_to_mtd(chip);
> >>  >>  >>  +	int i, eccsize = chip->ecc.size, ret;
> >>  >>  >>  +	int eccbytes = chip->ecc.bytes;
> >>  >>  >>  +	int eccsteps = chip->ecc.steps;
> >>  >>  >>  +	uint8_t *p = buf;
> >>  >>  >>  +	uint8_t *ecc_code = chip->ecc.code_buf;
> >>  >>  >>  +	unsigned int max_bitflips = 0;
> >>  >>  >>  +
> >>  >>  >>  +	/* Read the OOB area first */
> >>  >>  >>  +	ret = nand_read_oob_op(chip, page, 0, chip->oob_poi, >> >> >> mtd->oobsize);
> >>  >>  >>  +	if (ret)
> >>  >>  >>  +		return ret;
> >>  >>  >>  +
> >>  >>  >>  +	ret = nand_read_page_op(chip, page, 0, NULL, 0);  
> >>  >>  >
> >>  >>  > Definitely not, your are requesting the chip to do the >> read_page
> >>  >>  > operation twice. You only need a nand_change_read_column I >> >> believe.  
> >>  >> >>  Again, this code is just being moved around - don't shoot >> the >> messenger :)  
> >>  >
> >>  > haha
> >>  >
> >>  > Well, now you touch the core, so I need to be more careful, and >> the
> >>  > code is definitely wrong, so even if we don't move that code off, >> you
> >>  > definitely want to fix it in order to improve your performances.  
> >> >>  I don't see the read_page being done twice?
> >> >>  There's one read_oob, one read_page, then read_data in the loop.  
> > 
> > read_oob and read_page both end up sending READ0 and READSTART so
> > they do request the chip to perform an internal read twice. You
> > need this only once. The call to nand_read_page_op() should be a
> > nand_change_read_column() with no data requested.  
> 
> OK.
> 
> >   
> >>  >>  >>   /**
> >>  >>  >>    * nand_read_page_syndrome - [REPLACEABLE] hardware ECC >> >> syndrome >> based page read
> >>  >>  >>    * @chip: nand chip info structure
> >>  >>  >>  diff --git a/include/linux/mtd/rawnand.h >> >> >> b/include/linux/mtd/rawnand.h
> >>  >>  >>  index b2f9dd3cbd69..5b88cd51fadb 100644
> >>  >>  >>  --- a/include/linux/mtd/rawnand.h
> >>  >>  >>  +++ b/include/linux/mtd/rawnand.h
> >>  >>  >>  @@ -1539,6 +1539,8 @@ int nand_read_data_op(struct >> nand_chip >> *chip, >> void *buf, unsigned int len,
> >>  >>  >>   		      bool force_8bit, bool check_only);
> >>  >>  >>   int nand_write_data_op(struct nand_chip *chip, const void >> *buf,
> >>  >>  >>   		       unsigned int len, bool force_8bit);
> >>  >>  >>  +int nand_read_page_hwecc_oob_first(struct nand_chip *chip, >> >> uint8_t >> *buf,
> >>  >>  >>  +				   int oob_required, int page);  
> >>  >>  >
> >>  >>  > You certainly want to add this symbol closer to the other >> >> read/write
> >>  >>  > page helpers?  
> >>  >> >>  Where would that be? The other read/write page helpers are >> all >> "static" so they don't appear in any header.  
> >>  >
> >>  > I believe we should keep this header local as long as there are no
> >>  > other users.  
> >> >>  I'll move it to internal.h then.  
> > 
> > Why do you want to put it there is there is only one user?  
> 
> But there are two users: davinci_nand.c and (with patch [3/3]) ingenic/ingenic_nand_drv.c.

Oh right I missed that :)

Then please add two preparation patches which:
- fixes the comment (please reword it completely)
- avoid the double reading

And keep the location where you moved it (including the header) as-is.

Thanks,
Miquèl

Re: [PATCH 0/3] mtd: Ingenic NAND fix for JZ4740

[H. Nikolaus Schaller]

Hi Paul,

> Am 09.10.2021 um 20:49 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi,
> 
> Looks like NAND support has been broken on the JZ4740 SoC for a while;

Yes, I remember someone telling that something was fundamentally broken
and impossible to be fixed a while ago.

> it looks like it comes from the fact that the "hw_oob_first" mechanism
> was dropped from the NAND core and moved to the Davinci driver.
> 
> It turns out the JZ4740 SoC needs it too; I didn't notice it when
> writing the new ingenic-nand driver (to replace the old jz4740-nand
> driver) most likely because my Device Tree had the "nand-ecc-mode" set
> to "hw_oob_first".
> 
> I am not very sure about patch [1/3]; to me the original code does not
> make sense, and it didn't work out-of-the-box on the JZ4740 without it.
> By applying patch [1/3] the function nand_read_page_hwecc_oob_first()
> can be reused for the JZ4740 SoC as well. But I did not test patch [1/3]
> on Davinci.

would this also work for jz4780 NAND?

BR,
Nikolaus

Re: [PATCH 0/3] mtd: Ingenic NAND fix for JZ4740

[Paul Cercueil]

Hi Nikolaus,

Le dim., nov. 7 2021 at 14:47:43 +0100, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> Hi Paul,
> 
>>  Am 09.10.2021 um 20:49 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>>  Hi,
>> 
>>  Looks like NAND support has been broken on the JZ4740 SoC for a 
>> while;
> 
> Yes, I remember someone telling that something was fundamentally 
> broken
> and impossible to be fixed a while ago.

You mean MLC NAND, and that's still broken.

>>  it looks like it comes from the fact that the "hw_oob_first" 
>> mechanism
>>  was dropped from the NAND core and moved to the Davinci driver.
>> 
>>  It turns out the JZ4740 SoC needs it too; I didn't notice it when
>>  writing the new ingenic-nand driver (to replace the old jz4740-nand
>>  driver) most likely because my Device Tree had the "nand-ecc-mode" 
>> set
>>  to "hw_oob_first".
>> 
>>  I am not very sure about patch [1/3]; to me the original code does 
>> not
>>  make sense, and it didn't work out-of-the-box on the JZ4740 without 
>> it.
>>  By applying patch [1/3] the function 
>> nand_read_page_hwecc_oob_first()
>>  can be reused for the JZ4740 SoC as well. But I did not test patch 
>> [1/3]
>>  on Davinci.
> 
> would this also work for jz4780 NAND?

The JZ4780 NAND driver does work, but UBI refuses to use the CI20's 
NAND as it's a MLC.

-Paul