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

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

[Jisheng Zhang]

This reverts commit a027b2c5fed78851e69fab395b02d127a7759fc7.

The HW supports auto clock gating, so it's useless to do runtime pm
in software.

Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
---
 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

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

[Ulf Hansson]

On Wed, 13 May 2020 at 11:47, Jisheng Zhang <Jisheng.Zhang@synaptics.com> 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
"pltfm_host->clk", which means even if the controller supports auto
clock gating, gating/ungating the externally provided clock still
makes sense.

Kind regards
Uffe

>
> Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> ---
>  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
>

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@synaptics.com> 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@synaptics.com>
> > ---
> >  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
> >  

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

[Ulf Hansson]

On Thu, 14 May 2020 at 07:45, Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:
>
> On Wed, 13 May 2020 14:15:21 +0200 Ulf Hansson wrote:
>
> >
> >
> > On Wed, 13 May 2020 at 11:47, Jisheng Zhang <Jisheng.Zhang@synaptics.com> 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.

Right.

The re-tune may be costly, yes. However, whether the re-tune is
*really* needed actually varies depending on the sdhci variant and the
SoC. Additionally, re-tune isn't done for all types of (e)MMC/SD/SDIO
cards.

I see a few options that you can explore.

1. There is no requirement to call sdhci_runtime_suspend|resume_host()
from sdhci-xenon's ->runtime_suspend|resume() callbacks - if that's
not explicitly needed. The point is, you can do other things there,
that suits your variant/SoC better.

2. Perhaps for embedded eMMCs, with a non-removable slot, the
re-tuning is costly. If you want to prevent the device from entering
runtime suspend for that slot, for example, just do an additional
pm_runtime_get_noresume() during ->probe().

[...]

Kind regards
Uffe

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

[Jisheng Zhang]

On Thu, 14 May 2020 12:18:58 +0200 Ulf Hansson wrote:

> 
> 
> On Thu, 14 May 2020 at 07:45, Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:
> >
> > On Wed, 13 May 2020 14:15:21 +0200 Ulf Hansson wrote:
> >  
> > >
> > >
> > > On Wed, 13 May 2020 at 11:47, Jisheng Zhang <Jisheng.Zhang@synaptics.com> 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.

what's your option about this? My point is the HW can auto clock
gate, so what's the benefit of current rpm implementation given it only does
clock gating. FWICT, when submitting the xenon rpm patch, I don't think the
author  compared the power consumption. If the comparison is done, it's easy
to find the rpm doesn't bring any power consumption benefit at all.

> >  
> > > "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.  
> 
> Right.
> 
> The re-tune may be costly, yes. However, whether the re-tune is
> *really* needed actually varies depending on the sdhci variant and the
> SoC. Additionally, re-tune isn't done for all types of (e)MMC/SD/SDIO
> cards.
> 
> I see a few options that you can explore.
> 
> 1. There is no requirement to call sdhci_runtime_suspend|resume_host()
> from sdhci-xenon's ->runtime_suspend|resume() callbacks - if that's
> not explicitly needed. The point is, you can do other things there,
> that suits your variant/SoC better.

Yes, there's no requirement to call sdhci_runtime_suspend|resume_host().
But simply removing the calls would break system suspend. How to handle
this situation?

> 
> 2. Perhaps for embedded eMMCs, with a non-removable slot, the
> re-tuning is costly. If you want to prevent the device from entering
> runtime suspend for that slot, for example, just do an additional
> pm_runtime_get_noresume() during ->probe().
> 
> [...]
> 
> Kind regards
> Uffe

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

[Ulf Hansson]

On Fri, 15 May 2020 at 08:00, Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:
>
> On Thu, 14 May 2020 12:18:58 +0200 Ulf Hansson wrote:
>
> >
> >
> > On Thu, 14 May 2020 at 07:45, Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:
> > >
> > > On Wed, 13 May 2020 14:15:21 +0200 Ulf Hansson wrote:
> > >
> > > >
> > > >
> > > > On Wed, 13 May 2020 at 11:47, Jisheng Zhang <Jisheng.Zhang@synaptics.com> 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.
>
> what's your option about this? My point is the HW can auto clock
> gate, so what's the benefit of current rpm implementation given it only does
> clock gating. FWICT, when submitting the xenon rpm patch, I don't think the
> author  compared the power consumption. If the comparison is done, it's easy
> to find the rpm doesn't bring any power consumption benefit at all.

As I stated, runtime PM isn't solely about clock gating. It depends on the SoC.

For example, if this sdhci device is being powered by a shared voltage
rail through a PM domain, perhaps it's even critical from an energy
efficiency point to manage runtime PM correctly.

By looking at the original commit, that sounds exactly what is
happening as the registers seem to lose their context.

>
> > >
> > > > "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.
> >
> > Right.
> >
> > The re-tune may be costly, yes. However, whether the re-tune is
> > *really* needed actually varies depending on the sdhci variant and the
> > SoC. Additionally, re-tune isn't done for all types of (e)MMC/SD/SDIO
> > cards.
> >
> > I see a few options that you can explore.
> >
> > 1. There is no requirement to call sdhci_runtime_suspend|resume_host()
> > from sdhci-xenon's ->runtime_suspend|resume() callbacks - if that's
> > not explicitly needed. The point is, you can do other things there,
> > that suits your variant/SoC better.
>
> Yes, there's no requirement to call sdhci_runtime_suspend|resume_host().
> But simply removing the calls would break system suspend. How to handle
> this situation?

Alright. Then perhaps you need parts of what
sdhci_runtime_suspend|resume() is doing?

Anyway, as I said below. If you really have some good reasons from an
energy efficiency point of view, you can always disable runtime
suspend, along the lines of below.

To put it clear, to me the reasons you are providing to justify the
revert just doesn't make sense. Please try a different option.

>
> >
> > 2. Perhaps for embedded eMMCs, with a non-removable slot, the
> > re-tuning is costly. If you want to prevent the device from entering
> > runtime suspend for that slot, for example, just do an additional
> > pm_runtime_get_noresume() during ->probe().
> >
> > [...]
> >

Kind regards
Uffe