| Message ID | 2235a7748b8f7689a96b1e0f91461e36a946a4ef.1640548009.git.sander@svanheule.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Rework realtek-rtl IRQ driver | expand |
On Sun, 26 Dec 2021 19:59:25 +0000, Sander Vanheule <sander@svanheule.net> wrote: > > There is an offset between routing values (1..6) and the connected MIPS > CPU interrupts (2..7), but no distinction was made between these two > values. > > This issue was previously hidden during testing, because an interrupt > mapping was used where for each required interrupt another (unused) > routing was configured, with an offset of +1. Where does this 'other routing' come from? > > Offset the CPU IRQ numbers by -1 to retrieve the correct routing value. > > Fixes: 9f3a0f34b84a ("irqchip: Add support for Realtek RTL838x/RTL839x interrupt controller") > Signed-off-by: Sander Vanheule <sander@svanheule.net> > --- > drivers/irqchip/irq-realtek-rtl.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c > index d6788dd93c7b..568614edd88f 100644 > --- a/drivers/irqchip/irq-realtek-rtl.c > +++ b/drivers/irqchip/irq-realtek-rtl.c > @@ -95,7 +95,8 @@ static void realtek_irq_dispatch(struct irq_desc *desc) > * SoC interrupts are cascaded to MIPS CPU interrupts according to the > * interrupt-map in the device tree. Each SoC interrupt gets 4 bits for > * the CPU interrupt in an Interrupt Routing Register. Max 32 SoC interrupts > - * thus go into 4 IRRs. > + * thus go into 4 IRRs. A routing value of '0' means the interrupt is left > + * disconnected. Routing values {1..15} connect to output lines {0..14}. > */ > static int __init map_interrupts(struct device_node *node, struct irq_domain *domain) > { > @@ -134,7 +135,7 @@ static int __init map_interrupts(struct device_node *node, struct irq_domain *do > of_node_put(cpu_ictl); > > cpu_int = be32_to_cpup(imap + 2); > - if (cpu_int > 7) > + if (cpu_int > 7 || cpu_int < 2) How many output lines do you have? The comment above says something about having 15 output lines, but you limit it to 7... > return -EINVAL; > > if (!(mips_irqs_set & BIT(cpu_int))) { > @@ -143,7 +144,8 @@ static int __init map_interrupts(struct device_node *node, struct irq_domain *do > mips_irqs_set |= BIT(cpu_int); > } > > - regs[(soc_int * 4) / 32] |= cpu_int << (soc_int * 4) % 32; > + /* Use routing values (1..6) for CPU interrupts (2..7) */ > + regs[(soc_int * 4) / 32] |= (cpu_int - 1) << (soc_int * 4) % 32; > imap += 3; > } > What I don't understand is how this worked so far if all mappings were off my one. Or the mapping really doesn't matter, because this is all under SW control? M.
Hi Marc, On Mon, 2021-12-27 at 10:16 +0000, Marc Zyngier wrote: > On Sun, 26 Dec 2021 19:59:25 +0000, > Sander Vanheule <sander@svanheule.net> wrote: > > > > There is an offset between routing values (1..6) and the connected MIPS > > CPU interrupts (2..7), but no distinction was made between these two > > values. > > > > This issue was previously hidden during testing, because an interrupt > > mapping was used where for each required interrupt another (unused) > > routing was configured, with an offset of +1. > > Where does this 'other routing' come from? When I reported the interrupt-map issue with this binding/driver, I tried to reduce the mapping/routing to something as small as possible, but I couldn't get away with anything less than the following: interrupt-map = <31 &cpuintc 2>, /* UART0 */ <20 &cpuintc 3>, /* SWCORE */ <19 &cpuintc 4>, /* WDT IP1 */ <18 &cpuintc 5>; /* WDT IP2 */ The UART0 and WDT_IP1 mappings were required. These would cause the driver to assign chained handlers to <&cpuint 2> and <&cpuint 4>, and also write values "2" and "4" to the respective routing registers. The SWCORE mapping was not required for any configured features, but it was required to get the console to work. This is because a routing register value of "2", actually causes that interrupt input to be (electrically) cascaded into to <&cpuintc 3>. But <&cpuintc 3> would only be assigned a chained handler because of the SWCORE mapping. By then assigning the same handler to all parent interrupts, and not caring about which parent interrupt caused the handler to be called, this ended up actually working. > > > > Offset the CPU IRQ numbers by -1 to retrieve the correct routing value. > > > > Fixes: 9f3a0f34b84a ("irqchip: Add support for Realtek RTL838x/RTL839x interrupt > > controller") > > Signed-off-by: Sander Vanheule <sander@svanheule.net> > > --- > > drivers/irqchip/irq-realtek-rtl.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c > > index d6788dd93c7b..568614edd88f 100644 > > --- a/drivers/irqchip/irq-realtek-rtl.c > > +++ b/drivers/irqchip/irq-realtek-rtl.c > > @@ -95,7 +95,8 @@ static void realtek_irq_dispatch(struct irq_desc *desc) > > * SoC interrupts are cascaded to MIPS CPU interrupts according to the > > * interrupt-map in the device tree. Each SoC interrupt gets 4 bits for > > * the CPU interrupt in an Interrupt Routing Register. Max 32 SoC interrupts > > - * thus go into 4 IRRs. > > + * thus go into 4 IRRs. A routing value of '0' means the interrupt is left > > + * disconnected. Routing values {1..15} connect to output lines {0..14}. > > */ > > static int __init map_interrupts(struct device_node *node, struct irq_domain *domain) > > { > > @@ -134,7 +135,7 @@ static int __init map_interrupts(struct device_node *node, struct > > irq_domain *do > > of_node_put(cpu_ictl); > > > > cpu_int = be32_to_cpup(imap + 2); > > - if (cpu_int > 7) > > + if (cpu_int > 7 || cpu_int < 2) > > How many output lines do you have? The comment above says something > about having 15 output lines, but you limit it to 7... The SoCs we are using with this interrupt controller, connect their output lines to CPU IRQ 2..7. If "interrupt-map" specifies parent HW IRQ numbers, the driver needs to verify those numbers are valid. If they are, it can derive the routing register values from that. On the other hand, routing register values have four bits. "0" appears to disconnect an input interrupt, so that leaves 15 potential interrupt outputs (in theory, we don't have actual hardware descriptions). Only 6 outputs are used here, but that could be an implementation detail for these SoCs, rather than a limitation of the interrupt router. > > return -EINVAL; > > > > if (!(mips_irqs_set & BIT(cpu_int))) { > > @@ -143,7 +144,8 @@ static int __init map_interrupts(struct device_node *node, struct > > irq_domain *do > > mips_irqs_set |= BIT(cpu_int); > > } > > > > - regs[(soc_int * 4) / 32] |= cpu_int << (soc_int * 4) % 32; > > + /* Use routing values (1..6) for CPU interrupts (2..7) */ > > + regs[(soc_int * 4) / 32] |= (cpu_int - 1) << (soc_int * 4) % 32; > > imap += 3; > > } > > > > What I don't understand is how this worked so far if all mappings were > off my one. Or the mapping really doesn't matter, because this is all > under SW control? The reason this worked, was due to a number of issues compensating for each other, like I tried to explain above. The mapping is indeed arbitrary, and not designed into the hardware. So it could (should?) just be put in the driver, instead of the devicetree. Best, Sander
On Tue, 2021-12-28 at 10:59 +0000, Marc Zyngier wrote: > On Tue, 28 Dec 2021 10:13:26 +0000, > Sander Vanheule <sander@svanheule.net> wrote: > > > > Hi Marc, > > > > On Mon, 2021-12-27 at 10:16 +0000, Marc Zyngier wrote: > > > On Sun, 26 Dec 2021 19:59:25 +0000, > > > Sander Vanheule <sander@svanheule.net> wrote: > > > > > > > > There is an offset between routing values (1..6) and the connected MIPS > > > > CPU interrupts (2..7), but no distinction was made between these two > > > > values. > > > > > > > > This issue was previously hidden during testing, because an interrupt > > > > mapping was used where for each required interrupt another (unused) > > > > routing was configured, with an offset of +1. > > > > > > Where does this 'other routing' come from? > > > > When I reported the interrupt-map issue with this binding/driver, I > > tried to reduce the mapping/routing to something as small as > > possible, but I couldn't get away with anything less than the > > following: > > > > interrupt-map = > > <31 &cpuintc 2>, /* UART0 */ > > <20 &cpuintc 3>, /* SWCORE */ > > <19 &cpuintc 4>, /* WDT IP1 */ > > <18 &cpuintc 5>; /* WDT IP2 */ > > > > The UART0 and WDT_IP1 mappings were required. These would cause the > > driver to assign chained handlers to <&cpuint 2> and <&cpuint 4>, > > and also write values "2" and "4" to the respective routing > > registers. > > > > The SWCORE mapping was not required for any configured features, but > > it was required to get the console to work. This is because a > > routing register value of "2", actually causes that interrupt input > > to be (electrically) cascaded into to <&cpuintc 3>. But <&cpuintc 3> > > would only be assigned a chained handler because of the SWCORE > > mapping. > > > > By then assigning the same handler to all parent interrupts, and not > > caring about which parent interrupt caused the handler to be called, > > this ended up actually working. > > Urgh... Pure luck, then. > > > > > > > > > > > Offset the CPU IRQ numbers by -1 to retrieve the correct routing value. > > > > > > > > Fixes: 9f3a0f34b84a ("irqchip: Add support for Realtek RTL838x/RTL839x interrupt > > > > controller") > > > > Signed-off-by: Sander Vanheule <sander@svanheule.net> > > > > --- > > > > drivers/irqchip/irq-realtek-rtl.c | 8 +++++--- > > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c > > > > index d6788dd93c7b..568614edd88f 100644 > > > > --- a/drivers/irqchip/irq-realtek-rtl.c > > > > +++ b/drivers/irqchip/irq-realtek-rtl.c > > > > @@ -95,7 +95,8 @@ static void realtek_irq_dispatch(struct irq_desc *desc) > > > > * SoC interrupts are cascaded to MIPS CPU interrupts according to the > > > > * interrupt-map in the device tree. Each SoC interrupt gets 4 bits for > > > > * the CPU interrupt in an Interrupt Routing Register. Max 32 SoC interrupts > > > > - * thus go into 4 IRRs. > > > > + * thus go into 4 IRRs. A routing value of '0' means the interrupt is left > > > > + * disconnected. Routing values {1..15} connect to output lines {0..14}. > > > > */ > > > > static int __init map_interrupts(struct device_node *node, struct irq_domain > > > > *domain) > > > > { > > > > @@ -134,7 +135,7 @@ static int __init map_interrupts(struct device_node *node, > > > > struct > > > > irq_domain *do > > > > of_node_put(cpu_ictl); > > > > > > > > cpu_int = be32_to_cpup(imap + 2); > > > > - if (cpu_int > 7) > > > > + if (cpu_int > 7 || cpu_int < 2) > > > > > > How many output lines do you have? The comment above says something > > > about having 15 output lines, but you limit it to 7... > > > > The SoCs we are using with this interrupt controller, connect their > > output lines to CPU IRQ 2..7. If "interrupt-map" specifies parent HW > > IRQ numbers, the driver needs to verify those numbers are valid. If > > they are, it can derive the routing register values from that. > > > > On the other hand, routing register values have four bits. "0" > > appears to disconnect an input interrupt, so that leaves 15 > > potential interrupt outputs (in theory, we don't have actual > > hardware descriptions). Only 6 outputs are used here, but that could > > be an implementation detail for these SoCs, rather than a limitation > > of the interrupt router. > > It would be good to find out if there are more CPU interrupts that can > be targeted. My impression is that this is a copy/paste effect from > the original BSP, and that nobody actually checked. But maybe that's > the wrong impression. The BSP doesn't seems to refer to anything but the 6 CPU HW IRQs, so it appears to be a standard mti,cpu-interrupt-controller. Those only have 6 HW (2-7) and 2 SW (0-1) IRQs. > > > > > > return -EINVAL; > > > > > > > > if (!(mips_irqs_set & BIT(cpu_int))) { > > > > @@ -143,7 +144,8 @@ static int __init map_interrupts(struct device_node *node, > > > > struct > > > > irq_domain *do > > > > mips_irqs_set |= BIT(cpu_int); > > > > } > > > > > > > > - regs[(soc_int * 4) / 32] |= cpu_int << (soc_int * 4) % 32; > > > > + /* Use routing values (1..6) for CPU interrupts (2..7) */ > > > > + regs[(soc_int * 4) / 32] |= (cpu_int - 1) << (soc_int * 4) % 32; > > > > imap += 3; > > > > } > > > > > > > > > > What I don't understand is how this worked so far if all mappings were > > > off my one. Or the mapping really doesn't matter, because this is all > > > under SW control? > > > > The reason this worked, was due to a number of issues compensating > > for each other, like I tried to explain above. > > > > The mapping is indeed arbitrary, and not designed into the > > hardware. So it could (should?) just be put in the driver, instead > > of the devicetree. > > Indeed. Which is what I was advocating for the first place. What I > understand from the HW is that it is able to freely route any input to > any output (muxing them as required), and to map each output to any > CPU interrupt line. Indeed input-to-output is runtime configurable, but output-to-parent is hard-wired AFAICT. If the output-to-parent mapping is known, an input-to-parent mapping can be used (i.e. "interrupt-map") to select the outputs. > If my understanding is correct, the only thing you need from the DT is > a description of which input an endpoint is targeting (the interrupt > specifier), and a list of CPU interrupt lines. Everything else can be > decided at runtime. I think this hardware is similar to 'fsl,imx-intmux' (irq-imx-intmux.c). See also my comments on the bindings patch. > Anyway, I'll probably end-up queuing the first two patches for 5.17, > and a Cc: to stable. The rest we can discuss ad-nauseam. Would you like me to submit the first two separately or will you take them as they are from this series? Best, Sander
diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c index d6788dd93c7b..568614edd88f 100644 --- a/drivers/irqchip/irq-realtek-rtl.c +++ b/drivers/irqchip/irq-realtek-rtl.c @@ -95,7 +95,8 @@ static void realtek_irq_dispatch(struct irq_desc *desc) * SoC interrupts are cascaded to MIPS CPU interrupts according to the * interrupt-map in the device tree. Each SoC interrupt gets 4 bits for * the CPU interrupt in an Interrupt Routing Register. Max 32 SoC interrupts - * thus go into 4 IRRs. + * thus go into 4 IRRs. A routing value of '0' means the interrupt is left + * disconnected. Routing values {1..15} connect to output lines {0..14}. */ static int __init map_interrupts(struct device_node *node, struct irq_domain *domain) { @@ -134,7 +135,7 @@ static int __init map_interrupts(struct device_node *node, struct irq_domain *do of_node_put(cpu_ictl); cpu_int = be32_to_cpup(imap + 2); - if (cpu_int > 7) + if (cpu_int > 7 || cpu_int < 2) return -EINVAL; if (!(mips_irqs_set & BIT(cpu_int))) { @@ -143,7 +144,8 @@ static int __init map_interrupts(struct device_node *node, struct irq_domain *do mips_irqs_set |= BIT(cpu_int); } - regs[(soc_int * 4) / 32] |= cpu_int << (soc_int * 4) % 32; + /* Use routing values (1..6) for CPU interrupts (2..7) */ + regs[(soc_int * 4) / 32] |= (cpu_int - 1) << (soc_int * 4) % 32; imap += 3; }
There is an offset between routing values (1..6) and the connected MIPS CPU interrupts (2..7), but no distinction was made between these two values. This issue was previously hidden during testing, because an interrupt mapping was used where for each required interrupt another (unused) routing was configured, with an offset of +1. Offset the CPU IRQ numbers by -1 to retrieve the correct routing value. Fixes: 9f3a0f34b84a ("irqchip: Add support for Realtek RTL838x/RTL839x interrupt controller") Signed-off-by: Sander Vanheule <sander@svanheule.net> --- drivers/irqchip/irq-realtek-rtl.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)