[PATCH v2 3/3] irqchip/irq-pruss-intc: Fix processing of IEP interrupts

Meghana Malladi posted 3 patches 1 month, 1 week ago
[PATCH v2 3/3] irqchip/irq-pruss-intc: Fix processing of IEP interrupts
Posted by Meghana Malladi 1 month, 1 week ago
From: Suman Anna <s-anna@ti.com>

IEP compare/capture events are level-triggered and remain asserted until
the IEP CMP/CAP status register is cleared. The PRUSS INTC acknowledges
this event (via SICR) before the IEP source is actually cleared, leaving the
SECR latch still set. When linux unmasks the interrupt after the threaded
handler completes, the INTC still sees the event as pending, resulting in
an unintended second interrupt.

The solution is to actually ack these IRQs from pruss_intc_irq_unmask()
after the IRQ source is cleared in HW.

The interrupt handling sequence is as follows:
IEP hardware
============
[1] Compare match occurs
[2] IEP sets CMP/CAP status bit = 1
[3] Output level stays HIGH until software clears IEP status

PRUSS INTC
==========
[4] Detects level HIGH → sets SECR[event] = 1
[5] Raises host IRQ to Linux

Linux interrupt flow (oneshot)
==============================
 HARD IRQ:
   [6] pruss_intc_irq_handler()
   [7] mask_ack_irq()
         → writes SICR = event
         → tries to clear SECR
         BUT level still HIGH → INTC still sees it pending

 THREAD HANDLER:
   [8] icss_iep_cap_cmp_handler()
         → clears IEP CMP/CAP status bit
         → IEP output level goes LOW

 IRQ FINALIZATION:
   [9] irq_finalize_oneshot()
  [10] pruss_intc_irq_unmask():
         Without fix:
             - EISR reenables event
             - INTC still thinks event pending (stale SECR)
             → SECOND IRQ (spurious)

         With fix:
             - Write SICR again (now level LOW → INTC clears latch)
             - Then EISR enables event cleanly
             → No spurious IRQ

Fixes: 04e2d1e06978 ("irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts")
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Suman Anna <s-anna@ti.com>
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
Link: https://lore.kernel.org/all/20230919061900.369300-4-danishanwar@ti.com/
Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
Signed-off-by: Meghana Malladi <m-malladi@ti.com>
---

v2-v1:
- Updated the commit message with more details on the fix as per comments
  from Marc Zyngier <maz@kernel.org>

 drivers/irqchip/irq-pruss-intc.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c
index 1fdf110d94ed..17b03e0d012e 100644
--- a/drivers/irqchip/irq-pruss-intc.c
+++ b/drivers/irqchip/irq-pruss-intc.c
@@ -70,6 +70,8 @@
 #define MAX_PRU_SYS_EVENTS 160
 #define MAX_PRU_CHANNELS 20
 
+#define MAX_PRU_INT_EVENTS	64
+
 /**
  * struct pruss_intc_map_record - keeps track of actual mapping state
  * @value: The currently mapped value (channel or host)
@@ -85,10 +87,13 @@ struct pruss_intc_map_record {
  * @num_system_events: number of input system events handled by the PRUSS INTC
  * @num_host_events: number of host events (which is equal to number of
  *		     channels) supported by the PRUSS INTC
+ * @quirky_events: bitmask of events that need quirky IRQ handling (limited to
+ *		   (internal sources only for now, so 64 bits suffice)
  */
 struct pruss_intc_match_data {
 	u8 num_system_events;
 	u8 num_host_events;
+	u64 quirky_events;
 };
 
 /**
@@ -304,6 +309,10 @@ static void pruss_intc_irq_ack(struct irq_data *data)
 	struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
 	unsigned int hwirq = data->hwirq;
 
+	if (hwirq < MAX_PRU_INT_EVENTS &&
+	    intc->soc_config->quirky_events & BIT_ULL(hwirq))
+		return;
+
 	pruss_intc_write_reg(intc, PRU_INTC_SICR, hwirq);
 }
 
@@ -320,6 +329,9 @@ static void pruss_intc_irq_unmask(struct irq_data *data)
 	struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
 	unsigned int hwirq = data->hwirq;
 
+	if (hwirq < MAX_PRU_INT_EVENTS &&
+	    intc->soc_config->quirky_events & BIT_ULL(hwirq))
+		pruss_intc_write_reg(intc, PRU_INTC_SICR, hwirq);
 	pruss_intc_write_reg(intc, PRU_INTC_EISR, hwirq);
 }
 
@@ -641,11 +653,13 @@ static void pruss_intc_remove(struct platform_device *pdev)
 static const struct pruss_intc_match_data pruss_intc_data = {
 	.num_system_events = 64,
 	.num_host_events = 10,
+	.quirky_events = BIT_ULL(7), /* IEP capture/compare event */
 };
 
 static const struct pruss_intc_match_data icssg_intc_data = {
 	.num_system_events = 160,
 	.num_host_events = 20,
+	.quirky_events = BIT_ULL(7) | BIT_ULL(56), /* IEP{0,1} capture/compare events */
 };
 
 static const struct of_device_id pruss_intc_of_match[] = {
-- 
2.43.0

Re: [PATCH v2 3/3] irqchip/irq-pruss-intc: Fix processing of IEP interrupts
Posted by Thomas Gleixner 1 month, 1 week ago
On Wed, Feb 18 2026 at 15:07, Meghana Malladi wrote:
> IEP compare/capture events are level-triggered and remain asserted until
> the IEP CMP/CAP status register is cleared. The PRUSS INTC acknowledges

The INTC does not acknowledge anything. The driver does.

> this event (via SICR) before the IEP source is actually cleared, leaving the
> SECR latch still set. When linux unmasks the interrupt after the threaded
> handler completes, the INTC still sees the event as pending, resulting in
> an unintended second interrupt.
>
> The solution is to actually ack these IRQs from pruss_intc_irq_unmask()

s/IRQ/interrupt/g

> after the IRQ source is cleared in HW.
>
> The interrupt handling sequence is as follows:
> IEP hardware
> ============
> [1] Compare match occurs
> [2] IEP sets CMP/CAP status bit = 1
> [3] Output level stays HIGH until software clears IEP status
>
> PRUSS INTC
> ==========
> [4] Detects level HIGH → sets SECR[event] = 1
> [5] Raises host IRQ to Linux
>
> Linux interrupt flow (oneshot)
> ==============================
>  HARD IRQ:
>    [6] pruss_intc_irq_handler()
>    [7] mask_ack_irq()
>          → writes SICR = event
>          → tries to clear SECR
>          BUT level still HIGH → INTC still sees it pending
>
>  THREAD HANDLER:
>    [8] icss_iep_cap_cmp_handler()
>          → clears IEP CMP/CAP status bit
>          → IEP output level goes LOW
>
>  IRQ FINALIZATION:
>    [9] irq_finalize_oneshot()
>   [10] pruss_intc_irq_unmask():
>          Without fix:
>              - EISR reenables event
>              - INTC still thinks event pending (stale SECR)
>              → SECOND IRQ (spurious)
>
>          With fix:
>              - Write SICR again (now level LOW → INTC clears latch)
>              - Then EISR enables event cleanly
>              → No spurious IRQ

Having the ACK in the unmask is fundamentally wrong and a horrible hack.

The driver uses the wrong handler. That's what handle_fasteoi_irq() is
for.

All you need is to set the IRQCHIP_EOI_THREADED flag on the interrupt
chip and implement irq_eoi() instead of irq_ack().

For the non-threaded cases this spares the mask/unmask dance
completely. For the threaded oneshot case it masks on entry and
irq_finalize_oneshot() does the EOI and the unmask via
unmask_threaded_irq().

No?

Thanks,

        tglx