Re: [PATCH v2 1/7] clk: Add a generic clock infrastructure

From: Richard Zhao
Date: Fri Oct 14 2011 - 04:10:42 EST


Hi Mike,

On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote:
> From: Jeremy Kerr <jeremy.kerr@xxxxxxxxxxxxx>
>
> 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, and 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.
>
> The interface is split into two halves:
>
> * struct clk, which is the generic-device-driver interface. This
> provides a set of functions which drivers may use to request
> enable/disable, query or manipulate in a hardware-independent manner.
>
> * struct clk_hw and struct clk_hw_ops, which is the hardware-specific
> interface. Clock drivers implement the ops, which allow the core
> clock code to implement the generic 'struct clk' API.
>
> 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_GENERIC_CLK. In this case, the clock infrastructure consists of a
> common, opaque struct clk, and a set of clock operations (defined per
> type of clock):
>
> struct clk_hw_ops {
> int (*prepare)(struct clk_hw *);
> void (*unprepare)(struct clk_hw *);
> int (*enable)(struct clk_hw *);
> void (*disable)(struct clk_hw *);
> unsigned long (*recalc_rate)(struct clk_hw *);
> int (*set_rate)(struct clk_hw *,
> unsigned long, unsigned long *);
> long (*round_rate)(struct clk_hw *, unsigned long);
> int (*set_parent)(struct clk_hw *, struct clk *);
> struct clk * (*get_parent)(struct clk_hw *);
> };
>
> Platform clock code can register a clock through clk_register, passing a
> set of operations, and a pointer to hardware-specific data:
>
> struct clk_hw_foo {
> struct clk_hw clk;
> void __iomem *enable_reg;
> };
>
> #define to_clk_foo(c) offsetof(c, clk_hw_foo, clk)
>
> static int clk_foo_enable(struct clk_hw *clk)
> {
> struct clk_foo *foo = to_clk_foo(clk);
> raw_writeb(foo->enable_reg, 1);
> return 0;
> }
>
> struct clk_hw_ops clk_foo_ops = {
> .enable = clk_foo_enable,
> };
>
> And in the platform initialisation code:
>
> struct clk_foo my_clk_foo;
>
> void init_clocks(void)
> {
> my_clk_foo.enable_reg = ioremap(...);
>
> clk_register(&clk_foo_ops, &my_clk_foo, NULL);
> }
>
> Changes from Thomas Gleixner <tglx@xxxxxxxxxxxxx>.
>
> The common clock definitions are based on a development patch from Ben
> Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>.
>
> TODO:
>
> * We don't keep any internal reference to the clock topology at present.
>
> Signed-off-by: Jeremy Kerr <jeremy.kerr@xxxxxxxxxxxxx>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Signed-off-by: Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Mike Turquette <mturquette@xxxxxx>
> ---
> Changes since v1:
> Create a dummy clk_unregister and prototype/document it and clk_register
> Constify struct clk_hw_ops
> Remove spinlock.h header, include kernel.h
> Use EOPNOTSUPP instead of ENOTSUPP
> Add might_sleep to clk_prepare/clk_unprepare stubs
> Properly init children hlist and child_node
> Whitespace and typo fixes
>
> drivers/clk/Kconfig | 3 +
> drivers/clk/Makefile | 1 +
> drivers/clk/clk.c | 232 ++++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/clk/clkdev.c | 7 ++
> include/linux/clk.h | 140 +++++++++++++++++++++++++++---
> 5 files changed, 371 insertions(+), 12 deletions(-)
> create mode 100644 drivers/clk/clk.c
>
[...]
> +static void __clk_disable(struct clk *clk)
> +{
> + if (!clk)
> + return;
> +
> + if (WARN_ON(clk->enable_count == 0))
> + return;
> +
> + if (--clk->enable_count > 0)
> + return;
> +
> + if (clk->ops->disable)
> + clk->ops->disable(clk->hw);
> + __clk_disable(clk->parent);
> +}
> +
> +void clk_disable(struct clk *clk)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&enable_lock, flags);
> + __clk_disable(clk);
> + spin_unlock_irqrestore(&enable_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(clk_disable);
> +
> +static int __clk_enable(struct clk *clk)
> +{
> + int ret;
> +
> + if (!clk)
> + return 0;
> +
> + if (WARN_ON(clk->prepare_count == 0))
> + return -ESHUTDOWN;
> +
> +
> + if (clk->enable_count == 0) {
> + ret = __clk_enable(clk->parent);
> + if (ret)
> + return ret;
> +
> + if (clk->ops->enable) {
> + ret = clk->ops->enable(clk->hw);
> + if (ret) {
> + __clk_disable(clk->parent);
> + return ret;
> + }
> + }
> + }
> +
> + clk->enable_count++;
> + return 0;
> +}
Could you expose __clk_enable/__clk_disable? I find it hard to implement
clk group. clk group means, when a major clk enable/disable, it want a set
of other clks enable/disable accordingly.
> +
> +int clk_enable(struct clk *clk)
> +{
> + unsigned long flags;
> + int ret;
> +
> + spin_lock_irqsave(&enable_lock, flags);
> + ret = __clk_enable(clk);
> + spin_unlock_irqrestore(&enable_lock, flags);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(clk_enable);
> +
[...]

Thanks
Richard

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/