mmc: core: Add support for MMC power sequences
Subsystems (2)
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
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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
PATCH 2/4 mmc: pwrseq: Document DT bindings for the simple MMC power sequence
In reply to Ulf HanssonThe 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
@@ -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
@@ -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
PATCH 3/4 mmc: pwrseq: Initial support for the simple MMC power sequence provider
In reply to Ulf HanssonTo 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
@@ -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
@@ -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
PATCH 4/4 mmc: pwrseq_simple: Add support for a reset GPIO pin
In reply to Ulf HanssonThe 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
@@ -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
@@ -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
cover mmc: core: Add support for MMC power sequences
In reply to Ulf HanssonQuoted 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.
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
cover mmc: core: Add support for MMC power sequences
In reply to Russell King - ARM LinuxQuoted 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.
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.
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 frommmc_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()andpower_on()callbacks. It's written for use with a controller which implements appropriate actions onset_ios()withPOWER_UPandPOWER_ONstates implemented.So, what we have is:
pwrseq power sequence host state/action
power_upno power supplied
POWER_UPhost applies power to the card
power_onhost is supplying power to card
POWER_ONhost applies clocks to the cardNow, 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_upno power applied
POWER_UPhost does nothing
power_onno power applied
POWER_ONhost applies power to the card, waits,
and applies the clockAs 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
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 frommmc_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()andpower_on()callbacks. It's written for use with a controller which implements appropriate actions onset_ios()withPOWER_UPandPOWER_ONstates implemented.So, what we have is:
pwrseq power sequence host state/action
power_upno power supplied
POWER_UPhost applies power to the card
power_onhost is supplying power to card
POWER_ONhost applies clocks to the cardNow, 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_upno power applied
POWER_UPhost does nothing
power_onno power applied
POWER_ONhost applies power to the card, waits,
and applies the clockAs 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 getMMC_POWER_UPthrough the ->set_ios() callback. They shouldn't be taking short-cuts and ignore theMMC_POWER_UPstate, 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.
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 getMMC_POWER_UPthrough the ->set_ios() callback. They shouldn't be taking short-cuts and ignore theMMC_POWER_UPstate, 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_UPstage:pwrseq power sequence host action
power_upno power applied
POWER_UPhost applies power to the card, waits,
and applies the clock
Quoted 7 lines
power_onpower and clock is applied
POWER_ONhost does nothingSo 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_uphost method can save a copyhost->ios, and manipulate the saved copy to callmmc_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
cover mmc: core: Add support for MMC power sequences
In reply to Ulf HanssonQuoted 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_OFto 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
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_sdiocard 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
Tested-by Tomeu Vizoso <[email protected]>
Thanks for helping out testing!
Kind regards
Uffe