[PATCH v3 4/5] clk: basic gateable and fixed-rate clks

[PATCH v3 4/5] clk: basic gateable and fixed-rate clks

[Mike Turquette]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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 0000000..c039f94
--- /dev/null
+++ b/drivers/clk/clk-basic.c
@@ -0,0 +1,208 @@
+/*
+ * Copyright (C) 2010-2011 Canonical Ltd <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
+ * Copyright (C) 2011 Linaro Ltd <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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
-- 
1.7.4.1

Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks

[Arnd Bergmann]

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

Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks

[Mark Salter]

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

Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks

[Arnd Bergmann]

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

Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks

[Shawn Guo]

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'.

> +}

-- 
Regards,
Shawn

Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks

[Shawn Guo]

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);

Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks

[Turquette, Mike]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@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

[Andrew Lunn]

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

Re: [PATCH v3 4/5] clk: basic gateable and fixed-rate clks

[Turquette, Mike]

On Mon, Dec 12, 2011 at 11:47 AM, Andrew Lunn <andrew@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 majordomo@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

[Andrew Lunn]

> 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