linux-kernel - Re: [PATCH v6 2/3] clk: introduce the common clock framework

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJOA=zMT4vK7WPF7iEH9kN6GHHAv-LChHoadzieqV-Wd3Qj=WQ@mail.gmail.com>
Date:	Wed, 14 Mar 2012 17:51:48 -0700
From:	"Turquette, Mike" <mturquette@...com>
To:	Sascha Hauer <s.hauer@...gutronix.de>
Cc:	Russell King <linux@....linux.org.uk>,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linaro-dev@...ts.linaro.org, patches@...aro.org,
	Jeremy Kerr <jeremy.kerr@...onical.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Arnd Bergman <arnd.bergmann@...aro.org>,
	Paul Walmsley <paul@...an.com>,
	Shawn Guo <shawn.guo@...escale.com>,
	Richard Zhao <richard.zhao@...aro.org>,
	Saravana Kannan <skannan@...eaurora.org>,
	Magnus Damm <magnus.damm@...il.com>,
	Rob Herring <rob.herring@...xeda.com>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Linus Walleij <linus.walleij@...ricsson.com>,
	Stephen Boyd <sboyd@...eaurora.org>,
	Amit Kucheria <amit.kucheria@...aro.org>,
	Deepak Saxena <dsaxena@...aro.org>,
	Grant Likely <grant.likely@...retlab.ca>,
	Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH v6 2/3] clk: introduce the common clock framework

On Tue, Mar 13, 2012 at 5:05 AM, Sascha Hauer <s.hauer@...gutronix.de> wrote:
> On Mon, Mar 12, 2012 at 08:16:36PM -0700, Turquette, Mike wrote:
>> On Mon, Mar 12, 2012 at 4:51 AM, Sascha Hauer <s.hauer@...gutronix.de> wrote:
>> > 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.
>
> The way described above works for me now, see this branch:
>
> git://git.pengutronix.de/git/imx/linux-2.6.git v3.3-rc6-clkv6-fixup
>
> You may not necessarily like it as it changes quite a lot in the rate
> changing code.

I tried that code and I really like it!  It is much more readable and
feels less "fragile" than the previous recursive __clk_set_rate.  I
did quite a bit of testing with this code today.  One of the tests
looks like this:

    pll         (adjustable to anything)
     |
clk_divider     (5 bits wide)
     |
   dummy        (no clk_ops)

The new code did a fine job arbitrating rates for the PLL and the
intermediate divider even if I put weird constraints on the PLL.  For
instance if I artificially limited it to a minimum of 600MHz and then
ran clk_set_rate(dummy, 300MHz) it would lock at 600MHz and set
clk_divider to divide-by-2.  Setting to 600MHz or more set the divider
back to 1 and relocked the PLL appropriately.  Pretty cool.

I also tested the notifiers with this code and they seem to function
properly.  I'll take this code in for v7.  Thanks a lot for this
helpful contribution.

I did find that MULT_ROUND_UP caused trouble for my PLL's round_rate
implementation.  Maybe my PLL code is fragile but a quick fix was to
make sure that we send the exact value we want to the round_rate code.
 I also feel this is more correct.  Let me know what you think:

8<---------------------------------------------------------------

commit 189fecedb175d0366759246c4192f45b0bc39a50
Author: Mike Turquette <mturquette@...aro.org>
Date:   Wed Mar 14 17:29:51 2012 -0700

    clk-divider.c: round the actual rate we care about

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index 86ca9cd..06ef4a0 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -47,12 +47,6 @@ static unsigned long clk_divider_recalc_rate(struct
clk_hw *hw,
 }
 EXPORT_SYMBOL_GPL(clk_divider_recalc_rate);

-/*
- * The reverse of DIV_ROUND_UP: The maximum number which
- * divided by m is r
- */
-#define MULT_ROUND_UP(r, m) ((r) * (m) + (m) - 1)
-
 static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate,
 		unsigned long *best_parent_rate)
 {
@@ -84,9 +78,9 @@ static int clk_divider_bestdiv(struct clk_hw *hw,
unsigned long rate,

 	for (i = 1; i <= maxdiv; i++) {
 		parent_rate = __clk_round_rate(__clk_get_parent(hw->clk),
-				MULT_ROUND_UP(rate, i));
+				(rate * i));
 		now = parent_rate / i;
-		if (now <= rate && now >= best) {
+		if (now <= rate && now > best) {
 			bestdiv = i;
 			best = now;
 			*best_parent_rate = parent_rate;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/