[PATCH v2 0/8] mmc: sunxi: Add runtime PM support

[PATCH v2 0/8] mmc: sunxi: Add runtime PM support

[Maxime Ripard]

Hi,

Since it's introduction, our MMC controller has had its external clocks
running all the time.

While that was working great, the power usage and most importantly the EMI
that it generated was pretty bad.

Let's implement some runtime_pm hooks with an autosuspend to shut down the
whole block when the MMC is not active.

However, some SDIO devices will not probe properly if we keep shutting down
the controller, so the autosuspend is disabled for the SDIO devices.

Let me know what you think,
Maxime

Changes from v1:
  - Disable entirely the controller
  - Made sure the driver would work with CONFIG_PM off
  - Fixed an issue with the SDIO devices
  - Rebased on top of 4.16

Maxime Ripard (8):
  mmc: sunxi: Reorder the headers
  mmc: sunxi: Move the power off action in a separate function
  mmc: sunxi: Move the power on action in a separate function
  mmc: sunxi: Move the power up action in a separate function
  mmc: sunxi: Move resources management to separate functions
  mmc: sunxi: Move the reset deassertion before enabling the clocks
  mmc: sunxi: Set our device drvdata earlier
  mmc: sunxi: Add runtime_pm support

 drivers/mmc/host/sunxi-mmc.c | 346 ++++++++++++++++++++++--------------
 1 file changed, 216 insertions(+), 130 deletions(-)

base-commit: 661e50bc853209e41a5c14a290ca4decc43cbfd1
-- 
git-series 0.9.1

[PATCH v2 1/8] mmc: sunxi: Reorder the headers

[Maxime Ripard]

Our headers sort algorithm has had pretty chaotic results. Let's fix that.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/mmc/host/sunxi-mmc.c | 43 +++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 23 deletions(-)

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index bad612d6f879..f9723797a9f8 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -13,36 +13,33 @@
  * the License, or (at your option) any later version.
  */
 
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/io.h>
-#include <linux/device.h>
-#include <linux/interrupt.h>
-#include <linux/delay.h>
-#include <linux/err.h>
-
 #include <linux/clk.h>
 #include <linux/clk/sunxi-ng.h>
-#include <linux/gpio.h>
-#include <linux/platform_device.h>
-#include <linux/spinlock.h>
-#include <linux/scatterlist.h>
+#include <linux/delay.h>
+#include <linux/device.h>
 #include <linux/dma-mapping.h>
-#include <linux/slab.h>
-#include <linux/reset.h>
-#include <linux/regulator/consumer.h>
-
-#include <linux/of_address.h>
-#include <linux/of_gpio.h>
-#include <linux/of_platform.h>
-
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mmc/card.h>
+#include <linux/mmc/core.h>
+#include <linux/mmc/mmc.h>
 #include <linux/mmc/host.h>
 #include <linux/mmc/sd.h>
 #include <linux/mmc/sdio.h>
-#include <linux/mmc/mmc.h>
-#include <linux/mmc/core.h>
-#include <linux/mmc/card.h>
 #include <linux/mmc/slot-gpio.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_gpio.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/reset.h>
+#include <linux/scatterlist.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
 
 /* register offset definitions */
 #define SDXC_REG_GCTRL	(0x00) /* SMC Global Control Register */
-- 
git-series 0.9.1

[PATCH v2 2/8] mmc: sunxi: Move the power off action in a separate function

[Maxime Ripard]

We'll need to have the power off behaviour in order to implement
runtime_pm. Move it outside of the .set_ios callback for an easier access.

And it improves readibility as a bonus.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/mmc/host/sunxi-mmc.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index f9723797a9f8..a53e39d8b07e 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -855,6 +855,22 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host,
 	return 0;
 }
 
+static void sunxi_mmc_power_off(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+	struct sunxi_mmc_host *host = mmc_priv(mmc);
+
+	dev_dbg(mmc_dev(mmc), "Powering off\n");
+
+	sunxi_mmc_reset_host(host);
+	if (!IS_ERR(mmc->supply.vmmc))
+		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0);
+
+	if (!IS_ERR(mmc->supply.vqmmc) && host->vqmmc_enabled)
+		regulator_disable(mmc->supply.vqmmc);
+
+	host->vqmmc_enabled = false;
+}
+
 static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct sunxi_mmc_host *host = mmc_priv(mmc);
@@ -892,14 +908,7 @@ static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		break;
 
 	case MMC_POWER_OFF:
-		dev_dbg(mmc_dev(mmc), "power off!\n");
-		sunxi_mmc_reset_host(host);
-		if (!IS_ERR(mmc->supply.vmmc))
-			mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0);
-
-		if (!IS_ERR(mmc->supply.vqmmc) && host->vqmmc_enabled)
-			regulator_disable(mmc->supply.vqmmc);
-		host->vqmmc_enabled = false;
+		sunxi_mmc_power_off(mmc, ios);
 		break;
 	}
 
-- 
git-series 0.9.1

[PATCH v2 3/8] mmc: sunxi: Move the power on action in a separate function

[Maxime Ripard]

We'll need to have the power on behaviour in order to implement runtime_pm.
Move it outside of the .set_ios callback for an easier access.

And it improves readibility as a bonus.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/mmc/host/sunxi-mmc.c | 62 ++++++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 28 deletions(-)

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index a53e39d8b07e..4e81fe171532 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -855,6 +855,39 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host,
 	return 0;
 }
 
+static void sunxi_mmc_power_on(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+	struct sunxi_mmc_host *host = mmc_priv(mmc);
+	u32 rval;
+
+	dev_dbg(mmc_dev(mmc), "Powering on\n");
+
+	/* set bus width */
+	switch (ios->bus_width) {
+	case MMC_BUS_WIDTH_1:
+		mmc_writel(host, REG_WIDTH, SDXC_WIDTH1);
+		break;
+	case MMC_BUS_WIDTH_4:
+		mmc_writel(host, REG_WIDTH, SDXC_WIDTH4);
+		break;
+	case MMC_BUS_WIDTH_8:
+		mmc_writel(host, REG_WIDTH, SDXC_WIDTH8);
+		break;
+	}
+
+	/* set ddr mode */
+	rval = mmc_readl(host, REG_GCTRL);
+	if (ios->timing == MMC_TIMING_UHS_DDR50 ||
+	    ios->timing == MMC_TIMING_MMC_DDR52)
+		rval |= SDXC_DDR_MODE;
+	else
+		rval &= ~SDXC_DDR_MODE;
+	mmc_writel(host, REG_GCTRL, rval);
+
+	host->ferror = sunxi_mmc_clk_set_rate(host, ios);
+	/* Android code had a usleep_range(50000, 55000); here */
+}
+
 static void sunxi_mmc_power_off(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct sunxi_mmc_host *host = mmc_priv(mmc);
@@ -874,7 +907,6 @@ static void sunxi_mmc_power_off(struct mmc_host *mmc, struct mmc_ios *ios)
 static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct sunxi_mmc_host *host = mmc_priv(mmc);
-	u32 rval;
 
 	/* Set the power state */
 	switch (ios->power_mode) {
@@ -912,33 +944,7 @@ static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		break;
 	}
 
-	/* set bus width */
-	switch (ios->bus_width) {
-	case MMC_BUS_WIDTH_1:
-		mmc_writel(host, REG_WIDTH, SDXC_WIDTH1);
-		break;
-	case MMC_BUS_WIDTH_4:
-		mmc_writel(host, REG_WIDTH, SDXC_WIDTH4);
-		break;
-	case MMC_BUS_WIDTH_8:
-		mmc_writel(host, REG_WIDTH, SDXC_WIDTH8);
-		break;
-	}
-
-	/* set ddr mode */
-	rval = mmc_readl(host, REG_GCTRL);
-	if (ios->timing == MMC_TIMING_UHS_DDR50 ||
-	    ios->timing == MMC_TIMING_MMC_DDR52)
-		rval |= SDXC_DDR_MODE;
-	else
-		rval &= ~SDXC_DDR_MODE;
-	mmc_writel(host, REG_GCTRL, rval);
-
-	/* set up clock */
-	if (ios->power_mode) {
-		host->ferror = sunxi_mmc_clk_set_rate(host, ios);
-		/* Android code had a usleep_range(50000, 55000); here */
-	}
+	sunxi_mmc_power_on(mmc, ios);
 }
 
 static int sunxi_mmc_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios)
-- 
git-series 0.9.1

[PATCH v2 4/8] mmc: sunxi: Move the power up action in a separate function

[Maxime Ripard]

We'll need to have the power up behaviour in order to implement runtime_pm.
Move it outside of the .set_ios callback for an easier access.

And it improves readibility as a bonus.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/mmc/host/sunxi-mmc.c | 60 ++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 4e81fe171532..e4b44c4adb8f 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -888,6 +888,37 @@ static void sunxi_mmc_power_on(struct mmc_host *mmc, struct mmc_ios *ios)
 	/* Android code had a usleep_range(50000, 55000); here */
 }
 
+static void sunxi_mmc_power_up(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+	struct sunxi_mmc_host *host = mmc_priv(mmc);
+
+	dev_dbg(mmc_dev(mmc), "Powering up\n");
+
+	if (!IS_ERR(mmc->supply.vmmc)) {
+		host->ferror = mmc_regulator_set_ocr(mmc,
+						     mmc->supply.vmmc,
+						     ios->vdd);
+		if (host->ferror)
+			return;
+	}
+
+	if (!IS_ERR(mmc->supply.vqmmc)) {
+		host->ferror = regulator_enable(mmc->supply.vqmmc);
+		if (host->ferror) {
+			dev_err(mmc_dev(mmc),
+				"failed to enable vqmmc\n");
+			return;
+		}
+		host->vqmmc_enabled = true;
+	}
+
+	host->ferror = sunxi_mmc_init_host(mmc);
+	if (host->ferror)
+		return;
+
+	sunxi_mmc_power_on(mmc, ios);
+}
+
 static void sunxi_mmc_power_off(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct sunxi_mmc_host *host = mmc_priv(mmc);
@@ -906,45 +937,20 @@ static void sunxi_mmc_power_off(struct mmc_host *mmc, struct mmc_ios *ios)
 
 static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
-	struct sunxi_mmc_host *host = mmc_priv(mmc);
-
 	/* Set the power state */
 	switch (ios->power_mode) {
 	case MMC_POWER_ON:
+		sunxi_mmc_power_on(mmc, ios);
 		break;
 
 	case MMC_POWER_UP:
-		if (!IS_ERR(mmc->supply.vmmc)) {
-			host->ferror = mmc_regulator_set_ocr(mmc,
-							     mmc->supply.vmmc,
-							     ios->vdd);
-			if (host->ferror)
-				return;
-		}
-
-		if (!IS_ERR(mmc->supply.vqmmc)) {
-			host->ferror = regulator_enable(mmc->supply.vqmmc);
-			if (host->ferror) {
-				dev_err(mmc_dev(mmc),
-					"failed to enable vqmmc\n");
-				return;
-			}
-			host->vqmmc_enabled = true;
-		}
-
-		host->ferror = sunxi_mmc_init_host(mmc);
-		if (host->ferror)
-			return;
-
-		dev_dbg(mmc_dev(mmc), "power on!\n");
+		sunxi_mmc_power_up(mmc, ios);
 		break;
 
 	case MMC_POWER_OFF:
 		sunxi_mmc_power_off(mmc, ios);
 		break;
 	}
-
-	sunxi_mmc_power_on(mmc, ios);
 }
 
 static int sunxi_mmc_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios)
-- 
git-series 0.9.1

[PATCH v2 5/8] mmc: sunxi: Move resources management to separate functions

[Maxime Ripard]

We've had all our resources management, and especially the clocks and reset
sequence, done directly as part of the probe.

As we want to implement runtime_pm, we'll obviously want to have that
moved outside of the probe so that we can call do it in our runtime suspend
and resume hooks without too much duplication.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/mmc/host/sunxi-mmc.c | 142 ++++++++++++++++++++----------------
 1 file changed, 82 insertions(+), 60 deletions(-)

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index e4b44c4adb8f..34bc90bc04b2 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -265,6 +265,7 @@ struct sunxi_mmc_cfg {
 };
 
 struct sunxi_mmc_host {
+	struct device *dev;
 	struct mmc_host	*mmc;
 	struct reset_control *reset;
 	const struct sunxi_mmc_cfg *cfg;
@@ -1183,6 +1184,80 @@ static const struct of_device_id sunxi_mmc_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
 
+static int sunxi_mmc_enable(struct sunxi_mmc_host *host)
+{
+	int ret;
+
+	ret = clk_prepare_enable(host->clk_ahb);
+	if (ret) {
+		dev_err(dev, "Couldn't enable the bus clocks (%d)\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(host->clk_mmc);
+	if (ret) {
+		dev_err(host->dev, "Enable mmc clk err %d\n", ret);
+		goto error_disable_clk_ahb;
+	}
+
+	ret = clk_prepare_enable(host->clk_output);
+	if (ret) {
+		dev_err(host->dev, "Enable output clk err %d\n", ret);
+		goto error_disable_clk_mmc;
+	}
+
+	ret = clk_prepare_enable(host->clk_sample);
+	if (ret) {
+		dev_err(host->dev, "Enable sample clk err %d\n", ret);
+		goto error_disable_clk_output;
+	}
+
+	if (!IS_ERR(host->reset)) {
+		ret = reset_control_reset(host->reset);
+		if (ret) {
+			dev_err(dev, "Couldn't reset the MMC controller (%d)\n",
+				ret);
+			goto error_disable_clk_sample;
+		}
+	}
+
+	/*
+	 * Sometimes the controller asserts the irq on boot for some reason,
+	 * make sure the controller is in a sane state before enabling irqs.
+	 */
+	ret = sunxi_mmc_reset_host(host);
+	if (ret)
+		goto error_assert_reset;
+
+	return 0;
+
+error_assert_reset:
+	if (!IS_ERR(host->reset))
+		reset_control_assert(host->reset);
+error_disable_clk_sample:
+	clk_disable_unprepare(host->clk_sample);
+error_disable_clk_output:
+	clk_disable_unprepare(host->clk_output);
+error_disable_clk_mmc:
+	clk_disable_unprepare(host->clk_mmc);
+error_disable_clk_ahb:
+	clk_disable_unprepare(host->clk_ahb);
+	return ret;
+}
+
+static void sunxi_mmc_disable(struct sunxi_mmc_host *host)
+{
+	sunxi_mmc_reset_host(host);
+
+	if (!IS_ERR(host->reset))
+		reset_control_assert(host->reset);
+
+	clk_disable_unprepare(host->clk_sample);
+	clk_disable_unprepare(host->clk_output);
+	clk_disable_unprepare(host->clk_mmc);
+	clk_disable_unprepare(host->clk_ahb);
+}
+
 static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
 				      struct platform_device *pdev)
 {
@@ -1232,66 +1307,21 @@ static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
 	if (PTR_ERR(host->reset) == -EPROBE_DEFER)
 		return PTR_ERR(host->reset);
 
-	ret = clk_prepare_enable(host->clk_ahb);
-	if (ret) {
-		dev_err(&pdev->dev, "Enable ahb clk err %d\n", ret);
-		return ret;
-	}
-
-	ret = clk_prepare_enable(host->clk_mmc);
-	if (ret) {
-		dev_err(&pdev->dev, "Enable mmc clk err %d\n", ret);
-		goto error_disable_clk_ahb;
-	}
-
-	ret = clk_prepare_enable(host->clk_output);
-	if (ret) {
-		dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
-		goto error_disable_clk_mmc;
-	}
-
-	ret = clk_prepare_enable(host->clk_sample);
-	if (ret) {
-		dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
-		goto error_disable_clk_output;
-	}
-
-	if (!IS_ERR(host->reset)) {
-		ret = reset_control_reset(host->reset);
-		if (ret) {
-			dev_err(&pdev->dev, "reset err %d\n", ret);
-			goto error_disable_clk_sample;
-		}
-	}
-
-	/*
-	 * Sometimes the controller asserts the irq on boot for some reason,
-	 * make sure the controller is in a sane state before enabling irqs.
-	 */
-	ret = sunxi_mmc_reset_host(host);
+	ret = sunxi_mmc_enable(host);
 	if (ret)
-		goto error_assert_reset;
+		return ret;
 
 	host->irq = platform_get_irq(pdev, 0);
 	if (host->irq <= 0) {
 		ret = -EINVAL;
-		goto error_assert_reset;
+		goto error_disable_mmc;
 	}
 
 	return devm_request_threaded_irq(&pdev->dev, host->irq, sunxi_mmc_irq,
 			sunxi_mmc_handle_manual_stop, 0, "sunxi-mmc", host);
 
-error_assert_reset:
-	if (!IS_ERR(host->reset))
-		reset_control_assert(host->reset);
-error_disable_clk_sample:
-	clk_disable_unprepare(host->clk_sample);
-error_disable_clk_output:
-	clk_disable_unprepare(host->clk_output);
-error_disable_clk_mmc:
-	clk_disable_unprepare(host->clk_mmc);
-error_disable_clk_ahb:
-	clk_disable_unprepare(host->clk_ahb);
+error_disable_mmc:
+	sunxi_mmc_disable(host);
 	return ret;
 }
 
@@ -1308,6 +1338,7 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
 	}
 
 	host = mmc_priv(mmc);
+	host->dev = &pdev->dev;
 	host->mmc = mmc;
 	spin_lock_init(&host->lock);
 
@@ -1388,16 +1419,7 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
 
 	mmc_remove_host(mmc);
 	disable_irq(host->irq);
-	sunxi_mmc_reset_host(host);
-
-	if (!IS_ERR(host->reset))
-		reset_control_assert(host->reset);
-
-	clk_disable_unprepare(host->clk_sample);
-	clk_disable_unprepare(host->clk_output);
-	clk_disable_unprepare(host->clk_mmc);
-	clk_disable_unprepare(host->clk_ahb);
-
+	sunxi_mmc_disable(host);
 	dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma);
 	mmc_free_host(mmc);
 
-- 
git-series 0.9.1

[PATCH v2 6/8] mmc: sunxi: Move the reset deassertion before enabling the clocks

[Maxime Ripard]

As per Allwinner guidelines, the reset line should be deasserted before
turning the clocks on.

Implement it in our driver as well.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/mmc/host/sunxi-mmc.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 34bc90bc04b2..c6431f6e816f 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -1188,10 +1188,19 @@ static int sunxi_mmc_enable(struct sunxi_mmc_host *host)
 {
 	int ret;
 
+	if (!IS_ERR(host->reset)) {
+		ret = reset_control_reset(host->reset);
+		if (ret) {
+			dev_err(host->dev, "Couldn't reset the MMC controller (%d)\n",
+				ret);
+			return ret;
+		}
+	}
+
 	ret = clk_prepare_enable(host->clk_ahb);
 	if (ret) {
-		dev_err(dev, "Couldn't enable the bus clocks (%d)\n", ret);
-		return ret;
+		dev_err(host->dev, "Couldn't enable the bus clocks (%d)\n", ret);
+		goto error_assert_reset;
 	}
 
 	ret = clk_prepare_enable(host->clk_mmc);
@@ -1212,28 +1221,16 @@ static int sunxi_mmc_enable(struct sunxi_mmc_host *host)
 		goto error_disable_clk_output;
 	}
 
-	if (!IS_ERR(host->reset)) {
-		ret = reset_control_reset(host->reset);
-		if (ret) {
-			dev_err(dev, "Couldn't reset the MMC controller (%d)\n",
-				ret);
-			goto error_disable_clk_sample;
-		}
-	}
-
 	/*
 	 * Sometimes the controller asserts the irq on boot for some reason,
 	 * make sure the controller is in a sane state before enabling irqs.
 	 */
 	ret = sunxi_mmc_reset_host(host);
 	if (ret)
-		goto error_assert_reset;
+		goto error_disable_clk_sample;
 
 	return 0;
 
-error_assert_reset:
-	if (!IS_ERR(host->reset))
-		reset_control_assert(host->reset);
 error_disable_clk_sample:
 	clk_disable_unprepare(host->clk_sample);
 error_disable_clk_output:
@@ -1242,6 +1239,9 @@ static int sunxi_mmc_enable(struct sunxi_mmc_host *host)
 	clk_disable_unprepare(host->clk_mmc);
 error_disable_clk_ahb:
 	clk_disable_unprepare(host->clk_ahb);
+error_assert_reset:
+	if (!IS_ERR(host->reset))
+		reset_control_assert(host->reset);
 	return ret;
 }
 
@@ -1249,13 +1249,13 @@ static void sunxi_mmc_disable(struct sunxi_mmc_host *host)
 {
 	sunxi_mmc_reset_host(host);
 
-	if (!IS_ERR(host->reset))
-		reset_control_assert(host->reset);
-
 	clk_disable_unprepare(host->clk_sample);
 	clk_disable_unprepare(host->clk_output);
 	clk_disable_unprepare(host->clk_mmc);
 	clk_disable_unprepare(host->clk_ahb);
+
+	if (!IS_ERR(host->reset))
+		reset_control_assert(host->reset);
 }
 
 static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
-- 
git-series 0.9.1

[PATCH v2 7/8] mmc: sunxi: Set our device drvdata earlier

[Maxime Ripard]

As soon as the pm_runtime_enable hook is called, our runtime_suspend and
runtime_resume hooks can be called as well. However, we only set the device
drvdata that we will use after we have registered into the MMC core. Move
that earlier so that we don't have a race that could lead to a crash.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/mmc/host/sunxi-mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index c6431f6e816f..f6374066081b 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -1336,6 +1336,7 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "mmc alloc host failed\n");
 		return -ENOMEM;
 	}
+	platform_set_drvdata(pdev, mmc);
 
 	host = mmc_priv(mmc);
 	host->dev = &pdev->dev;
@@ -1402,7 +1403,6 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
 		goto error_free_dma;
 
 	dev_info(&pdev->dev, "base:0x%p irq:%u\n", host->reg_base, host->irq);
-	platform_set_drvdata(pdev, mmc);
 	return 0;
 
 error_free_dma:
-- 
git-series 0.9.1

[PATCH v2 8/8] mmc: sunxi: Add runtime_pm support

[Maxime Ripard]

So far, even if our card was not in use, we didn't shut down our main
clock, which meant that it was still output on the MMC bus.

While this obviously means that we could save some power there, it also
created issues when it comes with EMC control since we'll have a perfect
peak at the card clock rate.

Let's implement runtime_pm with autosuspend so that we will shut down the
clock when it's not been in use for quite some time.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/mmc/host/sunxi-mmc.c | 46 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 46 insertions(+)

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index f6374066081b..0f98a5fcaade 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -35,6 +35,7 @@
 #include <linux/of_gpio.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 #include <linux/scatterlist.h>
@@ -973,6 +974,9 @@ static void sunxi_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
 	unsigned long flags;
 	u32 imask;
 
+	if (enable)
+		pm_runtime_get_noresume(host->dev);
+
 	spin_lock_irqsave(&host->lock, flags);
 
 	imask = mmc_readl(host, REG_IMASK);
@@ -985,6 +989,9 @@ static void sunxi_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
 	}
 	mmc_writel(host, REG_IMASK, imask);
 	spin_unlock_irqrestore(&host->lock, flags);
+
+	if (!enable)
+		pm_runtime_put_noidle(host->mmc->parent);
 }
 
 static void sunxi_mmc_hw_reset(struct mmc_host *mmc)
@@ -1398,6 +1405,11 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
 	if (ret)
 		goto error_free_dma;
 
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
 	ret = mmc_add_host(mmc);
 	if (ret)
 		goto error_free_dma;
@@ -1418,6 +1430,7 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
 	struct sunxi_mmc_host *host = mmc_priv(mmc);
 
 	mmc_remove_host(mmc);
+	pm_runtime_force_suspend(&pdev->dev);
 	disable_irq(host->irq);
 	sunxi_mmc_disable(host);
 	dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma);
@@ -1426,10 +1439,43 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int sunxi_mmc_runtime_resume(struct device *dev)
+{
+	struct mmc_host	*mmc = dev_get_drvdata(dev);
+	struct sunxi_mmc_host *host = mmc_priv(mmc);
+	int ret;
+
+	ret = sunxi_mmc_enable(host);
+	if (ret)
+		return ret;
+
+	sunxi_mmc_power_up(mmc, &mmc->ios);
+
+	return 0;
+}
+
+static int sunxi_mmc_runtime_suspend(struct device *dev)
+{
+	struct mmc_host	*mmc = dev_get_drvdata(dev);
+	struct sunxi_mmc_host *host = mmc_priv(mmc);
+
+	sunxi_mmc_power_off(mmc, &mmc->ios);
+	sunxi_mmc_disable(host);
+
+	return 0;
+}
+
+static const struct dev_pm_ops sunxi_mmc_pm_ops = {
+	SET_RUNTIME_PM_OPS(sunxi_mmc_runtime_suspend,
+			   sunxi_mmc_runtime_resume,
+			   NULL)
+};
+
 static struct platform_driver sunxi_mmc_driver = {
 	.driver = {
 		.name	= "sunxi-mmc",
 		.of_match_table = of_match_ptr(sunxi_mmc_of_match),
+		.pm = &sunxi_mmc_pm_ops,
 	},
 	.probe		= sunxi_mmc_probe,
 	.remove		= sunxi_mmc_remove,
-- 
git-series 0.9.1

[PATCH v2 2/8] mmc: sunxi: Move the power off action in a separate function

[Ulf Hansson]

On 8 March 2018 at 15:52, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> We'll need to have the power off behaviour in order to implement
> runtime_pm. Move it outside of the .set_ios callback for an easier access.

This isn't correct. To implement runtime PM for the sunxi device,
which I think is what your goal is, you should not disable/enable
regulators to the (e)MMC/SD/SDIO card.

If you also want runtime PM deployment for the card, that is managed
by the mmc core already. All you have to do is to set
MMC_CAP_AGGRESSIVE_PM for your mmc host, if you want that.

>
> And it improves readibility as a bonus.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  drivers/mmc/host/sunxi-mmc.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index f9723797a9f8..a53e39d8b07e 100644
> --- a/drivers/mmc/host/sunxi-mmc.c
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -855,6 +855,22 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host,
>         return 0;
>  }
>
> +static void sunxi_mmc_power_off(struct mmc_host *mmc, struct mmc_ios *ios)
> +{
> +       struct sunxi_mmc_host *host = mmc_priv(mmc);
> +
> +       dev_dbg(mmc_dev(mmc), "Powering off\n");
> +
> +       sunxi_mmc_reset_host(host);
> +       if (!IS_ERR(mmc->supply.vmmc))
> +               mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0);
> +
> +       if (!IS_ERR(mmc->supply.vqmmc) && host->vqmmc_enabled)
> +               regulator_disable(mmc->supply.vqmmc);
> +
> +       host->vqmmc_enabled = false;
> +}
> +
>  static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  {
>         struct sunxi_mmc_host *host = mmc_priv(mmc);
> @@ -892,14 +908,7 @@ static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>                 break;
>
>         case MMC_POWER_OFF:
> -               dev_dbg(mmc_dev(mmc), "power off!\n");
> -               sunxi_mmc_reset_host(host);
> -               if (!IS_ERR(mmc->supply.vmmc))
> -                       mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0);
> -
> -               if (!IS_ERR(mmc->supply.vqmmc) && host->vqmmc_enabled)
> -                       regulator_disable(mmc->supply.vqmmc);
> -               host->vqmmc_enabled = false;
> +               sunxi_mmc_power_off(mmc, ios);
>                 break;
>         }
>
> --
> git-series 0.9.1

Kind regards
Uffe

[PATCH v2 8/8] mmc: sunxi: Add runtime_pm support

[Ulf Hansson]

On 8 March 2018 at 15:52, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> So far, even if our card was not in use, we didn't shut down our main
> clock, which meant that it was still output on the MMC bus.
>
> While this obviously means that we could save some power there, it also
> created issues when it comes with EMC control since we'll have a perfect
> peak at the card clock rate.
>
> Let's implement runtime_pm with autosuspend so that we will shut down the
> clock when it's not been in use for quite some time.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  drivers/mmc/host/sunxi-mmc.c | 46 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 46 insertions(+)
>
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index f6374066081b..0f98a5fcaade 100644
> --- a/drivers/mmc/host/sunxi-mmc.c
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -35,6 +35,7 @@
>  #include <linux/of_gpio.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
>  #include <linux/scatterlist.h>
> @@ -973,6 +974,9 @@ static void sunxi_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
>         unsigned long flags;
>         u32 imask;
>
> +       if (enable)
> +               pm_runtime_get_noresume(host->dev);
> +
>         spin_lock_irqsave(&host->lock, flags);
>
>         imask = mmc_readl(host, REG_IMASK);
> @@ -985,6 +989,9 @@ static void sunxi_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
>         }
>         mmc_writel(host, REG_IMASK, imask);
>         spin_unlock_irqrestore(&host->lock, flags);
> +
> +       if (!enable)
> +               pm_runtime_put_noidle(host->mmc->parent);
>  }
>
>  static void sunxi_mmc_hw_reset(struct mmc_host *mmc)
> @@ -1398,6 +1405,11 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
>         if (ret)
>                 goto error_free_dma;
>
> +       pm_runtime_set_active(&pdev->dev);
> +       pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> +       pm_runtime_use_autosuspend(&pdev->dev);
> +       pm_runtime_enable(&pdev->dev);
> +
>         ret = mmc_add_host(mmc);
>         if (ret)
>                 goto error_free_dma;
> @@ -1418,6 +1430,7 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
>         struct sunxi_mmc_host *host = mmc_priv(mmc);
>
>         mmc_remove_host(mmc);
> +       pm_runtime_force_suspend(&pdev->dev);
>         disable_irq(host->irq);
>         sunxi_mmc_disable(host);
>         dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma);
> @@ -1426,10 +1439,43 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
>         return 0;
>  }
>
> +static int sunxi_mmc_runtime_resume(struct device *dev)
> +{
> +       struct mmc_host *mmc = dev_get_drvdata(dev);
> +       struct sunxi_mmc_host *host = mmc_priv(mmc);
> +       int ret;
> +
> +       ret = sunxi_mmc_enable(host);
> +       if (ret)
> +               return ret;
> +
> +       sunxi_mmc_power_up(mmc, &mmc->ios);

Instead of doing power up, you may need restore some ios settings,
such as the clock rate for example.

You may also need to restore some registers in sunxi device, in case
it's possible that the controller loses context at runtime suspend.

> +
> +       return 0;
> +}
> +
> +static int sunxi_mmc_runtime_suspend(struct device *dev)
> +{
> +       struct mmc_host *mmc = dev_get_drvdata(dev);
> +       struct sunxi_mmc_host *host = mmc_priv(mmc);
> +
> +       sunxi_mmc_power_off(mmc, &mmc->ios);
> +       sunxi_mmc_disable(host);
> +
> +       return 0;
> +}
> +
> +static const struct dev_pm_ops sunxi_mmc_pm_ops = {
> +       SET_RUNTIME_PM_OPS(sunxi_mmc_runtime_suspend,
> +                          sunxi_mmc_runtime_resume,
> +                          NULL)
> +};
> +
>  static struct platform_driver sunxi_mmc_driver = {
>         .driver = {
>                 .name   = "sunxi-mmc",
>                 .of_match_table = of_match_ptr(sunxi_mmc_of_match),
> +               .pm = &sunxi_mmc_pm_ops,
>         },
>         .probe          = sunxi_mmc_probe,
>         .remove         = sunxi_mmc_remove,
> --
> git-series 0.9.1


Otherwise, overall, this looks rather okay to me.

Kind regards
Uffe

[PATCH v2 8/8] mmc: sunxi: Add runtime_pm support

[Maxime Ripard]

Hi Ulf,

On Thu, Mar 15, 2018 at 10:30:54AM +0100, Ulf Hansson wrote:
> On 8 March 2018 at 15:52, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > So far, even if our card was not in use, we didn't shut down our main
> > clock, which meant that it was still output on the MMC bus.
> >
> > While this obviously means that we could save some power there, it also
> > created issues when it comes with EMC control since we'll have a perfect
> > peak at the card clock rate.
> >
> > Let's implement runtime_pm with autosuspend so that we will shut down the
> > clock when it's not been in use for quite some time.
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > ---
> >  drivers/mmc/host/sunxi-mmc.c | 46 +++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 46 insertions(+)
> >
> > diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> > index f6374066081b..0f98a5fcaade 100644
> > --- a/drivers/mmc/host/sunxi-mmc.c
> > +++ b/drivers/mmc/host/sunxi-mmc.c
> > @@ -35,6 +35,7 @@
> >  #include <linux/of_gpio.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/reset.h>
> >  #include <linux/scatterlist.h>
> > @@ -973,6 +974,9 @@ static void sunxi_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
> >         unsigned long flags;
> >         u32 imask;
> >
> > +       if (enable)
> > +               pm_runtime_get_noresume(host->dev);
> > +
> >         spin_lock_irqsave(&host->lock, flags);
> >
> >         imask = mmc_readl(host, REG_IMASK);
> > @@ -985,6 +989,9 @@ static void sunxi_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
> >         }
> >         mmc_writel(host, REG_IMASK, imask);
> >         spin_unlock_irqrestore(&host->lock, flags);
> > +
> > +       if (!enable)
> > +               pm_runtime_put_noidle(host->mmc->parent);
> >  }
> >
> >  static void sunxi_mmc_hw_reset(struct mmc_host *mmc)
> > @@ -1398,6 +1405,11 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
> >         if (ret)
> >                 goto error_free_dma;
> >
> > +       pm_runtime_set_active(&pdev->dev);
> > +       pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> > +       pm_runtime_use_autosuspend(&pdev->dev);
> > +       pm_runtime_enable(&pdev->dev);
> > +
> >         ret = mmc_add_host(mmc);
> >         if (ret)
> >                 goto error_free_dma;
> > @@ -1418,6 +1430,7 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
> >         struct sunxi_mmc_host *host = mmc_priv(mmc);
> >
> >         mmc_remove_host(mmc);
> > +       pm_runtime_force_suspend(&pdev->dev);
> >         disable_irq(host->irq);
> >         sunxi_mmc_disable(host);
> >         dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma);
> > @@ -1426,10 +1439,43 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
> >         return 0;
> >  }
> >
> > +static int sunxi_mmc_runtime_resume(struct device *dev)
> > +{
> > +       struct mmc_host *mmc = dev_get_drvdata(dev);
> > +       struct sunxi_mmc_host *host = mmc_priv(mmc);
> > +       int ret;
> > +
> > +       ret = sunxi_mmc_enable(host);
> > +       if (ret)
> > +               return ret;
> > +
> > +       sunxi_mmc_power_up(mmc, &mmc->ios);
> 
> Instead of doing power up, you may need restore some ios settings,
> such as the clock rate for example.
> 
> You may also need to restore some registers in sunxi device, in case
> it's possible that the controller loses context at runtime suspend.

The thing I was precisely trying to avoid was this :)

I could save and restore registers when the MMC controller is put into
suspend, but that's pretty much exactly the same sequence than what is
done in the MMC_POWER_UP sequence in .set_ios.

So it just felt cleaner to just do the power_up sequence at resume
time. It avoids having to maintain the list of registers to save and
restore, and it involves less code overall.

It suprised me a bit that the core will not call .set_ios with
MMC_POWER_UP at resume by itself, but I guess there's a good reason
for that :)

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180315/f635bd7a/attachment-0001.sig>

[PATCH v2 5/8] mmc: sunxi: Move resources management to separate functions

[Ulf Hansson]

On 8 March 2018 at 15:52, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> We've had all our resources management, and especially the clocks and reset
> sequence, done directly as part of the probe.
>
> As we want to implement runtime_pm, we'll obviously want to have that
> moved outside of the probe so that we can call do it in our runtime suspend
> and resume hooks without too much duplication.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Thanks, applied for next!

Kind regards
Uffe

> ---
>  drivers/mmc/host/sunxi-mmc.c | 142 ++++++++++++++++++++----------------
>  1 file changed, 82 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index e4b44c4adb8f..34bc90bc04b2 100644
> --- a/drivers/mmc/host/sunxi-mmc.c
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -265,6 +265,7 @@ struct sunxi_mmc_cfg {
>  };
>
>  struct sunxi_mmc_host {
> +       struct device *dev;
>         struct mmc_host *mmc;
>         struct reset_control *reset;
>         const struct sunxi_mmc_cfg *cfg;
> @@ -1183,6 +1184,80 @@ static const struct of_device_id sunxi_mmc_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
>
> +static int sunxi_mmc_enable(struct sunxi_mmc_host *host)
> +{
> +       int ret;
> +
> +       ret = clk_prepare_enable(host->clk_ahb);
> +       if (ret) {
> +               dev_err(dev, "Couldn't enable the bus clocks (%d)\n", ret);
> +               return ret;
> +       }
> +
> +       ret = clk_prepare_enable(host->clk_mmc);
> +       if (ret) {
> +               dev_err(host->dev, "Enable mmc clk err %d\n", ret);
> +               goto error_disable_clk_ahb;
> +       }
> +
> +       ret = clk_prepare_enable(host->clk_output);
> +       if (ret) {
> +               dev_err(host->dev, "Enable output clk err %d\n", ret);
> +               goto error_disable_clk_mmc;
> +       }
> +
> +       ret = clk_prepare_enable(host->clk_sample);
> +       if (ret) {
> +               dev_err(host->dev, "Enable sample clk err %d\n", ret);
> +               goto error_disable_clk_output;
> +       }
> +
> +       if (!IS_ERR(host->reset)) {
> +               ret = reset_control_reset(host->reset);
> +               if (ret) {
> +                       dev_err(dev, "Couldn't reset the MMC controller (%d)\n",
> +                               ret);
> +                       goto error_disable_clk_sample;
> +               }
> +       }
> +
> +       /*
> +        * Sometimes the controller asserts the irq on boot for some reason,
> +        * make sure the controller is in a sane state before enabling irqs.
> +        */
> +       ret = sunxi_mmc_reset_host(host);
> +       if (ret)
> +               goto error_assert_reset;
> +
> +       return 0;
> +
> +error_assert_reset:
> +       if (!IS_ERR(host->reset))
> +               reset_control_assert(host->reset);
> +error_disable_clk_sample:
> +       clk_disable_unprepare(host->clk_sample);
> +error_disable_clk_output:
> +       clk_disable_unprepare(host->clk_output);
> +error_disable_clk_mmc:
> +       clk_disable_unprepare(host->clk_mmc);
> +error_disable_clk_ahb:
> +       clk_disable_unprepare(host->clk_ahb);
> +       return ret;
> +}
> +
> +static void sunxi_mmc_disable(struct sunxi_mmc_host *host)
> +{
> +       sunxi_mmc_reset_host(host);
> +
> +       if (!IS_ERR(host->reset))
> +               reset_control_assert(host->reset);
> +
> +       clk_disable_unprepare(host->clk_sample);
> +       clk_disable_unprepare(host->clk_output);
> +       clk_disable_unprepare(host->clk_mmc);
> +       clk_disable_unprepare(host->clk_ahb);
> +}
> +
>  static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
>                                       struct platform_device *pdev)
>  {
> @@ -1232,66 +1307,21 @@ static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
>         if (PTR_ERR(host->reset) == -EPROBE_DEFER)
>                 return PTR_ERR(host->reset);
>
> -       ret = clk_prepare_enable(host->clk_ahb);
> -       if (ret) {
> -               dev_err(&pdev->dev, "Enable ahb clk err %d\n", ret);
> -               return ret;
> -       }
> -
> -       ret = clk_prepare_enable(host->clk_mmc);
> -       if (ret) {
> -               dev_err(&pdev->dev, "Enable mmc clk err %d\n", ret);
> -               goto error_disable_clk_ahb;
> -       }
> -
> -       ret = clk_prepare_enable(host->clk_output);
> -       if (ret) {
> -               dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
> -               goto error_disable_clk_mmc;
> -       }
> -
> -       ret = clk_prepare_enable(host->clk_sample);
> -       if (ret) {
> -               dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
> -               goto error_disable_clk_output;
> -       }
> -
> -       if (!IS_ERR(host->reset)) {
> -               ret = reset_control_reset(host->reset);
> -               if (ret) {
> -                       dev_err(&pdev->dev, "reset err %d\n", ret);
> -                       goto error_disable_clk_sample;
> -               }
> -       }
> -
> -       /*
> -        * Sometimes the controller asserts the irq on boot for some reason,
> -        * make sure the controller is in a sane state before enabling irqs.
> -        */
> -       ret = sunxi_mmc_reset_host(host);
> +       ret = sunxi_mmc_enable(host);
>         if (ret)
> -               goto error_assert_reset;
> +               return ret;
>
>         host->irq = platform_get_irq(pdev, 0);
>         if (host->irq <= 0) {
>                 ret = -EINVAL;
> -               goto error_assert_reset;
> +               goto error_disable_mmc;
>         }
>
>         return devm_request_threaded_irq(&pdev->dev, host->irq, sunxi_mmc_irq,
>                         sunxi_mmc_handle_manual_stop, 0, "sunxi-mmc", host);
>
> -error_assert_reset:
> -       if (!IS_ERR(host->reset))
> -               reset_control_assert(host->reset);
> -error_disable_clk_sample:
> -       clk_disable_unprepare(host->clk_sample);
> -error_disable_clk_output:
> -       clk_disable_unprepare(host->clk_output);
> -error_disable_clk_mmc:
> -       clk_disable_unprepare(host->clk_mmc);
> -error_disable_clk_ahb:
> -       clk_disable_unprepare(host->clk_ahb);
> +error_disable_mmc:
> +       sunxi_mmc_disable(host);
>         return ret;
>  }
>
> @@ -1308,6 +1338,7 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
>         }
>
>         host = mmc_priv(mmc);
> +       host->dev = &pdev->dev;
>         host->mmc = mmc;
>         spin_lock_init(&host->lock);
>
> @@ -1388,16 +1419,7 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
>
>         mmc_remove_host(mmc);
>         disable_irq(host->irq);
> -       sunxi_mmc_reset_host(host);
> -
> -       if (!IS_ERR(host->reset))
> -               reset_control_assert(host->reset);
> -
> -       clk_disable_unprepare(host->clk_sample);
> -       clk_disable_unprepare(host->clk_output);
> -       clk_disable_unprepare(host->clk_mmc);
> -       clk_disable_unprepare(host->clk_ahb);
> -
> +       sunxi_mmc_disable(host);
>         dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma);
>         mmc_free_host(mmc);
>
> --
> git-series 0.9.1

[PATCH v2 6/8] mmc: sunxi: Move the reset deassertion before enabling the clocks

[Ulf Hansson]

On 8 March 2018 at 15:52, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> As per Allwinner guidelines, the reset line should be deasserted before
> turning the clocks on.
>
> Implement it in our driver as well.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Thanks, applied for next!

Kind regards
Uffe

> ---
>  drivers/mmc/host/sunxi-mmc.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index 34bc90bc04b2..c6431f6e816f 100644
> --- a/drivers/mmc/host/sunxi-mmc.c
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -1188,10 +1188,19 @@ static int sunxi_mmc_enable(struct sunxi_mmc_host *host)
>  {
>         int ret;
>
> +       if (!IS_ERR(host->reset)) {
> +               ret = reset_control_reset(host->reset);
> +               if (ret) {
> +                       dev_err(host->dev, "Couldn't reset the MMC controller (%d)\n",
> +                               ret);
> +                       return ret;
> +               }
> +       }
> +
>         ret = clk_prepare_enable(host->clk_ahb);
>         if (ret) {
> -               dev_err(dev, "Couldn't enable the bus clocks (%d)\n", ret);
> -               return ret;
> +               dev_err(host->dev, "Couldn't enable the bus clocks (%d)\n", ret);
> +               goto error_assert_reset;
>         }
>
>         ret = clk_prepare_enable(host->clk_mmc);
> @@ -1212,28 +1221,16 @@ static int sunxi_mmc_enable(struct sunxi_mmc_host *host)
>                 goto error_disable_clk_output;
>         }
>
> -       if (!IS_ERR(host->reset)) {
> -               ret = reset_control_reset(host->reset);
> -               if (ret) {
> -                       dev_err(dev, "Couldn't reset the MMC controller (%d)\n",
> -                               ret);
> -                       goto error_disable_clk_sample;
> -               }
> -       }
> -
>         /*
>          * Sometimes the controller asserts the irq on boot for some reason,
>          * make sure the controller is in a sane state before enabling irqs.
>          */
>         ret = sunxi_mmc_reset_host(host);
>         if (ret)
> -               goto error_assert_reset;
> +               goto error_disable_clk_sample;
>
>         return 0;
>
> -error_assert_reset:
> -       if (!IS_ERR(host->reset))
> -               reset_control_assert(host->reset);
>  error_disable_clk_sample:
>         clk_disable_unprepare(host->clk_sample);
>  error_disable_clk_output:
> @@ -1242,6 +1239,9 @@ static int sunxi_mmc_enable(struct sunxi_mmc_host *host)
>         clk_disable_unprepare(host->clk_mmc);
>  error_disable_clk_ahb:
>         clk_disable_unprepare(host->clk_ahb);
> +error_assert_reset:
> +       if (!IS_ERR(host->reset))
> +               reset_control_assert(host->reset);
>         return ret;
>  }
>
> @@ -1249,13 +1249,13 @@ static void sunxi_mmc_disable(struct sunxi_mmc_host *host)
>  {
>         sunxi_mmc_reset_host(host);
>
> -       if (!IS_ERR(host->reset))
> -               reset_control_assert(host->reset);
> -
>         clk_disable_unprepare(host->clk_sample);
>         clk_disable_unprepare(host->clk_output);
>         clk_disable_unprepare(host->clk_mmc);
>         clk_disable_unprepare(host->clk_ahb);
> +
> +       if (!IS_ERR(host->reset))
> +               reset_control_assert(host->reset);
>  }
>
>  static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
> --
> git-series 0.9.1

[PATCH v2 7/8] mmc: sunxi: Set our device drvdata earlier

[Ulf Hansson]

On 8 March 2018 at 15:52, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> As soon as the pm_runtime_enable hook is called, our runtime_suspend and
> runtime_resume hooks can be called as well. However, we only set the device
> drvdata that we will use after we have registered into the MMC core. Move
> that earlier so that we don't have a race that could lead to a crash.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Thanks, applied for next!

Kind regards
Uffe

> ---
>  drivers/mmc/host/sunxi-mmc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index c6431f6e816f..f6374066081b 100644
> --- a/drivers/mmc/host/sunxi-mmc.c
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -1336,6 +1336,7 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
>                 dev_err(&pdev->dev, "mmc alloc host failed\n");
>                 return -ENOMEM;
>         }
> +       platform_set_drvdata(pdev, mmc);
>
>         host = mmc_priv(mmc);
>         host->dev = &pdev->dev;
> @@ -1402,7 +1403,6 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
>                 goto error_free_dma;
>
>         dev_info(&pdev->dev, "base:0x%p irq:%u\n", host->reg_base, host->irq);
> -       platform_set_drvdata(pdev, mmc);
>         return 0;
>
>  error_free_dma:
> --
> git-series 0.9.1

[PATCH v2 8/8] mmc: sunxi: Add runtime_pm support

[Ulf Hansson]

On 15 March 2018 at 11:04, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> Hi Ulf,
>
> On Thu, Mar 15, 2018 at 10:30:54AM +0100, Ulf Hansson wrote:
>> On 8 March 2018 at 15:52, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>> > So far, even if our card was not in use, we didn't shut down our main
>> > clock, which meant that it was still output on the MMC bus.
>> >
>> > While this obviously means that we could save some power there, it also
>> > created issues when it comes with EMC control since we'll have a perfect
>> > peak at the card clock rate.
>> >
>> > Let's implement runtime_pm with autosuspend so that we will shut down the
>> > clock when it's not been in use for quite some time.
>> >
>> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>> > ---
>> >  drivers/mmc/host/sunxi-mmc.c | 46 +++++++++++++++++++++++++++++++++++++-
>> >  1 file changed, 46 insertions(+)
>> >
>> > diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
>> > index f6374066081b..0f98a5fcaade 100644
>> > --- a/drivers/mmc/host/sunxi-mmc.c
>> > +++ b/drivers/mmc/host/sunxi-mmc.c
>> > @@ -35,6 +35,7 @@
>> >  #include <linux/of_gpio.h>
>> >  #include <linux/of_platform.h>
>> >  #include <linux/platform_device.h>
>> > +#include <linux/pm_runtime.h>
>> >  #include <linux/regulator/consumer.h>
>> >  #include <linux/reset.h>
>> >  #include <linux/scatterlist.h>
>> > @@ -973,6 +974,9 @@ static void sunxi_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
>> >         unsigned long flags;
>> >         u32 imask;
>> >
>> > +       if (enable)
>> > +               pm_runtime_get_noresume(host->dev);
>> > +
>> >         spin_lock_irqsave(&host->lock, flags);
>> >
>> >         imask = mmc_readl(host, REG_IMASK);
>> > @@ -985,6 +989,9 @@ static void sunxi_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
>> >         }
>> >         mmc_writel(host, REG_IMASK, imask);
>> >         spin_unlock_irqrestore(&host->lock, flags);
>> > +
>> > +       if (!enable)
>> > +               pm_runtime_put_noidle(host->mmc->parent);
>> >  }
>> >
>> >  static void sunxi_mmc_hw_reset(struct mmc_host *mmc)
>> > @@ -1398,6 +1405,11 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
>> >         if (ret)
>> >                 goto error_free_dma;
>> >
>> > +       pm_runtime_set_active(&pdev->dev);
>> > +       pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
>> > +       pm_runtime_use_autosuspend(&pdev->dev);
>> > +       pm_runtime_enable(&pdev->dev);
>> > +
>> >         ret = mmc_add_host(mmc);
>> >         if (ret)
>> >                 goto error_free_dma;
>> > @@ -1418,6 +1430,7 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
>> >         struct sunxi_mmc_host *host = mmc_priv(mmc);
>> >
>> >         mmc_remove_host(mmc);
>> > +       pm_runtime_force_suspend(&pdev->dev);
>> >         disable_irq(host->irq);
>> >         sunxi_mmc_disable(host);
>> >         dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma);
>> > @@ -1426,10 +1439,43 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
>> >         return 0;
>> >  }
>> >
>> > +static int sunxi_mmc_runtime_resume(struct device *dev)
>> > +{
>> > +       struct mmc_host *mmc = dev_get_drvdata(dev);
>> > +       struct sunxi_mmc_host *host = mmc_priv(mmc);
>> > +       int ret;
>> > +
>> > +       ret = sunxi_mmc_enable(host);
>> > +       if (ret)
>> > +               return ret;
>> > +
>> > +       sunxi_mmc_power_up(mmc, &mmc->ios);
>>
>> Instead of doing power up, you may need restore some ios settings,
>> such as the clock rate for example.
>>
>> You may also need to restore some registers in sunxi device, in case
>> it's possible that the controller loses context at runtime suspend.
>
> The thing I was precisely trying to avoid was this :)
>
> I could save and restore registers when the MMC controller is put into
> suspend, but that's pretty much exactly the same sequence than what is
> done in the MMC_POWER_UP sequence in .set_ios.

Well, there may be some overlap.

>
> So it just felt cleaner to just do the power_up sequence at resume
> time. It avoids having to maintain the list of registers to save and
> restore, and it involves less code overall.

I understand.

>
> It suprised me a bit that the core will not call .set_ios with
> MMC_POWER_UP at resume by itself, but I guess there's a good reason
> for that :)

It does that when it runtime PM manages the mmc/sd/sdio card, don't
confuse that with the mmc host. That's because it needs to follow the
(e)MMC/SD/SDIO spec, which don't allows you to just cut the power to
card without first informing (sending commands) the card about it.

Kind regards
Uffe

[PATCH v2 8/8] mmc: sunxi: Add runtime_pm support

[Maxime Ripard]

Hi,

On Thu, Mar 15, 2018 at 11:24:36AM +0100, Ulf Hansson wrote:
> >> > +static int sunxi_mmc_runtime_resume(struct device *dev)
> >> > +{
> >> > +       struct mmc_host *mmc = dev_get_drvdata(dev);
> >> > +       struct sunxi_mmc_host *host = mmc_priv(mmc);
> >> > +       int ret;
> >> > +
> >> > +       ret = sunxi_mmc_enable(host);
> >> > +       if (ret)
> >> > +               return ret;
> >> > +
> >> > +       sunxi_mmc_power_up(mmc, &mmc->ios);
> >>
> >> Instead of doing power up, you may need restore some ios settings,
> >> such as the clock rate for example.
> >>
> >> You may also need to restore some registers in sunxi device, in case
> >> it's possible that the controller loses context at runtime suspend.
> >
> > The thing I was precisely trying to avoid was this :)
> >
> > I could save and restore registers when the MMC controller is put into
> > suspend, but that's pretty much exactly the same sequence than what is
> > done in the MMC_POWER_UP sequence in .set_ios.
> 
> Well, there may be some overlap.
> 
> >
> > So it just felt cleaner to just do the power_up sequence at resume
> > time. It avoids having to maintain the list of registers to save and
> > restore, and it involves less code overall.
> 
> I understand.
> 
> >
> > It suprised me a bit that the core will not call .set_ios with
> > MMC_POWER_UP at resume by itself, but I guess there's a good reason
> > for that :)
> 
> It does that when it runtime PM manages the mmc/sd/sdio card, don't
> confuse that with the mmc host. That's because it needs to follow the
> (e)MMC/SD/SDIO spec, which don't allows you to just cut the power to
> card without first informing (sending commands) the card about it.

Ok. So this might sound a bit trivial, but I'm confused now about what
set_ios is supposed to be doing.

I thought this was about the MMC controller itself, but from what
you're saying, it seems that it is used for both the MMC controller
and the MMC card itself, with the powermode being about the MMC card,
and the rest about the MMC controller.

I guess we're mixing the two then, especially the power off part that
will also reset the controller, or the controller that is initialised
only at power on. Other MMC controller drivers I could find were doing
the MMC controller setup unconditionally, so I guess we have room for
improvements here.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180315/139fa093/attachment.sig>

[PATCH v2 8/8] mmc: sunxi: Add runtime_pm support

[Ulf Hansson]

On 15 March 2018 at 13:04, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> Hi,
>
> On Thu, Mar 15, 2018 at 11:24:36AM +0100, Ulf Hansson wrote:
>> >> > +static int sunxi_mmc_runtime_resume(struct device *dev)
>> >> > +{
>> >> > +       struct mmc_host *mmc = dev_get_drvdata(dev);
>> >> > +       struct sunxi_mmc_host *host = mmc_priv(mmc);
>> >> > +       int ret;
>> >> > +
>> >> > +       ret = sunxi_mmc_enable(host);
>> >> > +       if (ret)
>> >> > +               return ret;
>> >> > +
>> >> > +       sunxi_mmc_power_up(mmc, &mmc->ios);
>> >>
>> >> Instead of doing power up, you may need restore some ios settings,
>> >> such as the clock rate for example.
>> >>
>> >> You may also need to restore some registers in sunxi device, in case
>> >> it's possible that the controller loses context at runtime suspend.
>> >
>> > The thing I was precisely trying to avoid was this :)
>> >
>> > I could save and restore registers when the MMC controller is put into
>> > suspend, but that's pretty much exactly the same sequence than what is
>> > done in the MMC_POWER_UP sequence in .set_ios.
>>
>> Well, there may be some overlap.
>>
>> >
>> > So it just felt cleaner to just do the power_up sequence at resume
>> > time. It avoids having to maintain the list of registers to save and
>> > restore, and it involves less code overall.
>>
>> I understand.
>>
>> >
>> > It suprised me a bit that the core will not call .set_ios with
>> > MMC_POWER_UP at resume by itself, but I guess there's a good reason
>> > for that :)
>>
>> It does that when it runtime PM manages the mmc/sd/sdio card, don't
>> confuse that with the mmc host. That's because it needs to follow the
>> (e)MMC/SD/SDIO spec, which don't allows you to just cut the power to
>> card without first informing (sending commands) the card about it.
>
> Ok. So this might sound a bit trivial, but I'm confused now about what
> set_ios is supposed to be doing.
>
> I thought this was about the MMC controller itself, but from what
> you're saying, it seems that it is used for both the MMC controller
> and the MMC card itself, with the powermode being about the MMC card,
> and the rest about the MMC controller.

Correct.

The ->set_ios() ops, is an interface called by the core at points when
it wants to change some settings for the card. Those settings may or
may not involve changes to internal controller registers, depending on
the controller/SoC.

For example, some mmc controllers have build in support for changing
(dividing) the clock rate. While others may rely solely on other
separate clock providers.

Same goes for powering the mmc card, while I would say it more common
to use external regulators controlled by the regulator APIs, some
actually have internal registers needed be change to provide power to
the card.

You may have a look at mmci.c, it has these kind of things.

>
> I guess we're mixing the two then, especially the power off part that
> will also reset the controller, or the controller that is initialised

If you need to reset the controller at runtime resume of the
controller device, that's controller and SoC specific.

> only at power on. Other MMC controller drivers I could find were doing
> the MMC controller setup unconditionally, so I guess we have room for
> improvements here.

Yeah, perhaps they are better safe than sorry. But again, it's device
and SoC specific.

Kind regards
Uffe

[PATCH v2 8/8] mmc: sunxi: Add runtime_pm support

[Maxime Ripard]

Hi Ulf,

On Thu, Mar 15, 2018 at 01:40:41PM +0100, Ulf Hansson wrote:
> On 15 March 2018 at 13:04, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > Hi,
> >
> > On Thu, Mar 15, 2018 at 11:24:36AM +0100, Ulf Hansson wrote:
> >> >> > +static int sunxi_mmc_runtime_resume(struct device *dev)
> >> >> > +{
> >> >> > +       struct mmc_host *mmc = dev_get_drvdata(dev);
> >> >> > +       struct sunxi_mmc_host *host = mmc_priv(mmc);
> >> >> > +       int ret;
> >> >> > +
> >> >> > +       ret = sunxi_mmc_enable(host);
> >> >> > +       if (ret)
> >> >> > +               return ret;
> >> >> > +
> >> >> > +       sunxi_mmc_power_up(mmc, &mmc->ios);
> >> >>
> >> >> Instead of doing power up, you may need restore some ios settings,
> >> >> such as the clock rate for example.
> >> >>
> >> >> You may also need to restore some registers in sunxi device, in case
> >> >> it's possible that the controller loses context at runtime suspend.
> >> >
> >> > The thing I was precisely trying to avoid was this :)
> >> >
> >> > I could save and restore registers when the MMC controller is put into
> >> > suspend, but that's pretty much exactly the same sequence than what is
> >> > done in the MMC_POWER_UP sequence in .set_ios.
> >>
> >> Well, there may be some overlap.
> >>
> >> >
> >> > So it just felt cleaner to just do the power_up sequence at resume
> >> > time. It avoids having to maintain the list of registers to save and
> >> > restore, and it involves less code overall.
> >>
> >> I understand.
> >>
> >> >
> >> > It suprised me a bit that the core will not call .set_ios with
> >> > MMC_POWER_UP at resume by itself, but I guess there's a good reason
> >> > for that :)
> >>
> >> It does that when it runtime PM manages the mmc/sd/sdio card, don't
> >> confuse that with the mmc host. That's because it needs to follow the
> >> (e)MMC/SD/SDIO spec, which don't allows you to just cut the power to
> >> card without first informing (sending commands) the card about it.
> >
> > Ok. So this might sound a bit trivial, but I'm confused now about what
> > set_ios is supposed to be doing.
> >
> > I thought this was about the MMC controller itself, but from what
> > you're saying, it seems that it is used for both the MMC controller
> > and the MMC card itself, with the powermode being about the MMC card,
> > and the rest about the MMC controller.
> 
> Correct.
> 
> The ->set_ios() ops, is an interface called by the core at points when
> it wants to change some settings for the card. Those settings may or
> may not involve changes to internal controller registers, depending on
> the controller/SoC.
> 
> For example, some mmc controllers have build in support for changing
> (dividing) the clock rate. While others may rely solely on other
> separate clock providers.
> 
> Same goes for powering the mmc card, while I would say it more common
> to use external regulators controlled by the regulator APIs, some
> actually have internal registers needed be change to provide power to
> the card.
> 
> You may have a look at mmci.c, it has these kind of things.

So I guess if I split out the MMC controller initialization into
separate functions, called in both the set_ios callback and the
runtime_resume path, that would be ok for you?

> >
> > I guess we're mixing the two then, especially the power off part that
> > will also reset the controller, or the controller that is initialised
> 
> If you need to reset the controller at runtime resume of the
> controller device, that's controller and SoC specific.

I was talking about:
https://elixir.bootlin.com/linux/v4.16-rc6/source/drivers/mmc/host/sunxi-mmc.c#L899

Ie. in set_ios, with MMC_POWER_DOWN, not during runtime_resume. I
guess from what you were telling me that it's not really advisable?

> > only at power on. Other MMC controller drivers I could find were doing
> > the MMC controller setup unconditionally, so I guess we have room for
> > improvements here.
> 
> Yeah, perhaps they are better safe than sorry. But again, it's device
> and SoC specific.

Ok.

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com