mmc: improve power-on for sdio wifi card.
Subsystems (2)
cover mmc: improve power-on for sdio wifi card.
These two patches improve power-on management for my 'libertas' wifi card.
On my board the card shares a regulator with a bluetooth device, so turning off the regulator may not powercycle the card. To get it to a clean state it is necessary to hold the reset line down while enabling the regulator.
So the first patch add a reset-gpio function for all sdio cards which is configured through devicetree.
The sequence of commands sent to the sdio device for power-up differs between runtime power resume (which works nicely) and system-suspend resume (which doesn't).
The second patch add to calls to make these sequences the same and allowed my device to work reliably after system suspend (though the libertas driver needs a bit of work before it is completely reliable).
Thanks,
NeilBrown
---
NeilBrown (2):
mmc: core: allow a reset gpio to be configured.
mmc: core: reset sdio card properly on resume.
Documentation/devicetree/bindings/mmc/mmc.txt | 3 + drivers/mmc/core/core.c | 3 + drivers/mmc/core/host.c | 12 ++++ drivers/mmc/core/sdio.c | 8 ++- drivers/mmc/core/slot-gpio.c | 70 +++++++++++++++++++++++++ include/linux/mmc/slot-gpio.h | 4 + 6 files changed, 97 insertions(+), 3 deletions(-)
--
Signature
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 1/2 mmc: core: allow a reset gpio to be configured.
If the regulator supplying an SDIO device is shared with another device, the turning the regulator 'on' and 'off' will not actually cycle power and so will not reset the device.
This is particularly a problem for some wi2si wireless modules which have a BT module on chip and can share power lines. Without the power-cycle, subsequent reset commands fail.
So allow a 'reset' gpio to be specified. If provided, the line is asserted during power-up.
--- Documentation/devicetree/bindings/mmc/mmc.txt | 3 + drivers/mmc/core/core.c | 3 + drivers/mmc/core/host.c | 12 ++++ drivers/mmc/core/slot-gpio.c | 70 +++++++++++++++++++++++++ include/linux/mmc/slot-gpio.h | 4 + 5 files changed, 91 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/mmc/mmc.txt +3 -0
@@ -21,6 +21,9 @@ Optional properties: below for the case, when a GPIO is used for the CD line - wp-inverted: when present, polarity on the WP line is inverted. See the note below for the case, when a GPIO is used for the WP line +- reset-gpios: Specify a GPIO to be asserted during power-up. This is + useful is power is not actually removed (e.g. due to shared + regulator) but a reset is needed before reconfiguration. - max-frequency: maximum operating clock frequency - no-1-8-v: when present, denotes that 1.8v card voltage is not supported on this system, even if the controller claims it is.
drivers/mmc/core/core.c +3 -0
@@ -1533,6 +1533,8 @@ void mmc_power_up(struct mmc_host *host, u32 ocr)mmc_host_clk_hold (host); +/* Reset during power-off */ +mmc_gpio_set_rs (host,1 ); host->ios.vdd =fls (ocr) -1 ;if (mmc_host_is_spi (host)) host->ios.chip_select =MMC_CS_HIGH ;
@@ -1568,6 +1570,7 @@ void mmc_power_up(struct mmc_host *host, u32 ocr)* time required to reach a stable voltage. */ mmc_delay (10 ); +mmc_gpio_set_rs (host,0 );mmc_host_clk_release (host); }
drivers/mmc/core/host.c +12 -0
@@ -415,6 +415,18 @@ int mmc_of_parse(struct mmc_host *host)if (explicit_inv_wp ^ gpio_inv_wp) host->caps2 |=MMC_CAP2_RO_ACTIVE_HIGH ; + gpio =of_get_named_gpio_flags (np,"reset-gpios" ,0 , &flags); +if (gpio == -EPROBE_DEFER ) { + ret = -EPROBE_DEFER ; + goto out; + } +if (gpio_is_valid (gpio)) { + ret =mmc_gpio_request_rs (host, gpio, + flags &OF_GPIO_ACTIVE_LOW ); +if (ret <0 ) + goto out; + } +if (of_find_property (np,"cap-sd-highspeed" , &len)) host->caps |=MMC_CAP_SD_HIGHSPEED ;if (of_find_property (np,"cap-mmc-highspeed" , &len))
drivers/mmc/core/slot-gpio.c +69 -1
109 lines not on this page · raw message
include/linux/mmc/slot-gpio.h +4 -0
@@ -22,6 +22,10 @@ int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio, unsigned int debounce);void mmc_gpio_free_cd (struct mmc_host *host); +void mmc_gpio_set_rs (struct mmc_host *host,int state); +int mmc_gpio_request_rs (struct mmc_host *host,unsigned int gpio,int inv); +void mmc_gpio_free_rs (struct mmc_host *host); +int mmc_gpiod_request_cd (struct mmc_host *host,const char *con_id,unsigned int idx,bool override_active_level,unsigned int debounce);
Re: [PATCH 1/2] mmc: core: allow a reset gpio to be configured.
Quoted 11 lines · NeilBrown
If the regulator supplying an SDIO device is shared with another device, the turning the regulator 'on' and 'off' will not actually cycle power and so will not reset the device.
This is particularly a problem for some wi2si wireless modules which have a BT module on chip and can share power lines. Without the power-cycle, subsequent reset commands fail.
So allow a 'reset' gpio to be specified. If provided, the line is asserted during power-up.
There have been several attempts to fix similar issues as this patch does. The latest one I posted a few month ago, which wasn't accepted.
http://comments.gmane.org/gmane.linux.power-management.general/46635
There has also been some off-list discussions on especially how we should describe this in DT and there were actually some consensus made around that. Still I haven't seen any patches on the mailing lists.
So to summarize, I am really concerned that we keep having these power sequence issues for SDIO devices and right now the discussion has been on hold. I am considering to hack on it myself, since I am tired of waiting. :-)
Regarding this patch, I don't intent to apply it.
Kind regards
Uffe
Re: [PATCH 1/2] mmc: core: allow a reset gpio to be configured.
Quoted 11 lines · Ulf Hansson
Quoted 8 lines · NeilBrown
If the regulator supplying an SDIO device is shared with another device, the turning the regulator 'on' and 'off' will not actually cycle power and so will not reset the device.
This is particularly a problem for some wi2si wireless modules which have a BT module on chip and can share power lines. Without the power-cycle, subsequent reset commands fail.
Quoted 6 lines
Quoted 2 lines
So allow a 'reset' gpio to be specified. If provided, the line is asserted during power-up.
There have been several attempts to fix similar issues as this patch does. The latest one I posted a few month ago, which wasn't accepted.
http://comments.gmane.org/gmane.linux.power-management.general/46635
Note also that if the issue is about needing to take different actions depending on if the power actually got pulled you can request notification from the regulator API when things actually happen.
Re: [PATCH 1/2] mmc: core: allow a reset gpio to be configured.
In reply to Ulf HanssonQuoted 10 lines · On Fri, 28 Nov 2014 12:56:33 +0100 Ulf Hansson <[email protected]> wrote:
Quoted 9 lines · NeilBrown
If the regulator supplying an SDIO device is shared with another device, the turning the regulator 'on' and 'off' will not actually cycle power and so will not reset the device.
This is particularly a problem for some wi2si wireless modules which have a BT module on chip and can share power lines. Without the power-cycle, subsequent reset commands fail.
Quoted 6 lines
Quoted 2 lines
So allow a 'reset' gpio to be specified. If provided, the line is asserted during power-up.
There have been several attempts to fix similar issues as this patch does. The latest one I posted a few month ago, which wasn't accepted.
http://comments.gmane.org/gmane.linux.power-management.general/46635
Thanks for that link.
Quoted 4 lines
There has also been some off-list discussions on especially how we should describe this in DT and there were actually some consensus made around that. Still I haven't seen any patches on the mailing lists.
Wish I could have a link for those off-list discussions :-)
Based on what I read and my own thoughts about other devices that I'm having trouble managing I wonder if the right approach might be to admit that these buses are *not* 100% discoverable.
i.e. you can discover what is there once it is turned on, but you cannot discover how to turn it on.
So the *fix* is to allow attached devices to be explicitly listed. In my case I would create a child node of the mmc1 node, which is compatible="libertas,wifi" (or whatever the proper name is).
Then when the mmc1 wants to power-up, it does:
device_for_each_child(mmc_dev, NULL, power_up)
where:
static int power_up(struct device *dev, void *data)
{
pm_runtime_get_sync(dev);
return 0;
}Then I can put my reset-line management in the libertas driver instead of trying to include some of it in the mmc driver.
This has the advantage of the devicetree actually describing the hardware (there is a libertas wifi SDIO chip attached) rather than the behaviour (please turn on this regulator and toggle this GPIO to initialise the device).
I want to do a very similar thing for UARTs (so my GPS and Bluetooth turn on when /dev/ttyO? is opened), and I've been thinking about something similar for USB - I have a USB attached GSM module, but it also has an Audio link and some reset/interrupt lines that need to be configured.
If I could say to device tree "This USB port has this device attached", I think it would be a step in the right direction.
Quoted 5 lines
So to summarize, I am really concerned that we keep having these power sequence issues for SDIO devices and right now the discussion has been on hold. I am considering to hack on it myself, since I am tired of waiting. :-)
Please Cc me if you do. Meanwhile I'll try to hack together code supporting my latest idea and let you know if I get anywhere.
Quoted 2 lines
Regarding this patch, I don't intent to apply it.
Fair enough, I'm starting to not like it so much anyway.
Thanks,
NeilBrown
Re: [Gta04-owner] [PATCH 1/2] mmc: core: allow a reset gpio to be configured.
Hi Neil,
Am 02.12.2014 um 02:55 schrieb NeilBrown <[email protected]>:
Quoted 26 lines · On Fri, 28 Nov 2014 12:56:33 +0100 Ulf Hansson <[email protected]> wrote:
Quoted 11 lines · NeilBrown
Quoted 9 lines
If the regulator supplying an SDIO device is shared with another device, the turning the regulator 'on' and 'off' will not actually cycle power and so will not reset the device.
This is particularly a problem for some wi2si wireless modules which have a BT module on chip and can share power lines. Without the power-cycle, subsequent reset commands fail.
Quoted 6 lines
Quoted 2 lines
So allow a 'reset' gpio to be specified. If provided, the line is asserted during power-up.
There have been several attempts to fix similar issues as this patch does. The latest one I posted a few month ago, which wasn't accepted.
http://comments.gmane.org/gmane.linux.power-management.general/46635Thanks for that link.
Quoted 4 lines
There has also been some off-list discussions on especially how we should describe this in DT and there were actually some consensus made around that. Still I haven't seen any patches on the mailing lists.
Quoted 8 lines
Wish I could have a link for those off-list discussions :-)
Based on what I read and my own thoughts about other devices that I'm having trouble managing I wonder if the right approach might be to admit that these buses are *not* 100% discoverable.
i.e. you can discover what is there once it is turned on, but you cannot discover how to turn it on.
Indeed.
Quoted 4 lines
So the *fix* is to allow attached devices to be explicitly listed. In my case I would create a child node of the mmc1 node, which is compatible=“libertas,wifi" (or whatever the proper name is).
Sounds like a good idea to me.
Quoted 20 lines
Then when the mmc1 wants to power-up, it does:
device_for_each_child(mmc_dev, NULL, power_up)where:
static int
power_up(struct device *dev, void *data){ pm_runtime_get_sync(dev); return 0; }Then I can put my reset-line management in the libertas driver instead of trying to include some of it in the mmc driver.
This has the advantage of the devicetree actually describing the hardware (there is a libertas wifi SDIO chip attached) rather than the behaviour (please turn on this regulator and toggle this GPIO to initialise the device).
Quoted 6 lines
I want to do a very similar thing for UARTs (so my GPS and Bluetooth turn on when
/dev/ttyO? is opened), and I've been thinking about something similar for USB - I have a USB attached GSM module, but it also has an Audio link and some reset/interrupt lines that need to be configured.
If I could say to device tree "This USB port has this device attached", I think it would be a step in the right direction.
Thinking a little further, it could either be the core driver of the device/bus/protocol or a special driver that only does power management. Or audio.
And we should consider using a list of strings in the compatible entry so that several drivers can be loaded if the subsystem structure shows that this is simpler.
It could be one for power, one for audio. Or in the case of the libertas a separate power&reset driver for a specific chip that uses the libertas driver so that chip specific reset management is not introduced into the libertas core but separate.
For the usb connected modem the subnode to be attached is likely the PHY where the self-powered device is connected to.
Quoted 9 lines
Quoted 8 lines
So to summarize, I am really concerned that we keep having these power sequence issues for SDIO devices and right now the discussion has been on hold. I am considering to hack on it myself, since I am tired of waiting. :-)
Quoted 8 lines
Please Cc me if you do. Meanwhile I'll try to hack together code supporting my latest idea and let you know if I get anywhere.
Quoted 2 lines
Regarding this patch, I don't intent to apply it.
Fair enough, I’m starting to not like it so much anyway.
BR,
Nikolaus
Re: [PATCH 1/2] mmc: core: allow a reset gpio to be configured.
In reply to NeilBrownQuoted 41 lines · NeilBrown
Quoted 10 lines · On Fri, 28 Nov 2014 12:56:33 +0100 Ulf Hansson <[email protected]> wrote:
Quoted 9 lines · NeilBrown
If the regulator supplying an SDIO device is shared with another device, the turning the regulator 'on' and 'off' will not actually cycle power and so will not reset the device.
This is particularly a problem for some wi2si wireless modules which have a BT module on chip and can share power lines. Without the power-cycle, subsequent reset commands fail.
Quoted 6 lines
Quoted 2 lines
So allow a 'reset' gpio to be specified. If provided, the line is asserted during power-up.
There have been several attempts to fix similar issues as this patch does. The latest one I posted a few month ago, which wasn't accepted.
http://comments.gmane.org/gmane.linux.power-management.general/46635Thanks for that link.
Quoted 4 lines
There has also been some off-list discussions on especially how we should describe this in DT and there were actually some consensus made around that. Still I haven't seen any patches on the mailing lists.
Wish I could have a link for those off-list discussions :-)
Based on what I read and my own thoughts about other devices that I'm having trouble managing I wonder if the right approach might be to admit that these buses are *not* 100% discoverable.
i.e. you can discover what is there once it is turned on, but you cannot discover how to turn it on.
So the *fix* is to allow attached devices to be explicitly listed. In my case I would create a child node of the mmc1 node, which is compatible="libertas,wifi" (or whatever the proper name is).
Then when the mmc1 wants to power-up, it does:
Quoted 7 lines
device_for_each_child(mmc_dev, NULL, power_up)where:
static int
power_up(struct device *dev, void *data){ pm_runtime_get_sync(dev);
No. We must not rely on runtime PM to be able to power up the device.
Quoted 5 lines
return 0; }Then I can put my reset-line management in the libertas driver instead of trying to include some of it in the mmc driver.
Well, "somewhere" we need to handle the different power up scenarios. These scenarios should be considered as SOC specific, and if we could keep that as a separate piece of code, that would be the best.
So, I agree that we shouldn't pollute mmc host drivers with such code, but I also think SDIO func drivers should be remained untouched.
Instead, my plan is to let the mmc core handle it.
Quoted 4 lines
This has the advantage of the devicetree actually describing the hardware (there is a libertas wifi SDIO chip attached) rather than the behaviour (please turn on this regulator and toggle this GPIO to initialise the device).
I think both ways are viable, since they are both describing the hardware and the characteristics of it.
Quoted 11 lines
I want to do a very similar thing for UARTs (so my GPS and Bluetooth turn on when
/dev/ttyO? is opened), and I've been thinking about something similar for USB - I have a USB attached GSM module, but it also has an Audio link and some reset/interrupt lines that need to be configured.
If I could say to device tree "This USB port has this device attached", I think it would be a step in the right direction.Quoted 1 line
Quoted 7 lines
Quoted 4 lines
So to summarize, I am really concerned that we keep having these power sequence issues for SDIO devices and right now the discussion has been on hold. I am considering to hack on it myself, since I am tired of waiting. :-)
Please Cc me if you do. Meanwhile I'll try to hack together code supporting my latest idea and let you know if I get anywhere.
Sounds good. I have started hacking on it as well, let's see where we end up.
Kind regards
Uffe
PATCH 2/2 mmc: core: reset sdio card properly on resume.
In reply to NeilBrownmmc_sdio_power_restore calls
mmc_send_if_cond(host, host->ocr_avail); ret = mmc_send_io_op_cond(host, 0, NULL);
between mmc_go_idle() and mmc_sdio_init_card(). mmc_sdio_resume needs to as well, else my libertas sdio wifi device doesn't resume properly from suspend.
--- drivers/mmc/core/sdio.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/mmc/core/sdio.c +6 -2
@@ -981,8 +981,12 @@ static int mmc_sdio_resume(struct mmc_host *host)if (mmc_card_is_removable (host) || !mmc_card_keep_power (host)) {sdio_reset (host);mmc_go_idle (host); - err =mmc_sdio_init_card (host, host->card->ocr, host->card, -mmc_card_keep_power (host)); +mmc_send_if_cond (host, host->ocr_avail); + err =mmc_send_io_op_cond (host,0 ,NULL ); +if (!err) + err =mmc_sdio_init_card (host, host->card->ocr, + host->card, +mmc_card_keep_power (host)); }else if (mmc_card_keep_power (host) &&mmc_card_wake_sdio_irq (host)) {/* We may have switched to 1-bit mode during suspend */ err =sdio_enable_4bit_bus (host->card);
Re: [PATCH 2/2] mmc: core: reset sdio card properly on resume.
Quoted 19 lines · NeilBrown
mmc_sdio_power_restorecallsmmc_send_if_cond(host, host->ocr_avail); ret = mmc_send_io_op_cond(host, 0, NULL);between
mmc_go_idle()andmmc_sdio_init_card().mmc_sdio_resumeneeds to as well, else my libertas sdio wifi device doesn't resume properly from suspend.Signed-off-by NeilBrown <[email protected]>--- drivers/mmc/core/sdio.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)drivers/mmc/core/sdio.c +0
-0
drivers/mmc/core/sdio.c static int mmc_sdio_resume(struct mmc_host *host) · line 984
Quoted 6 lines
drivers/mmc/core/sdio.c +1
-2@@ -981,8 +981,12 @@ static int mmc_sdio_resume(struct mmc_host *host)if (mmc_card_is_removable (host) || !mmc_card_keep_power (host)) {sdio_reset (host);mmc_go_idle (host); - err =mmc_sdio_init_card (host, host->card->ocr, host->card, -mmc_card_keep_power (host)); +mmc_send_if_cond (host, host->ocr_avail);
/s /host->ocr_avail /host->card->ocr
I would expect that to work. I do realize that "host->ocr_avail" is being used in the ->power_restore() callback, but I think that's wrong as well. Could you maybe verify that changing to host->card->ocr works in this path as well? That's would of course be a separate patch.
drivers/mmc/core/sdio.c static int mmc_sdio_resume(struct mmc_host *host) · line 992
Quoted 10 lines
drivers/mmc/core/sdio.c +5
-0+ err =mmc_send_io_op_cond (host,0 ,NULL ); +if (!err) + err =mmc_sdio_init_card (host, host->card->ocr, + host->card, +mmc_card_keep_power (host)); }else if (mmc_card_keep_power (host) &&mmc_card_wake_sdio_irq (host)) {/* We may have switched to 1-bit mode during suspend */ err =sdio_enable_4bit_bus (host->card);
Kind regards
Uffe
Re: [PATCH 2/2] mmc: core: reset sdio card properly on resume.
Quoted 29 lines · On Mon, 10 Nov 2014 14:29:19 +0100 Ulf Hansson…
Quoted 19 lines · NeilBrown
mmc_sdio_power_restorecallsmmc_send_if_cond(host, host->ocr_avail); ret = mmc_send_io_op_cond(host, 0, NULL);between
mmc_go_idle()andmmc_sdio_init_card().mmc_sdio_resumeneeds to as well, else my libertas sdio wifi device doesn't resume properly from suspend.Signed-off-by NeilBrown <[email protected]>--- drivers/mmc/core/sdio.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)drivers/mmc/core/sdio.c +0
-0drivers/mmc/core/sdio.c static int mmc_sdio_resume(struct mmc_host *host) · line 984
Quoted 6 lines
drivers/mmc/core/sdio.c +1
-2@@ -981,8 +981,12 @@ static int mmc_sdio_resume(struct mmc_host *host)if (mmc_card_is_removable (host) || !mmc_card_keep_power (host)) {sdio_reset (host);mmc_go_idle (host); - err =mmc_sdio_init_card (host, host->card->ocr, host->card, -mmc_card_keep_power (host)); +mmc_send_if_cond (host, host->ocr_avail);/s /host->ocr_avail /host->card->ocr
Quoted 4 lines
I would expect that to work. I do realize that "
host->ocr_avail" is being used in the ->power_restore() callback, but I think that's wrong as well. Could you maybe verify that changing tohost->card->ocrworks in this path as well? That's would of course be a separate patch.
Hi Ulf,
I made the substitution in mmc_sdio_resume and mmc_sdio_power_restore
and my sdio device continues to work, including after resume.
The host->card->ocr value is 0x10000, the host->ocr_avail value is 0x18000.
If you like I'll send the two patches formally, but it might be a day or so.
Thanks,
NeilBrown
Quoted 14 lines
drivers/mmc/core/sdio.c static int mmc_sdio_resume(struct mmc_host *host) · line 992
Quoted 11 lines
drivers/mmc/core/sdio.c +5
-0+ err =mmc_send_io_op_cond (host,0 ,NULL ); +if (!err) + err =mmc_sdio_init_card (host, host->card->ocr, + host->card, +mmc_card_keep_power (host)); }else if (mmc_card_keep_power (host) &&mmc_card_wake_sdio_irq (host)) {/* We may have switched to 1-bit mode during suspend */ err =sdio_enable_4bit_bus (host->card);Kind regards
Uffe
Re: [PATCH 2/2] mmc: core: reset sdio card properly on resume.
Quoted 36 lines · NeilBrown
Quoted 29 lines · On Mon, 10 Nov 2014 14:29:19 +0100 Ulf Hansson <[email protected]> wrote:
Quoted 19 lines · NeilBrown
mmc_sdio_power_restorecallsmmc_send_if_cond(host, host->ocr_avail); ret = mmc_send_io_op_cond(host, 0, NULL);between
mmc_go_idle()andmmc_sdio_init_card().mmc_sdio_resumeneeds to as well, else my libertas sdio wifi device doesn't resume properly from suspend.Signed-off-by NeilBrown <[email protected]>--- drivers/mmc/core/sdio.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)drivers/mmc/core/sdio.c +0
-0drivers/mmc/core/sdio.c static int mmc_sdio_resume(struct mmc_host *host) · line 984
Quoted 6 lines
drivers/mmc/core/sdio.c +1
-2@@ -981,8 +981,12 @@ static int mmc_sdio_resume(struct mmc_host *host)if (mmc_card_is_removable (host) || !mmc_card_keep_power (host)) {sdio_reset (host);mmc_go_idle (host); - err =mmc_sdio_init_card (host, host->card->ocr, host->card, -mmc_card_keep_power (host)); +mmc_send_if_cond (host, host->ocr_avail);/s /host->ocr_avail /host->card->ocr
Quoted 4 lines
I would expect that to work. I do realize that "
host->ocr_avail" is being used in the ->power_restore() callback, but I think that's wrong as well. Could you maybe verify that changing tohost->card->ocrworks in this path as well? That's would of course be a separate patch.
Quoted 5 lines
Hi Ulf,
I made the substitution inmmc_sdio_resumeandmmc_sdio_power_restore
and my sdio device continues to work, including after resume.The
host->card->ocrvalue is0x10000, thehost->ocr_availvalue is0x18000.
That makes sense.
The host->card->ocr mask has been negotiated at initialization between the host and the card.
Quoted 2 lines
If you like I'll send the two patches formally, but it might be a day or so.
Please do.
Kind regards
Uffe