From patchwork Tue Mar 20 23:31:33 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paul Walmsley X-Patchwork-Id: 7377 Return-Path: X-Original-To: patchwork@peony.canonical.com Delivered-To: patchwork@peony.canonical.com Received: from fiordland.canonical.com (fiordland.canonical.com [91.189.94.145]) by peony.canonical.com (Postfix) with ESMTP id 945B923E13 for ; Tue, 20 Mar 2012 23:31:43 +0000 (UTC) Received: from mail-iy0-f180.google.com (mail-iy0-f180.google.com [209.85.210.180]) by fiordland.canonical.com (Postfix) with ESMTP id 3AAF6A181AE for ; Tue, 20 Mar 2012 23:31:43 +0000 (UTC) Received: by iage36 with SMTP id e36so948190iag.11 for ; Tue, 20 Mar 2012 16:31:42 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-forwarded-to:x-forwarded-for:delivered-to:received-spf:date:from :to:cc:subject:in-reply-to:message-id:references:user-agent :mime-version:content-type:x-gm-message-state; bh=Y7Rn97488GVkVLkMwvt8GbWBkmtV7jWahc0dFPjqcBA=; b=SXCac4J/b/Z4IWWh8Jmb3o7JuzvlWONPJAn48Vqb+roiYSnteJNzGcRKzb+jOosiTO RDaCIa6UcruS2vrbMnEJhQx9N5KjjM9e5sNUznQ8KC47/ZzpDeOl5lVGb56qcjVT1KHI G5WMU0WEEyF7QTGuVuCN4B+0PVUNoHPzZ7UAMt9r9T37EeT0SuMoPw0D4Zb0C117DSNS GRmQR5lsQ54oESeSZryL4K4jAGWjbzsMB4cuUed2OXKEFhkIMHf2JJUiGMJMtVoPFqck W7Fuzit7WuZHkiiSkW6nS5KRExiUyYnixvH70w05pcmiU/weRyy1SYtInUhGB5mZFy5N yr6g== Received: by 10.50.155.229 with SMTP id vz5mr1348034igb.12.1332286302677; Tue, 20 Mar 2012 16:31:42 -0700 (PDT) X-Forwarded-To: linaro-patchwork@canonical.com X-Forwarded-For: patch@linaro.org linaro-patchwork@canonical.com Delivered-To: patches@linaro.org Received: by 10.231.203.79 with SMTP id fh15csp15357ibb; Tue, 20 Mar 2012 16:31:40 -0700 (PDT) Received: by 10.224.73.77 with SMTP id p13mr3189041qaj.96.1332286297422; Tue, 20 Mar 2012 16:31:37 -0700 (PDT) Received: from utopia.booyaka.com (utopia.booyaka.com. [72.9.107.138]) by mx.google.com with ESMTPS id ez4si2889858qab.107.2012.03.20.16.31.33 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 20 Mar 2012 16:31:37 -0700 (PDT) Received-SPF: pass (google.com: domain of pwsan@booyaka.com designates 72.9.107.138 as permitted sender) client-ip=72.9.107.138; Authentication-Results: mx.google.com; spf=pass (google.com: domain of pwsan@booyaka.com designates 72.9.107.138 as permitted sender) smtp.mail=pwsan@booyaka.com Received: (qmail 20339 invoked by uid 1019); 20 Mar 2012 23:31:33 -0000 Date: Tue, 20 Mar 2012 17:31:33 -0600 (MDT) From: Paul Walmsley To: Sascha Hauer cc: Arnd Bergmann , linux-arm-kernel@lists.infradead.org, Amit Kucheria , Nicolas Pitre , linaro-dev@lists.linaro.org, Linus Walleij , Grant Likely , Saravana Kannan , Jeremy Kerr , Mike Turquette , Mike Turquette , Magnus Damm , Deepak Saxena , patches@linaro.org, Rob Herring , Russell King , Thomas Gleixner , Richard Zhao , Shawn Guo , Linus Walleij , Mark Brown , Stephen Boyd , linux-omap@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v7 1/3] Documentation: common clk API In-Reply-To: <20120316234706.GJ3852@pengutronix.de> Message-ID: References: <1331878280-2758-1-git-send-email-mturquette@linaro.org> <201203161218.05125.arnd.bergmann@linaro.org> <201203162057.58706.arnd@arndb.de> <20120316234706.GJ3852@pengutronix.de> User-Agent: Alpine 2.00 (DEB 1167 2008-08-23) MIME-Version: 1.0 X-Gm-Message-State: ALoCoQl2ZeCfEqrfLePTCr9H25DsRaXkCrrogx0T1+QViX2VbSjTzoLHUONdu1iw4W+c+WAosIbu 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 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 Cc: Mike Turquette --- 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