| Message ID | 1675298118-64243-3-git-send-email-shawn.lin@rock-chips.com |
|---|---|
| State | New |
| Headers | show |
| Series | Some features and fix for sdhci-of-dwcmshc | expand |
On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote: > > This patch adds runtime PM support. > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > --- > > Changes in v2: None > > drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 50 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c > index 46b1ce7..fc917ed 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> > > @@ -551,6 +552,13 @@ static int dwcmshc_probe(struct platform_device *pdev) > if (err) > goto err_setup_host; > > + pm_runtime_get_noresume(&pdev->dev); > + 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); > + pm_runtime_put_autosuspend(&pdev->dev); > + > return 0; > > err_setup_host: > @@ -580,6 +588,11 @@ static int dwcmshc_remove(struct platform_device *pdev) > if (rk_priv) > clk_bulk_disable_unprepare(RK35xx_MAX_CLKS, > rk_priv->rockchip_clks); > + > + pm_runtime_get_sync(&pdev->dev); > + pm_runtime_disable(&pdev->dev); > + pm_runtime_put_noidle(&pdev->dev); > + > sdhci_pltfm_free(pdev); > > return 0; > @@ -638,7 +651,43 @@ static int dwcmshc_resume(struct device *dev) > } > #endif > > -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume); > +#ifdef CONFIG_PM > +static int dwcmshc_runtime_suspend(struct device *dev) > +{ > + struct sdhci_host *host = dev_get_drvdata(dev); > + u16 data; > + int ret; > + > + ret = sdhci_runtime_suspend_host(host); > + if (ret) > + return ret; > + > + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL); > + data &= ~SDHCI_CLOCK_CARD_EN; > + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL); > + > + return 0; > +} > + > +static int dwcmshc_runtime_resume(struct device *dev) > +{ > + struct sdhci_host *host = dev_get_drvdata(dev); > + u16 data; > + > + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL); > + data |= SDHCI_CLOCK_CARD_EN; > + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL); > + > + return sdhci_runtime_resume_host(host, 0); > +} > +#endif > + > +static const struct dev_pm_ops dwcmshc_pmops = { > + SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, > + dwcmshc_resume) I have looked at dwcmshc_suspend(), which calls sdhci_suspend_host(). As sdhci_suspend_host() will write to internal registers of the IP block, it's recommended to make sure the device's runtime resumed before doing that call. So that needs to be added along with $subject patch. There is also another option that may better for you, which is to use the pm_runtime_force_suspend|resume() helpers. There should be plenty of references to look at, but don't hesitate to ask around that, if you need some more help to get that working. > + SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend, > + dwcmshc_runtime_resume, NULL) > +}; > > static struct platform_driver sdhci_dwcmshc_driver = { > .driver = { Kind regards Uffe
Hi Ulf, On 2023/2/14 7:45, Ulf Hansson wrote: > On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> >> This patch adds runtime PM support. >> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> --- >> >> Changes in v2: None >> >> drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++- >> 1 file changed, 50 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c >> index 46b1ce7..fc917ed 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> >> >> @@ -551,6 +552,13 @@ static int dwcmshc_probe(struct platform_device *pdev) >> if (err) >> goto err_setup_host; >> >> + pm_runtime_get_noresume(&pdev->dev); >> + 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); >> + pm_runtime_put_autosuspend(&pdev->dev); >> + >> return 0; >> >> err_setup_host: >> @@ -580,6 +588,11 @@ static int dwcmshc_remove(struct platform_device *pdev) >> if (rk_priv) >> clk_bulk_disable_unprepare(RK35xx_MAX_CLKS, >> rk_priv->rockchip_clks); >> + >> + pm_runtime_get_sync(&pdev->dev); >> + pm_runtime_disable(&pdev->dev); >> + pm_runtime_put_noidle(&pdev->dev); >> + >> sdhci_pltfm_free(pdev); >> >> return 0; >> @@ -638,7 +651,43 @@ static int dwcmshc_resume(struct device *dev) >> } >> #endif >> >> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume); >> +#ifdef CONFIG_PM >> +static int dwcmshc_runtime_suspend(struct device *dev) >> +{ >> + struct sdhci_host *host = dev_get_drvdata(dev); >> + u16 data; >> + int ret; >> + >> + ret = sdhci_runtime_suspend_host(host); >> + if (ret) >> + return ret; >> + >> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL); >> + data &= ~SDHCI_CLOCK_CARD_EN; >> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL); >> + >> + return 0; >> +} >> + >> +static int dwcmshc_runtime_resume(struct device *dev) >> +{ >> + struct sdhci_host *host = dev_get_drvdata(dev); >> + u16 data; >> + >> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL); >> + data |= SDHCI_CLOCK_CARD_EN; >> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL); >> + >> + return sdhci_runtime_resume_host(host, 0); >> +} >> +#endif >> + >> +static const struct dev_pm_ops dwcmshc_pmops = { >> + SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, >> + dwcmshc_resume) > > I have looked at dwcmshc_suspend(), which calls sdhci_suspend_host(). > As sdhci_suspend_host() will write to internal registers of the IP > block, it's recommended to make sure the device's runtime resumed > before doing that call. So that needs to be added along with $subject > patch. > Yep, let me add a check here. > There is also another option that may better for you, which is to use > the pm_runtime_force_suspend|resume() helpers. There should be plenty > of references to look at, but don't hesitate to ask around that, if > you need some more help to get that working. If I understand correctly, pm_runtime_force_suspend|resume() helpers would use runtime PM ops for suspend/resume routine. In this case, the main issue is the handling of clock is quite different. In suspend we need to gate all clocks but in rpm case, it shouldn't. > >> + SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend, >> + dwcmshc_runtime_resume, NULL) >> +}; >> >> static struct platform_driver sdhci_dwcmshc_driver = { >> .driver = { > > Kind regards > Uffe
On Tue, 14 Feb 2023 at 04:36, Shawn Lin <shawn.lin@rock-chips.com> wrote: > > Hi Ulf, > > On 2023/2/14 7:45, Ulf Hansson wrote: > > On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote: > >> > >> This patch adds runtime PM support. > >> > >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > >> --- > >> > >> Changes in v2: None > >> > >> drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++- > >> 1 file changed, 50 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c > >> index 46b1ce7..fc917ed 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> > >> > >> @@ -551,6 +552,13 @@ static int dwcmshc_probe(struct platform_device *pdev) > >> if (err) > >> goto err_setup_host; > >> > >> + pm_runtime_get_noresume(&pdev->dev); > >> + 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); > >> + pm_runtime_put_autosuspend(&pdev->dev); > >> + > >> return 0; > >> > >> err_setup_host: > >> @@ -580,6 +588,11 @@ static int dwcmshc_remove(struct platform_device *pdev) > >> if (rk_priv) > >> clk_bulk_disable_unprepare(RK35xx_MAX_CLKS, > >> rk_priv->rockchip_clks); > >> + > >> + pm_runtime_get_sync(&pdev->dev); > >> + pm_runtime_disable(&pdev->dev); > >> + pm_runtime_put_noidle(&pdev->dev); > >> + > >> sdhci_pltfm_free(pdev); > >> > >> return 0; > >> @@ -638,7 +651,43 @@ static int dwcmshc_resume(struct device *dev) > >> } > >> #endif > >> > >> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume); > >> +#ifdef CONFIG_PM > >> +static int dwcmshc_runtime_suspend(struct device *dev) > >> +{ > >> + struct sdhci_host *host = dev_get_drvdata(dev); > >> + u16 data; > >> + int ret; > >> + > >> + ret = sdhci_runtime_suspend_host(host); > >> + if (ret) > >> + return ret; > >> + > >> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL); > >> + data &= ~SDHCI_CLOCK_CARD_EN; > >> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL); > >> + > >> + return 0; > >> +} > >> + > >> +static int dwcmshc_runtime_resume(struct device *dev) > >> +{ > >> + struct sdhci_host *host = dev_get_drvdata(dev); > >> + u16 data; > >> + > >> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL); > >> + data |= SDHCI_CLOCK_CARD_EN; > >> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL); > >> + > >> + return sdhci_runtime_resume_host(host, 0); > >> +} > >> +#endif > >> + > >> +static const struct dev_pm_ops dwcmshc_pmops = { > >> + SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, > >> + dwcmshc_resume) > > > > I have looked at dwcmshc_suspend(), which calls sdhci_suspend_host(). > > As sdhci_suspend_host() will write to internal registers of the IP > > block, it's recommended to make sure the device's runtime resumed > > before doing that call. So that needs to be added along with $subject > > patch. > > > > Yep, let me add a check here. > > > There is also another option that may better for you, which is to use > > the pm_runtime_force_suspend|resume() helpers. There should be plenty > > of references to look at, but don't hesitate to ask around that, if > > you need some more help to get that working. > > If I understand correctly, pm_runtime_force_suspend|resume() helpers > would use runtime PM ops for suspend/resume routine. In this case, the > main issue is the handling of clock is quite different. In suspend we > need to gate all clocks but in rpm case, it shouldn't. I see, but let me then ask, what's the point of keeping the clocks ungated at runtime suspend? That sounds like wasting energy to me - but maybe there are good reasons for it? [...] Kind regards Uffe
Hi Ulf On 2023/2/14 18:41, Ulf Hansson wrote: > On Tue, 14 Feb 2023 at 04:36, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> >> Hi Ulf, >> >> On 2023/2/14 7:45, Ulf Hansson wrote: >>> On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote: >>>> >>>> This patch adds runtime PM support. >>>> >>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >>>> --- >>>> >>>> Changes in v2: None >>>> >>>> drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++- >>>> 1 file changed, 50 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c >>>> index 46b1ce7..fc917ed 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> >>>> >>>> @@ -551,6 +552,13 @@ static int dwcmshc_probe(struct platform_device *pdev) >>>> if (err) >>>> goto err_setup_host; >>>> >>>> + pm_runtime_get_noresume(&pdev->dev); >>>> + 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); >>>> + pm_runtime_put_autosuspend(&pdev->dev); >>>> + >>>> return 0; >>>> >>>> err_setup_host: >>>> @@ -580,6 +588,11 @@ static int dwcmshc_remove(struct platform_device *pdev) >>>> if (rk_priv) >>>> clk_bulk_disable_unprepare(RK35xx_MAX_CLKS, >>>> rk_priv->rockchip_clks); >>>> + >>>> + pm_runtime_get_sync(&pdev->dev); >>>> + pm_runtime_disable(&pdev->dev); >>>> + pm_runtime_put_noidle(&pdev->dev); >>>> + >>>> sdhci_pltfm_free(pdev); >>>> >>>> return 0; >>>> @@ -638,7 +651,43 @@ static int dwcmshc_resume(struct device *dev) >>>> } >>>> #endif >>>> >>>> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume); >>>> +#ifdef CONFIG_PM >>>> +static int dwcmshc_runtime_suspend(struct device *dev) >>>> +{ >>>> + struct sdhci_host *host = dev_get_drvdata(dev); >>>> + u16 data; >>>> + int ret; >>>> + >>>> + ret = sdhci_runtime_suspend_host(host); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL); >>>> + data &= ~SDHCI_CLOCK_CARD_EN; >>>> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int dwcmshc_runtime_resume(struct device *dev) >>>> +{ >>>> + struct sdhci_host *host = dev_get_drvdata(dev); >>>> + u16 data; >>>> + >>>> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL); >>>> + data |= SDHCI_CLOCK_CARD_EN; >>>> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL); >>>> + >>>> + return sdhci_runtime_resume_host(host, 0); >>>> +} >>>> +#endif >>>> + >>>> +static const struct dev_pm_ops dwcmshc_pmops = { >>>> + SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, >>>> + dwcmshc_resume) >>> >>> I have looked at dwcmshc_suspend(), which calls sdhci_suspend_host(). >>> As sdhci_suspend_host() will write to internal registers of the IP >>> block, it's recommended to make sure the device's runtime resumed >>> before doing that call. So that needs to be added along with $subject >>> patch. >>> >> >> Yep, let me add a check here. >> >>> There is also another option that may better for you, which is to use >>> the pm_runtime_force_suspend|resume() helpers. There should be plenty >>> of references to look at, but don't hesitate to ask around that, if >>> you need some more help to get that working. >> >> If I understand correctly, pm_runtime_force_suspend|resume() helpers >> would use runtime PM ops for suspend/resume routine. In this case, the >> main issue is the handling of clock is quite different. In suspend we >> need to gate all clocks but in rpm case, it shouldn't. > > I see, but let me then ask, what's the point of keeping the clocks > ungated at runtime suspend? > > That sounds like wasting energy to me - but maybe there are good reasons for it? The point to keep the clocks ungated at runtime suspend is that if they are gated, the DLL would lost its locked state which causes retuning every time. It's quite painful for performance. However, if we just gate output clock to the device, the devices(basically I refer to eMMC) will get into power-save status by itself. That helps us too keep balance between power & performance during runtime. ：） > > [...] > > Kind regards > Uffe
+ Rafael On Wed, 15 Feb 2023 at 01:50, Shawn Lin <shawn.lin@rock-chips.com> wrote: > > Hi Ulf > > On 2023/2/14 18:41, Ulf Hansson wrote: > > On Tue, 14 Feb 2023 at 04:36, Shawn Lin <shawn.lin@rock-chips.com> wrote: > >> > >> Hi Ulf, > >> > >> On 2023/2/14 7:45, Ulf Hansson wrote: > >>> On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote: > >>>> > >>>> This patch adds runtime PM support. > >>>> > >>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > >>>> --- > >>>> > >>>> Changes in v2: None > >>>> > >>>> drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++- > >>>> 1 file changed, 50 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c > >>>> index 46b1ce7..fc917ed 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> > >>>> > >>>> @@ -551,6 +552,13 @@ static int dwcmshc_probe(struct platform_device *pdev) > >>>> if (err) > >>>> goto err_setup_host; > >>>> > >>>> + pm_runtime_get_noresume(&pdev->dev); > >>>> + 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); > >>>> + pm_runtime_put_autosuspend(&pdev->dev); > >>>> + > >>>> return 0; > >>>> > >>>> err_setup_host: > >>>> @@ -580,6 +588,11 @@ static int dwcmshc_remove(struct platform_device *pdev) > >>>> if (rk_priv) > >>>> clk_bulk_disable_unprepare(RK35xx_MAX_CLKS, > >>>> rk_priv->rockchip_clks); > >>>> + > >>>> + pm_runtime_get_sync(&pdev->dev); > >>>> + pm_runtime_disable(&pdev->dev); > >>>> + pm_runtime_put_noidle(&pdev->dev); > >>>> + > >>>> sdhci_pltfm_free(pdev); > >>>> > >>>> return 0; > >>>> @@ -638,7 +651,43 @@ static int dwcmshc_resume(struct device *dev) > >>>> } > >>>> #endif > >>>> > >>>> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume); > >>>> +#ifdef CONFIG_PM > >>>> +static int dwcmshc_runtime_suspend(struct device *dev) > >>>> +{ > >>>> + struct sdhci_host *host = dev_get_drvdata(dev); > >>>> + u16 data; > >>>> + int ret; > >>>> + > >>>> + ret = sdhci_runtime_suspend_host(host); > >>>> + if (ret) > >>>> + return ret; > >>>> + > >>>> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL); > >>>> + data &= ~SDHCI_CLOCK_CARD_EN; > >>>> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL); > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> +static int dwcmshc_runtime_resume(struct device *dev) > >>>> +{ > >>>> + struct sdhci_host *host = dev_get_drvdata(dev); > >>>> + u16 data; > >>>> + > >>>> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL); > >>>> + data |= SDHCI_CLOCK_CARD_EN; > >>>> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL); > >>>> + > >>>> + return sdhci_runtime_resume_host(host, 0); > >>>> +} > >>>> +#endif > >>>> + > >>>> +static const struct dev_pm_ops dwcmshc_pmops = { > >>>> + SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, > >>>> + dwcmshc_resume) > >>> > >>> I have looked at dwcmshc_suspend(), which calls sdhci_suspend_host(). > >>> As sdhci_suspend_host() will write to internal registers of the IP > >>> block, it's recommended to make sure the device's runtime resumed > >>> before doing that call. So that needs to be added along with $subject > >>> patch. > >>> > >> > >> Yep, let me add a check here. > >> > >>> There is also another option that may better for you, which is to use > >>> the pm_runtime_force_suspend|resume() helpers. There should be plenty > >>> of references to look at, but don't hesitate to ask around that, if > >>> you need some more help to get that working. > >> > >> If I understand correctly, pm_runtime_force_suspend|resume() helpers > >> would use runtime PM ops for suspend/resume routine. In this case, the > >> main issue is the handling of clock is quite different. In suspend we > >> need to gate all clocks but in rpm case, it shouldn't. > > > > I see, but let me then ask, what's the point of keeping the clocks > > ungated at runtime suspend? > > > > That sounds like wasting energy to me - but maybe there are good reasons for it? > > The point to keep the clocks ungated at runtime suspend is that if they > are gated, the DLL would lost its locked state which causes retuning > every time. It's quite painful for performance. However, if we just gate > output clock to the device, the devices(basically I refer to eMMC) will > get into power-save status by itself. That helps us too keep balance > between power & performance during runtime. ：） I see, thanks for clarifying! In principle your approach should work fine, but it depends a bit on the platform/soc too. I assume there could also be a PM domain attached to the mmc host device, right? If such a PM domain gets powered off, wouldn't you need to run a retuning sequence anyway? FYI, I have heard about similar problems for devices before - and it's been discussed too. In principle, it sounds to me that we have devices that would benefit from using *multiple* idle states, rather than just the one we have for runtime suspend and the one for system wide suspend. Kind regards Uffe
diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c index 46b1ce7..fc917ed 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> @@ -551,6 +552,13 @@ static int dwcmshc_probe(struct platform_device *pdev) if (err) goto err_setup_host; + pm_runtime_get_noresume(&pdev->dev); + 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); + pm_runtime_put_autosuspend(&pdev->dev); + return 0; err_setup_host: @@ -580,6 +588,11 @@ static int dwcmshc_remove(struct platform_device *pdev) if (rk_priv) clk_bulk_disable_unprepare(RK35xx_MAX_CLKS, rk_priv->rockchip_clks); + + pm_runtime_get_sync(&pdev->dev); + pm_runtime_disable(&pdev->dev); + pm_runtime_put_noidle(&pdev->dev); + sdhci_pltfm_free(pdev); return 0; @@ -638,7 +651,43 @@ static int dwcmshc_resume(struct device *dev) } #endif -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume); +#ifdef CONFIG_PM +static int dwcmshc_runtime_suspend(struct device *dev) +{ + struct sdhci_host *host = dev_get_drvdata(dev); + u16 data; + int ret; + + ret = sdhci_runtime_suspend_host(host); + if (ret) + return ret; + + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL); + data &= ~SDHCI_CLOCK_CARD_EN; + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL); + + return 0; +} + +static int dwcmshc_runtime_resume(struct device *dev) +{ + struct sdhci_host *host = dev_get_drvdata(dev); + u16 data; + + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL); + data |= SDHCI_CLOCK_CARD_EN; + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL); + + return sdhci_runtime_resume_host(host, 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 = {
This patch adds runtime PM support. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- Changes in v2: None drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-)