Armada 370/XP clocksource fixes

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 <[email protected]>

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/clocksource/time-armada-370-xp.c | 70 ++++++++++++++------------------
 1 file changed, 31 insertions(+), 39 deletions(-)

drivers/clocksource/time-armada-370-xp.c +2 -0

index 5ee4329..a2351ac 100644
@@ -71,6 +71,18 @@ static u32 ticks_per_jiffy;
 
 static struct clock_event_device __percpu **percpu_armada_370_xp_evt;
 
+void timer_ctrl_clrset(u32 clr, u32 set)
+{

Quoted 21 lines · 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 <[email protected]>

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/clocksource/time-armada-370-xp.c | 70 ++++++++++++++------------------
 1 file changed, 31 insertions(+), 39 deletions(-)

drivers/clocksource/time-armada-370-xp.c +2 -0

index 5ee4329..a2351ac 100644
@@ -71,6 +71,18 @@ static u32 ticks_per_jiffy;
 
 static struct clock_event_device __percpu **percpu_armada_370_xp_evt;
 
+void timer_ctrl_clrset(u32 clr, u32 set)
+{

drivers/clocksource/time-armada-370-xp.c static u32 ticks_per_jiffy; · line 85

Quoted 10 lines

drivers/clocksource/time-armada-370-xp.c +10 -0

+       writel((readl(timer_base + TIMER_CTRL_OFF) & ~clr) | set,
+               timer_base + TIMER_CTRL_OFF);
+}
+
+void local_timer_ctrl_clrset(u32 clr, u32 set)
+{
+       writel((readl(local_base + TIMER_CTRL_OFF) & ~clr) | set,
+               local_base + TIMER_CTRL_OFF);
+}
+

Ezequiel

I guess the watchdog will only require one of these two functions above, and probably not the local_timer function. So maybe make it static? Also, do you get sparse warnings from timer_ctrl_clrset() since it is not static, but also not declared in a header file somewhere?

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 <[email protected]>

---
 drivers/clocksource/time-armada-370-xp.c | 59 ++++++++++++++++++++++++--------
 1 file changed, 45 insertions(+), 14 deletions(-)

drivers/clocksource/time-armada-370-xp.c +12 -14

index a2351ac..1458a85 100644
@@ -212,7 +212,7 @@ static struct local_timer_ops armada_370_xp_local_timer_ops = {
 	.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,21 +221,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;
-	}
 	local_timer_ctrl_clrset(clr, set);
 	timer_ctrl_clrset(clr, set);
 

@@ -289,5 +278,47 @@ static void __init armada_370_xp_timer_init(struct device_node *np)
 #endif
 	}
 }
+
+static void __init armada_370_xp_timer_init(struct device_node *np)
+{
+	if (of_find_property(np, "marvell,timer-25Mhz", NULL)) {
+		/* The fixed 25MHz timer is required */
+		timer_clk = 25000000;
+	} else {
+		unsigned long rate = 0;
+		struct clk *clk = of_clk_get(np, 0);

Quoted 61 lines · 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 <[email protected]>

---
 drivers/clocksource/time-armada-370-xp.c | 59 ++++++++++++++++++++++++--------
 1 file changed, 45 insertions(+), 14 deletions(-)

drivers/clocksource/time-armada-370-xp.c +12 -14

index a2351ac..1458a85 100644
@@ -212,7 +212,7 @@ static struct local_timer_ops armada_370_xp_local_timer_ops = {
 	.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,21 +221,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;
-	}
 	local_timer_ctrl_clrset(clr, set);
 	timer_ctrl_clrset(clr, set);
 

@@ -289,5 +278,47 @@ static void __init armada_370_xp_timer_init(struct device_node *np)
 #endif
 	}
 }
+
+static void __init armada_370_xp_timer_init(struct device_node *np)
+{
+	if (of_find_property(np, "marvell,timer-25Mhz", NULL)) {
+		/* The fixed 25MHz timer is required */
+		timer_clk = 25000000;
+	} else {
+		unsigned long rate = 0;
+		struct clk *clk = of_clk_get(np, 0);

drivers/clocksource/time-armada-370-xp.c static void __init armada_370_xp_timer_init(struct device_node *np) · line 299

Quoted 10 lines

drivers/clocksource/time-armada-370-xp.c +8 -0

+		WARN_ON(IS_ERR(clk));
+		rate =  clk_get_rate(clk);
+		timer_clk = rate / TIMER_DIVIDER;
+		timer25Mhz = false;
+	}
+
+	armada_370_xp_timer_common_init(np);
+}
 CLOCKSOURCE_OF_DECLARE(armada_370_xp, "marvell,armada-370-xp-timer",
 		       armada_370_xp_timer_init);

Hi Ezequiel

I think it would be good to have a comment saying this compatibility string, and the marvell,timer-25Mhz is depreciated, and a short explanation why. Maybe sometime in the future we can even remove it, depending on the outcome of the DT stability discussions.

This commit fixes the DT binding for the Armada 370/XP SoC timer. The old "marvell,armada-370-xp-timer" compatible is marked deprecated and new compatible strings: "marvell,armada-xp-timer" and "marvell,armada-370-timer" are added instead.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 .../bindings/timer/marvell,armada-370-xp-timer.txt | 29 +++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

Documentation/devicetree/bindings/timer/marvell,armada-370-xp-timer.txt +26 -3

index 3638112..d6aeb5b 100644
@@ -2,7 +2,9 @@ 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.
+  The older "marvell,armada-370-xp-timer" is DEPRECATED and shouldn't be used.
 - interrupts: Should contain the list of Global Timer interrupts and
   then local timer interrupts
 - reg: Should contain location and length for timers register. First

@@ -11,5 +13,26 @@ Required properties:
 - clocks: clock driving the timer hardware
 
 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)
+- marvell,timer-25Mhz [DEPRECATED]:
+  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>;
+		clocks = <&coreclk 2>;
+	};

Signature · 1 line

1.8.1.5

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

The Armada 370 SoC has no 25 MHz fixed timer, while the Armada XP SoC cannot work properly without such 25 MHz fixed timer selected.

Quoted 3 lines · Ezequiel Garcia

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

Quoted 2 lines

The Armada 370 SoC has no 25 MHz fixed timer, while the Armada XP SoC cannot work properly without such 25 MHz fixed timer selected.

Hi Ezequiel

Is the XP case a silicon bug, or a design feature? If its a silicon bug, is it likely to be fixed in later steppings of the SOCs? If this feature is to come back, might we want to keep the property?