[PATCH 0/2] mmc: sdhci-pci: add runtime pm support

[PATCH 0/2] mmc: sdhci-pci: add runtime pm support

[Adrian Hunter]

Hi

Here are 2 patches to add runtime pm to sdhci-pci.

Adrian Hunter (2):
      mmc: core: move ->request() call from atomic context
      mmc: sdhci-pci: add runtime pm support

 drivers/mmc/core/core.c      |   28 ++++-
 drivers/mmc/host/sdhci-pci.c |  182 ++++++++++++++++++++++++++++++-
 drivers/mmc/host/sdhci.c     |  255 ++++++++++++++++++++++++++++++++++-------
 drivers/mmc/host/sdhci.h     |    5 +
 include/linux/mmc/sdhci.h    |    8 ++
 5 files changed, 426 insertions(+), 52 deletions(-)

Regards
Adrian Hunter

[PATCH 1/2] mmc: core: move ->request() call from atomic context

[Adrian Hunter]

mmc_request_done() is sometimes called from interrupt
or other atomic context.  Mostly all mmc_request_done()
does is complete(), however it contains code to retry
on error, which uses ->request().  As the error path
is certainly not performance critical, this may be
moved to the waiting function mmc_wait_for_req_done().

This allows ->request() to use runtime PM get_sync()
and guarantee it is never in an atomic context.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/core.c |   28 +++++++++++++++++++++-------
 1 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 9698d8a..ec76949 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -141,12 +141,12 @@ void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
 	}
 
 	if (err && cmd->retries) {
-		pr_debug("%s: req failed (CMD%u): %d, retrying...\n",
-			mmc_hostname(host), cmd->opcode, err);
-
-		cmd->retries--;
-		cmd->error = 0;
-		host->ops->request(host, mrq);
+		/*
+		 * Request starter must handle retries - see
+		 * mmc_wait_for_req_done().
+		 */
+		if (mrq->done)
+			mrq->done(mrq);
 	} else {
 		mmc_should_fail_request(host, mrq);
 
@@ -253,7 +253,21 @@ static void __mmc_start_req(struct mmc_host *host, struct mmc_request *mrq)
 static void mmc_wait_for_req_done(struct mmc_host *host,
 				  struct mmc_request *mrq)
 {
-	wait_for_completion(&mrq->completion);
+	struct mmc_command *cmd;
+
+	while (1) {
+		wait_for_completion(&mrq->completion);
+
+		cmd = mrq->cmd;
+		if (!cmd->error || !cmd->retries)
+			break;
+
+		pr_debug("%s: req failed (CMD%u): %d, retrying...\n",
+			 mmc_hostname(host), cmd->opcode, cmd->error);
+		cmd->retries--;
+		cmd->error = 0;
+		host->ops->request(host, mrq);
+	}
 }
 
 /**
-- 
1.7.6

[PATCH 2/2] mmc: sdhci-pci: add runtime pm support

[Adrian Hunter]

Ths patch allows runtime PM for sdhci-pci, runtime suspending
after inactivity of 50ms and ensuring runtime resume before
SDHC registers are accessed.  During runtime suspend, interrupts
are masked.  The host controller state is restored at runtime
resume.

For Medfield, the host controller's card detect mechanism
is supplanted by an always-on GPIO which provides for card
detect wake-up.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci-pci.c |  182 ++++++++++++++++++++++++++++++-
 drivers/mmc/host/sdhci.c     |  255 ++++++++++++++++++++++++++++++++++-------
 drivers/mmc/host/sdhci.h     |    5 +
 include/linux/mmc/sdhci.h    |    8 ++
 4 files changed, 405 insertions(+), 45 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index f8a17f9..79dd3a5 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -23,6 +23,7 @@
 #include <linux/io.h>
 #include <linux/gpio.h>
 #include <linux/sfi.h>
+#include <linux/pm_runtime.h>
 
 #include "sdhci.h"
 
@@ -62,6 +63,8 @@ struct sdhci_pci_slot {
 
 	int			pci_bar;
 	int			rst_n_gpio;
+	int			cd_gpio;
+	int			cd_irq;
 };
 
 struct sdhci_pci_chip {
@@ -189,6 +192,70 @@ static int mfd_emmc_gpio_parse(struct sfi_table_header *table)
 	return 0;
 }
 
+#ifdef CONFIG_PM_RUNTIME
+
+static irqreturn_t mfd_sd_cd(int irq, void *dev_id)
+{
+	struct sdhci_pci_slot *slot = dev_id;
+	struct sdhci_host *host = slot->host;
+
+	mmc_detect_change(host->mmc, msecs_to_jiffies(200));
+	return IRQ_HANDLED;
+}
+
+#define MFLD_SD_CD_PIN 69
+
+static int mfd_sd_probe_slot(struct sdhci_pci_slot *slot)
+{
+	int err, irq, gpio = MFLD_SD_CD_PIN;
+
+	slot->cd_gpio = -EINVAL;
+	slot->cd_irq = -EINVAL;
+
+	err = gpio_request(gpio, "sd_cd");
+	if (err < 0)
+		goto out;
+
+	err = gpio_direction_input(gpio);
+	if (err < 0)
+		goto out_free;
+
+	irq = gpio_to_irq(gpio);
+	if (irq < 0)
+		goto out_free;
+
+	err = request_irq(irq, mfd_sd_cd, IRQF_TRIGGER_RISING |
+			  IRQF_TRIGGER_FALLING, "sd_cd", slot);
+	if (err)
+		goto out_free;
+
+	slot->cd_gpio = gpio;
+	slot->cd_irq = irq;
+	slot->host->quirks2 |= SDHCI_QUIRK2_OWN_CARD_DETECTION;
+
+	return 0;
+
+out_free:
+	gpio_free(gpio);
+out:
+	dev_warn(&slot->chip->pdev->dev, "failed to setup card detect wake up\n");
+	return 0;
+}
+
+static void mfd_sd_remove_slot(struct sdhci_pci_slot *slot, int dead)
+{
+	if (slot->cd_irq >= 0)
+		free_irq(slot->cd_irq, slot);
+	gpio_free(slot->cd_gpio);
+}
+
+#else
+
+#define mfd_sd_probe_slot	NULL
+#define mfd_sd_remove_slot	NULL
+
+#endif
+
 static int mfd_emmc_probe_slot(struct sdhci_pci_slot *slot)
 {
 	const char *name = NULL;
@@ -213,7 +280,7 @@ static int mfd_emmc_probe_slot(struct sdhci_pci_slot *slot)
 		slot->host->mmc->caps |= MMC_CAP_HW_RESET;
 	}
 
-	slot->host->mmc->caps |= MMC_CAP_8_BIT_DATA;
+	slot->host->mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_NONREMOVABLE;
 
 	slot->host->mmc->caps2 = MMC_CAP2_BOOTPART_NOACC;
 
@@ -237,6 +304,8 @@ static const struct sdhci_pci_fixes sdhci_intel_mrst_hc1_hc2 = {
 
 static const struct sdhci_pci_fixes sdhci_intel_mfd_sd = {
 	.quirks		= SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
+	.probe_slot	= mfd_sd_probe_slot,
+	.remove_slot	= mfd_sd_remove_slot,
 };
 
 static const struct sdhci_pci_fixes sdhci_intel_mfd_sdio = {
@@ -1017,6 +1086,95 @@ static int sdhci_pci_resume(struct pci_dev *pdev)
 
 #endif /* CONFIG_PM */
 
+#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;
+	pm_message_t state = { .event = PM_EVENT_SUSPEND };
+	int i, ret;
+
+	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_host(slot->host);
+
+		if (ret) {
+			for (i--; i >= 0; i--)
+				sdhci_runtime_resume_host(chip->slots[i]->host);
+			return ret;
+		}
+	}
+
+	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_host(chip->slots[i]->host);
+			return ret;
+		}
+	}
+
+	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;
+
+	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_host(slot->host);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int sdhci_pci_runtime_idle(struct device *dev)
+{
+	return 0;
+}
+
+#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,
+};
+
 /*****************************************************************************\
  *                                                                           *
  * Device probing/removal                                                    *
@@ -1132,6 +1290,21 @@ static void sdhci_pci_remove_slot(struct sdhci_pci_slot *slot)
 	sdhci_free_host(slot->host);
 }
 
+static void __devinit sdhci_pci_runtime_pm_allow(struct device *dev)
+{
+	pm_runtime_put_noidle(dev);
+	pm_runtime_allow(dev);
+	pm_runtime_set_autosuspend_delay(dev, 50);
+	pm_runtime_use_autosuspend(dev);
+	pm_suspend_ignore_children(dev, 1);
+}
+
+static void __devexit sdhci_pci_runtime_pm_forbid(struct device *dev)
+{
+	pm_runtime_forbid(dev);
+	pm_runtime_get_noresume(dev);
+}
+
 static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
 				     const struct pci_device_id *ent)
 {
@@ -1207,6 +1380,8 @@ static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
 		chip->slots[i] = slot;
 	}
 
+	sdhci_pci_runtime_pm_allow(&pdev->dev);
+
 	return 0;
 
 free:
@@ -1223,6 +1398,8 @@ static void __devexit sdhci_pci_remove(struct pci_dev *pdev)
 	int i;
 	struct sdhci_pci_chip *chip;
 
+	sdhci_pci_runtime_pm_forbid(&pdev->dev);
+
 	chip = pci_get_drvdata(pdev);
 
 	if (chip) {
@@ -1243,6 +1420,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 d66a7a1..787b877 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>
 
@@ -41,6 +42,7 @@
 #define MAX_TUNING_LOOP 40
 
 static unsigned int debug_quirks = 0;
+static unsigned int debug_quirks2;
 
 static void sdhci_finish_data(struct sdhci_host *);
 
@@ -49,6 +51,20 @@ static void sdhci_finish_command(struct sdhci_host *);
 static int sdhci_execute_tuning(struct mmc_host *mmc);
 static void sdhci_tuning_timer(unsigned long data);
 
+#ifdef CONFIG_PM_RUNTIME
+static int sdhci_runtime_pm_get(struct sdhci_host *host);
+static int sdhci_runtime_pm_put(struct sdhci_host *host);
+#else
+static inline int sdhci_runtime_pm_get(struct sdhci_host *host)
+{
+	return 0;
+}
+static inline int sdhci_runtime_pm_put(struct sdhci_host *host)
+{
+	return 0;
+}
+#endif
+
 static void sdhci_dumpregs(struct sdhci_host *host)
 {
 	printk(KERN_DEBUG DRIVER_NAME ": =========== REGISTER DUMP (%s)===========\n",
@@ -132,6 +148,9 @@ static void sdhci_set_card_detection(struct sdhci_host *host, bool enable)
 	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
 		return;
 
+	if (host->quirks2 & SDHCI_QUIRK2_OWN_CARD_DETECTION)
+		return;
+
 	present = sdhci_readl(host, SDHCI_PRESENT_STATE) &
 			      SDHCI_CARD_PRESENT;
 	irqs = present ? SDHCI_INT_CARD_REMOVE : SDHCI_INT_CARD_INSERT;
@@ -251,11 +270,14 @@ static void sdhci_led_control(struct led_classdev *led,
 
 	spin_lock_irqsave(&host->lock, flags);
 
+	if (host->runtime_suspended)
+		goto out;
+
 	if (brightness == LED_OFF)
 		sdhci_deactivate_led(host);
 	else
 		sdhci_activate_led(host);
-
+out:
 	spin_unlock_irqrestore(&host->lock, flags);
 }
 #endif
@@ -1209,6 +1231,8 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 
 	host = mmc_priv(mmc);
 
+	sdhci_runtime_pm_get(host);
+
 	spin_lock_irqsave(&host->lock, flags);
 
 	WARN_ON(host->mrq != NULL);
@@ -1269,14 +1293,11 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 	spin_unlock_irqrestore(&host->lock, flags);
 }
 
-static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
 {
-	struct sdhci_host *host;
 	unsigned long flags;
 	u8 ctrl;
 
-	host = mmc_priv(mmc);
-
 	spin_lock_irqsave(&host->lock, flags);
 
 	if (host->flags & SDHCI_DEVICE_DEAD)
@@ -1426,7 +1447,16 @@ out:
 	spin_unlock_irqrestore(&host->lock, flags);
 }
 
-static int check_ro(struct sdhci_host *host)
+static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+
+	sdhci_runtime_pm_get(host);
+	sdhci_do_set_ios(host, ios);
+	sdhci_runtime_pm_put(host);
+}
+
+static int sdhci_check_ro(struct sdhci_host *host)
 {
 	unsigned long flags;
 	int is_readonly;
@@ -1450,19 +1480,16 @@ static int check_ro(struct sdhci_host *host)
 
 #define SAMPLE_COUNT	5
 
-static int sdhci_get_ro(struct mmc_host *mmc)
+static int sdhci_do_get_ro(struct sdhci_host *host)
 {
-	struct sdhci_host *host;
 	int i, ro_count;
 
-	host = mmc_priv(mmc);
-
 	if (!(host->quirks & SDHCI_QUIRK_UNSTABLE_RO_DETECT))
-		return check_ro(host);
+		return sdhci_check_ro(host);
 
 	ro_count = 0;
 	for (i = 0; i < SAMPLE_COUNT; i++) {
-		if (check_ro(host)) {
+		if (sdhci_check_ro(host)) {
 			if (++ro_count > SAMPLE_COUNT / 2)
 				return 1;
 		}
@@ -1479,38 +1506,56 @@ static void sdhci_hw_reset(struct mmc_host *mmc)
 		host->ops->hw_reset(host);
 }
 
-static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
+static int sdhci_get_ro(struct mmc_host *mmc)
 {
-	struct sdhci_host *host;
-	unsigned long flags;
-
-	host = mmc_priv(mmc);
+	struct sdhci_host *host = mmc_priv(mmc);
+	int ret;
 
-	spin_lock_irqsave(&host->lock, flags);
+	sdhci_runtime_pm_get(host);
+	ret = sdhci_do_get_ro(host);
+	sdhci_runtime_pm_put(host);
+	return ret;
+}
 
+static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int enable)
+{
 	if (host->flags & SDHCI_DEVICE_DEAD)
 		goto out;
 
 	if (enable)
+		host->flags |= SDHCI_SDIO_IRQ_ENABLED;
+	else
+		host->flags &= ~SDHCI_SDIO_IRQ_ENABLED;
+
+	/* SDIO IRQ will be enabled as appropriate in runtime resume */
+	if (host->runtime_suspended)
+		goto out;
+
+	if (enable)
 		sdhci_unmask_irqs(host, SDHCI_INT_CARD_INT);
 	else
 		sdhci_mask_irqs(host, SDHCI_INT_CARD_INT);
 out:
 	mmiowb();
+}
+
+static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	unsigned long flags;
 
+	spin_lock_irqsave(&host->lock, flags);
+	sdhci_enable_sdio_irq_nolock(host, enable);
 	spin_unlock_irqrestore(&host->lock, flags);
 }
 
-static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
-	struct mmc_ios *ios)
+static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
+						struct mmc_ios *ios)
 {
-	struct sdhci_host *host;
 	u8 pwr;
 	u16 clk, ctrl;
 	u32 present_state;
 
-	host = mmc_priv(mmc);
-
 	/*
 	 * Signal Voltage Switching is only applicable for Host Controllers
 	 * v3.00 and above.
@@ -1603,6 +1648,20 @@ static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
 		return 0;
 }
 
+static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
+	struct mmc_ios *ios)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	int err;
+
+	if (host->version < SDHCI_SPEC_300)
+		return 0;
+	sdhci_runtime_pm_get(host);
+	err = sdhci_do_start_signal_voltage_switch(host, ios);
+	sdhci_runtime_pm_put(host);
+	return err;
+}
+
 static int sdhci_execute_tuning(struct mmc_host *mmc)
 {
 	struct sdhci_host *host;
@@ -1614,6 +1673,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc)
 
 	host = mmc_priv(mmc);
 
+	sdhci_runtime_pm_get(host);
 	disable_irq(host->irq);
 	spin_lock(&host->lock);
 
@@ -1631,6 +1691,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc)
 	else {
 		spin_unlock(&host->lock);
 		enable_irq(host->irq);
+		sdhci_runtime_pm_put(host);
 		return 0;
 	}
 
@@ -1656,7 +1717,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc)
 	timeout = 150;
 	do {
 		struct mmc_command cmd = {0};
-		struct mmc_request mrq = {0};
+		struct mmc_request mrq = {NULL};
 
 		if (!tuning_loop_counter && !timeout)
 			break;
@@ -1774,18 +1835,16 @@ out:
 	sdhci_clear_set_irqs(host, SDHCI_INT_DATA_AVAIL, ier);
 	spin_unlock(&host->lock);
 	enable_irq(host->irq);
+	sdhci_runtime_pm_put(host);
 
 	return err;
 }
 
-static void sdhci_enable_preset_value(struct mmc_host *mmc, bool enable)
+static void sdhci_do_enable_preset_value(struct sdhci_host *host, bool enable)
 {
-	struct sdhci_host *host;
 	u16 ctrl;
 	unsigned long flags;
 
-	host = mmc_priv(mmc);
-
 	/* Host Controller v3.00 defines preset value registers */
 	if (host->version < SDHCI_SPEC_300)
 		return;
@@ -1801,14 +1860,25 @@ static void sdhci_enable_preset_value(struct mmc_host *mmc, bool enable)
 	if (enable && !(ctrl & SDHCI_CTRL_PRESET_VAL_ENABLE)) {
 		ctrl |= SDHCI_CTRL_PRESET_VAL_ENABLE;
 		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
+		host->flags |= SDHCI_PV_ENABLED;
 	} else if (!enable && (ctrl & SDHCI_CTRL_PRESET_VAL_ENABLE)) {
 		ctrl &= ~SDHCI_CTRL_PRESET_VAL_ENABLE;
 		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
+		host->flags &= ~SDHCI_PV_ENABLED;
 	}
 
 	spin_unlock_irqrestore(&host->lock, flags);
 }
 
+static void sdhci_enable_preset_value(struct mmc_host *mmc, bool enable)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+
+	sdhci_runtime_pm_get(host);
+	sdhci_do_enable_preset_value(host, enable);
+	sdhci_runtime_pm_put(host);
+}
+
 static const struct mmc_host_ops sdhci_ops = {
 	.request	= sdhci_request,
 	.set_ios	= sdhci_set_ios,
@@ -1835,19 +1905,19 @@ static void sdhci_tasklet_card(unsigned long param)
 
 	spin_lock_irqsave(&host->lock, flags);
 
-	if (!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)) {
-		if (host->mrq) {
-			printk(KERN_ERR "%s: Card removed during transfer!\n",
-				mmc_hostname(host->mmc));
-			printk(KERN_ERR "%s: Resetting controller.\n",
-				mmc_hostname(host->mmc));
+	/* Check host->mrq first in case we are runtime suspended */
+	if (host->mrq &&
+	    !(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)) {
+		printk(KERN_ERR "%s: Card removed during transfer!\n",
+			mmc_hostname(host->mmc));
+		printk(KERN_ERR "%s: Resetting controller.\n",
+			mmc_hostname(host->mmc));
 
-			sdhci_reset(host, SDHCI_RESET_CMD);
-			sdhci_reset(host, SDHCI_RESET_DATA);
+		sdhci_reset(host, SDHCI_RESET_CMD);
+		sdhci_reset(host, SDHCI_RESET_DATA);
 
-			host->mrq->cmd->error = -ENOMEDIUM;
-			tasklet_schedule(&host->finish_tasklet);
-		}
+		host->mrq->cmd->error = -ENOMEDIUM;
+		tasklet_schedule(&host->finish_tasklet);
 	}
 
 	spin_unlock_irqrestore(&host->lock, flags);
@@ -1863,14 +1933,16 @@ static void sdhci_tasklet_finish(unsigned long param)
 
 	host = (struct sdhci_host*)param;
 
+	spin_lock_irqsave(&host->lock, flags);
+
         /*
          * If this tasklet gets rescheduled while running, it will
          * be run again afterwards but without any active request.
          */
-	if (!host->mrq)
+	if (!host->mrq) {
+		spin_unlock_irqrestore(&host->lock, flags);
 		return;
-
-	spin_lock_irqsave(&host->lock, flags);
+	}
 
 	del_timer(&host->timer);
 
@@ -1914,6 +1986,7 @@ static void sdhci_tasklet_finish(unsigned long param)
 	spin_unlock_irqrestore(&host->lock, flags);
 
 	mmc_request_done(host->mmc, mrq);
+	sdhci_runtime_pm_put(host);
 }
 
 static void sdhci_timeout_timer(unsigned long data)
@@ -2145,12 +2218,19 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
 static irqreturn_t sdhci_irq(int irq, void *dev_id)
 {
 	irqreturn_t result;
-	struct sdhci_host* host = dev_id;
+	struct sdhci_host *host = dev_id;
 	u32 intmask;
 	int cardint = 0;
 
 	spin_lock(&host->lock);
 
+	if (host->runtime_suspended) {
+		spin_unlock(&host->lock);
+		printk(KERN_WARNING "%s: got irq while runtime suspended\n",
+		       mmc_hostname(host->mmc));
+		return IRQ_HANDLED;
+	}
+
 	intmask = sdhci_readl(host, SDHCI_INT_STATUS);
 
 	if (!intmask || intmask == 0xffffffff) {
@@ -2284,7 +2364,6 @@ int sdhci_resume_host(struct sdhci_host *host)
 			return ret;
 	}
 
-
 	if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA)) {
 		if (host->ops->enable_dma)
 			host->ops->enable_dma(host);
@@ -2323,6 +2402,90 @@ EXPORT_SYMBOL_GPL(sdhci_enable_irq_wakeups);
 
 #endif /* CONFIG_PM */
 
+#ifdef CONFIG_PM_RUNTIME
+
+static int sdhci_runtime_pm_get(struct sdhci_host *host)
+{
+	return pm_runtime_get_sync(host->mmc->parent);
+}
+
+static int sdhci_runtime_pm_put(struct sdhci_host *host)
+{
+	pm_runtime_mark_last_busy(host->mmc->parent);
+	return pm_runtime_put_autosuspend(host->mmc->parent);
+}
+
+int sdhci_runtime_suspend_host(struct sdhci_host *host)
+{
+	unsigned long flags;
+	int ret = 0;
+
+	/* Disable tuning since we are suspending */
+	if (host->version >= SDHCI_SPEC_300 &&
+	    host->tuning_mode == SDHCI_TUNING_MODE_1) {
+		del_timer_sync(&host->tuning_timer);
+		host->flags &= ~SDHCI_NEEDS_RETUNING;
+	}
+
+	spin_lock_irqsave(&host->lock, flags);
+	sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
+	spin_unlock_irqrestore(&host->lock, flags);
+
+	synchronize_irq(host->irq);
+
+	spin_lock_irqsave(&host->lock, flags);
+	host->runtime_suspended = true;
+	spin_unlock_irqrestore(&host->lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(sdhci_runtime_suspend_host);
+
+int sdhci_runtime_resume_host(struct sdhci_host *host)
+{
+	unsigned long flags;
+	int ret = 0, host_flags = host->flags;
+
+	if (host_flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA)) {
+		if (host->ops->enable_dma)
+			host->ops->enable_dma(host);
+	}
+
+	sdhci_init(host, 0);
+
+	/* Force clock and power re-program */
+	host->pwr = 0;
+	host->clock = 0;
+	sdhci_do_set_ios(host, &host->mmc->ios);
+
+	sdhci_do_start_signal_voltage_switch(host, &host->mmc->ios);
+	if (host_flags & SDHCI_PV_ENABLED)
+		sdhci_do_enable_preset_value(host, true);
+
+	/* Set the re-tuning expiration flag */
+	if ((host->version >= SDHCI_SPEC_300) && host->tuning_count &&
+	    (host->tuning_mode == SDHCI_TUNING_MODE_1))
+		host->flags |= SDHCI_NEEDS_RETUNING;
+
+	spin_lock_irqsave(&host->lock, flags);
+
+	host->runtime_suspended = false;
+
+	/* Enable SDIO IRQ */
+	if ((host->flags & SDHCI_SDIO_IRQ_ENABLED))
+		sdhci_enable_sdio_irq_nolock(host, true);
+
+	/* Enable Card Detection */
+	sdhci_enable_card_detection(host);
+
+	spin_unlock_irqrestore(&host->lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(sdhci_runtime_resume_host);
+
+#endif
+
 /*****************************************************************************\
  *                                                                           *
  * Device allocation/registration                                            *
@@ -2365,6 +2528,8 @@ int sdhci_add_host(struct sdhci_host *host)
 
 	if (debug_quirks)
 		host->quirks = debug_quirks;
+	if (debug_quirks2)
+		host->quirks2 = debug_quirks2;
 
 	sdhci_reset(host, SDHCI_RESET_ALL);
 
@@ -2887,9 +3052,11 @@ module_init(sdhci_drv_init);
 module_exit(sdhci_drv_exit);
 
 module_param(debug_quirks, uint, 0444);
+module_param(debug_quirks2, uint, 0444);
 
 MODULE_AUTHOR("Pierre Ossman <pierre@ossman.eu>");
 MODULE_DESCRIPTION("Secure Digital Host Controller Interface core driver");
 MODULE_LICENSE("GPL");
 
 MODULE_PARM_DESC(debug_quirks, "Force certain quirks.");
+MODULE_PARM_DESC(debug_quirks, "Force certain other quirks.");
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 7bd919c..0a5b654 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -379,4 +379,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_host(struct sdhci_host *host);
+extern int sdhci_runtime_resume_host(struct sdhci_host *host);
+#endif
+
 #endif /* __SDHCI_HW_H */
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index 5666f3a..e4b6935 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -88,6 +88,10 @@ struct sdhci_host {
 /* The read-only detection via SDHCI_PRESENT_STATE register is unstable */
 #define SDHCI_QUIRK_UNSTABLE_RO_DETECT			(1<<31)
 
+	unsigned int quirks2;	/* More deviations from spec. */
+
+#define SDHCI_QUIRK2_OWN_CARD_DETECTION			(1<<0)
+
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
 
@@ -115,6 +119,8 @@ struct sdhci_host {
 #define SDHCI_NEEDS_RETUNING	(1<<5)	/* Host needs retuning */
 #define SDHCI_AUTO_CMD12	(1<<6)	/* Auto CMD12 support */
 #define SDHCI_AUTO_CMD23	(1<<7)	/* Auto CMD23 support */
+#define SDHCI_PV_ENABLED	(1<<8)	/* Preset value enabled */
+#define SDHCI_SDIO_IRQ_ENABLED	(1<<9)	/* SDIO irq enabled */
 
 	unsigned int version;	/* SDHCI spec. version */
 
@@ -125,6 +131,8 @@ struct sdhci_host {
 	unsigned int clock;	/* Current clock (MHz) */
 	u8 pwr;			/* Current voltage */
 
+	bool runtime_suspended;	/* Host is runtime suspended */
+
 	struct mmc_request *mrq;	/* Current request */
 	struct mmc_command *cmd;	/* Current command */
 	struct mmc_data *data;	/* Current data request */
-- 
1.7.6

Re: [PATCH 2/2] mmc: sdhci-pci: add runtime pm support

[Jaehoon Chung]

Hi Adrian...

> +#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;
> +	pm_message_t state = { .event = PM_EVENT_SUSPEND };
> +	int i, ret;
> +
> +	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_host(slot->host);
> +
> +		if (ret) {
> +			for (i--; i >= 0; i--)
> +				sdhci_runtime_resume_host(chip->slots[i]->host);
> +			return ret;
> +		}
> +	}
> +
> +	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_host(chip->slots[i]->host);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}


sdhci_runtime_resume_host() is missed return-value.
And i looked into sdhci_runtime_resume/suspend_host()...but
always return 0...why did you check return-value..?

> +
> +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;
> +
> +	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_host(slot->host);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sdhci_pci_runtime_idle(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +#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,
> +};
> +
>  /*****************************************************************************\
>   *                                                                           *
>   * Device probing/removal                                                    *
> @@ -1132,6 +1290,21 @@ static void sdhci_pci_remove_slot(struct sdhci_pci_slot *slot)
>  	sdhci_free_host(slot->host);
>  }
>  
> +static void __devinit sdhci_pci_runtime_pm_allow(struct device *dev)
> +{
> +	pm_runtime_put_noidle(dev);
> +	pm_runtime_allow(dev);
> +	pm_runtime_set_autosuspend_delay(dev, 50);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_suspend_ignore_children(dev, 1);
> +}
> +
> +static void __devexit sdhci_pci_runtime_pm_forbid(struct device *dev)
> +{
> +	pm_runtime_forbid(dev);
> +	pm_runtime_get_noresume(dev);
> +}
> +
>  static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
>  				     const struct pci_device_id *ent)
>  {
> @@ -1207,6 +1380,8 @@ static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
>  		chip->slots[i] = slot;
>  	}
>  
> +	sdhci_pci_runtime_pm_allow(&pdev->dev);
> +
>  	return 0;
>  
>  free:
> @@ -1223,6 +1398,8 @@ static void __devexit sdhci_pci_remove(struct pci_dev *pdev)
>  	int i;
>  	struct sdhci_pci_chip *chip;
>  
> +	sdhci_pci_runtime_pm_forbid(&pdev->dev);
> +
>  	chip = pci_get_drvdata(pdev);
>  
>  	if (chip) {
> @@ -1243,6 +1420,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 d66a7a1..787b877 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>
>  
> @@ -41,6 +42,7 @@
>  #define MAX_TUNING_LOOP 40
>  
>  static unsigned int debug_quirks = 0;
> +static unsigned int debug_quirks2;
>  
>  static void sdhci_finish_data(struct sdhci_host *);
>  
> @@ -49,6 +51,20 @@ static void sdhci_finish_command(struct sdhci_host *);
>  static int sdhci_execute_tuning(struct mmc_host *mmc);
>  static void sdhci_tuning_timer(unsigned long data);
>  
> +#ifdef CONFIG_PM_RUNTIME
> +static int sdhci_runtime_pm_get(struct sdhci_host *host);
> +static int sdhci_runtime_pm_put(struct sdhci_host *host);
> +#else
> +static inline int sdhci_runtime_pm_get(struct sdhci_host *host)
> +{
> +	return 0;
> +}
> +static inline int sdhci_runtime_pm_put(struct sdhci_host *host)
> +{
> +	return 0;
> +}
> +#endif
> +
>  static void sdhci_dumpregs(struct sdhci_host *host)
>  {
>  	printk(KERN_DEBUG DRIVER_NAME ": =========== REGISTER DUMP (%s)===========\n",
> @@ -132,6 +148,9 @@ static void sdhci_set_card_detection(struct sdhci_host *host, bool enable)
>  	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
>  		return;
>  
> +	if (host->quirks2 & SDHCI_QUIRK2_OWN_CARD_DETECTION)
> +		return;
> +
>  	present = sdhci_readl(host, SDHCI_PRESENT_STATE) &
>  			      SDHCI_CARD_PRESENT;
>  	irqs = present ? SDHCI_INT_CARD_REMOVE : SDHCI_INT_CARD_INSERT;
> @@ -251,11 +270,14 @@ static void sdhci_led_control(struct led_classdev *led,
>  
>  	spin_lock_irqsave(&host->lock, flags);
>  
> +	if (host->runtime_suspended)
> +		goto out;
> +
>  	if (brightness == LED_OFF)
>  		sdhci_deactivate_led(host);
>  	else
>  		sdhci_activate_led(host);
> -
> +out:
>  	spin_unlock_irqrestore(&host->lock, flags);
>  }
>  #endif
> @@ -1209,6 +1231,8 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  
>  	host = mmc_priv(mmc);
>  
> +	sdhci_runtime_pm_get(host);
> +
>  	spin_lock_irqsave(&host->lock, flags);
>  
>  	WARN_ON(host->mrq != NULL);
> @@ -1269,14 +1293,11 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  	spin_unlock_irqrestore(&host->lock, flags);
>  }
>  
> -static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> +static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
>  {
> -	struct sdhci_host *host;
>  	unsigned long flags;
>  	u8 ctrl;
>  
> -	host = mmc_priv(mmc);
> -
>  	spin_lock_irqsave(&host->lock, flags);
>  
>  	if (host->flags & SDHCI_DEVICE_DEAD)
> @@ -1426,7 +1447,16 @@ out:
>  	spin_unlock_irqrestore(&host->lock, flags);
>  }
>  
> -static int check_ro(struct sdhci_host *host)
> +static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> +{
> +	struct sdhci_host *host = mmc_priv(mmc);
> +
> +	sdhci_runtime_pm_get(host);
> +	sdhci_do_set_ios(host, ios);
> +	sdhci_runtime_pm_put(host);
> +}
> +
> +static int sdhci_check_ro(struct sdhci_host *host)
>  {
>  	unsigned long flags;
>  	int is_readonly;
> @@ -1450,19 +1480,16 @@ static int check_ro(struct sdhci_host *host)
>  
>  #define SAMPLE_COUNT	5
>  
> -static int sdhci_get_ro(struct mmc_host *mmc)
> +static int sdhci_do_get_ro(struct sdhci_host *host)
>  {
> -	struct sdhci_host *host;
>  	int i, ro_count;
>  
> -	host = mmc_priv(mmc);
> -
>  	if (!(host->quirks & SDHCI_QUIRK_UNSTABLE_RO_DETECT))
> -		return check_ro(host);
> +		return sdhci_check_ro(host);
>  
>  	ro_count = 0;
>  	for (i = 0; i < SAMPLE_COUNT; i++) {
> -		if (check_ro(host)) {
> +		if (sdhci_check_ro(host)) {
>  			if (++ro_count > SAMPLE_COUNT / 2)
>  				return 1;
>  		}
> @@ -1479,38 +1506,56 @@ static void sdhci_hw_reset(struct mmc_host *mmc)
>  		host->ops->hw_reset(host);
>  }
>  
> -static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
> +static int sdhci_get_ro(struct mmc_host *mmc)
>  {
> -	struct sdhci_host *host;
> -	unsigned long flags;
> -
> -	host = mmc_priv(mmc);
> +	struct sdhci_host *host = mmc_priv(mmc);
> +	int ret;
>  
> -	spin_lock_irqsave(&host->lock, flags);
> +	sdhci_runtime_pm_get(host);
> +	ret = sdhci_do_get_ro(host);
> +	sdhci_runtime_pm_put(host);
> +	return ret;
> +}
>  
> +static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int enable)
> +{
>  	if (host->flags & SDHCI_DEVICE_DEAD)
>  		goto out;
>  
>  	if (enable)
> +		host->flags |= SDHCI_SDIO_IRQ_ENABLED;
> +	else
> +		host->flags &= ~SDHCI_SDIO_IRQ_ENABLED;
> +
> +	/* SDIO IRQ will be enabled as appropriate in runtime resume */
> +	if (host->runtime_suspended)
> +		goto out;
> +
> +	if (enable)
>  		sdhci_unmask_irqs(host, SDHCI_INT_CARD_INT);
>  	else
>  		sdhci_mask_irqs(host, SDHCI_INT_CARD_INT);
>  out:
>  	mmiowb();
> +}
> +
> +static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
> +{
> +	struct sdhci_host *host = mmc_priv(mmc);
> +	unsigned long flags;
>  
> +	spin_lock_irqsave(&host->lock, flags);
> +	sdhci_enable_sdio_irq_nolock(host, enable);
>  	spin_unlock_irqrestore(&host->lock, flags);
>  }
>  
> -static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
> -	struct mmc_ios *ios)
> +static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
> +						struct mmc_ios *ios)
>  {
> -	struct sdhci_host *host;
>  	u8 pwr;
>  	u16 clk, ctrl;
>  	u32 present_state;
>  
> -	host = mmc_priv(mmc);
> -
>  	/*
>  	 * Signal Voltage Switching is only applicable for Host Controllers
>  	 * v3.00 and above.
> @@ -1603,6 +1648,20 @@ static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
>  		return 0;
>  }
>  
> +static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
> +	struct mmc_ios *ios)
> +{
> +	struct sdhci_host *host = mmc_priv(mmc);
> +	int err;
> +
> +	if (host->version < SDHCI_SPEC_300)
> +		return 0;
> +	sdhci_runtime_pm_get(host);
> +	err = sdhci_do_start_signal_voltage_switch(host, ios);
> +	sdhci_runtime_pm_put(host);
> +	return err;
> +}
> +
>  static int sdhci_execute_tuning(struct mmc_host *mmc)
>  {
>  	struct sdhci_host *host;
> @@ -1614,6 +1673,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc)
>  
>  	host = mmc_priv(mmc);
>  
> +	sdhci_runtime_pm_get(host);
>  	disable_irq(host->irq);
>  	spin_lock(&host->lock);
>  
> @@ -1631,6 +1691,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc)
>  	else {
>  		spin_unlock(&host->lock);
>  		enable_irq(host->irq);
> +		sdhci_runtime_pm_put(host);
>  		return 0;
>  	}
>  
> @@ -1656,7 +1717,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc)
>  	timeout = 150;
>  	do {
>  		struct mmc_command cmd = {0};
> -		struct mmc_request mrq = {0};
> +		struct mmc_request mrq = {NULL};
>  
>  		if (!tuning_loop_counter && !timeout)
>  			break;
> @@ -1774,18 +1835,16 @@ out:
>  	sdhci_clear_set_irqs(host, SDHCI_INT_DATA_AVAIL, ier);
>  	spin_unlock(&host->lock);
>  	enable_irq(host->irq);
> +	sdhci_runtime_pm_put(host);
>  
>  	return err;
>  }
>  
> -static void sdhci_enable_preset_value(struct mmc_host *mmc, bool enable)
> +static void sdhci_do_enable_preset_value(struct sdhci_host *host, bool enable)
>  {
> -	struct sdhci_host *host;
>  	u16 ctrl;
>  	unsigned long flags;
>  
> -	host = mmc_priv(mmc);
> -
>  	/* Host Controller v3.00 defines preset value registers */
>  	if (host->version < SDHCI_SPEC_300)
>  		return;
> @@ -1801,14 +1860,25 @@ static void sdhci_enable_preset_value(struct mmc_host *mmc, bool enable)
>  	if (enable && !(ctrl & SDHCI_CTRL_PRESET_VAL_ENABLE)) {
>  		ctrl |= SDHCI_CTRL_PRESET_VAL_ENABLE;
>  		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
> +		host->flags |= SDHCI_PV_ENABLED;
>  	} else if (!enable && (ctrl & SDHCI_CTRL_PRESET_VAL_ENABLE)) {
>  		ctrl &= ~SDHCI_CTRL_PRESET_VAL_ENABLE;
>  		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
> +		host->flags &= ~SDHCI_PV_ENABLED;
>  	}
>  
>  	spin_unlock_irqrestore(&host->lock, flags);
>  }
>  
> +static void sdhci_enable_preset_value(struct mmc_host *mmc, bool enable)
> +{
> +	struct sdhci_host *host = mmc_priv(mmc);
> +
> +	sdhci_runtime_pm_get(host);
> +	sdhci_do_enable_preset_value(host, enable);
> +	sdhci_runtime_pm_put(host);
> +}
> +
>  static const struct mmc_host_ops sdhci_ops = {
>  	.request	= sdhci_request,
>  	.set_ios	= sdhci_set_ios,
> @@ -1835,19 +1905,19 @@ static void sdhci_tasklet_card(unsigned long param)
>  
>  	spin_lock_irqsave(&host->lock, flags);
>  
> -	if (!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)) {
> -		if (host->mrq) {
> -			printk(KERN_ERR "%s: Card removed during transfer!\n",
> -				mmc_hostname(host->mmc));
> -			printk(KERN_ERR "%s: Resetting controller.\n",
> -				mmc_hostname(host->mmc));
> +	/* Check host->mrq first in case we are runtime suspended */
> +	if (host->mrq &&
> +	    !(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)) {
> +		printk(KERN_ERR "%s: Card removed during transfer!\n",
> +			mmc_hostname(host->mmc));
> +		printk(KERN_ERR "%s: Resetting controller.\n",
> +			mmc_hostname(host->mmc));
>  
> -			sdhci_reset(host, SDHCI_RESET_CMD);
> -			sdhci_reset(host, SDHCI_RESET_DATA);
> +		sdhci_reset(host, SDHCI_RESET_CMD);
> +		sdhci_reset(host, SDHCI_RESET_DATA);
>  
> -			host->mrq->cmd->error = -ENOMEDIUM;
> -			tasklet_schedule(&host->finish_tasklet);
> -		}
> +		host->mrq->cmd->error = -ENOMEDIUM;
> +		tasklet_schedule(&host->finish_tasklet);
>  	}
>  
>  	spin_unlock_irqrestore(&host->lock, flags);
> @@ -1863,14 +1933,16 @@ static void sdhci_tasklet_finish(unsigned long param)
>  
>  	host = (struct sdhci_host*)param;
>  
> +	spin_lock_irqsave(&host->lock, flags);
> +
>          /*
>           * If this tasklet gets rescheduled while running, it will
>           * be run again afterwards but without any active request.
>           */
> -	if (!host->mrq)
> +	if (!host->mrq) {
> +		spin_unlock_irqrestore(&host->lock, flags);
>  		return;
> -
> -	spin_lock_irqsave(&host->lock, flags);
> +	}
>  
>  	del_timer(&host->timer);
>  
> @@ -1914,6 +1986,7 @@ static void sdhci_tasklet_finish(unsigned long param)
>  	spin_unlock_irqrestore(&host->lock, flags);
>  
>  	mmc_request_done(host->mmc, mrq);
> +	sdhci_runtime_pm_put(host);
>  }
>  
>  static void sdhci_timeout_timer(unsigned long data)
> @@ -2145,12 +2218,19 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
>  static irqreturn_t sdhci_irq(int irq, void *dev_id)
>  {
>  	irqreturn_t result;
> -	struct sdhci_host* host = dev_id;
> +	struct sdhci_host *host = dev_id;
>  	u32 intmask;
>  	int cardint = 0;
>  
>  	spin_lock(&host->lock);
>  
> +	if (host->runtime_suspended) {
> +		spin_unlock(&host->lock);
> +		printk(KERN_WARNING "%s: got irq while runtime suspended\n",
> +		       mmc_hostname(host->mmc));
> +		return IRQ_HANDLED;
> +	}
> +
>  	intmask = sdhci_readl(host, SDHCI_INT_STATUS);
>  
>  	if (!intmask || intmask == 0xffffffff) {
> @@ -2284,7 +2364,6 @@ int sdhci_resume_host(struct sdhci_host *host)
>  			return ret;
>  	}
>  
> -
>  	if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA)) {
>  		if (host->ops->enable_dma)
>  			host->ops->enable_dma(host);
> @@ -2323,6 +2402,90 @@ EXPORT_SYMBOL_GPL(sdhci_enable_irq_wakeups);
>  
>  #endif /* CONFIG_PM */
>  
> +#ifdef CONFIG_PM_RUNTIME
> +
> +static int sdhci_runtime_pm_get(struct sdhci_host *host)
> +{
> +	return pm_runtime_get_sync(host->mmc->parent);
> +}
> +
> +static int sdhci_runtime_pm_put(struct sdhci_host *host)
> +{
> +	pm_runtime_mark_last_busy(host->mmc->parent);
> +	return pm_runtime_put_autosuspend(host->mmc->parent);
> +}
> +
> +int sdhci_runtime_suspend_host(struct sdhci_host *host)
> +{
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	/* Disable tuning since we are suspending */
> +	if (host->version >= SDHCI_SPEC_300 &&
> +	    host->tuning_mode == SDHCI_TUNING_MODE_1) {
> +		del_timer_sync(&host->tuning_timer);
> +		host->flags &= ~SDHCI_NEEDS_RETUNING;
> +	}
> +
> +	spin_lock_irqsave(&host->lock, flags);
> +	sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> +	spin_unlock_irqrestore(&host->lock, flags);
> +
> +	synchronize_irq(host->irq);
> +
> +	spin_lock_irqsave(&host->lock, flags);
> +	host->runtime_suspended = true;
> +	spin_unlock_irqrestore(&host->lock, flags);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(sdhci_runtime_suspend_host);
> +
> +int sdhci_runtime_resume_host(struct sdhci_host *host)
> +{
> +	unsigned long flags;
> +	int ret = 0, host_flags = host->flags;
> +
> +	if (host_flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA)) {
> +		if (host->ops->enable_dma)
> +			host->ops->enable_dma(host);
> +	}
> +
> +	sdhci_init(host, 0);
> +
> +	/* Force clock and power re-program */
> +	host->pwr = 0;
> +	host->clock = 0;
> +	sdhci_do_set_ios(host, &host->mmc->ios);
> +
> +	sdhci_do_start_signal_voltage_switch(host, &host->mmc->ios);
> +	if (host_flags & SDHCI_PV_ENABLED)
> +		sdhci_do_enable_preset_value(host, true);
> +
> +	/* Set the re-tuning expiration flag */
> +	if ((host->version >= SDHCI_SPEC_300) && host->tuning_count &&
> +	    (host->tuning_mode == SDHCI_TUNING_MODE_1))
> +		host->flags |= SDHCI_NEEDS_RETUNING;
> +
> +	spin_lock_irqsave(&host->lock, flags);
> +
> +	host->runtime_suspended = false;
> +
> +	/* Enable SDIO IRQ */
> +	if ((host->flags & SDHCI_SDIO_IRQ_ENABLED))
> +		sdhci_enable_sdio_irq_nolock(host, true);
> +
> +	/* Enable Card Detection */
> +	sdhci_enable_card_detection(host);
> +
> +	spin_unlock_irqrestore(&host->lock, flags);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(sdhci_runtime_resume_host);
> +
> +#endif
> +
>  /*****************************************************************************\
>   *                                                                           *
>   * Device allocation/registration                                            *
> @@ -2365,6 +2528,8 @@ int sdhci_add_host(struct sdhci_host *host)
>  
>  	if (debug_quirks)
>  		host->quirks = debug_quirks;
> +	if (debug_quirks2)
> +		host->quirks2 = debug_quirks2;
>  
>  	sdhci_reset(host, SDHCI_RESET_ALL);
>  
> @@ -2887,9 +3052,11 @@ module_init(sdhci_drv_init);
>  module_exit(sdhci_drv_exit);
>  
>  module_param(debug_quirks, uint, 0444);
> +module_param(debug_quirks2, uint, 0444);
>  
>  MODULE_AUTHOR("Pierre Ossman <pierre@ossman.eu>");
>  MODULE_DESCRIPTION("Secure Digital Host Controller Interface core driver");
>  MODULE_LICENSE("GPL");
>  
>  MODULE_PARM_DESC(debug_quirks, "Force certain quirks.");
> +MODULE_PARM_DESC(debug_quirks, "Force certain other quirks.");
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 7bd919c..0a5b654 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -379,4 +379,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_host(struct sdhci_host *host);
> +extern int sdhci_runtime_resume_host(struct sdhci_host *host);
> +#endif
> +
>  #endif /* __SDHCI_HW_H */
> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
> index 5666f3a..e4b6935 100644
> --- a/include/linux/mmc/sdhci.h
> +++ b/include/linux/mmc/sdhci.h
> @@ -88,6 +88,10 @@ struct sdhci_host {
>  /* The read-only detection via SDHCI_PRESENT_STATE register is unstable */
>  #define SDHCI_QUIRK_UNSTABLE_RO_DETECT			(1<<31)
>  
> +	unsigned int quirks2;	/* More deviations from spec. */
> +
> +#define SDHCI_QUIRK2_OWN_CARD_DETECTION			(1<<0)
> +
>  	int irq;		/* Device IRQ */
>  	void __iomem *ioaddr;	/* Mapped address */
>  
> @@ -115,6 +119,8 @@ struct sdhci_host {
>  #define SDHCI_NEEDS_RETUNING	(1<<5)	/* Host needs retuning */
>  #define SDHCI_AUTO_CMD12	(1<<6)	/* Auto CMD12 support */
>  #define SDHCI_AUTO_CMD23	(1<<7)	/* Auto CMD23 support */
> +#define SDHCI_PV_ENABLED	(1<<8)	/* Preset value enabled */
> +#define SDHCI_SDIO_IRQ_ENABLED	(1<<9)	/* SDIO irq enabled */
>  
>  	unsigned int version;	/* SDHCI spec. version */
>  
> @@ -125,6 +131,8 @@ struct sdhci_host {
>  	unsigned int clock;	/* Current clock (MHz) */
>  	u8 pwr;			/* Current voltage */
>  
> +	bool runtime_suspended;	/* Host is runtime suspended */
> +
>  	struct mmc_request *mrq;	/* Current request */
>  	struct mmc_command *cmd;	/* Current command */
>  	struct mmc_data *data;	/* Current data request */

Re: [PATCH 2/2] mmc: sdhci-pci: add runtime pm support

[Adrian Hunter]

On 04/10/11 07:44, Jaehoon Chung wrote:
> Hi Adrian...
>
>> +#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;
>> +	pm_message_t state = { .event = PM_EVENT_SUSPEND };
>> +	int i, ret;
>> +
>> +	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_host(slot->host);
>> +
>> +		if (ret) {
>> +			for (i--; i>= 0; i--)
>> +				sdhci_runtime_resume_host(chip->slots[i]->host);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	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_host(chip->slots[i]->host);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>
>
> sdhci_runtime_resume_host() is missed return-value.

It is not needed, since we are anyway returning an error.

> And i looked into sdhci_runtime_resume/suspend_host()...but
> always return 0...why did you check return-value..?

sdhci_runtime_resume/suspend_host() will evolve and may return
non-zero in the future.

>
>> +
>> +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;
>> +
>> +	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_host(slot->host);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int sdhci_pci_runtime_idle(struct device *dev)
>> +{
>> +	return 0;
>> +}
>> +
>> +#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,
>> +};
>> +
>>   /*****************************************************************************\
>>    *                                                                           *
>>    * Device probing/removal                                                    *
>> @@ -1132,6 +1290,21 @@ static void sdhci_pci_remove_slot(struct sdhci_pci_slot *slot)
>>   	sdhci_free_host(slot->host);
>>   }
>>
>> +static void __devinit sdhci_pci_runtime_pm_allow(struct device *dev)
>> +{
>> +	pm_runtime_put_noidle(dev);
>> +	pm_runtime_allow(dev);
>> +	pm_runtime_set_autosuspend_delay(dev, 50);
>> +	pm_runtime_use_autosuspend(dev);
>> +	pm_suspend_ignore_children(dev, 1);
>> +}
>> +
>> +static void __devexit sdhci_pci_runtime_pm_forbid(struct device *dev)
>> +{
>> +	pm_runtime_forbid(dev);
>> +	pm_runtime_get_noresume(dev);
>> +}
>> +
>>   static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
>>   				     const struct pci_device_id *ent)
>>   {
>> @@ -1207,6 +1380,8 @@ static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
>>   		chip->slots[i] = slot;
>>   	}
>>
>> +	sdhci_pci_runtime_pm_allow(&pdev->dev);
>> +
>>   	return 0;
>>
>>   free:
>> @@ -1223,6 +1398,8 @@ static void __devexit sdhci_pci_remove(struct pci_dev *pdev)
>>   	int i;
>>   	struct sdhci_pci_chip *chip;
>>
>> +	sdhci_pci_runtime_pm_forbid(&pdev->dev);
>> +
>>   	chip = pci_get_drvdata(pdev);
>>
>>   	if (chip) {
>> @@ -1243,6 +1420,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 d66a7a1..787b877 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>
>>
>> @@ -41,6 +42,7 @@
>>   #define MAX_TUNING_LOOP 40
>>
>>   static unsigned int debug_quirks = 0;
>> +static unsigned int debug_quirks2;
>>
>>   static void sdhci_finish_data(struct sdhci_host *);
>>
>> @@ -49,6 +51,20 @@ static void sdhci_finish_command(struct sdhci_host *);
>>   static int sdhci_execute_tuning(struct mmc_host *mmc);
>>   static void sdhci_tuning_timer(unsigned long data);
>>
>> +#ifdef CONFIG_PM_RUNTIME
>> +static int sdhci_runtime_pm_get(struct sdhci_host *host);
>> +static int sdhci_runtime_pm_put(struct sdhci_host *host);
>> +#else
>> +static inline int sdhci_runtime_pm_get(struct sdhci_host *host)
>> +{
>> +	return 0;
>> +}
>> +static inline int sdhci_runtime_pm_put(struct sdhci_host *host)
>> +{
>> +	return 0;
>> +}
>> +#endif
>> +
>>   static void sdhci_dumpregs(struct sdhci_host *host)
>>   {
>>   	printk(KERN_DEBUG DRIVER_NAME ": =========== REGISTER DUMP (%s)===========\n",
>> @@ -132,6 +148,9 @@ static void sdhci_set_card_detection(struct sdhci_host *host, bool enable)
>>   	if (host->quirks&  SDHCI_QUIRK_BROKEN_CARD_DETECTION)
>>   		return;
>>
>> +	if (host->quirks2&  SDHCI_QUIRK2_OWN_CARD_DETECTION)
>> +		return;
>> +
>>   	present = sdhci_readl(host, SDHCI_PRESENT_STATE)&
>>   			SDHCI_CARD_PRESENT;
>>   	irqs = present ? SDHCI_INT_CARD_REMOVE : SDHCI_INT_CARD_INSERT;
>> @@ -251,11 +270,14 @@ static void sdhci_led_control(struct led_classdev *led,
>>
>>   	spin_lock_irqsave(&host->lock, flags);
>>
>> +	if (host->runtime_suspended)
>> +		goto out;
>> +
>>   	if (brightness == LED_OFF)
>>   		sdhci_deactivate_led(host);
>>   	else
>>   		sdhci_activate_led(host);
>> -
>> +out:
>>   	spin_unlock_irqrestore(&host->lock, flags);
>>   }
>>   #endif
>> @@ -1209,6 +1231,8 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>>
>>   	host = mmc_priv(mmc);
>>
>> +	sdhci_runtime_pm_get(host);
>> +
>>   	spin_lock_irqsave(&host->lock, flags);
>>
>>   	WARN_ON(host->mrq != NULL);
>> @@ -1269,14 +1293,11 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>>   	spin_unlock_irqrestore(&host->lock, flags);
>>   }
>>
>> -static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>> +static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
>>   {
>> -	struct sdhci_host *host;
>>   	unsigned long flags;
>>   	u8 ctrl;
>>
>> -	host = mmc_priv(mmc);
>> -
>>   	spin_lock_irqsave(&host->lock, flags);
>>
>>   	if (host->flags&  SDHCI_DEVICE_DEAD)
>> @@ -1426,7 +1447,16 @@ out:
>>   	spin_unlock_irqrestore(&host->lock, flags);
>>   }
>>
>> -static int check_ro(struct sdhci_host *host)
>> +static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>> +{
>> +	struct sdhci_host *host = mmc_priv(mmc);
>> +
>> +	sdhci_runtime_pm_get(host);
>> +	sdhci_do_set_ios(host, ios);
>> +	sdhci_runtime_pm_put(host);
>> +}
>> +
>> +static int sdhci_check_ro(struct sdhci_host *host)
>>   {
>>   	unsigned long flags;
>>   	int is_readonly;
>> @@ -1450,19 +1480,16 @@ static int check_ro(struct sdhci_host *host)
>>
>>   #define SAMPLE_COUNT	5
>>
>> -static int sdhci_get_ro(struct mmc_host *mmc)
>> +static int sdhci_do_get_ro(struct sdhci_host *host)
>>   {
>> -	struct sdhci_host *host;
>>   	int i, ro_count;
>>
>> -	host = mmc_priv(mmc);
>> -
>>   	if (!(host->quirks&  SDHCI_QUIRK_UNSTABLE_RO_DETECT))
>> -		return check_ro(host);
>> +		return sdhci_check_ro(host);
>>
>>   	ro_count = 0;
>>   	for (i = 0; i<  SAMPLE_COUNT; i++) {
>> -		if (check_ro(host)) {
>> +		if (sdhci_check_ro(host)) {
>>   			if (++ro_count>  SAMPLE_COUNT / 2)
>>   				return 1;
>>   		}
>> @@ -1479,38 +1506,56 @@ static void sdhci_hw_reset(struct mmc_host *mmc)
>>   		host->ops->hw_reset(host);
>>   }
>>
>> -static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
>> +static int sdhci_get_ro(struct mmc_host *mmc)
>>   {
>> -	struct sdhci_host *host;
>> -	unsigned long flags;
>> -
>> -	host = mmc_priv(mmc);
>> +	struct sdhci_host *host = mmc_priv(mmc);
>> +	int ret;
>>
>> -	spin_lock_irqsave(&host->lock, flags);
>> +	sdhci_runtime_pm_get(host);
>> +	ret = sdhci_do_get_ro(host);
>> +	sdhci_runtime_pm_put(host);
>> +	return ret;
>> +}
>>
>> +static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int enable)
>> +{
>>   	if (host->flags&  SDHCI_DEVICE_DEAD)
>>   		goto out;
>>
>>   	if (enable)
>> +		host->flags |= SDHCI_SDIO_IRQ_ENABLED;
>> +	else
>> +		host->flags&= ~SDHCI_SDIO_IRQ_ENABLED;
>> +
>> +	/* SDIO IRQ will be enabled as appropriate in runtime resume */
>> +	if (host->runtime_suspended)
>> +		goto out;
>> +
>> +	if (enable)
>>   		sdhci_unmask_irqs(host, SDHCI_INT_CARD_INT);
>>   	else
>>   		sdhci_mask_irqs(host, SDHCI_INT_CARD_INT);
>>   out:
>>   	mmiowb();
>> +}
>> +
>> +static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
>> +{
>> +	struct sdhci_host *host = mmc_priv(mmc);
>> +	unsigned long flags;
>>
>> +	spin_lock_irqsave(&host->lock, flags);
>> +	sdhci_enable_sdio_irq_nolock(host, enable);
>>   	spin_unlock_irqrestore(&host->lock, flags);
>>   }
>>
>> -static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
>> -	struct mmc_ios *ios)
>> +static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
>> +						struct mmc_ios *ios)
>>   {
>> -	struct sdhci_host *host;
>>   	u8 pwr;
>>   	u16 clk, ctrl;
>>   	u32 present_state;
>>
>> -	host = mmc_priv(mmc);
>> -
>>   	/*
>>   	 * Signal Voltage Switching is only applicable for Host Controllers
>>   	 * v3.00 and above.
>> @@ -1603,6 +1648,20 @@ static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
>>   		return 0;
>>   }
>>
>> +static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
>> +	struct mmc_ios *ios)
>> +{
>> +	struct sdhci_host *host = mmc_priv(mmc);
>> +	int err;
>> +
>> +	if (host->version<  SDHCI_SPEC_300)
>> +		return 0;
>> +	sdhci_runtime_pm_get(host);
>> +	err = sdhci_do_start_signal_voltage_switch(host, ios);
>> +	sdhci_runtime_pm_put(host);
>> +	return err;
>> +}
>> +
>>   static int sdhci_execute_tuning(struct mmc_host *mmc)
>>   {
>>   	struct sdhci_host *host;
>> @@ -1614,6 +1673,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc)
>>
>>   	host = mmc_priv(mmc);
>>
>> +	sdhci_runtime_pm_get(host);
>>   	disable_irq(host->irq);
>>   	spin_lock(&host->lock);
>>
>> @@ -1631,6 +1691,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc)
>>   	else {
>>   		spin_unlock(&host->lock);
>>   		enable_irq(host->irq);
>> +		sdhci_runtime_pm_put(host);
>>   		return 0;
>>   	}
>>
>> @@ -1656,7 +1717,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc)
>>   	timeout = 150;
>>   	do {
>>   		struct mmc_command cmd = {0};
>> -		struct mmc_request mrq = {0};
>> +		struct mmc_request mrq = {NULL};
>>
>>   		if (!tuning_loop_counter&&  !timeout)
>>   			break;
>> @@ -1774,18 +1835,16 @@ out:
>>   	sdhci_clear_set_irqs(host, SDHCI_INT_DATA_AVAIL, ier);
>>   	spin_unlock(&host->lock);
>>   	enable_irq(host->irq);
>> +	sdhci_runtime_pm_put(host);
>>
>>   	return err;
>>   }
>>
>> -static void sdhci_enable_preset_value(struct mmc_host *mmc, bool enable)
>> +static void sdhci_do_enable_preset_value(struct sdhci_host *host, bool enable)
>>   {
>> -	struct sdhci_host *host;
>>   	u16 ctrl;
>>   	unsigned long flags;
>>
>> -	host = mmc_priv(mmc);
>> -
>>   	/* Host Controller v3.00 defines preset value registers */
>>   	if (host->version<  SDHCI_SPEC_300)
>>   		return;
>> @@ -1801,14 +1860,25 @@ static void sdhci_enable_preset_value(struct mmc_host *mmc, bool enable)
>>   	if (enable&&  !(ctrl&  SDHCI_CTRL_PRESET_VAL_ENABLE)) {
>>   		ctrl |= SDHCI_CTRL_PRESET_VAL_ENABLE;
>>   		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>> +		host->flags |= SDHCI_PV_ENABLED;
>>   	} else if (!enable&&  (ctrl&  SDHCI_CTRL_PRESET_VAL_ENABLE)) {
>>   		ctrl&= ~SDHCI_CTRL_PRESET_VAL_ENABLE;
>>   		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>> +		host->flags&= ~SDHCI_PV_ENABLED;
>>   	}
>>
>>   	spin_unlock_irqrestore(&host->lock, flags);
>>   }
>>
>> +static void sdhci_enable_preset_value(struct mmc_host *mmc, bool enable)
>> +{
>> +	struct sdhci_host *host = mmc_priv(mmc);
>> +
>> +	sdhci_runtime_pm_get(host);
>> +	sdhci_do_enable_preset_value(host, enable);
>> +	sdhci_runtime_pm_put(host);
>> +}
>> +
>>   static const struct mmc_host_ops sdhci_ops = {
>>   	.request	= sdhci_request,
>>   	.set_ios	= sdhci_set_ios,
>> @@ -1835,19 +1905,19 @@ static void sdhci_tasklet_card(unsigned long param)
>>
>>   	spin_lock_irqsave(&host->lock, flags);
>>
>> -	if (!(sdhci_readl(host, SDHCI_PRESENT_STATE)&  SDHCI_CARD_PRESENT)) {
>> -		if (host->mrq) {
>> -			printk(KERN_ERR "%s: Card removed during transfer!\n",
>> -				mmc_hostname(host->mmc));
>> -			printk(KERN_ERR "%s: Resetting controller.\n",
>> -				mmc_hostname(host->mmc));
>> +	/* Check host->mrq first in case we are runtime suspended */
>> +	if (host->mrq&&
>> +	    !(sdhci_readl(host, SDHCI_PRESENT_STATE)&  SDHCI_CARD_PRESENT)) {
>> +		printk(KERN_ERR "%s: Card removed during transfer!\n",
>> +			mmc_hostname(host->mmc));
>> +		printk(KERN_ERR "%s: Resetting controller.\n",
>> +			mmc_hostname(host->mmc));
>>
>> -			sdhci_reset(host, SDHCI_RESET_CMD);
>> -			sdhci_reset(host, SDHCI_RESET_DATA);
>> +		sdhci_reset(host, SDHCI_RESET_CMD);
>> +		sdhci_reset(host, SDHCI_RESET_DATA);
>>
>> -			host->mrq->cmd->error = -ENOMEDIUM;
>> -			tasklet_schedule(&host->finish_tasklet);
>> -		}
>> +		host->mrq->cmd->error = -ENOMEDIUM;
>> +		tasklet_schedule(&host->finish_tasklet);
>>   	}
>>
>>   	spin_unlock_irqrestore(&host->lock, flags);
>> @@ -1863,14 +1933,16 @@ static void sdhci_tasklet_finish(unsigned long param)
>>
>>   	host = (struct sdhci_host*)param;
>>
>> +	spin_lock_irqsave(&host->lock, flags);
>> +
>>           /*
>>            * If this tasklet gets rescheduled while running, it will
>>            * be run again afterwards but without any active request.
>>            */
>> -	if (!host->mrq)
>> +	if (!host->mrq) {
>> +		spin_unlock_irqrestore(&host->lock, flags);
>>   		return;
>> -
>> -	spin_lock_irqsave(&host->lock, flags);
>> +	}
>>
>>   	del_timer(&host->timer);
>>
>> @@ -1914,6 +1986,7 @@ static void sdhci_tasklet_finish(unsigned long param)
>>   	spin_unlock_irqrestore(&host->lock, flags);
>>
>>   	mmc_request_done(host->mmc, mrq);
>> +	sdhci_runtime_pm_put(host);
>>   }
>>
>>   static void sdhci_timeout_timer(unsigned long data)
>> @@ -2145,12 +2218,19 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
>>   static irqreturn_t sdhci_irq(int irq, void *dev_id)
>>   {
>>   	irqreturn_t result;
>> -	struct sdhci_host* host = dev_id;
>> +	struct sdhci_host *host = dev_id;
>>   	u32 intmask;
>>   	int cardint = 0;
>>
>>   	spin_lock(&host->lock);
>>
>> +	if (host->runtime_suspended) {
>> +		spin_unlock(&host->lock);
>> +		printk(KERN_WARNING "%s: got irq while runtime suspended\n",
>> +		       mmc_hostname(host->mmc));
>> +		return IRQ_HANDLED;
>> +	}
>> +
>>   	intmask = sdhci_readl(host, SDHCI_INT_STATUS);
>>
>>   	if (!intmask || intmask == 0xffffffff) {
>> @@ -2284,7 +2364,6 @@ int sdhci_resume_host(struct sdhci_host *host)
>>   			return ret;
>>   	}
>>
>> -
>>   	if (host->flags&  (SDHCI_USE_SDMA | SDHCI_USE_ADMA)) {
>>   		if (host->ops->enable_dma)
>>   			host->ops->enable_dma(host);
>> @@ -2323,6 +2402,90 @@ EXPORT_SYMBOL_GPL(sdhci_enable_irq_wakeups);
>>
>>   #endif /* CONFIG_PM */
>>
>> +#ifdef CONFIG_PM_RUNTIME
>> +
>> +static int sdhci_runtime_pm_get(struct sdhci_host *host)
>> +{
>> +	return pm_runtime_get_sync(host->mmc->parent);
>> +}
>> +
>> +static int sdhci_runtime_pm_put(struct sdhci_host *host)
>> +{
>> +	pm_runtime_mark_last_busy(host->mmc->parent);
>> +	return pm_runtime_put_autosuspend(host->mmc->parent);
>> +}
>> +
>> +int sdhci_runtime_suspend_host(struct sdhci_host *host)
>> +{
>> +	unsigned long flags;
>> +	int ret = 0;
>> +
>> +	/* Disable tuning since we are suspending */
>> +	if (host->version>= SDHCI_SPEC_300&&
>> +	    host->tuning_mode == SDHCI_TUNING_MODE_1) {
>> +		del_timer_sync(&host->tuning_timer);
>> +		host->flags&= ~SDHCI_NEEDS_RETUNING;
>> +	}
>> +
>> +	spin_lock_irqsave(&host->lock, flags);
>> +	sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
>> +	spin_unlock_irqrestore(&host->lock, flags);
>> +
>> +	synchronize_irq(host->irq);
>> +
>> +	spin_lock_irqsave(&host->lock, flags);
>> +	host->runtime_suspended = true;
>> +	spin_unlock_irqrestore(&host->lock, flags);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(sdhci_runtime_suspend_host);
>> +
>> +int sdhci_runtime_resume_host(struct sdhci_host *host)
>> +{
>> +	unsigned long flags;
>> +	int ret = 0, host_flags = host->flags;
>> +
>> +	if (host_flags&  (SDHCI_USE_SDMA | SDHCI_USE_ADMA)) {
>> +		if (host->ops->enable_dma)
>> +			host->ops->enable_dma(host);
>> +	}
>> +
>> +	sdhci_init(host, 0);
>> +
>> +	/* Force clock and power re-program */
>> +	host->pwr = 0;
>> +	host->clock = 0;
>> +	sdhci_do_set_ios(host,&host->mmc->ios);
>> +
>> +	sdhci_do_start_signal_voltage_switch(host,&host->mmc->ios);
>> +	if (host_flags&  SDHCI_PV_ENABLED)
>> +		sdhci_do_enable_preset_value(host, true);
>> +
>> +	/* Set the re-tuning expiration flag */
>> +	if ((host->version>= SDHCI_SPEC_300)&&  host->tuning_count&&
>> +	    (host->tuning_mode == SDHCI_TUNING_MODE_1))
>> +		host->flags |= SDHCI_NEEDS_RETUNING;
>> +
>> +	spin_lock_irqsave(&host->lock, flags);
>> +
>> +	host->runtime_suspended = false;
>> +
>> +	/* Enable SDIO IRQ */
>> +	if ((host->flags&  SDHCI_SDIO_IRQ_ENABLED))
>> +		sdhci_enable_sdio_irq_nolock(host, true);
>> +
>> +	/* Enable Card Detection */
>> +	sdhci_enable_card_detection(host);
>> +
>> +	spin_unlock_irqrestore(&host->lock, flags);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(sdhci_runtime_resume_host);
>> +
>> +#endif
>> +
>>   /*****************************************************************************\
>>    *                                                                           *
>>    * Device allocation/registration                                            *
>> @@ -2365,6 +2528,8 @@ int sdhci_add_host(struct sdhci_host *host)
>>
>>   	if (debug_quirks)
>>   		host->quirks = debug_quirks;
>> +	if (debug_quirks2)
>> +		host->quirks2 = debug_quirks2;
>>
>>   	sdhci_reset(host, SDHCI_RESET_ALL);
>>
>> @@ -2887,9 +3052,11 @@ module_init(sdhci_drv_init);
>>   module_exit(sdhci_drv_exit);
>>
>>   module_param(debug_quirks, uint, 0444);
>> +module_param(debug_quirks2, uint, 0444);
>>
>>   MODULE_AUTHOR("Pierre Ossman<pierre@ossman.eu>");
>>   MODULE_DESCRIPTION("Secure Digital Host Controller Interface core driver");
>>   MODULE_LICENSE("GPL");
>>
>>   MODULE_PARM_DESC(debug_quirks, "Force certain quirks.");
>> +MODULE_PARM_DESC(debug_quirks, "Force certain other quirks.");
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index 7bd919c..0a5b654 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -379,4 +379,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_host(struct sdhci_host *host);
>> +extern int sdhci_runtime_resume_host(struct sdhci_host *host);
>> +#endif
>> +
>>   #endif /* __SDHCI_HW_H */
>> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
>> index 5666f3a..e4b6935 100644
>> --- a/include/linux/mmc/sdhci.h
>> +++ b/include/linux/mmc/sdhci.h
>> @@ -88,6 +88,10 @@ struct sdhci_host {
>>   /* The read-only detection via SDHCI_PRESENT_STATE register is unstable */
>>   #define SDHCI_QUIRK_UNSTABLE_RO_DETECT			(1<<31)
>>
>> +	unsigned int quirks2;	/* More deviations from spec. */
>> +
>> +#define SDHCI_QUIRK2_OWN_CARD_DETECTION			(1<<0)
>> +
>>   	int irq;		/* Device IRQ */
>>   	void __iomem *ioaddr;	/* Mapped address */
>>
>> @@ -115,6 +119,8 @@ struct sdhci_host {
>>   #define SDHCI_NEEDS_RETUNING	(1<<5)	/* Host needs retuning */
>>   #define SDHCI_AUTO_CMD12	(1<<6)	/* Auto CMD12 support */
>>   #define SDHCI_AUTO_CMD23	(1<<7)	/* Auto CMD23 support */
>> +#define SDHCI_PV_ENABLED	(1<<8)	/* Preset value enabled */
>> +#define SDHCI_SDIO_IRQ_ENABLED	(1<<9)	/* SDIO irq enabled */
>>
>>   	unsigned int version;	/* SDHCI spec. version */
>>
>> @@ -125,6 +131,8 @@ struct sdhci_host {
>>   	unsigned int clock;	/* Current clock (MHz) */
>>   	u8 pwr;			/* Current voltage */
>>
>> +	bool runtime_suspended;	/* Host is runtime suspended */
>> +
>>   	struct mmc_request *mrq;	/* Current request */
>>   	struct mmc_command *cmd;	/* Current command */
>>   	struct mmc_data *data;	/* Current data request */
>
>
>

Re: [PATCH 1/2] mmc: core: move ->request() call from atomic context

[Ulf Hansson]

Adrian Hunter wrote:
> mmc_request_done() is sometimes called from interrupt
> or other atomic context.  Mostly all mmc_request_done()
> does is complete(), however it contains code to retry
> on error, which uses ->request().  As the error path
> is certainly not performance critical, this may be
> moved to the waiting function mmc_wait_for_req_done().
> 
> This allows ->request() to use runtime PM get_sync()
> and guarantee it is never in an atomic context.
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>

Calling back into the host driver directly from mmc_request_done when 
doing error handling were just plain wrong.

This patch is really great, not just for pm_runtime issues!

Acked-by: Ulf Hansson <ulf.hansson@stericsson.com>

Re: [PATCH 2/2] mmc: sdhci-pci: add runtime pm support

[Ulf Hansson]

Adrian Hunter wrote:
> Ths patch allows runtime PM for sdhci-pci, runtime suspending
> after inactivity of 50ms and ensuring runtime resume before
> SDHC registers are accessed.  During runtime suspend, interrupts
> are masked.  The host controller state is restored at runtime
> resume.
> 
> For Medfield, the host controller's card detect mechanism
> is supplanted by an always-on GPIO which provides for card
> detect wake-up.
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>


I am not that into how the sdhci host drivers is working, myself is more 
working with the mmci driver and about to sent out patches with improved 
power management support soon. I would like to take the opportunity to 
discuss in general pm_runtime for mmc/sd/sdio.

Right now the mmc framework supports MMC_CAP_DISABLE, which means the 
host driver shall implement an enable/disable function. Very similar to 
what pm_runtime can be used for, but pm_runtime is a more modern way of 
solving power management. Is this your overall view on this as well?

Anyway, I would then suggest that it should be the framework 
responsibilities of doing:

1. pm_runtime_get_sync -> To make sure the mmc host is runtime resumed 
before the framework is using it. This could be done when "claiming" the 
host or more precisely as the first thing in the mmc_host_enable function.

2. pm_runtime_mark_last_busy -> To put a time stamp at the last time the 
framework used the mmc host. This could be done as the first thing in 
mmc_host_lazy_disable/mmc_host_disable.

3. pm_runtime_put_sync_autosuspend -> To trigger the timer of runtime 
suspending the mmc host driver. This could be done as the first thing in 
mmc_host_lazy_disable/mmc_host_disable.


The mmc host drivers will be responsible of the following:

1. Implementing the runtime_resume, runtime_suspend and possibly 
runtime_idle functions and register them in the device driver struct.

2. Initializing and setup pm_runtime for the mmc host driver. For 
example use:
pm_runtime_set_autosuspend_delay
pm_runtime_use_autosuspend
pm_runtime_enable


Moreover 1:
I have noticed the pm_runtime support is implemented for sdio and when 
having MMC_CAP_POWER_OFF_CARD. This needs to be "cleaned up" after such 
a change. It might also conflict with your patch considering "moreover 
2" below.

Moreover 2:
dev_pm_ops with runtime functions, are implemented for the mmc bus 
(core/bus.c). Maybe this should be moved from the responsibility of the 
bus into the mmc host drivers instead!? I think it makes more sense to 
leave such decisions as doing 
"mmc_power_save_host/mmc_power_restore_host" to each mmc host driver. 
Just because we get runtime suspended that does not have to mean we wnat 
to do mmc_power_save_host... What do you think?

Best regards
Ulf Hansson

Re: [PATCH 2/2] mmc: sdhci-pci: add runtime pm support

[Adrian Hunter]

On 05/10/11 17:59, Ulf Hansson wrote:
> I am not that into how the sdhci host drivers is working, myself is more working
> with the mmci driver and about to sent out patches with improved power
> management support soon. I would like to take the opportunity to discuss in
> general pm_runtime for mmc/sd/sdio.
>
> Right now the mmc framework supports MMC_CAP_DISABLE, which means the host
> driver shall implement an enable/disable function. Very similar to what
> pm_runtime can be used for, but pm_runtime is a more modern way of solving power
> management. Is this your overall view on this as well?

Yes

>
> Anyway, I would then suggest that it should be the framework responsibilities of
> doing:
>
> 1. pm_runtime_get_sync -> To make sure the mmc host is runtime resumed before
> the framework is using it. This could be done when "claiming" the host or more
> precisely as the first thing in the mmc_host_enable function.
>
> 2. pm_runtime_mark_last_busy -> To put a time stamp at the last time the
> framework used the mmc host. This could be done as the first thing in
> mmc_host_lazy_disable/mmc_host_disable.
>
> 3. pm_runtime_put_sync_autosuspend -> To trigger the timer of runtime suspending
> the mmc host driver. This could be done as the first thing in
> mmc_host_lazy_disable/mmc_host_disable.
>
>
> The mmc host drivers will be responsible of the following:
>
> 1. Implementing the runtime_resume, runtime_suspend and possibly runtime_idle
> functions and register them in the device driver struct.
>
> 2. Initializing and setup pm_runtime for the mmc host driver. For example use:
> pm_runtime_set_autosuspend_delay
> pm_runtime_use_autosuspend
> pm_runtime_enable

It has been discussed on the list before that the host controller driver should 
be solely responsible for host controller power.

>
>
> Moreover 1:
> I have noticed the pm_runtime support is implemented for sdio and when having
> MMC_CAP_POWER_OFF_CARD. This needs to be "cleaned up" after such a change. It
> might also conflict with your patch considering "moreover 2" below.

No.  The card and the host controller are separate devices.  The card can be 
powered up when the host controller is off and vice versa.  They are not 
inter-dependent in that regard.

>
> Moreover 2:
> dev_pm_ops with runtime functions, are implemented for the mmc bus (core/bus.c).
> Maybe this should be moved from the responsibility of the bus into the mmc host
> drivers instead!? I think it makes more sense to leave such decisions as doing
> "mmc_power_save_host/mmc_power_restore_host" to each mmc host driver. Just
> because we get runtime suspended that does not have to mean we wnat to do
> mmc_power_save_host... What do you think?

Again, this has been discussed before on the list, that the power management of 
the card is something that only upper layers control e.g. sdio function driver

Re: [PATCH 2/2] mmc: sdhci-pci: add runtime pm support

[Ulf Hansson]

> 
> It has been discussed on the list before that the host controller driver should 
> be solely responsible for host controller power.

OK I see, sorry that I missed that discussion earlier!

Anyway this should be feasible as well even it might mean duplicated 
code in each host driver. A positive side is that it gets more flexible 
for each host driver.

> 
>>
>> Moreover 1:
>> I have noticed the pm_runtime support is implemented for sdio and when having
>> MMC_CAP_POWER_OFF_CARD. This needs to be "cleaned up" after such a change. It
>> might also conflict with your patch considering "moreover 2" below.
> 
> No.  The card and the host controller are separate devices.  The card can be 
> powered up when the host controller is off and vice versa.  They are not 
> inter-dependent in that regard.

I see your point, but it is somewhere here it is getting a bit "messy". :-)

If I understand correct, you mean that the mmc/sd/sdio framework shall
be responsible of handling power management of the bus/card devices. In
other words the framework shall itself decide when it is feasible of
doing mmc_power_restore_host, mmc_power_save_host, mmc_suspend_host,
mmc_resume_host.

This is not the case right now, since most host drivers are normally in
control of calling these functions themselves. Should this be moved to 
the framework then?

> 
>> Moreover 2:
>> dev_pm_ops with runtime functions, are implemented for the mmc bus (core/bus.c).
>> Maybe this should be moved from the responsibility of the bus into the mmc host
>> drivers instead!? I think it makes more sense to leave such decisions as doing
>> "mmc_power_save_host/mmc_power_restore_host" to each mmc host driver. Just
>> because we get runtime suspended that does not have to mean we wnat to do
>> mmc_power_save_host... What do you think?
> 
> Again, this has been discussed before on the list, that the power management of 
> the card is something that only upper layers control e.g. sdio function driver
> 

I got it, thanks a lot for your answers and feedback!


> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


BR
Ulf Hansson

Re: [PATCH 1/2] mmc: core: move ->request() call from atomic context

[Chris Ball]

Hi,

On Wed, Oct 05 2011, Ulf Hansson wrote:
> Adrian Hunter wrote:
>> mmc_request_done() is sometimes called from interrupt
>> or other atomic context.  Mostly all mmc_request_done()
>> does is complete(), however it contains code to retry
>> on error, which uses ->request().  As the error path
>> is certainly not performance critical, this may be
>> moved to the waiting function mmc_wait_for_req_done().
>>
>> This allows ->request() to use runtime PM get_sync()
>> and guarantee it is never in an atomic context.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>
> Calling back into the host driver directly from mmc_request_done when
> doing error handling were just plain wrong.
>
> This patch is really great, not just for pm_runtime issues!
>
> Acked-by: Ulf Hansson <ulf.hansson@stericsson.com>

Pushed to mmc-next for 3.2.  Thanks!

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 2/2] mmc: sdhci-pci: add runtime pm support

[Chris Ball]

Hi,

On Mon, Oct 03 2011, Adrian Hunter wrote:
> Ths patch allows runtime PM for sdhci-pci, runtime suspending
> after inactivity of 50ms and ensuring runtime resume before
> SDHC registers are accessed.  During runtime suspend, interrupts
> are masked.  The host controller state is restored at runtime
> resume.
>
> For Medfield, the host controller's card detect mechanism
> is supplanted by an always-on GPIO which provides for card
> detect wake-up.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>

Pushed to mmc-next for 3.2, thanks.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 2/2] mmc: sdhci-pci: add runtime pm support

[Chris Ball]

Hi Adrian,

On Mon, Oct 03 2011, Adrian Hunter wrote:
> Ths patch allows runtime PM for sdhci-pci, runtime suspending
> after inactivity of 50ms and ensuring runtime resume before
> SDHC registers are accessed.  During runtime suspend, interrupts
> are masked.  The host controller state is restored at runtime
> resume.
>
> For Medfield, the host controller's card detect mechanism
> is supplanted by an always-on GPIO which provides for card
> detect wake-up.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>

Running linux-next on an x86 ThinkPad W510 with Ricoh 1180:e832 reader,
I get a long string of these messages at boot with no card inserted:

[    7.361614] mmc0: got irq while runtime suspended
[    7.381593] mmc0: got irq while runtime suspended
[    7.401602] mmc0: got irq while runtime suspended
[    7.401617] mmc0: got irq while runtime suspended
[    7.421510] mmc0: got irq while runtime suspended
[    7.421525] mmc0: got irq while runtime suspended

Card insertion detection after boot no longer works (nothing new appears
in dmesg), so I'm unable to use the reader.  Are you able to try to
figure this out?  I'm happy to try any patches you suggest.

The full dmesg, with CONFIG_MMC_DEBUG=y, is available at:
   http://chris.printf.net/sdhci-runtime-pm-dmesg

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 2/2] mmc: sdhci-pci: add runtime pm support

[Chris Ball]

Hi,

On Sat, Oct 15 2011, Chris Ball wrote:
> Card insertion detection after boot no longer works (nothing new appears
> in dmesg), so I'm unable to use the reader.  Are you able to try to
> figure this out?  I'm happy to try any patches you suggest.

If I boot with a card inserted, I still see repeated "irq while runtime
suspended" messages for 80 seconds after boot, but the card *does* work.
I don't see further "irq while runtime suspended" messages during or
after I/O to the card at that point.

So, we have a problem of unexpected irqs while runtime suspended, and a
problem of broken card detection when booted without a card inserted
(on my laptop).

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 2/2] mmc: sdhci-pci: add runtime pm support

[Adrian Hunter]

On 16/10/11 05:26, Chris Ball wrote:
> Hi,
> 
> On Sat, Oct 15 2011, Chris Ball wrote:
>> Card insertion detection after boot no longer works (nothing new appears
>> in dmesg), so I'm unable to use the reader.  Are you able to try to
>> figure this out?  I'm happy to try any patches you suggest.
> 
> If I boot with a card inserted, I still see repeated "irq while runtime
> suspended" messages for 80 seconds after boot, but the card *does* work.
> I don't see further "irq while runtime suspended" messages during or
> after I/O to the card at that point.
> 
> So, we have a problem of unexpected irqs while runtime suspended, and a
> problem of broken card detection when booted without a card inserted
> (on my laptop).

Unfortunately I do not have any information about the unexpected irqs.

Card detect will not work because the interrupt is masked while the
host controller is runtime suspended, on the grounds that the host
controller may be powered off and so cannot handle interrupts.  An
alternate mechanism is needed to enable the host controller to
wake-up from a card detect signal.

So Runtime PM needs to be opt-in.  I have sent a patch.