Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks
Hi Mike +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) + How do you suggest handling gated clocks which are already running when calling the register function? On my kirkwood bases system, the bootloader has already turned on a number of clocks. It does not seem right to start messing with clk-enable_count and clk-prepare_count. Could clk_register_gate() have one more parameter, a bool indicating running? The kirkwood mach code keeps a bitmap of which platform_data init functions are called from the board file. In a late_initcall function it then enables and disables clocks as needed. What i was thinking is i can ask the hardware what clocks are already running before i register them and register them as running/not running. Then let the driver probe functions use the API to enable clocks which are really needed. In a late_initcall function, i would then call clk_disable(), clk_unprepare() on clocks which the boot loader started, thus turning them off if no driver has claimed them. Is this how you envisage it working? Thanks Andrew -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks
On Mon, Dec 12, 2011 at 11:47 AM, Andrew Lunn and...@lunn.ch wrote: Hi Mike +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) + How do you suggest handling gated clocks which are already running when calling the register function? On my kirkwood bases system, the bootloader has already turned on a number of clocks. It does not seem right to start messing with clk-enable_count and clk-prepare_count. Could clk_register_gate() have one more parameter, a bool indicating running? I don't like this approach. If the bool for a particular clk is statically defined then it could be wrong (bootloader/kernel mismatch). I've been thinking of adding a clk-ops-init callback in clk_init, which is defined for a platform to use however the author sees fit. There have been a few cases where it would be nice to init a clk only once, when it is registered. That code could also handle detecting if a clk is enabled or not. On the other hand we already have a .get_parent callback which is only good for figuring out which parent a mux clk is using... maybe a .is_enabled or .get_enabled would be a good idea which also served the purpose of dynamically determining whether a clk is disabled or running. In general though I think we should try to keep the solution in the core code, not by having platform code pass in a bool. The kirkwood mach code keeps a bitmap of which platform_data init functions are called from the board file. In a late_initcall function it then enables and disables clocks as needed. What i was thinking is i can ask the hardware what clocks are already running before i register them and register them as running/not running. Then let the driver probe functions use the API to enable clocks which are really needed. In a late_initcall function, i would then call clk_disable(), clk_unprepare() on clocks which the boot loader started, thus turning them off if no driver has claimed them. The problem here is that you're solving the disabled unused clks issue in platform code. I've started to lay out a little groundwork for that with a flag in struct clk: http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=blob;f=include/linux/clk.h;h=3b0eb3f1caf1d6346b62c785b74a648587bfcc7f;hb=586c6e8922a889a2893ba4467bb3d13b471656a9#l35 The idea behind CLK_IGNORE_UNUSED flag on line 35 is that the common clk framework can walk the tree (probably as part of a late_initcall, as you suggested) and disable any clks that aren't already enabled and don't have this flag set. This involves zero platform-specific code, but I haven't gotten around to introducing the feature yet as I'm really trying to minimize footprint for the core code (and I'm not doing a good job of that since it keeps growing). Regards, Mike Is this how you envisage it working? Thanks Andrew -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks
I don't like this approach. If the bool for a particular clk is statically defined then it could be wrong (bootloader/kernel mismatch). I've been thinking of adding a clk-ops-init callback in clk_init, which is defined for a platform to use however the author sees fit. There have been a few cases where it would be nice to init a clk only once, when it is registered. That code could also handle detecting if a clk is enabled or not. On the other hand we already have a .get_parent callback which is only good for figuring out which parent a mux clk is using... maybe a .is_enabled or .get_enabled would be a good idea which also served the purpose of dynamically determining whether a clk is disabled or running. In general though I think we should try to keep the solution in the core code, not by having platform code pass in a bool. Hi Mike How about simply reading the bit in the control register? You are already doing a read/modify/write when enabling/disabling the clock, so it seems reasonably safe to assume the read gives you the current state. For those platforms which this does not work, you could add another flag disabling this read to get the initial state. Andrew -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks
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 mturque...@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'.
+}
--
Regards,
Shawn
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks
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);
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks
On Sat, Nov 26, 2011 at 5:48 AM, Shawn Guo shawn@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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks
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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks
On Tue, 2011-11-22 at 13:11 +, 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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks
On Tuesday 22 November 2011, Mark Salter wrote:
On Tue, 2011-11-22 at 13:11 +, 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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 4/5] clk: basic gateable and fixed-rate clks
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 mturque...@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
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 000..c039f94
--- /dev/null
+++ b/drivers/clk/clk-basic.c
@@ -0,0 +1,208 @@
+/*
+ * Copyright (C) 2010-2011 Canonical Ltd jeremy.k...@canonical.com
+ * Copyright (C) 2011 Linaro Ltd mturque...@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
