Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220330075255.12127-1-a-govindraju@ti.com>
In-Reply-To: <20220330075255.12127-1-a-govindraju@ti.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Thu, 7 Apr 2022 12:42:57 +0200
Message-ID: <CAPDyKFoEZPaPG1bGq+nHtyO=FNzSfmwDPtVh5tobSJcctmQD4A@mail.gmail.com>
Subject: Re: [PATCH RFC] mmc: sdhci_am654: Add support for PM suspend/resume
To:     Aswath Govindraju <a-govindraju@ti.com>
Cc:     Kishon Vijay Abraham I <kishon@ti.com>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Adrian Hunter <adrian.hunter@intel.com>,
        linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org,
        Faiz Abbas <faiz_abbas@ti.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

+ Faiz

On Wed, 30 Mar 2022 at 09:53, Aswath Govindraju <a-govindraju@ti.com> wrote:
>
> Add support for suspend/resume and pm_runtime resume/suspend.
>
> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> ---
>  drivers/mmc/host/sdhci_am654.c | 204 ++++++++++++++++++++++++++++++---
>  1 file changed, 191 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index e54fe24d47e7..e86df72dfd78 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -84,6 +84,7 @@
>  #define DRIVER_STRENGTH_40_OHM 0x4
>
>  #define CLOCK_TOO_SLOW_HZ      50000000
> +#define SDHCI_AM654_AUTOSUSPEND_DELAY  100
>
>  /* Command Queue Host Controller Interface Base address */
>  #define SDHCI_AM654_CQE_BASE_ADDR 0x200
> @@ -791,16 +792,10 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>
>         pltfm_host->clk = clk_xin;
>
> -       /* Clocks are enabled using pm_runtime */
> -       pm_runtime_enable(dev);
> -       ret = pm_runtime_resume_and_get(dev);
> -       if (ret)
> -               goto pm_runtime_disable;
> -
>         base = devm_platform_ioremap_resource(pdev, 1);
>         if (IS_ERR(base)) {
>                 ret = PTR_ERR(base);
> -               goto pm_runtime_put;
> +               goto err_pltfm_free;
>         }
>
>         sdhci_am654->base = devm_regmap_init_mmio(dev, base,
> @@ -808,30 +803,42 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>         if (IS_ERR(sdhci_am654->base)) {
>                 dev_err(dev, "Failed to initialize regmap\n");
>                 ret = PTR_ERR(sdhci_am654->base);
> -               goto pm_runtime_put;
> +               goto err_pltfm_free;
>         }
>
>         ret = sdhci_am654_get_of_property(pdev, sdhci_am654);
>         if (ret)
> -               goto pm_runtime_put;
> +               goto err_pltfm_free;
>
>         ret = mmc_of_parse(host->mmc);
>         if (ret) {
>                 dev_err(dev, "parsing dt failed (%d)\n", ret);
> -               goto pm_runtime_put;
> +               goto err_pltfm_free;
>         }
>
>         host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
>
> +       pm_runtime_set_active(dev);
> +       pm_runtime_enable(dev);
> +       clk_prepare_enable(pltfm_host->clk);

I think some error handling is missing, at least for clk_prepare_enable().

> +       ret = pm_runtime_resume_and_get(dev);

This can be replaced with a pm_runtime_get_noresume() - and I think it
would improve the readability of the code, to put the call above
pm_runtime_set_active().

> +       if (ret)
> +               goto clk_disable;
> +
>         ret = sdhci_am654_init(host);
>         if (ret)
> -               goto pm_runtime_put;
> +               goto clk_disable;
>
> +       /* Setting up autosuspend */
> +       pm_runtime_set_autosuspend_delay(dev, SDHCI_AM654_AUTOSUSPEND_DELAY);
> +       pm_runtime_use_autosuspend(dev);
> +       pm_runtime_mark_last_busy(dev);
> +       pm_runtime_put_autosuspend(dev);
>         return 0;
>
> -pm_runtime_put:
> +clk_disable:
> +       clk_disable_unprepare(pltfm_host->clk);
>         pm_runtime_put_sync(dev);
> -pm_runtime_disable:
>         pm_runtime_disable(dev);
>  err_pltfm_free:
>         sdhci_pltfm_free(pdev);
> @@ -841,6 +848,7 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>  static int sdhci_am654_remove(struct platform_device *pdev)
>  {
>         struct sdhci_host *host = platform_get_drvdata(pdev);
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>         int ret;
>
>         sdhci_remove_host(host, true);
> @@ -848,16 +856,186 @@ static int sdhci_am654_remove(struct platform_device *pdev)
>         if (ret < 0)
>                 return ret;
>
> +       clk_disable_unprepare(pltfm_host->clk);

To gate the clock, you need to make sure it has been ungated first. As
you anyway need to add a call pm_runtime_get_sync() prior to calling
sdhci_remove_host() a few lines above, this would fix it.

Moreover, the existing call to pm_runtime_put_sync() a few lines above
in sdhci_am654_remove(), should be replaced with a call to
pm_runtime_put_noidle() - and that call should be made below the call
pm_runtime_disable() to become correct.

>         pm_runtime_disable(&pdev->dev);
>         sdhci_pltfm_free(pdev);
> +       return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int sdhci_am654_restore(struct sdhci_host *host)
> +{
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
> +       u32 ctl_cfg_2 = 0;
> +       u32 val;
> +       int ret;
> +
> +       if (sdhci_am654->flags & DLL_CALIB) {
> +               regmap_read(sdhci_am654->base, PHY_STAT1, &val);
> +               if (~val & CALDONE_MASK) {
> +                       /* Calibrate IO lines */
> +                       regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
> +                                          PDB_MASK, PDB_MASK);
> +                       ret = regmap_read_poll_timeout(sdhci_am654->base,
> +                                                      PHY_STAT1, val,
> +                                                      val & CALDONE_MASK,
> +                                                      1, 20);
> +                       if (ret)
> +                               return ret;
> +               }
> +       }
> +
> +       /* Enable pins by setting IO mux to 0 */
> +       if (sdhci_am654->flags & IOMUX_PRESENT)
> +               regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
> +                                  IOMUX_ENABLE_MASK, 0);
> +
> +       /* Set slot type based on SD or eMMC */
> +       if (host->mmc->caps & MMC_CAP_NONREMOVABLE)
> +               ctl_cfg_2 = SLOTTYPE_EMBEDDED;
> +
> +       regmap_update_bits(sdhci_am654->base, CTL_CFG_2, SLOTTYPE_MASK,
> +                          ctl_cfg_2);
> +
> +       regmap_read(sdhci_am654->base, CTL_CFG_3, &val);
> +       if (~val & TUNINGFORSDR50_MASK)
> +               /* Enable tuning for SDR50 */
> +               regmap_update_bits(sdhci_am654->base, CTL_CFG_3, TUNINGFORSDR50_MASK,
> +                                  TUNINGFORSDR50_MASK);
>
>         return 0;
>  }
>
> +static int sdhci_am654_runtime_suspend(struct device *dev)
> +{
> +       struct sdhci_host *host = dev_get_drvdata(dev);
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       int ret;
> +
> +       if (host->tuning_mode != SDHCI_TUNING_MODE_3)
> +               mmc_retune_needed(host->mmc);
> +
> +       ret = cqhci_suspend(host->mmc);
> +       if (ret)
> +               return ret;
> +
> +       ret = sdhci_runtime_suspend_host(host);
> +       if (ret)
> +               return ret;
> +
> +       /* disable the clock */
> +       clk_disable_unprepare(pltfm_host->clk);
> +       return 0;
> +}
> +
> +static int sdhci_am654_runtime_resume(struct device *dev)
> +{
> +       struct sdhci_host *host = dev_get_drvdata(dev);
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       int ret;
> +
> +       /* Enable the clock */
> +       clk_prepare_enable(pltfm_host->clk);
> +       ret = sdhci_am654_restore(host);
> +       if (ret)
> +               return ret;
> +
> +       ret = sdhci_runtime_resume_host(host, 0);
> +       if (ret)
> +               return ret;
> +
> +       ret = cqhci_resume(host->mmc);
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +#endif
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int sdhci_am654_suspend(struct device *dev)
> +{
> +       struct sdhci_host *host = dev_get_drvdata(dev);
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       int ret;
> +
> +       if (!host)
> +               return 0;
> +
> +       ret = pm_runtime_get_sync(dev);
> +       if (ret < 0)
> +               return ret;
> +
> +       if (host->tuning_mode != SDHCI_TUNING_MODE_3)
> +               mmc_retune_needed(host->mmc);
> +
> +       ret = cqhci_suspend(host->mmc);
> +       if (ret)
> +               return ret;
> +
> +       ret = sdhci_suspend_host(host);
> +       if (ret)
> +               return ret;
> +
> +       ret = pm_runtime_put_sync(dev);
> +
> +       /* disable the clock */
> +       clk_disable_unprepare(pltfm_host->clk);
> +
> +       return 0;
> +}
> +
> +static int sdhci_am654_resume(struct device *dev)
> +{
> +       struct sdhci_host *host = dev_get_drvdata(dev);
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       int ret;
> +
> +       if (!host)
> +               return 0;
> +
> +       /* Enable the clock */
> +       clk_prepare_enable(pltfm_host->clk);
> +       ret = pm_runtime_get_sync(dev);
> +       if (ret < 0)
> +               goto clk_unprepare;
> +
> +       ret = sdhci_am654_restore(host);
> +       if (ret)
> +               goto put;
> +
> +       ret = sdhci_resume_host(host);
> +       if (ret)
> +               goto put;
> +
> +       ret = cqhci_resume(host->mmc);
> +       if (ret)
> +               goto put;
> +
> +       pm_runtime_mark_last_busy(dev);
> +       pm_runtime_put_autosuspend(dev);
> +       return 0;
> +
> +put:
> +       pm_runtime_put_sync(dev);
> +clk_unprepare:
> +       clk_disable_unprepare(pltfm_host->clk);
> +       return ret;
> +}

By looking at the code in sdhci_am654_suspend|resume(), I wonder if
those is better replaced with assigning system PM callbacks to
pm_runtime_force_suspend|resume() instead. Other sdhci variants do
this already, you may have a look at those to better get the idea.

> +#endif
> +
> +static const struct dev_pm_ops sdhci_am654_dev_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(sdhci_am654_suspend, sdhci_am654_resume)
> +       .runtime_suspend = sdhci_am654_runtime_suspend,
> +       .runtime_resume = sdhci_am654_runtime_resume,

For runtime PM callbacks, you can use SET_RUNTIME_PM_OPS(). Otherwise
I think this will cause a build error when built with CONFIG_PM unset.

> +};
> +
>  static struct platform_driver sdhci_am654_driver = {
>         .driver = {
>                 .name = "sdhci-am654",
>                 .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> +               .pm = &sdhci_am654_dev_pm_ops,
>                 .of_match_table = sdhci_am654_of_match,
>         },
>         .probe = sdhci_am654_probe,

Kind regards
Uffe