[v4,10/14] dt-bindings/interrupt-controller: update Marvell ICU bindings

Message ID 20180705124011.7661-11-miquel.raynal@bootlin.com
State Under Review
Headers show
Series
  • Add System Error Interrupt support to Armada SoCs
Related show

Commit Message

Miquel Raynal July 5, 2018, 12:40 p.m.
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.

The DT binding documentation still documents the legacy binding, where
there was a single node with no subnode.

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

Comments

Rob Herring July 16, 2018, 3:27 p.m. | #1
On Thu, Jul 05, 2018 at 02:40:07PM +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.
> 
> The DT binding documentation still documents the legacy binding, where
> there was a single node with no subnode.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  .../bindings/interrupt-controller/marvell,icu.txt  | 83 ++++++++++++++++++----
>  1 file changed, 71 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt b/Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt
> index 649b7ec9d9b1..83b4fbf8af65 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".
> +

I raised this before and still don't understand. You had 4 types before 
and now you only have 2 types? How do you handle SR and REI with the new 
binding?

Rob

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Miquel Raynal July 16, 2018, 4:39 p.m. | #2
Hi Rob,

Rob Herring <robh@kernel.org> wrote on Mon, 16 Jul 2018 09:27:34 -0600:

> On Thu, Jul 05, 2018 at 02:40:07PM +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.
> > 
> > The DT binding documentation still documents the legacy binding, where
> > there was a single node with no subnode.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  .../bindings/interrupt-controller/marvell,icu.txt  | 83 ++++++++++++++++++----
> >  1 file changed, 71 insertions(+), 12 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt b/Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt
> > index 649b7ec9d9b1..83b4fbf8af65 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".
> > +  
> 
> I raised this before and still don't understand. You had 4 types before 
> and now you only have 2 types? How do you handle SR and REI with the new 
> binding?

Indeed there are 4 types: NSR, SR, SEI, REI.

Until now only NSR were supported.

I'm adding SEI support.

In the future, people might want to add support for SR/REI as well but
they have never been supported in mainline. When support for these
interrupts will have been contributed, I suppose it will easy to add
two other compatibles with the same formatting "marvell,cp110-icu-xxx"?

I hope the changes in the driver will make such contribution almost
trivial.

Thanks,
Miquèl
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring July 16, 2018, 5:44 p.m. | #3
On Mon, Jul 16, 2018 at 06:39:35PM +0200, Miquel Raynal wrote:
> Hi Rob,
> 
> Rob Herring <robh@kernel.org> wrote on Mon, 16 Jul 2018 09:27:34 -0600:
> 
> > On Thu, Jul 05, 2018 at 02:40:07PM +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.
> > > 
> > > The DT binding documentation still documents the legacy binding, where
> > > there was a single node with no subnode.
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  .../bindings/interrupt-controller/marvell,icu.txt  | 83 ++++++++++++++++++----
> > >  1 file changed, 71 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt b/Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt
> > > index 649b7ec9d9b1..83b4fbf8af65 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".
> > > +  
> > 
> > I raised this before and still don't understand. You had 4 types before 
> > and now you only have 2 types? How do you handle SR and REI with the new 
> > binding?
> 
> Indeed there are 4 types: NSR, SR, SEI, REI.
> 
> Until now only NSR were supported.

All 4 were supported by the binding and now only 2 though.

> I'm adding SEI support.
> 
> In the future, people might want to add support for SR/REI as well but
> they have never been supported in mainline. When support for these
> interrupts will have been contributed, I suppose it will easy to add
> two other compatibles with the same formatting "marvell,cp110-icu-xxx"?

Perhaps, but I have know way to tell.

> I hope the changes in the driver will make such contribution almost
> trivial.

You can add bindings without adding driver.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Petazzoni July 16, 2018, 7:30 p.m. | #4
Hello,

On Mon, 16 Jul 2018 11:44:02 -0600, Rob Herring wrote:

> > > I raised this before and still don't understand. You had 4 types before 
> > > and now you only have 2 types? How do you handle SR and REI with the new 
> > > binding?  
> > 
> > Indeed there are 4 types: NSR, SR, SEI, REI.
> > 
> > Until now only NSR were supported.  
> 
> All 4 were supported by the binding and now only 2 though.

In fact, no, the old binding was not working for anything but NSR. It
pretended to support SEI, REI and SR, but because these had never been
implemented, the binding was not sufficient to over them.

To support SEI or REI, we need to point to a different msi-parent than
the one used for NSR, which wasn't possible with the previous binding.

So there is no regression of functionality by this binding change,
quite the contrary, as it really allows to add SEI support, which was
not possible before. You can consider that the previous binding was
buggy and dysfunctional.

Best regards,

Thomas Petazzoni
Thomas Petazzoni July 16, 2018, 7:38 p.m. | #5
Hello,

On Mon, 16 Jul 2018 09:27:34 -0600, Rob Herring wrote:

> > +Required properties for the icu_nsr/icu_sei subnodes:
> > +
> > +- compatible: Should be "marvell,cp110-icu-nsr" or "marvell,cp110-icu-sei".
> > +  
> 
> I raised this before and still don't understand. You had 4 types before 
> and now you only have 2 types? How do you handle SR and REI with the new 
> binding?

As I replied to a different e-mail, the current binding pretended to
support SR, SEI and REI, but it definitely could not.

The ICU collects wired interrupts from the CP, and forwards them to the
AP as MSIs. And contrary to what we thought when designing the current
binding, the target of the MSIs is different depending on the type of
interrupt. NSRs go to the GICP controller in the AP, SEIs go to a
special SEI controller in the AP, REIs go to a special REI controller
in the AP.

In order to represent that in the Device Tree, you need to have a
different msi-parent property for each of NSR, SEI and REI. This is not
something that the current broken binding allows to express, and it's
the reason why we're migrating to this new binding.

So even if the current binding documents that it supports NSR, SEI, REI
and SR, it definitely cannot support anything but NSR. Therefore, the
introduction of the new binding by Miquèl does not introduce any
regression in functionality: it simply allows to really support SEIs.

Does this explanation clarify the situation, and what we're trying to
achieve ?

Thanks!

Thomas

Patch

diff --git a/Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt b/Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt
index 649b7ec9d9b1..83b4fbf8af65 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,75 @@  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>;
+
+	CP110_LABEL(icu_nsr): interrupt-controller@10 {
+		compatible = "marvell,cp110-icu-nsr";
+		reg = <0x10 0x20>;
+		#interrupt-cells = <2>;
+		interrupt-controller;
+		msi-parent = <&gicp>;
+	};
+
+	CP110_LABEL(icu_sei): interrupt-controller@50 {
+		compatible = "marvell,cp110-icu-sei";
+		reg = <0x50 0x10>;
+		#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>;
+};
+
+/* Would not work with the above nodes */
+node3 {
+	interrupt-parent = <&icu_nsr>;
+	interrupts = <107 IRQ_TYPE_LEVEL_HIGH>;
+};
+
+Note on legacy bindings:
+Before using a subnode for each domain, only NSR were
+supported. Bindings were different in this way:
+
+- #interrupt-cells: The value was 3.
+	The 1st cell was the group type of the ICU interrupt. Possible
+	group types were:
+	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 was the index of the interrupt in the ICU unit.
+	The 3rd cell was the type of the interrupt. See arm,gic.txt for
+	details.
+
+Example:
+
+icu: interrupt-controller@1e0000 {
+	compatible = "marvell,cp110-icu";
+	reg = <0x1e0000 0x440>;
+
 	#interrupt-cells = <3>;
 	interrupt-controller;
 	msi-parent = <&gicp>;
 };
 
-usb3h0: usb3@500000 {
+node1 {
 	interrupt-parent = <&icu>;
 	interrupts = <ICU_GRP_NSR 106 IRQ_TYPE_LEVEL_HIGH>;
 };