[U-Boot] [PATCH 0/1] mtd: nand: fsl-ifc: Fix handling of bitflips

[U-Boot] [PATCH 0/1] mtd: nand: fsl-ifc: Fix handling of bitflips

[Kurt Kanzenbach]

Hi,

this is a resend of the following patch:

 https://patchwork.ozlabs.org/patch/774424/

It seems like that patch hasn't been merged, yet.

However, I stumbled across the same issue. Linux has an equivalent patch already
upstream.

Furthermore, I've tested the patch on hardware and it seems to work fine.

Thanks,
Kurt

Darwin Dingel (1):
  mtd: nand: fsl_ifc: Fix handling of bitflips in erased pages

 drivers/mtd/nand/fsl_ifc_nand.c | 69 +++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 30 deletions(-)

-- 
2.11.0

[U-Boot] [PATCH 1/1] mtd: nand: fsl_ifc: Fix handling of bitflips in erased pages

[Kurt Kanzenbach]

From: Darwin Dingel <darwin.dingel@alliedtelesis.co.nz>

This is a fix made for the fsl_ifc_nand driver on linux kernel by
Pavel Machek and is applied to uboot. It is currently on applied on
linux-mtd.

https://patchwork.kernel.org/patch/9758117/

IFC always raises ECC errors on erased pages. It is only ignored when
the buffer is checked for all 0xFF by is_blank(). The problem is a
single bitflip will cause is_blank() and then mtd_read to fail. The fix
makes use of nand_check_erased_ecc_chunk() to check for empty pages
instead of is_blank(). This also makes sure that reads are made at ECC
page size granularity to get a proper bitflip count. If the number of
bitflips does not exceed the ECC strength, the page is considered empty
and the bitflips will be corrected when data is sent to the higher
layers (e.g. ubi).

Signed-off-by: Darwin Dingel <darwin.dingel@alliedtelesis.co.nz>
Cc: Pavel Machek <pavel@denx.de>
Cc: Scott Wood <oss@buserror.net>
Acked-by: Pavel Machek <pavel@denx.de>
[Kurt: Replaced dev_err by printf due to compiler warnings]
Tested-by: Kurt Kanzenbach <kurt@linutronix.de>
Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/mtd/nand/fsl_ifc_nand.c | 69 +++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 30 deletions(-)

diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
index a47226bd2158..29f30d8ccc42 100644
--- a/drivers/mtd/nand/fsl_ifc_nand.c
+++ b/drivers/mtd/nand/fsl_ifc_nand.c
@@ -242,31 +242,6 @@ static void set_addr(struct mtd_info *mtd, int column, int page_addr, int oob)
 		ctrl->index += mtd->writesize;
 }
 
-static int is_blank(struct mtd_info *mtd, struct fsl_ifc_ctrl *ctrl,
-		    unsigned int bufnum)
-{
-	struct nand_chip *chip = mtd_to_nand(mtd);
-	struct fsl_ifc_mtd *priv = nand_get_controller_data(chip);
-	u8 __iomem *addr = priv->vbase + bufnum * (mtd->writesize * 2);
-	u32 __iomem *main = (u32 *)addr;
-	u8 __iomem *oob = addr + mtd->writesize;
-	int i;
-
-	for (i = 0; i < mtd->writesize / 4; i++) {
-		if (__raw_readl(&main[i]) != 0xffffffff)
-			return 0;
-	}
-
-	for (i = 0; i < chip->ecc.layout->eccbytes; i++) {
-		int pos = chip->ecc.layout->eccpos[i];
-
-		if (__raw_readb(&oob[pos]) != 0xff)
-			return 0;
-	}
-
-	return 1;
-}
-
 /* returns nonzero if entire page is blank */
 static int check_read_ecc(struct mtd_info *mtd, struct fsl_ifc_ctrl *ctrl,
 			  u32 eccstat, unsigned int bufnum)
@@ -331,16 +306,14 @@ static int fsl_ifc_run_command(struct mtd_info *mtd)
 			if (errors == 15) {
 				/*
 				 * Uncorrectable error.
-				 * OK only if the whole page is blank.
+				 * We'll check for blank pages later.
 				 *
 				 * We disable ECCER reporting due to erratum
 				 * IFC-A002770 -- so report it now if we
 				 * see an uncorrectable error in ECCSTAT.
 				 */
-				if (!is_blank(mtd, ctrl, bufnum))
-					ctrl->status |=
-						IFC_NAND_EVTER_STAT_ECCER;
-				break;
+				ctrl->status |= IFC_NAND_EVTER_STAT_ECCER;
+				continue;
 			}
 
 			mtd->ecc_stats.corrected += errors;
@@ -727,6 +700,39 @@ static int fsl_ifc_wait(struct mtd_info *mtd, struct nand_chip *chip)
 	return status | NAND_STATUS_WP;
 }
 
+/*
+ * The controller does not check for bitflips in erased pages,
+ * therefore software must check instead.
+ */
+static int
+check_erased_page(struct nand_chip *chip, u8 *buf, struct mtd_info *mtd)
+{
+	u8 *ecc = chip->oob_poi;
+	const int ecc_size = chip->ecc.bytes;
+	const int pkt_size = chip->ecc.size;
+	int i, res, bitflips;
+
+	/* IFC starts ecc bytes at offset 8 in the spare area. */
+	ecc += 8;
+	bitflips = 0;
+	for (i = 0; i < chip->ecc.steps; i++) {
+		res = nand_check_erased_ecc_chunk(buf, pkt_size, ecc, ecc_size,
+						  NULL, 0, chip->ecc.strength);
+
+		if (res < 0) {
+			printf("fsl-ifc: NAND Flash ECC Uncorrectable Error\n");
+			mtd->ecc_stats.failed++;
+		} else if (res > 0) {
+			mtd->ecc_stats.corrected += res;
+		}
+		bitflips = max(res, bitflips);
+		buf += pkt_size;
+		ecc += ecc_size;
+	}
+
+	return bitflips;
+}
+
 static int fsl_ifc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 			     uint8_t *buf, int oob_required, int page)
 {
@@ -736,6 +742,9 @@ static int fsl_ifc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 	fsl_ifc_read_buf(mtd, buf, mtd->writesize);
 	fsl_ifc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
 
+	if (ctrl->status & IFC_NAND_EVTER_STAT_ECCER)
+		return check_erased_page(chip, buf, mtd);
+
 	if (ctrl->status != IFC_NAND_EVTER_STAT_OPC)
 		mtd->ecc_stats.failed++;
 
-- 
2.11.0

[U-Boot] [PATCH 1/1] mtd: nand: fsl_ifc: Fix handling of bitflips in erased pages

[York Sun]

On 08/02/2018 01:03 AM, Kurt Kanzenbach wrote:
> From: Darwin Dingel <darwin.dingel@alliedtelesis.co.nz>
> 
> This is a fix made for the fsl_ifc_nand driver on linux kernel by
> Pavel Machek and is applied to uboot. It is currently on applied on
> linux-mtd.
> 
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fpatch%2F9758117%2F&amp;data=02%7C01%7Cyork.sun%40nxp.com%7C79aca70c61e943b59b1d08d5f84e77eb%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636687938268644852&amp;sdata=PjrH%2FF6LyGA%2BpVjHfqQpn4qhvJKeRHG%2Fb3434wuQy3o%3D&amp;reserved=0
> 
> IFC always raises ECC errors on erased pages. It is only ignored when
> the buffer is checked for all 0xFF by is_blank(). The problem is a
> single bitflip will cause is_blank() and then mtd_read to fail. The fix
> makes use of nand_check_erased_ecc_chunk() to check for empty pages
> instead of is_blank(). This also makes sure that reads are made at ECC
> page size granularity to get a proper bitflip count. If the number of
> bitflips does not exceed the ECC strength, the page is considered empty
> and the bitflips will be corrected when data is sent to the higher
> layers (e.g. ubi).
> 
> Signed-off-by: Darwin Dingel <darwin.dingel@alliedtelesis.co.nz>
> Cc: Pavel Machek <pavel@denx.de>
> Cc: Scott Wood <oss@buserror.net>
> Acked-by: Pavel Machek <pavel@denx.de>
> [Kurt: Replaced dev_err by printf due to compiler warnings]
> Tested-by: Kurt Kanzenbach <kurt@linutronix.de>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>  drivers/mtd/nand/fsl_ifc_nand.c | 69 +++++++++++++++++++++++------------------
>  1 file changed, 39 insertions(+), 30 deletions(-)

Scott,

Please comment. I can bring it in with your ack.

York

[U-Boot] [PATCH 1/1] mtd: nand: fsl_ifc: Fix handling of bitflips in erased pages

[Kurt Kanzenbach]

Hi York,

On Tue, Aug 07, 2018 at 09:52:46PM +0000, York Sun wrote:
> On 08/02/2018 01:03 AM, Kurt Kanzenbach wrote:
> > From: Darwin Dingel <darwin.dingel@alliedtelesis.co.nz>
> >
> > This is a fix made for the fsl_ifc_nand driver on linux kernel by
> > Pavel Machek and is applied to uboot. It is currently on applied on
> > linux-mtd.
> >
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fpatch%2F9758117%2F&amp;data=02%7C01%7Cyork.sun%40nxp.com%7C79aca70c61e943b59b1d08d5f84e77eb%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636687938268644852&amp;sdata=PjrH%2FF6LyGA%2BpVjHfqQpn4qhvJKeRHG%2Fb3434wuQy3o%3D&amp;reserved=0
> >
> > IFC always raises ECC errors on erased pages. It is only ignored when
> > the buffer is checked for all 0xFF by is_blank(). The problem is a
> > single bitflip will cause is_blank() and then mtd_read to fail. The fix
> > makes use of nand_check_erased_ecc_chunk() to check for empty pages
> > instead of is_blank(). This also makes sure that reads are made at ECC
> > page size granularity to get a proper bitflip count. If the number of
> > bitflips does not exceed the ECC strength, the page is considered empty
> > and the bitflips will be corrected when data is sent to the higher
> > layers (e.g. ubi).
> >
> > Signed-off-by: Darwin Dingel <darwin.dingel@alliedtelesis.co.nz>
> > Cc: Pavel Machek <pavel@denx.de>
> > Cc: Scott Wood <oss@buserror.net>
> > Acked-by: Pavel Machek <pavel@denx.de>
> > [Kurt: Replaced dev_err by printf due to compiler warnings]
> > Tested-by: Kurt Kanzenbach <kurt@linutronix.de>
> > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> > ---
> >  drivers/mtd/nand/fsl_ifc_nand.c | 69 +++++++++++++++++++++++------------------
> >  1 file changed, 39 insertions(+), 30 deletions(-)
>
> Scott,
>
> Please comment. I can bring it in with your ack.

I guess Scott stepped down as NAND maintainer (see MAINTAINERS
file). That's why I sent the patch directly to you.

Thanks,
Kurt

>
> York

[U-Boot] [PATCH 1/1] mtd: nand: fsl_ifc: Fix handling of bitflips in erased pages

[York Sun]

On 08/08/2018 04:55 AM, Kurt Kanzenbach wrote:
> Hi York,
> 
> On Tue, Aug 07, 2018 at 09:52:46PM +0000, York Sun wrote:
>> On 08/02/2018 01:03 AM, Kurt Kanzenbach wrote:
>>> From: Darwin Dingel <darwin.dingel@alliedtelesis.co.nz>
>>>
>>> This is a fix made for the fsl_ifc_nand driver on linux kernel by
>>> Pavel Machek and is applied to uboot. It is currently on applied on
>>> linux-mtd.
>>>
>>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fpatch%2F9758117%2F&amp;data=02%7C01%7Cyork.sun%40nxp.com%7C79aca70c61e943b59b1d08d5f84e77eb%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636687938268644852&amp;sdata=PjrH%2FF6LyGA%2BpVjHfqQpn4qhvJKeRHG%2Fb3434wuQy3o%3D&amp;reserved=0
>>>
>>> IFC always raises ECC errors on erased pages. It is only ignored when
>>> the buffer is checked for all 0xFF by is_blank(). The problem is a
>>> single bitflip will cause is_blank() and then mtd_read to fail. The fix
>>> makes use of nand_check_erased_ecc_chunk() to check for empty pages
>>> instead of is_blank(). This also makes sure that reads are made at ECC
>>> page size granularity to get a proper bitflip count. If the number of
>>> bitflips does not exceed the ECC strength, the page is considered empty
>>> and the bitflips will be corrected when data is sent to the higher
>>> layers (e.g. ubi).
>>>
>>> Signed-off-by: Darwin Dingel <darwin.dingel@alliedtelesis.co.nz>
>>> Cc: Pavel Machek <pavel@denx.de>
>>> Cc: Scott Wood <oss@buserror.net>
>>> Acked-by: Pavel Machek <pavel@denx.de>
>>> [Kurt: Replaced dev_err by printf due to compiler warnings]
>>> Tested-by: Kurt Kanzenbach <kurt@linutronix.de>
>>> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
>>> ---
>>>  drivers/mtd/nand/fsl_ifc_nand.c | 69 +++++++++++++++++++++++------------------
>>>  1 file changed, 39 insertions(+), 30 deletions(-)
>>
>> Scott,
>>
>> Please comment. I can bring it in with your ack.
> 
> I guess Scott stepped down as NAND maintainer (see MAINTAINERS
> file). That's why I sent the patch directly to you.
> 

The patch looks OK. I tested it on ls1043ardb with nand boot. I can see
this code works OK. Will bring it in on my next pull request.

York

[U-Boot] [PATCH 1/1] mtd: nand: fsl_ifc: Fix handling of bitflips in erased pages

[Kurt Kanzenbach]

On Thu, Aug 09, 2018 at 07:58:02PM +0000, York Sun wrote:
> On 08/08/2018 04:55 AM, Kurt Kanzenbach wrote:
> > Hi York,
> >
> > On Tue, Aug 07, 2018 at 09:52:46PM +0000, York Sun wrote:
> >> On 08/02/2018 01:03 AM, Kurt Kanzenbach wrote:
> >>> From: Darwin Dingel <darwin.dingel@alliedtelesis.co.nz>
> >>>
> >>> This is a fix made for the fsl_ifc_nand driver on linux kernel by
> >>> Pavel Machek and is applied to uboot. It is currently on applied on
> >>> linux-mtd.
> >>>
> >>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fpatch%2F9758117%2F&amp;data=02%7C01%7Cyork.sun%40nxp.com%7C79aca70c61e943b59b1d08d5f84e77eb%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636687938268644852&amp;sdata=PjrH%2FF6LyGA%2BpVjHfqQpn4qhvJKeRHG%2Fb3434wuQy3o%3D&amp;reserved=0
> >>>
> >>> IFC always raises ECC errors on erased pages. It is only ignored when
> >>> the buffer is checked for all 0xFF by is_blank(). The problem is a
> >>> single bitflip will cause is_blank() and then mtd_read to fail. The fix
> >>> makes use of nand_check_erased_ecc_chunk() to check for empty pages
> >>> instead of is_blank(). This also makes sure that reads are made at ECC
> >>> page size granularity to get a proper bitflip count. If the number of
> >>> bitflips does not exceed the ECC strength, the page is considered empty
> >>> and the bitflips will be corrected when data is sent to the higher
> >>> layers (e.g. ubi).
> >>>
> >>> Signed-off-by: Darwin Dingel <darwin.dingel@alliedtelesis.co.nz>
> >>> Cc: Pavel Machek <pavel@denx.de>
> >>> Cc: Scott Wood <oss@buserror.net>
> >>> Acked-by: Pavel Machek <pavel@denx.de>
> >>> [Kurt: Replaced dev_err by printf due to compiler warnings]
> >>> Tested-by: Kurt Kanzenbach <kurt@linutronix.de>
> >>> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> >>> ---
> >>>  drivers/mtd/nand/fsl_ifc_nand.c | 69 +++++++++++++++++++++++------------------
> >>>  1 file changed, 39 insertions(+), 30 deletions(-)
> >>
> >> Scott,
> >>
> >> Please comment. I can bring it in with your ack.
> >
> > I guess Scott stepped down as NAND maintainer (see MAINTAINERS
> > file). That's why I sent the patch directly to you.
> >
>
> The patch looks OK. I tested it on ls1043ardb with nand boot. I can see
> this code works OK. Will bring it in on my next pull request.

Thank you for testing the patch and bringing it in.

-- Kurt

[U-Boot] [PATCH 1/1] mtd: nand: fsl_ifc: Fix handling of bitflips in erased pages

[York Sun]

On 08/02/2018 01:03 AM, Kurt Kanzenbach wrote:
> From: Darwin Dingel <darwin.dingel@alliedtelesis.co.nz>
> 
> This is a fix made for the fsl_ifc_nand driver on linux kernel by
> Pavel Machek and is applied to uboot. It is currently on applied on
> linux-mtd.
> 
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fpatch%2F9758117%2F&amp;data=02%7C01%7Cyork.sun%40nxp.com%7C79aca70c61e943b59b1d08d5f84e77eb%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636687938268644852&amp;sdata=PjrH%2FF6LyGA%2BpVjHfqQpn4qhvJKeRHG%2Fb3434wuQy3o%3D&amp;reserved=0
> 
> IFC always raises ECC errors on erased pages. It is only ignored when
> the buffer is checked for all 0xFF by is_blank(). The problem is a
> single bitflip will cause is_blank() and then mtd_read to fail. The fix
> makes use of nand_check_erased_ecc_chunk() to check for empty pages
> instead of is_blank(). This also makes sure that reads are made at ECC
> page size granularity to get a proper bitflip count. If the number of
> bitflips does not exceed the ECC strength, the page is considered empty
> and the bitflips will be corrected when data is sent to the higher
> layers (e.g. ubi).
> 
> Signed-off-by: Darwin Dingel <darwin.dingel@alliedtelesis.co.nz>
> Cc: Pavel Machek <pavel@denx.de>
> Cc: Scott Wood <oss@buserror.net>
> Acked-by: Pavel Machek <pavel@denx.de>
> [Kurt: Replaced dev_err by printf due to compiler warnings]
> Tested-by: Kurt Kanzenbach <kurt@linutronix.de>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>  drivers/mtd/nand/fsl_ifc_nand.c | 69 +++++++++++++++++++++++------------------
>  1 file changed, 39 insertions(+), 30 deletions(-)


Applied to fsl-qoriq master, awaiting upstream.
Thanks.

York