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

[Eric Nelson <eric.nelson@boundarydevices.com>]

Hi Andrew,

On 04/04/2013 11:03 AM, Gabbasov, Andrew wrote:
> Hi Eric,
>
>> From: Eric Nelson [eric.nelson at boundarydevices.com]
>> Sent: Thursday, April 04, 2013 03:47
>> To: Gabbasov, Andrew
>> Cc: u-boot at lists.denx.de; Behme, Dirk - Bosch; Fabio Estevam
>> Subject: Re: [U-Boot] [PATCH] mx6: fsl_esdhc: Fix waiting for DMA operation completion
>>
>> 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.
>
> So, do you think the latter (modified) loop condition
>
>       } while (!(irqstat & IRQSTAT_TC) || !(irqstat & IRQSTAT_DINT) ||
>             (esdhc_read32(&regs->prsstat) & PRSSTAT_DLA));
>
> will be correct?
>

I think the right thing to do is eliminate the DLA test entirely,
so the loop condition can be simplified to something like this:

#define TRANSFER_COMPLETE (IRQSTAT_TC|IRQSTAT_DINT)

	do {
		...
	} while (TRANSFER_COMPLETE != (irqstat&TRANSFER_COMPLETE));

If there is another part that needs to bail out on PRSSTAT_DLA,
it seems that the affected part should be the one with the #ifdef

> It seems to be working without errors when reading boot scripts from
> dos partition on mmc.
>

Since this is a pretty small timing window, it's not that surprising
that this is hidden by any extra code (including the proper cache
flush).

Thanks for pushing this.


Eric