| Message ID | e13e177c732ee8ad2d6fe389c9bac21877f60c59.1313400777.git.mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 15, 2011 at 01:03:37PM +0300, Mika Westerberg wrote: > There are few places where we want to make sure that no clock gating takes > place. For example when we are updating several related fields in ios > structure and we don't want to accidentally pass the partially filled ios > to the host driver. > > To solve this we add two functions to enable/disable the aggressive clock > gating where this is needed. Chris, Linus W, Do you have any comments on this series? I should've added a better explanation of the problem. In summary, to me it looks like the aggressive clock gating framework can cause an incomplete ios to be passed to the host driver if we are just updating it in MMC core, which could cause the host driver to do wrong things. In our case it happened during call of mmc_power_off(), resulting an BUG() to be triggered. Also in mmc_set_ios() there's a possibility that the aggressive clock gating triggers just after we have tested ios->clock > 0. Next time a request is started, the mmc_host_clk_ungate() is called but it doesn't ungate the clock as it is already marked as ungated. This leads to a hang in our case (as the clock is stopped). I think the same problem was previously solved in 26fc8775b51 ("mmc: fix a race between card-detect rescan and clock-gate work instances") which got reverted. Thanks. > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > Cc: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/mmc/core/host.c | 39 ++++++++++++++++++++++++++++++++++++++- > drivers/mmc/core/host.h | 11 +++++++++++ > include/linux/mmc/host.h | 1 + > 3 files changed, 50 insertions(+), 1 deletions(-) > > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c > index b29d3e8..ee52246 100644 > --- a/drivers/mmc/core/host.c > +++ b/drivers/mmc/core/host.c > @@ -178,7 +178,7 @@ void mmc_host_clk_gate(struct mmc_host *host) > spin_lock_irqsave(&host->clk_lock, flags); > host->clk_requests--; > if (mmc_host_may_gate_card(host->card) && > - !host->clk_requests) > + !host->clk_requests && !host->clk_disabled) > schedule_work(&host->clk_gate_work); > spin_unlock_irqrestore(&host->clk_lock, flags); > } > @@ -204,6 +204,42 @@ unsigned int mmc_host_clk_rate(struct mmc_host *host) > } > > /** > + * mmc_host_clk_gate_disable - temporarily disable clock gating > + * @host: host to disable clock gating > + * > + * Function temporarily disables aggressive clock gating. This is to > + * prevent clock gating worker to kick in for example in a middle of > + * ios structure update. > + * > + * After this function returns, it is guaranteed that no clock gating > + * takes place until it is re-enabled again. > + */ > +void mmc_host_clk_gate_disable(struct mmc_host *host) > +{ > + spin_lock_irq(&host->clk_lock); > + WARN_ON(host->clk_disabled); > + host->clk_disabled = true; > + spin_unlock_irq(&host->clk_lock); > + > + cancel_work_sync(&host->clk_gate_work); > +} > + > +/** > + * mmc_host_clk_gate_enable - re-enables clock gating > + * @host: host to re-enable clock gating > + * > + * Allows aggressive clock gating framework to continue gating the > + * host clock. > + */ > +void mmc_host_clk_gate_enable(struct mmc_host *host) > +{ > + spin_lock_irq(&host->clk_lock); > + WARN_ON(!host->clk_disabled); > + host->clk_disabled = false; > + spin_unlock_irq(&host->clk_lock); > +} > + > +/** > * mmc_host_clk_init - set up clock gating code > * @host: host with potential clock to control > */ > @@ -213,6 +249,7 @@ static inline void mmc_host_clk_init(struct mmc_host *host) > /* Hold MCI clock for 8 cycles by default */ > host->clk_delay = 8; > host->clk_gated = false; > + host->clk_disabled = false; > INIT_WORK(&host->clk_gate_work, mmc_host_clk_gate_work); > spin_lock_init(&host->clk_lock); > mutex_init(&host->clk_gate_mutex); > diff --git a/drivers/mmc/core/host.h b/drivers/mmc/core/host.h > index de199f9..7148b24 100644 > --- a/drivers/mmc/core/host.h > +++ b/drivers/mmc/core/host.h > @@ -19,6 +19,8 @@ void mmc_unregister_host_class(void); > void mmc_host_clk_ungate(struct mmc_host *host); > void mmc_host_clk_gate(struct mmc_host *host); > unsigned int mmc_host_clk_rate(struct mmc_host *host); > +void mmc_host_clk_gate_disable(struct mmc_host *host); > +void mmc_host_clk_gate_enable(struct mmc_host *host); > > #else > static inline void mmc_host_clk_ungate(struct mmc_host *host) > @@ -33,6 +35,15 @@ static inline unsigned int mmc_host_clk_rate(struct mmc_host *host) > { > return host->ios.clock; > } > + > +static inline void mmc_host_clk_gate_disable(struct mmc_host *host) > +{ > +} > + > +static inline void mmc_host_clk_gate_enable(struct mmc_host *host) > +{ > +} > + > #endif > > void mmc_host_deeper_disable(struct work_struct *work); > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h > index 1d09562..dea6350 100644 > --- a/include/linux/mmc/host.h > +++ b/include/linux/mmc/host.h > @@ -240,6 +240,7 @@ struct mmc_host { > unsigned int clk_old; /* old clock value cache */ > spinlock_t clk_lock; /* lock for clk fields */ > struct mutex clk_gate_mutex; /* mutex for clock gating */ > + bool clk_disabled; /* gating is temporarily disabled */ > #endif > > /* host specific block data */ > -- > 1.7.5.4 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 15, 2011 at 12:03 PM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > There are few places where we want to make sure that no clock gating takes > place. For example when we are updating several related fields in ios > structure and we don't want to accidentally pass the partially filled ios > to the host driver. > > To solve this we add two functions to enable/disable the aggressive clock > gating where this is needed. OK I realize the problem. There is a terminologty problem here: "disable" is a word used in the clock framework clk.h and can be very cofusing, since it means "turn off the clock" or "decrease refcount on clock by one", whereas here the meaning is the exact opposite. Please use some other word like "gate inhibit" or so. > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c > index b29d3e8..ee52246 100644 > --- a/drivers/mmc/core/host.c > +++ b/drivers/mmc/core/host.c > @@ -178,7 +178,7 @@ void mmc_host_clk_gate(struct mmc_host *host) > spin_lock_irqsave(&host->clk_lock, flags); > host->clk_requests--; > if (mmc_host_may_gate_card(host->card) && > - !host->clk_requests) > + !host->clk_requests && !host->clk_disabled) > schedule_work(&host->clk_gate_work); > spin_unlock_irqrestore(&host->clk_lock, flags); > } > @@ -204,6 +204,42 @@ unsigned int mmc_host_clk_rate(struct mmc_host *host) > } > > /** > + * mmc_host_clk_gate_disable - temporarily disable clock gating > + * @host: host to disable clock gating > + * > + * Function temporarily disables aggressive clock gating. This is to > + * prevent clock gating worker to kick in for example in a middle of > + * ios structure update. > + * > + * After this function returns, it is guaranteed that no clock gating > + * takes place until it is re-enabled again. > + */ > +void mmc_host_clk_gate_disable(struct mmc_host *host) So rename "mmc_host_clk_gate_inhibit() or so. > +{ > + spin_lock_irq(&host->clk_lock); > + WARN_ON(host->clk_disabled); > + host->clk_disabled = true; > + spin_unlock_irq(&host->clk_lock); > + > + cancel_work_sync(&host->clk_gate_work); > +} To inhibit clock gating it should be enough to increase the refcount on the requests: spin_lock_*(&host->clk_lock); host->clk_requests++; spin_unlock_*(&host->clk_lock); But if you look closer at mmc_host_clk_gate() that is exactly what that function does. > +/** > + * mmc_host_clk_gate_enable - re-enables clock gating > + * @host: host to re-enable clock gating > + * > + * Allows aggressive clock gating framework to continue gating the > + * host clock. > + */ > +void mmc_host_clk_gate_enable(struct mmc_host *host) > +{ > + spin_lock_irq(&host->clk_lock); > + WARN_ON(!host->clk_disabled); > + host->clk_disabled = false; > + spin_unlock_irq(&host->clk_lock); > +} spin_lock_*(&host->clk_lock); host->clk_requests--; spin_unlock_*(&host->clk_lock); But this is equivalent to mmc_host_clk_gate(). > diff --git a/drivers/mmc/core/host.h b/drivers/mmc/core/host.h > index de199f9..7148b24 100644 > --- a/drivers/mmc/core/host.h > +++ b/drivers/mmc/core/host.h > @@ -19,6 +19,8 @@ void mmc_unregister_host_class(void); > void mmc_host_clk_ungate(struct mmc_host *host); > void mmc_host_clk_gate(struct mmc_host *host); > unsigned int mmc_host_clk_rate(struct mmc_host *host); > +void mmc_host_clk_gate_disable(struct mmc_host *host); > +void mmc_host_clk_gate_enable(struct mmc_host *host); > > #else > static inline void mmc_host_clk_ungate(struct mmc_host *host) > @@ -33,6 +35,15 @@ static inline unsigned int mmc_host_clk_rate(struct mmc_host *host) > { > return host->ios.clock; > } > + > +static inline void mmc_host_clk_gate_disable(struct mmc_host *host) > +{ > +} > + > +static inline void mmc_host_clk_gate_enable(struct mmc_host *host) > +{ > +} > + > #endif > > void mmc_host_deeper_disable(struct work_struct *work); > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h > index 1d09562..dea6350 100644 > --- a/include/linux/mmc/host.h > +++ b/include/linux/mmc/host.h > @@ -240,6 +240,7 @@ struct mmc_host { > unsigned int clk_old; /* old clock value cache */ > spinlock_t clk_lock; /* lock for clk fields */ > struct mutex clk_gate_mutex; /* mutex for clock gating */ > + bool clk_disabled; /* gating is temporarily disabled */ This name does not work. Please call it "gating_inhibited" or something similar if you use this approach. I would suggest that in all patches using these functions, try to replace: mmc_host_clk_disable() -> mmc_host_clk_ungate() mmc_host_clk_enable() -> mmc_host_clk_gate() Please tell us if this works! I understand that the names can be a bit confusing by but I think you can convince yourself that what this will do is simply increase the refcount host->clk_requests so the clock is not gated across these sections. If you think the names of the functions are confusing then you may rename them, say like this: mmc_host_clk_ungate() -> mmc_host_clk_hold() mmc_host_clk_gate() -> mmc_host_clk_release() Which would make the usecases more clear, I'd be happy to ACK a patch for this. Thanks, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 17, 2011 at 09:51:31AM +0200, Linus Walleij wrote: > > I would suggest that in all patches using these functions, try > to replace: > > mmc_host_clk_disable() -> mmc_host_clk_ungate() > mmc_host_clk_enable() -> mmc_host_clk_gate() > Wow, that is indeed *much* cleaner way of doing this! One thing is that if I call these from those ios functions, mmc_host_clk_ungate() will always try to restore the clock even if there is really no need. Do you see this as a problem? > Please tell us if this works! Certainly. I'll try this overnight and see whether it works. > I understand that the names can be a bit confusing by but > I think you can convince yourself that what this will do is > simply increase the refcount host->clk_requests so the > clock is not gated across these sections. > > If you think the names of the functions are confusing then > you may rename them, say like this: > > mmc_host_clk_ungate() -> mmc_host_clk_hold() > mmc_host_clk_gate() -> mmc_host_clk_release() > > Which would make the usecases more clear, I'd be happy > to ACK a patch for this. I agree, I'll cook a patch for that also. Thanks for the comments. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 17, 2011 at 03:19:14PM +0300, Mika Westerberg wrote: > On Wed, Aug 17, 2011 at 09:51:31AM +0200, Linus Walleij wrote: > > > > I would suggest that in all patches using these functions, try > > to replace: > > > > mmc_host_clk_disable() -> mmc_host_clk_ungate() > > mmc_host_clk_enable() -> mmc_host_clk_gate() > > > > Wow, that is indeed *much* cleaner way of doing this! > > One thing is that if I call these from those ios functions, > mmc_host_clk_ungate() will always try to restore the clock > even if there is really no need. Do you see this as a problem? > > > Please tell us if this works! > > Certainly. I'll try this overnight and see whether it works. The device survived overnight reboot loop test, so this scheme seems to work. I'll prepare a new series which uses mmc_host_clk_{ungate|gate}(). -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 18, 2011 at 1:34 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> I'll prepare a new series which uses mmc_host_clk_{ungate|gate}().
Mika, do the patches to check host->clock in
(mmc|sdhci)_set_data_timeout() become useless after?
If so, probably you could revert them as well.
On Thu, Aug 18, 2011 at 02:11:23PM +0300, Andy Shevchenko wrote: > Mika, do the patches to check host->clock in > (mmc|sdhci)_set_data_timeout() become useless after? > If so, probably you could revert them as well. IMHO these functions should work also when the clock is gated, so I suggest that we leave them as is for now. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c index b29d3e8..ee52246 100644 --- a/drivers/mmc/core/host.c +++ b/drivers/mmc/core/host.c @@ -178,7 +178,7 @@ void mmc_host_clk_gate(struct mmc_host *host) spin_lock_irqsave(&host->clk_lock, flags); host->clk_requests--; if (mmc_host_may_gate_card(host->card) && - !host->clk_requests) + !host->clk_requests && !host->clk_disabled) schedule_work(&host->clk_gate_work); spin_unlock_irqrestore(&host->clk_lock, flags); } @@ -204,6 +204,42 @@ unsigned int mmc_host_clk_rate(struct mmc_host *host) } /** + * mmc_host_clk_gate_disable - temporarily disable clock gating + * @host: host to disable clock gating + * + * Function temporarily disables aggressive clock gating. This is to + * prevent clock gating worker to kick in for example in a middle of + * ios structure update. + * + * After this function returns, it is guaranteed that no clock gating + * takes place until it is re-enabled again. + */ +void mmc_host_clk_gate_disable(struct mmc_host *host) +{ + spin_lock_irq(&host->clk_lock); + WARN_ON(host->clk_disabled); + host->clk_disabled = true; + spin_unlock_irq(&host->clk_lock); + + cancel_work_sync(&host->clk_gate_work); +} + +/** + * mmc_host_clk_gate_enable - re-enables clock gating + * @host: host to re-enable clock gating + * + * Allows aggressive clock gating framework to continue gating the + * host clock. + */ +void mmc_host_clk_gate_enable(struct mmc_host *host) +{ + spin_lock_irq(&host->clk_lock); + WARN_ON(!host->clk_disabled); + host->clk_disabled = false; + spin_unlock_irq(&host->clk_lock); +} + +/** * mmc_host_clk_init - set up clock gating code * @host: host with potential clock to control */ @@ -213,6 +249,7 @@ static inline void mmc_host_clk_init(struct mmc_host *host) /* Hold MCI clock for 8 cycles by default */ host->clk_delay = 8; host->clk_gated = false; + host->clk_disabled = false; INIT_WORK(&host->clk_gate_work, mmc_host_clk_gate_work); spin_lock_init(&host->clk_lock); mutex_init(&host->clk_gate_mutex); diff --git a/drivers/mmc/core/host.h b/drivers/mmc/core/host.h index de199f9..7148b24 100644 --- a/drivers/mmc/core/host.h +++ b/drivers/mmc/core/host.h @@ -19,6 +19,8 @@ void mmc_unregister_host_class(void); void mmc_host_clk_ungate(struct mmc_host *host); void mmc_host_clk_gate(struct mmc_host *host); unsigned int mmc_host_clk_rate(struct mmc_host *host); +void mmc_host_clk_gate_disable(struct mmc_host *host); +void mmc_host_clk_gate_enable(struct mmc_host *host); #else static inline void mmc_host_clk_ungate(struct mmc_host *host) @@ -33,6 +35,15 @@ static inline unsigned int mmc_host_clk_rate(struct mmc_host *host) { return host->ios.clock; } + +static inline void mmc_host_clk_gate_disable(struct mmc_host *host) +{ +} + +static inline void mmc_host_clk_gate_enable(struct mmc_host *host) +{ +} + #endif void mmc_host_deeper_disable(struct work_struct *work); diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index 1d09562..dea6350 100644 --- a/include/linux/mmc/host.h +++ b/include/linux/mmc/host.h @@ -240,6 +240,7 @@ struct mmc_host { unsigned int clk_old; /* old clock value cache */ spinlock_t clk_lock; /* lock for clk fields */ struct mutex clk_gate_mutex; /* mutex for clock gating */ + bool clk_disabled; /* gating is temporarily disabled */ #endif /* host specific block data */
There are few places where we want to make sure that no clock gating takes place. For example when we are updating several related fields in ios structure and we don't want to accidentally pass the partially filled ios to the host driver. To solve this we add two functions to enable/disable the aggressive clock gating where this is needed. Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> Cc: Linus Walleij <linus.walleij@linaro.org> --- drivers/mmc/core/host.c | 39 ++++++++++++++++++++++++++++++++++++++- drivers/mmc/core/host.h | 11 +++++++++++ include/linux/mmc/host.h | 1 + 3 files changed, 50 insertions(+), 1 deletions(-)