Add a common struct clk
in [Kernel]
|
Prev: X86:reboot.c Add some dmi entries to pci_reboot_dmi_table.
Next: ext3: fix non-update ctime when changing the file's permission by setfacl
From: Ryan Mallon on 3 Jun 2010 17:10 Ben Dooks wrote: > On Thu, Jun 03, 2010 at 11:21:19AM +0800, Jeremy Kerr wrote: >> Hi Ben, >> >>>> And a set of clock operations (defined per type of clock): >>>> >>>> struct clk_operations { >>>> >>>> int (*enable)(struct clk *); >>> I'd rather the enable/disable calls where simply a set >>> and a bool on/off, very rarelyt is the enable and disable >>> operartions different. >> I thought about merging these, but decided against it. It does work for the >> simple case where we're setting a bit in a register: >> >> static int clk_foo_set_state(struct clk *_clk, int enable) >> { >> struct clk_foo *clk = to_clk_foo(_clk) >> u32 reg; >> >> reg = raw_readl(foo->some_register); >> if (enable) >> reg |= FOO_ENABLE; >> else >> reg &= ~FOO_ENABLE; >> raw_writel(foo->some_register, reg); >> >> return 0; >> } >> >> However, for anything more complex than this - for example, if there's a >> parent clock - then we start getting pretty messy: >> >> static int clk_foo_set_state(struct clk *_clk, int enable) >> { >> struct clk_foo *clk = to_clk_foo(_clk) >> u32 reg; > > Yuck. I think this should really be handled by the base clk_enable() > and clk_disable() calls. Roughly based on what is currently in the > plat-samsung clock implementation: I think its a good idea to do this incrementally. The proposed patches don't require much code rewrite because the interface is basically the same. I think the best approach is to get the proposed patches applied, which basically just makes the common interface from include/linux/clock.h generic, and _all_ of the mach implementations (and possibly other archs such as powerpc) converted and tested first. Then we can go from there to see what other common functionality can be moved into the generic clock framework. ~Ryan -- Bluewater Systems Ltd - ARM Technology Solution Centre Ryan Mallon 5 Amuri Park, 404 Barbadoes St ryan(a)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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Ben Dooks on 3 Jun 2010 19:50 On Fri, Jun 04, 2010 at 09:09:29AM +1200, Ryan Mallon wrote: > Ben Dooks wrote: > > On Thu, Jun 03, 2010 at 11:21:19AM +0800, Jeremy Kerr wrote: > >> Hi Ben, > >> > >>>> And a set of clock operations (defined per type of clock): > >>>> > >>>> struct clk_operations { > >>>> > >>>> int (*enable)(struct clk *); > >>> I'd rather the enable/disable calls where simply a set > >>> and a bool on/off, very rarelyt is the enable and disable > >>> operartions different. > >> I thought about merging these, but decided against it. It does work for the > >> simple case where we're setting a bit in a register: > >> > >> static int clk_foo_set_state(struct clk *_clk, int enable) > >> { > >> struct clk_foo *clk = to_clk_foo(_clk) > >> u32 reg; > >> > >> reg = raw_readl(foo->some_register); > >> if (enable) > >> reg |= FOO_ENABLE; > >> else > >> reg &= ~FOO_ENABLE; > >> raw_writel(foo->some_register, reg); > >> > >> return 0; > >> } > >> > >> However, for anything more complex than this - for example, if there's a > >> parent clock - then we start getting pretty messy: > >> > >> static int clk_foo_set_state(struct clk *_clk, int enable) > >> { > >> struct clk_foo *clk = to_clk_foo(_clk) > >> u32 reg; > > > > Yuck. I think this should really be handled by the base clk_enable() > > and clk_disable() calls. Roughly based on what is currently in the > > plat-samsung clock implementation: > > I think its a good idea to do this incrementally. The proposed patches > don't require much code rewrite because the interface is basically the > same. I think the best approach is to get the proposed patches applied, > which basically just makes the common interface from Given the latest comments by Linus on churn, it would be better to get a well specified <linux/clk.h> decided on before it goes in so that everyone can move over to it. We're moving to a system where any change in functionality is going to cause problems with respect to a wide range of systems. If the new <linux/clk.h> is not well specified it is just goign to cause problems down the line of people infering behaviour from other implementations (a bad idea) and/or causing large tracts of changes. > include/linux/clock.h generic, and _all_ of the mach implementations > (and possibly other archs such as powerpc) converted and tested first. > Then we can go from there to see what other common functionality can be > moved into the generic clock framework. -- Ben Q: What's a light-year? A: One-third less calories than a regular year. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Ben Dooks on 3 Jun 2010 20:10 On Thu, Jun 03, 2010 at 12:05:33PM +0100, Russell King - ARM Linux wrote: > On Thu, Jun 03, 2010 at 06:24:50PM +0800, Jeremy Kerr wrote: > > OK, this would mean adding parent to struct clk: > > > > struct clk { > > struct clk_operations *ops; > > atomic_t enable_count; > > I don't think it makes sense for this to be an atomic type. > > > static inline int clk_enable(struct clk *clk) > > { > > int ret; > > > > if (atomic_inc(clk->enable_count) != 1)) > > return 0; > > Okay, so what if we sleep at this point, and someone else comes along > and calls clk_enable(), which'll merely increment the count and return. > > Unfortunately, the clock is still disabled at that point - which is a > violation of what's supposed to happen. > > Or to put it another way, the above method results in clk_enable() > sometimes returning with the clock enabled and sometimes with the > clock still disabled. > > That's not nice behaviour for drivers which may need the clock enabled > to read/write the device registers. You're right, especially if the clock takes time to enable. Either we need some form of locking to deal with this. An overall lock will cause problems with respect to re-using clk_enable() internally and per-clock locks are probably a bad idea with respect of trying to lock the parents (deadlock) depending on the implementation. I'm not sure if something like a wake queue is required, where any caller who is calling when the clock is disabled gets placed on the queue awaiting the original enabler finishing the enable operation. if (atomic_increment(&clk->usage_count) == 1) { /* start enable */ atomic_set(&clk->is_enabled, 1); wake_up(&clk->wait_for_enable); } else if (atomic_read(&clk->is_enabled) == 0) { wait_event(&clk->wait_for_enable); } Also, what about the behaviour across >1 CPU? There's a pile of up-comming SoCs with SMP ARM cores. If CPU1 enables the clock, and CPU2 comes along and tries to do that also, we have the same race condition again. As an addendum to Russell's comments, we need to specify the behaviour with resepect to what happens about clock stabilisation as well, if the clock we enabled takes time to stabilise, then should we wait? This will be even worse when we realise some of these architectures have clocks which are source from PLLs that can be turned on/off, and these PLLs have settling time in 100s of usecs. Even if the clock is enabled, it requires time to stabilise to avoid problems when the driver then goes to try and use it. -- Ben Q: What's a light-year? A: One-third less calories than a regular year. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jeremy Kerr on 3 Jun 2010 21:50 Hi Russell, > Or to put it another way, the above method results in clk_enable() > sometimes returning with the clock enabled and sometimes with the > clock still disabled. > > That's not nice behaviour for drivers which may need the clock enabled > to read/write the device registers. OK, I'll rework with proper locking to ensure that the clock is enabled/disabled on return from clk_enable. Cheers, Jeremy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Ben Dooks on 11 Jun 2010 00:30
On Fri, Jun 04, 2010 at 03:30:08PM +0800, Jeremy Kerr wrote: > 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. Whilst I agree that this is a useful thing to do, I'd like to ensure we get a good base for our work before we get another reason for Linus to complain about churn. > 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: What do people think of just changing everyone who is currently using clk support to using this new implementation? > struct clk { > const struct clk_ops *ops; > unsigned int enable_count; > struct mutex mutex; I'm a little worried about the size of struct clk if all of them have a mutex in them. If i'm right, this is 40 bytes of data each clock has to take on board. Doing the following: find arch/arm -type f -name "*.c" | xargs grep -c -E "struct.*clk.*=.*{" | grep -v ":0" | awk 'BEGIN { count=0; FS=":" } count += $2; END { print count }' indicates that there's approximately 2241 clocks under arch/arm at the moment. That's about 87KiB of mutexes if all are being compiled at once. ~> }; > > And a set of clock operations (defined per type of clock): > > struct clk_operations { > int (*enable)(struct clk *); still think that not passing an bool as a second argument is silly. > 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_operations clk_foo_ops = { > .get_rate = clk_foo_get_rate, > }; > > The common clock definitions are based on a development patch from Ben > Herrenschmidt <benh(a)kernel.crashing.org>. > > Signed-off-by: Jeremy Kerr <jeremy.kerr(a)canonical.com> > > --- > arch/Kconfig | 3 > include/linux/clk.h | 159 ++++++++++++++++++++++++++++++++++++-------- > 2 files changed, 135 insertions(+), 27 deletions(-) ~~~~> > diff --git a/arch/Kconfig b/arch/Kconfig > index acda512..2458b5e 100644 > --- a/arch/Kconfig > +++ b/arch/Kconfig > @@ -151,4 +151,7 @@ config HAVE_MIXED_BREAKPOINTS_REGS > config HAVE_USER_RETURN_NOTIFIER > bool > > +config USE_COMMON_STRUCT_CLK > + bool > + > source "kernel/gcov/Kconfig" > diff --git a/include/linux/clk.h b/include/linux/clk.h > index 1d37f42..bb6957a 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 Jeremy Kerr <jeremy.kerr(a)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,36 +12,125 @@ > #ifndef __LINUX_CLK_H > #define __LINUX_CLK_H > > -struct device; > +#include <linux/err.h> > +#include <linux/mutex.h> > > -/* > - * The base API. > +#ifdef CONFIG_USE_COMMON_STRUCT_CLK > + > +/* If we're using the common struct clk, we define the base clk object here, > + * which will be 'subclassed' by device-specific implementations. For example: > + * > + * struct clk_foo { > + * struct clk; > + * [device specific fields] > + * }; > + * > + * We define the common clock API through a set of static inlines that call the > + * corresponding clk_operations. The API is exactly the same as that documented > + * in the !CONFIG_USE_COMMON_STRUCT_CLK case. > */ > > +struct clk { > + const struct clk_ops *ops; > + unsigned int enable_count; > + struct mutex mutex; > +}; how about defining a nice kerneldoc for this. > +#define INIT_CLK(name, o) \ > + { .ops = &o, .enable_count = 0, \ > + .mutex = __MUTEX_INITIALIZER(name.mutex) } how about doing the mutex initinitialisation at registration time, will save a pile of non-zero code in the image to mess up the compression. ~> +struct clk_ops { > + int (*enable)(struct clk *); > + void (*disable)(struct clk *); > + unsigned long (*get_rate)(struct clk *); > + void (*put)(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 *); should each clock carry a parent field and the this is returned by the get parent call.~~ > +}; > + > +static inline int clk_enable(struct clk *clk) > +{ > + int ret = 0; > + > + if (!clk->ops->enable) > + return 0; > + > + mutex_lock(&clk->mutex); > + if (!clk->enable_count) > + ret = clk->ops->enable(clk); > + > + if (!ret) > + clk->enable_count++; > + mutex_unlock(&clk->mutex); > + > + return ret; > +} So we're leaving the enable parent code now to each implementation? I think this is a really bad decision, it leaves so much open to bad code repetition, as well as something the core should really be doing if it had a parent clock field. ~> +static inline void clk_disable(struct clk *clk) > +{ > + if (!clk->ops->enable) > + return; so if we've no enable call we ignore disable too? also, we don't keep an enable count if this fields are in use, could people rely on this being correct even if the clock has no enable/disable fields. Would much rather see the enable_count being kept up-to-date no matter what, given it may be watched by other parts of the implementation, useful for debug info, and possibly useful if later in the start sequence the clk_ops get changed to have this field.~ > +~ mutex_lock(&clk->mutex); > + > + if (!--clk->enable_count) > + clk->ops->disable(clk); > + > + mutex_unlock(&clk->mutex); > +} > + > +static inline unsigned long clk_get_rate(struct clk *clk) > +{ > + if (clk->ops->get_rate) > + return clk->ops->get_rate(clk); > + return 0; > +} > + > +static inline void clk_put(struct clk *clk) > +{ > + if (clk->ops->put) > + clk->ops->put(clk); > +} I'm beginging to wonder if we don't just have a set of default ops that get set into the clk+ops at registration time if these do not have an implementation. ~ > +static inline long clk_round_rate(struct clk *clk, unsigned long rate) > +{ > + if (clk->ops->round_rate) > + return clk->ops->round_rate(clk, rate); > + return -ENOSYS; > +} > + > +static inline int clk_set_rate(struct clk *clk, unsigned long rate) >~ +{ > + if (clk->ops->set_rate) > + return clk->ops->set_rate(clk, rate); > + return -ENOSYS; > +} > + > +static inline int clk_set_parent(struct clk *clk, struct clk *parent) > +{ > + if (clk->ops->set_parent) > + return clk->ops->set_parent(clk, parent); > + return -ENOSYS; > +} We have an interesting problem here which I belive should be dealt with, what happens when the clock's parent is changed with respect to the enable count of the parent. With the following instance: we have clocks a, b, c; a and b are possible parents for c; c starts off with a as parent then the driver comes along: 1) gets clocks a, b, c; 2) clk_enable(c); 3) clk_set_parent(c, b); now we have the following: A) clk a now has an enable count of non-zero B) clk b may not be enabled C) even though clk a may now be unused, it is still running D) even though clk c was enabled, it isn't running since step3 this means that either any driver that is using a multi-parent clock has to deal with the proper enable/disable of the parents (this is is going to lead to code repetition, and I bet people will get it badly wrong). I belive the core of the clock code should deal with this, since otherwise we end up with the situation of the same code being repeated throughout the kernel. > +static inline struct clk *clk_get_parent(struct clk *clk) > +{ > + if (clk->ops->get_parent) > + return clk->ops->get_parent(clk); > + return ERR_PTR(-ENOSYS); > +} > > +#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; > > /** > - * clk_get - lookup and obtain a reference to a clock producer. > - * @dev: device for clock "consumer" > - * @id: clock comsumer ID > - * > - * Returns a struct clk corresponding to the clock producer, or > - * valid IS_ERR() condition containing errno. The implementation > - * uses @dev and @id to determine the clock consumer, and thereby ~> - * the clock producer. (IOW, @id may be identical strings, but > - * clk_get may return different clock producers depending on @dev.) > - * > - * Drivers must assume that the clock source is not enabled. > - * > - * clk_get should not be called from within interrupt context. > - */ > -struct clk *clk_get(struct device *dev, const char *id); > - > -/** > * clk_enable - inform the system when the clock source should be running. > * @clk: clock source > * > @@ -83,12 +173,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 > @@ -125,6 +209,27 @@ int clk_set_parent(struct clk *clk, struct clk *parent); > */ > struct clk *clk_get_parent(struct clk *clk); > > +#endif /* !CONFIG_USE_COMMON_STRUCT_CLK */ > + > +struct device; > + > +/** > + * clk_get - lookup and obtain a reference to a clock producer. > + * @dev: device for clock "consumer" > + * @id: clock comsumer ID > + * > + * Returns a struct clk corresponding to the clock producer, or > + * valid IS_ERR() condition containing errno. The implementation > + * uses @dev and @id to determine the clock consumer, and thereby > + * the clock producer. (IOW, @id may be identical strings, but > + * clk_get may return different clock producers depending on @dev.) > + * > + * Drivers must assume that the clock source is not enabled. > + * > + * clk_get should not be called from within interrupt context. ~~> + */ > +struct clk *clk_get(struct device *dev, const char *id); > + > /** > * clk_get_sys - get a clock based upon the device name > * @dev_id: device name > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo(a)vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- Ben (ben(a)fluff.org, http://www.fluff.org/) 'a smiley only costs 4 bytes' -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |