Message-ID: <87pmqjm1c8.wl-maz@kernel.org>
Date:   Mon, 29 Nov 2021 15:24:39 +0000
From:   Marc Zyngier <maz@...nel.org>
To:     Shawn Guo <shawn.guo@...aro.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Loic Poulain <loic.poulain@...aro.org>,
        devicetree@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver

On Mon, 29 Nov 2021 13:33:12 +0000,
Shawn Guo <shawn.guo@...aro.org> wrote:
> 
> On Fri, Nov 26, 2021 at 07:19:07PM +0000, Marc Zyngier wrote:
> > [resending after having sorted my email config...]
> > 
> > Hi Shawn,
> 
> Hi Marc,
> 
> Thanks for all these review comments!
> 
> > 
> > On Fri, 26 Nov 2021 09:35:29 +0000,
> > Shawn Guo <shawn.guo@...aro.org> wrote:
> > > 
> > > Qualcomm SoCs based on the RPM architecture have a MSM Power Manager (MPM)
> > > in always-on domain. In addition to managing resources during sleep, the
> > > hardware also has an interrupt controller that monitors the interrupts
> > > when the system is asleep, wakes up the APSS when one of these interrupts
> > > occur and replays it to GIC after it becomes operational.
> > > 
> > > It adds an irqchip driver for this interrupt controller, and here are
> > > some notes about it.
> > > 
> > > - For given SoC, a fixed number of MPM pins are supported, e.g. 96 pins
> > >   on QCM2290.  Each of these MPM pins can be either a MPM_GIC pin or
> > >   a MPM_GPIO pin. The mapping between MPM_GIC pin and GIC interrupt
> > >   is defined by SoC, as well as the mapping between MPM_GPIO pin and
> > >   GPIO number.  The former mapping can be found as the SoC data in this
> > >   MPM driver, while the latter can be found as the msm_gpio_wakeirq_map[]
> > >   in TLMM driver.
> > > 
> > > - Two irq domains are created for a single irq_chip to handle MPM_GIC
> > >   and MPM_GPIO pins respectively, i.e. MPM_GIC domain and MPM_GPIO domain.
> > >   The former is a child domain of GIC irq domain, while the latter is
> > >   a parent domain of TLMM/GPIO irq domain.
> > > 
> > > - All the register settings are done by APSS on the an internal memory
> > >   region called vMPM, and RPM will flush them into hardware after it
> > >   receives a mailbox/IPC notification from APSS.
> > > 
> > > - When SoC gets awake from sleep mode, the driver will receive an
> > >   interrupt from RPM, so that it can replay interrupt for particular
> > >   polarity.
> > > 
> > > Signed-off-by: Shawn Guo <shawn.guo@...aro.org>
> > > ---
> > >  drivers/irqchip/Kconfig    |   8 +
> > >  drivers/irqchip/Makefile   |   1 +
> > >  drivers/irqchip/qcom-mpm.c | 487 +++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 496 insertions(+)
> > >  create mode 100644 drivers/irqchip/qcom-mpm.c
> > > 
> > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > > index 7038957f4a77..e126b19190ef 100644
> > > --- a/drivers/irqchip/Kconfig
> > > +++ b/drivers/irqchip/Kconfig
> > > @@ -430,6 +430,14 @@ config QCOM_PDC
> > >  	  Power Domain Controller driver to manage and configure wakeup
> > >  	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
> > >  
> > > +config QCOM_MPM
> > > +	bool "QCOM MPM"
> > 
> > Can't be built as a module?
> 
> The driver is implemented as a builtin_platform_driver().

This, on its own, shouldn't preclude the driver from being built as a
module. However, the config option only allows it to be built in. Why?

[...]

> > > +static inline void
> > > +qcom_mpm_write(struct qcom_mpm_priv *priv, unsigned int reg,
> > > +	       unsigned int index, u32 val)
> > > +{
> > > +	unsigned int offset = (reg * priv->reg_stride + index + 2) * 4;
> > > +	u32 r_val;
> > > +
> > > +	writel(val, priv->base + offset);
> > > +
> > > +	do {
> > > +		r_val = readl(priv->base + offset);
> > > +		udelay(5);
> > > +	} while (r_val != val);
> > 
> > What? Is this waiting for a bit to clear? Why isn't this one of the
> > read*_poll_timeout*() function instead? Surely you can't wait forever
> > here.
> 
> This is taken from downstream, and it seems to double check the written
> value by reading it back.  But to be honest, I'm not really this is
> necessary.  I will do some testing with the read-back check dropped.

How about asking for specs instead? There are QC people on Cc, and
many more reading the list. Hopefully they can explain what this is
all about.

> > 
> > > +}
> > > +
> > > +static inline void qcom_mpm_enable_irq(struct irq_data *d, bool en)
> > > +{
> > > +	struct qcom_mpm_priv *priv = d->domain->host_data;
> > 
> > This really should be stored in d->chip_data.
> 
> OK.
> 
> > 
> > > +	int pin = d->hwirq;
> > > +	unsigned int index = pin / 32;
> > > +	unsigned int shift = pin % 32;
> > > +	unsigned long flags;
> > > +	u32 val;
> > > +
> > > +	priv->pin_to_irq[pin] = d->irq;
> > 
> > This makes no sense. This is only reinventing the very notion of an
> > irq domain, which is to lookup the Linux interrupt based on a context
> > and a signal number.
> 
> The uniqueness of this driver is that it has two irq domains.  This
> little lookup table is created to avoid resolving mapping on each of
> these domains, which is less efficient.  But you think this is really
> nonsense, I can change.

"efficient"? You are taking an interrupt to... err... reinject some
other interrupts. I'm sorry, but the efficiency argument sailed once
someone built this wonderful piece of HW. The first mistake was to
involve SW here, so let's not optimise for something that really
doesn't need it.

Furthermore, why would you look up anywhere other than the wake-up
domain? My impression was that only these interrupts would require
being re-triggered.

[...]

> > > +static inline void
> > > +mpm_set_type(struct qcom_mpm_priv *priv, bool set, unsigned int reg,
> > > +	     unsigned int index, unsigned int shift)
> > > +{
> > > +	u32 val;
> > > +
> > > +	val = qcom_mpm_read(priv, reg, index);
> > > +	if (set)
> > > +		val |= 1 << shift;
> > > +	else
> > > +		val &= ~(1 << shift);
> > > +	qcom_mpm_write(priv, reg, index, val);
> > > +}
> > > +
> > > +static int qcom_mpm_set_type(struct irq_data *d, unsigned int type)
> > > +{
> > > +	struct qcom_mpm_priv *priv = d->domain->host_data;
> > > +	int pin = d->hwirq;
> > > +	unsigned int index = pin / 32;
> > > +	unsigned int shift = pin % 32;
> > > +	unsigned long flags;
> > > +
> > > +	spin_lock_irqsave(&priv->lock, flags);
> > > +
> > > +	mpm_set_type(priv, (type & IRQ_TYPE_EDGE_RISING) ? 1 : 0,
> > 
> > You have a bool type as the second parameter. Why the convoluted ?:
> > operator?
> 
> Will change to !!(type & IRQ_TYPE_EDGE_RISING).
> 
> > 
> > > +		     MPM_REG_RISING_EDGE, index, shift);
> > > +	mpm_set_type(priv, (type & IRQ_TYPE_EDGE_FALLING) ? 1 : 0,
> > > +		     MPM_REG_FALLING_EDGE, index, shift);
> > > +	mpm_set_type(priv, (type & IRQ_TYPE_LEVEL_HIGH) ? 1 : 0,
> > > +		     MPM_REG_POLARITY, index, shift);
> > 
> > Why does this mean for an edge interrupt?
> 
> Edge polarity is configured above on MPM_REG_RISING_EDGE and
> MPM_REG_FALLING_EDGE.  So I guess MPM_REG_POLARITY doesn't have an
> impact on edge interrupt.  I do not have any document or information
> other than downstream code to be sure though.

A well formed 'type' will have that bit clear when any of the EDGE
flags is set. So this will always be 0. It would also be much better
if you expressed the whole thing as a switch/case.

[...]

> > > +static int qcom_mpm_probe(struct platform_device *pdev)
> > > +{
> > > +	struct irq_domain *parent_domain, *mpm_gic_domain, *mpm_gpio_domain;
> > > +	struct device *dev = &pdev->dev;
> > > +	struct device_node *np = dev->of_node;
> > > +	struct device_node *parent = of_irq_find_parent(np);
> > > +	struct qcom_mpm_priv *priv;
> > > +	unsigned int pin_num;
> > > +	int irq;
> > > +	int ret;
> > > +
> > > +	/* See comments in platform_irqchip_probe() */
> > > +	if (parent && !irq_find_matching_host(parent, DOMAIN_BUS_ANY))
> > > +		return -EPROBE_DEFER;
> > 
> > So why aren't you using that infrastructure?
> 
> Because I need to populate .pm of platform_driver and use match data to
> pass mpm_data.

Then I'd rather you improve the existing infrastructure to pass the
bit of extra data you need, instead than reinventing your own.

[...]

> > > +	priv->mbox_client.dev = dev;
> > > +	priv->mbox_chan = mbox_request_channel(&priv->mbox_client, 0);
> > > +	if (IS_ERR(priv->mbox_chan)) {
> > > +		ret = PTR_ERR(priv->mbox_chan);
> > > +		dev_err(dev, "failed to acquire IPC channel: %d\n", ret);
> > > +		goto remove_gpio_domain;
> > 
> > Why don't you request this first, before all the allocations?
> 
> Then I will need to call mbox_free_channel() for any allocation failures
> afterward.

Which would be fine. Checking for dependencies before allocating
resources is good practice, specially when this can result in a probe
deferral.

Thanks,

	M.

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

Powered by blists - more mailing lists