| Message ID | 1447151810-21864-1-git-send-email-ludovic.desroches@atmel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10 November 2015 at 11:36, Ludovic Desroches <ludovic.desroches@atmel.com> wrote: > Add runtime PM support and use runtime_force_suspend|resume() for system > PM. > > Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com> > --- > > Changes: > - from v1: take a runtime PM centric approach > > drivers/mmc/host/sdhci-of-at91.c | 66 +++++++++++++++++++++++++++++++++++++++- > 1 file changed, 65 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c > index 06d0b50..5dbe0ce 100644 > --- a/drivers/mmc/host/sdhci-of-at91.c > +++ b/drivers/mmc/host/sdhci-of-at91.c > @@ -21,6 +21,8 @@ > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_device.h> > +#include <linux/pm.h> > +#include <linux/pm_runtime.h> > > #include "sdhci-pltfm.h" > > @@ -51,6 +53,62 @@ static const struct of_device_id sdhci_at91_dt_match[] = { > {} > }; > > +#ifdef CONFIG_PM > +static int sdhci_at91_runtime_suspend(struct device *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct sdhci_host *host = platform_get_drvdata(pdev); These two lines can be replaced by: struct sdhci_host *host = dev_get_drvdata(dev); > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_at91_priv *priv = pltfm_host->priv; > + int ret; > + > + ret = sdhci_runtime_suspend_host(host); > + > + clk_disable_unprepare(priv->gck); > + clk_disable_unprepare(priv->hclock); > + clk_disable_unprepare(priv->mainck); > + > + return ret; > +} > + > +static int sdhci_at91_runtime_resume(struct device *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct sdhci_host *host = platform_get_drvdata(pdev); Same comment as above. > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_at91_priv *priv = pltfm_host->priv; > + int ret; > + > + ret = clk_prepare_enable(priv->mainck); > + if (ret) { > + dev_err(dev, "can't enable mainck\n"); > + return ret; > + } > + > + ret = clk_prepare_enable(priv->hclock); > + if (ret) { > + dev_err(dev, "can't enable hclock\n"); > + return ret; > + } > + > + ret = clk_prepare_enable(priv->gck); > + if (ret) { > + dev_err(dev, "can't enable gck\n"); > + return ret; > + } > + > + return sdhci_runtime_resume_host(host); > +} > +#endif /* CONFIG_PM_RUNTIME */ /s/CONFIG_PM_RUNTIME/CONFIG_PM > + > +static const struct dev_pm_ops sdhci_at91_dev_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > + pm_runtime_force_resume) > + SET_RUNTIME_PM_OPS(sdhci_at91_runtime_suspend, > + sdhci_at91_runtime_resume, > + NULL) > +}; > + > static int sdhci_at91_probe(struct platform_device *pdev) > { > const struct of_device_id *match; > @@ -148,6 +206,11 @@ static int sdhci_at91_probe(struct platform_device *pdev) > if (ret) > goto clocks_disable_unprepare; > > + pm_runtime_set_active(&pdev->dev); > + pm_runtime_enable(&pdev->dev); > + pm_runtime_set_autosuspend_delay(&pdev->dev, 50); > + pm_runtime_use_autosuspend(&pdev->dev); Move these four runtime PM calls above sdhci_add_host(), as after that point the host is used and thus runtime PM operations starts. > + > return 0; > > clocks_disable_unprepare: > @@ -165,6 +228,7 @@ static int sdhci_at91_remove(struct platform_device *pdev) > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > struct sdhci_at91_priv *priv = pltfm_host->priv; > Add: pm_runtime_get_sync(); > + pm_runtime_disable(&pdev->dev); Add: pm_runtime_put_noidle(); > sdhci_pltfm_unregister(pdev); > > clk_disable_unprepare(priv->gck); > @@ -178,7 +242,7 @@ static struct platform_driver sdhci_at91_driver = { > .driver = { > .name = "sdhci-at91", > .of_match_table = sdhci_at91_dt_match, > - .pm = SDHCI_PLTFM_PMOPS, > + .pm = &sdhci_at91_dev_pm_ops, > }, > .probe = sdhci_at91_probe, > .remove = sdhci_at91_remove, > -- > 2.5.0 > Otherwise, this looks good! Kind regards Uffe
On Tue, Nov 10, 2015 at 12:12:30PM +0100, Ulf Hansson wrote: > On 10 November 2015 at 11:36, Ludovic Desroches > <ludovic.desroches@atmel.com> wrote: > > Add runtime PM support and use runtime_force_suspend|resume() for system > > PM. > > > > Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com> > > --- > > > > Changes: > > - from v1: take a runtime PM centric approach > > > > drivers/mmc/host/sdhci-of-at91.c | 66 +++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 65 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c > > index 06d0b50..5dbe0ce 100644 > > --- a/drivers/mmc/host/sdhci-of-at91.c > > +++ b/drivers/mmc/host/sdhci-of-at91.c > > @@ -21,6 +21,8 @@ > > #include <linux/module.h> > > #include <linux/of.h> > > #include <linux/of_device.h> > > +#include <linux/pm.h> > > +#include <linux/pm_runtime.h> > > > > #include "sdhci-pltfm.h" > > > > @@ -51,6 +53,62 @@ static const struct of_device_id sdhci_at91_dt_match[] = { > > {} > > }; > > > > +#ifdef CONFIG_PM > > +static int sdhci_at91_runtime_suspend(struct device *dev) > > +{ > > + struct platform_device *pdev = to_platform_device(dev); > > + struct sdhci_host *host = platform_get_drvdata(pdev); > > These two lines can be replaced by: > struct sdhci_host *host = dev_get_drvdata(dev); > Oh yes, trying to complicate things! > > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > > + struct sdhci_at91_priv *priv = pltfm_host->priv; > > + int ret; > > + > > + ret = sdhci_runtime_suspend_host(host); > > + > > + clk_disable_unprepare(priv->gck); > > + clk_disable_unprepare(priv->hclock); > > + clk_disable_unprepare(priv->mainck); > > + > > + return ret; > > +} > > + > > +static int sdhci_at91_runtime_resume(struct device *dev) > > +{ > > + struct platform_device *pdev = to_platform_device(dev); > > + struct sdhci_host *host = platform_get_drvdata(pdev); > > Same comment as above. > > > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > > + struct sdhci_at91_priv *priv = pltfm_host->priv; > > + int ret; > > + > > + ret = clk_prepare_enable(priv->mainck); > > + if (ret) { > > + dev_err(dev, "can't enable mainck\n"); > > + return ret; > > + } > > + > > + ret = clk_prepare_enable(priv->hclock); > > + if (ret) { > > + dev_err(dev, "can't enable hclock\n"); > > + return ret; > > + } > > + > > + ret = clk_prepare_enable(priv->gck); > > + if (ret) { > > + dev_err(dev, "can't enable gck\n"); > > + return ret; > > + } > > + > > + return sdhci_runtime_resume_host(host); > > +} > > +#endif /* CONFIG_PM_RUNTIME */ > > /s/CONFIG_PM_RUNTIME/CONFIG_PM > > > + > > +static const struct dev_pm_ops sdhci_at91_dev_pm_ops = { > > + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > > + pm_runtime_force_resume) > > + SET_RUNTIME_PM_OPS(sdhci_at91_runtime_suspend, > > + sdhci_at91_runtime_resume, > > + NULL) > > +}; > > + > > static int sdhci_at91_probe(struct platform_device *pdev) > > { > > const struct of_device_id *match; > > @@ -148,6 +206,11 @@ static int sdhci_at91_probe(struct platform_device *pdev) > > if (ret) > > goto clocks_disable_unprepare; > > > > + pm_runtime_set_active(&pdev->dev); > > + pm_runtime_enable(&pdev->dev); > > + pm_runtime_set_autosuspend_delay(&pdev->dev, 50); > > + pm_runtime_use_autosuspend(&pdev->dev); > > Move these four runtime PM calls above sdhci_add_host(), as after that > point the host is used and thus runtime PM operations starts. > Ok. > > + > > return 0; > > > > clocks_disable_unprepare: > > @@ -165,6 +228,7 @@ static int sdhci_at91_remove(struct platform_device *pdev) > > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > > struct sdhci_at91_priv *priv = pltfm_host->priv; > > > > Add: > pm_runtime_get_sync(); > > > + pm_runtime_disable(&pdev->dev); > > Add: > pm_runtime_put_noidle(); > This time I should complicate things a bit more. > > sdhci_pltfm_unregister(pdev); > > > > clk_disable_unprepare(priv->gck); > > @@ -178,7 +242,7 @@ static struct platform_driver sdhci_at91_driver = { > > .driver = { > > .name = "sdhci-at91", > > .of_match_table = sdhci_at91_dt_match, > > - .pm = SDHCI_PLTFM_PMOPS, > > + .pm = &sdhci_at91_dev_pm_ops, > > }, > > .probe = sdhci_at91_probe, > > .remove = sdhci_at91_remove, > > -- > > 2.5.0 > > > > Otherwise, this looks good! Thanks for your feedback, v3 is coming. Regards Ludovic
On Tue, Nov 10, 2015 at 12:12:30PM +0100, Ulf Hansson wrote: > On 10 November 2015 at 11:36, Ludovic Desroches > <ludovic.desroches@atmel.com> wrote: > > Add runtime PM support and use runtime_force_suspend|resume() for system > > PM. > > [...] > > static int sdhci_at91_probe(struct platform_device *pdev) > > { > > const struct of_device_id *match; > > @@ -148,6 +206,11 @@ static int sdhci_at91_probe(struct platform_device *pdev) > > if (ret) > > goto clocks_disable_unprepare; > > > > + pm_runtime_set_active(&pdev->dev); > > + pm_runtime_enable(&pdev->dev); > > + pm_runtime_set_autosuspend_delay(&pdev->dev, 50); > > + pm_runtime_use_autosuspend(&pdev->dev); > > Move these four runtime PM calls above sdhci_add_host(), as after that > point the host is used and thus runtime PM operations starts. Sadly I have discovered a bit late it is not working when doing this because the controller has been suspended before doing the sdhci_add_host(). What is the right way to fix it? Calling pm_runtime_get_noresume() before sdhci_add_host() and calling pm_runtime_put_autosuspend() after? > > > + > > return 0; > > > > clocks_disable_unprepare: > > @@ -165,6 +228,7 @@ static int sdhci_at91_remove(struct platform_device *pdev) > > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > > struct sdhci_at91_priv *priv = pltfm_host->priv; > > Regards Ludovic
On 26 November 2015 at 17:07, Ludovic Desroches <ludovic.desroches@atmel.com> wrote: > On Tue, Nov 10, 2015 at 12:12:30PM +0100, Ulf Hansson wrote: >> On 10 November 2015 at 11:36, Ludovic Desroches >> <ludovic.desroches@atmel.com> wrote: >> > Add runtime PM support and use runtime_force_suspend|resume() for system >> > PM. >> > > > [...] > >> > static int sdhci_at91_probe(struct platform_device *pdev) >> > { >> > const struct of_device_id *match; >> > @@ -148,6 +206,11 @@ static int sdhci_at91_probe(struct platform_device *pdev) >> > if (ret) >> > goto clocks_disable_unprepare; >> > >> > + pm_runtime_set_active(&pdev->dev); >> > + pm_runtime_enable(&pdev->dev); >> > + pm_runtime_set_autosuspend_delay(&pdev->dev, 50); >> > + pm_runtime_use_autosuspend(&pdev->dev); >> >> Move these four runtime PM calls above sdhci_add_host(), as after that >> point the host is used and thus runtime PM operations starts. > > Sadly I have discovered a bit late it is not working when doing this > because the controller has been suspended before doing the > sdhci_add_host(). > > What is the right way to fix it? Calling pm_runtime_get_noresume() I would do that before pm_runtime_enable(). > before sdhci_add_host() and calling pm_runtime_put_autosuspend() after? Yes. Sorry for not spotting this before. [...] Kind regards Uffe
On Thu, Nov 26, 2015 at 05:24:40PM +0100, Ulf Hansson wrote: > On 26 November 2015 at 17:07, Ludovic Desroches > <ludovic.desroches@atmel.com> wrote: > > On Tue, Nov 10, 2015 at 12:12:30PM +0100, Ulf Hansson wrote: > >> On 10 November 2015 at 11:36, Ludovic Desroches > >> <ludovic.desroches@atmel.com> wrote: > >> > Add runtime PM support and use runtime_force_suspend|resume() for system > >> > PM. > >> > > > > > [...] > > > >> > static int sdhci_at91_probe(struct platform_device *pdev) > >> > { > >> > const struct of_device_id *match; > >> > @@ -148,6 +206,11 @@ static int sdhci_at91_probe(struct platform_device *pdev) > >> > if (ret) > >> > goto clocks_disable_unprepare; > >> > > >> > + pm_runtime_set_active(&pdev->dev); > >> > + pm_runtime_enable(&pdev->dev); > >> > + pm_runtime_set_autosuspend_delay(&pdev->dev, 50); > >> > + pm_runtime_use_autosuspend(&pdev->dev); > >> > >> Move these four runtime PM calls above sdhci_add_host(), as after that > >> point the host is used and thus runtime PM operations starts. > > > > Sadly I have discovered a bit late it is not working when doing this > > because the controller has been suspended before doing the > > sdhci_add_host(). > > > > What is the right way to fix it? Calling pm_runtime_get_noresume() > > I would do that before pm_runtime_enable(). > > > before sdhci_add_host() and calling pm_runtime_put_autosuspend() after? > > Yes. Sorry for not spotting this before. No problem, I am faulty too, it was not so trivial! I am sending the fix. Thanks Regards Ludovic
diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c index 06d0b50..5dbe0ce 100644 --- a/drivers/mmc/host/sdhci-of-at91.c +++ b/drivers/mmc/host/sdhci-of-at91.c @@ -21,6 +21,8 @@ #include <linux/module.h> #include <linux/of.h> #include <linux/of_device.h> +#include <linux/pm.h> +#include <linux/pm_runtime.h> #include "sdhci-pltfm.h" @@ -51,6 +53,62 @@ static const struct of_device_id sdhci_at91_dt_match[] = { {} }; +#ifdef CONFIG_PM +static int sdhci_at91_runtime_suspend(struct device *dev) +{ + struct platform_device *pdev = to_platform_device(dev); + struct sdhci_host *host = platform_get_drvdata(pdev); + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct sdhci_at91_priv *priv = pltfm_host->priv; + int ret; + + ret = sdhci_runtime_suspend_host(host); + + clk_disable_unprepare(priv->gck); + clk_disable_unprepare(priv->hclock); + clk_disable_unprepare(priv->mainck); + + return ret; +} + +static int sdhci_at91_runtime_resume(struct device *dev) +{ + struct platform_device *pdev = to_platform_device(dev); + struct sdhci_host *host = platform_get_drvdata(pdev); + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct sdhci_at91_priv *priv = pltfm_host->priv; + int ret; + + ret = clk_prepare_enable(priv->mainck); + if (ret) { + dev_err(dev, "can't enable mainck\n"); + return ret; + } + + ret = clk_prepare_enable(priv->hclock); + if (ret) { + dev_err(dev, "can't enable hclock\n"); + return ret; + } + + ret = clk_prepare_enable(priv->gck); + if (ret) { + dev_err(dev, "can't enable gck\n"); + return ret; + } + + return sdhci_runtime_resume_host(host); +} +#endif /* CONFIG_PM_RUNTIME */ + +static const struct dev_pm_ops sdhci_at91_dev_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, + pm_runtime_force_resume) + SET_RUNTIME_PM_OPS(sdhci_at91_runtime_suspend, + sdhci_at91_runtime_resume, + NULL) +}; + static int sdhci_at91_probe(struct platform_device *pdev) { const struct of_device_id *match; @@ -148,6 +206,11 @@ static int sdhci_at91_probe(struct platform_device *pdev) if (ret) goto clocks_disable_unprepare; + pm_runtime_set_active(&pdev->dev); + pm_runtime_enable(&pdev->dev); + pm_runtime_set_autosuspend_delay(&pdev->dev, 50); + pm_runtime_use_autosuspend(&pdev->dev); + return 0; clocks_disable_unprepare: @@ -165,6 +228,7 @@ static int sdhci_at91_remove(struct platform_device *pdev) struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); struct sdhci_at91_priv *priv = pltfm_host->priv; + pm_runtime_disable(&pdev->dev); sdhci_pltfm_unregister(pdev); clk_disable_unprepare(priv->gck); @@ -178,7 +242,7 @@ static struct platform_driver sdhci_at91_driver = { .driver = { .name = "sdhci-at91", .of_match_table = sdhci_at91_dt_match, - .pm = SDHCI_PLTFM_PMOPS, + .pm = &sdhci_at91_dev_pm_ops, }, .probe = sdhci_at91_probe, .remove = sdhci_at91_remove,
Add runtime PM support and use runtime_force_suspend|resume() for system PM. Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com> --- Changes: - from v1: take a runtime PM centric approach drivers/mmc/host/sdhci-of-at91.c | 66 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 65 insertions(+), 1 deletion(-)