[PATCH 0/7] arm: zynq: cpufreq support

[PATCH 0/7] arm: zynq: cpufreq support

[Soren Brinkmann]

Hi all,

this series targets to enable cpufreq for Zynq which needs some
preparation in the DT and clocksource driver to work.

The first two patches are cleaning up the DT. The first just removes an
invalid property and the second adds a 'cpus' node to the DT. Those
changes should be okay independently of the final cpufreq patch.

Then the cadence_ttc patches: The first one fixes an issue which - I
think - shows in combination with cpuidle. I saw a kernel WARN triggered
because the driver calls clk_get_rate() from interrupt context. 3/7
should fix that. Patches 4 and 5 then are the actual preparation for
cpufreq. And 6 is an optimization to use the timer HW a little bit more
efficient.

And the final patch adds the required DT properties, platform devices
etc. to use cpufreq-cpu0 to scale the CPU frequency.

	Sören


Soren Brinkmann (7):
  arm: dt: zynq: Remove 'clock-ranges' from TTC nodes
  arm: dt: zynq: Add 'cpus' node
  clocksource/cadence_ttc: Store timer frequency in driver data
  clocksource/cadence_ttc: Adjust interval in clock notifier
  clocksource/cadence_ttc: Overhaul clocksource frequency adjustment
  clocksource/cadence_ttc: Use only one counter
  arm: zynq: Add support for cpufreq

 arch/arm/boot/dts/zynq-7000.dtsi        |  35 ++++-
 arch/arm/mach-zynq/Kconfig              |   2 +
 arch/arm/mach-zynq/common.c             |   3 +
 drivers/clocksource/cadence_ttc_timer.c | 259 +++++++++++++++++++++-----------
 4 files changed, 208 insertions(+), 91 deletions(-)

-- 
1.8.4.2

[PATCH 1/7] arm: dt: zynq: Remove 'clock-ranges' from TTC nodes

[Soren Brinkmann]

The bindings for the TTC changed in commit 'arm: zynq: Use standard
timer binding' (e932900a3279b5dbb6d8f43c7b369003620e137c). That change
removed possible subnodes from this driver rendering the 'clock-ranges'
property invalid for this node.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
Acked-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 arch/arm/boot/dts/zynq-7000.dtsi | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
index e32b92b949d2..27ebc1ba9671 100644
--- a/arch/arm/boot/dts/zynq-7000.dtsi
+++ b/arch/arm/boot/dts/zynq-7000.dtsi
@@ -98,7 +98,6 @@
 			compatible = "cdns,ttc";
 			clocks = <&clkc 6>;
 			reg = <0xF8001000 0x1000>;
-			clock-ranges;
 		};
 
 		ttc1: ttc1@f8002000 {
@@ -107,7 +106,6 @@
 			compatible = "cdns,ttc";
 			clocks = <&clkc 6>;
 			reg = <0xF8002000 0x1000>;
-			clock-ranges;
 		};
 		scutimer: scutimer@f8f00600 {
 			interrupt-parent = <&intc>;
-- 
1.8.4.2

[PATCH 2/7] arm: dt: zynq: Add 'cpus' node

[Soren Brinkmann]

Add a 'cpus' node to describe the CPU cores of Zynq.

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

diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
index 27ebc1ba9671..37fc04525142 100644
--- a/arch/arm/boot/dts/zynq-7000.dtsi
+++ b/arch/arm/boot/dts/zynq-7000.dtsi
@@ -15,6 +15,33 @@
 / {
 	compatible = "xlnx,zynq-7000";
 
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu@0 {
+			compatible = "arm,cortex-a9";
+			device_type = "cpu";
+			reg = <0>;
+			clocks = <&clkc 3>;
+			i-cache-size = <0x8000>;
+			i-cache-line-size = <0x20>;
+			d-cache-size = <0x8000>;
+			d-cache-line-size = <0x20>;
+		};
+
+		cpu@1 {
+			compatible = "arm,cortex-a9";
+			device_type = "cpu";
+			reg = <1>;
+			clocks = <&clkc 3>;
+			i-cache-size = <0x8000>;
+			i-cache-line-size = <0x20>;
+			d-cache-size = <0x8000>;
+			d-cache-line-size = <0x20>;
+		};
+	};
+
 	pmu {
 		compatible = "arm,cortex-a9-pmu";
 		interrupts = <0 5 4>, <0 6 4>;
-- 
1.8.4.2

[PATCH 3/7] clocksource/cadence_ttc: Store timer frequency in driver data

[Soren Brinkmann]

It is not allowed to call clk_get_rate() from interrupt context. To
avoid such calls the timer input frequency is stored in the driver's
data struct which makes it accessible to the driver in any context.

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

diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c
index b2bb3a4bc205..a92350b55d32 100644
--- a/drivers/clocksource/cadence_ttc_timer.c
+++ b/drivers/clocksource/cadence_ttc_timer.c
@@ -67,11 +67,13 @@
  * struct ttc_timer - This definition defines local timer structure
  *
  * @base_addr:	Base address of timer
+ * @freq:	Timer input clock frequency
  * @clk:	Associated clock source
  * @clk_rate_change_nb	Notifier block for clock rate changes
  */
 struct ttc_timer {
 	void __iomem *base_addr;
+	unsigned long freq;
 	struct clk *clk;
 	struct notifier_block clk_rate_change_nb;
 };
@@ -196,9 +198,8 @@ static void ttc_set_mode(enum clock_event_mode mode,
 
 	switch (mode) {
 	case CLOCK_EVT_MODE_PERIODIC:
-		ttc_set_interval(timer,
-				DIV_ROUND_CLOSEST(clk_get_rate(ttce->ttc.clk),
-					PRESCALE * HZ));
+		ttc_set_interval(timer, DIV_ROUND_CLOSEST(ttce->ttc.freq,
+						PRESCALE * HZ));
 		break;
 	case CLOCK_EVT_MODE_ONESHOT:
 	case CLOCK_EVT_MODE_UNUSED:
@@ -273,6 +274,8 @@ static void __init ttc_setup_clocksource(struct clk *clk, void __iomem *base)
 		return;
 	}
 
+	ttccs->ttc.freq = clk_get_rate(ttccs->ttc.clk);
+
 	ttccs->ttc.clk_rate_change_nb.notifier_call =
 		ttc_rate_change_clocksource_cb;
 	ttccs->ttc.clk_rate_change_nb.next = NULL;
@@ -298,16 +301,14 @@ static void __init ttc_setup_clocksource(struct clk *clk, void __iomem *base)
 	__raw_writel(CNT_CNTRL_RESET,
 		     ttccs->ttc.base_addr + TTC_CNT_CNTRL_OFFSET);
 
-	err = clocksource_register_hz(&ttccs->cs,
-			clk_get_rate(ttccs->ttc.clk) / PRESCALE);
+	err = clocksource_register_hz(&ttccs->cs, ttccs->ttc.freq / PRESCALE);
 	if (WARN_ON(err)) {
 		kfree(ttccs);
 		return;
 	}
 
 	ttc_sched_clock_val_reg = base + TTC_COUNT_VAL_OFFSET;
-	setup_sched_clock(ttc_sched_clock_read, 16,
-			clk_get_rate(ttccs->ttc.clk) / PRESCALE);
+	setup_sched_clock(ttc_sched_clock_read, 16, ttccs->ttc.freq / PRESCALE);
 }
 
 static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
@@ -334,6 +335,9 @@ static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
 				ndata->new_rate / PRESCALE);
 		local_irq_restore(flags);
 
+		/* update cached frequency */
+		ttc->freq = ndata->new_rate;
+
 		/* fall through */
 	}
 	case PRE_RATE_CHANGE:
@@ -367,6 +371,7 @@ static void __init ttc_setup_clockevent(struct clk *clk,
 	if (clk_notifier_register(ttcce->ttc.clk,
 				&ttcce->ttc.clk_rate_change_nb))
 		pr_warn("Unable to register clock notifier.\n");
+	ttcce->ttc.freq = clk_get_rate(ttcce->ttc.clk);
 
 	ttcce->ttc.base_addr = base;
 	ttcce->ce.name = "ttc_clockevent";
@@ -396,7 +401,7 @@ static void __init ttc_setup_clockevent(struct clk *clk,
 	}
 
 	clockevents_config_and_register(&ttcce->ce,
-			clk_get_rate(ttcce->ttc.clk) / PRESCALE, 1, 0xfffe);
+			ttcce->ttc.freq / PRESCALE, 1, 0xfffe);
 }
 
 /**
-- 
1.8.4.2

[PATCH 4/7] clocksource/cadence_ttc: Adjust interval in clock notifier

[Soren Brinkmann]

The clockevent has to be reprogrammed if the timer's input
clock frequency changes and the timer is in periodic mode, in order to
maintain the correct timer interval.

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

diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c
index a92350b55d32..68a336038d8f 100644
--- a/drivers/clocksource/cadence_ttc_timer.c
+++ b/drivers/clocksource/cadence_ttc_timer.c
@@ -338,6 +338,10 @@ static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
 		/* update cached frequency */
 		ttc->freq = ndata->new_rate;
 
+		if (ttcce->ce.mode == CLOCK_EVT_MODE_PERIODIC)
+			ttc_set_interval(ttc, DIV_ROUND_CLOSEST(ttc->freq,
+						PRESCALE * HZ));
+
 		/* fall through */
 	}
 	case PRE_RATE_CHANGE:
-- 
1.8.4.2

[PATCH 5/7] clocksource/cadence_ttc: Overhaul clocksource frequency adjustment

[Soren Brinkmann]

The currently used method adjusting the clocksource to a changing input
frequency does 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.

This limits 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>
---
 drivers/clocksource/cadence_ttc_timer.c | 142 +++++++++++++++++++++++++++-----
 1 file changed, 122 insertions(+), 20 deletions(-)

diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c
index 68a336038d8f..421b942dd707 100644
--- a/drivers/clocksource/cadence_ttc_timer.c
+++ b/drivers/clocksource/cadence_ttc_timer.c
@@ -16,12 +16,14 @@
  */
 
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/interrupt.h>
 #include <linux/clockchips.h>
 #include <linux/of_address.h>
 #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 +54,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 +67,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
  *
@@ -82,8 +88,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) \
@@ -228,33 +239,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)
@@ -283,6 +381,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.2

[PATCH 6/7] clocksource/cadence_ttc: Use only one counter

[Soren Brinkmann]

Currently the driver uses two of the three counters the TTC provides to
implement a clocksource and a clockevent device. By using the TTC's
match feature we can implement both use cases using a single counter
only.
The old approach is to use timer over-/underflow to generate an
interrupt. Using the match register allows to generate an interrupt on
arbitrary counter values. This way a dedicated clockevent counter can be
avoided.

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

diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c
index 421b942dd707..01dba97cee84 100644
--- a/drivers/clocksource/cadence_ttc_timer.c
+++ b/drivers/clocksource/cadence_ttc_timer.c
@@ -48,6 +48,7 @@
 #define TTC_CNT_CNTRL_OFFSET		0x0C /* Counter Control Reg, RW */
 #define TTC_COUNT_VAL_OFFSET		0x18 /* Counter Value Reg, RO */
 #define TTC_INTR_VAL_OFFSET		0x24 /* Interval Count Reg, RW */
+#define TTC_MATCH1_OFFSET	0x30 /* Match reg, RW */
 #define TTC_ISR_OFFSET		0x54 /* Interrupt Status Reg, RO */
 #define TTC_IER_OFFSET		0x60 /* Interrupt Enable Reg, RW */
 
@@ -65,7 +66,10 @@
 #define PRESCALE		2048	/* The exponent must match this */
 #define CLK_CNTRL_PRESCALE	((PRESCALE_EXPONENT - 1) << 1)
 #define CLK_CNTRL_PRESCALE_EN	1
-#define CNT_CNTRL_RESET		(1 << 4)
+#define CNT_CNTRL_RESET		BIT(4)
+#define CNT_CNTRL_MATCH		BIT(3)
+
+#define TTC_INTERRUPT_MATCH1	BIT(1)
 
 #define MAX_F_ERR 50
 
@@ -103,6 +107,7 @@ struct ttc_timer_clocksource {
 struct ttc_timer_clockevent {
 	struct ttc_timer		ttc;
 	struct clock_event_device	ce;
+	unsigned long			interval;
 };
 
 #define to_ttc_timer_clkevent(x) \
@@ -116,25 +121,20 @@ static void __iomem *ttc_sched_clock_val_reg;
  * @timer:	Pointer to the timer instance
  * @cycles:	Timer interval ticks
  **/
-static void ttc_set_interval(struct ttc_timer *timer,
-					unsigned long cycles)
+static void ttc_set_interval(struct ttc_timer *timer, unsigned long cycles)
 {
-	u32 ctrl_reg;
+	struct ttc_timer_clockevent *ttcce = container_of(timer,
+			struct ttc_timer_clockevent, ttc);
 
-	/* Disable the counter, set the counter value  and re-enable counter */
-	ctrl_reg = __raw_readl(timer->base_addr + TTC_CNT_CNTRL_OFFSET);
-	ctrl_reg |= TTC_CNT_CNTRL_DISABLE_MASK;
-	__raw_writel(ctrl_reg, timer->base_addr + TTC_CNT_CNTRL_OFFSET);
+	/* set interval */
+	u32 reg = __raw_readl(timer->base_addr + TTC_COUNT_VAL_OFFSET);
+	reg += cycles;
+	__raw_writel(reg, timer->base_addr + TTC_MATCH1_OFFSET);
 
-	__raw_writel(cycles, timer->base_addr + TTC_INTR_VAL_OFFSET);
+	/* enable match interrupt */
+	__raw_writel(TTC_INTERRUPT_MATCH1, timer->base_addr + TTC_IER_OFFSET);
 
-	/*
-	 * Reset the counter (0x10) so that it starts from 0, one-shot
-	 * mode makes this needed for timing to be right.
-	 */
-	ctrl_reg |= CNT_CNTRL_RESET;
-	ctrl_reg &= ~TTC_CNT_CNTRL_DISABLE_MASK;
-	__raw_writel(ctrl_reg, timer->base_addr + TTC_CNT_CNTRL_OFFSET);
+	ttcce->interval = cycles;
 }
 
 /**
@@ -152,6 +152,8 @@ static irqreturn_t ttc_clock_event_interrupt(int irq, void *dev_id)
 
 	/* Acknowledge the interrupt and call event handler */
 	__raw_readl(timer->base_addr + TTC_ISR_OFFSET);
+	if (ttce->ce.mode == CLOCK_EVT_MODE_PERIODIC)
+		ttc_set_interval(timer, ttce->interval);
 
 	ttce->ce.event_handler(&ttce->ce);
 
@@ -205,7 +207,6 @@ static void ttc_set_mode(enum clock_event_mode mode,
 {
 	struct ttc_timer_clockevent *ttce = to_ttc_timer_clkevent(evt);
 	struct ttc_timer *timer = &ttce->ttc;
-	u32 ctrl_reg;
 
 	switch (mode) {
 	case CLOCK_EVT_MODE_PERIODIC:
@@ -215,18 +216,9 @@ static void ttc_set_mode(enum clock_event_mode mode,
 	case CLOCK_EVT_MODE_ONESHOT:
 	case CLOCK_EVT_MODE_UNUSED:
 	case CLOCK_EVT_MODE_SHUTDOWN:
-		ctrl_reg = __raw_readl(timer->base_addr +
-					TTC_CNT_CNTRL_OFFSET);
-		ctrl_reg |= TTC_CNT_CNTRL_DISABLE_MASK;
-		__raw_writel(ctrl_reg,
-				timer->base_addr + TTC_CNT_CNTRL_OFFSET);
+		__raw_writel(0, timer->base_addr + TTC_IER_OFFSET);
 		break;
 	case CLOCK_EVT_MODE_RESUME:
-		ctrl_reg = __raw_readl(timer->base_addr +
-					TTC_CNT_CNTRL_OFFSET);
-		ctrl_reg &= ~TTC_CNT_CNTRL_DISABLE_MASK;
-		__raw_writel(ctrl_reg,
-				timer->base_addr + TTC_CNT_CNTRL_OFFSET);
 		break;
 	}
 }
@@ -392,17 +384,6 @@ static void __init ttc_setup_clocksource(struct clk *clk, void __iomem *base)
 	ttccs->cs.mask = CLOCKSOURCE_MASK(16);
 	ttccs->cs.flags = CLOCK_SOURCE_IS_CONTINUOUS;
 
-	/*
-	 * Setup the clock source counter to be an incrementing counter
-	 * with no interrupt and it rolls over at 0xFFFF. Pre-scale
-	 * it by 32 also. Let it start running now.
-	 */
-	__raw_writel(0x0,  ttccs->ttc.base_addr + TTC_IER_OFFSET);
-	__raw_writel(CLK_CNTRL_PRESCALE | CLK_CNTRL_PRESCALE_EN,
-		     ttccs->ttc.base_addr + TTC_CLK_CNTRL_OFFSET);
-	__raw_writel(CNT_CNTRL_RESET,
-		     ttccs->ttc.base_addr + TTC_CNT_CNTRL_OFFSET);
-
 	err = clocksource_register_hz(&ttccs->cs, ttccs->ttc.freq / PRESCALE);
 	if (WARN_ON(err)) {
 		kfree(ttccs);
@@ -488,16 +469,6 @@ static void __init ttc_setup_clockevent(struct clk *clk,
 	ttcce->ce.irq = irq;
 	ttcce->ce.cpumask = cpu_possible_mask;
 
-	/*
-	 * Setup the clock event timer to be an interval timer which
-	 * is prescaled by 32 using the interval interrupt. Leave it
-	 * disabled for now.
-	 */
-	__raw_writel(0x23, ttcce->ttc.base_addr + TTC_CNT_CNTRL_OFFSET);
-	__raw_writel(CLK_CNTRL_PRESCALE | CLK_CNTRL_PRESCALE_EN,
-		     ttcce->ttc.base_addr + TTC_CLK_CNTRL_OFFSET);
-	__raw_writel(0x1,  ttcce->ttc.base_addr + TTC_IER_OFFSET);
-
 	err = request_irq(irq, ttc_clock_event_interrupt,
 			  IRQF_DISABLED | IRQF_TIMER,
 			  ttcce->ce.name, ttcce);
@@ -520,7 +491,7 @@ static void __init ttc_timer_init(struct device_node *timer)
 {
 	unsigned int irq;
 	void __iomem *timer_baseaddr;
-	struct clk *clk_cs, *clk_ce;
+	struct clk *clk;
 	static int initialized;
 	int clksel;
 
@@ -540,7 +511,7 @@ static void __init ttc_timer_init(struct device_node *timer)
 		BUG();
 	}
 
-	irq = irq_of_parse_and_map(timer, 1);
+	irq = irq_of_parse_and_map(timer, 0);
 	if (irq <= 0) {
 		pr_err("ERROR: invalid interrupt number\n");
 		BUG();
@@ -548,22 +519,21 @@ static void __init ttc_timer_init(struct device_node *timer)
 
 	clksel = __raw_readl(timer_baseaddr + TTC_CLK_CNTRL_OFFSET);
 	clksel = !!(clksel & TTC_CLK_CNTRL_CSRC_MASK);
-	clk_cs = of_clk_get(timer, clksel);
-	if (IS_ERR(clk_cs)) {
+	clk = of_clk_get(timer, clksel);
+	if (IS_ERR(clk)) {
 		pr_err("ERROR: timer input clock not found\n");
 		BUG();
 	}
 
-	clksel = __raw_readl(timer_baseaddr + 4 + TTC_CLK_CNTRL_OFFSET);
-	clksel = !!(clksel & TTC_CLK_CNTRL_CSRC_MASK);
-	clk_ce = of_clk_get(timer, clksel);
-	if (IS_ERR(clk_ce)) {
-		pr_err("ERROR: timer input clock not found\n");
-		BUG();
-	}
+	__raw_writel(CLK_CNTRL_PRESCALE | CLK_CNTRL_PRESCALE_EN,
+			timer_baseaddr + TTC_CLK_CNTRL_OFFSET);
+
+	/* start timer in overflow and match mode */
+	__raw_writel(CNT_CNTRL_RESET | CNT_CNTRL_MATCH,
+			timer_baseaddr + TTC_CNT_CNTRL_OFFSET);
 
-	ttc_setup_clocksource(clk_cs, timer_baseaddr);
-	ttc_setup_clockevent(clk_ce, timer_baseaddr + 4, irq);
+	ttc_setup_clocksource(clk, timer_baseaddr);
+	ttc_setup_clockevent(clk, timer_baseaddr, irq);
 
 	pr_info("%s #0 at %p, irq=%d\n", timer->name, timer_baseaddr, irq);
 }
-- 
1.8.4.2

[PATCH 7/7] arm: zynq: Add support for cpufreq

[Soren Brinkmann]

The generic cpufreq-cpu0 driver can scale the CPU frequency on Zynq
SOCs. Add the required platform device to the BSP and appropriate
OPPs to the dts.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 arch/arm/boot/dts/zynq-7000.dtsi | 6 ++++++
 arch/arm/mach-zynq/Kconfig       | 2 ++
 arch/arm/mach-zynq/common.c      | 3 +++
 3 files changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
index 37fc04525142..aca41ca6de1d 100644
--- a/arch/arm/boot/dts/zynq-7000.dtsi
+++ b/arch/arm/boot/dts/zynq-7000.dtsi
@@ -28,6 +28,12 @@
 			i-cache-line-size = <0x20>;
 			d-cache-size = <0x8000>;
 			d-cache-line-size = <0x20>;
+			operating-points = <
+				/* kHz    uV */
+				666667  1000000
+				333334  1000000
+				111112  1000000
+			>;
 		};
 
 		cpu@1 {
diff --git a/arch/arm/mach-zynq/Kconfig b/arch/arm/mach-zynq/Kconfig
index 04f8a4a6e755..8b943f19107d 100644
--- a/arch/arm/mach-zynq/Kconfig
+++ b/arch/arm/mach-zynq/Kconfig
@@ -2,6 +2,8 @@ config ARCH_ZYNQ
 	bool "Xilinx Zynq ARM Cortex A9 Platform" if ARCH_MULTI_V7
 	select ARM_AMBA
 	select ARM_GIC
+	select ARCH_HAS_CPUFREQ
+	select ARCH_HAS_OPP
 	select COMMON_CLK
 	select CPU_V7
 	select GENERIC_CLOCKEVENTS
diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c
index 5f252569c689..20b373fe855a 100644
--- a/arch/arm/mach-zynq/common.c
+++ b/arch/arm/mach-zynq/common.c
@@ -50,12 +50,15 @@ static struct of_device_id zynq_of_bus_ids[] __initdata = {
  */
 static void __init zynq_init_machine(void)
 {
+	struct platform_device_info devinfo = { .name = "cpufreq-cpu0", };
+
 	/*
 	 * 64KB way size, 8-way associativity, parity disabled
 	 */
 	l2x0_of_init(0x02060000, 0xF0F0FFFF);
 
 	of_platform_bus_probe(NULL, zynq_of_bus_ids, NULL);
+	platform_device_register_full(&devinfo);
 }
 
 static void __init zynq_timer_init(void)
-- 
1.8.4.2

Re: [PATCH 2/7] arm: dt: zynq: Add 'cpus' node

[Sudeep KarkadaNagesha]

On 08/11/13 21:21, Soren Brinkmann wrote:
> Add a 'cpus' node to describe the CPU cores of Zynq.
> 
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> Acked-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>  arch/arm/boot/dts/zynq-7000.dtsi | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
> index 27ebc1ba9671..37fc04525142 100644
> --- a/arch/arm/boot/dts/zynq-7000.dtsi
> +++ b/arch/arm/boot/dts/zynq-7000.dtsi
> @@ -15,6 +15,33 @@
>  / {
>  	compatible = "xlnx,zynq-7000";
>  
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu@0 {
> +			compatible = "arm,cortex-a9";
> +			device_type = "cpu";
> +			reg = <0>;
> +			clocks = <&clkc 3>;
> +			i-cache-size = <0x8000>;
> +			i-cache-line-size = <0x20>;
> +			d-cache-size = <0x8000>;
> +			d-cache-line-size = <0x20>;

These cache properties can be identified through CCSIDR(Cache Size ID Registers)
on ARMv7 Cortex implementations. It's better not to have these in DT if they can
be identified runtime.

Regards,
Sudeep

Re: [PATCH 1/7] arm: dt: zynq: Remove 'clock-ranges' from TTC nodes

[Daniel Lezcano]

On 11/08/2013 10:21 PM, Soren Brinkmann wrote:
> The bindings for the TTC changed in commit 'arm: zynq: Use standard
> timer binding' (e932900a3279b5dbb6d8f43c7b369003620e137c). That change
> removed possible subnodes from this driver rendering the 'clock-ranges'
> property invalid for this node.
>
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> Acked-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> ---
>   arch/arm/boot/dts/zynq-7000.dtsi | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
> index e32b92b949d2..27ebc1ba9671 100644
> --- a/arch/arm/boot/dts/zynq-7000.dtsi
> +++ b/arch/arm/boot/dts/zynq-7000.dtsi
> @@ -98,7 +98,6 @@
>   			compatible = "cdns,ttc";
>   			clocks = <&clkc 6>;
>   			reg = <0xF8001000 0x1000>;
> -			clock-ranges;
>   		};
>
>   		ttc1: ttc1@f8002000 {
> @@ -107,7 +106,6 @@
>   			compatible = "cdns,ttc";
>   			clocks = <&clkc 6>;
>   			reg = <0xF8002000 0x1000>;
> -			clock-ranges;
>   		};
>   		scutimer: scutimer@f8f00600 {
>   			interrupt-parent = <&intc>;
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 3/7] clocksource/cadence_ttc: Store timer frequency in driver data

[Daniel Lezcano]

On 11/08/2013 10:21 PM, Soren Brinkmann wrote:
> It is not allowed to call clk_get_rate() from interrupt context. To
> avoid such calls the timer input frequency is stored in the driver's
> data struct which makes it accessible to the driver in any context.
>
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

>   drivers/clocksource/cadence_ttc_timer.c | 21 +++++++++++++--------
>   1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c
> index b2bb3a4bc205..a92350b55d32 100644
> --- a/drivers/clocksource/cadence_ttc_timer.c
> +++ b/drivers/clocksource/cadence_ttc_timer.c
> @@ -67,11 +67,13 @@
>    * struct ttc_timer - This definition defines local timer structure
>    *
>    * @base_addr:	Base address of timer
> + * @freq:	Timer input clock frequency
>    * @clk:	Associated clock source
>    * @clk_rate_change_nb	Notifier block for clock rate changes
>    */
>   struct ttc_timer {
>   	void __iomem *base_addr;
> +	unsigned long freq;
>   	struct clk *clk;
>   	struct notifier_block clk_rate_change_nb;
>   };
> @@ -196,9 +198,8 @@ static void ttc_set_mode(enum clock_event_mode mode,
>
>   	switch (mode) {
>   	case CLOCK_EVT_MODE_PERIODIC:
> -		ttc_set_interval(timer,
> -				DIV_ROUND_CLOSEST(clk_get_rate(ttce->ttc.clk),
> -					PRESCALE * HZ));
> +		ttc_set_interval(timer, DIV_ROUND_CLOSEST(ttce->ttc.freq,
> +						PRESCALE * HZ));
>   		break;
>   	case CLOCK_EVT_MODE_ONESHOT:
>   	case CLOCK_EVT_MODE_UNUSED:
> @@ -273,6 +274,8 @@ static void __init ttc_setup_clocksource(struct clk *clk, void __iomem *base)
>   		return;
>   	}
>
> +	ttccs->ttc.freq = clk_get_rate(ttccs->ttc.clk);
> +
>   	ttccs->ttc.clk_rate_change_nb.notifier_call =
>   		ttc_rate_change_clocksource_cb;
>   	ttccs->ttc.clk_rate_change_nb.next = NULL;
> @@ -298,16 +301,14 @@ static void __init ttc_setup_clocksource(struct clk *clk, void __iomem *base)
>   	__raw_writel(CNT_CNTRL_RESET,
>   		     ttccs->ttc.base_addr + TTC_CNT_CNTRL_OFFSET);
>
> -	err = clocksource_register_hz(&ttccs->cs,
> -			clk_get_rate(ttccs->ttc.clk) / PRESCALE);
> +	err = clocksource_register_hz(&ttccs->cs, ttccs->ttc.freq / PRESCALE);
>   	if (WARN_ON(err)) {
>   		kfree(ttccs);
>   		return;
>   	}
>
>   	ttc_sched_clock_val_reg = base + TTC_COUNT_VAL_OFFSET;
> -	setup_sched_clock(ttc_sched_clock_read, 16,
> -			clk_get_rate(ttccs->ttc.clk) / PRESCALE);
> +	setup_sched_clock(ttc_sched_clock_read, 16, ttccs->ttc.freq / PRESCALE);
>   }
>
>   static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
> @@ -334,6 +335,9 @@ static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
>   				ndata->new_rate / PRESCALE);
>   		local_irq_restore(flags);
>
> +		/* update cached frequency */
> +		ttc->freq = ndata->new_rate;
> +
>   		/* fall through */
>   	}
>   	case PRE_RATE_CHANGE:
> @@ -367,6 +371,7 @@ static void __init ttc_setup_clockevent(struct clk *clk,
>   	if (clk_notifier_register(ttcce->ttc.clk,
>   				&ttcce->ttc.clk_rate_change_nb))
>   		pr_warn("Unable to register clock notifier.\n");
> +	ttcce->ttc.freq = clk_get_rate(ttcce->ttc.clk);
>
>   	ttcce->ttc.base_addr = base;
>   	ttcce->ce.name = "ttc_clockevent";
> @@ -396,7 +401,7 @@ static void __init ttc_setup_clockevent(struct clk *clk,
>   	}
>
>   	clockevents_config_and_register(&ttcce->ce,
> -			clk_get_rate(ttcce->ttc.clk) / PRESCALE, 1, 0xfffe);
> +			ttcce->ttc.freq / PRESCALE, 1, 0xfffe);
>   }
>
>   /**
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 4/7] clocksource/cadence_ttc: Adjust interval in clock notifier

[Daniel Lezcano]

On 11/08/2013 10:21 PM, Soren Brinkmann wrote:
> The clockevent has to be reprogrammed if the timer's input
> clock frequency changes and the timer is in periodic mode, in order to
> maintain the correct timer interval.
>
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> ---
>   drivers/clocksource/cadence_ttc_timer.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c
> index a92350b55d32..68a336038d8f 100644
> --- a/drivers/clocksource/cadence_ttc_timer.c
> +++ b/drivers/clocksource/cadence_ttc_timer.c
> @@ -338,6 +338,10 @@ static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
>   		/* update cached frequency */
>   		ttc->freq = ndata->new_rate;
>
> +		if (ttcce->ce.mode == CLOCK_EVT_MODE_PERIODIC)
> +			ttc_set_interval(ttc, DIV_ROUND_CLOSEST(ttc->freq,
> +						PRESCALE * HZ));
> +

Couldn't be racy ?

>   		/* fall through */
>   	}
>   	case PRE_RATE_CHANGE:
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 2/7] arm: dt: zynq: Add 'cpus' node

[Sören Brinkmann]

On Mon, Nov 11, 2013 at 06:57:44PM +0000, Sudeep KarkadaNagesha wrote:
> On 08/11/13 21:21, Soren Brinkmann wrote:
> > Add a 'cpus' node to describe the CPU cores of Zynq.
> > 
> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> > Acked-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> > ---
> >  arch/arm/boot/dts/zynq-7000.dtsi | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
> > index 27ebc1ba9671..37fc04525142 100644
> > --- a/arch/arm/boot/dts/zynq-7000.dtsi
> > +++ b/arch/arm/boot/dts/zynq-7000.dtsi
> > @@ -15,6 +15,33 @@
> >  / {
> >  	compatible = "xlnx,zynq-7000";
> >  
> > +	cpus {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		cpu@0 {
> > +			compatible = "arm,cortex-a9";
> > +			device_type = "cpu";
> > +			reg = <0>;
> > +			clocks = <&clkc 3>;
> > +			i-cache-size = <0x8000>;
> > +			i-cache-line-size = <0x20>;
> > +			d-cache-size = <0x8000>;
> > +			d-cache-line-size = <0x20>;
> 
> These cache properties can be identified through CCSIDR(Cache Size ID Registers)
> on ARMv7 Cortex implementations. It's better not to have these in DT if they can
> be identified runtime.
Sounds good to me. I'll go ahead an remove them.

	Thanks,
	Sören

Re: [PATCH 5/7] clocksource/cadence_ttc: Overhaul clocksource frequency adjustment

[Daniel Lezcano]

On 11/08/2013 10:21 PM, Soren Brinkmann wrote:
> The currently used method adjusting the clocksource to a changing input
> frequency does 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.

It sounds very weird you have to add a PM notifier in this driver.

Have you been facing an issue or do you assume it could happen ?

> This limits 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>
> ---
>   drivers/clocksource/cadence_ttc_timer.c | 142 +++++++++++++++++++++++++++-----
>   1 file changed, 122 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c
> index 68a336038d8f..421b942dd707 100644
> --- a/drivers/clocksource/cadence_ttc_timer.c
> +++ b/drivers/clocksource/cadence_ttc_timer.c
> @@ -16,12 +16,14 @@
>    */
>
>   #include <linux/clk.h>
> +#include <linux/clk-provider.h>
>   #include <linux/interrupt.h>
>   #include <linux/clockchips.h>
>   #include <linux/of_address.h>
>   #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 +54,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 +67,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
>    *
> @@ -82,8 +88,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) \
> @@ -228,33 +239,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;

I don't see how it couldn't be racy. What prevents suspend to occur 
right after this check ?

> +
>   	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;
> +		}

Hmm, it could be interesting if Viresh can give its opinion on this 
patch [cc'ed].

> +
> +		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)
> @@ -283,6 +381,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;
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 5/7] clocksource/cadence_ttc: Overhaul clocksource frequency adjustment

[Sören Brinkmann]

On Tue, Nov 12, 2013 at 08:01:37PM +0100, Daniel Lezcano wrote:
> On 11/08/2013 10:21 PM, Soren Brinkmann wrote:
> >The currently used method adjusting the clocksource to a changing input
> >frequency does 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.
> 
> It sounds very weird you have to add a PM notifier in this driver.
> 
> Have you been facing an issue or do you assume it could happen ?
Yes, I have problems without it, which are probably Zynq specific
though.
When we suspend on Zynq, we bypass and power down PLLs, which is a
re-parent operation in the clock framework. This happens in early suspend
and triggers the clock notifiers. Due to the restrictions the TTC's
clock notifier applies to frequency changes, it would prevent the
re-parent operation even though we do not have to care about it in
suspend. The timer's own suspend/resume callbacks are called to
late/early and the PM notifier seemed to be the solution. But I'm open
to alternative approaches.

> 
> >This limits 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>
> >---
> >  drivers/clocksource/cadence_ttc_timer.c | 142 +++++++++++++++++++++++++++-----
> >  1 file changed, 122 insertions(+), 20 deletions(-)
> >
> >diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c
> >index 68a336038d8f..421b942dd707 100644
> >--- a/drivers/clocksource/cadence_ttc_timer.c
> >+++ b/drivers/clocksource/cadence_ttc_timer.c
> >@@ -16,12 +16,14 @@
> >   */
> >
> >  #include <linux/clk.h>
> >+#include <linux/clk-provider.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/clockchips.h>
> >  #include <linux/of_address.h>
> >  #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 +54,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 +67,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
> >   *
> >@@ -82,8 +88,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) \
> >@@ -228,33 +239,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;
> 
> I don't see how it couldn't be racy. What prevents suspend to occur
> right after this check ?
Well, in our case there are two situations which could trigger this
notifier.
1. cpufreq
2. suspend
As long as cpufreq is doing its stuff things should be okay and fine.
And suspend triggers the PM and clock notifier in the correct order,
which solves the issue I described above. So, it is not really pretty,
but works for me.

	Sören

Re: [PATCH 2/7] arm: dt: zynq: Add 'cpus' node

[Sören Brinkmann]

On Tue, Nov 12, 2013 at 10:06:05AM -0800, Sören Brinkmann wrote:
> On Mon, Nov 11, 2013 at 06:57:44PM +0000, Sudeep KarkadaNagesha wrote:
> > On 08/11/13 21:21, Soren Brinkmann wrote:
> > > Add a 'cpus' node to describe the CPU cores of Zynq.
> > > 
> > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> > > Acked-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> > > ---
> > >  arch/arm/boot/dts/zynq-7000.dtsi | 27 +++++++++++++++++++++++++++
> > >  1 file changed, 27 insertions(+)
> > > 
> > > diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
> > > index 27ebc1ba9671..37fc04525142 100644
> > > --- a/arch/arm/boot/dts/zynq-7000.dtsi
> > > +++ b/arch/arm/boot/dts/zynq-7000.dtsi
> > > @@ -15,6 +15,33 @@
> > >  / {
> > >  	compatible = "xlnx,zynq-7000";
> > >  
> > > +	cpus {
> > > +		#address-cells = <1>;
> > > +		#size-cells = <0>;
> > > +
> > > +		cpu@0 {
> > > +			compatible = "arm,cortex-a9";
> > > +			device_type = "cpu";
> > > +			reg = <0>;
> > > +			clocks = <&clkc 3>;
> > > +			i-cache-size = <0x8000>;
> > > +			i-cache-line-size = <0x20>;
> > > +			d-cache-size = <0x8000>;
> > > +			d-cache-line-size = <0x20>;
> > 
> > These cache properties can be identified through CCSIDR(Cache Size ID Registers)
> > on ARMv7 Cortex implementations. It's better not to have these in DT if they can
> > be identified runtime.
> Sounds good to me. I'll go ahead an remove them.
BTW: Documentation/devicetree/booting-without-of.txt lists those
properties at least as recommended. That should probably be updated.

	Sören

Re: [PATCH 5/7] clocksource/cadence_ttc: Overhaul clocksource frequency adjustment

[Viresh Kumar]

On 13 November 2013 00:31, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> Hmm, it could be interesting if Viresh can give its opinion on this patch
> [cc'ed].

I am not sure if I understood the problem completely, but if this is about
not allowing frequency transitions from cpufreq when we have started to
suspend our system, then you must have a look at this thread:

https://lkml.org/lkml/2013/10/24/369

Re: [PATCH 2/7] arm: dt: zynq: Add 'cpus' node

[Sudeep KarkadaNagesha]

On 12/11/13 21:58, Sören Brinkmann wrote:
> On Tue, Nov 12, 2013 at 10:06:05AM -0800, Sören Brinkmann wrote:
>> On Mon, Nov 11, 2013 at 06:57:44PM +0000, Sudeep KarkadaNagesha wrote:
>>> On 08/11/13 21:21, Soren Brinkmann wrote:
>>>> Add a 'cpus' node to describe the CPU cores of Zynq.
>>>>
>>>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
>>>> Acked-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>>> ---
>>>>  arch/arm/boot/dts/zynq-7000.dtsi | 27 +++++++++++++++++++++++++++
>>>>  1 file changed, 27 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
>>>> index 27ebc1ba9671..37fc04525142 100644
>>>> --- a/arch/arm/boot/dts/zynq-7000.dtsi
>>>> +++ b/arch/arm/boot/dts/zynq-7000.dtsi
>>>> @@ -15,6 +15,33 @@
>>>>  / {
>>>>  	compatible = "xlnx,zynq-7000";
>>>>  
>>>> +	cpus {
>>>> +		#address-cells = <1>;
>>>> +		#size-cells = <0>;
>>>> +
>>>> +		cpu@0 {
>>>> +			compatible = "arm,cortex-a9";
>>>> +			device_type = "cpu";
>>>> +			reg = <0>;
>>>> +			clocks = <&clkc 3>;
>>>> +			i-cache-size = <0x8000>;
>>>> +			i-cache-line-size = <0x20>;
>>>> +			d-cache-size = <0x8000>;
>>>> +			d-cache-line-size = <0x20>;
>>>
>>> These cache properties can be identified through CCSIDR(Cache Size ID Registers)
>>> on ARMv7 Cortex implementations. It's better not to have these in DT if they can
>>> be identified runtime.
>> Sounds good to me. I'll go ahead an remove them.
> BTW: Documentation/devicetree/booting-without-of.txt lists those
> properties at least as recommended. That should probably be updated.
> 

Correct, thanks for pointing at that. IMO it definitely needs an update with
respect to ARM/ARM64. E.g. the cpu topology bindings for ARM is now @
Documentation/devicetree/bindings/arm/cpus.txt and
Documentation/devicetree/bindings/arm/topology.txt, the cache properties can be
determined runtime so not a required property.

This document looks more general and specifics may not be applicable everywhere.
More over specific bindings are getting defined for many sections in this document.

Regards,
Sudeep

Re: [PATCH 5/7] clocksource/cadence_ttc: Overhaul clocksource frequency adjustment

[Daniel Lezcano]

On 11/13/2013 09:03 AM, Viresh Kumar wrote:
> On 13 November 2013 00:31, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>> Hmm, it could be interesting if Viresh can give its opinion on this patch
>> [cc'ed].
>
> I am not sure if I understood the problem completely, but if this is about
> not allowing frequency transitions from cpufreq when we have started to
> suspend our system, then you must have a look at this thread:
>
> https://lkml.org/lkml/2013/10/24/369

Thanks Viresh for the pointer.


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 5/7] clocksource/cadence_ttc: Overhaul clocksource frequency adjustment

[Sören Brinkmann]

On Wed, Nov 13, 2013 at 01:33:52PM +0530, Viresh Kumar wrote:
> On 13 November 2013 00:31, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> > Hmm, it could be interesting if Viresh can give its opinion on this patch
> > [cc'ed].
> 
> I am not sure if I understood the problem completely, but if this is about
> not allowing frequency transitions from cpufreq when we have started to
> suspend our system, then you must have a look at this thread:
> 
> https://lkml.org/lkml/2013/10/24/369

Thanks for the link. I had problems in that direction at some point too.
But the current issue, is not cpufreq changing the frequency, but our
suspend code in prepare_late() causes a frequency change of this timer
when the PLLs are powered down (it's done through a CCF clk_set_parent()
operation).

	Sören

Re: [PATCH 4/7] clocksource/cadence_ttc: Adjust interval in clock notifier

[Sören Brinkmann]

Hi Daniel,

On Tue, Nov 12, 2013 at 05:29:45PM +0100, Daniel Lezcano wrote:
> On 11/08/2013 10:21 PM, Soren Brinkmann wrote:
> >The clockevent has to be reprogrammed if the timer's input
> >clock frequency changes and the timer is in periodic mode, in order to
> >maintain the correct timer interval.
> >
> >Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> >---
> >  drivers/clocksource/cadence_ttc_timer.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> >diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c
> >index a92350b55d32..68a336038d8f 100644
> >--- a/drivers/clocksource/cadence_ttc_timer.c
> >+++ b/drivers/clocksource/cadence_ttc_timer.c
> >@@ -338,6 +338,10 @@ static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
> >  		/* update cached frequency */
> >  		ttc->freq = ndata->new_rate;
> >
> >+		if (ttcce->ce.mode == CLOCK_EVT_MODE_PERIODIC)
> >+			ttc_set_interval(ttc, DIV_ROUND_CLOSEST(ttc->freq,
> >+						PRESCALE * HZ));
> >+
> 
> Couldn't be racy ?
I guess you're right. What would be the proper way to resolve it? If we
look at a little more code things look like this:

	local_irq_save(flags);                                           
	clockevents_update_freq(&ttcce->ce,                              
	                ndata->new_rate / PRESCALE);                     
	local_irq_restore(flags);                                        
	                                                                 
	/* update cached frequency */                                    
	ttc->freq = ndata->new_rate;                                     
	                                                                 
	if (ttcce->ce.mode == CLOCK_EVT_MODE_PERIODIC)                   
	        ttc_set_interval(ttc, DIV_ROUND_CLOSEST(ttc->freq,       
	                                PRESCALE * HZ));

Would it be enough to move the ttc_set_interval() call within the
section where local IRQs are disabled? I have the feeling for this timer
the local_irq_save() may in general not be the right option since the
timer is not CPU local.
Should we probably change this do irq_disable(ttcce->ce.irq)?

	Thanks,
	Sören

Re: [PATCH 5/7] clocksource/cadence_ttc: Overhaul clocksource frequency adjustment

[Sören Brinkmann]

On Tue, Nov 12, 2013 at 08:01:37PM +0100, Daniel Lezcano wrote:
> On 11/08/2013 10:21 PM, Soren Brinkmann wrote:
> >The currently used method adjusting the clocksource to a changing input
> >frequency does 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.
> 
> It sounds very weird you have to add a PM notifier in this driver.
> 
> Have you been facing an issue or do you assume it could happen ?

As I said in my other email, we do have some issues related with
suspend. The construct with the PM notifier seemed to work but since we
upgraded our vendor tree to 3.12 I see issues with this code.

So, for now, I'll remove the PM notifier and associated code from this
patch.
I have to revisit the suspend case later.

	Thanks,
	Sören