[v2,2/2] irqchip: Add Qualcomm MPM controller driver

Commit Message

Shawn Guo Nov. 26, 2021, 9:35 a.m. UTC

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 <[e-mail]>
---
 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

Comments

Marc Zyngier Nov. 26, 2021, 3:13 p.m. UTC | #1

Hi Shawn,

On Fri, 26 Nov 2021 09:35:29 +0000,
Shawn Guo <[e-mail]> 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 <[e-mail]>
> ---
>  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?

> +	depends on ARCH_QCOM
> +	select IRQ_DOMAIN_HIERARCHY
> +	help
> +	  MSM Power Manager driver to manage and configure wakeup
> +	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
> +
>  config CSKY_MPINTC
>  	bool
>  	depends on CSKY
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index c1f611cbfbf8..0e2e10467e28 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -94,6 +94,7 @@ obj-$(CONFIG_MESON_IRQ_GPIO)		+= irq-meson-gpio.o
>  obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
>  obj-$(CONFIG_NDS32)			+= irq-ativic32.o
>  obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
> +obj-$(CONFIG_QCOM_MPM)			+= qcom-mpm.o
>  obj-$(CONFIG_CSKY_MPINTC)		+= irq-csky-mpintc.o
>  obj-$(CONFIG_CSKY_APB_INTC)		+= irq-csky-apb-intc.o
>  obj-$(CONFIG_RISCV_INTC)		+= irq-riscv-intc.o
> diff --git a/drivers/irqchip/qcom-mpm.c b/drivers/irqchip/qcom-mpm.c
> new file mode 100644
> index 000000000000..1880c734155f
> --- /dev/null
> +++ b/drivers/irqchip/qcom-mpm.c
> @@ -0,0 +1,487 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2021, Linaro Limited
> + * Copyright (c) 2010-2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqchip/arm-gic-v3.h>
> +#include <linux/irqdomain.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +/*
> + * vMPM register layout:
> + *
> + *    31                              0
> + *    +--------------------------------+
> + *    |            TIMER0              | 0x00
> + *    +--------------------------------+
> + *    |            TIMER1              | 0x04
> + *    +--------------------------------+
> + *    |            ENABLE0             | 0x08
> + *    +--------------------------------+
> + *    |              ...               | ...
> + *    +--------------------------------+
> + *    |            ENABLEn             |
> + *    +--------------------------------+
> + *    |          FALLING_EDGE0         |
> + *    +--------------------------------+
> + *    |              ...               |
> + *    +--------------------------------+
> + *    |            STATUSn             |
> + *    +--------------------------------+
> + *
> + * n = DIV_ROUND_UP(pin_num, 32)
> + *
> + */
> +#define MPM_REG_ENABLE		0
> +#define MPM_REG_FALLING_EDGE	1
> +#define MPM_REG_RISING_EDGE	2
> +#define MPM_REG_POLARITY	3
> +#define MPM_REG_STATUS		4
> +
> +#define MPM_NO_PARENT_IRQ	~0UL
> +
> +/* MPM pin and its GIC hwirq */
> +struct mpm_pin {
> +	int pin;
> +	irq_hw_number_t hwirq;
> +};
> +
> +struct mpm_data {
> +	unsigned int pin_num;
> +	const struct mpm_pin *gic_pins;
> +};
> +
> +struct qcom_mpm_priv {
> +	void __iomem *base;
> +	spinlock_t lock;
> +	struct mbox_client mbox_client;
> +	struct mbox_chan *mbox_chan;
> +	const struct mpm_data *data;
> +	unsigned int reg_stride;
> +
> +	/* MPM pin to Linux irq number */
> +	unsigned int *pin_to_irq;
> +};
> +
> +static inline u32
> +qcom_mpm_read(struct qcom_mpm_priv *priv, unsigned int reg, unsigned int index)
> +{
> +	unsigned int offset = (reg * priv->reg_stride + index + 2) * 4;
> +
> +	return readl(priv->base + offset);

Why the non-relaxed accessors?

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

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

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

> +
> +	spin_lock_irqsave(&priv->lock, flags);

This must be a raw spinlock.

> +
> +	val = qcom_mpm_read(priv, MPM_REG_ENABLE, index);
> +	if (en)
> +		val |= 1 << shift;
> +	else
> +		val &= ~(1 << shift);

Use BIT(shift).

> +	qcom_mpm_write(priv, MPM_REG_ENABLE, index, val);
> +
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +}
> +
> +static void qcom_mpm_mask(struct irq_data *d)
> +{
> +	qcom_mpm_enable_irq(d, false);
> +
> +	if (d->parent_data)
> +		irq_chip_mask_parent(d);
> +}
> +
> +static void qcom_mpm_unmask(struct irq_data *d)
> +{
> +	qcom_mpm_enable_irq(d, true);
> +
> +	if (d->parent_data)
> +		irq_chip_unmask_parent(d);
> +}
> +
> +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?

> +		     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?

> +
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	if (!d->parent_data)
> +		return 0;
> +
> +	if (type & IRQ_TYPE_EDGE_BOTH)
> +		type = IRQ_TYPE_EDGE_RISING;
> +
> +	if (type & IRQ_TYPE_LEVEL_MASK)
> +		type = IRQ_TYPE_LEVEL_HIGH;
> +
> +	return irq_chip_set_type_parent(d, type);
> +}
> +
> +static struct irq_chip qcom_mpm_chip = {
> +	.name = "mpm",
> +	.irq_eoi = irq_chip_eoi_parent,
> +	.irq_mask = qcom_mpm_mask,
> +	.irq_disable = qcom_mpm_mask,

No. If disable is really mask, then you only need mask.

> +	.irq_unmask = qcom_mpm_unmask,
> +	.irq_retrigger = irq_chip_retrigger_hierarchy,
> +	.irq_set_type = qcom_mpm_set_type,
> +	.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
> +	.irq_set_affinity = irq_chip_set_affinity_parent,

nit: please align the members vertically.

> +};
> +
> +static irq_hw_number_t get_parent_hwirq(struct qcom_mpm_priv *priv, int pin)
> +{
> +	const struct mpm_pin *gic_pins = priv->data->gic_pins;
> +	int i;
> +
> +	for (i = 0; gic_pins[i].pin >= 0; i++) {
> +		int p = gic_pins[i].pin;
> +
> +		if (p < 0)
> +			break;
> +
> +		if (p == pin)
> +			return gic_pins[i].hwirq;
> +	}
> +
> +	return MPM_NO_PARENT_IRQ;
> +}
> +
> +static int
> +qcom_mpm_translate(struct irq_domain *domain, struct irq_fwspec *fwspec,
> +		   unsigned long *hwirq, unsigned int *type)
> +{
> +	if (is_of_node(fwspec->fwnode)) {
> +		if (fwspec->param_count != 2)
> +			return -EINVAL;
> +
> +		*hwirq = fwspec->param[0];
> +		*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}

This is a copy of irq_domain_translate_twocell().

> +
> +static int qcom_mpm_gic_alloc(struct irq_domain *domain, unsigned int virq,
> +			      unsigned int nr_irqs, void *data)
> +{
> +	struct qcom_mpm_priv *priv = domain->host_data;
> +	struct irq_fwspec *fwspec = data;
> +	struct irq_fwspec parent_fwspec;
> +	irq_hw_number_t parent_hwirq;
> +	irq_hw_number_t hwirq;
> +	unsigned int type;
> +	int  ret;
> +
> +	ret = qcom_mpm_translate(domain, fwspec, &hwirq, &type);
> +	if (ret)
> +		return ret;
> +
> +	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> +					    &qcom_mpm_chip, NULL);
> +	if (ret)
> +		return ret;
> +
> +	parent_hwirq = get_parent_hwirq(priv, hwirq);
> +	if (parent_hwirq == MPM_NO_PARENT_IRQ)
> +		return irq_domain_disconnect_hierarchy(domain->parent, virq);
> +
> +	if (type & IRQ_TYPE_EDGE_BOTH)
> +		type = IRQ_TYPE_EDGE_RISING;
> +
> +	if (type & IRQ_TYPE_LEVEL_MASK)
> +		type = IRQ_TYPE_LEVEL_HIGH;
> +
> +	parent_fwspec.fwnode      = domain->parent->fwnode;
> +	parent_fwspec.param_count = 3;
> +	parent_fwspec.param[0]    = 0;
> +	parent_fwspec.param[1]    = parent_hwirq;
> +	parent_fwspec.param[2]    = type;
> +
> +	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
> +					    &parent_fwspec);
> +}
> +
> +static const struct irq_domain_ops qcom_mpm_gic_ops = {
> +	.alloc = qcom_mpm_gic_alloc,
> +	.free = irq_domain_free_irqs_common,
> +	.translate = qcom_mpm_translate,

Same nit as above.

> +};
> +
> +static int qcom_mpm_gpio_alloc(struct irq_domain *domain, unsigned int virq,
> +			       unsigned int nr_irqs, void *data)
> +{
> +	struct irq_fwspec *fwspec = data;
> +	unsigned int type = IRQ_TYPE_NONE;
> +	irq_hw_number_t hwirq;
> +	int ret;
> +
> +	ret = qcom_mpm_translate(domain, fwspec, &hwirq, &type);
> +	if (ret)
> +		return ret;
> +
> +	return irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> +					     &qcom_mpm_chip, NULL);
> +}
> +
> +static int qcom_mpm_gpio_domain_select(struct irq_domain *domain,
> +				       struct irq_fwspec *fwspec,
> +				       enum irq_domain_bus_token bus_token)
> +{
> +	return bus_token == DOMAIN_BUS_WAKEUP;
> +}
> +
> +static const struct irq_domain_ops qcom_mpm_gpio_ops = {
> +	.select = qcom_mpm_gpio_domain_select,
> +	.alloc = qcom_mpm_gpio_alloc,
> +	.free = irq_domain_free_irqs_common,
> +	.translate = qcom_mpm_translate,
> +};
> +
> +/* Triggered by RPM when system resumes from deep sleep */
> +static irqreturn_t qcom_mpm_handler(int irq, void *dev_id)
> +{
> +	struct qcom_mpm_priv *priv = dev_id;
> +	unsigned long enable, pending;
> +	int i, j;
> +
> +	for (i = 0; i < priv->reg_stride; i++) {
> +		enable = qcom_mpm_read(priv, MPM_REG_ENABLE, i);
> +		pending = qcom_mpm_read(priv, MPM_REG_STATUS, i);
> +		pending &= enable;
> +
> +		for_each_set_bit(j, &pending, 32) {
> +			unsigned int pin = 32 * i + j;
> +			int irq = priv->pin_to_irq[pin];
> +			struct irq_desc *desc = irq ? irq_to_desc(irq) : NULL;

How can this be 0 if you have masked out the disabled interrupts?

> +
> +			if (desc && !irqd_is_level_type(&desc->irq_data))
> +				irq_set_irqchip_state(irq,
> +						IRQCHIP_STATE_PENDING, true);
> +
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +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?

> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->data = of_device_get_match_data(dev);
> +	if (!priv->data)
> +		return -ENODEV;
> +
> +	pin_num = priv->data->pin_num;
> +	priv->pin_to_irq = devm_kcalloc(dev, pin_num, sizeof(*priv->pin_to_irq),
> +					GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->reg_stride = DIV_ROUND_UP(pin_num, 32);
> +	spin_lock_init(&priv->lock);
> +
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (!priv->base)
> +		return PTR_ERR(priv->base);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	parent_domain = irq_find_host(parent);
> +	if (!parent_domain) {
> +		dev_err(dev, "failed to find MPM parent domain\n");
> +		return -ENXIO;
> +	}
> +
> +	mpm_gic_domain = irq_domain_create_hierarchy(parent_domain, 0, pin_num,
> +				of_node_to_fwnode(np), &qcom_mpm_gic_ops, priv);
> +	if (!mpm_gic_domain) {
> +		dev_err(dev, "failed to create GIC domain\n");

The message is pretty misleading.

> +		return -ENOMEM;
> +	}
> +
> +	mpm_gpio_domain = irq_domain_create_linear(of_node_to_fwnode(np),
> +				pin_num, &qcom_mpm_gpio_ops, priv);
> +	if (!mpm_gpio_domain) {
> +		dev_err(dev, "failed to create GPIO domain\n");
> +		goto remove_gic_domain;
> +	}
> +
> +	irq_domain_update_bus_token(mpm_gpio_domain, DOMAIN_BUS_WAKEUP);
> +
> +	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?

> +	}
> +
> +	ret = devm_request_irq(dev, irq, qcom_mpm_handler,
> +			       IRQF_TRIGGER_RISING | IRQF_NO_SUSPEND,
> +			       "qcom_mpm", priv);
> +	if (ret) {
> +		dev_err(dev, "failed to request irq: %d\n", ret);
> +		goto free_mbox;
> +	}
> +
> +	dev_set_drvdata(dev, priv);
> +
> +	return 0;
> +
> +free_mbox:
> +	mbox_free_channel(priv->mbox_chan);
> +remove_gpio_domain:
> +	irq_domain_remove(mpm_gpio_domain);
> +remove_gic_domain:
> +	irq_domain_remove(mpm_gic_domain);
> +	return ret;
> +}
> +
> +static int __maybe_unused qcom_mpm_suspend_late(struct device *dev)
> +{
> +	struct qcom_mpm_priv *priv = dev_get_drvdata(dev);
> +	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 */
> +	ret = mbox_send_message(priv->mbox_chan, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused qcom_mpm_resume_early(struct device *dev)
> +{
> +	/* Nothing to do for now */
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops qcom_mpm_pm_ops = {
> +	SET_LATE_SYSTEM_SLEEP_PM_OPS(qcom_mpm_suspend_late,
> +				     qcom_mpm_resume_early)
> +};
> +
> +/* Taken from downstream qcom-mpm-scuba.c with hwirq number minus 32 */

So is that a full description? Or are we only hoping that this is good
enough?

> +const struct mpm_pin qcm2290_gic_pins[] = {
> +	{ 2, 275 },	/* tsens0_tsens_upper_lower_int */
> +	{ 5, 296 },	/* lpass_irq_out_sdc */
> +	{ 12, 422 },	/* b3_lfps_rxterm_irq */
> +	{ 24, 79 },	/* bi_px_lpi_1_aoss_mx */
> +	{ 86, 183 },	/* mpm_wake,spmi_m */
> +	{ 90, 260 },	/* eud_p0_dpse_int_mx */
> +	{ 91, 260 },	/* eud_p0_dmse_int_mx */
> +	{ -1 },
> +};
> +
> +const struct mpm_data qcm2290_data = {
> +	.pin_num = 96,
> +	.gic_pins = qcm2290_gic_pins,
> +};
> +
> +static const struct of_device_id qcom_mpm_match_table[] = {
> +	{ .compatible = "qcom,qcm2290-mpm", &qcm2290_data, },
> +	{ },
> +};
> +
> +static struct platform_driver qcom_mpm_driver = {
> +	.driver = {
> +		.name = "qcom_mpm",
> +		.owner = THIS_MODULE,
> +		.of_match_table = qcom_mpm_match_table,
> +		.pm = &qcom_mpm_pm_ops,
> +		.suppress_bind_attrs = true,
> +	},
> +	.probe  = qcom_mpm_probe,
> +};
> +builtin_platform_driver(qcom_mpm_driver)
> +
> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. MSM Power Manager");
> +MODULE_LICENSE("GPL v2");

Thanks,

	M.

Marc Zyngier Nov. 26, 2021, 7:19 p.m. UTC | #2

[resending after having sorted my email config...]

Hi Shawn,

On Fri, 26 Nov 2021 09:35:29 +0000,
Shawn Guo <[e-mail]> 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 <[e-mail]>
> ---
>  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?

> +	depends on ARCH_QCOM
> +	select IRQ_DOMAIN_HIERARCHY
> +	help
> +	  MSM Power Manager driver to manage and configure wakeup
> +	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
> +
>  config CSKY_MPINTC
>  	bool
>  	depends on CSKY
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index c1f611cbfbf8..0e2e10467e28 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -94,6 +94,7 @@ obj-$(CONFIG_MESON_IRQ_GPIO)		+= irq-meson-gpio.o
>  obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
>  obj-$(CONFIG_NDS32)			+= irq-ativic32.o
>  obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
> +obj-$(CONFIG_QCOM_MPM)			+= qcom-mpm.o
>  obj-$(CONFIG_CSKY_MPINTC)		+= irq-csky-mpintc.o
>  obj-$(CONFIG_CSKY_APB_INTC)		+= irq-csky-apb-intc.o
>  obj-$(CONFIG_RISCV_INTC)		+= irq-riscv-intc.o
> diff --git a/drivers/irqchip/qcom-mpm.c b/drivers/irqchip/qcom-mpm.c
> new file mode 100644
> index 000000000000..1880c734155f
> --- /dev/null
> +++ b/drivers/irqchip/qcom-mpm.c
> @@ -0,0 +1,487 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2021, Linaro Limited
> + * Copyright (c) 2010-2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqchip/arm-gic-v3.h>
> +#include <linux/irqdomain.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +/*
> + * vMPM register layout:
> + *
> + *    31                              0
> + *    +--------------------------------+
> + *    |            TIMER0              | 0x00
> + *    +--------------------------------+
> + *    |            TIMER1              | 0x04
> + *    +--------------------------------+
> + *    |            ENABLE0             | 0x08
> + *    +--------------------------------+
> + *    |              ...               | ...
> + *    +--------------------------------+
> + *    |            ENABLEn             |
> + *    +--------------------------------+
> + *    |          FALLING_EDGE0         |
> + *    +--------------------------------+
> + *    |              ...               |
> + *    +--------------------------------+
> + *    |            STATUSn             |
> + *    +--------------------------------+
> + *
> + * n = DIV_ROUND_UP(pin_num, 32)
> + *
> + */
> +#define MPM_REG_ENABLE		0
> +#define MPM_REG_FALLING_EDGE	1
> +#define MPM_REG_RISING_EDGE	2
> +#define MPM_REG_POLARITY	3
> +#define MPM_REG_STATUS		4
> +
> +#define MPM_NO_PARENT_IRQ	~0UL
> +
> +/* MPM pin and its GIC hwirq */
> +struct mpm_pin {
> +	int pin;
> +	irq_hw_number_t hwirq;
> +};
> +
> +struct mpm_data {
> +	unsigned int pin_num;
> +	const struct mpm_pin *gic_pins;
> +};
> +
> +struct qcom_mpm_priv {
> +	void __iomem *base;
> +	spinlock_t lock;
> +	struct mbox_client mbox_client;
> +	struct mbox_chan *mbox_chan;
> +	const struct mpm_data *data;
> +	unsigned int reg_stride;
> +
> +	/* MPM pin to Linux irq number */
> +	unsigned int *pin_to_irq;
> +};
> +
> +static inline u32
> +qcom_mpm_read(struct qcom_mpm_priv *priv, unsigned int reg, unsigned int index)
> +{
> +	unsigned int offset = (reg * priv->reg_stride + index + 2) * 4;
> +
> +	return readl(priv->base + offset);

Why the non-relaxed accessors?

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

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

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

> +
> +	spin_lock_irqsave(&priv->lock, flags);

This must be a raw spinlock.

> +
> +	val = qcom_mpm_read(priv, MPM_REG_ENABLE, index);
> +	if (en)
> +		val |= 1 << shift;
> +	else
> +		val &= ~(1 << shift);

Use BIT(shift).

> +	qcom_mpm_write(priv, MPM_REG_ENABLE, index, val);
> +
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +}
> +
> +static void qcom_mpm_mask(struct irq_data *d)
> +{
> +	qcom_mpm_enable_irq(d, false);
> +
> +	if (d->parent_data)
> +		irq_chip_mask_parent(d);
> +}
> +
> +static void qcom_mpm_unmask(struct irq_data *d)
> +{
> +	qcom_mpm_enable_irq(d, true);
> +
> +	if (d->parent_data)
> +		irq_chip_unmask_parent(d);
> +}
> +
> +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?

> +		     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?

> +
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	if (!d->parent_data)
> +		return 0;
> +
> +	if (type & IRQ_TYPE_EDGE_BOTH)
> +		type = IRQ_TYPE_EDGE_RISING;
> +
> +	if (type & IRQ_TYPE_LEVEL_MASK)
> +		type = IRQ_TYPE_LEVEL_HIGH;
> +
> +	return irq_chip_set_type_parent(d, type);
> +}
> +
> +static struct irq_chip qcom_mpm_chip = {
> +	.name = "mpm",
> +	.irq_eoi = irq_chip_eoi_parent,
> +	.irq_mask = qcom_mpm_mask,
> +	.irq_disable = qcom_mpm_mask,

No. If disable is really mask, then you only need mask.

> +	.irq_unmask = qcom_mpm_unmask,
> +	.irq_retrigger = irq_chip_retrigger_hierarchy,
> +	.irq_set_type = qcom_mpm_set_type,
> +	.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
> +	.irq_set_affinity = irq_chip_set_affinity_parent,

nit: please align the members vertically.

> +};
> +
> +static irq_hw_number_t get_parent_hwirq(struct qcom_mpm_priv *priv, int pin)
> +{
> +	const struct mpm_pin *gic_pins = priv->data->gic_pins;
> +	int i;
> +
> +	for (i = 0; gic_pins[i].pin >= 0; i++) {
> +		int p = gic_pins[i].pin;
> +
> +		if (p < 0)
> +			break;
> +
> +		if (p == pin)
> +			return gic_pins[i].hwirq;
> +	}
> +
> +	return MPM_NO_PARENT_IRQ;
> +}
> +
> +static int
> +qcom_mpm_translate(struct irq_domain *domain, struct irq_fwspec *fwspec,
> +		   unsigned long *hwirq, unsigned int *type)
> +{
> +	if (is_of_node(fwspec->fwnode)) {
> +		if (fwspec->param_count != 2)
> +			return -EINVAL;
> +
> +		*hwirq = fwspec->param[0];
> +		*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}

This is a copy of irq_domain_translate_twocell().

> +
> +static int qcom_mpm_gic_alloc(struct irq_domain *domain, unsigned int virq,
> +			      unsigned int nr_irqs, void *data)
> +{
> +	struct qcom_mpm_priv *priv = domain->host_data;
> +	struct irq_fwspec *fwspec = data;
> +	struct irq_fwspec parent_fwspec;
> +	irq_hw_number_t parent_hwirq;
> +	irq_hw_number_t hwirq;
> +	unsigned int type;
> +	int  ret;
> +
> +	ret = qcom_mpm_translate(domain, fwspec, &hwirq, &type);
> +	if (ret)
> +		return ret;
> +
> +	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> +					    &qcom_mpm_chip, NULL);
> +	if (ret)
> +		return ret;
> +
> +	parent_hwirq = get_parent_hwirq(priv, hwirq);
> +	if (parent_hwirq == MPM_NO_PARENT_IRQ)
> +		return irq_domain_disconnect_hierarchy(domain->parent, virq);
> +
> +	if (type & IRQ_TYPE_EDGE_BOTH)
> +		type = IRQ_TYPE_EDGE_RISING;
> +
> +	if (type & IRQ_TYPE_LEVEL_MASK)
> +		type = IRQ_TYPE_LEVEL_HIGH;
> +
> +	parent_fwspec.fwnode      = domain->parent->fwnode;
> +	parent_fwspec.param_count = 3;
> +	parent_fwspec.param[0]    = 0;
> +	parent_fwspec.param[1]    = parent_hwirq;
> +	parent_fwspec.param[2]    = type;
> +
> +	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
> +					    &parent_fwspec);
> +}
> +
> +static const struct irq_domain_ops qcom_mpm_gic_ops = {
> +	.alloc = qcom_mpm_gic_alloc,
> +	.free = irq_domain_free_irqs_common,
> +	.translate = qcom_mpm_translate,

Same nit as above.

> +};
> +
> +static int qcom_mpm_gpio_alloc(struct irq_domain *domain, unsigned int virq,
> +			       unsigned int nr_irqs, void *data)
> +{
> +	struct irq_fwspec *fwspec = data;
> +	unsigned int type = IRQ_TYPE_NONE;
> +	irq_hw_number_t hwirq;
> +	int ret;
> +
> +	ret = qcom_mpm_translate(domain, fwspec, &hwirq, &type);
> +	if (ret)
> +		return ret;
> +
> +	return irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> +					     &qcom_mpm_chip, NULL);
> +}
> +
> +static int qcom_mpm_gpio_domain_select(struct irq_domain *domain,
> +				       struct irq_fwspec *fwspec,
> +				       enum irq_domain_bus_token bus_token)
> +{
> +	return bus_token == DOMAIN_BUS_WAKEUP;
> +}
> +
> +static const struct irq_domain_ops qcom_mpm_gpio_ops = {
> +	.select = qcom_mpm_gpio_domain_select,
> +	.alloc = qcom_mpm_gpio_alloc,
> +	.free = irq_domain_free_irqs_common,
> +	.translate = qcom_mpm_translate,
> +};
> +
> +/* Triggered by RPM when system resumes from deep sleep */
> +static irqreturn_t qcom_mpm_handler(int irq, void *dev_id)
> +{
> +	struct qcom_mpm_priv *priv = dev_id;
> +	unsigned long enable, pending;
> +	int i, j;
> +
> +	for (i = 0; i < priv->reg_stride; i++) {
> +		enable = qcom_mpm_read(priv, MPM_REG_ENABLE, i);
> +		pending = qcom_mpm_read(priv, MPM_REG_STATUS, i);
> +		pending &= enable;
> +
> +		for_each_set_bit(j, &pending, 32) {
> +			unsigned int pin = 32 * i + j;
> +			int irq = priv->pin_to_irq[pin];
> +			struct irq_desc *desc = irq ? irq_to_desc(irq) : NULL;

How can this be 0 if you have masked out the disabled interrupts?

> +
> +			if (desc && !irqd_is_level_type(&desc->irq_data))
> +				irq_set_irqchip_state(irq,
> +						IRQCHIP_STATE_PENDING, true);
> +
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +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?

> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->data = of_device_get_match_data(dev);
> +	if (!priv->data)
> +		return -ENODEV;
> +
> +	pin_num = priv->data->pin_num;
> +	priv->pin_to_irq = devm_kcalloc(dev, pin_num, sizeof(*priv->pin_to_irq),
> +					GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->reg_stride = DIV_ROUND_UP(pin_num, 32);
> +	spin_lock_init(&priv->lock);
> +
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (!priv->base)
> +		return PTR_ERR(priv->base);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	parent_domain = irq_find_host(parent);
> +	if (!parent_domain) {
> +		dev_err(dev, "failed to find MPM parent domain\n");
> +		return -ENXIO;
> +	}
> +
> +	mpm_gic_domain = irq_domain_create_hierarchy(parent_domain, 0, pin_num,
> +				of_node_to_fwnode(np), &qcom_mpm_gic_ops, priv);
> +	if (!mpm_gic_domain) {
> +		dev_err(dev, "failed to create GIC domain\n");

The message is pretty misleading.

> +		return -ENOMEM;
> +	}
> +
> +	mpm_gpio_domain = irq_domain_create_linear(of_node_to_fwnode(np),
> +				pin_num, &qcom_mpm_gpio_ops, priv);
> +	if (!mpm_gpio_domain) {
> +		dev_err(dev, "failed to create GPIO domain\n");
> +		goto remove_gic_domain;
> +	}
> +
> +	irq_domain_update_bus_token(mpm_gpio_domain, DOMAIN_BUS_WAKEUP);
> +
> +	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?

> +	}
> +
> +	ret = devm_request_irq(dev, irq, qcom_mpm_handler,
> +			       IRQF_TRIGGER_RISING | IRQF_NO_SUSPEND,
> +			       "qcom_mpm", priv);
> +	if (ret) {
> +		dev_err(dev, "failed to request irq: %d\n", ret);
> +		goto free_mbox;
> +	}
> +
> +	dev_set_drvdata(dev, priv);
> +
> +	return 0;
> +
> +free_mbox:
> +	mbox_free_channel(priv->mbox_chan);
> +remove_gpio_domain:
> +	irq_domain_remove(mpm_gpio_domain);
> +remove_gic_domain:
> +	irq_domain_remove(mpm_gic_domain);
> +	return ret;
> +}
> +
> +static int __maybe_unused qcom_mpm_suspend_late(struct device *dev)
> +{
> +	struct qcom_mpm_priv *priv = dev_get_drvdata(dev);
> +	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 */
> +	ret = mbox_send_message(priv->mbox_chan, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused qcom_mpm_resume_early(struct device *dev)
> +{
> +	/* Nothing to do for now */
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops qcom_mpm_pm_ops = {
> +	SET_LATE_SYSTEM_SLEEP_PM_OPS(qcom_mpm_suspend_late,
> +				     qcom_mpm_resume_early)
> +};
> +
> +/* Taken from downstream qcom-mpm-scuba.c with hwirq number minus 32 */

So is that a full description? Or are we only hoping that this is good
enough?

> +const struct mpm_pin qcm2290_gic_pins[] = {
> +	{ 2, 275 },	/* tsens0_tsens_upper_lower_int */
> +	{ 5, 296 },	/* lpass_irq_out_sdc */
> +	{ 12, 422 },	/* b3_lfps_rxterm_irq */
> +	{ 24, 79 },	/* bi_px_lpi_1_aoss_mx */
> +	{ 86, 183 },	/* mpm_wake,spmi_m */
> +	{ 90, 260 },	/* eud_p0_dpse_int_mx */
> +	{ 91, 260 },	/* eud_p0_dmse_int_mx */
> +	{ -1 },
> +};
> +
> +const struct mpm_data qcm2290_data = {
> +	.pin_num = 96,
> +	.gic_pins = qcm2290_gic_pins,
> +};
> +
> +static const struct of_device_id qcom_mpm_match_table[] = {
> +	{ .compatible = "qcom,qcm2290-mpm", &qcm2290_data, },
> +	{ },
> +};
> +
> +static struct platform_driver qcom_mpm_driver = {
> +	.driver = {
> +		.name = "qcom_mpm",
> +		.owner = THIS_MODULE,
> +		.of_match_table = qcom_mpm_match_table,
> +		.pm = &qcom_mpm_pm_ops,
> +		.suppress_bind_attrs = true,
> +	},
> +	.probe  = qcom_mpm_probe,
> +};
> +builtin_platform_driver(qcom_mpm_driver)
> +
> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. MSM Power Manager");
> +MODULE_LICENSE("GPL v2");

Thanks,

	M.

kernel test robot Nov. 27, 2021, 7:49 a.m. UTC | #3

Hi Shawn,

I love your patch! Perhaps something to improve:

[auto build test WARNING on tip/irq/core]
[also build test WARNING on robh/for-next v5.16-rc2 next-20211126]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Shawn-Guo/Add-Qualcomm-MPM-irqchip-driver-support/20211126-174350
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 2258a6fc33d56227a981a45069fc651d85a0076f
config: arm64-buildonly-randconfig-r006-20211126 (https://download.01.org/0day-ci/archive/20211127/[e-mail]/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 5162b558d8c0b542e752b037e72a69d5fd51eb1e)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/0day-ci/linux/commit/c6f0c60a2d210e09a08be7a8f6e64d291fc708fd
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Shawn-Guo/Add-Qualcomm-MPM-irqchip-driver-support/20211126-174350
        git checkout c6f0c60a2d210e09a08be7a8f6e64d291fc708fd
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/iio/ drivers/irqchip/ drivers/spi/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[e-mail]>

All warnings (new ones prefixed by >>):

>> drivers/irqchip/qcom-mpm.c:389:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
           if (!mpm_gpio_domain) {
               ^~~~~~~~~~~~~~~~
   drivers/irqchip/qcom-mpm.c:422:9: note: uninitialized use occurs here
           return ret;
                  ^~~
   drivers/irqchip/qcom-mpm.c:389:2: note: remove the 'if' if its condition is always false
           if (!mpm_gpio_domain) {
           ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/irqchip/qcom-mpm.c:343:9: note: initialize the variable 'ret' to silence this warning
           int ret;
                  ^
                   = 0
   1 warning generated.


vim +389 drivers/irqchip/qcom-mpm.c

   333	
   334	static int qcom_mpm_probe(struct platform_device *pdev)
   335	{
   336		struct irq_domain *parent_domain, *mpm_gic_domain, *mpm_gpio_domain;
   337		struct device *dev = &pdev->dev;
   338		struct device_node *np = dev->of_node;
   339		struct device_node *parent = of_irq_find_parent(np);
   340		struct qcom_mpm_priv *priv;
   341		unsigned int pin_num;
   342		int irq;
   343		int ret;
   344	
   345		/* See comments in platform_irqchip_probe() */
   346		if (parent && !irq_find_matching_host(parent, DOMAIN_BUS_ANY))
   347			return -EPROBE_DEFER;
   348	
   349		priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
   350		if (!priv)
   351			return -ENOMEM;
   352	
   353		priv->data = of_device_get_match_data(dev);
   354		if (!priv->data)
   355			return -ENODEV;
   356	
   357		pin_num = priv->data->pin_num;
   358		priv->pin_to_irq = devm_kcalloc(dev, pin_num, sizeof(*priv->pin_to_irq),
   359						GFP_KERNEL);
   360		if (!priv)
   361			return -ENOMEM;
   362	
   363		priv->reg_stride = DIV_ROUND_UP(pin_num, 32);
   364		spin_lock_init(&priv->lock);
   365	
   366		priv->base = devm_platform_ioremap_resource(pdev, 0);
   367		if (!priv->base)
   368			return PTR_ERR(priv->base);
   369	
   370		irq = platform_get_irq(pdev, 0);
   371		if (irq < 0)
   372			return irq;
   373	
   374		parent_domain = irq_find_host(parent);
   375		if (!parent_domain) {
   376			dev_err(dev, "failed to find MPM parent domain\n");
   377			return -ENXIO;
   378		}
   379	
   380		mpm_gic_domain = irq_domain_create_hierarchy(parent_domain, 0, pin_num,
   381					of_node_to_fwnode(np), &qcom_mpm_gic_ops, priv);
   382		if (!mpm_gic_domain) {
   383			dev_err(dev, "failed to create GIC domain\n");
   384			return -ENOMEM;
   385		}
   386	
   387		mpm_gpio_domain = irq_domain_create_linear(of_node_to_fwnode(np),
   388					pin_num, &qcom_mpm_gpio_ops, priv);
 > 389		if (!mpm_gpio_domain) {
   390			dev_err(dev, "failed to create GPIO domain\n");
   391			goto remove_gic_domain;
   392		}
   393	
   394		irq_domain_update_bus_token(mpm_gpio_domain, DOMAIN_BUS_WAKEUP);
   395	
   396		priv->mbox_client.dev = dev;
   397		priv->mbox_chan = mbox_request_channel(&priv->mbox_client, 0);
   398		if (IS_ERR(priv->mbox_chan)) {
   399			ret = PTR_ERR(priv->mbox_chan);
   400			dev_err(dev, "failed to acquire IPC channel: %d\n", ret);
   401			goto remove_gpio_domain;
   402		}
   403	
   404		ret = devm_request_irq(dev, irq, qcom_mpm_handler,
   405				       IRQF_TRIGGER_RISING | IRQF_NO_SUSPEND,
   406				       "qcom_mpm", priv);
   407		if (ret) {
   408			dev_err(dev, "failed to request irq: %d\n", ret);
   409			goto free_mbox;
   410		}
   411	
   412		dev_set_drvdata(dev, priv);
   413	
   414		return 0;
   415	
   416	free_mbox:
   417		mbox_free_channel(priv->mbox_chan);
   418	remove_gpio_domain:
   419		irq_domain_remove(mpm_gpio_domain);
   420	remove_gic_domain:
   421		irq_domain_remove(mpm_gic_domain);
   422		return ret;
   423	}
   424	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

Maulik Shah Nov. 29, 2021, 7:23 a.m. UTC | #4

Hi Shawn,

On 11/26/2021 3:05 PM, Shawn Guo wrote:
> +static int __maybe_unused qcom_mpm_suspend_late(struct device *dev)

why maybe unused?

> +{
> +	struct qcom_mpm_priv *priv = dev_get_drvdata(dev);
> +	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 */
> +	ret = mbox_send_message(priv->mbox_chan, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused qcom_mpm_resume_early(struct device *dev)
> +{
> +	/* Nothing to do for now */
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops qcom_mpm_pm_ops = {
> +	SET_LATE_SYSTEM_SLEEP_PM_OPS(qcom_mpm_suspend_late,
> +				     qcom_mpm_resume_early)
> +};

This is not limited to suspend, you will need to notify RPM during 
deepest cpu idle state entry as well, since MPM may be monitoring 
interrupts in that case too.

This may be handled for both suspend/CPUidle using cpu pm notifications 
where in last cpu entering deepest idle may notify RPM (something 
similar to what rpmh-rsc.c does)

Thanks,
Maulik

Shawn Guo Nov. 29, 2021, 1:33 p.m. UTC | #5

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 <[e-mail]> 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 <[e-mail]>
> > ---
> >  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().

> 
> > +	depends on ARCH_QCOM
> > +	select IRQ_DOMAIN_HIERARCHY
> > +	help
> > +	  MSM Power Manager driver to manage and configure wakeup
> > +	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
> > +
> >  config CSKY_MPINTC
> >  	bool
> >  	depends on CSKY
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index c1f611cbfbf8..0e2e10467e28 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -94,6 +94,7 @@ obj-$(CONFIG_MESON_IRQ_GPIO)		+= irq-meson-gpio.o
> >  obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
> >  obj-$(CONFIG_NDS32)			+= irq-ativic32.o
> >  obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
> > +obj-$(CONFIG_QCOM_MPM)			+= qcom-mpm.o
> >  obj-$(CONFIG_CSKY_MPINTC)		+= irq-csky-mpintc.o
> >  obj-$(CONFIG_CSKY_APB_INTC)		+= irq-csky-apb-intc.o
> >  obj-$(CONFIG_RISCV_INTC)		+= irq-riscv-intc.o
> > diff --git a/drivers/irqchip/qcom-mpm.c b/drivers/irqchip/qcom-mpm.c
> > new file mode 100644
> > index 000000000000..1880c734155f
> > --- /dev/null
> > +++ b/drivers/irqchip/qcom-mpm.c
> > @@ -0,0 +1,487 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2021, Linaro Limited
> > + * Copyright (c) 2010-2020, The Linux Foundation. All rights reserved.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/irqchip.h>
> > +#include <linux/irqchip/arm-gic-v3.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/mailbox_client.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/spinlock.h>
> > +
> > +/*
> > + * vMPM register layout:
> > + *
> > + *    31                              0
> > + *    +--------------------------------+
> > + *    |            TIMER0              | 0x00
> > + *    +--------------------------------+
> > + *    |            TIMER1              | 0x04
> > + *    +--------------------------------+
> > + *    |            ENABLE0             | 0x08
> > + *    +--------------------------------+
> > + *    |              ...               | ...
> > + *    +--------------------------------+
> > + *    |            ENABLEn             |
> > + *    +--------------------------------+
> > + *    |          FALLING_EDGE0         |
> > + *    +--------------------------------+
> > + *    |              ...               |
> > + *    +--------------------------------+
> > + *    |            STATUSn             |
> > + *    +--------------------------------+
> > + *
> > + * n = DIV_ROUND_UP(pin_num, 32)
> > + *
> > + */
> > +#define MPM_REG_ENABLE		0
> > +#define MPM_REG_FALLING_EDGE	1
> > +#define MPM_REG_RISING_EDGE	2
> > +#define MPM_REG_POLARITY	3
> > +#define MPM_REG_STATUS		4
> > +
> > +#define MPM_NO_PARENT_IRQ	~0UL
> > +
> > +/* MPM pin and its GIC hwirq */
> > +struct mpm_pin {
> > +	int pin;
> > +	irq_hw_number_t hwirq;
> > +};
> > +
> > +struct mpm_data {
> > +	unsigned int pin_num;
> > +	const struct mpm_pin *gic_pins;
> > +};
> > +
> > +struct qcom_mpm_priv {
> > +	void __iomem *base;
> > +	spinlock_t lock;
> > +	struct mbox_client mbox_client;
> > +	struct mbox_chan *mbox_chan;
> > +	const struct mpm_data *data;
> > +	unsigned int reg_stride;
> > +
> > +	/* MPM pin to Linux irq number */
> > +	unsigned int *pin_to_irq;
> > +};
> > +
> > +static inline u32
> > +qcom_mpm_read(struct qcom_mpm_priv *priv, unsigned int reg, unsigned int index)
> > +{
> > +	unsigned int offset = (reg * priv->reg_stride + index + 2) * 4;
> > +
> > +	return readl(priv->base + offset);
> 
> Why the non-relaxed accessors?

OK, will change to relaxed ones.

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

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

> 
> > +
> > +	spin_lock_irqsave(&priv->lock, flags);
> 
> This must be a raw spinlock.

OK.

> 
> > +
> > +	val = qcom_mpm_read(priv, MPM_REG_ENABLE, index);
> > +	if (en)
> > +		val |= 1 << shift;
> > +	else
> > +		val &= ~(1 << shift);
> 
> Use BIT(shift).

OK.

> 
> > +	qcom_mpm_write(priv, MPM_REG_ENABLE, index, val);
> > +
> > +	spin_unlock_irqrestore(&priv->lock, flags);
> > +}
> > +
> > +static void qcom_mpm_mask(struct irq_data *d)
> > +{
> > +	qcom_mpm_enable_irq(d, false);
> > +
> > +	if (d->parent_data)
> > +		irq_chip_mask_parent(d);
> > +}
> > +
> > +static void qcom_mpm_unmask(struct irq_data *d)
> > +{
> > +	qcom_mpm_enable_irq(d, true);
> > +
> > +	if (d->parent_data)
> > +		irq_chip_unmask_parent(d);
> > +}
> > +
> > +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.

> 
> > +
> > +	spin_unlock_irqrestore(&priv->lock, flags);
> > +
> > +	if (!d->parent_data)
> > +		return 0;
> > +
> > +	if (type & IRQ_TYPE_EDGE_BOTH)
> > +		type = IRQ_TYPE_EDGE_RISING;
> > +
> > +	if (type & IRQ_TYPE_LEVEL_MASK)
> > +		type = IRQ_TYPE_LEVEL_HIGH;
> > +
> > +	return irq_chip_set_type_parent(d, type);
> > +}
> > +
> > +static struct irq_chip qcom_mpm_chip = {
> > +	.name = "mpm",
> > +	.irq_eoi = irq_chip_eoi_parent,
> > +	.irq_mask = qcom_mpm_mask,
> > +	.irq_disable = qcom_mpm_mask,
> 
> No. If disable is really mask, then you only need mask.

OK.

> 
> > +	.irq_unmask = qcom_mpm_unmask,
> > +	.irq_retrigger = irq_chip_retrigger_hierarchy,
> > +	.irq_set_type = qcom_mpm_set_type,
> > +	.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
> > +	.irq_set_affinity = irq_chip_set_affinity_parent,
> 
> nit: please align the members vertically.

Sure.

> 
> > +};
> > +
> > +static irq_hw_number_t get_parent_hwirq(struct qcom_mpm_priv *priv, int pin)
> > +{
> > +	const struct mpm_pin *gic_pins = priv->data->gic_pins;
> > +	int i;
> > +
> > +	for (i = 0; gic_pins[i].pin >= 0; i++) {
> > +		int p = gic_pins[i].pin;
> > +
> > +		if (p < 0)
> > +			break;
> > +
> > +		if (p == pin)
> > +			return gic_pins[i].hwirq;
> > +	}
> > +
> > +	return MPM_NO_PARENT_IRQ;
> > +}
> > +
> > +static int
> > +qcom_mpm_translate(struct irq_domain *domain, struct irq_fwspec *fwspec,
> > +		   unsigned long *hwirq, unsigned int *type)
> > +{
> > +	if (is_of_node(fwspec->fwnode)) {
> > +		if (fwspec->param_count != 2)
> > +			return -EINVAL;
> > +
> > +		*hwirq = fwspec->param[0];
> > +		*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
> > +		return 0;
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> 
> This is a copy of irq_domain_translate_twocell().

OK.

> 
> > +
> > +static int qcom_mpm_gic_alloc(struct irq_domain *domain, unsigned int virq,
> > +			      unsigned int nr_irqs, void *data)
> > +{
> > +	struct qcom_mpm_priv *priv = domain->host_data;
> > +	struct irq_fwspec *fwspec = data;
> > +	struct irq_fwspec parent_fwspec;
> > +	irq_hw_number_t parent_hwirq;
> > +	irq_hw_number_t hwirq;
> > +	unsigned int type;
> > +	int  ret;
> > +
> > +	ret = qcom_mpm_translate(domain, fwspec, &hwirq, &type);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> > +					    &qcom_mpm_chip, NULL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	parent_hwirq = get_parent_hwirq(priv, hwirq);
> > +	if (parent_hwirq == MPM_NO_PARENT_IRQ)
> > +		return irq_domain_disconnect_hierarchy(domain->parent, virq);
> > +
> > +	if (type & IRQ_TYPE_EDGE_BOTH)
> > +		type = IRQ_TYPE_EDGE_RISING;
> > +
> > +	if (type & IRQ_TYPE_LEVEL_MASK)
> > +		type = IRQ_TYPE_LEVEL_HIGH;
> > +
> > +	parent_fwspec.fwnode      = domain->parent->fwnode;
> > +	parent_fwspec.param_count = 3;
> > +	parent_fwspec.param[0]    = 0;
> > +	parent_fwspec.param[1]    = parent_hwirq;
> > +	parent_fwspec.param[2]    = type;
> > +
> > +	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
> > +					    &parent_fwspec);
> > +}
> > +
> > +static const struct irq_domain_ops qcom_mpm_gic_ops = {
> > +	.alloc = qcom_mpm_gic_alloc,
> > +	.free = irq_domain_free_irqs_common,
> > +	.translate = qcom_mpm_translate,
> 
> Same nit as above.
> 
> > +};
> > +
> > +static int qcom_mpm_gpio_alloc(struct irq_domain *domain, unsigned int virq,
> > +			       unsigned int nr_irqs, void *data)
> > +{
> > +	struct irq_fwspec *fwspec = data;
> > +	unsigned int type = IRQ_TYPE_NONE;
> > +	irq_hw_number_t hwirq;
> > +	int ret;
> > +
> > +	ret = qcom_mpm_translate(domain, fwspec, &hwirq, &type);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> > +					     &qcom_mpm_chip, NULL);
> > +}
> > +
> > +static int qcom_mpm_gpio_domain_select(struct irq_domain *domain,
> > +				       struct irq_fwspec *fwspec,
> > +				       enum irq_domain_bus_token bus_token)
> > +{
> > +	return bus_token == DOMAIN_BUS_WAKEUP;
> > +}
> > +
> > +static const struct irq_domain_ops qcom_mpm_gpio_ops = {
> > +	.select = qcom_mpm_gpio_domain_select,
> > +	.alloc = qcom_mpm_gpio_alloc,
> > +	.free = irq_domain_free_irqs_common,
> > +	.translate = qcom_mpm_translate,
> > +};
> > +
> > +/* Triggered by RPM when system resumes from deep sleep */
> > +static irqreturn_t qcom_mpm_handler(int irq, void *dev_id)
> > +{
> > +	struct qcom_mpm_priv *priv = dev_id;
> > +	unsigned long enable, pending;
> > +	int i, j;
> > +
> > +	for (i = 0; i < priv->reg_stride; i++) {
> > +		enable = qcom_mpm_read(priv, MPM_REG_ENABLE, i);
> > +		pending = qcom_mpm_read(priv, MPM_REG_STATUS, i);
> > +		pending &= enable;
> > +
> > +		for_each_set_bit(j, &pending, 32) {
> > +			unsigned int pin = 32 * i + j;
> > +			int irq = priv->pin_to_irq[pin];
> > +			struct irq_desc *desc = irq ? irq_to_desc(irq) : NULL;
> 
> How can this be 0 if you have masked out the disabled interrupts?

OK.

> 
> > +
> > +			if (desc && !irqd_is_level_type(&desc->irq_data))
> > +				irq_set_irqchip_state(irq,
> > +						IRQCHIP_STATE_PENDING, true);
> > +
> > +		}
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +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.

> 
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->data = of_device_get_match_data(dev);
> > +	if (!priv->data)
> > +		return -ENODEV;
> > +
> > +	pin_num = priv->data->pin_num;
> > +	priv->pin_to_irq = devm_kcalloc(dev, pin_num, sizeof(*priv->pin_to_irq),
> > +					GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->reg_stride = DIV_ROUND_UP(pin_num, 32);
> > +	spin_lock_init(&priv->lock);
> > +
> > +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (!priv->base)
> > +		return PTR_ERR(priv->base);
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0)
> > +		return irq;
> > +
> > +	parent_domain = irq_find_host(parent);
> > +	if (!parent_domain) {
> > +		dev_err(dev, "failed to find MPM parent domain\n");
> > +		return -ENXIO;
> > +	}
> > +
> > +	mpm_gic_domain = irq_domain_create_hierarchy(parent_domain, 0, pin_num,
> > +				of_node_to_fwnode(np), &qcom_mpm_gic_ops, priv);
> > +	if (!mpm_gic_domain) {
> > +		dev_err(dev, "failed to create GIC domain\n");
> 
> The message is pretty misleading.

OK, will add MPM_ prefix.

> 
> > +		return -ENOMEM;
> > +	}
> > +
> > +	mpm_gpio_domain = irq_domain_create_linear(of_node_to_fwnode(np),
> > +				pin_num, &qcom_mpm_gpio_ops, priv);
> > +	if (!mpm_gpio_domain) {
> > +		dev_err(dev, "failed to create GPIO domain\n");
> > +		goto remove_gic_domain;
> > +	}
> > +
> > +	irq_domain_update_bus_token(mpm_gpio_domain, DOMAIN_BUS_WAKEUP);
> > +
> > +	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.

> 
> > +	}
> > +
> > +	ret = devm_request_irq(dev, irq, qcom_mpm_handler,
> > +			       IRQF_TRIGGER_RISING | IRQF_NO_SUSPEND,
> > +			       "qcom_mpm", priv);
> > +	if (ret) {
> > +		dev_err(dev, "failed to request irq: %d\n", ret);
> > +		goto free_mbox;
> > +	}
> > +
> > +	dev_set_drvdata(dev, priv);
> > +
> > +	return 0;
> > +
> > +free_mbox:
> > +	mbox_free_channel(priv->mbox_chan);
> > +remove_gpio_domain:
> > +	irq_domain_remove(mpm_gpio_domain);
> > +remove_gic_domain:
> > +	irq_domain_remove(mpm_gic_domain);
> > +	return ret;
> > +}
> > +
> > +static int __maybe_unused qcom_mpm_suspend_late(struct device *dev)
> > +{
> > +	struct qcom_mpm_priv *priv = dev_get_drvdata(dev);
> > +	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 */
> > +	ret = mbox_send_message(priv->mbox_chan, NULL);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused qcom_mpm_resume_early(struct device *dev)
> > +{
> > +	/* Nothing to do for now */
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops qcom_mpm_pm_ops = {
> > +	SET_LATE_SYSTEM_SLEEP_PM_OPS(qcom_mpm_suspend_late,
> > +				     qcom_mpm_resume_early)
> > +};
> > +
> > +/* Taken from downstream qcom-mpm-scuba.c with hwirq number minus 32 */
> 
> So is that a full description? Or are we only hoping that this is good
> enough?

I will improve this documentation.

Thanks for your time, Marc!

Shawn

> 
> > +const struct mpm_pin qcm2290_gic_pins[] = {
> > +	{ 2, 275 },	/* tsens0_tsens_upper_lower_int */
> > +	{ 5, 296 },	/* lpass_irq_out_sdc */
> > +	{ 12, 422 },	/* b3_lfps_rxterm_irq */
> > +	{ 24, 79 },	/* bi_px_lpi_1_aoss_mx */
> > +	{ 86, 183 },	/* mpm_wake,spmi_m */
> > +	{ 90, 260 },	/* eud_p0_dpse_int_mx */
> > +	{ 91, 260 },	/* eud_p0_dmse_int_mx */
> > +	{ -1 },
> > +};
> > +
> > +const struct mpm_data qcm2290_data = {
> > +	.pin_num = 96,
> > +	.gic_pins = qcm2290_gic_pins,
> > +};
> > +
> > +static const struct of_device_id qcom_mpm_match_table[] = {
> > +	{ .compatible = "qcom,qcm2290-mpm", &qcm2290_data, },
> > +	{ },
> > +};
> > +
> > +static struct platform_driver qcom_mpm_driver = {
> > +	.driver = {
> > +		.name = "qcom_mpm",
> > +		.owner = THIS_MODULE,
> > +		.of_match_table = qcom_mpm_match_table,
> > +		.pm = &qcom_mpm_pm_ops,
> > +		.suppress_bind_attrs = true,
> > +	},
> > +	.probe  = qcom_mpm_probe,
> > +};
> > +builtin_platform_driver(qcom_mpm_driver)
> > +
> > +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. MSM Power Manager");
> > +MODULE_LICENSE("GPL v2");

Shawn Guo Nov. 29, 2021, 1:45 p.m. UTC | #6

On Mon, Nov 29, 2021 at 12:53:39PM +0530, Maulik Shah wrote:
> Hi Shawn,

Hi Maulik,

Thanks for the comment!

> 
> On 11/26/2021 3:05 PM, Shawn Guo wrote:
> > +static int __maybe_unused qcom_mpm_suspend_late(struct device *dev)
> 
> why maybe unused?
> 
> > +{
> > +	struct qcom_mpm_priv *priv = dev_get_drvdata(dev);
> > +	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 */
> > +	ret = mbox_send_message(priv->mbox_chan, NULL);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused qcom_mpm_resume_early(struct device *dev)
> > +{
> > +	/* Nothing to do for now */
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops qcom_mpm_pm_ops = {
> > +	SET_LATE_SYSTEM_SLEEP_PM_OPS(qcom_mpm_suspend_late,
> > +				     qcom_mpm_resume_early)
> > +};
> 
> This is not limited to suspend, you will need to notify RPM during deepest
> cpu idle state entry as well, since MPM may be monitoring interrupts in that
> case too.

Yeah, I was trying to test this MPM driver with cpuidle, but failed to
see the SoC get into vlow/vmin state from cpuidle.  Do you have any
suggestion how I should test it properly?

> This may be handled for both suspend/CPUidle using cpu pm notifications
> where in last cpu entering deepest idle may notify RPM (something similar to
> what rpmh-rsc.c does)

Thanks for the pointer!  I will take a look.

Shawn

Marc Zyngier Nov. 29, 2021, 3:24 p.m. UTC | #7

On Mon, 29 Nov 2021 13:33:12 +0000,
Shawn Guo <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
> > > ---
> > >  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.

Shawn Guo Nov. 30, 2021, 2:31 a.m. UTC | #8

+ Maulik

On Mon, Nov 29, 2021 at 03:24:39PM +0000, Marc Zyngier wrote:
[...]
> > > > @@ -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?

I just tried to build it as a module, and it seems that "irq_to_desc" is
only available for built-in build.

> 
> [...]
> 
> > > > +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.

Maulik,

If you have some information about this, that would be great.

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

OK.

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

Both domains have MPM pins that could wake up system.

> 
> [...]
> 
> > > > +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.

OK.

> 
> [...]
> 
> > > > +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.

OK.  I will see what I can do here.

> 
> [...]
> 
> > > > +	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.

Good point, thanks!

Shawn

Maulik Shah Nov. 30, 2021, 8:26 a.m. UTC | #9

Hi,

On 11/29/2021 7:15 PM, Shawn Guo wrote:
>> This is not limited to suspend, you will need to notify RPM during deepest
>> cpu idle state entry as well, since MPM may be monitoring interrupts in that
>> case too.
> Yeah, I was trying to test this MPM driver with cpuidle, but failed to
> see the SoC get into vlow/vmin state from cpuidle.

In a few cases SoC can enter vmin/vlow from cpuidle one is from static 
screen on.

> Do you have any
> suggestion how I should test it properly?
Suspend resume (use "s2idle" and not "deep" mode on upstream kernel) is 
one good method, but you will have to make sure all drivers have removed 
votes on xo clock when entering suspend.
Also need to make sure other subsystem like modem is in power collaspe 
(look at the internal master stats driver to know if other subsystems 
entering to low power mode or not).

Thanks,
Maulik

Shawn Guo Nov. 30, 2021, 8:31 a.m. UTC | #10

On Tue, Nov 30, 2021 at 01:19:48PM +0530, Maulik Shah wrote:
>    Hi Shawn,
> 
>    On 11/30/2021 8:01 AM, Shawn Guo wrote:
> 
> +       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.
> 
> Maulik,
> 
> If you have some information about this, that would be great.
> 
>    This can be converted to read poll_timeout(). This was introduced in
>    place of wmb() to make sure writes are completed.

Hmm, in this case, writel() will just do the right thing, as it wraps
wmb() there.  Or am I missing something?

Shawn

Marc Zyngier Nov. 30, 2021, 8:42 a.m. UTC | #11

On Tue, 30 Nov 2021 02:31:52 +0000,
Shawn Guo <[e-mail]> wrote:
> 
> + Maulik
> 
> On Mon, Nov 29, 2021 at 03:24:39PM +0000, Marc Zyngier wrote:
> [...]
> > > > > @@ -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?
> 
> I just tried to build it as a module, and it seems that "irq_to_desc" is
> only available for built-in build.

Yet another thing that you should not be using. The irqdomain code
gives you everything you need without having to resort to the
internals of the core IRQ infrastructure.

> > 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.
> 
> Both domains have MPM pins that could wake up system.

Then why do you need two domains?

	M.

Shawn Guo Nov. 30, 2021, 8:44 a.m. UTC | #12

On Tue, Nov 30, 2021 at 01:56:02PM +0530, Maulik Shah wrote:
> Hi,
> 
> On 11/29/2021 7:15 PM, Shawn Guo wrote:
> > > This is not limited to suspend, you will need to notify RPM during deepest
> > > cpu idle state entry as well, since MPM may be monitoring interrupts in that
> > > case too.
> > Yeah, I was trying to test this MPM driver with cpuidle, but failed to
> > see the SoC get into vlow/vmin state from cpuidle.
> 
> In a few cases SoC can enter vmin/vlow from cpuidle one is from static
> screen on.
> 
> > Do you have any
> > suggestion how I should test it properly?
> Suspend resume (use "s2idle" and not "deep" mode on upstream kernel) is one
> good method, but you will have to make sure all drivers have removed votes
> on xo clock when entering suspend.
> Also need to make sure other subsystem like modem is in power collaspe (look
> at the internal master stats driver to know if other subsystems entering to
> low power mode or not).

I have already been able to trigger a vmin sleep with s2idle by doing:

 $ echo mem > /sys/power/state

My question is how I can get a vmin sleep in idle case, so that MPM
driver can be tested in both suspend and idle context.

Shawn

Marc Zyngier Nov. 30, 2021, 8:47 a.m. UTC | #13

On Tue, 30 Nov 2021 07:49:48 +0000,
Maulik Shah <[e-mail]> wrote:
> 
> [1  <text/plain; UTF-8 (7bit)>]
> Hi Shawn,
> 
> On 11/30/2021 8:01 AM, Shawn Guo wrote:
> >>>>> +	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.
> > Maulik,
> > 
> > If you have some information about this, that would be great.
> 
> This can be converted to read poll_timeout(). This was introduced in
> place of wmb() to make sure writes are completed.

A string of reads isn't equivalent to a dsb(st). If there is a
requirement for the write to complete, then use the required barrier.

	M.

Marc Zyngier Nov. 30, 2021, 8:52 a.m. UTC | #14

On Tue, 30 Nov 2021 08:31:44 +0000,
Shawn Guo <[e-mail]> wrote:
> 
> On Tue, Nov 30, 2021 at 01:19:48PM +0530, Maulik Shah wrote:
> >    Hi Shawn,
> > 
> >    On 11/30/2021 8:01 AM, Shawn Guo wrote:
> > 
> > +       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.
> > 
> > Maulik,
> > 
> > If you have some information about this, that would be great.
> > 
> >    This can be converted to read poll_timeout(). This was introduced in
> >    place of wmb() to make sure writes are completed.
> 
> Hmm, in this case, writel() will just do the right thing, as it wraps
> wmb() there.  Or am I missing something?

writel() places the wmb() *before* the MMIO access. This is use for
ordering with RAM access if the device is DMA capable, for example. I
seriously doubt this is the case.

My understanding of Maulik's comment is that there is a requirement
for the MMIO access to complete. And for that, a barrier *after* the
write is the right tool for the job.

	M.

Maulik Shah Nov. 30, 2021, 8:54 a.m. UTC | #15

Hi,

On 11/30/2021 2:01 PM, Shawn Guo wrote:
>>     This can be converted to read poll_timeout(). This was introduced in
>>     place of wmb() to make sure writes are completed.
> Hmm, in this case, writel() will just do the right thing, as it wraps
> wmb() there.  Or am I missing something?
>
> Shawn
#define writel(v,c)             ({ __iowmb(); writel_relaxed((v),(c)); })

writel() does not do wmb() after writel_relaxed(), it does before.

we need to make sure write is propagated, so wmb() or read  back with 
timeout need to be kept after writel() is done.

Thanks,
Maulik

Maulik Shah Nov. 30, 2021, 9:04 a.m. UTC | #16

Hi,

On 11/30/2021 2:14 PM, Shawn Guo wrote:
> On Tue, Nov 30, 2021 at 01:56:02PM +0530, Maulik Shah wrote:
>> Hi,
>>
>> On 11/29/2021 7:15 PM, Shawn Guo wrote:
>>>> This is not limited to suspend, you will need to notify RPM during deepest
>>>> cpu idle state entry as well, since MPM may be monitoring interrupts in that
>>>> case too.
>>> Yeah, I was trying to test this MPM driver with cpuidle, but failed to
>>> see the SoC get into vlow/vmin state from cpuidle.
>> In a few cases SoC can enter vmin/vlow from cpuidle one is from static
>> screen on.
>>
>>> Do you have any
>>> suggestion how I should test it properly?
>> Suspend resume (use "s2idle" and not "deep" mode on upstream kernel) is one
>> good method, but you will have to make sure all drivers have removed votes
>> on xo clock when entering suspend.
>> Also need to make sure other subsystem like modem is in power collaspe (look
>> at the internal master stats driver to know if other subsystems entering to
>> low power mode or not).
> I have already been able to trigger a vmin sleep with s2idle by doing:
>
>   $ echo mem > /sys/power/state
>
> My question is how I can get a vmin sleep in idle case, so that MPM
> driver can be tested in both suspend and idle context.
>
> Shawn

In a few cases SoC can enter vmin/vlow from cpuidle one is from static screen on.
you can turn on display and set display off timeout to maximum (30 minutes) in android phone and then just leave the device idle for few minutes

another possible way (if display is not present) is to take some wake_lock (write something to /sys/power/wake_lock) and disconnect USB and leave the device idle for few minutes.
since taking wake_lock device will not enter suspend, cpuidle can make SoC enter deepest mode like vmin (if all other conditions like other subsystem sleeping and votes on xo clock removed, etc met).

Thanks,
Maulik

Shawn Guo Nov. 30, 2021, 9:17 a.m. UTC | #17

On Tue, Nov 30, 2021 at 08:42:53AM +0000, Marc Zyngier wrote:
> On Tue, 30 Nov 2021 02:31:52 +0000,
> Shawn Guo <[e-mail]> wrote:
> > 
> > + Maulik
> > 
> > On Mon, Nov 29, 2021 at 03:24:39PM +0000, Marc Zyngier wrote:
> > [...]
> > > > > > @@ -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?
> > 
> > I just tried to build it as a module, and it seems that "irq_to_desc" is
> > only available for built-in build.
> 
> Yet another thing that you should not be using. The irqdomain code
> gives you everything you need without having to resort to the
> internals of the core IRQ infrastructure.

I see.  I should use irq_get_irq_data() rather than &desc->irq_data.

> 
> > > 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.
> > 
> > Both domains have MPM pins that could wake up system.
> 
> Then why do you need two domains?

This is basically the same situation as qcom-pdc, and I have some
description about that in the commit log:

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

Shawn

Shawn Guo Nov. 30, 2021, 9:26 a.m. UTC | #18

On Tue, Nov 30, 2021 at 02:34:28PM +0530, Maulik Shah wrote:
> Hi,
> 
> On 11/30/2021 2:14 PM, Shawn Guo wrote:
> > On Tue, Nov 30, 2021 at 01:56:02PM +0530, Maulik Shah wrote:
> > > Hi,
> > > 
> > > On 11/29/2021 7:15 PM, Shawn Guo wrote:
> > > > > This is not limited to suspend, you will need to notify RPM during deepest
> > > > > cpu idle state entry as well, since MPM may be monitoring interrupts in that
> > > > > case too.
> > > > Yeah, I was trying to test this MPM driver with cpuidle, but failed to
> > > > see the SoC get into vlow/vmin state from cpuidle.
> > > In a few cases SoC can enter vmin/vlow from cpuidle one is from static
> > > screen on.
> > > 
> > > > Do you have any
> > > > suggestion how I should test it properly?
> > > Suspend resume (use "s2idle" and not "deep" mode on upstream kernel) is one
> > > good method, but you will have to make sure all drivers have removed votes
> > > on xo clock when entering suspend.
> > > Also need to make sure other subsystem like modem is in power collaspe (look
> > > at the internal master stats driver to know if other subsystems entering to
> > > low power mode or not).
> > I have already been able to trigger a vmin sleep with s2idle by doing:
> > 
> >   $ echo mem > /sys/power/state
> > 
> > My question is how I can get a vmin sleep in idle case, so that MPM
> > driver can be tested in both suspend and idle context.
> > 
> > Shawn
> 
> In a few cases SoC can enter vmin/vlow from cpuidle one is from static screen on.
> you can turn on display and set display off timeout to maximum (30 minutes) in android phone and then just leave the device idle for few minutes
> 
> another possible way (if display is not present) is to take some wake_lock (write something to /sys/power/wake_lock) and disconnect USB and leave the device idle for few minutes.
> since taking wake_lock device will not enter suspend, cpuidle can make SoC enter deepest mode like vmin (if all other conditions like other subsystem sleeping and votes on xo clock removed, etc met).

I'm running an upstream kernel with neither display nor USB enabled.  I
will test again and check conditions of other subsystems.

Thanks for the input, Maulik!

Shawn

Marc Zyngier Nov. 30, 2021, 10:44 a.m. UTC | #19

On Tue, 30 Nov 2021 09:17:08 +0000,
Shawn Guo <[e-mail]> wrote:
> 
> On Tue, Nov 30, 2021 at 08:42:53AM +0000, Marc Zyngier wrote:
> > On Tue, 30 Nov 2021 02:31:52 +0000,
> > Shawn Guo <[e-mail]> wrote:
> > > 
> > > + Maulik
> > > 
> > > On Mon, Nov 29, 2021 at 03:24:39PM +0000, Marc Zyngier wrote:
> > > [...]
> > > > > > > @@ -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?
> > > 
> > > I just tried to build it as a module, and it seems that "irq_to_desc" is
> > > only available for built-in build.
> > 
> > Yet another thing that you should not be using. The irqdomain code
> > gives you everything you need without having to resort to the
> > internals of the core IRQ infrastructure.
> 
> I see.  I should use irq_get_irq_data() rather than &desc->irq_data.

Even better:

	desc = irq_resolve_mapping(domain, hwirq);

Job done. No extra tracking, no dubious hack in the unmask callback,
works with modules.

> 
> > 
> > > > 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.
> > > 
> > > Both domains have MPM pins that could wake up system.
> > 
> > Then why do you need two domains?
> 
> This is basically the same situation as qcom-pdc, and I have some
> description about that in the commit log:
> 
> - 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.

That doesn't answer my question.

It doesn't matter what the pins are used for as long as you can
identify which ones are routed to the GIC and which are not. You are
obviously are able to do so, since you are able to disconnect part of
the hierarchy (why is qcom_mpm_gic_alloc() named as such, since most
of the interrupts it deals with are *never* routed to the GIC).

All the interrupts have the same irqchip callbacks and act on the same
'priv' data, so they it is obvious they don't overlap in the hwirq
space.

Ergo: you can implement the whole thing with a single domain. All you
need to make sure is that you identify the pins that are routed to the
GIC, and you already have that information.

	M.

Shawn Guo Dec. 1, 2021, 7:36 a.m. UTC | #20

On Tue, Nov 30, 2021 at 10:44:15AM +0000, Marc Zyngier wrote:
> On Tue, 30 Nov 2021 09:17:08 +0000,
> Shawn Guo <[e-mail]> wrote:
> > 
> > On Tue, Nov 30, 2021 at 08:42:53AM +0000, Marc Zyngier wrote:
> > > On Tue, 30 Nov 2021 02:31:52 +0000,
> > > Shawn Guo <[e-mail]> wrote:
> > > > 
> > > > + Maulik
> > > > 
> > > > On Mon, Nov 29, 2021 at 03:24:39PM +0000, Marc Zyngier wrote:
> > > > [...]
> > > > > > > > @@ -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?
> > > > 
> > > > I just tried to build it as a module, and it seems that "irq_to_desc" is
> > > > only available for built-in build.
> > > 
> > > Yet another thing that you should not be using. The irqdomain code
> > > gives you everything you need without having to resort to the
> > > internals of the core IRQ infrastructure.
> > 
> > I see.  I should use irq_get_irq_data() rather than &desc->irq_data.
> 
> Even better:
> 
> 	desc = irq_resolve_mapping(domain, hwirq);
> 
> Job done. No extra tracking, no dubious hack in the unmask callback,
> works with modules.
> 
> > 
> > > 
> > > > > 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.
> > > > 
> > > > Both domains have MPM pins that could wake up system.
> > > 
> > > Then why do you need two domains?
> > 
> > This is basically the same situation as qcom-pdc, and I have some
> > description about that in the commit log:
> > 
> > - 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.
> 
> That doesn't answer my question.
> 
> It doesn't matter what the pins are used for as long as you can
> identify which ones are routed to the GIC and which are not. You are
> obviously are able to do so, since you are able to disconnect part of
> the hierarchy (why is qcom_mpm_gic_alloc() named as such, since most
> of the interrupts it deals with are *never* routed to the GIC).
> 
> All the interrupts have the same irqchip callbacks and act on the same
> 'priv' data, so they it is obvious they don't overlap in the hwirq
> space.
> 
> Ergo: you can implement the whole thing with a single domain. All you
> need to make sure is that you identify the pins that are routed to the
> GIC, and you already have that information.

You are right!  A single domain works.  Nice and clean!  Thanks for the
comment, Marc!

Shawn

Patch

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"
+	depends on ARCH_QCOM
+	select IRQ_DOMAIN_HIERARCHY
+	help
+	  MSM Power Manager driver to manage and configure wakeup
+	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
+
 config CSKY_MPINTC
 	bool
 	depends on CSKY
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index c1f611cbfbf8..0e2e10467e28 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -94,6 +94,7 @@  obj-$(CONFIG_MESON_IRQ_GPIO)		+= irq-meson-gpio.o
 obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
 obj-$(CONFIG_NDS32)			+= irq-ativic32.o
 obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
+obj-$(CONFIG_QCOM_MPM)			+= qcom-mpm.o
 obj-$(CONFIG_CSKY_MPINTC)		+= irq-csky-mpintc.o
 obj-$(CONFIG_CSKY_APB_INTC)		+= irq-csky-apb-intc.o
 obj-$(CONFIG_RISCV_INTC)		+= irq-riscv-intc.o
diff --git a/drivers/irqchip/qcom-mpm.c b/drivers/irqchip/qcom-mpm.c
new file mode 100644
index 000000000000..1880c734155f
--- /dev/null
+++ b/drivers/irqchip/qcom-mpm.c
@@ -0,0 +1,487 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2021, Linaro Limited
+ * Copyright (c) 2010-2020, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/arm-gic-v3.h>
+#include <linux/irqdomain.h>
+#include <linux/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+/*
+ * vMPM register layout:
+ *
+ *    31                              0
+ *    +--------------------------------+
+ *    |            TIMER0              | 0x00
+ *    +--------------------------------+
+ *    |            TIMER1              | 0x04
+ *    +--------------------------------+
+ *    |            ENABLE0             | 0x08
+ *    +--------------------------------+
+ *    |              ...               | ...
+ *    +--------------------------------+
+ *    |            ENABLEn             |
+ *    +--------------------------------+
+ *    |          FALLING_EDGE0         |
+ *    +--------------------------------+
+ *    |              ...               |
+ *    +--------------------------------+
+ *    |            STATUSn             |
+ *    +--------------------------------+
+ *
+ * n = DIV_ROUND_UP(pin_num, 32)
+ *
+ */
+#define MPM_REG_ENABLE		0
+#define MPM_REG_FALLING_EDGE	1
+#define MPM_REG_RISING_EDGE	2
+#define MPM_REG_POLARITY	3
+#define MPM_REG_STATUS		4
+
+#define MPM_NO_PARENT_IRQ	~0UL
+
+/* MPM pin and its GIC hwirq */
+struct mpm_pin {
+	int pin;
+	irq_hw_number_t hwirq;
+};
+
+struct mpm_data {
+	unsigned int pin_num;
+	const struct mpm_pin *gic_pins;
+};
+
+struct qcom_mpm_priv {
+	void __iomem *base;
+	spinlock_t lock;
+	struct mbox_client mbox_client;
+	struct mbox_chan *mbox_chan;
+	const struct mpm_data *data;
+	unsigned int reg_stride;
+
+	/* MPM pin to Linux irq number */
+	unsigned int *pin_to_irq;
+};
+
+static inline u32
+qcom_mpm_read(struct qcom_mpm_priv *priv, unsigned int reg, unsigned int index)
+{
+	unsigned int offset = (reg * priv->reg_stride + index + 2) * 4;
+
+	return readl(priv->base + offset);
+}
+
+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);
+}
+
+static inline void qcom_mpm_enable_irq(struct irq_data *d, bool en)
+{
+	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;
+	u32 val;
+
+	priv->pin_to_irq[pin] = d->irq;
+
+	spin_lock_irqsave(&priv->lock, flags);
+
+	val = qcom_mpm_read(priv, MPM_REG_ENABLE, index);
+	if (en)
+		val |= 1 << shift;
+	else
+		val &= ~(1 << shift);
+	qcom_mpm_write(priv, MPM_REG_ENABLE, index, val);
+
+	spin_unlock_irqrestore(&priv->lock, flags);
+}
+
+static void qcom_mpm_mask(struct irq_data *d)
+{
+	qcom_mpm_enable_irq(d, false);
+
+	if (d->parent_data)
+		irq_chip_mask_parent(d);
+}
+
+static void qcom_mpm_unmask(struct irq_data *d)
+{
+	qcom_mpm_enable_irq(d, true);
+
+	if (d->parent_data)
+		irq_chip_unmask_parent(d);
+}
+
+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,
+		     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);
+
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	if (!d->parent_data)
+		return 0;
+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		type = IRQ_TYPE_EDGE_RISING;
+
+	if (type & IRQ_TYPE_LEVEL_MASK)
+		type = IRQ_TYPE_LEVEL_HIGH;
+
+	return irq_chip_set_type_parent(d, type);
+}
+
+static struct irq_chip qcom_mpm_chip = {
+	.name = "mpm",
+	.irq_eoi = irq_chip_eoi_parent,
+	.irq_mask = qcom_mpm_mask,
+	.irq_disable = qcom_mpm_mask,
+	.irq_unmask = qcom_mpm_unmask,
+	.irq_retrigger = irq_chip_retrigger_hierarchy,
+	.irq_set_type = qcom_mpm_set_type,
+	.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
+	.irq_set_affinity = irq_chip_set_affinity_parent,
+};
+
+static irq_hw_number_t get_parent_hwirq(struct qcom_mpm_priv *priv, int pin)
+{
+	const struct mpm_pin *gic_pins = priv->data->gic_pins;
+	int i;
+
+	for (i = 0; gic_pins[i].pin >= 0; i++) {
+		int p = gic_pins[i].pin;
+
+		if (p < 0)
+			break;
+
+		if (p == pin)
+			return gic_pins[i].hwirq;
+	}
+
+	return MPM_NO_PARENT_IRQ;
+}
+
+static int
+qcom_mpm_translate(struct irq_domain *domain, struct irq_fwspec *fwspec,
+		   unsigned long *hwirq, unsigned int *type)
+{
+	if (is_of_node(fwspec->fwnode)) {
+		if (fwspec->param_count != 2)
+			return -EINVAL;
+
+		*hwirq = fwspec->param[0];
+		*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int qcom_mpm_gic_alloc(struct irq_domain *domain, unsigned int virq,
+			      unsigned int nr_irqs, void *data)
+{
+	struct qcom_mpm_priv *priv = domain->host_data;
+	struct irq_fwspec *fwspec = data;
+	struct irq_fwspec parent_fwspec;
+	irq_hw_number_t parent_hwirq;
+	irq_hw_number_t hwirq;
+	unsigned int type;
+	int  ret;
+
+	ret = qcom_mpm_translate(domain, fwspec, &hwirq, &type);
+	if (ret)
+		return ret;
+
+	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+					    &qcom_mpm_chip, NULL);
+	if (ret)
+		return ret;
+
+	parent_hwirq = get_parent_hwirq(priv, hwirq);
+	if (parent_hwirq == MPM_NO_PARENT_IRQ)
+		return irq_domain_disconnect_hierarchy(domain->parent, virq);
+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		type = IRQ_TYPE_EDGE_RISING;
+
+	if (type & IRQ_TYPE_LEVEL_MASK)
+		type = IRQ_TYPE_LEVEL_HIGH;
+
+	parent_fwspec.fwnode      = domain->parent->fwnode;
+	parent_fwspec.param_count = 3;
+	parent_fwspec.param[0]    = 0;
+	parent_fwspec.param[1]    = parent_hwirq;
+	parent_fwspec.param[2]    = type;
+
+	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
+					    &parent_fwspec);
+}
+
+static const struct irq_domain_ops qcom_mpm_gic_ops = {
+	.alloc = qcom_mpm_gic_alloc,
+	.free = irq_domain_free_irqs_common,
+	.translate = qcom_mpm_translate,
+};
+
+static int qcom_mpm_gpio_alloc(struct irq_domain *domain, unsigned int virq,
+			       unsigned int nr_irqs, void *data)
+{
+	struct irq_fwspec *fwspec = data;
+	unsigned int type = IRQ_TYPE_NONE;
+	irq_hw_number_t hwirq;
+	int ret;
+
+	ret = qcom_mpm_translate(domain, fwspec, &hwirq, &type);
+	if (ret)
+		return ret;
+
+	return irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+					     &qcom_mpm_chip, NULL);
+}
+
+static int qcom_mpm_gpio_domain_select(struct irq_domain *domain,
+				       struct irq_fwspec *fwspec,
+				       enum irq_domain_bus_token bus_token)
+{
+	return bus_token == DOMAIN_BUS_WAKEUP;
+}
+
+static const struct irq_domain_ops qcom_mpm_gpio_ops = {
+	.select = qcom_mpm_gpio_domain_select,
+	.alloc = qcom_mpm_gpio_alloc,
+	.free = irq_domain_free_irqs_common,
+	.translate = qcom_mpm_translate,
+};
+
+/* Triggered by RPM when system resumes from deep sleep */
+static irqreturn_t qcom_mpm_handler(int irq, void *dev_id)
+{
+	struct qcom_mpm_priv *priv = dev_id;
+	unsigned long enable, pending;
+	int i, j;
+
+	for (i = 0; i < priv->reg_stride; i++) {
+		enable = qcom_mpm_read(priv, MPM_REG_ENABLE, i);
+		pending = qcom_mpm_read(priv, MPM_REG_STATUS, i);
+		pending &= enable;
+
+		for_each_set_bit(j, &pending, 32) {
+			unsigned int pin = 32 * i + j;
+			int irq = priv->pin_to_irq[pin];
+			struct irq_desc *desc = irq ? irq_to_desc(irq) : NULL;
+
+			if (desc && !irqd_is_level_type(&desc->irq_data))
+				irq_set_irqchip_state(irq,
+						IRQCHIP_STATE_PENDING, true);
+
+		}
+	}
+
+	return IRQ_HANDLED;
+}
+
+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;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->data = of_device_get_match_data(dev);
+	if (!priv->data)
+		return -ENODEV;
+
+	pin_num = priv->data->pin_num;
+	priv->pin_to_irq = devm_kcalloc(dev, pin_num, sizeof(*priv->pin_to_irq),
+					GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->reg_stride = DIV_ROUND_UP(pin_num, 32);
+	spin_lock_init(&priv->lock);
+
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (!priv->base)
+		return PTR_ERR(priv->base);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	parent_domain = irq_find_host(parent);
+	if (!parent_domain) {
+		dev_err(dev, "failed to find MPM parent domain\n");
+		return -ENXIO;
+	}
+
+	mpm_gic_domain = irq_domain_create_hierarchy(parent_domain, 0, pin_num,
+				of_node_to_fwnode(np), &qcom_mpm_gic_ops, priv);
+	if (!mpm_gic_domain) {
+		dev_err(dev, "failed to create GIC domain\n");
+		return -ENOMEM;
+	}
+
+	mpm_gpio_domain = irq_domain_create_linear(of_node_to_fwnode(np),
+				pin_num, &qcom_mpm_gpio_ops, priv);
+	if (!mpm_gpio_domain) {
+		dev_err(dev, "failed to create GPIO domain\n");
+		goto remove_gic_domain;
+	}
+
+	irq_domain_update_bus_token(mpm_gpio_domain, DOMAIN_BUS_WAKEUP);
+
+	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;
+	}
+
+	ret = devm_request_irq(dev, irq, qcom_mpm_handler,
+			       IRQF_TRIGGER_RISING | IRQF_NO_SUSPEND,
+			       "qcom_mpm", priv);
+	if (ret) {
+		dev_err(dev, "failed to request irq: %d\n", ret);
+		goto free_mbox;
+	}
+
+	dev_set_drvdata(dev, priv);
+
+	return 0;
+
+free_mbox:
+	mbox_free_channel(priv->mbox_chan);
+remove_gpio_domain:
+	irq_domain_remove(mpm_gpio_domain);
+remove_gic_domain:
+	irq_domain_remove(mpm_gic_domain);
+	return ret;
+}
+
+static int __maybe_unused qcom_mpm_suspend_late(struct device *dev)
+{
+	struct qcom_mpm_priv *priv = dev_get_drvdata(dev);
+	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 */
+	ret = mbox_send_message(priv->mbox_chan, NULL);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int __maybe_unused qcom_mpm_resume_early(struct device *dev)
+{
+	/* Nothing to do for now */
+	return 0;
+}
+
+static const struct dev_pm_ops qcom_mpm_pm_ops = {
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(qcom_mpm_suspend_late,
+				     qcom_mpm_resume_early)
+};
+
+/* Taken from downstream qcom-mpm-scuba.c with hwirq number minus 32 */
+const struct mpm_pin qcm2290_gic_pins[] = {
+	{ 2, 275 },	/* tsens0_tsens_upper_lower_int */
+	{ 5, 296 },	/* lpass_irq_out_sdc */
+	{ 12, 422 },	/* b3_lfps_rxterm_irq */
+	{ 24, 79 },	/* bi_px_lpi_1_aoss_mx */
+	{ 86, 183 },	/* mpm_wake,spmi_m */
+	{ 90, 260 },	/* eud_p0_dpse_int_mx */
+	{ 91, 260 },	/* eud_p0_dmse_int_mx */
+	{ -1 },
+};
+
+const struct mpm_data qcm2290_data = {
+	.pin_num = 96,
+	.gic_pins = qcm2290_gic_pins,
+};
+
+static const struct of_device_id qcom_mpm_match_table[] = {
+	{ .compatible = "qcom,qcm2290-mpm", &qcm2290_data, },
+	{ },
+};
+
+static struct platform_driver qcom_mpm_driver = {
+	.driver = {
+		.name = "qcom_mpm",
+		.owner = THIS_MODULE,
+		.of_match_table = qcom_mpm_match_table,
+		.pm = &qcom_mpm_pm_ops,
+		.suppress_bind_attrs = true,
+	},
+	.probe  = qcom_mpm_probe,
+};
+builtin_platform_driver(qcom_mpm_driver)
+
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. MSM Power Manager");
+MODULE_LICENSE("GPL v2");