[PATCH 0/9] sh_flctl hardware ECC mode cleanup

[PATCH 0/9] sh_flctl hardware ECC mode cleanup

[Bastian Hecht]

This series cleans up the flctl when run in hardware ecc mode. The first 2 patches make sure we catch all errors that result from hardware transmission. The other patches handle how the ecc is layed out, correct some code to write and read it and make sure we propagate the statistics about errors/repairs to the nand base.

Bastian Hecht (9):
  mtd: sh_flctl: Add support for error IRQ
  ARM: sh-mobile: mackerel: Add error IRQ resource
  mtd: sh_flctl: Use different OOB layout
  mtd: sh_flctl: Fix hardware ECC behaviour
  mtd: sh_flctl: Simplify the hardware ecc page read
  mtd: sh_flctl: Group sector accesses into a single transfer
  mtd: sh_flctl: Restructure the hardware ECC handling
  mtd: sh_flctl: Use user oob data in hardware ECC mode
  ARM: sh-mobile: mackerel: Use hardware error correction

 arch/arm/mach-shmobile/board-mackerel.c |    7 +-
 drivers/mtd/nand/sh_flctl.c             |  289 ++++++++++++++++++-------------
 include/linux/mtd/sh_flctl.h            |   21 ++-
 3 files changed, 188 insertions(+), 129 deletions(-)

-- 
1.7.5.4

[PATCH 1/9] mtd: sh_flctl: Add support for error IRQ

[Bastian Hecht]

When the data transfer between the controller and the NAND chip fails,
we now get notified.

Signed-off-by: Bastian Hecht <hechtb@gmail.com>
---
 drivers/mtd/nand/sh_flctl.c  |   31 +++++++++++++++++++++++++++++--
 include/linux/mtd/sh_flctl.h |    7 +++++++
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
index 2ee9a1b..3c27921 100644
--- a/drivers/mtd/nand/sh_flctl.c
+++ b/drivers/mtd/nand/sh_flctl.c
@@ -24,6 +24,7 @@
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/delay.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
@@ -69,7 +70,7 @@ static struct nand_bbt_descr flctl_4secc_largepage = {
 static void empty_fifo(struct sh_flctl *flctl)
 {
 	writel(0x000c0000, FLINTDMACR(flctl));	/* FIFO Clear */
-	writel(0x00000000, FLINTDMACR(flctl));	/* Clear Error flags */
+	writel(flctl->flintdmacr_base, FLINTDMACR(flctl));
 }
 
 static void start_translation(struct sh_flctl *flctl)
@@ -838,6 +839,14 @@ static int flctl_chip_init_tail(struct mtd_info *mtd)
 	return 0;
 }
 
+static irqreturn_t flctl_handle_flste(int irq, void *dev_id)
+{
+	struct sh_flctl *flctl = dev_id;
+	dev_err(&flctl->pdev->dev, "flste irq: %x\n", readl(FLINTDMACR(flctl)));
+
+	return IRQ_HANDLED;
+}
+
 static int __devinit flctl_probe(struct platform_device *pdev)
 {
 	struct resource *res;
@@ -846,6 +855,7 @@ static int __devinit flctl_probe(struct platform_device *pdev)
 	struct nand_chip *nand;
 	struct sh_flctl_platform_data *pdata;
 	int ret = -ENXIO;
+	int irq;
 
 	pdata = pdev->dev.platform_data;
 	if (pdata = NULL) {
@@ -871,14 +881,27 @@ static int __devinit flctl_probe(struct platform_device *pdev)
 		goto err_iomap;
 	}
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "failed to get flste irq data\n");
+		goto err_flste;
+	}
+
+	ret = request_irq(irq, flctl_handle_flste, IRQF_SHARED, "flste", flctl);
+	if (ret) {
+		dev_err(&pdev->dev, "request interrupt failed.\n");
+		goto err_flste;
+	}
+
 	platform_set_drvdata(pdev, flctl);
 	flctl_mtd = &flctl->mtd;
 	nand = &flctl->chip;
 	flctl_mtd->priv = nand;
 	flctl->pdev = pdev;
-	flctl->flcmncr_base = pdata->flcmncr_val;
 	flctl->hwecc = pdata->has_hwecc;
 	flctl->holden = pdata->use_holden;
+	flctl->flcmncr_base = pdata->flcmncr_val;
+	flctl->flintdmacr_base = flctl->hwecc ? (STERINTE | ECERB) : STERINTE;
 
 	nand->options = NAND_NO_AUTOINCR;
 
@@ -919,6 +942,9 @@ static int __devinit flctl_probe(struct platform_device *pdev)
 
 err_chip:
 	pm_runtime_disable(&pdev->dev);
+	free_irq(platform_get_irq(pdev, 0), flctl);
+err_flste:
+	iounmap(flctl->reg);
 err_iomap:
 	kfree(flctl);
 	return ret;
@@ -930,6 +956,7 @@ static int __devexit flctl_remove(struct platform_device *pdev)
 
 	nand_release(&flctl->mtd);
 	pm_runtime_disable(&pdev->dev);
+	free_irq(platform_get_irq(pdev, 0), flctl);
 	kfree(flctl);
 
 	return 0;
diff --git a/include/linux/mtd/sh_flctl.h b/include/linux/mtd/sh_flctl.h
index a38e1fa..6832a90 100644
--- a/include/linux/mtd/sh_flctl.h
+++ b/include/linux/mtd/sh_flctl.h
@@ -107,6 +107,12 @@
 #define DOCMD2_E	(0x1 << 17)	/* 2nd cmd stage execute */
 #define DOCMD1_E	(0x1 << 16)	/* 1st cmd stage execute */
 
+/* FLINTDMACR control bits */
+#define ESTERINTE	(0x1 << 24)	/* ECC error interrupt enable */
+#define ECERB		(0x1 << 9)	/* ECC error */
+#define STERB		(0x1 << 8)	/* Status error */
+#define STERINTE	(0x1 << 4)	/* Status error enable */
+
 /* FLTRCR control bits */
 #define TRSTRT		(0x1 << 0)	/* translation start */
 #define TREND		(0x1 << 1)	/* translation end */
@@ -145,6 +151,7 @@ struct sh_flctl {
 	uint32_t erase_ADRCNT;		/* bits of FLCMDCR in ERASE1 cmd */
 	uint32_t rw_ADRCNT;	/* bits of FLCMDCR in READ WRITE cmd */
 	uint32_t flcmncr_base;	/* base value of FLCMNCR */
+	uint32_t flintdmacr_base;	/* irq enable bits */
 
 	int	hwecc_cant_correct[4];
 
-- 
1.7.5.4

Re: [PATCH 1/9] mtd: sh_flctl: Add support for error IRQ

[Laurent Pinchart]

Hi Bastian,

Thanks for the patch.

On Friday 20 April 2012 11:13:42 Bastian Hecht wrote:
> When the data transfer between the controller and the NAND chip fails,
> we now get notified.
> 
> Signed-off-by: Bastian Hecht <hechtb@gmail.com>
> ---
>  drivers/mtd/nand/sh_flctl.c  |   31 +++++++++++++++++++++++++++++--
>  include/linux/mtd/sh_flctl.h |    7 +++++++
>  2 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
> index 2ee9a1b..3c27921 100644
> --- a/drivers/mtd/nand/sh_flctl.c
> +++ b/drivers/mtd/nand/sh_flctl.c
> @@ -24,6 +24,7 @@
>  #include <linux/module.h>
>  #include <linux/kernel.h>
>  #include <linux/delay.h>
> +#include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> @@ -69,7 +70,7 @@ static struct nand_bbt_descr flctl_4secc_largepage = {
>  static void empty_fifo(struct sh_flctl *flctl)
>  {
>  	writel(0x000c0000, FLINTDMACR(flctl));	/* FIFO Clear */

Shouldn't this write flctl->flintdmacr_base | 0x000c0000 ? If you change this 
line, you could also define and use the AC1CLR and AC0CLR bits.

> -	writel(0x00000000, FLINTDMACR(flctl));	/* Clear Error flags */
> +	writel(flctl->flintdmacr_base, FLINTDMACR(flctl));
>  }
> 
>  static void start_translation(struct sh_flctl *flctl)
> @@ -838,6 +839,14 @@ static int flctl_chip_init_tail(struct mtd_info *mtd)
>  	return 0;
>  }
> 
> +static irqreturn_t flctl_handle_flste(int irq, void *dev_id)
> +{
> +	struct sh_flctl *flctl = dev_id;
> +	dev_err(&flctl->pdev->dev, "flste irq: %x\n", readl(FLINTDMACR(flctl)));
> +

You should clear the interrupt flags here, otherwise endless interrupts will 
occur. I suppose this will be fixed in a later patch, but it's a good practice 
to avoid breaking git bisect.

> +	return IRQ_HANDLED;
> +}
> +
>  static int __devinit flctl_probe(struct platform_device *pdev)
>  {
>  	struct resource *res;
> @@ -846,6 +855,7 @@ static int __devinit flctl_probe(struct platform_device
> *pdev) struct nand_chip *nand;
>  	struct sh_flctl_platform_data *pdata;
>  	int ret = -ENXIO;
> +	int irq;
> 
>  	pdata = pdev->dev.platform_data;
>  	if (pdata = NULL) {
> @@ -871,14 +881,27 @@ static int __devinit flctl_probe(struct
> platform_device *pdev) goto err_iomap;
>  	}
> 
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "failed to get flste irq data\n");
> +		goto err_flste;
> +	}
> +
> +	ret = request_irq(irq, flctl_handle_flste, IRQF_SHARED, "flste", flctl);
> +	if (ret) {
> +		dev_err(&pdev->dev, "request interrupt failed.\n");
> +		goto err_flste;
> +	}
> +
>  	platform_set_drvdata(pdev, flctl);
>  	flctl_mtd = &flctl->mtd;
>  	nand = &flctl->chip;
>  	flctl_mtd->priv = nand;
>  	flctl->pdev = pdev;
> -	flctl->flcmncr_base = pdata->flcmncr_val;
>  	flctl->hwecc = pdata->has_hwecc;
>  	flctl->holden = pdata->use_holden;
> +	flctl->flcmncr_base = pdata->flcmncr_val;
> +	flctl->flintdmacr_base = flctl->hwecc ? (STERINTE | ECERB) : STERINTE;
> 
>  	nand->options = NAND_NO_AUTOINCR;
> 
> @@ -919,6 +942,9 @@ static int __devinit flctl_probe(struct platform_device
> *pdev)
> 
>  err_chip:
>  	pm_runtime_disable(&pdev->dev);
> +	free_irq(platform_get_irq(pdev, 0), flctl);
> +err_flste:
> +	iounmap(flctl->reg);

The missing iounmap() is a separate issue, you should split it to its own 
patch. It's also seems to be missing in flctl_remove() btw.

>  err_iomap:
>  	kfree(flctl);
>  	return ret;
> @@ -930,6 +956,7 @@ static int __devexit flctl_remove(struct platform_device
> *pdev)
> 
>  	nand_release(&flctl->mtd);
>  	pm_runtime_disable(&pdev->dev);
> +	free_irq(platform_get_irq(pdev, 0), flctl);
>  	kfree(flctl);
> 
>  	return 0;
> diff --git a/include/linux/mtd/sh_flctl.h b/include/linux/mtd/sh_flctl.h
> index a38e1fa..6832a90 100644
> --- a/include/linux/mtd/sh_flctl.h
> +++ b/include/linux/mtd/sh_flctl.h
> @@ -107,6 +107,12 @@
>  #define DOCMD2_E	(0x1 << 17)	/* 2nd cmd stage execute */
>  #define DOCMD1_E	(0x1 << 16)	/* 1st cmd stage execute */
> 
> +/* FLINTDMACR control bits */
> +#define ESTERINTE	(0x1 << 24)	/* ECC error interrupt enable */
> +#define ECERB		(0x1 << 9)	/* ECC error */
> +#define STERB		(0x1 << 8)	/* Status error */
> +#define STERINTE	(0x1 << 4)	/* Status error enable */
> +
>  /* FLTRCR control bits */
>  #define TRSTRT		(0x1 << 0)	/* translation start */
>  #define TREND		(0x1 << 1)	/* translation end */
> @@ -145,6 +151,7 @@ struct sh_flctl {
>  	uint32_t erase_ADRCNT;		/* bits of FLCMDCR in ERASE1 cmd */
>  	uint32_t rw_ADRCNT;	/* bits of FLCMDCR in READ WRITE cmd */
>  	uint32_t flcmncr_base;	/* base value of FLCMNCR */
> +	uint32_t flintdmacr_base;	/* irq enable bits */
> 
>  	int	hwecc_cant_correct[4];
-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/9] mtd: sh_flctl: Add support for error IRQ

[Bastian Hecht]

Hello Laurent,

2012/4/21 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Bastian,
>
> Thanks for the patch.

Thanks for the reviews. Let me know if you could need my help too.

> On Friday 20 April 2012 11:13:42 Bastian Hecht wrote:
>> When the data transfer between the controller and the NAND chip fails,
>> we now get notified.
>>
>> Signed-off-by: Bastian Hecht <hechtb@gmail.com>
>> ---
>>  drivers/mtd/nand/sh_flctl.c  |   31 +++++++++++++++++++++++++++++--
>>  include/linux/mtd/sh_flctl.h |    7 +++++++
>>  2 files changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
>> index 2ee9a1b..3c27921 100644
>> --- a/drivers/mtd/nand/sh_flctl.c
>> +++ b/drivers/mtd/nand/sh_flctl.c
>> @@ -24,6 +24,7 @@
>>  #include <linux/module.h>
>>  #include <linux/kernel.h>
>>  #include <linux/delay.h>
>> +#include <linux/interrupt.h>
>>  #include <linux/io.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/pm_runtime.h>
>> @@ -69,7 +70,7 @@ static struct nand_bbt_descr flctl_4secc_largepage = {
>>  static void empty_fifo(struct sh_flctl *flctl)
>>  {
>>       writel(0x000c0000, FLINTDMACR(flctl));  /* FIFO Clear */
>
> Shouldn't this write flctl->flintdmacr_base | 0x000c0000 ? If you change this
> line, you could also define and use the AC1CLR and AC0CLR bits.

True, it should be better not to toggle the other settings.

>> -     writel(0x00000000, FLINTDMACR(flctl));  /* Clear Error flags */
>> +     writel(flctl->flintdmacr_base, FLINTDMACR(flctl));
>>  }
>>
>>  static void start_translation(struct sh_flctl *flctl)
>> @@ -838,6 +839,14 @@ static int flctl_chip_init_tail(struct mtd_info *mtd)
>>       return 0;
>>  }
>>
>> +static irqreturn_t flctl_handle_flste(int irq, void *dev_id)
>> +{
>> +     struct sh_flctl *flctl = dev_id;
>> +     dev_err(&flctl->pdev->dev, "flste irq: %x\n", readl(FLINTDMACR(flctl)));
>> +
>
> You should clear the interrupt flags here, otherwise endless interrupts will
> occur. I suppose this will be fixed in a later patch, but it's a good practice
> to avoid breaking git bisect.

Oh yes! Thanks for the hint.

>> +     return IRQ_HANDLED;
>> +}
>> +
>>  static int __devinit flctl_probe(struct platform_device *pdev)
>>  {
>>       struct resource *res;
>> @@ -846,6 +855,7 @@ static int __devinit flctl_probe(struct platform_device
>> *pdev) struct nand_chip *nand;
>>       struct sh_flctl_platform_data *pdata;
>>       int ret = -ENXIO;
>> +     int irq;
>>
>>       pdata = pdev->dev.platform_data;
>>       if (pdata = NULL) {
>> @@ -871,14 +881,27 @@ static int __devinit flctl_probe(struct
>> platform_device *pdev) goto err_iomap;
>>       }
>>
>> +     irq = platform_get_irq(pdev, 0);
>> +     if (irq < 0) {
>> +             dev_err(&pdev->dev, "failed to get flste irq data\n");
>> +             goto err_flste;
>> +     }
>> +
>> +     ret = request_irq(irq, flctl_handle_flste, IRQF_SHARED, "flste", flctl);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "request interrupt failed.\n");
>> +             goto err_flste;
>> +     }
>> +
>>       platform_set_drvdata(pdev, flctl);
>>       flctl_mtd = &flctl->mtd;
>>       nand = &flctl->chip;
>>       flctl_mtd->priv = nand;
>>       flctl->pdev = pdev;
>> -     flctl->flcmncr_base = pdata->flcmncr_val;
>>       flctl->hwecc = pdata->has_hwecc;
>>       flctl->holden = pdata->use_holden;
>> +     flctl->flcmncr_base = pdata->flcmncr_val;
>> +     flctl->flintdmacr_base = flctl->hwecc ? (STERINTE | ECERB) : STERINTE;
>>
>>       nand->options = NAND_NO_AUTOINCR;
>>
>> @@ -919,6 +942,9 @@ static int __devinit flctl_probe(struct platform_device
>> *pdev)
>>
>>  err_chip:
>>       pm_runtime_disable(&pdev->dev);
>> +     free_irq(platform_get_irq(pdev, 0), flctl);
>> +err_flste:
>> +     iounmap(flctl->reg);
>
> The missing iounmap() is a separate issue, you should split it to its own
> patch. It's also seems to be missing in flctl_remove() btw.

Ok I see. I've added an additional patch.

Best regards,

 Bastian Hecht

>>  err_iomap:
>>       kfree(flctl);
>>       return ret;
>> @@ -930,6 +956,7 @@ static int __devexit flctl_remove(struct platform_device
>> *pdev)
>>
>>       nand_release(&flctl->mtd);
>>       pm_runtime_disable(&pdev->dev);
>> +     free_irq(platform_get_irq(pdev, 0), flctl);
>>       kfree(flctl);
>>
>>       return 0;
>> diff --git a/include/linux/mtd/sh_flctl.h b/include/linux/mtd/sh_flctl.h
>> index a38e1fa..6832a90 100644
>> --- a/include/linux/mtd/sh_flctl.h
>> +++ b/include/linux/mtd/sh_flctl.h
>> @@ -107,6 +107,12 @@
>>  #define DOCMD2_E     (0x1 << 17)     /* 2nd cmd stage execute */
>>  #define DOCMD1_E     (0x1 << 16)     /* 1st cmd stage execute */
>>
>> +/* FLINTDMACR control bits */
>> +#define ESTERINTE    (0x1 << 24)     /* ECC error interrupt enable */
>> +#define ECERB                (0x1 << 9)      /* ECC error */
>> +#define STERB                (0x1 << 8)      /* Status error */
>> +#define STERINTE     (0x1 << 4)      /* Status error enable */
>> +
>>  /* FLTRCR control bits */
>>  #define TRSTRT               (0x1 << 0)      /* translation start */
>>  #define TREND                (0x1 << 1)      /* translation end */
>> @@ -145,6 +151,7 @@ struct sh_flctl {
>>       uint32_t erase_ADRCNT;          /* bits of FLCMDCR in ERASE1 cmd */
>>       uint32_t rw_ADRCNT;     /* bits of FLCMDCR in READ WRITE cmd */
>>       uint32_t flcmncr_base;  /* base value of FLCMNCR */
>> +     uint32_t flintdmacr_base;       /* irq enable bits */
>>
>>       int     hwecc_cant_correct[4];
> --
> Regards,
>
> Laurent Pinchart
>

[PATCH 2/9] ARM: sh-mobile: mackerel: Add error IRQ resource

[Bastian Hecht]

Supply the platform data for the FLSTEI error IRQ.

Signed-off-by: Bastian Hecht <hechtb@gmail.com>
---
 arch/arm/mach-shmobile/board-mackerel.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-shmobile/board-mackerel.c b/arch/arm/mach-shmobile/board-mackerel.c
index 8758f94..1de1fb7 100644
--- a/arch/arm/mach-shmobile/board-mackerel.c
+++ b/arch/arm/mach-shmobile/board-mackerel.c
@@ -990,7 +990,11 @@ static struct resource nand_flash_resources[] = {
 		.start	= 0xe6a30000,
 		.end	= 0xe6a3009b,
 		.flags	= IORESOURCE_MEM,
-	}
+	},
+	[1] = {
+		.start	= evt2irq(0x0d80), /* flstei: status error irq */
+		.flags	= IORESOURCE_IRQ,
+	},
 };
 
 static struct sh_flctl_platform_data nand_flash_data = {
-- 
1.7.5.4

Re: [PATCH 2/9] ARM: sh-mobile: mackerel: Add error IRQ resource

[Laurent Pinchart]

Hi Bastian,

Thanks for the patch.

On Friday 20 April 2012 11:13:43 Bastian Hecht wrote:
> Supply the platform data for the FLSTEI error IRQ.

This should come before your 1/9 patch, otherwise the FLCTL driver will fail 
to get the IRQ, breaking git bisect.

> Signed-off-by: Bastian Hecht <hechtb@gmail.com>
> ---
>  arch/arm/mach-shmobile/board-mackerel.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-shmobile/board-mackerel.c
> b/arch/arm/mach-shmobile/board-mackerel.c index 8758f94..1de1fb7 100644
> --- a/arch/arm/mach-shmobile/board-mackerel.c
> +++ b/arch/arm/mach-shmobile/board-mackerel.c
> @@ -990,7 +990,11 @@ static struct resource nand_flash_resources[] = {
>  		.start	= 0xe6a30000,
>  		.end	= 0xe6a3009b,
>  		.flags	= IORESOURCE_MEM,
> -	}
> +	},
> +	[1] = {
> +		.start	= evt2irq(0x0d80), /* flstei: status error irq */
> +		.flags	= IORESOURCE_IRQ,
> +	},
>  };
> 
>  static struct sh_flctl_platform_data nand_flash_data = {
-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/9] ARM: sh-mobile: mackerel: Add error IRQ resource

[Bastian Hecht]

Hello Laurent,

2012/4/21 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Bastian,
>
> Thanks for the patch.
>
> On Friday 20 April 2012 11:13:43 Bastian Hecht wrote:
>> Supply the platform data for the FLSTEI error IRQ.
>
> This should come before your 1/9 patch, otherwise the FLCTL driver will fail
> to get the IRQ, breaking git bisect.

"git bisect" - I should try this :D

Thanks, I'll swap it.

Best regards,

 Bastian Hecht

>> Signed-off-by: Bastian Hecht <hechtb@gmail.com>
>> ---
>>  arch/arm/mach-shmobile/board-mackerel.c |    6 +++++-
>>  1 files changed, 5 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/mach-shmobile/board-mackerel.c
>> b/arch/arm/mach-shmobile/board-mackerel.c index 8758f94..1de1fb7 100644
>> --- a/arch/arm/mach-shmobile/board-mackerel.c
>> +++ b/arch/arm/mach-shmobile/board-mackerel.c
>> @@ -990,7 +990,11 @@ static struct resource nand_flash_resources[] = {
>>               .start  = 0xe6a30000,
>>               .end    = 0xe6a3009b,
>>               .flags  = IORESOURCE_MEM,
>> -     }
>> +     },
>> +     [1] = {
>> +             .start  = evt2irq(0x0d80), /* flstei: status error irq */
>> +             .flags  = IORESOURCE_IRQ,
>> +     },
>>  };
>>
>>  static struct sh_flctl_platform_data nand_flash_data = {
> --
> Regards,
>
> Laurent Pinchart
>

[PATCH 3/9] mtd: sh_flctl: Use different OOB layout

[Bastian Hecht]

The flctl hardware has changed and a new OOB layout must be adapted for
2k page size NAND chips when using hardware ECC.
The related bit fields ECCPOS[0-2] are gone - the bits are marked as
reserved now in the datasheet. As there are no official users of the
hardware ECC so far, they are completely removed.

Signed-off-by: Bastian Hecht <hechtb@gmail.com>
---
 drivers/mtd/nand/sh_flctl.c  |   20 +++++++++++++-------
 include/linux/mtd/sh_flctl.h |    4 ----
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
index 3c27921..411d783 100644
--- a/drivers/mtd/nand/sh_flctl.c
+++ b/drivers/mtd/nand/sh_flctl.c
@@ -44,11 +44,17 @@ static struct nand_ecclayout flctl_4secc_oob_16 = {
 };
 
 static struct nand_ecclayout flctl_4secc_oob_64 = {
-	.eccbytes = 10,
-	.eccpos = {48, 49, 50, 51, 52, 53, 54, 55, 56, 57},
-	.oobfree = {
-		{.offset = 60,
-		. length = 4} },
+	.eccbytes = 4 * 10,
+		.eccpos = {
+			 6,  7, 8,  9, 10, 11, 12, 13, 14, 15,
+			22, 23, 24, 25, 26, 27, 28, 29, 30, 31,
+			38, 39, 40, 41, 42, 43, 44, 45, 46, 47,
+			54, 55, 56, 57, 58, 59, 60, 61, 62, 63 },
+		.oobfree = {
+			{.offset = 2, .length = 4},
+			{.offset = 16, .length = 6},
+			{.offset = 32, .length = 6},
+			{.offset = 48, .length = 6} },
 };
 
 static uint8_t scan_ff_pattern[] = { 0xff, 0xff };
@@ -62,7 +68,7 @@ static struct nand_bbt_descr flctl_4secc_smallpage = {
 
 static struct nand_bbt_descr flctl_4secc_largepage = {
 	.options = NAND_BBT_SCAN2NDPAGE,
-	.offs = 58,
+	.offs = 0,
 	.len = 2,
 	.pattern = scan_ff_pattern,
 };
@@ -831,7 +837,7 @@ static int flctl_chip_init_tail(struct mtd_info *mtd)
 		chip->ecc.mode = NAND_ECC_HW;
 
 		/* 4 symbols ECC enabled */
-		flctl->flcmncr_base |= _4ECCEN | ECCPOS2 | ECCPOS_02;
+		flctl->flcmncr_base |= _4ECCEN;
 	} else {
 		chip->ecc.mode = NAND_ECC_SOFT;
 	}
diff --git a/include/linux/mtd/sh_flctl.h b/include/linux/mtd/sh_flctl.h
index 6832a90..91c5b01 100644
--- a/include/linux/mtd/sh_flctl.h
+++ b/include/linux/mtd/sh_flctl.h
@@ -49,7 +49,6 @@
 #define	FLERRADR(f)		(f->reg + 0x98)
 
 /* FLCMNCR control bits */
-#define ECCPOS2		(0x1 << 25)
 #define _4ECCCNTEN	(0x1 << 24)
 #define _4ECCEN		(0x1 << 23)
 #define _4ECCCORRECT	(0x1 << 22)
@@ -59,9 +58,6 @@
 #define QTSEL_E		(0x1 << 17)
 #define ENDIAN		(0x1 << 16)	/* 1 = little endian */
 #define FCKSEL_E	(0x1 << 15)
-#define ECCPOS_00	(0x00 << 12)
-#define ECCPOS_01	(0x01 << 12)
-#define ECCPOS_02	(0x02 << 12)
 #define ACM_SACCES_MODE	(0x01 << 10)
 #define NANWF_E		(0x1 << 9)
 #define SE_D		(0x1 << 8)	/* Spare area disable */
-- 
1.7.5.4

Re: [PATCH 3/9] mtd: sh_flctl: Use different OOB layout

[Laurent Pinchart]

Hi Bastian,

Thanks for the patch.

On Friday 20 April 2012 11:13:44 Bastian Hecht wrote:
> The flctl hardware has changed and a new OOB layout must be adapted for
> 2k page size NAND chips when using hardware ECC.
> The related bit fields ECCPOS[0-2] are gone - the bits are marked as
> reserved now in the datasheet. As there are no official users of the
> hardware ECC so far, they are completely removed.
> 
> Signed-off-by: Bastian Hecht <hechtb@gmail.com>
> ---
>  drivers/mtd/nand/sh_flctl.c  |   20 +++++++++++++-------
>  include/linux/mtd/sh_flctl.h |    4 ----
>  2 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
> index 3c27921..411d783 100644
> --- a/drivers/mtd/nand/sh_flctl.c
> +++ b/drivers/mtd/nand/sh_flctl.c
> @@ -44,11 +44,17 @@ static struct nand_ecclayout flctl_4secc_oob_16 = {
>  };
> 
>  static struct nand_ecclayout flctl_4secc_oob_64 = {
> -	.eccbytes = 10,
> -	.eccpos = {48, 49, 50, 51, 52, 53, 54, 55, 56, 57},
> -	.oobfree = {
> -		{.offset = 60,
> -		. length = 4} },
> +	.eccbytes = 4 * 10,
> +		.eccpos = {
> +			 6,  7, 8,  9, 10, 11, 12, 13, 14, 15,
> +			22, 23, 24, 25, 26, 27, 28, 29, 30, 31,
> +			38, 39, 40, 41, 42, 43, 44, 45, 46, 47,
> +			54, 55, 56, 57, 58, 59, 60, 61, 62, 63 },

Indentation mistake ?

> +		.oobfree = {
> +			{.offset = 2, .length = 4},
> +			{.offset = 16, .length = 6},
> +			{.offset = 32, .length = 6},
> +			{.offset = 48, .length = 6} },

Just for my information, where does that information come from ?

>  };
> 
>  static uint8_t scan_ff_pattern[] = { 0xff, 0xff };
> @@ -62,7 +68,7 @@ static struct nand_bbt_descr flctl_4secc_smallpage = {
> 
>  static struct nand_bbt_descr flctl_4secc_largepage = {
>  	.options = NAND_BBT_SCAN2NDPAGE,
> -	.offs = 58,
> +	.offs = 0,
>  	.len = 2,
>  	.pattern = scan_ff_pattern,
>  };
> @@ -831,7 +837,7 @@ static int flctl_chip_init_tail(struct mtd_info *mtd)
>  		chip->ecc.mode = NAND_ECC_HW;
> 
>  		/* 4 symbols ECC enabled */
> -		flctl->flcmncr_base |= _4ECCEN | ECCPOS2 | ECCPOS_02;
> +		flctl->flcmncr_base |= _4ECCEN;
>  	} else {
>  		chip->ecc.mode = NAND_ECC_SOFT;
>  	}
> diff --git a/include/linux/mtd/sh_flctl.h b/include/linux/mtd/sh_flctl.h
> index 6832a90..91c5b01 100644
> --- a/include/linux/mtd/sh_flctl.h
> +++ b/include/linux/mtd/sh_flctl.h
> @@ -49,7 +49,6 @@
>  #define	FLERRADR(f)		(f->reg + 0x98)
> 
>  /* FLCMNCR control bits */
> -#define ECCPOS2		(0x1 << 25)
>  #define _4ECCCNTEN	(0x1 << 24)
>  #define _4ECCEN		(0x1 << 23)
>  #define _4ECCCORRECT	(0x1 << 22)
> @@ -59,9 +58,6 @@
>  #define QTSEL_E		(0x1 << 17)
>  #define ENDIAN		(0x1 << 16)	/* 1 = little endian */
>  #define FCKSEL_E	(0x1 << 15)
> -#define ECCPOS_00	(0x00 << 12)
> -#define ECCPOS_01	(0x01 << 12)
> -#define ECCPOS_02	(0x02 << 12)
>  #define ACM_SACCES_MODE	(0x01 << 10)
>  #define NANWF_E		(0x1 << 9)
>  #define SE_D		(0x1 << 8)	/* Spare area disable */
-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/9] mtd: sh_flctl: Use different OOB layout

[Bastian Hecht]

Hello Laurent,

2012/4/21 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Bastian,
>
> Thanks for the patch.
>
> On Friday 20 April 2012 11:13:44 Bastian Hecht wrote:
>> The flctl hardware has changed and a new OOB layout must be adapted for
>> 2k page size NAND chips when using hardware ECC.
>> The related bit fields ECCPOS[0-2] are gone - the bits are marked as
>> reserved now in the datasheet. As there are no official users of the
>> hardware ECC so far, they are completely removed.
>>
>> Signed-off-by: Bastian Hecht <hechtb@gmail.com>
>> ---
>>  drivers/mtd/nand/sh_flctl.c  |   20 +++++++++++++-------
>>  include/linux/mtd/sh_flctl.h |    4 ----
>>  2 files changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
>> index 3c27921..411d783 100644
>> --- a/drivers/mtd/nand/sh_flctl.c
>> +++ b/drivers/mtd/nand/sh_flctl.c
>> @@ -44,11 +44,17 @@ static struct nand_ecclayout flctl_4secc_oob_16 = {
>>  };
>>
>>  static struct nand_ecclayout flctl_4secc_oob_64 = {
>> -     .eccbytes = 10,
>> -     .eccpos = {48, 49, 50, 51, 52, 53, 54, 55, 56, 57},
>> -     .oobfree = {
>> -             {.offset = 60,
>> -             . length = 4} },
>> +     .eccbytes = 4 * 10,
>> +             .eccpos = {
>> +                      6,  7, 8,  9, 10, 11, 12, 13, 14, 15,
>> +                     22, 23, 24, 25, 26, 27, 28, 29, 30, 31,
>> +                     38, 39, 40, 41, 42, 43, 44, 45, 46, 47,
>> +                     54, 55, 56, 57, 58, 59, 60, 61, 62, 63 },
>
> Indentation mistake ?

Yes, thanks for noticing.

>> +             .oobfree = {
>> +                     {.offset = 2, .length = 4},
>> +                     {.offset = 16, .length = 6},
>> +                     {.offset = 32, .length = 6},
>> +                     {.offset = 48, .length = 6} },
>
> Just for my information, where does that information come from ?

The first 2 bytes are used for bad block marking at the 1st page of
every erase block. This seems to be the default for all NAND chips
when they are marked by the manufacturer. The last 10 bytes are used
for ECC in each 16-bytes ECC hunk - I don't remember if I got this
info from the datasheet or by inspecting the page writes. BTW a page
is layed out this way: 512 bytes data / 16 bytes oob / 512 bytes data
/ 16 bytes oob / 512 bytes data / 16 bytes oob / 512 bytes data / 16
bytes oob. This leads to the code above.

Best regards,

 Bastian Hecht


>>  };
>>
>>  static uint8_t scan_ff_pattern[] = { 0xff, 0xff };
>> @@ -62,7 +68,7 @@ static struct nand_bbt_descr flctl_4secc_smallpage = {
>>
>>  static struct nand_bbt_descr flctl_4secc_largepage = {
>>       .options = NAND_BBT_SCAN2NDPAGE,
>> -     .offs = 58,
>> +     .offs = 0,
>>       .len = 2,
>>       .pattern = scan_ff_pattern,
>>  };
>> @@ -831,7 +837,7 @@ static int flctl_chip_init_tail(struct mtd_info *mtd)
>>               chip->ecc.mode = NAND_ECC_HW;
>>
>>               /* 4 symbols ECC enabled */
>> -             flctl->flcmncr_base |= _4ECCEN | ECCPOS2 | ECCPOS_02;
>> +             flctl->flcmncr_base |= _4ECCEN;
>>       } else {
>>               chip->ecc.mode = NAND_ECC_SOFT;
>>       }
>> diff --git a/include/linux/mtd/sh_flctl.h b/include/linux/mtd/sh_flctl.h
>> index 6832a90..91c5b01 100644
>> --- a/include/linux/mtd/sh_flctl.h
>> +++ b/include/linux/mtd/sh_flctl.h
>> @@ -49,7 +49,6 @@
>>  #define      FLERRADR(f)             (f->reg + 0x98)
>>
>>  /* FLCMNCR control bits */
>> -#define ECCPOS2              (0x1 << 25)
>>  #define _4ECCCNTEN   (0x1 << 24)
>>  #define _4ECCEN              (0x1 << 23)
>>  #define _4ECCCORRECT (0x1 << 22)
>> @@ -59,9 +58,6 @@
>>  #define QTSEL_E              (0x1 << 17)
>>  #define ENDIAN               (0x1 << 16)     /* 1 = little endian */
>>  #define FCKSEL_E     (0x1 << 15)
>> -#define ECCPOS_00    (0x00 << 12)
>> -#define ECCPOS_01    (0x01 << 12)
>> -#define ECCPOS_02    (0x02 << 12)
>>  #define ACM_SACCES_MODE      (0x01 << 10)
>>  #define NANWF_E              (0x1 << 9)
>>  #define SE_D         (0x1 << 8)      /* Spare area disable */
> --
> Regards,
>
> Laurent Pinchart
>

[PATCH 4/9] mtd: sh_flctl: Fix hardware ECC behaviour

[Bastian Hecht]

The flctl uses 10 bytes ECC data for every 512 bytes sector. This patch
makes the controller write all 40 bytes instead of 10 bytes only.

Signed-off-by: Bastian Hecht <hechtb@gmail.com>
---
 drivers/mtd/nand/sh_flctl.c |   30 +++++++-----------------------
 1 files changed, 7 insertions(+), 23 deletions(-)

diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
index 411d783..c16479c 100644
--- a/drivers/mtd/nand/sh_flctl.c
+++ b/drivers/mtd/nand/sh_flctl.c
@@ -427,30 +427,20 @@ static void execmd_read_page_sector(struct mtd_info *mtd, int page_addr)
 static void execmd_read_oob(struct mtd_info *mtd, int page_addr)
 {
 	struct sh_flctl *flctl = mtd_to_flctl(mtd);
+	int page_sectors = flctl->page_size ? 4 : 1;
+	int i;
 
 	set_cmd_regs(mtd, NAND_CMD_READ0,
 		(NAND_CMD_READSTART << 8) | NAND_CMD_READ0);
 
 	empty_fifo(flctl);
-	if (flctl->page_size) {
-		int i;
-		/* In case that the page size is 2k */
-		for (i = 0; i < 16 * 3; i++)
-			flctl->done_buff[i] = 0xFF;
-
-		set_addr(mtd, 3 * 528 + 512, page_addr);
-		writel(16, FLDTCNTR(flctl));
 
-		start_translation(flctl);
-		read_fiforeg(flctl, 16, 16 * 3);
-		wait_completion(flctl);
-	} else {
-		/* In case that the page size is 512b */
-		set_addr(mtd, 512, page_addr);
+	for (i = 0; i < page_sectors; i++) {
+		set_addr(mtd, (512 + 16) * i + 512 , page_addr);
 		writel(16, FLDTCNTR(flctl));
 
 		start_translation(flctl);
-		read_fiforeg(flctl, 16, 0);
+		read_fiforeg(flctl, 16, 16 * i);
 		wait_completion(flctl);
 	}
 }
@@ -495,18 +485,12 @@ static void execmd_write_oob(struct mtd_info *mtd)
 	int page_addr = flctl->seqin_page_addr;
 	int sector, page_sectors;
 
-	if (flctl->page_size) {
-		sector = 3;
-		page_sectors = 4;
-	} else {
-		sector = 0;
-		page_sectors = 1;
-	}
+	page_sectors = flctl->page_size ? 4 : 1;
 
 	set_cmd_regs(mtd, NAND_CMD_PAGEPROG,
 			(NAND_CMD_PAGEPROG << 8) | NAND_CMD_SEQIN);
 
-	for (; sector < page_sectors; sector++) {
+	for (sector = 0; sector < page_sectors; sector++) {
 		empty_fifo(flctl);
 		set_addr(mtd, sector * 528 + 512, page_addr);
 		writel(16, FLDTCNTR(flctl));	/* set read size */
-- 
1.7.5.4

[PATCH 5/9] mtd: sh_flctl: Simplify the hardware ecc page read/write

[Bastian Hecht]

As the equation mtd->writesize = eccsteps * eccsize holds, we can
simplify the code. The second loop of the 1st hunk is never entered,
so we delete it.

Signed-off-by: Bastian Hecht <hechtb@gmail.com>
---
 drivers/mtd/nand/sh_flctl.c |   25 ++-----------------------
 1 files changed, 2 insertions(+), 23 deletions(-)

diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
index c16479c..8e56f87 100644
--- a/drivers/mtd/nand/sh_flctl.c
+++ b/drivers/mtd/nand/sh_flctl.c
@@ -353,35 +353,14 @@ static void set_cmd_regs(struct mtd_info *mtd, uint32_t cmd, uint32_t flcmcdr_va
 static int flctl_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip,
 				uint8_t *buf, int page)
 {
-	int i, eccsize = chip->ecc.size;
-	int eccbytes = chip->ecc.bytes;
-	int eccsteps = chip->ecc.steps;
-	uint8_t *p = buf;
-	struct sh_flctl *flctl = mtd_to_flctl(mtd);
-
-	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize)
-		chip->read_buf(mtd, p, eccsize);
-
-	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
-		if (flctl->hwecc_cant_correct[i])
-			mtd->ecc_stats.failed++;
-		else
-			mtd->ecc_stats.corrected += 0;
-	}
-
+	chip->read_buf(mtd, buf, mtd->writesize);
 	return 0;
 }
 
 static void flctl_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip,
 				   const uint8_t *buf)
 {
-	int i, eccsize = chip->ecc.size;
-	int eccbytes = chip->ecc.bytes;
-	int eccsteps = chip->ecc.steps;
-	const uint8_t *p = buf;
-
-	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize)
-		chip->write_buf(mtd, p, eccsize);
+	chip->write_buf(mtd, buf, mtd->writesize);
 }
 
 static void execmd_read_page_sector(struct mtd_info *mtd, int page_addr)
-- 
1.7.5.4

[PATCH 6/9] mtd: sh_flctl: Group sector accesses into a single transfer

[Bastian Hecht]

When we use hardware ecc, the flctl is run in so-called "sector access
mode". We can bundle 4 sector accesses when using 2k page sizes to read
a whole page at once and speed up things.

Signed-off-by: Bastian Hecht <hechtb@gmail.com>
---
 drivers/mtd/nand/sh_flctl.c |   44 ++++++++++++++++++------------------------
 1 files changed, 19 insertions(+), 25 deletions(-)

diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
index 8e56f87..1e5d684 100644
--- a/drivers/mtd/nand/sh_flctl.c
+++ b/drivers/mtd/nand/sh_flctl.c
@@ -368,25 +368,21 @@ static void execmd_read_page_sector(struct mtd_info *mtd, int page_addr)
 	struct sh_flctl *flctl = mtd_to_flctl(mtd);
 	int sector, page_sectors;
 
-	if (flctl->page_size)
-		page_sectors = 4;
-	else
-		page_sectors = 1;
+	page_sectors = flctl->page_size ? 4 : 1;
+
+	set_cmd_regs(mtd, NAND_CMD_READ0,
+		(NAND_CMD_READSTART << 8) | NAND_CMD_READ0);
 
 	writel(readl(FLCMNCR(flctl)) | ACM_SACCES_MODE | _4ECCCORRECT,
 		 FLCMNCR(flctl));
+	writel(readl(FLCMDCR(flctl)) | page_sectors, FLCMDCR(flctl));
+	writel(page_addr << 2, FLADR(flctl));
 
-	set_cmd_regs(mtd, NAND_CMD_READ0,
-		(NAND_CMD_READSTART << 8) | NAND_CMD_READ0);
+	empty_fifo(flctl);
+	start_translation(flctl);
 
 	for (sector = 0; sector < page_sectors; sector++) {
 		int ret;
-
-		empty_fifo(flctl);
-		writel(readl(FLCMDCR(flctl)) | 1, FLCMDCR(flctl));
-		writel(page_addr << 2 | sector, FLADR(flctl));
-
-		start_translation(flctl);
 		read_fiforeg(flctl, 512, 512 * sector);
 
 		ret = read_ecfiforeg(flctl,
@@ -397,8 +393,10 @@ static void execmd_read_page_sector(struct mtd_info *mtd, int page_addr)
 			flctl->hwecc_cant_correct[sector] = 1;
 
 		writel(0x0, FL4ECCCR(flctl));
-		wait_completion(flctl);
 	}
+
+	wait_completion(flctl);
+
 	writel(readl(FLCMNCR(flctl)) & ~(ACM_SACCES_MODE | _4ECCCORRECT),
 			FLCMNCR(flctl));
 }
@@ -430,31 +428,27 @@ static void execmd_write_page_sector(struct mtd_info *mtd)
 	int i, page_addr = flctl->seqin_page_addr;
 	int sector, page_sectors;
 
-	if (flctl->page_size)
-		page_sectors = 4;
-	else
-		page_sectors = 1;
-
-	writel(readl(FLCMNCR(flctl)) | ACM_SACCES_MODE, FLCMNCR(flctl));
+	page_sectors = flctl->page_size ? 4 : 1;
 
 	set_cmd_regs(mtd, NAND_CMD_PAGEPROG,
 			(NAND_CMD_PAGEPROG << 8) | NAND_CMD_SEQIN);
 
-	for (sector = 0; sector < page_sectors; sector++) {
-		empty_fifo(flctl);
-		writel(readl(FLCMDCR(flctl)) | 1, FLCMDCR(flctl));
-		writel(page_addr << 2 | sector, FLADR(flctl));
+	empty_fifo(flctl);
+	writel(readl(FLCMNCR(flctl)) | ACM_SACCES_MODE, FLCMNCR(flctl));
+	writel(readl(FLCMDCR(flctl)) | page_sectors, FLCMDCR(flctl));
+	writel(page_addr << 2, FLADR(flctl));
+	start_translation(flctl);
 
-		start_translation(flctl);
+	for (sector = 0; sector < page_sectors; sector++) {
 		write_fiforeg(flctl, 512, 512 * sector);
 
 		for (i = 0; i < 4; i++) {
 			wait_wecfifo_ready(flctl); /* wait for write ready */
 			writel(0xFFFFFFFF, FLECFIFO(flctl));
 		}
-		wait_completion(flctl);
 	}
 
+	wait_completion(flctl);
 	writel(readl(FLCMNCR(flctl)) & ~ACM_SACCES_MODE, FLCMNCR(flctl));
 }
 
-- 
1.7.5.4

[PATCH 7/9] mtd: sh_flctl: Restructure the hardware ECC handling

[Bastian Hecht]

There are multiple reasons for a rewrite:
 - a race exists: when _4ECCEND is set, _4ECCFA may become true too
   meanwhile, which is lost and a non-correctable error is treated as
   correctable.
 - the ECC statistics don't get properly propagated to the base code.
 - empty pages would get marked as corrupted

The rewrite resolves the issues and I hope it gives a more explicit
code flow structure.

Signed-off-by: Bastian Hecht <hechtb@gmail.com>
---
 drivers/mtd/nand/sh_flctl.c  |  120 ++++++++++++++++++++++++++++--------------
 include/linux/mtd/sh_flctl.h |   10 +++-
 2 files changed, 88 insertions(+), 42 deletions(-)

diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
index 1e5d684..408e013 100644
--- a/drivers/mtd/nand/sh_flctl.c
+++ b/drivers/mtd/nand/sh_flctl.c
@@ -165,27 +165,56 @@ static void wait_wfifo_ready(struct sh_flctl *flctl)
 	timeout_error(flctl, __func__);
 }
 
-static int wait_recfifo_ready(struct sh_flctl *flctl, int sector_number)
+static enum flctl_ecc_res_t wait_recfifo_ready
+		(struct sh_flctl *flctl, int sector_number)
 {
 	uint32_t timeout = LOOP_TIMEOUT_MAX;
-	int checked[4];
 	void __iomem *ecc_reg[4];
 	int i;
+	int state = FL_SUCCESS;
 	uint32_t data, size;
 
-	memset(checked, 0, sizeof(checked));
-
+	/*
+	 * First this loops checks in FLDTCNTR if we are ready to read out the
+	 * oob data. This is the case if either all went fine without errors or
+	 * if the bottom part of the loop corrected the errors or marked them as
+	 * uncorrectable and the controller is given time to push the data into
+	 * the FIFO.
+	 */
 	while (timeout--) {
+		/* check if all is ok and we can read out the OOB */
 		size = readl(FLDTCNTR(flctl)) >> 24;
-		if (size & 0xFF)
-			return 0;	/* success */
+		if ((size & 0xFF) = 4)
+			return state;
+
+		/* check if a correction code has been calculated */
+		if (!(readl(FL4ECCCR(flctl)) & _4ECCEND)) {
+			/*
+			 * either we wait for the fifo to be filled or a
+			 * correction pattern is being generated
+			 */
+			udelay(1);
+			continue;
+		}
 
-		if (readl(FL4ECCCR(flctl)) & _4ECCFA)
-			return 1;	/* can't correct */
+		/* check for an uncorrectable error */
+		if (readl(FL4ECCCR(flctl)) & _4ECCFA) {
+			/* check if we face a non-empty page */
+			for (i = 0; i < 512; i++) {
+				if (flctl->done_buff[i] != 0xff) {
+					state = FL_ERROR; /* can't correct */
+					break;
+				}
+			}
 
-		udelay(1);
-		if (!(readl(FL4ECCCR(flctl)) & _4ECCEND))
+			if (state = FL_SUCCESS)
+				dev_dbg(&flctl->pdev->dev,
+				"reading empty sector %d, ecc error ignored\n",
+				sector_number);
+
+			writel(0, FL4ECCCR(flctl));
 			continue;
+		}
 
 		/* start error correction */
 		ecc_reg[0] = FL4ECCRESULT0(flctl);
@@ -194,28 +223,26 @@ static int wait_recfifo_ready(struct sh_flctl *flctl, int sector_number)
 		ecc_reg[3] = FL4ECCRESULT3(flctl);
 
 		for (i = 0; i < 3; i++) {
+			uint8_t org;
+			int index;
+
 			data = readl(ecc_reg[i]);
-			if (data != INIT_FL4ECCRESULT_VAL && !checked[i]) {
-				uint8_t org;
-				int index;
-
-				if (flctl->page_size)
-					index = (512 * sector_number) +
-						(data >> 16);
-				else
-					index = data >> 16;
-
-				org = flctl->done_buff[index];
-				flctl->done_buff[index] = org ^ (data & 0xFF);
-				checked[i] = 1;
-			}
-		}
 
+			if (flctl->page_size)
+				index = (512 * sector_number) +
+					(data >> 16);
+			else
+				index = data >> 16;
+
+			org = flctl->done_buff[index];
+			flctl->done_buff[index] = org ^ (data & 0xFF);
+		}
+		state = FL_REPAIRABLE;
 		writel(0, FL4ECCCR(flctl));
 	}
 
 	timeout_error(flctl, __func__);
-	return 1;	/* timeout */
+	return FL_TIMEOUT;	/* timeout */
 }
 
 static void wait_wecfifo_ready(struct sh_flctl *flctl)
@@ -259,20 +286,24 @@ static void read_fiforeg(struct sh_flctl *flctl, int rlen, int offset)
 	}
 }
 
-static int read_ecfiforeg(struct sh_flctl *flctl, uint8_t *buff, int sector)
+static enum flctl_ecc_res_t read_ecfiforeg
+		(struct sh_flctl *flctl, uint8_t *buff, int sector)
 {
 	int i;
+	enum flctl_ecc_res_t res;
 	unsigned long *ecc_buf = (unsigned long *)buff;
 	void *fifo_addr = (void *)FLECFIFO(flctl);
 
-	for (i = 0; i < 4; i++) {
-		if (wait_recfifo_ready(flctl , sector))
-			return 1;
-		ecc_buf[i] = readl(fifo_addr);
-		ecc_buf[i] = be32_to_cpu(ecc_buf[i]);
+	res = wait_recfifo_ready(flctl , sector);
+
+	if (res != FL_ERROR) {
+		for (i = 0; i < 4; i++) {
+			ecc_buf[i] = readl(fifo_addr);
+			ecc_buf[i] = be32_to_cpu(ecc_buf[i]);
+		}
 	}
 
-	return 0;
+	return res;
 }
 
 static void write_fiforeg(struct sh_flctl *flctl, int rlen, int offset)
@@ -367,6 +398,7 @@ static void execmd_read_page_sector(struct mtd_info *mtd, int page_addr)
 {
 	struct sh_flctl *flctl = mtd_to_flctl(mtd);
 	int sector, page_sectors;
+	enum flctl_ecc_res_t ecc_result;
 
 	page_sectors = flctl->page_size ? 4 : 1;
 
@@ -382,17 +414,27 @@ static void execmd_read_page_sector(struct mtd_info *mtd, int page_addr)
 	start_translation(flctl);
 
 	for (sector = 0; sector < page_sectors; sector++) {
-		int ret;
 		read_fiforeg(flctl, 512, 512 * sector);
 
-		ret = read_ecfiforeg(flctl,
+		ecc_result = read_ecfiforeg(flctl,
 			&flctl->done_buff[mtd->writesize + 16 * sector],
 			sector);
 
-		if (ret)
-			flctl->hwecc_cant_correct[sector] = 1;
-
-		writel(0x0, FL4ECCCR(flctl));
+		switch (ecc_result) {
+		case FL_REPAIRABLE:
+			dev_info(&flctl->pdev->dev,
+				"applied ecc on page 0x%x", page_addr);
+			flctl->mtd.ecc_stats.corrected++;
+			break;
+		case FL_ERROR:
+			dev_warn(&flctl->pdev->dev,
+				"page 0x%x contains corrupted data\n",
+				page_addr);
+			flctl->mtd.ecc_stats.failed++;
+			break;
+		default:
+			;
+		}
 	}
 
 	wait_completion(flctl);
diff --git a/include/linux/mtd/sh_flctl.h b/include/linux/mtd/sh_flctl.h
index 91c5b01..a9e24d7 100644
--- a/include/linux/mtd/sh_flctl.h
+++ b/include/linux/mtd/sh_flctl.h
@@ -127,9 +127,15 @@
 #define	_4ECCEND	(0x1 << 1)	/* 4 symbols end */
 #define	_4ECCEXST	(0x1 << 0)	/* 4 symbols exist */
 
-#define INIT_FL4ECCRESULT_VAL	0x03FF03FF
 #define LOOP_TIMEOUT_MAX	0x00010000
 
+enum flctl_ecc_res_t {
+	FL_SUCCESS,
+	FL_REPAIRABLE,
+	FL_ERROR,
+	FL_TIMEOUT
+};
+
 struct sh_flctl {
 	struct mtd_info		mtd;
 	struct nand_chip	chip;
@@ -149,8 +155,6 @@ struct sh_flctl {
 	uint32_t flcmncr_base;	/* base value of FLCMNCR */
 	uint32_t flintdmacr_base;	/* irq enable bits */
 
-	int	hwecc_cant_correct[4];
-
 	unsigned page_size:1;	/* NAND page size (0 = 512, 1 = 2048) */
 	unsigned hwecc:1;	/* Hardware ECC (0 = disabled, 1 = enabled) */
 	unsigned holden:1;	/* Hardware has FLHOLDCR and HOLDEN is set */
-- 
1.7.5.4

[PATCH 8/9] mtd: sh_flctl: Use user oob data in hardware ECC mode

[Bastian Hecht]

In hardware ecc mode, the flctl now writes and reads the oob data
provided by the user. Additionally the ECC is now returned in normal
page reads, not only when using the explicit NAND_CMD_READOOB command.

Signed-off-by: Bastian Hecht <hechtb@gmail.com>
---
 drivers/mtd/nand/sh_flctl.c |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
index 408e013..2766ce4 100644
--- a/drivers/mtd/nand/sh_flctl.c
+++ b/drivers/mtd/nand/sh_flctl.c
@@ -319,6 +319,19 @@ static void write_fiforeg(struct sh_flctl *flctl, int rlen, int offset)
 	}
 }
 
+static void write_ec_fiforeg(struct sh_flctl *flctl, int rlen, int offset)
+{
+	int i, len_4align;
+	unsigned long *data = (unsigned long *)&flctl->done_buff[offset];
+	void *fifo_addr = (void *)FLECFIFO(flctl);
+
+	len_4align = (rlen + 3) / 4;
+	for (i = 0; i < len_4align; i++) {
+		wait_wecfifo_ready(flctl);
+		writel(cpu_to_be32(data[i]), fifo_addr);
+	}
+}
+
 static void set_cmd_regs(struct mtd_info *mtd, uint32_t cmd, uint32_t flcmcdr_val)
 {
 	struct sh_flctl *flctl = mtd_to_flctl(mtd);
@@ -385,6 +398,7 @@ static int flctl_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip,
 				uint8_t *buf, int page)
 {
 	chip->read_buf(mtd, buf, mtd->writesize);
+	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
 	return 0;
 }
 
@@ -392,6 +406,7 @@ static void flctl_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip,
 				   const uint8_t *buf)
 {
 	chip->write_buf(mtd, buf, mtd->writesize);
+	chip->write_buf(mtd, chip->oob_poi, mtd->oobsize);
 }
 
 static void execmd_read_page_sector(struct mtd_info *mtd, int page_addr)
@@ -467,7 +482,7 @@ static void execmd_read_oob(struct mtd_info *mtd, int page_addr)
 static void execmd_write_page_sector(struct mtd_info *mtd)
 {
 	struct sh_flctl *flctl = mtd_to_flctl(mtd);
-	int i, page_addr = flctl->seqin_page_addr;
+	int page_addr = flctl->seqin_page_addr;
 	int sector, page_sectors;
 
 	page_sectors = flctl->page_size ? 4 : 1;
@@ -483,11 +498,7 @@ static void execmd_write_page_sector(struct mtd_info *mtd)
 
 	for (sector = 0; sector < page_sectors; sector++) {
 		write_fiforeg(flctl, 512, 512 * sector);
-
-		for (i = 0; i < 4; i++) {
-			wait_wecfifo_ready(flctl); /* wait for write ready */
-			writel(0xFFFFFFFF, FLECFIFO(flctl));
-		}
+		write_ec_fiforeg(flctl, 16, mtd->writesize + 16 * sector);
 	}
 
 	wait_completion(flctl);
-- 
1.7.5.4

Re: [PATCH 8/9] mtd: sh_flctl: Use user oob data in hardware ECC mode

[Laurent Pinchart]

Hi Bastian,

Thanks for the patch.

On Friday 20 April 2012 11:13:49 Bastian Hecht wrote:
> In hardware ecc mode, the flctl now writes and reads the oob data
> provided by the user. Additionally the ECC is now returned in normal
> page reads, not only when using the explicit NAND_CMD_READOOB command.

For my information again, what's the purpose of returning OOB data if the 
caller hasn't requested it ? What are those data then used for ?

> Signed-off-by: Bastian Hecht <hechtb@gmail.com>

-- 
Regards,

Laurent Pinchart

Re: [PATCH 8/9] mtd: sh_flctl: Use user oob data in hardware ECC mode

[Bastian Hecht]

Hello Laurent,

2012/4/21 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Bastian,
>
> Thanks for the patch.
>
> On Friday 20 April 2012 11:13:49 Bastian Hecht wrote:
>> In hardware ecc mode, the flctl now writes and reads the oob data
>> provided by the user. Additionally the ECC is now returned in normal
>> page reads, not only when using the explicit NAND_CMD_READOOB command.
>
> For my information again, what's the purpose of returning OOB data if the
> caller hasn't requested it ? What are those data then used for ?

There is an active discussion going on whether to pass a boolean to
nand_{read,write} that indicates if we need oob data or not. I assume
this to make it into the mainline then I can adapt this to the flctl
driver. The data can be used by file systems or bad block marking or
any other organizational needs like wear leveling and so on.

Best regards,

 Bastian Hecht


>> Signed-off-by: Bastian Hecht <hechtb@gmail.com>
>
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH 8/9] mtd: sh_flctl: Use user oob data in hardware ECC mode

[Bastian Hecht]

2012/4/23 Bastian Hecht <hechtb@googlemail.com>:
> Hello Laurent,
>
> 2012/4/21 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
>> Hi Bastian,
>>
>> Thanks for the patch.
>>
>> On Friday 20 April 2012 11:13:49 Bastian Hecht wrote:
>>> In hardware ecc mode, the flctl now writes and reads the oob data
>>> provided by the user. Additionally the ECC is now returned in normal
>>> page reads, not only when using the explicit NAND_CMD_READOOB command.
>>
>> For my information again, what's the purpose of returning OOB data if the
>> caller hasn't requested it ? What are those data then used for ?
>
> There is an active discussion going on whether to pass a boolean to
> nand_{read,write} that indicates if we need oob data or not. I assume
> this to make it into the mainline then I can adapt this to the flctl
> driver. The data can be used by file systems or bad block marking or
> any other organizational needs like wear leveling and so on.

I'm unsure if I missed your point here - we just don't know if we need
it or not. The discussion I mentioned primarily takes place here at
the mtd mailing list:

[PATCH 1/2] mtd: nand: add OOB argument to NAND {read,write}_page interfaces
http://lists.infradead.org/pipermail/linux-mtd/2012-April/040714.html

Now I'm confused as well whether I should skip the read oob part of the patch.
I'll skip the read part of the patch until a decision is made, I think.

Best regards,

 Bastian Hecht


> Best regards,
>
>  Bastian Hecht
>
>
>>> Signed-off-by: Bastian Hecht <hechtb@gmail.com>
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>>

Re: [PATCH 8/9] mtd: sh_flctl: Use user oob data in hardware ECC mode

[Laurent Pinchart]

Hi Bastian,

On Monday 23 April 2012 11:36:29 Bastian Hecht wrote:
> 2012/4/23 Bastian Hecht <hechtb@googlemail.com>:
> > 2012/4/21 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> >> On Friday 20 April 2012 11:13:49 Bastian Hecht wrote:
> >>> In hardware ecc mode, the flctl now writes and reads the oob data
> >>> provided by the user. Additionally the ECC is now returned in normal
> >>> page reads, not only when using the explicit NAND_CMD_READOOB command.
> >> 
> >> For my information again, what's the purpose of returning OOB data if the
> >> caller hasn't requested it ? What are those data then used for ?
> > 
> > There is an active discussion going on whether to pass a boolean to
> > nand_{read,write} that indicates if we need oob data or not. I assume
> > this to make it into the mainline then I can adapt this to the flctl
> > driver. The data can be used by file systems or bad block marking or
> > any other organizational needs like wear leveling and so on.
> 
> I'm unsure if I missed your point here - we just don't know if we need
> it or not. The discussion I mentioned primarily takes place here at
> the mtd mailing list:
> 
> [PATCH 1/2] mtd: nand: add OOB argument to NAND {read,write}_page interfaces
> http://lists.infradead.org/pipermail/linux-mtd/2012-April/040714.html
> 
> Now I'm confused as well whether I should skip the read oob part of the
> patch. I'll skip the read part of the patch until a decision is made, I
> think.

My point was just that it was pointless to read/write OOB data if the caller 
doesn't use them. It's an optimization: reading OOB data won't hurt regardless 
of what the caller does with it, but it will use CPU time and power for no 
reason. Adding an OOB argument to the {read,write}_page function would make 
this explicit.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 8/9] mtd: sh_flctl: Use user oob data in hardware ECC mode

[Brian Norris]

On Tue, Apr 24, 2012 at 2:33 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Monday 23 April 2012 11:36:29 Bastian Hecht wrote:
>> 2012/4/23 Bastian Hecht <hechtb@googlemail.com>:
>> > 2012/4/21 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
>> >> On Friday 20 April 2012 11:13:49 Bastian Hecht wrote:
>> >>> In hardware ecc mode, the flctl now writes and reads the oob data
>> >>> provided by the user. Additionally the ECC is now returned in normal
>> >>> page reads, not only when using the explicit NAND_CMD_READOOB command.
>> >>
>> >> For my information again, what's the purpose of returning OOB data if the
>> >> caller hasn't requested it ? What are those data then used for ?
>> >
>> > There is an active discussion going on whether to pass a boolean to
>> > nand_{read,write} that indicates if we need oob data or not. I assume
>> > this to make it into the mainline then I can adapt this to the flctl
>> > driver. The data can be used by file systems or bad block marking or
>> > any other organizational needs like wear leveling and so on.
>>
>> I'm unsure if I missed your point here - we just don't know if we need
>> it or not. The discussion I mentioned primarily takes place here at
>> the mtd mailing list:
>
> My point was just that it was pointless to read/write OOB data if the caller
> doesn't use them. It's an optimization: reading OOB data won't hurt regardless
> of what the caller does with it, but it will use CPU time and power for no
> reason. Adding an OOB argument to the {read,write}_page function would make
> this explicit.

Right, it is pointless and should be changed fairly soon, if my
patches go through. But until the additional argument is added, you
cannot guarantee that the interface wasn't expecting both data+OOB to
be read. For instance, the mtd_read_oob interface may call
nand_do_read_ops() with non-null datbuf and oobbuf. We have just such
a case in mtdswap.c and nand_bbt.c, I think. See Shmulik's comments
here (some of which only apply to mtd_write_oob):

http://lists.infradead.org/pipermail/linux-mtd/2012-April/040800.html

Brian

Re: [PATCH 8/9] mtd: sh_flctl: Use user oob data in hardware ECC mode

[Bastian Hecht]

>> My point was just that it was pointless to read/write OOB data if the caller
>> doesn't use them. It's an optimization: reading OOB data won't hurt regardless
>> of what the caller does with it, but it will use CPU time and power for no
>> reason. Adding an OOB argument to the {read,write}_page function would make
>> this explicit.
>
> Right, it is pointless and should be changed fairly soon, if my
> patches go through. But until the additional argument is added, you
> cannot guarantee that the interface wasn't expecting both data+OOB to
> be read. For instance, the mtd_read_oob interface may call
> nand_do_read_ops() with non-null datbuf and oobbuf. We have just such
> a case in mtdswap.c and nand_bbt.c, I think. See Shmulik's comments
> here (some of which only apply to mtd_write_oob):
>
> http://lists.infradead.org/pipermail/linux-mtd/2012-April/040800.html
>
> Brian

So after some back and forth, I'll leave the 8/9 patch as it is to
ensure compliance to the nand base code and wait for Brian's patches
to make it into the mainline.

Best regards,

 Bastian Hecht

[PATCH 9/9] ARM: sh-mobile: mackerel: Use hardware error correction

[Bastian Hecht]

Use the built-in hardware error code correction of the sh_flctl.

Signed-off-by: Bastian Hecht <hechtb@gmail.com>
---
 arch/arm/mach-shmobile/board-mackerel.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-shmobile/board-mackerel.c b/arch/arm/mach-shmobile/board-mackerel.c
index 1de1fb7..f8289f4 100644
--- a/arch/arm/mach-shmobile/board-mackerel.c
+++ b/arch/arm/mach-shmobile/board-mackerel.c
@@ -1003,6 +1003,7 @@ static struct sh_flctl_platform_data nand_flash_data = {
 	.flcmncr_val	= CLK_16B_12L_4H | TYPESEL_SET
 			| SHBUSSEL | SEL_16BIT | SNAND_E,
 	.use_holden	= 1,
+	.has_hwecc	= 1,
 };
 
 static struct platform_device nand_flash_device = {
-- 
1.7.5.4