Message ID | 1421658784-11980-2-git-send-email-ulf.hansson@linaro.org |
---|---|
State | New |
Headers | show |
+Russell King On 30 January 2015 at 23:27, NeilBrown <neilb@suse.de> wrote: > > On Mon, Jan 19, 2015 at 10:13 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >> index d5c176e87951..1be7055548cb 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); >> } > > Hi Ulf, > I think this may be too late for the _post_power_on() call. > > This is exactly where I release the reset line in my patch, and I've just discovered > that it is causing one of my problems. > > Shortly before this call is > > host->ios.power_mode = MMC_POWER_ON; > mmc_set_ios(host); > > and omap_hsmmc_set_ios() contains: > > switch (ios->power_mode) { > .... > case MMC_POWER_ON: > do_send_init_stream = 1; > break; > > > if (do_send_init_stream) > send_init_stream(host); > > Which sends the "init stream" to the card. > If the card is still being reset at this time, the stream may not be effective. > > I find that about 10%-20% of the time when I release the reset line *after* the > sequence is sent, my card fails to initialised. When I release *before* the sequence > is sent, it never fails. Okay, thanks for providing these details. > > I note that other drivers handle the init stream differently. > atmel-mci just sets a flag in set_ios, and the actually sends the stream in > atmci_start_request() > > dw_mmc, jz4740_mmc, mxcmmc, pxamci do much the same > > mmc_spi does it during set_ios like omap_hsmmc. > > The others I am not able to interpret easily. > > So it might make sense to change omap_hsmmc and mmc_spi to delay the init > stream until later, but I think it is probably safest to move the post_power_on call > to before the set_ios call. Well, the problem is that there are host drivers that don't consider MMC_POWER_UP and delays initialization/power_up to MMC_POWER_ON. So we might fix the issue for some, but breaks it for another. I had a discussion around inconsistent host driver behaviours from ->set_ios() callbacks with Russell, for the first version of this patchset. That's why I converted to mmc_pwrseq_post_power_on() instead of mmc_pwrseq_power_on() as it was in v1. At that point I didn't realize that the "initstream" sequence is an important factor to consider here as well. Additionally, those host drivers that ignores MMC_POWER_UP definitely behaves wrong. I did a research about this earlier and decided to go for a deeper look one more time. Actually I found only three drivers that needs some attention and surely those are easy to fix. au1xmmc cb710-mmc toshsd To have something working for 3.20, I suggest we follow your proposal about moving the call to mmc_pwrseq_post_power_on() to before the call to ->set_ios() with MMC_POWER_ON . I send a patch we can test. For reference, I still agree with Russell's proposal that the proper solution, long-term wise, should be to replace/extend MMC_POWER_UP|ON|OFF with new host callbacks. That will enable each host to perform its own optimized power up/off sequence. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2 February 2015 at 16:05, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Mon, Feb 02, 2015 at 03:57:37PM +0100, Ulf Hansson wrote: >> On 30 January 2015 at 23:27, NeilBrown <neilb@suse.de> wrote: >> > Shortly before this call is >> > >> > host->ios.power_mode = MMC_POWER_ON; >> > mmc_set_ios(host); >> > >> > and omap_hsmmc_set_ios() contains: >> > >> > switch (ios->power_mode) { >> > .... >> > case MMC_POWER_ON: >> > do_send_init_stream = 1; >> > break; >> > >> > >> > if (do_send_init_stream) >> > send_init_stream(host); >> > >> > Which sends the "init stream" to the card. >> > If the card is still being reset at this time, the stream may not >> > be effective. >> > >> > I find that about 10%-20% of the time when I release the reset line >> > *after* the sequence is sent, my card fails to initialised. When I >> > release *before* the sequence is sent, it never fails. >> >> Okay, thanks for providing these details. > > The right thing to do then is to pulse the reset line at the pre-power-up > stage. I don't think that will work, since the card hasn't yet been powered and will thus not respond properly to the reset. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2 February 2015 at 23:10, NeilBrown <neilb@suse.de> wrote: > On Mon, 2 Feb 2015 16:10:29 +0100 Ulf Hansson <ulf.hansson@linaro.org> wrote: > >> On 2 February 2015 at 16:05, Russell King - ARM Linux >> <linux@arm.linux.org.uk> wrote: >> > On Mon, Feb 02, 2015 at 03:57:37PM +0100, Ulf Hansson wrote: >> >> On 30 January 2015 at 23:27, NeilBrown <neilb@suse.de> wrote: >> >> > Shortly before this call is >> >> > >> >> > host->ios.power_mode = MMC_POWER_ON; >> >> > mmc_set_ios(host); >> >> > >> >> > and omap_hsmmc_set_ios() contains: >> >> > >> >> > switch (ios->power_mode) { >> >> > .... >> >> > case MMC_POWER_ON: >> >> > do_send_init_stream = 1; >> >> > break; >> >> > >> >> > >> >> > if (do_send_init_stream) >> >> > send_init_stream(host); >> >> > >> >> > Which sends the "init stream" to the card. >> >> > If the card is still being reset at this time, the stream may not >> >> > be effective. >> >> > >> >> > I find that about 10%-20% of the time when I release the reset line >> >> > *after* the sequence is sent, my card fails to initialised. When I >> >> > release *before* the sequence is sent, it never fails. >> >> >> >> Okay, thanks for providing these details. >> > >> > The right thing to do then is to pulse the reset line at the pre-power-up >> > stage. >> >> I don't think that will work, since the card hasn't yet been powered >> and will thus not respond properly to the reset. > > My understanding, at least in the case of my hardware, is that the power-on > is sufficient to reset the device. > In my case, the one supply is shared between two devices (wifi and bluetooth) > so if the bluetooth is on, then the wifi cannot be power-cycled. That is > when the reset is particularly needed. > > I'll try to arrange some testing to confirm this is the case. Of course, > this would not be general result, it would only be relevant for my hardware. I refreshed my memory for how the cw1200 WLAN device works on the ux500 SOC. I decided to post the complete information here, also for my own reference. These resources need to be controlled during the power sequence of CW1200: There are two GPIOs (one reset and one for SPI/SDIO selection), one LP (low power) clock and a few power supplies. The sequence for power up that needs to be followed are: 1. Keep reset GPIO asserted. 2. Enable power supplies. 3. Enable LP clock. 4. Wait a few LP clock cycles to let power/clock stabilize. 5. Make sure SPI/SDIO select pin is in SDIO mode. The value will be sampled by the WLAN device when reset GPIO is de-asserted. 6. De-assert reset GPIO. 7. Wait 30 ms for the WLAN device to power up. The power supplies may also be shared with a bluetooth device. That also means the reset GPIO must be kept asserted in power off state, to prevent power up of the WLAN device. From the above, it's clear that toggling the reset GPIOs in the ->pre_power_on() phase won't work for CW1200/UX500 case. That in conjunction with the "init stream" issue tells me that we need to progress with the below patch: "mmc: core: Invoke mmc_pwrseq_post_power_on() prior MMC_POWER_ON state" http://www.spinics.net/lists/arm-kernel/msg396592.html Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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 v4: - None. --- 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