ARM: Orion: Hoist bridge interrupt handling out of the timer

This adds the common DT code and board support for kirkwood and dove

- Add the marvell,orion-bridge-intc DT node to the DTSI file
- Make the call to orion_bridge_irq_init happen only in the non-DT case

Signed-off-by Jason Gunthorpe <[email protected]>

---
 .../devicetree/bindings/arm/mrvl/intc.txt          |   21 ++++++++++++++++++++
 arch/arm/boot/dts/dove.dtsi                        |   10 +++++++++
 arch/arm/boot/dts/kirkwood.dtsi                    |   12 +++++++++++
 arch/arm/mach-dove/common.c                        |    4 ---
 arch/arm/mach-dove/irq.c                           |    5 ++++
 arch/arm/mach-kirkwood/common.c                    |    4 ---
 arch/arm/mach-kirkwood/irq.c                       |    5 ++++
 arch/arm/plat-orion/irq.c                          |   20 +++++++++++++++++++
 8 files changed, 73 insertions(+), 8 deletions(-)

Documentation/devicetree/bindings/arm/mrvl/intc.txt +21 -0

index 8b53273..cf633f1 100644
@@ -58,3 +58,24 @@ Example:
                 reg = <0xfed20204 0x04>,
 		      <0xfed20214 0x04>;
         };
+
+* Marvell Orion Bridge Interrupt controller
+
+Required properties
+- compatible :  Should be "marvell,orion-bridge-intc".
+- #interrupt-cells: Specifies the number of cells needed to encode an
+  interrupt source. Supported value is <1>.
+- interrupt-controller : Declare this node to be an interrupt controller.
+- interrupts: The interrupt number the bridge is attached to on the main
+              controller.
+- reg : Interrupt cause address.
+
+Example:
+
+	bridge_intc: bridge_intc at 20110 {
+		compatible = "marvell,orion-bridge-intc";
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		interrupts = <1>;
+		reg = <0x20110 0x08>;
+        };

arch/arm/boot/dts/dove.dtsi +10 -0

index 5a00022..b726ba8 100644
@@ -67,6 +67,8 @@
 			status = "disabled";
 		};
 
+		/* The interrupt controller stanzas are in an
+		 * order that matches irqs.h */
 		gpio0: gpio at d0400 {
 			compatible = "marvell,orion-gpio";
 			#gpio-cells = <2>;

@@ -93,6 +95,14 @@
 			ngpio = <8>;
 		};
 
+		bridge_intc: bridge_intc at 20110 {
+			compatible = "marvell,orion-bridge-intc";
+			interrupt-controller;
+			#interrupt-cells = <1>;
+			interrupts = <0>;
+			reg = <0x20110 0x08>;
+		};
+
 		spi0: spi at 10600 {
 			compatible = "marvell,orion-spi";
 			#address-cells = <1>;

arch/arm/boot/dts/kirkwood.dtsi +12 -0

index 4e5b815..854e532 100644
@@ -19,6 +19,8 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 
+		/* The interrupt controller stanzas are in an
+		 * order that matches irqs.h */
 		gpio0: gpio at 10100 {
 			compatible = "marvell,orion-gpio";
 			#gpio-cells = <2>;

@@ -37,6 +39,14 @@
 			interrupts = <39>, <40>, <41>;
 		};
 
+		bridge_intc: bridge_intc at 20110 {
+			compatible = "marvell,orion-bridge-intc";
+			interrupt-controller;
+			#interrupt-cells = <1>;
+			interrupts = <1>;
+			reg = <0x20110 0x08>;
+		};
+
 		serial at 12000 {
 			compatible = "ns16550a";
 			reg = <0x12000 0x100>;

@@ -73,6 +83,8 @@
 
 		wdt at 20300 {
 			compatible = "marvell,orion-wdt";
+			interrupt-parent = <&bridge_intc>;
+			interrupts = <3>;
 			reg = <0x20300 0x28>;
 			status = "okay";
 		};

arch/arm/mach-dove/common.c +0 -4

index 6bad21b..b570211 100644
@@ -243,10 +243,6 @@ static int __init dove_find_tclk(void)
 static void __init dove_timer_init(void)
 {
 	dove_tclk = dove_find_tclk();
-	if (orion_bridge_irq_init(IRQ_DOVE_BRIDGE,
-				  IRQ_DOVE_BRIDGE_START,
-				  BRIDGE_CAUSE, NULL))
-		panic("Unable to setup bridge irqs");
 	orion_time_init(IRQ_DOVE_BRIDGE_TIMER1, dove_tclk);
 }
 

arch/arm/mach-dove/irq.c +5 -0

index bc4344a..bcb193f 100644
@@ -139,4 +139,9 @@ void __init dove_init_irq(void)
 		set_irq_flags(i, IRQF_VALID);
 	}
 	irq_set_chained_handler(IRQ_DOVE_PMU, pmu_irq_handler);
+
+	if (orion_bridge_irq_init(IRQ_DOVE_BRIDGE,
+				  IRQ_DOVE_BRIDGE_START,
+				  BRIDGE_CAUSE, NULL))
+		panic("Unable to setup bridge irqs");
 }

arch/arm/mach-kirkwood/common.c +0 -4

index ccd3ed5..7398f8b 100644
@@ -535,10 +535,6 @@ static void __init kirkwood_timer_init(void)
 {
 	kirkwood_tclk = kirkwood_find_tclk();
 
-	if (orion_bridge_irq_init(IRQ_KIRKWOOD_BRIDGE,
-				  IRQ_KIRKWOOD_BRIDGE_START,
-				  BRIDGE_CAUSE, NULL))
-		panic("Unable to setup bridge irqs");
 	orion_time_init(IRQ_KIRKWOOD_BRIDGE_TIMER1, kirkwood_tclk);
 }
 

arch/arm/mach-kirkwood/irq.c +5 -0

index 8847035..a1388ef 100644
@@ -41,4 +41,9 @@ void __init kirkwood_init_irq(void)
 			IRQ_KIRKWOOD_GPIO_START, gpio0_irqs);
 	orion_gpio_init(NULL, 32, 18, GPIO_HIGH_VIRT_BASE, 0,
 			IRQ_KIRKWOOD_GPIO_START + 32, gpio1_irqs);
+
+	if (orion_bridge_irq_init(IRQ_KIRKWOOD_BRIDGE,
+				  IRQ_KIRKWOOD_BRIDGE_START,
+				  BRIDGE_CAUSE, NULL))
+		panic("Unable to setup bridge irqs");
 }

arch/arm/plat-orion/irq.c +6 -0

index a903012..2dcccda 100644
@@ -131,9 +131,29 @@ static int __init orion_add_irq_domain(struct device_node *np,
 	return 0;
 }
 
+static int __init orion_add_bridge_irq_domain(
+	struct device_node *np, struct device_node *interrupt_parent)
+{
+	void __iomem *base;
+	int bridge_irq;
+

arch/arm/plat-orion/irq.c static int __init orion_add_irq_domain(struct device_node *np, · line 149

Quoted 7 lines

arch/arm/plat-orion/irq.c +7 -0

+	bridge_irq = irq_of_parse_and_map(np, 0);
+	/* FIXME: irq_of_parse_and_map returns 0 on error, but on Dove the
+	 * bridge IRQ is 0.
+	if (!bridge_irq)
+		return -ENODEV;*/
+
+	return orion_bridge_irq_init(bridge_irq, -1, base, np);

So does this mean for Dove it will currently always fail?

Quoted 9 lines

arch/arm/plat-orion/irq.c static int __init orion_add_irq_domain(struct device_node *np, · line 149

Quoted 7 lines

arch/arm/plat-orion/irq.c +7 -0

+	bridge_irq = irq_of_parse_and_map(np, 0);
+	/* FIXME: irq_of_parse_and_map returns 0 on error, but on Dove the
+	 * bridge IRQ is 0.
+	if (!bridge_irq)
+		return -ENODEV;*/
+
+	return orion_bridge_irq_init(bridge_irq, -1, base, np);

So does this mean for Dove it will currently always fail?

I guess that is hard to read without syntax hi-lighting, the 'if' is also commented out.

I was hopefull it would work as is, but looking deeper at irq_of_parse_and_map and its call tree makes me doubtfull now..

Fixing irq_of_parse_and_map is way to big a job for me :(

What to do from here? Add a dummy 0 interrupt to dove? Take dove out of this patch series?

Quoted 16 lines · Jason Gunthorpe

Quoted 9 lines

arch/arm/plat-orion/irq.c static int __init orion_add_irq_domain(struct device_node *np, · line 149

Quoted 7 lines

arch/arm/plat-orion/irq.c +7 -0

+	bridge_irq = irq_of_parse_and_map(np, 0);
+	/* FIXME: irq_of_parse_and_map returns 0 on error, but on Dove the
+	 * bridge IRQ is 0.
+	if (!bridge_irq)
+		return -ENODEV;*/
+
+	return orion_bridge_irq_init(bridge_irq, -1, base, np);

So does this mean for Dove it will currently always fail?

I guess that is hard to read without syntax hi-lighting, the 'if' is also commented out.

I was hopefull it would work as is, but looking deeper at irq_of_parse_and_map and its call tree makes me doubtfull now..

Quoted 1 line

Fixing irq_of_parse_and_map is way to big a job for me :(

Hi Jason

https://lwn.net/Articles/470820/

I don't think its fixable.

We need to see how other machine remap 0 to some other value and implement that for Dove.

Quoted 17 lines · Andrew Lunn

Quoted 16 lines · Jason Gunthorpe

Quoted 9 lines

arch/arm/plat-orion/irq.c static int __init orion_add_irq_domain(struct device_node *np, · line 149

Quoted 7 lines

arch/arm/plat-orion/irq.c +7 -0

+	bridge_irq = irq_of_parse_and_map(np, 0);
+	/* FIXME: irq_of_parse_and_map returns 0 on error, but on Dove the
+	 * bridge IRQ is 0.
+	if (!bridge_irq)
+		return -ENODEV;*/
+
+	return orion_bridge_irq_init(bridge_irq, -1, base, np);

So does this mean for Dove it will currently always fail?

I guess that is hard to read without syntax hi-lighting, the 'if' is also commented out.

I was hopefull it would work as is, but looking deeper at irq_of_parse_and_map and its call tree makes me doubtfull now..

Quoted 7 lines

Quoted 1 line

Fixing irq_of_parse_and_map is way to big a job for me :(

Hi Jason

https://lwn.net/Articles/470820/

I don't think its fixable.

Oh right. That.

Looking at this some more, I think it should work, but not for very good reasons.. irq_of_parse_and_map will return 0 on dove. It returns 0 because it is going through its error paths because it saw a 0 deep inside, but it still returns 0.

orion_bridge_irq_init will work with 0 as a bridge_irq, we know this because the timer code was using those code paths for a long time

Quoted 2 lines

We need to see how other machine remap 0 to some other value and implement that for Dove.

It looks like it is straightforward to adjust Dove's get_irqnr_and_base to add 1 to all the interrupt numbers, adjust irqs.h to add 1, and adjust the irq setup. There would be no performance penalty to doing this, but testing would be need to be sure 100% of changes were made. If the above works it would be better to do that sort of change after, or maybe after the non-DT code is purged...

Kirkwood is already OK WRT the 0 IRQ, it is handled internally to get_irqnr_and_base.

So, I will suggest to please test on dove as-is...

Sebastian,

Quoted 47 lines · Jason Gunthorpe

Quoted 17 lines · Andrew Lunn

Quoted 16 lines · Jason Gunthorpe

Quoted 9 lines

arch/arm/plat-orion/irq.c static int __init orion_add_irq_domain(struct device_node *np, · line 149

Quoted 7 lines

arch/arm/plat-orion/irq.c +7 -0

+	bridge_irq = irq_of_parse_and_map(np, 0);
+	/* FIXME: irq_of_parse_and_map returns 0 on error, but on Dove the
+	 * bridge IRQ is 0.
+	if (!bridge_irq)
+		return -ENODEV;*/
+
+	return orion_bridge_irq_init(bridge_irq, -1, base, np);

So does this mean for Dove it will currently always fail?

I guess that is hard to read without syntax hi-lighting, the 'if' is also commented out.

I was hopefull it would work as is, but looking deeper at irq_of_parse_and_map and its call tree makes me doubtfull now..

Quoted 7 lines

Quoted 1 line

Fixing irq_of_parse_and_map is way to big a job for me :(

Hi Jason

https://lwn.net/Articles/470820/

I don't think its fixable.

Oh right. That.

Looking at this some more, I think it should work, but not for very good reasons.. irq_of_parse_and_map will return 0 on dove. It returns 0 because it is going through its error paths because it saw a 0 deep inside, but it still returns 0.

orion_bridge_irq_init will work with 0 as a bridge_irq, we know this because the timer code was using those code paths for a long time

Quoted 2 lines

We need to see how other machine remap 0 to some other value and implement that for Dove.

It looks like it is straightforward to adjust Dove's get_irqnr_and_base to add 1 to all the interrupt numbers, adjust irqs.h to add 1, and adjust the irq setup. There would be no performance penalty to doing this, but testing would be need to be sure 100% of changes were made. If the above works it would be better to do that sort of change after, or maybe after the non-DT code is purged...

Quoted 4 lines

Kirkwood is already OK WRT the 0 IRQ, it is handled internally to get_irqnr_and_base.

So, I will suggest to please test on dove as-is...

Did you have a chance to test this on dove?

Quoted 55 lines · Jason Cooper

Sebastian,

Quoted 51 lines · Jason Gunthorpe

Quoted 17 lines · Andrew Lunn

Quoted 16 lines · Jason Gunthorpe

Quoted 9 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

+	bridge_irq = irq_of_parse_and_map(np, 0);
+	/* FIXME: irq_of_parse_and_map returns 0 on error, but on Dove the
+	 * bridge IRQ is 0.
+	if (!bridge_irq)
+		return -ENODEV;*/
+
+	return orion_bridge_irq_init(bridge_irq, -1, base, np);

So does this mean for Dove it will currently always fail?

I guess that is hard to read without syntax hi-lighting, the 'if' is also commented out.

I was hopefull it would work as is, but looking deeper at irq_of_parse_and_map and its call tree makes me doubtfull now..

Quoted 7 lines

Quoted 1 line

Fixing irq_of_parse_and_map is way to big a job for me :(

Hi Jason

https://lwn.net/Articles/470820/

I don't think its fixable.

Oh right. That.

Looking at this some more, I think it should work, but not for very good reasons.. irq_of_parse_and_map will return 0 on dove. It returns 0 because it is going through its error paths because it saw a 0 deep inside, but it still returns 0.

orion_bridge_irq_init will work with 0 as a bridge_irq, we know this because the timer code was using those code paths for a long time

Quoted 2 lines

We need to see how other machine remap 0 to some other value and implement that for Dove.

It looks like it is straightforward to adjust Dove's get_irqnr_and_base to add 1 to all the interrupt numbers, adjust irqs.h to add 1, and adjust the irq setup. There would be no performance penalty to doing this, but testing would be need to be sure 100% of changes were made. If the above works it would be better to do that sort of change after, or maybe after the non-DT code is purged...

Kirkwood is already OK WRT the 0 IRQ, it is handled internally to get_irqnr_and_base.

So, I will suggest to please test on dove as-is...

Quoted 1 line

Did you have a chance to test this on dove?

Hi Jason

We discussed this a little and I prefer Sebastians implementation.

Sebastian, do you have time to work on your code and get it ready?

Quoted 11 lines

Quoted 6 lines

Quoted 4 lines

Kirkwood is already OK WRT the 0 IRQ, it is handled internally to get_irqnr_and_base.

So, I will suggest to please test on dove as-is...

Did you have a chance to test this on dove?

Hi Jason

We discussed this a little and I prefer Sebastians implementation.

Quoted 1 line

Sebastian, do you have time to work on your code and get it ready?

Jason, Andrew,

we chose to jump to irqchip instead of fixing old code. I thought Jason Gunthorpe volunteered to get it ready? Moving to irqchip also requires clksrc-orion or reimplementing orion into clksrc-mvebu.

There is a bunch of prep stuff that may still be pending:
- Fixup remaining drivers (usb? ethernet?) to have DT bindings and get the IRQ number through DT

Jason,

Quoted 3 lines · Jason Gunthorpe

There is a bunch of prep stuff that may still be pending:
- Fixup remaining drivers (usb? ethernet?) to have DT bindings and get the IRQ number through DT

any chance I could get you post your changes to the ethernet driver (DT bindings)? iirc, you said you had based them off of Ian Molton's work. Even if you posted them RFC, it'd be a good place to start from, reignite conversation and so forth.

I've attached the main mv dt patch as a MIME, sorry for the awkwardness, I am travelling and won't be available... Please feel free to re-post/alter as necessary. As I said before, I'm not happy with it because it doesn't follow the standard convention for MDIO phys.

Dear Jason Gunthorpe,

Quoted 5 lines · Jason Gunthorpe

I've attached the main mv dt patch as a MIME, sorry for the awkwardness, I am travelling and won't be available... Please feel free to re-post/alter as necessary. As I said before, I'm not happy with it because it doesn't follow the standard convention for MDIO phys.

The MDIO registers for the mv643xx_eth hardware are the same as the ones for the mvneta hardware. Therefore, I've splitted this into the mvmdio driver (available in mainline since 3.8, drivers/net/ethernet/marvell). This allows to have a nicely separated driver for the MDIO interface.

My plan was ultimately to modify the mv643xx_eth driver to use the mvmdio driver, and then do a nice DT binding on top of that.

Does this sounds like a good plan?