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

[Chris Packham <Chris.Packham@xxxxxxxxxxxxxxxxxxx>]

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.

> But if this is a problem, many other parts of that driver should be
> equally affected.
> 
>> On 28/09/18 19:43, Daniel Mack wrote:
>>    >
>>    > Also, does my .EALREADY approach (v1) make any difference?
>>    >
>>
>> The v1 of this patch doesn't show the problem.
> 
> That's also very strange because the condition it triggers on is exactly
> the same.

One difference is that by calling complete() interrupts will be disabled 
in the spinlock.

> 
> 
> Thanks,
> Daniel
> 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/