| Message ID | 1309538376-23260-3-git-send-email-balajitk@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: linux-mmc-owner@vger.kernel.org > [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Kevin Hilman > Sent: Saturday, July 09, 2011 2:24 AM > To: Balaji T K > Cc: linux-omap@vger.kernel.org; linux-mmc@vger.kernel.org; cjb@laptop.org; > tony@atomide.com; madhu.cr@ti.com; b-cousson@ti.com; paul@pwsan.com; > kishore.kadiyala@ti.com > Subject: Re: [PATCHv4 2/3] MMC: OMAP: HSMMC: add runtime pm support > > Balaji T K <balajitk@ti.com> writes: > > > add runtime pm support to HSMMC host controller > > Use runtime pm API to enable/disable HSMMC clock > > Use runtime autosuspend APIs to enable auto suspend delay > > > > Based on OMAP HSMMC runtime implementation by Kevin Hilman, Kishore > Kadiyala > > > > Signed-off-by: Balaji T K <balajitk@ti.com> > > It's not relevant for this merge window, but I'm exploring some future > changes to our PM core code and have a question about how MMC works for > system suspend. > > Basially, the question is: can the driver be reworked such that a system > suspend does not need to runtime resume the device? For most devices, > we kind of expect that if the device is runtime suspended, a system > suspend will have nothing extra to do, but this driver runtime resumes > the device during system suspend in order to do "stuff", which I > admitedly don't fully undestand. > > Ideally, the "stuff" needed for runtime suspend and system suspend could > be made to be common such that a system suspend of a runtime suspended > device would be a noop. > > Is this possible? > > Kevin During system suspended patch, a callback named .prepare will be first done before .suspend is called, and .complete callback will be called after .resume is called. These two callbacks are in pair. If driver can implement the .prepare and hold the usage count in this callback, then runtime pm suspend/resume will not happen during device suspending. So there will be no need to add pm_runtime_get* and pm_runtime_put* in .suspend/.resume. BTW, if .prepare has hold the usage count, then .complete callback need to release the usage count and put device in runtime suspended mode. Thanks Chuanxiao > > > @@ -2100,6 +2087,7 @@ static int omap_hsmmc_suspend(struct device *dev) > > return 0; > > > > if (host) { > > + pm_runtime_get_sync(host->dev); > > host->suspended = 1; > > if (host->pdata->suspend) { > > ret = host->pdata->suspend(&pdev->dev, > > @@ -2114,13 +2102,11 @@ static int omap_hsmmc_suspend(struct device > *dev) > > } > > cancel_work_sync(&host->mmc_carddetect_work); > > ret = mmc_suspend_host(host->mmc); > > - mmc_host_enable(host->mmc); > > + > > if (ret == 0) { > > omap_hsmmc_disable_irq(host); > > OMAP_HSMMC_WRITE(host->base, HCTL, > > OMAP_HSMMC_READ(host->base, HCTL) & ~SDBP); > > - mmc_host_disable(host->mmc); > > - clk_disable(host->iclk); > > if (host->got_dbclk) > > clk_disable(host->dbclk); > > } else { > > @@ -2132,9 +2118,8 @@ static int omap_hsmmc_suspend(struct device *dev) > > dev_dbg(mmc_dev(host->mmc), > > "Unmask interrupt failed\n"); > > } > > - mmc_host_disable(host->mmc); > > } > > - > > + pm_runtime_put_sync(host->dev); > > } > > return ret; > > } > -- > 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 -- 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
"Dong, Chuanxiao" <chuanxiao.dong@intel.com> writes: [...] >> >> Basially, the question is: can the driver be reworked such that a system >> suspend does not need to runtime resume the device? For most devices, >> we kind of expect that if the device is runtime suspended, a system >> suspend will have nothing extra to do, but this driver runtime resumes >> the device during system suspend in order to do "stuff", which I >> admitedly don't fully undestand. >> >> Ideally, the "stuff" needed for runtime suspend and system suspend could >> be made to be common such that a system suspend of a runtime suspended >> device would be a noop. >> >> Is this possible? >> >> Kevin > > During system suspended patch, a callback named .prepare will be first > done before .suspend is called, and .complete callback will be called > after .resume is called. These two callbacks are in pair. If driver > can implement the .prepare and hold the usage count in this callback, > then runtime pm suspend/resume will not happen during device > suspending. So there will be no need to add pm_runtime_get* and > pm_runtime_put* in .suspend/.resume. That doesn't avoid the problem, since the device is still runtime resumed and then re-suspended during system suspend. My basic question is this: why does this device need to be runtime resumed during system suspend? Why can't it just stay runtime suspended? Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 13, 2011 at 8:29 PM, Kevin Hilman <khilman@ti.com> wrote: > "Dong, Chuanxiao" <chuanxiao.dong@intel.com> writes: > > [...] > >>> >>> Basially, the question is: can the driver be reworked such that a system >>> suspend does not need to runtime resume the device? For most devices, >>> we kind of expect that if the device is runtime suspended, a system >>> suspend will have nothing extra to do, but this driver runtime resumes >>> the device during system suspend in order to do "stuff", which I >>> admitedly don't fully undestand. >>> >>> Ideally, the "stuff" needed for runtime suspend and system suspend could >>> be made to be common such that a system suspend of a runtime suspended >>> device would be a noop. >>> >>> Is this possible? >>> >>> Kevin >> >> During system suspended patch, a callback named .prepare will be first >> done before .suspend is called, and .complete callback will be called >> after .resume is called. These two callbacks are in pair. If driver >> can implement the .prepare and hold the usage count in this callback, >> then runtime pm suspend/resume will not happen during device >> suspending. So there will be no need to add pm_runtime_get* and >> pm_runtime_put* in .suspend/.resume. > > That doesn't avoid the problem, since the device is still runtime > resumed and then re-suspended during system suspend. > > My basic question is this: why does this device need to be runtime > resumed during system suspend? Why can't it just stay runtime > suspended? > From my understanding, the runtime suspend is usually implemented to not lose the card 'context', i.e. transactions can continue after a runtime suspend / resume cycle. For system suspend, the MMC core sends a sleep command (which, in itself, is a transaction) to the card to go to sleep state, and for all practical purposes, the card is treated as 'removed'. When the system resumes, the card is rescanned and re-initialized. Hence, for system suspend, the MMC controller needs to be enabled to actually send the command which puts the card to sleep (and hence the resume). Best regards, Venkat. -- 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
"S, Venkatraman" <svenkatr@ti.com> writes: > On Wed, Jul 13, 2011 at 8:29 PM, Kevin Hilman <khilman@ti.com> wrote: [...] >> >> My basic question is this: why does this device need to be runtime >> resumed during system suspend? Why can't it just stay runtime >> suspended? >> > > From my understanding, the runtime suspend is usually implemented to not > lose the card 'context', i.e. transactions can continue after a > runtime suspend / > resume cycle. > > For system suspend, the MMC core sends a sleep command (which, in > itself, is a transaction) to the card to go to sleep state, and for > all practical purposes, the card is treated as 'removed'. When the > system resumes, the card is rescanned and re-initialized. > > Hence, for system suspend, the MMC controller needs to be enabled to > actually send the command which puts the card to sleep (and hence the > resume). Great, this is the detail I was looking for since my MMC knowledge is quite limited. So, in theory, this same sleep command could be sent during runtime suspend as well, so that a system suspend would not have to runtime resume, correct? But I suppose it would result in a much higher latency runtime resumes. It might be worth experimenting with doing this, possibly in combination with longer auto-suspend delay times. Thanks for the explanation, Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c index 819ff08..3d01e3f 100644 --- a/drivers/mmc/host/omap_hsmmc.c +++ b/drivers/mmc/host/omap_hsmmc.c @@ -33,6 +33,7 @@ #include <linux/semaphore.h> #include <linux/gpio.h> #include <linux/regulator/consumer.h> +#include <linux/pm_runtime.h> #include <plat/dma.h> #include <mach/hardware.h> #include <plat/board.h> @@ -116,6 +117,7 @@ #define OMAP_MMC4_DEVID 3 #define OMAP_MMC5_DEVID 4 +#define MMC_AUTOSUSPEND_DELAY 100 #define MMC_TIMEOUT_MS 20 #define OMAP_MMC_MASTER_CLOCK 96000000 #define DRIVER_NAME "omap_hsmmc" @@ -1147,8 +1149,7 @@ static int omap_hsmmc_switch_opcond(struct omap_hsmmc_host *host, int vdd) int ret; /* Disable the clocks */ - clk_disable(host->fclk); - clk_disable(host->iclk); + pm_runtime_put_sync(host->dev); if (host->got_dbclk) clk_disable(host->dbclk); @@ -1159,8 +1160,7 @@ static int omap_hsmmc_switch_opcond(struct omap_hsmmc_host *host, int vdd) if (!ret) ret = mmc_slot(host).set_power(host->dev, host->slot_id, 1, vdd); - clk_enable(host->iclk); - clk_enable(host->fclk); + pm_runtime_get_sync(host->dev); if (host->got_dbclk) clk_enable(host->dbclk); @@ -1526,7 +1526,7 @@ static void omap_hsmmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) u32 con; int do_send_init_stream = 0; - mmc_host_enable(host->mmc); + pm_runtime_get_sync(host->dev); if (ios->power_mode != host->power_mode) { switch (ios->power_mode) { @@ -1621,8 +1621,7 @@ static void omap_hsmmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) else OMAP_HSMMC_WRITE(host->base, CON, con & ~OD); - if (host->power_mode == MMC_POWER_OFF) - mmc_host_disable(host->mmc); + pm_runtime_put_autosuspend(host->dev); } static int omap_hsmmc_get_cd(struct mmc_host *mmc) @@ -1681,13 +1680,9 @@ static void omap_hsmmc_conf_bus_power(struct omap_hsmmc_host *host) static int omap_hsmmc_enable_fclk(struct mmc_host *mmc) { struct omap_hsmmc_host *host = mmc_priv(mmc); - int err; - err = clk_enable(host->fclk); - if (err) - return err; - dev_dbg(mmc_dev(host->mmc), "mmc_fclk: enabled\n"); - omap_hsmmc_context_restore(host); + pm_runtime_get_sync(host->dev); + return 0; } @@ -1695,9 +1690,9 @@ static int omap_hsmmc_disable_fclk(struct mmc_host *mmc, int lazy) { struct omap_hsmmc_host *host = mmc_priv(mmc); - omap_hsmmc_context_save(host); - clk_disable(host->fclk); - dev_dbg(mmc_dev(host->mmc), "mmc_fclk: disabled\n"); + pm_runtime_mark_last_busy(host->dev); + pm_runtime_put_autosuspend(host->dev); + return 0; } @@ -1738,10 +1733,7 @@ static int omap_hsmmc_regs_show(struct seq_file *s, void *data) return 0; } - if (clk_enable(host->fclk) != 0) { - seq_printf(s, "can't read the regs\n"); - return 0; - } + pm_runtime_get_sync(host->dev); seq_printf(s, "SYSCONFIG:\t0x%08x\n", OMAP_HSMMC_READ(host->base, SYSCONFIG)); @@ -1758,7 +1750,8 @@ static int omap_hsmmc_regs_show(struct seq_file *s, void *data) seq_printf(s, "CAPA:\t\t0x%08x\n", OMAP_HSMMC_READ(host->base, CAPA)); - clk_disable(host->fclk); + pm_runtime_mark_last_busy(host->dev); + pm_runtime_put_autosuspend(host->dev); return 0; } @@ -1878,18 +1871,10 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev) mmc->caps |= MMC_CAP_DISABLE; - if (clk_enable(host->iclk) != 0) { - clk_put(host->iclk); - clk_put(host->fclk); - goto err1; - } - - if (mmc_host_enable(host->mmc) != 0) { - clk_disable(host->iclk); - clk_put(host->iclk); - clk_put(host->fclk); - goto err1; - } + pm_runtime_enable(host->dev); + pm_runtime_get_sync(host->dev); + pm_runtime_set_autosuspend_delay(host->dev, MMC_AUTOSUSPEND_DELAY); + pm_runtime_use_autosuspend(host->dev); if (cpu_is_omap2430()) { host->dbclk = clk_get(&pdev->dev, "mmchsdb_fck"); @@ -2016,6 +2001,8 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev) } omap_hsmmc_debugfs(mmc); + pm_runtime_mark_last_busy(host->dev); + pm_runtime_put_autosuspend(host->dev); return 0; @@ -2031,8 +2018,8 @@ err_reg: err_irq_cd_init: free_irq(host->irq, host); err_irq: - mmc_host_disable(host->mmc); - clk_disable(host->iclk); + pm_runtime_mark_last_busy(host->dev); + pm_runtime_put_autosuspend(host->dev); clk_put(host->fclk); clk_put(host->iclk); if (host->got_dbclk) { @@ -2056,7 +2043,7 @@ static int omap_hsmmc_remove(struct platform_device *pdev) struct resource *res; if (host) { - mmc_host_enable(host->mmc); + pm_runtime_get_sync(host->dev); mmc_remove_host(host->mmc); if (host->use_reg) omap_hsmmc_reg_put(host); @@ -2067,8 +2054,8 @@ static int omap_hsmmc_remove(struct platform_device *pdev) free_irq(mmc_slot(host).card_detect_irq, host); flush_work_sync(&host->mmc_carddetect_work); - mmc_host_disable(host->mmc); - clk_disable(host->iclk); + pm_runtime_put_sync(host->dev); + pm_runtime_disable(host->dev); clk_put(host->fclk); clk_put(host->iclk); if (host->got_dbclk) { @@ -2100,6 +2087,7 @@ static int omap_hsmmc_suspend(struct device *dev) return 0; if (host) { + pm_runtime_get_sync(host->dev); host->suspended = 1; if (host->pdata->suspend) { ret = host->pdata->suspend(&pdev->dev, @@ -2114,13 +2102,11 @@ static int omap_hsmmc_suspend(struct device *dev) } cancel_work_sync(&host->mmc_carddetect_work); ret = mmc_suspend_host(host->mmc); - mmc_host_enable(host->mmc); + if (ret == 0) { omap_hsmmc_disable_irq(host); OMAP_HSMMC_WRITE(host->base, HCTL, OMAP_HSMMC_READ(host->base, HCTL) & ~SDBP); - mmc_host_disable(host->mmc); - clk_disable(host->iclk); if (host->got_dbclk) clk_disable(host->dbclk); } else { @@ -2132,9 +2118,8 @@ static int omap_hsmmc_suspend(struct device *dev) dev_dbg(mmc_dev(host->mmc), "Unmask interrupt failed\n"); } - mmc_host_disable(host->mmc); } - + pm_runtime_put_sync(host->dev); } return ret; } @@ -2150,14 +2135,7 @@ static int omap_hsmmc_resume(struct device *dev) return 0; if (host) { - ret = clk_enable(host->iclk); - if (ret) - goto clk_en_err; - - if (mmc_host_enable(host->mmc) != 0) { - clk_disable(host->iclk); - goto clk_en_err; - } + pm_runtime_get_sync(host->dev); if (host->got_dbclk) clk_enable(host->dbclk); @@ -2177,14 +2155,13 @@ static int omap_hsmmc_resume(struct device *dev) ret = mmc_resume_host(host->mmc); if (ret == 0) host->suspended = 0; + + pm_runtime_mark_last_busy(host->dev); + pm_runtime_put_autosuspend(host->dev); } return ret; -clk_en_err: - dev_dbg(mmc_dev(host->mmc), - "Failed to enable MMC clocks during resume\n"); - return ret; } #else @@ -2192,9 +2169,33 @@ clk_en_err: #define omap_hsmmc_resume NULL #endif +static int omap_hsmmc_runtime_suspend(struct device *dev) +{ + struct omap_hsmmc_host *host; + + host = platform_get_drvdata(to_platform_device(dev)); + omap_hsmmc_context_save(host); + dev_dbg(mmc_dev(host->mmc), "disabled\n"); + + return 0; +} + +static int omap_hsmmc_runtime_resume(struct device *dev) +{ + struct omap_hsmmc_host *host; + + host = platform_get_drvdata(to_platform_device(dev)); + omap_hsmmc_context_restore(host); + dev_dbg(mmc_dev(host->mmc), "enabled\n"); + + return 0; +} + static struct dev_pm_ops omap_hsmmc_dev_pm_ops = { .suspend = omap_hsmmc_suspend, .resume = omap_hsmmc_resume, + .runtime_suspend = omap_hsmmc_runtime_suspend, + .runtime_resume = omap_hsmmc_runtime_resume, }; static struct platform_driver omap_hsmmc_driver = {
add runtime pm support to HSMMC host controller Use runtime pm API to enable/disable HSMMC clock Use runtime autosuspend APIs to enable auto suspend delay Based on OMAP HSMMC runtime implementation by Kevin Hilman, Kishore Kadiyala Signed-off-by: Balaji T K <balajitk@ti.com> --- changes since v3 remove "host:" prefix for dev_dbg in omap_hsmmc_runtime_resume retain omap_hsmmc_enable/disable_fclk function names to avoid possible merge conflicts with [1] [1] http://www.spinics.net/lists/linux-mmc/msg08836.html changes since v2 Change autosuspend delay to 100ms. changes since v1 Removed pm_runtime_allow, pm_runtime_forbid, pm_suspend_ignore_children calls. drivers/mmc/host/omap_hsmmc.c | 111 +++++++++++++++++++++-------------------- 1 files changed, 56 insertions(+), 55 deletions(-)