Re: [PATCH v7 2/2] irqchip: Add Qualcomm MPM controller driver
From: Marc Zyngier
Date: Tue Mar 01 2022 - 06:13:51 EST
Hi Shawn,
On Tue, 01 Mar 2022 06:24:14 +0000,
Shawn Guo <shawn.guo@xxxxxxxxxx> 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 is retrieved from device tree, while
> the latter is defined in TLMM pinctrl driver.
>
> - qcom_mpm_enter_sleep() is called from power domain (PD) .power_off
> hook to notify RPM that APSS is about to power collapse. This
> requires MPM PD be the parent PD of CPU cluster.
Glad that you fixed that one. I guess that for RPM, this is too late
and that we're stuck with the existing stuff?
>
> - When SoC gets awake from sleep mode, the driver will receive an
> interrupt from RPM, so that it can replay interrupt for particular
> polarity.
>
> Signed-off-by: Shawn Guo <shawn.guo@xxxxxxxxxx>
> ---
> drivers/irqchip/Kconfig | 8 +
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-qcom-mpm.c | 448 +++++++++++++++++++++++++++++++++
> 3 files changed, 457 insertions(+)
> create mode 100644 drivers/irqchip/irq-qcom-mpm.c
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 7038957f4a77..680d2fcf2686 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
> + tristate "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..1f8990f812f1 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) += irq-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/irq-qcom-mpm.c b/drivers/irqchip/irq-qcom-mpm.c
> new file mode 100644
> index 000000000000..f4409c169a3a
> --- /dev/null
> +++ b/drivers/irqchip/irq-qcom-mpm.c
> @@ -0,0 +1,448 @@
> +// 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/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqchip/arm-gic-v3.h>
Please drop this. Nobody apart from the GIC drivers themselves, KVM
(which implements a GIC), and the arch code (which deals with
interrupt priorities) should ever include it.
> +#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/pm_domain.h>
> +#include <linux/slab.h>
> +#include <linux/soc/qcom/irq.h>
> +#include <linux/spinlock.h>
> +
> +/*
> + * This is the driver for Qualcomm MPM (MSM Power Manager) interrupt controller,
> + * which is commonly found on Qualcomm SoCs built on the RPM architecture.
> + * Sitting in always-on domain, MPM monitors the wakeup interrupts when SoC is
> + * asleep, and wakes up the AP when one of those interrupts occurs. This driver
> + * doesn't directly access physical MPM registers though. Instead, the access
> + * is bridged via a piece of internal memory (SRAM) that is accessible to both
> + * AP and RPM. This piece of memory is called 'vMPM' in the driver.
> + *
> + * When SoC is awake, the vMPM is owned by AP and the register setup by this
> + * driver all happens on vMPM. When AP is about to get power collapsed, the
> + * driver sends a mailbox notification to RPM, which will take over the vMPM
> + * ownership and dump vMPM into physical MPM registers. On wakeup, AP is woken
> + * up by a MPM pin/interrupt, and RPM will copy STATUS registers into vMPM.
> + * Then AP start owning vMPM again.
> + *
> + * vMPM register map:
> + *
> + * 31 0
> + * +--------------------------------+
> + * | TIMER0 | 0x00
> + * +--------------------------------+
> + * | TIMER1 | 0x04
> + * +--------------------------------+
> + * | ENABLE0 | 0x08
> + * +--------------------------------+
> + * | ... | ...
> + * +--------------------------------+
> + * | ENABLEn |
> + * +--------------------------------+
> + * | FALLING_EDGE0 |
> + * +--------------------------------+
> + * | ... |
> + * +--------------------------------+
> + * | STATUSn |
> + * +--------------------------------+
> + *
> + * n = DIV_ROUND_UP(pin_cnt, 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
> +
> +/* MPM pin map to GIC hwirq */
> +struct mpm_gic_map {
> + int pin;
> + irq_hw_number_t hwirq;
> +};
> +
> +struct qcom_mpm_priv {
> + void __iomem *base;
> + raw_spinlock_t lock;
> + struct mbox_client mbox_client;
> + struct mbox_chan *mbox_chan;
> + struct mpm_gic_map *maps;
> + unsigned int map_cnt;
> + unsigned int reg_stride;
> + struct irq_domain *domain;
> + struct generic_pm_domain genpd;
> +};
> +
> +static 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_relaxed(priv->base + offset);
> +}
> +
> +static 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;
> +
> + writel_relaxed(val, priv->base + offset);
> +
> + /* Ensure the write is completed */
> + wmb();
> +}
> +
> +static void qcom_mpm_enable_irq(struct irq_data *d, bool en)
> +{
> + struct qcom_mpm_priv *priv = d->chip_data;
> + int pin = d->hwirq;
> + unsigned int index = pin / 32;
> + unsigned int shift = pin % 32;
> + unsigned long flags;
> + u32 val;
> +
> + raw_spin_lock_irqsave(&priv->lock, flags);
> +
> + val = qcom_mpm_read(priv, MPM_REG_ENABLE, index);
> + if (en)
> + val |= BIT(shift);
> + else
> + val &= ~BIT(shift);
> + qcom_mpm_write(priv, MPM_REG_ENABLE, index, val);
> +
> + raw_spin_unlock_irqrestore(&priv->lock, flags);
__assign_bit() is what you want:
@@ -112,16 +111,12 @@ static void qcom_mpm_enable_irq(struct irq_data *d, bool en)
int pin = d->hwirq;
unsigned int index = pin / 32;
unsigned int shift = pin % 32;
- unsigned long flags;
- u32 val;
+ unsigned long flags, val;
raw_spin_lock_irqsave(&priv->lock, flags);
val = qcom_mpm_read(priv, MPM_REG_ENABLE, index);
- if (en)
- val |= BIT(shift);
- else
- val &= ~BIT(shift);
+ __assign_bit(shift, &val, en);
qcom_mpm_write(priv, MPM_REG_ENABLE, index, val);
raw_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 void mpm_set_type(struct qcom_mpm_priv *priv, bool set, unsigned int reg,
> + unsigned int index, unsigned int shift)
> +{
> + unsigned long flags;
> + u32 val;
> +
> + raw_spin_lock_irqsave(&priv->lock, flags);
> +
> + val = qcom_mpm_read(priv, reg, index);
> + if (set)
> + val |= BIT(shift);
> + else
> + val &= ~BIT(shift);
> + qcom_mpm_write(priv, reg, index, val);
> +
> + raw_spin_unlock_irqrestore(&priv->lock, flags);
Same thing:
@@ -146,16 +141,12 @@ static void qcom_mpm_unmask(struct irq_data *d)
static void mpm_set_type(struct qcom_mpm_priv *priv, bool set, unsigned int reg,
unsigned int index, unsigned int shift)
{
- unsigned long flags;
- u32 val;
+ unsigned long flags, val;
raw_spin_lock_irqsave(&priv->lock, flags);
val = qcom_mpm_read(priv, reg, index);
- if (set)
- val |= BIT(shift);
- else
- val &= ~BIT(shift);
+ __assign_bit(shift, &val, set);
qcom_mpm_write(priv, reg, index, val);
raw_spin_unlock_irqrestore(&priv->lock, flags);
> +}
> +
> +static int qcom_mpm_set_type(struct irq_data *d, unsigned int type)
> +{
> + struct qcom_mpm_priv *priv = d->chip_data;
> + int pin = d->hwirq;
> + unsigned int index = pin / 32;
> + unsigned int shift = pin % 32;
> +
> + switch (type & IRQ_TYPE_SENSE_MASK) {
> + case IRQ_TYPE_EDGE_RISING:
> + mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_RISING),
> + MPM_REG_RISING_EDGE, index, shift);
> + break;
> + case IRQ_TYPE_EDGE_FALLING:
> + mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_FALLING),
> + MPM_REG_FALLING_EDGE, index, shift);
> + break;
> + case IRQ_TYPE_LEVEL_HIGH:
> + mpm_set_type(priv, !!(type & IRQ_TYPE_LEVEL_HIGH),
> + MPM_REG_POLARITY, index, shift);
> + break;
> + }
All these '!!(type & BLAH)' are totally superfluous, as they all expand
to 'true' by construction. And this leads to a few questions:
- Shouldn't a rising interrupt clear the falling detection?
- Shouldn't a level-low clear the polarity?
- How do you handle IRQ_TYPE_EDGE_BOTH?
- How is MPM_REG_POLARITY evaluated for edge interrupts (resp the EDGE
registers for level interrupts), as you never seem to be configuring
a type here?
- What initialises the MPM trigger types at boot time?
I came up with this, but that's probably not enough (boot time init is
definitely missing):
@@ -170,16 +161,22 @@ static int qcom_mpm_set_type(struct irq_data *d, unsigned int type)
switch (type & IRQ_TYPE_SENSE_MASK) {
case IRQ_TYPE_EDGE_RISING:
- mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_RISING),
- MPM_REG_RISING_EDGE, index, shift);
+ mpm_set_type(priv, true, MPM_REG_RISING_EDGE, index, shift);
+ mpm_set_type(priv, false, MPM_REG_FALLING_EDGE, index, shift);
break;
case IRQ_TYPE_EDGE_FALLING:
- mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_FALLING),
- MPM_REG_FALLING_EDGE, index, shift);
+ mpm_set_type(priv, false, MPM_REG_RISING_EDGE, index, shift);
+ mpm_set_type(priv, true, MPM_REG_FALLING_EDGE, index, shift);
+ break;
+ case IRQ_TYPE_EDGE_BOTH:
+ mpm_set_type(priv, true, MPM_REG_RISING_EDGE, index, shift);
+ mpm_set_type(priv, true, MPM_REG_FALLING_EDGE, index, shift);
break;
case IRQ_TYPE_LEVEL_HIGH:
- mpm_set_type(priv, !!(type & IRQ_TYPE_LEVEL_HIGH),
- MPM_REG_POLARITY, index, shift);
+ mpm_set_type(priv, true, MPM_REG_POLARITY, index, shift);
+ break;
+ case IRQ_TYPE_LEVEL_LOW:
+ mpm_set_type(priv, false, MPM_REG_POLARITY, index, shift);
break;
}
> +
> + 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_unmask = qcom_mpm_unmask,
> + .irq_retrigger = irq_chip_retrigger_hierarchy,
> + .irq_set_type = qcom_mpm_set_type,
> + .irq_set_affinity = irq_chip_set_affinity_parent,
> + .flags = IRQCHIP_MASK_ON_SUSPEND |
> + IRQCHIP_SKIP_SET_WAKE,
> +};
> +
> +static struct mpm_gic_map *get_mpm_gic_map(struct qcom_mpm_priv *priv, int pin)
> +{
> + struct mpm_gic_map *maps = priv->maps;
> + int i;
> +
> + for (i = 0; i < priv->map_cnt; i++) {
> + if (maps[i].pin == pin)
> + return &maps[i];
> + }
> +
> + return NULL;
> +}
> +
> +static int qcom_mpm_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;
> + struct mpm_gic_map *map;
> + irq_hw_number_t hwirq;
> + unsigned int type;
> + int ret;
> +
> + ret = irq_domain_translate_twocell(domain, fwspec, &hwirq, &type);
> + if (ret)
> + return ret;
> +
> + ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> + &qcom_mpm_chip, priv);
> + if (ret)
> + return ret;
> +
> + map = get_mpm_gic_map(priv, hwirq);
> + if (map == NULL)
> + 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] = map->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_ops = {
> + .alloc = qcom_mpm_alloc,
> + .free = irq_domain_free_irqs_common,
> + .translate = irq_domain_translate_twocell,
> +};
> +
> +/* 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;
> + irqreturn_t ret = IRQ_NONE;
> + unsigned long flags;
> + int i, j;
> +
> + for (i = 0; i < priv->reg_stride; i++) {
> + raw_spin_lock_irqsave(&priv->lock, flags);
> + enable = qcom_mpm_read(priv, MPM_REG_ENABLE, i);
> + pending = qcom_mpm_read(priv, MPM_REG_STATUS, i);
> + pending &= enable;
> + raw_spin_unlock_irqrestore(&priv->lock, flags);
> +
> + for_each_set_bit(j, &pending, 32) {
> + unsigned int pin = 32 * i + j;
> + struct irq_desc *desc = irq_resolve_mapping(priv->domain, pin);
> + struct irq_data *d = &desc->irq_data;
> +
> + if (!irqd_is_level_type(d))
> + irq_set_irqchip_state(d->irq,
> + IRQCHIP_STATE_PENDING, true);
> + ret = IRQ_HANDLED;
> + }
> + }
> +
> + return ret;
> +}
> +
> +static int qcom_mpm_enter_sleep(struct qcom_mpm_priv *priv)
> +{
> + int i, ret;
> +
> + for (i = 0; i < priv->reg_stride; i++)
> + qcom_mpm_write(priv, MPM_REG_STATUS, i, 0);
> +
> + /* Notify RPM to write vMPM into HW */
> + ret = mbox_send_message(priv->mbox_chan, NULL);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int mpm_pd_power_off(struct generic_pm_domain *genpd)
> +{
> + struct qcom_mpm_priv *priv = container_of(genpd, struct qcom_mpm_priv,
> + genpd);
> +
> + return qcom_mpm_enter_sleep(priv);
> +}
Merge these two functions:
@@ -297,8 +294,10 @@ static irqreturn_t qcom_mpm_handler(int irq, void *dev_id)
return ret;
}
-static int qcom_mpm_enter_sleep(struct qcom_mpm_priv *priv)
+static int mpm_pd_power_off(struct generic_pm_domain *genpd)
{
+ struct qcom_mpm_priv *priv = container_of(genpd, struct qcom_mpm_priv,
+ genpd);
int i, ret;
for (i = 0; i < priv->reg_stride; i++)
@@ -312,14 +311,6 @@ static int qcom_mpm_enter_sleep(struct qcom_mpm_priv *priv)
return 0;
}
-static int mpm_pd_power_off(struct generic_pm_domain *genpd)
-{
- struct qcom_mpm_priv *priv = container_of(genpd, struct qcom_mpm_priv,
- genpd);
-
- return qcom_mpm_enter_sleep(priv);
-}
-
static int qcom_mpm_init(struct device_node *np, struct device_node *parent)
{
struct platform_device *pdev = of_find_device_by_node(np);
Thanks,
M.
--
Without deviation from the norm, progress is not possible.