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

[Gabbasov, Andrew]

> From: Eric Nelson [eric.nelson at boundarydevices.com]
> Sent: Wednesday, April 03, 2013 17:38
> To: Gabbasov, Andrew
> Cc: u-boot at lists.denx.de; Behme, Dirk - Bosch
> Subject: Re: [U-Boot] [PATCH] mx6: fsl_esdhc: Fix waiting for DMA operation completion
> 
> Hi Andrew,
> 
> On 04/02/2013 11:48 PM, Gabbasov, Andrew wrote:
> 
> >>>> On 04/02/2013 03:04 AM, Andrew Gabbasov wrote:
> >>>>> On iMX6 sometimes the Transfer Complete interrupt occurs earlier
> >>>>> than the DMA part completes its operation. If immediately after that
> >>>>> the read data is used for some data verification, those obtained data
> >>>>> may be incomplete, which causes intermittent verification failures.
> >>>>>
> >>>>
> >>>> Can you describe how to repeat this?
> >>>>
> >>>>> For example, when the default environment command tries to load and run
> >>>>> boot script from FAT partition on SD/MMC card, it sometimes fails,
> >>>>> reporting invalid partition table, or unknown partition type, or
> >>>>> something else of that kind. Such errors disappear if the build
> >>>>> configuration has CONFIG_SYS_FSL_ESDHC_USE_PIO, or if some delay
> >>>>> is added after transfer completion.
> >>>>>
> >>>
> >>
> >>  <snip>
> >>
> >> Looking at the code with fresh eyes, it appears that
> >> the cache invalidate is in the wrong place (after
> >> "command complete" but before "transfer complete"
> >> is checked).
> >>
> >>  <snip>
> >>
> >> Andrew, can you test with this patch to see if
> >> it also addresses the issue?
> >>
> > Hi Eric,
> >
> > Yes, this patch seems to fix the issue too.
> >
> 
> Thanks for testing.
> 
> > 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));

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.

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?

Thanks.

Best regards,
Andrew