| Message ID | 1321926047-14211-5-git-send-email-mturquette@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Tuesday 22 November 2011, Mike Turquette wrote: > +static void clk_hw_gate_set_bit(struct clk *clk) > +{ > + struct clk_hw_gate *gate = to_clk_hw_gate(clk); > + u32 reg; > + > + reg = __raw_readl(gate->reg); > + reg |= BIT(gate->bit_idx); > + __raw_writel(reg, gate->reg); > +} You cannot rely on __raw_readl() to do the right thing, especially in architecture independent code. The safe (but slightly inefficient) solution would be readl/writel. For ARM-only code, it would be best to use readl_relaxed()/writel_relaxed(), but most architectures do not implement that. We can probably add a set of helpers in asm-generic/ to define them to the default functions, like "#define readl_relaxed(x) readl(x)", which I think is a good idea anyway. Arnd
On Tue, 2011-11-22 at 13:11 +0000, Arnd Bergmann wrote: > On Tuesday 22 November 2011, Mike Turquette wrote: > > +static void clk_hw_gate_set_bit(struct clk *clk) > > +{ > > + struct clk_hw_gate *gate = to_clk_hw_gate(clk); > > + u32 reg; > > + > > + reg = __raw_readl(gate->reg); > > + reg |= BIT(gate->bit_idx); > > + __raw_writel(reg, gate->reg); > > +} > > You cannot rely on __raw_readl() to do the right thing, especially > in architecture independent code. The safe (but slightly inefficient) > solution would be readl/writel. For ARM-only code, it would be best > to use readl_relaxed()/writel_relaxed(), but most architectures do > not implement that. We can probably add a set of helpers in asm-generic/ > to define them to the default functions, like "#define readl_relaxed(x) > readl(x)", which I think is a good idea anyway. > readl/writel won't work for big endian CPU when the registers are on a bus that does the endian swabbing in hardware. --Mark
On Tuesday 22 November 2011, Mark Salter wrote: > > On Tue, 2011-11-22 at 13:11 +0000, Arnd Bergmann wrote: > > On Tuesday 22 November 2011, Mike Turquette wrote: > > > +static void clk_hw_gate_set_bit(struct clk *clk) > > > +{ > > > + struct clk_hw_gate *gate = to_clk_hw_gate(clk); > > > + u32 reg; > > > + > > > + reg = __raw_readl(gate->reg); > > > + reg |= BIT(gate->bit_idx); > > > + __raw_writel(reg, gate->reg); > > > +} > > > > You cannot rely on __raw_readl() to do the right thing, especially > > in architecture independent code. The safe (but slightly inefficient) > > solution would be readl/writel. For ARM-only code, it would be best > > to use readl_relaxed()/writel_relaxed(), but most architectures do > > not implement that. We can probably add a set of helpers in asm-generic/ > > to define them to the default functions, like "#define readl_relaxed(x) > > readl(x)", which I think is a good idea anyway. > > > > readl/writel won't work for big endian CPU when the registers are on a > bus that does the endian swabbing in hardware. That statement doesn't make any sense. You obviously have to specify the bit index in a way that works with the driver implementation and with the hardware. __raw_readl has an unspecified endianess, which is normally the same as the register endianess of the CPU (assuming a memory-mapped bus), which means you have to do extra work if the register layout is independent of the CPU endianess, which is about as common as MMIO registers defined as being the same endianes as the CPU in bi-endian implementations. Considering that hardware makers cannot agree on how to count bits (IBM calls the MSB bit 0 on big-endian systems), there is no way to please everyone, though you could debate about what the clearest semantics are that we should define. IMHO it would be nicer to use a bit-mask in bus-endian notation, e.g. reg = readl(gate->reg); reg |= le32_to_cpu(gate->bit_mask); writel(reg, gate->reg); but there are other ways to do this. The only thing that I would definitely ask for is having the interface clearly documented as being one of cpu-endian, bus-endian, fixed-endian or having the endianess specified in the device definition (device tree or platform data). Note that I don't object to adding a new cpu-endian mmio accessor, which has been discussed repeatedly in the past. It's just that this accessor does not exist, and using __raw_readl as a substitute causes additional problems. Arnd
On Mon, Nov 21, 2011 at 05:40:46PM -0800, Mike Turquette wrote: > Many platforms support simple gateable clks and fixed-rate clks that > should not be re-implemented by every platform. > > This patch introduces a gateable clk with a common programming model of > gate control via a write of 1 bit to a register. Both set-to-enable and > clear-to-enable are supported. > > Also introduced is a fixed-rate clk which has no reprogrammable aspects. > > The purpose of both types of clks is documented in drivers/clk/basic.c. > What I have seen is drivers/clk/clk-basic.c. > TODO: add support for a simple divider, simple mux and a dummy clk for > stubbing out platform support. > > Based on original patch by Jeremy Kerr contribution by Jamie Iles. > > Signed-off-by: Mike Turquette <mturquette@linaro.org> > --- > drivers/clk/Kconfig | 7 ++ > drivers/clk/Makefile | 5 +- > drivers/clk/clk-basic.c | 208 +++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/clk.h | 35 ++++++++ > 4 files changed, 253 insertions(+), 2 deletions(-) > create mode 100644 drivers/clk/clk-basic.c [...] > +int clk_register_gate(struct device *dev, const char *name, unsigned long flags, > + struct clk *fixed_parent, void __iomem *reg, u8 bit_idx, > + int set_to_enable) > +{ > + struct clk_hw_gate *gclk; > + struct clk *clk; > + > + gclk = kmalloc(sizeof(struct clk_hw_gate), GFP_KERNEL); > + > + if (!gclk) { > + pr_err("%s: could not allocate gated clk\n", __func__); > + return -ENOMEM; > + } > + > + clk = &gclk->clk; > + > + /* struct clk_hw_gate assignments */ > + gclk->fixed_parent = fixed_parent; > + gclk->reg = reg; > + gclk->bit_idx = bit_idx; > + > + /* struct clk assignments */ > + clk->name = name; > + clk->flags = flags; > + > + if (set_to_enable) > + clk->ops = &clk_hw_gate_set_enable_ops; > + else > + clk->ops = &clk_hw_gate_set_disable_ops; > + > + clk_init(NULL, clk); > + > + return 0; The device tree support needs to get this 'struct clk *', so we may want to have all these registering functions return the 'clk'. > +}
One comment was missed. On Mon, Nov 21, 2011 at 05:40:46PM -0800, Mike Turquette wrote: [...] > +struct clk_hw_ops clk_hw_gate_set_enable_ops = { const? > + .enable = clk_hw_gate_enable_set, > + .disable = clk_hw_gate_disable_clear, > + .recalc_rate = clk_hw_gate_recalc_rate, > + .get_parent = clk_hw_gate_get_parent, > +}; > +EXPORT_SYMBOL_GPL(clk_hw_gate_set_enable_ops); > + > +static int clk_hw_gate_enable_clear(struct clk *clk) > +{ > + clk_hw_gate_clear_bit(clk); > + > + return 0; > +} > + > +static void clk_hw_gate_disable_set(struct clk *clk) > +{ > + clk_hw_gate_set_bit(clk); > +} > + > +struct clk_hw_ops clk_hw_gate_set_disable_ops = { ditto Regards, Shawn > + .enable = clk_hw_gate_enable_clear, > + .disable = clk_hw_gate_disable_set, > + .recalc_rate = clk_hw_gate_recalc_rate, > + .get_parent = clk_hw_gate_get_parent, > +}; > +EXPORT_SYMBOL_GPL(clk_hw_gate_set_disable_ops);
On Sat, Nov 26, 2011 at 5:48 AM, Shawn Guo <shawn.guo@freescale.com> wrote: > On Mon, Nov 21, 2011 at 05:40:46PM -0800, Mike Turquette wrote: >> Many platforms support simple gateable clks and fixed-rate clks that >> should not be re-implemented by every platform. >> >> This patch introduces a gateable clk with a common programming model of >> gate control via a write of 1 bit to a register. Both set-to-enable and >> clear-to-enable are supported. >> >> Also introduced is a fixed-rate clk which has no reprogrammable aspects. >> >> The purpose of both types of clks is documented in drivers/clk/basic.c. >> > What I have seen is drivers/clk/clk-basic.c. Will fix in v4. >> +int clk_register_gate(struct device *dev, const char *name, unsigned long flags, >> + struct clk *fixed_parent, void __iomem *reg, u8 bit_idx, >> + int set_to_enable) >> +{ >> + struct clk_hw_gate *gclk; >> + struct clk *clk; >> + >> + gclk = kmalloc(sizeof(struct clk_hw_gate), GFP_KERNEL); >> + >> + if (!gclk) { >> + pr_err("%s: could not allocate gated clk\n", __func__); >> + return -ENOMEM; >> + } >> + >> + clk = &gclk->clk; >> + >> + /* struct clk_hw_gate assignments */ >> + gclk->fixed_parent = fixed_parent; >> + gclk->reg = reg; >> + gclk->bit_idx = bit_idx; >> + >> + /* struct clk assignments */ >> + clk->name = name; >> + clk->flags = flags; >> + >> + if (set_to_enable) >> + clk->ops = &clk_hw_gate_set_enable_ops; >> + else >> + clk->ops = &clk_hw_gate_set_disable_ops; >> + >> + clk_init(NULL, clk); >> + >> + return 0; > > The device tree support needs to get this 'struct clk *', so we may > want to have all these registering functions return the 'clk'. Thanks for the input. Truthfully I'm very DT-ignorant so I'm happy to reshape any APIs for that topic. Hope to fix that soon. Thanks for reviewing, Mike
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index adc0586..ba7eb8c 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -12,3 +12,10 @@ config HAVE_CLK_PREPARE config GENERIC_CLK bool select HAVE_CLK_PREPARE + +config GENERIC_CLK_BASIC + bool "Basic clock definitions" + depends on GENERIC_CLK + help + Allow use of basic, single-function clock types. These + common definitions can be used across many platforms. diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 570d5b9..68b20a1 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -1,3 +1,4 @@ -obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o -obj-$(CONFIG_GENERIC_CLK) += clk.o +obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o +obj-$(CONFIG_GENERIC_CLK) += clk.o +obj-$(CONFIG_GENERIC_CLK_BASIC) += clk-basic.o diff --git a/drivers/clk/clk-basic.c b/drivers/clk/clk-basic.c new file mode 100644 index 0000000..c039f94 --- /dev/null +++ b/drivers/clk/clk-basic.c @@ -0,0 +1,208 @@ +/* + * Copyright (C) 2010-2011 Canonical Ltd <jeremy.kerr@canonical.com> + * Copyright (C) 2011 Linaro Ltd <mturquette@linaro.org> + * + * 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. + * + * Basic single-function clk implementations + */ + +/* TODO add basic divider clk, basic mux clk and dummy clk */ + +#include <linux/clk.h> +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/io.h> + +/* + * NOTE: all of the basic clks here are just that: single-function + * simple clks. One assumption in their implementation is that existing + * locking mechanisms (prepare_mutex and enable_spinlock) are enough to + * prevent race conditions during register accesses. If this is not + * true for your platform then please implement your own version of + * these clks which take such issues into account. + */ + +#define to_clk_hw_gate(ck) container_of(ck, struct clk_hw_gate, clk) +#define to_clk_hw_fixed(ck) container_of(ck, struct clk_hw_fixed, clk) + +/** + * DOC: basic gatable clock which can gate and ungate it's ouput + * + * Traits of this clock: + * prepare - clk_prepare & clk_unprepare do nothing + * enable - clk_enable and clk_disable are functional & control gating + * rate - inherits rate from parent. No clk_set_rate support + * parent - fixed parent. No clk_set_parent support + * + * note: parent should not be NULL for this clock, but we check because we're + * paranoid + */ + +static unsigned long clk_hw_gate_recalc_rate(struct clk *clk) +{ + if (clk->parent) + return clk->parent->rate; + else + return 0; +} + +static struct clk *clk_hw_gate_get_parent(struct clk *clk) +{ + return to_clk_hw_gate(clk)->fixed_parent; +} + +static void clk_hw_gate_set_bit(struct clk *clk) +{ + struct clk_hw_gate *gate = to_clk_hw_gate(clk); + u32 reg; + + reg = __raw_readl(gate->reg); + reg |= BIT(gate->bit_idx); + __raw_writel(reg, gate->reg); +} + +static void clk_hw_gate_clear_bit(struct clk *clk) +{ + struct clk_hw_gate *gate = to_clk_hw_gate(clk); + u32 reg; + + reg = __raw_readl(gate->reg); + reg &= ~BIT(gate->bit_idx); + __raw_writel(reg, gate->reg); +} + +static int clk_hw_gate_enable_set(struct clk *clk) +{ + clk_hw_gate_set_bit(clk); + + return 0; +} + +static void clk_hw_gate_disable_clear(struct clk *clk) +{ + clk_hw_gate_clear_bit(clk); +} + +struct clk_hw_ops clk_hw_gate_set_enable_ops = { + .enable = clk_hw_gate_enable_set, + .disable = clk_hw_gate_disable_clear, + .recalc_rate = clk_hw_gate_recalc_rate, + .get_parent = clk_hw_gate_get_parent, +}; +EXPORT_SYMBOL_GPL(clk_hw_gate_set_enable_ops); + +static int clk_hw_gate_enable_clear(struct clk *clk) +{ + clk_hw_gate_clear_bit(clk); + + return 0; +} + +static void clk_hw_gate_disable_set(struct clk *clk) +{ + clk_hw_gate_set_bit(clk); +} + +struct clk_hw_ops clk_hw_gate_set_disable_ops = { + .enable = clk_hw_gate_enable_clear, + .disable = clk_hw_gate_disable_set, + .recalc_rate = clk_hw_gate_recalc_rate, + .get_parent = clk_hw_gate_get_parent, +}; +EXPORT_SYMBOL_GPL(clk_hw_gate_set_disable_ops); + +int clk_register_gate(struct device *dev, const char *name, unsigned long flags, + struct clk *fixed_parent, void __iomem *reg, u8 bit_idx, + int set_to_enable) +{ + struct clk_hw_gate *gclk; + struct clk *clk; + + gclk = kmalloc(sizeof(struct clk_hw_gate), GFP_KERNEL); + + if (!gclk) { + pr_err("%s: could not allocate gated clk\n", __func__); + return -ENOMEM; + } + + clk = &gclk->clk; + + /* struct clk_hw_gate assignments */ + gclk->fixed_parent = fixed_parent; + gclk->reg = reg; + gclk->bit_idx = bit_idx; + + /* struct clk assignments */ + clk->name = name; + clk->flags = flags; + + if (set_to_enable) + clk->ops = &clk_hw_gate_set_enable_ops; + else + clk->ops = &clk_hw_gate_set_disable_ops; + + clk_init(NULL, clk); + + return 0; +} + +/* + * DOC: basic fixed-rate clock that cannot gate + * + * Traits of this clock: + * prepare - clock never gates. clk_prepare & clk_unprepare do nothing + * enable - clock never gates. clk_enable & clk_disable do nothing + * rate - rate is always a fixed value. No clk_set_rate support + * parent - fixed parent. No clk_set_parent support + * + * note: parent can be NULL, which implies that this clock is a root clock. + */ + +static unsigned long clk_hw_fixed_recalc_rate(struct clk *clk) +{ + return to_clk_hw_fixed(clk)->fixed_rate; +} + +static struct clk *clk_hw_fixed_get_parent(struct clk *clk) +{ + return to_clk_hw_fixed(clk)->fixed_parent; +} + +struct clk_hw_ops clk_hw_fixed_ops = { + .recalc_rate = clk_hw_fixed_recalc_rate, + .get_parent = clk_hw_fixed_get_parent, +}; +EXPORT_SYMBOL_GPL(clk_hw_fixed_ops); + +int clk_register_fixed(struct device *dev, const char *name, + unsigned long flags, struct clk *fixed_parent, + unsigned long fixed_rate) +{ + struct clk_hw_fixed *fclk; + struct clk *clk; + + fclk = kmalloc(sizeof(struct clk_hw_fixed), GFP_KERNEL); + + if (!fclk) { + pr_err("%s: could not allocate fixed clk\n", __func__); + return -ENOMEM; + } + + clk = &fclk->clk; + + /* struct clk_hw_fixed assignments */ + fclk->fixed_parent = fixed_parent; + fclk->fixed_rate = fixed_rate; + + /* struct clk assignments */ + clk->name = name; + clk->flags = flags; + clk->ops = &clk_hw_fixed_ops; + + clk_init(NULL, clk); + + return 0; +} diff --git a/include/linux/clk.h b/include/linux/clk.h index 3b0eb3f..8ed354a 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -129,6 +129,41 @@ struct clk_hw_ops { int (*set_rate)(struct clk *clk, unsigned long); }; +/* + * Base clock implementations. Platform clock implementations can use these + * directly, or 'subclass' as approprate + */ + +#ifdef CONFIG_GENERIC_CLK_BASIC + +struct clk_hw_fixed { + struct clk clk; + struct clk *fixed_parent; + unsigned long fixed_rate; +}; + +extern struct clk_hw_ops clk_hw_fixed_ops; + +int clk_register_fixed(struct device *dev, const char *name, + unsigned long flags, struct clk *fixed_parent, + unsigned long fixed_rate); + +struct clk_hw_gate { + struct clk clk; + struct clk *fixed_parent; + void __iomem *reg; + u8 bit_idx; +}; + +extern struct clk_hw_ops clk_hw_gate_set_enable_ops; +extern struct clk_hw_ops clk_hw_gate_set_disable_ops; + +int clk_register_gate(struct device *dev, const char *name, unsigned long flags, + struct clk *fixed_parent, void __iomem *reg, u8 bit_idx, + int set_to_enable); + +#endif + /** * clk_init - initialize the data structures in a struct clk * @dev: device initializing this clk, placeholder for now
Many platforms support simple gateable clks and fixed-rate clks that should not be re-implemented by every platform. This patch introduces a gateable clk with a common programming model of gate control via a write of 1 bit to a register. Both set-to-enable and clear-to-enable are supported. Also introduced is a fixed-rate clk which has no reprogrammable aspects. The purpose of both types of clks is documented in drivers/clk/basic.c. TODO: add support for a simple divider, simple mux and a dummy clk for stubbing out platform support. Based on original patch by Jeremy Kerr contribution by Jamie Iles. Signed-off-by: Mike Turquette <mturquette@linaro.org> --- drivers/clk/Kconfig | 7 ++ drivers/clk/Makefile | 5 +- drivers/clk/clk-basic.c | 208 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk.h | 35 ++++++++ 4 files changed, 253 insertions(+), 2 deletions(-) create mode 100644 drivers/clk/clk-basic.c