| Message ID | 20180421135537.24716-14-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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(-)
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(-)
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 --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>; };
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(-)