From patchwork Sun Apr 12 23:50:21 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mike Turquette X-Patchwork-Id: 47093 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-la0-f69.google.com (mail-la0-f69.google.com [209.85.215.69]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id 504CE2121F for ; Sun, 12 Apr 2015 23:50:41 +0000 (UTC) Received: by laat2 with SMTP id t2sf14138094laa.2 for ; Sun, 12 Apr 2015 16:50:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:content-type:mime-version :content-transfer-encoding:to:from:in-reply-to:cc:references :message-id:user-agent:subject:date:sender:precedence:list-id :x-original-sender:x-original-authentication-results:mailing-list :list-post:list-help:list-archive:list-unsubscribe; bh=HNDmY2NJ2y3O8wGlU+za2x7ilajxqrZF9ajr6cwQ9YM=; b=F5QFSYATQ1wqA3rHZ3kq6kr3mW6wFy9yOrAI7p5CRPb8aeXNQgQkGTMM0z+ZiQb4ep KKmOSVcpz9A24hsYzalyTkBK3ZoTc8zSUy+vui5N+w0i1mp4yIYudljzYEaNpg+9YJcs Ov/ktMKk597squ1tnzxboYV/h8cW+HFVvR0pGKXxlw2h1dllka/DzBFdgT94OH0VC2/q C3oh/DTUy5OoW3E12y30ua25Z4dFV0Cjyn87salWufvIdS88JUkhAAB71MBCx5X7x6EM d3xiCzKJTBBNE0tnXEghU7dfsczqtHldtLLu4xnggxqvQqsE+6Cgv0tSZfuf8d4U6bEt qqlQ== X-Gm-Message-State: ALoCoQlVXETS6AtD3v9ZfUOYoUWGD6k15273uFeLOTH9iIAr8xj23IlybDHpWsDqtHi+qRqmk2hz X-Received: by 10.152.37.101 with SMTP id x5mr1745349laj.5.1428882640234; Sun, 12 Apr 2015 16:50:40 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.87.204 with SMTP id ba12ls667163lab.41.gmail; Sun, 12 Apr 2015 16:50:39 -0700 (PDT) X-Received: by 10.153.4.43 with SMTP id cb11mr11049899lad.122.1428882639848; Sun, 12 Apr 2015 16:50:39 -0700 (PDT) Received: from mail-lb0-f169.google.com (mail-lb0-f169.google.com. [209.85.217.169]) by mx.google.com with ESMTPS id kw12si7062761lac.3.2015.04.12.16.50.39 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 12 Apr 2015 16:50:39 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.169 as permitted sender) client-ip=209.85.217.169; Received: by lbbqq2 with SMTP id qq2so46867714lbb.3 for ; Sun, 12 Apr 2015 16:50:38 -0700 (PDT) X-Received: by 10.152.19.199 with SMTP id h7mr11149579lae.32.1428882638740; Sun, 12 Apr 2015 16:50:38 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.112.67.65 with SMTP id l1csp911540lbt; Sun, 12 Apr 2015 16:50:37 -0700 (PDT) X-Received: by 10.67.15.102 with SMTP id fn6mr22123330pad.120.1428882636512; Sun, 12 Apr 2015 16:50:36 -0700 (PDT) Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id gm10si13066351pbd.148.2015.04.12.16.50.35; Sun, 12 Apr 2015 16:50:36 -0700 (PDT) Received-SPF: none (google.com: linux-kernel-owner@vger.kernel.org does not designate permitted sender hosts) client-ip=209.132.180.67; Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752812AbbDLXub (ORCPT + 27 others); Sun, 12 Apr 2015 19:50:31 -0400 Received: from mail-ie0-f181.google.com ([209.85.223.181]:32796 "EHLO mail-ie0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752269AbbDLXua convert rfc822-to-8bit (ORCPT ); Sun, 12 Apr 2015 19:50:30 -0400 Received: by iebmp1 with SMTP id mp1so52901800ieb.0 for ; Sun, 12 Apr 2015 16:50:29 -0700 (PDT) X-Received: by 10.50.4.97 with SMTP id j1mr12823520igj.46.1428882629429; Sun, 12 Apr 2015 16:50:29 -0700 (PDT) Received: from localhost (pool-71-119-96-202.lsanca.fios.verizon.net. [71.119.96.202]) by mx.google.com with ESMTPSA id fs5sm908761igb.0.2015.04.12.16.50.27 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Sun, 12 Apr 2015 16:50:28 -0700 (PDT) MIME-Version: 1.0 To: Boris Brezillon , "Stephen Boyd" , "Tomeu Vizoso" From: Michael Turquette In-Reply-To: <20150327004054.2f6f34ee@bbrezillon> Cc: "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" References: <20150327004054.2f6f34ee@bbrezillon> Message-ID: <20150412235021.19585.27431@quantum> User-Agent: alot/0.3.5 Subject: Re: Propagating clock rate constraints Date: Sun, 12 Apr 2015 16:50:21 -0700 Sender: linux-kernel-owner@vger.kernel.org Precedence: list List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: mturquette@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.169 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 List-Post: , List-Help: , List-Archive: List-Unsubscribe: , Quoting Boris Brezillon (2015-03-26 16:40:54) > Hello, > > I recently had a problem with the at91 clk implementation: the > programmable clock driver is not forwarding set_rate() requests to its > parent, meaning that, if the PLLB is set to 0, it will choose another > parent which might be inappropriate to generate the requested clock. > ITOH, if we authorize forwarding set_rate requests without taking care > of constraints imposed by other users of the PLLB we might end-up > with a erroneous rate for the USB clock. > > I thought using set_rate_range() would be a good idea to address this > issue, but apparently rate constraints are not propagated to parent > clocks, and PLLB is never exposed to devices (it is accessed through > clk muxers, divisors, etc). > > Is there a plan to support propagating rate constraints to parent > clocks ? Thank for the diff below. Your problem needs to be addressed, though I'm not sure if propagating rate constraints up the tree is the right way to do it. Looking through the code I notice that we don't return an error code from .determine_rate callbacks in clk_calc_new_rates if the min/max constraints are out of bounds. We do this for .round_rate since that function does not take min/max rate as inputs. Furthermore .determine_returns a long, and we don't check for any kind of error here. Sigh. See below snippet: /* find the closest rate and parent clk/rate */ if (clk->ops->determine_rate) { parent_hw = parent ? parent->hw : NULL; new_rate = clk->ops->determine_rate(clk->hw, rate, min_rate, max_rate, &best_parent_rate, &parent_hw); parent = parent_hw ? parent_hw->core : NULL; } else if (clk->ops->round_rate) { new_rate = clk->ops->round_rate(clk->hw, rate, &best_parent_rate); if (new_rate < min_rate || new_rate > max_rate) return NULL; See that for round_rate we bail out if the rate change operation will go outside of our bounds? Seems we don't do that for determine_rate. Maybe doing so would at least bail gracefully for you case? Stephen, Tomeu, any thoughts on this? Completely untested diff below: > > Here is an example where propagating rate constraints would be useful: > > The USB controller wants a 48MHz clock and the audio controller needs a > programmable clock running at 12MHz. > > 1/ The USB driver calls set_rate() on the USB clock, which in turn > configures the PLLB to generate a 96MHz signal and sets its divisor > to 2. > Then it should call set_rate_range(usb_clk, 48Mhz, 48MHz) to prevent > anyone from messing up with his USB clock. > > 2/ The audio controller calls set_rate on the prog clock, which in > turn configures the PLLB to generate a 12MHz. > Since nobody explicitly set a constraint on the PLLB, the set_rate() > call should work, right ? > If it works, after this call, the USB clk will generate a 6MHz > signal instead of 48MHz one. I do not think propagating constraints up the tree is the correct way to solve this this. If you propagate a min,max constraint of (48,48) or (96,96) up to PLLB it may not be the right thing for all cases. Additionally, to get the right 12MHz rate that your audio clock wants, the clock framework will have to become a LOT smarter and end up being able to solve for lots cases at run-time (and it will be non-trivial run-time complexity to do this solving). I don't mean to be hand-wavey, so I'll try to explain a bit more. Today the clock framework tries to "solve" for complex rate relationships by propagating rates up the tree. This is somewhat fragile: 1) it only considers the rate for the clock that was passed into clk_set_rate, as it pre-dates any rate constraint stuff. This is the same he-who-writes-last-wins problem that most clock framework implementations have suffered. 2) there is no downwards traversal of the tree to try different combinations. If child_clk propagates a rate request up to parent_clk, and parent_clk can't do that rate then we bail. There is no logic where parent_clk can try to "suggest" a more optimal scenario, or try to arbitrate an agreeable solution between multiple downstream child clocks which consume parent_clk's rate. I don't think we should strive for this level complexity in the kernel, because it is too ... complex. > > > I started to look at how rate constraint propagation could be handled: > here [1] is a quick&dirty/untested patch adding several things to deal > with such cases. > The idea is to declare each clock as a clk user of its parent, and then > modify rate range appropriately so that all children clk constraints are > taken into account by the parent clock when a rate change is requested. > > Anyway, just tell me if you're already working on a solution for this > problem or if you need any help with that ? I appreciate the feedback on the constraint propagation and the diff you provided. Hopefully Stephen or Tomeu can comment further on it. More ideas are welcome for the rate constraint stuff in general. As for your case of needing to support a sane configuration where multiple clocks consume the same parent clock, I am slowly working on a clock driver-defined look-up table approach for this which may be helpful. Think of it as the configuration file that a system integrator is responsible for generating for each device to insure that the various use cases can have sane clock configuration. No promises on delivery dates though :-( Regards, Mike > > Best Regards, > > Boris > > [1]http://code.bulix.org/dcm4c8-88137 > > -- > Boris Brezillon, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com --- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index fa5a00e..482e906 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -1638,6 +1638,8 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *clk, max_rate, &best_parent_rate, &parent_hw); + if (new_rate < min_rate || new_rate > max_rate) + return NULL; parent = parent_hw ? parent_hw->core : NULL; } else if (clk->ops->round_rate) { new_rate = clk->ops->round_rate(clk->hw, rate,