[PATCH 0/3] Add device tree based initialisation for VIC

[PATCH 0/3] Add device tree based initialisation for VIC

[Jamie Iles]

This series aims to allow VIC devices to be probed from the device tree.
Currently the only in-tree device tree enabled user of the VIC is versatile,
but I'll shortly submit patches for picoxcell on device tree that uses 2
VIC's.

I've split this into 3 patches for bisectability, but this could be fairly
squashed into 2.

Jamie Iles (3):
  vic: add device tree bindings
  versatile dt: set the irq-start property for the vic
  versatile: convert to common VIC DT probing

 Documentation/devicetree/bindings/arm/vic.txt |   21 +++++++++++++++
 arch/arm/boot/dts/versatile-ab.dts            |    1 +
 arch/arm/common/vic.c                         |   35 +++++++++++++++++++++++++
 arch/arm/include/asm/hardware/vic.h           |    5 +++
 arch/arm/mach-versatile/core.c                |    9 +-----
 5 files changed, 63 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/vic.txt

-- 
1.7.4.1

[PATCH 1/3] vic: add device tree bindings

[Jamie Iles]

Allow the VIC to be used from device tree.  This adds vic_of_init() that
finds all compatible controllers in the device tree creating irq domains
and initialising the VIC.

We use of_iomap() for the IO mapping, but allow the entry functions in
arch/arm/include/asm/entry-macro-vic2.S to be used, this requires that a
static mapping is configured on the platform.

Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
Signed-off-by: Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>
---
 Documentation/devicetree/bindings/arm/vic.txt |   21 +++++++++++++++
 arch/arm/common/vic.c                         |   35 +++++++++++++++++++++++++
 arch/arm/include/asm/hardware/vic.h           |    5 +++
 3 files changed, 61 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/vic.txt

diff --git a/Documentation/devicetree/bindings/arm/vic.txt b/Documentation/devicetree/bindings/arm/vic.txt
new file mode 100644
index 0000000..be5abc9
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/vic.txt
@@ -0,0 +1,21 @@
+ARM Vectored Interrupt Controller
+
+Some ARM cores may contain a vectored interrupt controller (VIC).  This
+controller is represented in the device tree with:
+
+Required properties:
+  - #interrupt-cells : <1> (32 interrupt sources supported)
+  - compatible : "arm,pl190-vic", "arm,pl192-vic", "arm-vic"
+  - reg : Offset and length of the register set for this device
+  - interrupt-controller
+  - irq-start : The first interrupt number that the VIC services
+
+Example ARM VIC node:
+
+vic0@60000 {
+	compatible = "arm,pl192-vic";
+	interrupt-controller;
+	reg = <0x60000 0x1000>;
+	irq-start = <32>;
+	#interrupt-cells = <1>;
+};
diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c
index 7aa4262..5838b9f 100644
--- a/arch/arm/common/vic.c
+++ b/arch/arm/common/vic.c
@@ -25,6 +25,9 @@
 #include <linux/syscore_ops.h>
 #include <linux/device.h>
 #include <linux/amba/bus.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
 
 #include <asm/mach/irq.h>
 #include <asm/hardware/vic.h>
@@ -377,3 +380,35 @@ void __init vic_init(void __iomem *base, unsigned int irq_start,
 
 	vic_pm_register(base, irq_start, resume_sources);
 }
+
+#ifdef CONFIG_OF
+static const struct of_device_id arm_vic_ids[] __initconst = {
+	{ .compatible = "arm,pl190-vic" },
+	{ .compatible = "arm,pl192-vic" },
+	{ .compatible = "arm,vic" },
+	{},
+};
+
+void __init vic_of_init(void)
+{
+	struct device_node *np;
+
+	for_each_matching_node(np, arm_vic_ids) {
+		void __iomem *iobase;
+		u32 base_irq;
+
+		iobase = of_iomap(np, 0);
+
+		if (!iobase)
+			panic("Unable to map VIC");
+
+		if (of_property_read_u32(np, "irq-start", &base_irq))
+			panic("No irq-start property defined");
+
+		of_node_put(np);
+
+		vic_init(iobase, base_irq, ~0, 0);
+		irq_domain_add_simple(np, base_irq);
+	}
+}
+#endif /* CONFIG_OF */
diff --git a/arch/arm/include/asm/hardware/vic.h b/arch/arm/include/asm/hardware/vic.h
index 5d72550..dbda2d1 100644
--- a/arch/arm/include/asm/hardware/vic.h
+++ b/arch/arm/include/asm/hardware/vic.h
@@ -42,6 +42,11 @@
 
 #ifndef __ASSEMBLY__
 void vic_init(void __iomem *base, unsigned int irq_start, u32 vic_sources, u32 resume_sources);
+#ifdef CONFIG_OF
+void vic_of_init(void);
+#else /* CONFIG_OF */
+static inline void vic_of_init(void) {}
+#endif /* CONFIG_OF */
 #endif
 
 #endif
-- 
1.7.4.1

Re: [PATCH 1/3] vic: add device tree bindings

[Grant Likely]

On Mon, Jul 25, 2011 at 05:09:58PM +0100, Jamie Iles wrote:
> Allow the VIC to be used from device tree.  This adds vic_of_init() that
> finds all compatible controllers in the device tree creating irq domains
> and initialising the VIC.
> 
> We use of_iomap() for the IO mapping, but allow the entry functions in
> arch/arm/include/asm/entry-macro-vic2.S to be used, this requires that a
> static mapping is configured on the platform.
> 
> Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> Signed-off-by: Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/arm/vic.txt |   21 +++++++++++++++

Yay!  Thanks for documenting!

>  arch/arm/common/vic.c                         |   35 +++++++++++++++++++++++++
>  arch/arm/include/asm/hardware/vic.h           |    5 +++
>  3 files changed, 61 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/vic.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/vic.txt b/Documentation/devicetree/bindings/arm/vic.txt
> new file mode 100644
> index 0000000..be5abc9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/vic.txt
> @@ -0,0 +1,21 @@
> +ARM Vectored Interrupt Controller
> +
> +Some ARM cores may contain a vectored interrupt controller (VIC).  This
> +controller is represented in the device tree with:
> +
> +Required properties:
> +  - #interrupt-cells : <1> (32 interrupt sources supported)

Does the vic have any configuration for level/edge or polarity?

> +  - compatible : "arm,pl190-vic", "arm,pl192-vic", "arm-vic"

drop "arm-vic".  Compatible values should always be specific to an
implementation, preferably to a specific version of hardware, although
I'm also okay with an IP core name if the version if the core is
provided.

> +  - reg : Offset and length of the register set for this device
> +  - interrupt-controller
> +  - irq-start : The first interrupt number that the VIC services

Drop irq-start, it should not be needed.  The interrupt controller
should allocate its own range of irq numbers when it is set up.

> +
> +Example ARM VIC node:
> +
> +vic0@60000 {
> +	compatible = "arm,pl192-vic";
> +	interrupt-controller;
> +	reg = <0x60000 0x1000>;
> +	irq-start = <32>;
> +	#interrupt-cells = <1>;
> +};
> diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c
> index 7aa4262..5838b9f 100644
> --- a/arch/arm/common/vic.c
> +++ b/arch/arm/common/vic.c
> @@ -25,6 +25,9 @@
>  #include <linux/syscore_ops.h>
>  #include <linux/device.h>
>  #include <linux/amba/bus.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
>  
>  #include <asm/mach/irq.h>
>  #include <asm/hardware/vic.h>
> @@ -377,3 +380,35 @@ void __init vic_init(void __iomem *base, unsigned int irq_start,
>  
>  	vic_pm_register(base, irq_start, resume_sources);
>  }
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id arm_vic_ids[] __initconst = {
> +	{ .compatible = "arm,pl190-vic" },
> +	{ .compatible = "arm,pl192-vic" },
> +	{ .compatible = "arm,vic" },
> +	{},
> +};
> +
> +void __init vic_of_init(void)
> +{
> +	struct device_node *np;
> +
> +	for_each_matching_node(np, arm_vic_ids) {
> +		void __iomem *iobase;
> +		u32 base_irq;
> +
> +		iobase = of_iomap(np, 0);
> +
> +		if (!iobase)
> +			panic("Unable to map VIC");

If you WARN here instead, then there is a much greater chance of
actually getting output to the user.

> +
> +		if (of_property_read_u32(np, "irq-start", &base_irq))
> +			panic("No irq-start property defined");
> +
> +		of_node_put(np);
> +
> +		vic_init(iobase, base_irq, ~0, 0);
> +		irq_domain_add_simple(np, base_irq);

irq_domain_add_simple() is a stop-gap shortcut for interrupt
controllers that don't use irq_domain directly.  I'm okay with doing
this in the short term, but I imagine it will want to change in the
near future to take advantage of hw->linux irq translation provided by
irq_domain when it matures.

> +	}

I think that rather than writing a interrupt-controller-specific
parse route like this one, it would be much better to have a generic
helper that finds and sorts all the interrupt controllers before
calling a setup callback for each one.

> +}
> +#endif /* CONFIG_OF */
> diff --git a/arch/arm/include/asm/hardware/vic.h b/arch/arm/include/asm/hardware/vic.h
> index 5d72550..dbda2d1 100644
> --- a/arch/arm/include/asm/hardware/vic.h
> +++ b/arch/arm/include/asm/hardware/vic.h
> @@ -42,6 +42,11 @@
>  
>  #ifndef __ASSEMBLY__
>  void vic_init(void __iomem *base, unsigned int irq_start, u32 vic_sources, u32 resume_sources);
> +#ifdef CONFIG_OF
> +void vic_of_init(void);
> +#else /* CONFIG_OF */
> +static inline void vic_of_init(void) {}
> +#endif /* CONFIG_OF */
>  #endif
>  
>  #endif
> -- 
> 1.7.4.1
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 1/3] vic: add device tree bindings

[Jamie Iles]

Hi Grant,

On Mon, Jul 25, 2011 at 02:04:34PM -0600, Grant Likely wrote:
> On Mon, Jul 25, 2011 at 05:09:58PM +0100, Jamie Iles wrote:
[...]
> >  arch/arm/common/vic.c                         |   35 +++++++++++++++++++++++++
> >  arch/arm/include/asm/hardware/vic.h           |    5 +++
> >  3 files changed, 61 insertions(+), 0 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/arm/vic.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/vic.txt b/Documentation/devicetree/bindings/arm/vic.txt
> > new file mode 100644
> > index 0000000..be5abc9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/vic.txt
> > @@ -0,0 +1,21 @@
> > +ARM Vectored Interrupt Controller
> > +
> > +Some ARM cores may contain a vectored interrupt controller (VIC).  This
> > +controller is represented in the device tree with:
> > +
> > +Required properties:
> > +  - #interrupt-cells : <1> (32 interrupt sources supported)
> 
> Does the vic have any configuration for level/edge or polarity?

No, there's no configuration of this type.

> > +  - compatible : "arm,pl190-vic", "arm,pl192-vic", "arm-vic"
> 
> drop "arm-vic".  Compatible values should always be specific to an
> implementation, preferably to a specific version of hardware, although
> I'm also okay with an IP core name if the version if the core is
> provided.

OK, fair point.  Versatile currently uses arm-vic but I'm not sure what 
part it actually is as I don't have access to that platform.

> > +  - reg : Offset and length of the register set for this device
> > +  - interrupt-controller
> > +  - irq-start : The first interrupt number that the VIC services
> 
> Drop irq-start, it should not be needed.  The interrupt controller
> should allocate its own range of irq numbers when it is set up.

So the reason I have this is in our system (and I believe others too 
including some samsung parts), the vic's aren't cascaded so I'm not sure 
how the entry macros would resolve the outputs from each vic correctly.  
Perhaps I'm missing something here though.

> > +
> > +Example ARM VIC node:
> > +
> > +vic0@60000 {
> > +	compatible = "arm,pl192-vic";
> > +	interrupt-controller;
> > +	reg = <0x60000 0x1000>;
> > +	irq-start = <32>;
> > +	#interrupt-cells = <1>;
> > +};
> > diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c
> > index 7aa4262..5838b9f 100644
> > --- a/arch/arm/common/vic.c
> > +++ b/arch/arm/common/vic.c
> > @@ -25,6 +25,9 @@
> >  #include <linux/syscore_ops.h>
> >  #include <linux/device.h>
> >  #include <linux/amba/bus.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> >  
> >  #include <asm/mach/irq.h>
> >  #include <asm/hardware/vic.h>
> > @@ -377,3 +380,35 @@ void __init vic_init(void __iomem *base, unsigned int irq_start,
> >  
> >  	vic_pm_register(base, irq_start, resume_sources);
> >  }
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id arm_vic_ids[] __initconst = {
> > +	{ .compatible = "arm,pl190-vic" },
> > +	{ .compatible = "arm,pl192-vic" },
> > +	{ .compatible = "arm,vic" },
> > +	{},
> > +};
> > +
> > +void __init vic_of_init(void)
> > +{
> > +	struct device_node *np;
> > +
> > +	for_each_matching_node(np, arm_vic_ids) {
> > +		void __iomem *iobase;
> > +		u32 base_irq;
> > +
> > +		iobase = of_iomap(np, 0);
> > +
> > +		if (!iobase)
> > +			panic("Unable to map VIC");
> 
> If you WARN here instead, then there is a much greater chance of
> actually getting output to the user.

OK, I'll change this.

> > +
> > +		if (of_property_read_u32(np, "irq-start", &base_irq))
> > +			panic("No irq-start property defined");
> > +
> > +		of_node_put(np);
> > +
> > +		vic_init(iobase, base_irq, ~0, 0);
> > +		irq_domain_add_simple(np, base_irq);
> 
> irq_domain_add_simple() is a stop-gap shortcut for interrupt
> controllers that don't use irq_domain directly.  I'm okay with doing
> this in the short term, but I imagine it will want to change in the
> near future to take advantage of hw->linux irq translation provided by
> irq_domain when it matures.

I have to admit to taking this from other controllers without fully 
understanding it.  Is there any documentation on how this should be done 
correctly in the longer term?

> > +	}
> 
> I think that rather than writing a interrupt-controller-specific
> parse route like this one, it would be much better to have a generic
> helper that finds and sorts all the interrupt controllers before
> calling a setup callback for each one.

Hmm, not sure I follow this.  I can see that many controllers would have 
some common properties so there will be some common code - are you 
suggesting having something do all the parsing then callbacks for each 
controller type that takes some kind of template or am I way off the 
mark?

Thanks,

Jamie

Re: [PATCH 1/3] vic: add device tree bindings

[Grant Likely]

On Mon, Jul 25, 2011 at 11:31:51PM +0100, Jamie Iles wrote:
> Hi Grant,
> 
> On Mon, Jul 25, 2011 at 02:04:34PM -0600, Grant Likely wrote:
> > irq_domain_add_simple() is a stop-gap shortcut for interrupt
> > controllers that don't use irq_domain directly.  I'm okay with doing
> > this in the short term, but I imagine it will want to change in the
> > near future to take advantage of hw->linux irq translation provided by
> > irq_domain when it matures.
> 
> I have to admit to taking this from other controllers without fully 
> understanding it.  Is there any documentation on how this should be done 
> correctly in the longer term?

Documentation?  Ummmm... no, not yet.  :-/  There will be though.

> 
> > > +	}
> > 
> > I think that rather than writing a interrupt-controller-specific
> > parse route like this one, it would be much better to have a generic
> > helper that finds and sorts all the interrupt controllers before
> > calling a setup callback for each one.
> 
> Hmm, not sure I follow this.  I can see that many controllers would have 
> some common properties so there will be some common code - are you 
> suggesting having something do all the parsing then callbacks for each 
> controller type that takes some kind of template or am I way off the 
> mark?

No, I'm more talking about having a routine that finds all the
interrupt controllers and figures out the cascading order, and then
calls each irq controller setup routine in order.

g.

Re: [PATCH 1/3] vic: add device tree bindings

[Jamie Iles]

On Sat, Jul 30, 2011 at 10:11:07PM -0600, Grant Likely wrote:
> On Mon, Jul 25, 2011 at 11:31:51PM +0100, Jamie Iles wrote:
> > 
> > > > +	}
> > > 
> > > I think that rather than writing a interrupt-controller-specific
> > > parse route like this one, it would be much better to have a generic
> > > helper that finds and sorts all the interrupt controllers before
> > > calling a setup callback for each one.
> > 
> > Hmm, not sure I follow this.  I can see that many controllers would have 
> > some common properties so there will be some common code - are you 
> > suggesting having something do all the parsing then callbacks for each 
> > controller type that takes some kind of template or am I way off the 
> > mark?
> 
> No, I'm more talking about having a routine that finds all the
> interrupt controllers and figures out the cascading order, and then
> calls each irq controller setup routine in order.

OK, that makes sense.  I'm not sure how best to implement that but I'll 
give it some thought.

Regarding the irq-start property - on picoxcell we have 2 VIC's and they 
aren't cascaded - the outputs are just OR'd together so I can't work out 
how to fit in the IRQ decoding with get_irqnr_and_base without having 
this property.  Is there another way that I could implement that?

Jamie

[PATCH 2/3] versatile dt: set the irq-start property for the vic

[Jamie Iles]

Set an irq-start property for the vic in versatile's dt so that we can
convert the versatile platform to use common vic probing.

Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
Signed-off-by: Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>
---
 arch/arm/boot/dts/versatile-ab.dts |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/versatile-ab.dts b/arch/arm/boot/dts/versatile-ab.dts
index 2723804..1c6a1e3 100644
--- a/arch/arm/boot/dts/versatile-ab.dts
+++ b/arch/arm/boot/dts/versatile-ab.dts
@@ -79,6 +79,7 @@
 			interrupt-controller;
 			#interrupt-cells = <1>;
 			reg = <0x10140000 0x1000>;
+			irq-start = <0>;
 		};
 
 		sic: intc@10003000 {
-- 
1.7.4.1

[PATCH 3/3] versatile: convert to common VIC DT probing

[Jamie Iles]

Use the common VIC probing code in the VIC driver to instantiate the VIC
from the DT.

Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@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 |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-versatile/core.c b/arch/arm/mach-versatile/core.c
index 15c0091..3f29c01 100644
--- a/arch/arm/mach-versatile/core.c
+++ b/arch/arm/mach-versatile/core.c
@@ -85,12 +85,6 @@ 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,vic", },
-	{}
-};
-
 static const struct of_device_id sic_of_match[] __initconst = {
 	{ .compatible = "arm,sic", },
 	{}
@@ -98,8 +92,7 @@ static const struct of_device_id sic_of_match[] __initconst = {
 
 void __init versatile_init_irq(void)
 {
-	vic_init(VA_VIC_BASE, IRQ_VIC_START, ~0, 0);
-	irq_domain_generate_simple(vic_of_match, VERSATILE_VIC_BASE, IRQ_VIC_START);
+	vic_of_init();
 
 	writel(~0, VA_SIC_BASE + SIC_IRQ_ENABLE_CLEAR);
 
-- 
1.7.4.1

Re: [PATCH 0/3] Add device tree based initialisation for VIC

[Grant Likely]

On Mon, Jul 25, 2011 at 05:09:57PM +0100, Jamie Iles wrote:
> This series aims to allow VIC devices to be probed from the device tree.
> Currently the only in-tree device tree enabled user of the VIC is versatile,
> but I'll shortly submit patches for picoxcell on device tree that uses 2
> VIC's.
> 
> I've split this into 3 patches for bisectability, but this could be fairly
> squashed into 2.

I always prefer fewer patches.  Which 2 patches would you squash
together?  :-)

> 
> Jamie Iles (3):
>   vic: add device tree bindings
>   versatile dt: set the irq-start property for the vic
>   versatile: convert to common VIC DT probing
> 
>  Documentation/devicetree/bindings/arm/vic.txt |   21 +++++++++++++++
>  arch/arm/boot/dts/versatile-ab.dts            |    1 +
>  arch/arm/common/vic.c                         |   35 +++++++++++++++++++++++++
>  arch/arm/include/asm/hardware/vic.h           |    5 +++
>  arch/arm/mach-versatile/core.c                |    9 +-----
>  5 files changed, 63 insertions(+), 8 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/vic.txt
> 
> -- 
> 1.7.4.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] Add device tree based initialisation for VIC

[Jamie Iles]

On Mon, Jul 25, 2011 at 01:54:23PM -0600, Grant Likely wrote:
> On Mon, Jul 25, 2011 at 05:09:57PM +0100, Jamie Iles wrote:
> > This series aims to allow VIC devices to be probed from the device tree.
> > Currently the only in-tree device tree enabled user of the VIC is versatile,
> > but I'll shortly submit patches for picoxcell on device tree that uses 2
> > VIC's.
> > 
> > I've split this into 3 patches for bisectability, but this could be fairly
> > squashed into 2.
> 
> I always prefer fewer patches.  Which 2 patches would you squash
> together?  :-)

The conversion for versatile could go into one patch, but I'm not sure it 
would bisect as easily (I guess that's more important though).

Jamie