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

[Marc Zyngier]

On Wed, 02 Mar 2022 13:34:41 +0000,
Shawn Guo <shawn.guo@xxxxxxxxxx> wrote:
> 
> 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?

This code actually makes me ask more questions. Why is it programming
2 'pins' for each IRQ?

> 
> It seems MPM_REG_POLARITY is only meant for level interrupts, since edge
> interrupts already have separate registers for rising and falling.

Then level interrupts must clear both the edge registers at all times.

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

Whatever. It simply cannot be trusted. If you kexec another kernel,
you need to be able to restore a sane state at probe time. This isn't
optional.

	M.

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