| Message ID | 1416551950-8277-1-git-send-email-shawn.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi, Shawn. On 11/21/2014 03:39 PM, Shawn Guo wrote: > Commit f1d2736c8156 ("mmc: dw_mmc: control card read threshold") > introduces a regression for use case where PIO mode is used, i.e. > CONFIG_MMC_DW_IDMAC is not disabled. It stops kernel from booting > like below. > > dw_mmc 9408000.dwmmc: fifo-depth property not found, using value of FIFOTH register as default > dw_mmc 9408000.dwmmc: Using PIO mode. > dw_mmc 9408000.dwmmc: Version ID is 210a > dw_mmc 9408000.dwmmc: DW MMC controller at irq 72, 32 bit host data width, 32 deep fifo > dw_mmc 9408000.dwmmc: 1 slots initialized > ... > Waiting for root device /dev/mmcblk0p2... > mmc_host mmc0: Bus speed (slot 0) = 52000000Hz (slot req 50000000Hz, actual 26000000HZ div = 1) > mmc0: new high speed SDHC card at address b368 > mmcblk0: mmc0:b368 AF UD 3.84 GiB Stopping at here? I didn't know what's difference before applied your patch. Clearing Card-read threshold value at every time would be caused that stop kernel? Just wonder it. > > Rather than clearing read threshold bits, the best thing that function > dw_mci_ctrl_rd_thld() should do is leaving the bits untouched, in case > that card read threshold setup is not needed. > > The patch fixes the regression by changing dw_mci_ctrl_rd_thld() a bit > to do nothing in case card read threshold bits setup is not needed. > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > Fixes: f1d2736c8156 ("mmc: dw_mmc: control card read threshold") > Cc: <stable@vger.kernel.org> # 3.13+ > --- > drivers/mmc/host/dw_mmc.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 69f0cc68d5b2..52c04ba69970 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -628,13 +628,13 @@ static void dw_mci_ctrl_rd_thld(struct dw_mci *host, struct mmc_data *data) > > if (host->timing != MMC_TIMING_MMC_HS200 && > host->timing != MMC_TIMING_UHS_SDR104) > - goto disable; > + return; > > blksz_depth = blksz / (1 << host->data_shift); > fifo_depth = host->fifo_depth; > > if (blksz_depth > fifo_depth) > - goto disable; > + return; > > /* > * If (blksz_depth) >= (fifo_depth >> 1), should be 'thld_size <= blksz' > @@ -644,9 +644,6 @@ static void dw_mci_ctrl_rd_thld(struct dw_mci *host, struct mmc_data *data) > thld_size = blksz; > mci_writel(host, CDTHRCTL, SDMMC_SET_RD_THLD(thld_size, 1)); > return; Then "return" could be also removed. Best Regards, Jaehoon Chung > - > -disable: > - mci_writel(host, CDTHRCTL, SDMMC_SET_RD_THLD(0, 0)); > } > > static int dw_mci_submit_data_dma(struct dw_mci *host, struct mmc_data *data) > -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jaehoon, On Fri, Nov 21, 2014 at 07:30:50PM +0900, Jaehoon Chung wrote: > Hi, Shawn. > > On 11/21/2014 03:39 PM, Shawn Guo wrote: > > Commit f1d2736c8156 ("mmc: dw_mmc: control card read threshold") > > introduces a regression for use case where PIO mode is used, i.e. > > CONFIG_MMC_DW_IDMAC is not disabled. It stops kernel from booting > > like below. > > > > dw_mmc 9408000.dwmmc: fifo-depth property not found, using value of FIFOTH register as default > > dw_mmc 9408000.dwmmc: Using PIO mode. > > dw_mmc 9408000.dwmmc: Version ID is 210a > > dw_mmc 9408000.dwmmc: DW MMC controller at irq 72, 32 bit host data width, 32 deep fifo > > dw_mmc 9408000.dwmmc: 1 slots initialized > > ... > > Waiting for root device /dev/mmcblk0p2... > > mmc_host mmc0: Bus speed (slot 0) = 52000000Hz (slot req 50000000Hz, actual 26000000HZ div = 1) > > mmc0: new high speed SDHC card at address b368 > > mmcblk0: mmc0:b368 AF UD 3.84 GiB > > Stopping at here? I didn't know what's difference before applied your patch. > Clearing Card-read threshold value at every time would be caused that stop kernel? > Just wonder it. With my patch, the kernel will continue to mount the rootfs on SD card as below. mmcblk0: mmc0:b368 AF UD 3.84 GiB mmcblk0: p1 p2 EXT4-fs (mmcblk0p2): couldn't mount as ext3 due to feature incompatibilities EXT4-fs (mmcblk0p2): couldn't mount as ext2 due to feature incompatibilities EXT4-fs (mmcblk0p2): INFO: recovery required on readonly filesystem EXT4-fs (mmcblk0p2): write access will be enabled during recovery random: nonblocking pool is initialized EXT4-fs (mmcblk0p2): recovery complete EXT4-fs (mmcblk0p2): mounted filesystem with ordered data mode. Opts: (null) VFS: Mounted root (ext4 filesystem) readonly on device 179:2. > > > > > Rather than clearing read threshold bits, the best thing that function > > dw_mci_ctrl_rd_thld() should do is leaving the bits untouched, in case > > that card read threshold setup is not needed. > > > > The patch fixes the regression by changing dw_mci_ctrl_rd_thld() a bit > > to do nothing in case card read threshold bits setup is not needed. > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > > Fixes: f1d2736c8156 ("mmc: dw_mmc: control card read threshold") > > Cc: <stable@vger.kernel.org> # 3.13+ > > --- > > drivers/mmc/host/dw_mmc.c | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > > index 69f0cc68d5b2..52c04ba69970 100644 > > --- a/drivers/mmc/host/dw_mmc.c > > +++ b/drivers/mmc/host/dw_mmc.c > > @@ -628,13 +628,13 @@ static void dw_mci_ctrl_rd_thld(struct dw_mci *host, struct mmc_data *data) > > > > if (host->timing != MMC_TIMING_MMC_HS200 && > > host->timing != MMC_TIMING_UHS_SDR104) > > - goto disable; > > + return; > > > > blksz_depth = blksz / (1 << host->data_shift); > > fifo_depth = host->fifo_depth; > > > > if (blksz_depth > fifo_depth) > > - goto disable; > > + return; > > > > /* > > * If (blksz_depth) >= (fifo_depth >> 1), should be 'thld_size <= blksz' > > @@ -644,9 +644,6 @@ static void dw_mci_ctrl_rd_thld(struct dw_mci *host, struct mmc_data *data) > > thld_size = blksz; > > mci_writel(host, CDTHRCTL, SDMMC_SET_RD_THLD(thld_size, 1)); > > return; > > Then "return" could be also removed. You're right. Will remove it in v2. Shawn -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 21, 2014 at 02:39:10PM +0800, Shawn Guo wrote: > Commit f1d2736c8156 ("mmc: dw_mmc: control card read threshold") > introduces a regression for use case where PIO mode is used, i.e. > CONFIG_MMC_DW_IDMAC is not disabled. It stops kernel from booting s/disabled/enabled > like below. To make it clear, I will reword the sentence as below. It makes kernel fail to detect the partitions on SD/MMC device where rootfs is located. Shawn > > dw_mmc 9408000.dwmmc: fifo-depth property not found, using value of FIFOTH register as default > dw_mmc 9408000.dwmmc: Using PIO mode. > dw_mmc 9408000.dwmmc: Version ID is 210a > dw_mmc 9408000.dwmmc: DW MMC controller at irq 72, 32 bit host data width, 32 deep fifo > dw_mmc 9408000.dwmmc: 1 slots initialized > ... > Waiting for root device /dev/mmcblk0p2... > mmc_host mmc0: Bus speed (slot 0) = 52000000Hz (slot req 50000000Hz, actual 26000000HZ div = 1) > mmc0: new high speed SDHC card at address b368 > mmcblk0: mmc0:b368 AF UD 3.84 GiB > > Rather than clearing read threshold bits, the best thing that function > dw_mci_ctrl_rd_thld() should do is leaving the bits untouched, in case > that card read threshold setup is not needed. > > The patch fixes the regression by changing dw_mci_ctrl_rd_thld() a bit > to do nothing in case card read threshold bits setup is not needed. > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > Fixes: f1d2736c8156 ("mmc: dw_mmc: control card read threshold") > Cc: <stable@vger.kernel.org> # 3.13+ > --- > drivers/mmc/host/dw_mmc.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 69f0cc68d5b2..52c04ba69970 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -628,13 +628,13 @@ static void dw_mci_ctrl_rd_thld(struct dw_mci *host, struct mmc_data *data) > > if (host->timing != MMC_TIMING_MMC_HS200 && > host->timing != MMC_TIMING_UHS_SDR104) > - goto disable; > + return; > > blksz_depth = blksz / (1 << host->data_shift); > fifo_depth = host->fifo_depth; > > if (blksz_depth > fifo_depth) > - goto disable; > + return; > > /* > * If (blksz_depth) >= (fifo_depth >> 1), should be 'thld_size <= blksz' > @@ -644,9 +644,6 @@ static void dw_mci_ctrl_rd_thld(struct dw_mci *host, struct mmc_data *data) > thld_size = blksz; > mci_writel(host, CDTHRCTL, SDMMC_SET_RD_THLD(thld_size, 1)); > return; > - > -disable: > - mci_writel(host, CDTHRCTL, SDMMC_SET_RD_THLD(0, 0)); > } > > static int dw_mci_submit_data_dma(struct dw_mci *host, struct mmc_data *data) > -- > 1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Seungwon, Jaehoon, On Fri, Nov 21, 2014 at 02:39:10PM +0800, Shawn Guo wrote: > Commit f1d2736c8156 ("mmc: dw_mmc: control card read threshold") > introduces a regression for use case where PIO mode is used, i.e. > CONFIG_MMC_DW_IDMAC is not disabled. It stops kernel from booting s/not disabled/disabled > like below. For understanding the problem better, can you guys please to test the dw_mmc driver on Samsung platforms with CONFIG_MMC_DW_IDMAC disabled. I'm wondering this is a problem specific to my platform or a common one. Shawn > > dw_mmc 9408000.dwmmc: fifo-depth property not found, using value of FIFOTH register as default > dw_mmc 9408000.dwmmc: Using PIO mode. > dw_mmc 9408000.dwmmc: Version ID is 210a > dw_mmc 9408000.dwmmc: DW MMC controller at irq 72, 32 bit host data width, 32 deep fifo > dw_mmc 9408000.dwmmc: 1 slots initialized > ... > Waiting for root device /dev/mmcblk0p2... > mmc_host mmc0: Bus speed (slot 0) = 52000000Hz (slot req 50000000Hz, actual 26000000HZ div = 1) > mmc0: new high speed SDHC card at address b368 > mmcblk0: mmc0:b368 AF UD 3.84 GiB > > Rather than clearing read threshold bits, the best thing that function > dw_mci_ctrl_rd_thld() should do is leaving the bits untouched, in case > that card read threshold setup is not needed. > > The patch fixes the regression by changing dw_mci_ctrl_rd_thld() a bit > to do nothing in case card read threshold bits setup is not needed. > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > Fixes: f1d2736c8156 ("mmc: dw_mmc: control card read threshold") > Cc: <stable@vger.kernel.org> # 3.13+ > --- > drivers/mmc/host/dw_mmc.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 69f0cc68d5b2..52c04ba69970 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -628,13 +628,13 @@ static void dw_mci_ctrl_rd_thld(struct dw_mci *host, struct mmc_data *data) > > if (host->timing != MMC_TIMING_MMC_HS200 && > host->timing != MMC_TIMING_UHS_SDR104) > - goto disable; > + return; > > blksz_depth = blksz / (1 << host->data_shift); > fifo_depth = host->fifo_depth; > > if (blksz_depth > fifo_depth) > - goto disable; > + return; > > /* > * If (blksz_depth) >= (fifo_depth >> 1), should be 'thld_size <= blksz' > @@ -644,9 +644,6 @@ static void dw_mci_ctrl_rd_thld(struct dw_mci *host, struct mmc_data *data) > thld_size = blksz; > mci_writel(host, CDTHRCTL, SDMMC_SET_RD_THLD(thld_size, 1)); > return; > - > -disable: > - mci_writel(host, CDTHRCTL, SDMMC_SET_RD_THLD(0, 0)); > } > > static int dw_mci_submit_data_dma(struct dw_mci *host, struct mmc_data *data) > -- > 1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alim, On Sat, Nov 22, 2014 at 06:56:46PM +0530, Alim Akhtar wrote: > Hi Shawn, > > On Sat, Nov 22, 2014 at 5:01 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > > Seungwon, Jaehoon, > > > > On Fri, Nov 21, 2014 at 02:39:10PM +0800, Shawn Guo wrote: > >> Commit f1d2736c8156 ("mmc: dw_mmc: control card read threshold") > >> introduces a regression for use case where PIO mode is used, i.e. > >> CONFIG_MMC_DW_IDMAC is not disabled. It stops kernel from booting > > > > s/not disabled/disabled > > > >> like below. > > > > For understanding the problem better, can you guys please to test the > > dw_mmc driver on Samsung platforms with CONFIG_MMC_DW_IDMAC disabled. > > I'm wondering this is a problem specific to my platform or a common one. > > > I tested PIO mode on exynos5800-peach-pi board with Ulf's -next > branch, and it works fine. > To confirm PIO mode works, I ran iozone and it gave low performance > number (as expected) then IDMA mode. Thanks a lot for testing and confirming that it works on exynos. > > One thing to note, is your controller version is 210a, so not sure if > something more in missing from driver. Unfortunately, I do not have the reference manual for the hardware I'm running so far. But from the kernel log, the controller version is 210a. What do you mean by "something more in missing from driver"? The driver has any limitation for any particular controller versions? One thing I need to check with hardware vendor is whether register CDTHRCTL is available on my dwmmc controller. From my testing, even reading this register every time dw_mci_submit_data() is called causes problem for me. > Also from log its looks like you card clock is 26Mhz, did you tried > running at 52Mhz? I did not. But it's unlikely the problem of clock frequency from what I've seen. Shawn -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 24, 2014 at 03:36:13AM +0530, Alim Akhtar wrote: > Hi Shawn, > > On Sun, Nov 23, 2014 at 9:00 PM, Shawn Guo <shawn.guo@linaro.org> wrote: > > One thing I need to check with hardware vendor is whether register > > CDTHRCTL is available on my dwmmc controller. From my testing, even > > reading this register every time dw_mci_submit_data() is called causes > > problem for me. > > > yes, your guess is right, did you tested with ulf's -next branch? > Recently patch [1] is merged, that most likely to solve your platform > issue. Have a try. > > [1] http://www.spinics.net/lists/stable/msg69884.html Yeah, the patch fixes my problem. Thanks a lot for the pointer, Alim. Shawn -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index 69f0cc68d5b2..52c04ba69970 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -628,13 +628,13 @@ static void dw_mci_ctrl_rd_thld(struct dw_mci *host, struct mmc_data *data) if (host->timing != MMC_TIMING_MMC_HS200 && host->timing != MMC_TIMING_UHS_SDR104) - goto disable; + return; blksz_depth = blksz / (1 << host->data_shift); fifo_depth = host->fifo_depth; if (blksz_depth > fifo_depth) - goto disable; + return; /* * If (blksz_depth) >= (fifo_depth >> 1), should be 'thld_size <= blksz' @@ -644,9 +644,6 @@ static void dw_mci_ctrl_rd_thld(struct dw_mci *host, struct mmc_data *data) thld_size = blksz; mci_writel(host, CDTHRCTL, SDMMC_SET_RD_THLD(thld_size, 1)); return; - -disable: - mci_writel(host, CDTHRCTL, SDMMC_SET_RD_THLD(0, 0)); } static int dw_mci_submit_data_dma(struct dw_mci *host, struct mmc_data *data)
Commit f1d2736c8156 ("mmc: dw_mmc: control card read threshold") introduces a regression for use case where PIO mode is used, i.e. CONFIG_MMC_DW_IDMAC is not disabled. It stops kernel from booting like below. dw_mmc 9408000.dwmmc: fifo-depth property not found, using value of FIFOTH register as default dw_mmc 9408000.dwmmc: Using PIO mode. dw_mmc 9408000.dwmmc: Version ID is 210a dw_mmc 9408000.dwmmc: DW MMC controller at irq 72, 32 bit host data width, 32 deep fifo dw_mmc 9408000.dwmmc: 1 slots initialized ... Waiting for root device /dev/mmcblk0p2... mmc_host mmc0: Bus speed (slot 0) = 52000000Hz (slot req 50000000Hz, actual 26000000HZ div = 1) mmc0: new high speed SDHC card at address b368 mmcblk0: mmc0:b368 AF UD 3.84 GiB Rather than clearing read threshold bits, the best thing that function dw_mci_ctrl_rd_thld() should do is leaving the bits untouched, in case that card read threshold setup is not needed. The patch fixes the regression by changing dw_mci_ctrl_rd_thld() a bit to do nothing in case card read threshold bits setup is not needed. Signed-off-by: Shawn Guo <shawn.guo@linaro.org> Fixes: f1d2736c8156 ("mmc: dw_mmc: control card read threshold") Cc: <stable@vger.kernel.org> # 3.13+ --- drivers/mmc/host/dw_mmc.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)