Date:   Wed, 8 Dec 2021 18:04:57 +0800
From:   Shawn Guo <shawn.guo@linaro.org>
To:     Marc Zyngier <maz@kernel.org>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Maulik Shah <quic_mkshah@quicinc.com>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Loic Poulain <loic.poulain@linaro.org>,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 3/3] irqchip: Add Qualcomm MPM controller driver
Message-ID: <20211208100453.GP10105@dragon>
References: <20211202122122.23548-1-shawn.guo@linaro.org>
 <20211202122122.23548-4-shawn.guo@linaro.org>
 <87wnkikqve.wl-maz@kernel.org>
 <20211206131530.GN10105@dragon>
 <87wnkh26ar.wl-maz@kernel.org>
 <20211207094835.GO10105@dragon>
 <87mtlc1zzz.wl-maz@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87mtlc1zzz.wl-maz@kernel.org>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Tue, Dec 07, 2021 at 10:16:32AM +0000, Marc Zyngier wrote:
> On Tue, 07 Dec 2021 09:48:36 +0000,
> Shawn Guo <shawn.guo@linaro.org> wrote:
> > 
> > On Mon, Dec 06, 2021 at 01:48:12PM +0000, Marc Zyngier wrote:
> > > > > > +static int qcom_mpm_enter_sleep(struct qcom_mpm_priv *priv)
> > > > > > +{
> > > > > > +	int i, ret;
> > > > > > +
> > > > > > +	for (i = 0; i < priv->reg_stride; i++)
> > > > > > +		qcom_mpm_write(priv, MPM_REG_STATUS, i, 0);
> > > > > > +
> > > > > > +	/* Notify RPM to write vMPM into HW */
> > > > > 
> > > > > What do you mean by 'into HW'? We just did that, right? or are these
> > > > > registers just fake and most of the stuff is in the RPM?
> > > > 
> > > > I have a note about this in commit log.
> > > > 
> > > > - All the register settings are done by APSS on an internal memory
> > > >   region called vMPM, and RPM will flush them into hardware after it
> > > >   receives a mailbox/IPC notification from APSS.
> > > > 
> > > > So yes, these registers are fake/virtual in memory, and RPM will
> > > > actually flush the values into the MPM hardware block.
> > > 
> > > Then why are you using MMIO accessors all over the place if this is
> > > just RAM? Who *owns* this memory? Is it normal DRAM? Or some flops
> > > exposed by a device? Why isn't the state simply communicated over the
> > > mailbox instead?
> > 
> > It's a piece of internal memory (SRAM) which can be access by AP and
> > RPM.  The communication mechanism is defined by SoC/RPM design, and we
> > can do nothing but following the procedure.
> 
> Then the procedure needs to be documented:

Maulik, I'm trying my best to answer Marc's questions based on my
limited knowledges about the hardware.  Please clarify if there is
anything incorrect.

> - Who owns the memory at any given time?

The memory is owned by APSS when system is awake, and owned by RPM when
APSS gets power collapsed.  RPM is on the always-on domain and will be
managing resources and monitoring wake-up interrupts during sleep, with
the help of MPM.  MPM is not a core/master but a hardware
controller/block.

> - What are the events that trigger a change of ownership?

When APSS is about to get power collapsed, it sends a mailbox
notification to RPM, and RPM will take the ownership.  And when APSS is
woken up by a MPM pin/interrupt, APSS takes back the ownership.

> - What are the messages exchanged between these entities?

The messages exchanged are documented as "vMPM register layout" in the
beginning of the driver.  Basically, they are values of MPM registers
related to wake-up interrupt configurations, which will be directly
dumped into physical MPM registers by RPM, when RPM gets the ownership.
On wake-up, RPM will copy STATUS registers into vMPM memory and return
the ownership back to APSS.

> - What is the synchronisation mechanism between the various processing
>   entities (MPM. RPM, APSS...)?

A hardware IPC/mailbox channel is being used by APSS to tell RPM to take
over the ownership.  On wake-up, the wake-up event itself would be the
synchronisation.

> - Is the per-interrupt tracking a HW requirement or a SW
>   implementation choice? Could this instead be a one-shot operation
>   iterating over the whole state?

I do not quite sure what "per-interrupt tracking" means.  The HW
requirement is just that, when RPM takes the ownership of vMPM memory
region, the memory contains the MPM register values, that RPM can
directly dump into MPM registers.

> All this needs to be explained so that it is maintainable, because I'm
> getting tired of drivers that mimic the QC downstream code without
> justification nor documentation to support the implementation.

I really appreciate all your review comments and questions which are
driving for good and maintainable code!

Shawn