ARM: MXC: mxcmmc: misc cleanups

PATCH New Daniel Mack 4 participants 13 messages Mar 30, 2010 – Apr 1, 2010

Subsystems (2)

Arm i.MX and Layerscape MMC, SD and SDIO

lore

Share & tools

lore

Daniel Mack

author

2010-03-30 18:31 UTC #0 lore raw

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

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

Signed-off-by Daniel Mack <[email protected]>

Cc Sascha Hauer <[email protected]>

Cc Dan Williams <[email protected]>

Cc Volker Ernst <[email protected]>

Cc Jiri Kosina <[email protected]>

---
 drivers/mmc/host/mxcmmc.c |   28 +++++++++++++++++-----------
 1 files changed, 17 insertions(+), 11 deletions(-)

drivers/mmc/host/mxcmmc.c +17 ~~-11~~

index 2df9041..21037ff 100644
@@ -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)

Signature · 1 line

1.7.0

Daniel Mack

author

2010-03-30 18:32 UTC #1 lore raw

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

Successfully tested on MX31 hardware using libertas SDIO peripherals.

Signed-off-by Daniel Mack <[email protected]>

Cc Sascha Hauer <[email protected]>

Cc Dan Williams <[email protected]>

Cc Volker Ernst <[email protected]>

Cc Jiri Kosina <[email protected]>

---
 drivers/mmc/host/mxcmmc.c |   55 ++++++++++++++++++++++++++++++++++++--------
 1 files changed, 45 insertions(+), 10 deletions(-)

drivers/mmc/host/mxcmmc.c +45 ~~-10~~

index 21037ff..83a4545 100644
@@ -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);

@@ -266,7 +272,12 @@ 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;
+
+	if (host->use_sdio)
+		int_cntr |= INT_SDIO_IRQ_EN;
+
+	writel(int_cntr, host->base + MMC_REG_INT_CNTR);
 
 	host->req = NULL;
 	host->cmd = NULL;

@@ -536,8 +547,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 +689,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 +740,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 +759,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;

Signature · 1 line

1.7.0

Sascha Hauer

maintainer

2010-03-31 13:03 UTC #2 lore raw

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

Quoted 99 lines · Daniel Mack

Successfully tested on MX31 hardware using libertas SDIO peripherals.

Signed-off-by Daniel Mack <[email protected]>

Cc Sascha Hauer <[email protected]>

Cc Dan Williams <[email protected]>

Cc Volker Ernst <[email protected]>

Cc Jiri Kosina <[email protected]>

---
 drivers/mmc/host/mxcmmc.c |   55 ++++++++++++++++++++++++++++++++++++--------
 1 files changed, 45 insertions(+), 10 deletions(-)

drivers/mmc/host/mxcmmc.c +28 -6

index 21037ff..83a4545 100644
@@ -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);

@@ -266,7 +272,12 @@ 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;
+
+	if (host->use_sdio)
+		int_cntr |= INT_SDIO_IRQ_EN;
+
+	writel(int_cntr, host->base + MMC_REG_INT_CNTR);
 
 	host->req = NULL;
 	host->cmd = NULL;

@@ -536,8 +547,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 +689,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);

drivers/mmc/host/mxcmmc.c static int mxcmci_get_ro(struct mmc_host *mmc) · line 708

Quoted 10 lines

drivers/mmc/host/mxcmmc.c +10 -0

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

The other places where MMC_REG_INT_CNTR is touched should be protected by this spinlock aswell.

Sascha

Quoted 34 lines

drivers/mmc/host/mxcmmc.c +7 -4

+}
 
 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 +740,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 +759,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;

Signature · 1 line

1.7.0

Signature · 4 lines

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 |

Daniel Mack

author

2010-03-31 13:29 UTC #3 lore raw

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

Quoted 13 lines · Sascha Hauer

Quoted 12 lines · Daniel Mack

drivers/mmc/host/mxcmmc.c +12 -0

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

Quoted 8 lines

drivers/mmc/host/mxcmmc.c static int mxcmci_get_ro(struct mmc_host *mmc) · line 708
Quoted 5 lines
drivers/mmc/host/mxcmmc.c +5 -0
+	else
+		int_cntr &= ~INT_SDIO_IRQ_EN;
+
+	writel(int_cntr, host->base + MMC_REG_INT_CNTR);
+	spin_unlock_irqrestore(&host->lock, flags);
The other places where MMC_REG_INT_CNTR is touched should be protected by this spinlock aswell.

Hmm, all other place don't do a read/modify/write cycle, so I'd say the don't need protection?

Daniel

Michał Mirosław

2010-03-31 16:41 UTC #4 lore raw

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

2010/3/31 Daniel Mack <[email protected]>:

Quoted 22 lines · Sascha Hauer

Quoted 13 lines · Daniel Mack
Quoted 12 lines
(quoted diff) +12 -0
+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;
Quoted 8 lines
Quoted 5 lines
+ ? else
+ ? ? ? ? ? int_cntr &= ~INT_SDIO_IRQ_EN;
+
+ ? writel(int_cntr, host->base + MMC_REG_INT_CNTR);
+ ? spin_unlock_irqrestore(&host->lock, flags);
The other places where MMC_REG_INT_CNTR is touched should be protected by this spinlock aswell.

Quoted 2 lines

Hmm, all other place don't do a read/modify/write cycle, so I'd say the don't need protection?

If you miss spinlocks around the unconditional writes they might happen between your readl() and writel().

Best Regards,
Micha? Miros?aw

Daniel Mack

author

2010-03-31 17:04 UTC #5 lore raw

PATCH 1/1 ARM: MXC: mxcmmc: Teach the driver SDIO operations

Successfully tested on MX31 hardware using libertas SDIO peripherals.

Signed-off-by Daniel Mack <[email protected]>

Cc Sascha Hauer <[email protected]>

Cc Dan Williams <[email protected]>

Cc Volker Ernst <[email protected]>

Cc Jiri Kosina <[email protected]>

---
 drivers/mmc/host/mxcmmc.c |   62 +++++++++++++++++++++++++++++++++++++-------
 1 files changed, 52 insertions(+), 10 deletions(-)

drivers/mmc/host/mxcmmc.c +52 ~~-10~~

index 44a53ee..f91a2c3 100644
@@ -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,17 @@ 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;
+
+	spin_lock_irqsave(&host->lock, flags);
+	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 +276,15 @@ 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;
+
+	if (host->use_sdio)
+		int_cntr |= INT_SDIO_IRQ_EN;
+
+	spin_lock_irqsave(&host->lock, flags);
+	writel(int_cntr, host->base + MMC_REG_INT_CNTR);
+	spin_unlock_irqrestore(&host->lock, flags);
 
 	host->req = NULL;
 	host->cmd = NULL;

@@ -539,8 +557,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)))

@@ -677,11 +699,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 +750,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 +769,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;

Signature · 1 line

1.7.0

Michał Mirosław

2010-03-31 17:34 UTC #6 lore raw

PATCH 1/1 ARM: MXC: mxcmmc: Teach the driver SDIO operations

2010/3/31 Daniel Mack <[email protected]>:

Quoted 16 lines

Successfully tested on MX31 hardware using libertas SDIO peripherals.
Signed-off-by Daniel Mack <[email protected]>
Cc Sascha Hauer <[email protected]>
Cc Dan Williams <[email protected]>
Cc Volker Ernst <[email protected]>
Cc Jiri Kosina <[email protected]>
---
?drivers/mmc/host/mxcmmc.c | ? 62 +++++++++++++++++++++++++++++++++++++------- ?1 files changed, 52 insertions(+), 10 deletions(-)
drivers/mmc/host/mxcmmc.c +0 -0
index 44a53ee..f91a2c3 100644

drivers/mmc/host/mxcmmc.c struct mxcmci_host { · line 125

Quoted 7 lines

drivers/mmc/host/mxcmmc.c +1 -0

@@ -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;

This field should be protected by the same spinlock as it is used to set the reg's value.

Quoted 28 lines

drivers/mmc/host/mxcmmc.c +6 -3

@@ -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,17 @@ 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);

drivers/mmc/host/mxcmmc.c static int mxcmci_start_cmd(struct mxcmci_host *host, struct mmc_command *cmd, · line 267

Quoted 10 lines

drivers/mmc/host/mxcmmc.c +8 -2

- ? ? ? 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;
+
+ ? ? ? spin_lock_irqsave(&host->lock, flags);
+ ? ? ? writel(int_cntr, host->base + MMC_REG_INT_CNTR);
+ ? ? ? spin_unlock_irqrestore(&host->lock, flags);

Here.

Quoted 7 lines

drivers/mmc/host/mxcmmc.c +0 -0

 ? ? ? ?writew(cmd->opcode, host->base + MMC_REG_CMD);
 ? ? ? ?writel(cmd->arg, host->base + MMC_REG_ARG);

@@ -266,7 +276,15 @@ 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)
 ?{

drivers/mmc/host/mxcmmc.c static int mxcmci_start_cmd(struct mxcmci_host *host, struct mmc_command *cmd, · line 287

Quoted 10 lines

drivers/mmc/host/mxcmmc.c +9 -1

- ? ? ? writel(0, host->base + MMC_REG_INT_CNTR);
+ ? ? ? u32 int_cntr = 0;
+ ? ? ? unsigned long flags;
+
+ ? ? ? if (host->use_sdio)
+ ? ? ? ? ? ? ? int_cntr |= INT_SDIO_IRQ_EN;
+
+ ? ? ? spin_lock_irqsave(&host->lock, flags);
+ ? ? ? writel(int_cntr, host->base + MMC_REG_INT_CNTR);
+ ? ? ? spin_unlock_irqrestore(&host->lock, flags);

And here.

drivers/mmc/host/mxcmmc.c static irqreturn_t mxcmci_irq(int irq, void *devid) · line 562

Quoted 10 lines

drivers/mmc/host/mxcmmc.c +3 -0

 ? ? ? ?host->req = NULL;
 ? ? ? ?host->cmd = NULL;

@@ -539,8 +557,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);
+

And here.

Quoted 18 lines

drivers/mmc/host/mxcmmc.c +1 -0

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

drivers/mmc/host/mxcmmc.c +8 -0

@@ -677,11 +699,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;

drivers/mmc/host/mxcmmc.c static int mxcmci_get_ro(struct mmc_host *mmc) · line 719

Quoted 10 lines

drivers/mmc/host/mxcmmc.c +10 -0

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

This one is good.

[...]

Best Regards,
Micha? Miros?aw

Daniel Mack

author

2010-04-01 08:02 UTC #7 lore raw

PATCH 1/1 ARM: MXC: mxcmmc: Teach the driver SDIO operations

Quoted 12 lines · Micha? Miros?aw

2010/3/31 Daniel Mack <[email protected]>:
Quoted 11 lines
Successfully tested on MX31 hardware using libertas SDIO peripherals.
Signed-off-by Daniel Mack <[email protected]>
Cc Sascha Hauer <[email protected]>
Cc Dan Williams <[email protected]>
Cc Volker Ernst <[email protected]>
Cc Jiri Kosina <[email protected]>
---
?drivers/mmc/host/mxcmmc.c | ? 62 +++++++++++++++++++++++++++++++++++++------- ?1 files changed, 52 insertions(+), 10 deletions(-)

Quoted 15 lines

drivers/mmc/host/mxcmmc.c struct mxcmci_host { · line 125
Quoted 12 lines
drivers/mmc/host/mxcmmc.c +1 -0
index 44a53ee..f91a2c3 100644
@@ -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;
 
This field should be protected by the same spinlock as it is used to set the reg's value.

Ok - I'll repost all three patches.

Again, thanks for your review.

Daniel

Daniel Mack

author

2010-03-30 18:32 UTC #8 lore raw

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

In reply to 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 <[email protected]>

Cc Sascha Hauer <[email protected]>

Cc Dan Williams <[email protected]>

Cc Volker Ernst <[email protected]>

Cc Jiri Kosina <[email protected]>

---
 drivers/mmc/core/sdio.c   |    6 ++++++
 drivers/mmc/host/mxcmmc.c |   14 ++++++++++++++
 include/linux/mmc/host.h  |    3 +++
 3 files changed, 23 insertions(+), 0 deletions(-)

drivers/mmc/core/sdio.c +6 -0

index 2dd4cfe..b9dee28 100644
@@ -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)) {

drivers/mmc/host/mxcmmc.c +14 -0

index 83a4545..f584c1b 100644
@@ -708,11 +708,25 @@ 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;
+}
+
 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)

include/linux/mmc/host.h +3 -0

index 43eaf5c..3196c84 100644
@@ -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;

Signature · 1 line

1.7.0

Sascha Hauer

maintainer

2010-03-31 12:38 UTC #9 lore raw

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

In reply to Daniel Mack

Hi Daniel,

Quoted 40 lines · Daniel Mack

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

Signed-off-by Daniel Mack <[email protected]>

Cc Sascha Hauer <[email protected]>

Cc Dan Williams <[email protected]>

Cc Volker Ernst <[email protected]>

Cc Jiri Kosina <[email protected]>

---
 drivers/mmc/host/mxcmmc.c |   28 +++++++++++++++++-----------
 1 files changed, 17 insertions(+), 11 deletions(-)

drivers/mmc/host/mxcmmc.c +7 -2

index 2df9041..21037ff 100644
@@ -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 {

drivers/mmc/host/mxcmmc.c static int mxcmci_finish_data(struct mxcmci_host *host, unsigned int stat) · line 313

Quoted 10 lines

drivers/mmc/host/mxcmmc.c +6 -0

+				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.

drivers/mmc/host/mxcmmc.c static int mxcmci_finish_data(struct mxcmci_host *host, unsigned int stat) · line 307

Quoted 6 lines

drivers/mmc/host/mxcmmc.c +1 -2

 			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

Signature · 4 lines

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 |

Daniel Mack

author

2010-03-31 13:02 UTC #10 lore raw

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

Quoted 9 lines · Sascha Hauer

drivers/mmc/host/mxcmmc.c static int mxcmci_finish_data(struct mxcmci_host *host, unsigned int stat) · line 313
Quoted 5 lines · Daniel Mack
drivers/mmc/host/mxcmmc.c +3 -0
+			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 ;)

Quoted 13 lines

drivers/mmc/host/mxcmmc.c static int mxcmci_finish_data(struct mxcmci_host *host, unsigned int stat) · line 307
Quoted 7 lines
drivers/mmc/host/mxcmmc.c +1 -2
 			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

Daniel Mack

author

2010-03-31 13:35 UTC #11 lore raw

PATCH 1/1 ARM: MXC: mxcmmc: misc cleanups

In reply to Sascha Hauer

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

Signed-off-by Daniel Mack <[email protected]>

Cc Sascha Hauer <[email protected]>

Cc Dan Williams <[email protected]>

Cc Volker Ernst <[email protected]>

Cc Jiri Kosina <[email protected]>

---
 drivers/mmc/host/mxcmmc.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

drivers/mmc/host/mxcmmc.c +13 -4

index 2df9041..44a53ee 100644
@@ -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;

Signature · 1 line

1.7.0

Julien Boibessot

2010-03-31 13:17 UTC #12 lore raw

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

In reply to Daniel Mack

Hi Daniel,

Daniel Mack a ?crit :

drivers/mmc/host/mxcmmc.c static void mxcmci_cmd_done(struct mxcmci_host *host, unsigned int stat) · line 522

Quoted 12 lines

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

...

drivers/mmc/host/mxcmmc.c +1 -1

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