On Wed, Aug 14, 2024 at 05:43:19PM +0100, Conor Dooley wrote:
> On Wed, Aug 14, 2024 at 05:51:33PM +0200, Thomas Gleixner wrote:
> > On Wed, Aug 14 2024 at 14:45, Marek Behún wrote:
> >
> > Cc+ device tree people.
> >
> > > + The code binds this new interrupt domain to the same device-tree node as
> > > + the main interrupt domain. The main interrupt controller has its
> > > + interrupts described by one argument in device-tree
> > > + (#interrupt-cells = <1>), i.e.:
> > > +
> > > + interrupts-extended = <&mpic 8>;
> > > +
> > > + Because of backwards compatibility we cannot change this number of
> > > + arguments, and so the SoC Error interrupts must also be described by
> > > + this one number.
> > > +
> > > + Thus, to describe a SoC Error interrupt, one has to add the an offset
> > > + to the SoC Error interrupt number. Offset 0x400 was chosen because the
> > > + main controller supports at most 1024 interrupts (in theory; in practice
> > > + it seems to be 116 interrupts on all supported platforms). An example of
> > > + describing a SoC Error interrupt is
> > > +
> > > + interrupts-extended = <&mpic 0x404>;
> >
> > This looks like a horrible hack and I don't understand why this can't be
> > a separate interrupt controller, which it is in the hardware. That
> > controller utilizes interrupt 4 from the MPIC.
> >
> > But then my DT foo is limited, so I let the DT folks comment on that.
>
> I'm guessing, not being author and all that, that since the registers
> for this SOC_ERR business are intermingled with those of the existing
> interrupt controller it is not possible to create a standalone node for
> the new controller with it's own reg entry, without having to redo
> the binding etc for the existing controller, ending up with some sort of
> syscon.
> Alternatively, I suppose a child node could work, but it wouldn't be much
> less of a hack than deciding that numbers > 400 are the SOC_ERR ones.
> I see arguments for doing it either way and Marek must have an opinion
> on doing it without a new node, given the comment suggesting that a
> previous revision did have a dedicated node and that approach was
> dropped.
This is exactly the reason.
Pali's original code required creating another interrupt-controller node
(soc_err) within the mpic node:
mpic: interrupt-controller@20a00 {
compatible = "marvell,mpic";
reg = <0x20a00 0x2d0>, <0x21070 0x58>;
#interrupt-cells = <1>;
interrupt-controller;
msi-controller;
interrupts = <GIC_PPI 15 IRQ_TYPE_LEVEL_HIGH>;
soc_err: interrupt-controller@20 {
interrupt-controller;
#interrupt-cells = <1>;
};
};
I decided against it, because to do this correctly in device-tree
bindings, it gets unnecessarily complicated:
- it requires #address-cells and #size-cells within the mpic node
- the `interrupt-controller` property of mpic node can in some
interpretations clash with the `interrupt-controller@20` name of the
child node
- the registers of soc_err and mpic overlap
- the child note should have a compatible string if we want to be proper
I also did consider syscon, but IMO it just adds unnecessary burden to
backwards compatibility of device-trees.
The solution with the 0x400 offset solves the backwards compatiblity
issue.
Marek
