[patch 0/7] SDHCI support for Samsung SoC

[patch 0/7] SDHCI support for Samsung SoC

[Ben Dooks]

The following patch set gets working SDHCI on the
Samsung SoC series.

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

  'a smiley only costs 4 bytes'

[patch 1/7] SDHCI: Add timeout hooks

[Ben Dooks]

[-- Attachment #1: simtec/s3c64xx/sdhci-info-hooks1.patch --]
[-- Type: text/plain, Size: 2124 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-10-21 10:49:19.000000000 +0100
+++ linux.git/drivers/mmc/host/sdhci.c	2008-10-21 11:01:38.000000000 +0100
@@ -1604,17 +1604,23 @@ int sdhci_add_host(struct sdhci_host *ho
 		mmc_dev(host->mmc)->dma_mask = &host->dma_mask;
 	}
 
-	host->max_clk =
-		(caps & SDHCI_CLOCK_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT;
+	if (host->ops->get_max_clock)
+		host->max_clk = host->ops->get_max_clock(host);
+	else {
+		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));
 		return -ENODEV;
 	}
-	host->max_clk *= 1000000;
 
-	host->timeout_clk =
-		(caps & SDHCI_TIMEOUT_CLK_MASK) >> SDHCI_TIMEOUT_CLK_SHIFT;
+	if (host->ops->get_timeout_clock)
+		host->timeout_clk = host->ops->get_timeout_clock(host);
+	else
+		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));
Index: linux.git/drivers/mmc/host/sdhci.h
===================================================================
--- linux.git.orig/drivers/mmc/host/sdhci.h	2008-10-21 10:49:19.000000000 +0100
+++ linux.git/drivers/mmc/host/sdhci.h	2008-10-21 11:01:38.000000000 +0100
@@ -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'

[patch 2/7] 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/7] SDHCI: Add set_ios hook

[Ben Dooks]

[-- Attachment #1: simtec/s3c64xx/sdhci-add-ios-hook.patch --]
[-- Type: text/plain, Size: 1078 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>

--- linux.git.orig/drivers/mmc/host/sdhci.c	2008-10-28 08:35:50.000000000 +0000
+++ linux.git/drivers/mmc/host/sdhci.c	2008-10-28 08:35:51.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)
--- linux.git.orig/drivers/mmc/host/sdhci.h	2008-10-28 08:31:30.000000000 +0000
+++ linux.git/drivers/mmc/host/sdhci.h	2008-10-28 08:35:51.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'

[patch 4/7] 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: 2378 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-11-03 10:01:03.000000000 +0000
+++ linux.git/drivers/mmc/host/sdhci.c	2008-11-03 12:20:40.000000000 +0000
@@ -1291,11 +1291,24 @@ static void sdhci_cmd_irq(struct sdhci_h
 	 *       controllers.
 	 */
 	if (host->cmd->flags & MMC_RSP_BUSY) {
+		u32 present;
+
 		if (host->cmd->data)
 			DBG("Cannot wait for busy signal when also "
 				"doing a data transfer");
-		else
+		else if (!(host->quirks & SDHCI_QUIRK_NO_TCIRQ_ON_NOT_BUSY))
 			return;
+
+		/* The Samsung SDHCI does not seem to provide an INT_DATA_END
+		 * when the system goes non-busy, so check the state of the
+		 * transfer by reading SDHCI_PRESENT_STATE to see if the
+		 * controller is ready
+		 */
+
+		present = readl(host->ioaddr + SDHCI_PRESENT_STATE);
+		DBG("busy? present %08x, intstat %08x\n", present, intmask);
+
+		/* 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-11-03 10:01:03.000000000 +0000
+++ linux.git/drivers/mmc/host/sdhci.h	2008-11-03 12:20:00.000000000 +0000
@@ -57,6 +57,7 @@
 #define  SDHCI_DATA_AVAILABLE	0x00000800
 #define  SDHCI_CARD_PRESENT	0x00010000
 #define  SDHCI_WRITE_PROTECT	0x00080000
+#define  SDHCI_DATA_BIT(x)	(1 << ((x) + 20))
 
 #define SDHCI_HOST_CONTROL 	0x28
 #define  SDHCI_CTRL_LED		0x01
@@ -210,6 +211,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_TCIRQ_ON_NOT_BUSY		(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/7] SDHCI: Samsung SDHCI (HSMMC) driver

[Ben Dooks]

[-- Attachment #1: simtec/s3c64xx/sdhci-samsung.patch --]
[-- Type: text/plain, Size: 8717 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-11-03 12:20:00.000000000 +0000
+++ linux.git/drivers/mmc/host/Kconfig	2008-11-03 12:20:54.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-11-03 12:20:00.000000000 +0000
+++ linux.git/drivers/mmc/host/Makefile	2008-11-03 12:20:54.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-11-03 19:41:31.000000000 +0000
@@ -0,0 +1,294 @@
+/* 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 void sdhci_s3c_sel_sclk(struct sdhci_host *host)
+{
+	struct sdhci_s3c *ourhost = to_s3c(host);
+
+	/* select sclk */
+	u32 tmp = readl(host->ioaddr + 0x80);
+
+	if ((tmp & (3 << 4)) == (2 << 4))
+		return;
+
+	tmp &= ~(3<<4);
+	tmp |= (2 << 4);
+	writel(tmp, host->ioaddr + 0x80);
+}
+
+
+static unsigned int sdhci_s3c_get_max_clk(struct sdhci_host *host)
+{
+	struct sdhci_s3c *ourhost = to_s3c(host);
+	u32 control2;
+	unsigned int rate;
+	int clk;
+
+	/* note, a reset will reset the clock source */
+
+	sdhci_s3c_sel_sclk(host);
+
+	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;
+	int width;
+
+	sdhci_s3c_sel_sclk(host);
+
+	if (ios->power_mode != MMC_POWER_OFF) {
+		switch (ios->bus_width) {
+		case MMC_BUS_WIDTH_4:
+			width = 4;
+			break;
+		case MMC_BUS_WIDTH_1:
+			width = 1;
+			break;
+		default:
+			BUG();
+		}
+
+		if (pdata->cfg_gpio)
+			pdata->cfg_gpio(ourhost->pdev, width);
+	}
+
+	if (pdata->cfg_card)
+		pdata->cfg_card(ourhost->pdev, host->ioaddr,
+				ios, host->mmc->card);
+}
+
+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++) {
+		struct clk *clk;
+		char *name = pdata->clocks[ptr];
+
+		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;
+	}
+
+	/* Ensure we have minimal gpio selected CMD/CLK/Detect */
+	if (pdata->cfg_gpio)
+		pdata->cfg_gpio(pdev, 0);
+
+	sdhci_s3c_sel_sclk(host);
+
+	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, do 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_TCIRQ_ON_NOT_BUSY;
+
+	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\n");
+		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++) {
+		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'

[patch 6/7] 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'

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

[Ben Dooks]

[-- Attachment #1: simtec/s3c64xx/sdhci-select-clock.patch --]
[-- Type: text/plain, Size: 7599 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-11-03 12:17:50.000000000 +0000
+++ linux.git/drivers/mmc/host/sdhci-pci.c	2008-11-03 12:18:41.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-11-03 12:18:27.000000000 +0000
+++ linux.git/drivers/mmc/host/sdhci-s3c.c	2008-11-03 12:18:41.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,38 +43,50 @@ 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;
+}
 
-static void sdhci_s3c_sel_sclk(struct sdhci_host *host)
+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);
 
-	/* select sclk */
-	u32 tmp = readl(host->ioaddr + 0x80);
+	if (get_curclk(tmp) != ourhost->cur_clk) {
+		dev_dbg(&ourhost->pdev->dev, "restored ctrl2 clock setting\n");
 
-	if ((tmp & (3 << 4)) == (2 << 4))
-		return;
-
-	tmp &= ~(3<<4);
-	tmp |= (2 << 4);
-	writel(tmp, host->ioaddr + 0x80);
+		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);
-	u32 control2;
-	unsigned int rate;
+	struct clk *busclk;
+	unsigned int rate, max;
 	int clk;
 
 	/* note, a reset will reset the clock source */
 
-	sdhci_s3c_sel_sclk(host);
+	sdhci_s3c_check_sclk(host);
+
+	for (max = 0, clk = 0; clk < MAX_BUS_CLK; clk++) {
+		busclk = ourhost->clk_bus[clk];
+		if (!busclk)
+			continue;
 
-	control2 = readl(host->ioaddr + 0x80);
-	clk = clk_get_rate(ourhost->clk_bus[(control2 >> 4) & 3]);
+		rate = clk_get_rate(busclk);
+		if (rate > max)
+			max = rate;
+	}
 
-	return clk;
+	return max;
 }
 
 static unsigned int sdhci_s3c_get_timeout_clk(struct sdhci_host *host)
@@ -87,7 +101,7 @@ static void sdhci_s3c_set_ios(struct sdh
 	struct s3c_sdhci_platdata *pdata = ourhost->pdata;
 	int width;
 
-	sdhci_s3c_sel_sclk(host);
+	sdhci_s3c_check_sclk(host);
 
 	if (ios->power_mode != MMC_POWER_OFF) {
 		switch (ios->bus_width) {
@@ -110,9 +124,76 @@ static void sdhci_s3c_set_ios(struct sdh
 				ios, host->mmc->card);
 }
 
+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,
 };
 
@@ -210,7 +291,7 @@ static int __devinit sdhci_s3c_probe(str
 	if (pdata->cfg_gpio)
 		pdata->cfg_gpio(pdev, 0);
 
-	sdhci_s3c_sel_sclk(host);
+	sdhci_s3c_check_sclk(host);
 
 	host->hw_name = "samsung-hsmmc";
 	host->ops = &sdhci_s3c_ops;
Index: linux.git/drivers/mmc/host/sdhci.c
===================================================================
--- linux.git.orig/drivers/mmc/host/sdhci.c	2008-11-03 12:18:39.000000000 +0000
+++ linux.git/drivers/mmc/host/sdhci.c	2008-11-03 12:18:41.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-11-03 12:17:50.000000000 +0000
+++ linux.git/drivers/mmc/host/sdhci.h	2008-11-03 12:18:41.000000000 +0000
@@ -273,6 +273,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);
 };
@@ -282,6 +285,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 6/7] SDHCI: Check DMA for overruns at end of transfer

[Henrique de Moraes Holschuh]

Maybe I didn't understand it right, but if the DMA controller could overrun
a buffer, don't you ALSO need to add defensive padding (i.e. increase the
buffer) to make sure nothing important gets overrun?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

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

[Ben Dooks]

On Mon, Nov 03, 2008 at 07:12:00PM -0200, Henrique de Moraes Holschuh wrote:
> Maybe I didn't understand it right, but if the DMA controller could overrun
> a buffer, don't you ALSO need to add defensive padding (i.e. increase the
> buffer) to make sure nothing important gets overrun?

This is only generated by problems elsewhere in the driver, such as
getting the timeout clock wrong. It is here just as a precaution and
as an aide to debugging, it should not trigger in normal circumstances.

There is a seperate problem where the DMA buffer is passed from the stack
which is, IIRC, a complete no-no under Linux. 

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

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

[Henrique de Moraes Holschuh]

On Mon, 03 Nov 2008, Ben Dooks wrote:
> On Mon, Nov 03, 2008 at 07:12:00PM -0200, Henrique de Moraes Holschuh wrote:
> > Maybe I didn't understand it right, but if the DMA controller could overrun
> > a buffer, don't you ALSO need to add defensive padding (i.e. increase the
> > buffer) to make sure nothing important gets overrun?
> 
> This is only generated by problems elsewhere in the driver, such as
> getting the timeout clock wrong. It is here just as a precaution and
> as an aide to debugging, it should not trigger in normal circumstances.

Then why is it just a WARN_ON, since you had a rogue DMA operation
overwriting unknown kernel memory?  Seems like an outright BUG_ON to me.

> There is a seperate problem where the DMA buffer is passed from the stack
> which is, IIRC, a complete no-no under Linux. 

Can't say much on that.  I just found it strange that something as damaging
as an overrun was only getting a WARN_ON and no defensive measure.  If it is
not going to happen normally, it might not require a defensive buffer, but
once it happens, it looks like one must reboot ASAP from what you said...

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

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

[Ben Dooks]

On Mon, Nov 03, 2008 at 11:28:40PM -0200, Henrique de Moraes Holschuh wrote:
> On Mon, 03 Nov 2008, Ben Dooks wrote:
> > On Mon, Nov 03, 2008 at 07:12:00PM -0200, Henrique de Moraes Holschuh wrote:
> > > Maybe I didn't understand it right, but if the DMA controller could overrun
> > > a buffer, don't you ALSO need to add defensive padding (i.e. increase the
> > > buffer) to make sure nothing important gets overrun?
> > 
> > This is only generated by problems elsewhere in the driver, such as
> > getting the timeout clock wrong. It is here just as a precaution and
> > as an aide to debugging, it should not trigger in normal circumstances.
> 
> Then why is it just a WARN_ON, since you had a rogue DMA operation
> overwriting unknown kernel memory?  Seems like an outright BUG_ON to me.

It is a problem, but it doesn't kill the entire system. We could print it
at a higher level. The WARN_ON()/BUG_ON() where not appropriate, as we do
not need a whole stack backtrace, and I belive the mmc block thread somehow
seems to manage to keep running even with an OOPS.

> > There is a seperate problem where the DMA buffer is passed from the stack
> > which is, IIRC, a complete no-no under Linux. 
> 
> Can't say much on that.  I just found it strange that something as damaging
> as an overrun was only getting a WARN_ON and no defensive measure.  If it is
> not going to happen normally, it might not require a defensive buffer, but
> once it happens, it looks like one must reboot ASAP from what you said...

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

  'a smiley only costs 4 bytes'

Re: [patch 0/7] SDHCI support for Samsung SoC

[Ben Dooks]

On Mon, Nov 03, 2008 at 08:09:44PM +0000, Ben Dooks wrote:
> The following patch set gets working SDHCI on the
> Samsung SoC series.

Any more comments on this, can these be merged for the -next branch?

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

  'a smiley only costs 4 bytes'

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

[Pierre Ossman]

On Mon, 03 Nov 2008 20:09:45 +0000
Ben Dooks <ben-linux@fluff.org> 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>
> 

Could we at least initally try to strictly adhere to the SDHCI spec and
only use the callback if the controller reports 0 for the freqs?

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

Re: [patch 2/7] SDHCI: Print ADMA status and pointer on debug

[Pierre Ossman]

On Mon, 03 Nov 2008 20:09:46 +0000
Ben Dooks <ben-linux@fluff.org> wrote:

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

Looks ok. I think that dump routine could use some more TLC to be
really useful.

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

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

[Pierre Ossman]

On Mon, 03 Nov 2008 20:09:47 +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>
> 

Hmm... this is very general and very much undefined. I think we need to
have some kind of formal API here or I might end up breaking S3C
support when I hack away at the driver.

Preferable, I'd like to see a more fine grained set of callbacks.

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

Re: [patch 4/7] SDHCI: Add quirk for controller with no end-of-busy IRQ

[Pierre Ossman]

On Mon, 03 Nov 2008 20:09:48 +0000
Ben Dooks <ben-linux@fluff.org> wrote:

> 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-11-03 10:01:03.000000000 +0000
> +++ linux.git/drivers/mmc/host/sdhci.c	2008-11-03 12:20:40.000000000 +0000
> @@ -1291,11 +1291,24 @@ static void sdhci_cmd_irq(struct sdhci_h
>  	 *       controllers.
>  	 */
>  	if (host->cmd->flags & MMC_RSP_BUSY) {
> +		u32 present;
> +
>  		if (host->cmd->data)
>  			DBG("Cannot wait for busy signal when also "
>  				"doing a data transfer");
> -		else
> +		else if (!(host->quirks & SDHCI_QUIRK_NO_TCIRQ_ON_NOT_BUSY))
>  			return;

Not the clearest naming I've ever seen. :)
How about NO_BUSY_IRQ?

> +
> +		/* The Samsung SDHCI does not seem to provide an INT_DATA_END
> +		 * when the system goes non-busy, so check the state of the
> +		 * transfer by reading SDHCI_PRESENT_STATE to see if the
> +		 * controller is ready
> +		 */

There is already a note about this being a problem earlier up, so I
don't think we need another.

> +
> +		present = readl(host->ioaddr + SDHCI_PRESENT_STATE);
> +		DBG("busy? present %08x, intstat %08x\n", present, intmask);
> +

And what does this add really? Controllers not being able to wait for
the busy state to end is so common that we can just ignore the problem
here.

> +#define  SDHCI_DATA_BIT(x)	(1 << ((x) + 20))

This was not supposed to be included I suppose :)

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

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

[Pierre Ossman]

On Mon, 03 Nov 2008 20:09:49 +0000
Ben Dooks <ben-linux@fluff.org> wrote:

> +static void sdhci_s3c_sel_sclk(struct sdhci_host *host)
> +{
> +	struct sdhci_s3c *ourhost = to_s3c(host);
> +
> +	/* select sclk */
> +	u32 tmp = readl(host->ioaddr + 0x80);
> +
> +	if ((tmp & (3 << 4)) == (2 << 4))
> +		return;
> +
> +	tmp &= ~(3<<4);
> +	tmp |= (2 << 4);
> +	writel(tmp, host->ioaddr + 0x80);
> +}

No defines for this? This is not terribly readable.

> +	if (pdata->cfg_card)
> +		pdata->cfg_card(ourhost->pdev, host->ioaddr,
> +				ios, host->mmc->card);

What's the deal here? Hosts shouldn't know more about the card than the
MMC core tells them.

Since I have no hardware for this, could you take it upon you to handle
support for these chips? I'd like a MAINTAINERS patch for that as well.

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

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

[Pierre Ossman]

On Mon, 03 Nov 2008 20:09:50 +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.
> 
> Signed-off-by: Ben Dooks <ben-linux@fluff.org>

I'm sorry, but I don't see how this is anywhere near acceptable. This
should be a panic at the very least, and until this can be sorted out
and avoided the driver should avoid using DMA on these chips.

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

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

[Pierre Ossman]

On Mon, 03 Nov 2008 20:09:51 +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 afraid I don't quite follow on the requirements here. Care to
elaborate as to why this is needed?

> Index: linux.git/drivers/mmc/host/sdhci-pci.c
> ===================================================================
> --- linux.git.orig/drivers/mmc/host/sdhci-pci.c	2008-11-03 12:17:50.000000000 +0000
> +++ linux.git/drivers/mmc/host/sdhci-pci.c	2008-11-03 12:18:41.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,
>  };
>  
>  /*****************************************************************************\

This seems pointless :)

> @@ -950,6 +955,8 @@ out:
>  	host->clock = clock;
>  }
>  
> +EXPORT_SYMBOL_GPL(sdhci_set_clock);
> +

Wrong symbol. :)

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

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

[Ben Dooks]

On Fri, Nov 14, 2008 at 11:20:21PM +0100, Pierre Ossman wrote:
> On Mon, 03 Nov 2008 20:09:51 +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 afraid I don't quite follow on the requirements here. Care to
> elaborate as to why this is needed?

The controller has a number of possible clock sources, the bus clock
it is connected to (generally called hclk) and three more clocks from
the heirachy that is muxed/divided from either an 48MHz, 27MHz or the
EPLL.

The callback allows the recalculation and selection of the best clock
to run the card clock for the controller.
 
> > Index: linux.git/drivers/mmc/host/sdhci-pci.c
> > ===================================================================
> > --- linux.git.orig/drivers/mmc/host/sdhci-pci.c	2008-11-03 12:17:50.000000000 +0000
> > +++ linux.git/drivers/mmc/host/sdhci-pci.c	2008-11-03 12:18:41.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,
> >  };
> >  
> >  /*****************************************************************************\
> 
> This seems pointless :)
> 
> > @@ -950,6 +955,8 @@ out:
> >  	host->clock = clock;
> >  }
> >  
> > +EXPORT_SYMBOL_GPL(sdhci_set_clock);
> > +
> 
> Wrong symbol. :)

ok, will fix

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

  'a smiley only costs 4 bytes'

Re: [patch 4/7] SDHCI: Add quirk for controller with no end-of-busy IRQ

[Ben Dooks]

On Fri, Nov 14, 2008 at 10:41:14PM +0100, Pierre Ossman wrote:
> On Mon, 03 Nov 2008 20:09:48 +0000
> Ben Dooks <ben-linux@fluff.org> wrote:
> 
> > 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-11-03 10:01:03.000000000 +0000
> > +++ linux.git/drivers/mmc/host/sdhci.c	2008-11-03 12:20:40.000000000 +0000
> > @@ -1291,11 +1291,24 @@ static void sdhci_cmd_irq(struct sdhci_h
> >  	 *       controllers.
> >  	 */
> >  	if (host->cmd->flags & MMC_RSP_BUSY) {
> > +		u32 present;
> > +
> >  		if (host->cmd->data)
> >  			DBG("Cannot wait for busy signal when also "
> >  				"doing a data transfer");
> > -		else
> > +		else if (!(host->quirks & SDHCI_QUIRK_NO_TCIRQ_ON_NOT_BUSY))
> >  			return;
> 
> Not the clearest naming I've ever seen. :)
> How about NO_BUSY_IRQ?

ok, will change.
 
> > +
> > +		/* The Samsung SDHCI does not seem to provide an INT_DATA_END
> > +		 * when the system goes non-busy, so check the state of the
> > +		 * transfer by reading SDHCI_PRESENT_STATE to see if the
> > +		 * controller is ready
> > +		 */
> 
> There is already a note about this being a problem earlier up, so I
> don't think we need another.

right.
 
> > +
> > +		present = readl(host->ioaddr + SDHCI_PRESENT_STATE);
> > +		DBG("busy? present %08x, intstat %08x\n", present, intmask);
> > +
> 
> And what does this add really? Controllers not being able to wait for
> the busy state to end is so common that we can just ignore the problem
> here.
> 

that was debug.

> > +#define  SDHCI_DATA_BIT(x)	(1 << ((x) + 20))
> 
> This was not supposed to be included I suppose :)

was with teh debug.

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

  'a smiley only costs 4 bytes'

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

[Ben Dooks]

On Fri, Nov 14, 2008 at 10:48:13PM +0100, Pierre Ossman wrote:
> On Mon, 03 Nov 2008 20:09:49 +0000
> Ben Dooks <ben-linux@fluff.org> wrote:
> 
> > +static void sdhci_s3c_sel_sclk(struct sdhci_host *host)
> > +{
> > +	struct sdhci_s3c *ourhost = to_s3c(host);
> > +
> > +	/* select sclk */
> > +	u32 tmp = readl(host->ioaddr + 0x80);
> > +
> > +	if ((tmp & (3 << 4)) == (2 << 4))
> > +		return;
> > +
> > +	tmp &= ~(3<<4);
> > +	tmp |= (2 << 4);
> > +	writel(tmp, host->ioaddr + 0x80);
> > +}
> 
> No defines for this? This is not terribly readable.

it was only temporary to ensure that if people bisected before
the clock callback was added the host would at least work even
if it was locked to one clock input.
 
> > +	if (pdata->cfg_card)
> > +		pdata->cfg_card(ourhost->pdev, host->ioaddr,
> > +				ios, host->mmc->card);
> 
> What's the deal here? Hosts shouldn't know more about the card than the
> MMC core tells them.

Depending on exactly which version, there's a number of configurations
in the extra configuration registers that get changed, such as delays
used for clock feedback which are meant to be chnaged when the speed
of the card is changed. Unfortunately this changes depending on the
chip model and possibly also on the board it is on.

The other is that it is possible that depending on the chip and board
combination things like the GPIO driver-strength controls will need to
be changed.
 
> Since I have no hardware for this, could you take it upon you to handle
> support for these chips? I'd like a MAINTAINERS patch for that as well.

ok, i'll add it to the list.

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

  'a smiley only costs 4 bytes'

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

[Ben Dooks]

On Fri, Nov 14, 2008 at 11:00:26PM +0100, Pierre Ossman wrote:
> On Mon, 03 Nov 2008 20:09:50 +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.
> > 
> > Signed-off-by: Ben Dooks <ben-linux@fluff.org>
> 
> I'm sorry, but I don't see how this is anywhere near acceptable. This
> should be a panic at the very least, and until this can be sorted out
> and avoided the driver should avoid using DMA on these chips.

A panic won't help get the debug logs out of the kernel. This only
turned up whilst debugging the controller, I got the timeout clock
calculation wrong and thus ended up timing out pretty much all the
CMD25s and seeing this problem.

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

  'a smiley only costs 4 bytes'

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

[Pierre Ossman]

On Sun, 16 Nov 2008 00:03:02 +0000
Ben Dooks <ben-linux@fluff.org> wrote:

> On Fri, Nov 14, 2008 at 10:48:13PM +0100, Pierre Ossman wrote:
> > On Mon, 03 Nov 2008 20:09:49 +0000
> > Ben Dooks <ben-linux@fluff.org> wrote:
> > > +	if (pdata->cfg_card)
> > > +		pdata->cfg_card(ourhost->pdev, host->ioaddr,
> > > +				ios, host->mmc->card);
> > 
> > What's the deal here? Hosts shouldn't know more about the card than the
> > MMC core tells them.
> 
> Depending on exactly which version, there's a number of configurations
> in the extra configuration registers that get changed, such as delays
> used for clock feedback which are meant to be chnaged when the speed
> of the card is changed. Unfortunately this changes depending on the
> chip model and possibly also on the board it is on.
> 
> The other is that it is possible that depending on the chip and board
> combination things like the GPIO driver-strength controls will need to
> be changed.

I don't see where knowledge about the card gets into the picture here.
All of this sounds like information that is gathered from platform data
and/or ios information.

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

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

[Pierre Ossman]

On Sun, 16 Nov 2008 00:05:08 +0000
Ben Dooks <ben-linux@fluff.org> wrote:

> On Fri, Nov 14, 2008 at 11:00:26PM +0100, Pierre Ossman wrote:
> > 
> > I'm sorry, but I don't see how this is anywhere near acceptable. This
> > should be a panic at the very least, and until this can be sorted out
> > and avoided the driver should avoid using DMA on these chips.
> 
> A panic won't help get the debug logs out of the kernel.

Indeed, but it will avoid corrupting the system.

> This only
> turned up whilst debugging the controller, I got the timeout clock
> calculation wrong and thus ended up timing out pretty much all the
> CMD25s and seeing this problem.
> 

Just because you had to provoke it doesn't mean it won't appear under
"normal" circumstances for others.

Until this problem is fully understood I think DMA should be turned off
(or possibly needs to be explicitly forced on using Kconfig or a module
parameter).

Do you have any contacts at Samsung that can help out here?

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

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

[Pierre Ossman]

On Sat, 15 Nov 2008 23:57:39 +0000
Ben Dooks <ben-linux@fluff.org> wrote:

> 
> The controller has a number of possible clock sources, the bus clock
> it is connected to (generally called hclk) and three more clocks from
> the heirachy that is muxed/divided from either an 48MHz, 27MHz or the
> EPLL.
> 
> The callback allows the recalculation and selection of the best clock
> to run the card clock for the controller.
>  

Can these clock sources change mid-flight? Otherwise I don't see the
need for a callback up to the sdhci core. But I don't see any
connection points for such events.

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