From patchwork Mon Mar 12 11:51:17 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Sascha Hauer X-Patchwork-Id: 7230 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 5EFCB23E0C for ; Mon, 12 Mar 2012 11:51:45 +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 07DB8A18449 for ; Mon, 12 Mar 2012 11:51:44 +0000 (UTC) Received: by iage36 with SMTP id e36so8720867iag.11 for ; Mon, 12 Mar 2012 04:51:44 -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:message-id:references:mime-version:content-type :content-disposition:content-transfer-encoding:in-reply-to :x-sent-from:x-url:x-irc:x-accept-language:x-accept-content-type :x-uptime:user-agent:x-sa-exim-connect-ip:x-sa-exim-mail-from :x-sa-exim-scanned:x-ptx-original-recipient:x-gm-message-state; bh=G+BK5+AoZ+meq6tW1DM1BAfWa2EMaljjIjhTLLlH2N4=; b=BK9Kg6iQWJj6FaEfBEEgKNv5gMuW2V3ySoQNW8PZ93p5ukdK2cNElUizZ3kjomtkoK hRAwJWdjO7LGwBG/1N2Ng79lhP0cYUZ8AFy3ZFzApyju70ibfOF5y0TMcOESZt0ffEyg cCuALSydxVIQMdnjtvJ99rRZ7lYkFYfpYLOUK+r5WCX774D+hvcerR2gbGIR5E/3XcKq LkQv69igKZS+TtvE644Q4SfdaWuNXpzBt7IRBXZopCNz3obEm/JOKTpYFuR7eujnObx+ 1AkKvMROej08DKQJlKZC1IRkZtExuG5DMXvNiES30cyFYJCJtNZAVuuDlJKEUCQ4Aqa/ w1vw== Received: by 10.50.45.228 with SMTP id q4mr18268934igm.58.1331553104466; Mon, 12 Mar 2012 04:51:44 -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.53.18 with SMTP id k18csp38439ibg; Mon, 12 Mar 2012 04:51:43 -0700 (PDT) Received: by 10.205.129.137 with SMTP id hi9mr4784062bkc.131.1331553101247; Mon, 12 Mar 2012 04:51:41 -0700 (PDT) Received: from metis.ext.pengutronix.de (metis.ext.pengutronix.de. [92.198.50.35]) by mx.google.com with ESMTPS id us6si7478247bkb.59.2012.03.12.04.51.40 (version=TLSv1/SSLv3 cipher=OTHER); Mon, 12 Mar 2012 04:51:41 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of sha@pengutronix.de designates 92.198.50.35 as permitted sender) client-ip=92.198.50.35; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of sha@pengutronix.de designates 92.198.50.35 as permitted sender) smtp.mail=sha@pengutronix.de Received: from dude.hi.pengutronix.de ([2001:6f8:1178:2:21e:67ff:fe11:9c5c]) by metis.ext.pengutronix.de with esmtp (Exim 4.72) (envelope-from ) id 1S73mc-0002y1-G0; Mon, 12 Mar 2012 12:51:22 +0100 Received: from sha by dude.hi.pengutronix.de with local (Exim 4.77) (envelope-from ) id 1S73mX-0005Jw-5A; Mon, 12 Mar 2012 12:51:17 +0100 Date: Mon, 12 Mar 2012 12:51:17 +0100 From: Sascha Hauer To: "Turquette, Mike" Cc: Russell King , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linaro-dev@lists.linaro.org, patches@linaro.org, Jeremy Kerr , Thomas Gleixner , Arnd Bergman , Paul Walmsley , Shawn Guo , Richard Zhao , Saravana Kannan , Magnus Damm , Rob Herring , Mark Brown , Linus Walleij , Stephen Boyd , Amit Kucheria , Deepak Saxena , Grant Likely , Andrew Lunn Subject: Re: [PATCH v6 2/3] clk: introduce the common clock framework Message-ID: <20120312115117.GN3852@pengutronix.de> References: <1331366064-1273-1-git-send-email-mturquette@linaro.org> <1331366064-1273-3-git-send-email-mturquette@linaro.org> <20120311113414.GM3852@pengutronix.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Sent-From: Pengutronix Hildesheim X-URL: http://www.pengutronix.de/ X-IRC: #ptxdist @freenode X-Accept-Language: de,en X-Accept-Content-Type: text/plain X-Uptime: 12:34:08 up 120 days, 19:21, 99 users, load average: 0.00, 0.02, 0.16 User-Agent: Mutt/1.5.21 (2010-09-15) X-SA-Exim-Connect-IP: 2001:6f8:1178:2:21e:67ff:fe11:9c5c X-SA-Exim-Mail-From: sha@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: patches@linaro.org X-Gm-Message-State: ALoCoQl0I9XJJ4SB8I2mjCuqn5ssnt7T5cxx6tI+XsR/N5r94x+Czj0qjFDP8kd7nZtnjikHB7NO On Sun, Mar 11, 2012 at 02:24:46PM -0700, Turquette, Mike wrote: > On Sun, Mar 11, 2012 at 4:34 AM, Sascha Hauer wrote: > > Hi Mike, > > > > I was about to give my tested-by when I decided to test the set_rate > > function. Unfortunately this is broken for several reasons. I'll try > > to come up with a fixup series later the day. > > I haven't tested clk_set_rate since V4, but I also haven't changed the > code appreciably. I'll retest on my end also. > > > On Fri, Mar 09, 2012 at 11:54:23PM -0800, Mike Turquette wrote: > >> +     /* find the new rate and see if parent rate should change too */ > >> +     WARN_ON(!clk->ops->round_rate); > >> + > >> +     new_rate = clk->ops->round_rate(clk->hw, rate, &parent_new_rate); > > > > You don't need a WARN_ON when you derefence clk->ops->round_rate anyway. > > Agreed that the WARN_ON should not be there. > > The v6 Documentation/clk.txt states that .round_rate is mandatory for > clocks that can adjust their rate, but I need to clarify this a bit > more. Ideally we want to be able to call clk_set_rate on any clock > and get a changed rate (if possible) by either adjusting that clocks > rate direction (e.g. a PLL or an adjustable divider) or by propagating > __clk_set_rate up the parents (assuming of course that > CLK_SET_RATE_PARENT flag is set appropriately). > > > Also, even when the current clock does not have a set_rate function it > > can still change its rate when the CLK_SET_RATE_PARENT is set. > > Correct. I'll clean this up and make the documentation a bit more > verbose on when .set_rate/.round_rate/.recalc_rate are mandatory. > > > > >> + > >> +     /* NOTE: pre-rate change notifications will stack */ > >> +     if (clk->notifier_count) > >> +             ret = __clk_notify(clk, PRE_RATE_CHANGE, clk->rate, new_rate); > >> + > >> +     if (ret == NOTIFY_BAD) > >> +             return clk; > >> + > >> +     /* speculate rate changes down the tree */ > >> +     hlist_for_each_entry(child, tmp, &clk->children, child_node) { > >> +             ret = __clk_speculate_rates(child, new_rate); > >> +             if (ret == NOTIFY_BAD) > >> +                     return clk; > >> +     } > >> + > >> +     /* change the rate of this clk */ > >> +     if (clk->ops->set_rate) > >> +             ret = clk->ops->set_rate(clk->hw, new_rate); > > > > I don't know the reason why you change the child clock before the parent > > clock, but it cannot work since this clock will change its rate based on > > the old parent rate and not the new one. > > This depends on the .round_rate implementation, which I admit to > having lost some sleep over. A clever .round_rate will request the > "intermediate" rate for a clock when propagating a request to change > the parent rate later on. Take for instance the following: > > pll @ 200MHz (locked) > | > parent @ 100MHz (can divide by 1 or 2; currently divider is 2) > | > child @ 25MHz (can divide by 2 or 4; currently divider is 4) > > If we want child to run at 100MHz then the desirable configuration > would be to have parent divide-by-1 and child divide-by-2. When we > call, > > clk_set_rate(child, 100MHz); > > Its .round_rate should return 50MHz, and &parent_new_rate should be > 200MHz. So 50MHz is an "intermediate" rate, but it gets us the > divider we want. And in fact 50MHz reflects reality because that will > be the rate of child until the parent propagation completes and we can > adjust parent's dividers. (this is one reason why I prefer for > pre-rate change notifiers to stack on top of each other). > > So now that &parent_new_rate is > 0, __clk_set_rate will propagate the > request up and parent's .round_rate will simply return 200MHz and > leave it's own &parent_new_rate at 0. This will change from > divide-by-2 to divide-by-1 and from this highest point in the tree we > will propagate post-rate change notifiers downstream, as part of the > recalc_rate tree walk. > > I have tested this with OMAP4's CPUfreq driver and I think, while > complicated, it is a sound way to approach the problem. Maybe the API > can be cleaned up, if you have any suggestions. I cannot see all implications this way will have. All this rate propagation is more complex than I thought it would be. I tried another approach on the weekend which basically does not try to do all in a single recursion but instead sets the rate in multiple steps: 1) call a function which calculates all new rates of affected clocks in a rate change and safes the value in a clk->new_rate field. This function returns the topmost clock which has to be changed. 2) starting from the topmost clock notify all clients. This walks the whole subtree even if a notfifier refuses the change. If necessary we can walk the whole subtree again to abort the change. 3) actually change rates starting from the topmost clocks and notify all clients on the way. I changed the set_rate callback to void. Instead of failing (what is failing in case of set_rate? The clock will still have some rate) I check for the result with clk_ops->recalc_rate. In the end what's more important than the implementation details is that it actually works. I created a little test module which sets up two cascaded dividers, tries to change the rate at the output and checks the result. We might want to add something like this (and maybe similar tests for reparenting and other stuff) to the generic clock framework later. It's good to have something generic to test the framework with without depending on some particular SoC. Sascha 8<---------------------------------------------------- clk: Add clock test module Signed-off-by: Sascha Hauer --- drivers/clk/clk-test.c | 169 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 169 insertions(+), 0 deletions(-) create mode 100644 drivers/clk/clk-test.c diff --git a/drivers/clk/clk-test.c b/drivers/clk/clk-test.c new file mode 100644 index 0000000..6f901fd --- /dev/null +++ b/drivers/clk/clk-test.c @@ -0,0 +1,169 @@ +#include +#include +#include +#include + +/* + * We build a fixed rate source clock with two cascaded 2bit dividers: + * + * f1 -> div1 -> div2 + * + * and try set the rate of div2 from 0Hz to f1 + 1 Hz. + * + * This makes the following resulting dividers possible: + * + * 1, 2, 4, 6, 8, 9, 12, 16 + * + * Sometimes there are different results possible due to integer maths. + * For example if we have an input frequency of 100Hz and request an + * output of 16Hz the best divider values would be 2 and 4 resulting + * in a real frequency of 12.5Hz. We also accept 2 and 3 as result + * because 100 / (3 * 2) = 16.6667 which is too high, but multiplying + * 16 * 3 * 2 results in 96Hz which is lower than 100Hz. + * + */ + +static unsigned long div1_reg, div2_reg; + +static struct clk *f1, *div1, *div2; + +static DEFINE_SPINLOCK(imx_ccm_lock); + +//#define FIXED_RATE 144 +#define FIXED_RATE 100 + +static inline struct clk *register_divider(const char *name, const char *parent, + void __iomem *reg, u8 shift, u8 width) +{ + return clk_register_divider(NULL, name, parent, CLK_SET_RATE_PARENT, + reg, shift, width, 0, &imx_ccm_lock); +} + +static unsigned long divs[] = { + 1, 2, 3, 4, 6, 8, 9, 12, 16, +}; + +static int check_rounded_rate(unsigned long want, unsigned long rounded) +{ + int i, j; + unsigned long best1 = 0, best2 = 0, now; + + if (want > FIXED_RATE) { + best1 = best2 = FIXED_RATE; + goto found2; + } + + for (i = 1; i <= 4; i++) { + for (j = 1; j <= 4; j++) { + now = FIXED_RATE / i / j; + if (now <= want && now > best1) + best1 = now; + } + } + + if (!best1) + best1 = (FIXED_RATE / 4) / 4; + + for (i = 0; i < ARRAY_SIZE(divs); i++) { + if (FIXED_RATE <= want * divs[i]) { + best2 = FIXED_RATE / divs[i]; + goto found2; + } + } + + if (!best2) + best2 = FIXED_RATE / 16; +found2: + if (rounded != best1 && rounded != best2) { + if (best1 == best2) + printk("clk-test: wanted rate %ld, best result would be %ld," + " but we have %ld\n", + want, best1, rounded); + else + printk("clk-test: wanted rate %ld, best result would be %ld or %ld," + " but we have %ld\n", + want, best1, best2, rounded); + return -EINVAL; + } + + return 0; +} + +static int check_real_rate(unsigned long rate) +{ + unsigned long realrate1, realrate2; + + realrate1 = FIXED_RATE / (div1_reg + 1) / (div2_reg + 1); + realrate2 = FIXED_RATE / ((div1_reg + 1) * (div2_reg + 1)); + + if (rate != realrate1 && rate != realrate2) { + if (realrate1 == realrate2) + printk("clk-test: divider returns rate %ld, but instead has %ld\n", + rate, realrate1); + else + printk("clk-test: divider returns rate %ld, but instead has %ld or %ld\n", + rate, realrate1, realrate2); + return -EINVAL; + } + + return 0; +} + +static int set_rate_test(void) +{ + unsigned long i, rate, rounded; + int ret, errors = 0; + + for (i = 0; i < FIXED_RATE + 1; i++) { + rounded = clk_round_rate(div2, i); + ret = check_rounded_rate(i, rounded); + if (ret) + errors++; + ret = clk_set_rate(div2, i); + if (ret) { + printk("%s: setting rate of div2 to %ld failed with %d\n", + __func__, i, ret); + errors++; + } + + rate = clk_get_rate(div2); + + if (rounded != rate) { + printk("clk_test: wanted %ld, core rounded to %ld, have now: %ld\n", + i, rounded, rate); + errors++; + } + + ret = check_real_rate(rate); + if (ret) + errors++; + } + + return errors; +} + +static int clk_test_init(void) +{ + int errors; + + f1 = clk_register_fixed_rate(NULL, "f1", NULL, CLK_IS_ROOT, FIXED_RATE); + div1 = register_divider("div1", "f1", &div1_reg, 0, 2); + div2 = register_divider("div2", "div1", &div2_reg, 0, 2); + + if (!f1 || !div1 || !div2) { + printk("clk-test: failed to register clocks\n"); + return -EINVAL; + } + + errors = set_rate_test(); + + printk("clk-test: finished with %d errors\n", errors); +#if 0 + /* Oh, oh */ + clk_unregister(f1); + clk_unregister(div1); + clk_unregister(div2); +#endif + return -EINVAL; +} +subsys_initcall(clk_test_init);