* Re: [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver
2021-11-26 9:35 ` [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver Shawn Guo
@ 2021-11-26 15:13 ` Marc Zyngier
2021-11-26 19:19 ` Marc Zyngier
` (2 subsequent siblings)
3 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2021-11-26 15:13 UTC (permalink / raw)
To: Shawn Guo
Cc: Thomas Gleixner, Bjorn Andersson, Rob Herring, Loic Poulain,
devicetree, linux-arm-msm, linux-kernel
Hi Shawn,
On Fri, 26 Nov 2021 09:35:29 +0000,
Shawn Guo <shawn.guo@linaro.org> wrote:
>
> Qualcomm SoCs based on the RPM architecture have a MSM Power Manager (MPM)
> in always-on domain. In addition to managing resources during sleep, the
> hardware also has an interrupt controller that monitors the interrupts
> when the system is asleep, wakes up the APSS when one of these interrupts
> occur and replays it to GIC after it becomes operational.
>
> It adds an irqchip driver for this interrupt controller, and here are
> some notes about it.
>
> - For given SoC, a fixed number of MPM pins are supported, e.g. 96 pins
> on QCM2290. Each of these MPM pins can be either a MPM_GIC pin or
> a MPM_GPIO pin. The mapping between MPM_GIC pin and GIC interrupt
> is defined by SoC, as well as the mapping between MPM_GPIO pin and
> GPIO number. The former mapping can be found as the SoC data in this
> MPM driver, while the latter can be found as the msm_gpio_wakeirq_map[]
> in TLMM driver.
>
> - Two irq domains are created for a single irq_chip to handle MPM_GIC
> and MPM_GPIO pins respectively, i.e. MPM_GIC domain and MPM_GPIO domain.
> The former is a child domain of GIC irq domain, while the latter is
> a parent domain of TLMM/GPIO irq domain.
>
> - All the register settings are done by APSS on the an internal memory
> region called vMPM, and RPM will flush them into hardware after it
> receives a mailbox/IPC notification from APSS.
>
> - When SoC gets awake from sleep mode, the driver will receive an
> interrupt from RPM, so that it can replay interrupt for particular
> polarity.
>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
> drivers/irqchip/Kconfig | 8 +
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/qcom-mpm.c | 487 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 496 insertions(+)
> create mode 100644 drivers/irqchip/qcom-mpm.c
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 7038957f4a77..e126b19190ef 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -430,6 +430,14 @@ config QCOM_PDC
> Power Domain Controller driver to manage and configure wakeup
> IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
>
> +config QCOM_MPM
> + bool "QCOM MPM"
Can't be built as a module?
> + 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.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver
2021-11-26 9:35 ` [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver Shawn Guo
2021-11-26 15:13 ` Marc Zyngier
@ 2021-11-26 19:19 ` Marc Zyngier
2021-11-29 13:33 ` Shawn Guo
2021-11-27 7:49 ` kernel test robot
2021-11-29 7:23 ` Maulik Shah
3 siblings, 1 reply; 24+ messages in thread
From: Marc Zyngier @ 2021-11-26 19:19 UTC (permalink / raw)
To: Shawn Guo
Cc: Thomas Gleixner, Bjorn Andersson, Rob Herring, Loic Poulain,
devicetree, linux-arm-msm, linux-kernel
[resending after having sorted my email config...]
Hi Shawn,
On Fri, 26 Nov 2021 09:35:29 +0000,
Shawn Guo <shawn.guo@linaro.org> wrote:
>
> Qualcomm SoCs based on the RPM architecture have a MSM Power Manager (MPM)
> in always-on domain. In addition to managing resources during sleep, the
> hardware also has an interrupt controller that monitors the interrupts
> when the system is asleep, wakes up the APSS when one of these interrupts
> occur and replays it to GIC after it becomes operational.
>
> It adds an irqchip driver for this interrupt controller, and here are
> some notes about it.
>
> - For given SoC, a fixed number of MPM pins are supported, e.g. 96 pins
> on QCM2290. Each of these MPM pins can be either a MPM_GIC pin or
> a MPM_GPIO pin. The mapping between MPM_GIC pin and GIC interrupt
> is defined by SoC, as well as the mapping between MPM_GPIO pin and
> GPIO number. The former mapping can be found as the SoC data in this
> MPM driver, while the latter can be found as the msm_gpio_wakeirq_map[]
> in TLMM driver.
>
> - Two irq domains are created for a single irq_chip to handle MPM_GIC
> and MPM_GPIO pins respectively, i.e. MPM_GIC domain and MPM_GPIO domain.
> The former is a child domain of GIC irq domain, while the latter is
> a parent domain of TLMM/GPIO irq domain.
>
> - All the register settings are done by APSS on the an internal memory
> region called vMPM, and RPM will flush them into hardware after it
> receives a mailbox/IPC notification from APSS.
>
> - When SoC gets awake from sleep mode, the driver will receive an
> interrupt from RPM, so that it can replay interrupt for particular
> polarity.
>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
> drivers/irqchip/Kconfig | 8 +
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/qcom-mpm.c | 487 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 496 insertions(+)
> create mode 100644 drivers/irqchip/qcom-mpm.c
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 7038957f4a77..e126b19190ef 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -430,6 +430,14 @@ config QCOM_PDC
> Power Domain Controller driver to manage and configure wakeup
> IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
>
> +config QCOM_MPM
> + bool "QCOM MPM"
Can't be built as a module?
> + 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.
--
Without deviation from the norm, progress is not possible.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver
2021-11-26 19:19 ` Marc Zyngier
@ 2021-11-29 13:33 ` Shawn Guo
2021-11-29 15:24 ` Marc Zyngier
0 siblings, 1 reply; 24+ messages in thread
From: Shawn Guo @ 2021-11-29 13:33 UTC (permalink / raw)
To: Marc Zyngier
Cc: Thomas Gleixner, Bjorn Andersson, Rob Herring, Loic Poulain,
devicetree, linux-arm-msm, linux-kernel
On Fri, Nov 26, 2021 at 07:19:07PM +0000, Marc Zyngier wrote:
> [resending after having sorted my email config...]
>
> Hi Shawn,
Hi Marc,
Thanks for all these review comments!
>
> On Fri, 26 Nov 2021 09:35:29 +0000,
> Shawn Guo <shawn.guo@linaro.org> wrote:
> >
> > Qualcomm SoCs based on the RPM architecture have a MSM Power Manager (MPM)
> > in always-on domain. In addition to managing resources during sleep, the
> > hardware also has an interrupt controller that monitors the interrupts
> > when the system is asleep, wakes up the APSS when one of these interrupts
> > occur and replays it to GIC after it becomes operational.
> >
> > It adds an irqchip driver for this interrupt controller, and here are
> > some notes about it.
> >
> > - For given SoC, a fixed number of MPM pins are supported, e.g. 96 pins
> > on QCM2290. Each of these MPM pins can be either a MPM_GIC pin or
> > a MPM_GPIO pin. The mapping between MPM_GIC pin and GIC interrupt
> > is defined by SoC, as well as the mapping between MPM_GPIO pin and
> > GPIO number. The former mapping can be found as the SoC data in this
> > MPM driver, while the latter can be found as the msm_gpio_wakeirq_map[]
> > in TLMM driver.
> >
> > - Two irq domains are created for a single irq_chip to handle MPM_GIC
> > and MPM_GPIO pins respectively, i.e. MPM_GIC domain and MPM_GPIO domain.
> > The former is a child domain of GIC irq domain, while the latter is
> > a parent domain of TLMM/GPIO irq domain.
> >
> > - All the register settings are done by APSS on the an internal memory
> > region called vMPM, and RPM will flush them into hardware after it
> > receives a mailbox/IPC notification from APSS.
> >
> > - When SoC gets awake from sleep mode, the driver will receive an
> > interrupt from RPM, so that it can replay interrupt for particular
> > polarity.
> >
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> > drivers/irqchip/Kconfig | 8 +
> > drivers/irqchip/Makefile | 1 +
> > drivers/irqchip/qcom-mpm.c | 487 +++++++++++++++++++++++++++++++++++++
> > 3 files changed, 496 insertions(+)
> > create mode 100644 drivers/irqchip/qcom-mpm.c
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index 7038957f4a77..e126b19190ef 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -430,6 +430,14 @@ config QCOM_PDC
> > Power Domain Controller driver to manage and configure wakeup
> > IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
> >
> > +config QCOM_MPM
> > + bool "QCOM MPM"
>
> Can't be built as a module?
The driver is implemented as a builtin_platform_driver().
>
> > + 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");
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver
2021-11-29 13:33 ` Shawn Guo
@ 2021-11-29 15:24 ` Marc Zyngier
2021-11-30 2:31 ` Shawn Guo
0 siblings, 1 reply; 24+ messages in thread
From: Marc Zyngier @ 2021-11-29 15:24 UTC (permalink / raw)
To: Shawn Guo
Cc: Thomas Gleixner, Bjorn Andersson, Rob Herring, Loic Poulain,
devicetree, linux-arm-msm, linux-kernel
On Mon, 29 Nov 2021 13:33:12 +0000,
Shawn Guo <shawn.guo@linaro.org> wrote:
>
> On Fri, Nov 26, 2021 at 07:19:07PM +0000, Marc Zyngier wrote:
> > [resending after having sorted my email config...]
> >
> > Hi Shawn,
>
> Hi Marc,
>
> Thanks for all these review comments!
>
> >
> > On Fri, 26 Nov 2021 09:35:29 +0000,
> > Shawn Guo <shawn.guo@linaro.org> wrote:
> > >
> > > Qualcomm SoCs based on the RPM architecture have a MSM Power Manager (MPM)
> > > in always-on domain. In addition to managing resources during sleep, the
> > > hardware also has an interrupt controller that monitors the interrupts
> > > when the system is asleep, wakes up the APSS when one of these interrupts
> > > occur and replays it to GIC after it becomes operational.
> > >
> > > It adds an irqchip driver for this interrupt controller, and here are
> > > some notes about it.
> > >
> > > - For given SoC, a fixed number of MPM pins are supported, e.g. 96 pins
> > > on QCM2290. Each of these MPM pins can be either a MPM_GIC pin or
> > > a MPM_GPIO pin. The mapping between MPM_GIC pin and GIC interrupt
> > > is defined by SoC, as well as the mapping between MPM_GPIO pin and
> > > GPIO number. The former mapping can be found as the SoC data in this
> > > MPM driver, while the latter can be found as the msm_gpio_wakeirq_map[]
> > > in TLMM driver.
> > >
> > > - Two irq domains are created for a single irq_chip to handle MPM_GIC
> > > and MPM_GPIO pins respectively, i.e. MPM_GIC domain and MPM_GPIO domain.
> > > The former is a child domain of GIC irq domain, while the latter is
> > > a parent domain of TLMM/GPIO irq domain.
> > >
> > > - All the register settings are done by APSS on the an internal memory
> > > region called vMPM, and RPM will flush them into hardware after it
> > > receives a mailbox/IPC notification from APSS.
> > >
> > > - When SoC gets awake from sleep mode, the driver will receive an
> > > interrupt from RPM, so that it can replay interrupt for particular
> > > polarity.
> > >
> > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > ---
> > > drivers/irqchip/Kconfig | 8 +
> > > drivers/irqchip/Makefile | 1 +
> > > drivers/irqchip/qcom-mpm.c | 487 +++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 496 insertions(+)
> > > create mode 100644 drivers/irqchip/qcom-mpm.c
> > >
> > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > > index 7038957f4a77..e126b19190ef 100644
> > > --- a/drivers/irqchip/Kconfig
> > > +++ b/drivers/irqchip/Kconfig
> > > @@ -430,6 +430,14 @@ config QCOM_PDC
> > > Power Domain Controller driver to manage and configure wakeup
> > > IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
> > >
> > > +config QCOM_MPM
> > > + bool "QCOM MPM"
> >
> > Can't be built as a module?
>
> The driver is implemented as a builtin_platform_driver().
This, on its own, shouldn't preclude the driver from being built as a
module. However, the config option only allows it to be built in. Why?
[...]
> > > +static inline void
> > > +qcom_mpm_write(struct qcom_mpm_priv *priv, unsigned int reg,
> > > + unsigned int index, u32 val)
> > > +{
> > > + unsigned int offset = (reg * priv->reg_stride + index + 2) * 4;
> > > + u32 r_val;
> > > +
> > > + writel(val, priv->base + offset);
> > > +
> > > + do {
> > > + r_val = readl(priv->base + offset);
> > > + udelay(5);
> > > + } while (r_val != val);
> >
> > What? Is this waiting for a bit to clear? Why isn't this one of the
> > read*_poll_timeout*() function instead? Surely you can't wait forever
> > here.
>
> This is taken from downstream, and it seems to double check the written
> value by reading it back. But to be honest, I'm not really this is
> necessary. I will do some testing with the read-back check dropped.
How about asking for specs instead? There are QC people on Cc, and
many more reading the list. Hopefully they can explain what this is
all about.
> >
> > > +}
> > > +
> > > +static inline void qcom_mpm_enable_irq(struct irq_data *d, bool en)
> > > +{
> > > + struct qcom_mpm_priv *priv = d->domain->host_data;
> >
> > This really should be stored in d->chip_data.
>
> OK.
>
> >
> > > + int pin = d->hwirq;
> > > + unsigned int index = pin / 32;
> > > + unsigned int shift = pin % 32;
> > > + unsigned long flags;
> > > + u32 val;
> > > +
> > > + priv->pin_to_irq[pin] = d->irq;
> >
> > This makes no sense. This is only reinventing the very notion of an
> > irq domain, which is to lookup the Linux interrupt based on a context
> > and a signal number.
>
> The uniqueness of this driver is that it has two irq domains. This
> little lookup table is created to avoid resolving mapping on each of
> these domains, which is less efficient. But you think this is really
> nonsense, I can change.
"efficient"? You are taking an interrupt to... err... reinject some
other interrupts. I'm sorry, but the efficiency argument sailed once
someone built this wonderful piece of HW. The first mistake was to
involve SW here, so let's not optimise for something that really
doesn't need it.
Furthermore, why would you look up anywhere other than the wake-up
domain? My impression was that only these interrupts would require
being re-triggered.
[...]
> > > +static inline void
> > > +mpm_set_type(struct qcom_mpm_priv *priv, bool set, unsigned int reg,
> > > + unsigned int index, unsigned int shift)
> > > +{
> > > + u32 val;
> > > +
> > > + val = qcom_mpm_read(priv, reg, index);
> > > + if (set)
> > > + val |= 1 << shift;
> > > + else
> > > + val &= ~(1 << shift);
> > > + qcom_mpm_write(priv, reg, index, val);
> > > +}
> > > +
> > > +static int qcom_mpm_set_type(struct irq_data *d, unsigned int type)
> > > +{
> > > + struct qcom_mpm_priv *priv = d->domain->host_data;
> > > + int pin = d->hwirq;
> > > + unsigned int index = pin / 32;
> > > + unsigned int shift = pin % 32;
> > > + unsigned long flags;
> > > +
> > > + spin_lock_irqsave(&priv->lock, flags);
> > > +
> > > + mpm_set_type(priv, (type & IRQ_TYPE_EDGE_RISING) ? 1 : 0,
> >
> > You have a bool type as the second parameter. Why the convoluted ?:
> > operator?
>
> Will change to !!(type & IRQ_TYPE_EDGE_RISING).
>
> >
> > > + MPM_REG_RISING_EDGE, index, shift);
> > > + mpm_set_type(priv, (type & IRQ_TYPE_EDGE_FALLING) ? 1 : 0,
> > > + MPM_REG_FALLING_EDGE, index, shift);
> > > + mpm_set_type(priv, (type & IRQ_TYPE_LEVEL_HIGH) ? 1 : 0,
> > > + MPM_REG_POLARITY, index, shift);
> >
> > Why does this mean for an edge interrupt?
>
> Edge polarity is configured above on MPM_REG_RISING_EDGE and
> MPM_REG_FALLING_EDGE. So I guess MPM_REG_POLARITY doesn't have an
> impact on edge interrupt. I do not have any document or information
> other than downstream code to be sure though.
A well formed 'type' will have that bit clear when any of the EDGE
flags is set. So this will always be 0. It would also be much better
if you expressed the whole thing as a switch/case.
[...]
> > > +static int qcom_mpm_probe(struct platform_device *pdev)
> > > +{
> > > + struct irq_domain *parent_domain, *mpm_gic_domain, *mpm_gpio_domain;
> > > + struct device *dev = &pdev->dev;
> > > + struct device_node *np = dev->of_node;
> > > + struct device_node *parent = of_irq_find_parent(np);
> > > + struct qcom_mpm_priv *priv;
> > > + unsigned int pin_num;
> > > + int irq;
> > > + int ret;
> > > +
> > > + /* See comments in platform_irqchip_probe() */
> > > + if (parent && !irq_find_matching_host(parent, DOMAIN_BUS_ANY))
> > > + return -EPROBE_DEFER;
> >
> > So why aren't you using that infrastructure?
>
> Because I need to populate .pm of platform_driver and use match data to
> pass mpm_data.
Then I'd rather you improve the existing infrastructure to pass the
bit of extra data you need, instead than reinventing your own.
[...]
> > > + priv->mbox_client.dev = dev;
> > > + priv->mbox_chan = mbox_request_channel(&priv->mbox_client, 0);
> > > + if (IS_ERR(priv->mbox_chan)) {
> > > + ret = PTR_ERR(priv->mbox_chan);
> > > + dev_err(dev, "failed to acquire IPC channel: %d\n", ret);
> > > + goto remove_gpio_domain;
> >
> > Why don't you request this first, before all the allocations?
>
> Then I will need to call mbox_free_channel() for any allocation failures
> afterward.
Which would be fine. Checking for dependencies before allocating
resources is good practice, specially when this can result in a probe
deferral.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver
2021-11-29 15:24 ` Marc Zyngier
@ 2021-11-30 2:31 ` Shawn Guo
2021-11-30 8:42 ` Marc Zyngier
[not found] ` <2e821841-a921-3fda-9ee6-3d5127653033@quicinc.com>
0 siblings, 2 replies; 24+ messages in thread
From: Shawn Guo @ 2021-11-30 2:31 UTC (permalink / raw)
To: Marc Zyngier
Cc: Thomas Gleixner, Maulik Shah, Bjorn Andersson, Rob Herring,
Loic Poulain, devicetree, linux-arm-msm, linux-kernel
+ 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
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver
2021-11-30 2:31 ` Shawn Guo
@ 2021-11-30 8:42 ` Marc Zyngier
2021-11-30 9:17 ` Shawn Guo
[not found] ` <2e821841-a921-3fda-9ee6-3d5127653033@quicinc.com>
1 sibling, 1 reply; 24+ messages in thread
From: Marc Zyngier @ 2021-11-30 8:42 UTC (permalink / raw)
To: Shawn Guo
Cc: Thomas Gleixner, Maulik Shah, Bjorn Andersson, Rob Herring,
Loic Poulain, devicetree, linux-arm-msm, linux-kernel
On Tue, 30 Nov 2021 02:31:52 +0000,
Shawn Guo <shawn.guo@linaro.org> 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.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver
2021-11-30 8:42 ` Marc Zyngier
@ 2021-11-30 9:17 ` Shawn Guo
2021-11-30 10:44 ` Marc Zyngier
0 siblings, 1 reply; 24+ messages in thread
From: Shawn Guo @ 2021-11-30 9:17 UTC (permalink / raw)
To: Marc Zyngier
Cc: Thomas Gleixner, Maulik Shah, Bjorn Andersson, Rob Herring,
Loic Poulain, devicetree, linux-arm-msm, linux-kernel
On Tue, Nov 30, 2021 at 08:42:53AM +0000, Marc Zyngier wrote:
> On Tue, 30 Nov 2021 02:31:52 +0000,
> Shawn Guo <shawn.guo@linaro.org> 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver
2021-11-30 9:17 ` Shawn Guo
@ 2021-11-30 10:44 ` Marc Zyngier
2021-12-01 7:36 ` Shawn Guo
0 siblings, 1 reply; 24+ messages in thread
From: Marc Zyngier @ 2021-11-30 10:44 UTC (permalink / raw)
To: Shawn Guo
Cc: Thomas Gleixner, Maulik Shah, Bjorn Andersson, Rob Herring,
Loic Poulain, devicetree, linux-arm-msm, linux-kernel
On Tue, 30 Nov 2021 09:17:08 +0000,
Shawn Guo <shawn.guo@linaro.org> 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 <shawn.guo@linaro.org> 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.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver
2021-11-30 10:44 ` Marc Zyngier
@ 2021-12-01 7:36 ` Shawn Guo
0 siblings, 0 replies; 24+ messages in thread
From: Shawn Guo @ 2021-12-01 7:36 UTC (permalink / raw)
To: Marc Zyngier
Cc: Thomas Gleixner, Maulik Shah, Bjorn Andersson, Rob Herring,
Loic Poulain, devicetree, linux-arm-msm, linux-kernel
On Tue, Nov 30, 2021 at 10:44:15AM +0000, Marc Zyngier wrote:
> On Tue, 30 Nov 2021 09:17:08 +0000,
> Shawn Guo <shawn.guo@linaro.org> 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 <shawn.guo@linaro.org> 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
^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <2e821841-a921-3fda-9ee6-3d5127653033@quicinc.com>]
* Re: [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver
[not found] ` <2e821841-a921-3fda-9ee6-3d5127653033@quicinc.com>
@ 2021-11-30 8:31 ` Shawn Guo
2021-11-30 8:52 ` Marc Zyngier
2021-11-30 8:54 ` Maulik Shah
2021-11-30 8:47 ` Marc Zyngier
1 sibling, 2 replies; 24+ messages in thread
From: Shawn Guo @ 2021-11-30 8:31 UTC (permalink / raw)
To: Maulik Shah
Cc: Marc Zyngier, Thomas Gleixner, Bjorn Andersson, Rob Herring,
Loic Poulain, devicetree, linux-arm-msm, linux-kernel
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
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver
2021-11-30 8:31 ` Shawn Guo
@ 2021-11-30 8:52 ` Marc Zyngier
2021-11-30 8:54 ` Maulik Shah
1 sibling, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2021-11-30 8:52 UTC (permalink / raw)
To: Shawn Guo
Cc: Maulik Shah, Thomas Gleixner, Bjorn Andersson, Rob Herring,
Loic Poulain, devicetree, linux-arm-msm, linux-kernel
On Tue, 30 Nov 2021 08:31:44 +0000,
Shawn Guo <shawn.guo@linaro.org> 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.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver
2021-11-30 8:31 ` Shawn Guo
2021-11-30 8:52 ` Marc Zyngier
@ 2021-11-30 8:54 ` Maulik Shah
1 sibling, 0 replies; 24+ messages in thread
From: Maulik Shah @ 2021-11-30 8:54 UTC (permalink / raw)
To: Shawn Guo
Cc: Marc Zyngier, Thomas Gleixner, Bjorn Andersson, Rob Herring,
Loic Poulain, devicetree, linux-arm-msm, linux-kernel
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver
[not found] ` <2e821841-a921-3fda-9ee6-3d5127653033@quicinc.com>
2021-11-30 8:31 ` Shawn Guo
@ 2021-11-30 8:47 ` Marc Zyngier
1 sibling, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2021-11-30 8:47 UTC (permalink / raw)
To: Maulik Shah
Cc: Shawn Guo, Thomas Gleixner, Bjorn Andersson, Rob Herring,
Loic Poulain, devicetree, linux-arm-msm, linux-kernel
On Tue, 30 Nov 2021 07:49:48 +0000,
Maulik Shah <quic_mkshah@quicinc.com> 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.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver
2021-11-26 9:35 ` [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver Shawn Guo
2021-11-26 15:13 ` Marc Zyngier
2021-11-26 19:19 ` Marc Zyngier
@ 2021-11-27 7:49 ` kernel test robot
2021-11-29 7:23 ` Maulik Shah
3 siblings, 0 replies; 24+ messages in thread
From: kernel test robot @ 2021-11-27 7:49 UTC (permalink / raw)
To: Shawn Guo, Marc Zyngier, Thomas Gleixner
Cc: llvm, kbuild-all, Bjorn Andersson, Rob Herring, Loic Poulain,
devicetree, linux-arm-msm, linux-kernel, Shawn Guo
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/202111271507.zYc4AvpT-lkp@intel.com/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 <lkp@intel.com>
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/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver
2021-11-26 9:35 ` [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver Shawn Guo
` (2 preceding siblings ...)
2021-11-27 7:49 ` kernel test robot
@ 2021-11-29 7:23 ` Maulik Shah
2021-11-29 13:45 ` Shawn Guo
3 siblings, 1 reply; 24+ messages in thread
From: Maulik Shah @ 2021-11-29 7:23 UTC (permalink / raw)
To: Shawn Guo, Marc Zyngier, Thomas Gleixner
Cc: Bjorn Andersson, Rob Herring, Loic Poulain, devicetree,
linux-arm-msm, linux-kernel
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
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver
2021-11-29 7:23 ` Maulik Shah
@ 2021-11-29 13:45 ` Shawn Guo
2021-11-30 8:26 ` Maulik Shah
0 siblings, 1 reply; 24+ messages in thread
From: Shawn Guo @ 2021-11-29 13:45 UTC (permalink / raw)
To: Maulik Shah
Cc: Marc Zyngier, Thomas Gleixner, Bjorn Andersson, Rob Herring,
Loic Poulain, devicetree, linux-arm-msm, linux-kernel
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
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver
2021-11-29 13:45 ` Shawn Guo
@ 2021-11-30 8:26 ` Maulik Shah
2021-11-30 8:44 ` Shawn Guo
0 siblings, 1 reply; 24+ messages in thread
From: Maulik Shah @ 2021-11-30 8:26 UTC (permalink / raw)
To: Shawn Guo
Cc: Marc Zyngier, Thomas Gleixner, Bjorn Andersson, Rob Herring,
Loic Poulain, devicetree, linux-arm-msm, linux-kernel
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver
2021-11-30 8:26 ` Maulik Shah
@ 2021-11-30 8:44 ` Shawn Guo
2021-11-30 9:04 ` Maulik Shah
0 siblings, 1 reply; 24+ messages in thread
From: Shawn Guo @ 2021-11-30 8:44 UTC (permalink / raw)
To: Maulik Shah
Cc: Marc Zyngier, Thomas Gleixner, Bjorn Andersson, Rob Herring,
Loic Poulain, devicetree, linux-arm-msm, linux-kernel
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver
2021-11-30 8:44 ` Shawn Guo
@ 2021-11-30 9:04 ` Maulik Shah
2021-11-30 9:26 ` Shawn Guo
0 siblings, 1 reply; 24+ messages in thread
From: Maulik Shah @ 2021-11-30 9:04 UTC (permalink / raw)
To: Shawn Guo
Cc: Marc Zyngier, Thomas Gleixner, Bjorn Andersson, Rob Herring,
Loic Poulain, devicetree, linux-arm-msm, linux-kernel
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver
2021-11-30 9:04 ` Maulik Shah
@ 2021-11-30 9:26 ` Shawn Guo
0 siblings, 0 replies; 24+ messages in thread
From: Shawn Guo @ 2021-11-30 9:26 UTC (permalink / raw)
To: Maulik Shah
Cc: Marc Zyngier, Thomas Gleixner, Bjorn Andersson, Rob Herring,
Loic Poulain, devicetree, linux-arm-msm, linux-kernel
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
^ permalink raw reply [flat|nested] 24+ messages in thread