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

[Gabbasov, Andrew]

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?

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

Thanks.

Best regards,
Andrew