Message-ID: <CAPDyKFp1K1+J-urcdcOstDqVMnTaEUfc_jJri0ZgNkWB8cpuKQ@mail.gmail.com>
Date:   Mon, 24 Aug 2020 09:20:04 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Aniruddha Rao <anrao@...dia.com>
Cc:     Adrian Hunter <adrian.hunter@...el.com>,
        Thierry Reding <thierry.reding@...il.com>,
        Jon Hunter <jonathanh@...dia.com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        linux-tegra <linux-tegra@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mmc: tegra: Add Runtime PM callbacks

[...]

> @@ -1622,7 +1699,6 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
>
>                 goto err_clk_get;
>         }
> -       clk_prepare_enable(clk);
>         pltfm_host->clk = clk;
>
>         tegra_host->rst = devm_reset_control_get_exclusive(&pdev->dev,
> @@ -1645,16 +1721,29 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
>
>         usleep_range(2000, 4000);
>
> +       pm_runtime_enable(&pdev->dev);
> +       rc = pm_runtime_get_sync(&pdev->dev);
> +       if (rc < 0)
> +               goto pm_disable;
> +       pm_runtime_set_autosuspend_delay(&pdev->dev,
> +                                        SDHCI_TEGRA_RTPM_MSEC_TMOUT);
> +       pm_runtime_use_autosuspend(&pdev->dev);
> +
>         rc = sdhci_tegra_add_host(host);
>         if (rc)
>                 goto err_add_host;
>
> +       pm_runtime_mark_last_busy(&pdev->dev);
> +       pm_runtime_put_autosuspend(&pdev->dev);
> +
>         return 0;
>
>  err_add_host:
>         reset_control_assert(tegra_host->rst);
> +       pm_runtime_put_autosuspend(&pdev->dev);
> +pm_disable:
> +       pm_runtime_disable(&pdev->dev);
>  err_rst_get:
> -       clk_disable_unprepare(pltfm_host->clk);
>  err_clk_get:
>  err_power_req:
>  err_parse_dt:
> @@ -1679,6 +1768,41 @@ static int sdhci_tegra_remove(struct platform_device *pdev)
>         return 0;
>  }
>
> +static int sdhci_tegra_runtime_suspend(struct device *dev)
> +{
> +       struct sdhci_host *host = dev_get_drvdata(dev);
> +
> +       /* Disable SDMMC internal clock */
> +       sdhci_set_clock(host, 0);
> +
> +       /* Disable SDMMC host CAR clock and BG trimmer supply */
> +       return tegra_sdhci_set_host_clock(host, false);

Shouldn't you also call sdhci_runtime_suspend_host() somewhere around
here, to mask IRQs etc.

> +}
> +
> +static int sdhci_tegra_runtime_resume(struct device *dev)
> +{
> +       struct sdhci_host *host = dev_get_drvdata(dev);
> +       unsigned int clk;
> +       int err = 0;
> +
> +       /* Clock enable should be invoked with a non-zero freq */
> +       if (host->clock)
> +               clk = host->clock;
> +       else if (host->mmc->ios.clock)
> +               clk = host->mmc->ios.clock;
> +       else
> +               clk = SDHCI_TEGRA_FALLBACK_CLK_HZ;
> +
> +       /* Enable SDMMC host CAR clock and BG trimmer supply */

I don't know the Tegra controller very well, but to me, looks odd that
the BG trimmer supply hasn't been handled before. Looks like you need
to enable that, even if you don't use runtime PM, no?

> +       err = tegra_sdhci_set_host_clock(host, true);
> +       if (!err) {
> +               /* Re-enable SDMMC internal clock */
> +               sdhci_set_clock(host, clk);
> +       }

Maybe you need to call sdhci_runtime_resume_host() somewhere around here?

> +
> +       return err;
> +}
> +
>  #ifdef CONFIG_PM_SLEEP
>  static int __maybe_unused sdhci_tegra_suspend(struct device *dev)
>  {
> @@ -1686,6 +1810,12 @@ static int __maybe_unused sdhci_tegra_suspend(struct device *dev)
>         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>         int ret;
>
> +       if (pm_runtime_status_suspended(dev)) {
> +               ret = tegra_sdhci_set_host_clock(host, true);
> +               if (ret)
> +                       return ret;
> +       }

So you need to re-enable the clock above, if it's been turned off in
runtime suspend, to complete the below operations.

That makes me wonder about the below operations. Why don't you need to
call cqhci_suspend() at runtime suspend?

> +
>         if (host->mmc->caps2 & MMC_CAP2_CQE) {
>                 ret = cqhci_suspend(host->mmc);
>                 if (ret)
> @@ -1698,8 +1828,7 @@ static int __maybe_unused sdhci_tegra_suspend(struct device *dev)
>                 return ret;
>         }
>
> -       clk_disable_unprepare(pltfm_host->clk);
> -       return 0;
> +       return tegra_sdhci_set_host_clock(host, false);
>  }
>
>  static int __maybe_unused sdhci_tegra_resume(struct device *dev)
> @@ -1708,7 +1837,7 @@ static int __maybe_unused sdhci_tegra_resume(struct device *dev)
>         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>         int ret;
>
> -       ret = clk_prepare_enable(pltfm_host->clk);
> +       ret = tegra_sdhci_set_host_clock(host, true);
>         if (ret)
>                 return ret;
>
> @@ -1727,13 +1856,15 @@ static int __maybe_unused sdhci_tegra_resume(struct device *dev)
>  suspend_host:
>         sdhci_suspend_host(host);
>  disable_clk:
> -       clk_disable_unprepare(pltfm_host->clk);
> -       return ret;
> +       return tegra_sdhci_set_host_clock(host, false);
>  }
>  #endif
>
> -static SIMPLE_DEV_PM_OPS(sdhci_tegra_dev_pm_ops, sdhci_tegra_suspend,
> -                        sdhci_tegra_resume);
> +const struct dev_pm_ops sdhci_tegra_dev_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(sdhci_tegra_suspend, sdhci_tegra_resume)
> +       SET_RUNTIME_PM_OPS(sdhci_tegra_runtime_suspend,
> +                          sdhci_tegra_runtime_resume, NULL)
> +};
>
>  static struct platform_driver sdhci_tegra_driver = {
>         .driver         = {
> --
> 2.7.4
>

Kind regards
Uffe

Powered by blists - more mailing lists