[PATCH 0/5] MMC: mmci: Provide bindings for Device Tree

[PATCH 0/5] MMC: mmci: Provide bindings for Device Tree

[Lee Jones]

This patch-set splits out ux500 and ARM specific variants from mmci
core code, applies generic bindings for use by all variants, then 
enables the ux500 variant for use with Device Tree.

 Documentation/devicetree/bindings/mmc/mmci.txt |   27 +++
 arch/arm/boot/dts/db8500.dtsi                  |    6 +-
 arch/arm/boot/dts/snowball.dts                 |   30 +++-
 arch/arm/mach-ux500/board-mop500.c             |    3 +-
 drivers/mmc/host/Makefile                      |    2 +-
 drivers/mmc/host/mmci-arm.c                    |   65 ++++++
 drivers/mmc/host/mmci-ux500.c                  |  260 ++++++++++++++++++++++++
 drivers/mmc/host/mmci.c                        |  194 +++++-------------
 drivers/mmc/host/mmci.h                        |   37 ++++
 9 files changed, 473 insertions(+), 151 deletions(-)

[PATCH 1/5] MMC: mmci: Seperate ux500 variants from generic code

[Lee Jones]

This is a step in the right direction for future Device
Tree support. It will allow variant specific attributes
to be collected from a Device Tree without overloading
the MMCI core. It will also provide additional future
variants a cleaner way to add support.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mmc/host/Makefile     |    2 +-
 drivers/mmc/host/mmci-ux500.c |   93 +++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/host/mmci.c       |   90 ++++------------------------------------
 drivers/mmc/host/mmci.h       |   37 ++++++++++++++++
 4 files changed, 139 insertions(+), 83 deletions(-)
 create mode 100644 drivers/mmc/host/mmci-ux500.c

diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 745f8fc..58e2bdc 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -2,7 +2,7 @@
 # Makefile for MMC/SD host controller drivers
 #
 
-obj-$(CONFIG_MMC_ARMMMCI)	+= mmci.o
+obj-$(CONFIG_MMC_ARMMMCI)	+= mmci.o mmci-ux500.o
 obj-$(CONFIG_MMC_PXA)		+= pxamci.o
 obj-$(CONFIG_MMC_IMX)		+= imxmmc.o
 obj-$(CONFIG_MMC_MXC)		+= mxcmmc.o
diff --git a/drivers/mmc/host/mmci-ux500.c b/drivers/mmc/host/mmci-ux500.c
new file mode 100644
index 0000000..82e60f9
--- /dev/null
+++ b/drivers/mmc/host/mmci-ux500.c
@@ -0,0 +1,93 @@
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+/* MMCIPOWER bits */
+#define MCI_DATA2DIREN		(1 << 2)
+#define MCI_CMDDIREN		(1 << 3)
+#define MCI_DATA0DIREN		(1 << 4)
+#define MCI_DATA31DIREN	(1 << 5)
+#define MCI_FBCLKEN		(1 << 7)
+
+static struct variant_data variant_u300 = {
+	.fifosize		= 16 * 4,
+	.fifohalfsize		= 8 * 4,
+	.clkreg_enable		= MCI_ST_U300_HWFCEN,
+	.datalength_bits	= 16,
+	.sdio			= true,
+};
+
+static struct variant_data variant_ux500 = {
+	.fifosize		= 30 * 4,
+	.fifohalfsize		= 8 * 4,
+	.clkreg			= MCI_CLK_ENABLE,
+	.clkreg_enable		= MCI_ST_UX500_HWFCEN,
+	.datalength_bits	= 24,
+	.sdio			= true,
+	.st_clkdiv		= true,
+};
+
+static struct variant_data variant_ux500v2 = {
+	.fifosize		= 30 * 4,
+	.fifohalfsize		= 8 * 4,
+	.clkreg			= MCI_CLK_ENABLE,
+	.clkreg_enable		= MCI_ST_UX500_HWFCEN,
+	.datalength_bits	= 24,
+	.sdio			= true,
+	.st_clkdiv		= true,
+	.blksz_datactrl16	= true,
+};
+
+static struct amba_id mmci_ux500_ids[] = {
+	/* ST Micro variants */
+	{
+		.id     = 0x00180180,
+		.mask   = 0x00ffffff,
+		.data	= &variant_u300,
+	},
+	{
+		.id     = 0x00280180,
+		.mask   = 0x00ffffff,
+		.data	= &variant_u300,
+	},
+	{
+		.id     = 0x00480180,
+		.mask   = 0xf0ffffff,
+		.data	= &variant_ux500,
+	},
+	{
+		.id     = 0x10480180,
+		.mask   = 0xf0ffffff,
+		.data	= &variant_ux500v2,
+	},
+	{ 0, 0 },
+};
+
+MODULE_DEVICE_TABLE(amba, mmci_ux500_ids);
+
+static struct amba_driver mmci_ux500_driver = {
+	.drv		= {
+		.name	= DRIVER_NAME,
+	},
+	.probe		= mmci_probe,
+	.remove		= __devexit_p(mmci_remove),
+	.suspend	= mmci_suspend,
+	.resume		= mmci_resume,
+	.id_table	= mmci_ux500_ids,
+};
+
+static int __init mmci_ux500_init(void)
+{
+	return amba_driver_register(&mmci_ux500_driver);
+}
+
+static void __exit mmci_ux500_exit(void)
+{
+	amba_driver_unregister(&mmci_ux500_driver);
+}
+
+module_init(mmci_ux500_init);
+module_exit(mmci_ux500_exit);
+module_param(fmax, uint, 0444);
+
+MODULE_DESCRIPTION("ARM PrimeCell PL180/181 Multimedia Card Interface driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 304f2f9..ff44586 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -37,34 +37,6 @@
 
 #include "mmci.h"
 
-#define DRIVER_NAME "mmci-pl18x"
-
-static unsigned int fmax = 515633;
-
-/**
- * struct variant_data - MMCI variant-specific quirks
- * @clkreg: default value for MCICLOCK register
- * @clkreg_enable: enable value for MMCICLOCK register
- * @datalength_bits: number of bits in the MMCIDATALENGTH register
- * @fifosize: number of bytes that can be written when MMCI_TXFIFOEMPTY
- *	      is asserted (likewise for RX)
- * @fifohalfsize: number of bytes that can be written when MCI_TXFIFOHALFEMPTY
- *		  is asserted (likewise for RX)
- * @sdio: variant supports SDIO
- * @st_clkdiv: true if using a ST-specific clock divider algorithm
- * @blksz_datactrl16: true if Block size is@b16..b30 position in datactrl register
- */
-struct variant_data {
-	unsigned int		clkreg;
-	unsigned int		clkreg_enable;
-	unsigned int		datalength_bits;
-	unsigned int		fifosize;
-	unsigned int		fifohalfsize;
-	bool			sdio;
-	bool			st_clkdiv;
-	bool			blksz_datactrl16;
-};
-
 static struct variant_data variant_arm = {
 	.fifosize		= 16 * 4,
 	.fifohalfsize		= 8 * 4,
@@ -77,35 +49,6 @@ static struct variant_data variant_arm_extended_fifo = {
 	.datalength_bits	= 16,
 };
 
-static struct variant_data variant_u300 = {
-	.fifosize		= 16 * 4,
-	.fifohalfsize		= 8 * 4,
-	.clkreg_enable		= MCI_ST_U300_HWFCEN,
-	.datalength_bits	= 16,
-	.sdio			= true,
-};
-
-static struct variant_data variant_ux500 = {
-	.fifosize		= 30 * 4,
-	.fifohalfsize		= 8 * 4,
-	.clkreg			= MCI_CLK_ENABLE,
-	.clkreg_enable		= MCI_ST_UX500_HWFCEN,
-	.datalength_bits	= 24,
-	.sdio			= true,
-	.st_clkdiv		= true,
-};
-
-static struct variant_data variant_ux500v2 = {
-	.fifosize		= 30 * 4,
-	.fifohalfsize		= 8 * 4,
-	.clkreg			= MCI_CLK_ENABLE,
-	.clkreg_enable		= MCI_ST_UX500_HWFCEN,
-	.datalength_bits	= 24,
-	.sdio			= true,
-	.st_clkdiv		= true,
-	.blksz_datactrl16	= true,
-};
-
 /*
  * This must be called with host->lock held
  */
@@ -1125,7 +1068,7 @@ static const struct mmc_host_ops mmci_ops = {
 	.get_cd		= mmci_get_cd,
 };
 
-static int __devinit mmci_probe(struct amba_device *dev,
+extern int __devinit mmci_probe(struct amba_device *dev,
 	const struct amba_id *id)
 {
 	struct mmci_platform_data *plat = dev->dev.platform_data;
@@ -1376,8 +1319,9 @@ static int __devinit mmci_probe(struct amba_device *dev,
  out:
 	return ret;
 }
+EXPORT_SYMBOL_GPL(mmci_probe);
 
-static int __devexit mmci_remove(struct amba_device *dev)
+extern int __devexit mmci_remove(struct amba_device *dev)
 {
 	struct mmc_host *mmc = amba_get_drvdata(dev);
 
@@ -1428,9 +1372,10 @@ static int __devexit mmci_remove(struct amba_device *dev)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(mmci_remove);
 
 #ifdef CONFIG_PM
-static int mmci_suspend(struct amba_device *dev, pm_message_t state)
+extern int mmci_suspend(struct amba_device *dev, pm_message_t state)
 {
 	struct mmc_host *mmc = amba_get_drvdata(dev);
 	int ret = 0;
@@ -1445,8 +1390,9 @@ static int mmci_suspend(struct amba_device *dev, pm_message_t state)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(mmci_suspend);
 
-static int mmci_resume(struct amba_device *dev)
+extern int mmci_resume(struct amba_device *dev)
 {
 	struct mmc_host *mmc = amba_get_drvdata(dev);
 	int ret = 0;
@@ -1461,6 +1407,7 @@ static int mmci_resume(struct amba_device *dev)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(mmci_resume);
 #else
 #define mmci_suspend	NULL
 #define mmci_resume	NULL
@@ -1482,27 +1429,6 @@ static struct amba_id mmci_ids[] = {
 		.mask	= 0x000fffff,
 		.data	= &variant_arm,
 	},
-	/* ST Micro variants */
-	{
-		.id     = 0x00180180,
-		.mask   = 0x00ffffff,
-		.data	= &variant_u300,
-	},
-	{
-		.id     = 0x00280180,
-		.mask   = 0x00ffffff,
-		.data	= &variant_u300,
-	},
-	{
-		.id     = 0x00480180,
-		.mask   = 0xf0ffffff,
-		.data	= &variant_ux500,
-	},
-	{
-		.id     = 0x10480180,
-		.mask   = 0xf0ffffff,
-		.data	= &variant_ux500v2,
-	},
 	{ 0, 0 },
 };
 
diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
index 79e4143..4ede20d 100644
--- a/drivers/mmc/host/mmci.h
+++ b/drivers/mmc/host/mmci.h
@@ -7,6 +7,12 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
+#include <linux/amba/mmci.h>
+#include <linux/scatterlist.h>
+#include <linux/amba/bus.h>
+
+#define DRIVER_NAME "mmci-pl18x"
+
 #define MMCIPOWER		0x000
 #define MCI_PWR_OFF		0x00
 #define MCI_PWR_UP		0x02
@@ -162,6 +168,8 @@
 
 #define NR_SG		16
 
+static unsigned int fmax = 515633;
+
 struct clk;
 struct variant_data;
 struct dma_chan;
@@ -218,3 +226,32 @@ struct mmci_host {
 #endif
 };
 
+/**
+ * struct variant_data - MMCI variant-specific quirks
+ * @clkreg: default value for MCICLOCK register
+ * @clkreg_enable: enable value for MMCICLOCK register
+ * @datalength_bits: number of bits in the MMCIDATALENGTH register
+ * @fifosize: number of bytes that can be written when MMCI_TXFIFOEMPTY
+ *	      is asserted (likewise for RX)
+ * @fifohalfsize: number of bytes that can be written when MCI_TXFIFOHALFEMPTY
+ *		  is asserted (likewise for RX)
+ * @sdio: variant supports SDIO
+ * @st_clkdiv: true if using a ST-specific clock divider algorithm
+ * @blksz_datactrl16: true if Block size is at b16..b30 position in datactrl register
+ */
+struct variant_data {
+	unsigned int		clkreg;
+	unsigned int		clkreg_enable;
+	unsigned int		datalength_bits;
+	unsigned int		fifosize;
+	unsigned int		fifohalfsize;
+	bool			sdio;
+	bool			st_clkdiv;
+	bool			blksz_datactrl16;
+};
+
+extern int __devinit mmci_probe(struct amba_device *dev,
+				const struct amba_id *id);
+extern int __devexit mmci_remove(struct amba_device *dev);
+extern int mmci_suspend(struct amba_device *dev, pm_message_t state);
+extern int mmci_resume(struct amba_device *dev);
-- 
1.7.5.4

[PATCH 2/5] MMC: mmci: Seperate ARM variants from generic code

[Lee Jones]

This is a step in the right direction for future Device
Tree support. It will allow variant specific attributes
to be collected from a Device Tree without overloading
the MMCI core. It will also provide additional future
variants a cleaner way to add support.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mmc/host/Makefile   |    2 +-
 drivers/mmc/host/mmci-arm.c |   65 +++++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/host/mmci.c     |   61 ----------------------------------------
 3 files changed, 66 insertions(+), 62 deletions(-)
 create mode 100644 drivers/mmc/host/mmci-arm.c

diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 58e2bdc..0981787 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -2,7 +2,7 @@
 # Makefile for MMC/SD host controller drivers
 #
 
-obj-$(CONFIG_MMC_ARMMMCI)	+= mmci.o mmci-ux500.o
+obj-$(CONFIG_MMC_ARMMMCI)	+= mmci.o mmci-ux500.o mmci-arm.o
 obj-$(CONFIG_MMC_PXA)		+= pxamci.o
 obj-$(CONFIG_MMC_IMX)		+= imxmmc.o
 obj-$(CONFIG_MMC_MXC)		+= mxcmmc.o
diff --git a/drivers/mmc/host/mmci-arm.c b/drivers/mmc/host/mmci-arm.c
new file mode 100644
index 0000000..be78060
--- /dev/null
+++ b/drivers/mmc/host/mmci-arm.c
@@ -0,0 +1,65 @@
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#include "mmci.h"
+
+static struct variant_data variant_arm = {
+	.fifosize		= 16 * 4,
+	.fifohalfsize		= 8 * 4,
+	.datalength_bits	= 16,
+};
+
+static struct variant_data variant_arm_extended_fifo = {
+	.fifosize		= 128 * 4,
+	.fifohalfsize		= 64 * 4,
+	.datalength_bits	= 16,
+};
+
+static struct amba_id mmci_arm_ids[] = {
+	{
+		.id	= 0x00041180,
+		.mask	= 0xff0fffff,
+		.data	= &variant_arm,
+	},
+	{
+		.id	= 0x01041180,
+		.mask	= 0xff0fffff,
+		.data	= &variant_arm_extended_fifo,
+	},
+	{
+		.id	= 0x00041181,
+		.mask	= 0x000fffff,
+		.data	= &variant_arm,
+	},
+	{ 0, 0 },
+};
+
+MODULE_DEVICE_TABLE(amba, mmci_arm_ids);
+
+static struct amba_driver mmci_arm_driver = {
+	.drv		= {
+		.name	= DRIVER_NAME,
+	},
+	.probe		= mmci_probe,
+	.remove		= __devexit_p(mmci_remove),
+	.suspend	= mmci_suspend,
+	.resume		= mmci_resume,
+	.id_table	= mmci_arm_ids,
+};
+
+static int __init mmci_arm_init(void)
+{
+	return amba_driver_register(&mmci_arm_driver);
+}
+
+static void __exit mmci_arm_exit(void)
+{
+	amba_driver_unregister(&mmci_arm_driver);
+}
+
+module_init(mmci_arm_init);
+module_exit(mmci_arm_exit);
+module_param(fmax, uint, 0444);
+
+MODULE_DESCRIPTION("ARM PrimeCell PL180/181 Multimedia Card Interface driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index ff44586..23b41a5 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -37,18 +37,6 @@
 
 #include "mmci.h"
 
-static struct variant_data variant_arm = {
-	.fifosize		= 16 * 4,
-	.fifohalfsize		= 8 * 4,
-	.datalength_bits	= 16,
-};
-
-static struct variant_data variant_arm_extended_fifo = {
-	.fifosize		= 128 * 4,
-	.fifohalfsize		= 64 * 4,
-	.datalength_bits	= 16,
-};
-
 /*
  * This must be called with host->lock held
  */
@@ -1412,52 +1400,3 @@ EXPORT_SYMBOL_GPL(mmci_resume);
 #define mmci_suspend	NULL
 #define mmci_resume	NULL
 #endif
-
-static struct amba_id mmci_ids[] = {
-	{
-		.id	= 0x00041180,
-		.mask	= 0xff0fffff,
-		.data	= &variant_arm,
-	},
-	{
-		.id	= 0x01041180,
-		.mask	= 0xff0fffff,
-		.data	= &variant_arm_extended_fifo,
-	},
-	{
-		.id	= 0x00041181,
-		.mask	= 0x000fffff,
-		.data	= &variant_arm,
-	},
-	{ 0, 0 },
-};
-
-MODULE_DEVICE_TABLE(amba, mmci_ids);
-
-static struct amba_driver mmci_driver = {
-	.drv		= {
-		.name	= DRIVER_NAME,
-	},
-	.probe		= mmci_probe,
-	.remove		= __devexit_p(mmci_remove),
-	.suspend	= mmci_suspend,
-	.resume		= mmci_resume,
-	.id_table	= mmci_ids,
-};
-
-static int __init mmci_init(void)
-{
-	return amba_driver_register(&mmci_driver);
-}
-
-static void __exit mmci_exit(void)
-{
-	amba_driver_unregister(&mmci_driver);
-}
-
-module_init(mmci_init);
-module_exit(mmci_exit);
-module_param(fmax, uint, 0444);
-
-MODULE_DESCRIPTION("ARM PrimeCell PL180/181 Multimedia Card Interface driver");
-MODULE_LICENSE("GPL");
-- 
1.7.5.4

[PATCH 2/5] MMC: mmci: Seperate ARM variants from generic code

[Russell King - ARM Linux]

On Wed, Mar 14, 2012 at 02:20:00PM +0000, Lee Jones wrote:
> This is a step in the right direction for future Device
> Tree support. It will allow variant specific attributes
> to be collected from a Device Tree without overloading
> the MMCI core. It will also provide additional future
> variants a cleaner way to add support.

NAK.  You can't have two driver structures with the same name.  The names
are sysfs object names.

[PATCH 2/5] MMC: mmci: Seperate ARM variants from generic code

[Lee Jones]

On 15/03/12 17:32, Russell King - ARM Linux wrote:
> On Wed, Mar 14, 2012 at 02:20:00PM +0000, Lee Jones wrote:
>> This is a step in the right direction for future Device
>> Tree support. It will allow variant specific attributes
>> to be collected from a Device Tree without overloading
>> the MMCI core. It will also provide additional future
>> variants a cleaner way to add support.
> 
> NAK.  You can't have two driver structures with the same name.  The names
> are sysfs object names.

Well spotted.

I've already changed this in v2. :)

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 2/5] MMC: mmci: Seperate ARM variants from generic code

[Russell King - ARM Linux]

On Thu, Mar 15, 2012 at 05:36:04PM +0000, Lee Jones wrote:
> On 15/03/12 17:32, Russell King - ARM Linux wrote:
> > On Wed, Mar 14, 2012 at 02:20:00PM +0000, Lee Jones wrote:
> >> This is a step in the right direction for future Device
> >> Tree support. It will allow variant specific attributes
> >> to be collected from a Device Tree without overloading
> >> the MMCI core. It will also provide additional future
> >> variants a cleaner way to add support.
> > 
> > NAK.  You can't have two driver structures with the same name.  The names
> > are sysfs object names.
> 
> Well spotted.
> 
> I've already changed this in v2. :)

I'm not in favour of splitting the two variants either btw.

[PATCH 2/5] MMC: mmci: Seperate ARM variants from generic code

[Arnd Bergmann]

On Thursday 15 March 2012, Russell King - ARM Linux wrote:
> On Thu, Mar 15, 2012 at 05:36:04PM +0000, Lee Jones wrote:
> > On 15/03/12 17:32, Russell King - ARM Linux wrote:
> > > On Wed, Mar 14, 2012 at 02:20:00PM +0000, Lee Jones wrote:
> > >> This is a step in the right direction for future Device
> > >> Tree support. It will allow variant specific attributes
> > >> to be collected from a Device Tree without overloading
> > >> the MMCI core. It will also provide additional future
> > >> variants a cleaner way to add support.
> > > 
> > > NAK.  You can't have two driver structures with the same name.  The names
> > > are sysfs object names.
> > 
> > Well spotted.
> > 
> > I've already changed this in v2. :)
> 
> I'm not in favour of splitting the two variants either btw.

Can you elaborate? I suggested the split in order to keep the ux500
specific parts local to one file. With the device tree conversion,
we really want to have them out of the platform code, but sticking them
into the main driver seems wrong, too.

I don't mind leaving the ARM and u300 specific bits in the main driver,
but I think having the ux500 parts separate does indeed help.

	Arnd

[PATCH 2/5] MMC: mmci: Seperate ARM variants from generic code

[Russell King - ARM Linux]

On Thu, Mar 15, 2012 at 05:46:56PM +0000, Arnd Bergmann wrote:
> Can you elaborate? I suggested the split in order to keep the ux500
> specific parts local to one file. With the device tree conversion,
> we really want to have them out of the platform code, but sticking them
> into the main driver seems wrong, too.

You're talking about the data structures which describe what quirks to
apply for the ux500 parts, rather than the actual quirks themselves.
That's not a particularly clever thing to do because it separates out
the selection of the works from the rest of the driver, which means
simply searching for the flags to find out what's applied for what has
to span several files.

[PATCH 2/5] MMC: mmci: Seperate ARM variants from generic code

[Arnd Bergmann]

On Thursday 15 March 2012, Russell King - ARM Linux wrote:
> On Thu, Mar 15, 2012 at 05:46:56PM +0000, Arnd Bergmann wrote:
> > Can you elaborate? I suggested the split in order to keep the ux500
> > specific parts local to one file. With the device tree conversion,
> > we really want to have them out of the platform code, but sticking them
> > into the main driver seems wrong, too.
> 
> You're talking about the data structures which describe what quirks to
> apply for the ux500 parts, rather than the actual quirks themselves.
> That's not a particularly clever thing to do because it separates out
> the selection of the works from the rest of the driver, which means
> simply searching for the flags to find out what's applied for what has
> to span several files.

Right, that is a disadvantage, but it's also how a lot of other
drivers work, in particular sdhci.

My reasoning is roughly this:

* we want to get rid of arch/arm/mach-ux500/board-mop500-sdi.c
* the sdi0_configure() and mop500_sdi0_vdd_handler() functions
  need to be moved somewhere in order to do that, so they should
  be with the driver.
* we don't want them in mmci.c, so we create a new file for these.
* in order to call do the setup from sdi0_configure right, it needs
  to be done in the probe() function
* if we want to have a separate probe function, we also need to
  have a separate amba_driver structure
* the variant_data in mmci.c belongs with the amba_id, so that
  also gets moved to the ux500 file.

If you have a better solution for one or more of these, I'm
all ears.

The alternatives that I can see are:
a) keep using auxdata to supply a platform_data pointer and
   do everything in the main driver. Problem: we want to
   avoid auxdata if possible
b) move the code from patch 4 into mmci.c using #ifdef.
   Problem: it's ugly code that has nothing to do with mmci
   in general.
c) use the regulator framework to do the voltage selection
   here, and have only generic code in mmci.c. This may
   be the best solution, but I have no idea if this is
   actually possible, or how to do it.

	arnd

[PATCH 2/5] MMC: mmci: Seperate ARM variants from generic code

[Russell King - ARM Linux]

On Thu, Mar 15, 2012 at 06:23:32PM +0000, Arnd Bergmann wrote:
> On Thursday 15 March 2012, Russell King - ARM Linux wrote:
> > On Thu, Mar 15, 2012 at 05:46:56PM +0000, Arnd Bergmann wrote:
> > > Can you elaborate? I suggested the split in order to keep the ux500
> > > specific parts local to one file. With the device tree conversion,
> > > we really want to have them out of the platform code, but sticking them
> > > into the main driver seems wrong, too.
> > 
> > You're talking about the data structures which describe what quirks to
> > apply for the ux500 parts, rather than the actual quirks themselves.
> > That's not a particularly clever thing to do because it separates out
> > the selection of the works from the rest of the driver, which means
> > simply searching for the flags to find out what's applied for what has
> > to span several files.
> 
> Right, that is a disadvantage, but it's also how a lot of other
> drivers work, in particular sdhci.
> 
> My reasoning is roughly this:
> 
> * we want to get rid of arch/arm/mach-ux500/board-mop500-sdi.c
> * the sdi0_configure() and mop500_sdi0_vdd_handler() functions
>   need to be moved somewhere in order to do that, so they should
>   be with the driver.

I don't believe they should.  MMCI _is_ the peripheral itself.  All the
stuff that's in board-mop500-sdi.c seems to be doing is configuration
type stuff for the SoC that its been integrated into.  Are you seriously
advocating having multiple drivers for essentially the same hardware?

> * we don't want them in mmci.c, so we create a new file for these.
> * in order to call do the setup from sdi0_configure right, it needs
>   to be done in the probe() function

I've always disagreed with this whole approach of "drivers should callback
into generic code to claim their GPIOs".  If the GPIO is dedicated by a
platform to a function, then that function is there independent of whether
the driver is present or not.  In many cases, it is totally _wrong_ to
leave a signal which should be driven unconfigured - if the board design
is such that its intended to be an output and there's no pull-up, it
could end up floating at half-supply which will eat power.

I have always been of the opinion that resources like GPIOs which are
under platform control should be requested and configured by the platform
and not by their respective drivers.

Have you noticed how much of the configuration is tweaked depending on
which init function is called?

> * if we want to have a separate probe function, we also need to
>   have a separate amba_driver structure
> * the variant_data in mmci.c belongs with the amba_id, so that
>   also gets moved to the ux500 file.
> 
> If you have a better solution for one or more of these, I'm
> all ears.

Are you really advocating having mmci_foo.c where foo is an ever
increasing number of SoCs with the MMCI integrated because you want
to integrate SoC specific details into the MMCI peripheral driver?

> The alternatives that I can see are:
> a) keep using auxdata to supply a platform_data pointer and
>    do everything in the main driver. Problem: we want to
>    avoid auxdata if possible
> b) move the code from patch 4 into mmci.c using #ifdef.
>    Problem: it's ugly code that has nothing to do with mmci
>    in general.
> c) use the regulator framework to do the voltage selection
>    here, and have only generic code in mmci.c. This may
>    be the best solution, but I have no idea if this is
>    actually possible, or how to do it.

Let's ignore the DMA stuff for the time being - that's a separate problem
which needs to be addressed once we work out how to deal with how DMA
engine stuff gets resolved with DT based systems.

The remainder of it (mmci_platform_data) looks like it can be easily
represented as DT properties, the exception to that is the ios_handler.
I think the right solution for that is to move to the regulator framework,
but allow the driver to continue having the ios callback if there is
no attached regulator.  It needs that because of the four integrated
regulator control bits (which are entirely undefined.)

We _do_ need a solution for GPIO-controlled voltage regulators, as these
are actually quite common - see the various soc-common using PCMCIA
drivers.

So I think actually a bit more thought is required here, and there's a
need to use some of the facilities such as regulator which are now present
that weren't when the driver was originally written some 9 years ago.

[PATCH 2/5] MMC: mmci: Seperate ARM variants from generic code

[Arnd Bergmann]

On Thursday 15 March 2012, Russell King - ARM Linux wrote:
> On Thu, Mar 15, 2012 at 06:23:32PM +0000, Arnd Bergmann wrote:
> > 
> > * we want to get rid of arch/arm/mach-ux500/board-mop500-sdi.c
> > * the sdi0_configure() and mop500_sdi0_vdd_handler() functions
> >   need to be moved somewhere in order to do that, so they should
> >   be with the driver.
> 
> I don't believe they should.  MMCI _is_ the peripheral itself.  All the
> stuff that's in board-mop500-sdi.c seems to be doing is configuration
> type stuff for the SoC that its been integrated into.  Are you seriously
> advocating having multiple drivers for essentially the same hardware?

It's basically the same we have for sdhci or ohci, or even the various
8250 versions. There is usually an identical core used in a number of
chips with slight variantions. In some cases it's better to describe
them as quirks of one driver, in other cases it's better abstract
them as separate driver files. The debate is just over which of these
two cases mmci fits into.

> > * if we want to have a separate probe function, we also need to
> >   have a separate amba_driver structure
> > * the variant_data in mmci.c belongs with the amba_id, so that
> >   also gets moved to the ux500 file.
> > 
> > If you have a better solution for one or more of these, I'm
> > all ears.
> 
> Are you really advocating having mmci_foo.c where foo is an ever
> increasing number of SoCs with the MMCI integrated because you want
> to integrate SoC specific details into the MMCI peripheral driver?

Only when they are significantly different from the common one.
According to your arguments above, the ux500 one might not be
different enough, but if something comes along that needs something
wildly different while still sharing a lot of the other code,
I would certainly use the same model.

> > The alternatives that I can see are:
> > a) keep using auxdata to supply a platform_data pointer and
> >    do everything in the main driver. Problem: we want to
> >    avoid auxdata if possible
> > b) move the code from patch 4 into mmci.c using #ifdef.
> >    Problem: it's ugly code that has nothing to do with mmci
> >    in general.
> > c) use the regulator framework to do the voltage selection
> >    here, and have only generic code in mmci.c. This may
> >    be the best solution, but I have no idea if this is
> >    actually possible, or how to do it.
> 
> Let's ignore the DMA stuff for the time being - that's a separate problem
> which needs to be addressed once we work out how to deal with how DMA
> engine stuff gets resolved with DT based systems.

Agreed.

> The remainder of it (mmci_platform_data) looks like it can be easily
> represented as DT properties, the exception to that is the ios_handler.
> I think the right solution for that is to move to the regulator framework,
> but allow the driver to continue having the ios callback if there is
> no attached regulator.  It needs that because of the four integrated
> regulator control bits (which are entirely undefined.)

Yes, that's what I was not sure about whether it's worth it. If we do this,
I absolutely agree that there is no need to split mmci.c.

> We _do_ need a solution for GPIO-controlled voltage regulators, as these
> are actually quite common - see the various soc-common using PCMCIA
> drivers.
> 
> So I think actually a bit more thought is required here, and there's a
> need to use some of the facilities such as regulator which are now present
> that weren't when the driver was originally written some 9 years ago.

Ok. Let's see what Mark has to say about this.

	Arnd

[PATCH 2/5] MMC: mmci: Seperate ARM variants from generic code

[Mark Brown]

On Fri, Mar 16, 2012 at 12:48:15PM +0000, Arnd Bergmann wrote:
> On Thursday 15 March 2012, Russell King - ARM Linux wrote:

> > We _do_ need a solution for GPIO-controlled voltage regulators, as these
> > are actually quite common - see the various soc-common using PCMCIA
> > drivers.

> > So I think actually a bit more thought is required here, and there's a
> > need to use some of the facilities such as regulator which are now present
> > that weren't when the driver was originally written some 9 years ago.

> Ok. Let's see what Mark has to say about this.

There's the gpio-regulator driver in mainline which should be able to
support a fairly wide range of these simple regulators, and also the
fixed voltage regulator for extremely simple cases.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120317/90d2d093/attachment-0001.sig>

[PATCH 2/5] MMC: mmci: Seperate ARM variants from generic code

[Lee Jones]

On 15/03/12 17:36, Lee Jones wrote:
> On 15/03/12 17:32, Russell King - ARM Linux wrote:
>> On Wed, Mar 14, 2012 at 02:20:00PM +0000, Lee Jones wrote:
>>> This is a step in the right direction for future Device
>>> Tree support. It will allow variant specific attributes
>>> to be collected from a Device Tree without overloading
>>> the MMCI core. It will also provide additional future
>>> variants a cleaner way to add support.
>>
>> NAK.  You can't have two driver structures with the same name.  The names
>> are sysfs object names.
> 
> Well spotted.
> 
> I've already changed this in v2. :)

In case you were interested, see:

http://git.linaro.org/gitweb?p=people/ljones/linux-3.0-ux500.git;a=shortlog;h=refs/heads/device-tree-snowball-mmci

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 3/5] MMC: mmci: Add generic Device Tree bindings to mmci core code

[Lee Jones]

This adds the necessary bindings for collection of shared
attributes used in the mmci driver.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mmc/host/mmci.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 23b41a5..9132ca8 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -30,6 +30,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/amba/mmci.h>
 #include <linux/pm_runtime.h>
+#include <linux/of_gpio.h>
 
 #include <asm/div64.h>
 #include <asm/io.h>
@@ -1056,11 +1057,47 @@ static const struct mmc_host_ops mmci_ops = {
 	.get_cd		= mmci_get_cd,
 };
 
+#ifdef CONFIG_OF
+static void mmci_dt_populate_generic_pdata(struct device_node *np,
+					struct mmci_platform_data *pdata)
+{
+	const void *prop;
+	int len;
+
+	of_property_read_u32(np, "wp-gpios", &pdata->gpio_wp);
+	if (!pdata->gpio_wp)
+		pdata->gpio_wp = -1;
+
+	of_property_read_u32(np, "cd-gpios", &pdata->gpio_cd);
+	if (!pdata->gpio_cd)
+		pdata->gpio_cd = -1;
+
+	if (of_get_property(np, "cd-invert", NULL))
+		pdata->cd_invert = true;
+	else
+		pdata->cd_invert = false;
+
+	of_property_read_u32(np, "clock_frequency", &pdata->f_max);
+	if (!pdata->f_max)
+		pr_warning("%s has no 'clock_frequency' property\n", np->full_name);
+
+	if (of_get_property(np, "mmc_cap_4_bit_data", NULL))
+		pdata->capabilities |= MMC_CAP_4_BIT_DATA;
+	if (of_get_property(np, "mmc_cap_8_bit_data", NULL))
+		pdata->capabilities |= MMC_CAP_8_BIT_DATA;
+	if (of_get_property(np, "mmc_cap_mmc_highspeed", NULL))
+		pdata->capabilities |= MMC_CAP_MMC_HIGHSPEED;
+	if (of_get_property(np, "mmc_cap_sd_highspeed", NULL))
+		pdata->capabilities |= MMC_CAP_SD_HIGHSPEED;
+}
+#endif
+
 extern int __devinit mmci_probe(struct amba_device *dev,
 	const struct amba_id *id)
 {
 	struct mmci_platform_data *plat = dev->dev.platform_data;
 	struct variant_data *variant = id->data;
+	struct device_node *np = dev->dev.of_node;
 	struct mmci_host *host;
 	struct mmc_host *mmc;
 	int ret;
@@ -1071,6 +1108,12 @@ extern int __devinit mmci_probe(struct amba_device *dev,
 		goto out;
 	}
 
+#ifdef CONFIG_OF
+	if (np)
+		/* GPIOs, card detect invert, max freq, capabilites etc ... */
+		mmci_dt_populate_generic_pdata(np, plat);
+#endif
+
 	ret = amba_request_regions(dev, DRIVER_NAME);
 	if (ret)
 		goto out;
-- 
1.7.5.4

[PATCH 3/5] MMC: mmci: Add generic Device Tree bindings to mmci core code

[Per Forlin]

On Wed, Mar 14, 2012 at 3:20 PM, Lee Jones <lee.jones@linaro.org> wrote:
> This adds the necessary bindings for collection of shared
> attributes used in the mmci driver.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
> ?drivers/mmc/host/mmci.c | ? 43 +++++++++++++++++++++++++++++++++++++++++++
> ?1 files changed, 43 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 23b41a5..9132ca8 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -30,6 +30,7 @@
> ?#include <linux/dma-mapping.h>
> ?#include <linux/amba/mmci.h>
> ?#include <linux/pm_runtime.h>
> +#include <linux/of_gpio.h>
>
> ?#include <asm/div64.h>
> ?#include <asm/io.h>
> @@ -1056,11 +1057,47 @@ static const struct mmc_host_ops mmci_ops = {
> ? ? ? ?.get_cd ? ? ? ? = mmci_get_cd,
> ?};
>
> +#ifdef CONFIG_OF
> +static void mmci_dt_populate_generic_pdata(struct device_node *np,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct mmci_platform_data *pdata)
> +{
> + ? ? ? const void *prop;
> + ? ? ? int len;
> +
> + ? ? ? of_property_read_u32(np, "wp-gpios", &pdata->gpio_wp);
> + ? ? ? if (!pdata->gpio_wp)
> + ? ? ? ? ? ? ? pdata->gpio_wp = -1;
> +
> + ? ? ? of_property_read_u32(np, "cd-gpios", &pdata->gpio_cd);
> + ? ? ? if (!pdata->gpio_cd)
> + ? ? ? ? ? ? ? pdata->gpio_cd = -1;
> +
> + ? ? ? if (of_get_property(np, "cd-invert", NULL))
> + ? ? ? ? ? ? ? pdata->cd_invert = true;
> + ? ? ? else
> + ? ? ? ? ? ? ? pdata->cd_invert = false;
> +
> + ? ? ? of_property_read_u32(np, "clock_frequency", &pdata->f_max);
> + ? ? ? if (!pdata->f_max)
> + ? ? ? ? ? ? ? pr_warning("%s has no 'clock_frequency' property\n", np->full_name);
> +
> + ? ? ? if (of_get_property(np, "mmc_cap_4_bit_data", NULL))
I have no previous experience with DT. Could you please bring some
light on this.
Is it really necessary to represent each bit in the CAP with a string?
To add CAP_ERASE for instance I need to change the code here and
update the DT, right?

BR
Per

[PATCH 3/5] MMC: mmci: Add generic Device Tree bindings to mmci core code

[Lee Jones]

On 15/03/12 15:12, Per Forlin wrote:
> On Wed, Mar 14, 2012 at 3:20 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> This adds the necessary bindings for collection of shared
>> attributes used in the mmci driver.
>>
>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>> ---
>>  drivers/mmc/host/mmci.c |   43 +++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 43 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>> index 23b41a5..9132ca8 100644
>> --- a/drivers/mmc/host/mmci.c
>> +++ b/drivers/mmc/host/mmci.c
>> @@ -30,6 +30,7 @@
>>  #include <linux/dma-mapping.h>
>>  #include <linux/amba/mmci.h>
>>  #include <linux/pm_runtime.h>
>> +#include <linux/of_gpio.h>
>>
>>  #include <asm/div64.h>
>>  #include <asm/io.h>
>> @@ -1056,11 +1057,47 @@ static const struct mmc_host_ops mmci_ops = {
>>        .get_cd         = mmci_get_cd,
>>  };
>>
>> +#ifdef CONFIG_OF
>> +static void mmci_dt_populate_generic_pdata(struct device_node *np,
>> +                                       struct mmci_platform_data *pdata)
>> +{
>> +       const void *prop;
>> +       int len;
>> +
>> +       of_property_read_u32(np, "wp-gpios", &pdata->gpio_wp);
>> +       if (!pdata->gpio_wp)
>> +               pdata->gpio_wp = -1;
>> +
>> +       of_property_read_u32(np, "cd-gpios", &pdata->gpio_cd);
>> +       if (!pdata->gpio_cd)
>> +               pdata->gpio_cd = -1;
>> +
>> +       if (of_get_property(np, "cd-invert", NULL))
>> +               pdata->cd_invert = true;
>> +       else
>> +               pdata->cd_invert = false;
>> +
>> +       of_property_read_u32(np, "clock_frequency", &pdata->f_max);
>> +       if (!pdata->f_max)
>> +               pr_warning("%s has no 'clock_frequency' property\n", np->full_name);
>> +
>> +       if (of_get_property(np, "mmc_cap_4_bit_data", NULL))
> I have no previous experience with DT. Could you please bring some
> light on this.
> Is it really necessary to represent each bit in the CAP with a string?

No, you can use a straight integer in the DT blob, but this makes it
much easier to read.

> To add CAP_ERASE for instance I need to change the code here and
> update the DT, right?

That's right, but no MMCI user makes use of it, so it would just be
unnecessary/unused code clogging up the core driver.

Kind regards,
Lee

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 4/5] MMC: mmci: Enable Device Tree support for ux500 variants

[Lee Jones]

Provide a means to collect attributes specific to ST-Ericsson's
ux500 variant series. This patch registers itself as the AMBA
driver to be called during the probe process. Once all
attributes and ux500 specifics are are collected the normal
mmci core probe is called.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/db8500.dtsi      |    6 +-
 arch/arm/boot/dts/snowball.dts     |   30 ++++++-
 arch/arm/mach-ux500/board-mop500.c |    3 +-
 drivers/mmc/host/mmci-ux500.c      |  171 +++++++++++++++++++++++++++++++++++-
 4 files changed, 201 insertions(+), 9 deletions(-)

diff --git a/arch/arm/boot/dts/db8500.dtsi b/arch/arm/boot/dts/db8500.dtsi
index a9866a3..fd3d818 100644
--- a/arch/arm/boot/dts/db8500.dtsi
+++ b/arch/arm/boot/dts/db8500.dtsi
@@ -238,13 +238,13 @@
 		sdi at 80126000 {
 			compatible = "arm,pl18x", "arm,primecell";
 			reg = <0x80126000 0x1000>;
-			interrupts = <60>;
+			interrupts = <0 60 0x4>;
 			status = "disabled";
 		};
 		sdi at 80118000 {
 			compatible = "arm,pl18x", "arm,primecell";
 			reg = <0x80118000 0x1000>;
-			interrupts = <50>;
+			interrupts = <0 50 0x4>;
 			status = "disabled";
 		};
 		sdi at 80005000 {
@@ -262,7 +262,7 @@
 		sdi at 80114000 {
 			compatible = "arm,pl18x", "arm,primecell";
 			reg = <0x80114000 0x1000>;
-			interrupts = <99>;
+			interrupts = <0 99 0x4>;
 			status = "disabled";
 		};
 		sdi at 80008000 {
diff --git a/arch/arm/boot/dts/snowball.dts b/arch/arm/boot/dts/snowball.dts
index 359c6d6..164200d 100644
--- a/arch/arm/boot/dts/snowball.dts
+++ b/arch/arm/boot/dts/snowball.dts
@@ -87,13 +87,37 @@
 			};
 		};
 
+		// External Micro SD slot
 		sdi at 80126000 {
-			status = "enabled";
-			cd-gpios = <&gpio6 26>;
+			arm,primecell-periphid = <0x10480180>;
+			#gpio-cells = <1>;
+			gpio-en   = <217>; // <&gpio6 25>;
+			gpio-vsel = <228>; // <&gpio7 4>;
+			cd-gpios  = <218>; // <&gpio6 26>;
+			cd-invert;
+
+			ocr_mask = <0x00020000>;
+			clock_frequency = <50000000>;
+			rx_src_dev_type = <29>;
+			tx_dst_dev_type = <29>;
+			mmc_cap_4_bit_data;
+			mmc_cap_mmc_highspeed;
+
+			status = "okay";
 		};
 
+		// On-board eMMC
 		sdi at 80114000 {
-			status = "enabled";
+			arm,primecell-periphid = <0x10480180>;
+			ocr_mask = <0x00020000>;
+		        clock_frequency = <50000000>;
+			mmc_cap_4_bit_data;
+			mmc_cap_8_bit_data;
+			mmc_cap_mmc_highspeed;
+			rx_src_dev_type = <42>;
+			tx_dst_dev_type = <42>;
+
+			status = "okay";
 		};
 
 		uart at 80120000 {
diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index d0799d5..60c23c5 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -750,6 +750,8 @@ struct of_dev_auxdata u8500_auxdata_lookup[] __initdata = {
 	OF_DEV_AUXDATA("arm,pl011", 0x80121000, "uart1", &uart1_plat),
 	OF_DEV_AUXDATA("arm,pl011", 0x80007000, "uart2", &uart2_plat),
 	OF_DEV_AUXDATA("arm,pl022", 0x80002000, "ssp0",  &ssp0_plat),
+	OF_DEV_AUXDATA("arm,pl18x", 0x80126000, "sdi0", NULL),
+	OF_DEV_AUXDATA("arm,pl18x", 0x80114000, "sdi4", NULL),
 	{},
 };
 
@@ -789,7 +791,6 @@ static void __init u8500_init_machine(void)
 		platform_add_devices(snowball_platform_devs,
 				ARRAY_SIZE(snowball_platform_devs));
 
-		snowball_sdi_init(parent);
 	} else if (of_machine_is_compatible("st-ericsson,hrefv60+")) {
 		/*
 		 * The HREFv60 board removed a GPIO expander and routed
diff --git a/drivers/mmc/host/mmci-ux500.c b/drivers/mmc/host/mmci-ux500.c
index 82e60f9..75435e4 100644
--- a/drivers/mmc/host/mmci-ux500.c
+++ b/drivers/mmc/host/mmci-ux500.c
@@ -1,5 +1,11 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/of_gpio.h>
+
+#include <plat/ste_dma40.h>
+
+#include "mmci.h"
 
 /* MMCIPOWER bits */
 #define MCI_DATA2DIREN		(1 << 2)
@@ -37,6 +43,167 @@ static struct variant_data variant_ux500v2 = {
 	.blksz_datactrl16	= true,
 };
 
+struct mmci_ux500_platform_data {
+	struct mmci_platform_data pdata;
+	int gpio_en;
+	int gpio_vsel;
+};
+
+static u32 mmci_ux500_vdd_handler(struct device *dev, unsigned int vdd,
+				   unsigned char power_mode)
+{
+	struct mmci_ux500_platform_data *pdata =
+		container_of(dev->platform_data, struct mmci_ux500_platform_data, pdata);
+
+	switch (power_mode) {
+	case MMC_POWER_UP:
+	case MMC_POWER_ON:
+		/*
+		 * Level shifter voltage should depend on vdd to when deciding
+		 * on either 1.8V or 2.9V. Once the decision has been made the
+		 * level shifter must be disabled and re-enabled with a changed
+		 * select signal in order to switch the voltage. Since there is
+		 * no framework support yet for indicating 1.8V in vdd, use the
+		 * default 2.9V.
+		 */
+		if (pdata->gpio_vsel)
+			gpio_direction_output(pdata->gpio_vsel, 0);
+		if (pdata->gpio_en)
+			gpio_direction_output(pdata->gpio_en, 1);
+		break;
+	case MMC_POWER_OFF:
+		if (pdata->gpio_vsel)
+			gpio_direction_output(pdata->gpio_vsel, 0);
+		if (pdata->gpio_en)
+			gpio_direction_output(pdata->gpio_en, 0);
+		break;
+	}
+
+	return MCI_FBCLKEN | MCI_CMDDIREN | MCI_DATA0DIREN |
+	       MCI_DATA2DIREN | MCI_DATA31DIREN;
+}
+
+static int __devinit mmci_ux500_probe(struct amba_device *dev,
+				const struct amba_id *id)
+{
+	struct mmci_ux500_platform_data *ux500_pdata;
+	struct mmci_platform_data *pdata = dev->dev.platform_data;
+	struct device_node *np = dev->dev.of_node;
+	struct stedma40_chan_cfg *dma_tx_param;
+	struct stedma40_chan_cfg *dma_rx_param;
+	int val, ret = -EINVAL;
+
+	/* Check if we have already been populated by platform code. */
+	if (pdata)
+		return mmci_probe(dev, id);
+
+#ifdef CONFIG_OF
+	if (!np) {
+		pr_warning("No platform data or Device Tree found\n");
+		return -EINVAL;
+	}
+
+	ux500_pdata = kzalloc(sizeof(struct mmci_ux500_platform_data), GFP_KERNEL);
+	if (!ux500_pdata)
+		return -ENOMEM;
+
+	dma_tx_param = kzalloc(sizeof(struct stedma40_chan_cfg), GFP_KERNEL);
+	if (!dma_tx_param) {
+		ret = -ENOMEM;
+		goto out1;
+	}
+
+	dma_rx_param = kzalloc(sizeof(struct stedma40_chan_cfg), GFP_KERNEL);
+	if (!dma_rx_param) {
+		ret = -ENOMEM;
+		goto out2;
+	}
+
+	/* ST-Ericsson/Micro specific DMA attributes. */
+	ux500_pdata->gpio_vsel = 0;
+	ux500_pdata->gpio_en = 0;
+
+	of_property_read_u32(np, "gpio-en", &ux500_pdata->gpio_en);
+	of_property_read_u32(np, "gpio-vsel", &ux500_pdata->gpio_vsel);
+
+	if (ux500_pdata->gpio_vsel && ux500_pdata->gpio_en) {
+		ret  = gpio_request(ux500_pdata->gpio_en, "level shifter enable");
+		if (ret) {
+			pr_warning("GPIO request for level shifter enable failed.\n");
+			goto out3;
+		}
+
+		ret = gpio_request(ux500_pdata->gpio_vsel, "level shifter 1v8-3v select");
+		if (ret) {
+			pr_warning("GPIO request for level shifter select failed.\n");
+			goto out3;
+		}
+
+		/* Select the default 2.9V and enable level shifter */
+		gpio_direction_output(ux500_pdata->gpio_vsel, 0);
+		gpio_direction_output(ux500_pdata->gpio_en, 1);
+
+		ux500_pdata->pdata.vdd_handler = mmci_ux500_vdd_handler;
+	}
+
+	ux500_pdata->pdata.dma_filter  = stedma40_filter;
+
+	/* Mode. */
+	dma_tx_param->mode = STEDMA40_MODE_LOGICAL;
+	dma_rx_param->mode = STEDMA40_MODE_LOGICAL;
+
+	/* Direction. */
+	dma_tx_param->dir = STEDMA40_MEM_TO_PERIPH;
+	dma_rx_param->dir = STEDMA40_PERIPH_TO_MEM;
+
+	/* Device type. */
+	dma_tx_param->src_dev_type = STEDMA40_DEV_DST_MEMORY;
+
+	if (!of_property_read_u32(np, "tx_dst_dev_type", &val))
+		dma_tx_param->dst_dev_type = val;
+
+	dma_rx_param->dst_dev_type = STEDMA40_DEV_DST_MEMORY;
+
+	if (!of_property_read_u32(np, "rx_src_dev_type", &val))
+		dma_rx_param->src_dev_type = val;
+
+	/* Data width. */
+	dma_tx_param->src_info.data_width = STEDMA40_WORD_WIDTH;
+	dma_tx_param->dst_info.data_width = STEDMA40_WORD_WIDTH;
+	dma_rx_param->src_info.data_width = STEDMA40_WORD_WIDTH;
+	dma_rx_param->dst_info.data_width = STEDMA40_WORD_WIDTH;
+
+	ux500_pdata->pdata.dma_tx_param = dma_tx_param;
+	ux500_pdata->pdata.dma_rx_param = dma_rx_param;
+
+	dev->dev.platform_data = &ux500_pdata->pdata;
+
+	return mmci_probe(dev, id);
+
+out3:
+	kfree(dma_rx_param);
+out2:
+	kfree(dma_tx_param);
+out1:
+	kfree(ux500_pdata);
+#endif /* CONFIG_OF */
+
+	return ret;
+}
+
+static int __devexit mmci_ux500_remove(struct amba_device *dev)
+{
+	struct mmci_ux500_platform_data *pdata =
+		container_of(dev->dev.platform_data, struct mmci_ux500_platform_data, pdata);
+
+	kfree(pdata->pdata.dma_tx_param);
+	kfree(pdata->pdata.dma_tx_param);
+
+	mmci_remove(dev);
+
+	return 0;
+}
+
 static struct amba_id mmci_ux500_ids[] = {
 	/* ST Micro variants */
 	{
@@ -68,8 +235,8 @@ static struct amba_driver mmci_ux500_driver = {
 	.drv		= {
 		.name	= DRIVER_NAME,
 	},
-	.probe		= mmci_probe,
-	.remove		= __devexit_p(mmci_remove),
+	.probe		= mmci_ux500_probe,
+	.remove		= __devexit_p(mmci_ux500_remove),
 	.suspend	= mmci_suspend,
 	.resume		= mmci_resume,
 	.id_table	= mmci_ux500_ids,
-- 
1.7.5.4

[PATCH 5/5] MMC: mmci: Add required documentation for Device Tree bindings

[Lee Jones]

This provides full documentation detailing each compulsory,
optional and device (variant) specific properties available
for use within the MMCI Device Tree.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 Documentation/devicetree/bindings/mmc/mmci.txt |   27 ++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mmc/mmci.txt

diff --git a/Documentation/devicetree/bindings/mmc/mmci.txt b/Documentation/devicetree/bindings/mmc/mmci.txt
new file mode 100644
index 0000000..946f080
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/mmci.txt
@@ -0,0 +1,27 @@
+* ARM PrimeCell MultiMedia Card Interface (MMCI) PL180/1
+
+The ARM PrimeCell MMCI PL180 and PL181 provides and interface for 
+reading and writing to MultiMedia and SD cards alike.
+
+Required properties:
+- compatible             : contains "arm,pl18x", "arm,primecell".
+- reg                    : contains pl18x registers and length.
+- interrupts             : contains the device IRQ(s).
+- arm,primecell-periphid : contains the PrimeCell Peripheral ID.
+
+Optional properties:
+- ocr_mask               : contains the regulator oscillator mask
+- wp-gpios               : contains any write protect (ro) gpios
+- cd-gpios               : contains any card detection gpios
+- cd-invert              : indicates whether the cd gpio is inverted 
+- clock_frequency        : contains the maximum operating frequency
+- mmc_cap_4_bit_data     : indicates whether device is 4 bit capable
+- mmc_cap_8_bit_data     : indicates whether device is 8 bit capable
+- mmc_cap_mmc_highspeed  : indicates whether MMC is high speed capable
+- mmc_cap_sd_highspeed   : indicates whether SD is high speed capable
+
+ux500 variant specific properties:
+- gpio-en                : contains the level-shifter enable gpio
+- gpio-vsel              : contains the level-shifter voltage select gpio
+- tx_dst_dev_type        : contains the tx destination device (DMA) 
+- rx_src_dev_type        : contains the rx source device (DMA)
-- 
1.7.5.4

[PATCH 5/5] MMC: mmci: Add required documentation for Device Tree bindings

[Russell King - ARM Linux]

On Wed, Mar 14, 2012 at 02:20:03PM +0000, Lee Jones wrote:
> +ux500 variant specific properties:
> +- gpio-en                : contains the level-shifter enable gpio
> +- gpio-vsel              : contains the level-shifter voltage select gpio
> +- tx_dst_dev_type        : contains the tx destination device (DMA) 
> +- rx_src_dev_type        : contains the rx source device (DMA)

So what about the DMA support being proposed elsewhere?

This sounds like we're inventing one DMA solution for one bit of the tree
and another DMA solution for another bit.  This is madness.

Note that ARM boards like Versatile also have DMA support, even though
most of those platforms are diabolically crippled to the point of it being
useless.  I believe it works on a few select boards...

[PATCH 5/5] MMC: mmci: Add required documentation for Device Tree bindings

[Arnd Bergmann]

On Thursday 15 March 2012, Russell King - ARM Linux wrote:
> 
> On Wed, Mar 14, 2012 at 02:20:03PM +0000, Lee Jones wrote:
> > +ux500 variant specific properties:
> > +- gpio-en                : contains the level-shifter enable gpio
> > +- gpio-vsel              : contains the level-shifter voltage select gpio
> > +- tx_dst_dev_type        : contains the tx destination device (DMA) 
> > +- rx_src_dev_type        : contains the rx source device (DMA)
> 
> So what about the DMA support being proposed elsewhere?
> 
> This sounds like we're inventing one DMA solution for one bit of the tree
> and another DMA solution for another bit.  This is madness.

Agreed. I originally made a similar comment for an early version but had not
noticed that it made it into this one. This should be using the proper DMA
bindings, or just auxdata while we're not entirely there yet.

	Arnd

[PATCH 5/5] MMC: mmci: Add required documentation for Device Tree bindings

[Russell King - ARM Linux]

On Thu, Mar 15, 2012 at 05:49:07PM +0000, Arnd Bergmann wrote:
> On Thursday 15 March 2012, Russell King - ARM Linux wrote:
> > 
> > On Wed, Mar 14, 2012 at 02:20:03PM +0000, Lee Jones wrote:
> > > +ux500 variant specific properties:
> > > +- gpio-en                : contains the level-shifter enable gpio
> > > +- gpio-vsel              : contains the level-shifter voltage select gpio
> > > +- tx_dst_dev_type        : contains the tx destination device (DMA) 
> > > +- rx_src_dev_type        : contains the rx source device (DMA)
> > 
> > So what about the DMA support being proposed elsewhere?
> > 
> > This sounds like we're inventing one DMA solution for one bit of the tree
> > and another DMA solution for another bit.  This is madness.
> 
> Agreed. I originally made a similar comment for an early version but had not
> noticed that it made it into this one. This should be using the proper DMA
> bindings, or just auxdata while we're not entirely there yet.

It's something that I'm also concerned with the main OF DMA patches.  It's
not at all clear what we want from DMA bindings yet because we've hardly
started converting from platform specific DMA APIs to the DMA engine based
APIs.

The way DMA channels are selected by DMA APIs is significantly different
from the DMA engine based API, and I remain worried that we're locking in
to a platform specific DMA API method.

I think the DMA parts should be left alone until there has been much more
of an effort to convert to DMA engine based APIs for the various platforms.

Note: OMAP DMA is something I've been asked to take over, and is something
I've been looking at over the last couple of weeks so far.

[PATCH 5/5] MMC: mmci: Add required documentation for Device Tree bindings

[Arnd Bergmann]

On Wednesday 14 March 2012, Lee Jones wrote:

Some more comments about the individual properties

> +Required properties:
> +- compatible             : contains "arm,pl18x", "arm,primecell".

Better use the exact number for 'x' here.

> +- reg                    : contains pl18x registers and length.
> +- interrupts             : contains the device IRQ(s).

You should document what the first and second interrupts are,
respectively, if there is more than one.

> +- arm,primecell-periphid : contains the PrimeCell Peripheral ID.

This one should be optional.

> +
> +Optional properties:
> +- ocr_mask               : contains the regulator oscillator mask

Not sure about this one. If it's for the regulators, we should be
using the regulator bindings.

> +- wp-gpios               : contains any write protect (ro) gpios
> +- cd-gpios               : contains any card detection gpios
> +- cd-invert              : indicates whether the cd gpio is inverted 
> +- clock_frequency        : contains the maximum operating frequency

The name should be 'clock-frequency', not 'clock_frequency'.

> +- mmc_cap_4_bit_data     : indicates whether device is 4 bit capable
> +- mmc_cap_8_bit_data     : indicates whether device is 8 bit capable
> +- mmc_cap_mmc_highspeed  : indicates whether MMC is high speed capable
> +- mmc_cap_sd_highspeed   : indicates whether SD is high speed capable

I would strongly recommend using the same naming as sdhci here. I think
tegra has set a precedent.

> +ux500 variant specific properties:
> +- gpio-en                : contains the level-shifter enable gpio
> +- gpio-vsel              : contains the level-shifter voltage select gpio

I think these need to be 'en-gpios' and 'vsel-gpios', in order for the
gpio binding to work.

> +- tx_dst_dev_type        : contains the tx destination device (DMA) 
> +- rx_src_dev_type        : contains the rx source device (DMA)

As Russell commented already, these should not be here.

	Arnd

[PATCH 5/5] MMC: mmci: Add required documentation for Device Tree bindings

[Russell King - ARM Linux]

On Thu, Mar 15, 2012 at 05:53:25PM +0000, Arnd Bergmann wrote:
> On Wednesday 14 March 2012, Lee Jones wrote:
> > +
> > +Optional properties:
> > +- ocr_mask               : contains the regulator oscillator mask
> 
> Not sure about this one. If it's for the regulators, we should be
> using the regulator bindings.

It's also nothing to do with oscillators.

[PATCH 0/5] MMC: mmci: Provide bindings for Device Tree

[Per Forlin]

Hi,

I would like to see what the minimal required change is to support DT
for mmci without factorization.
1. Minimal change in mmci.
2. Add mmci_dt.c which contains the DT-populate code.

The factorization could be done as step 2 I think.

What do you say?

BR
Per

On Wed, Mar 14, 2012 at 3:19 PM, Lee Jones <lee.jones@linaro.org> wrote:
> This patch-set splits out ux500 and ARM specific variants from mmci
> core code, applies generic bindings for use by all variants, then
> enables the ux500 variant for use with Device Tree.
>
> ?Documentation/devicetree/bindings/mmc/mmci.txt | ? 27 +++
> ?arch/arm/boot/dts/db8500.dtsi ? ? ? ? ? ? ? ? ?| ? ?6 +-
> ?arch/arm/boot/dts/snowball.dts ? ? ? ? ? ? ? ? | ? 30 +++-
> ?arch/arm/mach-ux500/board-mop500.c ? ? ? ? ? ? | ? ?3 +-
> ?drivers/mmc/host/Makefile ? ? ? ? ? ? ? ? ? ? ?| ? ?2 +-
> ?drivers/mmc/host/mmci-arm.c ? ? ? ? ? ? ? ? ? ?| ? 65 ++++++
> ?drivers/mmc/host/mmci-ux500.c ? ? ? ? ? ? ? ? ?| ?260 ++++++++++++++++++++++++
> ?drivers/mmc/host/mmci.c ? ? ? ? ? ? ? ? ? ? ? ?| ?194 +++++-------------
> ?drivers/mmc/host/mmci.h ? ? ? ? ? ? ? ? ? ? ? ?| ? 37 ++++
> ?9 files changed, 473 insertions(+), 151 deletions(-)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html

[PATCH 0/5] MMC: mmci: Provide bindings for Device Tree

[Lee Jones]

Hi Per,

> I would like to see what the minimal required change is to support DT
> for mmci without factorization.
> 1. Minimal change in mmci.
> 2. Add mmci_dt.c which contains the DT-populate code.
> 
> The factorization could be done as step 2 I think.
> 
> What do you say?

I'm wondering what the difference is as the work has already been done.

It was Arnd's suggestion to separate out the two types of variants, and
I'm quite fond of the new (fully featured) layout.

Kind regards,
Lee

> On Wed, Mar 14, 2012 at 3:19 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> This patch-set splits out ux500 and ARM specific variants from mmci
>> core code, applies generic bindings for use by all variants, then
>> enables the ux500 variant for use with Device Tree.
>>
>>  Documentation/devicetree/bindings/mmc/mmci.txt |   27 +++
>>  arch/arm/boot/dts/db8500.dtsi                  |    6 +-
>>  arch/arm/boot/dts/snowball.dts                 |   30 +++-
>>  arch/arm/mach-ux500/board-mop500.c             |    3 +-
>>  drivers/mmc/host/Makefile                      |    2 +-
>>  drivers/mmc/host/mmci-arm.c                    |   65 ++++++
>>  drivers/mmc/host/mmci-ux500.c                  |  260 ++++++++++++++++++++++++
>>  drivers/mmc/host/mmci.c                        |  194 +++++-------------
>>  drivers/mmc/host/mmci.h                        |   37 ++++
>>  9 files changed, 473 insertions(+), 151 deletions(-)
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 0/5] MMC: mmci: Provide bindings for Device Tree

[Arnd Bergmann]

On Thursday 15 March 2012, Lee Jones wrote:
> > I would like to see what the minimal required change is to support DT
> > for mmci without factorization.
> > 1. Minimal change in mmci.
> > 2. Add mmci_dt.c which contains the DT-populate code.
> > 
> > The factorization could be done as step 2 I think.
> > 
> > What do you say?
> 
> I'm wondering what the difference is as the work has already been done.
> 
> It was Arnd's suggestion to separate out the two types of variants, and
> I'm quite fond of the new (fully featured) layout.

Right, I usually prefer cleanups or other refactoring to be done first, and
then features added on top.

You could in theory add have just patches 3/4/5 all applied without
the refactoring, but that I would be worried that this causes dependencies
between the mmci driver and ux500 specific functionality like the
stedma40_filter function.

	Arnd

[PATCH 0/5] MMC: mmci: Provide bindings for Device Tree

[Lee Jones]

On 15/03/12 15:32, Arnd Bergmann wrote:
> On Thursday 15 March 2012, Lee Jones wrote:
>>> I would like to see what the minimal required change is to support DT
>>> for mmci without factorization.
>>> 1. Minimal change in mmci.
>>> 2. Add mmci_dt.c which contains the DT-populate code.
>>>
>>> The factorization could be done as step 2 I think.
>>>
>>> What do you say?
>>
>> I'm wondering what the difference is as the work has already been done.
>>
>> It was Arnd's suggestion to separate out the two types of variants, and
>> I'm quite fond of the new (fully featured) layout.
> 
> Right, I usually prefer cleanups or other refactoring to be done first, and
> then features added on top.
> 
> You could in theory add have just patches 3/4/5 all applied without
> the refactoring, but that I would be worried that this causes dependencies
> between the mmci driver and ux500 specific functionality like the
> stedma40_filter function.

It's a possibility that we may have run into that even with this new
layout. Remember you suggested that the u300 variant may be better off
going into the ARM variant's file, but I wasn't quite sure, as it was an
ST product. Well the afore mentioned function may still cause us some
trouble. I am in the mists of some regression testing now. I'll report
back when I know more.

Kind regards,
Lee


-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 0/5] MMC: mmci: Provide bindings for Device Tree

[Per Forlin]

On Thu, Mar 15, 2012 at 4:32 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 15 March 2012, Lee Jones wrote:
>> > I would like to see what the minimal required change is to support DT
>> > for mmci without factorization.
>> > 1. Minimal change in mmci.
>> > 2. Add mmci_dt.c which contains the DT-populate code.
>> >
>> > The factorization could be done as step 2 I think.
>> >
>> > What do you say?
>>
>> I'm wondering what the difference is as the work has already been done.
>>
>> It was Arnd's suggestion to separate out the two types of variants, and
>> I'm quite fond of the new (fully featured) layout.
>
> Right, I usually prefer cleanups or other refactoring to be done first, and
> then features added on top.
>
> You could in theory add have just patches 3/4/5 all applied without
> the refactoring, but that I would be worried that this causes dependencies
> between the mmci driver and ux500 specific functionality like the
> stedma40_filter function.
>
About DT
I think I need to catch up on the DT-design a bit. I will try to catch
up on the DT-implementation and maybe then the refactorization will
make sense to me :)

Board specific data in mmci-driver
The stedma40 filter function is not really specific for ux500. ux500
use stedma40 but it should be possible to replace this DMA.IP with
some other DMA-controller. This is board specific configuration. You
should not need to change the mmci-driver just because the dma-driver
has changed, right?
Or will the board-configuration now be placed in mmci-ux500?

Common DT populate code for all mmc host drivers
Some parts of the DT-population is common for all the host driver, for
instance setting the CAP and CAP2. This code should be moved to a
common place to be used by other host drivers as well.

About refactoring
There are numbers of patches stashed up for MMCI waiting for verdict
here http://git.kernel.org/?p=linux/kernel/git/linusw/linux-stericsson.git;a=shortlog;h=refs/heads/mmci.
If doing a refactorization please rebase it on top of these patches.
This needs to be done carefully, there may be more to considerations
than just DT. Maybe the timing is better to do this for 4.5, just
after 4.4 is closed. Then we can make sure that all new patches are
made on top of the refactorized base.

BR
Per

[PATCH 0/5] MMC: mmci: Provide bindings for Device Tree

[Russell King - ARM Linux]

On Thu, Mar 15, 2012 at 08:12:49PM +0100, Per Forlin wrote:
> About refactoring
> There are numbers of patches stashed up for MMCI waiting for verdict
> here http://git.kernel.org/?p=linux/kernel/git/linusw/linux-stericsson.git;a=shortlog;h=refs/heads/mmci.
> If doing a refactorization please rebase it on top of these patches.

Quite a number of these are in linux-next via my tree:

http://ftp.arm.linux.org.uk/git/?p=linux-next.git;a=shortlog;h=7437cfa532842ce75189826742bddf1ba137f58e

There's 10 there, out of the 17 in Linus' tree.  See my previous replies
on why that's the case.

[PATCH 0/5] MMC: mmci: Provide bindings for Device Tree

[Arnd Bergmann]

On Thursday 15 March 2012, Per Forlin wrote:
> On Thu, Mar 15, 2012 at 4:32 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Thursday 15 March 2012, Lee Jones wrote:
> >> > I would like to see what the minimal required change is to support DT
> >> > for mmci without factorization.
> >> > 1. Minimal change in mmci.
> >> > 2. Add mmci_dt.c which contains the DT-populate code.
> >> >
> >> > The factorization could be done as step 2 I think.
> >> >
> >> > What do you say?
> >>
> >> I'm wondering what the difference is as the work has already been done.
> >>
> >> It was Arnd's suggestion to separate out the two types of variants, and
> >> I'm quite fond of the new (fully featured) layout.
> >
> > Right, I usually prefer cleanups or other refactoring to be done first, and
> > then features added on top.
> >
> > You could in theory add have just patches 3/4/5 all applied without
> > the refactoring, but that I would be worried that this causes dependencies
> > between the mmci driver and ux500 specific functionality like the
> > stedma40_filter function.
> >
> About DT
> I think I need to catch up on the DT-design a bit. I will try to catch
> up on the DT-implementation and maybe then the refactorization will
> make sense to me :)
> 
> Board specific data in mmci-driver
> The stedma40 filter function is not really specific for ux500. ux500
> use stedma40 but it should be possible to replace this DMA.IP with
> some other DMA-controller. This is board specific configuration. You
> should not need to change the mmci-driver just because the dma-driver
> has changed, right?
> Or will the board-configuration now be placed in mmci-ux500?

Right, the DMA configuration does not really belong in there, but
the voltage setup might (unless we convert that to the regulator
setup).

> Common DT populate code for all mmc host drivers
> Some parts of the DT-population is common for all the host driver, for
> instance setting the CAP and CAP2. This code should be moved to a
> common place to be used by other host drivers as well.

Good idea, yes. We should also have a generic mmc binding document
that describes how to set flags like 8-bit mode. We already have
two conflicting variants in .dts files and we should make sure
we bring everybody back together here.

> About refactoring
> There are numbers of patches stashed up for MMCI waiting for verdict
> here http://git.kernel.org/?p=linux/kernel/git/linusw/linux-stericsson.git;a=shortlog;h=refs/heads/mmci.
> If doing a refactorization please rebase it on top of these patches.
> This needs to be done carefully, there may be more to considerations
> than just DT. Maybe the timing is better to do this for 4.5, just
> after 4.4 is closed. Then we can make sure that all new patches are
> made on top of the refactorized base.

Yes, thanks for the info. Doing this for 3.4 is definitely out of the
question then, and hopefully we can use the new dma bindings when it's
done for 3.5

	Arnd

[PATCH 0/5] MMC: mmci: Provide bindings for Device Tree

[Linus Walleij]

On Thu, Mar 15, 2012 at 9:58 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> [Per]
>> The stedma40 filter function is not really specific for ux500. ux500
>> use stedma40 but it should be possible to replace this DMA.IP with
>> some other DMA-controller. This is board specific configuration. You
>> should not need to change the mmci-driver just because the dma-driver
>> has changed, right?
>> Or will the board-configuration now be placed in mmci-ux500?
>
> Right, the DMA configuration does not really belong in there, but
> the voltage setup might (unless we convert that to the regulator
> setup).

The voltage to power the card is already using the regulator
framework with the MMC-specific helpers in the MMCI driver.

But I guess you're after modelling the levelshifter as a regulator?

Basically the level shifter is a separate device has two voltage
inputs A and B (from other regulators) that is controlled by a
simple GPIO to select voltage A or B to drive the signals to
the card.

That could probably be modelled as a regulator with two
volategs for sure, but then we should maybe create a more
generic "struct level_shifter_regulator" (or whatever) for the
concept as a whole.

Let's page Mark about what to do with levelshifters and whether
they are regulators of sorts in his book.

Yours,
Linus Walleij

[PATCH 0/5] MMC: mmci: Provide bindings for Device Tree

[Arnd Bergmann]

On Friday 16 March 2012, Linus Walleij wrote:
> On Thu, Mar 15, 2012 at 9:58 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > [Per]
> >> The stedma40 filter function is not really specific for ux500. ux500
> >> use stedma40 but it should be possible to replace this DMA.IP with
> >> some other DMA-controller. This is board specific configuration. You
> >> should not need to change the mmci-driver just because the dma-driver
> >> has changed, right?
> >> Or will the board-configuration now be placed in mmci-ux500?
> >
> > Right, the DMA configuration does not really belong in there, but
> > the voltage setup might (unless we convert that to the regulator
> > setup).
> 
> The voltage to power the card is already using the regulator
> framework with the MMC-specific helpers in the MMCI driver.
> 
> But I guess you're after modelling the levelshifter as a regulator?
> 
> Basically the level shifter is a separate device has two voltage
> inputs A and B (from other regulators) that is controlled by a
> simple GPIO to select voltage A or B to drive the signals to
> the card.
> 
> That could probably be modelled as a regulator with two
> volategs for sure, but then we should maybe create a more
> generic "struct level_shifter_regulator" (or whatever) for the
> concept as a whole.

Ok, thanks for the explanation.

> Let's page Mark about what to do with levelshifters and whether
> they are regulators of sorts in his book.

It does sound appealing, especially because this one could be
done completely generically by defining a regulator that has
a bunch of other regulators as well as a set of gpio lines as
inputs and one output that can be used in other devices. We
would probably only use this one together with device tree then.

	Arnd

[PATCH 0/5] MMC: mmci: Provide bindings for Device Tree

[Mark Brown]

On Fri, Mar 16, 2012 at 12:36:35PM +0000, Arnd Bergmann wrote:
> On Friday 16 March 2012, Linus Walleij wrote:
> > On Thu, Mar 15, 2012 at 9:58 PM, Arnd Bergmann <arnd@arndb.de> wrote:

> > But I guess you're after modelling the levelshifter as a regulator?

> > Basically the level shifter is a separate device has two voltage
> > inputs A and B (from other regulators) that is controlled by a
> > simple GPIO to select voltage A or B to drive the signals to
> > the card.

> > That could probably be modelled as a regulator with two
> > volategs for sure, but then we should maybe create a more
> > generic "struct level_shifter_regulator" (or whatever) for the
> > concept as a whole.

> Ok, thanks for the explanation.

I'm not sure I'd bother defining a special regulator type for this if it
is done using regulators - given that it's likely to just be a GPIO
rather than a specific driver I'm not sure it's worth worrying about how
exactly the hardware is implemented.

> > Let's page Mark about what to do with levelshifters and whether
> > they are regulators of sorts in his book.

> It does sound appealing, especially because this one could be
> done completely generically by defining a regulator that has
> a bunch of other regulators as well as a set of gpio lines as
> inputs and one output that can be used in other devices. We
> would probably only use this one together with device tree then.

It seems sensible to me - probably the existing gpio-regulator driver
can do the job, though it's not unreasonable to expect that we'll want
to support switching between variable voltage supplies at some point.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120317/67f5e0c6/attachment.sig>