lkml.org 
[lkml]   [2023]   [Aug]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v8] mmc: sdhci-of-dwcmshc: Add runtime PM operations
    From
    On 17/08/23 19:22, Liming Sun wrote:
    >
    >
    >> -----Original Message-----
    >> From: Adrian Hunter <adrian.hunter@intel.com>
    >> Sent: Thursday, August 17, 2023 3:36 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 v8] mmc: sdhci-of-dwcmshc: Add runtime PM operations
    >>
    >> On 17/08/23 04:06, 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>
    >>> ---
    >>> 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, 73 insertions(+), 3 deletions(-)
    >>>
    >>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-
    >> of-dwcmshc.c
    >>> index e68cd87998c8..2196218c9d79 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);
    >>> @@ -594,6 +604,10 @@ static int dwcmshc_remove(struct platform_device
    >> *pdev)
    >>> }
    >>>
    >>> #ifdef CONFIG_PM_SLEEP
    >>> +/*
    >>> + * Note, runtime suspend changes only SDHCI_CLOCK_CARD_EN which has
    >> no effect on
    >>> + * system suspend.
    >>> + */
    >>
    >> This comment isn't needed since pm_runtime_get_sync() will
    >> always runtime resume the device if needed.
    >
    > Updated in v9.
    >
    >>
    >>> static int dwcmshc_suspend(struct device *dev)
    >>> {
    >>> struct sdhci_host *host = dev_get_drvdata(dev);
    >>> @@ -602,9 +616,11 @@ static int dwcmshc_suspend(struct device *dev)
    >>> struct rk35xx_priv *rk_priv = priv->priv;
    >>> int ret;
    >>>
    >>> + pm_runtime_get_sync(dev);
    >>
    >> This needs a corresponding pm_runtime_put() at the end of
    >> dwcmshc_resume().
    >
    > Updated in v9.
    >
    >>
    >>> +
    >>> ret = sdhci_suspend_host(host);
    >>> if (ret)
    >>> - return ret;
    >>> + goto err_suspend;
    >>>
    >>> clk_disable_unprepare(pltfm_host->clk);
    >>> if (!IS_ERR(priv->bus_clk))
    >>> @@ -614,9 +630,15 @@ static int dwcmshc_suspend(struct device *dev)
    >>> clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
    >>> rk_priv->rockchip_clks);
    >>>
    >>> +err_suspend:
    >>> + pm_runtime_put_noidle(dev);
    >>> return ret;
    >>> }
    >>>
    >>> +/*
    >>> + * Note, system resume leaves SDHCI_CLOCK_INT_EN off which is consistent
    >> with
    >>> + * either runtime suspended or runtime resumed.
    >>> + */
    >>
    >> As above, this comment isn't needed.
    >
    > Updated in v9.
    >
    >>
    >>> static int dwcmshc_resume(struct device *dev)
    >>
    >>
    >> As mentioned above, dwcmshc_resume() needs a pm_runtime_put()
    >> to balance the pm_runtime_get_sync().
    >
    > Updated in v9.
    >
    >>
    >> Could fix up the error path too, but that should be a
    >> separate prior patch.
    >>
    >> Probably end up looking like:
    >>
    >>
    >> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-
    >> of-dwcmshc.c
    >> index 20aa9b6327d2..c9b020b7a3f6 100644
    >> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
    >> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
    >> @@ -652,17 +652,33 @@ static int dwcmshc_resume(struct device *dev)
    >> if (!IS_ERR(priv->bus_clk)) {
    >> ret = clk_prepare_enable(priv->bus_clk);
    >> if (ret)
    >> - return ret;
    >> + goto disable_clk;
    >> }
    >>
    >> if (rk_priv) {
    >> ret = clk_bulk_prepare_enable(RK35xx_MAX_CLKS,
    >> rk_priv->rockchip_clks);
    >> if (ret)
    >> - return ret;
    >> + goto disable_bus_clk;
    >> }
    >>
    >> - return sdhci_resume_host(host);
    >> + ret = sdhci_resume_host(host);
    >> + if (ret)
    >> + goto disable_rockchip_clks;
    >> +
    >> + pm_runtime_put(dev);
    >> +
    >> + return 0;
    >> +
    >> +disable_rockchip_clks:
    >> + if (rk_priv)
    >> + clk_bulk_disable_unprepare(RK35xx_MAX_CLKS, rk_priv-
    >>> rockchip_clks);
    >> +disable_bus_clk:
    >> + if (!IS_ERR(priv->bus_clk))
    >> + clk_disable_unprepare(priv->bus_clk);
    >> +disable_clk:
    >> + clk_disable_unprepare(pltfm_host->clk);
    >> + return ret;
    >> }
    >> #endif
    >>
    >>
    >
    > Do you mean a separate 'post' patch? I left it out in v9 and could post another one for this error path. Or do you prefer the same patch series?

    Same patch series, 2 patches. First patch fixes the error paths in dwcmshc_resume().
    Second patch adds runtime pm.

    >
    >>
    >>
    >>> {
    >>> struct sdhci_host *host = dev_get_drvdata(dev);
    >>> @@ -646,7 +668,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 = {
    >

    \
     
     \ /
      Last update: 2023-08-18 09:28    [W:5.196 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site