Re: [PATCH] Revert "mmc: sdhci-xenon: add runtime pm support and reimplement standby"

[Jisheng Zhang]

On Wed, 13 May 2020 14:15:21 +0200 Ulf Hansson wrote:

> 
> 
> On Wed, 13 May 2020 at 11:47, Jisheng Zhang <Jisheng.Zhang@xxxxxxxxxxxxx> wrote:
> >
> > This reverts commit a027b2c5fed78851e69fab395b02d127a7759fc7.
> >
> > The HW supports auto clock gating, so it's useless to do runtime pm
> > in software.  
> 
> Runtime PM isn't soley about clock gating. Moreover it manages the

Per my understanding, current xenon rpm implementation is just clock gating.

> "pltfm_host->clk", which means even if the controller supports auto
> clock gating, gating/ungating the externally provided clock still
> makes sense.

       clock -----------  xenon IP
      |___ rpm           |__ HW Auto clock gate

Per my understanding, with rpm, both clock and IP is clock gated; while with
Auto clock gate, the IP is clock gated. So the only difference is clock itself.
Considering the gain(suspect we have power consumption gain, see below), the
pay -- 56 LoCs and latency doesn't deserve gain.

Even if considering from power consumption POV, sdhci_runtime_suspend_host(),
sdhci_runtime_resume_host(), and the retune process could be more than the clock
itself.


> 
> Kind regards
> Uffe
> 
> >
> > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@xxxxxxxxxxxxx>
> > ---
> >  drivers/mmc/host/sdhci-xenon.c | 87 +++++++---------------------------
> >  drivers/mmc/host/sdhci-xenon.h |  1 -
> >  2 files changed, 16 insertions(+), 72 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-xenon.c b/drivers/mmc/host/sdhci-xenon.c
> > index 4703cd540c7f..85414e13e7ea 100644
> > --- a/drivers/mmc/host/sdhci-xenon.c
> > +++ b/drivers/mmc/host/sdhci-xenon.c
> > @@ -15,8 +15,6 @@
> >  #include <linux/ktime.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > -#include <linux/pm.h>
> > -#include <linux/pm_runtime.h>
> >
> >  #include "sdhci-pltfm.h"
> >  #include "sdhci-xenon.h"
> > @@ -539,24 +537,13 @@ static int xenon_probe(struct platform_device *pdev)
> >         if (err)
> >                 goto err_clk_axi;
> >
> > -       pm_runtime_get_noresume(&pdev->dev);
> > -       pm_runtime_set_active(&pdev->dev);
> > -       pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> > -       pm_runtime_use_autosuspend(&pdev->dev);
> > -       pm_runtime_enable(&pdev->dev);
> > -       pm_suspend_ignore_children(&pdev->dev, 1);
> > -
> >         err = sdhci_add_host(host);
> >         if (err)
> >                 goto remove_sdhc;
> >
> > -       pm_runtime_put_autosuspend(&pdev->dev);
> > -
> >         return 0;
> >
> >  remove_sdhc:
> > -       pm_runtime_disable(&pdev->dev);
> > -       pm_runtime_put_noidle(&pdev->dev);
> >         xenon_sdhc_unprepare(host);
> >  err_clk_axi:
> >         clk_disable_unprepare(priv->axi_clk);
> > @@ -573,10 +560,6 @@ static int xenon_remove(struct platform_device *pdev)
> >         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >         struct xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> >
> > -       pm_runtime_get_sync(&pdev->dev);
> > -       pm_runtime_disable(&pdev->dev);
> > -       pm_runtime_put_noidle(&pdev->dev);
> > -
> >         sdhci_remove_host(host, 0);
> >
> >         xenon_sdhc_unprepare(host);
> > @@ -593,78 +576,40 @@ static int xenon_suspend(struct device *dev)
> >  {
> >         struct sdhci_host *host = dev_get_drvdata(dev);
> >         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > -       struct xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> >         int ret;
> >
> > -       ret = pm_runtime_force_suspend(dev);
> > +       ret = sdhci_suspend_host(host);
> > +       if (ret)
> > +               return ret;
> >
> > -       priv->restore_needed = true;
> > +       clk_disable_unprepare(pltfm_host->clk);
> >         return ret;
> >  }
> > -#endif
> >
> > -#ifdef CONFIG_PM
> > -static int xenon_runtime_suspend(struct device *dev)
> > +static int xenon_resume(struct device *dev)
> >  {
> >         struct sdhci_host *host = dev_get_drvdata(dev);
> >         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > -       struct xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> >         int ret;
> >
> > -       ret = sdhci_runtime_suspend_host(host);
> > +       ret = clk_prepare_enable(pltfm_host->clk);
> >         if (ret)
> >                 return ret;
> >
> > -       if (host->tuning_mode != SDHCI_TUNING_MODE_3)
> > -               mmc_retune_needed(host->mmc);
> > -
> > -       clk_disable_unprepare(pltfm_host->clk);
> >         /*
> > -        * Need to update the priv->clock here, or when runtime resume
> > -        * back, phy don't aware the clock change and won't adjust phy
> > -        * which will cause cmd err
> > +        * If SoCs power off the entire Xenon, registers setting will
> > +        * be lost.
> > +        * Re-configure Xenon specific register to enable current SDHC
> >          */
> > -       priv->clock = 0;
> > -       return 0;
> > -}
> > -
> > -static int xenon_runtime_resume(struct device *dev)
> > -{
> > -       struct sdhci_host *host = dev_get_drvdata(dev);
> > -       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > -       struct xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> > -       int ret;
> > -
> > -       ret = clk_prepare_enable(pltfm_host->clk);
> > -       if (ret) {
> > -               dev_err(dev, "can't enable mainck\n");
> > +       ret = xenon_sdhc_prepare(host);
> > +       if (ret)
> >                 return ret;
> > -       }
> > -
> > -       if (priv->restore_needed) {
> > -               ret = xenon_sdhc_prepare(host);
> > -               if (ret)
> > -                       goto out;
> > -               priv->restore_needed = false;
> > -       }
> >
> > -       ret = sdhci_runtime_resume_host(host, 0);
> > -       if (ret)
> > -               goto out;
> > -       return 0;
> > -out:
> > -       clk_disable_unprepare(pltfm_host->clk);
> > -       return ret;
> > +       return sdhci_resume_host(host);
> >  }
> > -#endif /* CONFIG_PM */
> > -
> > -static const struct dev_pm_ops sdhci_xenon_dev_pm_ops = {
> > -       SET_SYSTEM_SLEEP_PM_OPS(xenon_suspend,
> > -                               pm_runtime_force_resume)
> > -       SET_RUNTIME_PM_OPS(xenon_runtime_suspend,
> > -                          xenon_runtime_resume,
> > -                          NULL)
> > -};
> > +#endif
> > +
> > +static SIMPLE_DEV_PM_OPS(xenon_pmops, xenon_suspend, xenon_resume);
> >
> >  static const struct of_device_id sdhci_xenon_dt_ids[] = {
> >         { .compatible = "marvell,armada-ap806-sdhci",},
> > @@ -678,7 +623,7 @@ static struct platform_driver sdhci_xenon_driver = {
> >         .driver = {
> >                 .name   = "xenon-sdhci",
> >                 .of_match_table = sdhci_xenon_dt_ids,
> > -               .pm = &sdhci_xenon_dev_pm_ops,
> > +               .pm = &xenon_pmops,
> >         },
> >         .probe  = xenon_probe,
> >         .remove = xenon_remove,
> > diff --git a/drivers/mmc/host/sdhci-xenon.h b/drivers/mmc/host/sdhci-xenon.h
> > index 593b82d7b68a..2b9b96e51261 100644
> > --- a/drivers/mmc/host/sdhci-xenon.h
> > +++ b/drivers/mmc/host/sdhci-xenon.h
> > @@ -89,7 +89,6 @@ struct xenon_priv {
> >          */
> >         void            *phy_params;
> >         struct xenon_emmc_phy_regs *emmc_phy_regs;
> > -       bool restore_needed;
> >  };
> >
> >  int xenon_phy_adj(struct sdhci_host *host, struct mmc_ios *ios);
> > --
> > 2.26.2
> >