Re: [PATCH v2] mtd: rawnand: marvell: check for RDY bits after enabling the IRQ

[Boris Brezillon <boris.brezillon@bootlin.com>]

On Mon, 1 Oct 2018 19:59:11 +0000
Chris Packham <Chris.Packham@alliedtelesis.co.nz> wrote:

> On 01/10/18 18:31, Daniel Mack wrote:
> > On 30/9/2018 11:10 PM, Chris Packham wrote:  
> >>>> With this in mind, I don't see why this
> >>>>
> >>>> +	st = readl_relaxed(nfc->regs + NDSR);
> >>>> +	if (st & (NDSR_RDY(0) | NDSR_RDY(1)))
> >>>> +		complete(&nfc->complete);  
> >>> Yeah, me neither. Chris, are you absolutely sure this is the reason? I'm
> >>> asking because it took me several tries sometimes to trigger the bug, so
> >>> is there a chance that you see an error at all times, regardless of
> >>> whether my patch is applied?  
> >> It seems pretty consistent. Without this patch there seems to be no
> >> problem. With this patch it triggers pretty much straight away. I can't
> >> discount that there might be something wrong with my dts (the R/B
> >> configuration was missing initially).
> >>
> >> I've also been able to run this on the DB-88F6820-AMC board with the
> >> same result (the dts for this is in the for-next branch of
> >> git://git.infradead.org/linux-mvebu.git).
> >>
> >> The really odd thing is the following seems to avoid the problem
> >>
> >> +        st = readl_relaxed(nfc->regs + NDSR);
> >> +        udelay(1000);
> >> +        if (st & (NDSR_RDY(0) | NDSR_RDY(1)))
> >> +                complete(&nfc->complete);
> >>
> >> Which is weird because the st value has already been read so the udelay
> >> should have no effect.  
> > 
> > Erm, yes. That's totally weird. Which gcc are you using for this?  
> 
> arm-softfloat-linux-gnueabi-gcc (crosstool-NG crosstool-ng-1.22.0) 4.9.3
> 
> > Could you please try and use readl() here instead of readl_relaxed()?
> > That will place a memory barrier after the read to enforce ordering.  
> 
> I'd previously tried readl() based on the same hunch. No change.
> 
> I think my snippet above might be misleading. While a delay between 
> readl_relaxed() and the if should not change the outcome, this is also a 
> delay between marvell_nfc_enable_int() and marvell_nfc_disable_int() 
> which is probably more significant. Sure enough if I move the delay to 
> just before the marvell_nfc_disable_int() the error is not seen.

AFAICT, your timeout always happens when waiting for RDREQ, not RDYM.
So maybe disabling MRDY too early has a side-effect on the RDREQ event.