Hi Marc,
Marc Zyngier <marc.zyngier@xxxxxxx> 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?
>
> Or am I reading it the wrong way?
No, that's probably me not understanding correctly the purpose of the
above function.
Thanks,
Miquèl
