[PATCH v3] mmc: sdhci_am654: Add support for PM suspend/resume

[PATCH v3] mmc: sdhci_am654: Add support for PM suspend/resume

[Vignesh Raghavendra]

From: Aswath Govindraju <a-govindraju@ti.com>

Add support for suspend/resume and pm_runtime resume/suspend.

Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
---

Since RFC v2:
Address all comments around sdhci_am654_remove()
Set autosuspend_delay to -1 as SDHCI will host rootfs via SD/eMMC and
autosuspend can cause long latency for user interactive applications

RFC v2:
https://lore.kernel.org/all/20220408124338.27090-1-a-govindraju@ti.com/

 drivers/mmc/host/sdhci_am654.c | 147 +++++++++++++++++++++++++++++----
 1 file changed, 131 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index 672d37ea98d0..7cdf0f54e3a5 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -85,6 +85,7 @@
 #define DRIVER_STRENGTH_40_OHM	0x4
 
 #define CLOCK_TOO_SLOW_HZ	50000000
+#define SDHCI_AM654_AUTOSUSPEND_DELAY	-1
 
 /* Command Queue Host Controller Interface Base address */
 #define SDHCI_AM654_CQE_BASE_ADDR 0x200
@@ -808,16 +809,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,
@@ -825,31 +820,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_probe(dev, ret, "parsing dt failed\n");
-		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;
@@ -858,23 +869,127 @@ 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);
-	ret = pm_runtime_put_sync(&pdev->dev);
+	ret = pm_runtime_resume_and_get(&pdev->dev);
 	if (ret < 0)
 		return ret;
 
+	sdhci_remove_host(host, true);
+	clk_disable_unprepare(pltfm_host->clk);
 	pm_runtime_disable(&pdev->dev);
+	pm_runtime_put_noidle(&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,
-- 
2.40.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3] mmc: sdhci_am654: Add support for PM suspend/resume

[Vignesh Raghavendra]

From: Aswath Govindraju <a-govindraju@ti.com>

Add support for suspend/resume and pm_runtime resume/suspend.

Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
---

Since RFC v2:
Address all comments around sdhci_am654_remove()
Set autosuspend_delay to -1 as SDHCI will host rootfs via SD/eMMC and
autosuspend can cause long latency for user interactive applications

RFC v2:
https://lore.kernel.org/all/20220408124338.27090-1-a-govindraju@ti.com/

 drivers/mmc/host/sdhci_am654.c | 147 +++++++++++++++++++++++++++++----
 1 file changed, 131 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index 672d37ea98d0..7cdf0f54e3a5 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -85,6 +85,7 @@
 #define DRIVER_STRENGTH_40_OHM	0x4
 
 #define CLOCK_TOO_SLOW_HZ	50000000
+#define SDHCI_AM654_AUTOSUSPEND_DELAY	-1
 
 /* Command Queue Host Controller Interface Base address */
 #define SDHCI_AM654_CQE_BASE_ADDR 0x200
@@ -808,16 +809,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,
@@ -825,31 +820,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_probe(dev, ret, "parsing dt failed\n");
-		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;
@@ -858,23 +869,127 @@ 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);
-	ret = pm_runtime_put_sync(&pdev->dev);
+	ret = pm_runtime_resume_and_get(&pdev->dev);
 	if (ret < 0)
 		return ret;
 
+	sdhci_remove_host(host, true);
+	clk_disable_unprepare(pltfm_host->clk);
 	pm_runtime_disable(&pdev->dev);
+	pm_runtime_put_noidle(&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,
-- 
2.40.0

[PATCH v3] mmc: sdhci_am654: Add support for PM suspend/resume

[Vignesh Raghavendra]

From: Aswath Govindraju <a-govindraju@ti.com>

Add support for suspend/resume and pm_runtime resume/suspend.

Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
---
Since RFC v2:
Address all comments around sdhci_am654_remove()
Set autosuspend_delay to -1 as SDHCI will host rootfs via SD/eMMC and
autosuspend can cause long latency for user interactive applications

RFC v2:
https://lore.kernel.org/all/20220408124338.27090-1-a-govindraju@ti.com/

 drivers/mmc/host/sdhci_am654.c | 147 +++++++++++++++++++++++++++++----
 1 file changed, 131 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index 672d37ea98d0..7cdf0f54e3a5 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -85,6 +85,7 @@
 #define DRIVER_STRENGTH_40_OHM	0x4
 
 #define CLOCK_TOO_SLOW_HZ	50000000
+#define SDHCI_AM654_AUTOSUSPEND_DELAY	-1
 
 /* Command Queue Host Controller Interface Base address */
 #define SDHCI_AM654_CQE_BASE_ADDR 0x200
@@ -808,16 +809,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,
@@ -825,31 +820,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_probe(dev, ret, "parsing dt failed\n");
-		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;
@@ -858,23 +869,127 @@ 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);
-	ret = pm_runtime_put_sync(&pdev->dev);
+	ret = pm_runtime_resume_and_get(&pdev->dev);
 	if (ret < 0)
 		return ret;
 
+	sdhci_remove_host(host, true);
+	clk_disable_unprepare(pltfm_host->clk);
 	pm_runtime_disable(&pdev->dev);
+	pm_runtime_put_noidle(&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,
-- 
2.40.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3] mmc: sdhci_am654: Add support for PM suspend/resume

[Vignesh Raghavendra]

From: Aswath Govindraju <a-govindraju@ti.com>

Add support for suspend/resume and pm_runtime resume/suspend.

Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
---
Since RFC v2:
Address all comments around sdhci_am654_remove()
Set autosuspend_delay to -1 as SDHCI will host rootfs via SD/eMMC and
autosuspend can cause long latency for user interactive applications

RFC v2:
https://lore.kernel.org/all/20220408124338.27090-1-a-govindraju@ti.com/

 drivers/mmc/host/sdhci_am654.c | 147 +++++++++++++++++++++++++++++----
 1 file changed, 131 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index 672d37ea98d0..7cdf0f54e3a5 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -85,6 +85,7 @@
 #define DRIVER_STRENGTH_40_OHM	0x4
 
 #define CLOCK_TOO_SLOW_HZ	50000000
+#define SDHCI_AM654_AUTOSUSPEND_DELAY	-1
 
 /* Command Queue Host Controller Interface Base address */
 #define SDHCI_AM654_CQE_BASE_ADDR 0x200
@@ -808,16 +809,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,
@@ -825,31 +820,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_probe(dev, ret, "parsing dt failed\n");
-		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;
@@ -858,23 +869,127 @@ 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);
-	ret = pm_runtime_put_sync(&pdev->dev);
+	ret = pm_runtime_resume_and_get(&pdev->dev);
 	if (ret < 0)
 		return ret;
 
+	sdhci_remove_host(host, true);
+	clk_disable_unprepare(pltfm_host->clk);
 	pm_runtime_disable(&pdev->dev);
+	pm_runtime_put_noidle(&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,
-- 
2.40.0

Re: [PATCH v3] mmc: sdhci_am654: Add support for PM suspend/resume

[Vignesh Raghavendra]

On 31/03/23 15:46, Vignesh Raghavendra wrote:
> From: Aswath Govindraju <a-govindraju@ti.com>
> 
> Add support for suspend/resume and pm_runtime resume/suspend.
> 
> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> ---
> Since RFC v2:
> Address all comments around sdhci_am654_remove()
> Set autosuspend_delay to -1 as SDHCI will host rootfs via SD/eMMC and
> autosuspend can cause long latency for user interactive applications
> 
> RFC v2:
> https://lore.kernel.org/all/20220408124338.27090-1-a-govindraju@ti.com/

Oops, this is a stale patch. Please ignore this and consider [0] instead
Sorry for the noise

[0] lore.kernel.org/r/20230331101619.4117312-1-vigneshr@ti.com

-- 
Regards
Vignesh

Re: [PATCH v3] mmc: sdhci_am654: Add support for PM suspend/resume

[Vignesh Raghavendra]

On 31/03/23 15:46, Vignesh Raghavendra wrote:
> From: Aswath Govindraju <a-govindraju@ti.com>
> 
> Add support for suspend/resume and pm_runtime resume/suspend.
> 
> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> ---
> Since RFC v2:
> Address all comments around sdhci_am654_remove()
> Set autosuspend_delay to -1 as SDHCI will host rootfs via SD/eMMC and
> autosuspend can cause long latency for user interactive applications
> 
> RFC v2:
> https://lore.kernel.org/all/20220408124338.27090-1-a-govindraju@ti.com/

Oops, this is a stale patch. Please ignore this and consider [0] instead
Sorry for the noise

[0] lore.kernel.org/r/20230331101619.4117312-1-vigneshr@ti.com

-- 
Regards
Vignesh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] mmc: sdhci_am654: Add support for PM suspend/resume

[Ulf Hansson]

On Fri, 31 Mar 2023 at 12:17, Vignesh Raghavendra <vigneshr@ti.com> wrote:
>
> From: Aswath Govindraju <a-govindraju@ti.com>
>
> Add support for suspend/resume and pm_runtime resume/suspend.
>
> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> ---
>
> Since RFC v2:
> Address all comments around sdhci_am654_remove()
> Set autosuspend_delay to -1 as SDHCI will host rootfs via SD/eMMC and
> autosuspend can cause long latency for user interactive applications

I am curious about the "long" latency. Do you have some data that you
can share for how long it takes to runtime-resume the device?

Using -1 as the default value for the autosuspend_delay seems fine to
me, but did you consider using a longer timeout than the common 100ms?
Could that perhaps be a better default option?

Of course, in the end this is a choice of performance in favor of
wasting energy.

Kind regards
Uffe

>
> RFC v2:
> https://lore.kernel.org/all/20220408124338.27090-1-a-govindraju@ti.com/
>
>  drivers/mmc/host/sdhci_am654.c | 147 +++++++++++++++++++++++++++++----
>  1 file changed, 131 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index 672d37ea98d0..7cdf0f54e3a5 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -85,6 +85,7 @@
>  #define DRIVER_STRENGTH_40_OHM 0x4
>
>  #define CLOCK_TOO_SLOW_HZ      50000000
> +#define SDHCI_AM654_AUTOSUSPEND_DELAY  -1
>
>  /* Command Queue Host Controller Interface Base address */
>  #define SDHCI_AM654_CQE_BASE_ADDR 0x200
> @@ -808,16 +809,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,
> @@ -825,31 +820,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_probe(dev, ret, "parsing dt failed\n");
> -               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;
> @@ -858,23 +869,127 @@ 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);
> -       ret = pm_runtime_put_sync(&pdev->dev);
> +       ret = pm_runtime_resume_and_get(&pdev->dev);
>         if (ret < 0)
>                 return ret;
>
> +       sdhci_remove_host(host, true);
> +       clk_disable_unprepare(pltfm_host->clk);
>         pm_runtime_disable(&pdev->dev);
> +       pm_runtime_put_noidle(&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,
> --
> 2.40.0
>

Re: [PATCH v3] mmc: sdhci_am654: Add support for PM suspend/resume

[Ulf Hansson]

On Fri, 31 Mar 2023 at 12:17, Vignesh Raghavendra <vigneshr@ti.com> wrote:
>
> From: Aswath Govindraju <a-govindraju@ti.com>
>
> Add support for suspend/resume and pm_runtime resume/suspend.
>
> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> ---
>
> Since RFC v2:
> Address all comments around sdhci_am654_remove()
> Set autosuspend_delay to -1 as SDHCI will host rootfs via SD/eMMC and
> autosuspend can cause long latency for user interactive applications

I am curious about the "long" latency. Do you have some data that you
can share for how long it takes to runtime-resume the device?

Using -1 as the default value for the autosuspend_delay seems fine to
me, but did you consider using a longer timeout than the common 100ms?
Could that perhaps be a better default option?

Of course, in the end this is a choice of performance in favor of
wasting energy.

Kind regards
Uffe

>
> RFC v2:
> https://lore.kernel.org/all/20220408124338.27090-1-a-govindraju@ti.com/
>
>  drivers/mmc/host/sdhci_am654.c | 147 +++++++++++++++++++++++++++++----
>  1 file changed, 131 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index 672d37ea98d0..7cdf0f54e3a5 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -85,6 +85,7 @@
>  #define DRIVER_STRENGTH_40_OHM 0x4
>
>  #define CLOCK_TOO_SLOW_HZ      50000000
> +#define SDHCI_AM654_AUTOSUSPEND_DELAY  -1
>
>  /* Command Queue Host Controller Interface Base address */
>  #define SDHCI_AM654_CQE_BASE_ADDR 0x200
> @@ -808,16 +809,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,
> @@ -825,31 +820,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_probe(dev, ret, "parsing dt failed\n");
> -               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;
> @@ -858,23 +869,127 @@ 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);
> -       ret = pm_runtime_put_sync(&pdev->dev);
> +       ret = pm_runtime_resume_and_get(&pdev->dev);
>         if (ret < 0)
>                 return ret;
>
> +       sdhci_remove_host(host, true);
> +       clk_disable_unprepare(pltfm_host->clk);
>         pm_runtime_disable(&pdev->dev);
> +       pm_runtime_put_noidle(&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,
> --
> 2.40.0
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] mmc: sdhci_am654: Add support for PM suspend/resume

[Vignesh Raghavendra]

Hi,

On 31/03/23 20:01, Ulf Hansson wrote:
> On Fri, 31 Mar 2023 at 12:17, Vignesh Raghavendra <vigneshr@ti.com> wrote:
>>
>> From: Aswath Govindraju <a-govindraju@ti.com>
>>
>> Add support for suspend/resume and pm_runtime resume/suspend.
>>
>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>> Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
>> ---
>>
>> Since RFC v2:
>> Address all comments around sdhci_am654_remove()
>> Set autosuspend_delay to -1 as SDHCI will host rootfs via SD/eMMC and
>> autosuspend can cause long latency for user interactive applications
> 
> I am curious about the "long" latency. Do you have some data that you
> can share for how long it takes to runtime-resume the device?
> 

As part of wider teting, I discovered that on certain boards (J721e EVM
for example), there is a latency of ~500ms for controller to start up
(HW limitation around card detect debounce timer [0]). This results in
momentary (but noticeable) lag when playing a video for example.


[0[
https://elixir.bootlin.com/linux/latest/source/drivers/mmc/host/sdhci_am654.c#L361


> Using -1 as the default value for the autosuspend_delay seems fine to
> me, but did you consider using a longer timeout than the common 100ms?
> Could that perhaps be a better default option?

Its more of the startup delay than how long controller should remain
active. So tuning autosuspend_delay does not seem to help.

> 
> Of course, in the end this is a choice of performance in favor of
> wasting energy.

Yes, this is choice of opt-in vs opt-out. Users can tune the delay via
sysfs as needed.

I am setting it to -1 to avoid regressing any userspace apps (vs
enabling autosuspend and then asking impacted users to set to -1)

One solution is to use -1 on platforms where latency is significant
based on compatible / SoC detection?

Regards
Vignesh

> 
> Kind regards
> Uffe
> 
>>
>> RFC v2:
>> https://lore.kernel.org/all/20220408124338.27090-1-a-govindraju@ti.com/
>>
>>  drivers/mmc/host/sdhci_am654.c | 147 +++++++++++++++++++++++++++++----
>>  1 file changed, 131 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>> index 672d37ea98d0..7cdf0f54e3a5 100644
>> --- a/drivers/mmc/host/sdhci_am654.c
>> +++ b/drivers/mmc/host/sdhci_am654.c
>> @@ -85,6 +85,7 @@
>>  #define DRIVER_STRENGTH_40_OHM 0x4
>>
>>  #define CLOCK_TOO_SLOW_HZ      50000000
>> +#define SDHCI_AM654_AUTOSUSPEND_DELAY  -1
>>
>>  /* Command Queue Host Controller Interface Base address */
>>  #define SDHCI_AM654_CQE_BASE_ADDR 0x200
>> @@ -808,16 +809,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,
>> @@ -825,31 +820,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_probe(dev, ret, "parsing dt failed\n");
>> -               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;
>> @@ -858,23 +869,127 @@ 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);
>> -       ret = pm_runtime_put_sync(&pdev->dev);
>> +       ret = pm_runtime_resume_and_get(&pdev->dev);
>>         if (ret < 0)
>>                 return ret;
>>
>> +       sdhci_remove_host(host, true);
>> +       clk_disable_unprepare(pltfm_host->clk);
>>         pm_runtime_disable(&pdev->dev);
>> +       pm_runtime_put_noidle(&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,
>> --
>> 2.40.0
>>

-- 
Regards
Vignesh

Re: [PATCH v3] mmc: sdhci_am654: Add support for PM suspend/resume

[Vignesh Raghavendra]

Hi,

On 31/03/23 20:01, Ulf Hansson wrote:
> On Fri, 31 Mar 2023 at 12:17, Vignesh Raghavendra <vigneshr@ti.com> wrote:
>>
>> From: Aswath Govindraju <a-govindraju@ti.com>
>>
>> Add support for suspend/resume and pm_runtime resume/suspend.
>>
>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>> Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
>> ---
>>
>> Since RFC v2:
>> Address all comments around sdhci_am654_remove()
>> Set autosuspend_delay to -1 as SDHCI will host rootfs via SD/eMMC and
>> autosuspend can cause long latency for user interactive applications
> 
> I am curious about the "long" latency. Do you have some data that you
> can share for how long it takes to runtime-resume the device?
> 

As part of wider teting, I discovered that on certain boards (J721e EVM
for example), there is a latency of ~500ms for controller to start up
(HW limitation around card detect debounce timer [0]). This results in
momentary (but noticeable) lag when playing a video for example.


[0[
https://elixir.bootlin.com/linux/latest/source/drivers/mmc/host/sdhci_am654.c#L361


> Using -1 as the default value for the autosuspend_delay seems fine to
> me, but did you consider using a longer timeout than the common 100ms?
> Could that perhaps be a better default option?

Its more of the startup delay than how long controller should remain
active. So tuning autosuspend_delay does not seem to help.

> 
> Of course, in the end this is a choice of performance in favor of
> wasting energy.

Yes, this is choice of opt-in vs opt-out. Users can tune the delay via
sysfs as needed.

I am setting it to -1 to avoid regressing any userspace apps (vs
enabling autosuspend and then asking impacted users to set to -1)

One solution is to use -1 on platforms where latency is significant
based on compatible / SoC detection?

Regards
Vignesh

> 
> Kind regards
> Uffe
> 
>>
>> RFC v2:
>> https://lore.kernel.org/all/20220408124338.27090-1-a-govindraju@ti.com/
>>
>>  drivers/mmc/host/sdhci_am654.c | 147 +++++++++++++++++++++++++++++----
>>  1 file changed, 131 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>> index 672d37ea98d0..7cdf0f54e3a5 100644
>> --- a/drivers/mmc/host/sdhci_am654.c
>> +++ b/drivers/mmc/host/sdhci_am654.c
>> @@ -85,6 +85,7 @@
>>  #define DRIVER_STRENGTH_40_OHM 0x4
>>
>>  #define CLOCK_TOO_SLOW_HZ      50000000
>> +#define SDHCI_AM654_AUTOSUSPEND_DELAY  -1
>>
>>  /* Command Queue Host Controller Interface Base address */
>>  #define SDHCI_AM654_CQE_BASE_ADDR 0x200
>> @@ -808,16 +809,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,
>> @@ -825,31 +820,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_probe(dev, ret, "parsing dt failed\n");
>> -               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;
>> @@ -858,23 +869,127 @@ 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);
>> -       ret = pm_runtime_put_sync(&pdev->dev);
>> +       ret = pm_runtime_resume_and_get(&pdev->dev);
>>         if (ret < 0)
>>                 return ret;
>>
>> +       sdhci_remove_host(host, true);
>> +       clk_disable_unprepare(pltfm_host->clk);
>>         pm_runtime_disable(&pdev->dev);
>> +       pm_runtime_put_noidle(&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,
>> --
>> 2.40.0
>>

-- 
Regards
Vignesh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] mmc: sdhci_am654: Add support for PM suspend/resume

[Ulf Hansson]

On Mon, 3 Apr 2023 at 19:42, Vignesh Raghavendra <vigneshr@ti.com> wrote:
>
> Hi,
>
> On 31/03/23 20:01, Ulf Hansson wrote:
> > On Fri, 31 Mar 2023 at 12:17, Vignesh Raghavendra <vigneshr@ti.com> wrote:
> >>
> >> From: Aswath Govindraju <a-govindraju@ti.com>
> >>
> >> Add support for suspend/resume and pm_runtime resume/suspend.
> >>
> >> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> >> Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
> >> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> >> ---
> >>
> >> Since RFC v2:
> >> Address all comments around sdhci_am654_remove()
> >> Set autosuspend_delay to -1 as SDHCI will host rootfs via SD/eMMC and
> >> autosuspend can cause long latency for user interactive applications
> >
> > I am curious about the "long" latency. Do you have some data that you
> > can share for how long it takes to runtime-resume the device?
> >
>
> As part of wider teting, I discovered that on certain boards (J721e EVM
> for example), there is a latency of ~500ms for controller to start up
> (HW limitation around card detect debounce timer [0]). This results in
> momentary (but noticeable) lag when playing a video for example.

Ohh, that kind of sucks. There is no way to workaround that I guess?

>
>
> [0[
> https://elixir.bootlin.com/linux/latest/source/drivers/mmc/host/sdhci_am654.c#L361
>
>
> > Using -1 as the default value for the autosuspend_delay seems fine to
> > me, but did you consider using a longer timeout than the common 100ms?
> > Could that perhaps be a better default option?
>
> Its more of the startup delay than how long controller should remain
> active. So tuning autosuspend_delay does not seem to help.
>
> >
> > Of course, in the end this is a choice of performance in favor of
> > wasting energy.
>
> Yes, this is choice of opt-in vs opt-out. Users can tune the delay via
> sysfs as needed.
>
> I am setting it to -1 to avoid regressing any userspace apps (vs
> enabling autosuspend and then asking impacted users to set to -1)

Alright, makes sense!

>
> One solution is to use -1 on platforms where latency is significant
> based on compatible / SoC detection?

That's a good idea! It seems a pity to make all SoCs suffer, if there
is only a subset that has this problem. Although, let's consider that
as an improvement that can be done on top of $subject patch.

So, applied for next, thanks!

Kind regards
Uffe

> >>
> >> RFC v2:
> >> https://lore.kernel.org/all/20220408124338.27090-1-a-govindraju@ti.com/
> >>
> >>  drivers/mmc/host/sdhci_am654.c | 147 +++++++++++++++++++++++++++++----
> >>  1 file changed, 131 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> >> index 672d37ea98d0..7cdf0f54e3a5 100644
> >> --- a/drivers/mmc/host/sdhci_am654.c
> >> +++ b/drivers/mmc/host/sdhci_am654.c
> >> @@ -85,6 +85,7 @@
> >>  #define DRIVER_STRENGTH_40_OHM 0x4
> >>
> >>  #define CLOCK_TOO_SLOW_HZ      50000000
> >> +#define SDHCI_AM654_AUTOSUSPEND_DELAY  -1
> >>
> >>  /* Command Queue Host Controller Interface Base address */
> >>  #define SDHCI_AM654_CQE_BASE_ADDR 0x200
> >> @@ -808,16 +809,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,
> >> @@ -825,31 +820,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_probe(dev, ret, "parsing dt failed\n");
> >> -               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;
> >> @@ -858,23 +869,127 @@ 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);
> >> -       ret = pm_runtime_put_sync(&pdev->dev);
> >> +       ret = pm_runtime_resume_and_get(&pdev->dev);
> >>         if (ret < 0)
> >>                 return ret;
> >>
> >> +       sdhci_remove_host(host, true);
> >> +       clk_disable_unprepare(pltfm_host->clk);
> >>         pm_runtime_disable(&pdev->dev);
> >> +       pm_runtime_put_noidle(&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,
> >> --
> >> 2.40.0
> >>
>
> --
> Regards
> Vignesh

Re: [PATCH v3] mmc: sdhci_am654: Add support for PM suspend/resume

[Ulf Hansson]

On Mon, 3 Apr 2023 at 19:42, Vignesh Raghavendra <vigneshr@ti.com> wrote:
>
> Hi,
>
> On 31/03/23 20:01, Ulf Hansson wrote:
> > On Fri, 31 Mar 2023 at 12:17, Vignesh Raghavendra <vigneshr@ti.com> wrote:
> >>
> >> From: Aswath Govindraju <a-govindraju@ti.com>
> >>
> >> Add support for suspend/resume and pm_runtime resume/suspend.
> >>
> >> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> >> Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
> >> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> >> ---
> >>
> >> Since RFC v2:
> >> Address all comments around sdhci_am654_remove()
> >> Set autosuspend_delay to -1 as SDHCI will host rootfs via SD/eMMC and
> >> autosuspend can cause long latency for user interactive applications
> >
> > I am curious about the "long" latency. Do you have some data that you
> > can share for how long it takes to runtime-resume the device?
> >
>
> As part of wider teting, I discovered that on certain boards (J721e EVM
> for example), there is a latency of ~500ms for controller to start up
> (HW limitation around card detect debounce timer [0]). This results in
> momentary (but noticeable) lag when playing a video for example.

Ohh, that kind of sucks. There is no way to workaround that I guess?

>
>
> [0[
> https://elixir.bootlin.com/linux/latest/source/drivers/mmc/host/sdhci_am654.c#L361
>
>
> > Using -1 as the default value for the autosuspend_delay seems fine to
> > me, but did you consider using a longer timeout than the common 100ms?
> > Could that perhaps be a better default option?
>
> Its more of the startup delay than how long controller should remain
> active. So tuning autosuspend_delay does not seem to help.
>
> >
> > Of course, in the end this is a choice of performance in favor of
> > wasting energy.
>
> Yes, this is choice of opt-in vs opt-out. Users can tune the delay via
> sysfs as needed.
>
> I am setting it to -1 to avoid regressing any userspace apps (vs
> enabling autosuspend and then asking impacted users to set to -1)

Alright, makes sense!

>
> One solution is to use -1 on platforms where latency is significant
> based on compatible / SoC detection?

That's a good idea! It seems a pity to make all SoCs suffer, if there
is only a subset that has this problem. Although, let's consider that
as an improvement that can be done on top of $subject patch.

So, applied for next, thanks!

Kind regards
Uffe

> >>
> >> RFC v2:
> >> https://lore.kernel.org/all/20220408124338.27090-1-a-govindraju@ti.com/
> >>
> >>  drivers/mmc/host/sdhci_am654.c | 147 +++++++++++++++++++++++++++++----
> >>  1 file changed, 131 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> >> index 672d37ea98d0..7cdf0f54e3a5 100644
> >> --- a/drivers/mmc/host/sdhci_am654.c
> >> +++ b/drivers/mmc/host/sdhci_am654.c
> >> @@ -85,6 +85,7 @@
> >>  #define DRIVER_STRENGTH_40_OHM 0x4
> >>
> >>  #define CLOCK_TOO_SLOW_HZ      50000000
> >> +#define SDHCI_AM654_AUTOSUSPEND_DELAY  -1
> >>
> >>  /* Command Queue Host Controller Interface Base address */
> >>  #define SDHCI_AM654_CQE_BASE_ADDR 0x200
> >> @@ -808,16 +809,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,
> >> @@ -825,31 +820,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_probe(dev, ret, "parsing dt failed\n");
> >> -               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;
> >> @@ -858,23 +869,127 @@ 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);
> >> -       ret = pm_runtime_put_sync(&pdev->dev);
> >> +       ret = pm_runtime_resume_and_get(&pdev->dev);
> >>         if (ret < 0)
> >>                 return ret;
> >>
> >> +       sdhci_remove_host(host, true);
> >> +       clk_disable_unprepare(pltfm_host->clk);
> >>         pm_runtime_disable(&pdev->dev);
> >> +       pm_runtime_put_noidle(&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,
> >> --
> >> 2.40.0
> >>
>
> --
> Regards
> Vignesh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] mmc: sdhci_am654: Add support for PM suspend/resume

[Vignesh Raghavendra]

On 04/04/23 17:24, Ulf Hansson wrote:
> On Mon, 3 Apr 2023 at 19:42, Vignesh Raghavendra <vigneshr@ti.com> wrote:
>>
>> Hi,
>>
>> On 31/03/23 20:01, Ulf Hansson wrote:
>>> On Fri, 31 Mar 2023 at 12:17, Vignesh Raghavendra <vigneshr@ti.com> wrote:
>>>>
>>>> From: Aswath Govindraju <a-govindraju@ti.com>
>>>>
>>>> Add support for suspend/resume and pm_runtime resume/suspend.
>>>>
>>>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>>>> Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
>>>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
>>>> ---
>>>>
>>>> Since RFC v2:
>>>> Address all comments around sdhci_am654_remove()
>>>> Set autosuspend_delay to -1 as SDHCI will host rootfs via SD/eMMC and
>>>> autosuspend can cause long latency for user interactive applications
>>>
>>> I am curious about the "long" latency. Do you have some data that you
>>> can share for how long it takes to runtime-resume the device?
>>>
>>
>> As part of wider teting, I discovered that on certain boards (J721e EVM
>> for example), there is a latency of ~500ms for controller to start up
>> (HW limitation around card detect debounce timer [0]). This results in
>> momentary (but noticeable) lag when playing a video for example.
> 
> Ohh, that kind of sucks. There is no way to workaround that I guess?

Unfortunately no,

> 
>>
>>
>> [0[
>> https://elixir.bootlin.com/linux/latest/source/drivers/mmc/host/sdhci_am654.c#L361
>>
>>
>>> Using -1 as the default value for the autosuspend_delay seems fine to
>>> me, but did you consider using a longer timeout than the common 100ms?
>>> Could that perhaps be a better default option?
>>
>> Its more of the startup delay than how long controller should remain
>> active. So tuning autosuspend_delay does not seem to help.
>>
>>>
>>> Of course, in the end this is a choice of performance in favor of
>>> wasting energy.
>>
>> Yes, this is choice of opt-in vs opt-out. Users can tune the delay via
>> sysfs as needed.
>>
>> I am setting it to -1 to avoid regressing any userspace apps (vs
>> enabling autosuspend and then asking impacted users to set to -1)
> 
> Alright, makes sense!
> 
>>
>> One solution is to use -1 on platforms where latency is significant
>> based on compatible / SoC detection?
> 
> That's a good idea! It seems a pity to make all SoCs suffer, if there
> is only a subset that has this problem. Although, let's consider that
> as an improvement that can be done on top of $subject patch.

Sure, will follow up with a patch to reduce autosuspend_delay on
platforms w/o above issue.

> 
> So, applied for next, thanks!

Great, thanks!

-- 
Regards
Vignesh

Re: [PATCH v3] mmc: sdhci_am654: Add support for PM suspend/resume

[Vignesh Raghavendra]

On 04/04/23 17:24, Ulf Hansson wrote:
> On Mon, 3 Apr 2023 at 19:42, Vignesh Raghavendra <vigneshr@ti.com> wrote:
>>
>> Hi,
>>
>> On 31/03/23 20:01, Ulf Hansson wrote:
>>> On Fri, 31 Mar 2023 at 12:17, Vignesh Raghavendra <vigneshr@ti.com> wrote:
>>>>
>>>> From: Aswath Govindraju <a-govindraju@ti.com>
>>>>
>>>> Add support for suspend/resume and pm_runtime resume/suspend.
>>>>
>>>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>>>> Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
>>>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
>>>> ---
>>>>
>>>> Since RFC v2:
>>>> Address all comments around sdhci_am654_remove()
>>>> Set autosuspend_delay to -1 as SDHCI will host rootfs via SD/eMMC and
>>>> autosuspend can cause long latency for user interactive applications
>>>
>>> I am curious about the "long" latency. Do you have some data that you
>>> can share for how long it takes to runtime-resume the device?
>>>
>>
>> As part of wider teting, I discovered that on certain boards (J721e EVM
>> for example), there is a latency of ~500ms for controller to start up
>> (HW limitation around card detect debounce timer [0]). This results in
>> momentary (but noticeable) lag when playing a video for example.
> 
> Ohh, that kind of sucks. There is no way to workaround that I guess?

Unfortunately no,

> 
>>
>>
>> [0[
>> https://elixir.bootlin.com/linux/latest/source/drivers/mmc/host/sdhci_am654.c#L361
>>
>>
>>> Using -1 as the default value for the autosuspend_delay seems fine to
>>> me, but did you consider using a longer timeout than the common 100ms?
>>> Could that perhaps be a better default option?
>>
>> Its more of the startup delay than how long controller should remain
>> active. So tuning autosuspend_delay does not seem to help.
>>
>>>
>>> Of course, in the end this is a choice of performance in favor of
>>> wasting energy.
>>
>> Yes, this is choice of opt-in vs opt-out. Users can tune the delay via
>> sysfs as needed.
>>
>> I am setting it to -1 to avoid regressing any userspace apps (vs
>> enabling autosuspend and then asking impacted users to set to -1)
> 
> Alright, makes sense!
> 
>>
>> One solution is to use -1 on platforms where latency is significant
>> based on compatible / SoC detection?
> 
> That's a good idea! It seems a pity to make all SoCs suffer, if there
> is only a subset that has this problem. Although, let's consider that
> as an improvement that can be done on top of $subject patch.

Sure, will follow up with a patch to reduce autosuspend_delay on
platforms w/o above issue.

> 
> So, applied for next, thanks!

Great, thanks!

-- 
Regards
Vignesh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel