Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Wed, 02 Mar 2022 13:57:27 +0000
Message-ID: <875yow31a0.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Shawn Guo <shawn.guo@linaro.org>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Maulik Shah <quic_mkshah@quicinc.com>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Sudeep Holla <sudeep.holla@arm.com>,
        Rob Herring <robh+dt@kernel.org>, devicetree@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 2/2] irqchip: Add Qualcomm MPM controller driver
In-Reply-To: <20220302133441.GM269879@dragon>
References: <20220301062414.2987591-1-shawn.guo@linaro.org>
        <20220301062414.2987591-3-shawn.guo@linaro.org>
        <87ee3m2aed.wl-maz@kernel.org>
        <20220302084028.GL269879@dragon>
        <877d9c3b2u.wl-maz@kernel.org>
        <20220302133441.GM269879@dragon>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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