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