[PATCH RESEND 0/2] mtd: nand: pxa3xx-nand: fixes

[PATCH RESEND 0/2] mtd: nand: pxa3xx-nand: fixes

[Robert Jarzmik]

Hi Brian,

As promised, here are the remaining fixes I have on pxa3xx-nand I'd like you to
queue up. The other changes I have submitted are under review and not of "fixes" type.

Cheers.

--
Robert

Robert Jarzmik (2):
  mtd: nand: pxa3xx_nand: fix early spurious interrupt
  mtd: nand: pxa3xx-nand: fix random command timeouts

 drivers/mtd/nand/pxa3xx_nand.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

-- 
2.1.4

[PATCH RESEND 1/2] mtd: nand: pxa3xx_nand: fix early spurious interrupt

[Robert Jarzmik]

When the nand is first probe, and upon the first command start, the
status bits should be cleared before the interrupts are unmasked.

The bug is tricky : if the bootloader left a status bit set, the
unmasking of interrupts does trigger the interrupt handler before the
first command is issued, blocking the good behavior of the nand.

The same would happen if in pxa3xx_nand code flow a status bit is left,
and then a command is started.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/mtd/nand/pxa3xx_nand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 1259cc558ce9..d1a4c336de1d 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -439,8 +439,8 @@ static void pxa3xx_nand_start(struct pxa3xx_nand_info *info)
 	ndcr |= NDCR_ND_RUN;
 
 	/* clear status bits and run */
-	nand_writel(info, NDCR, 0);
 	nand_writel(info, NDSR, NDSR_MASK);
+	nand_writel(info, NDCR, 0);
 	nand_writel(info, NDCR, ndcr);
 }
 
-- 
2.1.4

[PATCH RESEND 2/2] mtd: nand: pxa3xx-nand: fix random command timeouts

[Robert Jarzmik]

When 2 commands are submitted in a row, and the second is very quick,
the completion of the second command might never come. This happens
especially if the second command is quick, such as a status read after
an erase.

The issue is that in the interrupt handler, the status bits are cleared
after the new command is issued. There is a small temporal window where
this happens :
 - the previous command has set the command done bit
 - the ready for a command bit is set
 - the handler submits the next command
   - just then, the command completes, and the command done bit is still
     set
 - the handler clears the "previous" command done bit
 - the handler exits

In this flow, the "command done" of the next command will never trigger
a new interrupt to finish the status command, as it was cleared for both
commands.

Fix this by clearing the status bit before submitting a new command.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/mtd/nand/pxa3xx_nand.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index d1a4c336de1d..d6c696798811 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -675,8 +675,14 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
 		is_ready = 1;
 	}
 
+	/*
+	 * Clear all status bit before issuing the next command, which
+	 * can and will alter the status bits and will deserve a new
+	 * interrupt on its own. This lets the controller exit the IRQ
+	 */
+	nand_writel(info, NDSR, status);
+
 	if (status & NDSR_WRCMDREQ) {
-		nand_writel(info, NDSR, NDSR_WRCMDREQ);
 		status &= ~NDSR_WRCMDREQ;
 		info->state = STATE_CMD_HANDLE;
 
@@ -697,8 +703,6 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
 			nand_writel(info, NDCB0, info->ndcb3);
 	}
 
-	/* clear NDSR to let the controller exit the IRQ */
-	nand_writel(info, NDSR, status);
 	if (is_completed)
 		complete(&info->cmd_complete);
 	if (is_ready)
-- 
2.1.4

Re: [PATCH RESEND 2/2] mtd: nand: pxa3xx-nand: fix random command timeouts

[Ezequiel Garcia]

On 19 August 2015 at 15:30, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> When 2 commands are submitted in a row, and the second is very quick,
> the completion of the second command might never come. This happens
> especially if the second command is quick, such as a status read after
> an erase.
>
> The issue is that in the interrupt handler, the status bits are cleared
> after the new command is issued. There is a small temporal window where
> this happens :
>  - the previous command has set the command done bit
>  - the ready for a command bit is set
>  - the handler submits the next command
>    - just then, the command completes, and the command done bit is still
>      set
>  - the handler clears the "previous" command done bit
>  - the handler exits
>
> In this flow, the "command done" of the next command will never trigger
> a new interrupt to finish the status command, as it was cleared for both
> commands.
>
> Fix this by clearing the status bit before submitting a new command.
>
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
>  drivers/mtd/nand/pxa3xx_nand.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index d1a4c336de1d..d6c696798811 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -675,8 +675,14 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>                 is_ready = 1;
>         }
>
> +       /*
> +        * Clear all status bit before issuing the next command, which
> +        * can and will alter the status bits and will deserve a new
> +        * interrupt on its own. This lets the controller exit the IRQ
> +        */
> +       nand_writel(info, NDSR, status);
> +

Actually, the comment I had in mind was more about why
we *cannot* clear the NDSR register before waking the
IRQ thread.

But this is a nitpick: I don't care much :-)

>         if (status & NDSR_WRCMDREQ) {
> -               nand_writel(info, NDSR, NDSR_WRCMDREQ);
>                 status &= ~NDSR_WRCMDREQ;
>                 info->state = STATE_CMD_HANDLE;
>
> @@ -697,8 +703,6 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>                         nand_writel(info, NDCB0, info->ndcb3);
>         }
>
> -       /* clear NDSR to let the controller exit the IRQ */
> -       nand_writel(info, NDSR, status);
>         if (is_completed)
>                 complete(&info->cmd_complete);
>         if (is_ready)
> --
> 2.1.4
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/



-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH RESEND 0/2] mtd: nand: pxa3xx-nand: fixes

[Ezequiel Garcia]

On 19 August 2015 at 15:30, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Hi Brian,
>
> As promised, here are the remaining fixes I have on pxa3xx-nand I'd like you to
> queue up. The other changes I have submitted are under review and not of "fixes" type.
>
> Cheers.
>
> --
> Robert
>
> Robert Jarzmik (2):
>   mtd: nand: pxa3xx_nand: fix early spurious interrupt
>   mtd: nand: pxa3xx-nand: fix random command timeouts
>

For both patches:

Acked-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

I tested this on Armada 370:

Tested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH RESEND 0/2] mtd: nand: pxa3xx-nand: fixes

[Brian Norris]

On Wed, Aug 19, 2015 at 06:18:57PM -0300, Ezequiel Garcia wrote:
> On 19 August 2015 at 15:30, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> > Hi Brian,
> >
> > As promised, here are the remaining fixes I have on pxa3xx-nand I'd like you to
> > queue up. The other changes I have submitted are under review and not of "fixes" type.
> >
> > Cheers.
> >
> > --
> > Robert
> >
> > Robert Jarzmik (2):
> >   mtd: nand: pxa3xx_nand: fix early spurious interrupt
> >   mtd: nand: pxa3xx-nand: fix random command timeouts
> >
> 
> For both patches:
> 
> Acked-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> 
> I tested this on Armada 370:
> 
> Tested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Pushed both to l2-mtd.git, with Ezequiel's tags. Thanks for collecting
the patches, testing, and reviewing.

Brian