arm: mvebu: add support for local timer for Armada 370/XP

Dear Gregory CLEMENT,

Just some minor nitpicks below.

Review

Quoted 3 lines

(quoted diff) +2 -0

+	if (hw < ARMADA_370_XP_MAX_PER_CPU_IRQS) {
+

Unneeded empty line.

arch/arm/mach-mvebu/irq-armada-370-xp.c static int armada_370_xp_mpic_irq_map(struct irq_domain *h, · line 159

Quoted 8 lines

arch/arm/mach-mvebu/irq-armada-370-xp.c +8 -0

+		irq_set_percpu_devid(virq);
+		irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip,
+					handle_percpu_devid_irq);
+
+	} else {
+		irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip,
+					handle_level_irq);
+	}

Braces useless since there is only one statement in the else.

arch/arm/mach-mvebu/irq-armada-370-xp.c static int armada_370_xp_mpic_irq_map(struct irq_domain *h, · line 160

Quoted 1 line

arch/arm/mach-mvebu/irq-armada-370-xp.c +1 -0

+		set_irq_flags(virq, IRQF_VALID | IRQF_PROBE);

Incorrect indentation for this line.

Thomas

Quoted 11 lines · Thomas Petazzoni

arch/arm/mach-mvebu/irq-armada-370-xp.c static int armada_370_xp_mpic_irq_map(struct irq_domain *h, · line 159

Quoted 8 lines

arch/arm/mach-mvebu/irq-armada-370-xp.c +8 -0

+		irq_set_percpu_devid(virq);
+		irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip,
+					handle_percpu_devid_irq);
+
+	} else {
+		irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip,
+					handle_level_irq);
+	}

Braces useless since there is only one statement in the else.

Hi Thomas and Gregory,

Quoted 12 lines · Gregory CLEMENT

Quoted 11 lines · Thomas Petazzoni

arch/arm/mach-mvebu/irq-armada-370-xp.c static int armada_370_xp_mpic_irq_map(struct irq_domain *h, · line 159

Quoted 8 lines

arch/arm/mach-mvebu/irq-armada-370-xp.c +8 -0

+		irq_set_percpu_devid(virq);
+		irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip,
+					handle_percpu_devid_irq);
+
+	} else {
+		irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip,
+					handle_level_irq);
+	}

Braces useless since there is only one statement in the else.

IMHO, this is an exception to the rule.
Since the first block is more than one line,
we usually put braces on the single line block too. (or at least that's what Documentation/CodingStyle says).

Regards,

You're right!
I would have liked to say I have done it on purpose, but in fact these braces are here only because during development I had had multiples lines inside the else.
However, as you pointed it, I will keep it.

drivers/clocksource/time-armada-370-xp.c line 73

Quoted 2 lines · Gregory CLEMENT

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

+struct clock_event_device __percpu **percpu_armada_370_xp_evt;
+

Have you run this through checkpatch? Should the above be static?

On the SOCs Armada 370 and Armada XP, each CPU comes with two private timers. This patch use the timer 0 of each CPU as local timer for the clockevent if CONFIG_LOCAL_TIMER is selected. In the other case, use only the private Timer 0 of CPU 0.

Signed-off-by Gregory CLEMENT <[email protected]>

---
 drivers/clocksource/time-armada-370-xp.c |  150 ++++++++++++++++++++++--------
 1 file changed, 112 insertions(+), 38 deletions(-)

drivers/clocksource/time-armada-370-xp.c +112 -38

index a4605fd..757f034 100644
@@ -27,8 +27,10 @@
 #include <linux/of_address.h>
 #include <linux/irq.h>
 #include <linux/module.h>
-#include <asm/sched_clock.h>
 
+#include <asm/sched_clock.h>
+#include <asm/localtimer.h>
+#include <linux/percpu.h>
 /*
  * Timer block registers.
  */

@@ -49,6 +51,7 @@
 #define TIMER1_RELOAD_OFF	0x0018
 #define TIMER1_VAL_OFF		0x001c
 
+#define LCL_TIMER_EVENTS_STATUS	0x0028
 /* Global timers are connected to the coherency fabric clock, and the
    below divider reduces their incrementing frequency. */
 #define TIMER_DIVIDER_SHIFT     5

@@ -57,14 +60,17 @@
 /*
  * SoC-specific data.
  */
-static void __iomem *timer_base;
-static int timer_irq;
+static void __iomem *timer_base, *local_base;
+static unsigned int timer_clk;
+static bool timer25Mhz = true;
 
 /*
  * Number of timer ticks per jiffy.
  */
 static u32 ticks_per_jiffy;
 
+struct clock_event_device __percpu **percpu_armada_370_xp_evt;
+
 static u32 notrace armada_370_xp_read_sched_clock(void)
 {
 	return ~readl(timer_base + TIMER0_VAL_OFF);

@@ -78,24 +84,23 @@ armada_370_xp_clkevt_next_event(unsigned long delta,
 				struct clock_event_device *dev)
 {
 	u32 u;
-
 	/*
 	 * Clear clockevent timer interrupt.
 	 */
-	writel(TIMER1_CLR_MASK, timer_base + TIMER_EVENTS_STATUS);
+	writel(TIMER0_CLR_MASK, local_base + LCL_TIMER_EVENTS_STATUS);
 
 	/*
 	 * Setup new clockevent timer value.
 	 */
-	writel(delta, timer_base + TIMER1_VAL_OFF);
+	writel(delta, local_base + TIMER0_VAL_OFF);
 
 	/*
 	 * Enable the timer.
 	 */
-	u = readl(timer_base + TIMER_CTRL_OFF);
-	u = ((u & ~TIMER1_RELOAD_EN) | TIMER1_EN |
-	     TIMER1_DIV(TIMER_DIVIDER_SHIFT));
-	writel(u, timer_base + TIMER_CTRL_OFF);
+	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);
 
 	return 0;
 }

@@ -107,37 +112,38 @@ armada_370_xp_clkevt_mode(enum clock_event_mode mode,
 	u32 u;
 
 	if (mode == CLOCK_EVT_MODE_PERIODIC) {
+
 		/*
 		 * Setup timer to fire at 1/HZ intervals.
 		 */
-		writel(ticks_per_jiffy - 1, timer_base + TIMER1_RELOAD_OFF);
-		writel(ticks_per_jiffy - 1, timer_base + TIMER1_VAL_OFF);
+		writel(ticks_per_jiffy - 1, local_base + TIMER0_RELOAD_OFF);
+		writel(ticks_per_jiffy - 1, local_base + TIMER0_VAL_OFF);
 
 		/*
 		 * Enable timer.
 		 */
-		u = readl(timer_base + TIMER_CTRL_OFF);
 
-		writel((u | TIMER1_EN | TIMER1_RELOAD_EN |
-			TIMER1_DIV(TIMER_DIVIDER_SHIFT)),
-		       timer_base + TIMER_CTRL_OFF);
+		u = readl(local_base + TIMER_CTRL_OFF);
+
+		writel((u | TIMER0_EN | TIMER0_RELOAD_EN |
+			TIMER0_DIV(TIMER_DIVIDER_SHIFT)),
+			local_base + TIMER_CTRL_OFF);
 	} else {
 		/*
 		 * Disable timer.
 		 */
-		u = readl(timer_base + TIMER_CTRL_OFF);
-		writel(u & ~TIMER1_EN, timer_base + TIMER_CTRL_OFF);
+		u = readl(local_base + TIMER_CTRL_OFF);
+		writel(u & ~TIMER0_EN, local_base + TIMER_CTRL_OFF);
 
 		/*
 		 * ACK pending timer interrupt.
 		 */
-		writel(TIMER1_CLR_MASK, timer_base + TIMER_EVENTS_STATUS);
-
+		writel(TIMER0_CLR_MASK, local_base + LCL_TIMER_EVENTS_STATUS);
 	}
 }
 
 static struct clock_event_device armada_370_xp_clkevt = {
-	.name		= "armada_370_xp_tick",
+	.name		= "armada_370_xp_per_cpu_tick",
 	.features	= CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC,
 	.shift		= 32,
 	.rating		= 300,

@@ -150,32 +156,78 @@ static irqreturn_t armada_370_xp_timer_interrupt(int irq, void *dev_id)
 	/*
 	 * ACK timer interrupt and call event handler.
 	 */
+	struct clock_event_device *evt = *(struct clock_event_device **)dev_id;
 
-	writel(TIMER1_CLR_MASK, timer_base + TIMER_EVENTS_STATUS);
-	armada_370_xp_clkevt.event_handler(&armada_370_xp_clkevt);
+	writel(TIMER0_CLR_MASK, local_base + LCL_TIMER_EVENTS_STATUS);
+	evt->event_handler(evt);
 
 	return IRQ_HANDLED;
 }
 
-static struct irqaction armada_370_xp_timer_irq = {
-	.name		= "armada_370_xp_tick",
-	.flags		= IRQF_DISABLED | IRQF_TIMER,
-	.handler	= armada_370_xp_timer_interrupt
+/*
+ * Setup the local clock events for a CPU.
+ */
+static int __cpuinit armada_370_xp_timer_setup(struct clock_event_device *evt)
+{
+	u32 u;
+	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);
+	else
+		writel(u & ~TIMER0_25MHZ, local_base + TIMER_CTRL_OFF);
+
+	evt->name		= armada_370_xp_clkevt.name;
+	evt->irq		= armada_370_xp_clkevt.irq;
+	evt->features		= armada_370_xp_clkevt.features;
+	evt->shift		= armada_370_xp_clkevt.shift;
+	evt->rating		= armada_370_xp_clkevt.rating,
+	evt->set_next_event	= armada_370_xp_clkevt_next_event,
+	evt->set_mode		= armada_370_xp_clkevt_mode,
+	evt->cpumask		= cpumask_of(cpu);
+
+	*__this_cpu_ptr(percpu_armada_370_xp_evt) = evt;
+
+	clockevents_config_and_register(evt, timer_clk, 1, 0xfffffffe);
+	enable_percpu_irq(evt->irq, 0);
+
+	return 0;
+}
+
+static void  armada_370_xp_timer_stop(struct clock_event_device *evt)
+{
+	evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt);
+	disable_percpu_irq(evt->irq);
+}
+
+static struct local_timer_ops armada_370_xp_local_timer_ops __cpuinitdata = {
+	.setup	= armada_370_xp_timer_setup,
+	.stop	=  armada_370_xp_timer_stop,
 };
 
 void __init armada_370_xp_timer_init(void)
 {
 	u32 u;
 	struct device_node *np;
-	unsigned int timer_clk;
+	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);
 
 	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 | TIMER1_25MHZ,
+		writel(u | TIMER0_25MHZ,
 		       timer_base + TIMER_CTRL_OFF);
 		timer_clk = 25000000;
 	} else {

@@ -183,15 +235,23 @@ void __init armada_370_xp_timer_init(void)
 		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 | TIMER1_25MHZ),
+		writel(u & ~(TIMER0_25MHZ),
 		       timer_base + TIMER_CTRL_OFF);
+
 		timer_clk = rate / TIMER_DIVIDER;
+		timer25Mhz = false;
 	}
 
-	/* We use timer 0 as clocksource, and timer 1 for
-	   clockevents */
-	timer_irq = irq_of_parse_and_map(np, 1);
+	/*
+	 * We use timer 0 as clocksource, and private(local) timer 0
+	 * for clockevents
+	 */
+	armada_370_xp_clkevt.irq = irq_of_parse_and_map(np, 4);
 
 	ticks_per_jiffy = (timer_clk + HZ / 2) / HZ;
 

@@ -216,12 +276,26 @@ void __init armada_370_xp_timer_init(void)
 			      "armada_370_xp_clocksource",
 			      timer_clk, 300, 32, clocksource_mmio_readl_down);
 
-	/*
-	 * Setup clockevent timer (interrupt-driven).
-	 */
-	setup_irq(timer_irq, &armada_370_xp_timer_irq);
+	/* Register the clockevent on the private timer of CPU 0 */
 	armada_370_xp_clkevt.cpumask = cpumask_of(0);
 	clockevents_config_and_register(&armada_370_xp_clkevt,
 					timer_clk, 1, 0xfffffffe);
-}
 
+	percpu_armada_370_xp_evt = alloc_percpu(struct clock_event_device *);
+
+
+	/*
+	 * Setup clockevent timer (interrupt-driven).
+	 */
+	*__this_cpu_ptr(percpu_armada_370_xp_evt) = &armada_370_xp_clkevt;
+	res = request_percpu_irq(armada_370_xp_clkevt.irq,
+				armada_370_xp_timer_interrupt,
+				armada_370_xp_clkevt.name,
+				percpu_armada_370_xp_evt);
+	if (!res) {
+		enable_percpu_irq(armada_370_xp_clkevt.irq, 0);
+#ifdef CONFIG_LOCAL_TIMERS
+		local_timer_register(&armada_370_xp_local_timer_ops);
+#endif
+	}
+}

arch/arm/Kconfig config LOCAL_TIMERS · line 1628

Quoted 7 lines · Gregory CLEMENT

arch/arm/Kconfig +1 -1

@@ -1624,7 +1624,7 @@ config LOCAL_TIMERS
        bool "Use local timer interrupts"
        depends on SMP
        default y
-       select HAVE_ARM_TWD if (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT)
+       select HAVE_ARM_TWD if (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT && !ARMADA_370_XP_TIMER)
 

Maybe it can be written as

config LOCAL_TIMERS
bool "Use local timer interrupts"
depends on SMP
default y

config HAVE_ARM_TWD
depends on LOCAL_TIMERS
default ARCH_MULTIPLATFORM || (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT) default y

This will still be slightly wrong (generating extra code) on a multiplatform kernel that has no platform other than MSM or EXYNOS, but the other alternative would be that each platform with TWD support has to select HAVE_ARM_TWD itself.

Quoted 21 lines · Arnd Bergmann

arch/arm/Kconfig config LOCAL_TIMERS · line 1628

Quoted 7 lines · Gregory CLEMENT

arch/arm/Kconfig +1 -1

@@ -1624,7 +1624,7 @@ config LOCAL_TIMERS
        bool "Use local timer interrupts"
        depends on SMP
        default y
-       select HAVE_ARM_TWD if (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT)
+       select HAVE_ARM_TWD if (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT && !ARMADA_370_XP_TIMER)
 

Maybe it can be written as

config LOCAL_TIMERS
bool "Use local timer interrupts"
depends on SMP
default y

config HAVE_ARM_TWD
depends on LOCAL_TIMERS
default ARCH_MULTIPLATFORM || (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT) default y

Quoted 3 lines

This will still be slightly wrong (generating extra code) on a multiplatform kernel that has no platform other than MSM or EXYNOS, but the other alternative would be that each platform with TWD support has to select HAVE_ARM_TWD itself.

Why would having each platform with TWD support select HAVE_ARM_TWD be a problem?

On multiplatform kernels where at least one arch has TWD support, it will be included, and where not, it will not.

It doesn't seem logical for a feature descriptor (HAVE_ARM_TWD) to rely on greater knowledge of what platforms may or may not support it. If we had to include every platform in that list it would be unwieldy.

Which leads me to this; I wonder if there should be a policy document that basically describes what HAVE_* really is meant for, and how dependencies on ARCH_* (or MACH_* in the world of multiplatform) should be handled for architectural features? That way there's a little more fixed definition of how Kconfigs should be written to effectively support multiplatform kernels and allow people to identify misuses and get rid of them for multiplatform, if only just to get the information out of the heads of maintainers and into a file somewhere that we can reference... maybe it exists already but I am missing it?

Documentation is generally considered a good thing, but few people can be bothered to write it, and few of the other people that should read it actually do.

Quoted 6 lines

On multiplatform kernels where at least one arch has TWD support, it will be included, and where not, it will not.

It doesn't seem logical for a feature descriptor (HAVE_ARM_TWD) to rely on greater knowledge of what platforms may or may not support it. If we had to include every platform in that list it would be unwieldy.

I would expect that all future platforms have ARM_TWD, so the list of the platforms that don't is not going to grow much, but the list of platforms that do will keep growing.

No, I expect all future platforms will have architected timers. TWD is pretty much A9 and A5 only I believe. Same with SCU. I've probably missed some in my list.

arch/arm/Kconfig config LOCAL_TIMERS · line 1628

Quoted 7 lines · Gregory CLEMENT

arch/arm/Kconfig +1 -1

@@ -1624,7 +1624,7 @@ config LOCAL_TIMERS
        bool "Use local timer interrupts"
        depends on SMP
        default y
-       select HAVE_ARM_TWD if (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT)
+       select HAVE_ARM_TWD if (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT && !ARMADA_370_XP_TIMER)
 

Your change is fine, but I just noticed that this line is asking for trouble when we enable multipleform support for MSM and/or EXYNOS.

Also, I wonder if we should change this somehow in 3.8, because it seems that for a multiplatform kernel including armadaxp and e.g. versatile express, you have no ARM_TWD support in the kernel, which seems wrong. Is there any reason we can't enable the ARM_TWD code to be built-in on platforms that don't use it?

Maybe it can be written as

config LOCAL_TIMERS
bool "Use local timer interrupts"
depends on SMP
default y

config HAVE_ARM_TWD

	depends on LOCAL_TIMERS
	default ARCH_MULTIPLATFORM || (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT)

default y

This will still be slightly wrong (generating extra code) on a multiplatform kernel that has no platform other than MSM or EXYNOS, but the other alternative would be that each platform with TWD support has to select HAVE_ARM_TWD itself.

I don't see a strong reason to not enable it if we don't use it. My concern was that I don't need it so I didn't want to include it and generating extra code for nothing. Then just after having sent this patch set, I received your patch set about build regression in 3.8 and especially the part about CONFIG_MULTIPLATFORM made me realized that it could be a problem.

Quoted 7 lines

Maybe it can be written as

config LOCAL_TIMERS
bool "Use local timer interrupts"
depends on SMP
default y

Quoted 3 lines

config HAVE_ARM_TWD
depends on LOCAL_TIMERS
default ARCH_MULTIPLATFORM || (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT)

So in this case why not written something like this:

        default ARCH_MULTIPLATFORM || (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT && !ARMADA_370_XP_TIMER)

Quoted 1 line

default y

I am not a kconfig expert, but won't this line set HAVE_ARM_TWD to 'y' whatever the result of the previous line?

Quoted 5 lines · Gregory CLEMENT

I don't see a strong reason to not enable it if we don't use it. My concern was that I don't need it so I didn't want to include it and generating extra code for nothing. Then just after having sent this patch set, I received your patch set about build regression in 3.8 and especially the part about CONFIG_MULTIPLATFORM made me realized that it could be a problem.

Ok.

Quoted 11 lines

Quoted 10 lines

Maybe it can be written as

config LOCAL_TIMERS
bool "Use local timer interrupts"
depends on SMP
default y

config HAVE_ARM_TWD
depends on LOCAL_TIMERS
default ARCH_MULTIPLATFORM || (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT)

Quoted 2 lines

So in this case why not written something like this:

        default ARCH_MULTIPLATFORM || (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT && !ARMADA_370_XP_TIMER)

That does not change anything, because ARMADA_370_XP_TIMER is only ever enabled when ARCH_MULTIPLATFORM is enabled as well.

Quoted 4 lines

Quoted 2 lines

default y

I am not a kconfig expert, but won't this line set HAVE_ARM_TWD to 'y' whatever the result of the previous line?

Yes, that was a mistake on my side.

Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 5 lines · Gregory CLEMENT

I don't see a strong reason to not enable it if we don't use it. My concern was that I don't need it so I didn't want to include it and generating extra code for nothing. Then just after having sent this patch set, I received your patch set about build regression in 3.8 and especially the part about CONFIG_MULTIPLATFORM made me realized that it could be a problem.

Ok.

Quoted 7 lines

Quoted 7 lines

Maybe it can be written as

config LOCAL_TIMERS
bool "Use local timer interrupts"
depends on SMP
default y

Quoted 6 lines

Quoted 3 lines

config HAVE_ARM_TWD
depends on LOCAL_TIMERS
default ARCH_MULTIPLATFORM || (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT)

So in this case why not written something like this:

        default ARCH_MULTIPLATFORM || (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT && !ARMADA_370_XP_TIMER)

That does not change anything, because ARMADA_370_XP_TIMER is only ever enabled when ARCH_MULTIPLATFORM is enabled as well.

Quoted 3 lines

Quoted 1 line

default y

I am not a kconfig expert, but won't this line set HAVE_ARM_TWD to 'y' whatever the result of the previous line?

Yes, that was a mistake on my side.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

default y

I am not a kconfig expert, but won't this line set HAVE_ARM_TWD to 'y' whatever the result of the previous line?

Yes, that was a mistake on my side.

Sigh. No. Wrong.

config HAVE_ARM_TWD
depends on LOCAL_TIMERS
default ARCH_MULTIPLATFORM || (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT && !ARMADA_370_XP_TIMER) default y

This takes the value of the first enabled default. The first enabled default is the first default (it's unconditional). So, the default y will never be used.

Given the above, it's far from clear what the actual behaviour being asked for is - it looks totally and utterly screwed to me - and the wrong thing to be doing.

If the desire is to have it enabled if ARCH_MULTIPLATFORM is set, then it's easy, and requires just a single line addition:

config LOCAL_TIMERS
bool "Use local timer interrupts"
depends on SMP
default y
select HAVE_ARM_TWD if (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT)
+ select HAVE_ARM_TWD if ARCH_MULTIPLATFORM

Quoted 11 lines · Gregory CLEMENT

Documentation/devicetree/bindings/timer/marvell,armada-370-xp-timer.txt new file +11 -0

+
+Required properties:
+- compatible: Should be "marvell,armada-370-xp-timer"
+- 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
+
+Optional properties:

Quoted 2 lines

+- marvell,timer-25Mhz: Tells whether the Global timer supports the 25
+  Mhz fixed mode (available on Armada XP and not on Armada 370)

This works fine, but I would consider it slightly cleaner to kill the marvell,timer-25Mhz property and instead have separate marvell,armada-370-timer and marvell,armada-xp-timer compatible strings, since the two timers are actually different, rather than wired differently.

Hum, I am not sure they are different. From my point of view it is the same IP, but the one on Armada XP can use a 25MHz fix source clock. Apart from this, all the registers are the same. Moreover currently with this property we still have the possibility to not use the 25MHz clock source on Armada XP which is totally valid. The use of this source clock is not mandatory for the Armada XP SoCs.

For now I prefer to keep this property.