* [PATCH RFC] mmc: sdhci_am654: Add support for PM suspend/resume
@ 2022-03-30 7:52 Aswath Govindraju
2022-04-07 10:42 ` Ulf Hansson
0 siblings, 1 reply; 4+ messages in thread
From: Aswath Govindraju @ 2022-03-30 7:52 UTC (permalink / raw)
Cc: Kishon Vijay Abraham I, Vignesh Raghavendra, Aswath Govindraju,
Adrian Hunter, Ulf Hansson, linux-mmc, linux-kernel
Add support for suspend/resume and pm_runtime resume/suspend.
Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
---
drivers/mmc/host/sdhci_am654.c | 204 ++++++++++++++++++++++++++++++---
1 file changed, 191 insertions(+), 13 deletions(-)
diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index e54fe24d47e7..e86df72dfd78 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,30 +803,42 @@ 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_set_active(dev);
+ pm_runtime_enable(dev);
+ clk_prepare_enable(pltfm_host->clk);
+ ret = pm_runtime_resume_and_get(dev);
+ if (ret)
+ goto clk_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:
+clk_disable:
+ clk_disable_unprepare(pltfm_host->clk);
pm_runtime_put_sync(dev);
-pm_runtime_disable:
pm_runtime_disable(dev);
err_pltfm_free:
sdhci_pltfm_free(pdev);
@@ -841,6 +848,7 @@ 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);
@@ -848,16 +856,186 @@ static int sdhci_am654_remove(struct platform_device *pdev)
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 */
+ clk_prepare_enable(pltfm_host->clk);
+ 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
+
+#ifdef CONFIG_PM_SLEEP
+static int sdhci_am654_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)
+ return 0;
+
+ ret = pm_runtime_get_sync(dev);
+ if (ret < 0)
+ return 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_suspend_host(host);
+ if (ret)
+ return ret;
+
+ ret = pm_runtime_put_sync(dev);
+
+ /* disable the clock */
+ clk_disable_unprepare(pltfm_host->clk);
+
+ return 0;
+}
+
+static int sdhci_am654_resume(struct device *dev)
+{
+ struct sdhci_host *host = dev_get_drvdata(dev);
+ struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+ int ret;
+
+ if (!host)
+ return 0;
+
+ /* Enable the clock */
+ clk_prepare_enable(pltfm_host->clk);
+ ret = pm_runtime_get_sync(dev);
+ if (ret < 0)
+ goto clk_unprepare;
+
+ ret = sdhci_am654_restore(host);
+ if (ret)
+ goto put;
+
+ ret = sdhci_resume_host(host);
+ if (ret)
+ goto put;
+
+ ret = cqhci_resume(host->mmc);
+ if (ret)
+ goto put;
+
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+ return 0;
+
+put:
+ pm_runtime_put_sync(dev);
+clk_unprepare:
+ clk_disable_unprepare(pltfm_host->clk);
+ return ret;
+}
+#endif
+
+static const struct dev_pm_ops sdhci_am654_dev_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(sdhci_am654_suspend, sdhci_am654_resume)
+ .runtime_suspend = sdhci_am654_runtime_suspend,
+ .runtime_resume = sdhci_am654_runtime_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.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH RFC] mmc: sdhci_am654: Add support for PM suspend/resume
2022-03-30 7:52 [PATCH RFC] mmc: sdhci_am654: Add support for PM suspend/resume Aswath Govindraju
@ 2022-04-07 10:42 ` Ulf Hansson
2022-04-07 12:24 ` Aswath Govindraju
0 siblings, 1 reply; 4+ messages in thread
From: Ulf Hansson @ 2022-04-07 10:42 UTC (permalink / raw)
To: Aswath Govindraju
Cc: Kishon Vijay Abraham I, Vignesh Raghavendra, Adrian Hunter,
linux-mmc, linux-kernel, Faiz Abbas
+ Faiz
On Wed, 30 Mar 2022 at 09:53, 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>
> ---
> drivers/mmc/host/sdhci_am654.c | 204 ++++++++++++++++++++++++++++++---
> 1 file changed, 191 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index e54fe24d47e7..e86df72dfd78 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,30 +803,42 @@ 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_set_active(dev);
> + pm_runtime_enable(dev);
> + clk_prepare_enable(pltfm_host->clk);
I think some error handling is missing, at least for clk_prepare_enable().
> + ret = pm_runtime_resume_and_get(dev);
This can be replaced with a pm_runtime_get_noresume() - and I think it
would improve the readability of the code, to put the call above
pm_runtime_set_active().
> + if (ret)
> + goto clk_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:
> +clk_disable:
> + clk_disable_unprepare(pltfm_host->clk);
> pm_runtime_put_sync(dev);
> -pm_runtime_disable:
> pm_runtime_disable(dev);
> err_pltfm_free:
> sdhci_pltfm_free(pdev);
> @@ -841,6 +848,7 @@ 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);
> @@ -848,16 +856,186 @@ static int sdhci_am654_remove(struct platform_device *pdev)
> if (ret < 0)
> return ret;
>
> + clk_disable_unprepare(pltfm_host->clk);
To gate the clock, you need to make sure it has been ungated first. As
you anyway need to add a call pm_runtime_get_sync() prior to calling
sdhci_remove_host() a few lines above, this would fix it.
Moreover, the existing call to pm_runtime_put_sync() a few lines above
in sdhci_am654_remove(), should be replaced with a call to
pm_runtime_put_noidle() - and that call should be made below the call
pm_runtime_disable() to become correct.
> 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 */
> + clk_prepare_enable(pltfm_host->clk);
> + 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
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int sdhci_am654_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)
> + return 0;
> +
> + ret = pm_runtime_get_sync(dev);
> + if (ret < 0)
> + return 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_suspend_host(host);
> + if (ret)
> + return ret;
> +
> + ret = pm_runtime_put_sync(dev);
> +
> + /* disable the clock */
> + clk_disable_unprepare(pltfm_host->clk);
> +
> + return 0;
> +}
> +
> +static int sdhci_am654_resume(struct device *dev)
> +{
> + struct sdhci_host *host = dev_get_drvdata(dev);
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + int ret;
> +
> + if (!host)
> + return 0;
> +
> + /* Enable the clock */
> + clk_prepare_enable(pltfm_host->clk);
> + ret = pm_runtime_get_sync(dev);
> + if (ret < 0)
> + goto clk_unprepare;
> +
> + ret = sdhci_am654_restore(host);
> + if (ret)
> + goto put;
> +
> + ret = sdhci_resume_host(host);
> + if (ret)
> + goto put;
> +
> + ret = cqhci_resume(host->mmc);
> + if (ret)
> + goto put;
> +
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
> + return 0;
> +
> +put:
> + pm_runtime_put_sync(dev);
> +clk_unprepare:
> + clk_disable_unprepare(pltfm_host->clk);
> + return ret;
> +}
By looking at the code in sdhci_am654_suspend|resume(), I wonder if
those is better replaced with assigning system PM callbacks to
pm_runtime_force_suspend|resume() instead. Other sdhci variants do
this already, you may have a look at those to better get the idea.
> +#endif
> +
> +static const struct dev_pm_ops sdhci_am654_dev_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(sdhci_am654_suspend, sdhci_am654_resume)
> + .runtime_suspend = sdhci_am654_runtime_suspend,
> + .runtime_resume = sdhci_am654_runtime_resume,
For runtime PM callbacks, you can use SET_RUNTIME_PM_OPS(). Otherwise
I think this will cause a build error when built with CONFIG_PM unset.
> +};
> +
> 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,
Kind regards
Uffe
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH RFC] mmc: sdhci_am654: Add support for PM suspend/resume
2022-04-07 10:42 ` Ulf Hansson
@ 2022-04-07 12:24 ` Aswath Govindraju
2022-04-07 15:09 ` Ulf Hansson
0 siblings, 1 reply; 4+ messages in thread
From: Aswath Govindraju @ 2022-04-07 12:24 UTC (permalink / raw)
To: Ulf Hansson
Cc: Kishon Vijay Abraham I, Vignesh Raghavendra, Adrian Hunter,
linux-mmc, linux-kernel, Faiz Abbas
Hi Uffe,
On 07/04/22 16:12, Ulf Hansson wrote:
> + Faiz
>
> On Wed, 30 Mar 2022 at 09:53, 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>
>> ---
>> drivers/mmc/host/sdhci_am654.c | 204 ++++++++++++++++++++++++++++++---
>> 1 file changed, 191 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>> index e54fe24d47e7..e86df72dfd78 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,30 +803,42 @@ 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_set_active(dev);
>> + pm_runtime_enable(dev);
>> + clk_prepare_enable(pltfm_host->clk);
>
> I think some error handling is missing, at least for clk_prepare_enable().
>
>> + ret = pm_runtime_resume_and_get(dev);
>
> This can be replaced with a pm_runtime_get_noresume() - and I think it
> would improve the readability of the code, to put the call above
> pm_runtime_set_active().
>
Shouldn't pm_runtime_get_* be only done after we execute
pm_runtime_enable and pm_runtime_set_active should be called before
pm_runtime_enable()
"In addition to that, the initial runtime PM status of all devices is
‘suspended’, but it need not reflect the actual physical state of the
device. Thus, if the device is initially active (i.e. it is able to
process I/O), its runtime PM status must be changed to ‘active’, with
the help of pm_runtime_set_active(), before pm_runtime_enable() is
called for the device." [1]
Yeah, and I agree that pm_runtime_get_noresume would be better to use
over here.
[1] - https://www.infradead.org/~mchehab/kernel_docs/power/runtime_pm.html
>> + if (ret)
>> + goto clk_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:
>> +clk_disable:
>> + clk_disable_unprepare(pltfm_host->clk);
>> pm_runtime_put_sync(dev);
>> -pm_runtime_disable:
>> pm_runtime_disable(dev);
>> err_pltfm_free:
>> sdhci_pltfm_free(pdev);
>> @@ -841,6 +848,7 @@ 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);
>> @@ -848,16 +856,186 @@ static int sdhci_am654_remove(struct platform_device *pdev)
>> if (ret < 0)
>> return ret;
>>
>> + clk_disable_unprepare(pltfm_host->clk);
>
> To gate the clock, you need to make sure it has been ungated first. As
> you anyway need to add a call pm_runtime_get_sync() prior to calling
> sdhci_remove_host() a few lines above, this would fix it.
>
This call was the counter part for the clk_enable_prepare called in
probe(). Yes, and I should have done a pm_runtime_get_sync before
calling sdhci_remove_host() in sdhci_am654_remove()
> Moreover, the existing call to pm_runtime_put_sync() a few lines above
> in sdhci_am654_remove(), should be replaced with a call to
> pm_runtime_put_noidle() - and that call should be made below the call
> pm_runtime_disable() to become correct.
Again shouldn't we disable pm_runtime after putting the device?
>
>> 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 */
>> + clk_prepare_enable(pltfm_host->clk);
>> + 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
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int sdhci_am654_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)
>> + return 0;
>> +
>> + ret = pm_runtime_get_sync(dev);
>> + if (ret < 0)
>> + return 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_suspend_host(host);
>> + if (ret)
>> + return ret;
>> +
>> + ret = pm_runtime_put_sync(dev);
>> +
>> + /* disable the clock */
>> + clk_disable_unprepare(pltfm_host->clk);
>> +
>> + return 0;
>> +}
>> +
>> +static int sdhci_am654_resume(struct device *dev)
>> +{
>> + struct sdhci_host *host = dev_get_drvdata(dev);
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + int ret;
>> +
>> + if (!host)
>> + return 0;
>> +
>> + /* Enable the clock */
>> + clk_prepare_enable(pltfm_host->clk);
>> + ret = pm_runtime_get_sync(dev);
>> + if (ret < 0)
>> + goto clk_unprepare;
>> +
>> + ret = sdhci_am654_restore(host);
>> + if (ret)
>> + goto put;
>> +
>> + ret = sdhci_resume_host(host);
>> + if (ret)
>> + goto put;
>> +
>> + ret = cqhci_resume(host->mmc);
>> + if (ret)
>> + goto put;
>> +
>> + pm_runtime_mark_last_busy(dev);
>> + pm_runtime_put_autosuspend(dev);
>> + return 0;
>> +
>> +put:
>> + pm_runtime_put_sync(dev);
>> +clk_unprepare:
>> + clk_disable_unprepare(pltfm_host->clk);
>> + return ret;
>> +}
>
> By looking at the code in sdhci_am654_suspend|resume(), I wonder if
> those is better replaced with assigning system PM callbacks to
> pm_runtime_force_suspend|resume() instead. Other sdhci variants do
> this already, you may have a look at those to better get the idea.
>
The reason why I used different functions was because of the
sdhci_runtime_suspend/resume vs sdhci_suspend/resume but I think from
the controller perspective there is no difference. So, yeah I think it
is better to go with pm_runtime_force_suspend/resume().
>> +#endif
>> +
>> +static const struct dev_pm_ops sdhci_am654_dev_pm_ops = {
>> + SET_SYSTEM_SLEEP_PM_OPS(sdhci_am654_suspend, sdhci_am654_resume)
>> + .runtime_suspend = sdhci_am654_runtime_suspend,
>> + .runtime_resume = sdhci_am654_runtime_resume,
>
> For runtime PM callbacks, you can use SET_RUNTIME_PM_OPS(). Otherwise
> I think this will cause a build error when built with CONFIG_PM unset.
>
Sure, will replace this in the respin.
>> +};
>> +
>> 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,
>
> Kind regards
> Uffe
--
Thanks,
Aswath
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH RFC] mmc: sdhci_am654: Add support for PM suspend/resume
2022-04-07 12:24 ` Aswath Govindraju
@ 2022-04-07 15:09 ` Ulf Hansson
0 siblings, 0 replies; 4+ messages in thread
From: Ulf Hansson @ 2022-04-07 15:09 UTC (permalink / raw)
To: Aswath Govindraju
Cc: Kishon Vijay Abraham I, Vignesh Raghavendra, Adrian Hunter,
linux-mmc, linux-kernel, Faiz Abbas
On Thu, 7 Apr 2022 at 14:24, Aswath Govindraju <a-govindraju@ti.com> wrote:
>
> Hi Uffe,
>
> On 07/04/22 16:12, Ulf Hansson wrote:
> > + Faiz
> >
> > On Wed, 30 Mar 2022 at 09:53, 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>
> >> ---
> >> drivers/mmc/host/sdhci_am654.c | 204 ++++++++++++++++++++++++++++++---
> >> 1 file changed, 191 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> >> index e54fe24d47e7..e86df72dfd78 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,30 +803,42 @@ 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_set_active(dev);
> >> + pm_runtime_enable(dev);
> >> + clk_prepare_enable(pltfm_host->clk);
> >
> > I think some error handling is missing, at least for clk_prepare_enable().
> >
> >> + ret = pm_runtime_resume_and_get(dev);
> >
> > This can be replaced with a pm_runtime_get_noresume() - and I think it
> > would improve the readability of the code, to put the call above
> > pm_runtime_set_active().
> >
>
> Shouldn't pm_runtime_get_* be only done after we execute
> pm_runtime_enable and pm_runtime_set_active should be called before
> pm_runtime_enable()
pm_runtime_get_noresume() is somewhat special in this regard. It only
bumps the usage count, which is to prevent any following attempts from
runtime suspending the device.
It's perfectly okay to call it, both before and after runtime PM has
been enabled.
>
> "In addition to that, the initial runtime PM status of all devices is
> ‘suspended’, but it need not reflect the actual physical state of the
> device. Thus, if the device is initially active (i.e. it is able to
> process I/O), its runtime PM status must be changed to ‘active’, with
> the help of pm_runtime_set_active(), before pm_runtime_enable() is
> called for the device." [1]
>
>
> Yeah, and I agree that pm_runtime_get_noresume would be better to use
> over here.
Great!
>
> [1] - https://www.infradead.org/~mchehab/kernel_docs/power/runtime_pm.html
>
>
> >> + if (ret)
> >> + goto clk_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:
> >> +clk_disable:
> >> + clk_disable_unprepare(pltfm_host->clk);
> >> pm_runtime_put_sync(dev);
> >> -pm_runtime_disable:
> >> pm_runtime_disable(dev);
> >> err_pltfm_free:
> >> sdhci_pltfm_free(pdev);
> >> @@ -841,6 +848,7 @@ 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);
> >> @@ -848,16 +856,186 @@ static int sdhci_am654_remove(struct platform_device *pdev)
> >> if (ret < 0)
> >> return ret;
> >>
> >> + clk_disable_unprepare(pltfm_host->clk);
> >
> > To gate the clock, you need to make sure it has been ungated first. As
> > you anyway need to add a call pm_runtime_get_sync() prior to calling
> > sdhci_remove_host() a few lines above, this would fix it.
> >
>
> This call was the counter part for the clk_enable_prepare called in
> probe(). Yes, and I should have done a pm_runtime_get_sync before
> calling sdhci_remove_host() in sdhci_am654_remove()
>
> > Moreover, the existing call to pm_runtime_put_sync() a few lines above
> > in sdhci_am654_remove(), should be replaced with a call to
> > pm_runtime_put_noidle() - and that call should be made below the call
> > pm_runtime_disable() to become correct.
>
> Again shouldn't we disable pm_runtime after putting the device?
pm_runtime_put_noidle() is special in this regard, it only decreases
the usage count and doesn't try to runtime suspend the device.
It's perfectly okay to call it, both before and after runtime PM has
been enabled.
>
> >
> >> pm_runtime_disable(&pdev->dev);
> >> sdhci_pltfm_free(pdev);
> >> + return 0;
> >> +}
[...]
Kind regards
Uffe
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-04-07 15:11 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-30 7:52 [PATCH RFC] mmc: sdhci_am654: Add support for PM suspend/resume Aswath Govindraju
2022-04-07 10:42 ` Ulf Hansson
2022-04-07 12:24 ` Aswath Govindraju
2022-04-07 15:09 ` Ulf Hansson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.