sdhci can turn off irq up to 200 ms

sdhci can turn off irq up to 200 ms

[Matthieu CASTET]

Hi,

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
}

The problem is that sdhci_reset [1] does busy pooling on a register up
to a timeout of 100 ms.
That's not low latency friendly.

On our system, we saw that sdhci_reset take 1 ms. That should be because
we enter in mdelay, even if the hardware clears the bit faster.
I wonder why there is an mdelay(1). Using cpu_relax and
time_is_after_jiffies should make sdhci_reset faster.


Matthieu

[1]
static void sdhci_reset(struct sdhci_host *host, u8 mask)
{
    unsigned long timeout;
    u32 uninitialized_var(ier);
[...]
    sdhci_writeb(host, mask, SDHCI_SOFTWARE_RESET);

    if (mask & SDHCI_RESET_ALL)
        host->clock = 0;

    /* Wait max 100 ms */
    timeout = 100;

    /* hw clears the bit when it's done */
    while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) {
        if (timeout == 0) {
            printk(KERN_ERR "%s: Reset 0x%x never completed.\n",
                mmc_hostname(host->mmc), (int)mask);
            sdhci_dumpregs(host);
            return;
        }
        timeout--;
        mdelay(1);
    }
[...]
}

Re: sdhci can turn off irq up to 200 ms

[Matthieu CASTET]

[-- Attachment #1: Type: text/plain, Size: 733 bytes --]

Matthieu CASTET a écrit :
> Hi,
> 
> 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
> }
> 
> The problem is that sdhci_reset [1] does busy pooling on a register up
> to a timeout of 100 ms.
> That's not low latency friendly.
> 
> On our system, we saw that sdhci_reset take 1 ms. That should be because
> we enter in mdelay, even if the hardware clears the bit faster.
> I wonder why there is an mdelay(1). Using cpu_relax and
> time_is_after_jiffies should make sdhci_reset faster.
> 
In case somebody cares, here a patch that reduce on our hardware
sdhci_reset from 1 ms to 30 us.


Matthieu

[-- Attachment #2: sdhci.diff --]
[-- Type: text/x-diff, Size: 763 bytes --]

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 6779b4e..3e199b6 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -156,18 +156,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)

Re: sdhci can turn off irq up to 200 ms

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 1298 bytes --]

On Thu, 9 Jul 2009 12:28:01 +0200
Matthieu CASTET <matthieu.castet@parrot.com> wrote:

> Matthieu CASTET a écrit :
> > Hi,
> > 
> > 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
> > }
> > 
> > The problem is that sdhci_reset [1] does busy pooling on a register up
> > to a timeout of 100 ms.
> > That's not low latency friendly.
> > 
> > On our system, we saw that sdhci_reset take 1 ms. That should be because
> > we enter in mdelay, even if the hardware clears the bit faster.
> > I wonder why there is an mdelay(1). Using cpu_relax and
> > time_is_after_jiffies should make sdhci_reset faster.
> > 
> In case somebody cares, here a patch that reduce on our hardware
> sdhci_reset from 1 ms to 30 us.
> 

I seem to recall having problems with jiffies not updating with those
locks held (or perhaps it was when inside the isr).

What arch have you been testing this on?

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

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

Re: sdhci can turn off irq up to 200 ms

[Matthieu CASTET]

Pierre Ossman a écrit :
> On Thu, 9 Jul 2009 12:28:01 +0200
> Matthieu CASTET <matthieu.castet@parrot.com> wrote:
> 
>> Matthieu CASTET a écrit :
>>> Hi,
>>>
>>> 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
>>> }
>>>
>>> The problem is that sdhci_reset [1] does busy pooling on a register up
>>> to a timeout of 100 ms.
>>> That's not low latency friendly.
>>>
>>> On our system, we saw that sdhci_reset take 1 ms. That should be because
>>> we enter in mdelay, even if the hardware clears the bit faster.
>>> I wonder why there is an mdelay(1). Using cpu_relax and
>>> time_is_after_jiffies should make sdhci_reset faster.
>>>
>> In case somebody cares, here a patch that reduce on our hardware
>> sdhci_reset from 1 ms to 30 us.
>>
> 
> I seem to recall having problems with jiffies not updating with those
> locks held (or perhaps it was when inside the isr).
> 
> What arch have you been testing this on?
It have been tested on arm.

Matthieu

Re: sdhci can turn off irq up to 200 ms

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 698 bytes --]

On Mon, 7 Sep 2009 11:56:10 +0200
Matthieu CASTET <matthieu.castet@parrot.com> wrote:

> Pierre Ossman a écrit :
> > 
> > I seem to recall having problems with jiffies not updating with those
> > locks held (or perhaps it was when inside the isr).
> > 
> > What arch have you been testing this on?
> It have been tested on arm.
> 

Things might be different there. I suggest you also try to get it
tested on x86, where this driver sees most of its use.

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

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

Re: sdhci can turn off irq up to 200 ms

[Matthieu CASTET]

Pierre Ossman a écrit :
> On Mon, 7 Sep 2009 11:56:10 +0200
> Matthieu CASTET <matthieu.castet@parrot.com> wrote:
> 
>> Pierre Ossman a écrit :
>>> I seem to recall having problems with jiffies not updating with those
>>> locks held (or perhaps it was when inside the isr).
>>>
>>> What arch have you been testing this on?
>> It have been tested on arm.
>>
> 
> Things might be different there. I suggest you also try to get it
> tested on x86, where this driver sees most of its use.
> 
I don't have sdhci device on x86. Can somebody on this list try it ?

Thanks

Matthieu

Re: sdhci can turn off irq up to 200 ms

[Daniel J Blueman]

On Jul 9, 11:30 am, Matthieu CASTET <matthieu.cas...@parrot.com>
wrote:
> Matthieu CASTET a écrit :
>
> > Hi,
>
> > 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
> > }
>
> > The problem is that sdhci_reset [1] does busy pooling on a register up
> > to a timeout of 100 ms.
> > That's not low latency friendly.
>
> > On our system, we saw that sdhci_reset take 1 ms. That should be because
> > we enter in mdelay, even if the hardware clears the bit faster.
> > I wonder why there is an mdelay(1). Using cpu_relax and
> > time_is_after_jiffies should make sdhci_reset faster.
>
> In case somebody cares, here a patch that reduce on our hardware
> sdhci_reset from 1 ms to 30 us.

On my Core2Duo, cpu_relax (implementing rep;nop) takes 3.2ns, but a
(synchronous) read over the PCI bus takes 0.5-1us, so it's hard to say
how much benefit the cpu_relax call will give, or am I missing
something?

If the code is reading from a memory location, or perhaps writing to
non-writethrough memory, it's a different story.

Daniel
--
Daniel J Blueman