sdhci runtime_pm implementation using mmc clock gating fw

PATCH In review Pierre Tardy 6 participants 25 messages Feb 6, 2011 – Feb 25, 2011

Subsystems (2)

MMC, SD and SDIO Secure digital host controller interface (SDHCI) driver

lore

Share & tools

lore

Pierre Tardy

author

2011-02-06 18:02 UTC #1 lore raw

PATCH v2 1/3 mmc: put the led blinking code after clock ungating

In reply to Pierre Tardy

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(-)

drivers/mmc/core/core.c +1 -2

index 6625c05..34a7e8c 100644
@@ -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);
 }

Signature · 13 lines

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.

Pierre Tardy

author

2011-02-06 18:02 UTC #2 lore raw

PATCH v2 1/3 mmc: put the led blinking code after clock ungating

In reply to Pierre Tardy

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(-)

drivers/mmc/core/core.c +1 -2

index 6625c05..34a7e8c 100644
@@ -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);
 }

Signature · 13 lines

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.

Pierre Tardy

author

2011-02-06 18:02 UTC #3 lore raw

PATCH v2 2/3 sdhci-pci : Enable runtime PM support

In reply to Pierre Tardy

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(-)

drivers/mmc/host/sdhci-pci.c +121 -0

index 0dc905b..22581a1 100644
@@ -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
+	},
 };
 
 /*****************************************************************************\

drivers/mmc/host/sdhci.c +31 -0

index 9e15f41..3e65d94 100644
@@ -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                                            *

drivers/mmc/host/sdhci.h +5 -0

index 6e0969e..1f032c0 100644
@@ -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 */

Signature · 13 lines

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.

Pierre Tardy

author

2011-02-06 18:02 UTC #19 lore raw

PATCH v2 3/3 sdhci:v2:use ios->clock to know when sdhci is idle

In reply to Pierre Tardy

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

Quoted 1 line

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(-)

drivers/mmc/host/sdhci-pci.c +3 -0

index 22581a1..9d23f4c 100644
@@ -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;

drivers/mmc/host/sdhci.c +29 -0

index 3e65d94..655617c 100644
@@ -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;
 }

include/linux/mmc/sdhci.h +1 -0

index 83bd9f7..a38d040 100644
@@ -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 */

Signature · 13 lines

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.

Pierre Tardy

author

2011-02-06 18:02 UTC #20 lore raw

PATCH v2 3/3 sdhci:v2:use ios->clock to know when sdhci is idle

In reply to Pierre Tardy

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

Quoted 1 line

From Linus Walleij <[email protected]>

author of mmc aggressive clock gating fw.

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(-)

drivers/mmc/host/sdhci-pci.c +3 -0

index 22581a1..9d23f4c 100644
@@ -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;

drivers/mmc/host/sdhci.c +29 -0

index 3e65d94..655617c 100644
@@ -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;
 }

include/linux/mmc/sdhci.h +1 -0

index 83bd9f7..a38d040 100644
@@ -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 */

Signature · 13 lines

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.

Pierre Tardy

author

2011-02-06 18:03 UTC #0 lore raw

v2 cover sdhci runtime_pm implementation using mmc clock gating fw

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(-)

Signature · 13 lines

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.

Pierre Tardy

author

2011-02-06 18:03 UTC #4 lore raw

PATCH v2 2/3 sdhci-pci : Enable runtime PM support

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(-)

drivers/mmc/host/sdhci-pci.c +121 -0

index 0dc905b..22581a1 100644
@@ -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
+	},
 };
 
 /*****************************************************************************\

drivers/mmc/host/sdhci.c +31 -0

index 9e15f41..3e65d94 100644
@@ -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                                            *

drivers/mmc/host/sdhci.h +5 -0

index 6e0969e..1f032c0 100644
@@ -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 */

Signature · 13 lines

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.

Linus Walleij

Acked-by on the v2 series

2011-02-06 18:54 UTC #21 lore raw

In reply to Pierre Tardy

2011/2/6 Pierre Tardy <[email protected]>:

Quoted 3 lines

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

Linus Walleij

Acked-by on the v2 series

2011-02-06 18:54 UTC #22 lore raw

In reply to Pierre Tardy

2011/2/6 Pierre Tardy <[email protected]>:

Quoted 3 lines

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

Chris Ball

2011-02-06 20:35 UTC #23 lore raw

In reply to Pierre Tardy

Hi Pierre,

Quoted 7 lines · Pierre Tardy

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.

Quoted 6 lines

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.

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

Chris Ball

2011-02-06 20:35 UTC #24 lore raw

In reply to Pierre Tardy

Hi Pierre,

Quoted 7 lines · Pierre Tardy

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.

Quoted 6 lines

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.

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

Chris Ball

2011-02-06 21:15 UTC #5 lore raw

Re: [PATCH v2 2/3] sdhci-pci : Enable runtime PM support

In reply to Pierre Tardy

Hi,

Quoted 10 lines · Pierre Tardy

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

Quoted 4 lines

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:

drivers/mmc/host/sdhci-pci.c +1 -0

index 22581a1..48061b3 100644
@@ -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.

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

Chris Ball

2011-02-06 21:15 UTC #6 lore raw

Re: [PATCH v2 2/3] sdhci-pci : Enable runtime PM support

In reply to Pierre Tardy

Hi,

Quoted 10 lines · Pierre Tardy

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

Quoted 4 lines

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:

drivers/mmc/host/sdhci-pci.c +1 -0

index 22581a1..48061b3 100644
@@ -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.

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

Chris Ball

2011-02-10 04:37 UTC #7 lore raw

Re: [PATCH v2 2/3] sdhci-pci : Enable runtime PM support

In reply to Pierre Tardy

Hi Pierre,

Quoted 9 lines · Pierre Tardy

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,

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

Chris Ball

2011-02-10 04:37 UTC #8 lore raw

Re: [PATCH v2 2/3] sdhci-pci : Enable runtime PM support

In reply to Pierre Tardy

Hi Pierre,

Quoted 9 lines · Pierre Tardy

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.

Full dmesg is at:
http://dev.laptop.org/~cjb/logs/runtime-pm-sdhci-20100209.log

Thanks,

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

Pierre Tardy

2011-02-13 10:42 UTC #9 lore raw

Re: [PATCH v2 2/3] sdhci-pci : Enable runtime PM support

Quoted 30 lines · Pierre Tardy

Quoted 9 lines
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!

Quoted 1 line

(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?

Quoted 8 lines

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

Pierre Tardy

2011-02-13 10:42 UTC #10 lore raw

Re: [PATCH v2 2/3] sdhci-pci : Enable runtime PM support

In reply to Chris Ball

Quoted 30 lines · Pierre Tardy

Quoted 9 lines
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!

Quoted 1 line

(Note the last line above.)

Quoted 8 lines

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.

Regards,
Pierre

Chris Ball

2011-02-21 20:45 UTC #11 lore raw

Re: [PATCH v2 2/3] sdhci-pci : Enable runtime PM support

Hi Pierre,

Quoted 8 lines · Pierre Tardy

Quoted 1 line
[ 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.

Quoted 11 lines

Quoted 7 lines
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.

Quoted 8 lines

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,

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

Chris Ball

2011-02-21 20:45 UTC #12 lore raw

Re: [PATCH v2 2/3] sdhci-pci : Enable runtime PM support

In reply to Pierre Tardy

Hi Pierre,

Quoted 8 lines · Pierre Tardy

Quoted 1 line
[ 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.

Quoted 11 lines

Quoted 7 lines
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.

Quoted 8 lines

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.

Thanks,

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

Tardy, Pierre

author

2011-02-25 07:33 UTC #13 lore raw

Re: [PATCH v2 2/3] sdhci-pci : Enable runtime PM support

Rafael,
Any comments here?

Quoted 12 lines

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.

Quoted 6 lines

Quoted 1 line
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.

Tardy, Pierre

author

2011-02-25 07:33 UTC #14 lore raw

RE: [PATCH v2 2/3] sdhci-pci : Enable runtime PM support

In reply to Chris Ball

Rafael,
Any comments here?

Quoted 12 lines

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.

Quoted 6 lines

Quoted 1 line
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?

Matthew Garrett

2011-02-25 14:54 UTC #15 lore raw

Re: [PATCH v2 2/3] sdhci-pci : Enable runtime PM support

Quoted 3 lines · Pierre

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.

Signature · 1 line

Matthew Garrett | [email protected]

Matthew Garrett

2011-02-25 14:54 UTC #16 lore raw

Re: [PATCH v2 2/3] sdhci-pci : Enable runtime PM support

In reply to Tardy, Pierre

Quoted 3 lines · Pierre

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.

Signature · 1 line

Matthew Garrett | [email protected]

R. J. Wysocki

2011-02-25 18:28 UTC #17 lore raw

Re: [PATCH v2 2/3] sdhci-pci : Enable runtime PM support

Quoted 10 lines · Matthew Garrett

Quoted 3 lines · Pierre
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

R. J. Wysocki

2011-02-25 18:28 UTC #18 lore raw

Re: [PATCH v2 2/3] sdhci-pci : Enable runtime PM support

In reply to Matthew Garrett

Quoted 10 lines · Matthew Garrett

Quoted 3 lines · Pierre
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