| Message ID | 1355235474-12783-1-git-send-email-ulf.hansson@stericsson.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 11, 2012 at 3:17 PM, Ulf Hansson <ulf.hansson@stericsson.com> wrote: > From: Ulf Hansson <ulf.hansson@linaro.org> > > The amba bus is already performing same actions but for the apb_pclk. > So here we just make sure the clock to card is gated as well to save > more power. At runtime resume we will thus restore the clock again. > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> Looks correct to me: Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 11, 2012 at 03:17:54PM +0100, Ulf Hansson wrote: > From: Ulf Hansson <ulf.hansson@linaro.org> > > The amba bus is already performing same actions but for the apb_pclk. > So here we just make sure the clock to card is gated as well to save > more power. At runtime resume we will thus restore the clock again. And how exactly do you ensure the requirement that a certain number of clocks is supplied to the card after the last command before you cut the clock? -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 11, 2012 at 3:52 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Tue, Dec 11, 2012 at 03:17:54PM +0100, Ulf Hansson wrote: >> From: Ulf Hansson <ulf.hansson@linaro.org> >> >> The amba bus is already performing same actions but for the apb_pclk. >> So here we just make sure the clock to card is gated as well to save >> more power. At runtime resume we will thus restore the clock again. > > And how exactly do you ensure the requirement that a certain number of > clocks is supplied to the card after the last command before you cut > the clock? Oh, that's gonna be a problem. So I can see a quite straight-forward way to do this actually: 1) Turn on MMC_CLKGATE for this driver (select from Kconfig) which means that the ios will be called with f=0 whenever the card is unused, taking into account the required number of clocks to the card. 2) Implement handling of f=0 from the ios in the ios callback. Then clk_disable(host->clk). I did a hack at this some months back but never got around to finish it, sorry for not doing that even though this clock gating was invented for the MMCI in the first place :-( 3) In the suspend() callback, sleep until the clock indicates that the card is declocked with something like: while (clk_is_enabled(host->clk)) { sleep(1) } However the clk framework does not have clk_is_enabled() so you'd either have to add that or use a local atomic variable host_clk_enabled set in (2). Should work, I think? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12 December 2012 07:53, Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Dec 11, 2012 at 3:52 PM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: >> On Tue, Dec 11, 2012 at 03:17:54PM +0100, Ulf Hansson wrote: >>> From: Ulf Hansson <ulf.hansson@linaro.org> >>> >>> The amba bus is already performing same actions but for the apb_pclk. >>> So here we just make sure the clock to card is gated as well to save >>> more power. At runtime resume we will thus restore the clock again. >> >> And how exactly do you ensure the requirement that a certain number of >> clocks is supplied to the card after the last command before you cut >> the clock? So, I agree that this needs to be taken care of, but I think it already is, see more comments below. > > Oh, that's gonna be a problem. > > So I can see a quite straight-forward way to do this actually: > > 1) Turn on MMC_CLKGATE for this driver (select from Kconfig) > which means that the ios will be called with f=0 whenever > the card is unused, taking into account the required number > of clocks to the card. I think MMC_CLKGATE was a good initiative in the past. But that was before runtime pm was there to use. Runtime pm suits much better for handling clock gating and other runtime power save actions that could be possible for a host driver to do. I would even suggest the MMC_CLKGATE should be removed from the protocol layer, once we see that all host drivers that used it has switch to runtime pm. > > 2) Implement handling of f=0 from the ios in the ios callback. > Then clk_disable(host->clk). I did a hack at this some months > back but never got around to finish it, sorry for not doing > that even though this clock gating was invented for the MMCI > in the first place :-( > > 3) In the suspend() callback, sleep until the clock indicates > that the card is declocked with something like: > while (clk_is_enabled(host->clk)) { sleep(1) } > However the clk framework does not have clk_is_enabled() > so you'd either have to add that or use a local atomic > variable host_clk_enabled set in (2). > > Should work, I think? I think the solution is much easier. Please consider the below: 1. Some host drivers which implements runtime pm and corresponding runtime callbacks, primarily rely on the the autosuspend timeout. In mmci case, this timeout is set to 50ms, which will be more than enough for any slow frequency to accomplish the 8 clock cycles. I believe sdhci_s3c driver and omap_hsmmc.c does this as well. 2. If we think 1) is not enough for all mmc/sd/sdio operations you could implement ".enable|disable" host function pointers. In those you can do pm_runtime_get|put. Thus the responsibility will be moved to the protocol layer of handling the mmc_claim|release_host properly. It must then keep the host claimed if by some reason the host must be prevented from entering "runtime power save mode". Although, I have not yet found any mmc/sd/sdio operations that seems to require this to be done, you will have to convince me about what circumstances this is need, except for the 8 clock cycles of course. omap_hsmmc.c, does this, I doubt it is needed. > > Yours, > Linus Walleij Kind regards Ulf Hansson -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 12, 2012 at 12:02 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 12 December 2012 07:53, Linus Walleij <linus.walleij@linaro.org> wrote: >> 1) Turn on MMC_CLKGATE for this driver (select from Kconfig) >> which means that the ios will be called with f=0 whenever >> the card is unused, taking into account the required number >> of clocks to the card. > > I think MMC_CLKGATE was a good initiative in the past. But that was > before runtime pm was there to use. Runtime pm suits much better for > handling clock gating and other runtime power save actions that could > be possible for a host driver to do. > > I would even suggest the MMC_CLKGATE should be removed from the > protocol layer, once we see that all host drivers that used it has > switch to runtime pm. Ah now I remember that you've actually even explained this to me ... memory is too short sometimes. But I follow this idea. Then we have only the coordination between runtime suspend and suspend proper to iron out. Russell's concern is valid if suspend() will not wait for runtime_suspend() to complete the last cycle before suspending. Hm this sounds scaringly familiar to what we've discussed with Kevin Hilman et al recently... Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c index aa04b42..0f397a2 100644 --- a/drivers/mmc/host/mmci.c +++ b/drivers/mmc/host/mmci.c @@ -1636,8 +1636,37 @@ static int mmci_resume(struct device *dev) } #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); + + if (mmc) { + struct mmci_host *host = mmc_priv(mmc); + clk_disable_unprepare(host->clk); + } + + 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); + + if (mmc) { + struct mmci_host *host = mmc_priv(mmc); + clk_prepare_enable(host->clk); + } + + 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, NULL) }; static struct amba_id mmci_ids[] = {