[PATCH 0/2] irqchip: ti-sci-intr: Add support for K3 Interrupt Router

[PATCH 0/2] irqchip: ti-sci-intr: Add support for K3 Interrupt Router

[Lokesh Vutla]

This series adds irqchip driver for Texas Instruments' K3 based
Interrupt Router.

This series depends on TISCI IRQ management support posted here[1]

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-October/605784.html

Lokesh Vutla (2):
  dt-bindings: irqchip: Introduce TISCI Interrupt router bindings
  irqchip: ti-sci-intr: Add support for Interrupt Router driver

 .../interrupt-controller/ti,sci-intr.txt      |  83 +++++
 MAINTAINERS                                   |   2 +
 drivers/irqchip/Kconfig                       |  11 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-ti-sci-intr.c             | 325 ++++++++++++++++++
 5 files changed, 422 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
 create mode 100644 drivers/irqchip/irq-ti-sci-intr.c

-- 
2.19.0

[PATCH 1/2] dt-bindings: irqchip: Introduce TISCI Interrupt router bindings

[Lokesh Vutla]

Add the DT binding documentation for Interrupt router driver.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 .../interrupt-controller/ti,sci-intr.txt      | 83 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 84 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt b/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
new file mode 100644
index 000000000000..681ca53cc5fb
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
@@ -0,0 +1,83 @@
+Texas Instruments K3 Interrupt Router
+=====================================
+
+The Interrupt Router (INTR) module provides a mechanism to mux M
+interrupt inputs to N interrupt outputs, where all M inputs are selectable
+to be driven per N output. There is one register per output (MUXCNTL_N) that
+controls the selection.
+
+
+                                 Interrupt Router
+                             +----------------------+
+                             |  Inputs     Outputs  |
+        +-------+            | +------+             |
+        | GPIO  |----------->| | irq0 |             |       Host IRQ
+        +-------+            | +------+             |      controller
+                             |    .        +-----+  |      +-------+
+        +-------+            |    .        |  0  |  |----->|  GIC  |
+        | INTA  |----------->|    .        +-----+  |      +-------+
+        +-------+            |    .          .      |
+                             | +------+      .      |
+                             | | irqM |    +-----+  |
+                             | +------+    |  N  |  |
+                             |             +-----+  |
+                             +----------------------+
+
+Configuration of these MUXCNTL_N registers is done by a system controller
+(like the Device Memory and Security Controller on K3 AM654 SoC). System
+controller will keep track of the used and unused registers within the Router.
+Driver should request the system controller to get the range of GIC IRQs
+assigned to the requesting hosts. It is the drivers responsibility to keep
+track of GIC IRQs.
+
+Communication between the host processor running an OS and the system
+controller happens through a protocol called TI System Control Interface
+(TISCI protocol). For more details refer:
+Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
+
+TISCI Interrupt Router Node:
+----------------------------
+- compatible:		Must be "ti,sci-intr".
+- interrupt-controller:	Identifies the node as an interrupt controller
+- #interrupt-cells:	Specifies the number of cells needed to encode an
+			interrupt source. The value should be 3.
+			First cell should contain the TISCI device ID of source
+			Second cell should contain the interrupt source offset
+			within the device
+			Third cell specifies the trigger type as defined
+			in interrupts.txt in this directory.
+- interrupt-parent:	phandle of irq parent for TISCI intr. The parent must
+			use the same interrupt-cells format as GIC.
+- ti,sci:		Phandle to TI-SCI compatible System controller node.
+- ti,sci-dst-id:	TISCI device ID of the destination IRQ controller.
+- ti,sci-rm-range-girq:	Tuple corresponding to unique TISCI resource type that
+			defines the dst host irq ranges assigned to this
+			interrupt router from this host context.
+			Tuple should be of format <type subtype>.
+
+Example:
+--------
+The following example demonstrates both interrupt router node and the consumer
+node(main gpio) on the AM654 SoC:
+
+main_intr: interrupt-controller@1 {
+	compatible = "ti,sci-intr";
+	interrupt-controller;
+	interrupt-parent = <&gic>;
+	#interrupt-cells = <3>;
+	ti,sci = <&dmsc>;
+	ti,sci-dst-id = <56>;
+	ti,sci-rm-range-girq = <0xb 0x1>;
+};
+
+main_gpio0:  main_gpio0@600000 {
+	...
+	interrupt-parent = <&main_intr>;
+	interrupts = <57 256 IRQ_TYPE_EDGE_RISING>,
+			<57 257 IRQ_TYPE_EDGE_RISING>,
+			<57 258 IRQ_TYPE_EDGE_RISING>,
+			<57 259 IRQ_TYPE_EDGE_RISING>,
+			<57 260 IRQ_TYPE_EDGE_RISING>,
+			<57 261 IRQ_TYPE_EDGE_RISING>;
+	...
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index 29c08106bd22..a23778b68d74 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14625,6 +14625,7 @@ F:	Documentation/devicetree/bindings/reset/ti,sci-reset.txt
 F:	Documentation/devicetree/bindings/clock/ti,sci-clk.txt
 F:	drivers/clk/keystone/sci-clk.c
 F:	drivers/reset/reset-ti-sci.c
+F:	Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
 
 THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER
 M:	Hans Verkuil <hverkuil@xs4all.nl>
-- 
2.19.0

Re: [PATCH 1/2] dt-bindings: irqchip: Introduce TISCI Interrupt router bindings

[Marc Zyngier]

On Sat, 06 Oct 2018 08:28:11 +0100,
Lokesh Vutla <lokeshvutla@ti.com> wrote:
> 
> Add the DT binding documentation for Interrupt router driver.
> 
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
>  .../interrupt-controller/ti,sci-intr.txt      | 83 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 84 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt b/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
> new file mode 100644
> index 000000000000..681ca53cc5fb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
> @@ -0,0 +1,83 @@
> +Texas Instruments K3 Interrupt Router
> +=====================================
> +
> +The Interrupt Router (INTR) module provides a mechanism to mux M
> +interrupt inputs to N interrupt outputs, where all M inputs are selectable
> +to be driven per N output. There is one register per output (MUXCNTL_N) that
> +controls the selection.
> +
> +
> +                                 Interrupt Router
> +                             +----------------------+
> +                             |  Inputs     Outputs  |
> +        +-------+            | +------+             |
> +        | GPIO  |----------->| | irq0 |             |       Host IRQ
> +        +-------+            | +------+             |      controller
> +                             |    .        +-----+  |      +-------+
> +        +-------+            |    .        |  0  |  |----->|  GIC  |
> +        | INTA  |----------->|    .        +-----+  |      +-------+
> +        +-------+            |    .          .      |
> +                             | +------+      .      |
> +                             | | irqM |    +-----+  |
> +                             | +------+    |  N  |  |
> +                             |             +-----+  |
> +                             +----------------------+
> +
> +Configuration of these MUXCNTL_N registers is done by a system controller
> +(like the Device Memory and Security Controller on K3 AM654 SoC). System
> +controller will keep track of the used and unused registers within the Router.
> +Driver should request the system controller to get the range of GIC IRQs
> +assigned to the requesting hosts. It is the drivers responsibility to keep
> +track of GIC IRQs.

I would drop the GIC here, and replace it by "parent interrupt
controller", as nothing here is GIC specific.

> +
> +Communication between the host processor running an OS and the system
> +controller happens through a protocol called TI System Control Interface
> +(TISCI protocol). For more details refer:
> +Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
> +
> +TISCI Interrupt Router Node:
> +----------------------------
> +- compatible:		Must be "ti,sci-intr".
> +- interrupt-controller:	Identifies the node as an interrupt controller
> +- #interrupt-cells:	Specifies the number of cells needed to encode an
> +			interrupt source. The value should be 3.
> +			First cell should contain the TISCI device ID of source
> +			Second cell should contain the interrupt source offset
> +			within the device
> +			Third cell specifies the trigger type as defined
> +			in interrupts.txt in this directory.

Are all trigger types supported?

> +- interrupt-parent:	phandle of irq parent for TISCI intr. The parent must
> +			use the same interrupt-cells format as GIC.

Why that constraint? From what I can see, the two are fairly
independent, and the constraint looks more of a Linux driver issue
than a DT constraint.

> +- ti,sci:		Phandle to TI-SCI compatible System controller node.
> +- ti,sci-dst-id:	TISCI device ID of the destination IRQ controller.
> +- ti,sci-rm-range-girq:	Tuple corresponding to unique TISCI resource type that
> +			defines the dst host irq ranges assigned to this
> +			interrupt router from this host context.
> +			Tuple should be of format <type subtype>.
> +
> +Example:
> +--------
> +The following example demonstrates both interrupt router node and the consumer
> +node(main gpio) on the AM654 SoC:
> +
> +main_intr: interrupt-controller@1 {
> +	compatible = "ti,sci-intr";
> +	interrupt-controller;
> +	interrupt-parent = <&gic>;
> +	#interrupt-cells = <3>;
> +	ti,sci = <&dmsc>;
> +	ti,sci-dst-id = <56>;
> +	ti,sci-rm-range-girq = <0xb 0x1>;
> +};
> +
> +main_gpio0:  main_gpio0@600000 {
> +	...
> +	interrupt-parent = <&main_intr>;
> +	interrupts = <57 256 IRQ_TYPE_EDGE_RISING>,
> +			<57 257 IRQ_TYPE_EDGE_RISING>,
> +			<57 258 IRQ_TYPE_EDGE_RISING>,
> +			<57 259 IRQ_TYPE_EDGE_RISING>,
> +			<57 260 IRQ_TYPE_EDGE_RISING>,
> +			<57 261 IRQ_TYPE_EDGE_RISING>;
> +	...
> +};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 29c08106bd22..a23778b68d74 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14625,6 +14625,7 @@ F:	Documentation/devicetree/bindings/reset/ti,sci-reset.txt
>  F:	Documentation/devicetree/bindings/clock/ti,sci-clk.txt
>  F:	drivers/clk/keystone/sci-clk.c
>  F:	drivers/reset/reset-ti-sci.c
> +F:	Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
>  
>  THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER
>  M:	Hans Verkuil <hverkuil@xs4all.nl>
> -- 
> 2.19.0
> 

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

Re: [PATCH 1/2] dt-bindings: irqchip: Introduce TISCI Interrupt router bindings

[Lokesh Vutla]

Hi Marc,

On Saturday 06 October 2018 03:32 PM, Marc Zyngier wrote:
> On Sat, 06 Oct 2018 08:28:11 +0100,
> Lokesh Vutla <lokeshvutla@ti.com> wrote:
>>
>> Add the DT binding documentation for Interrupt router driver.
>>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> ---
>>   .../interrupt-controller/ti,sci-intr.txt      | 83 +++++++++++++++++++
>>   MAINTAINERS                                   |  1 +
>>   2 files changed, 84 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt b/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
>> new file mode 100644
>> index 000000000000..681ca53cc5fb
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
>> @@ -0,0 +1,83 @@
>> +Texas Instruments K3 Interrupt Router
>> +=====================================
>> +
>> +The Interrupt Router (INTR) module provides a mechanism to mux M
>> +interrupt inputs to N interrupt outputs, where all M inputs are selectable
>> +to be driven per N output. There is one register per output (MUXCNTL_N) that
>> +controls the selection.
>> +
>> +
>> +                                 Interrupt Router
>> +                             +----------------------+
>> +                             |  Inputs     Outputs  |
>> +        +-------+            | +------+             |
>> +        | GPIO  |----------->| | irq0 |             |       Host IRQ
>> +        +-------+            | +------+             |      controller
>> +                             |    .        +-----+  |      +-------+
>> +        +-------+            |    .        |  0  |  |----->|  GIC  |
>> +        | INTA  |----------->|    .        +-----+  |      +-------+
>> +        +-------+            |    .          .      |
>> +                             | +------+      .      |
>> +                             | | irqM |    +-----+  |
>> +                             | +------+    |  N  |  |
>> +                             |             +-----+  |
>> +                             +----------------------+
>> +
>> +Configuration of these MUXCNTL_N registers is done by a system controller
>> +(like the Device Memory and Security Controller on K3 AM654 SoC). System
>> +controller will keep track of the used and unused registers within the Router.
>> +Driver should request the system controller to get the range of GIC IRQs
>> +assigned to the requesting hosts. It is the drivers responsibility to keep
>> +track of GIC IRQs.
> 
> I would drop the GIC here, and replace it by "parent interrupt
> controller", as nothing here is GIC specific
> 
>> +
>> +Communication between the host processor running an OS and the system
>> +controller happens through a protocol called TI System Control Interface
>> +(TISCI protocol). For more details refer:
>> +Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
>> +
>> +TISCI Interrupt Router Node:
>> +----------------------------
>> +- compatible:		Must be "ti,sci-intr".
>> +- interrupt-controller:	Identifies the node as an interrupt controller
>> +- #interrupt-cells:	Specifies the number of cells needed to encode an
>> +			interrupt source. The value should be 3.
>> +			First cell should contain the TISCI device ID of source
>> +			Second cell should contain the interrupt source offset
>> +			within the device
>> +			Third cell specifies the trigger type as defined
>> +			in interrupts.txt in this directory.
> 
> Are all trigger types supported?

Nope, only level interrupts are supported. Will fix it in v2.

> 
>> +- interrupt-parent:	phandle of irq parent for TISCI intr. The parent must
>> +			use the same interrupt-cells format as GIC.
> 
> Why that constraint? From what I can see, the two are fairly
> independent, and the constraint looks more of a Linux driver issue
> than a DT constraint.

Driver when calling irq_domain_alloc_irqs_parent(), the fwspec node that gets 
passed assumes that parent is gic. parameters are filled in with such 
assumption. Do you suggest anything to make it more generic?

> 
>> +- ti,sci:		Phandle to TI-SCI compatible System controller node.
>> +- ti,sci-dst-id:	TISCI device ID of the destination IRQ controller.
>> +- ti,sci-rm-range-girq:	Tuple corresponding to unique TISCI resource type that
>> +			defines the dst host irq ranges assigned to this
>> +			interrupt router from this host context.
>> +			Tuple should be of format <type subtype>.
>> +

Thanks a lot for the review. Also, I need a suggestion regarding one more 
interrupt controller(Interrupt Aggregator) on the same SoC controlled by 
TISCI_PROTOCOL.

The Interrupt Aggregator (INTA) provides a centralized machine
which handles the termination of system events to that they can
be coherently processed by the host(s) in the system. Integration looks 
something similar https://pastebin.ubuntu.com/p/T32vbrwsch/ .

Configuration of the Intmap registers that maps global events to vint is done
by a system controller (like the Device Memory and Security Controller on K3
AM654 SoC). Driver should request the system controller to get the range
of global events and vints assigned to the requesting host. Management
of these requested resources should be handled by driver and requests
system controller to map specific global event to vint, bit pair.

There can be cases such that IRQ routes can involve both INTR and INTA like below:
	IP ---> INTA ---> INTR ----> GIC.

In these cases TISCI involves only one message with parametes(source id, source 
offset, inta_id, dst id) for configuring IRQ route till the destination. Co 
processor will detect there is INTR in the IRQ path and configure that as well.

Right now I kind of differentiated this scenario in INTA driver by passing a 
flag(TI_SCI_EVENT) to INTR driver. If such flag comes, INTR driver should avoid 
calling ti_sci api for configuring. Do you think this is the right direction or 
do you suggest a better solution.

If I am not clear in the above description, I can post an RFC for INTA driver 
for continuing this discussion.

Thanks and regards,
Lokesh

Re: [PATCH 1/2] dt-bindings: irqchip: Introduce TISCI Interrupt router bindings

[Marc Zyngier]

On 08/10/18 10:46, Lokesh Vutla wrote:
> Hi Marc,
> 
> On Saturday 06 October 2018 03:32 PM, Marc Zyngier wrote:
>> On Sat, 06 Oct 2018 08:28:11 +0100,
>> Lokesh Vutla <lokeshvutla@ti.com> wrote:
>>>
>>> Add the DT binding documentation for Interrupt router driver.
>>>
>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>> ---
>>>    .../interrupt-controller/ti,sci-intr.txt      | 83 +++++++++++++++++++
>>>    MAINTAINERS                                   |  1 +
>>>    2 files changed, 84 insertions(+)
>>>    create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt b/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
>>> new file mode 100644
>>> index 000000000000..681ca53cc5fb
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
>>> @@ -0,0 +1,83 @@
>>> +Texas Instruments K3 Interrupt Router
>>> +=====================================
>>> +
>>> +The Interrupt Router (INTR) module provides a mechanism to mux M
>>> +interrupt inputs to N interrupt outputs, where all M inputs are selectable
>>> +to be driven per N output. There is one register per output (MUXCNTL_N) that
>>> +controls the selection.
>>> +
>>> +
>>> +                                 Interrupt Router
>>> +                             +----------------------+
>>> +                             |  Inputs     Outputs  |
>>> +        +-------+            | +------+             |
>>> +        | GPIO  |----------->| | irq0 |             |       Host IRQ
>>> +        +-------+            | +------+             |      controller
>>> +                             |    .        +-----+  |      +-------+
>>> +        +-------+            |    .        |  0  |  |----->|  GIC  |
>>> +        | INTA  |----------->|    .        +-----+  |      +-------+
>>> +        +-------+            |    .          .      |
>>> +                             | +------+      .      |
>>> +                             | | irqM |    +-----+  |
>>> +                             | +------+    |  N  |  |
>>> +                             |             +-----+  |
>>> +                             +----------------------+
>>> +
>>> +Configuration of these MUXCNTL_N registers is done by a system controller
>>> +(like the Device Memory and Security Controller on K3 AM654 SoC). System
>>> +controller will keep track of the used and unused registers within the Router.
>>> +Driver should request the system controller to get the range of GIC IRQs
>>> +assigned to the requesting hosts. It is the drivers responsibility to keep
>>> +track of GIC IRQs.
>>
>> I would drop the GIC here, and replace it by "parent interrupt
>> controller", as nothing here is GIC specific
>>
>>> +
>>> +Communication between the host processor running an OS and the system
>>> +controller happens through a protocol called TI System Control Interface
>>> +(TISCI protocol). For more details refer:
>>> +Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
>>> +
>>> +TISCI Interrupt Router Node:
>>> +----------------------------
>>> +- compatible:		Must be "ti,sci-intr".
>>> +- interrupt-controller:	Identifies the node as an interrupt controller
>>> +- #interrupt-cells:	Specifies the number of cells needed to encode an
>>> +			interrupt source. The value should be 3.
>>> +			First cell should contain the TISCI device ID of source
>>> +			Second cell should contain the interrupt source offset
>>> +			within the device
>>> +			Third cell specifies the trigger type as defined
>>> +			in interrupts.txt in this directory.
>>
>> Are all trigger types supported?
> 
> Nope, only level interrupts are supported. Will fix it in v2.
> 
>>
>>> +- interrupt-parent:	phandle of irq parent for TISCI intr. The parent must
>>> +			use the same interrupt-cells format as GIC.
>>
>> Why that constraint? From what I can see, the two are fairly
>> independent, and the constraint looks more of a Linux driver issue
>> than a DT constraint.
> 
> Driver when calling irq_domain_alloc_irqs_parent(), the fwspec node that gets
> passed assumes that parent is gic. parameters are filled in with such
> assumption. Do you suggest anything to make it more generic?

As I said, that's a Linux driver issue, not a DT specification at all. 
It is not worth it trying to generalize it in the driver implementation, 
but the DT spec it self should be generic enough.

> 
>>
>>> +- ti,sci:		Phandle to TI-SCI compatible System controller node.
>>> +- ti,sci-dst-id:	TISCI device ID of the destination IRQ controller.
>>> +- ti,sci-rm-range-girq:	Tuple corresponding to unique TISCI resource type that
>>> +			defines the dst host irq ranges assigned to this
>>> +			interrupt router from this host context.
>>> +			Tuple should be of format <type subtype>.
>>> +
> 
> Thanks a lot for the review. Also, I need a suggestion regarding one more
> interrupt controller(Interrupt Aggregator) on the same SoC controlled by
> TISCI_PROTOCOL.
> 
> The Interrupt Aggregator (INTA) provides a centralized machine
> which handles the termination of system events to that they can
> be coherently processed by the host(s) in the system. Integration looks
> something similar https://pastebin.ubuntu.com/p/T32vbrwsch/ .
> 
> Configuration of the Intmap registers that maps global events to vint is done
> by a system controller (like the Device Memory and Security Controller on K3
> AM654 SoC). Driver should request the system controller to get the range
> of global events and vints assigned to the requesting host. Management
> of these requested resources should be handled by driver and requests
> system controller to map specific global event to vint, bit pair.

I'm sorry, but I really have no idea what the global events and the 
vints are. Maybe you should describe what this is all about, and maybe 
provide a pointer to some documentation...

> 
> There can be cases such that IRQ routes can involve both INTR and INTA like below:
> 	IP ---> INTA ---> INTR ----> GIC.
> 
> In these cases TISCI involves only one message with parametes(source id, source
> offset, inta_id, dst id) for configuring IRQ route till the destination. Co
> processor will detect there is INTR in the IRQ path and configure that as well.
> 
> Right now I kind of differentiated this scenario in INTA driver by passing a
> flag(TI_SCI_EVENT) to INTR driver. If such flag comes, INTR driver should avoid
> calling ti_sci api for configuring. Do you think this is the right direction or
> do you suggest a better solution.

Frankly, it mostly indicates that the firmware does too much, and should 
be more flexible.

> If I am not clear in the above description, I can post an RFC for INTA driver
> for continuing this discussion.

That'd be preferable, IMO. Please provide definitions for all the above 
jargon, as well as pointers to publicly available documentation, if any.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/2] dt-bindings: irqchip: Introduce TISCI Interrupt router bindings

[Lokesh Vutla]

On Monday 08 October 2018 07:25 PM, Marc Zyngier wrote:
> On 08/10/18 10:46, Lokesh Vutla wrote:
>> Hi Marc,
>>
>> On Saturday 06 October 2018 03:32 PM, Marc Zyngier wrote:
>>> On Sat, 06 Oct 2018 08:28:11 +0100,
>>> Lokesh Vutla <lokeshvutla@ti.com> wrote:
>>>>
>>>> Add the DT binding documentation for Interrupt router driver.
>>>>
>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>>> ---
>>>>     .../interrupt-controller/ti,sci-intr.txt      | 83 +++++++++++++++++++
>>>>     MAINTAINERS                                   |  1 +
>>>>     2 files changed, 84 insertions(+)
>>>>     create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt b/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
>>>> new file mode 100644
>>>> index 000000000000..681ca53cc5fb
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
>>>> @@ -0,0 +1,83 @@
>>>> +Texas Instruments K3 Interrupt Router
>>>> +=====================================
>>>> +
>>>> +The Interrupt Router (INTR) module provides a mechanism to mux M
>>>> +interrupt inputs to N interrupt outputs, where all M inputs are selectable
>>>> +to be driven per N output. There is one register per output (MUXCNTL_N) that
>>>> +controls the selection.
>>>> +
>>>> +
>>>> +                                 Interrupt Router
>>>> +                             +----------------------+
>>>> +                             |  Inputs     Outputs  |
>>>> +        +-------+            | +------+             |
>>>> +        | GPIO  |----------->| | irq0 |             |       Host IRQ
>>>> +        +-------+            | +------+             |      controller
>>>> +                             |    .        +-----+  |      +-------+
>>>> +        +-------+            |    .        |  0  |  |----->|  GIC  |
>>>> +        | INTA  |----------->|    .        +-----+  |      +-------+
>>>> +        +-------+            |    .          .      |
>>>> +                             | +------+      .      |
>>>> +                             | | irqM |    +-----+  |
>>>> +                             | +------+    |  N  |  |
>>>> +                             |             +-----+  |
>>>> +                             +----------------------+
>>>> +
>>>> +Configuration of these MUXCNTL_N registers is done by a system controller
>>>> +(like the Device Memory and Security Controller on K3 AM654 SoC). System
>>>> +controller will keep track of the used and unused registers within the Router.
>>>> +Driver should request the system controller to get the range of GIC IRQs
>>>> +assigned to the requesting hosts. It is the drivers responsibility to keep
>>>> +track of GIC IRQs.
>>>
>>> I would drop the GIC here, and replace it by "parent interrupt
>>> controller", as nothing here is GIC specific
>>>
>>>> +
>>>> +Communication between the host processor running an OS and the system
>>>> +controller happens through a protocol called TI System Control Interface
>>>> +(TISCI protocol). For more details refer:
>>>> +Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
>>>> +
>>>> +TISCI Interrupt Router Node:
>>>> +----------------------------
>>>> +- compatible:		Must be "ti,sci-intr".
>>>> +- interrupt-controller:	Identifies the node as an interrupt controller
>>>> +- #interrupt-cells:	Specifies the number of cells needed to encode an
>>>> +			interrupt source. The value should be 3.
>>>> +			First cell should contain the TISCI device ID of source
>>>> +			Second cell should contain the interrupt source offset
>>>> +			within the device
>>>> +			Third cell specifies the trigger type as defined
>>>> +			in interrupts.txt in this directory.
>>>
>>> Are all trigger types supported?
>>
>> Nope, only level interrupts are supported. Will fix it in v2.
>>
>>>
>>>> +- interrupt-parent:	phandle of irq parent for TISCI intr. The parent must
>>>> +			use the same interrupt-cells format as GIC.
>>>
>>> Why that constraint? From what I can see, the two are fairly
>>> independent, and the constraint looks more of a Linux driver issue
>>> than a DT constraint.
>>
>> Driver when calling irq_domain_alloc_irqs_parent(), the fwspec node that gets
>> passed assumes that parent is gic. parameters are filled in with such
>> assumption. Do you suggest anything to make it more generic?
> 
> As I said, that's a Linux driver issue, not a DT specification at all.
> It is not worth it trying to generalize it in the driver implementation,
> but the DT spec it self should be generic enough.

okay, will fix it in next version.

> 
>>
>>>
>>>> +- ti,sci:		Phandle to TI-SCI compatible System controller node.
>>>> +- ti,sci-dst-id:	TISCI device ID of the destination IRQ controller.
>>>> +- ti,sci-rm-range-girq:	Tuple corresponding to unique TISCI resource type that
>>>> +			defines the dst host irq ranges assigned to this
>>>> +			interrupt router from this host context.
>>>> +			Tuple should be of format <type subtype>.
>>>> +
>>
>> Thanks a lot for the review. Also, I need a suggestion regarding one more
>> interrupt controller(Interrupt Aggregator) on the same SoC controlled by
>> TISCI_PROTOCOL.
>>
>> The Interrupt Aggregator (INTA) provides a centralized machine
>> which handles the termination of system events to that they can
>> be coherently processed by the host(s) in the system. Integration looks
>> something similar https://pastebin.ubuntu.com/p/T32vbrwsch/ .
>>
>> Configuration of the Intmap registers that maps global events to vint is done
>> by a system controller (like the Device Memory and Security Controller on K3
>> AM654 SoC). Driver should request the system controller to get the range
>> of global events and vints assigned to the requesting host. Management
>> of these requested resources should be handled by driver and requests
>> system controller to map specific global event to vint, bit pair.
> 
> I'm sorry, but I really have no idea what the global events and the
> vints are. Maybe you should describe what this is all about, and maybe
> provide a pointer to some documentation...

Sorry I should have done that earlier. TRM is available here[1], Section 9.3 
talks about Interrupt Router, Section 10.2.7 talks about Interrupt aggregator. 
Documentation for TISCI IRQ management is available here[2].


[1] http://www.ti.com/lit/ug/spruid7a/spruid7a.pdf
[2] http://downloads.ti.com/tisci/esd/latest/2_tisci_msgs/rm/rm_irq.html

> 
>>
>> There can be cases such that IRQ routes can involve both INTR and INTA like below:
>> 	IP ---> INTA ---> INTR ----> GIC.
>>
>> In these cases TISCI involves only one message with parametes(source id, source
>> offset, inta_id, dst id) for configuring IRQ route till the destination. Co
>> processor will detect there is INTR in the IRQ path and configure that as well.
>>
>> Right now I kind of differentiated this scenario in INTA driver by passing a
>> flag(TI_SCI_EVENT) to INTR driver. If such flag comes, INTR driver should avoid
>> calling ti_sci api for configuring. Do you think this is the right direction or
>> do you suggest a better solution.
> 
> Frankly, it mostly indicates that the firmware does too much, and should
> be more flexible.
> 
>> If I am not clear in the above description, I can post an RFC for INTA driver
>> for continuing this discussion.
> 
> That'd be preferable, IMO. Please provide definitions for all the above

Sure, will try to post the consolidated series asap. Thanks a lot for the help.

Regards,
Lokesh

Re: [PATCH 1/2] dt-bindings: irqchip: Introduce TISCI Interrupt router bindings

[Nishanth Menon]

On 12:58-20181006, Lokesh Vutla wrote:
> Add the DT binding documentation for Interrupt router driver.
> 
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
>  .../interrupt-controller/ti,sci-intr.txt      | 83 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 84 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt b/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
> new file mode 100644
> index 000000000000..681ca53cc5fb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
> @@ -0,0 +1,83 @@
> +Texas Instruments K3 Interrupt Router
> +=====================================
> +
> +The Interrupt Router (INTR) module provides a mechanism to mux M
> +interrupt inputs to N interrupt outputs, where all M inputs are selectable
> +to be driven per N output. There is one register per output (MUXCNTL_N) that
> +controls the selection.
> +
> +
> +                                 Interrupt Router
> +                             +----------------------+
> +                             |  Inputs     Outputs  |
> +        +-------+            | +------+             |
> +        | GPIO  |----------->| | irq0 |             |       Host IRQ
> +        +-------+            | +------+             |      controller
> +                             |    .        +-----+  |      +-------+
> +        +-------+            |    .        |  0  |  |----->|  GIC  |
> +        | INTA  |----------->|    .        +-----+  |      +-------+
> +        +-------+            |    .          .      |
> +                             | +------+      .      |
> +                             | | irqM |    +-----+  |
> +                             | +------+    |  N  |  |
> +                             |             +-----+  |
> +                             +----------------------+
> +
> +Configuration of these MUXCNTL_N registers is done by a system controller
> +(like the Device Memory and Security Controller on K3 AM654 SoC). System
> +controller will keep track of the used and unused registers within the Router.
> +Driver should request the system controller to get the range of GIC IRQs
> +assigned to the requesting hosts. It is the drivers responsibility to keep
> +track of GIC IRQs.
> +
> +Communication between the host processor running an OS and the system
> +controller happens through a protocol called TI System Control Interface
> +(TISCI protocol). For more details refer:
> +Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
> +
> +TISCI Interrupt Router Node:
> +----------------------------
> +- compatible:		Must be "ti,sci-intr".
> +- interrupt-controller:	Identifies the node as an interrupt controller
> +- #interrupt-cells:	Specifies the number of cells needed to encode an
> +			interrupt source. The value should be 3.
> +			First cell should contain the TISCI device ID of source
> +			Second cell should contain the interrupt source offset
> +			within the device
> +			Third cell specifies the trigger type as defined
> +			in interrupts.txt in this directory.
> +- interrupt-parent:	phandle of irq parent for TISCI intr. The parent must
> +			use the same interrupt-cells format as GIC.
> +- ti,sci:		Phandle to TI-SCI compatible System controller node.
> +- ti,sci-dst-id:	TISCI device ID of the destination IRQ controller.
> +- ti,sci-rm-range-girq:	Tuple corresponding to unique TISCI resource type that
> +			defines the dst host irq ranges assigned to this
> +			interrupt router from this host context.
> +			Tuple should be of format <type subtype>.
> +

Rob, DT maintainers,

I'd like a feedback from DT maintainers on this 'range' topic.

TISCI Firmware [1] currently seems to define a type corresponding to a
device ID[2]. in AM6 device, for example, this is different, however
have a 1 to 1 correspondence. However, there is expectation that type will
end up as device ID in a future SoC.

While this is subject to much debate internally, I'd like some feedback if this
is OK from Device tree representation - it is true that Firmware does
look at it as type, however in some future SoC, it could be that the
values themselves may correspond one to one with a device id -> The
original wish was that types might be something reusable across SoCs,
but that is turning out to be more of a theoretical wish than any thing
practical.

[1]http://software-dl.ti.com/tisci/esd/latest/5_soc_doc/am6x/resasg_types.html
[2]http://software-dl.ti.com/tisci/esd/latest/5_soc_doc/am6x/devices.html
-- 
Regards,
Nishanth Menon

Re: [PATCH 1/2] dt-bindings: irqchip: Introduce TISCI Interrupt router bindings

[Lokesh Vutla]

Hi Rob, DT maintainers,

On Saturday 06 October 2018 09:59 PM, Nishanth Menon wrote:
> On 12:58-20181006, Lokesh Vutla wrote:
>> Add the DT binding documentation for Interrupt router driver.
>>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> ---
>>   .../interrupt-controller/ti,sci-intr.txt      | 83 +++++++++++++++++++
>>   MAINTAINERS                                   |  1 +
>>   2 files changed, 84 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt b/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
>> new file mode 100644
>> index 000000000000..681ca53cc5fb
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
>> @@ -0,0 +1,83 @@
>> +Texas Instruments K3 Interrupt Router
>> +=====================================
>> +
>> +The Interrupt Router (INTR) module provides a mechanism to mux M
>> +interrupt inputs to N interrupt outputs, where all M inputs are selectable
>> +to be driven per N output. There is one register per output (MUXCNTL_N) that
>> +controls the selection.
>> +
>> +
>> +                                 Interrupt Router
>> +                             +----------------------+
>> +                             |  Inputs     Outputs  |
>> +        +-------+            | +------+             |
>> +        | GPIO  |----------->| | irq0 |             |       Host IRQ
>> +        +-------+            | +------+             |      controller
>> +                             |    .        +-----+  |      +-------+
>> +        +-------+            |    .        |  0  |  |----->|  GIC  |
>> +        | INTA  |----------->|    .        +-----+  |      +-------+
>> +        +-------+            |    .          .      |
>> +                             | +------+      .      |
>> +                             | | irqM |    +-----+  |
>> +                             | +------+    |  N  |  |
>> +                             |             +-----+  |
>> +                             +----------------------+
>> +
>> +Configuration of these MUXCNTL_N registers is done by a system controller
>> +(like the Device Memory and Security Controller on K3 AM654 SoC). System
>> +controller will keep track of the used and unused registers within the Router.
>> +Driver should request the system controller to get the range of GIC IRQs
>> +assigned to the requesting hosts. It is the drivers responsibility to keep
>> +track of GIC IRQs.
>> +
>> +Communication between the host processor running an OS and the system
>> +controller happens through a protocol called TI System Control Interface
>> +(TISCI protocol). For more details refer:
>> +Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
>> +
>> +TISCI Interrupt Router Node:
>> +----------------------------
>> +- compatible:		Must be "ti,sci-intr".
>> +- interrupt-controller:	Identifies the node as an interrupt controller
>> +- #interrupt-cells:	Specifies the number of cells needed to encode an
>> +			interrupt source. The value should be 3.
>> +			First cell should contain the TISCI device ID of source
>> +			Second cell should contain the interrupt source offset
>> +			within the device
>> +			Third cell specifies the trigger type as defined
>> +			in interrupts.txt in this directory.
>> +- interrupt-parent:	phandle of irq parent for TISCI intr. The parent must
>> +			use the same interrupt-cells format as GIC.
>> +- ti,sci:		Phandle to TI-SCI compatible System controller node.
>> +- ti,sci-dst-id:	TISCI device ID of the destination IRQ controller.
>> +- ti,sci-rm-range-girq:	Tuple corresponding to unique TISCI resource type that
>> +			defines the dst host irq ranges assigned to this
>> +			interrupt router from this host context.
>> +			Tuple should be of format <type subtype>.
>> +
> 
> Rob, DT maintainers,
> 
> I'd like a feedback from DT maintainers on this 'range' topic.
> 
> TISCI Firmware [1] currently seems to define a type corresponding to a
> device ID[2]. in AM6 device, for example, this is different, however
> have a 1 to 1 correspondence. However, there is expectation that type will
> end up as device ID in a future SoC.
> 
> While this is subject to much debate internally, I'd like some feedback if this
> is OK from Device tree representation - it is true that Firmware does
> look at it as type, however in some future SoC, it could be that the
> values themselves may correspond one to one with a device id -> The
> original wish was that types might be something reusable across SoCs,
> but that is turning out to be more of a theoretical wish than any thing
> practical.
> 
> [1]http://software-dl.ti.com/tisci/esd/latest/5_soc_doc/am6x/resasg_types.html
> [2]http://software-dl.ti.com/tisci/esd/latest/5_soc_doc/am6x/devices.html

Any help on this topic?

Thanks and regards,
Lokesh

> 

Re: [PATCH 1/2] dt-bindings: irqchip: Introduce TISCI Interrupt router bindings

[Rob Herring]

On Sat, Oct 06, 2018 at 11:29:29AM -0500, Nishanth Menon wrote:
> On 12:58-20181006, Lokesh Vutla wrote:
> > Add the DT binding documentation for Interrupt router driver.
> > 
> > Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> > ---
> >  .../interrupt-controller/ti,sci-intr.txt      | 83 +++++++++++++++++++
> >  MAINTAINERS                                   |  1 +
> >  2 files changed, 84 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt b/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
> > new file mode 100644
> > index 000000000000..681ca53cc5fb
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
> > @@ -0,0 +1,83 @@
> > +Texas Instruments K3 Interrupt Router
> > +=====================================
> > +
> > +The Interrupt Router (INTR) module provides a mechanism to mux M
> > +interrupt inputs to N interrupt outputs, where all M inputs are selectable
> > +to be driven per N output. There is one register per output (MUXCNTL_N) that
> > +controls the selection.
> > +
> > +
> > +                                 Interrupt Router
> > +                             +----------------------+
> > +                             |  Inputs     Outputs  |
> > +        +-------+            | +------+             |
> > +        | GPIO  |----------->| | irq0 |             |       Host IRQ
> > +        +-------+            | +------+             |      controller
> > +                             |    .        +-----+  |      +-------+
> > +        +-------+            |    .        |  0  |  |----->|  GIC  |
> > +        | INTA  |----------->|    .        +-----+  |      +-------+
> > +        +-------+            |    .          .      |
> > +                             | +------+      .      |
> > +                             | | irqM |    +-----+  |
> > +                             | +------+    |  N  |  |
> > +                             |             +-----+  |
> > +                             +----------------------+
> > +
> > +Configuration of these MUXCNTL_N registers is done by a system controller
> > +(like the Device Memory and Security Controller on K3 AM654 SoC). System
> > +controller will keep track of the used and unused registers within the Router.
> > +Driver should request the system controller to get the range of GIC IRQs
> > +assigned to the requesting hosts. It is the drivers responsibility to keep
> > +track of GIC IRQs.
> > +
> > +Communication between the host processor running an OS and the system
> > +controller happens through a protocol called TI System Control Interface
> > +(TISCI protocol). For more details refer:
> > +Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
> > +
> > +TISCI Interrupt Router Node:
> > +----------------------------
> > +- compatible:		Must be "ti,sci-intr".
> > +- interrupt-controller:	Identifies the node as an interrupt controller
> > +- #interrupt-cells:	Specifies the number of cells needed to encode an
> > +			interrupt source. The value should be 3.
> > +			First cell should contain the TISCI device ID of source
> > +			Second cell should contain the interrupt source offset
> > +			within the device
> > +			Third cell specifies the trigger type as defined
> > +			in interrupts.txt in this directory.
> > +- interrupt-parent:	phandle of irq parent for TISCI intr. The parent must
> > +			use the same interrupt-cells format as GIC.
> > +- ti,sci:		Phandle to TI-SCI compatible System controller node.
> > +- ti,sci-dst-id:	TISCI device ID of the destination IRQ controller.
> > +- ti,sci-rm-range-girq:	Tuple corresponding to unique TISCI resource type that
> > +			defines the dst host irq ranges assigned to this
> > +			interrupt router from this host context.
> > +			Tuple should be of format <type subtype>.
> > +
> 
> Rob, DT maintainers,
> 
> I'd like a feedback from DT maintainers on this 'range' topic.
> 
> TISCI Firmware [1] currently seems to define a type corresponding to a
> device ID[2]. in AM6 device, for example, this is different, however
> have a 1 to 1 correspondence. However, there is expectation that type will
> end up as device ID in a future SoC.
> 
> While this is subject to much debate internally, I'd like some feedback if this
> is OK from Device tree representation - it is true that Firmware does
> look at it as type, however in some future SoC, it could be that the
> values themselves may correspond one to one with a device id -> The
> original wish was that types might be something reusable across SoCs,
> but that is turning out to be more of a theoretical wish than any thing
> practical.

I'm not sure I follow all the terminology here of type, subtype, "dst 
host irq", etc. It looks to me like you should be using interrupt-map property.

Rob

> 
> [1]http://software-dl.ti.com/tisci/esd/latest/5_soc_doc/am6x/resasg_types.html
> [2]http://software-dl.ti.com/tisci/esd/latest/5_soc_doc/am6x/devices.html
> -- 
> Regards,
> Nishanth Menon

[PATCH 2/2] irqchip: ti-sci-intr: Add support for Interrupt Router driver

[Lokesh Vutla]

Texas Instruments' K3 generation SoCs has an IP Interrupt Router
that does allows for multiplexing of input interrupts to host
interrupt controller. Interrupt Router inputs are either from a
peripheral or from an Interrupt Aggregator which is another
interrupt controller.

Configuration of the interrupt router registers can only be done by
a system co-processor and the driver needs to send a message to this
co processor over TISCI protocol.

Add support for Interrupt Router driver over TISCI protocol.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 MAINTAINERS                       |   1 +
 drivers/irqchip/Kconfig           |  11 +
 drivers/irqchip/Makefile          |   1 +
 drivers/irqchip/irq-ti-sci-intr.c | 325 ++++++++++++++++++++++++++++++
 4 files changed, 338 insertions(+)
 create mode 100644 drivers/irqchip/irq-ti-sci-intr.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a23778b68d74..cf3c834f8cee 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14626,6 +14626,7 @@ F:	Documentation/devicetree/bindings/clock/ti,sci-clk.txt
 F:	drivers/clk/keystone/sci-clk.c
 F:	drivers/reset/reset-ti-sci.c
 F:	Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
+F:	drivers/irqchip/irq-ti-sci-intr.c
 
 THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER
 M:	Hans Verkuil <hverkuil@xs4all.nl>
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 96451b581452..9a965fe22043 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -374,6 +374,17 @@ config QCOM_PDC
 	  Power Domain Controller driver to manage and configure wakeup
 	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
 
+config TI_SCI_INTR_IRQCHIP
+	tristate "TISCI based Interrupt Router irqchip driver"
+	depends on TI_SCI_PROTOCOL && ARCH_K3
+	select IRQ_DOMAIN
+	select IRQ_DOMAIN_HIERARCHY
+	help
+	  This enables the irqchip driver support for K3 Interrupt router
+	  over TI System Control Interface available on some new TI's SoCs.
+	  If you wish to use interrupt router irq resources managed by the
+	  TI System Controller, say Y here. Otherwise, say N.
+
 endmenu
 
 config SIFIVE_PLIC
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index b822199445ff..44bf65606d60 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -89,3 +89,4 @@ obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
 obj-$(CONFIG_NDS32)			+= irq-ativic32.o
 obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
 obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
+obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
diff --git a/drivers/irqchip/irq-ti-sci-intr.c b/drivers/irqchip/irq-ti-sci-intr.c
new file mode 100644
index 000000000000..f04fe6da1b09
--- /dev/null
+++ b/drivers/irqchip/irq-ti-sci-intr.c
@@ -0,0 +1,325 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Texas Instruments' K3 Interrupt Router irqchip driver
+ *
+ * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
+ *	Lokesh Vutla <lokeshvutla@ti.com>
+ */
+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/irqchip.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/irqdomain.h>
+#include <linux/soc/ti/ti_sci_protocol.h>
+
+#define TI_SCI_DEV_ID_MASK	0xffff
+#define TI_SCI_DEV_ID_SHIFT	16
+#define TI_SCI_IRQ_ID_MASK	0xffff
+#define TI_SCI_IRQ_ID_SHIFT	0
+#define TI_SCI_IS_EVENT_IRQ	BIT(31)
+
+#define HWIRQ_TO_DEVID(HWIRQ)	(((HWIRQ) >> (TI_SCI_DEV_ID_SHIFT)) & \
+				 (TI_SCI_DEV_ID_MASK))
+#define HWIRQ_TO_IRQID(HWIRQ)	((HWIRQ) & (TI_SCI_IRQ_ID_MASK))
+
+/**
+ * struct ti_sci_intr_irq_domain - Structure representing a TISCI based
+ *				   Interrupt Router IRQ domain.
+ * @sci:	Pointer to TISCI handle
+ * @dst_irq:	TISCI resource pointer representing destination irq controller.
+ * @dst_id:	TISCI device ID of the destination irq controller.
+ */
+struct ti_sci_intr_irq_domain {
+	const struct ti_sci_handle *sci;
+	struct ti_sci_resource *dst_irq;
+	u16 dst_id;
+};
+
+/**
+ * struct ti_sci_intr_irq_desc - Description of an Interrupt Router IRQ
+ * @src_id:		TISCI device ID of the IRQ source
+ * @src_index:		IRQ source index within the device.
+ * @dst_irq:		Destination host IRQ.
+ */
+struct ti_sci_intr_irq_desc {
+	u16 src_id;
+	u16 src_index;
+	u16 dst_irq;
+};
+
+static struct irq_chip ti_sci_intr_irq_chip = {
+	.name			= "INTR",
+	.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,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+};
+
+/**
+ * ti_sci_intr_irq_domain_translate() - Retrieve hwirq and type from
+ *					IRQ firmware specific handler.
+ * @domain:	Pointer to IRQ domain
+ * @fwspec:	Pointer to IRQ specific firmware structure
+ * @hwirq:	IRQ number identified by hardware
+ * @type:	IRQ type
+ *
+ * Return 0 if all went ok else appropriate error.
+ */
+static int ti_sci_intr_irq_domain_translate(struct irq_domain *domain,
+					    struct irq_fwspec *fwspec,
+					    unsigned long *hwirq,
+					    unsigned int *type)
+{
+	if (is_of_node(fwspec->fwnode)) {
+		if (fwspec->param_count != 3)
+			return -EINVAL;
+
+		*hwirq = ((fwspec->param[0] & TI_SCI_DEV_ID_MASK) <<
+			  TI_SCI_DEV_ID_SHIFT) |
+			 (fwspec->param[1] & TI_SCI_IRQ_ID_MASK);
+		*type = fwspec->param[2];
+
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static inline void ti_sci_intr_delete_desc(struct ti_sci_intr_irq_domain *intr,
+					   struct ti_sci_intr_irq_desc *desc)
+{
+	intr->sci->ops.rm_irq_ops.free_direct_irq(intr->sci, desc->src_id,
+						  desc->src_index,
+						  intr->dst_id, desc->dst_irq);
+	pr_debug("%s: IRQ deleted from src = %d, src_index = %d, to dst = %d, dst_irq = %d\n",
+		 __func__, desc->src_id, desc->src_index, intr->dst_id,
+		desc->dst_irq);
+}
+
+/**
+ * ti_sci_intr_irq_domain_free() - Free the specified IRQs from the domain.
+ * @domain:	Domain to which the irqs belong
+ * @virq:	Linux virtual IRQ to be freed.
+ * @nr_irqs:	Number of continuous irqs to be freed
+ */
+static void ti_sci_intr_irq_domain_free(struct irq_domain *domain,
+					unsigned int virq, unsigned int nr_irqs)
+{
+	struct ti_sci_intr_irq_domain *intr = domain->host_data;
+	struct ti_sci_intr_irq_desc *desc;
+	struct irq_data *data;
+	int i;
+
+	intr = domain->host_data;
+
+	for (i = 0; i < nr_irqs; i++) {
+		data = irq_domain_get_irq_data(domain, virq + i);
+
+		desc = irq_data_get_irq_chip_data(data);
+
+		ti_sci_intr_delete_desc(intr, desc);
+		irq_domain_free_irqs_parent(domain, virq, 1);
+		irq_domain_reset_irq_data(data);
+		ti_sci_release_resource(intr->dst_irq, desc->dst_irq);
+		kfree(desc);
+	}
+}
+
+/**
+ * allocate_gic_irq() - Allocate GIC specific IRQ
+ * @domain:	Point to the interrupt router IRQ domain
+ * @dev:	TISCI device IRQ generating the IRQ
+ * @irq:	IRQ offset within the device
+ * @flags:	Corresponding flags to the IRQ
+ *
+ * Returns pointer to irq descriptor if all went well else appropriate
+ * error pointer.
+ */
+static struct ti_sci_intr_irq_desc *allocate_gic_irq(struct irq_domain *domain,
+						     unsigned int virq,
+						     u16 dev, u16 irq,
+						     u32 flags)
+{
+	struct ti_sci_intr_irq_domain *intr = domain->host_data;
+	struct ti_sci_intr_irq_desc *desc;
+	struct irq_fwspec fwspec;
+	int err;
+
+	if (!irq_domain_get_of_node(domain->parent))
+		return ERR_PTR(-EINVAL);
+
+	desc = kzalloc(sizeof(*desc), GFP_KERNEL);
+	if (!desc)
+		return ERR_PTR(-ENOMEM);
+
+	desc->src_id = dev;
+	desc->src_index = irq;
+	desc->dst_irq = ti_sci_get_free_resource(intr->dst_irq);
+
+	fwspec.fwnode = domain->parent->fwnode;
+	fwspec.param_count = 3;
+	fwspec.param[0] = 0;	/* SPI */
+	fwspec.param[1] = desc->dst_irq - 32; /* SPI offset */
+	fwspec.param[2] = flags & IRQ_TYPE_SENSE_MASK;
+
+	err = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
+	if (err)
+		goto err_irqs;
+
+	pr_debug("%s: IRQ requested from src = %d, src_index = %d, to dst = %d, dst_irq = %d\n",
+		 __func__, desc->src_id, desc->src_index, intr->dst_id,
+		 desc->dst_irq);
+
+	err = intr->sci->ops.rm_irq_ops.set_direct_irq(intr->sci, desc->src_id,
+						       desc->src_index,
+						       intr->dst_id,
+						       desc->dst_irq);
+	if (err) {
+		pr_err("%s: IRQ allocation failed from src = %d, src_index = %d to dst_id = %d, dst_irq = %d",
+		       __func__, desc->src_id, desc->src_index, intr->dst_id,
+		       desc->dst_irq);
+		goto err_msg;
+	}
+
+	return desc;
+
+err_msg:
+	irq_domain_free_irqs_parent(domain, virq, 1);
+err_irqs:
+	ti_sci_release_resource(intr->dst_irq, desc->dst_irq);
+	kfree(desc);
+	return ERR_PTR(err);
+}
+
+/**
+ * ti_sci_intr_irq_domain_alloc() - Allocate Interrupt router IRQs
+ * @domain:	Point to the interrupt router IRQ domain
+ * @virq:	Corresponding Linux virtual IRQ number
+ * @nr_irqs:	Continuous irqs to be allocated
+ * @data:	Pointer to firmware specifier
+ *
+ * Return 0 if all went well else appropriate error value.
+ */
+static int ti_sci_intr_irq_domain_alloc(struct irq_domain *domain,
+					unsigned int virq, unsigned int nr_irqs,
+					void *data)
+{
+	struct irq_fwspec *fwspec = data;
+	struct ti_sci_intr_irq_desc *desc;
+	unsigned long hwirq;
+	u16 src_id, src_index;
+	int i, err;
+	u32 type;
+
+	err = ti_sci_intr_irq_domain_translate(domain, fwspec, &hwirq, &type);
+	if (err)
+		return err;
+
+	src_id = HWIRQ_TO_DEVID(hwirq);
+	src_index = HWIRQ_TO_IRQID(hwirq);
+
+	for (i = 0; i < nr_irqs; i++) {
+		desc = allocate_gic_irq(domain, virq + i, src_id, src_index + i,
+					type);
+		if (IS_ERR(desc))
+			/* ToDO: Clean already allocated IRQs */
+			return PTR_ERR(desc);
+
+		err = irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
+						    &ti_sci_intr_irq_chip,
+						    desc);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static const struct irq_domain_ops ti_sci_intr_irq_domain_ops = {
+	.alloc		= ti_sci_intr_irq_domain_alloc,
+	.free		= ti_sci_intr_irq_domain_free,
+	.translate	= ti_sci_intr_irq_domain_translate,
+};
+
+static int ti_sci_intr_irq_domain_probe(struct platform_device *pdev)
+{
+	struct irq_domain *parent_domain, *domain;
+	struct ti_sci_intr_irq_domain *intr;
+	struct device_node *parent_node;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	parent_node = of_irq_find_parent(dev_of_node(dev));
+	if (!parent_node) {
+		dev_err(dev, "Failed to get IRQ parent node\n");
+		return -ENODEV;
+	}
+
+	parent_domain = irq_find_host(parent_node);
+	if (!parent_domain) {
+		dev_err(dev, "Failed to find IRQ parent domain\n");
+		return -ENODEV;
+	}
+
+	intr = devm_kzalloc(dev, sizeof(*intr), GFP_KERNEL);
+	if (!intr)
+		return -ENOMEM;
+
+	intr->sci = devm_ti_sci_get_by_phandle(dev, "ti,sci");
+	if (IS_ERR(intr->sci)) {
+		ret = PTR_ERR(intr->sci);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "ti,sci read fail %d\n", ret);
+		intr->sci = NULL;
+		return ret;
+	}
+
+	intr->dst_irq = devm_ti_sci_get_of_resource(intr->sci, dev,
+						    "ti,sci-rm-range-girq");
+	if (IS_ERR(intr->dst_irq)) {
+		dev_err(dev, "Destination irq resource allocation failed\n");
+		return PTR_ERR(intr->dst_irq);
+	}
+
+	ret = of_property_read_u32(dev_of_node(dev), "ti,sci-dst-id",
+				   (u32 *)&intr->dst_id);
+	if (ret) {
+		dev_err(dev, "missing 'ti,sci-dst-id' property\n");
+		return -EINVAL;
+	}
+
+	domain = irq_domain_add_hierarchy(parent_domain, 0, 0, dev_of_node(dev),
+					  &ti_sci_intr_irq_domain_ops, intr);
+	if (!domain) {
+		dev_err(dev, "Failed to allocate IRQ domain\n");
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id ti_sci_intr_irq_domain_of_match[] = {
+	{ .compatible = "ti,sci-intr", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, ti_sci_intr_irq_domain_of_match);
+
+static struct platform_driver ti_sci_intr_irq_domain_driver = {
+	.probe = ti_sci_intr_irq_domain_probe,
+	.driver = {
+		.name = "ti-sci-intr",
+		.of_match_table = ti_sci_intr_irq_domain_of_match,
+	},
+};
+module_platform_driver(ti_sci_intr_irq_domain_driver);
+
+MODULE_AUTHOR("Lokesh Vutla <lokeshvutla@ticom>");
+MODULE_DESCRIPTION("K3 Interrupt Router driver over TI SCI protocol");
+MODULE_LICENSE("GPL v2");
-- 
2.19.0

Re: [PATCH 2/2] irqchip: ti-sci-intr: Add support for Interrupt Router driver

[Marc Zyngier]

Hi Lokesh,

On Sat, 06 Oct 2018 08:28:12 +0100,
Lokesh Vutla <lokeshvutla@ti.com> wrote:
> 
> Texas Instruments' K3 generation SoCs has an IP Interrupt Router
> that does allows for multiplexing of input interrupts to host
> interrupt controller. Interrupt Router inputs are either from a
> peripheral or from an Interrupt Aggregator which is another
> interrupt controller.
> 
> Configuration of the interrupt router registers can only be done by
> a system co-processor and the driver needs to send a message to this
> co processor over TISCI protocol.

I assume that this co-processor only deals with the routing itself,
and doesn't need to be talked to during interrupt processing, right?

> 
> Add support for Interrupt Router driver over TISCI protocol.
> 
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
>  MAINTAINERS                       |   1 +
>  drivers/irqchip/Kconfig           |  11 +
>  drivers/irqchip/Makefile          |   1 +
>  drivers/irqchip/irq-ti-sci-intr.c | 325 ++++++++++++++++++++++++++++++
>  4 files changed, 338 insertions(+)
>  create mode 100644 drivers/irqchip/irq-ti-sci-intr.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a23778b68d74..cf3c834f8cee 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14626,6 +14626,7 @@ F:	Documentation/devicetree/bindings/clock/ti,sci-clk.txt
>  F:	drivers/clk/keystone/sci-clk.c
>  F:	drivers/reset/reset-ti-sci.c
>  F:	Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
> +F:	drivers/irqchip/irq-ti-sci-intr.c
>  
>  THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER
>  M:	Hans Verkuil <hverkuil@xs4all.nl>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 96451b581452..9a965fe22043 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -374,6 +374,17 @@ config QCOM_PDC
>  	  Power Domain Controller driver to manage and configure wakeup
>  	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
>  
> +config TI_SCI_INTR_IRQCHIP
> +	tristate "TISCI based Interrupt Router irqchip driver"
> +	depends on TI_SCI_PROTOCOL && ARCH_K3
> +	select IRQ_DOMAIN
> +	select IRQ_DOMAIN_HIERARCHY
> +	help
> +	  This enables the irqchip driver support for K3 Interrupt router
> +	  over TI System Control Interface available on some new TI's SoCs.
> +	  If you wish to use interrupt router irq resources managed by the
> +	  TI System Controller, say Y here. Otherwise, say N.

I don't really see the point of making this user-selectable. If you're
compiling support for a given platform, this platform configuration
fragment should itself select the necessary dependencies for the
system to work as expected. Here, you are leaving the choice to the
user, with a 50% chance of getting a system that doesn't boot...

> +
>  endmenu
>  
>  config SIFIVE_PLIC
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index b822199445ff..44bf65606d60 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -89,3 +89,4 @@ obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
>  obj-$(CONFIG_NDS32)			+= irq-ativic32.o
>  obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
>  obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
> +obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
> diff --git a/drivers/irqchip/irq-ti-sci-intr.c b/drivers/irqchip/irq-ti-sci-intr.c
> new file mode 100644
> index 000000000000..f04fe6da1b09
> --- /dev/null
> +++ b/drivers/irqchip/irq-ti-sci-intr.c
> @@ -0,0 +1,325 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Texas Instruments' K3 Interrupt Router irqchip driver
> + *
> + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
> + *	Lokesh Vutla <lokeshvutla@ti.com>
> + */
> +
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/irqchip.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/irqdomain.h>
> +#include <linux/soc/ti/ti_sci_protocol.h>
> +
> +#define TI_SCI_DEV_ID_MASK	0xffff
> +#define TI_SCI_DEV_ID_SHIFT	16
> +#define TI_SCI_IRQ_ID_MASK	0xffff
> +#define TI_SCI_IRQ_ID_SHIFT	0
> +#define TI_SCI_IS_EVENT_IRQ	BIT(31)
> +
> +#define HWIRQ_TO_DEVID(HWIRQ)	(((HWIRQ) >> (TI_SCI_DEV_ID_SHIFT)) & \
> +				 (TI_SCI_DEV_ID_MASK))
> +#define HWIRQ_TO_IRQID(HWIRQ)	((HWIRQ) & (TI_SCI_IRQ_ID_MASK))

nit: s/(HWIRQ)/(hwirq)/g

> +
> +/**
> + * struct ti_sci_intr_irq_domain - Structure representing a TISCI based
> + *				   Interrupt Router IRQ domain.
> + * @sci:	Pointer to TISCI handle
> + * @dst_irq:	TISCI resource pointer representing destination irq controller.
> + * @dst_id:	TISCI device ID of the destination irq controller.
> + */
> +struct ti_sci_intr_irq_domain {
> +	const struct ti_sci_handle *sci;
> +	struct ti_sci_resource *dst_irq;
> +	u16 dst_id;
> +};
> +
> +/**
> + * struct ti_sci_intr_irq_desc - Description of an Interrupt Router IRQ
> + * @src_id:		TISCI device ID of the IRQ source
> + * @src_index:		IRQ source index within the device.
> + * @dst_irq:		Destination host IRQ.
> + */
> +struct ti_sci_intr_irq_desc {
> +	u16 src_id;
> +	u16 src_index;
> +	u16 dst_irq;
> +};

Oh great. So this is reinventing the GICv3 ITS, only for SPIs. :-(

Now, this structure seems completely useless, see below.

> +
> +static struct irq_chip ti_sci_intr_irq_chip = {
> +	.name			= "INTR",
> +	.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,
> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
> +};
> +
> +/**
> + * ti_sci_intr_irq_domain_translate() - Retrieve hwirq and type from
> + *					IRQ firmware specific handler.
> + * @domain:	Pointer to IRQ domain
> + * @fwspec:	Pointer to IRQ specific firmware structure
> + * @hwirq:	IRQ number identified by hardware
> + * @type:	IRQ type
> + *
> + * Return 0 if all went ok else appropriate error.
> + */
> +static int ti_sci_intr_irq_domain_translate(struct irq_domain *domain,
> +					    struct irq_fwspec *fwspec,
> +					    unsigned long *hwirq,
> +					    unsigned int *type)
> +{
> +	if (is_of_node(fwspec->fwnode)) {
> +		if (fwspec->param_count != 3)
> +			return -EINVAL;
> +
> +		*hwirq = ((fwspec->param[0] & TI_SCI_DEV_ID_MASK) <<
> +			  TI_SCI_DEV_ID_SHIFT) |
> +			 (fwspec->param[1] & TI_SCI_IRQ_ID_MASK);

Maybe it would make sense to have a macro that hides this:

      	       *hwirq = FWSPEC_TO_HWIRQ(fwspec);

> +		*type = fwspec->param[2];
> +
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static inline void ti_sci_intr_delete_desc(struct ti_sci_intr_irq_domain *intr,
> +					   struct ti_sci_intr_irq_desc *desc)
> +{
> +	intr->sci->ops.rm_irq_ops.free_direct_irq(intr->sci, desc->src_id,
> +						  desc->src_index,
> +						  intr->dst_id, desc->dst_irq);

This looks horrible. Why doesn't your firmware interface have a helper
functions that hides this? Something like:

	ti_sci_free_direct_irq(intr, src_id, src_index, dst_irq);

and you could even add some error checking.

> +	pr_debug("%s: IRQ deleted from src = %d, src_index = %d, to dst = %d, dst_irq = %d\n",
> +		 __func__, desc->src_id, desc->src_index, intr->dst_id,
> +		desc->dst_irq);

And put this where it belongs (in the helper function).

> +}
> +
> +/**
> + * ti_sci_intr_irq_domain_free() - Free the specified IRQs from the domain.
> + * @domain:	Domain to which the irqs belong
> + * @virq:	Linux virtual IRQ to be freed.
> + * @nr_irqs:	Number of continuous irqs to be freed
> + */
> +static void ti_sci_intr_irq_domain_free(struct irq_domain *domain,
> +					unsigned int virq, unsigned int nr_irqs)
> +{
> +	struct ti_sci_intr_irq_domain *intr = domain->host_data;
> +	struct ti_sci_intr_irq_desc *desc;
> +	struct irq_data *data;
> +	int i;
> +
> +	intr = domain->host_data;
> +
> +	for (i = 0; i < nr_irqs; i++) {
> +		data = irq_domain_get_irq_data(domain, virq + i);
> +
> +		desc = irq_data_get_irq_chip_data(data);
> +
> +		ti_sci_intr_delete_desc(intr, desc);
> +		irq_domain_free_irqs_parent(domain, virq, 1);
> +		irq_domain_reset_irq_data(data);
> +		ti_sci_release_resource(intr->dst_irq, desc->dst_irq);
> +		kfree(desc);
> +	}
> +}
> +
> +/**
> + * allocate_gic_irq() - Allocate GIC specific IRQ
> + * @domain:	Point to the interrupt router IRQ domain
> + * @dev:	TISCI device IRQ generating the IRQ
> + * @irq:	IRQ offset within the device
> + * @flags:	Corresponding flags to the IRQ
> + *
> + * Returns pointer to irq descriptor if all went well else appropriate
> + * error pointer.
> + */
> +static struct ti_sci_intr_irq_desc *allocate_gic_irq(struct irq_domain *domain,
> +						     unsigned int virq,
> +						     u16 dev, u16 irq,
> +						     u32 flags)
> +{
> +	struct ti_sci_intr_irq_domain *intr = domain->host_data;
> +	struct ti_sci_intr_irq_desc *desc;
> +	struct irq_fwspec fwspec;
> +	int err;
> +
> +	if (!irq_domain_get_of_node(domain->parent))
> +		return ERR_PTR(-EINVAL);
> +
> +	desc = kzalloc(sizeof(*desc), GFP_KERNEL);
> +	if (!desc)
> +		return ERR_PTR(-ENOMEM);
> +
> +	desc->src_id = dev;
> +	desc->src_index = irq;
> +	desc->dst_irq = ti_sci_get_free_resource(intr->dst_irq);

I don't think this structure serves any purpose. src_id and src_index
are just a decomposition of hwirq. dst_irq is the GIC interrupt, which
is stored... by the GIC driver. Also, it is worth realising that
you're allocating per-interrupt data, but none of the per-interrupt
callbacks are using it. In my book, that's a sure sign that this
structure is pointless.

Am I missing anything here?

> +
> +	fwspec.fwnode = domain->parent->fwnode;
> +	fwspec.param_count = 3;
> +	fwspec.param[0] = 0;	/* SPI */
> +	fwspec.param[1] = desc->dst_irq - 32; /* SPI offset */
> +	fwspec.param[2] = flags & IRQ_TYPE_SENSE_MASK;
> +
> +	err = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
> +	if (err)
> +		goto err_irqs;
> +
> +	pr_debug("%s: IRQ requested from src = %d, src_index = %d, to dst = %d, dst_irq = %d\n",
> +		 __func__, desc->src_id, desc->src_index, intr->dst_id,
> +		 desc->dst_irq);
> +
> +	err = intr->sci->ops.rm_irq_ops.set_direct_irq(intr->sci, desc->src_id,
> +						       desc->src_index,
> +						       intr->dst_id,
> +						       desc->dst_irq);

Same remarks about the horrible interface.

> +	if (err) {
> +		pr_err("%s: IRQ allocation failed from src = %d, src_index = %d to dst_id = %d, dst_irq = %d",
> +		       __func__, desc->src_id, desc->src_index, intr->dst_id,
> +		       desc->dst_irq);
> +		goto err_msg;
> +	}
> +
> +	return desc;
> +
> +err_msg:
> +	irq_domain_free_irqs_parent(domain, virq, 1);
> +err_irqs:
> +	ti_sci_release_resource(intr->dst_irq, desc->dst_irq);
> +	kfree(desc);
> +	return ERR_PTR(err);
> +}
> +
> +/**
> + * ti_sci_intr_irq_domain_alloc() - Allocate Interrupt router IRQs
> + * @domain:	Point to the interrupt router IRQ domain
> + * @virq:	Corresponding Linux virtual IRQ number
> + * @nr_irqs:	Continuous irqs to be allocated
> + * @data:	Pointer to firmware specifier
> + *
> + * Return 0 if all went well else appropriate error value.
> + */
> +static int ti_sci_intr_irq_domain_alloc(struct irq_domain *domain,
> +					unsigned int virq, unsigned int nr_irqs,
> +					void *data)
> +{
> +	struct irq_fwspec *fwspec = data;
> +	struct ti_sci_intr_irq_desc *desc;
> +	unsigned long hwirq;
> +	u16 src_id, src_index;
> +	int i, err;
> +	u32 type;
> +
> +	err = ti_sci_intr_irq_domain_translate(domain, fwspec, &hwirq, &type);
> +	if (err)
> +		return err;
> +
> +	src_id = HWIRQ_TO_DEVID(hwirq);
> +	src_index = HWIRQ_TO_IRQID(hwirq);
> +
> +	for (i = 0; i < nr_irqs; i++) {
> +		desc = allocate_gic_irq(domain, virq + i, src_id, src_index + i,
> +					type);
> +		if (IS_ERR(desc))
> +			/* ToDO: Clean already allocated IRQs */
> +			return PTR_ERR(desc);

Please address this. But it also worth realising that this code will
never be called with nr_irqs!=1 (that's only for things like PCI
Multi-MSI).

> +
> +		err = irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> +						    &ti_sci_intr_irq_chip,
> +						    desc);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops ti_sci_intr_irq_domain_ops = {
> +	.alloc		= ti_sci_intr_irq_domain_alloc,
> +	.free		= ti_sci_intr_irq_domain_free,
> +	.translate	= ti_sci_intr_irq_domain_translate,
> +};
> +
> +static int ti_sci_intr_irq_domain_probe(struct platform_device *pdev)
> +{
> +	struct irq_domain *parent_domain, *domain;
> +	struct ti_sci_intr_irq_domain *intr;
> +	struct device_node *parent_node;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	parent_node = of_irq_find_parent(dev_of_node(dev));
> +	if (!parent_node) {
> +		dev_err(dev, "Failed to get IRQ parent node\n");
> +		return -ENODEV;
> +	}
> +
> +	parent_domain = irq_find_host(parent_node);
> +	if (!parent_domain) {
> +		dev_err(dev, "Failed to find IRQ parent domain\n");
> +		return -ENODEV;
> +	}
> +
> +	intr = devm_kzalloc(dev, sizeof(*intr), GFP_KERNEL);
> +	if (!intr)
> +		return -ENOMEM;
> +
> +	intr->sci = devm_ti_sci_get_by_phandle(dev, "ti,sci");
> +	if (IS_ERR(intr->sci)) {
> +		ret = PTR_ERR(intr->sci);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "ti,sci read fail %d\n", ret);
> +		intr->sci = NULL;
> +		return ret;
> +	}
> +
> +	intr->dst_irq = devm_ti_sci_get_of_resource(intr->sci, dev,
> +						    "ti,sci-rm-range-girq");
> +	if (IS_ERR(intr->dst_irq)) {
> +		dev_err(dev, "Destination irq resource allocation failed\n");
> +		return PTR_ERR(intr->dst_irq);
> +	}
> +
> +	ret = of_property_read_u32(dev_of_node(dev), "ti,sci-dst-id",
> +				   (u32 *)&intr->dst_id);
> +	if (ret) {
> +		dev_err(dev, "missing 'ti,sci-dst-id' property\n");
> +		return -EINVAL;
> +	}

Do you expect other drivers to require similar resource request? If
so, It might be worth getting the firmware interface to do that
work. Specially the "give me my SCI" part.

> +
> +	domain = irq_domain_add_hierarchy(parent_domain, 0, 0, dev_of_node(dev),
> +					  &ti_sci_intr_irq_domain_ops, intr);
> +	if (!domain) {
> +		dev_err(dev, "Failed to allocate IRQ domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ti_sci_intr_irq_domain_of_match[] = {
> +	{ .compatible = "ti,sci-intr", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, ti_sci_intr_irq_domain_of_match);
> +
> +static struct platform_driver ti_sci_intr_irq_domain_driver = {
> +	.probe = ti_sci_intr_irq_domain_probe,
> +	.driver = {
> +		.name = "ti-sci-intr",
> +		.of_match_table = ti_sci_intr_irq_domain_of_match,
> +	},
> +};
> +module_platform_driver(ti_sci_intr_irq_domain_driver);
> +
> +MODULE_AUTHOR("Lokesh Vutla <lokeshvutla@ticom>");
> +MODULE_DESCRIPTION("K3 Interrupt Router driver over TI SCI protocol");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.19.0
> 

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

Re: [PATCH 2/2] irqchip: ti-sci-intr: Add support for Interrupt Router driver

[Lokesh Vutla]

Hi Marc,

On 10/6/2018 3:25 PM, Marc Zyngier wrote:
> Hi Lokesh,
> 
> On Sat, 06 Oct 2018 08:28:12 +0100,
> Lokesh Vutla <lokeshvutla@ti.com> wrote:
>>
>> Texas Instruments' K3 generation SoCs has an IP Interrupt Router
>> that does allows for multiplexing of input interrupts to host
>> interrupt controller. Interrupt Router inputs are either from a
>> peripheral or from an Interrupt Aggregator which is another
>> interrupt controller.
>>
>> Configuration of the interrupt router registers can only be done by
>> a system co-processor and the driver needs to send a message to this
>> co processor over TISCI protocol.
> 
> I assume that this co-processor only deals with the routing itself,
> and doesn't need to be talked to during interrupt processing, right?

Yes, that's right.

> 
>>
>> Add support for Interrupt Router driver over TISCI protocol.
>>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> ---
>>  MAINTAINERS                       |   1 +
>>  drivers/irqchip/Kconfig           |  11 +
>>  drivers/irqchip/Makefile          |   1 +
>>  drivers/irqchip/irq-ti-sci-intr.c | 325 ++++++++++++++++++++++++++++++
>>  4 files changed, 338 insertions(+)
>>  create mode 100644 drivers/irqchip/irq-ti-sci-intr.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index a23778b68d74..cf3c834f8cee 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -14626,6 +14626,7 @@ F:	Documentation/devicetree/bindings/clock/ti,sci-clk.txt
>>  F:	drivers/clk/keystone/sci-clk.c
>>  F:	drivers/reset/reset-ti-sci.c
>>  F:	Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
>> +F:	drivers/irqchip/irq-ti-sci-intr.c
>>  
>>  THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER
>>  M:	Hans Verkuil <hverkuil@xs4all.nl>
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index 96451b581452..9a965fe22043 100644
>> --- a/drivers/irqchip/Kconfig
>> +++ b/drivers/irqchip/Kconfig
>> @@ -374,6 +374,17 @@ config QCOM_PDC
>>  	  Power Domain Controller driver to manage and configure wakeup
>>  	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
>>  
>> +config TI_SCI_INTR_IRQCHIP
>> +	tristate "TISCI based Interrupt Router irqchip driver"
>> +	depends on TI_SCI_PROTOCOL && ARCH_K3
>> +	select IRQ_DOMAIN
>> +	select IRQ_DOMAIN_HIERARCHY
>> +	help
>> +	  This enables the irqchip driver support for K3 Interrupt router
>> +	  over TI System Control Interface available on some new TI's SoCs.
>> +	  If you wish to use interrupt router irq resources managed by the
>> +	  TI System Controller, say Y here. Otherwise, say N.
> 
> I don't really see the point of making this user-selectable. If you're
> compiling support for a given platform, this platform configuration
> fragment should itself select the necessary dependencies for the
> system to work as expected. Here, you are leaving the choice to the
> user, with a 50% chance of getting a system that doesn't boot...

There are 2 reasons why I made it tristate:
- Not all interrupts go through this irqchip(At least in the AM6 SoC
using this). Most of the legacy peripherals still are directly connected
to GIC
- TI_SCI_PROTOCOL is defined as tristate.

If you still feel I should not make it user-selectable, I can drop it.

> 
>> +
>>  endmenu
>>  
>>  config SIFIVE_PLIC
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index b822199445ff..44bf65606d60 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -89,3 +89,4 @@ obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
>>  obj-$(CONFIG_NDS32)			+= irq-ativic32.o
>>  obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
>>  obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
>> +obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
>> diff --git a/drivers/irqchip/irq-ti-sci-intr.c b/drivers/irqchip/irq-ti-sci-intr.c
>> new file mode 100644
>> index 000000000000..f04fe6da1b09
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-ti-sci-intr.c
>> @@ -0,0 +1,325 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Texas Instruments' K3 Interrupt Router irqchip driver
>> + *
>> + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
>> + *	Lokesh Vutla <lokeshvutla@ti.com>
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/irqchip.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/module.h>
>> +#include <linux/moduleparam.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/soc/ti/ti_sci_protocol.h>
>> +
>> +#define TI_SCI_DEV_ID_MASK	0xffff
>> +#define TI_SCI_DEV_ID_SHIFT	16
>> +#define TI_SCI_IRQ_ID_MASK	0xffff
>> +#define TI_SCI_IRQ_ID_SHIFT	0
>> +#define TI_SCI_IS_EVENT_IRQ	BIT(31)
>> +
>> +#define HWIRQ_TO_DEVID(HWIRQ)	(((HWIRQ) >> (TI_SCI_DEV_ID_SHIFT)) & \
>> +				 (TI_SCI_DEV_ID_MASK))
>> +#define HWIRQ_TO_IRQID(HWIRQ)	((HWIRQ) & (TI_SCI_IRQ_ID_MASK))
> 
> nit: s/(HWIRQ)/(hwirq)/g

okay.

> 
>> +
>> +/**
>> + * struct ti_sci_intr_irq_domain - Structure representing a TISCI based
>> + *				   Interrupt Router IRQ domain.
>> + * @sci:	Pointer to TISCI handle
>> + * @dst_irq:	TISCI resource pointer representing destination irq controller.
>> + * @dst_id:	TISCI device ID of the destination irq controller.
>> + */
>> +struct ti_sci_intr_irq_domain {
>> +	const struct ti_sci_handle *sci;
>> +	struct ti_sci_resource *dst_irq;
>> +	u16 dst_id;
>> +};
>> +
>> +/**
>> + * struct ti_sci_intr_irq_desc - Description of an Interrupt Router IRQ
>> + * @src_id:		TISCI device ID of the IRQ source
>> + * @src_index:		IRQ source index within the device.
>> + * @dst_irq:		Destination host IRQ.
>> + */
>> +struct ti_sci_intr_irq_desc {
>> +	u16 src_id;
>> +	u16 src_index;
>> +	u16 dst_irq;
>> +};
> 
> Oh great. So this is reinventing the GICv3 ITS, only for SPIs. :-(
> 
> Now, this structure seems completely useless, see below.
> 
>> +
>> +static struct irq_chip ti_sci_intr_irq_chip = {
>> +	.name			= "INTR",
>> +	.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,
>> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
>> +};
>> +
>> +/**
>> + * ti_sci_intr_irq_domain_translate() - Retrieve hwirq and type from
>> + *					IRQ firmware specific handler.
>> + * @domain:	Pointer to IRQ domain
>> + * @fwspec:	Pointer to IRQ specific firmware structure
>> + * @hwirq:	IRQ number identified by hardware
>> + * @type:	IRQ type
>> + *
>> + * Return 0 if all went ok else appropriate error.
>> + */
>> +static int ti_sci_intr_irq_domain_translate(struct irq_domain *domain,
>> +					    struct irq_fwspec *fwspec,
>> +					    unsigned long *hwirq,
>> +					    unsigned int *type)
>> +{
>> +	if (is_of_node(fwspec->fwnode)) {
>> +		if (fwspec->param_count != 3)
>> +			return -EINVAL;
>> +
>> +		*hwirq = ((fwspec->param[0] & TI_SCI_DEV_ID_MASK) <<
>> +			  TI_SCI_DEV_ID_SHIFT) |
>> +			 (fwspec->param[1] & TI_SCI_IRQ_ID_MASK);
> 
> Maybe it would make sense to have a macro that hides this:

okay.

> 
>       	       *hwirq = FWSPEC_TO_HWIRQ(fwspec);
> 
>> +		*type = fwspec->param[2];
>> +
>> +		return 0;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static inline void ti_sci_intr_delete_desc(struct ti_sci_intr_irq_domain *intr,
>> +					   struct ti_sci_intr_irq_desc *desc)
>> +{
>> +	intr->sci->ops.rm_irq_ops.free_direct_irq(intr->sci, desc->src_id,
>> +						  desc->src_index,
>> +						  intr->dst_id, desc->dst_irq);
> 
> This looks horrible. Why doesn't your firmware interface have a helper
> functions that hides this? Something like:
> 
> 	ti_sci_free_direct_irq(intr, src_id, src_index, dst_irq);
> 
> and you could even add some error checking.

All existing TISCI users follow the same convention, so I did not bother adding 
any such wrapper. Will update TISCI with these wrappers and see what firmware 
maintainer says.

> 
>> +	pr_debug("%s: IRQ deleted from src = %d, src_index = %d, to dst = %d, dst_irq = %d\n",
>> +		 __func__, desc->src_id, desc->src_index, intr->dst_id,
>> +		desc->dst_irq);
> 
> And put this where it belongs (in the helper function).
> 
>> +}
>> +
>> +/**
>> + * ti_sci_intr_irq_domain_free() - Free the specified IRQs from the domain.
>> + * @domain:	Domain to which the irqs belong
>> + * @virq:	Linux virtual IRQ to be freed.
>> + * @nr_irqs:	Number of continuous irqs to be freed
>> + */
>> +static void ti_sci_intr_irq_domain_free(struct irq_domain *domain,
>> +					unsigned int virq, unsigned int nr_irqs)
>> +{
>> +	struct ti_sci_intr_irq_domain *intr = domain->host_data;
>> +	struct ti_sci_intr_irq_desc *desc;
>> +	struct irq_data *data;
>> +	int i;
>> +
>> +	intr = domain->host_data;
>> +
>> +	for (i = 0; i < nr_irqs; i++) {
>> +		data = irq_domain_get_irq_data(domain, virq + i);
>> +
>> +		desc = irq_data_get_irq_chip_data(data);
>> +
>> +		ti_sci_intr_delete_desc(intr, desc);
>> +		irq_domain_free_irqs_parent(domain, virq, 1);
>> +		irq_domain_reset_irq_data(data);
>> +		ti_sci_release_resource(intr->dst_irq, desc->dst_irq);
>> +		kfree(desc);
>> +	}
>> +}
>> +
>> +/**
>> + * allocate_gic_irq() - Allocate GIC specific IRQ
>> + * @domain:	Point to the interrupt router IRQ domain
>> + * @dev:	TISCI device IRQ generating the IRQ
>> + * @irq:	IRQ offset within the device
>> + * @flags:	Corresponding flags to the IRQ
>> + *
>> + * Returns pointer to irq descriptor if all went well else appropriate
>> + * error pointer.
>> + */
>> +static struct ti_sci_intr_irq_desc *allocate_gic_irq(struct irq_domain *domain,
>> +						     unsigned int virq,
>> +						     u16 dev, u16 irq,
>> +						     u32 flags)
>> +{
>> +	struct ti_sci_intr_irq_domain *intr = domain->host_data;
>> +	struct ti_sci_intr_irq_desc *desc;
>> +	struct irq_fwspec fwspec;
>> +	int err;
>> +
>> +	if (!irq_domain_get_of_node(domain->parent))
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	desc = kzalloc(sizeof(*desc), GFP_KERNEL);
>> +	if (!desc)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	desc->src_id = dev;
>> +	desc->src_index = irq;
>> +	desc->dst_irq = ti_sci_get_free_resource(intr->dst_irq);
> 
> I don't think this structure serves any purpose. src_id and src_index
> are just a decomposition of hwirq. dst_irq is the GIC interrupt, which
> is stored... by the GIC driver. Also, it is worth realising that
> you're allocating per-interrupt data, but none of the per-interrupt
> callbacks are using it. In my book, that's a sure sign that this
> structure is pointless.

hmm..you are right, these 3 fields can be dropped completely.

> 
> Am I missing anything here? >
>> +
>> +	fwspec.fwnode = domain->parent->fwnode;
>> +	fwspec.param_count = 3;
>> +	fwspec.param[0] = 0;	/* SPI */
>> +	fwspec.param[1] = desc->dst_irq - 32; /* SPI offset */
>> +	fwspec.param[2] = flags & IRQ_TYPE_SENSE_MASK;
>> +
>> +	err = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
>> +	if (err)
>> +		goto err_irqs;
>> +
>> +	pr_debug("%s: IRQ requested from src = %d, src_index = %d, to dst = %d, dst_irq = %d\n",
>> +		 __func__, desc->src_id, desc->src_index, intr->dst_id,
>> +		 desc->dst_irq);
>> +
>> +	err = intr->sci->ops.rm_irq_ops.set_direct_irq(intr->sci, desc->src_id,
>> +						       desc->src_index,
>> +						       intr->dst_id,
>> +						       desc->dst_irq);
> 
> Same remarks about the horrible interface.
> 
>> +	if (err) {
>> +		pr_err("%s: IRQ allocation failed from src = %d, src_index = %d to dst_id = %d, dst_irq = %d",
>> +		       __func__, desc->src_id, desc->src_index, intr->dst_id,
>> +		       desc->dst_irq);
>> +		goto err_msg;
>> +	}
>> +
>> +	return desc;
>> +
>> +err_msg:
>> +	irq_domain_free_irqs_parent(domain, virq, 1);
>> +err_irqs:
>> +	ti_sci_release_resource(intr->dst_irq, desc->dst_irq);
>> +	kfree(desc);
>> +	return ERR_PTR(err);
>> +}
>> +
>> +/**
>> + * ti_sci_intr_irq_domain_alloc() - Allocate Interrupt router IRQs
>> + * @domain:	Point to the interrupt router IRQ domain
>> + * @virq:	Corresponding Linux virtual IRQ number
>> + * @nr_irqs:	Continuous irqs to be allocated
>> + * @data:	Pointer to firmware specifier
>> + *
>> + * Return 0 if all went well else appropriate error value.
>> + */
>> +static int ti_sci_intr_irq_domain_alloc(struct irq_domain *domain,
>> +					unsigned int virq, unsigned int nr_irqs,
>> +					void *data)
>> +{
>> +	struct irq_fwspec *fwspec = data;
>> +	struct ti_sci_intr_irq_desc *desc;
>> +	unsigned long hwirq;
>> +	u16 src_id, src_index;
>> +	int i, err;
>> +	u32 type;
>> +
>> +	err = ti_sci_intr_irq_domain_translate(domain, fwspec, &hwirq, &type);
>> +	if (err)
>> +		return err;
>> +
>> +	src_id = HWIRQ_TO_DEVID(hwirq);
>> +	src_index = HWIRQ_TO_IRQID(hwirq);
>> +
>> +	for (i = 0; i < nr_irqs; i++) {
>> +		desc = allocate_gic_irq(domain, virq + i, src_id, src_index + i,
>> +					type);
>> +		if (IS_ERR(desc))
>> +			/* ToDO: Clean already allocated IRQs */
>> +			return PTR_ERR(desc);
> 
> Please address this. But it also worth realising that this code will
> never be called with nr_irqs!=1 (that's only for things like PCI
> Multi-MSI).

will fix it in v2.

> 
>> +
>> +		err = irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
>> +						    &ti_sci_intr_irq_chip,
>> +						    desc);
>> +		if (err)
>> +			return err;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct irq_domain_ops ti_sci_intr_irq_domain_ops = {
>> +	.alloc		= ti_sci_intr_irq_domain_alloc,
>> +	.free		= ti_sci_intr_irq_domain_free,
>> +	.translate	= ti_sci_intr_irq_domain_translate,
>> +};
>> +
>> +static int ti_sci_intr_irq_domain_probe(struct platform_device *pdev)
>> +{
>> +	struct irq_domain *parent_domain, *domain;
>> +	struct ti_sci_intr_irq_domain *intr;
>> +	struct device_node *parent_node;
>> +	struct device *dev = &pdev->dev;
>> +	int ret;
>> +
>> +	parent_node = of_irq_find_parent(dev_of_node(dev));
>> +	if (!parent_node) {
>> +		dev_err(dev, "Failed to get IRQ parent node\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	parent_domain = irq_find_host(parent_node);
>> +	if (!parent_domain) {
>> +		dev_err(dev, "Failed to find IRQ parent domain\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	intr = devm_kzalloc(dev, sizeof(*intr), GFP_KERNEL);
>> +	if (!intr)
>> +		return -ENOMEM;
>> +
>> +	intr->sci = devm_ti_sci_get_by_phandle(dev, "ti,sci");
>> +	if (IS_ERR(intr->sci)) {
>> +		ret = PTR_ERR(intr->sci);
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(dev, "ti,sci read fail %d\n", ret);
>> +		intr->sci = NULL;
>> +		return ret;
>> +	}
>> +
>> +	intr->dst_irq = devm_ti_sci_get_of_resource(intr->sci, dev,
>> +						    "ti,sci-rm-range-girq");
>> +	if (IS_ERR(intr->dst_irq)) {
>> +		dev_err(dev, "Destination irq resource allocation failed\n");
>> +		return PTR_ERR(intr->dst_irq);
>> +	}
>> +
>> +	ret = of_property_read_u32(dev_of_node(dev), "ti,sci-dst-id",
>> +				   (u32 *)&intr->dst_id);
>> +	if (ret) {
>> +		dev_err(dev, "missing 'ti,sci-dst-id' property\n");
>> +		return -EINVAL;
>> +	}
> 
> Do you expect other drivers to require similar resource request? If
> so, It might be worth getting the firmware interface to do that
> work. Specially the "give me my SCI" part.

I tried to consolidate sci resource part under devm_ti_sci_get_of_resource() api 
but dst-id is something that is used by irqchip driver. So couldn't consolidate 
it and had to get it from dt in the driver probe.

Thanks and regards,
Lokesh

Re: [PATCH 2/2] irqchip: ti-sci-intr: Add support for Interrupt Router driver

[Marc Zyngier]

Hi Lokesh,

On 08/10/18 10:48, Lokesh Vutla wrote:
> Hi Marc,
> 
> On 10/6/2018 3:25 PM, Marc Zyngier wrote:
>> Hi Lokesh,
>>
>> On Sat, 06 Oct 2018 08:28:12 +0100,
>> Lokesh Vutla <lokeshvutla@ti.com> wrote:
>>>
>>> Texas Instruments' K3 generation SoCs has an IP Interrupt Router
>>> that does allows for multiplexing of input interrupts to host
>>> interrupt controller. Interrupt Router inputs are either from a
>>> peripheral or from an Interrupt Aggregator which is another
>>> interrupt controller.
>>>
>>> Configuration of the interrupt router registers can only be done by
>>> a system co-processor and the driver needs to send a message to this
>>> co processor over TISCI protocol.
>>
>> I assume that this co-processor only deals with the routing itself,
>> and doesn't need to be talked to during interrupt processing, right?
> 
> Yes, that's right.
> 
>>
>>>
>>> Add support for Interrupt Router driver over TISCI protocol.
>>>
>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>> ---
>>>   MAINTAINERS                       |   1 +
>>>   drivers/irqchip/Kconfig           |  11 +
>>>   drivers/irqchip/Makefile          |   1 +
>>>   drivers/irqchip/irq-ti-sci-intr.c | 325 ++++++++++++++++++++++++++++++
>>>   4 files changed, 338 insertions(+)
>>>   create mode 100644 drivers/irqchip/irq-ti-sci-intr.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index a23778b68d74..cf3c834f8cee 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -14626,6 +14626,7 @@ F:	Documentation/devicetree/bindings/clock/ti,sci-clk.txt
>>>   F:	drivers/clk/keystone/sci-clk.c
>>>   F:	drivers/reset/reset-ti-sci.c
>>>   F:	Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
>>> +F:	drivers/irqchip/irq-ti-sci-intr.c
>>>   
>>>   THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER
>>>   M:	Hans Verkuil <hverkuil@xs4all.nl>
>>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>>> index 96451b581452..9a965fe22043 100644
>>> --- a/drivers/irqchip/Kconfig
>>> +++ b/drivers/irqchip/Kconfig
>>> @@ -374,6 +374,17 @@ config QCOM_PDC
>>>   	  Power Domain Controller driver to manage and configure wakeup
>>>   	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
>>>   
>>> +config TI_SCI_INTR_IRQCHIP
>>> +	tristate "TISCI based Interrupt Router irqchip driver"
>>> +	depends on TI_SCI_PROTOCOL && ARCH_K3
>>> +	select IRQ_DOMAIN
>>> +	select IRQ_DOMAIN_HIERARCHY
>>> +	help
>>> +	  This enables the irqchip driver support for K3 Interrupt router
>>> +	  over TI System Control Interface available on some new TI's SoCs.
>>> +	  If you wish to use interrupt router irq resources managed by the
>>> +	  TI System Controller, say Y here. Otherwise, say N.
>>
>> I don't really see the point of making this user-selectable. If you're
>> compiling support for a given platform, this platform configuration
>> fragment should itself select the necessary dependencies for the
>> system to work as expected. Here, you are leaving the choice to the
>> user, with a 50% chance of getting a system that doesn't boot...
> 
> There are 2 reasons why I made it tristate:
> - Not all interrupts go through this irqchip(At least in the AM6 SoC
> using this). Most of the legacy peripherals still are directly connected
> to GIC
> - TI_SCI_PROTOCOL is defined as tristate.

But as you said, these are "legacy" interrupts, and most of the 
interesting stuff is routed through the system controller. We also try 
not to have core interrupt controllers as modules. As for having the 
firmware interface as a module, I wonder what the use-case is.

> If you still feel I should not make it user-selectable, I can drop it.

I really wonder what the added value is for the user.

> 
>>
>>> +
>>>   endmenu
>>>   
>>>   config SIFIVE_PLIC
>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>>> index b822199445ff..44bf65606d60 100644
>>> --- a/drivers/irqchip/Makefile
>>> +++ b/drivers/irqchip/Makefile
>>> @@ -89,3 +89,4 @@ obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
>>>   obj-$(CONFIG_NDS32)			+= irq-ativic32.o
>>>   obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
>>>   obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
>>> +obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
>>> diff --git a/drivers/irqchip/irq-ti-sci-intr.c b/drivers/irqchip/irq-ti-sci-intr.c
>>> new file mode 100644
>>> index 000000000000..f04fe6da1b09
>>> --- /dev/null
>>> +++ b/drivers/irqchip/irq-ti-sci-intr.c
>>> @@ -0,0 +1,325 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Texas Instruments' K3 Interrupt Router irqchip driver
>>> + *
>>> + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
>>> + *	Lokesh Vutla <lokeshvutla@ti.com>
>>> + */
>>> +
>>> +#include <linux/err.h>
>>> +#include <linux/io.h>
>>> +#include <linux/irqchip.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/of_irq.h>
>>> +#include <linux/module.h>
>>> +#include <linux/moduleparam.h>
>>> +#include <linux/irqdomain.h>
>>> +#include <linux/soc/ti/ti_sci_protocol.h>
>>> +
>>> +#define TI_SCI_DEV_ID_MASK	0xffff
>>> +#define TI_SCI_DEV_ID_SHIFT	16
>>> +#define TI_SCI_IRQ_ID_MASK	0xffff
>>> +#define TI_SCI_IRQ_ID_SHIFT	0
>>> +#define TI_SCI_IS_EVENT_IRQ	BIT(31)
>>> +
>>> +#define HWIRQ_TO_DEVID(HWIRQ)	(((HWIRQ) >> (TI_SCI_DEV_ID_SHIFT)) & \
>>> +				 (TI_SCI_DEV_ID_MASK))
>>> +#define HWIRQ_TO_IRQID(HWIRQ)	((HWIRQ) & (TI_SCI_IRQ_ID_MASK))
>>
>> nit: s/(HWIRQ)/(hwirq)/g
> 
> okay.
> 
>>
>>> +
>>> +/**
>>> + * struct ti_sci_intr_irq_domain - Structure representing a TISCI based
>>> + *				   Interrupt Router IRQ domain.
>>> + * @sci:	Pointer to TISCI handle
>>> + * @dst_irq:	TISCI resource pointer representing destination irq controller.
>>> + * @dst_id:	TISCI device ID of the destination irq controller.
>>> + */
>>> +struct ti_sci_intr_irq_domain {
>>> +	const struct ti_sci_handle *sci;
>>> +	struct ti_sci_resource *dst_irq;
>>> +	u16 dst_id;
>>> +};
>>> +
>>> +/**
>>> + * struct ti_sci_intr_irq_desc - Description of an Interrupt Router IRQ
>>> + * @src_id:		TISCI device ID of the IRQ source
>>> + * @src_index:		IRQ source index within the device.
>>> + * @dst_irq:		Destination host IRQ.
>>> + */
>>> +struct ti_sci_intr_irq_desc {
>>> +	u16 src_id;
>>> +	u16 src_index;
>>> +	u16 dst_irq;
>>> +};
>>
>> Oh great. So this is reinventing the GICv3 ITS, only for SPIs. :-(
>>
>> Now, this structure seems completely useless, see below.
>>
>>> +
>>> +static struct irq_chip ti_sci_intr_irq_chip = {
>>> +	.name			= "INTR",
>>> +	.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,
>>> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
>>> +};
>>> +
>>> +/**
>>> + * ti_sci_intr_irq_domain_translate() - Retrieve hwirq and type from
>>> + *					IRQ firmware specific handler.
>>> + * @domain:	Pointer to IRQ domain
>>> + * @fwspec:	Pointer to IRQ specific firmware structure
>>> + * @hwirq:	IRQ number identified by hardware
>>> + * @type:	IRQ type
>>> + *
>>> + * Return 0 if all went ok else appropriate error.
>>> + */
>>> +static int ti_sci_intr_irq_domain_translate(struct irq_domain *domain,
>>> +					    struct irq_fwspec *fwspec,
>>> +					    unsigned long *hwirq,
>>> +					    unsigned int *type)
>>> +{
>>> +	if (is_of_node(fwspec->fwnode)) {
>>> +		if (fwspec->param_count != 3)
>>> +			return -EINVAL;
>>> +
>>> +		*hwirq = ((fwspec->param[0] & TI_SCI_DEV_ID_MASK) <<
>>> +			  TI_SCI_DEV_ID_SHIFT) |
>>> +			 (fwspec->param[1] & TI_SCI_IRQ_ID_MASK);
>>
>> Maybe it would make sense to have a macro that hides this:
> 
> okay.
> 
>>
>>        	       *hwirq = FWSPEC_TO_HWIRQ(fwspec);
>>
>>> +		*type = fwspec->param[2];
>>> +
>>> +		return 0;
>>> +	}
>>> +
>>> +	return -EINVAL;
>>> +}
>>> +
>>> +static inline void ti_sci_intr_delete_desc(struct ti_sci_intr_irq_domain *intr,
>>> +					   struct ti_sci_intr_irq_desc *desc)
>>> +{
>>> +	intr->sci->ops.rm_irq_ops.free_direct_irq(intr->sci, desc->src_id,
>>> +						  desc->src_index,
>>> +						  intr->dst_id, desc->dst_irq);
>>
>> This looks horrible. Why doesn't your firmware interface have a helper
>> functions that hides this? Something like:
>>
>> 	ti_sci_free_direct_irq(intr, src_id, src_index, dst_irq);
>>
>> and you could even add some error checking.
> 
> All existing TISCI users follow the same convention, so I did not bother adding
> any such wrapper. Will update TISCI with these wrappers and see what firmware
> maintainer says.

Frankly, exposing all kind of data structures to the world is a pretty 
poor form of abstraction, which is what the firmware is supposed to 
provide.

I'd strongly suggest that include/linux/soc/ti/ti_sci_protocol.h gets 
cleaned up, and that the whole ti_sci_ops disappears from the that file. 
Nobody outside of the firmware *implementation* needs to know about its, 
and it would be much better served by a set of helpers.

Finally, please make the TISCI interrupt management part of this series, 
so that I can review it as part of the code that uses it.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 2/2] irqchip: ti-sci-intr: Add support for Interrupt Router driver

[Lokesh Vutla]

On Monday 08 October 2018 06:30 PM, Marc Zyngier wrote:
> Hi Lokesh,
> 
> On 08/10/18 10:48, Lokesh Vutla wrote:
>> Hi Marc,
>>
>> On 10/6/2018 3:25 PM, Marc Zyngier wrote:
>>> Hi Lokesh,
>>>
>>> On Sat, 06 Oct 2018 08:28:12 +0100,
>>> Lokesh Vutla <lokeshvutla@ti.com> wrote:
>>>>
>>>> Texas Instruments' K3 generation SoCs has an IP Interrupt Router
>>>> that does allows for multiplexing of input interrupts to host
>>>> interrupt controller. Interrupt Router inputs are either from a
>>>> peripheral or from an Interrupt Aggregator which is another
>>>> interrupt controller.
>>>>
>>>> Configuration of the interrupt router registers can only be done by
>>>> a system co-processor and the driver needs to send a message to this
>>>> co processor over TISCI protocol.
>>>
>>> I assume that this co-processor only deals with the routing itself,
>>> and doesn't need to be talked to during interrupt processing, right?
>>
>> Yes, that's right.
>>
>>>
>>>>
>>>> Add support for Interrupt Router driver over TISCI protocol.
>>>>
>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>>> ---
>>>>    MAINTAINERS                       |   1 +
>>>>    drivers/irqchip/Kconfig           |  11 +
>>>>    drivers/irqchip/Makefile          |   1 +
>>>>    drivers/irqchip/irq-ti-sci-intr.c | 325 ++++++++++++++++++++++++++++++
>>>>    4 files changed, 338 insertions(+)
>>>>    create mode 100644 drivers/irqchip/irq-ti-sci-intr.c
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index a23778b68d74..cf3c834f8cee 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -14626,6 +14626,7 @@ F:	Documentation/devicetree/bindings/clock/ti,sci-clk.txt
>>>>    F:	drivers/clk/keystone/sci-clk.c
>>>>    F:	drivers/reset/reset-ti-sci.c
>>>>    F:	Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
>>>> +F:	drivers/irqchip/irq-ti-sci-intr.c
>>>>    
>>>>    THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER
>>>>    M:	Hans Verkuil <hverkuil@xs4all.nl>
>>>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>>>> index 96451b581452..9a965fe22043 100644
>>>> --- a/drivers/irqchip/Kconfig
>>>> +++ b/drivers/irqchip/Kconfig
>>>> @@ -374,6 +374,17 @@ config QCOM_PDC
>>>>    	  Power Domain Controller driver to manage and configure wakeup
>>>>    	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
>>>>    
>>>> +config TI_SCI_INTR_IRQCHIP
>>>> +	tristate "TISCI based Interrupt Router irqchip driver"
>>>> +	depends on TI_SCI_PROTOCOL && ARCH_K3
>>>> +	select IRQ_DOMAIN
>>>> +	select IRQ_DOMAIN_HIERARCHY
>>>> +	help
>>>> +	  This enables the irqchip driver support for K3 Interrupt router
>>>> +	  over TI System Control Interface available on some new TI's SoCs.
>>>> +	  If you wish to use interrupt router irq resources managed by the
>>>> +	  TI System Controller, say Y here. Otherwise, say N.
>>>
>>> I don't really see the point of making this user-selectable. If you're
>>> compiling support for a given platform, this platform configuration
>>> fragment should itself select the necessary dependencies for the
>>> system to work as expected. Here, you are leaving the choice to the
>>> user, with a 50% chance of getting a system that doesn't boot...
>>
>> There are 2 reasons why I made it tristate:
>> - Not all interrupts go through this irqchip(At least in the AM6 SoC
>> using this). Most of the legacy peripherals still are directly connected
>> to GIC
>> - TI_SCI_PROTOCOL is defined as tristate.
> 
> But as you said, these are "legacy" interrupts, and most of the
> interesting stuff is routed through the system controller. We also try
> not to have core interrupt controllers as modules. As for having the
> firmware interface as a module, I wonder what the use-case is.
> 
>> If you still feel I should not make it user-selectable, I can drop it.
> 
> I really wonder what the added value is for the user.

okay, will not make it use configurable in v2.

> 
>>
>>>
>>>> +
>>>>    endmenu
>>>>    
>>>>    config SIFIVE_PLIC
>>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>>>> index b822199445ff..44bf65606d60 100644
>>>> --- a/drivers/irqchip/Makefile
>>>> +++ b/drivers/irqchip/Makefile
>>>> @@ -89,3 +89,4 @@ obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
>>>>    obj-$(CONFIG_NDS32)			+= irq-ativic32.o
>>>>    obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
>>>>    obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
>>>> +obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
>>>> diff --git a/drivers/irqchip/irq-ti-sci-intr.c b/drivers/irqchip/irq-ti-sci-intr.c
>>>> new file mode 100644
>>>> index 000000000000..f04fe6da1b09
>>>> --- /dev/null
>>>> +++ b/drivers/irqchip/irq-ti-sci-intr.c
>>>> @@ -0,0 +1,325 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Texas Instruments' K3 Interrupt Router irqchip driver
>>>> + *
>>>> + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
>>>> + *	Lokesh Vutla <lokeshvutla@ti.com>
>>>> + */
>>>> +
>>>> +#include <linux/err.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/irqchip.h>
>>>> +#include <linux/of_platform.h>
>>>> +#include <linux/of_address.h>
>>>> +#include <linux/of_irq.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/moduleparam.h>
>>>> +#include <linux/irqdomain.h>
>>>> +#include <linux/soc/ti/ti_sci_protocol.h>
>>>> +
>>>> +#define TI_SCI_DEV_ID_MASK	0xffff
>>>> +#define TI_SCI_DEV_ID_SHIFT	16
>>>> +#define TI_SCI_IRQ_ID_MASK	0xffff
>>>> +#define TI_SCI_IRQ_ID_SHIFT	0
>>>> +#define TI_SCI_IS_EVENT_IRQ	BIT(31)
>>>> +
>>>> +#define HWIRQ_TO_DEVID(HWIRQ)	(((HWIRQ) >> (TI_SCI_DEV_ID_SHIFT)) & \
>>>> +				 (TI_SCI_DEV_ID_MASK))
>>>> +#define HWIRQ_TO_IRQID(HWIRQ)	((HWIRQ) & (TI_SCI_IRQ_ID_MASK))
>>>
>>> nit: s/(HWIRQ)/(hwirq)/g
>>
>> okay.
>>
>>>
>>>> +
>>>> +/**
>>>> + * struct ti_sci_intr_irq_domain - Structure representing a TISCI based
>>>> + *				   Interrupt Router IRQ domain.
>>>> + * @sci:	Pointer to TISCI handle
>>>> + * @dst_irq:	TISCI resource pointer representing destination irq controller.
>>>> + * @dst_id:	TISCI device ID of the destination irq controller.
>>>> + */
>>>> +struct ti_sci_intr_irq_domain {
>>>> +	const struct ti_sci_handle *sci;
>>>> +	struct ti_sci_resource *dst_irq;
>>>> +	u16 dst_id;
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct ti_sci_intr_irq_desc - Description of an Interrupt Router IRQ
>>>> + * @src_id:		TISCI device ID of the IRQ source
>>>> + * @src_index:		IRQ source index within the device.
>>>> + * @dst_irq:		Destination host IRQ.
>>>> + */
>>>> +struct ti_sci_intr_irq_desc {
>>>> +	u16 src_id;
>>>> +	u16 src_index;
>>>> +	u16 dst_irq;
>>>> +};
>>>
>>> Oh great. So this is reinventing the GICv3 ITS, only for SPIs. :-(
>>>
>>> Now, this structure seems completely useless, see below.
>>>
>>>> +
>>>> +static struct irq_chip ti_sci_intr_irq_chip = {
>>>> +	.name			= "INTR",
>>>> +	.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,
>>>> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
>>>> +};
>>>> +
>>>> +/**
>>>> + * ti_sci_intr_irq_domain_translate() - Retrieve hwirq and type from
>>>> + *					IRQ firmware specific handler.
>>>> + * @domain:	Pointer to IRQ domain
>>>> + * @fwspec:	Pointer to IRQ specific firmware structure
>>>> + * @hwirq:	IRQ number identified by hardware
>>>> + * @type:	IRQ type
>>>> + *
>>>> + * Return 0 if all went ok else appropriate error.
>>>> + */
>>>> +static int ti_sci_intr_irq_domain_translate(struct irq_domain *domain,
>>>> +					    struct irq_fwspec *fwspec,
>>>> +					    unsigned long *hwirq,
>>>> +					    unsigned int *type)
>>>> +{
>>>> +	if (is_of_node(fwspec->fwnode)) {
>>>> +		if (fwspec->param_count != 3)
>>>> +			return -EINVAL;
>>>> +
>>>> +		*hwirq = ((fwspec->param[0] & TI_SCI_DEV_ID_MASK) <<
>>>> +			  TI_SCI_DEV_ID_SHIFT) |
>>>> +			 (fwspec->param[1] & TI_SCI_IRQ_ID_MASK);
>>>
>>> Maybe it would make sense to have a macro that hides this:
>>
>> okay.
>>
>>>
>>>         	       *hwirq = FWSPEC_TO_HWIRQ(fwspec);
>>>
>>>> +		*type = fwspec->param[2];
>>>> +
>>>> +		return 0;
>>>> +	}
>>>> +
>>>> +	return -EINVAL;
>>>> +}
>>>> +
>>>> +static inline void ti_sci_intr_delete_desc(struct ti_sci_intr_irq_domain *intr,
>>>> +					   struct ti_sci_intr_irq_desc *desc)
>>>> +{
>>>> +	intr->sci->ops.rm_irq_ops.free_direct_irq(intr->sci, desc->src_id,
>>>> +						  desc->src_index,
>>>> +						  intr->dst_id, desc->dst_irq);
>>>
>>> This looks horrible. Why doesn't your firmware interface have a helper
>>> functions that hides this? Something like:
>>>
>>> 	ti_sci_free_direct_irq(intr, src_id, src_index, dst_irq);
>>>
>>> and you could even add some error checking.
>>
>> All existing TISCI users follow the same convention, so I did not bother adding
>> any such wrapper. Will update TISCI with these wrappers and see what firmware
>> maintainer says.
> 
> Frankly, exposing all kind of data structures to the world is a pretty
> poor form of abstraction, which is what the firmware is supposed to
> provide.
> 
> I'd strongly suggest that include/linux/soc/ti/ti_sci_protocol.h gets
> cleaned up, and that the whole ti_sci_ops disappears from the that file.
> Nobody outside of the firmware *implementation* needs to know about its,
> and it would be much better served by a set of helpers.
> 
> Finally, please make the TISCI interrupt management part of this series,
> so that I can review it as part of the code that uses it.

Sure, my next version will include TISCI interrupt management as well.

Thanks and regards,
Lokesh