[PATCH RFC 0/3] clocksource/cadence_ttc: Handle frequency changes

[PATCH RFC 0/3] clocksource/cadence_ttc: Handle frequency changes

[Soren Brinkmann]

Hi,

I've asked Thomas and John before, what to do about a clocksource with
variable input frequency. It looks like the easiest and probably also
best solution is to just not use a timer with a variable frequency - as
John suggested.

I thought another option to get around removing this feature might be
to use clock notifiers and updating the timer's prescaler on the fly.
But that seems to become messy really fast and causes more problems in
cases multiple clocksources are available, since the clock notifier
might impose the driver's restrictions on the input clock, although it
is not even the currently used clocksource.

I have sketched out both approaches in two series.

The first one, which should directly follow this email, is to introduce
a new flag in the driver's DT bindings, which marks the input clock as
'unstable'. In case this flag is present, the timer does not register
itself as clocksource (and not as sched_clock either, of course).
The clockevent feature is not affected.

The second approach uses clock and PM notifiers to adjust the timer's
pre-scaler on frequency changes. As mentioned above, I didn't see a way
to find out whether the timer is the current clock source to not enforce
restrictions in case it is not.

I'm actually not fully convinced of either approach. So, please let me
know of better solutions, or how to make either approach acceptable.
If everything works well, I send out the second series as reply to this
cover letter. I have both series listed below. Both are based on
tip/timers/core.

	Thanks,
	Sören


Approach 1, don't register as clocksource:
Soren Brinkmann (3):
  clocksource/cadence_ttc: Remove clocksource clock notifier
  clocksource/cadence_ttc: Make clocksource optional
  arm: dt: zynq: Mark TTC input clock as unstable

 .../bindings/timer/cadence,ttc-timer.txt           |  4 ++
 arch/arm/boot/dts/zynq-7000.dtsi                   |  2 +
 drivers/clocksource/cadence_ttc_timer.c            | 47 +---------------------
 3 files changed, 8 insertions(+), 45 deletions(-)

Approach 2, use notifiers to adjust:
Soren Brinkmann (1):
  clocksource/cadence_ttc: Overhaul clocksource frequency adjustment

 drivers/clocksource/cadence_ttc_timer.c | 141 +++++++++++++++++++++++++++-----
 1 file changed, 121 insertions(+), 20 deletions(-)

-- 
1.8.4

[PATCH RFC 1/3] clocksource/cadence_ttc: Remove clocksource clock notifier

[Soren Brinkmann]

Remove the clocksource related clock notifier which tries to update the
clocksource frequency in case the timer's input frequency changes.
The notifier's approach - unregistering and re-registering the clocksource
with the new frequency - is destined to fail, if the TTC is the only
available HR clocksource.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/clocksource/cadence_ttc_timer.c | 44 ---------------------------------
 1 file changed, 44 deletions(-)

diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c
index b2bb3a4b..99ec898 100644
--- a/drivers/clocksource/cadence_ttc_timer.c
+++ b/drivers/clocksource/cadence_ttc_timer.c
@@ -219,43 +219,6 @@ static void ttc_set_mode(enum clock_event_mode mode,
 	}
 }
 
-static int ttc_rate_change_clocksource_cb(struct notifier_block *nb,
-		unsigned long event, void *data)
-{
-	struct clk_notifier_data *ndata = data;
-	struct ttc_timer *ttc = to_ttc_timer(nb);
-	struct ttc_timer_clocksource *ttccs = container_of(ttc,
-			struct ttc_timer_clocksource, ttc);
-
-	switch (event) {
-	case POST_RATE_CHANGE:
-		/*
-		 * Do whatever is necessary to maintain a proper time base
-		 *
-		 * I cannot find a way to adjust the currently used clocksource
-		 * to the new frequency. __clocksource_updatefreq_hz() sounds
-		 * good, but does not work. Not sure what's that missing.
-		 *
-		 * This approach works, but triggers two clocksource switches.
-		 * The first after unregister to clocksource jiffies. And
-		 * another one after the register to the newly registered timer.
-		 *
-		 * Alternatively we could 'waste' another HW timer to ping pong
-		 * between clock sources. That would also use one register and
-		 * one unregister call, but only trigger one clocksource switch
-		 * for the cost of another HW timer used by the OS.
-		 */
-		clocksource_unregister(&ttccs->cs);
-		clocksource_register_hz(&ttccs->cs,
-				ndata->new_rate / PRESCALE);
-		/* fall through */
-	case PRE_RATE_CHANGE:
-	case ABORT_RATE_CHANGE:
-	default:
-		return NOTIFY_DONE;
-	}
-}
-
 static void __init ttc_setup_clocksource(struct clk *clk, void __iomem *base)
 {
 	struct ttc_timer_clocksource *ttccs;
@@ -273,13 +236,6 @@ static void __init ttc_setup_clocksource(struct clk *clk, void __iomem *base)
 		return;
 	}
 
-	ttccs->ttc.clk_rate_change_nb.notifier_call =
-		ttc_rate_change_clocksource_cb;
-	ttccs->ttc.clk_rate_change_nb.next = NULL;
-	if (clk_notifier_register(ttccs->ttc.clk,
-				&ttccs->ttc.clk_rate_change_nb))
-		pr_warn("Unable to register clock notifier.\n");
-
 	ttccs->ttc.base_addr = base;
 	ttccs->cs.name = "ttc_clocksource";
 	ttccs->cs.rating = 200;
-- 
1.8.4

[PATCH RFC 2/3] clocksource/cadence_ttc: Make clocksource optional

[Soren Brinkmann]

On some platforms, like Zynq, the input clock for the TTC is directly
derived from the CPU clock, which means it scales when cpufreq adjusts
the CPU frequency.
At the same time the clocksource core expects a clocksource to be
stable and doesn't allow frequency adjustments.

Therefore a new flag - 'input-clock-unstable' - is added to the DT
bindings for this driver. In case the flag is present the driver does
not register the timer as clocksource.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 Documentation/devicetree/bindings/timer/cadence,ttc-timer.txt | 4 ++++
 drivers/clocksource/cadence_ttc_timer.c                       | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/timer/cadence,ttc-timer.txt b/Documentation/devicetree/bindings/timer/cadence,ttc-timer.txt
index 993695c..a936c0a 100644
--- a/Documentation/devicetree/bindings/timer/cadence,ttc-timer.txt
+++ b/Documentation/devicetree/bindings/timer/cadence,ttc-timer.txt
@@ -6,6 +6,10 @@ Required properties:
 - interrupts : A list of 3 interrupts; one per timer channel.
 - clocks: phandle to the source clock
 
+Optional properties:
+- input-clock-unstable : Mark the timer's input clock as unstable. E.g. it
+			 scales with cpufreq.
+
 Example:
 
 ttc0: ttc0@f8001000 {
diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c
index 99ec898..66aaf6a 100644
--- a/drivers/clocksource/cadence_ttc_timer.c
+++ b/drivers/clocksource/cadence_ttc_timer.c
@@ -407,7 +407,8 @@ static void __init ttc_timer_init(struct device_node *timer)
 		BUG();
 	}
 
-	ttc_setup_clocksource(clk_cs, timer_baseaddr);
+	if (!of_property_read_bool(timer, "input-clock-unstable"))
+		ttc_setup_clocksource(clk_cs, timer_baseaddr);
 	ttc_setup_clockevent(clk_ce, timer_baseaddr + 4, irq);
 
 	pr_info("%s #0 at %p, irq=%d\n", timer->name, timer_baseaddr, irq);
-- 
1.8.4

[PATCH RFC 3/3] arm: dt: zynq: Mark TTC input clock as unstable

[Soren Brinkmann]

On Zynq the TTC's input clock is directly derived from the CPU clock.
I.e. the input clock is not constant but scales with the CPU frequency.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 arch/arm/boot/dts/zynq-7000.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
index 6f54a64..c4d4e59 100644
--- a/arch/arm/boot/dts/zynq-7000.dtsi
+++ b/arch/arm/boot/dts/zynq-7000.dtsi
@@ -97,6 +97,7 @@
 			interrupts = < 0 10 4 0 11 4 0 12 4 >;
 			compatible = "cdns,ttc";
 			clocks = <&clkc 6>;
+			input-clock-unstable;
 			reg = <0xF8001000 0x1000>;
 			clock-ranges;
 		};
@@ -107,6 +108,7 @@
 			compatible = "cdns,ttc";
 			clocks = <&clkc 6>;
 			reg = <0xF8002000 0x1000>;
+			input-clock-unstable;
 			clock-ranges;
 		};
 		scutimer: scutimer@f8f00600 {
-- 
1.8.4

[PATCH RFC] clocksource/cadence_ttc: Overhaul clocksource frequency adjustment

[Soren Brinkmann]

The currently used method adjusting the clocksource to a changing input
frequency will not work on kernels from 3.11 on.
The new approach is to keep the timer frequency as constant as possible.
I.e.
 - due to the TTC's prescaler limitations, allow frequency changes
   only if the frequency scales by a power of 2
 - adjust the counter's divider on the fly when a frequency change
   occurs

When suspending though, the driver should not prevent rate changes in
order to allow the system to enter its low power state. For that
reason a PM notifier is added so rate changes can be ignored during
suspend/resume.

Since time will pass between the actual change in HW of the input
clock and until SW and the TTC catch up, we sacrifice accuracy -
compared to an actually constant clock source.
Also we limit cpufreq to scale by certain factors only.
But we may keep the time base somewhat constant, so that sleep() & co
keep working as expected, while supporting cpufreq and suspend.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
The notifiers do always trigger, even when the TTC is not the current
clocksource. Is there a good way to prevent this?

 drivers/clocksource/cadence_ttc_timer.c | 141 +++++++++++++++++++++++++++-----
 1 file changed, 121 insertions(+), 20 deletions(-)

diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c
index b2bb3a4b..1719b6c 100644
--- a/drivers/clocksource/cadence_ttc_timer.c
+++ b/drivers/clocksource/cadence_ttc_timer.c
@@ -22,6 +22,7 @@
 #include <linux/of_irq.h>
 #include <linux/slab.h>
 #include <linux/sched_clock.h>
+#include <linux/suspend.h>
 
 /*
  * This driver configures the 2 16-bit count-up timers as follows:
@@ -52,6 +53,8 @@
 #define TTC_CNT_CNTRL_DISABLE_MASK	0x1
 
 #define TTC_CLK_CNTRL_CSRC_MASK		(1 << 5)	/* clock source */
+#define TTC_CLK_CNTRL_PSV_MASK		0x1e
+#define TTC_CLK_CNTRL_PSV_SHIFT		1
 
 /*
  * Setup the timers to use pre-scaling, using a fixed value for now that will
@@ -63,6 +66,8 @@
 #define CLK_CNTRL_PRESCALE_EN	1
 #define CNT_CNTRL_RESET		(1 << 4)
 
+#define MAX_F_ERR 50
+
 /**
  * struct ttc_timer - This definition defines local timer structure
  *
@@ -80,8 +85,13 @@ struct ttc_timer {
 		container_of(x, struct ttc_timer, clk_rate_change_nb)
 
 struct ttc_timer_clocksource {
+	int			scale_dir;
+	u32			scale_clk_ctrl_reg_old;
+	u32			scale_clk_ctrl_reg_new;
+	int			suspending;
 	struct ttc_timer	ttc;
 	struct clocksource	cs;
+	struct notifier_block	suspend_nb;
 };
 
 #define to_ttc_timer_clksrc(x) \
@@ -227,33 +237,120 @@ static int ttc_rate_change_clocksource_cb(struct notifier_block *nb,
 	struct ttc_timer_clocksource *ttccs = container_of(ttc,
 			struct ttc_timer_clocksource, ttc);
 
+	if (ttccs->suspending)
+		return NOTIFY_OK;
+
 	switch (event) {
-	case POST_RATE_CHANGE:
+	case PRE_RATE_CHANGE:
+	{
+		u32 psv;
+		unsigned long factor, rate_low, rate_high;
+
+		if (ndata->new_rate == ndata->old_rate) {
+			ttccs->scale_dir = 0;
+			return NOTIFY_OK;
+		}
+
+		if (ndata->new_rate > ndata->old_rate) {
+			factor = DIV_ROUND_CLOSEST(ndata->new_rate,
+					ndata->old_rate);
+			ttccs->scale_dir = 1;
+			rate_low = ndata->old_rate;
+			rate_high = ndata->new_rate;
+		} else {
+			factor = DIV_ROUND_CLOSEST(ndata->old_rate,
+					ndata->new_rate);
+			ttccs->scale_dir = -1;
+			rate_low = ndata->new_rate;
+			rate_high = ndata->old_rate;
+		}
+
+		if (!is_power_of_2(factor))
+				return NOTIFY_BAD;
+
+		if (abs(rate_high - (factor * rate_low)) > MAX_F_ERR)
+			return NOTIFY_BAD;
+
+		factor = __ilog2_u32(factor);
+
 		/*
-		 * Do whatever is necessary to maintain a proper time base
-		 *
-		 * I cannot find a way to adjust the currently used clocksource
-		 * to the new frequency. __clocksource_updatefreq_hz() sounds
-		 * good, but does not work. Not sure what's that missing.
-		 *
-		 * This approach works, but triggers two clocksource switches.
-		 * The first after unregister to clocksource jiffies. And
-		 * another one after the register to the newly registered timer.
-		 *
-		 * Alternatively we could 'waste' another HW timer to ping pong
-		 * between clock sources. That would also use one register and
-		 * one unregister call, but only trigger one clocksource switch
-		 * for the cost of another HW timer used by the OS.
+		 * store timer clock ctrl register so we can restore it in case
+		 * of an abort.
 		 */
-		clocksource_unregister(&ttccs->cs);
-		clocksource_register_hz(&ttccs->cs,
-				ndata->new_rate / PRESCALE);
-		/* fall through */
-	case PRE_RATE_CHANGE:
+		ttccs->scale_clk_ctrl_reg_old =
+			__raw_readl(ttccs->ttc.base_addr +
+					TTC_CLK_CNTRL_OFFSET);
+
+		psv = (ttccs->scale_clk_ctrl_reg_old &
+				TTC_CLK_CNTRL_PSV_MASK) >>
+				TTC_CLK_CNTRL_PSV_SHIFT;
+		if (ttccs->scale_dir < 0)
+			psv -= factor;
+		else
+			psv += factor;
+
+		/* prescaler within legal range? */
+		if (psv & ~(TTC_CLK_CNTRL_PSV_MASK >> TTC_CLK_CNTRL_PSV_SHIFT))
+			return NOTIFY_BAD;
+
+		ttccs->scale_clk_ctrl_reg_new = ttccs->scale_clk_ctrl_reg_old &
+			~TTC_CLK_CNTRL_PSV_MASK;
+		ttccs->scale_clk_ctrl_reg_new |= psv << TTC_CLK_CNTRL_PSV_SHIFT;
+
+
+		/* scale down: adjust divider in post-change notification */
+		if (ttccs->scale_dir < 0)
+			return NOTIFY_DONE;
+
+		/* scale up: adjust divider now - before frequency change */
+		__raw_writel(ttccs->scale_clk_ctrl_reg_new,
+				ttccs->ttc.base_addr + TTC_CLK_CNTRL_OFFSET);
+		break;
+	}
+	case POST_RATE_CHANGE:
+		/* scale up: pre-change notification did the adjustment */
+		if (ttccs->scale_dir >= 0)
+			return NOTIFY_OK;
+
+		/* scale down: adjust divider now - after frequency change */
+		__raw_writel(ttccs->scale_clk_ctrl_reg_new,
+				ttccs->ttc.base_addr + TTC_CLK_CNTRL_OFFSET);
+		break;
+
 	case ABORT_RATE_CHANGE:
+		/* we have to undo the adjustment in case we scale up */
+		if (ttccs->scale_dir <= 0)
+			return NOTIFY_OK;
+
+		/* restore original register value */
+		__raw_writel(ttccs->scale_clk_ctrl_reg_old,
+				ttccs->ttc.base_addr + TTC_CLK_CNTRL_OFFSET);
+		/* fall through */
 	default:
 		return NOTIFY_DONE;
 	}
+
+	return NOTIFY_DONE;
+}
+
+static int ttc_suspend_notifier_cb(struct notifier_block *nb,
+		unsigned long event, void *data)
+{
+	struct ttc_timer_clocksource *ttccs = container_of(nb,
+			struct ttc_timer_clocksource, suspend_nb);
+
+	switch (event) {
+	case PM_SUSPEND_PREPARE:
+		ttccs->suspending = 1;
+		break;
+	case PM_POST_SUSPEND:
+		ttccs->suspending = 0;
+		break;
+	default:
+		break;
+	}
+
+	return NOTIFY_DONE;
 }
 
 static void __init ttc_setup_clocksource(struct clk *clk, void __iomem *base)
@@ -280,6 +377,10 @@ static void __init ttc_setup_clocksource(struct clk *clk, void __iomem *base)
 				&ttccs->ttc.clk_rate_change_nb))
 		pr_warn("Unable to register clock notifier.\n");
 
+	ttccs->suspend_nb.notifier_call = ttc_suspend_notifier_cb;
+	if (register_pm_notifier(&ttccs->suspend_nb))
+		pr_warn("Unable to register PM notifier.\n");
+
 	ttccs->ttc.base_addr = base;
 	ttccs->cs.name = "ttc_clocksource";
 	ttccs->cs.rating = 200;
-- 
1.8.4