On Mon, May 02, 2011 at 11:35:25PM -0700, Colin Cross wrote:

On Sat, Apr 30, 2011 at 7:27 AM, torbenh [off-list ref] wrote:

quoted

On Fri, Apr 29, 2011 at 02:26:13PM +0100, Russell King - ARM Linux wrote:

quoted

On Fri, Apr 29, 2011 at 02:13:06PM +0200, Thomas Gleixner wrote:

quoted

This are all well defined semantical problems and should be solved in
common code rather than having different buggy implementations in each
SoC clock "framework".

Why are you associating functional elements in drivers/clk with the
SoC clock framework?

i think i pretty much understand what thomas is after. but i have
problems understanding, what you want here.

until this point, it looks like you only want the driver/clk to
model the clock endpoints inside the devices. A device itself shouldnt
be caring for clock parents or anything. its only concern is that a
clock pin is fed with the right frequency, or off...

quoted

I think that's the root cause of this misunderstanding, and until you
start realising that these building blocks are not part of the SoC
implementation, we're going to continue arguing about this.

what you say now, pretty much sounds like what thomas wants.
these building blocks would be objects derived from the clock baseclass,
which thomas is trying to define. he doesnt seem to be concerned about
the second more special layer yet.

i am quite puzzled, what exactly your fighting over :S

Thomas wants a core framework that handles the clock tree, with each
clock being the implementation of just the clock.  Russell thinks this
is impossible, and wants to stop at implementing building blocks for
the standard clock types, and let each SoC put them into a tree.

Thomas wants the clocks to reference a clock_chip.
(much like genirq... you have irq_desc and and irq_chip)
so there is still one component which has the "big picture"
and can implement crazy platform rules, if necessary.

I don't think anybody can argue that a shared clock tree framework, if
it was possible to do well, would be a benefit.  But there are a few
problems that make abstracting the differences between SoCs very hard
at that level.

After implementing the Tegra clock tree at least 10 times to solve
different problems, there are two very hard problems to solve.

First is locking.  Per-clock locking is hard because it requires
choosing up front which way locking will go, usually locking the
child, then locking the parent, then enabling the parent, and enabling
the child.  Any operation flow that requires traversing the tree from
the parent to the child, for example changing the rate of the parent,
causes lock inversion.  Global locking is easy, but can be wasteful on
platforms that have some very slow clocks (think external i2c clocks),
and some very fast clocks (internal register writes).  This is
mitigated by the separation of clk_prepare and clk_enable locking -
clk_enable can never sleep, so it can't be blocked for long.  Locking
at the root node of the tree would be nice, but most SoCs don't have a
root node - they have a fast root clock and a slow root clock, and can
switch the all or most of clock tree over to the slow clock in some
modes.

yeah. one of the big reasons, why the generic code should do the
locking. 

The second problem is rate changing.  Different platforms require
wildly different strategies for changing clock rates.  On Tegra,
clk_set_rate will never change the parent, because we can generally
set up the parent clocks to the correct frequencies for all the
devices, and then device drivers can change their dividers to get the
rate they need.  The few cases where the parent needs to change
(cpufreq and display pclk), the driver manually calls clk_set_parent.
On other platforms, it is common to need to change the parent to get
the necessary rate.

Automatically selecting parents in a generic way is impossible.  If
one DEV1 calls clk_set_rate and clk_enable, and the core framework
responds by setting pll A to frequency X, and setting the parent of
DEV1 to A, what happens when DEV2 calls clk_set_rate to a frequency
that can only be satisfied by setting pll A to frequency Y?  DEV1
can't be switched to another pll, because it is already enabled, and
some platforms can't support glitchlessly changing a mux, even if the
frequency doesn't change.

in the case where we cant change a parent clock, the generic code could
just fail to set_rate. then delegate the problem to the clk_chip which
can resolve the problem.
the locking would be tricky, but it is solvable.
but its still somthing we only want implemented once.

so how does changing a mux, which glitches, work now ?
do the child clocked devices need to be turned off/notified when this
happens ?

I still think a common implementation of the clock tree framework
would be useful, even if not all platforms can use it.  Allow the
platforms that can to use it, providing only clock data, and maybe a
few clock implementations if the building blocks are not sufficient,
and allow those that can't to implement their own versions of the
clk_* functions on top of the clock building blocks.  Tegra already
has a fairly generic implementation of a clock tree in
arch/arm/mach-tegra/clock.c, moving it to the common clk struct and
sharing it with other platforms wouldn't be any extra code, even if no
other platforms could use it.  And I bet at least OMAP could use it,
from what I've seen.

Per-tree locking is a solvable problem, as long as a tree was
considered as an ordered graph - two root nodes that feed into the
same clocks would be considered part of the same tree.  The core
framework could look at all possible parents of a clock to determine
what clock trees were truly independent, which would result in a
single global lock on most platforms, but multiple locks on platforms
that really had independent clock trees.  Dynamically added clocks
would cause problems with this solution.

I think automatic clock parent and rate setting would need to be
delegated to a platform-specific layer, and I'm not sure how.  Maybe
allow platforms to override the set_rate ops in the clock tree
building blocks, or a new op that can determine the new tree
structure, and call the old set_rate op to set the registers.

yeah, getting these constraints solved automatically is pretty
expensive. some heuristics are probably indicated here.

-- 
torben Hohn