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

Hi all,

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.

For drivers, this means that clk_prepare must be called (and have returned) 
before calling clk_enable.

== Enable/Prepare counts ==

I intend to do the enable and prepare "counting" in the core clock API, 
meaning that that the clk_ops callbacks will only invoked on the first 
prepare/enable and the last unprepare/disable.

== Concurrency ==

Splitting the prepare and enable stages introduces the concurrency 
requirements:

1) clk_enable must not return before the clock is outputting a valid clock 
   signal.

2) clk_prepare must not return before the clock is fully prepared (ie, it is 
   safe to call clk_enable).

It is not possible for clk_enable to wait for the clock to be prepared, 
because that would require synchronisation with clk_prepare, which may then 
require blocking. Therefore:

3) The clock consumer *must* respect the proper ordering of clk_prepare and 
   clk_enable. For example, drivers that call clk_enable during an interrupt 
   must ensure that the interrupt handler will not be invoked until 
   clk_prepare has returned.

== Other considerations ==

The time that a clock spends "prepared" is a superset of the the time that a 
clock spends "enabled". Therefore, clocks that are switched on during 
clk_prepare (ie, non-atomic clocks) will be running for a larger amount of 
time. In some cases, this can be mitigated by moving some of the final 
(atomic) switching functionality to the clk_enable function.

== Implementation ==

Basically:

struct clk {
	const struct clk_ops *ops
	int                  enable_count;
	spinlock_t           enable_lock;
	int                  prepare_count;
	struct mutex         prepare_lock;
};

int clk_enable(struct clk *clk)
{
	int ret = 0;

	spin_lock(&clk->enable_lock);
	if (!clk->enable_count)
		ret = clk->ops->enable(clk);

	if (!ret)
		clk->enable_count++;
	spin_unlock(&clk->enable_lock);

	return ret;
}

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

	mutex_lock(&clk->prepare_lock);
	if (!clk->prepare_count)
		ret = clk->ops->prepare(clk);

	if (!ret)
		clk->prepare_count++;
	mutex_unlock(&clk->prepare_lock);

	return ret;
}

-----

Comments welcome, code coming soon.

Cheers,


Jeremy

Hello Jeremy,

On Tue, Feb 01, 2011 at 05:11:29PM +0800, Jeremy Kerr wrote:

quoted

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.

For drivers, this means that clk_prepare must be called (and have returned) 
before calling clk_enable.

== Enable/Prepare counts ==

I intend to do the enable and prepare "counting" in the core clock API, 
meaning that that the clk_ops callbacks will only invoked on the first 
prepare/enable and the last unprepare/disable.

== Concurrency ==

Splitting the prepare and enable stages introduces the concurrency 
requirements:

1) clk_enable must not return before the clock is outputting a valid clock 
   signal.

2) clk_prepare must not return before the clock is fully prepared (ie, it is 
   safe to call clk_enable).

It is not possible for clk_enable to wait for the clock to be prepared, 
because that would require synchronisation with clk_prepare, which may then 
require blocking. Therefore:

3) The clock consumer *must* respect the proper ordering of clk_prepare and 
   clk_enable. For example, drivers that call clk_enable during an interrupt 
   must ensure that the interrupt handler will not be invoked until 
   clk_prepare has returned.

== Other considerations ==

The time that a clock spends "prepared" is a superset of the the time that a 
clock spends "enabled". Therefore, clocks that are switched on during 
clk_prepare (ie, non-atomic clocks) will be running for a larger amount of 
time. In some cases, this can be mitigated by moving some of the final 
(atomic) switching functionality to the clk_enable function.

== Implementation ==

Basically:

struct clk {
	const struct clk_ops *ops
	int                  enable_count;
	spinlock_t           enable_lock;
	int                  prepare_count;
	struct mutex         prepare_lock;
};

int clk_enable(struct clk *clk)
{
	int ret = 0;

	spin_lock(&clk->enable_lock);
	if (!clk->enable_count)
		ret = clk->ops->enable(clk);

	if (!ret)
		clk->enable_count++;
	spin_unlock(&clk->enable_lock);

	return ret;
}

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

	mutex_lock(&clk->prepare_lock);
	if (!clk->prepare_count)
		ret = clk->ops->prepare(clk);

	if (!ret)
		clk->prepare_count++;
	mutex_unlock(&clk->prepare_lock);

	return ret;
}

-----

Comments welcome, code coming soon.

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)?

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?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

2011/2/1 Uwe Kleine-K?nig [off-list ref]:

.....

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!

On Tue, Feb 01, 2011 at 10:05:56PM +0900, Jassi Brar wrote:

2011/2/1 Uwe Kleine-K?nig [off-list ref]:

.....

quoted

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.

quoted

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/  |

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:

quoted

2011/2/1 Uwe Kleine-K?nig [off-list ref]:

.....

quoted

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.

quoted

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
that drivers do call clk_prepare() before clk_enable() - though it can't
guarantee it in every case.

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:

quoted

On Tue, Feb 01, 2011 at 10:05:56PM +0900, Jassi Brar wrote:

quoted

2011/2/1 Uwe Kleine-K?nig [off-list ref]:

.....

quoted

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.

quoted

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
that drivers do call clk_prepare() before clk_enable() - though it can't
guarantee it in every case.

Full ack.  (I wonder if you misunderstood me or wanted to put my
statement into more words.  Jassi didn't like that a clk_enable without
a previous clk_prepare worked on some platforms and on others it
doesn't.  With BUG_ON(clk->ops->prepare && !clk->prepare_count) in
clk_enable we have exactly this situation.)

Best regards
Uwe


-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

On Tue, Feb 01, 2011 at 04:22:03PM +0100, Uwe Kleine-K?nig wrote:

Full ack.  (I wonder if you misunderstood me or wanted to put my
statement into more words.  Jassi didn't like that a clk_enable without
a previous clk_prepare worked on some platforms and on others it
doesn't.  With BUG_ON(clk->ops->prepare && !clk->prepare_count) in
clk_enable we have exactly this situation.)

Even with a NULL clk->ops->prepare function, we still want drivers to
have called clk_prepare().  So we can do something like:

	if (WARN_ON(clk->prepare_count == 0))
		return -EINVAL;

in clk_enable() should be sufficient and noisy enough not to be missed.

I'd avoid BUG_ON() here as that will take the system down, which may
increase the chances of getting useful bug reports.

On 02/01/2011 07:28 AM, Russell King - ARM Linux wrote:

On Tue, Feb 01, 2011 at 04:22:03PM +0100, Uwe Kleine-K?nig wrote:

quoted

Full ack.  (I wonder if you misunderstood me or wanted to put my
statement into more words.  Jassi didn't like that a clk_enable without
a previous clk_prepare worked on some platforms and on others it
doesn't.  With BUG_ON(clk->ops->prepare&&  !clk->prepare_count) in
clk_enable we have exactly this situation.)

Even with a NULL clk->ops->prepare function, we still want drivers to
have called clk_prepare().  So we can do something like:

	if (WARN_ON(clk->prepare_count == 0))
		return -EINVAL;

in clk_enable() should be sufficient and noisy enough not to be missed.

This code will only catch the error when it actually happens and will 
even miss catching some of them (if timed right -- unprepare happens in 
the other core after this check is executed).

I really wish there was something better we could do to help driver devs 
catch errors of calling enable without calling prepare(). Some thing 
like spin lock debug, or the might_sleeps() inside mutexes, etc.

Hmm... Jeremy, how about doing a similar check in the unprepare code? 
You could WARN/BUG ON the prepare count going to zero when the enable 
count is still non-zero?

Thanks,
Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

2011/2/2 Russell King - ARM Linux [off-list ref]:

On Tue, Feb 01, 2011 at 04:22:03PM +0100, Uwe Kleine-K?nig wrote:

quoted

Full ack. ?(I wonder if you misunderstood me or wanted to put my
statement into more words. ?Jassi didn't like that a clk_enable without
a previous clk_prepare worked on some platforms and on others it
doesn't. ?With BUG_ON(clk->ops->prepare && !clk->prepare_count) in
clk_enable we have exactly this situation.)

Even with a NULL clk->ops->prepare function, we still want drivers to
have called clk_prepare(). ?So we can do something like:

? ? ? ?if (WARN_ON(clk->prepare_count == 0))
? ? ? ? ? ? ? ?return -EINVAL;

in clk_enable() should be sufficient and noisy enough not to be missed.

I'd avoid BUG_ON() here as that will take the system down, which may
increase the chances of getting useful bug reports.

Having thought about it, I think it's not necessary to immediately catch
drivers that work on some platforms and not on others -- a mere comment
'please add clk_prepare' during code review or a patch adding 'clk_prepare'
later upon stumbling across a platform on which the driver doesn't work,
should be OK. Let us not fret about it.

That leaves us with only having to ensure that :-
a) No two calls to clk_prepare/unprepare _hooks_ are consecutive.
b) clk_prepare is done on the clock (not necessarily by the driver
    under consideration) before calls to clk_enable.

I think (a) is already easily managed by having the prepare_count,
and (b) can be reasonably managed by what Russell suggests above.

So, FWIW, I am for the idea.

Njoi!

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:

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

	might_sleep();

	spin_lock_irqsave(clk->enable_lock, flags);
	first = clk->prepare_count++ == 0;
	spin_unlock_irqrestore(clk->enable_clock, flags);

	if (first)
		ret = clk->ops->prepare(clk);

	return ret;
}

The problem with this is that you now don't have any sane locking on
the prepare callback, and the circumstances under which it's called
are very indefinite.  For example, consider a preempt-enabled system:

	thread 1		thread 2		prepare_count
	clk_prepare					0
	clk->prepare_count++				1
	<thread switch>
				clk_prepare		1
				clk->prepare_count++	2
				clk_prepare returns	2

				clk_enable		2
				<explodes as clock is not prepared>
				<thread switch>
	clk->ops->prepare(clk)

So really, what you're suggesting is completely broken.

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:

quoted

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)
		return 0;

	if (clk->ops->prepare) {
		might_sleep();
		ret = clk->ops->prepare(clk);
	}

	spin_lock_irqsave(&clk->enable_lock, flags);
	clk->flags &= ~CLK_BUSY;
	if (ret)
		clk->prepare_count--;
out_unlock:
	spin_unlock_irqrestore(&clk->enable_lock, flags);

	return ret;
}

If you now find a problem with that you can blame me not having thought
it to an end.

And note, this is only a suggestion.  I.e. I don't know what is the best
to do in the case where I implemented returning -EBUSY above.  BUG?
Wait for CLK_BUSY to be cleared?

I'm not sure I like "clk_prepare sleeps iff unprepared but preparable".
Still I think the approach is worth to be discussed.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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:

quoted

On Tue, Feb 01, 2011 at 11:54:49AM +0100, Uwe Kleine-K?nig wrote:

quoted

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;
	}
	first = clk->prepare_count++ == 0;
	if (first)
		clk->flags |= CLK_BUSY;
	spin_unlock_irqrestore(&clk->enable_lock, flags);

	if (!first)
		return 0;

	if (clk->ops->prepare) {
		might_sleep();
		ret = clk->ops->prepare(clk);
	}

	spin_lock_irqsave(&clk->enable_lock, flags);
	clk->flags &= ~CLK_BUSY;
	if (ret)
		clk->prepare_count--;
out_unlock:
	spin_unlock_irqrestore(&clk->enable_lock, flags);

	return ret;
}

If you now find a problem with that you can blame me not having thought
it to an end.

And note, this is only a suggestion.  I.e. I don't know what is the best
to do in the case where I implemented returning -EBUSY above.  BUG?
Wait for CLK_BUSY to be cleared?

So what're you proposing that a driver writer should do when he sees
-EBUSY returned from this function?  Abandon the probe() returning -EBUSY
and hope the user retries later?  Or maybe:

	do {
		err = clk_prepare(clk);
	} while (err == -EBUSY);

?

I don't think that's reasonable to offload this onto driver writers, who
already have a big enough problem already.  The less complexity that
driver writers have to deal with, the better.

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:

quoted

On Tue, Feb 01, 2011 at 01:15:12PM +0000, Russell King - ARM Linux wrote:

quoted

On Tue, Feb 01, 2011 at 11:54:49AM +0100, Uwe Kleine-K?nig wrote:

quoted

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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

On Tue, Feb 01, 2011 at 04:18:46PM +0100, Uwe Kleine-K?nig wrote:

yeah, didn't thought about multiple consumers, so (as Jeremy suggested)
the right thing is to sleep until CLK_BUSY is cleared.

A simpler way to write this is:

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

	mutex_lock(&clk->mutex);
	if (clk->prepared == 0)
		ret = clk->ops->prepare(clk);
	if (ret == 0)
		clk->prepared++;
	mutex_unlock(&clk->mutex);

	return ret;
}

I think we want to take a common mutex not only for clk_prepare(), but
also for clk_set_rate().  If prepare() is waiting for a PLL to lock,
we don't want a set_rate() interfering with that.

I'd also be tempted at this stage to build-in a no-op dummy clock,
that being the NULL clk:

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

	if (clk) {
		mutex_lock(&clk->mutex);
		if (clk->prepared == 0)
			ret = clk->ops->prepare(clk);
		if (ret == 0)
			clk->prepared++;
		mutex_unlock(&clk->mutex);
	}

	return ret;
}

as we have various platforms defining a dummy struct clk as a way of
satisfying various driver requirements.  These dummy clocks are exactly
that - they're complete no-ops.

On Tue, Feb 01, 2011 at 03:24:58PM +0000, Russell King - ARM Linux wrote:

On Tue, Feb 01, 2011 at 04:18:46PM +0100, Uwe Kleine-K?nig wrote:

quoted

yeah, didn't thought about multiple consumers, so (as Jeremy suggested)
the right thing is to sleep until CLK_BUSY is cleared.

A simpler way to write this is:

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

	mutex_lock(&clk->mutex);
	if (clk->prepared == 0)
		ret = clk->ops->prepare(clk);
	if (ret == 0)
		clk->prepared++;
	mutex_unlock(&clk->mutex);

	return ret;
}

But you cannot call this in atomic context when you know the clock is
already prepared.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

On Tue, Feb 01, 2011 at 04:53:44PM +0100, Uwe Kleine-K?nig wrote:

On Tue, Feb 01, 2011 at 03:24:58PM +0000, Russell King - ARM Linux wrote:

quoted

On Tue, Feb 01, 2011 at 04:18:46PM +0100, Uwe Kleine-K?nig wrote:

quoted

yeah, didn't thought about multiple consumers, so (as Jeremy suggested)
the right thing is to sleep until CLK_BUSY is cleared.

A simpler way to write this is:

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

	mutex_lock(&clk->mutex);
	if (clk->prepared == 0)
		ret = clk->ops->prepare(clk);
	if (ret == 0)
		clk->prepared++;
	mutex_unlock(&clk->mutex);

	return ret;
}

But you cannot call this in atomic context when you know the clock is
already prepared.

So?  You're not _supposed_ to call it from any atomic context ever.

On Tue, Feb 01, 2011 at 05:06:37PM +0000, Russell King - ARM Linux wrote:

On Tue, Feb 01, 2011 at 04:53:44PM +0100, Uwe Kleine-K?nig wrote:

quoted

On Tue, Feb 01, 2011 at 03:24:58PM +0000, Russell King - ARM Linux wrote:

quoted

On Tue, Feb 01, 2011 at 04:18:46PM +0100, Uwe Kleine-K?nig wrote:

quoted

yeah, didn't thought about multiple consumers, so (as Jeremy suggested)
the right thing is to sleep until CLK_BUSY is cleared.

A simpler way to write this is:

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

	mutex_lock(&clk->mutex);
	if (clk->prepared == 0)
		ret = clk->ops->prepare(clk);
	if (ret == 0)
		clk->prepared++;
	mutex_unlock(&clk->mutex);

	return ret;
}

But you cannot call this in atomic context when you know the clock is
already prepared.

So?  You're not _supposed_ to call it from any atomic context ever.

My motivation for a more complicated clk_prepare was to make clk_prepare
atomic when that's possible (i.e. when the clk is already prepared) and
call it before the enable callback in clk_enable.  Then everything
behaves nicely even if clk_enable is called from atomic context provided
that the clock was prepared before (or doesn't need to).

If a driver writer doesn't know that a certain clock might need to sleep
at some point he runs into an atomic might_sleep with your approach and
with mine.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

On Tue, Feb 01, 2011 at 08:32:01PM +0100, Uwe Kleine-K?nig wrote:

On Tue, Feb 01, 2011 at 05:06:37PM +0000, Russell King - ARM Linux wrote:

quoted

So?  You're not _supposed_ to call it from any atomic context ever.

My motivation for a more complicated clk_prepare was to make clk_prepare
atomic when that's possible (i.e. when the clk is already prepared) and
call it before the enable callback in clk_enable.  Then everything
behaves nicely even if clk_enable is called from atomic context provided
that the clock was prepared before (or doesn't need to).

You really don't get the point of clk_prepare() do you.  I'm not
going to bother trying to educate you anymore.

Hopefully someone with more patience can give you the necessary
teaching to make you understand.

On 02/01/2011 11:56 AM, Russell King - ARM Linux wrote:

On Tue, Feb 01, 2011 at 08:32:01PM +0100, Uwe Kleine-K?nig wrote:

quoted

On Tue, Feb 01, 2011 at 05:06:37PM +0000, Russell King - ARM Linux wrote:

quoted

So?  You're not _supposed_ to call it from any atomic context ever.

My motivation for a more complicated clk_prepare was to make clk_prepare
atomic when that's possible (i.e. when the clk is already prepared) and
call it before the enable callback in clk_enable.  Then everything
behaves nicely even if clk_enable is called from atomic context provided
that the clock was prepared before (or doesn't need to).

You really don't get the point of clk_prepare() do you.  I'm not
going to bother trying to educate you anymore.

Hopefully someone with more patience can give you the necessary
teaching to make you understand.

Uwe,

If the driver is calling clk_prepare() right next to clk_enable() 
knowing it's been already prepared and will hence be "atomic" (this is 
actually not true), then by your description, it's pointless to call 
clk_prepare().

If you want the driver to call clk_prepare() in atomic context because 
it will be atomic in most cases -- well, that's wrong. It's either 
atomic or is NOT atomic. There is no in between. If a call is NOT 
atomic, it can't be called in atomic context. Long story short, if you 
expect clk_prepare() to be atomic under any circumstance, it beats the 
point of introducing clk_prepare().

Hope I helped.

-Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Hello,

On Tue, Feb 01, 2011 at 12:21:45PM -0800, Saravana Kannan wrote:

If the driver is calling clk_prepare() right next to clk_enable()
knowing it's been already prepared and will hence be "atomic" (this
is actually not true), then by your description, it's pointless to
call clk_prepare().

Well not completely, as it increases the reference count.  The advantage
would be that clk_enable counts addionally as prepare, so it would be
impossible to unprepare an enabled clock.  And the other way round an
unprepared clock would never be enabled.
 

If you want the driver to call clk_prepare() in atomic context
because it will be atomic in most cases -- well, that's wrong. It's
either atomic or is NOT atomic. There is no in between. If a call is
NOT atomic, it can't be called in atomic context. Long story short,
if you expect clk_prepare() to be atomic under any circumstance, it
beats the point of introducing clk_prepare().

Well, with my suggestion it's atomic when certain precondions are given.
IMHO that's better than "atomic in most cases" because the caller can
assert that everything goes smooth.
These preconditions are asserted when the driver writer is careful
enough to stick to the API.

Either my idea is bad or I'm unable to sell it appropriately.  Be it as
it is, I will stop to make a case for it.

Best regards and thanks for your try,
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

On Tue, Feb 01, 2011 at 09:43:31PM +0100, Uwe Kleine-K?nig wrote:

Hello,

On Tue, Feb 01, 2011 at 12:21:45PM -0800, Saravana Kannan wrote:

quoted

If the driver is calling clk_prepare() right next to clk_enable()
knowing it's been already prepared and will hence be "atomic" (this
is actually not true), then by your description, it's pointless to
call clk_prepare().

Well not completely, as it increases the reference count.  The advantage
would be that clk_enable counts addionally as prepare, so it would be
impossible to unprepare an enabled clock.  And the other way round an
unprepared clock would never be enabled.
 

quoted

If you want the driver to call clk_prepare() in atomic context
because it will be atomic in most cases -- well, that's wrong. It's
either atomic or is NOT atomic. There is no in between. If a call is
NOT atomic, it can't be called in atomic context. Long story short,
if you expect clk_prepare() to be atomic under any circumstance, it
beats the point of introducing clk_prepare().

Well, with my suggestion it's atomic when certain precondions are given.
IMHO that's better than "atomic in most cases" because the caller can
assert that everything goes smooth.
These preconditions are asserted when the driver writer is careful
enough to stick to the API.

IMHO, clk_prepare is always called in non-atomic context, so it doesn't matter
whether it's really atomic or not. We don't have to make it as atomic as
possible.

Thanks
Richard

Either my idea is bad or I'm unable to sell it appropriately.  Be it as
it is, I will stop to make a case for it.

Best regards and thanks for your try,
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, 1 Feb 2011, Uwe Kleine-K?nig wrote:

My motivation for a more complicated clk_prepare was to make clk_prepare
atomic when that's possible (i.e. when the clk is already prepared) and
call it before the enable callback in clk_enable.  Then everything
behaves nicely even if clk_enable is called from atomic context provided
that the clock was prepared before (or doesn't need to).

NOOOOOOOOO!!!

We _do_ want drivers to _always_ call clk_prepare() in sleepable 
context, and _then_ always call clk_enable() in whatever context they 
wish.  Period.


Nicolas

On 02/01/2011 07:24 AM, Russell King - ARM Linux wrote:

A simpler way to write this is:

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

	mutex_lock(&clk->mutex);
	if (clk->prepared == 0)
		ret = clk->ops->prepare(clk);
	if (ret == 0)
		clk->prepared++;
	mutex_unlock(&clk->mutex);

	return ret;
}

I think we want to take a common mutex not only for clk_prepare(), but
also for clk_set_rate().  If prepare() is waiting for a PLL to lock,
we don't want a set_rate() interfering with that.

Looks like this is the best acknowledgment/response I can expect to get 
from Russell on this point that I raised.

Jeremy,

When you update the comments/doc to indicate clk_prepare/unprepare is 
not atomic, can you also update the comment for set_rate() and mark it 
as non-atomic?

Thanks for starting this thread. My efforts to reignite the other thread 
didn't go anywhere. Glad to see it's moving forward.

I'd also be tempted at this stage to build-in a no-op dummy clock,
that being the NULL clk:
[snip]
as we have various platforms defining a dummy struct clk as a way of
satisfying various driver requirements.  These dummy clocks are exactly
that - they're complete no-ops.

Unrelated to this thread, but I Ack this request too.

-Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

On Tue, Feb 01, 2011 at 12:33:12PM -0800, Saravana Kannan wrote:

Looks like this is the best acknowledgment/response I can expect to get  
from Russell on this point that I raised.

Sorry, I've been up to my eyeballs with other stuff over the last few
weeks.  Yes, I think clk_set_rate() needs to be sleep-able too.

On 02/01/2011 07:24 AM, Russell King - ARM Linux wrote:

I'd also be tempted at this stage to build-in a no-op dummy clock,
that being the NULL clk:

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

	if (clk) {
		mutex_lock(&clk->mutex);
		if (clk->prepared == 0)
			ret = clk->ops->prepare(clk);
		if (ret == 0)
			clk->prepared++;
		mutex_unlock(&clk->mutex);
	}

	return ret;
}

I'm afraid this will hide enable/disable imbalances on some targets and
then expose them on others. Maybe its not a big problem though since
this also elegantly handles the root(s) of the tree.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

On Tue, Feb 01, 2011 at 12:59:11PM -0800, Stephen Boyd wrote:

On 02/01/2011 07:24 AM, Russell King - ARM Linux wrote:

quoted

I'd also be tempted at this stage to build-in a no-op dummy clock,
that being the NULL clk:

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

	if (clk) {
		mutex_lock(&clk->mutex);
		if (clk->prepared == 0)
			ret = clk->ops->prepare(clk);
		if (ret == 0)
			clk->prepared++;
		mutex_unlock(&clk->mutex);
	}

	return ret;
}

I'm afraid this will hide enable/disable imbalances on some targets and
then expose them on others. Maybe its not a big problem though since
this also elegantly handles the root(s) of the tree.

You can't catch enable/disable imbalances in the prepare code, and you
can't really catch them in the unprepare code either.

Consider two drivers sharing the same struct clk.  When the second driver
prepares the clock, the enable count could well be non-zero, caused by
the first driver.  Ditto for when the second driver is removed, and it
calls unprepare - the enable count may well be non-zero.

The only thing you can check is that when the prepare count is zero,
the enable count is also zero.  You can also check in clk_enable() and
clk_disable() that the prepare count is non-zero.

If you want tigher checking than that, you need to somehow identify and
match up the clk_prepare/clk_enable/clk_disable/clk_unprepare calls from
a particular driver instance.  Addresses of the functions don't work as
you can't be certain that driver code will be co-located within a certain
range.  Adding an additional argument to these functions which is driver
instance specific seems to be horrible too.

On Tue, Feb 01, 2011 at 09:24:09PM +0000, Russell King - ARM Linux wrote:

On Tue, Feb 01, 2011 at 12:59:11PM -0800, Stephen Boyd wrote:

quoted

On 02/01/2011 07:24 AM, Russell King - ARM Linux wrote:

quoted

I'd also be tempted at this stage to build-in a no-op dummy clock,
that being the NULL clk:

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

	if (clk) {
		mutex_lock(&clk->mutex);
		if (clk->prepared == 0)
			ret = clk->ops->prepare(clk);
		if (ret == 0)
			clk->prepared++;
		mutex_unlock(&clk->mutex);
	}

	return ret;
}

I'm afraid this will hide enable/disable imbalances on some targets and
then expose them on others. Maybe its not a big problem though since
this also elegantly handles the root(s) of the tree.

You can't catch enable/disable imbalances in the prepare code, and you
can't really catch them in the unprepare code either.

Consider two drivers sharing the same struct clk.  When the second driver
prepares the clock, the enable count could well be non-zero, caused by
the first driver.  Ditto for when the second driver is removed, and it
calls unprepare - the enable count may well be non-zero.

The only thing you can check is that when the prepare count is zero,
the enable count is also zero.  You can also check in clk_enable() and
clk_disable() that the prepare count is non-zero.

but how can we check prepare count without mutex lock? Even if prepare count
is atomic_t, it can not guarantee the clock is actually prepared or unprepared.
So it's important for driver writer to maintain the call sequence.

Thanks
Richard

If you want tigher checking than that, you need to somehow identify and
match up the clk_prepare/clk_enable/clk_disable/clk_unprepare calls from
a particular driver instance.  Addresses of the functions don't work as
you can't be certain that driver code will be co-located within a certain
range.  Adding an additional argument to these functions which is driver
instance specific seems to be horrible too.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hello Richard,

On Fri, Feb 04, 2011 at 05:54:24PM +0800, Richard Zhao wrote:

On Tue, Feb 01, 2011 at 09:24:09PM +0000, Russell King - ARM Linux wrote:

quoted

On Tue, Feb 01, 2011 at 12:59:11PM -0800, Stephen Boyd wrote:

quoted

On 02/01/2011 07:24 AM, Russell King - ARM Linux wrote:

quoted

I'd also be tempted at this stage to build-in a no-op dummy clock,
that being the NULL clk:

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

	if (clk) {
		mutex_lock(&clk->mutex);
		if (clk->prepared == 0)
			ret = clk->ops->prepare(clk);
		if (ret == 0)
			clk->prepared++;
		mutex_unlock(&clk->mutex);
	}

	return ret;
}

I'm afraid this will hide enable/disable imbalances on some targets and
then expose them on others. Maybe its not a big problem though since
this also elegantly handles the root(s) of the tree.

You can't catch enable/disable imbalances in the prepare code, and you
can't really catch them in the unprepare code either.

Consider two drivers sharing the same struct clk.  When the second driver
prepares the clock, the enable count could well be non-zero, caused by
the first driver.  Ditto for when the second driver is removed, and it
calls unprepare - the enable count may well be non-zero.

The only thing you can check is that when the prepare count is zero,
the enable count is also zero.  You can also check in clk_enable() and
clk_disable() that the prepare count is non-zero.

but how can we check prepare count without mutex lock? Even if prepare count
is atomic_t, it can not guarantee the clock is actually prepared or unprepared.
So it's important for driver writer to maintain the call sequence.

I happily point out that the prepare_count needs to be protected by a
spinlock and you need a flag that signals a prepare or unprepare is
currently running.

SCNR
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

On Fri, Feb 04, 2011 at 11:21:20AM +0100, Uwe Kleine-K?nig wrote:

I happily point out that the prepare_count needs to be protected by a
spinlock and you need a flag that signals a prepare or unprepare is
currently running.

It's really simple.  You don't use a struct clk pointer in any way until
you've called a clk_get() to get a pointer.  So what's the problem with
ensuring that you do clk_prepare() on it before you register whatever
services may end up calling clk_enable().

That is good practice.  It's precisely the same practice which says that
you shall not register device drivers with subsystems, thereby making
them visible, until you're absolutely ready in the driver to start taking
requests to use your driver.  Precisely the same thing applies here.

In other words, to go back to the UART console driver case, in the
UART console setup function, you do this:

	clk = clk_get(...);
	if (IS_ERR(clk))
		return PTR_ERR(clk);

	err = clk_prepare(clk);
	if (err) {
		clk_put(clk);
		return err;
	}

	rate = clk_get_rate(clk);
	... setup UART, setup baud rate according to rate ...

	return 0;

So, this means that clk_enable() in the console write function will not
be called until after the initialization function has finished - by which
time clk_prepare() will have completed.

There is no need for any kind of spinlocking, atomic types or other such
crap for the prepare count.  We do not care about concurrent clk_enables().

The only time you'd need such games as you're suggesting is if you're still
promoting your idea about calling clk_prepare() from clk_enable() "in case
driver writers forget it", which is soo broken it's untrue.

On Fri, Feb 04, 2011 at 05:54:24PM +0800, Richard Zhao wrote:

On Tue, Feb 01, 2011 at 09:24:09PM +0000, Russell King - ARM Linux wrote:

quoted

You can't catch enable/disable imbalances in the prepare code, and you
can't really catch them in the unprepare code either.

Consider two drivers sharing the same struct clk.  When the second driver
prepares the clock, the enable count could well be non-zero, caused by
the first driver.  Ditto for when the second driver is removed, and it
calls unprepare - the enable count may well be non-zero.

The only thing you can check is that when the prepare count is zero,
the enable count is also zero.  You can also check in clk_enable() and
clk_disable() that the prepare count is non-zero.

but how can we check prepare count without mutex lock? Even if prepare count
is atomic_t, it can not guarantee the clock is actually prepared or unprepared.
So it's important for driver writer to maintain the call sequence.

Forget atomic_t - it's the most abused type in the kernel.  Just because
something says its atomic doesn't make it so.  In a use like this,
atomic_t just buys you additional needless complexity with no benefit.

Of course we can check the prepared count.  What we can't do is check
that it doesn't change concurrently - but that's something we can't do
anyway.

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

	if (clk) {
		if (WARN_ON(!clk->prepare_count))
			return -EINVAL;

		spin_lock_irqsave(&clk->lock, flags);
		if (clk->enable_count++ == 0)
			ret = clk->ops->enable(clk);
		spin_unlock_irqrestore(&clk->lock, flags);
	}
	return ret;
}

is entirely sufficient to catch the case of a single-use clock not being
prepared before clk_enable() is called.

We're after detecting drivers missing calls to clk_prepare(), we're not
after detecting concurrent calls to clk_prepare()/clk_unprepare().

On Fri, Feb 4, 2011 at 7:48 PM, Russell King - ARM Linux
[off-list ref] wrote:

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

? ? ? ?if (clk) {
? ? ? ? ? ? ? ?if (WARN_ON(!clk->prepare_count))
? ? ? ? ? ? ? ? ? ? ? ?return -EINVAL;

? ? ? ? ? ? ? ?spin_lock_irqsave(&clk->lock, flags);
? ? ? ? ? ? ? ?if (clk->enable_count++ == 0)
? ? ? ? ? ? ? ? ? ? ? ?ret = clk->ops->enable(clk);
? ? ? ? ? ? ? ?spin_unlock_irqrestore(&clk->lock, flags);
? ? ? ?}
? ? ? ?return ret;
}

is entirely sufficient to catch the case of a single-use clock not being
prepared before clk_enable() is called.

We're after detecting drivers missing calls to clk_prepare(), we're not
after detecting concurrent calls to clk_prepare()/clk_unprepare().

I hope you mean 'making sure the clock is prepared before it's enabled
' rather than
'catching a driver that doesn't do clk_prepare before clk_enable'.
Because, the above implementation still doesn't catch a driver that
doesn't call clk_prepare
but simply uses a clock that happens to have been already prepare'd by
some other
driver or the platform.

On Fri, Feb 04, 2011 at 08:04:03PM +0900, Jassi Brar wrote:

On Fri, Feb 4, 2011 at 7:48 PM, Russell King - ARM Linux
[off-list ref] wrote:

quoted

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

? ? ? ?if (clk) {
? ? ? ? ? ? ? ?if (WARN_ON(!clk->prepare_count))
? ? ? ? ? ? ? ? ? ? ? ?return -EINVAL;

? ? ? ? ? ? ? ?spin_lock_irqsave(&clk->lock, flags);
? ? ? ? ? ? ? ?if (clk->enable_count++ == 0)
? ? ? ? ? ? ? ? ? ? ? ?ret = clk->ops->enable(clk);
? ? ? ? ? ? ? ?spin_unlock_irqrestore(&clk->lock, flags);
? ? ? ?}
? ? ? ?return ret;
}

is entirely sufficient to catch the case of a single-use clock not being
prepared before clk_enable() is called.

We're after detecting drivers missing calls to clk_prepare(), we're not
after detecting concurrent calls to clk_prepare()/clk_unprepare().

I hope you mean 'making sure the clock is prepared before it's enabled
' rather than
'catching a driver that doesn't do clk_prepare before clk_enable'.
Because, the above implementation still doesn't catch a driver that
doesn't call clk_prepare
but simply uses a clock that happens to have been already prepare'd by
some other
driver or the platform.

No, I mean what I said.

The only way to do what you're asking is to attach a list of identifiers
which have prepared a clock to the struct clk, where each identifier is
unique to each driver instance.

So what that becomes is:

struct prepared_instance {
	struct list_head node;
	void *driver_id;
};

int clk_prepare(struct clk *clk, void *driver_id)
{
	struct prepared_instance *inst;
	int ret = 0;

	if (clk) {
		inst = kmalloc(sizeof(*inst), GFP_KERNEL);
		if (!inst)
			return -ENOMEM;

		inst->driver_id = driver_id;

		mutex_lock(&clk->mutex);
		if (clk->prepare_count++ == 0)
			ret = clk->ops->prepare(clk);

		if (ret == 0) {
			spin_lock_irqsave(&clk->lock, flags);
			list_add(&inst->node, &clk->prepare_list);
			spin_unlock_irqrestore(&clk->lock, flags);
		} else
			clk->prepare_count--;
		mutex_unlock(&clk->mutex);
	}
	return ret;
}

int clk_enable(struct clk *clk, void *driver_id)
{
	unsigned long flags;
	int ret = 0;

	if (clk) {
		struct prepare_instance *inst;

		spin_lock_irqsave(&clk->lock, flags);
		list_for_each_entry(inst, &clk->prepare_list, node)
			if (inst == driver_id)
				ret = -EINVAL;

		if (ret == 0 && clk->enable_count++ == 0) {
			ret = clk->ops->enable(clk);
			if (ret)
				clk->enable_count--;
		}
		spin_unlock_irqrestore(&clk->lock, flags);
	}
	return ret;
}

I think that's going completely over the top, and adds needless complexity
to drivers, which now have to pass an instance specific cookie into every
clk API call.

On Fri, Feb 4, 2011 at 8:18 PM, Russell King - ARM Linux
[off-list ref] wrote:

On Fri, Feb 04, 2011 at 08:04:03PM +0900, Jassi Brar wrote:

quoted

On Fri, Feb 4, 2011 at 7:48 PM, Russell King - ARM Linux
[off-list ref] wrote:

quoted

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

? ? ? ?if (clk) {
? ? ? ? ? ? ? ?if (WARN_ON(!clk->prepare_count))
? ? ? ? ? ? ? ? ? ? ? ?return -EINVAL;

? ? ? ? ? ? ? ?spin_lock_irqsave(&clk->lock, flags);
? ? ? ? ? ? ? ?if (clk->enable_count++ == 0)
? ? ? ? ? ? ? ? ? ? ? ?ret = clk->ops->enable(clk);
? ? ? ? ? ? ? ?spin_unlock_irqrestore(&clk->lock, flags);
? ? ? ?}
? ? ? ?return ret;
}

is entirely sufficient to catch the case of a single-use clock not being
prepared before clk_enable() is called.

We're after detecting drivers missing calls to clk_prepare(), we're not
after detecting concurrent calls to clk_prepare()/clk_unprepare().

I hope you mean 'making sure the clock is prepared before it's enabled
' rather than
'catching a driver that doesn't do clk_prepare before clk_enable'.
Because, the above implementation still doesn't catch a driver that
doesn't call clk_prepare
but simply uses a clock that happens to have been already prepare'd by
some other
driver or the platform.

No, I mean what I said.

Then, how does that function catch a driver that, say, doesn't do clk_prepare
but share the clk with another already active driver?
Because you said - "We're after detecting drivers missing calls to
clk_prepare()"

The point is, there is difference between detecting drivers that miss
the clk_prepare
and ensuring clk_prepare has been called before any call to
clk_enable. And making
that clear helps get rid of lots of confusion/misunderstanding. Uwe
seems to have
had similar confusions.

The only way to do what you're asking is to attach a list of identifiers
which have prepared a clock to the struct clk, where each identifier is
unique to each driver instance.

I am not asking what you think.
In my second last post, I am rather asking the other way around - that
let us not worry
about drivers missing the clk_prepare and not try to catch those by the new API.

I think that's going completely over the top, and adds needless complexity
to drivers, which now have to pass an instance specific cookie into every
clk API call.

Exactly.
All we need is to ensure clk_prepare has been called atleast once before
any call to clk_enable.

On Fri, Feb 04, 2011 at 08:51:15PM +0900, Jassi Brar wrote:

On Fri, Feb 4, 2011 at 8:18 PM, Russell King - ARM Linux
[off-list ref] wrote:

quoted

On Fri, Feb 04, 2011 at 08:04:03PM +0900, Jassi Brar wrote:

quoted

On Fri, Feb 4, 2011 at 7:48 PM, Russell King - ARM Linux
[off-list ref] wrote:

quoted

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

? ? ? ?if (clk) {
? ? ? ? ? ? ? ?if (WARN_ON(!clk->prepare_count))
? ? ? ? ? ? ? ? ? ? ? ?return -EINVAL;

? ? ? ? ? ? ? ?spin_lock_irqsave(&clk->lock, flags);
? ? ? ? ? ? ? ?if (clk->enable_count++ == 0)
? ? ? ? ? ? ? ? ? ? ? ?ret = clk->ops->enable(clk);
? ? ? ? ? ? ? ?spin_unlock_irqrestore(&clk->lock, flags);
? ? ? ?}
? ? ? ?return ret;
}

is entirely sufficient to catch the case of a single-use clock not being
prepared before clk_enable() is called.

We're after detecting drivers missing calls to clk_prepare(), we're not
after detecting concurrent calls to clk_prepare()/clk_unprepare().

I hope you mean 'making sure the clock is prepared before it's enabled
' rather than
'catching a driver that doesn't do clk_prepare before clk_enable'.
Because, the above implementation still doesn't catch a driver that
doesn't call clk_prepare
but simply uses a clock that happens to have been already prepare'd by
some other
driver or the platform.

No, I mean what I said.

Then, how does that function catch a driver that, say, doesn't do clk_prepare
but share the clk with another already active driver?

As per the code I just supplied!

Because you said - "We're after detecting drivers missing calls to
clk_prepare()"

The point is, there is difference between detecting drivers that miss
the clk_prepare
and ensuring clk_prepare has been called before any call to
clk_enable. And making
that clear helps get rid of lots of confusion/misunderstanding. Uwe
seems to have
had similar confusions.

As I said on the 1st February.

quoted

The only way to do what you're asking is to attach a list of identifiers
which have prepared a clock to the struct clk, where each identifier is
unique to each driver instance.

I am not asking what you think.
In my second last post, I am rather asking the other way around - that
let us not worry
about drivers missing the clk_prepare and not try to catch those by the
new API.

No.  That means we have no way to flag a call to clk_enable on an
unprepared clock, and will lead to unexplained system lockups.  What
I've been suggesting all along is the "best efforts" approach.  I'm
sorry you can't see that, but that's really not my problem.

quoted

I think that's going completely over the top, and adds needless complexity
to drivers, which now have to pass an instance specific cookie into every
clk API call.

Exactly.
All we need is to ensure clk_prepare has been called atleast once before
any call to clk_enable.

I described this fully in my reply to Stephen Boyd on 1st February,
which is a parent to this sub-thread.

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);

	if (!first)
		return 0;

	if (clk->ops->prepare) {
		might_sleep();
		ret = clk->ops->prepare(clk);
	}

	spin_lock_irqsave(&clk->enable_lock, flags);
	clk->flags &= ~CLK_BUSY;
	if (ret)
		clk->prepare_count--;
out_unlock:
	spin_unlock_irqrestore(&clk->enable_lock, flags);

	return ret;
}

Cheers,


Jeremy

On Tue, Feb 01, 2011 at 05:11:29PM +0800, Jeremy Kerr wrote:

Hi all,

quoted

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.

For drivers, this means that clk_prepare must be called (and have returned) 
before calling clk_enable.

== Enable/Prepare counts ==

I intend to do the enable and prepare "counting" in the core clock API, 
meaning that that the clk_ops callbacks will only invoked on the first 
prepare/enable and the last unprepare/disable.

== Concurrency ==

Splitting the prepare and enable stages introduces the concurrency 
requirements:

1) clk_enable must not return before the clock is outputting a valid clock 
   signal.

2) clk_prepare must not return before the clock is fully prepared (ie, it is 
   safe to call clk_enable).

It is not possible for clk_enable to wait for the clock to be prepared, 
because that would require synchronisation with clk_prepare, which may then 
require blocking. Therefore:

3) The clock consumer *must* respect the proper ordering of clk_prepare and 
   clk_enable. For example, drivers that call clk_enable during an interrupt 
   must ensure that the interrupt handler will not be invoked until 
   clk_prepare has returned.

== Other considerations ==

The time that a clock spends "prepared" is a superset of the the time that a 
clock spends "enabled". Therefore, clocks that are switched on during 
clk_prepare (ie, non-atomic clocks) will be running for a larger amount of 
time. In some cases, this can be mitigated by moving some of the final 
(atomic) switching functionality to the clk_enable function.

== Implementation ==

Basically:

struct clk {
	const struct clk_ops *ops
	int                  enable_count;
	spinlock_t           enable_lock;
	int                  prepare_count;
	struct mutex         prepare_lock;
};

int clk_enable(struct clk *clk)
{
	int ret = 0;

	spin_lock(&clk->enable_lock);
	if (!clk->enable_count)
		ret = clk->ops->enable(clk);

	if (!ret)
		clk->enable_count++;
	spin_unlock(&clk->enable_lock);

	return ret;
}

Why do we not call parent's clk_enable in this function? For flexible? How many
different cases is causing us to move the effert to platform clock driver?

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

	mutex_lock(&clk->prepare_lock);
	if (!clk->prepare_count)
		ret = clk->ops->prepare(clk);

	if (!ret)
		clk->prepare_count++;
	mutex_unlock(&clk->prepare_lock);

	return ret;
}

Same as above.
And for most clocks, prepare/unprepare may be NULL. So in such case, is it
better to call parent's prepare and increase its own prepare_count here?

Thanks
Richard

-----

Comments welcome, code coming soon.

Cheers,


Jeremy


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Fri, Feb 04, 2011 at 08:45:34PM +0800, Richard Zhao wrote:

quoted

== Implementation ==

Basically:

struct clk {
	const struct clk_ops *ops
	int                  enable_count;
	spinlock_t           enable_lock;
	int                  prepare_count;
	struct mutex         prepare_lock;
};

int clk_enable(struct clk *clk)
{
	int ret = 0;

	spin_lock(&clk->enable_lock);
	if (!clk->enable_count)
		ret = clk->ops->enable(clk);

	if (!ret)
		clk->enable_count++;
	spin_unlock(&clk->enable_lock);

	return ret;
}

Why do we not call parent's clk_enable in this function? For flexible? How many
different cases is causing us to move the effert to platform clock driver?

You may notice that struct clk above doesn't have a parent.

Hi all,

These patches are an attempt to allow platforms to share clock code. At
present, the definitions of 'struct clk' are local to platform code,
which makes allocating and initialising cross-platform clock sources
difficult, and makes it impossible to compile a single image containing
support for two ARM platforms with different struct clks.

The three patches are for the architecture-independent kernel code,
introducing the common clk infrastructure. The changelog for the first
patch includes details about the new clock definitions.

Ben Herrenschmidt is looking at using common struct clk code for powerpc
too, hence the kernel-wide approach.

Many thanks to the following for their input:
 * Ben Dooks [off-list ref]
 * Baruch Siach [off-list ref]
 * Russell King [off-list ref]
 * Uwe Kleine-K?nig [off-list ref]
 * Lorenzo Pieralisi [off-list ref]
 * Vincent Guittot [off-list ref]
 * Sascha Hauer [off-list ref]

Cheers,


Jeremy

--
v11:
 * add prepare/unprepare for non-atomic switching, document atomicity
 * move to drivers/clk/

v10:
 * comment fixups, from Uwe's review
 * added DEFINE_CLK_FIXED

v9:
 * comment improvements
 * kerneldoc fixups
 * add WARN_ON to clk_disable

v8:
 * add atomic clocks, and locking wrappers
 * expand comments on clk and clk_ops

v7:
 * change CLK_INIT to initialise clk->mutex statically

v6:
 * fixed up references to 'clk_operations' in the changelog

v5:
 * uninline main API, and share definitions with !USE_COMMON_STRUCT_CLK
 * add __clk_get
 * delay mutex init
 * kerneldoc for struct clk

v4:
 * use mutex for enable/disable locking
 * DEFINE_CLK -> INIT_CLK, and pass the clk name for mutex init
 * struct clk_operations -> struct clk_ops

v3:
 * do clock usage refcounting in common code
 * provide sample port

v2:
 * no longer ARM-specific
 * use clk_operations

---
Jeremy Kerr (3):
      Add a common struct clk
      clk: Generic support for fixed-rate clocks
      clk: add warnings for incorrect enable/prepare semantics

We currently have ~21 definitions of struct clk in the ARM architecture,
each defined on a per-platform basis. This makes it difficult to define
platform- (or architecture-) independent clock sources without making
assumptions about struct clk, and impossible to compile two
platforms with different struct clks into a single image.

This change is an effort to unify struct clk where possible, by defining
a common struct clk, containing a set of clock operations. Different
clock implementations can set their own operations, and have a standard
interface for generic code. The callback interface is exposed to the
kernel proper, while the clock implementations only need to be seen by
the platform internals.

This allows us to share clock code among platforms, and makes it
possible to dynamically create clock devices in platform-independent
code.

Platforms can enable the generic struct clock through
CONFIG_USE_COMMON_STRUCT_CLK. In this case, the clock infrastructure
consists of a common struct clk:

struct clk {
	const struct clk_ops	*ops;
	unsigned int		enable_count;
	unsigned int		prepare_count;
	spinlock_t		enable_lock;
	struct mutex		prepare_lock;
};

And a set of clock operations (defined per type of clock):

struct clk_ops {
	int             (*enable)(struct clk *);
	void            (*disable)(struct clk *);
	unsigned long   (*get_rate)(struct clk *);
	[...]
};

To define a hardware-specific clock, machine code can "subclass" the
struct clock into a new struct (adding any device-specific data), and
provide a set of operations:

struct clk_foo {
	struct clk	clk;
	void __iomem	*some_register;
};

struct clk_ops clk_foo_ops = {
	.get_rate = clk_foo_get_rate,
};

The common clock definitions are based on a development patch from Ben
Herrenschmidt [off-list ref].

Signed-off-by: Jeremy Kerr <redacted>

---
 drivers/clk/Kconfig  |    3 
 drivers/clk/Makefile |    1 
 drivers/clk/clk.c    |  134 +++++++++++++++++++++++++++++++
 drivers/clk/clkdev.c |    5 +
 include/linux/clk.h  |  184 ++++++++++++++++++++++++++++++++++++++++---
 5 files changed, 318 insertions(+), 9 deletions(-)

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 4168c88..6e3ae54 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig

@@ -2,3 +2,6 @@
 config CLKDEV_LOOKUP
 	bool
 	select HAVE_CLK
+
+config USE_COMMON_STRUCT_CLK
+	bool

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 07613fa..a1a06d3 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile

@@ -1,2 +1,3 @@
 
 obj-$(CONFIG_CLKDEV_LOOKUP)	+= clkdev.o
+obj-$(CONFIG_USE_COMMON_STRUCT_CLK) += clk.o

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
new file mode 100644
index 0000000..12e0daf
--- /dev/null
+++ b/drivers/clk/clk.c

@@ -0,0 +1,134 @@
+/*
+ * Copyright (C) 2010-2011 Canonical Ltd <jeremy.kerr@canonical.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Standard functionality for the common clock API.
+ */
+
+#include <linux/clk.h>
+#include <linux/module.h>
+
+int clk_prepare(struct clk *clk)
+{
+	int ret = 0;
+
+	if (!clk->ops->prepare)
+		return 0;
+
+	mutex_lock(&clk->prepare_lock);
+	if (clk->prepare_count == 0)
+		ret = clk->ops->prepare(clk);
+
+	if (!ret)
+		clk->prepare_count++;
+	mutex_unlock(&clk->prepare_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(clk_prepare);
+
+void clk_unprepare(struct clk *clk)
+{
+	if (!clk->ops->unprepare)
+		return;
+
+	mutex_lock(&clk->prepare_lock);
+	if (--clk->prepare_count == 0)
+		clk->ops->unprepare(clk);
+
+	mutex_unlock(&clk->prepare_lock);
+}
+EXPORT_SYMBOL_GPL(clk_unprepare);
+
+int clk_enable(struct clk *clk)
+{
+	int ret = 0;
+
+	if (!clk->ops->enable)
+		return 0;
+
+	spin_lock(&clk->enable_lock);
+	if (!clk->enable_count)
+		ret = clk->ops->enable(clk);
+
+	if (!ret)
+		clk->enable_count++;
+	spin_unlock(&clk->enable_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clk_enable);
+
+void clk_disable(struct clk *clk)
+{
+	if (!clk->ops->disable)
+		return;
+
+	spin_lock(&clk->enable_lock);
+
+	WARN_ON(!clk->enable_count);
+
+	if (!--clk->enable_count)
+		clk->ops->disable(clk);
+
+	spin_unlock(&clk->enable_lock);
+}
+EXPORT_SYMBOL_GPL(clk_disable);
+
+unsigned long clk_get_rate(struct clk *clk)
+{
+	if (clk->ops->get_rate)
+		return clk->ops->get_rate(clk);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(clk_get_rate);
+
+int __clk_get(struct clk *clk)
+{
+	if (clk->ops->get)
+		return clk->ops->get(clk);
+	return 1;
+}
+EXPORT_SYMBOL_GPL(__clk_get);
+
+void clk_put(struct clk *clk)
+{
+	if (clk->ops->put)
+		clk->ops->put(clk);
+}
+EXPORT_SYMBOL_GPL(clk_put);
+
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+	if (clk->ops->round_rate)
+		return clk->ops->round_rate(clk, rate);
+	return -ENOSYS;
+}
+EXPORT_SYMBOL_GPL(clk_round_rate);
+
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	if (clk->ops->set_rate)
+		return clk->ops->set_rate(clk, rate);
+	return -ENOSYS;
+}
+EXPORT_SYMBOL_GPL(clk_set_rate);
+
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	if (clk->ops->set_parent)
+		return clk->ops->set_parent(clk, parent);
+	return -ENOSYS;
+}
+EXPORT_SYMBOL_GPL(clk_set_parent);
+
+struct clk *clk_get_parent(struct clk *clk)
+{
+	if (clk->ops->get_parent)
+		return clk->ops->get_parent(clk);
+	return ERR_PTR(-ENOSYS);
+}
+EXPORT_SYMBOL_GPL(clk_get_parent);

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 0fc0a79..17619c7 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c

@@ -84,12 +84,17 @@ struct clk *clk_get(struct device *dev, const char *con_id)
 }
 EXPORT_SYMBOL(clk_get);
 
+#ifndef CONFIG_USE_COMMON_STRUCT_CLK
+/* For the common struct clk case, clk_put is provided by clk.c */
+
 void clk_put(struct clk *clk)
 {
 	__clk_put(clk);
 }
 EXPORT_SYMBOL(clk_put);
 
+#endif
+
 void clkdev_add(struct clk_lookup *cl)
 {
 	mutex_lock(&clocks_mutex);

diff --git a/include/linux/clk.h b/include/linux/clk.h
index 1d37f42..e081ca1 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h

@@ -3,6 +3,7 @@
  *
  *  Copyright (C) 2004 ARM Limited.
  *  Written by Deep Blue Solutions Limited.
+ *  Copyright (c) 2010-2011 Jeremy Kerr <jeremy.kerr@canonical.com>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as

@@ -11,18 +12,189 @@
 #ifndef __LINUX_CLK_H
 #define __LINUX_CLK_H
 
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/spinlock.h>
+
 struct device;
 
-/*
- * The base API.
+#ifdef CONFIG_USE_COMMON_STRUCT_CLK
+
+/* If we're using the common struct clk, we define the base clk object here */
+
+/**
+ * struct clk - hardware independent clock structure
+ * @ops:		implementation-specific ops for this clock
+ * @enable_count:	count of clk_enable() calls active on this clock
+ * @flags:		platform-independent flags
+ * @lock:		lock for enable/disable or other HW-specific ops
+ *
+ * The base clock object, used by drivers for hardware-independent manipulation
+ * of clock lines. This will be 'subclassed' by device-specific implementations,
+ * which add device-specific data to struct clk. For example:
+ *
+ *  struct clk_foo {
+ *      struct clk;
+ *      [device specific fields]
+ *  };
+ *
+ * The clock driver code will manage the device-specific data, and pass
+ * clk_foo.clk to the common clock code. The clock driver will be called
+ * through the @ops callbacks.
+ *
+ * The @lock member provides either a spinlock or a mutex to protect (at least)
+ * @enable_count. The type of lock used will depend on @flags; if CLK_ATOMIC is
+ * set, then the core clock code will use a spinlock, otherwise a mutex. This
+ * lock will be acquired during clk_enable and clk_disable, so for atomic
+ * clocks, these ops callbacks must not sleep.
+ *
+ * The choice of atomic or non-atomic clock depends on how the clock is enabled.
+ * Typically, you'll want to use a non-atomic clock. For clocks that need to be
+ * enabled/disabled in interrupt context, use CLK_ATOMIC. Note that atomic
+ * clocks with parents will typically cascade enable/disable operations to
+ * their parent, so the parent of an atomic clock *must* be atomic too.
  */
+struct clk {
+	const struct clk_ops	*ops;
+	unsigned int		enable_count;
+	unsigned int		prepare_count;
+	spinlock_t		enable_lock;
+	struct mutex		prepare_lock;
+};
+
+/* static initialiser for non-atomic clocks */
+#define INIT_CLK(name, o) {						\
+	.ops		= &o,						\
+	.enable_count	= 0,						\
+	.prepare_count	= 0,						\
+	.enable_lock	= __SPIN_LOCK_UNLOCKED(name.enable_lock),	\
+	.prepare_lock	= __MUTEX_INITIALIZER(name.prepare_lock),	\
+}
 
+/**
+ * struct clk_ops -  Callback operations for clocks; these are to be provided
+ * by the clock implementation, and will be called by drivers through the clk_*
+ * API.
+ *
+ * @prepare:	Prepare the clock for enabling. This must not return until
+ *		the clock is fully prepared, and it's safe to call clk_enable.
+ *		This callback is intended to allow clock implementations to
+ *		do any initialisation that may block. Called with
+ *		clk->prepare_lock held.
+ *
+ * @unprepare:	Release the clock from its prepared state. This will typically
+ *		undo any work done in the @prepare callback. Called with
+ *		clk->prepare_lock held.
+ *
+ * @enable:	Enable the clock atomically. This must not return until the
+ *		clock is generating a valid clock signal, usable by consumer
+ *		devices. Called with clk->enable_lock held.
+ *
+ * @disable:	Disable the clock atomically. Called with clk->enable_lock held.
+ *
+ * @get:	Called by the core clock code when a device driver acquires a
+ *		clock via clk_get(). Optional.
+ *
+ * @put:	Called by the core clock code when a devices driver releases a
+ *		clock via clk_put(). Optional.
+ *
+ * The clk_enable/clk_disable and clk_prepare/clk_unprepare pairs allow
+ * implementations to split any work between atomic (enable) and sleepable
+ * (prepare) contexts.  If a clock requires blocking code to be turned on, this
+ * should be done in clk_prepare. Switching that will not block should be done
+ * in clk_enable.
+ *
+ * Typically, drivers will call clk_prepare when a clock may be needed later
+ * (eg. when a device is opened), and clk_enable when the clock is actually
+ * required (eg. from an interrupt).
+ *
+ * For other callbacks, see the corresponding clk_* functions. Parameters and
+ * return values are passed directly from/to these API functions, or
+ * -ENOSYS (or zero, in the case of clk_get_rate) is returned if the callback
+ * is NULL, see kernel/clk.c for implementation details. All are optional.
+ */
+struct clk_ops {
+	int		(*prepare)(struct clk *);
+	void		(*unprepare)(struct clk *);
+	int		(*enable)(struct clk *);
+	void		(*disable)(struct clk *);
+	int		(*get)(struct clk *);
+	void		(*put)(struct clk *);
+	unsigned long	(*get_rate)(struct clk *);
+	long		(*round_rate)(struct clk *, unsigned long);
+	int		(*set_rate)(struct clk *, unsigned long);
+	int		(*set_parent)(struct clk *, struct clk *);
+	struct clk *	(*get_parent)(struct clk *);
+};
+
+/**
+ * __clk_get - update clock-specific refcounter
+ *
+ * @clk: The clock to refcount
+ *
+ * Before a clock is returned from clk_get, this function should be called
+ * to update any clock-specific refcounting.
+ *
+ * Returns non-zero on success, zero on failure.
+ *
+ * Drivers should not need this function; it is only needed by the
+ * arch-specific clk_get() implementations.
+ */
+int __clk_get(struct clk *clk);
+
+/**
+ * clk_prepare - prepare clock for atomic enabling.
+ *
+ * @clk: The clock to prepare
+ *
+ * Do any blocking initialisation on @clk, allowing the clock to be later
+ * enabled atomically (via clk_enable). This function may sleep.
+ */
+int clk_prepare(struct clk *clk);
+
+/**
+ * clk_unprepare - release clock from prepared state
+ *
+ * @clk: The clock to release
+ *
+ * Do any (possbly blocking) cleanup on clk. This function may sleep.
+ */
+void clk_unprepare(struct clk *clk);
+
+/**
+ * clk_common_init - initialise a clock for driver usage
+ *
+ * @clk: The clock to initialise
+ *
+ * Used for runtime intialization of clocks; you don't need to call this
+ * if your clock has been (statically) initialized with INIT_CLK.
+ */
+static inline void clk_common_init(struct clk *clk)
+{
+	clk->enable_count = clk->prepare_count = 0;
+	spin_lock_init(&clk->enable_lock);
+	mutex_init(&clk->prepare_lock);
+}
+
+#else /* !CONFIG_USE_COMMON_STRUCT_CLK */
 
 /*
- * struct clk - an machine class defined object / cookie.
+ * Global clock object, actual structure is declared per-machine
  */
 struct clk;
 
+static inline void clk_common_init(struct clk *clk) { }
+
+/*
+ * For !CONFIG_USE_COMMON_STRUCT_CLK, we don't enforce any atomicity
+ * requirements for clk_enable/clk_disable, so the prepare and unprepare
+ * functions are no-ops
+ */
+int clk_prepare(struct clk *clk) { return 0; }
+void clk_unprepare(struct clk *clk) { }
+
+#endif /* !CONFIG_USE_COMMON_STRUCT_CLK */
+
 /**
  * clk_get - lookup and obtain a reference to a clock producer.
  * @dev: device for clock "consumer"

@@ -83,12 +255,6 @@ unsigned long clk_get_rate(struct clk *clk);
  */
 void clk_put(struct clk *clk);
 
-
-/*
- * The remaining APIs are optional for machine class support.
- */
-
-
 /**
  * clk_round_rate - adjust a rate to the exact rate a clock can provide
  * @clk: clock source

On Mon, Feb 07, 2011 at 02:07:57PM +0800, Jeremy Kerr wrote:

Did we want to check for prepare_count > 0 here?  Russell's suggestion
was to do that without holding any lock.
(To make this a tad safer, you could use
ACCESS_ONCE(clk->enable_count)++ below. (Suggested by paulmck on irc.))

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

On Mon, Feb 07, 2011 at 02:07:57PM +0800, Jeremy Kerr wrote:

This comment needs a general overhaul.

(I had this comment already in my first mail, but I must have deleted it
when removing the unrealted quotes.)

+struct clk {
+	const struct clk_ops	*ops;
+	unsigned int		enable_count;
+	unsigned int		prepare_count;
+	spinlock_t		enable_lock;
+	struct mutex		prepare_lock;
+};

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

On Feb 6, 2011 10:08 PM, "Jeremy Kerr" [off-list ref] wrote:

return ret;

*con_id)

here */

+
+/**
+ * struct clk - hardware independent clock structure
+ * @ops:               implementation-specific ops for this clock
+ * @enable_count:      count of clk_enable() calls active on this clock
+ * @flags:             platform-independent flags
+ * @lock:              lock for enable/disable or other HW-specific ops
+ *
+ * The base clock object, used by drivers for hardware-independent

manipulation

+ * of clock lines. This will be 'subclassed' by device-specific

implementations,

+ * which add device-specific data to struct clk. For example:
+ *
+ *  struct clk_foo {
+ *      struct clk;
+ *      [device specific fields]
+ *  };
+ *
+ * The clock driver code will manage the device-specific data, and pass
+ * clk_foo.clk to the common clock code. The clock driver will be called
+ * through the @ops callbacks.
+ *
+ * The @lock member provides either a spinlock or a mutex to protect (at

least)

+ * @enable_count. The type of lock used will depend on @flags; if

CLK_ATOMIC is

+ * set, then the core clock code will use a spinlock, otherwise a mutex.

This

+ * lock will be acquired during clk_enable and clk_disable, so for atomic
+ * clocks, these ops callbacks must not sleep.
+ *
+ * The choice of atomic or non-atomic clock depends on how the clock is

enabled.

+ * Typically, you'll want to use a non-atomic clock. For clocks that need

to be

+ * enabled/disabled in interrupt context, use CLK_ATOMIC. Note that

atomic

+ * clocks with parents will typically cascade enable/disable operations

to

+ * their parent, so the parent of an atomic clock *must* be atomic too.
 */
+struct clk {
+       const struct clk_ops    *ops;
+       unsigned int            enable_count;
+       unsigned int            prepare_count;
+       spinlock_t              enable_lock;
+       struct mutex            prepare_lock;
+};
+
+/* static initialiser for non-atomic clocks */
+#define INIT_CLK(name, o) {                                            \
+       .ops            = &o,                                           \
+       .enable_count   = 0,                                            \
+       .prepare_count  = 0,                                            \
+       .enable_lock    = __SPIN_LOCK_UNLOCKED(name.enable_lock),       \
+       .prepare_lock   = __MUTEX_INITIALIZER(name.prepare_lock),       \
+}

+/**
+ * struct clk_ops -  Callback operations for clocks; these are to be

provided

+ * by the clock implementation, and will be called by drivers through the