[PATCH] ARM: Orion: Hoist bridge interrupt handling out of the timer

[PATCH] ARM: Orion: Hoist bridge interrupt handling out of the timer

[Jason Gunthorpe]

The intent of this patch is to expose the other bridge cause
interrupts to users in the kernel.

- Add orion_bridge_irq_init to create a new edge triggered interrupt
  chip based on the bridge cause register
- Remove all interrupt register code from time.c and use normal
  interrupt functions instead
- Update the machines that use orion_time_init to call
  orion_bridge_irq_init and use the new signature

Tested on a Kirkwood platform.

NOTE: I'm skeptical that MV78xx0 has a bridge interrupt cause/mask
register. However, it was setup so the timer code would touch those
registers, so I've preserved that. Unfortunately prior to this patch
the 'bridge cause register' was only written to, never read. If it is
wired-to-zero on MV78xx0 because it doesn't exist then the timer will
fail to function. The fix is easy, but I need someone with the
manual/system to tell me which is right.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 arch/arm/mach-dove/common.c                       |    5 +-
 arch/arm/mach-dove/include/mach/bridge-regs.h     |    2 +-
 arch/arm/mach-dove/include/mach/irqs.h            |    9 +++-
 arch/arm/mach-kirkwood/common.c                   |    6 ++-
 arch/arm/mach-kirkwood/include/mach/bridge-regs.h |    2 -
 arch/arm/mach-kirkwood/include/mach/irqs.h        |   14 +++++-
 arch/arm/mach-mv78xx0/common.c                    |    8 +++-
 arch/arm/mach-mv78xx0/include/mach/bridge-regs.h  |    2 +-
 arch/arm/mach-mv78xx0/include/mach/irqs.h         |    9 +++-
 arch/arm/mach-orion5x/common.c                    |    6 ++-
 arch/arm/mach-orion5x/include/mach/bridge-regs.h  |    2 -
 arch/arm/mach-orion5x/include/mach/irqs.h         |    9 +++-
 arch/arm/plat-orion/include/plat/irq.h            |    3 +
 arch/arm/plat-orion/include/plat/time.h           |    4 +-
 arch/arm/plat-orion/irq.c                         |   51 +++++++++++++++++++++
 arch/arm/plat-orion/time.c                        |   46 ++++---------------
 16 files changed, 120 insertions(+), 58 deletions(-)

My immediate need is to have the kernel panic on watchdog timer
expiry, but I also think this might be the first step to clean up this
item:

        /*
         * Disable propagation of mbus errors to the CPU local bus,
         * as this causes mbus errors (which can occur for example
         * for PCI aborts) to throw CPU aborts, which we're not set
         * up to deal with.
         */
        writel(readl(CPU_CONFIG) & ~CPU_CONFIG_ERROR_PROP, CPU_CONFIG);

Regards,
Jason

diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c
index f723fe1..9107808 100644
--- a/arch/arm/mach-dove/common.c
+++ b/arch/arm/mach-dove/common.c
@@ -243,8 +243,9 @@ static int __init dove_find_tclk(void)
 static void __init dove_timer_init(void)
 {
 	dove_tclk = dove_find_tclk();
-	orion_time_init(BRIDGE_VIRT_BASE, BRIDGE_INT_TIMER1_CLR,
-			IRQ_DOVE_BRIDGE, dove_tclk);
+	orion_bridge_irq_init(IRQ_DOVE_BRIDGE, IRQ_DOVE_BRIDGE_START,
+			      BRIDGE_CAUSE);
+	orion_time_init(IRQ_DOVE_BRIDGE_TIMER1, dove_tclk);
 }
 
 struct sys_timer dove_timer = {
diff --git a/arch/arm/mach-dove/include/mach/bridge-regs.h b/arch/arm/mach-dove/include/mach/bridge-regs.h
index 99f259e..3bd4656 100644
--- a/arch/arm/mach-dove/include/mach/bridge-regs.h
+++ b/arch/arm/mach-dove/include/mach/bridge-regs.h
@@ -26,7 +26,7 @@
 #define SYSTEM_SOFT_RESET	(BRIDGE_VIRT_BASE + 0x010c)
 #define  SOFT_RESET		0x00000001
 
-#define  BRIDGE_INT_TIMER1_CLR	(~0x0004)
+#define BRIDGE_CAUSE            (BRIDGE_VIRT_BASE + 0x0110)
 
 #define IRQ_VIRT_BASE		(BRIDGE_VIRT_BASE + 0x0200)
 #define IRQ_CAUSE_LOW_OFF	0x0000
diff --git a/arch/arm/mach-dove/include/mach/irqs.h b/arch/arm/mach-dove/include/mach/irqs.h
index 03d401d..53c7d82 100644
--- a/arch/arm/mach-dove/include/mach/irqs.h
+++ b/arch/arm/mach-dove/include/mach/irqs.h
@@ -78,9 +78,16 @@
 #define IRQ_DOVE_SATA		62
 
 /*
+ * Bridge Interrupt Controller
+ */
+#define IRQ_DOVE_BRIDGE_START   64
+#define IRQ_DOVE_BRIDGE_TIMER1  (IRQ_DOVE_BRIDGE_START + 2)
+#define NR_BRIDGE_IRQS          6
+
+/*
  * DOVE General Purpose Pins
  */
-#define IRQ_DOVE_GPIO_START	64
+#define IRQ_DOVE_GPIO_START	(IRQ_DOVE_BRIDGE_START + NR_BRIDGE_IRQS)
 #define NR_GPIO_IRQS		64
 
 /*
diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c
index 906c22e..6ec60b8 100644
--- a/arch/arm/mach-kirkwood/common.c
+++ b/arch/arm/mach-kirkwood/common.c
@@ -35,6 +35,7 @@
 #include <plat/time.h>
 #include <plat/addr-map.h>
 #include <linux/platform_data/dma-mv_xor.h>
+#include <plat/irq.h>
 #include "common.h"
 
 /*****************************************************************************
@@ -534,8 +535,9 @@ static void __init kirkwood_timer_init(void)
 {
 	kirkwood_tclk = kirkwood_find_tclk();
 
-	orion_time_init(BRIDGE_VIRT_BASE, BRIDGE_INT_TIMER1_CLR,
-			IRQ_KIRKWOOD_BRIDGE, kirkwood_tclk);
+	orion_bridge_irq_init(IRQ_KIRKWOOD_BRIDGE, IRQ_KIRKWOOD_BRIDGE_START,
+			      BRIDGE_CAUSE);
+	orion_time_init(IRQ_KIRKWOOD_BRIDGE_TIMER1, kirkwood_tclk);
 }
 
 struct sys_timer kirkwood_timer = {
diff --git a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
index 5c82b7d..fd66a56 100644
--- a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
+++ b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
@@ -29,8 +29,6 @@
 #define BRIDGE_CAUSE		(BRIDGE_VIRT_BASE + 0x0110)
 #define WDT_INT_REQ		0x0008
 
-#define BRIDGE_INT_TIMER1_CLR	(~0x0004)
-
 #define IRQ_VIRT_BASE		(BRIDGE_VIRT_BASE + 0x0200)
 #define IRQ_CAUSE_LOW_OFF	0x0000
 #define IRQ_MASK_LOW_OFF	0x0004
diff --git a/arch/arm/mach-kirkwood/include/mach/irqs.h b/arch/arm/mach-kirkwood/include/mach/irqs.h
index 2bf8161..9e6f406 100644
--- a/arch/arm/mach-kirkwood/include/mach/irqs.h
+++ b/arch/arm/mach-kirkwood/include/mach/irqs.h
@@ -54,9 +54,21 @@
 #define IRQ_KIRKWOOD_RTC        53
 
 /*
+ * Bridge Interrupt Controller
+ */
+#define IRQ_KIRKWOOD_BRIDGE_START     64
+#define IRQ_KIRKWOOD_BRIDGE_SELFINT   (IRQ_KIRKWOOD_BRIDGE_START + 0)
+#define IRQ_KIRKWOOD_BRIDGE_TIMER0    (IRQ_KIRKWOOD_BRIDGE_START + 1)
+#define IRQ_KIRKWOOD_BRIDGE_TIMER1    (IRQ_KIRKWOOD_BRIDGE_START + 2)
+#define IRQ_KIRKWOOD_BRIDGE_TIMERWD   (IRQ_KIRKWOOD_BRIDGE_START + 3)
+#define IRQ_KIRKWOOD_BRIDGE_ACCESSERR (IRQ_KIRKWOOD_BRIDGE_START + 4)
+#define IRQ_KIRKWOOD_BRIDGE_BIT64ERR  (IRQ_KIRKWOOD_BRIDGE_START + 5)
+#define NR_BRIDGE_IRQS          6
+
+/*
  * KIRKWOOD General Purpose Pins
  */
-#define IRQ_KIRKWOOD_GPIO_START	64
+#define IRQ_KIRKWOOD_GPIO_START	(IRQ_KIRKWOOD_BRIDGE_START + NR_BRIDGE_IRQS)
 #define NR_GPIO_IRQS		50
 
 #define NR_IRQS			(IRQ_KIRKWOOD_GPIO_START + NR_GPIO_IRQS)
diff --git a/arch/arm/mach-mv78xx0/common.c b/arch/arm/mach-mv78xx0/common.c
index d0cb485..34bdf2a 100644
--- a/arch/arm/mach-mv78xx0/common.c
+++ b/arch/arm/mach-mv78xx0/common.c
@@ -25,6 +25,7 @@
 #include <plat/time.h>
 #include <plat/common.h>
 #include <plat/addr-map.h>
+#include <plat/irq.h>
 #include "common.h"
 
 static int get_tclk(void);
@@ -338,8 +339,11 @@ void __init mv78xx0_init_early(void)
 
 static void __init_refok mv78xx0_timer_init(void)
 {
-	orion_time_init(BRIDGE_VIRT_BASE, BRIDGE_INT_TIMER1_CLR,
-			IRQ_MV78XX0_TIMER_1, get_tclk());
+	/* FIXME: MV78XX0 may not have a bridge cause register, in which case
+	 * the timer should run directly from IRQ_MV78XX0_TIMER_1 */
+	orion_bridge_irq_init(IRQ_MV78XX0_TIMER_1, IRQ_MV78XX0_BRIDGE_START,
+			      BRIDGE_CAUSE);
+	orion_time_init(IRQ_MV78XX0_BRIDGE_TIMER1, get_tclk());
 }
 
 struct sys_timer mv78xx0_timer = {
diff --git a/arch/arm/mach-mv78xx0/include/mach/bridge-regs.h b/arch/arm/mach-mv78xx0/include/mach/bridge-regs.h
index 5f03484..787bd2a 100644
--- a/arch/arm/mach-mv78xx0/include/mach/bridge-regs.h
+++ b/arch/arm/mach-mv78xx0/include/mach/bridge-regs.h
@@ -20,7 +20,7 @@
 #define SYSTEM_SOFT_RESET	(BRIDGE_VIRT_BASE + 0x010c)
 #define SOFT_RESET		0x00000001
 
-#define BRIDGE_INT_TIMER1_CLR	(~0x0004)
+#define BRIDGE_CAUSE            (BRIDGE_VIRT_BASE + 0x0110)
 
 #define IRQ_VIRT_BASE		(BRIDGE_VIRT_BASE + 0x0200)
 #define IRQ_CAUSE_ERR_OFF	0x0000
diff --git a/arch/arm/mach-mv78xx0/include/mach/irqs.h b/arch/arm/mach-mv78xx0/include/mach/irqs.h
index fa1d422..f5ebebc 100644
--- a/arch/arm/mach-mv78xx0/include/mach/irqs.h
+++ b/arch/arm/mach-mv78xx0/include/mach/irqs.h
@@ -83,9 +83,16 @@
 #define IRQ_MV78XX0_GE_ERR	70
 
 /*
+ * Bridge Interrupt Controller
+ */
+#define IRQ_MV78XX0_BRIDGE_START   96
+#define IRQ_MV78XX0_BRIDGE_TIMER1  (IRQ_MV78XX0_BRIDGE_START + 2)
+#define NR_BRIDGE_IRQS             6
+
+/*
  * MV78XX0 General Purpose Pins
  */
-#define IRQ_MV78XX0_GPIO_START	96
+#define IRQ_MV78XX0_GPIO_START	(IRQ_MV78XX0_BRIDGE_START + NR_BRIDGE_IRQS)
 #define NR_GPIO_IRQS		32
 
 #define NR_IRQS			(IRQ_MV78XX0_GPIO_START + NR_GPIO_IRQS)
diff --git a/arch/arm/mach-orion5x/common.c b/arch/arm/mach-orion5x/common.c
index b3eb3da..1d8c281 100644
--- a/arch/arm/mach-orion5x/common.c
+++ b/arch/arm/mach-orion5x/common.c
@@ -35,6 +35,7 @@
 #include <plat/time.h>
 #include <plat/common.h>
 #include <plat/addr-map.h>
+#include <plat/irq.h>
 #include "common.h"
 
 /*****************************************************************************
@@ -221,8 +222,9 @@ static void __init orion5x_timer_init(void)
 {
 	orion5x_tclk = orion5x_find_tclk();
 
-	orion_time_init(ORION5X_BRIDGE_VIRT_BASE, BRIDGE_INT_TIMER1_CLR,
-			IRQ_ORION5X_BRIDGE, orion5x_tclk);
+	orion_bridge_irq_init(IRQ_ORION5X_BRIDGE, IRQ_ORION5X_BRIDGE_START,
+			      BRIDGE_CAUSE);
+	orion_time_init(IRQ_ORION5X_BRIDGE_TIMER1, orion5x_tclk);
 }
 
 struct sys_timer orion5x_timer = {
diff --git a/arch/arm/mach-orion5x/include/mach/bridge-regs.h b/arch/arm/mach-orion5x/include/mach/bridge-regs.h
index 461fd69..0847182 100644
--- a/arch/arm/mach-orion5x/include/mach/bridge-regs.h
+++ b/arch/arm/mach-orion5x/include/mach/bridge-regs.h
@@ -28,8 +28,6 @@
 
 #define WDT_INT_REQ		0x0008
 
-#define BRIDGE_INT_TIMER1_CLR	(~0x0004)
-
 #define MAIN_IRQ_CAUSE		(ORION5X_BRIDGE_VIRT_BASE + 0x200)
 
 #define MAIN_IRQ_MASK		(ORION5X_BRIDGE_VIRT_BASE + 0x204)
diff --git a/arch/arm/mach-orion5x/include/mach/irqs.h b/arch/arm/mach-orion5x/include/mach/irqs.h
index a6fa9d8..329cdc2 100644
--- a/arch/arm/mach-orion5x/include/mach/irqs.h
+++ b/arch/arm/mach-orion5x/include/mach/irqs.h
@@ -49,9 +49,16 @@
 #define IRQ_ORION5X_XOR1		31
 
 /*
+ * Bridge Interrupt Controller
+ */
+#define IRQ_ORION5X_BRIDGE_START   32
+#define IRQ_ORION5X_BRIDGE_TIMER1  (IRQ_ORION5X_BRIDGE_START + 2)
+#define NR_BRIDGE_IRQS          6
+
+/*
  * Orion General Purpose Pins
  */
-#define IRQ_ORION5X_GPIO_START	32
+#define IRQ_ORION5X_GPIO_START	(IRQ_ORION5X_BRIDGE_START + NR_BRIDGE_IRQS)
 #define NR_GPIO_IRQS		32
 
 #define NR_IRQS			(IRQ_ORION5X_GPIO_START + NR_GPIO_IRQS)
diff --git a/arch/arm/plat-orion/include/plat/irq.h b/arch/arm/plat-orion/include/plat/irq.h
index 50547e4..10d1f44 100644
--- a/arch/arm/plat-orion/include/plat/irq.h
+++ b/arch/arm/plat-orion/include/plat/irq.h
@@ -12,5 +12,8 @@
 #define __PLAT_IRQ_H
 
 void orion_irq_init(unsigned int irq_start, void __iomem *maskaddr);
+void __init orion_bridge_irq_init(unsigned int bridge_irq,
+				  unsigned int irq_start,
+				  void __iomem *causeaddr);
 void __init orion_dt_init_irq(void);
 #endif
diff --git a/arch/arm/plat-orion/include/plat/time.h b/arch/arm/plat-orion/include/plat/time.h
index 07527e4..c5ccc0a 100644
--- a/arch/arm/plat-orion/include/plat/time.h
+++ b/arch/arm/plat-orion/include/plat/time.h
@@ -13,8 +13,6 @@
 
 void orion_time_set_base(void __iomem *timer_base);
 
-void orion_time_init(void __iomem *bridge_base, u32 bridge_timer1_clr_mask,
-		     unsigned int irq, unsigned int tclk);
-
+void orion_time_init(unsigned int irq, unsigned int tclk);
 
 #endif
diff --git a/arch/arm/plat-orion/irq.c b/arch/arm/plat-orion/irq.c
index 1867944..3455686 100644
--- a/arch/arm/plat-orion/irq.c
+++ b/arch/arm/plat-orion/irq.c
@@ -18,6 +18,57 @@
 #include <plat/irq.h>
 #include <plat/orion-gpio.h>
 
+static void bridge_irq_handler(unsigned irq, struct irq_desc *desc)
+{
+	struct irq_chip_generic *gc = irq_get_handler_data(irq);
+	u32 cause;
+	int i;
+
+	cause = readl(gc->reg_base) & readl(gc->reg_base + 4);
+	for (i = 0; i < 6; i++)
+		if ((cause & (1 << i)))
+			generic_handle_irq(i + gc->irq_base);
+}
+
+static void irq_gc_eoi_inv(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	u32 mask = 1 << (d->irq - gc->irq_base);
+	struct irq_chip_regs *regs;
+
+	regs = &container_of(d->chip, struct irq_chip_type, chip)->regs;
+	irq_gc_lock(gc);
+	irq_reg_writel(~mask, gc->reg_base + regs->eoi);
+	irq_gc_unlock(gc);
+}
+
+void __init orion_bridge_irq_init(unsigned int bridge_irq,
+				  unsigned int irq_start,
+				  void __iomem *causeaddr)
+{
+	struct irq_chip_generic *gc;
+	struct irq_chip_type *ct;
+
+	gc = irq_alloc_generic_chip("orion_irq_edge", 1, irq_start,
+				    causeaddr, handle_fasteoi_irq);
+	if (!gc)
+		BUG();
+	ct = gc->chip_types;
+	ct->regs.mask = 4;
+	ct->regs.eoi = 0;
+	/* ACK and mask all interrupts */
+	writel(0, causeaddr);
+	writel(0, causeaddr + ct->regs.mask);
+	ct->chip.irq_eoi = irq_gc_eoi_inv;
+	ct->chip.irq_mask = irq_gc_mask_clr_bit;
+	ct->chip.irq_unmask = irq_gc_mask_set_bit;
+	irq_setup_generic_chip(gc, IRQ_MSK(6), IRQ_GC_INIT_MASK_CACHE,
+			       IRQ_NOREQUEST, IRQ_LEVEL | IRQ_NOPROBE);
+	if (irq_set_handler_data(bridge_irq, gc) != 0)
+		BUG();
+	irq_set_chained_handler(bridge_irq, bridge_irq_handler);
+}
+
 void __init orion_irq_init(unsigned int irq_start, void __iomem *maskaddr)
 {
 	struct irq_chip_generic *gc;
diff --git a/arch/arm/plat-orion/time.c b/arch/arm/plat-orion/time.c
index 0f4fa86..da22aa4 100644
--- a/arch/arm/plat-orion/time.c
+++ b/arch/arm/plat-orion/time.c
@@ -17,15 +17,8 @@
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <asm/sched_clock.h>
-
-/*
- * MBus bridge block registers.
- */
-#define BRIDGE_CAUSE_OFF	0x0110
-#define BRIDGE_MASK_OFF		0x0114
-#define  BRIDGE_INT_TIMER0	 0x0002
-#define  BRIDGE_INT_TIMER1	 0x0004
-
+#include <linux/sched.h>
+#include <plat/time.h>
 
 /*
  * Timer block registers.
@@ -44,9 +37,8 @@
 /*
  * SoC-specific data.
  */
-static void __iomem *bridge_base;
-static u32 bridge_timer1_clr_mask;
 static void __iomem *timer_base;
+static unsigned int timer_irq;
 
 
 /*
@@ -82,11 +74,7 @@ orion_clkevt_next_event(unsigned long delta, struct clock_event_device *dev)
 	/*
 	 * Clear and enable clockevent timer interrupt.
 	 */
-	writel(bridge_timer1_clr_mask, bridge_base + BRIDGE_CAUSE_OFF);
-
-	u = readl(bridge_base + BRIDGE_MASK_OFF);
-	u |= BRIDGE_INT_TIMER1;
-	writel(u, bridge_base + BRIDGE_MASK_OFF);
+	enable_irq(timer_irq);
 
 	/*
 	 * Setup new clockevent timer value.
@@ -122,8 +110,7 @@ orion_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev)
 		/*
 		 * Enable timer interrupt.
 		 */
-		u = readl(bridge_base + BRIDGE_MASK_OFF);
-		writel(u | BRIDGE_INT_TIMER1, bridge_base + BRIDGE_MASK_OFF);
+		enable_irq(timer_irq);
 
 		/*
 		 * Enable timer.
@@ -141,14 +128,7 @@ orion_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev)
 		/*
 		 * Disable timer interrupt.
 		 */
-		u = readl(bridge_base + BRIDGE_MASK_OFF);
-		writel(u & ~BRIDGE_INT_TIMER1, bridge_base + BRIDGE_MASK_OFF);
-
-		/*
-		 * ACK pending timer interrupt.
-		 */
-		writel(bridge_timer1_clr_mask, bridge_base + BRIDGE_CAUSE_OFF);
-
+		disable_irq(timer_irq);
 	}
 	local_irq_restore(flags);
 }
@@ -164,10 +144,6 @@ static struct clock_event_device orion_clkevt = {
 
 static irqreturn_t orion_timer_interrupt(int irq, void *dev_id)
 {
-	/*
-	 * ACK timer interrupt and call event handler.
-	 */
-	writel(bridge_timer1_clr_mask, bridge_base + BRIDGE_CAUSE_OFF);
 	orion_clkevt.event_handler(&orion_clkevt);
 
 	return IRQ_HANDLED;
@@ -186,16 +162,14 @@ orion_time_set_base(void __iomem *_timer_base)
 }
 
 void __init
-orion_time_init(void __iomem *_bridge_base, u32 _bridge_timer1_clr_mask,
-		unsigned int irq, unsigned int tclk)
+orion_time_init(unsigned int irq, unsigned int tclk)
 {
 	u32 u;
 
 	/*
 	 * Set SoC-specific data.
 	 */
-	bridge_base = _bridge_base;
-	bridge_timer1_clr_mask = _bridge_timer1_clr_mask;
+	timer_irq = irq;
 
 	ticks_per_jiffy = (tclk + HZ/2) / HZ;
 
@@ -210,8 +184,6 @@ orion_time_init(void __iomem *_bridge_base, u32 _bridge_timer1_clr_mask,
 	 */
 	writel(0xffffffff, timer_base + TIMER0_VAL_OFF);
 	writel(0xffffffff, timer_base + TIMER0_RELOAD_OFF);
-	u = readl(bridge_base + BRIDGE_MASK_OFF);
-	writel(u & ~BRIDGE_INT_TIMER0, bridge_base + BRIDGE_MASK_OFF);
 	u = readl(timer_base + TIMER_CTRL_OFF);
 	writel(u | TIMER0_EN | TIMER0_RELOAD_EN, timer_base + TIMER_CTRL_OFF);
 	clocksource_mmio_init(timer_base + TIMER0_VAL_OFF, "orion_clocksource",
@@ -220,7 +192,7 @@ orion_time_init(void __iomem *_bridge_base, u32 _bridge_timer1_clr_mask,
 	/*
 	 * Setup clockevent timer (interrupt-driven).
 	 */
-	setup_irq(irq, &orion_timer_irq);
+	setup_irq(timer_irq, &orion_timer_irq);
 	orion_clkevt.mult = div_sc(tclk, NSEC_PER_SEC, orion_clkevt.shift);
 	orion_clkevt.max_delta_ns = clockevent_delta2ns(0xfffffffe, &orion_clkevt);
 	orion_clkevt.min_delta_ns = clockevent_delta2ns(1, &orion_clkevt);
-- 
1.7.5.4

Re: [PATCH] ARM: Orion: Hoist bridge interrupt handling out of the timer

[Andrew Lunn]

On Fri, Dec 07, 2012 at 03:55:07PM -0700, Jason Gunthorpe wrote:
> The intent of this patch is to expose the other bridge cause
> interrupts to users in the kernel.
> 
> - Add orion_bridge_irq_init to create a new edge triggered interrupt
>   chip based on the bridge cause register
> - Remove all interrupt register code from time.c and use normal
>   interrupt functions instead
> - Update the machines that use orion_time_init to call
>   orion_bridge_irq_init and use the new signature

Hi Jason

I like the idea, but i think it needs to go a bit further. 

1) It should have an IRQ domain, like the other IRQ chips we have.
2) It should have a DT binding, like the other IRQ chips we have.
3) We then pass the timer interrupt via DT to the timer driver.
4) We than pass the watchdog interrupt via DT.

We plan to remove old style platforms in the next few cycles, so its
important that changes like this are orientated towards DT but have
the necessary backward compatibility for the old ways for the boards
not yet converted. I think for Dove all boards are DT based...

3) is not so simple, because we currently don't have a timer binding
for Orion SoC. However, with this cleanup, we are much closer to being
able to use the 370/XP timer code.


> @@ -534,8 +535,9 @@ static void __init kirkwood_timer_init(void)
>  {
>  	kirkwood_tclk = kirkwood_find_tclk();
>  
> -	orion_time_init(BRIDGE_VIRT_BASE, BRIDGE_INT_TIMER1_CLR,
> -			IRQ_KIRKWOOD_BRIDGE, kirkwood_tclk);
> +	orion_bridge_irq_init(IRQ_KIRKWOOD_BRIDGE, IRQ_KIRKWOOD_BRIDGE_START,
> +			      BRIDGE_CAUSE);
> +	orion_time_init(IRQ_KIRKWOOD_BRIDGE_TIMER1, kirkwood_tclk);
>  }

I think it would be better to do this in kirkwood_init_irq(). Same for
the other platforms. 

> +static void bridge_irq_handler(unsigned irq, struct irq_desc *desc)
> +{
> +	struct irq_chip_generic *gc = irq_get_handler_data(irq);
> +	u32 cause;
> +	int i;
> +
> +	cause = readl(gc->reg_base) & readl(gc->reg_base + 4);

Could you add a #define for this 4. I guess its an interrupt enable
mask? Could regs.mask be used?

> +	for (i = 0; i < 6; i++)
> +		if ((cause & (1 << i)))
> +			generic_handle_irq(i + gc->irq_base);
> +}
> +
> +static void irq_gc_eoi_inv(struct irq_data *d)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	u32 mask = 1 << (d->irq - gc->irq_base);
> +	struct irq_chip_regs *regs;
> +
> +	regs = &container_of(d->chip, struct irq_chip_type, chip)->regs;
> +	irq_gc_lock(gc);
> +	irq_reg_writel(~mask, gc->reg_base + regs->eoi);
> +	irq_gc_unlock(gc);
> +}
> +
> +void __init orion_bridge_irq_init(unsigned int bridge_irq,
> +				  unsigned int irq_start,
> +				  void __iomem *causeaddr)
> +{
> +	struct irq_chip_generic *gc;
> +	struct irq_chip_type *ct;
> +
> +	gc = irq_alloc_generic_chip("orion_irq_edge", 1, irq_start,

Maybe the name orion_bridge would be better?

Thanks
      Andrew

Re: [PATCH] ARM: Orion: Hoist bridge interrupt handling out of the timer

[Jason Gunthorpe]

On Sat, Dec 08, 2012 at 12:26:24PM +0100, Andrew Lunn wrote:
 
> 1) It should have an IRQ domain, like the other IRQ chips we have.
> 2) It should have a DT binding, like the other IRQ chips we have.

I was going to look at a DT binding for this as a follow on, since
I'll want to bind to these interrupts. 

Are you OK with keeping this patch as is and seeing DT in a follow up,
or as a series? It is already pretty big.

> 4) We than pass the watchdog interrupt via DT.

Right now the watchdog driver is coded to cause a board reset, so it
doesn't use interrupts at all. Adding interrupt support to watchdog
seems orthogonal to this?

What would it look like? For my boards I want the watchdog to panic(),
because I have another watchdog that takes care of reset, but that
won't be universal.

> We plan to remove old style platforms in the next few cycles, so its

Yay :)

> 3) We then pass the timer interrupt via DT to the timer driver.
> 3) is not so simple, because we currently don't have a timer binding
> for Orion SoC. However, with this cleanup, we are much closer to being
> able to use the 370/XP timer code.

Interesting.. The 370/XP is a more advanced version of the same timer
IP, there are several registers that driver is touching that are not
HW supported, at least on kirkwood.

It would be straightforward to add a binding in the style of
time-armada-370-xp.c, I can probably send that as a third patch.

The two DT bindings are straightforward, and my testing on Kirkwood
should cover alot - but it would be great if non-kirkwood boards could
review/test with this patch..

Do you expect a DT conversion for all orion_time_init users, or just
the one I can test or ..?

> > @@ -534,8 +535,9 @@ static void __init kirkwood_timer_init(void)
> >  {
> >  	kirkwood_tclk = kirkwood_find_tclk();
> >  
> > -	orion_time_init(BRIDGE_VIRT_BASE, BRIDGE_INT_TIMER1_CLR,
> > -			IRQ_KIRKWOOD_BRIDGE, kirkwood_tclk);
> > +	orion_bridge_irq_init(IRQ_KIRKWOOD_BRIDGE, IRQ_KIRKWOOD_BRIDGE_START,
> > +			      BRIDGE_CAUSE);
> > +	orion_time_init(IRQ_KIRKWOOD_BRIDGE_TIMER1, kirkwood_tclk);
> >  }
> 
> I think it would be better to do this in kirkwood_init_irq(). Same for
> the other platforms. 

Yes.. I left it like this because it is very easy to audit that it is
correct (ie called, and called at the correc time). When DT support is
added this will have to change again, so expecting that the next patch
will have to change things so orion_bridge_irq_init is not called for
the DT case, and the patch after so orion_time_init is not called for
the DT case, are you OK with this patch leaving it here?

> > +	u32 cause;
> > +	int i;
> > +
> > +	cause = readl(gc->reg_base) & readl(gc->reg_base + 4);
> 
> Could you add a #define for this 4. I guess its an interrupt enable
> mask? Could regs.mask be used?

I will add a define, regs.mask could be used, but since the value is
known and constant I left it as a constant as a micro-optimization.

> > +	gc = irq_alloc_generic_chip("orion_irq_edge", 1, irq_start,
> 
> Maybe the name orion_bridge would be better?

Sure

Thanks,
Jason

Re: [PATCH] ARM: Orion: Hoist bridge interrupt handling out of the timer

[Andrew Lunn]

On Sat, Dec 08, 2012 at 07:57:48PM -0700, Jason Gunthorpe wrote:
> On Sat, Dec 08, 2012 at 12:26:24PM +0100, Andrew Lunn wrote:
>  
> > 1) It should have an IRQ domain, like the other IRQ chips we have.
> > 2) It should have a DT binding, like the other IRQ chips we have.
> 
> I was going to look at a DT binding for this as a follow on, since
> I'll want to bind to these interrupts. 
> 
> Are you OK with keeping this patch as is and seeing DT in a follow up,
> or as a series? It is already pretty big.

Hi Jason

A patch series is great. However, its not so good practice to add
something on the first patch, and then move it somewhere else in the
next. So i would suggest initializing the controller in
kirkwood_irq_init(), etc as its added.

> > 4) We than pass the watchdog interrupt via DT.
> 
> Right now the watchdog driver is coded to cause a board reset, so it
> doesn't use interrupts at all. Adding interrupt support to watchdog
> seems orthogonal to this?

Yes, its orthogonal, but a logical extension which could be part of a
patchset.
 
> What would it look like? For my boards I want the watchdog to panic(),
> because I have another watchdog that takes care of reset, but that
> won't be universal.

There are examples of watchdogs that allow this. However, none yet
have DT bindings. I would suggest adding an optional property,
"panic", which indicates the driver should panic rather than
reboot. Make sure to run this by the device tree mailing list.
 
> > 3) We then pass the timer interrupt via DT to the timer driver.
> > 3) is not so simple, because we currently don't have a timer binding
> > for Orion SoC. However, with this cleanup, we are much closer to being
> > able to use the 370/XP timer code.
> 
> Interesting.. The 370/XP is a more advanced version of the same timer
> IP, there are several registers that driver is touching that are not
> HW supported, at least on kirkwood.

Yes, the 25MHz and the divider for example. I'm not 100% sure it will
actually work, it will need a different compatibility string, and a
bit of configuration based on that string, but i think it goes. If you
compare the two different drivers, they are very similar.

> The two DT bindings are straightforward, and my testing on Kirkwood
> should cover alot - but it would be great if non-kirkwood boards could
> review/test with this patch..
> 
> Do you expect a DT conversion for all orion_time_init users, or just
> the one I can test or ..?

Please take a stab at converting them all. We have an active set of
testers. I can test kirkwood and orion5x, Sebastian tests Dove, Thomas
and Gregory test 370/XP if needed. Nobody seems to care about mv78xx0
so its slowly bit-rotting in a corner.

Thanks
	Andrew

Re: [PATCH] ARM: Orion: Hoist bridge interrupt handling out of the timer

[Sebastian Hesselbarth]

On 12/09/2012 09:30 AM, Andrew Lunn wrote:
> On Sat, Dec 08, 2012 at 07:57:48PM -0700, Jason Gunthorpe wrote:
>> On Sat, Dec 08, 2012 at 12:26:24PM +0100, Andrew Lunn wrote:
>>
>>> 1) It should have an IRQ domain, like the other IRQ chips we have.
>>> 2) It should have a DT binding, like the other IRQ chips we have.
>>
>> I was going to look at a DT binding for this as a follow on, since
>> I'll want to bind to these interrupts.
>>
>> Are you OK with keeping this patch as is and seeing DT in a follow up,
>> or as a series? It is already pretty big.
>
> Hi Jason
>
> A patch series is great. However, its not so good practice to add
> something on the first patch, and then move it somewhere else in the
> next. So i would suggest initializing the controller in
> kirkwood_irq_init(), etc as its added.

Hi Andrew, Jason,

first of all I have adjusted the Cc list a little bit: Gregory is missing,
and I removed Mike and Olof as they might not be that interested in discussing
mvebu internals. Please re-add if I am wrong.

I have an irqchip driver for orion that I wrote some weeks ago. I can
prepare a patch today. I will allow to have main irq controller by DT
and I can also add a secondary irq controller for the bridge registers
as chained irq handler.

If I dig hard enough in my branches I will also find time-orion drivers,
that are either "standalone" (i.e. orion only) or merged with time-mvebu.

The main irq controller will be required for sure, but for the secondary
irq controller we had a discussion long ago. IIRC Gregory proposed to have
shared irqs handled by timer and watchdog, I was proposing chained irqs.

For mvebu archs, IIRC, we have wrt to timer-related irqs:
- Armada 370/XP with different irq handler and timer irq handling within
   timer registers.
- Orion SoCs with Bridge irq registers for timer related stuff (timer0/1)
- Kirkwood and Dove with watchdog timers (both with wdt irq in bridge irqs)
- RTC in bridge irqs, but Dove with RTC connected to PMU irqs

I think we should have patches for irqchip-orion first and then rethink
if we want a standalone timer-orion or merge it with timer-mvebu. Having
watchdog using irqs is kind of independent from this.

> ...
>>> 3) We then pass the timer interrupt via DT to the timer driver.
>>> 3) is not so simple, because we currently don't have a timer binding
>>> for Orion SoC. However, with this cleanup, we are much closer to being
>>> able to use the 370/XP timer code.
>>
>> Interesting.. The 370/XP is a more advanced version of the same timer
>> IP, there are several registers that driver is touching that are not
>> HW supported, at least on kirkwood.
>
> Yes, the 25MHz and the divider for example. I'm not 100% sure it will
> actually work, it will need a different compatibility string, and a
> bit of configuration based on that string, but i think it goes. If you
> compare the two different drivers, they are very similar.

Back in the days when Gregory, Thomas, and I were looking into merged timer
we agreed not to have an extra check on 25MHz support. If you put the
property in the node, it will try to set the timer to fixed 25MHz. If you
use the property on Orion timer, it will just break timer handling.

Sebastian

Re: [PATCH] ARM: Orion: Hoist bridge interrupt handling out of the timer

[Jason Gunthorpe]

On Sun, Dec 09, 2012 at 02:06:48PM +0100, Sebastian Hesselbarth wrote:

> The main irq controller will be required for sure, but for the secondary
> irq controller we had a discussion long ago. IIRC Gregory proposed to have
> shared irqs handled by timer and watchdog, I was proposing chained irqs.

+1 on decoded IRQs for bridge. I've been running that configuration
now since this patch set was first posted.

There is too much HW variance, the timer, watchdog, etc drivers should
not have to poke into SOC specific registers just to get an interrupt.

The bridge decode can either be via a chained handler, or by incorporating
the bridge decode into the main kirkwood handler - the latter having
lower overhead for timer ticks.

> For mvebu archs, IIRC, we have wrt to timer-related irqs:
> - Armada 370/XP with different irq handler and timer irq handling within
>   timer registers.
> - Orion SoCs with Bridge irq registers for timer related stuff (timer0/1)
> - Kirkwood and Dove with watchdog timers (both with wdt irq in bridge irqs)
> - RTC in bridge irqs, but Dove with RTC connected to PMU irqs

> I think we should have patches for irqchip-orion first and then rethink
> if we want a standalone timer-orion or merge it with timer-mvebu. Having
> watchdog using irqs is kind of independent from this.

I would think the logical progression is:
- irq-chip orion combined with work to keep the existing timer working
- Patch to add the bridge irq-chip
- Patches to support orion/kirkwood/dove/etc in the existing timer drivers
- Patch to update the DT to switch to the bridge and updated timer
- Patch to remove the old timer

When I last looked briefly, it seems like merging with timer-mvebu was
fairly straightforward..

> Back in the days when Gregory, Thomas, and I were looking into merged timer
> we agreed not to have an extra check on 25MHz support. If you put the
> property in the node, it will try to set the timer to fixed 25MHz. If you
> use the property on Orion timer, it will just break timer handling.

As for the mveth case we should have a compatible tag for each SOC,
the driver can ignore it, but it should be in the DT for future use..

Jason

Re: [PATCH] ARM: Orion: Hoist bridge interrupt handling out of the timer

[Sebastian Hesselbarth]

On 06/04/13 19:26, Jason Gunthorpe wrote:
> On Sun, Dec 09, 2012 at 02:06:48PM +0100, Sebastian Hesselbarth wrote:
>> The main irq controller will be required for sure, but for the secondary
>> irq controller we had a discussion long ago. IIRC Gregory proposed to have
>> shared irqs handled by timer and watchdog, I was proposing chained irqs.
>
> +1 on decoded IRQs for bridge. I've been running that configuration
> now since this patch set was first posted.
>
> There is too much HW variance, the timer, watchdog, etc drivers should
> not have to poke into SOC specific registers just to get an interrupt.
>
> The bridge decode can either be via a chained handler, or by incorporating
> the bridge decode into the main kirkwood handler - the latter having
> lower overhead for timer ticks.

Jason,

I have irqchip and clocksource drivers done for Orion, just need to
find some time to rebase them.

>> For mvebu archs, IIRC, we have wrt to timer-related irqs:
>> - Armada 370/XP with different irq handler and timer irq handling within
>>    timer registers.
>> - Orion SoCs with Bridge irq registers for timer related stuff (timer0/1)
>> - Kirkwood and Dove with watchdog timers (both with wdt irq in bridge irqs)
>> - RTC in bridge irqs, but Dove with RTC connected to PMU irqs
>
>> I think we should have patches for irqchip-orion first and then rethink
>> if we want a standalone timer-orion or merge it with timer-mvebu. Having
>> watchdog using irqs is kind of independent from this.

I suggest not to merge clocksource for Orion and Armada 370/XP. They
are different enough to justify separate drivers. IIRC Armada 370/XP
acks timer interrupts by clearing timer register bits that are not
implemented in Orion SoCs.

> I would think the logical progression is:
> - irq-chip orion combined with work to keep the existing timer working
> - Patch to add the bridge irq-chip
> - Patches to support orion/kirkwood/dove/etc in the existing timer drivers
> - Patch to update the DT to switch to the bridge and updated timer
> - Patch to remove the old timer

I'd rather have irqchip and clocksource mainlined and enable both
drivers when they have surfaced. I try to sent patches by end of this week.

> When I last looked briefly, it seems like merging with timer-mvebu was
> fairly straightforward..
>
>> Back in the days when Gregory, Thomas, and I were looking into merged timer
>> we agreed not to have an extra check on 25MHz support. If you put the
>> property in the node, it will try to set the timer to fixed 25MHz. If you
>> use the property on Orion timer, it will just break timer handling.
>
> As for the mveth case we should have a compatible tag for each SOC,
> the driver can ignore it, but it should be in the DT for future use..

We could have a single clocksource driver but as said above,
clocksource is a tiny driver compared to others. Separate drivers will
save us from checking SoC on every timer event or have a callback for 
Armada 370/XP clearing timer irqs.

Sebastian