| Message ID | 1369764582-13888-1-git-send-email-fabio.estevam@freescale.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | Andy Fleming |
| Headers | show |
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?
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.
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
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
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
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
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?
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.
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.
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
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;
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(-)