[PATCH 0/3] OMAP: HSMMC: cleanup and runtime pm

[PATCH 0/3] OMAP: HSMMC: cleanup and runtime pm

[Balaji T K]

Removing the custom state machine - lazy disable framework in omap_hsmmc
to make way for runtime pm to handle host controller
power states.
This allows mmc_host_enable/mmc_host_disable to be replaced by
runtime get_sync and put_sync at host controller driver.

Enable runtime PM in omap_hsmmc

Balaji T K (3):
  MMC: OMAP: HSMMC: Remove lazy_disable
  MMC: OMAP: HSMMC: add runtime pm support
  OMAP2+: HSMMC: Remove unused iclk

 drivers/mmc/host/omap_hsmmc.c |  373 +++++++---------------------------------
 1 files changed, 66 insertions(+), 307 deletions(-)

[PATCH 1/3] MMC: OMAP: HSMMC: Remove lazy_disable

[Balaji T K]

lazy_disable framework in OMAP HSMMC manages multiple low power states
and Card is powered off after inactivity time of 8 seconds.
Based on previous discussion on the list Card power(regulator)
handling (when to power OFF/ON) should ideally be handled by core layer.
Remove usage of lazy disable to allow core layer _only_ to handle card power.
With the removal of lazy disable framework, MMC regulators
are left ON until MMC_POWER_OFF via set_ios.

Signed-off-by: Balaji T K <balajitk@ti.com>
---
 drivers/mmc/host/omap_hsmmc.c |  246 +----------------------------------------
 1 files changed, 2 insertions(+), 244 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 8707bcd..beb3249 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -120,11 +120,6 @@
 #define OMAP_MMC_MASTER_CLOCK	96000000
 #define DRIVER_NAME		"omap_hsmmc"
 
-/* Timeouts for entering power saving states on inactivity, msec */
-#define OMAP_MMC_DISABLED_TIMEOUT	100
-#define OMAP_MMC_SLEEP_TIMEOUT		1000
-#define OMAP_MMC_OFF_TIMEOUT		8000
-
 /*
  * One controller can have multiple slots, like on some omap boards using
  * omap.c controller driver. Luckily this is not currently done on any known
@@ -1630,8 +1625,6 @@ static void omap_hsmmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 
 	if (host->power_mode == MMC_POWER_OFF)
 		mmc_host_disable(host->mmc);
-	else
-		mmc_host_lazy_disable(host->mmc);
 }
 
 static int omap_hsmmc_get_cd(struct mmc_host *mmc)
@@ -1687,220 +1680,6 @@ static void omap_hsmmc_conf_bus_power(struct omap_hsmmc_host *host)
 	set_sd_bus_power(host);
 }
 
-/*
- * Dynamic power saving handling, FSM:
- *   ENABLED -> DISABLED -> CARDSLEEP / REGSLEEP -> OFF
- *     ^___________|          |                      |
- *     |______________________|______________________|
- *
- * ENABLED:   mmc host is fully functional
- * DISABLED:  fclk is off
- * CARDSLEEP: fclk is off, card is asleep, voltage regulator is asleep
- * REGSLEEP:  fclk is off, voltage regulator is asleep
- * OFF:       fclk is off, voltage regulator is off
- *
- * Transition handlers return the timeout for the next state transition
- * or negative error.
- */
-
-enum {ENABLED = 0, DISABLED, CARDSLEEP, REGSLEEP, OFF};
-
-/* Handler for [ENABLED -> DISABLED] transition */
-static int omap_hsmmc_enabled_to_disabled(struct omap_hsmmc_host *host)
-{
-	omap_hsmmc_context_save(host);
-	clk_disable(host->fclk);
-	host->dpm_state = DISABLED;
-
-	dev_dbg(mmc_dev(host->mmc), "ENABLED -> DISABLED\n");
-
-	if (host->power_mode == MMC_POWER_OFF)
-		return 0;
-
-	return OMAP_MMC_SLEEP_TIMEOUT;
-}
-
-/* Handler for [DISABLED -> REGSLEEP / CARDSLEEP] transition */
-static int omap_hsmmc_disabled_to_sleep(struct omap_hsmmc_host *host)
-{
-	int err, new_state;
-
-	if (!mmc_try_claim_host(host->mmc))
-		return 0;
-
-	clk_enable(host->fclk);
-	omap_hsmmc_context_restore(host);
-	if (mmc_card_can_sleep(host->mmc)) {
-		err = mmc_card_sleep(host->mmc);
-		if (err < 0) {
-			clk_disable(host->fclk);
-			mmc_release_host(host->mmc);
-			return err;
-		}
-		new_state = CARDSLEEP;
-	} else {
-		new_state = REGSLEEP;
-	}
-	if (mmc_slot(host).set_sleep)
-		mmc_slot(host).set_sleep(host->dev, host->slot_id, 1, 0,
-					 new_state == CARDSLEEP);
-	/* FIXME: turn off bus power and perhaps interrupts too */
-	clk_disable(host->fclk);
-	host->dpm_state = new_state;
-
-	mmc_release_host(host->mmc);
-
-	dev_dbg(mmc_dev(host->mmc), "DISABLED -> %s\n",
-		host->dpm_state == CARDSLEEP ? "CARDSLEEP" : "REGSLEEP");
-
-	if (mmc_slot(host).no_off)
-		return 0;
-
-	if ((host->mmc->caps & MMC_CAP_NONREMOVABLE) ||
-	    mmc_slot(host).card_detect ||
-	    (mmc_slot(host).get_cover_state &&
-	     mmc_slot(host).get_cover_state(host->dev, host->slot_id)))
-		return OMAP_MMC_OFF_TIMEOUT;
-
-	return 0;
-}
-
-/* Handler for [REGSLEEP / CARDSLEEP -> OFF] transition */
-static int omap_hsmmc_sleep_to_off(struct omap_hsmmc_host *host)
-{
-	if (!mmc_try_claim_host(host->mmc))
-		return 0;
-
-	if (mmc_slot(host).no_off)
-		return 0;
-
-	if (!((host->mmc->caps & MMC_CAP_NONREMOVABLE) ||
-	      mmc_slot(host).card_detect ||
-	      (mmc_slot(host).get_cover_state &&
-	       mmc_slot(host).get_cover_state(host->dev, host->slot_id)))) {
-		mmc_release_host(host->mmc);
-		return 0;
-	}
-
-	mmc_slot(host).set_power(host->dev, host->slot_id, 0, 0);
-	host->vdd = 0;
-	host->power_mode = MMC_POWER_OFF;
-
-	dev_dbg(mmc_dev(host->mmc), "%s -> OFF\n",
-		host->dpm_state == CARDSLEEP ? "CARDSLEEP" : "REGSLEEP");
-
-	host->dpm_state = OFF;
-
-	mmc_release_host(host->mmc);
-
-	return 0;
-}
-
-/* Handler for [DISABLED -> ENABLED] transition */
-static int omap_hsmmc_disabled_to_enabled(struct omap_hsmmc_host *host)
-{
-	int err;
-
-	err = clk_enable(host->fclk);
-	if (err < 0)
-		return err;
-
-	omap_hsmmc_context_restore(host);
-	host->dpm_state = ENABLED;
-
-	dev_dbg(mmc_dev(host->mmc), "DISABLED -> ENABLED\n");
-
-	return 0;
-}
-
-/* Handler for [SLEEP -> ENABLED] transition */
-static int omap_hsmmc_sleep_to_enabled(struct omap_hsmmc_host *host)
-{
-	if (!mmc_try_claim_host(host->mmc))
-		return 0;
-
-	clk_enable(host->fclk);
-	omap_hsmmc_context_restore(host);
-	if (mmc_slot(host).set_sleep)
-		mmc_slot(host).set_sleep(host->dev, host->slot_id, 0,
-			 host->vdd, host->dpm_state == CARDSLEEP);
-	if (mmc_card_can_sleep(host->mmc))
-		mmc_card_awake(host->mmc);
-
-	dev_dbg(mmc_dev(host->mmc), "%s -> ENABLED\n",
-		host->dpm_state == CARDSLEEP ? "CARDSLEEP" : "REGSLEEP");
-
-	host->dpm_state = ENABLED;
-
-	mmc_release_host(host->mmc);
-
-	return 0;
-}
-
-/* Handler for [OFF -> ENABLED] transition */
-static int omap_hsmmc_off_to_enabled(struct omap_hsmmc_host *host)
-{
-	clk_enable(host->fclk);
-
-	omap_hsmmc_context_restore(host);
-	omap_hsmmc_conf_bus_power(host);
-	mmc_power_restore_host(host->mmc);
-
-	host->dpm_state = ENABLED;
-
-	dev_dbg(mmc_dev(host->mmc), "OFF -> ENABLED\n");
-
-	return 0;
-}
-
-/*
- * Bring MMC host to ENABLED from any other PM state.
- */
-static int omap_hsmmc_enable(struct mmc_host *mmc)
-{
-	struct omap_hsmmc_host *host = mmc_priv(mmc);
-
-	switch (host->dpm_state) {
-	case DISABLED:
-		return omap_hsmmc_disabled_to_enabled(host);
-	case CARDSLEEP:
-	case REGSLEEP:
-		return omap_hsmmc_sleep_to_enabled(host);
-	case OFF:
-		return omap_hsmmc_off_to_enabled(host);
-	default:
-		dev_dbg(mmc_dev(host->mmc), "UNKNOWN state\n");
-		return -EINVAL;
-	}
-}
-
-/*
- * Bring MMC host in PM state (one level deeper).
- */
-static int omap_hsmmc_disable(struct mmc_host *mmc, int lazy)
-{
-	struct omap_hsmmc_host *host = mmc_priv(mmc);
-
-	switch (host->dpm_state) {
-	case ENABLED: {
-		int delay;
-
-		delay = omap_hsmmc_enabled_to_disabled(host);
-		if (lazy || delay < 0)
-			return delay;
-		return 0;
-	}
-	case DISABLED:
-		return omap_hsmmc_disabled_to_sleep(host);
-	case CARDSLEEP:
-	case REGSLEEP:
-		return omap_hsmmc_sleep_to_off(host);
-	default:
-		dev_dbg(mmc_dev(host->mmc), "UNKNOWN state\n");
-		return -EINVAL;
-	}
-}
-
 static int omap_hsmmc_enable_fclk(struct mmc_host *mmc)
 {
 	struct omap_hsmmc_host *host = mmc_priv(mmc);
@@ -1935,17 +1714,6 @@ static const struct mmc_host_ops omap_hsmmc_ops = {
 	/* NYET -- enable_sdio_irq */
 };
 
-static const struct mmc_host_ops omap_hsmmc_ps_ops = {
-	.enable = omap_hsmmc_enable,
-	.disable = omap_hsmmc_disable,
-	.request = omap_hsmmc_request,
-	.set_ios = omap_hsmmc_set_ios,
-	.get_cd = omap_hsmmc_get_cd,
-	.get_ro = omap_hsmmc_get_ro,
-	.init_card = omap_hsmmc_init_card,
-	/* NYET -- enable_sdio_irq */
-};
-
 #ifdef CONFIG_DEBUG_FS
 
 static int omap_hsmmc_regs_show(struct seq_file *s, void *data)
@@ -1967,7 +1735,7 @@ static int omap_hsmmc_regs_show(struct seq_file *s, void *data)
 			host->dpm_state, mmc->nesting_cnt,
 			host->context_loss, context_loss);
 
-	if (host->suspended || host->dpm_state == OFF) {
+	if (host->suspended) {
 		seq_printf(s, "host suspended, can't read registers\n");
 		return 0;
 	}
@@ -2080,10 +1848,7 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, host);
 	INIT_WORK(&host->mmc_carddetect_work, omap_hsmmc_detect);
 
-	if (mmc_slot(host).power_saving)
-		mmc->ops	= &omap_hsmmc_ps_ops;
-	else
-		mmc->ops	= &omap_hsmmc_ops;
+	mmc->ops	= &omap_hsmmc_ops;
 
 	/*
 	 * If regulator_disable can only put vcc_aux to sleep then there is
@@ -2114,9 +1879,6 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev)
 	omap_hsmmc_context_save(host);
 
 	mmc->caps |= MMC_CAP_DISABLE;
-	mmc_set_disable_delay(mmc, OMAP_MMC_DISABLED_TIMEOUT);
-	/* we start off in DISABLED state */
-	host->dpm_state = DISABLED;
 
 	if (clk_enable(host->iclk) != 0) {
 		clk_put(host->iclk);
@@ -2239,8 +2001,6 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev)
 
 	omap_hsmmc_disable_irq(host);
 
-	mmc_host_lazy_disable(host->mmc);
-
 	omap_hsmmc_protect_card(host);
 
 	mmc_add_host(mmc);
@@ -2419,8 +2179,6 @@ static int omap_hsmmc_resume(struct device *dev)
 		ret = mmc_resume_host(host->mmc);
 		if (ret == 0)
 			host->suspended = 0;
-
-		mmc_host_lazy_disable(host->mmc);
 	}
 
 	return ret;
-- 
1.7.0.4

Re: [PATCH 1/3] MMC: OMAP: HSMMC: Remove lazy_disable

[Kevin Hilman]

Balaji T K <balajitk@ti.com> writes:

> lazy_disable framework in OMAP HSMMC manages multiple low power states
> and Card is powered off after inactivity time of 8 seconds.
> Based on previous discussion on the list Card power(regulator)

needs some punctuation for readability.

> handling (when to power OFF/ON) should ideally be handled by core layer.

So are you working on the core layer replacement for this?  If not, is
there a plan or owner for this?

> Remove usage of lazy disable to allow core layer _only_ to handle card power.
> With the removal of lazy disable framework, MMC regulators
> are left ON until MMC_POWER_OFF via set_ios.
>
> Signed-off-by: Balaji T K <balajitk@ti.com>

[...]

> @@ -2080,10 +1848,7 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, host);
>  	INIT_WORK(&host->mmc_carddetect_work, omap_hsmmc_detect);
>  
> -	if (mmc_slot(host).power_saving)
> -		mmc->ops	= &omap_hsmmc_ps_ops;
> -	else
> -		mmc->ops	= &omap_hsmmc_ops;
> +	mmc->ops	= &omap_hsmmc_ops;

I guess the _ps_ops struct is now completely unused so should probably
be removed in this patch also.

Kevin

Re: [PATCH 1/3] MMC: OMAP: HSMMC: Remove lazy_disable

[T Krishnamoorthy, Balaji]

On Wed, Jun 22, 2011 at 11:56 PM, Kevin Hilman <khilman@ti.com> wrote:
> Balaji T K <balajitk@ti.com> writes:
>
>> lazy_disable framework in OMAP HSMMC manages multiple low power states
>> and Card is powered off after inactivity time of 8 seconds.
>> Based on previous discussion on the list Card power(regulator)
>
> needs some punctuation for readability.

OK

>
>> handling (when to power OFF/ON) should ideally be handled by core layer.
>
> So are you working on the core layer replacement for this?  If not, is
> there a plan or owner for this?

Venkat and myself are planing to start on this for MMC and SD.
Some partial work has already been done for SDIO,
need to consolidate this for all three of them.

>
>> Remove usage of lazy disable to allow core layer _only_ to handle card power.
>> With the removal of lazy disable framework, MMC regulators
>> are left ON until MMC_POWER_OFF via set_ios.
>>
>> Signed-off-by: Balaji T K <balajitk@ti.com>
>
> [...]
>
>> @@ -2080,10 +1848,7 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev)
>>       platform_set_drvdata(pdev, host);
>>       INIT_WORK(&host->mmc_carddetect_work, omap_hsmmc_detect);
>>
>> -     if (mmc_slot(host).power_saving)
>> -             mmc->ops        = &omap_hsmmc_ps_ops;
>> -     else
>> -             mmc->ops        = &omap_hsmmc_ops;
>> +     mmc->ops        = &omap_hsmmc_ops;
>
> I guess the _ps_ops struct is now completely unused so should probably
> be removed in this patch also.

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

[PATCH 2/3] MMC: OMAP: HSMMC: add runtime pm support

[Balaji T K]

add runtime pm support to HSMMC host controller
Use runtime pm API to enable/disable HSMMC clock
Use runtime autosuspend APIs to enable auto suspend delay

Based on OMAP HSMMC runtime implementation by Kevin Hilman, Kishore Kadiyala

Signed-off-by: Balaji T K <balajitk@ti.com>
---
 drivers/mmc/host/omap_hsmmc.c |  127 ++++++++++++++++++++++-------------------
 1 files changed, 69 insertions(+), 58 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index beb3249..5b81d8b 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -33,6 +33,7 @@
 #include <linux/semaphore.h>
 #include <linux/gpio.h>
 #include <linux/regulator/consumer.h>
+#include <linux/pm_runtime.h>
 #include <plat/dma.h>
 #include <mach/hardware.h>
 #include <plat/board.h>
@@ -116,6 +117,7 @@
 #define OMAP_MMC4_DEVID		3
 #define OMAP_MMC5_DEVID		4
 
+#define MMC_AUTOSUSPEND_DELAY	50
 #define MMC_TIMEOUT_MS		20
 #define OMAP_MMC_MASTER_CLOCK	96000000
 #define DRIVER_NAME		"omap_hsmmc"
@@ -1149,8 +1151,7 @@ static int omap_hsmmc_switch_opcond(struct omap_hsmmc_host *host, int vdd)
 	int ret;
 
 	/* Disable the clocks */
-	clk_disable(host->fclk);
-	clk_disable(host->iclk);
+	pm_runtime_put_sync(host->dev);
 	if (host->got_dbclk)
 		clk_disable(host->dbclk);
 
@@ -1161,8 +1162,7 @@ static int omap_hsmmc_switch_opcond(struct omap_hsmmc_host *host, int vdd)
 	if (!ret)
 		ret = mmc_slot(host).set_power(host->dev, host->slot_id, 1,
 					       vdd);
-	clk_enable(host->iclk);
-	clk_enable(host->fclk);
+	pm_runtime_get_sync(host->dev);
 	if (host->got_dbclk)
 		clk_enable(host->dbclk);
 
@@ -1528,7 +1528,7 @@ static void omap_hsmmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	u32 con;
 	int do_send_init_stream = 0;
 
-	mmc_host_enable(host->mmc);
+	pm_runtime_get_sync(host->dev);
 
 	if (ios->power_mode != host->power_mode) {
 		switch (ios->power_mode) {
@@ -1623,8 +1623,7 @@ static void omap_hsmmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	else
 		OMAP_HSMMC_WRITE(host->base, CON, con & ~OD);
 
-	if (host->power_mode == MMC_POWER_OFF)
-		mmc_host_disable(host->mmc);
+	pm_runtime_put_autosuspend(host->dev);
 }
 
 static int omap_hsmmc_get_cd(struct mmc_host *mmc)
@@ -1680,32 +1679,28 @@ static void omap_hsmmc_conf_bus_power(struct omap_hsmmc_host *host)
 	set_sd_bus_power(host);
 }
 
-static int omap_hsmmc_enable_fclk(struct mmc_host *mmc)
+static int omap_hsmmc_enable(struct mmc_host *mmc)
 {
 	struct omap_hsmmc_host *host = mmc_priv(mmc);
-	int err;
 
-	err = clk_enable(host->fclk);
-	if (err)
-		return err;
-	dev_dbg(mmc_dev(host->mmc), "mmc_fclk: enabled\n");
-	omap_hsmmc_context_restore(host);
+	pm_runtime_get_sync(host->dev);
+
 	return 0;
 }
 
-static int omap_hsmmc_disable_fclk(struct mmc_host *mmc, int lazy)
+static int omap_hsmmc_disable(struct mmc_host *mmc, int lazy)
 {
 	struct omap_hsmmc_host *host = mmc_priv(mmc);
 
-	omap_hsmmc_context_save(host);
-	clk_disable(host->fclk);
-	dev_dbg(mmc_dev(host->mmc), "mmc_fclk: disabled\n");
+	pm_runtime_mark_last_busy(host->dev);
+	pm_runtime_put_autosuspend(host->dev);
+
 	return 0;
 }
 
 static const struct mmc_host_ops omap_hsmmc_ops = {
-	.enable = omap_hsmmc_enable_fclk,
-	.disable = omap_hsmmc_disable_fclk,
+	.enable = omap_hsmmc_enable,
+	.disable = omap_hsmmc_disable,
 	.request = omap_hsmmc_request,
 	.set_ios = omap_hsmmc_set_ios,
 	.get_cd = omap_hsmmc_get_cd,
@@ -1740,10 +1735,7 @@ static int omap_hsmmc_regs_show(struct seq_file *s, void *data)
 		return 0;
 	}
 
-	if (clk_enable(host->fclk) != 0) {
-		seq_printf(s, "can't read the regs\n");
-		return 0;
-	}
+	pm_runtime_get_sync(host->dev);
 
 	seq_printf(s, "SYSCONFIG:\t0x%08x\n",
 			OMAP_HSMMC_READ(host->base, SYSCONFIG));
@@ -1760,7 +1752,8 @@ static int omap_hsmmc_regs_show(struct seq_file *s, void *data)
 	seq_printf(s, "CAPA:\t\t0x%08x\n",
 			OMAP_HSMMC_READ(host->base, CAPA));
 
-	clk_disable(host->fclk);
+	pm_runtime_mark_last_busy(host->dev);
+	pm_runtime_put_autosuspend(host->dev);
 
 	return 0;
 }
@@ -1880,18 +1873,12 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev)
 
 	mmc->caps |= MMC_CAP_DISABLE;
 
-	if (clk_enable(host->iclk) != 0) {
-		clk_put(host->iclk);
-		clk_put(host->fclk);
-		goto err1;
-	}
-
-	if (mmc_host_enable(host->mmc) != 0) {
-		clk_disable(host->iclk);
-		clk_put(host->iclk);
-		clk_put(host->fclk);
-		goto err1;
-	}
+	pm_runtime_enable(host->dev);
+	pm_runtime_allow(host->dev);
+	pm_runtime_get_sync(host->dev);
+	pm_runtime_set_autosuspend_delay(host->dev, MMC_AUTOSUSPEND_DELAY);
+	pm_runtime_use_autosuspend(host->dev);
+	pm_suspend_ignore_children(host->dev, 1);
 
 	if (cpu_is_omap2430()) {
 		host->dbclk = clk_get(&pdev->dev, "mmchsdb_fck");
@@ -2018,6 +2005,8 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev)
 	}
 
 	omap_hsmmc_debugfs(mmc);
+	pm_runtime_mark_last_busy(host->dev);
+	pm_runtime_put_autosuspend(host->dev);
 
 	return 0;
 
@@ -2033,8 +2022,8 @@ err_reg:
 err_irq_cd_init:
 	free_irq(host->irq, host);
 err_irq:
-	mmc_host_disable(host->mmc);
-	clk_disable(host->iclk);
+	pm_runtime_mark_last_busy(host->dev);
+	pm_runtime_put_autosuspend(host->dev);
 	clk_put(host->fclk);
 	clk_put(host->iclk);
 	if (host->got_dbclk) {
@@ -2058,7 +2047,7 @@ static int omap_hsmmc_remove(struct platform_device *pdev)
 	struct resource *res;
 
 	if (host) {
-		mmc_host_enable(host->mmc);
+		pm_runtime_get_sync(host->dev);
 		mmc_remove_host(host->mmc);
 		if (host->use_reg)
 			omap_hsmmc_reg_put(host);
@@ -2069,8 +2058,9 @@ static int omap_hsmmc_remove(struct platform_device *pdev)
 			free_irq(mmc_slot(host).card_detect_irq, host);
 		flush_work_sync(&host->mmc_carddetect_work);
 
-		mmc_host_disable(host->mmc);
-		clk_disable(host->iclk);
+		pm_runtime_put_sync(host->dev);
+		pm_runtime_forbid(host->dev);
+		pm_runtime_disable(host->dev);
 		clk_put(host->fclk);
 		clk_put(host->iclk);
 		if (host->got_dbclk) {
@@ -2102,6 +2092,8 @@ static int omap_hsmmc_suspend(struct device *dev)
 		return 0;
 
 	if (host) {
+		/* FIXME: TODO move get_sync to proper dev_pm_ops function */
+		pm_runtime_get_sync(host->dev);
 		host->suspended = 1;
 		if (host->pdata->suspend) {
 			ret = host->pdata->suspend(&pdev->dev,
@@ -2116,13 +2108,11 @@ static int omap_hsmmc_suspend(struct device *dev)
 		}
 		cancel_work_sync(&host->mmc_carddetect_work);
 		ret = mmc_suspend_host(host->mmc);
-		mmc_host_enable(host->mmc);
+
 		if (ret == 0) {
 			omap_hsmmc_disable_irq(host);
 			OMAP_HSMMC_WRITE(host->base, HCTL,
 				OMAP_HSMMC_READ(host->base, HCTL) & ~SDBP);
-			mmc_host_disable(host->mmc);
-			clk_disable(host->iclk);
 			if (host->got_dbclk)
 				clk_disable(host->dbclk);
 		} else {
@@ -2134,8 +2124,9 @@ static int omap_hsmmc_suspend(struct device *dev)
 					dev_dbg(mmc_dev(host->mmc),
 						"Unmask interrupt failed\n");
 			}
-			mmc_host_disable(host->mmc);
 		}
+		/* FIXME: TODO move put_sync to proper dev_pm_ops function */
+		pm_runtime_put_sync(host->dev);
 
 	}
 	return ret;
@@ -2152,14 +2143,8 @@ static int omap_hsmmc_resume(struct device *dev)
 		return 0;
 
 	if (host) {
-		ret = clk_enable(host->iclk);
-		if (ret)
-			goto clk_en_err;
-
-		if (mmc_host_enable(host->mmc) != 0) {
-			clk_disable(host->iclk);
-			goto clk_en_err;
-		}
+		/* FIXME: TODO move put_sync to proper dev_pm_ops function */
+		pm_runtime_get_sync(host->dev);
 
 		if (host->got_dbclk)
 			clk_enable(host->dbclk);
@@ -2179,14 +2164,14 @@ static int omap_hsmmc_resume(struct device *dev)
 		ret = mmc_resume_host(host->mmc);
 		if (ret == 0)
 			host->suspended = 0;
+
+		/* FIXME: TODO move put_sync to proper dev_pm_ops function */
+		pm_runtime_mark_last_busy(host->dev);
+		pm_runtime_put_autosuspend(host->dev);
 	}
 
 	return ret;
 
-clk_en_err:
-	dev_dbg(mmc_dev(host->mmc),
-		"Failed to enable MMC clocks during resume\n");
-	return ret;
 }
 
 #else
@@ -2194,9 +2179,35 @@ clk_en_err:
 #define omap_hsmmc_resume		NULL
 #endif
 
+static int omap_hsmmc_runtime_suspend(struct device *dev)
+{
+	struct omap_hsmmc_host *host;
+
+
+	host = platform_get_drvdata(to_platform_device(dev));
+	omap_hsmmc_context_save(host);
+	dev_dbg(mmc_dev(host->mmc), "disabled\n");
+
+	return 0;
+}
+
+static int omap_hsmmc_runtime_resume(struct device *dev)
+{
+	struct omap_hsmmc_host *host;
+
+
+	host = platform_get_drvdata(to_platform_device(dev));
+	omap_hsmmc_context_restore(host);
+	dev_dbg(mmc_dev(host->mmc), "host: enabled\n");
+
+	return 0;
+}
+
 static struct dev_pm_ops omap_hsmmc_dev_pm_ops = {
 	.suspend	= omap_hsmmc_suspend,
 	.resume		= omap_hsmmc_resume,
+	.runtime_suspend = omap_hsmmc_runtime_suspend,
+	.runtime_resume = omap_hsmmc_runtime_resume,
 };
 
 static struct platform_driver omap_hsmmc_driver = {
-- 
1.7.0.4

Re: [PATCH 2/3] MMC: OMAP: HSMMC: add runtime pm support

[Kevin Hilman]

Balaji T K <balajitk@ti.com> writes:

> add runtime pm support to HSMMC host controller
> Use runtime pm API to enable/disable HSMMC clock
> Use runtime autosuspend APIs to enable auto suspend delay
>
> Based on OMAP HSMMC runtime implementation by Kevin Hilman, Kishore Kadiyala
>
> Signed-off-by: Balaji T K <balajitk@ti.com>
> ---
>  drivers/mmc/host/omap_hsmmc.c |  127 ++++++++++++++++++++++-------------------
>  1 files changed, 69 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index beb3249..5b81d8b 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -33,6 +33,7 @@
>  #include <linux/semaphore.h>
>  #include <linux/gpio.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/pm_runtime.h>
>  #include <plat/dma.h>
>  #include <mach/hardware.h>
>  #include <plat/board.h>
> @@ -116,6 +117,7 @@
>  #define OMAP_MMC4_DEVID		3
>  #define OMAP_MMC5_DEVID		4
>  
> +#define MMC_AUTOSUSPEND_DELAY	50
>  #define MMC_TIMEOUT_MS		20
>  #define OMAP_MMC_MASTER_CLOCK	96000000
>  #define DRIVER_NAME		"omap_hsmmc"
> @@ -1149,8 +1151,7 @@ static int omap_hsmmc_switch_opcond(struct omap_hsmmc_host *host, int vdd)
>  	int ret;
>  
>  	/* Disable the clocks */
> -	clk_disable(host->fclk);
> -	clk_disable(host->iclk);
> +	pm_runtime_put_sync(host->dev);
>  	if (host->got_dbclk)
>  		clk_disable(host->dbclk);
>  
> @@ -1161,8 +1162,7 @@ static int omap_hsmmc_switch_opcond(struct omap_hsmmc_host *host, int vdd)
>  	if (!ret)
>  		ret = mmc_slot(host).set_power(host->dev, host->slot_id, 1,
>  					       vdd);
> -	clk_enable(host->iclk);
> -	clk_enable(host->fclk);
> +	pm_runtime_get_sync(host->dev);
>  	if (host->got_dbclk)
>  		clk_enable(host->dbclk);
>  
> @@ -1528,7 +1528,7 @@ static void omap_hsmmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  	u32 con;
>  	int do_send_init_stream = 0;
>  
> -	mmc_host_enable(host->mmc);
> +	pm_runtime_get_sync(host->dev);
>  
>  	if (ios->power_mode != host->power_mode) {
>  		switch (ios->power_mode) {
> @@ -1623,8 +1623,7 @@ static void omap_hsmmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  	else
>  		OMAP_HSMMC_WRITE(host->base, CON, con & ~OD);
>  
> -	if (host->power_mode == MMC_POWER_OFF)
> -		mmc_host_disable(host->mmc);
> +	pm_runtime_put_autosuspend(host->dev);
>  }
>  
>  static int omap_hsmmc_get_cd(struct mmc_host *mmc)
> @@ -1680,32 +1679,28 @@ static void omap_hsmmc_conf_bus_power(struct omap_hsmmc_host *host)
>  	set_sd_bus_power(host);
>  }
>  
> -static int omap_hsmmc_enable_fclk(struct mmc_host *mmc)
> +static int omap_hsmmc_enable(struct mmc_host *mmc)
>  {
>  	struct omap_hsmmc_host *host = mmc_priv(mmc);
> -	int err;
>  
> -	err = clk_enable(host->fclk);
> -	if (err)
> -		return err;
> -	dev_dbg(mmc_dev(host->mmc), "mmc_fclk: enabled\n");
> -	omap_hsmmc_context_restore(host);
> +	pm_runtime_get_sync(host->dev);
> +
>  	return 0;
>  }
>  
> -static int omap_hsmmc_disable_fclk(struct mmc_host *mmc, int lazy)
> +static int omap_hsmmc_disable(struct mmc_host *mmc, int lazy)
>  {
>  	struct omap_hsmmc_host *host = mmc_priv(mmc);
>  
> -	omap_hsmmc_context_save(host);
> -	clk_disable(host->fclk);
> -	dev_dbg(mmc_dev(host->mmc), "mmc_fclk: disabled\n");
> +	pm_runtime_mark_last_busy(host->dev);
> +	pm_runtime_put_autosuspend(host->dev);
> +
>  	return 0;
>  }
>  
>  static const struct mmc_host_ops omap_hsmmc_ops = {
> -	.enable = omap_hsmmc_enable_fclk,
> -	.disable = omap_hsmmc_disable_fclk,
> +	.enable = omap_hsmmc_enable,
> +	.disable = omap_hsmmc_disable,
>  	.request = omap_hsmmc_request,
>  	.set_ios = omap_hsmmc_set_ios,
>  	.get_cd = omap_hsmmc_get_cd,
> @@ -1740,10 +1735,7 @@ static int omap_hsmmc_regs_show(struct seq_file *s, void *data)
>  		return 0;
>  	}
>  
> -	if (clk_enable(host->fclk) != 0) {
> -		seq_printf(s, "can't read the regs\n");
> -		return 0;
> -	}
> +	pm_runtime_get_sync(host->dev);
>  
>  	seq_printf(s, "SYSCONFIG:\t0x%08x\n",
>  			OMAP_HSMMC_READ(host->base, SYSCONFIG));
> @@ -1760,7 +1752,8 @@ static int omap_hsmmc_regs_show(struct seq_file *s, void *data)
>  	seq_printf(s, "CAPA:\t\t0x%08x\n",
>  			OMAP_HSMMC_READ(host->base, CAPA));
>  
> -	clk_disable(host->fclk);
> +	pm_runtime_mark_last_busy(host->dev);
> +	pm_runtime_put_autosuspend(host->dev);
>  
>  	return 0;
>  }
> @@ -1880,18 +1873,12 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev)
>  
>  	mmc->caps |= MMC_CAP_DISABLE;
>  
> -	if (clk_enable(host->iclk) != 0) {
> -		clk_put(host->iclk);
> -		clk_put(host->fclk);
> -		goto err1;
> -	}
> -
> -	if (mmc_host_enable(host->mmc) != 0) {
> -		clk_disable(host->iclk);
> -		clk_put(host->iclk);
> -		clk_put(host->fclk);
> -		goto err1;
> -	}
> +	pm_runtime_enable(host->dev);
> +	pm_runtime_allow(host->dev);
> +	pm_runtime_get_sync(host->dev);
> +	pm_runtime_set_autosuspend_delay(host->dev, MMC_AUTOSUSPEND_DELAY);
> +	pm_runtime_use_autosuspend(host->dev);
> +	pm_suspend_ignore_children(host->dev, 1);

Why is ignore_children needed for this device?   Is this device the
parent of other devices?   If it is, why should it ignore it's
children?

>  	if (cpu_is_omap2430()) {
>  		host->dbclk = clk_get(&pdev->dev, "mmchsdb_fck");
> @@ -2018,6 +2005,8 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev)
>  	}
>  
>  	omap_hsmmc_debugfs(mmc);
> +	pm_runtime_mark_last_busy(host->dev);
> +	pm_runtime_put_autosuspend(host->dev);
>  
>  	return 0;
>  
> @@ -2033,8 +2022,8 @@ err_reg:
>  err_irq_cd_init:
>  	free_irq(host->irq, host);
>  err_irq:
> -	mmc_host_disable(host->mmc);
> -	clk_disable(host->iclk);
> +	pm_runtime_mark_last_busy(host->dev);
> +	pm_runtime_put_autosuspend(host->dev);
>  	clk_put(host->fclk);
>  	clk_put(host->iclk);
>  	if (host->got_dbclk) {
> @@ -2058,7 +2047,7 @@ static int omap_hsmmc_remove(struct platform_device *pdev)
>  	struct resource *res;
>  
>  	if (host) {
> -		mmc_host_enable(host->mmc);
> +		pm_runtime_get_sync(host->dev);
>  		mmc_remove_host(host->mmc);
>  		if (host->use_reg)
>  			omap_hsmmc_reg_put(host);
> @@ -2069,8 +2058,9 @@ static int omap_hsmmc_remove(struct platform_device *pdev)
>  			free_irq(mmc_slot(host).card_detect_irq, host);
>  		flush_work_sync(&host->mmc_carddetect_work);
>  
> -		mmc_host_disable(host->mmc);
> -		clk_disable(host->iclk);
> +		pm_runtime_put_sync(host->dev);
> +		pm_runtime_forbid(host->dev);

Why?

> +		pm_runtime_disable(host->dev);
>  		clk_put(host->fclk);
>  		clk_put(host->iclk);
>  		if (host->got_dbclk) {
> @@ -2102,6 +2092,8 @@ static int omap_hsmmc_suspend(struct device *dev)
>  		return 0;
>  
>  	if (host) {
> +		/* FIXME: TODO move get_sync to proper dev_pm_ops function */

what does this mean?  

> +		pm_runtime_get_sync(host->dev);
>  		host->suspended = 1;
>  		if (host->pdata->suspend) {
>  			ret = host->pdata->suspend(&pdev->dev,
> @@ -2116,13 +2108,11 @@ static int omap_hsmmc_suspend(struct device *dev)
>  		}
>  		cancel_work_sync(&host->mmc_carddetect_work);
>  		ret = mmc_suspend_host(host->mmc);
> -		mmc_host_enable(host->mmc);
> +
>  		if (ret == 0) {
>  			omap_hsmmc_disable_irq(host);
>  			OMAP_HSMMC_WRITE(host->base, HCTL,
>  				OMAP_HSMMC_READ(host->base, HCTL) & ~SDBP);
> -			mmc_host_disable(host->mmc);
> -			clk_disable(host->iclk);
>  			if (host->got_dbclk)
>  				clk_disable(host->dbclk);
>  		} else {
> @@ -2134,8 +2124,9 @@ static int omap_hsmmc_suspend(struct device *dev)
>  					dev_dbg(mmc_dev(host->mmc),
>  						"Unmask interrupt failed\n");
>  			}
> -			mmc_host_disable(host->mmc);
>  		}
> +		/* FIXME: TODO move put_sync to proper dev_pm_ops function */

ditto

> +		pm_runtime_put_sync(host->dev);
>  
>  	}
>  	return ret;
> @@ -2152,14 +2143,8 @@ static int omap_hsmmc_resume(struct device *dev)
>  		return 0;
>  
>  	if (host) {
> -		ret = clk_enable(host->iclk);
> -		if (ret)
> -			goto clk_en_err;
> -
> -		if (mmc_host_enable(host->mmc) != 0) {
> -			clk_disable(host->iclk);
> -			goto clk_en_err;
> -		}
> +		/* FIXME: TODO move put_sync to proper dev_pm_ops function */

comment says put_sync, code says get_sync, but again, comment doesn't
make any sense.

> +		pm_runtime_get_sync(host->dev);
>  
>  		if (host->got_dbclk)
>  			clk_enable(host->dbclk);
> @@ -2179,14 +2164,14 @@ static int omap_hsmmc_resume(struct device *dev)
>  		ret = mmc_resume_host(host->mmc);
>  		if (ret == 0)
>  			host->suspended = 0;
> +
> +		/* FIXME: TODO move put_sync to proper dev_pm_ops function */
> +		pm_runtime_mark_last_busy(host->dev);
> +		pm_runtime_put_autosuspend(host->dev);
>  	}
>  
>  	return ret;
>  
> -clk_en_err:
> -	dev_dbg(mmc_dev(host->mmc),
> -		"Failed to enable MMC clocks during resume\n");
> -	return ret;
>  }
>  
>  #else
> @@ -2194,9 +2179,35 @@ clk_en_err:
>  #define omap_hsmmc_resume		NULL
>  #endif
>  
> +static int omap_hsmmc_runtime_suspend(struct device *dev)
> +{
> +	struct omap_hsmmc_host *host;
> +
> +

extra blank line

> +	host = platform_get_drvdata(to_platform_device(dev));
> +	omap_hsmmc_context_save(host);
> +	dev_dbg(mmc_dev(host->mmc), "disabled\n");
> +
> +	return 0;
> +}
> +
> +static int omap_hsmmc_runtime_resume(struct device *dev)
> +{
> +	struct omap_hsmmc_host *host;
> +
> +

extra blank line

> +	host = platform_get_drvdata(to_platform_device(dev));
> +	omap_hsmmc_context_restore(host);
> +	dev_dbg(mmc_dev(host->mmc), "host: enabled\n");
> +
> +	return 0;
> +}
> +
>  static struct dev_pm_ops omap_hsmmc_dev_pm_ops = {
>  	.suspend	= omap_hsmmc_suspend,
>  	.resume		= omap_hsmmc_resume,
> +	.runtime_suspend = omap_hsmmc_runtime_suspend,
> +	.runtime_resume = omap_hsmmc_runtime_resume,
>  };
>  
>  static struct platform_driver omap_hsmmc_driver = {

Kevin

Re: [PATCH 2/3] MMC: OMAP: HSMMC: add runtime pm support

[T Krishnamoorthy, Balaji]

On Thu, Jun 23, 2011 at 12:08 AM, Kevin Hilman <khilman@ti.com> wrote:
> Balaji T K <balajitk@ti.com> writes:
>

>> @@ -1880,18 +1873,12 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev)
>>
>>       mmc->caps |= MMC_CAP_DISABLE;
>>
>> -     if (clk_enable(host->iclk) != 0) {
>> -             clk_put(host->iclk);
>> -             clk_put(host->fclk);
>> -             goto err1;
>> -     }
>> -
>> -     if (mmc_host_enable(host->mmc) != 0) {
>> -             clk_disable(host->iclk);
>> -             clk_put(host->iclk);
>> -             clk_put(host->fclk);
>> -             goto err1;
>> -     }
>> +     pm_runtime_enable(host->dev);
>> +     pm_runtime_allow(host->dev);
>> +     pm_runtime_get_sync(host->dev);
>> +     pm_runtime_set_autosuspend_delay(host->dev, MMC_AUTOSUSPEND_DELAY);
>> +     pm_runtime_use_autosuspend(host->dev);
>> +     pm_suspend_ignore_children(host->dev, 1);
>
> Why is ignore_children needed for this device?   Is this device the
> parent of other devices?   If it is, why should it ignore it's
> children?
>

No, I will remove. Added it for testing only.

>>       if (cpu_is_omap2430()) {
>>               host->dbclk = clk_get(&pdev->dev, "mmchsdb_fck");
>> @@ -2018,6 +2005,8 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev)
>>       }
>>
>>       omap_hsmmc_debugfs(mmc);
>> +     pm_runtime_mark_last_busy(host->dev);
>> +     pm_runtime_put_autosuspend(host->dev);
>>
>>       return 0;
>>
>> @@ -2033,8 +2022,8 @@ err_reg:
>>  err_irq_cd_init:
>>       free_irq(host->irq, host);
>>  err_irq:
>> -     mmc_host_disable(host->mmc);
>> -     clk_disable(host->iclk);
>> +     pm_runtime_mark_last_busy(host->dev);
>> +     pm_runtime_put_autosuspend(host->dev);
>>       clk_put(host->fclk);
>>       clk_put(host->iclk);
>>       if (host->got_dbclk) {
>> @@ -2058,7 +2047,7 @@ static int omap_hsmmc_remove(struct platform_device *pdev)
>>       struct resource *res;
>>
>>       if (host) {
>> -             mmc_host_enable(host->mmc);
>> +             pm_runtime_get_sync(host->dev);
>>               mmc_remove_host(host->mmc);
>>               if (host->use_reg)
>>                       omap_hsmmc_reg_put(host);
>> @@ -2069,8 +2058,9 @@ static int omap_hsmmc_remove(struct platform_device *pdev)
>>                       free_irq(mmc_slot(host).card_detect_irq, host);
>>               flush_work_sync(&host->mmc_carddetect_work);
>>
>> -             mmc_host_disable(host->mmc);
>> -             clk_disable(host->iclk);
>> +             pm_runtime_put_sync(host->dev);
>> +             pm_runtime_forbid(host->dev);
>
> Why?
>

Added for balancing pm_runtime_allow added in  _probe.
But forbid also resume the device on remove.
Should this be removed, keeping _allow in _probe ?

>> +             pm_runtime_disable(host->dev);
>>               clk_put(host->fclk);
>>               clk_put(host->iclk);
>>               if (host->got_dbclk) {
>> @@ -2102,6 +2092,8 @@ static int omap_hsmmc_suspend(struct device *dev)
>>               return 0;
>>
>>       if (host) {
>> +             /* FIXME: TODO move get_sync to proper dev_pm_ops function */
>
> what does this mean?

get_sync is needed to enable clock before accessing the registers but
the discusssion @
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg50819.html
suggested to move runtime get_sync calls to .prepare
Haven't tried it yet.

>
>> +             pm_runtime_get_sync(host->dev);
>>               host->suspended = 1;
>>               if (host->pdata->suspend) {
>>                       ret = host->pdata->suspend(&pdev->dev,
>> @@ -2116,13 +2108,11 @@ static int omap_hsmmc_suspend(struct device *dev)
>>               }
>>               cancel_work_sync(&host->mmc_carddetect_work);
>>               ret = mmc_suspend_host(host->mmc);
>> -             mmc_host_enable(host->mmc);
>> +
>>               if (ret == 0) {
>>                       omap_hsmmc_disable_irq(host);
>>                       OMAP_HSMMC_WRITE(host->base, HCTL,
>>                               OMAP_HSMMC_READ(host->base, HCTL) & ~SDBP);
>> -                     mmc_host_disable(host->mmc);
>> -                     clk_disable(host->iclk);
>>                       if (host->got_dbclk)
>>                               clk_disable(host->dbclk);
>>               } else {
>> @@ -2134,8 +2124,9 @@ static int omap_hsmmc_suspend(struct device *dev)
>>                                       dev_dbg(mmc_dev(host->mmc),
>>                                               "Unmask interrupt failed\n");
>>                       }
>> -                     mmc_host_disable(host->mmc);
>>               }
>> +             /* FIXME: TODO move put_sync to proper dev_pm_ops function */
>
> ditto
>
>> +             pm_runtime_put_sync(host->dev);
>>
>>       }
>>       return ret;
>> @@ -2152,14 +2143,8 @@ static int omap_hsmmc_resume(struct device *dev)
>>               return 0;
>>
>>       if (host) {
>> -             ret = clk_enable(host->iclk);
>> -             if (ret)
>> -                     goto clk_en_err;
>> -
>> -             if (mmc_host_enable(host->mmc) != 0) {
>> -                     clk_disable(host->iclk);
>> -                     goto clk_en_err;
>> -             }
>> +             /* FIXME: TODO move put_sync to proper dev_pm_ops function */
>
> comment says put_sync, code says get_sync, but again, comment doesn't
> make any sense.
>
>> +             pm_runtime_get_sync(host->dev);
>>
>>               if (host->got_dbclk)
>>                       clk_enable(host->dbclk);
>> @@ -2179,14 +2164,14 @@ static int omap_hsmmc_resume(struct device *dev)
>>               ret = mmc_resume_host(host->mmc);
>>               if (ret == 0)
>>                       host->suspended = 0;
>> +
>> +             /* FIXME: TODO move put_sync to proper dev_pm_ops function */
>> +             pm_runtime_mark_last_busy(host->dev);
>> +             pm_runtime_put_autosuspend(host->dev);
>>       }
>>
>>       return ret;
>>
>> -clk_en_err:
>> -     dev_dbg(mmc_dev(host->mmc),
>> -             "Failed to enable MMC clocks during resume\n");
>> -     return ret;
>>  }
>>
>>  #else
>> @@ -2194,9 +2179,35 @@ clk_en_err:
>>  #define omap_hsmmc_resume            NULL
>>  #endif
>>
>> +static int omap_hsmmc_runtime_suspend(struct device *dev)
>> +{
>> +     struct omap_hsmmc_host *host;
>> +
>> +
>
> extra blank line

Removed

>
>> +     host = platform_get_drvdata(to_platform_device(dev));
>> +     omap_hsmmc_context_save(host);
>> +     dev_dbg(mmc_dev(host->mmc), "disabled\n");
>> +
>> +     return 0;
>> +}
>> +
>> +static int omap_hsmmc_runtime_resume(struct device *dev)
>> +{
>> +     struct omap_hsmmc_host *host;
>> +
>> +
>
> extra blank line

Removed

Re: [PATCH 2/3] MMC: OMAP: HSMMC: add runtime pm support

[Kevin Hilman]

"T Krishnamoorthy, Balaji" <balajitk@ti.com> writes:

> On Thu, Jun 23, 2011 at 12:08 AM, Kevin Hilman <khilman@ti.com> wrote:
>> Balaji T K <balajitk@ti.com> writes:
>>
>
>>> @@ -1880,18 +1873,12 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev)
>>>
>>>       mmc->caps |= MMC_CAP_DISABLE;
>>>
>>> -     if (clk_enable(host->iclk) != 0) {
>>> -             clk_put(host->iclk);
>>> -             clk_put(host->fclk);
>>> -             goto err1;
>>> -     }
>>> -
>>> -     if (mmc_host_enable(host->mmc) != 0) {
>>> -             clk_disable(host->iclk);
>>> -             clk_put(host->iclk);
>>> -             clk_put(host->fclk);
>>> -             goto err1;
>>> -     }
>>> +     pm_runtime_enable(host->dev);
>>> +     pm_runtime_allow(host->dev);
>>> +     pm_runtime_get_sync(host->dev);
>>> +     pm_runtime_set_autosuspend_delay(host->dev, MMC_AUTOSUSPEND_DELAY);
>>> +     pm_runtime_use_autosuspend(host->dev);
>>> +     pm_suspend_ignore_children(host->dev, 1);
>>
>> Why is ignore_children needed for this device?   Is this device the
>> parent of other devices?   If it is, why should it ignore it's
>> children?
>>
>
> No, I will remove. Added it for testing only.
>
>>>       if (cpu_is_omap2430()) {
>>>               host->dbclk = clk_get(&pdev->dev, "mmchsdb_fck");
>>> @@ -2018,6 +2005,8 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev)
>>>       }
>>>
>>>       omap_hsmmc_debugfs(mmc);
>>> +     pm_runtime_mark_last_busy(host->dev);
>>> +     pm_runtime_put_autosuspend(host->dev);
>>>
>>>       return 0;
>>>
>>> @@ -2033,8 +2022,8 @@ err_reg:
>>>  err_irq_cd_init:
>>>       free_irq(host->irq, host);
>>>  err_irq:
>>> -     mmc_host_disable(host->mmc);
>>> -     clk_disable(host->iclk);
>>> +     pm_runtime_mark_last_busy(host->dev);
>>> +     pm_runtime_put_autosuspend(host->dev);
>>>       clk_put(host->fclk);
>>>       clk_put(host->iclk);
>>>       if (host->got_dbclk) {
>>> @@ -2058,7 +2047,7 @@ static int omap_hsmmc_remove(struct platform_device *pdev)
>>>       struct resource *res;
>>>
>>>       if (host) {
>>> -             mmc_host_enable(host->mmc);
>>> +             pm_runtime_get_sync(host->dev);
>>>               mmc_remove_host(host->mmc);
>>>               if (host->use_reg)
>>>                       omap_hsmmc_reg_put(host);
>>> @@ -2069,8 +2058,9 @@ static int omap_hsmmc_remove(struct platform_device *pdev)
>>>                       free_irq(mmc_slot(host).card_detect_irq, host);
>>>               flush_work_sync(&host->mmc_carddetect_work);
>>>
>>> -             mmc_host_disable(host->mmc);
>>> -             clk_disable(host->iclk);
>>> +             pm_runtime_put_sync(host->dev);
>>> +             pm_runtime_forbid(host->dev);
>>
>> Why?
>>
>
> Added for balancing pm_runtime_allow added in  _probe.
> But forbid also resume the device on remove.
> Should this be removed, keeping _allow in _probe ?

Neither the _allow or _forbid are needed,   _enable and _disable are enough.

>>> +             pm_runtime_disable(host->dev);
>>>               clk_put(host->fclk);
>>>               clk_put(host->iclk);
>>>               if (host->got_dbclk) {
>>> @@ -2102,6 +2092,8 @@ static int omap_hsmmc_suspend(struct device *dev)
>>>               return 0;
>>>
>>>       if (host) {
>>> +             /* FIXME: TODO move get_sync to proper dev_pm_ops function */
>>
>> what does this mean?
>
> get_sync is needed to enable clock before accessing the registers but
> the discusssion @
> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg50819.html
> suggested to move runtime get_sync calls to .prepare
> Haven't tried it yet.

The _get is fine here, it's the _put that may be the problem.

Based on that thread you mentioned, it is the using of _put and
_put_sync in the suspend path that is the problem.  Basically, use of
runtime PM calls in the suspend/resume path is not recommended and not
guaranteed to work.   It currently works on OMAP, but I may have to
change this.

For now, what is certain is that runtime PM calls in the suspend
callbacks must be the _sync versions.  I'm still working on how to
properly implement the PM domain part for OMAP to correctly implement
the restrictions that the linux-pm maintainers want to enforce.

Kevin

Re: [PATCH 2/3] MMC: OMAP: HSMMC: add runtime pm support

[Paul Walmsley]

(cc'ing Adrian also)

Hi Balaji

On Wed, 22 Jun 2011, Balaji T K wrote:

> Use runtime autosuspend APIs to enable auto suspend delay

Does this really need to use runtime autosuspend?  Seems to me that since 
PM runtime is just controlling the MMC IP blocks and not the regulators in 
this instance, this could simply use pm_runtime_put*() and just avoid the 
extra power wastage and complexity involved in autosuspend.

Besides, the correct autosuspend timeout seems use-case dependent.

By the way, if possible, it would be really good if you could update this 
patch series soon to address the comments.  If these MMC driver changes 
can be ready to merge for 3.1, then we also have a chance of 
integrating some hwmod cleanup patches that depend on this driver's PM 
runtime conversion.


- Paul

Re: [PATCH 2/3] MMC: OMAP: HSMMC: add runtime pm support

[T Krishnamoorthy, Balaji]

On Tue, Jun 28, 2011 at 10:52 PM, Paul Walmsley <paul@pwsan.com> wrote:
> (cc'ing Adrian also)
>
> Hi Balaji
>
> On Wed, 22 Jun 2011, Balaji T K wrote:
>
>> Use runtime autosuspend APIs to enable auto suspend delay
>
> Does this really need to use runtime autosuspend?  Seems to me that since
> PM runtime is just controlling the MMC IP blocks and not the regulators in
> this instance, this could simply use pm_runtime_put*() and just avoid the
> extra power wastage and complexity involved in autosuspend.
>

I have seen some instabilities if delay is very less, on some production boards.
The previous implementation used 100ms delay before disabling the clocks.
Is there any specific issue I should be aware of, for using _autosuspend ?

> Besides, the correct autosuspend timeout seems use-case dependent.
>
> By the way, if possible, it would be really good if you could update this
> patch series soon to address the comments.  If these MMC driver changes
> can be ready to merge for 3.1, then we also have a chance of
> integrating some hwmod cleanup patches that depend on this driver's PM
> runtime conversion.
>
>
Just noticed your mail after I posted my patches..

Re: [PATCH 2/3] MMC: OMAP: HSMMC: add runtime pm support

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 884 bytes --]

On Tue, 28 Jun 2011, T Krishnamoorthy, Balaji wrote:

> On Tue, Jun 28, 2011 at 10:52 PM, Paul Walmsley <paul@pwsan.com> wrote:
> >
> > On Wed, 22 Jun 2011, Balaji T K wrote:
> >
> >> Use runtime autosuspend APIs to enable auto suspend delay
> >
> > Does this really need to use runtime autosuspend?  Seems to me that since
> > PM runtime is just controlling the MMC IP blocks and not the regulators in
> > this instance, this could simply use pm_runtime_put*() and just avoid the
> > extra power wastage and complexity involved in autosuspend.
> 
> I have seen some instabilities if delay is very less, on some production boards.

Could you be more specific?  What type of instabilities did you see?

> The previous implementation used 100ms delay before disabling the clocks.
> Is there any specific issue I should be aware of, for using _autosuspend ?


- Paul

Re: [PATCH 2/3] MMC: OMAP: HSMMC: add runtime pm support

[T Krishnamoorthy, Balaji]

On Wed, Jun 29, 2011 at 12:11 AM, Paul Walmsley <paul@pwsan.com> wrote:
> On Tue, 28 Jun 2011, T Krishnamoorthy, Balaji wrote:
>
>> On Tue, Jun 28, 2011 at 10:52 PM, Paul Walmsley <paul@pwsan.com> wrote:
>> >
>> > On Wed, 22 Jun 2011, Balaji T K wrote:
>> >
>> >> Use runtime autosuspend APIs to enable auto suspend delay
>> >
>> > Does this really need to use runtime autosuspend?  Seems to me that since
>> > PM runtime is just controlling the MMC IP blocks and not the regulators in
>> > this instance, this could simply use pm_runtime_put*() and just avoid the
>> > extra power wastage and complexity involved in autosuspend.
>>
>> I have seen some instabilities if delay is very less, on some production boards.
>
> Could you be more specific?  What type of instabilities did you see?

There have been some experiments on our customer programs to reduce this
value to a few ms and infrequent crashes were observed (stress testing
for several hours) while
trying to access the controller registers.

>
>> The previous implementation used 100ms delay before disabling the clocks.
>> Is there any specific issue I should be aware of, for using _autosuspend ?
>
>
> - Paul

Re: [PATCH 2/3] MMC: OMAP: HSMMC: add runtime pm support

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1177 bytes --]

On Wed, 29 Jun 2011, T Krishnamoorthy, Balaji wrote:

> On Wed, Jun 29, 2011 at 12:11 AM, Paul Walmsley <paul@pwsan.com> wrote:
> > On Tue, 28 Jun 2011, T Krishnamoorthy, Balaji wrote:
> >
> >> On Tue, Jun 28, 2011 at 10:52 PM, Paul Walmsley <paul@pwsan.com> wrote:
> >> >
> >> > On Wed, 22 Jun 2011, Balaji T K wrote:
> >> >
> >> >> Use runtime autosuspend APIs to enable auto suspend delay
> >> >
> >> > Does this really need to use runtime autosuspend?  Seems to me that since
> >> > PM runtime is just controlling the MMC IP blocks and not the regulators in
> >> > this instance, this could simply use pm_runtime_put*() and just avoid the
> >> > extra power wastage and complexity involved in autosuspend.
> >>
> >> I have seen some instabilities if delay is very less, on some production boards.
> >
> > Could you be more specific?  What type of instabilities did you see?
> 
> There have been some experiments on our customer programs to reduce this 
> value to a few ms and infrequent crashes were observed (stress testing 
> for several hours) while trying to access the controller registers.

Was there an oops?  Any analysis, etc.?


- Paul

Re: [PATCH 2/3] MMC: OMAP: HSMMC: add runtime pm support

[T Krishnamoorthy, Balaji]

On Wed, Jun 29, 2011 at 8:12 PM, Paul Walmsley <paul@pwsan.com> wrote:
> On Wed, 29 Jun 2011, T Krishnamoorthy, Balaji wrote:
>
>> On Wed, Jun 29, 2011 at 12:11 AM, Paul Walmsley <paul@pwsan.com> wrote:
>> > On Tue, 28 Jun 2011, T Krishnamoorthy, Balaji wrote:
>> >
>> >> On Tue, Jun 28, 2011 at 10:52 PM, Paul Walmsley <paul@pwsan.com> wrote:
>> >> >
>> >> > On Wed, 22 Jun 2011, Balaji T K wrote:
>> >> >
>> >> >> Use runtime autosuspend APIs to enable auto suspend delay
>> >> >
>> >> > Does this really need to use runtime autosuspend?  Seems to me that since
>> >> > PM runtime is just controlling the MMC IP blocks and not the regulators in
>> >> > this instance, this could simply use pm_runtime_put*() and just avoid the
>> >> > extra power wastage and complexity involved in autosuspend.
>> >>
>> >> I have seen some instabilities if delay is very less, on some production boards.
>> >
>> > Could you be more specific?  What type of instabilities did you see?
>>
>> There have been some experiments on our customer programs to reduce this
>> value to a few ms and infrequent crashes were observed (stress testing
>> for several hours) while trying to access the controller registers.
>
> Was there an oops?  Any analysis, etc.?

OOPS pointed to omap_hsmmc_prepare_data / set_data_timeout
Use case was MMC + SDIO +GPS activity, on kernel 2.6.35 though.

Unhandled fault: imprecise external abort (0x1406) at 0x4073102c
Internal error: : 1406 [#1] PREEMPT SMP
last sysfs file: /sys/devices/platform/switch-sio/usb_sel
Modules linked in: param(P) j4fs(P) vibetonz Si4709_driver fm_drv(C)
bt_drv(C) st_drv(C)
CPU: 0    Tainted: P        WC   (2.6.35.7 #2)
PC is at clk_get_rate+0x4/0x48
LR is at set_data_timeout+0x68/0xf4
[<c06e78e0>] (set_data_timeout+0x0/0xf4) from [<c06e7dc8>]
(omap_hsmmc_request+0x2d0/0x5c8)
 r8:efa78400 r7:00000001 r6:00000000 r5:ef9efe78 r4:efa78640
r3:ef9efee4
[<c06e7af8>] (omap_hsmmc_request+0x0/0x5c8) from [<c06df040>]
(mmc_wait_for_req+0x118/0x130)
[<c06def28>] (mmc_wait_for_req+0x0/0x130) from [<c06e59e8>]
(mmc_blk_issue_rq+0x1c0/0x500)
 r6:ef86f000 r5:efa79000 r4:c91e61a0
[<c06e5828>] (mmc_blk_issue_rq+0x0/0x500) from [<c06e6620>]
(mmc_queue_thread+0xf4/0xf8)
[<c06e652c>] (mmc_queue_thread+0x0/0xf8) from [<c045ddec>] (kthread+0x84/0x8c)
[<c045dd68>] (kthread+0x0/0x8c) from [<c044b748>] (do_exit+0x0/0x604)
 r7:00000013 r6:c044b748 r5:c045dd68 r4:ef8d5d68
Code: e1a00004 e89da8f0 c0a653c0 e1a0c00d (e92dd818)
---[ end trace 533b4c955f5abafd ]---

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

Re: [PATCH 2/3] MMC: OMAP: HSMMC: add runtime pm support

[Paul Walmsley]

+ Venkat

Hi Balaji, Venkat,

On Wed, 29 Jun 2011, T Krishnamoorthy, Balaji wrote:

> OOPS pointed to omap_hsmmc_prepare_data / set_data_timeout
> Use case was MMC + SDIO +GPS activity, on kernel 2.6.35 though.
> 
> Unhandled fault: imprecise external abort (0x1406) at 0x4073102c
                   ^^^^^^^^^

That's not good...

> Internal error: : 1406 [#1] PREEMPT SMP
> last sysfs file: /sys/devices/platform/switch-sio/usb_sel
> Modules linked in: param(P) j4fs(P) vibetonz Si4709_driver fm_drv(C)
> bt_drv(C) st_drv(C)
> CPU: 0    Tainted: P        WC   (2.6.35.7 #2)
            ^^^^^^^^^^

Neither is this...

> PC is at clk_get_rate+0x4/0x48
> LR is at set_data_timeout+0x68/0xf4
> [<c06e78e0>] (set_data_timeout+0x0/0xf4) from [<c06e7dc8>]
> (omap_hsmmc_request+0x2d0/0x5c8)
>  r8:efa78400 r7:00000001 r6:00000000 r5:ef9efe78 r4:efa78640
> r3:ef9efee4
> [<c06e7af8>] (omap_hsmmc_request+0x0/0x5c8) from [<c06df040>]
> (mmc_wait_for_req+0x118/0x130)
> [<c06def28>] (mmc_wait_for_req+0x0/0x130) from [<c06e59e8>]
> (mmc_blk_issue_rq+0x1c0/0x500)
>  r6:ef86f000 r5:efa79000 r4:c91e61a0
> [<c06e5828>] (mmc_blk_issue_rq+0x0/0x500) from [<c06e6620>]
> (mmc_queue_thread+0xf4/0xf8)
> [<c06e652c>] (mmc_queue_thread+0x0/0xf8) from [<c045ddec>] (kthread+0x84/0x8c)
> [<c045dd68>] (kthread+0x0/0x8c) from [<c044b748>] (do_exit+0x0/0x604)
>  r7:00000013 r6:c044b748 r5:c045dd68 r4:ef8d5d68

Was CONFIG_PM enabled?  If so, was off-mode enabled for any IP blocks, or 
was it just clock-stop and/or retention?


- Paul

Re: [PATCH 2/3] MMC: OMAP: HSMMC: add runtime pm support

[Paul Walmsley]

On Wed, 29 Jun 2011, T Krishnamoorthy, Balaji wrote:

> There have been some experiments on our customer programs to reduce this 
> value to a few ms and infrequent crashes were observed (stress testing 
> for several hours) while trying to access the controller registers.

By the way, could you send along a copy of the stress test script?


- Paul

Re: [PATCH 2/3] MMC: OMAP: HSMMC: add runtime pm support

[S, Venkatraman]

On Wed, Jun 29, 2011 at 9:08 PM, Paul Walmsley <paul@pwsan.com> wrote:
> On Wed, 29 Jun 2011, T Krishnamoorthy, Balaji wrote:
>
>> There have been some experiments on our customer programs to reduce this
>> value to a few ms and infrequent crashes were observed (stress testing
>> for several hours) while trying to access the controller registers.
>
> By the way, could you send along a copy of the stress test script?
>

Paul, these scenarios are not scripted but end user tests with
additional devices
(WLAN, which is connected on the same controller) and executed 'on field' .
One such log is here .. http://pastebin.com/nq3cfZnT

Re: [PATCH 2/3] MMC: OMAP: HSMMC: add runtime pm support

[Paul Walmsley]

cc'ing lakml

Hi Venkat, Balaji,

On Wed, 29 Jun 2011, S, Venkatraman wrote:

> On Wed, Jun 29, 2011 at 9:08 PM, Paul Walmsley <paul@pwsan.com> wrote:
> > On Wed, 29 Jun 2011, T Krishnamoorthy, Balaji wrote:
> >
> >> There have been some experiments on our customer programs to reduce this
> >> value to a few ms and infrequent crashes were observed (stress testing
> >> for several hours) while trying to access the controller registers.
> >
> > By the way, could you send along a copy of the stress test script?
> >
> 
> Paul, these scenarios are not scripted but end user tests with
> additional devices
> (WLAN, which is connected on the same controller) and executed 'on field' .

OK, thanks Venkat.  Do you still have one of these devices so the test can 
be repeated?

> One such log is here .. http://pastebin.com/nq3cfZnT

Looks like this is an Android 2.6.35.7-derived kernel on a 4430 ES2.2 EMU.  
Power management is enabled but MPU, L3INIT, and PER aren't entering any 
deeper power states than retention idle, so no context save/restore or 
off-mode worries here.

The system looks like it's entered suspend at least once and resumed, 
before the oops.  Also the second CPU is starting up and shutting down 
dynamically.  Backtrace is copied below for the archives.

Does the above summary match your understanding?

...

Reviewing this backtrace and the one that Balaji sent, it looks to 
me like this write in omap_hsmmc_prepare_data() is the proximate 
cause of the abort:

	OMAP_HSMMC_WRITE(host->base, BLK, (req->data->blksz)
					| (req->data->blocks << 16));

I'll bet this was first access to the MMC IP block after the MMC layer 
re-enabled it.  The abort is imprecise because the Linux OMAP4 kernel 
marks MMIO registers as bufferable, so the ARM can continue executing 
while a register write is making its way across the OMAP interconnect(s).  
This guess also assumes that the ARM is executing instructions out of 
order, which is a reasonable assumption on a Cortex-A9.  This could be 
confirmed by reading some HSMMC register right before the 
OMAP_HSMMC_WRITE(); then the abort would turn precise and occur on the 
read.

Anyway, it looks like the HSMMC IP block wasn't yet ready to be accessed.  
Probably, this is because either the HSMMC IP block hasn't yet left the 
Idle or SleepTrans states, and the OMAP4 clock framework doesn't wait for 
that; or the PRCM is getting confused because the correct clockdomain 
enable sequence isn't being followed -- see for example the "Fix 
module-mode enable sequence on OMAP4" patch series that have been posted 
to the linux-omap mailing list.  Probably one of those two issues is the 
root cause.

If you have a testing setup where you can reproduce this problem, I'd 
suggest adding the read as described above.  Otherwise, I don't think this 
will be an issue for the runtime PM conversion: first, because the hwmod 
code will wait for the HSMMC block to indicate that it has left idle 
before continuing; and second, because we'll hopefully have a patch series 
going in at the same time to make sure the clockdomain enable sequence is 
correct.


- Paul


<0> Process mmcqd (pid: 851, stack limit = 0xef9682f8)
<0> Stack: (0xef969db8 to 0xef96a000)
<0> 9da0:                                                       ef969ee4 efa30640
<0> 9dc0: ef969e78 00000000 00000001 efa30400 ef969e2c ef969de0 c06ae2b8 c06ace10
<0> 9de0: 00000000 efa305d8 ef969e04 efa30400 00000000 efa30578 ef969e44 ef969e08
<0> 9e00: c054ea5c ef969e78 efa30400 ef969e34 00000001 ef837e4c 00000000 ef969ee4
<0> 9e20: ef969e64 ef969e30 c06a54d8 c06adff4 00000000 00000000 00000000 00000000
<0> 9e40: ef969e40 ef969e40 ed3d4680 ed3d4680 efa30c00 ef837e40 ef969f94 ef969e68
<0> 9e60: c06abe80 c06a53cc 00000000 efa31458 ef0cfdb4 ef0cfdb4 ef969e8c ef969ee4
<0> 9e80: ef969eb8 ef969e34 c06a55d0 00000019 00fd50a2 00000000 00000000 00000000
<0> 9ea0: 00000000 000000b5 00000000 00000000 ef969ee4 ef969e78 0000000c 00000000
<0> 9ec0: 00000000 00000000 00000000 00000000 0000049d 00000000 00000000 00000000
<0> 9ee0: ef969e78 23c34600 00000fa0 00000200 00000400 00000000 00000100 00000000
<0> 9f00: ef969eb8 ef969e78 0000003f ef238000 ef969f54 ef969f20 c0556c00 c0555fac
<0> 9f20: ef969f3c 00000001 c0425fa0 ef837e4c ef230000 ef837e54 ef837e4c ef230000
<0> 9f40: ef837e54 ef230000 ef969f7c ef969f58 00000000 ed3d4680 ef969f7c ef969f68
<0> 9f60: c054911c c054ee7c 01082e21 ef837e4c ef968000 ef837e54 ef230000 ef2301d8
<0> 9f80: 00000000 ed3d4680 ef969fbc ef969f98 c06acab8 c06abccc ef985d68 ef969fc4
<0> 9fa0: c06ac9c4 ef837e4c 00000000 00000000 ef969ff4 ef969fc0 c041fd20 c06ac9d0
<0> 9fc0: 00000000 00000000 00000000 00000000 ef969fd0 ef969fd0 ef985d68 c041fc9c
<0> 9fe0: c040d67c 00000013 00000000 ef969ff8 c040d67c c041fca8 00000000 00000000
<4> Backtrace:
<4> [<c06ace04>] (set_data_timeout+0x0/0xcc) from [<c06ae2b8>] (omap_hsmmc_request+0x2d0/0x5c8)
<4>  r8:efa30400 r7:00000001 r6:00000000 r5:ef969e78 r4:efa30640
<4> r3:ef969ee4
<4> [<c06adfe8>] (omap_hsmmc_request+0x0/0x5c8) from [<c06a54d8>] (mmc_wait_for_req+0x118/0x130)
<4> [<c06a53c0>] (mmc_wait_for_req+0x0/0x130) from [<c06abe80>] (mmc_blk_issue_rq+0x1c0/0x500)
<4>  r6:ef837e40 r5:efa30c00 r4:ed3d4680
<4> [<c06abcc0>] (mmc_blk_issue_rq+0x0/0x500) from [<c06acab8>] (mmc_queue_thread+0xf4/0xf8)
<4> [<c06ac9c4>] (mmc_queue_thread+0x0/0xf8) from [<c041fd20>] (kthread+0x84/0x8c)
<4> [<c041fc9c>] (kthread+0x0/0x8c) from [<c040d67c>] (do_exit+0x0/0x604)
<4>  r7:00000013 r6:c040d67c r5:c041fc9c r4:ef985d68
<0> Code: 11a0c423 11c0c0b0 e1a0f00e e2512001 (01a0f00e)
<4> ---[ end trace d27fcce5bd5b71d6 ]---

[PATCH 2/3] MMC: OMAP: HSMMC: add runtime pm support

[Paul Walmsley]

cc'ing lakml

Hi Venkat, Balaji,

On Wed, 29 Jun 2011, S, Venkatraman wrote:

> On Wed, Jun 29, 2011 at 9:08 PM, Paul Walmsley <paul@pwsan.com> wrote:
> > On Wed, 29 Jun 2011, T Krishnamoorthy, Balaji wrote:
> >
> >> There have been some experiments on our customer programs to reduce this
> >> value to a few ms and infrequent crashes were observed (stress testing
> >> for several hours) while trying to access the controller registers.
> >
> > By the way, could you send along a copy of the stress test script?
> >
> 
> Paul, these scenarios are not scripted but end user tests with
> additional devices
> (WLAN, which is connected on the same controller) and executed 'on field' .

OK, thanks Venkat.  Do you still have one of these devices so the test can 
be repeated?

> One such log is here .. http://pastebin.com/nq3cfZnT

Looks like this is an Android 2.6.35.7-derived kernel on a 4430 ES2.2 EMU.  
Power management is enabled but MPU, L3INIT, and PER aren't entering any 
deeper power states than retention idle, so no context save/restore or 
off-mode worries here.

The system looks like it's entered suspend at least once and resumed, 
before the oops.  Also the second CPU is starting up and shutting down 
dynamically.  Backtrace is copied below for the archives.

Does the above summary match your understanding?

...

Reviewing this backtrace and the one that Balaji sent, it looks to 
me like this write in omap_hsmmc_prepare_data() is the proximate 
cause of the abort:

	OMAP_HSMMC_WRITE(host->base, BLK, (req->data->blksz)
					| (req->data->blocks << 16));

I'll bet this was first access to the MMC IP block after the MMC layer 
re-enabled it.  The abort is imprecise because the Linux OMAP4 kernel 
marks MMIO registers as bufferable, so the ARM can continue executing 
while a register write is making its way across the OMAP interconnect(s).  
This guess also assumes that the ARM is executing instructions out of 
order, which is a reasonable assumption on a Cortex-A9.  This could be 
confirmed by reading some HSMMC register right before the 
OMAP_HSMMC_WRITE(); then the abort would turn precise and occur on the 
read.

Anyway, it looks like the HSMMC IP block wasn't yet ready to be accessed.  
Probably, this is because either the HSMMC IP block hasn't yet left the 
Idle or SleepTrans states, and the OMAP4 clock framework doesn't wait for 
that; or the PRCM is getting confused because the correct clockdomain 
enable sequence isn't being followed -- see for example the "Fix 
module-mode enable sequence on OMAP4" patch series that have been posted 
to the linux-omap mailing list.  Probably one of those two issues is the 
root cause.

If you have a testing setup where you can reproduce this problem, I'd 
suggest adding the read as described above.  Otherwise, I don't think this 
will be an issue for the runtime PM conversion: first, because the hwmod 
code will wait for the HSMMC block to indicate that it has left idle 
before continuing; and second, because we'll hopefully have a patch series 
going in@the same time to make sure the clockdomain enable sequence is 
correct.


- Paul


<0> Process mmcqd (pid: 851, stack limit = 0xef9682f8)
<0> Stack: (0xef969db8 to 0xef96a000)
<0> 9da0:                                                       ef969ee4 efa30640
<0> 9dc0: ef969e78 00000000 00000001 efa30400 ef969e2c ef969de0 c06ae2b8 c06ace10
<0> 9de0: 00000000 efa305d8 ef969e04 efa30400 00000000 efa30578 ef969e44 ef969e08
<0> 9e00: c054ea5c ef969e78 efa30400 ef969e34 00000001 ef837e4c 00000000 ef969ee4
<0> 9e20: ef969e64 ef969e30 c06a54d8 c06adff4 00000000 00000000 00000000 00000000
<0> 9e40: ef969e40 ef969e40 ed3d4680 ed3d4680 efa30c00 ef837e40 ef969f94 ef969e68
<0> 9e60: c06abe80 c06a53cc 00000000 efa31458 ef0cfdb4 ef0cfdb4 ef969e8c ef969ee4
<0> 9e80: ef969eb8 ef969e34 c06a55d0 00000019 00fd50a2 00000000 00000000 00000000
<0> 9ea0: 00000000 000000b5 00000000 00000000 ef969ee4 ef969e78 0000000c 00000000
<0> 9ec0: 00000000 00000000 00000000 00000000 0000049d 00000000 00000000 00000000
<0> 9ee0: ef969e78 23c34600 00000fa0 00000200 00000400 00000000 00000100 00000000
<0> 9f00: ef969eb8 ef969e78 0000003f ef238000 ef969f54 ef969f20 c0556c00 c0555fac
<0> 9f20: ef969f3c 00000001 c0425fa0 ef837e4c ef230000 ef837e54 ef837e4c ef230000
<0> 9f40: ef837e54 ef230000 ef969f7c ef969f58 00000000 ed3d4680 ef969f7c ef969f68
<0> 9f60: c054911c c054ee7c 01082e21 ef837e4c ef968000 ef837e54 ef230000 ef2301d8
<0> 9f80: 00000000 ed3d4680 ef969fbc ef969f98 c06acab8 c06abccc ef985d68 ef969fc4
<0> 9fa0: c06ac9c4 ef837e4c 00000000 00000000 ef969ff4 ef969fc0 c041fd20 c06ac9d0
<0> 9fc0: 00000000 00000000 00000000 00000000 ef969fd0 ef969fd0 ef985d68 c041fc9c
<0> 9fe0: c040d67c 00000013 00000000 ef969ff8 c040d67c c041fca8 00000000 00000000
<4> Backtrace:
<4> [<c06ace04>] (set_data_timeout+0x0/0xcc) from [<c06ae2b8>] (omap_hsmmc_request+0x2d0/0x5c8)
<4>  r8:efa30400 r7:00000001 r6:00000000 r5:ef969e78 r4:efa30640
<4> r3:ef969ee4
<4> [<c06adfe8>] (omap_hsmmc_request+0x0/0x5c8) from [<c06a54d8>] (mmc_wait_for_req+0x118/0x130)
<4> [<c06a53c0>] (mmc_wait_for_req+0x0/0x130) from [<c06abe80>] (mmc_blk_issue_rq+0x1c0/0x500)
<4>  r6:ef837e40 r5:efa30c00 r4:ed3d4680
<4> [<c06abcc0>] (mmc_blk_issue_rq+0x0/0x500) from [<c06acab8>] (mmc_queue_thread+0xf4/0xf8)
<4> [<c06ac9c4>] (mmc_queue_thread+0x0/0xf8) from [<c041fd20>] (kthread+0x84/0x8c)
<4> [<c041fc9c>] (kthread+0x0/0x8c) from [<c040d67c>] (do_exit+0x0/0x604)
<4>  r7:00000013 r6:c040d67c r5:c041fc9c r4:ef985d68
<0> Code: 11a0c423 11c0c0b0 e1a0f00e e2512001 (01a0f00e)
<4> ---[ end trace d27fcce5bd5b71d6 ]---

Re: [PATCH 2/3] MMC: OMAP: HSMMC: add runtime pm support

[S, Venkatraman]

On Thu, Jun 30, 2011 at 1:37 AM, Paul Walmsley <paul@pwsan.com> wrote:
> cc'ing lakml
>
> Hi Venkat, Balaji,
>
> On Wed, 29 Jun 2011, S, Venkatraman wrote:
>
>> On Wed, Jun 29, 2011 at 9:08 PM, Paul Walmsley <paul@pwsan.com> wrote:
>> > On Wed, 29 Jun 2011, T Krishnamoorthy, Balaji wrote:
>> >
>> >> There have been some experiments on our customer programs to reduce this
>> >> value to a few ms and infrequent crashes were observed (stress testing
>> >> for several hours) while trying to access the controller registers.
>> >
>> > By the way, could you send along a copy of the stress test script?
>> >
>>
>> Paul, these scenarios are not scripted but end user tests with
>> additional devices
>> (WLAN, which is connected on the same controller) and executed 'on field' .
>
> OK, thanks Venkat.  Do you still have one of these devices so the test can
> be repeated?
>
>> One such log is here .. http://pastebin.com/nq3cfZnT
>
> Looks like this is an Android 2.6.35.7-derived kernel on a 4430 ES2.2 EMU.
> Power management is enabled but MPU, L3INIT, and PER aren't entering any
> deeper power states than retention idle, so no context save/restore or
> off-mode worries here.
>
> The system looks like it's entered suspend at least once and resumed,
> before the oops.  Also the second CPU is starting up and shutting down
> dynamically.  Backtrace is copied below for the archives.
>
> Does the above summary match your understanding?

Yes it does.
>
> ...
>
> Reviewing this backtrace and the one that Balaji sent, it looks to
> me like this write in omap_hsmmc_prepare_data() is the proximate
> cause of the abort:
>
>        OMAP_HSMMC_WRITE(host->base, BLK, (req->data->blksz)
>                                        | (req->data->blocks << 16));
>
> I'll bet this was first access to the MMC IP block after the MMC layer
> re-enabled it.  The abort is imprecise because the Linux OMAP4 kernel
> marks MMIO registers as bufferable, so the ARM can continue executing
> while a register write is making its way across the OMAP interconnect(s).
> This guess also assumes that the ARM is executing instructions out of
> order, which is a reasonable assumption on a Cortex-A9.  This could be
> confirmed by reading some HSMMC register right before the
> OMAP_HSMMC_WRITE(); then the abort would turn precise and occur on the
> read.

Yes - The issue is not an issue with the set_data_timeout function but the
_first_ access to MMC IP register blocks after enabling the mmc_host.
(This backtrace signature is very common during MMC-PM hackathons )
  But I have not seen any difference if the access is read or write..
Will check again..

>
> Anyway, it looks like the HSMMC IP block wasn't yet ready to be accessed.
> Probably, this is because either the HSMMC IP block hasn't yet left the
> Idle or SleepTrans states, and the OMAP4 clock framework doesn't wait for
> that; or the PRCM is getting confused because the correct clockdomain
> enable sequence isn't being followed -- see for example the "Fix
> module-mode enable sequence on OMAP4" patch series that have been posted
> to the linux-omap mailing list.  Probably one of those two issues is the
> root cause.
>
> If you have a testing setup where you can reproduce this problem, I'd
> suggest adding the read as described above.  Otherwise, I don't think this
> will be an issue for the runtime PM conversion: first, because the hwmod
> code will wait for the HSMMC block to indicate that it has left idle
> before continuing; and second, because we'll hopefully have a patch series
> going in at the same time to make sure the clockdomain enable sequence is
> correct.
>

As you might have guessed, the test setup is not accessible for me and it's not
a simulated environment or scripted test. I'll try to check if some testcases
can be written to simulate this.

>
> - Paul
>
>
> <0> Process mmcqd (pid: 851, stack limit = 0xef9682f8)
> <0> Stack: (0xef969db8 to 0xef96a000)
> <0> 9da0:                                                       ef969ee4 efa30640
> <0> 9dc0: ef969e78 00000000 00000001 efa30400 ef969e2c ef969de0 c06ae2b8 c06ace10
> <0> 9de0: 00000000 efa305d8 ef969e04 efa30400 00000000 efa30578 ef969e44 ef969e08
> <0> 9e00: c054ea5c ef969e78 efa30400 ef969e34 00000001 ef837e4c 00000000 ef969ee4
> <0> 9e20: ef969e64 ef969e30 c06a54d8 c06adff4 00000000 00000000 00000000 00000000
> <0> 9e40: ef969e40 ef969e40 ed3d4680 ed3d4680 efa30c00 ef837e40 ef969f94 ef969e68
> <0> 9e60: c06abe80 c06a53cc 00000000 efa31458 ef0cfdb4 ef0cfdb4 ef969e8c ef969ee4
> <0> 9e80: ef969eb8 ef969e34 c06a55d0 00000019 00fd50a2 00000000 00000000 00000000
> <0> 9ea0: 00000000 000000b5 00000000 00000000 ef969ee4 ef969e78 0000000c 00000000
> <0> 9ec0: 00000000 00000000 00000000 00000000 0000049d 00000000 00000000 00000000
> <0> 9ee0: ef969e78 23c34600 00000fa0 00000200 00000400 00000000 00000100 00000000
> <0> 9f00: ef969eb8 ef969e78 0000003f ef238000 ef969f54 ef969f20 c0556c00 c0555fac
> <0> 9f20: ef969f3c 00000001 c0425fa0 ef837e4c ef230000 ef837e54 ef837e4c ef230000
> <0> 9f40: ef837e54 ef230000 ef969f7c ef969f58 00000000 ed3d4680 ef969f7c ef969f68
> <0> 9f60: c054911c c054ee7c 01082e21 ef837e4c ef968000 ef837e54 ef230000 ef2301d8
> <0> 9f80: 00000000 ed3d4680 ef969fbc ef969f98 c06acab8 c06abccc ef985d68 ef969fc4
> <0> 9fa0: c06ac9c4 ef837e4c 00000000 00000000 ef969ff4 ef969fc0 c041fd20 c06ac9d0
> <0> 9fc0: 00000000 00000000 00000000 00000000 ef969fd0 ef969fd0 ef985d68 c041fc9c
> <0> 9fe0: c040d67c 00000013 00000000 ef969ff8 c040d67c c041fca8 00000000 00000000
> <4> Backtrace:
> <4> [<c06ace04>] (set_data_timeout+0x0/0xcc) from [<c06ae2b8>] (omap_hsmmc_request+0x2d0/0x5c8)
> <4>  r8:efa30400 r7:00000001 r6:00000000 r5:ef969e78 r4:efa30640
> <4> r3:ef969ee4
> <4> [<c06adfe8>] (omap_hsmmc_request+0x0/0x5c8) from [<c06a54d8>] (mmc_wait_for_req+0x118/0x130)
> <4> [<c06a53c0>] (mmc_wait_for_req+0x0/0x130) from [<c06abe80>] (mmc_blk_issue_rq+0x1c0/0x500)
> <4>  r6:ef837e40 r5:efa30c00 r4:ed3d4680
> <4> [<c06abcc0>] (mmc_blk_issue_rq+0x0/0x500) from [<c06acab8>] (mmc_queue_thread+0xf4/0xf8)
> <4> [<c06ac9c4>] (mmc_queue_thread+0x0/0xf8) from [<c041fd20>] (kthread+0x84/0x8c)
> <4> [<c041fc9c>] (kthread+0x0/0x8c) from [<c040d67c>] (do_exit+0x0/0x604)
> <4>  r7:00000013 r6:c040d67c r5:c041fc9c r4:ef985d68
> <0> Code: 11a0c423 11c0c0b0 e1a0f00e e2512001 (01a0f00e)
> <4> ---[ end trace d27fcce5bd5b71d6 ]---
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] MMC: OMAP: HSMMC: add runtime pm support

[S, Venkatraman]

On Thu, Jun 30, 2011 at 1:37 AM, Paul Walmsley <paul@pwsan.com> wrote:
> cc'ing lakml
>
> Hi Venkat, Balaji,
>
> On Wed, 29 Jun 2011, S, Venkatraman wrote:
>
>> On Wed, Jun 29, 2011 at 9:08 PM, Paul Walmsley <paul@pwsan.com> wrote:
>> > On Wed, 29 Jun 2011, T Krishnamoorthy, Balaji wrote:
>> >
>> >> There have been some experiments on our customer programs to reduce this
>> >> value to a few ms and infrequent crashes were observed (stress testing
>> >> for several hours) while trying to access the controller registers.
>> >
>> > By the way, could you send along a copy of the stress test script?
>> >
>>
>> Paul, these scenarios are not scripted but end user tests with
>> additional devices
>> (WLAN, which is connected on the same controller) and executed 'on field' .
>
> OK, thanks Venkat. ?Do you still have one of these devices so the test can
> be repeated?
>
>> One such log is here .. http://pastebin.com/nq3cfZnT
>
> Looks like this is an Android 2.6.35.7-derived kernel on a 4430 ES2.2 EMU.
> Power management is enabled but MPU, L3INIT, and PER aren't entering any
> deeper power states than retention idle, so no context save/restore or
> off-mode worries here.
>
> The system looks like it's entered suspend at least once and resumed,
> before the oops. ?Also the second CPU is starting up and shutting down
> dynamically. ?Backtrace is copied below for the archives.
>
> Does the above summary match your understanding?

Yes it does.
>
> ...
>
> Reviewing this backtrace and the one that Balaji sent, it looks to
> me like this write in omap_hsmmc_prepare_data() is the proximate
> cause of the abort:
>
> ? ? ? ?OMAP_HSMMC_WRITE(host->base, BLK, (req->data->blksz)
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?| (req->data->blocks << 16));
>
> I'll bet this was first access to the MMC IP block after the MMC layer
> re-enabled it. ?The abort is imprecise because the Linux OMAP4 kernel
> marks MMIO registers as bufferable, so the ARM can continue executing
> while a register write is making its way across the OMAP interconnect(s).
> This guess also assumes that the ARM is executing instructions out of
> order, which is a reasonable assumption on a Cortex-A9. ?This could be
> confirmed by reading some HSMMC register right before the
> OMAP_HSMMC_WRITE(); then the abort would turn precise and occur on the
> read.

Yes - The issue is not an issue with the set_data_timeout function but the
_first_ access to MMC IP register blocks after enabling the mmc_host.
(This backtrace signature is very common during MMC-PM hackathons )
  But I have not seen any difference if the access is read or write..
Will check again..

>
> Anyway, it looks like the HSMMC IP block wasn't yet ready to be accessed.
> Probably, this is because either the HSMMC IP block hasn't yet left the
> Idle or SleepTrans states, and the OMAP4 clock framework doesn't wait for
> that; or the PRCM is getting confused because the correct clockdomain
> enable sequence isn't being followed -- see for example the "Fix
> module-mode enable sequence on OMAP4" patch series that have been posted
> to the linux-omap mailing list. ?Probably one of those two issues is the
> root cause.
>
> If you have a testing setup where you can reproduce this problem, I'd
> suggest adding the read as described above. ?Otherwise, I don't think this
> will be an issue for the runtime PM conversion: first, because the hwmod
> code will wait for the HSMMC block to indicate that it has left idle
> before continuing; and second, because we'll hopefully have a patch series
> going in at the same time to make sure the clockdomain enable sequence is
> correct.
>

As you might have guessed, the test setup is not accessible for me and it's not
a simulated environment or scripted test. I'll try to check if some testcases
can be written to simulate this.

>
> - Paul
>
>
> <0> Process mmcqd (pid: 851, stack limit = 0xef9682f8)
> <0> Stack: (0xef969db8 to 0xef96a000)
> <0> 9da0: ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ef969ee4 efa30640
> <0> 9dc0: ef969e78 00000000 00000001 efa30400 ef969e2c ef969de0 c06ae2b8 c06ace10
> <0> 9de0: 00000000 efa305d8 ef969e04 efa30400 00000000 efa30578 ef969e44 ef969e08
> <0> 9e00: c054ea5c ef969e78 efa30400 ef969e34 00000001 ef837e4c 00000000 ef969ee4
> <0> 9e20: ef969e64 ef969e30 c06a54d8 c06adff4 00000000 00000000 00000000 00000000
> <0> 9e40: ef969e40 ef969e40 ed3d4680 ed3d4680 efa30c00 ef837e40 ef969f94 ef969e68
> <0> 9e60: c06abe80 c06a53cc 00000000 efa31458 ef0cfdb4 ef0cfdb4 ef969e8c ef969ee4
> <0> 9e80: ef969eb8 ef969e34 c06a55d0 00000019 00fd50a2 00000000 00000000 00000000
> <0> 9ea0: 00000000 000000b5 00000000 00000000 ef969ee4 ef969e78 0000000c 00000000
> <0> 9ec0: 00000000 00000000 00000000 00000000 0000049d 00000000 00000000 00000000
> <0> 9ee0: ef969e78 23c34600 00000fa0 00000200 00000400 00000000 00000100 00000000
> <0> 9f00: ef969eb8 ef969e78 0000003f ef238000 ef969f54 ef969f20 c0556c00 c0555fac
> <0> 9f20: ef969f3c 00000001 c0425fa0 ef837e4c ef230000 ef837e54 ef837e4c ef230000
> <0> 9f40: ef837e54 ef230000 ef969f7c ef969f58 00000000 ed3d4680 ef969f7c ef969f68
> <0> 9f60: c054911c c054ee7c 01082e21 ef837e4c ef968000 ef837e54 ef230000 ef2301d8
> <0> 9f80: 00000000 ed3d4680 ef969fbc ef969f98 c06acab8 c06abccc ef985d68 ef969fc4
> <0> 9fa0: c06ac9c4 ef837e4c 00000000 00000000 ef969ff4 ef969fc0 c041fd20 c06ac9d0
> <0> 9fc0: 00000000 00000000 00000000 00000000 ef969fd0 ef969fd0 ef985d68 c041fc9c
> <0> 9fe0: c040d67c 00000013 00000000 ef969ff8 c040d67c c041fca8 00000000 00000000
> <4> Backtrace:
> <4> [<c06ace04>] (set_data_timeout+0x0/0xcc) from [<c06ae2b8>] (omap_hsmmc_request+0x2d0/0x5c8)
> <4> ?r8:efa30400 r7:00000001 r6:00000000 r5:ef969e78 r4:efa30640
> <4> r3:ef969ee4
> <4> [<c06adfe8>] (omap_hsmmc_request+0x0/0x5c8) from [<c06a54d8>] (mmc_wait_for_req+0x118/0x130)
> <4> [<c06a53c0>] (mmc_wait_for_req+0x0/0x130) from [<c06abe80>] (mmc_blk_issue_rq+0x1c0/0x500)
> <4> ?r6:ef837e40 r5:efa30c00 r4:ed3d4680
> <4> [<c06abcc0>] (mmc_blk_issue_rq+0x0/0x500) from [<c06acab8>] (mmc_queue_thread+0xf4/0xf8)
> <4> [<c06ac9c4>] (mmc_queue_thread+0x0/0xf8) from [<c041fd20>] (kthread+0x84/0x8c)
> <4> [<c041fc9c>] (kthread+0x0/0x8c) from [<c040d67c>] (do_exit+0x0/0x604)
> <4> ?r7:00000013 r6:c040d67c r5:c041fc9c r4:ef985d68
> <0> Code: 11a0c423 11c0c0b0 e1a0f00e e2512001 (01a0f00e)
> <4> ---[ end trace d27fcce5bd5b71d6 ]---
>
>
>

Re: [PATCH 2/3] MMC: OMAP: HSMMC: add runtime pm support

[Kevin Hilman]

"T Krishnamoorthy, Balaji" <balajitk@ti.com> writes:

> On Tue, Jun 28, 2011 at 10:52 PM, Paul Walmsley <paul@pwsan.com> wrote:
>> (cc'ing Adrian also)
>>
>> Hi Balaji
>>
>> On Wed, 22 Jun 2011, Balaji T K wrote:
>>
>>> Use runtime autosuspend APIs to enable auto suspend delay
>>
>> Does this really need to use runtime autosuspend?  Seems to me that since
>> PM runtime is just controlling the MMC IP blocks and not the regulators in
>> this instance, this could simply use pm_runtime_put*() and just avoid the
>> extra power wastage and complexity involved in autosuspend.
>>
>
> I have seen some instabilities if delay is very less, on some production boards.
> The previous implementation used 100ms delay before disabling the clocks.

And your new one is using 50ms.  How did this value come about?

As Paul mentioned, the timeout value here is probably usecase depeend

Kevin

Re: [PATCH 2/3] MMC: OMAP: HSMMC: add runtime pm support

[T Krishnamoorthy, Balaji]

On Wed, Jun 29, 2011 at 2:00 AM, Kevin Hilman <khilman@ti.com> wrote:
> "T Krishnamoorthy, Balaji" <balajitk@ti.com> writes:
>
>> On Tue, Jun 28, 2011 at 10:52 PM, Paul Walmsley <paul@pwsan.com> wrote:
>>> (cc'ing Adrian also)
>>>
>>> Hi Balaji
>>>
>>> On Wed, 22 Jun 2011, Balaji T K wrote:
>>>
>>>> Use runtime autosuspend APIs to enable auto suspend delay
>>>
>>> Does this really need to use runtime autosuspend?  Seems to me that since
>>> PM runtime is just controlling the MMC IP blocks and not the regulators in
>>> this instance, this could simply use pm_runtime_put*() and just avoid the
>>> extra power wastage and complexity involved in autosuspend.
>>>
>>
>> I have seen some instabilities if delay is very less, on some production boards.
>> The previous implementation used 100ms delay before disabling the clocks.
>
> And your new one is using 50ms.  How did this value come about?
>
I don't have any specific affinity to this number, but when requests
are bursty, they arrive
within a few 10s of ms within each other. Didn't want to have the
context/save restore
penalty associated with every request.

> As Paul mentioned, the timeout value here is probably usecase depeend
>

There is no direct relationship to the use case. Block layer buffers
and reworks the order
of requests and they are usually queued together. Having no context
save / restore
penalty for each request is definitely desirable.
(As I understand, MMC can lose context independent of MPU on OMAP4).

Re: [PATCH 2/3] MMC: OMAP: HSMMC: add runtime pm support

[Kevin Hilman]

"T Krishnamoorthy, Balaji" <balajitk@ti.com> writes:

> On Wed, Jun 29, 2011 at 2:00 AM, Kevin Hilman <khilman@ti.com> wrote:
>> "T Krishnamoorthy, Balaji" <balajitk@ti.com> writes:
>>
>>> On Tue, Jun 28, 2011 at 10:52 PM, Paul Walmsley <paul@pwsan.com> wrote:
>>>> (cc'ing Adrian also)
>>>>
>>>> Hi Balaji
>>>>
>>>> On Wed, 22 Jun 2011, Balaji T K wrote:
>>>>
>>>>> Use runtime autosuspend APIs to enable auto suspend delay
>>>>
>>>> Does this really need to use runtime autosuspend?  Seems to me that since
>>>> PM runtime is just controlling the MMC IP blocks and not the regulators in
>>>> this instance, this could simply use pm_runtime_put*() and just avoid the
>>>> extra power wastage and complexity involved in autosuspend.
>>>>
>>>
>>> I have seen some instabilities if delay is very less, on some production boards.
>>> The previous implementation used 100ms delay before disabling the clocks.
>>
>> And your new one is using 50ms.  How did this value come about?
>>

> I don't have any specific affinity to this number, but when requests
> are bursty, they arrive within a few 10s of ms within each
> other. Didn't want to have the context/save restore penalty associated
> with every request.

Have you measured the context save/restore penalty when only the clocks
are gated (and no regulators involved) ?

In the current code, it's understandable why there were large latencies
that were avoided because the regulators were also cut.  With only the
clocks being cut, the latency involved will be significantly less.

>> As Paul mentioned, the timeout value here is probably usecase depeend
>>
>
> There is no direct relationship to the use case. Block layer buffers
> and reworks the order of requests and they are usually queued
> together. Having no context save / restore penalty for each request is
> definitely desirable.  

Desirable for what?

What is implied in that statement is desirable for performance.

What if a different user would prefer the power savings gained by more
aggressively cuttting clocks over the performance gains of leaving
clocks enabled?

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

Re: [PATCH 2/3] MMC: OMAP: HSMMC: add runtime pm support

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1226 bytes --]

+ Venkat

Hi Balaji

On Wed, 29 Jun 2011, T Krishnamoorthy, Balaji wrote:

> On Wed, Jun 29, 2011 at 2:00 AM, Kevin Hilman <khilman@ti.com> wrote:
> > "T Krishnamoorthy, Balaji" <balajitk@ti.com> writes:
> >>
> >> I have seen some instabilities if delay is very less, on some 
> >> production boards. The previous implementation used 100ms delay 
> >> before disabling the clocks.
> >
> > And your new one is using 50ms.  How did this value come about?
>
> I don't have any specific affinity to this number, but when requests are 
> bursty, they arrive within a few 10s of ms within each other. Didn't 
> want to have the context/save restore penalty associated with every 
> request.

Kevin and I just chatted a little bit about this.  It seems best to 
separate the work done on the autosuspend timeout from the runtime PM 
conversion.  

So how about this: please send a new version of these patches with the 
previous value, 100ms, for the autosuspend timeout.  That should hopefully 
minimize the behavior change here for existing users.  And hopefully we'll 
be able to get the series in for this merge window.

Then later, we need to come back to this autosuspend timeout issue.


- Paul

Re: [PATCH 2/3] MMC: OMAP: HSMMC: add runtime pm support

[S, Venkatraman]

On Thu, Jun 30, 2011 at 6:10 AM, Paul Walmsley <paul@pwsan.com> wrote:
> + Venkat
>
> Hi Balaji
>
> On Wed, 29 Jun 2011, T Krishnamoorthy, Balaji wrote:
>
>> On Wed, Jun 29, 2011 at 2:00 AM, Kevin Hilman <khilman@ti.com> wrote:
>> > "T Krishnamoorthy, Balaji" <balajitk@ti.com> writes:
>> >>
>> >> I have seen some instabilities if delay is very less, on some
>> >> production boards. The previous implementation used 100ms delay
>> >> before disabling the clocks.
>> >
>> > And your new one is using 50ms.  How did this value come about?
>>
>> I don't have any specific affinity to this number, but when requests are
>> bursty, they arrive within a few 10s of ms within each other. Didn't
>> want to have the context/save restore penalty associated with every
>> request.
>
> Kevin and I just chatted a little bit about this.  It seems best to
> separate the work done on the autosuspend timeout from the runtime PM
> conversion.
>
> So how about this: please send a new version of these patches with the
> previous value, 100ms, for the autosuspend timeout.  That should hopefully
> minimize the behavior change here for existing users.  And hopefully we'll
> be able to get the series in for this merge window.
>
> Then later, we need to come back to this autosuspend timeout issue.
>

Ok.

[PATCH 3/3] MMC: OMAP: HSMMC: Remove unused iclk

[Balaji T K]

After runtime conversion to handle clk,
iclk node is not used
However fclk node is still used to get clock rate.

Signed-off-by: Balaji T K <balajitk@ti.com>
---
 drivers/mmc/host/omap_hsmmc.c |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 5b81d8b..afcca36 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -145,7 +145,6 @@ struct omap_hsmmc_host {
 	struct	mmc_command	*cmd;
 	struct	mmc_data	*data;
 	struct	clk		*fclk;
-	struct	clk		*iclk;
 	struct	clk		*dbclk;
 	/*
 	 * vcc == configured supply
@@ -1855,17 +1854,10 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev)
 
 	spin_lock_init(&host->irq_lock);
 
-	host->iclk = clk_get(&pdev->dev, "ick");
-	if (IS_ERR(host->iclk)) {
-		ret = PTR_ERR(host->iclk);
-		host->iclk = NULL;
-		goto err1;
-	}
 	host->fclk = clk_get(&pdev->dev, "fck");
 	if (IS_ERR(host->fclk)) {
 		ret = PTR_ERR(host->fclk);
 		host->fclk = NULL;
-		clk_put(host->iclk);
 		goto err1;
 	}
 
@@ -2025,7 +2017,6 @@ err_irq:
 	pm_runtime_mark_last_busy(host->dev);
 	pm_runtime_put_autosuspend(host->dev);
 	clk_put(host->fclk);
-	clk_put(host->iclk);
 	if (host->got_dbclk) {
 		clk_disable(host->dbclk);
 		clk_put(host->dbclk);
@@ -2062,7 +2053,6 @@ static int omap_hsmmc_remove(struct platform_device *pdev)
 		pm_runtime_forbid(host->dev);
 		pm_runtime_disable(host->dev);
 		clk_put(host->fclk);
-		clk_put(host->iclk);
 		if (host->got_dbclk) {
 			clk_disable(host->dbclk);
 			clk_put(host->dbclk);
-- 
1.7.0.4

Re: [PATCH 3/3] MMC: OMAP: HSMMC: Remove unused iclk

[Cousson, Benoit]

On 6/22/2011 4:18 PM, Krishnamoorthy, Balaji T wrote:
> After runtime conversion to handle clk,
> iclk node is not used
> However fclk node is still used to get clock rate.
>
> Signed-off-by: Balaji T K<balajitk@ti.com>
> ---
>   drivers/mmc/host/omap_hsmmc.c |   10 ----------
>   1 files changed, 0 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index 5b81d8b..afcca36 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -145,7 +145,6 @@ struct omap_hsmmc_host {
>   	struct	mmc_command	*cmd;
>   	struct	mmc_data	*data;
>   	struct	clk		*fclk;
> -	struct	clk		*iclk;
>   	struct	clk		*dbclk;
>   	/*
>   	 * vcc == configured supply
> @@ -1855,17 +1854,10 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev)
>
>   	spin_lock_init(&host->irq_lock);
>
> -	host->iclk = clk_get(&pdev->dev, "ick");
> -	if (IS_ERR(host->iclk)) {
> -		ret = PTR_ERR(host->iclk);
> -		host->iclk = NULL;
> -		goto err1;
> -	}
>   	host->fclk = clk_get(&pdev->dev, "fck");
>   	if (IS_ERR(host->fclk)) {
>   		ret = PTR_ERR(host->fclk);
>   		host->fclk = NULL;
> -		clk_put(host->iclk);
>   		goto err1;
>   	}
>
> @@ -2025,7 +2017,6 @@ err_irq:
>   	pm_runtime_mark_last_busy(host->dev);
>   	pm_runtime_put_autosuspend(host->dev);
>   	clk_put(host->fclk);

Since you just have to use the clk_get_rate, you can remove as well all 
the clock related functions for the fck node: clk_put / clk_enable...

> -	clk_put(host->iclk);
>   	if (host->got_dbclk) {
>   		clk_disable(host->dbclk);
>   		clk_put(host->dbclk);
> @@ -2062,7 +2053,6 @@ static int omap_hsmmc_remove(struct platform_device *pdev)
>   		pm_runtime_forbid(host->dev);
>   		pm_runtime_disable(host->dev);
>   		clk_put(host->fclk);

Here as well.

Regards,
Benoit

Re: [PATCH 3/3] MMC: OMAP: HSMMC: Remove unused iclk

[T Krishnamoorthy, Balaji]

On Wed, Jun 22, 2011 at 9:57 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
> On 6/22/2011 4:18 PM, Krishnamoorthy, Balaji T wrote:
>>
>> After runtime conversion to handle clk,
>> iclk node is not used
>> However fclk node is still used to get clock rate.
>>
>> Signed-off-by: Balaji T K<balajitk@ti.com>
>> ---
>>  drivers/mmc/host/omap_hsmmc.c |   10 ----------
>>  1 files changed, 0 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
>> index 5b81d8b..afcca36 100644
>> --- a/drivers/mmc/host/omap_hsmmc.c
>> +++ b/drivers/mmc/host/omap_hsmmc.c
>> @@ -145,7 +145,6 @@ struct omap_hsmmc_host {
>>        struct  mmc_command     *cmd;
>>        struct  mmc_data        *data;
>>        struct  clk             *fclk;
>> -       struct  clk             *iclk;
>>        struct  clk             *dbclk;
>>        /*
>>         * vcc == configured supply
>> @@ -1855,17 +1854,10 @@ static int __init omap_hsmmc_probe(struct
>> platform_device *pdev)
>>
>>        spin_lock_init(&host->irq_lock);
>>
>> -       host->iclk = clk_get(&pdev->dev, "ick");
>> -       if (IS_ERR(host->iclk)) {
>> -               ret = PTR_ERR(host->iclk);
>> -               host->iclk = NULL;
>> -               goto err1;
>> -       }
>>        host->fclk = clk_get(&pdev->dev, "fck");
>>        if (IS_ERR(host->fclk)) {
>>                ret = PTR_ERR(host->fclk);
>>                host->fclk = NULL;
>> -               clk_put(host->iclk);
>>                goto err1;
>>        }
>>
>> @@ -2025,7 +2017,6 @@ err_irq:
>>        pm_runtime_mark_last_busy(host->dev);
>>        pm_runtime_put_autosuspend(host->dev);
>>        clk_put(host->fclk);
>
> Since you just have to use the clk_get_rate, you can remove as well all the
> clock related functions for the fck node: clk_put / clk_enable...

clk_enable is removed, but would prefer to keep clk_put for balancing clk_get

>
>> -       clk_put(host->iclk);
>>        if (host->got_dbclk) {
>>                clk_disable(host->dbclk);
>>                clk_put(host->dbclk);
>> @@ -2062,7 +2053,6 @@ static int omap_hsmmc_remove(struct platform_device
>> *pdev)
>>                pm_runtime_forbid(host->dev);
>>                pm_runtime_disable(host->dev);
>>                clk_put(host->fclk);
>
> Here as well.
>
> Regards,
> Benoit
>

Re: [PATCH 0/3] OMAP: HSMMC: cleanup and runtime pm

[Cousson, Benoit]

Hi Balaji,

On 6/22/2011 4:18 PM, Krishnamoorthy, Balaji T wrote:
> Removing the custom state machine - lazy disable framework in omap_hsmmc
> to make way for runtime pm to handle host controller
> power states.
> This allows mmc_host_enable/mmc_host_disable to be replaced by
> runtime get_sync and put_sync at host controller driver.
>
> Enable runtime PM in omap_hsmmc

That's a good news, I was struggling with some hwmod cleanup due to the 
lack of pm_runtime support for the MMC driver.
I'll try that series without the hacks I had to do on top of my cleanup.

Thanks,
Benoit