Patchwork [U-Boot] mmc: fsl_esdhc: Fix hang after 'save' command

login
register
mail settings
Submitter Fabio Estevam
Date May 28, 2013, 6:09 p.m.
Message ID <1369764582-13888-1-git-send-email-fabio.estevam@freescale.com>
Download mbox | patch
Permalink /patch/246928/
State Accepted, archived
Delegated to: Andy Fleming
Headers show

Comments

Fabio Estevam - May 28, 2013, 6:09 p.m.
Since commit 48e0b2bd (powerpc/esdhc: Correct judgement for DATA PIO mode) 
we see mx6 systems to hang after doing a 'save' command.

Revert this commit since the original 'ifdef' logic from 7b43db92 
(drivers/mmc/fsl_esdhc.c: fix compiler warnings) was the correct one.

Reported-by: Tapani Utriainen <tapani@technexion.com>
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 drivers/mmc/fsl_esdhc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Otavio Salvador - May 28, 2013, 6:31 p.m.
On Tue, May 28, 2013 at 3:09 PM, Fabio Estevam
<fabio.estevam@freescale.com>wrote:

> Since commit 48e0b2bd (powerpc/esdhc: Correct judgement for DATA PIO mode)
> we see mx6 systems to hang after doing a 'save' command.
>
> Revert this commit since the original 'ifdef' logic from 7b43db92
> (drivers/mmc/fsl_esdhc.c: fix compiler warnings) was the correct one.
>
> Reported-by: Tapani Utriainen <tapani@technexion.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
>

This seem to affect 'master' branch, not 'imx' tree. Is that correct?
Fabio Estevam - May 28, 2013, 6:37 p.m.
On Tue, May 28, 2013 at 3:31 PM, Otavio Salvador
<otavio@ossystems.com.br> wrote:

> This seem to affect 'master' branch, not 'imx' tree. Is that correct?

Yes, the offending commit is in master, but not on u-boot-imx branch.
Andy Fleming - May 28, 2013, 7:51 p.m.
On May 28, 2013, at 1:09 PM, Fabio Estevam wrote:

> Since commit 48e0b2bd (powerpc/esdhc: Correct judgement for DATA PIO mode) 
> we see mx6 systems to hang after doing a 'save' command.
> 
> Revert this commit since the original 'ifdef' logic from 7b43db92 
> (drivers/mmc/fsl_esdhc.c: fix compiler warnings) was the correct one.
> 
> Reported-by: Tapani Utriainen <tapani@technexion.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>


My apologies. I misread that patch. I found it hard to believe that Wolfgang had broken the logic, and that it had been broken for 2 years, but somehow my brain insisted that was the case. I now agree with your assessment, and will apply your patch.

Haijun, please investigate the problem you were trying to solve with this patch, and determine what the actual cause was, and then submit a new patch to fix it.


Andy
Andy Fleming - June 13, 2013, 10:36 p.m.
On Tue, May 28, 2013 at 03:09:42PM -0300, Fabio Estevam wrote:
> Since commit 48e0b2bd (powerpc/esdhc: Correct judgement for DATA PIO mode) 
> we see mx6 systems to hang after doing a 'save' command.
> 
> Revert this commit since the original 'ifdef' logic from 7b43db92 
> (drivers/mmc/fsl_esdhc.c: fix compiler warnings) was the correct one.
> 
> Reported-by: Tapani Utriainen <tapani@technexion.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>


Applied, thanks.

Andy
Zhang Haijun-B42677 - June 27, 2013, 9:50 a.m.
Hi, Fleming and Fabio

Sorry to reply to you so late.

As our manual described.

In the eSDHC, the data buffer can hold up to 128 words (32-bit).

The watermark levels for both write and read can be configured for CPU polling mode.
The watermark level can be from 1 word to the maximum of 128 words. 

For both DMA read and write, the burst length, can be configured from 1 to the maximum of 16.

The host driver may configure watermark level and burst length value according to the system
situation and requirement in the watermark level register (WML).


So if we define CONFIG_SYS_FSL_ESDHC_USE_PIO, this should be needed for CPU Polling mode.
My patch works  well on my board both for DMA mod and CPU Polling mode.

> > Since commit 48e0b2bd (powerpc/esdhc: Correct judgement for DATA PIO
> > mode) we see mx6 systems to hang after doing a 'save' command.

Fabio, Did you boot up from sdcard? If so save command will write to SD card. In this case write command will be failed.
I'm not familiar with mx6 system. I don't know if there are some difference in Watermark Level Register (eSDHC_WML).


Regards
Haijun.


> -----Original Message-----
> From: Fleming Andy-AFLEMING
> Sent: Wednesday, May 29, 2013 3:52 AM
> To: Estevam Fabio-R49496
> Cc: Zhang Haijun-B42677; <wd@denx.de>; <u-boot@lists.denx.de>;
> <sbabic@denx.de>; <tapani@technexion.com>
> Subject: Re: [PATCH] mmc: fsl_esdhc: Fix hang after 'save' command
> 
> 
> On May 28, 2013, at 1:09 PM, Fabio Estevam wrote:
> 
> > Since commit 48e0b2bd (powerpc/esdhc: Correct judgement for DATA PIO
> > mode) we see mx6 systems to hang after doing a 'save' command.
> >
> > Revert this commit since the original 'ifdef' logic from 7b43db92
> > (drivers/mmc/fsl_esdhc.c: fix compiler warnings) was the correct one.
> >
> > Reported-by: Tapani Utriainen <tapani@technexion.com>
> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> 
> 
> My apologies. I misread that patch. I found it hard to believe that
> Wolfgang had broken the logic, and that it had been broken for 2 years,
> but somehow my brain insisted that was the case. I now agree with your
> assessment, and will apply your patch.
> 
> Haijun, please investigate the problem you were trying to solve with this
> patch, and determine what the actual cause was, and then submit a new
> patch to fix it.
> 
> 
> Andy
Andy Fleming - June 27, 2013, 7:04 p.m.
On Thu, Jun 27, 2013 at 4:50 AM, Zhang Haijun-B42677
<B42677@freescale.com>wrote:

> Hi, Fleming and Fabio
>
> Sorry to reply to you so late.
>
> As our manual described.
>
> In the eSDHC, the data buffer can hold up to 128 words (32-bit).
>
> The watermark levels for both write and read can be configured for CPU
> polling mode.
> The watermark level can be from 1 word to the maximum of 128 words.
>
> For both DMA read and write, the burst length, can be configured from 1 to
> the maximum of 16.
>
> The host driver may configure watermark level and burst length value
> according to the system
> situation and requirement in the watermark level register (WML).
>


I think you may have misunderstood the WML register. My read of the manual
indicates it is most definitely necessary for DMA. I think it is also
necessary for polling mode, and WML configuration was not added properly
when polling mode support was added to the driver. The solution may be to
set up WML for both modes, but the manual is a bit unclear (it sometimes
likes to refer to polling mode as "External DMA").

Andy
Fabio Estevam - June 27, 2013, 11:55 p.m.
On Thu, Jun 27, 2013 at 6:50 AM, Zhang Haijun-B42677
<B42677@freescale.com> wrote:
>
> Fabio, Did you boot up from sdcard? If so save command will write to SD card. In this case write command will be failed.
> I'm not familiar with mx6 system. I don't know if there are some difference in Watermark Level Register (eSDHC_WML).

Yes, I boot from sd card.

Can you try removing CONFIG_SYS_FSL_ESDHC_USE_PIO from your board file?
Fabio Estevam - June 28, 2013, 2:01 a.m.
On Thu, Jun 27, 2013 at 11:00 PM, Zhang Haijun-B42677
<B42677@freescale.com> wrote:
> My board use DMA to transfer data by default. So we don't use CONFIG_SYS_FSL_ESDHC_USE_PIO by default.
>
> Also my board works fine with PIO mode(with my patch).
>
> What about your board? Using PIO?

We use DMA on i.MX.
Fabio Estevam - June 28, 2013, 2:11 a.m.
On Thu, Jun 27, 2013 at 11:06 PM, Zhang Haijun-B42677
<B42677@freescale.com> wrote:
> How about the read write command and erase command in your uboot with my patch?
>
> I mean this:
> mmcinfo
> mmc read addr blk# cnt
> mmc write addr blk# cnt
> mmc erase blk# cnt
> mmc rescan
> mmc part - lists available partition on current mmc device
> mmc dev [dev] [part] - show or set current mmc device [partition]
> mmc list - lists available devices

I don't have the board handy to try, but your patch introduced a bug
by inverting the ifdef logic from commit 7b43db92.

And you never explained why you need 48e0b2bd, which board shows the
issue, what is the issue, etc.
Andy Fleming - June 28, 2013, 8:45 p.m.
On Thu, Jun 27, 2013 at 9:33 PM, Zhang Haijun-B42677
<B42677@freescale.com>wrote:

>  Hi, Fleming****
>
> ** **
>
> I found the root cause.****
>
> Our driver didn’t make a distinction between different board for this
> register. (Just distinguish by esdhc version)****
>
> On p1025 board watermark level was used both for watermark level and burst
> length.
>


Be careful in such descriptions. It's the SoC that defines things
differently, not the board. P1025 is a chip (SoC). P1025RDB is a board. All
P1025s have this difference.

esdhc version should be enough to distinguish between these two. 3041/4240
use esdhc 2.3, while 1025 still uses 2.2. Clearly, we need to add support
for distinguishing between those two.

However, what is also clear is that the code, as it is, is currently
broken. WML needs to be properly configured under *both* polling and DMA
modes, and *that* is the change that needs to be made.

Andy

Patch

diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
index 861f4b9..ec01795 100644
--- a/drivers/mmc/fsl_esdhc.c
+++ b/drivers/mmc/fsl_esdhc.c
@@ -178,7 +178,7 @@  static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data)
 	int timeout;
 	struct fsl_esdhc_cfg *cfg = (struct fsl_esdhc_cfg *)mmc->priv;
 	struct fsl_esdhc *regs = (struct fsl_esdhc *)cfg->esdhc_base;
-#ifdef CONFIG_SYS_FSL_ESDHC_USE_PIO
+#ifndef CONFIG_SYS_FSL_ESDHC_USE_PIO
 	uint wml_value;
 
 	wml_value = data->blocksize/4;