From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Nelson Date: Wed, 03 Apr 2013 16:47:35 -0700 Subject: [U-Boot] [PATCH] mx6: fsl_esdhc: Fix waiting for DMA operation completion In-Reply-To: <515CB88C.3030909@boundarydevices.com> References: <1364897095-28227-1-git-send-email-andrew_gabbasov@mentor.com>, <515AFE1E.801@boundarydevices.com> <6C5EA58090A5ED459815C4D04C2B466FD931DF69@EU-MBX-03.mgc.mentorg.com>, <515B4FDB.9020902@boundarydevices.com> <6C5EA58090A5ED459815C4D04C2B466FD931DFA2@EU-MBX-03.mgc.mentorg.com>, <515C30C4.1030802@boundarydevices.com> <6C5EA58090A5ED459815C4D04C2B466FD931E08D@EU-MBX-03.mgc.mentorg.com> <515CB88C.3030909@boundarydevices.com> Message-ID: <515CBF97.30701@boundarydevices.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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(®s->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(®s->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(®s->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.