Rename the IRQ DONE clear bit to align with other IRQ bits.
Signed-off-by: Stefan Agner <[email protected]>
---
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
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 <[email protected]>
Tested-by: Stefan Agner <[email protected]>
---
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
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 <[email protected]>
---
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
Hi Stefan,
Stefan Agner <[email protected]> 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 <[email protected]>
> ---
> 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