Re: [PATCH v7 2/2] irqchip: Add Qualcomm MPM controller driver

[Shawn Guo]

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