Skip to contentSkip to conversation

MMC: S3C24XX updates for s3cmci driver

PATCH In review Ben Dooks 2 participants 8 messages Oct 10, 2008 – Oct 12, 2008 MMC, SD and SDIO

lore
Share & tools
lore

Ben Dooks

author
#0 lore raw

cover MMC: S3C24XX updates for s3cmci driver

Updates and fixes for drivers/mmc/host/s3cmci.c

Signature · 3 lines
Ben ([email protected], http://www.fluff.org/)

  'a smiley only costs 4 bytes'
author
#1 lore raw

PATCH 1/5 s3cmci: Make general protocol errors less noisy

General errors, such as timeouts during probe do not need to be sent to the console, so move them down to be included if the debug is enabled.

Such errors include:
s3c2440-sdi s3c2440-sdi: s3cmci_request: no medium present

Signed-off-by Ben Dooks <[email protected]>
drivers/mmc/host/s3cmci.c +2 -2
Index: linux-2.6.26-quilt8/drivers/mmc/host/s3cmci.c
===================================================================
@@ -39,9 +39,9 @@ enum dbg_channels {
 	dbg_conf  = (1 << 8),
 };
 
-static const int dbgmap_err   = dbg_err | dbg_fail;
+static const int dbgmap_err   = dbg_fail;
 static const int dbgmap_info  = dbg_info | dbg_conf;
-static const int dbgmap_debug = dbg_debug;
+static const int dbgmap_debug = dbg_err | dbg_debug;
 
 #define dbg(host, channels, args...)		  \
 	do {					  \
Signature · 3 lines
Ben ([email protected], http://www.fluff.org/)

  'a smiley only costs 4 bytes'
author
#2 lore raw

PATCH 2/5 s3cmci: cpufreq support

In reply to Ben Dooks

Support for cpu frequency changing.

Signed-off-by Ben Dooks <[email protected]>
drivers/mmc/host/s3cmci.c +92 -19
Index: linux-2.6.27-rc2-quilt3/drivers/mmc/host/s3cmci.c
===================================================================
@@ -13,6 +13,7 @@
 #include <linux/clk.h>
 #include <linux/mmc/host.h>
 #include <linux/platform_device.h>
+#include <linux/cpufreq.h>
 #include <linux/irq.h>
 #include <linux/io.h>
 

@@ -1033,10 +1034,33 @@ static void s3cmci_request(struct mmc_ho
 		s3cmci_send_request(mmc);
 }
 
+static void s3cmci_set_clk(struct s3cmci_host *host, struct mmc_ios *ios)
+{
+	u32 mci_psc;
+
+	/* Set clock */
+	for (mci_psc = 0; mci_psc < 255; mci_psc++) {
+		host->real_rate = host->clk_rate / (host->clk_div*(mci_psc+1));
+
+		if (host->real_rate <= ios->clock)
+			break;
+	}
+
+	if (mci_psc > 255)
+		mci_psc = 255;
+
+	host->prescaler = mci_psc;
+	writel(host->prescaler, host->base + S3C2410_SDIPRE);
+
+	/* If requested clock is 0, real_rate will be 0, too */
+	if (ios->clock == 0)
+		host->real_rate = 0;
+}
+
 static void s3cmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct s3cmci_host *host = mmc_priv(mmc);
-	u32 mci_psc, mci_con;
+	u32 mci_con;
 
 	/* Set the power state */
 

@@ -1074,23 +1098,7 @@ static void s3cmci_set_ios(struct mmc_ho
 		break;
 	}
 
-	/* Set clock */
-	for (mci_psc = 0; mci_psc < 255; mci_psc++) {
-		host->real_rate = host->clk_rate / (host->clk_div*(mci_psc+1));
-
-		if (host->real_rate <= ios->clock)
-			break;
-	}
-
-	if (mci_psc > 255)
-		mci_psc = 255;
-
-	host->prescaler = mci_psc;
-	writel(host->prescaler, host->base + S3C2410_SDIPRE);
-
-	/* If requested clock is 0, real_rate will be 0, too */
-	if (ios->clock == 0)
-		host->real_rate = 0;
+	s3cmci_set_clk(host, ios);
 
 	/* Set CLOCK_ENABLE */
 	if (ios->clock)

@@ -1148,6 +1156,61 @@ static struct s3c24xx_mci_pdata s3cmci_d
 	 * checks. Any zero fields to ensure reaonable defaults are picked. */
 };
 
+#ifdef CONFIG_CPU_FREQ
+
+static int s3cmci_cpufreq_transition(struct notifier_block *nb,
+				     unsigned long val, void *data)
+{
+	struct s3cmci_host *host;
+	struct mmc_host *mmc;
+	unsigned long newclk;
+	unsigned long flags;
+
+	host = container_of(nb, struct s3cmci_host, freq_transition);
+	newclk = clk_get_rate(host->clk);
+	mmc = host->mmc;
+
+	if ((val == CPUFREQ_PRECHANGE && host->clk_rate > newclk) ||
+	    (val == CPUFREQ_POSTCHANGE && host->clk_rate < newclk)) {
+		spin_lock_irqsave(&mmc->lock, flags);
+
+		host->clk_rate = newclk;
+
+		if (mmc->ios.power_mode != MMC_POWER_OFF &&
+		    mmc->ios.clock != 0)
+			s3cmci_set_clk(host, &mmc->ios);
+
+		spin_unlock_irqrestore(&mmc->lock, flags);
+	}
+
+	return 0;
+}
+
+static inline int s3cmci_cpufreq_register(struct s3cmci_host *host)
+{
+	host->freq_transition.notifier_call = s3cmci_cpufreq_transition;
+
+	return cpufreq_register_notifier(&host->freq_transition,
+					 CPUFREQ_TRANSITION_NOTIFIER);
+}
+
+static inline void s3cmci_cpufreq_deregister(struct s3cmci_host *host)
+{
+	cpufreq_unregister_notifier(&host->freq_transition,
+				    CPUFREQ_TRANSITION_NOTIFIER);
+}
+
+#else
+static inline int s3cmci_cpufreq_register(struct s3cmci_host *host)
+{
+	return 0;
+}
+
+static inline void s3cmci_cpufreq_deregister(struct s3cmci_host *host)
+{
+}
+#endif
+
 static int __devinit s3cmci_probe(struct platform_device *pdev, int is2440)
 {
 	struct s3cmci_host *host;

@@ -1298,10 +1361,16 @@ static int __devinit s3cmci_probe(struct
 	    (host->is2440?"2440":""),
 	    host->base, host->irq, host->irq_cd, host->dma);
 
+	ret = s3cmci_cpufreq_register(host);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register cpufreq\n");
+		goto free_dmabuf;
+	}
+
 	ret = mmc_add_host(mmc);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to add mmc host.\n");
-		goto free_dmabuf;
+		goto free_cpufreq;
 	}
 
 	platform_set_drvdata(pdev, mmc);

@@ -1309,6 +1378,9 @@ static int __devinit s3cmci_probe(struct
 
 	return 0;
 
+ free_cpufreq:
+	s3cmci_cpufreq_deregister(host);
+
  free_dmabuf:
 	clk_disable(host->clk);
 

@@ -1342,6 +1414,7 @@ static void s3cmci_shutdown(struct platf
 	if (host->irq_cd >= 0)
 		free_irq(host->irq_cd, host);
 
+	s3cmci_cpufreq_deregister(host);
 	mmc_remove_host(mmc);
 	clk_disable(host->clk);
 }
drivers/mmc/host/s3cmci.h +4 -0
Index: linux-2.6.27-rc2-quilt3/drivers/mmc/host/s3cmci.h
===================================================================
@@ -67,4 +67,8 @@ struct s3cmci_host {
 
 	unsigned int		ccnt, dcnt;
 	struct tasklet_struct	pio_tasklet;
+
+#ifdef CONFIG_CPU_FREQ
+	struct notifier_block	freq_transition;
+#endif
 };
Signature · 3 lines
Ben ([email protected], http://www.fluff.org/)

  'a smiley only costs 4 bytes'
#3 lore raw

Re: [patch 2/5] s3cmci: cpufreq support

drivers/mmc/host/s3cmci.c static struct s3c24xx_mci_pdata s3cmci_d · line 1174

Quoted 6 lines · On Fri, 10 Oct 2008 11:32:23 +0100 Ben Dooks <[email protected]> wrote:
drivers/mmc/host/s3cmci.c +6 -0
+	host = container_of(nb, struct s3cmci_host, freq_transition);
+	newclk = clk_get_rate(host->clk);
+	mmc = host->mmc;
+
+	if ((val == CPUFREQ_PRECHANGE && host->clk_rate > newclk) ||
+	    (val == CPUFREQ_POSTCHANGE && host->clk_rate < newclk)) {

Isn't this backwards? The goal should be to avoid overclocking the card, so if the old clock (which I assume is clk_rate) is slower than the new clock (newclk), then we should adjust before the change happens, temporarily underclocking the card.

(and vice versa for when the old clock is faster than the new clock)

Signature · 9 lines
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  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.
author
#4 lore raw

PATCH 3/5 s3cmci: Support transfers which are not multiple of 32 bits.

In reply to Ben Dooks

From: Christer Weinigel <[email protected]>

To be able to do SDIO the s3cmci driver has to support non-word-sized transfers. Change pio_words into pio_bytes and fix up all the places where it is used.

This variant of the patch will not overrun the buffer when reading an odd number of bytes. When writing, this variant will still read past the end of the buffer, but since the driver can't support non-word- aligned transfers anyway, this should not be a problem, since a word-aligned transfer will never cross a page boundary.

This has been tested with a CSR SDIO Bluetooth Type A device on a Samsung S3C24A0 processor.

Signed-off-by Christer Weinigel <[email protected]>
Signed-off-by Ben Dooks <[email protected]>
drivers/mmc/host/s3cmci.c +53 -27
Index: linux-2.6.27-rc5-quilt1/drivers/mmc/host/s3cmci.c
===================================================================
@@ -190,7 +190,7 @@ static inline void clear_imask(struct s3
 }
 
 static inline int get_data_buffer(struct s3cmci_host *host,
-				  u32 *words, u32 **pointer)
+				  u32 *bytes, u32 **pointer)
 {
 	struct scatterlist *sg;
 

@@ -207,7 +207,7 @@ static inline int get_data_buffer(struct
 	}
 	sg = &host->mrq->data->sg[host->pio_sgptr];
 
-	*words = sg->length >> 2;
+	*bytes = sg->length;
 	*pointer = sg_virt(sg);
 
 	host->pio_sgptr++;

@@ -223,7 +223,7 @@ static inline u32 fifo_count(struct s3cm
 	u32 fifostat = readl(host->base + S3C2410_SDIFSTA);
 
 	fifostat &= S3C2410_SDIFSTA_COUNTMASK;
-	return fifostat >> 2;
+	return fifostat;
 }
 
 static inline u32 fifo_free(struct s3cmci_host *host)

@@ -231,13 +231,14 @@ static inline u32 fifo_free(struct s3cmc
 	u32 fifostat = readl(host->base + S3C2410_SDIFSTA);
 
 	fifostat &= S3C2410_SDIFSTA_COUNTMASK;
-	return (63 - fifostat) >> 2;
+	return 63 - fifostat;
 }
 
 static void do_pio_read(struct s3cmci_host *host)
 {
 	int res;
 	u32 fifo;
+	u32 fifo_words;
 	void __iomem *from_ptr;
 
 	/* write real prescaler to host, it might be set slow to fix */

@@ -246,8 +247,8 @@ static void do_pio_read(struct s3cmci_ho
 	from_ptr = host->base + host->sdidata;
 
 	while ((fifo = fifo_count(host))) {
-		if (!host->pio_words) {
-			res = get_data_buffer(host, &host->pio_words,
+		if (!host->pio_bytes) {
+			res = get_data_buffer(host, &host->pio_bytes,
 					      &host->pio_ptr);
 			if (res) {
 				host->pio_active = XFER_NONE;

@@ -260,26 +261,45 @@ static void do_pio_read(struct s3cmci_ho
 
 			dbg(host, dbg_pio,
 			    "pio_read(): new target: [%i]@[%p]\n",
-			    host->pio_words, host->pio_ptr);
+			    host->pio_bytes, host->pio_ptr);
 		}
 
 		dbg(host, dbg_pio,
 		    "pio_read(): fifo:[%02i] buffer:[%03i] dcnt:[%08X]\n",
-		    fifo, host->pio_words,
+		    fifo, host->pio_bytes,
 		    readl(host->base + S3C2410_SDIDCNT));
 
-		if (fifo > host->pio_words)
-			fifo = host->pio_words;
+		/* If we have reached the end of the block, we can
+		 * read a word and get 1 to 3 bytes.  If we in the
+		 * middle of the block, we have to read full words,
+		 * otherwise we will write garbage, so round down to
+		 * an even multiple of 4. */
+		if (fifo >= host->pio_bytes)
+			fifo = host->pio_bytes;
+		else
+			fifo -= fifo & 3;
 
-		host->pio_words -= fifo;
+		host->pio_bytes -= fifo;
 		host->pio_count += fifo;
 
-		while (fifo--)
+		fifo_words = fifo >> 2;
+		while (fifo_words--)
 			*(host->pio_ptr++) = readl(from_ptr);
+
+		if (fifo & 3) {
+			u32 n = fifo & 3;
+			u32 data = readl(from_ptr);
+			u8 *p = (u8 *)host->pio_ptr;
+
+			while (n--) {
+				*p++ = data;
+				data >>= 8;
+			}
+		}
 	}
 
-	if (!host->pio_words) {
-		res = get_data_buffer(host, &host->pio_words, &host->pio_ptr);
+	if (!host->pio_bytes) {
+		res = get_data_buffer(host, &host->pio_bytes, &host->pio_ptr);
 		if (res) {
 			dbg(host, dbg_pio,
 			    "pio_read(): complete (no more buffers).\n");

@@ -303,8 +323,8 @@ static void do_pio_write(struct s3cmci_h
 	to_ptr = host->base + host->sdidata;
 
 	while ((fifo = fifo_free(host))) {
-		if (!host->pio_words) {
-			res = get_data_buffer(host, &host->pio_words,
+		if (!host->pio_bytes) {
+			res = get_data_buffer(host, &host->pio_bytes,
 							&host->pio_ptr);
 			if (res) {
 				dbg(host, dbg_pio,

@@ -316,16 +336,23 @@ static void do_pio_write(struct s3cmci_h
 
 			dbg(host, dbg_pio,
 			    "pio_write(): new source: [%i]@[%p]\n",
-			    host->pio_words, host->pio_ptr);
+			    host->pio_bytes, host->pio_ptr);
 
 		}
 
-		if (fifo > host->pio_words)
-			fifo = host->pio_words;
+		/* If we have reached the end of the block, we have to
+		 * write exactly the remaining number of bytes.  If we
+		 * in the middle of the block, we have to write full
+		 * words, so round down to an even multiple of 4. */
+		if (fifo >= host->pio_bytes)
+			fifo = host->pio_bytes;
+		else
+			fifo -= fifo & 3;
 
-		host->pio_words -= fifo;
+		host->pio_bytes -= fifo;
 		host->pio_count += fifo;
 
+		fifo = (fifo + 3) >> 2;
 		while (fifo--)
 			writel(*(host->pio_ptr++), to_ptr);
 	}

@@ -350,9 +377,9 @@ static void pio_tasklet(unsigned long da
 		clear_imask(host);
 		if (host->pio_active != XFER_NONE) {
 			dbg(host, dbg_err, "unfinished %s "
-			    "- pio_count:[%u] pio_words:[%u]\n",
+			    "- pio_count:[%u] pio_bytes:[%u]\n",
 			    (host->pio_active == XFER_READ) ? "read" : "write",
-			    host->pio_count, host->pio_words);
+			    host->pio_count, host->pio_bytes);
 
 			if (host->mrq->data)
 				host->mrq->data->error = -EINVAL;

@@ -813,11 +840,10 @@ static int s3cmci_setup_data(struct s3cm
 		/* We cannot deal with unaligned blocks with more than
 		 * one block being transfered. */
 
-		if (data->blocks > 1)
+		if (data->blocks > 1) {
+			pr_warning("%s: can't do non-word sized block transfers (blksz %d)\n", __func__, data->blksz);
 			return -EINVAL;
-
-		/* No support yet for non-word block transfers. */
-		return -EINVAL;
+		}
 	}
 
 	while (readl(host->base + S3C2410_SDIDSTA) &

@@ -897,7 +923,7 @@ static int s3cmci_prepare_pio(struct s3c
 	BUG_ON((data->flags & BOTH_DIR) == BOTH_DIR);
 
 	host->pio_sgptr = 0;
-	host->pio_words = 0;
+	host->pio_bytes = 0;
 	host->pio_count = 0;
 	host->pio_active = rw ? XFER_WRITE : XFER_READ;
drivers/mmc/host/s3cmci.h +1 -1
Index: linux-2.6.27-rc5-quilt1/drivers/mmc/host/s3cmci.h
===================================================================
@@ -51,7 +51,7 @@ struct s3cmci_host {
 	int			dma_complete;
 
 	u32			pio_sgptr;
-	u32			pio_words;
+	u32			pio_bytes;
 	u32			pio_count;
 	u32			*pio_ptr;
 #define XFER_NONE 0
Signature · 3 lines
Ben ([email protected], http://www.fluff.org/)

  'a smiley only costs 4 bytes'
author
#5 lore raw

PATCH 4/5 s3cmci: fix continual accesses to host->pio_ptr

In reply to Ben Dooks

The s3cmci driver uses the host->pio_ptr field to point to the current position into the buffer for data transfer. During the transfers it does the following:

	while (fifo_words--)
		*(host->pio_ptr++) = readl(from_ptr);

This is inefficent, as host->pio_ptr is not used in any other part of the transfer but the compiler emits code which does the following:

	while (fifo_words--) {
		u32 *ptr = host->pio_ptr;
		*ptr = readl(from_ptr);
		ptr++;
		host->pio_ptr = ptr;
	}

This is obviously a waste of a load and store each time around the loop, which could be up to 16 times depending on how much needs to be transfered.

Move the ptr accesses to outside the while loop so that we do not end up reloading/re-writing the pointer.

Note, this seems to make the code 16 bytes larger.

Signed-off-by Ben Dooks <[email protected]>
drivers/mmc/host/s3cmci.c +8 -2
Index: linux-2.6.27-rc5-quilt1/drivers/mmc/host/s3cmci.c
===================================================================
@@ -238,6 +238,7 @@ static void do_pio_read(struct s3cmci_ho
 {
 	int res;
 	u32 fifo;
+	u32 *ptr;
 	u32 fifo_words;
 	void __iomem *from_ptr;
 

@@ -283,8 +284,10 @@ static void do_pio_read(struct s3cmci_ho
 		host->pio_count += fifo;
 
 		fifo_words = fifo >> 2;
+		ptr = host->pio_ptr;
 		while (fifo_words--)
-			*(host->pio_ptr++) = readl(from_ptr);
+			*ptr++ = readl(from_ptr);
+		host->pio_ptr = ptr;
 
 		if (fifo & 3) {
 			u32 n = fifo & 3;

@@ -319,6 +322,7 @@ static void do_pio_write(struct s3cmci_h
 	void __iomem *to_ptr;
 	int res;
 	u32 fifo;
+	u32 *ptr;
 
 	to_ptr = host->base + host->sdidata;
 

@@ -353,8 +357,10 @@ static void do_pio_write(struct s3cmci_h
 		host->pio_count += fifo;
 
 		fifo = (fifo + 3) >> 2;
+		ptr = host->pio_ptr;
 		while (fifo--)
-			writel(*(host->pio_ptr++), to_ptr);
+			writel(*ptr++, to_ptr);
+		host->pio_ptr = ptr;
 	}
 
 	enable_imask(host, S3C2410_SDIIMSK_TXFIFOHALF);
Signature · 3 lines
Ben ([email protected], http://www.fluff.org/)

  'a smiley only costs 4 bytes'
author
#6 lore raw

PATCH 5/5 s3cmci: Add Ben Dooks/Simtec Electronics to header & copyright

In reply to Ben Dooks

Since the original authour (Thomas Kleffel) has been too busy to merge the s3cmci driver and keep it up to date, I (mostly as part of my role with Simtec Electronics) got the driver to a mergable state and have been maintaining it since I think that I should be added to the header. Also add a copyright statement for the new work.

Signed-off-by Ben Dooks <[email protected]>
drivers/mmc/host/s3cmci.c +4 -1
Index: linux-2.6.27-rc6-quilt3/drivers/mmc/host/s3cmci.c
===================================================================
@@ -3,6 +3,9 @@
  *
  *  Copyright (C) 2004-2006 maintech GmbH, Thomas Kleffel <[email protected]>
  *
+ * Current driver maintained by Ben Dooks and Simtec Electronics
+ *  Copyright (C) 2008 Simtec Electronics <[email protected]>
+ *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.

@@ -1560,7 +1563,7 @@ module_exit(s3cmci_exit);
 
 MODULE_DESCRIPTION("Samsung S3C MMC/SD Card Interface driver");
 MODULE_LICENSE("GPL v2");
-MODULE_AUTHOR("Thomas Kleffel <[email protected]>");
+MODULE_AUTHOR("Thomas Kleffel <[email protected]>, Ben Dooks <[email protected]>");
 MODULE_ALIAS("platform:s3c2410-sdi");
 MODULE_ALIAS("platform:s3c2412-sdi");
 MODULE_ALIAS("platform:s3c2440-sdi");
Signature · 3 lines
Ben ([email protected], http://www.fluff.org/)

  'a smiley only costs 4 bytes'
#7 lore raw
In reply to Ben Dooks
Quoted 2 lines · On Fri, 10 Oct 2008 11:32:21 +0100 Ben Dooks <[email protected]> wrote:

Updates and fixes for drivers/mmc/host/s3cmci.c

In the future, please try to send them a bit earlier so that I have a reasonable amount of time to look at them. It also doesn't hurt to have them in linux-next for a bit (which might be a requirement soon anyway).

Rgds

Signature · 9 lines
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  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.