On Fri, 8 Apr 2022 at 14:43, Aswath Govindraju <a-govindraju@xxxxxx> wrote:
>
> Add support for suspend/resume and pm_runtime resume/suspend.
>
> Signed-off-by: Aswath Govindraju <a-govindraju@xxxxxx>
> ---
>
> Changes since v1:
> - Removed System pm calls and used force_runtime_pm calls instead
> - Added error handling at required places
> - Added pm_runtime_sync call in the sdhci_am654_remove function
> - Replaced pm_runtime_*_sync calls in probe with noidle and noresume
> calls
> - Used MACRO SET_RUNTIME_PM_OPS to avoid build errors when CONFIG_PM
> is disabled
>
> drivers/mmc/host/sdhci_am654.c | 146 +++++++++++++++++++++++++++++----
> 1 file changed, 132 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index e54fe24d47e7..131b2b9ae0e7 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,31 +803,47 @@ 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_get_noresume(dev);
> + ret = pm_runtime_set_active(dev);
> + if (ret)
> + goto pm_put;
> + pm_runtime_enable(dev);
> + ret = clk_prepare_enable(pltfm_host->clk);
> + if (ret)
> + goto pm_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:
> - pm_runtime_put_sync(dev);
> -pm_runtime_disable:
> +clk_disable:
> + clk_disable_unprepare(pltfm_host->clk);
> +pm_disable:
> pm_runtime_disable(dev);
> +pm_put:
> + pm_runtime_put_noidle(dev);
> err_pltfm_free:
> sdhci_pltfm_free(pdev);
> return ret;
> @@ -841,23 +852,130 @@ 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;
>
> + ret = pm_runtime_get_sync(&pdev->dev);
pm_runtime_resume_and_get() is better suited here, as it helps us to
restore the usage count in the error path.
> + if (ret < 0)
> + return ret;
> +
> sdhci_remove_host(host, true);
> ret = pm_runtime_put_sync(&pdev->dev);
We need to drop the above call to pm_runtime_put_sync(). Otherwise you
may potentially end up gating the clock, in which case the below call
to clk_disable_unprepare() becomes wrong.
> if (ret < 0)
> return ret;
>
> + clk_disable_unprepare(pltfm_host->clk);
> pm_runtime_disable(&pdev->dev);
You need a call to pm_runtime_put_noidle() here to restore the RPM
usage count to zero.
> sdhci_pltfm_free(pdev);
> + return 0;
> +}
[...]
Other than the rather minor things above, this looks good to me!
Kind regards
Uffe
