mtd: rawnand: brcmnand: Don't default to edu transfer

PATCH Superseded Kamal Dasu 2 participants 12 messages Jun 11, 2020 – Jun 15, 2020

Subsystems (3)

Memory technology devices (MTD) NAND flash Broadcom stb NAND flash driver

lore

Share & tools

lore

Kamal Dasu

author

2020-06-11 05:45 UTC #0 lore raw

PATCH 1/2 mtd: rawnand: brcmnand: Don't default to edu transfer

When flash-dma is absent do not default to using flash-edu. Make sure flash-edu is enabled before setting EDU transfer function.

Fixes a5d53ad26a8b ("mtd: rawnand: brcmnand: Add support for flash-edu for dma transfers")

Signed-off-by Kamal Dasu <[email protected]>

---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/mtd/nand/raw/brcmnand/brcmnand.c +3 -2

index 8f9ffb46a09f..0c1d6e543586 100644
@@ -2953,8 +2953,9 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
 		if (ret < 0)
 			goto err;
 
-		/* set edu transfer function to call */
-		ctrl->dma_trans = brcmnand_edu_trans;
+		if (has_edu(ctrl))
+			/* set edu transfer function to call */
+			ctrl->dma_trans = brcmnand_edu_trans;
 	}
 
 	/* Disable automatic device ID config, direct addressing */

Signature · 1 line

2.17.1

Kamal Dasu

author

2020-06-11 05:45 UTC #1 lore raw

PATCH 2/2 mtd: rawnand: brcmnand: Ecc error handling on EDU transfers

Implemented ECC correctable and uncorrectable error handling for EDU reads. If ECC correctable bitflips are encountered on EDU transfer, read page again using pio, This is needed due to a controller lmitation where read and corrected data is not transferred to the DMA buffer on ECC errors. This holds true for ECC correctable errors beyond set threshold.

Fixes a5d53ad26a8b ("mtd: rawnand: brcmnand: Add support for flash-edu for dma transfers")

Signed-off-by Kamal Dasu <[email protected]>

---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 26 ++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

drivers/mtd/nand/raw/brcmnand/brcmnand.c +26 -0

index 0c1d6e543586..d7daa83c8a58 100644
@@ -1855,6 +1855,22 @@ static int brcmnand_edu_trans(struct brcmnand_host *host, u64 addr, u32 *buf,
 	edu_writel(ctrl, EDU_STOP, 0); /* force stop */
 	edu_readl(ctrl, EDU_STOP);
 
+	if (ret == 0 && edu_cmd == EDU_CMD_READ) {
+		u64 err_addr = 0;
+
+		/*
+		 * check for ecc errors here, subpage ecc erros are
+		 * retained in ecc error addr register
+		 */
+		err_addr = brcmnand_get_uncorrecc_addr(ctrl);
+		if (!err_addr) {
+			err_addr = brcmnand_get_correcc_addr(ctrl);
+			if (err_addr)
+				ret = -EUCLEAN;
+		} else
+			ret = -EBADMSG;
+	}
+
 	return ret;
 }

@@ -2058,6 +2074,7 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
 	u64 err_addr = 0;
 	int err;
 	bool retry = true;
+	bool edu_read = false;
 
 	dev_dbg(ctrl->dev, "read %llx -> %p\n", (unsigned long long)addr, buf);

@@ -2075,6 +2092,10 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
 			else
 				return -EIO;
 		}
+
+		if (has_edu(ctrl))
+			edu_read = true;
+
 	} else {
 		if (oob)
 			memset(oob, 0x99, mtd->oobsize);

@@ -2122,6 +2143,11 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
 	if (mtd_is_bitflip(err)) {
 		unsigned int corrected = brcmnand_count_corrected(ctrl);
 
+		/* in case of edu correctable error we read again using pio */
+		if (edu_read)
+			err = brcmnand_read_by_pio(mtd, chip, addr, trans, buf,
+						   oob, &err_addr);
+
 		dev_dbg(ctrl->dev, "corrected error at 0x%llx\n",
 			(unsigned long long)err_addr);
 		mtd->ecc_stats.corrected += corrected;

Signature · 1 line

2.17.1

Miquel Raynal

maintainer

2020-06-11 07:27 UTC #2 lore raw

Re: [PATCH 2/2] mtd: rawnand: brcmnand: Ecc error handling on EDU transfers

Hi Kamal,

Kamal Dasu <[email protected]> wrote on Thu, 11 Jun 2020 01:44:54 -0400:

Quoted 1 line

Implemented ECC correctable and uncorrectable error handling for EDU

Implement?

Quoted 1 line

reads. If ECC correctable bitflips are encountered on EDU transfer,

extra space ^

Quoted 1 line

read page again using pio, This is needed due to a controller lmitation

s/pio/PIO/

Quoted 2 lines

where read and corrected data is not transferred to the DMA buffer on ECC errors. This holds true for ECC correctable errors beyond set threshold.

error.

Not sure what the last sentence means?

Quoted 4 lines

Fixes a5d53ad26a8b ("mtd: rawnand: brcmnand: Add support for flash-edu for dma transfers")
Signed-off-by Kamal Dasu <[email protected]>
---

Minor nits below :)

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_edu_trans(struct brcmnand_host *host, u64 addr, u32 *buf, · line 1858

Quoted 12 lines

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 26 ++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

drivers/mtd/nand/raw/brcmnand/brcmnand.c +1 -0

index 0c1d6e543586..d7daa83c8a58 100644
@@ -1855,6 +1855,22 @@ static int brcmnand_edu_trans(struct brcmnand_host *host, u64 addr, u32 *buf,
 	edu_writel(ctrl, EDU_STOP, 0); /* force stop */
 	edu_readl(ctrl, EDU_STOP);
 
+	if (ret == 0 && edu_cmd == EDU_CMD_READ) {

!ret

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_edu_trans(struct brcmnand_host *host, u64 addr, u32 *buf, · line 1863

Quoted 5 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c +5 -0

+		u64 err_addr = 0;
+
+		/*
+		 * check for ecc errors here, subpage ecc erros are
+		 * retained in ecc error addr register

s/ecc/ECC/
s/erros/errors/
s/addr/address/

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_edu_trans(struct brcmnand_host *host, u64 addr, u32 *buf, · line 1871

Quoted 8 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c +8 -0

+		 */
+		err_addr = brcmnand_get_uncorrecc_addr(ctrl);
+		if (!err_addr) {
+			err_addr = brcmnand_get_correcc_addr(ctrl);
+			if (err_addr)
+				ret = -EUCLEAN;
+		} else
+			ret = -EBADMSG;

I don't like very much to see these values being used within NAND controller drivers but I see it's already the cause, so I guess I can live with that.

Quoted 14 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c +3 -0

+	}
+
 	return ret;
 }

@@ -2058,6 +2074,7 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
 	u64 err_addr = 0;
 	int err;
 	bool retry = true;
+	bool edu_read = false;
 
 	dev_dbg(ctrl->dev, "read %llx -> %p\n", (unsigned long long)addr, buf);

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip, · line 2097

Quoted 6 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c +3 -0

@@ -2075,6 +2092,10 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
 			else
 				return -EIO;
 		}
+
+		if (has_edu(ctrl))
+			edu_read = true;

You don't need this extra value, you already have the cmd parameter which tells you if it is a read or a write. You might even want to create a if block so set dir and edu_cmd and eventually a local edu_read if you think it still makes sense.

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip, · line 2146

Quoted 9 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c +2 -0

+
 	} else {
 		if (oob)
 			memset(oob, 0x99, mtd->oobsize);

@@ -2122,6 +2143,11 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
 	if (mtd_is_bitflip(err)) {
 		unsigned int corrected = brcmnand_count_corrected(ctrl);
 
+		/* in case of edu correctable error we read again using pio */

s/edu/EDU/ ?
s/pio/PIO/

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip, · line 2153

Quoted 7 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c +4 -0

+		if (edu_read)
+			err = brcmnand_read_by_pio(mtd, chip, addr, trans, buf,
+						   oob, &err_addr);
+
 		dev_dbg(ctrl->dev, "corrected error at 0x%llx\n",
 			(unsigned long long)err_addr);
 		mtd->ecc_stats.corrected += corrected;

Thanks,
Miquèl

Kamal Dasu

author

2020-06-11 16:04 UTC #3 lore raw

Re: [PATCH 2/2] mtd: rawnand: brcmnand: Ecc error handling on EDU transfers

Quoted 16 lines · Miquel Raynal

Hi Kamal,
Kamal Dasu <[email protected]> wrote on Thu, 11 Jun 2020 01:44:54 -0400:
Quoted 1 line
Implemented ECC correctable and uncorrectable error handling for EDU
Implement?
Quoted 1 line
reads. If ECC correctable bitflips are encountered on EDU transfer,
extra space ^
Quoted 1 line
read page again using pio, This is needed due to a controller lmitation

Quoted 9 lines

s/pio/PIO/
Quoted 2 lines
where read and corrected data is not transferred to the DMA buffer on ECC errors. This holds true for ECC correctable errors beyond set threshold.
error.
Not sure what the last sentence means?

NAND controller allows for setting a correctable ECC threshold number of bits beyond which it will actually report the error to the driver. e.g. for BCH-4 the threshold is 3, so 3-bit and 4-bit errors will generate correctable ECC interrupt however 1-bit and 2-bit errors will be corrected silently.
From the above example EDU hardware will not transfer corrected data to the DMA buffer for 3-bit and 4-bit errors that get reported. So once we detect
the error duing EDU we read the page again using pio.

Quoted 58 lines

Quoted 4 lines
Fixes a5d53ad26a8b ("mtd: rawnand: brcmnand: Add support for flash-edu for dma transfers")
Signed-off-by Kamal Dasu <[email protected]>
---
Minor nits below :)
drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_edu_trans(struct brcmnand_host *host, u64 addr, u32 *buf, · line 1858
Quoted 12 lines
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 26 ++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +1 -0
index 0c1d6e543586..d7daa83c8a58 100644
@@ -1855,6 +1855,22 @@ static int brcmnand_edu_trans(struct brcmnand_host *host, u64 addr, u32 *buf,
      edu_writel(ctrl, EDU_STOP, 0); /* force stop */
      edu_readl(ctrl, EDU_STOP);
 
+     if (ret == 0 && edu_cmd == EDU_CMD_READ) {
!ret
drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_edu_trans(struct brcmnand_host *host, u64 addr, u32 *buf, · line 1863
Quoted 5 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +5 -0
+             u64 err_addr = 0;
+
+             /*
+              * check for ecc errors here, subpage ecc erros are
+              * retained in ecc error addr register
s/ecc/ECC/
s/erros/errors/
s/addr/address/
drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_edu_trans(struct brcmnand_host *host, u64 addr, u32 *buf, · line 1871
Quoted 8 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +8 -0
+              */
+             err_addr = brcmnand_get_uncorrecc_addr(ctrl);
+             if (!err_addr) {
+                     err_addr = brcmnand_get_correcc_addr(ctrl);
+                     if (err_addr)
+                             ret = -EUCLEAN;
+             } else
+                     ret = -EBADMSG;
I don't like very much to see these values being used within NAND controller drivers but I see it's already the cause, so I guess I can live with that.
Quoted 13 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +3 -0
+     }
+
      return ret;
 }
 
@@ -2058,6 +2074,7 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
      u64 err_addr = 0;
      int err;
      bool retry = true;
+     bool edu_read = false;
 
      dev_dbg(ctrl->dev, "read %llx -> %p\n", (unsigned long long)addr, buf);
 

Quoted 13 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip, · line 2097
Quoted 7 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +3 -0
@@ -2075,6 +2092,10 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
                      else
                              return -EIO;
              }
+
+             if (has_edu(ctrl))
+                     edu_read = true;
You don't need this extra value, you already have the cmd parameter which tells you if it is a read or a write. You might even want to create a if block so set dir and edu_cmd and eventually a local edu_read if you think it still makes sense.

I needed the value since dma and edu read has multiple conditions like oob is not included, buffer is aligned, virtual address is good. This indicates to
the if (mtd_is_bitflip(err)) block that the error was from an edu transaction that happened.This way all ecc error handling for dma, edu, pio is in one place.
Also there is more controller version specific logic for read error handling in there and this allows us to maintain the hierarchy how we handle both correctable
and uncorrectable error.

Quoted 8 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c +1 -0

+
      } else {
              if (oob)
                      memset(oob, 0x99, mtd->oobsize);

@@ -2122,6 +2143,11 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
      if (mtd_is_bitflip(err)) {
              unsigned int corrected = brcmnand_count_corrected(ctrl);

Quoted 13 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip, · line 2146

Quoted 1 line

drivers/mtd/nand/raw/brcmnand/brcmnand.c +1 -0

+             /* in case of edu correctable error we read again using pio */

s/edu/EDU/ ?
s/pio/PIO/

Quoted 7 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c +4 -0

+             if (edu_read)
+                     err = brcmnand_read_by_pio(mtd, chip, addr, trans, buf,
+                                                oob, &err_addr);
+
              dev_dbg(ctrl->dev, "corrected error at 0x%llx\n",
                      (unsigned long long)err_addr);
              mtd->ecc_stats.corrected += corrected;

Will fix all the other typos.

Quoted 2 lines

Thanks,
Miquèl

Thanks
Kamal

Miquel Raynal

maintainer

2020-06-12 07:07 UTC #4 lore raw

Re: [PATCH 2/2] mtd: rawnand: brcmnand: Ecc error handling on EDU transfers

Hi Kamal,

Kamal Dasu <[email protected]> wrote on Thu, 11 Jun 2020 12:04:29 -0400:

Quoted 27 lines · Miquel Raynal

Quoted 16 lines
Hi Kamal,
Kamal Dasu <[email protected]> wrote on Thu, 11 Jun 2020 01:44:54 -0400:
Quoted 1 line
Implemented ECC correctable and uncorrectable error handling for EDU
Implement?
Quoted 1 line
reads. If ECC correctable bitflips are encountered on EDU transfer,
extra space ^
Quoted 1 line
read page again using pio, This is needed due to a controller lmitation
Quoted 9 lines
s/pio/PIO/
Quoted 2 lines
where read and corrected data is not transferred to the DMA buffer on ECC errors. This holds true for ECC correctable errors beyond set threshold.
error.
Not sure what the last sentence means?

Quoted 9 lines

NAND controller allows for setting a correctable ECC threshold number of bits beyond which it will actually report the error to the driver. e.g. for BCH-4 the threshold is 3, so 3-bit and 4-bit errors will generate correctable ECC interrupt however 1-bit and 2-bit errors will be corrected silently.
From the above example EDU hardware will not transfer corrected data to the DMA buffer for 3-bit and 4-bit errors that get reported. So once we detect
the error duing EDU we read the page again using pio.

Ok I see what you mean, can't you fake the threshold instead? The NAND controller in Linux is not supposed to handle this threshold, the NAND core is in charge. So what the controller driver should do is just:
increase the number of bitflips + return the maximum number or bitflip or increase the failure counter. Is this already the case?

Quoted 42 lines

Quoted 42 lines
Quoted 5 lines
Fixes a5d53ad26a8b ("mtd: rawnand: brcmnand: Add support for flash-edu for dma transfers")
Signed-off-by Kamal Dasu <[email protected]>
---
Minor nits below :)
drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_edu_trans(struct brcmnand_host *host, u64 addr, u32 *buf, · line 1858
Quoted 12 lines
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 26 ++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +1 -0
index 0c1d6e543586..d7daa83c8a58 100644
@@ -1855,6 +1855,22 @@ static int brcmnand_edu_trans(struct brcmnand_host *host, u64 addr, u32 *buf,
      edu_writel(ctrl, EDU_STOP, 0); /* force stop */
      edu_readl(ctrl, EDU_STOP);
 
+     if (ret == 0 && edu_cmd == EDU_CMD_READ) {  
!ret
drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_edu_trans(struct brcmnand_host *host, u64 addr, u32 *buf, · line 1863
Quoted 5 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +5 -0
+             u64 err_addr = 0;
+
+             /*
+              * check for ecc errors here, subpage ecc erros are
+              * retained in ecc error addr register  
s/ecc/ECC/
s/erros/errors/
s/addr/address/
Quoted 8 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +8 -0
+              */
+             err_addr = brcmnand_get_uncorrecc_addr(ctrl);
+             if (!err_addr) {
+                     err_addr = brcmnand_get_correcc_addr(ctrl);
+                     if (err_addr)
+                             ret = -EUCLEAN;
+             } else
+                     ret = -EBADMSG;  

Quoted 2 lines

Quoted 2 lines
I don't like very much to see these values being used within NAND controller drivers but I see it's already the cause, so I guess I can

s/cause/case/

Quoted 29 lines

Quoted 15 lines
live with that.
Quoted 13 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +3 -0
+     }
+
      return ret;
 }
 
@@ -2058,6 +2074,7 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
      u64 err_addr = 0;
      int err;
      bool retry = true;
+     bool edu_read = false;
 
      dev_dbg(ctrl->dev, "read %llx -> %p\n", (unsigned long long)addr, buf);
 
Quoted 13 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip, · line 2097
Quoted 7 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +3 -0
@@ -2075,6 +2092,10 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
                      else
                              return -EIO;
              }
+
+             if (has_edu(ctrl))
+                     edu_read = true;  
You don't need this extra value, you already have the cmd parameter which tells you if it is a read or a write. You might even want to create a if block so set dir and edu_cmd and eventually a local edu_read if you think it still makes sense.

Quoted 10 lines

I needed the value since dma and edu read has multiple conditions like oob is not included, buffer is aligned, virtual address is good. This indicates to
the if (mtd_is_bitflip(err)) block that the error was from an edu transaction that happened.This way all ecc error handling for dma, edu, pio is in one place.
Also there is more controller version specific logic for read error handling in there and this allows us to maintain the hierarchy how we handle both correctable
and uncorrectable error.

Fair enough.

Quoted 23 lines

Quoted 9 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c +1 -0

+
      } else {
              if (oob)
                      memset(oob, 0x99, mtd->oobsize);

@@ -2122,6 +2143,11 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
      if (mtd_is_bitflip(err)) {
              unsigned int corrected = brcmnand_count_corrected(ctrl);

Quoted 13 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip, · line 2146

Quoted 1 line

drivers/mtd/nand/raw/brcmnand/brcmnand.c +1 -0

+             /* in case of edu correctable error we read again using pio */

s/edu/EDU/ ?
s/pio/PIO/

Quoted 7 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c +4 -0

+             if (edu_read)
+                     err = brcmnand_read_by_pio(mtd, chip, addr, trans, buf,
+                                                oob, &err_addr);
+
              dev_dbg(ctrl->dev, "corrected error at 0x%llx\n",
                      (unsigned long long)err_addr);
              mtd->ecc_stats.corrected += corrected;

Quoted 7 lines

Will fix all the other typos.
Quoted 2 lines
Thanks,
Miquèl
Thanks
Kamal

Thanks,
Miquèl

Kamal Dasu

author

2020-06-12 16:34 UTC #5 lore raw

Re: [PATCH 2/2] mtd: rawnand: brcmnand: Ecc error handling on EDU transfers

Quoted 43 lines · Miquel Raynal

Hi Kamal,
Kamal Dasu <[email protected]> wrote on Thu, 11 Jun 2020 12:04:29 -0400:
Quoted 27 lines · Miquel Raynal
Quoted 16 lines
Hi Kamal,
Kamal Dasu <[email protected]> wrote on Thu, 11 Jun 2020 01:44:54 -0400:
Quoted 1 line
Implemented ECC correctable and uncorrectable error handling for EDU
Implement?
Quoted 1 line
reads. If ECC correctable bitflips are encountered on EDU transfer,
extra space ^
Quoted 1 line
read page again using pio, This is needed due to a controller lmitation
Quoted 9 lines
s/pio/PIO/
Quoted 2 lines
where read and corrected data is not transferred to the DMA buffer on ECC errors. This holds true for ECC correctable errors beyond set threshold.
error.
Not sure what the last sentence means?
Quoted 9 lines
NAND controller allows for setting a correctable ECC threshold number of bits beyond which it will actually report the error to the driver. e.g. for BCH-4 the threshold is 3, so 3-bit and 4-bit errors will generate correctable ECC interrupt however 1-bit and 2-bit errors will be corrected silently.
From the above example EDU hardware will not transfer corrected data to the DMA buffer for 3-bit and 4-bit errors that get reported. So once we detect
the error duing EDU we read the page again using pio.

Quoted 6 lines

Ok I see what you mean, can't you fake the threshold instead? The NAND controller in Linux is not supposed to handle this threshold, the NAND core is in charge. So what the controller driver should do is just:
increase the number of bitflips + return the maximum number or bitflip or increase the failure counter. Is this already the case?

/* threshold = ceil(BCH-level * 0.75) */ brcmnand_wr_corr_thresh(host, DIV_ROUND_UP(chip->ecc.strength * 3, 4)); This how the threshold is set, all it means is that for high BCH levels don't interrupt on low number (less than threshold) of bit_flips. Yes the controller driver only increments correctable ECC count. But due the EDU design an EDU operation is disrupted when the controller interrupts on correctable ECC errors during subpage ECC calculations. Hence the driver needs to read the page again with PIO to transfer corrected data.

Quoted 123 lines

Quoted 42 lines
Quoted 42 lines
Quoted 5 lines
Fixes a5d53ad26a8b ("mtd: rawnand: brcmnand: Add support for flash-edu for dma transfers")
Signed-off-by Kamal Dasu <[email protected]>
---
Minor nits below :)
drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_edu_trans(struct brcmnand_host *host, u64 addr, u32 *buf, · line 1858
Quoted 12 lines
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 26 ++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +1 -0
index 0c1d6e543586..d7daa83c8a58 100644
@@ -1855,6 +1855,22 @@ static int brcmnand_edu_trans(struct brcmnand_host *host, u64 addr, u32 *buf,
      edu_writel(ctrl, EDU_STOP, 0); /* force stop */
      edu_readl(ctrl, EDU_STOP);
 
+     if (ret == 0 && edu_cmd == EDU_CMD_READ) {
!ret
drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_edu_trans(struct brcmnand_host *host, u64 addr, u32 *buf, · line 1863
Quoted 5 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +5 -0
+             u64 err_addr = 0;
+
+             /*
+              * check for ecc errors here, subpage ecc erros are
+              * retained in ecc error addr register
s/ecc/ECC/
s/erros/errors/
s/addr/address/
Quoted 8 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +8 -0
+              */
+             err_addr = brcmnand_get_uncorrecc_addr(ctrl);
+             if (!err_addr) {
+                     err_addr = brcmnand_get_correcc_addr(ctrl);
+                     if (err_addr)
+                             ret = -EUCLEAN;
+             } else
+                     ret = -EBADMSG;
Quoted 2 lines
Quoted 2 lines
I don't like very much to see these values being used within NAND controller drivers but I see it's already the cause, so I guess I can
s/cause/case/
Quoted 29 lines
Quoted 15 lines
live with that.
Quoted 13 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +3 -0
+     }
+
      return ret;
 }
 
@@ -2058,6 +2074,7 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
      u64 err_addr = 0;
      int err;
      bool retry = true;
+     bool edu_read = false;
 
      dev_dbg(ctrl->dev, "read %llx -> %p\n", (unsigned long long)addr, buf);
 
Quoted 13 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip, · line 2097
Quoted 7 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +3 -0
@@ -2075,6 +2092,10 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
                      else
                              return -EIO;
              }
+
+             if (has_edu(ctrl))
+                     edu_read = true;
You don't need this extra value, you already have the cmd parameter which tells you if it is a read or a write. You might even want to create a if block so set dir and edu_cmd and eventually a local edu_read if you think it still makes sense.
Quoted 10 lines
I needed the value since dma and edu read has multiple conditions like oob is not included, buffer is aligned, virtual address is good. This indicates to
the if (mtd_is_bitflip(err)) block that the error was from an edu transaction that happened.This way all ecc error handling for dma, edu, pio is in one place.
Also there is more controller version specific logic for read error handling in there and this allows us to maintain the hierarchy how we handle both correctable
and uncorrectable error.
Fair enough.
Quoted 23 lines
Quoted 9 lines
Quoted 9 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +1 -0
+
      } else {
              if (oob)
                      memset(oob, 0x99, mtd->oobsize);
@@ -2122,6 +2143,11 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
      if (mtd_is_bitflip(err)) {
              unsigned int corrected = brcmnand_count_corrected(ctrl);
 
Quoted 13 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip, · line 2146
Quoted 1 line
drivers/mtd/nand/raw/brcmnand/brcmnand.c +1 -0
+             /* in case of edu correctable error we read again using pio */
s/edu/EDU/ ?
s/pio/PIO/
Quoted 7 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +4 -0
+             if (edu_read)
+                     err = brcmnand_read_by_pio(mtd, chip, addr, trans, buf,
+                                                oob, &err_addr);
+
              dev_dbg(ctrl->dev, "corrected error at 0x%llx\n",
                      (unsigned long long)err_addr);
              mtd->ecc_stats.corrected += corrected;
Quoted 7 lines
Will fix all the other typos.
Quoted 2 lines
Thanks,
Miquèl
Thanks
Kamal
Thanks,
Miquèl

Miquel Raynal

maintainer

2020-06-15 07:19 UTC #6 lore raw

Re: [PATCH 2/2] mtd: rawnand: brcmnand: Ecc error handling on EDU transfers

Hi Kamal,

Kamal Dasu <[email protected]> wrote on Fri, 12 Jun 2020 12:34:22 -0400:

Quoted 50 lines · Miquel Raynal

Quoted 43 lines
Hi Kamal,
Kamal Dasu <[email protected]> wrote on Thu, 11 Jun 2020 12:04:29 -0400:
Quoted 27 lines · Miquel Raynal
Quoted 16 lines
Hi Kamal,
Kamal Dasu <[email protected]> wrote on Thu, 11 Jun 2020 01:44:54 -0400:
Quoted 1 line
Implemented ECC correctable and uncorrectable error handling for EDU
Implement?
Quoted 1 line
reads. If ECC correctable bitflips are encountered on EDU transfer,
extra space ^
Quoted 1 line
read page again using pio, This is needed due to a controller lmitation
Quoted 9 lines
s/pio/PIO/
Quoted 2 lines
where read and corrected data is not transferred to the DMA buffer on ECC errors. This holds true for ECC correctable errors beyond set threshold.
error.
Not sure what the last sentence means?
Quoted 9 lines
NAND controller allows for setting a correctable ECC threshold number of bits beyond which it will actually report the error to the driver. e.g. for BCH-4 the threshold is 3, so 3-bit and 4-bit errors will generate correctable ECC interrupt however 1-bit and 2-bit errors will be corrected silently.
From the above example EDU hardware will not transfer corrected data to the DMA buffer for 3-bit and 4-bit errors that get reported. So once we detect
the error duing EDU we read the page again using pio.
Quoted 6 lines
Ok I see what you mean, can't you fake the threshold instead? The NAND controller in Linux is not supposed to handle this threshold, the NAND core is in charge. So what the controller driver should do is just:
increase the number of bitflips + return the maximum number or bitflip or increase the failure counter. Is this already the case?

Quoted 9 lines

/* threshold = ceil(BCH-level * 0.75) */ brcmnand_wr_corr_thresh(host, DIV_ROUND_UP(chip->ecc.strength * 3, 4)); This how the threshold is set, all it means is that for high BCH levels don't interrupt on low number (less than threshold) of bit_flips. Yes the controller driver only increments correctable ECC count. But due the EDU design an EDU operation is disrupted when the controller interrupts on correctable ECC errors during subpage ECC calculations. Hence the driver needs to read the page again with PIO to transfer corrected data.

IIUC, you are doing the job twice: you should just return a number of bitflips or an error to the NAND core. So that's why I'm telling that you should get rid of this threshold. It would avoid the need for the PIO transfer too.

You also say that the controller "only increments correctable ECC count", what do you mean exactly? The controller does not report errors when the number of bitflips happens to be above the BCH threshold? This would be the only case where what is currently done would be actually needed though.

Thanks,
Miquèl

Kamal Dasu

author

2020-06-15 15:11 UTC #7 lore raw

Re: [PATCH 2/2] mtd: rawnand: brcmnand: Ecc error handling on EDU transfers

Quoted 66 lines · Miquel Raynal

Hi Kamal,
Kamal Dasu <[email protected]> wrote on Fri, 12 Jun 2020 12:34:22 -0400:
Quoted 50 lines · Miquel Raynal
Quoted 43 lines
Hi Kamal,
Kamal Dasu <[email protected]> wrote on Thu, 11 Jun 2020 12:04:29 -0400:
Quoted 27 lines · Miquel Raynal
Quoted 16 lines
Hi Kamal,
Kamal Dasu <[email protected]> wrote on Thu, 11 Jun 2020 01:44:54 -0400:
Quoted 1 line
Implemented ECC correctable and uncorrectable error handling for EDU
Implement?
Quoted 1 line
reads. If ECC correctable bitflips are encountered on EDU transfer,
extra space ^
Quoted 1 line
read page again using pio, This is needed due to a controller lmitation
Quoted 9 lines
s/pio/PIO/
Quoted 2 lines
where read and corrected data is not transferred to the DMA buffer on ECC errors. This holds true for ECC correctable errors beyond set threshold.
error.
Not sure what the last sentence means?
Quoted 9 lines
NAND controller allows for setting a correctable ECC threshold number of bits beyond which it will actually report the error to the driver. e.g. for BCH-4 the threshold is 3, so 3-bit and 4-bit errors will generate correctable ECC interrupt however 1-bit and 2-bit errors will be corrected silently.
From the above example EDU hardware will not transfer corrected data to the DMA buffer for 3-bit and 4-bit errors that get reported. So once we detect
the error duing EDU we read the page again using pio.
Quoted 6 lines
Ok I see what you mean, can't you fake the threshold instead? The NAND controller in Linux is not supposed to handle this threshold, the NAND core is in charge. So what the controller driver should do is just:
increase the number of bitflips + return the maximum number or bitflip or increase the failure counter. Is this already the case?
Quoted 9 lines
/* threshold = ceil(BCH-level * 0.75) */ brcmnand_wr_corr_thresh(host, DIV_ROUND_UP(chip->ecc.strength * 3, 4)); This how the threshold is set, all it means is that for high BCH levels don't interrupt on low number (less than threshold) of bit_flips. Yes the controller driver only increments correctable ECC count. But due the EDU design an EDU operation is disrupted when the controller interrupts on correctable ECC errors during subpage ECC calculations. Hence the driver needs to read the page again with PIO to transfer corrected data.

Quoted 4 lines

IIUC, you are doing the job twice: you should just return a number of bitflips or an error to the NAND core. So that's why I'm telling that you should get rid of this threshold. It would avoid the need for the PIO transfer too.

I think you are reading some statements in isolation that probably are causing some confusion.
EDU design has a flaw in case of reported ECC error interrupt in that corrected data is not transferred to the DMA buffer. The PIO is needed to read corrected data into the NAND data buffer and only for the reported errors. So there is no need to change the threshold calculation logic, if we get rid of the threshold then we will have to do the PIO read on any correctable bit error if it occurs during EDU reads.

Quoted 3 lines

You also say that the controller "only increments correctable ECC count", what do you mean exactly?

Maybe that statement was a bit misleading. To be clear when an ECC error is reported the controller gives the bit_flips count as well as updates the ECC error address Register and ecc error status registers. This logic works as expected in the hardware.

Quoted 4 lines

The controller does not report errors
when the number of bitflips happens to be above the BCH threshold? This would be the only case where what is currently done would be actually needed though.

It's the other way. The controller only reports bit errors beyond

Quoted 1 line

=threshold value, will not report otherwise and silently correct the

data. There is no problem in cases where erros are corrected silently. Now ECC (un)correctable on EDU reads are detected by simply reading back the ECC Error address register. And in case of reported uncorrectable ECC errors are treated as usual. And for reported correctable ECC errors we need to read the page again using PIO so that the corrected data is properly transferred. All this applies to EDU transfer only.

Quoted 3 lines

Thanks,
Miquèl

Kamal

Miquel Raynal

maintainer

2020-06-15 17:37 UTC #8 lore raw

Re: [PATCH 2/2] mtd: rawnand: brcmnand: Ecc error handling on EDU transfers

Hi Kamal,

Kamal Dasu <[email protected]> wrote on Mon, 15 Jun 2020 11:11:00 -0400:

Quoted 97 lines · Miquel Raynal

Quoted 66 lines
Hi Kamal,
Kamal Dasu <[email protected]> wrote on Fri, 12 Jun 2020 12:34:22 -0400:
Quoted 50 lines · Miquel Raynal
Quoted 43 lines
Hi Kamal,
Kamal Dasu <[email protected]> wrote on Thu, 11 Jun 2020 12:04:29 -0400:
Quoted 27 lines · Miquel Raynal
Quoted 16 lines
Hi Kamal,
Kamal Dasu <[email protected]> wrote on Thu, 11 Jun 2020 01:44:54 -0400:
Quoted 1 line
Implemented ECC correctable and uncorrectable error handling for EDU
Implement?
Quoted 1 line
reads. If ECC correctable bitflips are encountered on EDU transfer,
extra space ^
Quoted 1 line
read page again using pio, This is needed due to a controller lmitation
Quoted 9 lines
s/pio/PIO/
Quoted 2 lines
where read and corrected data is not transferred to the DMA buffer on ECC errors. This holds true for ECC correctable errors beyond set threshold.
error.
Not sure what the last sentence means?
Quoted 9 lines
NAND controller allows for setting a correctable ECC threshold number of bits beyond which it will actually report the error to the driver. e.g. for BCH-4 the threshold is 3, so 3-bit and 4-bit errors will generate correctable ECC interrupt however 1-bit and 2-bit errors will be corrected silently.
From the above example EDU hardware will not transfer corrected data to the DMA buffer for 3-bit and 4-bit errors that get reported. So once we detect
the error duing EDU we read the page again using pio.
Quoted 6 lines
Ok I see what you mean, can't you fake the threshold instead? The NAND controller in Linux is not supposed to handle this threshold, the NAND core is in charge. So what the controller driver should do is just:
increase the number of bitflips + return the maximum number or bitflip or increase the failure counter. Is this already the case?
Quoted 9 lines
/* threshold = ceil(BCH-level * 0.75) */ brcmnand_wr_corr_thresh(host, DIV_ROUND_UP(chip->ecc.strength * 3, 4)); This how the threshold is set, all it means is that for high BCH levels don't interrupt on low number (less than threshold) of bit_flips. Yes the controller driver only increments correctable ECC count. But due the EDU design an EDU operation is disrupted when the controller interrupts on correctable ECC errors during subpage ECC calculations. Hence the driver needs to read the page again with PIO to transfer corrected data.
Quoted 4 lines
IIUC, you are doing the job twice: you should just return a number of bitflips or an error to the NAND core. So that's why I'm telling that you should get rid of this threshold. It would avoid the need for the PIO transfer too.
I think you are reading some statements in isolation that probably are causing some confusion.
EDU design has a flaw in case of reported ECC error interrupt in that corrected data is not transferred to the DMA buffer. The PIO is needed to read corrected data into the NAND data buffer and only for the reported errors. So there is no need to change the threshold calculation logic, if we get rid of the threshold then we will have to do the PIO read on any correctable bit error if it occurs during EDU reads.
Quoted 3 lines
You also say that the controller "only increments correctable ECC count", what do you mean exactly?
Maybe that statement was a bit misleading. To be clear when an ECC error is reported the controller gives the bit_flips count as well as updates the ECC error address Register and ecc error status registers. This logic works as expected in the hardware.
Quoted 4 lines
The controller does not report errors
when the number of bitflips happens to be above the BCH threshold? This would be the only case where what is currently done would be actually needed though.
It's the other way. The controller only reports bit errors beyond

Quoted 9 lines

Quoted 1 line
=threshold value, will not report otherwise and silently correct the
data. There is no problem in cases where erros are corrected silently. Now ECC (un)correctable on EDU reads are detected by simply reading back the ECC Error address register. And in case of reported uncorrectable ECC errors are treated as usual. And for reported correctable ECC errors we need to read the page again using PIO so that the corrected data is properly transferred. All this applies to EDU transfer only.

Thank you very much for the explanation, I understand better how this controller works now.

Cheers,
Miquèl

Miquel Raynal

maintainer

2020-06-11 07:16 UTC #9 lore raw

In reply to Kamal Dasu

Hi Kamal,

Kamal Dasu <[email protected]> wrote on Thu, 11 Jun 2020 01:44:53 -0400:

Quoted 15 lines

When flash-dma is absent do not default to using flash-edu. Make sure flash-edu is enabled before setting EDU transfer function.
Fixes a5d53ad26a8b ("mtd: rawnand: brcmnand: Add support for flash-edu for dma transfers")
Signed-off-by Kamal Dasu <[email protected]>
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +0 -0
index 8f9ffb46a09f..0c1d6e543586 100644

drivers/mtd/nand/raw/brcmnand/brcmnand.c int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc) · line 2958

Quoted 8 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c +3 -2

@@ -2953,8 +2953,9 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
 		if (ret < 0)
 			goto err;
 
-		/* set edu transfer function to call */
-		ctrl->dma_trans = brcmnand_edu_trans;
+		if (has_edu(ctrl))
+			/* set edu transfer function to call */
+			ctrl->dma_trans = brcmnand_edu_trans;

Does this fallback to returning an error in case !has_edu() ? Othewise, how is it handled?

Thanks,
Miquèl

Kamal Dasu

author

2020-06-11 15:23 UTC #10 lore raw

Quoted 24 lines · Miquel Raynal

Hi Kamal,
Kamal Dasu <[email protected]> wrote on Thu, 11 Jun 2020 01:44:53 -0400:
Quoted 18 lines
When flash-dma is absent do not default to using flash-edu. Make sure flash-edu is enabled before setting EDU transfer function.
Fixes a5d53ad26a8b ("mtd: rawnand: brcmnand: Add support for flash-edu for dma transfers")
Signed-off-by Kamal Dasu <[email protected]>
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +0 -0
index 8f9ffb46a09f..0c1d6e543586 100644
@@ -2953,8 +2953,9 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
              if (ret < 0)
                      goto err;
 

Quoted 9 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc) · line 2958
Quoted 5 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +3 -2
-             /* set edu transfer function to call */
-             ctrl->dma_trans = brcmnand_edu_trans;
+             if (has_edu(ctrl))
+                     /* set edu transfer function to call */
+                     ctrl->dma_trans = brcmnand_edu_trans;
Does this fallback to returning an error in case !has_edu() ? Othewise, how is it handled?

The driver will default to pio if both flash-dma and falsh-edu are
not present.

Quoted 2 lines

Thanks,
Miquèl

Kamal

Miquel Raynal

maintainer

2020-06-11 15:24 UTC #11 lore raw

Hi Kamal,

Kamal Dasu <[email protected]> wrote on Thu, 11 Jun 2020 11:22:56 -0400:

Quoted 35 lines · Miquel Raynal

Quoted 24 lines
Hi Kamal,
Kamal Dasu <[email protected]> wrote on Thu, 11 Jun 2020 01:44:53 -0400:
Quoted 18 lines
When flash-dma is absent do not default to using flash-edu. Make sure flash-edu is enabled before setting EDU transfer function.
Fixes a5d53ad26a8b ("mtd: rawnand: brcmnand: Add support for flash-edu for dma transfers")
Signed-off-by Kamal Dasu <[email protected]>
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +0 -0
index 8f9ffb46a09f..0c1d6e543586 100644
@@ -2953,8 +2953,9 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
              if (ret < 0)
                      goto err;
 
Quoted 9 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc) · line 2958
Quoted 5 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +3 -2
-             /* set edu transfer function to call */
-             ctrl->dma_trans = brcmnand_edu_trans;
+             if (has_edu(ctrl))
+                     /* set edu transfer function to call */
+                     ctrl->dma_trans = brcmnand_edu_trans;  
Does this fallback to returning an error in case !has_edu() ? Othewise, how is it handled?

Quoted 7 lines

The driver will default to pio if both flash-dma and falsh-edu are
not present.
Quoted 2 lines
Thanks,
Miquèl
Kamal

Ok, thanks for the clarification!

Thanks,
Miquèl