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

[Ulf Hansson]

On Mon, 3 Apr 2023 at 19:42, Vignesh Raghavendra <vigneshr@xxxxxx> wrote:
>
> Hi,
>
> On 31/03/23 20:01, Ulf Hansson wrote:
> > 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?
> >
>
> As part of wider teting, I discovered that on certain boards (J721e EVM
> for example), there is a latency of ~500ms for controller to start up
> (HW limitation around card detect debounce timer [0]). This results in
> momentary (but noticeable) lag when playing a video for example.

Ohh, that kind of sucks. There is no way to workaround that I guess?

>
>
> [0[
> https://elixir.bootlin.com/linux/latest/source/drivers/mmc/host/sdhci_am654.c#L361
>
>
> > 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?
>
> Its more of the startup delay than how long controller should remain
> active. So tuning autosuspend_delay does not seem to help.
>
> >
> > Of course, in the end this is a choice of performance in favor of
> > wasting energy.
>
> Yes, this is choice of opt-in vs opt-out. Users can tune the delay via
> sysfs as needed.
>
> I am setting it to -1 to avoid regressing any userspace apps (vs
> enabling autosuspend and then asking impacted users to set to -1)

Alright, makes sense!

>
> One solution is to use -1 on platforms where latency is significant
> based on compatible / SoC detection?

That's a good idea! It seems a pity to make all SoCs suffer, if there
is only a subset that has this problem. Although, let's consider that
as an improvement that can be done on top of $subject patch.

So, applied for next, thanks!

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
> >>
>
> --
> Regards
> Vignesh