headers
Uwe Kleine-König | 1 Feb 2011 11:54
Picon
Favicon
Gravatar

Re: Locking in the clk API, part 2: clk_prepare/clk_unprepare

Hello Jeremy,

On Tue, Feb 01, 2011 at 05:11:29PM +0800, Jeremy Kerr wrote:
> > I suggested that clk_prepare() be callable only from non-atomic contexts,
> > and do whatever's required to ensure that the clock is available.  That
> > may end up enabling the clock as a result.
> 
> I think that clk_prepare/clk_unprepare looks like the most promising solution, 
> so will try to get some preliminary patches done. Here's what I'm planning:
> 
> -----
> 
> The changes to the API are essentially:
> 
> 1) Document clk_enable/clk_disable as callable from atomic contexts, and
>    so clock implementations must not sleep within this function.
> 
> 2) For clock implementations that may sleep to turn on a clock, we add a
>    new pair of functions to the clock API: clk_prepare and clk_unprepare.
> 
>    These will provide hooks for the clock implmentation to do any sleepable
>    work (eg, wait for PLLs to settle) in preparation for a later clk_enable.
> 
>    For the most common clock implemntation cases (where clocks can be enabled 
>    atomically), these functions will be a no-op, and all of the enable/disable
>    work can be done in clk_enable/clk_disable.
> 
>    For implementations where clocks require blocking on enable/disable, most
>    of the work will be done in clk_prepare/clk_unprepare. The clk_enable
>    and clk_disable functions may be no-ops.
(Continue reading)

Permalink | Reply | 33 comments |
headers
Jassi Brar | 1 Feb 2011 14:05
Picon

Re: Locking in the clk API, part 2: clk_prepare/clk_unprepare

2011/2/1 Uwe Kleine-König <u.kleine-koenig <at> pengutronix.de>:

.....

> Do you plan to handle the case that clk_enable is called while prepare
> isn't completed (considering the special case "not called at all")?
> Maybe BUG_ON(clk->ops->prepare && !clk->prepare_count)?
Sounds better than the second option.

> Alternatively don't force the sleep in clk_prepare (e.g. by protecting
> prepare_count by a spinlock (probably enable_lock)) and call clk_prepare
> before calling clk->ops->enable?
That might result in a driver working on some platforms(those have
atomic clk_prepare)
and not on others(those have sleeping).

Njoi!
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Permalink | Reply | 6 comments |
headers
Russell King - ARM Linux | 1 Feb 2011 14:15
Picon

Re: Locking in the clk API, part 2: clk_prepare/clk_unprepare

On Tue, Feb 01, 2011 at 11:54:49AM +0100, Uwe Kleine-König wrote:
> Alternatively don't force the sleep in clk_prepare (e.g. by protecting
> prepare_count by a spinlock (probably enable_lock)) and call clk_prepare
> before calling clk->ops->enable?

That's a completely bad idea.  I assume you haven't thought about this
very much.

There's two ways I can think of doing what you're suggesting:

int clk_prepare(struct clk *clk)
{
	unsigned long flags;
	int ret = 0;

	might_sleep();

	spin_lock_irqsave(&clk->enable_lock, flags);
	if (clk->prepare_count++ == 0)
		ret = clk->ops->prepare(clk);
	spin_unlock_irqrestore(&clk->enable_clock, flags);

	return ret;
}

The problem is that clk->ops->prepare() is called in a non-sleepable
context.  So this breaks the whole idea of clk_prepare(), and so isn't
a solution.

The other solution is:
(Continue reading)

Permalink | Reply | 25 comments |
headers
Uwe Kleine-König | 1 Feb 2011 15:00
Picon
Favicon
Gravatar

Re: Locking in the clk API, part 2: clk_prepare/clk_unprepare

On Tue, Feb 01, 2011 at 10:05:56PM +0900, Jassi Brar wrote:
> 2011/2/1 Uwe Kleine-König <u.kleine-koenig <at> pengutronix.de>:
> 
> .....
> 
> > Do you plan to handle the case that clk_enable is called while prepare
> > isn't completed (considering the special case "not called at all")?
> > Maybe BUG_ON(clk->ops->prepare && !clk->prepare_count)?
> Sounds better than the second option.
> 
> > Alternatively don't force the sleep in clk_prepare (e.g. by protecting
> > prepare_count by a spinlock (probably enable_lock)) and call clk_prepare
> > before calling clk->ops->enable?
> That might result in a driver working on some platforms(those have
> atomic clk_prepare)
> and not on others(those have sleeping).
The first option has the same result.  E.g. on some platforms
clk->ops->prepare might be NULL, on others it's not.

Uwe

--

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
(Continue reading)

Permalink | Reply | 5 comments |
headers
Uwe Kleine-König | 1 Feb 2011 15:18
Picon
Favicon
Gravatar

Re: Locking in the clk API, part 2: clk_prepare/clk_unprepare

On Tue, Feb 01, 2011 at 01:15:12PM +0000, Russell King - ARM Linux wrote:
> On Tue, Feb 01, 2011 at 11:54:49AM +0100, Uwe Kleine-König wrote:
> > Alternatively don't force the sleep in clk_prepare (e.g. by protecting
> > prepare_count by a spinlock (probably enable_lock)) and call clk_prepare
> > before calling clk->ops->enable?
> 
> That's a completely bad idea.  I assume you haven't thought about this
> very much.
Right, but I thought it a bit further than you did.  Like the following:

int clk_prepare(struct clk *clk)
{
	int ret = 0, first;
	unsigned long flags;

	spin_lock_irqsave(&clk->enable_lock, flags);
	if (clk->flags & CLK_BUSY) {
		/* 
		 * this must not happen, please serialize calls to
		 * clk_prepare/clk_enable
		 */
		ret = -EBUSY;
		goto out_unlock;
	}
	first = clk->prepare_count++ == 0;
	if (first)
		clk->flags |= CLK_BUSY;
	spin_unlock_irqrestore(&clk->enable_lock, flags);

	if (!first)
(Continue reading)

Permalink | Reply | 24 comments |
headers
Russell King - ARM Linux | 1 Feb 2011 15:39
Picon

Re: Locking in the clk API, part 2: clk_prepare/clk_unprepare

On Tue, Feb 01, 2011 at 03:18:37PM +0100, Uwe Kleine-König wrote:
> On Tue, Feb 01, 2011 at 01:15:12PM +0000, Russell King - ARM Linux wrote:
> > On Tue, Feb 01, 2011 at 11:54:49AM +0100, Uwe Kleine-König wrote:
> > > Alternatively don't force the sleep in clk_prepare (e.g. by protecting
> > > prepare_count by a spinlock (probably enable_lock)) and call clk_prepare
> > > before calling clk->ops->enable?
> > 
> > That's a completely bad idea.  I assume you haven't thought about this
> > very much.
> Right, but I thought it a bit further than you did.  Like the following:
>  
> int clk_prepare(struct clk *clk)
> {
> 	int ret = 0, first;
> 	unsigned long flags;
> 
> 	spin_lock_irqsave(&clk->enable_lock, flags);
> 	if (clk->flags & CLK_BUSY) {
> 		/* 
> 		 * this must not happen, please serialize calls to
> 		 * clk_prepare/clk_enable
> 		 */

How do different drivers serialize calls to clk_prepare?  Are you
really suggesting that we should have a global mutex somewhere to
prevent this?

> 		ret = -EBUSY;
> 		goto out_unlock;
> 	}
(Continue reading)

Permalink | Reply | 22 comments |
headers
Jeremy Kerr | 1 Feb 2011 15:40
Favicon

Re: Locking in the clk API, part 2: clk_prepare/clk_unprepare

Hi Uwe,

Thanks for the feedback, I'm not sure I like the more complex approach though:

> Right, but I thought it a bit further than you did.  Like the following:
> 
> int clk_prepare(struct clk *clk)
> {
> 	int ret = 0, first;
> 	unsigned long flags;
> 
> 	spin_lock_irqsave(&clk->enable_lock, flags);
> 	if (clk->flags & CLK_BUSY) {
> 		/*
> 		 * this must not happen, please serialize calls to
> 		 * clk_prepare/clk_enable
> 		 */
> 		ret = -EBUSY;
> 		goto out_unlock;

Why is this an error? Two separate drivers may be clk_prepare()-ing at the 
same time, which should be acceptable. Both calls should block until the 
prepare is complete.

> 	}
> 	first = clk->prepare_count++ == 0;
> 	if (first)
> 		clk->flags |= CLK_BUSY;
> 	spin_unlock_irqrestore(&clk->enable_lock, flags);
>
(Continue reading)

Permalink | Reply | 0 comments |
headers
Russell King - ARM Linux | 1 Feb 2011 16:14
Picon

Re: Locking in the clk API, part 2: clk_prepare/clk_unprepare

On Tue, Feb 01, 2011 at 03:00:24PM +0100, Uwe Kleine-König wrote:
> On Tue, Feb 01, 2011 at 10:05:56PM +0900, Jassi Brar wrote:
> > 2011/2/1 Uwe Kleine-König <u.kleine-koenig <at> pengutronix.de>:
> > 
> > .....
> > 
> > > Do you plan to handle the case that clk_enable is called while prepare
> > > isn't completed (considering the special case "not called at all")?
> > > Maybe BUG_ON(clk->ops->prepare && !clk->prepare_count)?
> > Sounds better than the second option.
> > 
> > > Alternatively don't force the sleep in clk_prepare (e.g. by protecting
> > > prepare_count by a spinlock (probably enable_lock)) and call clk_prepare
> > > before calling clk->ops->enable?
> > That might result in a driver working on some platforms(those have
> > atomic clk_prepare)
> > and not on others(those have sleeping).
> The first option has the same result.  E.g. on some platforms
> clk->ops->prepare might be NULL, on others it's not.

If clk->ops->prepare is NULL, then clk_prepare() better return success
as it should mean "no preparation necessary", not "someone didn't
implement it so its an error".

Calling clk->ops->enable() with a spinlock held will ensure that no one
tries to make that method sleep, so if people want sleeping stuff they
have to use the clk_prepare() stuff.  It's a self-enforcing API which
ensures that we don't get sleeping stuff inside clk_enable().

And with a check in clk_enable() for a preparation, it helps to ensure
(Continue reading)

Permalink | Reply | 4 comments |
headers
Uwe Kleine-König | 1 Feb 2011 16:18
Picon
Favicon
Gravatar

Re: Locking in the clk API, part 2: clk_prepare/clk_unprepare

On Tue, Feb 01, 2011 at 02:39:32PM +0000, Russell King - ARM Linux wrote:
> On Tue, Feb 01, 2011 at 03:18:37PM +0100, Uwe Kleine-König wrote:
> > On Tue, Feb 01, 2011 at 01:15:12PM +0000, Russell King - ARM Linux wrote:
> > > On Tue, Feb 01, 2011 at 11:54:49AM +0100, Uwe Kleine-König wrote:
> > > > Alternatively don't force the sleep in clk_prepare (e.g. by protecting
> > > > prepare_count by a spinlock (probably enable_lock)) and call clk_prepare
> > > > before calling clk->ops->enable?
> > > 
> > > That's a completely bad idea.  I assume you haven't thought about this
> > > very much.
> > Right, but I thought it a bit further than you did.  Like the following:
> >  
> > int clk_prepare(struct clk *clk)
> > {
> > 	int ret = 0, first;
> > 	unsigned long flags;
> > 
> > 	spin_lock_irqsave(&clk->enable_lock, flags);
> > 	if (clk->flags & CLK_BUSY) {
> > 		/* 
> > 		 * this must not happen, please serialize calls to
> > 		 * clk_prepare/clk_enable
> > 		 */
> 
> How do different drivers serialize calls to clk_prepare?  Are you
> really suggesting that we should have a global mutex somewhere to
> prevent this?
yeah, didn't thought about multiple consumers, so (as Jeremy suggested)
the right thing is to sleep until CLK_BUSY is cleared.
(Continue reading)

Permalink | Reply | 21 comments |
headers
Uwe Kleine-König | 1 Feb 2011 16:22
Picon
Favicon
Gravatar

Re: Locking in the clk API, part 2: clk_prepare/clk_unprepare

On Tue, Feb 01, 2011 at 03:14:18PM +0000, Russell King - ARM Linux wrote:
> On Tue, Feb 01, 2011 at 03:00:24PM +0100, Uwe Kleine-König wrote:
> > On Tue, Feb 01, 2011 at 10:05:56PM +0900, Jassi Brar wrote:
> > > 2011/2/1 Uwe Kleine-König <u.kleine-koenig <at> pengutronix.de>:
> > > 
> > > .....
> > > 
> > > > Do you plan to handle the case that clk_enable is called while prepare
> > > > isn't completed (considering the special case "not called at all")?
> > > > Maybe BUG_ON(clk->ops->prepare && !clk->prepare_count)?
> > > Sounds better than the second option.
> > > 
> > > > Alternatively don't force the sleep in clk_prepare (e.g. by protecting
> > > > prepare_count by a spinlock (probably enable_lock)) and call clk_prepare
> > > > before calling clk->ops->enable?
> > > That might result in a driver working on some platforms(those have
> > > atomic clk_prepare)
> > > and not on others(those have sleeping).
> > The first option has the same result.  E.g. on some platforms
> > clk->ops->prepare might be NULL, on others it's not.
> 
> If clk->ops->prepare is NULL, then clk_prepare() better return success
> as it should mean "no preparation necessary", not "someone didn't
> implement it so its an error".
> 
> Calling clk->ops->enable() with a spinlock held will ensure that no one
> tries to make that method sleep, so if people want sleeping stuff they
> have to use the clk_prepare() stuff.  It's a self-enforcing API which
> ensures that we don't get sleeping stuff inside clk_enable().
>
(Continue reading)

Permalink | Reply | 3 comments |

Gmane