Re: [PATCH v2 13/16] dt-bindings/interrupt-controller: add documentation for Marvell SEI controller

[Miquel Raynal]

Hi Rob, Marc,

On Tue, 5 Jun 2018 14:51:21 -0600, Rob Herring <robh@xxxxxxxxxx> wrote:

> On Tue, May 22, 2018 at 11:40:39AM +0200, Miquel Raynal wrote:
> > Describe the System Error Interrupt (SEI) controller. 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@xxxxxxxxxxx>
> > Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> > ---
> >  .../bindings/interrupt-controller/marvell,sei.txt  | 50 ++++++++++++++++++++++
> >  1 file changed, 50 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..689981036c30
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt
> > @@ -0,0 +1,50 @@
> > +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 parent interrupt
> > +controller.
> > +
> > +This interrupt controller 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.
> > +
> > +Child node 'sei-wired-controller' required properties:
> > +
> > +- marvell,sei-ranges: ranges 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:
> > +
> > +- marvell,sei-ranges: ranges 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>;
> > +
> > +               sei_wired_controller: sei-wired-controller@0 {
> > +                       marvell,sei-ranges = <0 21>;
> > +                       #interrupt-cells = <1>;
> > +                       interrupt-controller;
> > +               };
> > +
> > +               sei_msi_controller: sei-msi-controller@21 {
> > +                       marvell,sei-ranges = <21 43>;
> > +                       msi-controller;
> > +               };  
> 
> I still think this should just be all one node. There's several examples 
> in the tree of nodes which are both interrupt-controller and 
> msi-controller. Marvell MPIC is one example.

I checked Marvell MPIC example (armada 370 XP), it does not use
hierarchy domains, so I totally understand your point but I'm not sure
how I could get inspired by this driver (I'm looking for others).

Here I'm stuck. I know from a pure DT point of view the following is
not a valid argument. But from Linux, there is no easy way to handle
this situation without two different device nodes due to the internals
of the irqchip subsystem. There is simply no easy solution and having
only one node would require consequent changes in the core.

Maybe Marc will have an idea, but I think we already gave up on this
topic :/

Regards,
MiquÃl