[PATCH V2 0/4] mmc: core: Add support for MMC power sequences

[PATCH V2 0/4] mmc: core: Add support for MMC power sequences

[Ulf Hansson]

Changes in v2:
	Fixed comments from Russell King:
	- Renamed pwrseq callbacks and corresponding interface functions.
	- Move the call to the previous namned ->power_on() callback, to the
	  end of mmc_power_up() to get consistent behavior.


This is yet another try to solve the issues of dealing with power sequences for
the MMC subsystem. The latest attempt, see link below, took a generic approach
by adding a new top level driver layer. That's was rejected by several reasons.
http://lwn.net/Articles/602855/

This time the approach is focused to fix the issues for MMC only.

To give a short background, SOCs may specify a specific MMC power sequence. To
successfully detect an (e)MMC/SD/SDIO card, that power sequence must be followed
while initializing the card.

To be able to handle these SOC specific power sequences, we add a MMC power
sequence interface, which helps the mmc core to deal with such.

A MMC power sequence provider then implements a set of callbacks from the above
mentioned interface. The provider has a corresponding DT compatibility string
and relies on CONFIG_OF to be set to find it's various resourses, like for
example a GPIO reset.

The mmc core will from mmc_of_parse() try find a "mmc-pwrseq" DT node and then
call the corresponding MMC power sequence provider's initialization function.


Ulf Hansson (4):
  mmc: core: Initial support for MMC power sequences
  mmc: pwrseq: Document DT bindings for the simple MMC power sequence
  mmc: pwrseq: Initial support for the simple MMC power sequence
    provider
  mmc: pwrseq_simple: Add support for a reset GPIO pin

 .../devicetree/bindings/mmc/mmc,pwrseq-simple.txt  |  23 +++++
 Documentation/devicetree/bindings/mmc/mmc.txt      |   5 +
 drivers/mmc/core/Makefile                          |   2 +-
 drivers/mmc/core/core.c                            |   7 ++
 drivers/mmc/core/host.c                            |   4 +-
 drivers/mmc/core/pwrseq.c                          | 109 +++++++++++++++++++++
 drivers/mmc/core/pwrseq.h                          |  42 ++++++++
 drivers/mmc/core/pwrseq_simple.c                   |  86 ++++++++++++++++
 include/linux/mmc/host.h                           |   2 +
 9 files changed, 278 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
 create mode 100644 drivers/mmc/core/pwrseq.c
 create mode 100644 drivers/mmc/core/pwrseq.h
 create mode 100644 drivers/mmc/core/pwrseq_simple.c

-- 
1.9.1

[PATCH V2 1/4] mmc: core: Initial support for MMC power sequences

[Ulf Hansson]

System on chip designs may specify a specific MMC power sequence. To
successfully detect an (e)MMC/SD/SDIO card, that power sequence must
be followed while initializing the card.

To be able to handle these SOC specific power sequences, let's add a
MMC power sequence interface. It provides the following functions to
help the mmc core to deal with these power sequences.

mmc_pwrseq_alloc() - Invoked from mmc_of_parse(), to initialize data.
mmc_pwrseq_pre_power_on()- Invoked in the beginning of mmc_power_up().
mmc_pwrseq_post_power_on()- Invoked at the end in mmc_power_up().
mmc_pwrseq_power_off()- Invoked from mmc_power_off().
mmc_pwrseq_free() - Invoked from mmc_free_host(), to free data.

Each MMC power sequence provider will be responsible to implement a set
of callbacks. These callbacks mirrors the functions above.

This patch adds the skeleton, following patches will extend the core of
the MMC power sequence and add support for a specific simple MMC power
sequence.

Do note, since the mmc_pwrseq_alloc() is invoked from mmc_of_parse(),
host drivers needs to make use of this API to enable the support for
MMC power sequences. Moreover the MMC power sequence support depends on
CONFIG_OF.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	Fixed comments from Russell King:
	- Renamed pwrseq callbacks and corresponding interface functions.
	- Move the call to the previous namned ->power_on() callback, to the
	  end of mmc_power_up() to get consistent behavior.
	- Updated commit message according to the above changes.

---
 drivers/mmc/core/Makefile |  2 +-
 drivers/mmc/core/core.c   |  7 +++++++
 drivers/mmc/core/host.c   |  4 +++-
 drivers/mmc/core/pwrseq.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/core/pwrseq.h | 40 +++++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h  |  2 ++
 6 files changed, 103 insertions(+), 2 deletions(-)
 create mode 100644 drivers/mmc/core/pwrseq.c
 create mode 100644 drivers/mmc/core/pwrseq.h

diff --git a/drivers/mmc/core/Makefile b/drivers/mmc/core/Makefile
index 38ed210..ccdd35f 100644
--- a/drivers/mmc/core/Makefile
+++ b/drivers/mmc/core/Makefile
@@ -8,5 +8,5 @@ mmc_core-y			:= core.o bus.o host.o \
 				   sdio.o sdio_ops.o sdio_bus.o \
 				   sdio_cis.o sdio_io.o sdio_irq.o \
 				   quirks.o slot-gpio.o
-
+mmc_core-$(CONFIG_OF)		+= pwrseq.o
 mmc_core-$(CONFIG_DEBUG_FS)	+= debugfs.o
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index d5c176e..1be7055 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -40,6 +40,7 @@
 #include "bus.h"
 #include "host.h"
 #include "sdio_bus.h"
+#include "pwrseq.h"
 
 #include "mmc_ops.h"
 #include "sd_ops.h"
@@ -1615,6 +1616,8 @@ void mmc_power_up(struct mmc_host *host, u32 ocr)
 
 	mmc_host_clk_hold(host);
 
+	mmc_pwrseq_pre_power_on(host);
+
 	host->ios.vdd = fls(ocr) - 1;
 	host->ios.power_mode = MMC_POWER_UP;
 	/* Set initial state and call mmc_set_ios */
@@ -1645,6 +1648,8 @@ void mmc_power_up(struct mmc_host *host, u32 ocr)
 	 */
 	mmc_delay(10);
 
+	mmc_pwrseq_post_power_on(host);
+
 	mmc_host_clk_release(host);
 }
 
@@ -1655,6 +1660,8 @@ void mmc_power_off(struct mmc_host *host)
 
 	mmc_host_clk_hold(host);
 
+	mmc_pwrseq_power_off(host);
+
 	host->ios.clock = 0;
 	host->ios.vdd = 0;
 
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 0763644..8be0df7 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -30,6 +30,7 @@
 #include "core.h"
 #include "host.h"
 #include "slot-gpio.h"
+#include "pwrseq.h"
 
 #define cls_dev_to_mmc_host(d)	container_of(d, struct mmc_host, class_dev)
 
@@ -448,7 +449,7 @@ int mmc_of_parse(struct mmc_host *host)
 		host->dsr_req = 0;
 	}
 
-	return 0;
+	return mmc_pwrseq_alloc(host);
 }
 
 EXPORT_SYMBOL(mmc_of_parse);
@@ -588,6 +589,7 @@ EXPORT_SYMBOL(mmc_remove_host);
  */
 void mmc_free_host(struct mmc_host *host)
 {
+	mmc_pwrseq_free(host);
 	put_device(&host->class_dev);
 }
 
diff --git a/drivers/mmc/core/pwrseq.c b/drivers/mmc/core/pwrseq.c
new file mode 100644
index 0000000..bd08772
--- /dev/null
+++ b/drivers/mmc/core/pwrseq.c
@@ -0,0 +1,50 @@
+/*
+ *  Copyright (C) 2014 Linaro Ltd
+ *
+ * Author: Ulf Hansson <ulf.hansson@linaro.org>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ *
+ *  MMC power sequence management
+ */
+#include <linux/mmc/host.h>
+
+#include "pwrseq.h"
+
+
+int mmc_pwrseq_alloc(struct mmc_host *host)
+{
+	return 0;
+}
+
+void mmc_pwrseq_pre_power_on(struct mmc_host *host)
+{
+	struct mmc_pwrseq *pwrseq = host->pwrseq;
+
+	if (pwrseq && pwrseq->ops && pwrseq->ops->pre_power_on)
+		pwrseq->ops->pre_power_on(host);
+}
+
+void mmc_pwrseq_post_power_on(struct mmc_host *host)
+{
+	struct mmc_pwrseq *pwrseq = host->pwrseq;
+
+	if (pwrseq && pwrseq->ops && pwrseq->ops->post_power_on)
+		pwrseq->ops->post_power_on(host);
+}
+
+void mmc_pwrseq_power_off(struct mmc_host *host)
+{
+	struct mmc_pwrseq *pwrseq = host->pwrseq;
+
+	if (pwrseq && pwrseq->ops && pwrseq->ops->power_off)
+		pwrseq->ops->power_off(host);
+}
+
+void mmc_pwrseq_free(struct mmc_host *host)
+{
+	struct mmc_pwrseq *pwrseq = host->pwrseq;
+
+	if (pwrseq && pwrseq->ops && pwrseq->ops->free)
+		pwrseq->ops->free(host);
+}
diff --git a/drivers/mmc/core/pwrseq.h b/drivers/mmc/core/pwrseq.h
new file mode 100644
index 0000000..12aaf2b
--- /dev/null
+++ b/drivers/mmc/core/pwrseq.h
@@ -0,0 +1,40 @@
+/*
+ * Copyright (C) 2014 Linaro Ltd
+ *
+ * Author: Ulf Hansson <ulf.hansson@linaro.org>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+#ifndef _MMC_CORE_PWRSEQ_H
+#define _MMC_CORE_PWRSEQ_H
+
+struct mmc_pwrseq_ops {
+	void (*pre_power_on)(struct mmc_host *host);
+	void (*post_power_on)(struct mmc_host *host);
+	void (*power_off)(struct mmc_host *host);
+	void (*free)(struct mmc_host *host);
+};
+
+struct mmc_pwrseq {
+	struct mmc_pwrseq_ops *ops;
+};
+
+#ifdef CONFIG_OF
+
+int mmc_pwrseq_alloc(struct mmc_host *host);
+void mmc_pwrseq_pre_power_on(struct mmc_host *host);
+void mmc_pwrseq_post_power_on(struct mmc_host *host);
+void mmc_pwrseq_power_off(struct mmc_host *host);
+void mmc_pwrseq_free(struct mmc_host *host);
+
+#else
+
+static inline int mmc_pwrseq_alloc(struct mmc_host *host) { return 0; }
+static inline void mmc_pwrseq_pre_power_on(struct mmc_host *host) {}
+static inline void mmc_pwrseq_post_power_on(struct mmc_host *host) {}
+static inline void mmc_pwrseq_power_off(struct mmc_host *host) {}
+static inline void mmc_pwrseq_free(struct mmc_host *host) {}
+
+#endif
+
+#endif
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index b6bf718..0c8cbe5 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -195,6 +195,7 @@ struct mmc_context_info {
 };
 
 struct regulator;
+struct mmc_pwrseq;
 
 struct mmc_supply {
 	struct regulator *vmmc;		/* Card power supply */
@@ -206,6 +207,7 @@ struct mmc_host {
 	struct device		class_dev;
 	int			index;
 	const struct mmc_host_ops *ops;
+	struct mmc_pwrseq	*pwrseq;
 	unsigned int		f_min;
 	unsigned int		f_max;
 	unsigned int		f_init;
-- 
1.9.1

[PATCH V2 2/4] mmc: pwrseq: Document DT bindings for the simple MMC power sequence

[Ulf Hansson]

The simple MMC power sequence provider, intends to supports a set of
common properties between SOC designs. It thus enables us to re-use the
same provider for several SOCs.

In this initial step, let's add the top level description of the MMC
power sequence and describe the compatible string for the simple MMC
power sequence provider.

Following patches will step by step add support for new properties to
the simple MMC power sequence provider.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- None.

---
 .../devicetree/bindings/mmc/mmc,pwrseq-simple.txt      | 18 ++++++++++++++++++
 Documentation/devicetree/bindings/mmc/mmc.txt          |  5 +++++
 2 files changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt

diff --git a/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
new file mode 100644
index 0000000..e1b7f9c
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
@@ -0,0 +1,18 @@
+* The simple MMC power sequence provider
+
+System on chip designs may specify a specific MMC power sequence. To
+successfully detect an (e)MMC/SD/SDIO card, that power sequence must be
+maintained while initializing the card.
+
+The simple MMC power sequence provider, intends to supports a set of common
+properties between SOC designs. It thus enables us to re-use the same provider
+for several SOC designs.
+
+Required properties:
+- compatible : contains "mmc,pwrseq-simple".
+
+Example:
+
+	sdhci0_pwrseq {
+		compatible = "mmc,pwrseq-simple";
+	}
diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
index bac1311..b12de1e 100644
--- a/Documentation/devicetree/bindings/mmc/mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc.txt
@@ -64,6 +64,10 @@ 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
 
+Optional MMC power sequence:
+- mmc-pwrseq: phandle to the MMC power sequence node. See "mmc,pwrseq-*"
+	for documentation of MMC power sequence bindings.
+
 
 Use of Function subnodes
 ------------------------
@@ -101,6 +105,7 @@ sdhci@ab000000 {
 	max-frequency = <50000000>;
 	keep-power-in-suspend;
 	enable-sdio-wakeup;
+	mmc-pwrseq = <&sdhci0_pwrseq>
 }
 
 Example with sdio function subnode:
-- 
1.9.1

Re: [PATCH V2 2/4] mmc: pwrseq: Document DT bindings for the simple MMC power sequence

[Mark Rutland]

Hi Ulf,

On Wed, Jan 14, 2015 at 01:02:08PM +0000, Ulf Hansson wrote:
> The simple MMC power sequence provider, intends to supports a set of
> common properties between SOC designs. It thus enables us to re-use the
> same provider for several SOCs.
> 
> In this initial step, let's add the top level description of the MMC
> power sequence and describe the compatible string for the simple MMC
> power sequence provider.
> 
> Following patches will step by step add support for new properties to
> the simple MMC power sequence provider.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> 
> Changes in v2:
> 	- None.
> 
> ---
>  .../devicetree/bindings/mmc/mmc,pwrseq-simple.txt      | 18 ++++++++++++++++++
>  Documentation/devicetree/bindings/mmc/mmc.txt          |  5 +++++
>  2 files changed, 23 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
> 
> diff --git a/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
> new file mode 100644
> index 0000000..e1b7f9c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
> @@ -0,0 +1,18 @@
> +* The simple MMC power sequence provider
> +
> +System on chip designs may specify a specific MMC power sequence. To
> +successfully detect an (e)MMC/SD/SDIO card, that power sequence must be
> +maintained while initializing the card.
> +
> +The simple MMC power sequence provider, intends to supports a set of common
> +properties between SOC designs. It thus enables us to re-use the same provider
> +for several SOC designs.
> +
> +Required properties:
> +- compatible : contains "mmc,pwrseq-simple".

Nit: "mmc" is not a vendor prefix.

> +
> +Example:
> +
> +	sdhci0_pwrseq {
> +		compatible = "mmc,pwrseq-simple";
> +	}

I'm a little confused here. What specific sequence is described by this
node? We don't appear to have referred to any resources used as part of
that sequence, and the description above only mentions that there could
be a specific sequence, not what that sequence is.

So I don't think this makes sense on its own, and should probably be
folded with patches adding the initial support for the resources used as
part of the sequence (e.g. the GPIOs added in a later patch).

Thanks,
Mark.

> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
> index bac1311..b12de1e 100644
> --- a/Documentation/devicetree/bindings/mmc/mmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/mmc.txt
> @@ -64,6 +64,10 @@ 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
>  
> +Optional MMC power sequence:
> +- mmc-pwrseq: phandle to the MMC power sequence node. See "mmc,pwrseq-*"
> +	for documentation of MMC power sequence bindings.
> +
>  
>  Use of Function subnodes
>  ------------------------
> @@ -101,6 +105,7 @@ sdhci@ab000000 {
>  	max-frequency = <50000000>;
>  	keep-power-in-suspend;
>  	enable-sdio-wakeup;
> +	mmc-pwrseq = <&sdhci0_pwrseq>
>  }
>  
>  Example with sdio function subnode:
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH V2 2/4] mmc: pwrseq: Document DT bindings for the simple MMC power sequence

[NeilBrown]

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

On Thu, 15 Jan 2015 16:58:26 +0000 Mark Rutland <mark.rutland@arm.com> wrote:

> Hi Ulf,
> 
> On Wed, Jan 14, 2015 at 01:02:08PM +0000, Ulf Hansson wrote:
> > The simple MMC power sequence provider, intends to supports a set of
> > common properties between SOC designs. It thus enables us to re-use the
> > same provider for several SOCs.
> > 
> > In this initial step, let's add the top level description of the MMC
> > power sequence and describe the compatible string for the simple MMC
> > power sequence provider.
> > 
> > Following patches will step by step add support for new properties to
> > the simple MMC power sequence provider.
> > 
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> > 
> > Changes in v2:
> > 	- None.
> > 
> > ---
> >  .../devicetree/bindings/mmc/mmc,pwrseq-simple.txt      | 18 ++++++++++++++++++
> >  Documentation/devicetree/bindings/mmc/mmc.txt          |  5 +++++
> >  2 files changed, 23 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
> > new file mode 100644
> > index 0000000..e1b7f9c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
> > @@ -0,0 +1,18 @@
> > +* The simple MMC power sequence provider
> > +
> > +System on chip designs may specify a specific MMC power sequence. To
> > +successfully detect an (e)MMC/SD/SDIO card, that power sequence must be
> > +maintained while initializing the card.
> > +
> > +The simple MMC power sequence provider, intends to supports a set of common
> > +properties between SOC designs. It thus enables us to re-use the same provider
> > +for several SOC designs.
> > +
> > +Required properties:
> > +- compatible : contains "mmc,pwrseq-simple".
> 
> Nit: "mmc" is not a vendor prefix.
> 
> > +
> > +Example:
> > +
> > +	sdhci0_pwrseq {
> > +		compatible = "mmc,pwrseq-simple";
> > +	}
> 
> I'm a little confused here. What specific sequence is described by this
> node? We don't appear to have referred to any resources used as part of
> that sequence, and the description above only mentions that there could
> be a specific sequence, not what that sequence is.
> 
> So I don't think this makes sense on its own, and should probably be
> folded with patches adding the initial support for the resources used as
> part of the sequence (e.g. the GPIOs added in a later patch).
> 

I guess I assumed that this "simple" referred to the current behaviour, which
includes some of vmmc-supply, vqmmc-supply, vmmc_aux-supply and pbias-supply
being switched at "appropriate" times.

Would it make sense to bring all of these explicitly under the "pwrseq"
umbrella, leaving the current behaviour only when no pwrseq node is provided?

Also, it isn't clear to me whether the need for power/reset is a function of
the mmc-host, or a function of the device attached to the host.  I suspect
some are needed by one, some by the other.  Any by both?

Should the two needs be kept separate?

NeilBrown


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH V2 2/4] mmc: pwrseq: Document DT bindings for the simple MMC power sequence

[Ulf Hansson]

On 15 January 2015 at 19:53, NeilBrown <neilb-l3A5Bk7waGM@public.gmane.org> wrote:
> On Thu, 15 Jan 2015 16:58:26 +0000 Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>
>> Hi Ulf,
>>
>> On Wed, Jan 14, 2015 at 01:02:08PM +0000, Ulf Hansson wrote:
>> > The simple MMC power sequence provider, intends to supports a set of
>> > common properties between SOC designs. It thus enables us to re-use the
>> > same provider for several SOCs.
>> >
>> > In this initial step, let's add the top level description of the MMC
>> > power sequence and describe the compatible string for the simple MMC
>> > power sequence provider.
>> >
>> > Following patches will step by step add support for new properties to
>> > the simple MMC power sequence provider.
>> >
>> > Signed-off-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> > ---
>> >
>> > Changes in v2:
>> >     - None.
>> >
>> > ---
>> >  .../devicetree/bindings/mmc/mmc,pwrseq-simple.txt      | 18 ++++++++++++++++++
>> >  Documentation/devicetree/bindings/mmc/mmc.txt          |  5 +++++
>> >  2 files changed, 23 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
>> > new file mode 100644
>> > index 0000000..e1b7f9c
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
>> > @@ -0,0 +1,18 @@
>> > +* The simple MMC power sequence provider
>> > +
>> > +System on chip designs may specify a specific MMC power sequence. To
>> > +successfully detect an (e)MMC/SD/SDIO card, that power sequence must be
>> > +maintained while initializing the card.
>> > +
>> > +The simple MMC power sequence provider, intends to supports a set of common
>> > +properties between SOC designs. It thus enables us to re-use the same provider
>> > +for several SOC designs.
>> > +
>> > +Required properties:
>> > +- compatible : contains "mmc,pwrseq-simple".
>>
>> Nit: "mmc" is not a vendor prefix.
>>
>> > +
>> > +Example:
>> > +
>> > +   sdhci0_pwrseq {
>> > +           compatible = "mmc,pwrseq-simple";
>> > +   }
>>
>> I'm a little confused here. What specific sequence is described by this
>> node? We don't appear to have referred to any resources used as part of
>> that sequence, and the description above only mentions that there could
>> be a specific sequence, not what that sequence is.
>>
>> So I don't think this makes sense on its own, and should probably be
>> folded with patches adding the initial support for the resources used as
>> part of the sequence (e.g. the GPIOs added in a later patch).
>>
>
> I guess I assumed that this "simple" referred to the current behaviour, which
> includes some of vmmc-supply, vqmmc-supply, vmmc_aux-supply and pbias-supply
> being switched at "appropriate" times.
>
> Would it make sense to bring all of these explicitly under the "pwrseq"
> umbrella, leaving the current behaviour only when no pwrseq node is provided?

In principle there is nothing that prevents us from following your
suggestion. But why should we?

Moreover, I don't think it's required at this point, let's first get
the basic mmc-pwrseq support in place. Then we can evolve it. :-)

>
> Also, it isn't clear to me whether the need for power/reset is a function of
> the mmc-host, or a function of the device attached to the host.  I suspect
> some are needed by one, some by the other.  Any by both?

To me the the power is coupled with the host and thus controlled by
the mmc layer. But I guess we can discuss this in a forever long loop.
:-)

They may be other resources which is coupled with an SDIO function /
eMMC,SDIO card, such as irqs. Those shall be described in the child
node of the host.
I have queued the below patchset from Hans, which I believe should address this:

https://www.mail-archive.com/linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg27241.html

>
> Should the two needs be kept separate?

Yes, I think so.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 2/4] mmc: pwrseq: Document DT bindings for the simple MMC power sequence

[Russell King - ARM Linux]

On Fri, Jan 16, 2015 at 07:53:08AM +1300, NeilBrown wrote:
> Also, it isn't clear to me whether the need for power/reset is a function of
> the mmc-host, or a function of the device attached to the host.  I suspect
> some are needed by one, some by the other.  Any by both?

Neil,

There's a horrid issue there.  The standard model of MMC/SD is that the
device will be powered up by the host, and will then be probed to find
out what the device is.  This normally works fine as the host is
responsible for controlling the power to the socket which the card is
plugged into.

Where things fall down is when you have a MMC/SD device embedded onto
a board, and they decide that it'll be given separate controls for
power and reset which are not under the control of the host.

If the MMC/SD device is not powered up before we probe the bus, then
the device is not discoverable, and this breaks the standard model:
- If we run the standard MMC/SD device discovery code with the device
  powered down, it will find no device attached, so no device will be
  created.

- If we create a device, then the device driver will be probed
  immediately.  While the device driver can then bind, discover the
  power and reset controls, and activate them, it can't then talk to
  its device because the MMC layer hasn't gone through the required
  standard device discovery and probe sequence.  If we could do that,
  we would then need some way to stop that mechanism creating another
  device.

If we instead take the view that the host is responsible for powering
up the device, then we are merely keeping with the standard MMC/SD
model, and we don't have to hack around his.

Keeping to the standard model of a host responsible for power control
of its child devices is just a whole lot nicer.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 2/4] mmc: pwrseq: Document DT bindings for the simple MMC power sequence

[Ulf Hansson]

On 15 January 2015 at 17:58, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> Hi Ulf,
>
> On Wed, Jan 14, 2015 at 01:02:08PM +0000, Ulf Hansson wrote:
>> The simple MMC power sequence provider, intends to supports a set of
>> common properties between SOC designs. It thus enables us to re-use the
>> same provider for several SOCs.
>>
>> In this initial step, let's add the top level description of the MMC
>> power sequence and describe the compatible string for the simple MMC
>> power sequence provider.
>>
>> Following patches will step by step add support for new properties to
>> the simple MMC power sequence provider.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>
>> Changes in v2:
>>       - None.
>>
>> ---
>>  .../devicetree/bindings/mmc/mmc,pwrseq-simple.txt      | 18 ++++++++++++++++++
>>  Documentation/devicetree/bindings/mmc/mmc.txt          |  5 +++++
>>  2 files changed, 23 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
>> new file mode 100644
>> index 0000000..e1b7f9c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
>> @@ -0,0 +1,18 @@
>> +* The simple MMC power sequence provider
>> +
>> +System on chip designs may specify a specific MMC power sequence. To
>> +successfully detect an (e)MMC/SD/SDIO card, that power sequence must be
>> +maintained while initializing the card.
>> +
>> +The simple MMC power sequence provider, intends to supports a set of common
>> +properties between SOC designs. It thus enables us to re-use the same provider
>> +for several SOC designs.
>> +
>> +Required properties:
>> +- compatible : contains "mmc,pwrseq-simple".
>
> Nit: "mmc" is not a vendor prefix.
>
>> +
>> +Example:
>> +
>> +     sdhci0_pwrseq {
>> +             compatible = "mmc,pwrseq-simple";
>> +     }
>
> I'm a little confused here. What specific sequence is described by this
> node? We don't appear to have referred to any resources used as part of
> that sequence, and the description above only mentions that there could
> be a specific sequence, not what that sequence is.
>
> So I don't think this makes sense on its own, and should probably be
> folded with patches adding the initial support for the resources used as
> part of the sequence (e.g. the GPIOs added in a later patch).

You are absolutely right. I fix it.

Again, thanks for reviewing.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2 3/4] mmc: pwrseq: Initial support for the simple MMC power sequence provider

[Ulf Hansson]

To add the core part for the MMC power sequence, let's start by adding
initial support for the simple MMC power sequence provider.

In this initial step, the MMC power sequence node are fetched and the
compatible string for the simple MMC power sequence provider are
verified.

At this point we don't parse the node for any properties, but instead
that will be handled from following patches. Since there are no
properties supported yet, let's just implement the ->alloc() and the
->free() callbacks.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- None.

---
 drivers/mmc/core/Makefile        |  2 +-
 drivers/mmc/core/pwrseq.c        | 61 +++++++++++++++++++++++++++++++++++++++-
 drivers/mmc/core/pwrseq.h        |  2 ++
 drivers/mmc/core/pwrseq_simple.c | 48 +++++++++++++++++++++++++++++++
 4 files changed, 111 insertions(+), 2 deletions(-)
 create mode 100644 drivers/mmc/core/pwrseq_simple.c

diff --git a/drivers/mmc/core/Makefile b/drivers/mmc/core/Makefile
index ccdd35f..b39cbd2 100644
--- a/drivers/mmc/core/Makefile
+++ b/drivers/mmc/core/Makefile
@@ -8,5 +8,5 @@ mmc_core-y			:= core.o bus.o host.o \
 				   sdio.o sdio_ops.o sdio_bus.o \
 				   sdio_cis.o sdio_io.o sdio_irq.o \
 				   quirks.o slot-gpio.o
-mmc_core-$(CONFIG_OF)		+= pwrseq.o
+mmc_core-$(CONFIG_OF)		+= pwrseq.o pwrseq_simple.o
 mmc_core-$(CONFIG_DEBUG_FS)	+= debugfs.o
diff --git a/drivers/mmc/core/pwrseq.c b/drivers/mmc/core/pwrseq.c
index bd08772..7287585 100644
--- a/drivers/mmc/core/pwrseq.c
+++ b/drivers/mmc/core/pwrseq.c
@@ -7,14 +7,73 @@
  *
  *  MMC power sequence management
  */
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+
 #include <linux/mmc/host.h>
 
 #include "pwrseq.h"
 
+struct mmc_pwrseq_match {
+	const char *compatible;
+	int (*alloc)(struct mmc_host *host, struct device *dev);
+};
+
+static struct mmc_pwrseq_match pwrseq_match[] = {
+	{
+		.compatible = "mmc,pwrseq-simple",
+		.alloc = mmc_pwrseq_simple_alloc,
+	},
+};
+
+static struct mmc_pwrseq_match *mmc_pwrseq_find(struct device_node *np)
+{
+	struct mmc_pwrseq_match *match = ERR_PTR(-ENODEV);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(pwrseq_match); i++) {
+		if (of_device_is_compatible(np,	pwrseq_match[i].compatible)) {
+			match = &pwrseq_match[i];
+			break;
+		}
+	}
+
+	return match;
+}
 
 int mmc_pwrseq_alloc(struct mmc_host *host)
 {
-	return 0;
+	struct platform_device *pdev;
+	struct device_node *np;
+	struct mmc_pwrseq_match *match;
+	int ret = 0;
+
+	np = of_parse_phandle(host->parent->of_node, "mmc-pwrseq", 0);
+	if (!np)
+		return 0;
+
+	pdev = of_find_device_by_node(np);
+	if (!pdev) {
+		ret = -ENODEV;
+		goto err;
+	}
+
+	match = mmc_pwrseq_find(np);
+	if (IS_ERR(match)) {
+		ret = PTR_ERR(match);
+		goto err;
+	}
+
+	ret = match->alloc(host, &pdev->dev);
+	if (!ret)
+		dev_info(host->parent, "allocated mmc-pwrseq\n");
+
+err:
+	of_node_put(np);
+	return ret;
 }
 
 void mmc_pwrseq_pre_power_on(struct mmc_host *host)
diff --git a/drivers/mmc/core/pwrseq.h b/drivers/mmc/core/pwrseq.h
index 12aaf2b..bd860d8 100644
--- a/drivers/mmc/core/pwrseq.h
+++ b/drivers/mmc/core/pwrseq.h
@@ -27,6 +27,8 @@ void mmc_pwrseq_post_power_on(struct mmc_host *host);
 void mmc_pwrseq_power_off(struct mmc_host *host);
 void mmc_pwrseq_free(struct mmc_host *host);
 
+int mmc_pwrseq_simple_alloc(struct mmc_host *host, struct device *dev);
+
 #else
 
 static inline int mmc_pwrseq_alloc(struct mmc_host *host) { return 0; }
diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
new file mode 100644
index 0000000..7f87bc1
--- /dev/null
+++ b/drivers/mmc/core/pwrseq_simple.c
@@ -0,0 +1,48 @@
+/*
+ *  Copyright (C) 2014 Linaro Ltd
+ *
+ * Author: Ulf Hansson <ulf.hansson@linaro.org>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ *
+ *  Simple MMC power sequence management
+ */
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/err.h>
+
+#include <linux/mmc/host.h>
+
+#include "pwrseq.h"
+
+struct mmc_pwrseq_simple {
+	struct mmc_pwrseq pwrseq;
+};
+
+static void mmc_pwrseq_simple_free(struct mmc_host *host)
+{
+	struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq,
+					struct mmc_pwrseq_simple, pwrseq);
+
+	kfree(&pwrseq);
+	host->pwrseq = NULL;
+}
+
+static struct mmc_pwrseq_ops mmc_pwrseq_simple_ops = {
+	.free = mmc_pwrseq_simple_free,
+};
+
+int mmc_pwrseq_simple_alloc(struct mmc_host *host, struct device *dev)
+{
+	struct mmc_pwrseq_simple *pwrseq;
+
+	pwrseq = kzalloc(sizeof(struct mmc_pwrseq_simple), GFP_KERNEL);
+	if (!pwrseq)
+		return -ENOMEM;
+
+	pwrseq->pwrseq.ops = &mmc_pwrseq_simple_ops;
+	host->pwrseq = &pwrseq->pwrseq;
+
+	return 0;
+}
-- 
1.9.1

[PATCH V2 4/4] mmc: pwrseq_simple: Add support for a reset GPIO pin

[Ulf Hansson]

The need for a reset GPIO has several times been pointed out from
earlier posted patchsets. Especially some WLAN chips which are
attached to an SDIO interface may use a GPIO reset.

In this first version, one reset pin is supported. We may want to
extend the support to cover more pins, but let's leave that as a future
change. The added DT binding for the reset GPIO can easily be extended
to manage several pins.

The reset GPIO is asserted at initialization and prior we start the
power up procedure. It will then be de-asserted right after the power
has been provided to the external chip/card, from the ->post_power_on()
callback.

Note, the reset GPIO is optional. Thus we don't return an error even if
we can't find a GPIO pin for the consumer.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- Adopted to the changed names of the pwrseq callbacks.

---
 .../devicetree/bindings/mmc/mmc,pwrseq-simple.txt  |  5 +++
 drivers/mmc/core/pwrseq_simple.c                   | 38 ++++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
index e1b7f9c..6fe0cd6 100644
--- a/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
@@ -11,8 +11,13 @@ for several SOC designs.
 Required properties:
 - compatible : contains "mmc,pwrseq-simple".
 
+Optional properties:
+- reset-gpios : contains a list of GPIO specifiers, though currently only one
+		specifier is supported.
+
 Example:
 
 	sdhci0_pwrseq {
 		compatible = "mmc,pwrseq-simple";
+		reset-gpios = <&gpio1 12 0>;
 	}
diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
index 7f87bc1..42d9836 100644
--- a/drivers/mmc/core/pwrseq_simple.c
+++ b/drivers/mmc/core/pwrseq_simple.c
@@ -11,6 +11,7 @@
 #include <linux/slab.h>
 #include <linux/device.h>
 #include <linux/err.h>
+#include <linux/gpio/consumer.h>
 
 #include <linux/mmc/host.h>
 
@@ -18,31 +19,68 @@
 
 struct mmc_pwrseq_simple {
 	struct mmc_pwrseq pwrseq;
+	struct gpio_desc *reset_gpio;
 };
 
+static void mmc_pwrseq_simple_pre_power_on(struct mmc_host *host)
+{
+	struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq,
+					struct mmc_pwrseq_simple, pwrseq);
+
+	if (!IS_ERR(pwrseq->reset_gpio))
+		gpiod_set_value_cansleep(pwrseq->reset_gpio, 1);
+}
+
+static void mmc_pwrseq_simple_post_power_on(struct mmc_host *host)
+{
+	struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq,
+					struct mmc_pwrseq_simple, pwrseq);
+
+	if (!IS_ERR(pwrseq->reset_gpio))
+		gpiod_set_value_cansleep(pwrseq->reset_gpio, 0);
+}
+
 static void mmc_pwrseq_simple_free(struct mmc_host *host)
 {
 	struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq,
 					struct mmc_pwrseq_simple, pwrseq);
 
+	if (!IS_ERR(pwrseq->reset_gpio))
+		gpiod_put(pwrseq->reset_gpio);
+
 	kfree(&pwrseq);
 	host->pwrseq = NULL;
 }
 
 static struct mmc_pwrseq_ops mmc_pwrseq_simple_ops = {
+	.pre_power_on = mmc_pwrseq_simple_pre_power_on,
+	.post_power_on = mmc_pwrseq_simple_post_power_on,
+	.power_off = mmc_pwrseq_simple_pre_power_on,
 	.free = mmc_pwrseq_simple_free,
 };
 
 int mmc_pwrseq_simple_alloc(struct mmc_host *host, struct device *dev)
 {
 	struct mmc_pwrseq_simple *pwrseq;
+	int ret = 0;
 
 	pwrseq = kzalloc(sizeof(struct mmc_pwrseq_simple), GFP_KERNEL);
 	if (!pwrseq)
 		return -ENOMEM;
 
+	pwrseq->reset_gpio = gpiod_get_index(dev, "reset", 0, GPIOD_OUT_HIGH);
+	if (IS_ERR(pwrseq->reset_gpio) &&
+		PTR_ERR(pwrseq->reset_gpio) != -ENOENT &&
+		PTR_ERR(pwrseq->reset_gpio) != -ENOSYS) {
+		ret = PTR_ERR(pwrseq->reset_gpio);
+		goto free;
+	}
+
 	pwrseq->pwrseq.ops = &mmc_pwrseq_simple_ops;
 	host->pwrseq = &pwrseq->pwrseq;
 
 	return 0;
+free:
+	kfree(&pwrseq);
+	return ret;
 }
-- 
1.9.1

Re: [PATCH V2 4/4] mmc: pwrseq_simple: Add support for a reset GPIO pin

[Mark Rutland]

Hi,

On Wed, Jan 14, 2015 at 01:02:10PM +0000, Ulf Hansson wrote:
> The need for a reset GPIO has several times been pointed out from
> earlier posted patchsets. Especially some WLAN chips which are
> attached to an SDIO interface may use a GPIO reset.
> 
> In this first version, one reset pin is supported. We may want to
> extend the support to cover more pins, but let's leave that as a future
> change. The added DT binding for the reset GPIO can easily be extended
> to manage several pins.
> 
> The reset GPIO is asserted at initialization and prior we start the
> power up procedure. It will then be de-asserted right after the power
> has been provided to the external chip/card, from the ->post_power_on()
> callback.

This needs to be specified a little more explicitly in the binding
document below, as it forms part of the contract of the binding.

> 
> Note, the reset GPIO is optional. Thus we don't return an error even if
> we can't find a GPIO pin for the consumer.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> 
> Changes in v2:
> 	- Adopted to the changed names of the pwrseq callbacks.
> 
> ---
>  .../devicetree/bindings/mmc/mmc,pwrseq-simple.txt  |  5 +++
>  drivers/mmc/core/pwrseq_simple.c                   | 38 ++++++++++++++++++++++
>  2 files changed, 43 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
> index e1b7f9c..6fe0cd6 100644
> --- a/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
> +++ b/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
> @@ -11,8 +11,13 @@ for several SOC designs.
>  Required properties:
>  - compatible : contains "mmc,pwrseq-simple".
>  
> +Optional properties:
> +- reset-gpios : contains a list of GPIO specifiers, though currently only one
> +		specifier is supported.

The support is a Linux issue. If the binding is meant to describe a
list, mention that, and what the expected semantics are.

Is it that difficult to have the driver iterate over the list now?

Thanks,
Mark.

> +
>  Example:
>  
>  	sdhci0_pwrseq {
>  		compatible = "mmc,pwrseq-simple";
> +		reset-gpios = <&gpio1 12 0>;
>  	}
> diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
> index 7f87bc1..42d9836 100644
> --- a/drivers/mmc/core/pwrseq_simple.c
> +++ b/drivers/mmc/core/pwrseq_simple.c
> @@ -11,6 +11,7 @@
>  #include <linux/slab.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
> +#include <linux/gpio/consumer.h>
>  
>  #include <linux/mmc/host.h>
>  
> @@ -18,31 +19,68 @@
>  
>  struct mmc_pwrseq_simple {
>  	struct mmc_pwrseq pwrseq;
> +	struct gpio_desc *reset_gpio;
>  };
>  
> +static void mmc_pwrseq_simple_pre_power_on(struct mmc_host *host)
> +{
> +	struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq,
> +					struct mmc_pwrseq_simple, pwrseq);
> +
> +	if (!IS_ERR(pwrseq->reset_gpio))
> +		gpiod_set_value_cansleep(pwrseq->reset_gpio, 1);
> +}
> +
> +static void mmc_pwrseq_simple_post_power_on(struct mmc_host *host)
> +{
> +	struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq,
> +					struct mmc_pwrseq_simple, pwrseq);
> +
> +	if (!IS_ERR(pwrseq->reset_gpio))
> +		gpiod_set_value_cansleep(pwrseq->reset_gpio, 0);
> +}
> +
>  static void mmc_pwrseq_simple_free(struct mmc_host *host)
>  {
>  	struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq,
>  					struct mmc_pwrseq_simple, pwrseq);
>  
> +	if (!IS_ERR(pwrseq->reset_gpio))
> +		gpiod_put(pwrseq->reset_gpio);
> +
>  	kfree(&pwrseq);
>  	host->pwrseq = NULL;
>  }
>  
>  static struct mmc_pwrseq_ops mmc_pwrseq_simple_ops = {
> +	.pre_power_on = mmc_pwrseq_simple_pre_power_on,
> +	.post_power_on = mmc_pwrseq_simple_post_power_on,
> +	.power_off = mmc_pwrseq_simple_pre_power_on,
>  	.free = mmc_pwrseq_simple_free,
>  };
>  
>  int mmc_pwrseq_simple_alloc(struct mmc_host *host, struct device *dev)
>  {
>  	struct mmc_pwrseq_simple *pwrseq;
> +	int ret = 0;
>  
>  	pwrseq = kzalloc(sizeof(struct mmc_pwrseq_simple), GFP_KERNEL);
>  	if (!pwrseq)
>  		return -ENOMEM;
>  
> +	pwrseq->reset_gpio = gpiod_get_index(dev, "reset", 0, GPIOD_OUT_HIGH);
> +	if (IS_ERR(pwrseq->reset_gpio) &&
> +		PTR_ERR(pwrseq->reset_gpio) != -ENOENT &&
> +		PTR_ERR(pwrseq->reset_gpio) != -ENOSYS) {
> +		ret = PTR_ERR(pwrseq->reset_gpio);
> +		goto free;
> +	}
> +
>  	pwrseq->pwrseq.ops = &mmc_pwrseq_simple_ops;
>  	host->pwrseq = &pwrseq->pwrseq;
>  
>  	return 0;
> +free:
> +	kfree(&pwrseq);
> +	return ret;
>  }
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH V2 4/4] mmc: pwrseq_simple: Add support for a reset GPIO pin

[Ulf Hansson]

On 15 January 2015 at 18:04, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi,
>
> On Wed, Jan 14, 2015 at 01:02:10PM +0000, Ulf Hansson wrote:
>> The need for a reset GPIO has several times been pointed out from
>> earlier posted patchsets. Especially some WLAN chips which are
>> attached to an SDIO interface may use a GPIO reset.
>>
>> In this first version, one reset pin is supported. We may want to
>> extend the support to cover more pins, but let's leave that as a future
>> change. The added DT binding for the reset GPIO can easily be extended
>> to manage several pins.
>>
>> The reset GPIO is asserted at initialization and prior we start the
>> power up procedure. It will then be de-asserted right after the power
>> has been provided to the external chip/card, from the ->post_power_on()
>> callback.
>
> This needs to be specified a little more explicitly in the binding
> document below, as it forms part of the contract of the binding.
>
>>
>> Note, the reset GPIO is optional. Thus we don't return an error even if
>> we can't find a GPIO pin for the consumer.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>
>> Changes in v2:
>>       - Adopted to the changed names of the pwrseq callbacks.
>>
>> ---
>>  .../devicetree/bindings/mmc/mmc,pwrseq-simple.txt  |  5 +++
>>  drivers/mmc/core/pwrseq_simple.c                   | 38 ++++++++++++++++++++++
>>  2 files changed, 43 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
>> index e1b7f9c..6fe0cd6 100644
>> --- a/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
>> +++ b/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt
>> @@ -11,8 +11,13 @@ for several SOC designs.
>>  Required properties:
>>  - compatible : contains "mmc,pwrseq-simple".
>>
>> +Optional properties:
>> +- reset-gpios : contains a list of GPIO specifiers, though currently only one
>> +             specifier is supported.
>
> The support is a Linux issue. If the binding is meant to describe a
> list, mention that, and what the expected semantics are.
>
> Is it that difficult to have the driver iterate over the list now?

I was trying to keep code simple, maybe too simple. :-)

I will update the DT document according to your proposal and fix the code.

Thanks for reviewing!

Kind regards
Uffe

Re: [PATCH V2 2/4] mmc: pwrseq: Document DT bindings for the simple MMC power sequence

[NeilBrown]

On Fri, 16 Jan 2015 11:36:46 +0000 Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

> On Fri, Jan 16, 2015 at 07:53:08AM +1300, NeilBrown wrote:
> > Also, it isn't clear to me whether the need for power/reset is a function of
> > the mmc-host, or a function of the device attached to the host.  I suspect
> > some are needed by one, some by the other.  Any by both?
> 
> Neil,
> 
> There's a horrid issue there.  The standard model of MMC/SD is that the
> device will be powered up by the host, and will then be probed to find
> out what the device is.  This normally works fine as the host is
> responsible for controlling the power to the socket which the card is
> plugged into.
> 
> Where things fall down is when you have a MMC/SD device embedded onto
> a board, and they decide that it'll be given separate controls for
> power and reset which are not under the control of the host.
> 
> If the MMC/SD device is not powered up before we probe the bus, then
> the device is not discoverable, and this breaks the standard model:
> - If we run the standard MMC/SD device discovery code with the device
>   powered down, it will find no device attached, so no device will be
>   created.
> 
> - If we create a device, then the device driver will be probed
>   immediately.  While the device driver can then bind, discover the
>   power and reset controls, and activate them, it can't then talk to
>   its device because the MMC layer hasn't gone through the required
>   standard device discovery and probe sequence.  If we could do that,
>   we would then need some way to stop that mechanism creating another
>   device.

Hi Russell,
 thanks for the overview - quite helpful.
 I would like to particularly focus on that last sentence mentioning
 "some way to stop [standard device discovery] creating another device".

 That way already exists, or something very similar to it, in the mmc/sdio
 core. In particular, the 'oldcard' argument to mmc_sdio_init_card().

 This is called both when initialising a card and when restoring power to it.
 If 'oldcard' is set and the card found matches 'oldcard', then 'oldcard' is
 left unchanged.  i.e. a new device is not created.

 Suppose that devicetree listed a child node for an mmc host with
 "non-removable" set, then we could do a preliminary probe which sets
 up host->card so that subsequent "standard" probes find the properly
 matching card and leave 'oldcard' unchanged.  This card could somehow
 register its own power-up sequence.

 In the case of 'non-removable' it might be nice to be much more cautious
 about clearing host->card when a probe fails.  One of the (many) problems I
 have on my device is that occasionally the probe of the mmc/sdio/wifi card
 fails (EBUSY I think, but I'm not sure and I have no idea why).  Once that
 happens, the wifi becomes inaccessible until a reboot. 

 As mmc_rescan ensures that a non-removable card is only scanned once, it
 would be really good if we were extra careful never to remove a
 non-removable card, even in the face of error...


> 
> If we instead take the view that the host is responsible for powering
> up the device, then we are merely keeping with the standard MMC/SD
> model, and we don't have to hack around his.
> 
> Keeping to the standard model of a host responsible for power control
> of its child devices is just a whole lot nicer.
> 

I'm not really against the proposed patch.  It should remove at least one of
my problems, and I don't think it makes any other problems more difficult.
So if the above ideas don't appeal to anyone else, I'll raise no further
objections.

Thanks,
NeilBrown