[PATCH] mmc: sdio: reset card during power_restore

[PATCH] mmc: sdio: reset card during power_restore

[Daniel Drake]

mmc_sdio_power_restore() skips some steps that are performed in other
power-related codepaths which are necessary to fully reset the card.
Without this, the card can't be powered up and probe fails.

All these steps are needed, to satisfy the cases of both normal
runtime PM and also suspend/resume situations.

Tested on sd8686 libertas wifi on XO-1.5.

Signed-off-by: Daniel Drake <dsd@laptop.org>
---
 drivers/mmc/core/sdio.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 8af3330..9170ea2 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -706,10 +706,25 @@ static int mmc_sdio_power_restore(struct mmc_host *host)
 	BUG_ON(!host->card);
 
 	mmc_claim_host(host);
+
+	/*
+	 * Reset the card by performing the same steps that are taken by
+	 * mmc_rescan_try_freq() and mmc_attach_sdio() during a "normal" probe
+	 */
+	sdio_reset(host);
+	mmc_go_idle(host);
+	mmc_send_if_cond(host, host->ocr_avail);
+
+	ret = mmc_send_io_op_cond(host, 0, NULL);
+	if (ret)
+		goto out;
+
 	ret = mmc_sdio_init_card(host, host->ocr, host->card,
 				mmc_card_keep_power(host));
 	if (!ret && host->sdio_irqs)
 		mmc_signal_sdio_irq(host);
+
+out:
 	mmc_release_host(host);
 
 	return ret;
-- 
1.7.5.2

Re: [PATCH] mmc: sdio: reset card during power_restore

[Ohad Ben-Cohen]

Hi Daniel,

On Sun, Jun 5, 2011 at 3:38 PM, Daniel Drake <dsd@laptop.org> wrote:
> @@ -706,10 +706,25 @@ static int mmc_sdio_power_restore(struct mmc_host *host)
>        BUG_ON(!host->card);
>
>        mmc_claim_host(host);
> +
> +       /*
> +        * Reset the card by performing the same steps that are taken by
> +        * mmc_rescan_try_freq() and mmc_attach_sdio() during a "normal" probe
> +        */
> +       sdio_reset(host);

Sending a reset is necessary only if we are re-initing a powered on
card, but in this case, we know that we just powered the card up, so
this is not needed.

+       ret = mmc_send_io_op_cond(host, 0, NULL);
+       if (ret)
+               goto out;

Can you please add this under a card quirk ?

The spec does not require this for embedded sdio cards, and we would
like to skip this for wl12xx.

Thanks,
Ohad.

Re: [PATCH] mmc: sdio: reset card during power_restore

[Daniel Drake]

On 5 June 2011 14:48, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> +
>> +       /*
>> +        * Reset the card by performing the same steps that are taken by
>> +        * mmc_rescan_try_freq() and mmc_attach_sdio() during a "normal" probe
>> +        */
>> +       sdio_reset(host);
>
> Sending a reset is necessary only if we are re-initing a powered on
> card, but in this case, we know that we just powered the card up, so
> this is not needed.

Don't ask me for an explanation, but this is required here, for the
following scenario:

1. Power up and probe
2. Power down via runtime pm
3. Power up

step 3 needs the reset (even though it wasnt needed in step 1),
otherwise mmc_send_io_op_cond() fails with a timeout.

It is also needed for:

1. Power up
2. Suspend, libertas handler returns -ENOSYS so card gets powered down
3. System resume
4. Reprobe needs to power up card

Step 4 fails without this reset.

> +       ret = mmc_send_io_op_cond(host, 0, NULL);
> +       if (ret)
> +               goto out;
>
> Can you please add this under a card quirk ?
>
> The spec does not require this for embedded sdio cards, and we would
> like to skip this for wl12xx.

I could do this if it is required for merge, but I don't agree with
it. Are you suggesting that a quirk is used to enable this reset, or
to disable it? It seems overkill for something that is going to be
trivial and harmless to your setup.

Also, the most frustrating thing when working on this issue is that
the codepaths for powering up a SD card between the following 3
situations are significantly different:
 1. Power up and probe during boot
 2. Resume from suspend
 3. Power up after runtime suspend

It feels like there should be a lot more in common here. My patch is
adding some coherence to the process (and there is still plenty of
room for improvement, but adding quirks won't help...).

Daniel

Re: [PATCH] mmc: sdio: reset card during power_restore

[Ohad Ben-Cohen]

On Tue, Jun 7, 2011 at 7:41 PM, Daniel Drake <dsd@laptop.org> wrote:
> On 5 June 2011 14:48, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> Sending a reset is necessary only if we are re-initing a powered on
>> card, but in this case, we know that we just powered the card up, so
>> this is not needed.
>
> Don't ask me for an explanation, but this is required here

Uhm, strange. Maybe this has something to do with the sd8686's reset
line which you are not toggling ?

Btw, the previous patch which you sent didn't have this reset command,
and the patch did work for you. What has changed in this respect ?

>  Are you suggesting that a quirk is used to enable this reset, or
> to disable it?

To disable the CMD5 arg=0. it's not mandatory by the spec, and some
cards don't need it. It's not harmful either way, but it's just
cleaner for those cards (and it's really just adding 1 'if'
statement..).

Re: [PATCH] mmc: sdio: reset card during power_restore

[Ohad Ben-Cohen]

On Tue, Jun 7, 2011 at 7:41 PM, Daniel Drake <dsd@laptop.org> wrote:
> Also, the most frustrating thing when working on this issue is that
> the codepaths for powering up a SD card between the following 3
> situations are significantly different:
>  1. Power up and probe during boot
>  2. Resume from suspend
>  3. Power up after runtime suspend
>
> It feels like there should be a lot more in common here.

Definitely agree. I also suggested this in my first reply to you on
our other thread:

http://www.mail-archive.com/linux-mmc@vger.kernel.org/msg08193.html

Re: [PATCH] mmc: sdio: reset card during power_restore

[Daniel Drake]

On 7 June 2011 21:52, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Uhm, strange. Maybe this has something to do with the sd8686's reset
> line which you are not toggling ?

The GPIO reset line doesn't seem to make any difference here.

> Btw, the previous patch which you sent didn't have this reset command,
> and the patch did work for you. What has changed in this respect ?

In the first patch I sent I had only tested basic power-up after
probe, and found it to be working. Then I tested the other scenarios
listed in my last mail - basically powering it off and on *again* -
and found them to be broken, but fixed by further mirroring what
existing non-rpm code would do in order to power up a SD card.

Daniel

Re: [PATCH] mmc: sdio: reset card during power_restore

[Ohad Ben-Cohen]

On Wed, Jun 8, 2011 at 12:20 PM, Daniel Drake <dsd@laptop.org> wrote:
> In the first patch I sent I had only tested basic power-up after
> probe, and found it to be working. Then I tested the other scenarios
> listed in my last mail - basically powering it off and on *again* -
> and found them to be broken, but fixed by further mirroring what
> existing non-rpm code would do in order to power up a SD card.

Sounds like you didn't power the card off then in the driver after probe ?

Can you show the diff with which you did this experiment ?

There is no difference between powering the card up in sdio_bus_probe
to powering it up later in the driver. If the card was indeed powered
off beforehand, then the latter should work too. Your reset command
strongly suggests that wasn't the case.

Re: [PATCH] mmc: sdio: reset card during power_restore

[Daniel Drake]

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

On 8 June 2011 10:33, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Sounds like you didn't power the card off then in the driver after probe ?
>
> Can you show the diff with which you did this experiment ?

Attached. This is on top of your fix for correct driver deinit during suspend.

Test case is:
1. boot, load libertas driver
2. suspend in a way that libertas returns -ENOSYS to power down card
(this is the default)
3. resume
4. MMC layer powers up card to identify it, then powers it down, then
fails to power it up for libertas probe

If I add the reset (shown commented-out in the patch), it matches the
behaviour of other known-good codepaths and also solves the problem.

Output logs are:
bash-4.1# insmod /media/4E11-1D7C/libertas_sdio.ko
[   53.847863] libertas_sdio: Libertas SDIO driver
[   53.853104] libertas_sdio: Copyright Pierre Ossman
[   53.858040] mmc_power_restore_host mmc1
[   54.809019] libertas_sdio mmc1:0001:1: (unregistered net_device):
00:17:c4:a7:50:57, fw 9.70.3p36, cap 0x000003a3
[   54.825485] libertas_sdio mmc1:0001:1: wlan0: Marvell WLAN 802.11 adapter
bash-4.1# [   54.920650] udev[992]: renamed network interface wlan0 to eth0
[   54.978168] ieee80211 phy0: assoc: bss   (null) not in scan results
[   56.295225] ieee80211 phy0: assoc: bss   (null) not in scan results
[   56.337101] cfg80211: Calling CRDA for country: EU
bash-4.1# echo mem > /sys/power/state
[   65.393797] PM: Syncing filesystems ...
[   65.672372] done.
[   65.803121] Freezing user space processes ... (elapsed 0.01 seconds) done.
[   65.823048] Freezing remaining freezable tasks ... (elapsed 0.01
seconds) done.
[   65.884808] i8042 kbd 00:04: wake-up capability enabled by ACPI
[   65.891251] i8042 aux 00:03: wake-up capability disabled by ACPI
[   65.897820] ehci_hcd 0000:00:10.4: PCI INT D disabled
[   65.903294] uhci_hcd 0000:00:10.2: PCI INT C disabled
[   65.908900] uhci_hcd 0000:00:10.1: PCI INT B disabled
[   65.914431] libertas_sdio mmc1:0001:1: mmc1:0001:1: suspend: PM flags = 0x3
[   65.922077] uhci_hcd 0000:00:10.0: PCI INT A disabled
[   65.927578] viafb 0000:00:01.0: PCI INT A disabled
[   65.932646] libertas_sdio mmc1:0001:1: Suspend without wake params
-- powering down card
[   65.944166] cfg80211: Calling CRDA to update world regulatory domain
[   66.010534] mmc1: card 0001 removed
[   66.014310] sdhci-pci 0000:00:0c.0: PCI INT A disabled
[   66.030103] PM: suspend of devices complete after 186.534 msecs
[   66.110284] PM: late suspend of devices complete after 74.153 msecs
[   66.116751] ACPI: Preparing to enter system sleep state S3
[   66.122976] PM: Saving platform NVS memory
+r[   66.122976] ACPI: Low-level resume complete
[   66.122976] PM: Restoring platform NVS memory
[   66.122976] ACPI: Waking up from system sleep state S3
[   66.150047] uhci_hcd 0000:00:10.0: BAR 4: set to [io
0x8000-0x801f] (PCI address [0x8000-0x801f])
[   66.170036] uhci_hcd 0000:00:10.1: BAR 4: set to [io
0x8020-0x803f] (PCI address [0x8020-0x803f])
[   66.190035] uhci_hcd 0000:00:10.2: BAR 4: set to [io
0x8040-0x805f] (PCI address [0x8040-0x805f])
[   66.210034] ehci_hcd 0000:00:10.4: BAR 0: set to [mem
0x80003000-0x800030ff] (PCI address [0x80003000-0x800030ff])
[   66.220917] PM: early resume of devices complete after 87.797 msecs
[   66.227558] viafb 0000:00:01.0: PCI INT A -> GSI 16 (level, low) -> IRQ 16
[   66.234561] sdhci-pci 0000:00:0c.0: PCI INT A -> GSI 22 (level,
low) -> IRQ 22
[   66.241931] uhci_hcd 0000:00:10.0: PCI INT A -> GSI 20 (level, low) -> IRQ 20
[   66.249168] usb usb2: root hub lost power or was reset
[   66.260047] uhci_hcd 0000:00:10.1: PCI INT B -> GSI 22 (level, low) -> IRQ 22
[   66.260077] usb usb3: root hub lost power or was reset
[   66.260116] uhci_hcd 0000:00:10.2: PCI INT C -> GSI 21 (level, low) -> IRQ 21
[   66.260145] usb usb4: root hub lost power or was reset
[   66.260179] ehci_hcd 0000:00:10.4: PCI INT D -> GSI 23 (level, low) -> IRQ 23
[   66.260761] i8042 kbd 00:04: wake-up capability disabled by ACPI
[   66.560225] PM: resume of devices complete after 332.791 msecs
[   66.580429] Restarting tasks ... done.
[   66.650427] mmc0: mmc_rescan_try_freq: trying to init card at 400000 Hz
bash-4.1# [   66.690461] cfg80211: World regulatory domain updated:
[   66.695701] cfg80211:     (start_freq - end_freq @ bandwidth),
(max_antenna_gain, max_eirp)
[   66.733049] cfg80211:     (2402000 KHz - 2472000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[   66.751195] cfg80211:     (2457000 KHz - 2482000 KHz @ 20000 KHz),
(300 mBi, 2000 mBm)
[   66.759333] cfg80211:     (2474000 KHz - 2494000 KHz @ 20000 KHz),
(300 mBi, 2000 mBm)
[   66.795426] mmc0: mmc_rescan_try_freq: trying to init card at 300000 Hz
[   66.808884] cfg80211:     (5170000 KHz - 5250000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[   66.830830] cfg80211:     (5735000 KHz - 5835000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[   66.865778] cfg80211: Calling CRDA to update world regulatory domain
[   66.905733] cfg80211: World regulatory domain updated:
[   66.917952] mmc0: mmc_rescan_try_freq: trying to init card at 200000 Hz
[   66.968065] cfg80211:     (start_freq - end_freq @ bandwidth),
(max_antenna_gain, max_eirp)
[   66.985155] cfg80211:     (2402000 KHz - 2472000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[   67.015441] cfg80211:     (2457000 KHz - 2482000 KHz @ 20000 KHz),
(300 mBi, 2000 mBm)
[   67.045234] mmc0: mmc_rescan_try_freq: trying to init card at 187500 Hz
[   67.053616] cfg80211:     (2474000 KHz - 2494000 KHz @ 20000 KHz),
(300 mBi, 2000 mBm)
[   67.122604] cfg80211:     (5170000 KHz - 5250000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[   67.151879] mmc1: mmc_rescan_try_freq: trying to init card at 400000 Hz
[   67.189830] cfg80211:     (5735000 KHz - 5835000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[   67.296211] mmc1: new SDIO card at address 0001
[   67.337105] mmc_power_save_host mmc1
[   67.356088] mmc_power_restore_host mmc1
[   67.471529] CMD5 reset returned -110
[   67.477144] libertas_sdio: probe of mmc1:0001:1 failed with error -16

[-- Attachment #2: sdio-rpm-cant-power-up.txt --]
[-- Type: text/plain, Size: 2074 bytes --]

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 68091dd..92405e2 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1662,7 +1662,7 @@ void mmc_stop_host(struct mmc_host *host)
 int mmc_power_save_host(struct mmc_host *host)
 {
 	int ret = 0;
-
+printk("mmc_power_save_host %s\n", mmc_hostname(host));
 	mmc_bus_get(host);
 
 	if (!host->bus_ops || host->bus_dead || !host->bus_ops->power_restore) {
@@ -1685,6 +1685,7 @@ int mmc_power_restore_host(struct mmc_host *host)
 {
 	int ret;
 
+printk("mmc_power_restore_host %s\n", mmc_hostname(host));
 	mmc_bus_get(host);
 
 	if (!host->bus_ops || host->bus_dead || !host->bus_ops->power_restore) {
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 8af3330..61ee53a 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -706,10 +706,27 @@ static int mmc_sdio_power_restore(struct mmc_host *host)
 	BUG_ON(!host->card);
 
 	mmc_claim_host(host);
+
+	/*
+	 * Reset the card by performing the same steps that are taken by
+	 * mmc_rescan_try_freq() and mmc_attach_sdio() during a "normal" probe
+	 */
+	//sdio_reset(host);
+	//mmc_go_idle(host);
+	//mmc_send_if_cond(host, host->ocr_avail);
+
+	ret = mmc_send_io_op_cond(host, 0, NULL);
+	if (ret) {
+		printk("CMD5 reset returned %d\n", ret);
+		//goto out;
+	}
+
 	ret = mmc_sdio_init_card(host, host->ocr, host->card,
 				mmc_card_keep_power(host));
 	if (!ret && host->sdio_irqs)
 		mmc_signal_sdio_irq(host);
+
+out:
 	mmc_release_host(host);
 
 	return ret;
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 58d5436..ce3e2e2 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2488,7 +2488,7 @@ int sdhci_add_host(struct sdhci_host *host)
 	} else
 		mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200;
 
-	mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE | MMC_CAP_CMD23;
+	mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE | MMC_CAP_CMD23 | MMC_CAP_POWER_OFF_CARD;
 
 	if (host->quirks & SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12)
 		host->flags |= SDHCI_AUTO_CMD12;

Re: [PATCH] mmc: sdio: reset card during power_restore

[Ohad Ben-Cohen]

On Wed, Jun 8, 2011 at 4:36 PM, Daniel Drake <dsd@laptop.org> wrote:
> If I add the reset (shown commented-out in the patch), it matches the
> behaviour of other known-good codepaths and also solves the problem.

Yeah, but we don't know why. And we may very well be covering a
different bug if we just take it as-is.

Don't get me wrong, I have no objection to have this if it's needed,
but we shouldn't do this just "because it works".

So I'll try to look at the logs you provided a bit more later, but the
main questions I have are:

1. How come power off+on works for you in the first time, but doesn't
work in the second time ?
2. Was the card really powered off in this 2nd-time failed scenario ?
3. what if you do a series of insmod+rmmod ? does this work ? (power
should be taken down and up every time)
4. Are you also interested in powering off the card when the wlan
interface is down (i.e. coupling the power of the card with the state
of the interface) too ? it should be easy to debug.

Re: [PATCH] mmc: sdio: reset card during power_restore

[Daniel Drake]

On 8 June 2011 15:02, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Wed, Jun 8, 2011 at 4:36 PM, Daniel Drake <dsd@laptop.org> wrote:
>> If I add the reset (shown commented-out in the patch), it matches the
>> behaviour of other known-good codepaths and also solves the problem.
>
> Yeah, but we don't know why. And we may very well be covering a
> different bug if we just take it as-is.
>
> Don't get me wrong, I have no objection to have this if it's needed,
> but we shouldn't do this just "because it works".
>
> So I'll try to look at the logs you provided a bit more later, but the
> main questions I have are:
>
> 1. How come power off+on works for you in the first time, but doesn't
> work in the second time ?

I assume it is because a different codepath was taken in order to
power up the device the first time, before it then got powered down,
compared to the codepath that failed to power it up the 2nd time. My
efforts so far have been based around eliminating the differences in
those codepaths, and this approach seems to be successful.

> 2. Was the card really powered off in this 2nd-time failed scenario ?

How do you suggest I check that? I believe it was, since
mmc_stop_host() got called, and colleagues with proper equipment
measured the difference in power usage a while back.

> 3. what if you do a series of insmod+rmmod ? does this work ? (power
> should be taken down and up every time)

rmmod doesn't appear to take down the power.

> 4. Are you also interested in powering off the card when the wlan
> interface is down (i.e. coupling the power of the card with the state
> of the interface) too ? it should be easy to debug.

Yes, and I have a patch ready that does that. But first it depends on
getting the power on/off routines working reliably.

Thanks for your help.

Daniel

Re: [PATCH] mmc: sdio: reset card during power_restore

[Ohad Ben-Cohen]

On Wed, Jun 8, 2011 at 5:21 PM, Daniel Drake <dsd@laptop.org> wrote:
>> 1. How come power off+on works for you in the first time, but doesn't
>> work in the second time ?
>
> I assume it is because a different codepath was taken in order to
> power up the device the first time, before it then got powered down,
> compared to the codepath that failed to power it up the 2nd time. My
> efforts so far have been based around eliminating the differences in
> those codepaths, and this approach seems to be successful.

We have 3 power-on paths here: on boot, on insmod, and and resume.

I understand the first one succeeds (naturally), and the last one
failed, but now I'm confused what was the outcome of the 2nd (without
sending a reset cmd) ?

>> 3. what if you do a series of insmod+rmmod ? does this work ? (power
>> should be taken down and up every time)
>
> rmmod doesn't appear to take down the power.

Let's nail this one first. if we get it right, the rest will immediately follow.

Please reboot, and immediately after booting (without insmoding the
driver) tell me what's the output of :

mount -t debugfs none /sys/kernel/debug
cat /sys/kernel/debug/mmc1/ios

Then insmod the driver, and tell me again what's the output of
/sys/.../mmc1/ios.

Please send a contiguous terminal session of these steps (incl. the
boot messages), thanks !

Re: [PATCH] mmc: sdio: reset card during power_restore

[Daniel Drake]

On 8 June 2011 21:05, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> We have 3 power-on paths here: on boot, on insmod, and and resume.
>
> I understand the first one succeeds (naturally), and the last one
> failed, but now I'm confused what was the outcome of the 2nd (without
> sending a reset cmd) ?

Not really sure what you're asking. Unless its the test below?

>>> 3. what if you do a series of insmod+rmmod ? does this work ? (power
>>> should be taken down and up every time)
>>
>> rmmod doesn't appear to take down the power.
>
> Let's nail this one first. if we get it right, the rest will immediately follow.
>
> Please reboot, and immediately after booting (without insmoding the
> driver) tell me what's the output of :
>
> mount -t debugfs none /sys/kernel/debug
> cat /sys/kernel/debug/mmc1/ios
>
> Then insmod the driver, and tell me again what's the output of
> /sys/.../mmc1/ios.

Which base kernel setup should I run these tests on?

Thanks,
Daniel

Re: [PATCH] mmc: sdio: reset card during power_restore

[Ohad Ben-Cohen]

On Wed, Jun 8, 2011 at 11:58 PM, Daniel Drake <dsd@laptop.org> wrote:
>> Please reboot, and immediately after booting (without insmoding the
>> driver) tell me what's the output of :
>>
>> mount -t debugfs none /sys/kernel/debug
>> cat /sys/kernel/debug/mmc1/ios
>
> Which base kernel setup should I run these tests on?

Vanilla, with your temporary patch below.

Without this patch, mmc1 (I assume this what drives sd8686 in your
setup) should be powered on. With it, it should be powered off.

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 58d5436..ce3e2e2 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2488,7 +2488,7 @@ int sdhci_add_host(struct sdhci_host *host)
 	} else
 		mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200;

-	mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE | MMC_CAP_CMD23;
+	mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE | MMC_CAP_CMD23 |
MMC_CAP_POWER_OFF_CARD;

 	if (host->quirks & SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12)
 		host->flags |= SDHCI_AUTO_CMD12;

Re: [PATCH] mmc: sdio: reset card during power_restore

[Daniel Drake]

On 8 June 2011 21:05, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Please reboot, and immediately after booting (without insmoding the
> driver) tell me what's the output of :
>
> mount -t debugfs none /sys/kernel/debug
> cat /sys/kernel/debug/mmc1/ios
>
> Then insmod the driver, and tell me again what's the output of
> /sys/.../mmc1/ios.

bash-4.1# mount -t debugfs none /sys/kernel/debug
bash-4.1# cat /sys/kernel/debug/mmc1/ios
clock:		0 Hz
vdd:		0 (invalid)
bus mode:	1 (open drain)
chip select:	0 (don't care)
power mode:	0 (off)
bus width:	0 (1 bits)
timing spec:	0 (legacy)
bash-4.1# insmod /media/4E11-1D7C/libertas_sdio.ko
[   67.917748] libertas_sdio: Libertas SDIO driver
[   67.922976] libertas_sdio: Copyright Pierre Ossman
[   67.972073] libertas_sdio: probe of mmc1:0001:1 failed with error -16
bash-4.1# cat /sys/kernel/debug/mmc1/ios
clock:		400000 Hz
vdd:		21 (3.3 ~ 3.4 V)
bus mode:	1 (open drain)
chip select:	0 (don't care)
power mode:	2 (on)
bus width:	0 (1 bits)
timing spec:	0 (legacy)

> Please send a contiguous terminal session of these steps (incl. the
> boot messages), thanks !

Full log: http://dev.laptop.org/~dsd/20110609/sd-pwr-debug1.txt

Thanks
Daniel

Re: [PATCH] mmc: sdio: reset card during power_restore

[Ohad Ben-Cohen]

On Thu, Jun 9, 2011 at 6:51 PM, Daniel Drake <dsd@laptop.org> wrote:
> bash-4.1# mount -t debugfs none /sys/kernel/debug
> bash-4.1# cat /sys/kernel/debug/mmc1/ios
> clock:          0 Hz
> vdd:            0 (invalid)
> bus mode:       1 (open drain)
> chip select:    0 (don't care)
> power mode:     0 (off)
> bus width:      0 (1 bits)
> timing spec:    0 (legacy)
> bash-4.1# insmod /media/4E11-1D7C/libertas_sdio.ko
> [   67.917748] libertas_sdio: Libertas SDIO driver
> [   67.922976] libertas_sdio: Copyright Pierre Ossman
> [   67.972073] libertas_sdio: probe of mmc1:0001:1 failed with error -16
> bash-4.1# cat /sys/kernel/debug/mmc1/ios
> clock:          400000 Hz
> vdd:            21 (3.3 ~ 3.4 V)
> bus mode:       1 (open drain)
> chip select:    0 (don't care)
> power mode:     2 (on)
> bus width:      0 (1 bits)
> timing spec:    0 (legacy)

Ok, cool.

Now can you please repeat this, but this time add your original patch
(which only added the CMD5 arg=0 cmd, no sdio reset yet) ?

Thanks,
Ohad.

Re: [PATCH] mmc: sdio: reset card during power_restore

[Daniel Drake]

On 9 June 2011 16:59, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Now can you please repeat this, but this time add your original patch
> (which only added the CMD5 arg=0 cmd, no sdio reset yet) ?

With this version of the patch:
http://dev.laptop.org/~dsd/20110609/sd-pwr-debug2.patch

bash-4.1# mount -t debugfs none /sys/kernel/debug
bash-4.1# cat /sys/kernel/debug/mmc1/ios
clock:		0 Hz
vdd:		0 (invalid)
bus mode:	1 (open drain)
chip select:	0 (don't care)
power mode:	0 (off)
bus width:	0 (1 bits)
timing spec:	0 (legacy)
bash-4.1#
bash-4.1# insmod /media/4E11-1D7C/libertas_sdio.ko
[   53.917466] libertas_sdio: Libertas SDIO driver
[   53.922718] libertas_sdio: Copyright Pierre Ossman
[   54.839032] libertas_sdio mmc1:0001:1: (unregistered net_device):
00:17:c4:a7:50:57, fw 9.70.3p36, cap 0x000003a3
[   54.855479] libertas_sdio mmc1:0001:1: wlan0: Marvell WLAN 802.11 adapter
bash-4.1# [   54.941099] udev[985]: renamed network interface wlan0 to eth0
[   54.997656] ieee80211 phy0: assoc: bss   (null) not in scan results
[   56.310846] ieee80211 phy0: assoc: bss   (null) not in scan results
[   56.360840] cfg80211: Calling CRDA for country: EU

bash-4.1#
bash-4.1# cat /sys/kernel/debug/mmc1/ios
clock:		25000000 Hz
vdd:		20 (3.2 ~ 3.3 V)
bus mode:	2 (push-pull)
chip select:	0 (don't care)
power mode:	2 (on)
bus width:	2 (4 bits)
timing spec:	0 (legacy)

Full log: http://dev.laptop.org/~dsd/20110609/sd-pwr-debug2.txt

Note that the patch includes the mmc_select_voltage() call that I was
originally unsure about, and later did decide that equivalent code was
already being called elsewhere, thats why it was removed from the most
recent revision of the patch.



With a version of the patch that just does the reset, the post-powerup
"vdd" figure does change:
http://dev.laptop.org/~dsd/20110609/sd-pwr-debug3.patch

bash-4.1# cat /sys/kernel/debug/mmc1/ios
clock:		0 Hz
vdd:		0 (invalid)
bus mode:	1 (open drain)
chip select:	0 (don't care)
power mode:	0 (off)
bus width:	0 (1 bits)
timing spec:	0 (legacy)
bash-4.1#
bash-4.1# insmod /media/4E11-1D7C/libertas_sdio.ko
[   64.947624] libertas_sdio: Libertas SDIO driver
[   64.952866] libertas_sdio: Copyright Pierre Ossman
[   65.878171] libertas_sdio mmc1:0001:1: (unregistered net_device):
00:17:c4:a7:50:57, fw 9.70.3p36, cap 0x000003a3
[   65.896642] libertas_sdio mmc1:0001:1: wlan0: Marvell WLAN 802.11 adapter
bash-4.1# [   65.991205] udev[984]: renamed network interface wlan0 to eth0
[   66.048887] ieee80211 phy0: assoc: bss   (null) not in scan results
[   67.373572] ieee80211 phy0: assoc: bss   (null) not in scan results
[   67.423422] cfg80211: Calling CRDA for country: EU

bash-4.1# cat /sys/kernel/debug/mmc1/ios
clock:		25000000 Hz
vdd:		21 (3.3 ~ 3.4 V)
bus mode:	2 (push-pull)
chip select:	0 (don't care)
power mode:	2 (on)
bus width:	2 (4 bits)
timing spec:	0 (legacy)

Full log: http://dev.laptop.org/~dsd/20110609/sd-pwr-debug3.txt



For reference, here is the equivalent test performed without runtime
PM enabled (i.e. all changes reverted)

bash-4.1# mount -t debugfs none /sys/kernel/debug
bash-4.1# cat /sys/kernel/debug/mmc1/ios
clock:		25000000 Hz
vdd:		20 (3.2 ~ 3.3 V)
bus mode:	2 (push-pull)
chip select:	0 (don't care)
power mode:	2 (on)
bus width:	2 (4 bits)
timing spec:	0 (legacy)
bash-4.1#
bash-4.1# insmod /media/4E11-1D7C/libertas_sdio.ko
[   69.067761] libertas_sdio: Libertas SDIO driver
[   69.072995] libertas_sdio: Copyright Pierre Ossman
[   69.919033] libertas_sdio mmc1:0001:1: (unregistered net_device):
00:17:c4:a7:50:57, fw 9.70.3p36, cap 0x000003a3
[   69.935429] libertas_sdio mmc1:0001:1: wlan0: Marvell WLAN 802.11 adapter
[   70.021111] udev[991]: renamed network interface wlan0 to eth0
[   70.075870] ieee80211 phy0: assoc: bss   (null) not in scan results
[   71.460776] ieee80211 phy0: assoc: bss   (null) not in scan results
[   71.509660] cfg80211: Calling CRDA for country: EU
bash-4.1# cat /sys/kernel/debug/mmc1/ios
clock:		25000000 Hz
vdd:		20 (3.2 ~ 3.3 V)
bus mode:	2 (push-pull)
chip select:	0 (don't care)
power mode:	2 (on)
bus width:	2 (4 bits)
timing spec:	0 (legacy)

Full log: http://dev.laptop.org/~dsd/20110609/sd-pwr-debug0.txt


What next?

Thanks,
Daniel

Re: [PATCH] mmc: sdio: reset card during power_restore

[Ohad Ben-Cohen]

On Thu, Jun 9, 2011 at 7:21 PM, Daniel Drake <dsd@laptop.org> wrote:
> With this version of the patch:
> http://dev.laptop.org/~dsd/20110609/sd-pwr-debug2.patch
>
> bash-4.1# mount -t debugfs none /sys/kernel/debug
> bash-4.1# cat /sys/kernel/debug/mmc1/ios
> clock:          0 Hz
> vdd:            0 (invalid)
> bus mode:       1 (open drain)
> chip select:    0 (don't care)
> power mode:     0 (off)
> bus width:      0 (1 bits)
> timing spec:    0 (legacy)
> bash-4.1#
> bash-4.1# insmod /media/4E11-1D7C/libertas_sdio.ko
> [   53.917466] libertas_sdio: Libertas SDIO driver
> [   53.922718] libertas_sdio: Copyright Pierre Ossman
> [   54.839032] libertas_sdio mmc1:0001:1: (unregistered net_device):
> 00:17:c4:a7:50:57, fw 9.70.3p36, cap 0x000003a3
> [   54.855479] libertas_sdio mmc1:0001:1: wlan0: Marvell WLAN 802.11 adapter
> bash-4.1# [   54.941099] udev[985]: renamed network interface wlan0 to eth0
> [   54.997656] ieee80211 phy0: assoc: bss   (null) not in scan results
> [   56.310846] ieee80211 phy0: assoc: bss   (null) not in scan results
> [   56.360840] cfg80211: Calling CRDA for country: EU

Looks good.

At this point everything works ?
(can you bring up the interface and scan/connect)

Can you now do a series of insmod-rmmod-insmod.. and see if things
always work (with no runtime pm errors) after you insmod ?

If yes, we're good.

> Note that the patch includes the mmc_select_voltage() call

Good, keep that one please.

> With a version of the patch that just does the reset, the post-powerup
> "vdd" figure does change:
> http://dev.laptop.org/~dsd/20110609/sd-pwr-debug3.patch
...
> bash-4.1# cat /sys/kernel/debug/mmc1/ios
> clock:          25000000 Hz
> vdd:            21 (3.3 ~ 3.4 V)

Without even looking exactly why it happens, it doesn't look too good.
I don't see a reason to stick to that version. Let's use the
mmc_select_voltage.

>
> For reference, here is the equivalent test performed without runtime
> PM enabled (i.e. all changes reverted)

you mean runtime PM disabled, right ?

Thanks,
Ohad.

Re: [PATCH] mmc: sdio: reset card during power_restore

[Daniel Drake]

On 9 June 2011 17:30, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Looks good.
>
> At this point everything works ?
> (can you bring up the interface and scan/connect)

Yes.

> Can you now do a series of insmod-rmmod-insmod.. and see if things
> always work (with no runtime pm errors) after you insmod ?
>
> If yes, we're good.

See below.

>> Note that the patch includes the mmc_select_voltage() call
>
> Good, keep that one please.

OK

>> For reference, here is the equivalent test performed without runtime
>> PM enabled (i.e. all changes reverted)
>
> you mean runtime PM disabled, right ?

Yes.


Latest test, as suggested, based on this patch:
http://dev.laptop.org/~dsd/20110609/sd-pwr-debug4.patch
Note that I added in printk's to show when mmc_power_save_host() and
mmc_power_restore_host() get called.

bash-4.1# mount -t debugfs none /sys/kernel/debug
bash-4.1# cat /sys/kernel/debug/mmc1/ios
clock:		0 Hz
vdd:		0 (invalid)
bus mode:	1 (open drain)
chip select:	0 (don't care)
power mode:	0 (off)
bus width:	0 (1 bits)
timing spec:	0 (legacy)
bash-4.1#
bash-4.1# insmod /media/4E11-1D7C/libertas_sdio.ko
[   82.537594] libertas_sdio: Libertas SDIO driver
[   82.542838] libertas_sdio: Copyright Pierre Ossman
[   82.547792] mmc_power_restore_host mmc1
[   83.478197] libertas_sdio mmc1:0001:1: (unregistered net_device):
00:17:c4:a7:50:57, fw 9.70.3p36, cap 0x000003a3
[   83.496623] libertas_sdio mmc1:0001:1: wlan0: Marvell WLAN 802.11 adapter
bash-4.1# [   83.581679] udev[987]: renamed network interface wlan0 to eth0
[   83.635749] ieee80211 phy0: assoc: bss   (null) not in scan results
[   85.032955] ieee80211 phy0: assoc: bss   (null) not in scan results
[   85.087132] cfg80211: Calling CRDA for country: EU

bash-4.1# cat /sys/kernel/debug/mmc1/ios
clock:		25000000 Hz
vdd:		20 (3.2 ~ 3.3 V)
bus mode:	2 (push-pull)
chip select:	0 (don't care)
power mode:	2 (on)
bus width:	2 (4 bits)
timing spec:	0 (legacy)
bash-4.1#
bash-4.1# rmmod libertas_sdio
[   92.782496] cfg80211: Calling CRDA to update world regulatory domain
[   92.805661] cfg80211: World regulatory domain updated:
[   92.825864] cfg80211:     (start_freq - end_freq @ bandwidth),
(max_antenna_gain, max_eirp)
[   92.837401] cfg80211:     (2402000 KHz - 2472000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[   92.845708] cfg80211:     (2457000 KHz - 2482000 KHz @ 20000 KHz),
(300 mBi, 2000 mBm)
[   92.854000] cfg80211:     (2474000 KHz - 2494000 KHz @ 20000 KHz),
(300 mBi, 2000 mBm)
[   92.865497] cfg80211:     (5170000 KHz - 5250000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[   92.877042] cfg80211:     (5735000 KHz - 5835000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[   92.885562] cfg80211: Calling CRDA to update world regulatory domain
[   92.905832] cfg80211: World regulatory domain updated:
[   92.916165] cfg80211:     (start_freq - end_freq @ bandwidth),
(max_antenna_gain, max_eirp)
[   92.928171] cfg80211:     (2402000 KHz - 2472000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[   92.939651] cfg80211:     (2457000 KHz - 2482000 KHz @ 20000 KHz),
(300 mBi, 2000 mBm)
[   92.950209] cfg80211:     (2474000 KHz - 2494000 KHz @ 20000 KHz),
(300 mBi, 2000 mBm)
[   92.958179] cfg80211:     (5170000 KHz - 5250000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[   92.971593] cfg80211:     (5735000 KHz - 5835000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
bash-4.1#
bash-4.1# insmod /media/4E11-1D7C/libertas_sdio.ko
[  104.187988] libertas_sdio: Libertas SDIO driver
[  104.192840] libertas_sdio: Copyright Pierre Ossman
[  104.201237] libertas_sdio mmc1:0001:1: (unregistered net_device):
00:17:c4:a7:50:57, fw 9.70.3p36, cap 0x000003a3
[  104.230342] cfg80211: Calling CRDA for country: EU
[  104.238936] libertas_sdio mmc1:0001:1: wlan0: Marvell WLAN 802.11 adapter
[  104.320479] udev[2114]: renamed network interface wlan0 to eth0
[  104.366506] ieee80211 phy1: assoc: bss   (null) not in scan results
[  105.703988] ieee80211 phy1: assoc: bss   (null) not in scan results
[  105.746816] cfg80211: Calling CRDA to update world regulatory domain
[  105.780078] cfg80211: World regulatory domain updated:
[  105.800084] cfg80211:     (start_freq - end_freq @ bandwidth),
(max_antenna_gain, max_eirp)
[  105.808555] cfg80211:     (2402000 KHz - 2472000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[  105.850076] cfg80211:     (2457000 KHz - 2482000 KHz @ 20000 KHz),
(300 mBi, 2000 mBm)
[  105.871870] cfg80211:     (2474000 KHz - 2494000 KHz @ 20000 KHz),
(300 mBi, 2000 mBm)
[  105.879887] cfg80211:     (5170000 KHz - 5250000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[  105.900256] cfg80211:     (5735000 KHz - 5835000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)

bash-4.1# rmmod libertas_sdio
[  109.652081] cfg80211: Calling CRDA to update world regulatory domain
[  109.673075] cfg80211: World regulatory domain updated:
[  109.678279] cfg80211:     (start_freq - end_freq @ bandwidth),
(max_antenna_gain, max_eirp)
[  109.692581] cfg80211:     (2402000 KHz - 2472000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[  109.713624] cfg80211:     (2457000 KHz - 2482000 KHz @ 20000 KHz),
(300 mBi, 2000 mBm)
[  109.724811] cfg80211:     (2474000 KHz - 2494000 KHz @ 20000 KHz),
(300 mBi, 2000 mBm)
[  109.733089] cfg80211:     (5170000 KHz - 5250000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[  109.741521] cfg80211:     (5735000 KHz - 5835000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)

bash-4.1# insmod /media/4E11-1D7C/libertas_sdio.ko
[  112.378005] libertas_sdio: Libertas SDIO driver
[  112.382826] libertas_sdio: Copyright Pierre Ossman
[  112.391190] libertas_sdio mmc1:0001:1: (unregistered net_device):
00:17:c4:a7:50:57, fw 9.70.3p36, cap 0x000003a3
[  112.420334] cfg80211: Calling CRDA for country: EU
[  112.428883] libertas_sdio mmc1:0001:1: wlan0: Marvell WLAN 802.11 adapter
bash-4.1# [  112.510471] udev[2302]: renamed network interface wlan0 to eth0
[  112.556438] ieee80211 phy2: assoc: bss   (null) not in scan results
[  113.915702] ieee80211 phy2: assoc: bss   (null) not in scan results
[  113.955631] cfg80211: Calling CRDA to update world regulatory domain
[  113.993444] cfg80211: World regulatory domain updated:
[  113.998631] cfg80211:     (start_freq - end_freq @ bandwidth),
(max_antenna_gain, max_eirp)
[  114.025508] cfg80211:     (2402000 KHz - 2472000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[  114.052885] cfg80211:     (2457000 KHz - 2482000 KHz @ 20000 KHz),
(300 mBi, 2000 mBm)
[  114.070131] cfg80211:     (2474000 KHz - 2494000 KHz @ 20000 KHz),
(300 mBi, 2000 mBm)
[  114.078125] cfg80211:     (5170000 KHz - 5250000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[  114.102197] cfg80211:     (5735000 KHz - 5835000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)

bash-4.1# rmmod libertas_sdio
[  118.272469] cfg80211: Calling CRDA to update world regulatory domain
[  118.296325] cfg80211: World regulatory domain updated:
[  118.315555] cfg80211:     (start_freq - end_freq @ bandwidth),
(max_antenna_gain, max_eirp)
[  118.324822] cfg80211:     (2402000 KHz - 2472000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[  118.333074] cfg80211:     (2457000 KHz - 2482000 KHz @ 20000 KHz),
(300 mBi, 2000 mBm)
[  118.341331] cfg80211:     (2474000 KHz - 2494000 KHz @ 20000 KHz),
(300 mBi, 2000 mBm)
[  118.349327] cfg80211:     (5170000 KHz - 5250000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[  118.360932] cfg80211:     (5735000 KHz - 5835000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)

bash-4.1# insmod /media/4E11-1D7C/libertas_sdio.ko
[  120.557985] libertas_sdio: Libertas SDIO driver
[  120.562817] libertas_sdio: Copyright Pierre Ossman
[  120.572314] libertas_sdio mmc1:0001:1: (unregistered net_device):
00:17:c4:a7:50:57, fw 9.70.3p36, cap 0x000003a3
[  120.595998] cfg80211: Calling CRDA for country: EU
[  120.605524] libertas_sdio mmc1:0001:1: wlan0: Marvell WLAN 802.11 adapter
[  120.680466] udev[2474]: renamed network interface wlan0 to eth0
[  120.750792] ieee80211 phy3: assoc: bss   (null) not in scan results
[  121.966221] ieee80211 phy3: assoc: bss   (null) not in scan results
[  122.006462] cfg80211: Calling CRDA to update world regulatory domain
[  122.042731] cfg80211: World regulatory domain updated:
[  122.047938] cfg80211:     (start_freq - end_freq @ bandwidth),
(max_antenna_gain, max_eirp)
[  122.081694] cfg80211:     (2402000 KHz - 2472000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[  122.089714] cfg80211:     (2457000 KHz - 2482000 KHz @ 20000 KHz),
(300 mBi, 2000 mBm)
[  122.116718] cfg80211:     (2474000 KHz - 2494000 KHz @ 20000 KHz),
(300 mBi, 2000 mBm)
[  122.130095] cfg80211:     (5170000 KHz - 5250000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[  122.138125] cfg80211:     (5735000 KHz - 5835000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)

bash-4.1# cat /sys/kernel/debug/mmc1/ios
clock:		25000000 Hz
vdd:		20 (3.2 ~ 3.3 V)
bus mode:	2 (push-pull)
chip select:	0 (don't care)
power mode:	2 (on)
bus width:	2 (4 bits)
timing spec:	0 (legacy)




Full log: http://dev.laptop.org/~dsd/20110609/sd-pwr-debug4.txt

Note that during the insmod/rmmod calls, runtime PM did not touch the
power state - the card remained powered ever since the first insmod.
Not sure if this is in-line with your expectations.

Thanks!
Daniel

Re: [PATCH] mmc: sdio: reset card during power_restore

[Ohad Ben-Cohen]

On Thu, Jun 9, 2011 at 7:44 PM, Daniel Drake <dsd@laptop.org> wrote:
> Note that during the insmod/rmmod calls, runtime PM did not touch the
> power state - the card remained powered ever since the first insmod.
> Not sure if this is in-line with your expectations.

It definitely isn't. And that may very well be our culprit here.

Let's focus now on just this one sequence:

- boot
- cat /sys/.../ios (power should be off)
- insmod
- cat /sys/.../ios (power should be on)
- rmmod
- cat /sys/.../ios (power should be off again)

You will see the problem now.

And I think I know why - there was a driver core change that
indirectly caused this.

Please try to revert e1866b33b1e89f077b7132daae3dfd9a594e9a1a "PM /
Runtime: Rework runtime PM handling during driver removal" and tell me
if it helped.

If it did, un-revert that change, and try this one patch, which adopts
SDIO runtime PM to that change:

diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
index d29b9c3..d2565df 100644
--- a/drivers/mmc/core/sdio_bus.c
+++ b/drivers/mmc/core/sdio_bus.c
@@ -189,7 +189,7 @@ static int sdio_bus_remove(struct device *dev)

        /* Then undo the runtime PM settings in sdio_bus_probe() */
        if (func->card->host->caps & MMC_CAP_POWER_OFF_CARD)
-               pm_runtime_put_noidle(dev);
+               pm_runtime_put_sync(dev);

 out:
        return ret;

Thanks,
Ohad.

Re: [PATCH] mmc: sdio: reset card during power_restore

[Daniel Drake]

On 9 June 2011 18:27, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Let's focus now on just this one sequence:
>
> - boot
> - cat /sys/.../ios (power should be off)
> - insmod
> - cat /sys/.../ios (power should be on)
> - rmmod
> - cat /sys/.../ios (power should be off again)
>
> You will see the problem now.

As I think you expected, everything went fine until the last "cat"
where it showed that the power is still on (no change from the
previous cat).

> And I think I know why - there was a driver core change that
> indirectly caused this.
>
> Please try to revert e1866b33b1e89f077b7132daae3dfd9a594e9a1a "PM /
> Runtime: Rework runtime PM handling during driver removal" and tell me
> if it helped.

It does. Now your test case works, power gets turned off on rmmod.

> If it did, un-revert that change, and try this one patch, which adopts
> SDIO runtime PM to that change:
>
> diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
> index d29b9c3..d2565df 100644
> --- a/drivers/mmc/core/sdio_bus.c
> +++ b/drivers/mmc/core/sdio_bus.c
> @@ -189,7 +189,7 @@ static int sdio_bus_remove(struct device *dev)
>
>        /* Then undo the runtime PM settings in sdio_bus_probe() */
>        if (func->card->host->caps & MMC_CAP_POWER_OFF_CARD)
> -               pm_runtime_put_noidle(dev);
> +               pm_runtime_put_sync(dev);
>
>  out:
>        return ret;

Done, the rmmod issue remains fixed - the card gets powered down during rmmod.

rmmod/insmod repeatedly works as expected. It gets powered down on
rmmod, and powered up on insmod. The power-up works fine and ios
values look fine at all steps.

Progress! :)



Next up is the problem where on this setup, if I suspend after loading
the module, and the module asks the card to shut down during suspend,
it fails to get brought up during resume.
Note that I have now rolled in a patch you made in another thread to
correctly call into the driver's remove routine during suspend when
powering down the card.
I also added a printk message to detect when the CMD5 reset command failed.

New patch: http://dev.laptop.org/~dsd/20110609/sd-pwr-debug5.patch

Test case:

- boot
- mount debugfs
- insmod
- suspend
- resume
- cat /sys/.../ios

Output from resume onwards:

[   60.210326] Restarting tasks ... done.
[   60.346261] cfg80211: World regulatory domain updated:
[   60.368829] cfg80211:     (start_freq - end_freq @ bandwidth),
(max_antenna_gain, max_eirp)
[   60.394947] cfg80211:     (2402000 KHz - 2472000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[   60.425084] cfg80211:     (2457000 KHz - 2482000 KHz @ 20000 KHz),
(300 mBi, 2000 mBm)
[   60.433390] usb 1-2: USB disconnect, device number 2
[   60.475817] cfg80211:     (2474000 KHz - 2494000 KHz @ 20000 KHz),
(300 mBi, 2000 mBm)
[   60.587866] cfg80211:     (5170000 KHz - 5250000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[   60.611597] cfg80211:     (5735000 KHz - 5835000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[   60.684817] cfg80211: Calling CRDA to update world regulatory domain
[   60.736397] cfg80211: World regulatory domain updated:
[   60.784745] cfg80211:     (start_freq - end_freq @ bandwidth),
(max_antenna_gain, max_eirp)
[   60.801158] usb 1-2: new high speed USB device number 3 using ehci_hcd
[   60.815608] cfg80211:     (2402000 KHz - 2472000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[   60.841586] cfg80211:     (2457000 KHz - 2482000 KHz @ 20000 KHz),
(300 mBi, 2000 mBm)
[   60.883868] cfg80211:     (2474000 KHz - 2494000 KHz @ 20000 KHz),
(300 mBi, 2000 mBm)
[   60.893420] mmc1: new SDIO card at address 0001
[   60.913405] mmc_power_save_host mmc1
[   60.917148] mmc_power_restore_host mmc1
[   60.931707] cfg80211:     (5170000 KHz - 5250000 KHz @ 40000 KHz),
(300 mBi, 2000 mBm)
[   60.983389] usb 1-2: New USB device found, idVendor=090c, idProduct=1000
[   61.011508] CMD5 reset failed, err=-110
[   61.015475] libertas_sdio: probe of mmc1:0001:1 failed with error -16

Full log: http://dev.laptop.org/~dsd/20110609/sd-pwr-debug5.txt

Daniel

Re: [PATCH] mmc: sdio: reset card during power_restore

[Ohad Ben-Cohen]

On Thu, Jun 9, 2011 at 8:56 PM, Daniel Drake <dsd@laptop.org> wrote:
> Done, the rmmod issue remains fixed - the card gets powered down during rmmod.

great !

> Next up is the problem where on this setup, if I suspend after loading
> the module, and the module asks the card to shut down during suspend,
> it fails to get brought up during resume.

we need to take into account that driver core change I spotted.

> Note that I have now rolled in a patch you made in another thread to
> correctly call into the driver's remove routine during suspend when
> powering down the card.

let's update that patch. I'd send you an updated one, but I have to go
for awhile, so here's the quick change you need to do.

replace the put_noidle() call in the hunk below with a put_sync():

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 4d0c15b..e23888a 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -540,6 +540,13 @@ static void mmc_sdio_remove(struct mmc_host *host)
 	BUG_ON(!host);
 	BUG_ON(!host->card);

+	/*
+	 * if this card is managed by runtime pm, make sure it is powered on
+	 * before invoking its SDIO functions' ->remove() handler
+	 */
+	if (host->caps & MMC_CAP_POWER_OFF_CARD)
+		pm_runtime_get_sync(&host->card->dev);
+
 	for (i = 0;i < host->card->sdio_funcs;i++) {
 		if (host->card->sdio_func[i]) {
 			sdio_remove_func(host->card->sdio_func[i]);
@@ -547,6 +554,9 @@ static void mmc_sdio_remove(struct mmc_host *host)
 		}
 	}

+	if (host->caps & MMC_CAP_POWER_OFF_CARD)
+		pm_runtime_put_noidle(&host->card->dev);   //<==============  use
pm_runtime_put_sync here
+
 	mmc_remove_card(host->card);
 	host->card = NULL;
 }

tell me how it goes...

Re: [PATCH] mmc: sdio: reset card during power_restore

[Daniel Drake]

On 9 June 2011 19:25, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> let's update that patch. I'd send you an updated one, but I have to go
> for awhile, so here's the quick change you need to do.

Unfortunately it doesn't help.

New patch:
http://dev.laptop.org/~dsd/20110609/sd-pwr-debug6.patch

During resume, this happens:
[  152.156215] mmc1: new SDIO card at address 0001
[  152.198193] mmc_power_save_host mmc1
[  152.212397] mmc_power_restore_host mmc1
[  152.311567] CMD5 reset failed, err=-110
[  152.315513] libertas_sdio: probe of mmc1:0001:1 failed with error -16

bash-4.1# cat /sys/kernel/debug/mmc1/ios
clock:		400000 Hz
vdd:		21 (3.3 ~ 3.4 V)
bus mode:	1 (open drain)
chip select:	0 (don't care)
power mode:	2 (on)
bus width:	0 (1 bits)
timing spec:	0 (legacy)



(I had forgotten to include final ios output in my last test, sorry about that)

Full log: http://dev.laptop.org/~dsd/20110609/sd-pwr-debug6.txt

What next? Is it concerning that ios ends up running at the wrong
frequency and voltage?
Daniel

Re: [PATCH] mmc: sdio: reset card during power_restore

[Ohad Ben-Cohen]

On Thu, Jun 9, 2011 at 10:55 PM, Daniel Drake <dsd@laptop.org> wrote:
> On 9 June 2011 19:25, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> let's update that patch. I'd send you an updated one, but I have to go
>> for awhile, so here's the quick change you need to do.
>
> Unfortunately it doesn't help.

That's ok. Let's go back to small steps now.

We have cyclic rmmod/insmod working, and that's good: it means there's
no hw issue.

The next step to have working now is really the ifconfig up/down/up
scenario. That should come before suspend/resume. And this
step-by-step approach is even more relevant to libertas_sdio, because
I suspect there's some if_sdio.c refactoring needed to get it right,
but let's see.

The expected result you want to get is:

- boot, power is off
- insmod, power is still off
- ifconfig up, power is on
- ifconfig down or rmmod, power is back off

Re: [PATCH] mmc: sdio: reset card during power_restore

[Daniel Drake]

On 10 June 2011 00:27, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> The next step to have working now is really the ifconfig up/down/up
> scenario. That should come before suspend/resume. And this
> step-by-step approach is even more relevant to libertas_sdio, because
> I suspect there's some if_sdio.c refactoring needed to get it right,
> but let's see.

Done. Without bringing suspend/resume into the picture, this is
working fine. Power is removed when the interface goes down, and is
restored when the interface is brought up. ios values look fine at
every stage. Wireless card works fine after several power cycles.

Patches here: http://dev.laptop.org/~dsd/20110610/
The most relevant ones for this discussion are 6 and 7.

What next?

Daniel

Re: [PATCH] mmc: sdio: reset card during power_restore

[Ohad Ben-Cohen]

On Fri, Jun 10, 2011 at 7:15 PM, Daniel Drake <dsd@laptop.org> wrote:
> Done. Without bringing suspend/resume into the picture, this is
> working fine. Power is removed when the interface goes down, and is
> restored when the interface is brought up. ios values look fine at
> every stage. Wireless card works fine after several power cycles.

That sounds great.

> Patches here: http://dev.laptop.org/~dsd/20110610/
> The most relevant ones for this discussion are 6 and 7.
>
> What next?

We need to debug the suspend/resume path. Now that we have the other
runtime pm paths working, we can pretty much tell there's no hw issue
at hand.

I definitely plan to help you nail this, but I'm a little tied up with
some tight schedule for a little while.

Re: [PATCH] mmc: sdio: reset card during power_restore

[Daniel Drake]

On 13 June 2011 20:52, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> We need to debug the suspend/resume path. Now that we have the other
> runtime pm paths working, we can pretty much tell there's no hw issue
> at hand.

Found it. It's a timing issue. In our other tests we had not yet hit
the case when power is removed then immediately restored.  However,
during resume, the card is powered up, powered down, powered up, and
then probed by libertas.

A msleep(250) is needed during power_restore. Then the reset works fine.

Why is there so much power flipping going on during resume? Is this a
bug? Shouldn't it power it up, realise it has a driver already loaded,
and go straight into probe?

But even if that gets fixed, we still need to fix the case where the
network interface is brought down then up immediately (another way to
trigger the issue). Would you suggest a card quirk for that? Adding
another 250ms to the already-slow libertas powerup routine would be a
bit painful, would you support the added complexity needed to make the
250ms delay only occur when >250ms has passed since it was powered
off?

Thanks,
Daniel

Re: [PATCH] mmc: sdio: reset card during power_restore

[Philip Rakity]

Seeing something similar but the details are a little different.  My kernel is somewhat old but ...
Seeing this with sd card and android.
card is removed/inserted/removed/inserted etc very quickly during power operations.  After 20-30 times
a problem occurs.  this can be anything from file system errors, partition not recognized or a kernel hang.

My thought is this is not a quirk issue but power down needs to be delayed during the power up
sequence until processing has been completed in the mmc layer.  Any thoughts on how to 
debug this would be welcome.  When the hang happens the system is not dead (as in 
console output showing card insert/remove still happens but console is locked etc.

thanks,

Philip

I do not think a quirk is the answer.  
On Jun 16, 2011, at 10:27 AM, Daniel Drake wrote:

> On 13 June 2011 20:52, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> We need to debug the suspend/resume path. Now that we have the other
>> runtime pm paths working, we can pretty much tell there's no hw issue
>> at hand.
> 
> Found it. It's a timing issue. In our other tests we had not yet hit
> the case when power is removed then immediately restored.  However,
> during resume, the card is powered up, powered down, powered up, and
> then probed by libertas.
> 
> A msleep(250) is needed during power_restore. Then the reset works fine.
> 
> Why is there so much power flipping going on during resume? Is this a
> bug? Shouldn't it power it up, realise it has a driver already loaded,
> and go straight into probe?
> 
> But even if that gets fixed, we still need to fix the case where the
> network interface is brought down then up immediately (another way to
> trigger the issue). Would you suggest a card quirk for that? Adding
> another 250ms to the already-slow libertas powerup routine would be a
> bit painful, would you support the added complexity needed to make the
> 250ms delay only occur when >250ms has passed since it was powered
> off?
> 
> Thanks,
> Daniel
> --
> 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

Re: [PATCH] mmc: sdio: reset card during power_restore

[Ohad Ben-Cohen]

On Thu, Jun 16, 2011 at 8:27 PM, Daniel Drake <dsd@laptop.org> wrote:
> Why is there so much power flipping going on during resume?

There isn't.

You see this with libertas, because it doesn't really suspend/resume cleanly.

Instead, it uses a pretty awkward (in my opinion) mechanism to remove
the card on suspend, and then let the system re-probe it on resume.

> But even if that gets fixed, we still need to fix the case where the
> network interface is brought down then up immediately (another way to
> trigger the issue).

Could you also trigger this with the previous power off/on mechanism
you used (an rfkill driver IIRC) ? if not, what's the difference ? did
you have just enough delay there which didn't trigger this problem ?

> Would you suggest a card quirk for that? Adding
> another 250ms to the already-slow libertas powerup routine would be a
> bit painful, would you support the added complexity needed to make the
> 250ms delay only occur when >250ms has passed since it was powered
> off?

First, I suggest to understand where does this requirement come from.
Is this a rigid hardware requirement ? Does it always require waiting
250ms before powering it on again ?

If yes, then why not let the relevant regulator take care of it ? this
way you never have to care. you just power it off and on as you
please, and things just work.

Re: [PATCH] mmc: sdio: reset card during power_restore

[Daniel Drake]

On 16 June 2011 22:22, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Thu, Jun 16, 2011 at 8:27 PM, Daniel Drake <dsd@laptop.org> wrote:
>> Why is there so much power flipping going on during resume?
>
> There isn't.
>
> You see this with libertas, because it doesn't really suspend/resume cleanly.

It's not related to suspend/resume. If I build libertas_sdio into the
kernel I see the same thing happening during boot:

http://dev.laptop.org/~dsd/20110617/dmesg.txt

[    3.023445] mmc1: new SDIO card at address 0001
[    3.039597] mmc_power_save_host mmc1
[    3.054897] mmc_power_restore_host mmc1
[    3.409482] CMD5 reset failed, err=-110
[    3.427733] libertas_sdio: probe of mmc1:0001:1 failed with error -16

Ignore the probe error - just observe the fact that the card was
detected and powered on, powered off, powered on and *then* probed.
That's the same thing that happens in the resume path. Couldn't the
power have just been maintained from when the card was detected, given
that the driver was loaded and ready to go?

> Could you also trigger this with the previous power off/on mechanism
> you used (an rfkill driver IIRC) ? if not, what's the difference ? did
> you have just enough delay there which didn't trigger this problem ?

No, it couldn't be triggered. This is because our rfkill driver used
the "normal" MMC card powerup routine (which runtime PM doesn't use)
which unconditionally calls sdio_reset().

Thinking more, I think I prefer sdio_reset() as a quirk/workaround
instead of the sleep, because the sleep is quite long. What do you
think?

> First, I suggest to understand where does this requirement come from.
> Is this a rigid hardware requirement ? Does it always require waiting
> 250ms before powering it on again ?

Any suggestions for how I go about doing that? Unfortunately we have a
backlog of support requests with Marvell which have not yet been
responded to.

> If yes, then why not let the relevant regulator take care of it ? this
> way you never have to care. you just power it off and on as you
> please, and things just work.

Yes, a regulator could work. But, aren't regulators linked to the
platform rather than to the card? I would imagine that this quirk
needs to be applied to all users of the card, therefore a card quirk
could be more appropriate? Also, if you think the sdio_reset() option
is preferable to the sleep, can that be done from a regulator?

Thanks
Daniel

Re: [PATCH] mmc: sdio: reset card during power_restore

[Ohad Ben-Cohen]

On Fri, Jun 17, 2011 at 4:58 PM, Daniel Drake <dsd@laptop.org> wrote:
> Ignore the probe error - just observe the fact that the card was
> detected and powered on, powered off, powered on and *then* probed.

This is the normal behavior today when drivers are probed: we power up
the card before probing a driver, and if the probe fails, we power it
off.

To be exact, we're not even doing that deterministically. We are
powering off the *SDIO function*, and not the card. I once changed
this to be deterministic, but didn't pursue this eventually, because
frankly it's not hugely important.

> That's the same thing that happens in the resume path.

You mean on *libertas'* resume path, which doesn't really do
suspend/resume - it just removes the card, and let it be re-probed
later.

> Couldn't the
> power have just been maintained from when the card was detected, given
> that the driver was loaded and ready to go?

Most cards wouldn't care too much: we're not loading/unloading drivers
too frequently, and the advantage of the current solution is
simplicity.

But you are welcome to change this if you need to. I don't see how it
can hurt anyone, and anyway it can also happen today too in some
cases.

> Thinking more, I think I prefer sdio_reset() as a quirk/workaround
> instead of the sleep, because the sleep is quite long. What do you
> think?

I agree that a 250ms delay on the power up path is way too long.

But let's first try to understand what's the real problem it is
solving before picking a solution. This usually pays off, and
eventually you'd be happier with the end result.

> Any suggestions for how I go about doing that? Unfortunately we have a
> backlog of support requests with Marvell which have not yet been
> responded to.

Use the mailing list :)

The Marvell guys were very nice and replied my questions pretty fast.

> Yes, a regulator could work. But, aren't regulators linked to the
> platform rather than to the card? I would imagine that this quirk
> needs to be applied to all users of the card, therefore a card quirk
> could be more appropriate? Also, if you think the sdio_reset() option
> is preferable to the sleep, can that be done from a regulator?

I'd really prefer to understand what's wrong first.

If we have a rigid hardware requirement that demands waiting before
powering it up again, then we better obey it. But I somehow doubt that
a 250ms delay is required. That's way too much...

Thanks,
Ohad.

Re: [PATCH] mmc: sdio: reset card during power_restore

[Daniel Drake]

On 17 June 2011 15:31, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Fri, Jun 17, 2011 at 4:58 PM, Daniel Drake <dsd@laptop.org> wrote:
>> Ignore the probe error - just observe the fact that the card was
>> detected and powered on, powered off, powered on and *then* probed.
>
> This is the normal behavior today when drivers are probed: we power up
> the card before probing a driver, and if the probe fails, we power it
> off.

The power off happens before the libertas-level probe starts. Is that
still in line with your expectations?

[    3.023445] mmc1: new SDIO card at address 0001
[    3.039597] mmc_power_save_host mmc1
[    3.054897] mmc_power_restore_host mmc1
<now libertas probe starts>

> Most cards wouldn't care too much: we're not loading/unloading drivers
> too frequently, and the advantage of the current solution is
> simplicity.
>
> But you are welcome to change this if you need to. I don't see how it
> can hurt anyone, and anyway it can also happen today too in some
> cases.

OK - if you are aware of it and expect it then I'm fine with it.
Likewise I'm trying to maximise our understanding on these issues.

I'll start digging on that 250ms delay thing. Today I find that 250ms
is unreliable so I'm now looking at 300ms...

cheers
Daniel

Re: [PATCH] mmc: sdio: reset card during power_restore

[Daniel Drake]

Hi Ohad,

On 17 June 2011 15:31, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> Yes, a regulator could work. But, aren't regulators linked to the
>> platform rather than to the card? I would imagine that this quirk
>> needs to be applied to all users of the card, therefore a card quirk
>> could be more appropriate? Also, if you think the sdio_reset() option
>> is preferable to the sleep, can that be done from a regulator?
>
> I'd really prefer to understand what's wrong first.
>
> If we have a rigid hardware requirement that demands waiting before
> powering it up again, then we better obey it. But I somehow doubt that
> a 250ms delay is required. That's way too much...

So, as I found here: http://article.gmane.org/gmane.linux.kernel.mmc/8605
This is an OLPC-specific issue due to lack of power clamping on the
motherboard's power supply, which might not be shared by other sd8686
users, and a long delay is expected.

I have been testing further and I have seen a couple of times that
msleep(300) is not enough, or at least the card failed to do the cmd5
reset in those cases. The sdio_reset() method seems to be 100%
reliable however, I haven't seen a single failure with that yet.

Where do we go from here? Any suggested implementation approach?

Thanks,
Daniel

Re: [PATCH] mmc: sdio: reset card during power_restore

[Ohad Ben-Cohen]

Hi Daniel,

On Sun, Jun 19, 2011 at 1:33 PM, Daniel Drake <dsd@laptop.org> wrote:
> So, as I found here: http://article.gmane.org/gmane.linux.kernel.mmc/8605
> This is an OLPC-specific issue due to lack of power clamping on the
> motherboard's power supply, which might not be shared by other sd8686
> users, and a long delay is expected.
>
> I have been testing further and I have seen a couple of times that
> msleep(300) is not enough, or at least the card failed to do the cmd5
> reset in those cases. The sdio_reset() method seems to be 100%
> reliable however, I haven't seen a single failure with that yet.

Then sdio_reset() it is.

We're taking a risk of covering up bugs like the one we just found here:

http://www.mail-archive.com/linux-mmc@vger.kernel.org/msg08371.html

So people will be happy that everything (seem to) work, without even
knowing their card might not have been powered off at all.

But I still think it's generally better, given the alternatives.

And since this should not have any bad effect for anyone else, I'm
also OK with not wrapping the sdio_reset and the CMD5 arg=0 you're
adding with a quirk.

After that's in place, I plan to completely get rid of
MMC_CAP_POWER_OFF_CARD, since the only evidence we had when we added
it was the sd8686, and today we know it's not necessary at all.

Thanks,
Ohad.

Re: [PATCH] mmc: sdio: reset card during power_restore

[Daniel Drake]

Hi Ohad,

On 19 June 2011 12:00, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> But I still think it's generally better, given the alternatives.
>
> And since this should not have any bad effect for anyone else, I'm
> also OK with not wrapping the sdio_reset and the CMD5 arg=0 you're
> adding with a quirk.
>
> After that's in place, I plan to completely get rid of
> MMC_CAP_POWER_OFF_CARD, since the only evidence we had when we added
> it was the sd8686, and today we know it's not necessary at all.

Thanks - I've just submitted this patch.

Now we just need you to submit the remaining parts of
http://dev.laptop.org/~dsd/20110609/sd-pwr-debug6.patch - can you take
care of that? I'm not sure if/how it should be split up or how to
write the commit message.

Then we can get rid of MMC_CAP_POWER_OFF_CARD, which would be good to
do ASAP to maximize testing before Linux 3.1.

Thanks,
Daniel

Re: [PATCH] mmc: sdio: reset card during power_restore

[Ohad Ben-Cohen]

On Sat, Jun 25, 2011 at 9:23 PM, Daniel Drake <dsd@laptop.org> wrote:
> Now we just need you to submit the remaining parts of
> http://dev.laptop.org/~dsd/20110609/sd-pwr-debug6.patch - can you take
> care of that? I'm not sure if/how it should be split up or how to
> write the commit message.

Sure, i'll post it soon.

> Then we can get rid of MMC_CAP_POWER_OFF_CARD, which would be good to
> do ASAP to maximize testing before Linux 3.1.

I'll take care of that.

Thanks,
Ohad.

Re: [PATCH] mmc: sdio: reset card during power_restore

[zhangfei gao]

Hi, Ohad

One question :(

Under pm_runtime mechanism, how to dynamically open/close wlan.

If the wlan chip really power off, the firmware should be reloaded,
since the firmware is download to ram and disappear after power off.
So wlan probe function should be called for re-downloading, is it be
achieved by calling pm_runtime_get_sync and mmc_power_restore_host?

>From Daniel's test log, "echo mem > /sys/power/state" is used to
restart wlan, is this standard method?
Since host->bus_ops is already not NULL, mmc_rescan will return direclty.

Thanks

Re: [PATCH] mmc: sdio: reset card during power_restore

[Ohad Ben-Cohen]

Hi Zhangfei,

On Tue, Jun 28, 2011 at 12:13 PM, zhangfei gao <zhangfei.gao@gmail.com> wrote:
> One question :(

Np, feel free to ask !
(but it's usually better to start a new thread, rather then forking an
existing one under the same subject)

> Under pm_runtime mechanism, how to dynamically open/close wlan.

Actually it should be the other way around: when you enable/disable
WLAN (e.g. Ifconfig up/down), you then use the runtime PM API to power
up/down the device.

> If the wlan chip really power off, the firmware should be reloaded,
> since the firmware is download to ram and disappear after power off.

Right.

> So wlan probe function should be called for re-downloading, is it be
> achieved by calling pm_runtime_get_sync and mmc_power_restore_host?

Not really;

I assume you refer to libertas_sdio, which AFAICT, is built to power
on the device (and configure it) on ->probe(), and then power it off
on ->remove().
It then seems that this model was extended to support suspend/resume
by asking the MMC core to ->remove() the card on suspend, and then
relying on it to re-detect and re-add the card on resume, which will
trigger libertas' ->probe() again.

IMHO this model is a little awkward; as you can see, powering on/off
the chip requires re-probing the driver.

Take a look how mac80211 drivers (and wl12xx in particular) behave:
they are powered on (and firmware is downloaded) when the user brings
the wlan0 interface up, and then powered off when the wlan interface
is brought down.

The runtime PM API is only being used to control the power to the
device, but downloading the firmware and doing driver-specific
configuration is up to the driver to do.

> From Daniel's test log, "echo mem > /sys/power/state" is used to
> restart wlan, is this standard method?

That command was only used to trigger system suspend - Daniel was
testing libertas & runtime PM behavior in that scenario.

Thanks,
Ohad.

Re: [PATCH] mmc: sdio: reset card during power_restore

[zhangfei gao]

>> So wlan probe function should be called for re-downloading, is it be
>> achieved by calling pm_runtime_get_sync and mmc_power_restore_host?
>
> Not really;
>
> I assume you refer to libertas_sdio, which AFAICT, is built to power
> on the device (and configure it) on ->probe(), and then power it off
> on ->remove().
> It then seems that this model was extended to support suspend/resume
> by asking the MMC core to ->remove() the card on suspend, and then
> relying on it to re-detect and re-add the card on resume, which will
> trigger libertas' ->probe() again.
>
> IMHO this model is a little awkward; as you can see, powering on/off
> the chip requires re-probing the driver.
>
> Take a look how mac80211 drivers (and wl12xx in particular) behave:
> they are powered on (and firmware is downloaded) when the user brings
> the wlan0 interface up, and then powered off when the wlan interface
> is brought down.
>
> The runtime PM API is only being used to control the power to the
> device, but downloading the firmware and doing driver-specific
> configuration is up to the driver to do.

Thanks Ohad for your kind explanation.

However still not fully understand how to call ->remove to power off
wlan, using suspend system looks to me is only test method, which
counting on bus_ops->suspend returns -ENOSYS.

What we do before is
1. From user space use rfkill unblock wifi, or echo on/off > "/sys/~"
to enable/disable wlan power, which could be replaced by pm_runtime of
course.
2. Explicitly call mmc_detect_change,
when power off, mmc_rescan -> bus_ops->detect(host) -> mmc_select_card
fail -> mmc_sdio_remove;
when power on, mmc_rescan ->  mmc_attach_sdio->wlan probe.

However, this is not workable if using mmc_power_save_host, since
bus_ops->detect not workable after power off card.
1. detect is only for !MMC_CAP_NONREMOVABLE
2. In mmc_sdio_detect, power is provided before mmc_select_card via
pm_runtime_get_sync.
3. Most important, mmc_power_save_host->mmc_power_off->set clk=0 via
set_ios, so controller will no response and timeout.

How to enable/disable mac80211 at runtime from user space?
In wl12xx/debugfs.c, gpio_power_write->wl1271_sdio_power_off? still
unclear how remove is called.

Thanks

Re: [PATCH] mmc: sdio: reset card during power_restore

[Daniel Drake]

On 28 June 2011 10:13, zhangfei gao <zhangfei.gao@gmail.com> wrote:
> Hi, Ohad
>
> One question :(
>
> Under pm_runtime mechanism, how to dynamically open/close wlan.
>
> If the wlan chip really power off, the firmware should be reloaded,
> since the firmware is download to ram and disappear after power off.
> So wlan probe function should be called for re-downloading, is it be
> achieved by calling pm_runtime_get_sync and mmc_power_restore_host?

Here is how I'm doing it for libertas:
http://dev.laptop.org/~dsd/20110610/

See patches 6 and 7.

This is somewhat based off the wl1xxx example that Ohad gave.

Daniel

Re: [PATCH] mmc: sdio: reset card during power_restore

[Ohad Ben-Cohen]

On Wed, Jun 29, 2011 at 11:43 AM, zhangfei gao <zhangfei.gao@gmail.com> wrote:
> However still not fully understand how to call ->remove to power off
> wlan, using suspend system looks to me is only test method, which
> counting on bus_ops->suspend returns -ENOSYS.

Please take a look at mac80211 and wl12xx.

We're not using ->remove, ->probe or -ENOSYS at all.

When the user brings up the interface, we then power up the chip and
download the firmware.
Likewise, when the interface is brought down, we just power off the chip.

> 2. Explicitly call mmc_detect_change,
> when power off, mmc_rescan -> bus_ops->detect(host) -> mmc_select_card
> fail -> mmc_sdio_remove;
> when power on, mmc_rescan ->  mmc_attach_sdio->wlan probe.

This is awkward. Basically what you're saying is that your driver can
only power on the chip when it is probed.

IMHO you need to refactor your driver, so it will power on/off the
device according to interface status changes, without requiring the
system to remove and re-detect the device.

Re: [PATCH] mmc: sdio: reset card during power_restore

[zhangfei gao]

On Wed, Jun 29, 2011 at 4:57 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Wed, Jun 29, 2011 at 11:43 AM, zhangfei gao <zhangfei.gao@gmail.com> wrote:
>> However still not fully understand how to call ->remove to power off
>> wlan, using suspend system looks to me is only test method, which
>> counting on bus_ops->suspend returns -ENOSYS.
>
> Please take a look at mac80211 and wl12xx.
>
> We're not using ->remove, ->probe or -ENOSYS at all.
>
> When the user brings up the interface, we then power up the chip and
> download the firmware.
> Likewise, when the interface is brought down, we just power off the chip.

Thanks a lot Ohad & Daniel :)

Enable wlan: # ifconfig up mlan0 -> power up the chip via runtime PM
-> wlan_probe download the firmware
Disable wlan: # ifconfig down mlan0 -> power down the chip via runtime
PM -> wlan_remove ?

So every time ifconfig up/down, the chip is power up/down, and
firmware reloaded?

Is this understand correct?
Then runtime pm should also integrated into wlan driver as well.

Re: [PATCH] mmc: sdio: reset card during power_restore

[Ohad Ben-Cohen]

On Wed, Jun 29, 2011 at 12:19 PM, zhangfei gao <zhangfei.gao@gmail.com> wrote:
> Enable wlan: # ifconfig up mlan0 -> power up the chip via runtime PM
> -> wlan_probe download the firmware
> Disable wlan: # ifconfig down mlan0 -> power down the chip via runtime
> PM -> wlan_remove ?

Sounds all good, besides the part where you mention the "probe" and
"remove" names.

I'm not sure what you mean exactly, but generally, the driver's probe
and remove functions should be called only once during the lifetime of
the device it controls.

You may want to refactor the driver a bit, so you don't need those
handlers to be called whenever you power up/down the card.

> So every time ifconfig up/down, the chip is power up/down, and
> firmware reloaded?

Yes, this is one way to go (I know some other out-of-tree WLAN drivers
behave differently, but this is the way mac80211 works, and it's quite
nice).

Re: [PATCH] mmc: sdio: reset card during power_restore

[Chris Ball]

Hi,

On Sat, Jun 25 2011, Daniel Drake wrote:
> mmc_sdio_power_restore() skips some steps that are performed in other
> power-related codepaths which are necessary to fully reset the card.
> Without this, runtime PM fails for SD8686 SDIO wifi on OLPC XO-1.5.
>
> Signed-off-by: Daniel Drake <dsd@laptop.org>

Thanks, merged for 3.0-rc.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: sdio: reset card during power_restore

[Ohad Ben-Cohen]

On Sat, Jun 25, 2011 at 9:20 PM, Daniel Drake <dsd@laptop.org> wrote:
> mmc_sdio_power_restore() skips some steps that are performed in other
> power-related codepaths which are necessary to fully reset the card.
> Without this, runtime PM fails for SD8686 SDIO wifi on OLPC XO-1.5.
>
> Signed-off-by: Daniel Drake <dsd@laptop.org>

Thanks for your work, Daniel.

Acked-by: Ohad Ben-Cohen <ohad@wizery.com>

[PATCH] mmc: sdio: reset card during power_restore

[Daniel Drake]

mmc_sdio_power_restore() skips some steps that are performed in other
power-related codepaths which are necessary to fully reset the card.
Without this, runtime PM fails for SD8686 SDIO wifi on OLPC XO-1.5.

Signed-off-by: Daniel Drake <dsd@laptop.org>
---
 drivers/mmc/core/sdio.c |   39 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)

Replaces existing patch with the same name

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 4d0c15b..262fff0 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -691,15 +691,54 @@ static int mmc_sdio_resume(struct mmc_host *host)
 static int mmc_sdio_power_restore(struct mmc_host *host)
 {
 	int ret;
+	u32 ocr;
 
 	BUG_ON(!host);
 	BUG_ON(!host->card);
 
 	mmc_claim_host(host);
+
+	/*
+	 * Reset the card by performing the same steps that are taken by
+	 * mmc_rescan_try_freq() and mmc_attach_sdio() during a "normal" probe.
+	 *
+	 * sdio_reset() is technically not needed. Having just powered up the
+	 * hardware, it should already be in reset state. However, some
+	 * platforms (such as SD8686 on OLPC) do not instantly cut power,
+	 * meaning that a reset is required when restoring power soon after
+	 * powering off. It is harmless in other cases.
+	 *
+	 * The CMD5 reset (mmc_send_io_op_cond()), according to the SDIO spec,
+	 * is not necessary for non-removable cards. However, it is required
+	 * for OLPC SD8686 (which expects a [CMD5,5,3,7] init sequence), and
+	 * harmless in other situations.
+	 *
+	 * With these steps taken, mmc_select_voltage() is also required to
+	 * restore the correct voltage setting of the card.
+	 */
+	sdio_reset(host);
+	mmc_go_idle(host);
+	mmc_send_if_cond(host, host->ocr_avail);
+
+	ret = mmc_send_io_op_cond(host, 0, &ocr);
+	if (ret)
+		goto out;
+
+	if (host->ocr_avail_sdio)
+		host->ocr_avail = host->ocr_avail_sdio;
+
+	host->ocr = mmc_select_voltage(host, ocr & ~0x7F);
+	if (!host->ocr) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	ret = mmc_sdio_init_card(host, host->ocr, host->card,
 				mmc_card_keep_power(host));
 	if (!ret && host->sdio_irqs)
 		mmc_signal_sdio_irq(host);
+
+out:
 	mmc_release_host(host);
 
 	return ret;
-- 
1.7.5.4