| Message ID | 8a5931f18a6f1c92f8c8e4965dc65674d7e5a4c4.1640261161.git.sander@svanheule.net |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Rework realtek-rtl IRQ driver | expand |
| Context | Check | Description |
|---|---|---|
| robh/dt-meta-schema | success | |
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 78 lines checked |
| robh/dtbs-check | success |
On Thu, 23 Dec 2021 12:08:33 +0000, Sander Vanheule <sander@svanheule.net> wrote: > > The binding incorrectly specified the "interrupt-map" property should be > used, although the use is non-standard. A quirk had to be introduced in > commit de4adddcbcc2 ("of/irq: Add a quirk for controllers with their own > definition of interrupt-map") to allow the driver to function again. That's too late. We have released a kernel with this binding, and it will live on forever until we totally remove the platform from the tree. DT is an ABI, and only time travel can fix this blunder. M.
On Thu, 2021-12-23 at 18:00 +0000, Marc Zyngier wrote: > On Thu, 23 Dec 2021 12:08:33 +0000, > Sander Vanheule <sander@svanheule.net> wrote: > > > > The binding incorrectly specified the "interrupt-map" property should be > > used, although the use is non-standard. A quirk had to be introduced in > > commit de4adddcbcc2 ("of/irq: Add a quirk for controllers with their own > > definition of interrupt-map") to allow the driver to function again. > > That's too late. We have released a kernel with this binding, and it > will live on forever until we totally remove the platform from the > tree. > > DT is an ABI, and only time travel can fix this blunder. Taking into account your comments on the previous patch, this change wouldn't even be required if I correct the mappings for my devices. But that wouldn't get rid of the assumed mapping between output lines and parent interrupts. To what extent can the binding be updated to get rid of this assumption? Or would that require a completely new binding? Best, Sander
On Thu, 23 Dec 2021 19:29:23 +0000, Sander Vanheule <sander@svanheule.net> wrote: > > On Thu, 2021-12-23 at 18:00 +0000, Marc Zyngier wrote: > > On Thu, 23 Dec 2021 12:08:33 +0000, > > Sander Vanheule <sander@svanheule.net> wrote: > > > > > > The binding incorrectly specified the "interrupt-map" property should be > > > used, although the use is non-standard. A quirk had to be introduced in > > > commit de4adddcbcc2 ("of/irq: Add a quirk for controllers with their own > > > definition of interrupt-map") to allow the driver to function again. > > > > That's too late. We have released a kernel with this binding, and it > > will live on forever until we totally remove the platform from the > > tree. > > > > DT is an ABI, and only time travel can fix this blunder. > > Taking into account your comments on the previous patch, this change > wouldn't even be required if I correct the mappings for my > devices. But that wouldn't get rid of the assumed mapping between > output lines and parent interrupts. A driver can always ignore some information from the DT and do its own thing. No sure if that addresses your problem though. > > To what extent can the binding be updated to get rid of this > assumption? Or would that require a completely new binding? You can only extend a binding in a two-way fashion: old kernel works with new DT, new kernel works old DT. Which means that in practice, you can only *add* information to the DT, and have reasonable defaults in the driver when you don't find it. M.
Hi Marc, On Monday, 27 December 2021, Marc Zyngier wrote: > On Thu, 23 Dec 2021 19:29:23 +0000, > Sander Vanheule <sander@svanheule.net> wrote: > > > > On Thu, 2021-12-23 at 18:00 +0000, Marc Zyngier wrote: > > > On Thu, 23 Dec 2021 12:08:33 +0000, > > > Sander Vanheule <sander@svanheule.net> wrote: > > > > > > > > The binding incorrectly specified the "interrupt-map" property should be > > > > used, although the use is non-standard. A quirk had to be introduced in > > > > commit de4adddcbcc2 ("of/irq: Add a quirk for controllers with their own > > > > definition of interrupt-map") to allow the driver to function again. > > > > > > That's too late. We have released a kernel with this binding, and it > > > will live on forever until we totally remove the platform from the > > > tree. > > > > > > DT is an ABI, and only time travel can fix this blunder. > > > > Taking into account your comments on the previous patch, this change > > wouldn't even be required if I correct the mappings for my > > devices. But that wouldn't get rid of the assumed mapping between > > output lines and parent interrupts. > > A driver can always ignore some information from the DT and do its own > thing. No sure if that addresses your problem though. > > > > > To what extent can the binding be updated to get rid of this > > assumption? Or would that require a completely new binding? > > You can only extend a binding in a two-way fashion: old kernel works > with new DT, new kernel works old DT. Which means that in practice, > you can only *add* information to the DT, and have reasonable defaults > in the driver when you don't find it. Thanks for clarifying. In that case I don't think it is possible to get rid of the output-to-parent assumption entirely, since the driver would always need to accommodate for the original binding, where there is no output mapping specified in the binding. There are no SoC-specific compatibles (where a mapping could be assumed), and I don't know on how many MIPS platforms Realtek has used this interrupt router/controller. I don't have much time anymore today, but I'll break my head over it again tomorrow. Best, Sander
diff --git a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml index 9e76fff20323..4f7f30111a8e 100644 --- a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml +++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml @@ -6,6 +6,11 @@ $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. Most + commonly, the CPU's six hardware interrupts are used as parent interrupts. + maintainers: - Birger Koblitz <mail@birger-koblitz.de> - Bert Vermeulen <bert@biot.com> @@ -15,30 +20,40 @@ properties: compatible: const: realtek,rtl-intc - "#interrupt-cells": - const: 1 - reg: maxItems: 1 - interrupts: - maxItems: 1 + "#interrupt-cells": + const: 1 interrupt-controller: true - "#address-cells": - const: 0 + interrupts: + minItems: 1 + maxItems: 15 + description: + List of interrupts where SoC interrupts inputs can be routed to. Must be + provided in the same order as the output lines. The first interrupt is + thus selected via routing value 0, etc. - interrupt-map: - description: Describes mapping from SoC interrupts to CPU interrupts + realtek,interrupt-routing: + $ref: /schemas/types.yaml#/definitions/uint32-matrix + description: + List of <soc_int irq_idx> pairs, where "soc_int" is the interrupt line + number as provided by this controller. "irq_idx" is the index of the + interrupt in the list as specified the interrupts property. + items: + items: + - description: SoC interrupt index + - description: parent interrupt index required: - compatible - reg - "#interrupt-cells" - interrupt-controller - - "#address-cells" - - interrupt-map + - interrupts + - realtek,interrupt-routing additionalProperties: false @@ -49,9 +64,11 @@ examples: #interrupt-cells = <1>; interrupt-controller; reg = <0x3000 0x20>; - #address-cells = <0>; - interrupt-map = - <31 &cpuintc 2>, - <30 &cpuintc 1>, - <29 &cpuintc 5>; + + interrupt-parent = <&cpu_intc>; + interrupts = <2>, <3>, <4>; + realtek,interrupt-routing = + <31 0>, /* route to cpu_intc interrupt 2 */ + <30 1>, /* route to cpu_intc interrupt 3 */ + <29 2>; /* route to cpu_intc interrupt 4 */ };
The binding incorrectly specified the "interrupt-map" property should be used, although the use is non-standard. A quirk had to be introduced in commit de4adddcbcc2 ("of/irq: Add a quirk for controllers with their own definition of interrupt-map") to allow the driver to function again. Update the binding to require a list of parent interrupts instead, and replace the "interrupt-map" property by the custom "realtek,interrupt-routing" property. Signed-off-by: Sander Vanheule <sander@svanheule.net> --- The registers for this interrupt controller have 4 bits per SoC interrupt. This means that, in theory, 15 output interrupts could be wired up (a value of 0 means 'disconnected'), but we have only ever seen this router being used to map to the six MIPS CPU hardware interrupts. --- .../realtek,rtl-intc.yaml | 49 +++++++++++++------ 1 file changed, 33 insertions(+), 16 deletions(-)