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

[Meghana Malladi <m-malladi@ti.com>]

Hi Marc,

On 9/19/23 13:02, Marc Zyngier wrote:
> On Tue, 19 Sep 2023 07:19:00 +0100,
> MD Danish Anwar <danishanwar@ti.com> wrote:
>>
>> From: Suman Anna <s-anna@ti.com>
>>
>> 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@ti.com>
> 
> Nit: drop the empty line after Fixes:.
> 
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> ---
>>   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.
>