Re: [PATCH v4 09/14] irqchip/irq-mvebu-icu: add support for System Error Interrupts (SEI)

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Marc,

Marc Zyngier <marc.zyngier@arm.com> wrote on Tue, 21 Aug 2018 11:37:47
+0100:

> On 21/08/18 11:28, Miquel Raynal wrote:
> > Hi Marc,
> > 
> > Marc Zyngier <marc.zyngier@arm.com> wrote on Tue, 21 Aug 2018 10:19:04
> > +0100:
> >   
> >> On 21/08/18 10:08, Miquel Raynal wrote:  
> >>> Hi Marc,
> >>>
> >>> I'm fine with the rest of the comments, please find just one last
> >>> question below.
> >>>
> >>> [...]
> >>>     
> >>>>> @@ -133,12 +164,36 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
> >>>>>  		return -EINVAL;
> >>>>>  	}
> >>>>>  
> >>>>> -	/* Mask the type to prevent wrong DT configuration */
> >>>>> -	*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> >>>>> +	/*
> >>>>> +	 * The ICU receives level-interrupts. MSI SEI are
> >>>>> +	 * edge-interrupts while MSI NSR are level-interrupts. Update the type
> >>>>> +	 * accordingly for the parent irqchip.
> >>>>> +	 */
> >>>>> +	if (msi_data->subset_data->icu_group == ICU_GRP_SEI)
> >>>>> +		*type = IRQ_TYPE_EDGE_RISING;      
> >>>>
> >>>> That's interesting. How is the resampling done here?    
> >>>
> >>> I'm not sure to understand the question. What does 'resampling' means
> >>> in such context? MSI SEIs are of type "edge" and use the traditional
> >>> MSI signalling infrastructure. I'm asking to be sure not to ignore
> >>> something wrong in my code.    
> >>
> >> You seems to be turning a level interrupt into an edge.  
> > 
> > If is an SEI interrupt, it cannot be a level interrupt and the type
> > will be IRQ_TYPE_EDGE_RISING.
> >   
> >> You can do that,
> >> but only if you resample the level on EOI (and regenerate the
> >> corresponding edge if the level is still high).  
> > 
> > What is before is a '& IRQ_TYPE_SENSE_MASK' operation. In theory,
> > *type could be anything of IRQ_TYPE_{EDGE,LEVEL}_* at this moment. But,
> > as stated above, it cannot be anything else than IRQ_TYPE_EDGE_RISING.
> > I thought more clear to enforce it but if this unclear and useless,
> > let's drop it?  
> 
> I think overriding the trigger that way is very confusing. Either it
> comes from DT, or it comes from the MSI framwork. In both cases, it has
> to be correct, and overriding it is just papering over other bugs.
> 
> I'd rather you put a WARN_ON if you encounter an illegal interrupt trigger.

Actually I do remember now why I enforced the type:

Let's say my thermal IP in a CP110 triggers an interrupt. This
interrupt is of type LEVEL_HIGH, and it is declared in the DT as:

    thermal: thermal-sensor@... {
        [...]
        interrupts-extended = <&icu_sei 116 IRQ_TYPE_LEVEL_HIGH>;
    };

The ICU callback ->translate() will be called with fwspec being the C
view of the above "interrupts-extended" property, so the interrupt type
will be LEVEL_HIGH.

However, the "icu_sei" parent is the SEI IP in the AP806 and this IP
only receives edge interrupts. What happens is that the SEI's
->set_type() callback is called with LEVEL_HIGH type (while it
only supports EDGE_RISING interrupts) and I want the driver to throw an
error in this case.

My way of handling this was to force *type to be EDGE_RISING in the ICU
->translate() callback whenever an SEI was triggered. As this seems not
to be correct, how would you handle such situation?

Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel