| Message ID | 20230817162159.242087-1-limings@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v9] mmc: sdhci-of-dwcmshc: Add runtime PM operations | expand |
On Thu, 17 Aug 2023 at 18:22, Liming Sun <limings@nvidia.com> wrote: > > This commit implements the runtime PM operations to disable eMMC > card clock when idle. > > Reviewed-by: David Thompson <davthompson@nvidia.com> > Signed-off-by: Liming Sun <limings@nvidia.com> > --- > v8->v9: > - Address Adrian's comment to do the pm_runtime_put() in > dwcmshc_resume() instead; Error path changes not included yet. > v7->v8: > - Address Ulf's comment (option-1); > - Updates for Adrian's comment to remove the force_suspend/resume > in dwcmshc_resume()/dwcmshc_suspend(); Add comments for > dwcmshc_resume()/dwcmshc_suspend(); > v6->v7: > - Address Ulf's comment; > v5->v6: > - Address Adrian's more comments and add coordination between > runtime PM and system PM; > v4->v5: > - Address Adrian's comment to move the pm_enable to the end to > avoid race; > v3->v4: > - Fix compiling reported by 'kernel test robot'; > v2->v3: > - Revise the commit message; > v1->v2: > Updates for comments from Ulf: > - Make the runtime PM logic generic for sdhci-of-dwcmshc; > v1: Initial version. > --- > drivers/mmc/host/sdhci-of-dwcmshc.c | 76 +++++++++++++++++++++++++++-- > 1 file changed, 72 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c > index e68cd87998c8..3b40f55ce2a4 100644 > --- a/drivers/mmc/host/sdhci-of-dwcmshc.c > +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c > @@ -15,6 +15,7 @@ > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_device.h> > +#include <linux/pm_runtime.h> > #include <linux/reset.h> > #include <linux/sizes.h> > > @@ -548,9 +549,13 @@ static int dwcmshc_probe(struct platform_device *pdev) > > host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY; > > + pm_runtime_get_noresume(dev); > + pm_runtime_set_active(dev); > + pm_runtime_enable(dev); > + > err = sdhci_setup_host(host); > if (err) > - goto err_clk; > + goto err_rpm; > > if (rk_priv) > dwcmshc_rk35xx_postinit(host, priv); > @@ -559,10 +564,15 @@ static int dwcmshc_probe(struct platform_device *pdev) > if (err) > goto err_setup_host; > > + pm_runtime_put(dev); > + > return 0; > > err_setup_host: > sdhci_cleanup_host(host); > +err_rpm: > + pm_runtime_disable(dev); > + pm_runtime_put_noidle(dev); > err_clk: > clk_disable_unprepare(pltfm_host->clk); > clk_disable_unprepare(priv->bus_clk); > @@ -602,9 +612,13 @@ static int dwcmshc_suspend(struct device *dev) > struct rk35xx_priv *rk_priv = priv->priv; > int ret; > > + pm_runtime_get_sync(dev); > + > ret = sdhci_suspend_host(host); > - if (ret) > + if (ret) { > + pm_runtime_put(dev); > return ret; > + } > > clk_disable_unprepare(pltfm_host->clk); > if (!IS_ERR(priv->bus_clk)) > @@ -642,11 +656,65 @@ static int dwcmshc_resume(struct device *dev) > return ret; > } > > - return sdhci_resume_host(host); > + ret = sdhci_resume_host(host); > + if (ret) > + return ret; > + > + pm_runtime_put(dev); To simplify the error path, I would suggest that you move the call to pm_runtime_put() to dwcmshc_suspend(). In fact what you need is just a call to pm_runtime_put_noidle(), somewhere after the call to pm_runtime_get_sync(). This is because runtime suspend is prevented by the PM core as it bumps the usage count with a pm_runtime_get_noresume() in the device_prepare() phase. > + > + return 0; > } > #endif [...] Kind regards Uffe
On 18/08/23 12:00, Ulf Hansson wrote: > On Thu, 17 Aug 2023 at 18:22, Liming Sun <limings@nvidia.com> wrote: >> >> This commit implements the runtime PM operations to disable eMMC >> card clock when idle. >> >> Reviewed-by: David Thompson <davthompson@nvidia.com> >> Signed-off-by: Liming Sun <limings@nvidia.com> >> --- >> v8->v9: >> - Address Adrian's comment to do the pm_runtime_put() in >> dwcmshc_resume() instead; Error path changes not included yet. >> v7->v8: >> - Address Ulf's comment (option-1); >> - Updates for Adrian's comment to remove the force_suspend/resume >> in dwcmshc_resume()/dwcmshc_suspend(); Add comments for >> dwcmshc_resume()/dwcmshc_suspend(); >> v6->v7: >> - Address Ulf's comment; >> v5->v6: >> - Address Adrian's more comments and add coordination between >> runtime PM and system PM; >> v4->v5: >> - Address Adrian's comment to move the pm_enable to the end to >> avoid race; >> v3->v4: >> - Fix compiling reported by 'kernel test robot'; >> v2->v3: >> - Revise the commit message; >> v1->v2: >> Updates for comments from Ulf: >> - Make the runtime PM logic generic for sdhci-of-dwcmshc; >> v1: Initial version. >> --- >> drivers/mmc/host/sdhci-of-dwcmshc.c | 76 +++++++++++++++++++++++++++-- >> 1 file changed, 72 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c >> index e68cd87998c8..3b40f55ce2a4 100644 >> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c >> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c >> @@ -15,6 +15,7 @@ >> #include <linux/module.h> >> #include <linux/of.h> >> #include <linux/of_device.h> >> +#include <linux/pm_runtime.h> >> #include <linux/reset.h> >> #include <linux/sizes.h> >> >> @@ -548,9 +549,13 @@ static int dwcmshc_probe(struct platform_device *pdev) >> >> host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY; >> >> + pm_runtime_get_noresume(dev); >> + pm_runtime_set_active(dev); >> + pm_runtime_enable(dev); >> + >> err = sdhci_setup_host(host); >> if (err) >> - goto err_clk; >> + goto err_rpm; >> >> if (rk_priv) >> dwcmshc_rk35xx_postinit(host, priv); >> @@ -559,10 +564,15 @@ static int dwcmshc_probe(struct platform_device *pdev) >> if (err) >> goto err_setup_host; >> >> + pm_runtime_put(dev); >> + >> return 0; >> >> err_setup_host: >> sdhci_cleanup_host(host); >> +err_rpm: >> + pm_runtime_disable(dev); >> + pm_runtime_put_noidle(dev); >> err_clk: >> clk_disable_unprepare(pltfm_host->clk); >> clk_disable_unprepare(priv->bus_clk); >> @@ -602,9 +612,13 @@ static int dwcmshc_suspend(struct device *dev) >> struct rk35xx_priv *rk_priv = priv->priv; >> int ret; >> >> + pm_runtime_get_sync(dev); >> + >> ret = sdhci_suspend_host(host); >> - if (ret) >> + if (ret) { >> + pm_runtime_put(dev); >> return ret; >> + } >> >> clk_disable_unprepare(pltfm_host->clk); >> if (!IS_ERR(priv->bus_clk)) >> @@ -642,11 +656,65 @@ static int dwcmshc_resume(struct device *dev) >> return ret; >> } >> >> - return sdhci_resume_host(host); >> + ret = sdhci_resume_host(host); >> + if (ret) >> + return ret; >> + >> + pm_runtime_put(dev); > > To simplify the error path, I would suggest that you move the call to > pm_runtime_put() to dwcmshc_suspend(). In fact what you need is just a > call to pm_runtime_put_noidle(), somewhere after the call to > pm_runtime_get_sync(). > > This is because runtime suspend is prevented by the PM core as it > bumps the usage count with a pm_runtime_get_noresume() in the > device_prepare() phase. I thought you didn't want to assume that, because in that case it can just be pm_runtime_resume() instead of pm_runtime_get_sync(), and then no 'put' is needed at all. > >> + >> + return 0; >> } >> #endif > > [...] > > Kind regards > Uffe
On Fri, 18 Aug 2023 at 11:36, Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 18/08/23 12:00, Ulf Hansson wrote: > > On Thu, 17 Aug 2023 at 18:22, Liming Sun <limings@nvidia.com> wrote: > >> > >> This commit implements the runtime PM operations to disable eMMC > >> card clock when idle. > >> > >> Reviewed-by: David Thompson <davthompson@nvidia.com> > >> Signed-off-by: Liming Sun <limings@nvidia.com> > >> --- > >> v8->v9: > >> - Address Adrian's comment to do the pm_runtime_put() in > >> dwcmshc_resume() instead; Error path changes not included yet. > >> v7->v8: > >> - Address Ulf's comment (option-1); > >> - Updates for Adrian's comment to remove the force_suspend/resume > >> in dwcmshc_resume()/dwcmshc_suspend(); Add comments for > >> dwcmshc_resume()/dwcmshc_suspend(); > >> v6->v7: > >> - Address Ulf's comment; > >> v5->v6: > >> - Address Adrian's more comments and add coordination between > >> runtime PM and system PM; > >> v4->v5: > >> - Address Adrian's comment to move the pm_enable to the end to > >> avoid race; > >> v3->v4: > >> - Fix compiling reported by 'kernel test robot'; > >> v2->v3: > >> - Revise the commit message; > >> v1->v2: > >> Updates for comments from Ulf: > >> - Make the runtime PM logic generic for sdhci-of-dwcmshc; > >> v1: Initial version. > >> --- > >> drivers/mmc/host/sdhci-of-dwcmshc.c | 76 +++++++++++++++++++++++++++-- > >> 1 file changed, 72 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c > >> index e68cd87998c8..3b40f55ce2a4 100644 > >> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c > >> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c > >> @@ -15,6 +15,7 @@ > >> #include <linux/module.h> > >> #include <linux/of.h> > >> #include <linux/of_device.h> > >> +#include <linux/pm_runtime.h> > >> #include <linux/reset.h> > >> #include <linux/sizes.h> > >> > >> @@ -548,9 +549,13 @@ static int dwcmshc_probe(struct platform_device *pdev) > >> > >> host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY; > >> > >> + pm_runtime_get_noresume(dev); > >> + pm_runtime_set_active(dev); > >> + pm_runtime_enable(dev); > >> + > >> err = sdhci_setup_host(host); > >> if (err) > >> - goto err_clk; > >> + goto err_rpm; > >> > >> if (rk_priv) > >> dwcmshc_rk35xx_postinit(host, priv); > >> @@ -559,10 +564,15 @@ static int dwcmshc_probe(struct platform_device *pdev) > >> if (err) > >> goto err_setup_host; > >> > >> + pm_runtime_put(dev); > >> + > >> return 0; > >> > >> err_setup_host: > >> sdhci_cleanup_host(host); > >> +err_rpm: > >> + pm_runtime_disable(dev); > >> + pm_runtime_put_noidle(dev); > >> err_clk: > >> clk_disable_unprepare(pltfm_host->clk); > >> clk_disable_unprepare(priv->bus_clk); > >> @@ -602,9 +612,13 @@ static int dwcmshc_suspend(struct device *dev) > >> struct rk35xx_priv *rk_priv = priv->priv; > >> int ret; > >> > >> + pm_runtime_get_sync(dev); > >> + > >> ret = sdhci_suspend_host(host); > >> - if (ret) > >> + if (ret) { > >> + pm_runtime_put(dev); > >> return ret; > >> + } > >> > >> clk_disable_unprepare(pltfm_host->clk); > >> if (!IS_ERR(priv->bus_clk)) > >> @@ -642,11 +656,65 @@ static int dwcmshc_resume(struct device *dev) > >> return ret; > >> } > >> > >> - return sdhci_resume_host(host); > >> + ret = sdhci_resume_host(host); > >> + if (ret) > >> + return ret; > >> + > >> + pm_runtime_put(dev); > > > > To simplify the error path, I would suggest that you move the call to > > pm_runtime_put() to dwcmshc_suspend(). In fact what you need is just a > > call to pm_runtime_put_noidle(), somewhere after the call to > > pm_runtime_get_sync(). > > > > This is because runtime suspend is prevented by the PM core as it > > bumps the usage count with a pm_runtime_get_noresume() in the > > device_prepare() phase. > > I thought you didn't want to assume that, because in that case > it can just be pm_runtime_resume() instead of pm_runtime_get_sync(), > and then no 'put' is needed at all. I don't really care, but just wanted to keep it as simple as possible. So yes, I am fine with a pm_runtime_resume() too. Maybe even simpler in this case, as we are not using pm_runtime_force_suspend|resume() anymore. Kind regards Uffe
> -----Original Message----- > From: Ulf Hansson <ulf.hansson@linaro.org> > Sent: Friday, August 18, 2023 6:19 AM > To: Adrian Hunter <adrian.hunter@intel.com> > Cc: Liming Sun <limings@nvidia.com>; David Thompson > <davthompson@nvidia.com>; Shawn Lin <shawn.lin@rock-chips.com>; linux- > mmc@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v9] mmc: sdhci-of-dwcmshc: Add runtime PM operations > > On Fri, 18 Aug 2023 at 11:36, Adrian Hunter <adrian.hunter@intel.com> > wrote: > > > > On 18/08/23 12:00, Ulf Hansson wrote: > > > On Thu, 17 Aug 2023 at 18:22, Liming Sun <limings@nvidia.com> wrote: > > >> > > >> This commit implements the runtime PM operations to disable eMMC > > >> card clock when idle. > > >> > > >> Reviewed-by: David Thompson <davthompson@nvidia.com> > > >> Signed-off-by: Liming Sun <limings@nvidia.com> > > >> --- > > >> v8->v9: > > >> - Address Adrian's comment to do the pm_runtime_put() in > > >> dwcmshc_resume() instead; Error path changes not included yet. > > >> v7->v8: > > >> - Address Ulf's comment (option-1); > > >> - Updates for Adrian's comment to remove the force_suspend/resume > > >> in dwcmshc_resume()/dwcmshc_suspend(); Add comments for > > >> dwcmshc_resume()/dwcmshc_suspend(); > > >> v6->v7: > > >> - Address Ulf's comment; > > >> v5->v6: > > >> - Address Adrian's more comments and add coordination between > > >> runtime PM and system PM; > > >> v4->v5: > > >> - Address Adrian's comment to move the pm_enable to the end to > > >> avoid race; > > >> v3->v4: > > >> - Fix compiling reported by 'kernel test robot'; > > >> v2->v3: > > >> - Revise the commit message; > > >> v1->v2: > > >> Updates for comments from Ulf: > > >> - Make the runtime PM logic generic for sdhci-of-dwcmshc; > > >> v1: Initial version. > > >> --- > > >> drivers/mmc/host/sdhci-of-dwcmshc.c | 76 > +++++++++++++++++++++++++++-- > > >> 1 file changed, 72 insertions(+), 4 deletions(-) > > >> > > >> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c > b/drivers/mmc/host/sdhci-of-dwcmshc.c > > >> index e68cd87998c8..3b40f55ce2a4 100644 > > >> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c > > >> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c > > >> @@ -15,6 +15,7 @@ > > >> #include <linux/module.h> > > >> #include <linux/of.h> > > >> #include <linux/of_device.h> > > >> +#include <linux/pm_runtime.h> > > >> #include <linux/reset.h> > > >> #include <linux/sizes.h> > > >> > > >> @@ -548,9 +549,13 @@ static int dwcmshc_probe(struct > platform_device *pdev) > > >> > > >> host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY; > > >> > > >> + pm_runtime_get_noresume(dev); > > >> + pm_runtime_set_active(dev); > > >> + pm_runtime_enable(dev); > > >> + > > >> err = sdhci_setup_host(host); > > >> if (err) > > >> - goto err_clk; > > >> + goto err_rpm; > > >> > > >> if (rk_priv) > > >> dwcmshc_rk35xx_postinit(host, priv); > > >> @@ -559,10 +564,15 @@ static int dwcmshc_probe(struct > platform_device *pdev) > > >> if (err) > > >> goto err_setup_host; > > >> > > >> + pm_runtime_put(dev); > > >> + > > >> return 0; > > >> > > >> err_setup_host: > > >> sdhci_cleanup_host(host); > > >> +err_rpm: > > >> + pm_runtime_disable(dev); > > >> + pm_runtime_put_noidle(dev); > > >> err_clk: > > >> clk_disable_unprepare(pltfm_host->clk); > > >> clk_disable_unprepare(priv->bus_clk); > > >> @@ -602,9 +612,13 @@ static int dwcmshc_suspend(struct device *dev) > > >> struct rk35xx_priv *rk_priv = priv->priv; > > >> int ret; > > >> > > >> + pm_runtime_get_sync(dev); > > >> + > > >> ret = sdhci_suspend_host(host); > > >> - if (ret) > > >> + if (ret) { > > >> + pm_runtime_put(dev); > > >> return ret; > > >> + } > > >> > > >> clk_disable_unprepare(pltfm_host->clk); > > >> if (!IS_ERR(priv->bus_clk)) > > >> @@ -642,11 +656,65 @@ static int dwcmshc_resume(struct device *dev) > > >> return ret; > > >> } > > >> > > >> - return sdhci_resume_host(host); > > >> + ret = sdhci_resume_host(host); > > >> + if (ret) > > >> + return ret; > > >> + > > >> + pm_runtime_put(dev); > > > > > > To simplify the error path, I would suggest that you move the call to > > > pm_runtime_put() to dwcmshc_suspend(). In fact what you need is just a > > > call to pm_runtime_put_noidle(), somewhere after the call to > > > pm_runtime_get_sync(). > > > > > > This is because runtime suspend is prevented by the PM core as it > > > bumps the usage count with a pm_runtime_get_noresume() in the > > > device_prepare() phase. > > > > I thought you didn't want to assume that, because in that case > > it can just be pm_runtime_resume() instead of pm_runtime_get_sync(), > > and then no 'put' is needed at all. > > I don't really care, but just wanted to keep it as simple as possible. > > So yes, I am fine with a pm_runtime_resume() too. Maybe even simpler > in this case, as we are not using pm_runtime_force_suspend|resume() > anymore. Thanks. Will post v10 with ' pm_runtime_resume(dev);' in dwcmshc_suspend() to keep it simple. > > Kind regards > Uffe
diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c index e68cd87998c8..3b40f55ce2a4 100644 --- a/drivers/mmc/host/sdhci-of-dwcmshc.c +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c @@ -15,6 +15,7 @@ #include <linux/module.h> #include <linux/of.h> #include <linux/of_device.h> +#include <linux/pm_runtime.h> #include <linux/reset.h> #include <linux/sizes.h> @@ -548,9 +549,13 @@ static int dwcmshc_probe(struct platform_device *pdev) host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY; + pm_runtime_get_noresume(dev); + pm_runtime_set_active(dev); + pm_runtime_enable(dev); + err = sdhci_setup_host(host); if (err) - goto err_clk; + goto err_rpm; if (rk_priv) dwcmshc_rk35xx_postinit(host, priv); @@ -559,10 +564,15 @@ static int dwcmshc_probe(struct platform_device *pdev) if (err) goto err_setup_host; + pm_runtime_put(dev); + return 0; err_setup_host: sdhci_cleanup_host(host); +err_rpm: + pm_runtime_disable(dev); + pm_runtime_put_noidle(dev); err_clk: clk_disable_unprepare(pltfm_host->clk); clk_disable_unprepare(priv->bus_clk); @@ -602,9 +612,13 @@ static int dwcmshc_suspend(struct device *dev) struct rk35xx_priv *rk_priv = priv->priv; int ret; + pm_runtime_get_sync(dev); + ret = sdhci_suspend_host(host); - if (ret) + if (ret) { + pm_runtime_put(dev); return ret; + } clk_disable_unprepare(pltfm_host->clk); if (!IS_ERR(priv->bus_clk)) @@ -642,11 +656,65 @@ static int dwcmshc_resume(struct device *dev) return ret; } - return sdhci_resume_host(host); + ret = sdhci_resume_host(host); + if (ret) + return ret; + + pm_runtime_put(dev); + + return 0; } #endif -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume); +#ifdef CONFIG_PM + +static void dwcmshc_enable_card_clk(struct sdhci_host *host) +{ + u16 ctrl; + + ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL); + if ((ctrl & SDHCI_CLOCK_INT_EN) && !(ctrl & SDHCI_CLOCK_CARD_EN)) { + ctrl |= SDHCI_CLOCK_CARD_EN; + sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL); + } +} + +static void dwcmshc_disable_card_clk(struct sdhci_host *host) +{ + u16 ctrl; + + ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL); + if (ctrl & SDHCI_CLOCK_CARD_EN) { + ctrl &= ~SDHCI_CLOCK_CARD_EN; + sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL); + } +} + +static int dwcmshc_runtime_suspend(struct device *dev) +{ + struct sdhci_host *host = dev_get_drvdata(dev); + + dwcmshc_disable_card_clk(host); + + return 0; +} + +static int dwcmshc_runtime_resume(struct device *dev) +{ + struct sdhci_host *host = dev_get_drvdata(dev); + + dwcmshc_enable_card_clk(host); + + return 0; +} + +#endif + +static const struct dev_pm_ops dwcmshc_pmops = { + SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume) + SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend, + dwcmshc_runtime_resume, NULL) +}; static struct platform_driver sdhci_dwcmshc_driver = { .driver = {