| Message ID | 20230512181527.189345-1-limings@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] mmc: sdhci-of-dwcmshc: Add runtime PM operations | expand |
On 12/05/23 21:15, Liming Sun 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> > --- > 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 | 54 ++++++++++++++++++++++++++++- > 1 file changed, 53 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c > index e68cd87998c8..2d780a5bc8fb 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> > > @@ -546,6 +547,8 @@ static int dwcmshc_probe(struct platform_device *pdev) > sdhci_enable_v4_mode(host); > #endif > > + pm_runtime_enable(dev); Is there a reason to call it this early? That raises questions about runtime PM racing with the rest of the host initialization. Perhaps below instead (note also using devm): diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c index 2d780a5bc8fb..5cee42d72257 100644 --- a/drivers/mmc/host/sdhci-of-dwcmshc.c +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c @@ -547,8 +547,6 @@ static int dwcmshc_probe(struct platform_device *pdev) sdhci_enable_v4_mode(host); #endif - pm_runtime_enable(dev); - host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY; err = sdhci_setup_host(host); @@ -562,6 +560,8 @@ static int dwcmshc_probe(struct platform_device *pdev) if (err) goto err_setup_host; + devm_pm_runtime_enable(dev); + return 0; err_setup_host: > + > host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY; > > err = sdhci_setup_host(host); > @@ -646,7 +649,56 @@ static int dwcmshc_resume(struct device *dev) > } > #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); > + 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); > + 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); > + int ret = 0; > + > + ret = sdhci_runtime_suspend_host(host); > + if (!ret) > + dwcmshc_disable_card_clk(host); > + > + return ret; > +} > + > +static int dwcmshc_runtime_resume(struct device *dev) > +{ > + struct sdhci_host *host = dev_get_drvdata(dev); > + int ret = 0; > + > + dwcmshc_enable_card_clk(host); > + ret = sdhci_runtime_resume_host(host, 0); > + > + return ret; > +} > + > +#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 = {
Done, updated in v5. (sorry, not sure how I missed this comment earlier). > -----Original Message----- > From: Adrian Hunter <adrian.hunter@intel.com> > Sent: Friday, May 19, 2023 9:19 AM > To: Liming Sun <limings@nvidia.com>; Ulf Hansson <ulf.hansson@linaro.org>; > David Thompson <davthompson@nvidia.com>; Shawn Lin <shawn.lin@rock- > chips.com> > Cc: linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v4] mmc: sdhci-of-dwcmshc: Add runtime PM operations > > On 12/05/23 21:15, Liming Sun 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> > > --- > > 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 | 54 > ++++++++++++++++++++++++++++- > > 1 file changed, 53 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci- > of-dwcmshc.c > > index e68cd87998c8..2d780a5bc8fb 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> > > > > @@ -546,6 +547,8 @@ static int dwcmshc_probe(struct platform_device > *pdev) > > sdhci_enable_v4_mode(host); > > #endif > > > > + pm_runtime_enable(dev); > > Is there a reason to call it this early? That raises questions > about runtime PM racing with the rest of the host initialization. > Perhaps below instead (note also using devm): > > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci- > of-dwcmshc.c > index 2d780a5bc8fb..5cee42d72257 100644 > --- a/drivers/mmc/host/sdhci-of-dwcmshc.c > +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c > @@ -547,8 +547,6 @@ static int dwcmshc_probe(struct platform_device > *pdev) > sdhci_enable_v4_mode(host); > #endif > > - pm_runtime_enable(dev); > - > host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY; > > err = sdhci_setup_host(host); > @@ -562,6 +560,8 @@ static int dwcmshc_probe(struct platform_device > *pdev) > if (err) > goto err_setup_host; > > + devm_pm_runtime_enable(dev); > + > return 0; > > err_setup_host: > > > > + > > host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY; > > > > err = sdhci_setup_host(host); > > @@ -646,7 +649,56 @@ static int dwcmshc_resume(struct device *dev) > > } > > #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); > > + 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); > > + 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); > > + int ret = 0; > > + > > + ret = sdhci_runtime_suspend_host(host); > > + if (!ret) > > + dwcmshc_disable_card_clk(host); > > + > > + return ret; > > +} > > + > > +static int dwcmshc_runtime_resume(struct device *dev) > > +{ > > + struct sdhci_host *host = dev_get_drvdata(dev); > > + int ret = 0; > > + > > + dwcmshc_enable_card_clk(host); > > + ret = sdhci_runtime_resume_host(host, 0); > > + > > + return ret; > > +} > > + > > +#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 = {
diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c index e68cd87998c8..2d780a5bc8fb 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> @@ -546,6 +547,8 @@ static int dwcmshc_probe(struct platform_device *pdev) sdhci_enable_v4_mode(host); #endif + pm_runtime_enable(dev); + host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY; err = sdhci_setup_host(host); @@ -646,7 +649,56 @@ static int dwcmshc_resume(struct device *dev) } #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); + 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); + 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); + int ret = 0; + + ret = sdhci_runtime_suspend_host(host); + if (!ret) + dwcmshc_disable_card_clk(host); + + return ret; +} + +static int dwcmshc_runtime_resume(struct device *dev) +{ + struct sdhci_host *host = dev_get_drvdata(dev); + int ret = 0; + + dwcmshc_enable_card_clk(host); + ret = sdhci_runtime_resume_host(host, 0); + + return ret; +} + +#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 = {