diff mbox

[1/4] clk: Add a generic clock infrastructure

Message ID 1305876469.326305.518470530485.1.gpush@pororo (mailing list archive)
State New, archived
Headers show

Commit Message

Jeremy Kerr May 20, 2011, 7:27 a.m. UTC
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@linutronix.de>.

The common clock definitions are based on a development patch from Ben
Herrenschmidt <benh@kernel.crashing.org>.

TODO:

 * We don't keep any internal reference to the clock topology at present.

Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 drivers/clk/Kconfig  |    3 
 drivers/clk/Makefile |    1 
 drivers/clk/clk.c    |  229 +++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clkdev.c |    7 +
 include/linux/clk.h  |  102 +++++++++++++++++--
 5 files changed, 332 insertions(+), 10 deletions(-)

Comments

Colin Cross May 23, 2011, 11:55 p.m. UTC | #1
On Fri, May 20, 2011 at 12:27 AM, Jeremy Kerr <jeremy.kerr@canonical.com> 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, 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 *);
>  };

You might want to split these into three separate structs, for mux
ops, rate ops, and gate ops.  That way, multiple building blocks (a
gate and a divider, for example), can be easily combined into a single
clock node.  Also, an init op that reads the clock tree state from the
hardware has been very useful on Tegra - there are often clocks that
you can't or won't change, and being able to view their state as
configured by the bootloader, and base other clocks off of them, is
helpful.  It also allows you to turn off clocks that are enabled by
the bootloader, but never enabled by the kernel (enabled=1,
enable_count=0).

Also, OMAP currently has a second level of global ops that are called
before the per-clk ops, which handle the common parts of clock enable
(the "clockdomains" part), which I modeled as each clk having a
clk_chip, and the clk_chip having some ops.  It does add a lot of
complexity to the error handling, and OMAP doesn't have very many op
implementations, so unless other architectures need it, I don't see a
problem pushing those down into each op implementation.

> 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@linutronix.de>.
>
> The common clock definitions are based on a development patch from Ben
> Herrenschmidt <benh@kernel.crashing.org>.
>
> TODO:
>
>  * We don't keep any internal reference to the clock topology at present.
>
> Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>
> ---
>  drivers/clk/Kconfig  |    3
>  drivers/clk/Makefile |    1
>  drivers/clk/clk.c    |  229 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/clkdev.c |    7 +
>  include/linux/clk.h  |  102 +++++++++++++++++--
>  5 files changed, 332 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 4168c88..e611e34 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -2,3 +2,6 @@
>  config CLKDEV_LOOKUP
>        bool
>        select HAVE_CLK
> +
> +config GENERIC_CLK
> +       bool
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 07613fa..570d5b9 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -1,2 +1,3 @@
>
>  obj-$(CONFIG_CLKDEV_LOOKUP)    += clkdev.o
> +obj-$(CONFIG_GENERIC_CLK)      += clk.o
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> new file mode 100644
> index 0000000..ad90a90
> --- /dev/null
> +++ b/drivers/clk/clk.c
> @@ -0,0 +1,229 @@
> +/*
> + * Copyright (C) 2010-2011 Canonical Ltd <jeremy.kerr@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
> + * published by the Free Software Foundation.
> + *
> + * Standard functionality for the common clock API.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +struct clk {
> +       const char              *name;
> +       struct clk_hw_ops       *ops;
> +       struct clk_hw           *hw;
> +       unsigned int            enable_count;
> +       unsigned int            prepare_count;
> +       struct clk              *parent;

Storing the list of possible parents at this level can help abstract
some common code from mux ops if you pass the index into the list of
the new parent into the op - most mux ops only need to know which of
their mux inputs needs to be enabled.

> +       unsigned long           rate;

If you add an init op, an enabled flag here is also useful to track
whether the bootloader left the clock on, which allows turning off
unnecessary clocks.

I think you also need a list of current children of the clock to allow
propagating rate changes from parent to children.

> +};
> +
> +static DEFINE_SPINLOCK(enable_lock);
> +static DEFINE_MUTEX(prepare_lock);

There was some discussion earlier of per-tree locking instead of
global locking.  I have a clock tree that does per-tree locking, as
well as runtime addition of clocks to the tree (for example, a codec
chip that is complex enough to treat the internal clocks using the clk
api, but fed off a clock output from the main SoC), but it adds a ton
of complexity to the locking.

> +
> +static void __clk_unprepare(struct clk *clk)
> +{
> +       if (!clk)
> +               return;
> +
> +       if (WARN_ON(clk->prepare_count == 0))
> +               return;
> +
> +       if (--clk->prepare_count > 0)
> +               return;
> +
> +       WARN_ON(clk->enable_count > 0);
> +
> +       if (clk->ops->unprepare)
> +               clk->ops->unprepare(clk->hw);
> +
> +       __clk_unprepare(clk->parent);
> +}
Are there any cases where the unlocked versions of the clk calls need
to be exposed to the ops implementations?  For example, a set_rate op
may want to call clk_set_parent on itself to change its parent to a
better source, and then set its rate.  It would need to call
__clk_set_parent to avoid deadlocking on the prepare_lock.

> +void clk_unprepare(struct clk *clk)
> +{
> +       mutex_lock(&prepare_lock);
> +       __clk_unprepare(clk);
> +       mutex_unlock(&prepare_lock);
> +}
> +EXPORT_SYMBOL_GPL(clk_unprepare);
> +
> +static int __clk_prepare(struct clk *clk)
> +{
> +       int ret = 0;
> +
> +       if (!clk)
> +               return 0;
> +
> +       if (clk->prepare_count == 0) {
> +               ret = __clk_prepare(clk->parent);
> +               if (ret)
> +                       return ret;
> +
> +               if (clk->ops->prepare) {
> +                       ret = clk->ops->prepare(clk->hw);
> +                       if (ret) {
> +                               __clk_unprepare(clk->parent);
> +                               return ret;
> +                       }
> +               }
> +       }
> +
> +       clk->prepare_count++;
> +
> +       return 0;
> +}
> +
> +int clk_prepare(struct clk *clk)
> +{
> +       int ret;
> +
> +       mutex_lock(&prepare_lock);
> +       ret = __clk_prepare(clk);
> +       mutex_unlock(&prepare_lock);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(clk_prepare);
> +
> +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;
> +}
> +
> +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);
> +
> +unsigned long clk_get_rate(struct clk *clk)
> +{
> +       if (!clk)
> +               return 0;
> +       return clk->rate;
> +}
> +EXPORT_SYMBOL_GPL(clk_get_rate);
> +
> +long clk_round_rate(struct clk *clk, unsigned long rate)
> +{
> +       if (clk && clk->ops->round_rate)
> +               return clk->ops->round_rate(clk->hw, rate);
> +       return rate;
> +}
> +EXPORT_SYMBOL_GPL(clk_round_rate);

I think you should hold the prepare mutex around calls to
clk_round_rate, which will allow some code simplification similar to
what Russell suggested in another thread.  If you hold the mutex here,
as well as in clk_set_rate, and you call the round_rate op before the
set_rate op in clk_set_rate, op implementers can compute the rate in
their round_rate op, and save the register values needed to get that
rate in private temporary storage.  The set_rate op can then assume
that those register values are correct, because the lock is still
held, and just write them.  That moves all the computation to one
place, and it only needs to run once.

> +int clk_set_rate(struct clk *clk, unsigned long rate)
> +{
> +       /* not yet implemented */
> +       return -ENOSYS;
> +}
> +EXPORT_SYMBOL_GPL(clk_set_rate);

Here's my implementation, slightly modified and untested to work with
your variable names (sorry if whitespace gets mangled):

/*
 * called on a clock when it or any of its ancestors change parents or rates
 * must be called with prepare_lock held
 */
static void _propagate_rate(struct clk *clk)
{
	struct clk *child;

	if (clk->ops->recalc_rate)
		clk->rate = clk->ops->recalc_rate(clk);
	else if (clk->parent)
		clk->rate = clk->parent->rate;

	list_for_each_entry(child, &clk->children, parent_node)
		_propagate_rate(child);
}

long __clk_round_rate(struct clk *clk, unsigned long rate)
{
	if (clk->ops->round_rate)
		return clk->ops->round_rate(clk, rate);

	return clk->rate;
}

int __clk_set_rate(struct clk *clk, unsigned long rate)
{
	long new_rate;
	int ret;

	if (!clk->ops->set_rate)
		return -ENOSYS;

	new_rate = __clk_round_rate(clk, rate);
	if (new_rate < 0)
		return new_rate;

	ret = clk->ops->set_rate(clk, new_rate);
	if (ret)
		return ret;

	clk->rate = new_rate;

	_propagate_rate(clk);

	return 0;
}

int clk_set_rate(struct clk *clk, unsigned long rate)
{
	int ret;

	mutex_lock(&prepare_lock);

	ret = __clk_set_rate(clk, rate);

	mutex_unlock(prepare_lock);

	return ret;
}


> +struct clk *clk_get_parent(struct clk *clk)
> +{
> +       if (!clk)
> +               return NULL;
> +
> +       return clk->parent;
> +}
> +EXPORT_SYMBOL_GPL(clk_get_parent);
> +
> +int clk_set_parent(struct clk *clk, struct clk *parent)
> +{
> +       /* not yet implemented */
> +       return -ENOSYS;
> +}
> +EXPORT_SYMBOL_GPL(clk_set_parent);
> +
> +struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
> +               const char *name)
> +{
> +       struct clk *clk;
> +
> +       clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> +       if (!clk)
> +               return NULL;
> +
> +       clk->name = name;
> +       clk->ops = ops;
> +       clk->hw = hw;
> +       hw->clk = clk;
> +
> +       /* Query the hardware for parent and initial rate */
> +
> +       if (clk->ops->get_parent)
> +               /* We don't to lock against prepare/enable here, as
> +                * the clock is not yet accessible from anywhere */
> +               clk->parent = clk->ops->get_parent(clk->hw);
> +
> +       if (clk->ops->recalc_rate)
> +               clk->rate = clk->ops->recalc_rate(clk->hw);
> +
> +
> +       return clk;
> +}
> +EXPORT_SYMBOL_GPL(clk_register);

If you are requiring clk's parents (or possible parents?) to be
registered before clk, you could put the clk_lookup struct inside the
clk struct and call clkdev_add from clk_register, saving some
boilerplate in the platforms.

> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index 6db161f..e2a9719 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -23,6 +23,13 @@
>  static LIST_HEAD(clocks);
>  static DEFINE_MUTEX(clocks_mutex);
>
> +/* For USE_COMMON_STRUCT_CLK, these are provided in clk.c, but not exported
> + * through other headers; we don't want them used anywhere but here. */
> +#ifdef CONFIG_USE_COMMON_STRUCT_CLK
> +extern int __clk_get(struct clk *clk);
> +extern void __clk_put(struct clk *clk);
> +#endif
> +
>  /*
>  * Find the correct struct clk for the device and connection ID.
>  * We do slightly fuzzy matching here:
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 1d37f42..93ff870 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-2011 Jeremy Kerr <jeremy.kerr@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,17 +12,103 @@
>  #ifndef __LINUX_CLK_H
>  #define __LINUX_CLK_H
>
> +#include <linux/err.h>
> +#include <linux/spinlock.h>
> +
>  struct device;
>
> -/*
> - * The base API.
> +struct clk;
> +
> +#ifdef CONFIG_GENERIC_CLK
> +
> +struct clk_hw {
> +       struct clk *clk;
> +};
> +
> +/**
> + * struct clk_hw_ops -  Callback operations for hardware clocks; these are to
> + * be provided by the clock implementation, and will be called by drivers
> + * through the clk_* API.
> + *
> + * @prepare:   Prepare the clock for enabling. This must not return until
> + *             the clock is fully prepared, and it's safe to call clk_enable.
> + *             This callback is intended to allow clock implementations to
> + *             do any initialisation that may sleep. Called with
> + *             prepare_lock held.
> + *
> + * @unprepare: Release the clock from its prepared state. This will typically
> + *             undo any work done in the @prepare callback. Called with
> + *             prepare_lock held.
> + *
> + * @enable:    Enable the clock atomically. This must not return until the
> + *             clock is generating a valid clock signal, usable by consumer
> + *             devices. Called with enable_lock held. This function must not
> + *             sleep.
> + *
> + * @disable:   Disable the clock atomically. Called with enable_lock held.
> + *             This function must not sleep.
> + *
> + * @recalc_rate        Recalculate the rate of this clock, by quering hardware
> + *             and/or the clock's parent. Called with the global clock mutex
> + *             held. Optional, but recommended - if this op is not set,
> + *             clk_get_rate will return 0.
You need a callback to update the rate when the parent or parent's
rate changes, which I would call recalc_rate, as well as this
init-type call.

> + *
> + * @get_parent Query the parent of this clock; for clocks with multiple
> + *             possible parents, query the hardware for the current
> + *             parent. Currently only called when the clock is first
> + *             registered.
> + *
> + * The clk_enable/clk_disable and clk_prepare/clk_unprepare pairs allow
> + * implementations to split any work between atomic (enable) and sleepable
> + * (prepare) contexts.  If a clock requires sleeping code to be turned on, this
> + * should be done in clk_prepare. Switching that will not sleep should be done
> + * in clk_enable.
> + *
> + * Typically, drivers will call clk_prepare when a clock may be needed later
> + * (eg. when a device is opened), and clk_enable when the clock is actually
> + * required (eg. from an interrupt). Note that clk_prepare *must* have been
> + * called before clk_enable.
> + */
> +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 *);
> +       long            (*round_rate)(struct clk_hw *, unsigned long);
> +       struct clk *    (*get_parent)(struct clk_hw *);
> +};
> +
> +/**
> + * clk_prepare - prepare clock for atomic enabling.
> + *
> + * @clk: The clock to prepare
> + *
> + * Do any possibly sleeping initialisation on @clk, allowing the clock to be
> + * later enabled atomically (via clk_enable). This function may sleep.
>  */
> +int clk_prepare(struct clk *clk);
>
> +/**
> + * clk_unprepare - release clock from prepared state
> + *
> + * @clk: The clock to release
> + *
> + * Do any (possibly sleeping) cleanup on clk. This function may sleep.
> + */
> +void clk_unprepare(struct clk *clk);
> +
> +#else /* !CONFIG_GENERIC_CLK */
>
>  /*
> - * struct clk - an machine class defined object / cookie.
> + * For !CONFIG_GENERIC_CLK, we don't enforce any atomicity
> + * requirements for clk_enable/clk_disable, so the prepare and unprepare
> + * functions are no-ops
>  */
> -struct clk;
> +static inline int clk_prepare(struct clk *clk) { return 0; }
> +static inline void clk_unprepare(struct clk *clk) { }
> +
> +#endif /* !CONFIG_GENERIC_CLK */
>
>  /**
>  * clk_get - lookup and obtain a reference to a clock producer.
> @@ -67,6 +154,7 @@ void clk_disable(struct clk *clk);
>  /**
>  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
>  *               This is only valid once the clock source has been enabled.
> + *               Returns zero if the clock rate is unknown.
>  * @clk: clock source
>  */
>  unsigned long clk_get_rate(struct clk *clk);
> @@ -83,12 +171,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
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Viresh KUMAR May 24, 2011, 4:18 a.m. UTC | #2
On 05/20/2011 12:57 PM, Jeremy Kerr wrote:
> +static DEFINE_SPINLOCK(enable_lock);
> +static DEFINE_MUTEX(prepare_lock);

Probably all clocks can be handled separately, i.e. single lock for all
of them will make system slower. Suppose i want to enable UART's clock
then why should spi code be waiting for the lock.
So, we should have per clk lock.

<...>

> +struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
> +               const char *name)
> +{
> +       struct clk *clk;
> +
> +       clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> +       if (!clk)
> +               return NULL;
> +
> +       clk->name = name;
> +       clk->ops = ops;
> +       clk->hw = hw;
> +       hw->clk = clk;
> +
> +       /* Query the hardware for parent and initial rate */
> +

Can remove this blank line.

> +       if (clk->ops->get_parent)
> +               /* We don't to lock against prepare/enable here, as
> +                * the clock is not yet accessible from anywhere */

Shouldn't we use following style for multi-line comments.
/*
 * ....
 */

> +               clk->parent = clk->ops->get_parent(clk->hw);
> +
> +       if (clk->ops->recalc_rate)
> +               clk->rate = clk->ops->recalc_rate(clk->hw);
> +
> +

Can remove one of these blank lines.
Sascha Hauer May 24, 2011, 7:02 a.m. UTC | #3
On Mon, May 23, 2011 at 04:55:15PM -0700, Colin Cross wrote:
> On Fri, May 20, 2011 at 12:27 AM, Jeremy Kerr <jeremy.kerr@canonical.com> 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, 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 *);
> >  };
> 
> You might want to split these into three separate structs, for mux
> ops, rate ops, and gate ops.  That way, multiple building blocks (a
> gate and a divider, for example), can be easily combined into a single
> clock node.  Also, an init op that reads the clock tree state from the
> hardware has been very useful on Tegra - there are often clocks that
> you can't or won't change, and being able to view their state as
> configured by the bootloader, and base other clocks off of them, is
> helpful.

The clock state is read initially from the hardware with the
recalc_rate/get_parent ops. What do we need an additional init op for?

> It also allows you to turn off clocks that are enabled by
> the bootloader, but never enabled by the kernel (enabled=1,
> enable_count=0).

The enable count indeed is a problem. I don't think an init hook
would be the right solution for this though as this sounds platform
specific. struct clk_hw_ops should be specific to the type of clock
(mux, divider, gate) and should be present only once per type.

For the enable count (and whether a clock should initially be enabled or
not) I can think of something like this:

- A platform/SoC registers all clocks.
- It then calls clk_prepare/enable for all vital core clocks
  (SDRAM, CPU,...). At this time the enable counters are correct.
- Then some hook in the generic clock layer is called. This hook
  iterates over the tree and disables all clocks in hardware which
  have a enable count of 0.

> > +
> > +struct clk {
> > +       const char              *name;
> > +       struct clk_hw_ops       *ops;
> > +       struct clk_hw           *hw;
> > +       unsigned int            enable_count;
> > +       unsigned int            prepare_count;
> > +       struct clk              *parent;
> 
> Storing the list of possible parents at this level can help abstract
> some common code from mux ops if you pass the index into the list of
> the new parent into the op - most mux ops only need to know which of
> their mux inputs needs to be enabled.

Please don't. Only muxes have more than one possible parent, so this
should be handled there.

> 
> > +       unsigned long           rate;
> 
> If you add an init op, an enabled flag here is also useful to track
> whether the bootloader left the clock on, which allows turning off
> unnecessary clocks.
> 
> I think you also need a list of current children of the clock to allow
> propagating rate changes from parent to children.

This is added in another patch in this series implementing clk_set_rate.

> > +
> > +static void __clk_unprepare(struct clk *clk)
> > +{
> > +       if (!clk)
> > +               return;
> > +
> > +       if (WARN_ON(clk->prepare_count == 0))
> > +               return;
> > +
> > +       if (--clk->prepare_count > 0)
> > +               return;
> > +
> > +       WARN_ON(clk->enable_count > 0);
> > +
> > +       if (clk->ops->unprepare)
> > +               clk->ops->unprepare(clk->hw);
> > +
> > +       __clk_unprepare(clk->parent);
> > +}
> Are there any cases where the unlocked versions of the clk calls need
> to be exposed to the ops implementations?  For example, a set_rate op
> may want to call clk_set_parent on itself to change its parent to a
> better source, and then set its rate.  It would need to call
> __clk_set_parent to avoid deadlocking on the prepare_lock.

I hope we can avoid that. The decision of the best parent should be left
up to the user. Doing this in the mux/divider implementation would
torpedo attempts to implement generic building blocks.

> > +
> > +unsigned long clk_get_rate(struct clk *clk)
> > +{
> > +       if (!clk)
> > +               return 0;
> > +       return clk->rate;
> > +}
> > +EXPORT_SYMBOL_GPL(clk_get_rate);
> > +
> > +long clk_round_rate(struct clk *clk, unsigned long rate)
> > +{
> > +       if (clk && clk->ops->round_rate)
> > +               return clk->ops->round_rate(clk->hw, rate);
> > +       return rate;
> > +}
> > +EXPORT_SYMBOL_GPL(clk_round_rate);
> 
> I think you should hold the prepare mutex around calls to
> clk_round_rate, which will allow some code simplification similar to
> what Russell suggested in another thread.  If you hold the mutex here,
> as well as in clk_set_rate, and you call the round_rate op before the
> set_rate op in clk_set_rate, op implementers can compute the rate in
> their round_rate op, and save the register values needed to get that
> rate in private temporary storage.  The set_rate op can then assume
> that those register values are correct, because the lock is still
> held, and just write them.  That moves all the computation to one
> place, and it only needs to run once.

This won't work in the case of cascaded dividers. These have to call
clk_round_rate in their set_rate op for each possible divider value
to get the best result. They can't do this when both set_rate and
round_rate acquire the lock.


[...]

> > +struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
> > +               const char *name)
> > +{
> > +       struct clk *clk;
> > +
> > +       clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> > +       if (!clk)
> > +               return NULL;
> > +
> > +       clk->name = name;
> > +       clk->ops = ops;
> > +       clk->hw = hw;
> > +       hw->clk = clk;
> > +
> > +       /* Query the hardware for parent and initial rate */
> > +
> > +       if (clk->ops->get_parent)
> > +               /* We don't to lock against prepare/enable here, as
> > +                * the clock is not yet accessible from anywhere */
> > +               clk->parent = clk->ops->get_parent(clk->hw);
> > +
> > +       if (clk->ops->recalc_rate)
> > +               clk->rate = clk->ops->recalc_rate(clk->hw);
> > +
> > +
> > +       return clk;
> > +}
> > +EXPORT_SYMBOL_GPL(clk_register);
> 
> If you are requiring clk's parents (or possible parents?) to be
> registered before clk, you could put the clk_lookup struct inside the
> clk struct and call clkdev_add from clk_register, saving some
> boilerplate in the platforms.

There can be multiple struct clk_lookup for each clock.

> > + *
> > + * @prepare:   Prepare the clock for enabling. This must not return until
> > + *             the clock is fully prepared, and it's safe to call clk_enable.
> > + *             This callback is intended to allow clock implementations to
> > + *             do any initialisation that may sleep. Called with
> > + *             prepare_lock held.
> > + *
> > + * @unprepare: Release the clock from its prepared state. This will typically
> > + *             undo any work done in the @prepare callback. Called with
> > + *             prepare_lock held.
> > + *
> > + * @enable:    Enable the clock atomically. This must not return until the
> > + *             clock is generating a valid clock signal, usable by consumer
> > + *             devices. Called with enable_lock held. This function must not
> > + *             sleep.
> > + *
> > + * @disable:   Disable the clock atomically. Called with enable_lock held.
> > + *             This function must not sleep.
> > + *
> > + * @recalc_rate        Recalculate the rate of this clock, by quering hardware
> > + *             and/or the clock's parent. Called with the global clock mutex
> > + *             held. Optional, but recommended - if this op is not set,
> > + *             clk_get_rate will return 0.
> You need a callback to update the rate when the parent or parent's
> rate changes, which I would call recalc_rate, as well as this
> init-type call.

This is already done on framework level, I think in the clk_set_rate
patch.

Sascha
Colin Cross May 24, 2011, 7:51 a.m. UTC | #4
On Tue, May 24, 2011 at 12:02 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Mon, May 23, 2011 at 04:55:15PM -0700, Colin Cross wrote:
>> On Fri, May 20, 2011 at 12:27 AM, Jeremy Kerr <jeremy.kerr@canonical.com> 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, 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 *);
>> >  };
>>
>> You might want to split these into three separate structs, for mux
>> ops, rate ops, and gate ops.  That way, multiple building blocks (a
>> gate and a divider, for example), can be easily combined into a single
>> clock node.  Also, an init op that reads the clock tree state from the
>> hardware has been very useful on Tegra - there are often clocks that
>> you can't or won't change, and being able to view their state as
>> configured by the bootloader, and base other clocks off of them, is
>> helpful.
>
> The clock state is read initially from the hardware with the
> recalc_rate/get_parent ops. What do we need an additional init op for?

I see - I would rename them to make it clear they are for init, maybe
init_rate and init_parent, and not call them later - reading clock
state can be very expensive on some platforms, if not impossible -
Qualcomm requires RPCs to the modem to get clock state.  If every
clk_set_rate triggers state reads all the way through the descendants,
that could be a huge performance hit.  If you separate init and
recalculate, mux implementations can store their divider settings and
very easily recalculate their rate.

>> It also allows you to turn off clocks that are enabled by
>> the bootloader, but never enabled by the kernel (enabled=1,
>> enable_count=0).
>
> The enable count indeed is a problem. I don't think an init hook
> would be the right solution for this though as this sounds platform
> specific. struct clk_hw_ops should be specific to the type of clock
> (mux, divider, gate) and should be present only once per type.
>
> For the enable count (and whether a clock should initially be enabled or
> not) I can think of something like this:
>
> - A platform/SoC registers all clocks.
> - It then calls clk_prepare/enable for all vital core clocks
>  (SDRAM, CPU,...). At this time the enable counters are correct.
> - Then some hook in the generic clock layer is called. This hook
>  iterates over the tree and disables all clocks in hardware which
>  have a enable count of 0.

Is it always safe to disable a clock that is already disabled?  An
init_enable hook that set an enabled flag would let you only disable
clocks that were actually left on by the bootloader, and report to the
user which ones are actually being turned off (which has caught a lot
of problems on Tegra).

>> > +
>> > +struct clk {
>> > +       const char              *name;
>> > +       struct clk_hw_ops       *ops;
>> > +       struct clk_hw           *hw;
>> > +       unsigned int            enable_count;
>> > +       unsigned int            prepare_count;
>> > +       struct clk              *parent;
>>
>> Storing the list of possible parents at this level can help abstract
>> some common code from mux ops if you pass the index into the list of
>> the new parent into the op - most mux ops only need to know which of
>> their mux inputs needs to be enabled.
>
> Please don't. Only muxes have more than one possible parent, so this
> should be handled there.

The cost is one pointer per clock that is not actually a mux, and the
benefit is that you can move a very common search loop out of every
mux implementation into the framework.  It also lets you determine
which clocks are connected, which becomes necessary if you try to do
per-tree locking or sysfs controls for clocks.

>>
>> > +       unsigned long           rate;
>>
>> If you add an init op, an enabled flag here is also useful to track
>> whether the bootloader left the clock on, which allows turning off
>> unnecessary clocks.
>>
>> I think you also need a list of current children of the clock to allow
>> propagating rate changes from parent to children.
>
> This is added in another patch in this series implementing clk_set_rate.

Thanks, I'll take a look.

>> > +
>> > +static void __clk_unprepare(struct clk *clk)
>> > +{
>> > +       if (!clk)
>> > +               return;
>> > +
>> > +       if (WARN_ON(clk->prepare_count == 0))
>> > +               return;
>> > +
>> > +       if (--clk->prepare_count > 0)
>> > +               return;
>> > +
>> > +       WARN_ON(clk->enable_count > 0);
>> > +
>> > +       if (clk->ops->unprepare)
>> > +               clk->ops->unprepare(clk->hw);
>> > +
>> > +       __clk_unprepare(clk->parent);
>> > +}
>> Are there any cases where the unlocked versions of the clk calls need
>> to be exposed to the ops implementations?  For example, a set_rate op
>> may want to call clk_set_parent on itself to change its parent to a
>> better source, and then set its rate.  It would need to call
>> __clk_set_parent to avoid deadlocking on the prepare_lock.
>
> I hope we can avoid that. The decision of the best parent should be left
> up to the user. Doing this in the mux/divider implementation would
> torpedo attempts to implement generic building blocks.

I agree it would be nice, but it does push some knowledge of the clock
tree into device drivers.  For example, on Tegra the display driver
may need to change the source pll of the display clock to get the
required pclk, which requires passing all the possible parents of the
display clock into the display driver.  If this is a common usage
pattern, there needs to be a hook in the ops to allow the clock or
clock chip to make a more global decision.

>> > +
>> > +unsigned long clk_get_rate(struct clk *clk)
>> > +{
>> > +       if (!clk)
>> > +               return 0;
>> > +       return clk->rate;
>> > +}
>> > +EXPORT_SYMBOL_GPL(clk_get_rate);
>> > +
>> > +long clk_round_rate(struct clk *clk, unsigned long rate)
>> > +{
>> > +       if (clk && clk->ops->round_rate)
>> > +               return clk->ops->round_rate(clk->hw, rate);
>> > +       return rate;
>> > +}
>> > +EXPORT_SYMBOL_GPL(clk_round_rate);
>>
>> I think you should hold the prepare mutex around calls to
>> clk_round_rate, which will allow some code simplification similar to
>> what Russell suggested in another thread.  If you hold the mutex here,
>> as well as in clk_set_rate, and you call the round_rate op before the
>> set_rate op in clk_set_rate, op implementers can compute the rate in
>> their round_rate op, and save the register values needed to get that
>> rate in private temporary storage.  The set_rate op can then assume
>> that those register values are correct, because the lock is still
>> held, and just write them.  That moves all the computation to one
>> place, and it only needs to run once.
>
> This won't work in the case of cascaded dividers. These have to call
> clk_round_rate in their set_rate op for each possible divider value
> to get the best result. They can't do this when both set_rate and
> round_rate acquire the lock.

Then they call __clk_round_rate if they already have the lock?

> [...]
>
>> > +struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
>> > +               const char *name)
>> > +{
>> > +       struct clk *clk;
>> > +
>> > +       clk = kzalloc(sizeof(*clk), GFP_KERNEL);
>> > +       if (!clk)
>> > +               return NULL;
>> > +
>> > +       clk->name = name;
>> > +       clk->ops = ops;
>> > +       clk->hw = hw;
>> > +       hw->clk = clk;
>> > +
>> > +       /* Query the hardware for parent and initial rate */
>> > +
>> > +       if (clk->ops->get_parent)
>> > +               /* We don't to lock against prepare/enable here, as
>> > +                * the clock is not yet accessible from anywhere */
>> > +               clk->parent = clk->ops->get_parent(clk->hw);
>> > +
>> > +       if (clk->ops->recalc_rate)
>> > +               clk->rate = clk->ops->recalc_rate(clk->hw);
>> > +
>> > +
>> > +       return clk;
>> > +}
>> > +EXPORT_SYMBOL_GPL(clk_register);
>>
>> If you are requiring clk's parents (or possible parents?) to be
>> registered before clk, you could put the clk_lookup struct inside the
>> clk struct and call clkdev_add from clk_register, saving some
>> boilerplate in the platforms.
>
> There can be multiple struct clk_lookup for each clock.

Sure, and that could be handled by clk_register_alias.  Most of the
clocks have a single clk_lookup.

>> > + *
>> > + * @prepare:   Prepare the clock for enabling. This must not return until
>> > + *             the clock is fully prepared, and it's safe to call clk_enable.
>> > + *             This callback is intended to allow clock implementations to
>> > + *             do any initialisation that may sleep. Called with
>> > + *             prepare_lock held.
>> > + *
>> > + * @unprepare: Release the clock from its prepared state. This will typically
>> > + *             undo any work done in the @prepare callback. Called with
>> > + *             prepare_lock held.
>> > + *
>> > + * @enable:    Enable the clock atomically. This must not return until the
>> > + *             clock is generating a valid clock signal, usable by consumer
>> > + *             devices. Called with enable_lock held. This function must not
>> > + *             sleep.
>> > + *
>> > + * @disable:   Disable the clock atomically. Called with enable_lock held.
>> > + *             This function must not sleep.
>> > + *
>> > + * @recalc_rate        Recalculate the rate of this clock, by quering hardware
>> > + *             and/or the clock's parent. Called with the global clock mutex
>> > + *             held. Optional, but recommended - if this op is not set,
>> > + *             clk_get_rate will return 0.
>> You need a callback to update the rate when the parent or parent's
>> rate changes, which I would call recalc_rate, as well as this
>> init-type call.
>
> This is already done on framework level, I think in the clk_set_rate
> patch.

I'll comment there.

> Sascha
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>
Sascha Hauer May 24, 2011, 8:38 a.m. UTC | #5
On Tue, May 24, 2011 at 12:51:10AM -0700, Colin Cross wrote:
> On Tue, May 24, 2011 at 12:02 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Mon, May 23, 2011 at 04:55:15PM -0700, Colin Cross wrote:
> >> On Fri, May 20, 2011 at 12:27 AM, Jeremy Kerr <jeremy.kerr@canonical.com> wrote:
> >> >
> >> >  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 *);
> >> >  };
> >>
> >> You might want to split these into three separate structs, for mux
> >> ops, rate ops, and gate ops.  That way, multiple building blocks (a
> >> gate and a divider, for example), can be easily combined into a single
> >> clock node.  Also, an init op that reads the clock tree state from the
> >> hardware has been very useful on Tegra - there are often clocks that
> >> you can't or won't change, and being able to view their state as
> >> configured by the bootloader, and base other clocks off of them, is
> >> helpful.
> >
> > The clock state is read initially from the hardware with the
> > recalc_rate/get_parent ops. What do we need an additional init op for?
> 
> I see - I would rename them to make it clear they are for init, maybe
> init_rate and init_parent, and not call them later - reading clock
> state can be very expensive on some platforms, if not impossible -
> Qualcomm requires RPCs to the modem to get clock state.  If every
> clk_set_rate triggers state reads all the way through the descendants,
> that could be a huge performance hit.  If you separate init and
> recalculate, mux implementations can store their divider settings and
> very easily recalculate their rate.

Even without additional hooks divider and muxes can decide to cache
the actual register values.

> 
> >> It also allows you to turn off clocks that are enabled by
> >> the bootloader, but never enabled by the kernel (enabled=1,
> >> enable_count=0).
> >
> > The enable count indeed is a problem. I don't think an init hook
> > would be the right solution for this though as this sounds platform
> > specific. struct clk_hw_ops should be specific to the type of clock
> > (mux, divider, gate) and should be present only once per type.
> >
> > For the enable count (and whether a clock should initially be enabled or
> > not) I can think of something like this:
> >
> > - A platform/SoC registers all clocks.
> > - It then calls clk_prepare/enable for all vital core clocks
> >  (SDRAM, CPU,...). At this time the enable counters are correct.
> > - Then some hook in the generic clock layer is called. This hook
> >  iterates over the tree and disables all clocks in hardware which
> >  have a enable count of 0.
> 
> Is it always safe to disable a clock that is already disabled?

I'm not aware of any clock where that's not the case, but your mileage
may vary. At least the implementation should be able to determine
whether a clock already is disabled and just skip another disable.

>  An
> init_enable hook that set an enabled flag would let you only disable
> clocks that were actually left on by the bootloader, and report to the
> user which ones are actually being turned off (which has caught a lot
> of problems on Tegra).
> 
> >> > +
> >> > +struct clk {
> >> > +       const char              *name;
> >> > +       struct clk_hw_ops       *ops;
> >> > +       struct clk_hw           *hw;
> >> > +       unsigned int            enable_count;
> >> > +       unsigned int            prepare_count;
> >> > +       struct clk              *parent;
> >>
> >> Storing the list of possible parents at this level can help abstract
> >> some common code from mux ops if you pass the index into the list of
> >> the new parent into the op - most mux ops only need to know which of
> >> their mux inputs needs to be enabled.
> >
> > Please don't. Only muxes have more than one possible parent, so this
> > should be handled there.
> 
> The cost is one pointer per clock that is not actually a mux, and the
> benefit is that you can move a very common search loop out of every
> mux implementation into the framework.  It also lets you determine
> which clocks are connected, which becomes necessary if you try to do
> per-tree locking or sysfs controls for clocks.

I agree that some sort of possible_parents iteration would be nice.

> 
> >>
> >> > +
> >> > +       if (WARN_ON(clk->prepare_count == 0))
> >> > +               return;
> >> > +
> >> > +       if (--clk->prepare_count > 0)
> >> > +               return;
> >> > +
> >> > +       WARN_ON(clk->enable_count > 0);
> >> > +
> >> > +       if (clk->ops->unprepare)
> >> > +               clk->ops->unprepare(clk->hw);
> >> > +
> >> > +       __clk_unprepare(clk->parent);
> >> > +}
> >> Are there any cases where the unlocked versions of the clk calls need
> >> to be exposed to the ops implementations?  For example, a set_rate op
> >> may want to call clk_set_parent on itself to change its parent to a
> >> better source, and then set its rate.  It would need to call
> >> __clk_set_parent to avoid deadlocking on the prepare_lock.
> >
> > I hope we can avoid that. The decision of the best parent should be left
> > up to the user. Doing this in the mux/divider implementation would
> > torpedo attempts to implement generic building blocks.
> 
> I agree it would be nice, but it does push some knowledge of the clock
> tree into device drivers.  For example, on Tegra the display driver
> may need to change the source pll of the display clock to get the
> required pclk, which requires passing all the possible parents of the
> display clock into the display driver.  If this is a common usage
> pattern, there needs to be a hook in the ops to allow the clock or
> clock chip to make a more global decision.

I think this is a common pattern. Another solution to this would be that
the platform implements a clock whose only purpose is to build a bridge
between the driver and the clock tree. There may be more constrains, for
example in some cases you may need a clock which also runs in different
sleep states whereas sometimes you may need a clock which is turned of
when in sleep mode. I agree that this must be handled somewhere, but
the clock framework is not the place to implement this stuff.

> >>
> >> I think you should hold the prepare mutex around calls to
> >> clk_round_rate, which will allow some code simplification similar to
> >> what Russell suggested in another thread.  If you hold the mutex here,
> >> as well as in clk_set_rate, and you call the round_rate op before the
> >> set_rate op in clk_set_rate, op implementers can compute the rate in
> >> their round_rate op, and save the register values needed to get that
> >> rate in private temporary storage.  The set_rate op can then assume
> >> that those register values are correct, because the lock is still
> >> held, and just write them.  That moves all the computation to one
> >> place, and it only needs to run once.
> >
> > This won't work in the case of cascaded dividers. These have to call
> > clk_round_rate in their set_rate op for each possible divider value
> > to get the best result. They can't do this when both set_rate and
> > round_rate acquire the lock.
> 
> Then they call __clk_round_rate if they already have the lock?

I think this is an implementation detail. As our idea of how a clock
framework should work still is quite different we can discuss this later ;)

> 
> > [...]
> >
> >> > +struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
> >> > +               const char *name)
> >> > +{
> >> > +       struct clk *clk;
> >> > +
> >> > +       clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> >> > +       if (!clk)
> >> > +               return NULL;
> >> > +
> >> > +       clk->name = name;
> >> > +       clk->ops = ops;
> >> > +       clk->hw = hw;
> >> > +       hw->clk = clk;
> >> > +
> >> > +       /* Query the hardware for parent and initial rate */
> >> > +
> >> > +       if (clk->ops->get_parent)
> >> > +               /* We don't to lock against prepare/enable here, as
> >> > +                * the clock is not yet accessible from anywhere */
> >> > +               clk->parent = clk->ops->get_parent(clk->hw);
> >> > +
> >> > +       if (clk->ops->recalc_rate)
> >> > +               clk->rate = clk->ops->recalc_rate(clk->hw);
> >> > +
> >> > +
> >> > +       return clk;
> >> > +}
> >> > +EXPORT_SYMBOL_GPL(clk_register);
> >>
> >> If you are requiring clk's parents (or possible parents?) to be
> >> registered before clk, you could put the clk_lookup struct inside the
> >> clk struct and call clkdev_add from clk_register, saving some
> >> boilerplate in the platforms.
> >
> > There can be multiple struct clk_lookup for each clock.
> 
> Sure, and that could be handled by clk_register_alias.  Most of the
> clocks have a single clk_lookup.

In my idea only few of the clocks have a clk_lookup (you mentioned a
clock between the i2c divider and i2c gate elsewhere, this would never
be passed to a device).

Sascha
Richard Zhao May 25, 2011, 10:47 a.m. UTC | #6
On Fri, May 20, 2011 at 03:27:49PM +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, 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 *);
why not pass down parent rate?
>   	int		(*set_rate)(struct clk_hw *,
>   					unsigned long, unsigned long *);
>   	long		(*round_rate)(struct clk_hw *, unsigned long);
ditto
>   	int		(*set_parent)(struct clk_hw *, struct clk *);
>   	struct clk *	(*get_parent)(struct clk_hw *);
>   };
> 
[...]

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> new file mode 100644
> index 0000000..ad90a90
> --- /dev/null
> +++ b/drivers/clk/clk.c
> @@ -0,0 +1,229 @@
> +/*
> + * Copyright (C) 2010-2011 Canonical Ltd <jeremy.kerr@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
> + * published by the Free Software Foundation.
> + *
> + * Standard functionality for the common clock API.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +struct clk {
> +	const char		*name;
> +	struct clk_hw_ops	*ops;
> +	struct clk_hw		*hw;
If we don't let it void *, why not put clk_hw_ops into clk_hw too? It'll be
more easy to define static clocks.
> +	unsigned int		enable_count;
> +	unsigned int		prepare_count;
> +	struct clk		*parent;
> +	unsigned long		rate;
> +};
> +
> +static DEFINE_SPINLOCK(enable_lock);
> +static DEFINE_MUTEX(prepare_lock);
> +
> +static void __clk_unprepare(struct clk *clk)
> +{
> +	if (!clk)
> +		return;
> +
> +	if (WARN_ON(clk->prepare_count == 0))
> +		return;
> +
> +	if (--clk->prepare_count > 0)
> +		return;
> +
> +	WARN_ON(clk->enable_count > 0);
> +
> +	if (clk->ops->unprepare)
> +		clk->ops->unprepare(clk->hw);
> +
> +	__clk_unprepare(clk->parent);
> +}
> +
> +void clk_unprepare(struct clk *clk)
> +{
> +	mutex_lock(&prepare_lock);
> +	__clk_unprepare(clk);
> +	mutex_unlock(&prepare_lock);
> +}
> +EXPORT_SYMBOL_GPL(clk_unprepare);
> +
> +static int __clk_prepare(struct clk *clk)
> +{
> +	int ret = 0;
> +
> +	if (!clk)
> +		return 0;
> +
> +	if (clk->prepare_count == 0) {
> +		ret = __clk_prepare(clk->parent);
> +		if (ret)
> +			return ret;
> +
> +		if (clk->ops->prepare) {
> +			ret = clk->ops->prepare(clk->hw);
> +			if (ret) {
> +				__clk_unprepare(clk->parent);
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	clk->prepare_count++;
> +
> +	return 0;
> +}
> +
> +int clk_prepare(struct clk *clk)
> +{
> +	int ret;
> +
> +	mutex_lock(&prepare_lock);
> +	ret = __clk_prepare(clk);
> +	mutex_unlock(&prepare_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(clk_prepare);
> +
> +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;
> +}
> +
> +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);
> +
> +unsigned long clk_get_rate(struct clk *clk)
> +{
> +	if (!clk)
> +		return 0;
> +	return clk->rate;
> +}
> +EXPORT_SYMBOL_GPL(clk_get_rate);
> +
> +long clk_round_rate(struct clk *clk, unsigned long rate)
> +{
> +	if (clk && clk->ops->round_rate)
> +		return clk->ops->round_rate(clk->hw, rate);
pass parent rate? IMHO, common code can handle parent as much as possible.
> +	return rate;
> +}
> +EXPORT_SYMBOL_GPL(clk_round_rate);
> +
> +int clk_set_rate(struct clk *clk, unsigned long rate)
> +{
> +	/* not yet implemented */
> +	return -ENOSYS;
> +}
> +EXPORT_SYMBOL_GPL(clk_set_rate);
> +
> +struct clk *clk_get_parent(struct clk *clk)
> +{
> +	if (!clk)
> +		return NULL;
> +
> +	return clk->parent;
> +}
> +EXPORT_SYMBOL_GPL(clk_get_parent);
> +
> +int clk_set_parent(struct clk *clk, struct clk *parent)
> +{
> +	/* not yet implemented */
> +	return -ENOSYS;
> +}
> +EXPORT_SYMBOL_GPL(clk_set_parent);
> +
> +struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
> +		const char *name)
> +{
> +	struct clk *clk;
> +
> +	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> +	if (!clk)
> +		return NULL;
> +
> +	clk->name = name;
> +	clk->ops = ops;
> +	clk->hw = hw;
> +	hw->clk = clk;
> +
> +	/* Query the hardware for parent and initial rate */
> +
> +	if (clk->ops->get_parent)
> +		/* We don't to lock against prepare/enable here, as
> +		 * the clock is not yet accessible from anywhere */
> +		clk->parent = clk->ops->get_parent(clk->hw);
> +
> +	if (clk->ops->recalc_rate)
> +		clk->rate = clk->ops->recalc_rate(clk->hw);
ditto

Thanks
Richard
> +
> +
> +	return clk;
> +}
> +EXPORT_SYMBOL_GPL(clk_register);
> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index 6db161f..e2a9719 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -23,6 +23,13 @@
>  static LIST_HEAD(clocks);
>  static DEFINE_MUTEX(clocks_mutex);
>  
> +/* For USE_COMMON_STRUCT_CLK, these are provided in clk.c, but not exported
> + * through other headers; we don't want them used anywhere but here. */
> +#ifdef CONFIG_USE_COMMON_STRUCT_CLK
> +extern int __clk_get(struct clk *clk);
> +extern void __clk_put(struct clk *clk);
> +#endif
> +
>  /*
>   * Find the correct struct clk for the device and connection ID.
>   * We do slightly fuzzy matching here:
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 1d37f42..93ff870 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-2011 Jeremy Kerr <jeremy.kerr@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,17 +12,103 @@
>  #ifndef __LINUX_CLK_H
>  #define __LINUX_CLK_H
>  
> +#include <linux/err.h>
> +#include <linux/spinlock.h>
> +
>  struct device;
>  
> -/*
> - * The base API.
> +struct clk;
> +
> +#ifdef CONFIG_GENERIC_CLK
> +
> +struct clk_hw {
> +	struct clk *clk;
> +};
> +
> +/**
> + * struct clk_hw_ops -  Callback operations for hardware clocks; these are to
> + * be provided by the clock implementation, and will be called by drivers
> + * through the clk_* API.
> + *
> + * @prepare:	Prepare the clock for enabling. This must not return until
> + *		the clock is fully prepared, and it's safe to call clk_enable.
> + *		This callback is intended to allow clock implementations to
> + *		do any initialisation that may sleep. Called with
> + *		prepare_lock held.
> + *
> + * @unprepare:	Release the clock from its prepared state. This will typically
> + *		undo any work done in the @prepare callback. Called with
> + *		prepare_lock held.
> + *
> + * @enable:	Enable the clock atomically. This must not return until the
> + *		clock is generating a valid clock signal, usable by consumer
> + *		devices. Called with enable_lock held. This function must not
> + *		sleep.
> + *
> + * @disable:	Disable the clock atomically. Called with enable_lock held.
> + *		This function must not sleep.
> + *
> + * @recalc_rate	Recalculate the rate of this clock, by quering hardware
> + *		and/or the clock's parent. Called with the global clock mutex
> + *		held. Optional, but recommended - if this op is not set,
> + *		clk_get_rate will return 0.
> + *
> + * @get_parent	Query the parent of this clock; for clocks with multiple
> + *		possible parents, query the hardware for the current
> + *		parent. Currently only called when the clock is first
> + *		registered.
> + *
> + * The clk_enable/clk_disable and clk_prepare/clk_unprepare pairs allow
> + * implementations to split any work between atomic (enable) and sleepable
> + * (prepare) contexts.  If a clock requires sleeping code to be turned on, this
> + * should be done in clk_prepare. Switching that will not sleep should be done
> + * in clk_enable.
> + *
> + * Typically, drivers will call clk_prepare when a clock may be needed later
> + * (eg. when a device is opened), and clk_enable when the clock is actually
> + * required (eg. from an interrupt). Note that clk_prepare *must* have been
> + * called before clk_enable.
> + */
> +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 *);
> +	long		(*round_rate)(struct clk_hw *, unsigned long);
> +	struct clk *	(*get_parent)(struct clk_hw *);
> +};
> +
> +/**
> + * clk_prepare - prepare clock for atomic enabling.
> + *
> + * @clk: The clock to prepare
> + *
> + * Do any possibly sleeping initialisation on @clk, allowing the clock to be
> + * later enabled atomically (via clk_enable). This function may sleep.
>   */
> +int clk_prepare(struct clk *clk);
>  
> +/**
> + * clk_unprepare - release clock from prepared state
> + *
> + * @clk: The clock to release
> + *
> + * Do any (possibly sleeping) cleanup on clk. This function may sleep.
> + */
> +void clk_unprepare(struct clk *clk);
> +
> +#else /* !CONFIG_GENERIC_CLK */
>  
>  /*
> - * struct clk - an machine class defined object / cookie.
> + * For !CONFIG_GENERIC_CLK, we don't enforce any atomicity
> + * requirements for clk_enable/clk_disable, so the prepare and unprepare
> + * functions are no-ops
>   */
> -struct clk;
> +static inline int clk_prepare(struct clk *clk) { return 0; }
> +static inline void clk_unprepare(struct clk *clk) { }
> +
> +#endif /* !CONFIG_GENERIC_CLK */
>  
>  /**
>   * clk_get - lookup and obtain a reference to a clock producer.
> @@ -67,6 +154,7 @@ void clk_disable(struct clk *clk);
>  /**
>   * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
>   *		  This is only valid once the clock source has been enabled.
> + *		  Returns zero if the clock rate is unknown.
>   * @clk: clock source
>   */
>  unsigned long clk_get_rate(struct clk *clk);
> @@ -83,12 +171,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
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Richard Zhao May 25, 2011, 11:22 a.m. UTC | #7
On Tue, May 24, 2011 at 10:38:04AM +0200, Sascha Hauer wrote:
> On Tue, May 24, 2011 at 12:51:10AM -0700, Colin Cross wrote:
> > On Tue, May 24, 2011 at 12:02 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > > On Mon, May 23, 2011 at 04:55:15PM -0700, Colin Cross wrote:
> > >> On Fri, May 20, 2011 at 12:27 AM, Jeremy Kerr <jeremy.kerr@canonical.com> wrote:
> > >> >
> > >> >  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 *);
> > >> >  };
> > >>
> > >> You might want to split these into three separate structs, for mux
> > >> ops, rate ops, and gate ops.  That way, multiple building blocks (a
> > >> gate and a divider, for example), can be easily combined into a single
> > >> clock node.  Also, an init op that reads the clock tree state from the
> > >> hardware has been very useful on Tegra - there are often clocks that
> > >> you can't or won't change, and being able to view their state as
> > >> configured by the bootloader, and base other clocks off of them, is
> > >> helpful.
> > >
> > > The clock state is read initially from the hardware with the
> > > recalc_rate/get_parent ops. What do we need an additional init op for?
> > 
> > I see - I would rename them to make it clear they are for init, maybe
> > init_rate and init_parent, and not call them later - reading clock
> > state can be very expensive on some platforms, if not impossible -
> > Qualcomm requires RPCs to the modem to get clock state.  If every
> > clk_set_rate triggers state reads all the way through the descendants,
> > that could be a huge performance hit.  If you separate init and
> > recalculate, mux implementations can store their divider settings and
> > very easily recalculate their rate.
> 
> Even without additional hooks divider and muxes can decide to cache
> the actual register values.
> 
> > 
> > >> It also allows you to turn off clocks that are enabled by
> > >> the bootloader, but never enabled by the kernel (enabled=1,
> > >> enable_count=0).
> > >
> > > The enable count indeed is a problem. I don't think an init hook
> > > would be the right solution for this though as this sounds platform
> > > specific. struct clk_hw_ops should be specific to the type of clock
> > > (mux, divider, gate) and should be present only once per type.
> > >
> > > For the enable count (and whether a clock should initially be enabled or
> > > not) I can think of something like this:
> > >
> > > - A platform/SoC registers all clocks.
> > > - It then calls clk_prepare/enable for all vital core clocks
> > >  (SDRAM, CPU,...). At this time the enable counters are correct.
> > > - Then some hook in the generic clock layer is called. This hook
> > >  iterates over the tree and disables all clocks in hardware which
> > >  have a enable count of 0.
> > 
> > Is it always safe to disable a clock that is already disabled?
> 
> I'm not aware of any clock where that's not the case, but your mileage
> may vary. At least the implementation should be able to determine
> whether a clock already is disabled and just skip another disable.
Might be one possible case: arm AP and modem boot simultaneously, you can not
disable modem clock before modem driver increase the enable_count.
But nice to have a helper function to disable zero enable_count clocks.
> 
> >  An
> > init_enable hook that set an enabled flag would let you only disable
> > clocks that were actually left on by the bootloader, and report to the
> > user which ones are actually being turned off (which has caught a lot
> > of problems on Tegra).
> > 
> > >> > +
> > >> > +struct clk {
> > >> > +       const char              *name;
> > >> > +       struct clk_hw_ops       *ops;
> > >> > +       struct clk_hw           *hw;
> > >> > +       unsigned int            enable_count;
> > >> > +       unsigned int            prepare_count;
> > >> > +       struct clk              *parent;
> > >>
> > >> Storing the list of possible parents at this level can help abstract
> > >> some common code from mux ops if you pass the index into the list of
> > >> the new parent into the op - most mux ops only need to know which of
> > >> their mux inputs needs to be enabled.
> > >
> > > Please don't. Only muxes have more than one possible parent, so this
> > > should be handled there.
> > 
> > The cost is one pointer per clock that is not actually a mux, and the
> > benefit is that you can move a very common search loop out of every
> > mux implementation into the framework.  It also lets you determine
> > which clocks are connected, which becomes necessary if you try to do
> > per-tree locking or sysfs controls for clocks.
> 
> I agree that some sort of possible_parents iteration would be nice.
> 
> > 
> > >>
> > >> > +
> > >> > +       if (WARN_ON(clk->prepare_count == 0))
> > >> > +               return;
> > >> > +
> > >> > +       if (--clk->prepare_count > 0)
> > >> > +               return;
> > >> > +
> > >> > +       WARN_ON(clk->enable_count > 0);
> > >> > +
> > >> > +       if (clk->ops->unprepare)
> > >> > +               clk->ops->unprepare(clk->hw);
> > >> > +
> > >> > +       __clk_unprepare(clk->parent);
> > >> > +}
> > >> Are there any cases where the unlocked versions of the clk calls need
> > >> to be exposed to the ops implementations?  For example, a set_rate op
> > >> may want to call clk_set_parent on itself to change its parent to a
> > >> better source, and then set its rate.  It would need to call
> > >> __clk_set_parent to avoid deadlocking on the prepare_lock.
> > >
> > > I hope we can avoid that. The decision of the best parent should be left
> > > up to the user. Doing this in the mux/divider implementation would
> > > torpedo attempts to implement generic building blocks.
I believe some day, when we have to call child clock consumers' on-rate-change
hooks, we will need it.

Thanks
Richard
> > 
> > I agree it would be nice, but it does push some knowledge of the clock
> > tree into device drivers.  For example, on Tegra the display driver
> > may need to change the source pll of the display clock to get the
> > required pclk, which requires passing all the possible parents of the
> > display clock into the display driver.  If this is a common usage
> > pattern, there needs to be a hook in the ops to allow the clock or
> > clock chip to make a more global decision.
> 
> I think this is a common pattern. Another solution to this would be that
> the platform implements a clock whose only purpose is to build a bridge
> between the driver and the clock tree. There may be more constrains, for
> example in some cases you may need a clock which also runs in different
> sleep states whereas sometimes you may need a clock which is turned of
> when in sleep mode. I agree that this must be handled somewhere, but
> the clock framework is not the place to implement this stuff.
> 
> > >>
> > >> I think you should hold the prepare mutex around calls to
> > >> clk_round_rate, which will allow some code simplification similar to
> > >> what Russell suggested in another thread.  If you hold the mutex here,
> > >> as well as in clk_set_rate, and you call the round_rate op before the
> > >> set_rate op in clk_set_rate, op implementers can compute the rate in
> > >> their round_rate op, and save the register values needed to get that
> > >> rate in private temporary storage.  The set_rate op can then assume
> > >> that those register values are correct, because the lock is still
> > >> held, and just write them.  That moves all the computation to one
> > >> place, and it only needs to run once.
> > >
> > > This won't work in the case of cascaded dividers. These have to call
> > > clk_round_rate in their set_rate op for each possible divider value
> > > to get the best result. They can't do this when both set_rate and
> > > round_rate acquire the lock.
> > 
> > Then they call __clk_round_rate if they already have the lock?
> 
> I think this is an implementation detail. As our idea of how a clock
> framework should work still is quite different we can discuss this later ;)
> 
> > 
> > > [...]
> > >
> > >> > +struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
> > >> > +               const char *name)
> > >> > +{
> > >> > +       struct clk *clk;
> > >> > +
> > >> > +       clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> > >> > +       if (!clk)
> > >> > +               return NULL;
> > >> > +
> > >> > +       clk->name = name;
> > >> > +       clk->ops = ops;
> > >> > +       clk->hw = hw;
> > >> > +       hw->clk = clk;
> > >> > +
> > >> > +       /* Query the hardware for parent and initial rate */
> > >> > +
> > >> > +       if (clk->ops->get_parent)
> > >> > +               /* We don't to lock against prepare/enable here, as
> > >> > +                * the clock is not yet accessible from anywhere */
> > >> > +               clk->parent = clk->ops->get_parent(clk->hw);
> > >> > +
> > >> > +       if (clk->ops->recalc_rate)
> > >> > +               clk->rate = clk->ops->recalc_rate(clk->hw);
> > >> > +
> > >> > +
> > >> > +       return clk;
> > >> > +}
> > >> > +EXPORT_SYMBOL_GPL(clk_register);
> > >>
> > >> If you are requiring clk's parents (or possible parents?) to be
> > >> registered before clk, you could put the clk_lookup struct inside the
> > >> clk struct and call clkdev_add from clk_register, saving some
> > >> boilerplate in the platforms.
> > >
> > > There can be multiple struct clk_lookup for each clock.
> > 
> > Sure, and that could be handled by clk_register_alias.  Most of the
> > clocks have a single clk_lookup.
> 
> In my idea only few of the clocks have a clk_lookup (you mentioned a
> clock between the i2c divider and i2c gate elsewhere, this would never
> be passed to a device).
> 
> Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Thomas Gleixner May 25, 2011, 11:43 a.m. UTC | #8
On Tue, 24 May 2011, Colin Cross wrote:
> On Tue, May 24, 2011 at 12:02 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >> > +       struct clk              *parent;
> >>
> >> Storing the list of possible parents at this level can help abstract
> >> some common code from mux ops if you pass the index into the list of
> >> the new parent into the op - most mux ops only need to know which of
> >> their mux inputs needs to be enabled.
> >
> > Please don't. Only muxes have more than one possible parent, so this
> > should be handled there.
> 
> The cost is one pointer per clock that is not actually a mux, and the
> benefit is that you can move a very common search loop out of every
> mux implementation into the framework.  It also lets you determine
> which clocks are connected, which becomes necessary if you try to do
> per-tree locking or sysfs controls for clocks.

We can be more clever than storing the world and some more in the
internal implementation.

struct clk should be just the basic data structure to hold the
information which is shared by all clk types. The way I see it is:

struct clk {
       struct clk_ops *ops;
       struct clk_data *data;
       void *hwdata;
       unsigned long flags;
       ....
};

Now have

struct clk_data {
       union {
       	     struct clk_divider_data;
	     struct clk_mux_data;
	     struct clk_pll_data;
	     .....
       };
};

Now on registration time you tell the core which type you are handing
in. struct clk_mux_data would contain the list of possible parent
clocks and clk_pll_data the possible multiplier values (linear or
table based). Same for dividers. Now the core can do the evaluation of
clock rate settings or parents and hand in the index or the linear
range value into ops->set_rate/set_parent. And with the information
available to the core you can do even complex settings which involve
selecting the proper pll for your pixelclock example below.

> > I hope we can avoid that. The decision of the best parent should be left
> > up to the user. Doing this in the mux/divider implementation would
> > torpedo attempts to implement generic building blocks.
> 
> I agree it would be nice, but it does push some knowledge of the clock
> tree into device drivers.  For example, on Tegra the display driver
> may need to change the source pll of the display clock to get the
> required pclk, which requires passing all the possible parents of the
> display clock into the display driver.  If this is a common usage
> pattern, there needs to be a hook in the ops to allow the clock or
> clock chip to make a more global decision.

No, we really dont want to have that knowledge at the driver level. A
driver should just say: Enable "pixelclk" at rate X.

So then a mapper converts pixelclk to the particular clock on the SoC
it's running on and the clk framework level needs to figure out how to
achieve that by looking at the full chain down from the clk gate which
feeds the pixelclk. We don't want drivers to be aware about that at
all. They simply do not care and that's a preliminary for making
drivers usable on different SoC incarnations.
 
Thanks,

	tglx
Mike Frysinger May 30, 2011, 5 a.m. UTC | #9
On Wednesday, May 25, 2011 06:47:50 Richard Zhao wrote:
> On Fri, May 20, 2011 at 03:27:49PM +0800, Jeremy Kerr wrote:
> > +struct clk {
> > +	const char		*name;
> > +	struct clk_hw_ops	*ops;
> > +	struct clk_hw		*hw;
> 
> If we don't let it void *, why not put clk_hw_ops into clk_hw too? It'll be
> more easy to define static clocks.

because the ops should be const, and possibly shared among multiple clocks.  
i.e. if i have 4 SPI buses, there should only be one copy of the ops structure 
but 4 clk_hw instances.
-mike
diff mbox

Patch

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 4168c88..e611e34 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -2,3 +2,6 @@ 
 config CLKDEV_LOOKUP
 	bool
 	select HAVE_CLK
+
+config GENERIC_CLK
+	bool
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 07613fa..570d5b9 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -1,2 +1,3 @@ 
 
 obj-$(CONFIG_CLKDEV_LOOKUP)	+= clkdev.o
+obj-$(CONFIG_GENERIC_CLK)	+= clk.o
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
new file mode 100644
index 0000000..ad90a90
--- /dev/null
+++ b/drivers/clk/clk.c
@@ -0,0 +1,229 @@ 
+/*
+ * Copyright (C) 2010-2011 Canonical Ltd <jeremy.kerr@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
+ * published by the Free Software Foundation.
+ *
+ * Standard functionality for the common clock API.
+ */
+
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+struct clk {
+	const char		*name;
+	struct clk_hw_ops	*ops;
+	struct clk_hw		*hw;
+	unsigned int		enable_count;
+	unsigned int		prepare_count;
+	struct clk		*parent;
+	unsigned long		rate;
+};
+
+static DEFINE_SPINLOCK(enable_lock);
+static DEFINE_MUTEX(prepare_lock);
+
+static void __clk_unprepare(struct clk *clk)
+{
+	if (!clk)
+		return;
+
+	if (WARN_ON(clk->prepare_count == 0))
+		return;
+
+	if (--clk->prepare_count > 0)
+		return;
+
+	WARN_ON(clk->enable_count > 0);
+
+	if (clk->ops->unprepare)
+		clk->ops->unprepare(clk->hw);
+
+	__clk_unprepare(clk->parent);
+}
+
+void clk_unprepare(struct clk *clk)
+{
+	mutex_lock(&prepare_lock);
+	__clk_unprepare(clk);
+	mutex_unlock(&prepare_lock);
+}
+EXPORT_SYMBOL_GPL(clk_unprepare);
+
+static int __clk_prepare(struct clk *clk)
+{
+	int ret = 0;
+
+	if (!clk)
+		return 0;
+
+	if (clk->prepare_count == 0) {
+		ret = __clk_prepare(clk->parent);
+		if (ret)
+			return ret;
+
+		if (clk->ops->prepare) {
+			ret = clk->ops->prepare(clk->hw);
+			if (ret) {
+				__clk_unprepare(clk->parent);
+				return ret;
+			}
+		}
+	}
+
+	clk->prepare_count++;
+
+	return 0;
+}
+
+int clk_prepare(struct clk *clk)
+{
+	int ret;
+
+	mutex_lock(&prepare_lock);
+	ret = __clk_prepare(clk);
+	mutex_unlock(&prepare_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clk_prepare);
+
+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;
+}
+
+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);
+
+unsigned long clk_get_rate(struct clk *clk)
+{
+	if (!clk)
+		return 0;
+	return clk->rate;
+}
+EXPORT_SYMBOL_GPL(clk_get_rate);
+
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+	if (clk && clk->ops->round_rate)
+		return clk->ops->round_rate(clk->hw, rate);
+	return rate;
+}
+EXPORT_SYMBOL_GPL(clk_round_rate);
+
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	/* not yet implemented */
+	return -ENOSYS;
+}
+EXPORT_SYMBOL_GPL(clk_set_rate);
+
+struct clk *clk_get_parent(struct clk *clk)
+{
+	if (!clk)
+		return NULL;
+
+	return clk->parent;
+}
+EXPORT_SYMBOL_GPL(clk_get_parent);
+
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	/* not yet implemented */
+	return -ENOSYS;
+}
+EXPORT_SYMBOL_GPL(clk_set_parent);
+
+struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
+		const char *name)
+{
+	struct clk *clk;
+
+	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
+	if (!clk)
+		return NULL;
+
+	clk->name = name;
+	clk->ops = ops;
+	clk->hw = hw;
+	hw->clk = clk;
+
+	/* Query the hardware for parent and initial rate */
+
+	if (clk->ops->get_parent)
+		/* We don't to lock against prepare/enable here, as
+		 * the clock is not yet accessible from anywhere */
+		clk->parent = clk->ops->get_parent(clk->hw);
+
+	if (clk->ops->recalc_rate)
+		clk->rate = clk->ops->recalc_rate(clk->hw);
+
+
+	return clk;
+}
+EXPORT_SYMBOL_GPL(clk_register);
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 6db161f..e2a9719 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -23,6 +23,13 @@ 
 static LIST_HEAD(clocks);
 static DEFINE_MUTEX(clocks_mutex);
 
+/* For USE_COMMON_STRUCT_CLK, these are provided in clk.c, but not exported
+ * through other headers; we don't want them used anywhere but here. */
+#ifdef CONFIG_USE_COMMON_STRUCT_CLK
+extern int __clk_get(struct clk *clk);
+extern void __clk_put(struct clk *clk);
+#endif
+
 /*
  * Find the correct struct clk for the device and connection ID.
  * We do slightly fuzzy matching here:
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 1d37f42..93ff870 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-2011 Jeremy Kerr <jeremy.kerr@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,17 +12,103 @@ 
 #ifndef __LINUX_CLK_H
 #define __LINUX_CLK_H
 
+#include <linux/err.h>
+#include <linux/spinlock.h>
+
 struct device;
 
-/*
- * The base API.
+struct clk;
+
+#ifdef CONFIG_GENERIC_CLK
+
+struct clk_hw {
+	struct clk *clk;
+};
+
+/**
+ * struct clk_hw_ops -  Callback operations for hardware clocks; these are to
+ * be provided by the clock implementation, and will be called by drivers
+ * through the clk_* API.
+ *
+ * @prepare:	Prepare the clock for enabling. This must not return until
+ *		the clock is fully prepared, and it's safe to call clk_enable.
+ *		This callback is intended to allow clock implementations to
+ *		do any initialisation that may sleep. Called with
+ *		prepare_lock held.
+ *
+ * @unprepare:	Release the clock from its prepared state. This will typically
+ *		undo any work done in the @prepare callback. Called with
+ *		prepare_lock held.
+ *
+ * @enable:	Enable the clock atomically. This must not return until the
+ *		clock is generating a valid clock signal, usable by consumer
+ *		devices. Called with enable_lock held. This function must not
+ *		sleep.
+ *
+ * @disable:	Disable the clock atomically. Called with enable_lock held.
+ *		This function must not sleep.
+ *
+ * @recalc_rate	Recalculate the rate of this clock, by quering hardware
+ *		and/or the clock's parent. Called with the global clock mutex
+ *		held. Optional, but recommended - if this op is not set,
+ *		clk_get_rate will return 0.
+ *
+ * @get_parent	Query the parent of this clock; for clocks with multiple
+ *		possible parents, query the hardware for the current
+ *		parent. Currently only called when the clock is first
+ *		registered.
+ *
+ * The clk_enable/clk_disable and clk_prepare/clk_unprepare pairs allow
+ * implementations to split any work between atomic (enable) and sleepable
+ * (prepare) contexts.  If a clock requires sleeping code to be turned on, this
+ * should be done in clk_prepare. Switching that will not sleep should be done
+ * in clk_enable.
+ *
+ * Typically, drivers will call clk_prepare when a clock may be needed later
+ * (eg. when a device is opened), and clk_enable when the clock is actually
+ * required (eg. from an interrupt). Note that clk_prepare *must* have been
+ * called before clk_enable.
+ */
+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 *);
+	long		(*round_rate)(struct clk_hw *, unsigned long);
+	struct clk *	(*get_parent)(struct clk_hw *);
+};
+
+/**
+ * clk_prepare - prepare clock for atomic enabling.
+ *
+ * @clk: The clock to prepare
+ *
+ * Do any possibly sleeping initialisation on @clk, allowing the clock to be
+ * later enabled atomically (via clk_enable). This function may sleep.
  */
+int clk_prepare(struct clk *clk);
 
+/**
+ * clk_unprepare - release clock from prepared state
+ *
+ * @clk: The clock to release
+ *
+ * Do any (possibly sleeping) cleanup on clk. This function may sleep.
+ */
+void clk_unprepare(struct clk *clk);
+
+#else /* !CONFIG_GENERIC_CLK */
 
 /*
- * struct clk - an machine class defined object / cookie.
+ * For !CONFIG_GENERIC_CLK, we don't enforce any atomicity
+ * requirements for clk_enable/clk_disable, so the prepare and unprepare
+ * functions are no-ops
  */
-struct clk;
+static inline int clk_prepare(struct clk *clk) { return 0; }
+static inline void clk_unprepare(struct clk *clk) { }
+
+#endif /* !CONFIG_GENERIC_CLK */
 
 /**
  * clk_get - lookup and obtain a reference to a clock producer.
@@ -67,6 +154,7 @@  void clk_disable(struct clk *clk);
 /**
  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
  *		  This is only valid once the clock source has been enabled.
+ *		  Returns zero if the clock rate is unknown.
  * @clk: clock source
  */
 unsigned long clk_get_rate(struct clk *clk);
@@ -83,12 +171,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