Dear Stefano Babic,

On 05/04/2012 12:24, Marek Vasut wrote:

...

This SD DMA function of i.MX28 is still apparently too experimental to be enabled by default in 2012.04 release. Enable this feature only if the user plans to tinker with DCache or explicitly enables it.

Signed-off-by: Marek Vasut marex@denx.de Cc: Stefano Babic sbabic@denx.de Cc: Wolfgang Denk wd@denx.de Cc: Detlev Zundel dzu@denx.de Cc: Fabio Estevam fabio.estevam@freescale.com

---

Hi Marek,

...

drivers/mmc/mxsmmc.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 files changed, 40 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/mxsmmc.c b/drivers/mmc/mxsmmc.c index e8bad9d..7187796 100644 --- a/drivers/mmc/mxsmmc.c +++ b/drivers/mmc/mxsmmc.c @@ -66,8 +66,13 @@ mxsmmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)

struct mx28_ssp_regs *ssp_regs = priv->regs; uint32_t reg; int timeout;

• uint32_t data_count, cache_data_count;

• uint32_t data_count;

  uint32_t ctrl0;

+#ifndef CONFIG_MXS_MMC_DMA

• uint32_t *data_ptr;

+#else

• uint32_t cache_data_count;

+#endif

debug("MMC%d: CMD%d\n", mmc->block_dev.dev, cmd->cmdidx);

@@ -185,7 +190,9 @@ mxsmmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)

return 0;

data_count = data->blocksize * data->blocks;

• timeout = MXSMMC_MAX_TIMEOUT;

+#ifdef CONFIG_MXS_MMC_DMA

if (data_count % ARCH_DMA_MINALIGN)

cache_data_count = roundup(data_count, ARCH_DMA_MINALIGN);

else

@@ -218,6 +225,38 @@ mxsmmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)

invalidate_dcache_range((uint32_t)priv->desc->cmd.address,

(uint32_t)(priv->desc->cmd.address + cache_data_count));

}

+#else

• if (data->flags & MMC_DATA_READ) {

• data_ptr = (uint32_t *)data->dest;
  

• while (data_count && --timeout) {
  

• 	reg = readl(&ssp_regs->hw_ssp_status);
  

• 	if (!(reg & SSP_STATUS_FIFO_EMPTY)) {
  

• 		*data_ptr++ = readl(&ssp_regs->hw_ssp_data);
  

• 		data_count -= 4;
  

• 		timeout = MXSMMC_MAX_TIMEOUT;
  

• 	} else
  

• 		udelay(1000);
  

• }
  

• } else {

• data_ptr = (uint32_t *)data->src;
  

• timeout *= 100;
  

• while (data_count && --timeout) {
  

• 	reg = readl(&ssp_regs->hw_ssp_status);
  

• 	if (!(reg & SSP_STATUS_FIFO_FULL)) {
  

• 		writel(*data_ptr++, &ssp_regs->hw_ssp_data);
  

• 		data_count -= 4;
  

• 		timeout = MXSMMC_MAX_TIMEOUT;
  

• 	} else
  

• 		udelay(1000);
  

• }
  

• }

Ok, I see. This patch reverts to the status before applying the DMA's patch. I would only ask if it makes sense to leave DMA support on MMC when we know that is buggy.

It's buggy on some cards, I suspect some timing issue.

Is it DMA support working for NAND ? If yes, it is not possible to enable DMA for NAND and disable it for MMC, as both driver use the same CONFIG_MXS_MMC_DMA.

What do you mean? DMA is used for NAND ever since, I just recently tried enabling it also for MMC.

Anyway, I am not saying we should introduce a new switch, but maybe we should drop DMA support in msx_mmc for this release avoiding confusion. The current status with DMA support can be re-enabled on a -next branch.

Maybe adding comment to that switch that it's experimental and that it should be off to avoid problems would be enough?

Any thoughts ?

Best regards, Stefano Babic

Best regards, Marek Vasut

This SD DMA function of i.MX28 is still apparently too experimental to be enabled by default in 2012.04 release. Enable this feature only if the user plans to tinker with DCache or explicitly enables it.

Signed-off-by: Marek Vasut marex@denx.de Cc: Stefano Babic sbabic@denx.de Cc: Wolfgang Denk wd@denx.de Cc: Detlev Zundel dzu@denx.de Cc: Fabio Estevam fabio.estevam@freescale.com --- drivers/mmc/mxsmmc.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 47 insertions(+), 1 deletions(-)

V2: Drop dependency on enabled data cache V3: Add comment that this feature is experimental

diff --git a/drivers/mmc/mxsmmc.c b/drivers/mmc/mxsmmc.c index e8bad9d..dfceaef 100644 --- a/drivers/mmc/mxsmmc.c +++ b/drivers/mmc/mxsmmc.c @@ -43,6 +43,13 @@ #include <asm/arch/sys_proto.h> #include <asm/arch/dma.h>

+/* + * CONFIG_MXS_MMC_DMA: This feature is highly experimental and has no + * performance benefit unless you operate the platform with + * data cache enabled. This is disabled by default, enable + * only if you know what you're doing. + */ + struct mxsmmc_priv { int id; struct mx28_ssp_regs *regs; @@ -66,8 +73,13 @@ mxsmmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) struct mx28_ssp_regs *ssp_regs = priv->regs; uint32_t reg; int timeout; - uint32_t data_count, cache_data_count; + uint32_t data_count; uint32_t ctrl0; +#ifndef CONFIG_MXS_MMC_DMA + uint32_t *data_ptr; +#else + uint32_t cache_data_count; +#endif

debug("MMC%d: CMD%d\n", mmc->block_dev.dev, cmd->cmdidx);

@@ -185,7 +197,9 @@ mxsmmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) return 0;

data_count = data->blocksize * data->blocks; + timeout = MXSMMC_MAX_TIMEOUT;

+#ifdef CONFIG_MXS_MMC_DMA if (data_count % ARCH_DMA_MINALIGN) cache_data_count = roundup(data_count, ARCH_DMA_MINALIGN); else @@ -218,6 +232,38 @@ mxsmmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) invalidate_dcache_range((uint32_t)priv->desc->cmd.address, (uint32_t)(priv->desc->cmd.address + cache_data_count)); } +#else + if (data->flags & MMC_DATA_READ) { + data_ptr = (uint32_t *)data->dest; + while (data_count && --timeout) { + reg = readl(&ssp_regs->hw_ssp_status); + if (!(reg & SSP_STATUS_FIFO_EMPTY)) { + *data_ptr++ = readl(&ssp_regs->hw_ssp_data); + data_count -= 4; + timeout = MXSMMC_MAX_TIMEOUT; + } else + udelay(1000); + } + } else { + data_ptr = (uint32_t *)data->src; + timeout *= 100; + while (data_count && --timeout) { + reg = readl(&ssp_regs->hw_ssp_status); + if (!(reg & SSP_STATUS_FIFO_FULL)) { + writel(*data_ptr++, &ssp_regs->hw_ssp_data); + data_count -= 4; + timeout = MXSMMC_MAX_TIMEOUT; + } else + udelay(1000); + } + } + + if (!timeout) { + printf("MMC%d: Data timeout with command %d (status 0x%08x)!\n", + mmc->block_dev.dev, cmd->cmdidx, reg); + return COMM_ERR; + } +#endif

/* Check data errors */ reg = readl(&ssp_regs->hw_ssp_status);