misc MMC fixes

The 250ms timeout is too short.

On my system enabling the oclk takes under 50ms and disabling slightly over 100ms when idle. Under load disabling the clock can take over 350ms.

This does not make mmc clock gating look like good option to have on sunxi but the system should not crash with mmc clock gating enabled nonetheless.

This patch sets the timeout to 750ms and adds debug prints which show how long enabling/disabling the clock took so more data can be collected from other systems.

Signed-off-by Michal Suchanek <[email protected]>

--

- fix formatting

---
  drivers/mmc/host/sunxi-mmc.c | 22 +++++++++++++++++++---
  1 file changed, 19 insertions(+), 3 deletions(-)

drivers/mmc/host/sunxi-mmc.c +19 -3

index 4d3e1ff..f808a02 100644
@@ -595,7 +595,7 @@ static irqreturn_t sunxi_mmc_handle_manual_stop(int irq, void *dev_id)
 
  static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
  {
-	unsigned long expire = jiffies + msecs_to_jiffies(250);
+	unsigned long start, end;
  	u32 rval;
 
  	rval = mmc_readl(host, REG_CLKCR);

@@ -604,6 +604,8 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
  	if (oclk_en)
  		rval |= SDXC_CARD_CLOCK_ON;
 
+	start = jiffies;
+	end = start + msecs_to_jiffies(750);
  	mmc_writel(host, REG_CLKCR, rval);
 
  	rval = SDXC_START | SDXC_UPCLK_ONLY | SDXC_WAIT_PRE_OVER;

@@ -611,15 +613,29 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
 
  	do {
  		rval = mmc_readl(host, REG_CMDR);
-	} while (time_before(jiffies, expire) && (rval & SDXC_START));
+	} while (time_before(jiffies, end) && (rval & SDXC_START));
+	end = jiffies;
 
  	/* clear irq status bits set by the command */
  	mmc_writel(host, REG_RINTR,
  		   mmc_readl(host, REG_RINTR) & ~SDXC_SDIO_INTERRUPT);
 
  	if (rval & SDXC_START) {
-		dev_err(mmc_dev(host->mmc), "fatal err update clk timeout\n");
+		dev_err(mmc_dev(host->mmc),
+			"fatal err update oclk timeout. Could not %s in %ims.\n",
+			oclk_en ? "enable" : "disable",
+			jiffies_to_msecs(end - start));
  		return -EIO;
+	} else {
+		int msecs = jiffies_to_msecs(end - start);
+		const char *ing = oclk_en ? "enabling" : "disabling";
+
+		if ((msecs > 150) || (oclk_en && (msecs > 50)))
+			dev_warn(mmc_dev(host->mmc),
+				 "%s oclk took %ims", ing, msecs);
+		else
+			dev_dbg(mmc_dev(host->mmc),
+				 "%s oclk took %ims", ing, msecs);
  	}
 
  	return 0;

When core does not set the MMC_QUIRK_BROKEN_CLK_GATING flag enable automatic hardware controlled clock gating on the mmc interface.

Signed-off-by Michal Suchanek <[email protected]>

---
  drivers/mmc/host/sunxi-mmc.c | 7 ++++++-
  1 file changed, 6 insertions(+), 1 deletion(-)

drivers/mmc/host/sunxi-mmc.c +6 -1

index f808a02..443cab5 100644
@@ -601,8 +601,13 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
  	rval = mmc_readl(host, REG_CLKCR);
  	rval &= ~(SDXC_CARD_CLOCK_ON | SDXC_LOW_POWER_ON);
 
-	if (oclk_en)
+	if (oclk_en) {
  		rval |= SDXC_CARD_CLOCK_ON;
+		if (!host->mmc->card ||
+		     !(host->mmc->card->quirks & MMC_QUIRK_BROKEN_CLK_GATING))
+
+			rval |= SDXC_LOW_POWER_ON;
+	}
 
  	start = jiffies;
  	end = start + msecs_to_jiffies(750);

Hi,

Quoted 10 lines · michal.suchanek at ruk.cuni.cz

The 250ms timeout is too short.

On my system enabling the oclk takes under 50ms and disabling slightly over 100ms when idle. Under load disabling the clock can take over 350ms.

This does not make mmc clock gating look like good option to have on sunxi but the system should not crash with mmc clock gating enabled nonetheless.

Quoted 5 lines

This patch sets the timeout to 750ms and adds debug prints which show how long enabling/disabling the clock took so more data can be collected from other systems.

Signed-off-by Michal Suchanek <[email protected]>

This is a big patch for just changing a timeout, most of this is in extra verbosity which IMHO has little value, in the error path w know we will have waited aprox 750 ms, so printing the waiting time there is not worth all the extra code.

As for adding the warning I'm even less of a big fan of that, if we need higher timeouts, we need higher timeouts, spamming the kernel logs with that we are actually hitting the higher timeouts is not productive IMHO.

Can you please resend this as a one-liner just changing the timeout ?

Regards,

Hans

Quoted 67 lines

Signature · 6 lines

-- 

  - fix formatting
---
  drivers/mmc/host/sunxi-mmc.c | 22 +++++++++++++++++++---
  1 file changed, 19 insertions(+), 3 deletions(-)

drivers/mmc/host/sunxi-mmc.c +1 -1

index 4d3e1ff..f808a02 100644
@@ -595,7 +595,7 @@ static irqreturn_t 
 sunxi_mmc_handle_manual_stop(int irq, void *dev_id)
 
  static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 
 oclk_en)
  {
-    unsigned long expire = jiffies + msecs_to_jiffies(250);
+    unsigned long start, end;
      u32 rval;

      rval = mmc_readl(host, REG_CLKCR);

drivers/mmc/host/sunxi-mmc.c +2 -0

@@ -604,6 +604,8 @@ static int sunxi_mmc_oclk_onoff(struct 
 sunxi_mmc_host *host, u32 oclk_en)
      if (oclk_en)
          rval |= SDXC_CARD_CLOCK_ON;
 
+    start = jiffies;
+    end = start + msecs_to_jiffies(750);
      mmc_writel(host, REG_CLKCR, rval);
 

      rval = SDXC_START | SDXC_UPCLK_ONLY | SDXC_WAIT_PRE_OVER;

drivers/mmc/host/sunxi-mmc.c +16 -2

@@ -611,15 +613,29 @@ static int sunxi_mmc_oclk_onoff(struct 
 sunxi_mmc_host *host, u32 oclk_en)
 
      do {
          rval = mmc_readl(host, REG_CMDR);
-    } while (time_before(jiffies, expire) && (rval & SDXC_START));
+    } while (time_before(jiffies, end) && (rval & SDXC_START));
+    end = jiffies;
 
      /* clear irq status bits set by the command */
      mmc_writel(host, REG_RINTR,
             mmc_readl(host, REG_RINTR) & ~SDXC_SDIO_INTERRUPT);
 
      if (rval & SDXC_START) {
-        dev_err(mmc_dev(host->mmc), "fatal err update clk timeout\n");
+        dev_err(mmc_dev(host->mmc),
+            "fatal err update oclk timeout. Could not %s in %ims.\n",
+            oclk_en ? "enable" : "disable",
+            jiffies_to_msecs(end - start));
          return -EIO;
+    } else {
+        int msecs = jiffies_to_msecs(end - start);
+        const char *ing = oclk_en ? "enabling" : "disabling";
+
+        if ((msecs > 150) || (oclk_en && (msecs > 50)))
+            dev_warn(mmc_dev(host->mmc),
+                 "%s oclk took %ims", ing, msecs);
+        else
+            dev_dbg(mmc_dev(host->mmc),
+                 "%s oclk took %ims", ing, msecs);
      }
 

      return 0;

Hi,

Quoted 1 line · Michal Suchanek

Quoted 4 lines

When core does not set the MMC_QUIRK_BROKEN_CLK_GATING flag enable automatic hardware controlled clock gating on the mmc interface.

Signed-off-by Michal Suchanek <[email protected]>

In general this looks good, but I wonder how intensively this has been tested ?

Also given the long latencies when using manual clock on/off support, have you done any testing to check what

sort of latencies this adds, e.g. Both with and without the patch, dump all the filesystem caches:

echo 3 > /proc/sys/vm/drop_caches

And then do:

time cat /some/small/file/on/the/sdcard

Do this at least 3 time both with and without the patch, and see if there are any noticable differences ?

Quoted 27 lines · Hans de Goede

Hi,

Quoted 11 lines · michal.suchanek at ruk.cuni.cz

The 250ms timeout is too short.

On my system enabling the oclk takes under 50ms and disabling slightly over 100ms when idle. Under load disabling the clock can take over 350ms.

This does not make mmc clock gating look like good option to have on sunxi but the system should not crash with mmc clock gating enabled nonetheless.

Quoted 5 lines

This patch sets the timeout to 750ms and adds debug prints which show how long enabling/disabling the clock took so more data can be collected from other systems.

Signed-off-by Michal Suchanek <[email protected]>

This is a big patch for just changing a timeout, most of this is in extra verbosity which IMHO has little value, in the error path w know we will have waited aprox 750 ms, so printing the waiting time there is not worth all the extra code.

Quoted 7 lines

As for adding the warning I'm even less of a big fan of that, if we need higher timeouts, we need higher timeouts, spamming the kernel logs with that we are actually hitting the higher timeouts is not productive IMHO.

Can you please resend this as a one-liner just changing the timeout ?

While I can't speak for Michal obviously,

I left the debugging bit (in my v2 that i sent 2 minutes ago) as both you and Hans where content with it back then and both acked it.

Michal, feel free to send the v3 without the debug info, unless you want me to do it ;)

Hi,

Quoted 4 lines · Michal Suchanek

When core does not set the MMC_QUIRK_BROKEN_CLK_GATING flag enable automatic hardware controlled clock gating on the mmc interface.

Signed-off-by Michal Suchanek <[email protected]>

In general this looks good, but I wonder how intensively this has been tested ?

Also given the long latencies when using manual clock on/off support, have you done any testing to check what sort of latencies this adds, e.g. Both with and without the patch, dump all the filesystem caches:

echo 3 > /proc/sys/vm/drop_caches

And then do:

time cat /some/small/file/on/the/sdcard

Do this at least 3 time both with and without the patch, and see if there are any noticable differences ?

Regards,

Hans

Quoted 25 lines

---
  drivers/mmc/host/sunxi-mmc.c | 7 ++++++-
  1 file changed, 6 insertions(+), 1 deletion(-)

drivers/mmc/host/sunxi-mmc.c +6 -1

index f808a02..443cab5 100644
@@ -601,8 +601,13 @@ static int sunxi_mmc_oclk_onoff(struct 
 sunxi_mmc_host *host, u32 oclk_en)
      rval = mmc_readl(host, REG_CLKCR);
      rval &= ~(SDXC_CARD_CLOCK_ON | SDXC_LOW_POWER_ON);
 
-    if (oclk_en)
+    if (oclk_en) {
          rval |= SDXC_CARD_CLOCK_ON;
+        if (!host->mmc->card ||
+             !(host->mmc->card->quirks & MMC_QUIRK_BROKEN_CLK_GATING))
+
+            rval |= SDXC_LOW_POWER_ON;
+    }
 
      start = jiffies;

      end = start + msecs_to_jiffies(750);

Hey,

Quoted 11 lines · Hans de Goede

Hi,

Quoted 5 lines · Michal Suchanek

When core does not set the MMC_QUIRK_BROKEN_CLK_GATING flag enable automatic hardware controlled clock gating on the mmc interface.

Signed-off-by Michal Suchanek <[email protected]>

Quoted 2 lines

In general this looks good, but I wonder how intensively this has been tested ?

It doesn't matter actually, it took some time longer, but the mmc still craps out even with Michal's 3 patches. I'll revert hans's earlier patch again and do a bit more extensive testing.

The 250ms timeout is too short.

On my system enabling the oclk takes under 50ms and disabling slightly over 100ms when idle. Under load disabling the clock can take over 350ms.

This does not make mmc clock gating look like good option to have on sunxi but the system should not crash with mmc clock gating enabled nonetheless.

This patch sets the timeout to 750ms.

Signed-off-by Michal Suchanek <[email protected]>

---
v3
- remove debug message

v2
- fix formatting

---
  drivers/mmc/host/sunxi-mmc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mmc/host/sunxi-mmc.c +1 -1

index 4d3e1ff..a7b7a67 100644
@@ -595,7 +595,7 @@ static irqreturn_t sunxi_mmc_handle_manual_stop(int irq, void *dev_id)
 
  static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
  {
-	unsigned long expire = jiffies + msecs_to_jiffies(250);
+	unsigned long expire = jiffies + msecs_to_jiffies(750);
  	u32 rval;
 
  	rval = mmc_readl(host, REG_CLKCR);

Quoted 9 lines · Olliver Schinagl

Hey,

Quoted 6 lines · Hans de Goede

Hi,

Quoted 3 lines · Michal Suchanek

When core does not set the MMC_QUIRK_BROKEN_CLK_GATING flag enable automatic hardware controlled clock gating on the mmc interface.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Signed-off-by Michal Suchanek <[email protected]>

In general this looks good, but I wonder how intensively this has been tested ?

It doesn't matter actually, it took some time longer, but the mmc still craps out even with Michal's 3 patches. I'll revert hans's earlier patch again and do a bit more extensive testing.

Does the oclk switch timeout even after 750ms?

In some earlier tests I tried to enable/disable the clock repeatedly when it failed but it seemed to have little effect on the total time it took to disable the clock in the end.

Maybe it would be worh trying to set the timeout to some insanely long value and test stability with that. I picked 750 as around twice the maximum time it ever took on my board.

Thanks

Michal

The driver has open-coded test for SDIO cards. Use the mmc core provided MMC_QUIRK_BROKEN_CLK_GATING flag instead.

As a bonus this may enable clock gating on SDIO cards that are known to work with it.

Signed-off-by Michal Suchanek <[email protected]>

---
 drivers/mmc/host/dw_mmc.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

drivers/mmc/host/dw_mmc.c +15 -18

index 40e9d8e..3bc9fd7 100644
@@ -1335,27 +1335,24 @@ static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
 	 * description of the CLKENA register we should disable low power mode
 	 * for SDIO cards if we need SDIO interrupts to work.
 	 */
-	if (mmc->caps & MMC_CAP_SDIO_IRQ) {
-		const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
-		u32 clk_en_a_old;
-		u32 clk_en_a;
+	const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
+	u32 clk_en_a_old;
+	u32 clk_en_a;
 
-		clk_en_a_old = mci_readl(host, CLKENA);
+	clk_en_a_old = mci_readl(host, CLKENA);
 
-		if (card->type == MMC_TYPE_SDIO ||
-		    card->type == MMC_TYPE_SD_COMBO) {
-			set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
-			clk_en_a = clk_en_a_old & ~clken_low_pwr;
-		} else {
-			clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
-			clk_en_a = clk_en_a_old | clken_low_pwr;
-		}
+	if (card->quirks & MMC_QUIRK_BROKEN_CLK_GATING) {
+		set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
+		clk_en_a = clk_en_a_old & ~clken_low_pwr;
+	} else {
+		clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
+		clk_en_a = clk_en_a_old | clken_low_pwr;
+	}
 
-		if (clk_en_a != clk_en_a_old) {
-			mci_writel(host, CLKENA, clk_en_a);
-			mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
-				     SDMMC_CMD_PRV_DAT_WAIT, 0);
-		}
+	if (clk_en_a != clk_en_a_old) {
+		mci_writel(host, CLKENA, clk_en_a);
+		mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
+				SDMMC_CMD_PRV_DAT_WAIT, 0);
 	}
 }
 

Hi, Michal.

Quoted 2 lines · Michal Suchanek

The driver has open-coded test for SDIO cards. Use the mmc core provided MMC_QUIRK_BROKEN_CLK_GATING flag instead.

Did you use the clock-gating for SDIO cards? Doesn't MMC_CAP_SDIO_IRQ bit set? Which case is broken? Could you explain to me more?

Best Regards,
Jaehoon Chung

Quoted 57 lines

As a bonus this may enable clock gating on SDIO cards that are known to work with it.

Signed-off-by Michal Suchanek <[email protected]>

---
 drivers/mmc/host/dw_mmc.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

drivers/mmc/host/dw_mmc.c +15 -18

index 40e9d8e..3bc9fd7 100644
@@ -1335,27 +1335,24 @@ static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
       * description of the CLKENA register we should disable low power mode
       * for SDIO cards if we need SDIO interrupts to work.
       */
-     if (mmc->caps & MMC_CAP_SDIO_IRQ) {
-             const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
-             u32 clk_en_a_old;
-             u32 clk_en_a;
+     const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
+     u32 clk_en_a_old;
+     u32 clk_en_a;
 
-             clk_en_a_old = mci_readl(host, CLKENA);
+     clk_en_a_old = mci_readl(host, CLKENA);
 
-             if (card->type == MMC_TYPE_SDIO ||
-                 card->type == MMC_TYPE_SD_COMBO) {
-                     set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
-                     clk_en_a = clk_en_a_old & ~clken_low_pwr;
-             } else {
-                     clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
-                     clk_en_a = clk_en_a_old | clken_low_pwr;
-             }
+     if (card->quirks & MMC_QUIRK_BROKEN_CLK_GATING) {
+             set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
+             clk_en_a = clk_en_a_old & ~clken_low_pwr;
+     } else {
+             clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
+             clk_en_a = clk_en_a_old | clken_low_pwr;
+     }
 
-             if (clk_en_a != clk_en_a_old) {
-                     mci_writel(host, CLKENA, clk_en_a);
-                     mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
-                                  SDMMC_CMD_PRV_DAT_WAIT, 0);
-             }
+     if (clk_en_a != clk_en_a_old) {
+             mci_writel(host, CLKENA, clk_en_a);
+             mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
+                             SDMMC_CMD_PRV_DAT_WAIT, 0);
      }
 }
 

Hello,

Quoted 3 lines · Jaehoon Chung

Hi, Michal.

On 08/12/2015 09:23 PM, Michal Suchanek wrote:

Quoted 6 lines

Quoted 2 lines

The driver has open-coded test for SDIO cards. Use the mmc core provided MMC_QUIRK_BROKEN_CLK_GATING flag instead.

Did you use the clock-gating for SDIO cards? Doesn't MMC_CAP_SDIO_IRQ bit set? Which case is broken? Could you explain to me more?

The core flag for disabling power saving is MMC_QUIRK_BROKEN_CLK_GATING.

It may coincide with MMC_CAP_SDIO_IRQ but that's different flag for different purpose.

MMC_QUIRK_BROKEN_CLK_GATING is currently set for all SDIO cards except for cards on a whitelist.

I don't know any particular SDIO card that has problems when the clock is off and does not use an IRQ.

Thanks

Michal

Quoted 48 lines

Best Regards,
Jaehoon Chung

Quoted 44 lines

As a bonus this may enable clock gating on SDIO cards that are known to work with it.

Signed-off-by Michal Suchanek <[email protected]>

---
 drivers/mmc/host/dw_mmc.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

drivers/mmc/host/dw_mmc.c +11 -13

index 40e9d8e..3bc9fd7 100644
@@ -1335,27 +1335,24 @@ static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
       * description of the CLKENA register we should disable low power mode
       * for SDIO cards if we need SDIO interrupts to work.
       */
-     if (mmc->caps & MMC_CAP_SDIO_IRQ) {
-             const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
-             u32 clk_en_a_old;
-             u32 clk_en_a;
+     const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
+     u32 clk_en_a_old;
+     u32 clk_en_a;
 
-             clk_en_a_old = mci_readl(host, CLKENA);
+     clk_en_a_old = mci_readl(host, CLKENA);
 
-             if (card->type == MMC_TYPE_SDIO ||
-                 card->type == MMC_TYPE_SD_COMBO) {
-                     set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
-                     clk_en_a = clk_en_a_old & ~clken_low_pwr;
-             } else {
-                     clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
-                     clk_en_a = clk_en_a_old | clken_low_pwr;
-             }
+     if (card->quirks & MMC_QUIRK_BROKEN_CLK_GATING) {
+             set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
+             clk_en_a = clk_en_a_old & ~clken_low_pwr;
+     } else {
+             clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
+             clk_en_a = clk_en_a_old | clken_low_pwr;
+     }
 

Quoted 14 lines

drivers/mmc/host/dw_mmc.c static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card) · line 1358

Quoted 13 lines

drivers/mmc/host/dw_mmc.c +4 -5

-             if (clk_en_a != clk_en_a_old) {
-                     mci_writel(host, CLKENA, clk_en_a);
-                     mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
-                                  SDMMC_CMD_PRV_DAT_WAIT, 0);
-             }
+     if (clk_en_a != clk_en_a_old) {
+             mci_writel(host, CLKENA, clk_en_a);
+             mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
+                             SDMMC_CMD_PRV_DAT_WAIT, 0);
      }
 }
 

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

Quoted 9 lines · Michal Suchanek

Hello,

Quoted 6 lines · Jaehoon Chung

Hi, Michal.

Quoted 2 lines · Michal Suchanek

The driver has open-coded test for SDIO cards. Use the mmc core provided MMC_QUIRK_BROKEN_CLK_GATING flag instead.

Quoted 5 lines

Quoted 3 lines

Did you use the clock-gating for SDIO cards? Doesn't MMC_CAP_SDIO_IRQ bit set? Which case is broken? Could you explain to me more?

The core flag for disabling power saving is MMC_QUIRK_BROKEN_CLK_GATING.

I understood your intention. And i read the comment into mmc/core/quirks.c I will test SDIO card with this patch. Thanks.

Best Regards,
Jaehoon Chung

Quoted 76 lines

It may coincide with MMC_CAP_SDIO_IRQ but that's different flag for different purpose.

MMC_QUIRK_BROKEN_CLK_GATING is currently set for all SDIO cards except for cards on a whitelist.

I don't know any particular SDIO card that has problems when the clock is off and does not use an IRQ.

Thanks

Michal

Quoted 48 lines

Best Regards,
Jaehoon Chung

Quoted 44 lines

As a bonus this may enable clock gating on SDIO cards that are known to work with it.

Signed-off-by Michal Suchanek <[email protected]>

---
 drivers/mmc/host/dw_mmc.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

drivers/mmc/host/dw_mmc.c +11 -13

index 40e9d8e..3bc9fd7 100644
@@ -1335,27 +1335,24 @@ static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
       * description of the CLKENA register we should disable low power mode
       * for SDIO cards if we need SDIO interrupts to work.
       */
-     if (mmc->caps & MMC_CAP_SDIO_IRQ) {
-             const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
-             u32 clk_en_a_old;
-             u32 clk_en_a;
+     const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
+     u32 clk_en_a_old;
+     u32 clk_en_a;
 
-             clk_en_a_old = mci_readl(host, CLKENA);
+     clk_en_a_old = mci_readl(host, CLKENA);
 
-             if (card->type == MMC_TYPE_SDIO ||
-                 card->type == MMC_TYPE_SD_COMBO) {
-                     set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
-                     clk_en_a = clk_en_a_old & ~clken_low_pwr;
-             } else {
-                     clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
-                     clk_en_a = clk_en_a_old | clken_low_pwr;
-             }
+     if (card->quirks & MMC_QUIRK_BROKEN_CLK_GATING) {
+             set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
+             clk_en_a = clk_en_a_old & ~clken_low_pwr;
+     } else {
+             clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
+             clk_en_a = clk_en_a_old | clken_low_pwr;
+     }
 

Quoted 14 lines

drivers/mmc/host/dw_mmc.c static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card) · line 1358

Quoted 13 lines

drivers/mmc/host/dw_mmc.c +4 -5

-             if (clk_en_a != clk_en_a_old) {
-                     mci_writel(host, CLKENA, clk_en_a);
-                     mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
-                                  SDMMC_CMD_PRV_DAT_WAIT, 0);
-             }
+     if (clk_en_a != clk_en_a_old) {
+             mci_writel(host, CLKENA, clk_en_a);
+             mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
+                             SDMMC_CMD_PRV_DAT_WAIT, 0);
      }
 }
 

Quoted 5 lines

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

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

HI

Quoted 14 lines · Jaehoon Chung

Quoted 13 lines · Michal Suchanek

Hello,

Quoted 9 lines · Jaehoon Chung

Hi, Michal.

Quoted 2 lines · Michal Suchanek

The driver has open-coded test for SDIO cards. Use the mmc core provided MMC_QUIRK_BROKEN_CLK_GATING flag instead.

Did you use the clock-gating for SDIO cards? Doesn't MMC_CAP_SDIO_IRQ bit set? Which case is broken? Could you explain to me more?

Quoted 5 lines

Quoted 1 line

The core flag for disabling power saving is MMC_QUIRK_BROKEN_CLK_GATING.

I understood your intention. And i read the comment into mmc/core/quirks.c I will test SDIO card with this patch. Thanks.

When you test, please check if SDIO IRQ still works, we need to put dw_mmc in low_power mode otherwise SDIO IRQ will be not be generated by dw_mmc host controller.

Hello,

Quoted 17 lines · Alim Akhtar

HI

Quoted 14 lines · Jaehoon Chung

Quoted 13 lines · Michal Suchanek

Hello,

Quoted 9 lines · Jaehoon Chung

Hi, Michal.

Quoted 2 lines · Michal Suchanek

The driver has open-coded test for SDIO cards. Use the mmc core provided MMC_QUIRK_BROKEN_CLK_GATING flag instead.

Did you use the clock-gating for SDIO cards? Doesn't MMC_CAP_SDIO_IRQ bit set? Which case is broken? Could you explain to me more?

Quoted 9 lines

Quoted 5 lines

Quoted 1 line

The core flag for disabling power saving is MMC_QUIRK_BROKEN_CLK_GATING.

I understood your intention. And i read the comment into mmc/core/quirks.c I will test SDIO card with this patch. Thanks.

When you test, please check if SDIO IRQ still works, we need to put dw_mmc in low_power mode otherwise SDIO IRQ will be not be generated by dw_mmc host controller.

As far as I understand the logic which is removed in this patch and the core logic which replaces it is the same - low power by means of clock gating is *not* enabled for SDIO cards in either case.

The original code also checks for SDIO IRQ and disables clock gating regardless of card type which is probably redundant. If not it should be fixed in mmc core.

My recent kernel builds which I run on a system with mwifiex card probably include this patch.

Thanks

Michal

Hi,

Quoted 11 lines · Michal Suchanek

The 250ms timeout is too short.

On my system enabling the oclk takes under 50ms and disabling slightly over 100ms when idle. Under load disabling the clock can take over 350ms.

This does not make mmc clock gating look like good option to have on sunxi but the system should not crash with mmc clock gating enabled nonetheless.

Quoted 7 lines

This patch sets the timeout to 750ms.

Signed-off-by Michal Suchanek <[email protected]>

---
v3
- remove debug message

Thanks, this one looks good to me:

Acked-by Hans de Goede <[email protected]>

Ulf, can you apply this one please?

Thanks & Regards,

Hans

Quoted 24 lines

v2
- fix formatting

---
  drivers/mmc/host/sunxi-mmc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mmc/host/sunxi-mmc.c +1 -1

index 4d3e1ff..a7b7a67 100644
@@ -595,7 +595,7 @@ static irqreturn_t sunxi_mmc_handle_manual_stop(int
 irq, void *dev_id)
 
  static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32
 oclk_en)
  {
-       unsigned long expire = jiffies + msecs_to_jiffies(250);
+       unsigned long expire = jiffies + msecs_to_jiffies(750);
        u32 rval;

        rval = mmc_readl(host, REG_CLKCR);