On Wed, Mar 02, 2022 at 10:25:45AM +0000, Marc Zyngier wrote:
> On Wed, 02 Mar 2022 08:40:28 +0000,
> Shawn Guo <shawn.guo@xxxxxxxxxx> wrote:
> >
> > Hi Marc,
> >
> > On Tue, Mar 01, 2022 at 11:13:30AM +0000, Marc Zyngier wrote:
> > > Hi Shawn,
>
> [...]
>
> > >
> > > > +static int qcom_mpm_set_type(struct irq_data *d, unsigned int type)
> > > > +{
> > > > + struct qcom_mpm_priv *priv = d->chip_data;
> > > > + int pin = d->hwirq;
> > > > + unsigned int index = pin / 32;
> > > > + unsigned int shift = pin % 32;
> > > > +
> > > > + switch (type & IRQ_TYPE_SENSE_MASK) {
> > > > + case IRQ_TYPE_EDGE_RISING:
> > > > + mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_RISING),
> > > > + MPM_REG_RISING_EDGE, index, shift);
> > > > + break;
> > > > + case IRQ_TYPE_EDGE_FALLING:
> > > > + mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_FALLING),
> > > > + MPM_REG_FALLING_EDGE, index, shift);
> > > > + break;
> > > > + case IRQ_TYPE_LEVEL_HIGH:
> > > > + mpm_set_type(priv, !!(type & IRQ_TYPE_LEVEL_HIGH),
> > > > + MPM_REG_POLARITY, index, shift);
> > > > + break;
> > > > + }
> > >
> > > All these '!!(type & BLAH)' are totally superfluous, as they all expand
> > > to 'true' by construction.
> >
> > Yes, you are right!
> >
> > > And this leads to a few questions:
> > >
> > > - Shouldn't a rising interrupt clear the falling detection?
> > > - Shouldn't a level-low clear the polarity?
> > > - How do you handle IRQ_TYPE_EDGE_BOTH?
> > > - How is MPM_REG_POLARITY evaluated for edge interrupts (resp the EDGE
> > > registers for level interrupts), as you never seem to be configuring
> > > a type here?
> >
> > Honestly, qcom_mpm_set_type() was mostly taken from downstream without
> > too much thinking.
I have to take this statement back. It seems that the current code has
been diverted from the downstream in a wrong way.
> > I trusted it as a "good" reference as I have no
> > document to verify the code. These questions are great and resulted the
> > code changes are pretty sensible to me.
>
> I don't think these changes are enough. For example, an interrupt
> being switched from level to edge is likely to misbehave (how do you
> distinguish the two?). If that's what the downstream driver does, then
> it is terminally broken.
Could you take a look at downstream code and see if it answers all your
questions?
It seems MPM_REG_POLARITY is only meant for level interrupts, since edge
interrupts already have separate registers for rising and falling.
I will fix my broken code by respecting the downstream logic.
> As I asked before, we need some actual specs, or at least someone to
> paraphrase it for us. There are a number of QC folks on Cc, and I
> expect them to chime in and explain how MPM works here.
>
> >
> > > - What initialises the MPM trigger types at boot time?
> >
> > I dumped the vMPM region and it's all zeros. My understanding is if
> > vMPM needs any sort of initialization, it should be done by RPM firmware
> > before APSS gets booting.
>
> What about kexec? We can't rely on this memory region to always be
> 0-initialised, nor do we know what that means.
We are not relying on it being 0-initialised, but being initialised by
RPM with initial physical MPM register values.
Shawn
[1] https://source.codeaurora.org/quic/la/kernel/msm-5.4/tree/drivers/irqchip/qcom-mpm.c/?h=LE.UM.6.2.4.r1#n187
