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: Ben Dooks on 2 Jun 2010 08:10 On Wed, Jun 02, 2010 at 07:56:44PM +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. > > 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 { > struct clk_operations *ops; > atomic_t enable_count; > }; > > 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. an aside, you might want to just clal these clk_ops to get into the spirit of the original naming. > 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 | 148 +++++++++++++++++++++++++++++++++++--------- > 2 files changed, 124 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..f70137a 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,114 @@ > #ifndef __LINUX_CLK_H > #define __LINUX_CLK_H > > -struct device; > +#include <linux/err.h> > +#include <asm/atomic.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_operations *ops; > + atomic_t enable_count; > +}; > + > +#define DEFINE_CLK(o) \ > + { .ops = &o, .enable_count = ATOMIC_INIT(0) } > + > +struct clk_operations { > + 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 *); > +}; > + > +static inline int clk_enable(struct clk *clk) > +{ > + if (!clk->ops->enable) > + return 0; > + > + if (atomic_inc_return(&clk->enable_count) != 1) > + return 0; > + > + return clk->ops->enable(clk); > +} > + > +static inline void clk_disable(struct clk *clk) > +{ > + if (!clk->ops->disable) > + return; > + > + if (atomic_dec_return(&clk->enable_count) != 0) > + return; > + > + clk->ops->disable(clk); > +} > + > +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); > +} > + > +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; > +} > + > +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 +162,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 +198,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 > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel(a)lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- -- 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 2 Jun 2010 23:30 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; if (enable) { int ret = clk_enable(clk->parent); if (ret) return ret; } reg = raw_readl(foo->some_register); if (enable) reg |= FOO_ENABLE; else reg &= ~FOO_ENABLE; raw_writel(foo->some_register, reg); if (!enable) clk_disable(clk->parent); return 0; } - where most of the function becomes surrounded by "if (enable)" statements. I'm aware that we can turn this into a conditional call of clk_foo_enable or clk_foo_disable, but then we're back to square 1. I also think that the simple case is clearer (if a little more verbose) with separate functions. Also, enable and disable in the external clock API have different return types. > an aside, you might want to just clal these clk_ops to get into the > spirit of the original naming. Either is fine with me - looks like 'ops' is more commonly used: $ git grep -E '^struct \w*operations\s*\{' include/ | wc -l 30 $ git grep -E '^struct \w*ops\s*{' include/ | wc -l 138 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 3 Jun 2010 04:20 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: clk_enable(struct clk *clk) { if (clk->parent) clk_enable(clk->parent) ... } clk_disable(struct clk *clk) { ... if (clk->parent) clk_disable(clk->parent) } I think it is a really bad idea for each implementation to have to worry about this. It sounds like a recipie for people to get wrong, especially if we have a number of these implementations kicking around. > if (enable) { > int ret = clk_enable(clk->parent); > if (ret) > return ret; > } > > reg = raw_readl(foo->some_register); > if (enable) > reg |= FOO_ENABLE; > else > reg &= ~FOO_ENABLE; > > raw_writel(foo->some_register, reg); > > if (!enable) > clk_disable(clk->parent); > > return 0; > } > > - where most of the function becomes surrounded by "if (enable)" statements. > > I'm aware that we can turn this into a conditional call of clk_foo_enable or > clk_foo_disable, but then we're back to square 1. I also think that the simple > case is clearer (if a little more verbose) with separate functions. If we do decided to move the parent control functionality to the clock core, then I would prefer to see the change to a single enable/disable callback. Especially as it fits my current implementations well. As a note, I also left the enable callback in the 'struct clk' instead of in the ops, enable/disable is the most used case of these clock functions, and as such should probably be the easiest to get to. Also, wheras plat-samsung has very few sets of clk_ops sitting about, there are more enable/disable calls, and adding more fields to the clocks to deal with this would add extra space to the kernel. > Also, enable and disable in the external clock API have different return > types. does that really matter? > > an aside, you might want to just clal these clk_ops to get into the > > spirit of the original naming. > > Either is fine with me - looks like 'ops' is more commonly used: My pref. is for less typing. > $ git grep -E '^struct \w*operations\s*\{' include/ | wc -l > 30 > > $ git grep -E '^struct \w*ops\s*{' include/ | wc -l > 138 > > Cheers, > > > Jeremy -- -- 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 06:30 Hi Ben, > clk_enable(struct clk *clk) > { > if (clk->parent) > clk_enable(clk->parent) > ... > } > > clk_disable(struct clk *clk) > { > ... > if (clk->parent) > clk_disable(clk->parent) > } > > I think it is a really bad idea for each implementation to have to worry > about this. It sounds like a recipie for people to get wrong, especially > if we have a number of these implementations kicking around. OK, this would mean adding parent to struct clk: struct clk { struct clk_operations *ops; atomic_t enable_count; struct clk *parent; }; I was originally intending for struct clk to have the absolute minimum of fields, only those necessary for every clock in the system. parent didn't make the cut, as some clocks don't have a parent. However, lets explore this a little - handling parents in the infrastructure code this may simplify the hardware-specific code somewhat. We'd add the parent-handling stuff to the global clk_enable/clk_disable: static inline int clk_enable(struct clk *clk) { int ret; if (atomic_inc(clk->enable_count) != 1)) return 0; if (clk->parent) { ret = clk_enable(clk->parent); if (ret) goto out_dec; } ret = clk->ops->enable(clk) if (!ret) return 0; out_dec: atomic_dec(clk->enable_count); return ret; } The downside here is that the static inline becomes quite bloated, and we can no longer avoid the atomic operation if there is no enable op. I guess we could add a: if (!clk->ops->enable && !clk->parent) return 0; but now were in serious "don't do that as a inline" territory. So we'd be better off making this a proper function. [as an aside, I need to add the atomic_dec to the error path of my current code, will respin a new version. But even then, it's small enough to inline] I think that handling the enable/disable in the hardware-specific op is an acceptable solution. It's only one line of code (or two if you want to check for the presence of a parent first), and is, after all, a hardware-specific property of the clock. So, we either: 1) keep it as is, with the hw-specific code handling parent enable/disable; or 2) add the parent-handling code to the clock infrastructure and move the core API functions out-of-line. > As a note, I also left the enable callback in the 'struct clk' instead > of in the ops, enable/disable is the most used case of these clock > functions, and as such should probably be the easiest to get to. I strongly disagree on this one. I want all of the ops in one place, not scattered around the API. > Also, wheras plat-samsung has very few sets of clk_ops sitting about, > there are more enable/disable calls, and adding more fields to the > clocks to deal with this would add extra space to the kernel. Sorry, I don't think I understand your point here - you're saying that moving the enable/disable callbacks to struct clk will increase the size of the kernel (which is correct, as there are more struct clks than there are struct clk_operations), so doesn't this provide an argument against doing this? > > Also, enable and disable in the external clock API have different return > > types. > > does that really matter? Only if someone expects a failed disable to be detected by the driver. > > Either is fine with me - looks like 'ops' is more commonly used: > My pref. is for less typing. OK, will do this in the next revision. 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: Russell King - ARM Linux on 3 Jun 2010 07:10
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. -- 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/ |