[PATCH v1 1/3]mmc: implemented runtime pm for mmc host

[PATCH v1 1/3]mmc: implemented runtime pm for mmc host

[Chuanxiao Dong]

MMC host need to power ungate host each time before sending a request and
try to power gate host each time after the request is done.

MMC host has a sysfs interface to let user specify the auto suspended delay.
Each slot of the host controller can have a different delay for runtime
suspended.

Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com>
---
 drivers/mmc/core/core.c |   15 +++++++++++++++
 drivers/mmc/core/host.c |   21 +++++++++++++++++++++
 2 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 6625c05..e296c5a 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -133,6 +133,9 @@ void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
 			mrq->done(mrq);
 
 		mmc_host_clk_gate(host);
+
+		/* put the host after using */
+		pm_runtime_put(&host->class_dev);
 	}
 }
 
@@ -150,6 +153,12 @@ mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
 		 mmc_hostname(host), mrq->cmd->opcode,
 		 mrq->cmd->arg, mrq->cmd->flags);
 
+	/*
+	 * Before touch any of host controller registers,
+	 * driver should make sure the host is powered up
+	 */
+	pm_runtime_get_sync(&host->class_dev);
+
 	if (mrq->data) {
 		pr_debug("%s:     blksz %d blocks %d flags %08x "
 			"tsac %d ms nsac %d\n",
@@ -1522,6 +1531,9 @@ void mmc_rescan(struct work_struct *work)
 	if (host->rescan_disable)
 		return;
 
+	/* power up host controller first */
+	pm_runtime_get_sync(&host->class_dev);
+
 	mmc_bus_get(host);
 
 	/*
@@ -1564,6 +1576,9 @@ void mmc_rescan(struct work_struct *work)
 	mmc_release_host(host);
 
  out:
+	/* power off host controller */
+	pm_runtime_put(&host->class_dev);
+
 	if (host->caps & MMC_CAP_NEEDS_POLL)
 		mmc_schedule_delayed_work(&host->detect, HZ);
 }
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index b3ac6c5..a27b811 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -19,6 +19,7 @@
 #include <linux/leds.h>
 #include <linux/slab.h>
 #include <linux/suspend.h>
+#include <linux/pm_runtime.h>
 
 #include <linux/mmc/host.h>
 #include <linux/mmc/card.h>
@@ -28,6 +29,14 @@
 
 #define cls_dev_to_mmc_host(d)	container_of(d, struct mmc_host, class_dev)
 
+static const struct dev_pm_ops host_pm = {
+	SET_RUNTIME_PM_OPS(
+		pm_generic_runtime_suspend,
+		pm_generic_runtime_resume,
+		pm_generic_runtime_idle
+	)
+};
+
 static void mmc_host_classdev_release(struct device *dev)
 {
 	struct mmc_host *host = cls_dev_to_mmc_host(dev);
@@ -37,6 +46,7 @@ static void mmc_host_classdev_release(struct device *dev)
 static struct class mmc_host_class = {
 	.name		= "mmc_host",
 	.dev_release	= mmc_host_classdev_release,
+	.pm = &host_pm,
 };
 
 int mmc_register_host_class(void)
@@ -334,6 +344,15 @@ int mmc_add_host(struct mmc_host *host)
 	if (err)
 		return err;
 
+	/* enable runtime pm */
+	err = pm_runtime_set_active(&host->class_dev);
+	if (err)
+		pr_err("err is %d when active mmc host\n", err);
+	else {
+		pm_runtime_enable(&host->class_dev);
+		pm_runtime_set_autosuspend_delay(&host->class_dev, 100);
+	}
+
 #ifdef CONFIG_DEBUG_FS
 	mmc_add_host_debugfs(host);
 #endif
@@ -363,6 +382,8 @@ void mmc_remove_host(struct mmc_host *host)
 	mmc_remove_host_debugfs(host);
 #endif
 
+	pm_runtime_disable(&host->class_dev);
+
 	device_del(&host->class_dev);
 
 	led_trigger_unregister_simple(host->led);
-- 
1.6.6.1

RE: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host

[Tardy, Pierre]

> MMC host has a sysfs interface to let user specify the auto suspended delay.
> Each slot of the host controller can have a different delay for runtime
> suspended.


>   out:
> +	/* power off host controller */
> +	pm_runtime_put(&host->class_dev);
If you want to use autosuspend feature, then you need to use the function pm_runtime_put_autosuspend().


I'm also worried about this patch not taking in account the 8 clock cycles before shutting down the clock, contrary to the clock gating fw.
If you power down hc, you will also gate the clock. So the sdio protocol might not be strictly followed.


Regards,
Pierre
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

RE: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host

[Dong, Chuanxiao]

> -----Original Message-----
> From: Tardy, Pierre
> Sent: Monday, January 17, 2011 6:59 PM
> To: Dong, Chuanxiao; linux-mmc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; cjb@laptop.org; ohad@wizery.com
> Subject: RE: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host
> 
> 
> > MMC host has a sysfs interface to let user specify the auto suspended delay.
> > Each slot of the host controller can have a different delay for runtime
> > suspended.
> 
> 
> >   out:
> > +	/* power off host controller */
> > +	pm_runtime_put(&host->class_dev);
> If you want to use autosuspend feature, then you need to use the function
> pm_runtime_put_autosuspend().
> 
Eh, that is right. Will fix in the next submission

> 
> I'm also worried about this patch not taking in account the 8 clock cycles before
> shutting down the clock, contrary to the clock gating fw.
> If you power down hc, you will also gate the clock. So the sdio protocol might not be
> strictly followed.
As mmc spec said, 8 clock cycles is needed. That is right.
But I think if we have set the auto suspended delay for host, runtime pm core could make sure host to wait at least the auto suspended delay before shutting down the power of the device. So if the auto suspended delay is longer than 8 clock cycles, the mmc protocol can be followed, right?

Thanks
Chuanxiao

RE: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host

[Tardy, Pierre]

> > I'm also worried about this patch not taking in account the 8 clock cycles before
> > shutting down the clock, contrary to the clock gating fw.
> > If you power down hc, you will also gate the clock. So the sdio protocol might not be
> > strictly followed.
> As mmc spec said, 8 clock cycles is needed. That is right.
> But I think if we have set the auto suspended delay for host, runtime pm core could make sure host to
> wait at least the auto suspended delay before shutting down the power of the device. So if the auto
> suspended delay is longer than 8 clock cycles, the mmc protocol can be followed, right?
I disagree. You cannot enforce this. It depends on clock speed, that can vary with sdio cards. 
User can change the autosuspend delay to whatever he wants, it should just change performance, but not functionality.

Why don't you just take my clock gating version?
Its working well, I'm using this for 2 weeks, and follow the linux-mmc latest improvement.

Regards,
Pierre


---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

RE: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host

[Dong, Chuanxiao]

Hi Pierre,
I think the runtime pm should not have any dependency with clock gating. Even the MMC driver won't clock gate SD/MMC card, host controller should still enter runtime suspend state, right?
So if auto suspended delay could be changed by user, then we can use pm_suspend_schedule to specify the latency which is longer than 8 clock cycle.

Thanks
Chuanxiao
> -----Original Message-----
> From: Tardy, Pierre
> Sent: Wednesday, January 19, 2011 12:39 AM
> To: Dong, Chuanxiao; linux-mmc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; cjb@laptop.org; ohad@wizery.com
> Subject: RE: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host
> 
> > > I'm also worried about this patch not taking in account the 8 clock cycles before
> > > shutting down the clock, contrary to the clock gating fw.
> > > If you power down hc, you will also gate the clock. So the sdio protocol might not
> be
> > > strictly followed.
> > As mmc spec said, 8 clock cycles is needed. That is right.
> > But I think if we have set the auto suspended delay for host, runtime pm core
> could make sure host to
> > wait at least the auto suspended delay before shutting down the power of the
> device. So if the auto
> > suspended delay is longer than 8 clock cycles, the mmc protocol can be followed,
> right?
> I disagree. You cannot enforce this. It depends on clock speed, that can vary with
> sdio cards.
> User can change the autosuspend delay to whatever he wants, it should just
> change performance, but not functionality.
> 
> Why don't you just take my clock gating version?
> Its working well, I'm using this for 2 weeks, and follow the linux-mmc latest
> improvement.
> 
> Regards,
> Pierre
> 

RE: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host

[Tardy, Pierre]

Chris,
(Sorry for top posting)
Seems we have a intel intern disagreement.

Could we have maintainer opinion on this ?

Regards,
Pierre

> -----Original Message-----
> From: Dong, Chuanxiao
> Sent: Wednesday, January 19, 2011 10:04 AM
> To: Tardy, Pierre; linux-mmc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; cjb@laptop.org; ohad@wizery.com
> Subject: RE: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host
> 
> Hi Pierre,
> I think the runtime pm should not have any dependency with clock gating. Even the MMC driver won't
> clock gate SD/MMC card, host controller should still enter runtime suspend state, right?
> So if auto suspended delay could be changed by user, then we can use pm_suspend_schedule to specify
> the latency which is longer than 8 clock cycle.
> 
> Thanks
> Chuanxiao
> > -----Original Message-----
> > From: Tardy, Pierre
> > Sent: Wednesday, January 19, 2011 12:39 AM
> > To: Dong, Chuanxiao; linux-mmc@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org; cjb@laptop.org; ohad@wizery.com
> > Subject: RE: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host
> >
> > > > I'm also worried about this patch not taking in account the 8 clock cycles before
> > > > shutting down the clock, contrary to the clock gating fw.
> > > > If you power down hc, you will also gate the clock. So the sdio protocol might not
> > be
> > > > strictly followed.
> > > As mmc spec said, 8 clock cycles is needed. That is right.
> > > But I think if we have set the auto suspended delay for host, runtime pm core
> > could make sure host to
> > > wait at least the auto suspended delay before shutting down the power of the
> > device. So if the auto
> > > suspended delay is longer than 8 clock cycles, the mmc protocol can be followed,
> > right?
> > I disagree. You cannot enforce this. It depends on clock speed, that can vary with
> > sdio cards.
> > User can change the autosuspend delay to whatever he wants, it should just
> > change performance, but not functionality.
> >
> > Why don't you just take my clock gating version?
> > Its working well, I'm using this for 2 weeks, and follow the linux-mmc latest
> > improvement.
> >
> > Regards,
> > Pierre
> >

---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host

[Chris Ball]

On Wed, Jan 19, 2011 at 09:45:57AM +0000, Tardy, Pierre wrote:
> Chris,
> (Sorry for top posting)
> Seems we have a intel intern disagreement.
> 
> Could we have maintainer opinion on this ?

Linus W and Ohad, any input here?  Thanks,

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

Re: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host

[Ohad Ben-Cohen]

On Thu, Jan 20, 2011 at 7:08 AM, Chris Ball <cjb@laptop.org> wrote:
> On Wed, Jan 19, 2011 at 09:45:57AM +0000, Tardy, Pierre wrote:
>> Chris,
>> (Sorry for top posting)
>> Seems we have a intel intern disagreement.
>>
>> Could we have maintainer opinion on this ?
>
> Linus W and Ohad, any input here?  Thanks,

Personally I prefer the runtime PM approach.

The MMC aggressive clock gating framework is great, but it ended up
duplicating plumbing that already existed in the runtime PM framework
(workqueue, locking, usage refcount, "get/put" API).

Moving to runtime PM should reduce a fair amount of code from the MMC
core, make the code more readable and easier to maintain, and let us
take advantage of standard tools and knobs that are based on runtime
PM's sysfs entries.

But surely we don't want two frameworks doing the same; that would be
too messy. And we want to be sure that any runtime PM approach would
satisfy everyone's requirement.

Chuanxiao, do you think you can come up with a runtime PM-based patch
that provides the aggressive clock gating framework's functionality ?

If functionality and requirements are preserved, I don't think anyone
would complain about moving to an established and well-maintained
kernel subsystem.

Thanks,
Ohad.

Re: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host

[Nicolas Pitre]

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

On Sat, 22 Jan 2011, Ohad Ben-Cohen wrote:

> On Thu, Jan 20, 2011 at 7:08 AM, Chris Ball <cjb@laptop.org> wrote:
> > On Wed, Jan 19, 2011 at 09:45:57AM +0000, Tardy, Pierre wrote:
> >> Chris,
> >> (Sorry for top posting)
> >> Seems we have a intel intern disagreement.
> >>
> >> Could we have maintainer opinion on this ?
> >
> > Linus W and Ohad, any input here?  Thanks,
> 
> Personally I prefer the runtime PM approach.
> 
> The MMC aggressive clock gating framework is great, but it ended up
> duplicating plumbing that already existed in the runtime PM framework
> (workqueue, locking, usage refcount, "get/put" API).
> 
> Moving to runtime PM should reduce a fair amount of code from the MMC
> core, make the code more readable and easier to maintain, and let us
> take advantage of standard tools and knobs that are based on runtime
> PM's sysfs entries.

No! Please don't conflate those two cases together again.

First case:  you want to stop the clock _to_ the _card_ when not talking 
to it.  That has nothing to do with power saving performed within the 
host controller.  This is for reducing power consumption by the _card_ 
(and possibly by the clock generator).  The runtime PM stuff has no 
business here as the decision to gate the clock on the card require 
MMC/SD protocol knowledge.

Second case: you want the _host_CONTROLLER_ to be shut down so it 
doesn't consume power by itself when idle.  This would be performed when 
case 1 has gated the clock, but not necessarily always.  Here the 
runtime PM infrastructure could make some sense.

All host controllers can gate the card's clock, but not all of them can 
power themselves down.  And maybe there would be a case for keeping the 
clock to a SDIO card active while the host controller is powered down.  
Knowledge for gating the card's clock is in the core code and 
independent from the host controller.  Knowledge for powering down the 
host controller is in the host controller driver. So those two cases 
really need to remain separate.


Nicolas

RE: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host

[Tardy, Pierre]

> 
> First case:  you want to stop the clock _to_ the _card_ when not talking
> to it.  That has nothing to do with power saving performed within the
> host controller.  This is for reducing power consumption by the _card_
> (and possibly by the clock generator).  The runtime PM stuff has no
> business here as the decision to gate the clock on the card require
> MMC/SD protocol knowledge.

Could not the mmcclock be a device on this own, and use rpm to manage its state?

> Second case: you want the _host_CONTROLLER_ to be shut down so it
> doesn't consume power by itself when idle.  This would be performed when
> case 1 has gated the clock, but not necessarily always. 
Agree with this, this is the idea of the my implementation.

> Here the
> runtime PM infrastructure could make some sense.
> 
> All host controllers can gate the card's clock, but not all of them can
> power themselves down.  And maybe there would be a case for keeping the
> clock to a SDIO card active while the host controller is powered down.

> Knowledge for gating the card's clock is in the core code and
> independent from the host controller.
But knowledge on *how* to gate the clock is in the host controller driver, and/or platform code.


Pierre
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host

[Ohad Ben-Cohen]

On Sun, Jan 23, 2011 at 7:20 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
> First case:  you want to stop the clock _to_ the _card_ when not talking
> to it.  That has nothing to do with power saving performed within the
> host controller.  This is for reducing power consumption by the _card_
> (and possibly by the clock generator).  The runtime PM stuff has no
> business here as the decision to gate the clock on the card require
> MMC/SD protocol knowledge.

But why can't it be implemented using the runtime PM framework ?

It's just plumbing; the decision and knowledge stays at the MMC core.

It's possible to do it without making any compromises of use cases and
requirements. And if something is missing in the runtime PM framework,
it can be changed. It just needs someone who cares.

RE: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host

[Dong, Chuanxiao]

> -----Original Message-----
> From: Ohad Ben-Cohen [mailto:ohad@wizery.com]
> Sent: Monday, January 24, 2011 4:40 AM
> To: Nicolas Pitre
> Cc: Chris Ball; Dong, Chuanxiao; Tardy, Pierre; linux-mmc@vger.kernel.org; Linus
> Walleij; Gao, Yunpeng
> Subject: Re: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host
> 
> On Sun, Jan 23, 2011 at 7:20 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
> > First case:  you want to stop the clock _to_ the _card_ when not talking
> > to it.  That has nothing to do with power saving performed within the
> > host controller.  This is for reducing power consumption by the _card_
> > (and possibly by the clock generator).  The runtime PM stuff has no
> > business here as the decision to gate the clock on the card require
> > MMC/SD protocol knowledge.
> 
> But why can't it be implemented using the runtime PM framework ?
> 
> It's just plumbing; the decision and knowledge stays at the MMC core.
> 
> It's possible to do it without making any compromises of use cases and
> requirements. And if something is missing in the runtime PM framework,
> it can be changed. It just needs someone who cares.
I agree with Ohad.
First I also agree with Nicolas clock gating and runtime pm are different cases, all of us want runtime pm and clock gating for mmc driver. So that, I think we can treat the clock gating as a shallow power consuming and runtime pm as a deep power consuming. When there is no new request after about 8 clock cycles idle, mmc driver can try to do a shallow power consuming - clock gating. And then if in the next a few time, host controller is still idle, we can try to do a deep power consuming - power off host controller. Is this acceptable?

Second, let us consider about the implementation about clock gating. To my understanding, what MMC driver does in clock gating framework is just waiting for 8 clock cycles and then trying to gate the clock, no other special. Linus W, correct me if I am wrong. The runtime pm framework also provide some functions which can let driver wait for some clock cycles and then try to do something. So I think we can use what the runtime pm framework provided to implement the clock gating, no need to maintain some code in MMC driver doing the same thing which has already been implemented by runtime pm framework.

We can use runtime pm framework to implement clock gating and power management. The two cases has no dependence with each other. If the host controller cannot power off, just not enable the runtime pm in host controller driver.

Thanks
Chuanxiao

RE: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host

[Nicolas Pitre]

On Sun, 23 Jan 2011, Tardy, Pierre wrote:

> > 
> > First case:  you want to stop the clock _to_ the _card_ when not talking
> > to it.  That has nothing to do with power saving performed within the
> > host controller.  This is for reducing power consumption by the _card_
> > (and possibly by the clock generator).  The runtime PM stuff has no
> > business here as the decision to gate the clock on the card require
> > MMC/SD protocol knowledge.
> 
> Could not the mmcclock be a device on this own, and use rpm to manage its state?

Isn't that rather overkill?

The core stack may decide to tell the host controller to set the 
mmcclock to 400 kHz, 25 MHz, or even 0 Hz depending on various states 
that are deeply tied to the protocol used with the card and not 
necessarily for PM purposes.  So why would you make the 0 Hz a special 
case here and handle it in a different framework from the other cases?  
The fact that we do try to set the clock to 0 Hz as often as possible 
for PM purposes is only consequential and I don't see the need for a 
side structure just to handle that case.

> > Second case: you want the _host_CONTROLLER_ to be shut down so it
> > doesn't consume power by itself when idle.  This would be performed when
> > case 1 has gated the clock, but not necessarily always. 
> Agree with this, this is the idea of the my implementation.
> 
> > Here the
> > runtime PM infrastructure could make some sense.
> > 
> > All host controllers can gate the card's clock, but not all of them can
> > power themselves down.  And maybe there would be a case for keeping the
> > clock to a SDIO card active while the host controller is powered down.
> 
> > Knowledge for gating the card's clock is in the core code and
> > independent from the host controller.
> But knowledge on *how* to gate the clock is in the host controller driver, and/or platform code.

So what?  The clock is already handled today for reasons other than PM.  
And the aggressive clock gating had to modify only the core stack code 
and zero driver/platform code.  Hence I don't see the advantage to move 
away from what we have now.  Could you please elaborate more on the 
advantages you see there?


Nicolas

Re: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host

[Nicolas Pitre]

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

On Sun, 23 Jan 2011, Ohad Ben-Cohen wrote:

> On Sun, Jan 23, 2011 at 7:20 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
> > First case:  you want to stop the clock _to_ the _card_ when not talking
> > to it.  That has nothing to do with power saving performed within the
> > host controller.  This is for reducing power consumption by the _card_
> > (and possibly by the clock generator).  The runtime PM stuff has no
> > business here as the decision to gate the clock on the card require
> > MMC/SD protocol knowledge.
> 
> But why can't it be implemented using the runtime PM framework ?

Because as I see things now, it'll only make the code more complex for 
no real gain.

> It's just plumbing; the decision and knowledge stays at the MMC core.
> 
> It's possible to do it without making any compromises of use cases and
> requirements. And if something is missing in the runtime PM framework,
> it can be changed. It just needs someone who cares.

I don't feel like modifying a generic infrastructure like the runtime PM 
framework just to accommodate a special case from the MMC subsystem 
which the MMC subsystem already handles well now anyway is necessarily 
productive.  That will only increase complexity on both sides: the 
runtime PM framework _and_ the MMC subsystem.  But hey, that someone who 
cares may prove me wrong.


Nicolas

Re: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host

[Linus Walleij]

2011/1/20 Chris Ball <cjb@laptop.org>:
> On Wed, Jan 19, 2011 at 09:45:57AM +0000, Tardy, Pierre wrote:
>> Chris,
>> (Sorry for top posting)
>> Seems we have a intel intern disagreement.
>>
>> Could we have maintainer opinion on this ?
>
> Linus W and Ohad, any input here?  Thanks,

What I see is orthogonal purposes altogether. Usually there is
something like two clocks and two regulators or switches
available in every sufficiently advanced system:

- One regulator to power the card
- One clock to clock the card (MCLK)
- One regulator or switch to power the MMC IP block
- One clock to clock the MMC IP block

The MMC core regulator handling and the clock gating I
implemented is about the two first things.

I think this patch is about the two *other* things, if I read it
right.

So drivers can register PM runtime hooks to its class device
and that's probably a good thing, but doesn't it conflict with
the stuff we already have in place all over the core, that
makes sure that mmc_power_save_host() gets called
from the mmc bus.

I don't know how that fits with e.g. OMAP where they
already are doing this with mmc_power_save_host() so I
would really like input from them on this subject.
It looks like a duplication of that mechanism, just tied to the
class device instead of the mmc bus.

Further this patch cannot be applied as-is.
It needs a clause where it will wait for the clock gating to
be sync:ed *before* calling the suspend hook.

Just pm_generic_runtime_suspend won't do it. *If*
we have clock gating we certainly want to make sure
it is gated before this happens. With the current
mmc_power_save_host() we can do this in the driver
itself, taking the fact that the clock may be gated into
account.

The other question, whether the delay hysteresis
functions in runtime PM can be reused for clock gating
remains unanswered, the easiest thing to do is cook
up a patch. AFAICT that involves factoring out the
code dealing with that from runtime.c and when looking
at it that doesn't seem easy, it strictly requires a struct
device to live, and setting up abstract devices inside
the MMC framework for this doesn't seem like a good
idea to me.

Yours,
Linus Walleij
--
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 v1 1/3]mmc: implemented runtime pm for mmc host

[Tardy, Pierre]

> -----Original Message-----
> From: Linus Walleij [mailto:linus.ml.walleij@gmail.com]
> Sent: Monday, January 24, 2011 11:11 PM
> To: Chris Ball
> Cc: Tardy, Pierre; linux-mmc@vger.kernel.org; Ohad Ben-Cohen; linux-omap Mailing List
> Subject: Re: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host
> 
> 2011/1/20 Chris Ball <cjb@laptop.org>:
> > On Wed, Jan 19, 2011 at 09:45:57AM +0000, Tardy, Pierre wrote:
> >> Chris,
> >> (Sorry for top posting)
> >> Seems we have a intel intern disagreement.
> >>
> >> Could we have maintainer opinion on this ?
> >
> > Linus W and Ohad, any input here?  Thanks,
> 
> What I see is orthogonal purposes altogether. Usually there is
> something like two clocks and two regulators or switches
> available in every sufficiently advanced system:
> 
> - One regulator to power the card
> - One clock to clock the card (MCLK)
> - One regulator or switch to power the MMC IP block
> - One clock to clock the MMC IP block
> 
> The MMC core regulator handling and the clock gating I
> implemented is about the two first things.
> 
> I think this patch is about the two *other* things, if I read it
> right.

Actually in sdhci, you power off the MMC IP block, you power OFF the MCLK (at least on our platform)

I don't know other platform where mmc clock is not controlled more or less directlty by the sdhc IP.

> Further this patch cannot be applied as-is.
> It needs a clause where it will wait for the clock gating to
> be sync:ed *before* calling the suspend hook.
That's the purpose of those hunks:

iff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 8c3769b..27931f6 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -20,6 +20,7 @@
 #include <linux/slab.h>
 #include <linux/scatterlist.h>
 #include <linux/regulator/consumer.h>
+#include <linux/pm_runtime.h>
 
 #include <linux/leds.h>
 
@@ -1221,6 +1222,7 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
        struct sdhci_host *host;
        unsigned long flags;
+       unsigned int lastclock;
        u8 ctrl;
 
        host = mmc_priv(mmc);
@@ -1231,6 +1233,27 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
                goto out;
 
        /*
+        * get/put runtime_pm usage counter at ios->clock transitions
+        * We need to do it before any other chip access, as sdhci could
+        * be power gated
+        */
+       lastclock = host->iosclock;
+       host->iosclock = ios->clock;
+       if (lastclock == 0 && ios->clock != 0) {
+               spin_unlock_irqrestore(&host->lock, flags);
+               pm_runtime_get_sync(host->mmc->parent);
+               spin_lock_irqsave(&host->lock, flags);
+       } else if (lastclock != 0 && ios->clock == 0) {
+               spin_unlock_irqrestore(&host->lock, flags);
+               pm_runtime_mark_last_busy(host->mmc->parent);
+               pm_runtime_put_autosuspend(host->mmc->parent);
+               spin_lock_irqsave(&host->lock, flags);
+       }
+       /* no need to configure the rest.. */
+       if (host->iosclock == 0)
+               goto out;
+
+       /*
         * Reset the chip on each power off.
         * Should clear out any weird states.
         */
@@ -1303,6 +1326,8 @@ static int sdhci_get_ro(struct mmc_host *mmc)

I'm wondering if this code could be generic to all drivers, and if clock gating could not be taking/releasing reference counter on the mmc_bus whenever there is a clock gating transition?
We could condition that on some MMC_CAP_POWERGATE_WILL_CLKGATE capability flag.


Regards,
Pierre
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host

[Linus Walleij]

2011/1/25 Tardy, Pierre <pierre.tardy@intel.com>:

> Actually in sdhci, you power off the MMC IP block, you power OFF the
> MCLK (at least on our platform)
>
> I don't know other platform where mmc clock is not controlled more or
> less directlty by the sdhc IP.

This is the case with host/mmci.c.

It is taking a clock inside the driver (MCLK), but since it's an AMBA
primecell, you can see that when it's probed it also requests and
optional IP clock in drivers/amba/bus.c.

The ARM reference platforms are wired with two different clocks
connected to each of these.

There are platforms, such as the U300 that just connect the two
into one terminal though. To get a chance to handle the abstraction
for both cases these two clocks refer to the same struct clk in the
clock tree.

>> Further this patch cannot be applied as-is.
>> It needs a clause where it will wait for the clock gating to
>> be sync:ed *before* calling the suspend hook.
>
> That's the purpose of those hunks:
>
> iff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 8c3769b..27931f6 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -20,6 +20,7 @@
>  #include <linux/slab.h>
>  #include <linux/scatterlist.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/pm_runtime.h>
>
>  #include <linux/leds.h>
>
> @@ -1221,6 +1222,7 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  {
>        struct sdhci_host *host;
>        unsigned long flags;
> +       unsigned int lastclock;
>        u8 ctrl;
>
>        host = mmc_priv(mmc);
> @@ -1231,6 +1233,27 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>                goto out;
>
>        /*
> +        * get/put runtime_pm usage counter at ios->clock transitions
> +        * We need to do it before any other chip access, as sdhci could
> +        * be power gated
> +        */
> +       lastclock = host->iosclock;
> +       host->iosclock = ios->clock;
> +       if (lastclock == 0 && ios->clock != 0) {
> +               spin_unlock_irqrestore(&host->lock, flags);
> +               pm_runtime_get_sync(host->mmc->parent);
> +               spin_lock_irqsave(&host->lock, flags);
> +       } else if (lastclock != 0 && ios->clock == 0) {
> +               spin_unlock_irqrestore(&host->lock, flags);
> +               pm_runtime_mark_last_busy(host->mmc->parent);
> +               pm_runtime_put_autosuspend(host->mmc->parent);
> +               spin_lock_irqsave(&host->lock, flags);
> +       }
> +       /* no need to configure the rest.. */
> +       if (host->iosclock == 0)
> +               goto out;
> +
> +       /*
>         * Reset the chip on each power off.
>         * Should clear out any weird states.
>         */

Aha I get it. So it uses the freq shift from the existing clock gate
code to hint rpm, that's actually how I envisioned that this would
work for this case.

Now I really started liking this patch.
Acked-by: Linus Walleij <linus.walleij@stericsson.com>

> @@ -1303,6 +1326,8 @@ static int sdhci_get_ro(struct mmc_host *mmc)
>
> I'm wondering if this code could be generic to all drivers, and if clock gating could not be taking/releasing reference counter on the mmc_bus whenever there is a clock gating transition?
> We could condition that on some MMC_CAP_POWERGATE_WILL_CLKGATE capability flag.

Time will tell I guess, the code in each driver will surely be similar
to the above, but I already see some deviations, e.g. some HW
will need to delay the actual action taken, some may want to
manipulate a register or regulator etc, so I'd leave it open-ended
like this.

Yours,
Linus Walleij

Re: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host

[Linus Walleij]

2011/1/26 Linus Walleij <linus.walleij@linaro.org>:
> 2011/1/25 Tardy, Pierre <pierre.tardy@intel.com>:

>> @@ -1231,6 +1233,27 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>                goto out;
>>
>>        /*
>> +        * get/put runtime_pm usage counter at ios->clock transitions
>> +        * We need to do it before any other chip access, as sdhci could
>> +        * be power gated
>> +        */
>> +       lastclock = host->iosclock;
>> +       host->iosclock = ios->clock;
>> +       if (lastclock == 0 && ios->clock != 0) {
>> +               spin_unlock_irqrestore(&host->lock, flags);
>> +               pm_runtime_get_sync(host->mmc->parent);
>> +               spin_lock_irqsave(&host->lock, flags);
>> +       } else if (lastclock != 0 && ios->clock == 0) {
>> +               spin_unlock_irqrestore(&host->lock, flags);
>> +               pm_runtime_mark_last_busy(host->mmc->parent);
>> +               pm_runtime_put_autosuspend(host->mmc->parent);
>> +               spin_lock_irqsave(&host->lock, flags);
>> +       }
>> +       /* no need to configure the rest.. */
>> +       if (host->iosclock == 0)
>> +               goto out;
>> +
>> +       /*
>>         * Reset the chip on each power off.
>>         * Should clear out any weird states.
>>         */
>
> Aha I get it. So it uses the freq shift from the existing clock gate
> code to hint rpm, that's actually how I envisioned that this would
> work for this case.
>
> Now I really started liking this patch.
> Acked-by: Linus Walleij <linus.walleij@stericsson.com>

Which shall be interpreted as for the patch with the above code, not
the one which is subject for this post I believe.

Pierre, I can't locate your patches for some reason, care to repost
them?

Yours,
Linus Walleij
--
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 v1 1/3]mmc: implemented runtime pm for mmc host

[Tardy, Pierre]

[snip]
> >
> > Now I really started liking this patch.
> > Acked-by: Linus Walleij <linus.walleij@stericsson.com>
> 
> Which shall be interpreted as for the patch with the above code, not
> the one which is subject for this post I believe.
> 
> Pierre, I can't locate your patches for some reason, care to repost
> them?
https://patchwork.kernel.org/patch/427151/

I have newer version of them, which actually works (this one was just RFC)
I'll post it later in the week.

Pierre
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.