Skip to contentSkip to conversation

mmc: core: Add support for MMC power sequences

PATCH New Ulf Hansson 4 participants 14 messages Jan 2, 2015 – Jan 13, 2015

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover mmc: core: Add support for MMC power sequences

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
Signature · 1 line
1.9.1
#1 lore raw

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

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_power_up()- Invoked from mmc_power_up(), to power up. mmc_pwrseq_power_on()- Invoked from mmc_power_up(), to power on. mmc_pwrseq_power_off()- Invoked from mmc_power_off(), to 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.

---
 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
drivers/mmc/core/Makefile +1 -1
index 38ed210..ccdd35f 100644
@@ -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
drivers/mmc/core/core.c +7 -0
index d3bfbdf..fd13649 100644
@@ -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"
@@ -1583,6 +1584,8 @@ void mmc_power_up(struct mmc_host *host, u32 ocr)
 
 	mmc_host_clk_hold(host);
 
+	mmc_pwrseq_power_up(host);
+
 	host->ios.vdd = fls(ocr) - 1;
 	host->ios.power_mode = MMC_POWER_UP;
 	/* Set initial state and call mmc_set_ios */
@@ -1602,6 +1605,8 @@ void mmc_power_up(struct mmc_host *host, u32 ocr)
 	 */
 	mmc_delay(10);
 
+	mmc_pwrseq_power_on(host);
+
 	host->ios.clock = host->f_init;
 
 	host->ios.power_mode = MMC_POWER_ON;
@@ -1623,6 +1628,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;
 
drivers/mmc/core/host.c +3 -1
index 0763644..8be0df7 100644
@@ -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);
 }
 
drivers/mmc/core/pwrseq.c new file +50 -0

50 lines not on this page · raw message

drivers/mmc/core/pwrseq.h new file +40 -0
new file mode 100644
index 0000000..430874d
@@ -0,0 +1,40 @@
+/*
+ * Copyright (C) 2014 Linaro Ltd
+ *
+ * Author: Ulf Hansson <[email protected]>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+#ifndef _MMC_CORE_PWRSEQ_H
+#define _MMC_CORE_PWRSEQ_H
+
+struct mmc_pwrseq_ops {
+	void (*power_up)(struct mmc_host *host);
+	void (*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_power_up(struct mmc_host *host);
+void mmc_pwrseq_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_power_up(struct mmc_host *host) {}
+static inline void mmc_pwrseq_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
include/linux/mmc/host.h +2 -0
index b6bf718..0c8cbe5 100644
@@ -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;
Signature · 1 line
1.9.1
#2 lore raw

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

In reply to 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.

---
 .../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
Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt new file +18 -0
new file mode 100644
index 0000000..e1b7f9c
@@ -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";
+	}
Documentation/devicetree/bindings/mmc/mmc.txt +5 -0
index bac1311..b12de1e 100644
@@ -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 at ab000000 {
 	max-frequency = <50000000>;
 	keep-power-in-suspend;
 	enable-sdio-wakeup;
+	mmc-pwrseq = <&sdhci0_pwrseq>
 }
 
 Example with sdio function subnode:
Signature · 1 line
1.9.1
#3 lore raw

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

In reply to 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.

---
 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
drivers/mmc/core/Makefile +1 -1
index ccdd35f..b39cbd2 100644
@@ -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
drivers/mmc/core/pwrseq.c +60 -1

74 lines not on this page · raw message

drivers/mmc/core/pwrseq.h +2 -0
index 430874d..7e20dda 100644
@@ -27,6 +27,8 @@ void mmc_pwrseq_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; }
drivers/mmc/core/pwrseq_simple.c new file +48 -0

48 lines not on this page · raw message

Signature · 1 line
1.9.1
#4 lore raw

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

In reply to 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 for the external chip/card, from the ->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.

---
 .../devicetree/bindings/mmc/mmc,pwrseq-simple.txt  |  5 +++
 drivers/mmc/core/pwrseq_simple.c                   | 38 ++++++++++++++++++++++
 2 files changed, 43 insertions(+)
Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt +5 -0
index e1b7f9c..6fe0cd6 100644
@@ -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>;
 	}
drivers/mmc/core/pwrseq_simple.c +38 -0
index 7f87bc1..97112b1 100644
@@ -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_power_up(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_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 = {
+	.power_up = mmc_pwrseq_simple_power_up,
+	.power_on = mmc_pwrseq_simple_power_on,
+	.power_off = mmc_pwrseq_simple_power_up,
 	.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;
 }
Signature · 1 line
1.9.1
#5 lore raw

cover mmc: core: Add support for MMC power sequences

In reply to Ulf Hansson
Quoted 2 lines · Ulf Hansson

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.

I think this should be done differently - part of that is with hind sight given that we now have a range of interface types.

One of my early design mistakes with MMC was to incorporate the power up and initialisation protocol (the 74 clocks business) into the core code. This was wrong, because it is a detail which only applies to dumb host interfaces. More inteligent interfaces do not need that complexity as they handle that in hardware.

Rather than MMC ending up with more layers and more infrastructure like this - turning it more into the turd which is sdhci - I would like to see some proper thought put into design, specifically addressing the above design issue.

What should be done is to move away from the opaque "set_ios" method into something more appropriate - a set of callbacks which describe what we want to achieve.

In the case of power up, there should be a single call into the host controller code which is responsible for initiating the power up sequence, and waits for the power up sequence to complete. In other words, most of mmc_power_up() should be moved to a library function, which dumb host adapters hook into the "power up" method. More inteligent adapters (eg, PXA, sdhci) can add their own hook which poke the hardware and return when the hardware has completed its power up sequence.

That avoids the "mmc_delay(10)" for inteligent adapters where these delays are not required - which presumably also are not required if some special power up sequence for eMMC is required.

The same thing goes for this "powerseq" stuff - all you're doing is propagating the same design mistake (with the POWER_UP/POWER_ON etc details) into it. That shouldn't be the case - think about the dumb powerup method I described above vs the inteligent method - these are two separate powerup methods, and should be implemented as such.

Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
#6 lore raw

cover mmc: core: Add support for MMC power sequences

Hi Russell,

Quoted 13 lines · Russell King - ARM Linux
Quoted 2 lines · Ulf Hansson

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.

I think this should be done differently - part of that is with hind sight given that we now have a range of interface types.

One of my early design mistakes with MMC was to incorporate the power up and initialisation protocol (the 74 clocks business) into the core code. This was wrong, because it is a detail which only applies to dumb host interfaces. More inteligent interfaces do not need that complexity as they handle that in hardware.

Quoted 8 lines

Rather than MMC ending up with more layers and more infrastructure like this - turning it more into the turd which is sdhci - I would like to see some proper thought put into design, specifically addressing the above design issue.

What should be done is to move away from the opaque "set_ios" method into something more appropriate - a set of callbacks which describe what we want to achieve.

I think you misunderstand what this patch-set tries to do, this patch-set is intended for use with on board sdio devices, which typically may need a number of resources initialized before the can be probed over the sd bus at all. E.g. a regulator enabled, a reset de-asserted, a clk configured and provided, etc. So this is about sequencing of events before even trying to talk to the device at the basic spi compatible layer.

Your ideas seem like a good idea to improve the mmc subsys, but this is somewhat unrelated, in a way this could apply equally to say an usb attached device, or basically any discoverable bus, where some onboard devices may need some form of enabling (or power up) before they can be discovered.

Which is why the original patchset Ulf references in the cover letter was trying to be more generic, since the got nowhere Ulf now is trying with something mmc specific, since the direct need for this functionality is with sdio devices.

Note that this is a real world problem which has been hacked around a lot in vendor kernels, so although getting the design right as always is important, it is also important to at some point come up with something workable, and if you look at previous threads on this topic, you will see that has already been discussed a lot, and always seems to get stuck on people trying to find the "perfect. 100% generic" solution, a typical example of perfect being the enemy of good.

Regards,

Hans

#7 lore raw

cover mmc: core: Add support for MMC power sequences

In reply to Russell King - ARM Linux
Quoted 35 lines · Russell King - ARM Linux
Quoted 2 lines · Ulf Hansson

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.

I think this should be done differently - part of that is with hind sight given that we now have a range of interface types.

One of my early design mistakes with MMC was to incorporate the power up and initialisation protocol (the 74 clocks business) into the core code. This was wrong, because it is a detail which only applies to dumb host interfaces. More inteligent interfaces do not need that complexity as they handle that in hardware.

Rather than MMC ending up with more layers and more infrastructure like this - turning it more into the turd which is sdhci - I would like to see some proper thought put into design, specifically addressing the above design issue.

What should be done is to move away from the opaque "set_ios" method into something more appropriate - a set of callbacks which describe what we want to achieve.

In the case of power up, there should be a single call into the host controller code which is responsible for initiating the power up sequence, and waits for the power up sequence to complete. In other words, most of mmc_power_up() should be moved to a library function, which dumb host adapters hook into the "power up" method. More inteligent adapters (eg, PXA, sdhci) can add their own hook which poke the hardware and return when the hardware has completed its power up sequence.

That avoids the "mmc_delay(10)" for inteligent adapters where these delays are not required - which presumably also are not required if some special power up sequence for eMMC is required.

Quoted 5 lines

The same thing goes for this "powerseq" stuff - all you're doing is propagating the same design mistake (with the POWER_UP/POWER_ON etc details) into it. That shouldn't be the case - think about the dumb powerup method I described above vs the inteligent method - these are two separate powerup methods, and should be implemented as such.

Hi Russell,

Thanks for your comments!

You certainly have a point around how to improve the use the existing ->set_ios() callbacks. Though, as Hans also pointed out in his reply, I believe this patchset is not related to that.

Regarding you concern, about me propagating the same design mistake as for the ->set_ios() callback, I am not sure I fully understand why you think that's the case?

I am suggesting is to add a set of mmc-pwrseq callbacks, which I think is needed for a mmc-pwrseq provider to be able handle it's SOC specific mmc power sequence. There are no MMC/SD/SDIO protocol specific constraints that should be handled from them, but instead only SOC specific power resources and sequences.

If the _simple_ mmc power sequence provider, turns out to be too complex while trying to support several SOCs, we shall instead add yet another provider. In this case, we should of course share as much code as possible between the different providers.

Kind regards
Uffe

Quoted 4 lines

--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net.

#8 lore raw

cover mmc: core: Add support for MMC power sequences

Quoted 3 lines · Ulf Hansson

Regarding you concern, about me propagating the same design mistake as for the ->set_ios() callback, I am not sure I fully understand why you think that's the case?

Because of this:

mmc_pwrseq_power_up()- Invoked from mmc_power_up(), to power up. mmc_pwrseq_power_on()- Invoked from mmc_power_up(), to power on.

This re-inforces the "power up, wait, power on" _power_ _sequence_ as part of the software design.

We know that _today_ there is hardware which does not work like that. We know that we have host adapters which ignore the "power up" part, because they deal with all the sequencing in hardware.

I'll refer to your new infrastructure as "pwrseq" and the existing infrastructure as "power sequence". (That alone shows what an absurd situation this is - we have two things which have the same name!)

Let's say we have a pwrseq handler which implements the power_up() and power_on() callbacks. It's written for use with a controller which implements appropriate actions on set_ios() with POWER_UP and POWER_ON states implemented.

So, what we have is:

pwrseq power sequence host state/action

power_up no power supplied
POWER_UP host applies power to the card

power_on host is supplying power to card
POWER_ON host applies clocks to the card

Now, for a more inteligent host, where the hardware takes care of sequencing the application of power and clocks, we have this:

pwrseq power sequence host action

power_up no power applied
POWER_UP host does nothing

power_on no power applied
POWER_ON host applies power to the card, waits,
and applies the clock

As we can see, the hardware state which the power_on() method of the pwrseq is called is highly host dependent. In the first case, power has already been applied. In the second case, power has not been applied.

To have consistency, you need to /first/ solve the problem which I've been pointing out, otherwise we're going to have these new pwrseq callbacks being called with inconsistent, host specific power states being applied to the card.

An alternative way to get consistency is to eliminate the pwrseq "power_on" callback altogether; the "power_up" callback will always be made before the host interface power sequence is started - and that is about the only thing we can guarantee given the variability in host interface designs.

Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
#9 lore raw

cover mmc: core: Add support for MMC power sequences

Quoted 54 lines · Russell King - ARM Linux
Quoted 3 lines · Ulf Hansson

Regarding you concern, about me propagating the same design mistake as for the ->set_ios() callback, I am not sure I fully understand why you think that's the case?

Because of this:

mmc_pwrseq_power_up()- Invoked from mmc_power_up(), to power up. mmc_pwrseq_power_on()- Invoked from mmc_power_up(), to power on.

This re-inforces the "power up, wait, power on" _power_ _sequence_ as part of the software design.

We know that _today_ there is hardware which does not work like that. We know that we have host adapters which ignore the "power up" part, because they deal with all the sequencing in hardware.

I'll refer to your new infrastructure as "pwrseq" and the existing infrastructure as "power sequence". (That alone shows what an absurd situation this is - we have two things which have the same name!)

Let's say we have a pwrseq handler which implements the power_up() and power_on() callbacks. It's written for use with a controller which implements appropriate actions on set_ios() with POWER_UP and POWER_ON states implemented.

So, what we have is:

pwrseq power sequence host state/action

power_up no power supplied
POWER_UP host applies power to the card

power_on host is supplying power to card
POWER_ON host applies clocks to the card

Now, for a more inteligent host, where the hardware takes care of sequencing the application of power and clocks, we have this:

pwrseq power sequence host action

power_up no power applied
POWER_UP host does nothing

power_on no power applied
POWER_ON host applies power to the card, waits,
and applies the clock

As we can see, the hardware state which the power_on() method of the pwrseq is called is highly host dependent. In the first case, power has already been applied. In the second case, power has not been applied.

Quoted 4 lines

To have consistency, you need to /first/ solve the problem which I've been pointing out, otherwise we're going to have these new pwrseq callbacks being called with inconsistent, host specific power states being applied to the card.

Thanks a lot for elaborating! I understand your concern now.

In this context, I believe it's important to state that I think these host driver behaves badly! _All_ host drivers shall handle their "power_up" things while they get MMC_POWER_UP through the ->set_ios() callback. They shouldn't be taking short-cuts and ignore the MMC_POWER_UP state, that also includes those host drivers which are able to handle the power sequence by help from it's controller hardware.

I did quick research and found that the following host's ->set_ios() callbacks need to be fixed in this regards. au1xmmc_set_ios()
cb710_mmc_set_ios()
omap_hsmmc_set_ios()
sdricoh_set_ios()
__toshsd_set_ios()

Quoted 6 lines

An alternative way to get consistency is to eliminate the pwrseq "power_on" callback altogether; the "power_up" callback will always be made before the host interface power sequence is started - and that is about the only thing we can guarantee given the variability in host interface designs.

Eliminating is likely not an option, since it would mean we will have a hard time to cope with requirements for the pwrseq sequences.

An option would be to move the call to the pwrseq ->power_on() callback to the end of mmc_power_up(). That would make it as consistent as we can from mmc core point of view. Right?

Kind regards
Uffe

#10 lore raw

cover mmc: core: Add support for MMC power sequences

Quoted 63 lines · Ulf Hansson
Quoted 54 lines · Russell King - ARM Linux
Quoted 3 lines · Ulf Hansson

Regarding you concern, about me propagating the same design mistake as for the ->set_ios() callback, I am not sure I fully understand why you think that's the case?

Because of this:

mmc_pwrseq_power_up()- Invoked from mmc_power_up(), to power up. mmc_pwrseq_power_on()- Invoked from mmc_power_up(), to power on.

This re-inforces the "power up, wait, power on" _power_ _sequence_ as part of the software design.

We know that _today_ there is hardware which does not work like that. We know that we have host adapters which ignore the "power up" part, because they deal with all the sequencing in hardware.

I'll refer to your new infrastructure as "pwrseq" and the existing infrastructure as "power sequence". (That alone shows what an absurd situation this is - we have two things which have the same name!)

Let's say we have a pwrseq handler which implements the power_up() and power_on() callbacks. It's written for use with a controller which implements appropriate actions on set_ios() with POWER_UP and POWER_ON states implemented.

So, what we have is:

pwrseq power sequence host state/action

power_up no power supplied
POWER_UP host applies power to the card

power_on host is supplying power to card
POWER_ON host applies clocks to the card

Now, for a more inteligent host, where the hardware takes care of sequencing the application of power and clocks, we have this:

pwrseq power sequence host action

power_up no power applied
POWER_UP host does nothing

power_on no power applied
POWER_ON host applies power to the card, waits,
and applies the clock

As we can see, the hardware state which the power_on() method of the pwrseq is called is highly host dependent. In the first case, power has already been applied. In the second case, power has not been applied.

Quoted 4 lines

To have consistency, you need to /first/ solve the problem which I've been pointing out, otherwise we're going to have these new pwrseq callbacks being called with inconsistent, host specific power states being applied to the card.

Thanks a lot for elaborating! I understand your concern now.

Quoted 7 lines

In this context, I believe it's important to state that I think these host driver behaves badly! _All_ host drivers shall handle their "power_up" things while they get MMC_POWER_UP through the ->set_ios() callback. They shouldn't be taking short-cuts and ignore the MMC_POWER_UP state, that also includes those host drivers which are able to handle the power sequence by help from it's controller hardware.

You can't solve this in this way. Let's say that you decide to force the inteligent controllers to power up and enable clocks in the POWER_UP stage:

pwrseq power sequence host action

power_up no power applied
POWER_UP host applies power to the card, waits,
and applies the clock

power_on power and clock is applied
POWER_ON host does nothing

So now, you have a variability for the power_on() callback, where with dumb hosts, the clock is not enabled, but for more inteligent hosts, the clock has already enabled, and likely we have already waited the 74 clocks necessary in the MMC protocol.

Quoted 7 lines

I did quick research and found that the following host's ->set_ios() callbacks need to be fixed in this regards. au1xmmc_set_ios()
cb710_mmc_set_ios()
omap_hsmmc_set_ios()
sdricoh_set_ios()
__toshsd_set_ios()

Rather than fixing them, how about introducing a new method for host controllers:

	power_up()

which can be pointed to a library function which makes the ->set_ios calls as the current mmc_power_up() function does for those dumb controllers, but for the more inteligent controllers, just invokes their power up sequence and returns when that is complete.

Quoted 6 lines
Quoted 5 lines

An alternative way to get consistency is to eliminate the pwrseq "power_on" callback altogether; the "power_up" callback will always be made before the host interface power sequence is started - and that is about the only thing we can guarantee given the variability in host interface designs.

Quoted 6 lines

Eliminating is likely not an option, since it would mean we will have a hard time to cope with requirements for the pwrseq sequences.

An option would be to move the call to the pwrseq ->power_on() callback to the end of mmc_power_up(). That would make it as consistent as we can from mmc core point of view. Right?

Yes, and you might as well call the pwrseq callbacks "->pre_power_on()" and "->post_power_on()" - maybe even "->post_pwrclk_on()" to make it clear that it happens after the power and clocks have been applied according to the MMC/SD protocol.

What I'm envisioning mmc_power_up() becoming is something like this:

void mmc_power_up(struct mmc_host *host, u32 ocr)

{
        if (host->ios.power_mode == MMC_POWER_ON)
                return;

        mmc_host_clk_hold(host);

	if (pwrseq)
		pwrseq->pre_power_on(host);

        host->ios.vdd = fls(ocr) - 1;
	host->ios.clock = host->f_init;
	host->ios.power_mode = MMC_POWER_ON;
	/* and set remainder of initial state of host->ios */

	host->ops->power_up(host, &host->ios);

	if (pwrseq)
		pwrseq->post_power_on(host);

	mmc_host_clk_release(host);
}

The legacy power_up host method can save a copy host->ios, and manipulate the saved copy to call mmc_set_ios() as we currently do, with all the delays we have there.

Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
#11 lore raw

cover mmc: core: Add support for MMC power sequences

[...]

Quoted 21 lines
Quoted 10 lines

Thanks a lot for elaborating! I understand your concern now.

In this context, I believe it's important to state that I think these host driver behaves badly! _All_ host drivers shall handle their "power_up" things while they get MMC_POWER_UP through the ->set_ios() callback. They shouldn't be taking short-cuts and ignore the MMC_POWER_UP state, that also includes those host drivers which are able to handle the power sequence by help from it's controller hardware.

You can't solve this in this way. Let's say that you decide to force the inteligent controllers to power up and enable clocks in the POWER_UP stage:

pwrseq power sequence host action

power_up no power applied
POWER_UP host applies power to the card, waits,
and applies the clock

Quoted 7 lines

power_on power and clock is applied
POWER_ON host does nothing

So now, you have a variability for the power_on() callback, where with dumb hosts, the clock is not enabled, but for more inteligent hosts, the clock has already enabled, and likely we have already waited the 74 clocks necessary in the MMC protocol.

I understand, so I was not thinking of fixing the actual problem but more to make the behaviour consistent.

Quoted 12 lines
Quoted 8 lines

I did quick research and found that the following host's ->set_ios() callbacks need to be fixed in this regards. au1xmmc_set_ios()
cb710_mmc_set_ios()
omap_hsmmc_set_ios()
sdricoh_set_ios()
__toshsd_set_ios()

Rather than fixing them, how about introducing a new method for host controllers:

Quoted 6 lines
        power_up()

which can be pointed to a library function which makes the ->set_ios calls as the current mmc_power_up() function does for those dumb controllers, but for the more inteligent controllers, just invokes their power up sequence and returns when that is complete.

Seems like a good idea!

Quoted 44 lines
Quoted 7 lines
Quoted 6 lines

An alternative way to get consistency is to eliminate the pwrseq "power_on" callback altogether; the "power_up" callback will always be made before the host interface power sequence is started - and that is about the only thing we can guarantee given the variability in host interface designs.

Quoted 6 lines

Eliminating is likely not an option, since it would mean we will have a hard time to cope with requirements for the pwrseq sequences.

An option would be to move the call to the pwrseq ->power_on() callback to the end of mmc_power_up(). That would make it as consistent as we can from mmc core point of view. Right?

Yes, and you might as well call the pwrseq callbacks "->pre_power_on()" and "->post_power_on()" - maybe even "->post_pwrclk_on()" to make it clear that it happens after the power and clocks have been applied according to the MMC/SD protocol.

What I'm envisioning mmc_power_up() becoming is something like this:

void mmc_power_up(struct mmc_host *host, u32 ocr)

{
        if (host->ios.power_mode == MMC_POWER_ON)
                return;

        mmc_host_clk_hold(host);

        if (pwrseq)
                pwrseq->pre_power_on(host);

        host->ios.vdd = fls(ocr) - 1;
        host->ios.clock = host->f_init;
        host->ios.power_mode = MMC_POWER_ON;
        /* and set remainder of initial state of host->ios */

        host->ops->power_up(host, &host->ios);

        if (pwrseq)
                pwrseq->post_power_on(host);

        mmc_host_clk_release(host);
}
Quoted 4 lines

The legacy power_up host method can save a copy host->ios, and manipulate the saved copy to call mmc_set_ios() as we currently do, with all the delays we have there.

Looks good!

I will rename the pwrseq callbacks to your proposal and send a v2.

Kind regards
Uffe

#12 lore raw

cover mmc: core: Add support for MMC power sequences

In reply to Ulf Hansson
Quoted 10 lines · Ulf Hansson

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.

Hi Ulf,

have used this to successfully init a mwifiex_sdio card but had to also add a delay while the reset line is asserted. May make sense to add a property to the pwrseq node to specify the minimum amount of time that the reset GPIO needs to be asserted?

Thanks,

Tomeu

Quoted 42 lines

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

#13 lore raw

cover mmc: core: Add support for MMC power sequences

Quoted 12 lines · Tomeu Vizoso
Quoted 10 lines · Ulf Hansson

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.

Quoted 6 lines

Hi Ulf,

have used this to successfully init a mwifiex_sdio card but had to also add a delay while the reset line is asserted. May make sense to add a property to the pwrseq node to specify the minimum amount of time that the reset GPIO needs to be asserted?

To me that is description of the HW, so I would happily accept such a DT property. Can you please send a patch, based on top of this patchset?

BTW, I guess you only need the delay while the ->power_up() callback is invoked and not for ->power_off()?

Quoted 2 lines

Thanks for helping out testing!

Kind regards
Uffe