From patchwork Tue Feb 25 00:22:14 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Mike Turquette X-Patchwork-Id: 3712321 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id C547B9F2F7 for ; Tue, 25 Feb 2014 00:22:54 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id D065A2015A for ; Tue, 25 Feb 2014 00:22:53 +0000 (UTC) Received: from casper.infradead.org (casper.infradead.org [85.118.1.10]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 9E7F120154 for ; Tue, 25 Feb 2014 00:22:52 +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 1WI5nN-00036h-1F; Tue, 25 Feb 2014 00:22:49 +0000 Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1WI5nK-0007Yi-Gj; Tue, 25 Feb 2014 00:22:46 +0000 Received: from mail-pd0-f172.google.com ([209.85.192.172]) by merlin.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1WI5nG-0007XS-0q for linux-arm-kernel@lists.infradead.org; Tue, 25 Feb 2014 00:22:44 +0000 Received: by mail-pd0-f172.google.com with SMTP id p10so2563830pdj.31 for ; Mon, 24 Feb 2014 16:22:20 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:content-type:mime-version :content-transfer-encoding:to:from:in-reply-to:cc:references :message-id:user-agent:subject:date; bh=OoB/+JBAyBIU5xOW83FyNiLDqnoiZNPEhsTesofaYCw=; b=HAumPtjl/QUOyflTZPsAcufN1uTcVDDULSb3QijEeM7/W18J66sAtR6OrnhjcorzdA yCLFhzQ2nBdp/2/eLUxRhZQprZjMc2hwypcGS4PF8SaHk5+xHMh52um8ekS4sGIUcGM0 md97Ed/4Fs1Bm4ODwynjOjnvuN+Lkgu5dG+asafbVVtE7U5BfGL7mwjMagszzy/aAJYU /QIkhDskaFLD0OLGsi1QJfUBDx1qD6NEldWACU2G4enVgiu2RPTdQL4l8LHOngVpDxwf j13qbQ3CSIwbWLzJvBvhnnjUpgs4Hv/UOJGNAtR43D1QClcM4O5T7fkYml8J5CMIyIMY aCsw== X-Gm-Message-State: ALoCoQmnVMTser1wEHnGE6RhdzA0vQG3KDF7/CKNDPLGZmN7OKfmYyCpwV6ZNBli7GAOmoMQevlD X-Received: by 10.66.162.74 with SMTP id xy10mr28806749pab.4.1393287740088; Mon, 24 Feb 2014 16:22:20 -0800 (PST) Received: from localhost ([2601:9:5900:1fe:ca60:ff:fe0a:8a36]) by mx.google.com with ESMTPSA id vf7sm4062481pbc.5.2014.02.24.16.22.18 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Mon, 24 Feb 2014 16:22:19 -0800 (PST) MIME-Version: 1.0 To: =?utf-8?q?S=C3=B6ren_Brinkmann?= , From: Mike Turquette In-Reply-To: <32a83aee-624b-47ef-a13e-1a9d993005c0@CH1EHSMHS039.ehs.local> References: <20140224201046.GA13155@katana> <8e8ce023-b9a4-41a5-b06c-0f6d934f22e6@VA3EHSMHS020.ehs.local> <20140224230532.22529.7958@quantum> <32a83aee-624b-47ef-a13e-1a9d993005c0@CH1EHSMHS039.ehs.local> Message-ID: <20140225002214.22529.23643@quantum> User-Agent: alot/0.3.5 Subject: Re: ccf: where to put constraints? Date: Mon, 24 Feb 2014 16:22:14 -0800 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20140224_192242_239780_C5440A0D X-CRM114-Status: GOOD ( 34.56 ) X-Spam-Score: -1.9 (-) Cc: Laurent Pinchart , linux-arm-kernel@lists.infradead.org, Wolfram Sang 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 X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Quoting Sören Brinkmann (2014-02-24 15:11:55) > On Mon, 2014-02-24 at 03:05PM -0800, Mike Turquette wrote: > > Quoting Sören Brinkmann (2014-02-24 13:21:58) > > > Hi Wolfram, > > > > > > On Mon, 2014-02-24 at 09:10PM +0100, Wolfram Sang wrote: > > > > Hi, > > > > > > > > Where do I put constraints when using the CCF? I have a CPU and GPU > > > > clock derived from the same PLL. Both clocks have their own divider. > > > > Now, there is the additional constraint that the CPU clock must not be > > > > lower than the GPU clock. Do I add checks in the set_rate functions? > > > > Feels like the wrong layer, but not checking it and blindly relying on > > > > somebody else does not feel correct, too. How to do it best? > > > > > > I don't know if it's the best or only way, but so far, I did things like > > > that with clock notifiers. > > > > > > I.e. when a clock consumer needs to be notified about its input clock > > > being changed it registers a clock notifier. In that notifier callback > > > it then can react to the rate change. E.g. adjust dividers to stay > > > within legal limits or reject the change completely. > > > > Yes, this is why the notifiers were created. A nice side effect of this > > is that the code can live outside your clock driver. Often times the > > clocks are quite capable of running at these speeds, but the problem is > > the IP blocks (CPU & GPU in Wolfram's case) would be out of spec. It is > > arguable that this logic does not belong in the clock driver but instead > > in some cpufreq driver or something like it. > > > > The clock notifiers make it easy to put this logic wherever you like and > > you can even veto clock rate changes. > > Right, that's how I have it. If a device driver needs to enforce some > policy on its clock, it implements a clock notifier callback. > > The drawback though, as I see it, it makes interactions hard to > understand. With such a scheme, rate changes may fail and the cause is > not always obvious - often hidden really well. Usually all you get is a > message from the CCF that the rate change for clock failed, but > if your notifier callback isn't verbose, you can search forever for the > cause of that failure. > > On our internal repo I had it a few times that "bugs" get assigned to the > wrong piece. E.g. cpufreq, even though that works perfectly well and > correct on its own, but some other device enforced some policy through a > notifier. Yes, debugging across notifiers is notoriously annoying. How about something like the following patch? Regards, Mike From f5b30cad56b3439ac127b43148827a95f6391a92 Mon Sep 17 00:00:00 2001 From: Mike Turquette Date: Mon, 24 Feb 2014 16:08:41 -0800 Subject: [PATCH] clk: add pr_err and kerneldoc around clk notifiers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both the pr_err and the additional kerneldoc aim to help when debugging errors thrown from within a clock rate-change notifier callback. Reported-by: Sören Brinkmann Signed-off-by: Mike Turquette --- drivers/clk/clk.c | 5 ++++- include/linux/clk.h | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index a6f079d..1a95b92 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -1339,8 +1339,11 @@ static int __clk_speculate_rates(struct clk *clk, unsigned long parent_rate) if (clk->notifier_count) ret = __clk_notify(clk, PRE_RATE_CHANGE, clk->rate, new_rate); - if (ret & NOTIFY_STOP_MASK) + if (ret & NOTIFY_STOP_MASK) { + pr_err("%s: clk notifier callback for clock %s aborted with error %d\n", + __func__, clk->name, ret); goto out; + } hlist_for_each_entry(child, &clk->children, child_node) { ret = __clk_speculate_rates(child, new_rate); diff --git a/include/linux/clk.h b/include/linux/clk.h index 0dd9114..35a7e00 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -78,8 +78,22 @@ struct clk_notifier_data { unsigned long new_rate; }; +/** + * clk_notifier_register: register a clock rate-change notifier callback + * @clk: clock whose rate we are interested in + * @nb: notifier block with callback function pointer + * + * ProTip: debugging across notifier chains can be frustrating. Make sure that + * your notifier callback function prints a nice big warning in case of + * failure. + */ int clk_notifier_register(struct clk *clk, struct notifier_block *nb); +/** + * clk_notifier_unregister: unregister a clock rate-change notifier callback + * @clk: clock whose rate we are no longer interested in + * @nb: notifier block which will be unregistered + */ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb); /**