[PATCH] sdhci: wakeup from runtime PM

[PATCH] sdhci: wakeup from runtime PM

[Ludovic Desroches]

Hi,

When not using the SDHCI controller, it is logical to save power by suspending
it. The issue is that SDHCI assumes that the controller is completely disabled.
It means the only way to wake up on a card event is to have a gpio for the card
detection (so two pins for the same signal). A possible workaround is to use
polling but the controller will be resumed/suspended between each attempts.

We have already discussed a long time about this and it seems we don't agree.
In my opinion, even if I can't disable all clocks, I should use runtime PM 
to save some power.

I propose two patches, one which is a draft to try to solve it at sdhci level
and one at sdhci-of-at91 level.

Concerning the first one, I don't understand why we need to reject irqs if
runtime_suspended is true. Only SDHCI_INT_CARD_INT irq is enabled so why we
should have other IRQs than this one?

Since you were not in favour of allowing to wakeup on SDHCI_INT_CARD_INSERT or
SDHCI_INT_CARD_REMOVE, I assume you won't take it so I
solved my issue only by modifying my driver.

I have not noticed side effects. I have compared a wakeup with a gpio for card
detection and the controller card detect. It seems the two paths are rougly
the same (mmc_detect_change -> mmc_rescan then resume is performed).

Regards

Ludovic

[PATCH] sdhci: wakeup from runtime PM

[Ludovic Desroches]

Hi,

When not using the SDHCI controller, it is logical to save power by suspending
it. The issue is that SDHCI assumes that the controller is completely disabled.
It means the only way to wake up on a card event is to have a gpio for the card
detection (so two pins for the same signal). A possible workaround is to use
polling but the controller will be resumed/suspended between each attempts.

We have already discussed a long time about this and it seems we don't agree.
In my opinion, even if I can't disable all clocks, I should use runtime PM 
to save some power.

I propose two patches, one which is a draft to try to solve it at sdhci level
and one at sdhci-of-at91 level.

Concerning the first one, I don't understand why we need to reject irqs if
runtime_suspended is true. Only SDHCI_INT_CARD_INT irq is enabled so why we
should have other IRQs than this one?

Since you were not in favour of allowing to wakeup on SDHCI_INT_CARD_INSERT or
SDHCI_INT_CARD_REMOVE, I assume you won't take it so I
solved my issue only by modifying my driver.

I have not noticed side effects. I have compared a wakeup with a gpio for card
detection and the controller card detect. It seems the two paths are rougly
the same (mmc_detect_change -> mmc_rescan then resume is performed).

Regards

Ludovic

[PATCH] DRAFT: shdci: allows custom wakeup irqs for runtime PM

[Ludovic Desroches]

Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
---
 drivers/mmc/host/sdhci-of-at91.c | 36 ++++++++++--------------------------
 drivers/mmc/host/sdhci.c         | 12 +++++-------
 drivers/mmc/host/sdhci.h         |  2 +-
 3 files changed, 16 insertions(+), 34 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
index 2703aa9..d70bb7a 100644
--- a/drivers/mmc/host/sdhci-of-at91.c
+++ b/drivers/mmc/host/sdhci-of-at91.c
@@ -62,12 +62,14 @@ static int sdhci_at91_runtime_suspend(struct device *dev)
 	struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
 	int ret;
 
-	ret = sdhci_runtime_suspend_host(host);
+	ret = sdhci_runtime_suspend_host(host, SDHCI_INT_CARD_INT | SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE);
 
 	clk_disable_unprepare(priv->gck);
-	clk_disable_unprepare(priv->hclock);
+	//clk_disable_unprepare(priv->hclock);
 	clk_disable_unprepare(priv->mainck);
 
+	printk("sdhci_at91_runtime_suspend\n");
+
 	return ret;
 }
 
@@ -78,17 +80,18 @@ static int sdhci_at91_runtime_resume(struct device *dev)
 	struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
 	int ret;
 
+	printk("sdhci_at91_runtime_resume\n");
 	ret = clk_prepare_enable(priv->mainck);
 	if (ret) {
 		dev_err(dev, "can't enable mainck\n");
 		return ret;
 	}
 
-	ret = clk_prepare_enable(priv->hclock);
-	if (ret) {
-		dev_err(dev, "can't enable hclock\n");
-		return ret;
-	}
+	//ret = clk_prepare_enable(priv->hclock);
+	//if (ret) {
+	//	dev_err(dev, "can't enable hclock\n");
+	//	return ret;
+	//}
 
 	ret = clk_prepare_enable(priv->gck);
 	if (ret) {
@@ -205,25 +208,6 @@ static int sdhci_at91_probe(struct platform_device *pdev)
 	if (ret)
 		goto pm_runtime_disable;
 
-	/*
-	 * When calling sdhci_runtime_suspend_host(), the sdhci layer makes
-	 * the assumption that all the clocks of the controller are disabled.
-	 * It means we can't get irq from it when it is runtime suspended.
-	 * For that reason, it is not planned to wake-up on a card detect irq
-	 * from the controller.
-	 * If we want to use runtime PM and to be able to wake-up on card
-	 * insertion, we have to use a GPIO for the card detection or we can
-	 * use polling. Be aware that using polling will resume/suspend the
-	 * controller between each attempt.
-	 * Disable SDHCI_QUIRK_BROKEN_CARD_DETECTION to be sure nobody tries
-	 * to enable polling via device tree with broken-cd property.
-	 */
-	if (!(host->mmc->caps & MMC_CAP_NONREMOVABLE) &&
-	    IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc))) {
-		host->mmc->caps |= MMC_CAP_NEEDS_POLL;
-		host->quirks &= ~SDHCI_QUIRK_BROKEN_CARD_DETECTION;
-	}
-
 	pm_runtime_put_autosuspend(&pdev->dev);
 
 	return 0;
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 8670f16..619a64a 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2467,11 +2467,6 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 
 	spin_lock(&host->lock);
 
-	if (host->runtime_suspended && !sdhci_sdio_irq_enabled(host)) {
-		spin_unlock(&host->lock);
-		return IRQ_NONE;
-	}
-
 	intmask = sdhci_readl(host, SDHCI_INT_STATUS);
 	if (!intmask || intmask == 0xffffffff) {
 		result = IRQ_NONE;
@@ -2709,7 +2704,7 @@ static void sdhci_runtime_pm_bus_off(struct sdhci_host *host)
 	pm_runtime_put_noidle(host->mmc->parent);
 }
 
-int sdhci_runtime_suspend_host(struct sdhci_host *host)
+int sdhci_runtime_suspend_host(struct sdhci_host *host, u32 wakeup_irqs)
 {
 	unsigned long flags;
 
@@ -2717,7 +2712,10 @@ int sdhci_runtime_suspend_host(struct sdhci_host *host)
 	mmc_retune_needed(host->mmc);
 
 	spin_lock_irqsave(&host->lock, flags);
-	host->ier &= SDHCI_INT_CARD_INT;
+	if (wakeup_irqs)
+		host->ier = wakeup_irqs;
+	else
+		host->ier &= SDHCI_INT_CARD_INT;
 	sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
 	sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
 	spin_unlock_irqrestore(&host->lock, flags);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 3bd2803..b932e15 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -668,7 +668,7 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing);
 extern int sdhci_suspend_host(struct sdhci_host *host);
 extern int sdhci_resume_host(struct sdhci_host *host);
 extern void sdhci_enable_irq_wakeups(struct sdhci_host *host);
-extern int sdhci_runtime_suspend_host(struct sdhci_host *host);
+extern int sdhci_runtime_suspend_host(struct sdhci_host *host, u32 wakeup_irqs);
 extern int sdhci_runtime_resume_host(struct sdhci_host *host);
 #endif
 
-- 
2.5.0

[PATCH] DRAFT: shdci: allows custom wakeup irqs for runtime PM

[Ludovic Desroches]

Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
---
 drivers/mmc/host/sdhci-of-at91.c | 36 ++++++++++--------------------------
 drivers/mmc/host/sdhci.c         | 12 +++++-------
 drivers/mmc/host/sdhci.h         |  2 +-
 3 files changed, 16 insertions(+), 34 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
index 2703aa9..d70bb7a 100644
--- a/drivers/mmc/host/sdhci-of-at91.c
+++ b/drivers/mmc/host/sdhci-of-at91.c
@@ -62,12 +62,14 @@ static int sdhci_at91_runtime_suspend(struct device *dev)
 	struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
 	int ret;
 
-	ret = sdhci_runtime_suspend_host(host);
+	ret = sdhci_runtime_suspend_host(host, SDHCI_INT_CARD_INT | SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE);
 
 	clk_disable_unprepare(priv->gck);
-	clk_disable_unprepare(priv->hclock);
+	//clk_disable_unprepare(priv->hclock);
 	clk_disable_unprepare(priv->mainck);
 
+	printk("sdhci_at91_runtime_suspend\n");
+
 	return ret;
 }
 
@@ -78,17 +80,18 @@ static int sdhci_at91_runtime_resume(struct device *dev)
 	struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
 	int ret;
 
+	printk("sdhci_at91_runtime_resume\n");
 	ret = clk_prepare_enable(priv->mainck);
 	if (ret) {
 		dev_err(dev, "can't enable mainck\n");
 		return ret;
 	}
 
-	ret = clk_prepare_enable(priv->hclock);
-	if (ret) {
-		dev_err(dev, "can't enable hclock\n");
-		return ret;
-	}
+	//ret = clk_prepare_enable(priv->hclock);
+	//if (ret) {
+	//	dev_err(dev, "can't enable hclock\n");
+	//	return ret;
+	//}
 
 	ret = clk_prepare_enable(priv->gck);
 	if (ret) {
@@ -205,25 +208,6 @@ static int sdhci_at91_probe(struct platform_device *pdev)
 	if (ret)
 		goto pm_runtime_disable;
 
-	/*
-	 * When calling sdhci_runtime_suspend_host(), the sdhci layer makes
-	 * the assumption that all the clocks of the controller are disabled.
-	 * It means we can't get irq from it when it is runtime suspended.
-	 * For that reason, it is not planned to wake-up on a card detect irq
-	 * from the controller.
-	 * If we want to use runtime PM and to be able to wake-up on card
-	 * insertion, we have to use a GPIO for the card detection or we can
-	 * use polling. Be aware that using polling will resume/suspend the
-	 * controller between each attempt.
-	 * Disable SDHCI_QUIRK_BROKEN_CARD_DETECTION to be sure nobody tries
-	 * to enable polling via device tree with broken-cd property.
-	 */
-	if (!(host->mmc->caps & MMC_CAP_NONREMOVABLE) &&
-	    IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc))) {
-		host->mmc->caps |= MMC_CAP_NEEDS_POLL;
-		host->quirks &= ~SDHCI_QUIRK_BROKEN_CARD_DETECTION;
-	}
-
 	pm_runtime_put_autosuspend(&pdev->dev);
 
 	return 0;
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 8670f16..619a64a 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2467,11 +2467,6 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 
 	spin_lock(&host->lock);
 
-	if (host->runtime_suspended && !sdhci_sdio_irq_enabled(host)) {
-		spin_unlock(&host->lock);
-		return IRQ_NONE;
-	}
-
 	intmask = sdhci_readl(host, SDHCI_INT_STATUS);
 	if (!intmask || intmask == 0xffffffff) {
 		result = IRQ_NONE;
@@ -2709,7 +2704,7 @@ static void sdhci_runtime_pm_bus_off(struct sdhci_host *host)
 	pm_runtime_put_noidle(host->mmc->parent);
 }
 
-int sdhci_runtime_suspend_host(struct sdhci_host *host)
+int sdhci_runtime_suspend_host(struct sdhci_host *host, u32 wakeup_irqs)
 {
 	unsigned long flags;
 
@@ -2717,7 +2712,10 @@ int sdhci_runtime_suspend_host(struct sdhci_host *host)
 	mmc_retune_needed(host->mmc);
 
 	spin_lock_irqsave(&host->lock, flags);
-	host->ier &= SDHCI_INT_CARD_INT;
+	if (wakeup_irqs)
+		host->ier = wakeup_irqs;
+	else
+		host->ier &= SDHCI_INT_CARD_INT;
 	sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
 	sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
 	spin_unlock_irqrestore(&host->lock, flags);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 3bd2803..b932e15 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -668,7 +668,7 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing);
 extern int sdhci_suspend_host(struct sdhci_host *host);
 extern int sdhci_resume_host(struct sdhci_host *host);
 extern void sdhci_enable_irq_wakeups(struct sdhci_host *host);
-extern int sdhci_runtime_suspend_host(struct sdhci_host *host);
+extern int sdhci_runtime_suspend_host(struct sdhci_host *host, u32 wakeup_irqs);
 extern int sdhci_runtime_resume_host(struct sdhci_host *host);
 #endif
 
-- 
2.5.0

[PATCH] mmc: sdhci-of-at91: allow the use of controller card detect as wake up

[Ludovic Desroches]

At the moment, there are only two ways to wake up from runtime suspend state:
- using a gpio for the card detection which is not a good solution since
  you still need the card detect pin of the controller.
- using polling which is also not a good solution since it will
  resume/suspend the controller between each attempt.

The sdhci layer makes the assumption that all the clocks are disabled
when suspending the controller. For that reason, irqs are rejected if
runtime_suspended is true.

This patch duplicates the sdhci_runtime_suspend_host() but doesn't set
runtime_suspended and doesn't disable the interface clock if there is a
removable device and no gpio for card detection.

Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
---
 drivers/mmc/host/sdhci-of-at91.c | 71 +++++++++++++++++++++++++---------------
 1 file changed, 45 insertions(+), 26 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
index 2703aa9..6683f2d 100644
--- a/drivers/mmc/host/sdhci-of-at91.c
+++ b/drivers/mmc/host/sdhci-of-at91.c
@@ -35,6 +35,7 @@ struct sdhci_at91_priv {
 	struct clk *hclock;
 	struct clk *gck;
 	struct clk *mainck;
+	bool disable_interface_clk;
 };
 
 static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
@@ -55,20 +56,47 @@ static const struct of_device_id sdhci_at91_dt_match[] = {
 };
 
 #ifdef CONFIG_PM
+/*
+ * This function is the very same as sdhci_runtime_suspend_host but if the
+ * device is removable and we only have the controller card detect signal then
+ * we won't disable all the clocks. The interface clock will be needed to get
+ * card event interrupt.
+ * When calling sdhci_runtime_suspend_host(), the sdhci layer makes the
+ * assumption that all the clocks of the controller are disabled. If
+ * runtime_suspended is set, irqs will be rejected. That's why it won't be set
+ * if we want to wake up on card event.
+ */
 static int sdhci_at91_runtime_suspend(struct device *dev)
 {
 	struct sdhci_host *host = dev_get_drvdata(dev);
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
-	int ret;
+	unsigned long flags;
+
+	mmc_retune_timer_stop(host->mmc);
+	mmc_retune_needed(host->mmc);
+
+	spin_lock_irqsave(&host->lock, flags);
+	host->ier &= SDHCI_INT_CARD_INT;
+	if (!priv->disable_interface_clk)
+		host->ier |= SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE;
+	sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
+	sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
+	spin_unlock_irqrestore(&host->lock, flags);
 
-	ret = sdhci_runtime_suspend_host(host);
+	synchronize_hardirq(host->irq);
+
+	if (priv->disable_interface_clk) {
+		spin_lock_irqsave(&host->lock, flags);
+		host->runtime_suspended = true;
+		spin_unlock_irqrestore(&host->lock, flags);
+		clk_disable_unprepare(priv->hclock);
+	};
 
 	clk_disable_unprepare(priv->gck);
-	clk_disable_unprepare(priv->hclock);
 	clk_disable_unprepare(priv->mainck);
 
-	return ret;
+	return 0;
 }
 
 static int sdhci_at91_runtime_resume(struct device *dev)
@@ -84,18 +112,20 @@ static int sdhci_at91_runtime_resume(struct device *dev)
 		return ret;
 	}
 
-	ret = clk_prepare_enable(priv->hclock);
-	if (ret) {
-		dev_err(dev, "can't enable hclock\n");
-		return ret;
-	}
-
 	ret = clk_prepare_enable(priv->gck);
 	if (ret) {
 		dev_err(dev, "can't enable gck\n");
 		return ret;
 	}
 
+	if (priv->disable_interface_clk) {
+		ret = clk_prepare_enable(priv->hclock);
+		if (ret) {
+			dev_err(dev, "can't enable hclock\n");
+			return ret;
+		}
+	}
+
 	return sdhci_runtime_resume_host(host);
 }
 #endif /* CONFIG_PM */
@@ -206,23 +236,12 @@ static int sdhci_at91_probe(struct platform_device *pdev)
 		goto pm_runtime_disable;
 
 	/*
-	 * When calling sdhci_runtime_suspend_host(), the sdhci layer makes
-	 * the assumption that all the clocks of the controller are disabled.
-	 * It means we can't get irq from it when it is runtime suspended.
-	 * For that reason, it is not planned to wake-up on a card detect irq
-	 * from the controller.
-	 * If we want to use runtime PM and to be able to wake-up on card
-	 * insertion, we have to use a GPIO for the card detection or we can
-	 * use polling. Be aware that using polling will resume/suspend the
-	 * controller between each attempt.
-	 * Disable SDHCI_QUIRK_BROKEN_CARD_DETECTION to be sure nobody tries
-	 * to enable polling via device tree with broken-cd property.
+	 * If the device is non removable or if there is a gpio for the card
+	 * detection, all the clocks can be disabled in runtime suspend state.
 	 */
-	if (!(host->mmc->caps & MMC_CAP_NONREMOVABLE) &&
-	    IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc))) {
-		host->mmc->caps |= MMC_CAP_NEEDS_POLL;
-		host->quirks &= ~SDHCI_QUIRK_BROKEN_CARD_DETECTION;
-	}
+	priv->disable_interface_clk = (host->mmc->caps & MMC_CAP_NONREMOVABLE)
+				       || !IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc));
+
 
 	pm_runtime_put_autosuspend(&pdev->dev);
 
-- 
2.5.0

[PATCH] mmc: sdhci-of-at91: allow the use of controller card detect as wake up

[Ludovic Desroches]

At the moment, there are only two ways to wake up from runtime suspend state:
- using a gpio for the card detection which is not a good solution since
  you still need the card detect pin of the controller.
- using polling which is also not a good solution since it will
  resume/suspend the controller between each attempt.

The sdhci layer makes the assumption that all the clocks are disabled
when suspending the controller. For that reason, irqs are rejected if
runtime_suspended is true.

This patch duplicates the sdhci_runtime_suspend_host() but doesn't set
runtime_suspended and doesn't disable the interface clock if there is a
removable device and no gpio for card detection.

Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
---
 drivers/mmc/host/sdhci-of-at91.c | 71 +++++++++++++++++++++++++---------------
 1 file changed, 45 insertions(+), 26 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
index 2703aa9..6683f2d 100644
--- a/drivers/mmc/host/sdhci-of-at91.c
+++ b/drivers/mmc/host/sdhci-of-at91.c
@@ -35,6 +35,7 @@ struct sdhci_at91_priv {
 	struct clk *hclock;
 	struct clk *gck;
 	struct clk *mainck;
+	bool disable_interface_clk;
 };
 
 static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
@@ -55,20 +56,47 @@ static const struct of_device_id sdhci_at91_dt_match[] = {
 };
 
 #ifdef CONFIG_PM
+/*
+ * This function is the very same as sdhci_runtime_suspend_host but if the
+ * device is removable and we only have the controller card detect signal then
+ * we won't disable all the clocks. The interface clock will be needed to get
+ * card event interrupt.
+ * When calling sdhci_runtime_suspend_host(), the sdhci layer makes the
+ * assumption that all the clocks of the controller are disabled. If
+ * runtime_suspended is set, irqs will be rejected. That's why it won't be set
+ * if we want to wake up on card event.
+ */
 static int sdhci_at91_runtime_suspend(struct device *dev)
 {
 	struct sdhci_host *host = dev_get_drvdata(dev);
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host);
-	int ret;
+	unsigned long flags;
+
+	mmc_retune_timer_stop(host->mmc);
+	mmc_retune_needed(host->mmc);
+
+	spin_lock_irqsave(&host->lock, flags);
+	host->ier &= SDHCI_INT_CARD_INT;
+	if (!priv->disable_interface_clk)
+		host->ier |= SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE;
+	sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
+	sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
+	spin_unlock_irqrestore(&host->lock, flags);
 
-	ret = sdhci_runtime_suspend_host(host);
+	synchronize_hardirq(host->irq);
+
+	if (priv->disable_interface_clk) {
+		spin_lock_irqsave(&host->lock, flags);
+		host->runtime_suspended = true;
+		spin_unlock_irqrestore(&host->lock, flags);
+		clk_disable_unprepare(priv->hclock);
+	};
 
 	clk_disable_unprepare(priv->gck);
-	clk_disable_unprepare(priv->hclock);
 	clk_disable_unprepare(priv->mainck);
 
-	return ret;
+	return 0;
 }
 
 static int sdhci_at91_runtime_resume(struct device *dev)
@@ -84,18 +112,20 @@ static int sdhci_at91_runtime_resume(struct device *dev)
 		return ret;
 	}
 
-	ret = clk_prepare_enable(priv->hclock);
-	if (ret) {
-		dev_err(dev, "can't enable hclock\n");
-		return ret;
-	}
-
 	ret = clk_prepare_enable(priv->gck);
 	if (ret) {
 		dev_err(dev, "can't enable gck\n");
 		return ret;
 	}
 
+	if (priv->disable_interface_clk) {
+		ret = clk_prepare_enable(priv->hclock);
+		if (ret) {
+			dev_err(dev, "can't enable hclock\n");
+			return ret;
+		}
+	}
+
 	return sdhci_runtime_resume_host(host);
 }
 #endif /* CONFIG_PM */
@@ -206,23 +236,12 @@ static int sdhci_at91_probe(struct platform_device *pdev)
 		goto pm_runtime_disable;
 
 	/*
-	 * When calling sdhci_runtime_suspend_host(), the sdhci layer makes
-	 * the assumption that all the clocks of the controller are disabled.
-	 * It means we can't get irq from it when it is runtime suspended.
-	 * For that reason, it is not planned to wake-up on a card detect irq
-	 * from the controller.
-	 * If we want to use runtime PM and to be able to wake-up on card
-	 * insertion, we have to use a GPIO for the card detection or we can
-	 * use polling. Be aware that using polling will resume/suspend the
-	 * controller between each attempt.
-	 * Disable SDHCI_QUIRK_BROKEN_CARD_DETECTION to be sure nobody tries
-	 * to enable polling via device tree with broken-cd property.
+	 * If the device is non removable or if there is a gpio for the card
+	 * detection, all the clocks can be disabled in runtime suspend state.
 	 */
-	if (!(host->mmc->caps & MMC_CAP_NONREMOVABLE) &&
-	    IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc))) {
-		host->mmc->caps |= MMC_CAP_NEEDS_POLL;
-		host->quirks &= ~SDHCI_QUIRK_BROKEN_CARD_DETECTION;
-	}
+	priv->disable_interface_clk = (host->mmc->caps & MMC_CAP_NONREMOVABLE)
+				       || !IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc));
+
 
 	pm_runtime_put_autosuspend(&pdev->dev);
 
-- 
2.5.0

Re: [PATCH] DRAFT: shdci: allows custom wakeup irqs for runtime PM

[kbuild test robot]

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

Hi Ludovic,

[auto build test ERROR on ulf.hansson-mmc/next]
[also build test ERROR on v4.5 next-20160324]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Ludovic-Desroches/DRAFT-shdci-allows-custom-wakeup-irqs-for-runtime-PM/20160326-000948
base:   https://git.linaro.org/people/ulf.hansson/mmc next
config: x86_64-allyesdebian (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/mmc/host/sdhci-pci-core.c: In function 'sdhci_pci_runtime_suspend':
>> drivers/mmc/host/sdhci-pci-core.c:1497:9: error: too few arguments to function 'sdhci_runtime_suspend_host'
      ret = sdhci_runtime_suspend_host(slot->host);
            ^
   In file included from drivers/mmc/host/sdhci-pci-core.c:31:0:
   drivers/mmc/host/sdhci.h:671:12: note: declared here
    extern int sdhci_runtime_suspend_host(struct sdhci_host *host, u32 wakeup_irqs);
               ^

vim +/sdhci_runtime_suspend_host +1497 drivers/mmc/host/sdhci-pci-core.c

66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03  1491  
66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03  1492  	for (i = 0; i < chip->num_slots; i++) {
66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03  1493  		slot = chip->slots[i];
66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03  1494  		if (!slot)
66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03  1495  			continue;
66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03  1496  
66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03 @1497  		ret = sdhci_runtime_suspend_host(slot->host);
66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03  1498  
b678b91f drivers/mmc/host/sdhci-pci.c Axel Lin      2011-12-03  1499  		if (ret)
b678b91f drivers/mmc/host/sdhci-pci.c Axel Lin      2011-12-03  1500  			goto err_pci_runtime_suspend;

:::::: The code at line 1497 was first introduced by commit
:::::: 66fd8ad5100b5003046aa744a4f12fa31bb831f9 mmc: sdhci-pci: add runtime pm support

:::::: TO: Adrian Hunter <adrian.hunter@intel.com>
:::::: CC: Chris Ball <cjb@laptop.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 36639 bytes --]

Re: [PATCH] DRAFT: shdci: allows custom wakeup irqs for runtime PM

[kbuild test robot]

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

Hi Ludovic,

[auto build test ERROR on ulf.hansson-mmc/next]
[also build test ERROR on v4.5 next-20160324]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Ludovic-Desroches/DRAFT-shdci-allows-custom-wakeup-irqs-for-runtime-PM/20160326-000948
base:   https://git.linaro.org/people/ulf.hansson/mmc next
config: x86_64-allyesdebian (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/mmc/host/sdhci-pci-core.c: In function 'sdhci_pci_runtime_suspend':
>> drivers/mmc/host/sdhci-pci-core.c:1497:9: error: too few arguments to function 'sdhci_runtime_suspend_host'
      ret = sdhci_runtime_suspend_host(slot->host);
            ^
   In file included from drivers/mmc/host/sdhci-pci-core.c:31:0:
   drivers/mmc/host/sdhci.h:671:12: note: declared here
    extern int sdhci_runtime_suspend_host(struct sdhci_host *host, u32 wakeup_irqs);
               ^

vim +/sdhci_runtime_suspend_host +1497 drivers/mmc/host/sdhci-pci-core.c

66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03  1491  
66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03  1492  	for (i = 0; i < chip->num_slots; i++) {
66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03  1493  		slot = chip->slots[i];
66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03  1494  		if (!slot)
66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03  1495  			continue;
66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03  1496  
66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03 @1497  		ret = sdhci_runtime_suspend_host(slot->host);
66fd8ad5 drivers/mmc/host/sdhci-pci.c Adrian Hunter 2011-10-03  1498  
b678b91f drivers/mmc/host/sdhci-pci.c Axel Lin      2011-12-03  1499  		if (ret)
b678b91f drivers/mmc/host/sdhci-pci.c Axel Lin      2011-12-03  1500  			goto err_pci_runtime_suspend;

:::::: The code at line 1497 was first introduced by commit
:::::: 66fd8ad5100b5003046aa744a4f12fa31bb831f9 mmc: sdhci-pci: add runtime pm support

:::::: TO: Adrian Hunter <adrian.hunter@intel.com>
:::::: CC: Chris Ball <cjb@laptop.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 36639 bytes --]

Re: [PATCH] DRAFT: shdci: allows custom wakeup irqs for runtime PM

[kbuild test robot]

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

Hi Ludovic,

[auto build test ERROR on ulf.hansson-mmc/next]
[also build test ERROR on v4.5 next-20160324]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Ludovic-Desroches/DRAFT-shdci-allows-custom-wakeup-irqs-for-runtime-PM/20160326-000948
base:   https://git.linaro.org/people/ulf.hansson/mmc next
config: x86_64-rhel (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   drivers/mmc/host/sdhci-acpi.c: In function 'sdhci_acpi_runtime_suspend':
>> drivers/mmc/host/sdhci-acpi.c:466:9: error: too few arguments to function 'sdhci_runtime_suspend_host'
     return sdhci_runtime_suspend_host(c->host);
            ^
   In file included from drivers/mmc/host/sdhci-acpi.c:44:0:
   drivers/mmc/host/sdhci.h:671:12: note: declared here
    extern int sdhci_runtime_suspend_host(struct sdhci_host *host, u32 wakeup_irqs);
               ^
>> drivers/mmc/host/sdhci-acpi.c:467:1: warning: control reaches end of non-void function [-Wreturn-type]
    }
    ^

vim +/sdhci_runtime_suspend_host +466 drivers/mmc/host/sdhci-acpi.c

162d6f98 Rafael J. Wysocki 2014-12-05  460  #ifdef CONFIG_PM
c4e05037 Adrian Hunter     2012-11-23  461  
c4e05037 Adrian Hunter     2012-11-23  462  static int sdhci_acpi_runtime_suspend(struct device *dev)
c4e05037 Adrian Hunter     2012-11-23  463  {
c4e05037 Adrian Hunter     2012-11-23  464  	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
c4e05037 Adrian Hunter     2012-11-23  465  
c4e05037 Adrian Hunter     2012-11-23 @466  	return sdhci_runtime_suspend_host(c->host);
c4e05037 Adrian Hunter     2012-11-23 @467  }
c4e05037 Adrian Hunter     2012-11-23  468  
c4e05037 Adrian Hunter     2012-11-23  469  static int sdhci_acpi_runtime_resume(struct device *dev)
c4e05037 Adrian Hunter     2012-11-23  470  {

:::::: The code at line 466 was first introduced by commit
:::::: c4e050376c69bb9d67895842665264df2a2004d9 mmc: sdhci-acpi: add SDHCI ACPI driver

:::::: TO: Adrian Hunter <adrian.hunter@intel.com>
:::::: CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 36091 bytes --]

Re: [PATCH] DRAFT: shdci: allows custom wakeup irqs for runtime PM

[kbuild test robot]

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

Hi Ludovic,

[auto build test ERROR on ulf.hansson-mmc/next]
[also build test ERROR on v4.5 next-20160324]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Ludovic-Desroches/DRAFT-shdci-allows-custom-wakeup-irqs-for-runtime-PM/20160326-000948
base:   https://git.linaro.org/people/ulf.hansson/mmc next
config: x86_64-rhel (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   drivers/mmc/host/sdhci-acpi.c: In function 'sdhci_acpi_runtime_suspend':
>> drivers/mmc/host/sdhci-acpi.c:466:9: error: too few arguments to function 'sdhci_runtime_suspend_host'
     return sdhci_runtime_suspend_host(c->host);
            ^
   In file included from drivers/mmc/host/sdhci-acpi.c:44:0:
   drivers/mmc/host/sdhci.h:671:12: note: declared here
    extern int sdhci_runtime_suspend_host(struct sdhci_host *host, u32 wakeup_irqs);
               ^
>> drivers/mmc/host/sdhci-acpi.c:467:1: warning: control reaches end of non-void function [-Wreturn-type]
    }
    ^

vim +/sdhci_runtime_suspend_host +466 drivers/mmc/host/sdhci-acpi.c

162d6f98 Rafael J. Wysocki 2014-12-05  460  #ifdef CONFIG_PM
c4e05037 Adrian Hunter     2012-11-23  461  
c4e05037 Adrian Hunter     2012-11-23  462  static int sdhci_acpi_runtime_suspend(struct device *dev)
c4e05037 Adrian Hunter     2012-11-23  463  {
c4e05037 Adrian Hunter     2012-11-23  464  	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
c4e05037 Adrian Hunter     2012-11-23  465  
c4e05037 Adrian Hunter     2012-11-23 @466  	return sdhci_runtime_suspend_host(c->host);
c4e05037 Adrian Hunter     2012-11-23 @467  }
c4e05037 Adrian Hunter     2012-11-23  468  
c4e05037 Adrian Hunter     2012-11-23  469  static int sdhci_acpi_runtime_resume(struct device *dev)
c4e05037 Adrian Hunter     2012-11-23  470  {

:::::: The code at line 466 was first introduced by commit
:::::: c4e050376c69bb9d67895842665264df2a2004d9 mmc: sdhci-acpi: add SDHCI ACPI driver

:::::: TO: Adrian Hunter <adrian.hunter@intel.com>
:::::: CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 36091 bytes --]

Re: [PATCH] mmc: sdhci-of-at91: allow the use of controller card detect as wake up

[kbuild test robot]

Hi Ludovic,

[auto build test WARNING on ulf.hansson-mmc/next]
[also build test WARNING on next-20160324]
[cannot apply to v4.5]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Ludovic-Desroches/mmc-sdhci-of-at91-allow-the-use-of-controller-card-detect-as-wake-up/20160326-001143
base:   https://git.linaro.org/people/ulf.hansson/mmc next


coccinelle warnings: (new ones prefixed by >>)

>> drivers/mmc/host/sdhci-of-at91.c:94:2-3: Unneeded semicolon

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[PATCH] mmc: sdhci-of-at91: fix semicolon.cocci warnings

[kbuild test robot]

drivers/mmc/host/sdhci-of-at91.c:94:2-3: Unneeded semicolon


 Remove unneeded semicolon.

Generated by: scripts/coccinelle/misc/semicolon.cocci

CC: Ludovic Desroches <ludovic.desroches@atmel.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---

 sdhci-of-at91.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/mmc/host/sdhci-of-at91.c
+++ b/drivers/mmc/host/sdhci-of-at91.c
@@ -91,7 +91,7 @@ static int sdhci_at91_runtime_suspend(st
 		host->runtime_suspended = true;
 		spin_unlock_irqrestore(&host->lock, flags);
 		clk_disable_unprepare(priv->hclock);
-	};
+	}
 
 	clk_disable_unprepare(priv->gck);
 	clk_disable_unprepare(priv->mainck);

Re: [PATCH] mmc: sdhci-of-at91: allow the use of controller card detect as wake up

[kbuild test robot]

Hi Ludovic,

[auto build test WARNING on ulf.hansson-mmc/next]
[also build test WARNING on next-20160324]
[cannot apply to v4.5]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Ludovic-Desroches/mmc-sdhci-of-at91-allow-the-use-of-controller-card-detect-as-wake-up/20160326-001143
base:   https://git.linaro.org/people/ulf.hansson/mmc next


coccinelle warnings: (new ones prefixed by >>)

>> drivers/mmc/host/sdhci-of-at91.c:94:2-3: Unneeded semicolon

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[PATCH] mmc: sdhci-of-at91: fix semicolon.cocci warnings

[kbuild test robot]

drivers/mmc/host/sdhci-of-at91.c:94:2-3: Unneeded semicolon


 Remove unneeded semicolon.

Generated by: scripts/coccinelle/misc/semicolon.cocci

CC: Ludovic Desroches <ludovic.desroches@atmel.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---

 sdhci-of-at91.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/mmc/host/sdhci-of-at91.c
+++ b/drivers/mmc/host/sdhci-of-at91.c
@@ -91,7 +91,7 @@ static int sdhci_at91_runtime_suspend(st
 		host->runtime_suspended = true;
 		spin_unlock_irqrestore(&host->lock, flags);
 		clk_disable_unprepare(priv->hclock);
-	};
+	}
 
 	clk_disable_unprepare(priv->gck);
 	clk_disable_unprepare(priv->mainck);

Re: [PATCH] DRAFT: shdci: allows custom wakeup irqs for runtime PM

[kbuild test robot]

Hi Ludovic,

[auto build test WARNING on ulf.hansson-mmc/next]
[also build test WARNING on v4.5 next-20160324]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Ludovic-Desroches/DRAFT-shdci-allows-custom-wakeup-irqs-for-runtime-PM/20160326-000948
base:   https://git.linaro.org/people/ulf.hansson/mmc next
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

   include/linux/compiler.h:228:8: sparse: attribute 'no_sanitize_address': unknown attribute
>> drivers/mmc/host/sdhci-pci-core.c:1497:49: sparse: not enough arguments for function sdhci_runtime_suspend_host
   drivers/mmc/host/sdhci-pci-core.c: In function 'sdhci_pci_runtime_suspend':
   drivers/mmc/host/sdhci-pci-core.c:1497:9: error: too few arguments to function 'sdhci_runtime_suspend_host'
      ret = sdhci_runtime_suspend_host(slot->host);
            ^
   In file included from drivers/mmc/host/sdhci-pci-core.c:31:0:
   drivers/mmc/host/sdhci.h:671:12: note: declared here
    extern int sdhci_runtime_suspend_host(struct sdhci_host *host, u32 wakeup_irqs);
               ^
--
   include/linux/compiler.h:228:8: sparse: attribute 'no_sanitize_address': unknown attribute
>> drivers/mmc/host/sdhci-acpi.c:466:42: sparse: not enough arguments for function sdhci_runtime_suspend_host
   drivers/mmc/host/sdhci-acpi.c: In function 'sdhci_acpi_runtime_suspend':
   drivers/mmc/host/sdhci-acpi.c:466:9: error: too few arguments to function 'sdhci_runtime_suspend_host'
     return sdhci_runtime_suspend_host(c->host);
            ^
   In file included from drivers/mmc/host/sdhci-acpi.c:44:0:
   drivers/mmc/host/sdhci.h:671:12: note: declared here
    extern int sdhci_runtime_suspend_host(struct sdhci_host *host, u32 wakeup_irqs);
               ^
   drivers/mmc/host/sdhci-acpi.c:467:1: warning: control reaches end of non-void function [-Wreturn-type]
    }
    ^
--
   include/linux/compiler.h:228:8: sparse: attribute 'no_sanitize_address': unknown attribute
>> drivers/mmc/host/sdhci-pxav3.c:539:41: sparse: not enough arguments for function sdhci_runtime_suspend_host
   drivers/mmc/host/sdhci-pxav3.c: In function 'sdhci_pxav3_runtime_suspend':
   drivers/mmc/host/sdhci-pxav3.c:539:8: error: too few arguments to function 'sdhci_runtime_suspend_host'
     ret = sdhci_runtime_suspend_host(host);
           ^
   In file included from drivers/mmc/host/sdhci-pxav3.c:39:0:
   drivers/mmc/host/sdhci.h:671:12: note: declared here
    extern int sdhci_runtime_suspend_host(struct sdhci_host *host, u32 wakeup_irqs);
               ^

vim +1497 drivers/mmc/host/sdhci-pci-core.c

66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1481  static int sdhci_pci_runtime_suspend(struct device *dev)
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1482  {
923a231c drivers/mmc/host/sdhci-pci-core.c Geliang Tang  2015-12-27  1483  	struct pci_dev *pdev = to_pci_dev(dev);
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1484  	struct sdhci_pci_chip *chip;
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1485  	struct sdhci_pci_slot *slot;
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1486  	int i, ret;
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1487  
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1488  	chip = pci_get_drvdata(pdev);
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1489  	if (!chip)
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1490  		return 0;
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1491  
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1492  	for (i = 0; i < chip->num_slots; i++) {
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1493  		slot = chip->slots[i];
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1494  		if (!slot)
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1495  			continue;
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1496  
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03 @1497  		ret = sdhci_runtime_suspend_host(slot->host);
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1498  
b678b91f drivers/mmc/host/sdhci-pci.c      Axel Lin      2011-12-03  1499  		if (ret)
b678b91f drivers/mmc/host/sdhci-pci.c      Axel Lin      2011-12-03  1500  			goto err_pci_runtime_suspend;
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1501  	}
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1502  
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1503  	if (chip->fixes && chip->fixes->suspend) {
29495aa0 drivers/mmc/host/sdhci-pci.c      Manuel Lauss  2011-11-03  1504  		ret = chip->fixes->suspend(chip);
b678b91f drivers/mmc/host/sdhci-pci.c      Axel Lin      2011-12-03  1505  		if (ret)

:::::: The code at line 1497 was first introduced by commit
:::::: 66fd8ad5100b5003046aa744a4f12fa31bb831f9 mmc: sdhci-pci: add runtime pm support

:::::: TO: Adrian Hunter <adrian.hunter@intel.com>
:::::: CC: Chris Ball <cjb@laptop.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH] DRAFT: shdci: allows custom wakeup irqs for runtime PM

[kbuild test robot]

Hi Ludovic,

[auto build test WARNING on ulf.hansson-mmc/next]
[also build test WARNING on v4.5 next-20160324]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Ludovic-Desroches/DRAFT-shdci-allows-custom-wakeup-irqs-for-runtime-PM/20160326-000948
base:   https://git.linaro.org/people/ulf.hansson/mmc next
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

   include/linux/compiler.h:228:8: sparse: attribute 'no_sanitize_address': unknown attribute
>> drivers/mmc/host/sdhci-pci-core.c:1497:49: sparse: not enough arguments for function sdhci_runtime_suspend_host
   drivers/mmc/host/sdhci-pci-core.c: In function 'sdhci_pci_runtime_suspend':
   drivers/mmc/host/sdhci-pci-core.c:1497:9: error: too few arguments to function 'sdhci_runtime_suspend_host'
      ret = sdhci_runtime_suspend_host(slot->host);
            ^
   In file included from drivers/mmc/host/sdhci-pci-core.c:31:0:
   drivers/mmc/host/sdhci.h:671:12: note: declared here
    extern int sdhci_runtime_suspend_host(struct sdhci_host *host, u32 wakeup_irqs);
               ^
--
   include/linux/compiler.h:228:8: sparse: attribute 'no_sanitize_address': unknown attribute
>> drivers/mmc/host/sdhci-acpi.c:466:42: sparse: not enough arguments for function sdhci_runtime_suspend_host
   drivers/mmc/host/sdhci-acpi.c: In function 'sdhci_acpi_runtime_suspend':
   drivers/mmc/host/sdhci-acpi.c:466:9: error: too few arguments to function 'sdhci_runtime_suspend_host'
     return sdhci_runtime_suspend_host(c->host);
            ^
   In file included from drivers/mmc/host/sdhci-acpi.c:44:0:
   drivers/mmc/host/sdhci.h:671:12: note: declared here
    extern int sdhci_runtime_suspend_host(struct sdhci_host *host, u32 wakeup_irqs);
               ^
   drivers/mmc/host/sdhci-acpi.c:467:1: warning: control reaches end of non-void function [-Wreturn-type]
    }
    ^
--
   include/linux/compiler.h:228:8: sparse: attribute 'no_sanitize_address': unknown attribute
>> drivers/mmc/host/sdhci-pxav3.c:539:41: sparse: not enough arguments for function sdhci_runtime_suspend_host
   drivers/mmc/host/sdhci-pxav3.c: In function 'sdhci_pxav3_runtime_suspend':
   drivers/mmc/host/sdhci-pxav3.c:539:8: error: too few arguments to function 'sdhci_runtime_suspend_host'
     ret = sdhci_runtime_suspend_host(host);
           ^
   In file included from drivers/mmc/host/sdhci-pxav3.c:39:0:
   drivers/mmc/host/sdhci.h:671:12: note: declared here
    extern int sdhci_runtime_suspend_host(struct sdhci_host *host, u32 wakeup_irqs);
               ^

vim +1497 drivers/mmc/host/sdhci-pci-core.c

66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1481  static int sdhci_pci_runtime_suspend(struct device *dev)
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1482  {
923a231c drivers/mmc/host/sdhci-pci-core.c Geliang Tang  2015-12-27  1483  	struct pci_dev *pdev = to_pci_dev(dev);
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1484  	struct sdhci_pci_chip *chip;
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1485  	struct sdhci_pci_slot *slot;
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1486  	int i, ret;
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1487  
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1488  	chip = pci_get_drvdata(pdev);
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1489  	if (!chip)
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1490  		return 0;
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1491  
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1492  	for (i = 0; i < chip->num_slots; i++) {
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1493  		slot = chip->slots[i];
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1494  		if (!slot)
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1495  			continue;
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1496  
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03 @1497  		ret = sdhci_runtime_suspend_host(slot->host);
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1498  
b678b91f drivers/mmc/host/sdhci-pci.c      Axel Lin      2011-12-03  1499  		if (ret)
b678b91f drivers/mmc/host/sdhci-pci.c      Axel Lin      2011-12-03  1500  			goto err_pci_runtime_suspend;
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1501  	}
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1502  
66fd8ad5 drivers/mmc/host/sdhci-pci.c      Adrian Hunter 2011-10-03  1503  	if (chip->fixes && chip->fixes->suspend) {
29495aa0 drivers/mmc/host/sdhci-pci.c      Manuel Lauss  2011-11-03  1504  		ret = chip->fixes->suspend(chip);
b678b91f drivers/mmc/host/sdhci-pci.c      Axel Lin      2011-12-03  1505  		if (ret)

:::::: The code at line 1497 was first introduced by commit
:::::: 66fd8ad5100b5003046aa744a4f12fa31bb831f9 mmc: sdhci-pci: add runtime pm support

:::::: TO: Adrian Hunter <adrian.hunter@intel.com>
:::::: CC: Chris Ball <cjb@laptop.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH] DRAFT: shdci: allows custom wakeup irqs for runtime PM

[kbuild test robot]

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

Hi Ludovic,

[auto build test ERROR on ulf.hansson-mmc/next]
[also build test ERROR on v4.5 next-20160324]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Ludovic-Desroches/DRAFT-shdci-allows-custom-wakeup-irqs-for-runtime-PM/20160326-000948
base:   https://git.linaro.org/people/ulf.hansson/mmc next
config: i386-allmodconfig (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/mmc/host/sdhci-pxav3.c: In function 'sdhci_pxav3_runtime_suspend':
>> drivers/mmc/host/sdhci-pxav3.c:539:8: error: too few arguments to function 'sdhci_runtime_suspend_host'
     ret = sdhci_runtime_suspend_host(host);
           ^
   In file included from drivers/mmc/host/sdhci-pxav3.c:39:0:
   drivers/mmc/host/sdhci.h:671:12: note: declared here
    extern int sdhci_runtime_suspend_host(struct sdhci_host *host, u32 wakeup_irqs);
               ^

vim +/sdhci_runtime_suspend_host +539 drivers/mmc/host/sdhci-pxav3.c

bb691ae4 Kevin Liu     2013-02-01  533  {
bb691ae4 Kevin Liu     2013-02-01  534  	struct sdhci_host *host = dev_get_drvdata(dev);
bb691ae4 Kevin Liu     2013-02-01  535  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
f599da40 Jisheng Zhang 2016-02-16  536  	struct sdhci_pxa *pxa = sdhci_pltfm_priv(pltfm_host);
3bb10f60 Jisheng Zhang 2015-01-23  537  	int ret;
bb691ae4 Kevin Liu     2013-02-01  538  
3bb10f60 Jisheng Zhang 2015-01-23 @539  	ret = sdhci_runtime_suspend_host(host);
3bb10f60 Jisheng Zhang 2015-01-23  540  	if (ret)
3bb10f60 Jisheng Zhang 2015-01-23  541  		return ret;
bb691ae4 Kevin Liu     2013-02-01  542  

:::::: The code at line 539 was first introduced by commit
:::::: 3bb10f60933e84abfe2be69f60b3486f9b96348b mmc: sdhci-pxav3: fix race between runtime pm and irq

:::::: TO: Jisheng Zhang <jszhang@marvell.com>
:::::: CC: Ulf Hansson <ulf.hansson@linaro.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 53462 bytes --]

Re: [PATCH] DRAFT: shdci: allows custom wakeup irqs for runtime PM

[kbuild test robot]

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

Hi Ludovic,

[auto build test ERROR on ulf.hansson-mmc/next]
[also build test ERROR on v4.5 next-20160324]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Ludovic-Desroches/DRAFT-shdci-allows-custom-wakeup-irqs-for-runtime-PM/20160326-000948
base:   https://git.linaro.org/people/ulf.hansson/mmc next
config: i386-allmodconfig (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/mmc/host/sdhci-pxav3.c: In function 'sdhci_pxav3_runtime_suspend':
>> drivers/mmc/host/sdhci-pxav3.c:539:8: error: too few arguments to function 'sdhci_runtime_suspend_host'
     ret = sdhci_runtime_suspend_host(host);
           ^
   In file included from drivers/mmc/host/sdhci-pxav3.c:39:0:
   drivers/mmc/host/sdhci.h:671:12: note: declared here
    extern int sdhci_runtime_suspend_host(struct sdhci_host *host, u32 wakeup_irqs);
               ^

vim +/sdhci_runtime_suspend_host +539 drivers/mmc/host/sdhci-pxav3.c

bb691ae4 Kevin Liu     2013-02-01  533  {
bb691ae4 Kevin Liu     2013-02-01  534  	struct sdhci_host *host = dev_get_drvdata(dev);
bb691ae4 Kevin Liu     2013-02-01  535  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
f599da40 Jisheng Zhang 2016-02-16  536  	struct sdhci_pxa *pxa = sdhci_pltfm_priv(pltfm_host);
3bb10f60 Jisheng Zhang 2015-01-23  537  	int ret;
bb691ae4 Kevin Liu     2013-02-01  538  
3bb10f60 Jisheng Zhang 2015-01-23 @539  	ret = sdhci_runtime_suspend_host(host);
3bb10f60 Jisheng Zhang 2015-01-23  540  	if (ret)
3bb10f60 Jisheng Zhang 2015-01-23  541  		return ret;
bb691ae4 Kevin Liu     2013-02-01  542  

:::::: The code at line 539 was first introduced by commit
:::::: 3bb10f60933e84abfe2be69f60b3486f9b96348b mmc: sdhci-pxav3: fix race between runtime pm and irq

:::::: TO: Jisheng Zhang <jszhang@marvell.com>
:::::: CC: Ulf Hansson <ulf.hansson@linaro.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 53462 bytes --]

Re: [PATCH] sdhci: wakeup from runtime PM

[Adrian Hunter]

On 25/03/16 18:05, Ludovic Desroches wrote:
> Hi,
> 
> When not using the SDHCI controller, it is logical to save power by suspending
> it. The issue is that SDHCI assumes that the controller is completely disabled.
> It means the only way to wake up on a card event is to have a gpio for the card
> detection (so two pins for the same signal). A possible workaround is to use
> polling but the controller will be resumed/suspended between each attempts.
> 
> We have already discussed a long time about this and it seems we don't agree.
> In my opinion, even if I can't disable all clocks, I should use runtime PM 
> to save some power.
> 
> I propose two patches, one which is a draft to try to solve it at sdhci level
> and one at sdhci-of-at91 level.
> 
> Concerning the first one, I don't understand why we need to reject irqs if
> runtime_suspended is true.

The interrupt handler might be called because the interrupt is shared i.e.
the interrupt is for a different device.  In that case the host controller
might be off and the registers inaccessible.  In that case we cannot even
look at the interrupt register to determine if we were expecting the interrupt.

>                            Only SDHCI_INT_CARD_INT irq is enabled so why we
> should have other IRQs than this one?

In the case of SDIO Card interrupt, it is delivered via the host controller,
so we have to assume the registers are accessible if we are
runtime-suspended with the SDIO IRQ enabled.

> 
> Since you were not in favour of allowing to wakeup on SDHCI_INT_CARD_INSERT or
> SDHCI_INT_CARD_REMOVE, I assume you won't take it so I
> solved my issue only by modifying my driver.

I don't mind allowing card detect interrupts while runtime suspended, but we
need a flag so that:
- runtime suspend leaves the insert/remove interrupts enabled
- irq handler knows it can access registers
- irq thread handler knows to runtime resume before doing anything else

But it seems like you need to persuade Ulf first.

Re: [PATCH] sdhci: wakeup from runtime PM

[Ulf Hansson]

On 5 April 2016 at 14:40, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 25/03/16 18:05, Ludovic Desroches wrote:
>> Hi,
>>
>> When not using the SDHCI controller, it is logical to save power by suspending
>> it. The issue is that SDHCI assumes that the controller is completely disabled.
>> It means the only way to wake up on a card event is to have a gpio for the card
>> detection (so two pins for the same signal). A possible workaround is to use
>> polling but the controller will be resumed/suspended between each attempts.
>>
>> We have already discussed a long time about this and it seems we don't agree.
>> In my opinion, even if I can't disable all clocks, I should use runtime PM
>> to save some power.
>>
>> I propose two patches, one which is a draft to try to solve it at sdhci level
>> and one at sdhci-of-at91 level.
>>
>> Concerning the first one, I don't understand why we need to reject irqs if
>> runtime_suspended is true.
>
> The interrupt handler might be called because the interrupt is shared i.e.
> the interrupt is for a different device.  In that case the host controller
> might be off and the registers inaccessible.  In that case we cannot even
> look at the interrupt register to determine if we were expecting the interrupt.
>
>>                            Only SDHCI_INT_CARD_INT irq is enabled so why we
>> should have other IRQs than this one?
>
> In the case of SDIO Card interrupt, it is delivered via the host controller,
> so we have to assume the registers are accessible if we are
> runtime-suspended with the SDIO IRQ enabled.
>
>>
>> Since you were not in favour of allowing to wakeup on SDHCI_INT_CARD_INSERT or
>> SDHCI_INT_CARD_REMOVE, I assume you won't take it so I
>> solved my issue only by modifying my driver.
>
> I don't mind allowing card detect interrupts while runtime suspended, but we
> need a flag so that:
> - runtime suspend leaves the insert/remove interrupts enabled
> - irq handler knows it can access registers

To me, this seem like the wrong way of how to configure wake-ups for
these kind of devices.

I don't think the regular IRQs shall be enabled and the driver
shouldn't assume the registers are accessible without first runtime
resuming the device.

> - irq thread handler knows to runtime resume before doing anything else
>
> But it seems like you need to persuade Ulf first.

For a more thorough explanation to why I don't like, please have a
look at my comment for another related thread on the mmc list.
http://www.spinics.net/lists/linux-mmc/msg36132.html

Kind regards
Uffe

Re: [PATCH] sdhci: wakeup from runtime PM

[Ludovic Desroches]

On Thu, Apr 07, 2016 at 11:11:08AM +0200, Ulf Hansson wrote:
> On 5 April 2016 at 14:40, Adrian Hunter <adrian.hunter@intel.com> wrote:
> > On 25/03/16 18:05, Ludovic Desroches wrote:
> >> Hi,
> >>
> >> When not using the SDHCI controller, it is logical to save power by suspending
> >> it. The issue is that SDHCI assumes that the controller is completely disabled.
> >> It means the only way to wake up on a card event is to have a gpio for the card
> >> detection (so two pins for the same signal). A possible workaround is to use
> >> polling but the controller will be resumed/suspended between each attempts.
> >>
> >> We have already discussed a long time about this and it seems we don't agree.
> >> In my opinion, even if I can't disable all clocks, I should use runtime PM
> >> to save some power.
> >>
> >> I propose two patches, one which is a draft to try to solve it at sdhci level
> >> and one at sdhci-of-at91 level.
> >>
> >> Concerning the first one, I don't understand why we need to reject irqs if
> >> runtime_suspended is true.
> >
> > The interrupt handler might be called because the interrupt is shared i.e.
> > the interrupt is for a different device.  In that case the host controller
> > might be off and the registers inaccessible.  In that case we cannot even
> > look at the interrupt register to determine if we were expecting the interrupt.

Ok, I have missed we can get a shared irq.

> >
> >>                            Only SDHCI_INT_CARD_INT irq is enabled so why we
> >> should have other IRQs than this one?
> >
> > In the case of SDIO Card interrupt, it is delivered via the host controller,
> > so we have to assume the registers are accessible if we are
> > runtime-suspended with the SDIO IRQ enabled.
> >
> >>
> >> Since you were not in favour of allowing to wakeup on SDHCI_INT_CARD_INSERT or
> >> SDHCI_INT_CARD_REMOVE, I assume you won't take it so I
> >> solved my issue only by modifying my driver.
> >
> > I don't mind allowing card detect interrupts while runtime suspended, but we
> > need a flag so that:
> > - runtime suspend leaves the insert/remove interrupts enabled
> > - irq handler knows it can access registers
> 
> To me, this seem like the wrong way of how to configure wake-ups for
> these kind of devices.

We are definitely not in agreement on this point. It means I have to
rely on polling for card detection. I did it but I had in mind it would
be a temporary workaround while waiting to find a better one.

> 
> I don't think the regular IRQs shall be enabled and the driver
> shouldn't assume the registers are accessible without first runtime
> resuming the device.
> 
> > - irq thread handler knows to runtime resume before doing anything else
> >
> > But it seems like you need to persuade Ulf first.
> 

It will be a difficult task :)

> For a more thorough explanation to why I don't like, please have a
> look at my comment for another related thread on the mmc list.
> http://www.spinics.net/lists/linux-mmc/msg36132.html
> 

To facilitate discussion, I copy a part of your explanation here:

> My interpretation of how runtime PM should be deployed, it's s that a
> device shall not process data in the runtime suspend state. To do
> that, it first needs to be runtime resumed. I think this conforms to
> what the runtime PM documentation also recommends in section 2, Device
> Runtime PM Callbacks.

So simply invoking the resume callback when the card detect irq is
received should be in accordance with the PM documentation.

> Moreover I think it's an SoC specific detail whether a driver *can*
> access a device's registers in the runtime suspend state (thus it
> shouldn't do it). Especially, devices may reside in a PM domain, which
> could enter a retention state, because all its devices are runtime
> suspended. Typically for an ARM SoC is that devices looses their
> register's context in such retention state.

Maybe it's a SoC specific detail but that doesn't mean that this detail
should not be taken into account. The PM documentation mentions 'the PM
core regards the device as suspended, which need not mean that it has been  
put into a low power state'. It sounds like we can access to the
registers, there is no requirement to disable all the clocks.

I am not arguing for SD card detection wake-up itself but for saving power
when I don't use my SDHCI controller. I boot Linux without a SD card,
the controller is runtime suspended, it seems obvious that the controller
should be resumed when I insert a card. I really want to use runtime PM
because the controller should be most of the time suspended excepting if the
rootfs is on the SD card.

Am I the only one to find that the constraint is too big to use runtime PM?


Regards

Ludovic

Re: [PATCH] sdhci: wakeup from runtime PM

[Ulf Hansson]

- linux-kernel list

On 7 April 2016 at 17:12, Ludovic Desroches <ludovic.desroches@atmel.com> wrote:
> On Thu, Apr 07, 2016 at 11:11:08AM +0200, Ulf Hansson wrote:
>> On 5 April 2016 at 14:40, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> > On 25/03/16 18:05, Ludovic Desroches wrote:
>> >> Hi,
>> >>
>> >> When not using the SDHCI controller, it is logical to save power by suspending
>> >> it. The issue is that SDHCI assumes that the controller is completely disabled.
>> >> It means the only way to wake up on a card event is to have a gpio for the card
>> >> detection (so two pins for the same signal). A possible workaround is to use
>> >> polling but the controller will be resumed/suspended between each attempts.
>> >>

>From power consumption point if view we already discussed this
particular case in an earlier thread, but let me elaborate what I
think one more time:

Now, this is relevant for removable cards lacking GPIO card detect, as
for other cases and non-removable cards I think we are in agreement
that their is no issue from power consumption point of view, right!?

1. The current solution:
- Use MMC_CAP_NEEDS_POLL which makes the mmc core to re-schedule a
work once every second to poll for a card (Why is it one second? Could
we perhaps have that configurable?).

In case of no card inserted, the polling consists of runtime resuming
the SDHCI controller and then reading a couple of registers to find
out if there is a new card. I assume this will be a fast operation. In
the below calculation I have neglected its impact which of course is a
simplification.

This solution allows the driver in runtime suspend to *gate all three
clocks* used by the SDHCI controller. In-between polling attempts it
will thus save power.

Currently the mmc core *always* respects runtime PM autosuspend when
putting the controller device back to runtime suspend. I suggest we
change this in cases when the polling operation doesn't detect any
changes.

For the sdhci-of-at91 driver case, the autosuspend timeout is set to
50 ms (which is a common value among mmc host drivers).

In the current code this means for a period over ~10 s, *all three
clocks* will be ungated for 50 ms x 10 times = ~500 ms.

If we adopt to what I propose above, this time will become significant
lesser as the autosuspend timer will not be respected.

2. What you propose (I think):
* Don't use MMC_CAP_NEEDS_POLL, but instead rely on the SDHCI
controller to pick up card detect IRQs in runtime suspended state.

In case of no card inserted, the controller would then stay runtime
suspended but with IRQs enabled to deal with card detect IRQs.
According to what you told me earlier, this means that the controller
requires one of the three clocks to be ungated in the runtime suspend
state. So, only two clocks can be managed by runtime PM.

Assuming that no cards get inserted over a period of ~10 s, *one clock
will be ungated* for ~10 s.


Can you show that option 2) is better in saving power than option 1) ?

[...]

>> >
>> > I don't mind allowing card detect interrupts while runtime suspended, but we
>> > need a flag so that:
>> > - runtime suspend leaves the insert/remove interrupts enabled
>> > - irq handler knows it can access registers
>>
>> To me, this seem like the wrong way of how to configure wake-ups for
>> these kind of devices.
>
> We are definitely not in agreement on this point. It means I have to
> rely on polling for card detection. I did it but I had in mind it would
> be a temporary workaround while waiting to find a better one.
>
>>
>> I don't think the regular IRQs shall be enabled and the driver
>> shouldn't assume the registers are accessible without first runtime
>> resuming the device.
>>
>> > - irq thread handler knows to runtime resume before doing anything else
>> >
>> > But it seems like you need to persuade Ulf first.
>>
>
> It will be a difficult task :)

Yes, but I have changed my mind earlier... :-)

[...]

>
> I am not arguing for SD card detection wake-up itself but for saving power
> when I don't use my SDHCI controller. I boot Linux without a SD card,
> the controller is runtime suspended, it seems obvious that the controller
> should be resumed when I insert a card. I really want to use runtime PM
> because the controller should be most of the time suspended excepting if the
> rootfs is on the SD card.

I am not sure I understand what you states here, but I assume my upper
elaboration on your proposal is correct!? If not, please try to be a
bit more specific.

Moreover, let's not involve "rootfs" to this discussion, but stay to
discussing removable cards as those are the interesting ones, right!?

[...]

Kind regards
Uffe

Re: [PATCH] sdhci: wakeup from runtime PM

[Alan Stern]

On Fri, 8 Apr 2016, Ulf Hansson wrote:

> From power consumption point if view we already discussed this
> particular case in an earlier thread, but let me elaborate what I
> think one more time:
> 
> Now, this is relevant for removable cards lacking GPIO card detect, as
> for other cases and non-removable cards I think we are in agreement
> that their is no issue from power consumption point of view, right!?
> 
> 1. The current solution:
> - Use MMC_CAP_NEEDS_POLL which makes the mmc core to re-schedule a
> work once every second to poll for a card (Why is it one second? Could
> we perhaps have that configurable?).

The block layer handles media polling for removable block devices, and
it is configurable.  For example, /sys/block/sr0/events_poll_msecs can
be set to control how often the system polls for a disc in my DVD
drive.

Does MMC do its own polling separate from the block layer?  If so, why?

Alan Stern

Re: [PATCH] sdhci: wakeup from runtime PM

[Ulf Hansson]

On 8 April 2016 at 17:19, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Fri, 8 Apr 2016, Ulf Hansson wrote:
>
>> From power consumption point if view we already discussed this
>> particular case in an earlier thread, but let me elaborate what I
>> think one more time:
>>
>> Now, this is relevant for removable cards lacking GPIO card detect, as
>> for other cases and non-removable cards I think we are in agreement
>> that their is no issue from power consumption point of view, right!?
>>
>> 1. The current solution:
>> - Use MMC_CAP_NEEDS_POLL which makes the mmc core to re-schedule a
>> work once every second to poll for a card (Why is it one second? Could
>> we perhaps have that configurable?).
>
> The block layer handles media polling for removable block devices, and
> it is configurable.  For example, /sys/block/sr0/events_poll_msecs can
> be set to control how often the system polls for a disc in my DVD
> drive.
>
> Does MMC do its own polling separate from the block layer?  If so, why?

Good question. Probably because of legacy or perhaps to deal with corner cases.

When I get some spare bandwidth I will check to see if it makes sense
to convert MMC polling to the block layer polling.

Kind regards
Uffe

Re: [PATCH] sdhci: wakeup from runtime PM

[Ludovic Desroches]

On Fri, Apr 08, 2016 at 10:35:00AM +0200, Ulf Hansson wrote:
> - linux-kernel list
> 
> On 7 April 2016 at 17:12, Ludovic Desroches <ludovic.desroches@atmel.com> wrote:
> > On Thu, Apr 07, 2016 at 11:11:08AM +0200, Ulf Hansson wrote:
> >> On 5 April 2016 at 14:40, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >> > On 25/03/16 18:05, Ludovic Desroches wrote:
> >> >> Hi,
> >> >>
> >> >> When not using the SDHCI controller, it is logical to save power by suspending
> >> >> it. The issue is that SDHCI assumes that the controller is completely disabled.
> >> >> It means the only way to wake up on a card event is to have a gpio for the card
> >> >> detection (so two pins for the same signal). A possible workaround is to use
> >> >> polling but the controller will be resumed/suspended between each attempts.
> >> >>
> 
> From power consumption point if view we already discussed this
> particular case in an earlier thread, but let me elaborate what I
> think one more time:
> 
> Now, this is relevant for removable cards lacking GPIO card detect, as
> for other cases and non-removable cards I think we are in agreement
> that their is no issue from power consumption point of view, right!?
> 
> 1. The current solution:
> - Use MMC_CAP_NEEDS_POLL which makes the mmc core to re-schedule a
> work once every second to poll for a card (Why is it one second? Could
> we perhaps have that configurable?).
> 
> In case of no card inserted, the polling consists of runtime resuming
> the SDHCI controller and then reading a couple of registers to find
> out if there is a new card. I assume this will be a fast operation. In
> the below calculation I have neglected its impact which of course is a
> simplification.
> 
> This solution allows the driver in runtime suspend to *gate all three
> clocks* used by the SDHCI controller. In-between polling attempts it
> will thus save power.
> 
> Currently the mmc core *always* respects runtime PM autosuspend when
> putting the controller device back to runtime suspend. I suggest we
> change this in cases when the polling operation doesn't detect any
> changes.
> 
> For the sdhci-of-at91 driver case, the autosuspend timeout is set to
> 50 ms (which is a common value among mmc host drivers).
> 
> In the current code this means for a period over ~10 s, *all three
> clocks* will be ungated for 50 ms x 10 times = ~500 ms.
> 
> If we adopt to what I propose above, this time will become significant
> lesser as the autosuspend timer will not be respected.
> 
> 2. What you propose (I think):
> * Don't use MMC_CAP_NEEDS_POLL, but instead rely on the SDHCI
> controller to pick up card detect IRQs in runtime suspended state.
> 
> In case of no card inserted, the controller would then stay runtime
> suspended but with IRQs enabled to deal with card detect IRQs.
> According to what you told me earlier, this means that the controller
> requires one of the three clocks to be ungated in the runtime suspend
> state. So, only two clocks can be managed by runtime PM.
> 
> Assuming that no cards get inserted over a period of ~10 s, *one clock
> will be ungated* for ~10 s.
> 
> 
> Can you show that option 2) is better in saving power than option 1) ?

Unfortunately not. I have discussed with hardware guys. They confirmed me
that the power consumption to keep the interface enabled is very low but it
will be difficult to measure the consumption difference between the two
options because it would be minimal.

If in practice the result was almost the same, it is not useful to
continue this debate. I'll keep the polling workaround and I'll update system
PM callbacks.

[...]

Regards

Ludovic