After seeing some use, and with more devices tested, the current
implementation for the Realtek SoC interrupt controller was found to
contain a few flaws.

The driver requires the following fixes:
- irq_domain_ops::map should map the virq, not the hwirq (patch 1)
- routing has an off-by-one error. Routing values (1..6) correspond to
  MIPS CAUSEF(2..7) (patch 2)

The following improvements should also be made:
- Use N real cascaded interrupts with an interrupt-specific mask of
  child irq lines. Otherwise a high-priority interrupt may cause a
  low-priority interrupt to be handled first. (patch 3)
- Get rid of assumed routing to parent interrupts of the original
  implementation (patch 4, 5)

Changes since v1:
Link: https://lore.kernel.org/all/cover.1640261161.git.sander@svanheule.net/

Still an RFC. Mainly since I don't like the open coding in the last
patch, but also since I still have a question about the chained IRQ
handlers.

- Split some of the changes to limit the patch scope to one issue.
- Dropped some small (spurious or unneeded) changes
- Instead of dropping/replacing interrupt-map, the last patches now
  provide an implementation that amends the current situtation.

Sander Vanheule (5):
  irqchip/realtek-rtl: map control data to virq
  irqchip/realtek-rtl: fix off-by-one in routing
  irqchip/realtek-rtl: use per-parent irq handling
  dt-bindings: interrupt-controller: realtek,rtl-intc: map output lines
  irqchip/realtek-rtl: add explicit output routing

 .../realtek,rtl-intc.yaml                     |  38 ++-
 drivers/irqchip/irq-realtek-rtl.c             | 232 ++++++++++++++----
 2 files changed, 218 insertions(+), 52 deletions(-)

-- 
2.33.1

Hi,

I don't think the IRQ routing has an off-by one error. This was chosen
by John to correspond to Realtek's own "documentation" and to
take account of the special meaning of IRQs 0, 1 for VSMP and 6 and 7
for the Realtek SoCs. In any case it would break the ABI as the meaning
of these values changes and I don't think the change in range actually
gives any additional functionality.

With regards to the RTL8390, that SoC actually has two IRQ controllers
to allow VSMP. The changes in parent routing have a good chance of breaking
VSMP on the RTL8390 targets. Did you stress test this new logic under VSMP?

Cheers,
   Birger


On 26/12/2021 20:59, Sander Vanheule wrote:
> After seeing some use, and with more devices tested, the current
> implementation for the Realtek SoC interrupt controller was found to
> contain a few flaws.
> 
> The driver requires the following fixes:
> - irq_domain_ops::map should map the virq, not the hwirq (patch 1)
> - routing has an off-by-one error. Routing values (1..6) correspond to
>    MIPS CAUSEF(2..7) (patch 2)
> 
> The following improvements should also be made:
> - Use N real cascaded interrupts with an interrupt-specific mask of
>    child irq lines. Otherwise a high-priority interrupt may cause a
>    low-priority interrupt to be handled first. (patch 3)
> - Get rid of assumed routing to parent interrupts of the original
>    implementation (patch 4, 5)
> 
> Changes since v1:
> Link: https://lore.kernel.org/all/cover.1640261161.git.sander@svanheule.net/
> 
> Still an RFC. Mainly since I don't like the open coding in the last
> patch, but also since I still have a question about the chained IRQ
> handlers.
> 
> - Split some of the changes to limit the patch scope to one issue.
> - Dropped some small (spurious or unneeded) changes
> - Instead of dropping/replacing interrupt-map, the last patches now
>    provide an implementation that amends the current situtation.
> 
> Sander Vanheule (5):
>    irqchip/realtek-rtl: map control data to virq
>    irqchip/realtek-rtl: fix off-by-one in routing
>    irqchip/realtek-rtl: use per-parent irq handling
>    dt-bindings: interrupt-controller: realtek,rtl-intc: map output lines
>    irqchip/realtek-rtl: add explicit output routing
> 
>   .../realtek,rtl-intc.yaml                     |  38 ++-
>   drivers/irqchip/irq-realtek-rtl.c             | 232 ++++++++++++++----
>   2 files changed, 218 insertions(+), 52 deletions(-)
> 

Hi Birger,

On Monday, 27 December 2021, Birger Koblitz wrote:
> Hi,
> 
> I don't think the IRQ routing has an off-by one error. This was chosen
> by John to correspond to Realtek's own "documentation" and to
> take account of the special meaning of IRQs 0, 1 for VSMP and 6 and 7
> for the Realtek SoCs. In any case it would break the ABI as the meaning
> of these values changes and I don't think the change in range actually
> gives any additional functionality.

Realtek's SDK provides routing register values. I would have to check to see what CPU IRQs it then binds to, to service those interrupts. The binding wouldn't have to change, but we could fix the driver and devicetrees.

The binding specifies that interrupt-map provides a mapping of interrupt inputs to parent interrupt. The driver then takes these values, but doesn't check what the parent interrupt controller actually is, and finally assigns a chained handler to a hardware IRQ index (instead of a VIRQ).

You can try limiting the interrupt-map to only the mapping for UART0 with the current driver, and you will find that you end up with a broken system.

CPU IRQs 0 and 1 are indeed special (IPI for VSMP), yet we have interrupt mappings that contain <... &cpuintc 1>. Furthermore, if you specify a mapping of <... &cpuint 6> for an active interrupt source, you will get spurious timer (CPU IRQ 7) interrupts. This can't be correct. 
 
> With regards to the RTL8390, that SoC actually has two IRQ controllers
> to allow VSMP. The changes in parent routing have a good chance of breaking
> VSMP on the RTL8390 targets. Did you stress test this new logic under VSMP?

I haven't tested this with VSMP, because it is out of scope for this series. For the binding, I expect that would only require N register ranges instead of one; one per CPU. I think the driver should then be able to perform the IRQ balancing based on that information alone, given that the parent IRQs are available at each CPU.

Best,
Sander

Hi Sander,
> I haven't tested this with VSMP, because it is out of scope for this series. For the binding, I expect that would only require N register ranges instead of one; one per CPU. I think the driver should then be able to perform the IRQ balancing based on that information alone, given that the parent IRQs are available at each CPU.

whether this is out of the scope of this series is not the point. In my experience you only see issues with locking and race conditions with the IRQ driver if you test with VSMP enabled,
because only with VSMP you can be in the IRQ code multiple times at the same time. Since you want to change routing logic and hierarchies I would believe it to be a very good idea
to test that. The present code passes that test.

Cheers,
   Birger

Hi Birger,

On Tue, 2021-12-28 at 09:09 +0100, Birger Koblitz wrote:
> Hi Sander,
> > I haven't tested this with VSMP, because it is out of scope for this series. For the
> > binding, I expect that would only require N register ranges instead of one; one per
> > CPU. I think the driver should then be able to perform the IRQ balancing based on that
> > information alone, given that the parent IRQs are available at each CPU.
> 
> whether this is out of the scope of this series is not the point. In my experience you
> only see issues with locking and race conditions with the IRQ driver if you test with
> VSMP enabled,
> because only with VSMP you can be in the IRQ code multiple times at the same time. Since
> you want to change routing logic and hierarchies I would believe it to be a very good
> idea
> to test that. The present code passes that test.

Implementing CPU affinity is a separate issue for after these patches IMHO. The current
problems have to be fixed anyway. Otherwise you're just compounding (potential) issues,
only making further development harder.

FWIW, the driver with these (reworked) patches runs fine on my RTL839x (Zyxel GS1900-48)
with SMP enabled. That's without implementing CPU affinity support on this driver, so all
SoC interrupts just go to CPU0. If any issues with lock-ups show up later, we can fix them
when they appear.

Best,
Sander