[PATCH 0/2] Full DT IRQ support for versatile

[PATCH 0/2] Full DT IRQ support for versatile

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

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

[PATCH 1/2] versatile: sic: add device tree bindings

[Jamie Iles]

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 <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
Signed-off-by: Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>
---
 .../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

diff --git a/Documentation/devicetree/bindings/arm/versatile-sic.txt b/Documentation/devicetree/bindings/arm/versatile-sic.txt
new file mode 100644
index 0000000..971a42e
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/versatile-sic.txt
@@ -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@10003000 {
+		compatible = "arm,versatile-sic";
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		reg = <0x10003000 0x1000>;
+		interrupt-parent = <&vic>;
+		interrupts = <31>;
+	};
diff --git a/arch/arm/plat-versatile/Kconfig b/arch/arm/plat-versatile/Kconfig
index 52353be..a07ffe5 100644
--- a/arch/arm/plat-versatile/Kconfig
+++ b/arch/arm/plat-versatile/Kconfig
@@ -4,6 +4,7 @@ config PLAT_VERSATILE_CLCD
 	bool
 
 config PLAT_VERSATILE_FPGA_IRQ
+	select IRQ_DOMAIN
 	bool
 
 config PLAT_VERSATILE_LEDS
diff --git a/arch/arm/plat-versatile/fpga-irq.c b/arch/arm/plat-versatile/fpga-irq.c
index f0cc8e1..fd87b06 100644
--- a/arch/arm/plat-versatile/fpga-irq.c
+++ b/arch/arm/plat-versatile/fpga-irq.c
@@ -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);
 }
@@ -23,7 +29,7 @@ static void fpga_irq_mask(struct irq_data *d)
 static void fpga_irq_unmask(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_SET);
 }
@@ -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);
@@ -70,3 +80,40 @@ void __init fpga_irq_init(int parent_irq, u32 valid, struct fpga_irq_data *f)
 		}
 	}
 }
+
+#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 */
diff --git a/arch/arm/plat-versatile/include/plat/fpga-irq.h b/arch/arm/plat-versatile/include/plat/fpga-irq.h
index 627fafd..a1391f3 100644
--- a/arch/arm/plat-versatile/include/plat/fpga-irq.h
+++ b/arch/arm/plat-versatile/include/plat/fpga-irq.h
@@ -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
-- 
1.7.5.4

Re: [PATCH 1/2] versatile: sic: add device tree bindings

[Grant Likely]

On Fri, Jan 13, 2012 at 12:44:00AM +0000, Jamie Iles wrote:
> 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 <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> Signed-off-by: Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>
> ---
>  .../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
> 
> diff --git a/Documentation/devicetree/bindings/arm/versatile-sic.txt b/Documentation/devicetree/bindings/arm/versatile-sic.txt
> new file mode 100644
> index 0000000..971a42e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/versatile-sic.txt
> @@ -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@10003000 {
> +		compatible = "arm,versatile-sic";
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +		reg = <0x10003000 0x1000>;
> +		interrupt-parent = <&vic>;
> +		interrupts = <31>;
> +	};
> diff --git a/arch/arm/plat-versatile/Kconfig b/arch/arm/plat-versatile/Kconfig
> index 52353be..a07ffe5 100644
> --- a/arch/arm/plat-versatile/Kconfig
> +++ b/arch/arm/plat-versatile/Kconfig
> @@ -4,6 +4,7 @@ config PLAT_VERSATILE_CLCD
>  	bool
>  
>  config PLAT_VERSATILE_FPGA_IRQ
> +	select IRQ_DOMAIN

My plan is to select this from CONFIG_ARM, and that is what is in the
irqdomain/next branch.

>  	bool
>  
>  config PLAT_VERSATILE_LEDS
> diff --git a/arch/arm/plat-versatile/fpga-irq.c b/arch/arm/plat-versatile/fpga-irq.c
> index f0cc8e1..fd87b06 100644
> --- a/arch/arm/plat-versatile/fpga-irq.c
> +++ b/arch/arm/plat-versatile/fpga-irq.c
> @@ -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);
>  }
> @@ -23,7 +29,7 @@ static void fpga_irq_mask(struct irq_data *d)
>  static void fpga_irq_unmask(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_SET);
>  }

fpga_irq_handle() also needs to be modified to call irq_find_mapping()
to translate from the hwirq to the linux number.  There should be no
references to irq_start after migrating to use irq_domain.

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

> +
> +#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 */
> diff --git a/arch/arm/plat-versatile/include/plat/fpga-irq.h b/arch/arm/plat-versatile/include/plat/fpga-irq.h
> index 627fafd..a1391f3 100644
> --- a/arch/arm/plat-versatile/include/plat/fpga-irq.h
> +++ b/arch/arm/plat-versatile/include/plat/fpga-irq.h
> @@ -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
> -- 
> 1.7.5.4
> 

Re: [PATCH 1/2] versatile: sic: add device tree bindings

[Jamie Iles]

On Thu, Jan 12, 2012 at 07:29:32PM -0700, Grant Likely wrote:
> On Fri, Jan 13, 2012 at 12:44:00AM +0000, Jamie Iles wrote:
> > @@ -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);
> > @@ -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.

I've also exported irq_domain_simple_xlate() so I can use that for the 
sic ops, I'll post that patch later.

Jamie

8<----

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 92928e2..e948490 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -121,11 +121,23 @@ struct irq_domain {
 };
 
 #ifdef CONFIG_IRQ_DOMAIN
-struct irq_domain *irq_domain_add_legacy(struct device_node *of_node,
-					 unsigned int size,
-					 unsigned int first_irq,
-					 irq_hw_number_t first_hwirq,
-					 struct irq_domain_ops *ops);
+struct irq_domain *irq_domain_add_legacy_data(struct device_node *of_node,
+					      unsigned int size,
+					      unsigned int first_irq,
+					      irq_hw_number_t first_hwirq,
+					      struct irq_domain_ops *ops,
+					      void *host_data);
+static inline struct irq_domain *
+irq_domain_add_legacy(struct device_node *of_node,
+		      unsigned int size,
+		      unsigned int first_irq,
+		      irq_hw_number_t first_hwirq,
+		      struct irq_domain_ops *ops)
+{
+	return irq_domain_add_legacy_data(of_node, size, first_irq,
+					  first_hwirq, ops, NULL);
+}
+
 struct irq_domain *irq_domain_add_linear(struct device_node *of_node,
 					 unsigned int size,
 					 struct irq_domain_ops *ops);
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 67b28f6..2fd00a4 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -75,11 +75,12 @@ static struct irq_domain *irq_domain_add(struct device_node *of_node,
  * for all legacy interrupts except 0 (which is always the invalid irq for
  * a legacy controller).
  */
-struct irq_domain *irq_domain_add_legacy(struct device_node *of_node,
-					 unsigned int size,
-					 unsigned int first_irq,
-					 irq_hw_number_t first_hwirq,
-					 struct irq_domain_ops *ops)
+struct irq_domain *irq_domain_add_legacy_data(struct device_node *of_node,
+					      unsigned int size,
+					      unsigned int first_irq,
+					      irq_hw_number_t first_hwirq,
+					      struct irq_domain_ops *ops,
+					      void *host_data)
 {
 	struct irq_domain *domain;
 	unsigned int i;
@@ -87,6 +88,7 @@ struct irq_domain *irq_domain_add_legacy(struct device_node *of_node,
 	domain = irq_domain_alloc(of_node, IRQ_DOMAIN_MAP_LEGACY, ops);
 	if (!domain)
 		return NULL;
+	domain->host_data = host_data;
 
 	mutex_lock(&irq_domain_mutex);
 	/* Verify that all the irqs are available */

Re: [PATCH 1/2] versatile: sic: add device tree bindings

[Grant Likely]

On Fri, Jan 13, 2012 at 10:35:54AM +0000, Jamie Iles wrote:
> On Thu, Jan 12, 2012 at 07:29:32PM -0700, Grant Likely wrote:
> > On Fri, Jan 13, 2012 at 12:44:00AM +0000, Jamie Iles wrote:
> > > @@ -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);
> > > @@ -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.

g.

Re: [PATCH 1/2] versatile: sic: add device tree bindings

[Russell King - ARM Linux]

On Fri, Jan 13, 2012 at 02:00:41PM -0700, Grant Likely wrote:
> 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.

The easy answer is not to register the IRQs which don't actually exist
or we don't want to be used out of the set of 32.

However, the problem comes with:

#define PIC_MASK        0xFFD00000

where we have an IRQ23 but no IRQ22.

Re: [PATCH 1/2] versatile: sic: add device tree bindings

[Russell King - ARM Linux]

On Fri, Jan 13, 2012 at 12:44:00AM +0000, Jamie Iles wrote:
> +#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);

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

Re: [PATCH 1/2] versatile: sic: add device tree bindings

[Jamie Iles]

On Fri, Jan 13, 2012 at 10:48:42AM +0000, Russell King - ARM Linux wrote:
> On Fri, Jan 13, 2012 at 12:44:00AM +0000, Jamie Iles wrote:
> > +#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);
> 
> 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.

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

OK, I didn't know that.  So fpga-irq is an appropriate name (is it 
always an FPGA?) and arm,versatile-fpga-irq OK for a compatible string?

Jamie

Re: [PATCH 1/2] versatile: sic: add device tree bindings

[Grant Likely]

On Fri, Jan 13, 2012 at 10:58:34AM +0000, Jamie Iles wrote:
> On Fri, Jan 13, 2012 at 10:48:42AM +0000, Russell King - ARM Linux wrote:
> > On Fri, Jan 13, 2012 at 12:44:00AM +0000, Jamie Iles wrote:
> > > +#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);
> > 
> > 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.

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

Re: [PATCH 1/2] versatile: sic: add device tree bindings

[Russell King - ARM Linux]

On Fri, Jan 13, 2012 at 11:41:51AM -0700, Grant Likely wrote:
> On Fri, Jan 13, 2012 at 10:58:34AM +0000, Jamie Iles wrote:
> > On Fri, Jan 13, 2012 at 10:48:42AM +0000, Russell King - ARM Linux wrote:
> > > On Fri, Jan 13, 2012 at 12:44:00AM +0000, Jamie Iles wrote:
> > > > +#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);
> > > 
> > > 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.
> 
> Shouldn't be necessary when the driver is converted to using the
> linear mapping.  When that happens, only the requested irqs will get
> mapped.

What I'm saying is that they shouldn't even be _published_ as
potentially valid IRQs.

[PATCH 2/2] versatile: dt: register interrupt controllers from dt

[Jamie Iles]

Switch the versatile_dt board to instantiate both interrupt controllers
from the device tree using the of_irq_init() helper.

Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
Signed-off-by: Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>
---
 arch/arm/mach-versatile/core.c         |   27 ++++++++-------------------
 arch/arm/mach-versatile/core.h         |    1 +
 arch/arm/mach-versatile/versatile_dt.c |   15 ++++++++++++++-
 3 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/arch/arm/mach-versatile/core.c b/arch/arm/mach-versatile/core.c
index cbcda61..1ad2447 100644
--- a/arch/arm/mach-versatile/core.c
+++ b/arch/arm/mach-versatile/core.c
@@ -86,27 +86,9 @@ static struct fpga_irq_data sic_irq = {
 #define PIC_MASK	0
 #endif
 
-/* Lookup table for finding a DT node that represents the vic instance */
-static const struct of_device_id vic_of_match[] __initconst = {
-	{ .compatible = "arm,versatile-vic", },
-	{}
-};
-
-static const struct of_device_id sic_of_match[] __initconst = {
-	{ .compatible = "arm,versatile-sic", },
-	{}
-};
-
-void __init versatile_init_irq(void)
+void __init versatile_route_irqs(void)
 {
-	vic_init(VA_VIC_BASE, IRQ_VIC_START, ~0, 0);
-	irq_domain_generate_simple(vic_of_match, VERSATILE_VIC_BASE, IRQ_VIC_START);
-
 	writel(~0, VA_SIC_BASE + SIC_IRQ_ENABLE_CLEAR);
-
-	fpga_irq_init(IRQ_VICSOURCE31, ~PIC_MASK, &sic_irq);
-	irq_domain_generate_simple(sic_of_match, VERSATILE_SIC_BASE, IRQ_SIC_START);
-
 	/*
 	 * Interrupts on secondary controller from 0 to 8 are routed to
 	 * source 31 on PIC.
@@ -117,6 +99,13 @@ void __init versatile_init_irq(void)
 	writel(PIC_MASK, VA_SIC_BASE + SIC_INT_PIC_ENABLE);
 }
 
+void __init versatile_init_irq(void)
+{
+	versatile_route_irqs();
+	vic_init(VA_VIC_BASE, IRQ_VIC_START, ~0, 0);
+	fpga_irq_init(IRQ_VICSOURCE31, ~PIC_MASK, &sic_irq);
+}
+
 static struct map_desc versatile_io_desc[] __initdata = {
 	{
 		.virtual	=  IO_ADDRESS(VERSATILE_SYS_BASE),
diff --git a/arch/arm/mach-versatile/core.h b/arch/arm/mach-versatile/core.h
index 2ef2f55..1117e32 100644
--- a/arch/arm/mach-versatile/core.h
+++ b/arch/arm/mach-versatile/core.h
@@ -29,6 +29,7 @@ extern void __init versatile_init(void);
 extern void __init versatile_init_early(void);
 extern void __init versatile_init_irq(void);
 extern void __init versatile_map_io(void);
+extern void __init versatile_route_irqs(void);
 extern struct sys_timer versatile_timer;
 extern void versatile_restart(char, const char *);
 extern unsigned int mmc_status(struct device *dev);
diff --git a/arch/arm/mach-versatile/versatile_dt.c b/arch/arm/mach-versatile/versatile_dt.c
index ae5ad3c..abb310d 100644
--- a/arch/arm/mach-versatile/versatile_dt.c
+++ b/arch/arm/mach-versatile/versatile_dt.c
@@ -27,6 +27,7 @@
 #include <asm/hardware/vic.h>
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
+#include <plat/fpga-irq.h>
 
 #include "core.h"
 
@@ -36,6 +37,18 @@ static void __init versatile_dt_init(void)
 			     versatile_auxdata_lookup, NULL);
 }
 
+static const struct of_device_id versatile_intc_of_match[] = {
+	{ .compatible = "arm,versatile-vic", .data = vic_of_init, },
+	{ .compatible = "arm,versatile-sic", .data = sic_of_init, },
+	{ /* Sentinel */ }
+};
+
+static void __init versatile_dt_init_irq(void)
+{
+	versatile_route_irqs();
+	of_irq_init(versatile_intc_of_match);
+}
+
 static const char *versatile_dt_match[] __initconst = {
 	"arm,versatile-ab",
 	"arm,versatile-pb",
@@ -45,7 +58,7 @@ static const char *versatile_dt_match[] __initconst = {
 DT_MACHINE_START(VERSATILE_PB, "ARM-Versatile (Device Tree Support)")
 	.map_io		= versatile_map_io,
 	.init_early	= versatile_init_early,
-	.init_irq	= versatile_init_irq,
+	.init_irq	= versatile_dt_init_irq,
 	.handle_irq	= vic_handle_irq,
 	.timer		= &versatile_timer,
 	.init_machine	= versatile_dt_init,
-- 
1.7.5.4

Re: [PATCH 0/2] Full DT IRQ support for versatile

[Grant Likely]

On Thu, Jan 12, 2012 at 5:43 PM, Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org> wrote:
> 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

g.

>
> 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-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 0/2] Full DT IRQ support for versatile

[Jamie Iles]

On Thu, Jan 12, 2012 at 06:20:37PM -0700, Grant Likely wrote:
> On Thu, Jan 12, 2012 at 5:43 PM, Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org> wrote:
> > 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

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

diff --git a/arch/arm/plat-versatile/fpga-irq.c b/arch/arm/plat-versatile/fpga-irq.c
index fd87b06..c4dac93 100644
--- a/arch/arm/plat-versatile/fpga-irq.c
+++ b/arch/arm/plat-versatile/fpga-irq.c
@@ -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);
@@ -103,7 +102,8 @@ int __init sic_of_init(struct device_node *np, struct device_node *parent)
 		err = sic_data->irq_start;
 		goto out_unmap;
 	}
-	sic_data->domain.of_node = of_node_get(np);
+	sic_data->domain = irq_domain_add_legacy(np, 32, sic_data->irq_start,
+						 0, &irq_domain_simple_ops);
 
 	fpga_irq_init(irq, ~0, sic_data);
 
diff --git a/arch/arm/plat-versatile/include/plat/fpga-irq.h b/arch/arm/plat-versatile/include/plat/fpga-irq.h
index a1391f3..5e300e9 100644
--- a/arch/arm/plat-versatile/include/plat/fpga-irq.h
+++ b/arch/arm/plat-versatile/include/plat/fpga-irq.h
@@ -1,15 +1,14 @@
 #ifndef PLAT_FPGA_IRQ_H
 #define PLAT_FPGA_IRQ_H
 
-#include <linux/irqdomain.h>
-
 struct device_node;
+struct irq_domain;
 
 struct fpga_irq_data {
 	void __iomem *base;
 	unsigned int irq_start;
 	struct irq_chip chip;
-	struct irq_domain domain;
+	struct irq_domain *domain;
 };
 
 void fpga_irq_init(int, u32, struct fpga_irq_data *);

Re: [PATCH 0/2] Full DT IRQ support for versatile

[Grant Likely]

On Fri, Jan 13, 2012 at 01:42:50AM +0000, Jamie Iles wrote:
> On Thu, Jan 12, 2012 at 06:20:37PM -0700, Grant Likely wrote:
> > On Thu, Jan 12, 2012 at 5:43 PM, Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org> wrote:
> > > 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
> 
> 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<----
> 
> diff --git a/arch/arm/plat-versatile/fpga-irq.c b/arch/arm/plat-versatile/fpga-irq.c
> index fd87b06..c4dac93 100644
> --- a/arch/arm/plat-versatile/fpga-irq.c
> +++ b/arch/arm/plat-versatile/fpga-irq.c
> @@ -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);
> @@ -103,7 +102,8 @@ int __init sic_of_init(struct device_node *np, struct device_node *parent)
>  		err = sic_data->irq_start;
>  		goto out_unmap;
>  	}
> -	sic_data->domain.of_node = of_node_get(np);
> +	sic_data->domain = irq_domain_add_legacy(np, 32, sic_data->irq_start,
> +						 0, &irq_domain_simple_ops);

For the DT use-case, you should try irq_domain_add_linear(np, 32);
which makes the controller use irqs allocated only when they are
requested.