diff mbox

[13/17] dt-bindings/interrupt-controller: update Marvell ICU bindings

Message ID 20180421135537.24716-14-miquel.raynal@bootlin.com
State New, archived
Headers show

Commit Message

Miquel Raynal April 21, 2018, 1:55 p.m. UTC
Change the documentation to reflect the new bindings used for Marvell
ICU. This involves describing each interrupt group as a subnode of the
ICU node. Each of them having their own compatible.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../bindings/interrupt-controller/marvell,icu.txt  | 60 ++++++++++++++++------
 1 file changed, 43 insertions(+), 17 deletions(-)

Comments

Rob Herring April 27, 2018, 8:47 p.m. UTC | #1
On Sat, Apr 21, 2018 at 03:55:33PM +0200, Miquel Raynal wrote:
> Change the documentation to reflect the new bindings used for Marvell
> ICU. This involves describing each interrupt group as a subnode of the
> ICU node. Each of them having their own compatible.

Why?

This breaks compatibility.

> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  .../bindings/interrupt-controller/marvell,icu.txt  | 60 ++++++++++++++++------
>  1 file changed, 43 insertions(+), 17 deletions(-)
Miquel Raynal April 28, 2018, 10:42 a.m. UTC | #2
Hi Rob,

On Fri, 27 Apr 2018 15:47:23 -0500, Rob Herring <robh@kernel.org> wrote:

> On Sat, Apr 21, 2018 at 03:55:33PM +0200, Miquel Raynal wrote:
> > Change the documentation to reflect the new bindings used for Marvell
> > ICU. This involves describing each interrupt group as a subnode of the
> > ICU node. Each of them having their own compatible.  
> 
> Why?
> 
> This breaks compatibility.

It does indeed.

The problem comes from the inability to handle multiple MSI parents
for one device and the fact that the API was not designed in this way
at all. I would not risk myself to explain the MSI arcana but I suggest
you to have a look at this thread which led to this series:

https://www.spinics.net/lists/arm-kernel/msg645115.html

Of course I am open to suggestions.

Thanks,
Miquèl
 
> 
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  .../bindings/interrupt-controller/marvell,icu.txt  | 60 ++++++++++++++++------
> >  1 file changed, 43 insertions(+), 17 deletions(-)
Thomas Petazzoni April 28, 2018, 10:50 a.m. UTC | #3
Hello,

On Sat, 28 Apr 2018 12:42:04 +0200, Miquel Raynal wrote:

> On Fri, 27 Apr 2018 15:47:23 -0500, Rob Herring <robh@kernel.org> wrote:
> 
> > On Sat, Apr 21, 2018 at 03:55:33PM +0200, Miquel Raynal wrote:  
> > > Change the documentation to reflect the new bindings used for Marvell
> > > ICU. This involves describing each interrupt group as a subnode of the
> > > ICU node. Each of them having their own compatible.    
> > 
> > Why?
> > 
> > This breaks compatibility.  
> 
> It does indeed.

Well, yes and no: your driver supports both the old and new binding, so
the compatibility is not broken. So, yes the binding is changed in an
incompatible way, but the driver continues to support the old binding.

Perhaps the binding documentation should continue to document the
legacy binding, mark it as still supported but deprecated.

Best regards,

Thomas
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt b/Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt
index 649b7ec9d9b1..b856e91105e4 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt
@@ -5,6 +5,8 @@  The Marvell ICU (Interrupt Consolidation Unit) controller is
 responsible for collecting all wired-interrupt sources in the CP and
 communicating them to the GIC in the AP, the unit translates interrupt
 requests on input wires to MSG memory mapped transactions to the GIC.
+These messages will access a different GIC memory area depending on
+their type (NSR, SR, SEI, REI, etc).
 
 Required properties:
 
@@ -12,20 +14,19 @@  Required properties:
 
 - reg: Should contain ICU registers location and length.
 
+Subnodes: Each group of interrupt is declared as a subnode of the ICU,
+with their own compatible.
+
+Required properties for the icu_nsr/icu_sei subnodes:
+
+- compatible: Should be "marvell,cp110-icu-nsr" or "marvell,cp110-icu-sei".
+
 - #interrupt-cells: Specifies the number of cells needed to encode an
-  interrupt source. The value shall be 3.
+  interrupt source. The value shall be 2.
 
-  The 1st cell is the group type of the ICU interrupt. Possible group
-  types are:
+  The 1st cell is the index of the interrupt in the ICU unit.
 
-   ICU_GRP_NSR (0x0) : Shared peripheral interrupt, non-secure
-   ICU_GRP_SR  (0x1) : Shared peripheral interrupt, secure
-   ICU_GRP_SEI (0x4) : System error interrupt
-   ICU_GRP_REI (0x5) : RAM error interrupt
-
-  The 2nd cell is the index of the interrupt in the ICU unit.
-
-  The 3rd cell is the type of the interrupt. See arm,gic.txt for
+  The 2nd cell is the type of the interrupt. See arm,gic.txt for
   details.
 
 - interrupt-controller: Identifies the node as an interrupt
@@ -35,17 +36,42 @@  Required properties:
   that allows to trigger interrupts using MSG memory mapped
   transactions.
 
+Note: each 'interrupts' property referring to any 'icu_xxx' node shall
+      have a different number within [0:206].
+
 Example:
 
 icu: interrupt-controller@1e0000 {
 	compatible = "marvell,cp110-icu";
 	reg = <0x1e0000 0x440>;
-	#interrupt-cells = <3>;
-	interrupt-controller;
-	msi-parent = <&gicp>;
+
+	CP110_LABEL(icu_nsr): icu-nsr {
+		compatible = "marvell,cp110-icu-nsr";
+		#interrupt-cells = <2>;
+		interrupt-controller;
+		msi-parent = <&gicp>;
+	};
+
+        CP110_LABEL(icu_sei): icu-sei {
+                compatible = "marvell,cp110-icu-sei";
+                #interrupt-cells = <2>;
+                interrupt-controller;
+                msi-parent = <&sei>;
+        };
+};
+
+node1 {
+	interrupt-parent = <&icu_nsr>;
+	interrupts = <106 IRQ_TYPE_LEVEL_HIGH>;
+};
+
+node2 {
+	interrupt-parent = <&icu_sei>;
+	interrupts = <107 IRQ_TYPE_LEVEL_HIGH>;
 };
 
-usb3h0: usb3@500000 {
-	interrupt-parent = <&icu>;
-	interrupts = <ICU_GRP_NSR 106 IRQ_TYPE_LEVEL_HIGH>;
+/* Would not work with the above nodes */
+node3 {
+	interrupt-parent = <&icu_nsr>;
+	interrupts = <107 IRQ_TYPE_LEVEL_HIGH>;
 };