[PATCH v4 0/6] Armada 370/XP clocksource fixes

[PATCH v4 0/6] Armada 370/XP clocksource fixes

[Ezequiel Garcia]

This small patchset fixes a somewhat minor issue found in the clocksource
driver for Armada 370/XP SoC.

On one side the Armada 370 SoC has no 25 MHz fixed timer.
On the other side the Armada XP SoC cannot work properly without such 25 MHz
fixed timer selected, because otherwise the base clock frequency would vary
when doing cpufreq frequency changes.

Therefore we can consider the SoCs as not being compatible, being better to
have two compatible strings, one for each SoC. The previous compatible and
its behavior has been removed, considering there are no DT-enabled boards
in use in the field.

In addition, CLOCKSOURCE_OF_DECLARE is used to simplify the initialization.

This patchset is based on tip/timers/core.

Changes from v3:

  * Rebased on tip/timers/core.

Changes from v2:

  * As suggested by Jason Cooper, and given there are no DT-enabled
    boards in the field, the previous compatible string is *removed*
    instead of being retained and changes in documentation and .dts
    files are added as needed.

Changes from v1:

  * Declare TIMER_CTRL register access helpers as static,
    as reported by Andrew Lunn.

  * Add some documentation about the deprecated compatible string
    in the clocksource driver, as suggested by Andrew Lunn.

  * Add to the series a cosmetic patch to use BIT()

  * Rebased on top of two patches for the armada-370-xp clocksource
    driver, that are already in linux-next.

  * Reordered the patches: bare cleanup first, improvements later.

Ezequiel Garcia (6):
  clocksource: armada-370-xp: Use BIT()
  clocksource: armada-370-xp: Simplify TIMER_CTRL register access
  clocksource: armada-370-xp: Use CLOCKSOURCE_OF_DECLARE
  clocksource: armada-370-xp: Introduce new compatibles
  clocksource: armada-370-xp: Fix device-tree binding
  ARM: mvebu: Fix the Armada 370/XP timer compatible strings

 .../bindings/timer/marvell,armada-370-xp-timer.txt |  27 ++++-
 arch/arm/boot/dts/armada-370-xp.dtsi               |   2 -
 arch/arm/boot/dts/armada-370.dtsi                  |   5 +
 arch/arm/boot/dts/armada-xp.dtsi                   |   2 +-
 arch/arm/mach-mvebu/armada-370-xp.c                |   4 +-
 drivers/clocksource/time-armada-370-xp.c           | 133 ++++++++++++---------
 include/linux/time-armada-370-xp.h                 |  18 ---
 7 files changed, 104 insertions(+), 87 deletions(-)
 delete mode 100644 include/linux/time-armada-370-xp.h

-- 
1.8.1.5

[PATCH v4 1/6] clocksource: armada-370-xp: Use BIT()

[Ezequiel Garcia]

This is a purely cosmetic commit: we replace hardcoded values that
representing bits by BIT(), which is slightly more readable.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/clocksource/time-armada-370-xp.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/clocksource/time-armada-370-xp.c b/drivers/clocksource/time-armada-370-xp.c
index efdca32..ab22873 100644
--- a/drivers/clocksource/time-armada-370-xp.c
+++ b/drivers/clocksource/time-armada-370-xp.c
@@ -35,13 +35,13 @@
  * Timer block registers.
  */
 #define TIMER_CTRL_OFF		0x0000
-#define  TIMER0_EN		 0x0001
-#define  TIMER0_RELOAD_EN	 0x0002
-#define  TIMER0_25MHZ            0x0800
+#define  TIMER0_EN		 BIT(0)
+#define  TIMER0_RELOAD_EN	 BIT(1)
+#define  TIMER0_25MHZ            BIT(11)
 #define  TIMER0_DIV(div)         ((div) << 19)
-#define  TIMER1_EN		 0x0004
-#define  TIMER1_RELOAD_EN	 0x0008
-#define  TIMER1_25MHZ            0x1000
+#define  TIMER1_EN		 BIT(2)
+#define  TIMER1_RELOAD_EN	 BIT(3)
+#define  TIMER1_25MHZ            BIT(12)
 #define  TIMER1_DIV(div)         ((div) << 22)
 #define TIMER_EVENTS_STATUS	0x0004
 #define  TIMER0_CLR_MASK         (~0x1)
-- 
1.8.1.5

[PATCH v4 2/6] clocksource: armada-370-xp: Simplify TIMER_CTRL register access

[Ezequiel Garcia]

This commit creates two functions to access the TIMER_CTRL register:
one for global one for the per-cpu. This makes the code much more
readable. In addition, since the TIMER_CTRL register is also used for
watchdog, this is preparation work for future thread-safe improvements.

Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/clocksource/time-armada-370-xp.c | 69 ++++++++++++++------------------
 1 file changed, 30 insertions(+), 39 deletions(-)

diff --git a/drivers/clocksource/time-armada-370-xp.c b/drivers/clocksource/time-armada-370-xp.c
index ab22873..78ddf6a 100644
--- a/drivers/clocksource/time-armada-370-xp.c
+++ b/drivers/clocksource/time-armada-370-xp.c
@@ -71,6 +71,18 @@ static u32 ticks_per_jiffy;
 
 static struct clock_event_device __percpu **percpu_armada_370_xp_evt;
 
+static void timer_ctrl_clrset(u32 clr, u32 set)
+{
+	writel((readl(timer_base + TIMER_CTRL_OFF) & ~clr) | set,
+		timer_base + TIMER_CTRL_OFF);
+}
+
+static void local_timer_ctrl_clrset(u32 clr, u32 set)
+{
+	writel((readl(local_base + TIMER_CTRL_OFF) & ~clr) | set,
+		local_base + TIMER_CTRL_OFF);
+}
+
 static u32 notrace armada_370_xp_read_sched_clock(void)
 {
 	return ~readl(timer_base + TIMER0_VAL_OFF);
@@ -83,7 +95,6 @@ static int
 armada_370_xp_clkevt_next_event(unsigned long delta,
 				struct clock_event_device *dev)
 {
-	u32 u;
 	/*
 	 * Clear clockevent timer interrupt.
 	 */
@@ -97,11 +108,8 @@ armada_370_xp_clkevt_next_event(unsigned long delta,
 	/*
 	 * Enable the timer.
 	 */
-	u = readl(local_base + TIMER_CTRL_OFF);
-	u = ((u & ~TIMER0_RELOAD_EN) | TIMER0_EN |
-	     TIMER0_DIV(TIMER_DIVIDER_SHIFT));
-	writel(u, local_base + TIMER_CTRL_OFF);
-
+	local_timer_ctrl_clrset(TIMER0_RELOAD_EN,
+				TIMER0_EN | TIMER0_DIV(TIMER_DIVIDER_SHIFT));
 	return 0;
 }
 
@@ -109,8 +117,6 @@ static void
 armada_370_xp_clkevt_mode(enum clock_event_mode mode,
 			  struct clock_event_device *dev)
 {
-	u32 u;
-
 	if (mode == CLOCK_EVT_MODE_PERIODIC) {
 
 		/*
@@ -122,18 +128,14 @@ armada_370_xp_clkevt_mode(enum clock_event_mode mode,
 		/*
 		 * Enable timer.
 		 */
-
-		u = readl(local_base + TIMER_CTRL_OFF);
-
-		writel((u | TIMER0_EN | TIMER0_RELOAD_EN |
-			TIMER0_DIV(TIMER_DIVIDER_SHIFT)),
-			local_base + TIMER_CTRL_OFF);
+		local_timer_ctrl_clrset(0, TIMER0_RELOAD_EN |
+					   TIMER0_EN |
+					   TIMER0_DIV(TIMER_DIVIDER_SHIFT));
 	} else {
 		/*
 		 * Disable timer.
 		 */
-		u = readl(local_base + TIMER_CTRL_OFF);
-		writel(u & ~TIMER0_EN, local_base + TIMER_CTRL_OFF);
+		local_timer_ctrl_clrset(TIMER0_EN, 0);
 
 		/*
 		 * ACK pending timer interrupt.
@@ -169,18 +171,18 @@ static irqreturn_t armada_370_xp_timer_interrupt(int irq, void *dev_id)
  */
 static int __cpuinit armada_370_xp_timer_setup(struct clock_event_device *evt)
 {
-	u32 u;
+	u32 clr = 0, set = 0;
 	int cpu = smp_processor_id();
 
 	/* Use existing clock_event for cpu 0 */
 	if (!smp_processor_id())
 		return 0;
 
-	u = readl(local_base + TIMER_CTRL_OFF);
 	if (timer25Mhz)
-		writel(u | TIMER0_25MHZ, local_base + TIMER_CTRL_OFF);
+		set = TIMER0_25MHZ;
 	else
-		writel(u & ~TIMER0_25MHZ, local_base + TIMER_CTRL_OFF);
+		clr = TIMER0_25MHZ;
+	local_timer_ctrl_clrset(clr, set);
 
 	evt->name		= armada_370_xp_clkevt.name;
 	evt->irq		= armada_370_xp_clkevt.irq;
@@ -212,7 +214,7 @@ static struct local_timer_ops armada_370_xp_local_timer_ops __cpuinitdata = {
 
 void __init armada_370_xp_timer_init(void)
 {
-	u32 u;
+	u32 clr = 0, set = 0;
 	struct device_node *np;
 	int res;
 
@@ -223,29 +225,20 @@ void __init armada_370_xp_timer_init(void)
 
 	if (of_find_property(np, "marvell,timer-25Mhz", NULL)) {
 		/* The fixed 25MHz timer is available so let's use it */
-		u = readl(local_base + TIMER_CTRL_OFF);
-		writel(u | TIMER0_25MHZ,
-		       local_base + TIMER_CTRL_OFF);
-		u = readl(timer_base + TIMER_CTRL_OFF);
-		writel(u | TIMER0_25MHZ,
-		       timer_base + TIMER_CTRL_OFF);
+		set = TIMER0_25MHZ;
 		timer_clk = 25000000;
 	} else {
 		unsigned long rate = 0;
 		struct clk *clk = of_clk_get(np, 0);
 		WARN_ON(IS_ERR(clk));
 		rate =  clk_get_rate(clk);
-		u = readl(local_base + TIMER_CTRL_OFF);
-		writel(u & ~(TIMER0_25MHZ),
-		       local_base + TIMER_CTRL_OFF);
-
-		u = readl(timer_base + TIMER_CTRL_OFF);
-		writel(u & ~(TIMER0_25MHZ),
-		       timer_base + TIMER_CTRL_OFF);
-
 		timer_clk = rate / TIMER_DIVIDER;
+
+		clr = TIMER0_25MHZ;
 		timer25Mhz = false;
 	}
+	timer_ctrl_clrset(clr, set);
+	local_timer_ctrl_clrset(clr, set);
 
 	/*
 	 * We use timer 0 as clocksource, and private(local) timer 0
@@ -267,10 +260,8 @@ void __init armada_370_xp_timer_init(void)
 	writel(0xffffffff, timer_base + TIMER0_VAL_OFF);
 	writel(0xffffffff, timer_base + TIMER0_RELOAD_OFF);
 
-	u = readl(timer_base + TIMER_CTRL_OFF);
-
-	writel((u | TIMER0_EN | TIMER0_RELOAD_EN |
-		TIMER0_DIV(TIMER_DIVIDER_SHIFT)), timer_base + TIMER_CTRL_OFF);
+	timer_ctrl_clrset(0, TIMER0_EN | TIMER0_RELOAD_EN |
+			     TIMER0_DIV(TIMER_DIVIDER_SHIFT));
 
 	clocksource_mmio_init(timer_base + TIMER0_VAL_OFF,
 			      "armada_370_xp_clocksource",
-- 
1.8.1.5

[PATCH v4 3/6] clocksource: armada-370-xp: Use CLOCKSOURCE_OF_DECLARE

[Ezequiel Garcia]

This is almost cosmetic: we achieve a bit of consistency with
other clocksource drivers by using the CLOCKSOURCE_OF_DECLARE
macro for the boilerplate code.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/mach-mvebu/armada-370-xp.c      |  4 ++--
 drivers/clocksource/time-armada-370-xp.c |  6 +++---
 include/linux/time-armada-370-xp.h       | 18 ------------------
 3 files changed, 5 insertions(+), 23 deletions(-)
 delete mode 100644 include/linux/time-armada-370-xp.h

diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
index 97cbb80..4ea03ad 100644
--- a/arch/arm/mach-mvebu/armada-370-xp.c
+++ b/arch/arm/mach-mvebu/armada-370-xp.c
@@ -18,7 +18,7 @@
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
 #include <linux/io.h>
-#include <linux/time-armada-370-xp.h>
+#include <linux/clocksource.h>
 #include <linux/dma-mapping.h>
 #include <linux/mbus.h>
 #include <asm/hardware/cache-l2x0.h>
@@ -69,7 +69,7 @@ static void __init armada_370_xp_mbus_init(void)
 static void __init armada_370_xp_timer_and_clk_init(void)
 {
 	of_clk_init(NULL);
-	armada_370_xp_timer_init();
+	clocksource_of_init();
 	coherency_init();
 	armada_370_xp_mbus_init();
 #ifdef CONFIG_CACHE_L2X0
diff --git a/drivers/clocksource/time-armada-370-xp.c b/drivers/clocksource/time-armada-370-xp.c
index 78ddf6a..a6405eb 100644
--- a/drivers/clocksource/time-armada-370-xp.c
+++ b/drivers/clocksource/time-armada-370-xp.c
@@ -212,13 +212,11 @@ static struct local_timer_ops armada_370_xp_local_timer_ops __cpuinitdata = {
 	.stop	=  armada_370_xp_timer_stop,
 };
 
-void __init armada_370_xp_timer_init(void)
+static void __init armada_370_xp_timer_init(struct device_node *np)
 {
 	u32 clr = 0, set = 0;
-	struct device_node *np;
 	int res;
 
-	np = of_find_compatible_node(NULL, NULL, "marvell,armada-370-xp-timer");
 	timer_base = of_iomap(np, 0);
 	WARN_ON(!timer_base);
 	local_base = of_iomap(np, 1);
@@ -290,3 +288,5 @@ void __init armada_370_xp_timer_init(void)
 #endif
 	}
 }
+CLOCKSOURCE_OF_DECLARE(armada_370_xp, "marvell,armada-370-xp-timer",
+		       armada_370_xp_timer_init);
diff --git a/include/linux/time-armada-370-xp.h b/include/linux/time-armada-370-xp.h
deleted file mode 100644
index dfdfdc0..0000000
--- a/include/linux/time-armada-370-xp.h
+++ /dev/null
@@ -1,18 +0,0 @@
-/*
- * Marvell Armada 370/XP SoC timer handling.
- *
- * Copyright (C) 2012 Marvell
- *
- * Lior Amsalem <alior@marvell.com>
- * Gregory CLEMENT <gregory.clement@free-electrons.com>
- * Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
- *
- */
-#ifndef __TIME_ARMADA_370_XPPRCMU_H
-#define __TIME_ARMADA_370_XPPRCMU_H
-
-#include <linux/init.h>
-
-void __init armada_370_xp_timer_init(void);
-
-#endif
-- 
1.8.1.5

[PATCH v4 4/6] clocksource: armada-370-xp: Introduce new compatibles

[Ezequiel Garcia]

The Armada XP SoC clocksource driver cannot work without the 25 MHz
fixed timer. Therefore it's appropriate to introduce a new compatible
string and use it to set the 25 MHz fixed timer.

The 'marvell,timer-25MHz' property will be marked as deprecated.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/clocksource/time-armada-370-xp.c | 54 +++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 15 deletions(-)

diff --git a/drivers/clocksource/time-armada-370-xp.c b/drivers/clocksource/time-armada-370-xp.c
index a6405eb..8db4517 100644
--- a/drivers/clocksource/time-armada-370-xp.c
+++ b/drivers/clocksource/time-armada-370-xp.c
@@ -13,6 +13,19 @@
  *
  * Timer 0 is used as free-running clocksource, while timer 1 is
  * used as clock_event_device.
+ *
+ * ---
+ * Clocksource driver for Armada 370 and Armada XP SoC.
+ * This driver implements one compatible string for each SoC, given
+ * each has its own characteristics:
+ *
+ *   * Armada 370 has no 25 MHz fixed timer.
+ *
+ *   * Armada XP cannot work properly without such 25 MHz fixed timer as
+ *     doing otherwise leads to using a clocksource whose frequency varies
+ *     when doing cpufreq frequency changes.
+ *
+ * See Documentation/devicetree/bindings/timer/marvell,armada-370-xp-timer.txt
  */
 
 #include <linux/init.h>
@@ -212,7 +225,7 @@ static struct local_timer_ops armada_370_xp_local_timer_ops __cpuinitdata = {
 	.stop	=  armada_370_xp_timer_stop,
 };
 
-static void __init armada_370_xp_timer_init(struct device_node *np)
+static void __init armada_370_xp_timer_common_init(struct device_node *np)
 {
 	u32 clr = 0, set = 0;
 	int res;
@@ -221,20 +234,10 @@ static void __init armada_370_xp_timer_init(struct device_node *np)
 	WARN_ON(!timer_base);
 	local_base = of_iomap(np, 1);
 
-	if (of_find_property(np, "marvell,timer-25Mhz", NULL)) {
-		/* The fixed 25MHz timer is available so let's use it */
+	if (timer25Mhz)
 		set = TIMER0_25MHZ;
-		timer_clk = 25000000;
-	} else {
-		unsigned long rate = 0;
-		struct clk *clk = of_clk_get(np, 0);
-		WARN_ON(IS_ERR(clk));
-		rate =  clk_get_rate(clk);
-		timer_clk = rate / TIMER_DIVIDER;
-
+	else
 		clr = TIMER0_25MHZ;
-		timer25Mhz = false;
-	}
 	timer_ctrl_clrset(clr, set);
 	local_timer_ctrl_clrset(clr, set);
 
@@ -288,5 +291,26 @@ static void __init armada_370_xp_timer_init(struct device_node *np)
 #endif
 	}
 }
-CLOCKSOURCE_OF_DECLARE(armada_370_xp, "marvell,armada-370-xp-timer",
-		       armada_370_xp_timer_init);
+
+static void __init armada_xp_timer_init(struct device_node *np)
+{
+	/* The fixed 25MHz timer is required, timer25Mhz is true by default */
+	timer_clk = 25000000;
+
+	armada_370_xp_timer_common_init(np);
+}
+CLOCKSOURCE_OF_DECLARE(armada_xp, "marvell,armada-xp-timer",
+		       armada_xp_timer_init);
+
+static void __init armada_370_timer_init(struct device_node *np)
+{
+	struct clk *clk = of_clk_get(np, 0);
+
+	WARN_ON(IS_ERR(clk));
+	timer_clk = clk_get_rate(clk) / TIMER_DIVIDER;
+	timer25Mhz = false;
+
+	armada_370_xp_timer_common_init(np);
+}
+CLOCKSOURCE_OF_DECLARE(armada_370, "marvell,armada-370-timer",
+		       armada_370_timer_init);
-- 
1.8.1.5

[PATCH v4 5/6] clocksource: armada-370-xp: Fix device-tree binding

[Ezequiel Garcia]

This commit fixes the DT binding for the Armada 370/XP SoC timer.
The previous "marvell,armada-370-xp-timer" compatible is removed and
two new compatible strings are introduced: "marvell,armada-xp-timer"
and "marvell,armada-370-timer".

The rationale behind this change is that the Armada 370 SoC and the
Armada XP SoC timers are not really compatible:

  * Armada 370 has no 25 MHz fixed timer.

  * Armada XP cannot work properly without such 25 MHz fixed timer
    as doing otherwise leads to using a clocksource whose frequency
    varies when doing cpufreq frequency changes.

This commit also removes the "marvell,timer-25Mhz" property, given
it's now meaningless.

Cc: devicetree at vger.kernel.org
Acked-by: Jason Cooper <jason@lakedaemon.net>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 .../bindings/timer/marvell,armada-370-xp-timer.txt | 27 ++++++++++++++++++----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/timer/marvell,armada-370-xp-timer.txt b/Documentation/devicetree/bindings/timer/marvell,armada-370-xp-timer.txt
index 3638112..4c453b2 100644
--- a/Documentation/devicetree/bindings/timer/marvell,armada-370-xp-timer.txt
+++ b/Documentation/devicetree/bindings/timer/marvell,armada-370-xp-timer.txt
@@ -2,14 +2,31 @@ Marvell Armada 370 and Armada XP Timers
 ---------------------------------------
 
 Required properties:
-- compatible: Should be "marvell,armada-370-xp-timer"
+- compatible: Should be either "marvell,armada-370-timer" or
+  "marvell,armada-xp-timer" as appropriate.
 - interrupts: Should contain the list of Global Timer interrupts and
   then local timer interrupts
 - reg: Should contain location and length for timers register. First
   pair for the Global Timer registers, second pair for the
   local/private timers.
-- clocks: clock driving the timer hardware
+- clocks: clock driving the timer hardware, only required for
+  "marvell,armada-370-timer";
 
-Optional properties:
-- marvell,timer-25Mhz: Tells whether the Global timer supports the 25
-  Mhz fixed mode (available on Armada XP and not on Armada 370)
+Examples:
+
+- Armada 370:
+
+	timer {
+		compatible = "marvell,armada-370-timer";
+		reg = <0x20300 0x30>, <0x21040 0x30>;
+		interrupts = <37>, <38>, <39>, <40>, <5>, <6>;
+		clocks = <&coreclk 2>;
+	};
+
+- Armada XP:
+
+	timer {
+		compatible = "marvell,armada-xp-timer";
+		reg = <0x20300 0x30>, <0x21040 0x30>;
+		interrupts = <37>, <38>, <39>, <40>, <5>, <6>;
+	};
-- 
1.8.1.5

[PATCH v4 5/6] clocksource: armada-370-xp: Fix device-tree binding

[Mark Rutland]

Hi,

On Tue, Aug 13, 2013 at 03:43:14PM +0100, Ezequiel Garcia wrote:
> This commit fixes the DT binding for the Armada 370/XP SoC timer.
> The previous "marvell,armada-370-xp-timer" compatible is removed and
> two new compatible strings are introduced: "marvell,armada-xp-timer"
> and "marvell,armada-370-timer".
> 
> The rationale behind this change is that the Armada 370 SoC and the
> Armada XP SoC timers are not really compatible:
> 
>   * Armada 370 has no 25 MHz fixed timer.
> 
>   * Armada XP cannot work properly without such 25 MHz fixed timer
>     as doing otherwise leads to using a clocksource whose frequency
>     varies when doing cpufreq frequency changes.
> 
> This commit also removes the "marvell,timer-25Mhz" property, given
> it's now meaningless.

Given we have a mechanism already for handling this, I'm not sure. The
new binding certainly looks nicer (and if there's no-one using it, then
migrating makes sense), but I still have concerns about it. From the
description of the problem, and a look at the driver, the timer hardware
actually has (at least) two clock inputs:

(a) Wired to a fixed 25MHz clock on the XP, but not described.
    Not wired to anything on the 370?

(b) Wired to the CPU clock on the XP?
    Wired to some external clock on the 370.

If that's the case, this should be described, even if we can't use that
information right now. (a) can be a 25MHz fixed-clock on the XP, and
just not described on the 370. We can use clock-names to handle the
optional presence of any input clock.

Having separate compatible strings may still make sense, but I'd prefer
to understand the hardware better first. Not having full visibility over
a piece of hardware at review stage is precisely why we get into a mess
trying to change bindings later.

Thanks,
Mark.

> 
> Cc: devicetree at vger.kernel.org
> Acked-by: Jason Cooper <jason@lakedaemon.net>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  .../bindings/timer/marvell,armada-370-xp-timer.txt | 27 ++++++++++++++++++----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/timer/marvell,armada-370-xp-timer.txt b/Documentation/devicetree/bindings/timer/marvell,armada-370-xp-timer.txt
> index 3638112..4c453b2 100644
> --- a/Documentation/devicetree/bindings/timer/marvell,armada-370-xp-timer.txt
> +++ b/Documentation/devicetree/bindings/timer/marvell,armada-370-xp-timer.txt
> @@ -2,14 +2,31 @@ Marvell Armada 370 and Armada XP Timers
>  ---------------------------------------
>  
>  Required properties:
> -- compatible: Should be "marvell,armada-370-xp-timer"
> +- compatible: Should be either "marvell,armada-370-timer" or
> +  "marvell,armada-xp-timer" as appropriate.
>  - interrupts: Should contain the list of Global Timer interrupts and
>    then local timer interrupts
>  - reg: Should contain location and length for timers register. First
>    pair for the Global Timer registers, second pair for the
>    local/private timers.
> -- clocks: clock driving the timer hardware
> +- clocks: clock driving the timer hardware, only required for
> +  "marvell,armada-370-timer";
>  
> -Optional properties:
> -- marvell,timer-25Mhz: Tells whether the Global timer supports the 25
> -  Mhz fixed mode (available on Armada XP and not on Armada 370)
> +Examples:
> +
> +- Armada 370:
> +
> +	timer {
> +		compatible = "marvell,armada-370-timer";
> +		reg = <0x20300 0x30>, <0x21040 0x30>;
> +		interrupts = <37>, <38>, <39>, <40>, <5>, <6>;
> +		clocks = <&coreclk 2>;
> +	};
> +
> +- Armada XP:
> +
> +	timer {
> +		compatible = "marvell,armada-xp-timer";
> +		reg = <0x20300 0x30>, <0x21040 0x30>;
> +		interrupts = <37>, <38>, <39>, <40>, <5>, <6>;
> +	};
> -- 
> 1.8.1.5
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH v4 5/6] clocksource: armada-370-xp: Fix device-tree binding

[Ezequiel Garcia]

Hi Mark,

Thanks a lot for reviewing this.

On Wed, Aug 14, 2013 at 04:26:21PM +0100, Mark Rutland wrote:
> 
> On Tue, Aug 13, 2013 at 03:43:14PM +0100, Ezequiel Garcia wrote:
> > This commit fixes the DT binding for the Armada 370/XP SoC timer.
> > The previous "marvell,armada-370-xp-timer" compatible is removed and
> > two new compatible strings are introduced: "marvell,armada-xp-timer"
> > and "marvell,armada-370-timer".
> > 
> > The rationale behind this change is that the Armada 370 SoC and the
> > Armada XP SoC timers are not really compatible:
> > 
> >   * Armada 370 has no 25 MHz fixed timer.
> > 
> >   * Armada XP cannot work properly without such 25 MHz fixed timer
> >     as doing otherwise leads to using a clocksource whose frequency
> >     varies when doing cpufreq frequency changes.
> > 
> > This commit also removes the "marvell,timer-25Mhz" property, given
> > it's now meaningless.
> 
> Given we have a mechanism already for handling this, I'm not sure. The
> new binding certainly looks nicer (and if there's no-one using it, then
> migrating makes sense), but I still have concerns about it. From the
> description of the problem, and a look at the driver, the timer hardware
> actually has (at least) two clock inputs:
> 
> (a) Wired to a fixed 25MHz clock on the XP, but not described.
>     Not wired to anything on the 370?
> 
> (b) Wired to the CPU clock on the XP?
>     Wired to some external clock on the 370.
> 

Mmm.. I don't think the above describes the situation.

> If that's the case, this should be described, even if we can't use that
> information right now. (a) can be a 25MHz fixed-clock on the XP, and
> just not described on the 370. We can use clock-names to handle the
> optional presence of any input clock.
> 
> Having separate compatible strings may still make sense, but I'd prefer
> to understand the hardware better first. Not having full visibility over
> a piece of hardware at review stage is precisely why we get into a mess
> trying to change bindings later.
> 

I agree completely, let me try to describe the hardware better then.

First of all let's consider Armada 370 and Armada XP as not compatible,
they are different hardwares from my point of view.

Armada 370
----------

A single clock source is available for timer and watchdog counters:

Timer and watchdog counters decrement rate is a configurable ratio
of the L2/coherency fabric clock. The current clocksource driver
implementation chooses an abritrary ratio.

So, for this hardware the DT binding proposed in this patch is, IMO, accurate:

timer {
	compatible = "marvell,armada-370-timer";
	reg = ...
	interrupts = ...
	clocks = <&coreclk 2>; /*  L2/coherency fabric clock  */
};

Armada XP
---------

Two clock sources are available for timer and watchdog counters:

Just as explained for the Armada 370, the timer and watchdog counters decrement
rate is a configurable ratio of the L2/coherency fabric clock.
The current clocksource driver implementation chooses an abritrary ratio.

In addition to this, both timer and watchdog counter rate can be configured
to use an (internal) 25 MHz fixed clock.

However, and as explained in this patchset, using the letter fixed clock is in
practice the only choice. Once CPUFreq support is implemented for the
Armada XP SoC the L2/coherency fabric clock will change its rate, and handling
such change will be problematic.

For this reason, the DT binding proposed is:

timer {
	compatible = "marvell,armada-xp-timer";
	reg = ...
	interrupts = ...
};

Maybe the most accurate representation would be something like this...

timer {
	compatible = "marvell,armada-xp-timer";
	reg = ...
	interrupts = ...
	clocks = ... /* either 25 MHz fixed clock
	              * or L2/coherency fabric clock */
};

... adding a clock property to represent the actual clock source,
but I think such property can be added later on top of the current
patchset.

In other words, from my point of view, the current proposal is fine,
and represents the hardware in the best way possible.

Of course, I'm open to discussion. What do you think?
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v4 5/6] clocksource: armada-370-xp: Fix device-tree binding

[Stephen Warren]

On 08/15/2013 10:27 AM, Ezequiel Garcia wrote:
...
> Armada XP
> ---------
> 
> Two clock sources are available for timer and watchdog counters:
> 
> Just as explained for the Armada 370, the timer and watchdog counters decrement
> rate is a configurable ratio of the L2/coherency fabric clock.
> The current clocksource driver implementation chooses an abritrary ratio.
> 
> In addition to this, both timer and watchdog counter rate can be configured
> to use an (internal) 25 MHz fixed clock.

So there are clearly two clocks fed into the HW block here. The DT
should reflect that.

> However, and as explained in this patchset, using the letter fixed clock is in
> practice the only choice. Once CPUFreq support is implemented for the
> Armada XP SoC the L2/coherency fabric clock will change its rate, and handling
> such change will be problematic.

It's not mandatory for an OS to implement cpufreq, so it'd be quite
possible to still use that variable-rate clock on some HW. What SW
may-or-may-not do on HW isn't something that should feed into the DT
binding design.

> Maybe the most accurate representation would be something like this...
> 
> timer {
> 	compatible = "marvell,armada-xp-timer";
> 	reg = ...
> 	interrupts = ...
> 	clocks = ... /* either 25 MHz fixed clock
> 	              * or L2/coherency fabric clock */
> };

I think better would be:

clocks = <&xxx ...> <&yyy ...>;
clock-names = "fabric", "fixed";

In the documentation for this HW block, there's a register that switches
between the two clock sources. What names are used for the clocks in the
documentation of that register? Those names should be used for
clock-names entries.

Is the fact that one of "fabric" and "fixed" is variable and one
fixed-rate more a facet of the integration of the IP block into the SoC,
and not forced by the design of the IP block itself? If so, perhaps
you'd want some additional properties indicating which of those clocks
was fixed and which was variable, which would allow SW to make
intelligent decisions re: which to use. Perhaps it can be assumed this
information can be queried from the source of the clock though?

[PATCH v4 5/6] clocksource: armada-370-xp: Fix device-tree binding

[Tomasz Figa]

On Friday 16 of August 2013 17:29:00 Stephen Warren wrote:
> On 08/15/2013 10:27 AM, Ezequiel Garcia wrote:
> ...
> 
> > Armada XP
> > ---------
> > 
> > Two clock sources are available for timer and watchdog counters:
> > 
> > Just as explained for the Armada 370, the timer and watchdog counters
> > decrement rate is a configurable ratio of the L2/coherency fabric
> > clock. The current clocksource driver implementation chooses an
> > abritrary ratio.
> > 
> > In addition to this, both timer and watchdog counter rate can be
> > configured to use an (internal) 25 MHz fixed clock.
> 
> So there are clearly two clocks fed into the HW block here. The DT
> should reflect that.

I fully agree. DT should list all the input clocks that are fed into the 
IP block being described.

> > However, and as explained in this patchset, using the letter fixed
> > clock is in practice the only choice. Once CPUFreq support is
> > implemented for the Armada XP SoC the L2/coherency fabric clock will
> > change its rate, and handling such change will be problematic.
> 
> It's not mandatory for an OS to implement cpufreq, so it'd be quite
> possible to still use that variable-rate clock on some HW. What SW
> may-or-may-not do on HW isn't something that should feed into the DT
> binding design.
> 
> > Maybe the most accurate representation would be something like this...
> > 
> > timer {
> > 
> > 	compatible = "marvell,armada-xp-timer";
> > 	reg = ...
> > 	interrupts = ...
> > 	clocks = ... /* either 25 MHz fixed clock
> > 	
> > 	              * or L2/coherency fabric clock */
> > 
> > };
> 
> I think better would be:
> 
> clocks = <&xxx ...> <&yyy ...>;
> clock-names = "fabric", "fixed";
> 
> In the documentation for this HW block, there's a register that switches
> between the two clock sources. What names are used for the clocks in
> the documentation of that register? Those names should be used for
> clock-names entries.
> 
> Is the fact that one of "fabric" and "fixed" is variable and one
> fixed-rate more a facet of the integration of the IP block into the SoC,
> and not forced by the design of the IP block itself? If so, perhaps
> you'd want some additional properties indicating which of those clocks
> was fixed and which was variable, which would allow SW to make
> intelligent decisions re: which to use. Perhaps it can be assumed this
> information can be queried from the source of the clock though?

Well, I believe this kind of information can be hardcoded in the driver 
itself, which can simply know which input clock to use on given platform, 
in the same way it currently knows what divisor to use.

Sure it isn't the ideal solution, but you can't know which of the clocks 
is going to be reconfigured by other drivers, as this might depend on the 
built-in set of drivers, particular OS (as you said, not all OSes might be 
implementing cpufreq, even the same OS, but with different configs might 
differ in this aspect).

Best regards,
Tomasz

[PATCH v4 5/6] clocksource: armada-370-xp: Fix device-tree binding

[Sebastian Hesselbarth]

On 08/17/2013 02:09 PM, Tomasz Figa wrote:
> On Friday 16 of August 2013 17:29:00 Stephen Warren wrote:
>> On 08/15/2013 10:27 AM, Ezequiel Garcia wrote:
>> ...
>>
>>> Armada XP
>>> ---------
>>>
>>> Two clock sources are available for timer and watchdog counters:
>>>
>>> Just as explained for the Armada 370, the timer and watchdog counters
>>> decrement rate is a configurable ratio of the L2/coherency fabric
>>> clock. The current clocksource driver implementation chooses an
>>> abritrary ratio.
>>>
>>> In addition to this, both timer and watchdog counter rate can be
>>> configured to use an (internal) 25 MHz fixed clock.
>>
>> So there are clearly two clocks fed into the HW block here. The DT
>> should reflect that.
>
> I fully agree. DT should list all the input clocks that are fed into the
> IP block being described.

I don't object to the above, but strictly speaking the consequence
would be, that all nodes require a clocks property. For A370/XP timer
the fabric clock is configurable and needs to be passed among core
clocks and timer, the 25MHz clock is not and _could_ be seen as an extra
feature of the core.

But in the end, passing it by DT should be the way to go. I cannot look
into the XP datasheet, but I would guess that the exact feature of the
ip is not to use _the_ fixed 25MHz clock but XTAL as reference. Maybe
one of the free-electrons guys can look it up?

Sebastian

[PATCH v4 5/6] clocksource: armada-370-xp: Fix device-tree binding

[Tomasz Figa]

On Saturday 17 of August 2013 14:29:28 Sebastian Hesselbarth wrote:
> On 08/17/2013 02:09 PM, Tomasz Figa wrote:
> > On Friday 16 of August 2013 17:29:00 Stephen Warren wrote:
> >> On 08/15/2013 10:27 AM, Ezequiel Garcia wrote:
> >> ...
> >> 
> >>> Armada XP
> >>> ---------
> >>> 
> >>> Two clock sources are available for timer and watchdog counters:
> >>> 
> >>> Just as explained for the Armada 370, the timer and watchdog
> >>> counters
> >>> decrement rate is a configurable ratio of the L2/coherency fabric
> >>> clock. The current clocksource driver implementation chooses an
> >>> abritrary ratio.
> >>> 
> >>> In addition to this, both timer and watchdog counter rate can be
> >>> configured to use an (internal) 25 MHz fixed clock.
> >> 
> >> So there are clearly two clocks fed into the HW block here. The DT
> >> should reflect that.
> > 
> > I fully agree. DT should list all the input clocks that are fed into
> > the IP block being described.
> 
> I don't object to the above, but strictly speaking the consequence
> would be, that all nodes require a clocks property.

Well, I should have added something like "if this is relevant to the IP 
being described", e.g. it can select its operating or I/O clock from those 
input clocks.

> For A370/XP timer
> the fabric clock is configurable and needs to be passed among core
> clocks and timer, the 25MHz clock is not and _could_ be seen as an extra
> feature of the core.

Well, so the core allows selection between both clocks and so both should 
be listed IMHO.

Best regards,
Tomasz

[PATCH v4 5/6] clocksource: armada-370-xp: Fix device-tree binding

[Ezequiel Garcia]

On Sat, Aug 17, 2013 at 02:29:28PM +0200, Sebastian Hesselbarth wrote:
> On 08/17/2013 02:09 PM, Tomasz Figa wrote:
> > On Friday 16 of August 2013 17:29:00 Stephen Warren wrote:
> >> On 08/15/2013 10:27 AM, Ezequiel Garcia wrote:
> >> ...
> >>
> >>> Armada XP
> >>> ---------
> >>>
> >>> Two clock sources are available for timer and watchdog counters:
> >>>
> >>> Just as explained for the Armada 370, the timer and watchdog counters
> >>> decrement rate is a configurable ratio of the L2/coherency fabric
> >>> clock. The current clocksource driver implementation chooses an
> >>> abritrary ratio.
> >>>
> >>> In addition to this, both timer and watchdog counter rate can be
> >>> configured to use an (internal) 25 MHz fixed clock.
> >>
> >> So there are clearly two clocks fed into the HW block here. The DT
> >> should reflect that.
> >
> > I fully agree. DT should list all the input clocks that are fed into the
> > IP block being described.
> 
> I don't object to the above, but strictly speaking the consequence
> would be, that all nodes require a clocks property. For A370/XP timer
> the fabric clock is configurable and needs to be passed among core
> clocks and timer, the 25MHz clock is not and _could_ be seen as an extra
> feature of the core.
> 

In fact: I'm not sure. I'm slightly inclined towards considering both
clocks as clock sources, just as Stephen and Tomasz are proposing.

> But in the end, passing it by DT should be the way to go. I cannot look
> into the XP datasheet, but I would guess that the exact feature of the
> ip is not to use _the_ fixed 25MHz clock but XTAL as reference. Maybe
> one of the free-electrons guys can look it up?
> 

No, the documentation has a register bit for "25Mhz frequency enable",
for each timer/watchdog.

---

Anyway, I (almost) agree that the 25Mhz fixed clock must be somehow represented
in the device-tree, but I'm not exactly sure how. Gregory: maybe you can help
in this?

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v4 5/6] clocksource: armada-370-xp: Fix device-tree binding

[Sebastian Hesselbarth]

On 08/17/2013 06:43 PM, Ezequiel Garcia wrote:
> On Sat, Aug 17, 2013 at 02:29:28PM +0200, Sebastian Hesselbarth wrote:
>
> In fact: I'm not sure. I'm slightly inclined towards considering both
> clocks as clock sources, just as Stephen and Tomasz are proposing.
>
>> But in the end, passing it by DT should be the way to go. I cannot look
>> into the XP datasheet, but I would guess that the exact feature of the
>> ip is not to use _the_ fixed 25MHz clock but XTAL as reference. Maybe
>> one of the free-electrons guys can look it up?
>>
>
> No, the documentation has a register bit for "25Mhz frequency enable",
> for each timer/watchdog.
>
> ---
>
> Anyway, I (almost) agree that the 25Mhz fixed clock must be somehow represented
> in the device-tree, but I'm not exactly sure how. Gregory: maybe you can help
> in this?
>

In armada-370-xp.dtsi add a 25MHz ref clock node and modify timer node
to reference it:

cpus { ... };

clocks {
	ref25M: timer-clock {
		compatible = "fixed-clock";
		clock-frequency = <25000000>;
	};
}

...

internal-regs {
	timer at 20300 {
		compatible = "marvell,armada-370-xp-timer";
		reg = <0x20300 0x30>, <0x21040 0x30>;
		interrupts = <37>, <38>, <39>, <40>, <5>, <6>;
		clocks = <&coreclk 2>, <&ref25M>;
		clock-names = "fabric", "fixed";
	};
};

Then use of_clk_get_by_name(np, "fabric") and of_clk_get_by_name(np,
"fixed") respectively. Clock select strategy should be, (a) fail if
no clock, (b) use fabric/fixed if just one clock, (c) use fixed if
both clocks.

Does that answer your questions? :)

Sebastian

[PATCH v4 5/6] clocksource: armada-370-xp: Fix device-tree binding

[Tomasz Figa]

On Monday 19 of August 2013 01:33:18 Sebastian Hesselbarth wrote:
> On 08/17/2013 06:43 PM, Ezequiel Garcia wrote:
> > On Sat, Aug 17, 2013 at 02:29:28PM +0200, Sebastian Hesselbarth wrote:
> > 
> > In fact: I'm not sure. I'm slightly inclined towards considering both
> > clocks as clock sources, just as Stephen and Tomasz are proposing.
> > 
> >> But in the end, passing it by DT should be the way to go. I cannot
> >> look
> >> into the XP datasheet, but I would guess that the exact feature of
> >> the
> >> ip is not to use _the_ fixed 25MHz clock but XTAL as reference. Maybe
> >> one of the free-electrons guys can look it up?
> > 
> > No, the documentation has a register bit for "25Mhz frequency enable",
> > for each timer/watchdog.
> > 
> > ---
> > 
> > Anyway, I (almost) agree that the 25Mhz fixed clock must be somehow
> > represented in the device-tree, but I'm not exactly sure how.
> > Gregory: maybe you can help in this?
> 
> In armada-370-xp.dtsi add a 25MHz ref clock node and modify timer node
> to reference it:
> 
> cpus { ... };
> 
> clocks {
> 	ref25M: timer-clock {
> 		compatible = "fixed-clock";
> 		clock-frequency = <25000000>;
> 	};
> }
> 
> ...
> 
> internal-regs {
> 	timer at 20300 {
> 		compatible = "marvell,armada-370-xp-timer";
> 		reg = <0x20300 0x30>, <0x21040 0x30>;
> 		interrupts = <37>, <38>, <39>, <40>, <5>, <6>;
> 		clocks = <&coreclk 2>, <&ref25M>;
> 		clock-names = "fabric", "fixed";

These names look more like SoC-level clock names, not clock input names of 
the IP. Are they referenced by such names in documentation?

> 	};
> };
> 
> Then use of_clk_get_by_name(np, "fabric") and of_clk_get_by_name(np,
> "fixed") respectively. Clock select strategy should be, (a) fail if
> no clock, (b) use fabric/fixed if just one clock, (c) use fixed if
> both clocks.

This basically sounds good, except those names.

Best regards,
Tomasz

[PATCH v4 5/6] clocksource: armada-370-xp: Fix device-tree binding

[Ezequiel Garcia]

On Mon, Aug 19, 2013 at 01:01:12AM +0200, Tomasz Figa wrote:
> On Monday 19 of August 2013 01:33:18 Sebastian Hesselbarth wrote:
> > On 08/17/2013 06:43 PM, Ezequiel Garcia wrote:
> > > On Sat, Aug 17, 2013 at 02:29:28PM +0200, Sebastian Hesselbarth wrote:
> > > 
> > > In fact: I'm not sure. I'm slightly inclined towards considering both
> > > clocks as clock sources, just as Stephen and Tomasz are proposing.
> > > 
> > >> But in the end, passing it by DT should be the way to go. I cannot
> > >> look
> > >> into the XP datasheet, but I would guess that the exact feature of
> > >> the
> > >> ip is not to use _the_ fixed 25MHz clock but XTAL as reference. Maybe
> > >> one of the free-electrons guys can look it up?
> > > 
> > > No, the documentation has a register bit for "25Mhz frequency enable",
> > > for each timer/watchdog.
> > > 
> > > ---
> > > 
> > > Anyway, I (almost) agree that the 25Mhz fixed clock must be somehow
> > > represented in the device-tree, but I'm not exactly sure how.
> > > Gregory: maybe you can help in this?
> > 
> > In armada-370-xp.dtsi add a 25MHz ref clock node and modify timer node
> > to reference it:
> > 
> > cpus { ... };
> > 
> > clocks {
> > 	ref25M: timer-clock {
> > 		compatible = "fixed-clock";
> > 		clock-frequency = <25000000>;
> > 	};
> > }
> > 
> > ...
> > 
> > internal-regs {
> > 	timer at 20300 {
> > 		compatible = "marvell,armada-370-xp-timer";
> > 		reg = <0x20300 0x30>, <0x21040 0x30>;
> > 		interrupts = <37>, <38>, <39>, <40>, <5>, <6>;
> > 		clocks = <&coreclk 2>, <&ref25M>;
> > 		clock-names = "fabric", "fixed";
> 
> These names look more like SoC-level clock names, not clock input names of 
> the IP. Are they referenced by such names in documentation?
> 

The L2/Coherency fabric clock is called "nbclk" both in marvell's documentation
and in Documentation/device-tree/binding/clock/mvebu-core-clock.txt.

The 25 MHz clock is (very scarcely) called "refclk".

> > 	};
> > };
> > 
> > Then use of_clk_get_by_name(np, "fabric") and of_clk_get_by_name(np,
> > "fixed") respectively. Clock select strategy should be, (a) fail if
> > no clock, (b) use fabric/fixed if just one clock, (c) use fixed if
> > both clocks.
> 
> This basically sounds good, except those names.
> 

Okey, I've just sent a new patchset, on top of the previous (already
applied) work.

Feel free to comment on that.

Sebastian: Thanks a lot for suggesting this!
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v4 5/6] clocksource: armada-370-xp: Fix device-tree binding

[Ezequiel Garcia]

On Mon, Aug 19, 2013 at 01:33:18AM +0200, Sebastian Hesselbarth wrote:
> On 08/17/2013 06:43 PM, Ezequiel Garcia wrote:
> > On Sat, Aug 17, 2013 at 02:29:28PM +0200, Sebastian Hesselbarth wrote:
> >
> > In fact: I'm not sure. I'm slightly inclined towards considering both
> > clocks as clock sources, just as Stephen and Tomasz are proposing.
> >
> >> But in the end, passing it by DT should be the way to go. I cannot look
> >> into the XP datasheet, but I would guess that the exact feature of the
> >> ip is not to use _the_ fixed 25MHz clock but XTAL as reference. Maybe
> >> one of the free-electrons guys can look it up?
> >>
> >
> > No, the documentation has a register bit for "25Mhz frequency enable",
> > for each timer/watchdog.
> >
> > ---
> >
> > Anyway, I (almost) agree that the 25Mhz fixed clock must be somehow represented
> > in the device-tree, but I'm not exactly sure how. Gregory: maybe you can help
> > in this?
> >
> 
> In armada-370-xp.dtsi add a 25MHz ref clock node and modify timer node
> to reference it:
> 
> cpus { ... };
> 
> clocks {
> 	ref25M: timer-clock {
> 		compatible = "fixed-clock";
> 		clock-frequency = <25000000>;
> 	};
> }
> 
> ...
> 
> internal-regs {
> 	timer at 20300 {
> 		compatible = "marvell,armada-370-xp-timer";
> 		reg = <0x20300 0x30>, <0x21040 0x30>;
> 		interrupts = <37>, <38>, <39>, <40>, <5>, <6>;
> 		clocks = <&coreclk 2>, <&ref25M>;
> 		clock-names = "fabric", "fixed";
> 	};
> };
> 
> Then use of_clk_get_by_name(np, "fabric") and of_clk_get_by_name(np,
> "fixed") respectively. Clock select strategy should be, (a) fail if
> no clock, (b) use fabric/fixed if just one clock, (c) use fixed if
> both clocks.
> 
> Does that answer your questions? :)
> 

Indeed this looks nice :-)

However, I still have a few concerns. I thought we would describe the
clock selection in the DT. With the above we return to the single
compatible string "armada-370-xp-timer" for both SoCs.

I'm not entirely convinced. Isn't it better to distinguish them?
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v4 5/6] clocksource: armada-370-xp: Fix device-tree binding

[Ezequiel Garcia]

On Fri, Aug 16, 2013 at 05:29:00PM -0600, Stephen Warren wrote:
> On 08/15/2013 10:27 AM, Ezequiel Garcia wrote:
> ...
> > Armada XP
> > ---------
> > 
> > Two clock sources are available for timer and watchdog counters:
> > 
> > Just as explained for the Armada 370, the timer and watchdog counters decrement
> > rate is a configurable ratio of the L2/coherency fabric clock.
> > The current clocksource driver implementation chooses an abritrary ratio.
> > 
> > In addition to this, both timer and watchdog counter rate can be configured
> > to use an (internal) 25 MHz fixed clock.
> 
> So there are clearly two clocks fed into the HW block here. The DT
> should reflect that.
> 

Right.

> > However, and as explained in this patchset, using the letter fixed clock is in
> > practice the only choice. Once CPUFreq support is implemented for the
> > Armada XP SoC the L2/coherency fabric clock will change its rate, and handling
> > such change will be problematic.
> 
> It's not mandatory for an OS to implement cpufreq, so it'd be quite
> possible to still use that variable-rate clock on some HW. What SW
> may-or-may-not do on HW isn't something that should feed into the DT
> binding design.
> 

Agreed.

> > Maybe the most accurate representation would be something like this...
> > 
> > timer {
> > 	compatible = "marvell,armada-xp-timer";
> > 	reg = ...
> > 	interrupts = ...
> > 	clocks = ... /* either 25 MHz fixed clock
> > 	              * or L2/coherency fabric clock */
> > };
> 
> I think better would be:
> 
> clocks = <&xxx ...> <&yyy ...>;
> clock-names = "fabric", "fixed";
> 
> In the documentation for this HW block, there's a register that switches
> between the two clock sources. What names are used for the clocks in the
> documentation of that register? Those names should be used for
> clock-names entries.
> 

Strictly speaking the documentation says the timer/watchdog counters
are incremented at a configurable rate of the "coherency fabric clock".

In addition, there's a register bit that enables/disables a
"25Mhz frequency" rate for each timer/watchdog.

The names between "" are the names used in the documentation: "coherency
fabric clock" and "25Mhz frequency" (yes, it's odd).

> Is the fact that one of "fabric" and "fixed" is variable and one
> fixed-rate more a facet of the integration of the IP block into the SoC,
> and not forced by the design of the IP block itself? If so, perhaps
> you'd want some additional properties indicating which of those clocks
> was fixed and which was variable, which would allow SW to make
> intelligent decisions re: which to use. Perhaps it can be assumed this
> information can be queried from the source of the clock though?

Not sure about this, but I think I agree with Tomasz Figa in that
this kind of details can be inside the driver.

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v4 6/6] ARM: mvebu: Fix the Armada 370/XP timer compatible strings

[Ezequiel Garcia]

The "marvell,armada-370-xp-timer" compatible string, together with
the "marvell,timer-25Mhz" property are deprecated and should be
removed from current DT.

Instead, the timer DT nodes are now required to have an appropriate
compatible string, which should be either "marvell,armada-370-timer"
or "marvell,armada-xp-timer", depending on SoC.

The clock property is now required only for Armada 370 so move it accordingly.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-xp.dtsi | 2 --
 arch/arm/boot/dts/armada-370.dtsi    | 5 +++++
 arch/arm/boot/dts/armada-xp.dtsi     | 2 +-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 90b1176..442c84e 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -81,10 +81,8 @@
 			};
 
 			timer at 20300 {
-				compatible = "marvell,armada-370-xp-timer";
 				reg = <0x20300 0x30>, <0x21040 0x30>;
 				interrupts = <37>, <38>, <39>, <40>, <5>, <6>;
-				clocks = <&coreclk 2>;
 			};
 
 			sata at a0000 {
diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index fa3dfc6..f166367 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -104,6 +104,11 @@
 				interrupts = <91>;
 			};
 
+			timer at 20300 {
+				compatible = "marvell,armada-370-timer";
+				clocks = <&coreclk 2>;
+			};
+
 			coreclk: mvebu-sar at 18230 {
 				compatible = "marvell,armada-370-core-clock";
 				reg = <0x18230 0x08>;
diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
index 416eb94..549151e 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -62,7 +62,7 @@
 			};
 
 			timer at 20300 {
-				marvell,timer-25Mhz;
+				compatible = "marvell,armada-xp-timer";
 			};
 
 			coreclk: mvebu-sar at 18230 {
-- 
1.8.1.5

[PATCH v4 6/6] ARM: mvebu: Fix the Armada 370/XP timer compatible strings

[Jason Cooper]

On Tue, Aug 13, 2013 at 11:43:15AM -0300, Ezequiel Garcia wrote:
> The "marvell,armada-370-xp-timer" compatible string, together with
> the "marvell,timer-25Mhz" property are deprecated and should be
> removed from current DT.
> 
> Instead, the timer DT nodes are now required to have an appropriate
> compatible string, which should be either "marvell,armada-370-timer"
> or "marvell,armada-xp-timer", depending on SoC.
> 
> The clock property is now required only for Armada 370 so move it accordingly.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  arch/arm/boot/dts/armada-370-xp.dtsi | 2 --
>  arch/arm/boot/dts/armada-370.dtsi    | 5 +++++
>  arch/arm/boot/dts/armada-xp.dtsi     | 2 +-
>  3 files changed, 6 insertions(+), 3 deletions(-)

Applied to mvebu/dt

thx,

Jason.

[PATCH v4 0/6] Armada 370/XP clocksource fixes

[Daniel Lezcano]

On 08/13/2013 04:43 PM, Ezequiel Garcia wrote:
> This small patchset fixes a somewhat minor issue found in the clocksource
> driver for Armada 370/XP SoC.
> 
> On one side the Armada 370 SoC has no 25 MHz fixed timer.
> On the other side the Armada XP SoC cannot work properly without such 25 MHz
> fixed timer selected, because otherwise the base clock frequency would vary
> when doing cpufreq frequency changes.
> 
> Therefore we can consider the SoCs as not being compatible, being better to
> have two compatible strings, one for each SoC. The previous compatible and
> its behavior has been removed, considering there are no DT-enabled boards
> in use in the field.
> 
> In addition, CLOCKSOURCE_OF_DECLARE is used to simplify the initialization.
> 
> This patchset is based on tip/timers/core.
> 

Applied to my tree as 3.12 material.

Thanks
  -- Daniel

-- 
 <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

[PATCH v4 0/6] Armada 370/XP clocksource fixes

[Jason Cooper]

Daniel,

On Tue, Aug 13, 2013 at 06:22:48PM +0200, Daniel Lezcano wrote:
> On 08/13/2013 04:43 PM, Ezequiel Garcia wrote:
> > This small patchset fixes a somewhat minor issue found in the clocksource
> > driver for Armada 370/XP SoC.
> > 
> > On one side the Armada 370 SoC has no 25 MHz fixed timer.
> > On the other side the Armada XP SoC cannot work properly without such 25 MHz
> > fixed timer selected, because otherwise the base clock frequency would vary
> > when doing cpufreq frequency changes.
> > 
> > Therefore we can consider the SoCs as not being compatible, being better to
> > have two compatible strings, one for each SoC. The previous compatible and
> > its behavior has been removed, considering there are no DT-enabled boards
> > in use in the field.
> > 
> > In addition, CLOCKSOURCE_OF_DECLARE is used to simplify the initialization.
> > 
> > This patchset is based on tip/timers/core.
> > 
> 
> Applied to my tree as 3.12 material.

Did you take patch #6 as well?

thx,

Jason.

[PATCH v4 0/6] Armada 370/XP clocksource fixes

[Daniel Lezcano]

On 08/13/2013 06:52 PM, Jason Cooper wrote:
> Daniel,
> 
> On Tue, Aug 13, 2013 at 06:22:48PM +0200, Daniel Lezcano wrote:
>> On 08/13/2013 04:43 PM, Ezequiel Garcia wrote:
>>> This small patchset fixes a somewhat minor issue found in the clocksource
>>> driver for Armada 370/XP SoC.
>>>
>>> On one side the Armada 370 SoC has no 25 MHz fixed timer.
>>> On the other side the Armada XP SoC cannot work properly without such 25 MHz
>>> fixed timer selected, because otherwise the base clock frequency would vary
>>> when doing cpufreq frequency changes.
>>>
>>> Therefore we can consider the SoCs as not being compatible, being better to
>>> have two compatible strings, one for each SoC. The previous compatible and
>>> its behavior has been removed, considering there are no DT-enabled boards
>>> in use in the field.
>>>
>>> In addition, CLOCKSOURCE_OF_DECLARE is used to simplify the initialization.
>>>
>>> This patchset is based on tip/timers/core.
>>>
>>
>> Applied to my tree as 3.12 material.
> 
> Did you take patch #6 as well?

Yes. Not good ?


-- 
 <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

[PATCH v4 0/6] Armada 370/XP clocksource fixes

[Jason Cooper]

On Tue, Aug 13, 2013 at 07:48:40PM +0200, Daniel Lezcano wrote:
> On 08/13/2013 06:52 PM, Jason Cooper wrote:
> > Daniel,
> > 
> > On Tue, Aug 13, 2013 at 06:22:48PM +0200, Daniel Lezcano wrote:
> >> On 08/13/2013 04:43 PM, Ezequiel Garcia wrote:
> >>> This small patchset fixes a somewhat minor issue found in the clocksource
> >>> driver for Armada 370/XP SoC.
> >>>
> >>> On one side the Armada 370 SoC has no 25 MHz fixed timer.
> >>> On the other side the Armada XP SoC cannot work properly without such 25 MHz
> >>> fixed timer selected, because otherwise the base clock frequency would vary
> >>> when doing cpufreq frequency changes.
> >>>
> >>> Therefore we can consider the SoCs as not being compatible, being better to
> >>> have two compatible strings, one for each SoC. The previous compatible and
> >>> its behavior has been removed, considering there are no DT-enabled boards
> >>> in use in the field.
> >>>
> >>> In addition, CLOCKSOURCE_OF_DECLARE is used to simplify the initialization.
> >>>
> >>> This patchset is based on tip/timers/core.
> >>>
> >>
> >> Applied to my tree as 3.12 material.
> > 
> > Did you take patch #6 as well?
> 
> Yes. Not good ?

I usually try to keep all the *.dts? changes in one tree so I can
resolve the conflicts internally because our dts files have been subject
to large changes (adding pcie/mbus, relocating nodes under the proper
parents, etc).

By way of example:

$ git diff --stat v3.11-rc1..for-next -- arch/arm/boot/dts/armada-*
 arch/arm/boot/dts/armada-370-db.dts              |   5 +-
 arch/arm/boot/dts/armada-370-mirabox.dts         |  37 +-
 arch/arm/boot/dts/armada-370-rd.dts              |  37 +-
 arch/arm/boot/dts/armada-370-xp.dtsi             | 114 ++++---
 arch/arm/boot/dts/armada-370.dtsi                | 127 ++++---
 arch/arm/boot/dts/armada-xp-db.dts               | 131 ++++---
 arch/arm/boot/dts/armada-xp-gp.dts               | 107 +++---
 arch/arm/boot/dts/armada-xp-mv78230.dtsi         | 229 +++++++------
 arch/arm/boot/dts/armada-xp-mv78260.dtsi         | 270 ++++++++-------
 arch/arm/boot/dts/armada-xp-mv78460.dtsi         | 418 ++++++++++++-----------
 arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts |  88 +++--
 arch/arm/boot/dts/armada-xp.dtsi                 |  19 +-
 12 files changed, 849 insertions(+), 733 deletions(-)

I would prefer not to subject arm-soc or Linus to resolving those when
they are best resolved at our level.

Is it possible for you to drop that one so we can take it?

thx,

Jason.

[PATCH v4 0/6] Armada 370/XP clocksource fixes

[Daniel Lezcano]

On 08/13/2013 07:58 PM, Jason Cooper wrote:
> On Tue, Aug 13, 2013 at 07:48:40PM +0200, Daniel Lezcano wrote:
>> On 08/13/2013 06:52 PM, Jason Cooper wrote:
>>> Daniel,
>>>
>>> On Tue, Aug 13, 2013 at 06:22:48PM +0200, Daniel Lezcano wrote:
>>>> On 08/13/2013 04:43 PM, Ezequiel Garcia wrote:
>>>>> This small patchset fixes a somewhat minor issue found in the clocksource
>>>>> driver for Armada 370/XP SoC.
>>>>>
>>>>> On one side the Armada 370 SoC has no 25 MHz fixed timer.
>>>>> On the other side the Armada XP SoC cannot work properly without such 25 MHz
>>>>> fixed timer selected, because otherwise the base clock frequency would vary
>>>>> when doing cpufreq frequency changes.
>>>>>
>>>>> Therefore we can consider the SoCs as not being compatible, being better to
>>>>> have two compatible strings, one for each SoC. The previous compatible and
>>>>> its behavior has been removed, considering there are no DT-enabled boards
>>>>> in use in the field.
>>>>>
>>>>> In addition, CLOCKSOURCE_OF_DECLARE is used to simplify the initialization.
>>>>>
>>>>> This patchset is based on tip/timers/core.
>>>>>
>>>>
>>>> Applied to my tree as 3.12 material.
>>>
>>> Did you take patch #6 as well?
>>
>> Yes. Not good ?
> 
> I usually try to keep all the *.dts? changes in one tree so I can
> resolve the conflicts internally because our dts files have been subject
> to large changes (adding pcie/mbus, relocating nodes under the proper
> parents, etc).
> 
> By way of example:
> 
> $ git diff --stat v3.11-rc1..for-next -- arch/arm/boot/dts/armada-*
>  arch/arm/boot/dts/armada-370-db.dts              |   5 +-
>  arch/arm/boot/dts/armada-370-mirabox.dts         |  37 +-
>  arch/arm/boot/dts/armada-370-rd.dts              |  37 +-
>  arch/arm/boot/dts/armada-370-xp.dtsi             | 114 ++++---
>  arch/arm/boot/dts/armada-370.dtsi                | 127 ++++---
>  arch/arm/boot/dts/armada-xp-db.dts               | 131 ++++---
>  arch/arm/boot/dts/armada-xp-gp.dts               | 107 +++---
>  arch/arm/boot/dts/armada-xp-mv78230.dtsi         | 229 +++++++------
>  arch/arm/boot/dts/armada-xp-mv78260.dtsi         | 270 ++++++++-------
>  arch/arm/boot/dts/armada-xp-mv78460.dtsi         | 418 ++++++++++++-----------
>  arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts |  88 +++--
>  arch/arm/boot/dts/armada-xp.dtsi                 |  19 +-
>  12 files changed, 849 insertions(+), 733 deletions(-)
> 
> I would prefer not to subject arm-soc or Linus to resolving those when
> they are best resolved at our level.
> 
> Is it possible for you to drop that one so we can take it?

Sure, no problem. Dropped.

Thanks
  -- Daniel


-- 
 <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

[PATCH v4 0/6] Armada 370/XP clocksource fixes

[Jason Cooper]

On Tue, Aug 13, 2013 at 08:04:40PM +0200, Daniel Lezcano wrote:
> On 08/13/2013 07:58 PM, Jason Cooper wrote:
> > On Tue, Aug 13, 2013 at 07:48:40PM +0200, Daniel Lezcano wrote:
> >> On 08/13/2013 06:52 PM, Jason Cooper wrote:
> >>> Daniel,
> >>>
> >>> On Tue, Aug 13, 2013 at 06:22:48PM +0200, Daniel Lezcano wrote:
> >>>> On 08/13/2013 04:43 PM, Ezequiel Garcia wrote:
> >>>>> This small patchset fixes a somewhat minor issue found in the clocksource
> >>>>> driver for Armada 370/XP SoC.
> >>>>>
> >>>>> On one side the Armada 370 SoC has no 25 MHz fixed timer.
> >>>>> On the other side the Armada XP SoC cannot work properly without such 25 MHz
> >>>>> fixed timer selected, because otherwise the base clock frequency would vary
> >>>>> when doing cpufreq frequency changes.
> >>>>>
> >>>>> Therefore we can consider the SoCs as not being compatible, being better to
> >>>>> have two compatible strings, one for each SoC. The previous compatible and
> >>>>> its behavior has been removed, considering there are no DT-enabled boards
> >>>>> in use in the field.
> >>>>>
> >>>>> In addition, CLOCKSOURCE_OF_DECLARE is used to simplify the initialization.
> >>>>>
> >>>>> This patchset is based on tip/timers/core.
> >>>>>
> >>>>
> >>>> Applied to my tree as 3.12 material.
> >>>
> >>> Did you take patch #6 as well?
> >>
> >> Yes. Not good ?
> > 
> > I usually try to keep all the *.dts? changes in one tree so I can
> > resolve the conflicts internally because our dts files have been subject
> > to large changes (adding pcie/mbus, relocating nodes under the proper
> > parents, etc).
> > 
> > By way of example:
> > 
> > $ git diff --stat v3.11-rc1..for-next -- arch/arm/boot/dts/armada-*
> >  arch/arm/boot/dts/armada-370-db.dts              |   5 +-
> >  arch/arm/boot/dts/armada-370-mirabox.dts         |  37 +-
> >  arch/arm/boot/dts/armada-370-rd.dts              |  37 +-
> >  arch/arm/boot/dts/armada-370-xp.dtsi             | 114 ++++---
> >  arch/arm/boot/dts/armada-370.dtsi                | 127 ++++---
> >  arch/arm/boot/dts/armada-xp-db.dts               | 131 ++++---
> >  arch/arm/boot/dts/armada-xp-gp.dts               | 107 +++---
> >  arch/arm/boot/dts/armada-xp-mv78230.dtsi         | 229 +++++++------
> >  arch/arm/boot/dts/armada-xp-mv78260.dtsi         | 270 ++++++++-------
> >  arch/arm/boot/dts/armada-xp-mv78460.dtsi         | 418 ++++++++++++-----------
> >  arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts |  88 +++--
> >  arch/arm/boot/dts/armada-xp.dtsi                 |  19 +-
> >  12 files changed, 849 insertions(+), 733 deletions(-)
> > 
> > I would prefer not to subject arm-soc or Linus to resolving those when
> > they are best resolved at our level.
> > 
> > Is it possible for you to drop that one so we can take it?
> 
> Sure, no problem. Dropped.

Great!  Thanks Daniel.

thx,

Jason.

[PATCH v4 0/6] Armada 370/XP clocksource fixes

[Ezequiel Garcia]

On Tue, Aug 13, 2013 at 06:22:48PM +0200, Daniel Lezcano wrote:
> On 08/13/2013 04:43 PM, Ezequiel Garcia wrote:
> > This small patchset fixes a somewhat minor issue found in the clocksource
> > driver for Armada 370/XP SoC.
> > 
> > On one side the Armada 370 SoC has no 25 MHz fixed timer.
> > On the other side the Armada XP SoC cannot work properly without such 25 MHz
> > fixed timer selected, because otherwise the base clock frequency would vary
> > when doing cpufreq frequency changes.
> > 
> > Therefore we can consider the SoCs as not being compatible, being better to
> > have two compatible strings, one for each SoC. The previous compatible and
> > its behavior has been removed, considering there are no DT-enabled boards
> > in use in the field.
> > 
> > In addition, CLOCKSOURCE_OF_DECLARE is used to simplify the initialization.
> > 
> > This patchset is based on tip/timers/core.
> > 
> 
> Applied to my tree as 3.12 material.
> 

Just to check: on which tree/branch is this applied?

Thanks!
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com