[patch 0/8] Update Samsung SDHCI

[patch 0/8] Update Samsung SDHCI

[Ben Dooks]

This is an updated set of SDHCI patches to add support
for the Samsung S3C64XX and S3C2443 SoC range.

The DMA check is still in there, it hasn't been triggered
since getting the timeout clock wrong.

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

[patch 1/8] SDHCI: Add timeout hooks

[Ben Dooks]

[-- Attachment #1: simtec/s3c64xx/sdhci-info-hooks1.patch --]
[-- Type: text/plain, Size: 2209 bytes --]

Some controllers do not provide clock information in their
capabilities (in the Samsung case, it is because there are
multiple clock sources available to the controller). Add hooks
to allow the system to supply clock information.

Signed-off-by: Ben Dooks <ben-linux@fluff.org>

Index: linux.git/drivers/mmc/host/sdhci.c
===================================================================
--- linux.git.orig/drivers/mmc/host/sdhci.c	2008-12-01 19:11:54.000000000 +0000
+++ linux.git/drivers/mmc/host/sdhci.c	2008-12-01 19:24:14.000000000 +0000
@@ -1606,18 +1606,26 @@ int sdhci_add_host(struct sdhci_host *ho
 
 	host->max_clk =
 		(caps & SDHCI_CLOCK_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT;
+	host->max_clk *= 1000000;
 	if (host->max_clk == 0) {
-		printk(KERN_ERR "%s: Hardware doesn't specify base clock "
-			"frequency.\n", mmc_hostname(mmc));
+		if (host->ops->get_max_clock)
+			host->max_clk = host->ops->get_max_clock(host);
+		else
+			printk(KERN_ERR
+			       "%s: Hardware doesn't specify base clock "
+			       "frequency.\n", mmc_hostname(mmc));
 		return -ENODEV;
 	}
-	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));
+		if (host->ops->get_timeout_clock)
+			host->timeout_clk = host->ops->get_timeout_clock(host);
+		else
+			printk(KERN_ERR
+			       "%s: Hardware doesn't specify timeout clock "
+			       "frequency.\n", mmc_hostname(mmc));
 		return -ENODEV;
 	}
 	if (caps & SDHCI_TIMEOUT_CLK_UNIT)
Index: linux.git/drivers/mmc/host/sdhci.h
===================================================================
--- linux.git.orig/drivers/mmc/host/sdhci.h	2008-12-01 19:11:54.000000000 +0000
+++ linux.git/drivers/mmc/host/sdhci.h	2008-12-01 19:17:55.000000000 +0000
@@ -267,6 +267,8 @@ struct sdhci_host {
 
 struct sdhci_ops {
 	int		(*enable_dma)(struct sdhci_host *host);
+	unsigned int	(*get_max_clock)(struct sdhci_host *host);
+	unsigned int	(*get_timeout_clock)(struct sdhci_host *host);
 };
 
 

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [patch 1/8] SDHCI: Add timeout hooks

[smohideen]

On Tue, Dec 02, 2008 at 03:40:19PM +0000, Ben Dooks wrote:
> Some controllers do not provide clock information in their
> capabilities (in the Samsung case, it is because there are
> multiple clock sources available to the controller). Add hooks
> to allow the system to supply clock information.
> 
> Signed-off-by: Ben Dooks <ben-linux@fluff.org>
> 
> Index: linux.git/drivers/mmc/host/sdhci.c
> ===================================================================
> --- linux.git.orig/drivers/mmc/host/sdhci.c	2008-12-01 19:11:54.000000000 +0000
> +++ linux.git/drivers/mmc/host/sdhci.c	2008-12-01 19:24:14.000000000 +0000
> @@ -1606,18 +1606,26 @@ int sdhci_add_host(struct sdhci_host *ho
>  
>  	host->max_clk =
>  		(caps & SDHCI_CLOCK_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT;
> +	host->max_clk *= 1000000;
>  	if (host->max_clk == 0) {
> -		printk(KERN_ERR "%s: Hardware doesn't specify base clock "
> -			"frequency.\n", mmc_hostname(mmc));
> +		if (host->ops->get_max_clock)
> +			host->max_clk = host->ops->get_max_clock(host);
> +		else
> +			printk(KERN_ERR
> +			       "%s: Hardware doesn't specify base clock "
> +			       "frequency.\n", mmc_hostname(mmc));
>  		return -ENODEV;
>  	}
> -	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));
> +		if (host->ops->get_timeout_clock)
> +			host->timeout_clk = host->ops->get_timeout_clock(host);
> +		else
> +			printk(KERN_ERR
> +			       "%s: Hardware doesn't specify timeout clock "
> +			       "frequency.\n", mmc_hostname(mmc));
>  		return -ENODEV;
>  	}
>  	if (caps & SDHCI_TIMEOUT_CLK_UNIT)
> Index: linux.git/drivers/mmc/host/sdhci.h
> ===================================================================
> --- linux.git.orig/drivers/mmc/host/sdhci.h	2008-12-01 19:11:54.000000000 +0000
> +++ linux.git/drivers/mmc/host/sdhci.h	2008-12-01 19:17:55.000000000 +0000
> @@ -267,6 +267,8 @@ struct sdhci_host {
>  
>  struct sdhci_ops {
>  	int		(*enable_dma)(struct sdhci_host *host);
> +	unsigned int	(*get_max_clock)(struct sdhci_host *host);
> +	unsigned int	(*get_timeout_clock)(struct sdhci_host *host);
>  };
>  

	Can we cache the multiple dereferences. Here host->ops can be
	saved to a local, instead of multiple dereference several time.

	

Re: [patch 1/8] SDHCI: Add timeout hooks

[Pierre Ossman]

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

On Tue, 2 Dec 2008 22:16:50 +0530
smohideen@mx2.labs.rootshell.ws wrote:

> 
> 	Can we cache the multiple dereferences. Here host->ops can be
> 	saved to a local, instead of multiple dereference several time.
> 

The ops are used fairly rarely and so far not in any hot-paths, so
there would not be much gain in doing that.

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 2/8] SDHCI: Print ADMA status and pointer on debug

[Ben Dooks]

[-- Attachment #1: simtec/s3c64xx/sdhci-print-adma-status-if-enabled.patch --]
[-- Type: text/plain, Size: 863 bytes --]

If using ADMA, then we should print the ADMA error
and current pointer in sdhci_dumpregs() when any
debug is requested.

Signed-off-by: Ben Dooks <ben-linux@fluff.org>

--- linux.git.orig/drivers/mmc/host/sdhci.c	2008-10-27 08:17:43.000000000 +0000
+++ linux.git/drivers/mmc/host/sdhci.c	2008-10-27 11:01:20.000000000 +0000
@@ -73,6 +73,11 @@ static void sdhci_dumpregs(struct sdhci_
 		readl(host->ioaddr + SDHCI_CAPABILITIES),
 		readl(host->ioaddr + SDHCI_MAX_CURRENT));
 
+	if (host->flags & SDHCI_USE_ADMA)
+		printk(KERN_DEBUG DRIVER_NAME ": ADMA Err: 0x%08x | ADMA Ptr: 0x%08x\n",
+		       readl(host->ioaddr + SDHCI_ADMA_ERROR),
+		       readl(host->ioaddr + SDHCI_ADMA_ADDRESS));
+
 	printk(KERN_DEBUG DRIVER_NAME ": ===========================================\n");
 }
 

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

[patch 3/8] SDHCI: Add set_ios hook

[Ben Dooks]

[-- Attachment #1: simtec/s3c64xx/sdhci-add-ios-hook.patch --]
[-- Type: text/plain, Size: 2138 bytes --]

Add a set_ios hook which is called when the SDHCI driver
is called to change parameters such as clock or card
width.

Signed-off-by: Ben Dooks <ben-linux@fluff.org>

Index: linux.git/drivers/mmc/host/sdhci.c
===================================================================
--- linux.git.orig/drivers/mmc/host/sdhci.c	2008-12-01 19:26:25.000000000 +0000
+++ linux.git/drivers/mmc/host/sdhci.c	2008-12-02 14:36:47.000000000 +0000
@@ -1038,6 +1038,9 @@ static void sdhci_set_ios(struct mmc_hos
 		sdhci_init(host);
 	}
 
+	if (host->ops->set_ios)
+		host->ops->set_ios(host, ios);
+
 	sdhci_set_clock(host, ios->clock);
 
 	if (ios->power_mode == MMC_POWER_OFF)
@@ -1615,11 +1618,12 @@ int sdhci_add_host(struct sdhci_host *ho
 	if (host->max_clk == 0) {
 		if (host->ops->get_max_clock)
 			host->max_clk = host->ops->get_max_clock(host);
-		else
+		else {
 			printk(KERN_ERR
 			       "%s: Hardware doesn't specify base clock "
 			       "frequency.\n", mmc_hostname(mmc));
-		return -ENODEV;
+			return -ENODEV;
+		}
 	}
 
 	host->timeout_clk =
@@ -1627,11 +1631,12 @@ int sdhci_add_host(struct sdhci_host *ho
 	if (host->timeout_clk == 0) {
 		if (host->ops->get_timeout_clock)
 			host->timeout_clk = host->ops->get_timeout_clock(host);
-		else
+		else {
 			printk(KERN_ERR
 			       "%s: Hardware doesn't specify timeout clock "
 			       "frequency.\n", mmc_hostname(mmc));
-		return -ENODEV;
+			return -ENODEV;
+		}
 	}
 	if (caps & SDHCI_TIMEOUT_CLK_UNIT)
 		host->timeout_clk *= 1000;
Index: linux.git/drivers/mmc/host/sdhci.h
===================================================================
--- linux.git.orig/drivers/mmc/host/sdhci.h	2008-12-01 19:17:55.000000000 +0000
+++ linux.git/drivers/mmc/host/sdhci.h	2008-12-02 14:36:39.000000000 +0000
@@ -269,6 +269,9 @@ struct sdhci_ops {
 	int		(*enable_dma)(struct sdhci_host *host);
 	unsigned int	(*get_max_clock)(struct sdhci_host *host);
 	unsigned int	(*get_timeout_clock)(struct sdhci_host *host);
+
+	void		(*set_ios)(struct sdhci_host *host,
+				   struct mmc_ios *ios);
 };
 
 

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [patch 3/8] SDHCI: Add set_ios hook

[Pierre Ossman]

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

On Tue, 02 Dec 2008 15:40:21 +0000
Ben Dooks <ben-linux@fluff.org> wrote:

> Add a set_ios hook which is called when the SDHCI driver
> is called to change parameters such as clock or card
> width.
> 
> Signed-off-by: Ben Dooks <ben-linux@fluff.org>
> 

NAK. As I explained earlier, this is a too broad hook and you haven't
given a proper reason for examining the card structure (which is all
that the set_ios callback does).

-- 
     -- 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 4/8] SDHCI: Add quirk for controller with no end-of-busy IRQ

[Ben Dooks]

[-- Attachment #1: simtec/s3c64xx/sdhci-add-no-busy-transfer-finish-irq-quirk.patch --]
[-- Type: text/plain, Size: 1731 bytes --]

The Samsung SDHCI 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>

Index: linux.git/drivers/mmc/host/sdhci.c
===================================================================
--- linux.git.orig/drivers/mmc/host/sdhci.c	2008-12-01 19:31:15.000000000 +0000
+++ linux.git/drivers/mmc/host/sdhci.c	2008-12-01 19:43:30.000000000 +0000
@@ -1294,8 +1294,11 @@ static void sdhci_cmd_irq(struct sdhci_h
 		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)
Index: linux.git/drivers/mmc/host/sdhci.h
===================================================================
--- linux.git.orig/drivers/mmc/host/sdhci.h	2008-12-01 19:31:15.000000000 +0000
+++ linux.git/drivers/mmc/host/sdhci.h	2008-12-01 19:42:27.000000000 +0000
@@ -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 */

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

[patch 5/8] SDHCI: Samsung SDHCI (HSMMC) driver

[Ben Dooks]

[-- Attachment #1: simtec/s3c64xx/sdhci-samsung.patch --]
[-- Type: text/plain, Size: 8038 bytes --]

Add support for the 'HSMMC' block(s) in the Samsung SoC
line. These are compatible with the SDHCI driver so add
the necessary setup and driver binding for the platform
devices.

Signed-off-by: Ben Dooks <ben-linux@fluff.org>

Index: linux.git/drivers/mmc/host/Kconfig
===================================================================
--- linux.git.orig/drivers/mmc/host/Kconfig	2008-12-01 23:18:20.000000000 +0000
+++ linux.git/drivers/mmc/host/Kconfig	2008-12-01 23:25:18.000000000 +0000
@@ -48,6 +48,18 @@ config MMC_SDHCI_PCI
 
 	  If unsure, say N.
 
+config MMC_SDHCI_S3C
+	tristate "SDHCI support on Samsung S3C SoC"
+	depends on MMC_SDHCI && (PLAT_S3C24XX || PLAT_S3C64XX)
+	help
+	  This selects the Secure Digital Host Controller Interface (SDHCI)
+	  often referrered to as the HSMMC block in some of the Samsung S3C
+	  range of SoC.
+
+	  If you have a controller with this interface, say Y or M here.
+
+	  If unsure, say N.
+
 config MMC_RICOH_MMC
 	tristate "Ricoh MMC Controller Disabler  (EXPERIMENTAL)"
 	depends on MMC_SDHCI_PCI
Index: linux.git/drivers/mmc/host/Makefile
===================================================================
--- linux.git.orig/drivers/mmc/host/Makefile	2008-12-01 23:18:20.000000000 +0000
+++ linux.git/drivers/mmc/host/Makefile	2008-12-01 23:25:18.000000000 +0000
@@ -11,6 +11,7 @@ obj-$(CONFIG_MMC_PXA)		+= pxamci.o
 obj-$(CONFIG_MMC_IMX)		+= imxmmc.o
 obj-$(CONFIG_MMC_SDHCI)		+= sdhci.o
 obj-$(CONFIG_MMC_SDHCI_PCI)	+= sdhci-pci.o
+obj-$(CONFIG_MMC_SDHCI_S3C)	+= sdhci-s3c.o
 obj-$(CONFIG_MMC_RICOH_MMC)	+= ricoh_mmc.o
 obj-$(CONFIG_MMC_WBSD)		+= wbsd.o
 obj-$(CONFIG_MMC_AU1X)		+= au1xmmc.o
Index: linux.git/drivers/mmc/host/sdhci-s3c.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux.git/drivers/mmc/host/sdhci-s3c.c	2008-12-02 14:13:21.000000000 +0000
@@ -0,0 +1,256 @@
+/* linux/drivers/mmc/host/sdhci-s3c.c
+ *
+ * Copyright 2008 Openmoko Inc.
+ * Copyright 2008 Simtec Electronics
+ *      Ben Dooks <ben@simtec.co.uk>
+ *      http://armlinux.simtec.co.uk/
+ *
+ * SDHCI (HSMMC) support for Samsung SoC
+ *
+ * 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.
+ */
+
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+
+#include <linux/mmc/host.h>
+
+#include <plat/sdhci.h>
+
+#include "sdhci.h"
+
+#define MAX_BUS_CLK	(4)
+
+struct sdhci_s3c {
+	struct sdhci_host	*host;
+	struct platform_device	*pdev;
+	struct resource		*ioarea;
+	struct s3c_sdhci_platdata *pdata;
+
+	struct clk		*clk_io;	/* clock for io bus */
+	struct clk		*clk_bus[MAX_BUS_CLK];
+};
+
+static inline struct sdhci_s3c *to_s3c(struct sdhci_host *host)
+{
+	return sdhci_priv(host);
+}
+
+static unsigned int sdhci_s3c_get_max_clk(struct sdhci_host *host)
+{
+	struct sdhci_s3c *ourhost = to_s3c(host);
+	unsigned int rate;
+	u32 control2;
+	int clk;
+
+	/* note, a reset will reset the clock source */
+
+	control2 = readl(host->ioaddr + 0x80);
+	clk = clk_get_rate(ourhost->clk_bus[(control2 >> 4) & 3]);
+
+	return clk;
+}
+
+static unsigned int sdhci_s3c_get_timeout_clk(struct sdhci_host *host)
+{
+	return sdhci_s3c_get_max_clk(host) / 1000000;
+}
+
+static void sdhci_s3c_set_ios(struct sdhci_host *host,
+			      struct mmc_ios *ios)
+{
+	struct sdhci_s3c *ourhost = to_s3c(host);
+	struct s3c_sdhci_platdata *pdata = ourhost->pdata;
+
+	if (pdata->cfg_card)
+		pdata->cfg_card(ourhost->pdev, host->ioaddr, ios);
+}
+
+static struct sdhci_ops sdhci_s3c_ops = {
+	.get_max_clock		= sdhci_s3c_get_max_clk,
+	.get_timeout_clock	= sdhci_s3c_get_timeout_clk,
+	.set_ios		= sdhci_s3c_set_ios,
+};
+
+static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
+{
+	struct s3c_sdhci_platdata *pdata = pdev->dev.platform_data;
+	struct device *dev = &pdev->dev;
+	struct sdhci_host *host;
+	struct sdhci_s3c *sc;
+	struct resource *res;
+	int ret, irq, ptr, clks;
+
+	if (!pdata) {
+		dev_err(dev, "no device data specified\n");
+		return -ENOENT;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "no irq specified\n");
+		return irq;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(dev, "no memory specified\n");
+		return -ENOENT;
+	}
+
+	host = sdhci_alloc_host(dev, sizeof(struct sdhci_s3c));
+	if (IS_ERR(host)) {
+		dev_err(dev, "sdhci_alloc_host() failed\n");
+		return PTR_ERR(host);
+	}
+
+	sc = sdhci_priv(host);
+
+	sc->host = host;
+	sc->pdev = pdev;
+	sc->pdata = pdata;
+
+	sc->clk_io = clk_get(dev, "hsmmc");
+	if (IS_ERR(sc->clk_io)) {
+		dev_err(dev, "failed to get io clock\n");
+		ret = PTR_ERR(sc->clk_io);
+		goto err_io_clk;
+	}
+
+	/* enable the local io clock and keep it running for the moment. */
+	clk_enable(sc->clk_io);
+
+	for (clks = 0, ptr = 0; ptr < MAX_BUS_CLK; ptr++) {
+		char *name = pdata->clocks[ptr];
+		struct clk *clk;
+
+		if (name == NULL)
+			continue;
+
+		clk = clk_get(dev, name);
+		if (IS_ERR(clk)) {
+			dev_err(dev, "failed to get clock %s\n", name);
+			continue;
+		}
+
+		clks++;
+		sc->clk_bus[ptr] = clk;
+		clk_enable(clk);
+
+		dev_info(dev, "clock source %d: %s (%ld Hz)\n",
+			 ptr, name, clk_get_rate(clk));
+	}
+
+	if (clks == 0) {
+		dev_err(dev, "failed to find any bus clocks\n");
+		ret = -ENOENT;
+		goto err_no_busclks;
+	}
+
+	sc->ioarea = request_mem_region(res->start, resource_size(res),
+					mmc_hostname(host->mmc));
+	if (!sc->ioarea) {
+		dev_err(dev, "failed to reserve register area\n");
+		ret = -ENXIO;
+		goto err_req_regs;
+	}
+
+	host->ioaddr = ioremap_nocache(res->start, resource_size(res));
+	if (!host->ioaddr) {
+		dev_err(dev, "failed to map registers\n");
+		ret = -ENXIO;
+		goto err_req_regs;
+	}
+
+	/* Setup our GPIO */
+	if (pdata->cfg_gpio)
+		pdata->cfg_gpio(pdev, pdata->max_width);
+
+	host->hw_name = "samsung-hsmmc";
+	host->ops = &sdhci_s3c_ops;
+	host->quirks = 0;
+	host->irq = irq;
+
+	/* Setup quirks for the controller */
+
+	/* Currently with ADMA enabled we are getting some length
+	 * interrupts that are not being dealt with, so disable
+	 * ADMA until this is sorted out. */
+	host->quirks |= SDHCI_QUIRK_BROKEN_ADMA;
+	host->quirks |= SDHCI_QUIRK_32BIT_ADMA_SIZE;
+
+	/* It seems we do not get an DATA transfer complete on non-busy
+	 * transfers, not sure if this is a problem with this specific
+	 * SDHCI block, or a missing configuration that needs to be set. */
+	host->quirks |= SDHCI_QUIRK_NO_BUSY_IRQ;
+
+	host->quirks |= (SDHCI_QUIRK_32BIT_DMA_ADDR |
+			 SDHCI_QUIRK_32BIT_DMA_SIZE);
+
+	ret = sdhci_add_host(host);
+	if (ret) {
+		dev_err(dev, "sdhci_add_host() failed (%d)\n", ret);
+		goto err_add_host;
+	}
+
+	return 0;
+
+ err_add_host:
+	release_resource(sc->ioarea);
+	kfree(sc->ioarea);
+
+ err_req_regs:
+	for (ptr = 0; ptr < MAX_BUS_CLK; ptr++) {
+		if (!sc->clk_bus[ptr])
+			continue;
+
+		clk_disable(sc->clk_bus[ptr]);
+		clk_put(sc->clk_bus[ptr]);
+	}
+
+ err_no_busclks:
+	clk_disable(sc->clk_io);
+	clk_put(sc->clk_io);
+
+ err_io_clk:
+	sdhci_free_host(host);
+
+	return ret;
+}
+
+static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
+{
+	return 0;
+}
+
+static struct platform_driver sdhci_s3c_driver = {
+	.probe		= sdhci_s3c_probe,
+	.remove		= __devexit_p(sdhci_s3c_remove),
+	.driver		= {
+		.owner	= THIS_MODULE,
+		.name	= "s3c-sdhci",
+	},
+};
+
+static int __init sdhci_s3c_init(void)
+{
+	return platform_driver_register(&sdhci_s3c_driver);
+}
+
+static void __exit sdhci_s3c_exit(void)
+{
+	platform_driver_unregister(&sdhci_s3c_driver);
+}
+
+module_init(sdhci_s3c_init);
+module_exit(sdhci_s3c_exit);
+
+MODULE_DESCRIPTION("Samsung SDHCI (HSMMC) glue");
+MODULE_AUTHOR("Ben Dooks, <ben@simtec.co.uk>");
+MODULE_LICENSE("GPLv2");
+MODULE_ALIAS("platform:s3c-sdhci");

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [patch 5/8] SDHCI: Samsung SDHCI (HSMMC) driver

[Pierre Ossman]

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

On Tue, 02 Dec 2008 15:40:23 +0000
Ben Dooks <ben-linux@fluff.org> wrote:

> Index: linux.git/drivers/mmc/host/Kconfig
> ===================================================================
> --- linux.git.orig/drivers/mmc/host/Kconfig	2008-12-01 23:18:20.000000000 +0000
> +++ linux.git/drivers/mmc/host/Kconfig	2008-12-01 23:25:18.000000000 +0000
> @@ -48,6 +48,18 @@ config MMC_SDHCI_PCI
>  
>  	  If unsure, say N.
>  
> +config MMC_SDHCI_S3C
> +	tristate "SDHCI support on Samsung S3C SoC"
> +	depends on MMC_SDHCI && (PLAT_S3C24XX || PLAT_S3C64XX)
> +	help
> +	  This selects the Secure Digital Host Controller Interface (SDHCI)
> +	  often referrered to as the HSMMC block in some of the Samsung S3C
> +	  range of SoC.
> +
> +	  If you have a controller with this interface, say Y or M here.
> +
> +	  If unsure, say N.
> +
>  config MMC_RICOH_MMC
>  	tristate "Ricoh MMC Controller Disabler  (EXPERIMENTAL)"
>  	depends on MMC_SDHCI_PCI

You should probably put it under MMC_RICOH_MMC as that will become a
subentry under MMC_SDHCI_PCI.

> +static void sdhci_s3c_set_ios(struct sdhci_host *host,
> +			      struct mmc_ios *ios)
> +{
> +	struct sdhci_s3c *ourhost = to_s3c(host);
> +	struct s3c_sdhci_platdata *pdata = ourhost->pdata;
> +
> +	if (pdata->cfg_card)
> +		pdata->cfg_card(ourhost->pdev, host->ioaddr, ios);
> +}

I'm still waiting for a proper reason to allow this hook. Since you've
added this, I assume you have at least one implementation of cfg_card.
Care to include that so that I can see what it is you're trying to do?

-- 
     -- 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 6/8] SDHCI: Check DMA for overruns at end of transfer

[Ben Dooks]

[-- Attachment #1: simtec/s3c64xx/debug-card.patch --]
[-- Type: text/plain, Size: 1961 bytes --]

At the end of a transfer, check that the DMA engine in the
SDHCI controller actually did what it was meant to and didn't
overrun the end of the buffer.

This seems to be triggered by a timeout during an CMD25 (multiple block         
write) to a card. The mmc_block module then issues a command to find out        
how much data was moved and this seems to end up triggering this DMA            
check. The result is the card's queue generates an OOPS as the stack has        
been trampled on due to the extra data transfered.

Signed-off-by: Ben Dooks <ben-linux@fluff.org>

Index: linux.git/drivers/mmc/host/sdhci.c
===================================================================
--- linux.git.orig/drivers/mmc/host/sdhci.c	2008-10-29 16:59:38.000000000 +0000
+++ linux.git/drivers/mmc/host/sdhci.c	2008-10-29 16:59:41.000000000 +0000
@@ -736,6 +736,23 @@ static void sdhci_set_transfer_mode(stru
 	writew(mode, host->ioaddr + SDHCI_TRANSFER_MODE);
 }
 
+static void shdci_check_dma_overrun(struct sdhci_host *host, struct mmc_data *data)
+{
+	u32 dma_pos = readl(host->ioaddr + SDHCI_DMA_ADDRESS);
+	u32 dma_start = sg_dma_address(data->sg);
+	u32 dma_end = dma_start + data->sg->length;
+
+	/* Test whether we ended up moving more data than
+	 * was originally requested. */
+
+	if (dma_pos <= dma_end)
+		return;
+
+	printk(KERN_ERR "%s: dma overrun, dma %08x, req %08x..%08x\n",
+	       mmc_hostname(host->mmc), dma_pos,
+	       dma_start, dma_end);
+}
+
 static void sdhci_finish_data(struct sdhci_host *host)
 {
 	struct mmc_data *data;
@@ -749,6 +766,8 @@ static void sdhci_finish_data(struct sdh
 		if (host->flags & SDHCI_USE_ADMA)
 			sdhci_adma_table_post(host, data);
 		else {
+			shdci_check_dma_overrun(host, data);
+
 			dma_unmap_sg(mmc_dev(host->mmc), data->sg,
 				data->sg_len, (data->flags & MMC_DATA_READ) ?
 					DMA_FROM_DEVICE : DMA_TO_DEVICE);

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [patch 6/8] SDHCI: Check DMA for overruns at end of transfer

[Pierre Ossman]

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

On Tue, 02 Dec 2008 15:40:24 +0000
Ben Dooks <ben-linux@fluff.org> wrote:

> At the end of a transfer, check that the DMA engine in the
> SDHCI controller actually did what it was meant to and didn't
> overrun the end of the buffer.
> 
> This seems to be triggered by a timeout during an CMD25 (multiple block         
> write) to a card. The mmc_block module then issues a command to find out        
> how much data was moved and this seems to end up triggering this DMA            
> check. The result is the card's queue generates an OOPS as the stack has        
> been trampled on due to the extra data transfered.
> 

Hmm... this is even stranger. CMD25 (write) will only read from main
memory and should not be able to mess up the stack. And it shouldn't be
the stack getting messed up anyway as that is not near the buffer
memory.

I still consider this a critical bug since it seems to be provoked by
hitting the timeout condition.

I'll queue up your patches (once the other things are fixed), but I
will add a patch disabling DMA on this controller until this is
properly understood and handled.

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 7/8] SDHCI: Add change_clock callback for glue drivers

[Ben Dooks]

[-- Attachment #1: simtec/s3c64xx/sdhci-select-clock.patch --]
[-- Type: text/plain, Size: 6954 bytes --]

Add a change_clock callback to allow drivers to update
device specific clock selections and control registers
when there is a change in clock.

Move the main part of sdhci_set_clock() to a new routine
which can be called by the glue drivers to do the sdhci
standard clock management.

Update the sdhci-s3c driver to use this to select the
appropriate clock source when clocks change.

Signed-off-by: Ben Dooks <ben-linux@fluff.org>

Index: linux.git/drivers/mmc/host/sdhci-pci.c
===================================================================
--- linux.git.orig/drivers/mmc/host/sdhci-pci.c	2008-12-02 14:36:39.000000000 +0000
+++ linux.git/drivers/mmc/host/sdhci-pci.c	2008-12-02 14:36:52.000000000 +0000
@@ -391,6 +391,7 @@ static int sdhci_pci_enable_dma(struct s
 
 static struct sdhci_ops sdhci_pci_ops = {
 	.enable_dma	= sdhci_pci_enable_dma,
+	.change_clock	= sdhci_change_clock,
 };
 
 /*****************************************************************************\
Index: linux.git/drivers/mmc/host/sdhci-s3c.c
===================================================================
--- linux.git.orig/drivers/mmc/host/sdhci-s3c.c	2008-12-02 14:36:52.000000000 +0000
+++ linux.git/drivers/mmc/host/sdhci-s3c.c	2008-12-02 14:47:04.000000000 +0000
@@ -20,6 +20,7 @@
 
 #include <linux/mmc/host.h>
 
+#include <plat/regs-sdhci.h>
 #include <plat/sdhci.h>
 
 #include "sdhci.h"
@@ -31,6 +32,7 @@ struct sdhci_s3c {
 	struct platform_device	*pdev;
 	struct resource		*ioarea;
 	struct s3c_sdhci_platdata *pdata;
+	unsigned int		cur_clk;
 
 	struct clk		*clk_io;	/* clock for io bus */
 	struct clk		*clk_bus[MAX_BUS_CLK];
@@ -41,15 +43,46 @@ static inline struct sdhci_s3c *to_s3c(s
 	return sdhci_priv(host);
 }
 
+static u32 get_curclk(u32 ctrl2)
+{
+	ctrl2 &= S3C_SDHCI_CTRL2_SELBASECLK_MASK;
+	ctrl2 >>= S3C_SDHCI_CTRL2_SELBASECLK_SHIFT;
+
+	return ctrl2;
+}
+
+/**
+ * sdhci_s3c_check_sclk() - check the clock selection against what we set
+ * @host: The host to check
+ *
+ * Certain controller resets clear the extra configuration register(s) and
+ * thus we need to check that the controller still has the clock setting
+ * we set for it.
+ */
+static void sdhci_s3c_check_sclk(struct sdhci_host *host)
+{
+	struct sdhci_s3c *ourhost = to_s3c(host);
+	u32 tmp = readl(host->ioaddr + S3C_SDHCI_CONTROL2);
+
+	if (get_curclk(tmp) != ourhost->cur_clk) {
+		dev_dbg(&ourhost->pdev->dev, "restored ctrl2 clock setting\n");
+
+		tmp &= ~S3C_SDHCI_CTRL2_SELBASECLK_MASK;
+		tmp |= ourhost->cur_clk << S3C_SDHCI_CTRL2_SELBASECLK_SHIFT;
+		writel(tmp, host->ioaddr + 0x80);
+	}
+}
+
 static unsigned int sdhci_s3c_get_max_clk(struct sdhci_host *host)
 {
 	struct sdhci_s3c *ourhost = to_s3c(host);
-	unsigned int rate;
 	u32 control2;
 	int clk;
 
 	/* note, a reset will reset the clock source */
 
+	sdhci_s3c_check_sclk(host);
+
 	control2 = readl(host->ioaddr + 0x80);
 	clk = clk_get_rate(ourhost->clk_bus[(control2 >> 4) & 3]);
 
@@ -67,13 +100,82 @@ static void sdhci_s3c_set_ios(struct sdh
 	struct sdhci_s3c *ourhost = to_s3c(host);
 	struct s3c_sdhci_platdata *pdata = ourhost->pdata;
 
+	sdhci_s3c_check_sclk(host);
+
 	if (pdata->cfg_card)
 		pdata->cfg_card(ourhost->pdev, host->ioaddr, ios);
 }
 
+static unsigned int sdhci_s3c_consider_clock(struct sdhci_s3c *ourhost,
+					     unsigned int src,
+					     unsigned int wanted)
+{
+	unsigned long rate;
+	struct clk *clksrc = ourhost->clk_bus[src];
+	int div;
+
+	if (!clksrc)
+		return UINT_MAX;
+
+	rate = clk_get_rate(clksrc);
+
+	for (div = 1; div < 256; div *= 2) {
+		if ((rate / div) <= wanted)
+			break;
+	}
+
+	dev_dbg(&ourhost->pdev->dev, "clk %d: rate %ld, want %d, got %ld\n",
+		src, rate, wanted, rate / div);
+
+	return (wanted - (rate / div));
+}
+
+static void sdhci_s3c_change_clock(struct sdhci_host *host, unsigned int clock)
+{
+	struct sdhci_s3c *ourhost = to_s3c(host);
+	unsigned int best = UINT_MAX;
+	unsigned int delta;
+	int best_src = 0;
+	int src;
+	u32 ctrl;
+
+	for (src = 0; src < MAX_BUS_CLK; src++) {
+		delta = sdhci_s3c_consider_clock(ourhost, src, clock);
+		if (delta < best) {
+			best = delta;
+			best_src = src;
+		}
+	}
+
+	dev_dbg(&ourhost->pdev->dev,
+		"selected source %d, clock %d, delta %d\n",
+		 best_src, clock, best);
+
+	/* turn clock off to card before changing clock source */
+	writew(0, host->ioaddr + SDHCI_CLOCK_CONTROL);
+
+	/* select the new clock source */
+
+	if (ourhost->cur_clk != best_src) {
+		struct clk *clk = ourhost->clk_bus[best_src];
+
+		ourhost->cur_clk = best_src;
+		host->max_clk = clk_get_rate(clk);
+		host->timeout_clk = host->max_clk / 1000000;
+
+		ctrl = readl(host->ioaddr + S3C_SDHCI_CONTROL2);
+		ctrl &= ~S3C_SDHCI_CTRL2_SELBASECLK_MASK;
+		ctrl |= best_src << S3C_SDHCI_CTRL2_SELBASECLK_SHIFT;
+		writel(ctrl, host->ioaddr + S3C_SDHCI_CONTROL2);
+	}
+
+	sdhci_change_clock(host, clock);
+}
+
 static struct sdhci_ops sdhci_s3c_ops = {
 	.get_max_clock		= sdhci_s3c_get_max_clk,
 	.get_timeout_clock	= sdhci_s3c_get_timeout_clk,
+	.change_clock		= sdhci_s3c_change_clock,
 	.set_ios		= sdhci_s3c_set_ios,
 };
 
Index: linux.git/drivers/mmc/host/sdhci.c
===================================================================
--- linux.git.orig/drivers/mmc/host/sdhci.c	2008-12-02 14:36:52.000000000 +0000
+++ linux.git/drivers/mmc/host/sdhci.c	2008-12-02 14:36:52.000000000 +0000
@@ -907,13 +907,18 @@ static void sdhci_finish_command(struct 
 
 static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 {
+	if (clock == host->clock)
+		return;
+
+	host->ops->change_clock(host, clock);
+}
+
+void sdhci_change_clock(struct sdhci_host *host, unsigned int clock)
+{
 	int div;
 	u16 clk;
 	unsigned long timeout;
 
-	if (clock == host->clock)
-		return;
-
 	writew(0, host->ioaddr + SDHCI_CLOCK_CONTROL);
 
 	if (clock == 0)
@@ -950,6 +955,8 @@ out:
 	host->clock = clock;
 }
 
+EXPORT_SYMBOL_GPL(sdhci_set_clock);
+
 static void sdhci_set_power(struct sdhci_host *host, unsigned short power)
 {
 	u8 pwr;
Index: linux.git/drivers/mmc/host/sdhci.h
===================================================================
--- linux.git.orig/drivers/mmc/host/sdhci.h	2008-12-02 14:36:52.000000000 +0000
+++ linux.git/drivers/mmc/host/sdhci.h	2008-12-02 14:36:52.000000000 +0000
@@ -272,6 +272,9 @@ struct sdhci_ops {
 	unsigned int	(*get_max_clock)(struct sdhci_host *host);
 	unsigned int	(*get_timeout_clock)(struct sdhci_host *host);
 
+	void		(*change_clock)(struct sdhci_host *host,
+					unsigned int clock);
+
 	void		(*set_ios)(struct sdhci_host *host,
 				   struct mmc_ios *ios);
 };
@@ -281,6 +284,8 @@ extern struct sdhci_host *sdhci_alloc_ho
 	size_t priv_size);
 extern void sdhci_free_host(struct sdhci_host *host);
 
+extern void sdhci_change_clock(struct sdhci_host *host, unsigned int clock);
+
 static inline void *sdhci_priv(struct sdhci_host *host)
 {
 	return (void *)host->private;

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [patch 7/8] SDHCI: Add change_clock callback for glue drivers

[Pierre Ossman]

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

On Tue, 02 Dec 2008 15:40:25 +0000
Ben Dooks <ben-linux@fluff.org> wrote:

> Add a change_clock callback to allow drivers to update
> device specific clock selections and control registers
> when there is a change in clock.
> 
> Move the main part of sdhci_set_clock() to a new routine
> which can be called by the glue drivers to do the sdhci
> standard clock management.
> 
> Update the sdhci-s3c driver to use this to select the
> appropriate clock source when clocks change.
> 
> Signed-off-by: Ben Dooks <ben-linux@fluff.org>
> 

I'm still waiting for a proper description of things here.

Looking at the code, it looks like you have multiple clock sources and
selecting the best one dynamically. That's probably a nice feature, but
it should be possible to solve whilst still keeping calls going only
from sdhci to sdhci-s3c.

E.g. we could add a caps field (to keep quirks just for bugs) which
states "has multiple clock sources". Before the core changes the clock,
it calls ops->select_clock(target_hz), which changes clock source and
returns the new base frequency.

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 8/8] SDHCI: Add maintainers entry for Samsung SoC

[Ben Dooks]

[-- Attachment #1: simtec/s3c64xx/sdhci-maintainers.patch --]
[-- Type: text/plain, Size: 817 bytes --]

Add a maintainers entry for myself as the mainatiner
of the Samsung SoC SDHCI glue layer

Signed-off-by: Ben Dooks <ben-linux@fluff.org>

Index: linux.git/MAINTAINERS
===================================================================
--- linux.git.orig/MAINTAINERS	2008-12-01 23:39:25.000000000 +0000
+++ linux.git/MAINTAINERS	2008-12-02 14:13:39.000000000 +0000
@@ -3720,6 +3720,13 @@ L:	sdhci-devel@list.drzeus.cx
 L:	linux-arm-kernel@lists.arm.linux.org.uk (subscribers-only)
 S:	Maintained
 
+SDHCI SAMSUNG SoC DRIVER
+P:	Ben Dooks
+M:	ben-linux@fluff.org
+L:	sdhci-devel@list.drzeus.cx
+L:	linux-arm-kernel@lists.arm.linux.org.uk (subscribers-only)
+S:	Maintained
+
 SECURITY CONTACT
 P:	Security Officers
 M:	security@kernel.org

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [patch 8/8] SDHCI: Add maintainers entry for Samsung SoC

[Pierre Ossman]

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

On Tue, 02 Dec 2008 15:40:26 +0000
Ben Dooks <ben-linux@fluff.org> wrote:

> Add a maintainers entry for myself as the mainatiner
> of the Samsung SoC SDHCI glue layer
> 
> Signed-off-by: Ben Dooks <ben-linux@fluff.org>
> 

It's probably better if you give it a name so that is next to the main
SDHCI entry ("SECURE DIGITAL HOST CONTROLLER INTERFACE (SDHCI) DRIVER").

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