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

[Ezequiel Garcia]

On 16 August 2015 at 19:18, Robert Jarzmik <robert.jarzmik@xxxxxxx> wrote:
> Ezequiel Garcia <ezequiel@xxxxxxxxxxxxxxxxxxxx> writes:
>
>> On 12 Aug 06:22 PM, Robert Jarzmik wrote:
>>
>> This fix looks correct. Thanks!
>>
>> Couple questions:
>>
>> 1. In which platform are you seeing this bug?
> zylonite with a pxa310 (ie. internal stacked NAND).
>
>> 2. Is this a regression? (i.e. should we queue it for -stable?)
> No, it's been there for ages I think.
>
>> Also, one might question why we can't just write NDSR right after it's read,
>> before we wake the IRQ thread or start DMA. It appears this is
>> a requirement of BCH, as per the comment in drain_fifo.
> For irq thread that won't make any difference, the irq handler will finish first
> and clear the bits anyway. For DMA it's better.
>
> And more generaly speaking, I like it better, to clear it once read.
>
>> It would be nice to put a comment explaining why we clear NDSR only
>> before the check to WRCMDREQ. Maybe even copy-pasting something
>> from the commit log?
> If we move it up to something like that :
>         status = nand_readl(info, NDSR);
>         nand_writel(info, NDSR, status);
> Then the comment is overkill I think.
>

Well, the comment in drain_fifo says that doing this would break
reads with BCH enabled:

                /*
                 * According to the datasheet, when reading from NDDB
                 * with BCH enabled, after each 32 bytes reads, we
                 * have to make sure that the NDSR.RDDREQ bit is set.
                 *
                 * Drain the FIFO 8 32 bits reads at a time, and skip
                 * the polling on the last read.
                 */

In other words, it seems that we must wake the IRQ thread handler
_before_ we clear RDDREQ, not after.

So unless I'm completely off, the current patch is right, and a comment
would be helpful.

>> I'd like to say "Yay, let's pick it" but I'd like to make sure this is
>> tested on all platforms first (unless you've tested it already).
> I tested on zylonite (where bug occurs) and cm-x300 (where bug never occurs).
>

OK, I'll see about testing your four patches on some Armada 370/XP.

-- 
Ezequiel GarcÃa, VanguardiaSur
www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/