Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1606557 > unrolled thread

[PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-22 15:20 +0100
    [PATCH v2 10/53] mtd: nand: denali: fix erased page checking Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-22 15:20 +0100
      Re: [PATCH v2 10/53] mtd: nand: denali: fix erased page checking Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-03-22 21:40 +0100
        Re: [PATCH v2 10/53] mtd: nand: denali: fix erased page checking Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-23 08:40 +0100
          Re: [PATCH v2 10/53] mtd: nand: denali: fix erased page checking Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-03-23 09:10 +0100
      Re: [PATCH v2 10/53] mtd: nand: denali: fix erased page checking Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-03-22 22:00 +0100
        Re: [PATCH v2 10/53] mtd: nand: denali: fix erased page checking Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-23 08:40 +0100
          Re: [PATCH v2 10/53] mtd: nand: denali: fix erased page checking Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-03-23 09:00 +0100
            Re: [PATCH v2 10/53] mtd: nand: denali: fix erased page checking Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-24 03:50 +0100
              Re: [PATCH v2 10/53] mtd: nand: denali: fix erased page checking Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-03-24 09:10 +0100
    [PATCH v2 20/53] mtd: nand: denali: do not set mtd->name Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-22 15:20 +0100
      Re: [PATCH v2 20/53] mtd: nand: denali: do not set mtd->name Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-03-27 17:40 +0200
    [PATCH v2 18/53] mtd: nand: denali: use nand_chip to hold frequently accessed data Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-22 15:20 +0100
    [PATCH v2 07/53] mtd: nand: denali: consolidate INTR_STATUS__* and INTR_EN__* macros Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-22 15:20 +0100
    [PATCH v2 14/53] mtd: nand: denali: support 64bit capable DMA engine Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-22 15:20 +0100
    [PATCH v2 02/53] mtd: nand: use read_oob() instead of cmdfunc() for bad block check Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-22 15:30 +0100
    [PATCH v2 06/53] mtd: nand: denali: fix comment of denali_nand_info::flash_mem Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-22 15:30 +0100
    [PATCH v2 19/53] mtd: nand: denali: call nand_set_flash_node() to set DT node Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-22 15:30 +0100
    [PATCH v2 24/53] mtd: nand: denali: remove meaningless writes to read-only registers Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-22 15:30 +0100
    [PATCH v2 03/53] mtd: nand: denali: remove unused CONFIG option and macros Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-22 15:30 +0100
    [PATCH v2 01/53] mtd: nand: allow to set only one of ECC size and ECC strength from DT Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-22 15:30 +0100
    [PATCH v2 21/53] mtd: nand: denali: move multi device fixup code to a helper function Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-22 15:30 +0100
    [PATCH v2 11/53] mtd: nand: denali: fix bitflips calculation in handle_ecc() Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-22 15:30 +0100
      Re: [PATCH v2 11/53] mtd: nand: denali: fix bitflips calculation in  handle_ecc() Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-03-22 22:00 +0100
        Re: [PATCH v2 11/53] mtd: nand: denali: fix bitflips calculation in handle_ecc() Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-23 08:40 +0100
          Re: [PATCH v2 11/53] mtd: nand: denali: fix bitflips calculation in  handle_ecc() Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-03-23 09:20 +0100
    [PATCH v2 15/53] mtd: nand: denali_dt: remove dma-mask DT property Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-22 15:30 +0100
    [PATCH v2 12/53] mtd: nand: denali: support HW_ECC_FIXUP capability Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-22 15:30 +0100
      Re: [PATCH v2 12/53] mtd: nand: denali: support HW_ECC_FIXUP  capability Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-03-22 22:10 +0100
        Re: [PATCH v2 12/53] mtd: nand: denali: support HW_ECC_FIXUP capability Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-23 08:40 +0100
          Re: [PATCH v2 12/53] mtd: nand: denali: support HW_ECC_FIXUP  capability Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-03-23 09:30 +0100
      Re: [PATCH v2 12/53] mtd: nand: denali: support HW_ECC_FIXUP  capability Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-03-22 22:20 +0100
        Re: [PATCH v2 12/53] mtd: nand: denali: support HW_ECC_FIXUP capability Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-23 08:40 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1606575 — [PATCH v2 01/53] mtd: nand: allow to set only one of ECC size and ECC strength from DT

Currently, it is valid to specify both "nand-ecc-step-size" and
"nand-ecc-strength", but not allowed to set only one of them.

This requirement has a conflict with "nand-ecc-maximize"; this flag
is used when you want the driver to choose the best ECC strength.
If "nand-ecc-maximize" is set, "nand-ecc-strength" is very likely to
be unset.

It would be possible to make the if-conditional more complex by
adding the check for the NAND_ECC_MAXIMIZE flag, but I chose to drop
the check entirely.  I thought of the situation where the hardware
has a fixed ECC step size (so it can be hard-coded in the driver),
whereas the ECC strength is configurable by software.  In that case,
we may want to only set "nand-ecc-strength" (or "nand-ecc-maximize")
in DT.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

The Denali NAND is the case.

The ecc.size is fixed when the RTL is delivered, while the
driver can choose ecc.strength from some supported values.

 For Intel and Altera, available ecc.strength are 8, 15.
 For Socionext UniPhier, available ecc.strength are 8, 16, 24.


Changes in v2: None

 drivers/mtd/nand/nand_base.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index b0524f8..a3c0f47 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -4333,12 +4333,6 @@ static int nand_dt_init(struct nand_chip *chip)
 	ecc_strength = of_get_nand_ecc_strength(dn);
 	ecc_step = of_get_nand_ecc_step_size(dn);
 
-	if ((ecc_step >= 0 && !(ecc_strength >= 0)) ||
-	    (!(ecc_step >= 0) && ecc_strength >= 0)) {
-		pr_err("must set both strength and step size in DT\n");
-		return -EINVAL;
-	}
-
 	if (ecc_mode >= 0)
 		chip->ecc.mode = ecc_mode;
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1606577 — [PATCH v2 21/53] mtd: nand: denali: move multi device fixup code to a helper function

Collect multi NAND fixups into a helper function instead of
scattering them in denali_init().

I am rewording the comment block to clearly explain what is called
"multi device".

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2:
  - Reword the comment block for clarification

 drivers/mtd/nand/denali.c | 54 +++++++++++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 21 deletions(-)

diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 1706975..4e63d57 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1458,6 +1458,36 @@ static void denali_drv_init(struct denali_nand_info *denali)
 	denali->irq_status = 0;
 }
 
+static void denali_multidev_fixup(struct denali_nand_info *denali)
+{
+	struct nand_chip *chip = &denali->nand;
+	struct mtd_info *mtd = nand_to_mtd(chip);
+
+	/*
+	 * Support for multi device:
+	 * When the IP configuration is x16 capable and two x8 chips are
+	 * connected in parallel, DEVICES_CONNECTED should be set to 2.
+	 * In this case, the core framework knows nothing about this fact,
+	 * so we should tell it the _logical_ pagesize and anything necessary.
+	 */
+	denali->devnum = ioread32(denali->flash_reg + DEVICES_CONNECTED);
+
+	mtd->size <<= denali->devnum - 1;
+	mtd->erasesize <<= denali->devnum - 1;
+	mtd->writesize <<= denali->devnum - 1;
+	mtd->oobsize <<= denali->devnum - 1;
+	chip->chipsize <<= denali->devnum - 1;
+	chip->page_shift += denali->devnum - 1;
+	chip->phys_erase_shift += denali->devnum - 1;
+	chip->bbt_erase_shift += denali->devnum - 1;
+	chip->chip_shift += denali->devnum - 1;
+	chip->pagemask <<= denali->devnum - 1;
+	chip->ecc.size *= denali->devnum;
+	chip->ecc.bytes *= denali->devnum;
+	chip->ecc.strength *= denali->devnum;
+	denali->bbtskipbytes *= denali->devnum;
+}
+
 int denali_init(struct denali_nand_info *denali)
 {
 	struct nand_chip *chip = &denali->nand;
@@ -1540,24 +1570,6 @@ int denali_init(struct denali_nand_info *denali)
 	}
 
 	/*
-	 * support for multi nand
-	 * MTD known nothing about multi nand, so we should tell it
-	 * the real pagesize and anything necessery
-	 */
-	denali->devnum = ioread32(denali->flash_reg + DEVICES_CONNECTED);
-	chip->chipsize <<= denali->devnum - 1;
-	chip->page_shift += denali->devnum - 1;
-	chip->pagemask = (chip->chipsize >> chip->page_shift) - 1;
-	chip->bbt_erase_shift += denali->devnum - 1;
-	chip->phys_erase_shift = chip->bbt_erase_shift;
-	chip->chip_shift += denali->devnum - 1;
-	mtd->writesize <<= denali->devnum - 1;
-	mtd->oobsize <<= denali->devnum - 1;
-	mtd->erasesize <<= denali->devnum - 1;
-	mtd->size = chip->numchips * chip->chipsize;
-	denali->bbtskipbytes *= denali->devnum;
-
-	/*
 	 * second stage of the NAND scan
 	 * this stage requires information regarding ECC and
 	 * bad block management.
@@ -1600,11 +1612,9 @@ int denali_init(struct denali_nand_info *denali)
 	}
 
 	mtd_set_ooblayout(mtd, &denali_ooblayout_ops);
-	chip->ecc.bytes *= denali->devnum;
-	chip->ecc.strength *= denali->devnum;
 
 	/* override the default read operations */
-	chip->ecc.size = ECC_SECTOR_SIZE * denali->devnum;
+	chip->ecc.size = ECC_SECTOR_SIZE;
 	chip->ecc.read_page = denali_read_page;
 	chip->ecc.read_page_raw = denali_read_page_raw;
 	chip->ecc.write_page = denali_write_page;
@@ -1613,6 +1623,8 @@ int denali_init(struct denali_nand_info *denali)
 	chip->ecc.write_oob = denali_write_oob;
 	chip->erase = denali_erase;
 
+	denali_multidev_fixup(denali);
+
 	ret = nand_scan_tail(mtd);
 	if (ret)
 		goto failed_req_irq;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1606582 — [PATCH v2 11/53] mtd: nand: denali: fix bitflips calculation in handle_ecc()

This function is wrong in multiple ways:

[1] Counting corrected bytes instead of corrected bits.

The following code is counting the number of corrected _bytes_.

    /* correct the ECC error */
    buf[offset] ^= err_cor_value;
    mtd->ecc_stats.corrected++;
    bitflips++;

What the core framework expects is the number of corrected _bits_.
They can be different if multiple bitflips occur within one byte.

[2] total number of errors instead of max of per-sector errors

The core framework expects that corrected errors are counted per
sector, then the max value should be taken.  The current code simply
iterates over the whole page, i.e. counts the total number of
correction in the page.  This means "too many bitflips" is triggered
earlier than it should be, i.e. the NAND device is worn out sooner.

Besides those bugs, this function is unreadable due to the deep
nesting.  Notice the whole code in this function is wrapped in
if (irq_status & INTR__ECC_ERR), so this conditional can be moved
out of the function.  Also, use shorter names for local variables.

Re-work the function to fix all the issues.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2:
  - Use shorter names for local variables.
  - Fix bugs addressed by [1], [2]

 drivers/mtd/nand/denali.c | 157 ++++++++++++++++++++++++----------------------
 1 file changed, 82 insertions(+), 75 deletions(-)

diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 86381ac..608fe6f 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -888,80 +888,87 @@ static void read_oob_data(struct mtd_info *mtd, uint8_t *buf, int page)
 #define ECC_SECTOR(x)	(((x) & ECC_ERROR_ADDRESS__SECTOR_NR) >> 12)
 #define ECC_BYTE(x)	(((x) & ECC_ERROR_ADDRESS__OFFSET))
 #define ECC_CORRECTION_VALUE(x) ((x) & ERR_CORRECTION_INFO__BYTEMASK)
-#define ECC_ERROR_CORRECTABLE(x) (!((x) & ERR_CORRECTION_INFO__ERROR_TYPE))
+#define ECC_ERROR_UNCORRECTABLE(x) ((x) & ERR_CORRECTION_INFO__ERROR_TYPE)
 #define ECC_ERR_DEVICE(x)	(((x) & ERR_CORRECTION_INFO__DEVICE_NR) >> 8)
 #define ECC_LAST_ERR(x)		((x) & ERR_CORRECTION_INFO__LAST_ERR_INFO)
 
-static bool handle_ecc(struct denali_nand_info *denali, uint8_t *buf,
-		       uint32_t irq_status, unsigned int *max_bitflips)
+static int handle_ecc(struct mtd_info *mtd,
+		      struct denali_nand_info *denali, uint8_t *buf)
 {
-	bool check_erased_page = false;
 	unsigned int bitflips = 0;
+	unsigned int max_bitflips = 0;
+	unsigned int total_bitflips = 0;
+	uint32_t err_addr, err_cor_info;
+	unsigned int err_byte, err_sector, err_device;
+	uint8_t err_cor_value;
+	unsigned int prev_sector = 0;
+	int ret = 0;
+
+	/* read the ECC errors. we'll ignore them for now */
+	denali_set_intr_modes(denali, false);
 
-	if (irq_status & INTR__ECC_ERR) {
-		/* read the ECC errors. we'll ignore them for now */
-		uint32_t err_address, err_correction_info, err_byte,
-			 err_sector, err_device, err_correction_value;
-		denali_set_intr_modes(denali, false);
-
-		do {
-			err_address = ioread32(denali->flash_reg +
-						ECC_ERROR_ADDRESS);
-			err_sector = ECC_SECTOR(err_address);
-			err_byte = ECC_BYTE(err_address);
-
-			err_correction_info = ioread32(denali->flash_reg +
-						ERR_CORRECTION_INFO);
-			err_correction_value =
-				ECC_CORRECTION_VALUE(err_correction_info);
-			err_device = ECC_ERR_DEVICE(err_correction_info);
-
-			if (ECC_ERROR_CORRECTABLE(err_correction_info)) {
-				/*
-				 * If err_byte is larger than ECC_SECTOR_SIZE,
-				 * means error happened in OOB, so we ignore
-				 * it. It's no need for us to correct it
-				 * err_device is represented the NAND error
-				 * bits are happened in if there are more
-				 * than one NAND connected.
-				 */
-				if (err_byte < ECC_SECTOR_SIZE) {
-					struct mtd_info *mtd =
-						nand_to_mtd(&denali->nand);
-					int offset;
-
-					offset = (err_sector *
-							ECC_SECTOR_SIZE +
-							err_byte) *
-							denali->devnum +
-							err_device;
-					/* correct the ECC error */
-					buf[offset] ^= err_correction_value;
-					mtd->ecc_stats.corrected++;
-					bitflips++;
-				}
-			} else {
-				/*
-				 * if the error is not correctable, need to
-				 * look at the page to see if it is an erased
-				 * page. if so, then it's not a real ECC error
-				 */
-				check_erased_page = true;
-			}
-		} while (!ECC_LAST_ERR(err_correction_info));
-		/*
-		 * Once handle all ecc errors, controller will triger
-		 * a ECC_TRANSACTION_DONE interrupt, so here just wait
-		 * for a while for this interrupt
-		 */
-		while (!(read_interrupt_status(denali) &
-				INTR__ECC_TRANSACTION_DONE))
-			cpu_relax();
-		clear_interrupts(denali);
-		denali_set_intr_modes(denali, true);
-	}
-	*max_bitflips = bitflips;
-	return check_erased_page;
+	do {
+		err_addr = ioread32(denali->flash_reg + ECC_ERROR_ADDRESS);
+		err_sector = ECC_SECTOR(err_addr);
+		err_byte = ECC_BYTE(err_addr);
+
+		err_cor_info = ioread32(denali->flash_reg + ERR_CORRECTION_INFO);
+		err_cor_value = ECC_CORRECTION_VALUE(err_cor_info);
+		err_device = ECC_ERR_DEVICE(err_cor_info);
+
+		/* reset the bitflip counter when crossing ECC sector */
+		if (err_sector != prev_sector)
+			bitflips = 0;
+
+		if (ECC_ERROR_UNCORRECTABLE(err_cor_info)) {
+			/*
+			 * if the error is not correctable, need to look at the
+			 * page to see if it is an erased page. if so, then
+			 * it's not a real ECC error
+			 */
+			ret = -EBADMSG;
+		} else if (err_byte < ECC_SECTOR_SIZE) {
+			/*
+			 * If err_byte is larger than ECC_SECTOR_SIZE, means error
+			 * happened in OOB, so we ignore it. It's no need for
+			 * us to correct it err_device is represented the NAND
+			 * error bits are happened in if there are more than
+			 * one NAND connected.
+			 */
+			int offset;
+			unsigned int flips_in_byte;
+
+			offset = (err_sector * ECC_SECTOR_SIZE + err_byte) *
+						denali->devnum + err_device;
+
+			/* correct the ECC error */
+			flips_in_byte = hweight8(buf[offset] ^ err_cor_value);
+			bitflips += flips_in_byte;
+			total_bitflips += flips_in_byte;
+			buf[offset] ^= err_cor_value;
+
+			max_bitflips = max(max_bitflips, bitflips);
+		}
+
+		prev_sector = err_sector;
+	} while (!ECC_LAST_ERR(err_cor_info));
+
+	/*
+	 * Once handle all ecc errors, controller will trigger a
+	 * ECC_TRANSACTION_DONE interrupt, so here just wait for
+	 * a while for this interrupt
+	 */
+	while (!(read_interrupt_status(denali) & INTR__ECC_TRANSACTION_DONE))
+		cpu_relax();
+	clear_interrupts(denali);
+	denali_set_intr_modes(denali, true);
+
+	if (ret)
+		return ret;
+
+	mtd->ecc_stats.corrected += total_bitflips;
+
+	return max_bitflips;
 }
 
 /* programs the controller to either enable/disable DMA transfers */
@@ -1097,7 +1104,6 @@ static int denali_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
 static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 			    uint8_t *buf, int oob_required, int page)
 {
-	unsigned int max_bitflips;
 	struct denali_nand_info *denali = mtd_to_denali(mtd);
 
 	dma_addr_t addr = denali->buf.dma_buf;
@@ -1105,8 +1111,7 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 
 	uint32_t irq_status;
 	uint32_t irq_mask = INTR__ECC_TRANSACTION_DONE | INTR__ECC_ERR;
-	bool check_erased_page = false;
-	int stat;
+	int stat = 0;
 
 	if (page != denali->page) {
 		dev_err(denali->dev,
@@ -1130,10 +1135,11 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 
 	memcpy(buf, denali->buf.buf, mtd->writesize);
 
-	check_erased_page = handle_ecc(denali, buf, irq_status, &max_bitflips);
+	if (irq_status & INTR__ECC_ERR)
+		stat = handle_ecc(mtd, denali, buf);
 	denali_enable_dma(denali, false);
 
-	if (check_erased_page) {
+	if (stat == -EBADMSG) {
 		read_oob_data(mtd, chip->oob_poi, denali->page);
 
 		stat = nand_check_erased_ecc_chunk(
@@ -1144,10 +1150,11 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 		if (stat < 0) {
 			mtd->ecc_stats.failed++;
 			/* return 0 for uncorrectable bitflips */
-			max_bitflips = 0;
+			stat = 0;
 		}
 	}
-	return max_bitflips;
+
+	return stat;
 }
 
 static int denali_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1607016 — Re: [PATCH v2 11/53] mtd: nand: denali: fix bitflips calculation in handle_ecc()

On Wed, 22 Mar 2017 23:07:18 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> +	do {
> +		err_addr = ioread32(denali->flash_reg + ECC_ERROR_ADDRESS);
> +		err_sector = ECC_SECTOR(err_addr);
> +		err_byte = ECC_BYTE(err_addr);
> +
> +		err_cor_info = ioread32(denali->flash_reg + ERR_CORRECTION_INFO);
> +		err_cor_value = ECC_CORRECTION_VALUE(err_cor_info);
> +		err_device = ECC_ERR_DEVICE(err_cor_info);
> +
> +		/* reset the bitflip counter when crossing ECC sector */
> +		if (err_sector != prev_sector)
> +			bitflips = 0;
> +
> +		if (ECC_ERROR_UNCORRECTABLE(err_cor_info)) {
> +			/*
> +			 * if the error is not correctable, need to look at the
> +			 * page to see if it is an erased page. if so, then
> +			 * it's not a real ECC error
> +			 */
> +			ret = -EBADMSG;

You should never return -EBADMSG directly. Just increment
ecc_stats.failed and let the core return -EBADMSG to the upper layer.

[toc] | [prev] | [next] | [standalone]

#1607185 — Re: [PATCH v2 11/53] mtd: nand: denali: fix bitflips calculation in handle_ecc()

Hi Boris,

2017-03-23 5:57 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Wed, 22 Mar 2017 23:07:18 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> +     do {
>> +             err_addr = ioread32(denali->flash_reg + ECC_ERROR_ADDRESS);
>> +             err_sector = ECC_SECTOR(err_addr);
>> +             err_byte = ECC_BYTE(err_addr);
>> +
>> +             err_cor_info = ioread32(denali->flash_reg + ERR_CORRECTION_INFO);
>> +             err_cor_value = ECC_CORRECTION_VALUE(err_cor_info);
>> +             err_device = ECC_ERR_DEVICE(err_cor_info);
>> +
>> +             /* reset the bitflip counter when crossing ECC sector */
>> +             if (err_sector != prev_sector)
>> +                     bitflips = 0;
>> +
>> +             if (ECC_ERROR_UNCORRECTABLE(err_cor_info)) {
>> +                     /*
>> +                      * if the error is not correctable, need to look at the
>> +                      * page to see if it is an erased page. if so, then
>> +                      * it's not a real ECC error
>> +                      */
>> +                     ret = -EBADMSG;
>
> You should never return -EBADMSG directly. Just increment
> ecc_stats.failed and let the core return -EBADMSG to the upper layer.
>

Here, -EBADMSG is used like that returned from  ->ecc.correct()


Please notice denali_read_page() never returns -EBADMSG.

 -EBADMSG is used as a mark "we need erased page check".


I think nand_read_page_syndrome() does similar;
-EBADMSG is used internally.



-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [next] | [standalone]

#1607242 — Re: [PATCH v2 11/53] mtd: nand: denali: fix bitflips calculation in handle_ecc()

On Thu, 23 Mar 2017 16:02:02 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> Hi Boris,
> 
> 2017-03-23 5:57 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > On Wed, 22 Mar 2017 23:07:18 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >  
> >> +     do {
> >> +             err_addr = ioread32(denali->flash_reg + ECC_ERROR_ADDRESS);
> >> +             err_sector = ECC_SECTOR(err_addr);
> >> +             err_byte = ECC_BYTE(err_addr);
> >> +
> >> +             err_cor_info = ioread32(denali->flash_reg + ERR_CORRECTION_INFO);
> >> +             err_cor_value = ECC_CORRECTION_VALUE(err_cor_info);
> >> +             err_device = ECC_ERR_DEVICE(err_cor_info);
> >> +
> >> +             /* reset the bitflip counter when crossing ECC sector */
> >> +             if (err_sector != prev_sector)
> >> +                     bitflips = 0;
> >> +
> >> +             if (ECC_ERROR_UNCORRECTABLE(err_cor_info)) {
> >> +                     /*
> >> +                      * if the error is not correctable, need to look at the
> >> +                      * page to see if it is an erased page. if so, then
> >> +                      * it's not a real ECC error
> >> +                      */
> >> +                     ret = -EBADMSG;  
> >
> > You should never return -EBADMSG directly. Just increment
> > ecc_stats.failed and let the core return -EBADMSG to the upper layer.
> >  
> 
> Here, -EBADMSG is used like that returned from  ->ecc.correct()
> 
> 
> Please notice denali_read_page() never returns -EBADMSG.
> 
>  -EBADMSG is used as a mark "we need erased page check".
> 
> 
> I think nand_read_page_syndrome() does similar;
> -EBADMSG is used internally.

That's not exactly what happens. nand_read_page_syndrome() calls
ecc->correct() for each chunk, and if this method returns -EBADMSG (and
nand_check_erased_ecc_chunk() returns -EBADMSG too) it increments the
ecc_stats.failed counter.

Here you check all chunks in the same function and only increment
ecc_stats.failed once in denali_read_page() even if several chunks are
uncorrectable.
You handle_ecc() should act like nand_read_page_syndrome() WRT ECC
checking: check each block one by one, call
nand_check_erased_ecc_chunk() if needed, increment ecc_stats.failed
when an uncorrectable error is detected, and return max_bitflips at the
end.

[toc] | [prev] | [next] | [standalone]

#1606583 — [PATCH v2 15/53] mtd: nand: denali_dt: remove dma-mask DT property

The driver sets appropriate DMA mask.  Delete the "dma-mask" DT
property.  See [1] for negative comments for this binding.

[1] https://lkml.org/lkml/2016/2/8/57

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Acked-by: Rob Herring <robh@kernel.org>
---

Changes in v2: None

 Documentation/devicetree/bindings/mtd/denali-nand.txt | 2 --
 drivers/mtd/nand/denali_dt.c                          | 9 ---------
 2 files changed, 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
index 6f4ab4c..e593bbe 100644
--- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
@@ -6,7 +6,6 @@ Required properties:
   - reg : should contain registers location and length for data and reg.
   - reg-names: Should contain the reg names "nand_data" and "denali_reg"
   - interrupts : The interrupt number.
-  - dm-mask : DMA bit mask
 
 The device tree may optionally contain sub-nodes describing partitions of the
 address space. See partition.txt for more detail.
@@ -20,5 +19,4 @@ nand: nand@ff900000 {
 	reg = <0xff900000 0x100000>, <0xffb80000 0x10000>;
 	reg-names = "nand_data", "denali_reg";
 	interrupts = <0 144 4>;
-	dma-mask = <0xffffffff>;
 };
diff --git a/drivers/mtd/nand/denali_dt.c b/drivers/mtd/nand/denali_dt.c
index 9577bfd..b8a8284 100644
--- a/drivers/mtd/nand/denali_dt.c
+++ b/drivers/mtd/nand/denali_dt.c
@@ -46,8 +46,6 @@ static const struct of_device_id denali_nand_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, denali_nand_dt_ids);
 
-static u64 denali_dma_mask;
-
 static int denali_dt_probe(struct platform_device *ofdev)
 {
 	struct resource *denali_reg, *nand_data;
@@ -83,13 +81,6 @@ static int denali_dt_probe(struct platform_device *ofdev)
 	if (IS_ERR(denali->flash_mem))
 		return PTR_ERR(denali->flash_mem);
 
-	if (!of_property_read_u32(ofdev->dev.of_node,
-		"dma-mask", (u32 *)&denali_dma_mask)) {
-		denali->dev->dma_mask = &denali_dma_mask;
-	} else {
-		denali->dev->dma_mask = NULL;
-	}
-
 	dt->clk = devm_clk_get(&ofdev->dev, NULL);
 	if (IS_ERR(dt->clk)) {
 		dev_err(&ofdev->dev, "no clk available\n");
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1606584 — [PATCH v2 12/53] mtd: nand: denali: support HW_ECC_FIXUP capability

Some old versions of the Denali IP (perhaps used only for Intel?)
detects ECC errors and provides correct data via a register, but
does not touch the transferred data.  So, the software must fixup
the data in the buffer according to the provided ECC correction
information.

Newer versions perform ECC correction before transferring the data.
No more software intervention is needed.  The ECC_ERROR_ADDRESS and
ECC_CORRECTION_INFO registers were deprecated.  Instead, the number
of corrected bit-flips can be read from the ECC_COR_INFO register.
When an uncorrectable ECC error happens, a status flag is set to the
INTR_STATUS and ECC_COR_INFO registers.

As is often the case with this IP, the register view of INTR_STATUS
had broken compatibility.

For older versions (SW ECC fixup):
  bit 0:  ECC_TRANSACTION_DONE
  bit 1:  ECC_ERR

For newer versions (HW ECC fixup):
  bit 0:  ECC_UNCOR_ERR
  bit 1:  Reserved

Due to this difference, the irq_mask must be fixed too.  The comment
block in handle_ecc() has been moved to the common part because the
comment applies to both cases.  The existing handle_ecc() has been
renamed to denali_sw_ecc_fixup() for clarification.

The U-Boot port of this driver already supports the HW ECC fixup.  I
borrowed the comment "Some versions of ..." in denali.h from U-Boot.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2:
  - Change the capability prefix DENALI_CAPS_ -> DENALI_CAP_

 drivers/mtd/nand/denali.c | 52 ++++++++++++++++++++++++++++++++++++-----------
 drivers/mtd/nand/denali.h | 14 +++++++++++++
 2 files changed, 54 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 608fe6f..91f0def 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -883,6 +883,32 @@ static void read_oob_data(struct mtd_info *mtd, uint8_t *buf, int page)
 	}
 }
 
+static int denali_hw_ecc_fixup(struct mtd_info *mtd,
+			       struct denali_nand_info *denali)
+{
+	int bank = denali->flash_bank;
+	uint32_t ecc_cor;
+	unsigned int max_bitflips;
+
+	ecc_cor = ioread32(denali->flash_reg + ECC_COR_INFO(bank));
+	ecc_cor >>= ECC_COR_INFO__SHIFT(bank);
+
+	if (ecc_cor & ECC_COR_INFO__UNCOR_ERR)
+		return -EBADMSG;
+
+	max_bitflips = ecc_cor & ECC_COR_INFO__MAX_ERRORS;
+
+	/*
+	 * The register holds the maximum of the number of corrected bitflips
+	 * per sector.  This can be returned from ecc->read_page() as-is.
+	 * Unfortunately, we can not know the total number of corrected bits
+	 * in the page.  mtd->ecc_stats.corrected is compromised here.
+	 */
+	mtd->ecc_stats.corrected += max_bitflips;
+
+	return max_bitflips;
+}
+
 #define ECC_SECTOR_SIZE 512
 
 #define ECC_SECTOR(x)	(((x) & ECC_ERROR_ADDRESS__SECTOR_NR) >> 12)
@@ -892,8 +918,8 @@ static void read_oob_data(struct mtd_info *mtd, uint8_t *buf, int page)
 #define ECC_ERR_DEVICE(x)	(((x) & ERR_CORRECTION_INFO__DEVICE_NR) >> 8)
 #define ECC_LAST_ERR(x)		((x) & ERR_CORRECTION_INFO__LAST_ERR_INFO)
 
-static int handle_ecc(struct mtd_info *mtd,
-		      struct denali_nand_info *denali, uint8_t *buf)
+static int denali_sw_ecc_fixup(struct mtd_info *mtd,
+			       struct denali_nand_info *denali, uint8_t *buf)
 {
 	unsigned int bitflips = 0;
 	unsigned int max_bitflips = 0;
@@ -921,11 +947,6 @@ static int handle_ecc(struct mtd_info *mtd,
 			bitflips = 0;
 
 		if (ECC_ERROR_UNCORRECTABLE(err_cor_info)) {
-			/*
-			 * if the error is not correctable, need to look at the
-			 * page to see if it is an erased page. if so, then
-			 * it's not a real ECC error
-			 */
 			ret = -EBADMSG;
 		} else if (err_byte < ECC_SECTOR_SIZE) {
 			/*
@@ -1105,12 +1126,12 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 			    uint8_t *buf, int oob_required, int page)
 {
 	struct denali_nand_info *denali = mtd_to_denali(mtd);
-
 	dma_addr_t addr = denali->buf.dma_buf;
 	size_t size = mtd->writesize + mtd->oobsize;
-
 	uint32_t irq_status;
-	uint32_t irq_mask = INTR__ECC_TRANSACTION_DONE | INTR__ECC_ERR;
+	uint32_t irq_mask = denali->caps & DENALI_CAP_HW_ECC_FIXUP ?
+				INTR__DMA_CMD_COMP | INTR__ECC_UNCOR_ERR :
+				INTR__ECC_TRANSACTION_DONE | INTR__ECC_ERR;
 	int stat = 0;
 
 	if (page != denali->page) {
@@ -1135,11 +1156,18 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 
 	memcpy(buf, denali->buf.buf, mtd->writesize);
 
-	if (irq_status & INTR__ECC_ERR)
-		stat = handle_ecc(mtd, denali, buf);
+	if (denali->caps & DENALI_CAP_HW_ECC_FIXUP)
+		stat = denali_hw_ecc_fixup(mtd, denali);
+	else if (irq_status & INTR__ECC_ERR)
+		stat = denali_sw_ecc_fixup(mtd, denali, buf);
 	denali_enable_dma(denali, false);
 
 	if (stat == -EBADMSG) {
+		/*
+		 * If the error is not correctable, need to look at the page to
+		 * see if it is an erased page. If so, then it's not a real ECC
+		 * error.
+		 */
 		read_oob_data(mtd, chip->oob_poi, denali->page);
 
 		stat = nand_check_erased_ecc_chunk(
diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
index 7b2d785..ed42b16 100644
--- a/drivers/mtd/nand/denali.h
+++ b/drivers/mtd/nand/denali.h
@@ -20,6 +20,7 @@
 #ifndef __DENALI_H__
 #define __DENALI_H__
 
+#include <linux/bitops.h>
 #include <linux/mtd/nand.h>
 
 #define DEVICE_RESET				0x0
@@ -218,6 +219,13 @@
 
 #define INTR_STATUS(__bank)	(0x410 + ((__bank) * 0x50))
 #define INTR_EN(__bank)		(0x420 + ((__bank) * 0x50))
+/*
+ * Some versions of the IP have the ECC fixup handled in hardware.  In this
+ * configuration we only get interrupted when the error is uncorrectable.
+ * Unfortunately this bit replaces INTR_STATUS__ECC_TRANSACTION_DONE from the
+ * old IP.
+ */
+#define     INTR__ECC_UNCOR_ERR				0x0001
 #define     INTR__ECC_TRANSACTION_DONE			0x0001
 #define     INTR__ECC_ERR				0x0002
 #define     INTR__DMA_CMD_COMP				0x0004
@@ -259,6 +267,11 @@
 #define     ERR_CORRECTION_INFO__ERROR_TYPE		0x4000
 #define     ERR_CORRECTION_INFO__LAST_ERR_INFO		0x8000
 
+#define ECC_COR_INFO(bank)			(0x650 + (bank) / 2 * 0x10)
+#define     ECC_COR_INFO__SHIFT(bank)			((bank) % 2 * 8)
+#define     ECC_COR_INFO__MAX_ERRORS			0x007f
+#define     ECC_COR_INFO__UNCOR_ERR			0x0080
+
 #define DMA_ENABLE				0x700
 #define     DMA_ENABLE__FLAG				0x0001
 
@@ -339,6 +352,7 @@ struct denali_nand_info {
 	int bbtskipbytes;
 	int max_banks;
 	unsigned int caps;
+#define DENALI_CAP_HW_ECC_FIXUP			BIT(0)
 };
 
 extern int denali_init(struct denali_nand_info *denali);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1607022 — Re: [PATCH v2 12/53] mtd: nand: denali: support HW_ECC_FIXUP capability

On Wed, 22 Mar 2017 23:07:19 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> @@ -339,6 +352,7 @@ struct denali_nand_info {
>  	int bbtskipbytes;
>  	int max_banks;
>  	unsigned int caps;
> +#define DENALI_CAP_HW_ECC_FIXUP			BIT(0)

Can you move this definition outside of the struct denali_nand_info
definition?

>  };
>  
>  extern int denali_init(struct denali_nand_info *denali);

[toc] | [prev] | [next] | [standalone]

#1607191 — Re: [PATCH v2 12/53] mtd: nand: denali: support HW_ECC_FIXUP capability

Hi Boris,

2017-03-23 6:09 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Wed, 22 Mar 2017 23:07:19 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> @@ -339,6 +352,7 @@ struct denali_nand_info {
>>       int bbtskipbytes;
>>       int max_banks;
>>       unsigned int caps;
>> +#define DENALI_CAP_HW_ECC_FIXUP                      BIT(0)
>
> Can you move this definition outside of the struct denali_nand_info
> definition?


My idea was to put relevant macros close to the "caps" member.
Actually, "struct mmc_host" does similar.


If you do not like it, yes, I can move it.
Anyway, this is a matter of taste,
and I do not have a strong opinion about this.


-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [next] | [standalone]

#1607245 — Re: [PATCH v2 12/53] mtd: nand: denali: support HW_ECC_FIXUP capability

On Thu, 23 Mar 2017 16:06:19 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> Hi Boris,
> 
> 2017-03-23 6:09 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > On Wed, 22 Mar 2017 23:07:19 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >  
> >> @@ -339,6 +352,7 @@ struct denali_nand_info {
> >>       int bbtskipbytes;
> >>       int max_banks;
> >>       unsigned int caps;
> >> +#define DENALI_CAP_HW_ECC_FIXUP                      BIT(0)  
> >
> > Can you move this definition outside of the struct denali_nand_info
> > definition?  
> 
> 
> My idea was to put relevant macros close to the "caps" member.
> Actually, "struct mmc_host" does similar.
> 
> 
> If you do not like it, yes, I can move it.
> Anyway, this is a matter of taste,
> and I do not have a strong opinion about this.
> 
> 

Indeed, I prefer when macro and struct definitions are clearly
separated.

[toc] | [prev] | [next] | [standalone]

#1607026 — Re: [PATCH v2 12/53] mtd: nand: denali: support HW_ECC_FIXUP capability

On Wed, 22 Mar 2017 23:07:19 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> +static int denali_hw_ecc_fixup(struct mtd_info *mtd,
> +			       struct denali_nand_info *denali)
> +{
> +	int bank = denali->flash_bank;
> +	uint32_t ecc_cor;
> +	unsigned int max_bitflips;
> +
> +	ecc_cor = ioread32(denali->flash_reg + ECC_COR_INFO(bank));
> +	ecc_cor >>= ECC_COR_INFO__SHIFT(bank);
> +
> +	if (ecc_cor & ECC_COR_INFO__UNCOR_ERR)
> +		return -EBADMSG;

As previously mentioned, just increment ecc_stats.failed.

> +
> +	max_bitflips = ecc_cor & ECC_COR_INFO__MAX_ERRORS;
> +
> +	/*
> +	 * The register holds the maximum of the number of corrected bitflips
> +	 * per sector.  This can be returned from ecc->read_page() as-is.
> +	 * Unfortunately, we can not know the total number of corrected bits
> +	 * in the page.  mtd->ecc_stats.corrected is compromised here.
> +	 */
> +	mtd->ecc_stats.corrected += max_bitflips;
> +
> +	return max_bitflips;
> +}

[toc] | [prev] | [next] | [standalone]

#1607192 — Re: [PATCH v2 12/53] mtd: nand: denali: support HW_ECC_FIXUP capability

Hi Boris,


2017-03-23 6:12 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Wed, 22 Mar 2017 23:07:19 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> +static int denali_hw_ecc_fixup(struct mtd_info *mtd,
>> +                            struct denali_nand_info *denali)
>> +{
>> +     int bank = denali->flash_bank;
>> +     uint32_t ecc_cor;
>> +     unsigned int max_bitflips;
>> +
>> +     ecc_cor = ioread32(denali->flash_reg + ECC_COR_INFO(bank));
>> +     ecc_cor >>= ECC_COR_INFO__SHIFT(bank);
>> +
>> +     if (ecc_cor & ECC_COR_INFO__UNCOR_ERR)
>> +             return -EBADMSG;
>
> As previously mentioned, just increment ecc_stats.failed.
>

I explained the intention of -EBADMSG in 11/53.

ecc_stats.failed is incremented in denali_read_page().


-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web