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

[Eric Nelson]

Hi Andrew,

On 04/04/2013 11:03 AM, Gabbasov, Andrew wrote:

Hi Eric,

From: Eric Nelson [eric.nel...@boundarydevices.com]
Sent: Thursday, April 04, 2013 03:47
To: Gabbasov, Andrew
Cc: u-boot@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

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot