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

[Miquel Raynal]

Hi Rob, Marc,

On Fri, 8 Jun 2018 16:46:23 +0200, Miquel Raynal
<miquel.raynal@xxxxxxxxxxx> wrote:

> 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 :/

I double checked the MPIC driver and I still don't understand how we
can allocate both MSIs and wired interrupts with the current driver.

Anyway, I finally managed to merge 'sei_wired_controller' and
'sei_msi_controller' and have only one 'sei' node.

I had to do not use the fwnode of the 'sei' node in at least one IRQ
domain (I choose the wired one) and instead implement for that domain
the ->match() hook. I hope this is the right way to do it.

Please have a look at the v3 coming soon.

Thanks,
MiquÃl