Hello Sascha,
On Sat, 17 Mar 2012, Sascha Hauer wrote:
> On Fri, Mar 16, 2012 at 04:21:17PM -0600, Paul Walmsley wrote:
>
> > If the common clock code is to go upstream now, it should be marked as
> > experimental.
>
> No, please don't do this. This effectively marks the architectures using
> the generic clock framework experimental. We can mark drivers as 'you
> have been warned', but marking an architecture as experimental is the
> wrong sign for both its users and people willing to adopt the framework.
> Also we get this:
>
> warning: (ARCH_MX1 && MACH_MX21 && ARCH_MX25 && MACH_MX27) selects COMMON_CLK which has unmet direct dependencies (EXPERIMENTAL)
>
> (and no, I don't want to support to clock frameworks in parallel)
It sounds as if your objection is with CONFIG_EXPERIMENTAL. If that is
indeed your objection, I personally have no objection to simply marking
the code experimental in the Kconfig text. (Patch at the bottom of this
message.)
We need to indicate in some way that the existing code and API is very
likely to change in ways that could involve quite a bit of work for
adopters.
> > This is because we know the API is not well-defined, and
> > that both the API and the underlying mechanics will almost certainly need
> > to change for non-trivial uses of the rate changing code (e.g., DVFS with
> > external I/O devices).
>
> Please leave DVFS out of the game. DVFS will use the clock framework for
> the F part and the regulator framework for the V part, but the clock
> framework should not get extended with DVFS features. The notifiers we
> currently have in the clock framework should give enough information
> for DVFS implementations.
Sadly, that's not so.
Consider a CPUFreq driver as one common clock framework user. This driver
will attempt to repeatedly change the rate of a clock. Changing that
clock's rate may also involve changing the rate of several other clocks
used by active devices. So drivers for these devices will need to
register rate change notifiers. The notifier callbacks might involve
heavyweight work, such as asserting flow control to an
externally-connected device.
Suppose now that the last registered device in the notifier chain cannot
handle the frequency transition and must abort it. This in turn will
require notifier callbacks to all of the previously-notified devices to
abort the change. And then shortly after that, CPUFreq is likely to
request the same frequency change again: hammering a notifier chain that
can never succeed.
Bad enough. We know at least one way to solve this problem. We can use
something like the clk_{block,allow}_changes() functions that have been
discussed for some time now. But these quickly reveal another problem in
the existing API. By default, when there are multiple enabled users of a
clock, another entity is allowed to change the clock's rate, or the rate
of any parent of that clock (!).
This has several implications. One that is significant is that for any
non-trivial clock tree, any driver that cares about its clock's rate will
need to implement notifier callbacks. This is because the driver has no
way of knowing if or when any other code on the system will attempt to
change that clock's rate or source. Or any parent clock's rate or source
might change. Should we really force all current drivers using the clock
code to implement these callbacks? Or can we restrict the situations in
which the clock's rate or parent can change by placing restrictions on the
API? But then, driver code may need to be rewritten, and behavior
assumptions may change.
> Even if they don't and we have to change something here this will have
> no influence on the architectures implementing their clock tree with the
> common clock framework.
That sounds pretty confident. Are you sure that the semantics are so well
understood?
For example, should we allow clk_set_rate() on disabled clocks? How about
prepared, but disabled clocks? If so, what exactly should the clock
framework do in these circumstances? Should notifier callbacks go out
immediately to registered callbacks? Or should those callbacks be delayed
until the clock is prepared or enabled? How should that work when
clk_enable() cannot block? And are you confident that any other user of
the clock framework will answer these undefined questions in the same way
you would?
The above questions are simply "scratching the surface." (Just as
examples, there are still significant questions about locking, reentrancy,
and so on - [1] is just one example)
These questions have reasonable answers that I think can be mostly aligned
on. Thinking through the use-cases, and implications, and answering them,
should have been the first task in working on the common clock code. I am
sorry to say -- and perhaps this is partially my fault -- that it seems as
if most people are not even aware that these questions exist, despite
discussions at several conferences and on the mailing lists.
Anyway. It is okay if we want to have some starter common clock framework
in mainline; this is why deferring the merge hasn't been proposed. But
the point is that anyone who bases their code or platform on the common
clock framework needs to be aware that, to satisfy one of its major
use-cases, the behavior and/or API of the common clock code may need to
change significantly.
Explicitly stating this is not only simple courtesy to its users, many of
whom won't have been privy to its development. It also is intended to
make the code easier to change once it reaches mainline. Once several
platforms start using it, there will naturally be resistance and
conservatism in changing its semantics and interface. Many drivers may
have to be changed, across many different maintainers. And power
management code may well need to be revalidated on the platforms that use
it. PM code, in my opinion, is generally the most difficult code to debug
in the kernel.
So, until the API is well-defined and does all that it needs to do for its
major users, we should at least have something like the following patch
applied.
- Paul
1. King, Russell. _Re: [PATCH v3 3/5] clk: introduce the common clock
framework_. 2 Dec 2011 20:23:06 +0000. linux-omap mailing list.
http://www.spinics.net/lists/linux-omap/msg61024.html
From: Paul Walmsley <paul@xxxxxxxxx>
Date: Tue, 20 Mar 2012 17:19:06 -0600
Subject: [PATCH] clk: note that the common clk code is still subject to
significant change
Indicate that the common clk code and API is still likely to require
significant change. The API is not well-defined and both it and the
underlying mechanics are likely to need significant changes to support
non-trivial uses of the rate changing code, such as DVFS with external
I/O devices. So any platforms that switch their implementation over
to this may need to revise much of their driver code and revalidate
their implementations until the behavior of the code is
better-defined.
A good time for removing this help text would be after at least two
platforms that do DVFS on groups of external I/O devices have ported
their clock implementations over to the common clk code.
Signed-off-by: Paul Walmsley <paul@xxxxxxxxx>
Cc: Mike Turquette <mturquette@xxxxxx>
---
drivers/clk/Kconfig | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 2eaf17e..dd2d528 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -21,6 +21,11 @@ menuconfig COMMON_CLK
this option for loadable modules requiring the common clock
framework.
+ The API and implementation enabled by this option is still
+ incomplete. The API and implementation is expected to be
+ fluid and subject to change at any time, potentially
+ breaking existing users.
+
If in doubt, say "N".
if COMMON_CLK
--
1.7.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
