Re: [PATCH v3] mmc: sdhci_am654: Add support for PM suspend/resume

From: Ulf Hansson
Date: Fri Mar 31 2023 - 10:32:25 EST

Next message: Carlos Bilbao: "Re: [PATCH] docs/sp_SP: Add translation of process/adding-syscalls"
Previous message: Ahelenia Ziemiańska: "[PATCH] KEYS: Make use of platform keyring for module signature verification"
In reply to: Vignesh Raghavendra: "Re: [PATCH v3] mmc: sdhci_am654: Add support for PM suspend/resume"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, 31 Mar 2023 at 12:17, Vignesh Raghavendra <vigneshr@xxxxxx> wrote:
>
> From: Aswath Govindraju <a-govindraju@xxxxxx>
>
> Add support for suspend/resume and pm_runtime resume/suspend.
>
> Signed-off-by: Aswath Govindraju <a-govindraju@xxxxxx>
> Signed-off-by: Georgi Vlaev <g-vlaev@xxxxxx>
> Signed-off-by: Vignesh Raghavendra <vigneshr@xxxxxx>
> ---
>
> Since RFC v2:
> Address all comments around sdhci_am654_remove()
> Set autosuspend_delay to -1 as SDHCI will host rootfs via SD/eMMC and
> autosuspend can cause long latency for user interactive applications

I am curious about the "long" latency. Do you have some data that you
can share for how long it takes to runtime-resume the device?

Using -1 as the default value for the autosuspend_delay seems fine to
me, but did you consider using a longer timeout than the common 100ms?
Could that perhaps be a better default option?

Of course, in the end this is a choice of performance in favor of
wasting energy.

Kind regards
Uffe

>
> RFC v2:
> https://lore.kernel.org/all/20220408124338.27090-1-a-govindraju@xxxxxx/
>
> drivers/mmc/host/sdhci_am654.c | 147 +++++++++++++++++++++++++++++----
> 1 file changed, 131 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index 672d37ea98d0..7cdf0f54e3a5 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -85,6 +85,7 @@
> #define DRIVER_STRENGTH_40_OHM 0x4
>
> #define CLOCK_TOO_SLOW_HZ 50000000
> +#define SDHCI_AM654_AUTOSUSPEND_DELAY -1
>
> /* Command Queue Host Controller Interface Base address */
> #define SDHCI_AM654_CQE_BASE_ADDR 0x200
> @@ -808,16 +809,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,
> @@ -825,31 +820,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_probe(dev, ret, "parsing dt failed\n");
> - 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;
> @@ -858,23 +869,127 @@ 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);
> - ret = pm_runtime_put_sync(&pdev->dev);
> + ret = pm_runtime_resume_and_get(&pdev->dev);
> if (ret < 0)
> return ret;
>
> + sdhci_remove_host(host, true);
> + clk_disable_unprepare(pltfm_host->clk);
> pm_runtime_disable(&pdev->dev);
> + pm_runtime_put_noidle(&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 */
> + ret = clk_prepare_enable(pltfm_host->clk);
> + if (ret)
> + return ret;
> +
> + 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
> +
> +static const struct dev_pm_ops sdhci_am654_dev_pm_ops = {
> + SET_RUNTIME_PM_OPS(sdhci_am654_runtime_suspend,
> + sdhci_am654_runtime_resume, NULL)
> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> + pm_runtime_force_resume)
> +};
> +
> 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,
> --
> 2.40.0
>

Next message: Carlos Bilbao: "Re: [PATCH] docs/sp_SP: Add translation of process/adding-syscalls"
Previous message: Ahelenia Ziemiańska: "[PATCH] KEYS: Make use of platform keyring for module signature verification"
In reply to: Vignesh Raghavendra: "Re: [PATCH v3] mmc: sdhci_am654: Add support for PM suspend/resume"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]