| Message ID | 1311610200-12408-2-git-send-email-jamie@jamieiles.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@arm.linux.org.uk> > Cc: devicetree-discuss@lists.ozlabs.org > Signed-off-by: Jamie Iles <jamie@jamieiles.com> > --- > 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@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/devicetree-discuss
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
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.
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
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
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@arm.linux.org.uk> Cc: devicetree-discuss@lists.ozlabs.org Signed-off-by: Jamie Iles <jamie@jamieiles.com> --- 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