Patchwork [U-Boot] i.MX28: Allow coexistence of PIO and DMA mode for SD/MMC

login
register
mail settings
Submitter Marek Vasut
Date April 5, 2012, 10:24 a.m.
Message ID <1333621458-6846-1-git-send-email-marex@denx.de>
Download mbox | patch
Permalink /patch/150904/
State Superseded
Headers show

Comments

Marek Vasut - April 5, 2012, 10:24 a.m.
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 |   41 ++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 40 insertions(+), 1 deletions(-)
Stefano Babic - April 5, 2012, 12:12 p.m.
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. 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.

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.

Any thoughts ?

Best regards,
Stefano Babic
Marek Vasut - April 5, 2012, 12:17 p.m.
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
Stefano Babic - April 5, 2012, 12:28 p.m.
On 05/04/2012 14:17, Marek Vasut wrote:

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

Sorry, I have not read accurately - we have two different CONFIG_

>> 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?

Yes. We need to make clear that DMA on MMC is in a working status.

Stefano

Patch

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);
+		}
+	}
+
+	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);