[PATCH] mmc: mmci: Improve runtime PM support

[PATCH] mmc: mmci: Improve runtime PM support

[Ulf Hansson]

Runtime PM support is now dynamically controling the
enable/disable of the clock and vcore regulator.

In runtime_suspend the register values are saved and in
runtime_resume register values are restored. We also make
use of the runtime_autosuspend with a timeout value
set to 50 ms.

Moreover a runtime_idle function is implemented to be able
to enter runtime suspend state after probe and when no
requests are recieved from the mmc framework (due to that
there are no card inserted).

Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
---
 drivers/mmc/host/mmci.c |  154 ++++++++++++++++++++++++++++++++++++++---------
 drivers/mmc/host/mmci.h |    6 ++-
 2 files changed, 131 insertions(+), 29 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 50b5f99..571834a 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -166,14 +166,10 @@ mmci_request_end(struct mmci_host *host, struct mmc_request *mrq)
 	host->mrq = NULL;
 	host->cmd = NULL;
 
-	/*
-	 * Need to drop the host lock here; mmc_request_done may call
-	 * back into the driver...
-	 */
-	spin_unlock(&host->lock);
-	pm_runtime_put(mmc_dev(host->mmc));
 	mmc_request_done(host->mmc, mrq);
-	spin_lock(&host->lock);
+
+	pm_runtime_mark_last_busy(host->mmc->parent);
+	pm_runtime_put_autosuspend(host->mmc->parent);
 }
 
 static void mmci_set_mask1(struct mmci_host *host, unsigned int mask)
@@ -986,7 +982,7 @@ static void mmci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 		return;
 	}
 
-	pm_runtime_get_sync(mmc_dev(mmc));
+	pm_runtime_get_sync(mmc->parent);
 
 	spin_lock_irqsave(&host->lock, flags);
 
@@ -1010,6 +1006,8 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	unsigned long flags;
 	int ret;
 
+	pm_runtime_get_sync(mmc->parent);
+
 	switch (ios->power_mode) {
 	case MMC_POWER_OFF:
 		if (host->vcc)
@@ -1058,12 +1056,15 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 
 	mmci_set_clkreg(host, ios->clock);
 
-	if (host->pwr != pwr) {
-		host->pwr = pwr;
+	if (host->pwr_reg != pwr) {
+		host->pwr_reg = pwr;
 		writel(pwr, host->base + MMCIPOWER);
 	}
 
 	spin_unlock_irqrestore(&host->lock, flags);
+
+	pm_runtime_mark_last_busy(mmc->parent);
+	pm_runtime_put_autosuspend(mmc->parent);
 }
 
 static int mmci_get_ro(struct mmc_host *mmc)
@@ -1147,6 +1148,9 @@ static int __devinit mmci_probe(struct amba_device *dev,
 	host->gpio_wp = -ENOSYS;
 	host->gpio_cd = -ENOSYS;
 	host->gpio_cd_irq = -1;
+	host->irqmask0_reg = 0;
+	host->pwr_reg = 0;
+	host->clk_reg = 0;
 
 	host->hw_designer = amba_manf(dev);
 	host->hw_revision = amba_rev(dev);
@@ -1324,6 +1328,7 @@ static int __devinit mmci_probe(struct amba_device *dev,
 			goto irq0_free;
 	}
 
+	host->irqmask0_reg = MCI_IRQENABLE;
 	writel(MCI_IRQENABLE, host->base + MMCIMASK0);
 
 	amba_set_drvdata(dev, mmc);
@@ -1335,7 +1340,9 @@ static int __devinit mmci_probe(struct amba_device *dev,
 
 	mmci_dma_setup(host);
 
-	pm_runtime_put(&dev->dev);
+	pm_runtime_set_autosuspend_delay(mmc->parent, 50);
+	pm_runtime_use_autosuspend(mmc->parent);
+	pm_runtime_put(mmc->parent);
 
 	mmc_add_host(mmc);
 
@@ -1377,10 +1384,11 @@ static int __devexit mmci_remove(struct amba_device *dev)
 		struct mmci_host *host = mmc_priv(mmc);
 
 		/*
-		 * Undo pm_runtime_put() in probe.  We use the _sync
-		 * version here so that we can access the primecell.
+		 * Make sure the host is resumed and undo the
+		 * pm_runtime_put in probe.
 		 */
-		pm_runtime_get_sync(&dev->dev);
+		pm_runtime_resume(mmc->parent);
+		pm_runtime_get_noresume(mmc->parent);
 
 		mmc_remove_host(mmc);
 
@@ -1419,43 +1427,134 @@ static int __devexit mmci_remove(struct amba_device *dev)
 	return 0;
 }
 
-#ifdef CONFIG_PM
-static int mmci_suspend(struct amba_device *dev, pm_message_t state)
+#ifdef CONFIG_SUSPEND
+
+#ifdef CONFIG_PM_RUNTIME
+static void mmci_disable_irq(struct mmci_host *host) {}
+static void mmci_enable_irq(struct mmci_host *host) {}
+#else
+static void mmci_disable_irq(struct mmci_host *host)
 {
-	struct mmc_host *mmc = amba_get_drvdata(dev);
+	writel(0, host->base + MMCIMASK0);
+}
+static void mmci_enable_irq(struct mmci_host *host)
+{
+	writel(host->irqmask0_reg, host->base + MMCIMASK0);
+}
+#endif
+
+static int mmci_suspend(struct device *dev)
+{
+	struct amba_device *adev = to_amba_device(dev);
+	struct mmc_host *mmc = amba_get_drvdata(adev);
 	int ret = 0;
 
 	if (mmc) {
 		struct mmci_host *host = mmc_priv(mmc);
 
+		pm_runtime_get_sync(mmc->parent);
+
 		ret = mmc_suspend_host(mmc);
-		if (ret == 0)
-			writel(0, host->base + MMCIMASK0);
+		if (!ret)
+			mmci_disable_irq(host);
+
+		pm_runtime_put_sync_suspend(mmc->parent);
 	}
 
 	return ret;
 }
 
-static int mmci_resume(struct amba_device *dev)
+static int mmci_resume(struct device *dev)
 {
-	struct mmc_host *mmc = amba_get_drvdata(dev);
+	struct amba_device *adev = to_amba_device(dev);
+	struct mmc_host *mmc = amba_get_drvdata(adev);
 	int ret = 0;
 
 	if (mmc) {
 		struct mmci_host *host = mmc_priv(mmc);
 
-		writel(MCI_IRQENABLE, host->base + MMCIMASK0);
-
+		mmci_enable_irq(host);
 		ret = mmc_resume_host(mmc);
 	}
 
 	return ret;
 }
-#else
-#define mmci_suspend	NULL
-#define mmci_resume	NULL
 #endif
 
+#ifdef CONFIG_PM_RUNTIME
+static int mmci_runtime_suspend(struct device *dev)
+{
+	struct amba_device *adev = to_amba_device(dev);
+	struct mmc_host *mmc = amba_get_drvdata(adev);
+	unsigned long flags;
+
+	if (mmc) {
+		struct mmci_host *host = mmc_priv(mmc);
+
+		spin_lock_irqsave(&host->lock, flags);
+
+		/* Save registers for POWER, CLOCK and IRQMASK0 */
+		host->irqmask0_reg = readl(host->base + MMCIMASK0);
+		host->pwr_reg = readl(host->base + MMCIPOWER);
+		host->clk_reg = readl(host->base + MMCICLOCK);
+
+		/*
+		 * Make sure we do not get any interrupts when we disabled the
+		 * clock and the regulator and as well make sure to clear the
+		 * registers for clock and power.
+		 */
+		writel(0, host->base + MMCIMASK0);
+		writel(0, host->base + MMCIPOWER);
+		writel(0, host->base + MMCICLOCK);
+
+		spin_unlock_irqrestore(&host->lock, flags);
+
+		clk_disable(host->clk);
+		amba_vcore_disable(adev);
+	}
+
+	return 0;
+}
+
+static int mmci_runtime_resume(struct device *dev)
+{
+	struct amba_device *adev = to_amba_device(dev);
+	struct mmc_host *mmc = amba_get_drvdata(adev);
+	unsigned long flags;
+
+	if (mmc) {
+		struct mmci_host *host = mmc_priv(mmc);
+
+		amba_vcore_enable(adev);
+		clk_enable(host->clk);
+
+		spin_lock_irqsave(&host->lock, flags);
+
+		/* Restore registers for POWER, CLOCK and IRQMASK0 */
+		writel(host->clk_reg, host->base + MMCICLOCK);
+		writel(host->pwr_reg, host->base + MMCIPOWER);
+		writel(host->irqmask0_reg, host->base + MMCIMASK0);
+
+		spin_unlock_irqrestore(&host->lock, flags);
+	}
+
+	return 0;
+}
+
+static int mmci_runtime_idle(struct device *dev)
+{
+	pm_runtime_suspend(dev);
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops mmci_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(mmci_suspend, mmci_resume)
+	SET_RUNTIME_PM_OPS(mmci_runtime_suspend,
+			   mmci_runtime_resume,
+			   mmci_runtime_idle)
+};
+
 static struct amba_id mmci_ids[] = {
 	{
 		.id	= 0x00041180,
@@ -1499,11 +1598,10 @@ static struct amba_id mmci_ids[] = {
 static struct amba_driver mmci_driver = {
 	.drv		= {
 		.name	= DRIVER_NAME,
+		.pm	= &mmci_dev_pm_ops,
 	},
 	.probe		= mmci_probe,
 	.remove		= __devexit_p(mmci_remove),
-	.suspend	= mmci_suspend,
-	.resume		= mmci_resume,
 	.id_table	= mmci_ids,
 };
 
diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
index 79e4143..4ae0f84 100644
--- a/drivers/mmc/host/mmci.h
+++ b/drivers/mmc/host/mmci.h
@@ -189,7 +189,6 @@ struct mmci_host {
 
 	unsigned int		mclk;
 	unsigned int		cclk;
-	u32			pwr;
 	struct mmci_platform_data *plat;
 	struct variant_data	*variant;
 
@@ -199,6 +198,11 @@ struct mmci_host {
 	struct timer_list	timer;
 	unsigned int		oldstat;
 
+	/* register cache */
+	u32			irqmask0_reg;
+	u32			pwr_reg;
+	u32			clk_reg;
+
 	/* pio stuff */
 	struct sg_mapping_iter	sg_miter;
 	unsigned int		size;
-- 
1.7.5.4

[PATCH] mmc: mmci: Improve runtime PM support

[Russell King - ARM Linux]

On Fri, Oct 21, 2011 at 05:25:54PM +0200, Ulf Hansson wrote:
> +#ifdef CONFIG_PM_RUNTIME
> +static int mmci_runtime_suspend(struct device *dev)
> +{
> +	struct amba_device *adev = to_amba_device(dev);
> +	struct mmc_host *mmc = amba_get_drvdata(adev);
> +	unsigned long flags;
> +
> +	if (mmc) {
> +		struct mmci_host *host = mmc_priv(mmc);
> +
> +		spin_lock_irqsave(&host->lock, flags);
> +
> +		/* Save registers for POWER, CLOCK and IRQMASK0 */
> +		host->irqmask0_reg = readl(host->base + MMCIMASK0);
> +		host->pwr_reg = readl(host->base + MMCIPOWER);
> +		host->clk_reg = readl(host->base + MMCICLOCK);
> +
> +		/*
> +		 * Make sure we do not get any interrupts when we disabled the
> +		 * clock and the regulator and as well make sure to clear the
> +		 * registers for clock and power.
> +		 */
> +		writel(0, host->base + MMCIMASK0);
> +		writel(0, host->base + MMCIPOWER);
> +		writel(0, host->base + MMCICLOCK);

Err, no.  You're not allowed to power down the card between commands
unless the card has been removed or been has finished with.

If you power down the card (which you _are_ doing by writing zero to
the MMCIPOWER register), then you have to do a full setup of the card
when you resume.

This is completely unsuitable for runtime-PM usage.

[PATCH] mmc: mmci: Improve runtime PM support

[Sebastian Rasmussen]

Hi!

> Err, no.  You're not allowed to power down the card between commands
> unless the card has been removed or been has finished with.
>
> If you power down the card (which you _are_ doing by writing zero to
> the MMCIPOWER register), then you have to do a full setup of the card
> when you resume.

MCIPower is according to ARM PL180 TRM signalling to an external power
supply to turn on/off (MCIPWR), whether to use open-drain (MCIROD),
what voltage to use (MCIVDD) and whether the card is clocked (MCICLK).

According ST-Ericsson's public PL180 derivative spec[1] it seems to work
roughly in same way (but renaming the register SDI_PWR and the signals
SDIPWR & SDICLK). However, there is no SDIVDD as the derivative can not
signal desired voltage level externally (there are no bits in SDI_PWR for this).
This makes it plausible that SDIPWR may not be routed externally either.
Can you verify this as there are no signal routing diagrams in the spec..?

This leads me to believe that writing 0 to SDI_PWR/MMC in actual practice
doesn't really do much but disabling the clock to the card (and for
ST-Ericsson's PL180, disable direction signalling to the external level
shifter). Clearing bit 8 of MCIClock/SDI_CLKCR also disables the clock.

I guess the patch would appeal more to Russell if mmci_runtime_suspend()
only cleared MCIMask0/SDI_MASK0 and MCIClock/SDI_CLKCR and left
MCIPower/SDI_PWR unchanged. It may be the case that the signal direction
bits need to be cleared for the ST-Ericsson PL180, but I haven't yet verified
this on my Snowball dev board yet.

 / Sebastian

[1] http://www.stericsson.com/developers/DM00030004_AP9500_reference_manual_rev1.pdf

[PATCH] mmc: mmci: Improve runtime PM support

[Ulf Hansson]

Sebastian Rasmussen wrote:
> Hi!
> 
>> Err, no.  You're not allowed to power down the card between commands
>> unless the card has been removed or been has finished with.
>>
>> If you power down the card (which you _are_ doing by writing zero to
>> the MMCIPOWER register), then you have to do a full setup of the card
>> when you resume.
> 
> MCIPower is according to ARM PL180 TRM signalling to an external power
> supply to turn on/off (MCIPWR), whether to use open-drain (MCIROD),
> what voltage to use (MCIVDD) and whether the card is clocked (MCICLK).
> 
> According ST-Ericsson's public PL180 derivative spec[1] it seems to work
> roughly in same way (but renaming the register SDI_PWR and the signals
> SDIPWR & SDICLK). However, there is no SDIVDD as the derivative can not
> signal desired voltage level externally (there are no bits in SDI_PWR for this).
> This makes it plausible that SDIPWR may not be routed externally either.
> Can you verify this as there are no signal routing diagrams in the spec..?
> 

The hole idea with this PM patch is to make sure the vcore regulator and 
the clock are disabled in runtime_suspend to be able to save a huge 
amount of current in "idle" mode.

Disabling the vcore regulator will sooner or later (depending on your 
regulator tree) mean that that power to the controller is actually cut, 
which then means that all registers will be "cleared" including the 
MCIPWR. So the actual reason for clearing the registers in the 
runtime_suspend function is because of two reasons.

1. Set the controller in a known state so no "magic" things happens when 
we are runtime suspended, for example getting an IRQ.

2. Save power by disabling the clock etc. The actual power to the 
controller does not have to be cut just because we have disabled the 
vore regulator.

If the ARM PL180 TRM prevent us from from doing this kind of operations 
in runtime_suspend, we must think of an alternative solution which just 
apply for ST-Ericssons derivative of PL180. THIS IS VERY IMPORTANT to be 
able to implement a proper power management solution.

Please check this Russell, this is VERY IMPORTANT!


> This leads me to believe that writing 0 to SDI_PWR/MMC in actual practice
> doesn't really do much but disabling the clock to the card (and for
> ST-Ericsson's PL180, disable direction signalling to the external level
> shifter). Clearing bit 8 of MCIClock/SDI_CLKCR also disables the clock.
> 
> I guess the patch would appeal more to Russell if mmci_runtime_suspend()
> only cleared MCIMask0/SDI_MASK0 and MCIClock/SDI_CLKCR and left
> MCIPower/SDI_PWR unchanged. It may be the case that the signal direction
> bits need to be cleared for the ST-Ericsson PL180, but I haven't yet verified
> this on my Snowball dev board yet.

This is according the comment above not feasible, since the vcore 
regulator to the controller is disabled all registers will be "cleared" 
anyway.

> 
>  / Sebastian
> 
> [1] http://www.stericsson.com/developers/DM00030004_AP9500_reference_manual_rev1.pdf
> 

BR
Ulf Hansson

[PATCH] mmc: mmci: Improve runtime PM support

[Russell King - ARM Linux]

On Mon, Oct 24, 2011 at 10:05:30AM +0200, Ulf Hansson wrote:
> Sebastian Rasmussen wrote:
>> Hi!
>>
>>> Err, no.  You're not allowed to power down the card between commands
>>> unless the card has been removed or been has finished with.
>>>
>>> If you power down the card (which you _are_ doing by writing zero to
>>> the MMCIPOWER register), then you have to do a full setup of the card
>>> when you resume.
>>
>> MCIPower is according to ARM PL180 TRM signalling to an external power
>> supply to turn on/off (MCIPWR), whether to use open-drain (MCIROD),
>> what voltage to use (MCIVDD) and whether the card is clocked (MCICLK).
>>
>> According ST-Ericsson's public PL180 derivative spec[1] it seems to work
>> roughly in same way (but renaming the register SDI_PWR and the signals
>> SDIPWR & SDICLK). However, there is no SDIVDD as the derivative can not
>> signal desired voltage level externally (there are no bits in SDI_PWR for this).
>> This makes it plausible that SDIPWR may not be routed externally either.
>> Can you verify this as there are no signal routing diagrams in the spec..?
>>
>
> The hole idea with this PM patch is to make sure the vcore regulator and  
> the clock are disabled in runtime_suspend to be able to save a huge  
> amount of current in "idle" mode.
>
> Disabling the vcore regulator will sooner or later (depending on your  
> regulator tree) mean that that power to the controller is actually cut,  
> which then means that all registers will be "cleared" including the  
> MCIPWR. So the actual reason for clearing the registers in the  
> runtime_suspend function is because of two reasons.
>
> 1. Set the controller in a known state so no "magic" things happens when  
> we are runtime suspended, for example getting an IRQ.
>
> 2. Save power by disabling the clock etc. The actual power to the  
> controller does not have to be cut just because we have disabled the  
> vore regulator.
>
> If the ARM PL180 TRM prevent us from from doing this kind of operations  
> in runtime_suspend, we must think of an alternative solution which just  
> apply for ST-Ericssons derivative of PL180. THIS IS VERY IMPORTANT to be  
> able to implement a proper power management solution.
>
> Please check this Russell, this is VERY IMPORTANT!

I repeat: if you cut power to the card, you have to re-initialize it.
Re-initialization takes quite a bit of time to re-detect and setup
the card.  You'd also need to re-configure things like the transfer
mode and so forth.

The other problem is if you're doing runtime-pm between commands,
the re-initialization takes multiple commands - we don't want to be
in the middle of a reinitialization while we're also doing something
with runtime-pm - nested initialization would not be a nice problem
to solve.

[PATCH] mmc: mmci: Improve runtime PM support

[Ulf Hansson]

Russell King - ARM Linux wrote:
> On Mon, Oct 24, 2011 at 10:05:30AM +0200, Ulf Hansson wrote:
>> Sebastian Rasmussen wrote:
>>> Hi!
>>>
>>>> Err, no.  You're not allowed to power down the card between commands
>>>> unless the card has been removed or been has finished with.
>>>>
>>>> If you power down the card (which you _are_ doing by writing zero to
>>>> the MMCIPOWER register), then you have to do a full setup of the card
>>>> when you resume.
>>> MCIPower is according to ARM PL180 TRM signalling to an external power
>>> supply to turn on/off (MCIPWR), whether to use open-drain (MCIROD),
>>> what voltage to use (MCIVDD) and whether the card is clocked (MCICLK).
>>>
>>> According ST-Ericsson's public PL180 derivative spec[1] it seems to work
>>> roughly in same way (but renaming the register SDI_PWR and the signals
>>> SDIPWR & SDICLK). However, there is no SDIVDD as the derivative can not
>>> signal desired voltage level externally (there are no bits in SDI_PWR for this).
>>> This makes it plausible that SDIPWR may not be routed externally either.
>>> Can you verify this as there are no signal routing diagrams in the spec..?
>>>
>> The hole idea with this PM patch is to make sure the vcore regulator and  
>> the clock are disabled in runtime_suspend to be able to save a huge  
>> amount of current in "idle" mode.
>>
>> Disabling the vcore regulator will sooner or later (depending on your  
>> regulator tree) mean that that power to the controller is actually cut,  
>> which then means that all registers will be "cleared" including the  
>> MCIPWR. So the actual reason for clearing the registers in the  
>> runtime_suspend function is because of two reasons.
>>
>> 1. Set the controller in a known state so no "magic" things happens when  
>> we are runtime suspended, for example getting an IRQ.
>>
>> 2. Save power by disabling the clock etc. The actual power to the  
>> controller does not have to be cut just because we have disabled the  
>> vore regulator.
>>
>> If the ARM PL180 TRM prevent us from from doing this kind of operations  
>> in runtime_suspend, we must think of an alternative solution which just  
>> apply for ST-Ericssons derivative of PL180. THIS IS VERY IMPORTANT to be  
>> able to implement a proper power management solution.
>>
>> Please check this Russell, this is VERY IMPORTANT!
> 
> I repeat: if you cut power to the card, you have to re-initialize it.
> Re-initialization takes quite a bit of time to re-detect and setup
> the card.  You'd also need to re-configure things like the transfer
> mode and so forth.

Right now host->vcc (vmmc) regulator is controlling the power to card. 
Not the MCIPWR register!

It is possible for ARM PL180 trm (not for STE version) to use MCIPWR to 
control some external regulator. But right now, this is not used 
according the code and it also feels like a very strange setup.

I would be very surprised if any hardware has this kind of setup, that 
the PL180 itself controls a regulator.

> 
> The other problem is if you're doing runtime-pm between commands,
> the re-initialization takes multiple commands - we don't want to be
> in the middle of a reinitialization while we're also doing something
> with runtime-pm - nested initialization would not be a nice problem
> to solve.
> 

You do not have to re-init the card as long at is still powered. It is 
just a matter of restoring register values. This is really quick. But at 
the same time, we do not want to do it for every request thus we use 
timeout of 50 ms.

[PATCH] mmc: mmci: Improve runtime PM support

[Russell King - ARM Linux]

On Mon, Oct 24, 2011 at 11:36:01AM +0200, Ulf Hansson wrote:
> Russell King - ARM Linux wrote:
>> I repeat: if you cut power to the card, you have to re-initialize it.
>> Re-initialization takes quite a bit of time to re-detect and setup
>> the card.  You'd also need to re-configure things like the transfer
>> mode and so forth.
>
> Right now host->vcc (vmmc) regulator is controlling the power to card.  
> Not the MCIPWR register!

Maybe for you, but that's not the case on all platforms.

You *really* need to get out of the idea that just because your
implementation works one way that everything works that way.  You're
working on a cross-SoC cross-platform driver, and you need to take
account of how other platforms work.

In that case, there *are* platforms which the MCIPWR register does
indeed control power to the card - and setting this to zero _will_
power down the card.

> I would be very surprised if any hardware has this kind of setup, that  
> the PL180 itself controls a regulator.

ARM dev boards all use the MCIPWR bits to control an external power
switch - there's no adjustment of the voltage except via soldered
links on the board.

[PATCH] mmc: mmci: Improve runtime PM support

[Ulf Hansson]

Russell King - ARM Linux wrote:
> On Mon, Oct 24, 2011 at 11:36:01AM +0200, Ulf Hansson wrote:
>> Russell King - ARM Linux wrote:
>>> I repeat: if you cut power to the card, you have to re-initialize it.
>>> Re-initialization takes quite a bit of time to re-detect and setup
>>> the card.  You'd also need to re-configure things like the transfer
>>> mode and so forth.
>> Right now host->vcc (vmmc) regulator is controlling the power to card.  
>> Not the MCIPWR register!
> 
> Maybe for you, but that's not the case on all platforms.
> 
> You *really* need to get out of the idea that just because your
> implementation works one way that everything works that way.  You're
> working on a cross-SoC cross-platform driver, and you need to take
> account of how other platforms work.

Sorry for being a pain in the ass. :-) I am thinking of the above, even 
if it not seems like that.

> 
> In that case, there *are* platforms which the MCIPWR register does
> indeed control power to the card - and setting this to zero _will_
> power down the card.
> 
>> I would be very surprised if any hardware has this kind of setup, that  
>> the PL180 itself controls a regulator.
> 
> ARM dev boards all use the MCIPWR bits to control an external power
> switch - there's no adjustment of the voltage except via soldered
> links on the board.

That explains it!

My believe was that since the bits for controlling the voltage levels 
etc was not used (which is replaced with directions bits for 
ST-version), this functionality was not used either.

Could you maybe elaborate a bit of how the power is controlled in the 
ARM dev boards? Would it be possible to control such a switch in "GPIO" 
manner instead? For example via the vdd_handler or similar?

I will also think if and see if is feasible to re-design and see if this 
hole feature can be controlled by the variant struct instead. My feeling 
is although it can be kind of messy. But let's see...

BR
Ulf Hansson

[PATCH] mmc: mmci: Improve runtime PM support

[Russell King - ARM Linux]

On Mon, Oct 24, 2011 at 12:06:27PM +0200, Ulf Hansson wrote:
> Russell King - ARM Linux wrote:
>> On Mon, Oct 24, 2011 at 11:36:01AM +0200, Ulf Hansson wrote:
>>> Russell King - ARM Linux wrote:
>>>> I repeat: if you cut power to the card, you have to re-initialize it.
>>>> Re-initialization takes quite a bit of time to re-detect and setup
>>>> the card.  You'd also need to re-configure things like the transfer
>>>> mode and so forth.
>>> Right now host->vcc (vmmc) regulator is controlling the power to 
>>> card.  Not the MCIPWR register!
>>
>> Maybe for you, but that's not the case on all platforms.
>>
>> You *really* need to get out of the idea that just because your
>> implementation works one way that everything works that way.  You're
>> working on a cross-SoC cross-platform driver, and you need to take
>> account of how other platforms work.
>
> Sorry for being a pain in the ass. :-) I am thinking of the above, even  
> if it not seems like that.
>
>>
>> In that case, there *are* platforms which the MCIPWR register does
>> indeed control power to the card - and setting this to zero _will_
>> power down the card.
>>
>>> I would be very surprised if any hardware has this kind of setup, 
>>> that  the PL180 itself controls a regulator.
>>
>> ARM dev boards all use the MCIPWR bits to control an external power
>> switch - there's no adjustment of the voltage except via soldered
>> links on the board.
>
> That explains it!
>
> My believe was that since the bits for controlling the voltage levels  
> etc was not used (which is replaced with directions bits for  
> ST-version), this functionality was not used either.
>
> Could you maybe elaborate a bit of how the power is controlled in the  
> ARM dev boards? Would it be possible to control such a switch in "GPIO"  
> manner instead? For example via the vdd_handler or similar?
>
> I will also think if and see if is feasible to re-design and see if this  
> hole feature can be controlled by the variant struct instead. My feeling  
> is although it can be kind of messy. But let's see...

The MCIPWR register contains two bits to control _the_ _power_ _state_.
See Table A-4: the MCIPWR signal.  See section 2.2.2.  See section 3.3.1.

The MCIPWR signal controls the external power switch.  This is the only
signal for it.  This is the only connection for it.  There is no other
control form for this power switch.

[PATCH] mmc: mmci: Improve runtime PM support

[Ulf Hansson]

>> Could you maybe elaborate a bit of how the power is controlled in the  
>> ARM dev boards? Would it be possible to control such a switch in "GPIO"  
>> manner instead? For example via the vdd_handler or similar?
>>
>> I will also think if and see if is feasible to re-design and see if this  
>> hole feature can be controlled by the variant struct instead. My feeling  
>> is although it can be kind of messy. But let's see...
> 
> The MCIPWR register contains two bits to control _the_ _power_ _state_.
> See Table A-4: the MCIPWR signal.  See section 2.2.2.  See section 3.3.1.
> 
> The MCIPWR signal controls the external power switch.  This is the only
> signal for it.  This is the only connection for it.  There is no other
> control form for this power switch.
> 

Then we are only left to use the variant struct I believe. In principle, 
a flag in the variant struct, could indicate whether it is OK to disable 
the vcore regulator and thus clear the MCIPWR when doing runtime_suspend.

How do you feel about this kind of approach to find a solution?

BR
Ulf Hansson

[PATCH] mmc: mmci: Improve runtime PM support

[Linus Walleij]

On Mon, Oct 24, 2011 at 1:48 PM, Ulf Hansson <ulf.hansson@stericsson.com> wrote:
> [Russell]
>> The MCIPWR signal controls the external power switch. ?This is the only
>> signal for it. ?This is the only connection for it. ?There is no other
>> control form for this power switch.
>>
>
> Then we are only left to use the variant struct I believe. In principle, a
> flag in the variant struct, could indicate whether it is OK to disable the
> vcore regulator and thus clear the MCIPWR when doing runtime_suspend.

Yep I think the best could be to add some variant named
bool external_card_power; and then document in the kerneldoc that this
means the driver can clear MMCIPWR without risk of cutting the power
to the card.

This should be true for Ux500, U300 and Nomadik (just checked the designs -
they all have external regulators).

So the state save/restore and amba_vcore_disable(adev); should be done
only for those variants.

However this:

clk_disable(host->clk);

We ought to be able to do for *all* variants, provided we can create
pm_runtime_get/put and delay properly to cover all bus traffic
(looks like the patch already does that), plus all the time the card is
signalling busy. The best I can think of is to just return -EBUSY to runtime
PM like this:

if ((readl(base + MMCISTATUS) & (MCI_CMDACTIVE | MCI_TXACTIVE| MCI_RXACTIVE))
  return -EBUSY;

Yours,
Linus Walleij

[PATCH] mmc: mmci: Improve runtime PM support

[Ulf Hansson]

Linus Walleij wrote:
> On Mon, Oct 24, 2011 at 1:48 PM, Ulf Hansson <ulf.hansson@stericsson.com> wrote:
>> [Russell]
>>> The MCIPWR signal controls the external power switch.  This is the only
>>> signal for it.  This is the only connection for it.  There is no other
>>> control form for this power switch.
>>>
>> Then we are only left to use the variant struct I believe. In principle, a
>> flag in the variant struct, could indicate whether it is OK to disable the
>> vcore regulator and thus clear the MCIPWR when doing runtime_suspend.
> 
> Yep I think the best could be to add some variant named
> bool external_card_power; and then document in the kerneldoc that this
> means the driver can clear MMCIPWR without risk of cutting the power
> to the card.
> 
> This should be true for Ux500, U300 and Nomadik (just checked the designs -
> they all have external regulators).
> 

Could we assume that all boards which utilizes the ARM PL180 are using 
the MMCIPWR register to control power the card? Or should we add a new 
amba mmci platform member so this is configurable for each board?

An option could also be if we might want to simplify code to just skip 
the entire runtime_suspend|idle|resume function (ie stubb it or 
something) for these kind of boards?

What do you prefer?

> So the state save/restore and amba_vcore_disable(adev); should be done
> only for those variants.
> 
> However this:
> 
> clk_disable(host->clk);
> 
> We ought to be able to do for *all* variants, provided we can create
> pm_runtime_get/put and delay properly to cover all bus traffic
> (looks like the patch already does that), plus all the time the card is
> signalling busy. The best I can think of is to just return -EBUSY to runtime
> PM like this:
> 
> if ((readl(base + MMCISTATUS) & (MCI_CMDACTIVE | MCI_TXACTIVE| MCI_RXACTIVE))
>   return -EBUSY;
> 
> Yours,
> Linus Walleij
> 

[PATCH] mmc: mmci: Improve runtime PM support

[Ulf Hansson]

Ulf Hansson wrote:
> Linus Walleij wrote:
>> On Mon, Oct 24, 2011 at 1:48 PM, Ulf Hansson <ulf.hansson@stericsson.com> wrote:
>>> [Russell]
>>>> The MCIPWR signal controls the external power switch.  This is the only
>>>> signal for it.  This is the only connection for it.  There is no other
>>>> control form for this power switch.
>>>>
>>> Then we are only left to use the variant struct I believe. In principle, a
>>> flag in the variant struct, could indicate whether it is OK to disable the
>>> vcore regulator and thus clear the MCIPWR when doing runtime_suspend.
>> Yep I think the best could be to add some variant named
>> bool external_card_power; and then document in the kerneldoc that this
>> means the driver can clear MMCIPWR without risk of cutting the power
>> to the card.
>>
>> This should be true for Ux500, U300 and Nomadik (just checked the designs -
>> they all have external regulators).
>>
> 
> Could we assume that all boards which utilizes the ARM PL180 are using 
> the MMCIPWR register to control power the card? Or should we add a new 
> amba mmci platform member so this is configurable for each board?
> 
> An option could also be if we might want to simplify code to just skip 
> the entire runtime_suspend|idle|resume function (ie stubb it or 
> something) for these kind of boards?
> 
> What do you prefer?

By the way, there is also another option. In the runtime_suspend 
function for ARM PL180 block we can use mmc_power_save_host (and then 
also disable vcore etc) and vice verse in runtime_resume with 
mmc_power_restore_host.

Of course, these kind of operations takes quite some time to execute and 
therefore we use a much bigger timeout than 50 ms (for example 10 s 
instead) for ARM PL180.

Just an idea...

> 
>> So the state save/restore and amba_vcore_disable(adev); should be done
>> only for those variants.
>>
>> However this:
>>
>> clk_disable(host->clk);
>>
>> We ought to be able to do for *all* variants, provided we can create
>> pm_runtime_get/put and delay properly to cover all bus traffic
>> (looks like the patch already does that), plus all the time the card is
>> signalling busy. The best I can think of is to just return -EBUSY to runtime
>> PM like this:
>>
>> if ((readl(base + MMCISTATUS) & (MCI_CMDACTIVE | MCI_TXACTIVE| MCI_RXACTIVE))
>>   return -EBUSY;
>>
>> Yours,
>> Linus Walleij
>>
> 
> 

[PATCH] mmc: mmci: Improve runtime PM support

[Linus Walleij]

2011/10/24 Ulf Hansson <ulf.hansson@stericsson.com>:

> Could we assume that all boards which utilizes the ARM PL180 are using the
> MMCIPWR register to control power the card? Or should we add a new amba mmci
> platform member so this is configurable for each board?

I think we should take entire variants, like currently U300 and Ux500.
In those designs the MMCIPWR signal is left dangling on the chip,
so no-one electronics designer can use that signal for anything even
if s/he would have wanted to.

> An option could also be if we might want to simplify code to just skip the
> entire runtime_suspend|idle|resume function (ie stubb it or something) for
> these kind of boards?

I think the clk_disable()/clk_enable() pair in runtime suspend/resume
is still valuable for all other variants too. That has nothing to do with
MMCIPWR.

> By the way, there is also another option. In the runtime_suspend function
> for ARM PL180 block we can use mmc_power_save_host (and then also
> disable vcore etc) and vice verse in runtime_resume with
> mmc_power_restore_host.

This is what the OMAP driver does right?

> Of course, these kind of operations takes quite some time to execute and
> therefore we use a much bigger timeout than 50 ms (for example 10 s
> instead) for ARM PL180.

Can we do both?

1) Break out mmci_state_save_restore() from the runtime PM hooks.

2) I.e. have a short time-out that will runtime-suspend the external
  regulator variants quickly with finer granularity.

3) Have a long time-out that will runtime-suspend the MMCIPWR
  variants using mmc_power_save_host() with a longer delay?
  Since mmc_power_save_host() calls .set_ios() to really shut
  down the power to the card we should be on the safe side.

Looks like a nice silver bullet to me :-)

I volunteer to test the long time-out code path on the Integrator
PB1176 to verify this works.

Yours,
Linus Walleij

[PATCH] mmc: mmci: Improve runtime PM support

[Ulf Hansson]

Linus Walleij wrote:
> 2011/10/24 Ulf Hansson <ulf.hansson@stericsson.com>:
> 
>> Could we assume that all boards which utilizes the ARM PL180 are using the
>> MMCIPWR register to control power the card? Or should we add a new amba mmci
>> platform member so this is configurable for each board?
> 
> I think we should take entire variants, like currently U300 and Ux500.
> In those designs the MMCIPWR signal is left dangling on the chip,
> so no-one electronics designer can use that signal for anything even
> if s/he would have wanted to.
> 
>> An option could also be if we might want to simplify code to just skip the
>> entire runtime_suspend|idle|resume function (ie stubb it or something) for
>> these kind of boards?
> 
> I think the clk_disable()/clk_enable() pair in runtime suspend/resume
> is still valuable for all other variants too. That has nothing to do with
> MMCIPWR.
> 
>> By the way, there is also another option. In the runtime_suspend function
>> for ARM PL180 block we can use mmc_power_save_host (and then also
>> disable vcore etc) and vice verse in runtime_resume with
>> mmc_power_restore_host.
> 
> This is what the OMAP driver does right?

Nope.

Right now I think it is only SDIO function drivers through pm_runtime 
that can trigger the use of these functions though the bus registered 
runtime functions.

> 
>> Of course, these kind of operations takes quite some time to execute and
>> therefore we use a much bigger timeout than 50 ms (for example 10 s
>> instead) for ARM PL180.
> 
> Can we do both?

That is possible! But I think we need confirmation from Russell first.

Using mmc_power_save_host/mmc_power_restore_host could be a way forward 
for ARM PL180, but you should note that initializing a card after it has 
been fully powered down, can for really crappy SD-cards take up to 1 s. 
Normally this time is 100-400 ms.

This initialization time will then be the latency for the first 
read/write after the runtime timeout has expired. There is always a cost 
in "performance" when doing power save it seems. :-)

> 
> 1) Break out mmci_state_save_restore() from the runtime PM hooks.
> 
> 2) I.e. have a short time-out that will runtime-suspend the external
>   regulator variants quickly with finer granularity.
> 
> 3) Have a long time-out that will runtime-suspend the MMCIPWR
>   variants using mmc_power_save_host() with a longer delay?
>   Since mmc_power_save_host() calls .set_ios() to really shut
>   down the power to the card we should be on the safe side.
> 
> Looks like a nice silver bullet to me :-)
> 
> I volunteer to test the long time-out code path on the Integrator
> PB1176 to verify this works.
> 
> Yours,
> Linus Walleij
> 

[PATCH] mmc: mmci: Improve runtime PM support

[Sebastian Rasmussen]

Hi!

> If the ARM PL180 TRM prevent us from from doing this kind of operations in
> runtime_suspend, we must think of an alternative solution which just apply
> for ST-Ericssons derivative of PL180. THIS IS VERY IMPORTANT to be able to
> implement a proper power management solution.

I believe that what it boils down to is how the power regulators are controlled.

In ST-Ericssons PL180 derivative there must be two independent regulators,
controlled by something outside the PL180 itself, one for the controller and
one for the external card. So if you know what you are doing you can power
down the controller while still keeping the card powered.

In the case of ARM PL180 it seems to me as if there is one power regulator,
controlled by something outside the PL180, that supplies power to the
controller, and one power regulator, controlled by the PL180, that supplies
power to the card. Therefore it is simply not possible to clear the ARM
PL180s MCIPower register (because that means that the card will loose
power since the control signals to the card's power regulator are now asking
it to cut power to the card). In addition I believe that it may not be
possible to
power the controller down...

 / Sebastian

[PATCH] mmc: mmci: Improve runtime PM support

[Linus Walleij]

On Mon, Oct 24, 2011 at 10:05 AM, Ulf Hansson
<ulf.hansson@stericsson.com> wrote:

> The hole idea with this PM patch is to make sure the vcore regulator and the
> clock are disabled in runtime_suspend to be able to save a huge amount of
> current in "idle" mode.
>
> Disabling the vcore regulator will sooner or later (depending on your
> regulator tree) mean that that power to the controller is actually cut,
> which then means that all registers will be "cleared" including the MCIPWR.

I guess since this means writing power-off to bits 1 and 0 of
of MCIPower meaning "power off"  it should not be done unless the MMC
core has first issued mmci_set_ios() with MMC_POWER_OFF set in
ios->power_mode. Which means mmc_power_off() must have
been called.

That in turn is called by mmc_power_save_host() or
mmc_suspend_host().

mmc_power_save_host() is in turn called by
mmc_runtime_suspend() in drivers/mmc/bus.c!

So if that is called *before* the driver-local runtime suspend
calls, you're actually on the safe side, and you can
proceed to cut the power.

So these need to be zero before we enter runtime suspend.
This could then be assured by:

if (readl(host->base + MMCIPOWER) & 3)
  return -EBUSY;

If this has already been done we are on the green and can
runtime suspend the controller I guess?

So what this code

> +             /* Save registers for POWER, CLOCK and IRQMASK0 */
> +             host->irqmask0_reg = readl(host->base + MMCIMASK0);
> +             host->pwr_reg = readl(host->base + MMCIPOWER);
> +             host->clk_reg = readl(host->base + MMCICLOCK);

In the end does is save the contents of the other 30 bits.
(MCI_OD, MCI_ROD, and the ST-specific bits)

So can we first:

- Assure that mmc_power_save_host() has already been
  done (MMCI_POWER_OFF)

- Make this variable a prerequiste for cutting the power in
  runtime_pm?

Yours,
Linus Walleij

[PATCH] mmc: mmci: Improve runtime PM support

[Russell King - ARM Linux]

On Sun, Oct 23, 2011 at 02:31:39AM +0200, Sebastian Rasmussen wrote:
> I guess the patch would appeal more to Russell if mmci_runtime_suspend()
> only cleared MCIMask0/SDI_MASK0 and MCIClock/SDI_CLKCR and left
> MCIPower/SDI_PWR unchanged. It may be the case that the signal direction
> bits need to be cleared for the ST-Ericsson PL180, but I haven't yet verified
> this on my Snowball dev board yet.

There's also the issue that the specs call for the clock to run after
a command has completed for a certain number of cycles, and that the
clock must continue to run until the card reports not-busy after a
programming or erase cycle has completed - that can be long after the
previous command has 'completed'.

[PATCH] mmc: mmci: Improve runtime PM support

[Linus Walleij]

On Mon, Oct 24, 2011 at 11:36 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sun, Oct 23, 2011 at 02:31:39AM +0200, Sebastian Rasmussen wrote:
>> I guess the patch would appeal more to Russell if mmci_runtime_suspend()
>> only cleared MCIMask0/SDI_MASK0 and MCIClock/SDI_CLKCR and left
>> MCIPower/SDI_PWR unchanged. It may be the case that the signal direction
>> bits need to be cleared for the ST-Ericsson PL180, but I haven't yet verified
>> this on my Snowball dev board yet.
>
> There's also the issue that the specs call for the clock to run after
> a command has completed for a certain number of cycles, and that the
> clock must continue to run until the card reports not-busy after a
> programming or erase cycle has completed - that can be long after the
> previous command has 'completed'.

It's 8 cycles on MCLK required.

I think that is taken care of by this:
pm_runtime_set_autosuspend_delay(mmc->parent, 50);

50 ms is >> 8 MCLK in any practical case, but if we want to
be really, really sure we can always:

#define DEFAULT_DELAY 50
int delay;
delay = DIV_ROUND_UP((1000 * 8), host->mclk); /* 8 MCLK in ms */
delay = max(DEFAULT_DELAY, min_delay);
pm_runtime_set_autosuspend_delay(mmc->parent, delay);

So we have encoded all assumptions.

Yours,
Linus Walleij

[PATCH] mmc: mmci: Improve runtime PM support

[Russell King - ARM Linux]

On Mon, Oct 24, 2011 at 11:54:00AM +0200, Linus Walleij wrote:
> On Mon, Oct 24, 2011 at 11:36 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Sun, Oct 23, 2011 at 02:31:39AM +0200, Sebastian Rasmussen wrote:
> >> I guess the patch would appeal more to Russell if mmci_runtime_suspend()
> >> only cleared MCIMask0/SDI_MASK0 and MCIClock/SDI_CLKCR and left
> >> MCIPower/SDI_PWR unchanged. It may be the case that the signal direction
> >> bits need to be cleared for the ST-Ericsson PL180, but I haven't yet verified
> >> this on my Snowball dev board yet.
> >
> > There's also the issue that the specs call for the clock to run after
> > a command has completed for a certain number of cycles, and that the
> > clock must continue to run until the card reports not-busy after a
> > programming or erase cycle has completed - that can be long after the
> > previous command has 'completed'.
> 
> It's 8 cycles on MCLK required.

_Plus_ keeping the clock running while the card is signalling busy.

If you don't clock the card while its signalling busy, it will never
go non-busy (the data line becomes frozen.)

[PATCH] mmc: mmci: Improve runtime PM support

[Ulf Hansson]

Russell King - ARM Linux wrote:
> On Mon, Oct 24, 2011 at 11:54:00AM +0200, Linus Walleij wrote:
>> On Mon, Oct 24, 2011 at 11:36 AM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>> On Sun, Oct 23, 2011 at 02:31:39AM +0200, Sebastian Rasmussen wrote:
>>>> I guess the patch would appeal more to Russell if mmci_runtime_suspend()
>>>> only cleared MCIMask0/SDI_MASK0 and MCIClock/SDI_CLKCR and left
>>>> MCIPower/SDI_PWR unchanged. It may be the case that the signal direction
>>>> bits need to be cleared for the ST-Ericsson PL180, but I haven't yet verified
>>>> this on my Snowball dev board yet.
>>> There's also the issue that the specs call for the clock to run after
>>> a command has completed for a certain number of cycles, and that the
>>> clock must continue to run until the card reports not-busy after a
>>> programming or erase cycle has completed - that can be long after the
>>> previous command has 'completed'.
>> It's 8 cycles on MCLK required.
> 
> _Plus_ keeping the clock running while the card is signalling busy.
> 
> If you don't clock the card while its signalling busy, it will never
> go non-busy (the data line becomes frozen.)
> 

Good point!

Is the aggressive clk gating feature in the mmc framework taking this 
into account as well?

Potentially the framework could do a get_sync/put (in claim/release 
host) to prevent hosts from being runtime disabled. Similar how the 
enable/disable mechanism is working.

[PATCH] mmc: mmci: Improve runtime PM support

[Linus Walleij]

On Mon, Oct 24, 2011 at 12:17 PM, Ulf Hansson
<ulf.hansson@stericsson.com> wrote:
> Russell King - ARM Linux wrote:
>> On Mon, Oct 24, 2011 at 11:54:00AM +0200, Linus Walleij wrote:
>>>
>>> It's 8 cycles on MCLK required.
>>
>> _Plus_ keeping the clock running while the card is signalling busy.
>>
>> If you don't clock the card while its signalling busy, it will never
>> go non-busy (the data line becomes frozen.)
>>
>
> (...)
> Is the aggressive clk gating feature in the mmc framework taking this into
> account as well?

Nope.

That is why frequency 0 is not handled like some special case in the
MMCI driver, which means you can turn it on but nothing spectacularly
power saving will happen. (i.e. clock gating does not gate the clock
on the mmci driver) I did a rough patch to cut the clock
but Russell pointed out the problem here (same as discussed above):
http://marc.info/?l=linux-mmc&m=129146545916794&w=2

I *was* meaning to fix it. However now I feel that since we're
anyway ironing out the painful details of runtime PM, we can
probably use that framework to handle also aggressive clock
gating.

> Potentially the framework could do a get_sync/put (in claim/release host) to
> prevent hosts from being runtime disabled. Similar how the enable/disable
> mechanism is working.

Yep. We just need to make sure the hysteresis/delay value covers
the bus activity with some margin.

Yours,
Linus Walleij

[PATCH] mmc: mmci: Improve runtime PM support

[Adrian Hunter]

On 24/10/11 12:56, Russell King - ARM Linux wrote:
> On Mon, Oct 24, 2011 at 11:54:00AM +0200, Linus Walleij wrote:
>> On Mon, Oct 24, 2011 at 11:36 AM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>> On Sun, Oct 23, 2011 at 02:31:39AM +0200, Sebastian Rasmussen wrote:
>>>> I guess the patch would appeal more to Russell if mmci_runtime_suspend()
>>>> only cleared MCIMask0/SDI_MASK0 and MCIClock/SDI_CLKCR and left
>>>> MCIPower/SDI_PWR unchanged. It may be the case that the signal direction
>>>> bits need to be cleared for the ST-Ericsson PL180, but I haven't yet verified
>>>> this on my Snowball dev board yet.
>>>
>>> There's also the issue that the specs call for the clock to run after
>>> a command has completed for a certain number of cycles, and that the
>>> clock must continue to run until the card reports not-busy after a
>>> programming or erase cycle has completed - that can be long after the
>>> previous command has 'completed'.
>>
>> It's 8 cycles on MCLK required.
> 
> _Plus_ keeping the clock running while the card is signalling busy.
> 
> If you don't clock the card while its signalling busy, it will never
> go non-busy (the data line becomes frozen.)

The spec does say that it is OK to turn off the clock, but it
must be turned back on to release the busy signal.  That will always
happen with the block driver because it always sends a status command
when waiting.

Maybe it is an issue with SDIO but currently SDIO is using
MMC_QUIRK_BROKEN_CLK_GATING by default (although wl1271 is
using it)


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

[PATCH] mmc: mmci: Improve runtime PM support

[Linus Walleij]

On Tue, Oct 25, 2011 at 10:05 AM, Adrian Hunter <adrian.hunter@intel.com> wrote:

> The spec does say that it is OK to turn off the clock, but it
> must be turned back on to release the busy signal. ?That will always
> happen with the block driver because it always sends a status command
> when waiting.

Aha, thanks for clearing this up!

> Maybe it is an issue with SDIO but currently SDIO is using
> MMC_QUIRK_BROKEN_CLK_GATING by default (although wl1271 is
> using it)

IIRC SDIO is supposed to allow their clock to be cut, if it is a
peripheral that needs constant clocking it should have its own
clockline/crystal or whatever, basically.

However most SDIO device engineers just see the MCLK
and like "hey, clock, I'll just lock my PLL onto that sweet
clock line and save the crystal..." violating the spec. In worst
cases they even drive the entire logic inside the device from
the MCLK line and count on it being perpetually enabled.
Atleast that's how I think about it.

Yours,
Linus Walleij