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

On Mon, May 23, 2011 at 04:55:15PM -0700, Colin Cross wrote:

On Fri, May 20, 2011 at 12:27 AM, Jeremy Kerr [off-list ref] wrote:

quoted

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.

quoted

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

quoted

+ ? ? ? 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.

quoted

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

quoted

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


[...]

quoted

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

quoted

+ *
+ * @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

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