TMIO MMC: full patchset.

TMIO MMC: full patchset.

[Ian Molton]

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

Hi guys,

This is the full TMIO MMC patchset, compiled, tested, checkpatch passed.

Please base all tmio based code on top of this patchset.

Also, please can I get some review on the 'slow init' patch in this
set as it modifies the mmc core. It should be harmless for all
non-broken hardware.

-- 
Ian Molton
Linux, Automotive, and other hacking:
http://www.mnementh.co.uk/

[-- Attachment #2: 0001-MMC-hardware-abstraction-for-CNF-area.patch --]
[-- Type: text/x-patch, Size: 22040 bytes --]

From 46909530b67a1c83c1e22ac5fcfcada27d4fab54 Mon Sep 17 00:00:00 2001
From: Ian Molton <ian@mnementh.co.uk>
Date: Thu, 24 Sep 2009 23:24:18 +0100
Subject: [PATCH 1/5] MMC: hardware abstraction for CNF area

	This patch abstracts out the CNF area code from tmio_mmc which is
not present in all hardware that can use this driver. This is required so that
we can suppot non-toshiba based hardware.

Signed-off-by: Ian Molton <ian@mnementh.co.uk>
---
 drivers/mfd/Makefile        |    6 +-
 drivers/mfd/t7l66xb.c       |   31 +++++++++++--
 drivers/mfd/tc6387xb.c      |   97 ++++++++++++++++++++++++++++++++++--------
 drivers/mfd/tc6393xb.c      |   56 +++++++++++++++++++++----
 drivers/mfd/tmio_core.c     |   54 ++++++++++++++++++++++++
 drivers/mmc/host/tmio_mmc.c |   63 +++++++++------------------
 drivers/mmc/host/tmio_mmc.h |   46 +++------------------
 include/linux/mfd/tmio.h    |   33 +++++++++++++++
 8 files changed, 268 insertions(+), 118 deletions(-)
 create mode 100644 drivers/mfd/tmio_core.c

diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 6f8a9a1..9be344a 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -10,9 +10,9 @@ obj-$(CONFIG_HTC_PASIC3)	+= htc-pasic3.o
 
 obj-$(CONFIG_MFD_DM355EVM_MSP)	+= dm355evm_msp.o
 
-obj-$(CONFIG_MFD_T7L66XB)	+= t7l66xb.o
-obj-$(CONFIG_MFD_TC6387XB)	+= tc6387xb.o
-obj-$(CONFIG_MFD_TC6393XB)	+= tc6393xb.o
+obj-$(CONFIG_MFD_T7L66XB)	+= t7l66xb.o tmio_core.o
+obj-$(CONFIG_MFD_TC6387XB)	+= tc6387xb.o tmio_core.o
+obj-$(CONFIG_MFD_TC6393XB)	+= tc6393xb.o tmio_core.o
 
 obj-$(CONFIG_MFD_WM8400)	+= wm8400-core.o
 wm8350-objs			:= wm8350-core.o wm8350-regmap.o wm8350-gpio.o
diff --git a/drivers/mfd/t7l66xb.c b/drivers/mfd/t7l66xb.c
index 0a255c1..2fa0703 100644
--- a/drivers/mfd/t7l66xb.c
+++ b/drivers/mfd/t7l66xb.c
@@ -38,6 +38,8 @@ enum {
 	T7L66XB_CELL_MMC,
 };
 
+static const struct resource t7l66xb_mmc_resources[];
+
 #define SCR_REVID	0x08		/* b Revision ID	*/
 #define SCR_IMR		0x42		/* b Interrupt Mask	*/
 #define SCR_DEV_CTL	0xe0		/* b Device control	*/
@@ -83,6 +85,9 @@ static int t7l66xb_mmc_enable(struct platform_device *mmc)
 
 	spin_unlock_irqrestore(&t7l66xb->lock, flags);
 
+	tmio_core_mmc_enable(t7l66xb->scr + 0x200,
+		t7l66xb_mmc_resources[0].start & 0xfffe);
+
 	return 0;
 }
 
@@ -106,10 +111,28 @@ static int t7l66xb_mmc_disable(struct platform_device *mmc)
 	return 0;
 }
 
+static void t7l66xb_mmc_pwr(struct platform_device *mmc, int state)
+{
+	struct platform_device *dev = to_platform_device(mmc->dev.parent);
+	struct t7l66xb *t7l66xb = platform_get_drvdata(dev);
+
+	tmio_core_mmc_pwr(t7l66xb->scr + 0x200, state);
+}
+
+static void t7l66xb_mmc_clk_div(struct platform_device *mmc, int state)
+{
+	struct platform_device *dev = to_platform_device(mmc->dev.parent);
+	struct t7l66xb *t7l66xb = platform_get_drvdata(dev);
+
+	tmio_core_mmc_clk_div(t7l66xb->scr + 0x200, state);
+}
+
 /*--------------------------------------------------------------------------*/
 
 static struct tmio_mmc_data t7166xb_mmc_data = {
 	.hclk = 24000000,
+	.set_pwr = t7l66xb_mmc_pwr,
+	.set_no_clk_div = t7l66xb_mmc_clk_div,
 };
 
 static const struct resource t7l66xb_mmc_resources[] = {
@@ -119,11 +142,6 @@ static const struct resource t7l66xb_mmc_resources[] = {
 		.flags = IORESOURCE_MEM,
 	},
 	{
-		.start = 0x200,
-		.end	= 0x2ff,
-		.flags = IORESOURCE_MEM,
-	},
-	{
 		.start = IRQ_T7L66XB_MMC,
 		.end	= IRQ_T7L66XB_MMC,
 		.flags = IORESOURCE_IRQ,
@@ -282,6 +300,9 @@ static int t7l66xb_resume(struct platform_device *dev)
 	if (pdata && pdata->resume)
 		pdata->resume(dev);
 
+	tmio_core_mmc_enable(t7l66xb->scr + 0x200,
+		t7l66xb_mmc_resources[0].start & 0xfffe);
+
 	return 0;
 }
 #else
diff --git a/drivers/mfd/tc6387xb.c b/drivers/mfd/tc6387xb.c
index 3280ab3..fa8da02 100644
--- a/drivers/mfd/tc6387xb.c
+++ b/drivers/mfd/tc6387xb.c
@@ -22,28 +22,41 @@ enum {
 	TC6387XB_CELL_MMC,
 };
 
+struct tc6387xb {
+	void __iomem *scr;
+	struct clk *clk32k;
+	struct resource rscr;
+};
+
+static struct resource tc6387xb_mmc_resources[];
+
+/*--------------------------------------------------------------------------*/
+
 #ifdef CONFIG_PM
 static int tc6387xb_suspend(struct platform_device *dev, pm_message_t state)
 {
-	struct clk *clk32k = platform_get_drvdata(dev);
+	struct tc6387xb *tc6387xb = platform_get_drvdata(dev);
 	struct tc6387xb_platform_data *pdata = dev->dev.platform_data;
 
 	if (pdata && pdata->suspend)
 		pdata->suspend(dev);
-	clk_disable(clk32k);
+	clk_disable(tc6387xb->clk32k);
 
 	return 0;
 }
 
 static int tc6387xb_resume(struct platform_device *dev)
 {
-	struct clk *clk32k = platform_get_drvdata(dev);
+	struct tc6387xb *tc6387xb = platform_get_drvdata(dev);
 	struct tc6387xb_platform_data *pdata = dev->dev.platform_data;
 
-	clk_enable(clk32k);
+	clk_enable(tc6387xb->clk32k);
 	if (pdata && pdata->resume)
 		pdata->resume(dev);
 
+	tmio_core_mmc_resume(tc6387xb->scr + 0x200,
+		tc6387xb_mmc_resources[0].start & 0xfffe);
+
 	return 0;
 }
 #else
@@ -53,12 +66,32 @@ static int tc6387xb_resume(struct platform_device *dev)
 
 /*--------------------------------------------------------------------------*/
 
+static void tc6387xb_mmc_pwr(struct platform_device *mmc, int state)
+{
+	struct platform_device *dev = to_platform_device(mmc->dev.parent);
+	struct tc6387xb *tc6387xb = platform_get_drvdata(dev);
+
+	tmio_core_mmc_pwr(tc6387xb->scr + 0x200, state);
+}
+
+static void tc6387xb_mmc_clk_div(struct platform_device *mmc, int state)
+{
+	struct platform_device *dev = to_platform_device(mmc->dev.parent);
+	struct tc6387xb *tc6387xb = platform_get_drvdata(dev);
+
+	tmio_core_mmc_clk_div(tc6387xb->scr + 0x200, state);
+}
+
+
 static int tc6387xb_mmc_enable(struct platform_device *mmc)
 {
 	struct platform_device *dev      = to_platform_device(mmc->dev.parent);
-	struct clk *clk32k = platform_get_drvdata(dev);
+	struct tc6387xb *tc6387xb = platform_get_drvdata(dev);
 
-	clk_enable(clk32k);
+	clk_enable(tc6387xb->clk32k);
+
+	tmio_core_mmc_enable(tc6387xb->scr + 0x200,
+		tc6387xb_mmc_resources[0].start & 0xfffe);
 
 	return 0;
 }
@@ -66,19 +99,21 @@ static int tc6387xb_mmc_enable(struct platform_device *mmc)
 static int tc6387xb_mmc_disable(struct platform_device *mmc)
 {
 	struct platform_device *dev      = to_platform_device(mmc->dev.parent);
-	struct clk *clk32k = platform_get_drvdata(dev);
+	struct tc6387xb *tc6387xb = platform_get_drvdata(dev);
 
-	clk_disable(clk32k);
+	clk_disable(tc6387xb->clk32k);
 
 	return 0;
 }
 
-/*--------------------------------------------------------------------------*/
-
 static struct tmio_mmc_data tc6387xb_mmc_data = {
 	.hclk = 24000000,
+	.set_pwr = tc6387xb_mmc_pwr,
+	.set_no_clk_div = tc6387xb_mmc_clk_div,
 };
 
+/*--------------------------------------------------------------------------*/
+
 static struct resource tc6387xb_mmc_resources[] = {
 	{
 		.start = 0x800,
@@ -86,11 +121,6 @@ static struct resource tc6387xb_mmc_resources[] = {
 		.flags = IORESOURCE_MEM,
 	},
 	{
-		.start = 0x200,
-		.end   = 0x2ff,
-		.flags = IORESOURCE_MEM,
-	},
-	{
 		.start = 0,
 		.end   = 0,
 		.flags = IORESOURCE_IRQ,
@@ -111,8 +141,9 @@ static struct mfd_cell tc6387xb_cells[] = {
 static int tc6387xb_probe(struct platform_device *dev)
 {
 	struct tc6387xb_platform_data *pdata = dev->dev.platform_data;
-	struct resource *iomem;
+	struct resource *iomem, *rscr;
 	struct clk *clk32k;
+	struct tc6387xb *tc6387xb;
 	int irq, ret;
 
 	iomem = platform_get_resource(dev, IORESOURCE_MEM, 0);
@@ -120,18 +151,40 @@ static int tc6387xb_probe(struct platform_device *dev)
 		return -EINVAL;
 	}
 
+	tc6387xb = kzalloc(sizeof *tc6387xb, GFP_KERNEL);
+	if (!tc6387xb)
+		return -ENOMEM;
+
 	ret  = platform_get_irq(dev, 0);
 	if (ret >= 0)
 		irq = ret;
 	else
-		goto err_resource;
+		goto err_no_irq;
 
 	clk32k = clk_get(&dev->dev, "CLK_CK32K");
 	if (IS_ERR(clk32k)) {
 		ret = PTR_ERR(clk32k);
+		goto err_no_clk;
+	}
+
+	rscr = &tc6387xb->rscr;
+	rscr->name = "tc6387xb-core";
+	rscr->start = iomem->start;
+	rscr->end = iomem->start + 0xff;
+	rscr->flags = IORESOURCE_MEM;
+
+	ret = request_resource(iomem, rscr);
+	if (ret)
 		goto err_resource;
+
+	tc6387xb->scr = ioremap(rscr->start, rscr->end - rscr->start + 1);
+	if (!tc6387xb->scr) {
+		ret = -ENOMEM;
+		goto err_ioremap;
 	}
-	platform_set_drvdata(dev, clk32k);
+
+	tc6387xb->clk32k = clk32k;
+	platform_set_drvdata(dev, tc6387xb);
 
 	if (pdata && pdata->enable)
 		pdata->enable(dev);
@@ -149,8 +202,13 @@ static int tc6387xb_probe(struct platform_device *dev)
 	if (!ret)
 		return 0;
 
-	clk_put(clk32k);
+err_ioremap:
+	release_resource(&tc6387xb->rscr);
 err_resource:
+	clk_put(clk32k);
+err_no_clk:
+err_no_irq:
+	kfree(tc6387xb);
 	return ret;
 }
 
@@ -195,3 +253,4 @@ MODULE_DESCRIPTION("Toshiba TC6387XB core driver");
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Ian Molton");
 MODULE_ALIAS("platform:tc6387xb");
+
diff --git a/drivers/mfd/tc6393xb.c b/drivers/mfd/tc6393xb.c
index 1429a73..a44b6b0 100644
--- a/drivers/mfd/tc6393xb.c
+++ b/drivers/mfd/tc6393xb.c
@@ -136,10 +136,6 @@ static int tc6393xb_nand_enable(struct platform_device *nand)
 	return 0;
 }
 
-static struct tmio_mmc_data tc6393xb_mmc_data = {
-	.hclk = 24000000,
-};
-
 static struct resource __devinitdata tc6393xb_nand_resources[] = {
 	{
 		.start	= 0x1000,
@@ -165,11 +161,6 @@ static struct resource __devinitdata tc6393xb_mmc_resources[] = {
 		.flags	= IORESOURCE_MEM,
 	},
 	{
-		.start	= 0x200,
-		.end	= 0x2ff,
-		.flags	= IORESOURCE_MEM,
-	},
-	{
 		.start	= IRQ_TC6393_MMC,
 		.end	= IRQ_TC6393_MMC,
 		.flags	= IORESOURCE_IRQ,
@@ -346,6 +337,50 @@ int tc6393xb_lcd_mode(struct platform_device *fb,
 }
 EXPORT_SYMBOL(tc6393xb_lcd_mode);
 
+static int tc6393xb_mmc_enable(struct platform_device *mmc)
+{
+	struct platform_device *dev = to_platform_device(mmc->dev.parent);
+	struct tc6393xb *tc6393xb = platform_get_drvdata(dev);
+
+	tmio_core_mmc_enable(tc6393xb->scr + 0x200,
+		tc6393xb_mmc_resources[0].start & 0xfffe);
+
+	return 0;
+}
+
+static int tc6393xb_mmc_resume(struct platform_device *mmc)
+{
+	struct platform_device *dev = to_platform_device(mmc->dev.parent);
+	struct tc6393xb *tc6393xb = platform_get_drvdata(dev);
+
+	tmio_core_mmc_resume(tc6393xb->scr + 0x200,
+		tc6393xb_mmc_resources[0].start & 0xfffe);
+
+	return 0;
+}
+
+static void tc6393xb_mmc_pwr(struct platform_device *mmc, int state)
+{
+	struct platform_device *dev = to_platform_device(mmc->dev.parent);
+	struct tc6393xb *tc6393xb = platform_get_drvdata(dev);
+
+	tmio_core_mmc_pwr(tc6393xb->scr + 0x200, state);
+}
+
+static void tc6393xb_mmc_clk_div(struct platform_device *mmc, int state)
+{
+	struct platform_device *dev = to_platform_device(mmc->dev.parent);
+	struct tc6393xb *tc6393xb = platform_get_drvdata(dev);
+
+	tmio_core_mmc_clk_div(tc6393xb->scr + 0x200, state);
+}
+
+static struct tmio_mmc_data tc6393xb_mmc_data = {
+	.hclk = 24000000,
+	.set_pwr = tc6393xb_mmc_pwr,
+	.set_no_clk_div = tc6393xb_mmc_clk_div,
+};
+
 static struct mfd_cell __devinitdata tc6393xb_cells[] = {
 	[TC6393XB_CELL_NAND] = {
 		.name = "tmio-nand",
@@ -355,6 +390,8 @@ static struct mfd_cell __devinitdata tc6393xb_cells[] = {
 	},
 	[TC6393XB_CELL_MMC] = {
 		.name = "tmio-mmc",
+		.enable = tc6393xb_mmc_enable,
+		.resume = tc6393xb_mmc_resume,
 		.driver_data = &tc6393xb_mmc_data,
 		.num_resources = ARRAY_SIZE(tc6393xb_mmc_resources),
 		.resources = tc6393xb_mmc_resources,
@@ -836,3 +873,4 @@ MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Ian Molton, Dmitry Baryshkov and Dirk Opfer");
 MODULE_DESCRIPTION("tc6393xb Toshiba Mobile IO Controller");
 MODULE_ALIAS("platform:tc6393xb");
+
diff --git a/drivers/mfd/tmio_core.c b/drivers/mfd/tmio_core.c
new file mode 100644
index 0000000..483fbbb
--- /dev/null
+++ b/drivers/mfd/tmio_core.c
@@ -0,0 +1,54 @@
+/*
+ * Toshiba TC6393XB SoC support
+ *
+ * Copyright(c) 2009 Ian Molton <spyro@f2s.com>
+ *
+ * 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/mfd/tmio.h>
+
+int tmio_core_mmc_enable(void __iomem *cnf, unsigned long base)
+{
+	/* Enable the MMC/SD Control registers */
+	sd_config_write16(cnf + CNF_CMD, SDCREN);
+	sd_config_write32(cnf + CNF_CTL_BASE, base & 0xfffe);
+
+	/* Disable SD power during suspend */
+	sd_config_write8(cnf + CNF_PWR_CTL_3, 0x01);
+
+	/* The below is required but why? FIXME */
+	sd_config_write8(cnf + CNF_STOP_CLK_CTL, 0x1f);
+
+	/* Power down SD bus*/
+	sd_config_write8(cnf + CNF_PWR_CTL_2, 0x00);
+
+	return 0;
+}
+EXPORT_SYMBOL(tmio_core_mmc_enable);
+
+int tmio_core_mmc_resume(void __iomem *cnf, unsigned long base)
+{
+
+	/* Enable the MMC/SD Control registers */
+	sd_config_write16(cnf + CNF_CMD, SDCREN);
+	sd_config_write32(cnf + CNF_CTL_BASE, base & 0xfffe);
+
+	return 0;
+}
+EXPORT_SYMBOL(tmio_core_mmc_resume);
+
+void tmio_core_mmc_pwr(void __iomem *cnf, int state)
+{
+	sd_config_write8(cnf + CNF_PWR_CTL_2, state ? 0x02 : 0x00);
+}
+EXPORT_SYMBOL(tmio_core_mmc_pwr);
+
+void tmio_core_mmc_clk_div(void __iomem *cnf, int state)
+{
+	sd_config_write8(cnf + CNF_SD_CLK_MODE, state ? 1 : 0);
+}
+EXPORT_SYMBOL(tmio_core_mmc_clk_div);
+
diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c
index 91991b4..e701732 100644
--- a/drivers/mmc/host/tmio_mmc.c
+++ b/drivers/mmc/host/tmio_mmc.c
@@ -46,7 +46,9 @@ static void tmio_mmc_set_clock(struct tmio_mmc_host *host, int new_clock)
 		clk |= 0x100;
 	}
 
-	sd_config_write8(host, CNF_SD_CLK_MODE, clk >> 22);
+	if (host->set_no_clk_div)
+		host->set_no_clk_div(host->pdev, (clk>>22) & 1);
+
 	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, clk & 0x1ff);
 }
 
@@ -427,12 +429,13 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	/* Power sequence - OFF -> ON -> UP */
 	switch (ios->power_mode) {
 	case MMC_POWER_OFF: /* power down SD bus */
-		sd_config_write8(host, CNF_PWR_CTL_2, 0x00);
+		if (host->set_pwr)
+			host->set_pwr(host->pdev, 0);
 		tmio_mmc_clk_stop(host);
 		break;
 	case MMC_POWER_ON: /* power up SD bus */
-
-		sd_config_write8(host, CNF_PWR_CTL_2, 0x02);
+		if (host->set_pwr)
+			host->set_pwr(host->pdev, 1);
 		break;
 	case MMC_POWER_UP: /* start bus clock */
 		tmio_mmc_clk_start(host);
@@ -475,8 +478,8 @@ static int tmio_mmc_suspend(struct platform_device *dev, pm_message_t state)
 	ret = mmc_suspend_host(mmc, state);
 
 	/* Tell MFD core it can disable us now.*/
-	if (!ret && cell->disable)
-		cell->disable(dev);
+	if (!ret && cell->suspend)
+		cell->suspend(dev);
 
 	return ret;
 }
@@ -485,21 +488,15 @@ static int tmio_mmc_resume(struct platform_device *dev)
 {
 	struct mfd_cell	*cell = (struct mfd_cell *)dev->dev.platform_data;
 	struct mmc_host *mmc = platform_get_drvdata(dev);
-	struct tmio_mmc_host *host = mmc_priv(mmc);
 	int ret = 0;
 
 	/* Tell the MFD core we are ready to be enabled */
-	if (cell->enable) {
-		ret = cell->enable(dev);
+	if (cell->resume) {
+		ret = cell->resume(dev);
 		if (ret)
 			goto out;
 	}
 
-	/* Enable the MMC/SD Control registers */
-	sd_config_write16(host, CNF_CMD, SDCREN);
-	sd_config_write32(host, CNF_CTL_BASE,
-		(dev->resource[0].start >> host->bus_shift) & 0xfffe);
-
 	mmc_resume_host(mmc);
 
 out:
@@ -514,17 +511,16 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 {
 	struct mfd_cell	*cell = (struct mfd_cell *)dev->dev.platform_data;
 	struct tmio_mmc_data *pdata;
-	struct resource *res_ctl, *res_cnf;
+	struct resource *res_ctl;
 	struct tmio_mmc_host *host;
 	struct mmc_host *mmc;
 	int ret = -EINVAL;
 
-	if (dev->num_resources != 3)
+	if (dev->num_resources != 2)
 		goto out;
 
 	res_ctl = platform_get_resource(dev, IORESOURCE_MEM, 0);
-	res_cnf = platform_get_resource(dev, IORESOURCE_MEM, 1);
-	if (!res_ctl || !res_cnf)
+	if (!res_ctl)
 		goto out;
 
 	pdata = cell->driver_data;
@@ -539,8 +535,12 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 
 	host = mmc_priv(mmc);
 	host->mmc = mmc;
+	host->pdev = dev;
 	platform_set_drvdata(dev, mmc);
 
+	host->set_pwr = pdata->set_pwr;
+	host->set_no_clk_div = pdata->set_no_clk_div;
+
 	/* SD control register space size is 0x200, 0x400 for bus_shift=1 */
 	host->bus_shift = resource_size(res_ctl) >> 10;
 
@@ -548,10 +548,6 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 	if (!host->ctl)
 		goto host_free;
 
-	host->cnf = ioremap(res_cnf->start, resource_size(res_cnf));
-	if (!host->cnf)
-		goto unmap_ctl;
-
 	mmc->ops = &tmio_mmc_ops;
 	mmc->caps = MMC_CAP_4_BIT_DATA;
 	mmc->f_max = pdata->hclk;
@@ -562,23 +558,9 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 	if (cell->enable) {
 		ret = cell->enable(dev);
 		if (ret)
-			goto unmap_cnf;
+			goto unmap_ctl;
 	}
 
-	/* Enable the MMC/SD Control registers */
-	sd_config_write16(host, CNF_CMD, SDCREN);
-	sd_config_write32(host, CNF_CTL_BASE,
-		(dev->resource[0].start >> host->bus_shift) & 0xfffe);
-
-	/* Disable SD power during suspend */
-	sd_config_write8(host, CNF_PWR_CTL_3, 0x01);
-
-	/* The below is required but why? FIXME */
-	sd_config_write8(host, CNF_STOP_CLK_CTL, 0x1f);
-
-	/* Power down SD bus*/
-	sd_config_write8(host, CNF_PWR_CTL_2, 0x00);
-
 	tmio_mmc_clk_stop(host);
 	reset(host);
 
@@ -586,14 +568,14 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 	if (ret >= 0)
 		host->irq = ret;
 	else
-		goto unmap_cnf;
+		goto unmap_ctl;
 
 	disable_mmc_irqs(host, TMIO_MASK_ALL);
 
 	ret = request_irq(host->irq, tmio_mmc_irq, IRQF_DISABLED |
 		IRQF_TRIGGER_FALLING, "tmio-mmc", host);
 	if (ret)
-		goto unmap_cnf;
+		goto unmap_ctl;
 
 	mmc_add_host(mmc);
 
@@ -605,8 +587,6 @@ static int __devinit tmio_mmc_probe(struct platform_device *dev)
 
 	return 0;
 
-unmap_cnf:
-	iounmap(host->cnf);
 unmap_ctl:
 	iounmap(host->ctl);
 host_free:
@@ -626,7 +606,6 @@ static int __devexit tmio_mmc_remove(struct platform_device *dev)
 		mmc_remove_host(mmc);
 		free_irq(host->irq, host);
 		iounmap(host->ctl);
-		iounmap(host->cnf);
 		mmc_free_host(mmc);
 	}
 
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 9fa9985..c676767 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -11,26 +11,6 @@
 
 #include <linux/highmem.h>
 
-#define CNF_CMD     0x04
-#define CNF_CTL_BASE   0x10
-#define CNF_INT_PIN  0x3d
-#define CNF_STOP_CLK_CTL 0x40
-#define CNF_GCLK_CTL 0x41
-#define CNF_SD_CLK_MODE 0x42
-#define CNF_PIN_STATUS 0x44
-#define CNF_PWR_CTL_1 0x48
-#define CNF_PWR_CTL_2 0x49
-#define CNF_PWR_CTL_3 0x4a
-#define CNF_CARD_DETECT_MODE 0x4c
-#define CNF_SD_SLOT 0x50
-#define CNF_EXT_GCLK_CTL_1 0xf0
-#define CNF_EXT_GCLK_CTL_2 0xf1
-#define CNF_EXT_GCLK_CTL_3 0xf9
-#define CNF_SD_LED_EN_1 0xfa
-#define CNF_SD_LED_EN_2 0xfe
-
-#define   SDCREN 0x2   /* Enable access to MMC CTL regs. (flag in COMMAND_REG)*/
-
 #define CTL_SD_CMD 0x00
 #define CTL_ARG_REG 0x04
 #define CTL_STOP_INTERNAL_ACTION 0x08
@@ -110,7 +90,6 @@
 
 
 struct tmio_mmc_host {
-	void __iomem *cnf;
 	void __iomem *ctl;
 	unsigned long bus_shift;
 	struct mmc_command      *cmd;
@@ -119,10 +98,16 @@ struct tmio_mmc_host {
 	struct mmc_host         *mmc;
 	int                     irq;
 
+	/* Callbacks for clock / power control */
+	void (*set_pwr)(struct platform_device *host, int state);
+	void (*set_no_clk_div)(struct platform_device *host, int state);
+
 	/* pio related stuff */
 	struct scatterlist      *sg_ptr;
 	unsigned int            sg_len;
 	unsigned int            sg_off;
+
+	struct platform_device *pdev;
 };
 
 #include <linux/io.h>
@@ -163,25 +148,6 @@ static inline void sd_ctrl_write32(struct tmio_mmc_host *host, int addr,
 	writew(val >> 16, host->ctl + ((addr + 2) << host->bus_shift));
 }
 
-static inline void sd_config_write8(struct tmio_mmc_host *host, int addr,
-		u8 val)
-{
-	writeb(val, host->cnf + (addr << host->bus_shift));
-}
-
-static inline void sd_config_write16(struct tmio_mmc_host *host, int addr,
-		u16 val)
-{
-	writew(val, host->cnf + (addr << host->bus_shift));
-}
-
-static inline void sd_config_write32(struct tmio_mmc_host *host, int addr,
-		u32 val)
-{
-	writew(val, host->cnf + (addr << host->bus_shift));
-	writew(val >> 16, host->cnf + ((addr + 2) << host->bus_shift));
-}
-
 #include <linux/scatterlist.h>
 #include <linux/blkdev.h>
 
diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
index 6b9c5d0..a206a8d 100644
--- a/include/linux/mfd/tmio.h
+++ b/include/linux/mfd/tmio.h
@@ -2,6 +2,8 @@
 #define MFD_TMIO_H
 
 #include <linux/fb.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
 
 #define tmio_ioread8(addr) readb(addr)
 #define tmio_ioread16(addr) readw(addr)
@@ -18,11 +20,42 @@
 	writew((val) >> 16, (addr) + 2); \
 	} while (0)
 
+#define CNF_CMD     0x04
+#define CNF_CTL_BASE   0x10
+#define CNF_INT_PIN  0x3d
+#define CNF_STOP_CLK_CTL 0x40
+#define CNF_GCLK_CTL 0x41
+#define CNF_SD_CLK_MODE 0x42
+#define CNF_PIN_STATUS 0x44
+#define CNF_PWR_CTL_1 0x48
+#define CNF_PWR_CTL_2 0x49
+#define CNF_PWR_CTL_3 0x4a
+#define CNF_CARD_DETECT_MODE 0x4c
+#define CNF_SD_SLOT 0x50
+#define CNF_EXT_GCLK_CTL_1 0xf0
+#define CNF_EXT_GCLK_CTL_2 0xf1
+#define CNF_EXT_GCLK_CTL_3 0xf9
+#define CNF_SD_LED_EN_1 0xfa
+#define CNF_SD_LED_EN_2 0xfe
+
+#define   SDCREN 0x2   /* Enable access to MMC CTL regs. (flag in COMMAND_REG)*/
+
+#define sd_config_write8(a, b)  tmio_iowrite8((b), (a))
+#define sd_config_write16(a, b) tmio_iowrite16((b), (a))
+#define sd_config_write32(a, b) tmio_iowrite32((b), (a))
+
+int tmio_core_mmc_enable(void __iomem *cnf, unsigned long base);
+int tmio_core_mmc_resume(void __iomem *cnf, unsigned long base);
+void tmio_core_mmc_pwr(void __iomem *cnf, int state);
+void tmio_core_mmc_clk_div(void __iomem *cnf, int state);
+
 /*
  * data for the MMC controller
  */
 struct tmio_mmc_data {
 	const unsigned int		hclk;
+	void (*set_pwr)(struct platform_device *host, int state);
+	void (*set_no_clk_div)(struct platform_device *host, int state);
 };
 
 /*
-- 
1.6.3.3


[-- Attachment #3: 0002-MFD-tc6393xb-fix-clock-frequency.patch --]
[-- Type: text/x-patch, Size: 921 bytes --]

From d6791f2b9a6178aa15edcc854148f9d733ea43a8 Mon Sep 17 00:00:00 2001
From: Ian Molton <ian@mnementh.co.uk>
Date: Sat, 26 Sep 2009 00:44:33 +0100
Subject: [PATCH 2/5] MFD: tc6393xb: fix clock frequency

	This patch corrects the HCLK frequency in the tc6393xb driver, I believe that it uses 33MHz, based on the comments in the (poor) documentation I have.

Signed-off-by: Ian Molton <ian@mnementh.co.uk>
---
 drivers/mfd/tc6393xb.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mfd/tc6393xb.c b/drivers/mfd/tc6393xb.c
index a44b6b0..40bb375 100644
--- a/drivers/mfd/tc6393xb.c
+++ b/drivers/mfd/tc6393xb.c
@@ -376,7 +376,7 @@ static void tc6393xb_mmc_clk_div(struct platform_device *mmc, int state)
 }
 
 static struct tmio_mmc_data tc6393xb_mmc_data = {
-	.hclk = 24000000,
+	.hclk = 33000000,
 	.set_pwr = tc6393xb_mmc_pwr,
 	.set_no_clk_div = tc6393xb_mmc_clk_div,
 };
-- 
1.6.3.3


[-- Attachment #4: 0003-MMC-tmio-mmc-Fix-clocking.patch --]
[-- Type: text/x-patch, Size: 1243 bytes --]

From afbf6ec6df3b5aed93abcfe8f86a32708d4e2ef3 Mon Sep 17 00:00:00 2001
From: Ian Molton <ian@mnementh.co.uk>
Date: Tue, 29 Sep 2009 12:52:03 +0100
Subject: [PATCH 3/5] MMC: tmio-mmc: Fix clocking

	This patch alters the clock selection on tmio-mmc based hosts such that
	it will select the clock closest in frequency to the desired frequency.

	This should improve the accuracy of clock selection, particularly
	during card detection.

Signed-off-by: Ian Molton <ian@mnementh.co.uk>
---
 drivers/mmc/host/tmio_mmc.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c
index e701732..a4cdc0b 100644
--- a/drivers/mmc/host/tmio_mmc.c
+++ b/drivers/mmc/host/tmio_mmc.c
@@ -43,6 +43,15 @@ static void tmio_mmc_set_clock(struct tmio_mmc_host *host, int new_clock)
 		for (clock = host->mmc->f_min, clk = 0x80000080;
 			new_clock >= (clock<<1); clk >>= 1)
 			clock <<= 1;
+
+	/* Round the clock to the closest available. This is required
+	 * for some fussy cards that dont like to initialise below 400kHz
+	 */
+		if (new_clock - clock >= (clock << 1) - new_clock) {
+			clk >>= 1; clock <<= 1;
+		}
+
+	/* Clock enable */
 		clk |= 0x100;
 	}
 
-- 
1.6.3.3


[-- Attachment #5: 0004-MMC-tmio-mmc-Fix-power-sequencing.patch --]
[-- Type: text/x-patch, Size: 1349 bytes --]

From 690e9aea8fcfb78c7fae07d4ccdcffda9cf4ef23 Mon Sep 17 00:00:00 2001
From: Ian Molton <ian@mnementh.co.uk>
Date: Tue, 29 Sep 2009 12:55:27 +0100
Subject: [PATCH 4/5] MMC: tmio-mmc: Fix power sequencing

	This patch fixes power sequencing on tmio-mmc based hosts so that
	the clock is enabled only once the bus has been powered up.

Signed-off-by: Ian Molton <ian@mnementh.co.uk>
---
 drivers/mmc/host/tmio_mmc.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c
index a4cdc0b..f233867 100644
--- a/drivers/mmc/host/tmio_mmc.c
+++ b/drivers/mmc/host/tmio_mmc.c
@@ -435,18 +435,18 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	if (ios->clock)
 		tmio_mmc_set_clock(host, ios->clock);
 
-	/* Power sequence - OFF -> ON -> UP */
+	/* Power sequence - OFF -> UP -> ON */
 	switch (ios->power_mode) {
 	case MMC_POWER_OFF: /* power down SD bus */
 		if (host->set_pwr)
 			host->set_pwr(host->pdev, 0);
 		tmio_mmc_clk_stop(host);
 		break;
-	case MMC_POWER_ON: /* power up SD bus */
+	case MMC_POWER_UP: /* power up SD bus */
 		if (host->set_pwr)
 			host->set_pwr(host->pdev, 1);
 		break;
-	case MMC_POWER_UP: /* start bus clock */
+	case MMC_POWER_ON: /* enable bus clock */
 		tmio_mmc_clk_start(host);
 		break;
 	}
-- 
1.6.3.3


[-- Attachment #6: 0005-MMC-Retry-initialisation-at-a-lower-frequency-if-it-.patch --]
[-- Type: text/x-patch, Size: 1291 bytes --]

From bc0e0adbc24cb7bf8c0119e65d43b42410ce316a Mon Sep 17 00:00:00 2001
From: Ian Molton <ian@mnementh.co.uk>
Date: Tue, 29 Sep 2009 13:39:29 +0100
Subject: [PATCH 5/5] MMC: Retry initialisation at a lower frequency if it times out

	This patch retrys the MMC card initialisation at a lower frequency
	if it fails. It is needed on certain tmio controllers with fussy cards.

Signed-off-by: Ian Molton <ian@mnementh.co.uk>
---
 drivers/mmc/core/mmc.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 06084db..03e782f 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -295,13 +295,17 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
 	struct mmc_card *oldcard)
 {
 	struct mmc_card *card;
-	int err;
+	int err, slow_retry = 0;
 	u32 cid[4];
 	unsigned int max_dtr;
 
 	BUG_ON(!host);
 	WARN_ON(!host->claimed);
 
+retry:
+	if (slow_retry)
+		mmc_set_clock(host, host->f_min);
+
 	/*
 	 * Since we're changing the OCR value, we seem to
 	 * need to tell some cards to go back to the idle
@@ -464,6 +468,11 @@ free_card:
 		mmc_remove_card(card);
 err:
 
+	if (err == -ETIMEDOUT && host->f_min < 400000) {
+		slow_retry = 1;
+		goto retry;
+	}
+
 	return err;
 }
 
-- 
1.6.3.3

Re: TMIO MMC: full patchset.

[Matt Fleming]

On Tue, Sep 29, 2009 at 01:52:50PM +0100, Ian Molton wrote:
> From bc0e0adbc24cb7bf8c0119e65d43b42410ce316a Mon Sep 17 00:00:00 2001
> From: Ian Molton <ian@mnementh.co.uk>
> Date: Tue, 29 Sep 2009 13:39:29 +0100
> Subject: [PATCH 5/5] MMC: Retry initialisation at a lower frequency if it times out
> 
> 	This patch retrys the MMC card initialisation at a lower frequency
> 	if it fails. It is needed on certain tmio controllers with fussy cards.
> 
> Signed-off-by: Ian Molton <ian@mnementh.co.uk>
> ---
>  drivers/mmc/core/mmc.c |   11 ++++++++++-
>  1 files changed, 10 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 06084db..03e782f 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -295,13 +295,17 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>  	struct mmc_card *oldcard)
>  {
>  	struct mmc_card *card;
> -	int err;
> +	int err, slow_retry = 0;
>  	u32 cid[4];
>  	unsigned int max_dtr;
>  
>  	BUG_ON(!host);
>  	WARN_ON(!host->claimed);
>  
> +retry:
> +	if (slow_retry)
> +		mmc_set_clock(host, host->f_min);
> +
>  	/*
>  	 * Since we're changing the OCR value, we seem to
>  	 * need to tell some cards to go back to the idle
> @@ -464,6 +468,11 @@ free_card:
>  		mmc_remove_card(card);
>  err:
>  
> +	if (err == -ETIMEDOUT && host->f_min < 400000) {
> +		slow_retry = 1;
> +		goto retry;
> +	}
> +
>  	return err;
>  }
>  
> -- 
> 1.6.3.3
> 

I'm not sure that propagating this magical 400kHz constant around the
MMC code is the best way to go. It could do with a #define.

You're the second person to run into issues with the lower limit
initialisation patch. Does anybody know where that value came from? Is
it definitely the best value to use or has it just been picked out of
the air?

Re: TMIO MMC: full patchset.

[Pierre Ossman]

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

On Tue, 29 Sep 2009 14:20:58 +0100
Matt Fleming <matt@console-pimps.org> wrote:

> 
> I'm not sure that propagating this magical 400kHz constant around the
> MMC code is the best way to go. It could do with a #define.
> 
> You're the second person to run into issues with the lower limit
> initialisation patch. Does anybody know where that value came from? Is
> it definitely the best value to use or has it just been picked out of
> the air?

The MMC and SD specs state that init must be done at < 400 kHz. We
originally used the lowest possible freq. the controller could handle,
but there was one controller who went so ridiculously low that it caused
other problems.

Re this patch, I'd think it would be more sensible to lower the init
frequency a bit than some retry logic. It is possible to init a card
and for it to end up in a weird state, so we should have margins
already at the first attempt.

Rgds
-- 
     -- Pierre Ossman

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

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

Re: TMIO MMC: full patchset.

[Magnus Damm]

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

Hey Ian,

[CC akpm]

On Tue, Sep 29, 2009 at 9:52 PM, Ian Molton <ian@mnementh.co.uk> wrote:
> Hi guys,
>
> This is the full TMIO MMC patchset, compiled, tested, checkpatch passed.
>
> Please base all tmio based code on top of this patchset.

I just took patch 0001->0005 for a spin on SuperH and they seem fine.
Feel free to add an acked-by from me for the first patch. I know too
little about the other parts to say anything useful.

To get SuperH working I also need to update the kconfig. And to detect
the clock rate dynamically I needed another minor patch. If you
dislike the clock patch then I can also work around it in the board
code for now.

I plan on submitting the platform data patches to the SuperH mailing
list in the near future, but I may need to rework them somehow if you
dislike the clock patch approach.

Cheers,

/ magnus

[-- Attachment #2: linux-2.6.32-rc-tmio_mmc-kconfig-superh-20090930b.patch --]
[-- Type: application/octet-stream, Size: 747 bytes --]

From: Magnus Damm <damm@opensource.se>

Add SUPERH to the Kconfig dependencies for tmio_mmc.

This change allows us to drive the SDHI hardware blocks
found in SuperH Mobile with tmio_mmc.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 drivers/mmc/host/Kconfig |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- 0001/drivers/mmc/host/Kconfig
+++ work/drivers/mmc/host/Kconfig	2009-09-30 17:26:02.000000000 +0900
@@ -288,7 +288,7 @@ config MMC_SDRICOH_CS
 
 config MMC_TMIO
 	tristate "Toshiba Mobile IO Controller (TMIO) MMC/SD function support"
-	depends on MFD_TMIO || MFD_ASIC3
+	depends on MFD_TMIO || MFD_ASIC3 || SUPERH
 	help
 	  This provides support for the SD/MMC cell found in TC6393XB,
 	  T7L66XB and also HTC ASIC3

[-- Attachment #3: linux-2.6.32-rc-tmio_mmc-clk-20090930b.patch --]
[-- Type: application/octet-stream, Size: 2364 bytes --]

From: Magnus Damm <damm@opensource.se>

Extend the tmio_mmc platform data with a struct clk.

With this in place we can assign each tmio_mmc driver
instance their own SDHI clock from the board code.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 drivers/mmc/host/tmio_mmc.c |   17 +++++++++++++++--
 include/linux/mfd/tmio.h    |    1 +
 2 files changed, 16 insertions(+), 2 deletions(-)

--- 0006/drivers/mmc/host/tmio_mmc.c
+++ work/drivers/mmc/host/tmio_mmc.c	2009-09-30 17:43:28.000000000 +0900
@@ -32,6 +32,7 @@
 #include <linux/mmc/host.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/tmio.h>
+#include <linux/clk.h>
 
 #include "tmio_mmc.h"
 
@@ -533,7 +534,7 @@ static int __devinit tmio_mmc_probe(stru
 		goto out;
 
 	pdata = cell->driver_data;
-	if (!pdata || !pdata->hclk)
+	if (!pdata || !(pdata->hclk || pdata->clk))
 		goto out;
 
 	ret = -ENOMEM;
@@ -557,9 +558,15 @@ static int __devinit tmio_mmc_probe(stru
 	if (!host->ctl)
 		goto host_free;
 
+	if (pdata->clk) {
+		clk_enable(pdata->clk);
+		mmc->f_max = clk_get_rate(pdata->clk);
+	} else {
+		mmc->f_max = pdata->hclk;
+	}
+
 	mmc->ops = &tmio_mmc_ops;
 	mmc->caps = MMC_CAP_4_BIT_DATA;
-	mmc->f_max = pdata->hclk;
 	mmc->f_min = mmc->f_max / 512;
 	mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
 
@@ -597,6 +604,8 @@ static int __devinit tmio_mmc_probe(stru
 	return 0;
 
 unmap_ctl:
+	if (pdata->clk)
+		clk_disable(pdata->clk);
 	iounmap(host->ctl);
 host_free:
 	mmc_free_host(mmc);
@@ -607,12 +616,16 @@ out:
 static int __devexit tmio_mmc_remove(struct platform_device *dev)
 {
 	struct mmc_host *mmc = platform_get_drvdata(dev);
+	struct mfd_cell	*cell = (struct mfd_cell *)dev->dev.platform_data;
+	struct tmio_mmc_data *pdata = cell->driver_data;
 
 	platform_set_drvdata(dev, NULL);
 
 	if (mmc) {
 		struct tmio_mmc_host *host = mmc_priv(mmc);
 		mmc_remove_host(mmc);
+		if (pdata->clk)
+			clk_disable(pdata->clk);
 		free_irq(host->irq, host);
 		iounmap(host->ctl);
 		mmc_free_host(mmc);
--- 0003/include/linux/mfd/tmio.h
+++ work/include/linux/mfd/tmio.h	2009-09-30 17:43:28.000000000 +0900
@@ -54,6 +54,7 @@ void tmio_core_mmc_clk_div(void __iomem 
  */
 struct tmio_mmc_data {
 	const unsigned int		hclk;
+	struct clk		*clk;
 	void (*set_pwr)(struct platform_device *host, int state);
 	void (*set_no_clk_div)(struct platform_device *host, int state);
 };

[-- Attachment #4: linux-2.6.32-rc-sh-migor-tmio_mmc-pdata-20090930c.patch --]
[-- Type: application/octet-stream, Size: 3315 bytes --]

From: Magnus Damm <damm@opensource.se>

Convert the Migo-R board to use tmio_mmc for the
SD Card connected to CN9 instead of mmc_spi.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 arch/sh/boards/mach-migor/setup.c |   63 +++++++++++++++++++++++--------------
 1 file changed, 40 insertions(+), 23 deletions(-)

--- 0001/arch/sh/boards/mach-migor/setup.c
+++ work/arch/sh/boards/mach-migor/setup.c	2009-09-30 19:58:41.000000000 +0900
@@ -18,8 +18,6 @@
 #include <linux/delay.h>
 #include <linux/clk.h>
 #include <linux/gpio.h>
-#include <linux/spi/spi.h>
-#include <linux/spi/spi_gpio.h>
 #include <video/sh_mobile_lcdc.h>
 #include <media/sh_mobile_ceu.h>
 #include <media/ov772x.h>
@@ -30,6 +28,8 @@
 #include <asm/sh_keysc.h>
 #include <mach/migor.h>
 #include <cpu/sh7722.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/tmio.h>
 
 /* Address     IRQ  Size  Bus  Description
  * 0x00000000       64MB  16   NOR Flash (SP29PL256N)
@@ -390,17 +390,35 @@ static struct platform_device migor_ceu_
 	},
 };
 
-struct spi_gpio_platform_data sdcard_cn9_platform_data = {
-	.sck = GPIO_PTD0,
-	.mosi = GPIO_PTD1,
-	.miso = GPIO_PTD2,
-	.num_chipselect = 1,
+static struct tmio_mmc_data sdhi_cn9_driver_data = {
 };
 
-static struct platform_device sdcard_cn9_device = {
-	.name		= "spi_gpio",
+static struct mfd_cell sdhi_cn9_platform_data = {
+	.driver_data = &sdhi_cn9_driver_data,
+};
+
+static struct resource sdhi_cn9_resources[] = {
+	[0] = {
+		.name	= "SDHI",
+		.start	= 0x04ce0000,
+		.end	= 0x04ce01ff,
+		.flags	= IORESOURCE_MEM,
+	},
+	[1] = {
+		.start	= 101,
+		.flags  = IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device sdhi_cn9_device = {
+	.name		= "tmio-mmc",
+	.num_resources	= ARRAY_SIZE(sdhi_cn9_resources),
+	.resource	= sdhi_cn9_resources,
 	.dev	= {
-		.platform_data	= &sdcard_cn9_platform_data,
+		.platform_data	= &sdhi_cn9_platform_data,
+	},
+	.archdata = {
+		.hwblk_id = HWBLK_SDHI,
 	},
 };
 
@@ -467,20 +485,11 @@ static struct platform_device *migor_dev
 	&migor_ceu_device,
 	&migor_nor_flash_device,
 	&migor_nand_flash_device,
-	&sdcard_cn9_device,
+	&sdhi_cn9_device,
 	&migor_camera[0],
 	&migor_camera[1],
 };
 
-static struct spi_board_info migor_spi_devices[] = {
-	{
-		.modalias = "mmc_spi",
-		.max_speed_hz = 5000000,
-		.chip_select = 0,
-		.controller_data = (void *) GPIO_PTD5,
-	},
-};
-
 static int __init migor_devices_setup(void)
 {
 
@@ -525,6 +534,17 @@ static int __init migor_devices_setup(vo
 	gpio_request(GPIO_PTA1, NULL);
 	gpio_direction_input(GPIO_PTA1);
 
+	/* SDHI */
+	gpio_request(GPIO_FN_SDHICD, NULL);
+	gpio_request(GPIO_FN_SDHIWP, NULL);
+	gpio_request(GPIO_FN_SDHID3, NULL);
+	gpio_request(GPIO_FN_SDHID2, NULL);
+	gpio_request(GPIO_FN_SDHID1, NULL);
+	gpio_request(GPIO_FN_SDHID0, NULL);
+	gpio_request(GPIO_FN_SDHICMD, NULL);
+	gpio_request(GPIO_FN_SDHICLK, NULL);
+	sdhi_cn9_driver_data.clk = clk_get(NULL, "sdhi0");
+
 	/* Touch Panel */
 	gpio_request(GPIO_FN_IRQ6, NULL);
 
@@ -612,9 +632,6 @@ static int __init migor_devices_setup(vo
 	i2c_register_board_info(0, migor_i2c_devices,
 				ARRAY_SIZE(migor_i2c_devices));
 
-	spi_register_board_info(migor_spi_devices,
-				ARRAY_SIZE(migor_spi_devices));
-
 	return platform_add_devices(migor_devices, ARRAY_SIZE(migor_devices));
 }
 arch_initcall(migor_devices_setup);

[-- Attachment #5: linux-2.6.32-rc-sh-ap325rxa-tmio_mmc-pdata-20090930c.patch --]
[-- Type: application/octet-stream, Size: 3235 bytes --]

From: Magnus Damm <damm@opensource.se>

Convert the AP325 board to use tmio_mmc for the
SD Card connected to CN3 instead of mmc_spi.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 arch/sh/boards/board-ap325rxa.c |   63 ++++++++++++++++++++++++---------------
 1 file changed, 40 insertions(+), 23 deletions(-)

--- 0001/arch/sh/boards/board-ap325rxa.c
+++ work/arch/sh/boards/board-ap325rxa.c	2009-09-30 20:06:59.000000000 +0900
@@ -20,8 +20,6 @@
 #include <linux/i2c.h>
 #include <linux/smsc911x.h>
 #include <linux/gpio.h>
-#include <linux/spi/spi.h>
-#include <linux/spi/spi_gpio.h>
 #include <media/ov772x.h>
 #include <media/soc_camera.h>
 #include <media/soc_camera_platform.h>
@@ -30,6 +28,8 @@
 #include <asm/io.h>
 #include <asm/clock.h>
 #include <cpu/sh7723.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/tmio.h>
 
 static struct smsc911x_platform_config smsc911x_config = {
 	.phy_interface	= PHY_INTERFACE_MODE_MII,
@@ -409,17 +409,35 @@ static struct platform_device ceu_device
 	},
 };
 
-struct spi_gpio_platform_data sdcard_cn3_platform_data = {
-	.sck = GPIO_PTD0,
-	.mosi = GPIO_PTD1,
-	.miso = GPIO_PTD2,
-	.num_chipselect = 1,
+static struct resource sdhi0_cn3_resources[] = {
+	[0] = {
+		.name	= "SDHI0",
+		.start	= 0x04ce0000,
+		.end	= 0x04ce01ff,
+		.flags	= IORESOURCE_MEM,
+	},
+	[1] = {
+		.start	= 101,
+		.flags  = IORESOURCE_IRQ,
+	},
+};
+
+static struct tmio_mmc_data sdhi0_cn3_driver_data = {
+};
+
+struct mfd_cell sdhi0_cn3_platform_data = {
+	.driver_data = &sdhi0_cn3_driver_data,
 };
 
-static struct platform_device sdcard_cn3_device = {
-	.name		= "spi_gpio",
+static struct platform_device sdhi0_cn3_device = {
+	.name		= "tmio-mmc",
+	.num_resources	= ARRAY_SIZE(sdhi0_cn3_resources),
+	.resource	= sdhi0_cn3_resources,
 	.dev	= {
-		.platform_data	= &sdcard_cn3_platform_data,
+		.platform_data	= &sdhi0_cn3_platform_data,
+	},
+	.archdata = {
+		.hwblk_id = HWBLK_SDHI0,
 	},
 };
 
@@ -470,20 +488,11 @@ static struct platform_device *ap325rxa_
 	&lcdc_device,
 	&ceu_device,
 	&nand_flash_device,
-	&sdcard_cn3_device,
+	&sdhi0_cn3_device,
 	&ap325rxa_camera[0],
 	&ap325rxa_camera[1],
 };
 
-static struct spi_board_info ap325rxa_spi_devices[] = {
-	{
-		.modalias = "mmc_spi",
-		.max_speed_hz = 5000000,
-		.chip_select = 0,
-		.controller_data = (void *) GPIO_PTD5,
-	},
-};
-
 static int __init ap325rxa_devices_setup(void)
 {
 	/* LD3 and LD4 LEDs */
@@ -578,12 +587,20 @@ static int __init ap325rxa_devices_setup
 
 	platform_resource_setup_memory(&ceu_device, "ceu", 4 << 20);
 
+	/* SDHI0 */
+	gpio_request(GPIO_FN_SDHI0CD_PTD, NULL);
+	gpio_request(GPIO_FN_SDHI0WP_PTD, NULL);
+	gpio_request(GPIO_FN_SDHI0D3_PTD, NULL);
+	gpio_request(GPIO_FN_SDHI0D2_PTD, NULL);
+	gpio_request(GPIO_FN_SDHI0D1_PTD, NULL);
+	gpio_request(GPIO_FN_SDHI0D0_PTD, NULL);
+	gpio_request(GPIO_FN_SDHI0CMD_PTD, NULL);
+	gpio_request(GPIO_FN_SDHI0CLK_PTD, NULL);
+	sdhi0_cn3_driver_data.clk = clk_get(NULL, "sdhi0");
+
 	i2c_register_board_info(0, ap325rxa_i2c_devices,
 				ARRAY_SIZE(ap325rxa_i2c_devices));
 
-	spi_register_board_info(ap325rxa_spi_devices,
-				ARRAY_SIZE(ap325rxa_spi_devices));
-
 	return platform_add_devices(ap325rxa_devices,
 				ARRAY_SIZE(ap325rxa_devices));
 }

[-- Attachment #6: linux-2.6.32-rc-sh-se7724-tmio_mmc-pdata-20090930c.patch --]
[-- Type: application/octet-stream, Size: 2325 bytes --]

From: Magnus Damm <damm@opensource.se>

Add SD Card support to the se7724 board using the
tmio_mmc driver hooked up to SDHI0 and CN7.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 arch/sh/boards/mach-se/7724/setup.c |   46 +++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

--- 0001/arch/sh/boards/mach-se/7724/setup.c
+++ work/arch/sh/boards/mach-se/7724/setup.c	2009-09-30 20:10:28.000000000 +0900
@@ -20,6 +20,8 @@
 #include <linux/gpio.h>
 #include <linux/input.h>
 #include <linux/usb/r8a66597.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/tmio.h>
 #include <video/sh_mobile_lcdc.h>
 #include <media/sh_mobile_ceu.h>
 #include <sound/sh_fsi.h>
@@ -448,6 +450,38 @@ static struct platform_device sh7724_usb
 	.resource	= sh7724_usb1_gadget_resources,
 };
 
+static struct resource sdhi0_cn7_resources[] = {
+	[0] = {
+		.name	= "SDHI0",
+		.start  = 0x04ce0000,
+		.end    = 0x04ce01ff,
+		.flags  = IORESOURCE_MEM,
+	},
+	[1] = {
+		.start  = 101,
+		.flags  = IORESOURCE_IRQ,
+	},
+};
+
+struct tmio_mmc_data sdhi0_cn7_driver_data = {
+};
+
+struct mfd_cell sdhi0_cn7_platform_data = {
+	.driver_data = &sdhi0_cn7_driver_data,
+};
+
+static struct platform_device sdhi0_cn7_device = {
+	.name           = "tmio-mmc",
+	.num_resources  = ARRAY_SIZE(sdhi0_cn7_resources),
+	.resource       = sdhi0_cn7_resources,
+	.dev    = {
+		.platform_data  = &sdhi0_cn7_platform_data,
+	},
+	.archdata = {
+		.hwblk_id = HWBLK_SDHI0,
+	},
+};
+
 static struct platform_device *ms7724se_devices[] __initdata = {
 	&heartbeat_device,
 	&smc91x_eth_device,
@@ -460,6 +494,7 @@ static struct platform_device *ms7724se_
 	&sh7724_usb0_host_device,
 	&sh7724_usb1_gadget_device,
 	&fsi_device,
+	&sdhi0_cn7_device,
 };
 
 #define EEPROM_OP   0xBA206000
@@ -698,6 +733,17 @@ static int __init devices_setup(void)
 	clk_set_rate(&fsimcka_clk, 11000);
 	clk_put(fsia_clk);
 
+	/* SDHI0 connected to cn7 */
+	gpio_request(GPIO_FN_SDHI0CD, NULL);
+	gpio_request(GPIO_FN_SDHI0WP, NULL);
+	gpio_request(GPIO_FN_SDHI0D3, NULL);
+	gpio_request(GPIO_FN_SDHI0D2, NULL);
+	gpio_request(GPIO_FN_SDHI0D1, NULL);
+	gpio_request(GPIO_FN_SDHI0D0, NULL);
+	gpio_request(GPIO_FN_SDHI0CMD, NULL);
+	gpio_request(GPIO_FN_SDHI0CLK, NULL);
+	sdhi0_cn7_driver_data.clk = clk_get(NULL, "sdhi0");
+
 	/*
 	 * enable SH-Eth
 	 *

[-- Attachment #7: linux-2.6.32-rc-sh-kfr2r09-tmio_mmc-pdata-20090930c.patch --]
[-- Type: application/octet-stream, Size: 2423 bytes --]

From: Magnus Damm <damm@opensource.se>

Add SD Card support to the kfr2r09 board using the
tmio_mmc driver hooked up to SDHI0 and yc304.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 arch/sh/boards/mach-kfr2r09/setup.c |   46 +++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

--- 0001/arch/sh/boards/mach-kfr2r09/setup.c
+++ work/arch/sh/boards/mach-kfr2r09/setup.c	2009-09-30 20:15:33.000000000 +0900
@@ -19,6 +19,8 @@
 #include <linux/i2c.h>
 #include <linux/usb/r8a66597.h>
 #include <video/sh_mobile_lcdc.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/tmio.h>
 #include <asm/clock.h>
 #include <asm/machvec.h>
 #include <asm/io.h>
@@ -212,11 +214,44 @@ static struct platform_device kfr2r09_us
 	.resource	= kfr2r09_usb0_gadget_resources,
 };
 
+static struct resource kfr2r09_sh_sdhi0_resources[] = {
+	[0] = {
+		.name	= "SDHI0",
+		.start  = 0x04ce0000,
+		.end    = 0x04ce01ff,
+		.flags  = IORESOURCE_MEM,
+	},
+	[1] = {
+		.start  = 101,
+		.flags  = IORESOURCE_IRQ,
+	},
+};
+
+struct tmio_mmc_data kfr2r09_sh_sdhi0_driver_data = {
+};
+
+struct mfd_cell kfr2r09_sh_sdhi0_platform_data = {
+	.driver_data = &kfr2r09_sh_sdhi0_driver_data,
+};
+
+static struct platform_device kfr2r09_sh_sdhi0_device = {
+	.name           = "tmio-mmc",
+	.num_resources  = ARRAY_SIZE(kfr2r09_sh_sdhi0_resources),
+	.resource       = kfr2r09_sh_sdhi0_resources,
+	.dev    = {
+		.platform_data  = &kfr2r09_sh_sdhi0_platform_data,
+	},
+	.archdata = {
+		.hwblk_id = HWBLK_SDHI0,
+	},
+};
+
 static struct platform_device *kfr2r09_devices[] __initdata = {
 	&kfr2r09_nor_flash_device,
 	&kfr2r09_nand_flash_device,
 	&kfr2r09_sh_keysc_device,
 	&kfr2r09_sh_lcdc_device,
+	&kfr2r09_sh_sdhi0_device,
 };
 
 #define BSC_CS0BCR 0xfec10004
@@ -361,6 +396,17 @@ static int __init kfr2r09_devices_setup(
 	if (kfr2r09_usb0_gadget_setup() == 0)
 		platform_device_register(&kfr2r09_usb0_gadget_device);
 
+	/* SDHI0 connected to yc304 */
+	gpio_request(GPIO_FN_SDHI0CD, NULL);
+	gpio_request(GPIO_FN_SDHI0WP, NULL);
+	gpio_request(GPIO_FN_SDHI0D3, NULL);
+	gpio_request(GPIO_FN_SDHI0D2, NULL);
+	gpio_request(GPIO_FN_SDHI0D1, NULL);
+	gpio_request(GPIO_FN_SDHI0D0, NULL);
+	gpio_request(GPIO_FN_SDHI0CMD, NULL);
+	gpio_request(GPIO_FN_SDHI0CLK, NULL);
+	kfr2r09_sh_sdhi0_driver_data.clk = clk_get(NULL, "sdhi0");
+
 	return platform_add_devices(kfr2r09_devices,
 				    ARRAY_SIZE(kfr2r09_devices));
 }

Re: TMIO MMC: full patchset.

[Philipp Zabel]

Am Dienstag, den 29.09.2009, 13:52 +0100 schrieb Ian Molton: 
> Hi guys,
> 
> This is the full TMIO MMC patchset, compiled, tested, checkpatch passed.

With bus shift support dropped from the SD_CONFIG register accessors, I
can't use this as-is on ASIC3. What do you think about the following
(sketch of a) patch to add it back?

regards
Philipp 

diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
index a206a8d..89042d6 100644
--- a/include/linux/mfd/tmio.h
+++ b/include/linux/mfd/tmio.h
@@ -40,14 +40,38 @@
 
 #define   SDCREN 0x2   /* Enable access to MMC CTL regs. (flag in COMMAND_REG)*/
 
-#define sd_config_write8(a, b)  tmio_iowrite8((b), (a))
-#define sd_config_write16(a, b) tmio_iowrite16((b), (a))
-#define sd_config_write32(a, b) tmio_iowrite32((b), (a))
-
-int tmio_core_mmc_enable(void __iomem *cnf, unsigned long base);
-int tmio_core_mmc_resume(void __iomem *cnf, unsigned long base);
-void tmio_core_mmc_pwr(void __iomem *cnf, int state);
-void tmio_core_mmc_clk_div(void __iomem *cnf, int state);
+/*
+ * data for the tmio_core_mmc functions (CONFIG regs)
+ */
+struct tmio_core_mmc {
+	void __iomem *cnf;
+	int           bus_shift;
+	unsigned long base;
+};
+
+static inline void sd_config_write8(struct tmio_core_mmc *sd_config, int addr,
+		u8 val)
+{
+	writeb(val, sd_config->cnf + (addr << sd_config->bus_shift));
+}
+
+static inline void sd_config_write16(struct tmio_core_mmc *sd_config, int addr,
+		u16 val)
+{
+	writew(val, sd_config->cnf + (addr << sd_config->bus_shift));
+}
+
+static inline void sd_config_write32(struct tmio_core_mmc *sd_config, int addr,
+		u32 val)
+{
+	writew(val, sd_config->cnf + (addr << sd_config->bus_shift));
+	writew(val >> 16, sd_config->cnf + ((addr + 2) << sd_config->bus_shift));
+}
+
+int tmio_core_mmc_enable(struct tmio_core_mmc *sd_config);
+int tmio_core_mmc_resume(struct tmio_core_mmc *sd_config);
+void tmio_core_mmc_pwr(struct tmio_core_mmc *sd_config, int state);
+void tmio_core_mmc_clk_div(struct tmio_core_mmc *sd_config, int state);
 
 /*
  * data for the MMC controller
diff --git a/drivers/mfd/tmio_core.c b/drivers/mfd/tmio_core.c
index 483fbbb..7abb997 100644
--- a/drivers/mfd/tmio_core.c
+++ b/drivers/mfd/tmio_core.c
@@ -10,45 +10,45 @@
 
 #include <linux/mfd/tmio.h>
 
-int tmio_core_mmc_enable(void __iomem *cnf, unsigned long base)
+int tmio_core_mmc_enable(struct tmio_core_mmc *sd_config, unsigned long base)
 {
 	/* Enable the MMC/SD Control registers */
-	sd_config_write16(cnf + CNF_CMD, SDCREN);
-	sd_config_write32(cnf + CNF_CTL_BASE, base & 0xfffe);
+	sd_config_write16(sd_config, CNF_CMD, SDCREN);
+	sd_config_write32(sd_config, CNF_CTL_BASE, base & 0xfffe);
 
 	/* Disable SD power during suspend */
-	sd_config_write8(cnf + CNF_PWR_CTL_3, 0x01);
+	sd_config_write8(sd_config, CNF_PWR_CTL_3, 0x01);
 
 	/* The below is required but why? FIXME */
-	sd_config_write8(cnf + CNF_STOP_CLK_CTL, 0x1f);
+	sd_config_write8(sd_config, CNF_STOP_CLK_CTL, 0x1f);
 
 	/* Power down SD bus*/
-	sd_config_write8(cnf + CNF_PWR_CTL_2, 0x00);
+	sd_config_write8(sd_config, CNF_PWR_CTL_2, 0x00);
 
 	return 0;
 }
 EXPORT_SYMBOL(tmio_core_mmc_enable);
 
-int tmio_core_mmc_resume(void __iomem *cnf, unsigned long base)
+int tmio_core_mmc_resume(struct tmio_core_mmc *sd_config, unsigned long base)
 {
 
 	/* Enable the MMC/SD Control registers */
-	sd_config_write16(cnf + CNF_CMD, SDCREN);
-	sd_config_write32(cnf + CNF_CTL_BASE, base & 0xfffe);
+	sd_config_write16(sd_config, CNF_CMD, SDCREN);
+	sd_config_write32(sd_config, CNF_CTL_BASE, base & 0xfffe);
 
 	return 0;
 }
 EXPORT_SYMBOL(tmio_core_mmc_resume);
 
-void tmio_core_mmc_pwr(void __iomem *cnf, int state)
+void tmio_core_mmc_pwr(struct tmio_core_mmc *sd_config, int state)
 {
-	sd_config_write8(cnf + CNF_PWR_CTL_2, state ? 0x02 : 0x00);
+	sd_config_write8(sd_config, CNF_PWR_CTL_2, state ? 0x02 : 0x00);
 }
 EXPORT_SYMBOL(tmio_core_mmc_pwr);
 
-void tmio_core_mmc_clk_div(void __iomem *cnf, int state)
+void tmio_core_mmc_clk_div(struct tmio_core_mmc *sd_config, int state)
 {
-	sd_config_write8(cnf + CNF_SD_CLK_MODE, state ? 1 : 0);
+	sd_config_write8(sd_config, CNF_SD_CLK_MODE, state ? 1 : 0);
 }
 EXPORT_SYMBOL(tmio_core_mmc_clk_div);
 
diff --git a/drivers/mfd/tc6387xb.c b/drivers/mfd/tc6387xb.c
index fa8da02..520effe 100644
--- a/drivers/mfd/tc6387xb.c
+++ b/drivers/mfd/tc6387xb.c
@@ -26,10 +26,9 @@ struct tc6387xb {
 	void __iomem *scr;
 	struct clk *clk32k;
 	struct resource rscr;
+	struct tmio_core_mmc sd_config;
 };
 
-static struct resource tc6387xb_mmc_resources[];
-
 /*--------------------------------------------------------------------------*/
 
 #ifdef CONFIG_PM
@@ -54,8 +53,7 @@ static int tc6387xb_resume(struct platform_device *dev)
 	if (pdata && pdata->resume)
 		pdata->resume(dev);
 
-	tmio_core_mmc_resume(tc6387xb->scr + 0x200,
-		tc6387xb_mmc_resources[0].start & 0xfffe);
+	tmio_core_mmc_resume(&tc6387xb->sd_config);
 
 	return 0;
 }
@@ -71,7 +69,7 @@ static void tc6387xb_mmc_pwr(struct platform_device *mmc, int state)
 	struct platform_device *dev = to_platform_device(mmc->dev.parent);
 	struct tc6387xb *tc6387xb = platform_get_drvdata(dev);
 
-	tmio_core_mmc_pwr(tc6387xb->scr + 0x200, state);
+	tmio_core_mmc_pwr(&tc6387xb->sd_config, state);
 }
 
 static void tc6387xb_mmc_clk_div(struct platform_device *mmc, int state)
@@ -79,7 +77,7 @@ static void tc6387xb_mmc_clk_div(struct platform_device *mmc, int state)
 	struct platform_device *dev = to_platform_device(mmc->dev.parent);
 	struct tc6387xb *tc6387xb = platform_get_drvdata(dev);
 
-	tmio_core_mmc_clk_div(tc6387xb->scr + 0x200, state);
+	tmio_core_mmc_clk_div(&tc6387xb->sd_config, state);
 }
 

@@ -90,8 +88,7 @@ static int tc6387xb_mmc_enable(struct platform_device *mmc)
 
 	clk_enable(tc6387xb->clk32k);
 
-	tmio_core_mmc_enable(tc6387xb->scr + 0x200,
-		tc6387xb_mmc_resources[0].start & 0xfffe);
+	tmio_core_mmc_enable(&tc6387xb->sd_config);
 
 	return 0;
 }
@@ -196,6 +193,9 @@ static int tc6387xb_probe(struct platform_device *dev)
 	tc6387xb_cells[TC6387XB_CELL_MMC].data_size =
 		sizeof(tc6387xb_cells[TC6387XB_CELL_MMC]);
 
+	tc6387xb->sd_config.cnf = tc6387xb->scr + 0x200;
+	tc6387xb->sd_config.base = tc6387xb_mmc_resources[0].start & 0xfffe;
+
 	ret = mfd_add_devices(&dev->dev, dev->id, tc6387xb_cells,
 			      ARRAY_SIZE(tc6387xb_cells), iomem, irq);
 

Re: TMIO MMC: full patchset.

[Ian Molton]

No, sorry, this is not the way to do it. Please actually use the
platform hooks I provided, thats *why* I spent all this time rewriting
the driver for you. All you've done is add the same hook I added but
in a broken way.

Passing struct clk pointers about is _not the right way_ its not the
way the clk api was designed to be used. clocks are _got and _put by
name and device references, and the pointers should not be getting
passed from platform code to modular drivers.

Doing it from the platform code via the hooks I provided is not a
workaround - its the proper way to do it until such time as the clk
API is merged into the core kernel and divorced from its CPU
dependency (so that MFD drivers can use it to provide clocks and the
tmio_mmc driver can lose the platform hooks for clocking).

Some constructive advice follows:

Does your clock rate vary dynamically, or is it on a per platform
basis? If the latter then thats fine. If the former, you will need to
do a lot more work as tmio-mmc was not written to support the clock
rate getting changed under its feet, and you could fry peoples
hardware.

Also I would recommend that you merge all the platform data patches
along with the kconfig patch as a single changeset in git or a single
patch if you arent using git (you should use git). There is no reason
to seperate them, and the platform data patches are useless without
the kconfig patch (and vice-versa).

Since you will be then submitting the platform patches all at once,
then I suggest you factor out all the identical MMC related structs
into a seperate file (pxa has devices.c - superh may have something
similar?) and then you can simply add the devices to a list of devices
or have a single function call in each of the platform files, rather
than duplicating the code all over the place. My guess is these wont
be the only superH boards to use this driver. If the name of the MMC
clock differs on each board, you can solve this by using a clock alias
on each board (you may need to copy the clock alias support I wrote
for SA1100/PXA - not sure if it went in to the generic code or just
ARM stuff)

Also you should consider adding updated defconfigs for each of the
affected platforms to that patchset when you submit it - if you dont
you will break them because they will lose their spi MMC access. That
might be a bit serious if thats where their root fs is...

As a bonus of using the tmio-mmc platform hooks properly, you also get
to submit everything via the superH tree. You may consider the Kconfig
patch acked-by me but I think it should be merged as part of a
patchset containing all the platform patches too.

I'd like to see the updated version when you're done.

HTH!

-Ian

2009/9/30 Magnus Damm <magnus.damm@gmail.com>:
> Hey Ian,
>
> [CC akpm]
>
> On Tue, Sep 29, 2009 at 9:52 PM, Ian Molton <ian@mnementh.co.uk> wrote:
>> Hi guys,
>>
>> This is the full TMIO MMC patchset, compiled, tested, checkpatch passed.
>>
>> Please base all tmio based code on top of this patchset.
>
> I just took patch 0001->0005 for a spin on SuperH and they seem fine.
> Feel free to add an acked-by from me for the first patch. I know too
> little about the other parts to say anything useful.
>
> To get SuperH working I also need to update the kconfig. And to detect
> the clock rate dynamically I needed another minor patch. If you
> dislike the clock patch then I can also work around it in the board
> code for now.
>
> I plan on submitting the platform data patches to the SuperH mailing
> list in the near future, but I may need to rework them somehow if you
> dislike the clock patch approach.
>
> Cheers,
>
> / magnus
>



-- 
Ian Molton
Linux, Automotive, and other hacking:
http://www.mnementh.co.uk/

Re: TMIO MMC: full patchset.

[Ian Molton]

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

Hi,

Sorry about that I forgot the base gets shifted there. I dont like
tihs implementation too much though as I want to keep knowledge of the
bus shift out of as muich code as possible. (currently none of the
other tmio MFD cores know about it).

Could you give this a whirl on top of my current patchset and see if
it fixes things for you?

I think the odds of a machine existing that needs two different bus
shifts for two different tmio MFDs is vanishingly small and this code
still allows us to build a kernel that supports machines with
differing bus shifts to each other...

2009/9/30 Philipp Zabel <philipp.zabel@gmail.com>:
> Am Dienstag, den 29.09.2009, 13:52 +0100 schrieb Ian Molton:
>> Hi guys,
>>
>> This is the full TMIO MMC patchset, compiled, tested, checkpatch passed.
>
> With bus shift support dropped from the SD_CONFIG register accessors, I
> can't use this as-is on ASIC3. What do you think about the following
> (sketch of a) patch to add it back?
>
> regards
> Philipp
>
> diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
> index a206a8d..89042d6 100644
> --- a/include/linux/mfd/tmio.h
> +++ b/include/linux/mfd/tmio.h
> @@ -40,14 +40,38 @@
>
>  #define   SDCREN 0x2   /* Enable access to MMC CTL regs. (flag in COMMAND_REG)*/
>
> -#define sd_config_write8(a, b)  tmio_iowrite8((b), (a))
> -#define sd_config_write16(a, b) tmio_iowrite16((b), (a))
> -#define sd_config_write32(a, b) tmio_iowrite32((b), (a))
> -
> -int tmio_core_mmc_enable(void __iomem *cnf, unsigned long base);
> -int tmio_core_mmc_resume(void __iomem *cnf, unsigned long base);
> -void tmio_core_mmc_pwr(void __iomem *cnf, int state);
> -void tmio_core_mmc_clk_div(void __iomem *cnf, int state);
> +/*
> + * data for the tmio_core_mmc functions (CONFIG regs)
> + */
> +struct tmio_core_mmc {
> +       void __iomem *cnf;
> +       int           bus_shift;
> +       unsigned long base;
> +};
> +
> +static inline void sd_config_write8(struct tmio_core_mmc *sd_config, int addr,
> +               u8 val)
> +{
> +       writeb(val, sd_config->cnf + (addr << sd_config->bus_shift));
> +}
> +
> +static inline void sd_config_write16(struct tmio_core_mmc *sd_config, int addr,
> +               u16 val)
> +{
> +       writew(val, sd_config->cnf + (addr << sd_config->bus_shift));
> +}
> +
> +static inline void sd_config_write32(struct tmio_core_mmc *sd_config, int addr,
> +               u32 val)
> +{
> +       writew(val, sd_config->cnf + (addr << sd_config->bus_shift));
> +       writew(val >> 16, sd_config->cnf + ((addr + 2) << sd_config->bus_shift));
> +}
> +
> +int tmio_core_mmc_enable(struct tmio_core_mmc *sd_config);
> +int tmio_core_mmc_resume(struct tmio_core_mmc *sd_config);
> +void tmio_core_mmc_pwr(struct tmio_core_mmc *sd_config, int state);
> +void tmio_core_mmc_clk_div(struct tmio_core_mmc *sd_config, int state);
>
>  /*
>  * data for the MMC controller
> diff --git a/drivers/mfd/tmio_core.c b/drivers/mfd/tmio_core.c
> index 483fbbb..7abb997 100644
> --- a/drivers/mfd/tmio_core.c
> +++ b/drivers/mfd/tmio_core.c
> @@ -10,45 +10,45 @@
>
>  #include <linux/mfd/tmio.h>
>
> -int tmio_core_mmc_enable(void __iomem *cnf, unsigned long base)
> +int tmio_core_mmc_enable(struct tmio_core_mmc *sd_config, unsigned long base)
>  {
>        /* Enable the MMC/SD Control registers */
> -       sd_config_write16(cnf + CNF_CMD, SDCREN);
> -       sd_config_write32(cnf + CNF_CTL_BASE, base & 0xfffe);
> +       sd_config_write16(sd_config, CNF_CMD, SDCREN);
> +       sd_config_write32(sd_config, CNF_CTL_BASE, base & 0xfffe);
>
>        /* Disable SD power during suspend */
> -       sd_config_write8(cnf + CNF_PWR_CTL_3, 0x01);
> +       sd_config_write8(sd_config, CNF_PWR_CTL_3, 0x01);
>
>        /* The below is required but why? FIXME */
> -       sd_config_write8(cnf + CNF_STOP_CLK_CTL, 0x1f);
> +       sd_config_write8(sd_config, CNF_STOP_CLK_CTL, 0x1f);
>
>        /* Power down SD bus*/
> -       sd_config_write8(cnf + CNF_PWR_CTL_2, 0x00);
> +       sd_config_write8(sd_config, CNF_PWR_CTL_2, 0x00);
>
>        return 0;
>  }
>  EXPORT_SYMBOL(tmio_core_mmc_enable);
>
> -int tmio_core_mmc_resume(void __iomem *cnf, unsigned long base)
> +int tmio_core_mmc_resume(struct tmio_core_mmc *sd_config, unsigned long base)
>  {
>
>        /* Enable the MMC/SD Control registers */
> -       sd_config_write16(cnf + CNF_CMD, SDCREN);
> -       sd_config_write32(cnf + CNF_CTL_BASE, base & 0xfffe);
> +       sd_config_write16(sd_config, CNF_CMD, SDCREN);
> +       sd_config_write32(sd_config, CNF_CTL_BASE, base & 0xfffe);
>
>        return 0;
>  }
>  EXPORT_SYMBOL(tmio_core_mmc_resume);
>
> -void tmio_core_mmc_pwr(void __iomem *cnf, int state)
> +void tmio_core_mmc_pwr(struct tmio_core_mmc *sd_config, int state)
>  {
> -       sd_config_write8(cnf + CNF_PWR_CTL_2, state ? 0x02 : 0x00);
> +       sd_config_write8(sd_config, CNF_PWR_CTL_2, state ? 0x02 : 0x00);
>  }
>  EXPORT_SYMBOL(tmio_core_mmc_pwr);
>
> -void tmio_core_mmc_clk_div(void __iomem *cnf, int state)
> +void tmio_core_mmc_clk_div(struct tmio_core_mmc *sd_config, int state)
>  {
> -       sd_config_write8(cnf + CNF_SD_CLK_MODE, state ? 1 : 0);
> +       sd_config_write8(sd_config, CNF_SD_CLK_MODE, state ? 1 : 0);
>  }
>  EXPORT_SYMBOL(tmio_core_mmc_clk_div);
>
> diff --git a/drivers/mfd/tc6387xb.c b/drivers/mfd/tc6387xb.c
> index fa8da02..520effe 100644
> --- a/drivers/mfd/tc6387xb.c
> +++ b/drivers/mfd/tc6387xb.c
> @@ -26,10 +26,9 @@ struct tc6387xb {
>        void __iomem *scr;
>        struct clk *clk32k;
>        struct resource rscr;
> +       struct tmio_core_mmc sd_config;
>  };
>
> -static struct resource tc6387xb_mmc_resources[];
> -
>  /*--------------------------------------------------------------------------*/
>
>  #ifdef CONFIG_PM
> @@ -54,8 +53,7 @@ static int tc6387xb_resume(struct platform_device *dev)
>        if (pdata && pdata->resume)
>                pdata->resume(dev);
>
> -       tmio_core_mmc_resume(tc6387xb->scr + 0x200,
> -               tc6387xb_mmc_resources[0].start & 0xfffe);
> +       tmio_core_mmc_resume(&tc6387xb->sd_config);
>
>        return 0;
>  }
> @@ -71,7 +69,7 @@ static void tc6387xb_mmc_pwr(struct platform_device *mmc, int state)
>        struct platform_device *dev = to_platform_device(mmc->dev.parent);
>        struct tc6387xb *tc6387xb = platform_get_drvdata(dev);
>
> -       tmio_core_mmc_pwr(tc6387xb->scr + 0x200, state);
> +       tmio_core_mmc_pwr(&tc6387xb->sd_config, state);
>  }
>
>  static void tc6387xb_mmc_clk_div(struct platform_device *mmc, int state)
> @@ -79,7 +77,7 @@ static void tc6387xb_mmc_clk_div(struct platform_device *mmc, int state)
>        struct platform_device *dev = to_platform_device(mmc->dev.parent);
>        struct tc6387xb *tc6387xb = platform_get_drvdata(dev);
>
> -       tmio_core_mmc_clk_div(tc6387xb->scr + 0x200, state);
> +       tmio_core_mmc_clk_div(&tc6387xb->sd_config, state);
>  }
>
>
> @@ -90,8 +88,7 @@ static int tc6387xb_mmc_enable(struct platform_device *mmc)
>
>        clk_enable(tc6387xb->clk32k);
>
> -       tmio_core_mmc_enable(tc6387xb->scr + 0x200,
> -               tc6387xb_mmc_resources[0].start & 0xfffe);
> +       tmio_core_mmc_enable(&tc6387xb->sd_config);
>
>        return 0;
>  }
> @@ -196,6 +193,9 @@ static int tc6387xb_probe(struct platform_device *dev)
>        tc6387xb_cells[TC6387XB_CELL_MMC].data_size =
>                sizeof(tc6387xb_cells[TC6387XB_CELL_MMC]);
>
> +       tc6387xb->sd_config.cnf = tc6387xb->scr + 0x200;
> +       tc6387xb->sd_config.base = tc6387xb_mmc_resources[0].start & 0xfffe;
> +
>        ret = mfd_add_devices(&dev->dev, dev->id, tc6387xb_cells,
>                              ARRAY_SIZE(tc6387xb_cells), iomem, irq);
>
>
>
>
>



-- 
Ian Molton
Linux, Automotive, and other hacking:
http://www.mnementh.co.uk/

[-- Attachment #2: asic3_addendum.patch --]
[-- Type: text/x-patch, Size: 2885 bytes --]

diff --git a/drivers/mfd/tmio_core.c b/drivers/mfd/tmio_core.c
index 483fbbb..9b0f660 100644
--- a/drivers/mfd/tmio_core.c
+++ b/drivers/mfd/tmio_core.c
@@ -10,20 +10,22 @@
 
 #include <linux/mfd/tmio.h>
 
+static int shift;
+
 int tmio_core_mmc_enable(void __iomem *cnf, unsigned long base)
 {
 	/* Enable the MMC/SD Control registers */
-	sd_config_write16(cnf + CNF_CMD, SDCREN);
-	sd_config_write32(cnf + CNF_CTL_BASE, base & 0xfffe);
+	sd_config_write16(cnf, shift, CNF_CMD, SDCREN);
+	sd_config_write32(cnf, shift, CNF_CTL_BASE, ((base << shift) & 0xfffe));
 
 	/* Disable SD power during suspend */
-	sd_config_write8(cnf + CNF_PWR_CTL_3, 0x01);
+	sd_config_write8(cnf, shift, CNF_PWR_CTL_3, 0x01);
 
 	/* The below is required but why? FIXME */
-	sd_config_write8(cnf + CNF_STOP_CLK_CTL, 0x1f);
+	sd_config_write8(cnf, shift, CNF_STOP_CLK_CTL, 0x1f);
 
 	/* Power down SD bus*/
-	sd_config_write8(cnf + CNF_PWR_CTL_2, 0x00);
+	sd_config_write8(cnf, shift, CNF_PWR_CTL_2, 0x00);
 
 	return 0;
 }
@@ -33,8 +35,8 @@ int tmio_core_mmc_resume(void __iomem *cnf, unsigned long base)
 {
 
 	/* Enable the MMC/SD Control registers */
-	sd_config_write16(cnf + CNF_CMD, SDCREN);
-	sd_config_write32(cnf + CNF_CTL_BASE, base & 0xfffe);
+	sd_config_write16(cnf, shift, CNF_CMD, SDCREN);
+	sd_config_write32(cnf, shift, CNF_CTL_BASE, (base << shift) & 0xfffe);
 
 	return 0;
 }
@@ -42,13 +44,19 @@ EXPORT_SYMBOL(tmio_core_mmc_resume);
 
 void tmio_core_mmc_pwr(void __iomem *cnf, int state)
 {
-	sd_config_write8(cnf + CNF_PWR_CTL_2, state ? 0x02 : 0x00);
+	sd_config_write8(cnf, shift, CNF_PWR_CTL_2, state ? 0x02 : 0x00);
 }
 EXPORT_SYMBOL(tmio_core_mmc_pwr);
 
 void tmio_core_mmc_clk_div(void __iomem *cnf, int state)
 {
-	sd_config_write8(cnf + CNF_SD_CLK_MODE, state ? 1 : 0);
+	sd_config_write8(cnf, shift, CNF_SD_CLK_MODE, state ? 1 : 0);
 }
 EXPORT_SYMBOL(tmio_core_mmc_clk_div);
 
+void tmio_core_set_bus_shift(int bus_shift) 
+{
+	shift = bus_shift;
+}
+EXPORT_SYMBOL(tmio_core_set_bus_shift);
+
diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
index a206a8d..796b3b3 100644
--- a/include/linux/mfd/tmio.h
+++ b/include/linux/mfd/tmio.h
@@ -40,9 +40,12 @@
 
 #define   SDCREN 0x2   /* Enable access to MMC CTL regs. (flag in COMMAND_REG)*/
 
-#define sd_config_write8(a, b)  tmio_iowrite8((b), (a))
-#define sd_config_write16(a, b) tmio_iowrite16((b), (a))
-#define sd_config_write32(a, b) tmio_iowrite32((b), (a))
+#define sd_config_write8(base, shift, reg, val) \
+	tmio_iowrite8((val), (base) + ((reg) << (shift)))
+#define sd_config_write16(base, shift, reg, val) \
+	tmio_iowrite16((val), (base) + ((reg) << (shift)))
+#define sd_config_write32(base, shift, reg, val) \
+	tmio_iowrite32((val), (base) + ((reg) << (shift)))
 
 int tmio_core_mmc_enable(void __iomem *cnf, unsigned long base);
 int tmio_core_mmc_resume(void __iomem *cnf, unsigned long base);

Re: TMIO MMC: full patchset.

[Magnus Damm]

On Thu, Oct 1, 2009 at 5:15 AM, Ian Molton <ian@mnementh.co.uk> wrote:
> No, sorry, this is not the way to do it. Please actually use the
> platform hooks I provided, thats *why* I spent all this time rewriting
> the driver for you. All you've done is add the same hook I added but
> in a broken way.
>
> Passing struct clk pointers about is _not the right way_ its not the
> way the clk api was designed to be used. clocks are _got and _put by
> name and device references, and the pointers should not be getting
> passed from platform code to modular drivers.

Right, I agree that passing a struct clk pointer is far from perfect.

> Doing it from the platform code via the hooks I provided is not a
> workaround - its the proper way to do it until such time as the clk
> API is merged into the core kernel and divorced from its CPU
> dependency (so that MFD drivers can use it to provide clocks and the
> tmio_mmc driver can lose the platform hooks for clocking).

So you mean that the platform code should do clk_enable() and
clk_get_rate(), then feed the rate to the driver using hclk? That is
doable, but..

The downside of this is that the clock will be enabled regardless if
the tmio-mmc driver is enabled or not. From that perspective I would
prefer that the clock would be enabled in the actual tmio-mmc driver
itself.

Or I could add some ugly #ifdefs around the platform device code.
Maybe the best option is to hack up a mfd driver for just the
superh-mobile specific sdhi part.

> Some constructive advice follows:
>
> Does your clock rate vary dynamically, or is it on a per platform
> basis? If the latter then thats fine. If the former, you will need to
> do a lot more work as tmio-mmc was not written to support the clock
> rate getting changed under its feet, and you could fry peoples
> hardware.

Well, the goal is dynamic clocks, but at this point it's dynamic until
the clock is enabled. So i'd like to only enable the clock when
absolutely needed. I do however understand that neither tmio-mmc nor
the mmc subsystem was written with dynamically changing clocks in
mind.

Using clk_get_rate() after clk_enable() should be fine, see the
comment in linux/clk.h:

---
/**
 * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
 *		  This is only valid once the clock source has been enabled.
 * @clk: clock source
 */
unsigned long clk_get_rate(struct clk *clk);
---

At some point I'd like to add runtime pm support to the tmio-mmc
driver as well. With such a change it should be possible to power down
the hardware block while the system is idle and re-setup everything
when next access happens.

I hacked up a prototype for just that a few months ago, but it needs
more work. For now it's probably best just to focus on getting
_something_ supported to begin with.

> Also I would recommend that you merge all the platform data patches
> along with the kconfig patch as a single changeset in git or a single
> patch if you arent using git (you should use git). There is no reason
> to seperate them, and the platform data patches are useless without
> the kconfig patch (and vice-versa).

Thanks for your recommendation, but I'd like to keep the patches as is.

This because:
a) they are independent changes
b) they are potentially destined for more than one tree
c) they are for individual cpus/boards, a combined patch makes back
porting painful
d) having them separate minimizes amount of potential collision

> Since you will be then submitting the platform patches all at once,
> then I suggest you factor out all the identical MMC related structs
> into a seperate file (pxa has devices.c - superh may have something
> similar?) and then you can simply add the devices to a list of devices
> or have a single function call in each of the platform files, rather
> than duplicating the code all over the place. My guess is these wont
> be the only superH boards to use this driver. If the name of the MMC
> clock differs on each board, you can solve this by using a clock alias
> on each board (you may need to copy the clock alias support I wrote
> for SA1100/PXA - not sure if it went in to the generic code or just
> ARM stuff)

You are right that other boards and processor will want to use this.

Regarding duplicated code, please have a second look. The platform
data is similar but not identical. The hwblk_id part of the platform
device varies with processor model. With the platform data we also
need pinmux configuration, and that varies with board type so we end
up with board specific code regardless.

> Also you should consider adding updated defconfigs for each of the
> affected platforms to that patchset when you submit it - if you dont
> you will break them because they will lose their spi MMC access. That
> might be a bit serious if thats where their root fs is...

Right, but updating the defconfig usually happens later in the cycle.
Updating the defconfig for each little change will just force a
certain merge order, and this may make the entire tree stall if one
little patch gets delayed. So to maximize patch throughput I prefer to
minimize the amount of dependencies thank you.

As for the mmc over spi access, the platform data should be merged
after your patches and the kconfig bits so that will not generate any
breakage. We do of course control these things through the superh tree
already.

> As a bonus of using the tmio-mmc platform hooks properly, you also get
> to submit everything via the superH tree. You may consider the Kconfig
> patch acked-by me but I think it should be merged as part of a
> patchset containing all the platform patches too.

The kconfig bit can be merged at any point. If someone enables the
driver in the kconfig but lacks platform data then nothing happens.
But yes, I prefer to take things through the superh tree.

> I'd like to see the updated version when you're done.

I'll do my best to keep you CC:ed.

Any chance we can get at least the ian-0001 patch and the kconfig bit
merged in 2.6.32-rc? I realize it's a bit late in the merge cycle
though. If we stick to 2.6.33 then this little change took more than
one year from initial patches to inclusion in a stable kernel....

Thanks,

/ magnus

Re: TMIO MMC: full patchset.

[Andrew Morton]

On Thu, 1 Oct 2009 10:30:42 +0900 Magnus Damm <magnus.damm@gmail.com> wrote:

> Any chance we can get at least the ian-0001 patch and the kconfig bit
> merged in 2.6.32-rc?

<wakes up>

Please flag the proposed merge plans clearly in the description when
you send out the patch series and lets take a look at it then.

I don't need much reason to shove stuff into -rc2, but it's nice to
have _some_ reason ;)

Re: TMIO MMC: full patchset.

[Ian Molton]

2009/10/1 Magnus Damm <magnus.damm@gmail.com>:
>
>> Passing struct clk pointers about is _not the right way_ its not the
>> way the clk api was designed to be used. clocks are _got and _put by
>> name and device references, and the pointers should not be getting
>> passed from platform code to modular drivers.
>
> Right, I agree that passing a struct clk pointer is far from perfect.

It isnt imperfect, its a violation of the API.
> So you mean that the platform code should do clk_enable() and
> clk_get_rate(), then feed the rate to the driver using hclk? That is
> doable, but..
>
> The downside of this is that the clock will be enabled regardless if
> the tmio-mmc driver is enabled or not. From that perspective I would
> prefer that the clock would be enabled in the actual tmio-mmc driver
> itself.

Thats unfortunate - but realistically thee only good fix for that is
fixing the clk API so  that its not cpu tied.  I've altered tmio-mmc.c
as much as I am comfortable with already.

> Or I could add some ugly #ifdefs around the platform device code.

No.

> Maybe the best option is to hack up a mfd driver for just the
> superh-mobile specific sdhi part.

I dont see why not - the MFD layer is quite lightweight and you only
need to provide enable and disable hooks.

Your current implementation is broken in that if as you say the clock
rate can vary prior to loading the MMC  driver then you will end up
with f_max being set to whatever random clock was set when the driver
probes.

> At some point I'd like to add runtime pm support to the tmio-mmc
> driver as well. With such a change it should be possible to power down
> the hardware block while the system is idle and re-setup everything
> when next access happens.

it does stop the clock already when its not in use. (just not the input clock).

> You are right that other boards and processor will want to use this.
>
> Regarding duplicated code, please have a second look. The platform
> data is similar but not identical.

The resources are...

> Any chance we can get at least the ian-0001 patch and the kconfig bit
> merged in 2.6.32-rc? I realize it's a bit late in the merge cycle
> though.

I have a complete set of patches here, I'd like to send them in
together, so just waiting on ASIC3 stuff to settle now.

> If we stick to 2.6.33 then this little change took more than
> one year from initial patches to inclusion in a stable kernel....

That happens. No point in rushing things, lets get it _right_.

btw, tc6387xb is a very 'thin' MFD layer for a similar sounding
device, you might use that?

-- 
Ian Molton
Linux, Automotive, and other hacking:
http://www.mnementh.co.uk/

Re: TMIO MMC: full patchset.

[Ian Molton]

Hi :-)

I've got a small patchset for TMIO nearly ready - I'm waiting for some
ASIC3 stuff, then I'd like to push the set.

2009/10/1 Andrew Morton <akpm@linux-foundation.org>:
> On Thu, 1 Oct 2009 10:30:42 +0900 Magnus Damm <magnus.damm@gmail.com> wrote:
>
>> Any chance we can get at least the ian-0001 patch and the kconfig bit
>> merged in 2.6.32-rc?
>
> <wakes up>
>
> Please flag the proposed merge plans clearly in the description when
> you send out the patch series and lets take a look at it then.
>
> I don't need much reason to shove stuff into -rc2, but it's nice to
> have _some_ reason ;)
>
>



-- 
Ian Molton
Linux, Automotive, and other hacking:
http://www.mnementh.co.uk/

Re: TMIO MMC: full patchset.

[Magnus Damm]

[CC Paul Mundt]

On Thu, Oct 1, 2009 at 7:19 PM, Ian Molton <ian@mnementh.co.uk> wrote:
> 2009/10/1 Magnus Damm <magnus.damm@gmail.com>:

>> Maybe the best option is to hack up a mfd driver for just the
>> superh-mobile specific sdhi part.
>
> I dont see why not - the MFD layer is quite lightweight and you only
> need to provide enable and disable hooks.

Right, the MFD driver approach is probably the best one.

> Your current implementation is broken in that if as you say the clock
> rate can vary prior to loading the MMC  driver then you will end up
> with f_max being set to whatever random clock was set when the driver
> probes.

No, it's not broken. When the clock is enabled the frequency stays
fixed. How would things work at all if that wasn't the case? The clock
rate comes from whatever bus frequency that is used for the bus that
the SDHI block is hooked up to. So it's far from random.

>> At some point I'd like to add runtime pm support to the tmio-mmc
>> driver as well. With such a change it should be possible to power down
>> the hardware block while the system is idle and re-setup everything
>> when next access happens.
>
> it does stop the clock already when its not in use. (just not the input clock).

Yes, the external clock. That's good.

I'm talking about the clock driving the SDHI block. The clock stopping
is not so exciting though, the real power savings come when we can
disable the power to the hardware block as well dynamically.

>> Any chance we can get at least the ian-0001 patch and the kconfig bit
>> merged in 2.6.32-rc? I realize it's a bit late in the merge cycle
>> though.
>
> I have a complete set of patches here, I'd like to send them in
> together, so just waiting on ASIC3 stuff to settle now.

I only need the first patch to get SuperH Mobile SDHI up and running.

Andrew, Paul, please let me know if you need anything else from me.

Cheers,

/ magnus

Re: TMIO MMC: full patchset.

[Magnus Damm]

On Thu, Oct 1, 2009 at 10:39 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Thu, 1 Oct 2009 10:30:42 +0900 Magnus Damm <magnus.damm@gmail.com> wrote:
>
>> Any chance we can get at least the ian-0001 patch and the kconfig bit
>> merged in 2.6.32-rc?
>
> <wakes up>
>
> Please flag the proposed merge plans clearly in the description when
> you send out the patch series and lets take a look at it then.

I propose the following merge order:

1) The following MMC specific patch from Ian gets merged first:
[PATCH 1/5] MMC: hardware abstraction for CNF area
(maybe we need a new version of that one?)

2) My series is then merged through the SuperH tree:
[PATCH 00/07] sh: SuperH Mobile SDHI changes
(I may need to rework some patches after feedback)

I'd like to see this in 2.6.32 since it's a fairly insular change and
we've been struggling with this issue for more than half a year now.
It would be nice to not delay it any further.

Cheers,

/ magnus

Re: TMIO MMC: full patchset.

[Ian Molton]

I'd prefer to see both the superH and ASIC3 MFD core drivers working
with the new tmio-mmc.c without needing changes made to tmoi-mmc.c
before we merge anything. I expect you can get the superH MFD driver
written in about 15 minutes (copy tc6387xb, drop the CNF related io,
and tweak to taste), so it shouldnt be much of a hardship.

I *think* I've done enough to support superH now, but I'd like to be
sure, and thus wait for Philipp to have his say.

It wont take long, be patient.

-Ian

2009/10/2 Magnus Damm <magnus.damm@gmail.com>:
> On Thu, Oct 1, 2009 at 10:39 AM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
>> On Thu, 1 Oct 2009 10:30:42 +0900 Magnus Damm <magnus.damm@gmail.com> wrote:
>>
>>> Any chance we can get at least the ian-0001 patch and the kconfig bit
>>> merged in 2.6.32-rc?
>>
>> <wakes up>
>>
>> Please flag the proposed merge plans clearly in the description when
>> you send out the patch series and lets take a look at it then.
>
> I propose the following merge order:
>
> 1) The following MMC specific patch from Ian gets merged first:
> [PATCH 1/5] MMC: hardware abstraction for CNF area
> (maybe we need a new version of that one?)
>
> 2) My series is then merged through the SuperH tree:
> [PATCH 00/07] sh: SuperH Mobile SDHI changes
> (I may need to rework some patches after feedback)
>
> I'd like to see this in 2.6.32 since it's a fairly insular change and
> we've been struggling with this issue for more than half a year now.
> It would be nice to not delay it any further.
>
> Cheers,
>
> / magnus
>



-- 
Ian Molton
Linux, Automotive, and other hacking:
http://www.mnementh.co.uk/

Re: TMIO MMC: full patchset.

[Ian Molton]

2009/10/2 Magnus Damm <magnus.damm@gmail.com>:
> [CC Paul Mundt]
>
> Right, the MFD driver approach is probably the best one.

:-)

>> Your current implementation is broken in that if as you say the clock
>> rate can vary prior to loading the MMC  driver then you will end up
>> with f_max being set to whatever random clock was set when the driver
>> probes.
>
> No, it's not broken. When the clock is enabled the frequency stays
> fixed. How would things work at all if that wasn't the case?

I asked if the clock freq. changes - you said 'Well, the goal is
dynamic clocks, but at this point it's dynamic until the clock is
enabled'.

Since your code doesnt set the clock frequency, that means the MMC
drivers f_max will be set to whatever the clock was running at when
you grab it.

> The clock
> rate comes from whatever bus frequency that is used for the bus that
> the SDHI block is hooked up to. So it's far from random.

As long as it has no other users that try to change it. What you are
describing isnt a dynamic clock. its a fixed clock that is diifferent
on different platforms.

>> it does stop the clock already when its not in use. (just not the input clock).
>
> Yes, the external clock. That's good.

I think you mean bus clock there.

> I'm talking about the clock driving the SDHI block.

The *input* clock, yes.

> The clock stopping
> is not so exciting though, the real power savings come when we can
> disable the power to the hardware block as well dynamically.

Well the tmio MFD drivers do just that, if you take a look. The hooks
are already there.

> I only need the first patch to get SuperH Mobile SDHI up and running.

IMHO the first patch is not ready until superH and ASIC3 have been
tested and working without modifying it. Thats why I've asked Philipp
to test my changes to it.

-- 
Ian Molton
Linux, Automotive, and other hacking:
http://www.mnementh.co.uk/

Re: TMIO MMC: full patchset.

[pHilipp Zabel]

On Fri, Oct 2, 2009 at 9:23 AM, Ian Molton <ian@mnementh.co.uk> wrote:
> I'd prefer to see both the superH and ASIC3 MFD core drivers working
> with the new tmio-mmc.c without needing changes made to tmoi-mmc.c
> before we merge anything. I expect you can get the superH MFD driver
> written in about 15 minutes (copy tc6387xb, drop the CNF related io,
> and tweak to taste), so it shouldnt be much of a hardship.
>
> I *think* I've done enough to support superH now, but I'd like to be
> sure, and thus wait for Philipp to have his say.
>
> It wont take long, be patient.

Yes, bear with me for two more days.
I'll test on ASIC3 this weekend, the static int base_shift approach
looks ok to me.

regards
Philipp

Re: TMIO MMC: full patchset.

[Magnus Damm]

On Fri, Oct 2, 2009 at 4:31 PM, Ian Molton <ian@mnementh.co.uk> wrote:
> 2009/10/2 Magnus Damm <magnus.damm@gmail.com>:
>> [CC Paul Mundt]
>>
>> Right, the MFD driver approach is probably the best one.
>
> :-)

I'm glad we can agree on that one at least. =)

>>> Your current implementation is broken in that if as you say the clock
>>> rate can vary prior to loading the MMC  driver then you will end up
>>> with f_max being set to whatever random clock was set when the driver
>>> probes.
>>
>> No, it's not broken. When the clock is enabled the frequency stays
>> fixed. How would things work at all if that wasn't the case?
>
> I asked if the clock freq. changes - you said 'Well, the goal is
> dynamic clocks, but at this point it's dynamic until the clock is
> enabled'.
>
> Since your code doesnt set the clock frequency, that means the MMC
> drivers f_max will be set to whatever the clock was running at when
> you grab it.

Yes?

>> The clock
>> rate comes from whatever bus frequency that is used for the bus that
>> the SDHI block is hooked up to. So it's far from random.
>
> As long as it has no other users that try to change it. What you are
> describing isnt a dynamic clock. its a fixed clock that is diifferent
> on different platforms.

You can call it whatever you want. On SuperH we can do clock scaling
which seems very dynamic to me. But before changing the clock we need
to make sure that all drivers and  hardware blocks support changing
the frequency. Changing the clock frequency while the driver or
hardware needs it fixed is of course a programming error.

>>> it does stop the clock already when its not in use. (just not the input clock).
>>
>> Yes, the external clock. That's good.
>
> I think you mean bus clock there.
>
>> I'm talking about the clock driving the SDHI block.
>
> The *input* clock, yes.

This conversation is giving me headache. We obviously talk about
different clocks. I want to control the clock that drives the SDHI
hardware block. So when the system is idle, both the clock driving the
SDHI block and the clock driving the SD Card will be stopped.

>> The clock stopping
>> is not so exciting though, the real power savings come when we can
>> disable the power to the hardware block as well dynamically.
>
> Well the tmio MFD drivers do just that, if you take a look. The hooks
> are already there.

Do you mean powering off the SD-Card? I'm talking about cutting power
to the MMC controller driven by tmio-mmc. All register state will be
gone. This while the system is kept running from a user point of view.

I don't think your MFD drivers do that, especially since the Runtime
PM framework was merged quite recently and I doubt that there are any
ARM implementations available upstream at this point.

>> I only need the first patch to get SuperH Mobile SDHI up and running.
>
> IMHO the first patch is not ready until superH and ASIC3 have been
> tested and working without modifying it. Thats why I've asked Philipp
> to test my changes to it.

I've already tested the first patch with my series that that I posted
earlier today. So from the SuperH side things are ok.

/ magnus

Re: TMIO MMC: full patchset.

[Ian Molton]

2009/10/2 Magnus Damm <magnus.damm@gmail.com>:

>> I asked if the clock freq. changes - you said 'Well, the goal is
>> dynamic clocks, but at this point it's dynamic until the clock is
>> enabled'.
>>
>> Since your code doesnt set the clock frequency, that means the MMC
>> drivers f_max will be set to whatever the clock was running at when
>> you grab it.
>
> Yes?

Well if its set to 512kHz prior to being claimed, that means the MMC
clock will run between 1kHz and 512kHz

tmio_mmc will work like that but it seems kinda daft...

> You can call it whatever you want. On SuperH we can do clock scaling
> which seems very dynamic to me. But before changing the clock we need
> to make sure that all drivers and  hardware blocks support changing
> the frequency. Changing the clock frequency while the driver or
> hardware needs it fixed is of course a programming error.

exactly - which is one reason why I said your clk based implementation
is broken - tmio_mmc has no logic for coping with a clock that changed
frequency. Since all tmio-based devices appear to have the
frequency-divider  built in, I suspect that all tmio units expect to
have a host (input) clock in the 20-30MHz range. Since the superH tmio
blocks clock divider is missing the unity divisor, varying the imput
clock speed to change the MMC bus frequency seems like a bad idea. If
you could do unity-divisor then you might be able to save a little
power disabling the TMIO divisor and adjusting the input clock
frequency

>> Well the tmio MFD drivers do just that, if you take a look. The hooks
>> are already there.
>
> Do you mean powering off the SD-Card?

Well the CNF area controls card clocking and power.
but the MFD code provides enable and disable hooks. Cutting the clock
and sleeping the chip is the best the toshiba hardware can do, but
theres no reason not to turn off the chip when its not in use (as
opposed to suspended / idle - or the registers will lose state)

> I don't think your MFD drivers do that,

Actually they do provide provision for that by means of the enable /
disable hooks.

if you unload the driver, the disable hook will get called and you can
power it off there.

> especially since the Runtime
> PM framework was merged quite recently and I doubt that there are any
> ARM implementations available upstream at this point.

Thats something for the future - right now tmio-mmc wont take power
being interrupted whilst it runs.

> I've already tested the first patch with my series that that I posted
> earlier today. So from the SuperH side things are ok.

Great - as soon as Philipp gets back to us with the ASIC3 update I'll
re-do my patchset to include ASIC3 - if the changes to tmio-mmc and
submitted seperately, it will break ASIC3 so that needs to go in the
same patchset. Philipp will be ready this weekend, not long to wait.

-- 
Ian Molton
Linux, Automotive, and other hacking:
http://www.mnementh.co.uk/

Re: TMIO MMC: full patchset.

[Magnus Damm]

On Sat, Oct 3, 2009 at 3:14 AM, Ian Molton <ian@mnementh.co.uk> wrote:
> 2009/10/2 Magnus Damm <magnus.damm@gmail.com>:
>
>>> I asked if the clock freq. changes - you said 'Well, the goal is
>>> dynamic clocks, but at this point it's dynamic until the clock is
>>> enabled'.
>>>
>>> Since your code doesnt set the clock frequency, that means the MMC
>>> drivers f_max will be set to whatever the clock was running at when
>>> you grab it.
>>
>> Yes?
>
> Well if its set to 512kHz prior to being claimed, that means the MMC
> clock will run between 1kHz and 512kHz
>
> tmio_mmc will work like that but it seems kinda daft...

512kHz?

Exactly how the SDHI is hooked up varies from processor to processor,
and since the clock generator is configurable by software the rate may
vary depending on embedded application. The numbers I've seen so far
seem to be around 100 _MHz_ which should be more than enough. Don't
worry.

>> You can call it whatever you want. On SuperH we can do clock scaling
>> which seems very dynamic to me. But before changing the clock we need
>> to make sure that all drivers and  hardware blocks support changing
>> the frequency. Changing the clock frequency while the driver or
>> hardware needs it fixed is of course a programming error.
>
> exactly - which is one reason why I said your clk based implementation
> is broken - tmio_mmc has no logic for coping with a clock that changed
> frequency. Since all tmio-based devices appear to have the
> frequency-divider  built in, I suspect that all tmio units expect to
> have a host (input) clock in the 20-30MHz range. Since the superH tmio
> blocks clock divider is missing the unity divisor, varying the imput
> clock speed to change the MMC bus frequency seems like a bad idea. If
> you could do unity-divisor then you might be able to save a little
> power disabling the TMIO divisor and adjusting the input clock
> frequency

So the shared SDHI input clock is usually set to around 100 MHz. If
I'm right then this clock is divided by the value in the
CTL_SD_CARD_CLK_CTL register which allows us to divide by a maximum of
512. Which would be enough to give us a wide range of valid MMC
frequencies.

In my opinion the code is not broken since we're _not_ changing the
frequency while running today. After the clock is enabled with
clk_enable(), the rate is fixed. The frequency may change again after
the clock is disabled with clk_disable(), so if we ever extend the
code to disable and enable on a more fine grained basis we need to
read out the frequency again after redoing clk_enable(). The
i2c-sh_mobile.c driver is a good example of a driver with more fine
grained clock control - it keeps the clock off while idle and enables
and reads out the frequency when it is requested to do any i2c
activity and then disables the clock again when done.

Right now we keep the clock constantly enabled while the tmio-mmc
driver instance is running so there is no problem. And if someone
would change the frequency while we are running then it's a bug. But
that's a system configuration issue which will affect a whole bunch of
drivers so I wouldn't worry about it if I were you. And it's far from
a unique problem to the tmio-mmc driver. Think about it - on-chip we
have serial ports, USB controllers, sound interfaces, RAM controllers,
ethernet controllers, LCD panel hardware interface blocks, camera
interfaces and a whole bunch of other more specialized hardware
blocks. Most of these have drivers that use the clock framework
already and if someone would change the frequency "behind the back"
without notifying and updating the drivers then the clocks for the
hardware interfaces may go out of range and things will just stop
working. So we're not doing that. That would be silly.

But yes, neither the tmio-mmc driver nor the mmc framework supports
updating the frequency while running. At least last time I checked.
That's usually the case when the frequency range is set once at
register time. =) It would be nice to add support for updating the
range though. I did just that for clockevents a few months ago.

>>> Well the tmio MFD drivers do just that, if you take a look. The hooks
>>> are already there.
>>
>> Do you mean powering off the SD-Card?
>
> Well the CNF area controls card clocking and power.
> but the MFD code provides enable and disable hooks. Cutting the clock
> and sleeping the chip is the best the toshiba hardware can do, but
> theres no reason not to turn off the chip when its not in use (as
> opposed to suspended / idle - or the registers will lose state)

Well, _we_ can control the power domain where the SDHI block(s) is/are
located. And I suspect SoCs from other embedded processor vendors can
do the same. But to do that we need to have support for saving and
restoring the state in the driver.

Try this:
damm@rxone ~/git/linux-2.6 $ find drivers/ | xargs grep pm_runtime

As you can see, quite a few of my drivers save and restore state
already. I'd be happy doing the technical work for the tmio-mmc
driver. Unless you want to. =)

But before I even think of extending the tmio-mmc driver _we_ _really_
need to work on improving the patch throughput a bit. I guess it's a
question of trust. From my point of view it is risky and frustrating
to do the work if we can't agree on things. From your side I suspect
that you just don't want any breakage. I think you should trust me on
not wanting to break your driver and in the rare case I would then I
have time to spend on fixing it up.

>> I don't think your MFD drivers do that,
>
> Actually they do provide provision for that by means of the enable /
> disable hooks.
>
> if you unload the driver, the disable hook will get called and you can
> power it off there.

I can see a cell->enable() call in probe(), but I can't see any
cell->disable() calls in the tmio-mmc driver. I sort of expected the
remove() function in tmio-mmc and the probe() error handling code to
do cell->disable(), but maybe I'm misunderstanding the way the MFD
drivers work?

>> especially since the Runtime
>> PM framework was merged quite recently and I doubt that there are any
>> ARM implementations available upstream at this point.
>
> Thats something for the future - right now tmio-mmc wont take power
> being interrupted whilst it runs.

Yeah, indeed something for the future.

>> I've already tested the first patch with my series that that I posted
>> earlier today. So from the SuperH side things are ok.
>
> Great - as soon as Philipp gets back to us with the ASIC3 update I'll
> re-do my patchset to include ASIC3 - if the changes to tmio-mmc and
> submitted seperately, it will break ASIC3 so that needs to go in the
> same patchset. Philipp will be ready this weekend, not long to wait.

Great, thanks a lot guys!

/ magnus

Re: TMIO MMC: full patchset.

[Ian Molton]

2009/10/3 Magnus Damm <magnus.damm@gmail.com>:
> On Sat, Oct 3, 2009 at 3:14 AM, Ian Molton <ian@mnementh.co.uk> wrote:

> 512kHz?

That was an example. I suppose if your HCLK (the input or host clock,
as named in the TMIO specs) never falls below double the maximum
MMC/SD bus speed, then you will be ok.

Do _any_ SH platforms actually share HCLK with other peripherals?

> So the shared SDHI input clock is usually set to around 100 MHz. If
> I'm right then this clock is divided by the value in the
> CTL_SD_CARD_CLK_CTL register which allows us to divide by a maximum of
> 512.

And in your case, a minimum of 2, IIRC.

> Which would be enough to give us a wide range of valid MMC
> frequencies.

If nothing else shares the HCLK line, I'd recommend you set it such
that one of the lower clock speeds is exactly 400kHz. I'd also leave
200 and 100kHz as options below that - some cards really do need to
init that slowly.

> But yes, neither the tmio-mmc driver nor the mmc framework supports
> updating the frequency while running. At least last time I checked.
> That's usually the case when the frequency range is set once at
> register time. =) It would be nice to add support for updating the
> range though. I did just that for clockevents a few months ago.

I doubt it'd be  much use, the TMIO MMC core doesnt use much power to
preserve its state. on my platforms, the chip containing it is simply
put to sleep and can remain so for days without draining the battery,
and when it wakes up again the registers are as expected.

Thus far only one TMIO block has register save / restore and thats
because its in a braindamaged hardware design that shuts off power to
the ASIC when it sleeps.

> Well, _we_ can control the power domain where the SDHI block(s) is/are
> located.

Its probably not worth it, unless the SH design is merely a copy of
the tmio design rather than a reimplementation. it barely uses any
power when the clock is off anyway.

> And I suspect SoCs from other embedded processor vendors can
> do the same. But to do that we need to have support for saving and
> restoring the state in the driver.

Given how low power the TMIO hardware is, and how fast it resumes,
that seems counterproductive to me.

> As you can see, quite a few of my drivers save and restore state
> already. I'd be happy doing the technical work for the tmio-mmc
> driver. Unless you want to. =)

I'd like to see that this would result in significant power saving
first. simply halting the block on my boards drops it to damn near
zero consumption, and its still ready to go then.

> But before I even think of extending the tmio-mmc driver _we_ _really_
> need to work on improving the patch throughput a bit. I guess it's a
> question of trust. From my point of view it is risky and frustrating
> to do the work if we can't agree on things.

Thats easily solved - agree on what needs to be done first.

> From your side I suspect
> that you just don't want any breakage. I think you should trust me on
> not wanting to break your driver and in the rare case I would then I
> have time to spend on fixing it up.

Well, if I had done that this time, we'd have a driver with two
clocking methods in it now...

I don't think my review has been overly slow, especially in the light
of the fact that _I_ had to write the code you're now using, despite
having no platforms that benefit from it at all. Perhaps if you'd done
that the first time I suggetsed it, you wouldnt have had to wait for
me to do it for you?

Yes, the above paragraph is meant to sound irritated. I've put quite
some time into updating the driver _just for you_.  Time I could have
spent doing other things, and time I'm sure Philipp could be better
using rather than updating ASIC3 _just for you_.   If you wanted it
doing faster, you could have done it, but I still would have reviewed
it and still would have wanted it done this way. I was *quite* clear
about that since some time ago. If you'd simply done that rather than
keep pushing patches I'd already said I wasnt taking, it'd have taken
no time at all.   Yes, you'd have had to update the other 4 MFD
drivers using tmio-mmc yourself, and yes, thats a ball-ache. But since
_you_ want the change, you should do the work. Quite honestly, if
someone else does it for you, you should be grateful and not complain
about  how long it took.

>>> I don't think your MFD drivers do that,
>>
>> Actually they do provide provision for that by means of the enable /
>> disable hooks.
>>
>> if you unload the driver, the disable hook will get called and you can
>> power it off there.
>
> I can see a cell->enable() call in probe(), but I can't see any
> cell->disable() calls in the tmio-mmc driver. I sort of expected the
> remove() function in tmio-mmc and the probe() error handling code to
> do cell->disable(), but maybe I'm misunderstanding the way the MFD
> drivers work?

If the disable hook is not called then that may be a bug. Patches welcome.

My point still stands that the facility to power the block on / off as
and when the module is loaded is quite clearly available in the MFD
API.

>> Great - as soon as Philipp gets back to us with the ASIC3 update I'll
>> re-do my patchset to include ASIC3 - if the changes to tmio-mmc and
>> submitted seperately, it will break ASIC3 so that needs to go in the
>> same patchset. Philipp will be ready this weekend, not long to wait.
>
> Great, thanks a lot guys!

Hope the work is appreciated. Tracking down the init failure on my
(slow) netbook took the best part of 24 hours.

-- 
Ian Molton
Linux, Automotive, and other hacking:
http://www.mnementh.co.uk/

Re: TMIO MMC: full patchset.

[Magnus Damm]

Hi Ian,

On Sun, Oct 4, 2009 at 9:00 AM, Ian Molton <ian@mnementh.co.uk> wrote:
> 2009/10/3 Magnus Damm <magnus.damm@gmail.com>:
>> On Sat, Oct 3, 2009 at 3:14 AM, Ian Molton <ian@mnementh.co.uk> wrote:
>
>> 512kHz?
>
> That was an example. I suppose if your HCLK (the input or host clock,
> as named in the TMIO specs) never falls below double the maximum
> MMC/SD bus speed, then you will be ok.

That should be the case. Thanks for explaining.

> Do _any_ SH platforms actually share HCLK with other peripherals?

_All_ the SuperH Mobile SoCs that I've seen so far share the clock.
The clock for the SDHI block(s) can usually be stopped individually,
but the clock rate is shared with a bunch of other blocks. Just in the
sh7724 case there are 13 hardware blocks that share the same clock.
The clock that goes by HCLK in TMIO lingo. Most of these blocks
interface external hardware so changing the frequency will affect
other drivers.

>> So the shared SDHI input clock is usually set to around 100 MHz. If
>> I'm right then this clock is divided by the value in the
>> CTL_SD_CARD_CLK_CTL register which allows us to divide by a maximum of
>> 512.
>
> And in your case, a minimum of 2, IIRC.
>
>> Which would be enough to give us a wide range of valid MMC
>> frequencies.
>
> If nothing else shares the HCLK line, I'd recommend you set it such
> that one of the lower clock speeds is exactly 400kHz. I'd also leave
> 200 and 100kHz as options below that - some cards really do need to
> init that slowly.

I would if I could. =)

>> But yes, neither the tmio-mmc driver nor the mmc framework supports
>> updating the frequency while running. At least last time I checked.
>> That's usually the case when the frequency range is set once at
>> register time. =) It would be nice to add support for updating the
>> range though. I did just that for clockevents a few months ago.
>
> I doubt it'd be  much use, the TMIO MMC core doesnt use much power to
> preserve its state. on my platforms, the chip containing it is simply
> put to sleep and can remain so for days without draining the battery,
> and when it wakes up again the registers are as expected.

Well you measure days but I want to measure weeks. The SDHI block can
most likely be found in SoCs aimed for portable handsets, and for such
devices "days" are just not good enough.

The TMIO block itself may not use much power, but it's pretty common
to have power domains that cover a range of hardware blocks. So there
are complex dependencies. As an example, if we want to power off the
USB controller hardware block then we may have to power off the SDHI
block as well. That's just an example, but it shows how we may need
SDHI runtime pm to save and restore state even though the SDHI block
itself may not consume much power. The exact partitioning of the power
domains vary with processor model, but the concept is pretty much the
same for all SoCs. Regardless of vendor. Most vendors probably have
out-of-tree/private code for this already - the feature is most likely
just missing from the upstream driver.

> Thus far only one TMIO block has register save / restore and thats
> because its in a braindamaged hardware design that shuts off power to
> the ASIC when it sleeps.

Or maybe they have very coarse grained power domains. =) Some deeper
sleep modes on certain embedded SoCs turn off the power domains which
will give the same result as you are describing.

>> Well, _we_ can control the power domain where the SDHI block(s) is/are
>> located.
>
> Its probably not worth it, unless the SH design is merely a copy of
> the tmio design rather than a reimplementation. it barely uses any
> power when the clock is off anyway.

It's worth it, believe me.

>> And I suspect SoCs from other embedded processor vendors can
>> do the same. But to do that we need to have support for saving and
>> restoring the state in the driver.
>
> Given how low power the TMIO hardware is, and how fast it resumes,
> that seems counterproductive to me.

Well, ask anyone technically competent doing runtime power management
for embedded systems and they'll probably say the same thing. I can
introduce you to some ARM hackers fairly high in the food chain that
most likely will agree with me.

>> As you can see, quite a few of my drivers save and restore state
>> already. I'd be happy doing the technical work for the tmio-mmc
>> driver. Unless you want to. =)
>
> I'd like to see that this would result in significant power saving
> first. simply halting the block on my boards drops it to damn near
> zero consumption, and its still ready to go then.

Right, stopping the clock is great as a first step. This is the very
first step of power managment. As for the SDHI block - we're not even
fully there yet because we don't control what you call HCLK
dynamically. The next step after mucking around with more fine grained
clock stopping would be to implement save and restore.

>> But before I even think of extending the tmio-mmc driver _we_ _really_
>> need to work on improving the patch throughput a bit. I guess it's a
>> question of trust. From my point of view it is risky and frustrating
>> to do the work if we can't agree on things.
>
> Thats easily solved - agree on what needs to be done first.

Good. But I think we both knew what was needed earlier. From my
perspective, I needed to use the tmio-mmc driver on hardware that
wasn't supported yet. I sent a bunch of patches. Some got in, some
didn't which left my hardware unsupported.

Then there was this clock framework discussion. I can understand your
pain with the lack of arch-independent clock framework support. But on
my hardware the clock framework is working perfectly fine.

>> From your side I suspect
>> that you just don't want any breakage. I think you should trust me on
>> not wanting to break your driver and in the rare case I would then I
>> have time to spend on fixing it up.
>
> Well, if I had done that this time, we'd have a driver with two
> clocking methods in it now...

I appreciate your work, but seriously, who cares if you have two
clocking methods? This is just one driver of an entire system. I'm
sure there are more important matters in the world. That zoomed out
perspective aside, I do think your patch removing the second area is
much nicer.

So again, I really appreciate your help. But from my point of view you
could have just accepted my "make the second io area optional" patch
earlier and done this cleanup at any time later without stress. That
would have disconnected the two activities and we would have had
SuperH support half a year ago.

The stress you feel now is because the task is made more complicated
than it had to be IMO.

> I don't think my review has been overly slow, especially in the light
> of the fact that _I_ had to write the code you're now using, despite
> having no platforms that benefit from it at all. Perhaps if you'd done
> that the first time I suggetsed it, you wouldnt have had to wait for
> me to do it for you?

I didn't force you to write the code. I came to you with a rather
light weight implementation that you rejected.

You are right that you asked me to rewrite things. But you also asked
me to make the clock framework arch independent. Adding that
dependency didn't make any sense to me. This through the eyes of a guy
who had worked on implementing SuperH PM for half a year at that
point. So I do understand the internals of the clock framework,
believe me. And making it arch independent is not an easy task. It
involves changing code for a whole bunch of architectures. That's much
much much more complicated than any of this work so far.

From my point of view you kept on insisting on adding this clock
framework dependency, adding a virtual dependency and keeping the code
hostage. So I put the tmio-mmc driver on the list of drivers that I
need to deal with. Not technically - just communication wise I
suppose. I suppose meeting face to face would improve things. What do
you think?

> Yes, the above paragraph is meant to sound irritated. I've put quite
> some time into updating the driver _just for you_.  Time I could have
> spent doing other things, and time I'm sure Philipp could be better
> using rather than updating ASIC3 _just for you_.   If you wanted it
> doing faster, you could have done it, but I still would have reviewed
> it and still would have wanted it done this way. I was *quite* clear
> about that since some time ago. If you'd simply done that rather than
> keep pushing patches I'd already said I wasnt taking, it'd have taken
> no time at all.   Yes, you'd have had to update the other 4 MFD
> drivers using tmio-mmc yourself, and yes, thats a ball-ache. But since
> _you_ want the change, you should do the work. Quite honestly, if
> someone else does it for you, you should be grateful and not complain
> about  how long it took.

I _am_ really grateful, believe me. Thank you very much to both you and Philipp.

But from my point of view I didn't force you to do this work. You do
seem to think so. My original patch didn't require any MFD driver
changes. I understand and agree that breaking out the code for the
second io window results in cleaner code. I would have been happy to
do that work myself if I felt that the chance for a simple ACK would
have been higher. My memories is from last time just wild NAKing and
random clock framework rework requirements.

I'm not here to break your driver, you know. And I don't invent
requirements just for fun. I wanted to use the tmio-mmc driver because
I discovered that there is a lot of hardware out there that can make
use of it. The same thing with real requirements goes for Runtime PM -
I'm not pulling these requirements out of the dark - state save and
restore is really needed for proper power managment.

>> I can see a cell->enable() call in probe(), but I can't see any
>> cell->disable() calls in the tmio-mmc driver. I sort of expected the
>> remove() function in tmio-mmc and the probe() error handling code to
>> do cell->disable(), but maybe I'm misunderstanding the way the MFD
>> drivers work?
>
> If the disable hook is not called then that may be a bug. Patches welcome.

Cool. Where should it be called then? Both in the error case of
probe() and in remove()? I plan on doing nothing at this point, but if
I make a patch I'd like to avoidto break all the other MFD drivers.

> My point still stands that the facility to power the block on / off as
> and when the module is loaded is quite clearly available in the MFD
> API.

Yes, I agree it's available _when_the_module_is_loaded. But that's not
what I want. I'd like to see a more fine grained control. When the
system is idle it should have the option of being powered off.
Transparently.

This feature will require some serious rework of both the driver and
the MMC framework. And I suspect that you feel far from motivated by
such a change since it doesn't benefit any of your platforms.

>>> Great - as soon as Philipp gets back to us with the ASIC3 update I'll
>>> re-do my patchset to include ASIC3 - if the changes to tmio-mmc and
>>> submitted seperately, it will break ASIC3 so that needs to go in the
>>> same patchset. Philipp will be ready this weekend, not long to wait.
>>
>> Great, thanks a lot guys!
>
> Hope the work is appreciated. Tracking down the init failure on my
> (slow) netbook took the best part of 24 hours.

Ouch. Don't get me started on netbooks. =)

Your help is greatly appreciated.

Cheers,

/ magnus

Re: TMIO MMC: full patchset.

[Phil Blundell]

On Sun, 2009-10-04 at 16:51 +0100, Ian Molton wrote:
> Seriously, on e750 the power draw of the tc6393 in standby is so low
> my meter cant measure it.

What's the resolution of your meter? 

p.

Re: TMIO MMC: full patchset.

[Ian Molton]

1mA IIRC. Its never been calibrated though, its just a mid-priced multimeter.

2009/10/4 Phil Blundell <pb@handhelds.org>:
> On Sun, 2009-10-04 at 16:51 +0100, Ian Molton wrote:
>> Seriously, on e750 the power draw of the tc6393 in standby is so low
>> my meter cant measure it.
>
> What's the resolution of your meter?
>
> p.
>
>
>



-- 
Ian Molton
Linux, Automotive, and other hacking:
http://www.mnementh.co.uk/

Re: TMIO MMC: full patchset.

[Magnus Damm]

Hi Ian,

On Mon, Oct 5, 2009 at 12:51 AM, Ian Molton <ian@mnementh.co.uk> wrote:
> 2009/10/4 Magnus Damm <magnus.damm@gmail.com>:
>> The TMIO block itself may not use much power, but it's pretty common
>> to have power domains that cover a range of hardware blocks.
>
> Hm, that could get ugly. the MFD concept would tend to consider an MFD
> to control the power to its blocks. You want to use an MFD subdriver
> outside the power domain of the 'MFD' that contains it.

For SuperH SoCs most on-chip devices are registered as platform
devices, and for those devices architecture specific platform bus
extensions handle the runtime pm management. So the architecture code
gets to do whatever it needs to make sure the right low power states
can be entered during runtime. So it's all done without MFD today. Not
sure which way is best to deal with the MFD stuff at this point.

>>> Thus far only one TMIO block has register save / restore and thats
>>> because its in a braindamaged hardware design that shuts off power to
>>> the ASIC when it sleeps.
>>
>> Or maybe they have very coarse grained power domains. =) Some deeper
>> sleep modes on certain embedded SoCs turn off the power domains which
>> will give the same result as you are describing.
>
> No, its because that particular device is braindamaged. Normally, the
> tc6393xb MFD maintainns the state of all its blocks (even if the block
> itself is disabled in the MFD system config register) so you can
> suspend and resume the MFD without powering it off. One of sharps
> devices kills power to the ship too, which needed working around in
> the video driver.

So it's a matter of just stopping the clock or stopping clock + power off?

>>> Its probably not worth it, unless the SH design is merely a copy of
>>> the tmio design rather than a reimplementation. it barely uses any
>>> power when the clock is off anyway.
>>
>> It's worth it, believe me.
>
> Do you  have any figures (measure current draw) to back that up?

I've calculated the numbers below using values from the sh7722 data sheet:

Typical Processor Power Consumption [SH7722 @ 266/66/133 MHz]:
    No Power Management: ~0.6W
    "Sleep Mode": ~0.2W
    "Software Standby Mode": ~0.01W
    "U-Standby Mode": ~0.0001W

As for SDHI and tmio-mmc:
- To enter "Software Standby Mode" properly we need to have more fine
grained clock control.
- To enter "U-Standby Mode" we need runtime pm save/restore state support.

With the current MMC code the clock to the SDHI block will be enabled
as long as the driver is loaded. This blocks transition to deeper
sleep states. The best we can do with MMC enabled is basically "Sleep
Mode" which is far from good. I want to be able to enter the deepest
sleep state even though there's a SD Card plugged in and the driver is
loaded.

There are some real world numbers to back up some of the above in my
Runtime PM presentation from ELC2009:
http://www.celinuxforum.org/CelfPubWiki/ELC2009Presentations

> Seriously, on e750 the power draw of the tc6393 in standby is so low
> my meter cant measure it.

The Linux PM mini-summit notes from earlier this year contains a list
of recommended power meters:
http://lwn.net/Articles/345007/

>>> I'd like to see that this would result in significant power saving
>>> first. simply halting the block on my boards drops it to damn near
>>> zero consumption, and its still ready to go then.
>>
>> Right, stopping the clock is great as a first step. This is the very
>> first step of power managment. As for the SDHI block - we're not even
>> fully there yet because we don't control what you call HCLK
>> dynamically. The next step after mucking around with more fine grained
>> clock stopping would be to implement save and restore.
>
> We dont control HCLK because the documentation for the tmio-mmc block
> says one should use the clock gating that is provided by it. Also
> stopping HCLK on platforms with tc63**xb would probably hang the
> system as soon as any of the other peripherals in the MFD got
> accessed. the SDHI block has the same clock gating available, so I
> expect its best to use it. Further, there is nothing in the docs
> detailing how one should start / stop HCLK for tmio-mmc - it is
> assumed that the MFD containing it always has HCLK enabled prior to
> starting the MMC block, and stopped after disabling it (or on suspend
> / resume)

Right, but it would be nice with more fine grained control for
hardware that supports stopping the HCLK without affecting other
hardware blocks. I believe that such code can easily coexist with the
existing clock gating stragegy that you mention.

>>> Thats easily solved - agree on what needs to be done first.
>>
>> Good. But I think we both knew what was needed earlier. From my
>> perspective, I needed to use the tmio-mmc driver on hardware that
>> wasn't supported yet. I sent a bunch of patches. Some got in, some
>> didn't which left my hardware unsupported.
>
> Thats pretty normal patch submission procedure. Somee make it, some
> get asked for a rework.

Right, I agree with you.

>> Then there was this clock framework discussion. I can understand your
>> pain with the lack of arch-independent clock framework support. But on
>> my hardware the clock framework is working perfectly fine.
>
> Your hardware is one of (currently) 5 users of the driver (with
> another one being developed, although I havent heard from the guy
> recently).
>
> I conceeded that expecting the clock API to be reworked wasnt
> realistic at the time, so I rewrote the clock/power handling for
> tmio-mmc.

That was a wise decision IMO.

> I dont intend to modify the clock / power handling further at this
> point because it works well enough now.

And my numbers above? =)

> Fully dynamic clocking / power control will require some sort  of API
> to manage clocks and power. The dev pm stuff might do for power, but
> there is nothing suitable for use with clocks yet. Until there is, its
> going to be messy.

We use runtime pm to both control clocks and save/restore state. For
SuperH Mobile the power is controlled on a more global scale and can
only be turned off if clocks are off and state is saved.

> Might I suggest that it should be now, now that you can use the
> in-kernel tmio-mmc driver, that you, I, we, and anyone else
> interested, should look into actually making the clk API useful?

Good suggestion. Maybe I can fit that with some other tasks that I
have lined up.

> So you agree that the code is better for the extra time it took - then
> there should be no problem. Throwing code into the kernel in a big
> heap isnt the way to go. its fine for private trees etc. but it
> rapidly gets out of control if stuff just getsb accepted without being
> thought out properly. I spent a night removing and coalescing in one
> place, about 20 cookie-cutter functions from the ASoC code a while
> back, which, if people had done things properly to begin with, would
> never have existed in the first place. Its all wasted time. That code
> should have been spotted in someones personal tree long ago and been
> fixed before it ever got to mainline.

I agree that the code looks better than before. But I don't agree on
the idea of upstream always containing the correct long term solution.
You don't want things out of control, but the code also moves in some
direction all the time. Your words above "I dont intend to modify the
clock / power handling further at this point because it works well
enough now." makes me think that you believe that there is no need to
improve the driver. But from my point there is a whole lot to do with
the driver.

>> So again, I really appreciate your help. But from my point of view you
>> could have just accepted my "make the second io area optional" patch
>> earlier and done this cleanup at any time later without stress.
>
> IOW, you want your code rushed into the kernel and me to 'just fix
> things later' for you.
>
> Since you want the feature in question, why should not _you_ be the
> one tasked with doing it properly?

I wanted the code included so I can show people here that we have a
working upstream driver and a healthy relationship with the maintainer
so we can proceed investing time in it. So I would have done the work.

>> That
>> would have disconnected the two activities and we would have had
>> SuperH support half a year ago.
>
> Just because you gave up for several months after your first attempt
> doesnt mean the process had to take that long.

I could have pinged more frequently, yes, I agree.

>> I didn't force you to write the code. I came to you with a rather
>> light weight implementation that you rejected.
>
> Yes. Normally when that happens, you go away and rewrite the code, not
> expect the maintainer to do it for you.

I'm not expecting the maintainer to rewrite the code. I just want good
and clear communication so I can rewrite the code myself - just like
the one zillion other drivers I've spent time hacking on.

>> So I do understand the internals of the clock framework,
>> believe me.
>
> When you do things like pass struct clk pointers around in violation
> of the API, that makes it a bit more tricky to believe.

You can call it a violation if that makes you happy. I call it an
ARM-centric convention.

>> And making it arch independent is not an easy task. It
>> involves changing code for a whole bunch of architectures. That's much
>> much much more complicated than any of this work so far.
>
> Perhaps you and I should collaborate on that and actually get the job
> done, rather tham complain about it?

So what do you propose?

>> I _am_ really grateful, believe me. Thank you very much to both you and Philipp.
>>
>> But from my point of view I didn't force you to do this work.
>
> True. Perhaps I should have kept NAKing until you did it?

So if you NAKed and explained your goals clearly then I would have
rewritten the code. If it was a contant NAK-fest without any clear
technical reasoning I would have made sure the code got merged some
other way.

>> I wanted to use the tmio-mmc driver because
>> I discovered that there is a lot of hardware out there that can make
>> use of it. The same thing with real requirements goes for Runtime PM -
>> I'm not pulling these requirements out of the dark - state save and
>> restore is really needed for proper power managment.
>
> Well, its possible it might give a fractional improvement. Prove me
> wrong with numbers...

See above! =)

> I think that tmio-mmc should be left alone in favour of improving the
> clk API at this point, which would mean you could use your struct clk
> directly in the driver and we could look at putting finer grained PM
> in the MFD drivers too.

Sounds like a good plan. I don't have any hardware where the clock
framework isn't working as expected though. All boards that I have
come with SDHI integrated in the SoC, and in that case it's the same
clock framework as the main processor clock framework. That's slightly
different from the outside-the-soc MFD case.

>>> If the disable hook is not called then that may be a bug. Patches welcome.
>>
>> Cool. Where should it be called then? Both in the error case of
>> probe() and in remove()?
>
> I expect so (not looked at the code yet)
>
>> I plan on doing nothing at this point, but if
>> I make a patch I'd like to avoidto break all the other MFD drivers.
>
> Its the intended use of the hooks, so feel free to write a patch for it.

Ok, will put that on my TODO list.

>> Yes, I agree it's available _when_the_module_is_loaded. But that's not
>> what I want. I'd like to see a more fine grained control. When the
>> system is idle it should have the option of being powered off.
>> Transparently.
>
> I think it should be found out if its worth it first. Its a lot of
> code for little gain probably.

I'll let the numbers speak for themselves.

> That said the mmc block is fairly simple, it can be simply reset to
> restore state.

Nice.

> Of course there are other issues like how to suupport SD cards that
> remain poowered during suspend...

Right, suspend issues. I have to admit that I have very little
interest in suspend. My main goal is to get an idle running system as
close as possible to the power efficiency of a suspended system.

>> And I suspect that you feel far from motivated by
>> such a change since it doesn't benefit any of your platforms.
>
> Correct, however if you can back your claims with some current
> measurements with the block powered off  / unclocked vs unclocked vs
> running, and if the gain is worthwhile, I will consider such a patch.
>
>> Ouch. Don't get me started on netbooks. =)
>
> I like my netbook. I just wishh it was a 4 core ARM rather than X86...

I like the concept of small laptops, but they've been around forever
here in japan. I guess it's nice with the price drop, but the engineer
in me is not so keen on seeing old technology with poor performance
and crappy battery life being reused.

Cheers,

/ magnus