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

[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 2/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>
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 |   70 +++++++++++++++++++++++++++++++++++++-------
 1 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index 44a53ee..51e880c 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,9 @@ 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;
+	unsigned long flags;
+
 	WARN_ON(host->cmd != NULL);
 	host->cmd = cmd;
 
@@ -249,12 +254,16 @@ 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;
+
+	spin_lock_irqsave(&host->lock, flags);
+	if (host->use_sdio)
+		int_cntr |= INT_SDIO_IRQ_EN;
+	writel(int_cntr, host->base + MMC_REG_INT_CNTR);
+	spin_unlock_irqrestore(&host->lock, flags);
 
 	writew(cmd->opcode, host->base + MMC_REG_CMD);
 	writel(cmd->arg, host->base + MMC_REG_ARG);
@@ -266,7 +275,14 @@ static int mxcmci_start_cmd(struct mxcmci_host *host, struct mmc_command *cmd,
 static void mxcmci_finish_request(struct mxcmci_host *host,
 		struct mmc_request *req)
 {
-	writel(0, host->base + MMC_REG_INT_CNTR);
+	u32 int_cntr = 0;
+	unsigned long flags;
+
+	spin_lock_irqsave(&host->lock, flags);
+	if (host->use_sdio)
+		int_cntr |= INT_SDIO_IRQ_EN;
+	writel(int_cntr, host->base + MMC_REG_INT_CNTR);
+	spin_unlock_irqrestore(&host->lock, flags);
 
 	host->req = NULL;
 	host->cmd = NULL;
@@ -532,15 +548,27 @@ static void mxcmci_cmd_done(struct mxcmci_host *host, unsigned int stat)
 static irqreturn_t mxcmci_irq(int irq, void *devid)
 {
 	struct mxcmci_host *host = devid;
+	unsigned long flags;
+	bool sdio_irq;
 	u32 stat;
 
 	stat = readl(host->base + MMC_REG_STATUS);
-	writel(stat, host->base + MMC_REG_STATUS);
+	writel(stat & ~STATUS_SDIO_INT_ACTIVE, host->base + MMC_REG_STATUS);
 
 	dev_dbg(mmc_dev(host->mmc), "%s: 0x%08x\n", __func__, stat);
 
+	spin_lock_irqsave(&host->lock, flags);
+	sdio_irq = (stat & STATUS_SDIO_INT_ACTIVE) && host->use_sdio;
+	spin_unlock_irqrestore(&host->lock, flags);
+
+	if (sdio_irq) {
+		writel(STATUS_SDIO_INT_ACTIVE, host->base + MMC_REG_STATUS);
+		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)))
@@ -677,11 +705,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)
@@ -709,7 +756,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;
@@ -728,6 +775,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.7.0

[PATCH 3/3] ARM: MXC: mxcmmc: work around a bug in the SDHC busy line handling

[Daniel Mack]

MX3 SoCs have a silicon bug which corrupts CRC calculation of
multi-block transfers when connected SDIO peripheral doesn't drive the
BUSY line as required by the specs.

One way to prevent this is to only allow 1-bit transfers.

Another way is playing tricks with the DMA engine, but this isn't
mainline yet. So for now, we live with the performance drawback of 1-bit
transfers until a nicer solution is found.

This patch introduces a new host controller callback 'init_card' which
is for now only called from mmc_sdio_init_card().

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/core/sdio.c   |    6 ++++++
 drivers/mmc/host/mxcmmc.c |   16 ++++++++++++++++
 include/linux/mmc/host.h  |    3 +++
 3 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 2dd4cfe..b9dee28 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -296,6 +296,12 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 	card->type = MMC_TYPE_SDIO;
 
 	/*
+	 * Call the optional HC's init_card function to handle quirks.
+	 */
+	if (host->ops->init_card)
+		host->ops->init_card(host, card);
+
+	/*
 	 * For native busses:  set card RCA and quit open drain mode.
 	 */
 	if (!powered_resume && !mmc_host_is_spi(host)) {
diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index 51e880c..2c53024 100644
--- a/drivers/mmc/host/mxcmmc.c
+++ b/drivers/mmc/host/mxcmmc.c
@@ -724,11 +724,27 @@ static void mxcmci_enable_sdio_irq(struct mmc_host *mmc, int enable)
 	spin_unlock_irqrestore(&host->lock, flags);
 }
 
+static void mxcmci_init_card(struct mmc_host *host, struct mmc_card *card)
+{
+	/*
+	 * MX3 SoCs have a silicon bug which corrupts CRC calculation of
+	 * multi-block transfers when connected SDIO peripheral doesn't
+	 * drive the BUSY line as required by the specs.
+	 * One way to prevent this is to only allow 1-bit transfers.
+	 */
+
+	if (cpu_is_mx3() && card->type == MMC_TYPE_SDIO)
+		host->caps &= ~MMC_CAP_4_BIT_DATA;
+	else
+		host->caps |= MMC_CAP_4_BIT_DATA;
+}
+
 static const struct mmc_host_ops mxcmci_ops = {
 	.request		= mxcmci_request,
 	.set_ios		= mxcmci_set_ios,
 	.get_ro			= mxcmci_get_ro,
 	.enable_sdio_irq	= mxcmci_enable_sdio_irq,
+	.init_card		= mxcmci_init_card,
 };
 
 static int mxcmci_probe(struct platform_device *pdev)
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 43eaf5c..3196c84 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -108,6 +108,9 @@ struct mmc_host_ops {
 	int	(*get_cd)(struct mmc_host *host);
 
 	void	(*enable_sdio_irq)(struct mmc_host *host, int enable);
+
+	/* optional callback for HC quirks */
+	void	(*init_card)(struct mmc_host *host, struct mmc_card *card);
 };
 
 struct mmc_card;
-- 
1.7.0

[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

[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]

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

[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

[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

[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

[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

[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]

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

[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

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