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

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

[Andrew Gabbasov]

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.

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.

Adding extra waiting for DMA completion after Transfer Complete
event fixes this issue.

Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
 drivers/mmc/fsl_esdhc.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
index d2a505e..806c6dd 100644
--- a/drivers/mmc/fsl_esdhc.c
+++ b/drivers/mmc/fsl_esdhc.c
@@ -402,6 +402,12 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
 				return COMM_ERR;
 		} while (!(irqstat & IRQSTAT_TC) &&
 				(esdhc_read32(&regs->prsstat) & PRSSTAT_DLA));
+#ifdef CONFIG_MX6
+		/* In imx6 TC (data end) interrupt sometimes occur earlier
+		   than DMA completes. In this case just wait a little more. */ 
+		while (!(irqstat & (IRQSTAT_DINT | IRQSTAT_DMAE)))
+			irqstat = esdhc_read32(&regs->irqstat);
+#endif
 #endif
 	}
 
-- 
1.7.10.4

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

[Eric Nelson]

Thanks Andrew,

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.
>
We do this on every boot on SABRE Lite and Nitrogen6x boards,
and haven't seen an issue.

What board are you testing on?

Do you have cache enabled?

Is this with an SD card or eMMC?

> Adding extra waiting for DMA completion after Transfer Complete
> event fixes this issue.
>
> Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> ---
>   drivers/mmc/fsl_esdhc.c |    6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> index d2a505e..806c6dd 100644
> --- a/drivers/mmc/fsl_esdhc.c
> +++ b/drivers/mmc/fsl_esdhc.c
> @@ -402,6 +402,12 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
>   				return COMM_ERR;
>   		} while (!(irqstat & IRQSTAT_TC) &&
>   				(esdhc_read32(&regs->prsstat) & PRSSTAT_DLA));
> +#ifdef CONFIG_MX6
> +		/* In imx6 TC (data end) interrupt sometimes occur earlier
> +		   than DMA completes. In this case just wait a little more. */
> +		while (!(irqstat & (IRQSTAT_DINT | IRQSTAT_DMAE)))
> +			irqstat = esdhc_read32(&regs->irqstat);
> +#endif
>   #endif
>   	}
>
>

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

[Dirk Behme]

Am 02.04.2013 17:49, schrieb Eric Nelson:
> Thanks Andrew,
>
> 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.
>>
> We do this on every boot on SABRE Lite and Nitrogen6x boards,
> and haven't seen an issue.
>
> What board are you testing on?
>
> Do you have cache enabled?
>
> Is this with an SD card or eMMC?

Andrew will have the details, but to my understanding this implements 
in U-Boot what the Freescale kernel has in

http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/drivers/mmc/host/sdhci-esdhc-imx.c?h=imx_3.0.35_12.09.01#n234

for TO 1.0.

Best regards

Dirk

>> Adding extra waiting for DMA completion after Transfer Complete
>> event fixes this issue.
>>
>> Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
>> ---
>>   drivers/mmc/fsl_esdhc.c |    6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
>> index d2a505e..806c6dd 100644
>> --- a/drivers/mmc/fsl_esdhc.c
>> +++ b/drivers/mmc/fsl_esdhc.c
>> @@ -402,6 +402,12 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd
>> *cmd, struct mmc_data *data)
>>                   return COMM_ERR;
>>           } while (!(irqstat & IRQSTAT_TC) &&
>>                   (esdhc_read32(&regs->prsstat) & PRSSTAT_DLA));
>> +#ifdef CONFIG_MX6
>> +        /* In imx6 TC (data end) interrupt sometimes occur earlier
>> +           than DMA completes. In this case just wait a little
>> more. */
>> +        while (!(irqstat & (IRQSTAT_DINT | IRQSTAT_DMAE)))
>> +            irqstat = esdhc_read32(&regs->irqstat);
>> +#endif
>>   #endif
>>       }
>>
>>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

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

[Eric Nelson]

Thanks Dirk,

On 04/02/2013 11:10 AM, Dirk Behme wrote:
> Am 02.04.2013 17:49, schrieb Eric Nelson:
>> Thanks Andrew,
>>
>> 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.
>>>
>> We do this on every boot on SABRE Lite and Nitrogen6x boards,
>> and haven't seen an issue.
>>
>> What board are you testing on?
>>
>> Do you have cache enabled?
>>
>> Is this with an SD card or eMMC?
>
> Andrew will have the details, but to my understanding this implements in
> U-Boot what the Freescale kernel has in
>
> http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/drivers/mmc/host/sdhci-esdhc-imx.c?h=imx_3.0.35_12.09.01#n234
>

Andrew, did you trap one of these?

[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 01:50
> To: Dirk Behme
> Cc: Gabbasov, Andrew; u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH] mx6: fsl_esdhc: Fix waiting for DMA operation completion
> 
> Thanks Dirk,
> 
> On 04/02/2013 11:10 AM, Dirk Behme wrote:
> > Am 02.04.2013 17:49, schrieb Eric Nelson:
> >> Thanks Andrew,
> >>
> >> 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.
> >>>
> >> We do this on every boot on SABRE Lite and Nitrogen6x boards,
> >> and haven't seen an issue.
> >>
> >> What board are you testing on?
> >>
> >> Do you have cache enabled?
> >>
> >> Is this with an SD card or eMMC?
> >
> > Andrew will have the details, but to my understanding this implements in
> > U-Boot what the Freescale kernel has in
> >
> > http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/drivers/mmc/host/sdhci-esdhc-imx.c?h=imx_3.0.35_12.09.01#n234
> >
> 
> Andrew, did you trap one of these?

Hi Eric,

Yes, I saw that part of code in the kernel and that led me to check if the events can come out of order in my case, and after finding that they indeed do, to make that patch. So, this is the similar workaround but on U-Boot level.

Thanks.

Best regards,
Andrew

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

[Gabbasov, Andrew]

> From: Eric Nelson [eric.nelson at boundarydevices.com]
> Sent: Tuesday, April 02, 2013 19:49
> To: Gabbasov, Andrew
> Cc: u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH] mx6: fsl_esdhc: Fix waiting for DMA operation completion
> 
> Thanks Andrew,
> 
> 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.
> >
> We do this on every boot on SABRE Lite and Nitrogen6x boards,
> and haven't seen an issue.
> 
> What board are you testing on?
> 
> Do you have cache enabled?
> 
> Is this with an SD card or eMMC?

Hi Eric,

Thank you for the response.

I'm using SabreLite with SD card.

For reproducing a problem I took the latest version of U-Boot from master branch and made
a couple of changes in include/configs/mx6qsabrelite.h:
- removed duplication of "mmc dev ${mmcdev};" in CONFIG_BOOTCOMMAND;
- and changed CONFIG_ENV_IS_IN_MMC to CONFIG_ENV_IS_NOWHERE
(thus disabling to try reading saved environment from mmc).
The boot.scr contains a single command "printenv". When I'm doing resets with this
configuration, I'm getting errors in MMC access approximately once per 4-5 boots.
The errors can be "Invalid partition 2" or "No partition table".

Indeed, I was unable to reproduce it with plain master version, but 2 simple changes in configuration, described above, allowed me to reproduce it.

Thanks.

Best regards,
Andrew

> 
> > Adding extra waiting for DMA completion after Transfer Complete
> > event fixes this issue.
> >
> > Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> > ---
> >   drivers/mmc/fsl_esdhc.c |    6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> > index d2a505e..806c6dd 100644
> > --- a/drivers/mmc/fsl_esdhc.c
> > +++ b/drivers/mmc/fsl_esdhc.c
> > @@ -402,6 +402,12 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
> >                               return COMM_ERR;
> >               } while (!(irqstat & IRQSTAT_TC) &&
> >                               (esdhc_read32(&regs->prsstat) & PRSSTAT_DLA));
> > +#ifdef CONFIG_MX6
> > +             /* In imx6 TC (data end) interrupt sometimes occur earlier
> > +                than DMA completes. In this case just wait a little more. */
> > +             while (!(irqstat & (IRQSTAT_DINT | IRQSTAT_DMAE)))
> > +                     irqstat = esdhc_read32(&regs->irqstat);
> > +#endif
> >   #endif
> >       }
> >
> >

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

[Eric Nelson]

Thanks Andrew,

On 04/02/2013 11:21 AM, Gabbasov, Andrew wrote:
>> From: Eric Nelson [eric.nelson at boundarydevices.com]
>> Sent: Tuesday, April 02, 2013 19:49
>> To: Gabbasov, Andrew
>> Cc: u-boot at lists.denx.de
>> Subject: Re: [U-Boot] [PATCH] mx6: fsl_esdhc: Fix waiting for DMA operation completion
>>
>> Thanks Andrew,
>>
>> 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.
>>>
>> We do this on every boot on SABRE Lite and Nitrogen6x boards,
>> and haven't seen an issue.
>>
>> What board are you testing on?
>>
>> Do you have cache enabled?
>>
>> Is this with an SD card or eMMC?
>
> Hi Eric,
>
> Thank you for the response.
>
> I'm using SabreLite with SD card.
>
> For reproducing a problem I took the latest version of U-Boot from master branch and made
> a couple of changes in include/configs/mx6qsabrelite.h:
> - removed duplication of "mmc dev ${mmcdev};" in CONFIG_BOOTCOMMAND;
> - and changed CONFIG_ENV_IS_IN_MMC to CONFIG_ENV_IS_NOWHERE
> (thus disabling to try reading saved environment from mmc).
> The boot.scr contains a single command "printenv". When I'm doing resets with this
> configuration, I'm getting errors in MMC access approximately once per 4-5 boots.
> The errors can be "Invalid partition 2" or "No partition table".
>
> Indeed, I was unable to reproduce it with plain master version, but 2 simple changes in configuration, described above, allowed me to reproduce it.
>

I think you exposed another bug here.

I was able to to reproduce the symptom (failure reading from
filesystem) on master by simply setting my bootcmd to this:
	mmc dev 0
	&& fatload mmc 0 10800000 uImage
	&& fatload mmc 0 12800000 uramdisk.img
	&& bootm 10800000 12800000

The failures differed somewhat from boot to boot,
but this was typical:

	reading uImage
	4350916 bytes read in 216 ms (19.2 MiB/s)
	bad MBR sector signature 0x0000
	** Invalid partition 1 **

I've seen weirdness in the past with cache enabled (as is the
case in the mx6qsabrelite_config), so I re-tested with
a 'dcache off' in bootcmd:

	dcache off ;
	mmc dev 0
	&& fatload mmc 0 10800000 uImage
	&& fatload mmc 0 12800000 uramdisk.img
	&& bootm 10800000 12800000

This appeared to fix the issue (through 100 boots).

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).

Dirk, this was on a TO1.2 device, so I don't think
this was caused by the out-of-order completion
interrupts.

Andrew, can you test with this patch to see if
it also addresses the issue?

By inspection, it seems needed.

Regards,


Eric
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fsl-esdhc-flush-cache-after-completion.patch
Type: text/x-diff
Size: 834 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130402/2be85957/attachment.patch>

[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 01: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
> 
> Thanks Andrew,
> 
> On 04/02/2013 11:21 AM, Gabbasov, Andrew wrote:
> >> From: Eric Nelson [eric.nelson at boundarydevices.com]
> >> Sent: Tuesday, April 02, 2013 19:49
> >> To: Gabbasov, Andrew
> >> Cc: u-boot at lists.denx.de
> >> Subject: Re: [U-Boot] [PATCH] mx6: fsl_esdhc: Fix waiting for DMA operation completion
> >>
> >> Thanks Andrew,
> >>
> >> 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.
> >>>
> >> We do this on every boot on SABRE Lite and Nitrogen6x boards,
> >> and haven't seen an issue.
> >>
> >> What board are you testing on?
> >>
> >> Do you have cache enabled?
> >>
> >> Is this with an SD card or eMMC?
> >
> > Hi Eric,
> >
> > Thank you for the response.
> >
> > I'm using SabreLite with SD card.
> >
> > For reproducing a problem I took the latest version of U-Boot from master branch and made
> > a couple of changes in include/configs/mx6qsabrelite.h:
> > - removed duplication of "mmc dev ${mmcdev};" in CONFIG_BOOTCOMMAND;
> > - and changed CONFIG_ENV_IS_IN_MMC to CONFIG_ENV_IS_NOWHERE
> > (thus disabling to try reading saved environment from mmc).
> > The boot.scr contains a single command "printenv". When I'm doing resets with this
> > configuration, I'm getting errors in MMC access approximately once per 4-5 boots.
> > The errors can be "Invalid partition 2" or "No partition table".
> >
> > Indeed, I was unable to reproduce it with plain master version, but 2 simple changes in configuration,
> described above, allowed me to reproduce it.
> >
> 
> I think you exposed another bug here.
> 
> I was able to to reproduce the symptom (failure reading from
> filesystem) on master by simply setting my bootcmd to this:
>         mmc dev 0
>         && fatload mmc 0 10800000 uImage
>         && fatload mmc 0 12800000 uramdisk.img
>         && bootm 10800000 12800000
> 
> The failures differed somewhat from boot to boot,
> but this was typical:
> 
>         reading uImage
>         4350916 bytes read in 216 ms (19.2 MiB/s)
>         bad MBR sector signature 0x0000
>         ** Invalid partition 1 **
> 
> I've seen weirdness in the past with cache enabled (as is the
> case in the mx6qsabrelite_config), so I re-tested with
> a 'dcache off' in bootcmd:
> 
>         dcache off ;
>         mmc dev 0
>         && fatload mmc 0 10800000 uImage
>         && fatload mmc 0 12800000 uramdisk.img
>         && bootm 10800000 12800000
> 
> This appeared to fix the issue (through 100 boots).
> 
> 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).
> 
> Dirk, this was on a TO1.2 device, so I don't think
> this was caused by the out-of-order completion
> interrupts.
> 
> Andrew, can you test with this patch to see if
> it also addresses the issue?
> 
> By inspection, it seems needed.
> 
> Regards,
> 
> Eric

Hi Eric,

Yes, this patch seems to fix the issue too.

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.

Thanks.

Best regards,
Andrew

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

[Eric Nelson]

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

[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

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

[Eric Nelson]

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.

Fabio, can you comment on this?

The code appears to have been like this since the beginning
of main-line support for i.MX so there's no history to go on.

The same goes in Freescale's git tree.

Regards,


Eric

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

[Eric Nelson]

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.

[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

[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.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?
>

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] [PATCH] mx6: fsl_esdhc: Fix waiting for DMA operation completion

[Fleming Andy-AFLEMING]

On Apr 4, 2013, at 1:41 PM, Eric Nelson wrote:

> 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
>>> 
>> 
>> 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));


That looks right to me. I have been known to mistakenly write loops that are supposed to wait for two conditions to only wait for one of those. Apparently I need remedial boolean logic lessons.


> 
> 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


I don't think we need a special case. Just correct logic. :/

Andy

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

[Eric Nelson]

Thanks for the review Andy.

On 04/05/2013 01:18 PM, Fleming Andy-AFLEMING wrote:
>
> On Apr 4, 2013, at 1:41 PM, Eric Nelson wrote:
>
>> 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
>>>>
>>>
>>> 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));
>
> That looks right to me. I have been known to mistakenly write loops
> that are supposed to wait for two conditions to only wait for one of
> those. Apparently I need remedial boolean logic lessons.
>
>
>>
>> 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
>
> I don't think we need a special case. Just correct logic. :/
>

Cool. It's always hard to tell when IP like this is used for
multiple processors.

So many data sheets, so little time...

Regards,


Eric

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

[Gabbasov, Andrew]

> From: Fleming Andy-AFLEMING [afleming at freescale.com]
> Sent: Saturday, April 06, 2013 00:18
> To: Eric Nelson
> Cc: Gabbasov, Andrew; u-boot at lists.denx.de; Behme, Dirk - Bosch; Estevam Fabio-R49496
> Subject: Re: [U-Boot] [PATCH] mx6: fsl_esdhc: Fix waiting for DMA operation completion
> 
> On Apr 4, 2013, at 1:41 PM, Eric Nelson wrote:
> 
> > 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
> >>>
> >>
> >> 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));
> 
> 
> That looks right to me. I have been known to mistakenly write loops that are supposed to wait for two conditions to only wait for one of those. Apparently I need remedial boolean logic lessons.
> 
> 
> >
> > 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
> 
> 
> I don't think we need a special case. Just correct logic. :/
> 
> Andy

Hi Andy, Eric,

Thanks for the review!

I've submitted a new patch ("[U-Boot] [Patch] fsl_esdhc: Fix DMA transfer completion waiting loop"),
that reworks this loop condition. I've changed the patch subject to better reflect the fix contents,
and since it no longer relates mx6 only. So, it becomes not the V2 of this patch, but a new one.

Please, let me know if there are any problems with the new patch.

Thanks.

Best regards,
Andrew

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

[Fabio Estevam]

Hi Eric,

On Wed, Apr 3, 2013 at 8:17 PM, Eric Nelson
<eric.nelson@boundarydevices.com> wrote:

>> 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.
>
> Fabio, can you comment on this?

I am not very familiar with the mmc driver. Adding Andy in case he has
some insight about it.

>
> The code appears to have been like this since the beginning
> of main-line support for i.MX so there's no history to go on.
>
> The same goes in Freescale's git tree.

Do you mean FSL U-boot or kernel tree? Is it the same with the
mainline kernel driver?

Regards,

Fabio Estevam

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

[Fleming Andy-AFLEMING]

On Apr 4, 2013, at 13:12, "Fabio Estevam" <festevam@gmail.com> wrote:

> Hi Eric,
> 
> On Wed, Apr 3, 2013 at 8:17 PM, Eric Nelson
> <eric.nelson@boundarydevices.com> wrote:
> 
>>> 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.
>> 
>> Fabio, can you comment on this?
> 
> I am not very familiar with the mmc driver. Adding Andy in case he has
> some insight about it.


Hmm, that does seem weird. I'll look into it.


> 
>> 
>> The code appears to have been like this since the beginning
>> of main-line support for i.MX so there's no history to go on.
>> 
>> The same goes in Freescale's git tree.
> 
> Do you mean FSL U-boot or kernel tree? Is it the same with the
> mainline kernel driver?
> 
> Regards,
> 
> Fabio Estevam
>