[RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management

[RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management

[Philip Rakity]

Pierre,

I am preparing a patch to just diable to interrupt line to the SD controller so global interrupts are not locked out.  This
will allow mdelay to work correctly in the routines the set_ios() calls.

The pm_ calls  remove the lock before being called and restore it afterwards.  Is this just to ensure interrupts are globally
enabled ?  If so, I can simplify my patch deleting the original spin_lock calls that surround the pm_ calls.

Philip


>From 782c474f1abe992a203ad76594db025414332e67 Mon Sep 17 00:00:00 2001
From: Philip Rakity <prakity@marvell.com>
Date: Tue, 25 Jan 2011 16:04:42 -0800
Subject: [PATCH 1/2] sdhci: sdhci.c: Do not disable global interrupts while waiting in set_ios

holding the spin_lock with all interrupts disabled
is not good form when doing mdelay.
set_ios calls routines that need to delay for power to
stabalize or for chip issues when quirks are defined.

disable only our interupt and spin_lock on the chip
instead of spin_lock with all interrupts disabled.

Signed-off-by: Philip Rakity <prakity@marvell.com>
Signed-off-by: Mark F. Brown <markb@marvell.com>
---
 drivers/mmc/host/sdhci.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 655617c..1a88303 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1161,13 +1161,13 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct sdhci_host *host;
-	unsigned long flags;
 	unsigned int lastclock;
 	u8 ctrl;
 
 	host = mmc_priv(mmc);
 
-	spin_lock_irqsave(&host->lock, flags);
+	disable_irq(host->irq);
+	spin_lock(&host->lock);
 
 	if (host->flags & SDHCI_DEVICE_DEAD)
 		goto out;
@@ -1180,14 +1180,14 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	lastclock = host->iosclock;
 	host->iosclock = ios->clock;
 	if (lastclock == 0 && ios->clock != 0) {
-		spin_unlock_irqrestore(&host->lock, flags);
+		spin_unlock(&host->lock);
 		pm_runtime_get_sync(host->mmc->parent);
-		spin_lock_irqsave(&host->lock, flags);
+		spin_lock(&host->lock);
 	} else if (lastclock != 0 && ios->clock == 0) {
-		spin_unlock_irqrestore(&host->lock, flags);
+		spin_unlock(&host->lock);
 		pm_runtime_mark_last_busy(host->mmc->parent);
 		pm_runtime_put_autosuspend(host->mmc->parent);
-		spin_lock_irqsave(&host->lock, flags);
+		spin_lock(&host->lock);
 	}
 	/* no need to configure the rest.. */
 	if (host->iosclock == 0)
@@ -1257,7 +1257,8 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 
 out:
 	mmiowb();
-	spin_unlock_irqrestore(&host->lock, flags);
+	spin_unlock(&host->lock);
+	enable_irq(host->irq);
 }
 
 static int sdhci_get_ro(struct mmc_host *mmc)
-- 
1.7.0.4

Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management

[Pierre Tardy]

On Sun, Feb 13, 2011 at 6:39 AM, Philip Rakity <prakity@marvell.com> wrote:
>
> Pierre,
>
> I am preparing a patch to just diable to interrupt line to the SD controller so global interrupts are not locked out.
I think I already have seen previous version of that patch earlier. It
does not sound like a bad idea. However, disabling all interrupt is
really cheap, and disabling only one is a bit more expensive. AFAIK,
spin_lock and co are really meant for very short time locks.

>  This
> will allow mdelay to work correctly in the routines the set_ios() calls.
This sounds a little bit worse. mdelay is never good, as this is
active wait, that is loosing tons of good cpu cycles.
Do we really need those? Can we do the same thing asynchronous?

>
> The pm_ calls  remove the lock before being called and restore it afterwards.  Is this just to ensure interrupts are globally
> enabled ?
At least on my platform, pm_ calls cannot be done in atomic context,
and sdhci runtime_pm flow will call set_ios() back, so you will
re-enter the function.

> If so, I can simplify my patch deleting the original spin_lock calls that surround the pm_ calls.
No, you cannot safely remove the spin_lock calls here.

Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management

[Philip Rakity]

On Feb 13, 2011, at 2:25 AM, Pierre Tardy wrote:

> On Sun, Feb 13, 2011 at 6:39 AM, Philip Rakity <prakity@marvell.com> wrote:
>> 
>> Pierre,
>> 
>> I am preparing a patch to just diable to interrupt line to the SD controller so global interrupts are not locked out.
> I think I already have seen previous version of that patch earlier. It
> does not sound like a bad idea. However, disabling all interrupt is
> really cheap, and disabling only one is a bit more expensive. AFAIK,
> spin_lock and co are really meant for very short time locks.
> 
>>  This
>> will allow mdelay to work correctly in the routines the set_ios() calls.
> This sounds a little bit worse. mdelay is never good, as this is
> active wait, that is loosing tons of good cpu cycles.

do not need active waiting.  Good point.

> Do we really need those? Can we do the same thing asynchronous?

This might be possible without a restructure to separate out all the sub-calls that are
done in set_ios. Otherwise  we would require a restructure of the code in core/ to not use
set_ios but to call only the function they need via separate call in's.
(for example setting the width or power) and the associated changes in all the drivers in host.

Rather then do mdelay we could use:  (code from core.h)

static inline void mmc_delay(unsigned int ms)
{
	if (ms < 1000 / HZ) {
		cond_resched();
		mdelay(ms);
	} else {
		msleep(ms);
	}
}

Since the whole point of locking out the one interrupt line is to let everything else
work this look attractive.   -- Opintions Please --

> 
>> 
>> The pm_ calls  remove the lock before being called and restore it afterwards.  Is this just to ensure interrupts are globally
>> enabled ?
> At least on my platform, pm_ calls cannot be done in atomic context,
> and sdhci runtime_pm flow will call set_ios() back, so you will
> re-enter the function.

ok

> 
>>  If so, I can simplify my patch deleting the original spin_lock calls that surround the pm_ calls.
> No, you cannot safely remove the spin_lock calls here.

ok

Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management

[Philip Rakity]

Pierre,

are you comfortable with this ?

Philip

>From 85b200161edac97fc2817ad3f213795df99cd7e8 Mon Sep 17 00:00:00 2001
From: Philip Rakity <prakity@marvell.com>
Date: Tue, 25 Jan 2011 16:04:42 -0800
Subject: [PATCH] sdhci: sdhci.c: Do not disable global interrupts while waiting in set_ios

holding the spin_lock with all interrupts disabled
is not good form when doing mdelay.
set_ios calls routines that need to delay for power to
stabalize or for chip issues when quirks are defined.

disable only our interupt and spin_lock on the chip
instead of spin_lock with all interrupts disabled.

use mmc_delay (if possible) rather then mdelay
to avoid active waits. sdhci_reset not changed.
still uses mdelay as we do not know if interrupts
have been disabled so we play "safe"

Signed-off-by: Philip Rakity <prakity@marvell.com>
---
 drivers/mmc/core/core.h  |   12 ------------
 drivers/mmc/host/sdhci.c |   26 +++++++++++++++++---------
 include/linux/mmc/host.h |   11 +++++++++++
 3 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 20b1c08..146b6f6 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -11,8 +11,6 @@
 #ifndef _MMC_CORE_CORE_H
 #define _MMC_CORE_CORE_H
 
-#include <linux/delay.h>
-
 #define MMC_CMD_RETRIES        3
 
 struct mmc_bus_ops {
@@ -43,16 +41,6 @@ void mmc_set_bus_width_ddr(struct mmc_host *host, unsigned int width,
 u32 mmc_select_voltage(struct mmc_host *host, u32 ocr);
 void mmc_set_timing(struct mmc_host *host, unsigned int timing);
 
-static inline void mmc_delay(unsigned int ms)
-{
-	if (ms < 1000 / HZ) {
-		cond_resched();
-		mdelay(ms);
-	} else {
-		msleep(ms);
-	}
-}
-
 void mmc_rescan(struct work_struct *work);
 void mmc_start_host(struct mmc_host *host);
 void mmc_stop_host(struct mmc_host *host);
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 655617c..2d7fbd1 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1043,7 +1043,7 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 			return;
 		}
 		timeout--;
-		mdelay(1);
+		mmc_delay(1);
 	}
 
 	clk |= SDHCI_CLOCK_CARD_EN;
@@ -1108,7 +1108,7 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned short power)
 	 * can apply clock after applying power
 	 */
 	if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER)
-		mdelay(10);
+		mmc_delay(10);
 }
 
 /*****************************************************************************\
@@ -1161,22 +1161,21 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct sdhci_host *host;
-	unsigned long flags;
 	unsigned int lastclock;
+	unsigned long flags;
 	u8 ctrl;
 
 	host = mmc_priv(mmc);
 
-	spin_lock_irqsave(&host->lock, flags);
-
 	if (host->flags & SDHCI_DEVICE_DEAD)
-		goto out;
+		return;
 
 	/*
 	 * get/put runtime_pm usage counter at ios->clock transitions
 	 * We need to do it before any other chip access, as sdhci could
 	 * be power gated
 	 */
+	spin_lock_irqsave(&host->lock, flags);
 	lastclock = host->iosclock;
 	host->iosclock = ios->clock;
 	if (lastclock == 0 && ios->clock != 0) {
@@ -1189,9 +1188,18 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		pm_runtime_put_autosuspend(host->mmc->parent);
 		spin_lock_irqsave(&host->lock, flags);
 	}
+	spin_unlock_irqrestore(&host->lock, flags);
+
 	/* no need to configure the rest.. */
 	if (host->iosclock == 0)
-		goto out;
+		return;
+
+	/* disable local interrupts and lock structures so
+	 * that mmc_delay can be scheduled.  This cannot happen
+	 * if interrupts are globally disabled
+	 */
+	disable_irq(host->irq);
+	spin_lock(&host->lock);
 
 	/*
 	 * Reset the chip on each power off.
@@ -1255,9 +1263,9 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	if(host->quirks & SDHCI_QUIRK_RESET_CMD_DATA_ON_IOS)
 		sdhci_reset(host, SDHCI_RESET_CMD | SDHCI_RESET_DATA);
 
-out:
 	mmiowb();
-	spin_unlock_irqrestore(&host->lock, flags);
+	spin_unlock(&host->lock);
+	enable_irq(host->irq);
 }
 
 static int sdhci_get_ro(struct mmc_host *mmc)
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index bcb793e..d9edc06 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -15,6 +15,7 @@
 
 #include <linux/mmc/core.h>
 #include <linux/mmc/pm.h>
+#include <linux/delay.h>
 
 struct mmc_ios {
 	unsigned int	clock;			/* clock rate */
@@ -326,5 +327,15 @@ static inline int mmc_card_is_powered_resumed(struct mmc_host *host)
 	return host->pm_flags & MMC_PM_KEEP_POWER;
 }
 
+static inline void mmc_delay(unsigned int ms)
+{
+	if (ms < 1000 / HZ) {
+		cond_resched();
+		mdelay(ms);
+	} else {
+		msleep(ms);
+	}
+}
+
 #endif
 
-- 
1.7.0.4

Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management

[Pierre Tardy]

On Mon, Feb 14, 2011 at 6:50 AM, Philip Rakity <prakity@marvell.com> wrote:
>
> Pierre,
>
> are you comfortable with this ?
The mmc_delay() looks hackish at the first sight. Looks like Pierre
Ossman was the author, he must have his reasons.

It looks like the mdelay are not in the fastpath, so why bother.

And, more important, you will do cond_resched while holding you
spinlock, which is *bad*.
What if the mmc stack will call you again from another thread? deadlock...


> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
> -static inline void mmc_delay(unsigned int ms)
why do you want to remove it from here ?
It is still used by the core code.


> @@ -1043,7 +1043,7 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
>                        return;
>                }
>                timeout--;
> -               mdelay(1);
> +               mmc_delay(1);
This looks like timeout code that never happen in normal condition.


> @@ -1108,7 +1108,7 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned short power)
>         * can apply clock after applying power
>         */
>        if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER)
> -               mdelay(10);
> +               mmc_delay(10);
Do you need this quirk in your platform?


Pierre

Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management

[Wolfram Sang]

> It looks like the mdelay are not in the fastpath, so why bother.

On embedded systems, we had audio streaming or CAN communication or similar
running in parallel. Currently, those mdelays (there are a couple, sadly)
produce significant latencies, e.g. when inserting/removing a card. The latter
gets especially bad with BROKEN_CARD_DETECTION.

The proper solution is probably to check the locking and see if it can be made
fine-granular, I'd assume?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management

[Philip Rakity]

I wonder if I can remove the locking (spin_lock).  The disable and enable irq should be enough.  

Thoughts ..

Philip

On Feb 14, 2011, at 1:55 AM, Wolfram Sang wrote:

> 
>> It looks like the mdelay are not in the fastpath, so why bother.
> 
> On embedded systems, we had audio streaming or CAN communication or similar
> running in parallel. Currently, those mdelays (there are a couple, sadly)
> produce significant latencies, e.g. when inserting/removing a card. The latter
> gets especially bad with BROKEN_CARD_DETECTION.
> 
> The proper solution is probably to check the locking and see if it can be made
> fine-granular, I'd assume?
> 
> Regards,
> 
>   Wolfram
> 
> -- 
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management

[Philip Rakity]

On Feb 14, 2011, at 1:29 AM, Pierre Tardy wrote:

> On Mon, Feb 14, 2011 at 6:50 AM, Philip Rakity <prakity@marvell.com> wrote:
>> 
>> Pierre,
>> 
>> are you comfortable with this ?
> The mmc_delay() looks hackish at the first sight. Looks like Pierre
> Ossman was the author, he must have his reasons.
> 
> It looks like the mdelay are not in the fastpath, so why bother.
> 
> And, more important, you will do cond_resched while holding you
> spinlock, which is *bad*.
> What if the mmc stack will call you again from another thread? deadlock...


Assumptions -- Please Confirm 
-------------------------------------------

There is a one to one relationship
between the calls in the core layer into the SD driver.

Each thread is independent of the other.

Each thread maps into exactly one register instance for the SD driver

eg --> 2 threads cannot be active at the same time that use the same
registers in the SD driver.

Question
------------

If these assumptions are correct how can the deadlock happen ?
host->lock is held on a per thread basis.

======

> 
> 
>> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
>> -static inline void mmc_delay(unsigned int ms)
> why do you want to remove it from here ?
> It is still used by the core code.

moved to include/linux/mmc so okay for sdhci driver.  Did not want to duplicate code.

> 
> 
>> @@ -1043,7 +1043,7 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
>>                        return;
>>                }
>>                timeout--;
>> -               mdelay(1);
>> +               mmc_delay(1);
> This looks like timeout code that never happen in normal condition.
> 
> 
>> @@ -1108,7 +1108,7 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned short power)
>>         * can apply clock after applying power
>>         */
>>        if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER)
>> -               mdelay(10);
>> +               mmc_delay(10);
> Do you need this quirk in your platform?

No

> 
> 
> Pierre

RE: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management

[Tardy, Pierre]

Philip,
> > And, more important, you will do cond_resched while holding you
> > spinlock, which is *bad*.
> > What if the mmc stack will call you again from another thread? deadlock...

> Assumptions -- Please Confirm
> -------------------------------------------
No need to do assumptions, schedule while holding spinlocks is bad, your kernel will oops with something like:
BUG: scheduling while in atomic.


> >> @@ -1108,7 +1108,7 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned short power)
> >>         * can apply clock after applying power
> >>         */
> >>        if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER)
> >> -               mdelay(10);
> >> +               mmc_delay(10);
> > Do you need this quirk in your platform?
> 
> No
Then, you dont have any mdelay in your set_ios, and you are trying to optimize something that never happen.

Pierre

---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management

[Philip Rakity]

According to Zhangfei Gao -- android crashes when we are code to handle dual data rate because of the mdelay of 5ms that 
is needed for the voltage to become stable.  (see my ddr patch) 

I have never seen this problem with android.

The dual data rate code is in another patch;  it adds another call into set_ios to handle this.  

His comment was doing mdelay stopped the system from being scheduled since mdelay is done with interrupts disabled.  It also
stopped the system for keeping correct time.  

If doing mdelay breaks android then the original code (no ddr code) is wrong since mdelay should not be issued with
interrupts disabled and depending on the quirks defined the delays can be very large.

Wanted to handle his concern.


On Feb 14, 2011, at 10:41 AM, Tardy, Pierre wrote:

> Philip,
>>> And, more important, you will do cond_resched while holding you
>>> spinlock, which is *bad*.
>>> What if the mmc stack will call you again from another thread? deadlock...
> 
>> Assumptions -- Please Confirm
>> -------------------------------------------
> No need to do assumptions, schedule while holding spinlocks is bad, your kernel will oops with something like:
> BUG: scheduling while in atomic.
> 
> 
>>>> @@ -1108,7 +1108,7 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned short power)
>>>>        * can apply clock after applying power
>>>>        */
>>>>       if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER)
>>>> -               mdelay(10);
>>>> +               mmc_delay(10);
>>> Do you need this quirk in your platform?
>> 
>> No
> Then, you dont have any mdelay in your set_ios, and you are trying to optimize something that never happen.
> 
> Pierre
> 
> ---------------------------------------------------------------------
> Intel Corporation SAS (French simplified joint stock company)
> Registered headquarters: "Les Montalets"- 2, rue de Paris, 
> 92196 Meudon Cedex, France
> Registration Number:  302 456 199 R.C.S. NANTERRE
> Capital: 4,572,000 Euros
> 
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
> 

Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management

[Philip Rakity]

Pierre,

The spin_lock may not be needed if the same thread cannot be re-entered.

That was the reason for the assumptions.  Sorry about not being clear.

Philip

On Feb 14, 2011, at 10:41 AM, Tardy, Pierre wrote:

> Philip,
>>> And, more important, you will do cond_resched while holding you
>>> spinlock, which is *bad*.
>>> What if the mmc stack will call you again from another thread? deadlock...
> 
>> Assumptions -- Please Confirm
>> -------------------------------------------
> No need to do assumptions, schedule while holding spinlocks is bad, your kernel will oops with something like:
> BUG: scheduling while in atomic.
> 
> 
>>>> @@ -1108,7 +1108,7 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned short power)
>>>>        * can apply clock after applying power
>>>>        */
>>>>       if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER)
>>>> -               mdelay(10);
>>>> +               mmc_delay(10);
>>> Do you need this quirk in your platform?
>> 
>> No
> Then, you dont have any mdelay in your set_ios, and you are trying to optimize something that never happen.
> 
> Pierre
> 
> ---------------------------------------------------------------------
> Intel Corporation SAS (French simplified joint stock company)
> Registered headquarters: "Les Montalets"- 2, rue de Paris, 
> 92196 Meudon Cedex, France
> Registration Number:  302 456 199 R.C.S. NANTERRE
> Capital: 4,572,000 Euros
> 
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
> 

Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management

[Philip Rakity]

On Feb 14, 2011, at 1:29 AM, Pierre Tardy wrote:

> On Mon, Feb 14, 2011 at 6:50 AM, Philip Rakity <prakity@marvell.com> wrote:
>> 
>> Pierre,
>> 
>> are you comfortable with this ?
> The mmc_delay() looks hackish at the first sight. Looks like Pierre
> Ossman was the author, he must have his reasons.
> 
> It looks like the mdelay are not in the fastpath, so why bother.
> 
> And, more important, you will do cond_resched while holding you
> spinlock, which is *bad*.
> What if the mmc stack will call you again from another thread? deadlock...

The original code used  a 	spin_lock_irqsave and  a spin_lock_irqrestore so
if the code was called again the deadlock surely would have happened by now.

> 
> 
>> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
>> -static inline void mmc_delay(unsigned int ms)
> why do you want to remove it from here ?
> It is still used by the core code.
> 
> 
>> @@ -1043,7 +1043,7 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
>>                        return;
>>                }
>>                timeout--;
>> -               mdelay(1);
>> +               mmc_delay(1);
> This looks like timeout code that never happen in normal condition.
> 
> 
>> @@ -1108,7 +1108,7 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned short power)
>>         * can apply clock after applying power
>>         */
>>        if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER)
>> -               mdelay(10);
>> +               mmc_delay(10);
> Do you need this quirk in your platform?
> 
> 
> Pierre

Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management

[Wolfram Sang]

On Mon, Feb 14, 2011 at 06:41:47PM +0000, Tardy, Pierre wrote:
> Philip,
> > > And, more important, you will do cond_resched while holding you
> > > spinlock, which is *bad*.
> > > What if the mmc stack will call you again from another thread? deadlock...
> 
> > Assumptions -- Please Confirm
> > -------------------------------------------
> No need to do assumptions, schedule while holding spinlocks is bad, your kernel will oops with something like:
> BUG: scheduling while in atomic.

I thought the assumptions were made in order to check where the spinlock is
_really_ needed. Thinking in that direction is a good aproach IMO.

> > >> @@ -1108,7 +1108,7 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned short power)
> > >>         * can apply clock after applying power
> > >>         */
> > >>        if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER)
> > >> -               mdelay(10);
> > >> +               mmc_delay(10);
> > > Do you need this quirk in your platform?
> > 
> > No
> Then, you dont have any mdelay in your set_ios, and you are trying to optimize something that never happen.

There are some more mdelays in error-conditions which could happen occasionally
(see sdhci_send_command, spinlocked by sdhci_request for example).

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management

[Wolfram Sang]

On Mon, Feb 14, 2011 at 09:27:52AM -0800, Philip Rakity wrote:
> 
> I wonder if I can remove the locking (spin_lock).  The disable and enable irq should be enough.  

I discussed this shortly with Sascha yesterday and we agree the core serializes
requests to the host (by means of mmc_claim_host and synchronus requests).
Protecting only the interrupts works for drivers like pxa and mxc. Such locking
might or might not be not enough for sdhci, but that would have to be carefully
evaluated. Should be worth trying, though (unless we missed something).

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |