[RFC,PATCHv3 0/3] sdhci runtime_pm implementation

[RFC,PATCHv3 0/3] sdhci runtime_pm implementation

[Pierre Tardy]

Please find sdhci runtime_pm implementation.

It uses clock gating fw as a tip to know when our chip is idle.
It implements wake up from card insertion/removal.

This is RFC, please dont merge yet. I really would like to have deep review
from PCI linux-pm guys.

Opens are: 

1/ Not sure if the pci configs in the driver in rpm_suspend/resume flow
 are not duplicate from what the core is doing.

2/ Wakeup from D3hot: I cannot find any driver that is implementing it in current upstream,
I'm not sure if I'm doing it the good way. 
We have the exact same issue for our not yet upstreamable usb-otg driver.
Not sure if this cannot be implemented generically in core.

version 1: was RFC and untested,
version 2: has been tested on intel medfield platform.
version 3: RFC, untested.
- Change runtime_pm initialization to match latest PCI framework scheme
- Added wake-up support

Pierre Tardy (2):
  mmc: sdhci: use ios->clock to know when sdhci is idle
  mmc: sdhci: handle wake-up from runtime_pm

Yunpeng Gao (1):
  mmc: sdhci-pci: Enable runtime PM support

 drivers/mmc/host/sdhci-pci.c |  117 +++++++++++++++++++++++++++++++++++++++++-
 drivers/mmc/host/sdhci.c     |   88 +++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.h     |    5 ++
 include/linux/mmc/sdhci.h    |    3 +
 scripts/checkpatch.pl        |    6 +-
 5 files changed, 215 insertions(+), 4 deletions(-)

[RFC,PATCHv3 1/3] mmc: sdhci-pci: Enable runtime PM support

[Pierre Tardy]

From: Yunpeng Gao <yunpeng.gao@intel.com>

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 a following patch

This patch also does not enable wakeups, so card insertion will not work
if runtime_pm is activated.
This is also dealt in a following patch

Original version from: Yunpeng Gao <yunpeng.gao@intel.com>
with modifications by: Pierre Tardy <pierre.tardy@intel.com>

Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
---
 drivers/mmc/host/sdhci-pci.c |  120 +++++++++++++++++++++++++++++++++++++++++-
 drivers/mmc/host/sdhci.c     |   31 +++++++++++
 drivers/mmc/host/sdhci.h     |    5 ++
 3 files changed, 155 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 2f8d468..f167a55 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>
 
@@ -1031,6 +1032,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)
@@ -1086,7 +1089,7 @@ static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
 
 		chip->slots[i] = slot;
 	}
-
+	pm_runtime_put_noidle(&pdev->dev);
 	return 0;
 
 free:
@@ -1114,8 +1117,120 @@ static void __devexit sdhci_pci_remove(struct pci_dev *pdev)
 	}
 
 	pci_disable_device(pdev);
+	pm_runtime_get_noresume(&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,
@@ -1123,6 +1238,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..cd79a28 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 */
+	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.1

[RFC, PATCHv3 1/3] mmc: sdhci-pci: Enable runtime PM support

[Pierre Tardy]

From: Yunpeng Gao <yunpeng.gao@intel.com>

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 a following patch

This patch also does not enable wakeups, so card insertion will not work
if runtime_pm is activated.
This is also dealt in a following patch

Original version from: Yunpeng Gao <yunpeng.gao@intel.com>
with modifications by: Pierre Tardy <pierre.tardy@intel.com>

Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
---
 drivers/mmc/host/sdhci-pci.c |  120 +++++++++++++++++++++++++++++++++++++++++-
 drivers/mmc/host/sdhci.c     |   31 +++++++++++
 drivers/mmc/host/sdhci.h     |    5 ++
 3 files changed, 155 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 2f8d468..f167a55 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>
 
@@ -1031,6 +1032,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)
@@ -1086,7 +1089,7 @@ static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
 
 		chip->slots[i] = slot;
 	}
-
+	pm_runtime_put_noidle(&pdev->dev);
 	return 0;
 
 free:
@@ -1114,8 +1117,120 @@ static void __devexit sdhci_pci_remove(struct pci_dev *pdev)
 	}
 
 	pci_disable_device(pdev);
+	pm_runtime_get_noresume(&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,
@@ -1123,6 +1238,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..cd79a28 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 */
+	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.1

[RFC,PATCHv3 2/3] mmc: sdhci: use ios->clock to know when sdhci is idle

[Pierre Tardy]

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 <linus.walleij@stericsson.com>
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 <linus.walleij@stericsson.com>

Signed-off-by: Pierre Tardy <tardyp@gmail.com>
---
 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 f167a55..1b5e70b 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -1089,6 +1089,9 @@ static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
 
 		chip->slots[i] = slot;
 	}
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_suspend_ignore_children(&pdev->dev, 1);
 	pm_runtime_put_noidle(&pdev->dev);
 	return 0;
 
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index cd79a28..735c3f7 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.1

[RFC, PATCHv3 2/3] mmc: sdhci: use ios->clock to know when sdhci is idle

[Pierre Tardy]

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 <linus.walleij@stericsson.com>
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 <linus.walleij@stericsson.com>

Signed-off-by: Pierre Tardy <tardyp@gmail.com>
---
 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 f167a55..1b5e70b 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -1089,6 +1089,9 @@ static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
 
 		chip->slots[i] = slot;
 	}
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_suspend_ignore_children(&pdev->dev, 1);
 	pm_runtime_put_noidle(&pdev->dev);
 	return 0;
 
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index cd79a28..735c3f7 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.1

[RFC,PATCHv3 3/3] mmc: sdhci: handle wake-up from runtime_pm

[Pierre Tardy]

When sdhci is runtime_suspended, it can still receive a wake-up
because of card insertion.
The wake-up will then be signaled by an interrupt which cannot be serviced
immediatly, as the device is powered off, and register are not accessibles.

We cannot call pm_runtime_get_sync() in irq context as on some architecture (e.g. intel_mid),
runtime_resume can lead to too much latency, due to e.g. PLL warm-up and such aggressive
power gating.

We temporarly disable the interrupt, and trigger a runtime_resume of the device
The interrupt will be re-enabled when resume is finished.

Signed-off-by: Pierre Tardy <tardyp@gmail.com>
---
 drivers/mmc/host/sdhci-pci.c |    8 +-------
 drivers/mmc/host/sdhci.c     |   30 +++++++++++++++++++++++++++++-
 include/linux/mmc/sdhci.h    |    2 ++
 scripts/checkpatch.pl        |    6 +++---
 4 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 1b5e70b..fc3d2e1 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -1166,13 +1166,7 @@ static int sdhci_pci_runtime_suspend(struct device *dev)
 	}
 
 	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_enable_wake(pdev, PCI_D3hot, 1);
 	pci_set_power_state(pdev, PCI_D3hot);
 
 	return 0;
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 735c3f7..85a1956 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1599,6 +1599,21 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 	u32 intmask;
 	int cardint = 0;
 
+	/* we might come from a sd insertion or sdio irq wake.. */
+	if (pm_runtime_suspended()) {
+		host->waking_up = 1;
+		/* Note that we disable temporarly the interrupt until we do the
+		 * resume. If we don't then we'll get constantly interrupted
+		 * until we actually resume.
+		 *
+		 * as the irq is shared, this might not be very friendly to our
+		 * irq sharers but the pm_runtime workqueue should really be
+		 * called soon.
+		 */
+		disable_irq_nosync(irq);
+		pm_runtime_get(host->mmc->parent);
+		return IRQ_NONE;
+	}
 	spin_lock(&host->lock);
 
 	intmask = sdhci_readl(host, SDHCI_INT_STATUS);
@@ -1747,7 +1762,12 @@ EXPORT_SYMBOL_GPL(sdhci_enable_irq_wakeups);
 
 int sdhci_runtime_suspend(struct sdhci_host *host)
 {
-	/* nothing to do yet */
+	u8 val;
+	/* make sure we wake up on sdcard insert/remove enabled */
+	val = SDHCI_WAKE_ON_INSERT | SDHCI_WAKE_ON_REMOVE;
+	if (host->mmc->pm_flags & MMC_PM_WAKE_SDIO_IRQ)
+		val |= SDHCI_WAKE_ON_INT;
+	sdhci_writeb(host, val, SDHCI_WAKE_UP_CONTROL);
 	return 0;
 }
 
@@ -1768,6 +1788,14 @@ int sdhci_runtime_resume(struct sdhci_host *host)
 	sdhci_set_ios(host->mmc, &host->mmc->ios);
 	mmiowb();
 
+	if (host->waking_up) {
+		host->waking_up = 0;
+		/* Re-enable the irq now we are perfectly resumed.
+		 * Any yet unhandled interrupt (i.e. the wake) will retrigger
+		 * irq
+		 */
+		irq_enable(host->irq);
+	}
 	return ret;
 }
 EXPORT_SYMBOL_GPL(sdhci_runtime_resume);
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index a38d040..6a3c6af 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -146,6 +146,8 @@ struct sdhci_host {
 	unsigned int            ocr_avail_sd;
 	unsigned int            ocr_avail_mmc;
 
+	bool                    waking_up;
+
 	unsigned long private[0] ____cacheline_aligned;
 };
 #endif /* __SDHCI_H */
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e3c7fc0..17aeda3 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1325,8 +1325,8 @@ sub process {
 		$prefix = "$filename:$realline: " if ($emacs && $file);
 		$prefix = "$filename:$linenr: " if ($emacs && !$file);
 
-		$here = "#$linenr: " if (!$file);
-		$here = "#$realline: " if ($file);
+		$here = "" if (!$file);
+		$here = "" if ($file);
 
 		# extract the filename as it passes
 		if ($line =~ /^diff --git.*?(\S+)$/) {
@@ -1349,7 +1349,7 @@ sub process {
 			next;
 		}
 
-		$here .= "FILE: $realfile:$realline:" if ($realcnt != 0);
+		$here .= "$realfile:$realline:" if ($realcnt != 0);
 
 		my $hereline = "$here\n$rawline\n";
 		my $herecurr = "$here\n$rawline\n";
-- 
1.7.1

[RFC, PATCHv3 3/3] mmc: sdhci: handle wake-up from runtime_pm

[Pierre Tardy]

When sdhci is runtime_suspended, it can still receive a wake-up
because of card insertion.
The wake-up will then be signaled by an interrupt which cannot be serviced
immediatly, as the device is powered off, and register are not accessibles.

We cannot call pm_runtime_get_sync() in irq context as on some architecture (e.g. intel_mid),
runtime_resume can lead to too much latency, due to e.g. PLL warm-up and such aggressive
power gating.

We temporarly disable the interrupt, and trigger a runtime_resume of the device
The interrupt will be re-enabled when resume is finished.

Signed-off-by: Pierre Tardy <tardyp@gmail.com>
---
 drivers/mmc/host/sdhci-pci.c |    8 +-------
 drivers/mmc/host/sdhci.c     |   30 +++++++++++++++++++++++++++++-
 include/linux/mmc/sdhci.h    |    2 ++
 scripts/checkpatch.pl        |    6 +++---
 4 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 1b5e70b..fc3d2e1 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -1166,13 +1166,7 @@ static int sdhci_pci_runtime_suspend(struct device *dev)
 	}
 
 	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_enable_wake(pdev, PCI_D3hot, 1);
 	pci_set_power_state(pdev, PCI_D3hot);
 
 	return 0;
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 735c3f7..85a1956 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1599,6 +1599,21 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 	u32 intmask;
 	int cardint = 0;
 
+	/* we might come from a sd insertion or sdio irq wake.. */
+	if (pm_runtime_suspended()) {
+		host->waking_up = 1;
+		/* Note that we disable temporarly the interrupt until we do the
+		 * resume. If we don't then we'll get constantly interrupted
+		 * until we actually resume.
+		 *
+		 * as the irq is shared, this might not be very friendly to our
+		 * irq sharers but the pm_runtime workqueue should really be
+		 * called soon.
+		 */
+		disable_irq_nosync(irq);
+		pm_runtime_get(host->mmc->parent);
+		return IRQ_NONE;
+	}
 	spin_lock(&host->lock);
 
 	intmask = sdhci_readl(host, SDHCI_INT_STATUS);
@@ -1747,7 +1762,12 @@ EXPORT_SYMBOL_GPL(sdhci_enable_irq_wakeups);
 
 int sdhci_runtime_suspend(struct sdhci_host *host)
 {
-	/* nothing to do yet */
+	u8 val;
+	/* make sure we wake up on sdcard insert/remove enabled */
+	val = SDHCI_WAKE_ON_INSERT | SDHCI_WAKE_ON_REMOVE;
+	if (host->mmc->pm_flags & MMC_PM_WAKE_SDIO_IRQ)
+		val |= SDHCI_WAKE_ON_INT;
+	sdhci_writeb(host, val, SDHCI_WAKE_UP_CONTROL);
 	return 0;
 }
 
@@ -1768,6 +1788,14 @@ int sdhci_runtime_resume(struct sdhci_host *host)
 	sdhci_set_ios(host->mmc, &host->mmc->ios);
 	mmiowb();
 
+	if (host->waking_up) {
+		host->waking_up = 0;
+		/* Re-enable the irq now we are perfectly resumed.
+		 * Any yet unhandled interrupt (i.e. the wake) will retrigger
+		 * irq
+		 */
+		irq_enable(host->irq);
+	}
 	return ret;
 }
 EXPORT_SYMBOL_GPL(sdhci_runtime_resume);
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index a38d040..6a3c6af 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -146,6 +146,8 @@ struct sdhci_host {
 	unsigned int            ocr_avail_sd;
 	unsigned int            ocr_avail_mmc;
 
+	bool                    waking_up;
+
 	unsigned long private[0] ____cacheline_aligned;
 };
 #endif /* __SDHCI_H */
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e3c7fc0..17aeda3 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1325,8 +1325,8 @@ sub process {
 		$prefix = "$filename:$realline: " if ($emacs && $file);
 		$prefix = "$filename:$linenr: " if ($emacs && !$file);
 
-		$here = "#$linenr: " if (!$file);
-		$here = "#$realline: " if ($file);
+		$here = "" if (!$file);
+		$here = "" if ($file);
 
 		# extract the filename as it passes
 		if ($line =~ /^diff --git.*?(\S+)$/) {
@@ -1349,7 +1349,7 @@ sub process {
 			next;
 		}
 
-		$here .= "FILE: $realfile:$realline:" if ($realcnt != 0);
+		$here .= "$realfile:$realline:" if ($realcnt != 0);
 
 		my $hereline = "$here\n$rawline\n";
 		my $herecurr = "$here\n$rawline\n";
-- 
1.7.1

Re: [linux-pm] [RFC,PATCHv3 0/3] sdhci runtime_pm implementation

[Alan Stern]

On Tue, 1 Mar 2011, Pierre Tardy wrote:

> Please find sdhci runtime_pm implementation.
> 
> It uses clock gating fw as a tip to know when our chip is idle.
> It implements wake up from card insertion/removal.
> 
> This is RFC, please dont merge yet. I really would like to have deep review
> from PCI linux-pm guys.
> 
> Opens are: 
> 
> 1/ Not sure if the pci configs in the driver in rpm_suspend/resume flow
>  are not duplicate from what the core is doing.

There may be one or two small errors.

> 2/ Wakeup from D3hot: I cannot find any driver that is implementing it in current upstream,

Other drivers do it, but they use PCI PME# instead of interrupts.

> I'm not sure if I'm doing it the good way. 
> We have the exact same issue for our not yet upstreamable usb-otg driver.
> Not sure if this cannot be implemented generically in core.

Alan Stern

Re: [RFC,PATCHv3 0/3] sdhci runtime_pm implementation

[Alan Stern]

On Tue, 1 Mar 2011, Pierre Tardy wrote:

> Please find sdhci runtime_pm implementation.
> 
> It uses clock gating fw as a tip to know when our chip is idle.
> It implements wake up from card insertion/removal.
> 
> This is RFC, please dont merge yet. I really would like to have deep review
> from PCI linux-pm guys.
> 
> Opens are: 
> 
> 1/ Not sure if the pci configs in the driver in rpm_suspend/resume flow
>  are not duplicate from what the core is doing.

There may be one or two small errors.

> 2/ Wakeup from D3hot: I cannot find any driver that is implementing it in current upstream,

Other drivers do it, but they use PCI PME# instead of interrupts.

> I'm not sure if I'm doing it the good way. 
> We have the exact same issue for our not yet upstreamable usb-otg driver.
> Not sure if this cannot be implemented generically in core.

Alan Stern

Re: [linux-pm] [RFC,PATCHv3 0/3] sdhci runtime_pm implementation

[Alan Stern]

On Tue, 1 Mar 2011, Pierre Tardy wrote:

> Please find sdhci runtime_pm implementation.
> 
> It uses clock gating fw as a tip to know when our chip is idle.
> It implements wake up from card insertion/removal.
> 
> This is RFC, please dont merge yet. I really would like to have deep review
> from PCI linux-pm guys.
> 
> Opens are: 
> 
> 1/ Not sure if the pci configs in the driver in rpm_suspend/resume flow
>  are not duplicate from what the core is doing.

There may be one or two small errors.

> 2/ Wakeup from D3hot: I cannot find any driver that is implementing it in current upstream,

Other drivers do it, but they use PCI PME# instead of interrupts.

> I'm not sure if I'm doing it the good way. 
> We have the exact same issue for our not yet upstreamable usb-otg driver.
> Not sure if this cannot be implemented generically in core.

Alan Stern

Re: [linux-pm] [RFC,PATCHv3 0/3] sdhci runtime_pm implementation

[Pierre Tardy]

On Tue, Mar 1, 2011 at 8:33 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 1 Mar 2011, Pierre Tardy wrote:
>
>> Please find sdhci runtime_pm implementation.
>>
>> It uses clock gating fw as a tip to know when our chip is idle.
>> It implements wake up from card insertion/removal.
>>
>> This is RFC, please dont merge yet. I really would like to have deep review
>> from PCI linux-pm guys.
>>
>> Opens are:
>>
>> 1/ Not sure if the pci configs in the driver in rpm_suspend/resume flow
>>  are not duplicate from what the core is doing.
>
> There may be one or two small errors.
>
>> 2/ Wakeup from D3hot: I cannot find any driver that is implementing it in current upstream,
>
> Other drivers do it, but they use PCI PME# instead of interrupts.
Could you please elaborate?
My understanding is that PCI PME will generate MSI, which translate in
interrupt.

Thanks
Pierre

Re: [RFC,PATCHv3 0/3] sdhci runtime_pm implementation

[Pierre Tardy]

On Tue, Mar 1, 2011 at 8:33 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 1 Mar 2011, Pierre Tardy wrote:
>
>> Please find sdhci runtime_pm implementation.
>>
>> It uses clock gating fw as a tip to know when our chip is idle.
>> It implements wake up from card insertion/removal.
>>
>> This is RFC, please dont merge yet. I really would like to have deep review
>> from PCI linux-pm guys.
>>
>> Opens are:
>>
>> 1/ Not sure if the pci configs in the driver in rpm_suspend/resume flow
>>  are not duplicate from what the core is doing.
>
> There may be one or two small errors.
>
>> 2/ Wakeup from D3hot: I cannot find any driver that is implementing it in current upstream,
>
> Other drivers do it, but they use PCI PME# instead of interrupts.
Could you please elaborate?
My understanding is that PCI PME will generate MSI, which translate in
interrupt.

Thanks
Pierre

Re: [linux-pm] [RFC, PATCHv3 1/3] mmc: sdhci-pci: Enable runtime PM support

[Alan Stern]

On Tue, 1 Mar 2011, Pierre Tardy wrote:

> From: Yunpeng Gao <yunpeng.gao@intel.com>
> 
> 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 a following patch
> 
> This patch also does not enable wakeups, so card insertion will not work
> if runtime_pm is activated.
> This is also dealt in a following patch
> 
> Original version from: Yunpeng Gao <yunpeng.gao@intel.com>
> with modifications by: Pierre Tardy <pierre.tardy@intel.com>
> 
> Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
> ---
>  drivers/mmc/host/sdhci-pci.c |  120 +++++++++++++++++++++++++++++++++++++++++-
>  drivers/mmc/host/sdhci.c     |   31 +++++++++++
>  drivers/mmc/host/sdhci.h     |    5 ++
>  3 files changed, 155 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
> index 2f8d468..f167a55 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>
>  
> @@ -1031,6 +1032,8 @@ static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
>  	if (ret)
>  		return ret;
>  
> +	pm_runtime_set_active(&pdev->dev);

This line is redundant.  It won't do anything, and the PCI core already 
does it.

> +
>  	slots = PCI_SLOT_INFO_SLOTS(slots) + 1;
>  	dev_dbg(&pdev->dev, "found %d slot(s)\n", slots);
>  	if (slots == 0)
> @@ -1086,7 +1089,7 @@ static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
>  
>  		chip->slots[i] = slot;
>  	}
> -
> +	pm_runtime_put_noidle(&pdev->dev);
>  	return 0;
>  
>  free:
> @@ -1114,8 +1117,120 @@ static void __devexit sdhci_pci_remove(struct pci_dev *pdev)
>  	}
>  
>  	pci_disable_device(pdev);
> +	pm_runtime_get_noresume(&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;
> +		}
> +	}

You probably should call synchronize_irq() here.  Otherwise a delayed
IRQ might get delivered after the device has changed to D3.

> +
> +	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);

Most of this isn't needed.  The PCI core does pci_save_state(), 
pci_enable_wake(), and pci_set_power_state().

Of course, you might want to override the wakeup setting selected by
the core.  This is determined by pci_dev_run_wake(), for which you may 
need to call device_set_run_wake().

> +
> +	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);

The PCI core does pci_set_power_state() and pci_restore_state().

> +	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;
> +}

Alan Stern

Re: [RFC, PATCHv3 1/3] mmc: sdhci-pci: Enable runtime PM support

[Alan Stern]

On Tue, 1 Mar 2011, Pierre Tardy wrote:

> From: Yunpeng Gao <yunpeng.gao@intel.com>
> 
> 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 a following patch
> 
> This patch also does not enable wakeups, so card insertion will not work
> if runtime_pm is activated.
> This is also dealt in a following patch
> 
> Original version from: Yunpeng Gao <yunpeng.gao@intel.com>
> with modifications by: Pierre Tardy <pierre.tardy@intel.com>
> 
> Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
> ---
>  drivers/mmc/host/sdhci-pci.c |  120 +++++++++++++++++++++++++++++++++++++++++-
>  drivers/mmc/host/sdhci.c     |   31 +++++++++++
>  drivers/mmc/host/sdhci.h     |    5 ++
>  3 files changed, 155 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
> index 2f8d468..f167a55 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>
>  
> @@ -1031,6 +1032,8 @@ static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
>  	if (ret)
>  		return ret;
>  
> +	pm_runtime_set_active(&pdev->dev);

This line is redundant.  It won't do anything, and the PCI core already 
does it.

> +
>  	slots = PCI_SLOT_INFO_SLOTS(slots) + 1;
>  	dev_dbg(&pdev->dev, "found %d slot(s)\n", slots);
>  	if (slots == 0)
> @@ -1086,7 +1089,7 @@ static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
>  
>  		chip->slots[i] = slot;
>  	}
> -
> +	pm_runtime_put_noidle(&pdev->dev);
>  	return 0;
>  
>  free:
> @@ -1114,8 +1117,120 @@ static void __devexit sdhci_pci_remove(struct pci_dev *pdev)
>  	}
>  
>  	pci_disable_device(pdev);
> +	pm_runtime_get_noresume(&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;
> +		}
> +	}

You probably should call synchronize_irq() here.  Otherwise a delayed
IRQ might get delivered after the device has changed to D3.

> +
> +	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);

Most of this isn't needed.  The PCI core does pci_save_state(), 
pci_enable_wake(), and pci_set_power_state().

Of course, you might want to override the wakeup setting selected by
the core.  This is determined by pci_dev_run_wake(), for which you may 
need to call device_set_run_wake().

> +
> +	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);

The PCI core does pci_set_power_state() and pci_restore_state().

> +	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;
> +}

Alan Stern

Re: [linux-pm] [RFC, PATCHv3 1/3] mmc: sdhci-pci: Enable runtime PM support

[Alan Stern]

On Tue, 1 Mar 2011, Pierre Tardy wrote:

> From: Yunpeng Gao <yunpeng.gao@intel.com>
> 
> 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 a following patch
> 
> This patch also does not enable wakeups, so card insertion will not work
> if runtime_pm is activated.
> This is also dealt in a following patch
> 
> Original version from: Yunpeng Gao <yunpeng.gao@intel.com>
> with modifications by: Pierre Tardy <pierre.tardy@intel.com>
> 
> Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
> ---
>  drivers/mmc/host/sdhci-pci.c |  120 +++++++++++++++++++++++++++++++++++++++++-
>  drivers/mmc/host/sdhci.c     |   31 +++++++++++
>  drivers/mmc/host/sdhci.h     |    5 ++
>  3 files changed, 155 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
> index 2f8d468..f167a55 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>
>  
> @@ -1031,6 +1032,8 @@ static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
>  	if (ret)
>  		return ret;
>  
> +	pm_runtime_set_active(&pdev->dev);

This line is redundant.  It won't do anything, and the PCI core already 
does it.

> +
>  	slots = PCI_SLOT_INFO_SLOTS(slots) + 1;
>  	dev_dbg(&pdev->dev, "found %d slot(s)\n", slots);
>  	if (slots == 0)
> @@ -1086,7 +1089,7 @@ static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
>  
>  		chip->slots[i] = slot;
>  	}
> -
> +	pm_runtime_put_noidle(&pdev->dev);
>  	return 0;
>  
>  free:
> @@ -1114,8 +1117,120 @@ static void __devexit sdhci_pci_remove(struct pci_dev *pdev)
>  	}
>  
>  	pci_disable_device(pdev);
> +	pm_runtime_get_noresume(&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;
> +		}
> +	}

You probably should call synchronize_irq() here.  Otherwise a delayed
IRQ might get delivered after the device has changed to D3.

> +
> +	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);

Most of this isn't needed.  The PCI core does pci_save_state(), 
pci_enable_wake(), and pci_set_power_state().

Of course, you might want to override the wakeup setting selected by
the core.  This is determined by pci_dev_run_wake(), for which you may 
need to call device_set_run_wake().

> +
> +	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);

The PCI core does pci_set_power_state() and pci_restore_state().

> +	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;
> +}

Alan Stern

Re: [linux-pm] [RFC, PATCHv3 3/3] mmc: sdhci: handle wake-up from runtime_pm

[Alan Stern]

On Tue, 1 Mar 2011, Pierre Tardy wrote:

> When sdhci is runtime_suspended, it can still receive a wake-up
> because of card insertion.
> The wake-up will then be signaled by an interrupt which cannot be serviced
> immediatly, as the device is powered off, and register are not accessibles.
> 
> We cannot call pm_runtime_get_sync() in irq context as on some architecture (e.g. intel_mid),
> runtime_resume can lead to too much latency, due to e.g. PLL warm-up and such aggressive
> power gating.
> 
> We temporarly disable the interrupt, and trigger a runtime_resume of the device
> The interrupt will be re-enabled when resume is finished.
> 
> Signed-off-by: Pierre Tardy <tardyp@gmail.com>
> ---
>  drivers/mmc/host/sdhci-pci.c |    8 +-------
>  drivers/mmc/host/sdhci.c     |   30 +++++++++++++++++++++++++++++-
>  include/linux/mmc/sdhci.h    |    2 ++
>  scripts/checkpatch.pl        |    6 +++---
>  4 files changed, 35 insertions(+), 11 deletions(-)

> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 735c3f7..85a1956 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1599,6 +1599,21 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
>  	u32 intmask;
>  	int cardint = 0;
>  
> +	/* we might come from a sd insertion or sdio irq wake.. */
> +	if (pm_runtime_suspended()) {
> +		host->waking_up = 1;
> +		/* Note that we disable temporarly the interrupt until we do the
> +		 * resume. If we don't then we'll get constantly interrupted
> +		 * until we actually resume.
> +		 *
> +		 * as the irq is shared, this might not be very friendly to our
> +		 * irq sharers but the pm_runtime workqueue should really be
> +		 * called soon.

Instead of disabling the IRQ, would it be possible to tell the device 
to stop generating an interrupt request?

> +		 */
> +		disable_irq_nosync(irq);
> +		pm_runtime_get(host->mmc->parent);

Does this pm_runtime_get() have a corresponding pm_runtime_put()?  I 
didn't notice one anywhere.

> +		return IRQ_NONE;
> +	}
>  	spin_lock(&host->lock);
>  
>  	intmask = sdhci_readl(host, SDHCI_INT_STATUS);
> @@ -1747,7 +1762,12 @@ EXPORT_SYMBOL_GPL(sdhci_enable_irq_wakeups);
>  
>  int sdhci_runtime_suspend(struct sdhci_host *host)
>  {
> -	/* nothing to do yet */
> +	u8 val;
> +	/* make sure we wake up on sdcard insert/remove enabled */
> +	val = SDHCI_WAKE_ON_INSERT | SDHCI_WAKE_ON_REMOVE;
> +	if (host->mmc->pm_flags & MMC_PM_WAKE_SDIO_IRQ)
> +		val |= SDHCI_WAKE_ON_INT;
> +	sdhci_writeb(host, val, SDHCI_WAKE_UP_CONTROL);
>  	return 0;
>  }
>  
> @@ -1768,6 +1788,14 @@ int sdhci_runtime_resume(struct sdhci_host *host)
>  	sdhci_set_ios(host->mmc, &host->mmc->ios);
>  	mmiowb();
>  
> +	if (host->waking_up) {
> +		host->waking_up = 0;
> +		/* Re-enable the irq now we are perfectly resumed.
> +		 * Any yet unhandled interrupt (i.e. the wake) will retrigger
> +		 * irq
> +		 */
> +		irq_enable(host->irq);
> +	}
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(sdhci_runtime_resume);

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index e3c7fc0..17aeda3 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1325,8 +1325,8 @@ sub process {
>  		$prefix = "$filename:$realline: " if ($emacs && $file);
>  		$prefix = "$filename:$linenr: " if ($emacs && !$file);
>  
> -		$here = "#$linenr: " if (!$file);
> -		$here = "#$realline: " if ($file);
> +		$here = "" if (!$file);
> +		$here = "" if ($file);
>  
>  		# extract the filename as it passes
>  		if ($line =~ /^diff --git.*?(\S+)$/) {
> @@ -1349,7 +1349,7 @@ sub process {
>  			next;
>  		}
>  
> -		$here .= "FILE: $realfile:$realline:" if ($realcnt != 0);
> +		$here .= "$realfile:$realline:" if ($realcnt != 0);
>  
>  		my $hereline = "$here\n$rawline\n";
>  		my $herecurr = "$here\n$rawline\n";
> 

Surely this doesn't belong in the patch.

Alan Stern

Re: [RFC, PATCHv3 3/3] mmc: sdhci: handle wake-up from runtime_pm

[Alan Stern]

On Tue, 1 Mar 2011, Pierre Tardy wrote:

> When sdhci is runtime_suspended, it can still receive a wake-up
> because of card insertion.
> The wake-up will then be signaled by an interrupt which cannot be serviced
> immediatly, as the device is powered off, and register are not accessibles.
> 
> We cannot call pm_runtime_get_sync() in irq context as on some architecture (e.g. intel_mid),
> runtime_resume can lead to too much latency, due to e.g. PLL warm-up and such aggressive
> power gating.
> 
> We temporarly disable the interrupt, and trigger a runtime_resume of the device
> The interrupt will be re-enabled when resume is finished.
> 
> Signed-off-by: Pierre Tardy <tardyp@gmail.com>
> ---
>  drivers/mmc/host/sdhci-pci.c |    8 +-------
>  drivers/mmc/host/sdhci.c     |   30 +++++++++++++++++++++++++++++-
>  include/linux/mmc/sdhci.h    |    2 ++
>  scripts/checkpatch.pl        |    6 +++---
>  4 files changed, 35 insertions(+), 11 deletions(-)

> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 735c3f7..85a1956 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1599,6 +1599,21 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
>  	u32 intmask;
>  	int cardint = 0;
>  
> +	/* we might come from a sd insertion or sdio irq wake.. */
> +	if (pm_runtime_suspended()) {
> +		host->waking_up = 1;
> +		/* Note that we disable temporarly the interrupt until we do the
> +		 * resume. If we don't then we'll get constantly interrupted
> +		 * until we actually resume.
> +		 *
> +		 * as the irq is shared, this might not be very friendly to our
> +		 * irq sharers but the pm_runtime workqueue should really be
> +		 * called soon.

Instead of disabling the IRQ, would it be possible to tell the device 
to stop generating an interrupt request?

> +		 */
> +		disable_irq_nosync(irq);
> +		pm_runtime_get(host->mmc->parent);

Does this pm_runtime_get() have a corresponding pm_runtime_put()?  I 
didn't notice one anywhere.

> +		return IRQ_NONE;
> +	}
>  	spin_lock(&host->lock);
>  
>  	intmask = sdhci_readl(host, SDHCI_INT_STATUS);
> @@ -1747,7 +1762,12 @@ EXPORT_SYMBOL_GPL(sdhci_enable_irq_wakeups);
>  
>  int sdhci_runtime_suspend(struct sdhci_host *host)
>  {
> -	/* nothing to do yet */
> +	u8 val;
> +	/* make sure we wake up on sdcard insert/remove enabled */
> +	val = SDHCI_WAKE_ON_INSERT | SDHCI_WAKE_ON_REMOVE;
> +	if (host->mmc->pm_flags & MMC_PM_WAKE_SDIO_IRQ)
> +		val |= SDHCI_WAKE_ON_INT;
> +	sdhci_writeb(host, val, SDHCI_WAKE_UP_CONTROL);
>  	return 0;
>  }
>  
> @@ -1768,6 +1788,14 @@ int sdhci_runtime_resume(struct sdhci_host *host)
>  	sdhci_set_ios(host->mmc, &host->mmc->ios);
>  	mmiowb();
>  
> +	if (host->waking_up) {
> +		host->waking_up = 0;
> +		/* Re-enable the irq now we are perfectly resumed.
> +		 * Any yet unhandled interrupt (i.e. the wake) will retrigger
> +		 * irq
> +		 */
> +		irq_enable(host->irq);
> +	}
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(sdhci_runtime_resume);

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index e3c7fc0..17aeda3 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1325,8 +1325,8 @@ sub process {
>  		$prefix = "$filename:$realline: " if ($emacs && $file);
>  		$prefix = "$filename:$linenr: " if ($emacs && !$file);
>  
> -		$here = "#$linenr: " if (!$file);
> -		$here = "#$realline: " if ($file);
> +		$here = "" if (!$file);
> +		$here = "" if ($file);
>  
>  		# extract the filename as it passes
>  		if ($line =~ /^diff --git.*?(\S+)$/) {
> @@ -1349,7 +1349,7 @@ sub process {
>  			next;
>  		}
>  
> -		$here .= "FILE: $realfile:$realline:" if ($realcnt != 0);
> +		$here .= "$realfile:$realline:" if ($realcnt != 0);
>  
>  		my $hereline = "$here\n$rawline\n";
>  		my $herecurr = "$here\n$rawline\n";
> 

Surely this doesn't belong in the patch.

Alan Stern

Re: [linux-pm] [RFC, PATCHv3 3/3] mmc: sdhci: handle wake-up from runtime_pm

[Alan Stern]

On Tue, 1 Mar 2011, Pierre Tardy wrote:

> When sdhci is runtime_suspended, it can still receive a wake-up
> because of card insertion.
> The wake-up will then be signaled by an interrupt which cannot be serviced
> immediatly, as the device is powered off, and register are not accessibles.
> 
> We cannot call pm_runtime_get_sync() in irq context as on some architecture (e.g. intel_mid),
> runtime_resume can lead to too much latency, due to e.g. PLL warm-up and such aggressive
> power gating.
> 
> We temporarly disable the interrupt, and trigger a runtime_resume of the device
> The interrupt will be re-enabled when resume is finished.
> 
> Signed-off-by: Pierre Tardy <tardyp@gmail.com>
> ---
>  drivers/mmc/host/sdhci-pci.c |    8 +-------
>  drivers/mmc/host/sdhci.c     |   30 +++++++++++++++++++++++++++++-
>  include/linux/mmc/sdhci.h    |    2 ++
>  scripts/checkpatch.pl        |    6 +++---
>  4 files changed, 35 insertions(+), 11 deletions(-)

> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 735c3f7..85a1956 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1599,6 +1599,21 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
>  	u32 intmask;
>  	int cardint = 0;
>  
> +	/* we might come from a sd insertion or sdio irq wake.. */
> +	if (pm_runtime_suspended()) {
> +		host->waking_up = 1;
> +		/* Note that we disable temporarly the interrupt until we do the
> +		 * resume. If we don't then we'll get constantly interrupted
> +		 * until we actually resume.
> +		 *
> +		 * as the irq is shared, this might not be very friendly to our
> +		 * irq sharers but the pm_runtime workqueue should really be
> +		 * called soon.

Instead of disabling the IRQ, would it be possible to tell the device 
to stop generating an interrupt request?

> +		 */
> +		disable_irq_nosync(irq);
> +		pm_runtime_get(host->mmc->parent);

Does this pm_runtime_get() have a corresponding pm_runtime_put()?  I 
didn't notice one anywhere.

> +		return IRQ_NONE;
> +	}
>  	spin_lock(&host->lock);
>  
>  	intmask = sdhci_readl(host, SDHCI_INT_STATUS);
> @@ -1747,7 +1762,12 @@ EXPORT_SYMBOL_GPL(sdhci_enable_irq_wakeups);
>  
>  int sdhci_runtime_suspend(struct sdhci_host *host)
>  {
> -	/* nothing to do yet */
> +	u8 val;
> +	/* make sure we wake up on sdcard insert/remove enabled */
> +	val = SDHCI_WAKE_ON_INSERT | SDHCI_WAKE_ON_REMOVE;
> +	if (host->mmc->pm_flags & MMC_PM_WAKE_SDIO_IRQ)
> +		val |= SDHCI_WAKE_ON_INT;
> +	sdhci_writeb(host, val, SDHCI_WAKE_UP_CONTROL);
>  	return 0;
>  }
>  
> @@ -1768,6 +1788,14 @@ int sdhci_runtime_resume(struct sdhci_host *host)
>  	sdhci_set_ios(host->mmc, &host->mmc->ios);
>  	mmiowb();
>  
> +	if (host->waking_up) {
> +		host->waking_up = 0;
> +		/* Re-enable the irq now we are perfectly resumed.
> +		 * Any yet unhandled interrupt (i.e. the wake) will retrigger
> +		 * irq
> +		 */
> +		irq_enable(host->irq);
> +	}
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(sdhci_runtime_resume);

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index e3c7fc0..17aeda3 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1325,8 +1325,8 @@ sub process {
>  		$prefix = "$filename:$realline: " if ($emacs && $file);
>  		$prefix = "$filename:$linenr: " if ($emacs && !$file);
>  
> -		$here = "#$linenr: " if (!$file);
> -		$here = "#$realline: " if ($file);
> +		$here = "" if (!$file);
> +		$here = "" if ($file);
>  
>  		# extract the filename as it passes
>  		if ($line =~ /^diff --git.*?(\S+)$/) {
> @@ -1349,7 +1349,7 @@ sub process {
>  			next;
>  		}
>  
> -		$here .= "FILE: $realfile:$realline:" if ($realcnt != 0);
> +		$here .= "$realfile:$realline:" if ($realcnt != 0);
>  
>  		my $hereline = "$here\n$rawline\n";
>  		my $herecurr = "$here\n$rawline\n";
> 

Surely this doesn't belong in the patch.

Alan Stern

Re: [linux-pm] [RFC,PATCHv3 0/3] sdhci runtime_pm implementation

On Tue, 1 Mar 2011, Pierre Tardy wrote:

> On Tue, Mar 1, 2011 at 8:33 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Tue, 1 Mar 2011, Pierre Tardy wrote:
> >
> >> Please find sdhci runtime_pm implementation.
> >>
> >> It uses clock gating fw as a tip to know when our chip is idle.
> >> It implements wake up from card insertion/removal.
> >>
> >> This is RFC, please dont merge yet. I really would like to have deep review
> >> from PCI linux-pm guys.
> >>
> >> Opens are:
> >>
> >> 1/ Not sure if the pci configs in the driver in rpm_suspend/resume flow
> >>  are not duplicate from what the core is doing.
> >
> > There may be one or two small errors.
> >
> >> 2/ Wakeup from D3hot: I cannot find any driver that is implementing it in current upstream,
> >
> > Other drivers do it, but they use PCI PME# instead of interrupts.
> Could you please elaborate?
> My understanding is that PCI PME will generate MSI, which translate in
> interrupt.

It depends on the platform.  On systems with ACPI, PCI PME generates an 
ACPI I/O event, which is handled by the ACPI and PM cores.  It does not 
invoke the device driver's interrupt handler.

Alan Stern

Re: [RFC,PATCHv3 0/3] sdhci runtime_pm implementation

[Alan Stern]

On Tue, 1 Mar 2011, Pierre Tardy wrote:

> On Tue, Mar 1, 2011 at 8:33 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Tue, 1 Mar 2011, Pierre Tardy wrote:
> >
> >> Please find sdhci runtime_pm implementation.
> >>
> >> It uses clock gating fw as a tip to know when our chip is idle.
> >> It implements wake up from card insertion/removal.
> >>
> >> This is RFC, please dont merge yet. I really would like to have deep review
> >> from PCI linux-pm guys.
> >>
> >> Opens are:
> >>
> >> 1/ Not sure if the pci configs in the driver in rpm_suspend/resume flow
> >>  are not duplicate from what the core is doing.
> >
> > There may be one or two small errors.
> >
> >> 2/ Wakeup from D3hot: I cannot find any driver that is implementing it in current upstream,
> >
> > Other drivers do it, but they use PCI PME# instead of interrupts.
> Could you please elaborate?
> My understanding is that PCI PME will generate MSI, which translate in
> interrupt.

It depends on the platform.  On systems with ACPI, PCI PME generates an 
ACPI I/O event, which is handled by the ACPI and PM cores.  It does not 
invoke the device driver's interrupt handler.

Alan Stern

Re: [linux-pm] [RFC,PATCHv3 0/3] sdhci runtime_pm implementation

[Alan Stern]

On Tue, 1 Mar 2011, Pierre Tardy wrote:

> On Tue, Mar 1, 2011 at 8:33 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Tue, 1 Mar 2011, Pierre Tardy wrote:
> >
> >> Please find sdhci runtime_pm implementation.
> >>
> >> It uses clock gating fw as a tip to know when our chip is idle.
> >> It implements wake up from card insertion/removal.
> >>
> >> This is RFC, please dont merge yet. I really would like to have deep review
> >> from PCI linux-pm guys.
> >>
> >> Opens are:
> >>
> >> 1/ Not sure if the pci configs in the driver in rpm_suspend/resume flow
> >>  are not duplicate from what the core is doing.
> >
> > There may be one or two small errors.
> >
> >> 2/ Wakeup from D3hot: I cannot find any driver that is implementing it in current upstream,
> >
> > Other drivers do it, but they use PCI PME# instead of interrupts.
> Could you please elaborate?
> My understanding is that PCI PME will generate MSI, which translate in
> interrupt.

It depends on the platform.  On systems with ACPI, PCI PME generates an 
ACPI I/O event, which is handled by the ACPI and PM cores.  It does not 
invoke the device driver's interrupt handler.

Alan Stern

Re: [linux-pm] [RFC, PATCHv3 3/3] mmc: sdhci: handle wake-up from runtime_pm

[Pierre Tardy]

>> +     if (pm_runtime_suspended()) {
>> +             host->waking_up = 1;
>> +             /* Note that we disable temporarly the interrupt until we do the
>> +              * resume. If we don't then we'll get constantly interrupted
>> +              * until we actually resume.
>> +              *
>> +              * as the irq is shared, this might not be very friendly to our
>> +              * irq sharers but the pm_runtime workqueue should really be
>> +              * called soon.
>
> Instead of disabling the IRQ, would it be possible to tell the device
> to stop generating an interrupt request?

Well, the device is power gated, so any access to it will cause a bus
timeout (depending of the platform)

>> +              */
>> +             disable_irq_nosync(irq);
>> +             pm_runtime_get(host->mmc->parent);
>
> Does this pm_runtime_get() have a corresponding pm_runtime_put()?  I
> didn't notice one anywhere.
oups, forgot to add the corresponding snippet in sdhci_runtime_resume:
if (host->waking_up) {
    pm_runtime_put();
    host->waking_up = 0;
}

>>
>
> Surely this doesn't belong in the patch.
yep sorry, I should stop doing git commit -a...

Pierre

Re: [RFC, PATCHv3 3/3] mmc: sdhci: handle wake-up from runtime_pm

[Pierre Tardy]

>> +     if (pm_runtime_suspended()) {
>> +             host->waking_up = 1;
>> +             /* Note that we disable temporarly the interrupt until we do the
>> +              * resume. If we don't then we'll get constantly interrupted
>> +              * until we actually resume.
>> +              *
>> +              * as the irq is shared, this might not be very friendly to our
>> +              * irq sharers but the pm_runtime workqueue should really be
>> +              * called soon.
>
> Instead of disabling the IRQ, would it be possible to tell the device
> to stop generating an interrupt request?

Well, the device is power gated, so any access to it will cause a bus
timeout (depending of the platform)

>> +              */
>> +             disable_irq_nosync(irq);
>> +             pm_runtime_get(host->mmc->parent);
>
> Does this pm_runtime_get() have a corresponding pm_runtime_put()?  I
> didn't notice one anywhere.
oups, forgot to add the corresponding snippet in sdhci_runtime_resume:
if (host->waking_up) {
    pm_runtime_put();
    host->waking_up = 0;
}

>>
>
> Surely this doesn't belong in the patch.
yep sorry, I should stop doing git commit -a...

Pierre

Re: [linux-pm] [RFC,PATCHv3 0/3] sdhci runtime_pm implementation

[Pierre Tardy]

On Tue, Mar 1, 2011 at 8:57 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 1 Mar 2011, Pierre Tardy wrote:
>
>> On Tue, Mar 1, 2011 at 8:33 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Tue, 1 Mar 2011, Pierre Tardy wrote:
>> >
>> >> Please find sdhci runtime_pm implementation.
>> >>
>> >> It uses clock gating fw as a tip to know when our chip is idle.
>> >> It implements wake up from card insertion/removal.
>> >>
>> >> This is RFC, please dont merge yet. I really would like to have deep review
>> >> from PCI linux-pm guys.
>> >>
>> >> Opens are:
>> >>
>> >> 1/ Not sure if the pci configs in the driver in rpm_suspend/resume flow
>> >>  are not duplicate from what the core is doing.
>> >
>> > There may be one or two small errors.
>> >
>> >> 2/ Wakeup from D3hot: I cannot find any driver that is implementing it in current upstream,
>> >
>> > Other drivers do it, but they use PCI PME# instead of interrupts.
>> Could you please elaborate?
>> My understanding is that PCI PME will generate MSI, which translate in
>> interrupt.
>
> It depends on the platform.  On systems with ACPI, PCI PME generates an
> ACPI I/O event, which is handled by the ACPI and PM cores.  It does not
> invoke the device driver's interrupt handler.
So, let's say, in the ACPI case, if the interrupt handler dont get
called, how would the driver know that he got a sdcard insert event,
and trigger a mmc_rescan() ?

Regards,
Pierre

Re: [RFC,PATCHv3 0/3] sdhci runtime_pm implementation

[Pierre Tardy]

On Tue, Mar 1, 2011 at 8:57 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 1 Mar 2011, Pierre Tardy wrote:
>
>> On Tue, Mar 1, 2011 at 8:33 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Tue, 1 Mar 2011, Pierre Tardy wrote:
>> >
>> >> Please find sdhci runtime_pm implementation.
>> >>
>> >> It uses clock gating fw as a tip to know when our chip is idle.
>> >> It implements wake up from card insertion/removal.
>> >>
>> >> This is RFC, please dont merge yet. I really would like to have deep review
>> >> from PCI linux-pm guys.
>> >>
>> >> Opens are:
>> >>
>> >> 1/ Not sure if the pci configs in the driver in rpm_suspend/resume flow
>> >>  are not duplicate from what the core is doing.
>> >
>> > There may be one or two small errors.
>> >
>> >> 2/ Wakeup from D3hot: I cannot find any driver that is implementing it in current upstream,
>> >
>> > Other drivers do it, but they use PCI PME# instead of interrupts.
>> Could you please elaborate?
>> My understanding is that PCI PME will generate MSI, which translate in
>> interrupt.
>
> It depends on the platform.  On systems with ACPI, PCI PME generates an
> ACPI I/O event, which is handled by the ACPI and PM cores.  It does not
> invoke the device driver's interrupt handler.
So, let's say, in the ACPI case, if the interrupt handler dont get
called, how would the driver know that he got a sdcard insert event,
and trigger a mmc_rescan() ?

Regards,
Pierre

Re: [linux-pm] [RFC, PATCHv3 3/3] mmc: sdhci: handle wake-up from runtime_pm

[Alan Stern]

On Tue, 1 Mar 2011, Pierre Tardy wrote:

> >> +     if (pm_runtime_suspended()) {
> >> +             host->waking_up = 1;
> >> +             /* Note that we disable temporarly the interrupt until we do the
> >> +              * resume. If we don't then we'll get constantly interrupted
> >> +              * until we actually resume.
> >> +              *
> >> +              * as the irq is shared, this might not be very friendly to our
> >> +              * irq sharers but the pm_runtime workqueue should really be
> >> +              * called soon.
> >
> > Instead of disabling the IRQ, would it be possible to tell the device
> > to stop generating an interrupt request?
> 
> Well, the device is power gated, so any access to it will cause a bus
> timeout (depending of the platform)

But could you gate the power back on and then turn off the interrupt 
request?

It seems like a foolish design to have an interrupt request that can't
be turned off by an interrupt handler.

Alan Stern

Re: [RFC, PATCHv3 3/3] mmc: sdhci: handle wake-up from runtime_pm

[Alan Stern]

On Tue, 1 Mar 2011, Pierre Tardy wrote:

> >> +     if (pm_runtime_suspended()) {
> >> +             host->waking_up = 1;
> >> +             /* Note that we disable temporarly the interrupt until we do the
> >> +              * resume. If we don't then we'll get constantly interrupted
> >> +              * until we actually resume.
> >> +              *
> >> +              * as the irq is shared, this might not be very friendly to our
> >> +              * irq sharers but the pm_runtime workqueue should really be
> >> +              * called soon.
> >
> > Instead of disabling the IRQ, would it be possible to tell the device
> > to stop generating an interrupt request?
> 
> Well, the device is power gated, so any access to it will cause a bus
> timeout (depending of the platform)

But could you gate the power back on and then turn off the interrupt 
request?

It seems like a foolish design to have an interrupt request that can't
be turned off by an interrupt handler.

Alan Stern

Re: [linux-pm] [RFC, PATCHv3 3/3] mmc: sdhci: handle wake-up from runtime_pm

[Alan Stern]

On Tue, 1 Mar 2011, Pierre Tardy wrote:

> >> +     if (pm_runtime_suspended()) {
> >> +             host->waking_up = 1;
> >> +             /* Note that we disable temporarly the interrupt until we do the
> >> +              * resume. If we don't then we'll get constantly interrupted
> >> +              * until we actually resume.
> >> +              *
> >> +              * as the irq is shared, this might not be very friendly to our
> >> +              * irq sharers but the pm_runtime workqueue should really be
> >> +              * called soon.
> >
> > Instead of disabling the IRQ, would it be possible to tell the device
> > to stop generating an interrupt request?
> 
> Well, the device is power gated, so any access to it will cause a bus
> timeout (depending of the platform)

But could you gate the power back on and then turn off the interrupt 
request?

It seems like a foolish design to have an interrupt request that can't
be turned off by an interrupt handler.

Alan Stern

Re: [linux-pm] [RFC,PATCHv3 0/3] sdhci runtime_pm implementation

[Alan Stern]

On Tue, 1 Mar 2011, Pierre Tardy wrote:

> >> > Other drivers do it, but they use PCI PME# instead of interrupts.
> >> Could you please elaborate?
> >> My understanding is that PCI PME will generate MSI, which translate in
> >> interrupt.
> >
> > It depends on the platform.  On systems with ACPI, PCI PME generates an
> > ACPI I/O event, which is handled by the ACPI and PM cores.  It does not
> > invoke the device driver's interrupt handler.
> So, let's say, in the ACPI case, if the interrupt handler dont get
> called, how would the driver know that he got a sdcard insert event,
> and trigger a mmc_rescan() ?

When the ACPI and PCI cores process the PME event, they call
pm_runtime_resume().  The driver's runtime_resume routine would read
the device status and see the insert event, which would cause it to
trigger mmc_rescan().

Alan Stern

Re: [RFC,PATCHv3 0/3] sdhci runtime_pm implementation

[Alan Stern]

On Tue, 1 Mar 2011, Pierre Tardy wrote:

> >> > Other drivers do it, but they use PCI PME# instead of interrupts.
> >> Could you please elaborate?
> >> My understanding is that PCI PME will generate MSI, which translate in
> >> interrupt.
> >
> > It depends on the platform.  On systems with ACPI, PCI PME generates an
> > ACPI I/O event, which is handled by the ACPI and PM cores.  It does not
> > invoke the device driver's interrupt handler.
> So, let's say, in the ACPI case, if the interrupt handler dont get
> called, how would the driver know that he got a sdcard insert event,
> and trigger a mmc_rescan() ?

When the ACPI and PCI cores process the PME event, they call
pm_runtime_resume().  The driver's runtime_resume routine would read
the device status and see the insert event, which would cause it to
trigger mmc_rescan().

Alan Stern

Re: [linux-pm] [RFC,PATCHv3 0/3] sdhci runtime_pm implementation

[Alan Stern]

On Tue, 1 Mar 2011, Pierre Tardy wrote:

> >> > Other drivers do it, but they use PCI PME# instead of interrupts.
> >> Could you please elaborate?
> >> My understanding is that PCI PME will generate MSI, which translate in
> >> interrupt.
> >
> > It depends on the platform.  On systems with ACPI, PCI PME generates an
> > ACPI I/O event, which is handled by the ACPI and PM cores.  It does not
> > invoke the device driver's interrupt handler.
> So, let's say, in the ACPI case, if the interrupt handler dont get
> called, how would the driver know that he got a sdcard insert event,
> and trigger a mmc_rescan() ?

When the ACPI and PCI cores process the PME event, they call
pm_runtime_resume().  The driver's runtime_resume routine would read
the device status and see the insert event, which would cause it to
trigger mmc_rescan().

Alan Stern

Re: [linux-pm] [RFC, PATCHv3 3/3] mmc: sdhci: handle wake-up from runtime_pm

[Pierre Tardy]

On Tue, Mar 1, 2011 at 9:27 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 1 Mar 2011, Pierre Tardy wrote:
>
>> >> +     if (pm_runtime_suspended()) {
>> >> +             host->waking_up = 1;
>> >> +             /* Note that we disable temporarly the interrupt until we do the
>> >> +              * resume. If we don't then we'll get constantly interrupted
>> >> +              * until we actually resume.
>> >> +              *
>> >> +              * as the irq is shared, this might not be very friendly to our
>> >> +              * irq sharers but the pm_runtime workqueue should really be
>> >> +              * called soon.
>> >
>> > Instead of disabling the IRQ, would it be possible to tell the device
>> > to stop generating an interrupt request?
>>
>> Well, the device is power gated, so any access to it will cause a bus
>> timeout (depending of the platform)
>
> But could you gate the power back on and then turn off the interrupt
> request?
That's the role of runtime_resume... On some platform, this may need
up to sending some SPI commands to the PMIC, to get those power rails
back on.
Something that you cannot do in interrupt context.

>
> It seems like a foolish design to have an interrupt request that can't
> be turned off by an interrupt handler.
It's what we have for now ( looks like I need to go back to my system
architects to see if we can change that...)

Alan,
thanks for all this feedback. I have now a much more clear view on how
this is working on ACPI, I'll try to see how we can mimic this
behaviour on intel_mid.

Re: [RFC, PATCHv3 3/3] mmc: sdhci: handle wake-up from runtime_pm

[Pierre Tardy]

On Tue, Mar 1, 2011 at 9:27 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 1 Mar 2011, Pierre Tardy wrote:
>
>> >> +     if (pm_runtime_suspended()) {
>> >> +             host->waking_up = 1;
>> >> +             /* Note that we disable temporarly the interrupt until we do the
>> >> +              * resume. If we don't then we'll get constantly interrupted
>> >> +              * until we actually resume.
>> >> +              *
>> >> +              * as the irq is shared, this might not be very friendly to our
>> >> +              * irq sharers but the pm_runtime workqueue should really be
>> >> +              * called soon.
>> >
>> > Instead of disabling the IRQ, would it be possible to tell the device
>> > to stop generating an interrupt request?
>>
>> Well, the device is power gated, so any access to it will cause a bus
>> timeout (depending of the platform)
>
> But could you gate the power back on and then turn off the interrupt
> request?
That's the role of runtime_resume... On some platform, this may need
up to sending some SPI commands to the PMIC, to get those power rails
back on.
Something that you cannot do in interrupt context.

>
> It seems like a foolish design to have an interrupt request that can't
> be turned off by an interrupt handler.
It's what we have for now ( looks like I need to go back to my system
architects to see if we can change that...)

Alan,
thanks for all this feedback. I have now a much more clear view on how
this is working on ACPI, I'll try to see how we can mimic this
behaviour on intel_mid.

Re: [linux-pm] [RFC,PATCHv3 0/3] sdhci runtime_pm implementation

[Rafael J. Wysocki]

On Tuesday, March 01, 2011, Pierre Tardy wrote:
> On Tue, Mar 1, 2011 at 8:33 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Tue, 1 Mar 2011, Pierre Tardy wrote:
> >
> >> Please find sdhci runtime_pm implementation.
> >>
> >> It uses clock gating fw as a tip to know when our chip is idle.
> >> It implements wake up from card insertion/removal.
> >>
> >> This is RFC, please dont merge yet. I really would like to have deep review
> >> from PCI linux-pm guys.
> >>
> >> Opens are:
> >>
> >> 1/ Not sure if the pci configs in the driver in rpm_suspend/resume flow
> >>  are not duplicate from what the core is doing.
> >
> > There may be one or two small errors.
> >
> >> 2/ Wakeup from D3hot: I cannot find any driver that is implementing it in current upstream,
> >
> > Other drivers do it, but they use PCI PME# instead of interrupts.
> Could you please elaborate?
> My understanding is that PCI PME will generate MSI, which translate in
> interrupt.

Your driver won't get those interrupts.

How it works is, basically, that when the device signals wakeup, it either
causes a PME# signal to be raised (parallel PCI), or a PME Message to be
sent upstream (PCIe).  In the first case it will cause a platform event
(eg. ACPI GPE) to occur and the handle of that event will resume your
device (using pm_request_resume()).  In the second case it will cause the PCIe
root port handling the PME Message to generate an interrupt and the handler of
that interrupt will resume your device.

And this is a good reason why your driver shouldn't touch the PCI-specific
part of suspend/resume handling (it may not really know what it's doing).

In fact, there are a few drivers in the tree using this mechanism already
(r8169, e1000e, i believe PCI HCDs too).

Thanks,
Rafael

Re: [RFC,PATCHv3 0/3] sdhci runtime_pm implementation

[Rafael J. Wysocki]

On Tuesday, March 01, 2011, Pierre Tardy wrote:
> On Tue, Mar 1, 2011 at 8:33 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Tue, 1 Mar 2011, Pierre Tardy wrote:
> >
> >> Please find sdhci runtime_pm implementation.
> >>
> >> It uses clock gating fw as a tip to know when our chip is idle.
> >> It implements wake up from card insertion/removal.
> >>
> >> This is RFC, please dont merge yet. I really would like to have deep review
> >> from PCI linux-pm guys.
> >>
> >> Opens are:
> >>
> >> 1/ Not sure if the pci configs in the driver in rpm_suspend/resume flow
> >>  are not duplicate from what the core is doing.
> >
> > There may be one or two small errors.
> >
> >> 2/ Wakeup from D3hot: I cannot find any driver that is implementing it in current upstream,
> >
> > Other drivers do it, but they use PCI PME# instead of interrupts.
> Could you please elaborate?
> My understanding is that PCI PME will generate MSI, which translate in
> interrupt.

Your driver won't get those interrupts.

How it works is, basically, that when the device signals wakeup, it either
causes a PME# signal to be raised (parallel PCI), or a PME Message to be
sent upstream (PCIe).  In the first case it will cause a platform event
(eg. ACPI GPE) to occur and the handle of that event will resume your
device (using pm_request_resume()).  In the second case it will cause the PCIe
root port handling the PME Message to generate an interrupt and the handler of
that interrupt will resume your device.

And this is a good reason why your driver shouldn't touch the PCI-specific
part of suspend/resume handling (it may not really know what it's doing).

In fact, there are a few drivers in the tree using this mechanism already
(r8169, e1000e, i believe PCI HCDs too).

Thanks,
Rafael

Re: [linux-pm] [RFC,PATCHv3 0/3] sdhci runtime_pm implementation

[Pierre Tardy]

On Tue, Mar 1, 2011 at 10:07 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Tuesday, March 01, 2011, Pierre Tardy wrote:
>> On Tue, Mar 1, 2011 at 8:33 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Tue, 1 Mar 2011, Pierre Tardy wrote:
>> >
>> >> Please find sdhci runtime_pm implementation.
>> >>
>> >> It uses clock gating fw as a tip to know when our chip is idle.
>> >> It implements wake up from card insertion/removal.
>> >>
>> >> This is RFC, please dont merge yet. I really would like to have deep review
>> >> from PCI linux-pm guys.
>> >>
>> >> Opens are:
>> >>
>> >> 1/ Not sure if the pci configs in the driver in rpm_suspend/resume flow
>> >>  are not duplicate from what the core is doing.
>> >
>> > There may be one or two small errors.
>> >
>> >> 2/ Wakeup from D3hot: I cannot find any driver that is implementing it in current upstream,
>> >
>> > Other drivers do it, but they use PCI PME# instead of interrupts.
>> Could you please elaborate?
>> My understanding is that PCI PME will generate MSI, which translate in
>> interrupt.
>
> Your driver won't get those interrupts.
>
> How it works is, basically, that when the device signals wakeup, it either
> causes a PME# signal to be raised (parallel PCI), or a PME Message to be
> sent upstream (PCIe).  In the first case it will cause a platform event
> (eg. ACPI GPE) to occur and the handle of that event will resume your
> device (using pm_request_resume()).  In the second case it will cause the PCIe
> root port handling the PME Message to generate an interrupt and the handler of
> that interrupt will resume your device.
Thanks, that explain a lot how it works.
What I still dont understand is that the wake source I'll have (e.g.
sd card insert) is still an interrupt source.
So it is supposed to generate interrupt until the interrupt source is
acknowledged.
Are we supposed to mask that interrupt source while suspended to make
sure we have either wake or interrupt?

>
> In fact, there are a few drivers in the tree using this mechanism already
> (r8169, e1000e, i believe PCI HCDs too).
Hope there will be more and more.

thanks,
Pierre

Re: [RFC,PATCHv3 0/3] sdhci runtime_pm implementation

[Pierre Tardy]

On Tue, Mar 1, 2011 at 10:07 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Tuesday, March 01, 2011, Pierre Tardy wrote:
>> On Tue, Mar 1, 2011 at 8:33 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Tue, 1 Mar 2011, Pierre Tardy wrote:
>> >
>> >> Please find sdhci runtime_pm implementation.
>> >>
>> >> It uses clock gating fw as a tip to know when our chip is idle.
>> >> It implements wake up from card insertion/removal.
>> >>
>> >> This is RFC, please dont merge yet. I really would like to have deep review
>> >> from PCI linux-pm guys.
>> >>
>> >> Opens are:
>> >>
>> >> 1/ Not sure if the pci configs in the driver in rpm_suspend/resume flow
>> >>  are not duplicate from what the core is doing.
>> >
>> > There may be one or two small errors.
>> >
>> >> 2/ Wakeup from D3hot: I cannot find any driver that is implementing it in current upstream,
>> >
>> > Other drivers do it, but they use PCI PME# instead of interrupts.
>> Could you please elaborate?
>> My understanding is that PCI PME will generate MSI, which translate in
>> interrupt.
>
> Your driver won't get those interrupts.
>
> How it works is, basically, that when the device signals wakeup, it either
> causes a PME# signal to be raised (parallel PCI), or a PME Message to be
> sent upstream (PCIe).  In the first case it will cause a platform event
> (eg. ACPI GPE) to occur and the handle of that event will resume your
> device (using pm_request_resume()).  In the second case it will cause the PCIe
> root port handling the PME Message to generate an interrupt and the handler of
> that interrupt will resume your device.
Thanks, that explain a lot how it works.
What I still dont understand is that the wake source I'll have (e.g.
sd card insert) is still an interrupt source.
So it is supposed to generate interrupt until the interrupt source is
acknowledged.
Are we supposed to mask that interrupt source while suspended to make
sure we have either wake or interrupt?

>
> In fact, there are a few drivers in the tree using this mechanism already
> (r8169, e1000e, i believe PCI HCDs too).
Hope there will be more and more.

thanks,
Pierre

Re: [linux-pm] [RFC,PATCHv3 0/3] sdhci runtime_pm implementation

[Rafael J. Wysocki]

On Wednesday, March 02, 2011, Pierre Tardy wrote:
> On Tue, Mar 1, 2011 at 10:07 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Tuesday, March 01, 2011, Pierre Tardy wrote:
> >> On Tue, Mar 1, 2011 at 8:33 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> >> > On Tue, 1 Mar 2011, Pierre Tardy wrote:
> >> >
> >> >> Please find sdhci runtime_pm implementation.
> >> >>
> >> >> It uses clock gating fw as a tip to know when our chip is idle.
> >> >> It implements wake up from card insertion/removal.
> >> >>
> >> >> This is RFC, please dont merge yet. I really would like to have deep review
> >> >> from PCI linux-pm guys.
> >> >>
> >> >> Opens are:
> >> >>
> >> >> 1/ Not sure if the pci configs in the driver in rpm_suspend/resume flow
> >> >>  are not duplicate from what the core is doing.
> >> >
> >> > There may be one or two small errors.
> >> >
> >> >> 2/ Wakeup from D3hot: I cannot find any driver that is implementing it in current upstream,
> >> >
> >> > Other drivers do it, but they use PCI PME# instead of interrupts.
> >> Could you please elaborate?
> >> My understanding is that PCI PME will generate MSI, which translate in
> >> interrupt.
> >
> > Your driver won't get those interrupts.
> >
> > How it works is, basically, that when the device signals wakeup, it either
> > causes a PME# signal to be raised (parallel PCI), or a PME Message to be
> > sent upstream (PCIe).  In the first case it will cause a platform event
> > (eg. ACPI GPE) to occur and the handle of that event will resume your
> > device (using pm_request_resume()).  In the second case it will cause the PCIe
> > root port handling the PME Message to generate an interrupt and the handler of
> > that interrupt will resume your device.
> Thanks, that explain a lot how it works.
> What I still dont understand is that the wake source I'll have (e.g.
> sd card insert) is still an interrupt source.
> So it is supposed to generate interrupt until the interrupt source is
> acknowledged.
> Are we supposed to mask that interrupt source while suspended to make
> sure we have either wake or interrupt?

I think the interrupt should be masked while suspended and the driver's resume
routine should unmask it.

Thanks,
Rafael

Re: [RFC,PATCHv3 0/3] sdhci runtime_pm implementation

[Rafael J. Wysocki]

On Wednesday, March 02, 2011, Pierre Tardy wrote:
> On Tue, Mar 1, 2011 at 10:07 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Tuesday, March 01, 2011, Pierre Tardy wrote:
> >> On Tue, Mar 1, 2011 at 8:33 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> >> > On Tue, 1 Mar 2011, Pierre Tardy wrote:
> >> >
> >> >> Please find sdhci runtime_pm implementation.
> >> >>
> >> >> It uses clock gating fw as a tip to know when our chip is idle.
> >> >> It implements wake up from card insertion/removal.
> >> >>
> >> >> This is RFC, please dont merge yet. I really would like to have deep review
> >> >> from PCI linux-pm guys.
> >> >>
> >> >> Opens are:
> >> >>
> >> >> 1/ Not sure if the pci configs in the driver in rpm_suspend/resume flow
> >> >>  are not duplicate from what the core is doing.
> >> >
> >> > There may be one or two small errors.
> >> >
> >> >> 2/ Wakeup from D3hot: I cannot find any driver that is implementing it in current upstream,
> >> >
> >> > Other drivers do it, but they use PCI PME# instead of interrupts.
> >> Could you please elaborate?
> >> My understanding is that PCI PME will generate MSI, which translate in
> >> interrupt.
> >
> > Your driver won't get those interrupts.
> >
> > How it works is, basically, that when the device signals wakeup, it either
> > causes a PME# signal to be raised (parallel PCI), or a PME Message to be
> > sent upstream (PCIe).  In the first case it will cause a platform event
> > (eg. ACPI GPE) to occur and the handle of that event will resume your
> > device (using pm_request_resume()).  In the second case it will cause the PCIe
> > root port handling the PME Message to generate an interrupt and the handler of
> > that interrupt will resume your device.
> Thanks, that explain a lot how it works.
> What I still dont understand is that the wake source I'll have (e.g.
> sd card insert) is still an interrupt source.
> So it is supposed to generate interrupt until the interrupt source is
> acknowledged.
> Are we supposed to mask that interrupt source while suspended to make
> sure we have either wake or interrupt?

I think the interrupt should be masked while suspended and the driver's resume
routine should unmask it.

Thanks,
Rafael

Re: [linux-pm] [RFC,PATCHv3 0/3] sdhci runtime_pm implementation

[Alan Stern]

On Wed, 2 Mar 2011, Rafael J. Wysocki wrote:

> > > How it works is, basically, that when the device signals wakeup, it either
> > > causes a PME# signal to be raised (parallel PCI), or a PME Message to be
> > > sent upstream (PCIe).  In the first case it will cause a platform event
> > > (eg. ACPI GPE) to occur and the handle of that event will resume your
> > > device (using pm_request_resume()).  In the second case it will cause the PCIe
> > > root port handling the PME Message to generate an interrupt and the handler of
> > > that interrupt will resume your device.
> > Thanks, that explain a lot how it works.
> > What I still dont understand is that the wake source I'll have (e.g.
> > sd card insert) is still an interrupt source.
> > So it is supposed to generate interrupt until the interrupt source is
> > acknowledged.
> > Are we supposed to mask that interrupt source while suspended to make
> > sure we have either wake or interrupt?
> 
> I think the interrupt should be masked while suspended and the driver's resume
> routine should unmask it.

Right.  That's how the PCI-based USB host controller drivers work.  
The suspend routine disables interrupt generation and enables PME
wakeup signals, and the resume routine does the reverse.

Alan Stern

Re: [RFC,PATCHv3 0/3] sdhci runtime_pm implementation

[Alan Stern]

On Wed, 2 Mar 2011, Rafael J. Wysocki wrote:

> > > How it works is, basically, that when the device signals wakeup, it either
> > > causes a PME# signal to be raised (parallel PCI), or a PME Message to be
> > > sent upstream (PCIe).  In the first case it will cause a platform event
> > > (eg. ACPI GPE) to occur and the handle of that event will resume your
> > > device (using pm_request_resume()).  In the second case it will cause the PCIe
> > > root port handling the PME Message to generate an interrupt and the handler of
> > > that interrupt will resume your device.
> > Thanks, that explain a lot how it works.
> > What I still dont understand is that the wake source I'll have (e.g.
> > sd card insert) is still an interrupt source.
> > So it is supposed to generate interrupt until the interrupt source is
> > acknowledged.
> > Are we supposed to mask that interrupt source while suspended to make
> > sure we have either wake or interrupt?
> 
> I think the interrupt should be masked while suspended and the driver's resume
> routine should unmask it.

Right.  That's how the PCI-based USB host controller drivers work.  
The suspend routine disables interrupt generation and enables PME
wakeup signals, and the resume routine does the reverse.

Alan Stern

Re: [linux-pm] [RFC,PATCHv3 0/3] sdhci runtime_pm implementation

[Alan Stern]

On Wed, 2 Mar 2011, Rafael J. Wysocki wrote:

> > > How it works is, basically, that when the device signals wakeup, it either
> > > causes a PME# signal to be raised (parallel PCI), or a PME Message to be
> > > sent upstream (PCIe).  In the first case it will cause a platform event
> > > (eg. ACPI GPE) to occur and the handle of that event will resume your
> > > device (using pm_request_resume()).  In the second case it will cause the PCIe
> > > root port handling the PME Message to generate an interrupt and the handler of
> > > that interrupt will resume your device.
> > Thanks, that explain a lot how it works.
> > What I still dont understand is that the wake source I'll have (e.g.
> > sd card insert) is still an interrupt source.
> > So it is supposed to generate interrupt until the interrupt source is
> > acknowledged.
> > Are we supposed to mask that interrupt source while suspended to make
> > sure we have either wake or interrupt?
> 
> I think the interrupt should be masked while suspended and the driver's resume
> routine should unmask it.

Right.  That's how the PCI-based USB host controller drivers work.  
The suspend routine disables interrupt generation and enables PME
wakeup signals, and the resume routine does the reverse.

Alan Stern