From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Nelson Date: Thu, 04 Apr 2013 11:41:09 -0700 Subject: [U-Boot] [PATCH] mx6: fsl_esdhc: Fix waiting for DMA operation completion In-Reply-To: <6C5EA58090A5ED459815C4D04C2B466FD931E121@EU-MBX-03.mgc.mentorg.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>, <515CBF97.30701@boundarydevices.com> <6C5EA58090A5ED459815C4D04C2B466FD931E121@EU-MBX-03.mgc.mentorg.com> Message-ID: <515DC945.20306@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 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(®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. > > So, do you think the latter (modified) loop condition > > } while (!(irqstat & IRQSTAT_TC) || !(irqstat & IRQSTAT_DINT) || > (esdhc_read32(®s->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