| Message ID | 1396029548-10928-8-git-send-email-b.brezillon.dev@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
Dear Boris BREZILLON, On Fri, 28 Mar 2014 18:59:05 +0100, Boris BREZILLON wrote: > +Optional children nodes: > +- muxed irq entries: > + Required properties: > + * compatible: Shall be > + "atmel,aic-mux-1reg-irq": irq enable/disable/retrieve-status is done by > + setting/clearing/reading flags in a specific register > + or > + "atmel,aic-mux-3reg-irq": irq enable/disable/retrieve-status is done > + by writing/reading flags in specific enable/disable/mask registers > + * reg: encode the interrupt control register. > + The first cell encode the irq line. > + The second cell encode the offset register within its iomem range > + The last cell encode the iomem region size (should always be set to 0x4). > + * atmel,aic-mux-reg-mask: define the mask used to disable the interrupts > + generated by the muxed entry. Can you describe in more details what are these muxed irqs? Are they interrupts raised to the AIC that may actually be related to several devices, like a shared interrupt? If that's the case, then what you want is to implement separate interrupt controller drivers to handle those shared interrupts, and demux them into multiple separate interrupts. Note that the way you use the "ranges" property seems wrong to me: you're using it as a "hack" to define the base address of some peripherals that are outside the AIC, while the ranges property is normally used to describe the address translations between a child bus and a parent bus. Which is not what you have here, as far as I can understand. So could you give more details about the design of the AIC and these muxed interrupts, to see if the DT binding you're proposing is actually the right way of representing the hardware? Thomas
Hello Thomas, Le 29/03/2014 10:19, Thomas Petazzoni a écrit : > Dear Boris BREZILLON, > > On Fri, 28 Mar 2014 18:59:05 +0100, Boris BREZILLON wrote: > >> +Optional children nodes: >> +- muxed irq entries: >> + Required properties: >> + * compatible: Shall be >> + "atmel,aic-mux-1reg-irq": irq enable/disable/retrieve-status is done by >> + setting/clearing/reading flags in a specific register >> + or >> + "atmel,aic-mux-3reg-irq": irq enable/disable/retrieve-status is done >> + by writing/reading flags in specific enable/disable/mask registers >> + * reg: encode the interrupt control register. >> + The first cell encode the irq line. >> + The second cell encode the offset register within its iomem range >> + The last cell encode the iomem region size (should always be set to 0x4). >> + * atmel,aic-mux-reg-mask: define the mask used to disable the interrupts >> + generated by the muxed entry. > Can you describe in more details what are these muxed irqs? Are they > interrupts raised to the AIC that may actually be related to several > devices, like a shared interrupt? Exactly, muxed irqs are shared irqs. > If that's the case, then what you want is to implement separate > interrupt controller drivers to handle those shared interrupts, and > demux them into multiple separate interrupts. It may work (I'll take a look), but we may have to modify several drivers (and it may break the DT ABI). > > Note that the way you use the "ranges" property seems wrong to me: > you're using it as a "hack" to define the base address of some > peripherals that are outside the AIC, while the ranges property is > normally used to describe the address translations between a child bus > and a parent bus. Which is not what you have here, as far as I can > understand. This is clearly not a real bus, but more a virtual/conceptual bus where each irq source is a bus device. > So could you give more details about the design of the AIC and these > muxed interrupts, to see if the DT binding you're proposing is actually > the right way of representing the hardware? The AIC controller muxes several peripheral irqs to one irq line. If a given peripheral has a pending irq, and the driver requesting this irq is not loaded (either because it was not enabled or because it is not loaded yet) it may generate spurious interrupts (or even hang forever), waiting for someone clearing/reading the interrupt flag(s). See this thread for an example of what can happen if we don't disable all muxed interrupts before enabling an IRQ line: https://lkml.org/lkml/2013/3/8/176 In this series' implementation, I define all the muxed (or shared) interrupt entries (using the DT) and let the AIC driver disable all the interrupt sources when shutting down an interrupt line or before starting the AIC controller. This provides a generic solution to avoid these spurious interrupt issues instead of adding several hooks in the machine specific code (one for each impacted peripheral). Please tell me if you see a better solution (but keep in mind, this should be done during early init, because the at91 init timer is using a muxed irq line on almost all at91 SoCs). Best Regards, Boris > > Thomas -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Thomas, Le 29/03/2014 10:19, Thomas Petazzoni a écrit : > Dear Boris BREZILLON, > > On Fri, 28 Mar 2014 18:59:05 +0100, Boris BREZILLON wrote: > >> +Optional children nodes: >> +- muxed irq entries: >> + Required properties: >> + * compatible: Shall be >> + "atmel,aic-mux-1reg-irq": irq enable/disable/retrieve-status is done by >> + setting/clearing/reading flags in a specific register >> + or >> + "atmel,aic-mux-3reg-irq": irq enable/disable/retrieve-status is done >> + by writing/reading flags in specific enable/disable/mask registers >> + * reg: encode the interrupt control register. >> + The first cell encode the irq line. >> + The second cell encode the offset register within its iomem range >> + The last cell encode the iomem region size (should always be set to 0x4). >> + * atmel,aic-mux-reg-mask: define the mask used to disable the interrupts >> + generated by the muxed entry. > Can you describe in more details what are these muxed irqs? Are they > interrupts raised to the AIC that may actually be related to several > devices, like a shared interrupt? Exactly, muxed irqs are shared irqs. > If that's the case, then what you want is to implement separate > interrupt controller drivers to handle those shared interrupts, and > demux them into multiple separate interrupts. It may work (I'll take a look), but we may have to modify several drivers (and it may break the DT ABI). > > Note that the way you use the "ranges" property seems wrong to me: > you're using it as a "hack" to define the base address of some > peripherals that are outside the AIC, while the ranges property is > normally used to describe the address translations between a child bus > and a parent bus. Which is not what you have here, as far as I can > understand. This is clearly not a real bus, but more a virtual/conceptual bus where each irq source is a bus device. > So could you give more details about the design of the AIC and these > muxed interrupts, to see if the DT binding you're proposing is actually > the right way of representing the hardware? The AIC controller muxes several peripheral irqs to one irq line. If a given peripheral has a pending irq, and the driver requesting this irq is not loaded (either because it was not enabled or because it is not loaded yet) it may generate spurious interrupts (or even hang forever), waiting for someone clearing/reading the interrupt flag(s). See this thread for an example of what can happen if we don't disable all muxed interrupts before enabling an IRQ line: https://lkml.org/lkml/2013/3/8/176 In this series' implementation, I define all the muxed (or shared) interrupt entries (using the DT) and let the AIC driver disable all the interrupt sources when shutting down an interrupt line or before starting the AIC controller. This provides a generic solution to avoid these spurious interrupt issues instead of adding several hooks in the machine specific code (one for each impacted peripheral). Please tell me if you see a better solution (but keep in mind, this should be done during early init, because the at91 init timer is using a muxed irq line on almost all at91 SoCs). Best Regards, Boris > > Thomas -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/interrupt-controller/atmel,aic.txt b/Documentation/devicetree/bindings/interrupt-controller/atmel,aic.txt index 2742e9c..d46ec8e 100644 --- a/Documentation/devicetree/bindings/interrupt-controller/atmel,aic.txt +++ b/Documentation/devicetree/bindings/interrupt-controller/atmel,aic.txt @@ -18,8 +18,33 @@ Required properties: The third cell is used to specify the irq priority from 0 (lowest) to 7 (highest). - reg: Should contain AIC registers location and length +- #address-cells: Shall be 2. The first cell encodes the irq line (or irq id). + The second cell encodes the register offset within the iomem range. +- #size-cells: Shall be 1. +- ranges: Defines the iomem ranges attached to a given irq line (e.i. irq + line 1 <=> SYSC range). - atmel,external-irqs: u32 array of external irqs. +Optional properties: +- atmel,irq-mapping: u32 mask array representing the available irqs: + e.i. : atmel,irq-mapping = <0xffff1fff> => irqs 13 to 15 are unavailables + +Optional children nodes: +- muxed irq entries: + Required properties: + * compatible: Shall be + "atmel,aic-mux-1reg-irq": irq enable/disable/retrieve-status is done by + setting/clearing/reading flags in a specific register + or + "atmel,aic-mux-3reg-irq": irq enable/disable/retrieve-status is done + by writing/reading flags in specific enable/disable/mask registers + * reg: encode the interrupt control register. + The first cell encode the irq line. + The second cell encode the offset register within its iomem range + The last cell encode the iomem region size (should always be set to 0x4). + * atmel,aic-mux-reg-mask: define the mask used to disable the interrupts + generated by the muxed entry. + Examples: /* * AIC @@ -29,11 +54,26 @@ Examples: interrupt-controller; interrupt-parent; #interrupt-cells = <3>; + #address-cells = <2>; + #size-cells = <1>; reg = <0xfffff000 0x200>; + ranges = <0x1 0x0 0xffffc000 0x4000>; + + dbgu_irq: irq@1,320c { + compatible = "atmel,aic-mux-3reg-irq"; + reg = <1 0x320c 0x4>; + atmel,aic-mux-reg-mask = <0xc0001afb>; + }; + + pmc_irq: irq@1,3c64 { + compatible = "atmel,aic-mux-3reg-irq"; + reg = <1 0x3c64 0x4>; + atmel,aic-mux-reg-mask = <0xf0f>; + }; }; /* - * An interrupt generating device that is wired to an AIC. + * A device generating interrupts wired to the AIC. */ dma: dma-controller@ffffec00 { compatible = "atmel,at91sam9g45-dma";
Add irq muxing and irq-mapping dt bindings documentation. Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com> --- .../bindings/interrupt-controller/atmel,aic.txt | 42 +++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-)