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

login
register
mail settings
Submitter Bastian Hecht
Date April 20, 2012, 9:13 a.m.
Message ID <1334913230-23615-2-git-send-email-hechtb@gmail.com>
Download mbox | patch
Permalink /patch/153974/
State New
Headers show

Comments

Bastian Hecht - April 20, 2012, 9:13 a.m.
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(-)
Laurent Pinchart - April 21, 2012, 2:29 p.m.
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];
Bastian Hecht - April 23, 2012, 8:41 a.m.
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

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];