[PATCH v3 00/13] mmc: core and driver DT and related development

[PATCH v3 00/13] mmc: core and driver DT and related development

[Guennadi Liakhovetski]

This is v3 of my mmc DT patchset with several patches updated and two
patches, previously sent separately, now integrated into the series.

Guennadi Liakhovetski (13):
  mmc: sdhi, tmio: only check flags in tmio-mmc driver proper
  mmc: detailed definition of CD and WP MMC line polarities in DT
  mmc: provide a standard MMC device-tree binding parser centrally
  mmc: (cosmetic) remove "extern" from function declarations
  mmc: sh-mmcif: use mmc_of_parse() to parse standard MMC DT bindings
  mmc: tmio-mmc: define device-tree bindings
  mmc: tmio-mmc: parse device-tree bindings
  mmc: sh_mobile_sdhi: remove unused .pdata field
  mmc: sh_mobile_sdhi: use managed resource allocations
  mmc: tmio: remove unused and deprecated symbols
  mmc: tmio: add support for the VccQ regulator
  mmc: add DT bindings for more MMC capability flags
  mmc: tmio: add barriers to IO operations

 Documentation/devicetree/bindings/mmc/mmc.txt      |   47 +++++++++-
 Documentation/devicetree/bindings/mmc/tmio_mmc.txt |   15 +++
 drivers/mmc/core/host.c                            |  103 ++++++++++++++++++++
 drivers/mmc/host/sh_mmcif.c                        |    3 +-
 drivers/mmc/host/sh_mobile_sdhi.c                  |   64 +++---------
 drivers/mmc/host/tmio_mmc.h                        |   18 ++--
 drivers/mmc/host/tmio_mmc_pio.c                    |   83 ++++++++++++++---
 include/linux/mfd/tmio.h                           |   18 ----
 include/linux/mmc/host.h                           |   23 +++--
 include/linux/mmc/sh_mobile_sdhi.h                 |    2 -
 10 files changed, 273 insertions(+), 103 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mmc/tmio_mmc.txt

-- 
1.7.2.5

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

[PATCH v3 01/13] mmc: sdhi, tmio: only check flags in tmio-mmc driver proper

[Guennadi Liakhovetski]

tmio-mmc platform flags can be set by various means, including caller
drivers and device-tree bindings, therefore it is better to only check
them in the tmio-mmc driver proper, not in caller drivers themselves.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/mmc/host/sh_mobile_sdhi.c |    3 +--
 drivers/mmc/host/tmio_mmc_pio.c   |    3 +++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 524a7f7..e0ca0ab 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -153,10 +153,9 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 	mmc_data->clk_enable = sh_mobile_sdhi_clk_enable;
 	mmc_data->clk_disable = sh_mobile_sdhi_clk_disable;
 	mmc_data->capabilities = MMC_CAP_MMC_HIGHSPEED;
+	mmc_data->write16_hook = sh_mobile_sdhi_write16_hook;
 	if (p) {
 		mmc_data->flags = p->tmio_flags;
-		if (mmc_data->flags & TMIO_MMC_HAS_IDLE_WAIT)
-			mmc_data->write16_hook = sh_mobile_sdhi_write16_hook;
 		mmc_data->ocr_mask = p->tmio_ocr_mask;
 		mmc_data->capabilities |= p->tmio_caps;
 		mmc_data->capabilities2 |= p->tmio_caps2;
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index 0f992e9..b25adb4 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -928,6 +928,9 @@ int tmio_mmc_host_probe(struct tmio_mmc_host **host,
 	int ret;
 	u32 irq_mask = TMIO_MASK_CMD;
 
+	if (!(pdata->flags & TMIO_MMC_HAS_IDLE_WAIT))
+		pdata->write16_hook = NULL;
+
 	res_ctl = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res_ctl)
 		return -EINVAL;
-- 
1.7.2.5

[PATCH v3 02/13] mmc: detailed definition of CD and WP MMC line polarities in DT

[Guennadi Liakhovetski]

Clarify ways to specify write-protect and card-detect MMC lines in FDT.

Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

v3: {wp,cd}-inverted properties can now be used together with GPIO
    binding flags. A detailed explanation added.

 Documentation/devicetree/bindings/mmc/mmc.txt |   43 +++++++++++++++++++++++-
 1 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
index 34f28ed..51dd64f 100644
--- a/Documentation/devicetree/bindings/mmc/mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc.txt
@@ -18,12 +18,51 @@ Only one of the properties in this section should be supplied:
 
 Optional properties:
 - wp-gpios: Specify GPIOs for write protection, see gpio binding
-- cd-inverted: when present, polarity on the cd gpio line is inverted
-- wp-inverted: when present, polarity on the wp gpio line is inverted
+- cd-inverted: when present, polarity on the CD line is inverted. See the note
+  below for the case, when a GPIO is used for the CD line
+- wp-inverted: when present, polarity on the WP line is inverted. See the note
+  below for the case, when a GPIO is used for the WP line
 - max-frequency: maximum operating clock frequency
 - no-1-8-v: when present, denotes that 1.8v card voltage is not supported on
   this system, even if the controller claims it is.
 
+*NOTE* on CD and WP polarity. As of 3.8, SD/MMC drivers parse their DT nodes
+each in its own way, including calculation of CD and WP polarities. Our goal is
+to implement a common MMC DT parser and convert all drivers to using it. For
+this we also have to fix the meaning of the "normal" and "inverted" line levels.
+We choose to follow the SDHCI standard, which specifies both those lines as
+"active low." To deliver line polarity to drivers the parser will set
+MMC_CAP2_CD_ACTIVE_HIGH and / or MMC_CAP2_RO_ACTIVE_HIGH capabilities.
+
+CD and WP lines can be implemented on the hardware in one of two ways: as GPIOs,
+specified in cd-gpios and wp-gpios properties, or as dedicated pins. Polarity of
+dedicated pins can be specified, using *-inverted properties. GPIO polarity can
+also be specified using the OF_GPIO_ACTIVE_LOW flag. This creates an ambiguity
+in the latter case. So far there are no drivers, that evaluate both these
+possibilities. Also, all .dts files only use one of them. Boards with mmc
+devices, whose drivers use *-inverted properties, leave the OF_GPIO_ACTIVE_LOW
+flag clear (which is actually usually wrong, since it means "active high," i.e.
+inverted.) OTOH, boards with mmc devices, whose drivers use the
+OF_GPIO_ACTIVE_LOW flag, never specify *-inverted properties. This means, that
+the safest way to implement a common MMC DT parser is by using the XOR logic, as
+in
+
+	explicit_inv_cd = !!of_find_property(np, "cd-inverted", &len);
+
+	cd_gpio = of_get_named_gpio_flags(np, "cd-gpios", 0, &flags);
+	if (gpio_is_valid(cd_gpio) && !(flags & OF_GPIO_ACTIVE_LOW))
+		gpio_inv_cd = 1;
+	else
+		gpio_inv_cd = 0;
+
+	if (gpio_inv_cd ^ explicit_inv_cd)
+		host->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
+
+Similarly for WP. This way drivers, currently only interpreting *-inverted
+properties with .dts files always having cd-gpios' OF_GPIO_ACTIVE_LOW flag
+clear, will have to invert the MMC_CAP2_CD_ACTIVE_HIGH capability, but at least
+this capability will not miss any toggles.
+
 Optional SDIO properties:
 - keep-power-in-suspend: Preserves card power during a suspend/resume cycle
 - enable-sdio-wakeup: Enables wake up of host system on SDIO IRQ assertion
-- 
1.7.2.5

Re: [PATCH v3 02/13] mmc: detailed definition of CD and WP MMC line polarities in DT

[Arnd Bergmann]

On Wednesday 06 February 2013, Guennadi Liakhovetski wrote:
> +*NOTE* on CD and WP polarity. As of 3.8, SD/MMC drivers parse their DT nodes
> +each in its own way, including calculation of CD and WP polarities. Our goal is
> +to implement a common MMC DT parser and convert all drivers to using it. For
> +this we also have to fix the meaning of the "normal" and "inverted" line levels.
> +We choose to follow the SDHCI standard, which specifies both those lines as
> +"active low." To deliver line polarity to drivers the parser will set
> +MMC_CAP2_CD_ACTIVE_HIGH and / or MMC_CAP2_RO_ACTIVE_HIGH capabilities.

As mentioned in my other comment, please leave out references to the
Linux implementation. The long-term goal is to split out the
bindings from the Linux kernel, because they are used by other
projects as well. We also cannot easily change bindings that are
already established, so referring to a specific version here is not
helpful.

Your descriptions all make sense and I think we should put them
somewhere, but for the binding, we need a more neutral text
that just describes the interface to someone writing a device
tree file.
	
	Arnd

[PATCH v3 03/13] mmc: provide a standard MMC device-tree binding parser centrally

[Guennadi Liakhovetski]

MMC defines a number of standard DT bindings. Having each driver parse
them individually adds code redundancy and is error prone. Provide a
standard function to unify the parsing. After all drivers are converted
to using it instead of their own parsers, this function can be integrated
into mmc_alloc_host().

Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

v3: updated to match v3 of mmc.txt disambiguation in patch 02/13.

 drivers/mmc/core/host.c  |   90 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h |    1 +
 2 files changed, 91 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index ee2e16b..c412612 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -15,6 +15,8 @@
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/idr.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/pagemap.h>
 #include <linux/export.h>
 #include <linux/leds.h>
@@ -23,6 +25,7 @@
 
 #include <linux/mmc/host.h>
 #include <linux/mmc/card.h>
+#include <linux/mmc/slot-gpio.h>
 
 #include "core.h"
 #include "host.h"
@@ -294,6 +297,93 @@ static inline void mmc_host_clk_sysfs_init(struct mmc_host *host)
 
 #endif
 
+void mmc_of_parse(struct mmc_host *host)
+{
+	struct device_node *np;
+	u32 bus_width;
+	bool explicit_inv_wp, gpio_inv_wp = false;
+	enum of_gpio_flags flags;
+	int len, ret, gpio;
+
+	if (!host->parent || !host->parent->of_node)
+		return;
+
+	np = host->parent->of_node;
+
+	if (of_property_read_u32_array(np, "bus-width", &bus_width, 1) < 0) {
+		dev_err(host->parent,
+			"Compulsory \"bus-width\" property is missing!\n");
+		return;
+	}
+
+	switch (bus_width) {
+	case 8:
+		host->caps |= MMC_CAP_8_BIT_DATA;
+		/* Hosts, capable of 8-bit transfers can also do 4 bits */
+	case 4:
+		host->caps |= MMC_CAP_4_BIT_DATA;
+		break;
+	case 1:
+		break;
+	default:
+		dev_err(host->parent,
+			"Invalid \"bus-width\" value %ud!\n", bus_width);
+	}
+
+	/* Optional property: ignore errors */
+	of_property_read_u32_array(np, "max-frequency", &host->f_max, 1);
+
+	/* Both CD and WP pins are by default active low */
+
+	/* Parse Card Detection */
+	if (of_find_property(np, "non-removable", &len)) {
+		host->caps |= MMC_CAP_NONREMOVABLE;
+	} else {
+		bool explicit_inv_cd, gpio_inv_cd = false;
+
+		explicit_inv_cd = !!of_find_property(np, "cd-inverted", &len);
+
+		if (of_find_property(np, "broken-cd", &len))
+			host->caps |= MMC_CAP_NEEDS_POLL;
+
+		gpio = of_get_named_gpio_flags(np, "cd-gpios", 0, &flags);
+		if (gpio_is_valid(gpio)) {
+			if (!(flags & OF_GPIO_ACTIVE_LOW))
+				gpio_inv_cd = true;
+
+			ret = mmc_gpio_request_cd(host, gpio);
+			if (ret < 0)
+				dev_err(host->parent,
+					"Failed to request CD GPIO #%d: %d!\n",
+					gpio, ret);
+			else
+				dev_info(host->parent, "Got CD GPIO #%d.\n",
+					 gpio);
+		}
+
+		if (explicit_inv_cd ^ gpio_inv_cd)
+			host->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
+	}
+
+	/* Parse Write Protection */
+	explicit_inv_wp = !!of_find_property(np, "wp-inverted", &len);
+
+	gpio = of_get_named_gpio_flags(np, "wp-gpios", 0, &flags);
+	if (gpio_is_valid(gpio)) {
+		if (!(flags & OF_GPIO_ACTIVE_LOW))
+			gpio_inv_wp = true;
+
+		ret = mmc_gpio_request_ro(host, gpio);
+		if (ret < 0)
+			dev_err(host->parent,
+				"Failed to request WP GPIO: %d!\n", ret);
+	}
+	if (explicit_inv_wp ^ gpio_inv_wp)
+		host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
+}
+
+EXPORT_SYMBOL(mmc_of_parse);
+
 /**
  *	mmc_alloc_host - initialise the per-host structure.
  *	@extra: sizeof private data structure
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 61a10c1..13c0c8d 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -345,6 +345,7 @@ extern struct mmc_host *mmc_alloc_host(int extra, struct device *);
 extern int mmc_add_host(struct mmc_host *);
 extern void mmc_remove_host(struct mmc_host *);
 extern void mmc_free_host(struct mmc_host *);
+void mmc_of_parse(struct mmc_host *host);
 
 static inline void *mmc_priv(struct mmc_host *host)
 {
-- 
1.7.2.5

Re: [PATCH v3 03/13] mmc: provide a standard MMC device-tree binding parser centrally

[Markus Pargmann]

On Wed, Feb 06, 2013 at 08:46:50PM +0100, Guennadi Liakhovetski wrote:
> +	/* Parse Card Detection */
> +	if (of_find_property(np, "non-removable", &len)) {
> +		host->caps |= MMC_CAP_NONREMOVABLE;
> +	} else {
> +		bool explicit_inv_cd, gpio_inv_cd = false;
> +
> +		explicit_inv_cd = !!of_find_property(np, "cd-inverted", &len);

You could use of_property_read_bool here

> +	/* Parse Write Protection */
> +	explicit_inv_wp = !!of_find_property(np, "wp-inverted", &len);

and here.

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v3 04/13] mmc: (cosmetic) remove "extern" from function declarations

[Guennadi Liakhovetski]

The "extern" keyword isn't required in function declarations, remove it.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 include/linux/mmc/host.h |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 13c0c8d..356ae0a 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -341,10 +341,10 @@ struct mmc_host {
 	unsigned long		private[0] ____cacheline_aligned;
 };
 
-extern struct mmc_host *mmc_alloc_host(int extra, struct device *);
-extern int mmc_add_host(struct mmc_host *);
-extern void mmc_remove_host(struct mmc_host *);
-extern void mmc_free_host(struct mmc_host *);
+struct mmc_host *mmc_alloc_host(int extra, struct device *);
+int mmc_add_host(struct mmc_host *);
+void mmc_remove_host(struct mmc_host *);
+void mmc_free_host(struct mmc_host *);
 void mmc_of_parse(struct mmc_host *host);
 
 static inline void *mmc_priv(struct mmc_host *host)
@@ -358,16 +358,16 @@ static inline void *mmc_priv(struct mmc_host *host)
 #define mmc_classdev(x)	(&(x)->class_dev)
 #define mmc_hostname(x)	(dev_name(&(x)->class_dev))
 
-extern int mmc_suspend_host(struct mmc_host *);
-extern int mmc_resume_host(struct mmc_host *);
+int mmc_suspend_host(struct mmc_host *);
+int mmc_resume_host(struct mmc_host *);
 
-extern int mmc_power_save_host(struct mmc_host *host);
-extern int mmc_power_restore_host(struct mmc_host *host);
+int mmc_power_save_host(struct mmc_host *host);
+int mmc_power_restore_host(struct mmc_host *host);
 
-extern void mmc_detect_change(struct mmc_host *, unsigned long delay);
-extern void mmc_request_done(struct mmc_host *, struct mmc_request *);
+void mmc_detect_change(struct mmc_host *, unsigned long delay);
+void mmc_request_done(struct mmc_host *, struct mmc_request *);
 
-extern int mmc_cache_ctrl(struct mmc_host *, u8);
+int mmc_cache_ctrl(struct mmc_host *, u8);
 
 static inline void mmc_signal_sdio_irq(struct mmc_host *host)
 {
-- 
1.7.2.5

[PATCH v3 05/13] mmc: sh-mmcif: use mmc_of_parse() to parse standard MMC DT bindings

[Guennadi Liakhovetski]

Use mmc_of_parse() to get interface capability flags and used GPIOs from
device-tree bindings.

Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

v3: updated on top of "mmc: sh_mmcif: Avoid unnecessary mmc_delay() at
    mmc_card_sleepawake()"

 drivers/mmc/host/sh_mmcif.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 7d05852..adb4f68 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -1331,6 +1331,7 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 		ret = -ENOMEM;
 		goto ealloch;
 	}
+	mmc_of_parse(mmc);
 	host		= mmc_priv(mmc);
 	host->mmc	= mmc;
 	host->addr	= reg;
@@ -1343,7 +1344,7 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 	mmc->ops = &sh_mmcif_ops;
 	sh_mmcif_init_ocr(host);
 
-	mmc->caps = MMC_CAP_MMC_HIGHSPEED | MMC_CAP_WAIT_WHILE_BUSY;
+	mmc->caps |= MMC_CAP_MMC_HIGHSPEED | MMC_CAP_WAIT_WHILE_BUSY;
 	if (pd && pd->caps)
 		mmc->caps |= pd->caps;
 	mmc->max_segs = 32;
-- 
1.7.2.5

[PATCH v3 06/13] mmc: tmio-mmc: define device-tree bindings

[Guennadi Liakhovetski]

Define device-tree bindings for the tmio-mmc driver to be able to specify
parameters, currently provided in platform data.

Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org>
---

v3: make the property to set TMIO_MMC_SDIO_IRQ global

 Documentation/devicetree/bindings/mmc/tmio_mmc.txt |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mmc/tmio_mmc.txt

diff --git a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
new file mode 100644
index 0000000..5762a55
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
@@ -0,0 +1,15 @@
+* Toshiba Mobile IO SD/MMC controller
+
+The tmio-mmc driver doesn't probe its devices actively, instead its binding to
+devices is managed by either MFD drivers or by the sh_mobile_sdhi platform
+driver. Those drivers supply the tmio-mmc driver with platform data, that either
+describe hardware capabilities, known to them, or are obtained by them from
+their own platform data or from their DT information. In the latter case all
+compulsory and any optional properties, common to all SD/MMC drivers, as
+described in mmc.txt, should or can be used. Additionally the following optional
+bindings can be used. They set respective TMIO_MMC_* flags.
+
+Optional properties:
+- toshiba,mmc-wrprotect-disable	: set TMIO_MMC_WRPROTECT_DISABLE flag
+- toshiba,mmc-blksz-2bytes	: set TMIO_MMC_BLKSZ_2BYTES
+- toshiba,mmc-has-idle-wait	: set TMIO_MMC_HAS_IDLE_WAIT
-- 
1.7.2.5

Re: [PATCH v3 06/13] mmc: tmio-mmc: define device-tree bindings

[Arnd Bergmann]

On Wednesday 06 February 2013, Guennadi Liakhovetski wrote:
> +* Toshiba Mobile IO SD/MMC controller
> +
> +The tmio-mmc driver doesn't probe its devices actively, instead its binding to
> +devices is managed by either MFD drivers or by the sh_mobile_sdhi platform
> +driver. Those drivers supply the tmio-mmc driver with platform data, that either
> +describe hardware capabilities, known to them, or are obtained by them from
> +their own platform data or from their DT information. In the latter case all
> +compulsory and any optional properties, common to all SD/MMC drivers, as
> +described in mmc.txt, should or can be used. Additionally the following optional
> +bindings can be used. They set respective TMIO_MMC_* flags.
> +
> +Optional properties:
> +- toshiba,mmc-wrprotect-disable        : set TMIO_MMC_WRPROTECT_DISABLE flag
> +- toshiba,mmc-blksz-2bytes     : set TMIO_MMC_BLKSZ_2BYTES
> +- toshiba,mmc-has-idle-wait    : set TMIO_MMC_HAS_IDLE_WAIT

Please write the binding in a way that does not refer to a specific
implementation in Linux: The binding should describe the hardware
independent of details in the driver. In particular, I think you
should not refer to the TMIO_MMC_BLKSZ_2BYTES etc macros but describe
in text what the flags are about.

Regarding the toshiba,mmc-wrprotect-disable property, would it be
enough to just check the presence of the wp-gpios property?

TMIO_MMC_BLKSZ_2BYTES seems to be set unconditionally in
sh_mobile_sdhi_probe and nowhere else, so I'd assume we don't
actually need to provide this here, but can keep that knowledge
implicit based on whether we're talking to sh_mobile_sdhi
or another tmio_mmc variant.

For the other last one, is that actually board specific, or just
a feature of a given chip? If we can tell by the SoC, then I'd
suggest using separate "compatible" properties instead, and
put a bitmask of features into the .data field of the of match
table. For all I can tell, SH7372 does not set it, while SH73A0,
R8A7740 and R8A7779 always do.

	Arnd

Re: [PATCH v3 06/13] mmc: tmio-mmc: define device-tree bindings

[Simon Horman]

On Wed, Feb 06, 2013 at 10:24:20PM +0000, Arnd Bergmann wrote:
> On Wednesday 06 February 2013, Guennadi Liakhovetski wrote:
> > +* Toshiba Mobile IO SD/MMC controller
> > +
> > +The tmio-mmc driver doesn't probe its devices actively, instead its binding to
> > +devices is managed by either MFD drivers or by the sh_mobile_sdhi platform
> > +driver. Those drivers supply the tmio-mmc driver with platform data, that either
> > +describe hardware capabilities, known to them, or are obtained by them from
> > +their own platform data or from their DT information. In the latter case all
> > +compulsory and any optional properties, common to all SD/MMC drivers, as
> > +described in mmc.txt, should or can be used. Additionally the following optional
> > +bindings can be used. They set respective TMIO_MMC_* flags.
> > +
> > +Optional properties:
> > +- toshiba,mmc-wrprotect-disable        : set TMIO_MMC_WRPROTECT_DISABLE flag
> > +- toshiba,mmc-blksz-2bytes     : set TMIO_MMC_BLKSZ_2BYTES
> > +- toshiba,mmc-has-idle-wait    : set TMIO_MMC_HAS_IDLE_WAIT
> 
> Please write the binding in a way that does not refer to a specific
> implementation in Linux: The binding should describe the hardware
> independent of details in the driver. In particular, I think you
> should not refer to the TMIO_MMC_BLKSZ_2BYTES etc macros but describe
> in text what the flags are about.
> 
> Regarding the toshiba,mmc-wrprotect-disable property, would it be
> enough to just check the presence of the wp-gpios property?
> 
> TMIO_MMC_BLKSZ_2BYTES seems to be set unconditionally in
> sh_mobile_sdhi_probe and nowhere else, so I'd assume we don't
> actually need to provide this here, but can keep that knowledge
> implicit based on whether we're talking to sh_mobile_sdhi
> or another tmio_mmc variant.
> 
> For the other last one, is that actually board specific, or just
> a feature of a given chip? If we can tell by the SoC, then I'd
> suggest using separate "compatible" properties instead, and
> put a bitmask of features into the .data field of the of match
> table. For all I can tell, SH7372 does not set it, while SH73A0,
> R8A7740 and R8A7779 always do.

My understanding is that TMIO_MMC_HAS_IDLE_WAIT can be set based
on the SoC in use.

Re: [PATCH v3 06/13] mmc: tmio-mmc: define device-tree bindings

[Arnd Bergmann]

On Thursday 07 February 2013, Simon Horman wrote:
> > Please write the binding in a way that does not refer to a specific
> > implementation in Linux: The binding should describe the hardware
> > independent of details in the driver. In particular, I think you
> > should not refer to the TMIO_MMC_BLKSZ_2BYTES etc macros but describe
> > in text what the flags are about.
> > 
> > Regarding the toshiba,mmc-wrprotect-disable property, would it be
> > enough to just check the presence of the wp-gpios property?
> > 
> > TMIO_MMC_BLKSZ_2BYTES seems to be set unconditionally in
> > sh_mobile_sdhi_probe and nowhere else, so I'd assume we don't
> > actually need to provide this here, but can keep that knowledge
> > implicit based on whether we're talking to sh_mobile_sdhi
> > or another tmio_mmc variant.
> > 
> > For the other last one, is that actually board specific, or just
> > a feature of a given chip? If we can tell by the SoC, then I'd
> > suggest using separate "compatible" properties instead, and
> > put a bitmask of features into the .data field of the of match
> > table. For all I can tell, SH7372 does not set it, while SH73A0,
> > R8A7740 and R8A7779 always do.
> 
> My understanding is that TMIO_MMC_HAS_IDLE_WAIT can be set based
> on the SoC in use.

Ok, thanks for the confirmation. Just to be clear: Either way (compatible
or separate property) works fine and can be used here. I tend to prefer
basing these things on the "compatible" string in order to keep
specific knowledge of the device internals out of the device tree
binding though.

	Arnd

Re: [PATCH v3 06/13] mmc: tmio-mmc: define device-tree bindings

[Guennadi Liakhovetski]

On Thu, 7 Feb 2013, Simon Horman wrote:

> On Wed, Feb 06, 2013 at 10:24:20PM +0000, Arnd Bergmann wrote:
> > On Wednesday 06 February 2013, Guennadi Liakhovetski wrote:
> > > +* Toshiba Mobile IO SD/MMC controller
> > > +
> > > +The tmio-mmc driver doesn't probe its devices actively, instead its binding to
> > > +devices is managed by either MFD drivers or by the sh_mobile_sdhi platform
> > > +driver. Those drivers supply the tmio-mmc driver with platform data, that either
> > > +describe hardware capabilities, known to them, or are obtained by them from
> > > +their own platform data or from their DT information. In the latter case all
> > > +compulsory and any optional properties, common to all SD/MMC drivers, as
> > > +described in mmc.txt, should or can be used. Additionally the following optional
> > > +bindings can be used. They set respective TMIO_MMC_* flags.
> > > +
> > > +Optional properties:
> > > +- toshiba,mmc-wrprotect-disable        : set TMIO_MMC_WRPROTECT_DISABLE flag
> > > +- toshiba,mmc-blksz-2bytes     : set TMIO_MMC_BLKSZ_2BYTES
> > > +- toshiba,mmc-has-idle-wait    : set TMIO_MMC_HAS_IDLE_WAIT
> > 
> > Please write the binding in a way that does not refer to a specific
> > implementation in Linux: The binding should describe the hardware
> > independent of details in the driver. In particular, I think you
> > should not refer to the TMIO_MMC_BLKSZ_2BYTES etc macros but describe
> > in text what the flags are about.
> > 
> > Regarding the toshiba,mmc-wrprotect-disable property, would it be
> > enough to just check the presence of the wp-gpios property?
> > 
> > TMIO_MMC_BLKSZ_2BYTES seems to be set unconditionally in
> > sh_mobile_sdhi_probe and nowhere else, so I'd assume we don't
> > actually need to provide this here, but can keep that knowledge
> > implicit based on whether we're talking to sh_mobile_sdhi
> > or another tmio_mmc variant.

Can do that, yes.

> > For the other last one, is that actually board specific, or just
> > a feature of a given chip? If we can tell by the SoC, then I'd
> > suggest using separate "compatible" properties instead, and
> > put a bitmask of features into the .data field of the of match
> > table. For all I can tell, SH7372 does not set it, while SH73A0,
> > R8A7740 and R8A7779 always do.
> 
> My understanding is that TMIO_MMC_HAS_IDLE_WAIT can be set based
> on the SoC in use.

So far TMIO_MMC_HAS_IDLE_WAIT is set on

board-kzm9g.c (sh73a0 / AG5)
board-ag5evm.c (sh73a0 / AG5)
board-armadillo800eva.c (r8a7740 / A1)
board-kota2.c (sh73a0 / AG5)
board-marzen.c (r8a7779 / H1)

and isn't set on

board-ap4evb.c (sh7372 / ap4)
board-bonito.c (r8a7740 / a1, SDHI isn't used)
board-mackerel.c (sh7372 / ap4)

So, shall we use a compatible property for this and drop this property? We 
can add later at any time, if needed, which is better, than defining 
something redundant. OTOH I seem to remember, that using SoC-version from 
the "compatible" property was considered by someone inappropriate. Magnus, 
what do you think?

So, if we drop TMIO_MMC_BLKSZ_2BYTES and TMIO_MMC_HAS_IDLE_WAIT, we only 
keep toshiba,mmc-wrprotect-disable to set TMIO_MMC_WRPROTECT_DISABLE? And 
that one is definitely needed, because that even differs between SDHI 
instances on one SoC.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH v3 06/13] mmc: tmio-mmc: define device-tree bindings

[Magnus Damm]

On Thu, Feb 14, 2013 at 12:59 AM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> On Thu, 7 Feb 2013, Simon Horman wrote:
>
>> On Wed, Feb 06, 2013 at 10:24:20PM +0000, Arnd Bergmann wrote:
>> > On Wednesday 06 February 2013, Guennadi Liakhovetski wrote:
>> > > +* Toshiba Mobile IO SD/MMC controller
>> > > +
>> > > +The tmio-mmc driver doesn't probe its devices actively, instead its binding to
>> > > +devices is managed by either MFD drivers or by the sh_mobile_sdhi platform
>> > > +driver. Those drivers supply the tmio-mmc driver with platform data, that either
>> > > +describe hardware capabilities, known to them, or are obtained by them from
>> > > +their own platform data or from their DT information. In the latter case all
>> > > +compulsory and any optional properties, common to all SD/MMC drivers, as
>> > > +described in mmc.txt, should or can be used. Additionally the following optional
>> > > +bindings can be used. They set respective TMIO_MMC_* flags.
>> > > +
>> > > +Optional properties:
>> > > +- toshiba,mmc-wrprotect-disable        : set TMIO_MMC_WRPROTECT_DISABLE flag
>> > > +- toshiba,mmc-blksz-2bytes     : set TMIO_MMC_BLKSZ_2BYTES
>> > > +- toshiba,mmc-has-idle-wait    : set TMIO_MMC_HAS_IDLE_WAIT
>> >
>> > Please write the binding in a way that does not refer to a specific
>> > implementation in Linux: The binding should describe the hardware
>> > independent of details in the driver. In particular, I think you
>> > should not refer to the TMIO_MMC_BLKSZ_2BYTES etc macros but describe
>> > in text what the flags are about.
>> >
>> > Regarding the toshiba,mmc-wrprotect-disable property, would it be
>> > enough to just check the presence of the wp-gpios property?
>> >
>> > TMIO_MMC_BLKSZ_2BYTES seems to be set unconditionally in
>> > sh_mobile_sdhi_probe and nowhere else, so I'd assume we don't
>> > actually need to provide this here, but can keep that knowledge
>> > implicit based on whether we're talking to sh_mobile_sdhi
>> > or another tmio_mmc variant.
>
> Can do that, yes.
>
>> > For the other last one, is that actually board specific, or just
>> > a feature of a given chip? If we can tell by the SoC, then I'd
>> > suggest using separate "compatible" properties instead, and
>> > put a bitmask of features into the .data field of the of match
>> > table. For all I can tell, SH7372 does not set it, while SH73A0,
>> > R8A7740 and R8A7779 always do.
>>
>> My understanding is that TMIO_MMC_HAS_IDLE_WAIT can be set based
>> on the SoC in use.
>
> So far TMIO_MMC_HAS_IDLE_WAIT is set on
>
> board-kzm9g.c (sh73a0 / AG5)
> board-ag5evm.c (sh73a0 / AG5)
> board-armadillo800eva.c (r8a7740 / A1)
> board-kota2.c (sh73a0 / AG5)
> board-marzen.c (r8a7779 / H1)
>
> and isn't set on
>
> board-ap4evb.c (sh7372 / ap4)
> board-bonito.c (r8a7740 / a1, SDHI isn't used)
> board-mackerel.c (sh7372 / ap4)
>
> So, shall we use a compatible property for this and drop this property? We
> can add later at any time, if needed, which is better, than defining
> something redundant. OTOH I seem to remember, that using SoC-version from
> the "compatible" property was considered by someone inappropriate. Magnus,
> what do you think?

I prefer you to use a hardware-block version compatible suffix instead
of SoC suffix.

This since we have more SoCs than actual hardware block
configurations. Using the list above, how many configurations would we
have?

Actually, forcing the drivers to be updated for each new SoC sounds
like a pretty terrible idea. Wouldn't that be against one of the
merits of using DT? Also, don't you have enough interesting work piled
up already? =)

Basically, I can't see any point in adding an extra unnecessary need
for updating the drivers when there is no real functional change.

Thanks,

/ magnus

Re: [PATCH v3 06/13] mmc: tmio-mmc: define device-tree bindings

[Simon Horman]

On Thu, Feb 14, 2013 at 10:42:21AM +0900, Magnus Damm wrote:
> On Thu, Feb 14, 2013 at 12:59 AM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > On Thu, 7 Feb 2013, Simon Horman wrote:
> >
> >> On Wed, Feb 06, 2013 at 10:24:20PM +0000, Arnd Bergmann wrote:
> >> > On Wednesday 06 February 2013, Guennadi Liakhovetski wrote:
> >> > > +* Toshiba Mobile IO SD/MMC controller
> >> > > +
> >> > > +The tmio-mmc driver doesn't probe its devices actively, instead its binding to
> >> > > +devices is managed by either MFD drivers or by the sh_mobile_sdhi platform
> >> > > +driver. Those drivers supply the tmio-mmc driver with platform data, that either
> >> > > +describe hardware capabilities, known to them, or are obtained by them from
> >> > > +their own platform data or from their DT information. In the latter case all
> >> > > +compulsory and any optional properties, common to all SD/MMC drivers, as
> >> > > +described in mmc.txt, should or can be used. Additionally the following optional
> >> > > +bindings can be used. They set respective TMIO_MMC_* flags.
> >> > > +
> >> > > +Optional properties:
> >> > > +- toshiba,mmc-wrprotect-disable        : set TMIO_MMC_WRPROTECT_DISABLE flag
> >> > > +- toshiba,mmc-blksz-2bytes     : set TMIO_MMC_BLKSZ_2BYTES
> >> > > +- toshiba,mmc-has-idle-wait    : set TMIO_MMC_HAS_IDLE_WAIT
> >> >
> >> > Please write the binding in a way that does not refer to a specific
> >> > implementation in Linux: The binding should describe the hardware
> >> > independent of details in the driver. In particular, I think you
> >> > should not refer to the TMIO_MMC_BLKSZ_2BYTES etc macros but describe
> >> > in text what the flags are about.
> >> >
> >> > Regarding the toshiba,mmc-wrprotect-disable property, would it be
> >> > enough to just check the presence of the wp-gpios property?
> >> >
> >> > TMIO_MMC_BLKSZ_2BYTES seems to be set unconditionally in
> >> > sh_mobile_sdhi_probe and nowhere else, so I'd assume we don't
> >> > actually need to provide this here, but can keep that knowledge
> >> > implicit based on whether we're talking to sh_mobile_sdhi
> >> > or another tmio_mmc variant.
> >
> > Can do that, yes.
> >
> >> > For the other last one, is that actually board specific, or just
> >> > a feature of a given chip? If we can tell by the SoC, then I'd
> >> > suggest using separate "compatible" properties instead, and
> >> > put a bitmask of features into the .data field of the of match
> >> > table. For all I can tell, SH7372 does not set it, while SH73A0,
> >> > R8A7740 and R8A7779 always do.
> >>
> >> My understanding is that TMIO_MMC_HAS_IDLE_WAIT can be set based
> >> on the SoC in use.
> >
> > So far TMIO_MMC_HAS_IDLE_WAIT is set on
> >
> > board-kzm9g.c (sh73a0 / AG5)
> > board-ag5evm.c (sh73a0 / AG5)
> > board-armadillo800eva.c (r8a7740 / A1)
> > board-kota2.c (sh73a0 / AG5)
> > board-marzen.c (r8a7779 / H1)
> >
> > and isn't set on
> >
> > board-ap4evb.c (sh7372 / ap4)
> > board-bonito.c (r8a7740 / a1, SDHI isn't used)
> > board-mackerel.c (sh7372 / ap4)
> >
> > So, shall we use a compatible property for this and drop this property? We
> > can add later at any time, if needed, which is better, than defining
> > something redundant. OTOH I seem to remember, that using SoC-version from
> > the "compatible" property was considered by someone inappropriate. Magnus,
> > what do you think?
> 
> I prefer you to use a hardware-block version compatible suffix instead
> of SoC suffix.
> 
> This since we have more SoCs than actual hardware block
> configurations. Using the list above, how many configurations would we
> have?
> 
> Actually, forcing the drivers to be updated for each new SoC sounds
> like a pretty terrible idea. Wouldn't that be against one of the
> merits of using DT? Also, don't you have enough interesting work piled
> up already? =)
> 
> Basically, I can't see any point in adding an extra unnecessary need
> for updating the drivers when there is no real functional change.

My understanding is that the discussion is about the details of
bindings that are required for SDHI to function when brought
up using DT on a variety of boards. Not exciting new work.

In particular how to set TMIO_MMC_HAS_IDLE_WAIT via DT.

Re: [PATCH v3 06/13] mmc: tmio-mmc: define device-tree bindings

[Magnus Damm]

On Thu, Feb 14, 2013 at 10:59 AM, Simon Horman <horms@verge.net.au> wrote:
> On Thu, Feb 14, 2013 at 10:42:21AM +0900, Magnus Damm wrote:
>> On Thu, Feb 14, 2013 at 12:59 AM, Guennadi Liakhovetski
>> <g.liakhovetski@gmx.de> wrote:
>> > On Thu, 7 Feb 2013, Simon Horman wrote:
>> >
>> >> On Wed, Feb 06, 2013 at 10:24:20PM +0000, Arnd Bergmann wrote:
>> >> > On Wednesday 06 February 2013, Guennadi Liakhovetski wrote:
>> >> > > +* Toshiba Mobile IO SD/MMC controller
>> >> > > +
>> >> > > +The tmio-mmc driver doesn't probe its devices actively, instead its binding to
>> >> > > +devices is managed by either MFD drivers or by the sh_mobile_sdhi platform
>> >> > > +driver. Those drivers supply the tmio-mmc driver with platform data, that either
>> >> > > +describe hardware capabilities, known to them, or are obtained by them from
>> >> > > +their own platform data or from their DT information. In the latter case all
>> >> > > +compulsory and any optional properties, common to all SD/MMC drivers, as
>> >> > > +described in mmc.txt, should or can be used. Additionally the following optional
>> >> > > +bindings can be used. They set respective TMIO_MMC_* flags.
>> >> > > +
>> >> > > +Optional properties:
>> >> > > +- toshiba,mmc-wrprotect-disable        : set TMIO_MMC_WRPROTECT_DISABLE flag
>> >> > > +- toshiba,mmc-blksz-2bytes     : set TMIO_MMC_BLKSZ_2BYTES
>> >> > > +- toshiba,mmc-has-idle-wait    : set TMIO_MMC_HAS_IDLE_WAIT
>> >> >
>> >> > Please write the binding in a way that does not refer to a specific
>> >> > implementation in Linux: The binding should describe the hardware
>> >> > independent of details in the driver. In particular, I think you
>> >> > should not refer to the TMIO_MMC_BLKSZ_2BYTES etc macros but describe
>> >> > in text what the flags are about.
>> >> >
>> >> > Regarding the toshiba,mmc-wrprotect-disable property, would it be
>> >> > enough to just check the presence of the wp-gpios property?
>> >> >
>> >> > TMIO_MMC_BLKSZ_2BYTES seems to be set unconditionally in
>> >> > sh_mobile_sdhi_probe and nowhere else, so I'd assume we don't
>> >> > actually need to provide this here, but can keep that knowledge
>> >> > implicit based on whether we're talking to sh_mobile_sdhi
>> >> > or another tmio_mmc variant.
>> >
>> > Can do that, yes.
>> >
>> >> > For the other last one, is that actually board specific, or just
>> >> > a feature of a given chip? If we can tell by the SoC, then I'd
>> >> > suggest using separate "compatible" properties instead, and
>> >> > put a bitmask of features into the .data field of the of match
>> >> > table. For all I can tell, SH7372 does not set it, while SH73A0,
>> >> > R8A7740 and R8A7779 always do.
>> >>
>> >> My understanding is that TMIO_MMC_HAS_IDLE_WAIT can be set based
>> >> on the SoC in use.
>> >
>> > So far TMIO_MMC_HAS_IDLE_WAIT is set on
>> >
>> > board-kzm9g.c (sh73a0 / AG5)
>> > board-ag5evm.c (sh73a0 / AG5)
>> > board-armadillo800eva.c (r8a7740 / A1)
>> > board-kota2.c (sh73a0 / AG5)
>> > board-marzen.c (r8a7779 / H1)
>> >
>> > and isn't set on
>> >
>> > board-ap4evb.c (sh7372 / ap4)
>> > board-bonito.c (r8a7740 / a1, SDHI isn't used)
>> > board-mackerel.c (sh7372 / ap4)
>> >
>> > So, shall we use a compatible property for this and drop this property? We
>> > can add later at any time, if needed, which is better, than defining
>> > something redundant. OTOH I seem to remember, that using SoC-version from
>> > the "compatible" property was considered by someone inappropriate. Magnus,
>> > what do you think?
>>
>> I prefer you to use a hardware-block version compatible suffix instead
>> of SoC suffix.
>>
>> This since we have more SoCs than actual hardware block
>> configurations. Using the list above, how many configurations would we
>> have?
>>
>> Actually, forcing the drivers to be updated for each new SoC sounds
>> like a pretty terrible idea. Wouldn't that be against one of the
>> merits of using DT? Also, don't you have enough interesting work piled
>> up already? =)
>>
>> Basically, I can't see any point in adding an extra unnecessary need
>> for updating the drivers when there is no real functional change.
>
> My understanding is that the discussion is about the details of
> bindings that are required for SDHI to function when brought
> up using DT on a variety of boards. Not exciting new work.
>
> In particular how to set TMIO_MMC_HAS_IDLE_WAIT via DT.

No, not exciting new work. More describing certain versions of the
SDHI hardware. This is a SDHI-specific configuration which may end up
being used on a certain SoC. There are also some board specific
details that need to be taken into consideration. I believe it is
important to understand the difference between hardware-block
configuration (SDHI in this particular case), SoC and board.

So the way I see it we have 3 ways to deal with it:

1) Use the "toshiba,mmc-has-idle-wait" property proposed by Guennadi
or
2) Use a SoC suffix in the compatible string and deal with
TMIO_MMC_HAS_IDLE_WAIT in the driver
or
3) Use a SDHI-specific version suffix in the compatible string and
deal with TMIO_MMC_HAS_IDLE_WAIT in the driver

I am fine with 1) or 3) but I don't want to go down the route of 2)
because it will just lead to more pointless driver changes than are
actually needed. And the TMIO_MMC_HAS_IDLE_WAIT flag is not
SoC-specific so using a SoC suffix seems incorrect to me.

Thanks,

/ magnus

Re: [PATCH v3 06/13] mmc: tmio-mmc: define device-tree bindings

[Simon Horman]

On Thu, Feb 14, 2013 at 11:09:06AM +0900, Magnus Damm wrote:
> On Thu, Feb 14, 2013 at 10:59 AM, Simon Horman <horms@verge.net.au> wrote:
> > On Thu, Feb 14, 2013 at 10:42:21AM +0900, Magnus Damm wrote:
> >> On Thu, Feb 14, 2013 at 12:59 AM, Guennadi Liakhovetski
> >> <g.liakhovetski@gmx.de> wrote:
> >> > On Thu, 7 Feb 2013, Simon Horman wrote:
> >> >
> >> >> On Wed, Feb 06, 2013 at 10:24:20PM +0000, Arnd Bergmann wrote:
> >> >> > On Wednesday 06 February 2013, Guennadi Liakhovetski wrote:
> >> >> > > +* Toshiba Mobile IO SD/MMC controller
> >> >> > > +
> >> >> > > +The tmio-mmc driver doesn't probe its devices actively, instead its binding to
> >> >> > > +devices is managed by either MFD drivers or by the sh_mobile_sdhi platform
> >> >> > > +driver. Those drivers supply the tmio-mmc driver with platform data, that either
> >> >> > > +describe hardware capabilities, known to them, or are obtained by them from
> >> >> > > +their own platform data or from their DT information. In the latter case all
> >> >> > > +compulsory and any optional properties, common to all SD/MMC drivers, as
> >> >> > > +described in mmc.txt, should or can be used. Additionally the following optional
> >> >> > > +bindings can be used. They set respective TMIO_MMC_* flags.
> >> >> > > +
> >> >> > > +Optional properties:
> >> >> > > +- toshiba,mmc-wrprotect-disable        : set TMIO_MMC_WRPROTECT_DISABLE flag
> >> >> > > +- toshiba,mmc-blksz-2bytes     : set TMIO_MMC_BLKSZ_2BYTES
> >> >> > > +- toshiba,mmc-has-idle-wait    : set TMIO_MMC_HAS_IDLE_WAIT
> >> >> >
> >> >> > Please write the binding in a way that does not refer to a specific
> >> >> > implementation in Linux: The binding should describe the hardware
> >> >> > independent of details in the driver. In particular, I think you
> >> >> > should not refer to the TMIO_MMC_BLKSZ_2BYTES etc macros but describe
> >> >> > in text what the flags are about.
> >> >> >
> >> >> > Regarding the toshiba,mmc-wrprotect-disable property, would it be
> >> >> > enough to just check the presence of the wp-gpios property?
> >> >> >
> >> >> > TMIO_MMC_BLKSZ_2BYTES seems to be set unconditionally in
> >> >> > sh_mobile_sdhi_probe and nowhere else, so I'd assume we don't
> >> >> > actually need to provide this here, but can keep that knowledge
> >> >> > implicit based on whether we're talking to sh_mobile_sdhi
> >> >> > or another tmio_mmc variant.
> >> >
> >> > Can do that, yes.
> >> >
> >> >> > For the other last one, is that actually board specific, or just
> >> >> > a feature of a given chip? If we can tell by the SoC, then I'd
> >> >> > suggest using separate "compatible" properties instead, and
> >> >> > put a bitmask of features into the .data field of the of match
> >> >> > table. For all I can tell, SH7372 does not set it, while SH73A0,
> >> >> > R8A7740 and R8A7779 always do.
> >> >>
> >> >> My understanding is that TMIO_MMC_HAS_IDLE_WAIT can be set based
> >> >> on the SoC in use.
> >> >
> >> > So far TMIO_MMC_HAS_IDLE_WAIT is set on
> >> >
> >> > board-kzm9g.c (sh73a0 / AG5)
> >> > board-ag5evm.c (sh73a0 / AG5)
> >> > board-armadillo800eva.c (r8a7740 / A1)
> >> > board-kota2.c (sh73a0 / AG5)
> >> > board-marzen.c (r8a7779 / H1)
> >> >
> >> > and isn't set on
> >> >
> >> > board-ap4evb.c (sh7372 / ap4)
> >> > board-bonito.c (r8a7740 / a1, SDHI isn't used)
> >> > board-mackerel.c (sh7372 / ap4)
> >> >
> >> > So, shall we use a compatible property for this and drop this property? We
> >> > can add later at any time, if needed, which is better, than defining
> >> > something redundant. OTOH I seem to remember, that using SoC-version from
> >> > the "compatible" property was considered by someone inappropriate. Magnus,
> >> > what do you think?
> >>
> >> I prefer you to use a hardware-block version compatible suffix instead
> >> of SoC suffix.
> >>
> >> This since we have more SoCs than actual hardware block
> >> configurations. Using the list above, how many configurations would we
> >> have?
> >>
> >> Actually, forcing the drivers to be updated for each new SoC sounds
> >> like a pretty terrible idea. Wouldn't that be against one of the
> >> merits of using DT? Also, don't you have enough interesting work piled
> >> up already? =)
> >>
> >> Basically, I can't see any point in adding an extra unnecessary need
> >> for updating the drivers when there is no real functional change.
> >
> > My understanding is that the discussion is about the details of
> > bindings that are required for SDHI to function when brought
> > up using DT on a variety of boards. Not exciting new work.
> >
> > In particular how to set TMIO_MMC_HAS_IDLE_WAIT via DT.
> 
> No, not exciting new work. More describing certain versions of the
> SDHI hardware. This is a SDHI-specific configuration which may end up
> being used on a certain SoC. There are also some board specific
> details that need to be taken into consideration. I believe it is
> important to understand the difference between hardware-block
> configuration (SDHI in this particular case), SoC and board.
> 
> So the way I see it we have 3 ways to deal with it:
> 
> 1) Use the "toshiba,mmc-has-idle-wait" property proposed by Guennadi
> or
> 2) Use a SoC suffix in the compatible string and deal with
> TMIO_MMC_HAS_IDLE_WAIT in the driver
> or
> 3) Use a SDHI-specific version suffix in the compatible string and
> deal with TMIO_MMC_HAS_IDLE_WAIT in the driver
> 
> I am fine with 1) or 3) but I don't want to go down the route of 2)
> because it will just lead to more pointless driver changes than are
> actually needed. And the TMIO_MMC_HAS_IDLE_WAIT flag is not
> SoC-specific so using a SoC suffix seems incorrect to me.

It seems that 3) coincides best with your desires and Arnd's feedback to date.

Re: [PATCH v3 06/13] mmc: tmio-mmc: define device-tree bindings

[Guennadi Liakhovetski]

On Thu, 14 Feb 2013, Magnus Damm wrote:

> On Thu, Feb 14, 2013 at 10:59 AM, Simon Horman <horms@verge.net.au> wrote:
> > On Thu, Feb 14, 2013 at 10:42:21AM +0900, Magnus Damm wrote:
> >> On Thu, Feb 14, 2013 at 12:59 AM, Guennadi Liakhovetski
> >> <g.liakhovetski@gmx.de> wrote:
> >> > On Thu, 7 Feb 2013, Simon Horman wrote:
> >> >
> >> >> On Wed, Feb 06, 2013 at 10:24:20PM +0000, Arnd Bergmann wrote:
> >> >> > On Wednesday 06 February 2013, Guennadi Liakhovetski wrote:
> >> >> > > +* Toshiba Mobile IO SD/MMC controller
> >> >> > > +
> >> >> > > +The tmio-mmc driver doesn't probe its devices actively, instead its binding to
> >> >> > > +devices is managed by either MFD drivers or by the sh_mobile_sdhi platform
> >> >> > > +driver. Those drivers supply the tmio-mmc driver with platform data, that either
> >> >> > > +describe hardware capabilities, known to them, or are obtained by them from
> >> >> > > +their own platform data or from their DT information. In the latter case all
> >> >> > > +compulsory and any optional properties, common to all SD/MMC drivers, as
> >> >> > > +described in mmc.txt, should or can be used. Additionally the following optional
> >> >> > > +bindings can be used. They set respective TMIO_MMC_* flags.
> >> >> > > +
> >> >> > > +Optional properties:
> >> >> > > +- toshiba,mmc-wrprotect-disable        : set TMIO_MMC_WRPROTECT_DISABLE flag
> >> >> > > +- toshiba,mmc-blksz-2bytes     : set TMIO_MMC_BLKSZ_2BYTES
> >> >> > > +- toshiba,mmc-has-idle-wait    : set TMIO_MMC_HAS_IDLE_WAIT
> >> >> >
> >> >> > Please write the binding in a way that does not refer to a specific
> >> >> > implementation in Linux: The binding should describe the hardware
> >> >> > independent of details in the driver. In particular, I think you
> >> >> > should not refer to the TMIO_MMC_BLKSZ_2BYTES etc macros but describe
> >> >> > in text what the flags are about.
> >> >> >
> >> >> > Regarding the toshiba,mmc-wrprotect-disable property, would it be
> >> >> > enough to just check the presence of the wp-gpios property?

No, normally WP would be implemented, using the native dedicated SDHI WP 
pin. So, toshiba,mmc-wrprotect-disable is used when no such pin is 
available on this interface, or it's not routed to the socket and no GPIO 
is used for that either. So, this flag is the only way to know, whether a 
WP pin is used, if no wp-gpios is used.

> >> >> > TMIO_MMC_BLKSZ_2BYTES seems to be set unconditionally in
> >> >> > sh_mobile_sdhi_probe and nowhere else, so I'd assume we don't
> >> >> > actually need to provide this here, but can keep that knowledge
> >> >> > implicit based on whether we're talking to sh_mobile_sdhi
> >> >> > or another tmio_mmc variant.
> >> >
> >> > Can do that, yes.
> >> >
> >> >> > For the other last one, is that actually board specific, or just
> >> >> > a feature of a given chip? If we can tell by the SoC, then I'd
> >> >> > suggest using separate "compatible" properties instead, and
> >> >> > put a bitmask of features into the .data field of the of match
> >> >> > table. For all I can tell, SH7372 does not set it, while SH73A0,
> >> >> > R8A7740 and R8A7779 always do.
> >> >>
> >> >> My understanding is that TMIO_MMC_HAS_IDLE_WAIT can be set based
> >> >> on the SoC in use.
> >> >
> >> > So far TMIO_MMC_HAS_IDLE_WAIT is set on
> >> >
> >> > board-kzm9g.c (sh73a0 / AG5)
> >> > board-ag5evm.c (sh73a0 / AG5)
> >> > board-armadillo800eva.c (r8a7740 / A1)
> >> > board-kota2.c (sh73a0 / AG5)
> >> > board-marzen.c (r8a7779 / H1)
> >> >
> >> > and isn't set on
> >> >
> >> > board-ap4evb.c (sh7372 / ap4)
> >> > board-bonito.c (r8a7740 / a1, SDHI isn't used)
> >> > board-mackerel.c (sh7372 / ap4)
> >> >
> >> > So, shall we use a compatible property for this and drop this property? We
> >> > can add later at any time, if needed, which is better, than defining
> >> > something redundant. OTOH I seem to remember, that using SoC-version from
> >> > the "compatible" property was considered by someone inappropriate. Magnus,
> >> > what do you think?
> >>
> >> I prefer you to use a hardware-block version compatible suffix instead
> >> of SoC suffix.
> >>
> >> This since we have more SoCs than actual hardware block
> >> configurations. Using the list above, how many configurations would we
> >> have?
> >>
> >> Actually, forcing the drivers to be updated for each new SoC sounds
> >> like a pretty terrible idea. Wouldn't that be against one of the
> >> merits of using DT? Also, don't you have enough interesting work piled
> >> up already? =)
> >>
> >> Basically, I can't see any point in adding an extra unnecessary need
> >> for updating the drivers when there is no real functional change.
> >
> > My understanding is that the discussion is about the details of
> > bindings that are required for SDHI to function when brought
> > up using DT on a variety of boards. Not exciting new work.
> >
> > In particular how to set TMIO_MMC_HAS_IDLE_WAIT via DT.
> 
> No, not exciting new work. More describing certain versions of the
> SDHI hardware. This is a SDHI-specific configuration which may end up
> being used on a certain SoC. There are also some board specific
> details that need to be taken into consideration. I believe it is
> important to understand the difference between hardware-block
> configuration (SDHI in this particular case), SoC and board.
> 
> So the way I see it we have 3 ways to deal with it:
> 
> 1) Use the "toshiba,mmc-has-idle-wait" property proposed by Guennadi
> or
> 2) Use a SoC suffix in the compatible string and deal with
> TMIO_MMC_HAS_IDLE_WAIT in the driver
> or
> 3) Use a SDHI-specific version suffix in the compatible string and
> deal with TMIO_MMC_HAS_IDLE_WAIT in the driver
> 
> I am fine with 1) or 3) but I don't want to go down the route of 2)
> because it will just lead to more pointless driver changes than are
> actually needed. And the TMIO_MMC_HAS_IDLE_WAIT flag is not
> SoC-specific so using a SoC suffix seems incorrect to me.

My take on this is the following: having N optionally available on 
different IP-versions features, I'd rather have N DT properties, than up 
to 2^N abstract vX versions. Yes, I realise, that in practice we'll never 
have 2^N, rather just a bit more than N, still, my main problem with those 
versions, is that they are purely abstracted. I'd be happy if that was a 
real hardware revision string, that you could look up in a datasheet. But 
if you have to look in some text file... That just seems much less 
user-friendly to me, than selecting single properties. Just imagine, what 
would you prefer, either specifying

	feature-A;
	feature-B;

in your .dts or going through a list of

v1: has no features
v2: feature A only
v3: feature B only
v4: features A and B
...

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH v3 06/13] mmc: tmio-mmc: define device-tree bindings

[Magnus Damm]

Hi Guennadi,

[Added Laurent to CC]

On Thu, Feb 14, 2013 at 5:28 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> On Thu, 14 Feb 2013, Magnus Damm wrote:
>
>> On Thu, Feb 14, 2013 at 10:59 AM, Simon Horman <horms@verge.net.au> wrote:
>> > On Thu, Feb 14, 2013 at 10:42:21AM +0900, Magnus Damm wrote:
>> >> On Thu, Feb 14, 2013 at 12:59 AM, Guennadi Liakhovetski
>> >> <g.liakhovetski@gmx.de> wrote:
>> >> > On Thu, 7 Feb 2013, Simon Horman wrote:
>> >> >
>> >> >> On Wed, Feb 06, 2013 at 10:24:20PM +0000, Arnd Bergmann wrote:
>> >> >> > On Wednesday 06 February 2013, Guennadi Liakhovetski wrote:
>> >> >> > > +* Toshiba Mobile IO SD/MMC controller
>> >> >> > > +
>> >> >> > > +The tmio-mmc driver doesn't probe its devices actively, instead its binding to
>> >> >> > > +devices is managed by either MFD drivers or by the sh_mobile_sdhi platform
>> >> >> > > +driver. Those drivers supply the tmio-mmc driver with platform data, that either
>> >> >> > > +describe hardware capabilities, known to them, or are obtained by them from
>> >> >> > > +their own platform data or from their DT information. In the latter case all
>> >> >> > > +compulsory and any optional properties, common to all SD/MMC drivers, as
>> >> >> > > +described in mmc.txt, should or can be used. Additionally the following optional
>> >> >> > > +bindings can be used. They set respective TMIO_MMC_* flags.
>> >> >> > > +
>> >> >> > > +Optional properties:
>> >> >> > > +- toshiba,mmc-wrprotect-disable        : set TMIO_MMC_WRPROTECT_DISABLE flag
>> >> >> > > +- toshiba,mmc-blksz-2bytes     : set TMIO_MMC_BLKSZ_2BYTES
>> >> >> > > +- toshiba,mmc-has-idle-wait    : set TMIO_MMC_HAS_IDLE_WAIT
>> >> >> >
>> >> >> > Please write the binding in a way that does not refer to a specific
>> >> >> > implementation in Linux: The binding should describe the hardware
>> >> >> > independent of details in the driver. In particular, I think you
>> >> >> > should not refer to the TMIO_MMC_BLKSZ_2BYTES etc macros but describe
>> >> >> > in text what the flags are about.
>> >> >> >
>> >> >> > Regarding the toshiba,mmc-wrprotect-disable property, would it be
>> >> >> > enough to just check the presence of the wp-gpios property?
>
> No, normally WP would be implemented, using the native dedicated SDHI WP
> pin. So, toshiba,mmc-wrprotect-disable is used when no such pin is
> available on this interface, or it's not routed to the socket and no GPIO
> is used for that either. So, this flag is the only way to know, whether a
> WP pin is used, if no wp-gpios is used.

Can't we request this information via PINCTRL somehow? I feel that I
may have asked this before, so correct me if I am going in circles.

If we have a native SDHI WP pin and it is used on our board then this
information is known with the board configuration already. And that
pin function needs to be select. Same thing if we happen to have a
GPIO pin for WP, then that need to be selected as well. And if we have
neither SDHI WP nor GPIO WP then we don't have to bother with the WP
signal at all.

>> >> >> > TMIO_MMC_BLKSZ_2BYTES seems to be set unconditionally in
>> >> >> > sh_mobile_sdhi_probe and nowhere else, so I'd assume we don't
>> >> >> > actually need to provide this here, but can keep that knowledge
>> >> >> > implicit based on whether we're talking to sh_mobile_sdhi
>> >> >> > or another tmio_mmc variant.
>> >> >
>> >> > Can do that, yes.
>> >> >
>> >> >> > For the other last one, is that actually board specific, or just
>> >> >> > a feature of a given chip? If we can tell by the SoC, then I'd
>> >> >> > suggest using separate "compatible" properties instead, and
>> >> >> > put a bitmask of features into the .data field of the of match
>> >> >> > table. For all I can tell, SH7372 does not set it, while SH73A0,
>> >> >> > R8A7740 and R8A7779 always do.
>> >> >>
>> >> >> My understanding is that TMIO_MMC_HAS_IDLE_WAIT can be set based
>> >> >> on the SoC in use.
>> >> >
>> >> > So far TMIO_MMC_HAS_IDLE_WAIT is set on
>> >> >
>> >> > board-kzm9g.c (sh73a0 / AG5)
>> >> > board-ag5evm.c (sh73a0 / AG5)
>> >> > board-armadillo800eva.c (r8a7740 / A1)
>> >> > board-kota2.c (sh73a0 / AG5)
>> >> > board-marzen.c (r8a7779 / H1)
>> >> >
>> >> > and isn't set on
>> >> >
>> >> > board-ap4evb.c (sh7372 / ap4)
>> >> > board-bonito.c (r8a7740 / a1, SDHI isn't used)
>> >> > board-mackerel.c (sh7372 / ap4)
>> >> >
>> >> > So, shall we use a compatible property for this and drop this property? We
>> >> > can add later at any time, if needed, which is better, than defining
>> >> > something redundant. OTOH I seem to remember, that using SoC-version from
>> >> > the "compatible" property was considered by someone inappropriate. Magnus,
>> >> > what do you think?
>> >>
>> >> I prefer you to use a hardware-block version compatible suffix instead
>> >> of SoC suffix.
>> >>
>> >> This since we have more SoCs than actual hardware block
>> >> configurations. Using the list above, how many configurations would we
>> >> have?
>> >>
>> >> Actually, forcing the drivers to be updated for each new SoC sounds
>> >> like a pretty terrible idea. Wouldn't that be against one of the
>> >> merits of using DT? Also, don't you have enough interesting work piled
>> >> up already? =)
>> >>
>> >> Basically, I can't see any point in adding an extra unnecessary need
>> >> for updating the drivers when there is no real functional change.
>> >
>> > My understanding is that the discussion is about the details of
>> > bindings that are required for SDHI to function when brought
>> > up using DT on a variety of boards. Not exciting new work.
>> >
>> > In particular how to set TMIO_MMC_HAS_IDLE_WAIT via DT.
>>
>> No, not exciting new work. More describing certain versions of the
>> SDHI hardware. This is a SDHI-specific configuration which may end up
>> being used on a certain SoC. There are also some board specific
>> details that need to be taken into consideration. I believe it is
>> important to understand the difference between hardware-block
>> configuration (SDHI in this particular case), SoC and board.
>>
>> So the way I see it we have 3 ways to deal with it:
>>
>> 1) Use the "toshiba,mmc-has-idle-wait" property proposed by Guennadi
>> or
>> 2) Use a SoC suffix in the compatible string and deal with
>> TMIO_MMC_HAS_IDLE_WAIT in the driver
>> or
>> 3) Use a SDHI-specific version suffix in the compatible string and
>> deal with TMIO_MMC_HAS_IDLE_WAIT in the driver
>>
>> I am fine with 1) or 3) but I don't want to go down the route of 2)
>> because it will just lead to more pointless driver changes than are
>> actually needed. And the TMIO_MMC_HAS_IDLE_WAIT flag is not
>> SoC-specific so using a SoC suffix seems incorrect to me.
>
> My take on this is the following: having N optionally available on
> different IP-versions features, I'd rather have N DT properties, than up
> to 2^N abstract vX versions. Yes, I realise, that in practice we'll never
> have 2^N, rather just a bit more than N, still, my main problem with those
> versions, is that they are purely abstracted. I'd be happy if that was a
> real hardware revision string, that you could look up in a datasheet. But
> if you have to look in some text file... That just seems much less
> user-friendly to me, than selecting single properties. Just imagine, what
> would you prefer, either specifying
>
>         feature-A;
>         feature-B;
>
> in your .dts or going through a list of
>
> v1: has no features
> v2: feature A only
> v3: feature B only
> v4: features A and B
> ...

In this particular case based on the information above:

toshiba,mmc-blksz-2bytes
toshiba,mmc-has-idle-wait

or my guess:

renesas,shmobile-sdhi-v1: TMIO_MMC_BLKSZ_2BYTES
renesas,shmobile-sdhi-v2: TMIO_MMC_BLKSZ_2BYTES + TMIO_MMC_HAS_IDLE_WAIT

I think the WP and CD pins should be handled differently if possible.

Thanks,

/ magnus

Re: [PATCH v3 06/13] mmc: tmio-mmc: define device-tree bindings

[Guennadi Liakhovetski]

Hi Magnus

On Thu, 14 Feb 2013, Magnus Damm wrote:

> Hi Guennadi,
> 
> [Added Laurent to CC]
> 
> On Thu, Feb 14, 2013 at 5:28 PM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > On Thu, 14 Feb 2013, Magnus Damm wrote:
> >
> >> On Thu, Feb 14, 2013 at 10:59 AM, Simon Horman <horms@verge.net.au> wrote:
> >> > On Thu, Feb 14, 2013 at 10:42:21AM +0900, Magnus Damm wrote:
> >> >> On Thu, Feb 14, 2013 at 12:59 AM, Guennadi Liakhovetski
> >> >> <g.liakhovetski@gmx.de> wrote:
> >> >> > On Thu, 7 Feb 2013, Simon Horman wrote:
> >> >> >
> >> >> >> On Wed, Feb 06, 2013 at 10:24:20PM +0000, Arnd Bergmann wrote:
> >> >> >> > On Wednesday 06 February 2013, Guennadi Liakhovetski wrote:
> >> >> >> > > +* Toshiba Mobile IO SD/MMC controller
> >> >> >> > > +
> >> >> >> > > +The tmio-mmc driver doesn't probe its devices actively, instead its binding to
> >> >> >> > > +devices is managed by either MFD drivers or by the sh_mobile_sdhi platform
> >> >> >> > > +driver. Those drivers supply the tmio-mmc driver with platform data, that either
> >> >> >> > > +describe hardware capabilities, known to them, or are obtained by them from
> >> >> >> > > +their own platform data or from their DT information. In the latter case all
> >> >> >> > > +compulsory and any optional properties, common to all SD/MMC drivers, as
> >> >> >> > > +described in mmc.txt, should or can be used. Additionally the following optional
> >> >> >> > > +bindings can be used. They set respective TMIO_MMC_* flags.
> >> >> >> > > +
> >> >> >> > > +Optional properties:
> >> >> >> > > +- toshiba,mmc-wrprotect-disable        : set TMIO_MMC_WRPROTECT_DISABLE flag
> >> >> >> > > +- toshiba,mmc-blksz-2bytes     : set TMIO_MMC_BLKSZ_2BYTES
> >> >> >> > > +- toshiba,mmc-has-idle-wait    : set TMIO_MMC_HAS_IDLE_WAIT
> >> >> >> >
> >> >> >> > Please write the binding in a way that does not refer to a specific
> >> >> >> > implementation in Linux: The binding should describe the hardware
> >> >> >> > independent of details in the driver. In particular, I think you
> >> >> >> > should not refer to the TMIO_MMC_BLKSZ_2BYTES etc macros but describe
> >> >> >> > in text what the flags are about.
> >> >> >> >
> >> >> >> > Regarding the toshiba,mmc-wrprotect-disable property, would it be
> >> >> >> > enough to just check the presence of the wp-gpios property?
> >
> > No, normally WP would be implemented, using the native dedicated SDHI WP
> > pin. So, toshiba,mmc-wrprotect-disable is used when no such pin is
> > available on this interface, or it's not routed to the socket and no GPIO
> > is used for that either. So, this flag is the only way to know, whether a
> > WP pin is used, if no wp-gpios is used.
> 
> Can't we request this information via PINCTRL somehow? I feel that I
> may have asked this before, so correct me if I am going in circles.

Yes, you have. And we did want to do that, and I even proposed patches, 
but the status hasn't changed, I'm afraid: the pinctrl maintainer doesn't 
want that API, he considers it going against the pinctrl concept. He 
doesn't want drivers to inquire pinctrl about the status... What you could 
do though, you could probably create a special "wp" configuration and try 
to request that and see whether that succeeds... I think there was a 
gotcha with that too, which is why you normally only want to request the 
default settings. But maybe this can be reconsidered. I hope I passed the 
contents of that old discussion correctly, if not - sorry, feel free to 
correct me.

> If we have a native SDHI WP pin and it is used on our board then this
> information is known with the board configuration already. And that
> pin function needs to be select. Same thing if we happen to have a
> GPIO pin for WP, then that need to be selected as well. And if we have
> neither SDHI WP nor GPIO WP then we don't have to bother with the WP
> signal at all.
> 
> >> >> >> > TMIO_MMC_BLKSZ_2BYTES seems to be set unconditionally in
> >> >> >> > sh_mobile_sdhi_probe and nowhere else, so I'd assume we don't
> >> >> >> > actually need to provide this here, but can keep that knowledge
> >> >> >> > implicit based on whether we're talking to sh_mobile_sdhi
> >> >> >> > or another tmio_mmc variant.
> >> >> >
> >> >> > Can do that, yes.
> >> >> >
> >> >> >> > For the other last one, is that actually board specific, or just
> >> >> >> > a feature of a given chip? If we can tell by the SoC, then I'd
> >> >> >> > suggest using separate "compatible" properties instead, and
> >> >> >> > put a bitmask of features into the .data field of the of match
> >> >> >> > table. For all I can tell, SH7372 does not set it, while SH73A0,
> >> >> >> > R8A7740 and R8A7779 always do.
> >> >> >>
> >> >> >> My understanding is that TMIO_MMC_HAS_IDLE_WAIT can be set based
> >> >> >> on the SoC in use.
> >> >> >
> >> >> > So far TMIO_MMC_HAS_IDLE_WAIT is set on
> >> >> >
> >> >> > board-kzm9g.c (sh73a0 / AG5)
> >> >> > board-ag5evm.c (sh73a0 / AG5)
> >> >> > board-armadillo800eva.c (r8a7740 / A1)
> >> >> > board-kota2.c (sh73a0 / AG5)
> >> >> > board-marzen.c (r8a7779 / H1)
> >> >> >
> >> >> > and isn't set on
> >> >> >
> >> >> > board-ap4evb.c (sh7372 / ap4)
> >> >> > board-bonito.c (r8a7740 / a1, SDHI isn't used)
> >> >> > board-mackerel.c (sh7372 / ap4)
> >> >> >
> >> >> > So, shall we use a compatible property for this and drop this property? We
> >> >> > can add later at any time, if needed, which is better, than defining
> >> >> > something redundant. OTOH I seem to remember, that using SoC-version from
> >> >> > the "compatible" property was considered by someone inappropriate. Magnus,
> >> >> > what do you think?
> >> >>
> >> >> I prefer you to use a hardware-block version compatible suffix instead
> >> >> of SoC suffix.
> >> >>
> >> >> This since we have more SoCs than actual hardware block
> >> >> configurations. Using the list above, how many configurations would we
> >> >> have?
> >> >>
> >> >> Actually, forcing the drivers to be updated for each new SoC sounds
> >> >> like a pretty terrible idea. Wouldn't that be against one of the
> >> >> merits of using DT? Also, don't you have enough interesting work piled
> >> >> up already? =)
> >> >>
> >> >> Basically, I can't see any point in adding an extra unnecessary need
> >> >> for updating the drivers when there is no real functional change.
> >> >
> >> > My understanding is that the discussion is about the details of
> >> > bindings that are required for SDHI to function when brought
> >> > up using DT on a variety of boards. Not exciting new work.
> >> >
> >> > In particular how to set TMIO_MMC_HAS_IDLE_WAIT via DT.
> >>
> >> No, not exciting new work. More describing certain versions of the
> >> SDHI hardware. This is a SDHI-specific configuration which may end up
> >> being used on a certain SoC. There are also some board specific
> >> details that need to be taken into consideration. I believe it is
> >> important to understand the difference between hardware-block
> >> configuration (SDHI in this particular case), SoC and board.
> >>
> >> So the way I see it we have 3 ways to deal with it:
> >>
> >> 1) Use the "toshiba,mmc-has-idle-wait" property proposed by Guennadi
> >> or
> >> 2) Use a SoC suffix in the compatible string and deal with
> >> TMIO_MMC_HAS_IDLE_WAIT in the driver
> >> or
> >> 3) Use a SDHI-specific version suffix in the compatible string and
> >> deal with TMIO_MMC_HAS_IDLE_WAIT in the driver
> >>
> >> I am fine with 1) or 3) but I don't want to go down the route of 2)
> >> because it will just lead to more pointless driver changes than are
> >> actually needed. And the TMIO_MMC_HAS_IDLE_WAIT flag is not
> >> SoC-specific so using a SoC suffix seems incorrect to me.
> >
> > My take on this is the following: having N optionally available on
> > different IP-versions features, I'd rather have N DT properties, than up
> > to 2^N abstract vX versions. Yes, I realise, that in practice we'll never
> > have 2^N, rather just a bit more than N, still, my main problem with those
> > versions, is that they are purely abstracted. I'd be happy if that was a
> > real hardware revision string, that you could look up in a datasheet. But
> > if you have to look in some text file... That just seems much less
> > user-friendly to me, than selecting single properties. Just imagine, what
> > would you prefer, either specifying
> >
> >         feature-A;
> >         feature-B;
> >
> > in your .dts or going through a list of
> >
> > v1: has no features
> > v2: feature A only
> > v3: feature B only
> > v4: features A and B
> > ...
> 
> In this particular case based on the information above:
> 
> toshiba,mmc-blksz-2bytes
> toshiba,mmc-has-idle-wait
> 
> or my guess:
> 
> renesas,shmobile-sdhi-v1: TMIO_MMC_BLKSZ_2BYTES
> renesas,shmobile-sdhi-v2: TMIO_MMC_BLKSZ_2BYTES + TMIO_MMC_HAS_IDLE_WAIT

toshiba,mmc-blksz-2bytes we don't need - it's set for all SDHI devices. 
So, _so far_ we only have one feature, that we have to specify in DT 
either via a property or a compatible version string.

> I think the WP and CD pins should be handled differently if possible.

WP we're currently discussing, for CD there's already a common MMC 
binding: broken-cd.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH v3 06/13] mmc: tmio-mmc: define device-tree bindings

[Arnd Bergmann]

On Thursday 14 February 2013, Guennadi Liakhovetski wrote:
> On Thu, 14 Feb 2013, Magnus Damm wrote:
> > On Thu, Feb 14, 2013 at 5:28 PM, Guennadi Liakhovetski
> > > My take on this is the following: having N optionally available on
> > > different IP-versions features, I'd rather have N DT properties, than up
> > > to 2^N abstract vX versions. Yes, I realise, that in practice we'll never
> > > have 2^N, rather just a bit more than N, still, my main problem with those
> > > versions, is that they are purely abstracted. I'd be happy if that was a
> > > real hardware revision string, that you could look up in a datasheet. But
> > > if you have to look in some text file... That just seems much less
> > > user-friendly to me, than selecting single properties. Just imagine, what
> > > would you prefer, either specifying
> > >
> > >         feature-A;
> > >         feature-B;
> > >
> > > in your .dts or going through a list of
> > >
> > > v1: has no features
> > > v2: feature A only
> > > v3: feature B only
> > > v4: features A and B
> > > ...
> > 
> > In this particular case based on the information above:
> > 
> > toshiba,mmc-blksz-2bytes
> > toshiba,mmc-has-idle-wait
> > 
> > or my guess:
> > 
> > renesas,shmobile-sdhi-v1: TMIO_MMC_BLKSZ_2BYTES
> > renesas,shmobile-sdhi-v2: TMIO_MMC_BLKSZ_2BYTES + TMIO_MMC_HAS_IDLE_WAIT
> 
> toshiba,mmc-blksz-2bytes we don't need - it's set for all SDHI devices. 
> So, _so far_ we only have one feature, that we have to specify in DT 
> either via a property or a compatible version string.

Right. I would still prefer the different "compatible" property, but it's not
an extremely important thing here. Regarding the naming of the "compatible"
string, I would not use v1 and v2 postfixes though, unless that is what they
are called in the data sheet. Ideally we would know the exact version
of the IP block that went into the chip and drop the "shmobile-" part.

That would make it something like "toshiba,sdhi-1.23.45". If you cannot
easily find out those versions, the next best solution would be naming it
after the chip that first used a particular version of that IP block,
like "renesas,shmobile1234-sdhi". The device tree include file for
shmobile5678 should then list the sdhi part as being compatible with
the "reneasas,shmobile1234-sdhi" if they are the same, or as
a separate "reneasas,shmobile5678-sdhi" if they behave in a different
way. In either case, I would also list the "toshiba,sdhi" name as
the more generic option in the "compatible" list, but that is optional.

	Arnd

[PATCH v3 07/13] mmc: tmio-mmc: parse device-tree bindings

[Guennadi Liakhovetski]

Add parsing of common and driver-specific DT bindings to the tmio-mmc
MMC host driver.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Arnd Bergmann <arnd@arndb.de>
---

v3: remove the "toshiba,mmc-cap-sdio-irq" property

 drivers/mmc/host/tmio_mmc_pio.c |   24 ++++++++++++++++++++++--
 1 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index b25adb4..e32e9b4 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -918,6 +918,21 @@ static void tmio_mmc_init_ocr(struct tmio_mmc_host *host)
 		dev_warn(mmc_dev(mmc), "Platform OCR mask is ignored\n");
 }
 
+static void tmio_mmc_of_parse(struct platform_device *pdev,
+			      struct tmio_mmc_data *pdata)
+{
+	const struct device_node *np = pdev->dev.of_node;
+	if (!np)
+		return;
+
+	if (of_get_property(np, "toshiba,mmc-wrprotect-disable", NULL))
+		pdata->flags |= TMIO_MMC_WRPROTECT_DISABLE;
+	if (of_get_property(np, "toshiba,mmc-blksz-2bytes", NULL))
+		pdata->flags |= TMIO_MMC_BLKSZ_2BYTES;
+	if (of_get_property(np, "toshiba,mmc-has-idle-wait", NULL))
+		pdata->flags |= TMIO_MMC_HAS_IDLE_WAIT;
+}
+
 int tmio_mmc_host_probe(struct tmio_mmc_host **host,
 				  struct platform_device *pdev,
 				  struct tmio_mmc_data *pdata)
@@ -928,6 +943,8 @@ int tmio_mmc_host_probe(struct tmio_mmc_host **host,
 	int ret;
 	u32 irq_mask = TMIO_MASK_CMD;
 
+	tmio_mmc_of_parse(pdev, pdata);
+
 	if (!(pdata->flags & TMIO_MMC_HAS_IDLE_WAIT))
 		pdata->write16_hook = NULL;
 
@@ -939,6 +956,8 @@ int tmio_mmc_host_probe(struct tmio_mmc_host **host,
 	if (!mmc)
 		return -ENOMEM;
 
+	mmc_of_parse(mmc);
+
 	pdata->dev = &pdev->dev;
 	_host = mmc_priv(mmc);
 	_host->pdata = pdata;
@@ -959,7 +978,7 @@ int tmio_mmc_host_probe(struct tmio_mmc_host **host,
 	}
 
 	mmc->ops = &tmio_mmc_ops;
-	mmc->caps = MMC_CAP_4_BIT_DATA | pdata->capabilities;
+	mmc->caps |= MMC_CAP_4_BIT_DATA | pdata->capabilities;
 	mmc->caps2 = pdata->capabilities2;
 	mmc->max_segs = 32;
 	mmc->max_blk_size = 512;
@@ -971,7 +990,8 @@ int tmio_mmc_host_probe(struct tmio_mmc_host **host,
 
 	_host->native_hotplug = !(pdata->flags & TMIO_MMC_USE_GPIO_CD ||
 				  mmc->caps & MMC_CAP_NEEDS_POLL ||
-				  mmc->caps & MMC_CAP_NONREMOVABLE);
+				  mmc->caps & MMC_CAP_NONREMOVABLE ||
+				  mmc->slot.cd_irq >= 0);
 
 	_host->power = false;
 	pm_runtime_enable(&pdev->dev);
-- 
1.7.2.5

[PATCH v3 08/13] mmc: sh_mobile_sdhi: remove unused .pdata field

[Guennadi Liakhovetski]

The struct sh_mobile_sdhi_info::pdata field was only used for platform-
based card detection and isn't used anymore since the migration to GPIO-
based MMC slot functions. Remove it.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/mmc/host/sh_mobile_sdhi.c  |    4 ----
 include/linux/mmc/sh_mobile_sdhi.h |    2 --
 2 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index e0ca0ab..17d5778 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -135,7 +135,6 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 	mmc_data = &priv->mmc_data;
 
 	if (p) {
-		p->pdata = mmc_data;
 		if (p->init) {
 			ret = p->init(pdev, &sdhi_ops);
 			if (ret)
@@ -284,9 +283,6 @@ static int sh_mobile_sdhi_remove(struct platform_device *pdev)
 	struct sh_mobile_sdhi_info *p = pdev->dev.platform_data;
 	int i = 0, irq;
 
-	if (p)
-		p->pdata = NULL;
-
 	tmio_mmc_host_remove(host);
 
 	while (1) {
diff --git a/include/linux/mmc/sh_mobile_sdhi.h b/include/linux/mmc/sh_mobile_sdhi.h
index b65679f..b76bcf0 100644
--- a/include/linux/mmc/sh_mobile_sdhi.h
+++ b/include/linux/mmc/sh_mobile_sdhi.h
@@ -4,7 +4,6 @@
 #include <linux/types.h>
 
 struct platform_device;
-struct tmio_mmc_data;
 
 #define SH_MOBILE_SDHI_IRQ_CARD_DETECT	"card_detect"
 #define SH_MOBILE_SDHI_IRQ_SDCARD	"sdcard"
@@ -26,7 +25,6 @@ struct sh_mobile_sdhi_info {
 	unsigned long tmio_caps2;
 	u32 tmio_ocr_mask;	/* available MMC voltages */
 	unsigned int cd_gpio;
-	struct tmio_mmc_data *pdata;
 	void (*set_pwr)(struct platform_device *pdev, int state);
 	int (*get_cd)(struct platform_device *pdev);
 
-- 
1.7.2.5

[PATCH v3 09/13] mmc: sh_mobile_sdhi: use managed resource allocations

[Guennadi Liakhovetski]

Use managed allocations to get memory, clock and interrupts . This
significantly simplifies clean up paths.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/mmc/host/sh_mobile_sdhi.c |   57 +++++++++----------------------------
 1 files changed, 14 insertions(+), 43 deletions(-)

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 17d5778..e095d5c 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -126,7 +126,7 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 	int irq, ret, i = 0;
 	bool multiplexed_isr = true;
 
-	priv = kzalloc(sizeof(struct sh_mobile_sdhi), GFP_KERNEL);
+	priv = devm_kzalloc(&pdev->dev, sizeof(struct sh_mobile_sdhi), GFP_KERNEL);
 	if (priv == NULL) {
 		dev_err(&pdev->dev, "kzalloc failed\n");
 		return -ENOMEM;
@@ -138,11 +138,11 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 		if (p->init) {
 			ret = p->init(pdev, &sdhi_ops);
 			if (ret)
-				goto einit;
+				return ret;
 		}
 	}
 
-	priv->clk = clk_get(&pdev->dev, NULL);
+	priv->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(priv->clk)) {
 		ret = PTR_ERR(priv->clk);
 		dev_err(&pdev->dev, "cannot get clock: %d\n", ret);
@@ -197,33 +197,33 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 	irq = platform_get_irq_byname(pdev, SH_MOBILE_SDHI_IRQ_CARD_DETECT);
 	if (irq >= 0) {
 		multiplexed_isr = false;
-		ret = request_irq(irq, tmio_mmc_card_detect_irq, 0,
+		ret = devm_request_irq(&pdev->dev, irq, tmio_mmc_card_detect_irq, 0,
 				  dev_name(&pdev->dev), host);
 		if (ret)
-			goto eirq_card_detect;
+			goto eirq;
 	}
 
 	irq = platform_get_irq_byname(pdev, SH_MOBILE_SDHI_IRQ_SDIO);
 	if (irq >= 0) {
 		multiplexed_isr = false;
-		ret = request_irq(irq, tmio_mmc_sdio_irq, 0,
+		ret = devm_request_irq(&pdev->dev, irq, tmio_mmc_sdio_irq, 0,
 				  dev_name(&pdev->dev), host);
 		if (ret)
-			goto eirq_sdio;
+			goto eirq;
 	}
 
 	irq = platform_get_irq_byname(pdev, SH_MOBILE_SDHI_IRQ_SDCARD);
 	if (irq >= 0) {
 		multiplexed_isr = false;
-		ret = request_irq(irq, tmio_mmc_sdcard_irq, 0,
+		ret = devm_request_irq(&pdev->dev, irq, tmio_mmc_sdcard_irq, 0,
 				  dev_name(&pdev->dev), host);
 		if (ret)
-			goto eirq_sdcard;
+			goto eirq;
 	} else if (!multiplexed_isr) {
 		dev_err(&pdev->dev,
 			"Principal SD-card IRQ is missing among named interrupts\n");
 		ret = irq;
-		goto eirq_sdcard;
+		goto eirq;
 	}
 
 	if (multiplexed_isr) {
@@ -232,15 +232,15 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 			if (irq < 0)
 				break;
 			i++;
-			ret = request_irq(irq, tmio_mmc_irq, 0,
+			ret = devm_request_irq(&pdev->dev, irq, tmio_mmc_irq, 0,
 					  dev_name(&pdev->dev), host);
 			if (ret)
-				goto eirq_multiplexed;
+				goto eirq;
 		}
 
 		/* There must be at least one IRQ source */
 		if (!i)
-			goto eirq_multiplexed;
+			goto eirq;
 	}
 
 	dev_info(&pdev->dev, "%s base at 0x%08lx clock rate %u MHz\n",
@@ -250,28 +250,12 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 
 	return ret;
 
-eirq_multiplexed:
-	while (i--) {
-		irq = platform_get_irq(pdev, i);
-		free_irq(irq, host);
-	}
-eirq_sdcard:
-	irq = platform_get_irq_byname(pdev, SH_MOBILE_SDHI_IRQ_SDIO);
-	if (irq >= 0)
-		free_irq(irq, host);
-eirq_sdio:
-	irq = platform_get_irq_byname(pdev, SH_MOBILE_SDHI_IRQ_CARD_DETECT);
-	if (irq >= 0)
-		free_irq(irq, host);
-eirq_card_detect:
+eirq:
 	tmio_mmc_host_remove(host);
 eprobe:
-	clk_put(priv->clk);
 eclkget:
 	if (p && p->cleanup)
 		p->cleanup(pdev);
-einit:
-	kfree(priv);
 	return ret;
 }
 
@@ -279,26 +263,13 @@ static int sh_mobile_sdhi_remove(struct platform_device *pdev)
 {
 	struct mmc_host *mmc = platform_get_drvdata(pdev);
 	struct tmio_mmc_host *host = mmc_priv(mmc);
-	struct sh_mobile_sdhi *priv = container_of(host->pdata, struct sh_mobile_sdhi, mmc_data);
 	struct sh_mobile_sdhi_info *p = pdev->dev.platform_data;
-	int i = 0, irq;
 
 	tmio_mmc_host_remove(host);
 
-	while (1) {
-		irq = platform_get_irq(pdev, i++);
-		if (irq < 0)
-			break;
-		free_irq(irq, host);
-	}
-
-	clk_put(priv->clk);
-
 	if (p && p->cleanup)
 		p->cleanup(pdev);
 
-	kfree(priv);
-
 	return 0;
 }
 
-- 
1.7.2.5

[PATCH v3 10/13] mmc: tmio: remove unused and deprecated symbols

[Guennadi Liakhovetski]

The tmio_mmc_cd_wakeup() inline function has been deprecated since 3.4 and
is unused since 3.4 too. Remove them.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 include/linux/mfd/tmio.h |   18 ------------------
 1 files changed, 0 insertions(+), 18 deletions(-)

diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
index d83af39..99bf3e66 100644
--- a/include/linux/mfd/tmio.h
+++ b/include/linux/mfd/tmio.h
@@ -65,12 +65,6 @@
  */
 #define TMIO_MMC_SDIO_IRQ		(1 << 2)
 /*
- * Some platforms can detect card insertion events with controller powered
- * down, using a GPIO IRQ, in which case they have to fill in cd_irq, cd_gpio,
- * and cd_flags fields of struct tmio_mmc_data.
- */
-#define TMIO_MMC_HAS_COLD_CD		(1 << 3)
-/*
  * Some controllers require waiting for the SD bus to become
  * idle before writing to some registers.
  */
@@ -117,18 +111,6 @@ struct tmio_mmc_data {
 };
 
 /*
- * This function is deprecated and will be removed soon. Please, convert your
- * platform to use drivers/mmc/core/cd-gpio.c
- */
-#include <linux/mmc/host.h>
-static inline void tmio_mmc_cd_wakeup(struct tmio_mmc_data *pdata)
-{
-	if (pdata)
-		mmc_detect_change(dev_get_drvdata(pdata->dev),
-				  msecs_to_jiffies(100));
-}
-
-/*
  * data for the NAND controller
  */
 struct tmio_nand_data {
-- 
1.7.2.5

[PATCH v3 11/13] mmc: tmio: add support for the VccQ regulator

[Guennadi Liakhovetski]

Some SD/MMC interfaces use 2 power regulators: one to power the card itself
(Vcc) and another one to pull signal lines up (VccQ). In case of eMMC and
UHS SD cards the regulators also have to be configured to supply different
voltages. The preferred order of turning supply power on and off is to
turn Vcc first on and last off.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/mmc/host/tmio_mmc_pio.c |   56 ++++++++++++++++++++++++++++++++-------
 1 files changed, 46 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index e32e9b4..3a44086 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -43,6 +43,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_qos.h>
 #include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
 #include <linux/scatterlist.h>
 #include <linux/spinlock.h>
 #include <linux/workqueue.h>
@@ -155,6 +156,7 @@ static void tmio_mmc_set_clock(struct tmio_mmc_host *host, int new_clock)
 		host->set_clk_div(host->pdev, (clk>>22) & 1);
 
 	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, clk & 0x1ff);
+	msleep(10);
 }
 
 static void tmio_mmc_clk_stop(struct tmio_mmc_host *host)
@@ -768,16 +770,48 @@ static int tmio_mmc_clk_update(struct mmc_host *mmc)
 	return ret;
 }
 
-static void tmio_mmc_set_power(struct tmio_mmc_host *host, struct mmc_ios *ios)
+static void tmio_mmc_power_on(struct tmio_mmc_host *host, unsigned short vdd)
 {
 	struct mmc_host *mmc = host->mmc;
+	int ret = 0;
+
+	/* .set_ios() is returning void, so, no chance to report an error */
 
 	if (host->set_pwr)
-		host->set_pwr(host->pdev, ios->power_mode != MMC_POWER_OFF);
+		host->set_pwr(host->pdev, 1);
+
+	if (!IS_ERR(mmc->supply.vmmc)) {
+		ret = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
+		/*
+		 * Attention: empiric value. With a b43 WiFi SDIO card this
+		 * delay proved necessary for reliable card-insertion probing.
+		 * 100us were not enough. Is this the same 140us delay, as in
+		 * tmio_mmc_set_ios()?
+		 */
+		udelay(200);
+	}
+	/*
+	 * It seems, VccQ should be switched on after Vcc, this is also what the
+	 * omap_hsmmc.c driver does.
+	 */
+	if (!IS_ERR(mmc->supply.vqmmc) && !ret) {
+		regulator_enable(mmc->supply.vqmmc);
+		udelay(200);
+	}
+}
+
+static void tmio_mmc_power_off(struct tmio_mmc_host *host)
+{
+	struct mmc_host *mmc = host->mmc;
+
+	if (!IS_ERR(mmc->supply.vqmmc))
+		regulator_disable(mmc->supply.vqmmc);
+
 	if (!IS_ERR(mmc->supply.vmmc))
-		/* Errors ignored... */
-		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,
-				      ios->power_mode ? ios->vdd : 0);
+		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0);
+
+	if (host->set_pwr)
+		host->set_pwr(host->pdev, 0);
 }
 
 /* Set MMC clock / power.
@@ -828,18 +862,20 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		if (!host->power) {
 			tmio_mmc_clk_update(mmc);
 			pm_runtime_get_sync(dev);
-			host->power = true;
 		}
 		tmio_mmc_set_clock(host, ios->clock);
-		/* power up SD bus */
-		tmio_mmc_set_power(host, ios);
+		if (!host->power) {
+			/* power up SD card and the bus */
+			tmio_mmc_power_on(host, ios->vdd);
+			host->power = true;
+		}
 		/* start bus clock */
 		tmio_mmc_clk_start(host);
 	} else if (ios->power_mode != MMC_POWER_UP) {
-		if (ios->power_mode == MMC_POWER_OFF)
-			tmio_mmc_set_power(host, ios);
 		if (host->power) {
 			struct tmio_mmc_data *pdata = host->pdata;
+			if (ios->power_mode == MMC_POWER_OFF)
+				tmio_mmc_power_off(host);
 			tmio_mmc_clk_stop(host);
 			host->power = false;
 			pm_runtime_put(dev);
-- 
1.7.2.5

[PATCH v3 12/13] mmc: add DT bindings for more MMC capability flags

[Guennadi Liakhovetski]

Many MMC capability flags are platform-dependent and are traditionally set
in platform data. With DT often each such capability requires a special
binding. Add bindings for MMC_CAP_SD_HIGHSPEED, MMC_CAP_MMC_HIGHSPEED,
MMC_CAP_POWER_OFF_CARD and MMC_CAP_SDIO_IRQ capabilities. Also add code to
DT parser to look up "keep-power-in-suspend" and "enable-sdio-wakeup"
bindings and set MMC_PM_KEEP_POWER and MMC_PM_WAKE_SDIO_IRQ respectively,
if found.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Arnd Bergmann <arnd@arndb.de>
---

v3: add cap-sdio-irq

 Documentation/devicetree/bindings/mmc/mmc.txt |    4 ++++
 drivers/mmc/core/host.c                       |   13 +++++++++++++
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
index 51dd64f..a043c78 100644
--- a/Documentation/devicetree/bindings/mmc/mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc.txt
@@ -25,6 +25,10 @@ Optional properties:
 - max-frequency: maximum operating clock frequency
 - no-1-8-v: when present, denotes that 1.8v card voltage is not supported on
   this system, even if the controller claims it is.
+- cap-sd-highspeed: SD high-speed timing is supported
+- cap-mmc-highspeed: MMC high-speed timing is supported
+- cap-power-off-card: powering off the card is safe
+- cap-sdio-irq: enable SDIO IRQ signalling on this interface
 
 *NOTE* on CD and WP polarity. As of 3.8, SD/MMC drivers parse their DT nodes
 each in its own way, including calculation of CD and WP polarities. Our goal is
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index c412612..56b6227 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -380,6 +380,19 @@ void mmc_of_parse(struct mmc_host *host)
 	}
 	if (explicit_inv_wp ^ gpio_inv_wp)
 		host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
+
+	if (of_find_property(np, "cap-sd-highspeed", &len))
+		host->caps |= MMC_CAP_SD_HIGHSPEED;
+	if (of_find_property(np, "cap-mmc-highspeed", &len))
+		host->caps |= MMC_CAP_MMC_HIGHSPEED;
+	if (of_find_property(np, "cap-power-off-card", &len))
+		host->caps |= MMC_CAP_POWER_OFF_CARD;
+	if (of_find_property(np, "cap-sdio-irq", &len))
+		host->caps |= MMC_CAP_SDIO_IRQ;
+	if (of_find_property(np, "keep-power-in-suspend", &len))
+		host->pm_caps |= MMC_PM_KEEP_POWER;
+	if (of_find_property(np, "enable-sdio-wakeup", &len))
+		host->pm_caps |= MMC_PM_WAKE_SDIO_IRQ;
 }
 
 EXPORT_SYMBOL(mmc_of_parse);
-- 
1.7.2.5

[PATCH v3 13/13] mmc: tmio: add barriers to IO operations

[Guennadi Liakhovetski]

Without barriers SDIO operations fail with runtime PM enabled.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Paul Mundt <lethal@linux-sh.org>
---

v3: use iowrite16_rep() and ioread16_rep() for consistency.

 drivers/mmc/host/tmio_mmc.h |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index d857f5c..a10ebd0 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -159,19 +159,20 @@ int tmio_mmc_host_runtime_resume(struct device *dev);
 
 static inline u16 sd_ctrl_read16(struct tmio_mmc_host *host, int addr)
 {
-	return readw(host->ctl + (addr << host->bus_shift));
+	return ioread16(host->ctl + (addr << host->bus_shift));
 }
 
 static inline void sd_ctrl_read16_rep(struct tmio_mmc_host *host, int addr,
 		u16 *buf, int count)
 {
-	readsw(host->ctl + (addr << host->bus_shift), buf, count);
+	wmb();
+	ioread16_rep(host->ctl + (addr << host->bus_shift), buf, count);
 }
 
 static inline u32 sd_ctrl_read32(struct tmio_mmc_host *host, int addr)
 {
-	return readw(host->ctl + (addr << host->bus_shift)) |
-	       readw(host->ctl + ((addr + 2) << host->bus_shift)) << 16;
+	return ioread16(host->ctl + (addr << host->bus_shift)) |
+	       ioread16(host->ctl + ((addr + 2) << host->bus_shift)) << 16;
 }
 
 static inline void sd_ctrl_write16(struct tmio_mmc_host *host, int addr, u16 val)
@@ -181,19 +182,20 @@ static inline void sd_ctrl_write16(struct tmio_mmc_host *host, int addr, u16 val
 	 */
 	if (host->pdata->write16_hook && host->pdata->write16_hook(host, addr))
 		return;
-	writew(val, host->ctl + (addr << host->bus_shift));
+	iowrite16(val, host->ctl + (addr << host->bus_shift));
 }
 
 static inline void sd_ctrl_write16_rep(struct tmio_mmc_host *host, int addr,
 		u16 *buf, int count)
 {
-	writesw(host->ctl + (addr << host->bus_shift), buf, count);
+	iowrite16_rep(host->ctl + (addr << host->bus_shift), buf, count);
+	wmb();
 }
 
 static inline void sd_ctrl_write32(struct tmio_mmc_host *host, int addr, u32 val)
 {
-	writew(val, host->ctl + (addr << host->bus_shift));
-	writew(val >> 16, host->ctl + ((addr + 2) << host->bus_shift));
+	iowrite16(val, host->ctl + (addr << host->bus_shift));
+	iowrite16(val >> 16, host->ctl + ((addr + 2) << host->bus_shift));
 }
 
 
-- 
1.7.2.5

Re: [PATCH v3 13/13] mmc: tmio: add barriers to IO operations

[Paul Mundt]

On Wed, Feb 06, 2013 at 08:47:00PM +0100, Guennadi Liakhovetski wrote:
> Without barriers SDIO operations fail with runtime PM enabled.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Paul Mundt <lethal@linux-sh.org>
> ---
> 
> v3: use iowrite16_rep() and ioread16_rep() for consistency.
> 
Looks better to me!

Reviewed-by: Paul Mundt <lethal@linux-sh.org>