Skip to contentSkip to conversation

sdhci : reduce irq off latency

PATCH Matthieu CASTET 3 participants 6 messages Jun 17, 2010 – Aug 30, 2010

Subsystems (2)
MMC, SD and SDIO Secure digital host controller interface (SDHCI) driver
lore
Share & tools
lore

Matthieu CASTET

author
#0 lore raw

sdhci code got tasklets (sdhci_tasklet_card and sdhci_tasklet_finish), that does :
{
spin_lock_irqsave

if (cond) {
sdhci_reset
sdhci_reset
}

spin_unlock_irqrestore
}

sdhci_reset {
...
while (read_reg) {
if (timeout == 0)
break;
timeout--;
mdelay(1);
}
...
}

The problem is that sdhci_reset [1] does busy pooling (with a granularity of 1 ms) on a register up to a timeout of 100 ms.

With the current code, we got irq off during 2*1ms. With the attached patch we reduce irq off to 30 us.

Note that worst case 100 ms irq off still exist.

Signed-off-by Matthieu CASTET <[email protected]>
drivers/mmc/host/sdhci.c +9 -12
index c6d1bd8..03d6bde 100644
@@ -157,18 +157,17 @@ static void sdhci_reset(struct sdhci_host *host, u8 mask)
 		host->clock = 0;
 
 	/* Wait max 100 ms */
-	timeout = 100;
+	timeout = jiffies + msecs_to_jiffies(100);
 
 	/* hw clears the bit when it's done */
 	while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) {
-		if (timeout == 0) {
+		if (time_is_before_jiffies(timeout)) {
 			printk(KERN_ERR "%s: Reset 0x%x never completed.\n",
 				mmc_hostname(host->mmc), (int)mask);
 			sdhci_dumpregs(host);
 			return;
 		}
-		timeout--;
-		mdelay(1);
+		cpu_relax();
 	}
 
 	if (host->quirks & SDHCI_QUIRK_RESTORE_IRQS_AFTER_RESET)

@@ -882,7 +881,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 	WARN_ON(host->cmd);
 
 	/* Wait max 10 ms */
-	timeout = 10;
+	timeout = jiffies + msecs_to_jiffies(10);
 
 	mask = SDHCI_CMD_INHIBIT;
 	if ((cmd->data != NULL) || (cmd->flags & MMC_RSP_BUSY))

@@ -894,7 +893,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 		mask &= ~SDHCI_DATA_INHIBIT;
 
 	while (sdhci_readl(host, SDHCI_PRESENT_STATE) & mask) {
-		if (timeout == 0) {
+		if (time_is_before_jiffies(timeout)) {
 			printk(KERN_ERR "%s: Controller never released "
 				"inhibit bit(s).\n", mmc_hostname(host->mmc));
 			sdhci_dumpregs(host);

@@ -902,8 +901,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 			tasklet_schedule(&host->finish_tasklet);
 			return;
 		}
-		timeout--;
-		mdelay(1);
+		cpu_relax();
 	}
 
 	mod_timer(&host->timer, jiffies + 10 * HZ);

@@ -1007,17 +1005,16 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
 
 	/* Wait max 20 ms */
-	timeout = 20;
+	timeout = jiffies + msecs_to_jiffies(20)
 	while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
 		& SDHCI_CLOCK_INT_STABLE)) {
-		if (timeout == 0) {
+		if (time_is_before_jiffies(timeout)) {
 			printk(KERN_ERR "%s: Internal clock never "
 				"stabilised.\n", mmc_hostname(host->mmc));
 			sdhci_dumpregs(host);
 			return;
 		}
-		timeout--;
-		mdelay(1);
+		cpu_relax();
 	}
 
 	clk |= SDHCI_CLOCK_CARD_EN;
#1 lore raw

Hi,

Not good news. This patch make a hang at probe time. it's tested against commit 7e27d6e778cd87b6f2415515d7127eba53fe5d02

Author: Linus Torvalds <[email protected]>
Date:   Fri Jun 11 19:14:04 2010 -0700
    Linux 2.6.35-rc3

Thank you,
Kyungmin Park

Quoted 36 lines · Matthieu CASTET

sdhci code got tasklets (sdhci_tasklet_card and sdhci_tasklet_finish), that does :
{
spin_lock_irqsave

if (cond) {
sdhci_reset
sdhci_reset
}

spin_unlock_irqrestore
}

sdhci_reset {
...
while (read_reg) {
if (timeout == 0)
  break;
timeout--;
mdelay(1);
}
...
}

The problem is that sdhci_reset [1] does busy pooling (with a granularity of 1 ms) on a register up to a timeout of 100 ms.

With the current code, we got irq off during 2*1ms. With the attached patch we reduce irq off to 30 us.

Note that worst case 100 ms irq off still exist.

Signed-off-by Matthieu CASTET <[email protected]>
author
#2 lore raw

Hi,

Thanks you for the test.

here a new version of the patch. It doesn't replace mdelay with jiffies anymore. It changes mdelay to udelay (the granularity of 1 ms becomes 10 us). I didn't mesure irq off latency with this patch.

Matthieu

Quoted 7 lines · Kyungmin Park

Hi,

Not good news. This patch make a hang at probe time. it's tested against commit 7e27d6e778cd87b6f2415515d7127eba53fe5d02

Author: Linus Torvalds <[email protected]>
Date:   Fri Jun 11 19:14:04 2010 -0700
Quoted 43 lines
    Linux 2.6.35-rc3

Thank you,
Kyungmin Park

Quoted 36 lines · Matthieu CASTET

sdhci code got tasklets (sdhci_tasklet_card and sdhci_tasklet_finish), that does :
{
spin_lock_irqsave

if (cond) {
sdhci_reset
sdhci_reset
}

spin_unlock_irqrestore
}

sdhci_reset {
...
while (read_reg) {
if (timeout == 0)
break;
timeout--;
mdelay(1);
}
...
}

The problem is that sdhci_reset [1] does busy pooling (with a granularity of 1 ms) on a register up to a timeout of 100 ms.

With the current code, we got irq off during 2*1ms. With the attached patch we reduce irq off to 30 us.

Note that worst case 100 ms irq off still exist.

Signed-off-by Matthieu CASTET <[email protected]>
drivers/mmc/host/sdhci.c +6 -6
index c6d1bd8..fa06c70 100644
@@ -157,7 +157,7 @@ static void sdhci_reset(struct sdhci_host *host, u8 mask)
 		host->clock = 0;
 
 	/* Wait max 100 ms */
-	timeout = 100;
+	timeout = 100*100;
 
 	/* hw clears the bit when it's done */
 	while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) {

@@ -168,7 +168,7 @@ static void sdhci_reset(struct sdhci_host *host, u8 mask)
 			return;
 		}
 		timeout--;
-		mdelay(1);
+		udelay(10);
 	}
 
 	if (host->quirks & SDHCI_QUIRK_RESTORE_IRQS_AFTER_RESET)

@@ -882,7 +882,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 	WARN_ON(host->cmd);
 
 	/* Wait max 10 ms */
-	timeout = 10;
+	timeout = 10*100;
 
 	mask = SDHCI_CMD_INHIBIT;
 	if ((cmd->data != NULL) || (cmd->flags & MMC_RSP_BUSY))

@@ -903,7 +903,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 			return;
 		}
 		timeout--;
-		mdelay(1);
+		udelay(10);
 	}
 
 	mod_timer(&host->timer, jiffies + 10 * HZ);

@@ -1007,7 +1007,7 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
 
 	/* Wait max 20 ms */
-	timeout = 20;
+	timeout = 20*100;
 	while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
 		& SDHCI_CLOCK_INT_STABLE)) {
 		if (timeout == 0) {

@@ -1017,7 +1017,7 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 			return;
 		}
 		timeout--;
-		mdelay(1);
+		udelay(10);
 	}
 
 	clk |= SDHCI_CLOCK_CARD_EN;
#3 lore raw

The main problem is sdhci_send_command function. it loops at while function infinitely.

I just modify it to use udelay at there. then it's working well. also I reduce the granularity as 1 usec instead of 10 usec (as you suggested). it's also works.

Thank you,
Kyungmin Park

Quoted 68 lines · Matthieu CASTET

Hi,

Thanks you for the test.

here a new version of the patch. It doesn't replace mdelay with jiffies anymore. It changes mdelay to udelay (the granularity of 1 ms becomes 10 us).
I didn't mesure irq off latency with this patch.

Matthieu

Quoted 54 lines · Kyungmin Park

Hi,

Not good news. This patch make a hang at probe time. it's tested against commit 7e27d6e778cd87b6f2415515d7127eba53fe5d02
Author: Linus Torvalds <[email protected]>
Date:   Fri Jun 11 19:14:04 2010 -0700

   Linux 2.6.35-rc3

Thank you,
Kyungmin Park

Quoted 39 lines · Matthieu CASTET

sdhci code got tasklets (sdhci_tasklet_card and sdhci_tasklet_finish), that does :
{
spin_lock_irqsave

if (cond) {
sdhci_reset
sdhci_reset
}

spin_unlock_irqrestore
}

sdhci_reset {
...
while (read_reg) {
if (timeout == 0)
 break;
timeout--;
mdelay(1);
}
...
}

The problem is that sdhci_reset [1] does busy pooling (with a granularity of
1 ms) on a register up to a timeout of 100 ms.

With the current code, we got irq off during 2*1ms. With the attached patch
we reduce irq off to 30 us.

Note that worst case 100 ms irq off still exist.

Signed-off-by Matthieu CASTET <[email protected]>
#4 lore raw
In reply to Matthieu CASTET

Hi Matthieu,

This patch didn't make it upstream. Kyungmin offered a suggestion for fixing it -- would you like to resend an updated patch?

(As a minor aside, I have a fairly irrational preference for "time_after(jiffies, timeout)" over "time_is_before_jiffies(timeout)", because the latter reads as the inverse of what it actually does to me. They compile to the same code.)

Date: Thu, 17 Jun 2010 11:07:15 +0200
From: Matthieu CASTET <[email protected]>

sdhci code got tasklets (sdhci_tasklet_card and sdhci_tasklet_finish), that does :
{
spin_lock_irqsave

if (cond) {
sdhci_reset
sdhci_reset
}

spin_unlock_irqrestore
}

sdhci_reset {
...
while (read_reg) {
if (timeout == 0)
break;
timeout--;
mdelay(1);
}
...
}

The problem is that sdhci_reset [1] does busy pooling (with a granularity of 1 ms) on a register up to a timeout of 100 ms.

With the current code, we got irq off during 2*1ms. With the attached patch we reduce irq off to 30 us.

Note that worst case 100 ms irq off still exist.

Signed-off-by Matthieu CASTET <[email protected]>
Signed-off-by Chris Ball <[email protected]>

---

drivers/mmc/host/sdhci.c +9 -12
index c6d1bd8..03d6bde 100644
@@ -157,18 +157,17 @@ static void sdhci_reset(struct sdhci_host *host, u8 mask)
 		host->clock = 0;
 
 	/* Wait max 100 ms */
-	timeout = 100;
+	timeout = jiffies + msecs_to_jiffies(100);
 
 	/* hw clears the bit when it's done */
 	while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) {
-		if (timeout == 0) {
+		if (time_is_before_jiffies(timeout)) {
 			printk(KERN_ERR "%s: Reset 0x%x never completed.\n",
 				mmc_hostname(host->mmc), (int)mask);
 			sdhci_dumpregs(host);
 			return;
 		}
-		timeout--;
-		mdelay(1);
+		cpu_relax();
 	}
 
 	if (host->quirks & SDHCI_QUIRK_RESTORE_IRQS_AFTER_RESET)

@@ -882,7 +881,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 	WARN_ON(host->cmd);
 
 	/* Wait max 10 ms */
-	timeout = 10;
+	timeout = jiffies + msecs_to_jiffies(10);
 
 	mask = SDHCI_CMD_INHIBIT;
 	if ((cmd->data != NULL) || (cmd->flags & MMC_RSP_BUSY))

@@ -894,7 +893,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 		mask &= ~SDHCI_DATA_INHIBIT;
 
 	while (sdhci_readl(host, SDHCI_PRESENT_STATE) & mask) {
-		if (timeout == 0) {
+		if (time_is_before_jiffies(timeout)) {
 			printk(KERN_ERR "%s: Controller never released "
 				"inhibit bit(s).\n", mmc_hostname(host->mmc));
 			sdhci_dumpregs(host);

@@ -902,8 +901,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 			tasklet_schedule(&host->finish_tasklet);
 			return;
 		}
-		timeout--;
-		mdelay(1);
+		cpu_relax();
 	}
 
 	mod_timer(&host->timer, jiffies + 10 * HZ);

@@ -1007,17 +1005,16 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
 
 	/* Wait max 20 ms */
-	timeout = 20;
+	timeout = jiffies + msecs_to_jiffies(20)
 	while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
 		& SDHCI_CLOCK_INT_STABLE)) {
-		if (timeout == 0) {
+		if (time_is_before_jiffies(timeout)) {
 			printk(KERN_ERR "%s: Internal clock never "
 				"stabilised.\n", mmc_hostname(host->mmc));
 			sdhci_dumpregs(host);
 			return;
 		}
-		timeout--;
-		mdelay(1);
+		cpu_relax();
 	}
 
 	clk |= SDHCI_CLOCK_CARD_EN;
Signature · 2 lines
Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child