[PATCH v7 0/2] Add Qualcomm MPM irqchip driver support

[PATCH v7 0/2] Add Qualcomm MPM irqchip driver support

[Shawn Guo]

It adds DT binding and driver support for Qualcomm MPM (MSM Power Manager)
interrupt controller.

Changes for v7:
- Add the missing 'static' declaration for get_mpm_gic_map()
- Add PD in MPM driver and call qcom_mpm_enter_sleep() from .power_off
  hook of the PD
- Rename driver file to irq-qcom-mpm.c

Changes for v6:
- Add new event CPU_LAST_PM_ENTER (and CPU_FIRST_PM_EXIT) in cpu_pm
- Drop vendor driver notes from commit log
- Check NULL mpm_gic_map instead to save the use of MPM_NO_PARENT_IRQ
- Add lock protection for register read in qcom_mpm_handler()
- Return IRQ_NONE if there is no pending interrupt
- Drop IRQF_TRIGGER_RISING flag from devm_request_irq() call since it's
  being specified in DT
- Drop dev_set_drvdata() call which is a leftover from previous version
- Fix dt_binding_check errors reported by upgraded dtschema

Changes for v5:
- Drop inline attributes and let compiler to decide
- Use _irqsave/_irqrestore flavour for spin lock
- Assignment on a single for irq_resolve_mapping() call
- Add documentation to explain vMPM ownership transition
- Move MPM pin map data into device tree and so use a generic compatible
- Drop the code that counts CPUs in PM and use CPU_CLUSTER_PM_ENTER
  notification instead

Changes for v4:
- Add the missing include of <linux/interrupt.h> to fix build errors
  on arm architecture.
- Leave IRQCHIP_PLATFORM_DRIVER infrastructural unchanged, and use
  of_find_device_by_node() to get platform_device pointer.

Changes for v3:
- Support module build
- Use relaxed accessors
- Add barrier call to ensure MMIO write completes
- Use d->chip_data to pass driver private data
- Use raw spinlock
- USe BIT() for bit shift
- Create a single irq domain to cover both types of MPM pins
- Call irq_resolve_mapping() to find out Linux irq number
- Save the use of ternary conditional operator and use switch/case for
  .irq_set_type call
- Drop unnecessary .irq_disable hook
- Align qcom_mpm_chip and qcom_mpm_ops members vertically
- Use helper irq_domain_translate_twocell()
- Move mailbox requesting forward in probe function
- Improve the documentation on qcm2290_gic_pins[]
- Use IRQCHIP_PLATFORM_DRIVER infrastructural
- Use cpu_pm notifier instead of .suspend_late hook to write MPM for
  sleep, so that MPM can be set up for both suspend and idle context.
  The TIMER0/1 setup is currently omitted for idle use case though,
  as I haven't been able to successfully test the idle context.

Shawn Guo (2):
  dt-bindings: interrupt-controller: Add Qualcomm MPM support
  irqchip: Add Qualcomm MPM controller driver

 .../interrupt-controller/qcom,mpm.yaml        |  96 ++++
 drivers/irqchip/Kconfig                       |   8 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-qcom-mpm.c                | 448 ++++++++++++++++++
 4 files changed, 553 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml
 create mode 100644 drivers/irqchip/irq-qcom-mpm.c

-- 
2.25.1

[PATCH v7 1/2] dt-bindings: interrupt-controller: Add Qualcomm MPM support

[Shawn Guo]

It adds DT binding support for Qualcomm MPM interrupt controller.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 .../interrupt-controller/qcom,mpm.yaml        | 96 +++++++++++++++++++
 1 file changed, 96 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml

diff --git a/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml b/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml
new file mode 100644
index 000000000000..509d20c091af
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml
@@ -0,0 +1,96 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/qcom,mpm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcom MPM Interrupt Controller
+
+maintainers:
+  - Shawn Guo <shawn.guo@linaro.org>
+
+description:
+  Qualcomm Technologies Inc. SoCs based on the RPM architecture have a
+  MSM Power Manager (MPM) that is 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 interrupt controller
+  after GIC becomes operational.
+
+allOf:
+  - $ref: /schemas/interrupt-controller.yaml#
+
+properties:
+  compatible:
+    items:
+      - const: qcom,mpm
+
+  reg:
+    maxItems: 1
+    description:
+      Specifies the base address and size of vMPM registers in RPM MSG RAM.
+
+  interrupts:
+    maxItems: 1
+    description:
+      Specify the IRQ used by RPM to wakeup APSS.
+
+  mboxes:
+    maxItems: 1
+    description:
+      Specify the mailbox used to notify RPM for writing vMPM registers.
+
+  interrupt-controller: true
+
+  '#interrupt-cells':
+    const: 2
+    description:
+      The first cell is the MPM pin number for the interrupt, and the second
+      is the trigger type.
+
+  qcom,mpm-pin-count:
+    description:
+      Specify the total MPM pin count that a SoC supports.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  qcom,mpm-pin-map:
+    description:
+      A set of MPM pin numbers and the corresponding GIC SPIs.
+    $ref: /schemas/types.yaml#/definitions/uint32-matrix
+    items:
+      items:
+        - description: MPM pin number
+        - description: GIC SPI number for the MPM pin
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - mboxes
+  - interrupt-controller
+  - '#interrupt-cells'
+  - qcom,mpm-pin-count
+  - qcom,mpm-pin-map
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    mpm: interrupt-controller@45f01b8 {
+        compatible = "qcom,mpm";
+        interrupts = <GIC_SPI 197 IRQ_TYPE_EDGE_RISING>;
+        reg = <0x45f01b8 0x1000>;
+        mboxes = <&apcs_glb 1>;
+        interrupt-controller;
+        #interrupt-cells = <2>;
+        interrupt-parent = <&intc>;
+        qcom,mpm-pin-count = <96>;
+        qcom,mpm-pin-map = <2 275>,
+                           <5 296>,
+                           <12 422>,
+                           <24 79>,
+                           <86 183>,
+                           <90 260>,
+                           <91 260>;
+    };
-- 
2.25.1

[PATCH v7 2/2] irqchip: Add Qualcomm MPM controller driver

[Shawn Guo]

Qualcomm SoCs based on the RPM architecture have a MSM Power Manager (MPM)
in always-on domain. In addition to managing resources during sleep, the
hardware also has an interrupt controller that monitors the interrupts
when the system is asleep, wakes up the APSS when one of these interrupts
occur and replays it to GIC after it becomes operational.

It adds an irqchip driver for this interrupt controller, and here are
some notes about it.

- For given SoC, a fixed number of MPM pins are supported, e.g. 96 pins
  on QCM2290.  Each of these MPM pins can be either a MPM_GIC pin or
  a MPM_GPIO pin. The mapping between MPM_GIC pin and GIC interrupt
  is defined by SoC, as well as the mapping between MPM_GPIO pin and
  GPIO number.  The former mapping is retrieved from device tree, while
  the latter is defined in TLMM pinctrl driver.

- qcom_mpm_enter_sleep() is called from power domain (PD) .power_off
  hook to notify RPM that APSS is about to power collapse.  This
  requires MPM PD be the parent PD of CPU cluster.

- 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/irq-qcom-mpm.c | 448 +++++++++++++++++++++++++++++++++
 3 files changed, 457 insertions(+)
 create mode 100644 drivers/irqchip/irq-qcom-mpm.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 7038957f4a77..680d2fcf2686 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -430,6 +430,14 @@ config QCOM_PDC
 	  Power Domain Controller driver to manage and configure wakeup
 	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
 
+config QCOM_MPM
+	tristate "QCOM MPM"
+	depends on ARCH_QCOM
+	select IRQ_DOMAIN_HIERARCHY
+	help
+	  MSM Power Manager driver to manage and configure wakeup
+	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
+
 config CSKY_MPINTC
 	bool
 	depends on CSKY
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index c1f611cbfbf8..1f8990f812f1 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -94,6 +94,7 @@ obj-$(CONFIG_MESON_IRQ_GPIO)		+= irq-meson-gpio.o
 obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
 obj-$(CONFIG_NDS32)			+= irq-ativic32.o
 obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
+obj-$(CONFIG_QCOM_MPM)			+= irq-qcom-mpm.o
 obj-$(CONFIG_CSKY_MPINTC)		+= irq-csky-mpintc.o
 obj-$(CONFIG_CSKY_APB_INTC)		+= irq-csky-apb-intc.o
 obj-$(CONFIG_RISCV_INTC)		+= irq-riscv-intc.o
diff --git a/drivers/irqchip/irq-qcom-mpm.c b/drivers/irqchip/irq-qcom-mpm.c
new file mode 100644
index 000000000000..f4409c169a3a
--- /dev/null
+++ b/drivers/irqchip/irq-qcom-mpm.c
@@ -0,0 +1,448 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2021, Linaro Limited
+ * Copyright (c) 2010-2020, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/arm-gic-v3.h>
+#include <linux/irqdomain.h>
+#include <linux/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/slab.h>
+#include <linux/soc/qcom/irq.h>
+#include <linux/spinlock.h>
+
+/*
+ * This is the driver for Qualcomm MPM (MSM Power Manager) interrupt controller,
+ * which is commonly found on Qualcomm SoCs built on the RPM architecture.
+ * Sitting in always-on domain, MPM monitors the wakeup interrupts when SoC is
+ * asleep, and wakes up the AP when one of those interrupts occurs.  This driver
+ * doesn't directly access physical MPM registers though.  Instead, the access
+ * is bridged via a piece of internal memory (SRAM) that is accessible to both
+ * AP and RPM.  This piece of memory is called 'vMPM' in the driver.
+ *
+ * When SoC is awake, the vMPM is owned by AP and the register setup by this
+ * driver all happens on vMPM.  When AP is about to get power collapsed, the
+ * driver sends a mailbox notification to RPM, which will take over the vMPM
+ * ownership and dump vMPM into physical MPM registers.  On wakeup, AP is woken
+ * up by a MPM pin/interrupt, and RPM will copy STATUS registers into vMPM.
+ * Then AP start owning vMPM again.
+ *
+ * vMPM register map:
+ *
+ *    31                              0
+ *    +--------------------------------+
+ *    |            TIMER0              | 0x00
+ *    +--------------------------------+
+ *    |            TIMER1              | 0x04
+ *    +--------------------------------+
+ *    |            ENABLE0             | 0x08
+ *    +--------------------------------+
+ *    |              ...               | ...
+ *    +--------------------------------+
+ *    |            ENABLEn             |
+ *    +--------------------------------+
+ *    |          FALLING_EDGE0         |
+ *    +--------------------------------+
+ *    |              ...               |
+ *    +--------------------------------+
+ *    |            STATUSn             |
+ *    +--------------------------------+
+ *
+ *    n = DIV_ROUND_UP(pin_cnt, 32)
+ *
+ */
+
+#define MPM_REG_ENABLE		0
+#define MPM_REG_FALLING_EDGE	1
+#define MPM_REG_RISING_EDGE	2
+#define MPM_REG_POLARITY	3
+#define MPM_REG_STATUS		4
+
+/* MPM pin map to GIC hwirq */
+struct mpm_gic_map {
+	int pin;
+	irq_hw_number_t hwirq;
+};
+
+struct qcom_mpm_priv {
+	void __iomem *base;
+	raw_spinlock_t lock;
+	struct mbox_client mbox_client;
+	struct mbox_chan *mbox_chan;
+	struct mpm_gic_map *maps;
+	unsigned int map_cnt;
+	unsigned int reg_stride;
+	struct irq_domain *domain;
+	struct generic_pm_domain genpd;
+};
+
+static u32 qcom_mpm_read(struct qcom_mpm_priv *priv, unsigned int reg,
+			 unsigned int index)
+{
+	unsigned int offset = (reg * priv->reg_stride + index + 2) * 4;
+
+	return readl_relaxed(priv->base + offset);
+}
+
+static void qcom_mpm_write(struct qcom_mpm_priv *priv, unsigned int reg,
+			   unsigned int index, u32 val)
+{
+	unsigned int offset = (reg * priv->reg_stride + index + 2) * 4;
+
+	writel_relaxed(val, priv->base + offset);
+
+	/* Ensure the write is completed */
+	wmb();
+}
+
+static void qcom_mpm_enable_irq(struct irq_data *d, bool en)
+{
+	struct qcom_mpm_priv *priv = d->chip_data;
+	int pin = d->hwirq;
+	unsigned int index = pin / 32;
+	unsigned int shift = pin % 32;
+	unsigned long flags;
+	u32 val;
+
+	raw_spin_lock_irqsave(&priv->lock, flags);
+
+	val = qcom_mpm_read(priv, MPM_REG_ENABLE, index);
+	if (en)
+		val |= BIT(shift);
+	else
+		val &= ~BIT(shift);
+	qcom_mpm_write(priv, MPM_REG_ENABLE, index, val);
+
+	raw_spin_unlock_irqrestore(&priv->lock, flags);
+}
+
+static void qcom_mpm_mask(struct irq_data *d)
+{
+	qcom_mpm_enable_irq(d, false);
+
+	if (d->parent_data)
+		irq_chip_mask_parent(d);
+}
+
+static void qcom_mpm_unmask(struct irq_data *d)
+{
+	qcom_mpm_enable_irq(d, true);
+
+	if (d->parent_data)
+		irq_chip_unmask_parent(d);
+}
+
+static void mpm_set_type(struct qcom_mpm_priv *priv, bool set, unsigned int reg,
+			 unsigned int index, unsigned int shift)
+{
+	unsigned long flags;
+	u32 val;
+
+	raw_spin_lock_irqsave(&priv->lock, flags);
+
+	val = qcom_mpm_read(priv, reg, index);
+	if (set)
+		val |= BIT(shift);
+	else
+		val &= ~BIT(shift);
+	qcom_mpm_write(priv, reg, index, val);
+
+	raw_spin_unlock_irqrestore(&priv->lock, flags);
+}
+
+static int qcom_mpm_set_type(struct irq_data *d, unsigned int type)
+{
+	struct qcom_mpm_priv *priv = d->chip_data;
+	int pin = d->hwirq;
+	unsigned int index = pin / 32;
+	unsigned int shift = pin % 32;
+
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_RISING:
+		mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_RISING),
+			     MPM_REG_RISING_EDGE, index, shift);
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_FALLING),
+			     MPM_REG_FALLING_EDGE, index, shift);
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		mpm_set_type(priv, !!(type & IRQ_TYPE_LEVEL_HIGH),
+			     MPM_REG_POLARITY, index, shift);
+		break;
+	}
+
+	if (!d->parent_data)
+		return 0;
+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		type = IRQ_TYPE_EDGE_RISING;
+
+	if (type & IRQ_TYPE_LEVEL_MASK)
+		type = IRQ_TYPE_LEVEL_HIGH;
+
+	return irq_chip_set_type_parent(d, type);
+}
+
+static struct irq_chip qcom_mpm_chip = {
+	.name			= "mpm",
+	.irq_eoi		= irq_chip_eoi_parent,
+	.irq_mask		= qcom_mpm_mask,
+	.irq_unmask		= qcom_mpm_unmask,
+	.irq_retrigger		= irq_chip_retrigger_hierarchy,
+	.irq_set_type		= qcom_mpm_set_type,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+	.flags			= IRQCHIP_MASK_ON_SUSPEND |
+				  IRQCHIP_SKIP_SET_WAKE,
+};
+
+static struct mpm_gic_map *get_mpm_gic_map(struct qcom_mpm_priv *priv, int pin)
+{
+	struct mpm_gic_map *maps = priv->maps;
+	int i;
+
+	for (i = 0; i < priv->map_cnt; i++) {
+		if (maps[i].pin == pin)
+			return &maps[i];
+	}
+
+	return NULL;
+}
+
+static int qcom_mpm_alloc(struct irq_domain *domain, unsigned int virq,
+			  unsigned int nr_irqs, void *data)
+{
+	struct qcom_mpm_priv *priv = domain->host_data;
+	struct irq_fwspec *fwspec = data;
+	struct irq_fwspec parent_fwspec;
+	struct mpm_gic_map *map;
+	irq_hw_number_t hwirq;
+	unsigned int type;
+	int  ret;
+
+	ret = irq_domain_translate_twocell(domain, fwspec, &hwirq, &type);
+	if (ret)
+		return ret;
+
+	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+					    &qcom_mpm_chip, priv);
+	if (ret)
+		return ret;
+
+	map = get_mpm_gic_map(priv, hwirq);
+	if (map == NULL)
+		return irq_domain_disconnect_hierarchy(domain->parent, virq);
+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		type = IRQ_TYPE_EDGE_RISING;
+
+	if (type & IRQ_TYPE_LEVEL_MASK)
+		type = IRQ_TYPE_LEVEL_HIGH;
+
+	parent_fwspec.fwnode = domain->parent->fwnode;
+	parent_fwspec.param_count = 3;
+	parent_fwspec.param[0] = 0;
+	parent_fwspec.param[1] = map->hwirq;
+	parent_fwspec.param[2] = type;
+
+	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
+					    &parent_fwspec);
+}
+
+static const struct irq_domain_ops qcom_mpm_ops = {
+	.alloc		= qcom_mpm_alloc,
+	.free		= irq_domain_free_irqs_common,
+	.translate	= irq_domain_translate_twocell,
+};
+
+/* Triggered by RPM when system resumes from deep sleep */
+static irqreturn_t qcom_mpm_handler(int irq, void *dev_id)
+{
+	struct qcom_mpm_priv *priv = dev_id;
+	unsigned long enable, pending;
+	irqreturn_t ret = IRQ_NONE;
+	unsigned long flags;
+	int i, j;
+
+	for (i = 0; i < priv->reg_stride; i++) {
+		raw_spin_lock_irqsave(&priv->lock, flags);
+		enable = qcom_mpm_read(priv, MPM_REG_ENABLE, i);
+		pending = qcom_mpm_read(priv, MPM_REG_STATUS, i);
+		pending &= enable;
+		raw_spin_unlock_irqrestore(&priv->lock, flags);
+
+		for_each_set_bit(j, &pending, 32) {
+			unsigned int pin = 32 * i + j;
+			struct irq_desc *desc = irq_resolve_mapping(priv->domain, pin);
+			struct irq_data *d = &desc->irq_data;
+
+			if (!irqd_is_level_type(d))
+				irq_set_irqchip_state(d->irq,
+						IRQCHIP_STATE_PENDING, true);
+			ret = IRQ_HANDLED;
+		}
+	}
+
+	return ret;
+}
+
+static int qcom_mpm_enter_sleep(struct qcom_mpm_priv *priv)
+{
+	int i, ret;
+
+	for (i = 0; i < priv->reg_stride; i++)
+		qcom_mpm_write(priv, MPM_REG_STATUS, i, 0);
+
+	/* Notify RPM to write vMPM into HW */
+	ret = mbox_send_message(priv->mbox_chan, NULL);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int mpm_pd_power_off(struct generic_pm_domain *genpd)
+{
+	struct qcom_mpm_priv *priv = container_of(genpd, struct qcom_mpm_priv,
+						  genpd);
+
+	return qcom_mpm_enter_sleep(priv);
+}
+
+static int qcom_mpm_init(struct device_node *np, struct device_node *parent)
+{
+	struct platform_device *pdev = of_find_device_by_node(np);
+	struct device *dev = &pdev->dev;
+	struct irq_domain *parent_domain;
+	struct generic_pm_domain *genpd;
+	struct qcom_mpm_priv *priv;
+	unsigned int pin_cnt;
+	int i, irq;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	ret = of_property_read_u32(np, "qcom,mpm-pin-count", &pin_cnt);
+	if (ret) {
+		dev_err(dev, "failed to read qcom,mpm-pin-count: %d\n", ret);
+		return ret;
+	}
+
+	priv->reg_stride = DIV_ROUND_UP(pin_cnt, 32);
+
+	ret = of_property_count_u32_elems(np, "qcom,mpm-pin-map");
+	if (ret < 0) {
+		dev_err(dev, "failed to read qcom,mpm-pin-map: %d\n", ret);
+		return ret;
+	}
+
+	if (ret % 2) {
+		dev_err(dev, "invalid qcom,mpm-pin-map\n");
+		return -EINVAL;
+	}
+
+	priv->map_cnt = ret / 2;
+	priv->maps = devm_kcalloc(dev, priv->map_cnt, sizeof(*priv->maps),
+				  GFP_KERNEL);
+	if (!priv->maps)
+		return -ENOMEM;
+
+	for (i = 0; i < priv->map_cnt; i++) {
+		of_property_read_u32_index(np, "qcom,mpm-pin-map", i * 2,
+					   &priv->maps[i].pin);
+		of_property_read_u32_index(np, "qcom,mpm-pin-map", i * 2 + 1,
+					   (u32 *) &priv->maps[i].hwirq);
+	}
+
+	raw_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;
+
+	genpd = &priv->genpd;
+	genpd->flags = GENPD_FLAG_IRQ_SAFE;
+	genpd->power_off = mpm_pd_power_off;
+
+	genpd->name = devm_kasprintf(dev, GFP_KERNEL, "%s", dev_name(dev));
+	if (!genpd->name)
+		return -ENOMEM;
+
+	ret = pm_genpd_init(genpd, NULL, false);
+	if (ret) {
+		dev_err(dev, "failed to init genpd: %d\n", ret);
+		return ret;
+	}
+
+	ret = of_genpd_add_provider_simple(np, genpd);
+	if (ret) {
+		dev_err(dev, "failed to add genpd provider: %d\n", ret);
+		goto remove_genpd;
+	}
+
+	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);
+		return ret;
+	}
+
+	parent_domain = irq_find_host(parent);
+	if (!parent_domain) {
+		dev_err(dev, "failed to find MPM parent domain\n");
+		ret = -ENXIO;
+		goto free_mbox;
+	}
+
+	priv->domain = irq_domain_create_hierarchy(parent_domain,
+				IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP, pin_cnt,
+				of_node_to_fwnode(np), &qcom_mpm_ops, priv);
+	if (!priv->domain) {
+		dev_err(dev, "failed to create MPM domain\n");
+		ret = -ENOMEM;
+		goto free_mbox;
+	}
+
+	irq_domain_update_bus_token(priv->domain, DOMAIN_BUS_WAKEUP);
+
+	ret = devm_request_irq(dev, irq, qcom_mpm_handler, IRQF_NO_SUSPEND,
+			       "qcom_mpm", priv);
+	if (ret) {
+		dev_err(dev, "failed to request irq: %d\n", ret);
+		goto remove_domain;
+	}
+
+	return 0;
+
+remove_domain:
+	irq_domain_remove(priv->domain);
+free_mbox:
+	mbox_free_channel(priv->mbox_chan);
+remove_genpd:
+	pm_genpd_remove(genpd);
+	return ret;
+}
+
+IRQCHIP_PLATFORM_DRIVER_BEGIN(qcom_mpm)
+IRQCHIP_MATCH("qcom,mpm", qcom_mpm_init)
+IRQCHIP_PLATFORM_DRIVER_END(qcom_mpm)
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. MSM Power Manager");
+MODULE_LICENSE("GPL v2");
-- 
2.25.1

Re: [PATCH v7 2/2] irqchip: Add Qualcomm MPM controller driver

[Marc Zyngier]

Hi Shawn,

On Tue, 01 Mar 2022 06:24:14 +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 is retrieved from device tree, while
>   the latter is defined in TLMM pinctrl driver.
> 
> - qcom_mpm_enter_sleep() is called from power domain (PD) .power_off
>   hook to notify RPM that APSS is about to power collapse.  This
>   requires MPM PD be the parent PD of CPU cluster.

Glad that you fixed that one. I guess that for RPM, this is too late
and that we're stuck with the existing stuff?

> 
> - When SoC gets awake from sleep mode, the driver will receive an
>   interrupt from RPM, so that it can replay interrupt for particular
>   polarity.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/irqchip/Kconfig        |   8 +
>  drivers/irqchip/Makefile       |   1 +
>  drivers/irqchip/irq-qcom-mpm.c | 448 +++++++++++++++++++++++++++++++++
>  3 files changed, 457 insertions(+)
>  create mode 100644 drivers/irqchip/irq-qcom-mpm.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 7038957f4a77..680d2fcf2686 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -430,6 +430,14 @@ config QCOM_PDC
>  	  Power Domain Controller driver to manage and configure wakeup
>  	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
>  
> +config QCOM_MPM
> +	tristate "QCOM MPM"
> +	depends on ARCH_QCOM
> +	select IRQ_DOMAIN_HIERARCHY
> +	help
> +	  MSM Power Manager driver to manage and configure wakeup
> +	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
> +
>  config CSKY_MPINTC
>  	bool
>  	depends on CSKY
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index c1f611cbfbf8..1f8990f812f1 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -94,6 +94,7 @@ obj-$(CONFIG_MESON_IRQ_GPIO)		+= irq-meson-gpio.o
>  obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
>  obj-$(CONFIG_NDS32)			+= irq-ativic32.o
>  obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
> +obj-$(CONFIG_QCOM_MPM)			+= irq-qcom-mpm.o
>  obj-$(CONFIG_CSKY_MPINTC)		+= irq-csky-mpintc.o
>  obj-$(CONFIG_CSKY_APB_INTC)		+= irq-csky-apb-intc.o
>  obj-$(CONFIG_RISCV_INTC)		+= irq-riscv-intc.o
> diff --git a/drivers/irqchip/irq-qcom-mpm.c b/drivers/irqchip/irq-qcom-mpm.c
> new file mode 100644
> index 000000000000..f4409c169a3a
> --- /dev/null
> +++ b/drivers/irqchip/irq-qcom-mpm.c
> @@ -0,0 +1,448 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2021, Linaro Limited
> + * Copyright (c) 2010-2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqchip/arm-gic-v3.h>

Please drop this. Nobody apart from the GIC drivers themselves, KVM
(which implements a GIC), and the arch code (which deals with
interrupt priorities) should ever include it.

> +#include <linux/irqdomain.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/slab.h>
> +#include <linux/soc/qcom/irq.h>
> +#include <linux/spinlock.h>
> +
> +/*
> + * This is the driver for Qualcomm MPM (MSM Power Manager) interrupt controller,
> + * which is commonly found on Qualcomm SoCs built on the RPM architecture.
> + * Sitting in always-on domain, MPM monitors the wakeup interrupts when SoC is
> + * asleep, and wakes up the AP when one of those interrupts occurs.  This driver
> + * doesn't directly access physical MPM registers though.  Instead, the access
> + * is bridged via a piece of internal memory (SRAM) that is accessible to both
> + * AP and RPM.  This piece of memory is called 'vMPM' in the driver.
> + *
> + * When SoC is awake, the vMPM is owned by AP and the register setup by this
> + * driver all happens on vMPM.  When AP is about to get power collapsed, the
> + * driver sends a mailbox notification to RPM, which will take over the vMPM
> + * ownership and dump vMPM into physical MPM registers.  On wakeup, AP is woken
> + * up by a MPM pin/interrupt, and RPM will copy STATUS registers into vMPM.
> + * Then AP start owning vMPM again.
> + *
> + * vMPM register map:
> + *
> + *    31                              0
> + *    +--------------------------------+
> + *    |            TIMER0              | 0x00
> + *    +--------------------------------+
> + *    |            TIMER1              | 0x04
> + *    +--------------------------------+
> + *    |            ENABLE0             | 0x08
> + *    +--------------------------------+
> + *    |              ...               | ...
> + *    +--------------------------------+
> + *    |            ENABLEn             |
> + *    +--------------------------------+
> + *    |          FALLING_EDGE0         |
> + *    +--------------------------------+
> + *    |              ...               |
> + *    +--------------------------------+
> + *    |            STATUSn             |
> + *    +--------------------------------+
> + *
> + *    n = DIV_ROUND_UP(pin_cnt, 32)
> + *
> + */
> +
> +#define MPM_REG_ENABLE		0
> +#define MPM_REG_FALLING_EDGE	1
> +#define MPM_REG_RISING_EDGE	2
> +#define MPM_REG_POLARITY	3
> +#define MPM_REG_STATUS		4
> +
> +/* MPM pin map to GIC hwirq */
> +struct mpm_gic_map {
> +	int pin;
> +	irq_hw_number_t hwirq;
> +};
> +
> +struct qcom_mpm_priv {
> +	void __iomem *base;
> +	raw_spinlock_t lock;
> +	struct mbox_client mbox_client;
> +	struct mbox_chan *mbox_chan;
> +	struct mpm_gic_map *maps;
> +	unsigned int map_cnt;
> +	unsigned int reg_stride;
> +	struct irq_domain *domain;
> +	struct generic_pm_domain genpd;
> +};
> +
> +static u32 qcom_mpm_read(struct qcom_mpm_priv *priv, unsigned int reg,
> +			 unsigned int index)
> +{
> +	unsigned int offset = (reg * priv->reg_stride + index + 2) * 4;
> +
> +	return readl_relaxed(priv->base + offset);
> +}
> +
> +static void qcom_mpm_write(struct qcom_mpm_priv *priv, unsigned int reg,
> +			   unsigned int index, u32 val)
> +{
> +	unsigned int offset = (reg * priv->reg_stride + index + 2) * 4;
> +
> +	writel_relaxed(val, priv->base + offset);
> +
> +	/* Ensure the write is completed */
> +	wmb();
> +}
> +
> +static void qcom_mpm_enable_irq(struct irq_data *d, bool en)
> +{
> +	struct qcom_mpm_priv *priv = d->chip_data;
> +	int pin = d->hwirq;
> +	unsigned int index = pin / 32;
> +	unsigned int shift = pin % 32;
> +	unsigned long flags;
> +	u32 val;
> +
> +	raw_spin_lock_irqsave(&priv->lock, flags);
> +
> +	val = qcom_mpm_read(priv, MPM_REG_ENABLE, index);
> +	if (en)
> +		val |= BIT(shift);
> +	else
> +		val &= ~BIT(shift);
> +	qcom_mpm_write(priv, MPM_REG_ENABLE, index, val);
> +
> +	raw_spin_unlock_irqrestore(&priv->lock, flags);

__assign_bit() is what you want:

@@ -112,16 +111,12 @@ static void qcom_mpm_enable_irq(struct irq_data *d, bool en)
 	int pin = d->hwirq;
 	unsigned int index = pin / 32;
 	unsigned int shift = pin % 32;
-	unsigned long flags;
-	u32 val;
+	unsigned long flags, val;
 
 	raw_spin_lock_irqsave(&priv->lock, flags);
 
 	val = qcom_mpm_read(priv, MPM_REG_ENABLE, index);
-	if (en)
-		val |= BIT(shift);
-	else
-		val &= ~BIT(shift);
+	__assign_bit(shift, &val, en);
 	qcom_mpm_write(priv, MPM_REG_ENABLE, index, val);
 
 	raw_spin_unlock_irqrestore(&priv->lock, flags);

> +}
> +
> +static void qcom_mpm_mask(struct irq_data *d)
> +{
> +	qcom_mpm_enable_irq(d, false);
> +
> +	if (d->parent_data)
> +		irq_chip_mask_parent(d);
> +}
> +
> +static void qcom_mpm_unmask(struct irq_data *d)
> +{
> +	qcom_mpm_enable_irq(d, true);
> +
> +	if (d->parent_data)
> +		irq_chip_unmask_parent(d);
> +}
> +
> +static void mpm_set_type(struct qcom_mpm_priv *priv, bool set, unsigned int reg,
> +			 unsigned int index, unsigned int shift)
> +{
> +	unsigned long flags;
> +	u32 val;
> +
> +	raw_spin_lock_irqsave(&priv->lock, flags);
> +
> +	val = qcom_mpm_read(priv, reg, index);
> +	if (set)
> +		val |= BIT(shift);
> +	else
> +		val &= ~BIT(shift);
> +	qcom_mpm_write(priv, reg, index, val);
> +
> +	raw_spin_unlock_irqrestore(&priv->lock, flags);

Same thing:

@@ -146,16 +141,12 @@ static void qcom_mpm_unmask(struct irq_data *d)
 static void mpm_set_type(struct qcom_mpm_priv *priv, bool set, unsigned int reg,
 			 unsigned int index, unsigned int shift)
 {
-	unsigned long flags;
-	u32 val;
+	unsigned long flags, val;
 
 	raw_spin_lock_irqsave(&priv->lock, flags);
 
 	val = qcom_mpm_read(priv, reg, index);
-	if (set)
-		val |= BIT(shift);
-	else
-		val &= ~BIT(shift);
+	__assign_bit(shift, &val, set);
 	qcom_mpm_write(priv, reg, index, val);
 
 	raw_spin_unlock_irqrestore(&priv->lock, flags);

> +}
> +
> +static int qcom_mpm_set_type(struct irq_data *d, unsigned int type)
> +{
> +	struct qcom_mpm_priv *priv = d->chip_data;
> +	int pin = d->hwirq;
> +	unsigned int index = pin / 32;
> +	unsigned int shift = pin % 32;
> +
> +	switch (type & IRQ_TYPE_SENSE_MASK) {
> +	case IRQ_TYPE_EDGE_RISING:
> +		mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_RISING),
> +			     MPM_REG_RISING_EDGE, index, shift);
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_FALLING),
> +			     MPM_REG_FALLING_EDGE, index, shift);
> +		break;
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		mpm_set_type(priv, !!(type & IRQ_TYPE_LEVEL_HIGH),
> +			     MPM_REG_POLARITY, index, shift);
> +		break;
> +	}

All these '!!(type & BLAH)' are totally superfluous, as they all expand
to 'true' by construction. And this leads to a few questions:

- Shouldn't a rising interrupt clear the falling detection?
- Shouldn't a level-low clear the polarity?
- How do you handle IRQ_TYPE_EDGE_BOTH?
- How is MPM_REG_POLARITY evaluated for edge interrupts (resp the EDGE
  registers for level interrupts), as you never seem to be configuring
  a type here?
- What initialises the MPM trigger types at boot time?

I came up with this, but that's probably not enough (boot time init is
definitely missing):

@@ -170,16 +161,22 @@ static int qcom_mpm_set_type(struct irq_data *d, unsigned int type)
 
 	switch (type & IRQ_TYPE_SENSE_MASK) {
 	case IRQ_TYPE_EDGE_RISING:
-		mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_RISING),
-			     MPM_REG_RISING_EDGE, index, shift);
+		mpm_set_type(priv, true, MPM_REG_RISING_EDGE, index, shift);
+		mpm_set_type(priv, false, MPM_REG_FALLING_EDGE, index, shift);
 		break;
 	case IRQ_TYPE_EDGE_FALLING:
-		mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_FALLING),
-			     MPM_REG_FALLING_EDGE, index, shift);
+		mpm_set_type(priv, false, MPM_REG_RISING_EDGE, index, shift);
+		mpm_set_type(priv, true, MPM_REG_FALLING_EDGE, index, shift);
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		mpm_set_type(priv, true, MPM_REG_RISING_EDGE, index, shift);
+		mpm_set_type(priv, true, MPM_REG_FALLING_EDGE, index, shift);
 		break;
 	case IRQ_TYPE_LEVEL_HIGH:
-		mpm_set_type(priv, !!(type & IRQ_TYPE_LEVEL_HIGH),
-			     MPM_REG_POLARITY, index, shift);
+		mpm_set_type(priv, true, MPM_REG_POLARITY, index, shift);
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		mpm_set_type(priv, false, MPM_REG_POLARITY, index, shift);
 		break;
 	}

> +
> +	if (!d->parent_data)
> +		return 0;
> +
> +	if (type & IRQ_TYPE_EDGE_BOTH)
> +		type = IRQ_TYPE_EDGE_RISING;
> +
> +	if (type & IRQ_TYPE_LEVEL_MASK)
> +		type = IRQ_TYPE_LEVEL_HIGH;
> +
> +	return irq_chip_set_type_parent(d, type);
> +}
> +
> +static struct irq_chip qcom_mpm_chip = {
> +	.name			= "mpm",
> +	.irq_eoi		= irq_chip_eoi_parent,
> +	.irq_mask		= qcom_mpm_mask,
> +	.irq_unmask		= qcom_mpm_unmask,
> +	.irq_retrigger		= irq_chip_retrigger_hierarchy,
> +	.irq_set_type		= qcom_mpm_set_type,
> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
> +	.flags			= IRQCHIP_MASK_ON_SUSPEND |
> +				  IRQCHIP_SKIP_SET_WAKE,
> +};
> +
> +static struct mpm_gic_map *get_mpm_gic_map(struct qcom_mpm_priv *priv, int pin)
> +{
> +	struct mpm_gic_map *maps = priv->maps;
> +	int i;
> +
> +	for (i = 0; i < priv->map_cnt; i++) {
> +		if (maps[i].pin == pin)
> +			return &maps[i];
> +	}
> +
> +	return NULL;
> +}
> +
> +static int qcom_mpm_alloc(struct irq_domain *domain, unsigned int virq,
> +			  unsigned int nr_irqs, void *data)
> +{
> +	struct qcom_mpm_priv *priv = domain->host_data;
> +	struct irq_fwspec *fwspec = data;
> +	struct irq_fwspec parent_fwspec;
> +	struct mpm_gic_map *map;
> +	irq_hw_number_t hwirq;
> +	unsigned int type;
> +	int  ret;
> +
> +	ret = irq_domain_translate_twocell(domain, fwspec, &hwirq, &type);
> +	if (ret)
> +		return ret;
> +
> +	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> +					    &qcom_mpm_chip, priv);
> +	if (ret)
> +		return ret;
> +
> +	map = get_mpm_gic_map(priv, hwirq);
> +	if (map == NULL)
> +		return irq_domain_disconnect_hierarchy(domain->parent, virq);
> +
> +	if (type & IRQ_TYPE_EDGE_BOTH)
> +		type = IRQ_TYPE_EDGE_RISING;
> +
> +	if (type & IRQ_TYPE_LEVEL_MASK)
> +		type = IRQ_TYPE_LEVEL_HIGH;
> +
> +	parent_fwspec.fwnode = domain->parent->fwnode;
> +	parent_fwspec.param_count = 3;
> +	parent_fwspec.param[0] = 0;
> +	parent_fwspec.param[1] = map->hwirq;
> +	parent_fwspec.param[2] = type;
> +
> +	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
> +					    &parent_fwspec);
> +}
> +
> +static const struct irq_domain_ops qcom_mpm_ops = {
> +	.alloc		= qcom_mpm_alloc,
> +	.free		= irq_domain_free_irqs_common,
> +	.translate	= irq_domain_translate_twocell,
> +};
> +
> +/* Triggered by RPM when system resumes from deep sleep */
> +static irqreturn_t qcom_mpm_handler(int irq, void *dev_id)
> +{
> +	struct qcom_mpm_priv *priv = dev_id;
> +	unsigned long enable, pending;
> +	irqreturn_t ret = IRQ_NONE;
> +	unsigned long flags;
> +	int i, j;
> +
> +	for (i = 0; i < priv->reg_stride; i++) {
> +		raw_spin_lock_irqsave(&priv->lock, flags);
> +		enable = qcom_mpm_read(priv, MPM_REG_ENABLE, i);
> +		pending = qcom_mpm_read(priv, MPM_REG_STATUS, i);
> +		pending &= enable;
> +		raw_spin_unlock_irqrestore(&priv->lock, flags);
> +
> +		for_each_set_bit(j, &pending, 32) {
> +			unsigned int pin = 32 * i + j;
> +			struct irq_desc *desc = irq_resolve_mapping(priv->domain, pin);
> +			struct irq_data *d = &desc->irq_data;
> +
> +			if (!irqd_is_level_type(d))
> +				irq_set_irqchip_state(d->irq,
> +						IRQCHIP_STATE_PENDING, true);
> +			ret = IRQ_HANDLED;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static int qcom_mpm_enter_sleep(struct qcom_mpm_priv *priv)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < priv->reg_stride; i++)
> +		qcom_mpm_write(priv, MPM_REG_STATUS, i, 0);
> +
> +	/* Notify RPM to write vMPM into HW */
> +	ret = mbox_send_message(priv->mbox_chan, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int mpm_pd_power_off(struct generic_pm_domain *genpd)
> +{
> +	struct qcom_mpm_priv *priv = container_of(genpd, struct qcom_mpm_priv,
> +						  genpd);
> +
> +	return qcom_mpm_enter_sleep(priv);
> +}

Merge these two functions:

@@ -297,8 +294,10 @@ static irqreturn_t qcom_mpm_handler(int irq, void *dev_id)
 	return ret;
 }
 
-static int qcom_mpm_enter_sleep(struct qcom_mpm_priv *priv)
+static int mpm_pd_power_off(struct generic_pm_domain *genpd)
 {
+	struct qcom_mpm_priv *priv = container_of(genpd, struct qcom_mpm_priv,
+						  genpd);
 	int i, ret;
 
 	for (i = 0; i < priv->reg_stride; i++)
@@ -312,14 +311,6 @@ static int qcom_mpm_enter_sleep(struct qcom_mpm_priv *priv)
 	return 0;
 }
 
-static int mpm_pd_power_off(struct generic_pm_domain *genpd)
-{
-	struct qcom_mpm_priv *priv = container_of(genpd, struct qcom_mpm_priv,
-						  genpd);
-
-	return qcom_mpm_enter_sleep(priv);
-}
-
 static int qcom_mpm_init(struct device_node *np, struct device_node *parent)
 {
 	struct platform_device *pdev = of_find_device_by_node(np);


Thanks,

	M.

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

Re: [PATCH v7 2/2] irqchip: Add Qualcomm MPM controller driver

[Shawn Guo]

Hi Marc,

On Tue, Mar 01, 2022 at 11:13:30AM +0000, Marc Zyngier wrote:
> Hi Shawn,
> 
> On Tue, 01 Mar 2022 06:24:14 +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 is retrieved from device tree, while
> >   the latter is defined in TLMM pinctrl driver.
> > 
> > - qcom_mpm_enter_sleep() is called from power domain (PD) .power_off
> >   hook to notify RPM that APSS is about to power collapse.  This
> >   requires MPM PD be the parent PD of CPU cluster.
> 
> Glad that you fixed that one. I guess that for RPM, this is too late
> and that we're stuck with the existing stuff?

I guess you are talking about rpmh_rsc_cpu_pm_callback() in rpmh-rsc
driver?  If so, yes, if we want to maintain the DTB compatibility, the
existing code needs to be there anyway, since power domain approach
requires a dt-bindings change.

> 
> > 
> > - 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/irq-qcom-mpm.c | 448 +++++++++++++++++++++++++++++++++
> >  3 files changed, 457 insertions(+)
> >  create mode 100644 drivers/irqchip/irq-qcom-mpm.c
> > 
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index 7038957f4a77..680d2fcf2686 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -430,6 +430,14 @@ config QCOM_PDC
> >  	  Power Domain Controller driver to manage and configure wakeup
> >  	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
> >  
> > +config QCOM_MPM
> > +	tristate "QCOM MPM"
> > +	depends on ARCH_QCOM
> > +	select IRQ_DOMAIN_HIERARCHY
> > +	help
> > +	  MSM Power Manager driver to manage and configure wakeup
> > +	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
> > +
> >  config CSKY_MPINTC
> >  	bool
> >  	depends on CSKY
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index c1f611cbfbf8..1f8990f812f1 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -94,6 +94,7 @@ obj-$(CONFIG_MESON_IRQ_GPIO)		+= irq-meson-gpio.o
> >  obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
> >  obj-$(CONFIG_NDS32)			+= irq-ativic32.o
> >  obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
> > +obj-$(CONFIG_QCOM_MPM)			+= irq-qcom-mpm.o
> >  obj-$(CONFIG_CSKY_MPINTC)		+= irq-csky-mpintc.o
> >  obj-$(CONFIG_CSKY_APB_INTC)		+= irq-csky-apb-intc.o
> >  obj-$(CONFIG_RISCV_INTC)		+= irq-riscv-intc.o
> > diff --git a/drivers/irqchip/irq-qcom-mpm.c b/drivers/irqchip/irq-qcom-mpm.c
> > new file mode 100644
> > index 000000000000..f4409c169a3a
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-qcom-mpm.c
> > @@ -0,0 +1,448 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2021, Linaro Limited
> > + * Copyright (c) 2010-2020, The Linux Foundation. All rights reserved.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/irqchip.h>
> > +#include <linux/irqchip/arm-gic-v3.h>
> 
> Please drop this. Nobody apart from the GIC drivers themselves, KVM
> (which implements a GIC), and the arch code (which deals with
> interrupt priorities) should ever include it.

Indeed!  It's not needed here.

> 
> > +#include <linux/irqdomain.h>
> > +#include <linux/mailbox_client.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_domain.h>
> > +#include <linux/slab.h>
> > +#include <linux/soc/qcom/irq.h>
> > +#include <linux/spinlock.h>
> > +
> > +/*
> > + * This is the driver for Qualcomm MPM (MSM Power Manager) interrupt controller,
> > + * which is commonly found on Qualcomm SoCs built on the RPM architecture.
> > + * Sitting in always-on domain, MPM monitors the wakeup interrupts when SoC is
> > + * asleep, and wakes up the AP when one of those interrupts occurs.  This driver
> > + * doesn't directly access physical MPM registers though.  Instead, the access
> > + * is bridged via a piece of internal memory (SRAM) that is accessible to both
> > + * AP and RPM.  This piece of memory is called 'vMPM' in the driver.
> > + *
> > + * When SoC is awake, the vMPM is owned by AP and the register setup by this
> > + * driver all happens on vMPM.  When AP is about to get power collapsed, the
> > + * driver sends a mailbox notification to RPM, which will take over the vMPM
> > + * ownership and dump vMPM into physical MPM registers.  On wakeup, AP is woken
> > + * up by a MPM pin/interrupt, and RPM will copy STATUS registers into vMPM.
> > + * Then AP start owning vMPM again.
> > + *
> > + * vMPM register map:
> > + *
> > + *    31                              0
> > + *    +--------------------------------+
> > + *    |            TIMER0              | 0x00
> > + *    +--------------------------------+
> > + *    |            TIMER1              | 0x04
> > + *    +--------------------------------+
> > + *    |            ENABLE0             | 0x08
> > + *    +--------------------------------+
> > + *    |              ...               | ...
> > + *    +--------------------------------+
> > + *    |            ENABLEn             |
> > + *    +--------------------------------+
> > + *    |          FALLING_EDGE0         |
> > + *    +--------------------------------+
> > + *    |              ...               |
> > + *    +--------------------------------+
> > + *    |            STATUSn             |
> > + *    +--------------------------------+
> > + *
> > + *    n = DIV_ROUND_UP(pin_cnt, 32)
> > + *
> > + */
> > +
> > +#define MPM_REG_ENABLE		0
> > +#define MPM_REG_FALLING_EDGE	1
> > +#define MPM_REG_RISING_EDGE	2
> > +#define MPM_REG_POLARITY	3
> > +#define MPM_REG_STATUS		4
> > +
> > +/* MPM pin map to GIC hwirq */
> > +struct mpm_gic_map {
> > +	int pin;
> > +	irq_hw_number_t hwirq;
> > +};
> > +
> > +struct qcom_mpm_priv {
> > +	void __iomem *base;
> > +	raw_spinlock_t lock;
> > +	struct mbox_client mbox_client;
> > +	struct mbox_chan *mbox_chan;
> > +	struct mpm_gic_map *maps;
> > +	unsigned int map_cnt;
> > +	unsigned int reg_stride;
> > +	struct irq_domain *domain;
> > +	struct generic_pm_domain genpd;
> > +};
> > +
> > +static u32 qcom_mpm_read(struct qcom_mpm_priv *priv, unsigned int reg,
> > +			 unsigned int index)
> > +{
> > +	unsigned int offset = (reg * priv->reg_stride + index + 2) * 4;
> > +
> > +	return readl_relaxed(priv->base + offset);
> > +}
> > +
> > +static void qcom_mpm_write(struct qcom_mpm_priv *priv, unsigned int reg,
> > +			   unsigned int index, u32 val)
> > +{
> > +	unsigned int offset = (reg * priv->reg_stride + index + 2) * 4;
> > +
> > +	writel_relaxed(val, priv->base + offset);
> > +
> > +	/* Ensure the write is completed */
> > +	wmb();
> > +}
> > +
> > +static void qcom_mpm_enable_irq(struct irq_data *d, bool en)
> > +{
> > +	struct qcom_mpm_priv *priv = d->chip_data;
> > +	int pin = d->hwirq;
> > +	unsigned int index = pin / 32;
> > +	unsigned int shift = pin % 32;
> > +	unsigned long flags;
> > +	u32 val;
> > +
> > +	raw_spin_lock_irqsave(&priv->lock, flags);
> > +
> > +	val = qcom_mpm_read(priv, MPM_REG_ENABLE, index);
> > +	if (en)
> > +		val |= BIT(shift);
> > +	else
> > +		val &= ~BIT(shift);
> > +	qcom_mpm_write(priv, MPM_REG_ENABLE, index, val);
> > +
> > +	raw_spin_unlock_irqrestore(&priv->lock, flags);
> 
> __assign_bit() is what you want:

OK, thanks!

> 
> @@ -112,16 +111,12 @@ static void qcom_mpm_enable_irq(struct irq_data *d, bool en)
>  	int pin = d->hwirq;
>  	unsigned int index = pin / 32;
>  	unsigned int shift = pin % 32;
> -	unsigned long flags;
> -	u32 val;
> +	unsigned long flags, val;
>  
>  	raw_spin_lock_irqsave(&priv->lock, flags);
>  
>  	val = qcom_mpm_read(priv, MPM_REG_ENABLE, index);
> -	if (en)
> -		val |= BIT(shift);
> -	else
> -		val &= ~BIT(shift);
> +	__assign_bit(shift, &val, en);
>  	qcom_mpm_write(priv, MPM_REG_ENABLE, index, val);
>  
>  	raw_spin_unlock_irqrestore(&priv->lock, flags);
> 
> > +}
> > +
> > +static void qcom_mpm_mask(struct irq_data *d)
> > +{
> > +	qcom_mpm_enable_irq(d, false);
> > +
> > +	if (d->parent_data)
> > +		irq_chip_mask_parent(d);
> > +}
> > +
> > +static void qcom_mpm_unmask(struct irq_data *d)
> > +{
> > +	qcom_mpm_enable_irq(d, true);
> > +
> > +	if (d->parent_data)
> > +		irq_chip_unmask_parent(d);
> > +}
> > +
> > +static void mpm_set_type(struct qcom_mpm_priv *priv, bool set, unsigned int reg,
> > +			 unsigned int index, unsigned int shift)
> > +{
> > +	unsigned long flags;
> > +	u32 val;
> > +
> > +	raw_spin_lock_irqsave(&priv->lock, flags);
> > +
> > +	val = qcom_mpm_read(priv, reg, index);
> > +	if (set)
> > +		val |= BIT(shift);
> > +	else
> > +		val &= ~BIT(shift);
> > +	qcom_mpm_write(priv, reg, index, val);
> > +
> > +	raw_spin_unlock_irqrestore(&priv->lock, flags);
> 
> Same thing:
> 
> @@ -146,16 +141,12 @@ static void qcom_mpm_unmask(struct irq_data *d)
>  static void mpm_set_type(struct qcom_mpm_priv *priv, bool set, unsigned int reg,
>  			 unsigned int index, unsigned int shift)
>  {
> -	unsigned long flags;
> -	u32 val;
> +	unsigned long flags, val;
>  
>  	raw_spin_lock_irqsave(&priv->lock, flags);
>  
>  	val = qcom_mpm_read(priv, reg, index);
> -	if (set)
> -		val |= BIT(shift);
> -	else
> -		val &= ~BIT(shift);
> +	__assign_bit(shift, &val, set);
>  	qcom_mpm_write(priv, reg, index, val);
>  
>  	raw_spin_unlock_irqrestore(&priv->lock, flags);
> 
> > +}
> > +
> > +static int qcom_mpm_set_type(struct irq_data *d, unsigned int type)
> > +{
> > +	struct qcom_mpm_priv *priv = d->chip_data;
> > +	int pin = d->hwirq;
> > +	unsigned int index = pin / 32;
> > +	unsigned int shift = pin % 32;
> > +
> > +	switch (type & IRQ_TYPE_SENSE_MASK) {
> > +	case IRQ_TYPE_EDGE_RISING:
> > +		mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_RISING),
> > +			     MPM_REG_RISING_EDGE, index, shift);
> > +		break;
> > +	case IRQ_TYPE_EDGE_FALLING:
> > +		mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_FALLING),
> > +			     MPM_REG_FALLING_EDGE, index, shift);
> > +		break;
> > +	case IRQ_TYPE_LEVEL_HIGH:
> > +		mpm_set_type(priv, !!(type & IRQ_TYPE_LEVEL_HIGH),
> > +			     MPM_REG_POLARITY, index, shift);
> > +		break;
> > +	}
> 
> All these '!!(type & BLAH)' are totally superfluous, as they all expand
> to 'true' by construction.

Yes, you are right!

> And this leads to a few questions:
> 
> - Shouldn't a rising interrupt clear the falling detection?
> - Shouldn't a level-low clear the polarity?
> - How do you handle IRQ_TYPE_EDGE_BOTH?
> - How is MPM_REG_POLARITY evaluated for edge interrupts (resp the EDGE
>   registers for level interrupts), as you never seem to be configuring
>   a type here?

Honestly, qcom_mpm_set_type() was mostly taken from downstream without
too much thinking.  I trusted it as a "good" reference as I have no
document to verify the code.  These questions are great and resulted the
code changes are pretty sensible to me.

> - What initialises the MPM trigger types at boot time?

I dumped the vMPM region and it's all zeros.  My understanding is if
vMPM needs any sort of initialization, it should be done by RPM firmware
before APSS gets booting.

> 
> I came up with this, but that's probably not enough (boot time init is
> definitely missing):

Thanks much!

> 
> @@ -170,16 +161,22 @@ static int qcom_mpm_set_type(struct irq_data *d, unsigned int type)
>  
>  	switch (type & IRQ_TYPE_SENSE_MASK) {
>  	case IRQ_TYPE_EDGE_RISING:
> -		mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_RISING),
> -			     MPM_REG_RISING_EDGE, index, shift);
> +		mpm_set_type(priv, true, MPM_REG_RISING_EDGE, index, shift);
> +		mpm_set_type(priv, false, MPM_REG_FALLING_EDGE, index, shift);
>  		break;
>  	case IRQ_TYPE_EDGE_FALLING:
> -		mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_FALLING),
> -			     MPM_REG_FALLING_EDGE, index, shift);
> +		mpm_set_type(priv, false, MPM_REG_RISING_EDGE, index, shift);
> +		mpm_set_type(priv, true, MPM_REG_FALLING_EDGE, index, shift);
> +		break;
> +	case IRQ_TYPE_EDGE_BOTH:
> +		mpm_set_type(priv, true, MPM_REG_RISING_EDGE, index, shift);
> +		mpm_set_type(priv, true, MPM_REG_FALLING_EDGE, index, shift);
>  		break;
>  	case IRQ_TYPE_LEVEL_HIGH:
> -		mpm_set_type(priv, !!(type & IRQ_TYPE_LEVEL_HIGH),
> -			     MPM_REG_POLARITY, index, shift);
> +		mpm_set_type(priv, true, MPM_REG_POLARITY, index, shift);
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		mpm_set_type(priv, false, MPM_REG_POLARITY, index, shift);
>  		break;
>  	}
> 
> > +
> > +	if (!d->parent_data)
> > +		return 0;
> > +
> > +	if (type & IRQ_TYPE_EDGE_BOTH)
> > +		type = IRQ_TYPE_EDGE_RISING;
> > +
> > +	if (type & IRQ_TYPE_LEVEL_MASK)
> > +		type = IRQ_TYPE_LEVEL_HIGH;
> > +
> > +	return irq_chip_set_type_parent(d, type);
> > +}
> > +
> > +static struct irq_chip qcom_mpm_chip = {
> > +	.name			= "mpm",
> > +	.irq_eoi		= irq_chip_eoi_parent,
> > +	.irq_mask		= qcom_mpm_mask,
> > +	.irq_unmask		= qcom_mpm_unmask,
> > +	.irq_retrigger		= irq_chip_retrigger_hierarchy,
> > +	.irq_set_type		= qcom_mpm_set_type,
> > +	.irq_set_affinity	= irq_chip_set_affinity_parent,
> > +	.flags			= IRQCHIP_MASK_ON_SUSPEND |
> > +				  IRQCHIP_SKIP_SET_WAKE,
> > +};
> > +
> > +static struct mpm_gic_map *get_mpm_gic_map(struct qcom_mpm_priv *priv, int pin)
> > +{
> > +	struct mpm_gic_map *maps = priv->maps;
> > +	int i;
> > +
> > +	for (i = 0; i < priv->map_cnt; i++) {
> > +		if (maps[i].pin == pin)
> > +			return &maps[i];
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +static int qcom_mpm_alloc(struct irq_domain *domain, unsigned int virq,
> > +			  unsigned int nr_irqs, void *data)
> > +{
> > +	struct qcom_mpm_priv *priv = domain->host_data;
> > +	struct irq_fwspec *fwspec = data;
> > +	struct irq_fwspec parent_fwspec;
> > +	struct mpm_gic_map *map;
> > +	irq_hw_number_t hwirq;
> > +	unsigned int type;
> > +	int  ret;
> > +
> > +	ret = irq_domain_translate_twocell(domain, fwspec, &hwirq, &type);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> > +					    &qcom_mpm_chip, priv);
> > +	if (ret)
> > +		return ret;
> > +
> > +	map = get_mpm_gic_map(priv, hwirq);
> > +	if (map == NULL)
> > +		return irq_domain_disconnect_hierarchy(domain->parent, virq);
> > +
> > +	if (type & IRQ_TYPE_EDGE_BOTH)
> > +		type = IRQ_TYPE_EDGE_RISING;
> > +
> > +	if (type & IRQ_TYPE_LEVEL_MASK)
> > +		type = IRQ_TYPE_LEVEL_HIGH;
> > +
> > +	parent_fwspec.fwnode = domain->parent->fwnode;
> > +	parent_fwspec.param_count = 3;
> > +	parent_fwspec.param[0] = 0;
> > +	parent_fwspec.param[1] = map->hwirq;
> > +	parent_fwspec.param[2] = type;
> > +
> > +	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
> > +					    &parent_fwspec);
> > +}
> > +
> > +static const struct irq_domain_ops qcom_mpm_ops = {
> > +	.alloc		= qcom_mpm_alloc,
> > +	.free		= irq_domain_free_irqs_common,
> > +	.translate	= irq_domain_translate_twocell,
> > +};
> > +
> > +/* Triggered by RPM when system resumes from deep sleep */
> > +static irqreturn_t qcom_mpm_handler(int irq, void *dev_id)
> > +{
> > +	struct qcom_mpm_priv *priv = dev_id;
> > +	unsigned long enable, pending;
> > +	irqreturn_t ret = IRQ_NONE;
> > +	unsigned long flags;
> > +	int i, j;
> > +
> > +	for (i = 0; i < priv->reg_stride; i++) {
> > +		raw_spin_lock_irqsave(&priv->lock, flags);
> > +		enable = qcom_mpm_read(priv, MPM_REG_ENABLE, i);
> > +		pending = qcom_mpm_read(priv, MPM_REG_STATUS, i);
> > +		pending &= enable;
> > +		raw_spin_unlock_irqrestore(&priv->lock, flags);
> > +
> > +		for_each_set_bit(j, &pending, 32) {
> > +			unsigned int pin = 32 * i + j;
> > +			struct irq_desc *desc = irq_resolve_mapping(priv->domain, pin);
> > +			struct irq_data *d = &desc->irq_data;
> > +
> > +			if (!irqd_is_level_type(d))
> > +				irq_set_irqchip_state(d->irq,
> > +						IRQCHIP_STATE_PENDING, true);
> > +			ret = IRQ_HANDLED;
> > +		}
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int qcom_mpm_enter_sleep(struct qcom_mpm_priv *priv)
> > +{
> > +	int i, ret;
> > +
> > +	for (i = 0; i < priv->reg_stride; i++)
> > +		qcom_mpm_write(priv, MPM_REG_STATUS, i, 0);
> > +
> > +	/* Notify RPM to write vMPM into HW */
> > +	ret = mbox_send_message(priv->mbox_chan, NULL);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mpm_pd_power_off(struct generic_pm_domain *genpd)
> > +{
> > +	struct qcom_mpm_priv *priv = container_of(genpd, struct qcom_mpm_priv,
> > +						  genpd);
> > +
> > +	return qcom_mpm_enter_sleep(priv);
> > +}
> 
> Merge these two functions:

OK.  Thanks for all these code changes!

Shawn

> 
> @@ -297,8 +294,10 @@ static irqreturn_t qcom_mpm_handler(int irq, void *dev_id)
>  	return ret;
>  }
>  
> -static int qcom_mpm_enter_sleep(struct qcom_mpm_priv *priv)
> +static int mpm_pd_power_off(struct generic_pm_domain *genpd)
>  {
> +	struct qcom_mpm_priv *priv = container_of(genpd, struct qcom_mpm_priv,
> +						  genpd);
>  	int i, ret;
>  
>  	for (i = 0; i < priv->reg_stride; i++)
> @@ -312,14 +311,6 @@ static int qcom_mpm_enter_sleep(struct qcom_mpm_priv *priv)
>  	return 0;
>  }
>  
> -static int mpm_pd_power_off(struct generic_pm_domain *genpd)
> -{
> -	struct qcom_mpm_priv *priv = container_of(genpd, struct qcom_mpm_priv,
> -						  genpd);
> -
> -	return qcom_mpm_enter_sleep(priv);
> -}
> -
>  static int qcom_mpm_init(struct device_node *np, struct device_node *parent)
>  {
>  	struct platform_device *pdev = of_find_device_by_node(np);

Re: [PATCH v7 2/2] irqchip: Add Qualcomm MPM controller driver

[Marc Zyngier]

On Wed, 02 Mar 2022 08:40:28 +0000,
Shawn Guo <shawn.guo@linaro.org> wrote:
> 
> Hi Marc,
> 
> On Tue, Mar 01, 2022 at 11:13:30AM +0000, Marc Zyngier wrote:
> > Hi Shawn,

[...]

> > 
> > > +static int qcom_mpm_set_type(struct irq_data *d, unsigned int type)
> > > +{
> > > +	struct qcom_mpm_priv *priv = d->chip_data;
> > > +	int pin = d->hwirq;
> > > +	unsigned int index = pin / 32;
> > > +	unsigned int shift = pin % 32;
> > > +
> > > +	switch (type & IRQ_TYPE_SENSE_MASK) {
> > > +	case IRQ_TYPE_EDGE_RISING:
> > > +		mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_RISING),
> > > +			     MPM_REG_RISING_EDGE, index, shift);
> > > +		break;
> > > +	case IRQ_TYPE_EDGE_FALLING:
> > > +		mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_FALLING),
> > > +			     MPM_REG_FALLING_EDGE, index, shift);
> > > +		break;
> > > +	case IRQ_TYPE_LEVEL_HIGH:
> > > +		mpm_set_type(priv, !!(type & IRQ_TYPE_LEVEL_HIGH),
> > > +			     MPM_REG_POLARITY, index, shift);
> > > +		break;
> > > +	}
> > 
> > All these '!!(type & BLAH)' are totally superfluous, as they all expand
> > to 'true' by construction.
> 
> Yes, you are right!
> 
> > And this leads to a few questions:
> > 
> > - Shouldn't a rising interrupt clear the falling detection?
> > - Shouldn't a level-low clear the polarity?
> > - How do you handle IRQ_TYPE_EDGE_BOTH?
> > - How is MPM_REG_POLARITY evaluated for edge interrupts (resp the EDGE
> >   registers for level interrupts), as you never seem to be configuring
> >   a type here?
> 
> Honestly, qcom_mpm_set_type() was mostly taken from downstream without
> too much thinking.  I trusted it as a "good" reference as I have no
> document to verify the code.  These questions are great and resulted the
> code changes are pretty sensible to me.

I don't think these changes are enough. For example, an interrupt
being switched from level to edge is likely to misbehave (how do you
distinguish the two?). If that's what the downstream driver does, then
it is terminally broken.

As I asked before, we need some actual specs, or at least someone to
paraphrase it for us. There are a number of QC folks on Cc, and I
expect them to chime in and explain how MPM works here.

> 
> > - What initialises the MPM trigger types at boot time?
> 
> I dumped the vMPM region and it's all zeros.  My understanding is if
> vMPM needs any sort of initialization, it should be done by RPM firmware
> before APSS gets booting.

What about kexec? We can't rely on this memory region to always be
0-initialised, nor do we know what that means.

Thanks,

	M.

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

Re: [PATCH v7 2/2] irqchip: Add Qualcomm MPM controller driver

[Shawn Guo]

On Wed, Mar 02, 2022 at 10:25:45AM +0000, Marc Zyngier wrote:
> On Wed, 02 Mar 2022 08:40:28 +0000,
> Shawn Guo <shawn.guo@linaro.org> wrote:
> > 
> > Hi Marc,
> > 
> > On Tue, Mar 01, 2022 at 11:13:30AM +0000, Marc Zyngier wrote:
> > > Hi Shawn,
> 
> [...]
> 
> > > 
> > > > +static int qcom_mpm_set_type(struct irq_data *d, unsigned int type)
> > > > +{
> > > > +	struct qcom_mpm_priv *priv = d->chip_data;
> > > > +	int pin = d->hwirq;
> > > > +	unsigned int index = pin / 32;
> > > > +	unsigned int shift = pin % 32;
> > > > +
> > > > +	switch (type & IRQ_TYPE_SENSE_MASK) {
> > > > +	case IRQ_TYPE_EDGE_RISING:
> > > > +		mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_RISING),
> > > > +			     MPM_REG_RISING_EDGE, index, shift);
> > > > +		break;
> > > > +	case IRQ_TYPE_EDGE_FALLING:
> > > > +		mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_FALLING),
> > > > +			     MPM_REG_FALLING_EDGE, index, shift);
> > > > +		break;
> > > > +	case IRQ_TYPE_LEVEL_HIGH:
> > > > +		mpm_set_type(priv, !!(type & IRQ_TYPE_LEVEL_HIGH),
> > > > +			     MPM_REG_POLARITY, index, shift);
> > > > +		break;
> > > > +	}
> > > 
> > > All these '!!(type & BLAH)' are totally superfluous, as they all expand
> > > to 'true' by construction.
> > 
> > Yes, you are right!
> > 
> > > And this leads to a few questions:
> > > 
> > > - Shouldn't a rising interrupt clear the falling detection?
> > > - Shouldn't a level-low clear the polarity?
> > > - How do you handle IRQ_TYPE_EDGE_BOTH?
> > > - How is MPM_REG_POLARITY evaluated for edge interrupts (resp the EDGE
> > >   registers for level interrupts), as you never seem to be configuring
> > >   a type here?
> > 
> > Honestly, qcom_mpm_set_type() was mostly taken from downstream without
> > too much thinking.

I have to take this statement back.  It seems that the current code has
been diverted from the downstream in a wrong way.

> > I trusted it as a "good" reference as I have no
> > document to verify the code.  These questions are great and resulted the
> > code changes are pretty sensible to me.
> 
> I don't think these changes are enough. For example, an interrupt
> being switched from level to edge is likely to misbehave (how do you
> distinguish the two?). If that's what the downstream driver does, then
> it is terminally broken.

Could you take a look at downstream code and see if it answers all your
questions?

It seems MPM_REG_POLARITY is only meant for level interrupts, since edge
interrupts already have separate registers for rising and falling.

I will fix my broken code by respecting the downstream logic.

> As I asked before, we need some actual specs, or at least someone to
> paraphrase it for us. There are a number of QC folks on Cc, and I
> expect them to chime in and explain how MPM works here.
> 
> > 
> > > - What initialises the MPM trigger types at boot time?
> > 
> > I dumped the vMPM region and it's all zeros.  My understanding is if
> > vMPM needs any sort of initialization, it should be done by RPM firmware
> > before APSS gets booting.
> 
> What about kexec? We can't rely on this memory region to always be
> 0-initialised, nor do we know what that means.

We are not relying on it being 0-initialised, but being initialised by
RPM with initial physical MPM register values.

Shawn

[1] https://source.codeaurora.org/quic/la/kernel/msm-5.4/tree/drivers/irqchip/qcom-mpm.c/?h=LE.UM.6.2.4.r1#n187

Re: [PATCH v7 2/2] irqchip: Add Qualcomm MPM controller driver

[Marc Zyngier]

On Wed, 02 Mar 2022 13:34:41 +0000,
Shawn Guo <shawn.guo@linaro.org> wrote:
> 
> On Wed, Mar 02, 2022 at 10:25:45AM +0000, Marc Zyngier wrote:
> > On Wed, 02 Mar 2022 08:40:28 +0000,
> > Shawn Guo <shawn.guo@linaro.org> wrote:
> > > 
> > > Hi Marc,
> > > 
> > > On Tue, Mar 01, 2022 at 11:13:30AM +0000, Marc Zyngier wrote:
> > > > Hi Shawn,
> > 
> > [...]
> > 
> > > > 
> > > > > +static int qcom_mpm_set_type(struct irq_data *d, unsigned int type)
> > > > > +{
> > > > > +	struct qcom_mpm_priv *priv = d->chip_data;
> > > > > +	int pin = d->hwirq;
> > > > > +	unsigned int index = pin / 32;
> > > > > +	unsigned int shift = pin % 32;
> > > > > +
> > > > > +	switch (type & IRQ_TYPE_SENSE_MASK) {
> > > > > +	case IRQ_TYPE_EDGE_RISING:
> > > > > +		mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_RISING),
> > > > > +			     MPM_REG_RISING_EDGE, index, shift);
> > > > > +		break;
> > > > > +	case IRQ_TYPE_EDGE_FALLING:
> > > > > +		mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_FALLING),
> > > > > +			     MPM_REG_FALLING_EDGE, index, shift);
> > > > > +		break;
> > > > > +	case IRQ_TYPE_LEVEL_HIGH:
> > > > > +		mpm_set_type(priv, !!(type & IRQ_TYPE_LEVEL_HIGH),
> > > > > +			     MPM_REG_POLARITY, index, shift);
> > > > > +		break;
> > > > > +	}
> > > > 
> > > > All these '!!(type & BLAH)' are totally superfluous, as they all expand
> > > > to 'true' by construction.
> > > 
> > > Yes, you are right!
> > > 
> > > > And this leads to a few questions:
> > > > 
> > > > - Shouldn't a rising interrupt clear the falling detection?
> > > > - Shouldn't a level-low clear the polarity?
> > > > - How do you handle IRQ_TYPE_EDGE_BOTH?
> > > > - How is MPM_REG_POLARITY evaluated for edge interrupts (resp the EDGE
> > > >   registers for level interrupts), as you never seem to be configuring
> > > >   a type here?
> > > 
> > > Honestly, qcom_mpm_set_type() was mostly taken from downstream without
> > > too much thinking.
> 
> I have to take this statement back.  It seems that the current code has
> been diverted from the downstream in a wrong way.
> 
> > > I trusted it as a "good" reference as I have no
> > > document to verify the code.  These questions are great and resulted the
> > > code changes are pretty sensible to me.
> > 
> > I don't think these changes are enough. For example, an interrupt
> > being switched from level to edge is likely to misbehave (how do you
> > distinguish the two?). If that's what the downstream driver does, then
> > it is terminally broken.
> 
> Could you take a look at downstream code and see if it answers all your
> questions?

This code actually makes me ask more questions. Why is it programming
2 'pins' for each IRQ?

> 
> It seems MPM_REG_POLARITY is only meant for level interrupts, since edge
> interrupts already have separate registers for rising and falling.

Then level interrupts must clear both the edge registers at all times.

> 
> I will fix my broken code by respecting the downstream logic.
> 
> > As I asked before, we need some actual specs, or at least someone to
> > paraphrase it for us. There are a number of QC folks on Cc, and I
> > expect them to chime in and explain how MPM works here.
> > 
> > > 
> > > > - What initialises the MPM trigger types at boot time?
> > > 
> > > I dumped the vMPM region and it's all zeros.  My understanding is if
> > > vMPM needs any sort of initialization, it should be done by RPM firmware
> > > before APSS gets booting.
> > 
> > What about kexec? We can't rely on this memory region to always be
> > 0-initialised, nor do we know what that means.
> 
> We are not relying on it being 0-initialised, but being initialised by
> RPM with initial physical MPM register values.

Whatever. It simply cannot be trusted. If you kexec another kernel,
you need to be able to restore a sane state at probe time. This isn't
optional.

	M.

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

Re: [PATCH v7 2/2] irqchip: Add Qualcomm MPM controller driver

[Shawn Guo]

On Wed, Mar 02, 2022 at 01:57:27PM +0000, Marc Zyngier wrote:
> On Wed, 02 Mar 2022 13:34:41 +0000,
> Shawn Guo <shawn.guo@linaro.org> wrote:
> > 
> > On Wed, Mar 02, 2022 at 10:25:45AM +0000, Marc Zyngier wrote:
> > > On Wed, 02 Mar 2022 08:40:28 +0000,
> > > Shawn Guo <shawn.guo@linaro.org> wrote:
> > > > 
> > > > Hi Marc,
> > > > 
> > > > On Tue, Mar 01, 2022 at 11:13:30AM +0000, Marc Zyngier wrote:
> > > > > Hi Shawn,
> > > 
> > > [...]
> > > 
> > > > > 
> > > > > > +static int qcom_mpm_set_type(struct irq_data *d, unsigned int type)
> > > > > > +{
> > > > > > +	struct qcom_mpm_priv *priv = d->chip_data;
> > > > > > +	int pin = d->hwirq;
> > > > > > +	unsigned int index = pin / 32;
> > > > > > +	unsigned int shift = pin % 32;
> > > > > > +
> > > > > > +	switch (type & IRQ_TYPE_SENSE_MASK) {
> > > > > > +	case IRQ_TYPE_EDGE_RISING:
> > > > > > +		mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_RISING),
> > > > > > +			     MPM_REG_RISING_EDGE, index, shift);
> > > > > > +		break;
> > > > > > +	case IRQ_TYPE_EDGE_FALLING:
> > > > > > +		mpm_set_type(priv, !!(type & IRQ_TYPE_EDGE_FALLING),
> > > > > > +			     MPM_REG_FALLING_EDGE, index, shift);
> > > > > > +		break;
> > > > > > +	case IRQ_TYPE_LEVEL_HIGH:
> > > > > > +		mpm_set_type(priv, !!(type & IRQ_TYPE_LEVEL_HIGH),
> > > > > > +			     MPM_REG_POLARITY, index, shift);
> > > > > > +		break;
> > > > > > +	}
> > > > > 
> > > > > All these '!!(type & BLAH)' are totally superfluous, as they all expand
> > > > > to 'true' by construction.
> > > > 
> > > > Yes, you are right!
> > > > 
> > > > > And this leads to a few questions:
> > > > > 
> > > > > - Shouldn't a rising interrupt clear the falling detection?
> > > > > - Shouldn't a level-low clear the polarity?
> > > > > - How do you handle IRQ_TYPE_EDGE_BOTH?
> > > > > - How is MPM_REG_POLARITY evaluated for edge interrupts (resp the EDGE
> > > > >   registers for level interrupts), as you never seem to be configuring
> > > > >   a type here?
> > > > 
> > > > Honestly, qcom_mpm_set_type() was mostly taken from downstream without
> > > > too much thinking.
> > 
> > I have to take this statement back.  It seems that the current code has
> > been diverted from the downstream in a wrong way.
> > 
> > > > I trusted it as a "good" reference as I have no
> > > > document to verify the code.  These questions are great and resulted the
> > > > code changes are pretty sensible to me.
> > > 
> > > I don't think these changes are enough. For example, an interrupt
> > > being switched from level to edge is likely to misbehave (how do you
> > > distinguish the two?). If that's what the downstream driver does, then
> > > it is terminally broken.
> > 
> > Could you take a look at downstream code and see if it answers all your
> > questions?
> 
> This code actually makes me ask more questions. Why is it programming
> 2 'pins' for each IRQ?

The mapping between MPM pin and GIC IRQ is not strictly 1-1.  There are
some rare case that up to 2 MPM pins map to a single GIC IRQ, for
example the last two in QC2290 'qcom,mpm-pin-map' below.

	qcom,mpm-pin-map = <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 */


The downstream uses a DT bindings that specifies GIC hwirq number in
client device nodes.  In that case, d->hwirq in the driver is GIC IRQ
number, and the driver will need to query mapping table, find out the
possible 2 MPM pins, and set them up.

The patches I'm posting here use a different bindings that specifies MPM
pin instead in client device nodes.  Thus the driver can simply get the
MPM pin from d->hwirq, so that the whole look-up procedure can be saved.

> 
> > 
> > It seems MPM_REG_POLARITY is only meant for level interrupts, since edge
> > interrupts already have separate registers for rising and falling.
> 
> Then level interrupts must clear both the edge registers at all times.

The downstream logic already covers that, right?  The edge register bits
will be cleared as long as 'flowtype' is not EDGE.

static void msm_mpm_set_type(struct irq_data *d,
                                        unsigned int flowtype, bool is_mpmgic)
{   
        int mpm_pin[MAX_MPM_PIN_PER_IRQ] = {-1, -1};
        unsigned long flags;  
        int i = 0;
        unsigned int index, mask;      
        unsigned int reg = 0; 
    
        msm_get_mpm_pin(d, mpm_pin, is_mpmgic);
        for (i = 0; i < MAX_MPM_PIN_PER_IRQ; i++) {
                if (mpm_pin[i] < 0)            
                        return;                        
    
                index = mpm_pin[i]/32;         
                mask = mpm_pin[i]%32;          
    
                spin_lock_irqsave(&mpm_lock, flags);
                reg = MPM_REG_RISING_EDGE;     
                if (flowtype & IRQ_TYPE_EDGE_RISING)
                        msm_mpm_program_set_type(1, reg, index, mask);
                else          
                        msm_mpm_program_set_type(0, reg, index, mask);

                reg = MPM_REG_FALLING_EDGE;    
                if (flowtype & IRQ_TYPE_EDGE_FALLING)
                        msm_mpm_program_set_type(1, reg, index, mask);
                else          
                        msm_mpm_program_set_type(0, reg, index, mask);
    
                reg = MPM_REG_POLARITY;        
                if (flowtype & IRQ_TYPE_LEVEL_HIGH)
                        msm_mpm_program_set_type(1, reg, index, mask);
                else
                        msm_mpm_program_set_type(0, reg, index, mask);
                spin_unlock_irqrestore(&mpm_lock, flags);
        }
}

> > I will fix my broken code by respecting the downstream logic.
> > 
> > > As I asked before, we need some actual specs, or at least someone to
> > > paraphrase it for us. There are a number of QC folks on Cc, and I
> > > expect them to chime in and explain how MPM works here.
> > > 
> > > > 
> > > > > - What initialises the MPM trigger types at boot time?
> > > > 
> > > > I dumped the vMPM region and it's all zeros.  My understanding is if
> > > > vMPM needs any sort of initialization, it should be done by RPM firmware
> > > > before APSS gets booting.
> > > 
> > > What about kexec? We can't rely on this memory region to always be
> > > 0-initialised, nor do we know what that means.
> > 
> > We are not relying on it being 0-initialised, but being initialised by
> > RPM with initial physical MPM register values.
> 
> Whatever. It simply cannot be trusted. If you kexec another kernel,
> you need to be able to restore a sane state at probe time. This isn't
> optional.

Right, I will add an explicit initialization on vMPM region at probe
time.

Shawn

Re: [PATCH v7 2/2] irqchip: Add Qualcomm MPM controller driver

[Marc Zyngier]

On Thu, 03 Mar 2022 04:02:29 +0000,
Shawn Guo <shawn.guo@linaro.org> wrote:
> 
> On Wed, Mar 02, 2022 at 01:57:27PM +0000, Marc Zyngier wrote:
> > This code actually makes me ask more questions. Why is it programming
> > 2 'pins' for each IRQ?
> 
> The mapping between MPM pin and GIC IRQ is not strictly 1-1.  There are
> some rare case that up to 2 MPM pins map to a single GIC IRQ, for
> example the last two in QC2290 'qcom,mpm-pin-map' below.
> 
> 	qcom,mpm-pin-map = <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 */
> 
> 
> The downstream uses a DT bindings that specifies GIC hwirq number in
> client device nodes.  In that case, d->hwirq in the driver is GIC IRQ
> number, and the driver will need to query mapping table, find out the
> possible 2 MPM pins, and set them up.
> 
> The patches I'm posting here use a different bindings that specifies MPM
> pin instead in client device nodes.  Thus the driver can simply get the
> MPM pin from d->hwirq, so that the whole look-up procedure can be saved.

It still remains that there is no 1:1 mapping between input and
output, which is the rule #1 to be able to use a hierarchical setup.

/me puzzled.

>
> > 
> > > 
> > > It seems MPM_REG_POLARITY is only meant for level interrupts, since edge
> > > interrupts already have separate registers for rising and falling.
> > 
> > Then level interrupts must clear both the edge registers at all times.
> 
> The downstream logic already covers that, right?  The edge register bits
> will be cleared as long as 'flowtype' is not EDGE.

I am talking about *your* code, not the Qualcomm stuff.

	M.

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

Re: [PATCH v7 2/2] irqchip: Add Qualcomm MPM controller driver

[Shawn Guo]

On Fri, Mar 04, 2022 at 07:59:15AM +0000, Marc Zyngier wrote:
> On Thu, 03 Mar 2022 04:02:29 +0000,
> Shawn Guo <shawn.guo@linaro.org> wrote:
> > 
> > On Wed, Mar 02, 2022 at 01:57:27PM +0000, Marc Zyngier wrote:
> > > This code actually makes me ask more questions. Why is it programming
> > > 2 'pins' for each IRQ?
> > 
> > The mapping between MPM pin and GIC IRQ is not strictly 1-1.  There are
> > some rare case that up to 2 MPM pins map to a single GIC IRQ, for
> > example the last two in QC2290 'qcom,mpm-pin-map' below.
> > 
> > 	qcom,mpm-pin-map = <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 */
> > 
> > 
> > The downstream uses a DT bindings that specifies GIC hwirq number in
> > client device nodes.  In that case, d->hwirq in the driver is GIC IRQ
> > number, and the driver will need to query mapping table, find out the
> > possible 2 MPM pins, and set them up.
> > 
> > The patches I'm posting here use a different bindings that specifies MPM
> > pin instead in client device nodes.  Thus the driver can simply get the
> > MPM pin from d->hwirq, so that the whole look-up procedure can be saved.
> 
> It still remains that there is no 1:1 mapping between input and
> output, which is the rule #1 to be able to use a hierarchical setup.

For direction of MPM pin -> GIC interrupt, it's a 1:1 mapping, i.e. for
given MPM pin, there is only one GIC interrupt.  And that's the
mapping MPM driver relies on.  For GIC interrupt -> MPM pin, it's not
a strict 1:1 mapping.  For given GIC interrupt, there could be up to 2
MPM pin mapped to it.  MPM driver doesn't use this direction of mapping
though.

> 
> /me puzzled.
> 
> >
> > > 
> > > > 
> > > > It seems MPM_REG_POLARITY is only meant for level interrupts, since edge
> > > > interrupts already have separate registers for rising and falling.
> > > 
> > > Then level interrupts must clear both the edge registers at all times.
> > 
> > The downstream logic already covers that, right?  The edge register bits
> > will be cleared as long as 'flowtype' is not EDGE.
> 
> I am talking about *your* code, not the Qualcomm stuff.

OK. If you do not see anything wrong on the vendor code logic, I plan to
replace my broken code with it.

Shawn

Re: [PATCH v7 2/2] irqchip: Add Qualcomm MPM controller driver

[Marc Zyngier]

On Fri, 04 Mar 2022 08:23:42 +0000,
Shawn Guo <shawn.guo@linaro.org> wrote:
> 
> On Fri, Mar 04, 2022 at 07:59:15AM +0000, Marc Zyngier wrote:
> > On Thu, 03 Mar 2022 04:02:29 +0000,
> > Shawn Guo <shawn.guo@linaro.org> wrote:
> > > 
> > > On Wed, Mar 02, 2022 at 01:57:27PM +0000, Marc Zyngier wrote:
> > > > This code actually makes me ask more questions. Why is it programming
> > > > 2 'pins' for each IRQ?
> > > 
> > > The mapping between MPM pin and GIC IRQ is not strictly 1-1.  There are
> > > some rare case that up to 2 MPM pins map to a single GIC IRQ, for
> > > example the last two in QC2290 'qcom,mpm-pin-map' below.
> > > 
> > > 	qcom,mpm-pin-map = <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 */
> > > 
> > > 
> > > The downstream uses a DT bindings that specifies GIC hwirq number in
> > > client device nodes.  In that case, d->hwirq in the driver is GIC IRQ
> > > number, and the driver will need to query mapping table, find out the
> > > possible 2 MPM pins, and set them up.
> > > 
> > > The patches I'm posting here use a different bindings that specifies MPM
> > > pin instead in client device nodes.  Thus the driver can simply get the
> > > MPM pin from d->hwirq, so that the whole look-up procedure can be saved.
> > 
> > It still remains that there is no 1:1 mapping between input and
> > output, which is the rule #1 to be able to use a hierarchical setup.
> 
> For direction of MPM pin -> GIC interrupt, it's a 1:1 mapping, i.e. for
> given MPM pin, there is only one GIC interrupt.  And that's the
> mapping MPM driver relies on.  For GIC interrupt -> MPM pin, it's not
> a strict 1:1 mapping.

Then this isn't a 1:1 mapping *AT ALL*. The hierarchical setup
mandates that the mapping is a bijective function, and that's exactly
what 1:1 means. There is no such thing a 1:1 in a single
direction. When you take an interrupt, all you see is the GIC
interrupt. How do you know which of the *two* pins interrupted you? Oh
wait, you *can't* know. You end-up never servicing one of the two
interrupts (and I suspect this results in memory corruption if you
tear a hierarchy down).

This HW deals with 2:1 mappings, i.e. it is a mux. So all the time
spent on this driver is totally lost, because you have the wrong
abstraction. And the QC driver is just as bad.

	M.

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

Re: [PATCH v7 2/2] irqchip: Add Qualcomm MPM controller driver

[Shawn Guo]

On Fri, Mar 04, 2022 at 03:24:43PM +0000, Marc Zyngier wrote:
> On Fri, 04 Mar 2022 08:23:42 +0000,
> Shawn Guo <shawn.guo@linaro.org> wrote:
> > 
> > On Fri, Mar 04, 2022 at 07:59:15AM +0000, Marc Zyngier wrote:
> > > On Thu, 03 Mar 2022 04:02:29 +0000,
> > > Shawn Guo <shawn.guo@linaro.org> wrote:
> > > > 
> > > > On Wed, Mar 02, 2022 at 01:57:27PM +0000, Marc Zyngier wrote:
> > > > > This code actually makes me ask more questions. Why is it programming
> > > > > 2 'pins' for each IRQ?
> > > > 
> > > > The mapping between MPM pin and GIC IRQ is not strictly 1-1.  There are
> > > > some rare case that up to 2 MPM pins map to a single GIC IRQ, for
> > > > example the last two in QC2290 'qcom,mpm-pin-map' below.
> > > > 
> > > > 	qcom,mpm-pin-map = <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 */
> > > > 
> > > > 
> > > > The downstream uses a DT bindings that specifies GIC hwirq number in
> > > > client device nodes.  In that case, d->hwirq in the driver is GIC IRQ
> > > > number, and the driver will need to query mapping table, find out the
> > > > possible 2 MPM pins, and set them up.
> > > > 
> > > > The patches I'm posting here use a different bindings that specifies MPM
> > > > pin instead in client device nodes.  Thus the driver can simply get the
> > > > MPM pin from d->hwirq, so that the whole look-up procedure can be saved.
> > > 
> > > It still remains that there is no 1:1 mapping between input and
> > > output, which is the rule #1 to be able to use a hierarchical setup.
> > 
> > For direction of MPM pin -> GIC interrupt, it's a 1:1 mapping, i.e. for
> > given MPM pin, there is only one GIC interrupt.  And that's the
> > mapping MPM driver relies on.  For GIC interrupt -> MPM pin, it's not
> > a strict 1:1 mapping.
> 
> Then this isn't a 1:1 mapping *AT ALL*. The hierarchical setup
> mandates that the mapping is a bijective function, and that's exactly
> what 1:1 means. There is no such thing a 1:1 in a single
> direction. When you take an interrupt, all you see is the GIC
> interrupt. How do you know which of the *two* pins interrupted you? Oh
> wait, you *can't* know. You end-up never servicing one of the two
> interrupts

Yes, you are right!  But that might be a problem only in theory.  I
checked all the Qualcomm platforms I know built on MPM, and found that
the only 2:1 case is USB DP & DM sensing pins.  Since these two pins
will be handled by USB driver with a single interrupt handler, it should
not cause any problem in practice.  That said, the 2:1 mapping is just
a special case specific to USB, and MPM driver can be implemented as if
it's just a 1:1 mapping.

Shawn

> (and I suspect this results in memory corruption if you
> tear a hierarchy down).

Re: [PATCH v7 2/2] irqchip: Add Qualcomm MPM controller driver

[Marc Zyngier]

On Sat, 05 Mar 2022 09:24:20 +0000,
Shawn Guo <shawn.guo@linaro.org> wrote:
> 
> On Fri, Mar 04, 2022 at 03:24:43PM +0000, Marc Zyngier wrote:
> > On Fri, 04 Mar 2022 08:23:42 +0000,
> > Shawn Guo <shawn.guo@linaro.org> wrote:
> > > 
> > > On Fri, Mar 04, 2022 at 07:59:15AM +0000, Marc Zyngier wrote:
> > > > On Thu, 03 Mar 2022 04:02:29 +0000,
> > > > Shawn Guo <shawn.guo@linaro.org> wrote:
> > > > > 
> > > > > On Wed, Mar 02, 2022 at 01:57:27PM +0000, Marc Zyngier wrote:
> > > > > > This code actually makes me ask more questions. Why is it programming
> > > > > > 2 'pins' for each IRQ?
> > > > > 
> > > > > The mapping between MPM pin and GIC IRQ is not strictly 1-1.  There are
> > > > > some rare case that up to 2 MPM pins map to a single GIC IRQ, for
> > > > > example the last two in QC2290 'qcom,mpm-pin-map' below.
> > > > > 
> > > > > 	qcom,mpm-pin-map = <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 */
> > > > > 
> > > > > 
> > > > > The downstream uses a DT bindings that specifies GIC hwirq number in
> > > > > client device nodes.  In that case, d->hwirq in the driver is GIC IRQ
> > > > > number, and the driver will need to query mapping table, find out the
> > > > > possible 2 MPM pins, and set them up.
> > > > > 
> > > > > The patches I'm posting here use a different bindings that specifies MPM
> > > > > pin instead in client device nodes.  Thus the driver can simply get the
> > > > > MPM pin from d->hwirq, so that the whole look-up procedure can be saved.
> > > > 
> > > > It still remains that there is no 1:1 mapping between input and
> > > > output, which is the rule #1 to be able to use a hierarchical setup.
> > > 
> > > For direction of MPM pin -> GIC interrupt, it's a 1:1 mapping, i.e. for
> > > given MPM pin, there is only one GIC interrupt.  And that's the
> > > mapping MPM driver relies on.  For GIC interrupt -> MPM pin, it's not
> > > a strict 1:1 mapping.
> > 
> > Then this isn't a 1:1 mapping *AT ALL*. The hierarchical setup
> > mandates that the mapping is a bijective function, and that's exactly
> > what 1:1 means. There is no such thing a 1:1 in a single
> > direction. When you take an interrupt, all you see is the GIC
> > interrupt. How do you know which of the *two* pins interrupted you? Oh
> > wait, you *can't* know. You end-up never servicing one of the two
> > interrupts
> 
> Yes, you are right!  But that might be a problem only in theory.  I
> checked all the Qualcomm platforms I know built on MPM, and found that
> the only 2:1 case is USB DP & DM sensing pins.  Since these two pins
> will be handled by USB driver with a single interrupt handler, it should
> not cause any problem in practice.  That said, the 2:1 mapping is just
> a special case specific to USB, and MPM driver can be implemented as if
> it's just a 1:1 mapping.
>
> Shawn
> 
> > (and I suspect this results in memory corruption if you
> > tear a hierarchy down).

Key point here ^^^^^^^^^^

You can't have *any* interrupt that fits this 2:1 model if the irqchip
implements 1:1. Think about the data structures for a second:

Pins x and y and routed to GIC interrupt z. This results in the
following irq_data structures:

   MPM-x ---\
             GIC-z
   MPM-y ---/

Now, the driver using these interrupts is being removed, and the
hierarchies is being freed. Tearing down the interrupt with pin x will
result in z being also freed. And then you'll process pin y, which
will just explode.

So either you make sure these mappings can never be created, or you
change the driver to not be hierarchical. You absolutely cannot have
it both ways.

	M.

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

Re: [PATCH v7 2/2] irqchip: Add Qualcomm MPM controller driver

[Shawn Guo]

On Sat, Mar 05, 2022 at 11:05:07AM +0000, Marc Zyngier wrote:
> On Sat, 05 Mar 2022 09:24:20 +0000,
> Shawn Guo <shawn.guo@linaro.org> wrote:
> > 
> > On Fri, Mar 04, 2022 at 03:24:43PM +0000, Marc Zyngier wrote:
> > > On Fri, 04 Mar 2022 08:23:42 +0000,
> > > Shawn Guo <shawn.guo@linaro.org> wrote:
> > > > 
> > > > On Fri, Mar 04, 2022 at 07:59:15AM +0000, Marc Zyngier wrote:
> > > > > On Thu, 03 Mar 2022 04:02:29 +0000,
> > > > > Shawn Guo <shawn.guo@linaro.org> wrote:
> > > > > > 
> > > > > > On Wed, Mar 02, 2022 at 01:57:27PM +0000, Marc Zyngier wrote:
> > > > > > > This code actually makes me ask more questions. Why is it programming
> > > > > > > 2 'pins' for each IRQ?
> > > > > > 
> > > > > > The mapping between MPM pin and GIC IRQ is not strictly 1-1.  There are
> > > > > > some rare case that up to 2 MPM pins map to a single GIC IRQ, for
> > > > > > example the last two in QC2290 'qcom,mpm-pin-map' below.
> > > > > > 
> > > > > > 	qcom,mpm-pin-map = <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 */
> > > > > > 
> > > > > > 
> > > > > > The downstream uses a DT bindings that specifies GIC hwirq number in
> > > > > > client device nodes.  In that case, d->hwirq in the driver is GIC IRQ
> > > > > > number, and the driver will need to query mapping table, find out the
> > > > > > possible 2 MPM pins, and set them up.
> > > > > > 
> > > > > > The patches I'm posting here use a different bindings that specifies MPM
> > > > > > pin instead in client device nodes.  Thus the driver can simply get the
> > > > > > MPM pin from d->hwirq, so that the whole look-up procedure can be saved.
> > > > > 
> > > > > It still remains that there is no 1:1 mapping between input and
> > > > > output, which is the rule #1 to be able to use a hierarchical setup.
> > > > 
> > > > For direction of MPM pin -> GIC interrupt, it's a 1:1 mapping, i.e. for
> > > > given MPM pin, there is only one GIC interrupt.  And that's the
> > > > mapping MPM driver relies on.  For GIC interrupt -> MPM pin, it's not
> > > > a strict 1:1 mapping.
> > > 
> > > Then this isn't a 1:1 mapping *AT ALL*. The hierarchical setup
> > > mandates that the mapping is a bijective function, and that's exactly
> > > what 1:1 means. There is no such thing a 1:1 in a single
> > > direction. When you take an interrupt, all you see is the GIC
> > > interrupt. How do you know which of the *two* pins interrupted you? Oh
> > > wait, you *can't* know. You end-up never servicing one of the two
> > > interrupts
> > 
> > Yes, you are right!  But that might be a problem only in theory.  I
> > checked all the Qualcomm platforms I know built on MPM, and found that
> > the only 2:1 case is USB DP & DM sensing pins.  Since these two pins
> > will be handled by USB driver with a single interrupt handler, it should
> > not cause any problem in practice.  That said, the 2:1 mapping is just
> > a special case specific to USB, and MPM driver can be implemented as if
> > it's just a 1:1 mapping.
> >
> > Shawn
> > 
> > > (and I suspect this results in memory corruption if you
> > > tear a hierarchy down).
> 
> Key point here ^^^^^^^^^^
> 
> You can't have *any* interrupt that fits this 2:1 model if the irqchip
> implements 1:1. Think about the data structures for a second:
> 
> Pins x and y and routed to GIC interrupt z. This results in the
> following irq_data structures:
> 
>    MPM-x ---\
>              GIC-z
>    MPM-y ---/
> 
> Now, the driver using these interrupts is being removed, and the
> hierarchies is being freed. Tearing down the interrupt with pin x will
> result in z being also freed. And then you'll process pin y, which
> will just explode.

I tested with manually unbinding the USB driver and didn't run into any
memory corruption.  If I read irq_domain code right, it seems that
irq_domain_alloc_irq_data() will call into irq_domain_insert_irq_data()
to allocate z irq_data in context of virq x and y respectively.  So x
and y do not share a single parent (z) irq_data but have their own copy
of z irq_data, no?

Shawn

Re: [PATCH v7 2/2] irqchip: Add Qualcomm MPM controller driver

[Marc Zyngier]

On Sun, 06 Mar 2022 12:57:10 +0000,
Shawn Guo <shawn.guo@linaro.org> wrote:
> 
> On Sat, Mar 05, 2022 at 11:05:07AM +0000, Marc Zyngier wrote:
> > On Sat, 05 Mar 2022 09:24:20 +0000,
> > Shawn Guo <shawn.guo@linaro.org> wrote:
> > > 
> > > On Fri, Mar 04, 2022 at 03:24:43PM +0000, Marc Zyngier wrote:
> > > > On Fri, 04 Mar 2022 08:23:42 +0000,
> > > > Shawn Guo <shawn.guo@linaro.org> wrote:
> > > > > 
> > > > > On Fri, Mar 04, 2022 at 07:59:15AM +0000, Marc Zyngier wrote:
> > > > > > On Thu, 03 Mar 2022 04:02:29 +0000,
> > > > > > Shawn Guo <shawn.guo@linaro.org> wrote:
> > > > > > > 
> > > > > > > On Wed, Mar 02, 2022 at 01:57:27PM +0000, Marc Zyngier wrote:
> > > > > > > > This code actually makes me ask more questions. Why is it programming
> > > > > > > > 2 'pins' for each IRQ?
> > > > > > > 
> > > > > > > The mapping between MPM pin and GIC IRQ is not strictly 1-1.  There are
> > > > > > > some rare case that up to 2 MPM pins map to a single GIC IRQ, for
> > > > > > > example the last two in QC2290 'qcom,mpm-pin-map' below.
> > > > > > > 
> > > > > > > 	qcom,mpm-pin-map = <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 */
> > > > > > > 
> > > > > > > 
> > > > > > > The downstream uses a DT bindings that specifies GIC hwirq number in
> > > > > > > client device nodes.  In that case, d->hwirq in the driver is GIC IRQ
> > > > > > > number, and the driver will need to query mapping table, find out the
> > > > > > > possible 2 MPM pins, and set them up.
> > > > > > > 
> > > > > > > The patches I'm posting here use a different bindings that specifies MPM
> > > > > > > pin instead in client device nodes.  Thus the driver can simply get the
> > > > > > > MPM pin from d->hwirq, so that the whole look-up procedure can be saved.
> > > > > > 
> > > > > > It still remains that there is no 1:1 mapping between input and
> > > > > > output, which is the rule #1 to be able to use a hierarchical setup.
> > > > > 
> > > > > For direction of MPM pin -> GIC interrupt, it's a 1:1 mapping, i.e. for
> > > > > given MPM pin, there is only one GIC interrupt.  And that's the
> > > > > mapping MPM driver relies on.  For GIC interrupt -> MPM pin, it's not
> > > > > a strict 1:1 mapping.
> > > > 
> > > > Then this isn't a 1:1 mapping *AT ALL*. The hierarchical setup
> > > > mandates that the mapping is a bijective function, and that's exactly
> > > > what 1:1 means. There is no such thing a 1:1 in a single
> > > > direction. When you take an interrupt, all you see is the GIC
> > > > interrupt. How do you know which of the *two* pins interrupted you? Oh
> > > > wait, you *can't* know. You end-up never servicing one of the two
> > > > interrupts
> > > 
> > > Yes, you are right!  But that might be a problem only in theory.  I
> > > checked all the Qualcomm platforms I know built on MPM, and found that
> > > the only 2:1 case is USB DP & DM sensing pins.  Since these two pins
> > > will be handled by USB driver with a single interrupt handler, it should
> > > not cause any problem in practice.  That said, the 2:1 mapping is just
> > > a special case specific to USB, and MPM driver can be implemented as if
> > > it's just a 1:1 mapping.
> > >
> > > Shawn
> > > 
> > > > (and I suspect this results in memory corruption if you
> > > > tear a hierarchy down).
> > 
> > Key point here ^^^^^^^^^^
> > 
> > You can't have *any* interrupt that fits this 2:1 model if the irqchip
> > implements 1:1. Think about the data structures for a second:
> > 
> > Pins x and y and routed to GIC interrupt z. This results in the
> > following irq_data structures:
> > 
> >    MPM-x ---\
> >              GIC-z
> >    MPM-y ---/
> > 
> > Now, the driver using these interrupts is being removed, and the
> > hierarchies is being freed. Tearing down the interrupt with pin x will
> > result in z being also freed. And then you'll process pin y, which
> > will just explode.
> 
> I tested with manually unbinding the USB driver and didn't run into any
> memory corruption.  If I read irq_domain code right, it seems that
> irq_domain_alloc_irq_data() will call into irq_domain_insert_irq_data()
> to allocate z irq_data in context of virq x and y respectively.  So x
> and y do not share a single parent (z) irq_data but have their own copy
> of z irq_data, no?

Which is just another bug you are relying on. Maybe you're OK with
that, but I'm not (and I intend to fix this bug).

I'm not taking this driver until you either:

- prevent a pin sharing a GIC interrupt from triggering an interrupt
  allocation in the driver

- or turn this driver into something that isn't a hierarchical setup

	M.

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