ccf: where to put constraints?

[Mike Turquette]

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 <name> 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 <mturquette at linaro.org>
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 <soren.brinkmann at xilinx.com>
Signed-off-by: Mike Turquette <mturquette at linaro.org>
---
 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);
 
 /**
-- 
1.8.3.2