[PATCH] mmc: dw_mmc: Reset DMA before enabling IDMAC

[Sonny Rao]

We've already got a reset of DMA after it's done. Add one before we
start DMA too. This fixes a data corruption on Rockchip SoCs which
will get bad data when doing a DMA transfer after doing a PIO transfer.

We tested this on an Exynos 5800 with HS200 and didn't notice any
difference in sequential read throughput.

Signed-off-by: Sonny Rao <***@chromium.org>
Signed-off-by: Doug Anderson <***@chromium.org>
---
drivers/mmc/host/dw_mmc.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 69f0cc6..2b5401e 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -83,6 +83,7 @@ struct idmac_desc {
#endif /* CONFIG_MMC_DW_IDMAC */

static bool dw_mci_reset(struct dw_mci *host);
+static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset);

#if defined(CONFIG_DEBUG_FS)
static int dw_mci_req_show(struct seq_file *s, void *v)
@@ -448,6 +449,9 @@ static void dw_mci_idmac_start_dma(struct dw_mci *host, unsigned int sg_len)

dw_mci_translate_sglist(host, host->data, sg_len);

+ /* Make sure to reset DMA in case we did PIO before this */
+ dw_mci_ctrl_reset(host, SDMMC_CTRL_DMA_RESET);
+
/* Select IDMAC interface */
temp = mci_readl(host, CTRL);
temp |= SDMMC_CTRL_USE_IDMAC;

[Doug Anderson]

Sonny,

Post by Sonny Rao
We've already got a reset of DMA after it's done. Add one before we
start DMA too. This fixes a data corruption on Rockchip SoCs which
will get bad data when doing a DMA transfer after doing a PIO transfer.
We tested this on an Exynos 5800 with HS200 and didn't notice any
difference in sequential read throughput.
---
drivers/mmc/host/dw_mmc.c | 4 ++++
1 file changed, 4 insertions(+)

Reviewed-by: Doug Anderson <***@chromium.org>
Tested-by: Doug Anderson <***@chromium.org>

[Jaehoon Chung]

Post by Doug Anderson
Sonny,

Post by Sonny Rao
We've already got a reset of DMA after it's done. Add one before we
start DMA too. This fixes a data corruption on Rockchip SoCs which
will get bad data when doing a DMA transfer after doing a PIO transfer.
We tested this on an Exynos 5800 with HS200 and didn't notice any
difference in sequential read throughput.
---
drivers/mmc/host/dw_mmc.c | 4 ++++
1 file changed, 4 insertions(+)

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Alim Akhtar]

Hi Sonny/Doug,

Post by Sonny Rao
We've already got a reset of DMA after it's done. Add one before we
start DMA too. This fixes a data corruption on Rockchip SoCs which
will get bad data when doing a DMA transfer after doing a PIO transfer.
We tested this on an Exynos 5800 with HS200 and didn't notice any
difference in sequential read throughput.
---
drivers/mmc/host/dw_mmc.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 69f0cc6..2b5401e 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -83,6 +83,7 @@ struct idmac_desc {
#endif /* CONFIG_MMC_DW_IDMAC */
static bool dw_mci_reset(struct dw_mci *host);
+static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset);
#if defined(CONFIG_DEBUG_FS)
static int dw_mci_req_show(struct seq_file *s, void *v)
@@ -448,6 +449,9 @@ static void dw_mci_idmac_start_dma(struct dw_mci *host, unsigned int sg_len)
dw_mci_translate_sglist(host, host->data, sg_len);
+ /* Make sure to reset DMA in case we did PIO before this */
+ dw_mci_ctrl_reset(host, SDMMC_CTRL_DMA_RESET);
+

Though this is good to do, but this does not look complete to me.
dw_mmc data book does mention that " It is recommended that the host
issue reset to DMA interface by setting DMA_RESET bit of the CTRL
register and then issue a IDMAC software reset."
The above lines are from 'Transmission and reception with internal
DMA' section of the data book.
My suggestion here to add dw_mci_idmac_reset() call after this above change.

What is the controller version used in your case?

Post by Sonny Rao
/* Select IDMAC interface */
temp = mci_readl(host, CTRL);
temp |= SDMMC_CTRL_USE_IDMAC;
--
1.8.3.2
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Alim Akhtar]

Hi Eddie,

On Thu, Oct 9, 2014 at 7:43 PM, Eddie Cai(=E8=94=A1=E6=9E=AB) <eddie.ca=

Hi Alim
2014=E5=B9=B410=E6=9C=888=E6=97=A5 =E4=B8=8A=E5=8D=884:28=E4=BA=8E "A=

Post by Alim Akhtar
Hi Sonny/Doug,

We've already got a reset of DMA after it's done. Add one before =

we

Post by Alim Akhtar

start DMA too. This fixes a data corruption on Rockchip SoCs whic=

h

Post by Alim Akhtar

will get bad data when doing a DMA transfer after doing a PIO tran=

sfer.

Post by Alim Akhtar

We tested this on an Exynos 5800 with HS200 and didn't notice any
difference in sequential read throughput.
---
drivers/mmc/host/dw_mmc.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 69f0cc6..2b5401e 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -83,6 +83,7 @@ struct idmac_desc {
#endif /* CONFIG_MMC_DW_IDMAC */
static bool dw_mci_reset(struct dw_mci *host);
+static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset);
#if defined(CONFIG_DEBUG_FS)
static int dw_mci_req_show(struct seq_file *s, void *v)
@@ -448,6 +449,9 @@ static void dw_mci_idmac_start_dma(struct dw_m=

ci

Post by Alim Akhtar

*host, unsigned int sg_len)
dw_mci_translate_sglist(host, host->data, sg_len);
+ /* Make sure to reset DMA in case we did PIO before this *=

/

Post by Alim Akhtar

+ dw_mci_ctrl_reset(host, SDMMC_CTRL_DMA_RESET);
+

Though this is good to do, but this does not look complete to me.
dw_mmc data book does mention that " It is recommended that the host
issue reset to DMA interface by setting DMA_RESET bit of the CTRL
register and then issue a IDMAC software reset."
The above lines are from 'Transmission and reception with internal
DMA' section of the data book.
My suggestion here to add dw_mci_idmac_reset() call after this above change.
What is the controller version used in your case?

it just compatible with dw mmc controler but not the same one. it is
designed by rockchip.

Thats fine, I think every vendor (most of them) has a custom
implementation of dw_mmc, but they do have VERID register to check the
dw_mmc version.
The reason why I asked is, I have seen inconsistency in card
enumeration on few controller version, and this patch alone does not
help, and adding a call to dw_mci_idmac_reset() after DMA reset is
needed. And this is what is recommended in the synopsys's data book
also.
Do you see any issue/side effect after adding dw_mci_idmac_reset()?

Post by Alim Akhtar

/* Select IDMAC interface */
temp =3D mci_readl(host, CTRL);
temp |=3D SDMMC_CTRL_USE_IDMAC;
--
1.8.3.2
--
To unsubscribe from this list: send the line "unsubscribe linux-mm=

c" in

Post by Alim Akhtar

More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Regards,
Alim

--=20
Regards,
Alim

[Doug Anderson]

Alim,

Post by Alim Akhtar
Thats fine, I think every vendor (most of them) has a custom
implementation of dw_mmc, but they do have VERID register to check the
dw_mmc version.
The reason why I asked is, I have seen inconsistency in card
enumeration on few controller version, and this patch alone does not
help, and adding a call to dw_mci_idmac_reset() after DMA reset is
needed. And this is what is recommended in the synopsys's data book
also.
Do you see any issue/side effect after adding dw_mci_idmac_reset()?

A quick test shows no problem with adding this to Sonny's patch:

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 66dc8fe..588b5b8 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -481,6 +481,7 @@ static void dw_mci_idmac_start_dma(struct dw_mci
*host, unsigned int sg_len)

/* Make sure to reset DMA in case we did PIO before this */
dw_mci_ctrl_reset(host, SDMMC_CTRL_DMA_RESET);
+ dw_mci_idmac_reset(host);

/* Select IDMAC interface */
temp = mci_readl(host, CTRL);

I'll start reboot tests now to see how it behaves... I think Sonny is
out of the office for a few days so we might need to wait for a spin,
but I'll run with that change in the meantime and see how it behaves
for me.

Thanks!

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Doug Anderson]

Hi,

Post by Sonny Rao
Alim,

Post by Alim Akhtar
Thats fine, I think every vendor (most of them) has a custom
implementation of dw_mmc, but they do have VERID register to check the
dw_mmc version.
The reason why I asked is, I have seen inconsistency in card
enumeration on few controller version, and this patch alone does not
help, and adding a call to dw_mci_idmac_reset() after DMA reset is
needed. And this is what is recommended in the synopsys's data book
also.
Do you see any issue/side effect after adding dw_mci_idmac_reset()?

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 66dc8fe..588b5b8 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -481,6 +481,7 @@ static void dw_mci_idmac_start_dma(struct dw_mci
*host, unsigned int sg_len)
/* Make sure to reset DMA in case we did PIO before this */
dw_mci_ctrl_reset(host, SDMMC_CTRL_DMA_RESET);
+ dw_mci_idmac_reset(host);
/* Select IDMAC interface */
temp = mci_readl(host, CTRL);
I'll start reboot tests now to see how it behaves... I think Sonny is
out of the office for a few days so we might need to wait for a spin,
but I'll run with that change in the meantime and see how it behaves
for me.
Thanks!

Just FYI that I've been running with Alim's proposed change for
several days and it seems solid. I think Sonny may be able to spin
his patch this week. :)

-Doug

[Alim Akhtar]

Hi Doug,

Post by Doug Anderson
Hi,

Post by Sonny Rao
Alim,

Post by Alim Akhtar
Thats fine, I think every vendor (most of them) has a custom
implementation of dw_mmc, but they do have VERID register to check the
dw_mmc version.
The reason why I asked is, I have seen inconsistency in card
enumeration on few controller version, and this patch alone does not
help, and adding a call to dw_mci_idmac_reset() after DMA reset is
needed. And this is what is recommended in the synopsys's data book
also.
Do you see any issue/side effect after adding dw_mci_idmac_reset()?

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 66dc8fe..588b5b8 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -481,6 +481,7 @@ static void dw_mci_idmac_start_dma(struct dw_mci
*host, unsigned int sg_len)
/* Make sure to reset DMA in case we did PIO before this */
dw_mci_ctrl_reset(host, SDMMC_CTRL_DMA_RESET);
+ dw_mci_idmac_reset(host);
/* Select IDMAC interface */
temp = mci_readl(host, CTRL);
I'll start reboot tests now to see how it behaves... I think Sonny is
out of the office for a few days so we might need to wait for a spin,
but I'll run with that change in the meantime and see how it behaves
for me.
Thanks!

Just FYI that I've been running with Alim's proposed change for
several days and it seems solid. I think Sonny may be able to spin
his patch this week. :)

Sounds great. Thanks for testing.

Post by Doug Anderson
-Doug