[PATCH 1/3] mtd: rawnand: vf610_nfc: align IRQ bit naming

[PATCH 1/3] mtd: rawnand: vf610_nfc: align IRQ bit naming

[Stefan Agner]

Rename the IRQ DONE clear bit to align with other IRQ bits.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/mtd/nand/raw/vf610_nfc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/vf610_nfc.c b/drivers/mtd/nand/raw/vf610_nfc.c
index d5a22fc96878..740a91c5c86e 100644
--- a/drivers/mtd/nand/raw/vf610_nfc.c
+++ b/drivers/mtd/nand/raw/vf610_nfc.c
@@ -132,7 +132,7 @@
 /* NFC_IRQ_STATUS Field */
 #define IDLE_IRQ_BIT				BIT(29)
 #define IDLE_EN_BIT				BIT(20)
-#define CMD_DONE_CLEAR_BIT			BIT(18)
+#define DONE_CLEAR_BIT				BIT(18)
 #define IDLE_CLEAR_BIT				BIT(17)
 
 /*
@@ -290,7 +290,7 @@ static inline void vf610_nfc_clear_status(struct vf610_nfc *nfc)
 {
 	u32 tmp = vf610_nfc_read(nfc, NFC_IRQ_STATUS);
 
-	tmp |= CMD_DONE_CLEAR_BIT | IDLE_CLEAR_BIT;
+	tmp |= DONE_CLEAR_BIT | IDLE_CLEAR_BIT;
 	vf610_nfc_write(nfc, NFC_IRQ_STATUS, tmp);
 }
 
-- 
2.18.0

[PATCH 2/3] mtd: rawnand: vf610_nfc: explicitly disable interrupts first

[Stefan Agner]

Explicitly disable all interrupts on probe. This should be the
default state, but the bootloader could leave the device in any
state. No issues have been observed so far, but it is still worth
fixing it.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/mtd/nand/raw/vf610_nfc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/mtd/nand/raw/vf610_nfc.c b/drivers/mtd/nand/raw/vf610_nfc.c
index 740a91c5c86e..52e7811c0bde 100644
--- a/drivers/mtd/nand/raw/vf610_nfc.c
+++ b/drivers/mtd/nand/raw/vf610_nfc.c
@@ -130,8 +130,13 @@
 #define CONFIG_PAGE_CNT_SHIFT			0
 
 /* NFC_IRQ_STATUS Field */
+#define WERR_IRQ_BIT				BIT(31)
+#define DONE_IRQ_BIT				BIT(30)
 #define IDLE_IRQ_BIT				BIT(29)
+#define WERR_EN_BIT				BIT(22)
+#define DONE_EN_BIT				BIT(21)
 #define IDLE_EN_BIT				BIT(20)
+#define WERR_CLEAR_BIT				BIT(19)
 #define DONE_CLEAR_BIT				BIT(18)
 #define IDLE_CLEAR_BIT				BIT(17)
 
@@ -819,6 +824,10 @@ static int vf610_nfc_probe(struct platform_device *pdev)
 
 	init_completion(&nfc->cmd_done);
 
+	vf610_nfc_clear(nfc, NFC_IRQ_STATUS, WERR_EN_BIT);
+	vf610_nfc_clear(nfc, NFC_IRQ_STATUS, DONE_EN_BIT);
+	vf610_nfc_clear(nfc, NFC_IRQ_STATUS, IDLE_EN_BIT);
+
 	err = devm_request_irq(nfc->dev, irq, vf610_nfc_irq, 0, DRV_NAME, mtd);
 	if (err) {
 		dev_err(nfc->dev, "Error requesting IRQ!\n");
-- 
2.18.0

[PATCH 3/3] mtd: rawnand: vf610_nfc: handle only idle interrupts

[Stefan Agner]

There are two similar sounding interrupts: IDLE and DONE. The DONE
interrupt only reflects the NAND command state, but not DMA or HW
ECC correction. The IDLE interrupt reflects that all components are
idle. The driver can assume that the NAND command finished and that
data have been transferred and corrected.

This driver makes use of the IDLE interrupt only. But before this
patch the driver still handled all interrupts and cleared the DONE
interrupt although not used. This works just fine in practice, but
is not the way it should be done.

This patch makes sure that only the IDLE interrupt is handled and
cleared. To do so we have to clear the status bit in the interrupt
routine. With that it is also save to keep the interrupt always
enabled and just clear the IRQ in the interrupt handler.

This new interrupt handling has been tested for several thousand
boots and did not show any adverse effects.

Signed-off-by: Stefan Agner <stefan@agner.ch>
Tested-by: Stefan Agner <stefan@agner.ch>
---
 drivers/mtd/nand/raw/vf610_nfc.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/mtd/nand/raw/vf610_nfc.c b/drivers/mtd/nand/raw/vf610_nfc.c
index 52e7811c0bde..39307f4b1c94 100644
--- a/drivers/mtd/nand/raw/vf610_nfc.c
+++ b/drivers/mtd/nand/raw/vf610_nfc.c
@@ -290,15 +290,6 @@ static inline void vf610_nfc_wr_to_sram(void __iomem *dst, const void *src,
 	}
 }
 
-/* Clear flags for upcoming command */
-static inline void vf610_nfc_clear_status(struct vf610_nfc *nfc)
-{
-	u32 tmp = vf610_nfc_read(nfc, NFC_IRQ_STATUS);
-
-	tmp |= DONE_CLEAR_BIT | IDLE_CLEAR_BIT;
-	vf610_nfc_write(nfc, NFC_IRQ_STATUS, tmp);
-}
-
 static void vf610_nfc_done(struct vf610_nfc *nfc)
 {
 	unsigned long timeout = msecs_to_jiffies(100);
@@ -310,24 +301,29 @@ static void vf610_nfc_done(struct vf610_nfc *nfc)
 	 * vf610_nfc_set implicates such a barrier by using writel
 	 * to write to the register.
 	 */
-	vf610_nfc_set(nfc, NFC_IRQ_STATUS, IDLE_EN_BIT);
 	vf610_nfc_set(nfc, NFC_FLASH_CMD2, START_BIT);
 
 	if (!wait_for_completion_timeout(&nfc->cmd_done, timeout))
 		dev_warn(nfc->dev, "Timeout while waiting for BUSY.\n");
-
-	vf610_nfc_clear_status(nfc);
 }
 
 static irqreturn_t vf610_nfc_irq(int irq, void *data)
 {
 	struct mtd_info *mtd = data;
 	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
+	u32 status;
 
-	vf610_nfc_clear(nfc, NFC_IRQ_STATUS, IDLE_EN_BIT);
-	complete(&nfc->cmd_done);
+	status = vf610_nfc_read(nfc, NFC_IRQ_STATUS);
 
-	return IRQ_HANDLED;
+	if (status & IDLE_IRQ_BIT) {
+		status |= IDLE_CLEAR_BIT;
+		vf610_nfc_write(nfc, NFC_IRQ_STATUS, status);
+		complete(&nfc->cmd_done);
+
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
 }
 
 static inline void vf610_nfc_ecc_mode(struct vf610_nfc *nfc, int ecc_mode)
@@ -826,7 +822,9 @@ static int vf610_nfc_probe(struct platform_device *pdev)
 
 	vf610_nfc_clear(nfc, NFC_IRQ_STATUS, WERR_EN_BIT);
 	vf610_nfc_clear(nfc, NFC_IRQ_STATUS, DONE_EN_BIT);
-	vf610_nfc_clear(nfc, NFC_IRQ_STATUS, IDLE_EN_BIT);
+
+	vf610_nfc_set(nfc, NFC_IRQ_STATUS, IDLE_CLEAR_BIT);
+	vf610_nfc_set(nfc, NFC_IRQ_STATUS, IDLE_EN_BIT);
 
 	err = devm_request_irq(nfc->dev, irq, vf610_nfc_irq, 0, DRV_NAME, mtd);
 	if (err) {
-- 
2.18.0

Re: [PATCH 2/3] mtd: rawnand: vf610_nfc: explicitly disable interrupts first

[Miquel Raynal]

Hi Stefan,

Stefan Agner <stefan@agner.ch> wrote on Mon,  6 Aug 2018 11:29:08 +0200:

> Explicitly disable all interrupts on probe. This should be the
> default state, but the bootloader could leave the device in any
> state. No issues have been observed so far, but it is still worth
> fixing it.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  drivers/mtd/nand/raw/vf610_nfc.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/vf610_nfc.c b/drivers/mtd/nand/raw/vf610_nfc.c
> index 740a91c5c86e..52e7811c0bde 100644
> --- a/drivers/mtd/nand/raw/vf610_nfc.c
> +++ b/drivers/mtd/nand/raw/vf610_nfc.c
> @@ -130,8 +130,13 @@
>  #define CONFIG_PAGE_CNT_SHIFT			0
>  
>  /* NFC_IRQ_STATUS Field */
> +#define WERR_IRQ_BIT				BIT(31)
> +#define DONE_IRQ_BIT				BIT(30)
>  #define IDLE_IRQ_BIT				BIT(29)
> +#define WERR_EN_BIT				BIT(22)
> +#define DONE_EN_BIT				BIT(21)
>  #define IDLE_EN_BIT				BIT(20)
> +#define WERR_CLEAR_BIT				BIT(19)
>  #define DONE_CLEAR_BIT				BIT(18)
>  #define IDLE_CLEAR_BIT				BIT(17)
>  
> @@ -819,6 +824,10 @@ static int vf610_nfc_probe(struct platform_device *pdev)
>  
>  	init_completion(&nfc->cmd_done);
>  
> +	vf610_nfc_clear(nfc, NFC_IRQ_STATUS, WERR_EN_BIT);
> +	vf610_nfc_clear(nfc, NFC_IRQ_STATUS, DONE_EN_BIT);
> +	vf610_nfc_clear(nfc, NFC_IRQ_STATUS, IDLE_EN_BIT);
> +

Why not just one call with:

	vf610_nfc_clear(nfc, NFC_IRQ_STATUS, WERR_EN_BIT | DONE_EN_BIT | IDLE_EN_BIT);

The comment applies for the next patch as well.

>  	err = devm_request_irq(nfc->dev, irq, vf610_nfc_irq, 0, DRV_NAME, mtd);
>  	if (err) {
>  		dev_err(nfc->dev, "Error requesting IRQ!\n");

Thanks,
Miquèl