Common struct clk implementation, v10

+ * 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.

Quoted 11 lines · Jeremy Kerr

+ * 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.

...

include/linux/clk.h line 67

Quoted 9 lines

include/linux/clk.h +9 -0

+struct clk {
+	const struct clk_ops	*ops;
+	unsigned int		enable_count;
+	int			flags;
+	union {
+		struct mutex	mutex;
+		spinlock_t	spinlock;
+	} lock;
+};

Here you have a "polymorphic" lock, where the clock instance knows which type it is supposed to be. I got flak from David Miller and others trying to do the same thing with the mdio_bus:

   http://kerneltrap.org/mailarchive/linux-netdev/2010/7/6/6280618

The criticism, applied to your case, is that the clk_enable() caller cannot know whether it is safe to make the call or not. I was told, "there has got to be a better way."

Quoted 16 lines

include/linux/clk.h line 67

Quoted 9 lines

include/linux/clk.h +9 -0

+struct clk {
+	const struct clk_ops	*ops;
+	unsigned int		enable_count;
+	int			flags;
+	union {
+		struct mutex	mutex;
+		spinlock_t	spinlock;
+	} lock;
+};

Here you have a "polymorphic" lock, where the clock instance knows which type it is supposed to be. I got flak from David Miller and

others trying to do the same thing with the mdio_bus:

   http://kerneltrap.org/mailarchive/linux-netdev/2010/7/6/6280618

Quoted 3 lines

The criticism, applied to your case, is that the clk_enable() caller cannot know whether it is safe to make the call or not. I was told, "there has got to be a better way."

Note that this is not "new". Currently there is no convention available if clk_enable sleeps or not. See e.g.
http://thread.gmane.org/gmane.linux.ports.arm.kernel/100744

Hi Richard,

Quoted 16 lines

Quoted 16 lines

include/linux/clk.h line 67

Quoted 9 lines

include/linux/clk.h +9 -0

+struct clk {
+	const struct clk_ops	*ops;
+	unsigned int		enable_count;
+	int			flags;
+	union {
+		struct mutex	mutex;
+		spinlock_t	spinlock;
+	} lock;
+};

Here you have a "polymorphic" lock, where the clock instance knows which type it is supposed to be. I got flak from David Miller and

others trying to do the same thing with the mdio_bus:

   http://kerneltrap.org/mailarchive/linux-netdev/2010/7/6/6280618

Quoted 7 lines

Quoted 3 lines

The criticism, applied to your case, is that the clk_enable() caller cannot know whether it is safe to make the call or not. I was told, "there has got to be a better way."

Note that this is not "new". Currently there is no convention available if clk_enable sleeps or not. See e.g.
http://thread.gmane.org/gmane.linux.ports.arm.kernel/100744

As Uwe says, the common clock does not change these semantics; I would prefer to keep the driver API changes at a minimum with these patches.

But yes, it would be a good idea to:

* introduce clk_enable_atomic, which requires clk->flags & CLK_ATOMIC

* add might_sleep to clk_enable(), encouraging clk uses in atomic contexts to switch to clk_enable_atomic.

We'd still be able to handle CLK_ATOMIC clocks in clk_enable(), so the enforcement only needs to be one-way.

Quoted 31 lines · Jeremy Kerr

Hi Richard,

Quoted 16 lines

Quoted 16 lines

include/linux/clk.h line 67

Quoted 9 lines

include/linux/clk.h +9 -0

+struct clk {
+	const struct clk_ops	*ops;
+	unsigned int		enable_count;
+	int			flags;
+	union {
+		struct mutex	mutex;
+		spinlock_t	spinlock;
+	} lock;
+};

Here you have a "polymorphic" lock, where the clock instance knows which type it is supposed to be. I got flak from David Miller and

others trying to do the same thing with the mdio_bus:

   http://kerneltrap.org/mailarchive/linux-netdev/2010/7/6/6280618

Quoted 7 lines

Quoted 3 lines

The criticism, applied to your case, is that the clk_enable() caller cannot know whether it is safe to make the call or not. I was told, "there has got to be a better way."

Note that this is not "new". Currently there is no convention available if clk_enable sleeps or not. See e.g.
http://thread.gmane.org/gmane.linux.ports.arm.kernel/100744

As Uwe says, the common clock does not change these semantics; I would prefer to keep the driver API changes at a minimum with these patches.

But yes, it would be a good idea to:

Quoted 7 lines

* introduce clk_enable_atomic, which requires clk->flags & CLK_ATOMIC

* add might_sleep to clk_enable(), encouraging clk uses in atomic contexts to switch to clk_enable_atomic.

We'd still be able to handle CLK_ATOMIC clocks in clk_enable(), so the enforcement only needs to be one-way.

I think the atomic stuff should be the norm through and through - otherwise we're going to end up with problems in drivers where they use the _atomic() stuff, but the clocks behind are coded to sleep.

I hate the GPIO APIs for doing this _cansleep crap as the decision of whether to use the _cansleep or normal APIs normally can't be made at the time when the API is used, but sometime later. Many people just use the non-_cansleep versions irrespective of the context they're in - which is unnecessarily restrictive - consider what happens if you then have that driver use a GPIO on an I2C peripheral...

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;
	int			flags;
	union {
		struct mutex	mutex;
		spinlock_t	spinlock;
	} lock;
};

I'm currently thinking about how to get the locking right with this approach. In the current i.MX implementation we have a global lock which protects the clock enable counter and also the register accesses in the clock code. With the common struct clock we have a lock per clock which only protects the enable counter, so we have to introduce a second lock to protect the register accesses.

The problem comes with nested calls to for example clk_enable which happens when the parent clock gets enabled. currently we do this with clk->enable(clk->parent) which results in an unlocked clk_enable of the parent. With common struct clk we would have to call clk_enable(clk_get_parent(clk) which results in taking the lock a second time.
Any ideas how to solve this?

Hi Sascha,

Quoted 6 lines

I'm currently thinking about how to get the locking right with this approach. In the current i.MX implementation we have a global lock which protects the clock enable counter and also the register accesses in the clock code. With the common struct clock we have a lock per clock which only protects the enable counter, so we have to introduce a second lock to protect the register accesses.

Are the registers shared between clocks? If not, you can just use the existing per-clk lock. Otherwise it'd be reasonable to add a global register lock, protecting accesses to the shared register set (and *only* protecting these registers).

Quoted 8 lines

The problem comes with nested calls to for example clk_enable which happens when the parent clock gets enabled. currently we do this with clk->enable(clk->parent) which results in an unlocked clk_enable of the parent. With common struct clk we would have to call clk_enable(clk_get_parent(clk) which results in taking the lock a second time.
Any ideas how to solve this?

With the shared register lock, you just need to make sure that you don't recurse to the parent while holding the lock.

For clocks with a shared register set, the general pattern would be something like:

struct clk_foo {
	struct clk clk;
	u32        enable_reg;
	u32        enable_mask;
	struct clk *parent;
};

static DEFINE_SPINLOCK(clk_foo_register_lock);

/* called with _clk->lock held */
static int clk_foo_enable(struct clk *_clk)

{
	struct clk_foo *clk = to_clk_foo(_clk);
	int reg, rc;

	/* enable parent - will acquire and release the parent's per-clk lock */
	rc = clk_enable(clk->parent);
	if (rc)
		return rc;

	/* do register update, under global register lock */
	spin_lock(&clk_foo_register_lock);

	reg = __raw_readl(clk->reg);
	__raw_writel(clk->reg, reg | clk->enable_mask);

	spin_unlock(&clk_foo_register_lock);

	return 0;
}

struct clk_foo_ops = {
	.enable = clk_foo_enable;
	[...]
};

However, because clk_mxc introduces its own set of abstractions, there may be some merging to do here. For my work on mx51, I've done a very basic port:

* changed plat-mxc's struct clk to struct clk_mxc
* embedded struct clk into struct clk_mxc (ie, making it use the common API)
* separated some of the simpler clocks to separate types (eg clk_fixed, clk_pll, clk_ccgr).

The goal here is to separate all of the clocks into their most basic types, leaving no clk_mxc clocks remaining, then the locking should be much simpler.

This branch must be reworked because the correct locking is missing, but the first branch should be ready for merging once your clk-common patches are merged. I'll post the patches for review soon. I hope it's clear soon that your clk-common patches get merged.

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.

Hi Russell,

Quoted 4 lines

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.

Quoted 9 lines · Jeremy Kerr

Hi Russell,

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 18 lines · Grant Likely

Quoted 9 lines · Jeremy Kerr

Hi Russell,

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 7 lines

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.

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.

Quoted 27 lines · Ryan Mallon

Quoted 25 lines · Grant Likely

Quoted 9 lines · Jeremy Kerr

Hi Russell,

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 16 lines

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.

Quoted 4 lines

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

kernel/clk.c new file +13 -0

new file mode 100644
index 0000000..8de8fe3
@@ -0,0 +1,102 @@
+/*
+ * Copyright (C) 2010 Canonical Ltd <[email protected]>
+ *
+ * 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>
+

The i.MX clk implementation disables the old parent if clk is enabled and enables the new parent if clk is enabled (modulo bugs). Shouldn't we do this here aswell?

I thought about returning -EBUSY if clk_set_parent is called for an enabled clk, but this way we could never reparent the cpu clock which I think is done in the Freescale BSP for power saving.

Hi Sascha,

Quoted 3 lines

The i.MX clk implementation disables the old parent if clk is enabled and enables the new parent if clk is enabled (modulo bugs). Shouldn't we do this here aswell?

Sounds reasonable, yes.

Quoted 3 lines

I thought about returning -EBUSY if clk_set_parent is called for an enabled clk, but this way we could never reparent the cpu clock which I think is done in the Freescale BSP for power saving.

I think that the possibility for changing the parent really depends on the implementation; in some cases we may want to disallow it, in others it might be fine.

Related: do we really need globally-accessible clk_{get,set}_parent in the clk API? For cases where we need to set the parent, we probably need details about the platform clock configuration (eg, which clocks are possible parents). In this case, we could just call into the clock driver directly.

Quoted 16 lines · Jeremy Kerr

Hi Sascha,

Quoted 3 lines

The i.MX clk implementation disables the old parent if clk is enabled and enables the new parent if clk is enabled (modulo bugs). Shouldn't we do this here aswell?

Sounds reasonable, yes.

Quoted 3 lines

I thought about returning -EBUSY if clk_set_parent is called for an enabled clk, but this way we could never reparent the cpu clock which I think is done in the Freescale BSP for power saving.

I think that the possibility for changing the parent really depends on the implementation; in some cases we may want to disallow it, in others it might be fine.

Quoted 4 lines

Related: do we really need globally-accessible clk_{get,set}_parent in the clk API? For cases where we need to set the parent, we probably need details about the platform clock configuration (eg, which clocks are possible parents). In this case, we could just call into the clock driver directly.

I agree that drivers have no business calling clk_{get,set}_parent, this is purely platform specific.

Quoted 22 lines · Sascha Hauer

Quoted 20 lines · Jeremy Kerr

Hi Sascha,

Quoted 3 lines

The i.MX clk implementation disables the old parent if clk is enabled and enables the new parent if clk is enabled (modulo bugs). Shouldn't we do this here aswell?

Sounds reasonable, yes.

Quoted 3 lines

I thought about returning -EBUSY if clk_set_parent is called for an enabled clk, but this way we could never reparent the cpu clock which I think is done in the Freescale BSP for power saving.

I think that the possibility for changing the parent really depends on the implementation; in some cases we may want to disallow it, in others it might be fine.

Related: do we really need globally-accessible clk_{get,set}_parent in the clk API? For cases where we need to set the parent, we probably need details about the platform clock configuration (eg, which clocks are possible parents). In this case, we could just call into the clock driver directly.

Quoted 3 lines

I agree that drivers have no business calling clk_{get,set}_parent, this is purely platform specific.

Do you guys even bother to grep the kernel for users of the API before coming up with arbitrary policy?

There are plenty of cases where clocks are allocated dynamically by driver code that in turn can be set up as a parent for other dynamically allocated clocks. This has not a damn thing to do with platform code and everything to do with the clock circuitry of the device or IP block in question.

If you actually want this API unification thing to not be completely stillborn, I recommend focusing on how people are actually using the API today (especially the parts that are used by the majority of users, not the one you happened to base your implementation off of), and then trying to push your interpretation or roadmap for the API at a later stage. This way the former has a chance of getting upstream without the latter completely derailing it.

Quoted 2 lines · Paul Mundt

Do you guys even bother to grep the kernel for users of the API before coming up with arbitrary policy?

Actually I did bother to grep the kernel and I came up with these results:

drivers/video/omap2/dss/dss.c:219: dpll4_ck_rate = clk_get_rate(clk_get_parent(dss.dpll4_m4_ck));
drivers/video/omap2/dss/dss.c:312: prate = clk_get_rate(clk_get_parent(dss.dpll4_m4_ck));

drivers/video/omap2/dss/dss.c:325:              prate = clk_get_rate(clk_get_parent(dss.dpll4_m4_ck));
drivers/video/omap2/dss/dss.c:344:              prate = clk_get_rate(clk_get_parent(dss.dpll4_m4_ck));
drivers/video/omap2/dss/dss.c:359:              return clk_get_rate(clk_get_parent(dss.dpll4_m4_ck));

drivers/video/sh_mobile_hdmi.c:716: else if (clk_get_parent(hdmi->hdmi_clk))
drivers/video/sh_mobile_hdmi.c:717: *parent_rate = clk_get_rate(clk_get_parent(hdmi->hdmi_clk));
drivers/video/sh_mobile_hdmi.c:1089: if (parent_rate && clk_get_parent(hdmi->hdmi_clk)) {
drivers/video/sh_mobile_hdmi.c:1090: ret = clk_set_rate(clk_get_parent(hdmi->hdmi_clk), parent_rate);
drivers/usb/host/ehci-omap.c:405: ret = clk_set_parent(omap->utmi_p1_fck,
drivers/usb/host/ehci-omap.c:435: ret = clk_set_parent(omap->utmi_p2_fck,

Quoted 5 lines

There are plenty of cases where clocks are allocated dynamically by driver code that in turn can be set up as a parent for other dynamically allocated clocks. This has not a damn thing to do with platform code and everything to do with the clock circuitry of the device or IP block in question.

The majority of users do not use clk_{get,set}_parent at all. And it's really questionable whether drivers should know anything about the layout of the clock tree. I'm pretty sure that these are the code pieces where there will be a if_soc_rev(x) around it once the next incarnation of a SoC comes out.

---
 arch/Kconfig        |    3 
 include/linux/clk.h |  164 +++++++++++++++++++++++++++++++++++++++++---
 kernel/Makefile     |    1 
 kernel/clk.c        |  102 +++++++++++++++++++++++++++
 4 files changed, 261 insertions(+), 9 deletions(-)

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.

Quoted 7 lines · Jeremy Kerr

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.

no, it doesn't actually. you could do run-time link and select the right arch support.