Using irq-crossbar.c

Using irq-crossbar.c

[Sebastian Frias]

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
+
 	};
 };

Re: Using irq-crossbar.c

[Marc Zyngier]

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

Re: Using irq-crossbar.c

[Mason]

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.

Re: Using irq-crossbar.c

[Marc Zyngier]

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.

Re: Using irq-crossbar.c

[Mason]

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

Re: Using irq-crossbar.c

[Marc Zyngier]

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.

Re: Using irq-crossbar.c

[Mason]

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.

Re: Using irq-crossbar.c

[Marc Zyngier]

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

Re: Using irq-crossbar.c

[Lennart Sorensen]

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

Re: Using irq-crossbar.c

[Sebastian Frias]

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

Re: Using irq-crossbar.c

[Lennart Sorensen]

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

Re: Using irq-crossbar.c

[Mason]

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.

Re: Using irq-crossbar.c

[Marc Zyngier]

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

Re: Using irq-crossbar.c

[Lennart Sorensen]

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

Re: Using irq-crossbar.c

[Sebastian Frias]

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

Re: Using irq-crossbar.c

[Marc Zyngier]

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

Re: Using irq-crossbar.c

[Sebastian Frias]

[-- 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);

Re: Using irq-crossbar.c

[Marc Zyngier]

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

Re: Using irq-crossbar.c

[Sebastian Frias]

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

Re: Using irq-crossbar.c

[Sebastian Frias]

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

Re: Using irq-crossbar.c

[Sebastian Frias]

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

Re: Using irq-crossbar.c

[Marc Zyngier]

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

Re: Using irq-crossbar.c

[Sebastian Frias]

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

Re: Using irq-crossbar.c

[Marc Zyngier]

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

Re: Using irq-crossbar.c

[Sebastian Frias]

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

Re: Using irq-crossbar.c

[Lennart Sorensen]

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

Re: Using irq-crossbar.c

[Lennart Sorensen]

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