From patchwork Wed Apr 3 21:34:12 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mike Turquette X-Patchwork-Id: 2389351 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from casper.infradead.org (casper.infradead.org [85.118.1.10]) by patchwork1.kernel.org (Postfix) with ESMTP id AA3423FC71 for ; Wed, 3 Apr 2013 21:42:56 +0000 (UTC) Received: from merlin.infradead.org ([2001:4978:20e::2]) by casper.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1UNVSI-0002Tr-Ew for patchwork-linux-arm@patchwork.kernel.org; Wed, 03 Apr 2013 21:42:54 +0000 Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1UNVK3-0002kz-8X; Wed, 03 Apr 2013 21:34:23 +0000 Received: from mail-pd0-f181.google.com ([209.85.192.181]) by merlin.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1UNVJz-0002kT-Gp for linux-arm-kernel@lists.infradead.org; Wed, 03 Apr 2013 21:34:21 +0000 Received: by mail-pd0-f181.google.com with SMTP id y10so1055818pdj.40 for ; Wed, 03 Apr 2013 14:34:18 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-received:content-type:mime-version:content-transfer-encoding:to :from:in-reply-to:cc:references:message-id:user-agent:subject:date :x-gm-message-state; bh=rXC2GOY9nNgseFpYLKsBm7ShHbezcAY7J4vIzk/ldZk=; b=cYUu0pw95rbyyILrOxhHkykFnibE8E+rcbvZNBVH6Ciw0XmnmDbiZbSwMsqmGVJHDn AQ5Bh/bbY9ZFJ7+TwlPdZLeK5gv04e/xaLCeaxf9gsNBztSa3thkCqtT8abVYvDbrvuq 2hKQrhfIeXVN/FFomiMYc9OHR2ZVOGNhJhupzOxrx+U4whkTichg4eNfguwtDwBh5pL5 otEM76Ux9QzzphAd0Q2zt9NA3dsiP9IPj/C+OEpaNs/U/yR+W0C82XAEnithsTwv4bV2 aOFDnlxDW/u4vDSvQr1DpHu2Pom0QZV1Zy1z5g1RW/ZmfBR1TVJAdOSIRpieSw4m/Hx4 49dg== X-Received: by 10.66.234.198 with SMTP id ug6mr5751291pac.43.1365024857925; Wed, 03 Apr 2013 14:34:17 -0700 (PDT) Received: from localhost (adsl-69-228-93-79.dsl.pltn13.pacbell.net. [69.228.93.79]) by mx.google.com with ESMTPS id ce16sm8439463pac.5.2013.04.03.14.34.14 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Wed, 03 Apr 2013 14:34:16 -0700 (PDT) MIME-Version: 1.0 To: James Hogan , Stephen Boyd From: Mike Turquette In-Reply-To: References: <1363967031-22781-1-git-send-email-james.hogan@imgtec.com> <515B8EAD.8020109@codeaurora.org> Message-ID: <20130403213412.3383.84823@quantum> User-Agent: alot/0.3.3+ Subject: Re: [RFC PATCH v1 0/3] clk: implement remuxing during set_rate Date: Wed, 03 Apr 2013 14:34:12 -0700 X-Gm-Message-State: ALoCoQnfwfeKETxdrpJ5IFMAnnEbUt8M7tAavuZoi0znBDjjxV7D1x8y1pAw4PvsAvxmuikyvihu X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20130403_173419_732920_203BE5F6 X-CRM114-Status: GOOD ( 49.98 ) X-Spam-Score: -1.9 (-) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-1.9 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at http://www.dnswl.org/, no trust [209.85.192.181 listed in list.dnswl.org] -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: James Hogan , Sascha Hauer , Chao Xie , LKML , linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org Quoting James Hogan (2013-04-03 14:08:17) > On 3 April 2013 03:06, Stephen Boyd wrote: > > > > On 03/22/13 08:43, James Hogan wrote: > > > This patchset adds support for automatic selection of the best parent > > > for a clock mux, i.e. the one which can provide the closest clock rate > > > to that requested. It can be controlled by a new CLK_SET_RATE_REMUX flag > > > so that it doesn't happen unless explicitly allowed. > > > > > > This works by way of adding a parameter to the round_rate clock op which > > > allows the clock driver to optionally select a different parent index. > > > This is used in clk_calc_new_rates to decide whether to initiate a > > > set_parent operation. This would obviously require the argument to be > > > added to all users of round_rate, something this patchset doesn't do as > > > I'm not sure if it's really the preferred method (hence the RFC). > > > > > > An alternative would be to add a new callback, but that would complicate > > > the code in clk.c somewhat. I suppose it would also be possible for the > > > round_rate callback to call a function to set a struct clk member to > > > mark that the parent should change (it's all within mutex protected > > > code after all). Comments anyone? > > > > It seems like you want to be able to call clk_set_rate() on a mux? > > Yes, that's right. > > > Usually when this comes up people say you should use clk_set_parent() > > but I have a real use case (see below) and maybe you do too. > > Agreed, it shouldn't be up to generic drivers to know what clocks to > reparent to in order to get a given frequency. > > > > > This patch set caught my eye because we need to be able to switch > > parents during clk_set_rate() on MSM. We usually have two or three PLLs > > feed into a mux that might feed into a divider that feeds into one or > > two gates. I want clk_set_rate() on these gates to propagate up through > > the divider to the mux and then have the mux switch depending on the rate. > > We have a similar sort of thing here too. With the hardware I'm > working on, various clocks can be sourced from external oscillators, > the system clock (usually derived form a PLL), or in some cases from > other more complex clocks and PLLs, and are often fed into dividers > and gates which are the clocks that drivers are provided with. > > > > > I would like to get away without writing any new code beyond the ops > > that are already there though. Well, I may have to write one new op > > because I have hardware that can change the mux and divider at the same > > time. > > > > Can we push the code into the core? Perhaps by doing what you do in > > clk-mux.c directly in clk_calc_new_rates()? We could store a new_parent > > pointer in struct clk along with the new_rate when we find the best rate > > and then when we propagate rates we can propagate the parent switch. > > This part would be a little tricky if a clock has both a set_parent and > > a set_rate op; what do you call first? The set_rate op? The set_parent > > op? It matters for my hardware. We probably need to introduce a new > > set_rate_and_parent op in case both parts of the equation change so that > > the driver can do it atomically if desired (for my hardware) or in the > > correct order (this part could be two generic functions like > > generic_clk_rate_parent() and generic_clk_parent_rate() for the two > > different orders, or two flags and the logic in the core, whatever). > > This has been discussed several times in the past and the core definitely should handle this in a similar fashion to the way clk->ops->recalc_rate can request a better parent rate. One thought to not break compatibility might be something like: Obviously lots of stuff is missing there, but it would not force every user of .round_rate to add a new best_parent_clk parameter. Perhaps .round_rate could be phased out in the future... or perhaps it would be better to just do a mass update of all .round_rate implementations. It is worth pointing out that the top-level clk_round_rate api would not be changed; the idea above only affects clk drivers using the common struct clk. > > I like keeping it in the core because we wouldn't need to change the > > round_rate() function signature, it would be called in a loop with > > different parent rates, and we wouldn't have to touch the clk-mux code > > at all because it would all be done generically. Plus I get almost > > everything for free for my hardware, I just need to write a new op that > > does that atomic mux and rate switch. What do you think? > Flags might be necessary to favor rate-change versus reparent, but that is a small detail. > I like this idea a lot. It feels cleaner, and as far as I can tell at > a glance it should be possible. Thanks for the feedback, and comments > about atomic changes, I'll certainly bear those issues in mind so I > don't make it difficult to add that callback. I think it's probably > best in the non-atomic case to default to the existing behaviour (set > parent rate before child rate, which translates to setting parent > before setting rate), and if somebody comes along with a different use > case they can add the necessary flag or whatever to alter the > behaviour. > > I won't get any time to try this out for a couple of weeks as I'm on > paternity leave at the moment. > Congratulations! Enjoy your leave, or at least try to not become sleep deprived. I'll add this to my stack but it's pretty far and away compared to other items. Feel free to beat me to it if you like. Regards, Mike > Cheers > James diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 0230c9d..0708fa3 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -1043,12 +1043,17 @@ static struct clk *clk_calc_new_rates(struct clk *clk, unsigned long rate) /* never propagate up to the parent */ if (!(clk->flags & CLK_SET_RATE_PARENT)) { - if (!clk->ops->round_rate) { + if (clk->ops->determine_rate) { + new_rate = clk->ops->determine_rate(clk->hw, rate, + &best_parent_rate, &best_parent_clk); + goto out; + } else if (clk->ops->round_rate) { + new_rate = clk->ops->round_rate(clk->hw, rate, &best_parent_rate); + goto out; + } else { clk->new_rate = clk->rate; return NULL; } - new_rate = clk->ops->round_rate(clk->hw, rate, &best_parent_rate); - goto out; } /* need clk->parent from here on out */ diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 9fdfae7..1a19186 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -126,6 +126,9 @@ struct clk_ops { unsigned long parent_rate); long (*round_rate)(struct clk_hw *hw, unsigned long, unsigned long *); + s64 (*determine_rate)(struct clk_hw *hw, unsigned long rate, + unsigned long *best_parent_rate, + struct clk *best_parent_clk); int (*set_parent)(struct clk_hw *hw, u8 index); u8 (*get_parent)(struct clk_hw *hw); int (*set_rate)(struct clk_hw *hw, unsigned long,