Full DT IRQ support for versatile

Add a device tree binding for the FPGA SIC on versatile platforms. This also requires the addition of irq domain support for mapping to Linux IRQs.

Cc Grant Likely <[email protected]>

Cc Rob Herring <[email protected]>

Cc Russell King <[email protected]>

Signed-off-by Jamie Iles <[email protected]>

---
 .../devicetree/bindings/arm/versatile-sic.txt      |   27 ++++++++++
 arch/arm/plat-versatile/Kconfig                    |    1 +
 arch/arm/plat-versatile/fpga-irq.c                 |   51 +++++++++++++++++++-
 arch/arm/plat-versatile/include/plat/fpga-irq.h    |    6 ++
 4 files changed, 83 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/versatile-sic.txt

Documentation/devicetree/bindings/arm/versatile-sic.txt new file +27 -0

new file mode 100644
index 0000000..971a42e
@@ -0,0 +1,27 @@
+* ARM Versatile FPGA Secondary Interrupt Controller
+
+The SIC is an interrupt controller embedded in an FPGA found on ARM versatile
+platforms.
+
+Required properties:
+
+- compatible : "arm,versatile-sic"
+- interrupt-controller : Identifies the node as an interrupt controller
+- #interrupt-cells : The number of cells to define the interrupts.  Must be 1 as
+  the SIC has no configuration options for interrupt sources.  The cell is a u32
+  and defines the interrupt number.
+- reg : The register bank for the VIC.
+- interrupt-parent : The interrupt controller that IRQ's are cascaded to.
+- interrupts : Interrupt source for the interrupt that SIC IRQs are routed to
+  in the primary interrupt controller.
+
+Example:
+
+	sic: intc at 10003000 {
+		compatible = "arm,versatile-sic";
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		reg = <0x10003000 0x1000>;
+		interrupt-parent = <&vic>;
+		interrupts = <31>;
+	};

arch/arm/plat-versatile/Kconfig +0 -0

index 52353be..a07ffe5 100644

arch/arm/plat-versatile/Kconfig +0 -0

 	bool
 
 config PLAT_VERSATILE_LEDS

arch/arm/plat-versatile/fpga-irq.c +7 -1

index f0cc8e1..fd87b06 100644
@@ -2,7 +2,13 @@
  *  Support for Versatile FPGA-based IRQ controllers
  */
 #include <linux/irq.h>
+#include <linux/irqdomain.h>
 #include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/slab.h>
 
 #include <asm/mach/irq.h>
 #include <plat/fpga-irq.h>

@@ -15,7 +21,7 @@
 static void fpga_irq_mask(struct irq_data *d)
 {
 	struct fpga_irq_data *f = irq_data_get_irq_chip_data(d);
-	u32 mask = 1 << (d->irq - f->irq_start);
+	u32 mask = 1 << d->hwirq;
 
 	writel(mask, f->base + IRQ_ENABLE_CLEAR);
 }

arch/arm/plat-versatile/fpga-irq.c +4 -0

@@ -53,6 +59,10 @@ void __init fpga_irq_init(int parent_irq, u32 valid, struct fpga_irq_data *f)
 	f->chip.irq_ack = fpga_irq_mask;
 	f->chip.irq_mask = fpga_irq_mask;
 	f->chip.irq_unmask = fpga_irq_unmask;
+	f->domain.irq_base = f->irq_start;
+	f->domain.nr_irq = 32;
+	f->domain.ops = &irq_domain_simple_ops;
+	irq_domain_add(&f->domain);
 
 	if (parent_irq != -1) {
 		irq_set_handler_data(parent_irq, f);

arch/arm/plat-versatile/fpga-irq.c +37 -0

+
+#ifdef CONFIG_OF
+int __init sic_of_init(struct device_node *np, struct device_node *parent)
+{
+	struct fpga_irq_data *sic_data = kzalloc(sizeof(*sic_data), GFP_KERNEL);
+	int err = -ENOMEM, irq;
+
+	if (WARN_ON(!sic_data))
+		return err;
+
+	sic_data->base = of_iomap(np, 0);
+	if (WARN_ON(!sic_data->base))
+		goto out_free_data;
+
+	irq = irq_of_parse_and_map(np, 0);
+	if (irq < 0)
+		goto out_free_data;
+
+	sic_data->irq_start = irq_alloc_descs(-1, 0, 32, numa_node_id());
+	if (WARN_ON(sic_data->irq_start < 0)) {
+		err = sic_data->irq_start;
+		goto out_unmap;
+	}
+	sic_data->domain.of_node = of_node_get(np);
+
+	fpga_irq_init(irq, ~0, sic_data);
+
+	return 0;
+
+out_unmap:
+	iounmap(sic_data->base);
+out_free_data:
+	kfree(sic_data);
+
+	return err;
+}
+#endif /* CONFIG_OF */

arch/arm/plat-versatile/include/plat/fpga-irq.h +6 -0

index 627fafd..a1391f3 100644
@@ -1,12 +1,18 @@
 #ifndef PLAT_FPGA_IRQ_H
 #define PLAT_FPGA_IRQ_H
 
+#include <linux/irqdomain.h>
+
+struct device_node;
+
 struct fpga_irq_data {
 	void __iomem *base;
 	unsigned int irq_start;
 	struct irq_chip chip;
+	struct irq_domain domain;
 };
 
 void fpga_irq_init(int, u32, struct fpga_irq_data *);
+int sic_of_init(struct device_node *np, struct device_node *parent);
 
 #endif

Signature · 1 line

1.7.5.4

Quoted 10 lines · Jamie Iles

(quoted diff) +4 -0

@@ -53,6 +59,10 @@ void __init fpga_irq_init(int parent_irq, u32 
 valid, struct fpga_irq_data *f)
 	f->chip.irq_ack = fpga_irq_mask;
 	f->chip.irq_mask = fpga_irq_mask;
 	f->chip.irq_unmask = fpga_irq_unmask;
+	f->domain.irq_base = f->irq_start;
+	f->domain.nr_irq = 32;
+	f->domain.ops = &irq_domain_simple_ops;
+	irq_domain_add(&f->domain);
 

Review void __init fpga_irq_init(int parent_irq, u32 valid, struct fpga_irq_data *f) · line 82

Quoted 6 lines

(quoted diff) +0 -0

 	if (parent_irq != -1) {

(quoted diff) +0 -0

 		irq_set_handler_data(parent_irq, f);

@@ -70,3 +80,40 @@ void __init fpga_irq_init(int parent_irq, u32 valid, struct fpga_irq_data *f)
 		}
 	}
 }

There is a loop in fpga_irq_init() that sets up all the irqs, but with the introduction of the full irq_domain, that can be moved out to the .map irq_domain ops function. The irq_domain will call it to set up each irq when it is requested.

Quoted 11 lines · Grant Likely

Quoted 10 lines · Jamie Iles

(quoted diff) +4 -0

@@ -53,6 +59,10 @@ void __init fpga_irq_init(int parent_irq, u32 
 valid, struct fpga_irq_data *f)
 	f->chip.irq_ack = fpga_irq_mask;
 	f->chip.irq_mask = fpga_irq_mask;
 	f->chip.irq_unmask = fpga_irq_unmask;
+	f->domain.irq_base = f->irq_start;
+	f->domain.nr_irq = 32;
+	f->domain.ops = &irq_domain_simple_ops;
+	irq_domain_add(&f->domain);
 

Quoted 11 lines

Review void __init fpga_irq_init(int parent_irq, u32 valid, struct fpga_irq_data *f) · line 82

Quoted 6 lines

(quoted diff) +0 -0

 	if (parent_irq != -1) {

(quoted diff) +0 -0

 		irq_set_handler_data(parent_irq, f);

@@ -70,3 +80,40 @@ void __init fpga_irq_init(int parent_irq, u32 valid, struct fpga_irq_data *f)
 		}
 	}
 }

There is a loop in fpga_irq_init() that sets up all the irqs, but with the introduction of the full irq_domain, that can be moved out to the .map irq_domain ops function. The irq_domain will call it to set up each irq when it is requested.

That's not quite working for me - I have the following .map() function:

static int sic_irq_map(struct irq_domain *d, unsigned int virq,
irq_hw_number_t hw)

{
	struct fpga_irq_data *f = d->host_data;

	irq_set_chip_data(virq, f);
	irq_set_chip_and_handler(virq, &f->chip, handle_level_irq);
	set_irq_flags(virq, IRQF_VALID | IRQF_PROBE);

	return 0;
}

but when irq_domain_add_legacy() iterates over the irq's I haven't had chance to set the host_data. The patch below works for me, not sure what you think.

Let me look at it a bit. You can leave the initialization loop in fpga_irq_init() for now. I'm looking at adding a mask field to irq_domain_add_legacy in addition to the host_data pointer. I also want to investigate replacing fpga_irq with generic-chip.c. The problem I've currently got though is that irq_domain_add_legacy() can currently support any number of irqs, but adding a simple mask argument will limit it to 32. That won't work for large irq controllers like the gic.

arch/arm/plat-versatile/fpga-irq.c +15 -0

+#ifdef CONFIG_OF
+int __init sic_of_init(struct device_node *np, struct device_node *parent)
+{
+	struct fpga_irq_data *sic_data = kzalloc(sizeof(*sic_data), GFP_KERNEL);
+	int err = -ENOMEM, irq;
+
+	if (WARN_ON(!sic_data))
+		return err;
+
+	sic_data->base = of_iomap(np, 0);
+	if (WARN_ON(!sic_data->base))
+		goto out_free_data;
+
+	irq = irq_of_parse_and_map(np, 0);
+	if (irq < 0)

Quoted 21 lines · Jamie Iles

arch/arm/plat-versatile/fpga-irq.c +21 -0

+#ifdef CONFIG_OF
+int __init sic_of_init(struct device_node *np, struct device_node *parent)
+{
+	struct fpga_irq_data *sic_data = kzalloc(sizeof(*sic_data), GFP_KERNEL);
+	int err = -ENOMEM, irq;
+
+	if (WARN_ON(!sic_data))
+		return err;
+
+	sic_data->base = of_iomap(np, 0);
+	if (WARN_ON(!sic_data->base))
+		goto out_free_data;
+
+	irq = irq_of_parse_and_map(np, 0);
+	if (irq < 0)
+		goto out_free_data;
+
+	sic_data->irq_start = irq_alloc_descs(-1, 0, 32, numa_node_id());
+	if (WARN_ON(sic_data->irq_start < 0)) {
+		err = sic_data->irq_start;
+		goto out_unmap;

arch/arm/plat-versatile/fpga-irq.c void __init fpga_irq_init(int parent_irq, u32 valid, struct fpga_irq_data *f) · line 108

Quoted 4 lines

arch/arm/plat-versatile/fpga-irq.c +4 -0

+	}
+	sic_data->domain.of_node = of_node_get(np);
+
+	fpga_irq_init(irq, ~0, sic_data);

I notice that you completely ignore the validity mask, and initialize all 32 interrupts. This is wrong. You don't think I put the validity mask there just to obfuscate the code?

And this shouldn't be called 'sic' at all. It may be the secondary interrupt controller on the Versatile, but it's also the PIC, SIC and CIC on Integrator platforms. Calling anything in this file (which is entirely separate of any platform) 'sic' is _wrong_.

Quoted 22 lines · Russell King - ARM Linux

Quoted 21 lines · Jamie Iles

arch/arm/plat-versatile/fpga-irq.c +21 -0

+#ifdef CONFIG_OF
+int __init sic_of_init(struct device_node *np, struct device_node *parent)
+{
+	struct fpga_irq_data *sic_data = kzalloc(sizeof(*sic_data), GFP_KERNEL);
+	int err = -ENOMEM, irq;
+
+	if (WARN_ON(!sic_data))
+		return err;
+
+	sic_data->base = of_iomap(np, 0);
+	if (WARN_ON(!sic_data->base))
+		goto out_free_data;
+
+	irq = irq_of_parse_and_map(np, 0);
+	if (irq < 0)
+		goto out_free_data;
+
+	sic_data->irq_start = irq_alloc_descs(-1, 0, 32, numa_node_id());
+	if (WARN_ON(sic_data->irq_start < 0)) {
+		err = sic_data->irq_start;
+		goto out_unmap;

Quoted 8 lines

arch/arm/plat-versatile/fpga-irq.c void __init fpga_irq_init(int parent_irq, u32 valid, struct fpga_irq_data *f) · line 108

Quoted 4 lines

arch/arm/plat-versatile/fpga-irq.c +4 -0

+	}
+	sic_data->domain.of_node = of_node_get(np);
+
+	fpga_irq_init(irq, ~0, sic_data);

I notice that you completely ignore the validity mask, and initialize all 32 interrupts. This is wrong. You don't think I put the validity mask there just to obfuscate the code?

OK, that is wrong. The validity mask is used for the non-DT platforms, for the DT one I'll add a property stating the valid mask:

	arm,valid-irq-mask = <0x001fffff>;

to the node in the trees.

Quoted 35 lines · Jamie Iles

Quoted 22 lines · Russell King - ARM Linux

Quoted 21 lines · Jamie Iles

arch/arm/plat-versatile/fpga-irq.c +21 -0

+#ifdef CONFIG_OF
+int __init sic_of_init(struct device_node *np, struct device_node *parent)
+{
+	struct fpga_irq_data *sic_data = kzalloc(sizeof(*sic_data), GFP_KERNEL);
+	int err = -ENOMEM, irq;
+
+	if (WARN_ON(!sic_data))
+		return err;
+
+	sic_data->base = of_iomap(np, 0);
+	if (WARN_ON(!sic_data->base))
+		goto out_free_data;
+
+	irq = irq_of_parse_and_map(np, 0);
+	if (irq < 0)
+		goto out_free_data;
+
+	sic_data->irq_start = irq_alloc_descs(-1, 0, 32, numa_node_id());
+	if (WARN_ON(sic_data->irq_start < 0)) {
+		err = sic_data->irq_start;
+		goto out_unmap;

Quoted 8 lines

arch/arm/plat-versatile/fpga-irq.c void __init fpga_irq_init(int parent_irq, u32 valid, struct fpga_irq_data *f) · line 108

Quoted 4 lines

arch/arm/plat-versatile/fpga-irq.c +4 -0

+	}
+	sic_data->domain.of_node = of_node_get(np);
+
+	fpga_irq_init(irq, ~0, sic_data);

I notice that you completely ignore the validity mask, and initialize all 32 interrupts. This is wrong. You don't think I put the validity mask there just to obfuscate the code?

OK, that is wrong. The validity mask is used for the non-DT platforms, for the DT one I'll add a property stating the valid mask:

Quoted 3 lines

	arm,valid-irq-mask = <0x001fffff>;

to the node in the trees.

Shouldn't be necessary when the driver is converted to using the linear mapping. When that happens, only the requested irqs will get mapped.

These patches move versatile over to instantiating both interrupt controllers from the device tree. ?I've tested this in qemu for both DT and non-DT platforms but only with an initramfs. ?I've seen interrupts generated on both controllers for both boot methods.

Grant, Rob, this makes use of irqdomain again as I'm not sure what the replacement is going to look like yet. ?versatile works with DT in mainline, but that's kind of lucky... ?I know there's been patches to make irq_domain_add_simple() return the irq_domain and can rebase if needed.

Jamie Iles (2):
?versatile: sic: add device tree bindings ?versatile: dt: register interrupt controllers from dt

?.../devicetree/bindings/arm/versatile-sic.txt ? ? ?| ? 27 ++++++++++
?arch/arm/mach-versatile/core.c ? ? ? ? ? ? ? ? ? ? | ? 27 +++-------
?arch/arm/mach-versatile/core.h ? ? ? ? ? ? ? ? ? ? | ? ?1 +
?arch/arm/mach-versatile/versatile_dt.c ? ? ? ? ? ? | ? 15 +++++-
?arch/arm/plat-versatile/Kconfig ? ? ? ? ? ? ? ? ? ?| ? ?1 +
?arch/arm/plat-versatile/fpga-irq.c ? ? ? ? ? ? ? ? | ? 51 +++++++++++++++++++-
?arch/arm/plat-versatile/include/plat/fpga-irq.h ? ?| ? ?6 ++

?7 files changed, 106 insertions(+), 22 deletions(-)
?create mode 100644 Documentation/devicetree/bindings/arm/versatile-sic.txt

--
1.7.5.4

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

Quoted 10 lines · Jamie Iles

These patches move versatile over to instantiating both interrupt controllers from the device tree. ?I've tested this in qemu for both DT and non-DT platforms but only with an initramfs. ?I've seen interrupts generated on both controllers for both boot methods.

Grant, Rob, this makes use of irqdomain again as I'm not sure what the replacement is going to look like yet. ?versatile works with DT in mainline, but that's kind of lucky... ?I know there's been patches to make irq_domain_add_simple() return the irq_domain and can rebase if needed.

I posted the patches for finishing up irq_domain migration yesterday. irq_domain_add_simple() can trivially be changed to irq_domain_add_legacy. I'll try out these
patches on top of my irqdomain/next branch

Quoted 12 lines · Grant Likely

Quoted 10 lines · Jamie Iles

These patches move versatile over to instantiating both interrupt controllers from the device tree. ?I've tested this in qemu for both DT and non-DT platforms but only with an initramfs. ?I've seen interrupts generated on both controllers for both boot methods.

Grant, Rob, this makes use of irqdomain again as I'm not sure what the replacement is going to look like yet. ?versatile works with DT in mainline, but that's kind of lucky... ?I know there's been patches to make irq_domain_add_simple() return the irq_domain and can rebase if needed.

Quoted 4 lines

I posted the patches for finishing up irq_domain migration yesterday. irq_domain_add_simple() can trivially be changed to irq_domain_add_legacy. I'll try out these
patches on top of my irqdomain/next branch

Excellent! I've just grabbed your branch and given it a spin. I fixed up my patches with the patch below and had to revert "irqdesc:
Consolidate irq reservation logic" as I saw Rob mentioned as I was getting negative irq_desc's allocated but otherwise it works nicely!

I've also tested your tree with the irqdesc patch reverted on picoxcell and that's working nicely too!

Jamie

8<----

arch/arm/plat-versatile/fpga-irq.c +3 -4

index fd87b06..c4dac93 100644
@@ -59,10 +59,9 @@ void __init fpga_irq_init(int parent_irq, u32 valid, struct fpga_irq_data *f)
 	f->chip.irq_ack = fpga_irq_mask;
 	f->chip.irq_mask = fpga_irq_mask;
 	f->chip.irq_unmask = fpga_irq_unmask;
-	f->domain.irq_base = f->irq_start;
-	f->domain.nr_irq = 32;
-	f->domain.ops = &irq_domain_simple_ops;
-	irq_domain_add(&f->domain);
+	if (!f->domain)
+		f->domain = irq_domain_add_legacy(NULL, 32, f->irq_start,
+						  0, &irq_domain_simple_ops);
 
 	if (parent_irq != -1) {
 		irq_set_handler_data(parent_irq, f);