On Mon, 2021-12-27 at 11:17 +0000, Marc Zyngier wrote:
> On Sun, 26 Dec 2021 19:59:27 +0000,
> Sander Vanheule <sander@xxxxxxxxxxxxx> wrote:
> >
> > Amend the binding to also require a list of parent interrupts, and an
> > optional mask to specify which parent is mapped to which output.
> >
> > Without this information, any driver would have to make an assumption on
> > which parent interrupt is connected to which output.
>
> Why should an endpoint driver care at all?
Interrupt inputs to interrupt outputs are SW configurable, but outputs to parent
interrupts are hard-wired and cannot be modified. "interrupt-map" defines an input to
parent interrupt mapping, so it seems a piece of information is missing. This is currently
provided as an assumption in the driver ("CPU IRQs (2..7) are connected to outputs
(1..6)").
Input-to-output is SW configurable, so that can be put in the driver. Output-to-parent is
hardware configuration,
> >
> > Additionally, extend (or add) the relevant descriptions to more clearly
> > describe the inputs and outputs of this router.
> >
> > Signed-off-by: Sander Vanheule <sander@xxxxxxxxxxxxx>
> > ---
> > Since it does not properly describe the hardware, I would still really
> > rather get rid of "interrupt-map", even though that would mean breaking
> > ABI for this binding. As we've argued before [1], that is our prefered
> > solution, and would enable us to not carry more (hacky) code because of
> > a mistake with the initial submission.
>
> Again, this is too late. Broken bindings live forever.
>
> >
> > Vendors don't ship independent DT blobs for devices with this hardware,
> > so the independent devicetree/kernel upgrades issue is really rather
> > theoretical here. Realtek isn't driving the development of the bindings
> > and associated drivers for this platform. They have their SDK and seem
> > to care very little about proper kernel integration.
>
> Any vendor can do whatever they want. You can do the same thing if you
> really want to.
>
> >
> > Furthermore, there are currently no device descriptions in the kernel
> > using this binding. There are in OpenWrt, but OpenWrt firmware images
> > for this platform always contain both the kernel and the appended DTB,
> > so there's also no breakage to worry about.
>
> That's just one use case. Who knows who is using this stuff in a
> different context? Nobody can tell.
>
> >
> > [1] https://lore.kernel.org/all/9c169aad-3c7b-2ffb-90a2-1ca791a3f411@xxxxxxxxxxx/
> >
> > Differences with v1:
> > - Don't drop the "interrupt-map" property
> > - Add the "realtek,output-valid-mask" property
> > ---
> > .../realtek,rtl-intc.yaml | 38 ++++++++++++++++---
> > 1 file changed, 33 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-
> > intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-
> > intc.yaml
> > index 9e76fff20323..29014673c34e 100644
> > --- a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
> > @@ -6,6 +6,10 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
> >
> > title: Realtek RTL SoC interrupt controller devicetree bindings
> >
> > +description:
> > + Interrupt router for Realtek MIPS SoCs, allowing up to 32 SoC interrupts to
> > + be routed to one of up to 15 parent interrupts, or left disconnected.
> > +
> > maintainers:
> > - Birger Koblitz <mail@xxxxxxxxxxxxxxxxx>
> > - Bert Vermeulen <bert@xxxxxxxx>
> > @@ -22,7 +26,11 @@ properties:
> > maxItems: 1
> >
> > interrupts:
> > - maxItems: 1
> > + minItems: 1
> > + maxItems: 15
> > + description:
> > + List of parent interrupts, in the order that they are connected to this
> > + interrupt router's outputs.
>
> Is that to support multiple SoCs? I'd expect a given SoC to have a
> fixed number of output interrupts.
It is, and they do AFAICT. But all values from 1 to 15 can be written to the routing
registers, so I wanted this definition to be as broad as possible.
The SoCs I'm working with only connect to the six CPU HW interrupts, but I don't know what
the actual limit of this interrupt hardware is, or if the outputs always connect to the
MIPS CPU HW interrupts.
> >
> > interrupt-controller: true
> >
> > @@ -30,7 +38,21 @@ properties:
> > const: 0
> >
> > interrupt-map:
> > - description: Describes mapping from SoC interrupts to CPU interrupts
> > + description:
> > + List of <soc_int parent_phandle parent_args ...> tuples, where "soc_int"
> > + is the interrupt input line number as provided by this controller.
> > + "parent_phandle" and "parent_args" specify which parent interrupt this
> > + line should be routed to. Note that interrupt specifiers should be
> > + identical to the parents specified in the "interrupts" property.
> > +
> > + realtek,output-valid-mask:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description:
> > + Optional bit mask indicating which outputs are connected to the parent
> > + interrupts. The lowest set bit indicates which output line the first
> > + interrupt from "interrupts" is connected to, the second lowest set bit
> > + for the second interrupt, etc. If not provided, parent interrupts will be
> > + assigned sequentially to the outputs.
> >
> > required:
> > - compatible
> > @@ -39,6 +61,7 @@ required:
> > - interrupt-controller
> > - "#address-cells"
> > - interrupt-map
> > + - interrupts
> >
> > additionalProperties: false
> >
> > @@ -49,9 +72,14 @@ examples:
> > #interrupt-cells = <1>;
> > interrupt-controller;
> > reg = <0x3000 0x20>;
> > +
> > + interrupt-parent = <&cpuintc>;
> > + interrupts = <1>, <2>, <5>;
> > + realtek,output-valid-mask = <0x13>;
>
> What additional information does this bring? From the description
> above, this is all SW configurable, so why should this be described in
> the DT?
The hardware I currently have, has a single contiguous range of outputs hard-wired to
parent interrupts, starting at the first output.
I wanted to provide maximum flexibility for output connections, which I think should
support sparse output connections. However, since this would be an optional property, and
is currently not needed for any hardware, I suppose it could be added later when needed.
Adding "interrupts" as a required property is also a no-go, I assume, since that would
invalidate currently-valid DT-s.
> > +
> > #address-cells = <0>;
> > interrupt-map =
> > - <31 &cpuintc 2>,
> > - <30 &cpuintc 1>,
> > - <29 &cpuintc 5>;
> > + <31 &cpuintc 2>, /* connect to cpuintc 2 via output 1 */
> > + <30 &cpuintc 1>, /* connect to cpuintc 1 via output 0 */
> > + <29 &cpuintc 5>; /* connect to cpuintc 5 via output 4 */
> > };
>
> My conclusion here is that, as I stated in my initial review of this
> series, you could completely ignore the 3rd field of the map, and let
> the driver decide on the mapping without any extra information.
>
> We already have plenty of crossbar-type drivers in the tree that can
> mux a number of input to a number of outputs and route them
> accordingly to a set of parent interrupts. None of this requires to be
> described in DT.
I had another look and "fsl,imx-intmux" looks like what I had in mind.
If I understand correctly, changing "#interrupt-cells" (to add the routing info) and the
required properties (to add "interrupts"), would require a new set of compatibles, in
addition to some fall-back behaviour if only the original compatible is provided.
Let me give this another spin, see what I can come up with.
Best,
Sander
