[PATCH 0/5] Armada 370/XP clocksource fixes

[PATCH 0/5] 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.

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.

Therefore, it makes no sense to have a DT property to select the 25 MHz fixed
timer, and instead it's better to declare two different compatible strings:
one for each SoC.
The previous compatible and its behavior has been maintained to preserve
backwards compatibility.

In addition, CLOCKSOURCE_OF_DECLARE is used to simplify the initialization.

Any testing and feedback is highly appreciated!

Ezequiel Garcia (5):
  clocksource: armada-370-xp: Use CLOCKSOURCE_OF_DECLARE
  clocksource: armada-370-xp: Simplify TIMER_CTRL register access
  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 |  29 ++++-
 arch/arm/boot/dts/armada-370-xp.dtsi               |   1 -
 arch/arm/boot/dts/armada-370.dtsi                  |   4 +
 arch/arm/boot/dts/armada-xp.dtsi                   |   2 +-
 arch/arm/mach-mvebu/armada-370-xp.c                |   4 +-
 drivers/clocksource/time-armada-370-xp.c           | 129 ++++++++++++---------
 include/linux/time-armada-370-xp.h                 |  18 ---
 7 files changed, 109 insertions(+), 78 deletions(-)
 delete mode 100644 include/linux/time-armada-370-xp.h

-- 
1.8.1.5

[PATCH 1/5] 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 1b04b7e..5ee4329 100644
--- a/drivers/clocksource/time-armada-370-xp.c
+++ b/drivers/clocksource/time-armada-370-xp.c
@@ -210,13 +210,11 @@ static struct local_timer_ops armada_370_xp_local_timer_ops = {
 	.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 u;
-	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);
@@ -299,3 +297,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 2/5] 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 | 70 ++++++++++++++------------------
 1 file changed, 31 insertions(+), 39 deletions(-)

diff --git a/drivers/clocksource/time-armada-370-xp.c b/drivers/clocksource/time-armada-370-xp.c
index 5ee4329..a2351ac 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;
 
+void timer_ctrl_clrset(u32 clr, u32 set)
+{
+       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);
+}
+
 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 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 = {
 
 static void __init armada_370_xp_timer_init(struct device_node *np)
 {
-	u32 u;
+	u32 clr = 0, set = 0;
 	int res;
 
 	timer_base = of_iomap(np, 0);
@@ -220,30 +222,22 @@ static void __init armada_370_xp_timer_init(struct device_node *np)
 	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 */
-		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;
 	}
+	local_timer_ctrl_clrset(clr, set);
+	timer_ctrl_clrset(clr, set);
 
 	/*
 	 * We use timer 0 as clocksource, and private(local) timer 0
@@ -265,10 +259,8 @@ static void __init armada_370_xp_timer_init(struct device_node *np)
 	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 2/5] clocksource: armada-370-xp: Simplify TIMER_CTRL register access

[Andrew Lunn]

On Wed, Aug 07, 2013 at 08:52:33PM -0300, Ezequiel Garcia wrote:
> 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 | 70 ++++++++++++++------------------
>  1 file changed, 31 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/clocksource/time-armada-370-xp.c b/drivers/clocksource/time-armada-370-xp.c
> index 5ee4329..a2351ac 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;
>  
> +void timer_ctrl_clrset(u32 clr, u32 set)
> +{
> +       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?

	Andrew

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

[Ezequiel Garcia]

On Thu, Aug 08, 2013 at 09:20:16AM +0200, Andrew Lunn wrote:
> On Wed, Aug 07, 2013 at 08:52:33PM -0300, Ezequiel Garcia wrote:
> > 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 | 70 ++++++++++++++------------------
> >  1 file changed, 31 insertions(+), 39 deletions(-)
> > 
> > diff --git a/drivers/clocksource/time-armada-370-xp.c b/drivers/clocksource/time-armada-370-xp.c
> > index 5ee4329..a2351ac 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;
> >  
> > +void timer_ctrl_clrset(u32 clr, u32 set)
> > +{
> > +       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?
> 

Nice catch! Both should be static for now, we can export/declare
the proper one in the proper time.

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

[PATCH 3/5] 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 | 59 ++++++++++++++++++++++++--------
 1 file changed, 45 insertions(+), 14 deletions(-)

diff --git a/drivers/clocksource/time-armada-370-xp.c b/drivers/clocksource/time-armada-370-xp.c
index a2351ac..1458a85 100644
--- a/drivers/clocksource/time-armada-370-xp.c
+++ b/drivers/clocksource/time-armada-370-xp.c
@@ -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);
+		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);
+
+static void __init armada_xp_timer_init(struct device_node *np)
+{
+	/* The fixed 25MHz timer is required */
+	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)
+{
+	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;
+	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 3/5] clocksource: armada-370-xp: Introduce new compatibles

[Andrew Lunn]

On Wed, Aug 07, 2013 at 08:52:34PM -0300, Ezequiel Garcia wrote:
> 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 | 59 ++++++++++++++++++++++++--------
>  1 file changed, 45 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/clocksource/time-armada-370-xp.c b/drivers/clocksource/time-armada-370-xp.c
> index a2351ac..1458a85 100644
> --- a/drivers/clocksource/time-armada-370-xp.c
> +++ b/drivers/clocksource/time-armada-370-xp.c
> @@ -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);
> +		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.

	  Andrew

[PATCH 3/5] clocksource: armada-370-xp: Introduce new compatibles

[Ezequiel Garcia]

On Thu, Aug 08, 2013 at 09:53:56AM +0200, Andrew Lunn wrote:
> On Wed, Aug 07, 2013 at 08:52:34PM -0300, Ezequiel Garcia wrote:
> > 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 | 59 ++++++++++++++++++++++++--------
> >  1 file changed, 45 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/clocksource/time-armada-370-xp.c b/drivers/clocksource/time-armada-370-xp.c
> > index a2351ac..1458a85 100644
> > --- a/drivers/clocksource/time-armada-370-xp.c
> > +++ b/drivers/clocksource/time-armada-370-xp.c
> > @@ -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);
> > +		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.
> 

Sure, I'll do that.

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

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

[Ezequiel Garcia]

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 <ezequiel.garcia@free-electrons.com>
---
 .../bindings/timer/marvell,armada-370-xp-timer.txt | 29 +++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 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..d6aeb5b 100644
--- a/Documentation/devicetree/bindings/timer/marvell,armada-370-xp-timer.txt
+++ b/Documentation/devicetree/bindings/timer/marvell,armada-370-xp-timer.txt
@@ -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>;
+	};
-- 
1.8.1.5

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

[Jason Cooper]

+ devicetree ml

On Wed, Aug 07, 2013 at 08:52:35PM -0300, Ezequiel Garcia wrote:
> 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 <ezequiel.garcia@free-electrons.com>
> ---
>  .../bindings/timer/marvell,armada-370-xp-timer.txt | 29 +++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 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..d6aeb5b 100644
> --- a/Documentation/devicetree/bindings/timer/marvell,armada-370-xp-timer.txt
> +++ b/Documentation/devicetree/bindings/timer/marvell,armada-370-xp-timer.txt
> @@ -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>;
> +	};
> -- 
> 1.8.1.5
> 

[PATCH 5/5] 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.

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

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 90b1176..3ed5de4 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -81,7 +81,6 @@
 			};
 
 			timer at 20300 {
-				compatible = "marvell,armada-370-xp-timer";
 				reg = <0x20300 0x30>, <0x21040 0x30>;
 				interrupts = <37>, <38>, <39>, <40>, <5>, <6>;
 				clocks = <&coreclk 2>;
diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index fa3dfc6..f7b9fc6 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -104,6 +104,10 @@
 				interrupts = <91>;
 			};
 
+			timer at 20300 {
+				compatible = "marvell,armada-370-timer";
+			};
+
 			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 0/5] Armada 370/XP clocksource fixes

[Andrew Lunn]

On Wed, Aug 07, 2013 at 08:52:31PM -0300, Ezequiel Garcia wrote:
> 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.

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?

	Andrew

[PATCH 0/5] Armada 370/XP clocksource fixes

[Thomas Petazzoni]

Dear Andrew Lunn,

On Thu, 8 Aug 2013 09:12:06 +0200, Andrew Lunn wrote:
> On Wed, Aug 07, 2013 at 08:52:31PM -0300, Ezequiel Garcia wrote:
> > 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.
> 
> 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?

My understanding is that it's a design feature. Using the non-25 Mhz
timer leads to using a clocksource whose frequency varies when doing
cpufreq frequency changes, which isn't nice to handle. The fixed 25
Mhz was added in Armada XP to solve this problem, as far as I
understood.

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com