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

[Meghana Malladi]

Hi Marc,

On 9/19/23 13:02, Marc Zyngier wrote:
> On Tue, 19 Sep 2023 07:19:00 +0100,
> MD Danish Anwar <danishanwar@xxxxxx> wrote:
> > From: Suman Anna <s-anna@xxxxxx>
> >
> > It was discovered that IEP capture/compare IRQs (event #7 on all SoCs
> > and event #56 on K3 SoCs) are always triggered twice when PPS is
> > generated and CMP hit event detected by IEP.
> >
> > An example of the problem is:
> >    pruss_intc_irq_handler
> >     generic_handle_irq
> >      handle_level_irq
> >        mask_ack_irq -> IRQ 7 masked and asked in INTC,
> >                        but it's not yet cleared on HW level
> >        handle_irq_event()
> >          <threaded on RT>
> >             icss_iep_cap_cmp_handler() -> IRQ 7 is actually processed in HW
> >          irq_finalize_oneshot()
> >           unmask_irq()
> >             pruss_intc_irq_unmask() -> IRQ 7 status is still observed as set
> >
> > The solution is to actually ack these IRQs from pruss_intc_irq_unmask()
> > after the IRQ source is cleared in HW.
>
> What you don't explain is whether the interrupt is level or edge
> triggered? If it is level, then the "quirk" is that the interrupt
> controller is slow to recognise that the level has changed. If it is
> edge, this is a guaranteed recipe to lose interrupts.

These are level IRQs, but INTC has latency detecting the source 
deassertion, causing the double delivery (The hardware keeps the event 
asserted until cleared in IEP) - I will add more details about this fix 
in the commit message for v2.

> Even if it is level, what happens if you issue a mask/unmask sequence
> outside of the interrupt handling and that such an interrupt becomes
> pending in between? Does this spurious ack have an effect on the now
> pending interrupt?

This ack will not result in any event loss outside IRQ context because,
even if a new event arrives between mask/unmask, it is not lost - as the 
level stays asserted until the driver's IEP handler clears it.

> > No public errata available for this yet.
> >
> > Fixes: 04e2d1e06978 ("irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts")
> >
> > Signed-off-by: Grygorii Strashko <grygorii.strashko@xxxxxx>
>
> Nit: drop the empty line after Fixes:.
>
> > Signed-off-by: Suman Anna <s-anna@xxxxxx>
> > Signed-off-by: MD Danish Anwar <danishanwar@xxxxxx>
> > ---
> >   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 3cf684ede564..9907847dbda8 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;
>
> Why limit this to the first 64 interrupts, while the intc can deal
> with 160? What makes you confident that this is solely limited to this
> particular source? And why this source only?

This quirk only applies to internal IEP-generated events (IEP0 CMP/CAP & 
IEP1 CMP/CAP) and there are no known external events with this behavior. 
If future SoCs need quirks beyond 64, the field can be widened, as there 
is no known case today.


> 	M.