[PATCH 1/2] Add a common struct clk

[Ryan Mallon]

On 01/17/2011 10:07 AM, Uwe Kleine-König wrote:
> On Mon, Jan 17, 2011 at 09:41:46AM +1300, Ryan Mallon wrote:
>> On 01/16/2011 08:26 PM, Grant Likely wrote:
>>> On Mon, Jan 10, 2011 at 5:54 PM, Jeremy Kerr <jeremy.kerr at canonical.com> wrote:
>>>> Hi Russell,
>>>>
>>>>> Unless the locking problems can be resolved, the patches aren't ready.
>>>>>
>>>>> From what I've seen there's still quite a problem with what kind of
>>>>> lock to use in the clock - mutex or spinlock.
>>>>
>>>> Yes, the clock driver may either use a spinlock or mutex.
>>>>
>>>> However, this exactly the same as the current clock code, my patches do not
>>>> alter what we currently have.
>>>>
>>>> I do agree that we should define some specific semantics for the clock API
>>>> with regards to sleeping, and I'll start a new thread about that. But we
>>>> should definitely separate that issue from the problem of having multiple
>>>> definitions for struct clk, which is what these patches address.
>>>
>>> Given that each current struct clk implementation makes it's own
>>> decisions about how to handle locking, would it not make sense to omit
>>> locking entirely?  At least as a first step?  Let the clock ops
>>> implement their own locking.  Make enable count management their
>>> responsibility too.  That's all that the lock protects anyway.  The
>>> clk_* apis can fall directly through to the ops hooks with no
>>> manipulation or locking.  The way v3 of the patch worked.
>>
>> This doesn't really work in a generic API because the call sites need to
>> know whether it safe to call clk_enable/disable, etc from a
>> non-sleepable context. This has essentially the same problems as using a
>> union with both a mutex and a spinlock.
> Note that this is the current status quo, too.  clk_enable on imx
> sleeps, clk_enable on at91 doesn't.  I think we all agree we don't want
> to change that during the unifying step.  So the common struct clk might
> not solve all problems, but it's not worse than the state now.

Yes, I pointed this out. I think we have ended up in this ugly situation
because the clock API calls never defined rules about calling context
and so some platforms have ended up needing to sleep in the calls, while
drivers for other platforms assume that the clock API calls can be
safely called from atomic context.

My point, and I think Russell's point, is that if you are going to have
a truly generic, unified API we don't want this sort of mix and match.
The clock API functions should have clearly defined rules about what
context they can be called from and each platform/machine implementation
of the API should obey those rules.

Partly this is about code clarity. At the moment, to determine if a
clk_enable call might sleep I need to go and look at the implementation
for the platform I am on. There are drivers, such as the amba-pl010
serial driver, which can be used on multiple platforms. To verify that
the clock API call sites are correct you need to check each platform
that the driver runs on. By having an explicit separation of the clock
implementations which are atomic and those which may need to sleep these
problems disappear.

For the current patch series, the documentation for clk_enable would
need include this.

  "This function may or may not be safe to call from atomic context
   depending on the platform you are on."

I don't think we have too many other generic APIs in the kernel which
have this kind of loose documentation.

>> That said, although we are aiming for a generic clock API, it's usage
>> typically isn't. Most of the users of the clock API are machine/platform
>> specific drivers and so they can make assumptions about the
>> implementation of the clock API calls. We currently do have a generic
>> set of clock calls which do not define rules about calling from atomic
>> context. We have a number of drivers, for several platforms, using the
>> clk_enable/disable calls. Some platforms are using sleeping clocks and
>> others are using atomic clocks, but because the _usage_ of the API is
>> largely platform specific it all hangs together.
>>
>> This is a bit ugly in practice. It's hardly desirable to have an API
>> which looks generic, but in reality is platform/machine specific. I'm
>> guessing that a large benefit of having a unified API and well defined
>> rules about whether clock API calls may sleep or not allows the usage of
>> the clock API to become more generic.
>>
>> I agree with Russell that the best short term approach is to create two
>> clock APIs, one for clocks which are atomic and the other for clocks
>> which need to sleep. If nothing else this at least allows the rules
>> about calling context to nailed down.
> For me this sounds like code duplication if everything is identical only
> spinlocks are substituted by mutexes.  What do you think about my
> suggestion to use a Kconfig symbol that is selected by platforms (e.g.
> COMMON_CLK_SLEEPING) and then use:
> 
> 	#ifdef CONFIG_COMMON_CLK_SLEEPING
> 		... mutex stuff ...
> 	#else
> 		... spinlock stuff ...
> 	#endif

You still have the problem that cannot make any assumptions about
calling context for calls to the clock API. The point is that the
semantics about calling context for a generic clock API are currently
too loose. Part of consolidating the clock implementations should be to
fix that. Russell's suggestion to reduce to two implementations (from
the 30+ we have now) will have a net reduction in code, but will also
clarify which platforms/drivers are using sleeping clocks.

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan at bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934