[PATCH RFC 0/11] FSL eSDHC support: second call for comments

[PATCH RFC 0/11] FSL eSDHC support: second call for comments

[Anton Vorontsov]

Hi all,

There were only a few comments on the previous version. So, here is
the second call for comments.

Changes since the first RFC:

- Use of_iomap() in sdhci-of.c (suggested by Arnd Bergmann). Also added
  Arnd's Acked-by: line for the sdhci-of patch.
- Kconfig help text improved (thanks to Matt Sealey and M. Warner Losh).
- In "sdhci: Add quirk to suppress PIO interrupts during DMA transfers"
  patch: sdhci_init() now clears SDHCI_PIO_DISABLED flag, otherwise we
  won't disable PIO interrupts after suspend.
- New patch: "sdhci: Add type checking for IO memory accessors"

That's all, for now.


Pierre, what do you think about memory accessors, what is your
preference regarding mem ops? Some options were discussed here:
http://lkml.org/lkml/2009/1/22/261


Thanks!

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

[PATCH 01/11] sdhci: Add quirk for controllers with no end-of-busy IRQ

[Anton Vorontsov]

From: Ben Dooks <ben-linux@fluff.org>

The Samsung SDHCI (and FSL eSDHC) controller block seems to fail
to generate an INT_DATA_END after the transfer has completed and
the bus busy state finished.

Changes in e809517f6fa5803a5a1cd56026f0e2190fc13d5c to use the
new busy method are the cause of the behaviour change.

Signed-off-by: Ben Dooks <ben-linux@fluff.org>
Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/mmc/host/sdhci.c |    5 ++++-
 drivers/mmc/host/sdhci.h |    2 ++
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 6b2d1f9..0a1f5c5 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1291,8 +1291,11 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask)
 		if (host->cmd->data)
 			DBG("Cannot wait for busy signal when also "
 				"doing a data transfer");
-		else
+		else if (!(host->quirks & SDHCI_QUIRK_NO_BUSY_IRQ))
 			return;
+
+		/* The controller does not support the end-of-busy IRQ,
+		 * fall through and take the SDHCI_INT_RESPONSE */
 	}
 
 	if (intmask & SDHCI_INT_RESPONSE)
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 3efba23..2d08dd4 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -210,6 +210,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_BROKEN_SMALL_PIO			(1<<13)
 /* Controller supports high speed but doesn't have the caps bit set */
 #define SDHCI_QUIRK_FORCE_HIGHSPEED			(1<<14)
+/* Controller does not provide transfer-complete interrupt when not busy */
+#define SDHCI_QUIRK_NO_BUSY_IRQ				(1<<15)
 
 	int			irq;		/* Device IRQ */
 	void __iomem *		ioaddr;		/* Mapped address */
-- 
1.5.6.5

[PATCH 02/11] sdhci: Add support for bus-specific IO memory accessors

[Anton Vorontsov]

Currently the SDHCI driver works with PCI accessors (write{l,b,w} and
read{l,b,w}).

With this patch drivers may change memory accessors, so that we can
support hosts with "weird" IO memory access requirments.

For example, in "FSL eSDHC" SDHCI hardware all registers are 32 bit
width, with big-endian addressing. That is, readb(0x2f) should turn
into readb(0x2c), and readw(0x2c) should be translated to
le16_to_cpu(readw(0x2e)).

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/mmc/host/sdhci-pci.c |    4 +-
 drivers/mmc/host/sdhci.c     |  205 +++++++++++++++++++++++++-----------------
 drivers/mmc/host/sdhci.h     |   48 ++++++++++
 3 files changed, 174 insertions(+), 83 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index f07255c..a3ced4d 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -212,7 +212,7 @@ static int jmicron_probe_slot(struct sdhci_pci_slot *slot)
 	if (slot->chip->pdev->revision == 0) {
 		u16 version;
 
-		version = readl(slot->host->ioaddr + SDHCI_HOST_VERSION);
+		version = sdhci_readl(slot->host, SDHCI_HOST_VERSION);
 		version = (version & SDHCI_VENDOR_VER_MASK) >>
 			SDHCI_VENDOR_VER_SHIFT;
 
@@ -583,7 +583,7 @@ static void sdhci_pci_remove_slot(struct sdhci_pci_slot *slot)
 	u32 scratch;
 
 	dead = 0;
-	scratch = readl(slot->host->ioaddr + SDHCI_INT_STATUS);
+	scratch = sdhci_readl(slot->host, SDHCI_INT_STATUS);
 	if (scratch == (u32)-1)
 		dead = 1;
 
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 0a1f5c5..8557521 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -48,35 +48,35 @@ static void sdhci_dumpregs(struct sdhci_host *host)
 	printk(KERN_DEBUG DRIVER_NAME ": ============== REGISTER DUMP ==============\n");
 
 	printk(KERN_DEBUG DRIVER_NAME ": Sys addr: 0x%08x | Version:  0x%08x\n",
-		readl(host->ioaddr + SDHCI_DMA_ADDRESS),
-		readw(host->ioaddr + SDHCI_HOST_VERSION));
+		sdhci_readl(host, SDHCI_DMA_ADDRESS),
+		sdhci_readw(host, SDHCI_HOST_VERSION));
 	printk(KERN_DEBUG DRIVER_NAME ": Blk size: 0x%08x | Blk cnt:  0x%08x\n",
-		readw(host->ioaddr + SDHCI_BLOCK_SIZE),
-		readw(host->ioaddr + SDHCI_BLOCK_COUNT));
+		sdhci_readw(host, SDHCI_BLOCK_SIZE),
+		sdhci_readw(host, SDHCI_BLOCK_COUNT));
 	printk(KERN_DEBUG DRIVER_NAME ": Argument: 0x%08x | Trn mode: 0x%08x\n",
-		readl(host->ioaddr + SDHCI_ARGUMENT),
-		readw(host->ioaddr + SDHCI_TRANSFER_MODE));
+		sdhci_readl(host, SDHCI_ARGUMENT),
+		sdhci_readw(host, SDHCI_TRANSFER_MODE));
 	printk(KERN_DEBUG DRIVER_NAME ": Present:  0x%08x | Host ctl: 0x%08x\n",
-		readl(host->ioaddr + SDHCI_PRESENT_STATE),
-		readb(host->ioaddr + SDHCI_HOST_CONTROL));
+		sdhci_readl(host, SDHCI_PRESENT_STATE),
+		sdhci_readb(host, SDHCI_HOST_CONTROL));
 	printk(KERN_DEBUG DRIVER_NAME ": Power:    0x%08x | Blk gap:  0x%08x\n",
-		readb(host->ioaddr + SDHCI_POWER_CONTROL),
-		readb(host->ioaddr + SDHCI_BLOCK_GAP_CONTROL));
+		sdhci_readb(host, SDHCI_POWER_CONTROL),
+		sdhci_readb(host, SDHCI_BLOCK_GAP_CONTROL));
 	printk(KERN_DEBUG DRIVER_NAME ": Wake-up:  0x%08x | Clock:    0x%08x\n",
-		readb(host->ioaddr + SDHCI_WAKE_UP_CONTROL),
-		readw(host->ioaddr + SDHCI_CLOCK_CONTROL));
+		sdhci_readb(host, SDHCI_WAKE_UP_CONTROL),
+		sdhci_readw(host, SDHCI_CLOCK_CONTROL));
 	printk(KERN_DEBUG DRIVER_NAME ": Timeout:  0x%08x | Int stat: 0x%08x\n",
-		readb(host->ioaddr + SDHCI_TIMEOUT_CONTROL),
-		readl(host->ioaddr + SDHCI_INT_STATUS));
+		sdhci_readb(host, SDHCI_TIMEOUT_CONTROL),
+		sdhci_readl(host, SDHCI_INT_STATUS));
 	printk(KERN_DEBUG DRIVER_NAME ": Int enab: 0x%08x | Sig enab: 0x%08x\n",
-		readl(host->ioaddr + SDHCI_INT_ENABLE),
-		readl(host->ioaddr + SDHCI_SIGNAL_ENABLE));
+		sdhci_readl(host, SDHCI_INT_ENABLE),
+		sdhci_readl(host, SDHCI_SIGNAL_ENABLE));
 	printk(KERN_DEBUG DRIVER_NAME ": AC12 err: 0x%08x | Slot int: 0x%08x\n",
-		readw(host->ioaddr + SDHCI_ACMD12_ERR),
-		readw(host->ioaddr + SDHCI_SLOT_INT_STATUS));
+		sdhci_readw(host, SDHCI_ACMD12_ERR),
+		sdhci_readw(host, SDHCI_SLOT_INT_STATUS));
 	printk(KERN_DEBUG DRIVER_NAME ": Caps:     0x%08x | Max curr: 0x%08x\n",
-		readl(host->ioaddr + SDHCI_CAPABILITIES),
-		readl(host->ioaddr + SDHCI_MAX_CURRENT));
+		sdhci_readl(host, SDHCI_CAPABILITIES),
+		sdhci_readl(host, SDHCI_MAX_CURRENT));
 
 	printk(KERN_DEBUG DRIVER_NAME ": ===========================================\n");
 }
@@ -87,17 +87,47 @@ static void sdhci_dumpregs(struct sdhci_host *host)
  *                                                                           *
 \*****************************************************************************/
 
+static u32 sdhci_def_readl(struct sdhci_host *host, int reg)
+{
+	return readl(host->ioaddr + reg);
+}
+
+static u16 sdhci_def_readw(struct sdhci_host *host, int reg)
+{
+	return readw(host->ioaddr + reg);
+}
+
+static u8 sdhci_def_readb(struct sdhci_host *host, int reg)
+{
+	return readb(host->ioaddr + reg);
+}
+
+static void sdhci_def_writel(struct sdhci_host *host, u32 val, int reg)
+{
+	writel(val, host->ioaddr + reg);
+}
+
+static void sdhci_def_writew(struct sdhci_host *host, u16 val, int reg)
+{
+	writew(val, host->ioaddr + reg);
+}
+
+static void sdhci_def_writeb(struct sdhci_host *host, u8 val, int reg)
+{
+	writeb(val, host->ioaddr + reg);
+}
+
 static void sdhci_reset(struct sdhci_host *host, u8 mask)
 {
 	unsigned long timeout;
 
 	if (host->quirks & SDHCI_QUIRK_NO_CARD_NO_RESET) {
-		if (!(readl(host->ioaddr + SDHCI_PRESENT_STATE) &
+		if (!(sdhci_readl(host, SDHCI_PRESENT_STATE) &
 			SDHCI_CARD_PRESENT))
 			return;
 	}
 
-	writeb(mask, host->ioaddr + SDHCI_SOFTWARE_RESET);
+	sdhci_writeb(host, mask, SDHCI_SOFTWARE_RESET);
 
 	if (mask & SDHCI_RESET_ALL)
 		host->clock = 0;
@@ -106,7 +136,7 @@ static void sdhci_reset(struct sdhci_host *host, u8 mask)
 	timeout = 100;
 
 	/* hw clears the bit when it's done */
-	while (readb(host->ioaddr + SDHCI_SOFTWARE_RESET) & mask) {
+	while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) {
 		if (timeout == 0) {
 			printk(KERN_ERR "%s: Reset 0x%x never completed.\n",
 				mmc_hostname(host->mmc), (int)mask);
@@ -132,26 +162,26 @@ static void sdhci_init(struct sdhci_host *host)
 		SDHCI_INT_DMA_END | SDHCI_INT_DATA_END | SDHCI_INT_RESPONSE |
 		SDHCI_INT_ADMA_ERROR;
 
-	writel(intmask, host->ioaddr + SDHCI_INT_ENABLE);
-	writel(intmask, host->ioaddr + SDHCI_SIGNAL_ENABLE);
+	sdhci_writel(host, intmask, SDHCI_INT_ENABLE);
+	sdhci_writel(host, intmask, SDHCI_SIGNAL_ENABLE);
 }
 
 static void sdhci_activate_led(struct sdhci_host *host)
 {
 	u8 ctrl;
 
-	ctrl = readb(host->ioaddr + SDHCI_HOST_CONTROL);
+	ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
 	ctrl |= SDHCI_CTRL_LED;
-	writeb(ctrl, host->ioaddr + SDHCI_HOST_CONTROL);
+	sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
 }
 
 static void sdhci_deactivate_led(struct sdhci_host *host)
 {
 	u8 ctrl;
 
-	ctrl = readb(host->ioaddr + SDHCI_HOST_CONTROL);
+	ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
 	ctrl &= ~SDHCI_CTRL_LED;
-	writeb(ctrl, host->ioaddr + SDHCI_HOST_CONTROL);
+	sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
 }
 
 #ifdef SDHCI_USE_LEDS_CLASS
@@ -205,7 +235,7 @@ static void sdhci_read_block_pio(struct sdhci_host *host)
 
 		while (len) {
 			if (chunk == 0) {
-				scratch = readl(host->ioaddr + SDHCI_BUFFER);
+				scratch = sdhci_readl(host, SDHCI_BUFFER);
 				chunk = 4;
 			}
 
@@ -257,7 +287,7 @@ static void sdhci_write_block_pio(struct sdhci_host *host)
 			len--;
 
 			if ((chunk == 4) || ((len == 0) && (blksize == 0))) {
-				writel(scratch, host->ioaddr + SDHCI_BUFFER);
+				sdhci_writel(host, scratch, SDHCI_BUFFER);
 				chunk = 0;
 				scratch = 0;
 			}
@@ -292,7 +322,7 @@ static void sdhci_transfer_pio(struct sdhci_host *host)
 		(host->data->blocks == 1))
 		mask = ~0;
 
-	while (readl(host->ioaddr + SDHCI_PRESENT_STATE) & mask) {
+	while (sdhci_readl(host, SDHCI_PRESENT_STATE) & mask) {
 		if (host->data->flags & MMC_DATA_READ)
 			sdhci_read_block_pio(host);
 		else
@@ -581,7 +611,7 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
 	host->data_early = 0;
 
 	count = sdhci_calc_timeout(host, data);
-	writeb(count, host->ioaddr + SDHCI_TIMEOUT_CONTROL);
+	sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
 
 	if (host->flags & SDHCI_USE_DMA)
 		host->flags |= SDHCI_REQ_USE_DMA;
@@ -661,8 +691,8 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
 				WARN_ON(1);
 				host->flags &= ~SDHCI_REQ_USE_DMA;
 			} else {
-				writel(host->adma_addr,
-					host->ioaddr + SDHCI_ADMA_ADDRESS);
+				sdhci_writel(host, host->adma_addr,
+					SDHCI_ADMA_ADDRESS);
 			}
 		} else {
 			int sg_cnt;
@@ -681,8 +711,8 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
 				host->flags &= ~SDHCI_REQ_USE_DMA;
 			} else {
 				WARN_ON(sg_cnt != 1);
-				writel(sg_dma_address(data->sg),
-					host->ioaddr + SDHCI_DMA_ADDRESS);
+				sdhci_writel(host, sg_dma_address(data->sg),
+					SDHCI_DMA_ADDRESS);
 			}
 		}
 	}
@@ -693,14 +723,14 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
 	 * is ADMA.
 	 */
 	if (host->version >= SDHCI_SPEC_200) {
-		ctrl = readb(host->ioaddr + SDHCI_HOST_CONTROL);
+		ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
 		ctrl &= ~SDHCI_CTRL_DMA_MASK;
 		if ((host->flags & SDHCI_REQ_USE_DMA) &&
 			(host->flags & SDHCI_USE_ADMA))
 			ctrl |= SDHCI_CTRL_ADMA32;
 		else
 			ctrl |= SDHCI_CTRL_SDMA;
-		writeb(ctrl, host->ioaddr + SDHCI_HOST_CONTROL);
+		sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
 	}
 
 	if (!(host->flags & SDHCI_REQ_USE_DMA)) {
@@ -710,9 +740,8 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
 	}
 
 	/* We do not handle DMA boundaries, so set it to max (512 KiB) */
-	writew(SDHCI_MAKE_BLKSZ(7, data->blksz),
-		host->ioaddr + SDHCI_BLOCK_SIZE);
-	writew(data->blocks, host->ioaddr + SDHCI_BLOCK_COUNT);
+	sdhci_writew(host, SDHCI_MAKE_BLKSZ(7, data->blksz), SDHCI_BLOCK_SIZE);
+	sdhci_writew(host, data->blocks, SDHCI_BLOCK_COUNT);
 }
 
 static void sdhci_set_transfer_mode(struct sdhci_host *host,
@@ -733,7 +762,7 @@ static void sdhci_set_transfer_mode(struct sdhci_host *host,
 	if (host->flags & SDHCI_REQ_USE_DMA)
 		mode |= SDHCI_TRNS_DMA;
 
-	writew(mode, host->ioaddr + SDHCI_TRANSFER_MODE);
+	sdhci_writew(host, mode, SDHCI_TRANSFER_MODE);
 }
 
 static void sdhci_finish_data(struct sdhci_host *host)
@@ -802,7 +831,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 	if (host->mrq->data && (cmd == host->mrq->data->stop))
 		mask &= ~SDHCI_DATA_INHIBIT;
 
-	while (readl(host->ioaddr + SDHCI_PRESENT_STATE) & mask) {
+	while (sdhci_readl(host, SDHCI_PRESENT_STATE) & mask) {
 		if (timeout == 0) {
 			printk(KERN_ERR "%s: Controller never released "
 				"inhibit bit(s).\n", mmc_hostname(host->mmc));
@@ -821,7 +850,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 
 	sdhci_prepare_data(host, cmd->data);
 
-	writel(cmd->arg, host->ioaddr + SDHCI_ARGUMENT);
+	sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT);
 
 	sdhci_set_transfer_mode(host, cmd->data);
 
@@ -849,8 +878,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 	if (cmd->data)
 		flags |= SDHCI_CMD_DATA;
 
-	writew(SDHCI_MAKE_CMD(cmd->opcode, flags),
-		host->ioaddr + SDHCI_COMMAND);
+	sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND);
 }
 
 static void sdhci_finish_command(struct sdhci_host *host)
@@ -863,15 +891,15 @@ static void sdhci_finish_command(struct sdhci_host *host)
 		if (host->cmd->flags & MMC_RSP_136) {
 			/* CRC is stripped so we need to do some shifting. */
 			for (i = 0;i < 4;i++) {
-				host->cmd->resp[i] = readl(host->ioaddr +
+				host->cmd->resp[i] = sdhci_readl(host,
 					SDHCI_RESPONSE + (3-i)*4) << 8;
 				if (i != 3)
 					host->cmd->resp[i] |=
-						readb(host->ioaddr +
+						sdhci_readb(host,
 						SDHCI_RESPONSE + (3-i)*4-1);
 			}
 		} else {
-			host->cmd->resp[0] = readl(host->ioaddr + SDHCI_RESPONSE);
+			host->cmd->resp[0] = sdhci_readl(host, SDHCI_RESPONSE);
 		}
 	}
 
@@ -895,7 +923,7 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 	if (clock == host->clock)
 		return;
 
-	writew(0, host->ioaddr + SDHCI_CLOCK_CONTROL);
+	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
 
 	if (clock == 0)
 		goto out;
@@ -908,11 +936,11 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 
 	clk = div << SDHCI_DIVIDER_SHIFT;
 	clk |= SDHCI_CLOCK_INT_EN;
-	writew(clk, host->ioaddr + SDHCI_CLOCK_CONTROL);
+	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
 
 	/* Wait max 10 ms */
 	timeout = 10;
-	while (!((clk = readw(host->ioaddr + SDHCI_CLOCK_CONTROL))
+	while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
 		& SDHCI_CLOCK_INT_STABLE)) {
 		if (timeout == 0) {
 			printk(KERN_ERR "%s: Internal clock never "
@@ -925,7 +953,7 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 	}
 
 	clk |= SDHCI_CLOCK_CARD_EN;
-	writew(clk, host->ioaddr + SDHCI_CLOCK_CONTROL);
+	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
 
 out:
 	host->clock = clock;
@@ -939,7 +967,7 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned short power)
 		return;
 
 	if (power == (unsigned short)-1) {
-		writeb(0, host->ioaddr + SDHCI_POWER_CONTROL);
+		sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
 		goto out;
 	}
 
@@ -948,7 +976,7 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned short power)
 	 * a new value. Some controllers don't seem to like this though.
 	 */
 	if (!(host->quirks & SDHCI_QUIRK_SINGLE_POWER_WRITE))
-		writeb(0, host->ioaddr + SDHCI_POWER_CONTROL);
+		sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
 
 	pwr = SDHCI_POWER_ON;
 
@@ -973,10 +1001,9 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned short power)
 	 * and set turn on power at the same time, so set the voltage first.
 	 */
 	if ((host->quirks & SDHCI_QUIRK_NO_SIMULT_VDD_AND_POWER))
-		writeb(pwr & ~SDHCI_POWER_ON,
-				host->ioaddr + SDHCI_POWER_CONTROL);
+		sdhci_writeb(host, pwr & ~SDHCI_POWER_ON, SDHCI_POWER_CONTROL);
 
-	writeb(pwr, host->ioaddr + SDHCI_POWER_CONTROL);
+	sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
 
 out:
 	host->power = power;
@@ -1005,7 +1032,7 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 
 	host->mrq = mrq;
 
-	if (!(readl(host->ioaddr + SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)
+	if (!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)
 		|| (host->flags & SDHCI_DEVICE_DEAD)) {
 		host->mrq->cmd->error = -ENOMEDIUM;
 		tasklet_schedule(&host->finish_tasklet);
@@ -1034,7 +1061,7 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	 * Should clear out any weird states.
 	 */
 	if (ios->power_mode == MMC_POWER_OFF) {
-		writel(0, host->ioaddr + SDHCI_SIGNAL_ENABLE);
+		sdhci_writel(host, 0, SDHCI_SIGNAL_ENABLE);
 		sdhci_init(host);
 	}
 
@@ -1045,7 +1072,7 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	else
 		sdhci_set_power(host, ios->vdd);
 
-	ctrl = readb(host->ioaddr + SDHCI_HOST_CONTROL);
+	ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
 
 	if (ios->bus_width == MMC_BUS_WIDTH_4)
 		ctrl |= SDHCI_CTRL_4BITBUS;
@@ -1057,7 +1084,7 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	else
 		ctrl &= ~SDHCI_CTRL_HISPD;
 
-	writeb(ctrl, host->ioaddr + SDHCI_HOST_CONTROL);
+	sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
 
 	/*
 	 * Some (ENE) controllers go apeshit on some ios operation,
@@ -1085,7 +1112,7 @@ static int sdhci_get_ro(struct mmc_host *mmc)
 	if (host->flags & SDHCI_DEVICE_DEAD)
 		present = 0;
 	else
-		present = readl(host->ioaddr + SDHCI_PRESENT_STATE);
+		present = sdhci_readl(host, SDHCI_PRESENT_STATE);
 
 	spin_unlock_irqrestore(&host->lock, flags);
 
@@ -1105,14 +1132,14 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
 	if (host->flags & SDHCI_DEVICE_DEAD)
 		goto out;
 
-	ier = readl(host->ioaddr + SDHCI_INT_ENABLE);
+	ier = sdhci_readl(host, SDHCI_INT_ENABLE);
 
 	ier &= ~SDHCI_INT_CARD_INT;
 	if (enable)
 		ier |= SDHCI_INT_CARD_INT;
 
-	writel(ier, host->ioaddr + SDHCI_INT_ENABLE);
-	writel(ier, host->ioaddr + SDHCI_SIGNAL_ENABLE);
+	sdhci_writel(host, ier, SDHCI_INT_ENABLE);
+	sdhci_writel(host, ier, SDHCI_SIGNAL_ENABLE);
 
 out:
 	mmiowb();
@@ -1142,7 +1169,7 @@ static void sdhci_tasklet_card(unsigned long param)
 
 	spin_lock_irqsave(&host->lock, flags);
 
-	if (!(readl(host->ioaddr + SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)) {
+	if (!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)) {
 		if (host->mrq) {
 			printk(KERN_ERR "%s: Card removed during transfer!\n",
 				mmc_hostname(host->mmc));
@@ -1346,8 +1373,8 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
 		 * we need to at least restart the transfer.
 		 */
 		if (intmask & SDHCI_INT_DMA_END)
-			writel(readl(host->ioaddr + SDHCI_DMA_ADDRESS),
-				host->ioaddr + SDHCI_DMA_ADDRESS);
+			sdhci_writel(host, sdhci_readl(host, SDHCI_DMA_ADDRESS),
+				SDHCI_DMA_ADDRESS);
 
 		if (intmask & SDHCI_INT_DATA_END) {
 			if (host->cmd) {
@@ -1373,7 +1400,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 
 	spin_lock(&host->lock);
 
-	intmask = readl(host->ioaddr + SDHCI_INT_STATUS);
+	intmask = sdhci_readl(host, SDHCI_INT_STATUS);
 
 	if (!intmask || intmask == 0xffffffff) {
 		result = IRQ_NONE;
@@ -1384,22 +1411,22 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 		mmc_hostname(host->mmc), intmask);
 
 	if (intmask & (SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE)) {
-		writel(intmask & (SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE),
-			host->ioaddr + SDHCI_INT_STATUS);
+		sdhci_writel(host, intmask & (SDHCI_INT_CARD_INSERT |
+			SDHCI_INT_CARD_REMOVE), SDHCI_INT_STATUS);
 		tasklet_schedule(&host->card_tasklet);
 	}
 
 	intmask &= ~(SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE);
 
 	if (intmask & SDHCI_INT_CMD_MASK) {
-		writel(intmask & SDHCI_INT_CMD_MASK,
-			host->ioaddr + SDHCI_INT_STATUS);
+		sdhci_writel(host, intmask & SDHCI_INT_CMD_MASK,
+			SDHCI_INT_STATUS);
 		sdhci_cmd_irq(host, intmask & SDHCI_INT_CMD_MASK);
 	}
 
 	if (intmask & SDHCI_INT_DATA_MASK) {
-		writel(intmask & SDHCI_INT_DATA_MASK,
-			host->ioaddr + SDHCI_INT_STATUS);
+		sdhci_writel(host, intmask & SDHCI_INT_DATA_MASK,
+			SDHCI_INT_STATUS);
 		sdhci_data_irq(host, intmask & SDHCI_INT_DATA_MASK);
 	}
 
@@ -1410,7 +1437,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 	if (intmask & SDHCI_INT_BUS_POWER) {
 		printk(KERN_ERR "%s: Card is consuming too much power!\n",
 			mmc_hostname(host->mmc));
-		writel(SDHCI_INT_BUS_POWER, host->ioaddr + SDHCI_INT_STATUS);
+		sdhci_writel(host, SDHCI_INT_BUS_POWER, SDHCI_INT_STATUS);
 	}
 
 	intmask &= ~SDHCI_INT_BUS_POWER;
@@ -1425,7 +1452,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 			mmc_hostname(host->mmc), intmask);
 		sdhci_dumpregs(host);
 
-		writel(intmask, host->ioaddr + SDHCI_INT_STATUS);
+		sdhci_writel(host, intmask, SDHCI_INT_STATUS);
 	}
 
 	result = IRQ_HANDLED;
@@ -1535,9 +1562,25 @@ int sdhci_add_host(struct sdhci_host *host)
 	if (debug_quirks)
 		host->quirks = debug_quirks;
 
+	if (host->ops->readl) {
+		host->readl = host->ops->readl;
+		host->readw = host->ops->readw;
+		host->readb = host->ops->readb;
+		host->writel = host->ops->writel;
+		host->writew = host->ops->writew;
+		host->writeb = host->ops->writeb;
+	} else {
+		host->readl = sdhci_def_readl;
+		host->readw = sdhci_def_readw;
+		host->readb = sdhci_def_readb;
+		host->writel = sdhci_def_writel;
+		host->writew = sdhci_def_writew;
+		host->writeb = sdhci_def_writeb;
+	}
+
 	sdhci_reset(host, SDHCI_RESET_ALL);
 
-	host->version = readw(host->ioaddr + SDHCI_HOST_VERSION);
+	host->version = sdhci_readw(host, SDHCI_HOST_VERSION);
 	host->version = (host->version & SDHCI_SPEC_VER_MASK)
 				>> SDHCI_SPEC_VER_SHIFT;
 	if (host->version > SDHCI_SPEC_200) {
@@ -1546,7 +1589,7 @@ int sdhci_add_host(struct sdhci_host *host)
 			host->version);
 	}
 
-	caps = readl(host->ioaddr + SDHCI_CAPABILITIES);
+	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
 
 	if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
 		host->flags |= SDHCI_USE_DMA;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 2d08dd4..8e4c4a6 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -9,6 +9,7 @@
  * your option) any later version.
  */
 
+#include <linux/types.h>
 #include <linux/scatterlist.h>
 
 /*
@@ -263,14 +264,61 @@ struct sdhci_host {
 
 	struct timer_list	timer;		/* Timer for timeouts */
 
+	/*
+	 * We have to duplicate a part of `struct sdhci_ops' since ->ops are
+	 * const, so the sdhci core won't able to assign default ops.
+	 */
+	u32		(*readl)(struct sdhci_host *host, int reg);
+	u16		(*readw)(struct sdhci_host *host, int reg);
+	u8		(*readb)(struct sdhci_host *host, int reg);
+	void		(*writel)(struct sdhci_host *host, u32 val, int reg);
+	void		(*writew)(struct sdhci_host *host, u16 val, int reg);
+	void		(*writeb)(struct sdhci_host *host, u8 val, int reg);
+
 	unsigned long		private[0] ____cacheline_aligned;
 };
 
 
 struct sdhci_ops {
+	u32		(*readl)(struct sdhci_host *host, int reg);
+	u16		(*readw)(struct sdhci_host *host, int reg);
+	u8		(*readb)(struct sdhci_host *host, int reg);
+	void		(*writel)(struct sdhci_host *host, u32 val, int reg);
+	void		(*writew)(struct sdhci_host *host, u16 val, int reg);
+	void		(*writeb)(struct sdhci_host *host, u8 val, int reg);
+
 	int		(*enable_dma)(struct sdhci_host *host);
 };
 
+static inline void sdhci_writel(struct sdhci_host *host, u32 val, int reg)
+{
+	host->writel(host, val, reg);
+}
+
+static inline void sdhci_writew(struct sdhci_host *host, u16 val, int reg)
+{
+	host->writew(host, val, reg);
+}
+
+static inline void sdhci_writeb(struct sdhci_host *host, u8 val, int reg)
+{
+	host->writeb(host, val, reg);
+}
+
+static inline u32 sdhci_readl(struct sdhci_host *host, int reg)
+{
+	return host->readl(host, reg);
+}
+
+static inline u16 sdhci_readw(struct sdhci_host *host, int reg)
+{
+	return host->readw(host, reg);
+}
+
+static inline u8 sdhci_readb(struct sdhci_host *host, int reg)
+{
+	return host->readb(host, reg);
+}
 
 extern struct sdhci_host *sdhci_alloc_host(struct device *dev,
 	size_t priv_size);
-- 
1.5.6.5

Re: [PATCH 02/11] sdhci: Add support for bus-specific IO memory accessors

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 1897 bytes --]

On Fri, 6 Feb 2009 21:06:45 +0300
Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> Currently the SDHCI driver works with PCI accessors (write{l,b,w} and
> read{l,b,w}).
> 
> With this patch drivers may change memory accessors, so that we can
> support hosts with "weird" IO memory access requirments.
> 
> For example, in "FSL eSDHC" SDHCI hardware all registers are 32 bit
> width, with big-endian addressing. That is, readb(0x2f) should turn
> into readb(0x2c), and readw(0x2c) should be translated to
> le16_to_cpu(readw(0x2e)).
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---

I was hoping we wouldn't have to do a lot of magic in the accessors
since the spec is rather clear on the register interface. :/

Let's see if I've understood this correctly.

1. The CPU is big-endian but the register are little-endian (as the
spec requires). I was under the impression that the read*/write*
accessor handled any endian conversion between the bus and the cpu? How
do e.g. PCI work on Sparc?

2. Register access must be done 32 bits at a time. Now this is just
broken and might cause big problems as some registers cannot just be
read and written back to. OTOH you refer to readw() in your example,
not readl(). What's the deal here?

> +static inline void sdhci_writel(struct sdhci_host *host, u32 val, int reg)
> +{
> +	host->writel(host, val, reg);
> +}

Having to override these are worst case scenario as far as I'm
concerned, so I'd prefer something like:

if (!host->ops->writel)
	writel(host->ioaddr + reg, val);
else
	host->ops->writel(host, val, reg);

and maybe even a likely() up there.

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 02/11] sdhci: Add support for bus-specific IO memory accessors

[Anton Vorontsov]

On Sun, Feb 08, 2009 at 09:50:20PM +0100, Pierre Ossman wrote:
> On Fri, 6 Feb 2009 21:06:45 +0300
> Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> > Currently the SDHCI driver works with PCI accessors (write{l,b,w} and
> > read{l,b,w}).
> > 
> > With this patch drivers may change memory accessors, so that we can
> > support hosts with "weird" IO memory access requirments.
> > 
> > For example, in "FSL eSDHC" SDHCI hardware all registers are 32 bit
> > width, with big-endian addressing. That is, readb(0x2f) should turn
> > into readb(0x2c), and readw(0x2c) should be translated to
> > le16_to_cpu(readw(0x2e)).
> > 
> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > ---
> 
> I was hoping we wouldn't have to do a lot of magic in the accessors
> since the spec is rather clear on the register interface. :/
> 
> Let's see if I've understood this correctly.
> 
> 1. The CPU is big-endian but the register are little-endian (as the
> spec requires).

No, on eSDHC the registers are big-endian, 32-bit width, with, for
example, two 16-bit "logical" registers packed into it.

That is,

 0x4  0x5 0x6  0x7
|~~~~~~~~:~~~~~~~~|
| BLKCNT : BLKSZ  |
|________:________|
 31              0

( The register looks wrong, right? BLKSZ should be at 0x4. But imagine
  that you swapped bytes in this 32 bit register... then the registers
  and their byte addresses will look normal. )

So if we try to issue readw(SDHCI_BLOCK_SIZE), i.e. readw(0x4):

- We'll read BLKCNT, while we wanted BLKSZ. This is because the
  address bits should be translated before we try word or byte
  reads/writes.
- On powerpc read{l,w}() convert the read value from little-endian
  to big-endian byte order, which is wrong for our case (the
  register is big-endian already).

That means that we have to convert address, but we don't want to
convert the result of read/write ops.

> I was under the impression that the read*/write*
> accessor handled any endian conversion between the bus and the cpu? How
> do e.g. PCI work on Sparc?

read{l,w} are guaranteed to return values in CPU byte order, so
if CPU is in big-endian mode, then the PCI IO accessors should
convert values. And just as on PowerPC, Sparc's read*() accessors
swap bytes of a result:

static inline u32 __readl(const volatile void __iomem *addr)
{
        return flip_dword(*(__force volatile u32 *)addr);
}

#define readl(__addr)           __readl(__addr)

> 2. Register access must be done 32 bits at a time. Now this is just
> broken and might cause big problems as some registers cannot just be
> read and written back to.

We must only take special care when working with "triggering"
registers, and that's handled by the "sdhci: Add support for hosts
with strict 32 bit addressing" patch.

> OTOH you refer to readw() in your example,
> not readl(). What's the deal here?

readw() was just an example (most complicated one).

> > +static inline void sdhci_writel(struct sdhci_host *host, u32 val, int reg)
> > +{
> > +	host->writel(host, val, reg);
> > +}
> 
> Having to override these are worst case scenario

Hm. It's not a worst case scenario, it's a normal scenario for
eSDHC. Why should we treat eSDHC as a second-class citizen?

> as far as I'm
> concerned, so I'd prefer something like:
> 
> if (!host->ops->writel)
> 	writel(host->ioaddr + reg, val);
> else
> 	host->ops->writel(host, val, reg);

Hm.

-- What I purpose:

$ size drivers/mmc/host/sdhci.o
   text    data     bss     dec     hex filename
  15173       8       4   15185    3b51 drivers/mmc/host/sdhci.o

And there is a minimum run-time overhead (dereference + branch).

+ no first/second-class citizen separation.

-- What you purpose (inlined):

$ size drivers/mmc/host/sdhci.o
   text    data     bss     dec     hex filename
  17853       8       4   17865    45c9 drivers/mmc/host/sdhci.o

Runtime overhead: dereference + dereference + compare +
(maybe)branch + larger code.

-- What you purpose (uninlined):

$ size drivers/mmc/host/sdhci.o
   text    data     bss     dec     hex filename
  14692       8       4   14704    3970 drivers/mmc/host/sdhci.o

Better. But the runtime overhead: branch + dereference + dereference +
compare + (maybe)branch.


Surely the overhead isn't measurable... but why we purposely make
things worse?

Though, this is not something I'm going to argue about, I'll just
do it the way you prefer. ;-) For an updated patch set I took
the uninlined variant, hope this is OK.

Thanks for the review,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 02/11] sdhci: Add support for bus-specific IO memory accessors

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 2701 bytes --]

On Fri, 13 Feb 2009 17:40:39 +0300
Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> 
> No, on eSDHC the registers are big-endian, 32-bit width, with, for
> example, two 16-bit "logical" registers packed into it.
> 
> That is,
> 
>  0x4  0x5 0x6  0x7
> |~~~~~~~~:~~~~~~~~|
> | BLKCNT : BLKSZ  |
> |________:________|
>  31              0
> 
> ( The register looks wrong, right? BLKSZ should be at 0x4. But imagine
>   that you swapped bytes in this 32 bit register... then the registers
>   and their byte addresses will look normal. )
> 
> So if we try to issue readw(SDHCI_BLOCK_SIZE), i.e. readw(0x4):
> 
> - We'll read BLKCNT, while we wanted BLKSZ. This is because the
>   address bits should be translated before we try word or byte
>   reads/writes.
> - On powerpc read{l,w}() convert the read value from little-endian
>   to big-endian byte order, which is wrong for our case (the
>   register is big-endian already).
> 
> That means that we have to convert address, but we don't want to
> convert the result of read/write ops.
> 

*cries*

Now this is just incredibly horrible. Why the hell did they try to use
the sdhci interface and then do stupid things like this?

> > > +static inline void sdhci_writel(struct sdhci_host *host, u32 val, int reg)
> > > +{
> > > +	host->writel(host, val, reg);
> > > +}
> > 
> > Having to override these are worst case scenario
> 
> Hm. It's not a worst case scenario, it's a normal scenario for
> eSDHC. Why should we treat eSDHC as a second-class citizen?
> 

Because it's complete and utter crap. Freescale has completely ignored
the basic register interface requirements of the SDHCI spec. Treating
eSDHC as a second-class citizen is generous IMO.

> > as far as I'm
> > concerned, so I'd prefer something like:
> > 
> > if (!host->ops->writel)
> > 	writel(host->ioaddr + reg, val);
> > else
> > 	host->ops->writel(host, val, reg);
> 
> Surely the overhead isn't measurable... but why we purposely make
> things worse?
> 

We can most likely do some micro-optimisation do make the compare part
cheaper, but the point was to avoid a function call for all the
properly implemented controllers out there. We could have a flag so
that it only has to check host->flags, which will most likely be in the
cache anyway.

Overhead for eSDHC is not a concern in my book, what is interesting is
how much this change slows things down for other controllers.

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 02/11] sdhci: Add support for bus-specific IO memory accessors

[Anton Vorontsov]

On Sat, Feb 21, 2009 at 04:57:57PM +0100, Pierre Ossman wrote:
> On Fri, 13 Feb 2009 17:40:39 +0300
> Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> 
> > 
> > No, on eSDHC the registers are big-endian, 32-bit width, with, for
> > example, two 16-bit "logical" registers packed into it.
> > 
> > That is,
> > 
> >  0x4  0x5 0x6  0x7
> > |~~~~~~~~:~~~~~~~~|
> > | BLKCNT : BLKSZ  |
> > |________:________|
> >  31              0
> > 
> > ( The register looks wrong, right? BLKSZ should be at 0x4. But imagine
> >   that you swapped bytes in this 32 bit register... then the registers
> >   and their byte addresses will look normal. )
> > 
> > So if we try to issue readw(SDHCI_BLOCK_SIZE), i.e. readw(0x4):
> > 
> > - We'll read BLKCNT, while we wanted BLKSZ. This is because the
> >   address bits should be translated before we try word or byte
> >   reads/writes.
> > - On powerpc read{l,w}() convert the read value from little-endian
> >   to big-endian byte order, which is wrong for our case (the
> >   register is big-endian already).
> > 
> > That means that we have to convert address, but we don't want to
> > convert the result of read/write ops.
> > 
> 
> *cries*

:-)

[...]
> > > as far as I'm
> > > concerned, so I'd prefer something like:
> > > 
> > > if (!host->ops->writel)
> > > 	writel(host->ioaddr + reg, val);
> > > else
> > > 	host->ops->writel(host, val, reg);
> > 
> > Surely the overhead isn't measurable... but why we purposely make
> > things worse?
> > 
> 
> We can most likely do some micro-optimisation do make the compare part
> cheaper, but the point was to avoid a function call for all the
> properly implemented controllers out there. We could have a flag so
> that it only has to check host->flags, which will most likely be in the
> cache anyway.
> 
> Overhead for eSDHC is not a concern in my book, what is interesting is
> how much this change slows things down for other controllers.

OK, I see. Will the patch down below make you a little bit more happy
wrt normal controllers? Two #ifdefs, but then there is absolutely
zero overhead for the fully compliant SDHCI controllers.

(So far it's just on top of this series, but I can incorporate it
into the "sdhci: Add support for bus-specific IO memory accessors"
patch, if you like).

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 73b79e1..69bd124 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -37,6 +37,13 @@ config MMC_SDHCI
 
 	  If unsure, say N.
 
+config MMC_SDHCI_IO_ACCESSORS
+	bool
+	depends on MMC_SDHCI
+	help
+	  This is silent Kconfig symbol that is selected by the drivers that
+	  need to overwrite SDHCI IO memory accessors.
+
 config MMC_SDHCI_PCI
 	tristate "SDHCI support on PCI bus"
 	depends on MMC_SDHCI && PCI
@@ -68,6 +75,7 @@ config MMC_RICOH_MMC
 config MMC_SDHCI_OF
 	tristate "SDHCI support on OpenFirmware platforms"
 	depends on MMC_SDHCI && PPC_OF
+	select MMC_SDHCI_IO_ACCESSORS
 	help
 	  This selects the OF support for Secure Digital Host Controller
 	  Interfaces. So far, only the Freescale eSDHC controller is known
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 284bc5d..fb08d3b 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -88,60 +88,6 @@ static void sdhci_dumpregs(struct sdhci_host *host)
  *                                                                           *
 \*****************************************************************************/
 
-void sdhci_writel(struct sdhci_host *host, u32 val, int reg)
-{
-	if (unlikely(host->ops->writel))
-		host->ops->writel(host, val, reg);
-	else
-		writel(val, host->ioaddr + reg);
-}
-EXPORT_SYMBOL_GPL(sdhci_writel);
-
-void sdhci_writew(struct sdhci_host *host, u16 val, int reg)
-{
-	if (unlikely(host->ops->writew))
-		host->ops->writew(host, val, reg);
-	else
-		writew(val, host->ioaddr + reg);
-}
-EXPORT_SYMBOL_GPL(sdhci_writew);
-
-void sdhci_writeb(struct sdhci_host *host, u8 val, int reg)
-{
-	if (unlikely(host->ops->writeb))
-		host->ops->writeb(host, val, reg);
-	else
-		writeb(val, host->ioaddr + reg);
-}
-EXPORT_SYMBOL_GPL(sdhci_writeb);
-
-u32 sdhci_readl(struct sdhci_host *host, int reg)
-{
-	if (unlikely(host->ops->readl))
-		return host->ops->readl(host, reg);
-	else
-		return readl(host->ioaddr + reg);
-}
-EXPORT_SYMBOL_GPL(sdhci_readl);
-
-u16 sdhci_readw(struct sdhci_host *host, int reg)
-{
-	if (unlikely(host->ops->readw))
-		return host->ops->readw(host, reg);
-	else
-		return readw(host->ioaddr + reg);
-}
-EXPORT_SYMBOL_GPL(sdhci_readw);
-
-u8 sdhci_readb(struct sdhci_host *host, int reg)
-{
-	if (unlikely(host->ops->readb))
-		return host->ops->readb(host, reg);
-	else
-		return readb(host->ioaddr + reg);
-}
-EXPORT_SYMBOL_GPL(sdhci_readb);
-
 static void sdhci_clear_set_irqs(struct sdhci_host *host, u32 clear, u32 set)
 {
 	u32 ier;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 1697e01..b6675df 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -281,12 +281,14 @@ struct sdhci_host {
 
 
 struct sdhci_ops {
+#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
 	u32		(*readl)(struct sdhci_host *host, int reg);
 	u16		(*readw)(struct sdhci_host *host, int reg);
 	u8		(*readb)(struct sdhci_host *host, int reg);
 	void		(*writel)(struct sdhci_host *host, u32 val, int reg);
 	void		(*writew)(struct sdhci_host *host, u16 val, int reg);
 	void		(*writeb)(struct sdhci_host *host, u8 val, int reg);
+#endif
 
 	void	(*set_clock)(struct sdhci_host *host, unsigned int clock);
 
@@ -295,12 +297,89 @@ struct sdhci_ops {
 	unsigned int	(*get_timeout_clock)(struct sdhci_host *host);
 };
 
-extern void sdhci_writel(struct sdhci_host *host, u32 val, int reg);
-extern void sdhci_writew(struct sdhci_host *host, u16 val, int reg);
-extern void sdhci_writeb(struct sdhci_host *host, u8 val, int reg);
-extern u32 sdhci_readl(struct sdhci_host *host, int reg);
-extern u16 sdhci_readw(struct sdhci_host *host, int reg);
-extern u8 sdhci_readb(struct sdhci_host *host, int reg);
+#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
+
+static inline void sdhci_writel(struct sdhci_host *host, u32 val, int reg)
+{
+	if (unlikely(host->ops->writel))
+		host->ops->writel(host, val, reg);
+	else
+		writel(val, host->ioaddr + reg);
+}
+
+static inline void sdhci_writew(struct sdhci_host *host, u16 val, int reg)
+{
+	if (unlikely(host->ops->writew))
+		host->ops->writew(host, val, reg);
+	else
+		writew(val, host->ioaddr + reg);
+}
+
+static inline void sdhci_writeb(struct sdhci_host *host, u8 val, int reg)
+{
+	if (unlikely(host->ops->writeb))
+		host->ops->writeb(host, val, reg);
+	else
+		writeb(val, host->ioaddr + reg);
+}
+
+static inline u32 sdhci_readl(struct sdhci_host *host, int reg)
+{
+	if (unlikely(host->ops->readl))
+		return host->ops->readl(host, reg);
+	else
+		return readl(host->ioaddr + reg);
+}
+
+static inline u16 sdhci_readw(struct sdhci_host *host, int reg)
+{
+	if (unlikely(host->ops->readw))
+		return host->ops->readw(host, reg);
+	else
+		return readw(host->ioaddr + reg);
+}
+
+static inline u8 sdhci_readb(struct sdhci_host *host, int reg)
+{
+	if (unlikely(host->ops->readb))
+		return host->ops->readb(host, reg);
+	else
+		return readb(host->ioaddr + reg);
+}
+
+#else
+
+static inline void sdhci_writel(struct sdhci_host *host, u32 val, int reg)
+{
+	writel(val, host->ioaddr + reg);
+}
+
+static inline void sdhci_writew(struct sdhci_host *host, u16 val, int reg)
+{
+	writew(val, host->ioaddr + reg);
+}
+
+static inline void sdhci_writeb(struct sdhci_host *host, u8 val, int reg)
+{
+	writeb(val, host->ioaddr + reg);
+}
+
+static inline u32 sdhci_readl(struct sdhci_host *host, int reg)
+{
+	return readl(host->ioaddr + reg);
+}
+
+static inline u16 sdhci_readw(struct sdhci_host *host, int reg)
+{
+	return readw(host->ioaddr + reg);
+}
+
+static inline u8 sdhci_readb(struct sdhci_host *host, int reg)
+{
+	return readb(host->ioaddr + reg);
+}
+
+#endif /* CONFIG_MMC_SDHCI_IO_ACCESSORS */
 
 extern struct sdhci_host *sdhci_alloc_host(struct device *dev,
 	size_t priv_size);

Re: [PATCH 02/11] sdhci: Add support for bus-specific IO memory accessors

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 1588 bytes --]

On Wed, 4 Mar 2009 20:46:58 +0300
Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> On Sat, Feb 21, 2009 at 04:57:57PM +0100, Pierre Ossman wrote:
> > 
> > We can most likely do some micro-optimisation do make the compare part
> > cheaper, but the point was to avoid a function call for all the
> > properly implemented controllers out there. We could have a flag so
> > that it only has to check host->flags, which will most likely be in the
> > cache anyway.
> > 
> > Overhead for eSDHC is not a concern in my book, what is interesting is
> > how much this change slows things down for other controllers.
> 
> OK, I see. Will the patch down below make you a little bit more happy
> wrt normal controllers? Two #ifdefs, but then there is absolutely
> zero overhead for the fully compliant SDHCI controllers.
> 

I can't say this makes me happy either, but I think it's acceptable for
now so that we can move forward. I'd like a common code path for this
thing, but I think I'm going to have to put a bit more time into it
myself than I currently have available.

> (So far it's just on top of this series, but I can incorporate it
> into the "sdhci: Add support for bus-specific IO memory accessors"
> patch, if you like).
> 

Please do. Have one patch add some code and another remove it in the
same set is just silly. :)

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 03/11] sdhci: Add type checking for IO memory accessors

[Anton Vorontsov]

A new restricted integer type introduced: sdhci_reg_t.

Header file now specifies registers via sdhci_reg() inline function.
Only one place (not counting sdhci_def_*() accessors) need to cast
a register back to an offset, i.e. sdhci_finish_command().

>From now on sparse tool will warn about IO memory accessors misuses,
for exampple:

sdhci_writeb(host, SDHCI_TIMEOUT_CONTROL, count);

  CHECK   sdhci.c
sdhci.c:614:21: warning: incorrect type in argument 2 (different base types)
sdhci.c:614:21:    expected unsigned char [unsigned] [usertype] val
sdhci.c:614:21:    got restricted int
sdhci.c:614:44: warning: incorrect type in argument 3 (different base types)
sdhci.c:614:44:    expected restricted int [usertype] reg
sdhci.c:614:44:    got unsigned char [unsigned] [assigned] [usertype] count

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/mmc/host/sdhci.c |   32 +++++++------
 drivers/mmc/host/sdhci.h |  113 ++++++++++++++++++++++++++-------------------
 2 files changed, 82 insertions(+), 63 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 8557521..b7a79a0 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -87,34 +87,34 @@ static void sdhci_dumpregs(struct sdhci_host *host)
  *                                                                           *
 \*****************************************************************************/
 
-static u32 sdhci_def_readl(struct sdhci_host *host, int reg)
+static u32 sdhci_def_readl(struct sdhci_host *host, sdhci_reg_t reg)
 {
-	return readl(host->ioaddr + reg);
+	return readl(host->ioaddr + sdhci_off(reg));
 }
 
-static u16 sdhci_def_readw(struct sdhci_host *host, int reg)
+static u16 sdhci_def_readw(struct sdhci_host *host, sdhci_reg_t reg)
 {
-	return readw(host->ioaddr + reg);
+	return readw(host->ioaddr + sdhci_off(reg));
 }
 
-static u8 sdhci_def_readb(struct sdhci_host *host, int reg)
+static u8 sdhci_def_readb(struct sdhci_host *host, sdhci_reg_t reg)
 {
-	return readb(host->ioaddr + reg);
+	return readb(host->ioaddr + sdhci_off(reg));
 }
 
-static void sdhci_def_writel(struct sdhci_host *host, u32 val, int reg)
+static void sdhci_def_writel(struct sdhci_host *host, u32 val, sdhci_reg_t reg)
 {
-	writel(val, host->ioaddr + reg);
+	writel(val, host->ioaddr + sdhci_off(reg));
 }
 
-static void sdhci_def_writew(struct sdhci_host *host, u16 val, int reg)
+static void sdhci_def_writew(struct sdhci_host *host, u16 val, sdhci_reg_t reg)
 {
-	writew(val, host->ioaddr + reg);
+	writew(val, host->ioaddr + sdhci_off(reg));
 }
 
-static void sdhci_def_writeb(struct sdhci_host *host, u8 val, int reg)
+static void sdhci_def_writeb(struct sdhci_host *host, u8 val, sdhci_reg_t reg)
 {
-	writeb(val, host->ioaddr + reg);
+	writeb(val, host->ioaddr + sdhci_off(reg));
 }
 
 static void sdhci_reset(struct sdhci_host *host, u8 mask)
@@ -891,12 +891,14 @@ static void sdhci_finish_command(struct sdhci_host *host)
 		if (host->cmd->flags & MMC_RSP_136) {
 			/* CRC is stripped so we need to do some shifting. */
 			for (i = 0;i < 4;i++) {
+				int resp = sdhci_off(SDHCI_RESPONSE);
+
 				host->cmd->resp[i] = sdhci_readl(host,
-					SDHCI_RESPONSE + (3-i)*4) << 8;
+					sdhci_reg(resp + (3-i)*4)) << 8;
 				if (i != 3)
 					host->cmd->resp[i] |=
-						sdhci_readb(host,
-						SDHCI_RESPONSE + (3-i)*4-1);
+						sdhci_readb(host, sdhci_reg(
+							resp + (3-i)*4-1));
 			}
 		} else {
 			host->cmd->resp[0] = sdhci_readl(host, SDHCI_RESPONSE);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 8e4c4a6..5a5c119 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -12,27 +12,41 @@
 #include <linux/types.h>
 #include <linux/scatterlist.h>
 
+typedef int __bitwise__ sdhci_reg_t;
+
+/* Cast pure integer to register. */
+static inline sdhci_reg_t sdhci_reg(int off)
+{
+	return (sdhci_reg_t __force)off;
+}
+
+/* Cast register to integer (for use with arithmetics). */
+static inline int sdhci_off(sdhci_reg_t reg)
+{
+	return (int __force)reg;
+}
+
 /*
  * Controller registers
  */
 
-#define SDHCI_DMA_ADDRESS	0x00
+#define SDHCI_DMA_ADDRESS	sdhci_reg(0x00)
 
-#define SDHCI_BLOCK_SIZE	0x04
+#define SDHCI_BLOCK_SIZE	sdhci_reg(0x04)
 #define  SDHCI_MAKE_BLKSZ(dma, blksz) (((dma & 0x7) << 12) | (blksz & 0xFFF))
 
-#define SDHCI_BLOCK_COUNT	0x06
+#define SDHCI_BLOCK_COUNT	sdhci_reg(0x06)
 
-#define SDHCI_ARGUMENT		0x08
+#define SDHCI_ARGUMENT		sdhci_reg(0x08)
 
-#define SDHCI_TRANSFER_MODE	0x0C
+#define SDHCI_TRANSFER_MODE	sdhci_reg(0x0C)
 #define  SDHCI_TRNS_DMA		0x01
 #define  SDHCI_TRNS_BLK_CNT_EN	0x02
 #define  SDHCI_TRNS_ACMD12	0x04
 #define  SDHCI_TRNS_READ	0x10
 #define  SDHCI_TRNS_MULTI	0x20
 
-#define SDHCI_COMMAND		0x0E
+#define SDHCI_COMMAND		sdhci_reg(0x0E)
 #define  SDHCI_CMD_RESP_MASK	0x03
 #define  SDHCI_CMD_CRC		0x08
 #define  SDHCI_CMD_INDEX	0x10
@@ -45,11 +59,11 @@
 
 #define SDHCI_MAKE_CMD(c, f) (((c & 0xff) << 8) | (f & 0xff))
 
-#define SDHCI_RESPONSE		0x10
+#define SDHCI_RESPONSE		sdhci_reg(0x10)
 
-#define SDHCI_BUFFER		0x20
+#define SDHCI_BUFFER		sdhci_reg(0x20)
 
-#define SDHCI_PRESENT_STATE	0x24
+#define SDHCI_PRESENT_STATE	sdhci_reg(0x24)
 #define  SDHCI_CMD_INHIBIT	0x00000001
 #define  SDHCI_DATA_INHIBIT	0x00000002
 #define  SDHCI_DOING_WRITE	0x00000100
@@ -59,7 +73,7 @@
 #define  SDHCI_CARD_PRESENT	0x00010000
 #define  SDHCI_WRITE_PROTECT	0x00080000
 
-#define SDHCI_HOST_CONTROL 	0x28
+#define SDHCI_HOST_CONTROL 	sdhci_reg(0x28)
 #define  SDHCI_CTRL_LED		0x01
 #define  SDHCI_CTRL_4BITBUS	0x02
 #define  SDHCI_CTRL_HISPD	0x04
@@ -69,32 +83,32 @@
 #define   SDHCI_CTRL_ADMA32	0x10
 #define   SDHCI_CTRL_ADMA64	0x18
 
-#define SDHCI_POWER_CONTROL	0x29
+#define SDHCI_POWER_CONTROL	sdhci_reg(0x29)
 #define  SDHCI_POWER_ON		0x01
 #define  SDHCI_POWER_180	0x0A
 #define  SDHCI_POWER_300	0x0C
 #define  SDHCI_POWER_330	0x0E
 
-#define SDHCI_BLOCK_GAP_CONTROL	0x2A
+#define SDHCI_BLOCK_GAP_CONTROL	sdhci_reg(0x2A)
 
-#define SDHCI_WAKE_UP_CONTROL	0x2B
+#define SDHCI_WAKE_UP_CONTROL	sdhci_reg(0x2B)
 
-#define SDHCI_CLOCK_CONTROL	0x2C
+#define SDHCI_CLOCK_CONTROL	sdhci_reg(0x2C)
 #define  SDHCI_DIVIDER_SHIFT	8
 #define  SDHCI_CLOCK_CARD_EN	0x0004
 #define  SDHCI_CLOCK_INT_STABLE	0x0002
 #define  SDHCI_CLOCK_INT_EN	0x0001
 
-#define SDHCI_TIMEOUT_CONTROL	0x2E
+#define SDHCI_TIMEOUT_CONTROL	sdhci_reg(0x2E)
 
-#define SDHCI_SOFTWARE_RESET	0x2F
+#define SDHCI_SOFTWARE_RESET	sdhci_reg(0x2F)
 #define  SDHCI_RESET_ALL	0x01
 #define  SDHCI_RESET_CMD	0x02
 #define  SDHCI_RESET_DATA	0x04
 
-#define SDHCI_INT_STATUS	0x30
-#define SDHCI_INT_ENABLE	0x34
-#define SDHCI_SIGNAL_ENABLE	0x38
+#define SDHCI_INT_STATUS	sdhci_reg(0x30)
+#define SDHCI_INT_ENABLE	sdhci_reg(0x34)
+#define SDHCI_SIGNAL_ENABLE	sdhci_reg(0x38)
 #define  SDHCI_INT_RESPONSE	0x00000001
 #define  SDHCI_INT_DATA_END	0x00000002
 #define  SDHCI_INT_DMA_END	0x00000008
@@ -125,11 +139,11 @@
 		SDHCI_INT_DATA_TIMEOUT | SDHCI_INT_DATA_CRC | \
 		SDHCI_INT_DATA_END_BIT)
 
-#define SDHCI_ACMD12_ERR	0x3C
+#define SDHCI_ACMD12_ERR	sdhci_reg(0x3C)
 
 /* 3E-3F reserved */
 
-#define SDHCI_CAPABILITIES	0x40
+#define SDHCI_CAPABILITIES	sdhci_reg(0x40)
 #define  SDHCI_TIMEOUT_CLK_MASK	0x0000003F
 #define  SDHCI_TIMEOUT_CLK_SHIFT 0
 #define  SDHCI_TIMEOUT_CLK_UNIT	0x00000080
@@ -148,24 +162,24 @@
 
 /* 44-47 reserved for more caps */
 
-#define SDHCI_MAX_CURRENT	0x48
+#define SDHCI_MAX_CURRENT	sdhci_reg(0x48)
 
 /* 4C-4F reserved for more max current */
 
-#define SDHCI_SET_ACMD12_ERROR	0x50
-#define SDHCI_SET_INT_ERROR	0x52
+#define SDHCI_SET_ACMD12_ERROR	sdhci_reg(0x50)
+#define SDHCI_SET_INT_ERROR	sdhci_reg(0x52)
 
-#define SDHCI_ADMA_ERROR	0x54
+#define SDHCI_ADMA_ERROR	sdhci_reg(0x54)
 
 /* 55-57 reserved */
 
-#define SDHCI_ADMA_ADDRESS	0x58
+#define SDHCI_ADMA_ADDRESS	sdhci_reg(0x58)
 
 /* 60-FB reserved */
 
-#define SDHCI_SLOT_INT_STATUS	0xFC
+#define SDHCI_SLOT_INT_STATUS	sdhci_reg(0xFC)
 
-#define SDHCI_HOST_VERSION	0xFE
+#define SDHCI_HOST_VERSION	sdhci_reg(0xFE)
 #define  SDHCI_VENDOR_VER_MASK	0xFF00
 #define  SDHCI_VENDOR_VER_SHIFT	8
 #define  SDHCI_SPEC_VER_MASK	0x00FF
@@ -268,54 +282,57 @@ struct sdhci_host {
 	 * We have to duplicate a part of `struct sdhci_ops' since ->ops are
 	 * const, so the sdhci core won't able to assign default ops.
 	 */
-	u32		(*readl)(struct sdhci_host *host, int reg);
-	u16		(*readw)(struct sdhci_host *host, int reg);
-	u8		(*readb)(struct sdhci_host *host, int reg);
-	void		(*writel)(struct sdhci_host *host, u32 val, int reg);
-	void		(*writew)(struct sdhci_host *host, u16 val, int reg);
-	void		(*writeb)(struct sdhci_host *host, u8 val, int reg);
+	u32	(*readl)(struct sdhci_host *host, sdhci_reg_t reg);
+	u16	(*readw)(struct sdhci_host *host, sdhci_reg_t reg);
+	u8	(*readb)(struct sdhci_host *host, sdhci_reg_t reg);
+	void	(*writel)(struct sdhci_host *host, u32 val, sdhci_reg_t reg);
+	void	(*writew)(struct sdhci_host *host, u16 val, sdhci_reg_t reg);
+	void	(*writeb)(struct sdhci_host *host, u8 val, sdhci_reg_t reg);
 
 	unsigned long		private[0] ____cacheline_aligned;
 };
 
 
 struct sdhci_ops {
-	u32		(*readl)(struct sdhci_host *host, int reg);
-	u16		(*readw)(struct sdhci_host *host, int reg);
-	u8		(*readb)(struct sdhci_host *host, int reg);
-	void		(*writel)(struct sdhci_host *host, u32 val, int reg);
-	void		(*writew)(struct sdhci_host *host, u16 val, int reg);
-	void		(*writeb)(struct sdhci_host *host, u8 val, int reg);
-
-	int		(*enable_dma)(struct sdhci_host *host);
+	u32	(*readl)(struct sdhci_host *host, sdhci_reg_t reg);
+	u16	(*readw)(struct sdhci_host *host, sdhci_reg_t reg);
+	u8	(*readb)(struct sdhci_host *host, sdhci_reg_t reg);
+	void	(*writel)(struct sdhci_host *host, u32 val, sdhci_reg_t reg);
+	void	(*writew)(struct sdhci_host *host, u16 val, sdhci_reg_t reg);
+	void	(*writeb)(struct sdhci_host *host, u8 val, sdhci_reg_t reg);
+
+	int	(*enable_dma)(struct sdhci_host *host);
 };
 
-static inline void sdhci_writel(struct sdhci_host *host, u32 val, int reg)
+static inline void sdhci_writel(struct sdhci_host *host, u32 val,
+				sdhci_reg_t reg)
 {
 	host->writel(host, val, reg);
 }
 
-static inline void sdhci_writew(struct sdhci_host *host, u16 val, int reg)
+static inline void sdhci_writew(struct sdhci_host *host, u16 val,
+				sdhci_reg_t reg)
 {
 	host->writew(host, val, reg);
 }
 
-static inline void sdhci_writeb(struct sdhci_host *host, u8 val, int reg)
+static inline void sdhci_writeb(struct sdhci_host *host, u8 val,
+				sdhci_reg_t reg)
 {
 	host->writeb(host, val, reg);
 }
 
-static inline u32 sdhci_readl(struct sdhci_host *host, int reg)
+static inline u32 sdhci_readl(struct sdhci_host *host, sdhci_reg_t reg)
 {
 	return host->readl(host, reg);
 }
 
-static inline u16 sdhci_readw(struct sdhci_host *host, int reg)
+static inline u16 sdhci_readw(struct sdhci_host *host, sdhci_reg_t reg)
 {
 	return host->readw(host, reg);
 }
 
-static inline u8 sdhci_readb(struct sdhci_host *host, int reg)
+static inline u8 sdhci_readb(struct sdhci_host *host, sdhci_reg_t reg)
 {
 	return host->readb(host, reg);
 }
-- 
1.5.6.5

Re: [PATCH 03/11] sdhci: Add type checking for IO memory accessors

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 1375 bytes --]

On Fri, 6 Feb 2009 21:06:48 +0300
Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> A new restricted integer type introduced: sdhci_reg_t.
> 
> Header file now specifies registers via sdhci_reg() inline function.
> Only one place (not counting sdhci_def_*() accessors) need to cast
> a register back to an offset, i.e. sdhci_finish_command().
> 
> From now on sparse tool will warn about IO memory accessors misuses,
> for exampple:
> 
> sdhci_writeb(host, SDHCI_TIMEOUT_CONTROL, count);
> 
>   CHECK   sdhci.c
> sdhci.c:614:21: warning: incorrect type in argument 2 (different base types)
> sdhci.c:614:21:    expected unsigned char [unsigned] [usertype] val
> sdhci.c:614:21:    got restricted int
> sdhci.c:614:44: warning: incorrect type in argument 3 (different base types)
> sdhci.c:614:44:    expected restricted int [usertype] reg
> sdhci.c:614:44:    got unsigned char [unsigned] [assigned] [usertype] count
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---

Is this really a problem? It's a lot of noise in the code and I can't
really see this as a major issue, or even a minor one. :)

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 04/11] sdhci: Add support for card-detection polling

[Anton Vorontsov]

This patch adds SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk. When specified,
sdhci driver will set MMC_CAP_NEEDS_POLL MMC host capability, and won't
enable card insert/remove interrupts.

This is needed for hosts with unreliable card detection, such as FSL
eSDHC. The original eSDHC driver was tring to "debounce" card-detection
IRQs by reading present state and disabling particular interrupts. But
with this debouncing scheme I noticed that sometimes we miss card
insertion/removal events.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/mmc/host/sdhci.c |    6 ++++++
 drivers/mmc/host/sdhci.h |    2 ++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index b7a79a0..45c5f1f 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -162,6 +162,9 @@ static void sdhci_init(struct sdhci_host *host)
 		SDHCI_INT_DMA_END | SDHCI_INT_DATA_END | SDHCI_INT_RESPONSE |
 		SDHCI_INT_ADMA_ERROR;
 
+	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
+		intmask &= ~(SDHCI_INT_CARD_REMOVE | SDHCI_INT_CARD_INSERT);
+
 	sdhci_writel(host, intmask, SDHCI_INT_ENABLE);
 	sdhci_writel(host, intmask, SDHCI_SIGNAL_ENABLE);
 }
@@ -1684,6 +1687,9 @@ int sdhci_add_host(struct sdhci_host *host)
 	mmc->f_max = host->max_clk;
 	mmc->caps = MMC_CAP_4_BIT_DATA | MMC_CAP_SDIO_IRQ;
 
+	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
+		mmc->caps |= MMC_CAP_NEEDS_POLL;
+
 	if ((caps & SDHCI_CAN_DO_HISPD) ||
 		(host->quirks & SDHCI_QUIRK_FORCE_HIGHSPEED))
 		mmc->caps |= MMC_CAP_SD_HIGHSPEED;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 5a5c119..03f035e 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -227,6 +227,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_FORCE_HIGHSPEED			(1<<14)
 /* Controller does not provide transfer-complete interrupt when not busy */
 #define SDHCI_QUIRK_NO_BUSY_IRQ				(1<<15)
+/* Controller has unreliable card detection */
+#define SDHCI_QUIRK_BROKEN_CARD_DETECTION		(1<<16)
 
 	int			irq;		/* Device IRQ */
 	void __iomem *		ioaddr;		/* Mapped address */
-- 
1.5.6.5

Re: [PATCH 04/11] sdhci: Add support for card-detection polling

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 1802 bytes --]

On Fri, 6 Feb 2009 21:06:50 +0300
Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> This patch adds SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk. When specified,
> sdhci driver will set MMC_CAP_NEEDS_POLL MMC host capability, and won't
> enable card insert/remove interrupts.
> 
> This is needed for hosts with unreliable card detection, such as FSL
> eSDHC. The original eSDHC driver was tring to "debounce" card-detection
> IRQs by reading present state and disabling particular interrupts. But
> with this debouncing scheme I noticed that sometimes we miss card
> insertion/removal events.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---

I guess you need to fix the check at the start of the request function
as well.

> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index b7a79a0..45c5f1f 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -162,6 +162,9 @@ static void sdhci_init(struct sdhci_host *host)
>  		SDHCI_INT_DMA_END | SDHCI_INT_DATA_END | SDHCI_INT_RESPONSE |
>  		SDHCI_INT_ADMA_ERROR;
>  
> +	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
> +		intmask &= ~(SDHCI_INT_CARD_REMOVE | SDHCI_INT_CARD_INSERT);
> +

A matter of taste perhaps, but I think it would make more sense to not
add them in the first place than to add and then remove them.

Card detection interrupts should be handled separately anyway as they
should not be enabled before mmc_add_host() returns and should be
disabled before calling mmc_remove_host(). Patch welcome. ;)

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 05/11] sdhci: Add support for hosts reporting inverted write-protect state

[Anton Vorontsov]

This patch adds SDHCI_QUIRK_INVERTED_WRITE_PROTECT quirk. When
specified, the sdhci driver will invert WP state.

p.s. Actually, the quirk is more board-specific than
     controller-specific.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/mmc/host/sdhci.c |    2 ++
 drivers/mmc/host/sdhci.h |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 45c5f1f..85f3780 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1121,6 +1121,8 @@ static int sdhci_get_ro(struct mmc_host *mmc)
 
 	spin_unlock_irqrestore(&host->lock, flags);
 
+	if (host->quirks & SDHCI_QUIRK_INVERTED_WRITE_PROTECT)
+		return !!(present & SDHCI_WRITE_PROTECT);
 	return !(present & SDHCI_WRITE_PROTECT);
 }
 
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 03f035e..fd151a1 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -229,6 +229,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_NO_BUSY_IRQ				(1<<15)
 /* Controller has unreliable card detection */
 #define SDHCI_QUIRK_BROKEN_CARD_DETECTION		(1<<16)
+/* Controller reports inverted write-protect state */
+#define SDHCI_QUIRK_INVERTED_WRITE_PROTECT		(1<<17)
 
 	int			irq;		/* Device IRQ */
 	void __iomem *		ioaddr;		/* Mapped address */
-- 
1.5.6.5

[PATCH 06/11] sdhci: Add support for hosts with strict 32 bit addressing

[Anton Vorontsov]

SDHCI driver must take special care when working with "triggering"
registers on hosts with strict 32 bit addressing.

In FSL eSDHC hosts all registers are 32 bit width, writing to the
first half of any register will cause [undefined?] write the second
half of the register. That is, 16 bit write to the TRANSFER_MODE
register, makes hardware see a bogus write to the COMMAND register
(these two registers are adjacent).

This patch adds SDHCI_QUIRK_32BIT_REGISTERS quirk. When specified,
the sdhci driver will try to "pack" all dangerous writes into single
32 bit write transaction.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/mmc/host/sdhci.c |   19 ++++++++++++++-----
 drivers/mmc/host/sdhci.h |    4 ++++
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 85f3780..9b43588 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -747,13 +747,13 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
 	sdhci_writew(host, data->blocks, SDHCI_BLOCK_COUNT);
 }
 
-static void sdhci_set_transfer_mode(struct sdhci_host *host,
+static u16 sdhci_get_transfer_mode(struct sdhci_host *host,
 	struct mmc_data *data)
 {
 	u16 mode;
 
 	if (data == NULL)
-		return;
+		return 0;
 
 	WARN_ON(!host->data);
 
@@ -765,7 +765,7 @@ static void sdhci_set_transfer_mode(struct sdhci_host *host,
 	if (host->flags & SDHCI_REQ_USE_DMA)
 		mode |= SDHCI_TRNS_DMA;
 
-	sdhci_writew(host, mode, SDHCI_TRANSFER_MODE);
+	return mode;
 }
 
 static void sdhci_finish_data(struct sdhci_host *host)
@@ -817,6 +817,7 @@ static void sdhci_finish_data(struct sdhci_host *host)
 static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 {
 	int flags;
+	u16 xfer_mode;
 	u32 mask;
 	unsigned long timeout;
 
@@ -855,7 +856,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 
 	sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT);
 
-	sdhci_set_transfer_mode(host, cmd->data);
+	xfer_mode = sdhci_get_transfer_mode(host, cmd->data);
 
 	if ((cmd->flags & MMC_RSP_136) && (cmd->flags & MMC_RSP_BUSY)) {
 		printk(KERN_ERR "%s: Unsupported response type!\n",
@@ -881,7 +882,15 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 	if (cmd->data)
 		flags |= SDHCI_CMD_DATA;
 
-	sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND);
+	if (host->quirks & SDHCI_QUIRK_32BIT_REGISTERS) {
+		sdhci_writel(host,
+			SDHCI_MAKE_CMD_32BIT(cmd->opcode, flags, xfer_mode),
+			SDHCI_TRANSFER_MODE);
+	} else {
+		sdhci_writew(host, xfer_mode, SDHCI_TRANSFER_MODE);
+		sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags),
+			SDHCI_COMMAND);
+	}
 }
 
 static void sdhci_finish_command(struct sdhci_host *host)
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index fd151a1..c4edd52 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -58,6 +58,8 @@ static inline int sdhci_off(sdhci_reg_t reg)
 #define  SDHCI_CMD_RESP_SHORT_BUSY 0x03
 
 #define SDHCI_MAKE_CMD(c, f) (((c & 0xff) << 8) | (f & 0xff))
+#define SDHCI_MAKE_CMD_32BIT(c, f, m) \
+	((((c) & 0xff) << 24) | (((f) & 0xff) << 16) | ((m) & 0x37))
 
 #define SDHCI_RESPONSE		sdhci_reg(0x10)
 
@@ -231,6 +233,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_BROKEN_CARD_DETECTION		(1<<16)
 /* Controller reports inverted write-protect state */
 #define SDHCI_QUIRK_INVERTED_WRITE_PROTECT		(1<<17)
+/* Controller has all registers of 32 bit width */
+#define SDHCI_QUIRK_32BIT_REGISTERS			(1<<18)
 
 	int			irq;		/* Device IRQ */
 	void __iomem *		ioaddr;		/* Mapped address */
-- 
1.5.6.5

[PATCH 07/11] sdhci: Add quirk to suppress PIO interrupts during DMA transfers

[Anton Vorontsov]

Some hosts (that is, FSL eSDHC) throw PIO interrupts during DMA
transfers, this causes tons of unneeded interrupts, and thus highly
degraded speed.

This patch adds SDHCI_QUIRK_PIO_IRQS_DURING_DMA quirk. When specified,
the sdhci driver will disable PIO interrupts during DMA transfers.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/mmc/host/sdhci.c |   32 ++++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.h |    3 +++
 2 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 9b43588..ede3790 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -167,6 +167,8 @@ static void sdhci_init(struct sdhci_host *host)
 
 	sdhci_writel(host, intmask, SDHCI_INT_ENABLE);
 	sdhci_writel(host, intmask, SDHCI_SIGNAL_ENABLE);
+
+	host->flags &= ~SDHCI_PIO_DISABLED;
 }
 
 static void sdhci_activate_led(struct sdhci_host *host)
@@ -594,6 +596,33 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_data *data)
 	return count;
 }
 
+static void sdhci_set_pio_irqs(struct sdhci_host *host, bool state)
+{
+	bool current_state = !(host->flags & SDHCI_PIO_DISABLED);
+	u32 ier;
+
+	/*
+	 * We only care about PIO IRQs if the host issues PIO IRQs during
+	 * DMA transfers. Otherwise we can keep the irqs always enabled.
+	 */
+	if (!(host->quirks & SDHCI_QUIRK_PIO_IRQS_DURING_DMA))
+		return;
+
+	if (current_state == state)
+		return;
+
+	ier = sdhci_readl(host, SDHCI_INT_ENABLE);
+	if (state) {
+		ier |= SDHCI_INT_DATA_AVAIL | SDHCI_INT_SPACE_AVAIL;
+		host->flags &= ~SDHCI_PIO_DISABLED;
+	} else {
+		ier &= ~(SDHCI_INT_DATA_AVAIL | SDHCI_INT_SPACE_AVAIL);
+		host->flags |= SDHCI_PIO_DISABLED;
+	}
+	sdhci_writel(host, ier, SDHCI_INT_ENABLE);
+	sdhci_writel(host, ier, SDHCI_SIGNAL_ENABLE);
+}
+
 static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
 {
 	u8 count;
@@ -740,6 +769,9 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
 		sg_miter_start(&host->sg_miter,
 			data->sg, data->sg_len, SG_MITER_ATOMIC);
 		host->blocks = data->blocks;
+		sdhci_set_pio_irqs(host, true);
+	} else {
+		sdhci_set_pio_irqs(host, false);
 	}
 
 	/* We do not handle DMA boundaries, so set it to max (512 KiB) */
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index c4edd52..2e8dfd1 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -235,6 +235,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_INVERTED_WRITE_PROTECT		(1<<17)
 /* Controller has all registers of 32 bit width */
 #define SDHCI_QUIRK_32BIT_REGISTERS			(1<<18)
+/* Controller issues PIO interrupts during DMA transfers */
+#define SDHCI_QUIRK_PIO_IRQS_DURING_DMA			(1<<19)
 
 	int			irq;		/* Device IRQ */
 	void __iomem *		ioaddr;		/* Mapped address */
@@ -256,6 +258,7 @@ struct sdhci_host {
 #define SDHCI_USE_ADMA		(1<<1)		/* Host is ADMA capable */
 #define SDHCI_REQ_USE_DMA	(1<<2)		/* Use DMA for this req. */
 #define SDHCI_DEVICE_DEAD	(1<<3)		/* Device unresponsive */
+#define SDHCI_PIO_DISABLED	(1<<4)		/* PIO IRQs disabled */
 
 	unsigned int		version;	/* SDHCI spec. version */
 
-- 
1.5.6.5

Re: [PATCH 07/11] sdhci: Add quirk to suppress PIO interrupts during DMA transfers

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 890 bytes --]

On Fri, 6 Feb 2009 21:06:55 +0300
Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> Some hosts (that is, FSL eSDHC) throw PIO interrupts during DMA
> transfers, this causes tons of unneeded interrupts, and thus highly
> degraded speed.
> 
> This patch adds SDHCI_QUIRK_PIO_IRQS_DURING_DMA quirk. When specified,
> the sdhci driver will disable PIO interrupts during DMA transfers.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---

It's probably better to change the interrupt handling to only enable
relevant interrupts instead of having everything on constantly. Too
many quirks just makes the driver difficult to understand.

-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 08/11] sdhci: Add support for hosts that don't specify clocks in the cap. register

[Anton Vorontsov]

FSL eSDHC hosts don't provide clocks bits in the capabilities register,
instead we're getting clocks values from the device tree.

There is somewhat similar change[1] from Ben Dooks, the change adds
callbacks for getting the clocks. But for eSDHC the callbacks are
superfluous, since the clocks are static.

[1] http://lkml.org/lkml/2008/12/2/157

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/mmc/host/sdhci.c |   31 +++++++++++++++----------------
 1 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ede3790..0293368 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1703,24 +1703,23 @@ int sdhci_add_host(struct sdhci_host *host)
 		mmc_dev(host->mmc)->dma_mask = &host->dma_mask;
 	}
 
-	host->max_clk =
-		(caps & SDHCI_CLOCK_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT;
-	if (host->max_clk == 0) {
-		printk(KERN_ERR "%s: Hardware doesn't specify base clock "
-			"frequency.\n", mmc_hostname(mmc));
-		return -ENODEV;
+	if (!host->max_clk) {
+		host->max_clk = (caps & SDHCI_CLOCK_BASE_MASK) >>
+						SDHCI_CLOCK_BASE_SHIFT;
+		if (host->max_clk == 0) {
+			printk(KERN_ERR "%s: Hardware doesn't specify base "
+				"clock frequency.\n", mmc_hostname(mmc));
+			return -ENODEV;
+		}
+		host->max_clk *= 1000000;
 	}
-	host->max_clk *= 1000000;
 
-	host->timeout_clk =
-		(caps & SDHCI_TIMEOUT_CLK_MASK) >> SDHCI_TIMEOUT_CLK_SHIFT;
-	if (host->timeout_clk == 0) {
-		printk(KERN_ERR "%s: Hardware doesn't specify timeout clock "
-			"frequency.\n", mmc_hostname(mmc));
-		return -ENODEV;
+	if (!host->timeout_clk) {
+		host->timeout_clk = (caps & SDHCI_TIMEOUT_CLK_MASK) >>
+						SDHCI_TIMEOUT_CLK_SHIFT;
+		if (caps & SDHCI_TIMEOUT_CLK_UNIT)
+			host->timeout_clk *= 1000;
 	}
-	if (caps & SDHCI_TIMEOUT_CLK_UNIT)
-		host->timeout_clk *= 1000;
 
 	/*
 	 * Set host parameters.
-- 
1.5.6.5

Re: [PATCH 08/11] sdhci: Add support for hosts that don't specify clocks in the cap. register

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 1033 bytes --]

On Fri, 6 Feb 2009 21:06:57 +0300
Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> FSL eSDHC hosts don't provide clocks bits in the capabilities register,
> instead we're getting clocks values from the device tree.
> 
> There is somewhat similar change[1] from Ben Dooks, the change adds
> callbacks for getting the clocks. But for eSDHC the callbacks are
> superfluous, since the clocks are static.
> 
> [1] http://lkml.org/lkml/2008/12/2/157
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---

As I told Ben, I prefer if we stick to the standard as much as
possible. So no external info unless the register is set to zero.

And since we know the Samsung chip needs callbacks, we might as well
add them here. It's not like this is a performance critical path.

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 09/11] sdhci: Add set_clock callback

[Anton Vorontsov]

FSL eSDHC hosts have incompatible register map to manage the SDCLK.
This patch adds set_clock callback so that drivers could overwrite
set_clock behaviour.

Similar patch[1] was posted by Ben Dooks, though in Ben's version the
callback is named change_clock, plus the patch has some unrelated bits
that makes the patch difficult to reuse.

[1] http://lkml.org/lkml/2008/12/2/160

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/mmc/host/sdhci.c |    5 +++++
 drivers/mmc/host/sdhci.h |    2 ++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 0293368..3c1f1d5 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -969,6 +969,11 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 	if (clock == host->clock)
 		return;
 
+	if (host->ops->set_clock) {
+		host->ops->set_clock(host, clock);
+		return;
+	}
+
 	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
 
 	if (clock == 0)
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 2e8dfd1..497276b 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -312,6 +312,8 @@ struct sdhci_ops {
 	void	(*writew)(struct sdhci_host *host, u16 val, sdhci_reg_t reg);
 	void	(*writeb)(struct sdhci_host *host, u8 val, sdhci_reg_t reg);
 
+	void	(*set_clock)(struct sdhci_host *host, unsigned int clock);
+
 	int	(*enable_dma)(struct sdhci_host *host);
 };
 
-- 
1.5.6.5

Re: [PATCH 09/11] sdhci: Add set_clock callback

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 984 bytes --]

On Fri, 6 Feb 2009 21:06:59 +0300
Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> FSL eSDHC hosts have incompatible register map to manage the SDCLK.
> This patch adds set_clock callback so that drivers could overwrite
> set_clock behaviour.
> 
> Similar patch[1] was posted by Ben Dooks, though in Ben's version the
> callback is named change_clock, plus the patch has some unrelated bits
> that makes the patch difficult to reuse.
> 
> [1] http://lkml.org/lkml/2008/12/2/160
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---

A set_clock() callback is reasonable as there might be a clock source
that needs to be set up, but completely overriding the normal routine
(i.e. the "return") should be quirked IMO.

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 10/11] sdhci: Add quirk for Freescale eSDHC controllers

[Anton Vorontsov]

This patch adds SDHCI_QUIRK_FSL quirk. The quirk is used to instruct
the sdhci driver about various FSL eSDHC host incompatibilities:

1) FSL eSDHC controllers can support maximum block size up to 4096
   bytes. The MBL (Maximum Block Length) field in the capabilities
   register extended by one bit.

   (Should we implement a dedicated quirk for this? I.e.
    SDHCI_QUIRK_MAX_BLK_SZ_4096?)

2) sdhci_init() is needed after error conditions.

   (Can we safely do this for all controllers?)

3) Small udelay is needed to make eSDHC work in PIO mode. Without
   the delay reading causes endless interrupt storm, and writing
   corrupts data. The first guess would be that we must wait for
   some bit in some register, but I didn't find any reliable bits
   that changes before and after the delay. Though, more investigation
   on this is in my todo list.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/mmc/host/sdhci.c |   36 ++++++++++++++++++++++++++++++------
 drivers/mmc/host/sdhci.h |    3 +++
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 3c1f1d5..87ba0ed 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -328,6 +328,10 @@ static void sdhci_transfer_pio(struct sdhci_host *host)
 		mask = ~0;
 
 	while (sdhci_readl(host, SDHCI_PRESENT_STATE) & mask) {
+		/* FIXME */
+		if (host->quirks & SDHCI_QUIRK_FSL)
+			udelay(100);
+
 		if (host->data->flags & MMC_DATA_READ)
 			sdhci_read_block_pio(host);
 		else
@@ -625,6 +629,7 @@ static void sdhci_set_pio_irqs(struct sdhci_host *host, bool state)
 
 static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
 {
+	u16 blksz;
 	u8 count;
 	u8 ctrl;
 	int ret;
@@ -775,7 +780,12 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
 	}
 
 	/* We do not handle DMA boundaries, so set it to max (512 KiB) */
-	sdhci_writew(host, SDHCI_MAKE_BLKSZ(7, data->blksz), SDHCI_BLOCK_SIZE);
+	if (host->quirks & SDHCI_QUIRK_FSL)
+		blksz = data->blksz;
+	else
+		blksz = SDHCI_MAKE_BLKSZ(7, data->blksz);
+
+	sdhci_writew(host, blksz, SDHCI_BLOCK_SIZE);
 	sdhci_writew(host, data->blocks, SDHCI_BLOCK_COUNT);
 }
 
@@ -1280,6 +1290,9 @@ static void sdhci_tasklet_finish(unsigned long param)
 		   controllers do not like that. */
 		sdhci_reset(host, SDHCI_RESET_CMD);
 		sdhci_reset(host, SDHCI_RESET_DATA);
+		/* FSL controllers need this. */
+		if (host->quirks & SDHCI_QUIRK_FSL)
+			sdhci_init(host);
 	}
 
 	host->mrq = NULL;
@@ -1789,13 +1802,24 @@ int sdhci_add_host(struct sdhci_host *host)
 	 * Maximum block size. This varies from controller to controller and
 	 * is specified in the capabilities register.
 	 */
-	mmc->max_blk_size = (caps & SDHCI_MAX_BLOCK_MASK) >> SDHCI_MAX_BLOCK_SHIFT;
-	if (mmc->max_blk_size >= 3) {
+	if (host->quirks & SDHCI_QUIRK_FSL) {
+		mmc->max_blk_size = (caps & SDHCI_FSL_MAX_BLOCK_MASK) >>
+				SDHCI_MAX_BLOCK_SHIFT;
+		if (mmc->max_blk_size >= 4)
+			mmc->max_blk_size = ~0;
+	} else {
+		mmc->max_blk_size = (caps & SDHCI_MAX_BLOCK_MASK) >>
+				SDHCI_MAX_BLOCK_SHIFT;
+		if (mmc->max_blk_size >= 3)
+			mmc->max_blk_size = ~0;
+	}
+
+	if (mmc->max_blk_size == ~0) {
 		printk(KERN_WARNING "%s: Invalid maximum block size, "
 			"assuming 512 bytes\n", mmc_hostname(mmc));
-		mmc->max_blk_size = 512;
-	} else
-		mmc->max_blk_size = 512 << mmc->max_blk_size;
+		mmc->max_blk_size = 0;
+	}
+	mmc->max_blk_size = 512 << mmc->max_blk_size;
 
 	/*
 	 * Maximum block count.
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 497276b..cb575f3 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -152,6 +152,7 @@ static inline int sdhci_off(sdhci_reg_t reg)
 #define  SDHCI_CLOCK_BASE_MASK	0x00003F00
 #define  SDHCI_CLOCK_BASE_SHIFT	8
 #define  SDHCI_MAX_BLOCK_MASK	0x00030000
+#define  SDHCI_FSL_MAX_BLOCK_MASK 0x00070000
 #define  SDHCI_MAX_BLOCK_SHIFT  16
 #define  SDHCI_CAN_DO_ADMA2	0x00080000
 #define  SDHCI_CAN_DO_ADMA1	0x00100000
@@ -237,6 +238,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_32BIT_REGISTERS			(1<<18)
 /* Controller issues PIO interrupts during DMA transfers */
 #define SDHCI_QUIRK_PIO_IRQS_DURING_DMA			(1<<19)
+/* Controller is Freescale eSDHC */
+#define SDHCI_QUIRK_FSL					(1<<20)
 
 	int			irq;		/* Device IRQ */
 	void __iomem *		ioaddr;		/* Mapped address */
-- 
1.5.6.5

Re: [PATCH 10/11] sdhci: Add quirk for Freescale eSDHC controllers

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 1716 bytes --]

On Fri, 6 Feb 2009 21:07:01 +0300
Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> This patch adds SDHCI_QUIRK_FSL quirk. The quirk is used to instruct
> the sdhci driver about various FSL eSDHC host incompatibilities:
> 

No device quirks please. They should be for specific bugs, not lumping
things together like this. Otherwise we'll soon have an unmanageable
mess.

> 1) FSL eSDHC controllers can support maximum block size up to 4096
>    bytes. The MBL (Maximum Block Length) field in the capabilities
>    register extended by one bit.
> 
>    (Should we implement a dedicated quirk for this? I.e.
>     SDHCI_QUIRK_MAX_BLK_SZ_4096?)
> 

Yes please. It would have to mean "always support 4096" though, not
"turn reserved bit 18 into a block length bit".

> 2) sdhci_init() is needed after error conditions.
> 
>    (Can we safely do this for all controllers?)
> 

Please investigate which part of sdhci_init() is needed. How does it
break without this?

> 3) Small udelay is needed to make eSDHC work in PIO mode. Without
>    the delay reading causes endless interrupt storm, and writing
>    corrupts data. The first guess would be that we must wait for
>    some bit in some register, but I didn't find any reliable bits
>    that changes before and after the delay. Though, more investigation
>    on this is in my todo list.

Please try to investigate more, but if you cannot improve it further
then a specific quirk can be added.

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 10/11] sdhci: Add quirk for Freescale eSDHC controllers

[Anton Vorontsov]

On Sun, Feb 08, 2009 at 10:12:09PM +0100, Pierre Ossman wrote:
> On Fri, 6 Feb 2009 21:07:01 +0300
> Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> 
> > This patch adds SDHCI_QUIRK_FSL quirk. The quirk is used to instruct
> > the sdhci driver about various FSL eSDHC host incompatibilities:
> > 
> 
> No device quirks please. They should be for specific bugs, not lumping
> things together like this. Otherwise we'll soon have an unmanageable
> mess.

OK.

> > 1) FSL eSDHC controllers can support maximum block size up to 4096
> >    bytes. The MBL (Maximum Block Length) field in the capabilities
> >    register extended by one bit.
> > 
> >    (Should we implement a dedicated quirk for this? I.e.
> >     SDHCI_QUIRK_MAX_BLK_SZ_4096?)
> > 
> 
> Yes please. It would have to mean "always support 4096" though, not
> "turn reserved bit 18 into a block length bit".

OK.

> > 2) sdhci_init() is needed after error conditions.
> > 
> >    (Can we safely do this for all controllers?)
> > 
> 
> Please investigate which part of sdhci_init() is needed. How does it
> break without this?

After reset eSDHC lose signal/interrupt enable states:

Before reset:

sdhci: ============== REGISTER DUMP ==============
sdhci: Sys addr: 0x00000008 | Version:  0x00000000
sdhci: Blk size: 0x00000008 | Blk cnt:  0x00000000
sdhci: Argument: 0x000001aa | Trn mode: 0x00000000
sdhci: Present:  0xff850000 | Host ctl: 0x00000021
sdhci: Power:    0x00000000 | Blk gap:  0x00000000
sdhci: Wake-up:  0x00000000 | Clock:    0x00001077
sdhci: Timeout:  0x00000000 | Int stat: 0x00000000
sdhci: Int enab: 0x007f0003 | Sig enab: 0x007f0003
sdhci: AC12 err: 0x00000000 | Slot int: 0x00000001
sdhci: Caps:     0x01e30000 | Max curr: 0x00000000
sdhci: ===========================================

after sdhci_reset(host, SDHCI_RESET_CMD):

sdhci: ============== REGISTER DUMP ==============
sdhci: Sys addr: 0x00000008 | Version:  0x00000000
sdhci: Blk size: 0x00000008 | Blk cnt:  0x00000000
sdhci: Argument: 0x00000000 | Trn mode: 0x00000000
sdhci: Present:  0xff850000 | Host ctl: 0x00000021
sdhci: Power:    0x00000000 | Blk gap:  0x00000000
sdhci: Wake-up:  0x00000000 | Clock:    0x00001077
sdhci: Timeout:  0x00000000 | Int stat: 0x00000000
sdhci: Int enab: 0x017f0003 | Sig enab: 0x00700002
sdhci: AC12 err: 0x00000000 | Slot int: 0x00000001
sdhci: Caps:     0x01e30000 | Max curr: 0x00000000
sdhci: ===========================================

After sdhci_reset(host, SDHCI_RESET_DATA):

sdhci: ============== REGISTER DUMP ==============
sdhci: Sys addr: 0x00000008 | Version:  0x00000000
sdhci: Blk size: 0x00000008 | Blk cnt:  0x00000000
sdhci: Argument: 0x00000000 | Trn mode: 0x00000000
sdhci: Present:  0xff850000 | Host ctl: 0x00000021
sdhci: Power:    0x00000000 | Blk gap:  0x00000000
sdhci: Wake-up:  0x00000000 | Clock:    0x00001077
sdhci: Timeout:  0x00000000 | Int stat: 0x00000000
sdhci: Int enab: 0x117f003f | Sig enab: 0x00000000
sdhci: AC12 err: 0x00000000 | Slot int: 0x00000001
sdhci: Caps:     0x01e30000 | Max curr: 0x00000000
sdhci: ===========================================

> > 3) Small udelay is needed to make eSDHC work in PIO mode. Without
> >    the delay reading causes endless interrupt storm, and writing
> >    corrupts data. The first guess would be that we must wait for
> >    some bit in some register, but I didn't find any reliable bits
> >    that changes before and after the delay. Though, more investigation
> >    on this is in my todo list.
> 
> Please try to investigate more, but if you cannot improve it further
> then a specific quirk can be added.

No luck so far... :-/


Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

[PATCH 11/11] mmc: Add OpenFirmware bindings for SDHCI driver

[Anton Vorontsov]

This patch adds a new driver: sdhci-of. The driver is similar to
the sdhci-pci, it contains common probe code, and controller-specific
ops and quirks.

So far there are only Freescale eSDHC ops and quirks.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/mmc/host/Kconfig    |   10 ++
 drivers/mmc/host/Makefile   |    1 +
 drivers/mmc/host/sdhci-of.c |  264 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 275 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mmc/host/sdhci-of.c

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 99d4b28..73b79e1 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -65,6 +65,16 @@ config MMC_RICOH_MMC
 
 	  If unsure, say Y.
 
+config MMC_SDHCI_OF
+	tristate "SDHCI support on OpenFirmware platforms"
+	depends on MMC_SDHCI && PPC_OF
+	help
+	  This selects the OF support for Secure Digital Host Controller
+	  Interfaces. So far, only the Freescale eSDHC controller is known
+	  to exist on OF platforms.
+
+	  If unsure, say N.
+
 config MMC_OMAP
 	tristate "TI OMAP Multimedia Card Interface support"
 	depends on ARCH_OMAP
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index dedec55..dd512d9 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_MMC_MXC)		+= mxcmmc.o
 obj-$(CONFIG_MMC_SDHCI)		+= sdhci.o
 obj-$(CONFIG_MMC_SDHCI_PCI)	+= sdhci-pci.o
 obj-$(CONFIG_MMC_RICOH_MMC)	+= ricoh_mmc.o
+obj-$(CONFIG_MMC_SDHCI_OF)	+= sdhci-of.o
 obj-$(CONFIG_MMC_WBSD)		+= wbsd.o
 obj-$(CONFIG_MMC_AU1X)		+= au1xmmc.o
 obj-$(CONFIG_MMC_OMAP)		+= omap.o
diff --git a/drivers/mmc/host/sdhci-of.c b/drivers/mmc/host/sdhci-of.c
new file mode 100644
index 0000000..734e751
--- /dev/null
+++ b/drivers/mmc/host/sdhci-of.c
@@ -0,0 +1,264 @@
+/*
+ * OpenFirmware bindings for Secure Digital Host Controller Interface.
+ *
+ * Copyright (c) 2007 Freescale Semiconductor, Inc.
+ * Copyright (c) 2009 MontaVista Software, Inc.
+ *
+ * Authors: Xiaobo Xie <X.Xie@freescale.com>
+ *	    Anton Vorontsov <avorontsov@ru.mvista.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or (at
+ * your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/mmc/host.h>
+#include "sdhci.h"
+
+struct sdhci_of_data {
+	unsigned int quirks;
+	struct sdhci_ops ops;
+};
+
+/*
+ * Ops and quirks for the Freescale eSDHC controller.
+ */
+
+#define ESDHC_DMA_SYSCTL	0x40c
+#define ESDHC_DMA_SNOOP		0x00000040
+
+#define ESDHC_SYSTEM_CONTROL	0x2c
+#define ESDHC_CLOCK_MASK	0x0000fff0
+#define ESDHC_PREDIV_SHIFT	8
+#define ESDHC_DIVIDER_SHIFT	4
+#define ESDHC_CLOCK_PEREN	0x00000004
+#define ESDHC_CLOCK_HCKEN	0x00000002
+#define ESDHC_CLOCK_IPGEN	0x00000001
+
+static u32 esdhc_readl(struct sdhci_host *host, sdhci_reg_t reg)
+{
+	return in_be32(host->ioaddr + sdhci_off(reg));
+}
+
+static u16 esdhc_readw(struct sdhci_host *host, sdhci_reg_t reg)
+{
+	return in_be16(host->ioaddr + (sdhci_off(reg) ^ 0x2));
+}
+
+static u8 esdhc_readb(struct sdhci_host *host, sdhci_reg_t reg)
+{
+	return in_8(host->ioaddr + (sdhci_off(reg) ^ 0x3));
+}
+
+static void esdhc_writel(struct sdhci_host *host, u32 val, sdhci_reg_t reg)
+{
+	out_be32(host->ioaddr + sdhci_off(reg), val);
+}
+
+static void esdhc_writew(struct sdhci_host *host, u16 val, sdhci_reg_t reg)
+{
+	int base = sdhci_off(reg) & ~0x3;
+	int shift = (sdhci_off(reg) & 0x2) * 8;
+
+	clrsetbits_be32(host->ioaddr + base, 0xffff << shift, val << shift);
+}
+
+static void esdhc_writeb(struct sdhci_host *host, u8 val, sdhci_reg_t reg)
+{
+	int base = sdhci_off(reg) & ~0x3;
+	int shift = (sdhci_off(reg) & 0x3) * 8;
+
+	clrsetbits_be32(host->ioaddr + base , 0xff << shift, val << shift);
+}
+
+static void esdhc_set_clock(struct sdhci_host *host, unsigned int clock)
+{
+	int div;
+	int pre_div = 1;
+
+	setbits32(host->ioaddr + ESDHC_SYSTEM_CONTROL, ESDHC_CLOCK_IPGEN |
+		  ESDHC_CLOCK_HCKEN | ESDHC_CLOCK_PEREN);
+
+	clrbits32(host->ioaddr + ESDHC_SYSTEM_CONTROL, ESDHC_CLOCK_MASK);
+
+	if (clock == 0)
+		goto out;
+
+	if (host->max_clk / 16 > clock) {
+		for (; pre_div < 256; pre_div *= 2) {
+			if (host->max_clk / pre_div < clock * 16)
+				break;
+		}
+	}
+
+	for (div = 1; div <= 16; div++) {
+		if (host->max_clk / (div * pre_div) <= clock)
+			break;
+	}
+
+	pre_div >>= 1;
+	div -= 1;
+
+	setbits32(host->ioaddr + ESDHC_SYSTEM_CONTROL,
+		div << ESDHC_DIVIDER_SHIFT | pre_div << ESDHC_PREDIV_SHIFT);
+	mdelay(10);
+out:
+	host->clock = clock;
+}
+
+static int esdhc_enable_dma(struct sdhci_host *host)
+{
+	setbits32(host->ioaddr + ESDHC_DMA_SYSCTL, ESDHC_DMA_SNOOP);
+	return 0;
+}
+
+static struct sdhci_of_data sdhci_esdhc = {
+	.quirks = SDHCI_QUIRK_FSL |
+		  SDHCI_QUIRK_32BIT_REGISTERS |
+		  SDHCI_QUIRK_PIO_IRQS_DURING_DMA |
+		  SDHCI_QUIRK_BROKEN_CARD_DETECTION |
+		  SDHCI_QUIRK_INVERTED_WRITE_PROTECT |
+		  SDHCI_QUIRK_NO_BUSY_IRQ |
+		  SDHCI_QUIRK_NO_CARD_NO_RESET,
+	.ops = {
+		.readl = esdhc_readl,
+		.readw = esdhc_readw,
+		.readb = esdhc_readb,
+		.writel = esdhc_writel,
+		.writew = esdhc_writew,
+		.writeb = esdhc_writeb,
+		.set_clock = esdhc_set_clock,
+		.enable_dma = esdhc_enable_dma,
+	},
+};
+
+#ifdef CONFIG_PM
+
+static int sdhci_of_suspend(struct of_device *ofdev, pm_message_t state)
+{
+	struct sdhci_host *host = dev_get_drvdata(&ofdev->dev);
+
+	return mmc_suspend_host(host->mmc, state);
+}
+
+static int sdhci_of_resume(struct of_device *ofdev)
+{
+	struct sdhci_host *host = dev_get_drvdata(&ofdev->dev);
+
+	return mmc_resume_host(host->mmc);
+}
+
+#else
+
+#define sdhci_of_suspend NULL
+#define sdhci_of_resume NULL
+
+#endif
+
+static int __devinit sdhci_of_probe(struct of_device *ofdev,
+				 const struct of_device_id *match)
+{
+	struct device_node *np = ofdev->node;
+	struct sdhci_of_data *sdhci_of_data = match->data;
+	struct sdhci_host *host;
+	const u32 *clk;
+	int size;
+	int ret;
+
+	host = sdhci_alloc_host(&ofdev->dev, sizeof(*host));
+	if (!host)
+		return -ENOMEM;
+
+	dev_set_drvdata(&ofdev->dev, host);
+
+	host->ioaddr = of_iomap(np, 0);
+	if (!host->ioaddr) {
+		ret = -ENOMEM;
+		goto err_addr_map;
+	}
+
+	host->irq = irq_of_parse_and_map(np, 0);
+	if (!host->irq) {
+		ret = -EINVAL;
+		goto err_no_irq;
+	}
+
+	host->hw_name = dev_name(&ofdev->dev);
+	if (sdhci_of_data) {
+		host->quirks = sdhci_of_data->quirks;
+		host->ops = &sdhci_of_data->ops;
+	}
+
+	clk = of_get_property(np, "clock-frequency", &size);
+	if (clk && size == sizeof(*clk) && *clk) {
+		host->max_clk = *clk;
+		host->timeout_clk = *clk / 1000;
+	}
+
+	ret = sdhci_add_host(host);
+	if (ret)
+		goto err_add_host;
+
+	return 0;
+
+err_add_host:
+	irq_dispose_mapping(host->irq);
+err_no_irq:
+	iounmap(host->ioaddr);
+err_addr_map:
+	sdhci_free_host(host);
+	return ret;
+}
+
+static int __devexit sdhci_of_remove(struct of_device *ofdev)
+{
+	struct sdhci_host *host = dev_get_drvdata(&ofdev->dev);
+
+	sdhci_remove_host(host, 0);
+	sdhci_free_host(host);
+	irq_dispose_mapping(host->irq);
+	iounmap(host->ioaddr);
+	return 0;
+}
+
+static const struct of_device_id sdhci_of_match[] = {
+	{ .compatible = "fsl,mpc8379-esdhc", .data = &sdhci_esdhc, },
+	{ .compatible = "fsl,mpc8536-esdhc", .data = &sdhci_esdhc, },
+	{ .compatible = "generic-sdhci", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sdhci_of_match);
+
+static struct of_platform_driver sdhci_of_driver = {
+	.driver.name = "sdhci-of",
+	.match_table = sdhci_of_match,
+	.probe = sdhci_of_probe,
+	.remove = __devexit_p(sdhci_of_remove),
+	.suspend = sdhci_of_suspend,
+	.resume	= sdhci_of_resume,
+};
+
+static int __init sdhci_of_init(void)
+{
+	return of_register_platform_driver(&sdhci_of_driver);
+}
+module_init(sdhci_of_init);
+
+static void __exit sdhci_of_exit(void)
+{
+	of_unregister_platform_driver(&sdhci_of_driver);
+}
+module_exit(sdhci_of_exit);
+
+MODULE_DESCRIPTION("Secure Digital Host Controller Interface OF driver");
+MODULE_AUTHOR("Xiaobo Xie <X.Xie@freescale.com>, "
+	      "Anton Vorontsov <avorontsov@ru.mvista.com>");
+MODULE_LICENSE("GPL");
-- 
1.5.6.5

Re: [PATCH RFC 0/11] FSL eSDHC support: second call for comments

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 575 bytes --]

On Fri, 6 Feb 2009 21:05:20 +0300
Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> Hi all,
> 
> There were only a few comments on the previous version. So, here is
> the second call for comments.
> 

Yeah sorry, haven't really had time for looking over your patches
properly. See my first comments to relevant patches now though.

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH v2 0/11] FSL eSDHC support

[Anton Vorontsov]

Hi all,

Much thanks for the previous comments. Here comes v2.

Changes since v1:
- "Add support for bus-specific IO memory accessors" patch no longer
  touches sdhci-pci. The changes were no longer needed since I dropped
  "Add type checking ..." patch back in RFCv2;
- Patch "Add support for hosts with strict 32 bit addressing" dropped.
  Now we handle the 32 bit magic in eSDHC's writew() accessor;
- Patch "Add quirk for controllers with max. block size up to 4096 bytes"
  replaced by "Add quirk for forcing maximum block size to 2048 bytes";
- SDHCI_INT_ALL_MASK changed to (unsigned int)-1;
- Addressed Pierre's comments in "Add support for card-detection polling"
  patch.

Changes since the third RFC:
- Use uninitialized_var() (suggested by Laurent Pinchart);
- Fixed a bug in eSDHC SDCLK prescaler calculations because of which
  we were over-clocking the SDCLK, and that caused CRC errors using some
  SD cards;
- Not a change, but some status: SDHS (50 MHz) cards were tested to
  NOT work (at least on MPC837x boards) -- this is to be investigated
  further. SDHC (> 4 GB) cards were not tested, yet.

Changes since the second RFC:
- Addressed all comments that were raised by Pierre Ossman.
  There were too many to mention them all, so here is the link:
  http://lkml.org/lkml/2009/2/6/320

Changes since the first RFC:
- Use of_iomap() in sdhci-of.c (suggested by Arnd Bergmann). Also added
  Arnd's Acked-by: line for the sdhci-of patch.
- Kconfig help text improved (thanks to Matt Sealey and M. Warner Losh).
- In "sdhci: Add quirk to suppress PIO interrupts during DMA transfers"
  patch: sdhci_init() now clears SDHCI_PIO_DISABLED flag, otherwise we
  won't disable PIO interrupts after suspend.
- New patch: "sdhci: Add type checking for IO memory accessors"

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

[PATCH 04/11] sdhci: Add support for card-detection polling

[Anton Vorontsov]

This patch adds SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk. When specified,
sdhci driver will set MMC_CAP_NEEDS_POLL MMC host capability, and won't
enable card insert/remove interrupts.

This is needed for hosts with unreliable card detection, such as FSL
eSDHC. The original eSDHC driver was tring to "debounce" card-detection
IRQs by reading present state and disabling particular interrupts. But
with this debouncing scheme I noticed that sometimes we miss card
insertion/removal events.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/mmc/host/sdhci.c |   17 +++++++++++++++--
 drivers/mmc/host/sdhci.h |    2 ++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 0cbde8e..d71c877 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -167,6 +167,9 @@ static void sdhci_set_card_detection(struct sdhci_host *host, bool enable)
 {
 	u32 irqs = SDHCI_INT_CARD_REMOVE | SDHCI_INT_CARD_INSERT;
 
+	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
+		return;
+
 	if (enable)
 		sdhci_unmask_irqs(host, irqs);
 	else
@@ -1096,6 +1099,7 @@ out:
 static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 {
 	struct sdhci_host *host;
+	bool present;
 	unsigned long flags;
 
 	host = mmc_priv(mmc);
@@ -1110,8 +1114,14 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 
 	host->mrq = mrq;
 
-	if (!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)
-		|| (host->flags & SDHCI_DEVICE_DEAD)) {
+	/* If polling, assume that the card is always present. */
+	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
+		present = true;
+	else
+		present = sdhci_readl(host, SDHCI_PRESENT_STATE) &
+				SDHCI_CARD_PRESENT;
+
+	if (!present || host->flags & SDHCI_DEVICE_DEAD) {
 		host->mrq->cmd->error = -ENOMEDIUM;
 		tasklet_schedule(&host->finish_tasklet);
 	} else
@@ -1745,6 +1755,9 @@ int sdhci_add_host(struct sdhci_host *host)
 	if (caps & SDHCI_CAN_DO_HISPD)
 		mmc->caps |= MMC_CAP_SD_HIGHSPEED;
 
+	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
+		mmc->caps |= MMC_CAP_NEEDS_POLL;
+
 	mmc->ocr_avail = 0;
 	if (caps & SDHCI_CAN_VDD_330)
 		mmc->ocr_avail |= MMC_VDD_32_33|MMC_VDD_33_34;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 1c29895..09a4363 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -212,6 +212,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_BROKEN_SMALL_PIO			(1<<13)
 /* Controller does not provide transfer-complete interrupt when not busy */
 #define SDHCI_QUIRK_NO_BUSY_IRQ				(1<<14)
+/* Controller has unreliable card detection */
+#define SDHCI_QUIRK_BROKEN_CARD_DETECTION		(1<<15)
 
 	int			irq;		/* Device IRQ */
 	void __iomem *		ioaddr;		/* Mapped address */
-- 
1.5.6.5

[PATCH v3 0/11] FSL eSDHC support

[Anton Vorontsov]

Hi all,

Here comes another version, let's hope that one is final. ;-)

Changes since v2:
- Introduced MMC_SDHCI_IO_ACCESSORS Kconfig symbol. If not selected,
  SDHCI will just use PCI memory accessors unconditionally.
- Added MAINTAINERS entry.

Changes since v1:
- "Add support for bus-specific IO memory accessors" patch no longer
  touches sdhci-pci. The changes were no longer needed since I dropped
  "Add type checking ..." patch back in RFCv2;
- Patch "Add support for hosts with strict 32 bit addressing" dropped.
  Now we handle the 32 bit magic in eSDHC's writew() accessor;
- Patch "Add quirk for controllers with max. block size up to 4096 bytes"
  replaced by "Add quirk for forcing maximum block size to 2048 bytes";
- SDHCI_INT_ALL_MASK changed to (unsigned int)-1;
- Addressed Pierre's comments in "Add support for card-detection polling"
  patch.

Changes since the third RFC:
- Use uninitialized_var() (suggested by Laurent Pinchart);
- Fixed a bug in eSDHC SDCLK prescaler calculations because of which
  we were over-clocking the SDCLK, and that caused CRC errors using some
  SD cards;
- Not a change, but some status: SDHS (50 MHz) cards were tested to
  NOT work (at least on MPC837x boards) -- this is to be investigated
  further. SDHC (> 4 GB) cards were not tested, yet.

Changes since the second RFC:
- Addressed all comments that were raised by Pierre Ossman.
  There were too many to mention them all, so here is the link:
  http://lkml.org/lkml/2009/2/6/320

Changes since the first RFC:
- Use of_iomap() in sdhci-of.c (suggested by Arnd Bergmann). Also added
  Arnd's Acked-by: line for the sdhci-of patch.
- Kconfig help text improved (thanks to Matt Sealey and M. Warner Losh).
- In "sdhci: Add quirk to suppress PIO interrupts during DMA transfers"
  patch: sdhci_init() now clears SDHCI_PIO_DISABLED flag, otherwise we
  won't disable PIO interrupts after suspend.
- New patch: "sdhci: Add type checking for IO memory accessors"

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

[PATCH 04/11] sdhci: Add support for card-detection polling

[Anton Vorontsov]

This patch adds SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk. When specified,
sdhci driver will set MMC_CAP_NEEDS_POLL MMC host capability, and won't
enable card insert/remove interrupts.

This is needed for hosts with unreliable card detection, such as FSL
eSDHC. The original eSDHC driver was tring to "debounce" card-detection
IRQs by reading present state and disabling particular interrupts. But
with this debouncing scheme I noticed that sometimes we miss card
insertion/removal events.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/mmc/host/sdhci.c |   17 +++++++++++++++--
 drivers/mmc/host/sdhci.h |    2 ++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 6fbbc00..fc7cb48 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -112,6 +112,9 @@ static void sdhci_set_card_detection(struct sdhci_host *host, bool enable)
 {
 	u32 irqs = SDHCI_INT_CARD_REMOVE | SDHCI_INT_CARD_INSERT;
 
+	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
+		return;
+
 	if (enable)
 		sdhci_unmask_irqs(host, irqs);
 	else
@@ -1041,6 +1044,7 @@ out:
 static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 {
 	struct sdhci_host *host;
+	bool present;
 	unsigned long flags;
 
 	host = mmc_priv(mmc);
@@ -1055,8 +1059,14 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 
 	host->mrq = mrq;
 
-	if (!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)
-		|| (host->flags & SDHCI_DEVICE_DEAD)) {
+	/* If polling, assume that the card is always present. */
+	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
+		present = true;
+	else
+		present = sdhci_readl(host, SDHCI_PRESENT_STATE) &
+				SDHCI_CARD_PRESENT;
+
+	if (!present || host->flags & SDHCI_DEVICE_DEAD) {
 		host->mrq->cmd->error = -ENOMEDIUM;
 		tasklet_schedule(&host->finish_tasklet);
 	} else
@@ -1690,6 +1700,9 @@ int sdhci_add_host(struct sdhci_host *host)
 	if (caps & SDHCI_CAN_DO_HISPD)
 		mmc->caps |= MMC_CAP_SD_HIGHSPEED;
 
+	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
+		mmc->caps |= MMC_CAP_NEEDS_POLL;
+
 	mmc->ocr_avail = 0;
 	if (caps & SDHCI_CAN_VDD_330)
 		mmc->ocr_avail |= MMC_VDD_32_33|MMC_VDD_33_34;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index a9e25c6..968d713 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -214,6 +214,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_BROKEN_SMALL_PIO			(1<<13)
 /* Controller does not provide transfer-complete interrupt when not busy */
 #define SDHCI_QUIRK_NO_BUSY_IRQ				(1<<14)
+/* Controller has unreliable card detection */
+#define SDHCI_QUIRK_BROKEN_CARD_DETECTION		(1<<15)
 
 	int			irq;		/* Device IRQ */
 	void __iomem *		ioaddr;		/* Mapped address */
-- 
1.5.6.5