RE: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host
[snip] > > > > Now I really started liking this patch. > > Acked-by: Linus Walleij > > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@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
2011/1/26 Linus Walleij :
> 2011/1/25 Tardy, Pierre :
>> @@ -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
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 majord...@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
2011/1/25 Tardy, Pierre :
> 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
> #include
> #include
> +#include
>
> #include
>
> @@ -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
> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@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
> -Original Message-
> From: Linus Walleij [mailto:linus.ml.wall...@gmail.com]
> Sent: Monday, January 24, 2011 11:11 PM
> To: Chris Ball
> Cc: Tardy, Pierre; linux-...@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 :
> > On Wed, Jan 19, 2011 at 09:45:57AM +, 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
#include
#include
+#include
#include
@@ -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.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@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
2011/1/20 Chris Ball : > On Wed, Jan 19, 2011 at 09:45:57AM +, 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
