diff mbox series

[RFC,v2] mmc: sdhci_am654: Add support for PM suspend/resume

Message ID 20220408124338.27090-1-a-govindraju@ti.com (mailing list archive)
State New, archived
Headers show
Series [RFC,v2] mmc: sdhci_am654: Add support for PM suspend/resume | expand

Commit Message

Aswath Govindraju April 8, 2022, 12:43 p.m. UTC
Add support for suspend/resume and pm_runtime resume/suspend.

Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
---

Changes since v1:
- Removed System pm calls and used force_runtime_pm calls instead
- Added error handling at required places
- Added pm_runtime_sync call in the sdhci_am654_remove function
- Replaced pm_runtime_*_sync calls in probe with noidle and noresume
  calls
- Used MACRO SET_RUNTIME_PM_OPS to avoid build errors when CONFIG_PM
  is disabled

 drivers/mmc/host/sdhci_am654.c | 146 +++++++++++++++++++++++++++++----
 1 file changed, 132 insertions(+), 14 deletions(-)

Comments

Ulf Hansson April 12, 2022, 2:38 p.m. UTC | #1
On Fri, 8 Apr 2022 at 14:43, Aswath Govindraju <a-govindraju@ti.com> wrote:
>
> Add support for suspend/resume and pm_runtime resume/suspend.
>
> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> ---
>
> Changes since v1:
> - Removed System pm calls and used force_runtime_pm calls instead
> - Added error handling at required places
> - Added pm_runtime_sync call in the sdhci_am654_remove function
> - Replaced pm_runtime_*_sync calls in probe with noidle and noresume
>   calls
> - Used MACRO SET_RUNTIME_PM_OPS to avoid build errors when CONFIG_PM
>   is disabled
>
>  drivers/mmc/host/sdhci_am654.c | 146 +++++++++++++++++++++++++++++----
>  1 file changed, 132 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index e54fe24d47e7..131b2b9ae0e7 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -84,6 +84,7 @@
>  #define DRIVER_STRENGTH_40_OHM 0x4
>
>  #define CLOCK_TOO_SLOW_HZ      50000000
> +#define SDHCI_AM654_AUTOSUSPEND_DELAY  100
>
>  /* Command Queue Host Controller Interface Base address */
>  #define SDHCI_AM654_CQE_BASE_ADDR 0x200
> @@ -791,16 +792,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,
> @@ -808,31 +803,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(dev, "parsing dt failed (%d)\n", ret);
> -               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;
> @@ -841,23 +852,130 @@ 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;
>
> +       ret = pm_runtime_get_sync(&pdev->dev);

pm_runtime_resume_and_get() is better suited here, as it helps us to
restore the usage count in the error path.

> +       if (ret < 0)
> +               return ret;
> +
>         sdhci_remove_host(host, true);
>         ret = pm_runtime_put_sync(&pdev->dev);

We need to drop the above call to pm_runtime_put_sync(). Otherwise you
may potentially end up gating the clock, in which case the below call
to clk_disable_unprepare() becomes wrong.

>         if (ret < 0)
>                 return ret;
>
> +       clk_disable_unprepare(pltfm_host->clk);
>         pm_runtime_disable(&pdev->dev);

You need a call to pm_runtime_put_noidle() here to restore the RPM
usage count to zero.

>         sdhci_pltfm_free(pdev);
> +       return 0;
> +}

[...]

Other than the rather minor things above, this looks good to me!

Kind regards
Uffe
Aswath Govindraju April 13, 2022, 12:07 p.m. UTC | #2
Hi Uffe,

On 12/04/22 20:08, Ulf Hansson wrote:
> On Fri, 8 Apr 2022 at 14:43, Aswath Govindraju <a-govindraju@ti.com> wrote:
>>
>> Add support for suspend/resume and pm_runtime resume/suspend.
>>
>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>> ---
>>
>> Changes since v1:
>> - Removed System pm calls and used force_runtime_pm calls instead
>> - Added error handling at required places
>> - Added pm_runtime_sync call in the sdhci_am654_remove function
>> - Replaced pm_runtime_*_sync calls in probe with noidle and noresume
>>   calls
>> - Used MACRO SET_RUNTIME_PM_OPS to avoid build errors when CONFIG_PM
>>   is disabled
>>
>>  drivers/mmc/host/sdhci_am654.c | 146 +++++++++++++++++++++++++++++----
>>  1 file changed, 132 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>> index e54fe24d47e7..131b2b9ae0e7 100644
>> --- a/drivers/mmc/host/sdhci_am654.c
>> +++ b/drivers/mmc/host/sdhci_am654.c
>> @@ -84,6 +84,7 @@
>>  #define DRIVER_STRENGTH_40_OHM 0x4
>>
>>  #define CLOCK_TOO_SLOW_HZ      50000000
>> +#define SDHCI_AM654_AUTOSUSPEND_DELAY  100
>>
>>  /* Command Queue Host Controller Interface Base address */
>>  #define SDHCI_AM654_CQE_BASE_ADDR 0x200
>> @@ -791,16 +792,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,
>> @@ -808,31 +803,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(dev, "parsing dt failed (%d)\n", ret);
>> -               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;
>> @@ -841,23 +852,130 @@ 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;
>>
>> +       ret = pm_runtime_get_sync(&pdev->dev);
> 
> pm_runtime_resume_and_get() is better suited here, as it helps us to
> restore the usage count in the error path.
> >> +       if (ret < 0)
>> +               return ret;
>> +
>>         sdhci_remove_host(host, true);
>>         ret = pm_runtime_put_sync(&pdev->dev);
> 
> We need to drop the above call to pm_runtime_put_sync(). Otherwise you
> may potentially end up gating the clock, in which case the below call
> to clk_disable_unprepare() becomes wrong.
> >>         if (ret < 0)
>>                 return ret;
>>
>> +       clk_disable_unprepare(pltfm_host->clk);
>>         pm_runtime_disable(&pdev->dev);
> 
> You need a call to pm_runtime_put_noidle() here to restore the RPM
> usage count to zero.
> 
>>         sdhci_pltfm_free(pdev);
>> +       return 0;
>> +}
> 
> [...]
> 
> Other than the rather minor things above, this looks good to me!
> 

Thank you for the review.

Regards,
Aswath

> Kind regards
> Uffe
diff mbox series

Patch

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index e54fe24d47e7..131b2b9ae0e7 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -84,6 +84,7 @@ 
 #define DRIVER_STRENGTH_40_OHM	0x4
 
 #define CLOCK_TOO_SLOW_HZ	50000000
+#define SDHCI_AM654_AUTOSUSPEND_DELAY	100
 
 /* Command Queue Host Controller Interface Base address */
 #define SDHCI_AM654_CQE_BASE_ADDR 0x200
@@ -791,16 +792,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,
@@ -808,31 +803,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(dev, "parsing dt failed (%d)\n", ret);
-		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;
@@ -841,23 +852,130 @@  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;
 
+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret < 0)
+		return ret;
+
 	sdhci_remove_host(host, true);
 	ret = pm_runtime_put_sync(&pdev->dev);
 	if (ret < 0)
 		return ret;
 
+	clk_disable_unprepare(pltfm_host->clk);
 	pm_runtime_disable(&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,