[PATCH 0/6] arm: mvebu: add support for local timer for Armada 370/XP

[PATCH 0/6] arm: mvebu: add support for local timer for Armada 370/XP

[Gregory CLEMENT]

Hello,

The Armada XP SoCs comes with private timers. This allows us to use
local timers through CONFIG_LOCAL_TIMERS and as stated in the kconfig
help, it prevents a "thundering herd" at every timer tick.

Armada 370 also have these private timers, and even if it comes only
with a single CPU, the feature is also enabled for this SoC to keep
the code generic.

In order to be able to use the local timer, I also had to add the
support for the per-CPU interrupts.

As this patch set need to modify several sub-systems, I'd like that
Jason take the whole series with the acked-by of each maintainer.

Jason I also try to create the patch to fit the order you expect
drivers, then boards and finally dt.

The 1st patch which add support for local interrupt should go directly
to the driver branch of Jason.

The 2nd patch which adds local timer support for Armada 370/XP should
received a acked-by John Stultz or Thomas Gleixner before going to the
driver branch of Jason.

For the 3rd patch, which just allow to not select TWD by default when
local timers are selected for Armada 370/XP, it would be nice if
Russell could give his acked-by. I think this patch should go to the
board branch of Jason, but I am not sure.

The 4th patch is an update to the mvebu_defconfig and should go to the
board branch of Jason.

The 5th and 6th patches are about the DT bindings. An acked-by or at
least a reviewed-by from Rob Herring or Grant Likely would be nice
before going ending to the dt branch of Jason.

This patch set is based on 3.8-rc4 and is obviously 3.9 material. The
git branch called local_timer is available at:
https://github.com/MISL-EBU-System-SW/mainline-public.git.

Thanks,

Gregory CLEMENT (6):
  arm: mvebu: Add support for local interrupt
  clocksource: time-armada-370-xp: add local timer support
  arm: kconfig: don't select TWD with local timer for Armada 370/XP
  arm: mvebu: update defconfig with local timer support
  arm: mvebu: update DT to support local timers
  clocksource: update and move armada-370-xp-timer documentation to
    timer directory

 .../bindings/arm/armada-370-xp-timer.txt           |   12 --
 .../bindings/timer/marvell,armada-370-xp-timer.txt |   15 ++
 arch/arm/Kconfig                                   |    2 +-
 arch/arm/boot/dts/armada-370-xp.dtsi               |    5 +-
 arch/arm/configs/mvebu_defconfig                   |    1 -
 arch/arm/mach-mvebu/irq-armada-370-xp.c            |   18 ++-
 drivers/clocksource/time-armada-370-xp.c           |  150 +++++++++++++++-----
 7 files changed, 144 insertions(+), 59 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/arm/armada-370-xp-timer.txt
 create mode 100644 Documentation/devicetree/bindings/timer/marvell,armada-370-xp-timer.txt

-- 
1.7.9.5

[PATCH 1/6] arm: mvebu: Add support for local interrupt

[Gregory CLEMENT]

MPIC allows the use of private interrupt for each CPUs. The 28th first
interrupts are per-cpu. This patch adds support to use them.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/mach-mvebu/irq-armada-370-xp.c |   18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-mvebu/irq-armada-370-xp.c b/arch/arm/mach-mvebu/irq-armada-370-xp.c
index f99a4a2..c007fdc 100644
--- a/arch/arm/mach-mvebu/irq-armada-370-xp.c
+++ b/arch/arm/mach-mvebu/irq-armada-370-xp.c
@@ -145,11 +145,19 @@ static int armada_370_xp_mpic_irq_map(struct irq_domain *h,
 {
 	armada_370_xp_irq_mask(irq_get_irq_data(virq));
 	writel(hw, main_int_base + ARMADA_370_XP_INT_SET_ENABLE_OFFS);
-
-	irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip,
-				 handle_level_irq);
 	irq_set_status_flags(virq, IRQ_LEVEL);
-	set_irq_flags(virq, IRQF_VALID | IRQF_PROBE);
+
+	if (hw < ARMADA_370_XP_MAX_PER_CPU_IRQS) {
+
+		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);
+	}
+		set_irq_flags(virq, IRQF_VALID | IRQF_PROBE);
 
 	return 0;
 }
@@ -245,7 +253,7 @@ asmlinkage void __exception_irq_entry armada_370_xp_handle_irq(struct pt_regs
 		if (irqnr > 1022)
 			break;
 
-		if (irqnr >= 8) {
+		if (irqnr > 0) {
 			irqnr =	irq_find_mapping(armada_370_xp_mpic_domain,
 					irqnr);
 			handle_IRQ(irqnr, regs);
-- 
1.7.9.5

[PATCH 1/6] arm: mvebu: Add support for local interrupt

[Thomas Petazzoni]

Dear Gregory CLEMENT,

Just some minor nitpicks below.

On Mon, 21 Jan 2013 18:53:57 +0100, Gregory CLEMENT wrote:

> +	if (hw < ARMADA_370_XP_MAX_PER_CPU_IRQS) {
> +

Unneeded empty line.

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

> +		set_irq_flags(virq, IRQF_VALID | IRQF_PROBE);

Incorrect indentation for this line.

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

[PATCH 1/6] arm: mvebu: Add support for local interrupt

[Gregory CLEMENT]

On 01/21/2013 07:17 PM, Thomas Petazzoni wrote:
> Dear Gregory CLEMENT,
> 
> Just some minor nitpicks below.

OK I will take them into account for the V2.

Thanks

> 
> On Mon, 21 Jan 2013 18:53:57 +0100, Gregory CLEMENT wrote:
> 
>> +	if (hw < ARMADA_370_XP_MAX_PER_CPU_IRQS) {
>> +
> 
> Unneeded empty line.
> 
>> +		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.
> 
>> +		set_irq_flags(virq, IRQF_VALID | IRQF_PROBE);
> 
> Incorrect indentation for this line.
> 
> Thomas
> 


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

[PATCH 1/6] arm: mvebu: Add support for local interrupt

[Ezequiel Garcia]

Hi Thomas and Gregory,

On Mon, Jan 21, 2013 at 11:07:10PM +0100, Gregory CLEMENT wrote:
> On 01/21/2013 07:17 PM, Thomas Petazzoni wrote:
> >> +		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,

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

[PATCH 1/6] arm: mvebu: Add support for local interrupt

[Gregory CLEMENT]

On 01/22/2013 12:26 AM, Ezequiel Garcia wrote:
> Hi Thomas and Gregory,
> 
> On Mon, Jan 21, 2013 at 11:07:10PM +0100, Gregory CLEMENT wrote:
>> On 01/21/2013 07:17 PM, Thomas Petazzoni wrote:
>>>> +		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).
> 

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.

> Regards,
> 


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

[PATCH 1/6] arm: mvebu: Add support for local interrupt

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Tue, 22 Jan 2013 10:09:03 +0100, Gregory CLEMENT wrote:

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

ACK, fine. Didn't know about this specific point of the CodingStyle.

Best regards,

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

[PATCH 2/6] clocksource: time-armada-370-xp: add local timer support

[Gregory CLEMENT]

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 <gregory.clement@free-electrons.com>
---
 drivers/clocksource/time-armada-370-xp.c |  150 ++++++++++++++++++++++--------
 1 file changed, 112 insertions(+), 38 deletions(-)

diff --git a/drivers/clocksource/time-armada-370-xp.c b/drivers/clocksource/time-armada-370-xp.c
index a4605fd..757f034 100644
--- a/drivers/clocksource/time-armada-370-xp.c
+++ b/drivers/clocksource/time-armada-370-xp.c
@@ -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
+	}
+}
-- 
1.7.9.5

[PATCH 2/6] clocksource: time-armada-370-xp: add local timer support

[Russell King - ARM Linux]

On Mon, Jan 21, 2013 at 06:53:58PM +0100, Gregory CLEMENT wrote:
> +struct clock_event_device __percpu **percpu_armada_370_xp_evt;
> +

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

[PATCH 2/6] clocksource: time-armada-370-xp: add local timer support

[Gregory CLEMENT]

On 01/21/2013 06:59 PM, Russell King - ARM Linux wrote:
> On Mon, Jan 21, 2013 at 06:53:58PM +0100, Gregory CLEMENT wrote:
>> +struct clock_event_device __percpu **percpu_armada_370_xp_evt;
>> +
> 
> Have you run this through checkpatch?  Should the above be static?
> 

Yes (and it didn't complain) and yes it should be static, I will fix
this in the V2.

Thanks

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

[PATCH 2/6] clocksource: time-armada-370-xp: add local timer support

[Gregory CLEMENT]

On 01/21/2013 06:53 PM, Gregory CLEMENT wrote:
> 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.

John,

Do you have any comments on this patch before I send a second patch set
to take into account the remarks I got on the other patches ?

Thanks,

> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  drivers/clocksource/time-armada-370-xp.c |  150 ++++++++++++++++++++++--------
>  1 file changed, 112 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/clocksource/time-armada-370-xp.c b/drivers/clocksource/time-armada-370-xp.c
> index a4605fd..757f034 100644
> --- a/drivers/clocksource/time-armada-370-xp.c
> +++ b/drivers/clocksource/time-armada-370-xp.c
> @@ -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
> +	}
> +}
> 


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

[PATCH 3/6] arm: kconfig: don't select TWD with local timer for Armada 370/XP

[Gregory CLEMENT]

The Armada 370 and Armada XP SoCs don't use the TWD timers, so don't
select it by default if CONFIG_LOCAL_TIMERS is selected

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/Kconfig |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 67874b8..4c4d010 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -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)
 	help
 	  Enable support for local timers on SMP platforms, rather then the
 	  legacy IPI broadcast method.  Local timers allows the system
-- 
1.7.9.5

[PATCH 3/6] arm: kconfig: don't select TWD with local timer for Armada 370/XP

[Arnd Bergmann]

On Monday 21 January 2013, Gregory CLEMENT wrote:
> @@ -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.

	Arnd

[PATCH 3/6] arm: kconfig: don't select TWD with local timer for Armada 370/XP

[Matt Sealey]

On Mon, Jan 21, 2013 at 12:31 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 21 January 2013, Gregory CLEMENT wrote:
>> @@ -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.

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

If we look at it logically, this is the most efficient solution in
terms of brevity of the (already huge) Kconfig.

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?

--
Matt Sealey <matt@genesi-usa.com>
Product Development Analyst, Genesi USA, Inc.

[PATCH 3/6] arm: kconfig: don't select TWD with local timer for Armada 370/XP

[Arnd Bergmann]

On Monday 21 January 2013, Matt Sealey wrote:
> On Mon, Jan 21, 2013 at 12:31 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Monday 21 January 2013, Gregory CLEMENT wrote:
> >> @@ -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.
> 
> Why would having each platform with TWD support select HAVE_ARM_TWD be
> a problem?

Not a big one, it's just one more Kconfig line for each of a large number
of platforms in the long run.

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

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

To some extent this problem will improve over time for the ARM
multiplatform support as we find all the bugs like this one
and fix them.

	Arnd

[PATCH 3/6] arm: kconfig: don't select TWD with local timer for Armada 370/XP

[Russell King - ARM Linux]

On Mon, Jan 21, 2013 at 08:44:41PM +0000, Arnd Bergmann wrote:
> 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.

Actually _that_ is just one of the problems with documentation.  The
whole thing is full of problems:

1. People don't write documentation.

2. People don't bother to update correct documentation when the code
   changes in a way that the documentation should be updated for.

3. People don't bother to read documentation which is provided.

(2) is about as bad as it gets - because the small percentage of people
who do read the documentation rather than the code now are lead down
paths which are no longer true.

And (2) educates people to behave like (3).  And (3) educates people to
behave like (1) - why should they bother if they're just going to have
to waste time in an email explaining it time and time again (even when
documentation does exist.)

[PATCH 3/6] arm: kconfig: don't select TWD with local timer for Armada 370/XP

[Rob Herring]

On 01/21/2013 02:44 PM, Arnd Bergmann wrote:

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

Rob

[PATCH 3/6] arm: kconfig: don't select TWD with local timer for Armada 370/XP

[Arnd Bergmann]

On Tuesday 22 January 2013, Rob Herring wrote:
> 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.

Ah, I was incorrectly assuming that TWD was referring to the architected
timers, sorry about that.

Maybe we should do this differently then and do the more logical

config LOCAL_TIMERS
	bool

config ARM_TWD
	bool "ARM TWD local timer"
	depends on SMP && CPU_V7
	select LOCAL_TIMERS

config EXYNOS4_MCT
	bool "Samsung Exynos4 MCT Timer"
	depends on SMP && ARCH_EXYNOS
	select LOCAL_TIMERS

config MSM_LOCAL_TIMER
	bool "Qualcomm MSM Local Timer"
	depends on SMP && ARCH_MSM
	select LOCAL_TIMERS

That way we have no implicit assumptions. Everything will still work if you
disable local timers, but of course we will enable them in the defconfigs.

	Arnd

[PATCH 3/6] arm: kconfig: don't select TWD with local timer for Armada 370/XP

[Gregory CLEMENT]

On 01/21/2013 07:31 PM, Arnd Bergmann wrote:
> On Monday 21 January 2013, Gregory CLEMENT wrote:
>> @@ -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?

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.

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

So in this case why not written something like this:
 	default ARCH_MULTIPLATFORM || (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT && !ARMADA_370_XP_TIMER)

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

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

Gregory

[PATCH 3/6] arm: kconfig: don't select TWD with local timer for Armada 370/XP

[Arnd Bergmann]

On Monday 21 January 2013, Gregory CLEMENT wrote:
> 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.

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

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

[PATCH 3/6] arm: kconfig: don't select TWD with local timer for Armada 370/XP

[Gregory CLEMENT]

On 01/22/2013 04:57 PM, Arnd Bergmann wrote:
> On Monday 21 January 2013, Gregory CLEMENT wrote:
>> 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.
> 
>>> 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)
>>
>> 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.

Yes you're right.

So I remove this patch of my series as I don't need it anymore for supporting local
timer on Armada XP/370.

And I will submit this patch as a standalone one.

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


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

[PATCH] arm: kconfig: always select TWD with local timer for multiplatform

[Gregory CLEMENT]

When we enable multiplatform and local timers we should not disable
the build of the TWD driver even if there is support for MSM and/or
EXYNOS (those SoCs don't use TWD for their local timers). Indeed most
of the other SoCs will need it.

If multiplatform is not enable we keep the possibility to not add
support for TWD as we only built a kernel for MSM or for EXYNOS.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/Kconfig |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 67874b8..482ee23 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1625,6 +1625,7 @@ config LOCAL_TIMERS
 	depends on SMP
 	default y
 	select HAVE_ARM_TWD if (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT)
+	select HAVE_ARM_TWD if ARCH_MULTIPLATFORM
 	help
 	  Enable support for local timers on SMP platforms, rather then the
 	  legacy IPI broadcast method.  Local timers allows the system
-- 
1.7.9.5

[PATCH 3/6] arm: kconfig: don't select TWD with local timer for Armada 370/XP

[Russell King - ARM Linux]

On Tue, Jan 22, 2013 at 03:57:02PM +0000, Arnd Bergmann wrote:
> On Monday 21 January 2013, Gregory CLEMENT wrote:
> > 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.
> 
> > > 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)
> > 
> > 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.
> 
> > >       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

[PATCH 3/6] arm: kconfig: don't select TWD with local timer for Armada 370/XP

[Arnd Bergmann]

On Tuesday 22 January 2013, Russell King - ARM Linux wrote:
> > 
> > > >       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.

Right. I actually know how it works, but didn't think through it this
time. The 'default y' was a mistake on my side and should not have
been in there. I was also missing a 'bool' statement in there, which
makes the whole thing a syntax error.

> 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

Right. The effect would be the same as what I intended to write:

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

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

but your patch is simpler.

	Arnd

[PATCH 4/6] arm: mvebu: update defconfig with local timer support

[Gregory CLEMENT]

Now that we have support for local timers, enable it by default

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/configs/mvebu_defconfig |    1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/configs/mvebu_defconfig b/arch/arm/configs/mvebu_defconfig
index b5bc96c..28c1e38 100644
--- a/arch/arm/configs/mvebu_defconfig
+++ b/arch/arm/configs/mvebu_defconfig
@@ -14,7 +14,6 @@ CONFIG_MACH_ARMADA_XP=y
 # CONFIG_CACHE_L2X0 is not set
 # CONFIG_SWP_EMULATE is not set
 CONFIG_SMP=y
-# CONFIG_LOCAL_TIMERS is not set
 CONFIG_AEABI=y
 CONFIG_HIGHMEM=y
 # CONFIG_COMPACTION is not set
-- 
1.7.9.5

[PATCH 5/6] arm: mvebu: update DT to support local timers

[Gregory CLEMENT]

Now that the time-armada-370-xp support local timers, updated the
device tree to take it into account.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-xp.dtsi |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 4c0abe8..aa6a187 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -68,8 +68,9 @@
 
 		timer at d0020300 {
 			       compatible = "marvell,armada-370-xp-timer";
-			       reg = <0xd0020300 0x30>;
-			       interrupts = <37>, <38>, <39>, <40>;
+			       reg = <0xd0020300 0x30>,
+			       <0xd0021040 0x30>;
+			       interrupts = <37>, <38>, <39>, <40>, <5>, <6>;
 			       clocks = <&coreclk 2>;
 		};
 
-- 
1.7.9.5

[PATCH 6/6] clocksource: update and move armada-370-xp-timer documentation to timer directory

[Gregory CLEMENT]

Timer driver for Armada 370 and Armada XP have gained local timers
support. So it needs new resources information regarding the IRQs
and the registers.

Also move the documentation in the new and more accurate directory

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 .../bindings/arm/armada-370-xp-timer.txt           |   12 ------------
 .../bindings/timer/marvell,armada-370-xp-timer.txt |   15 +++++++++++++++
 2 files changed, 15 insertions(+), 12 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/arm/armada-370-xp-timer.txt
 create mode 100644 Documentation/devicetree/bindings/timer/marvell,armada-370-xp-timer.txt

diff --git a/Documentation/devicetree/bindings/arm/armada-370-xp-timer.txt b/Documentation/devicetree/bindings/arm/armada-370-xp-timer.txt
deleted file mode 100644
index 6483011..0000000
--- a/Documentation/devicetree/bindings/arm/armada-370-xp-timer.txt
+++ /dev/null
@@ -1,12 +0,0 @@
-Marvell Armada 370 and Armada XP Global Timers
-----------------------------------------------
-
-Required properties:
-- compatible: Should be "marvell,armada-370-xp-timer"
-- interrupts: Should contain the list of Global Timer interrupts
-- reg: Should contain the base address of the Global Timer registers
-- 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)
diff --git a/Documentation/devicetree/bindings/timer/marvell,armada-370-xp-timer.txt b/Documentation/devicetree/bindings/timer/marvell,armada-370-xp-timer.txt
new file mode 100644
index 0000000..3638112
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/marvell,armada-370-xp-timer.txt
@@ -0,0 +1,15 @@
+Marvell Armada 370 and Armada XP Timers
+---------------------------------------
+
+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:
+- marvell,timer-25Mhz: Tells whether the Global timer supports the 25
+  Mhz fixed mode (available on Armada XP and not on Armada 370)
-- 
1.7.9.5

[PATCH 6/6] clocksource: update and move armada-370-xp-timer documentation to timer directory

[Arnd Bergmann]

On Monday 21 January 2013, Gregory CLEMENT wrote:
> +
> +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:
> +- 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.

	Arnd

[PATCH 6/6] clocksource: update and move armada-370-xp-timer documentation to timer directory

[Gregory CLEMENT]

On 01/21/2013 07:22 PM, Arnd Bergmann wrote:
> On Monday 21 January 2013, Gregory CLEMENT wrote:
>> +
>> +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:
>> +- 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.

Gregory

[PATCH 6/6] clocksource: update and move armada-370-xp-timer documentation to timer directory

[Arnd Bergmann]

On Monday 21 January 2013, Gregory CLEMENT wrote:
> 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.

Ok, fair enough.

	Arnd