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.
