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