MMC runtime PM patches break libertas probe

MMC runtime PM patches break libertas probe

[Daniel Drake]

Hi,

I'm running linus master and can't load the libertas module for the
sd8686 wifi hardware in the OLPC XO-1.5 laptop.
Probe fails with -16.

dmesg and config:
http://dev.laptop.org/~dsd/20101031/dmesg-sdio-probe-fail.txt
http://dev.laptop.org/~dsd/20101031/config-sdio-probe-fail.txt

I've done some initial investigation; in sdio_bus_probe() we do this:
	ret = pm_runtime_get_sync(dev);
	if (ret < 0)
		goto out;

and pm_runtime_get_sync() returns -16

Daniel

Re: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen]

Hi Daniel,

On Sun, Oct 31, 2010 at 4:29 PM, Daniel Drake <dsd@laptop.org> wrote:
> I'm running linus master and can't load the libertas module for the
> sd8686 wifi hardware in the OLPC XO-1.5 laptop.
> Probe fails with -16.

That's why I asked you if this scenario works in the other thread.
It's probably the same issue (both scenarios try to power up the chip
by calling mmc_sdio_power_restore).

I guess the error comes from mmc_sdio_init_card() - can you please
check out what exactly triggers it inside that function (just put some
printk's there..) ?

Thanks!
Ohad.

>
> dmesg and config:
> http://dev.laptop.org/~dsd/20101031/dmesg-sdio-probe-fail.txt
> http://dev.laptop.org/~dsd/20101031/config-sdio-probe-fail.txt
>
> I've done some initial investigation; in sdio_bus_probe() we do this:
>        ret = pm_runtime_get_sync(dev);
>        if (ret < 0)
>                goto out;
>
> and pm_runtime_get_sync() returns -16
>
> Daniel
>

Re: MMC runtime PM patches break libertas probe

[Daniel Drake]

On 31 October 2010 14:42, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> I guess the error comes from mmc_sdio_init_card() - can you please
> check out what exactly triggers it inside that function (just put some
> printk's there..) ?

	/*
	 * For native busses:  set card RCA and quit open drain mode.
	 */
	if (!powered_resume && !mmc_host_is_spi(host)) {
		err = mmc_send_relative_addr(host, &card->rca);

This returns -110

Thanks,
Daniel

Re: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen]

On Sun, Oct 31, 2010 at 4:55 PM, Daniel Drake <dsd@laptop.org> wrote:
>        /*
>         * For native busses:  set card RCA and quit open drain mode.
>         */
>        if (!powered_resume && !mmc_host_is_spi(host)) {
>                err = mmc_send_relative_addr(host, &card->rca);
>
> This returns -110

Quick question - how did you manage the power to the sd8686 ? Was it
possible to power it down after boot or was it always kept high ?

Thanks,
Ohad.

>
> Thanks,
> Daniel
>

Re: MMC runtime PM patches break libertas probe

[Daniel Drake]

On 31 October 2010 15:08, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Quick question - how did you manage the power to the sd8686 ? Was it
> possible to power it down after boot or was it always kept high ?

I didn't do anything except boot then try and load the module. Does
that answer the question?

Daniel

Re: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen]

On Sun, Oct 31, 2010 at 5:10 PM, Daniel Drake <dsd@laptop.org> wrote:
> On 31 October 2010 15:08, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> Quick question - how did you manage the power to the sd8686 ? Was it
>> possible to power it down after boot or was it always kept high ?
>
> I didn't do anything except boot then try and load the module. Does
> that answer the question?

No, I'm asking a more general question about the sd8686 and the XO-1.5.
How can one control the power to the sd8686 ? Is it always on and
can't be controlled ? Is there a GPIO that controls it ? or any other
mean to control its power ?

I'm almost sure I understand what's going on, but your answer can
confirm my speculation.

Thanks,
Ohad.

Re: MMC runtime PM patches break libertas probe

[Daniel Drake]

On 31 October 2010 15:16, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> No, I'm asking a more general question about the sd8686 and the XO-1.5.
> How can one control the power to the sd8686 ? Is it always on and
> can't be controlled ? Is there a GPIO that controls it ? or any other
> mean to control its power ?

The power can be controlled by the regular

Re: MMC runtime PM patches break libertas probe

[Daniel Drake]

On 31 October 2010 15:16, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> No, I'm asking a more general question about the sd8686 and the XO-1.5.
> How can one control the power to the sd8686 ? Is it always on and
> can't be controlled ? Is there a GPIO that controls it ? or any other
> mean to control its power ?

The power can be controlled by the regular SD power pin. There is no
GPIO to control it.

Daniel

Re: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen]

On Sun, Oct 31, 2010 at 5:21 PM, Daniel Drake <dsd@laptop.org> wrote:
> The power can be controlled by the regular SD power pin. There is no
> GPIO to control it.

OK, Good.

Can you please tell me the output of the following line
(after boot, but before you load the driver):

cat /sys/kernel/debug/mmc1/ios

(obviously you will have to mount -t debugfs none /sys/kernel/debug)

Re: MMC runtime PM patches break libertas probe

[Daniel Drake]

On 31 October 2010 15:27, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Can you please tell me the output of the following line
> (after boot, but before you load the driver):
>
> 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)

Re: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen]

On Sun, Oct 31, 2010 at 5:57 PM, Daniel Drake <dsd@laptop.org> wrote:
>> 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)

Good. Power is taken down after the mmc+sdio devices are added to the
device tree.

Just to make sure - on an older kernel (that doesn't have SDIO runtime
pm), the card is powered on at this stage (this info will help me rule
out some corner cases) ?

Re: MMC runtime PM patches break libertas probe

[Daniel Drake]

On 31 October 2010 16:16, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Just to make sure - on an older kernel (that doesn't have SDIO runtime
> pm), the card is powered on at this stage (this info will help me rule
> out some corner cases) ?

Looks that way. Same test, after reverting your patches:

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)

Re: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen]

On Sun, Oct 31, 2010 at 6:24 PM, Daniel Drake <dsd@laptop.org> wrote:
> On 31 October 2010 16:16, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> Just to make sure - on an older kernel (that doesn't have SDIO runtime
>> pm), the card is powered on at this stage (this info will help me rule
>> out some corner cases) ?
>
> Looks that way. Same test, after reverting your patches:
>
> 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)

OK, as expected.

So to summarize:
1. Card is powered up at boot, and successfully initialized
2. After mmc + sdio devices are added to the device tree, power is
(seemingly) taken down by runtime PM
3. When the driver is loaded, card is powered up again, but doesn't
respond to CMD3

The only explanation I can think of why the card doesn't respond to
CMD3, after its power is brought up again, is that we didn't have a
full reset here (i.e. mmc_power_off() didn't completely power off
everything).

It's interesting to understand exactly what happens in the context of
the XO-1.5 (there can be several possible culprits, including a
controller quirk), but even if that's not exactly the case here, it is
pretty clear that we need to support boards with controllers/cards
which we can't power off in runtime.

On such boards, the right thing to do would be to disable runtime PM altogether.

Luckily I'll have the XO-1.5 later on this week to cook a fix and test it.

Thanks,
Ohad.

Re: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen]

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

Hi Daniel,

On Sun, Oct 31, 2010 at 9:06 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
...
> we need to support boards with controllers/cards
> which we can't power off in runtime.
>
> On such boards, the right thing to do would be to disable runtime PM altogether.

The patch below (/attached) should trivially fix the problem - can you
please check it out ?

It's very simple: it just requires hosts to explicitly indicate they
support runtime powering off/on their cards (by means of
MMC_CAP_RUNTIME_PM).

There is no way around this I'm afraid: it is legitimate for a
board/host/card configuration not to support powering off the card
after boot, and we must allow it.

In addition having an explicit MMC_CAP_RUNTIME_PM will also allow us
smoother transition to runtime PM behavior. Developers will have to
explicitly turn it on, and will not be surprised if things won't
immediately work.

Please note that this cap is not interchangeable, and can't be replace
with CONFIG_PM_RUNTIME.

Having said that, I still think we need to understand why CMD3 timed
out on the XO-1.5.

Thanks,
Ohad.

>From 6b5691bdd8184cc0876d209c69d38844ea10f678 Mon Sep 17 00:00:00 2001
From: Ohad Ben-Cohen <ohad@wizery.com>
Date: Mon, 1 Nov 2010 09:41:44 +0200
Subject: [PATCH] mmc: add MMC_CAP_RUNTIME_PM

Some board/card/host configurations are not capable of powering off/on
on the card during runtime.

To support such configurations, and to allow smoother transition to
runtime PM behavior, MMC_CAP_RUNTIME_PM is added, so hosts need to
explicitly indicate that it's OK to power off their cards after boot.

This will prevent sdio_bus_probe() from failing to power on the card
when the driver is loaded on such setups.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 drivers/mmc/core/sdio.c     |   37 +++++++++++++++++++++++--------------
 drivers/mmc/core/sdio_bus.c |   33 ++++++++++++++++++++++-----------
 include/linux/mmc/host.h    |    1 +
 3 files changed, 46 insertions(+), 25 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 6a9ad40..373d56d 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -547,9 +547,11 @@ static void mmc_sdio_detect(struct mmc_host *host)
 	BUG_ON(!host->card);

 	/* Make sure card is powered before detecting it */
-	err = pm_runtime_get_sync(&host->card->dev);
-	if (err < 0)
-		goto out;
+	if (host->caps & MMC_CAP_RUNTIME_PM) {
+		err = pm_runtime_get_sync(&host->card->dev);
+		if (err < 0)
+			goto out;
+	}

 	mmc_claim_host(host);

@@ -570,7 +572,8 @@ out:
 	}

 	/* Tell PM core that we're done */
-	pm_runtime_put(&host->card->dev);
+	if (host->caps & MMC_CAP_RUNTIME_PM)
+		pm_runtime_put(&host->card->dev);
 }

 /*
@@ -720,16 +723,21 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 	card = host->card;

 	/*
-	 * Let runtime PM core know our card is active
+	 * Enable runtime PM only if supported by host+card+board
 	 */
-	err = pm_runtime_set_active(&card->dev);
-	if (err)
-		goto remove;
+	if (host->caps & MMC_CAP_RUNTIME_PM) {
+		/*
+		 * Let runtime PM core know our card is active
+		 */
+		err = pm_runtime_set_active(&card->dev);
+		if (err)
+			goto remove;

-	/*
-	 * Enable runtime PM for this card
-	 */
-	pm_runtime_enable(&card->dev);
+		/*
+		 * Enable runtime PM for this card
+		 */
+		pm_runtime_enable(&card->dev);
+	}

 	/*
 	 * The number of functions on the card is encoded inside
@@ -747,9 +755,10 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 			goto remove;

 		/*
-		 * Enable Runtime PM for this func
+		 * Enable Runtime PM for this func (if supported)
 		 */
-		pm_runtime_enable(&card->sdio_func[i]->dev);
+		if (host->caps & MMC_CAP_RUNTIME_PM)
+			pm_runtime_enable(&card->sdio_func[i]->dev);
 	}

 	mmc_release_host(host);
diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
index 2716c7a..f3ce21b 100644
--- a/drivers/mmc/core/sdio_bus.c
+++ b/drivers/mmc/core/sdio_bus.c
@@ -17,6 +17,7 @@
 #include <linux/pm_runtime.h>

 #include <linux/mmc/card.h>
+#include <linux/mmc/host.h>
 #include <linux/mmc/sdio_func.h>

 #include "sdio_cis.h"
@@ -132,9 +133,11 @@ static int sdio_bus_probe(struct device *dev)
 	 * it should call pm_runtime_put_noidle() in its probe routine and
 	 * pm_runtime_get_noresume() in its remove routine.
 	 */
-	ret = pm_runtime_get_sync(dev);
-	if (ret < 0)
-		goto out;
+	if (func->card->host->caps & MMC_CAP_RUNTIME_PM) {
+		ret = pm_runtime_get_sync(dev);
+		if (ret < 0)
+			goto out;
+	}

 	/* Set the default block size so the driver is sure it's something
 	 * sensible. */
@@ -151,7 +154,8 @@ static int sdio_bus_probe(struct device *dev)
 	return 0;

 disable_runtimepm:
-	pm_runtime_put_noidle(dev);
+	if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
+		pm_runtime_put_noidle(dev);
 out:
 	return ret;
 }
@@ -160,12 +164,14 @@ static int sdio_bus_remove(struct device *dev)
 {
 	struct sdio_driver *drv = to_sdio_driver(dev->driver);
 	struct sdio_func *func = dev_to_sdio_func(dev);
-	int ret;
+	int ret = 0;

 	/* Make sure card is powered before invoking ->remove() */
-	ret = pm_runtime_get_sync(dev);
-	if (ret < 0)
-		goto out;
+	if (func->card->host->caps & MMC_CAP_RUNTIME_PM) {
+		ret = pm_runtime_get_sync(dev);
+		if (ret < 0)
+			goto out;
+	}

 	drv->remove(func);

@@ -178,10 +184,12 @@ static int sdio_bus_remove(struct device *dev)
 	}

 	/* First, undo the increment made directly above */
-	pm_runtime_put_noidle(dev);
+	if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
+		pm_runtime_put_noidle(dev);

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

 out:
 	return ret;
@@ -191,6 +199,8 @@ out:

 static int sdio_bus_pm_prepare(struct device *dev)
 {
+	struct sdio_func *func = dev_to_sdio_func(dev);
+
 	/*
 	 * Resume an SDIO device which was suspended at run time at this
 	 * point, in order to allow standard SDIO suspend/resume paths
@@ -212,7 +222,8 @@ static int sdio_bus_pm_prepare(struct device *dev)
 	 * since there is little point in failing system suspend if a
 	 * device can't be resumed.
 	 */
-	pm_runtime_resume(dev);
+	if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
+		pm_runtime_resume(dev);

 	return 0;
 }
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index c3ffad8..e5eee0e 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -167,6 +167,7 @@ struct mmc_host {
 						/* DDR mode at 1.8V */
 #define MMC_CAP_1_2V_DDR	(1 << 12)	/* can support */
 						/* DDR mode at 1.2V */
+#define MMC_CAP_RUNTIME_PM	(1 << 13)	/* Can power off/on in runtime */

 	mmc_pm_flag_t		pm_caps;	/* supported pm features */

-- 
1.7.0.4

[-- Attachment #2: 0001-mmc-add-MMC_CAP_RUNTIME_PM.patch --]
[-- Type: text/x-patch, Size: 5656 bytes --]

From 6b5691bdd8184cc0876d209c69d38844ea10f678 Mon Sep 17 00:00:00 2001
From: Ohad Ben-Cohen <ohad@wizery.com>
Date: Mon, 1 Nov 2010 09:41:44 +0200
Subject: [PATCH] mmc: add MMC_CAP_RUNTIME_PM

Some board/card/host configurations are not capable of powering off/on
on the card during runtime.

To support such configurations, and to allow smoother transition to
runtime PM behavior, MMC_CAP_RUNTIME_PM is added, so hosts need to
explicitly indicate that it's OK to power off their cards after boot.

This will prevent sdio_bus_probe() from failing to power on the card
when the driver is loaded on such setups.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 drivers/mmc/core/sdio.c     |   37 +++++++++++++++++++++++--------------
 drivers/mmc/core/sdio_bus.c |   33 ++++++++++++++++++++++-----------
 include/linux/mmc/host.h    |    1 +
 3 files changed, 46 insertions(+), 25 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 6a9ad40..373d56d 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -547,9 +547,11 @@ static void mmc_sdio_detect(struct mmc_host *host)
 	BUG_ON(!host->card);
 
 	/* Make sure card is powered before detecting it */
-	err = pm_runtime_get_sync(&host->card->dev);
-	if (err < 0)
-		goto out;
+	if (host->caps & MMC_CAP_RUNTIME_PM) {
+		err = pm_runtime_get_sync(&host->card->dev);
+		if (err < 0)
+			goto out;
+	}
 
 	mmc_claim_host(host);
 
@@ -570,7 +572,8 @@ out:
 	}
 
 	/* Tell PM core that we're done */
-	pm_runtime_put(&host->card->dev);
+	if (host->caps & MMC_CAP_RUNTIME_PM)
+		pm_runtime_put(&host->card->dev);
 }
 
 /*
@@ -720,16 +723,21 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 	card = host->card;
 
 	/*
-	 * Let runtime PM core know our card is active
+	 * Enable runtime PM only if supported by host+card+board
 	 */
-	err = pm_runtime_set_active(&card->dev);
-	if (err)
-		goto remove;
+	if (host->caps & MMC_CAP_RUNTIME_PM) {
+		/*
+		 * Let runtime PM core know our card is active
+		 */
+		err = pm_runtime_set_active(&card->dev);
+		if (err)
+			goto remove;
 
-	/*
-	 * Enable runtime PM for this card
-	 */
-	pm_runtime_enable(&card->dev);
+		/*
+		 * Enable runtime PM for this card
+		 */
+		pm_runtime_enable(&card->dev);
+	}
 
 	/*
 	 * The number of functions on the card is encoded inside
@@ -747,9 +755,10 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 			goto remove;
 
 		/*
-		 * Enable Runtime PM for this func
+		 * Enable Runtime PM for this func (if supported)
 		 */
-		pm_runtime_enable(&card->sdio_func[i]->dev);
+		if (host->caps & MMC_CAP_RUNTIME_PM)
+			pm_runtime_enable(&card->sdio_func[i]->dev);
 	}
 
 	mmc_release_host(host);
diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
index 2716c7a..f3ce21b 100644
--- a/drivers/mmc/core/sdio_bus.c
+++ b/drivers/mmc/core/sdio_bus.c
@@ -17,6 +17,7 @@
 #include <linux/pm_runtime.h>
 
 #include <linux/mmc/card.h>
+#include <linux/mmc/host.h>
 #include <linux/mmc/sdio_func.h>
 
 #include "sdio_cis.h"
@@ -132,9 +133,11 @@ static int sdio_bus_probe(struct device *dev)
 	 * it should call pm_runtime_put_noidle() in its probe routine and
 	 * pm_runtime_get_noresume() in its remove routine.
 	 */
-	ret = pm_runtime_get_sync(dev);
-	if (ret < 0)
-		goto out;
+	if (func->card->host->caps & MMC_CAP_RUNTIME_PM) {
+		ret = pm_runtime_get_sync(dev);
+		if (ret < 0)
+			goto out;
+	}
 
 	/* Set the default block size so the driver is sure it's something
 	 * sensible. */
@@ -151,7 +154,8 @@ static int sdio_bus_probe(struct device *dev)
 	return 0;
 
 disable_runtimepm:
-	pm_runtime_put_noidle(dev);
+	if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
+		pm_runtime_put_noidle(dev);
 out:
 	return ret;
 }
@@ -160,12 +164,14 @@ static int sdio_bus_remove(struct device *dev)
 {
 	struct sdio_driver *drv = to_sdio_driver(dev->driver);
 	struct sdio_func *func = dev_to_sdio_func(dev);
-	int ret;
+	int ret = 0;
 
 	/* Make sure card is powered before invoking ->remove() */
-	ret = pm_runtime_get_sync(dev);
-	if (ret < 0)
-		goto out;
+	if (func->card->host->caps & MMC_CAP_RUNTIME_PM) {
+		ret = pm_runtime_get_sync(dev);
+		if (ret < 0)
+			goto out;
+	}
 
 	drv->remove(func);
 
@@ -178,10 +184,12 @@ static int sdio_bus_remove(struct device *dev)
 	}
 
 	/* First, undo the increment made directly above */
-	pm_runtime_put_noidle(dev);
+	if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
+		pm_runtime_put_noidle(dev);
 
 	/* Then undo the runtime PM settings in sdio_bus_probe() */
-	pm_runtime_put_noidle(dev);
+	if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
+		pm_runtime_put_noidle(dev);
 
 out:
 	return ret;
@@ -191,6 +199,8 @@ out:
 
 static int sdio_bus_pm_prepare(struct device *dev)
 {
+	struct sdio_func *func = dev_to_sdio_func(dev);
+
 	/*
 	 * Resume an SDIO device which was suspended at run time at this
 	 * point, in order to allow standard SDIO suspend/resume paths
@@ -212,7 +222,8 @@ static int sdio_bus_pm_prepare(struct device *dev)
 	 * since there is little point in failing system suspend if a
 	 * device can't be resumed.
 	 */
-	pm_runtime_resume(dev);
+	if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
+		pm_runtime_resume(dev);
 
 	return 0;
 }
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index c3ffad8..e5eee0e 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -167,6 +167,7 @@ struct mmc_host {
 						/* DDR mode at 1.8V */
 #define MMC_CAP_1_2V_DDR	(1 << 12)	/* can support */
 						/* DDR mode at 1.2V */
+#define MMC_CAP_RUNTIME_PM	(1 << 13)	/* Can power off/on in runtime */
 
 	mmc_pm_flag_t		pm_caps;	/* supported pm features */
 
-- 
1.7.0.4

Re: MMC runtime PM patches break libertas probe

[Daniel Drake]

On 1 November 2010 08:27, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> On such boards, the right thing to do would be to disable runtime PM altogether.
>
> The patch below (/attached) should trivially fix the problem - can you
> please check it out ?

Thanks.
It solves the problem.

But, what you point out is quite interesting.
We currently have a (not-yet-upstream) rfkill driver for the XO-1.5,
which we also put in place for power saving reasons. It operates by
calling mmc_stop_host(). In light of your work, I guess that wasn't
really turning off the card.

So, it would be great if you could get runtime PM working on this
combo, and then later we could make a real rfkill driver.

I guess you have an XO-1.5 now. I'm happy to help you get started, I
guess this is your first time working with an XO. Drop by irc.oftc.net
#olpc-devel and I'll happily help you get started booting your own
kernel etc.

Thanks!
Daniel

Re: MMC runtime PM patches break libertas probe

[Nicolas Pitre]

On Sat, 6 Nov 2010, Daniel Drake wrote:

> On 1 November 2010 08:27, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> >> On such boards, the right thing to do would be to disable runtime PM altogether.
> >
> > The patch below (/attached) should trivially fix the problem - can you
> > please check it out ?
> 
> Thanks.
> It solves the problem.
> 
> But, what you point out is quite interesting.
> We currently have a (not-yet-upstream) rfkill driver for the XO-1.5,
> which we also put in place for power saving reasons. It operates by
> calling mmc_stop_host().

Couldn't you implement this any other way?  This seems to be a really 
bad strategy for this IMHO.


Nicolas

Re: MMC runtime PM patches break libertas probe

[Daniel Drake]

On 7 November 2010 01:48, Nicolas Pitre <nico@fluxnic.net> wrote:
> Couldn't you implement this any other way?  This seems to be a really
> bad strategy for this IMHO.

What's so bad about it?
Any suggestions?

Daniel

Re: MMC runtime PM patches break libertas probe

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 456 bytes --]

On Sun, 7 Nov 2010, Daniel Drake wrote:

> On 7 November 2010 01:48, Nicolas Pitre <nico@fluxnic.net> wrote:
> > Couldn't you implement this any other way?  This seems to be a really
> > bad strategy for this IMHO.
> 
> What's so bad about it?
> Any suggestions?

If you want rfkill functionality, then maybe that would be a good idea 
to implement it _within_ the libertas driver instead of hacking 
something around it in an unrelated fashion.


Nicolas

Re: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen]

On Sat, Nov 6, 2010 at 11:19 PM, Daniel Drake <dsd@laptop.org> wrote:
> Thanks.
> It solves the problem.

Good. Did it also solve the 1st issue you had ?

Anyway I'm still interested to reproduce that crash. No reason to get
a kernel panic whatsoever.

>
> But, what you point out is quite interesting.
> We currently have a (not-yet-upstream) rfkill driver for the XO-1.5,
> which we also put in place for power saving reasons.

Can you please elaborate ? What power saving ? How do you exactly plan
to use it ? Do you plan to power off the card without taking the wlan
interface down first ?

> It operates by
> calling mmc_stop_host(). In light of your work, I guess that wasn't
> really turning off the card.

Sounds like it.

Chris and I briefly discussed this in LPC, and he mentioned that you
might have an external power source to that card, that is not
controlled by the mmc regulator, and which you disable/enable from
user space just before suspending the host ?

>
> So, it would be great if you could get runtime PM working on this
> combo, and then later we could make a real rfkill driver.

The XO-1.5 problem is orthogonal to runtime PM. If your regulator
really powered down your card, then everything would just have worked.

>
> I guess you have an XO-1.5 now. I'm happy to help you get started, I
> guess this is your first time working with an XO. Drop by irc.oftc.net
> #olpc-devel and I'll happily help you get started booting your own
> kernel etc.

Thanks. This would allow me to test generic core code on a platform
which is completely different from those I have.

Thanks,
Ohad.

>
> Thanks!
> Daniel
>

Re: MMC runtime PM patches break libertas probe

[Daniel Drake]

On 7 November 2010 10:42, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Good. Did it also solve the 1st issue you had ?

Yes.

>> But, what you point out is quite interesting.
>> We currently have a (not-yet-upstream) rfkill driver for the XO-1.5,
>> which we also put in place for power saving reasons.
>
> Can you please elaborate ? What power saving ? How do you exactly plan
> to use it ? Do you plan to power off the card without taking the wlan
> interface down first ?

If you take the interface down, the card is still powered up in some
way. We were looking for a way to cut as much power as possible.
It can be used by the "rfkill block wifi" command, and theres a
checkbox available in the sugar UI which calls that.

> Chris and I briefly discussed this in LPC, and he mentioned that you
> might have an external power source to that card, that is not
> controlled by the mmc regulator, and which you disable/enable from
> user space just before suspending the host ?

Not sure what he's referring to there. Perhaps he can clarify.

When I asked the hardware guy he said it was entirely controlled by
the normal SD power pin, but a full card reset will be needed after
cycling the power.

Daniel

Re: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen]

On Sun, Nov 7, 2010 at 12:51 PM, Daniel Drake <dsd@laptop.org> wrote:
> If you take the interface down, the card is still powered up in some
> way.

With SDIO runtime pm support, it shouldn't be.

> When I asked the hardware guy he said it was entirely controlled by
> the normal SD power pin, but a full card reset will be needed after
> cycling the power.

We need to understand why the wlan card on the XO-1.5 does not go
through full power off/on cycle, despite software calling
mmc_power_off/on.

If this is solved, I don't think you will still need that rfkill driver.

Re: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen]

On Mon, Nov 1, 2010 at 10:27 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Sun, Oct 31, 2010 at 9:06 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> ...
>> we need to support boards with controllers/cards
>> which we can't power off in runtime.
>>
>> On such boards, the right thing to do would be to disable runtime PM altogether.
>
> The patch below (/attached) should trivially fix the problem - can you
> please check it out ?
>
> It's very simple: it just requires hosts to explicitly indicate they
> support runtime powering off/on their cards (by means of
> MMC_CAP_RUNTIME_PM).
>
> There is no way around this I'm afraid: it is legitimate for a
> board/host/card configuration not to support powering off the card
> after boot, and we must allow it.
>
> In addition having an explicit MMC_CAP_RUNTIME_PM will also allow us
> smoother transition to runtime PM behavior. Developers will have to
> explicitly turn it on, and will not be surprised if things won't
> immediately work.
>
> Please note that this cap is not interchangeable, and can't be replace
> with CONFIG_PM_RUNTIME.
>
> Having said that, I still think we need to understand why CMD3 timed
> out on the XO-1.5.

Just to update the list, the problem with the XO-1.5 was because the
sd8686 has an external reset gpio line which is currently being
manipulated manually by an out-of-tree kernel patch:

http://dev.laptop.org/git/olpc-2.6/commit/?h=olpc-2.6.35&id=e9bee721fb0cc303286d1fe5df4930ce79b0b1e0

... which makes me wonder whether we really want to take that
MMC_CAP_RUNTIME_PM road. I'm not sure anymore.

We need a demonstrated hardware limitation before we take that
approach (or a setup which worked using vanilla kernels and now
doesn't), because frankly this MMC_CAP_RUNTIME_PM approach is
cumbersome and involving. I would not like to introduce it just to fix
out-of-tree software issues, and it seems that at least the XO-1.5
case can be cleanly solved by software (e.g. by letting the regulator
handle that sd8686 quirk).

I'm looping in Arnd, who reported similar problems with b43-sdio on
AP4EVB (arm) with tmio_mmc.

Arnd,

We're trying to exactly understand the reasons behind the reported
SDIO runtime pm failures (we had two, yours, and OLPC). So far it
seems that the OLPC had a software issue, and I'm wondering about
yours.

Can you please supply additional information about your board ? where
does your wifi card gets its power from ? is there an external gpio
involved ? Were you able to work with vanilla kernels (prior to
2.6.37) or do you carry some out-of-tree patches ?

Thanks,
Ohad.

>
> From 6b5691bdd8184cc0876d209c69d38844ea10f678 Mon Sep 17 00:00:00 2001
> From: Ohad Ben-Cohen <ohad@wizery.com>
> Date: Mon, 1 Nov 2010 09:41:44 +0200
> Subject: [PATCH] mmc: add MMC_CAP_RUNTIME_PM
>
> Some board/card/host configurations are not capable of powering off/on
> on the card during runtime.
>
> To support such configurations, and to allow smoother transition to
> runtime PM behavior, MMC_CAP_RUNTIME_PM is added, so hosts need to
> explicitly indicate that it's OK to power off their cards after boot.
>
> This will prevent sdio_bus_probe() from failing to power on the card
> when the driver is loaded on such setups.
>
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
> ---
>  drivers/mmc/core/sdio.c     |   37 +++++++++++++++++++++++--------------
>  drivers/mmc/core/sdio_bus.c |   33 ++++++++++++++++++++++-----------
>  include/linux/mmc/host.h    |    1 +
>  3 files changed, 46 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index 6a9ad40..373d56d 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -547,9 +547,11 @@ static void mmc_sdio_detect(struct mmc_host *host)
>        BUG_ON(!host->card);
>
>        /* Make sure card is powered before detecting it */
> -       err = pm_runtime_get_sync(&host->card->dev);
> -       if (err < 0)
> -               goto out;
> +       if (host->caps & MMC_CAP_RUNTIME_PM) {
> +               err = pm_runtime_get_sync(&host->card->dev);
> +               if (err < 0)
> +                       goto out;
> +       }
>
>        mmc_claim_host(host);
>
> @@ -570,7 +572,8 @@ out:
>        }
>
>        /* Tell PM core that we're done */
> -       pm_runtime_put(&host->card->dev);
> +       if (host->caps & MMC_CAP_RUNTIME_PM)
> +               pm_runtime_put(&host->card->dev);
>  }
>
>  /*
> @@ -720,16 +723,21 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
>        card = host->card;
>
>        /*
> -        * Let runtime PM core know our card is active
> +        * Enable runtime PM only if supported by host+card+board
>         */
> -       err = pm_runtime_set_active(&card->dev);
> -       if (err)
> -               goto remove;
> +       if (host->caps & MMC_CAP_RUNTIME_PM) {
> +               /*
> +                * Let runtime PM core know our card is active
> +                */
> +               err = pm_runtime_set_active(&card->dev);
> +               if (err)
> +                       goto remove;
>
> -       /*
> -        * Enable runtime PM for this card
> -        */
> -       pm_runtime_enable(&card->dev);
> +               /*
> +                * Enable runtime PM for this card
> +                */
> +               pm_runtime_enable(&card->dev);
> +       }
>
>        /*
>         * The number of functions on the card is encoded inside
> @@ -747,9 +755,10 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
>                        goto remove;
>
>                /*
> -                * Enable Runtime PM for this func
> +                * Enable Runtime PM for this func (if supported)
>                 */
> -               pm_runtime_enable(&card->sdio_func[i]->dev);
> +               if (host->caps & MMC_CAP_RUNTIME_PM)
> +                       pm_runtime_enable(&card->sdio_func[i]->dev);
>        }
>
>        mmc_release_host(host);
> diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
> index 2716c7a..f3ce21b 100644
> --- a/drivers/mmc/core/sdio_bus.c
> +++ b/drivers/mmc/core/sdio_bus.c
> @@ -17,6 +17,7 @@
>  #include <linux/pm_runtime.h>
>
>  #include <linux/mmc/card.h>
> +#include <linux/mmc/host.h>
>  #include <linux/mmc/sdio_func.h>
>
>  #include "sdio_cis.h"
> @@ -132,9 +133,11 @@ static int sdio_bus_probe(struct device *dev)
>         * it should call pm_runtime_put_noidle() in its probe routine and
>         * pm_runtime_get_noresume() in its remove routine.
>         */
> -       ret = pm_runtime_get_sync(dev);
> -       if (ret < 0)
> -               goto out;
> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM) {
> +               ret = pm_runtime_get_sync(dev);
> +               if (ret < 0)
> +                       goto out;
> +       }
>
>        /* Set the default block size so the driver is sure it's something
>         * sensible. */
> @@ -151,7 +154,8 @@ static int sdio_bus_probe(struct device *dev)
>        return 0;
>
>  disable_runtimepm:
> -       pm_runtime_put_noidle(dev);
> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
> +               pm_runtime_put_noidle(dev);
>  out:
>        return ret;
>  }
> @@ -160,12 +164,14 @@ static int sdio_bus_remove(struct device *dev)
>  {
>        struct sdio_driver *drv = to_sdio_driver(dev->driver);
>        struct sdio_func *func = dev_to_sdio_func(dev);
> -       int ret;
> +       int ret = 0;
>
>        /* Make sure card is powered before invoking ->remove() */
> -       ret = pm_runtime_get_sync(dev);
> -       if (ret < 0)
> -               goto out;
> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM) {
> +               ret = pm_runtime_get_sync(dev);
> +               if (ret < 0)
> +                       goto out;
> +       }
>
>        drv->remove(func);
>
> @@ -178,10 +184,12 @@ static int sdio_bus_remove(struct device *dev)
>        }
>
>        /* First, undo the increment made directly above */
> -       pm_runtime_put_noidle(dev);
> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
> +               pm_runtime_put_noidle(dev);
>
>        /* Then undo the runtime PM settings in sdio_bus_probe() */
> -       pm_runtime_put_noidle(dev);
> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
> +               pm_runtime_put_noidle(dev);
>
>  out:
>        return ret;
> @@ -191,6 +199,8 @@ out:
>
>  static int sdio_bus_pm_prepare(struct device *dev)
>  {
> +       struct sdio_func *func = dev_to_sdio_func(dev);
> +
>        /*
>         * Resume an SDIO device which was suspended at run time at this
>         * point, in order to allow standard SDIO suspend/resume paths
> @@ -212,7 +222,8 @@ static int sdio_bus_pm_prepare(struct device *dev)
>         * since there is little point in failing system suspend if a
>         * device can't be resumed.
>         */
> -       pm_runtime_resume(dev);
> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
> +               pm_runtime_resume(dev);
>
>        return 0;
>  }
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index c3ffad8..e5eee0e 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -167,6 +167,7 @@ struct mmc_host {
>                                                /* DDR mode at 1.8V */
>  #define MMC_CAP_1_2V_DDR       (1 << 12)       /* can support */
>                                                /* DDR mode at 1.2V */
> +#define MMC_CAP_RUNTIME_PM     (1 << 13)       /* Can power off/on in runtime */
>
>        mmc_pm_flag_t           pm_caps;        /* supported pm features */
>
> --
> 1.7.0.4
>

Re: MMC runtime PM patches break libertas probe

[Daniel Drake]

On 16 November 2010 13:22, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Just to update the list, the problem with the XO-1.5 was because the
> sd8686 has an external reset gpio line which is currently being
> manipulated manually by an out-of-tree kernel patch:
>
> http://dev.laptop.org/git/olpc-2.6/commit/?h=olpc-2.6.35&id=e9bee721fb0cc303286d1fe5df4930ce79b0b1e0
>
> ... which makes me wonder whether we really want to take that
> MMC_CAP_RUNTIME_PM road. I'm not sure anymore.

OLPC is not the only user of the sd8686.
Every other user will face the same problem.

Other users may not have the luxury of having a GPIO hooked up to the
reset line.
It's not like Marvell made this limitation very widely known.

Daniel

Re: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen]

On Tue, Nov 16, 2010 at 4:29 PM, Daniel Drake <dsd@laptop.org> wrote:
> On 16 November 2010 13:22, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> Just to update the list, the problem with the XO-1.5 was because the
>> sd8686 has an external reset gpio line which is currently being
>> manipulated manually by an out-of-tree kernel patch:
>>
>> http://dev.laptop.org/git/olpc-2.6/commit/?h=olpc-2.6.35&id=e9bee721fb0cc303286d1fe5df4930ce79b0b1e0
>>
>> ... which makes me wonder whether we really want to take that
>> MMC_CAP_RUNTIME_PM road. I'm not sure anymore.
>
> OLPC is not the only user of the sd8686.
> Every other user will face the same problem.
>
> Other users may not have the luxury of having a GPIO hooked up to the
> reset line.

Agree; those users will need a MMC_CAP_RUNTIME_PM (or maybe call it
with the capability it really stands for which is something like
MMC_CAP_POWER_OFF_CARD).

But I want to be positively sure we have such users (or is it that obvious?).

How is the sd8686's reset line manipulated on other platforms ? Or is
the sd8686 usually just kept powered on after boot ?

I'm looping in libertas-dev.

Thanks,
Ohad.

Re: MMC runtime PM patches break libertas probe

[Mike Rapoport]

On 11/16/10 16:49, Ohad Ben-Cohen wrote:
> On Tue, Nov 16, 2010 at 4:29 PM, Daniel Drake <dsd@laptop.org> wrote:
>> On 16 November 2010 13:22, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>>> Just to update the list, the problem with the XO-1.5 was because the
>>> sd8686 has an external reset gpio line which is currently being
>>> manipulated manually by an out-of-tree kernel patch:
>>>
>>> http://dev.laptop.org/git/olpc-2.6/commit/?h=olpc-2.6.35&id=e9bee721fb0cc303286d1fe5df4930ce79b0b1e0
>>>
>>> ... which makes me wonder whether we really want to take that
>>> MMC_CAP_RUNTIME_PM road. I'm not sure anymore.
>>
>> OLPC is not the only user of the sd8686.
>> Every other user will face the same problem.
>>
>> Other users may not have the luxury of having a GPIO hooked up to the
>> reset line.
> 
> Agree; those users will need a MMC_CAP_RUNTIME_PM (or maybe call it
> with the capability it really stands for which is something like
> MMC_CAP_POWER_OFF_CARD).
> 
> But I want to be positively sure we have such users (or is it that obvious?).
> 
> How is the sd8686's reset line manipulated on other platforms ? Or is
> the sd8686 usually just kept powered on after boot ?

On our platforms we just keep it powered on after boot with the reset line held
high. (e.g. arch/arm/mach-pxa/cm-x300.c, arch/arm/mach-omap/board-cm-t35.c). We
don't bother much for power savings, though.

> I'm looping in libertas-dev.
> 
> Thanks,
> Ohad.
> 
> _______________________________________________
> libertas-dev mailing list
> libertas-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/libertas-dev


-- 
Sincerely yours,
Mike.

Re: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen]

On Wed, Nov 17, 2010 at 8:46 AM, Mike Rapoport <mike@compulab.co.il> wrote:
> On our platforms we just keep it powered on after boot with the reset line held
> high. (e.g. arch/arm/mach-pxa/cm-x300.c, arch/arm/mach-omap/board-cm-t35.c).
> We don't bother much for power savings, though.

Thanks, Mike !

This paves the road for MMC_CAP_POWER_OFF_CARD.

SDIO core will enable runtime PM for the card only if that cap is set.
As a result, the card will be powered down after boot, and will only
be powered up again when a driver is loaded (and then it's up to the
driver whether power will be kept or not).

Re: MMC runtime PM patches break libertas probe

[Nicolas Pitre]

On Wed, 17 Nov 2010, Ohad Ben-Cohen wrote:

> On Wed, Nov 17, 2010 at 8:46 AM, Mike Rapoport <mike@compulab.co.il> wrote:
> > On our platforms we just keep it powered on after boot with the reset line held
> > high. (e.g. arch/arm/mach-pxa/cm-x300.c, arch/arm/mach-omap/board-cm-t35.c).
> > We don't bother much for power savings, though.
> 
> Thanks, Mike !
> 
> This paves the road for MMC_CAP_POWER_OFF_CARD.
> 
> SDIO core will enable runtime PM for the card only if that cap is set.
> As a result, the card will be powered down after boot, and will only
> be powered up again when a driver is loaded (and then it's up to the
> driver whether power will be kept or not).

Makes sense.


Nicolas

Re: MMC runtime PM patches break libertas probe

[Arnd Hannemann]

Am 16.11.2010 14:22, schrieb Ohad Ben-Cohen:
> On Mon, Nov 1, 2010 at 10:27 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>   
>> On Sun, Oct 31, 2010 at 9:06 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> ...
>>     
>>> we need to support boards with controllers/cards
>>> which we can't power off in runtime.
>>>
>>> On such boards, the right thing to do would be to disable runtime PM altogether.
>>>       
>> The patch below (/attached) should trivially fix the problem - can you
>> please check it out ?
>>
>> It's very simple: it just requires hosts to explicitly indicate they
>> support runtime powering off/on their cards (by means of
>> MMC_CAP_RUNTIME_PM).
>>
>> There is no way around this I'm afraid: it is legitimate for a
>> board/host/card configuration not to support powering off the card
>> after boot, and we must allow it.
>>
>> In addition having an explicit MMC_CAP_RUNTIME_PM will also allow us
>> smoother transition to runtime PM behavior. Developers will have to
>> explicitly turn it on, and will not be surprised if things won't
>> immediately work.
>>
>> Please note that this cap is not interchangeable, and can't be replace
>> with CONFIG_PM_RUNTIME.
>>
>> Having said that, I still think we need to understand why CMD3 timed
>> out on the XO-1.5.
>>     
> Just to update the list, the problem with the XO-1.5 was because the
> sd8686 has an external reset gpio line which is currently being
> manipulated manually by an out-of-tree kernel patch:
>
> http://dev.laptop.org/git/olpc-2.6/commit/?h=olpc-2.6.35&id=e9bee721fb0cc303286d1fe5df4930ce79b0b1e0
>
> ... which makes me wonder whether we really want to take that
> MMC_CAP_RUNTIME_PM road. I'm not sure anymore.
>
> We need a demonstrated hardware limitation before we take that
> approach (or a setup which worked using vanilla kernels and now
> doesn't), because frankly this MMC_CAP_RUNTIME_PM approach is
> cumbersome and involving. I would not like to introduce it just to fix
> out-of-tree software issues, and it seems that at least the XO-1.5
> case can be cleanly solved by software (e.g. by letting the regulator
> handle that sd8686 quirk).
>
> I'm looping in Arnd, who reported similar problems with b43-sdio on
> AP4EVB (arm) with tmio_mmc.
>
> Arnd,
>
> We're trying to exactly understand the reasons behind the reported
> SDIO runtime pm failures (we had two, yours, and OLPC). So far it
> seems that the OLPC had a software issue, and I'm wondering about
> yours.
>
> Can you please supply additional information about your board ? where
> does your wifi card gets its power from ? is there an external gpio
> involved ? Were you able to work with vanilla kernels (prior to
> 2.6.37) or do you carry some out-of-tree patches ?
>   
Its an AP4 (SH7372) evaluation board from renesas. It has an SD-Slot,
where you plug the SDIO card into it. No special wiring or something
like this. So I doubt some external GPIOs are involved.
I have no idea how the wifi card gets its power, but I hope its over
some well defined pins within the SD slot...
I was able to work with 2.6.35 and .36 plus some out-of-tree patches
for the sh_mobile_sdhi mfd, which are now in mainline:
mmc: Allow 2 byte requests in 4-bit mode for tmio_mmc
tmio_mmc: don't clear unhandled pending interrupts
tmio_mmc: don't clear unhandled pending interrupts

>> From 6b5691bdd8184cc0876d209c69d38844ea10f678 Mon Sep 17 00:00:00 2001
>> From: Ohad Ben-Cohen <ohad@wizery.com>
>> Date: Mon, 1 Nov 2010 09:41:44 +0200
>> Subject: [PATCH] mmc: add MMC_CAP_RUNTIME_PM
>>
>> Some board/card/host configurations are not capable of powering off/on
>> on the card during runtime.
>>
>> To support such configurations, and to allow smoother transition to
>> runtime PM behavior, MMC_CAP_RUNTIME_PM is added, so hosts need to
>> explicitly indicate that it's OK to power off their cards after boot.
>>
>> This will prevent sdio_bus_probe() from failing to power on the card
>> when the driver is loaded on such setups.
>>
>> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
>> ---
>>  drivers/mmc/core/sdio.c     |   37 +++++++++++++++++++++++--------------
>>  drivers/mmc/core/sdio_bus.c |   33 ++++++++++++++++++++++-----------
>>  include/linux/mmc/host.h    |    1 +
>>  3 files changed, 46 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>> index 6a9ad40..373d56d 100644
>> --- a/drivers/mmc/core/sdio.c
>> +++ b/drivers/mmc/core/sdio.c
>> @@ -547,9 +547,11 @@ static void mmc_sdio_detect(struct mmc_host *host)
>>        BUG_ON(!host->card);
>>
>>        /* Make sure card is powered before detecting it */
>> -       err = pm_runtime_get_sync(&host->card->dev);
>> -       if (err < 0)
>> -               goto out;
>> +       if (host->caps & MMC_CAP_RUNTIME_PM) {
>> +               err = pm_runtime_get_sync(&host->card->dev);
>> +               if (err < 0)
>> +                       goto out;
>> +       }
>>
>>        mmc_claim_host(host);
>>
>> @@ -570,7 +572,8 @@ out:
>>        }
>>
>>        /* Tell PM core that we're done */
>> -       pm_runtime_put(&host->card->dev);
>> +       if (host->caps & MMC_CAP_RUNTIME_PM)
>> +               pm_runtime_put(&host->card->dev);
>>  }
>>
>>  /*
>> @@ -720,16 +723,21 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
>>        card = host->card;
>>
>>        /*
>> -        * Let runtime PM core know our card is active
>> +        * Enable runtime PM only if supported by host+card+board
>>         */
>> -       err = pm_runtime_set_active(&card->dev);
>> -       if (err)
>> -               goto remove;
>> +       if (host->caps & MMC_CAP_RUNTIME_PM) {
>> +               /*
>> +                * Let runtime PM core know our card is active
>> +                */
>> +               err = pm_runtime_set_active(&card->dev);
>> +               if (err)
>> +                       goto remove;
>>
>> -       /*
>> -        * Enable runtime PM for this card
>> -        */
>> -       pm_runtime_enable(&card->dev);
>> +               /*
>> +                * Enable runtime PM for this card
>> +                */
>> +               pm_runtime_enable(&card->dev);
>> +       }
>>
>>        /*
>>         * The number of functions on the card is encoded inside
>> @@ -747,9 +755,10 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
>>                        goto remove;
>>
>>                /*
>> -                * Enable Runtime PM for this func
>> +                * Enable Runtime PM for this func (if supported)
>>                 */
>> -               pm_runtime_enable(&card->sdio_func[i]->dev);
>> +               if (host->caps & MMC_CAP_RUNTIME_PM)
>> +                       pm_runtime_enable(&card->sdio_func[i]->dev);
>>        }
>>
>>        mmc_release_host(host);
>> diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
>> index 2716c7a..f3ce21b 100644
>> --- a/drivers/mmc/core/sdio_bus.c
>> +++ b/drivers/mmc/core/sdio_bus.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/pm_runtime.h>
>>
>>  #include <linux/mmc/card.h>
>> +#include <linux/mmc/host.h>
>>  #include <linux/mmc/sdio_func.h>
>>
>>  #include "sdio_cis.h"
>> @@ -132,9 +133,11 @@ static int sdio_bus_probe(struct device *dev)
>>         * it should call pm_runtime_put_noidle() in its probe routine and
>>         * pm_runtime_get_noresume() in its remove routine.
>>         */
>> -       ret = pm_runtime_get_sync(dev);
>> -       if (ret < 0)
>> -               goto out;
>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM) {
>> +               ret = pm_runtime_get_sync(dev);
>> +               if (ret < 0)
>> +                       goto out;
>> +       }
>>
>>        /* Set the default block size so the driver is sure it's something
>>         * sensible. */
>> @@ -151,7 +154,8 @@ static int sdio_bus_probe(struct device *dev)
>>        return 0;
>>
>>  disable_runtimepm:
>> -       pm_runtime_put_noidle(dev);
>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
>> +               pm_runtime_put_noidle(dev);
>>  out:
>>        return ret;
>>  }
>> @@ -160,12 +164,14 @@ static int sdio_bus_remove(struct device *dev)
>>  {
>>        struct sdio_driver *drv = to_sdio_driver(dev->driver);
>>        struct sdio_func *func = dev_to_sdio_func(dev);
>> -       int ret;
>> +       int ret = 0;
>>
>>        /* Make sure card is powered before invoking ->remove() */
>> -       ret = pm_runtime_get_sync(dev);
>> -       if (ret < 0)
>> -               goto out;
>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM) {
>> +               ret = pm_runtime_get_sync(dev);
>> +               if (ret < 0)
>> +                       goto out;
>> +       }
>>
>>        drv->remove(func);
>>
>> @@ -178,10 +184,12 @@ static int sdio_bus_remove(struct device *dev)
>>        }
>>
>>        /* First, undo the increment made directly above */
>> -       pm_runtime_put_noidle(dev);
>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
>> +               pm_runtime_put_noidle(dev);
>>
>>        /* Then undo the runtime PM settings in sdio_bus_probe() */
>> -       pm_runtime_put_noidle(dev);
>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
>> +               pm_runtime_put_noidle(dev);
>>
>>  out:
>>        return ret;
>> @@ -191,6 +199,8 @@ out:
>>
>>  static int sdio_bus_pm_prepare(struct device *dev)
>>  {
>> +       struct sdio_func *func = dev_to_sdio_func(dev);
>> +
>>        /*
>>         * Resume an SDIO device which was suspended at run time at this
>>         * point, in order to allow standard SDIO suspend/resume paths
>> @@ -212,7 +222,8 @@ static int sdio_bus_pm_prepare(struct device *dev)
>>         * since there is little point in failing system suspend if a
>>         * device can't be resumed.
>>         */
>> -       pm_runtime_resume(dev);
>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
>> +               pm_runtime_resume(dev);
>>
>>        return 0;
>>  }
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index c3ffad8..e5eee0e 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -167,6 +167,7 @@ struct mmc_host {
>>                                                /* DDR mode at 1.8V */
>>  #define MMC_CAP_1_2V_DDR       (1 << 12)       /* can support */
>>                                                /* DDR mode at 1.2V */
>> +#define MMC_CAP_RUNTIME_PM     (1 << 13)       /* Can power off/on in runtime */
>>
>>        mmc_pm_flag_t           pm_caps;        /* supported pm features */
>>
>> --
>> 1.7.0.4
>>
>>     

Re: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen]

On Tue, Nov 16, 2010 at 7:17 PM, Arnd Hannemann
<hannemann@nets.rwth-aachen.de> >> > Its an AP4 (SH7372) evaluation
board from renesas. It has an SD-Slot,
> where you plug the SDIO card into it. No special wiring or something
> like this. So I doubt some external GPIOs are involved.
> I have no idea how the wifi card gets its power, but I hope its over
> some well defined pins within the SD slot...

Can you please specify what was the wlan card you used ? is it a
commercial card or an evaluation board of some sort ?

You said it, but just to be sure - I assume there is no external power
supply involved with that card, right ? it's just plugged into the SD
slot with no extra wire whatsoever ?

Thanks!
Ohad.

> I was able to work with 2.6.35 and .36 plus some out-of-tree patches
> for the sh_mobile_sdhi mfd, which are now in mainline:
> mmc: Allow 2 byte requests in 4-bit mode for tmio_mmc
> tmio_mmc: don't clear unhandled pending interrupts
> tmio_mmc: don't clear unhandled pending interrupts
>
>>> From 6b5691bdd8184cc0876d209c69d38844ea10f678 Mon Sep 17 00:00:00 2001
>>> From: Ohad Ben-Cohen <ohad@wizery.com>
>>> Date: Mon, 1 Nov 2010 09:41:44 +0200
>>> Subject: [PATCH] mmc: add MMC_CAP_RUNTIME_PM
>>>
>>> Some board/card/host configurations are not capable of powering off/on
>>> on the card during runtime.
>>>
>>> To support such configurations, and to allow smoother transition to
>>> runtime PM behavior, MMC_CAP_RUNTIME_PM is added, so hosts need to
>>> explicitly indicate that it's OK to power off their cards after boot.
>>>
>>> This will prevent sdio_bus_probe() from failing to power on the card
>>> when the driver is loaded on such setups.
>>>
>>> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
>>> ---
>>>  drivers/mmc/core/sdio.c     |   37 +++++++++++++++++++++++--------------
>>>  drivers/mmc/core/sdio_bus.c |   33 ++++++++++++++++++++++-----------
>>>  include/linux/mmc/host.h    |    1 +
>>>  3 files changed, 46 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>>> index 6a9ad40..373d56d 100644
>>> --- a/drivers/mmc/core/sdio.c
>>> +++ b/drivers/mmc/core/sdio.c
>>> @@ -547,9 +547,11 @@ static void mmc_sdio_detect(struct mmc_host *host)
>>>        BUG_ON(!host->card);
>>>
>>>        /* Make sure card is powered before detecting it */
>>> -       err = pm_runtime_get_sync(&host->card->dev);
>>> -       if (err < 0)
>>> -               goto out;
>>> +       if (host->caps & MMC_CAP_RUNTIME_PM) {
>>> +               err = pm_runtime_get_sync(&host->card->dev);
>>> +               if (err < 0)
>>> +                       goto out;
>>> +       }
>>>
>>>        mmc_claim_host(host);
>>>
>>> @@ -570,7 +572,8 @@ out:
>>>        }
>>>
>>>        /* Tell PM core that we're done */
>>> -       pm_runtime_put(&host->card->dev);
>>> +       if (host->caps & MMC_CAP_RUNTIME_PM)
>>> +               pm_runtime_put(&host->card->dev);
>>>  }
>>>
>>>  /*
>>> @@ -720,16 +723,21 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
>>>        card = host->card;
>>>
>>>        /*
>>> -        * Let runtime PM core know our card is active
>>> +        * Enable runtime PM only if supported by host+card+board
>>>         */
>>> -       err = pm_runtime_set_active(&card->dev);
>>> -       if (err)
>>> -               goto remove;
>>> +       if (host->caps & MMC_CAP_RUNTIME_PM) {
>>> +               /*
>>> +                * Let runtime PM core know our card is active
>>> +                */
>>> +               err = pm_runtime_set_active(&card->dev);
>>> +               if (err)
>>> +                       goto remove;
>>>
>>> -       /*
>>> -        * Enable runtime PM for this card
>>> -        */
>>> -       pm_runtime_enable(&card->dev);
>>> +               /*
>>> +                * Enable runtime PM for this card
>>> +                */
>>> +               pm_runtime_enable(&card->dev);
>>> +       }
>>>
>>>        /*
>>>         * The number of functions on the card is encoded inside
>>> @@ -747,9 +755,10 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
>>>                        goto remove;
>>>
>>>                /*
>>> -                * Enable Runtime PM for this func
>>> +                * Enable Runtime PM for this func (if supported)
>>>                 */
>>> -               pm_runtime_enable(&card->sdio_func[i]->dev);
>>> +               if (host->caps & MMC_CAP_RUNTIME_PM)
>>> +                       pm_runtime_enable(&card->sdio_func[i]->dev);
>>>        }
>>>
>>>        mmc_release_host(host);
>>> diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
>>> index 2716c7a..f3ce21b 100644
>>> --- a/drivers/mmc/core/sdio_bus.c
>>> +++ b/drivers/mmc/core/sdio_bus.c
>>> @@ -17,6 +17,7 @@
>>>  #include <linux/pm_runtime.h>
>>>
>>>  #include <linux/mmc/card.h>
>>> +#include <linux/mmc/host.h>
>>>  #include <linux/mmc/sdio_func.h>
>>>
>>>  #include "sdio_cis.h"
>>> @@ -132,9 +133,11 @@ static int sdio_bus_probe(struct device *dev)
>>>         * it should call pm_runtime_put_noidle() in its probe routine and
>>>         * pm_runtime_get_noresume() in its remove routine.
>>>         */
>>> -       ret = pm_runtime_get_sync(dev);
>>> -       if (ret < 0)
>>> -               goto out;
>>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM) {
>>> +               ret = pm_runtime_get_sync(dev);
>>> +               if (ret < 0)
>>> +                       goto out;
>>> +       }
>>>
>>>        /* Set the default block size so the driver is sure it's something
>>>         * sensible. */
>>> @@ -151,7 +154,8 @@ static int sdio_bus_probe(struct device *dev)
>>>        return 0;
>>>
>>>  disable_runtimepm:
>>> -       pm_runtime_put_noidle(dev);
>>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
>>> +               pm_runtime_put_noidle(dev);
>>>  out:
>>>        return ret;
>>>  }
>>> @@ -160,12 +164,14 @@ static int sdio_bus_remove(struct device *dev)
>>>  {
>>>        struct sdio_driver *drv = to_sdio_driver(dev->driver);
>>>        struct sdio_func *func = dev_to_sdio_func(dev);
>>> -       int ret;
>>> +       int ret = 0;
>>>
>>>        /* Make sure card is powered before invoking ->remove() */
>>> -       ret = pm_runtime_get_sync(dev);
>>> -       if (ret < 0)
>>> -               goto out;
>>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM) {
>>> +               ret = pm_runtime_get_sync(dev);
>>> +               if (ret < 0)
>>> +                       goto out;
>>> +       }
>>>
>>>        drv->remove(func);
>>>
>>> @@ -178,10 +184,12 @@ static int sdio_bus_remove(struct device *dev)
>>>        }
>>>
>>>        /* First, undo the increment made directly above */
>>> -       pm_runtime_put_noidle(dev);
>>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
>>> +               pm_runtime_put_noidle(dev);
>>>
>>>        /* Then undo the runtime PM settings in sdio_bus_probe() */
>>> -       pm_runtime_put_noidle(dev);
>>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
>>> +               pm_runtime_put_noidle(dev);
>>>
>>>  out:
>>>        return ret;
>>> @@ -191,6 +199,8 @@ out:
>>>
>>>  static int sdio_bus_pm_prepare(struct device *dev)
>>>  {
>>> +       struct sdio_func *func = dev_to_sdio_func(dev);
>>> +
>>>        /*
>>>         * Resume an SDIO device which was suspended at run time at this
>>>         * point, in order to allow standard SDIO suspend/resume paths
>>> @@ -212,7 +222,8 @@ static int sdio_bus_pm_prepare(struct device *dev)
>>>         * since there is little point in failing system suspend if a
>>>         * device can't be resumed.
>>>         */
>>> -       pm_runtime_resume(dev);
>>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
>>> +               pm_runtime_resume(dev);
>>>
>>>        return 0;
>>>  }
>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>> index c3ffad8..e5eee0e 100644
>>> --- a/include/linux/mmc/host.h
>>> +++ b/include/linux/mmc/host.h
>>> @@ -167,6 +167,7 @@ struct mmc_host {
>>>                                                /* DDR mode at 1.8V */
>>>  #define MMC_CAP_1_2V_DDR       (1 << 12)       /* can support */
>>>                                                /* DDR mode at 1.2V */
>>> +#define MMC_CAP_RUNTIME_PM     (1 << 13)       /* Can power off/on in runtime */
>>>
>>>        mmc_pm_flag_t           pm_caps;        /* supported pm features */
>>>
>>> --
>>> 1.7.0.4
>>>
>>>
>
>

Re: MMC runtime PM patches break libertas probe

[Arnd Hannemann]

Am 16.11.2010 21:58, schrieb Ohad Ben-Cohen:
> On Tue, Nov 16, 2010 at 7:17 PM, Arnd Hannemann
> <hannemann@nets.rwth-aachen.de> >> > Its an AP4 (SH7372) evaluation
> board from renesas. It has an SD-Slot,
>   
>> where you plug the SDIO card into it. No special wiring or something
>> like this. So I doubt some external GPIOs are involved.
>> I have no idea how the wifi card gets its power, but I hope its over
>> some well defined pins within the SD slot...
>>     
> Can you please specify what was the wlan card you used ? is it a
> commercial card or an evaluation board of some sort ?
>   
It says: c-guys sd link11b driver is b43.
It's an commercial card.

> You said it, but just to be sure - I assume there is no external power
> supply involved with that card, right ? it's just plugged into the SD
> slot with no extra wire whatsoever ?
>   
Yes, just plugged it in. No extra wire whatsover.

Regards,
Arnd

> Thanks!
> Ohad.
>
>   
>> I was able to work with 2.6.35 and .36 plus some out-of-tree patches
>> for the sh_mobile_sdhi mfd, which are now in mainline:
>> mmc: Allow 2 byte requests in 4-bit mode for tmio_mmc
>> tmio_mmc: don't clear unhandled pending interrupts
>> tmio_mmc: don't clear unhandled pending interrupts
>>
>>     
>>>> From 6b5691bdd8184cc0876d209c69d38844ea10f678 Mon Sep 17 00:00:00 2001
>>>> From: Ohad Ben-Cohen <ohad@wizery.com>
>>>> Date: Mon, 1 Nov 2010 09:41:44 +0200
>>>> Subject: [PATCH] mmc: add MMC_CAP_RUNTIME_PM
>>>>
>>>> Some board/card/host configurations are not capable of powering off/on
>>>> on the card during runtime.
>>>>
>>>> To support such configurations, and to allow smoother transition to
>>>> runtime PM behavior, MMC_CAP_RUNTIME_PM is added, so hosts need to
>>>> explicitly indicate that it's OK to power off their cards after boot.
>>>>
>>>> This will prevent sdio_bus_probe() from failing to power on the card
>>>> when the driver is loaded on such setups.
>>>>
>>>> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
>>>> ---
>>>>  drivers/mmc/core/sdio.c     |   37 +++++++++++++++++++++++--------------
>>>>  drivers/mmc/core/sdio_bus.c |   33 ++++++++++++++++++++++-----------
>>>>  include/linux/mmc/host.h    |    1 +
>>>>  3 files changed, 46 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>>>> index 6a9ad40..373d56d 100644
>>>> --- a/drivers/mmc/core/sdio.c
>>>> +++ b/drivers/mmc/core/sdio.c
>>>> @@ -547,9 +547,11 @@ static void mmc_sdio_detect(struct mmc_host *host)
>>>>        BUG_ON(!host->card);
>>>>
>>>>        /* Make sure card is powered before detecting it */
>>>> -       err = pm_runtime_get_sync(&host->card->dev);
>>>> -       if (err < 0)
>>>> -               goto out;
>>>> +       if (host->caps & MMC_CAP_RUNTIME_PM) {
>>>> +               err = pm_runtime_get_sync(&host->card->dev);
>>>> +               if (err < 0)
>>>> +                       goto out;
>>>> +       }
>>>>
>>>>        mmc_claim_host(host);
>>>>
>>>> @@ -570,7 +572,8 @@ out:
>>>>        }
>>>>
>>>>        /* Tell PM core that we're done */
>>>> -       pm_runtime_put(&host->card->dev);
>>>> +       if (host->caps & MMC_CAP_RUNTIME_PM)
>>>> +               pm_runtime_put(&host->card->dev);
>>>>  }
>>>>
>>>>  /*
>>>> @@ -720,16 +723,21 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
>>>>        card = host->card;
>>>>
>>>>        /*
>>>> -        * Let runtime PM core know our card is active
>>>> +        * Enable runtime PM only if supported by host+card+board
>>>>         */
>>>> -       err = pm_runtime_set_active(&card->dev);
>>>> -       if (err)
>>>> -               goto remove;
>>>> +       if (host->caps & MMC_CAP_RUNTIME_PM) {
>>>> +               /*
>>>> +                * Let runtime PM core know our card is active
>>>> +                */
>>>> +               err = pm_runtime_set_active(&card->dev);
>>>> +               if (err)
>>>> +                       goto remove;
>>>>
>>>> -       /*
>>>> -        * Enable runtime PM for this card
>>>> -        */
>>>> -       pm_runtime_enable(&card->dev);
>>>> +               /*
>>>> +                * Enable runtime PM for this card
>>>> +                */
>>>> +               pm_runtime_enable(&card->dev);
>>>> +       }
>>>>
>>>>        /*
>>>>         * The number of functions on the card is encoded inside
>>>> @@ -747,9 +755,10 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
>>>>                        goto remove;
>>>>
>>>>                /*
>>>> -                * Enable Runtime PM for this func
>>>> +                * Enable Runtime PM for this func (if supported)
>>>>                 */
>>>> -               pm_runtime_enable(&card->sdio_func[i]->dev);
>>>> +               if (host->caps & MMC_CAP_RUNTIME_PM)
>>>> +                       pm_runtime_enable(&card->sdio_func[i]->dev);
>>>>        }
>>>>
>>>>        mmc_release_host(host);
>>>> diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
>>>> index 2716c7a..f3ce21b 100644
>>>> --- a/drivers/mmc/core/sdio_bus.c
>>>> +++ b/drivers/mmc/core/sdio_bus.c
>>>> @@ -17,6 +17,7 @@
>>>>  #include <linux/pm_runtime.h>
>>>>
>>>>  #include <linux/mmc/card.h>
>>>> +#include <linux/mmc/host.h>
>>>>  #include <linux/mmc/sdio_func.h>
>>>>
>>>>  #include "sdio_cis.h"
>>>> @@ -132,9 +133,11 @@ static int sdio_bus_probe(struct device *dev)
>>>>         * it should call pm_runtime_put_noidle() in its probe routine and
>>>>         * pm_runtime_get_noresume() in its remove routine.
>>>>         */
>>>> -       ret = pm_runtime_get_sync(dev);
>>>> -       if (ret < 0)
>>>> -               goto out;
>>>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM) {
>>>> +               ret = pm_runtime_get_sync(dev);
>>>> +               if (ret < 0)
>>>> +                       goto out;
>>>> +       }
>>>>
>>>>        /* Set the default block size so the driver is sure it's something
>>>>         * sensible. */
>>>> @@ -151,7 +154,8 @@ static int sdio_bus_probe(struct device *dev)
>>>>        return 0;
>>>>
>>>>  disable_runtimepm:
>>>> -       pm_runtime_put_noidle(dev);
>>>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
>>>> +               pm_runtime_put_noidle(dev);
>>>>  out:
>>>>        return ret;
>>>>  }
>>>> @@ -160,12 +164,14 @@ static int sdio_bus_remove(struct device *dev)
>>>>  {
>>>>        struct sdio_driver *drv = to_sdio_driver(dev->driver);
>>>>        struct sdio_func *func = dev_to_sdio_func(dev);
>>>> -       int ret;
>>>> +       int ret = 0;
>>>>
>>>>        /* Make sure card is powered before invoking ->remove() */
>>>> -       ret = pm_runtime_get_sync(dev);
>>>> -       if (ret < 0)
>>>> -               goto out;
>>>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM) {
>>>> +               ret = pm_runtime_get_sync(dev);
>>>> +               if (ret < 0)
>>>> +                       goto out;
>>>> +       }
>>>>
>>>>        drv->remove(func);
>>>>
>>>> @@ -178,10 +184,12 @@ static int sdio_bus_remove(struct device *dev)
>>>>        }
>>>>
>>>>        /* First, undo the increment made directly above */
>>>> -       pm_runtime_put_noidle(dev);
>>>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
>>>> +               pm_runtime_put_noidle(dev);
>>>>
>>>>        /* Then undo the runtime PM settings in sdio_bus_probe() */
>>>> -       pm_runtime_put_noidle(dev);
>>>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
>>>> +               pm_runtime_put_noidle(dev);
>>>>
>>>>  out:
>>>>        return ret;
>>>> @@ -191,6 +199,8 @@ out:
>>>>
>>>>  static int sdio_bus_pm_prepare(struct device *dev)
>>>>  {
>>>> +       struct sdio_func *func = dev_to_sdio_func(dev);
>>>> +
>>>>        /*
>>>>         * Resume an SDIO device which was suspended at run time at this
>>>>         * point, in order to allow standard SDIO suspend/resume paths
>>>> @@ -212,7 +222,8 @@ static int sdio_bus_pm_prepare(struct device *dev)
>>>>         * since there is little point in failing system suspend if a
>>>>         * device can't be resumed.
>>>>         */
>>>> -       pm_runtime_resume(dev);
>>>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
>>>> +               pm_runtime_resume(dev);
>>>>
>>>>        return 0;
>>>>  }
>>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>>> index c3ffad8..e5eee0e 100644
>>>> --- a/include/linux/mmc/host.h
>>>> +++ b/include/linux/mmc/host.h
>>>> @@ -167,6 +167,7 @@ struct mmc_host {
>>>>                                                /* DDR mode at 1.8V */
>>>>  #define MMC_CAP_1_2V_DDR       (1 << 12)       /* can support */
>>>>                                                /* DDR mode at 1.2V */
>>>> +#define MMC_CAP_RUNTIME_PM     (1 << 13)       /* Can power off/on in runtime */
>>>>
>>>>        mmc_pm_flag_t           pm_caps;        /* supported pm features */
>>>>
>>>> --
>>>> 1.7.0.4
>>>>
>>>>
>>>>         
>>
>>     
>   

Re: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen]

On Tue, Nov 16, 2010 at 11:16 PM, Arnd Hannemann <arnd@arndnet.de> wrote:
> Yes, just plugged it in. No extra wire whatsover.

I wonder - when you suspend/resume the host, with that card plugged
in, do you see any errors (particularly in the resume phase) ?

After suspend/resume was completed, can you still work with that card
(wlan is still functional) ?

I'm asking because SDIO suspend/resume is very similar to what the new
2.6.37-rc1 SDIO runtime pm code does.

Thanks,
Ohad.

>
> Regards,
> Arnd
>
>> Thanks!
>> Ohad.
>>
>>
>>> I was able to work with 2.6.35 and .36 plus some out-of-tree patches
>>> for the sh_mobile_sdhi mfd, which are now in mainline:
>>> mmc: Allow 2 byte requests in 4-bit mode for tmio_mmc
>>> tmio_mmc: don't clear unhandled pending interrupts
>>> tmio_mmc: don't clear unhandled pending interrupts
>>>
>>>
>>>>> From 6b5691bdd8184cc0876d209c69d38844ea10f678 Mon Sep 17 00:00:00 2001
>>>>> From: Ohad Ben-Cohen <ohad@wizery.com>
>>>>> Date: Mon, 1 Nov 2010 09:41:44 +0200
>>>>> Subject: [PATCH] mmc: add MMC_CAP_RUNTIME_PM
>>>>>
>>>>> Some board/card/host configurations are not capable of powering off/on
>>>>> on the card during runtime.
>>>>>
>>>>> To support such configurations, and to allow smoother transition to
>>>>> runtime PM behavior, MMC_CAP_RUNTIME_PM is added, so hosts need to
>>>>> explicitly indicate that it's OK to power off their cards after boot.
>>>>>
>>>>> This will prevent sdio_bus_probe() from failing to power on the card
>>>>> when the driver is loaded on such setups.
>>>>>
>>>>> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
>>>>> ---
>>>>>  drivers/mmc/core/sdio.c     |   37 +++++++++++++++++++++++--------------
>>>>>  drivers/mmc/core/sdio_bus.c |   33 ++++++++++++++++++++++-----------
>>>>>  include/linux/mmc/host.h    |    1 +
>>>>>  3 files changed, 46 insertions(+), 25 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>>>>> index 6a9ad40..373d56d 100644
>>>>> --- a/drivers/mmc/core/sdio.c
>>>>> +++ b/drivers/mmc/core/sdio.c
>>>>> @@ -547,9 +547,11 @@ static void mmc_sdio_detect(struct mmc_host *host)
>>>>>        BUG_ON(!host->card);
>>>>>
>>>>>        /* Make sure card is powered before detecting it */
>>>>> -       err = pm_runtime_get_sync(&host->card->dev);
>>>>> -       if (err < 0)
>>>>> -               goto out;
>>>>> +       if (host->caps & MMC_CAP_RUNTIME_PM) {
>>>>> +               err = pm_runtime_get_sync(&host->card->dev);
>>>>> +               if (err < 0)
>>>>> +                       goto out;
>>>>> +       }
>>>>>
>>>>>        mmc_claim_host(host);
>>>>>
>>>>> @@ -570,7 +572,8 @@ out:
>>>>>        }
>>>>>
>>>>>        /* Tell PM core that we're done */
>>>>> -       pm_runtime_put(&host->card->dev);
>>>>> +       if (host->caps & MMC_CAP_RUNTIME_PM)
>>>>> +               pm_runtime_put(&host->card->dev);
>>>>>  }
>>>>>
>>>>>  /*
>>>>> @@ -720,16 +723,21 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
>>>>>        card = host->card;
>>>>>
>>>>>        /*
>>>>> -        * Let runtime PM core know our card is active
>>>>> +        * Enable runtime PM only if supported by host+card+board
>>>>>         */
>>>>> -       err = pm_runtime_set_active(&card->dev);
>>>>> -       if (err)
>>>>> -               goto remove;
>>>>> +       if (host->caps & MMC_CAP_RUNTIME_PM) {
>>>>> +               /*
>>>>> +                * Let runtime PM core know our card is active
>>>>> +                */
>>>>> +               err = pm_runtime_set_active(&card->dev);
>>>>> +               if (err)
>>>>> +                       goto remove;
>>>>>
>>>>> -       /*
>>>>> -        * Enable runtime PM for this card
>>>>> -        */
>>>>> -       pm_runtime_enable(&card->dev);
>>>>> +               /*
>>>>> +                * Enable runtime PM for this card
>>>>> +                */
>>>>> +               pm_runtime_enable(&card->dev);
>>>>> +       }
>>>>>
>>>>>        /*
>>>>>         * The number of functions on the card is encoded inside
>>>>> @@ -747,9 +755,10 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
>>>>>                        goto remove;
>>>>>
>>>>>                /*
>>>>> -                * Enable Runtime PM for this func
>>>>> +                * Enable Runtime PM for this func (if supported)
>>>>>                 */
>>>>> -               pm_runtime_enable(&card->sdio_func[i]->dev);
>>>>> +               if (host->caps & MMC_CAP_RUNTIME_PM)
>>>>> +                       pm_runtime_enable(&card->sdio_func[i]->dev);
>>>>>        }
>>>>>
>>>>>        mmc_release_host(host);
>>>>> diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
>>>>> index 2716c7a..f3ce21b 100644
>>>>> --- a/drivers/mmc/core/sdio_bus.c
>>>>> +++ b/drivers/mmc/core/sdio_bus.c
>>>>> @@ -17,6 +17,7 @@
>>>>>  #include <linux/pm_runtime.h>
>>>>>
>>>>>  #include <linux/mmc/card.h>
>>>>> +#include <linux/mmc/host.h>
>>>>>  #include <linux/mmc/sdio_func.h>
>>>>>
>>>>>  #include "sdio_cis.h"
>>>>> @@ -132,9 +133,11 @@ static int sdio_bus_probe(struct device *dev)
>>>>>         * it should call pm_runtime_put_noidle() in its probe routine and
>>>>>         * pm_runtime_get_noresume() in its remove routine.
>>>>>         */
>>>>> -       ret = pm_runtime_get_sync(dev);
>>>>> -       if (ret < 0)
>>>>> -               goto out;
>>>>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM) {
>>>>> +               ret = pm_runtime_get_sync(dev);
>>>>> +               if (ret < 0)
>>>>> +                       goto out;
>>>>> +       }
>>>>>
>>>>>        /* Set the default block size so the driver is sure it's something
>>>>>         * sensible. */
>>>>> @@ -151,7 +154,8 @@ static int sdio_bus_probe(struct device *dev)
>>>>>        return 0;
>>>>>
>>>>>  disable_runtimepm:
>>>>> -       pm_runtime_put_noidle(dev);
>>>>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
>>>>> +               pm_runtime_put_noidle(dev);
>>>>>  out:
>>>>>        return ret;
>>>>>  }
>>>>> @@ -160,12 +164,14 @@ static int sdio_bus_remove(struct device *dev)
>>>>>  {
>>>>>        struct sdio_driver *drv = to_sdio_driver(dev->driver);
>>>>>        struct sdio_func *func = dev_to_sdio_func(dev);
>>>>> -       int ret;
>>>>> +       int ret = 0;
>>>>>
>>>>>        /* Make sure card is powered before invoking ->remove() */
>>>>> -       ret = pm_runtime_get_sync(dev);
>>>>> -       if (ret < 0)
>>>>> -               goto out;
>>>>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM) {
>>>>> +               ret = pm_runtime_get_sync(dev);
>>>>> +               if (ret < 0)
>>>>> +                       goto out;
>>>>> +       }
>>>>>
>>>>>        drv->remove(func);
>>>>>
>>>>> @@ -178,10 +184,12 @@ static int sdio_bus_remove(struct device *dev)
>>>>>        }
>>>>>
>>>>>        /* First, undo the increment made directly above */
>>>>> -       pm_runtime_put_noidle(dev);
>>>>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
>>>>> +               pm_runtime_put_noidle(dev);
>>>>>
>>>>>        /* Then undo the runtime PM settings in sdio_bus_probe() */
>>>>> -       pm_runtime_put_noidle(dev);
>>>>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
>>>>> +               pm_runtime_put_noidle(dev);
>>>>>
>>>>>  out:
>>>>>        return ret;
>>>>> @@ -191,6 +199,8 @@ out:
>>>>>
>>>>>  static int sdio_bus_pm_prepare(struct device *dev)
>>>>>  {
>>>>> +       struct sdio_func *func = dev_to_sdio_func(dev);
>>>>> +
>>>>>        /*
>>>>>         * Resume an SDIO device which was suspended at run time at this
>>>>>         * point, in order to allow standard SDIO suspend/resume paths
>>>>> @@ -212,7 +222,8 @@ static int sdio_bus_pm_prepare(struct device *dev)
>>>>>         * since there is little point in failing system suspend if a
>>>>>         * device can't be resumed.
>>>>>         */
>>>>> -       pm_runtime_resume(dev);
>>>>> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
>>>>> +               pm_runtime_resume(dev);
>>>>>
>>>>>        return 0;
>>>>>  }
>>>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>>>> index c3ffad8..e5eee0e 100644
>>>>> --- a/include/linux/mmc/host.h
>>>>> +++ b/include/linux/mmc/host.h
>>>>> @@ -167,6 +167,7 @@ struct mmc_host {
>>>>>                                                /* DDR mode at 1.8V */
>>>>>  #define MMC_CAP_1_2V_DDR       (1 << 12)       /* can support */
>>>>>                                                /* DDR mode at 1.2V */
>>>>> +#define MMC_CAP_RUNTIME_PM     (1 << 13)       /* Can power off/on in runtime */
>>>>>
>>>>>        mmc_pm_flag_t           pm_caps;        /* supported pm features */
>>>>>
>>>>> --
>>>>> 1.7.0.4
>>>>>
>>>>>
>>>>>
>>>
>>>
>>
>
>

Re: MMC runtime PM patches break libertas probe

[Daniel Drake]

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

Hi Ohad,

On 31 October 2010 19:06, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> OK, as expected.
>
> So to summarize:
> 1. Card is powered up at boot, and successfully initialized
> 2. After mmc + sdio devices are added to the device tree, power is
> (seemingly) taken down by runtime PM
> 3. When the driver is loaded, card is powered up again, but doesn't
> respond to CMD3
>
> The only explanation I can think of why the card doesn't respond to
> CMD3, after its power is brought up again, is that we didn't have a
> full reset here (i.e. mmc_power_off() didn't completely power off
> everything).

I have investigated this again, as we'd like runtime PM to work.

It's certainly possible that there's something weird about the
hardware in question, but we *are* able to successfully power down and
up the card with a hacky rfkill driver that calls mmc_stop_host /
mmc_start_host. So I went around finding out what the difference was
between these functions and the runtime PM implementation.

Through this comparison I think mmc_power_save_host() does almost
exactly the same as mmc_stop_host() (good), but
mmc_power_restore_host() lacks some steps which would otherwise be
taken by mmc_start_host(). These are:

in mmc_rescan_try_freq():

	/*
	 * sdio_reset sends CMD52 to reset card.  Since we do not know
	 * if the card is being re-initialized, just send it.  CMD52
	 * should be ignored by SD/eMMC cards.
	 */
	sdio_reset(host);
	mmc_go_idle(host);

	mmc_send_if_cond(host, host->ocr_avail);


In mmc_attach_sdio():

	err = mmc_send_io_op_cond(host, 0, &ocr);
	if (err)
		return err;

	mmc_attach_bus(host, &mmc_sdio_ops);
	if (host->ocr_avail_sdio)
		host->ocr_avail = host->ocr_avail_sdio;

	/*
	 * Sanity check the voltages that the card claims to
	 * support.
	 */
	if (ocr & 0x7F) {
		printk(KERN_WARNING "%s: card claims to support voltages "
		       "below the defined range. These will be ignored.\n",
		       mmc_hostname(host));
		ocr &= ~0x7F;
	}

	host->ocr = mmc_select_voltage(host, ocr);

	/*
	 * Can we support the voltage(s) of the card(s)?
	 */
	if (!host->ocr) {
		err = -EINVAL;
		goto err;
	}


Should anything in those code snippets be running during runtime PM resume?

I went ahead and ran the obvious test by putting those bits of code in
the runtime PM resume path... the first snippet doesn't seem to
improve or hurt anything, but the second snippet fixes the problem. At
least it means I can boot, the card gets powered down during boot,
then I load the module and it powers up and initialises correctly.
Patch attached for clarity.

Any thoughts?

Thanks,
Daniel

[-- Attachment #2: fix-sdio-power-restore.txt --]
[-- Type: text/plain, Size: 1166 bytes --]

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 4d0c15b..3b06379 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -691,15 +691,47 @@ 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);
+
+	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;
+
+	/*
+	 * Sanity check the voltages that the card claims to
+	 * support.
+	 */
+	if (ocr & 0x7F) {
+		printk(KERN_WARNING "%s: card claims to support voltages "
+		       "below the defined range. These will be ignored.\n",
+		       mmc_hostname(host));
+		ocr &= ~0x7F;
+	}
+
+	host->ocr = mmc_select_voltage(host, ocr);
+
+	/*
+	 * Can we support the voltage(s) of the card(s)?
+	 */
+	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;

Re: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen]

Hi Daniel,

On Sun, May 29, 2011 at 7:21 PM, Daniel Drake <dsd@laptop.org> wrote:
> It's certainly possible that there's something weird about the
> hardware in question, but we *are* able to successfully power down and
> up the card with a hacky rfkill driver that calls mmc_stop_host /
> mmc_start_host.

Are we talking about the XO-1.5 and the sd8686 ?

Last we talked, we found out runtime PM didn't work because the sd8686
required an additional manipulation of an external reset gpio line,
and that the only reason OLPC could power it down/up was this patch:

http://dev.laptop.org/git/olpc-2.6/commit/?h=olpc-2.6.35&id=e9bee721fb0cc303286d1fe5df4930ce79b0b1e0

One of the suggested solutions back then was to use a regulator for this.

Has anything changed since then ? does mmc_stop_host+mmc_start_host
work for you without manipulating that reset gpio ?

>        err = mmc_send_io_op_cond(host, 0, &ocr);
>        if (err)
>                return err;

This part actually makes sense.

While sending a CMD5 arg=0 is not mandatory when initializing embedded
SDIO cards (like the wl12xx is), some cards might still require it,
and it is actually anyway required by the spec for removal cards.

I just tried it briefly with the wl12xx, and things seems ok.

>     printk(KERN_WARNING "%s: card claims to support voltages "
>                      "below the defined range. These will be ignored.\n",
>                      mmc_hostname(host));

Please drop this warning though. It was already displayed when
mmc_attach_sdio() executed, so the user already knows his card has
this thing. With the wl12xx, you'd see this everytime you bring up the
interface, so it's really unnecessarily too noisy.

> Any thoughts?

One other thing to consider is system resume. when wol is disabled,
and your chip is powered off during system suspend, you'd need this
CMD5 arg=0 in the resume path as well.

Some code refactoring should be considered to avoid duplicating this
snippet three times though.

Otherwise, submit :)

Thanks,
Ohad.

Re: MMC runtime PM patches break libertas probe

[Daniel Drake]

On 30 May 2011 07:52, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Sun, May 29, 2011 at 7:21 PM, Daniel Drake <dsd@laptop.org> wrote:
>> It's certainly possible that there's something weird about the
>> hardware in question, but we *are* able to successfully power down and
>> up the card with a hacky rfkill driver that calls mmc_stop_host /
>> mmc_start_host.
>
> Are we talking about the XO-1.5 and the sd8686 ?

Yes.

> Last we talked, we found out runtime PM didn't work because the sd8686
> required an additional manipulation of an external reset gpio line,
> and that the only reason OLPC could power it down/up was this patch:
>
> http://dev.laptop.org/git/olpc-2.6/commit/?h=olpc-2.6.35&id=e9bee721fb0cc303286d1fe5df4930ce79b0b1e0

My further investigation here suggests that this change is not
necessary. It was added in response to a separate (hard-to-reproduce)
issue and it was never known if it would actually fix that issue, it
was more of a guess. We don't have any convincing evidence that it
helps, so it will be dropped in future.

Anyway, just to be sure, I tried combining this hack with runtime PM,
and also as a regulator, and it didn't help anything. runtime PM still
fails to power up the card.

Sorry for leading you down the wrong path there.

> does mmc_stop_host+mmc_start_host
> work for you without manipulating that reset gpio ?

Yes.

...
> Otherwise, submit :)

Thanks for reviewing, I'll go ahead and clean it up.

You didn't comment on the added mmc_select_voltage() call. Is that one
also sensible?

Thanks,
Daniel

Re: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen]

On Mon, May 30, 2011 at 10:01 AM, Daniel Drake <dsd@laptop.org> wrote:
> On 30 May 2011 07:52, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> Last we talked, we found out runtime PM didn't work because the sd8686
>> required an additional manipulation of an external reset gpio line,
>> and that the only reason OLPC could power it down/up was this patch:
>>
>> http://dev.laptop.org/git/olpc-2.6/commit/?h=olpc-2.6.35&id=e9bee721fb0cc303286d1fe5df4930ce79b0b1e0
>
> My further investigation here suggests that this change is not
> necessary. It was added in response to a separate (hard-to-reproduce)
> issue and it was never known if it would actually fix that issue, it
> was more of a guess. We don't have any convincing evidence that it
> helps, so it will be dropped in future.
>
> Anyway, just to be sure, I tried combining this hack with runtime PM,
> and also as a regulator, and it didn't help anything. runtime PM still
> fails to power up the card.
>
> Sorry for leading you down the wrong path there.
...
>> does mmc_stop_host+mmc_start_host
>> work for you without manipulating that reset gpio ?
>
> Yes.

Hm. I still don't entirely get it, because we had others (Mike, cc'd)
saying too that the sd8686 requires manipulating an external reset
gpio after bringing the power back up.

Maybe someone from Marvell can comment on this (cc'ing Zhangfei Gao) ?

> You didn't comment on the added mmc_select_voltage() call. Is that one
> also sensible?

I guess. if we're reading the I/O OCR, might as well use it. This way
our runtime PM power up path will also be identical to the one induced
by mmc_attach_sdio.

Re: MMC runtime PM patches break libertas probe

[zhangfei gao]

On Mon, May 30, 2011 at 3:32 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Mon, May 30, 2011 at 10:01 AM, Daniel Drake <dsd@laptop.org> wrote:
>> On 30 May 2011 07:52, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>>> Last we talked, we found out runtime PM didn't work because the sd8686
>>> required an additional manipulation of an external reset gpio line,
>>> and that the only reason OLPC could power it down/up was this patch:
>>>
>>> http://dev.laptop.org/git/olpc-2.6/commit/?h=olpc-2.6.35&id=e9bee721fb0cc303286d1fe5df4930ce79b0b1e0
>>
>> My further investigation here suggests that this change is not
>> necessary. It was added in response to a separate (hard-to-reproduce)
>> issue and it was never known if it would actually fix that issue, it
>> was more of a guess. We don't have any convincing evidence that it
>> helps, so it will be dropped in future.
>>
>> Anyway, just to be sure, I tried combining this hack with runtime PM,
>> and also as a regulator, and it didn't help anything. runtime PM still
>> fails to power up the card.
>>
>> Sorry for leading you down the wrong path there.
> ...
>>> does mmc_stop_host+mmc_start_host
>>> work for you without manipulating that reset gpio ?
>>
>> Yes.
>
> Hm. I still don't entirely get it, because we had others (Mike, cc'd)
> saying too that the sd8686 requires manipulating an external reset
> gpio after bringing the power back up.

sd8686 requires two pins to control power, PDn and RESETn.
To enable sd8686, PDn and RESETn should both set high.
To disable sd8686, PDn should be set low.
We also have plans to use runtime PM to optimize these two pins,
suggested by Ohad before.
However currently we still use rfkill method to control the two pins.
In the stress test, we found wlan card may not be probed successfully
even the two pins are set correctly, so we manually detect whether
mmc->card is allocated or not, not sure whether runtime PM to handle
this case.

>
> Maybe someone from Marvell can comment on this (cc'ing Zhangfei Gao) ?
>
>> You didn't comment on the added mmc_select_voltage() call. Is that one
>> also sensible?
>
> I guess. if we're reading the I/O OCR, might as well use it. This way
> our runtime PM power up path will also be identical to the one induced
> by mmc_attach_sdio.
> --
> 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: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen]

Hi Zhangfei, Bing,

On Mon, May 30, 2011 at 2:04 PM, zhangfei gao <zhangfei.gao@gmail.com> wrote:
> sd8686 requires two pins to control power, PDn and RESETn.
> To enable sd8686, PDn and RESETn should both set high.
> To disable sd8686, PDn should be set low.

If we power down the sd8686, and power it up again, without toggling
the reset pin at all (e.g. keep it always high), will the card work ?

Thanks,
Ohad.

RE: MMC runtime PM patches break libertas probe

[Bing Zhao]

Hi Ohad,

> If we power down the sd8686, and power it up again, without toggling
> the reset pin at all (e.g. keep it always high), will the card work ?

Yes. The card should work without toggling the reset pin.

Regards,
Bing

Re: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen]

On Thu, Jun 2, 2011 at 11:39 AM, Bing Zhao <bzhao@marvell.com> wrote:
>> If we power down the sd8686, and power it up again, without toggling
>> the reset pin at all (e.g. keep it always high), will the card work ?
>
> Yes. The card should work without toggling the reset pin.

Thanks.

Just for closure-sake, can you please confirm that the sd8686 requires
sending a CMD5 arg=0 as part of the initialization sequence after
powering it on ?

(we'd probably add it anyway, but it'd be nice to get a confirmation
about this from you guys)

Btw, it will be nice to allow cards to avoid sending this if not
required; a simple card quirk will do. wl12xx will definitely use such
a quirk.

RE: MMC runtime PM patches break libertas probe

[Bing Zhao]

> On Thu, Jun 2, 2011 at 11:39 AM, Bing Zhao <bzhao@marvell.com> wrote:
>>> If we power down the sd8686, and power it up again, without toggling
>>> the reset pin at all (e.g. keep it always high), will the card work ?
>>
>> Yes. The card should work without toggling the reset pin.
> 
> Thanks.
> 
> Just for closure-sake, can you please confirm that the sd8686 requires
> sending a CMD5 arg=0 as part of the initialization sequence after
> powering it on ?

"CMD5 Arg=0" refers to the very first CMD5 sent from host during initialization sequence.
This is required because our state machine always expects two CMD5 from host (5, 5, 3, 7, ...).

Regards,
Bing

> (we'd probably add it anyway, but it'd be nice to get a confirmation
> about this from you guys)
> 
> Btw, it will be nice to allow cards to avoid sending this if not
> required; a simple card quirk will do. wl12xx will definitely use such
> a quirk.

Re: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen]

(cc'ing Arnd Hannermann)

On Sat, Jun 4, 2011 at 1:28 AM, Bing Zhao <bzhao@marvell.com> wrote:
> "CMD5 Arg=0" refers to the very first CMD5 sent from host during initialization sequence.
> This is required because our state machine always expects two CMD5 from host (5, 5, 3, 7, ...).

Great, thanks for confirming this !

Arnd, you have reported SDIO runtime pm issues with the b43 in the
past. If you're still interested and have some free cycles, you might
want to check out Daniel's patch and see if that fixes the issues you
had too. With Daniel's patch we're now following the spec more
strictly, and the change is particularly relevant for removable SDIO
cards like the one you were using.

Daniel, please go ahead and submit your patch when you're ready.

Thanks,
Ohad.

Re: MMC runtime PM patches break libertas probe

[Arnd Hannemann]

Hi Ohad,

Am 04.06.2011 00:52, schrieb Ohad Ben-Cohen:
> (cc'ing Arnd Hannermann)
> 
> On Sat, Jun 4, 2011 at 1:28 AM, Bing Zhao <bzhao@marvell.com> wrote:
>> "CMD5 Arg=0" refers to the very first CMD5 sent from host during initialization sequence.
>> This is required because our state machine always expects two CMD5 from host (5, 5, 3, 7, ...).
> 
> Great, thanks for confirming this !
> 
> Arnd, you have reported SDIO runtime pm issues with the b43 in the
> past. If you're still interested and have some free cycles, you might
> want to check out Daniel's patch and see if that fixes the issues you
> had too. With Daniel's patch we're now following the spec more
> strictly, and the change is particularly relevant for removable SDIO
> cards like the one you were using.

Sorry, I don't have the hardware anymore so I'm not able to check this.

Best regards
Arnd

Re: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen]

On Tue, Jun 7, 2011 at 5:34 PM, Arnd Hannemann <arnd@arndnet.de> wrote:
> Sorry, I don't have the hardware anymore so I'm not able to check this.

Ok, thanks for the reply !

Re: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen]

Hi Bing,

On Sat, Jun 4, 2011 at 1:52 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Sat, Jun 4, 2011 at 1:28 AM, Bing Zhao <bzhao@marvell.com> wrote:
>> "CMD5 Arg=0" refers to the very first CMD5 sent from host during initialization sequence.
>> This is required because our state machine always expects two CMD5 from host (5, 5, 3, 7, ...).
>
> Great, thanks for confirming this !

I have another question please.

Does the sd8686 require an SDIO I/O reset (CMD52 setting bit 3 of
address 0x6 of the CCCR) to it after powering it on ?

Daniel (cc'ed) is trying to power it off and back on, and it does seem
to work in the first time, without sending a reset. In the second
time, though, the card doesn't answer CMD5 anymore, unless Daniel is
sending it an SDIO I/O reset. I was wondering whether this is an
sd8686 requirement, or whether we have some other issue at hand.

Thanks,
Ohad.

Re: MMC runtime PM patches break libertas probe

[zhangfei gao]

On Wed, Jun 8, 2011 at 10:34 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Hi Bing,
>
> On Sat, Jun 4, 2011 at 1:52 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> On Sat, Jun 4, 2011 at 1:28 AM, Bing Zhao <bzhao@marvell.com> wrote:
>>> "CMD5 Arg=0" refers to the very first CMD5 sent from host during initialization sequence.
>>> This is required because our state machine always expects two CMD5 from host (5, 5, 3, 7, ...).
>>
>> Great, thanks for confirming this !
>
> I have another question please.
>
> Does the sd8686 require an SDIO I/O reset (CMD52 setting bit 3 of
> address 0x6 of the CCCR) to it after powering it on ?
>
> Daniel (cc'ed) is trying to power it off and back on, and it does seem
> to work in the first time, without sending a reset. In the second
> time, though, the card doesn't answer CMD5 anymore, unless Daniel is
> sending it an SDIO I/O reset. I was wondering whether this is an
> sd8686 requirement, or whether we have some other issue at hand.

Hi, Ohad

Here is answer got from the sd8686 maintainer.

For 8686, the SDIO state machine can only handle init sequence (CMD5,
5, 3, 7) from host once. If host sends another init sequence, it will
not be able to handle CMD5 and causes the SDIO block to hang. Chips
that are newer than 8686 will be able to handle multiple init sequence
from host.

So yes, for 8686, an IO reset is needed before host can send a new set
of init sequence.


>
> Thanks,
> Ohad.
>

Re: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen]

Hi Zhangfei,

On Fri, Jun 10, 2011 at 5:02 AM, zhangfei gao <zhangfei.gao@gmail.com> wrote:
> Here is answer got from the sd8686 maintainer.
>
> For 8686, the SDIO state machine can only handle init sequence (CMD5,
> 5, 3, 7) from host once. If host sends another init sequence, it will
> not be able to handle CMD5 and causes the SDIO block to hang. Chips
> that are newer than 8686 will be able to handle multiple init sequence
> from host.

Thanks for the reply !

> So yes, for 8686, an IO reset is needed before host can send a new set
> of init sequence.

But if we're powering down and up the device first, then the init
sequence is considered the first one, and then we don't need an IO
reset, right ? That was what we wondered about.

Thanks,
Ohad.

Re: MMC runtime PM patches break libertas probe

[zhangfei gao]

On Fri, Jun 10, 2011 at 12:28 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Hi Zhangfei,
>
> On Fri, Jun 10, 2011 at 5:02 AM, zhangfei gao <zhangfei.gao@gmail.com> wrote:
>> Here is answer got from the sd8686 maintainer.
>>
>> For 8686, the SDIO state machine can only handle init sequence (CMD5,
>> 5, 3, 7) from host once. If host sends another init sequence, it will
>> not be able to handle CMD5 and causes the SDIO block to hang. Chips
>> that are newer than 8686 will be able to handle multiple init sequence
>> from host.
>
> Thanks for the reply !
>
>> So yes, for 8686, an IO reset is needed before host can send a new set
>> of init sequence.
>
> But if we're powering down and up the device first, then the init
> sequence is considered the first one, and then we don't need an IO
> reset, right ? That was what we wondered about.

Hi Ohad,

If you power down and up the device, then IO reset is not needed and
8686 can process host init sequence correctly.

CC Benson.

>
> Thanks,
> Ohad.
>

Re: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen]

On Sat, Jun 11, 2011 at 5:33 AM, zhangfei gao <zhangfei.gao@gmail.com> wrote:
> If you power down and up the device, then IO reset is not needed and
> 8686 can process host init sequence correctly.

Thanks, that's what I thought.

Daniel, this ultimately means that whenever sending a reset is
required to re-init the 8686, we can surely say that the chip wasn't
really powered off beforehand, and that something went wrong in the
software, leading us to think the chip is powered off when it is
really not.

But I think we also demonstrated this with the simple
insmod/rmmod/insmod scenario, where every insmod successfully
re-initialized the chip without sending an sdio reset.

Thanks,
Ohad.