| Message ID | 20230808202319.191434-1-limings@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v7] mmc: sdhci-of-dwcmshc: Add runtime PM operations | expand |
On 8/08/23 23:23, 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> > --- > 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 | 72 ++++++++++++++++++++++++++++- > 1 file changed, 70 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c > index e68cd87998c8..c8e145031429 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); > @@ -606,6 +616,12 @@ static int dwcmshc_suspend(struct device *dev) > if (ret) > return ret; > > + ret = pm_runtime_force_suspend(dev); > + if (ret) { > + sdhci_resume_host(host); > + return ret; > + } Since you are only using the runtime PM callbacks to turn off the card clock via SDHCI_CLOCK_CONTROL, pm_runtime_force_suspend() and pm_runtime_force_resume() are not needed at all. sdhci_suspend_host() does not care if SDHCI_CLOCK_CARD_EN is on or off. (And you are disabling pltfm_host->clk and priv->bus_clk, so presumably the result is no clock either way) sdhci_resume_host() does not restore state unless SDHCI_QUIRK2_HOST_OFF_CARD_ON is used, it just resets, so the internal clock SDHCI_CLOCK_INT_EN is off which is consistent with either runtime suspended or runtime resumed. So it just needs some comments, for example: +/* + * Note, runtime suspend changes only SDHCI_CLOCK_CARD_EN which has no effect on + * system suspend. + */ static int dwcmshc_suspend(struct device *dev) +/* + * Note, system resume leaves SDHCI_CLOCK_INT_EN off which is consistent with + * either runtime suspended or runtime resumed. + */ static int dwcmshc_resume(struct device *dev) > + > clk_disable_unprepare(pltfm_host->clk); > if (!IS_ERR(priv->bus_clk)) > clk_disable_unprepare(priv->bus_clk); > @@ -625,6 +641,10 @@ static int dwcmshc_resume(struct device *dev) > struct rk35xx_priv *rk_priv = priv->priv; > int ret; > > + ret = pm_runtime_force_resume(dev); > + if (ret) > + return ret; > + > ret = clk_prepare_enable(pltfm_host->clk); > if (ret) > return ret; > @@ -646,7 +666,55 @@ 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); > + 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 = {
On Thu, 10 Aug 2023 at 10:13, Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 8/08/23 23:23, 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> > > --- > > 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 | 72 ++++++++++++++++++++++++++++- > > 1 file changed, 70 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c > > index e68cd87998c8..c8e145031429 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); > > @@ -606,6 +616,12 @@ static int dwcmshc_suspend(struct device *dev) > > if (ret) > > return ret; > > > > + ret = pm_runtime_force_suspend(dev); > > + if (ret) { > > + sdhci_resume_host(host); > > + return ret; > > + } > > Since you are only using the runtime PM callbacks to turn off the card > clock via SDHCI_CLOCK_CONTROL, pm_runtime_force_suspend() and > pm_runtime_force_resume() are not needed at all. Right, it can be done without these too. > > sdhci_suspend_host() does not care if SDHCI_CLOCK_CARD_EN is on or off. > (And you are disabling pltfm_host->clk and priv->bus_clk, so presumably > the result is no clock either way) > > sdhci_resume_host() does not restore state unless > SDHCI_QUIRK2_HOST_OFF_CARD_ON is used, it just resets, so the internal clock > SDHCI_CLOCK_INT_EN is off which is consistent with either runtime suspended > or runtime resumed. Even if this may work, to me, it doesn't look like good practice for how to use runtime PM in combination with system wide suspend/resume. The point is, sdhci_suspend|resume_host() may end up reading/writing to sdhci registers - and we should *not* allow that (because it may not always work), unless the sdhci controller has been runtime resumed first, right? [...] Kind regards Uffe
On 10/08/23 13:21, Ulf Hansson wrote: > On Thu, 10 Aug 2023 at 10:13, Adrian Hunter <adrian.hunter@intel.com> wrote: >> >> On 8/08/23 23:23, 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> >>> --- >>> 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 | 72 ++++++++++++++++++++++++++++- >>> 1 file changed, 70 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c >>> index e68cd87998c8..c8e145031429 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); >>> @@ -606,6 +616,12 @@ static int dwcmshc_suspend(struct device *dev) >>> if (ret) >>> return ret; >>> >>> + ret = pm_runtime_force_suspend(dev); >>> + if (ret) { >>> + sdhci_resume_host(host); >>> + return ret; >>> + } >> >> Since you are only using the runtime PM callbacks to turn off the card >> clock via SDHCI_CLOCK_CONTROL, pm_runtime_force_suspend() and >> pm_runtime_force_resume() are not needed at all. > > Right, it can be done without these too. > >> >> sdhci_suspend_host() does not care if SDHCI_CLOCK_CARD_EN is on or off. >> (And you are disabling pltfm_host->clk and priv->bus_clk, so presumably >> the result is no clock either way) >> >> sdhci_resume_host() does not restore state unless >> SDHCI_QUIRK2_HOST_OFF_CARD_ON is used, it just resets, so the internal clock >> SDHCI_CLOCK_INT_EN is off which is consistent with either runtime suspended >> or runtime resumed. > > Even if this may work, to me, it doesn't look like good practice for > how to use runtime PM in combination with system wide suspend/resume. > > The point is, sdhci_suspend|resume_host() may end up reading/writing > to sdhci registers - and we should *not* allow that (because it may > not always work), unless the sdhci controller has been runtime resumed > first, right? I am OK with drivers that just want to use runtime PM to turn off a functional clock. sdhci-tegra.c is also doing that although using the clock framework. Certainly that approach assumes that the host controller's power state is not changed due to runtime PM. To ensure that the host controller is runtime resumed before calling sdhci_suspend_host(), we can just call pm_runtime_resume() I think.
On Thu, 10 Aug 2023 at 14:44, Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 10/08/23 13:21, Ulf Hansson wrote: > > On Thu, 10 Aug 2023 at 10:13, Adrian Hunter <adrian.hunter@intel.com> wrote: > >> > >> On 8/08/23 23:23, 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> > >>> --- > >>> 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 | 72 ++++++++++++++++++++++++++++- > >>> 1 file changed, 70 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c > >>> index e68cd87998c8..c8e145031429 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); > >>> @@ -606,6 +616,12 @@ static int dwcmshc_suspend(struct device *dev) > >>> if (ret) > >>> return ret; > >>> > >>> + ret = pm_runtime_force_suspend(dev); > >>> + if (ret) { > >>> + sdhci_resume_host(host); > >>> + return ret; > >>> + } > >> > >> Since you are only using the runtime PM callbacks to turn off the card > >> clock via SDHCI_CLOCK_CONTROL, pm_runtime_force_suspend() and > >> pm_runtime_force_resume() are not needed at all. > > > > Right, it can be done without these too. > > > >> > >> sdhci_suspend_host() does not care if SDHCI_CLOCK_CARD_EN is on or off. > >> (And you are disabling pltfm_host->clk and priv->bus_clk, so presumably > >> the result is no clock either way) > >> > >> sdhci_resume_host() does not restore state unless > >> SDHCI_QUIRK2_HOST_OFF_CARD_ON is used, it just resets, so the internal clock > >> SDHCI_CLOCK_INT_EN is off which is consistent with either runtime suspended > >> or runtime resumed. > > > > Even if this may work, to me, it doesn't look like good practice for > > how to use runtime PM in combination with system wide suspend/resume. > > > > The point is, sdhci_suspend|resume_host() may end up reading/writing > > to sdhci registers - and we should *not* allow that (because it may > > not always work), unless the sdhci controller has been runtime resumed > > first, right? > > I am OK with drivers that just want to use runtime PM to turn off a > functional clock. sdhci-tegra.c is also doing that although using the > clock framework. Yes, I agree. At least this works for SoC specific drivers. > > Certainly that approach assumes that the host controller's power state > is not changed due to runtime PM. > > To ensure that the host controller is runtime resumed before calling > sdhci_suspend_host(), we can just call pm_runtime_resume() I think. Yes, that was kind of what I proposed in the other thread as option 1) (except for the replacement of pm_runtime_force_suspend|resume). Although, to be clear I would probably use pm_runtime_get_sync() instead, to make sure the usage count is incremented too. I don't have a strong opinion here, but from an optimization point of view I would at least consider what I proposed in option 2) (in the other thread). The benefit is that it can allow us to potentially avoid runtime resuming the device, during system suspend. Kind regards Uffe
On 10/08/23 19:34, Ulf Hansson wrote: > On Thu, 10 Aug 2023 at 14:44, Adrian Hunter <adrian.hunter@intel.com> wrote: >> >> On 10/08/23 13:21, Ulf Hansson wrote: >>> On Thu, 10 Aug 2023 at 10:13, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>> >>>> On 8/08/23 23:23, 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> >>>>> --- >>>>> 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 | 72 ++++++++++++++++++++++++++++- >>>>> 1 file changed, 70 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c >>>>> index e68cd87998c8..c8e145031429 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); >>>>> @@ -606,6 +616,12 @@ static int dwcmshc_suspend(struct device *dev) >>>>> if (ret) >>>>> return ret; >>>>> >>>>> + ret = pm_runtime_force_suspend(dev); >>>>> + if (ret) { >>>>> + sdhci_resume_host(host); >>>>> + return ret; >>>>> + } >>>> >>>> Since you are only using the runtime PM callbacks to turn off the card >>>> clock via SDHCI_CLOCK_CONTROL, pm_runtime_force_suspend() and >>>> pm_runtime_force_resume() are not needed at all. >>> >>> Right, it can be done without these too. >>> >>>> >>>> sdhci_suspend_host() does not care if SDHCI_CLOCK_CARD_EN is on or off. >>>> (And you are disabling pltfm_host->clk and priv->bus_clk, so presumably >>>> the result is no clock either way) >>>> >>>> sdhci_resume_host() does not restore state unless >>>> SDHCI_QUIRK2_HOST_OFF_CARD_ON is used, it just resets, so the internal clock >>>> SDHCI_CLOCK_INT_EN is off which is consistent with either runtime suspended >>>> or runtime resumed. >>> >>> Even if this may work, to me, it doesn't look like good practice for >>> how to use runtime PM in combination with system wide suspend/resume. >>> >>> The point is, sdhci_suspend|resume_host() may end up reading/writing >>> to sdhci registers - and we should *not* allow that (because it may >>> not always work), unless the sdhci controller has been runtime resumed >>> first, right? >> >> I am OK with drivers that just want to use runtime PM to turn off a >> functional clock. sdhci-tegra.c is also doing that although using the >> clock framework. > > Yes, I agree. At least this works for SoC specific drivers. > >> >> Certainly that approach assumes that the host controller's power state >> is not changed due to runtime PM. >> >> To ensure that the host controller is runtime resumed before calling >> sdhci_suspend_host(), we can just call pm_runtime_resume() I think. > > Yes, that was kind of what I proposed in the other thread as option 1) > (except for the replacement of pm_runtime_force_suspend|resume). > > Although, to be clear I would probably use pm_runtime_get_sync() > instead, to make sure the usage count is incremented too. In that case, a matching pm_runtime_put() is needed also at the end of the resume callback. > > I don't have a strong opinion here, but from an optimization point of > view I would at least consider what I proposed in option 2) (in the > other thread). The benefit is that it can allow us to potentially > avoid runtime resuming the device, during system suspend. > > Kind regards > Uffe
On Fri, 11 Aug 2023 at 07:57, Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 10/08/23 19:34, Ulf Hansson wrote: > > On Thu, 10 Aug 2023 at 14:44, Adrian Hunter <adrian.hunter@intel.com> wrote: > >> > >> On 10/08/23 13:21, Ulf Hansson wrote: > >>> On Thu, 10 Aug 2023 at 10:13, Adrian Hunter <adrian.hunter@intel.com> wrote: > >>>> > >>>> On 8/08/23 23:23, 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> > >>>>> --- > >>>>> 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 | 72 ++++++++++++++++++++++++++++- > >>>>> 1 file changed, 70 insertions(+), 2 deletions(-) > >>>>> > >>>>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c > >>>>> index e68cd87998c8..c8e145031429 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); > >>>>> @@ -606,6 +616,12 @@ static int dwcmshc_suspend(struct device *dev) > >>>>> if (ret) > >>>>> return ret; > >>>>> > >>>>> + ret = pm_runtime_force_suspend(dev); > >>>>> + if (ret) { > >>>>> + sdhci_resume_host(host); > >>>>> + return ret; > >>>>> + } > >>>> > >>>> Since you are only using the runtime PM callbacks to turn off the card > >>>> clock via SDHCI_CLOCK_CONTROL, pm_runtime_force_suspend() and > >>>> pm_runtime_force_resume() are not needed at all. > >>> > >>> Right, it can be done without these too. > >>> > >>>> > >>>> sdhci_suspend_host() does not care if SDHCI_CLOCK_CARD_EN is on or off. > >>>> (And you are disabling pltfm_host->clk and priv->bus_clk, so presumably > >>>> the result is no clock either way) > >>>> > >>>> sdhci_resume_host() does not restore state unless > >>>> SDHCI_QUIRK2_HOST_OFF_CARD_ON is used, it just resets, so the internal clock > >>>> SDHCI_CLOCK_INT_EN is off which is consistent with either runtime suspended > >>>> or runtime resumed. > >>> > >>> Even if this may work, to me, it doesn't look like good practice for > >>> how to use runtime PM in combination with system wide suspend/resume. > >>> > >>> The point is, sdhci_suspend|resume_host() may end up reading/writing > >>> to sdhci registers - and we should *not* allow that (because it may > >>> not always work), unless the sdhci controller has been runtime resumed > >>> first, right? > >> > >> I am OK with drivers that just want to use runtime PM to turn off a > >> functional clock. sdhci-tegra.c is also doing that although using the > >> clock framework. > > > > Yes, I agree. At least this works for SoC specific drivers. > > > >> > >> Certainly that approach assumes that the host controller's power state > >> is not changed due to runtime PM. > >> > >> To ensure that the host controller is runtime resumed before calling > >> sdhci_suspend_host(), we can just call pm_runtime_resume() I think. > > > > Yes, that was kind of what I proposed in the other thread as option 1) > > (except for the replacement of pm_runtime_force_suspend|resume). > > > > Although, to be clear I would probably use pm_runtime_get_sync() > > instead, to make sure the usage count is incremented too. > > In that case, a matching pm_runtime_put() is needed also at the > end of the resume callback. Yes, of course. Or depending if we are using the force_suspend|resume helper, a pm_runtime_put_noidle is sufficient after pm_runtime_force_suspend() has been called. [...] Kind regards Uffe
> -----Original Message----- > From: Ulf Hansson <ulf.hansson@linaro.org> > Sent: Friday, August 11, 2023 4:36 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 v7] mmc: sdhci-of-dwcmshc: Add runtime PM operations > > On Fri, 11 Aug 2023 at 07:57, Adrian Hunter <adrian.hunter@intel.com> > wrote: > > > > On 10/08/23 19:34, Ulf Hansson wrote: > > > On Thu, 10 Aug 2023 at 14:44, Adrian Hunter <adrian.hunter@intel.com> > wrote: > > >> > > >> On 10/08/23 13:21, Ulf Hansson wrote: > > >>> On Thu, 10 Aug 2023 at 10:13, Adrian Hunter > <adrian.hunter@intel.com> wrote: > > >>>> > > >>>> On 8/08/23 23:23, 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> > > >>>>> --- > > >>>>> 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 | 72 > ++++++++++++++++++++++++++++- > > >>>>> 1 file changed, 70 insertions(+), 2 deletions(-) > > >>>>> > > >>>>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c > b/drivers/mmc/host/sdhci-of-dwcmshc.c > > >>>>> index e68cd87998c8..c8e145031429 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); > > >>>>> @@ -606,6 +616,12 @@ static int dwcmshc_suspend(struct device > *dev) > > >>>>> if (ret) > > >>>>> return ret; > > >>>>> > > >>>>> + ret = pm_runtime_force_suspend(dev); > > >>>>> + if (ret) { > > >>>>> + sdhci_resume_host(host); > > >>>>> + return ret; > > >>>>> + } > > >>>> > > >>>> Since you are only using the runtime PM callbacks to turn off the card > > >>>> clock via SDHCI_CLOCK_CONTROL, pm_runtime_force_suspend() and > > >>>> pm_runtime_force_resume() are not needed at all. > > >>> > > >>> Right, it can be done without these too. > > >>> > > >>>> > > >>>> sdhci_suspend_host() does not care if SDHCI_CLOCK_CARD_EN is on or > off. > > >>>> (And you are disabling pltfm_host->clk and priv->bus_clk, so > presumably > > >>>> the result is no clock either way) > > >>>> > > >>>> sdhci_resume_host() does not restore state unless > > >>>> SDHCI_QUIRK2_HOST_OFF_CARD_ON is used, it just resets, so the > internal clock > > >>>> SDHCI_CLOCK_INT_EN is off which is consistent with either runtime > suspended > > >>>> or runtime resumed. > > >>> > > >>> Even if this may work, to me, it doesn't look like good practice for > > >>> how to use runtime PM in combination with system wide > suspend/resume. > > >>> > > >>> The point is, sdhci_suspend|resume_host() may end up reading/writing > > >>> to sdhci registers - and we should *not* allow that (because it may > > >>> not always work), unless the sdhci controller has been runtime resumed > > >>> first, right? > > >> > > >> I am OK with drivers that just want to use runtime PM to turn off a > > >> functional clock. sdhci-tegra.c is also doing that although using the > > >> clock framework. > > > > > > Yes, I agree. At least this works for SoC specific drivers. > > > > > >> > > >> Certainly that approach assumes that the host controller's power state > > >> is not changed due to runtime PM. > > >> > > >> To ensure that the host controller is runtime resumed before calling > > >> sdhci_suspend_host(), we can just call pm_runtime_resume() I think. > > > > > > Yes, that was kind of what I proposed in the other thread as option 1) > > > (except for the replacement of pm_runtime_force_suspend|resume). > > > > > > Although, to be clear I would probably use pm_runtime_get_sync() > > > instead, to make sure the usage count is incremented too. > > > > In that case, a matching pm_runtime_put() is needed also at the > > end of the resume callback. > > Yes, of course. Or depending if we are using the force_suspend|resume > helper, a pm_runtime_put_noidle is sufficient after > pm_runtime_force_suspend() has been called. Thanks Ulf/Adrian! Plan to upload v8 with the following changes: - Remove pm_runtime_force_suspend/resume() from dwcmshc_suspend()/dwcmshc_resume() (Adrian's comment). - Add comments for dwcmshc_resume()/dwcmshc_suspend(); (According to Andrian's comment). - Add pm_runtime_get_sync()/pm_runtime_put() in dwcmshc_suspend(), which is Ulf option-1. Option-2 seems more efficient, but it involves more changes and I couldn't test the impact on other SoC. Maybe for future enhancement? > > [...] > > Kind regards > Uffe
[...] > > > >>>> > > > >>>> Since you are only using the runtime PM callbacks to turn off the card > > > >>>> clock via SDHCI_CLOCK_CONTROL, pm_runtime_force_suspend() and > > > >>>> pm_runtime_force_resume() are not needed at all. > > > >>> > > > >>> Right, it can be done without these too. > > > >>> > > > >>>> > > > >>>> sdhci_suspend_host() does not care if SDHCI_CLOCK_CARD_EN is on or > > off. > > > >>>> (And you are disabling pltfm_host->clk and priv->bus_clk, so > > presumably > > > >>>> the result is no clock either way) > > > >>>> > > > >>>> sdhci_resume_host() does not restore state unless > > > >>>> SDHCI_QUIRK2_HOST_OFF_CARD_ON is used, it just resets, so the > > internal clock > > > >>>> SDHCI_CLOCK_INT_EN is off which is consistent with either runtime > > suspended > > > >>>> or runtime resumed. > > > >>> > > > >>> Even if this may work, to me, it doesn't look like good practice for > > > >>> how to use runtime PM in combination with system wide > > suspend/resume. > > > >>> > > > >>> The point is, sdhci_suspend|resume_host() may end up reading/writing > > > >>> to sdhci registers - and we should *not* allow that (because it may > > > >>> not always work), unless the sdhci controller has been runtime resumed > > > >>> first, right? > > > >> > > > >> I am OK with drivers that just want to use runtime PM to turn off a > > > >> functional clock. sdhci-tegra.c is also doing that although using the > > > >> clock framework. > > > > > > > > Yes, I agree. At least this works for SoC specific drivers. > > > > > > > >> > > > >> Certainly that approach assumes that the host controller's power state > > > >> is not changed due to runtime PM. > > > >> > > > >> To ensure that the host controller is runtime resumed before calling > > > >> sdhci_suspend_host(), we can just call pm_runtime_resume() I think. > > > > > > > > Yes, that was kind of what I proposed in the other thread as option 1) > > > > (except for the replacement of pm_runtime_force_suspend|resume). > > > > > > > > Although, to be clear I would probably use pm_runtime_get_sync() > > > > instead, to make sure the usage count is incremented too. > > > > > > In that case, a matching pm_runtime_put() is needed also at the > > > end of the resume callback. > > > > Yes, of course. Or depending if we are using the force_suspend|resume > > helper, a pm_runtime_put_noidle is sufficient after > > pm_runtime_force_suspend() has been called. > > Thanks Ulf/Adrian! Plan to upload v8 with the following changes: > - Remove pm_runtime_force_suspend/resume() from dwcmshc_suspend()/dwcmshc_resume() (Adrian's comment). > - Add comments for dwcmshc_resume()/dwcmshc_suspend(); > (According to Andrian's comment). > - Add pm_runtime_get_sync()/pm_runtime_put() in dwcmshc_suspend(), which is Ulf option-1. Option-2 seems more efficient, but it involves more changes and I couldn't test the impact on other SoC. Maybe for future enhancement? That works for me! Kind regards Uffe
diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c index e68cd87998c8..c8e145031429 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); @@ -606,6 +616,12 @@ static int dwcmshc_suspend(struct device *dev) if (ret) return ret; + ret = pm_runtime_force_suspend(dev); + if (ret) { + sdhci_resume_host(host); + return ret; + } + clk_disable_unprepare(pltfm_host->clk); if (!IS_ERR(priv->bus_clk)) clk_disable_unprepare(priv->bus_clk); @@ -625,6 +641,10 @@ static int dwcmshc_resume(struct device *dev) struct rk35xx_priv *rk_priv = priv->priv; int ret; + ret = pm_runtime_force_resume(dev); + if (ret) + return ret; + ret = clk_prepare_enable(pltfm_host->clk); if (ret) return ret; @@ -646,7 +666,55 @@ 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); + 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 = {