* [PATCH v3 0/3] Add Qualcomm MPM irqchip driver support
@ 2021-12-02 12:21 Shawn Guo
2021-12-02 12:21 ` [PATCH v3 1/3] irqchip: Pass platform_device pointer to init_cb Shawn Guo
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Shawn Guo @ 2021-12-02 12:21 UTC (permalink / raw)
To: Marc Zyngier, Thomas Gleixner
Cc: Maulik Shah, Bjorn Andersson, Loic Poulain, linux-arm-msm,
linux-kernel, Shawn Guo
It updates IRQCHIP_PLATFORM_DRIVER infrastructural to pass
platform_device pointer to init_cb. On top of that, add DT binding
and driver support for Qualcomm MPM (MSM Power Manager) interrupt
controller.
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 (3):
irqchip: Pass platform_device pointer to init_cb
dt-bindings: interrupt-controller: Add Qualcomm MPM support
irqchip: Add Qualcomm MPM controller driver
.../interrupt-controller/qcom,mpm.yaml | 72 +++
drivers/irqchip/Kconfig | 8 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-bcm7038-l1.c | 3 +-
drivers/irqchip/irq-bcm7120-l2.c | 10 +-
drivers/irqchip/irq-brcmstb-l2.c | 10 +-
drivers/irqchip/irq-mchp-eic.c | 4 +-
drivers/irqchip/irq-meson-gpio.c | 7 +-
drivers/irqchip/irqchip.c | 4 +-
drivers/irqchip/qcom-mpm.c | 481 ++++++++++++++++++
drivers/irqchip/qcom-pdc.c | 4 +-
include/linux/irqchip.h | 8 +-
12 files changed, 596 insertions(+), 16 deletions(-)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml
create mode 100644 drivers/irqchip/qcom-mpm.c
--
2.17.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 1/3] irqchip: Pass platform_device pointer to init_cb
2021-12-02 12:21 [PATCH v3 0/3] Add Qualcomm MPM irqchip driver support Shawn Guo
@ 2021-12-02 12:21 ` Shawn Guo
2021-12-02 17:52 ` Florian Fainelli
2021-12-02 12:21 ` [PATCH v3 2/3] dt-bindings: interrupt-controller: Add Qualcomm MPM support Shawn Guo
2021-12-02 12:21 ` [PATCH v3 3/3] irqchip: Add Qualcomm MPM controller driver Shawn Guo
2 siblings, 1 reply; 17+ messages in thread
From: Shawn Guo @ 2021-12-02 12:21 UTC (permalink / raw)
To: Marc Zyngier, Thomas Gleixner
Cc: Maulik Shah, Bjorn Andersson, Loic Poulain, linux-arm-msm,
linux-kernel, Shawn Guo, Florian Fainelli, Claudiu Beznea,
Neil Armstrong
It makes sense to just pass device_node for callback in IRQCHIP_DECLARE
case, but not so much for IRQCHIP_PLATFORM_DRIVER one, because
platform_driver probe/init usually needs device pointer for various
purposes, e.g. resource allocation, service request, device prefixed
message output, etc. Create a new callback type irqchip_init_cb_t which
takes platform_device pointer as parameter, and update the existing
IRQCHIP_PLATFORM_DRIVER users accordingly.
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
drivers/irqchip/irq-bcm7038-l1.c | 3 ++-
drivers/irqchip/irq-bcm7120-l2.c | 10 ++++++----
drivers/irqchip/irq-brcmstb-l2.c | 10 ++++++----
drivers/irqchip/irq-mchp-eic.c | 4 +++-
drivers/irqchip/irq-meson-gpio.c | 7 +++++--
drivers/irqchip/irqchip.c | 4 ++--
drivers/irqchip/qcom-pdc.c | 4 +++-
include/linux/irqchip.h | 8 +++++++-
8 files changed, 34 insertions(+), 16 deletions(-)
diff --git a/drivers/irqchip/irq-bcm7038-l1.c b/drivers/irqchip/irq-bcm7038-l1.c
index a62b96237b82..d52a598f73df 100644
--- a/drivers/irqchip/irq-bcm7038-l1.c
+++ b/drivers/irqchip/irq-bcm7038-l1.c
@@ -396,9 +396,10 @@ static const struct irq_domain_ops bcm7038_l1_domain_ops = {
.map = bcm7038_l1_map,
};
-static int __init bcm7038_l1_of_init(struct device_node *dn,
+static int __init bcm7038_l1_of_init(struct platform_device *pdev,
struct device_node *parent)
{
+ struct device_node *dn = pdev->dev.of_node;
struct bcm7038_l1_chip *intc;
int idx, ret;
diff --git a/drivers/irqchip/irq-bcm7120-l2.c b/drivers/irqchip/irq-bcm7120-l2.c
index d80e67a6aad2..82a75eb11523 100644
--- a/drivers/irqchip/irq-bcm7120-l2.c
+++ b/drivers/irqchip/irq-bcm7120-l2.c
@@ -341,17 +341,19 @@ static int __init bcm7120_l2_intc_probe(struct device_node *dn,
return ret;
}
-static int __init bcm7120_l2_intc_probe_7120(struct device_node *dn,
+static int __init bcm7120_l2_intc_probe_7120(struct platform_device *pdev,
struct device_node *parent)
{
- return bcm7120_l2_intc_probe(dn, parent, bcm7120_l2_intc_iomap_7120,
+ return bcm7120_l2_intc_probe(pdev->dev.of_node, parent,
+ bcm7120_l2_intc_iomap_7120,
"BCM7120 L2");
}
-static int __init bcm7120_l2_intc_probe_3380(struct device_node *dn,
+static int __init bcm7120_l2_intc_probe_3380(struct platform_device *pdev,
struct device_node *parent)
{
- return bcm7120_l2_intc_probe(dn, parent, bcm7120_l2_intc_iomap_3380,
+ return bcm7120_l2_intc_probe(pdev->dev.of_node, parent,
+ bcm7120_l2_intc_iomap_3380,
"BCM3380 L2");
}
diff --git a/drivers/irqchip/irq-brcmstb-l2.c b/drivers/irqchip/irq-brcmstb-l2.c
index e4efc08ac594..52886fbcea18 100644
--- a/drivers/irqchip/irq-brcmstb-l2.c
+++ b/drivers/irqchip/irq-brcmstb-l2.c
@@ -270,16 +270,18 @@ static int __init brcmstb_l2_intc_of_init(struct device_node *np,
return ret;
}
-static int __init brcmstb_l2_edge_intc_of_init(struct device_node *np,
+static int __init brcmstb_l2_edge_intc_of_init(struct platform_device *pdev,
struct device_node *parent)
{
- return brcmstb_l2_intc_of_init(np, parent, &l2_edge_intc_init);
+ return brcmstb_l2_intc_of_init(pdev->dev.of_node, parent,
+ &l2_edge_intc_init);
}
-static int __init brcmstb_l2_lvl_intc_of_init(struct device_node *np,
+static int __init brcmstb_l2_lvl_intc_of_init(struct platform_device *pdev,
struct device_node *parent)
{
- return brcmstb_l2_intc_of_init(np, parent, &l2_lvl_intc_init);
+ return brcmstb_l2_intc_of_init(pdev->dev.of_node, parent,
+ &l2_lvl_intc_init);
}
IRQCHIP_PLATFORM_DRIVER_BEGIN(brcmstb_l2)
diff --git a/drivers/irqchip/irq-mchp-eic.c b/drivers/irqchip/irq-mchp-eic.c
index c726a19837d2..1ab34af841f6 100644
--- a/drivers/irqchip/irq-mchp-eic.c
+++ b/drivers/irqchip/irq-mchp-eic.c
@@ -199,8 +199,10 @@ static const struct irq_domain_ops mchp_eic_domain_ops = {
.free = irq_domain_free_irqs_common,
};
-static int mchp_eic_init(struct device_node *node, struct device_node *parent)
+static int mchp_eic_init(struct platform_device *pdev,
+ struct device_node *parent)
{
+ struct device_node *node = pdev->dev.of_node;
struct irq_domain *parent_domain = NULL;
int ret, i;
diff --git a/drivers/irqchip/irq-meson-gpio.c b/drivers/irqchip/irq-meson-gpio.c
index d90ff0b92480..1c841189defa 100644
--- a/drivers/irqchip/irq-meson-gpio.c
+++ b/drivers/irqchip/irq-meson-gpio.c
@@ -436,7 +436,8 @@ static const struct irq_domain_ops meson_gpio_irq_domain_ops = {
.translate = meson_gpio_irq_domain_translate,
};
-static int meson_gpio_irq_parse_dt(struct device_node *node, struct meson_gpio_irq_controller *ctl)
+static int meson_gpio_irq_parse_dt(struct device_node *node,
+ struct meson_gpio_irq_controller *ctl)
{
const struct of_device_id *match;
int ret;
@@ -462,8 +463,10 @@ static int meson_gpio_irq_parse_dt(struct device_node *node, struct meson_gpio_i
return 0;
}
-static int meson_gpio_irq_of_init(struct device_node *node, struct device_node *parent)
+static int meson_gpio_irq_of_init(struct platform_device *pdev,
+ struct device_node *parent)
{
+ struct device_node *node = pdev->dev.of_node;
struct irq_domain *domain, *parent_domain;
struct meson_gpio_irq_controller *ctl;
int ret;
diff --git a/drivers/irqchip/irqchip.c b/drivers/irqchip/irqchip.c
index 3570f0a588c4..62e6dbc3c4d0 100644
--- a/drivers/irqchip/irqchip.c
+++ b/drivers/irqchip/irqchip.c
@@ -36,7 +36,7 @@ int platform_irqchip_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
struct device_node *par_np = of_irq_find_parent(np);
- of_irq_init_cb_t irq_init_cb = of_device_get_match_data(&pdev->dev);
+ irqchip_init_cb_t irq_init_cb = of_device_get_match_data(&pdev->dev);
if (!irq_init_cb)
return -EINVAL;
@@ -55,6 +55,6 @@ int platform_irqchip_probe(struct platform_device *pdev)
if (par_np && !irq_find_matching_host(par_np, DOMAIN_BUS_ANY))
return -EPROBE_DEFER;
- return irq_init_cb(np, par_np);
+ return irq_init_cb(pdev, par_np);
}
EXPORT_SYMBOL_GPL(platform_irqchip_probe);
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index 173e6520e06e..b66ac9dd46c3 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -359,8 +359,10 @@ static int pdc_setup_pin_mapping(struct device_node *np)
return 0;
}
-static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
+static int qcom_pdc_init(struct platform_device *pdev,
+ struct device_node *parent)
{
+ struct device_node *node = pdev->dev.of_node;
struct irq_domain *parent_domain, *pdc_domain, *pdc_gpio_domain;
int ret;
diff --git a/include/linux/irqchip.h b/include/linux/irqchip.h
index 3a091d0710ae..da33a21c0297 100644
--- a/include/linux/irqchip.h
+++ b/include/linux/irqchip.h
@@ -36,13 +36,19 @@ extern of_irq_init_cb_t typecheck_irq_init_cb;
#define IRQCHIP_DECLARE(name, compat, fn) \
OF_DECLARE_2(irqchip, name, compat, typecheck_irq_init_cb(fn))
+typedef int (*irqchip_init_cb_t)(struct platform_device *,
+ struct device_node *);
+extern irqchip_init_cb_t typecheck_irqchip_init_cb;
+#define typecheck_irqchip_init_cb(fn) \
+ (__typecheck(typecheck_irqchip_init_cb, &fn) ? fn : fn)
+
extern int platform_irqchip_probe(struct platform_device *pdev);
#define IRQCHIP_PLATFORM_DRIVER_BEGIN(drv_name) \
static const struct of_device_id drv_name##_irqchip_match_table[] = {
#define IRQCHIP_MATCH(compat, fn) { .compatible = compat, \
- .data = typecheck_irq_init_cb(fn), },
+ .data = typecheck_irqchip_init_cb(fn), },
#define IRQCHIP_PLATFORM_DRIVER_END(drv_name) \
{}, \
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v3 2/3] dt-bindings: interrupt-controller: Add Qualcomm MPM support
2021-12-02 12:21 [PATCH v3 0/3] Add Qualcomm MPM irqchip driver support Shawn Guo
2021-12-02 12:21 ` [PATCH v3 1/3] irqchip: Pass platform_device pointer to init_cb Shawn Guo
@ 2021-12-02 12:21 ` Shawn Guo
2021-12-02 12:21 ` [PATCH v3 3/3] irqchip: Add Qualcomm MPM controller driver Shawn Guo
2 siblings, 0 replies; 17+ messages in thread
From: Shawn Guo @ 2021-12-02 12:21 UTC (permalink / raw)
To: Marc Zyngier, Thomas Gleixner
Cc: Maulik Shah, Bjorn Andersson, Loic Poulain, linux-arm-msm,
linux-kernel, 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 | 72 +++++++++++++++++++
1 file changed, 72 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..22e87fe2eb8e
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml
@@ -0,0 +1,72 @@
+# 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,qcm2290-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.
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - mboxes
+ - interrupt-controller
+ - '#interrupt-cells'
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ mpm: interrupt-controller@45f01b8 {
+ compatible = "qcom,qcm2290-mpm";
+ interrupts = <GIC_SPI 197 IRQ_TYPE_EDGE_RISING>;
+ reg = <0x45f01b8 0x1000>;
+ mboxes = <&apcs_glb 1>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ interrupt-parent = <&intc>;
+ };
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v3 3/3] irqchip: Add Qualcomm MPM controller driver
2021-12-02 12:21 [PATCH v3 0/3] Add Qualcomm MPM irqchip driver support Shawn Guo
2021-12-02 12:21 ` [PATCH v3 1/3] irqchip: Pass platform_device pointer to init_cb Shawn Guo
2021-12-02 12:21 ` [PATCH v3 2/3] dt-bindings: interrupt-controller: Add Qualcomm MPM support Shawn Guo
@ 2021-12-02 12:21 ` Shawn Guo
2021-12-03 0:00 ` kernel test robot
2021-12-06 9:46 ` Marc Zyngier
2 siblings, 2 replies; 17+ messages in thread
From: Shawn Guo @ 2021-12-02 12:21 UTC (permalink / raw)
To: Marc Zyngier, Thomas Gleixner
Cc: Maulik Shah, Bjorn Andersson, Loic Poulain, linux-arm-msm,
linux-kernel, 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 can be found as the SoC data in this
MPM driver, while the latter can be found as the msm_gpio_wakeirq_map[]
in TLMM driver.
- All the register settings are done by APSS on an internal memory
region called vMPM, and RPM will flush them into hardware after it
receives a mailbox/IPC notification from APSS.
- When SoC gets awake from sleep mode, the driver will receive an
interrupt from RPM, so that it can replay interrupt for particular
polarity.
Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
drivers/irqchip/Kconfig | 8 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/qcom-mpm.c | 481 +++++++++++++++++++++++++++++++++++++
3 files changed, 490 insertions(+)
create mode 100644 drivers/irqchip/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..0e2e10467e28 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -94,6 +94,7 @@ obj-$(CONFIG_MESON_IRQ_GPIO) += irq-meson-gpio.o
obj-$(CONFIG_GOLDFISH_PIC) += irq-goldfish-pic.o
obj-$(CONFIG_NDS32) += irq-ativic32.o
obj-$(CONFIG_QCOM_PDC) += qcom-pdc.o
+obj-$(CONFIG_QCOM_MPM) += qcom-mpm.o
obj-$(CONFIG_CSKY_MPINTC) += irq-csky-mpintc.o
obj-$(CONFIG_CSKY_APB_INTC) += irq-csky-apb-intc.o
obj-$(CONFIG_RISCV_INTC) += irq-riscv-intc.o
diff --git a/drivers/irqchip/qcom-mpm.c b/drivers/irqchip/qcom-mpm.c
new file mode 100644
index 000000000000..4a0dac2e2c6e
--- /dev/null
+++ b/drivers/irqchip/qcom-mpm.c
@@ -0,0 +1,481 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2021, Linaro Limited
+ * Copyright (c) 2010-2020, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/atomic.h>
+#include <linux/cpu_pm.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/arm-gic-v3.h>
+#include <linux/irqdomain.h>
+#include <linux/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/soc/qcom/irq.h>
+#include <linux/spinlock.h>
+
+/*
+ * vMPM register layout:
+ *
+ * 31 0
+ * +--------------------------------+
+ * | TIMER0 | 0x00
+ * +--------------------------------+
+ * | TIMER1 | 0x04
+ * +--------------------------------+
+ * | ENABLE0 | 0x08
+ * +--------------------------------+
+ * | ... | ...
+ * +--------------------------------+
+ * | ENABLEn |
+ * +--------------------------------+
+ * | FALLING_EDGE0 |
+ * +--------------------------------+
+ * | ... |
+ * +--------------------------------+
+ * | STATUSn |
+ * +--------------------------------+
+ *
+ * n = DIV_ROUND_UP(pin_num, 32)
+ *
+ */
+#define MPM_REG_ENABLE 0
+#define MPM_REG_FALLING_EDGE 1
+#define MPM_REG_RISING_EDGE 2
+#define MPM_REG_POLARITY 3
+#define MPM_REG_STATUS 4
+
+#define MPM_NO_PARENT_IRQ ~0UL
+
+/* MPM pin and its GIC hwirq */
+struct mpm_pin {
+ int pin;
+ irq_hw_number_t hwirq;
+};
+
+struct mpm_data {
+ unsigned int pin_num;
+ const struct mpm_pin *gic_pins;
+};
+
+struct qcom_mpm_priv {
+ void __iomem *base;
+ raw_spinlock_t lock;
+ struct mbox_client mbox_client;
+ struct mbox_chan *mbox_chan;
+ const struct mpm_data *data;
+ unsigned int reg_stride;
+ struct irq_domain *domain;
+ struct notifier_block pm_nb;
+ atomic_t cpus_in_pm;
+};
+
+static inline u32
+qcom_mpm_read(struct qcom_mpm_priv *priv, unsigned int reg, unsigned int index)
+{
+ unsigned int offset = (reg * priv->reg_stride + index + 2) * 4;
+
+ return readl_relaxed(priv->base + offset);
+}
+
+static inline void
+qcom_mpm_write(struct qcom_mpm_priv *priv, unsigned int reg,
+ unsigned int index, u32 val)
+{
+ unsigned int offset = (reg * priv->reg_stride + index + 2) * 4;
+
+ writel_relaxed(val, priv->base + offset);
+
+ /* Ensure the write is completed */
+ wmb();
+}
+
+static inline 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;
+ u32 val;
+
+ raw_spin_lock(&priv->lock);
+
+ 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(&priv->lock);
+}
+
+static void qcom_mpm_mask(struct irq_data *d)
+{
+ qcom_mpm_enable_irq(d, false);
+
+ if (d->parent_data)
+ irq_chip_mask_parent(d);
+}
+
+static void qcom_mpm_unmask(struct irq_data *d)
+{
+ qcom_mpm_enable_irq(d, true);
+
+ if (d->parent_data)
+ irq_chip_unmask_parent(d);
+}
+
+static inline void
+mpm_set_type(struct qcom_mpm_priv *priv, bool set, unsigned int reg,
+ unsigned int index, unsigned int shift)
+{
+ u32 val;
+
+ raw_spin_lock(&priv->lock);
+
+ 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(&priv->lock);
+}
+
+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 irq_hw_number_t get_parent_hwirq(struct qcom_mpm_priv *priv, int pin)
+{
+ const struct mpm_pin *gic_pins = priv->data->gic_pins;
+ int i;
+
+ for (i = 0; gic_pins[i].pin >= 0; i++) {
+ int p = gic_pins[i].pin;
+
+ if (p < 0)
+ break;
+
+ if (p == pin)
+ return gic_pins[i].hwirq;
+ }
+
+ return MPM_NO_PARENT_IRQ;
+}
+
+static int qcom_mpm_alloc(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs, void *data)
+{
+ struct qcom_mpm_priv *priv = domain->host_data;
+ struct irq_fwspec *fwspec = data;
+ struct irq_fwspec parent_fwspec;
+ irq_hw_number_t parent_hwirq;
+ irq_hw_number_t hwirq;
+ unsigned int type;
+ int ret;
+
+ ret = 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;
+
+ parent_hwirq = get_parent_hwirq(priv, hwirq);
+ if (parent_hwirq == MPM_NO_PARENT_IRQ)
+ return irq_domain_disconnect_hierarchy(domain->parent, virq);
+
+ if (type & IRQ_TYPE_EDGE_BOTH)
+ type = IRQ_TYPE_EDGE_RISING;
+
+ if (type & IRQ_TYPE_LEVEL_MASK)
+ type = IRQ_TYPE_LEVEL_HIGH;
+
+ parent_fwspec.fwnode = domain->parent->fwnode;
+ parent_fwspec.param_count = 3;
+ parent_fwspec.param[0] = 0;
+ parent_fwspec.param[1] = parent_hwirq;
+ parent_fwspec.param[2] = type;
+
+ return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
+ &parent_fwspec);
+}
+
+static const struct irq_domain_ops qcom_mpm_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;
+ int i, j;
+
+ for (i = 0; i < priv->reg_stride; i++) {
+ enable = qcom_mpm_read(priv, MPM_REG_ENABLE, i);
+ pending = qcom_mpm_read(priv, MPM_REG_STATUS, i);
+ pending &= enable;
+
+ for_each_set_bit(j, &pending, 32) {
+ unsigned int pin = 32 * i + j;
+ 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);
+
+ }
+ }
+
+ return IRQ_HANDLED;
+}
+
+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 qcom_mpm_cpu_pm_callback(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct qcom_mpm_priv *priv = container_of(nb, struct qcom_mpm_priv,
+ pm_nb);
+ int ret = NOTIFY_OK;
+ int cpus_in_pm;
+
+ switch (action) {
+ case CPU_PM_ENTER:
+ cpus_in_pm = atomic_inc_return(&priv->cpus_in_pm);
+ /*
+ * NOTE: comments for num_online_cpus() point out that it's
+ * only a snapshot so we need to be careful. It should be OK
+ * for us to use, though. It's important for us not to miss
+ * if we're the last CPU going down so it would only be a
+ * problem if a CPU went offline right after we did the check
+ * AND that CPU was not idle AND that CPU was the last non-idle
+ * CPU. That can't happen. CPUs would have to come out of idle
+ * before the CPU could go offline.
+ */
+ if (cpus_in_pm < num_online_cpus())
+ return NOTIFY_OK;
+ break;
+ case CPU_PM_ENTER_FAILED:
+ case CPU_PM_EXIT:
+ atomic_dec(&priv->cpus_in_pm);
+ return NOTIFY_OK;
+ default:
+ return NOTIFY_DONE;
+ }
+
+ /*
+ * It's likely we're on the last CPU. Grab the lock and write MPM for
+ * sleep. Grabbing the lock means that if we race with another CPU
+ * coming up we are still guaranteed to be safe.
+ */
+ if (raw_spin_trylock(&priv->lock)) {
+ if (qcom_mpm_enter_sleep(priv))
+ ret = NOTIFY_BAD;
+ raw_spin_unlock(&priv->lock);
+ } else {
+ /* Another CPU must be up */
+ return NOTIFY_OK;
+ }
+
+ if (ret == NOTIFY_BAD) {
+ /* Double-check if we're here because someone else is up */
+ if (cpus_in_pm < num_online_cpus())
+ ret = NOTIFY_OK;
+ else
+ /* We won't be called w/ CPU_PM_ENTER_FAILED */
+ atomic_dec(&priv->cpus_in_pm);
+ }
+
+ return ret;
+}
+
+static int qcom_mpm_init(struct platform_device *pdev,
+ struct device_node *parent,
+ const struct mpm_data *data)
+{
+ struct irq_domain *parent_domain;
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+ struct qcom_mpm_priv *priv;
+ unsigned int pin_num;
+ int irq;
+ int ret;
+
+ if (!data)
+ return -ENODEV;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->data = data;
+ pin_num = priv->data->pin_num;
+ priv->reg_stride = DIV_ROUND_UP(pin_num, 32);
+
+ 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;
+
+ 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_num,
+ 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_TRIGGER_RISING | IRQF_NO_SUSPEND,
+ "qcom_mpm", priv);
+ if (ret) {
+ dev_err(dev, "failed to request irq: %d\n", ret);
+ goto remove_domain;
+ }
+
+ priv->pm_nb.notifier_call = qcom_mpm_cpu_pm_callback;
+ cpu_pm_register_notifier(&priv->pm_nb);
+
+ dev_set_drvdata(dev, priv);
+
+ return 0;
+
+remove_domain:
+ irq_domain_remove(priv->domain);
+free_mbox:
+ mbox_free_channel(priv->mbox_chan);
+ return ret;
+}
+
+/*
+ * The mapping between MPM_GIC pin and GIC SPI number on QCM2290. It's taken
+ * from downstream qcom-mpm-scuba.c with a little transform on the GIC
+ * SPI numbers (the second column). Due to the binding difference from
+ * the downstream, where GIC SPI numbering starts from 32, we expect the
+ * numbering starts from 0 here, and that's why we have the number minus 32
+ * comparing to the downstream.
+ */
+const struct mpm_pin qcm2290_gic_pins[] = {
+ { 2, 275 }, /* tsens0_tsens_upper_lower_int */
+ { 5, 296 }, /* lpass_irq_out_sdc */
+ { 12, 422 }, /* b3_lfps_rxterm_irq */
+ { 24, 79 }, /* bi_px_lpi_1_aoss_mx */
+ { 86, 183 }, /* mpm_wake,spmi_m */
+ { 90, 260 }, /* eud_p0_dpse_int_mx */
+ { 91, 260 }, /* eud_p0_dmse_int_mx */
+ { -1 },
+};
+
+const struct mpm_data qcm2290_data = {
+ .pin_num = 96,
+ .gic_pins = qcm2290_gic_pins,
+};
+
+static int qcm2290_mpm_init(struct platform_device *pdev,
+ struct device_node *parent)
+{
+ return qcom_mpm_init(pdev, parent, &qcm2290_data);
+}
+
+IRQCHIP_PLATFORM_DRIVER_BEGIN(qcom_mpm)
+IRQCHIP_MATCH("qcom,qcm2290-mpm", qcm2290_mpm_init)
+IRQCHIP_PLATFORM_DRIVER_END(qcom_mpm)
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. MSM Power Manager");
+MODULE_LICENSE("GPL v2");
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/3] irqchip: Pass platform_device pointer to init_cb
2021-12-02 12:21 ` [PATCH v3 1/3] irqchip: Pass platform_device pointer to init_cb Shawn Guo
@ 2021-12-02 17:52 ` Florian Fainelli
2021-12-02 19:10 ` Marc Zyngier
2021-12-06 6:35 ` Shawn Guo
0 siblings, 2 replies; 17+ messages in thread
From: Florian Fainelli @ 2021-12-02 17:52 UTC (permalink / raw)
To: Shawn Guo, Marc Zyngier, Thomas Gleixner
Cc: Maulik Shah, Bjorn Andersson, Loic Poulain, linux-arm-msm,
linux-kernel, Claudiu Beznea, Neil Armstrong
On 12/2/21 4:21 AM, Shawn Guo wrote:
> It makes sense to just pass device_node for callback in IRQCHIP_DECLARE
> case, but not so much for IRQCHIP_PLATFORM_DRIVER one, because
> platform_driver probe/init usually needs device pointer for various
> purposes, e.g. resource allocation, service request, device prefixed
> message output, etc. Create a new callback type irqchip_init_cb_t which
> takes platform_device pointer as parameter, and update the existing
> IRQCHIP_PLATFORM_DRIVER users accordingly.
>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Could you copy all recipients on all 3 patches plus your cover letter
next time so we have the full context? Thanks!
[snip]
>
> -static int __init bcm7120_l2_intc_probe_7120(struct device_node *dn,
> +static int __init bcm7120_l2_intc_probe_7120(struct platform_device *pdev,
> struct device_node *parent)
> {
> - return bcm7120_l2_intc_probe(dn, parent, bcm7120_l2_intc_iomap_7120,
> + return bcm7120_l2_intc_probe(pdev->dev.of_node, parent,
> + bcm7120_l2_intc_iomap_7120,
> "BCM7120 L2");
If you look further into that driver, you will see that we do something
like this in bcm7120_l2_intc_probe:
pdev = of_find_device_by_node(dn);
if (!pdev) {
ret = -ENODEV;
goto out_free_data;
}
which would be completely superfluous now that we pass a platform_device
directly. Can you rework your patch so as to eliminate that
of_find_device_by_ndoe() (and the companion put_device call)?
--
Florian
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/3] irqchip: Pass platform_device pointer to init_cb
2021-12-02 17:52 ` Florian Fainelli
@ 2021-12-02 19:10 ` Marc Zyngier
2021-12-02 21:44 ` Florian Fainelli
2021-12-06 6:40 ` Shawn Guo
2021-12-06 6:35 ` Shawn Guo
1 sibling, 2 replies; 17+ messages in thread
From: Marc Zyngier @ 2021-12-02 19:10 UTC (permalink / raw)
To: Florian Fainelli, Shawn Guo
Cc: Thomas Gleixner, Maulik Shah, Bjorn Andersson, Loic Poulain,
linux-arm-msm, linux-kernel, Claudiu Beznea, Neil Armstrong
On 2021-12-02 17:52, Florian Fainelli wrote:
> On 12/2/21 4:21 AM, Shawn Guo wrote:
>> It makes sense to just pass device_node for callback in
>> IRQCHIP_DECLARE
>> case, but not so much for IRQCHIP_PLATFORM_DRIVER one, because
>> platform_driver probe/init usually needs device pointer for various
>> purposes, e.g. resource allocation, service request, device prefixed
>> message output, etc. Create a new callback type irqchip_init_cb_t
>> which
>> takes platform_device pointer as parameter, and update the existing
>> IRQCHIP_PLATFORM_DRIVER users accordingly.
>>
>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>> Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
>> Cc: Neil Armstrong <narmstrong@baylibre.com>
>> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
>
> Could you copy all recipients on all 3 patches plus your cover letter
> next time so we have the full context? Thanks!
>
> [snip]
>
>>
>> -static int __init bcm7120_l2_intc_probe_7120(struct device_node *dn,
>> +static int __init bcm7120_l2_intc_probe_7120(struct platform_device
>> *pdev,
>> struct device_node *parent)
>> {
>> - return bcm7120_l2_intc_probe(dn, parent, bcm7120_l2_intc_iomap_7120,
>> + return bcm7120_l2_intc_probe(pdev->dev.of_node, parent,
>> + bcm7120_l2_intc_iomap_7120,
>> "BCM7120 L2");
>
> If you look further into that driver, you will see that we do something
> like this in bcm7120_l2_intc_probe:
>
> pdev = of_find_device_by_node(dn);
> if (!pdev) {
> ret = -ENODEV;
> goto out_free_data;
> }
>
> which would be completely superfluous now that we pass a
> platform_device
> directly. Can you rework your patch so as to eliminate that
> of_find_device_by_ndoe() (and the companion put_device call)?
Or just adopt the same construct in the MPM driver. At this stage,
drivers
requiring a platform_device are the minority.
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/3] irqchip: Pass platform_device pointer to init_cb
2021-12-02 19:10 ` Marc Zyngier
@ 2021-12-02 21:44 ` Florian Fainelli
2021-12-06 6:40 ` Shawn Guo
1 sibling, 0 replies; 17+ messages in thread
From: Florian Fainelli @ 2021-12-02 21:44 UTC (permalink / raw)
To: Marc Zyngier, Shawn Guo
Cc: Thomas Gleixner, Maulik Shah, Bjorn Andersson, Loic Poulain,
linux-arm-msm, linux-kernel, Claudiu Beznea, Neil Armstrong
On 12/2/21 11:10 AM, Marc Zyngier wrote:
> On 2021-12-02 17:52, Florian Fainelli wrote:
>> On 12/2/21 4:21 AM, Shawn Guo wrote:
>>> It makes sense to just pass device_node for callback in IRQCHIP_DECLARE
>>> case, but not so much for IRQCHIP_PLATFORM_DRIVER one, because
>>> platform_driver probe/init usually needs device pointer for various
>>> purposes, e.g. resource allocation, service request, device prefixed
>>> message output, etc. Create a new callback type irqchip_init_cb_t which
>>> takes platform_device pointer as parameter, and update the existing
>>> IRQCHIP_PLATFORM_DRIVER users accordingly.
>>>
>>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>>> Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
>>> Cc: Neil Armstrong <narmstrong@baylibre.com>
>>> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
>>
>> Could you copy all recipients on all 3 patches plus your cover letter
>> next time so we have the full context? Thanks!
>>
>> [snip]
>>
>>>
>>> -static int __init bcm7120_l2_intc_probe_7120(struct device_node *dn,
>>> +static int __init bcm7120_l2_intc_probe_7120(struct platform_device
>>> *pdev,
>>> struct device_node *parent)
>>> {
>>> - return bcm7120_l2_intc_probe(dn, parent,
>>> bcm7120_l2_intc_iomap_7120,
>>> + return bcm7120_l2_intc_probe(pdev->dev.of_node, parent,
>>> + bcm7120_l2_intc_iomap_7120,
>>> "BCM7120 L2");
>>
>> If you look further into that driver, you will see that we do something
>> like this in bcm7120_l2_intc_probe:
>>
>> pdev = of_find_device_by_node(dn);
>> if (!pdev) {
>> ret = -ENODEV;
>> goto out_free_data;
>> }
>>
>> which would be completely superfluous now that we pass a platform_device
>> directly. Can you rework your patch so as to eliminate that
>> of_find_device_by_ndoe() (and the companion put_device call)?
>
> Or just adopt the same construct in the MPM driver. At this stage, drivers
> requiring a platform_device are the minority.
Works for me.
--
Florian
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 3/3] irqchip: Add Qualcomm MPM controller driver
2021-12-02 12:21 ` [PATCH v3 3/3] irqchip: Add Qualcomm MPM controller driver Shawn Guo
@ 2021-12-03 0:00 ` kernel test robot
2021-12-06 9:46 ` Marc Zyngier
1 sibling, 0 replies; 17+ messages in thread
From: kernel test robot @ 2021-12-03 0:00 UTC (permalink / raw)
To: Shawn Guo, Marc Zyngier, Thomas Gleixner
Cc: kbuild-all, Maulik Shah, Bjorn Andersson, Loic Poulain,
linux-arm-msm, linux-kernel, Shawn Guo
Hi Shawn,
I love your patch! Yet something to improve:
[auto build test ERROR on tip/irq/core]
[also build test ERROR on linux/master linus/master v5.16-rc3 next-20211202]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Shawn-Guo/Add-Qualcomm-MPM-irqchip-driver-support/20211202-202327
base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 4946f15e8c334840bf277a0bf924371eae120fcd
config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20211203/202112030740.6LvRpZLQ-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/d575d38e462a2d09c7e36150fb9a638d591a9c91
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Shawn-Guo/Add-Qualcomm-MPM-irqchip-driver-support/20211202-202327
git checkout d575d38e462a2d09c7e36150fb9a638d591a9c91
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm SHELL=/bin/bash drivers/irqchip/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/irqchip/qcom-mpm.c: In function 'qcom_mpm_handler':
>> drivers/irqchip/qcom-mpm.c:284:33: error: implicit declaration of function 'irq_set_irqchip_state'; did you mean 'irq_set_chip_data'? [-Werror=implicit-function-declaration]
284 | irq_set_irqchip_state(d->irq,
| ^~~~~~~~~~~~~~~~~~~~~
| irq_set_chip_data
>> drivers/irqchip/qcom-mpm.c:285:49: error: 'IRQCHIP_STATE_PENDING' undeclared (first use in this function)
285 | IRQCHIP_STATE_PENDING, true);
| ^~~~~~~~~~~~~~~~~~~~~
drivers/irqchip/qcom-mpm.c:285:49: note: each undeclared identifier is reported only once for each function it appears in
drivers/irqchip/qcom-mpm.c: In function 'qcom_mpm_init':
>> drivers/irqchip/qcom-mpm.c:425:15: error: implicit declaration of function 'devm_request_irq'; did you mean 'can_request_irq'? [-Werror=implicit-function-declaration]
425 | ret = devm_request_irq(dev, irq, qcom_mpm_handler,
| ^~~~~~~~~~~~~~~~
| can_request_irq
>> drivers/irqchip/qcom-mpm.c:426:32: error: 'IRQF_TRIGGER_RISING' undeclared (first use in this function); did you mean 'IRQD_TRIGGER_MASK'?
426 | IRQF_TRIGGER_RISING | IRQF_NO_SUSPEND,
| ^~~~~~~~~~~~~~~~~~~
| IRQD_TRIGGER_MASK
>> drivers/irqchip/qcom-mpm.c:426:54: error: 'IRQF_NO_SUSPEND' undeclared (first use in this function)
426 | IRQF_TRIGGER_RISING | IRQF_NO_SUSPEND,
| ^~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +284 drivers/irqchip/qcom-mpm.c
264
265 /* Triggered by RPM when system resumes from deep sleep */
266 static irqreturn_t qcom_mpm_handler(int irq, void *dev_id)
267 {
268 struct qcom_mpm_priv *priv = dev_id;
269 unsigned long enable, pending;
270 int i, j;
271
272 for (i = 0; i < priv->reg_stride; i++) {
273 enable = qcom_mpm_read(priv, MPM_REG_ENABLE, i);
274 pending = qcom_mpm_read(priv, MPM_REG_STATUS, i);
275 pending &= enable;
276
277 for_each_set_bit(j, &pending, 32) {
278 unsigned int pin = 32 * i + j;
279 struct irq_desc *desc =
280 irq_resolve_mapping(priv->domain, pin);
281 struct irq_data *d = &desc->irq_data;
282
283 if (!irqd_is_level_type(d))
> 284 irq_set_irqchip_state(d->irq,
> 285 IRQCHIP_STATE_PENDING, true);
286
287 }
288 }
289
290 return IRQ_HANDLED;
291 }
292
293 static int qcom_mpm_enter_sleep(struct qcom_mpm_priv *priv)
294 {
295 int i, ret;
296
297 for (i = 0; i < priv->reg_stride; i++)
298 qcom_mpm_write(priv, MPM_REG_STATUS, i, 0);
299
300 /* Notify RPM to write vMPM into HW */
301 ret = mbox_send_message(priv->mbox_chan, NULL);
302 if (ret < 0)
303 return ret;
304
305 return 0;
306 }
307
308 static int qcom_mpm_cpu_pm_callback(struct notifier_block *nb,
309 unsigned long action, void *data)
310 {
311 struct qcom_mpm_priv *priv = container_of(nb, struct qcom_mpm_priv,
312 pm_nb);
313 int ret = NOTIFY_OK;
314 int cpus_in_pm;
315
316 switch (action) {
317 case CPU_PM_ENTER:
318 cpus_in_pm = atomic_inc_return(&priv->cpus_in_pm);
319 /*
320 * NOTE: comments for num_online_cpus() point out that it's
321 * only a snapshot so we need to be careful. It should be OK
322 * for us to use, though. It's important for us not to miss
323 * if we're the last CPU going down so it would only be a
324 * problem if a CPU went offline right after we did the check
325 * AND that CPU was not idle AND that CPU was the last non-idle
326 * CPU. That can't happen. CPUs would have to come out of idle
327 * before the CPU could go offline.
328 */
329 if (cpus_in_pm < num_online_cpus())
330 return NOTIFY_OK;
331 break;
332 case CPU_PM_ENTER_FAILED:
333 case CPU_PM_EXIT:
334 atomic_dec(&priv->cpus_in_pm);
335 return NOTIFY_OK;
336 default:
337 return NOTIFY_DONE;
338 }
339
340 /*
341 * It's likely we're on the last CPU. Grab the lock and write MPM for
342 * sleep. Grabbing the lock means that if we race with another CPU
343 * coming up we are still guaranteed to be safe.
344 */
345 if (raw_spin_trylock(&priv->lock)) {
346 if (qcom_mpm_enter_sleep(priv))
347 ret = NOTIFY_BAD;
348 raw_spin_unlock(&priv->lock);
349 } else {
350 /* Another CPU must be up */
351 return NOTIFY_OK;
352 }
353
354 if (ret == NOTIFY_BAD) {
355 /* Double-check if we're here because someone else is up */
356 if (cpus_in_pm < num_online_cpus())
357 ret = NOTIFY_OK;
358 else
359 /* We won't be called w/ CPU_PM_ENTER_FAILED */
360 atomic_dec(&priv->cpus_in_pm);
361 }
362
363 return ret;
364 }
365
366 static int qcom_mpm_init(struct platform_device *pdev,
367 struct device_node *parent,
368 const struct mpm_data *data)
369 {
370 struct irq_domain *parent_domain;
371 struct device *dev = &pdev->dev;
372 struct device_node *np = dev->of_node;
373 struct qcom_mpm_priv *priv;
374 unsigned int pin_num;
375 int irq;
376 int ret;
377
378 if (!data)
379 return -ENODEV;
380
381 priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
382 if (!priv)
383 return -ENOMEM;
384
385 priv->data = data;
386 pin_num = priv->data->pin_num;
387 priv->reg_stride = DIV_ROUND_UP(pin_num, 32);
388
389 raw_spin_lock_init(&priv->lock);
390
391 priv->base = devm_platform_ioremap_resource(pdev, 0);
392 if (!priv->base)
393 return PTR_ERR(priv->base);
394
395 irq = platform_get_irq(pdev, 0);
396 if (irq < 0)
397 return irq;
398
399 priv->mbox_client.dev = dev;
400 priv->mbox_chan = mbox_request_channel(&priv->mbox_client, 0);
401 if (IS_ERR(priv->mbox_chan)) {
402 ret = PTR_ERR(priv->mbox_chan);
403 dev_err(dev, "failed to acquire IPC channel: %d\n", ret);
404 return ret;
405 }
406
407 parent_domain = irq_find_host(parent);
408 if (!parent_domain) {
409 dev_err(dev, "failed to find MPM parent domain\n");
410 ret = -ENXIO;
411 goto free_mbox;
412 }
413
414 priv->domain = irq_domain_create_hierarchy(parent_domain,
415 IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP, pin_num,
416 of_node_to_fwnode(np), &qcom_mpm_ops, priv);
417 if (!priv->domain) {
418 dev_err(dev, "failed to create MPM domain\n");
419 ret = -ENOMEM;
420 goto free_mbox;
421 }
422
423 irq_domain_update_bus_token(priv->domain, DOMAIN_BUS_WAKEUP);
424
> 425 ret = devm_request_irq(dev, irq, qcom_mpm_handler,
> 426 IRQF_TRIGGER_RISING | IRQF_NO_SUSPEND,
427 "qcom_mpm", priv);
428 if (ret) {
429 dev_err(dev, "failed to request irq: %d\n", ret);
430 goto remove_domain;
431 }
432
433 priv->pm_nb.notifier_call = qcom_mpm_cpu_pm_callback;
434 cpu_pm_register_notifier(&priv->pm_nb);
435
436 dev_set_drvdata(dev, priv);
437
438 return 0;
439
440 remove_domain:
441 irq_domain_remove(priv->domain);
442 free_mbox:
443 mbox_free_channel(priv->mbox_chan);
444 return ret;
445 }
446
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/3] irqchip: Pass platform_device pointer to init_cb
2021-12-02 17:52 ` Florian Fainelli
2021-12-02 19:10 ` Marc Zyngier
@ 2021-12-06 6:35 ` Shawn Guo
1 sibling, 0 replies; 17+ messages in thread
From: Shawn Guo @ 2021-12-06 6:35 UTC (permalink / raw)
To: Florian Fainelli
Cc: Marc Zyngier, Thomas Gleixner, Maulik Shah, Bjorn Andersson,
Loic Poulain, linux-arm-msm, linux-kernel, Claudiu Beznea,
Neil Armstrong
On Thu, Dec 02, 2021 at 09:52:55AM -0800, Florian Fainelli wrote:
> On 12/2/21 4:21 AM, Shawn Guo wrote:
> > It makes sense to just pass device_node for callback in IRQCHIP_DECLARE
> > case, but not so much for IRQCHIP_PLATFORM_DRIVER one, because
> > platform_driver probe/init usually needs device pointer for various
> > purposes, e.g. resource allocation, service request, device prefixed
> > message output, etc. Create a new callback type irqchip_init_cb_t which
> > takes platform_device pointer as parameter, and update the existing
> > IRQCHIP_PLATFORM_DRIVER users accordingly.
> >
> > Cc: Florian Fainelli <f.fainelli@gmail.com>
> > Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
> > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
>
> Could you copy all recipients on all 3 patches plus your cover letter
> next time so we have the full context? Thanks!
>
> [snip]
>
> >
> > -static int __init bcm7120_l2_intc_probe_7120(struct device_node *dn,
> > +static int __init bcm7120_l2_intc_probe_7120(struct platform_device *pdev,
> > struct device_node *parent)
> > {
> > - return bcm7120_l2_intc_probe(dn, parent, bcm7120_l2_intc_iomap_7120,
> > + return bcm7120_l2_intc_probe(pdev->dev.of_node, parent,
> > + bcm7120_l2_intc_iomap_7120,
> > "BCM7120 L2");
>
> If you look further into that driver, you will see that we do something
> like this in bcm7120_l2_intc_probe:
>
> pdev = of_find_device_by_node(dn);
> if (!pdev) {
> ret = -ENODEV;
> goto out_free_data;
> }
>
> which would be completely superfluous now that we pass a platform_device
> directly. Can you rework your patch so as to eliminate that
> of_find_device_by_ndoe() (and the companion put_device call)?
Firstly, I do not see any companion put_device call in the driver.
Secondly, the existing code seems to have some problem in the "out"
order. The out_unmap should go before out_free_l1_data, right?
@@ -329,13 +323,13 @@ static int __init bcm7120_l2_intc_probe(struct device_node *dn,
out_free_domain:
irq_domain_remove(data->domain);
-out_free_l1_data:
- kfree(data->l1_data);
out_unmap:
for (idx = 0; idx < MAX_MAPPINGS; idx++) {
if (data->map_base[idx])
iounmap(data->map_base[idx]);
}
+out_free_l1_data:
+ kfree(data->l1_data);
out_free_data:
kfree(data);
return ret;
Shawn
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/3] irqchip: Pass platform_device pointer to init_cb
2021-12-02 19:10 ` Marc Zyngier
2021-12-02 21:44 ` Florian Fainelli
@ 2021-12-06 6:40 ` Shawn Guo
2021-12-06 8:29 ` Marc Zyngier
1 sibling, 1 reply; 17+ messages in thread
From: Shawn Guo @ 2021-12-06 6:40 UTC (permalink / raw)
To: Marc Zyngier
Cc: Florian Fainelli, Thomas Gleixner, Maulik Shah, Bjorn Andersson,
Loic Poulain, linux-arm-msm, linux-kernel, Claudiu Beznea,
Neil Armstrong
On Thu, Dec 02, 2021 at 07:10:04PM +0000, Marc Zyngier wrote:
> On 2021-12-02 17:52, Florian Fainelli wrote:
> > On 12/2/21 4:21 AM, Shawn Guo wrote:
> > > It makes sense to just pass device_node for callback in
> > > IRQCHIP_DECLARE
> > > case, but not so much for IRQCHIP_PLATFORM_DRIVER one, because
> > > platform_driver probe/init usually needs device pointer for various
> > > purposes, e.g. resource allocation, service request, device prefixed
> > > message output, etc. Create a new callback type irqchip_init_cb_t
> > > which
> > > takes platform_device pointer as parameter, and update the existing
> > > IRQCHIP_PLATFORM_DRIVER users accordingly.
> > >
> > > Cc: Florian Fainelli <f.fainelli@gmail.com>
> > > Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
> > > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> >
> > Could you copy all recipients on all 3 patches plus your cover letter
> > next time so we have the full context? Thanks!
> >
> > [snip]
> >
> > >
> > > -static int __init bcm7120_l2_intc_probe_7120(struct device_node *dn,
> > > +static int __init bcm7120_l2_intc_probe_7120(struct platform_device
> > > *pdev,
> > > struct device_node *parent)
> > > {
> > > - return bcm7120_l2_intc_probe(dn, parent, bcm7120_l2_intc_iomap_7120,
> > > + return bcm7120_l2_intc_probe(pdev->dev.of_node, parent,
> > > + bcm7120_l2_intc_iomap_7120,
> > > "BCM7120 L2");
> >
> > If you look further into that driver, you will see that we do something
> > like this in bcm7120_l2_intc_probe:
> >
> > pdev = of_find_device_by_node(dn);
> > if (!pdev) {
> > ret = -ENODEV;
> > goto out_free_data;
> > }
> >
> > which would be completely superfluous now that we pass a platform_device
> > directly. Can you rework your patch so as to eliminate that
> > of_find_device_by_ndoe() (and the companion put_device call)?
>
> Or just adopt the same construct in the MPM driver. At this stage, drivers
> requiring a platform_device are the minority.
Marc,
I need to ensure I understand you comment. Are you suggesting that I
keep IRQCHIP_MATCH() unchanged, and go back to the MPM driver
construction I used in v2?
Shawn
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/3] irqchip: Pass platform_device pointer to init_cb
2021-12-06 6:40 ` Shawn Guo
@ 2021-12-06 8:29 ` Marc Zyngier
0 siblings, 0 replies; 17+ messages in thread
From: Marc Zyngier @ 2021-12-06 8:29 UTC (permalink / raw)
To: Shawn Guo
Cc: Florian Fainelli, Thomas Gleixner, Maulik Shah, Bjorn Andersson,
Loic Poulain, linux-arm-msm, linux-kernel, Claudiu Beznea,
Neil Armstrong
On Mon, 06 Dec 2021 06:40:05 +0000,
Shawn Guo <shawn.guo@linaro.org> wrote:
>
> On Thu, Dec 02, 2021 at 07:10:04PM +0000, Marc Zyngier wrote:
> > On 2021-12-02 17:52, Florian Fainelli wrote:
> > > On 12/2/21 4:21 AM, Shawn Guo wrote:
> > > > It makes sense to just pass device_node for callback in
> > > > IRQCHIP_DECLARE
> > > > case, but not so much for IRQCHIP_PLATFORM_DRIVER one, because
> > > > platform_driver probe/init usually needs device pointer for various
> > > > purposes, e.g. resource allocation, service request, device prefixed
> > > > message output, etc. Create a new callback type irqchip_init_cb_t
> > > > which
> > > > takes platform_device pointer as parameter, and update the existing
> > > > IRQCHIP_PLATFORM_DRIVER users accordingly.
> > > >
> > > > Cc: Florian Fainelli <f.fainelli@gmail.com>
> > > > Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
> > > > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > >
> > > Could you copy all recipients on all 3 patches plus your cover letter
> > > next time so we have the full context? Thanks!
> > >
> > > [snip]
> > >
> > > >
> > > > -static int __init bcm7120_l2_intc_probe_7120(struct device_node *dn,
> > > > +static int __init bcm7120_l2_intc_probe_7120(struct platform_device
> > > > *pdev,
> > > > struct device_node *parent)
> > > > {
> > > > - return bcm7120_l2_intc_probe(dn, parent, bcm7120_l2_intc_iomap_7120,
> > > > + return bcm7120_l2_intc_probe(pdev->dev.of_node, parent,
> > > > + bcm7120_l2_intc_iomap_7120,
> > > > "BCM7120 L2");
> > >
> > > If you look further into that driver, you will see that we do something
> > > like this in bcm7120_l2_intc_probe:
> > >
> > > pdev = of_find_device_by_node(dn);
> > > if (!pdev) {
> > > ret = -ENODEV;
> > > goto out_free_data;
> > > }
> > >
> > > which would be completely superfluous now that we pass a platform_device
> > > directly. Can you rework your patch so as to eliminate that
> > > of_find_device_by_ndoe() (and the companion put_device call)?
> >
> > Or just adopt the same construct in the MPM driver. At this stage, drivers
> > requiring a platform_device are the minority.
>
> Marc,
>
> I need to ensure I understand you comment. Are you suggesting that I
> keep IRQCHIP_MATCH() unchanged, and go back to the MPM driver
> construction I used in v2?
No. I suggest that you leave the irqchip API as is (i.e. drop this
patch) and use of_find_device_by_node() in the MPM driver, just like
the Broadcom driver does. This should be enough for your use case.
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 3/3] irqchip: Add Qualcomm MPM controller driver
2021-12-02 12:21 ` [PATCH v3 3/3] irqchip: Add Qualcomm MPM controller driver Shawn Guo
2021-12-03 0:00 ` kernel test robot
@ 2021-12-06 9:46 ` Marc Zyngier
2021-12-06 13:15 ` Shawn Guo
1 sibling, 1 reply; 17+ messages in thread
From: Marc Zyngier @ 2021-12-06 9:46 UTC (permalink / raw)
To: Shawn Guo
Cc: Thomas Gleixner, Maulik Shah, Bjorn Andersson, Loic Poulain,
linux-arm-msm, linux-kernel
On Thu, 02 Dec 2021 12:21:22 +0000,
Shawn Guo <shawn.guo@linaro.org> wrote:
>
> Qualcomm SoCs based on the RPM architecture have a MSM Power Manager (MPM)
> in always-on domain. In addition to managing resources during sleep, the
> hardware also has an interrupt controller that monitors the interrupts
> when the system is asleep, wakes up the APSS when one of these interrupts
> occur and replays it to GIC after it becomes operational.
>
> It adds an irqchip driver for this interrupt controller, and here are
> some notes about it.
>
> - For given SoC, a fixed number of MPM pins are supported, e.g. 96 pins
> on QCM2290. Each of these MPM pins can be either a MPM_GIC pin or
> a MPM_GPIO pin. The mapping between MPM_GIC pin and GIC interrupt
> is defined by SoC, as well as the mapping between MPM_GPIO pin and
> GPIO number. The former mapping can be found as the SoC data in this
> MPM driver, while the latter can be found as the msm_gpio_wakeirq_map[]
> in TLMM driver.
>
> - All the register settings are done by APSS on an internal memory
> region called vMPM, and RPM will flush them into hardware after it
> receives a mailbox/IPC notification from APSS.
>
> - When SoC gets awake from sleep mode, the driver will receive an
> interrupt from RPM, so that it can replay interrupt for particular
> polarity.
>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
> drivers/irqchip/Kconfig | 8 +
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/qcom-mpm.c | 481 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 490 insertions(+)
> create mode 100644 drivers/irqchip/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..0e2e10467e28 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -94,6 +94,7 @@ obj-$(CONFIG_MESON_IRQ_GPIO) += irq-meson-gpio.o
> obj-$(CONFIG_GOLDFISH_PIC) += irq-goldfish-pic.o
> obj-$(CONFIG_NDS32) += irq-ativic32.o
> obj-$(CONFIG_QCOM_PDC) += qcom-pdc.o
> +obj-$(CONFIG_QCOM_MPM) += qcom-mpm.o
> obj-$(CONFIG_CSKY_MPINTC) += irq-csky-mpintc.o
> obj-$(CONFIG_CSKY_APB_INTC) += irq-csky-apb-intc.o
> obj-$(CONFIG_RISCV_INTC) += irq-riscv-intc.o
> diff --git a/drivers/irqchip/qcom-mpm.c b/drivers/irqchip/qcom-mpm.c
> new file mode 100644
> index 000000000000..4a0dac2e2c6e
> --- /dev/null
> +++ b/drivers/irqchip/qcom-mpm.c
> @@ -0,0 +1,481 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2021, Linaro Limited
> + * Copyright (c) 2010-2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/atomic.h>
> +#include <linux/cpu_pm.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqchip/arm-gic-v3.h>
> +#include <linux/irqdomain.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/soc/qcom/irq.h>
> +#include <linux/spinlock.h>
> +
> +/*
> + * vMPM register layout:
> + *
> + * 31 0
> + * +--------------------------------+
> + * | TIMER0 | 0x00
> + * +--------------------------------+
> + * | TIMER1 | 0x04
> + * +--------------------------------+
> + * | ENABLE0 | 0x08
> + * +--------------------------------+
> + * | ... | ...
> + * +--------------------------------+
> + * | ENABLEn |
> + * +--------------------------------+
> + * | FALLING_EDGE0 |
> + * +--------------------------------+
> + * | ... |
> + * +--------------------------------+
> + * | STATUSn |
> + * +--------------------------------+
> + *
> + * n = DIV_ROUND_UP(pin_num, 32)
> + *
> + */
> +#define MPM_REG_ENABLE 0
> +#define MPM_REG_FALLING_EDGE 1
> +#define MPM_REG_RISING_EDGE 2
> +#define MPM_REG_POLARITY 3
> +#define MPM_REG_STATUS 4
> +
> +#define MPM_NO_PARENT_IRQ ~0UL
> +
> +/* MPM pin and its GIC hwirq */
> +struct mpm_pin {
> + int pin;
> + irq_hw_number_t hwirq;
> +};
> +
> +struct mpm_data {
> + unsigned int pin_num;
> + const struct mpm_pin *gic_pins;
> +};
> +
> +struct qcom_mpm_priv {
> + void __iomem *base;
> + raw_spinlock_t lock;
> + struct mbox_client mbox_client;
> + struct mbox_chan *mbox_chan;
> + const struct mpm_data *data;
> + unsigned int reg_stride;
> + struct irq_domain *domain;
> + struct notifier_block pm_nb;
> + atomic_t cpus_in_pm;
> +};
> +
> +static inline u32
Drop all the inline attributes in this file. The compiler will inline
it if required.
> +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 inline void
> +qcom_mpm_write(struct qcom_mpm_priv *priv, unsigned int reg,
> + unsigned int index, u32 val)
> +{
> + unsigned int offset = (reg * priv->reg_stride + index + 2) * 4;
> +
> + writel_relaxed(val, priv->base + offset);
> +
> + /* Ensure the write is completed */
> + wmb();
> +}
> +
> +static inline 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;
> + u32 val;
> +
> + raw_spin_lock(&priv->lock);
Hum. This function is called from mask/unmask and acts on a shared
register. Mask/unmask can be called from an interrupt handler on a
different interrupt sharing the same register, and you will end-up in
a deadlock if this happens on the same CPU.
You need to use the _irqsave/_irqrestore flavour everywhere.
> +
> + 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(&priv->lock);
> +}
> +
> +static void qcom_mpm_mask(struct irq_data *d)
> +{
> + qcom_mpm_enable_irq(d, false);
> +
> + if (d->parent_data)
> + irq_chip_mask_parent(d);
> +}
> +
> +static void qcom_mpm_unmask(struct irq_data *d)
> +{
> + qcom_mpm_enable_irq(d, true);
> +
> + if (d->parent_data)
> + irq_chip_unmask_parent(d);
> +}
> +
> +static inline void
> +mpm_set_type(struct qcom_mpm_priv *priv, bool set, unsigned int reg,
> + unsigned int index, unsigned int shift)
> +{
> + u32 val;
> +
> + raw_spin_lock(&priv->lock);
> +
> + 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(&priv->lock);
> +}
> +
> +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 irq_hw_number_t get_parent_hwirq(struct qcom_mpm_priv *priv, int pin)
> +{
> + const struct mpm_pin *gic_pins = priv->data->gic_pins;
> + int i;
> +
> + for (i = 0; gic_pins[i].pin >= 0; i++) {
> + int p = gic_pins[i].pin;
> +
> + if (p < 0)
> + break;
> +
> + if (p == pin)
> + return gic_pins[i].hwirq;
> + }
> +
> + return MPM_NO_PARENT_IRQ;
> +}
> +
> +static int qcom_mpm_alloc(struct irq_domain *domain, unsigned int virq,
> + unsigned int nr_irqs, void *data)
> +{
> + struct qcom_mpm_priv *priv = domain->host_data;
> + struct irq_fwspec *fwspec = data;
> + struct irq_fwspec parent_fwspec;
> + irq_hw_number_t parent_hwirq;
> + irq_hw_number_t hwirq;
> + unsigned int type;
> + int ret;
> +
> + ret = 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;
> +
> + parent_hwirq = get_parent_hwirq(priv, hwirq);
> + if (parent_hwirq == MPM_NO_PARENT_IRQ)
> + return irq_domain_disconnect_hierarchy(domain->parent, virq);
> +
> + if (type & IRQ_TYPE_EDGE_BOTH)
> + type = IRQ_TYPE_EDGE_RISING;
> +
> + if (type & IRQ_TYPE_LEVEL_MASK)
> + type = IRQ_TYPE_LEVEL_HIGH;
> +
> + parent_fwspec.fwnode = domain->parent->fwnode;
> + parent_fwspec.param_count = 3;
> + parent_fwspec.param[0] = 0;
> + parent_fwspec.param[1] = parent_hwirq;
> + parent_fwspec.param[2] = type;
> +
> + return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
> + &parent_fwspec);
> +}
> +
> +static const struct irq_domain_ops qcom_mpm_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;
> + int i, j;
> +
> + for (i = 0; i < priv->reg_stride; i++) {
> + enable = qcom_mpm_read(priv, MPM_REG_ENABLE, i);
> + pending = qcom_mpm_read(priv, MPM_REG_STATUS, i);
> + pending &= enable;
> +
> + for_each_set_bit(j, &pending, 32) {
> + unsigned int pin = 32 * i + j;
> + struct irq_desc *desc =
> + irq_resolve_mapping(priv->domain, pin);
Assignments on a single line, please.
> + struct irq_data *d = &desc->irq_data;
> +
> + if (!irqd_is_level_type(d))
> + irq_set_irqchip_state(d->irq,
> + IRQCHIP_STATE_PENDING, true);
> +
> + }
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +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 */
What do you mean by 'into HW'? We just did that, right? or are these
registers just fake and most of the stuff is in the RPM?
> + ret = mbox_send_message(priv->mbox_chan, NULL);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int qcom_mpm_cpu_pm_callback(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + struct qcom_mpm_priv *priv = container_of(nb, struct qcom_mpm_priv,
> + pm_nb);
> + int ret = NOTIFY_OK;
> + int cpus_in_pm;
> +
> + switch (action) {
> + case CPU_PM_ENTER:
> + cpus_in_pm = atomic_inc_return(&priv->cpus_in_pm);
> + /*
> + * NOTE: comments for num_online_cpus() point out that it's
> + * only a snapshot so we need to be careful. It should be OK
> + * for us to use, though. It's important for us not to miss
> + * if we're the last CPU going down so it would only be a
> + * problem if a CPU went offline right after we did the check
> + * AND that CPU was not idle AND that CPU was the last non-idle
> + * CPU. That can't happen. CPUs would have to come out of idle
> + * before the CPU could go offline.
> + */
> + if (cpus_in_pm < num_online_cpus())
> + return NOTIFY_OK;
> + break;
I'm really not keen on this sort of tracking in an irqchip driver.
Nobody else needs this. This is also copy-pasted (without even
mentioning it) from rpmh_rsc_cpu_pm_callback(). Why is that logic
needed twice, given that the RPM is also involved is this sequence?
> + case CPU_PM_ENTER_FAILED:
> + case CPU_PM_EXIT:
> + atomic_dec(&priv->cpus_in_pm);
> + return NOTIFY_OK;
> + default:
> + return NOTIFY_DONE;
> + }
> +
> + /*
> + * It's likely we're on the last CPU. Grab the lock and write MPM for
> + * sleep. Grabbing the lock means that if we race with another CPU
> + * coming up we are still guaranteed to be safe.
> + */
> + if (raw_spin_trylock(&priv->lock)) {
> + if (qcom_mpm_enter_sleep(priv))
> + ret = NOTIFY_BAD;
> + raw_spin_unlock(&priv->lock);
> + } else {
> + /* Another CPU must be up */
> + return NOTIFY_OK;
> + }
> +
> + if (ret == NOTIFY_BAD) {
> + /* Double-check if we're here because someone else is up */
> + if (cpus_in_pm < num_online_cpus())
> + ret = NOTIFY_OK;
> + else
> + /* We won't be called w/ CPU_PM_ENTER_FAILED */
> + atomic_dec(&priv->cpus_in_pm);
> + }
> +
> + return ret;
> +}
> +
> +static int qcom_mpm_init(struct platform_device *pdev,
> + struct device_node *parent,
> + const struct mpm_data *data)
> +{
> + struct irq_domain *parent_domain;
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + struct qcom_mpm_priv *priv;
> + unsigned int pin_num;
> + int irq;
> + int ret;
> +
> + if (!data)
> + return -ENODEV;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->data = data;
> + pin_num = priv->data->pin_num;
> + priv->reg_stride = DIV_ROUND_UP(pin_num, 32);
> +
> + 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;
> +
> + 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_num,
> + 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_TRIGGER_RISING | IRQF_NO_SUSPEND,
> + "qcom_mpm", priv);
> + if (ret) {
> + dev_err(dev, "failed to request irq: %d\n", ret);
> + goto remove_domain;
> + }
> +
> + priv->pm_nb.notifier_call = qcom_mpm_cpu_pm_callback;
> + cpu_pm_register_notifier(&priv->pm_nb);
> +
> + dev_set_drvdata(dev, priv);
> +
> + return 0;
> +
> +remove_domain:
> + irq_domain_remove(priv->domain);
> +free_mbox:
> + mbox_free_channel(priv->mbox_chan);
> + return ret;
> +}
> +
> +/*
> + * The mapping between MPM_GIC pin and GIC SPI number on QCM2290. It's taken
> + * from downstream qcom-mpm-scuba.c with a little transform on the GIC
> + * SPI numbers (the second column). Due to the binding difference from
> + * the downstream, where GIC SPI numbering starts from 32, we expect the
> + * numbering starts from 0 here, and that's why we have the number minus 32
> + * comparing to the downstream.
This doesn't answer my earlier question: is this list complete? Or is
it likely to change? Also, why is that in not in the device tree,
given that it is likely to change from one SoC to another?
> + */
> +const struct mpm_pin qcm2290_gic_pins[] = {
> + { 2, 275 }, /* tsens0_tsens_upper_lower_int */
> + { 5, 296 }, /* lpass_irq_out_sdc */
> + { 12, 422 }, /* b3_lfps_rxterm_irq */
> + { 24, 79 }, /* bi_px_lpi_1_aoss_mx */
> + { 86, 183 }, /* mpm_wake,spmi_m */
> + { 90, 260 }, /* eud_p0_dpse_int_mx */
> + { 91, 260 }, /* eud_p0_dmse_int_mx */
> + { -1 },
> +};
> +
> +const struct mpm_data qcm2290_data = {
> + .pin_num = 96,
> + .gic_pins = qcm2290_gic_pins,
> +};
> +
> +static int qcm2290_mpm_init(struct platform_device *pdev,
> + struct device_node *parent)
> +{
> + return qcom_mpm_init(pdev, parent, &qcm2290_data);
> +}
> +
> +IRQCHIP_PLATFORM_DRIVER_BEGIN(qcom_mpm)
> +IRQCHIP_MATCH("qcom,qcm2290-mpm", qcm2290_mpm_init)
> +IRQCHIP_PLATFORM_DRIVER_END(qcom_mpm)
> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. MSM Power Manager");
> +MODULE_LICENSE("GPL v2");
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 3/3] irqchip: Add Qualcomm MPM controller driver
2021-12-06 9:46 ` Marc Zyngier
@ 2021-12-06 13:15 ` Shawn Guo
2021-12-06 13:48 ` Marc Zyngier
0 siblings, 1 reply; 17+ messages in thread
From: Shawn Guo @ 2021-12-06 13:15 UTC (permalink / raw)
To: Marc Zyngier
Cc: Thomas Gleixner, Maulik Shah, Bjorn Andersson, Loic Poulain,
linux-arm-msm, linux-kernel
On Mon, Dec 06, 2021 at 09:46:29AM +0000, Marc Zyngier wrote:
[...]
> > +/* Triggered by RPM when system resumes from deep sleep */
> > +static irqreturn_t qcom_mpm_handler(int irq, void *dev_id)
> > +{
> > + struct qcom_mpm_priv *priv = dev_id;
> > + unsigned long enable, pending;
> > + int i, j;
> > +
> > + for (i = 0; i < priv->reg_stride; i++) {
> > + enable = qcom_mpm_read(priv, MPM_REG_ENABLE, i);
> > + pending = qcom_mpm_read(priv, MPM_REG_STATUS, i);
> > + pending &= enable;
> > +
> > + for_each_set_bit(j, &pending, 32) {
> > + unsigned int pin = 32 * i + j;
> > + struct irq_desc *desc =
> > + irq_resolve_mapping(priv->domain, pin);
>
> Assignments on a single line, please.
This is to avoid over 80 columns check patch warning.
>
> > + struct irq_data *d = &desc->irq_data;
> > +
> > + if (!irqd_is_level_type(d))
> > + irq_set_irqchip_state(d->irq,
> > + IRQCHIP_STATE_PENDING, true);
> > +
> > + }
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +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 */
>
> What do you mean by 'into HW'? We just did that, right? or are these
> registers just fake and most of the stuff is in the RPM?
I have a note about this in commit log.
- All the register settings are done by APSS on an internal memory
region called vMPM, and RPM will flush them into hardware after it
receives a mailbox/IPC notification from APSS.
So yes, these registers are fake/virtual in memory, and RPM will
actually flush the values into the MPM hardware block.
>
> > + ret = mbox_send_message(priv->mbox_chan, NULL);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static int qcom_mpm_cpu_pm_callback(struct notifier_block *nb,
> > + unsigned long action, void *data)
> > +{
> > + struct qcom_mpm_priv *priv = container_of(nb, struct qcom_mpm_priv,
> > + pm_nb);
> > + int ret = NOTIFY_OK;
> > + int cpus_in_pm;
> > +
> > + switch (action) {
> > + case CPU_PM_ENTER:
> > + cpus_in_pm = atomic_inc_return(&priv->cpus_in_pm);
> > + /*
> > + * NOTE: comments for num_online_cpus() point out that it's
> > + * only a snapshot so we need to be careful. It should be OK
> > + * for us to use, though. It's important for us not to miss
> > + * if we're the last CPU going down so it would only be a
> > + * problem if a CPU went offline right after we did the check
> > + * AND that CPU was not idle AND that CPU was the last non-idle
> > + * CPU. That can't happen. CPUs would have to come out of idle
> > + * before the CPU could go offline.
> > + */
> > + if (cpus_in_pm < num_online_cpus())
> > + return NOTIFY_OK;
> > + break;
>
> I'm really not keen on this sort of tracking in an irqchip driver.
> Nobody else needs this. This is also copy-pasted (without even
> mentioning it) from rpmh_rsc_cpu_pm_callback(). Why is that logic
> needed twice, given that the RPM is also involved is this sequence?
Yes, this is copy-pasted from rpmh-rsc and I should have mentioned that
in some way. But rpmh-rsc is a driver specific to RPM-Hardened (RPMH)
which can be found on Qualcomm high-range SoCs like SDM845, SC7180. And
on these RPMH based SoCs, PDC works as the wake-up interrupt controller.
However, on mid-range SoCs like SDM660, QCM2290, they run a different
combination, that is RPM + MPM. Note - RPMH and RPM are two similar but
different and separate blocks.
What we are implementing is for RPM + MPM based SoCs, and rpmh-rsc is
irrelevant here. The same cpu_pm tracking logic can be reused though.
>
> > + case CPU_PM_ENTER_FAILED:
> > + case CPU_PM_EXIT:
> > + atomic_dec(&priv->cpus_in_pm);
> > + return NOTIFY_OK;
> > + default:
> > + return NOTIFY_DONE;
> > + }
> > +
> > + /*
> > + * It's likely we're on the last CPU. Grab the lock and write MPM for
> > + * sleep. Grabbing the lock means that if we race with another CPU
> > + * coming up we are still guaranteed to be safe.
> > + */
> > + if (raw_spin_trylock(&priv->lock)) {
> > + if (qcom_mpm_enter_sleep(priv))
> > + ret = NOTIFY_BAD;
> > + raw_spin_unlock(&priv->lock);
> > + } else {
> > + /* Another CPU must be up */
> > + return NOTIFY_OK;
> > + }
> > +
> > + if (ret == NOTIFY_BAD) {
> > + /* Double-check if we're here because someone else is up */
> > + if (cpus_in_pm < num_online_cpus())
> > + ret = NOTIFY_OK;
> > + else
> > + /* We won't be called w/ CPU_PM_ENTER_FAILED */
> > + atomic_dec(&priv->cpus_in_pm);
> > + }
> > +
> > + return ret;
> > +}
[...]
> > +/*
> > + * The mapping between MPM_GIC pin and GIC SPI number on QCM2290. It's taken
> > + * from downstream qcom-mpm-scuba.c with a little transform on the GIC
> > + * SPI numbers (the second column). Due to the binding difference from
> > + * the downstream, where GIC SPI numbering starts from 32, we expect the
> > + * numbering starts from 0 here, and that's why we have the number minus 32
> > + * comparing to the downstream.
>
> This doesn't answer my earlier question: is this list complete? Or is
> it likely to change?
Yes, it's complete for QCM2290.
> Also, why is that in not in the device tree,
> given that it is likely to change from one SoC to another?
Yes, this is a SoC specific data, and will be different from one SoC to
another. Different subsystem maintainers have different views on such
SoC data. Some think they belong to kernel/driver and can be matched
using SoC specific compatible, while others prefer to have them in DT.
As I mentioned in the commit log, this SoC data actually consists of two
parts:
1) Map of MPM_GIC pin <--> GIC interrupt number
2) Map of MPM_GPIO pin <--> GPIO number
Since we already have map 2) defined in pinctrl driver rather than DT,
I put map 1) in MPM driver.
Shawn
>
> > + */
> > +const struct mpm_pin qcm2290_gic_pins[] = {
> > + { 2, 275 }, /* tsens0_tsens_upper_lower_int */
> > + { 5, 296 }, /* lpass_irq_out_sdc */
> > + { 12, 422 }, /* b3_lfps_rxterm_irq */
> > + { 24, 79 }, /* bi_px_lpi_1_aoss_mx */
> > + { 86, 183 }, /* mpm_wake,spmi_m */
> > + { 90, 260 }, /* eud_p0_dpse_int_mx */
> > + { 91, 260 }, /* eud_p0_dmse_int_mx */
> > + { -1 },
> > +};
> > +
> > +const struct mpm_data qcm2290_data = {
> > + .pin_num = 96,
> > + .gic_pins = qcm2290_gic_pins,
> > +};
> > +
> > +static int qcm2290_mpm_init(struct platform_device *pdev,
> > + struct device_node *parent)
> > +{
> > + return qcom_mpm_init(pdev, parent, &qcm2290_data);
> > +}
> > +
> > +IRQCHIP_PLATFORM_DRIVER_BEGIN(qcom_mpm)
> > +IRQCHIP_MATCH("qcom,qcm2290-mpm", qcm2290_mpm_init)
> > +IRQCHIP_PLATFORM_DRIVER_END(qcom_mpm)
> > +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. MSM Power Manager");
> > +MODULE_LICENSE("GPL v2");
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 3/3] irqchip: Add Qualcomm MPM controller driver
2021-12-06 13:15 ` Shawn Guo
@ 2021-12-06 13:48 ` Marc Zyngier
2021-12-07 9:48 ` Shawn Guo
0 siblings, 1 reply; 17+ messages in thread
From: Marc Zyngier @ 2021-12-06 13:48 UTC (permalink / raw)
To: Shawn Guo
Cc: Thomas Gleixner, Maulik Shah, Bjorn Andersson, Loic Poulain,
linux-arm-msm, linux-kernel
On Mon, 06 Dec 2021 13:15:31 +0000,
Shawn Guo <shawn.guo@linaro.org> wrote:
>
> On Mon, Dec 06, 2021 at 09:46:29AM +0000, Marc Zyngier wrote:
> [...]
> > > +/* Triggered by RPM when system resumes from deep sleep */
> > > +static irqreturn_t qcom_mpm_handler(int irq, void *dev_id)
> > > +{
> > > + struct qcom_mpm_priv *priv = dev_id;
> > > + unsigned long enable, pending;
> > > + int i, j;
> > > +
> > > + for (i = 0; i < priv->reg_stride; i++) {
> > > + enable = qcom_mpm_read(priv, MPM_REG_ENABLE, i);
> > > + pending = qcom_mpm_read(priv, MPM_REG_STATUS, i);
> > > + pending &= enable;
> > > +
> > > + for_each_set_bit(j, &pending, 32) {
> > > + unsigned int pin = 32 * i + j;
> > > + struct irq_desc *desc =
> > > + irq_resolve_mapping(priv->domain, pin);
> >
> > Assignments on a single line, please.
>
> This is to avoid over 80 columns check patch warning.
I don't care about what checkpatch says. This is unreadable and
ungrepable.
>
> >
> > > + struct irq_data *d = &desc->irq_data;
> > > +
> > > + if (!irqd_is_level_type(d))
> > > + irq_set_irqchip_state(d->irq,
> > > + IRQCHIP_STATE_PENDING, true);
> > > +
> > > + }
> > > + }
> > > +
> > > + return IRQ_HANDLED;
> > > +}
> > > +
> > > +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 */
> >
> > What do you mean by 'into HW'? We just did that, right? or are these
> > registers just fake and most of the stuff is in the RPM?
>
> I have a note about this in commit log.
>
> - All the register settings are done by APSS on an internal memory
> region called vMPM, and RPM will flush them into hardware after it
> receives a mailbox/IPC notification from APSS.
>
> So yes, these registers are fake/virtual in memory, and RPM will
> actually flush the values into the MPM hardware block.
Then why are you using MMIO accessors all over the place if this is
just RAM? Who *owns* this memory? Is it normal DRAM? Or some flops
exposed by a device? Why isn't the state simply communicated over the
mailbox instead?
>
> >
> > > + ret = mbox_send_message(priv->mbox_chan, NULL);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int qcom_mpm_cpu_pm_callback(struct notifier_block *nb,
> > > + unsigned long action, void *data)
> > > +{
> > > + struct qcom_mpm_priv *priv = container_of(nb, struct qcom_mpm_priv,
> > > + pm_nb);
> > > + int ret = NOTIFY_OK;
> > > + int cpus_in_pm;
> > > +
> > > + switch (action) {
> > > + case CPU_PM_ENTER:
> > > + cpus_in_pm = atomic_inc_return(&priv->cpus_in_pm);
> > > + /*
> > > + * NOTE: comments for num_online_cpus() point out that it's
> > > + * only a snapshot so we need to be careful. It should be OK
> > > + * for us to use, though. It's important for us not to miss
> > > + * if we're the last CPU going down so it would only be a
> > > + * problem if a CPU went offline right after we did the check
> > > + * AND that CPU was not idle AND that CPU was the last non-idle
> > > + * CPU. That can't happen. CPUs would have to come out of idle
> > > + * before the CPU could go offline.
> > > + */
> > > + if (cpus_in_pm < num_online_cpus())
> > > + return NOTIFY_OK;
> > > + break;
> >
> > I'm really not keen on this sort of tracking in an irqchip driver.
> > Nobody else needs this. This is also copy-pasted (without even
> > mentioning it) from rpmh_rsc_cpu_pm_callback(). Why is that logic
> > needed twice, given that the RPM is also involved is this sequence?
>
> Yes, this is copy-pasted from rpmh-rsc and I should have mentioned that
> in some way. But rpmh-rsc is a driver specific to RPM-Hardened (RPMH)
> which can be found on Qualcomm high-range SoCs like SDM845, SC7180. And
> on these RPMH based SoCs, PDC works as the wake-up interrupt controller.
> However, on mid-range SoCs like SDM660, QCM2290, they run a different
> combination, that is RPM + MPM. Note - RPMH and RPM are two similar but
> different and separate blocks.
>
> What we are implementing is for RPM + MPM based SoCs, and rpmh-rsc is
> irrelevant here. The same cpu_pm tracking logic can be reused though.
No, I really don't think this is the right place for this.
This is generic CPU power management code (finding out who is the last
man standing), and I don't want it buried into some random drivers. If
you have a need for this kind of logic, build it *outside* of the
driver infrastructure, and into the CPU PM code. Interrupt controllers
have no business with this stuff.
> >
> > > + case CPU_PM_ENTER_FAILED:
> > > + case CPU_PM_EXIT:
> > > + atomic_dec(&priv->cpus_in_pm);
> > > + return NOTIFY_OK;
> > > + default:
> > > + return NOTIFY_DONE;
> > > + }
> > > +
> > > + /*
> > > + * It's likely we're on the last CPU. Grab the lock and write MPM for
> > > + * sleep. Grabbing the lock means that if we race with another CPU
> > > + * coming up we are still guaranteed to be safe.
> > > + */
> > > + if (raw_spin_trylock(&priv->lock)) {
> > > + if (qcom_mpm_enter_sleep(priv))
> > > + ret = NOTIFY_BAD;
> > > + raw_spin_unlock(&priv->lock);
> > > + } else {
> > > + /* Another CPU must be up */
> > > + return NOTIFY_OK;
> > > + }
> > > +
> > > + if (ret == NOTIFY_BAD) {
> > > + /* Double-check if we're here because someone else is up */
> > > + if (cpus_in_pm < num_online_cpus())
> > > + ret = NOTIFY_OK;
> > > + else
> > > + /* We won't be called w/ CPU_PM_ENTER_FAILED */
> > > + atomic_dec(&priv->cpus_in_pm);
> > > + }
> > > +
> > > + return ret;
> > > +}
>
> [...]
>
> > > +/*
> > > + * The mapping between MPM_GIC pin and GIC SPI number on QCM2290. It's taken
> > > + * from downstream qcom-mpm-scuba.c with a little transform on the GIC
> > > + * SPI numbers (the second column). Due to the binding difference from
> > > + * the downstream, where GIC SPI numbering starts from 32, we expect the
> > > + * numbering starts from 0 here, and that's why we have the number minus 32
> > > + * comparing to the downstream.
> >
> > This doesn't answer my earlier question: is this list complete? Or is
> > it likely to change?
>
> Yes, it's complete for QCM2290.
>
> > Also, why is that in not in the device tree,
> > given that it is likely to change from one SoC to another?
>
> Yes, this is a SoC specific data, and will be different from one SoC to
> another. Different subsystem maintainers have different views on such
> SoC data. Some think they belong to kernel/driver and can be matched
> using SoC specific compatible, while others prefer to have them in DT.
>
> As I mentioned in the commit log, this SoC data actually consists of two
> parts:
>
> 1) Map of MPM_GIC pin <--> GIC interrupt number
> 2) Map of MPM_GPIO pin <--> GPIO number
>
> Since we already have map 2) defined in pinctrl driver rather than DT,
> I put map 1) in MPM driver.
That's a pinctrl decision. I don't want more static tables in random
drivers on the irqchip side. You are just reinventing the board files
we got rid of 10 years ago, only spread all over the various drivers
instead of arch/arm/mach-*/...
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 3/3] irqchip: Add Qualcomm MPM controller driver
2021-12-06 13:48 ` Marc Zyngier
@ 2021-12-07 9:48 ` Shawn Guo
2021-12-07 10:16 ` Marc Zyngier
0 siblings, 1 reply; 17+ messages in thread
From: Shawn Guo @ 2021-12-07 9:48 UTC (permalink / raw)
To: Marc Zyngier
Cc: Thomas Gleixner, Maulik Shah, Bjorn Andersson, Loic Poulain,
linux-arm-msm, linux-kernel
On Mon, Dec 06, 2021 at 01:48:12PM +0000, Marc Zyngier wrote:
> > > > +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 */
> > >
> > > What do you mean by 'into HW'? We just did that, right? or are these
> > > registers just fake and most of the stuff is in the RPM?
> >
> > I have a note about this in commit log.
> >
> > - All the register settings are done by APSS on an internal memory
> > region called vMPM, and RPM will flush them into hardware after it
> > receives a mailbox/IPC notification from APSS.
> >
> > So yes, these registers are fake/virtual in memory, and RPM will
> > actually flush the values into the MPM hardware block.
>
> Then why are you using MMIO accessors all over the place if this is
> just RAM? Who *owns* this memory? Is it normal DRAM? Or some flops
> exposed by a device? Why isn't the state simply communicated over the
> mailbox instead?
It's a piece of internal memory (SRAM) which can be access by AP and
RPM. The communication mechanism is defined by SoC/RPM design, and we
can do nothing but following the procedure.
Shawn
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 3/3] irqchip: Add Qualcomm MPM controller driver
2021-12-07 9:48 ` Shawn Guo
@ 2021-12-07 10:16 ` Marc Zyngier
2021-12-08 10:04 ` Shawn Guo
0 siblings, 1 reply; 17+ messages in thread
From: Marc Zyngier @ 2021-12-07 10:16 UTC (permalink / raw)
To: Shawn Guo
Cc: Thomas Gleixner, Maulik Shah, Bjorn Andersson, Loic Poulain,
linux-arm-msm, linux-kernel
On Tue, 07 Dec 2021 09:48:36 +0000,
Shawn Guo <shawn.guo@linaro.org> wrote:
>
> On Mon, Dec 06, 2021 at 01:48:12PM +0000, Marc Zyngier wrote:
> > > > > +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 */
> > > >
> > > > What do you mean by 'into HW'? We just did that, right? or are these
> > > > registers just fake and most of the stuff is in the RPM?
> > >
> > > I have a note about this in commit log.
> > >
> > > - All the register settings are done by APSS on an internal memory
> > > region called vMPM, and RPM will flush them into hardware after it
> > > receives a mailbox/IPC notification from APSS.
> > >
> > > So yes, these registers are fake/virtual in memory, and RPM will
> > > actually flush the values into the MPM hardware block.
> >
> > Then why are you using MMIO accessors all over the place if this is
> > just RAM? Who *owns* this memory? Is it normal DRAM? Or some flops
> > exposed by a device? Why isn't the state simply communicated over the
> > mailbox instead?
>
> It's a piece of internal memory (SRAM) which can be access by AP and
> RPM. The communication mechanism is defined by SoC/RPM design, and we
> can do nothing but following the procedure.
Then the procedure needs to be documented:
- Who owns the memory at any given time?
- What are the events that trigger a change of ownership?
- What are the messages exchanged between these entities?
- What is the synchronisation mechanism between the various processing
entities (MPM. RPM, APSS...)?
- Is the per-interrupt tracking a HW requirement or a SW
implementation choice? Could this instead be a one-shot operation
iterating over the whole state?
All this needs to be explained so that it is maintainable, because I'm
getting tired of drivers that mimic the QC downstream code without
justification nor documentation to support the implementation.
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 3/3] irqchip: Add Qualcomm MPM controller driver
2021-12-07 10:16 ` Marc Zyngier
@ 2021-12-08 10:04 ` Shawn Guo
0 siblings, 0 replies; 17+ messages in thread
From: Shawn Guo @ 2021-12-08 10:04 UTC (permalink / raw)
To: Marc Zyngier
Cc: Thomas Gleixner, Maulik Shah, Bjorn Andersson, Loic Poulain,
linux-arm-msm, linux-kernel
On Tue, Dec 07, 2021 at 10:16:32AM +0000, Marc Zyngier wrote:
> On Tue, 07 Dec 2021 09:48:36 +0000,
> Shawn Guo <shawn.guo@linaro.org> wrote:
> >
> > On Mon, Dec 06, 2021 at 01:48:12PM +0000, Marc Zyngier wrote:
> > > > > > +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 */
> > > > >
> > > > > What do you mean by 'into HW'? We just did that, right? or are these
> > > > > registers just fake and most of the stuff is in the RPM?
> > > >
> > > > I have a note about this in commit log.
> > > >
> > > > - All the register settings are done by APSS on an internal memory
> > > > region called vMPM, and RPM will flush them into hardware after it
> > > > receives a mailbox/IPC notification from APSS.
> > > >
> > > > So yes, these registers are fake/virtual in memory, and RPM will
> > > > actually flush the values into the MPM hardware block.
> > >
> > > Then why are you using MMIO accessors all over the place if this is
> > > just RAM? Who *owns* this memory? Is it normal DRAM? Or some flops
> > > exposed by a device? Why isn't the state simply communicated over the
> > > mailbox instead?
> >
> > It's a piece of internal memory (SRAM) which can be access by AP and
> > RPM. The communication mechanism is defined by SoC/RPM design, and we
> > can do nothing but following the procedure.
>
> Then the procedure needs to be documented:
Maulik, I'm trying my best to answer Marc's questions based on my
limited knowledges about the hardware. Please clarify if there is
anything incorrect.
> - Who owns the memory at any given time?
The memory is owned by APSS when system is awake, and owned by RPM when
APSS gets power collapsed. RPM is on the always-on domain and will be
managing resources and monitoring wake-up interrupts during sleep, with
the help of MPM. MPM is not a core/master but a hardware
controller/block.
> - What are the events that trigger a change of ownership?
When APSS is about to get power collapsed, it sends a mailbox
notification to RPM, and RPM will take the ownership. And when APSS is
woken up by a MPM pin/interrupt, APSS takes back the ownership.
> - What are the messages exchanged between these entities?
The messages exchanged are documented as "vMPM register layout" in the
beginning of the driver. Basically, they are values of MPM registers
related to wake-up interrupt configurations, which will be directly
dumped into physical MPM registers by RPM, when RPM gets the ownership.
On wake-up, RPM will copy STATUS registers into vMPM memory and return
the ownership back to APSS.
> - What is the synchronisation mechanism between the various processing
> entities (MPM. RPM, APSS...)?
A hardware IPC/mailbox channel is being used by APSS to tell RPM to take
over the ownership. On wake-up, the wake-up event itself would be the
synchronisation.
> - Is the per-interrupt tracking a HW requirement or a SW
> implementation choice? Could this instead be a one-shot operation
> iterating over the whole state?
I do not quite sure what "per-interrupt tracking" means. The HW
requirement is just that, when RPM takes the ownership of vMPM memory
region, the memory contains the MPM register values, that RPM can
directly dump into MPM registers.
> All this needs to be explained so that it is maintainable, because I'm
> getting tired of drivers that mimic the QC downstream code without
> justification nor documentation to support the implementation.
I really appreciate all your review comments and questions which are
driving for good and maintainable code!
Shawn
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-12-08 10:05 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-02 12:21 [PATCH v3 0/3] Add Qualcomm MPM irqchip driver support Shawn Guo
2021-12-02 12:21 ` [PATCH v3 1/3] irqchip: Pass platform_device pointer to init_cb Shawn Guo
2021-12-02 17:52 ` Florian Fainelli
2021-12-02 19:10 ` Marc Zyngier
2021-12-02 21:44 ` Florian Fainelli
2021-12-06 6:40 ` Shawn Guo
2021-12-06 8:29 ` Marc Zyngier
2021-12-06 6:35 ` Shawn Guo
2021-12-02 12:21 ` [PATCH v3 2/3] dt-bindings: interrupt-controller: Add Qualcomm MPM support Shawn Guo
2021-12-02 12:21 ` [PATCH v3 3/3] irqchip: Add Qualcomm MPM controller driver Shawn Guo
2021-12-03 0:00 ` kernel test robot
2021-12-06 9:46 ` Marc Zyngier
2021-12-06 13:15 ` Shawn Guo
2021-12-06 13:48 ` Marc Zyngier
2021-12-07 9:48 ` Shawn Guo
2021-12-07 10:16 ` Marc Zyngier
2021-12-08 10:04 ` Shawn Guo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).