Please find a simple sdhci runtime_pm implementation
that uses clock gating fw as a tip to know when
our chip is idle.
Previous version was RFC and untested,
This version has been tested on intel medfield platform.
Pierre Tardy (2):
mmc: put the led blinking code after clock ungating
sdhci:v2:use ios->clock to know when sdhci is idle
Yunpeng Gao (1):
sdhci-pci : Enable runtime PM support
drivers/mmc/core/core.c | 3 +-
drivers/mmc/host/sdhci-pci.c | 124 ++++++++++++++++++++++++++++++++++++++++++
drivers/mmc/host/sdhci.c | 60 ++++++++++++++++++++
drivers/mmc/host/sdhci.h | 5 ++
include/linux/mmc/sdhci.h | 1 +
5 files changed, 191 insertions(+), 2 deletions(-)
--
1.7.2.3
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris,
92196 Meudon Cedex, France
Registration Number: 302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
as mmc clock gating can also be used as a power gating
tip, this is better to put the led blinking after having
ungate the clock
Signed-off-by: Pierre Tardy <[email protected]>
---
drivers/mmc/core/core.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 6625c05..34a7e8c 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -167,8 +167,6 @@ mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
WARN_ON(!host->claimed);
- led_trigger_event(host->led, LED_FULL);
-
mrq->cmd->error = 0;
mrq->cmd->mrq = mrq;
if (mrq->data) {
@@ -194,6 +192,7 @@ mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
}
}
mmc_host_clk_ungate(host);
+ led_trigger_event(host->led, LED_FULL);
host->ops->request(host, mrq);
}
--
1.7.2.3
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris,
92196 Meudon Cedex, France
Registration Number: 302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
From: Yunpeng Gao <[email protected]>
Follow the kernel runtime PM framework, enable runtime PM support of the
sdhci host controller with pci interface.
Note that this patch implements runtime_pm but now actually detects
activity.
It relies on higher level (childrens) to do actual waking up
Activity detection is put in following patch
Original version from: Yunpeng Gao <[email protected]>
with modifications by: Pierre Tardy <[email protected]>
Signed-off-by: Pierre Tardy <[email protected]>
---
drivers/mmc/host/sdhci-pci.c | 121 ++++++++++++++++++++++++++++++++++++++++++
drivers/mmc/host/sdhci.c | 31 +++++++++++
drivers/mmc/host/sdhci.h | 5 ++
3 files changed, 157 insertions(+), 0 deletions(-)
diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 0dc905b..22581a1 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -1027,6 +1027,8 @@ static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
if (ret)
return ret;
+ pm_runtime_set_active(&pdev->dev);
+
slots = PCI_SLOT_INFO_SLOTS(slots) + 1;
dev_dbg(&pdev->dev, "found %d slot(s)\n", slots);
if (slots == 0)
@@ -1082,6 +1084,8 @@ static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
chip->slots[i] = slot;
}
+ pm_runtime_enable(&pdev->dev);
+ pm_runtime_allow(&pdev->dev);
return 0;
@@ -1110,8 +1114,122 @@ static void __devexit sdhci_pci_remove(struct pci_dev *pdev)
}
pci_disable_device(pdev);
+
+ pm_runtime_forbid(&pdev->dev);
+ pm_runtime_disable(&pdev->dev);
+
+}
+
+#ifdef CONFIG_PM_RUNTIME
+
+static int sdhci_pci_runtime_suspend(struct device *dev)
+{
+ struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
+ struct sdhci_pci_chip *chip;
+ struct sdhci_pci_slot *slot;
+ int i, ret;
+ mmc_pm_flag_t pm_flags = 0;
+ pm_message_t state;
+
+ chip = pci_get_drvdata(pdev);
+ if (!chip)
+ return 0;
+
+ for (i = 0; i < chip->num_slots; i++) {
+ slot = chip->slots[i];
+ if (!slot)
+ continue;
+
+ ret = sdhci_runtime_suspend(slot->host);
+
+ if (ret) {
+ for (i--; i >= 0; i--)
+ sdhci_runtime_resume(chip->slots[i]->host);
+ return ret;
+ }
+
+ pm_flags |= slot->host->mmc->pm_flags;
+ }
+
+ state.event = PM_EVENT_AUTO_SUSPEND;
+ if (chip->fixes && chip->fixes->suspend) {
+ ret = chip->fixes->suspend(chip, state);
+ if (ret) {
+ for (i = chip->num_slots - 1; i >= 0; i--)
+ sdhci_runtime_resume(chip->slots[i]->host);
+ return ret;
+ }
+ }
+
+ pci_save_state(pdev);
+ if (pm_flags & MMC_PM_KEEP_POWER) {
+ if (pm_flags & MMC_PM_WAKE_SDIO_IRQ)
+ pci_enable_wake(pdev, PCI_D3hot, 1);
+ } else {
+ pci_enable_wake(pdev, PCI_D3hot, 0);
+ pci_disable_device(pdev);
+ }
+ pci_set_power_state(pdev, PCI_D3hot);
+
+ return 0;
}
+static int sdhci_pci_runtime_resume(struct device *dev)
+{
+ struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
+ struct sdhci_pci_chip *chip;
+ struct sdhci_pci_slot *slot;
+ int i, ret;
+
+ chip = pci_get_drvdata(pdev);
+ if (!chip)
+ return 0;
+
+ pci_set_power_state(pdev, PCI_D0);
+ pci_restore_state(pdev);
+ ret = pci_enable_device(pdev);
+ if (ret)
+ return ret;
+
+ if (chip->fixes && chip->fixes->resume) {
+ ret = chip->fixes->resume(chip);
+ if (ret)
+ return ret;
+ }
+
+ for (i = 0; i < chip->num_slots; i++) {
+ slot = chip->slots[i];
+ if (!slot)
+ continue;
+
+ ret = sdhci_runtime_resume(slot->host);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int sdhci_pci_runtime_idle(struct device *dev)
+{
+ pm_runtime_autosuspend(dev);
+ return -EAGAIN;
+}
+
+#else
+
+#define sdhci_pci_runtime_suspend NULL
+#define sdhci_pci_runtime_resume NULL
+#define sdhci_pci_runtime_idle NULL
+
+#endif
+
+static const struct dev_pm_ops sdhci_pci_pm_ops = {
+ .runtime_suspend = sdhci_pci_runtime_suspend,
+ .runtime_resume = sdhci_pci_runtime_resume,
+ .runtime_idle = sdhci_pci_runtime_idle,
+};
+
static struct pci_driver sdhci_driver = {
.name = "sdhci-pci",
.id_table = pci_ids,
@@ -1119,6 +1237,9 @@ static struct pci_driver sdhci_driver = {
.remove = __devexit_p(sdhci_pci_remove),
.suspend = sdhci_pci_suspend,
.resume = sdhci_pci_resume,
+ .driver = {
+ .pm = &sdhci_pci_pm_ops
+ },
};
/*****************************************************************************\
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 9e15f41..3e65d94 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1715,6 +1715,37 @@ EXPORT_SYMBOL_GPL(sdhci_enable_irq_wakeups);
#endif /* CONFIG_PM */
+#ifdef CONFIG_PM_RUNTIME
+
+int sdhci_runtime_suspend(struct sdhci_host *host)
+{
+ /* Nothing to do yet. Still leave the placeholder */
+ return 0;
+}
+
+EXPORT_SYMBOL_GPL(sdhci_runtime_suspend);
+
+int sdhci_runtime_resume(struct sdhci_host *host)
+{
+ int ret = 0;
+
+ if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA)) {
+ if (host->ops->enable_dma)
+ host->ops->enable_dma(host);
+ }
+
+ sdhci_init(host, (host->mmc->pm_flags & MMC_PM_KEEP_POWER));
+ host->pwr = 0; /* force power reprogram */
+ host->clock = 0; /* force clock reprogram */
+ sdhci_set_ios(host->mmc, &host->mmc->ios);
+ mmiowb();
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(sdhci_runtime_resume);
+
+#endif /* CONFIG_PM_RUNTIME */
+
/*****************************************************************************\
* *
* Device allocation/registration *
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 6e0969e..1f032c0 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -327,4 +327,9 @@ extern int sdhci_resume_host(struct sdhci_host *host);
extern void sdhci_enable_irq_wakeups(struct sdhci_host *host);
#endif
+#ifdef CONFIG_PM_RUNTIME
+extern int sdhci_runtime_suspend(struct sdhci_host *host);
+extern int sdhci_runtime_resume(struct sdhci_host *host);
+#endif
+
#endif /* __SDHCI_HW_H */
--
1.7.2.3
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris,
92196 Meudon Cedex, France
Registration Number: 302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
From: Pierre Tardy <[email protected]>
This allows sdhci to detect its own activity and to autosuspend
when not used
inspired from mmci: handle clock frequency 0 properly
>From Linus Walleij <[email protected]>
author of mmc aggressive clock gating fw.
The idea of using mmc clock gating fw in order to power gate the
sdhci is simple (it still need some other get/set because set_ios() is
not cause first before any other acticity):
Whenever the mmc fw tells we dont need the MMC clock, we dont need
the sdhci power as well.
This does not mean that the child (card) is
suspended. In case of a Wifi SDIO card, the card will be suspended
and resumed according to the ifconfig up/down status.
Even if the Wifi interface is up, user might not use the network.
Sdhci can be powered off during those period. It is up to the HW
implementation to implement smart enough power gating to still
support enough always-on circuitry allowing to detect sdio
interrupts.
Acked-by: Linus Walleij <[email protected]>
Signed-off-by: Pierre Tardy <[email protected]>
---
drivers/mmc/host/sdhci-pci.c | 3 +++
drivers/mmc/host/sdhci.c | 29 +++++++++++++++++++++++++++++
include/linux/mmc/sdhci.h | 1 +
3 files changed, 33 insertions(+), 0 deletions(-)
diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 22581a1..9d23f4c 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -1086,6 +1086,9 @@ static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
}
pm_runtime_enable(&pdev->dev);
pm_runtime_allow(&pdev->dev);
+ pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
+ pm_runtime_use_autosuspend(&pdev->dev);
+ pm_suspend_ignore_children(&pdev->dev, 1);
return 0;
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 3e65d94..655617c 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -20,6 +20,7 @@
#include <linux/slab.h>
#include <linux/scatterlist.h>
#include <linux/regulator/consumer.h>
+#include <linux/pm_runtime.h>
#include <linux/leds.h>
@@ -1161,6 +1162,7 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
{
struct sdhci_host *host;
unsigned long flags;
+ unsigned int lastclock;
u8 ctrl;
host = mmc_priv(mmc);
@@ -1171,6 +1173,27 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
goto out;
/*
+ * get/put runtime_pm usage counter at ios->clock transitions
+ * We need to do it before any other chip access, as sdhci could
+ * be power gated
+ */
+ lastclock = host->iosclock;
+ host->iosclock = ios->clock;
+ if (lastclock == 0 && ios->clock != 0) {
+ spin_unlock_irqrestore(&host->lock, flags);
+ pm_runtime_get_sync(host->mmc->parent);
+ spin_lock_irqsave(&host->lock, flags);
+ } else if (lastclock != 0 && ios->clock == 0) {
+ spin_unlock_irqrestore(&host->lock, flags);
+ pm_runtime_mark_last_busy(host->mmc->parent);
+ pm_runtime_put_autosuspend(host->mmc->parent);
+ spin_lock_irqsave(&host->lock, flags);
+ }
+ /* no need to configure the rest.. */
+ if (host->iosclock == 0)
+ goto out;
+
+ /*
* Reset the chip on each power off.
* Should clear out any weird states.
*/
@@ -1244,6 +1267,8 @@ static int sdhci_get_ro(struct mmc_host *mmc)
int is_readonly;
host = mmc_priv(mmc);
+ /* this function is called before set_ios... */
+ pm_runtime_get_sync(mmc->parent);
spin_lock_irqsave(&host->lock, flags);
@@ -1257,6 +1282,7 @@ static int sdhci_get_ro(struct mmc_host *mmc)
spin_unlock_irqrestore(&host->lock, flags);
+ pm_runtime_put_autosuspend(mmc->parent);
/* This quirk needs to be replaced by a callback-function later */
return host->quirks & SDHCI_QUIRK_INVERTED_WRITE_PROTECT ?
!is_readonly : is_readonly;
@@ -1268,6 +1294,7 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
unsigned long flags;
host = mmc_priv(mmc);
+ pm_runtime_get_sync(mmc->parent);
spin_lock_irqsave(&host->lock, flags);
@@ -1282,6 +1309,7 @@ out:
mmiowb();
spin_unlock_irqrestore(&host->lock, flags);
+ pm_runtime_put_autosuspend(mmc->parent);
}
static const struct mmc_host_ops sdhci_ops = {
@@ -1766,6 +1794,7 @@ struct sdhci_host *sdhci_alloc_host(struct device *dev,
host = mmc_priv(mmc);
host->mmc = mmc;
+ host->iosclock = 0;
return host;
}
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index 83bd9f7..a38d040 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -116,6 +116,7 @@ struct sdhci_host {
unsigned int timeout_clk; /* Timeout freq (KHz) */
unsigned int clock; /* Current clock (MHz) */
+ unsigned int iosclock; /* Last clock asked via set_ios */
u8 pwr; /* Current voltage */
struct mmc_request *mrq; /* Current request */
--
1.7.2.3
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris,
92196 Meudon Cedex, France
Registration Number: 302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
2011/2/6 Pierre Tardy <[email protected]>:
> Please find a simple sdhci runtime_pm implementation
> that uses clock gating fw as a tip to know when
> our chip is idle.
I like this approach, the gated clock hints to rpm that it's time
to idle the device. Great work.
Acked-by: Linus Walleij <[email protected]>
For all patches.
Thanks,
Linus Walleij
Hi Pierre,
On Sun, Feb 06, 2011 at 07:02:47PM +0100, Pierre Tardy wrote:
> Please find a simple sdhci runtime_pm implementation
> that uses clock gating fw as a tip to know when
> our chip is idle.
>
> Previous version was RFC and untested,
> This version has been tested on intel medfield platform.
>
> Pierre Tardy (2):
> mmc: put the led blinking code after clock ungating
> sdhci:v2:use ios->clock to know when sdhci is idle
>
> Yunpeng Gao (1):
> sdhci-pci : Enable runtime PM support
Thanks very much, pushed to mmc-next with Linus W's ACK.
--
Chris Ball <[email protected]> <http://printf.net/>
One Laptop Per Child
Hi,
On Sun, Feb 06, 2011 at 07:02:49PM +0100, Pierre Tardy wrote:
> From: Yunpeng Gao <[email protected]>
>
> Follow the kernel runtime PM framework, enable runtime PM support of the
> sdhci host controller with pci interface.
>
> Note that this patch implements runtime_pm but now actually detects
> activity.
> It relies on higher level (childrens) to do actual waking up
> Activity detection is put in following patch
>
> Original version from: Yunpeng Gao <[email protected]>
> with modifications by: Pierre Tardy <[email protected]>
>
> Signed-off-by: Pierre Tardy <[email protected]>
I had to add a:
diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 22581a1..48061b3 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -18,6 +18,7 @@
#include <linux/dma-mapping.h>
#include <linux/slab.h>
#include <linux/device.h>
+#include <linux/pm_runtime.h>
#include <linux/mmc/host.h>
to prevent a compile error.
--
Chris Ball <[email protected]> <http://printf.net/>
One Laptop Per Child
Hi Pierre,
On Sun, Feb 06, 2011 at 07:02:49PM +0100, Pierre Tardy wrote:
> From: Yunpeng Gao <[email protected]>
>
> Follow the kernel runtime PM framework, enable runtime PM support of the
> sdhci host controller with pci interface.
>
> Note that this patch implements runtime_pm but now actually detects
> activity.
> It relies on higher level (childrens) to do actual waking up
> Activity detection is put in following patch
Testing this patchset in linux-next-20100208 gives:
[ 10.829223] sdhci: Secure Digital Host Controller Interface driver
[ 10.829548] sdhci: Copyright(c) Pierre Ossman
[ 10.883906] sdhci-pci 0000:0d:00.0: SDHCI controller found [1180:e822] (rev 1)
[ 10.884533] sdhci-pci 0000:0d:00.0: PCI INT A -> GSI 16 (level, low) -> IRQ 16
[ 10.885189] sdhci-pci 0000:0d:00.0: Will use DMA mode even though HW doesn't fully claim to support it.
[ 10.886176] sdhci-pci 0000:0d:00.0: setting latency timer to 64
[ 10.886819] Registered led device: mmc0::
[ 10.887281] mmc0: SDHCI controller on PCI [0000:0d:00.0] using DMA
[ 10.887724] sdhci-pci 0000:0d:00.0: Unbalanced pm_runtime_enable!
[ 10.888498] sdhci-pci 0000:17:00.0: SDHCI controller found [1180:e822] (rev 1)
[ 10.889179] sdhci-pci 0000:17:00.0: PCI INT A -> GSI 19 (level, low) -> IRQ 19
[ 10.889876] sdhci-pci 0000:17:00.0: Will use DMA mode even though HW doesn't fully claim to support it.
[ 10.890924] sdhci-pci 0000:17:00.0: setting latency timer to 64
[ 10.891387] Registered led device: mmc1::
[ 10.891900] mmc1: SDHCI controller on PCI [0000:17:00.0] using DMA
[ 10.892280] sdhci-pci 0000:17:00.0: Unbalanced pm_runtime_enable!
(Note the last line above.)
I then removed the card and reinserted it, but there's no dmesg output
related to the reinsert, only to the card removal:
[ 65.381047] mmc0: card d555 removed
So, card insertion is broken. I then did rmmod sdhci-pci && modprobe
sdhci-pci, and it picked up the card again.
There was also a weird UI hang for a few hundred msecs around the time
the second modprobe happened, but I don't expect you to be able to do
anything about that without a more helpful report; if it happens again
I'll try to record what's going on using ftrace.
Full dmesg is at:
http://dev.laptop.org/~cjb/logs/runtime-pm-sdhci-20100209.log
Thanks,
--
Chris Ball <[email protected]> <http://printf.net/>
One Laptop Per Child
> On Sun, Feb 06, 2011 at 07:02:49PM +0100, Pierre Tardy wrote:
>> From: Yunpeng Gao <[email protected]>
>>
>> Follow the kernel runtime PM framework, enable runtime PM support of the
>> sdhci host controller with pci interface.
>>
>> Note that this patch implements runtime_pm but now actually detects
>> activity.
>> It relies on higher level (childrens) to do actual waking up
>> Activity detection is put in following patch
>
> Testing this patchset in linux-next-20100208 gives:
>
> [ ? 10.829223] sdhci: Secure Digital Host Controller Interface driver
> [ ? 10.829548] sdhci: Copyright(c) Pierre Ossman
> [ ? 10.883906] sdhci-pci 0000:0d:00.0: SDHCI controller found [1180:e822] (rev 1)
> [ ? 10.884533] sdhci-pci 0000:0d:00.0: PCI INT A -> GSI 16 (level, low) -> IRQ 16
> [ ? 10.885189] sdhci-pci 0000:0d:00.0: Will use DMA mode even though HW doesn't fully claim to support it.
> [ ? 10.886176] sdhci-pci 0000:0d:00.0: setting latency timer to 64
> [ ? 10.886819] Registered led device: mmc0::
> [ ? 10.887281] mmc0: SDHCI controller on PCI [0000:0d:00.0] using DMA
> [ ? 10.887724] sdhci-pci 0000:0d:00.0: Unbalanced pm_runtime_enable!
> [ ? 10.888498] sdhci-pci 0000:17:00.0: SDHCI controller found [1180:e822] (rev 1)
> [ ? 10.889179] sdhci-pci 0000:17:00.0: PCI INT A -> GSI 19 (level, low) -> IRQ 19
> [ ? 10.889876] sdhci-pci 0000:17:00.0: Will use DMA mode even though HW doesn't fully claim to support it.
> [ ? 10.890924] sdhci-pci 0000:17:00.0: setting latency timer to 64
> [ ? 10.891387] Registered led device: mmc1::
> [ ? 10.891900] mmc1: SDHCI controller on PCI [0000:17:00.0] using DMA
> [ ? 10.892280] sdhci-pci 0000:17:00.0: Unbalanced pm_runtime_enable!
>
> (Note the last line above.)
I never have seen this before. I'm not testing those patches on
mmc-next, as our platform is not totally upstream yet, and do not boot
on 2.6.37+ based kernel.
However, we backported the pm_runtime patches from 2.6.37. Is there
some differences in very latest patches on default enablement of
runtime_pm?
>
> I then removed the card and reinserted it, but there's no dmesg output
> related to the reinsert, only to the card removal:
>
> [ ? 65.381047] mmc0: card d555 removed
>
> So, card insertion is broken. ?I then did rmmod sdhci-pci && modprobe
> sdhci-pci, and it picked up the card again.
So what you have here is non working card-detection when device is in PCI_D3.
This sounds like a HW limitation in you platform.
D3 is originally meant for suspend to ram, so it sounds logic that
some HW wont support wake on card detect.
it sounds like we need a SDHCI_RUNTIME_PM_CAP, so that we can properly
describe which HW can be safely put D3 at runtime.
Also, maybe on some HW, D1 will support wake on card detect. I dont
know how can the driver hint pci subsystem that it should go D1 rather
than D3 in the runtime_pm flow.
Maybe Rafael can advice on this.
Regards,
Pierre
Hi Pierre,
On Sun, Feb 13, 2011 at 11:42:01AM +0100, Pierre Tardy wrote:
> > [ ? 10.892280] sdhci-pci 0000:17:00.0: Unbalanced pm_runtime_enable!
>
> I never have seen this before. I'm not testing those patches on
> mmc-next, as our platform is not totally upstream yet, and do not boot
> on 2.6.37+ based kernel.
> However, we backported the pm_runtime patches from 2.6.37. Is there
> some differences in very latest patches on default enablement of
> runtime_pm?
It's getting enabled by drivers/pci/pci-driver.c:local_pci_probe(),
before the sdhci-pci probe function runs. local_pci_probe() does:
/* Unbound PCI devices are always set to disabled and suspended.
* During probe, the device is set to enabled and active and the
* usage count is incremented. If the driver supports runtime PM,
* it should call pm_runtime_put_noidle() in its probe routine and
* pm_runtime_get_noresume() in its remove routine.
*/
pm_runtime_get_noresume(dev);
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
and then the "Unbalanced pm_runtime_enable!" message appears when
sdhci_pci_probe() calls pm_runtime_enable() for the second time.
> > I then removed the card and reinserted it, but there's no dmesg output
> > related to the reinsert, only to the card removal:
> >
> > [ ? 65.381047] mmc0: card d555 removed
> >
> > So, card insertion is broken. ?I then did rmmod sdhci-pci && modprobe
> > sdhci-pci, and it picked up the card again.
>
> So what you have here is non working card-detection when device is in PCI_D3.
> This sounds like a HW limitation in you platform.
>
> D3 is originally meant for suspend to ram, so it sounds logic that
> some HW wont support wake on card detect.
> it sounds like we need a SDHCI_RUNTIME_PM_CAP, so that we can properly
> describe which HW can be safely put D3 at runtime.
> Also, maybe on some HW, D1 will support wake on card detect. I dont
> know how can the driver hint pci subsystem that it should go D1 rather
> than D3 in the runtime_pm flow.
> Maybe Rafael can advice on this.
The controller advertises the ability to generate PME wakeups from D3
in its PCI capabilities; perhaps we just aren't programming it properly.
I don't see anything in your patch that would set SDHCI wakeup bits or
unmask card insertion/removal IRQs -- maybe that could be it?
Thanks,
--
Chris Ball <[email protected]> <http://printf.net/>
One Laptop Per Child
Rafael,
Any comments here?
> It's getting enabled by drivers/pci/pci-driver.c:local_pci_probe(),
> before the sdhci-pci probe function runs. local_pci_probe() does:
>
> /* Unbound PCI devices are always set to disabled and suspended.
> * During probe, the device is set to enabled and active and the
> * usage count is incremented. If the driver supports runtime PM,
> * it should call pm_runtime_put_noidle() in its probe routine and
> * pm_runtime_get_noresume() in its remove routine.
> */
> pm_runtime_get_noresume(dev);
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
OK. Sounds like it is a change post 2.6.37.
So we need to replace the current sdhci runtime_pm initialization by what is in this comments.
> > Maybe Rafael can advice on this.
>
> The controller advertises the ability to generate PME wakeups from D3
> in its PCI capabilities; perhaps we just aren't programming it properly.
> I don't see anything in your patch that would set SDHCI wakeup bits or
> unmask card insertion/removal IRQs -- maybe that could be it?
My understanding is the pci driver is not supposed to do any set_power_state/pci_save_state/wake_enable.
Everything is supposed to be generically handle by pci frameworks's runtime_pm impl.
Need confirmation from Rafael.
Regards,
Pierre
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris,
92196 Meudon Cedex, France
Registration Number: 302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
On Fri, Feb 25, 2011 at 07:33:12AM +0000, Tardy, Pierre wrote:
> My understanding is the pci driver is not supposed to do any set_power_state/pci_save_state/wake_enable.
> Everything is supposed to be generically handle by pci frameworks's runtime_pm impl.
> Need confirmation from Rafael.
The core can only enable PME generation, it can't configure what
generates PMEs. There's a register in sdhci that needs to be programmed
to enable wakeups on card insert/removal/interrupt. If you don't then
you won't get a PME no matter what the core does.
--
Matthew Garrett | [email protected]
On Friday, February 25, 2011, Matthew Garrett wrote:
> On Fri, Feb 25, 2011 at 07:33:12AM +0000, Tardy, Pierre wrote:
>
> > My understanding is the pci driver is not supposed to do any set_power_state/pci_save_state/wake_enable.
> > Everything is supposed to be generically handle by pci frameworks's runtime_pm impl.
> > Need confirmation from Rafael.
>
> The core can only enable PME generation, it can't configure what
> generates PMEs. There's a register in sdhci that needs to be programmed
> to enable wakeups on card insert/removal/interrupt. If you don't then
> you won't get a PME no matter what the core does.
That's correct. The driver shoulnd't touch the generic-PCI part of wakeup
preparation, but it _should_ configure the device-specific part of it.
It's quite similar to network adapters where you have to configure the adapter
to react to magic packets, for example, in addition to the PCI PME setup done
by the core.
Thanks,
Rafael