| Message ID | 1375825071-20922-3-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Doug, See my comment inline. On Tuesday 06 of August 2013 14:37:49 Doug Anderson wrote: > If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up > looping around forever. This has been seen to happen on exynos5420 > silicon despite the fact that we haven't enabled any wakeup events due > to a silicon errata. It is safe to do on all exynos variants. > > Signed-off-by: Doug Anderson <dianders@chromium.org> > --- > Changes in v4: > - Take Seungwon's suggestion and don't add any dw_mmc-pltfm code. > > Changes in v3: > - Add freeze/thaw and poweroff/restore noirq entries. > > Changes in v2: > - Use suspend_noirq as per James Hogan. > > drivers/mmc/host/dw_mmc-exynos.c | 51 > ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 49 > insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/host/dw_mmc-exynos.c > b/drivers/mmc/host/dw_mmc-exynos.c index 866edef..0c1f192 100644 > --- a/drivers/mmc/host/dw_mmc-exynos.c > +++ b/drivers/mmc/host/dw_mmc-exynos.c > @@ -30,6 +30,7 @@ > #define SDMMC_CLKSEL_TIMING(x, y, z) (SDMMC_CLKSEL_CCLK_SAMPLE(x) | \ > SDMMC_CLKSEL_CCLK_DRIVE(y) | \ > SDMMC_CLKSEL_CCLK_DIVIDER(z)) > +#define SDMMC_CLKSEL_WAKEUP_INT BIT(11) > > #define EXYNOS4210_FIXED_CIU_CLK_DIV 2 > #define EXYNOS4412_FIXED_CIU_CLK_DIV 4 > @@ -100,6 +101,30 @@ static int dw_mci_exynos_setup_clock(struct dw_mci > *host) return 0; > } > > +/** > + * dw_mci_exynos_resume_noirq - Exynos-specific resume code > + * > + * On exynos5420 there is a silicon errata that will sometimes leave > the + * WAKEUP_INT bit in the CLKSEL register asserted. This bit is 1 > to indicate + * that it fired and we can clear it by writing a 1 back. > Clear it to prevent + * interrupts from going off constantly. > + * > + * We run this code on all exynos variants because it doesn't hurt and > the bug + * may be more widespread than just exynos5420. > + */ > + > +static int dw_mci_exynos_resume_noirq(struct device *dev) > +{ > + struct dw_mci *host = dev_get_drvdata(dev); > + u32 clksel; > + > + clksel = mci_readl(host, CLKSEL); > + if (clksel & SDMMC_CLKSEL_WAKEUP_INT) > + mci_writel(host, CLKSEL, clksel); What about clock gating? Will the clock used for clocking this register be always enabled when this gets called? Best regards, Tomasz > + return 0; > +} > + > static void dw_mci_exynos_prepare_command(struct dw_mci *host, u32 > *cmdr) { > /* > @@ -187,17 +212,39 @@ static int dw_mci_exynos_probe(struct > platform_device *pdev) return dw_mci_pltfm_register(pdev, drv_data); > } > > +static struct dev_pm_ops dw_mci_exynos_pmops; > + > static struct platform_driver dw_mci_exynos_pltfm_driver = { > .probe = dw_mci_exynos_probe, > .remove = __exit_p(dw_mci_pltfm_remove), > .driver = { > .name = "dwmmc_exynos", > .of_match_table = dw_mci_exynos_match, > - .pm = &dw_mci_pltfm_pmops, > + .pm = &dw_mci_exynos_pmops, > }, > }; > > -module_platform_driver(dw_mci_exynos_pltfm_driver); > +static int __init dw_mci_exynos_init(void) > +{ > + /* Add a "noirq" resume to platform pmops */ > + memcpy(&dw_mci_exynos_pmops, &dw_mci_pltfm_pmops, > + sizeof(dw_mci_exynos_pmops)); > + WARN_ON(dw_mci_exynos_pmops.resume_noirq || > + dw_mci_exynos_pmops.thaw_noirq || > + dw_mci_exynos_pmops.restore_noirq); > + dw_mci_exynos_pmops.resume_noirq = dw_mci_exynos_resume_noirq; > + dw_mci_exynos_pmops.thaw_noirq = dw_mci_exynos_resume_noirq; > + dw_mci_exynos_pmops.restore_noirq = dw_mci_exynos_resume_noirq; > + > + return platform_driver_register(&dw_mci_exynos_pltfm_driver); > +} > +module_init(dw_mci_exynos_init); > + > +static void __exit dw_mci_exynos_exit(void) > +{ > + platform_driver_unregister(&dw_mci_exynos_pltfm_driver); > +} > +module_exit(dw_mci_exynos_exit); > > MODULE_DESCRIPTION("Samsung Specific DW-MSHC Driver Extension"); > MODULE_AUTHOR("Thomas Abraham <thomas.ab@samsung.com");
Tomasz, On Tue, Aug 6, 2013 at 2:58 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: >> +static int dw_mci_exynos_resume_noirq(struct device *dev) >> +{ >> + struct dw_mci *host = dev_get_drvdata(dev); >> + u32 clksel; >> + >> + clksel = mci_readl(host, CLKSEL); >> + if (clksel & SDMMC_CLKSEL_WAKEUP_INT) >> + mci_writel(host, CLKSEL, clksel); > > What about clock gating? Will the clock used for clocking this register be > always enabled when this gets called? Since this is just accessing and writing a register in the "Mobile Storage Host" block, I'd imagine that this should be the "biu" (bus interface unit) clock, right? The dw_mmc code grabs the biu clock at probe time and never lets it go. That means that we're OK as long as common clock framework has already restored clocks to normal operation by this time. Do you think that common clock framework might not have put the clocks back into order by the time "noirq" callbacks are executed? -Doug
On Tuesday 06 of August 2013 15:09:46 Doug Anderson wrote: > Tomasz, > > On Tue, Aug 6, 2013 at 2:58 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > >> +static int dw_mci_exynos_resume_noirq(struct device *dev) > >> +{ > >> + struct dw_mci *host = dev_get_drvdata(dev); > >> + u32 clksel; > >> + > >> + clksel = mci_readl(host, CLKSEL); > >> + if (clksel & SDMMC_CLKSEL_WAKEUP_INT) > >> + mci_writel(host, CLKSEL, clksel); > > > > What about clock gating? Will the clock used for clocking this > > register be always enabled when this gets called? > > Since this is just accessing and writing a register in the "Mobile > Storage Host" block, I'd imagine that this should be the "biu" (bus > interface unit) clock, right? The dw_mmc code grabs the biu clock at > probe time and never lets it go. That means that we're OK as long as > common clock framework has already restored clocks to normal operation > by this time. > > Do you think that common clock framework might not have put the clocks > back into order by the time "noirq" callbacks are executed? Ahh, so the dw_mmc driver doesn't do any clock gating? This is not very nice of it. Well, in this case your patch is OK, but possibly some clock gating will have to be added to this driver at some point of time. Anyway: Reviewed-by: Tomasz Figa <t.figa@samsung.com> Best regards, Tomasz
On Wed, August 07, 2013, Doug Anderson wrote: > If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up > looping around forever. This has been seen to happen on exynos5420 > silicon despite the fact that we haven't enabled any wakeup events due > to a silicon errata. It is safe to do on all exynos variants. > > Signed-off-by: Doug Anderson <dianders@chromium.org> > --- > Changes in v4: > - Take Seungwon's suggestion and don't add any dw_mmc-pltfm code. > > Changes in v3: > - Add freeze/thaw and poweroff/restore noirq entries. > > Changes in v2: > - Use suspend_noirq as per James Hogan. > > drivers/mmc/host/dw_mmc-exynos.c | 51 ++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 49 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c > index 866edef..0c1f192 100644 > --- a/drivers/mmc/host/dw_mmc-exynos.c > +++ b/drivers/mmc/host/dw_mmc-exynos.c > @@ -30,6 +30,7 @@ > #define SDMMC_CLKSEL_TIMING(x, y, z) (SDMMC_CLKSEL_CCLK_SAMPLE(x) | \ > SDMMC_CLKSEL_CCLK_DRIVE(y) | \ > SDMMC_CLKSEL_CCLK_DIVIDER(z)) > +#define SDMMC_CLKSEL_WAKEUP_INT BIT(11) > > #define EXYNOS4210_FIXED_CIU_CLK_DIV 2 > #define EXYNOS4412_FIXED_CIU_CLK_DIV 4 > @@ -100,6 +101,30 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host) > return 0; > } > > +/** > + * dw_mci_exynos_resume_noirq - Exynos-specific resume code > + * > + * On exynos5420 there is a silicon errata that will sometimes leave the > + * WAKEUP_INT bit in the CLKSEL register asserted. This bit is 1 to indicate > + * that it fired and we can clear it by writing a 1 back. Clear it to prevent > + * interrupts from going off constantly. > + * > + * We run this code on all exynos variants because it doesn't hurt and the bug > + * may be more widespread than just exynos5420. I guess just above comment can be removed. (Not be widespread) Updating the origin value of CLKSEL looks like no harm while SDMMC_CLKSEL_WAKEUP_INT is cleared. > + */ > + > +static int dw_mci_exynos_resume_noirq(struct device *dev) > +{ > + struct dw_mci *host = dev_get_drvdata(dev); > + u32 clksel; > + > + clksel = mci_readl(host, CLKSEL); > + if (clksel & SDMMC_CLKSEL_WAKEUP_INT) > + mci_writel(host, CLKSEL, clksel); > + > + return 0; > +} > + > static void dw_mci_exynos_prepare_command(struct dw_mci *host, u32 *cmdr) > { > /* > @@ -187,17 +212,39 @@ static int dw_mci_exynos_probe(struct platform_device *pdev) > return dw_mci_pltfm_register(pdev, drv_data); > } > > +static struct dev_pm_ops dw_mci_exynos_pmops; > + > static struct platform_driver dw_mci_exynos_pltfm_driver = { > .probe = dw_mci_exynos_probe, > .remove = __exit_p(dw_mci_pltfm_remove), > .driver = { > .name = "dwmmc_exynos", > .of_match_table = dw_mci_exynos_match, > - .pm = &dw_mci_pltfm_pmops, > + .pm = &dw_mci_exynos_pmops, > }, > }; > > -module_platform_driver(dw_mci_exynos_pltfm_driver); > +static int __init dw_mci_exynos_init(void) > +{ > + /* Add a "noirq" resume to platform pmops */ > + memcpy(&dw_mci_exynos_pmops, &dw_mci_pltfm_pmops, > + sizeof(dw_mci_exynos_pmops)); > + WARN_ON(dw_mci_exynos_pmops.resume_noirq || > + dw_mci_exynos_pmops.thaw_noirq || > + dw_mci_exynos_pmops.restore_noirq); > + dw_mci_exynos_pmops.resume_noirq = dw_mci_exynos_resume_noirq; > + dw_mci_exynos_pmops.thaw_noirq = dw_mci_exynos_resume_noirq; > + dw_mci_exynos_pmops.restore_noirq = dw_mci_exynos_resume_noirq; If CONFIG_PM_SLEEP is not defined, we don't need to add it. And also, instead of reusing dw_mci_pltfm_pmops, how about defining dw_mci_exynos_pmops's own? Of course, suspend/resume will not different with dw_mci_pltfm* just now. But specific code for exynos would be added soon. Thanks, Seungwon Jeon
Seungwon, On Fri, Aug 9, 2013 at 6:33 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote: > On Wed, August 07, 2013, Doug Anderson wrote: >> If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up >> looping around forever. This has been seen to happen on exynos5420 >> silicon despite the fact that we haven't enabled any wakeup events due >> to a silicon errata. It is safe to do on all exynos variants. >> >> Signed-off-by: Doug Anderson <dianders@chromium.org> >> --- >> Changes in v4: >> - Take Seungwon's suggestion and don't add any dw_mmc-pltfm code. >> >> Changes in v3: >> - Add freeze/thaw and poweroff/restore noirq entries. >> >> Changes in v2: >> - Use suspend_noirq as per James Hogan. >> >> drivers/mmc/host/dw_mmc-exynos.c | 51 ++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 49 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c >> index 866edef..0c1f192 100644 >> --- a/drivers/mmc/host/dw_mmc-exynos.c >> +++ b/drivers/mmc/host/dw_mmc-exynos.c >> @@ -30,6 +30,7 @@ >> #define SDMMC_CLKSEL_TIMING(x, y, z) (SDMMC_CLKSEL_CCLK_SAMPLE(x) | \ >> SDMMC_CLKSEL_CCLK_DRIVE(y) | \ >> SDMMC_CLKSEL_CCLK_DIVIDER(z)) >> +#define SDMMC_CLKSEL_WAKEUP_INT BIT(11) >> >> #define EXYNOS4210_FIXED_CIU_CLK_DIV 2 >> #define EXYNOS4412_FIXED_CIU_CLK_DIV 4 >> @@ -100,6 +101,30 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host) >> return 0; >> } >> >> +/** >> + * dw_mci_exynos_resume_noirq - Exynos-specific resume code >> + * >> + * On exynos5420 there is a silicon errata that will sometimes leave the >> + * WAKEUP_INT bit in the CLKSEL register asserted. This bit is 1 to indicate >> + * that it fired and we can clear it by writing a 1 back. Clear it to prevent >> + * interrupts from going off constantly. >> + * >> + * We run this code on all exynos variants because it doesn't hurt and the bug >> + * may be more widespread than just exynos5420. > I guess just above comment can be removed. (Not be widespread) > Updating the origin value of CLKSEL looks like no harm while SDMMC_CLKSEL_WAKEUP_INT is cleared. OK, no problem. I'll clean up the comment next time revision. >> -module_platform_driver(dw_mci_exynos_pltfm_driver); >> +static int __init dw_mci_exynos_init(void) >> +{ >> + /* Add a "noirq" resume to platform pmops */ >> + memcpy(&dw_mci_exynos_pmops, &dw_mci_pltfm_pmops, >> + sizeof(dw_mci_exynos_pmops)); >> + WARN_ON(dw_mci_exynos_pmops.resume_noirq || >> + dw_mci_exynos_pmops.thaw_noirq || >> + dw_mci_exynos_pmops.restore_noirq); >> + dw_mci_exynos_pmops.resume_noirq = dw_mci_exynos_resume_noirq; >> + dw_mci_exynos_pmops.thaw_noirq = dw_mci_exynos_resume_noirq; >> + dw_mci_exynos_pmops.restore_noirq = dw_mci_exynos_resume_noirq; > > If CONFIG_PM_SLEEP is not defined, we don't need to add it. > And also, instead of reusing dw_mci_pltfm_pmops, how about defining dw_mci_exynos_pmops's own? > Of course, suspend/resume will not different with dw_mci_pltfm* just now. > But specific code for exynos would be added soon. Whoops! ...of course this should be conditional on CONFIG_PM_SLEEP. Thank you for catching. I spent a bit of time debating whether I should make my own structure or do a copy like this. It felt like a bit of a toss up to me, but I'm happy to do it the other way. I will call dw_mci_suspend(host) directly and assume hope that nobody adds any important code to dw_mci_pltfm_suspend(). The other alternative would be make dw_mci_pltfm_suspend() exported or call it indirectly through dw_mci_pltfm_pmops, both of which seem slightly worse. -Doug
diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c index 866edef..0c1f192 100644 --- a/drivers/mmc/host/dw_mmc-exynos.c +++ b/drivers/mmc/host/dw_mmc-exynos.c @@ -30,6 +30,7 @@ #define SDMMC_CLKSEL_TIMING(x, y, z) (SDMMC_CLKSEL_CCLK_SAMPLE(x) | \ SDMMC_CLKSEL_CCLK_DRIVE(y) | \ SDMMC_CLKSEL_CCLK_DIVIDER(z)) +#define SDMMC_CLKSEL_WAKEUP_INT BIT(11) #define EXYNOS4210_FIXED_CIU_CLK_DIV 2 #define EXYNOS4412_FIXED_CIU_CLK_DIV 4 @@ -100,6 +101,30 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host) return 0; } +/** + * dw_mci_exynos_resume_noirq - Exynos-specific resume code + * + * On exynos5420 there is a silicon errata that will sometimes leave the + * WAKEUP_INT bit in the CLKSEL register asserted. This bit is 1 to indicate + * that it fired and we can clear it by writing a 1 back. Clear it to prevent + * interrupts from going off constantly. + * + * We run this code on all exynos variants because it doesn't hurt and the bug + * may be more widespread than just exynos5420. + */ + +static int dw_mci_exynos_resume_noirq(struct device *dev) +{ + struct dw_mci *host = dev_get_drvdata(dev); + u32 clksel; + + clksel = mci_readl(host, CLKSEL); + if (clksel & SDMMC_CLKSEL_WAKEUP_INT) + mci_writel(host, CLKSEL, clksel); + + return 0; +} + static void dw_mci_exynos_prepare_command(struct dw_mci *host, u32 *cmdr) { /* @@ -187,17 +212,39 @@ static int dw_mci_exynos_probe(struct platform_device *pdev) return dw_mci_pltfm_register(pdev, drv_data); } +static struct dev_pm_ops dw_mci_exynos_pmops; + static struct platform_driver dw_mci_exynos_pltfm_driver = { .probe = dw_mci_exynos_probe, .remove = __exit_p(dw_mci_pltfm_remove), .driver = { .name = "dwmmc_exynos", .of_match_table = dw_mci_exynos_match, - .pm = &dw_mci_pltfm_pmops, + .pm = &dw_mci_exynos_pmops, }, }; -module_platform_driver(dw_mci_exynos_pltfm_driver); +static int __init dw_mci_exynos_init(void) +{ + /* Add a "noirq" resume to platform pmops */ + memcpy(&dw_mci_exynos_pmops, &dw_mci_pltfm_pmops, + sizeof(dw_mci_exynos_pmops)); + WARN_ON(dw_mci_exynos_pmops.resume_noirq || + dw_mci_exynos_pmops.thaw_noirq || + dw_mci_exynos_pmops.restore_noirq); + dw_mci_exynos_pmops.resume_noirq = dw_mci_exynos_resume_noirq; + dw_mci_exynos_pmops.thaw_noirq = dw_mci_exynos_resume_noirq; + dw_mci_exynos_pmops.restore_noirq = dw_mci_exynos_resume_noirq; + + return platform_driver_register(&dw_mci_exynos_pltfm_driver); +} +module_init(dw_mci_exynos_init); + +static void __exit dw_mci_exynos_exit(void) +{ + platform_driver_unregister(&dw_mci_exynos_pltfm_driver); +} +module_exit(dw_mci_exynos_exit); MODULE_DESCRIPTION("Samsung Specific DW-MSHC Driver Extension"); MODULE_AUTHOR("Thomas Abraham <thomas.ab@samsung.com");
If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up looping around forever. This has been seen to happen on exynos5420 silicon despite the fact that we haven't enabled any wakeup events due to a silicon errata. It is safe to do on all exynos variants. Signed-off-by: Doug Anderson <dianders@chromium.org> --- Changes in v4: - Take Seungwon's suggestion and don't add any dw_mmc-pltfm code. Changes in v3: - Add freeze/thaw and poweroff/restore noirq entries. Changes in v2: - Use suspend_noirq as per James Hogan. drivers/mmc/host/dw_mmc-exynos.c | 51 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 49 insertions(+), 2 deletions(-)