Hi Marc,

Marc Zyngier [off-list ref] wrote on Tue, 21 Aug 2018 11:37:47
+0100:

On 21/08/18 11:28, Miquel Raynal wrote:

quoted

Hi Marc,

Marc Zyngier [off-list ref] wrote on Tue, 21 Aug 2018 10:19:04
+0100:
  

quoted

On 21/08/18 10:08, Miquel Raynal wrote:  

quoted

Hi Marc,

I'm fine with the rest of the comments, please find just one last
question below.

[...]
    

quoted

quoted

@@ -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.
  

quoted

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 at ... {
        [...]
        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