| Message ID | 1308752314-32079-3-git-send-email-balajitk@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> > --- > drivers/mmc/host/omap_hsmmc.c | 127 ++++++++++++++++++++++------------------- > 1 files changed, 69 insertions(+), 58 deletions(-) > > diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c > index beb3249..5b81d8b 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 50 > #define MMC_TIMEOUT_MS 20 > #define OMAP_MMC_MASTER_CLOCK 96000000 > #define DRIVER_NAME "omap_hsmmc" > @@ -1149,8 +1151,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); > > @@ -1161,8 +1162,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); > > @@ -1528,7 +1528,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) { > @@ -1623,8 +1623,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) > @@ -1680,32 +1679,28 @@ static void omap_hsmmc_conf_bus_power(struct omap_hsmmc_host *host) > set_sd_bus_power(host); > } > > -static int omap_hsmmc_enable_fclk(struct mmc_host *mmc) > +static int omap_hsmmc_enable(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; > } > > -static int omap_hsmmc_disable_fclk(struct mmc_host *mmc, int lazy) > +static int omap_hsmmc_disable(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; > } > > static const struct mmc_host_ops omap_hsmmc_ops = { > - .enable = omap_hsmmc_enable_fclk, > - .disable = omap_hsmmc_disable_fclk, > + .enable = omap_hsmmc_enable, > + .disable = omap_hsmmc_disable, > .request = omap_hsmmc_request, > .set_ios = omap_hsmmc_set_ios, > .get_cd = omap_hsmmc_get_cd, > @@ -1740,10 +1735,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)); > @@ -1760,7 +1752,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; > } > @@ -1880,18 +1873,12 @@ 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_allow(host->dev); > + pm_runtime_get_sync(host->dev); > + pm_runtime_set_autosuspend_delay(host->dev, MMC_AUTOSUSPEND_DELAY); > + pm_runtime_use_autosuspend(host->dev); > + pm_suspend_ignore_children(host->dev, 1); Why is ignore_children needed for this device? Is this device the parent of other devices? If it is, why should it ignore it's children? > if (cpu_is_omap2430()) { > host->dbclk = clk_get(&pdev->dev, "mmchsdb_fck"); > @@ -2018,6 +2005,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; > > @@ -2033,8 +2022,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) { > @@ -2058,7 +2047,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); > @@ -2069,8 +2058,9 @@ 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_forbid(host->dev); Why? > + pm_runtime_disable(host->dev); > clk_put(host->fclk); > clk_put(host->iclk); > if (host->got_dbclk) { > @@ -2102,6 +2092,8 @@ static int omap_hsmmc_suspend(struct device *dev) > return 0; > > if (host) { > + /* FIXME: TODO move get_sync to proper dev_pm_ops function */ what does this mean? > + pm_runtime_get_sync(host->dev); > host->suspended = 1; > if (host->pdata->suspend) { > ret = host->pdata->suspend(&pdev->dev, > @@ -2116,13 +2108,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 { > @@ -2134,8 +2124,9 @@ static int omap_hsmmc_suspend(struct device *dev) > dev_dbg(mmc_dev(host->mmc), > "Unmask interrupt failed\n"); > } > - mmc_host_disable(host->mmc); > } > + /* FIXME: TODO move put_sync to proper dev_pm_ops function */ ditto > + pm_runtime_put_sync(host->dev); > > } > return ret; > @@ -2152,14 +2143,8 @@ 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; > - } > + /* FIXME: TODO move put_sync to proper dev_pm_ops function */ comment says put_sync, code says get_sync, but again, comment doesn't make any sense. > + pm_runtime_get_sync(host->dev); > > if (host->got_dbclk) > clk_enable(host->dbclk); > @@ -2179,14 +2164,14 @@ static int omap_hsmmc_resume(struct device *dev) > ret = mmc_resume_host(host->mmc); > if (ret == 0) > host->suspended = 0; > + > + /* FIXME: TODO move put_sync to proper dev_pm_ops function */ > + 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 > @@ -2194,9 +2179,35 @@ clk_en_err: > #define omap_hsmmc_resume NULL > #endif > > +static int omap_hsmmc_runtime_suspend(struct device *dev) > +{ > + struct omap_hsmmc_host *host; > + > + extra blank line > + 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; > + > + extra blank line > + host = platform_get_drvdata(to_platform_device(dev)); > + omap_hsmmc_context_restore(host); > + dev_dbg(mmc_dev(host->mmc), "host: 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 = { Kevin -- 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 Thu, Jun 23, 2011 at 12:08 AM, Kevin Hilman <khilman@ti.com> wrote: > Balaji T K <balajitk@ti.com> writes: > >> @@ -1880,18 +1873,12 @@ 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_allow(host->dev); >> + pm_runtime_get_sync(host->dev); >> + pm_runtime_set_autosuspend_delay(host->dev, MMC_AUTOSUSPEND_DELAY); >> + pm_runtime_use_autosuspend(host->dev); >> + pm_suspend_ignore_children(host->dev, 1); > > Why is ignore_children needed for this device? Is this device the > parent of other devices? If it is, why should it ignore it's > children? > No, I will remove. Added it for testing only. >> if (cpu_is_omap2430()) { >> host->dbclk = clk_get(&pdev->dev, "mmchsdb_fck"); >> @@ -2018,6 +2005,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; >> >> @@ -2033,8 +2022,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) { >> @@ -2058,7 +2047,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); >> @@ -2069,8 +2058,9 @@ 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_forbid(host->dev); > > Why? > Added for balancing pm_runtime_allow added in _probe. But forbid also resume the device on remove. Should this be removed, keeping _allow in _probe ? >> + pm_runtime_disable(host->dev); >> clk_put(host->fclk); >> clk_put(host->iclk); >> if (host->got_dbclk) { >> @@ -2102,6 +2092,8 @@ static int omap_hsmmc_suspend(struct device *dev) >> return 0; >> >> if (host) { >> + /* FIXME: TODO move get_sync to proper dev_pm_ops function */ > > what does this mean? get_sync is needed to enable clock before accessing the registers but the discusssion @ http://www.mail-archive.com/linux-omap@vger.kernel.org/msg50819.html suggested to move runtime get_sync calls to .prepare Haven't tried it yet. > >> + pm_runtime_get_sync(host->dev); >> host->suspended = 1; >> if (host->pdata->suspend) { >> ret = host->pdata->suspend(&pdev->dev, >> @@ -2116,13 +2108,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 { >> @@ -2134,8 +2124,9 @@ static int omap_hsmmc_suspend(struct device *dev) >> dev_dbg(mmc_dev(host->mmc), >> "Unmask interrupt failed\n"); >> } >> - mmc_host_disable(host->mmc); >> } >> + /* FIXME: TODO move put_sync to proper dev_pm_ops function */ > > ditto > >> + pm_runtime_put_sync(host->dev); >> >> } >> return ret; >> @@ -2152,14 +2143,8 @@ 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; >> - } >> + /* FIXME: TODO move put_sync to proper dev_pm_ops function */ > > comment says put_sync, code says get_sync, but again, comment doesn't > make any sense. > >> + pm_runtime_get_sync(host->dev); >> >> if (host->got_dbclk) >> clk_enable(host->dbclk); >> @@ -2179,14 +2164,14 @@ static int omap_hsmmc_resume(struct device *dev) >> ret = mmc_resume_host(host->mmc); >> if (ret == 0) >> host->suspended = 0; >> + >> + /* FIXME: TODO move put_sync to proper dev_pm_ops function */ >> + 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 >> @@ -2194,9 +2179,35 @@ clk_en_err: >> #define omap_hsmmc_resume NULL >> #endif >> >> +static int omap_hsmmc_runtime_suspend(struct device *dev) >> +{ >> + struct omap_hsmmc_host *host; >> + >> + > > extra blank line Removed > >> + 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; >> + >> + > > extra blank line Removed -- 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
"T Krishnamoorthy, Balaji" <balajitk@ti.com> writes: > On Thu, Jun 23, 2011 at 12:08 AM, Kevin Hilman <khilman@ti.com> wrote: >> Balaji T K <balajitk@ti.com> writes: >> > >>> @@ -1880,18 +1873,12 @@ 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_allow(host->dev); >>> + pm_runtime_get_sync(host->dev); >>> + pm_runtime_set_autosuspend_delay(host->dev, MMC_AUTOSUSPEND_DELAY); >>> + pm_runtime_use_autosuspend(host->dev); >>> + pm_suspend_ignore_children(host->dev, 1); >> >> Why is ignore_children needed for this device? Is this device the >> parent of other devices? If it is, why should it ignore it's >> children? >> > > No, I will remove. Added it for testing only. > >>> if (cpu_is_omap2430()) { >>> host->dbclk = clk_get(&pdev->dev, "mmchsdb_fck"); >>> @@ -2018,6 +2005,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; >>> >>> @@ -2033,8 +2022,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) { >>> @@ -2058,7 +2047,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); >>> @@ -2069,8 +2058,9 @@ 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_forbid(host->dev); >> >> Why? >> > > Added for balancing pm_runtime_allow added in _probe. > But forbid also resume the device on remove. > Should this be removed, keeping _allow in _probe ? Neither the _allow or _forbid are needed, _enable and _disable are enough. >>> + pm_runtime_disable(host->dev); >>> clk_put(host->fclk); >>> clk_put(host->iclk); >>> if (host->got_dbclk) { >>> @@ -2102,6 +2092,8 @@ static int omap_hsmmc_suspend(struct device *dev) >>> return 0; >>> >>> if (host) { >>> + /* FIXME: TODO move get_sync to proper dev_pm_ops function */ >> >> what does this mean? > > get_sync is needed to enable clock before accessing the registers but > the discusssion @ > http://www.mail-archive.com/linux-omap@vger.kernel.org/msg50819.html > suggested to move runtime get_sync calls to .prepare > Haven't tried it yet. The _get is fine here, it's the _put that may be the problem. Based on that thread you mentioned, it is the using of _put and _put_sync in the suspend path that is the problem. Basically, use of runtime PM calls in the suspend/resume path is not recommended and not guaranteed to work. It currently works on OMAP, but I may have to change this. For now, what is certain is that runtime PM calls in the suspend callbacks must be the _sync versions. I'm still working on how to properly implement the PM domain part for OMAP to correctly implement the restrictions that the linux-pm maintainers want to enforce. Kevin -- 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
(cc'ing Adrian also)
Hi Balaji
On Wed, 22 Jun 2011, Balaji T K wrote:
> Use runtime autosuspend APIs to enable auto suspend delay
Does this really need to use runtime autosuspend? Seems to me that since
PM runtime is just controlling the MMC IP blocks and not the regulators in
this instance, this could simply use pm_runtime_put*() and just avoid the
extra power wastage and complexity involved in autosuspend.
Besides, the correct autosuspend timeout seems use-case dependent.
By the way, if possible, it would be really good if you could update this
patch series soon to address the comments. If these MMC driver changes
can be ready to merge for 3.1, then we also have a chance of
integrating some hwmod cleanup patches that depend on this driver's PM
runtime conversion.
- Paul
--
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, Jun 28, 2011 at 10:52 PM, Paul Walmsley <paul@pwsan.com> wrote: > (cc'ing Adrian also) > > Hi Balaji > > On Wed, 22 Jun 2011, Balaji T K wrote: > >> Use runtime autosuspend APIs to enable auto suspend delay > > Does this really need to use runtime autosuspend? Seems to me that since > PM runtime is just controlling the MMC IP blocks and not the regulators in > this instance, this could simply use pm_runtime_put*() and just avoid the > extra power wastage and complexity involved in autosuspend. > I have seen some instabilities if delay is very less, on some production boards. The previous implementation used 100ms delay before disabling the clocks. Is there any specific issue I should be aware of, for using _autosuspend ? > Besides, the correct autosuspend timeout seems use-case dependent. > > By the way, if possible, it would be really good if you could update this > patch series soon to address the comments. If these MMC driver changes > can be ready to merge for 3.1, then we also have a chance of > integrating some hwmod cleanup patches that depend on this driver's PM > runtime conversion. > > Just noticed your mail after I posted my patches.. -- 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, 28 Jun 2011, T Krishnamoorthy, Balaji wrote: > On Tue, Jun 28, 2011 at 10:52 PM, Paul Walmsley <paul@pwsan.com> wrote: > > > > On Wed, 22 Jun 2011, Balaji T K wrote: > > > >> Use runtime autosuspend APIs to enable auto suspend delay > > > > Does this really need to use runtime autosuspend? Seems to me that since > > PM runtime is just controlling the MMC IP blocks and not the regulators in > > this instance, this could simply use pm_runtime_put*() and just avoid the > > extra power wastage and complexity involved in autosuspend. > > I have seen some instabilities if delay is very less, on some production boards. Could you be more specific? What type of instabilities did you see? > The previous implementation used 100ms delay before disabling the clocks. > Is there any specific issue I should be aware of, for using _autosuspend ? - Paul
"T Krishnamoorthy, Balaji" <balajitk@ti.com> writes: > On Tue, Jun 28, 2011 at 10:52 PM, Paul Walmsley <paul@pwsan.com> wrote: >> (cc'ing Adrian also) >> >> Hi Balaji >> >> On Wed, 22 Jun 2011, Balaji T K wrote: >> >>> Use runtime autosuspend APIs to enable auto suspend delay >> >> Does this really need to use runtime autosuspend? Seems to me that since >> PM runtime is just controlling the MMC IP blocks and not the regulators in >> this instance, this could simply use pm_runtime_put*() and just avoid the >> extra power wastage and complexity involved in autosuspend. >> > > I have seen some instabilities if delay is very less, on some production boards. > The previous implementation used 100ms delay before disabling the clocks. And your new one is using 50ms. How did this value come about? As Paul mentioned, the timeout value here is probably usecase depeend Kevin -- 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, Jun 29, 2011 at 12:11 AM, Paul Walmsley <paul@pwsan.com> wrote: > On Tue, 28 Jun 2011, T Krishnamoorthy, Balaji wrote: > >> On Tue, Jun 28, 2011 at 10:52 PM, Paul Walmsley <paul@pwsan.com> wrote: >> > >> > On Wed, 22 Jun 2011, Balaji T K wrote: >> > >> >> Use runtime autosuspend APIs to enable auto suspend delay >> > >> > Does this really need to use runtime autosuspend? Seems to me that since >> > PM runtime is just controlling the MMC IP blocks and not the regulators in >> > this instance, this could simply use pm_runtime_put*() and just avoid the >> > extra power wastage and complexity involved in autosuspend. >> >> I have seen some instabilities if delay is very less, on some production boards. > > Could you be more specific? What type of instabilities did you see? There have been some experiments on our customer programs to reduce this value to a few ms and infrequent crashes were observed (stress testing for several hours) while trying to access the controller registers. > >> The previous implementation used 100ms delay before disabling the clocks. >> Is there any specific issue I should be aware of, for using _autosuspend ? > > > - Paul -- 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, Jun 29, 2011 at 2:00 AM, Kevin Hilman <khilman@ti.com> wrote: > "T Krishnamoorthy, Balaji" <balajitk@ti.com> writes: > >> On Tue, Jun 28, 2011 at 10:52 PM, Paul Walmsley <paul@pwsan.com> wrote: >>> (cc'ing Adrian also) >>> >>> Hi Balaji >>> >>> On Wed, 22 Jun 2011, Balaji T K wrote: >>> >>>> Use runtime autosuspend APIs to enable auto suspend delay >>> >>> Does this really need to use runtime autosuspend? Seems to me that since >>> PM runtime is just controlling the MMC IP blocks and not the regulators in >>> this instance, this could simply use pm_runtime_put*() and just avoid the >>> extra power wastage and complexity involved in autosuspend. >>> >> >> I have seen some instabilities if delay is very less, on some production boards. >> The previous implementation used 100ms delay before disabling the clocks. > > And your new one is using 50ms. How did this value come about? > I don't have any specific affinity to this number, but when requests are bursty, they arrive within a few 10s of ms within each other. Didn't want to have the context/save restore penalty associated with every request. > As Paul mentioned, the timeout value here is probably usecase depeend > There is no direct relationship to the use case. Block layer buffers and reworks the order of requests and they are usually queued together. Having no context save / restore penalty for each request is definitely desirable. (As I understand, MMC can lose context independent of MPU on OMAP4). -- 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, 29 Jun 2011, T Krishnamoorthy, Balaji wrote: > On Wed, Jun 29, 2011 at 12:11 AM, Paul Walmsley <paul@pwsan.com> wrote: > > On Tue, 28 Jun 2011, T Krishnamoorthy, Balaji wrote: > > > >> On Tue, Jun 28, 2011 at 10:52 PM, Paul Walmsley <paul@pwsan.com> wrote: > >> > > >> > On Wed, 22 Jun 2011, Balaji T K wrote: > >> > > >> >> Use runtime autosuspend APIs to enable auto suspend delay > >> > > >> > Does this really need to use runtime autosuspend? Seems to me that since > >> > PM runtime is just controlling the MMC IP blocks and not the regulators in > >> > this instance, this could simply use pm_runtime_put*() and just avoid the > >> > extra power wastage and complexity involved in autosuspend. > >> > >> I have seen some instabilities if delay is very less, on some production boards. > > > > Could you be more specific? What type of instabilities did you see? > > There have been some experiments on our customer programs to reduce this > value to a few ms and infrequent crashes were observed (stress testing > for several hours) while trying to access the controller registers. Was there an oops? Any analysis, etc.? - Paul
On Wed, 29 Jun 2011, T Krishnamoorthy, Balaji wrote: > There have been some experiments on our customer programs to reduce this > value to a few ms and infrequent crashes were observed (stress testing > for several hours) while trying to access the controller registers. By the way, could you send along a copy of the stress test script? - Paul -- 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, Jun 29, 2011 at 8:12 PM, Paul Walmsley <paul@pwsan.com> wrote: > On Wed, 29 Jun 2011, T Krishnamoorthy, Balaji wrote: > >> On Wed, Jun 29, 2011 at 12:11 AM, Paul Walmsley <paul@pwsan.com> wrote: >> > On Tue, 28 Jun 2011, T Krishnamoorthy, Balaji wrote: >> > >> >> On Tue, Jun 28, 2011 at 10:52 PM, Paul Walmsley <paul@pwsan.com> wrote: >> >> > >> >> > On Wed, 22 Jun 2011, Balaji T K wrote: >> >> > >> >> >> Use runtime autosuspend APIs to enable auto suspend delay >> >> > >> >> > Does this really need to use runtime autosuspend? Seems to me that since >> >> > PM runtime is just controlling the MMC IP blocks and not the regulators in >> >> > this instance, this could simply use pm_runtime_put*() and just avoid the >> >> > extra power wastage and complexity involved in autosuspend. >> >> >> >> I have seen some instabilities if delay is very less, on some production boards. >> > >> > Could you be more specific? What type of instabilities did you see? >> >> There have been some experiments on our customer programs to reduce this >> value to a few ms and infrequent crashes were observed (stress testing >> for several hours) while trying to access the controller registers. > > Was there an oops? Any analysis, etc.? OOPS pointed to omap_hsmmc_prepare_data / set_data_timeout Use case was MMC + SDIO +GPS activity, on kernel 2.6.35 though. Unhandled fault: imprecise external abort (0x1406) at 0x4073102c Internal error: : 1406 [#1] PREEMPT SMP last sysfs file: /sys/devices/platform/switch-sio/usb_sel Modules linked in: param(P) j4fs(P) vibetonz Si4709_driver fm_drv(C) bt_drv(C) st_drv(C) CPU: 0 Tainted: P WC (2.6.35.7 #2) PC is at clk_get_rate+0x4/0x48 LR is at set_data_timeout+0x68/0xf4 [<c06e78e0>] (set_data_timeout+0x0/0xf4) from [<c06e7dc8>] (omap_hsmmc_request+0x2d0/0x5c8) r8:efa78400 r7:00000001 r6:00000000 r5:ef9efe78 r4:efa78640 r3:ef9efee4 [<c06e7af8>] (omap_hsmmc_request+0x0/0x5c8) from [<c06df040>] (mmc_wait_for_req+0x118/0x130) [<c06def28>] (mmc_wait_for_req+0x0/0x130) from [<c06e59e8>] (mmc_blk_issue_rq+0x1c0/0x500) r6:ef86f000 r5:efa79000 r4:c91e61a0 [<c06e5828>] (mmc_blk_issue_rq+0x0/0x500) from [<c06e6620>] (mmc_queue_thread+0xf4/0xf8) [<c06e652c>] (mmc_queue_thread+0x0/0xf8) from [<c045ddec>] (kthread+0x84/0x8c) [<c045dd68>] (kthread+0x0/0x8c) from [<c044b748>] (do_exit+0x0/0x604) r7:00000013 r6:c044b748 r5:c045dd68 r4:ef8d5d68 Code: e1a00004 e89da8f0 c0a653c0 e1a0c00d (e92dd818) ---[ end trace 533b4c955f5abafd ]--- > > > - Paul -- 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, Jun 29, 2011 at 9:08 PM, Paul Walmsley <paul@pwsan.com> wrote: > On Wed, 29 Jun 2011, T Krishnamoorthy, Balaji wrote: > >> There have been some experiments on our customer programs to reduce this >> value to a few ms and infrequent crashes were observed (stress testing >> for several hours) while trying to access the controller registers. > > By the way, could you send along a copy of the stress test script? > Paul, these scenarios are not scripted but end user tests with additional devices (WLAN, which is connected on the same controller) and executed 'on field' . One such log is here .. http://pastebin.com/nq3cfZnT -- 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
"T Krishnamoorthy, Balaji" <balajitk@ti.com> writes: > On Wed, Jun 29, 2011 at 2:00 AM, Kevin Hilman <khilman@ti.com> wrote: >> "T Krishnamoorthy, Balaji" <balajitk@ti.com> writes: >> >>> On Tue, Jun 28, 2011 at 10:52 PM, Paul Walmsley <paul@pwsan.com> wrote: >>>> (cc'ing Adrian also) >>>> >>>> Hi Balaji >>>> >>>> On Wed, 22 Jun 2011, Balaji T K wrote: >>>> >>>>> Use runtime autosuspend APIs to enable auto suspend delay >>>> >>>> Does this really need to use runtime autosuspend? Seems to me that since >>>> PM runtime is just controlling the MMC IP blocks and not the regulators in >>>> this instance, this could simply use pm_runtime_put*() and just avoid the >>>> extra power wastage and complexity involved in autosuspend. >>>> >>> >>> I have seen some instabilities if delay is very less, on some production boards. >>> The previous implementation used 100ms delay before disabling the clocks. >> >> And your new one is using 50ms. How did this value come about? >> > I don't have any specific affinity to this number, but when requests > are bursty, they arrive within a few 10s of ms within each > other. Didn't want to have the context/save restore penalty associated > with every request. Have you measured the context save/restore penalty when only the clocks are gated (and no regulators involved) ? In the current code, it's understandable why there were large latencies that were avoided because the regulators were also cut. With only the clocks being cut, the latency involved will be significantly less. >> As Paul mentioned, the timeout value here is probably usecase depeend >> > > There is no direct relationship to the use case. Block layer buffers > and reworks the order of requests and they are usually queued > together. Having no context save / restore penalty for each request is > definitely desirable. Desirable for what? What is implied in that statement is desirable for performance. What if a different user would prefer the power savings gained by more aggressively cuttting clocks over the performance gains of leaving clocks enabled? Kevin -- 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
+ Venkat Hi Balaji, Venkat, On Wed, 29 Jun 2011, T Krishnamoorthy, Balaji wrote: > OOPS pointed to omap_hsmmc_prepare_data / set_data_timeout > Use case was MMC + SDIO +GPS activity, on kernel 2.6.35 though. > > Unhandled fault: imprecise external abort (0x1406) at 0x4073102c ^^^^^^^^^ That's not good... > Internal error: : 1406 [#1] PREEMPT SMP > last sysfs file: /sys/devices/platform/switch-sio/usb_sel > Modules linked in: param(P) j4fs(P) vibetonz Si4709_driver fm_drv(C) > bt_drv(C) st_drv(C) > CPU: 0 Tainted: P WC (2.6.35.7 #2) ^^^^^^^^^^ Neither is this... > PC is at clk_get_rate+0x4/0x48 > LR is at set_data_timeout+0x68/0xf4 > [<c06e78e0>] (set_data_timeout+0x0/0xf4) from [<c06e7dc8>] > (omap_hsmmc_request+0x2d0/0x5c8) > r8:efa78400 r7:00000001 r6:00000000 r5:ef9efe78 r4:efa78640 > r3:ef9efee4 > [<c06e7af8>] (omap_hsmmc_request+0x0/0x5c8) from [<c06df040>] > (mmc_wait_for_req+0x118/0x130) > [<c06def28>] (mmc_wait_for_req+0x0/0x130) from [<c06e59e8>] > (mmc_blk_issue_rq+0x1c0/0x500) > r6:ef86f000 r5:efa79000 r4:c91e61a0 > [<c06e5828>] (mmc_blk_issue_rq+0x0/0x500) from [<c06e6620>] > (mmc_queue_thread+0xf4/0xf8) > [<c06e652c>] (mmc_queue_thread+0x0/0xf8) from [<c045ddec>] (kthread+0x84/0x8c) > [<c045dd68>] (kthread+0x0/0x8c) from [<c044b748>] (do_exit+0x0/0x604) > r7:00000013 r6:c044b748 r5:c045dd68 r4:ef8d5d68 Was CONFIG_PM enabled? If so, was off-mode enabled for any IP blocks, or was it just clock-stop and/or retention? - Paul -- 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
cc'ing lakml Hi Venkat, Balaji, On Wed, 29 Jun 2011, S, Venkatraman wrote: > On Wed, Jun 29, 2011 at 9:08 PM, Paul Walmsley <paul@pwsan.com> wrote: > > On Wed, 29 Jun 2011, T Krishnamoorthy, Balaji wrote: > > > >> There have been some experiments on our customer programs to reduce this > >> value to a few ms and infrequent crashes were observed (stress testing > >> for several hours) while trying to access the controller registers. > > > > By the way, could you send along a copy of the stress test script? > > > > Paul, these scenarios are not scripted but end user tests with > additional devices > (WLAN, which is connected on the same controller) and executed 'on field' . OK, thanks Venkat. Do you still have one of these devices so the test can be repeated? > One such log is here .. http://pastebin.com/nq3cfZnT Looks like this is an Android 2.6.35.7-derived kernel on a 4430 ES2.2 EMU. Power management is enabled but MPU, L3INIT, and PER aren't entering any deeper power states than retention idle, so no context save/restore or off-mode worries here. The system looks like it's entered suspend at least once and resumed, before the oops. Also the second CPU is starting up and shutting down dynamically. Backtrace is copied below for the archives. Does the above summary match your understanding? ... Reviewing this backtrace and the one that Balaji sent, it looks to me like this write in omap_hsmmc_prepare_data() is the proximate cause of the abort: OMAP_HSMMC_WRITE(host->base, BLK, (req->data->blksz) | (req->data->blocks << 16)); I'll bet this was first access to the MMC IP block after the MMC layer re-enabled it. The abort is imprecise because the Linux OMAP4 kernel marks MMIO registers as bufferable, so the ARM can continue executing while a register write is making its way across the OMAP interconnect(s). This guess also assumes that the ARM is executing instructions out of order, which is a reasonable assumption on a Cortex-A9. This could be confirmed by reading some HSMMC register right before the OMAP_HSMMC_WRITE(); then the abort would turn precise and occur on the read. Anyway, it looks like the HSMMC IP block wasn't yet ready to be accessed. Probably, this is because either the HSMMC IP block hasn't yet left the Idle or SleepTrans states, and the OMAP4 clock framework doesn't wait for that; or the PRCM is getting confused because the correct clockdomain enable sequence isn't being followed -- see for example the "Fix module-mode enable sequence on OMAP4" patch series that have been posted to the linux-omap mailing list. Probably one of those two issues is the root cause. If you have a testing setup where you can reproduce this problem, I'd suggest adding the read as described above. Otherwise, I don't think this will be an issue for the runtime PM conversion: first, because the hwmod code will wait for the HSMMC block to indicate that it has left idle before continuing; and second, because we'll hopefully have a patch series going in at the same time to make sure the clockdomain enable sequence is correct. - Paul <0> Process mmcqd (pid: 851, stack limit = 0xef9682f8) <0> Stack: (0xef969db8 to 0xef96a000) <0> 9da0: ef969ee4 efa30640 <0> 9dc0: ef969e78 00000000 00000001 efa30400 ef969e2c ef969de0 c06ae2b8 c06ace10 <0> 9de0: 00000000 efa305d8 ef969e04 efa30400 00000000 efa30578 ef969e44 ef969e08 <0> 9e00: c054ea5c ef969e78 efa30400 ef969e34 00000001 ef837e4c 00000000 ef969ee4 <0> 9e20: ef969e64 ef969e30 c06a54d8 c06adff4 00000000 00000000 00000000 00000000 <0> 9e40: ef969e40 ef969e40 ed3d4680 ed3d4680 efa30c00 ef837e40 ef969f94 ef969e68 <0> 9e60: c06abe80 c06a53cc 00000000 efa31458 ef0cfdb4 ef0cfdb4 ef969e8c ef969ee4 <0> 9e80: ef969eb8 ef969e34 c06a55d0 00000019 00fd50a2 00000000 00000000 00000000 <0> 9ea0: 00000000 000000b5 00000000 00000000 ef969ee4 ef969e78 0000000c 00000000 <0> 9ec0: 00000000 00000000 00000000 00000000 0000049d 00000000 00000000 00000000 <0> 9ee0: ef969e78 23c34600 00000fa0 00000200 00000400 00000000 00000100 00000000 <0> 9f00: ef969eb8 ef969e78 0000003f ef238000 ef969f54 ef969f20 c0556c00 c0555fac <0> 9f20: ef969f3c 00000001 c0425fa0 ef837e4c ef230000 ef837e54 ef837e4c ef230000 <0> 9f40: ef837e54 ef230000 ef969f7c ef969f58 00000000 ed3d4680 ef969f7c ef969f68 <0> 9f60: c054911c c054ee7c 01082e21 ef837e4c ef968000 ef837e54 ef230000 ef2301d8 <0> 9f80: 00000000 ed3d4680 ef969fbc ef969f98 c06acab8 c06abccc ef985d68 ef969fc4 <0> 9fa0: c06ac9c4 ef837e4c 00000000 00000000 ef969ff4 ef969fc0 c041fd20 c06ac9d0 <0> 9fc0: 00000000 00000000 00000000 00000000 ef969fd0 ef969fd0 ef985d68 c041fc9c <0> 9fe0: c040d67c 00000013 00000000 ef969ff8 c040d67c c041fca8 00000000 00000000 <4> Backtrace: <4> [<c06ace04>] (set_data_timeout+0x0/0xcc) from [<c06ae2b8>] (omap_hsmmc_request+0x2d0/0x5c8) <4> r8:efa30400 r7:00000001 r6:00000000 r5:ef969e78 r4:efa30640 <4> r3:ef969ee4 <4> [<c06adfe8>] (omap_hsmmc_request+0x0/0x5c8) from [<c06a54d8>] (mmc_wait_for_req+0x118/0x130) <4> [<c06a53c0>] (mmc_wait_for_req+0x0/0x130) from [<c06abe80>] (mmc_blk_issue_rq+0x1c0/0x500) <4> r6:ef837e40 r5:efa30c00 r4:ed3d4680 <4> [<c06abcc0>] (mmc_blk_issue_rq+0x0/0x500) from [<c06acab8>] (mmc_queue_thread+0xf4/0xf8) <4> [<c06ac9c4>] (mmc_queue_thread+0x0/0xf8) from [<c041fd20>] (kthread+0x84/0x8c) <4> [<c041fc9c>] (kthread+0x0/0x8c) from [<c040d67c>] (do_exit+0x0/0x604) <4> r7:00000013 r6:c040d67c r5:c041fc9c r4:ef985d68 <0> Code: 11a0c423 11c0c0b0 e1a0f00e e2512001 (01a0f00e) <4> ---[ end trace d27fcce5bd5b71d6 ]--- -- 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
+ Venkat Hi Balaji On Wed, 29 Jun 2011, T Krishnamoorthy, Balaji wrote: > On Wed, Jun 29, 2011 at 2:00 AM, Kevin Hilman <khilman@ti.com> wrote: > > "T Krishnamoorthy, Balaji" <balajitk@ti.com> writes: > >> > >> I have seen some instabilities if delay is very less, on some > >> production boards. The previous implementation used 100ms delay > >> before disabling the clocks. > > > > And your new one is using 50ms. How did this value come about? > > I don't have any specific affinity to this number, but when requests are > bursty, they arrive within a few 10s of ms within each other. Didn't > want to have the context/save restore penalty associated with every > request. Kevin and I just chatted a little bit about this. It seems best to separate the work done on the autosuspend timeout from the runtime PM conversion. So how about this: please send a new version of these patches with the previous value, 100ms, for the autosuspend timeout. That should hopefully minimize the behavior change here for existing users. And hopefully we'll be able to get the series in for this merge window. Then later, we need to come back to this autosuspend timeout issue. - Paul
On Thu, Jun 30, 2011 at 1:37 AM, Paul Walmsley <paul@pwsan.com> wrote: > cc'ing lakml > > Hi Venkat, Balaji, > > On Wed, 29 Jun 2011, S, Venkatraman wrote: > >> On Wed, Jun 29, 2011 at 9:08 PM, Paul Walmsley <paul@pwsan.com> wrote: >> > On Wed, 29 Jun 2011, T Krishnamoorthy, Balaji wrote: >> > >> >> There have been some experiments on our customer programs to reduce this >> >> value to a few ms and infrequent crashes were observed (stress testing >> >> for several hours) while trying to access the controller registers. >> > >> > By the way, could you send along a copy of the stress test script? >> > >> >> Paul, these scenarios are not scripted but end user tests with >> additional devices >> (WLAN, which is connected on the same controller) and executed 'on field' . > > OK, thanks Venkat. Do you still have one of these devices so the test can > be repeated? > >> One such log is here .. http://pastebin.com/nq3cfZnT > > Looks like this is an Android 2.6.35.7-derived kernel on a 4430 ES2.2 EMU. > Power management is enabled but MPU, L3INIT, and PER aren't entering any > deeper power states than retention idle, so no context save/restore or > off-mode worries here. > > The system looks like it's entered suspend at least once and resumed, > before the oops. Also the second CPU is starting up and shutting down > dynamically. Backtrace is copied below for the archives. > > Does the above summary match your understanding? Yes it does. > > ... > > Reviewing this backtrace and the one that Balaji sent, it looks to > me like this write in omap_hsmmc_prepare_data() is the proximate > cause of the abort: > > OMAP_HSMMC_WRITE(host->base, BLK, (req->data->blksz) > | (req->data->blocks << 16)); > > I'll bet this was first access to the MMC IP block after the MMC layer > re-enabled it. The abort is imprecise because the Linux OMAP4 kernel > marks MMIO registers as bufferable, so the ARM can continue executing > while a register write is making its way across the OMAP interconnect(s). > This guess also assumes that the ARM is executing instructions out of > order, which is a reasonable assumption on a Cortex-A9. This could be > confirmed by reading some HSMMC register right before the > OMAP_HSMMC_WRITE(); then the abort would turn precise and occur on the > read. Yes - The issue is not an issue with the set_data_timeout function but the _first_ access to MMC IP register blocks after enabling the mmc_host. (This backtrace signature is very common during MMC-PM hackathons ) But I have not seen any difference if the access is read or write.. Will check again.. > > Anyway, it looks like the HSMMC IP block wasn't yet ready to be accessed. > Probably, this is because either the HSMMC IP block hasn't yet left the > Idle or SleepTrans states, and the OMAP4 clock framework doesn't wait for > that; or the PRCM is getting confused because the correct clockdomain > enable sequence isn't being followed -- see for example the "Fix > module-mode enable sequence on OMAP4" patch series that have been posted > to the linux-omap mailing list. Probably one of those two issues is the > root cause. > > If you have a testing setup where you can reproduce this problem, I'd > suggest adding the read as described above. Otherwise, I don't think this > will be an issue for the runtime PM conversion: first, because the hwmod > code will wait for the HSMMC block to indicate that it has left idle > before continuing; and second, because we'll hopefully have a patch series > going in at the same time to make sure the clockdomain enable sequence is > correct. > As you might have guessed, the test setup is not accessible for me and it's not a simulated environment or scripted test. I'll try to check if some testcases can be written to simulate this. > > - Paul > > > <0> Process mmcqd (pid: 851, stack limit = 0xef9682f8) > <0> Stack: (0xef969db8 to 0xef96a000) > <0> 9da0: ef969ee4 efa30640 > <0> 9dc0: ef969e78 00000000 00000001 efa30400 ef969e2c ef969de0 c06ae2b8 c06ace10 > <0> 9de0: 00000000 efa305d8 ef969e04 efa30400 00000000 efa30578 ef969e44 ef969e08 > <0> 9e00: c054ea5c ef969e78 efa30400 ef969e34 00000001 ef837e4c 00000000 ef969ee4 > <0> 9e20: ef969e64 ef969e30 c06a54d8 c06adff4 00000000 00000000 00000000 00000000 > <0> 9e40: ef969e40 ef969e40 ed3d4680 ed3d4680 efa30c00 ef837e40 ef969f94 ef969e68 > <0> 9e60: c06abe80 c06a53cc 00000000 efa31458 ef0cfdb4 ef0cfdb4 ef969e8c ef969ee4 > <0> 9e80: ef969eb8 ef969e34 c06a55d0 00000019 00fd50a2 00000000 00000000 00000000 > <0> 9ea0: 00000000 000000b5 00000000 00000000 ef969ee4 ef969e78 0000000c 00000000 > <0> 9ec0: 00000000 00000000 00000000 00000000 0000049d 00000000 00000000 00000000 > <0> 9ee0: ef969e78 23c34600 00000fa0 00000200 00000400 00000000 00000100 00000000 > <0> 9f00: ef969eb8 ef969e78 0000003f ef238000 ef969f54 ef969f20 c0556c00 c0555fac > <0> 9f20: ef969f3c 00000001 c0425fa0 ef837e4c ef230000 ef837e54 ef837e4c ef230000 > <0> 9f40: ef837e54 ef230000 ef969f7c ef969f58 00000000 ed3d4680 ef969f7c ef969f68 > <0> 9f60: c054911c c054ee7c 01082e21 ef837e4c ef968000 ef837e54 ef230000 ef2301d8 > <0> 9f80: 00000000 ed3d4680 ef969fbc ef969f98 c06acab8 c06abccc ef985d68 ef969fc4 > <0> 9fa0: c06ac9c4 ef837e4c 00000000 00000000 ef969ff4 ef969fc0 c041fd20 c06ac9d0 > <0> 9fc0: 00000000 00000000 00000000 00000000 ef969fd0 ef969fd0 ef985d68 c041fc9c > <0> 9fe0: c040d67c 00000013 00000000 ef969ff8 c040d67c c041fca8 00000000 00000000 > <4> Backtrace: > <4> [<c06ace04>] (set_data_timeout+0x0/0xcc) from [<c06ae2b8>] (omap_hsmmc_request+0x2d0/0x5c8) > <4> r8:efa30400 r7:00000001 r6:00000000 r5:ef969e78 r4:efa30640 > <4> r3:ef969ee4 > <4> [<c06adfe8>] (omap_hsmmc_request+0x0/0x5c8) from [<c06a54d8>] (mmc_wait_for_req+0x118/0x130) > <4> [<c06a53c0>] (mmc_wait_for_req+0x0/0x130) from [<c06abe80>] (mmc_blk_issue_rq+0x1c0/0x500) > <4> r6:ef837e40 r5:efa30c00 r4:ed3d4680 > <4> [<c06abcc0>] (mmc_blk_issue_rq+0x0/0x500) from [<c06acab8>] (mmc_queue_thread+0xf4/0xf8) > <4> [<c06ac9c4>] (mmc_queue_thread+0x0/0xf8) from [<c041fd20>] (kthread+0x84/0x8c) > <4> [<c041fc9c>] (kthread+0x0/0x8c) from [<c040d67c>] (do_exit+0x0/0x604) > <4> r7:00000013 r6:c040d67c r5:c041fc9c r4:ef985d68 > <0> Code: 11a0c423 11c0c0b0 e1a0f00e e2512001 (01a0f00e) > <4> ---[ end trace d27fcce5bd5b71d6 ]--- > > > -- 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/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c index beb3249..5b81d8b 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 50 #define MMC_TIMEOUT_MS 20 #define OMAP_MMC_MASTER_CLOCK 96000000 #define DRIVER_NAME "omap_hsmmc" @@ -1149,8 +1151,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); @@ -1161,8 +1162,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); @@ -1528,7 +1528,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) { @@ -1623,8 +1623,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) @@ -1680,32 +1679,28 @@ static void omap_hsmmc_conf_bus_power(struct omap_hsmmc_host *host) set_sd_bus_power(host); } -static int omap_hsmmc_enable_fclk(struct mmc_host *mmc) +static int omap_hsmmc_enable(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; } -static int omap_hsmmc_disable_fclk(struct mmc_host *mmc, int lazy) +static int omap_hsmmc_disable(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; } static const struct mmc_host_ops omap_hsmmc_ops = { - .enable = omap_hsmmc_enable_fclk, - .disable = omap_hsmmc_disable_fclk, + .enable = omap_hsmmc_enable, + .disable = omap_hsmmc_disable, .request = omap_hsmmc_request, .set_ios = omap_hsmmc_set_ios, .get_cd = omap_hsmmc_get_cd, @@ -1740,10 +1735,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)); @@ -1760,7 +1752,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; } @@ -1880,18 +1873,12 @@ 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_allow(host->dev); + pm_runtime_get_sync(host->dev); + pm_runtime_set_autosuspend_delay(host->dev, MMC_AUTOSUSPEND_DELAY); + pm_runtime_use_autosuspend(host->dev); + pm_suspend_ignore_children(host->dev, 1); if (cpu_is_omap2430()) { host->dbclk = clk_get(&pdev->dev, "mmchsdb_fck"); @@ -2018,6 +2005,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; @@ -2033,8 +2022,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) { @@ -2058,7 +2047,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); @@ -2069,8 +2058,9 @@ 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_forbid(host->dev); + pm_runtime_disable(host->dev); clk_put(host->fclk); clk_put(host->iclk); if (host->got_dbclk) { @@ -2102,6 +2092,8 @@ static int omap_hsmmc_suspend(struct device *dev) return 0; if (host) { + /* FIXME: TODO move get_sync to proper dev_pm_ops function */ + pm_runtime_get_sync(host->dev); host->suspended = 1; if (host->pdata->suspend) { ret = host->pdata->suspend(&pdev->dev, @@ -2116,13 +2108,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 { @@ -2134,8 +2124,9 @@ static int omap_hsmmc_suspend(struct device *dev) dev_dbg(mmc_dev(host->mmc), "Unmask interrupt failed\n"); } - mmc_host_disable(host->mmc); } + /* FIXME: TODO move put_sync to proper dev_pm_ops function */ + pm_runtime_put_sync(host->dev); } return ret; @@ -2152,14 +2143,8 @@ 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; - } + /* FIXME: TODO move put_sync to proper dev_pm_ops function */ + pm_runtime_get_sync(host->dev); if (host->got_dbclk) clk_enable(host->dbclk); @@ -2179,14 +2164,14 @@ static int omap_hsmmc_resume(struct device *dev) ret = mmc_resume_host(host->mmc); if (ret == 0) host->suspended = 0; + + /* FIXME: TODO move put_sync to proper dev_pm_ops function */ + 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 @@ -2194,9 +2179,35 @@ 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), "host: 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> --- drivers/mmc/host/omap_hsmmc.c | 127 ++++++++++++++++++++++------------------- 1 files changed, 69 insertions(+), 58 deletions(-)