diff mbox

[14/17] dt-bindings/interrupt-controller: add description for Marvell SEI node

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

Commit Message

Miquel Raynal April 21, 2018, 1:55 p.m. UTC
Describe the SEI (System Error Interrupt) controller driver. The
controller is part of the GIC. It aggregates two types of interrupts,
wired and MSIs from respectively the AP and the CPs, into a single SPI
interrupt.

Suggested-by: Haim Boot <hayim@marvell.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../bindings/interrupt-controller/marvell,sei.txt  | 54 ++++++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt

Comments

Rob Herring April 27, 2018, 8:50 p.m. UTC | #1
On Sat, Apr 21, 2018 at 03:55:34PM +0200, Miquel Raynal wrote:
> Describe the SEI (System Error Interrupt) controller driver. The
> controller is part of the GIC. It aggregates two types of interrupts,
> wired and MSIs from respectively the AP and the CPs, into a single SPI
> interrupt.
> 
> Suggested-by: Haim Boot <hayim@marvell.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  .../bindings/interrupt-controller/marvell,sei.txt  | 54 ++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt b/Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt
> new file mode 100644
> index 000000000000..a246d59552b1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt
> @@ -0,0 +1,54 @@
> +Marvell SEI (System Error Interrupt) Controller
> +-----------------------------------------------
> +
> +Marvell SEI (System Error Interrupt) controller is an interrupt aggregator.
> +It receives interrupts from several sources and aggregates them to a single
> +interrupt line (an SPI) on the primary interrupt controller.
> +
> +The IRQ chip can handle up to 64 SEIs, a set comes from the AP and is
> +wired while a second set comes from the CPs by the mean of MSIs. Each
> +'domain' is represented as a subnode.
> +
> +Required properties:
> +
> +- compatible: should be "marvell,armada-8k-sei".
> +- reg: SEI registers location and length.
> +- interrupts: identifies the parent IRQ that will be triggered.
> +- #address-cells: should be '1', represents the position of the first
> +                  IRQ of a given type in the SEI range.
> +- #size-cells: should be '1', represents the number of a given type of
> +               IRQs.
> +
> +Child node 'sei-wired-controller' required properties:
> +
> +- reg: the range of wired interrupts.
> +- #interrupt-cells: number of cells to define an SEI wired interrupt
> +                    coming from the AP, should be 1. The cell is the IRQ
> +                    number.
> +- interrupt-controller: identifies the node as an interrupt controller.
> +
> +Child node 'sei-msi-controller' required properties:
> +
> +- reg: the range of non-wired interrupts triggered by way of MSIs.
> +- msi-controller: identifies the node as an MSI controller.
> +
> +Example:
> +
> +        sei: sei@3f0200 {
> +               compatible = "marvell,armada-8k-sei";
> +               reg = <0x3f0200 0x40>;
> +               interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +
> +               sei_wired_controller: sei-wired-controller@0 {
> +                       reg = <0 21>;

Using interrupt numbers in reg is strange.

> +                       #interrupt-cells = <1>;
> +                       interrupt-controller;
> +               };
> +
> +               sei_msi_controller: sei-msi-controller@21 {
> +                       reg = <21 43>;
> +                       msi-controller;

Can't the parent be both an interrupt-controller and msi-controller?

> +               };
> +       };
> -- 
> 2.14.1
>
Miquel Raynal April 28, 2018, 10:48 a.m. UTC | #2
Hi Rob,

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

> On Sat, Apr 21, 2018 at 03:55:34PM +0200, Miquel Raynal wrote:
> > Describe the SEI (System Error Interrupt) controller driver. The
> > controller is part of the GIC. It aggregates two types of interrupts,
> > wired and MSIs from respectively the AP and the CPs, into a single SPI
> > interrupt.
> > 
> > Suggested-by: Haim Boot <hayim@marvell.com>
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  .../bindings/interrupt-controller/marvell,sei.txt  | 54 ++++++++++++++++++++++
> >  1 file changed, 54 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt b/Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt
> > new file mode 100644
> > index 000000000000..a246d59552b1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt
> > @@ -0,0 +1,54 @@
> > +Marvell SEI (System Error Interrupt) Controller
> > +-----------------------------------------------
> > +
> > +Marvell SEI (System Error Interrupt) controller is an interrupt aggregator.
> > +It receives interrupts from several sources and aggregates them to a single
> > +interrupt line (an SPI) on the primary interrupt controller.
> > +
> > +The IRQ chip can handle up to 64 SEIs, a set comes from the AP and is
> > +wired while a second set comes from the CPs by the mean of MSIs. Each
> > +'domain' is represented as a subnode.
> > +
> > +Required properties:
> > +
> > +- compatible: should be "marvell,armada-8k-sei".
> > +- reg: SEI registers location and length.
> > +- interrupts: identifies the parent IRQ that will be triggered.
> > +- #address-cells: should be '1', represents the position of the first
> > +                  IRQ of a given type in the SEI range.
> > +- #size-cells: should be '1', represents the number of a given type of
> > +               IRQs.
> > +
> > +Child node 'sei-wired-controller' required properties:
> > +
> > +- reg: the range of wired interrupts.
> > +- #interrupt-cells: number of cells to define an SEI wired interrupt
> > +                    coming from the AP, should be 1. The cell is the IRQ
> > +                    number.
> > +- interrupt-controller: identifies the node as an interrupt controller.
> > +
> > +Child node 'sei-msi-controller' required properties:
> > +
> > +- reg: the range of non-wired interrupts triggered by way of MSIs.
> > +- msi-controller: identifies the node as an MSI controller.
> > +
> > +Example:
> > +
> > +        sei: sei@3f0200 {
> > +               compatible = "marvell,armada-8k-sei";
> > +               reg = <0x3f0200 0x40>;
> > +               interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
> > +               #address-cells = <1>;
> > +               #size-cells = <1>;
> > +
> > +               sei_wired_controller: sei-wired-controller@0 {
> > +                       reg = <0 21>;  
> 
> Using interrupt numbers in reg is strange.

I thought the reg property was the one to choose here, I can of course
change it, what would you suggest?

> 
> > +                       #interrupt-cells = <1>;
> > +                       interrupt-controller;
> > +               };
> > +
> > +               sei_msi_controller: sei-msi-controller@21 {
> > +                       reg = <21 43>;
> > +                       msi-controller;  
> 
> Can't the parent be both an interrupt-controller and msi-controller?

We need to know which one aggregates interrupts, which one receives
MSIs and most importantly which interrupt is what (within the 64
that are handled by the SEI).

I thought it would describe the hardware pretty well, do you think to
another solution?
 
> 
> > +               };
> > +       };
> > -- 
> > 2.14.1
> >   

Thanks,
Miquèl
Rob Herring April 30, 2018, 2:09 p.m. UTC | #3
On Sat, Apr 28, 2018 at 5:48 AM, Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
> Hi Rob,
>
> On Fri, 27 Apr 2018 15:50:32 -0500, Rob Herring <robh@kernel.org> wrote:
>
>> On Sat, Apr 21, 2018 at 03:55:34PM +0200, Miquel Raynal wrote:
>> > Describe the SEI (System Error Interrupt) controller driver. The
>> > controller is part of the GIC. It aggregates two types of interrupts,
>> > wired and MSIs from respectively the AP and the CPs, into a single SPI
>> > interrupt.
>> >
>> > Suggested-by: Haim Boot <hayim@marvell.com>
>> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>> > ---
>> >  .../bindings/interrupt-controller/marvell,sei.txt  | 54 ++++++++++++++++++++++
>> >  1 file changed, 54 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt b/Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt
>> > new file mode 100644
>> > index 000000000000..a246d59552b1
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt
>> > @@ -0,0 +1,54 @@
>> > +Marvell SEI (System Error Interrupt) Controller
>> > +-----------------------------------------------
>> > +
>> > +Marvell SEI (System Error Interrupt) controller is an interrupt aggregator.
>> > +It receives interrupts from several sources and aggregates them to a single
>> > +interrupt line (an SPI) on the primary interrupt controller.
>> > +
>> > +The IRQ chip can handle up to 64 SEIs, a set comes from the AP and is
>> > +wired while a second set comes from the CPs by the mean of MSIs. Each
>> > +'domain' is represented as a subnode.
>> > +
>> > +Required properties:
>> > +
>> > +- compatible: should be "marvell,armada-8k-sei".
>> > +- reg: SEI registers location and length.
>> > +- interrupts: identifies the parent IRQ that will be triggered.
>> > +- #address-cells: should be '1', represents the position of the first
>> > +                  IRQ of a given type in the SEI range.
>> > +- #size-cells: should be '1', represents the number of a given type of
>> > +               IRQs.
>> > +
>> > +Child node 'sei-wired-controller' required properties:
>> > +
>> > +- reg: the range of wired interrupts.
>> > +- #interrupt-cells: number of cells to define an SEI wired interrupt
>> > +                    coming from the AP, should be 1. The cell is the IRQ
>> > +                    number.
>> > +- interrupt-controller: identifies the node as an interrupt controller.
>> > +
>> > +Child node 'sei-msi-controller' required properties:
>> > +
>> > +- reg: the range of non-wired interrupts triggered by way of MSIs.
>> > +- msi-controller: identifies the node as an MSI controller.
>> > +
>> > +Example:
>> > +
>> > +        sei: sei@3f0200 {
>> > +               compatible = "marvell,armada-8k-sei";
>> > +               reg = <0x3f0200 0x40>;
>> > +               interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
>> > +               #address-cells = <1>;
>> > +               #size-cells = <1>;
>> > +
>> > +               sei_wired_controller: sei-wired-controller@0 {
>> > +                       reg = <0 21>;
>>
>> Using interrupt numbers in reg is strange.
>
> I thought the reg property was the one to choose here, I can of course
> change it, what would you suggest?
>
>>
>> > +                       #interrupt-cells = <1>;
>> > +                       interrupt-controller;
>> > +               };
>> > +
>> > +               sei_msi_controller: sei-msi-controller@21 {
>> > +                       reg = <21 43>;
>> > +                       msi-controller;
>>
>> Can't the parent be both an interrupt-controller and msi-controller?
>
> We need to know which one aggregates interrupts, which one receives
> MSIs and most importantly which interrupt is what (within the 64
> that are handled by the SEI).

You mean that 0-20 are wired and 21-63 are msi? "marvell,msi-base =
<21>;" in the parent would be sufficient for that though you may want
something more flexible to have multiple ranges.

It looks like there may already be similar bindings. See
'msi-available-ranges" and "{al,arm},msi-base-spi".

Rob
Thomas Petazzoni April 30, 2018, 2:24 p.m. UTC | #4
Hello Miquèl,

Title should rather be: "add DT binding documentation for the Marvell
SEI controller" or something like that. Talking about "node" is a bit
weird here.

On Sat, 21 Apr 2018 15:55:34 +0200, Miquel Raynal wrote:
> Describe the SEI (System Error Interrupt) controller driver.

As soon as you say "driver" in a DT binding documentation, you're on
the wrong track. A binding documentation never describes a driver, but
a piece of hardware.

> The controller is part of the GIC.

I don't think we should state that, especially since it's not part of
the GIC shipped by ARM as far as I know, and we represent it as a
separate Device Tree nodes.

> diff --git a/Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt b/Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt
> new file mode 100644
> index 000000000000..a246d59552b1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt
> @@ -0,0 +1,54 @@
> +Marvell SEI (System Error Interrupt) Controller
> +-----------------------------------------------
> +
> +Marvell SEI (System Error Interrupt) controller is an interrupt aggregator.
> +It receives interrupts from several sources and aggregates them to a single
> +interrupt line (an SPI) on the primary interrupt controller.

Rather than primary, I would use "parent" here.

> +The IRQ chip can handle up to 64 SEIs, a set comes from the AP and is

"IRQ chip" is a Linux concept, I would stick to "This interrupt
controller can handle up ..."

> +wired while a second set comes from the CPs by the mean of MSIs. Each
> +'domain' is represented as a subnode.
> +
> +Required properties:
> +
> +- compatible: should be "marvell,armada-8k-sei".
> +- reg: SEI registers location and length.
> +- interrupts: identifies the parent IRQ that will be triggered.
> +- #address-cells: should be '1', represents the position of the first
> +                  IRQ of a given type in the SEI range.
> +- #size-cells: should be '1', represents the number of a given type of
> +               IRQs.

What is the "given type" ?

> +Child node 'sei-wired-controller' required properties:
> +
> +- reg: the range of wired interrupts.
> +- #interrupt-cells: number of cells to define an SEI wired interrupt
> +                    coming from the AP, should be 1. The cell is the IRQ
> +                    number.
> +- interrupt-controller: identifies the node as an interrupt controller.
> +
> +Child node 'sei-msi-controller' required properties:
> +
> +- reg: the range of non-wired interrupts triggered by way of MSIs.
> +- msi-controller: identifies the node as an MSI controller.
> +
> +Example:
> +
> +        sei: sei@3f0200 {
> +               compatible = "marvell,armada-8k-sei";
> +               reg = <0x3f0200 0x40>;
> +               interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +
> +               sei_wired_controller: sei-wired-controller@0 {
> +                       reg = <0 21>;
> +                       #interrupt-cells = <1>;
> +                       interrupt-controller;
> +               };
> +
> +               sei_msi_controller: sei-msi-controller@21 {
> +                       reg = <21 43>;
> +                       msi-controller;
> +               };

As Rob asked, I'm not sure we need subnodes here. Did you check if it
is was doable to have a single node which is both an
interrupt-controller and a msi-controller ?

And indeed, as Rob said, using reg to encode interrupt ranges doesn't
look good.

Best regards,

Thomas
Miquel Raynal May 18, 2018, 2:48 p.m. UTC | #5
Hi Rob,

On Mon, 30 Apr 2018 09:09:10 -0500, Rob Herring <robh@kernel.org> wrote:

> On Sat, Apr 28, 2018 at 5:48 AM, Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> > Hi Rob,
> >
> > On Fri, 27 Apr 2018 15:50:32 -0500, Rob Herring <robh@kernel.org> wrote:
> >  
> >> On Sat, Apr 21, 2018 at 03:55:34PM +0200, Miquel Raynal wrote:  
> >> > Describe the SEI (System Error Interrupt) controller driver. The
> >> > controller is part of the GIC. It aggregates two types of interrupts,
> >> > wired and MSIs from respectively the AP and the CPs, into a single SPI
> >> > interrupt.
> >> >
> >> > Suggested-by: Haim Boot <hayim@marvell.com>
> >> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >> > ---
> >> >  .../bindings/interrupt-controller/marvell,sei.txt  | 54 ++++++++++++++++++++++
> >> >  1 file changed, 54 insertions(+)
> >> >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt b/Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt
> >> > new file mode 100644
> >> > index 000000000000..a246d59552b1
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt
> >> > @@ -0,0 +1,54 @@
> >> > +Marvell SEI (System Error Interrupt) Controller
> >> > +-----------------------------------------------
> >> > +
> >> > +Marvell SEI (System Error Interrupt) controller is an interrupt aggregator.
> >> > +It receives interrupts from several sources and aggregates them to a single
> >> > +interrupt line (an SPI) on the primary interrupt controller.
> >> > +
> >> > +The IRQ chip can handle up to 64 SEIs, a set comes from the AP and is
> >> > +wired while a second set comes from the CPs by the mean of MSIs. Each
> >> > +'domain' is represented as a subnode.
> >> > +
> >> > +Required properties:
> >> > +
> >> > +- compatible: should be "marvell,armada-8k-sei".
> >> > +- reg: SEI registers location and length.
> >> > +- interrupts: identifies the parent IRQ that will be triggered.
> >> > +- #address-cells: should be '1', represents the position of the first
> >> > +                  IRQ of a given type in the SEI range.
> >> > +- #size-cells: should be '1', represents the number of a given type of
> >> > +               IRQs.
> >> > +
> >> > +Child node 'sei-wired-controller' required properties:
> >> > +
> >> > +- reg: the range of wired interrupts.
> >> > +- #interrupt-cells: number of cells to define an SEI wired interrupt
> >> > +                    coming from the AP, should be 1. The cell is the IRQ
> >> > +                    number.
> >> > +- interrupt-controller: identifies the node as an interrupt controller.
> >> > +
> >> > +Child node 'sei-msi-controller' required properties:
> >> > +
> >> > +- reg: the range of non-wired interrupts triggered by way of MSIs.
> >> > +- msi-controller: identifies the node as an MSI controller.
> >> > +
> >> > +Example:
> >> > +
> >> > +        sei: sei@3f0200 {
> >> > +               compatible = "marvell,armada-8k-sei";
> >> > +               reg = <0x3f0200 0x40>;
> >> > +               interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
> >> > +               #address-cells = <1>;
> >> > +               #size-cells = <1>;
> >> > +
> >> > +               sei_wired_controller: sei-wired-controller@0 {
> >> > +                       reg = <0 21>;  
> >>
> >> Using interrupt numbers in reg is strange.  
> >
> > I thought the reg property was the one to choose here, I can of course
> > change it, what would you suggest?
> >  
> >>  
> >> > +                       #interrupt-cells = <1>;
> >> > +                       interrupt-controller;
> >> > +               };
> >> > +
> >> > +               sei_msi_controller: sei-msi-controller@21 {
> >> > +                       reg = <21 43>;
> >> > +                       msi-controller;  
> >>
> >> Can't the parent be both an interrupt-controller and msi-controller?  

I would prefer to describe how the hardware is split in two
sub-controllers. And to answer the question: no, the parent cannot be
both and interrupt-controller and an msi-controller.

> >
> > We need to know which one aggregates interrupts, which one receives
> > MSIs and most importantly which interrupt is what (within the 64
> > that are handled by the SEI).  
> 
> You mean that 0-20 are wired and 21-63 are msi? "marvell,msi-base =
> <21>;" in the parent would be sufficient for that though you may want
> something more flexible to have multiple ranges.

Absolutely, I would like something more flexible.

> 
> It looks like there may already be similar bindings. See
> 'msi-available-ranges" and "{al,arm},msi-base-spi".

If you are ok, I will use a new property named 'marvell,sei-ranges'
instead of the reg property which is indeed more explicit.

Thanks,
Miquèl
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt b/Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt
new file mode 100644
index 000000000000..a246d59552b1
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt
@@ -0,0 +1,54 @@ 
+Marvell SEI (System Error Interrupt) Controller
+-----------------------------------------------
+
+Marvell SEI (System Error Interrupt) controller is an interrupt aggregator.
+It receives interrupts from several sources and aggregates them to a single
+interrupt line (an SPI) on the primary interrupt controller.
+
+The IRQ chip can handle up to 64 SEIs, a set comes from the AP and is
+wired while a second set comes from the CPs by the mean of MSIs. Each
+'domain' is represented as a subnode.
+
+Required properties:
+
+- compatible: should be "marvell,armada-8k-sei".
+- reg: SEI registers location and length.
+- interrupts: identifies the parent IRQ that will be triggered.
+- #address-cells: should be '1', represents the position of the first
+                  IRQ of a given type in the SEI range.
+- #size-cells: should be '1', represents the number of a given type of
+               IRQs.
+
+Child node 'sei-wired-controller' required properties:
+
+- reg: the range of wired interrupts.
+- #interrupt-cells: number of cells to define an SEI wired interrupt
+                    coming from the AP, should be 1. The cell is the IRQ
+                    number.
+- interrupt-controller: identifies the node as an interrupt controller.
+
+Child node 'sei-msi-controller' required properties:
+
+- reg: the range of non-wired interrupts triggered by way of MSIs.
+- msi-controller: identifies the node as an MSI controller.
+
+Example:
+
+        sei: sei@3f0200 {
+               compatible = "marvell,armada-8k-sei";
+               reg = <0x3f0200 0x40>;
+               interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
+               #address-cells = <1>;
+               #size-cells = <1>;
+
+               sei_wired_controller: sei-wired-controller@0 {
+                       reg = <0 21>;
+                       #interrupt-cells = <1>;
+                       interrupt-controller;
+               };
+
+               sei_msi_controller: sei-msi-controller@21 {
+                       reg = <21 43>;
+                       msi-controller;
+               };
+       };