[PATCH v2 0/9] Init runtime PM support for dw_mmc

[PATCH v2 0/9] Init runtime PM support for dw_mmc

[Shawn Lin]

Hi Jaehoon and Ulf,

   This patch is gonna support runtime PM for dw_mmc.
It could support to disable ciu_clk by default and disable
biu_clk if the devices are non-removeable, or removeable
with gpio-base card detect.

   Then I remove the system PM since the runtime PM actually
does the same thing as it. So I help migrate the dw_mmc variant
drivers to use runtime PM pairs and pm_runtime_force_*. Note
that I only enable runtime PM for dw_mmc-rockchip as I will
leave the decision to the owners of the corresponding drivers.
I just tested it on my RK3288 platform with linux-next to make
the runtime PM and system PM work fine for my emmc, sd card and
sdio. But I don't have hardware to help test other variant drivers.
But in theory it should work fine as I mentioned that the runtime
PM does the same thing as system PM except for disabling ciu_clk
aggressively which should not be related to the variant hosts.

   As you could see that I just extend the slot-gpio a bit, so the
ideal way is Ulf could pick them up with Jaehoon's ack. :)


Changes in v2:
- use struct device as argument for runtime callback
- use dw_mci_runtime_* directly
- use dw_mci_runtime_* directly
- minor fix since I change the argument for dw_mci_runtime_*
- use dw_mci_runtime_* directly
- use dw_mci_runtime_* directly

Shawn Lin (9):
  mmc: dw_mmc: add runtime PM callback
  mmc: dw_mmc-rockchip: add runtime PM support
  mmc: core: expose the capability of gpio card detect
  mmc: dw_mmc: disable biu clk if possible
  mmc: dw_mmc-k3: deploy runtime PM facilities
  mmc: dw_mmc-exynos: deploy runtime PM facilities
  mmc: dw_mmc-pci: deploy runtime PM facilities
  mmc: dw_mmc-pltfm: deploy runtime PM facilities
  mmc: dw_mmc: remove system PM callback

 drivers/mmc/core/slot-gpio.c       |  8 +++++++
 drivers/mmc/host/dw_mmc-exynos.c   | 24 +++++++++-----------
 drivers/mmc/host/dw_mmc-k3.c       | 39 ++++++++------------------------
 drivers/mmc/host/dw_mmc-pci.c      | 29 ++++++++----------------
 drivers/mmc/host/dw_mmc-pltfm.c    | 28 +++++++----------------
 drivers/mmc/host/dw_mmc-rockchip.c | 42 +++++++++++++++++++++++++++++++---
 drivers/mmc/host/dw_mmc.c          | 46 +++++++++++++++++++++++++-------------
 drivers/mmc/host/dw_mmc.h          |  6 ++---
 include/linux/mmc/slot-gpio.h      |  1 +
 9 files changed, 117 insertions(+), 106 deletions(-)

-- 
2.3.7

[PATCH v2 1/9] mmc: dw_mmc: add runtime PM callback

[Shawn Lin]

This patch add dw_mci_runtime_suspend/resume interfaces
and expose it to dw_mci variant driver to support runtime
PM.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

Changes in v2:
- use struct device as argument for runtime callback

 drivers/mmc/host/dw_mmc.c | 32 ++++++++++++++++++++++++++++++--
 drivers/mmc/host/dw_mmc.h |  4 +++-
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 4fcbc40..67fa088 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -3266,7 +3266,7 @@ EXPORT_SYMBOL(dw_mci_remove);
 
 
 
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_PM
 /*
  * TODO: we should probably disable the clock to the card in the suspend path.
  */
@@ -3324,7 +3324,35 @@ int dw_mci_resume(struct dw_mci *host)
 	return 0;
 }
 EXPORT_SYMBOL(dw_mci_resume);
-#endif /* CONFIG_PM_SLEEP */
+
+int dw_mci_runtime_suspend(struct device *dev)
+{
+	int err = 0;
+	struct dw_mci *host = dev_get_drvdata(dev);
+
+	err = dw_mci_suspend(host);
+	if (err)
+		return err;
+
+	clk_disable_unprepare(host->ciu_clk);
+
+	return err;
+}
+EXPORT_SYMBOL(dw_mci_runtime_suspend);
+
+int dw_mci_runtime_resume(struct device *dev)
+{
+	int ret = 0;
+	struct dw_mci *host = dev_get_drvdata(dev);
+
+	ret = clk_prepare_enable(host->ciu_clk);
+	if (ret)
+		return ret;
+
+	return dw_mci_resume(host);
+}
+EXPORT_SYMBOL(dw_mci_runtime_resume);
+#endif /* CONFIG_PM */
 
 static int __init dw_mci_init(void)
 {
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index e8cd2de..d14a391 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -234,9 +234,11 @@
 
 extern int dw_mci_probe(struct dw_mci *host);
 extern void dw_mci_remove(struct dw_mci *host);
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_PM
 extern int dw_mci_suspend(struct dw_mci *host);
 extern int dw_mci_resume(struct dw_mci *host);
+extern int dw_mci_runtime_suspend(struct device *device);
+extern int dw_mci_runtime_resume(struct device *device);
 #endif
 
 /**
-- 
2.3.7

[PATCH v2 2/9] mmc: dw_mmc-rockchip: add runtime PM support

[Shawn Lin]

This patch adds runtime PM support for dw_mmc-rockchip.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

Changes in v2:
- use dw_mci_runtime_* directly

 drivers/mmc/host/dw_mmc-rockchip.c | 41 +++++++++++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
index 25eae35..0a05ad3 100644
--- a/drivers/mmc/host/dw_mmc-rockchip.c
+++ b/drivers/mmc/host/dw_mmc-rockchip.c
@@ -13,6 +13,7 @@
 #include <linux/mmc/host.h>
 #include <linux/mmc/dw_mmc.h>
 #include <linux/of_address.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 
 #include "dw_mmc.h"
@@ -325,6 +326,7 @@ static int dw_mci_rockchip_probe(struct platform_device *pdev)
 {
 	const struct dw_mci_drv_data *drv_data;
 	const struct of_device_id *match;
+	int ret;
 
 	if (!pdev->dev.of_node)
 		return -ENODEV;
@@ -332,16 +334,49 @@ static int dw_mci_rockchip_probe(struct platform_device *pdev)
 	match = of_match_node(dw_mci_rockchip_match, pdev->dev.of_node);
 	drv_data = match->data;
 
-	return dw_mci_pltfm_register(pdev, drv_data);
+	pm_runtime_get_noresume(&pdev->dev);
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
+	pm_runtime_use_autosuspend(&pdev->dev);
+
+	ret = dw_mci_pltfm_register(pdev, drv_data);
+	if (ret) {
+		pm_runtime_disable(&pdev->dev);
+		pm_runtime_set_suspended(&pdev->dev);
+		pm_runtime_put_noidle(&pdev->dev);
+		return ret;
+	}
+
+	pm_runtime_put_autosuspend(&pdev->dev);
+
+	return 0;
 }
 
+static int dw_mci_rockchip_remove(struct platform_device *pdev)
+{
+	pm_runtime_get_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_put_noidle(&pdev->dev);
+
+	return dw_mci_pltfm_remove(pdev);
+}
+
+static const struct dev_pm_ops dw_mci_rockchip_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(dw_mci_runtime_suspend,
+			   dw_mci_runtime_resume,
+			   NULL)
+};
+
 static struct platform_driver dw_mci_rockchip_pltfm_driver = {
 	.probe		= dw_mci_rockchip_probe,
-	.remove		= dw_mci_pltfm_remove,
+	.remove		= dw_mci_rockchip_remove,
 	.driver		= {
 		.name		= "dwmmc_rockchip",
 		.of_match_table	= dw_mci_rockchip_match,
-		.pm		= &dw_mci_pltfm_pmops,
+		.pm		= &dw_mci_rockchip_dev_pm_ops,
 	},
 };
 
-- 
2.3.7

[PATCH v2 3/9] mmc: core: expose the capability of gpio card detect

[Shawn Lin]

Add new helper API mmc_can_gpio_cd for slot-gpio to make
host drivers know whether it supports gpio card detect.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v2: None

 drivers/mmc/core/slot-gpio.c  | 8 ++++++++
 include/linux/mmc/slot-gpio.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
index 27117ba..babe591 100644
--- a/drivers/mmc/core/slot-gpio.c
+++ b/drivers/mmc/core/slot-gpio.c
@@ -258,6 +258,14 @@ int mmc_gpiod_request_cd(struct mmc_host *host, const char *con_id,
 }
 EXPORT_SYMBOL(mmc_gpiod_request_cd);
 
+bool mmc_can_gpio_cd(struct mmc_host *host)
+{
+	struct mmc_gpio *ctx = host->slot.handler_priv;
+
+	return ctx->cd_gpio ? true : false;
+}
+EXPORT_SYMBOL(mmc_can_gpio_cd);
+
 /**
  * mmc_gpiod_request_ro - request a gpio descriptor for write protection
  * @host: mmc host
diff --git a/include/linux/mmc/slot-gpio.h b/include/linux/mmc/slot-gpio.h
index 3945a8c..a7972cd 100644
--- a/include/linux/mmc/slot-gpio.h
+++ b/include/linux/mmc/slot-gpio.h
@@ -29,5 +29,6 @@ int mmc_gpiod_request_ro(struct mmc_host *host, const char *con_id,
 void mmc_gpio_set_cd_isr(struct mmc_host *host,
 			 irqreturn_t (*isr)(int irq, void *dev_id));
 void mmc_gpiod_request_cd_irq(struct mmc_host *host);
+bool mmc_can_gpio_cd(struct mmc_host *host);
 
 #endif
-- 
2.3.7

[PATCH v2 4/9] mmc: dw_mmc: disable biu clk if possible

[Shawn Lin]

We could disable biu clk if gpio card detect available,
or it is a non-removable device.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v2: None

 drivers/mmc/host/dw_mmc-rockchip.c |  1 +
 drivers/mmc/host/dw_mmc.c          | 13 +++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
index 0a05ad3..9a46e46 100644
--- a/drivers/mmc/host/dw_mmc-rockchip.c
+++ b/drivers/mmc/host/dw_mmc-rockchip.c
@@ -13,6 +13,7 @@
 #include <linux/mmc/host.h>
 #include <linux/mmc/dw_mmc.h>
 #include <linux/of_address.h>
+#include <linux/mmc/slot-gpio.h>
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
 
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 67fa088..297b6c2 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -3336,6 +3336,11 @@ int dw_mci_runtime_suspend(struct device *dev)
 
 	clk_disable_unprepare(host->ciu_clk);
 
+	if (host->cur_slot &&
+	    (mmc_can_gpio_cd(host->cur_slot->mmc) ||
+	     !mmc_card_is_removable(host->cur_slot->mmc)))
+		clk_disable_unprepare(host->biu_clk);
+
 	return err;
 }
 EXPORT_SYMBOL(dw_mci_runtime_suspend);
@@ -3345,6 +3350,14 @@ int dw_mci_runtime_resume(struct device *dev)
 	int ret = 0;
 	struct dw_mci *host = dev_get_drvdata(dev);
 
+	if (host->cur_slot &&
+	    (mmc_can_gpio_cd(host->cur_slot->mmc) ||
+	     !mmc_card_is_removable(host->cur_slot->mmc))) {
+		ret = clk_prepare_enable(host->biu_clk);
+		if (ret)
+			return ret;
+	}
+
 	ret = clk_prepare_enable(host->ciu_clk);
 	if (ret)
 		return ret;
-- 
2.3.7

[PATCH v2 5/9] mmc: dw_mmc-k3: deploy runtime PM facilities

[Shawn Lin]

Let's migrate it to use runtime PM and remove the system
PM callback from this driver. With this patch, it could
handle system PM properly and could also use runtime PM
if we enable it.

Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>

---

Changes in v2:
- use dw_mci_runtime_* directly

 drivers/mmc/host/dw_mmc-k3.c | 39 +++++++++------------------------------
 1 file changed, 9 insertions(+), 30 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc-k3.c b/drivers/mmc/host/dw_mmc-k3.c
index 6247894..9821e6b 100644
--- a/drivers/mmc/host/dw_mmc-k3.c
+++ b/drivers/mmc/host/dw_mmc-k3.c
@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
@@ -162,35 +163,13 @@ static int dw_mci_k3_probe(struct platform_device *pdev)
 	return dw_mci_pltfm_register(pdev, drv_data);
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int dw_mci_k3_suspend(struct device *dev)
-{
-	struct dw_mci *host = dev_get_drvdata(dev);
-	int ret;
-
-	ret = dw_mci_suspend(host);
-	if (!ret)
-		clk_disable_unprepare(host->ciu_clk);
-
-	return ret;
-}
-
-static int dw_mci_k3_resume(struct device *dev)
-{
-	struct dw_mci *host = dev_get_drvdata(dev);
-	int ret;
-
-	ret = clk_prepare_enable(host->ciu_clk);
-	if (ret) {
-		dev_err(host->dev, "failed to enable ciu clock\n");
-		return ret;
-	}
-
-	return dw_mci_resume(host);
-}
-#endif /* CONFIG_PM_SLEEP */
-
-static SIMPLE_DEV_PM_OPS(dw_mci_k3_pmops, dw_mci_k3_suspend, dw_mci_k3_resume);
+static const struct dev_pm_ops dw_mci_k3_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(dw_mci_runtime_suspend,
+			   dw_mci_runtime_resume,
+			   NULL)
+};
 
 static struct platform_driver dw_mci_k3_pltfm_driver = {
 	.probe		= dw_mci_k3_probe,
@@ -198,7 +177,7 @@ static struct platform_driver dw_mci_k3_pltfm_driver = {
 	.driver		= {
 		.name		= "dwmmc_k3",
 		.of_match_table	= dw_mci_k3_match,
-		.pm		= &dw_mci_k3_pmops,
+		.pm		= &dw_mci_k3_dev_pm_ops,
 	},
 };
 
-- 
2.3.7

[PATCH v2 6/9] mmc: dw_mmc-exynos: deploy runtime PM facilities

[Shawn Lin]

Let's migrate it to use runtime PM and remove the system
PM callback from this driver. With this patch, it could
handle system PM properly and could also use runtime PM
if we enable it.

Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>

---

Changes in v2:
- minor fix since I change the argument for dw_mci_runtime_*

 drivers/mmc/host/dw_mmc-exynos.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
index 7ab3d74..e2439a1 100644
--- a/drivers/mmc/host/dw_mmc-exynos.c
+++ b/drivers/mmc/host/dw_mmc-exynos.c
@@ -17,6 +17,7 @@
 #include <linux/mmc/mmc.h>
 #include <linux/of.h>
 #include <linux/of_gpio.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 
 #include "dw_mmc.h"
@@ -161,20 +162,13 @@ static void dw_mci_exynos_set_clksel_timing(struct dw_mci *host, u32 timing)
 		set_bit(DW_MMC_CARD_NO_USE_HOLD, &host->cur_slot->flags);
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int dw_mci_exynos_suspend(struct device *dev)
-{
-	struct dw_mci *host = dev_get_drvdata(dev);
-
-	return dw_mci_suspend(host);
-}
-
-static int dw_mci_exynos_resume(struct device *dev)
+#ifdef CONFIG_PM
+static int dw_mci_exynos_runtime_resume(struct device *dev)
 {
 	struct dw_mci *host = dev_get_drvdata(dev);
 
 	dw_mci_exynos_config_smu(host);
-	return dw_mci_resume(host);
+	return dw_mci_runtime_resume(dev);
 }
 
 /**
@@ -211,10 +205,8 @@ static int dw_mci_exynos_resume_noirq(struct device *dev)
 	return 0;
 }
 #else
-#define dw_mci_exynos_suspend		NULL
-#define dw_mci_exynos_resume		NULL
 #define dw_mci_exynos_resume_noirq	NULL
-#endif /* CONFIG_PM_SLEEP */
+#endif /* CONFIG_PM */
 
 static void dw_mci_exynos_config_hs400(struct dw_mci *host, u32 timing)
 {
@@ -531,7 +523,11 @@ static int dw_mci_exynos_probe(struct platform_device *pdev)
 }
 
 static const struct dev_pm_ops dw_mci_exynos_pmops = {
-	SET_SYSTEM_SLEEP_PM_OPS(dw_mci_exynos_suspend, dw_mci_exynos_resume)
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(dw_mci_runtime_suspend,
+			   dw_mci_exynos_runtime_resume,
+			   NULL)
 	.resume_noirq = dw_mci_exynos_resume_noirq,
 	.thaw_noirq = dw_mci_exynos_resume_noirq,
 	.restore_noirq = dw_mci_exynos_resume_noirq,
-- 
2.3.7

[PATCH v2 7/9] mmc: dw_mmc-pci: deploy runtime PM facilities

[Shawn Lin]

Let's migrate it to use runtime PM and remove the system
PM callback from this driver. With this patch, it could
handle system PM properly and could also use runtime PM
if we enable it.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

Changes in v2:
- use dw_mci_runtime_* directly

 drivers/mmc/host/dw_mmc-pci.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc-pci.c b/drivers/mmc/host/dw_mmc-pci.c
index 4c69fbd..ab82796 100644
--- a/drivers/mmc/host/dw_mmc-pci.c
+++ b/drivers/mmc/host/dw_mmc-pci.c
@@ -14,6 +14,7 @@
 #include <linux/io.h>
 #include <linux/irq.h>
 #include <linux/pci.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/mmc/host.h>
 #include <linux/mmc/mmc.h>
@@ -79,25 +80,13 @@ static void dw_mci_pci_remove(struct pci_dev *pdev)
 	dw_mci_remove(host);
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int dw_mci_pci_suspend(struct device *dev)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-	struct dw_mci *host = pci_get_drvdata(pdev);
-
-	return dw_mci_suspend(host);
-}
-
-static int dw_mci_pci_resume(struct device *dev)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-	struct dw_mci *host = pci_get_drvdata(pdev);
-
-	return dw_mci_resume(host);
-}
-#endif /* CONFIG_PM_SLEEP */
-
-static SIMPLE_DEV_PM_OPS(dw_mci_pci_pmops, dw_mci_pci_suspend, dw_mci_pci_resume);
+static const struct dev_pm_ops dw_mci_pci_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(dw_mci_runtime_suspend,
+			   dw_mci_runtime_resume,
+			   NULL)
+};
 
 static const struct pci_device_id dw_mci_pci_id[] = {
 	{ PCI_DEVICE(SYNOPSYS_DW_MCI_VENDOR_ID, SYNOPSYS_DW_MCI_DEVICE_ID) },
@@ -111,7 +100,7 @@ static struct pci_driver dw_mci_pci_driver = {
 	.probe		= dw_mci_pci_probe,
 	.remove		= dw_mci_pci_remove,
 	.driver		=	{
-		.pm =   &dw_mci_pci_pmops
+		.pm =   &dw_mci_pci_dev_pm_ops,
 	},
 };
 
-- 
2.3.7

[PATCH v2 8/9] mmc: dw_mmc-pltfm: deploy runtime PM facilities

[Shawn Lin]

Let's migrate it to use runtime PM and remove the system
PM callback from this driver. With this patch, it could
handle system PM properly and could also use runtime PM
if we enable it.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

Changes in v2:
- use dw_mci_runtime_* directly

 drivers/mmc/host/dw_mmc-pltfm.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c
index c0bb0c7..b486fef 100644
--- a/drivers/mmc/host/dw_mmc-pltfm.c
+++ b/drivers/mmc/host/dw_mmc-pltfm.c
@@ -16,6 +16,7 @@
 #include <linux/io.h>
 #include <linux/irq.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/mmc/host.h>
 #include <linux/mmc/mmc.h>
@@ -57,26 +58,13 @@ int dw_mci_pltfm_register(struct platform_device *pdev,
 }
 EXPORT_SYMBOL_GPL(dw_mci_pltfm_register);
 
-#ifdef CONFIG_PM_SLEEP
-/*
- * TODO: we should probably disable the clock to the card in the suspend path.
- */
-static int dw_mci_pltfm_suspend(struct device *dev)
-{
-	struct dw_mci *host = dev_get_drvdata(dev);
-
-	return dw_mci_suspend(host);
-}
-
-static int dw_mci_pltfm_resume(struct device *dev)
-{
-	struct dw_mci *host = dev_get_drvdata(dev);
-
-	return dw_mci_resume(host);
-}
-#endif /* CONFIG_PM_SLEEP */
-
-SIMPLE_DEV_PM_OPS(dw_mci_pltfm_pmops, dw_mci_pltfm_suspend, dw_mci_pltfm_resume);
+const struct dev_pm_ops dw_mci_pltfm_pmops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(dw_mci_runtime_suspend,
+			   dw_mci_runtime_resume,
+			   NULL)
+};
 EXPORT_SYMBOL_GPL(dw_mci_pltfm_pmops);
 
 static const struct of_device_id dw_mci_pltfm_match[] = {
-- 
2.3.7

[PATCH v2 9/9] mmc: dw_mmc: remove system PM callback

[Shawn Lin]

Now there are no variant drivers using dw_mci_suspend
and dw_mci_resume, so let's remove it.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v2: None

 drivers/mmc/host/dw_mmc.c | 81 ++++++++++++++++-------------------------------
 drivers/mmc/host/dw_mmc.h |  2 --
 2 files changed, 27 insertions(+), 56 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 297b6c2..1c9ee36 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -3267,27 +3267,41 @@ EXPORT_SYMBOL(dw_mci_remove);
 
 
 #ifdef CONFIG_PM
-/*
- * TODO: we should probably disable the clock to the card in the suspend path.
- */
-int dw_mci_suspend(struct dw_mci *host)
+int dw_mci_runtime_suspend(struct device *dev)
 {
+	struct dw_mci *host = dev_get_drvdata(dev);
+
 	if (host->use_dma && host->dma_ops->exit)
 		host->dma_ops->exit(host);
 
+	clk_disable_unprepare(host->ciu_clk);
+
+	if (host->cur_slot &&
+	    (mmc_can_gpio_cd(host->cur_slot->mmc) ||
+	     !mmc_card_is_removable(host->cur_slot->mmc)))
+		clk_disable_unprepare(host->biu_clk);
+
 	return 0;
 }
-EXPORT_SYMBOL(dw_mci_suspend);
+EXPORT_SYMBOL(dw_mci_runtime_suspend);
 
-int dw_mci_resume(struct dw_mci *host)
+int dw_mci_runtime_resume(struct device *dev)
 {
-	int i, ret;
+	int i, ret = 0;
+	struct dw_mci *host = dev_get_drvdata(dev);
 
-	if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS)) {
-		ret = -ENODEV;
-		return ret;
+	if (host->cur_slot &&
+	    (mmc_can_gpio_cd(host->cur_slot->mmc) ||
+	     !mmc_card_is_removable(host->cur_slot->mmc))) {
+		ret = clk_prepare_enable(host->biu_clk);
+		if (ret)
+			return ret;
 	}
 
+	ret = clk_prepare_enable(host->ciu_clk);
+	if (ret)
+		return ret;
+
 	if (host->use_dma && host->dma_ops->init)
 		host->dma_ops->init(host);
 
@@ -3295,8 +3309,8 @@ int dw_mci_resume(struct dw_mci *host)
 	 * Restore the initial value at FIFOTH register
 	 * And Invalidate the prev_blksz with zero
 	 */
-	mci_writel(host, FIFOTH, host->fifoth_val);
-	host->prev_blksz = 0;
+	 mci_writel(host, FIFOTH, host->fifoth_val);
+	 host->prev_blksz = 0;
 
 	/* Put in max timeout */
 	mci_writel(host, TMOUT, 0xFFFFFFFF);
@@ -3321,48 +3335,7 @@ int dw_mci_resume(struct dw_mci *host)
 	/* Now that slots are all setup, we can enable card detect */
 	dw_mci_enable_cd(host);
 
-	return 0;
-}
-EXPORT_SYMBOL(dw_mci_resume);
-
-int dw_mci_runtime_suspend(struct device *dev)
-{
-	int err = 0;
-	struct dw_mci *host = dev_get_drvdata(dev);
-
-	err = dw_mci_suspend(host);
-	if (err)
-		return err;
-
-	clk_disable_unprepare(host->ciu_clk);
-
-	if (host->cur_slot &&
-	    (mmc_can_gpio_cd(host->cur_slot->mmc) ||
-	     !mmc_card_is_removable(host->cur_slot->mmc)))
-		clk_disable_unprepare(host->biu_clk);
-
-	return err;
-}
-EXPORT_SYMBOL(dw_mci_runtime_suspend);
-
-int dw_mci_runtime_resume(struct device *dev)
-{
-	int ret = 0;
-	struct dw_mci *host = dev_get_drvdata(dev);
-
-	if (host->cur_slot &&
-	    (mmc_can_gpio_cd(host->cur_slot->mmc) ||
-	     !mmc_card_is_removable(host->cur_slot->mmc))) {
-		ret = clk_prepare_enable(host->biu_clk);
-		if (ret)
-			return ret;
-	}
-
-	ret = clk_prepare_enable(host->ciu_clk);
-	if (ret)
-		return ret;
-
-	return dw_mci_resume(host);
+	return ret;
 }
 EXPORT_SYMBOL(dw_mci_runtime_resume);
 #endif /* CONFIG_PM */
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index d14a391..4a6ae750 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -235,8 +235,6 @@
 extern int dw_mci_probe(struct dw_mci *host);
 extern void dw_mci_remove(struct dw_mci *host);
 #ifdef CONFIG_PM
-extern int dw_mci_suspend(struct dw_mci *host);
-extern int dw_mci_resume(struct dw_mci *host);
 extern int dw_mci_runtime_suspend(struct device *device);
 extern int dw_mci_runtime_resume(struct device *device);
 #endif
-- 
2.3.7

Re: [PATCH v2 0/9] Init runtime PM support for dw_mmc

[Jaehoon Chung]

Hi Shawn,

On 10/12/2016 11:50 AM, Shawn Lin wrote:
> Hi Jaehoon and Ulf,
> 
>    This patch is gonna support runtime PM for dw_mmc.
> It could support to disable ciu_clk by default and disable
> biu_clk if the devices are non-removeable, or removeable
> with gpio-base card detect.
> 
>    Then I remove the system PM since the runtime PM actually
> does the same thing as it. So I help migrate the dw_mmc variant
> drivers to use runtime PM pairs and pm_runtime_force_*. Note
> that I only enable runtime PM for dw_mmc-rockchip as I will
> leave the decision to the owners of the corresponding drivers.
> I just tested it on my RK3288 platform with linux-next to make
> the runtime PM and system PM work fine for my emmc, sd card and
> sdio. But I don't have hardware to help test other variant drivers.
> But in theory it should work fine as I mentioned that the runtime
> PM does the same thing as system PM except for disabling ciu_clk
> aggressively which should not be related to the variant hosts.
> 
>    As you could see that I just extend the slot-gpio a bit, so the
> ideal way is Ulf could pick them up with Jaehoon's ack. :)

I have tested with my boards..it's working fine..I will apply on this if there is no special issue.

Thanks!

Best Regards,
Jaehoon Chung

> 
> 
> Changes in v2:
> - use struct device as argument for runtime callback
> - use dw_mci_runtime_* directly
> - use dw_mci_runtime_* directly
> - minor fix since I change the argument for dw_mci_runtime_*
> - use dw_mci_runtime_* directly
> - use dw_mci_runtime_* directly
> 
> Shawn Lin (9):
>   mmc: dw_mmc: add runtime PM callback
>   mmc: dw_mmc-rockchip: add runtime PM support
>   mmc: core: expose the capability of gpio card detect
>   mmc: dw_mmc: disable biu clk if possible
>   mmc: dw_mmc-k3: deploy runtime PM facilities
>   mmc: dw_mmc-exynos: deploy runtime PM facilities
>   mmc: dw_mmc-pci: deploy runtime PM facilities
>   mmc: dw_mmc-pltfm: deploy runtime PM facilities
>   mmc: dw_mmc: remove system PM callback
> 
>  drivers/mmc/core/slot-gpio.c       |  8 +++++++
>  drivers/mmc/host/dw_mmc-exynos.c   | 24 +++++++++-----------
>  drivers/mmc/host/dw_mmc-k3.c       | 39 ++++++++------------------------
>  drivers/mmc/host/dw_mmc-pci.c      | 29 ++++++++----------------
>  drivers/mmc/host/dw_mmc-pltfm.c    | 28 +++++++----------------
>  drivers/mmc/host/dw_mmc-rockchip.c | 42 +++++++++++++++++++++++++++++++---
>  drivers/mmc/host/dw_mmc.c          | 46 +++++++++++++++++++++++++-------------
>  drivers/mmc/host/dw_mmc.h          |  6 ++---
>  include/linux/mmc/slot-gpio.h      |  1 +
>  9 files changed, 117 insertions(+), 106 deletions(-)
> 

Re: [PATCH v2 0/9] Init runtime PM support for dw_mmc

[Shawn Lin]

Hi Jaehoon,

On 2016/10/18 8:24, Jaehoon Chung wrote:
> Hi Shawn,
>
> On 10/12/2016 11:50 AM, Shawn Lin wrote:
>> Hi Jaehoon and Ulf,
>>
>>    This patch is gonna support runtime PM for dw_mmc.
>> It could support to disable ciu_clk by default and disable
>> biu_clk if the devices are non-removeable, or removeable
>> with gpio-base card detect.
>>
>>    Then I remove the system PM since the runtime PM actually
>> does the same thing as it. So I help migrate the dw_mmc variant
>> drivers to use runtime PM pairs and pm_runtime_force_*. Note
>> that I only enable runtime PM for dw_mmc-rockchip as I will
>> leave the decision to the owners of the corresponding drivers.
>> I just tested it on my RK3288 platform with linux-next to make
>> the runtime PM and system PM work fine for my emmc, sd card and
>> sdio. But I don't have hardware to help test other variant drivers.
>> But in theory it should work fine as I mentioned that the runtime
>> PM does the same thing as system PM except for disabling ciu_clk
>> aggressively which should not be related to the variant hosts.
>>
>>    As you could see that I just extend the slot-gpio a bit, so the
>> ideal way is Ulf could pick them up with Jaehoon's ack. :)
>
> I have tested with my boards..it's working fine..I will apply on this if there is no special issue.

Great to hear this. The test for this patchset have been running for a
while on my rk3288/3368 boards and it seems no failures were found
until now.

Thanks!

>
> Thanks!
>
> Best Regards,
> Jaehoon Chung
>
>>
>>
>> Changes in v2:
>> - use struct device as argument for runtime callback
>> - use dw_mci_runtime_* directly
>> - use dw_mci_runtime_* directly
>> - minor fix since I change the argument for dw_mci_runtime_*
>> - use dw_mci_runtime_* directly
>> - use dw_mci_runtime_* directly
>>
>> Shawn Lin (9):
>>   mmc: dw_mmc: add runtime PM callback
>>   mmc: dw_mmc-rockchip: add runtime PM support
>>   mmc: core: expose the capability of gpio card detect
>>   mmc: dw_mmc: disable biu clk if possible
>>   mmc: dw_mmc-k3: deploy runtime PM facilities
>>   mmc: dw_mmc-exynos: deploy runtime PM facilities
>>   mmc: dw_mmc-pci: deploy runtime PM facilities
>>   mmc: dw_mmc-pltfm: deploy runtime PM facilities
>>   mmc: dw_mmc: remove system PM callback
>>
>>  drivers/mmc/core/slot-gpio.c       |  8 +++++++
>>  drivers/mmc/host/dw_mmc-exynos.c   | 24 +++++++++-----------
>>  drivers/mmc/host/dw_mmc-k3.c       | 39 ++++++++------------------------
>>  drivers/mmc/host/dw_mmc-pci.c      | 29 ++++++++----------------
>>  drivers/mmc/host/dw_mmc-pltfm.c    | 28 +++++++----------------
>>  drivers/mmc/host/dw_mmc-rockchip.c | 42 +++++++++++++++++++++++++++++++---
>>  drivers/mmc/host/dw_mmc.c          | 46 +++++++++++++++++++++++++-------------
>>  drivers/mmc/host/dw_mmc.h          |  6 ++---
>>  include/linux/mmc/slot-gpio.h      |  1 +
>>  9 files changed, 117 insertions(+), 106 deletions(-)
>>
>
>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH v2 0/9] Init runtime PM support for dw_mmc

[Ulf Hansson]

+ Heiko

On 12 October 2016 at 04:50, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> Hi Jaehoon and Ulf,
>
>    This patch is gonna support runtime PM for dw_mmc.
> It could support to disable ciu_clk by default and disable
> biu_clk if the devices are non-removeable, or removeable
> with gpio-base card detect.
>
>    Then I remove the system PM since the runtime PM actually
> does the same thing as it. So I help migrate the dw_mmc variant
> drivers to use runtime PM pairs and pm_runtime_force_*. Note
> that I only enable runtime PM for dw_mmc-rockchip as I will
> leave the decision to the owners of the corresponding drivers.
> I just tested it on my RK3288 platform with linux-next to make
> the runtime PM and system PM work fine for my emmc, sd card and
> sdio. But I don't have hardware to help test other variant drivers.
> But in theory it should work fine as I mentioned that the runtime
> PM does the same thing as system PM except for disabling ciu_clk
> aggressively which should not be related to the variant hosts.
>
>    As you could see that I just extend the slot-gpio a bit, so the
> ideal way is Ulf could pick them up with Jaehoon's ack. :)

The mmc core change looks fine to me, so I will wait for a pull
request from Jaehoon.

>
>
> Changes in v2:
> - use struct device as argument for runtime callback
> - use dw_mci_runtime_* directly
> - use dw_mci_runtime_* directly
> - minor fix since I change the argument for dw_mci_runtime_*
> - use dw_mci_runtime_* directly
> - use dw_mci_runtime_* directly
>
> Shawn Lin (9):
>   mmc: dw_mmc: add runtime PM callback
>   mmc: dw_mmc-rockchip: add runtime PM support
>   mmc: core: expose the capability of gpio card detect
>   mmc: dw_mmc: disable biu clk if possible
>   mmc: dw_mmc-k3: deploy runtime PM facilities
>   mmc: dw_mmc-exynos: deploy runtime PM facilities
>   mmc: dw_mmc-pci: deploy runtime PM facilities
>   mmc: dw_mmc-pltfm: deploy runtime PM facilities
>   mmc: dw_mmc: remove system PM callback
>
>  drivers/mmc/core/slot-gpio.c       |  8 +++++++
>  drivers/mmc/host/dw_mmc-exynos.c   | 24 +++++++++-----------
>  drivers/mmc/host/dw_mmc-k3.c       | 39 ++++++++------------------------
>  drivers/mmc/host/dw_mmc-pci.c      | 29 ++++++++----------------
>  drivers/mmc/host/dw_mmc-pltfm.c    | 28 +++++++----------------
>  drivers/mmc/host/dw_mmc-rockchip.c | 42 +++++++++++++++++++++++++++++++---
>  drivers/mmc/host/dw_mmc.c          | 46 +++++++++++++++++++++++++-------------
>  drivers/mmc/host/dw_mmc.h          |  6 ++---
>  include/linux/mmc/slot-gpio.h      |  1 +
>  9 files changed, 117 insertions(+), 106 deletions(-)
>

Overall these changes looks good to me, so I am ready to accept the PR
from Jaehoon!!


Although, highly related to this patchset, I am worried that there is
a misunderstanding on how MMC_PM_KEEP_POWER (DT binding
"keep-power-in-suspend") is being used for dw_mmc. Perhaps I am wrong,
but I would appreciate if you could elaborate a bit for my
understanding.

First, this cap is solely intended to be used for controllers which
may have SDIO cards attached, as it indicates those cards may be
configured to be powered on while the system enters suspend state. By
looking at some DTS files, for example
arch/arm64/boot/dts/rockchip/rk3368-orion-r68-meta.dts which uses it
for an eMMC slot, this is clearly being abused.

Anyway, the wrong DT configurations might not be a big deal, as in
dw_mci_resume(), it's not the capabilities bit that is checked but the
corresponding "pm_flag". This flag is set via calling
sdio_set_host_pm_flags(), but as that doesn't happen for an eMMC card
we should be fine, right!?

Now, what also do puzzles me, is exactly that piece of code in
dw_mci_resume() that is being executed *when* the pm_flag contains
MMC_PM_KEEP_POWER:
if (slot->mmc->pm_flags & MMC_PM_KEEP_POWER) {
     dw_mci_set_ios(slot->mmc, &slot->mmc->ios);
     dw_mci_setup_bus(slot, true);
}

So, in the system resume path, the above do makes sense as you need to
restore the registers etc for the dw_mmc controller to enable it to
operate the SDIO card. Such as bus width, clocks, etc.

Although, I would expect something similar would be needed in the new
runtime resume path as well. And in particular also for eMMC/SD cards,
as you need to restore the dw_mmc registers to be able to operate the
card again. Don't you?

So in the end, perhaps you should *always* call dw_mci_set_ios() and
dw_mci_setup_bus() in dw_mci_resume() instead of conditionally check
for MMC_PM_KEEP_POWER? Or maybe only a subset of those functions?

Kind regards
Uffe

Re: [PATCH v2 0/9] Init runtime PM support for dw_mmc

[Jaehoon Chung]

On 10/18/2016 05:46 PM, Ulf Hansson wrote:
> + Heiko
> 
> On 12 October 2016 at 04:50, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>
>> Hi Jaehoon and Ulf,
>>
>>    This patch is gonna support runtime PM for dw_mmc.
>> It could support to disable ciu_clk by default and disable
>> biu_clk if the devices are non-removeable, or removeable
>> with gpio-base card detect.
>>
>>    Then I remove the system PM since the runtime PM actually
>> does the same thing as it. So I help migrate the dw_mmc variant
>> drivers to use runtime PM pairs and pm_runtime_force_*. Note
>> that I only enable runtime PM for dw_mmc-rockchip as I will
>> leave the decision to the owners of the corresponding drivers.
>> I just tested it on my RK3288 platform with linux-next to make
>> the runtime PM and system PM work fine for my emmc, sd card and
>> sdio. But I don't have hardware to help test other variant drivers.
>> But in theory it should work fine as I mentioned that the runtime
>> PM does the same thing as system PM except for disabling ciu_clk
>> aggressively which should not be related to the variant hosts.
>>
>>    As you could see that I just extend the slot-gpio a bit, so the
>> ideal way is Ulf could pick them up with Jaehoon's ack. :)
> 
> The mmc core change looks fine to me, so I will wait for a pull
> request from Jaehoon.
> 
>>
>>
>> Changes in v2:
>> - use struct device as argument for runtime callback
>> - use dw_mci_runtime_* directly
>> - use dw_mci_runtime_* directly
>> - minor fix since I change the argument for dw_mci_runtime_*
>> - use dw_mci_runtime_* directly
>> - use dw_mci_runtime_* directly
>>
>> Shawn Lin (9):
>>   mmc: dw_mmc: add runtime PM callback
>>   mmc: dw_mmc-rockchip: add runtime PM support
>>   mmc: core: expose the capability of gpio card detect
>>   mmc: dw_mmc: disable biu clk if possible
>>   mmc: dw_mmc-k3: deploy runtime PM facilities
>>   mmc: dw_mmc-exynos: deploy runtime PM facilities
>>   mmc: dw_mmc-pci: deploy runtime PM facilities
>>   mmc: dw_mmc-pltfm: deploy runtime PM facilities
>>   mmc: dw_mmc: remove system PM callback
>>
>>  drivers/mmc/core/slot-gpio.c       |  8 +++++++
>>  drivers/mmc/host/dw_mmc-exynos.c   | 24 +++++++++-----------
>>  drivers/mmc/host/dw_mmc-k3.c       | 39 ++++++++------------------------
>>  drivers/mmc/host/dw_mmc-pci.c      | 29 ++++++++----------------
>>  drivers/mmc/host/dw_mmc-pltfm.c    | 28 +++++++----------------
>>  drivers/mmc/host/dw_mmc-rockchip.c | 42 +++++++++++++++++++++++++++++++---
>>  drivers/mmc/host/dw_mmc.c          | 46 +++++++++++++++++++++++++-------------
>>  drivers/mmc/host/dw_mmc.h          |  6 ++---
>>  include/linux/mmc/slot-gpio.h      |  1 +
>>  9 files changed, 117 insertions(+), 106 deletions(-)
>>
> 
> Overall these changes looks good to me, so I am ready to accept the PR
> from Jaehoon!!
> 

Yes..I will apply on my repository..i'm testing on dwmmc-testing branch..
Actually, i found the some blocking issue but it's not relevant to these patches.
after checking the stress test until today..(eMMC/SD/SDIO)..I will do.

Entire patches looks good to me..thanks for Shawn's effort! :)

> 
> Although, highly related to this patchset, I am worried that there is
> a misunderstanding on how MMC_PM_KEEP_POWER (DT binding
> "keep-power-in-suspend") is being used for dw_mmc. Perhaps I am wrong,
> but I would appreciate if you could elaborate a bit for my
> understanding.
> 
> First, this cap is solely intended to be used for controllers which
> may have SDIO cards attached, as it indicates those cards may be
> configured to be powered on while the system enters suspend state. By
> looking at some DTS files, for example
> arch/arm64/boot/dts/rockchip/rk3368-orion-r68-meta.dts which uses it
> for an eMMC slot, this is clearly being abused.

Right..it doesn't need to uses "keep-power-in-suspend" for eMMC/SD.

> 
> Anyway, the wrong DT configurations might not be a big deal, as in
> dw_mci_resume(), it's not the capabilities bit that is checked but the
> corresponding "pm_flag". This flag is set via calling
> sdio_set_host_pm_flags(), but as that doesn't happen for an eMMC card
> we should be fine, right!?
> 
> Now, what also do puzzles me, is exactly that piece of code in
> dw_mci_resume() that is being executed *when* the pm_flag contains
> MMC_PM_KEEP_POWER:
> if (slot->mmc->pm_flags & MMC_PM_KEEP_POWER) {
>      dw_mci_set_ios(slot->mmc, &slot->mmc->ios);
>      dw_mci_setup_bus(slot, true);
> }

There is no place to set this in dw_mmc controller..Maybe it should be located in other vendor driver.
Almost all cards that used SDIO might be WiFi..Of course..there might be other cards.
e.g) In exnyos, use the sdio as WiFi card(broadcom chipset) then broadcom driver also used..
When broadcom driver initialized or suspending time, it should be set to this flag..
As i mentioned..it's not normal case.

> 
> So, in the system resume path, the above do makes sense as you need to
> restore the registers etc for the dw_mmc controller to enable it to
> operate the SDIO card. Such as bus width, clocks, etc.
> 
> Although, I would expect something similar would be needed in the new
> runtime resume path as well. And in particular also for eMMC/SD cards,
> as you need to restore the dw_mmc registers to be able to operate the
> card again. Don't you?

Need to check :)

> 
> So in the end, perhaps you should *always* call dw_mci_set_ios() and
> dw_mci_setup_bus() in dw_mci_resume() instead of conditionally check
> for MMC_PM_KEEP_POWER? Or maybe only a subset of those functions?

It's reasonable..but it also needs to check on real case and stress test.

Best Regards,
Jaehoon Chung

> 
> Kind regards
> Uffe
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 

Re: [PATCH v2 0/9] Init runtime PM support for dw_mmc

[Ulf Hansson]

[...]

>>
>> Now, what also do puzzles me, is exactly that piece of code in
>> dw_mci_resume() that is being executed *when* the pm_flag contains
>> MMC_PM_KEEP_POWER:
>> if (slot->mmc->pm_flags & MMC_PM_KEEP_POWER) {
>>      dw_mci_set_ios(slot->mmc, &slot->mmc->ios);
>>      dw_mci_setup_bus(slot, true);
>> }
>
> There is no place to set this in dw_mmc controller..Maybe it should be located in other vendor driver.
> Almost all cards that used SDIO might be WiFi..Of course..there might be other cards.
> e.g) In exnyos, use the sdio as WiFi card(broadcom chipset) then broadcom driver also used..
> When broadcom driver initialized or suspending time, it should be set to this flag..
> As i mentioned..it's not normal case.
>
>>
>> So, in the system resume path, the above do makes sense as you need to
>> restore the registers etc for the dw_mmc controller to enable it to
>> operate the SDIO card. Such as bus width, clocks, etc.
>>
>> Although, I would expect something similar would be needed in the new
>> runtime resume path as well. And in particular also for eMMC/SD cards,
>> as you need to restore the dw_mmc registers to be able to operate the
>> card again. Don't you?
>
> Need to check :)
>
>>
>> So in the end, perhaps you should *always* call dw_mci_set_ios() and
>> dw_mci_setup_bus() in dw_mci_resume() instead of conditionally check
>> for MMC_PM_KEEP_POWER? Or maybe only a subset of those functions?
>
> It's reasonable..but it also needs to check on real case and stress test.

Yes, and we can try out that later on top of this series.

Anyway, I wanted to highlight my concern, as I think it's only a
matter of time before we needed this on for runtime PM for some SoCs
that uses dw_mmc.

Kind regards
Uffe

Re: [PATCH v2 0/9] Init runtime PM support for dw_mmc

[Shawn Lin]

在 2016/10/18 16:46, Ulf Hansson 写道:
> + Heiko
>
> On 12 October 2016 at 04:50, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>
>> Hi Jaehoon and Ulf,
>>
>>    This patch is gonna support runtime PM for dw_mmc.
>> It could support to disable ciu_clk by default and disable
>> biu_clk if the devices are non-removeable, or removeable
>> with gpio-base card detect.
>>
>>    Then I remove the system PM since the runtime PM actually
>> does the same thing as it. So I help migrate the dw_mmc variant
>> drivers to use runtime PM pairs and pm_runtime_force_*. Note
>> that I only enable runtime PM for dw_mmc-rockchip as I will
>> leave the decision to the owners of the corresponding drivers.
>> I just tested it on my RK3288 platform with linux-next to make
>> the runtime PM and system PM work fine for my emmc, sd card and
>> sdio. But I don't have hardware to help test other variant drivers.
>> But in theory it should work fine as I mentioned that the runtime
>> PM does the same thing as system PM except for disabling ciu_clk
>> aggressively which should not be related to the variant hosts.
>>
>>    As you could see that I just extend the slot-gpio a bit, so the
>> ideal way is Ulf could pick them up with Jaehoon's ack. :)
>
> The mmc core change looks fine to me, so I will wait for a pull
> request from Jaehoon.
>
>>
>>
>> Changes in v2:
>> - use struct device as argument for runtime callback
>> - use dw_mci_runtime_* directly
>> - use dw_mci_runtime_* directly
>> - minor fix since I change the argument for dw_mci_runtime_*
>> - use dw_mci_runtime_* directly
>> - use dw_mci_runtime_* directly
>>
>> Shawn Lin (9):
>>   mmc: dw_mmc: add runtime PM callback
>>   mmc: dw_mmc-rockchip: add runtime PM support
>>   mmc: core: expose the capability of gpio card detect
>>   mmc: dw_mmc: disable biu clk if possible
>>   mmc: dw_mmc-k3: deploy runtime PM facilities
>>   mmc: dw_mmc-exynos: deploy runtime PM facilities
>>   mmc: dw_mmc-pci: deploy runtime PM facilities
>>   mmc: dw_mmc-pltfm: deploy runtime PM facilities
>>   mmc: dw_mmc: remove system PM callback
>>
>>  drivers/mmc/core/slot-gpio.c       |  8 +++++++
>>  drivers/mmc/host/dw_mmc-exynos.c   | 24 +++++++++-----------
>>  drivers/mmc/host/dw_mmc-k3.c       | 39 ++++++++------------------------
>>  drivers/mmc/host/dw_mmc-pci.c      | 29 ++++++++----------------
>>  drivers/mmc/host/dw_mmc-pltfm.c    | 28 +++++++----------------
>>  drivers/mmc/host/dw_mmc-rockchip.c | 42 +++++++++++++++++++++++++++++++---
>>  drivers/mmc/host/dw_mmc.c          | 46 +++++++++++++++++++++++++-------------
>>  drivers/mmc/host/dw_mmc.h          |  6 ++---
>>  include/linux/mmc/slot-gpio.h      |  1 +
>>  9 files changed, 117 insertions(+), 106 deletions(-)
>>
>
> Overall these changes looks good to me, so I am ready to accept the PR
> from Jaehoon!!
>
>
> Although, highly related to this patchset, I am worried that there is
> a misunderstanding on how MMC_PM_KEEP_POWER (DT binding
> "keep-power-in-suspend") is being used for dw_mmc. Perhaps I am wrong,
> but I would appreciate if you could elaborate a bit for my
> understanding.
>
> First, this cap is solely intended to be used for controllers which
> may have SDIO cards attached, as it indicates those cards may be
> configured to be powered on while the system enters suspend state. By
> looking at some DTS files, for example
> arch/arm64/boot/dts/rockchip/rk3368-orion-r68-meta.dts which uses it
> for an eMMC slot, this is clearly being abused.

Indeed. In general, it should be copy-paste issues as folks maybe write
their dts referring to the exist dts there. So yes, I will do some 
cleanup work for them in prevent of further spread of abused code.

>
> Anyway, the wrong DT configurations might not be a big deal, as in
> dw_mci_resume(), it's not the capabilities bit that is checked but the
> corresponding "pm_flag". This flag is set via calling
> sdio_set_host_pm_flags(), but as that doesn't happen for an eMMC card
> we should be fine, right!?
>
> Now, what also do puzzles me, is exactly that piece of code in
> dw_mci_resume() that is being executed *when* the pm_flag contains
> MMC_PM_KEEP_POWER:
> if (slot->mmc->pm_flags & MMC_PM_KEEP_POWER) {
>      dw_mci_set_ios(slot->mmc, &slot->mmc->ios);
>      dw_mci_setup_bus(slot, true);
> }
>
> So, in the system resume path, the above do makes sense as you need to
> restore the registers etc for the dw_mmc controller to enable it to
> operate the SDIO card. Such as bus width, clocks, etc.
>
> Although, I would expect something similar would be needed in the new
> runtime resume path as well. And in particular also for eMMC/SD cards,
> as you need to restore the dw_mmc registers to be able to operate the
> card again. Don't you?

yes, we do.

>
> So in the end, perhaps you should *always* call dw_mci_set_ios() and
> dw_mci_setup_bus() in dw_mci_resume() instead of conditionally check
> for MMC_PM_KEEP_POWER? Or maybe only a subset of those functions?
>

Thanks for noticing this.

Personally, I realize there is no need to check MMC_PM_KEEP_POWER but
it could be highly related to the cost of S-2-R, I guess. I just checked
sdhci and saw the similar cases you mentioned at the first glance.
Maybe I'm wrong but I need more time to investigate this issue later.

There are still some on-going cleanup work for dw_mmc listed on my TODO
list, including bugfix, legacy/redundant code etc.. So I will check this 
one either. Maybe Jaehoon could also do some stree test on enxyos
platforms. :)


> Kind regards
> Uffe
>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH v2 0/9] Init runtime PM support for dw_mmc

[Heiko Stübner]

Am Dienstag, 18. Oktober 2016, 19:35:31 schrieb Shawn Lin:
> 在 2016/10/18 16:46, Ulf Hansson 写道:
> > + Heiko
> > 
> > On 12 October 2016 at 04:50, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> >> Hi Jaehoon and Ulf,
> >> 
> >>    This patch is gonna support runtime PM for dw_mmc.
> >> 
> >> It could support to disable ciu_clk by default and disable
> >> biu_clk if the devices are non-removeable, or removeable
> >> with gpio-base card detect.
> >> 
> >>    Then I remove the system PM since the runtime PM actually
> >> 
> >> does the same thing as it. So I help migrate the dw_mmc variant
> >> drivers to use runtime PM pairs and pm_runtime_force_*. Note
> >> that I only enable runtime PM for dw_mmc-rockchip as I will
> >> leave the decision to the owners of the corresponding drivers.
> >> I just tested it on my RK3288 platform with linux-next to make
> >> the runtime PM and system PM work fine for my emmc, sd card and
> >> sdio. But I don't have hardware to help test other variant drivers.
> >> But in theory it should work fine as I mentioned that the runtime
> >> PM does the same thing as system PM except for disabling ciu_clk
> >> aggressively which should not be related to the variant hosts.
> >> 
> >>    As you could see that I just extend the slot-gpio a bit, so the
> >> 
> >> ideal way is Ulf could pick them up with Jaehoon's ack. :)
> > 
> > The mmc core change looks fine to me, so I will wait for a pull
> > request from Jaehoon.
> > 
> >> Changes in v2:
> >> - use struct device as argument for runtime callback
> >> - use dw_mci_runtime_* directly
> >> - use dw_mci_runtime_* directly
> >> - minor fix since I change the argument for dw_mci_runtime_*
> >> - use dw_mci_runtime_* directly
> >> - use dw_mci_runtime_* directly
> >> 
> >> Shawn Lin (9):
> >>   mmc: dw_mmc: add runtime PM callback
> >>   mmc: dw_mmc-rockchip: add runtime PM support
> >>   mmc: core: expose the capability of gpio card detect
> >>   mmc: dw_mmc: disable biu clk if possible
> >>   mmc: dw_mmc-k3: deploy runtime PM facilities
> >>   mmc: dw_mmc-exynos: deploy runtime PM facilities
> >>   mmc: dw_mmc-pci: deploy runtime PM facilities
> >>   mmc: dw_mmc-pltfm: deploy runtime PM facilities
> >>   mmc: dw_mmc: remove system PM callback
> >>  
> >>  drivers/mmc/core/slot-gpio.c       |  8 +++++++
> >>  drivers/mmc/host/dw_mmc-exynos.c   | 24 +++++++++-----------
> >>  drivers/mmc/host/dw_mmc-k3.c       | 39 ++++++++------------------------
> >>  drivers/mmc/host/dw_mmc-pci.c      | 29 ++++++++----------------
> >>  drivers/mmc/host/dw_mmc-pltfm.c    | 28 +++++++----------------
> >>  drivers/mmc/host/dw_mmc-rockchip.c | 42
> >>  +++++++++++++++++++++++++++++++---
> >>  drivers/mmc/host/dw_mmc.c          | 46
> >>  +++++++++++++++++++++++++------------- drivers/mmc/host/dw_mmc.h       
> >>    |  6 ++---
> >>  include/linux/mmc/slot-gpio.h      |  1 +
> >>  9 files changed, 117 insertions(+), 106 deletions(-)
> > 
> > Overall these changes looks good to me, so I am ready to accept the PR
> > from Jaehoon!!
> > 
> > 
> > Although, highly related to this patchset, I am worried that there is
> > a misunderstanding on how MMC_PM_KEEP_POWER (DT binding
> > "keep-power-in-suspend") is being used for dw_mmc. Perhaps I am wrong,
> > but I would appreciate if you could elaborate a bit for my
> > understanding.
> > 
> > First, this cap is solely intended to be used for controllers which
> > may have SDIO cards attached, as it indicates those cards may be
> > configured to be powered on while the system enters suspend state. By
> > looking at some DTS files, for example
> > arch/arm64/boot/dts/rockchip/rk3368-orion-r68-meta.dts which uses it
> > for an eMMC slot, this is clearly being abused.
> 
> Indeed. In general, it should be copy-paste issues as folks maybe write
> their dts referring to the exist dts there. So yes, I will do some
> cleanup work for them in prevent of further spread of abused code.

late to the party, but a copy-paste mistake were my thoughts as well.
Thanks Shawn for providing the patch cleaning that up.


Heiko


> > Anyway, the wrong DT configurations might not be a big deal, as in
> > dw_mci_resume(), it's not the capabilities bit that is checked but the
> > corresponding "pm_flag". This flag is set via calling
> > sdio_set_host_pm_flags(), but as that doesn't happen for an eMMC card
> > we should be fine, right!?
> > 
> > Now, what also do puzzles me, is exactly that piece of code in
> > dw_mci_resume() that is being executed *when* the pm_flag contains
> > MMC_PM_KEEP_POWER:
> > if (slot->mmc->pm_flags & MMC_PM_KEEP_POWER) {
> > 
> >      dw_mci_set_ios(slot->mmc, &slot->mmc->ios);
> >      dw_mci_setup_bus(slot, true);
> > 
> > }
> > 
> > So, in the system resume path, the above do makes sense as you need to
> > restore the registers etc for the dw_mmc controller to enable it to
> > operate the SDIO card. Such as bus width, clocks, etc.
> > 
> > Although, I would expect something similar would be needed in the new
> > runtime resume path as well. And in particular also for eMMC/SD cards,
> > as you need to restore the dw_mmc registers to be able to operate the
> > card again. Don't you?
> 
> yes, we do.
> 
> > So in the end, perhaps you should *always* call dw_mci_set_ios() and
> > dw_mci_setup_bus() in dw_mci_resume() instead of conditionally check
> > for MMC_PM_KEEP_POWER? Or maybe only a subset of those functions?
> 
> Thanks for noticing this.
> 
> Personally, I realize there is no need to check MMC_PM_KEEP_POWER but
> it could be highly related to the cost of S-2-R, I guess. I just checked
> sdhci and saw the similar cases you mentioned at the first glance.
> Maybe I'm wrong but I need more time to investigate this issue later.
> 
> There are still some on-going cleanup work for dw_mmc listed on my TODO
> list, including bugfix, legacy/redundant code etc.. So I will check this
> one either. Maybe Jaehoon could also do some stree test on enxyos
> platforms. :)
> 
> > Kind regards
> > Uffe

Re: [PATCH v2 0/9] Init runtime PM support for dw_mmc

[Jaehoon Chung]

Hi Shawn,

On 10/18/2016 08:35 PM, Shawn Lin wrote:
> 在 2016/10/18 16:46, Ulf Hansson 写道:
>> + Heiko
>>
>> On 12 October 2016 at 04:50, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>>
>>> Hi Jaehoon and Ulf,

I applied this on my repository. If there are any issue, let me know.
And minor issue can be fixed..step by step.

Best Regards,
Jaehoon Chung

>>>
>>>    This patch is gonna support runtime PM for dw_mmc.
>>> It could support to disable ciu_clk by default and disable
>>> biu_clk if the devices are non-removeable, or removeable
>>> with gpio-base card detect.
>>>
>>>    Then I remove the system PM since the runtime PM actually
>>> does the same thing as it. So I help migrate the dw_mmc variant
>>> drivers to use runtime PM pairs and pm_runtime_force_*. Note
>>> that I only enable runtime PM for dw_mmc-rockchip as I will
>>> leave the decision to the owners of the corresponding drivers.
>>> I just tested it on my RK3288 platform with linux-next to make
>>> the runtime PM and system PM work fine for my emmc, sd card and
>>> sdio. But I don't have hardware to help test other variant drivers.
>>> But in theory it should work fine as I mentioned that the runtime
>>> PM does the same thing as system PM except for disabling ciu_clk
>>> aggressively which should not be related to the variant hosts.
>>>
>>>    As you could see that I just extend the slot-gpio a bit, so the
>>> ideal way is Ulf could pick them up with Jaehoon's ack. :)
>>
>> The mmc core change looks fine to me, so I will wait for a pull
>> request from Jaehoon.
>>
>>>
>>>
>>> Changes in v2:
>>> - use struct device as argument for runtime callback
>>> - use dw_mci_runtime_* directly
>>> - use dw_mci_runtime_* directly
>>> - minor fix since I change the argument for dw_mci_runtime_*
>>> - use dw_mci_runtime_* directly
>>> - use dw_mci_runtime_* directly
>>>
>>> Shawn Lin (9):
>>>   mmc: dw_mmc: add runtime PM callback
>>>   mmc: dw_mmc-rockchip: add runtime PM support
>>>   mmc: core: expose the capability of gpio card detect
>>>   mmc: dw_mmc: disable biu clk if possible
>>>   mmc: dw_mmc-k3: deploy runtime PM facilities
>>>   mmc: dw_mmc-exynos: deploy runtime PM facilities
>>>   mmc: dw_mmc-pci: deploy runtime PM facilities
>>>   mmc: dw_mmc-pltfm: deploy runtime PM facilities
>>>   mmc: dw_mmc: remove system PM callback
>>>
>>>  drivers/mmc/core/slot-gpio.c       |  8 +++++++
>>>  drivers/mmc/host/dw_mmc-exynos.c   | 24 +++++++++-----------
>>>  drivers/mmc/host/dw_mmc-k3.c       | 39 ++++++++------------------------
>>>  drivers/mmc/host/dw_mmc-pci.c      | 29 ++++++++----------------
>>>  drivers/mmc/host/dw_mmc-pltfm.c    | 28 +++++++----------------
>>>  drivers/mmc/host/dw_mmc-rockchip.c | 42 +++++++++++++++++++++++++++++++---
>>>  drivers/mmc/host/dw_mmc.c          | 46 +++++++++++++++++++++++++-------------
>>>  drivers/mmc/host/dw_mmc.h          |  6 ++---
>>>  include/linux/mmc/slot-gpio.h      |  1 +
>>>  9 files changed, 117 insertions(+), 106 deletions(-)
>>>
>>
>> Overall these changes looks good to me, so I am ready to accept the PR
>> from Jaehoon!!
>>
>>
>> Although, highly related to this patchset, I am worried that there is
>> a misunderstanding on how MMC_PM_KEEP_POWER (DT binding
>> "keep-power-in-suspend") is being used for dw_mmc. Perhaps I am wrong,
>> but I would appreciate if you could elaborate a bit for my
>> understanding.
>>
>> First, this cap is solely intended to be used for controllers which
>> may have SDIO cards attached, as it indicates those cards may be
>> configured to be powered on while the system enters suspend state. By
>> looking at some DTS files, for example
>> arch/arm64/boot/dts/rockchip/rk3368-orion-r68-meta.dts which uses it
>> for an eMMC slot, this is clearly being abused.
> 
> Indeed. In general, it should be copy-paste issues as folks maybe write
> their dts referring to the exist dts there. So yes, I will do some cleanup work for them in prevent of further spread of abused code.
> 
>>
>> Anyway, the wrong DT configurations might not be a big deal, as in
>> dw_mci_resume(), it's not the capabilities bit that is checked but the
>> corresponding "pm_flag". This flag is set via calling
>> sdio_set_host_pm_flags(), but as that doesn't happen for an eMMC card
>> we should be fine, right!?
>>
>> Now, what also do puzzles me, is exactly that piece of code in
>> dw_mci_resume() that is being executed *when* the pm_flag contains
>> MMC_PM_KEEP_POWER:
>> if (slot->mmc->pm_flags & MMC_PM_KEEP_POWER) {
>>      dw_mci_set_ios(slot->mmc, &slot->mmc->ios);
>>      dw_mci_setup_bus(slot, true);
>> }
>>
>> So, in the system resume path, the above do makes sense as you need to
>> restore the registers etc for the dw_mmc controller to enable it to
>> operate the SDIO card. Such as bus width, clocks, etc.
>>
>> Although, I would expect something similar would be needed in the new
>> runtime resume path as well. And in particular also for eMMC/SD cards,
>> as you need to restore the dw_mmc registers to be able to operate the
>> card again. Don't you?
> 
> yes, we do.
> 
>>
>> So in the end, perhaps you should *always* call dw_mci_set_ios() and
>> dw_mci_setup_bus() in dw_mci_resume() instead of conditionally check
>> for MMC_PM_KEEP_POWER? Or maybe only a subset of those functions?
>>
> 
> Thanks for noticing this.
> 
> Personally, I realize there is no need to check MMC_PM_KEEP_POWER but
> it could be highly related to the cost of S-2-R, I guess. I just checked
> sdhci and saw the similar cases you mentioned at the first glance.
> Maybe I'm wrong but I need more time to investigate this issue later.
> 
> There are still some on-going cleanup work for dw_mmc listed on my TODO
> list, including bugfix, legacy/redundant code etc.. So I will check this one either. Maybe Jaehoon could also do some stree test on enxyos
> platforms. :)
> 
> 
>> Kind regards
>> Uffe
>>
>>
>>
> 
> 

Re: [PATCH v2 0/9] Init runtime PM support for dw_mmc

[Shawn Lin]

Hi Jaehoon,

On 2016/10/21 9:56, Jaehoon Chung wrote:
> Hi Shawn,
>
> On 10/18/2016 08:35 PM, Shawn Lin wrote:
>> 在 2016/10/18 16:46, Ulf Hansson 写道:
>>> + Heiko
>>>
>>> On 12 October 2016 at 04:50, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>>>
>>>> Hi Jaehoon and Ulf,
>
> I applied this on my repository. If there are any issue, let me know.
> And minor issue can be fixed..step by step.

I was checking your repository and there is no doubt for them.

Thanks a lot!

>
> Best Regards,
> Jaehoon Chung
>
>>>>
>>>>    This patch is gonna support runtime PM for dw_mmc.
>>>> It could support to disable ciu_clk by default and disable
>>>> biu_clk if the devices are non-removeable, or removeable
>>>> with gpio-base card detect.
>>>>
>>>>    Then I remove the system PM since the runtime PM actually
>>>> does the same thing as it. So I help migrate the dw_mmc variant
>>>> drivers to use runtime PM pairs and pm_runtime_force_*. Note
>>>> that I only enable runtime PM for dw_mmc-rockchip as I will
>>>> leave the decision to the owners of the corresponding drivers.
>>>> I just tested it on my RK3288 platform with linux-next to make
>>>> the runtime PM and system PM work fine for my emmc, sd card and
>>>> sdio. But I don't have hardware to help test other variant drivers.
>>>> But in theory it should work fine as I mentioned that the runtime
>>>> PM does the same thing as system PM except for disabling ciu_clk
>>>> aggressively which should not be related to the variant hosts.
>>>>
>>>>    As you could see that I just extend the slot-gpio a bit, so the
>>>> ideal way is Ulf could pick them up with Jaehoon's ack. :)
>>>
>>> The mmc core change looks fine to me, so I will wait for a pull
>>> request from Jaehoon.
>>>
>>>>
>>>>
>>>> Changes in v2:
>>>> - use struct device as argument for runtime callback
>>>> - use dw_mci_runtime_* directly
>>>> - use dw_mci_runtime_* directly
>>>> - minor fix since I change the argument for dw_mci_runtime_*
>>>> - use dw_mci_runtime_* directly
>>>> - use dw_mci_runtime_* directly
>>>>
>>>> Shawn Lin (9):
>>>>   mmc: dw_mmc: add runtime PM callback
>>>>   mmc: dw_mmc-rockchip: add runtime PM support
>>>>   mmc: core: expose the capability of gpio card detect
>>>>   mmc: dw_mmc: disable biu clk if possible
>>>>   mmc: dw_mmc-k3: deploy runtime PM facilities
>>>>   mmc: dw_mmc-exynos: deploy runtime PM facilities
>>>>   mmc: dw_mmc-pci: deploy runtime PM facilities
>>>>   mmc: dw_mmc-pltfm: deploy runtime PM facilities
>>>>   mmc: dw_mmc: remove system PM callback
>>>>
>>>>  drivers/mmc/core/slot-gpio.c       |  8 +++++++
>>>>  drivers/mmc/host/dw_mmc-exynos.c   | 24 +++++++++-----------
>>>>  drivers/mmc/host/dw_mmc-k3.c       | 39 ++++++++------------------------
>>>>  drivers/mmc/host/dw_mmc-pci.c      | 29 ++++++++----------------
>>>>  drivers/mmc/host/dw_mmc-pltfm.c    | 28 +++++++----------------
>>>>  drivers/mmc/host/dw_mmc-rockchip.c | 42 +++++++++++++++++++++++++++++++---
>>>>  drivers/mmc/host/dw_mmc.c          | 46 +++++++++++++++++++++++++-------------
>>>>  drivers/mmc/host/dw_mmc.h          |  6 ++---
>>>>  include/linux/mmc/slot-gpio.h      |  1 +
>>>>  9 files changed, 117 insertions(+), 106 deletions(-)
>>>>
>>>
>>> Overall these changes looks good to me, so I am ready to accept the PR
>>> from Jaehoon!!
>>>
>>>
>>> Although, highly related to this patchset, I am worried that there is
>>> a misunderstanding on how MMC_PM_KEEP_POWER (DT binding
>>> "keep-power-in-suspend") is being used for dw_mmc. Perhaps I am wrong,
>>> but I would appreciate if you could elaborate a bit for my
>>> understanding.
>>>
>>> First, this cap is solely intended to be used for controllers which
>>> may have SDIO cards attached, as it indicates those cards may be
>>> configured to be powered on while the system enters suspend state. By
>>> looking at some DTS files, for example
>>> arch/arm64/boot/dts/rockchip/rk3368-orion-r68-meta.dts which uses it
>>> for an eMMC slot, this is clearly being abused.
>>
>> Indeed. In general, it should be copy-paste issues as folks maybe write
>> their dts referring to the exist dts there. So yes, I will do some cleanup work for them in prevent of further spread of abused code.
>>
>>>
>>> Anyway, the wrong DT configurations might not be a big deal, as in
>>> dw_mci_resume(), it's not the capabilities bit that is checked but the
>>> corresponding "pm_flag". This flag is set via calling
>>> sdio_set_host_pm_flags(), but as that doesn't happen for an eMMC card
>>> we should be fine, right!?
>>>
>>> Now, what also do puzzles me, is exactly that piece of code in
>>> dw_mci_resume() that is being executed *when* the pm_flag contains
>>> MMC_PM_KEEP_POWER:
>>> if (slot->mmc->pm_flags & MMC_PM_KEEP_POWER) {
>>>      dw_mci_set_ios(slot->mmc, &slot->mmc->ios);
>>>      dw_mci_setup_bus(slot, true);
>>> }
>>>
>>> So, in the system resume path, the above do makes sense as you need to
>>> restore the registers etc for the dw_mmc controller to enable it to
>>> operate the SDIO card. Such as bus width, clocks, etc.
>>>
>>> Although, I would expect something similar would be needed in the new
>>> runtime resume path as well. And in particular also for eMMC/SD cards,
>>> as you need to restore the dw_mmc registers to be able to operate the
>>> card again. Don't you?
>>
>> yes, we do.
>>
>>>
>>> So in the end, perhaps you should *always* call dw_mci_set_ios() and
>>> dw_mci_setup_bus() in dw_mci_resume() instead of conditionally check
>>> for MMC_PM_KEEP_POWER? Or maybe only a subset of those functions?
>>>
>>
>> Thanks for noticing this.
>>
>> Personally, I realize there is no need to check MMC_PM_KEEP_POWER but
>> it could be highly related to the cost of S-2-R, I guess. I just checked
>> sdhci and saw the similar cases you mentioned at the first glance.
>> Maybe I'm wrong but I need more time to investigate this issue later.
>>
>> There are still some on-going cleanup work for dw_mmc listed on my TODO
>> list, including bugfix, legacy/redundant code etc.. So I will check this one either. Maybe Jaehoon could also do some stree test on enxyos
>> platforms. :)
>>
>>
>>> Kind regards
>>> Uffe
>>>
>>>
>>>
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
Best Regards
Shawn Lin