[PATCH v3 0/7] mmc: sunxi: Add runtime PM support

[PATCH v3 0/7] 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 v2:
  - Dropped the patches already applied
  - Do not power up or down the card during a runtime_resume or
    runtime_suspend, but only take care of the controller
  - In order to do the above, split the set_ios function into smaller
    chunks that can be called from set_ios and our runtime_resume hook

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 (7):
  mmc: sunxi: Reorder the headers
  mmc: sunxi: Change sunxi_mmc_init_host argument type
  mmc: sunxi: Move bus width configuration to a function
  mmc: sunxi: Move clock configuration to a function
  mmc: sunxi: Move the card power configuration to a function
  mmc: sunxi: Add runtime_pm support
  mmc: sunxi: Drop the init / reset of the controller from set_ios

 drivers/mmc/host/sunxi-mmc.c | 185 +++++++++++++++++++++++-------------
 1 file changed, 122 insertions(+), 63 deletions(-)

base-commit: 60cc43fc888428bb2f18f08997432d426a243338
-- 
git-series 0.9.1

[PATCH v3 0/7] 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 v2:
  - Dropped the patches already applied
  - Do not power up or down the card during a runtime_resume or
    runtime_suspend, but only take care of the controller
  - In order to do the above, split the set_ios function into smaller
    chunks that can be called from set_ios and our runtime_resume hook

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 (7):
  mmc: sunxi: Reorder the headers
  mmc: sunxi: Change sunxi_mmc_init_host argument type
  mmc: sunxi: Move bus width configuration to a function
  mmc: sunxi: Move clock configuration to a function
  mmc: sunxi: Move the card power configuration to a function
  mmc: sunxi: Add runtime_pm support
  mmc: sunxi: Drop the init / reset of the controller from set_ios

 drivers/mmc/host/sunxi-mmc.c | 185 +++++++++++++++++++++++-------------
 1 file changed, 122 insertions(+), 63 deletions(-)

base-commit: 60cc43fc888428bb2f18f08997432d426a243338
-- 
git-series 0.9.1

[PATCH v3 1/7] 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 20cfb20418f3..91feed8045fd 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/host.h>
+#include <linux/mmc/mmc.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 v3 1/7] 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 20cfb20418f3..91feed8045fd 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/host.h>
+#include <linux/mmc/mmc.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 v3 2/7] mmc: sunxi: Change sunxi_mmc_init_host argument type

[Maxime Ripard]

All the other functions in the driver take a struct sunxi_mmc_host pointer.
Let's make it consistent.

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

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 91feed8045fd..9fdaea37aa74 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -319,10 +319,9 @@ static int sunxi_mmc_reset_host(struct sunxi_mmc_host *host)
 	return 0;
 }
 
-static int sunxi_mmc_init_host(struct mmc_host *mmc)
+static int sunxi_mmc_init_host(struct sunxi_mmc_host *host)
 {
 	u32 rval;
-	struct sunxi_mmc_host *host = mmc_priv(mmc);
 
 	if (sunxi_mmc_reset_host(host))
 		return -EIO;
@@ -885,7 +884,7 @@ static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 			host->vqmmc_enabled = true;
 		}
 
-		host->ferror = sunxi_mmc_init_host(mmc);
+		host->ferror = sunxi_mmc_init_host(host);
 		if (host->ferror)
 			return;
 
-- 
git-series 0.9.1

[PATCH v3 2/7] mmc: sunxi: Change sunxi_mmc_init_host argument type

[Maxime Ripard]

All the other functions in the driver take a struct sunxi_mmc_host pointer.
Let's make it consistent.

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

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 91feed8045fd..9fdaea37aa74 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -319,10 +319,9 @@ static int sunxi_mmc_reset_host(struct sunxi_mmc_host *host)
 	return 0;
 }
 
-static int sunxi_mmc_init_host(struct mmc_host *mmc)
+static int sunxi_mmc_init_host(struct sunxi_mmc_host *host)
 {
 	u32 rval;
-	struct sunxi_mmc_host *host = mmc_priv(mmc);
 
 	if (sunxi_mmc_reset_host(host))
 		return -EIO;
@@ -885,7 +884,7 @@ static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 			host->vqmmc_enabled = true;
 		}
 
-		host->ferror = sunxi_mmc_init_host(mmc);
+		host->ferror = sunxi_mmc_init_host(host);
 		if (host->ferror)
 			return;
 
-- 
git-series 0.9.1

[PATCH v3 3/7] mmc: sunxi: Move bus width configuration to a function

[Maxime Ripard]

In order to improve readibility and reusability, let's move the bus width
setup to a small function called by our .set_ios hook.

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

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 9fdaea37aa74..c0bb75e17a44 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_set_bus_width(struct sunxi_mmc_host *host,
+				   unsigned char width)
+{
+	switch (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;
+	}
+}
+
 static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct sunxi_mmc_host *host = mmc_priv(mmc);
@@ -903,18 +919,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;
-	}
+	sunxi_mmc_set_bus_width(host, ios->bus_width);
 
 	/* set ddr mode */
 	rval = mmc_readl(host, REG_GCTRL);
-- 
git-series 0.9.1

[PATCH v3 3/7] mmc: sunxi: Move bus width configuration to a function

[Maxime Ripard]

In order to improve readibility and reusability, let's move the bus width
setup to a small function called by our .set_ios hook.

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

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 9fdaea37aa74..c0bb75e17a44 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_set_bus_width(struct sunxi_mmc_host *host,
+				   unsigned char width)
+{
+	switch (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;
+	}
+}
+
 static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct sunxi_mmc_host *host = mmc_priv(mmc);
@@ -903,18 +919,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;
-	}
+	sunxi_mmc_set_bus_width(host, ios->bus_width);
 
 	/* set ddr mode */
 	rval = mmc_readl(host, REG_GCTRL);
-- 
git-series 0.9.1

[PATCH v3 4/7] mmc: sunxi: Move clock configuration to a function

[Maxime Ripard]

In order to improve readibility and reusability, let's move the clock setup
to a small function called by our .set_ios hook.

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

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index c0bb75e17a44..3c1ab46cc60a 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -871,6 +871,23 @@ static void sunxi_mmc_set_bus_width(struct sunxi_mmc_host *host,
 	}
 }
 
+static void sunxi_mmc_set_clk(struct sunxi_mmc_host *host, struct mmc_ios *ios)
+{
+	u32 rval;
+
+	/* 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_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct sunxi_mmc_host *host = mmc_priv(mmc);
@@ -920,21 +937,7 @@ static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	}
 
 	sunxi_mmc_set_bus_width(host, ios->bus_width);
-
-	/* 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_set_clk(host, ios);
 }
 
 static int sunxi_mmc_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios)
-- 
git-series 0.9.1

[PATCH v3 4/7] mmc: sunxi: Move clock configuration to a function

[Maxime Ripard]

In order to improve readibility and reusability, let's move the clock setup
to a small function called by our .set_ios hook.

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

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index c0bb75e17a44..3c1ab46cc60a 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -871,6 +871,23 @@ static void sunxi_mmc_set_bus_width(struct sunxi_mmc_host *host,
 	}
 }
 
+static void sunxi_mmc_set_clk(struct sunxi_mmc_host *host, struct mmc_ios *ios)
+{
+	u32 rval;
+
+	/* 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_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct sunxi_mmc_host *host = mmc_priv(mmc);
@@ -920,21 +937,7 @@ static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	}
 
 	sunxi_mmc_set_bus_width(host, ios->bus_width);
-
-	/* 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_set_clk(host, ios);
 }
 
 static int sunxi_mmc_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios)
-- 
git-series 0.9.1

[PATCH v3 5/7] mmc: sunxi: Move the card power configuration to a function

[Maxime Ripard]

In order to improve readibility and reusability, let's move the card setup
to a small function called by our .set_ios hook.

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

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 3c1ab46cc60a..0165da0d022a 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -888,17 +888,15 @@ static void sunxi_mmc_set_clk(struct sunxi_mmc_host *host, struct mmc_ios *ios)
 	/* Android code had a usleep_range(50000, 55000); here */
 }
 
-static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+static void sunxi_mmc_card_power(struct sunxi_mmc_host *host,
+				 struct mmc_ios *ios)
 {
-	struct sunxi_mmc_host *host = mmc_priv(mmc);
-	u32 rval;
+	struct mmc_host *mmc = host->mmc;
 
-	/* Set the power state */
 	switch (ios->power_mode) {
-	case MMC_POWER_ON:
-		break;
-
 	case MMC_POWER_UP:
+		dev_dbg(mmc_dev(mmc), "Powering card up\n");
+
 		if (!IS_ERR(mmc->supply.vmmc)) {
 			host->ferror = mmc_regulator_set_ocr(mmc,
 							     mmc->supply.vmmc,
@@ -916,25 +914,37 @@ static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 			}
 			host->vqmmc_enabled = true;
 		}
-
-		host->ferror = sunxi_mmc_init_host(host);
-		if (host->ferror)
-			return;
-
-		dev_dbg(mmc_dev(mmc), "power on!\n");
 		break;
 
 	case MMC_POWER_OFF:
-		dev_dbg(mmc_dev(mmc), "power off!\n");
-		sunxi_mmc_reset_host(host);
+		dev_dbg(mmc_dev(mmc), "Powering card off\n");
+
 		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;
 		break;
+
+	default:
+		dev_dbg(mmc_dev(mmc), "Ignoring unknown card power state\n");
+		break;
 	}
+}
+
+static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+	struct sunxi_mmc_host *host = mmc_priv(mmc);
+
+	if (ios->power_mode == MMC_POWER_OFF)
+		sunxi_mmc_reset_host(host);
+
+	sunxi_mmc_card_power(host, ios);
+
+	if (ios->power_mode == MMC_POWER_UP)
+		sunxi_mmc_init_host(host);
 
 	sunxi_mmc_set_bus_width(host, ios->bus_width);
 	sunxi_mmc_set_clk(host, ios);
-- 
git-series 0.9.1

[PATCH v3 5/7] mmc: sunxi: Move the card power configuration to a function

[Maxime Ripard]

In order to improve readibility and reusability, let's move the card setup
to a small function called by our .set_ios hook.

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

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 3c1ab46cc60a..0165da0d022a 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -888,17 +888,15 @@ static void sunxi_mmc_set_clk(struct sunxi_mmc_host *host, struct mmc_ios *ios)
 	/* Android code had a usleep_range(50000, 55000); here */
 }
 
-static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+static void sunxi_mmc_card_power(struct sunxi_mmc_host *host,
+				 struct mmc_ios *ios)
 {
-	struct sunxi_mmc_host *host = mmc_priv(mmc);
-	u32 rval;
+	struct mmc_host *mmc = host->mmc;
 
-	/* Set the power state */
 	switch (ios->power_mode) {
-	case MMC_POWER_ON:
-		break;
-
 	case MMC_POWER_UP:
+		dev_dbg(mmc_dev(mmc), "Powering card up\n");
+
 		if (!IS_ERR(mmc->supply.vmmc)) {
 			host->ferror = mmc_regulator_set_ocr(mmc,
 							     mmc->supply.vmmc,
@@ -916,25 +914,37 @@ static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 			}
 			host->vqmmc_enabled = true;
 		}
-
-		host->ferror = sunxi_mmc_init_host(host);
-		if (host->ferror)
-			return;
-
-		dev_dbg(mmc_dev(mmc), "power on!\n");
 		break;
 
 	case MMC_POWER_OFF:
-		dev_dbg(mmc_dev(mmc), "power off!\n");
-		sunxi_mmc_reset_host(host);
+		dev_dbg(mmc_dev(mmc), "Powering card off\n");
+
 		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;
 		break;
+
+	default:
+		dev_dbg(mmc_dev(mmc), "Ignoring unknown card power state\n");
+		break;
 	}
+}
+
+static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+	struct sunxi_mmc_host *host = mmc_priv(mmc);
+
+	if (ios->power_mode == MMC_POWER_OFF)
+		sunxi_mmc_reset_host(host);
+
+	sunxi_mmc_card_power(host, ios);
+
+	if (ios->power_mode == MMC_POWER_UP)
+		sunxi_mmc_init_host(host);
 
 	sunxi_mmc_set_bus_width(host, ios->bus_width);
 	sunxi_mmc_set_clk(host, ios);
-- 
git-series 0.9.1

[PATCH v3 6/7] mmc: sunxi: Add runtime_pm support

[Maxime Ripard]

So far, even if our card was not in use, we didn't shut down our MMC
controller, which meant that it was still active and clocking the bus.

While this obviously means that we could save some power there, it also
creates issues when it comes to 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
controller 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 | 48 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+)

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 0165da0d022a..0253deb153a4 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>
@@ -969,6 +970,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);
@@ -981,6 +985,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)
@@ -1394,6 +1401,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;
@@ -1414,6 +1426,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);
@@ -1422,10 +1435,45 @@ 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_init_host(host);
+	sunxi_mmc_set_bus_width(host, mmc->ios.bus_width);
+	sunxi_mmc_set_clk(host, &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_reset_host(host);
+	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 v3 6/7] mmc: sunxi: Add runtime_pm support

[Maxime Ripard]

So far, even if our card was not in use, we didn't shut down our MMC
controller, which meant that it was still active and clocking the bus.

While this obviously means that we could save some power there, it also
creates issues when it comes to 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
controller 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 | 48 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+)

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 0165da0d022a..0253deb153a4 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>
@@ -969,6 +970,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);
@@ -981,6 +985,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)
@@ -1394,6 +1401,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;
@@ -1414,6 +1426,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);
@@ -1422,10 +1435,45 @@ 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_init_host(host);
+	sunxi_mmc_set_bus_width(host, mmc->ios.bus_width);
+	sunxi_mmc_set_clk(host, &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_reset_host(host);
+	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

Re: [PATCH v3 6/7] mmc: sunxi: Add runtime_pm support

[Marc Zyngier]

Hi Maxime,

On 16/04/18 15:23, Maxime Ripard wrote:
> So far, even if our card was not in use, we didn't shut down our MMC
> controller, which meant that it was still active and clocking the bus.
> 
> While this obviously means that we could save some power there, it also
> creates issues when it comes to 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
> controller 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 | 48 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 48 insertions(+)
> 
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index 0165da0d022a..0253deb153a4 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>
> @@ -969,6 +970,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);
> @@ -981,6 +985,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)
> @@ -1394,6 +1401,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;
> @@ -1414,6 +1426,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);
> @@ -1422,10 +1435,45 @@ 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_init_host(host);
> +	sunxi_mmc_set_bus_width(host, mmc->ios.bus_width);
> +	sunxi_mmc_set_clk(host, &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_reset_host(host);
> +	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,
> 

This patch has the unfortunate impact of killing my A20 system
(cubietruck), as of 9a8e1e8cc2c02c57c4e941651a8481a633506c91:

[...]
[    3.286649] NET: Registered protocol family 10
[    3.291898]  mmcblk0: p1
[    3.295297] mmc1: queuing unknown CIS tuple 0x80 (2 bytes)
[    3.302773] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
[    3.305787] Segment Routing with IPv6
[    3.312225] mip6: Mobile IPv6
[    3.316166] sit: IPv6, IPv4 and MPLS over IPv4 tunneling driver
[    3.316246] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
[    3.323721] ip6_gre: GRE over IPv6 tunneling driver
[    3.333954] NET: Registered protocol family 17
[    3.338837] 9pnet: Installing 9P2000 support
[    3.343379] NET: Registered protocol family 37
[    3.347885] Key type dns_resolver registered
[    3.352214] mmc1: queuing unknown CIS tuple 0x80 (7 bytes)
[    3.352217] openvswitch: Open vSwitch switching datapath
[    3.352620] mpls_gso: MPLS GSO support
[    3.367001] ThumbEE CPU extension supported.

and that's where it stops. No message, just a hard lockup (I suppose
that both the CPUs are trying to access some device that is now not
clocked).

With a working kernel, I see SATA and the wifi SDIO being probed.

Happy to help testing stuff if you have any idea.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v3 6/7] mmc: sunxi: Add runtime_pm support

[Marc Zyngier]

Hi Maxime,

On 16/04/18 15:23, Maxime Ripard wrote:
> So far, even if our card was not in use, we didn't shut down our MMC
> controller, which meant that it was still active and clocking the bus.
> 
> While this obviously means that we could save some power there, it also
> creates issues when it comes to 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
> controller 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 | 48 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 48 insertions(+)
> 
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index 0165da0d022a..0253deb153a4 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>
> @@ -969,6 +970,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);
> @@ -981,6 +985,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)
> @@ -1394,6 +1401,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;
> @@ -1414,6 +1426,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);
> @@ -1422,10 +1435,45 @@ 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_init_host(host);
> +	sunxi_mmc_set_bus_width(host, mmc->ios.bus_width);
> +	sunxi_mmc_set_clk(host, &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_reset_host(host);
> +	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,
> 

This patch has the unfortunate impact of killing my A20 system
(cubietruck), as of 9a8e1e8cc2c02c57c4e941651a8481a633506c91:

[...]
[    3.286649] NET: Registered protocol family 10
[    3.291898]  mmcblk0: p1
[    3.295297] mmc1: queuing unknown CIS tuple 0x80 (2 bytes)
[    3.302773] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
[    3.305787] Segment Routing with IPv6
[    3.312225] mip6: Mobile IPv6
[    3.316166] sit: IPv6, IPv4 and MPLS over IPv4 tunneling driver
[    3.316246] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
[    3.323721] ip6_gre: GRE over IPv6 tunneling driver
[    3.333954] NET: Registered protocol family 17
[    3.338837] 9pnet: Installing 9P2000 support
[    3.343379] NET: Registered protocol family 37
[    3.347885] Key type dns_resolver registered
[    3.352214] mmc1: queuing unknown CIS tuple 0x80 (7 bytes)
[    3.352217] openvswitch: Open vSwitch switching datapath
[    3.352620] mpls_gso: MPLS GSO support
[    3.367001] ThumbEE CPU extension supported.

and that's where it stops. No message, just a hard lockup (I suppose
that both the CPUs are trying to access some device that is now not
clocked).

With a working kernel, I see SATA and the wifi SDIO being probed.

Happy to help testing stuff if you have any idea.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 6/7] mmc: sunxi: Add runtime_pm support

[Kevin Hilman]

On Thu, Jun 14, 2018 at 7:12 AM Marc Zyngier <marc.zyngier@arm.com> wrote:
>
> Hi Maxime,
>
> On 16/04/18 15:23, Maxime Ripard wrote:
> > So far, even if our card was not in use, we didn't shut down our MMC
> > controller, which meant that it was still active and clocking the bus.
> >
> > While this obviously means that we could save some power there, it also
> > creates issues when it comes to 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
> > controller when it's not been in use for quite some time.
> >
[...]

> This patch has the unfortunate impact of killing my A20 system
> (cubietruck), as of 9a8e1e8cc2c02c57c4e941651a8481a633506c91:

kernelCI also found failures on a few a10/a20 platfforms[1], and they
all fail to reach userspace, similar to what Marc reported.

I bisected on a sun7i-a20-bananapi and that pointed at this commit
also (in mainline as 9a8e1e8cc2c0 mmc: sunxi: Add runtime_pm support)

Kevin

[1] https://kernelci.org/boot/all/job/mainline/branch/master/kernel/v4.17-11928-g2837461dbe6f/
[2] $ git bisect log
git bisect start
# good: [5037be168f0e4ee910602935b1180291082d3aac] Merge branch
'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux
git bisect good 5037be168f0e4ee910602935b1180291082d3aac
# bad: [f60342fac9fae20ada2cd5faadbc2a1337cae03f] Merge tag
'mmc-v4.18' of git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc
git bisect bad f60342fac9fae20ada2cd5faadbc2a1337cae03f
# good: [fd59ccc53062964007beda8787ffd9cd93968d63] Merge tag
'fscrypt_for_linus' of
git://git.kernel.org/pub/scm/linux/kernel/git/tytso/fscrypt
git bisect good fd59ccc53062964007beda8787ffd9cd93968d63
# good: [2158091d9cda6f126f71973667e8a9fc1e795d03] Merge branch
'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input
git bisect good 2158091d9cda6f126f71973667e8a9fc1e795d03
# bad: [bfd694d5e21c2f0d344db6afeaf993bb0f299545] mmc: core: Add
tunable delay before detecting card after card is inserted
git bisect bad bfd694d5e21c2f0d344db6afeaf993bb0f299545
# good: [743b819e4178935e3f098e5f13db301f532fa9e0] mmc: sunxi: Reorder
the headers
git bisect good 743b819e4178935e3f098e5f13db301f532fa9e0
# bad: [ebca50dfae525341c48c2f69798667352318549e] mmc:
renesas_sdhi_internal_dmac: remove superfluous WARN
git bisect bad ebca50dfae525341c48c2f69798667352318549e
# bad: [eef797ac13c08fae0f0ce7d2215d0951e884fa2d] mmc: sunxi: Drop the
init / reset of the controller from set_ios
git bisect bad eef797ac13c08fae0f0ce7d2215d0951e884fa2d
# good: [ad04d9555da02c719de25b7d1e81ea8d0d2c4838] mmc: sunxi: Move
clock configuration to a function
git bisect good ad04d9555da02c719de25b7d1e81ea8d0d2c4838
# bad: [9a8e1e8cc2c02c57c4e941651a8481a633506c91] mmc: sunxi: Add
runtime_pm support
git bisect bad 9a8e1e8cc2c02c57c4e941651a8481a633506c91
# good: [e27e1f3d04061ccc3735361554088cd7aa286e31] mmc: sunxi: Move
the card power configuration to a function
git bisect good e27e1f3d04061ccc3735361554088cd7aa286e31
# first bad commit: [9a8e1e8cc2c02c57c4e941651a8481a633506c91] mmc:
sunxi: Add runtime_pm support

[PATCH v3 6/7] mmc: sunxi: Add runtime_pm support

[Kevin Hilman]

On Thu, Jun 14, 2018 at 7:12 AM Marc Zyngier <marc.zyngier@arm.com> wrote:
>
> Hi Maxime,
>
> On 16/04/18 15:23, Maxime Ripard wrote:
> > So far, even if our card was not in use, we didn't shut down our MMC
> > controller, which meant that it was still active and clocking the bus.
> >
> > While this obviously means that we could save some power there, it also
> > creates issues when it comes to 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
> > controller when it's not been in use for quite some time.
> >
[...]

> This patch has the unfortunate impact of killing my A20 system
> (cubietruck), as of 9a8e1e8cc2c02c57c4e941651a8481a633506c91:

kernelCI also found failures on a few a10/a20 platfforms[1], and they
all fail to reach userspace, similar to what Marc reported.

I bisected on a sun7i-a20-bananapi and that pointed at this commit
also (in mainline as 9a8e1e8cc2c0 mmc: sunxi: Add runtime_pm support)

Kevin

[1] https://kernelci.org/boot/all/job/mainline/branch/master/kernel/v4.17-11928-g2837461dbe6f/
[2] $ git bisect log
git bisect start
# good: [5037be168f0e4ee910602935b1180291082d3aac] Merge branch
'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux
git bisect good 5037be168f0e4ee910602935b1180291082d3aac
# bad: [f60342fac9fae20ada2cd5faadbc2a1337cae03f] Merge tag
'mmc-v4.18' of git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc
git bisect bad f60342fac9fae20ada2cd5faadbc2a1337cae03f
# good: [fd59ccc53062964007beda8787ffd9cd93968d63] Merge tag
'fscrypt_for_linus' of
git://git.kernel.org/pub/scm/linux/kernel/git/tytso/fscrypt
git bisect good fd59ccc53062964007beda8787ffd9cd93968d63
# good: [2158091d9cda6f126f71973667e8a9fc1e795d03] Merge branch
'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input
git bisect good 2158091d9cda6f126f71973667e8a9fc1e795d03
# bad: [bfd694d5e21c2f0d344db6afeaf993bb0f299545] mmc: core: Add
tunable delay before detecting card after card is inserted
git bisect bad bfd694d5e21c2f0d344db6afeaf993bb0f299545
# good: [743b819e4178935e3f098e5f13db301f532fa9e0] mmc: sunxi: Reorder
the headers
git bisect good 743b819e4178935e3f098e5f13db301f532fa9e0
# bad: [ebca50dfae525341c48c2f69798667352318549e] mmc:
renesas_sdhi_internal_dmac: remove superfluous WARN
git bisect bad ebca50dfae525341c48c2f69798667352318549e
# bad: [eef797ac13c08fae0f0ce7d2215d0951e884fa2d] mmc: sunxi: Drop the
init / reset of the controller from set_ios
git bisect bad eef797ac13c08fae0f0ce7d2215d0951e884fa2d
# good: [ad04d9555da02c719de25b7d1e81ea8d0d2c4838] mmc: sunxi: Move
clock configuration to a function
git bisect good ad04d9555da02c719de25b7d1e81ea8d0d2c4838
# bad: [9a8e1e8cc2c02c57c4e941651a8481a633506c91] mmc: sunxi: Add
runtime_pm support
git bisect bad 9a8e1e8cc2c02c57c4e941651a8481a633506c91
# good: [e27e1f3d04061ccc3735361554088cd7aa286e31] mmc: sunxi: Move
the card power configuration to a function
git bisect good e27e1f3d04061ccc3735361554088cd7aa286e31
# first bad commit: [9a8e1e8cc2c02c57c4e941651a8481a633506c91] mmc:
sunxi: Add runtime_pm support

Re: [PATCH v3 6/7] mmc: sunxi: Add runtime_pm support

[Ulf Hansson]

On 14 June 2018 at 16:11, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Hi Maxime,
>
> On 16/04/18 15:23, Maxime Ripard wrote:
>> So far, even if our card was not in use, we didn't shut down our MMC
>> controller, which meant that it was still active and clocking the bus.
>>
>> While this obviously means that we could save some power there, it also
>> creates issues when it comes to 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
>> controller 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 | 48 +++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 48 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
>> index 0165da0d022a..0253deb153a4 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>
>> @@ -969,6 +970,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);
>> @@ -981,6 +985,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)
>> @@ -1394,6 +1401,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;
>> @@ -1414,6 +1426,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);
>> @@ -1422,10 +1435,45 @@ 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_init_host(host);
>> +     sunxi_mmc_set_bus_width(host, mmc->ios.bus_width);
>> +     sunxi_mmc_set_clk(host, &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_reset_host(host);
>> +     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,
>>
>
> This patch has the unfortunate impact of killing my A20 system
> (cubietruck), as of 9a8e1e8cc2c02c57c4e941651a8481a633506c91:
>
> [...]
> [    3.286649] NET: Registered protocol family 10
> [    3.291898]  mmcblk0: p1
> [    3.295297] mmc1: queuing unknown CIS tuple 0x80 (2 bytes)
> [    3.302773] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
> [    3.305787] Segment Routing with IPv6
> [    3.312225] mip6: Mobile IPv6
> [    3.316166] sit: IPv6, IPv4 and MPLS over IPv4 tunneling driver
> [    3.316246] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
> [    3.323721] ip6_gre: GRE over IPv6 tunneling driver
> [    3.333954] NET: Registered protocol family 17
> [    3.338837] 9pnet: Installing 9P2000 support
> [    3.343379] NET: Registered protocol family 37
> [    3.347885] Key type dns_resolver registered
> [    3.352214] mmc1: queuing unknown CIS tuple 0x80 (7 bytes)
> [    3.352217] openvswitch: Open vSwitch switching datapath
> [    3.352620] mpls_gso: MPLS GSO support
> [    3.367001] ThumbEE CPU extension supported.
>
> and that's where it stops. No message, just a hard lockup (I suppose
> that both the CPUs are trying to access some device that is now not
> clocked).

Seems like a reasonable analyze.

>
> With a working kernel, I see SATA and the wifi SDIO being probed.
>
> Happy to help testing stuff if you have any idea.

In principle I would start with avoiding having the sunxi-mmc driver
to probe. Or bail out early in probe, whichever is the easiest for
you.

The point is, if the sunxi-mmc driver doesn't even enable its clock,
it would be interesting to see if there are other that depends on it.

One could also play with clk_disable_unused(), the
late_initcall_sync(), which can be turned off with the module
parameter "clk_ignore_unused".

Anyway, to hide/fix the problem for now, we could add a call to
pm_runtime_get_noresume() before the sunxi-driver calls
pm_runtime_enable().

Kind regards
Uffe

[PATCH v3 6/7] mmc: sunxi: Add runtime_pm support

[Ulf Hansson]

On 14 June 2018 at 16:11, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Hi Maxime,
>
> On 16/04/18 15:23, Maxime Ripard wrote:
>> So far, even if our card was not in use, we didn't shut down our MMC
>> controller, which meant that it was still active and clocking the bus.
>>
>> While this obviously means that we could save some power there, it also
>> creates issues when it comes to 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
>> controller 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 | 48 +++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 48 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
>> index 0165da0d022a..0253deb153a4 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>
>> @@ -969,6 +970,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);
>> @@ -981,6 +985,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)
>> @@ -1394,6 +1401,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;
>> @@ -1414,6 +1426,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);
>> @@ -1422,10 +1435,45 @@ 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_init_host(host);
>> +     sunxi_mmc_set_bus_width(host, mmc->ios.bus_width);
>> +     sunxi_mmc_set_clk(host, &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_reset_host(host);
>> +     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,
>>
>
> This patch has the unfortunate impact of killing my A20 system
> (cubietruck), as of 9a8e1e8cc2c02c57c4e941651a8481a633506c91:
>
> [...]
> [    3.286649] NET: Registered protocol family 10
> [    3.291898]  mmcblk0: p1
> [    3.295297] mmc1: queuing unknown CIS tuple 0x80 (2 bytes)
> [    3.302773] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
> [    3.305787] Segment Routing with IPv6
> [    3.312225] mip6: Mobile IPv6
> [    3.316166] sit: IPv6, IPv4 and MPLS over IPv4 tunneling driver
> [    3.316246] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
> [    3.323721] ip6_gre: GRE over IPv6 tunneling driver
> [    3.333954] NET: Registered protocol family 17
> [    3.338837] 9pnet: Installing 9P2000 support
> [    3.343379] NET: Registered protocol family 37
> [    3.347885] Key type dns_resolver registered
> [    3.352214] mmc1: queuing unknown CIS tuple 0x80 (7 bytes)
> [    3.352217] openvswitch: Open vSwitch switching datapath
> [    3.352620] mpls_gso: MPLS GSO support
> [    3.367001] ThumbEE CPU extension supported.
>
> and that's where it stops. No message, just a hard lockup (I suppose
> that both the CPUs are trying to access some device that is now not
> clocked).

Seems like a reasonable analyze.

>
> With a working kernel, I see SATA and the wifi SDIO being probed.
>
> Happy to help testing stuff if you have any idea.

In principle I would start with avoiding having the sunxi-mmc driver
to probe. Or bail out early in probe, whichever is the easiest for
you.

The point is, if the sunxi-mmc driver doesn't even enable its clock,
it would be interesting to see if there are other that depends on it.

One could also play with clk_disable_unused(), the
late_initcall_sync(), which can be turned off with the module
parameter "clk_ignore_unused".

Anyway, to hide/fix the problem for now, we could add a call to
pm_runtime_get_noresume() before the sunxi-driver calls
pm_runtime_enable().

Kind regards
Uffe

Re: [PATCH v3 6/7] mmc: sunxi: Add runtime_pm support

[Kevin Hilman]

On Fri, Jun 15, 2018 at 1:56 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On 14 June 2018 at 16:11, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > Hi Maxime,
> >
> > On 16/04/18 15:23, Maxime Ripard wrote:
> >> So far, even if our card was not in use, we didn't shut down our MMC
> >> controller, which meant that it was still active and clocking the bus.
> >>
> >> While this obviously means that we could save some power there, it also
> >> creates issues when it comes to 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
> >> controller 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 | 48 +++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 48 insertions(+)
> >>
> >> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> >> index 0165da0d022a..0253deb153a4 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>
> >> @@ -969,6 +970,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);
> >> @@ -981,6 +985,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)
> >> @@ -1394,6 +1401,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;
> >> @@ -1414,6 +1426,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);
> >> @@ -1422,10 +1435,45 @@ 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_init_host(host);
> >> +     sunxi_mmc_set_bus_width(host, mmc->ios.bus_width);
> >> +     sunxi_mmc_set_clk(host, &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_reset_host(host);
> >> +     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,
> >>
> >
> > This patch has the unfortunate impact of killing my A20 system
> > (cubietruck), as of 9a8e1e8cc2c02c57c4e941651a8481a633506c91:
> >
> > [...]
> > [    3.286649] NET: Registered protocol family 10
> > [    3.291898]  mmcblk0: p1
> > [    3.295297] mmc1: queuing unknown CIS tuple 0x80 (2 bytes)
> > [    3.302773] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
> > [    3.305787] Segment Routing with IPv6
> > [    3.312225] mip6: Mobile IPv6
> > [    3.316166] sit: IPv6, IPv4 and MPLS over IPv4 tunneling driver
> > [    3.316246] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
> > [    3.323721] ip6_gre: GRE over IPv6 tunneling driver
> > [    3.333954] NET: Registered protocol family 17
> > [    3.338837] 9pnet: Installing 9P2000 support
> > [    3.343379] NET: Registered protocol family 37
> > [    3.347885] Key type dns_resolver registered
> > [    3.352214] mmc1: queuing unknown CIS tuple 0x80 (7 bytes)
> > [    3.352217] openvswitch: Open vSwitch switching datapath
> > [    3.352620] mpls_gso: MPLS GSO support
> > [    3.367001] ThumbEE CPU extension supported.
> >
> > and that's where it stops. No message, just a hard lockup (I suppose
> > that both the CPUs are trying to access some device that is now not
> > clocked).
>
> Seems like a reasonable analyze.
>
> >
> > With a working kernel, I see SATA and the wifi SDIO being probed.
> >
> > Happy to help testing stuff if you have any idea.
>
> In principle I would start with avoiding having the sunxi-mmc driver
> to probe. Or bail out early in probe, whichever is the easiest for
> you.
>
> The point is, if the sunxi-mmc driver doesn't even enable its clock,
> it would be interesting to see if there are other that depends on it.
>
> One could also play with clk_disable_unused(), the
> late_initcall_sync(), which can be turned off with the module
> parameter "clk_ignore_unused".

I added clk_ignore_unused to the kernel command-line, and that didn't
help, so it's not just an init-time clock that's causing the problem.

> Anyway, to hide/fix the problem for now, we could add a call to
> pm_runtime_get_noresume() before the sunxi-driver calls
> pm_runtime_enable().

I tried that and it makes the kernel finish booting, so that smells
definitely like the MMC is disabling a clock when it goes idle that
some other device (or CPU) depends on.

Kevin

[PATCH v3 6/7] mmc: sunxi: Add runtime_pm support

[Kevin Hilman]

On Fri, Jun 15, 2018 at 1:56 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On 14 June 2018 at 16:11, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > Hi Maxime,
> >
> > On 16/04/18 15:23, Maxime Ripard wrote:
> >> So far, even if our card was not in use, we didn't shut down our MMC
> >> controller, which meant that it was still active and clocking the bus.
> >>
> >> While this obviously means that we could save some power there, it also
> >> creates issues when it comes to 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
> >> controller 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 | 48 +++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 48 insertions(+)
> >>
> >> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> >> index 0165da0d022a..0253deb153a4 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>
> >> @@ -969,6 +970,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);
> >> @@ -981,6 +985,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)
> >> @@ -1394,6 +1401,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;
> >> @@ -1414,6 +1426,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);
> >> @@ -1422,10 +1435,45 @@ 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_init_host(host);
> >> +     sunxi_mmc_set_bus_width(host, mmc->ios.bus_width);
> >> +     sunxi_mmc_set_clk(host, &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_reset_host(host);
> >> +     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,
> >>
> >
> > This patch has the unfortunate impact of killing my A20 system
> > (cubietruck), as of 9a8e1e8cc2c02c57c4e941651a8481a633506c91:
> >
> > [...]
> > [    3.286649] NET: Registered protocol family 10
> > [    3.291898]  mmcblk0: p1
> > [    3.295297] mmc1: queuing unknown CIS tuple 0x80 (2 bytes)
> > [    3.302773] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
> > [    3.305787] Segment Routing with IPv6
> > [    3.312225] mip6: Mobile IPv6
> > [    3.316166] sit: IPv6, IPv4 and MPLS over IPv4 tunneling driver
> > [    3.316246] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
> > [    3.323721] ip6_gre: GRE over IPv6 tunneling driver
> > [    3.333954] NET: Registered protocol family 17
> > [    3.338837] 9pnet: Installing 9P2000 support
> > [    3.343379] NET: Registered protocol family 37
> > [    3.347885] Key type dns_resolver registered
> > [    3.352214] mmc1: queuing unknown CIS tuple 0x80 (7 bytes)
> > [    3.352217] openvswitch: Open vSwitch switching datapath
> > [    3.352620] mpls_gso: MPLS GSO support
> > [    3.367001] ThumbEE CPU extension supported.
> >
> > and that's where it stops. No message, just a hard lockup (I suppose
> > that both the CPUs are trying to access some device that is now not
> > clocked).
>
> Seems like a reasonable analyze.
>
> >
> > With a working kernel, I see SATA and the wifi SDIO being probed.
> >
> > Happy to help testing stuff if you have any idea.
>
> In principle I would start with avoiding having the sunxi-mmc driver
> to probe. Or bail out early in probe, whichever is the easiest for
> you.
>
> The point is, if the sunxi-mmc driver doesn't even enable its clock,
> it would be interesting to see if there are other that depends on it.
>
> One could also play with clk_disable_unused(), the
> late_initcall_sync(), which can be turned off with the module
> parameter "clk_ignore_unused".

I added clk_ignore_unused to the kernel command-line, and that didn't
help, so it's not just an init-time clock that's causing the problem.

> Anyway, to hide/fix the problem for now, we could add a call to
> pm_runtime_get_noresume() before the sunxi-driver calls
> pm_runtime_enable().

I tried that and it makes the kernel finish booting, so that smells
definitely like the MMC is disabling a clock when it goes idle that
some other device (or CPU) depends on.

Kevin

Re: [PATCH v3 6/7] mmc: sunxi: Add runtime_pm support

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1630 bytes --]

On Fri, Jun 15, 2018 at 07:45:03AM -0700, Kevin Hilman wrote:
> > >
> > > With a working kernel, I see SATA and the wifi SDIO being probed.
> > >
> > > Happy to help testing stuff if you have any idea.
> >
> > In principle I would start with avoiding having the sunxi-mmc driver
> > to probe. Or bail out early in probe, whichever is the easiest for
> > you.
> >
> > The point is, if the sunxi-mmc driver doesn't even enable its clock,
> > it would be interesting to see if there are other that depends on it.
> >
> > One could also play with clk_disable_unused(), the
> > late_initcall_sync(), which can be turned off with the module
> > parameter "clk_ignore_unused".
> 
> I added clk_ignore_unused to the kernel command-line, and that didn't
> help, so it's not just an init-time clock that's causing the problem.
> 
> > Anyway, to hide/fix the problem for now, we could add a call to
> > pm_runtime_get_noresume() before the sunxi-driver calls
> > pm_runtime_enable().
> 
> I tried that and it makes the kernel finish booting, so that smells
> definitely like the MMC is disabling a clock when it goes idle that
> some other device (or CPU) depends on.

I quickly looked at the A10 and A20 clock driver and I have not seen
any obvious mishap.

If you have ftrace enabled, could you add the trace_clk_disable*
events (along with tp_printk since the kernel seems to break the
entire boot).

That will allow us to see which clock is disabled and shouldn't.

Thanks!
Maxime

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

[PATCH v3 6/7] mmc: sunxi: Add runtime_pm support

[Maxime Ripard]

On Fri, Jun 15, 2018 at 07:45:03AM -0700, Kevin Hilman wrote:
> > >
> > > With a working kernel, I see SATA and the wifi SDIO being probed.
> > >
> > > Happy to help testing stuff if you have any idea.
> >
> > In principle I would start with avoiding having the sunxi-mmc driver
> > to probe. Or bail out early in probe, whichever is the easiest for
> > you.
> >
> > The point is, if the sunxi-mmc driver doesn't even enable its clock,
> > it would be interesting to see if there are other that depends on it.
> >
> > One could also play with clk_disable_unused(), the
> > late_initcall_sync(), which can be turned off with the module
> > parameter "clk_ignore_unused".
> 
> I added clk_ignore_unused to the kernel command-line, and that didn't
> help, so it's not just an init-time clock that's causing the problem.
> 
> > Anyway, to hide/fix the problem for now, we could add a call to
> > pm_runtime_get_noresume() before the sunxi-driver calls
> > pm_runtime_enable().
> 
> I tried that and it makes the kernel finish booting, so that smells
> definitely like the MMC is disabling a clock when it goes idle that
> some other device (or CPU) depends on.

I quickly looked at the A10 and A20 clock driver and I have not seen
any obvious mishap.

If you have ftrace enabled, could you add the trace_clk_disable*
events (along with tp_printk since the kernel seems to break the
entire boot).

That will allow us to see which clock is disabled and shouldn't.

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/20180615/9d42e687/attachment.sig>

Re: [PATCH v3 6/7] mmc: sunxi: Add runtime_pm support

[Maxime Ripard]

On Fri, Jun 15, 2018 at 07:45:03AM -0700, Kevin Hilman wrote:
> On Fri, Jun 15, 2018 at 1:56 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On 14 June 2018 at 16:11, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > > Hi Maxime,
> > >
> > > On 16/04/18 15:23, Maxime Ripard wrote:
> > >> So far, even if our card was not in use, we didn't shut down our MMC
> > >> controller, which meant that it was still active and clocking the bus.
> > >>
> > >> While this obviously means that we could save some power there, it also
> > >> creates issues when it comes to 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
> > >> controller 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 | 48 +++++++++++++++++++++++++++++++++++++-
> > >>  1 file changed, 48 insertions(+)
> > >>
> > >> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> > >> index 0165da0d022a..0253deb153a4 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>
> > >> @@ -969,6 +970,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);
> > >> @@ -981,6 +985,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)
> > >> @@ -1394,6 +1401,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;
> > >> @@ -1414,6 +1426,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);
> > >> @@ -1422,10 +1435,45 @@ 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_init_host(host);
> > >> +     sunxi_mmc_set_bus_width(host, mmc->ios.bus_width);
> > >> +     sunxi_mmc_set_clk(host, &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_reset_host(host);
> > >> +     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,
> > >>
> > >
> > > This patch has the unfortunate impact of killing my A20 system
> > > (cubietruck), as of 9a8e1e8cc2c02c57c4e941651a8481a633506c91:
> > >
> > > [...]
> > > [    3.286649] NET: Registered protocol family 10
> > > [    3.291898]  mmcblk0: p1
> > > [    3.295297] mmc1: queuing unknown CIS tuple 0x80 (2 bytes)
> > > [    3.302773] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
> > > [    3.305787] Segment Routing with IPv6
> > > [    3.312225] mip6: Mobile IPv6
> > > [    3.316166] sit: IPv6, IPv4 and MPLS over IPv4 tunneling driver
> > > [    3.316246] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
> > > [    3.323721] ip6_gre: GRE over IPv6 tunneling driver
> > > [    3.333954] NET: Registered protocol family 17
> > > [    3.338837] 9pnet: Installing 9P2000 support
> > > [    3.343379] NET: Registered protocol family 37
> > > [    3.347885] Key type dns_resolver registered
> > > [    3.352214] mmc1: queuing unknown CIS tuple 0x80 (7 bytes)
> > > [    3.352217] openvswitch: Open vSwitch switching datapath
> > > [    3.352620] mpls_gso: MPLS GSO support
> > > [    3.367001] ThumbEE CPU extension supported.
> > >
> > > and that's where it stops. No message, just a hard lockup (I suppose
> > > that both the CPUs are trying to access some device that is now not
> > > clocked).
> >
> > Seems like a reasonable analyze.
> >
> > >
> > > With a working kernel, I see SATA and the wifi SDIO being probed.
> > >
> > > Happy to help testing stuff if you have any idea.
> >
> > In principle I would start with avoiding having the sunxi-mmc driver
> > to probe. Or bail out early in probe, whichever is the easiest for
> > you.
> >
> > The point is, if the sunxi-mmc driver doesn't even enable its clock,
> > it would be interesting to see if there are other that depends on it.
> >
> > One could also play with clk_disable_unused(), the
> > late_initcall_sync(), which can be turned off with the module
> > parameter "clk_ignore_unused".
> 
> I added clk_ignore_unused to the kernel command-line, and that didn't
> help, so it's not just an init-time clock that's causing the problem.
> 
> > Anyway, to hide/fix the problem for now, we could add a call to
> > pm_runtime_get_noresume() before the sunxi-driver calls
> > pm_runtime_enable().
> 
> I tried that and it makes the kernel finish booting, so that smells
> definitely like the MMC is disabling a clock when it goes idle that
> some other device (or CPU) depends on.

I've looked into it, and I can't seem to reproduce it on a 4.18-rc2
kernel with my cubieboard2, booting from NFS and trying to access the
SD card.

Marc is booting from SATA, but apparently your boards Kevin are
booting from initramfs?

Apart from the ftrace option, can you add a call in the probe to
clk_prepare_enable for clk_ahb and clk_mmc clocks, and see if there's
one that fixes the issue? the output and sample clocks are not
gatable, so it shouldn't cause any trouble.

Maxime

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

[PATCH v3 6/7] mmc: sunxi: Add runtime_pm support

[Maxime Ripard]

On Fri, Jun 15, 2018 at 07:45:03AM -0700, Kevin Hilman wrote:
> On Fri, Jun 15, 2018 at 1:56 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On 14 June 2018 at 16:11, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > > Hi Maxime,
> > >
> > > On 16/04/18 15:23, Maxime Ripard wrote:
> > >> So far, even if our card was not in use, we didn't shut down our MMC
> > >> controller, which meant that it was still active and clocking the bus.
> > >>
> > >> While this obviously means that we could save some power there, it also
> > >> creates issues when it comes to 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
> > >> controller 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 | 48 +++++++++++++++++++++++++++++++++++++-
> > >>  1 file changed, 48 insertions(+)
> > >>
> > >> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> > >> index 0165da0d022a..0253deb153a4 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>
> > >> @@ -969,6 +970,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);
> > >> @@ -981,6 +985,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)
> > >> @@ -1394,6 +1401,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;
> > >> @@ -1414,6 +1426,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);
> > >> @@ -1422,10 +1435,45 @@ 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_init_host(host);
> > >> +     sunxi_mmc_set_bus_width(host, mmc->ios.bus_width);
> > >> +     sunxi_mmc_set_clk(host, &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_reset_host(host);
> > >> +     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,
> > >>
> > >
> > > This patch has the unfortunate impact of killing my A20 system
> > > (cubietruck), as of 9a8e1e8cc2c02c57c4e941651a8481a633506c91:
> > >
> > > [...]
> > > [    3.286649] NET: Registered protocol family 10
> > > [    3.291898]  mmcblk0: p1
> > > [    3.295297] mmc1: queuing unknown CIS tuple 0x80 (2 bytes)
> > > [    3.302773] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
> > > [    3.305787] Segment Routing with IPv6
> > > [    3.312225] mip6: Mobile IPv6
> > > [    3.316166] sit: IPv6, IPv4 and MPLS over IPv4 tunneling driver
> > > [    3.316246] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
> > > [    3.323721] ip6_gre: GRE over IPv6 tunneling driver
> > > [    3.333954] NET: Registered protocol family 17
> > > [    3.338837] 9pnet: Installing 9P2000 support
> > > [    3.343379] NET: Registered protocol family 37
> > > [    3.347885] Key type dns_resolver registered
> > > [    3.352214] mmc1: queuing unknown CIS tuple 0x80 (7 bytes)
> > > [    3.352217] openvswitch: Open vSwitch switching datapath
> > > [    3.352620] mpls_gso: MPLS GSO support
> > > [    3.367001] ThumbEE CPU extension supported.
> > >
> > > and that's where it stops. No message, just a hard lockup (I suppose
> > > that both the CPUs are trying to access some device that is now not
> > > clocked).
> >
> > Seems like a reasonable analyze.
> >
> > >
> > > With a working kernel, I see SATA and the wifi SDIO being probed.
> > >
> > > Happy to help testing stuff if you have any idea.
> >
> > In principle I would start with avoiding having the sunxi-mmc driver
> > to probe. Or bail out early in probe, whichever is the easiest for
> > you.
> >
> > The point is, if the sunxi-mmc driver doesn't even enable its clock,
> > it would be interesting to see if there are other that depends on it.
> >
> > One could also play with clk_disable_unused(), the
> > late_initcall_sync(), which can be turned off with the module
> > parameter "clk_ignore_unused".
> 
> I added clk_ignore_unused to the kernel command-line, and that didn't
> help, so it's not just an init-time clock that's causing the problem.
> 
> > Anyway, to hide/fix the problem for now, we could add a call to
> > pm_runtime_get_noresume() before the sunxi-driver calls
> > pm_runtime_enable().
> 
> I tried that and it makes the kernel finish booting, so that smells
> definitely like the MMC is disabling a clock when it goes idle that
> some other device (or CPU) depends on.

I've looked into it, and I can't seem to reproduce it on a 4.18-rc2
kernel with my cubieboard2, booting from NFS and trying to access the
SD card.

Marc is booting from SATA, but apparently your boards Kevin are
booting from initramfs?

Apart from the ftrace option, can you add a call in the probe to
clk_prepare_enable for clk_ahb and clk_mmc clocks, and see if there's
one that fixes the issue? the output and sample clocks are not
gatable, so it shouldn't cause any trouble.

Maxime

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

[PATCH v3 7/7] mmc: sunxi: Drop the init / reset of the controller from set_ios

[Maxime Ripard]

Our set_ios hook is, when the card is power up or down, either doing a full
init or put our controller back into a reset mode.

Since we're also doing that in our runtime_pm hooks, and at possibly much
more often, we can drop it from the set_ios, and either rely on our
runtime_pm hooks or our probe to do it.

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

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 0253deb153a4..97c6b79b7d6f 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -939,14 +939,7 @@ static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct sunxi_mmc_host *host = mmc_priv(mmc);
 
-	if (ios->power_mode == MMC_POWER_OFF)
-		sunxi_mmc_reset_host(host);
-
 	sunxi_mmc_card_power(host, ios);
-
-	if (ios->power_mode == MMC_POWER_UP)
-		sunxi_mmc_init_host(host);
-
 	sunxi_mmc_set_bus_width(host, ios->bus_width);
 	sunxi_mmc_set_clk(host, ios);
 }
@@ -1401,6 +1394,10 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
 	if (ret)
 		goto error_free_dma;
 
+	ret = sunxi_mmc_init_host(host);
+	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);
-- 
git-series 0.9.1

[PATCH v3 7/7] mmc: sunxi: Drop the init / reset of the controller from set_ios

[Maxime Ripard]

Our set_ios hook is, when the card is power up or down, either doing a full
init or put our controller back into a reset mode.

Since we're also doing that in our runtime_pm hooks, and at possibly much
more often, we can drop it from the set_ios, and either rely on our
runtime_pm hooks or our probe to do it.

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

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 0253deb153a4..97c6b79b7d6f 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -939,14 +939,7 @@ static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct sunxi_mmc_host *host = mmc_priv(mmc);
 
-	if (ios->power_mode == MMC_POWER_OFF)
-		sunxi_mmc_reset_host(host);
-
 	sunxi_mmc_card_power(host, ios);
-
-	if (ios->power_mode == MMC_POWER_UP)
-		sunxi_mmc_init_host(host);
-
 	sunxi_mmc_set_bus_width(host, ios->bus_width);
 	sunxi_mmc_set_clk(host, ios);
 }
@@ -1401,6 +1394,10 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
 	if (ret)
 		goto error_free_dma;
 
+	ret = sunxi_mmc_init_host(host);
+	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);
-- 
git-series 0.9.1

Re: [PATCH v3 0/7] mmc: sunxi: Add runtime PM support

[Ulf Hansson]

On 16 April 2018 at 16:22, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> 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
>

Nice work! Applied for next!

Kind regards
Uffe

> Changes from v2:
>   - Dropped the patches already applied
>   - Do not power up or down the card during a runtime_resume or
>     runtime_suspend, but only take care of the controller
>   - In order to do the above, split the set_ios function into smaller
>     chunks that can be called from set_ios and our runtime_resume hook
>
> 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 (7):
>   mmc: sunxi: Reorder the headers
>   mmc: sunxi: Change sunxi_mmc_init_host argument type
>   mmc: sunxi: Move bus width configuration to a function
>   mmc: sunxi: Move clock configuration to a function
>   mmc: sunxi: Move the card power configuration to a function
>   mmc: sunxi: Add runtime_pm support
>   mmc: sunxi: Drop the init / reset of the controller from set_ios
>
>  drivers/mmc/host/sunxi-mmc.c | 185 +++++++++++++++++++++++-------------
>  1 file changed, 122 insertions(+), 63 deletions(-)
>
> base-commit: 60cc43fc888428bb2f18f08997432d426a243338
> --
> git-series 0.9.1

[PATCH v3 0/7] mmc: sunxi: Add runtime PM support

[Ulf Hansson]

On 16 April 2018 at 16:22, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> 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
>

Nice work! Applied for next!

Kind regards
Uffe

> Changes from v2:
>   - Dropped the patches already applied
>   - Do not power up or down the card during a runtime_resume or
>     runtime_suspend, but only take care of the controller
>   - In order to do the above, split the set_ios function into smaller
>     chunks that can be called from set_ios and our runtime_resume hook
>
> 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 (7):
>   mmc: sunxi: Reorder the headers
>   mmc: sunxi: Change sunxi_mmc_init_host argument type
>   mmc: sunxi: Move bus width configuration to a function
>   mmc: sunxi: Move clock configuration to a function
>   mmc: sunxi: Move the card power configuration to a function
>   mmc: sunxi: Add runtime_pm support
>   mmc: sunxi: Drop the init / reset of the controller from set_ios
>
>  drivers/mmc/host/sunxi-mmc.c | 185 +++++++++++++++++++++++-------------
>  1 file changed, 122 insertions(+), 63 deletions(-)
>
> base-commit: 60cc43fc888428bb2f18f08997432d426a243338
> --
> git-series 0.9.1

Re: [PATCH v3 0/7] mmc: sunxi: Add runtime PM support

[Ondřej Jirman]

Hello Maxime,

On Mon, Apr 16, 2018 at 04:22:58PM +0200, Maxime Ripard wrote:
> 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.

I've tested your patches on TBS A711 tablet (A83T). I get this in the log:

  sunxi-mmc 1c10000.mmc: fatal err update clk timeout

Interestingly, this only happens after the first boot. If I hard power cycle the
tablet (removing/reinserting the battery) the first boot after that, there's no
timeout, and WiFi chip connected to the mmc controller works.

Any other reboot (even power cycling via PMIC), it fails with the above error.
Maybe there's some unforeseen interaction of runtime PM with mmc-powerseq used
for the wifi chip power seuencing? Do you know what's the above error about?
I don't know anything about mmc subsystem.

Reverting "mmc: sunxi: Add runtime_pm support" (+ various cleanup patches
afterwards) helps.

thank you and regards,
  o.

> Let me know what you think,
> Maxime
> 
> Changes from v2:
>   - Dropped the patches already applied
>   - Do not power up or down the card during a runtime_resume or
>     runtime_suspend, but only take care of the controller
>   - In order to do the above, split the set_ios function into smaller
>     chunks that can be called from set_ios and our runtime_resume hook
> 
> 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 (7):
>   mmc: sunxi: Reorder the headers
>   mmc: sunxi: Change sunxi_mmc_init_host argument type
>   mmc: sunxi: Move bus width configuration to a function
>   mmc: sunxi: Move clock configuration to a function
>   mmc: sunxi: Move the card power configuration to a function
>   mmc: sunxi: Add runtime_pm support
>   mmc: sunxi: Drop the init / reset of the controller from set_ios
> 
>  drivers/mmc/host/sunxi-mmc.c | 185 +++++++++++++++++++++++-------------
>  1 file changed, 122 insertions(+), 63 deletions(-)
> 
> base-commit: 60cc43fc888428bb2f18f08997432d426a243338
> -- 
> git-series 0.9.1
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 0/7] mmc: sunxi: Add runtime PM support

[Ondřej Jirman]

Hello Maxime,

On Mon, Apr 16, 2018 at 04:22:58PM +0200, Maxime Ripard wrote:
> 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.

I've tested your patches on TBS A711 tablet (A83T). I get this in the log:

  sunxi-mmc 1c10000.mmc: fatal err update clk timeout

Interestingly, this only happens after the first boot. If I hard power cycle the
tablet (removing/reinserting the battery) the first boot after that, there's no
timeout, and WiFi chip connected to the mmc controller works.

Any other reboot (even power cycling via PMIC), it fails with the above error.
Maybe there's some unforeseen interaction of runtime PM with mmc-powerseq used
for the wifi chip power seuencing? Do you know what's the above error about?
I don't know anything about mmc subsystem.

Reverting "mmc: sunxi: Add runtime_pm support" (+ various cleanup patches
afterwards) helps.

thank you and regards,
  o.

> Let me know what you think,
> Maxime
> 
> Changes from v2:
>   - Dropped the patches already applied
>   - Do not power up or down the card during a runtime_resume or
>     runtime_suspend, but only take care of the controller
>   - In order to do the above, split the set_ios function into smaller
>     chunks that can be called from set_ios and our runtime_resume hook
> 
> 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 (7):
>   mmc: sunxi: Reorder the headers
>   mmc: sunxi: Change sunxi_mmc_init_host argument type
>   mmc: sunxi: Move bus width configuration to a function
>   mmc: sunxi: Move clock configuration to a function
>   mmc: sunxi: Move the card power configuration to a function
>   mmc: sunxi: Add runtime_pm support
>   mmc: sunxi: Drop the init / reset of the controller from set_ios
> 
>  drivers/mmc/host/sunxi-mmc.c | 185 +++++++++++++++++++++++-------------
>  1 file changed, 122 insertions(+), 63 deletions(-)
> 
> base-commit: 60cc43fc888428bb2f18f08997432d426a243338
> -- 
> git-series 0.9.1
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 0/7] mmc: sunxi: Add runtime PM support

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1650 bytes --]

Hi Ondrej,

On Mon, Jul 16, 2018 at 07:14:28PM +0200, Ondřej Jirman wrote:
> On Mon, Apr 16, 2018 at 04:22:58PM +0200, Maxime Ripard wrote:
> > 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.
> 
> I've tested your patches on TBS A711 tablet (A83T). I get this in the log:
> 
>   sunxi-mmc 1c10000.mmc: fatal err update clk timeout
> 
> Interestingly, this only happens after the first boot. If I hard power cycle the
> tablet (removing/reinserting the battery) the first boot after that, there's no
> timeout, and WiFi chip connected to the mmc controller works.
> 
> Any other reboot (even power cycling via PMIC), it fails with the above error.
> Maybe there's some unforeseen interaction of runtime PM with mmc-powerseq used
> for the wifi chip power seuencing? Do you know what's the above error about?
> I don't know anything about mmc subsystem.

It's kind of weird. Do you have an oscilloscope available to look at
the clock signal (and the reset gpio) and see if there's any
difference of behaviour?

Thanks!
Maxime

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

[PATCH v3 0/7] mmc: sunxi: Add runtime PM support

[Maxime Ripard]

Hi Ondrej,

On Mon, Jul 16, 2018 at 07:14:28PM +0200, Ond?ej Jirman wrote:
> On Mon, Apr 16, 2018 at 04:22:58PM +0200, Maxime Ripard wrote:
> > 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.
> 
> I've tested your patches on TBS A711 tablet (A83T). I get this in the log:
> 
>   sunxi-mmc 1c10000.mmc: fatal err update clk timeout
> 
> Interestingly, this only happens after the first boot. If I hard power cycle the
> tablet (removing/reinserting the battery) the first boot after that, there's no
> timeout, and WiFi chip connected to the mmc controller works.
> 
> Any other reboot (even power cycling via PMIC), it fails with the above error.
> Maybe there's some unforeseen interaction of runtime PM with mmc-powerseq used
> for the wifi chip power seuencing? Do you know what's the above error about?
> I don't know anything about mmc subsystem.

It's kind of weird. Do you have an oscilloscope available to look at
the clock signal (and the reset gpio) and see if there's any
difference of behaviour?

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/20180720/476fd7a9/attachment.sig>

Re: [PATCH v3 0/7] mmc: sunxi: Add runtime PM support

[Ondřej Jirman]

Hi Maxime,

On Fri, Jul 20, 2018 at 04:51:43PM +0200, Maxime Ripard wrote:
> Hi Ondrej,
> 
> On Mon, Jul 16, 2018 at 07:14:28PM +0200, Ondřej Jirman wrote:
> > On Mon, Apr 16, 2018 at 04:22:58PM +0200, Maxime Ripard wrote:
> > > 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.
> > 
> > I've tested your patches on TBS A711 tablet (A83T). I get this in the log:
> > 
> >   sunxi-mmc 1c10000.mmc: fatal err update clk timeout
> > 
> > Interestingly, this only happens after the first boot. If I hard power cycle the
> > tablet (removing/reinserting the battery) the first boot after that, there's no
> > timeout, and WiFi chip connected to the mmc controller works.
> > 
> > Any other reboot (even power cycling via PMIC), it fails with the above error.
> > Maybe there's some unforeseen interaction of runtime PM with mmc-powerseq used
> > for the wifi chip power seuencing? Do you know what's the above error about?
> > I don't know anything about mmc subsystem.
> 
> It's kind of weird. Do you have an oscilloscope available to look at
> the clock signal (and the reset gpio) and see if there's any
> difference of behaviour?

No, I don't. I can try some printks on the kernel side if it toggles the WiFi
reset/power enable gpios differntly.

It looks like something's different on initialization when booting the kernel.
WiFi works fine for extended periods of time after the hard power cycle. But
when rebooting, it fails.

Adding to the weirdness factor may be the fact that WiFi is not connected via
PMIC, so it gets power almost directly from the battery. So kernel may not be
doing the WiFi reset properly on boot (which is fine right after the hard power
cycle, but leaves the WiFi in unknown state on subsequent reboots).

Thank you and regards,
  o.

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

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

[PATCH v3 0/7] mmc: sunxi: Add runtime PM support

[Ondřej Jirman]

Hi Maxime,

On Fri, Jul 20, 2018 at 04:51:43PM +0200, Maxime Ripard wrote:
> Hi Ondrej,
> 
> On Mon, Jul 16, 2018 at 07:14:28PM +0200, Ond?ej Jirman wrote:
> > On Mon, Apr 16, 2018 at 04:22:58PM +0200, Maxime Ripard wrote:
> > > 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.
> > 
> > I've tested your patches on TBS A711 tablet (A83T). I get this in the log:
> > 
> >   sunxi-mmc 1c10000.mmc: fatal err update clk timeout
> > 
> > Interestingly, this only happens after the first boot. If I hard power cycle the
> > tablet (removing/reinserting the battery) the first boot after that, there's no
> > timeout, and WiFi chip connected to the mmc controller works.
> > 
> > Any other reboot (even power cycling via PMIC), it fails with the above error.
> > Maybe there's some unforeseen interaction of runtime PM with mmc-powerseq used
> > for the wifi chip power seuencing? Do you know what's the above error about?
> > I don't know anything about mmc subsystem.
> 
> It's kind of weird. Do you have an oscilloscope available to look at
> the clock signal (and the reset gpio) and see if there's any
> difference of behaviour?

No, I don't. I can try some printks on the kernel side if it toggles the WiFi
reset/power enable gpios differntly.

It looks like something's different on initialization when booting the kernel.
WiFi works fine for extended periods of time after the hard power cycle. But
when rebooting, it fails.

Adding to the weirdness factor may be the fact that WiFi is not connected via
PMIC, so it gets power almost directly from the battery. So kernel may not be
doing the WiFi reset properly on boot (which is fine right after the hard power
cycle, but leaves the WiFi in unknown state on subsequent reboots).

Thank you and regards,
  o.

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