[U-Boot] [PATCH] mx6: fsl_esdhc: Fix waiting for DMA operation completion

[Eric Nelson]

On 04/03/2013 04:17 PM, Eric Nelson wrote:
> Hi Andrew,
>
> On 04/03/2013 10:30 AM, Gabbasov, Andrew wrote:
>>>
>>>> I think, it would be useful to have both patches. Although
>>>> invalidating cache
>>>> (by adding some delay) indirectly helps with waiting for DMA End event,
>>>> it is probably worth having explicit DMA completion waiting patch too.
>>>>
>>> I agree wholeheartedly.
>>>
>>> I do wonder if the previous loop should be re-worked though.
>>> It seems that we should be waiting for TC & (DINT|DMAE) on
>>> all processor variants and the previous loop has tests for
>>> timeout and data errors.
>>>
>>> Regards,
>>>
>>> Eric
>>
>> Hi Eric,
>>
>> Do you mean making it something like
>>
>>     do {
>>        irqstat = esdhc_read32(&regs->irqstat);
>>
>>        if (irqstat & IRQSTAT_DTOE)
>>           return TIMEOUT;
>>
>>        if (irqstat & (DATA_ERR | IRQSTAT_DMAE))
>>           return COMM_ERR;
>>
>>     } while (!((irqstat & IRQSTAT_TC) && (irqstat & IRQSTAT_DINT)) &&
>>           (esdhc_read32(&regs->prsstat) & PRSSTAT_DLA));
>>
>
> Yes. That's what I was thinking.
>
>> The check for DMAE (DMA Error) can be combined with other data errors
>> and cause exit from the loop. And DINT can be checked with TC in the loop
>> condition.
>>
> Makes sense.
>
>> Actually, I'm a little confused by PRSSTAT_DLA checking: currently the
>> loop exits
>> when either IRQSTAT_TC occurs _or_ PRSSTAT_DLA flag comes to 0. Is
>> that correct?
>> I'm not quite familiar with using this flag, but should the loop exit
>> when both
>> IRQSTAT_TC occurs _and_ PRSSTAT_DLA flag comes to 0 (i.e. in current
>> code '&&'
>> should be replaced by '||')? And then the modified loop condition
>> (with DMA check)
>> would be
>>
>>     } while (!(irqstat & IRQSTAT_TC) || !(irqstat & IRQSTAT_DINT) ||
>>           (esdhc_read32(&regs->prsstat) & PRSSTAT_DLA));
>>
>> Can you advise anything on using this flag?
>>
>
> That is weird, and suspect. The reference manual indicates that this
> bit (Data line active) will go low when the data lines are done with
> the transaction, but that will happen before the DMA completes, so
> it seems like a bad way to short-circuit the loop.
>
I just put a test in to see if PRSSTAT_DLA clears before IRQSTAT_TC
and was able to see it, but only with cache enabled (when presumably
the loop runs faster).

I pulled it out of the while loop test so I've seen at one
read with both TC and DINT bits clear in irqstat after a
read of prsstat with DLA high.

IOW, we are sometimes short-circuiting the loop based on DLA
clear, which seems faulty.