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

[Marc Zyngier]

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

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.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.