* Using irq-crossbar.c @ 2016-06-10 15:37 Sebastian Frias 2016-06-10 16:05 ` Marc Zyngier 2016-06-10 16:06 ` Lennart Sorensen 0 siblings, 2 replies; 27+ messages in thread From: Sebastian Frias @ 2016-06-10 15:37 UTC (permalink / raw) To: Thomas Gleixner, Marc Zyngier Cc: LKML, Grygorii Strashko, Sricharan R, Mason, Måns Rullgård Hi, We are trying to write a driver for an interrupt controller (actually more of a crossbar) for an ARM-based SoC. This IRQ crossbar has 128 inputs and 24 outputs, the outputs are connected directly to the GIC. The idea is that the GIC handles everything, and just request a mapping from an IRQ number (0...127, from a device's DT entry) into one of its 24 input lines. By looking at current code (4.7-rc1) there seems to be a driver (drivers/irqchip/irq-crossbar.c) that provides similar functionality. The driver uses hierarchical irq domains (since commit 783d31863fb8 "irqchip: crossbar: Convert dra7 crossbar to stacked domains") which we believe we don't need because the only controller is the GIC. However the API used previously, register_routable_domain_ops(), was removed with commit a5561c3e845c "irqchip: gic: Get rid of routable domain". Trying to use the driver with hierarchical domains (after modifications for our SoC), results on the kernel being blocked at some point: [ 0.041524] ThumbEE CPU extension supported. [ 0.041589] Registering SWP/SWPB emulation handler [ 0.052022] Freeing unused kernel memory: 12364K (c029b000 - c0eae000) [ 0.074084] random: dbus-uuidgen urandom read with 0 bits of entropy available We've put logs on the different domain_ops calls (alloc, free, translate) but they are not called, even if the DT is supposed to tell devices to take interrupts from this controller (*). Do you have suggestions on what APIs should be used, further reading/examples and/or pointers on how debug this (logs to enable, things to look for, etc.)? Thanks in advance. Best regards, Sebastian (*): here's the diff on our DT: --- tango4-common.dtsi 2016-06-10 16:23:08.244246017 +0200 +++ tangox_irqv2-common.dtsi 2016-06-10 16:24:01.212588737 +0200 @@ -47,7 +47,7 @@ soc { compatible = "simple-bus"; - interrupt-parent = <&irq0>; + interrupt-parent = <&irq_mux>; #address-cells = <1>; #size-cells = <1>; ranges; @@ -75,7 +75,7 @@ uart: serial@10700 { compatible = "ralink,rt2880-uart"; reg = <0x10700 0x30>; - interrupts = <1 IRQ_TYPE_LEVEL_HIGH>; + interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>; clock-frequency = <7372800>; reg-shift = <2>; }; @@ -83,10 +83,11 @@ eth0: ethernet@26000 { compatible = "sigma,smp8734-ethernet"; reg = <0x26000 0x800>; - interrupts = <38 IRQ_TYPE_LEVEL_HIGH>; + interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>; clocks = <&clkgen 1>; }; +#if 0 intc: interrupt-controller@6e000 { compatible = "sigma,smp8642-intc"; reg = <0x6e000 0x400>; @@ -117,5 +118,16 @@ interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>; }; }; +#else + irq_mux: irq_mux@6f800 { + compatible = "sigma,smp-irq-mux"; + reg = <0x6f800 0x400>; + interrupt-controller; + interrupt-parent = <&gic>; + irqs-reserved = <2 3 4 125 126 127>; + }; + +#endif + }; }; ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-10 15:37 Using irq-crossbar.c Sebastian Frias @ 2016-06-10 16:05 ` Marc Zyngier 2016-06-10 19:36 ` Mason 2016-06-13 15:46 ` Sebastian Frias 2016-06-10 16:06 ` Lennart Sorensen 1 sibling, 2 replies; 27+ messages in thread From: Marc Zyngier @ 2016-06-10 16:05 UTC (permalink / raw) To: Sebastian Frias, Thomas Gleixner Cc: LKML, Grygorii Strashko, Sricharan R, Mason, Måns Rullgård On 10/06/16 16:37, Sebastian Frias wrote: > Hi, > > We are trying to write a driver for an interrupt controller (actually > more of a crossbar) for an ARM-based SoC. This IRQ crossbar has 128 > inputs and 24 outputs, the outputs are connected directly to the > GIC. The idea is that the GIC handles everything, and just request a > mapping from an IRQ number (0...127, from a device's DT entry) into > one of its 24 input lines. "Just request a mapping...". > By looking at current code (4.7-rc1) there seems to be a driver > (drivers/irqchip/irq-crossbar.c) that provides similar > functionality. The driver uses hierarchical irq domains (since commit > 783d31863fb8 "irqchip: crossbar: Convert dra7 crossbar to stacked > domains") which we believe we don't need because the only controller > is the GIC. So you need it, but you don't need it? The GIC may be the only interrupt controller with which software interacts when the interrupt occurs, but the crossbar does play a major role in *routing* the interrupt to the right GIC pin. > However the API used previously, register_routable_domain_ops(), was > removed with commit a5561c3e845c "irqchip: gic: Get rid of routable > domain". And every day, I thank $DEITY for having delivered us from this evil. Really. And it wasn't much of an API. It was the son of a hack, bolted on the side of another hack. Unmaintainable, getting in the way. I had much fun slaughtering it! ;-) > Trying to use the driver with hierarchical domains (after > modifications for our SoC), results on the kernel being blocked at > some point: > > [ 0.041524] ThumbEE CPU extension supported. > [ 0.041589] Registering SWP/SWPB emulation handler > [ 0.052022] Freeing unused kernel memory: 12364K (c029b000 - c0eae000) > [ 0.074084] random: dbus-uuidgen urandom read with 0 bits of entropy available Sorry, but that's not much of a log. Anything related to interrupts, maybe? > We've put logs on the different domain_ops calls (alloc, free, > translate) but they are not called, even if the DT is supposed to > tell devices to take interrupts from this controller (*). At all? Nobody is talking to the GIC? > Do you have suggestions on what APIs should be used, further > reading/examples and/or pointers on how debug this (logs to enable, > things to look for, etc.)? You could start with posting actual logs of an interrupt being requested, as well as perform some basic tracing of the various callbacks into the irqdomain and irqchip layers, all the way down to the interrupt controllers (note the plural). Also, without seeing the code, it is pretty difficult to make any meaningful comment... > Thanks in advance. > Best regards, > > Sebastian > > > (*): > here's the diff on our DT: > > --- tango4-common.dtsi 2016-06-10 16:23:08.244246017 +0200 > +++ tangox_irqv2-common.dtsi 2016-06-10 16:24:01.212588737 +0200 > @@ -47,7 +47,7 @@ > > soc { > compatible = "simple-bus"; > - interrupt-parent = <&irq0>; > + interrupt-parent = <&irq_mux>; > #address-cells = <1>; > #size-cells = <1>; > ranges; > @@ -75,7 +75,7 @@ > uart: serial@10700 { > compatible = "ralink,rt2880-uart"; > reg = <0x10700 0x30>; > - interrupts = <1 IRQ_TYPE_LEVEL_HIGH>; > + interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>; > clock-frequency = <7372800>; > reg-shift = <2>; > }; > @@ -83,10 +83,11 @@ > eth0: ethernet@26000 { > compatible = "sigma,smp8734-ethernet"; > reg = <0x26000 0x800>; > - interrupts = <38 IRQ_TYPE_LEVEL_HIGH>; > + interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&clkgen 1>; > }; > > +#if 0 > intc: interrupt-controller@6e000 { > compatible = "sigma,smp8642-intc"; > reg = <0x6e000 0x400>; > @@ -117,5 +118,16 @@ > interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>; > }; > }; > +#else > + irq_mux: irq_mux@6f800 { > + compatible = "sigma,smp-irq-mux"; > + reg = <0x6f800 0x400>; > + interrupt-controller; > + interrupt-parent = <&gic>; > + irqs-reserved = <2 3 4 125 126 127>; > + }; Where is the GIC? Where is the #interrupt-cells property? What is the interrupt parent for the GIC itself? (and I'm tempted to add "What is your name? What is you quest?", but that's because it is Friday and I feel like I need a beer...). M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-10 16:05 ` Marc Zyngier @ 2016-06-10 19:36 ` Mason 2016-06-11 9:58 ` Marc Zyngier 2016-06-13 15:46 ` Sebastian Frias 1 sibling, 1 reply; 27+ messages in thread From: Mason @ 2016-06-10 19:36 UTC (permalink / raw) To: Marc Zyngier, Sebastian Frias Cc: Thomas Gleixner, LKML, Grygorii Strashko, Sricharan R, Mans Rullgard On 10/06/2016 18:05, Marc Zyngier wrote: > On 10/06/16 16:37, Sebastian Frias wrote: > >> here's the diff on our DT: >> >> --- tango4-common.dtsi 2016-06-10 16:23:08.244246017 +0200 >> +++ tangox_irqv2-common.dtsi 2016-06-10 16:24:01.212588737 +0200 >> @@ -47,7 +47,7 @@ >> >> soc { >> compatible = "simple-bus"; >> - interrupt-parent = <&irq0>; >> + interrupt-parent = <&irq_mux>; >> #address-cells = <1>; >> #size-cells = <1>; >> ranges; >> @@ -75,7 +75,7 @@ >> uart: serial@10700 { >> compatible = "ralink,rt2880-uart"; >> reg = <0x10700 0x30>; >> - interrupts = <1 IRQ_TYPE_LEVEL_HIGH>; >> + interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>; >> clock-frequency = <7372800>; >> reg-shift = <2>; >> }; >> @@ -83,10 +83,11 @@ >> eth0: ethernet@26000 { >> compatible = "sigma,smp8734-ethernet"; >> reg = <0x26000 0x800>; >> - interrupts = <38 IRQ_TYPE_LEVEL_HIGH>; >> + interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>; >> clocks = <&clkgen 1>; >> }; >> >> +#if 0 >> intc: interrupt-controller@6e000 { >> compatible = "sigma,smp8642-intc"; >> reg = <0x6e000 0x400>; >> @@ -117,5 +118,16 @@ >> interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>; >> }; >> }; >> +#else >> + irq_mux: irq_mux@6f800 { >> + compatible = "sigma,smp-irq-mux"; >> + reg = <0x6f800 0x400>; >> + interrupt-controller; >> + interrupt-parent = <&gic>; >> + irqs-reserved = <2 3 4 125 126 127>; >> + }; > > Where is the GIC? Where is the #interrupt-cells property? What is the > interrupt parent for the GIC itself? (and I'm tempted to add "What is > your name? What is you quest?", but that's because it is Friday and I > feel like I need a beer...). Beer and cheese? Beurk! ;-) I think Sebastian is even more baffled by the DT mess (sorry, intricacies) than I am. The base file he was referring to is: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/tango4-common.dtsi Regards. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-10 19:36 ` Mason @ 2016-06-11 9:58 ` Marc Zyngier 2016-06-11 15:37 ` Mason 2016-06-13 15:15 ` Sebastian Frias 0 siblings, 2 replies; 27+ messages in thread From: Marc Zyngier @ 2016-06-11 9:58 UTC (permalink / raw) To: Mason Cc: Sebastian Frias, Thomas Gleixner, LKML, Grygorii Strashko, Sricharan R, Mans Rullgard On Fri, 10 Jun 2016 21:36:29 +0200 Mason <slash.tmp@free.fr> wrote: > On 10/06/2016 18:05, Marc Zyngier wrote: > > > On 10/06/16 16:37, Sebastian Frias wrote: > > > >> here's the diff on our DT: > >> > >> --- tango4-common.dtsi 2016-06-10 16:23:08.244246017 +0200 > >> +++ tangox_irqv2-common.dtsi 2016-06-10 16:24:01.212588737 +0200 > >> @@ -47,7 +47,7 @@ > >> > >> soc { > >> compatible = "simple-bus"; > >> - interrupt-parent = <&irq0>; > >> + interrupt-parent = <&irq_mux>; > >> #address-cells = <1>; > >> #size-cells = <1>; > >> ranges; > >> @@ -75,7 +75,7 @@ > >> uart: serial@10700 { > >> compatible = "ralink,rt2880-uart"; > >> reg = <0x10700 0x30>; > >> - interrupts = <1 IRQ_TYPE_LEVEL_HIGH>; > >> + interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>; > >> clock-frequency = <7372800>; > >> reg-shift = <2>; > >> }; > >> @@ -83,10 +83,11 @@ > >> eth0: ethernet@26000 { > >> compatible = "sigma,smp8734-ethernet"; > >> reg = <0x26000 0x800>; > >> - interrupts = <38 IRQ_TYPE_LEVEL_HIGH>; > >> + interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>; > >> clocks = <&clkgen 1>; > >> }; > >> > >> +#if 0 > >> intc: interrupt-controller@6e000 { > >> compatible = "sigma,smp8642-intc"; > >> reg = <0x6e000 0x400>; > >> @@ -117,5 +118,16 @@ > >> interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>; > >> }; > >> }; > >> +#else > >> + irq_mux: irq_mux@6f800 { > >> + compatible = "sigma,smp-irq-mux"; > >> + reg = <0x6f800 0x400>; > >> + interrupt-controller; > >> + interrupt-parent = <&gic>; > >> + irqs-reserved = <2 3 4 125 126 127>; > >> + }; > > > > Where is the GIC? Where is the #interrupt-cells property? What is the > > interrupt parent for the GIC itself? (and I'm tempted to add "What is > > your name? What is you quest?", but that's because it is Friday and I > > feel like I need a beer...). > > Beer and cheese? Beurk! ;-) I don't think you've ever tried (or at least, not with proper cheese and/or beer). > I think Sebastian is even more baffled by the DT mess > (sorry, intricacies) than I am. This mess is what has saved us from the apocalypse 5 years ago, and describing a complex system is not easy (what a surprise...). If you just want to apply recipes without understanding the underlying constraints, you're in for a lot of pain. > The base file he was referring to is: > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/tango4-common.dtsi I know which file that is, it is mentioned in the diff. I was merely trying to point out the glaring mistakes that could be enough for a interrupt controller hierarchy to be completely non-functional: - Your crossbar doesn't have a #interrupt-cells property. How do you expect the interrupt specifiers to be interpreted? - You've changed the default interrupt controller to be your crossbar. Which means that all the sub-nodes are inheriting it. Have you checked that this was valid for all of these nodes? And as it has been pointed out before, you seem to be reusing an existing driver. Do you know for sure that the usage constraints are similar? M. -- Jazz is not dead. It just smells funny. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-11 9:58 ` Marc Zyngier @ 2016-06-11 15:37 ` Mason 2016-06-12 10:00 ` Marc Zyngier 2016-06-13 14:04 ` Lennart Sorensen 2016-06-13 15:15 ` Sebastian Frias 1 sibling, 2 replies; 27+ messages in thread From: Mason @ 2016-06-11 15:37 UTC (permalink / raw) To: Marc Zyngier Cc: Sebastian Frias, Thomas Gleixner, LKML, Grygorii Strashko, Mans Rullgard On 11/06/2016 11:58, Marc Zyngier wrote: > Mason wrote: > >> I think Sebastian is even more baffled by the DT mess >> (sorry, intricacies) than I am. > > This mess is what has saved us from the apocalypse 5 years ago, and > describing a complex system is not easy (what a surprise...). The problem with some Linux APIs is that they're logical and obvious to people who've been using them for years. For newcomers, it's not always so obvious. In this specific instance, the problem statement seems rather simple, on the surface. An interrupt controller, X=0..127 lines in, Y=0..23 lines out (connected to GIC interrupt lines 0..23) and "all" we need is a way to map Xs to Ys. As a first order approximation, it's enough to map all Xs to 0. And provide a way for the kernel to check the registers containing the bit-vectors indicating which interrupt(s) fired. > If you just want to apply recipes without understanding the underlying > constraints, you're in for a lot of pain. For example, the IRQ driver for Tango3/4 calls irq_find_mapping generic_handle_irq irq_desc_get_handler_data irq_desc_get_chip chained_irq_enter/chained_irq_exit irq_setup_alt_chip irq_get_domain_generic_chip irq_domain_add_linear irq_alloc_domain_generic_chips irq_set_chained_handler irq_set_handler_data Taking irq_find_mapping, I see that there's a short comment in kernel/irq/irqdomain.c /** * irq_find_mapping() - Find a linux irq from an hw irq number. * @domain: domain owning this hardware interrupt * @hwirq: hardware irq number in that domain space */ Is this Doxygen format? Is there a make target to generate some documentation? Other relevant resources, for my own reference: https://www.kernel.org/doc/Documentation/IRQ-domain.txt https://stackoverflow.com/questions/34371352/what-are-linux-irq-domains-why-are-they-needed https://community.nxp.com/thread/332183 Are there other important kernel documentation? >> The base file he was referring to is: >> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/tango4-common.dtsi > > I know which file that is, it is mentioned in the diff. I was merely > trying to point out the glaring mistakes that could be enough for a > interrupt controller hierarchy to be completely non-functional: Only the name of the file was provided, not the path. I was not aware that you already knew where to find it. I made no claim whatsoever on the implementation. In fact, I agree with everything Lennart wrote. > - Your crossbar doesn't have a #interrupt-cells property. How do you > expect the interrupt specifiers to be interpreted? Why do "fundamental" DT properties start with hash? > - You've changed the default interrupt controller to be your crossbar. > Which means that all the sub-nodes are inheriting it. Have you > checked that this was valid for all of these nodes? I'm not sure I follow. All platform interrupts flow into the platform controller. Maybe other platforms have more complex setups, with several cascaded controllers? Have a nice week end :-) ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-11 15:37 ` Mason @ 2016-06-12 10:00 ` Marc Zyngier 2016-06-12 13:50 ` Mason 2016-06-13 14:04 ` Lennart Sorensen 1 sibling, 1 reply; 27+ messages in thread From: Marc Zyngier @ 2016-06-12 10:00 UTC (permalink / raw) To: Mason Cc: Sebastian Frias, Thomas Gleixner, LKML, Grygorii Strashko, Mans Rullgard On Sat, 11 Jun 2016 17:37:51 +0200 Mason <slash.tmp@free.fr> wrote: > On 11/06/2016 11:58, Marc Zyngier wrote: > > > Mason wrote: > > > >> I think Sebastian is even more baffled by the DT mess > >> (sorry, intricacies) than I am. > > > > This mess is what has saved us from the apocalypse 5 years ago, and > > describing a complex system is not easy (what a surprise...). > > The problem with some Linux APIs is that they're logical and obvious > to people who've been using them for years. For newcomers, it's not > always so obvious. > > In this specific instance, the problem statement seems rather simple, > on the surface. An interrupt controller, X=0..127 lines in, Y=0..23 > lines out (connected to GIC interrupt lines 0..23) and "all" we need > is a way to map Xs to Ys. > > As a first order approximation, it's enough to map all Xs to 0. > And provide a way for the kernel to check the registers containing > the bit-vectors indicating which interrupt(s) fired. If that's what your hardware is, then you are taking the wrong approach. The irq-crossbar driver does not do that at all: it has x inputs and y outputs, but connects exactly *one input to one output*. No multiplexing. And the hierarchical domain infrastructure enforces a similar property: a Linux interrupt is dealt with at each level of the hierarchy without multiplexing: the "irq" is the same, while the "hwirq" varies to reflect the "input pin" for a given interrupt controller. In your particular case, you have an evolved chained interrupt controller, and nothing else. > > > If you just want to apply recipes without understanding the underlying > > constraints, you're in for a lot of pain. > > For example, the IRQ driver for Tango3/4 calls > > irq_find_mapping > generic_handle_irq > irq_desc_get_handler_data > irq_desc_get_chip > chained_irq_enter/chained_irq_exit > irq_setup_alt_chip > irq_get_domain_generic_chip > irq_domain_add_linear > irq_alloc_domain_generic_chips > irq_set_chained_handler > irq_set_handler_data > > Taking irq_find_mapping, I see that there's a short comment in > kernel/irq/irqdomain.c > > /** > * irq_find_mapping() - Find a linux irq from an hw irq number. > * @domain: domain owning this hardware interrupt > * @hwirq: hardware irq number in that domain space > */ > > Is this Doxygen format? Is there a make target to generate > some documentation? Try "make help". > > Other relevant resources, for my own reference: > > https://www.kernel.org/doc/Documentation/IRQ-domain.txt > https://stackoverflow.com/questions/34371352/what-are-linux-irq-domains-why-are-they-needed > https://community.nxp.com/thread/332183 > > Are there other important kernel documentation? > > >> The base file he was referring to is: > >> > >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/tango4-common.dtsi > > > > I know which file that is, it is mentioned in the diff. I was merely > > trying to point out the glaring mistakes that could be enough for a > > interrupt controller hierarchy to be completely non-functional: > > Only the name of the file was provided, not the path. I was not aware > that you already knew where to find it. I made no claim whatsoever on > the implementation. In fact, I agree with everything Lennart wrote. > > > - Your crossbar doesn't have a #interrupt-cells property. How do you > > expect the interrupt specifiers to be interpreted? > > Why do "fundamental" DT properties start with hash? Because # is a shorthand for "number of". > > > - You've changed the default interrupt controller to be your crossbar. > > Which means that all the sub-nodes are inheriting it. Have you > > checked that this was valid for all of these nodes? > > I'm not sure I follow. All platform interrupts flow into the platform > controller. Maybe other platforms have more complex setups, with > several cascaded controllers? Most embedded platforms do. M. -- Jazz is not dead. It just smells funny. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-12 10:00 ` Marc Zyngier @ 2016-06-12 13:50 ` Mason 2016-06-13 7:58 ` Marc Zyngier 0 siblings, 1 reply; 27+ messages in thread From: Mason @ 2016-06-12 13:50 UTC (permalink / raw) To: Marc Zyngier Cc: Sebastian Frias, Thomas Gleixner, LKML, Grygorii Strashko, Mans Rullgard On 12/06/2016 12:00, Marc Zyngier wrote: > Mason wrote: > >> The problem with some Linux APIs is that they're logical and obvious >> to people who've been using them for years. For newcomers, it's not >> always so obvious. >> >> In this specific instance, the problem statement seems rather simple, >> on the surface. An interrupt controller, X=0..127 lines in, Y=0..23 >> lines out (connected to GIC interrupt lines 0..23) and "all" we need >> is a way to map Xs to Ys. >> >> As a first order approximation, it's enough to map all Xs to 0. >> And provide a way for the kernel to check the registers containing >> the bit-vectors indicating which interrupt(s) fired. > > If that's what your hardware is, then you are taking the wrong > approach. The irq-crossbar driver does not do that at all: it has x > inputs and y outputs, but connects exactly *one input to one output*. > No multiplexing. Connecting one input to one output is possible iff x=y right? (In other words, a bijection.) > And the hierarchical domain infrastructure enforces a similar property: > a Linux interrupt is dealt with at each level of the hierarchy without > multiplexing: the "irq" is the same, while the "hwirq" varies to > reflect the "input pin" for a given interrupt controller. > > In your particular case, you have an evolved chained interrupt > controller, and nothing else. Is it possible to support such an "evolved chained intc" through DT only, or does it require a few function calls from driver code? >> Is this Doxygen format? Is there a make target to generate >> some documentation? > > Try "make help". Documentation targets: Linux kernel internal documentation in different formats: htmldocs - HTML pdfdocs - PDF psdocs - Postscript xmldocs - XML DocBook mandocs - man pages installmandocs - install man pages generated by mandocs cleandocs - clean all generated DocBook files >>> - You've changed the default interrupt controller to be your crossbar. >>> Which means that all the sub-nodes are inheriting it. Have you >>> checked that this was valid for all of these nodes? >> >> I'm not sure I follow. All platform interrupts flow into the platform >> controller. Maybe other platforms have more complex setups, with >> several cascaded controllers? > > Most embedded platforms do. My imagination is lacking, I don't see why it needs to be more complex than N platform input lines, and M output lines feeding into the GIC (with M <= N) (Our previous intc had N=64 and M=3; the new one has N=128 and M=24) Regards. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-12 13:50 ` Mason @ 2016-06-13 7:58 ` Marc Zyngier 0 siblings, 0 replies; 27+ messages in thread From: Marc Zyngier @ 2016-06-13 7:58 UTC (permalink / raw) To: Mason Cc: Sebastian Frias, Thomas Gleixner, LKML, Grygorii Strashko, Mans Rullgard On 12/06/16 14:50, Mason wrote: > On 12/06/2016 12:00, Marc Zyngier wrote: > >> Mason wrote: >> >>> The problem with some Linux APIs is that they're logical and obvious >>> to people who've been using them for years. For newcomers, it's not >>> always so obvious. >>> >>> In this specific instance, the problem statement seems rather simple, >>> on the surface. An interrupt controller, X=0..127 lines in, Y=0..23 >>> lines out (connected to GIC interrupt lines 0..23) and "all" we need >>> is a way to map Xs to Ys. >>> >>> As a first order approximation, it's enough to map all Xs to 0. >>> And provide a way for the kernel to check the registers containing >>> the bit-vectors indicating which interrupt(s) fired. >> >> If that's what your hardware is, then you are taking the wrong >> approach. The irq-crossbar driver does not do that at all: it has x >> inputs and y outputs, but connects exactly *one input to one output*. >> No multiplexing. > > Connecting one input to one output is possible iff x=y right? > (In other words, a bijection.) It is *always* possible to connect anything to anything else. You were assuming that this particular driver was fitting your particular case, and it is obvious that it is not (iow: the crossbar transformation cannot be surjective). >> And the hierarchical domain infrastructure enforces a similar property: >> a Linux interrupt is dealt with at each level of the hierarchy without >> multiplexing: the "irq" is the same, while the "hwirq" varies to >> reflect the "input pin" for a given interrupt controller. >> >> In your particular case, you have an evolved chained interrupt >> controller, and nothing else. > > Is it possible to support such an "evolved chained intc" through DT only, > or does it require a few function calls from driver code? There is no such thing as "DT only". You will have to do some actual irqchip development. >>>> - You've changed the default interrupt controller to be your crossbar. >>>> Which means that all the sub-nodes are inheriting it. Have you >>>> checked that this was valid for all of these nodes? >>> >>> I'm not sure I follow. All platform interrupts flow into the platform >>> controller. Maybe other platforms have more complex setups, with >>> several cascaded controllers? >> >> Most embedded platforms do. > > My imagination is lacking, I don't see why it needs to be more > complex than N platform input lines, and M output lines feeding > into the GIC (with M <= N) It is not more complex. It is different. M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-11 15:37 ` Mason 2016-06-12 10:00 ` Marc Zyngier @ 2016-06-13 14:04 ` Lennart Sorensen 2016-06-13 14:57 ` Sebastian Frias 1 sibling, 1 reply; 27+ messages in thread From: Lennart Sorensen @ 2016-06-13 14:04 UTC (permalink / raw) To: Mason Cc: Marc Zyngier, Sebastian Frias, Thomas Gleixner, LKML, Grygorii Strashko, Mans Rullgard On Sat, Jun 11, 2016 at 05:37:51PM +0200, Mason wrote: > Only the name of the file was provided, not the path. I was not aware > that you already knew where to find it. I made no claim whatsoever on > the implementation. In fact, I agree with everything Lennart wrote. The way the TI irq crossbar works is: You have say 200 interrupt sources in the system. You have some coprocessor (say PRU0) that you want to handle UART7, so you tell the crossbar: Please map the IRQ from UART7 (which might be input 146 on the crossbar) to IRQ 10 on the PRU0 processor. Now whenever the interrupt from UART7 fires, PRU0 will see it as IRQ10. There are no interrupt registers to check in the crossbar, it is purely virtual wires for routing interrupts. It just happens to be an enourmouse and very very flexible set of wires. If on the other hand you have 128 inputs and 24 outputs and you want all 128 inputs to work, and you have 24 128bit registers in the device to tell you which of the inputs fired to cause a given output to fire, then you in fact have an interrupt controller, not a crossbar. This would be much more similar to how the original x86 design has an i8259 connected with 8 inputs and 1 output to an input on a second i8259 (the output of the secondary chip connects to IRQ2 on the primary). You actually need an irq controller driver to go read the registers to determine which external interrupt fired and to assign a virtual IRQ number so that drivers can register for a given externel pin. Cascaded interrupt controllers like this is perfectly common on lots of systems. Now of course you could cheat and simply declare that all the inputs that are mixed to a single output are in fact sharing a single interrupt (fine if they are level triggered, could be problematic if edge triggered, depending on how it works exactly). Of course then when an interrupt happens, every driver that handles some device on a given shared interrupt will have the handler called in turn until everyone has had a change to handle the interrupt, at which point everyone should have stopped triggering it and it will clear itself, unless of course another event happened on one of the devices in which case another loop through everyone happens to handle the new events. The loop will continue until the interrupt clears, as long as some of the handlers return that they in fact did do some work to handle the interrupt event. If no one claims to have done something and the interrupt stays, you get the kernel killing the interrupt with a message about "Interrupt foo happened and nobody cared". > I'm not sure I follow. All platform interrupts flow into the platform > controller. Maybe other platforms have more complex setups, with > several cascaded controllers? I get the impression yours is not as simple as you thought either. -- len Sorensen ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-13 14:04 ` Lennart Sorensen @ 2016-06-13 14:57 ` Sebastian Frias 2016-06-13 15:42 ` Lennart Sorensen 0 siblings, 1 reply; 27+ messages in thread From: Sebastian Frias @ 2016-06-13 14:57 UTC (permalink / raw) To: Lennart Sorensen, Mason Cc: Marc Zyngier, Thomas Gleixner, LKML, Grygorii Strashko, Mans Rullgard Hi Lennart, Thanks for your input, please find my comments below. On 06/13/2016 04:04 PM, Lennart Sorensen wrote: > On Sat, Jun 11, 2016 at 05:37:51PM +0200, Mason wrote: >> Only the name of the file was provided, not the path. I was not aware >> that you already knew where to find it. I made no claim whatsoever on >> the implementation. In fact, I agree with everything Lennart wrote. > > The way the TI irq crossbar works is: > > You have say 200 interrupt sources in the system. You have some > coprocessor (say PRU0) that you want to handle UART7, so you tell the > crossbar: Please map the IRQ from UART7 (which might be input 146 on the > crossbar) to IRQ 10 on the PRU0 processor. Now whenever the interrupt > from UART7 fires, PRU0 will see it as IRQ10. There are no interrupt > registers to check in the crossbar, it is purely virtual wires for > routing interrupts. It just happens to be an enourmouse and very very > flexible set of wires. > Ok, thanks for the information. > If on the other hand you have 128 inputs and 24 outputs and you want > all 128 inputs to work, and you have 24 128bit registers in the device > to tell you which of the inputs fired to cause a given output to fire, > then you in fact have an interrupt controller, not a crossbar. Actually we have 128 inputs and 24 outputs, the 24 outputs go straight to the GIC. The HW block is a many-to-many router. There are 128 32bit registers which specify, for each of the corresponding 128 inputs, to which of the 24 outputs it would be routed to. There are 4 32bit registers that can show the RAW status of the 128 inputs, but they do not latch on the inputs. That's why our understanding is that on Linux terms it is not an interrupt controller, but just a many-to-many mux, the only real interrupt-controller (where one can set if the line is active high or low for example) is the GIC. > This would > be much more similar to how the original x86 design has an i8259 connected > with 8 inputs and 1 output to an input on a second i8259 (the output of > the secondary chip connects to IRQ2 on the primary). You actually need > an irq controller driver to go read the registers to determine which > external interrupt fired and to assign a virtual IRQ number so that > drivers can register for a given externel pin. Cascaded interrupt > controllers like this is perfectly common on lots of systems. Thanks for the background on the i8259 and the cascaded interrupts. However, our understanding is that it would only be required if more than 24 devices request IRQ lines, in which case, some of them would have to share a single GIC IRQ line, right? Shall we worry about that now? > > Now of course you could cheat and simply declare that all the inputs > that are mixed to a single output are in fact sharing a single interrupt > (fine if they are level triggered, could be problematic if edge triggered, > depending on how it works exactly). Of course then when an interrupt > happens, every driver that handles some device on a given shared interrupt > will have the handler called in turn until everyone has had a change > to handle the interrupt, at which point everyone should have stopped > triggering it and it will clear itself, unless of course another event > happened on one of the devices in which case another loop through everyone > happens to handle the new events. The loop will continue until the > interrupt clears, as long as some of the handlers return that they in fact > did do some work to handle the interrupt event. If no one claims to have > done something and the interrupt stays, you get the kernel killing the > interrupt with a message about "Interrupt foo happened and nobody cared". This is interesting. We have one interrupt controller already upstream, drivers/irqchip/irq-tango.c, and our understanding is that it dispatches one IRQ at the time, see tangox_dispatch_irqs() function, is that what you are discussing? Best regards, Sebastian ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-13 14:57 ` Sebastian Frias @ 2016-06-13 15:42 ` Lennart Sorensen 2016-06-13 15:49 ` Mason 0 siblings, 1 reply; 27+ messages in thread From: Lennart Sorensen @ 2016-06-13 15:42 UTC (permalink / raw) To: Sebastian Frias Cc: Mason, Marc Zyngier, Thomas Gleixner, LKML, Grygorii Strashko, Mans Rullgard On Mon, Jun 13, 2016 at 04:57:13PM +0200, Sebastian Frias wrote: > Actually we have 128 inputs and 24 outputs, the 24 outputs go straight to the GIC. > The HW block is a many-to-many router. > There are 128 32bit registers which specify, for each of the corresponding 128 inputs, to which of the 24 outputs it would be routed to. > > There are 4 32bit registers that can show the RAW status of the 128 inputs, but they do not latch on the inputs. > That's why our understanding is that on Linux terms it is not an interrupt controller, but just a many-to-many mux, the only real interrupt-controller (where one can set if the line is active high or low for example) is the GIC. Well that does just sound like a mux. But that does mean you either can't use more than 24 inputs at once, or you will be sharing interrupts. I really hate shared interrutps so I would never design something that way, but it is simpler. > Thanks for the background on the i8259 and the cascaded interrupts. > However, our understanding is that it would only be required if more than 24 devices request IRQ lines, in which case, some of them would have to share a single GIC IRQ line, right? > Shall we worry about that now? Well if you are sure you never need more than 24 devices registered at once, then it shouldn't be a problem. > This is interesting. > We have one interrupt controller already upstream, drivers/irqchip/irq-tango.c, and our understanding is that it dispatches one IRQ at the time, see tangox_dispatch_irqs() function, is that what you are discussing? That does look like a proper interrupt controller that could be cascaded of another one if needed. -- Len Sorensen ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-13 15:42 ` Lennart Sorensen @ 2016-06-13 15:49 ` Mason 2016-06-13 15:57 ` Marc Zyngier 2016-06-13 17:55 ` Lennart Sorensen 0 siblings, 2 replies; 27+ messages in thread From: Mason @ 2016-06-13 15:49 UTC (permalink / raw) To: Lennart Sorensen, Sebastian Frias Cc: Marc Zyngier, Thomas Gleixner, LKML, Grygorii Strashko, Mans Rullgard On 13/06/2016 17:42, Lennart Sorensen wrote: > On Mon, Jun 13, 2016 at 04:57:13PM +0200, Sebastian Frias wrote: >> Actually we have 128 inputs and 24 outputs, the 24 outputs go straight to the GIC. >> The HW block is a many-to-many router. >> There are 128 32bit registers which specify, for each of the corresponding 128 inputs, to which of the 24 outputs it would be routed to. >> >> There are 4 32bit registers that can show the RAW status of the 128 inputs, but they do not latch on the inputs. >> That's why our understanding is that on Linux terms it is not an interrupt controller, but just a many-to-many mux, the only real interrupt-controller (where one can set if the line is active high or low for example) is the GIC. > > Well that does just sound like a mux. But that does mean you either > can't use more than 24 inputs at once, or you will be sharing interrupts. > > I really hate shared interrupts so I would never design something that > way, but it is simpler. If I am not mistaken, the Cortex A9 MPCore GIC has 32 inputs. So any SoC with more than 32 devices capable of generating IRQs would have to share interrupts, right? Regards. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-13 15:49 ` Mason @ 2016-06-13 15:57 ` Marc Zyngier 2016-06-13 17:55 ` Lennart Sorensen 1 sibling, 0 replies; 27+ messages in thread From: Marc Zyngier @ 2016-06-13 15:57 UTC (permalink / raw) To: Mason, Lennart Sorensen, Sebastian Frias Cc: Thomas Gleixner, LKML, Grygorii Strashko, Mans Rullgard On 13/06/16 16:49, Mason wrote: > On 13/06/2016 17:42, Lennart Sorensen wrote: >> On Mon, Jun 13, 2016 at 04:57:13PM +0200, Sebastian Frias wrote: >>> Actually we have 128 inputs and 24 outputs, the 24 outputs go straight to the GIC. >>> The HW block is a many-to-many router. >>> There are 128 32bit registers which specify, for each of the corresponding 128 inputs, to which of the 24 outputs it would be routed to. >>> >>> There are 4 32bit registers that can show the RAW status of the 128 inputs, but they do not latch on the inputs. >>> That's why our understanding is that on Linux terms it is not an interrupt controller, but just a many-to-many mux, the only real interrupt-controller (where one can set if the line is active high or low for example) is the GIC. >> >> Well that does just sound like a mux. But that does mean you either >> can't use more than 24 inputs at once, or you will be sharing interrupts. >> >> I really hate shared interrupts so I would never design something that >> way, but it is simpler. > > If I am not mistaken, the Cortex A9 MPCore GIC has 32 inputs. Up to 224 SPIs actually, plus 16 PPIs. M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-13 15:49 ` Mason 2016-06-13 15:57 ` Marc Zyngier @ 2016-06-13 17:55 ` Lennart Sorensen 1 sibling, 0 replies; 27+ messages in thread From: Lennart Sorensen @ 2016-06-13 17:55 UTC (permalink / raw) To: Mason Cc: Sebastian Frias, Marc Zyngier, Thomas Gleixner, LKML, Grygorii Strashko, Mans Rullgard On Mon, Jun 13, 2016 at 05:49:55PM +0200, Mason wrote: > If I am not mistaken, the Cortex A9 MPCore GIC has 32 inputs. > > So any SoC with more than 32 devices capable of generating IRQs > would have to share interrupts, right? No, you simply add another interrupt controller to cascade it. That way every device can still have one interrupt it owns. One interrupt on the GIC is owned by the second interrupt controller, and its driver determines which actual external interrupt occured. It is only shared when there is no way to determine which device caused a given interrupt to happen and you have to ask every one of the devices sharing it if they caused it. -- Len Sorensen ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-11 9:58 ` Marc Zyngier 2016-06-11 15:37 ` Mason @ 2016-06-13 15:15 ` Sebastian Frias 2016-06-13 16:26 ` Marc Zyngier 1 sibling, 1 reply; 27+ messages in thread From: Sebastian Frias @ 2016-06-13 15:15 UTC (permalink / raw) To: Marc Zyngier Cc: Mason, Thomas Gleixner, LKML, Grygorii Strashko, Sricharan R, Mans Rullgard Hi Marc, Thanks for your input, please find my comments below. On 06/11/2016 11:58 AM, Marc Zyngier wrote: >> I think Sebastian is even more baffled by the DT mess >> (sorry, intricacies) than I am. > > This mess is what has saved us from the apocalypse 5 years ago, and > describing a complex system is not easy (what a surprise...). If you > just want to apply recipes without understanding the underlying > constraints, you're in for a lot of pain. Nobody said we wanted a recipe, but it would help to have examples of each of the configurations supported. Indeed, there is effort in writing the DT bindings and even Documentation/IRQ-domain.txt yet I did not see example drivers for the different configurations. Also, it is difficult to know what is the recommended way, since old APIs are kept for a while. APIs are usually extended/designed around specific cases, so there must be examples, but they are not easy to find. > >> The base file he was referring to is: >> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/tango4-common.dtsi > > I know which file that is, it is mentioned in the diff. I was merely > trying to point out the glaring mistakes that could be enough for a > interrupt controller hierarchy to be completely non-functional: > > - Your crossbar doesn't have a #interrupt-cells property. How do you > expect the interrupt specifiers to be interpreted? Thanks for the pointer, actually, after adding "#interrupt-cells = <3>;" I can see the driver getting requests for domain maps and xlates :-) > - You've changed the default interrupt controller to be your crossbar. > Which means that all the sub-nodes are inheriting it. Have you > checked that this was valid for all of these nodes? What do you mean with "valid for all these nodes"? All HW irq lines go to the crossbar. > > And as it has been pointed out before, you seem to be reusing an > existing driver. Do you know for sure that the usage constraints are > similar? It is not being reused as is, I just thought (incorrectly apparently) that it was a good example for our case. Best regards, Sebastian ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-13 15:15 ` Sebastian Frias @ 2016-06-13 16:26 ` Marc Zyngier 0 siblings, 0 replies; 27+ messages in thread From: Marc Zyngier @ 2016-06-13 16:26 UTC (permalink / raw) To: Sebastian Frias Cc: Mason, Thomas Gleixner, LKML, Grygorii Strashko, Sricharan R, Mans Rullgard On 13/06/16 16:15, Sebastian Frias wrote: >>> The base file he was referring to is: >>> >>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/tango4-common.dtsi >> >> I know which file that is, it is mentioned in the diff. I was merely >> trying to point out the glaring mistakes that could be enough for a >> interrupt controller hierarchy to be completely non-functional: >> >> - Your crossbar doesn't have a #interrupt-cells property. How do you >> expect the interrupt specifiers to be interpreted? > > Thanks for the pointer, actually, after adding "#interrupt-cells = > <3>;" I can see the driver getting requests for domain maps and > xlates :-) OK, we're getting somewhere. >> - You've changed the default interrupt controller to be your crossbar. >> Which means that all the sub-nodes are inheriting it. Have you >> checked that this was valid for all of these nodes? > > What do you mean with "valid for all these nodes"? All HW irq lines go to the crossbar. All the interrupt lines *in the system*? Or just those that are in the nodes below the "soc" section? Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-10 16:05 ` Marc Zyngier 2016-06-10 19:36 ` Mason @ 2016-06-13 15:46 ` Sebastian Frias 2016-06-13 16:24 ` Marc Zyngier 2016-06-13 17:59 ` Lennart Sorensen 1 sibling, 2 replies; 27+ messages in thread From: Sebastian Frias @ 2016-06-13 15:46 UTC (permalink / raw) To: Marc Zyngier Cc: Thomas Gleixner, LKML, Grygorii Strashko, Sricharan R, Mason, Måns Rullgård [-- Attachment #1: Type: text/plain, Size: 5491 bytes --] Hi Marc, Thanks for your reply, please find my comments below. On 06/10/2016 06:05 PM, Marc Zyngier wrote: > On 10/06/16 16:37, Sebastian Frias wrote: >> Hi, >> >> We are trying to write a driver for an interrupt controller (actually >> more of a crossbar) for an ARM-based SoC. This IRQ crossbar has 128 >> inputs and 24 outputs, the outputs are connected directly to the >> GIC. The idea is that the GIC handles everything, and just request a >> mapping from an IRQ number (0...127, from a device's DT entry) into >> one of its 24 input lines. > > "Just request a mapping...". > >> By looking at current code (4.7-rc1) there seems to be a driver >> (drivers/irqchip/irq-crossbar.c) that provides similar >> functionality. The driver uses hierarchical irq domains (since commit >> 783d31863fb8 "irqchip: crossbar: Convert dra7 crossbar to stacked >> domains") which we believe we don't need because the only controller >> is the GIC. > > So you need it, but you don't need it? The GIC may be the only interrupt > controller with which software interacts when the interrupt occurs, but > the crossbar does play a major role in *routing* the interrupt to the > right GIC pin. My understanding of "hierarchical irq domains" is that it is useful when there are multiple stacked interrupt controllers. Also, the documentation says "the majority of drivers should use the linear map" (as opposed to the hierarchical one). Maybe the definition of "interrupt controller" could benefit from some clarification, but my understanding is that, in our case, the GIC is the only interrupt controller (that's where the interrupt type must be set active_high/active_low/edge, etc.), in front of it, it happens to be a crossbar, that happens to be programmable, and that can be used to map any 128 line into any of 24 lines of the GIC (actually it is a many-to-many router, without any latching nor edge detection functionality) Obviously, when the DT says that ethernet driver uses IRQ=120 (for example), the crossbar must be setup to route line 120 to one of the 24 lines going to the GIC. So a minimum of interaction between the GIC driver (and/or the Linux IRQ framework) and the driver programming the crossbar is required, and that's what we are trying to achieve. Does that makes sense? > >> However the API used previously, register_routable_domain_ops(), was >> removed with commit a5561c3e845c "irqchip: gic: Get rid of routable >> domain". > > And every day, I thank $DEITY for having delivered us from this evil. > Really. And it wasn't much of an API. It was the son of a hack, bolted > on the side of another hack. Unmaintainable, getting in the way. I had > much fun slaughtering it! ;-) > >> Trying to use the driver with hierarchical domains (after >> modifications for our SoC), results on the kernel being blocked at >> some point: >> >> [ 0.041524] ThumbEE CPU extension supported. >> [ 0.041589] Registering SWP/SWPB emulation handler >> [ 0.052022] Freeing unused kernel memory: 12364K (c029b000 - c0eae000) >> [ 0.074084] random: dbus-uuidgen urandom read with 0 bits of entropy available > > Sorry, but that's not much of a log. Anything related to interrupts, maybe? That's the last log (it's stuck there) and I was asking how/where to enable more logs to be able to debug this. Or there are no standardised logs and every person has to come up with its own debug logs? > >> We've put logs on the different domain_ops calls (alloc, free, >> translate) but they are not called, even if the DT is supposed to >> tell devices to take interrupts from this controller (*). > > At all? Nobody is talking to the GIC? > >> Do you have suggestions on what APIs should be used, further >> reading/examples and/or pointers on how debug this (logs to enable, >> things to look for, etc.)? > > You could start with posting actual logs of an interrupt being > requested, as well as perform some basic tracing of the various > callbacks into the irqdomain and irqchip layers, all the way down to the > interrupt controllers (note the plural). Ok, thanks. In the meanwhile, and in case irq-crossbar is not the good example for our case, would it be possible to get some guidance, examples, tips, on how to write a driver like the one described? ie: [0..127] IRQ inputs => BIG_MUX => [0..23] outputs => [0..23] GIC inputs BIG_MUX is a many-to-many router: - 128x32bit registers to setup a route between any of the 128 input (IRQ_dev) and any of the 24 outputs (IRQ_gic) - 4x32bit registers that read the RAW status of each of the 128 lines (no latching nor edge detection) not sure how useful is to read such RAW status, because (naive hypothesis follows) Linux's IRQ framework could remember which IRQ_dev lines are routed to which IRQ_gic, and thus when handling IRQ_gic(x), Linux could just ask the drivers tied to it to check for interruptions. >Also, without seeing the code, > it is pretty difficult to make any meaningful comment... Base code is either 4.7rc1 or 4.4. The irq-crossbar code is not much different from TI, but you can find it attached. > > Where is the GIC? Where is the #interrupt-cells property? What is the > interrupt parent for the GIC itself? (and I'm tempted to add "What is > your name? What is you quest?", but that's because it is Friday and I > feel like I need a beer...). Anything missing on the diff can be found on the original file: arch/arm/boot/dts/tango4-common.dtsi Thanks in advance. Best regards, Sebastian [-- Attachment #2: irqv2.c --] [-- Type: text/x-csrc, Size: 9184 bytes --] #include <linux/init.h> #include <linux/irq.h> #include <linux/irqchip.h> #include <linux/irqchip/chained_irq.h> #include <linux/ioport.h> #include <linux/io.h> #include <linux/of_address.h> #include <linux/of_irq.h> #include <linux/slab.h> #define IRQ_MUX_INPUT_LINES (128) #define IRQ_MUX_OUTPUT_LINES (24) #define IRQ_FREE (-1) #define IRQ_RESERVED (-2) /** * struct tangox_irq_mux : irq mux private driver data * @lock: spinlock serializing access to @irq_map * @mux_inputs: inputs (irq lines entering) the mux * @mux_outputs: outputs (irq lines exiting) the mux (connected to the GIC) * @irq_map: irq input->output map * @reg_base: mux base address */ struct tangox_irq_mux { raw_spinlock_t lock; uint mux_inputs; uint mux_outputs; uint *irq_map; void __iomem *reg_base; }; #define DBGLOG(__format, ...) \ do { \ pr_info("[%s:%d] %s(): " __format, __FILE__, __LINE__, __FUNCTION__ , ##__VA_ARGS__); \ } while (0) static inline u32 intc_readl(int address) { u32 value = readl_relaxed((void __iomem *)address); //DBGLOG("read 0x%x @ 0x%x\n", value, address); return value; } static inline void intc_writel(int value, int address) { //DBGLOG("write 0x%x @ 0x%x\n", value, address); writel_relaxed(value, (void __iomem *)address); } static inline void tangox_setup_irq_route(struct tangox_irq_mux *irq_mux_context, int irq_in, int irq_out) { u32 value = irq_out; u32 offset = (irq_in * 4); u32 address = irq_mux_context->reg_base + offset; DBGLOG("route irq %2d (@ 0x%08x) => irq %2d\n", irq_in, address, value); if (value) value |= 0x80000000; intc_writel(value, address); } static int tangox_allocate_gic_irq(struct irq_domain *domain, unsigned virq, irq_hw_number_t hwirq) { struct tangox_irq_mux *irq_mux_context = domain->host_data; struct irq_fwspec fwspec; int i; int err; DBGLOG("domain 0x%p, virq %d (0x%x) hwirq %d (0x%x)\n", domain, virq, virq, hwirq, hwirq); if (!irq_domain_get_of_node(domain->parent)) return -EINVAL; raw_spin_lock(&(irq_mux_context->lock)); for (i = irq_mux_context->mux_outputs - 1; i >= 0; i--) { if (irq_mux_context->irq_map[i] == IRQ_FREE) { irq_mux_context->irq_map[i] = hwirq; break; } } raw_spin_unlock(&(irq_mux_context->lock)); if (i < 0) return -ENODEV; fwspec.fwnode = domain->parent->fwnode; fwspec.param_count = 3; fwspec.param[0] = 0; /* SPI */ fwspec.param[1] = i; fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH; err = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec); if (err) irq_mux_context->irq_map[i] = IRQ_FREE; else tangox_setup_irq_route(irq_mux_context, hwirq, i); return err; } static struct irq_chip mux_chip = { .name = "CBAR", .irq_eoi = irq_chip_eoi_parent, .irq_mask = irq_chip_mask_parent, .irq_unmask = irq_chip_unmask_parent, .irq_retrigger = irq_chip_retrigger_hierarchy, .irq_set_type = irq_chip_set_type_parent, .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE, #ifdef CONFIG_SMP .irq_set_affinity = irq_chip_set_affinity_parent, #endif }; /** * tangox_irq_mux_domain_alloc - map/reserve a mux<->irq connection * @domain: domain of irq to map * @virq: virq number * @nr_irqs: number of irqs to reserve * */ static int tangox_irq_mux_domain_alloc(struct irq_domain *domain, unsigned int virq, unsigned int nr_irqs, void *data) { struct tangox_irq_mux *irq_mux_context = domain->host_data; struct irq_fwspec *fwspec = data; irq_hw_number_t hwirq; int i; DBGLOG("domain 0x%p, virq %d (0x%x) nr_irqs %d\n", domain, virq, virq, nr_irqs); if (fwspec->param_count != 3) return -EINVAL; /* Not GIC compliant */ if (fwspec->param[0] != 0) return -EINVAL; /* No PPI should point to this domain */ hwirq = fwspec->param[1]; if ((hwirq + nr_irqs) > irq_mux_context->mux_inputs) return -EINVAL; /* Can't deal with this */ for (i = 0; i < nr_irqs; i++) { int err = tangox_allocate_gic_irq(domain, virq + i, hwirq + i); if (err) return err; irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i, &mux_chip, NULL); } return 0; } /** * tangox_irq_mux_domain_free - unmap/free a mux<->irq connection * @domain: domain of irq to unmap * @virq: virq number * @nr_irqs: number of irqs to free * * We do not maintain a use count of total number of map/unmap * calls for a particular irq to find out if a irq can be really * unmapped. This is because unmap is called during irq_dispose_mapping(irq), * after which irq is anyways unusable. So an explicit map has to be called * after that. */ static void tangox_irq_mux_domain_free(struct irq_domain *domain, unsigned int virq, unsigned int nr_irqs) { int i; struct tangox_irq_mux *irq_mux_context = domain->host_data; DBGLOG("domain 0x%p, virq %d (0x%x) nr_irqs %d\n", domain, virq, virq, nr_irqs); raw_spin_lock(&(irq_mux_context->lock)); for (i = 0; i < nr_irqs; i++) { struct irq_data *irqdata = irq_domain_get_irq_data(domain, virq + i); irq_domain_reset_irq_data(irqdata); irq_mux_context->irq_map[irqdata->hwirq] = IRQ_FREE; tangox_setup_irq_route(irq_mux_context, 0x0, irqdata->hwirq); } raw_spin_unlock(&(irq_mux_context->lock)); } static int tangox_irq_mux_domain_translate(struct irq_domain *domain, struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type) { DBGLOG("domain 0x%p\n", domain); if (is_of_node(fwspec->fwnode)) { if (fwspec->param_count != 3) return -EINVAL; /* No PPI should point to this domain */ if (fwspec->param[0] != 0) return -EINVAL; *hwirq = fwspec->param[1]; *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; return 0; } return -EINVAL; } static const struct irq_domain_ops tangox_irq_mux_domain_ops = { .alloc = tangox_irq_mux_domain_alloc, .free = tangox_irq_mux_domain_free, .translate = tangox_irq_mux_domain_translate, }; static int __init tangox_irq_mux_init(struct device_node *node, struct tangox_irq_mux *irq_mux_context) { int i, per_irq_reg_size, max_in, max_out, offset, entry, entry_size; const __be32 *reserved_irq_list; int ret = -ENOMEM; DBGLOG("init\n"); if (!irq_mux_context) return -ENOMEM; irq_mux_context->reg_base = of_iomap(node, 0); if (!irq_mux_context->reg_base) goto err_exit; irq_mux_context->mux_inputs = IRQ_MUX_INPUT_LINES; irq_mux_context->mux_outputs = IRQ_MUX_OUTPUT_LINES; max_in = irq_mux_context->mux_inputs; max_out = irq_mux_context->mux_outputs; irq_mux_context->irq_map = kcalloc(max_out, sizeof(int), GFP_KERNEL); if (!irq_mux_context->irq_map) goto err_unmap_base; for (i = 0; i < max_out; i++) irq_mux_context->irq_map[i] = IRQ_FREE; // mark reserved IRQ lines reserved_irq_list = of_get_property(node, "irqs-reserved", &entry_size); if (reserved_irq_list) { entry_size /= sizeof(__be32); DBGLOG("setting up %d reserved irqs\n", entry_size); for (i = 0; i < entry_size; i++) { of_property_read_u32_index(node, "irqs-reserved", i, &entry); if (entry >= max_out) { pr_err("Invalid reserved entry %d > %d: ignored\n", entry, max_out); continue; } irq_mux_context->irq_map[entry] = IRQ_RESERVED; } } DBGLOG("disabling free IRQs\n"); // disable free IRQs during initialisation for (i = 0; i < max_in; i++) { tangox_setup_irq_route(irq_mux_context, i, 0); } DBGLOG("init backward compatible map\n"); tangox_setup_irq_route(irq_mux_context, 125, 2); tangox_setup_irq_route(irq_mux_context, 126, 3); tangox_setup_irq_route(irq_mux_context, 127, 4); raw_spin_lock_init(&irq_mux_context->lock); return 0; err_free_map: kfree(irq_mux_context->irq_map); err_unmap_base: iounmap(irq_mux_context->reg_base); err_exit: return ret; } static int __init tangox_irq_mux_deinit(struct tangox_irq_mux *irq_mux_context) { if (!irq_mux_context) return -ENOMEM; if (irq_mux_context->reg_base) iounmap(irq_mux_context->reg_base); if (irq_mux_context->irq_map) kfree(irq_mux_context->irq_map); kfree(irq_mux_context); return 0; } static int __init tangox_of_irq_mux_init(struct device_node *node, struct device_node *parent) { struct tangox_irq_mux *irq_mux_context; struct irq_domain *parent_domain, *domain; int err; DBGLOG("irqv2 begin\n"); if (!parent) { pr_err("%s: no parent, giving up\n", node->full_name); return -ENODEV; } parent_domain = irq_find_host(parent); if (!parent_domain) { pr_err("%s: unable to obtain parent domain\n", node->full_name); return -ENXIO; } irq_mux_context = kzalloc(sizeof(*irq_mux_context), GFP_KERNEL); err = tangox_irq_mux_init(node, irq_mux_context); if (err) { pr_err("%s: init failed (%d)\n", node->full_name, err); tangox_irq_mux_deinit(irq_mux_context); return err; } domain = irq_domain_add_hierarchy(parent_domain, 0, irq_mux_context->mux_inputs, node, &tangox_irq_mux_domain_ops, irq_mux_context); if (!domain) { pr_err("%s: failed to allocated domain\n", node->full_name); tangox_irq_mux_deinit(irq_mux_context); return -ENOMEM; } return 0; } IRQCHIP_DECLARE(tangox_intc, "sigma,smp-irq-mux", tangox_of_irq_mux_init); ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-13 15:46 ` Sebastian Frias @ 2016-06-13 16:24 ` Marc Zyngier 2016-06-14 16:37 ` Sebastian Frias 2016-06-13 17:59 ` Lennart Sorensen 1 sibling, 1 reply; 27+ messages in thread From: Marc Zyngier @ 2016-06-13 16:24 UTC (permalink / raw) To: Sebastian Frias Cc: Thomas Gleixner, LKML, Grygorii Strashko, Sricharan R, Mason, Måns Rullgård Sebastian, On 13/06/16 16:46, Sebastian Frias wrote: > Hi Marc, > > Thanks for your reply, please find my comments below. > > On 06/10/2016 06:05 PM, Marc Zyngier wrote: >> On 10/06/16 16:37, Sebastian Frias wrote: >>> Hi, >>> >>> We are trying to write a driver for an interrupt controller (actually >>> more of a crossbar) for an ARM-based SoC. This IRQ crossbar has 128 >>> inputs and 24 outputs, the outputs are connected directly to the >>> GIC. The idea is that the GIC handles everything, and just request a >>> mapping from an IRQ number (0...127, from a device's DT entry) into >>> one of its 24 input lines. >> >> "Just request a mapping...". >> >>> By looking at current code (4.7-rc1) there seems to be a driver >>> (drivers/irqchip/irq-crossbar.c) that provides similar >>> functionality. The driver uses hierarchical irq domains (since commit >>> 783d31863fb8 "irqchip: crossbar: Convert dra7 crossbar to stacked >>> domains") which we believe we don't need because the only controller >>> is the GIC. >> >> So you need it, but you don't need it? The GIC may be the only interrupt >> controller with which software interacts when the interrupt occurs, but >> the crossbar does play a major role in *routing* the interrupt to the >> right GIC pin. > > My understanding of "hierarchical irq domains" is that it is useful > when there are multiple stacked interrupt controllers. Also, the > documentation says "the majority of drivers should use the linear > map" (as opposed to the hierarchical one). The "linear map" to be opposed to the "tree", not to the hierarchy. Hierarchies themselves can be built out most domain type (only the underlying data structure changes). > Maybe the definition of "interrupt controller" could benefit from > some clarification, but my understanding is that, in our case, the > GIC is the only interrupt controller (that's where the interrupt type > must be set active_high/active_low/edge, etc.), in front of it, it > happens to be a crossbar, that happens to be programmable, and that > can be used to map any 128 line into any of 24 lines of the GIC > (actually it is a many-to-many router, without any latching nor edge > detection functionality) An interrupt controller is absolutely *anything* that is on the interrupt path, unless it is absolutely transparent, invisible to software, and doesn't require any form of configuration. Your own definition of an interrupt controller is way too restrictive. > Obviously, when the DT says that ethernet driver uses IRQ=120 (for > example), the crossbar must be setup to route line 120 to one of the > 24 lines going to the GIC. So a minimum of interaction between the > GIC driver (and/or the Linux IRQ framework) and the driver > programming the crossbar is required, and that's what we are trying > to achieve. > > Does that makes sense? Maybe you and Mason should get together and decide what you want to support. Because you seem to have diverging requirements (Mason suggesting the exact opposite over the weekend). > >> >>> However the API used previously, register_routable_domain_ops(), was >>> removed with commit a5561c3e845c "irqchip: gic: Get rid of routable >>> domain". >> >> And every day, I thank $DEITY for having delivered us from this evil. >> Really. And it wasn't much of an API. It was the son of a hack, bolted >> on the side of another hack. Unmaintainable, getting in the way. I had >> much fun slaughtering it! ;-) >> >>> Trying to use the driver with hierarchical domains (after >>> modifications for our SoC), results on the kernel being blocked at >>> some point: >>> >>> [ 0.041524] ThumbEE CPU extension supported. >>> [ 0.041589] Registering SWP/SWPB emulation handler >>> [ 0.052022] Freeing unused kernel memory: 12364K (c029b000 - c0eae000) >>> [ 0.074084] random: dbus-uuidgen urandom read with 0 bits of entropy available >> >> Sorry, but that's not much of a log. Anything related to interrupts, maybe? > > That's the last log (it's stuck there) and I was asking how/where to enable more logs to be able to debug this. > > Or there are no standardised logs and every person has to come up > with its own debug logs? We have basic tracing in the irqdomain layer, and you can then instrument your own driver. >> >>> We've put logs on the different domain_ops calls (alloc, free, >>> translate) but they are not called, even if the DT is supposed to >>> tell devices to take interrupts from this controller (*). >> >> At all? Nobody is talking to the GIC? >> >>> Do you have suggestions on what APIs should be used, further >>> reading/examples and/or pointers on how debug this (logs to enable, >>> things to look for, etc.)? >> >> You could start with posting actual logs of an interrupt being >> requested, as well as perform some basic tracing of the various >> callbacks into the irqdomain and irqchip layers, all the way down to the >> interrupt controllers (note the plural). > > Ok, thanks. > > In the meanwhile, and in case irq-crossbar is not the good example for our case, would it be possible to get some guidance, examples, tips, on how to write a driver like the one described? ie: > > [0..127] IRQ inputs => BIG_MUX => [0..23] outputs => [0..23] GIC inputs > > BIG_MUX is a many-to-many router: > - 128x32bit registers to setup a route between any of the 128 input > (IRQ_dev) and any of the 24 outputs (IRQ_gic) > - 4x32bit registers that read the RAW status of each of the 128 > lines (no latching nor edge detection) not sure how useful is to read > such RAW status, because (naive hypothesis follows) Linux's IRQ > framework could remember which IRQ_dev lines are routed to which > IRQ_gic, and thus when handling IRQ_gic(x), Linux could just ask the > drivers tied to it to check for interruptions. > OK, so this is definitely a pure router, and the lack of latch makes it completely unsuitable for a a cascaded interrupt controller. At least, we've managed to establish that this thing will never be able to handle more than 24 devices in a sane way. So let's forget about Mason's idea of cascading everything to a single output line, and let's focus on your initial idea of having something similar to TI's crossbar, which is a much saner approach. > >> Also, without seeing the code, >> it is pretty difficult to make any meaningful comment... > > Base code is either 4.7rc1 or 4.4. > The irq-crossbar code is not much different from TI, but you can find it attached. Please post it separately (and inline), the email client I have here makes it hard to review attached patches. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-13 16:24 ` Marc Zyngier @ 2016-06-14 16:37 ` Sebastian Frias 2016-06-14 16:39 ` Sebastian Frias 0 siblings, 1 reply; 27+ messages in thread From: Sebastian Frias @ 2016-06-14 16:37 UTC (permalink / raw) To: Marc Zyngier Cc: Thomas Gleixner, LKML, Grygorii Strashko, Sricharan R, Mason, Måns Rullgård Hi Marc, On 06/13/2016 06:24 PM, Marc Zyngier wrote: >> My understanding of "hierarchical irq domains" is that it is useful >> when there are multiple stacked interrupt controllers. Also, the >> documentation says "the majority of drivers should use the linear >> map" (as opposed to the hierarchical one). > > The "linear map" to be opposed to the "tree", not to the hierarchy. > Hierarchies themselves can be built out most domain type (only the > underlying data structure changes). Thanks for the clarification. > >> Maybe the definition of "interrupt controller" could benefit from >> some clarification, but my understanding is that, in our case, the >> GIC is the only interrupt controller (that's where the interrupt type >> must be set active_high/active_low/edge, etc.), in front of it, it >> happens to be a crossbar, that happens to be programmable, and that >> can be used to map any 128 line into any of 24 lines of the GIC >> (actually it is a many-to-many router, without any latching nor edge >> detection functionality) > > An interrupt controller is absolutely *anything* that is on the > interrupt path, unless it is absolutely transparent, invisible to > software, and doesn't require any form of configuration. Well, one could imagine that this many-to-many router could be pre-configured, and thus not require any form of configuration from Linux, yet, somehow the resulting mapping needs to be communicated to Linux, right? What APIs calls should be used? >Your own > definition of an interrupt controller is way too restrictive. I see, thanks for the clarification. > >> Obviously, when the DT says that ethernet driver uses IRQ=120 (for >> example), the crossbar must be setup to route line 120 to one of the >> 24 lines going to the GIC. So a minimum of interaction between the >> GIC driver (and/or the Linux IRQ framework) and the driver >> programming the crossbar is required, and that's what we are trying >> to achieve. >> >> Does that makes sense? > > Maybe you and Mason should get together and decide what you want to > support. Because you seem to have diverging requirements (Mason > suggesting the exact opposite over the weekend). IIUC what you refer to, when Mason said that we could route all 128 lines to a single GIC line he was talking about a first order approximation. >> >> That's the last log (it's stuck there) and I was asking how/where to enable more logs to be able to debug this. >> >> Or there are no standardised logs and every person has to come up >> with its own debug logs? > > We have basic tracing in the irqdomain layer, I followed this https://www.kernel.org/doc/local/pr_debug.txt for kernel/irq/irqdomain.c Do you want the whole boot log? Or just the snippets from irqdomain.c and those from our driver? >and you can then > instrument your own driver. > >> >> In the meanwhile, and in case irq-crossbar is not the good example for our case, would it be possible to get some guidance, examples, tips, on how to write a driver like the one described? ie: >> >> [0..127] IRQ inputs => BIG_MUX => [0..23] outputs => [0..23] GIC inputs >> >> BIG_MUX is a many-to-many router: >> - 128x32bit registers to setup a route between any of the 128 input >> (IRQ_dev) and any of the 24 outputs (IRQ_gic) >> - 4x32bit registers that read the RAW status of each of the 128 >> lines (no latching nor edge detection) not sure how useful is to read >> such RAW status, because (naive hypothesis follows) Linux's IRQ >> framework could remember which IRQ_dev lines are routed to which >> IRQ_gic, and thus when handling IRQ_gic(x), Linux could just ask the >> drivers tied to it to check for interruptions. >> > > OK, so this is definitely a pure router, and the lack of latch makes it > completely unsuitable for a a cascaded interrupt controller. At least, > we've managed to establish that this thing will never be able to handle > more than 24 devices in a sane way. So let's forget about Mason's idea > of cascading everything to a single output line, and let's focus on your > initial idea of having something similar to TI's crossbar, which is a > much saner approach. Ok, so after discussing with some HW engineers, they said that even if this is a pure router and cannot latch by itself, since the devices themselves latch their IRQ output, reading the 4x32bit RAW status registers could work as well, that means that if there are more than 24 devices, some could share IRQs, right? Two questions then: a) let's say we need to share some of the IRQs, which APIs should be used? b) some people have been asking about IRQ affinity, they have not been clear about it, but I suppose maybe they want to redistribute the IRQs. In this case, when using IRQ sharing a device may go from sharing an IRQ line to an exclusive line or viceversa, right? Does Linux handles that on its own or there's some API to call as well? > >> >>> Also, without seeing the code, >>> it is pretty difficult to make any meaningful comment... >> >> Base code is either 4.7rc1 or 4.4. >> The irq-crossbar code is not much different from TI, but you can find it attached. > > Please post it separately (and inline), the email client I have here > makes it hard to review attached patches. Ok, I'll post it in a separate email and inline. Best regards, Sebastian ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-14 16:37 ` Sebastian Frias @ 2016-06-14 16:39 ` Sebastian Frias 2016-06-16 12:39 ` Sebastian Frias 0 siblings, 1 reply; 27+ messages in thread From: Sebastian Frias @ 2016-06-14 16:39 UTC (permalink / raw) To: Marc Zyngier Cc: Thomas Gleixner, LKML, Grygorii Strashko, Sricharan R, Mason, Måns Rullgård On 06/14/2016 06:37 PM, Sebastian Frias wrote: >>>> Also, without seeing the code, >>>> it is pretty difficult to make any meaningful comment... >>> >>> Base code is either 4.7rc1 or 4.4. >>> The irq-crossbar code is not much different from TI, but you can find it attached. >> >> Please post it separately (and inline), the email client I have here >> makes it hard to review attached patches. > > Ok, I'll post it in a separate email and inline. > Here it goes: #include <linux/init.h> #include <linux/irq.h> #include <linux/irqchip.h> #include <linux/irqchip/chained_irq.h> #include <linux/ioport.h> #include <linux/io.h> #include <linux/of_address.h> #include <linux/of_irq.h> #include <linux/slab.h> #define IRQ_MUX_INPUT_LINES (128) #define IRQ_MUX_OUTPUT_LINES (24) #define IRQ_FREE (-1) #define IRQ_RESERVED (-2) /** * struct tangox_irq_mux : irq mux private driver data * @lock: spinlock serializing access to @irq_map * @mux_inputs: inputs (irq lines entering) the mux * @mux_outputs: outputs (irq lines exiting) the mux (connected to the GIC) * @irq_map: irq input->output map * @reg_base: mux base address */ struct tangox_irq_mux { raw_spinlock_t lock; uint mux_inputs; uint mux_outputs; uint *irq_map; void __iomem *reg_base; }; #define DBGLOG(__format, ...) \ do { \ pr_info("[%s:%d] %s(): " __format, __FILE__, __LINE__, __FUNCTION__ , ##__VA_ARGS__); \ } while (0) static inline u32 intc_readl(int address) { u32 value = readl_relaxed((void __iomem *)address); //DBGLOG("read 0x%x @ 0x%x\n", value, address); return value; } static inline void intc_writel(int value, int address) { //DBGLOG("write 0x%x @ 0x%x\n", value, address); writel_relaxed(value, (void __iomem *)address); } static inline void tangox_setup_irq_route(struct tangox_irq_mux *irq_mux_context, int irq_in, int irq_out) { u32 value = irq_out; u32 offset = (irq_in * 4); u32 address = irq_mux_context->reg_base + offset; DBGLOG("route irq %2d (@ 0x%08x) => irq %2d\n", irq_in, address, value); if (value) value |= 0x80000000; intc_writel(value, address); } static int tangox_allocate_gic_irq(struct irq_domain *domain, unsigned virq, irq_hw_number_t hwirq) { struct tangox_irq_mux *irq_mux_context = domain->host_data; struct irq_fwspec fwspec; int i; int err; DBGLOG("domain 0x%p, virq %d (0x%x) hwirq %d (0x%x)\n", domain, virq, virq, hwirq, hwirq); if (!irq_domain_get_of_node(domain->parent)) return -EINVAL; raw_spin_lock(&(irq_mux_context->lock)); for (i = irq_mux_context->mux_outputs - 1; i >= 0; i--) { if (irq_mux_context->irq_map[i] == IRQ_FREE) { irq_mux_context->irq_map[i] = hwirq; break; } } raw_spin_unlock(&(irq_mux_context->lock)); if (i < 0) return -ENODEV; fwspec.fwnode = domain->parent->fwnode; fwspec.param_count = 3; fwspec.param[0] = 0; /* SPI */ fwspec.param[1] = i; fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH; err = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec); if (err) irq_mux_context->irq_map[i] = IRQ_FREE; else tangox_setup_irq_route(irq_mux_context, hwirq, i); return err; } static struct irq_chip mux_chip = { .name = "CBAR", .irq_eoi = irq_chip_eoi_parent, .irq_mask = irq_chip_mask_parent, .irq_unmask = irq_chip_unmask_parent, .irq_retrigger = irq_chip_retrigger_hierarchy, .irq_set_type = irq_chip_set_type_parent, .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE, #ifdef CONFIG_SMP .irq_set_affinity = irq_chip_set_affinity_parent, #endif }; /** * tangox_irq_mux_domain_alloc - map/reserve a mux<->irq connection * @domain: domain of irq to map * @virq: virq number * @nr_irqs: number of irqs to reserve * */ static int tangox_irq_mux_domain_alloc(struct irq_domain *domain, unsigned int virq, unsigned int nr_irqs, void *data) { struct tangox_irq_mux *irq_mux_context = domain->host_data; struct irq_fwspec *fwspec = data; irq_hw_number_t hwirq; int i; DBGLOG("domain 0x%p, virq %d (0x%x) nr_irqs %d\n", domain, virq, virq, nr_irqs); if (fwspec->param_count != 3) return -EINVAL; /* Not GIC compliant */ if (fwspec->param[0] != 0) return -EINVAL; /* No PPI should point to this domain */ hwirq = fwspec->param[1]; if ((hwirq + nr_irqs) > irq_mux_context->mux_inputs) return -EINVAL; /* Can't deal with this */ for (i = 0; i < nr_irqs; i++) { int err = tangox_allocate_gic_irq(domain, virq + i, hwirq + i); if (err) return err; irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i, &mux_chip, NULL); } return 0; } /** * tangox_irq_mux_domain_free - unmap/free a mux<->irq connection * @domain: domain of irq to unmap * @virq: virq number * @nr_irqs: number of irqs to free * * We do not maintain a use count of total number of map/unmap * calls for a particular irq to find out if a irq can be really * unmapped. This is because unmap is called during irq_dispose_mapping(irq), * after which irq is anyways unusable. So an explicit map has to be called * after that. */ static void tangox_irq_mux_domain_free(struct irq_domain *domain, unsigned int virq, unsigned int nr_irqs) { int i; struct tangox_irq_mux *irq_mux_context = domain->host_data; DBGLOG("domain 0x%p, virq %d (0x%x) nr_irqs %d\n", domain, virq, virq, nr_irqs); raw_spin_lock(&(irq_mux_context->lock)); for (i = 0; i < nr_irqs; i++) { struct irq_data *irqdata = irq_domain_get_irq_data(domain, virq + i); irq_domain_reset_irq_data(irqdata); irq_mux_context->irq_map[irqdata->hwirq] = IRQ_FREE; tangox_setup_irq_route(irq_mux_context, 0x0, irqdata->hwirq); } raw_spin_unlock(&(irq_mux_context->lock)); } static int tangox_irq_mux_domain_translate(struct irq_domain *domain, struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type) { DBGLOG("domain 0x%p\n", domain); if (is_of_node(fwspec->fwnode)) { if (fwspec->param_count != 3) return -EINVAL; /* No PPI should point to this domain */ if (fwspec->param[0] != 0) return -EINVAL; *hwirq = fwspec->param[1]; *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; return 0; } return -EINVAL; } static const struct irq_domain_ops tangox_irq_mux_domain_ops = { .alloc = tangox_irq_mux_domain_alloc, .free = tangox_irq_mux_domain_free, .translate = tangox_irq_mux_domain_translate, }; static int __init tangox_irq_mux_init(struct device_node *node, struct tangox_irq_mux *irq_mux_context) { int i, per_irq_reg_size, max_in, max_out, offset, entry, entry_size; const __be32 *reserved_irq_list; int ret = -ENOMEM; DBGLOG("init\n"); if (!irq_mux_context) return -ENOMEM; irq_mux_context->reg_base = of_iomap(node, 0); if (!irq_mux_context->reg_base) goto err_exit; irq_mux_context->mux_inputs = IRQ_MUX_INPUT_LINES; irq_mux_context->mux_outputs = IRQ_MUX_OUTPUT_LINES; max_in = irq_mux_context->mux_inputs; max_out = irq_mux_context->mux_outputs; irq_mux_context->irq_map = kcalloc(max_out, sizeof(int), GFP_KERNEL); if (!irq_mux_context->irq_map) goto err_unmap_base; for (i = 0; i < max_out; i++) irq_mux_context->irq_map[i] = IRQ_FREE; // mark reserved IRQ lines reserved_irq_list = of_get_property(node, "irqs-reserved", &entry_size); if (reserved_irq_list) { entry_size /= sizeof(__be32); DBGLOG("setting up %d reserved irqs\n", entry_size); for (i = 0; i < entry_size; i++) { of_property_read_u32_index(node, "irqs-reserved", i, &entry); if (entry >= max_out) { pr_err("Invalid reserved entry %d > %d: ignored\n", entry, max_out); continue; } irq_mux_context->irq_map[entry] = IRQ_RESERVED; } } DBGLOG("disabling free IRQs\n"); // disable free IRQs during initialisation for (i = 0; i < max_in; i++) { tangox_setup_irq_route(irq_mux_context, i, 0); } DBGLOG("init backward compatible map\n"); tangox_setup_irq_route(irq_mux_context, 125, 2); tangox_setup_irq_route(irq_mux_context, 126, 3); tangox_setup_irq_route(irq_mux_context, 127, 4); raw_spin_lock_init(&irq_mux_context->lock); return 0; err_free_map: kfree(irq_mux_context->irq_map); err_unmap_base: iounmap(irq_mux_context->reg_base); err_exit: return ret; } static int __init tangox_irq_mux_deinit(struct tangox_irq_mux *irq_mux_context) { if (!irq_mux_context) return -ENOMEM; if (irq_mux_context->reg_base) iounmap(irq_mux_context->reg_base); if (irq_mux_context->irq_map) kfree(irq_mux_context->irq_map); kfree(irq_mux_context); return 0; } static int __init tangox_of_irq_mux_init(struct device_node *node, struct device_node *parent) { struct tangox_irq_mux *irq_mux_context; struct irq_domain *parent_domain, *domain; int err; DBGLOG("irqv2 begin\n"); if (!parent) { pr_err("%s: no parent, giving up\n", node->full_name); return -ENODEV; } parent_domain = irq_find_host(parent); if (!parent_domain) { pr_err("%s: unable to obtain parent domain\n", node->full_name); return -ENXIO; } irq_mux_context = kzalloc(sizeof(*irq_mux_context), GFP_KERNEL); err = tangox_irq_mux_init(node, irq_mux_context); if (err) { pr_err("%s: init failed (%d)\n", node->full_name, err); tangox_irq_mux_deinit(irq_mux_context); return err; } domain = irq_domain_add_hierarchy(parent_domain, 0, irq_mux_context->mux_inputs, node, &tangox_irq_mux_domain_ops, irq_mux_context); if (!domain) { pr_err("%s: failed to allocated domain\n", node->full_name); tangox_irq_mux_deinit(irq_mux_context); return -ENOMEM; } return 0; } IRQCHIP_DECLARE(tangox_intc, "sigma,smp-irq-mux", tangox_of_irq_mux_init); ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-14 16:39 ` Sebastian Frias @ 2016-06-16 12:39 ` Sebastian Frias 2016-06-21 10:18 ` Marc Zyngier 0 siblings, 1 reply; 27+ messages in thread From: Sebastian Frias @ 2016-06-16 12:39 UTC (permalink / raw) To: Marc Zyngier Cc: Thomas Gleixner, LKML, Grygorii Strashko, Mason, Måns Rullgård Hi Marc, On 06/14/2016 06:39 PM, Sebastian Frias wrote: > On 06/14/2016 06:37 PM, Sebastian Frias wrote: >>>>> Also, without seeing the code, >>>>> it is pretty difficult to make any meaningful comment... >>>> >>>> Base code is either 4.7rc1 or 4.4. >>>> The irq-crossbar code is not much different from TI, but you can find it attached. >>> >>> Please post it separately (and inline), the email client I have here >>> makes it hard to review attached patches. >> >> Ok, I'll post it in a separate email and inline. >> > > Here it goes: > > <snipped code> > IRQCHIP_DECLARE(tangox_intc, "sigma,smp-irq-mux", tangox_of_irq_mux_init); > > I have tested the code, and aside from the missing #interrupt-cells in the DT that you pointed out, it seems it is working (devices using IRQ appear functional), here's some log: tangox_irq_mux_domain_translate(): domain 0xcf805000 tangox_irq_mux_domain_translate(): hwirq 1 (0x1) type 4 (0x4) tangox_irq_mux_domain_alloc(): domain 0xcf805000, virq 18 (0x12) nr_irqs 1 tangox_allocate_gic_irq(): domain 0xcf805000, virq 18 (0x12) hwirq 1 (0x1) tangox_setup_irq_route(): route irq 1 (@ 0xf006f804) => irq 23 tangox_irq_mux_domain_translate(): domain 0xcf805000 tangox_irq_mux_domain_translate(): hwirq 38 (0x26) type 4 (0x4) tangox_irq_mux_domain_alloc(): domain 0xcf805000, virq 19 (0x13) nr_irqs 1 tangox_allocate_gic_irq(): domain 0xcf805000, virq 19 (0x13) hwirq 38 (0x26) tangox_setup_irq_route(): route irq 38 (@ 0xf006f898) => irq 22 tangox_irq_mux_domain_translate(): domain 0xcf805000 tangox_irq_mux_domain_translate(): hwirq 67 (0x43) type 4 (0x4) tangox_irq_mux_domain_alloc(): domain 0xcf805000, virq 20 (0x14) nr_irqs 1 tangox_allocate_gic_irq(): domain 0xcf805000, virq 20 (0x14) hwirq 67 (0x43) tangox_setup_irq_route(): route irq 67 (@ 0xf006f90c) => irq 21 Since irq-tango_v2.c is similar to irq-crossbar.c from TI (since it is based on it), I was wondering what is the policy or recommendation in such cases? Should I attempt to merge the code (mainly the way to set up the registers) on irq-crossbar.c or should we consider irq-tango_v2.c to live its own life? NOTE: IMHO, irq-crossbar.c could benefit from clearer names for some DT properties, because "max_irqs" and "max-crossbar-sources" are not straight forward names for "mux_outputs" and "mux_inputs" (respectively) NOTE2: current irq-tango_v2.c code still has a 24 IRQ limitation since it is not using any API that would allow it to setup IRQ sharing. Thanks in advance. Best regards, Sebastian ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-16 12:39 ` Sebastian Frias @ 2016-06-21 10:18 ` Marc Zyngier 2016-06-21 11:03 ` Sebastian Frias 0 siblings, 1 reply; 27+ messages in thread From: Marc Zyngier @ 2016-06-21 10:18 UTC (permalink / raw) To: Sebastian Frias Cc: Thomas Gleixner, LKML, Grygorii Strashko, Mason, Måns Rullgård [been away for a while, catching up...] On 16/06/16 13:39, Sebastian Frias wrote: > Hi Marc, > > On 06/14/2016 06:39 PM, Sebastian Frias wrote: >> On 06/14/2016 06:37 PM, Sebastian Frias wrote: >>>>>> Also, without seeing the code, >>>>>> it is pretty difficult to make any meaningful comment... >>>>> >>>>> Base code is either 4.7rc1 or 4.4. >>>>> The irq-crossbar code is not much different from TI, but you can find it attached. >>>> >>>> Please post it separately (and inline), the email client I have here >>>> makes it hard to review attached patches. >>> >>> Ok, I'll post it in a separate email and inline. >>> >> >> Here it goes: >> >> > > <snipped code> > >> IRQCHIP_DECLARE(tangox_intc, "sigma,smp-irq-mux", tangox_of_irq_mux_init); >> >> > > I have tested the code, and aside from the missing #interrupt-cells > in the DT that you pointed out, it seems it is working (devices using > IRQ appear functional), here's some log: > > tangox_irq_mux_domain_translate(): domain 0xcf805000 > tangox_irq_mux_domain_translate(): hwirq 1 (0x1) type 4 (0x4) > tangox_irq_mux_domain_alloc(): domain 0xcf805000, virq 18 (0x12) nr_irqs 1 > tangox_allocate_gic_irq(): domain 0xcf805000, virq 18 (0x12) hwirq 1 (0x1) > tangox_setup_irq_route(): route irq 1 (@ 0xf006f804) => irq 23 > tangox_irq_mux_domain_translate(): domain 0xcf805000 > tangox_irq_mux_domain_translate(): hwirq 38 (0x26) type 4 (0x4) > tangox_irq_mux_domain_alloc(): domain 0xcf805000, virq 19 (0x13) nr_irqs 1 > tangox_allocate_gic_irq(): domain 0xcf805000, virq 19 (0x13) hwirq 38 (0x26) > tangox_setup_irq_route(): route irq 38 (@ 0xf006f898) => irq 22 > tangox_irq_mux_domain_translate(): domain 0xcf805000 > tangox_irq_mux_domain_translate(): hwirq 67 (0x43) type 4 (0x4) > tangox_irq_mux_domain_alloc(): domain 0xcf805000, virq 20 (0x14) nr_irqs 1 > tangox_allocate_gic_irq(): domain 0xcf805000, virq 20 (0x14) hwirq 67 (0x43) > tangox_setup_irq_route(): route irq 67 (@ 0xf006f90c) => irq 21 > > Since irq-tango_v2.c is similar to irq-crossbar.c from TI (since it > is based on it), I was wondering what is the policy or recommendation > in such cases? > Should I attempt to merge the code (mainly the way to set up the > registers) on irq-crossbar.c or should we consider irq-tango_v2.c to > live its own life? If the HW is significantly different, I'd rather have a separate driver. We can always share some things later on by having a small library of "stuff". > NOTE: IMHO, irq-crossbar.c could benefit from clearer names for some > DT properties, because "max_irqs" and "max-crossbar-sources" are not > straight forward names for "mux_outputs" and "mux_inputs" > (respectively) Maybe, but this ship has sailed a long time ago. This is an ABI now, and it is not going to change unless proven to be broken. On the other hand, you can name your own properties as you see fit. > NOTE2: current irq-tango_v2.c code still has a 24 IRQ limitation > since it is not using any API that would allow it to setup IRQ > sharing. Unless you limit your mux level interrupts only, I cannot see how you could deal with cascaded interrupts. By the time you receive an edge, the line will have dropped, and you won't be able to identify the source interrupt. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-21 10:18 ` Marc Zyngier @ 2016-06-21 11:03 ` Sebastian Frias 2016-06-21 12:41 ` Marc Zyngier 0 siblings, 1 reply; 27+ messages in thread From: Sebastian Frias @ 2016-06-21 11:03 UTC (permalink / raw) To: Marc Zyngier Cc: Thomas Gleixner, LKML, Grygorii Strashko, Mason, Måns Rullgård Hi Marc, On 06/21/2016 12:18 PM, Marc Zyngier wrote: >> Since irq-tango_v2.c is similar to irq-crossbar.c from TI (since it >> is based on it), I was wondering what is the policy or recommendation >> in such cases? >> Should I attempt to merge the code (mainly the way to set up the >> registers) on irq-crossbar.c or should we consider irq-tango_v2.c to >> live its own life? > > If the HW is significantly different, I'd rather have a separate driver. > We can always share some things later on by having a small library of > "stuff". I'd say it is very similar. Most of the changes I did were done to understand how it worked. However, it may end up being different if we use cascaded interrupts. > >> NOTE: IMHO, irq-crossbar.c could benefit from clearer names for some >> DT properties, because "max_irqs" and "max-crossbar-sources" are not >> straight forward names for "mux_outputs" and "mux_inputs" >> (respectively) > > Maybe, but this ship has sailed a long time ago. This is an ABI now, and > it is not going to change unless proven to be broken. On the other hand, > you can name your own properties as you see fit. Ok. > >> NOTE2: current irq-tango_v2.c code still has a 24 IRQ limitation >> since it is not using any API that would allow it to setup IRQ >> sharing. > > Unless you limit your mux level interrupts only, I cannot see how you > could deal with cascaded interrupts. By the time you receive an edge, > the line will have dropped, and you won't be able to identify the source > interrupt. Yes, cascaded interrupts would be limited to level only. By the way, did you see my other questions? (copy/pasted here for convenience): ---- Ok, so after discussing with some HW engineers, they said that even if this is a pure router and cannot latch by itself, since the devices themselves latch their IRQ output, reading the 4x32bit RAW status registers could work as well, that means that if there are more than 24 devices, some could share IRQs, right? Two questions then: a) let's say we need to share some of the IRQs, which APIs should be used? b) some people have been asking about IRQ affinity, they have not been clear about it, but I suppose maybe they want to redistribute the IRQs. In this case, when using IRQ sharing a device may go from sharing an IRQ line to an exclusive line or viceversa, right? Does Linux handles that on its own or there's some API to call as well? ---- About a) I did not find any driver that uses irq_domain_add_linear() and irq_domain_add_hierarchy() but maybe I'm not approaching the problem from the right angle. Best regards, Sebastian ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-21 11:03 ` Sebastian Frias @ 2016-06-21 12:41 ` Marc Zyngier 2016-06-21 15:29 ` Sebastian Frias 0 siblings, 1 reply; 27+ messages in thread From: Marc Zyngier @ 2016-06-21 12:41 UTC (permalink / raw) To: Sebastian Frias Cc: Thomas Gleixner, LKML, Grygorii Strashko, Mason, Måns Rullgård On 21/06/16 12:03, Sebastian Frias wrote: > Hi Marc, > > On 06/21/2016 12:18 PM, Marc Zyngier wrote: >>> Since irq-tango_v2.c is similar to irq-crossbar.c from TI (since it >>> is based on it), I was wondering what is the policy or recommendation >>> in such cases? >>> Should I attempt to merge the code (mainly the way to set up the >>> registers) on irq-crossbar.c or should we consider irq-tango_v2.c to >>> live its own life? >> >> If the HW is significantly different, I'd rather have a separate driver. >> We can always share some things later on by having a small library of >> "stuff". > > I'd say it is very similar. Most of the changes I did were done to understand how it worked. > However, it may end up being different if we use cascaded interrupts. > >> >>> NOTE: IMHO, irq-crossbar.c could benefit from clearer names for some >>> DT properties, because "max_irqs" and "max-crossbar-sources" are not >>> straight forward names for "mux_outputs" and "mux_inputs" >>> (respectively) >> >> Maybe, but this ship has sailed a long time ago. This is an ABI now, and >> it is not going to change unless proven to be broken. On the other hand, >> you can name your own properties as you see fit. > > Ok. > >> >>> NOTE2: current irq-tango_v2.c code still has a 24 IRQ limitation >>> since it is not using any API that would allow it to setup IRQ >>> sharing. >> >> Unless you limit your mux level interrupts only, I cannot see how you >> could deal with cascaded interrupts. By the time you receive an edge, >> the line will have dropped, and you won't be able to identify the source >> interrupt. > > Yes, cascaded interrupts would be limited to level only. > > By the way, did you see my other questions? (copy/pasted here for convenience): > > ---- > Ok, so after discussing with some HW engineers, they said that even > if this is a pure router and cannot latch by itself, since the > devices themselves latch their IRQ output, reading the 4x32bit RAW > status registers could work as well, that means that if there are > more than 24 devices, some could share IRQs, right? As mentioned earlier, this only works for level interrupts. If you enforce this, this is OK. I assume that you also have a way to mask these interrupts, right? > Two questions then: > a) let's say we need to share some of the IRQs, which APIs should be used? The usual irq_set_chained_handler_and_data()/chained_irq_enter()/chained_irq_exit(). > b) some people have been asking about IRQ affinity, they have not > been clear about it, but I suppose maybe they want to redistribute > the IRQs. In this case, when using IRQ sharing a device may go from > sharing an IRQ line to an exclusive line or viceversa, right? Does > Linux handles that on its own or there's some API to call as well? You need to implement the .irq_set_affinity in the irqchip. But it hardly makes sense in your particular case: - if you're using it as a pure router (no multiplexing), the affinity is controlled by the the GIC itself (i.e nothing to do here, except forwarding the request to the underlying irqchip). - If you're using it as a chained irqchip, then you can't easily migrate a bunch of interrupt, because this is not what userspace expects. What you could do would be to dedicate an irq line per CPU, and reconfigure the mux when changing the affinity. > About a) I did not find any driver that uses irq_domain_add_linear() > and irq_domain_add_hierarchy() but maybe I'm not approaching the > problem from the right angle. A chained interrupt controller cannot be hierarchical. That's pretty fundamental. Overall, I think you need to settle for a use case (either pure router or chained controller) and implement that. To support both, you'll need two different implementations (basically two drivers in one). Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-21 12:41 ` Marc Zyngier @ 2016-06-21 15:29 ` Sebastian Frias 0 siblings, 0 replies; 27+ messages in thread From: Sebastian Frias @ 2016-06-21 15:29 UTC (permalink / raw) To: Marc Zyngier Cc: Thomas Gleixner, LKML, Grygorii Strashko, Mason, Måns Rullgård Hi Marc, On 06/21/2016 02:41 PM, Marc Zyngier wrote: >> Ok, so after discussing with some HW engineers, they said that even >> if this is a pure router and cannot latch by itself, since the >> devices themselves latch their IRQ output, reading the 4x32bit RAW >> status registers could work as well, that means that if there are >> more than 24 devices, some could share IRQs, right? > > As mentioned earlier, this only works for level interrupts. If you > enforce this, this is OK. I assume that you also have a way to mask > these interrupts, right? Yes, that's what the bit 31 does (see tangox_setup_irq_route() function in the code I sent). For reference, here's the documentation of the mux: ---- CPU block interrupt interface is now 32bits. The 24 first interrupt bits are generated from the system interrupts and the 8 msb interrupts are cpu local interrupts : IRQs [23:0] tango system irqs. IRQs [27:24] CPU core cross trigger interface interrupt (1 per core). IRQs [31:28] CPU core PMU (performance unit) interrupt (1 per core). The 24 lsb interrupts are generated through a new interrupt map module that maps the tango 128 interrupts to those 24 interrupts. For each of the 128 input system interrupt, one register is dedicated to program the destination interrupt among the 24 available. The mapper is configured as follows, starting at address (0x6f800) : offset name description 0x000 irq_in_0_cfg "en"=bit[31]; "inv"=bit[16]; "dest"=bits[4:0] 0x004 irq_in_1_cfg "en"=bit[31]; "inv"=bit[16]; "dest"=bits[4:0] . . . 0x1FC irq_in_127_cfg "en"=bit[31]; "inv"=bit[16]; "dest"=bits[4:0] 0x200 soft_irq_cfg "enable"=bits[15:0] 0x204 soft_irq_map0 "map3"=bits[28:24]; "map2"=bits[20:16]; "map1"=bits[12:8]; "map0"=bits[4:0] 0x208 soft_irq_map1 "map3"=bits[28:24]; "map2"=bits[20:16]; "map1"=bits[12:8]; "map0"=bits[4:0] 0x20C soft_irq_map2 "map3"=bits[28:24]; "map2"=bits[20:16]; "map1"=bits[12:8]; "map0"=bits[4:0] 0x210 soft_irq_map3 "map3"=bits[28:24]; "map2"=bits[20:16]; "map1"=bits[12:8]; "map0"=bits[4:0] 0x214 soft_irq_set "set"=bits[15:0] 0x218 soft_irq_clear "clear"=bits[15:0] 0x21C read_cpu_irq "cpu_block_irq"=bits[23:0] 0x220 read_sys_irq0 "system_irq"=bits[31:0]; (irqs: 0->31) 0x224 read_sys_irq1 "system_irq"=bits[31:0]; (irqs: 32->63) 0x228 read_sys_irq2 "system_irq"=bits[31:0]; (irqs: 64->95) 0x22C read_sys_irq3 "system_irq"=bits[31:0]; (irqs: 96->127) irq_in_N_cfg : input N mapping : - dest bits[4:0] => set destination interrupt among the 24 output interrupts. (if multiple inputs are mapped to the same output, result is an OR of the inputs). - inv bit[16] => if set, inverts input interrupt polarity (active at 0). - en bit[31] => enable interrupt. Acts like a mask on the input interrupt. soft_irq : this module supports up to 16 software interrupts. - enable bits[15:0] => enable usage of software IRQs (SIRQ), 1 bit per SIRQ. soft_irq_mapN : For each of the 16 soft IRQ (SIRQ), map them in out IRQ[23:0] vector. - mapN => 5 bits to select where to connect the SIRQ among the 23 bits output IRQ. (if multiple SIRQ are mapped to the same output IRQ, result is an OR of those signals). soft_irq_set : 16bits, write 1 bit at one set the corresponding SIRQ. Read returns the software SIRQ vector value. soft_irq_clear : 16bits, write 1 bit at one clear the corresponding software SIRQ. Read returns the software SIRQ vector value. read_cpu_irq : 24bits, returns output IRQ value (IRQs connected to the ARM cluster). read_sys_irqN : 32bits, returns input system IRQ value before mapping. > >> Two questions then: >> a) let's say we need to share some of the IRQs, which APIs should be used? > > The usual > irq_set_chained_handler_and_data()/chained_irq_enter()/chained_irq_exit(). Ok, thanks. At first I thought that I could modify tangox_allocate_gic_irq() so that when running out of IRQ lines going to the GIC, it: - reserved the last line of the GIC with: irq = fwspec.param[1]; err = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec); - then, it could add an irqchip to the domain with: err = irq_alloc_domain_generic_chips(domain, 128, 1, node->name, handle_level_irq, 0, 0, 0); for (i = 0; i < 4; i++) { gc = irq_get_domain_generic_chip(domain, i * 32); tangox_irq_init_chip(gc, i * IRQ_CTL_HI, i * EDGE_CTL_HI); } irq_set_chained_handler(irq, tangox_irq_handler); irq_set_handler_data(irq, domain); Not sure if that makes sense (it's hard to get a clear understanding of all these APIs and their possible interactions) > >> b) some people have been asking about IRQ affinity, they have not >> been clear about it, but I suppose maybe they want to redistribute >> the IRQs. In this case, when using IRQ sharing a device may go from >> sharing an IRQ line to an exclusive line or viceversa, right? Does >> Linux handles that on its own or there's some API to call as well? > > You need to implement the .irq_set_affinity in the irqchip. But it > hardly makes sense in your particular case: > - if you're using it as a pure router (no multiplexing), the affinity is > controlled by the the GIC itself (i.e nothing to do here, except > forwarding the request to the underlying irqchip). Ok, that confirms my guess from seeing that the .irq_set_affinity callback is set to a framework callback ('irq_chip_set_affinity_parent') in irq-crossbar.c > - If you're using it as a chained irqchip, then you can't easily migrate > a bunch of interrupt, because this is not what userspace expects. > Ok, so IIUC, when chaining irqchips (like irq-tango.c) it's hard to migrate the interrupts. > What you could do would be to dedicate an irq line per CPU, and > reconfigure the mux when changing the affinity. Ok, so basically, we'd use only N lines of the GIC (N=number of CPUs, instead of the 24 available) and multiplex the M>N interrupt lines into the N used inputs of the GIC, right? I think this is similar to what Mason had proposed as "first order approximation". > >> About a) I did not find any driver that uses irq_domain_add_linear() >> and irq_domain_add_hierarchy() but maybe I'm not approaching the >> problem from the right angle. > > A chained interrupt controller cannot be hierarchical. That's pretty > fundamental. Oh, sorry I must have misunderstood the terminology. I mean, at the end of IRQ-domain.txt it says: "3) Optionally implement an irq_chip to manage the interrupt controller hardware." which I understood as calls to irq_alloc_domain_generic_chips() with a domain allocated with irq_domain_add_linear() > > Overall, I think you need to settle for a use case (either pure router > or chained controller) and implement that. To support both, you'll need > two different implementations (basically two drivers in one). > Ok thanks for the advise. The pure router, while already implemented, seems to have the issue of not being able to handle more than 24 interrupts, and I'm not sure how to handle the sharing. Also I don't see how would the sharing be described in the DT. With the old driver (irq-tango.c) I can see how the sharing is accomplished, because there were several calls to irq_domain_add_linear/irq_alloc_domain_generic_chips/irq_set_chained_handler/irq_set_handler_data (one for each IRQ line going to the GIC) and then the DT for the device would specify an interrupt-parent + interrupts keys, but I don't see how to do that with the domain_hierarchy or how to describe that on DT. Best regards, Sebastian ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-13 15:46 ` Sebastian Frias 2016-06-13 16:24 ` Marc Zyngier @ 2016-06-13 17:59 ` Lennart Sorensen 1 sibling, 0 replies; 27+ messages in thread From: Lennart Sorensen @ 2016-06-13 17:59 UTC (permalink / raw) To: Sebastian Frias Cc: Marc Zyngier, Thomas Gleixner, LKML, Grygorii Strashko, Sricharan R, Mason, Måns Rullgård On Mon, Jun 13, 2016 at 05:46:19PM +0200, Sebastian Frias wrote: > My understanding of "hierarchical irq domains" is that it is useful when there are multiple stacked interrupt controllers. > Also, the documentation says "the majority of drivers should use the linear map" (as opposed to the hierarchical one). > > Maybe the definition of "interrupt controller" could benefit from some clarification, but my understanding is that, in our case, the GIC is the only interrupt controller (that's where the interrupt type must be set active_high/active_low/edge, etc.), in front of it, it happens to be a crossbar, that happens to be programmable, and that can be used to map any 128 line into any of 24 lines of the GIC (actually it is a many-to-many router, without any latching nor edge detection functionality) > > Obviously, when the DT says that ethernet driver uses IRQ=120 (for example), the crossbar must be setup to route line 120 to one of the 24 lines going to the GIC. > So a minimum of interaction between the GIC driver (and/or the Linux IRQ framework) and the driver programming the crossbar is required, and that's what we are trying to achieve. > > Does that makes sense? Well something has to tell the crossbar to map 120 to one of the 24. Of course the driver for the device needs to be told to use the one of the 24 it was mapped to. As far as I recall the TI irq-crossbar allows you to specify in the DT that you want to use a given input to the crossbar for a given device, and it dynamically allocated a mapping to a free IRQ on the CPU and maps it and the driver is told the correct interrupt to use for that device. Pretty handy. With older versions of the driver you had to setup all the maps manually in the devicetree and manually assign the correct IRQ to the driver, which was more error prone of course and a bit of a hassle. -- Len Sorensen ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Using irq-crossbar.c 2016-06-10 15:37 Using irq-crossbar.c Sebastian Frias 2016-06-10 16:05 ` Marc Zyngier @ 2016-06-10 16:06 ` Lennart Sorensen 1 sibling, 0 replies; 27+ messages in thread From: Lennart Sorensen @ 2016-06-10 16:06 UTC (permalink / raw) To: Sebastian Frias Cc: Thomas Gleixner, Marc Zyngier, LKML, Grygorii Strashko, Sricharan R, Mason, Måns Rullgård On Fri, Jun 10, 2016 at 05:37:30PM +0200, Sebastian Frias wrote: > We are trying to write a driver for an interrupt controller (actually more of a crossbar) for an ARM-based SoC. > This IRQ crossbar has 128 inputs and 24 outputs, the outputs are connected directly to the GIC. > The idea is that the GIC handles everything, and just request a mapping from an IRQ number (0...127, from a device's DT entry) into one of its 24 input lines. > > By looking at current code (4.7-rc1) there seems to be a driver (drivers/irqchip/irq-crossbar.c) that provides similar functionality. > The driver uses hierarchical irq domains (since commit 783d31863fb8 "irqchip: crossbar: Convert dra7 crossbar to stacked domains") which we believe we don't need because the only controller is the GIC. > However the API used previously, register_routable_domain_ops(), was removed with commit a5561c3e845c "irqchip: gic: Get rid of routable domain". > > Trying to use the driver with hierarchical domains (after modifications for our SoC), results on the kernel being blocked at some point: > > [ 0.041524] ThumbEE CPU extension supported. > [ 0.041589] Registering SWP/SWPB emulation handler > [ 0.052022] Freeing unused kernel memory: 12364K (c029b000 - c0eae000) > [ 0.074084] random: dbus-uuidgen urandom read with 0 bits of entropy available > > We've put logs on the different domain_ops calls (alloc, free, translate) but they are not called, even if the DT is supposed to tell devices to take interrupts from this controller (*). > > Do you have suggestions on what APIs should be used, further reading/examples and/or pointers on how debug this (logs to enable, things to look for, etc.)? Well irq-crossbar.c seems to be very specific to the crossbar in TI chips which handle lots of inputs and lots of outputs to multiple receivers (not just the GIC) as far as I have understood it. Also, unless you modified the IRQCHIP_DECLARE at the end, I see nothing in your dtb that would match the driver at all. And even if that did match, is your crossbar at all register compatible with the TI design? irq-crossbar might be a good example for how to write such a driver, but I wouldn't have much hope of it being useful as a generic driver, never mind the unfortunate name the source file has. I suspect ti-irq-crossbar.c would have been much more appropriate. -- Len Soremsem ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2016-06-21 15:55 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-10 15:37 Using irq-crossbar.c Sebastian Frias 2016-06-10 16:05 ` Marc Zyngier 2016-06-10 19:36 ` Mason 2016-06-11 9:58 ` Marc Zyngier 2016-06-11 15:37 ` Mason 2016-06-12 10:00 ` Marc Zyngier 2016-06-12 13:50 ` Mason 2016-06-13 7:58 ` Marc Zyngier 2016-06-13 14:04 ` Lennart Sorensen 2016-06-13 14:57 ` Sebastian Frias 2016-06-13 15:42 ` Lennart Sorensen 2016-06-13 15:49 ` Mason 2016-06-13 15:57 ` Marc Zyngier 2016-06-13 17:55 ` Lennart Sorensen 2016-06-13 15:15 ` Sebastian Frias 2016-06-13 16:26 ` Marc Zyngier 2016-06-13 15:46 ` Sebastian Frias 2016-06-13 16:24 ` Marc Zyngier 2016-06-14 16:37 ` Sebastian Frias 2016-06-14 16:39 ` Sebastian Frias 2016-06-16 12:39 ` Sebastian Frias 2016-06-21 10:18 ` Marc Zyngier 2016-06-21 11:03 ` Sebastian Frias 2016-06-21 12:41 ` Marc Zyngier 2016-06-21 15:29 ` Sebastian Frias 2016-06-13 17:59 ` Lennart Sorensen 2016-06-10 16:06 ` Lennart Sorensen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).