Patches for mxcmmc.c

Patches for mxcmmc.c

[Daniel Mack]

Here are three update patches for the mxc mmc driver to bring it closer
to a working SDIO implementation.

(There is one more pending which modifies the way the buffers' busy
 flags are interpreted, but we're not yet sure whether this one does
 the right thing, so I'm not yet posting it now.)

I'm not sure whether the problems we're having here are due to the
fact that DMA does not yet work on MX3. Hence, I would appreciate if
anyone with access to an MX2 board could try these patches and see
if the hardware can talk to SDIO connected peripherals (preferably
a Libertas Wifi solution, but trying anything else might also give
a hint).

Thanks,
Daniel

[PATCH 1/3] ARM: MXC: mxcmmc: misc cleanups
[PATCH 2/3] ARM: MXC: mxcmmc: Fix max_seg_size assignment
[PATCH 3/3] ARM: MXC: mxcmmc: Teach the driver SDIO operations

[PATCH 1/3] ARM: MXC: mxcmmc: misc cleanups

[Daniel Mack]

Add some more debug information and fix a couple of coding style things
in mxcmmc.c.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Dan Williams <dan.j.williams@intel.com>
---
 drivers/mmc/host/mxcmmc.c |   28 +++++++++++++++++-----------
 1 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index 8867152..32ac139 100644
--- a/drivers/mmc/host/mxcmmc.c
+++ b/drivers/mmc/host/mxcmmc.c
@@ -151,6 +151,8 @@ static void mxcmci_softreset(struct mxcmci_host *host)
 {
 	int i;
 
+	dev_dbg(mmc_dev(host->mmc), "mxcmci_softreset\n");
+
 	/* reset sequence */
 	writew(STR_STP_CLK_RESET, host->base + MMC_REG_STR_STP_CLK);
 	writew(STR_STP_CLK_RESET | STR_STP_CLK_START_CLK,
@@ -290,21 +292,29 @@ static int mxcmci_finish_data(struct mxcmci_host *host, unsigned int stat)
 		dev_dbg(mmc_dev(host->mmc), "request failed. status: 0x%08x\n",
 				stat);
 		if (stat & STATUS_CRC_READ_ERR) {
+			dev_err(mmc_dev(host->mmc), "%s: -EILSEQ\n", __func__);
 			data->error = -EILSEQ;
 		} else if (stat & STATUS_CRC_WRITE_ERR) {
 			u32 err_code = (stat >> 9) & 0x3;
-			if (err_code == 2) /* No CRC response */
+			if (err_code == 2) { /* No CRC response */
+				dev_err(mmc_dev(host->mmc),
+					"%s: No CRC -ETIMEDOUT\n", __func__);
 				data->error = -ETIMEDOUT;
-			else
+			} else {
+				dev_err(mmc_dev(host->mmc),
+					"%s: -EILSEQ\n", __func__);
 				data->error = -EILSEQ;
+			}
 		} else if (stat & STATUS_TIME_OUT_READ) {
+			dev_err(mmc_dev(host->mmc),
+				"%s: read -ETIMEDOUT\n", __func__);
 			data->error = -ETIMEDOUT;
 		} else {
+			dev_err(mmc_dev(host->mmc), "%s: -EIO\n", __func__);
 			data->error = -EIO;
 		}
-	} else {
+	} else
 		data->bytes_xfered = host->datasize;
-	}
 
 	data_error = data->error;
 
@@ -433,8 +443,6 @@ static int mxcmci_transfer_data(struct mxcmci_host *host)
 	struct scatterlist *sg;
 	int stat, i;
 
-	host->datasize = 0;
-
 	host->data = data;
 	host->datasize = 0;
 
@@ -471,9 +479,8 @@ static void mxcmci_datawork(struct work_struct *work)
 			mxcmci_finish_request(host, host->req);
 			return;
 		}
-	} else {
+	} else
 		mxcmci_finish_request(host, host->req);
-	}
 }
 
 #ifdef HAS_DMA
@@ -495,9 +502,8 @@ static void mxcmci_data_done(struct mxcmci_host *host, unsigned int stat)
 			mxcmci_finish_request(host, host->req);
 			return;
 		}
-	} else {
+	} else
 		mxcmci_finish_request(host, host->req);
-	}
 }
 #endif /* HAS_DMA */
 
@@ -512,7 +518,7 @@ static void mxcmci_cmd_done(struct mxcmci_host *host, unsigned int stat)
 	}
 
 	/* For the DMA case the DMA engine handles the data transfer
-	 * automatically. For non DMA we have to do it ourselves.
+	 * automatically. For non DMA we have to to it ourselves.
 	 * Don't do it in interrupt context though.
 	 */
 	if (!mxcmci_use_dma(host) && host->data)
-- 
1.6.5.2

[PATCH 2/3] ARM: MXC: mxcmmc: Fix max_seg_size assignment

[Daniel Mack]

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Dan Williams <dan.j.williams@intel.com>
---
 drivers/mmc/host/mxcmmc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index 32ac139..8aa6de0 100644
--- a/drivers/mmc/host/mxcmmc.c
+++ b/drivers/mmc/host/mxcmmc.c
@@ -714,7 +714,7 @@ static int mxcmci_probe(struct platform_device *pdev)
 	mmc->max_blk_size = 2048;
 	mmc->max_blk_count = 65535;
 	mmc->max_req_size = mmc->max_blk_size * mmc->max_blk_count;
-	mmc->max_seg_size = mmc->max_seg_size;
+	mmc->max_seg_size = mmc->max_req_size;
 
 	host = mmc_priv(mmc);
 	host->base = ioremap(r->start, resource_size(r));
-- 
1.6.5.2

[PATCH 3/3] ARM: MXC: mxcmmc: Teach the driver SDIO operations

[Daniel Mack]

Successfully tested on MX31 hardware using libertas SDIO peripherals.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Dan Williams <dan.j.williams@intel.com>
---
 drivers/mmc/host/mxcmmc.c |   48 ++++++++++++++++++++++++++++++++++++--------
 1 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index 8aa6de0..01324c5 100644
--- a/drivers/mmc/host/mxcmmc.c
+++ b/drivers/mmc/host/mxcmmc.c
@@ -119,6 +119,7 @@ struct mxcmci_host {
 	int			detect_irq;
 	int			dma;
 	int			do_dma;
+	int			use_sdio;
 	unsigned int		power_mode;
 	struct imxmmc_platform_data *pdata;
 
@@ -138,6 +139,7 @@ struct mxcmci_host {
 	int			clock;
 
 	struct work_struct	datawork;
+	spinlock_t		lock;
 };
 
 static void mxcmci_set_clk_rate(struct mxcmci_host *host, unsigned int clk_ios);
@@ -226,6 +228,8 @@ static int mxcmci_setup_data(struct mxcmci_host *host, struct mmc_data *data)
 static int mxcmci_start_cmd(struct mxcmci_host *host, struct mmc_command *cmd,
 		unsigned int cmdat)
 {
+	u32 int_cntr;
+
 	WARN_ON(host->cmd != NULL);
 	host->cmd = cmd;
 
@@ -249,13 +253,15 @@ static int mxcmci_start_cmd(struct mxcmci_host *host, struct mmc_command *cmd,
 		return -EINVAL;
 	}
 
+	int_cntr = INT_END_CMD_RES_EN;
+
 	if (mxcmci_use_dma(host))
-		writel(INT_READ_OP_EN | INT_WRITE_OP_DONE_EN |
-				INT_END_CMD_RES_EN,
-				host->base + MMC_REG_INT_CNTR);
-	else
-		writel(INT_END_CMD_RES_EN, host->base + MMC_REG_INT_CNTR);
+		int_cntr |= INT_READ_OP_EN | INT_WRITE_OP_DONE_EN;
+
+	if (host->use_sdio)
+		int_cntr |= INT_SDIO_IRQ_EN;
 
+	writel(int_cntr, host->base + MMC_REG_INT_CNTR);
 	writew(cmd->opcode, host->base + MMC_REG_CMD);
 	writel(cmd->arg, host->base + MMC_REG_ARG);
 	writew(cmdat, host->base + MMC_REG_CMD_DAT_CONT);
@@ -536,8 +542,12 @@ static irqreturn_t mxcmci_irq(int irq, void *devid)
 
 	dev_dbg(mmc_dev(host->mmc), "%s: 0x%08x\n", __func__, stat);
 
+	if ((stat & STATUS_SDIO_INT_ACTIVE) && host->use_sdio)
+		mmc_signal_sdio_irq(host->mmc);
+
 	if (stat & STATUS_END_CMD_RESP)
 		mxcmci_cmd_done(host, stat);
+
 #ifdef HAS_DMA
 	if (mxcmci_use_dma(host) &&
 		  (stat & (STATUS_DATA_TRANS_DONE | STATUS_WRITE_OP_DONE)))
@@ -674,11 +684,30 @@ static int mxcmci_get_ro(struct mmc_host *mmc)
 	return -ENOSYS;
 }
 
+static void mxcmci_enable_sdio_irq(struct mmc_host *mmc, int enable)
+{
+	struct mxcmci_host *host = mmc_priv(mmc);
+	unsigned long flags;
+	u32 int_cntr;
+
+	spin_lock_irqsave(&host->lock, flags);
+	host->use_sdio = enable;
+	int_cntr = readl(host->base + MMC_REG_INT_CNTR);
+
+	if (enable)
+		int_cntr |= INT_SDIO_IRQ_EN;
+	else
+		int_cntr &= ~INT_SDIO_IRQ_EN;
+
+	writel(int_cntr, host->base + MMC_REG_INT_CNTR);
+	spin_unlock_irqrestore(&host->lock, flags);
+}
 
 static const struct mmc_host_ops mxcmci_ops = {
-	.request	= mxcmci_request,
-	.set_ios	= mxcmci_set_ios,
-	.get_ro		= mxcmci_get_ro,
+	.request		= mxcmci_request,
+	.set_ios		= mxcmci_set_ios,
+	.get_ro			= mxcmci_get_ro,
+	.enable_sdio_irq	= mxcmci_enable_sdio_irq,
 };
 
 static int mxcmci_probe(struct platform_device *pdev)
@@ -706,7 +735,7 @@ static int mxcmci_probe(struct platform_device *pdev)
 	}
 
 	mmc->ops = &mxcmci_ops;
-	mmc->caps = MMC_CAP_4_BIT_DATA;
+	mmc->caps = MMC_CAP_4_BIT_DATA | MMC_CAP_SDIO_IRQ;
 
 	/* MMC core transfer sizes tunable parameters */
 	mmc->max_hw_segs = 64;
@@ -725,6 +754,7 @@ static int mxcmci_probe(struct platform_device *pdev)
 
 	host->mmc = mmc;
 	host->pdata = pdev->dev.platform_data;
+	spin_lock_init(&host->lock);
 
 	if (host->pdata && host->pdata->ocr_avail)
 		mmc->ocr_avail = host->pdata->ocr_avail;
-- 
1.6.5.2

[PATCH 2/3] ARM: MXC: mxcmmc: Fix max_seg_size assignment

[Russell King - ARM Linux]

Err "fix" ?  No, that's not what this is.

max_seg_size is the maximum segment size, not the maximum request size.
The naming of these comes from the block IO subsystem, so it's a good
idea to preserve the naming (and therefore the meaning.)

max_req_size suggests that this parameter indicates the maximum size
of any single request.  A single request can be made up of multiple
segments, so clearly this change is misleading.

On Thu, Nov 05, 2009 at 06:13:35PM +0100, Daniel Mack wrote:
> Signed-off-by: Daniel Mack <daniel@caiaq.de>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/mmc/host/mxcmmc.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
> index 32ac139..8aa6de0 100644
> --- a/drivers/mmc/host/mxcmmc.c
> +++ b/drivers/mmc/host/mxcmmc.c
> @@ -714,7 +714,7 @@ static int mxcmci_probe(struct platform_device *pdev)
>  	mmc->max_blk_size = 2048;
>  	mmc->max_blk_count = 65535;
>  	mmc->max_req_size = mmc->max_blk_size * mmc->max_blk_count;
> -	mmc->max_seg_size = mmc->max_seg_size;
> +	mmc->max_seg_size = mmc->max_req_size;
>  
>  	host = mmc_priv(mmc);
>  	host->base = ioremap(r->start, resource_size(r));
> -- 
> 1.6.5.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] ARM: MXC: mxcmmc: Fix max_seg_size assignment

[Daniel Mack]

On Thu, Nov 05, 2009 at 05:32:51PM +0000, Russell King - ARM Linux wrote:
> Err "fix" ?  No, that's not what this is.
> 
> max_seg_size is the maximum segment size, not the maximum request size.
> The naming of these comes from the block IO subsystem, so it's a good
> idea to preserve the naming (and therefore the meaning.)
> 
> max_req_size suggests that this parameter indicates the maximum size
> of any single request.  A single request can be made up of multiple
> segments, so clearly this change is misleading.

Well - I considered that a typo only, as the left and right side were
identical:

> > -	mmc->max_seg_size = mmc->max_seg_size;

Other drivers define seg_size = reg_size, and I thought that was what
was intended here.

Daniel

Patches for mxcmmc.c

[Daniel Mack]

On Thu, Nov 05, 2009 at 06:13:33PM +0100, Daniel Mack wrote:
> Here are three update patches for the mxc mmc driver to bring it closer
> to a working SDIO implementation.
> 
> (There is one more pending which modifies the way the buffers' busy
>  flags are interpreted, but we're not yet sure whether this one does
>  the right thing, so I'm not yet posting it now.)
> 
> I'm not sure whether the problems we're having here are due to the
> fact that DMA does not yet work on MX3. Hence, I would appreciate if
> anyone with access to an MX2 board could try these patches and see
> if the hardware can talk to SDIO connected peripherals (preferably
> a Libertas Wifi solution, but trying anything else might also give
> a hint).

Was anyone with a MX2 based board able to help testing this? It would be
very important for us to know whether SDIO cards work with DMA.

Thanks,
Daniel

[PATCH 1/3] ARM: MXC: mxcmmc: misc cleanups

[Andy Green]

On 04/14/10 08:21:16, Sascha Hauer wrote:
> On Tue, Apr 13, 2010 at 12:34:11PM +0200, Daniel Mack wrote:
> > On Thu, Apr 08, 2010 at 11:34:38AM +0200, Sascha Hauer wrote:
> > > On Tue, Apr 06, 2010 at 12:44:55PM +0200, Daniel Mack wrote:
> > > > Is this series ok to go in?
> > > > Sascha, will this go thru your tree?
> > > 
> > > Can do. One of your patches touches the generic SDIO layer. Can I
> have
> > > an acked-by from someone involved in it?
> > 
> > I think you can get a Tested-by: from Volker Ernst and Andy Green
> > and a Reviewed-by: from Micha? Miros?aw who was guiding me thru the
> > locking bits. Can anyone give some thumbs-up, please?
> 
> Ok, I think it's been enough time for someone to complain. Applied to
> mxc-master.

Sorry I was travelling yesterday.  The patch is definitely worth 
having.

We've been running a variation of this patch for several days, however 
not 100% the same since Volker has ported the iMX31 SDMA engine from 
the freescale tree for a few weeks now on our tree, and now has the 
SDIO working through SDMA (and a second channel with SD Card also 
on SDMA) FWIW.

We're not planning to do the work to get that into mainline but I think 
Daniel expressed an interest.

-Andy

[PATCH 1/3] ARM: MXC: mxcmmc: misc cleanups

[Sascha Hauer]

On Tue, Apr 13, 2010 at 12:34:11PM +0200, Daniel Mack wrote:
> On Thu, Apr 08, 2010 at 11:34:38AM +0200, Sascha Hauer wrote:
> > On Tue, Apr 06, 2010 at 12:44:55PM +0200, Daniel Mack wrote:
> > > Is this series ok to go in?
> > > Sascha, will this go thru your tree?
> > 
> > Can do. One of your patches touches the generic SDIO layer. Can I have
> > an acked-by from someone involved in it?
> 
> I think you can get a Tested-by: from Volker Ernst and Andy Green
> and a Reviewed-by: from Micha? Miros?aw who was guiding me thru the
> locking bits. Can anyone give some thumbs-up, please?

Ok, I think it's been enough time for someone to complain. Applied to
mxc-master.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 1/3] ARM: MXC: mxcmmc: misc cleanups

[Daniel Mack]

On Thu, Apr 08, 2010 at 11:34:38AM +0200, Sascha Hauer wrote:
> On Tue, Apr 06, 2010 at 12:44:55PM +0200, Daniel Mack wrote:
> > Is this series ok to go in?
> > Sascha, will this go thru your tree?
> 
> Can do. One of your patches touches the generic SDIO layer. Can I have
> an acked-by from someone involved in it?

I think you can get a Tested-by: from Volker Ernst and Andy Green
and a Reviewed-by: from Micha? Miros?aw who was guiding me thru the
locking bits. Can anyone give some thumbs-up, please?

Thanks,
Daniel

[PATCH 1/3] ARM: MXC: mxcmmc: misc cleanups

[Daniel Mack]

On Thu, Apr 08, 2010 at 11:34:38AM +0200, Sascha Hauer wrote:
> On Tue, Apr 06, 2010 at 12:44:55PM +0200, Daniel Mack wrote:
> > Is this series ok to go in?
> > Sascha, will this go thru your tree?
> 
> Can do. One of your patches touches the generic SDIO layer. Can I have
> an acked-by from someone involved in it?

AFAIK, there is currently no active maintainer for that.

Daniel

[PATCH 1/3] ARM: MXC: mxcmmc: misc cleanups

[Sascha Hauer]

On Tue, Apr 06, 2010 at 12:44:55PM +0200, Daniel Mack wrote:
> Is this series ok to go in?
> Sascha, will this go thru your tree?

Can do. One of your patches touches the generic SDIO layer. Can I have
an acked-by from someone involved in it?

Sascha



-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 1/3] ARM: MXC: mxcmmc: misc cleanups

[Daniel Mack]

Is this series ok to go in?
Sascha, will this go thru your tree?

Thanks,
Daniel

On Thu, Apr 01, 2010 at 10:03:23AM +0200, Daniel Mack wrote:
> Be more verbose on error messages and add one debug message.
> 
> Signed-off-by: Daniel Mack <daniel@caiaq.de>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Volker Ernst <volker.ernst@txtr.com>
> Cc: Jiri Kosina <jkosina@suse.cz>
> Cc: Micha? Miros?aw <mirqus@gmail.com>
> ---
>  drivers/mmc/host/mxcmmc.c |   17 +++++++++++++----
>  1 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
> index 2df9041..44a53ee 100644
> --- a/drivers/mmc/host/mxcmmc.c
> +++ b/drivers/mmc/host/mxcmmc.c
> @@ -151,6 +151,8 @@ static void mxcmci_softreset(struct mxcmci_host *host)
>  {
>  	int i;
>  
> +	dev_dbg(mmc_dev(host->mmc), "mxcmci_softreset\n");
> +
>  	/* reset sequence */
>  	writew(STR_STP_CLK_RESET, host->base + MMC_REG_STR_STP_CLK);
>  	writew(STR_STP_CLK_RESET | STR_STP_CLK_START_CLK,
> @@ -290,16 +292,25 @@ static int mxcmci_finish_data(struct mxcmci_host *host, unsigned int stat)
>  		dev_dbg(mmc_dev(host->mmc), "request failed. status: 0x%08x\n",
>  				stat);
>  		if (stat & STATUS_CRC_READ_ERR) {
> +			dev_err(mmc_dev(host->mmc), "%s: -EILSEQ\n", __func__);
>  			data->error = -EILSEQ;
>  		} else if (stat & STATUS_CRC_WRITE_ERR) {
>  			u32 err_code = (stat >> 9) & 0x3;
> -			if (err_code == 2) /* No CRC response */
> +			if (err_code == 2) { /* No CRC response */
> +				dev_err(mmc_dev(host->mmc),
> +					"%s: No CRC -ETIMEDOUT\n", __func__);
>  				data->error = -ETIMEDOUT;
> -			else
> +			} else {
> +				dev_err(mmc_dev(host->mmc),
> +					"%s: -EILSEQ\n", __func__);
>  				data->error = -EILSEQ;
> +			}
>  		} else if (stat & STATUS_TIME_OUT_READ) {
> +			dev_err(mmc_dev(host->mmc),
> +				"%s: read -ETIMEDOUT\n", __func__);
>  			data->error = -ETIMEDOUT;
>  		} else {
> +			dev_err(mmc_dev(host->mmc), "%s: -EIO\n", __func__);
>  			data->error = -EIO;
>  		}
>  	} else {
> @@ -433,8 +444,6 @@ static int mxcmci_transfer_data(struct mxcmci_host *host)
>  	struct scatterlist *sg;
>  	int stat, i;
>  
> -	host->datasize = 0;
> -
>  	host->data = data;
>  	host->datasize = 0;
>  
> -- 
> 1.7.0
> 

[PATCH 1/3] ARM: MXC: mxcmmc: misc cleanups

[Daniel Mack]

Be more verbose on error messages and add one debug message.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Volker Ernst <volker.ernst@txtr.com>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: Micha? Miros?aw <mirqus@gmail.com>
---
 drivers/mmc/host/mxcmmc.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index 2df9041..44a53ee 100644
--- a/drivers/mmc/host/mxcmmc.c
+++ b/drivers/mmc/host/mxcmmc.c
@@ -151,6 +151,8 @@ static void mxcmci_softreset(struct mxcmci_host *host)
 {
 	int i;
 
+	dev_dbg(mmc_dev(host->mmc), "mxcmci_softreset\n");
+
 	/* reset sequence */
 	writew(STR_STP_CLK_RESET, host->base + MMC_REG_STR_STP_CLK);
 	writew(STR_STP_CLK_RESET | STR_STP_CLK_START_CLK,
@@ -290,16 +292,25 @@ static int mxcmci_finish_data(struct mxcmci_host *host, unsigned int stat)
 		dev_dbg(mmc_dev(host->mmc), "request failed. status: 0x%08x\n",
 				stat);
 		if (stat & STATUS_CRC_READ_ERR) {
+			dev_err(mmc_dev(host->mmc), "%s: -EILSEQ\n", __func__);
 			data->error = -EILSEQ;
 		} else if (stat & STATUS_CRC_WRITE_ERR) {
 			u32 err_code = (stat >> 9) & 0x3;
-			if (err_code == 2) /* No CRC response */
+			if (err_code == 2) { /* No CRC response */
+				dev_err(mmc_dev(host->mmc),
+					"%s: No CRC -ETIMEDOUT\n", __func__);
 				data->error = -ETIMEDOUT;
-			else
+			} else {
+				dev_err(mmc_dev(host->mmc),
+					"%s: -EILSEQ\n", __func__);
 				data->error = -EILSEQ;
+			}
 		} else if (stat & STATUS_TIME_OUT_READ) {
+			dev_err(mmc_dev(host->mmc),
+				"%s: read -ETIMEDOUT\n", __func__);
 			data->error = -ETIMEDOUT;
 		} else {
+			dev_err(mmc_dev(host->mmc), "%s: -EIO\n", __func__);
 			data->error = -EIO;
 		}
 	} else {
@@ -433,8 +444,6 @@ static int mxcmci_transfer_data(struct mxcmci_host *host)
 	struct scatterlist *sg;
 	int stat, i;
 
-	host->datasize = 0;
-
 	host->data = data;
 	host->datasize = 0;
 
-- 
1.7.0

[PATCH 1/3] ARM: MXC: mxcmmc: misc cleanups

[Julien Boibessot]

Hi Daniel,

Daniel Mack a ?crit :
> Add some more debug information and fix a couple of coding style things
> in mxcmmc.c.
>
> ...
>  
> @@ -512,7 +518,7 @@ static void mxcmci_cmd_done(struct mxcmci_host *host, unsigned int stat)
>  	}
>  
>  	/* For the DMA case the DMA engine handles the data transfer
> -	 * automatically. For non DMA we have to do it ourselves.
> +	 * automatically. For non DMA we have to to it ourselves.
>   
I think you introduced a typo here ("to to")...

Regards,
Julien

Re: [PATCH 1/3] ARM: MXC: mxcmmc: misc cleanups

[Daniel Mack]

On Wed, Mar 31, 2010 at 02:38:56PM +0200, Sascha Hauer wrote:
> On Tue, Mar 30, 2010 at 08:31:59PM +0200, Daniel Mack wrote:
> > +			dev_err(mmc_dev(host->mmc),
> > +				"%s: read -ETIMEDOUT\n", __func__);
> >  			data->error = -ETIMEDOUT;
> >  		} else {
> > +			dev_err(mmc_dev(host->mmc), "%s: -EIO\n", __func__);
> 
> Do we really want to have these messages with dev_err? In the subject
> you are talking about debug output.

This _is_ definitely an error if get there, so I'll rather change the
commit message ;)

> 
> >  			data->error = -EIO;
> >  		}
> > -	} else {
> > +	} else
> >  		data->bytes_xfered = host->datasize;
> > -	}
> 
> Documentation/CodingStyle says that if braces are used in one branch of
> a conditional then they should be used in both branches.
> I personally don't care much about this statement but I think we should
> leave it as is. Otherwise some day someone wants to change it back
> according to the coding style.

Ok, true. I'll fix this (and the commit message) and resend.

Thanks,
Daniel

[PATCH 1/3] ARM: MXC: mxcmmc: misc cleanups

[Daniel Mack]

On Wed, Mar 31, 2010 at 02:38:56PM +0200, Sascha Hauer wrote:
> On Tue, Mar 30, 2010 at 08:31:59PM +0200, Daniel Mack wrote:
> > +			dev_err(mmc_dev(host->mmc),
> > +				"%s: read -ETIMEDOUT\n", __func__);
> >  			data->error = -ETIMEDOUT;
> >  		} else {
> > +			dev_err(mmc_dev(host->mmc), "%s: -EIO\n", __func__);
> 
> Do we really want to have these messages with dev_err? In the subject
> you are talking about debug output.

This _is_ definitely an error if get there, so I'll rather change the
commit message ;)

> 
> >  			data->error = -EIO;
> >  		}
> > -	} else {
> > +	} else
> >  		data->bytes_xfered = host->datasize;
> > -	}
> 
> Documentation/CodingStyle says that if braces are used in one branch of
> a conditional then they should be used in both branches.
> I personally don't care much about this statement but I think we should
> leave it as is. Otherwise some day someone wants to change it back
> according to the coding style.

Ok, true. I'll fix this (and the commit message) and resend.

Thanks,
Daniel

Re: [PATCH 1/3] ARM: MXC: mxcmmc: misc cleanups

[Sascha Hauer]

Hi Daniel,

On Tue, Mar 30, 2010 at 08:31:59PM +0200, Daniel Mack wrote:
> Add some more debug information and fix a couple of coding style things
> in mxcmmc.c.
> 
> Signed-off-by: Daniel Mack <daniel@caiaq.de>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Volker Ernst <volker.ernst@txtr.com>
> Cc: Jiri Kosina <jkosina@suse.cz>
> ---
>  drivers/mmc/host/mxcmmc.c |   28 +++++++++++++++++-----------
>  1 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
> index 2df9041..21037ff 100644
> --- a/drivers/mmc/host/mxcmmc.c
> +++ b/drivers/mmc/host/mxcmmc.c
> @@ -151,6 +151,8 @@ static void mxcmci_softreset(struct mxcmci_host *host)
>  {
>  	int i;
>  
> +	dev_dbg(mmc_dev(host->mmc), "mxcmci_softreset\n");
> +
>  	/* reset sequence */
>  	writew(STR_STP_CLK_RESET, host->base + MMC_REG_STR_STP_CLK);
>  	writew(STR_STP_CLK_RESET | STR_STP_CLK_START_CLK,
> @@ -290,21 +292,29 @@ static int mxcmci_finish_data(struct mxcmci_host *host, unsigned int stat)
>  		dev_dbg(mmc_dev(host->mmc), "request failed. status: 0x%08x\n",
>  				stat);
>  		if (stat & STATUS_CRC_READ_ERR) {
> +			dev_err(mmc_dev(host->mmc), "%s: -EILSEQ\n", __func__);
>  			data->error = -EILSEQ;
>  		} else if (stat & STATUS_CRC_WRITE_ERR) {
>  			u32 err_code = (stat >> 9) & 0x3;
> -			if (err_code == 2) /* No CRC response */
> +			if (err_code == 2) { /* No CRC response */
> +				dev_err(mmc_dev(host->mmc),
> +					"%s: No CRC -ETIMEDOUT\n", __func__);
>  				data->error = -ETIMEDOUT;
> -			else
> +			} else {
> +				dev_err(mmc_dev(host->mmc),
> +					"%s: -EILSEQ\n", __func__);
>  				data->error = -EILSEQ;
> +			}
>  		} else if (stat & STATUS_TIME_OUT_READ) {
> +			dev_err(mmc_dev(host->mmc),
> +				"%s: read -ETIMEDOUT\n", __func__);
>  			data->error = -ETIMEDOUT;
>  		} else {
> +			dev_err(mmc_dev(host->mmc), "%s: -EIO\n", __func__);

Do we really want to have these messages with dev_err? In the subject
you are talking about debug output.

>  			data->error = -EIO;
>  		}
> -	} else {
> +	} else
>  		data->bytes_xfered = host->datasize;
> -	}

Documentation/CodingStyle says that if braces are used in one branch of
a conditional then they should be used in both branches.
I personally don't care much about this statement but I think we should
leave it as is. Otherwise some day someone wants to change it back
according to the coding style.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 1/3] ARM: MXC: mxcmmc: misc cleanups

[Sascha Hauer]

Hi Daniel,

On Tue, Mar 30, 2010 at 08:31:59PM +0200, Daniel Mack wrote:
> Add some more debug information and fix a couple of coding style things
> in mxcmmc.c.
> 
> Signed-off-by: Daniel Mack <daniel@caiaq.de>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Volker Ernst <volker.ernst@txtr.com>
> Cc: Jiri Kosina <jkosina@suse.cz>
> ---
>  drivers/mmc/host/mxcmmc.c |   28 +++++++++++++++++-----------
>  1 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
> index 2df9041..21037ff 100644
> --- a/drivers/mmc/host/mxcmmc.c
> +++ b/drivers/mmc/host/mxcmmc.c
> @@ -151,6 +151,8 @@ static void mxcmci_softreset(struct mxcmci_host *host)
>  {
>  	int i;
>  
> +	dev_dbg(mmc_dev(host->mmc), "mxcmci_softreset\n");
> +
>  	/* reset sequence */
>  	writew(STR_STP_CLK_RESET, host->base + MMC_REG_STR_STP_CLK);
>  	writew(STR_STP_CLK_RESET | STR_STP_CLK_START_CLK,
> @@ -290,21 +292,29 @@ static int mxcmci_finish_data(struct mxcmci_host *host, unsigned int stat)
>  		dev_dbg(mmc_dev(host->mmc), "request failed. status: 0x%08x\n",
>  				stat);
>  		if (stat & STATUS_CRC_READ_ERR) {
> +			dev_err(mmc_dev(host->mmc), "%s: -EILSEQ\n", __func__);
>  			data->error = -EILSEQ;
>  		} else if (stat & STATUS_CRC_WRITE_ERR) {
>  			u32 err_code = (stat >> 9) & 0x3;
> -			if (err_code == 2) /* No CRC response */
> +			if (err_code == 2) { /* No CRC response */
> +				dev_err(mmc_dev(host->mmc),
> +					"%s: No CRC -ETIMEDOUT\n", __func__);
>  				data->error = -ETIMEDOUT;
> -			else
> +			} else {
> +				dev_err(mmc_dev(host->mmc),
> +					"%s: -EILSEQ\n", __func__);
>  				data->error = -EILSEQ;
> +			}
>  		} else if (stat & STATUS_TIME_OUT_READ) {
> +			dev_err(mmc_dev(host->mmc),
> +				"%s: read -ETIMEDOUT\n", __func__);
>  			data->error = -ETIMEDOUT;
>  		} else {
> +			dev_err(mmc_dev(host->mmc), "%s: -EIO\n", __func__);

Do we really want to have these messages with dev_err? In the subject
you are talking about debug output.

>  			data->error = -EIO;
>  		}
> -	} else {
> +	} else
>  		data->bytes_xfered = host->datasize;
> -	}

Documentation/CodingStyle says that if braces are used in one branch of
a conditional then they should be used in both branches.
I personally don't care much about this statement but I think we should
leave it as is. Otherwise some day someone wants to change it back
according to the coding style.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 1/3] ARM: MXC: mxcmmc: misc cleanups

[Daniel Mack]

Add some more debug information and fix a couple of coding style things
in mxcmmc.c.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Volker Ernst <volker.ernst@txtr.com>
Cc: Jiri Kosina <jkosina@suse.cz>
---
 drivers/mmc/host/mxcmmc.c |   28 +++++++++++++++++-----------
 1 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index 2df9041..21037ff 100644
--- a/drivers/mmc/host/mxcmmc.c
+++ b/drivers/mmc/host/mxcmmc.c
@@ -151,6 +151,8 @@ static void mxcmci_softreset(struct mxcmci_host *host)
 {
 	int i;
 
+	dev_dbg(mmc_dev(host->mmc), "mxcmci_softreset\n");
+
 	/* reset sequence */
 	writew(STR_STP_CLK_RESET, host->base + MMC_REG_STR_STP_CLK);
 	writew(STR_STP_CLK_RESET | STR_STP_CLK_START_CLK,
@@ -290,21 +292,29 @@ static int mxcmci_finish_data(struct mxcmci_host *host, unsigned int stat)
 		dev_dbg(mmc_dev(host->mmc), "request failed. status: 0x%08x\n",
 				stat);
 		if (stat & STATUS_CRC_READ_ERR) {
+			dev_err(mmc_dev(host->mmc), "%s: -EILSEQ\n", __func__);
 			data->error = -EILSEQ;
 		} else if (stat & STATUS_CRC_WRITE_ERR) {
 			u32 err_code = (stat >> 9) & 0x3;
-			if (err_code == 2) /* No CRC response */
+			if (err_code == 2) { /* No CRC response */
+				dev_err(mmc_dev(host->mmc),
+					"%s: No CRC -ETIMEDOUT\n", __func__);
 				data->error = -ETIMEDOUT;
-			else
+			} else {
+				dev_err(mmc_dev(host->mmc),
+					"%s: -EILSEQ\n", __func__);
 				data->error = -EILSEQ;
+			}
 		} else if (stat & STATUS_TIME_OUT_READ) {
+			dev_err(mmc_dev(host->mmc),
+				"%s: read -ETIMEDOUT\n", __func__);
 			data->error = -ETIMEDOUT;
 		} else {
+			dev_err(mmc_dev(host->mmc), "%s: -EIO\n", __func__);
 			data->error = -EIO;
 		}
-	} else {
+	} else
 		data->bytes_xfered = host->datasize;
-	}
 
 	data_error = data->error;
 
@@ -433,8 +443,6 @@ static int mxcmci_transfer_data(struct mxcmci_host *host)
 	struct scatterlist *sg;
 	int stat, i;
 
-	host->datasize = 0;
-
 	host->data = data;
 	host->datasize = 0;
 
@@ -471,9 +479,8 @@ static void mxcmci_datawork(struct work_struct *work)
 			mxcmci_finish_request(host, host->req);
 			return;
 		}
-	} else {
+	} else
 		mxcmci_finish_request(host, host->req);
-	}
 }
 
 #ifdef HAS_DMA
@@ -495,9 +502,8 @@ static void mxcmci_data_done(struct mxcmci_host *host, unsigned int stat)
 			mxcmci_finish_request(host, host->req);
 			return;
 		}
-	} else {
+	} else
 		mxcmci_finish_request(host, host->req);
-	}
 }
 #endif /* HAS_DMA */
 
@@ -512,7 +518,7 @@ static void mxcmci_cmd_done(struct mxcmci_host *host, unsigned int stat)
 	}
 
 	/* For the DMA case the DMA engine handles the data transfer
-	 * automatically. For non DMA we have to do it ourselves.
+	 * automatically. For non DMA we have to to it ourselves.
 	 * Don't do it in interrupt context though.
 	 */
 	if (!mxcmci_use_dma(host) && host->data)
-- 
1.7.0

[PATCH 1/3] ARM: MXC: mxcmmc: misc cleanups

[Daniel Mack]

Add some more debug information and fix a couple of coding style things
in mxcmmc.c.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Volker Ernst <volker.ernst@txtr.com>
Cc: Jiri Kosina <jkosina@suse.cz>
---
 drivers/mmc/host/mxcmmc.c |   28 +++++++++++++++++-----------
 1 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index 2df9041..21037ff 100644
--- a/drivers/mmc/host/mxcmmc.c
+++ b/drivers/mmc/host/mxcmmc.c
@@ -151,6 +151,8 @@ static void mxcmci_softreset(struct mxcmci_host *host)
 {
 	int i;
 
+	dev_dbg(mmc_dev(host->mmc), "mxcmci_softreset\n");
+
 	/* reset sequence */
 	writew(STR_STP_CLK_RESET, host->base + MMC_REG_STR_STP_CLK);
 	writew(STR_STP_CLK_RESET | STR_STP_CLK_START_CLK,
@@ -290,21 +292,29 @@ static int mxcmci_finish_data(struct mxcmci_host *host, unsigned int stat)
 		dev_dbg(mmc_dev(host->mmc), "request failed. status: 0x%08x\n",
 				stat);
 		if (stat & STATUS_CRC_READ_ERR) {
+			dev_err(mmc_dev(host->mmc), "%s: -EILSEQ\n", __func__);
 			data->error = -EILSEQ;
 		} else if (stat & STATUS_CRC_WRITE_ERR) {
 			u32 err_code = (stat >> 9) & 0x3;
-			if (err_code == 2) /* No CRC response */
+			if (err_code == 2) { /* No CRC response */
+				dev_err(mmc_dev(host->mmc),
+					"%s: No CRC -ETIMEDOUT\n", __func__);
 				data->error = -ETIMEDOUT;
-			else
+			} else {
+				dev_err(mmc_dev(host->mmc),
+					"%s: -EILSEQ\n", __func__);
 				data->error = -EILSEQ;
+			}
 		} else if (stat & STATUS_TIME_OUT_READ) {
+			dev_err(mmc_dev(host->mmc),
+				"%s: read -ETIMEDOUT\n", __func__);
 			data->error = -ETIMEDOUT;
 		} else {
+			dev_err(mmc_dev(host->mmc), "%s: -EIO\n", __func__);
 			data->error = -EIO;
 		}
-	} else {
+	} else
 		data->bytes_xfered = host->datasize;
-	}
 
 	data_error = data->error;
 
@@ -433,8 +443,6 @@ static int mxcmci_transfer_data(struct mxcmci_host *host)
 	struct scatterlist *sg;
 	int stat, i;
 
-	host->datasize = 0;
-
 	host->data = data;
 	host->datasize = 0;
 
@@ -471,9 +479,8 @@ static void mxcmci_datawork(struct work_struct *work)
 			mxcmci_finish_request(host, host->req);
 			return;
 		}
-	} else {
+	} else
 		mxcmci_finish_request(host, host->req);
-	}
 }
 
 #ifdef HAS_DMA
@@ -495,9 +502,8 @@ static void mxcmci_data_done(struct mxcmci_host *host, unsigned int stat)
 			mxcmci_finish_request(host, host->req);
 			return;
 		}
-	} else {
+	} else
 		mxcmci_finish_request(host, host->req);
-	}
 }
 #endif /* HAS_DMA */
 
@@ -512,7 +518,7 @@ static void mxcmci_cmd_done(struct mxcmci_host *host, unsigned int stat)
 	}
 
 	/* For the DMA case the DMA engine handles the data transfer
-	 * automatically. For non DMA we have to do it ourselves.
+	 * automatically. For non DMA we have to to it ourselves.
 	 * Don't do it in interrupt context though.
 	 */
 	if (!mxcmci_use_dma(host) && host->data)
-- 
1.7.0