[PATCH 0/9] sunxi: Support IRQ wakeup from deep sleep

[PATCH 0/9] sunxi: Support IRQ wakeup from deep sleep

[Samuel Holland]

Allwinner sun8i/sun50i SoCs (A31 and newer) have two interrupt
controllers: GIC and R_INTC. GIC does not support wakeup, and is
inaccessible from the ARISC (power management coprocessor). R_INTC
controls the NMI pin, and provides 16-32 IRQs to the ARISC. The first 16
of these correspond 1:1 to a block of GIC IRQs starting with the NMI.

This series replaces the existing chained irqchip driver used only to
control the NMI, with a stacked irqchip driver that also provides wakeup
capability for those 16 IRQs. The idea is that we preconfigure the
ARISC's IRQ controller, and then it knows to wake up as soon as it
receives an IRQ.

I went back and forth about updating the existing driver versus writing
a new one. The NMI-only control on sun7i (A20) and sun9i (A80) is
missing MASK_REG, so it would need a different irq_chip definition. And
the only benefit it would get is the chained->stacked conversion, since
there's no separate coprocessor to see the IRQs during suspend. So
ultimately I went with a new driver. It may be useful to separately do
the chained->stacked conversion on the sunxi-nmi driver as well.

Patch 1 adds the new driver.
Patch 2 adds wakeup capability.
Remaining patches update the DT+bindings to use R_INTC where beneficial

With appropriate firmware, this series allows waking from RTC and NMI
(power button, plugging in USB, etc.). Wake from Port L GPIO interrupts
(gpio-keys, wifi, etc.) requires some patches to the pinctrl driver.

Samuel Holland (9):
  irqchip/sun6i-r: Switch to a stacked irqchip driver
  irqchip/sun6i-r: Add wakeup support
  dt-bindings: irq: Add a compatible for the H3 R_INTC
  ARM: dts: sunxi: h3/h5: Add r_intc node
  ARM: dts: sunxi: h3/h5: Move wakeup-capable IRQs to r_intc
  ARM: dts: sunxi: a83t: Move wakeup-capable IRQs to r_intc
  arm64: dts: allwinner: a64: Move wakeup-capable IRQs to r_intc
  arm64: dts: allwinner: h6: Fix indentation of IR node
  arm64: dts: allwinner: Move wakeup-capable IRQs to r_intc

 .../allwinner,sun7i-a20-sc-nmi.yaml           |   3 +
 arch/arm/boot/dts/sun8i-a83t.dtsi             |   9 +-
 arch/arm/boot/dts/sunxi-h3-h5.dtsi            |  20 +-
 arch/arm/mach-sunxi/Kconfig                   |   1 +
 arch/arm64/Kconfig.platforms                  |   1 +
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi |  11 +-
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi  |  33 ++-
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-sun6i-r.c                 | 273 ++++++++++++++++++
 drivers/irqchip/irq-sunxi-nmi.c               |  26 +-
 10 files changed, 329 insertions(+), 49 deletions(-)
 create mode 100644 drivers/irqchip/irq-sun6i-r.c

-- 
2.23.0

[PATCH 1/9] irqchip/sun6i-r: Switch to a stacked irqchip driver

[Samuel Holland]

The R_INTC in the A31 and newer sun8i/sun50i SoCs is more similar to the
original sun4i interrupt controller than the sun7i/sun9i NMI controller.
It is used for two distinct purposes:
 1) To control the trigger and mask for the NMI input pin
 2) To provide the interrupt input for the ARISC coprocessor

As this interrupt controller is not documented, information about it
comes from reverse-engineering the BSP-provided ARISC firmware.

Like the original sun4i interrupt controller, it has:
 - A VECTOR_REG at 0x00 (configurable via the BASE_ADDR_REG at 0x04)
 - A NMI_CTRL_REG, PENDING_REG, and ENABLE_REG as used by both the
   sun4i and sunxi-nmi drivers
 - A MASK_REG at 0x50

Differences from the sun4i interrupt controller appear to be:
 - It is only known to have one register of each kind (max 32 inputs)
 - There is no FIQ-related logic
 - There is no interrupt priority logic

In order to fulfill its two purposes, this hardware block combines two
types of IRQs. First, the NMI pin is routed to the "IRQ 0" input on this
chip, with a trigger type controlled by the NMI_CTRL_REG. The (masked)
"IRQ 0 pending" output from this chip is then routed to a non-maskable
SPI IRQ input on the GIC, as IRQ_TYPE_LEVEL_HIGH. In other words, bit 0
of ENABLE_REG and MASK_REG *do* affect the IRQs seen at the GIC.

The NMI is then followed by a contiguous block of (at least) 15 IRQ
inputs that are connected *in parallel* to both R_INTC and the GIC. Or
in other words, the other bits of ENABLE_REG and MASK_REG *do not*
affect the IRQs seen at the GIC.

Finally, the global "IRQ pending" output from R_INTC is connected to the
"external interrupt" input of the ARISC CPU (an OR1200).

Because of the 1:1 correspondence between R_INTC and GIC inputs, this is
a perfect scenario for using a stacked irqchip driver. We want to hook
into enabling/disabling/masking IRQs to add more features to the GIC
(specifically to allow masking the NMI and setting its trigger type),
but we don't need to actually *handle* the IRQ.

And since R_INTC is in the always-on power domain, and its output is
connected directly in to the power management coprocessor, a stacked
irqchip driver provides a simple way to add wakeup support to this set
of IRQs. That is a future patch; for now, just the NMI is moved over.

This driver keeps the same DT binding as the existing driver. The
"interrupt" property of the R_INTC node is used to determine 1) the
offset between GIC and R_INTC hwirq numbers and 2) the type of trigger
between the R_INTC "IRQ 0 pending" output and the GIC NMI input.

This commit mostly reverts commit 173bda53b340 ("irqchip/sunxi-nmi:
Support sun6i-a31-r-intc compatible").

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 arch/arm/mach-sunxi/Kconfig     |   1 +
 arch/arm64/Kconfig.platforms    |   1 +
 drivers/irqchip/Makefile        |   1 +
 drivers/irqchip/irq-sun6i-r.c   | 220 ++++++++++++++++++++++++++++++++
 drivers/irqchip/irq-sunxi-nmi.c |  26 +---
 5 files changed, 226 insertions(+), 23 deletions(-)
 create mode 100644 drivers/irqchip/irq-sun6i-r.c

diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
index eeadb1a4dcfe..ef1cc25902b5 100644
--- a/arch/arm/mach-sunxi/Kconfig
+++ b/arch/arm/mach-sunxi/Kconfig
@@ -6,6 +6,7 @@ menuconfig ARCH_SUNXI
 	select CLKSRC_MMIO
 	select GENERIC_IRQ_CHIP
 	select GPIOLIB
+	select IRQ_DOMAIN_HIERARCHY
 	select PINCTRL
 	select PM_OPP
 	select SUN4I_TIMER
diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 16d761475a86..d282d0a1d17d 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -17,6 +17,7 @@ config ARCH_SUNXI
 	bool "Allwinner sunxi 64-bit SoC Family"
 	select ARCH_HAS_RESET_CONTROLLER
 	select GENERIC_IRQ_CHIP
+	select IRQ_DOMAIN_HIERARCHY
 	select PINCTRL
 	select RESET_CONTROLLER
 	help
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index cc7c43932f16..41996d98c30a 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_OR1K_PIC)			+= irq-or1k-pic.o
 obj-$(CONFIG_ORION_IRQCHIP)		+= irq-orion.o
 obj-$(CONFIG_OMAP_IRQCHIP)		+= irq-omap-intc.o
 obj-$(CONFIG_ARCH_SUNXI)		+= irq-sun4i.o
+obj-$(CONFIG_ARCH_SUNXI)		+= irq-sun6i-r.o
 obj-$(CONFIG_ARCH_SUNXI)		+= irq-sunxi-nmi.o
 obj-$(CONFIG_ARCH_SPEAR3XX)		+= spear-shirq.o
 obj-$(CONFIG_ARM_GIC)			+= irq-gic.o irq-gic-common.o
diff --git a/drivers/irqchip/irq-sun6i-r.c b/drivers/irqchip/irq-sun6i-r.c
new file mode 100644
index 000000000000..37b6e9c60bf8
--- /dev/null
+++ b/drivers/irqchip/irq-sun6i-r.c
@@ -0,0 +1,220 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Allwinner A31 and newer SoCs R_INTC driver
+//
+
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
+#define NMI_HWIRQ		0
+
+#define SUN6I_R_INTC_NR_IRQS	16
+
+#define SUN6I_R_INTC_CTRL	0x0c
+#define SUN6I_R_INTC_PENDING	0x10
+#define SUN6I_R_INTC_ENABLE	0x40
+#define SUN6I_R_INTC_MASK	0x50
+
+enum {
+	SUNXI_SRC_TYPE_LEVEL_LOW = 0,
+	SUNXI_SRC_TYPE_EDGE_FALLING,
+	SUNXI_SRC_TYPE_LEVEL_HIGH,
+	SUNXI_SRC_TYPE_EDGE_RISING,
+};
+
+static void __iomem *base;
+static irq_hw_number_t parent_offset;
+static u32 parent_type;
+
+static void sun6i_r_intc_irq_enable(struct irq_data *data)
+{
+	if (data->hwirq == NMI_HWIRQ)
+		writel(BIT(NMI_HWIRQ), base + SUN6I_R_INTC_ENABLE);
+
+	irq_chip_enable_parent(data);
+}
+
+static void sun6i_r_intc_irq_disable(struct irq_data *data)
+{
+	if (data->hwirq == NMI_HWIRQ)
+		writel(0, base + SUN6I_R_INTC_ENABLE);
+
+	irq_chip_disable_parent(data);
+}
+
+static void sun6i_r_intc_irq_mask(struct irq_data *data)
+{
+	if (data->hwirq == NMI_HWIRQ)
+		writel(BIT(NMI_HWIRQ), base + SUN6I_R_INTC_MASK);
+
+	irq_chip_mask_parent(data);
+}
+
+static void sun6i_r_intc_irq_unmask(struct irq_data *data)
+{
+	if (data->hwirq == NMI_HWIRQ)
+		writel(0, base + SUN6I_R_INTC_MASK);
+
+	irq_chip_unmask_parent(data);
+}
+
+static void sun6i_r_intc_irq_eoi(struct irq_data *data)
+{
+	if (data->hwirq == NMI_HWIRQ)
+		writel(BIT(NMI_HWIRQ), base + SUN6I_R_INTC_PENDING);
+
+	irq_chip_eoi_parent(data);
+}
+
+static int sun6i_r_intc_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	if (data->hwirq == NMI_HWIRQ) {
+		u32 src_type;
+
+		switch (type) {
+		case IRQ_TYPE_EDGE_FALLING:
+			src_type = SUNXI_SRC_TYPE_EDGE_FALLING;
+			break;
+		case IRQ_TYPE_EDGE_RISING:
+			src_type = SUNXI_SRC_TYPE_EDGE_RISING;
+			break;
+		case IRQ_TYPE_LEVEL_HIGH:
+			src_type = SUNXI_SRC_TYPE_LEVEL_HIGH;
+			break;
+		case IRQ_TYPE_NONE:
+		case IRQ_TYPE_LEVEL_LOW:
+			src_type = SUNXI_SRC_TYPE_LEVEL_LOW;
+			break;
+		default:
+			pr_err("%pOF: invalid trigger type %d for IRQ %d\n",
+			       irq_domain_get_of_node(data->domain), type,
+			       data->irq);
+			return -EBADR;
+		}
+		writel(src_type, base + SUN6I_R_INTC_CTRL);
+
+		irqd_set_trigger_type(data, type);
+
+		/* Send the R_INTC -> GIC trigger type to the GIC driver. */
+		type = parent_type;
+	}
+
+	return irq_chip_set_type_parent(data, type);
+}
+
+static struct irq_chip sun6i_r_intc_chip = {
+	.name			= "sun6i-r-intc",
+	.irq_enable		= sun6i_r_intc_irq_enable,
+	.irq_disable		= sun6i_r_intc_irq_disable,
+	.irq_mask		= sun6i_r_intc_irq_mask,
+	.irq_unmask		= sun6i_r_intc_irq_unmask,
+	.irq_eoi		= sun6i_r_intc_irq_eoi,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+	.irq_retrigger		= irq_chip_retrigger_hierarchy,
+	.irq_set_type		= sun6i_r_intc_irq_set_type,
+	.irq_set_vcpu_affinity	= irq_chip_set_vcpu_affinity_parent,
+};
+
+static int sun6i_r_intc_domain_translate(struct irq_domain *domain,
+					 struct irq_fwspec *fwspec,
+					 unsigned long *hwirq,
+					 unsigned int *type)
+{
+	if (!is_of_node(fwspec->fwnode) || fwspec->param_count != 2)
+		return -EINVAL;
+
+	*hwirq = fwspec->param[0];
+	*type  = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+
+	return 0;
+}
+
+static int sun6i_r_intc_domain_alloc(struct irq_domain *domain,
+				     unsigned int virq,
+				     unsigned int nr_irqs, void *arg)
+{
+	struct irq_fwspec *fwspec = arg;
+	struct irq_fwspec gic_fwspec;
+	irq_hw_number_t hwirq;
+	unsigned int type;
+	int i, ret;
+
+	ret = sun6i_r_intc_domain_translate(domain, fwspec, &hwirq, &type);
+	if (ret)
+		return ret;
+	if (hwirq + nr_irqs > SUN6I_R_INTC_NR_IRQS)
+		return -EINVAL;
+
+	/* Construct a GIC-compatible fwspec from this fwspec. */
+	gic_fwspec = (struct irq_fwspec) {
+		.fwnode      = domain->parent->fwnode,
+		.param_count = 3,
+		.param       = { GIC_SPI, parent_offset + hwirq, type },
+	};
+
+	for (i = 0; i < nr_irqs; ++i)
+		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
+					      &sun6i_r_intc_chip, NULL);
+
+	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &gic_fwspec);
+}
+
+static const struct irq_domain_ops sun6i_r_intc_domain_ops = {
+	.translate	= sun6i_r_intc_domain_translate,
+	.alloc		= sun6i_r_intc_domain_alloc,
+	.free		= irq_domain_free_irqs_common,
+};
+
+static int __init sun6i_r_intc_init(struct device_node *node,
+				    struct device_node *parent)
+{
+	struct irq_domain *domain, *parent_domain;
+	struct of_phandle_args parent_irq;
+	int ret;
+
+	/* Extract the R_INTC -> GIC mapping from the OF node. */
+	ret = of_irq_parse_one(node, 0, &parent_irq);
+	if (ret)
+		return ret;
+	if (parent_irq.args_count != 3 || parent_irq.args[0] != GIC_SPI)
+		return -EINVAL;
+	parent_offset = parent_irq.args[1];
+	parent_type   = parent_irq.args[2];
+
+	parent_domain = irq_find_host(parent);
+	if (!parent_domain) {
+		pr_err("%pOF: Failed to obtain parent domain\n", node);
+		return -ENXIO;
+	}
+
+	base = of_io_request_and_map(node, 0, NULL);
+	if (IS_ERR(base)) {
+		pr_err("%pOF: Failed to map MMIO region\n", node);
+		return PTR_ERR(base);
+	}
+
+	domain = irq_domain_add_hierarchy(parent_domain, 0,
+					  SUN6I_R_INTC_NR_IRQS, node,
+					  &sun6i_r_intc_domain_ops, NULL);
+	if (!domain) {
+		pr_err("%pOF: Failed to allocate domain\n", node);
+		iounmap(base);
+		return -ENOMEM;
+	}
+
+	/* Disable and unmask all interrupts. */
+	writel(0, base + SUN6I_R_INTC_ENABLE);
+	writel(0, base + SUN6I_R_INTC_MASK);
+
+	/* Clear any pending interrupts. */
+	writel(~0, base + SUN6I_R_INTC_PENDING);
+
+	return 0;
+}
+IRQCHIP_DECLARE(sun6i_r_intc, "allwinner,sun6i-a31-r-intc", sun6i_r_intc_init);
diff --git a/drivers/irqchip/irq-sunxi-nmi.c b/drivers/irqchip/irq-sunxi-nmi.c
index a412b5d5d0fa..9f2bd0c5d289 100644
--- a/drivers/irqchip/irq-sunxi-nmi.c
+++ b/drivers/irqchip/irq-sunxi-nmi.c
@@ -27,18 +27,12 @@
 
 #define SUNXI_NMI_IRQ_BIT	BIT(0)
 
-#define SUN6I_R_INTC_CTRL	0x0c
-#define SUN6I_R_INTC_PENDING	0x10
-#define SUN6I_R_INTC_ENABLE	0x40
-
 /*
  * For deprecated sun6i-a31-sc-nmi compatible.
- * Registers are offset by 0x0c.
  */
-#define SUN6I_R_INTC_NMI_OFFSET	0x0c
-#define SUN6I_NMI_CTRL		(SUN6I_R_INTC_CTRL - SUN6I_R_INTC_NMI_OFFSET)
-#define SUN6I_NMI_PENDING	(SUN6I_R_INTC_PENDING - SUN6I_R_INTC_NMI_OFFSET)
-#define SUN6I_NMI_ENABLE	(SUN6I_R_INTC_ENABLE - SUN6I_R_INTC_NMI_OFFSET)
+#define SUN6I_NMI_CTRL		0x00
+#define SUN6I_NMI_PENDING	0x04
+#define SUN6I_NMI_ENABLE	0x34
 
 #define SUN7I_NMI_CTRL		0x00
 #define SUN7I_NMI_PENDING	0x04
@@ -61,12 +55,6 @@ struct sunxi_sc_nmi_reg_offs {
 	u32 enable;
 };
 
-static const struct sunxi_sc_nmi_reg_offs sun6i_r_intc_reg_offs __initconst = {
-	.ctrl	= SUN6I_R_INTC_CTRL,
-	.pend	= SUN6I_R_INTC_PENDING,
-	.enable	= SUN6I_R_INTC_ENABLE,
-};
-
 static const struct sunxi_sc_nmi_reg_offs sun6i_reg_offs __initconst = {
 	.ctrl	= SUN6I_NMI_CTRL,
 	.pend	= SUN6I_NMI_PENDING,
@@ -232,14 +220,6 @@ static int __init sunxi_sc_nmi_irq_init(struct device_node *node,
 	return ret;
 }
 
-static int __init sun6i_r_intc_irq_init(struct device_node *node,
-					struct device_node *parent)
-{
-	return sunxi_sc_nmi_irq_init(node, &sun6i_r_intc_reg_offs);
-}
-IRQCHIP_DECLARE(sun6i_r_intc, "allwinner,sun6i-a31-r-intc",
-		sun6i_r_intc_irq_init);
-
 static int __init sun6i_sc_nmi_irq_init(struct device_node *node,
 					struct device_node *parent)
 {
-- 
2.23.0

[PATCH 2/9] irqchip/sun6i-r: Add wakeup support

[Samuel Holland]

We maintain a mask of wake-enabled IRQs, and enable them in hardware
during the syscore phase of suspend (once IRQs are globally turned off).
We restore the original mask (either nothing or NMI only) during resume.

This serves two purposes. First, it lets power management firmware
running on the ARISC coprocessor know which wakeup sources Linux wants
to have enabled. That way, it can avoid turning them off when it shuts
down the remainder of the clock tree. Second, it preconfigures the
ARISC coprocessor's interrupt controller, so the firmware's wakeup logic
is as simple as waiting for an interrupt to arrive.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/irqchip/irq-sun6i-r.c | 53 +++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/irqchip/irq-sun6i-r.c b/drivers/irqchip/irq-sun6i-r.c
index 37b6e9c60bf8..f4a4e335061b 100644
--- a/drivers/irqchip/irq-sun6i-r.c
+++ b/drivers/irqchip/irq-sun6i-r.c
@@ -3,12 +3,14 @@
 // Allwinner A31 and newer SoCs R_INTC driver
 //
 
+#include <linux/atomic.h>
 #include <linux/irq.h>
 #include <linux/irqchip.h>
 #include <linux/irqdomain.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
+#include <linux/syscore_ops.h>
 
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 
@@ -31,6 +33,9 @@ enum {
 static void __iomem *base;
 static irq_hw_number_t parent_offset;
 static u32 parent_type;
+#ifdef CONFIG_PM_SLEEP
+static atomic_t wake_mask;
+#endif
 
 static void sun6i_r_intc_irq_enable(struct irq_data *data)
 {
@@ -108,6 +113,21 @@ static int sun6i_r_intc_irq_set_type(struct irq_data *data, unsigned int type)
 	return irq_chip_set_type_parent(data, type);
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int sun6i_r_intc_irq_set_wake(struct irq_data *data, unsigned int on)
+{
+	if (on)
+		atomic_or(BIT(data->hwirq), &wake_mask);
+	else
+		atomic_andnot(BIT(data->hwirq), &wake_mask);
+
+	/* GIC cannot wake, so there is no need to call the parent hook. */
+	return 0;
+}
+#else
+#define sun6i_r_intc_irq_set_wake NULL
+#endif
+
 static struct irq_chip sun6i_r_intc_chip = {
 	.name			= "sun6i-r-intc",
 	.irq_enable		= sun6i_r_intc_irq_enable,
@@ -118,6 +138,7 @@ static struct irq_chip sun6i_r_intc_chip = {
 	.irq_set_affinity	= irq_chip_set_affinity_parent,
 	.irq_retrigger		= irq_chip_retrigger_hierarchy,
 	.irq_set_type		= sun6i_r_intc_irq_set_type,
+	.irq_set_wake		= sun6i_r_intc_irq_set_wake,
 	.irq_set_vcpu_affinity	= irq_chip_set_vcpu_affinity_parent,
 };
 
@@ -171,6 +192,36 @@ static const struct irq_domain_ops sun6i_r_intc_domain_ops = {
 	.free		= irq_domain_free_irqs_common,
 };
 
+#ifdef CONFIG_PM_SLEEP
+static int sun6i_r_intc_suspend(void)
+{
+	/* All wake IRQs are enabled during suspend. */
+	writel(atomic_read(&wake_mask), base + SUN6I_R_INTC_ENABLE);
+
+	return 0;
+}
+
+static void sun6i_r_intc_resume(void)
+{
+	u32 mask = atomic_read(&wake_mask) & BIT(NMI_HWIRQ);
+
+	/* Only the NMI is relevant during normal operation. */
+	writel(mask, base + SUN6I_R_INTC_ENABLE);
+}
+
+static struct syscore_ops sun6i_r_intc_syscore_ops = {
+	.suspend	= sun6i_r_intc_suspend,
+	.resume		= sun6i_r_intc_resume,
+};
+
+static void sun6i_r_intc_syscore_init(void)
+{
+	register_syscore_ops(&sun6i_r_intc_syscore_ops);
+}
+#else
+static inline void sun6i_r_intc_syscore_init(void) {}
+#endif
+
 static int __init sun6i_r_intc_init(struct device_node *node,
 				    struct device_node *parent)
 {
@@ -215,6 +266,8 @@ static int __init sun6i_r_intc_init(struct device_node *node,
 	/* Clear any pending interrupts. */
 	writel(~0, base + SUN6I_R_INTC_PENDING);
 
+	sun6i_r_intc_syscore_init();
+
 	return 0;
 }
 IRQCHIP_DECLARE(sun6i_r_intc, "allwinner,sun6i-a31-r-intc", sun6i_r_intc_init);
-- 
2.23.0

[PATCH 3/9] dt-bindings: irq: Add a compatible for the H3 R_INTC

[Samuel Holland]

The Allwinner H3 SoC contains an R_INTC that is, as far as we know,
compatible with the R_INTC present in other sun8i/sun50i SoCs starting
with the A31. Since the R_INTC hardware is undocumented, introduce a new
compatible for the R_INTC variant in this SoC, in case there turns out
to be some difference.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 .../interrupt-controller/allwinner,sun7i-a20-sc-nmi.yaml       | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/allwinner,sun7i-a20-sc-nmi.yaml b/Documentation/devicetree/bindings/interrupt-controller/allwinner,sun7i-a20-sc-nmi.yaml
index 0eccf5551786..fffffcd0eea3 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/allwinner,sun7i-a20-sc-nmi.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/allwinner,sun7i-a20-sc-nmi.yaml
@@ -29,6 +29,9 @@ properties:
       - items:
         - const: allwinner,sun8i-a83t-r-intc
         - const: allwinner,sun6i-a31-r-intc
+      - items:
+        - const: allwinner,sun8i-h3-r-intc
+        - const: allwinner,sun6i-a31-r-intc
       - const: allwinner,sun9i-a80-sc-nmi
       - items:
         - const: allwinner,sun50i-a64-r-intc
-- 
2.23.0

[PATCH 4/9] ARM: dts: sunxi: h3/h5: Add r_intc node

[Samuel Holland]

The H3 and H5 SoCs have an additional interrupt controller in the RTC
power domain that can be used to enable wakeup for certain IRQs.

Add a node for it.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 arch/arm/boot/dts/sunxi-h3-h5.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
index 107eeafad20a..62660108550a 100644
--- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
+++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
@@ -814,6 +814,15 @@
 			#clock-cells = <1>;
 		};
 
+		r_intc: interrupt-controller@1f00c00 {
+			compatible = "allwinner,sun8i-h3-r-intc",
+				     "allwinner,sun6i-a31-r-intc";
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			reg = <0x01f00c00 0x400>;
+			interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
 		r_ccu: clock@1f01400 {
 			compatible = "allwinner,sun8i-h3-r-ccu";
 			reg = <0x01f01400 0x100>;
-- 
2.23.0

[PATCH 5/9] ARM: dts: sunxi: h3/h5: Move wakeup-capable IRQs to r_intc

[Samuel Holland]

All IRQs that can be used to wake up the system must be routed through
r_intc, so they are visible to firmware while the system is suspended.

r_intc IRQ numbers are offset by 32 from the GIC IRQ numbers.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 arch/arm/boot/dts/sunxi-h3-h5.dtsi | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
index 62660108550a..7aca128cfe09 100644
--- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
+++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
@@ -807,8 +807,9 @@
 		rtc: rtc@1f00000 {
 			/* compatible is in per SoC .dtsi file */
 			reg = <0x01f00000 0x400>;
-			interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-parent = <&r_intc>;
+			interrupts = <8 IRQ_TYPE_LEVEL_HIGH>,
+				     <9 IRQ_TYPE_LEVEL_HIGH>;
 			clock-output-names = "osc32k", "osc32k-out", "iosc";
 			clocks = <&osc32k>;
 			#clock-cells = <1>;
@@ -842,7 +843,8 @@
 			clocks = <&r_ccu CLK_APB0_IR>, <&r_ccu CLK_IR>;
 			clock-names = "apb", "ir";
 			resets = <&r_ccu RST_APB0_IR>;
-			interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-parent = <&r_intc>;
+			interrupts = <5 IRQ_TYPE_LEVEL_HIGH>;
 			reg = <0x01f02000 0x400>;
 			status = "disabled";
 		};
@@ -863,7 +865,8 @@
 		r_pio: pinctrl@1f02c00 {
 			compatible = "allwinner,sun8i-h3-r-pinctrl";
 			reg = <0x01f02c00 0x400>;
-			interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-parent = <&r_intc>;
+			interrupts = <13 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&r_ccu CLK_APB0_PIO>, <&osc24M>, <&rtc 0>;
 			clock-names = "apb", "hosc", "losc";
 			gpio-controller;
-- 
2.23.0

[PATCH 6/9] ARM: dts: sunxi: a83t: Move wakeup-capable IRQs to r_intc

[Samuel Holland]

All IRQs that can be used to wake up the system must be routed through
r_intc, so they are visible to firmware while the system is suspended.

r_intc IRQ numbers are offset by 32 from the GIC IRQ numbers.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 arch/arm/boot/dts/sun8i-a83t.dtsi | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/sun8i-a83t.dtsi b/arch/arm/boot/dts/sun8i-a83t.dtsi
index 74bb053cf23c..98513f2af21c 100644
--- a/arch/arm/boot/dts/sun8i-a83t.dtsi
+++ b/arch/arm/boot/dts/sun8i-a83t.dtsi
@@ -1103,7 +1103,8 @@
 			clocks = <&r_ccu CLK_APB0_IR>, <&r_ccu CLK_IR>;
 			clock-names = "apb", "ir";
 			resets = <&r_ccu RST_APB0_IR>;
-			interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-parent = <&r_intc>;
+			interrupts = <5 IRQ_TYPE_LEVEL_HIGH>;
 			reg = <0x01f02000 0x400>;
 			pinctrl-names = "default";
 			pinctrl-0 = <&r_cir_pin>;
@@ -1113,14 +1114,16 @@
 		r_lradc: lradc@1f03c00 {
 			compatible = "allwinner,sun8i-a83t-r-lradc";
 			reg = <0x01f03c00 0x100>;
-			interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-parent = <&r_intc>;
+			interrupts = <10 IRQ_TYPE_LEVEL_HIGH>;
 			status = "disabled";
 		};
 
 		r_pio: pinctrl@1f02c00 {
 			compatible = "allwinner,sun8i-a83t-r-pinctrl";
 			reg = <0x01f02c00 0x400>;
-			interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-parent = <&r_intc>;
+			interrupts = <13 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&r_ccu CLK_APB0_PIO>, <&osc24M>,
 				 <&osc16Md512>;
 			clock-names = "apb", "hosc", "losc";
-- 
2.23.0

[PATCH 7/9] arm64: dts: allwinner: a64: Move wakeup-capable IRQs to r_intc

[Samuel Holland]

All IRQs that can be used to wake up the system must be routed through
r_intc, so they are visible to firmware while the system is suspended.

r_intc IRQ numbers are offset by 32 from the GIC IRQ numbers.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 70f4cce6be43..7b2cacc0aecc 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -1044,8 +1044,9 @@
 			compatible = "allwinner,sun50i-a64-rtc",
 				     "allwinner,sun8i-h3-rtc";
 			reg = <0x01f00000 0x400>;
-			interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-parent = <&r_intc>;
+			interrupts = <8 IRQ_TYPE_LEVEL_HIGH>,
+				     <9 IRQ_TYPE_LEVEL_HIGH>;
 			clock-output-names = "osc32k", "osc32k-out", "iosc";
 			clocks = <&osc32k>;
 			#clock-cells = <1>;
@@ -1094,7 +1095,8 @@
 			clocks = <&r_ccu CLK_APB0_IR>, <&r_ccu CLK_IR>;
 			clock-names = "apb", "ir";
 			resets = <&r_ccu RST_APB0_IR>;
-			interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-parent = <&r_intc>;
+			interrupts = <5 IRQ_TYPE_LEVEL_HIGH>;
 			pinctrl-names = "default";
 			pinctrl-0 = <&r_ir_rx_pin>;
 			status = "disabled";
@@ -1114,7 +1116,8 @@
 		r_pio: pinctrl@1f02c00 {
 			compatible = "allwinner,sun50i-a64-r-pinctrl";
 			reg = <0x01f02c00 0x400>;
-			interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-parent = <&r_intc>;
+			interrupts = <13 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&r_ccu CLK_APB0_PIO>, <&osc24M>, <&osc32k>;
 			clock-names = "apb", "hosc", "losc";
 			gpio-controller;
-- 
2.23.0

[PATCH 8/9] arm64: dts: allwinner: h6: Fix indentation of IR node

[Samuel Holland]

This node was indented by two tabs when added instead of one.
Remove the extra tab.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 22 ++++++++++----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
index 0d5ea19336a1..f597f3fe06c1 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -766,17 +766,17 @@
 		};
 
 		r_ir: ir@7040000 {
-				compatible = "allwinner,sun50i-h6-ir",
-					     "allwinner,sun6i-a31-ir";
-				reg = <0x07040000 0x400>;
-				interrupts = <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>;
-				clocks = <&r_ccu CLK_R_APB1_IR>,
-					 <&r_ccu CLK_IR>;
-				clock-names = "apb", "ir";
-				resets = <&r_ccu RST_R_APB1_IR>;
-				pinctrl-names = "default";
-				pinctrl-0 = <&r_ir_rx_pin>;
-				status = "disabled";
+			compatible = "allwinner,sun50i-h6-ir",
+				     "allwinner,sun6i-a31-ir";
+			reg = <0x07040000 0x400>;
+			interrupts = <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&r_ccu CLK_R_APB1_IR>,
+				 <&r_ccu CLK_IR>;
+			clock-names = "apb", "ir";
+			resets = <&r_ccu RST_R_APB1_IR>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&r_ir_rx_pin>;
+			status = "disabled";
 		};
 
 		r_i2c: i2c@7081400 {
-- 
2.23.0

[PATCH 9/9] arm64: dts: allwinner: Move wakeup-capable IRQs to r_intc

[Samuel Holland]

All IRQs that can be used to wake up the system must be routed through
r_intc, so they are visible to firmware while the system is suspended.

r_intc IRQ numbers are offset by 96 from the GIC IRQ numbers.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
index f597f3fe06c1..6285354e83a6 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -708,8 +708,9 @@
 		rtc: rtc@7000000 {
 			compatible = "allwinner,sun50i-h6-rtc";
 			reg = <0x07000000 0x400>;
-			interrupts = <GIC_SPI 101 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-parent = <&r_intc>;
+			interrupts = <5 IRQ_TYPE_LEVEL_HIGH>,
+				     <6 IRQ_TYPE_LEVEL_HIGH>;
 			clock-output-names = "osc32k", "osc32k-out", "iosc";
 			clocks = <&ext_osc32k>;
 			#clock-cells = <1>;
@@ -745,8 +746,9 @@
 		r_pio: pinctrl@7022000 {
 			compatible = "allwinner,sun50i-h6-r-pinctrl";
 			reg = <0x07022000 0x400>;
-			interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 111 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-parent = <&r_intc>;
+			interrupts = < 9 IRQ_TYPE_LEVEL_HIGH>,
+				     <15 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&r_ccu CLK_R_APB1>, <&osc24M>, <&rtc 0>;
 			clock-names = "apb", "hosc", "losc";
 			gpio-controller;
@@ -769,7 +771,8 @@
 			compatible = "allwinner,sun50i-h6-ir",
 				     "allwinner,sun6i-a31-ir";
 			reg = <0x07040000 0x400>;
-			interrupts = <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-parent = <&r_intc>;
+			interrupts = <13 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&r_ccu CLK_R_APB1_IR>,
 				 <&r_ccu CLK_IR>;
 			clock-names = "apb", "ir";
-- 
2.23.0

Re: [PATCH 3/9] dt-bindings: irq: Add a compatible for the H3 R_INTC

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1503 bytes --]

On Sun, Jan 12, 2020 at 10:49:30PM -0600, Samuel Holland wrote:
> The Allwinner H3 SoC contains an R_INTC that is, as far as we know,
> compatible with the R_INTC present in other sun8i/sun50i SoCs starting
> with the A31. Since the R_INTC hardware is undocumented, introduce a new
> compatible for the R_INTC variant in this SoC, in case there turns out
> to be some difference.
>
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  .../interrupt-controller/allwinner,sun7i-a20-sc-nmi.yaml       | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/allwinner,sun7i-a20-sc-nmi.yaml b/Documentation/devicetree/bindings/interrupt-controller/allwinner,sun7i-a20-sc-nmi.yaml
> index 0eccf5551786..fffffcd0eea3 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/allwinner,sun7i-a20-sc-nmi.yaml
> +++ b/Documentation/devicetree/bindings/interrupt-controller/allwinner,sun7i-a20-sc-nmi.yaml
> @@ -29,6 +29,9 @@ properties:
>        - items:
>          - const: allwinner,sun8i-a83t-r-intc
>          - const: allwinner,sun6i-a31-r-intc
> +      - items:
> +        - const: allwinner,sun8i-h3-r-intc
> +        - const: allwinner,sun6i-a31-r-intc

If we are to add more compatibles, I guess we could switch to
something like:

items:
  - enum:
    - allwinner,sun8i-a83t-r-intc
    - allwinner,sun8i-h3-r-intc
  - const: allwinner,sun6i-a31-r-intc

It's going to be easier to maintain in the long run.

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/9] irqchip/sun6i-r: Switch to a stacked irqchip driver

[Marc Zyngier]

Hi Samuel,

On 2020-01-13 05:49, Samuel Holland wrote:
> The R_INTC in the A31 and newer sun8i/sun50i SoCs is more similar to 
> the
> original sun4i interrupt controller than the sun7i/sun9i NMI 
> controller.
> It is used for two distinct purposes:
>  1) To control the trigger and mask for the NMI input pin
>  2) To provide the interrupt input for the ARISC coprocessor
> 
> As this interrupt controller is not documented, information about it
> comes from reverse-engineering the BSP-provided ARISC firmware.
> 
> Like the original sun4i interrupt controller, it has:
>  - A VECTOR_REG at 0x00 (configurable via the BASE_ADDR_REG at 0x04)
>  - A NMI_CTRL_REG, PENDING_REG, and ENABLE_REG as used by both the
>    sun4i and sunxi-nmi drivers
>  - A MASK_REG at 0x50
> 
> Differences from the sun4i interrupt controller appear to be:
>  - It is only known to have one register of each kind (max 32 inputs)
>  - There is no FIQ-related logic
>  - There is no interrupt priority logic
> 
> In order to fulfill its two purposes, this hardware block combines two
> types of IRQs. First, the NMI pin is routed to the "IRQ 0" input on 
> this
> chip, with a trigger type controlled by the NMI_CTRL_REG. The (masked)
> "IRQ 0 pending" output from this chip is then routed to a non-maskable
> SPI IRQ input on the GIC, as IRQ_TYPE_LEVEL_HIGH. In other words, bit 0

I object to the "non-maskable" wording here. It may be non-maskable
at this irqchip level (and yet you seem to have code to that effect),
but the GIC definitely should be able to mask things.

> of ENABLE_REG and MASK_REG *do* affect the IRQs seen at the GIC.
> 
> The NMI is then followed by a contiguous block of (at least) 15 IRQ
> inputs that are connected *in parallel* to both R_INTC and the GIC. Or
> in other words, the other bits of ENABLE_REG and MASK_REG *do not*
> affect the IRQs seen at the GIC.
> 
> Finally, the global "IRQ pending" output from R_INTC is connected to 
> the
> "external interrupt" input of the ARISC CPU (an OR1200).
> 
> Because of the 1:1 correspondence between R_INTC and GIC inputs, this 
> is
> a perfect scenario for using a stacked irqchip driver. We want to hook
> into enabling/disabling/masking IRQs to add more features to the GIC
> (specifically to allow masking the NMI and setting its trigger type),
> but we don't need to actually *handle* the IRQ.
> 
> And since R_INTC is in the always-on power domain, and its output is
> connected directly in to the power management coprocessor, a stacked
> irqchip driver provides a simple way to add wakeup support to this set
> of IRQs. That is a future patch; for now, just the NMI is moved over.
> 
> This driver keeps the same DT binding as the existing driver. The
> "interrupt" property of the R_INTC node is used to determine 1) the
> offset between GIC and R_INTC hwirq numbers and 2) the type of trigger
> between the R_INTC "IRQ 0 pending" output and the GIC NMI input.
> 
> This commit mostly reverts commit 173bda53b340 ("irqchip/sunxi-nmi:
> Support sun6i-a31-r-intc compatible").
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  arch/arm/mach-sunxi/Kconfig     |   1 +
>  arch/arm64/Kconfig.platforms    |   1 +
>  drivers/irqchip/Makefile        |   1 +
>  drivers/irqchip/irq-sun6i-r.c   | 220 ++++++++++++++++++++++++++++++++
>  drivers/irqchip/irq-sunxi-nmi.c |  26 +---
>  5 files changed, 226 insertions(+), 23 deletions(-)
>  create mode 100644 drivers/irqchip/irq-sun6i-r.c
> 
> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> index eeadb1a4dcfe..ef1cc25902b5 100644
> --- a/arch/arm/mach-sunxi/Kconfig
> +++ b/arch/arm/mach-sunxi/Kconfig
> @@ -6,6 +6,7 @@ menuconfig ARCH_SUNXI
>  	select CLKSRC_MMIO
>  	select GENERIC_IRQ_CHIP
>  	select GPIOLIB
> +	select IRQ_DOMAIN_HIERARCHY
>  	select PINCTRL
>  	select PM_OPP
>  	select SUN4I_TIMER
> diff --git a/arch/arm64/Kconfig.platforms 
> b/arch/arm64/Kconfig.platforms
> index 16d761475a86..d282d0a1d17d 100644
> --- a/arch/arm64/Kconfig.platforms
> +++ b/arch/arm64/Kconfig.platforms
> @@ -17,6 +17,7 @@ config ARCH_SUNXI
>  	bool "Allwinner sunxi 64-bit SoC Family"
>  	select ARCH_HAS_RESET_CONTROLLER
>  	select GENERIC_IRQ_CHIP
> +	select IRQ_DOMAIN_HIERARCHY
>  	select PINCTRL
>  	select RESET_CONTROLLER
>  	help
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index cc7c43932f16..41996d98c30a 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_OR1K_PIC)			+= irq-or1k-pic.o
>  obj-$(CONFIG_ORION_IRQCHIP)		+= irq-orion.o
>  obj-$(CONFIG_OMAP_IRQCHIP)		+= irq-omap-intc.o
>  obj-$(CONFIG_ARCH_SUNXI)		+= irq-sun4i.o
> +obj-$(CONFIG_ARCH_SUNXI)		+= irq-sun6i-r.o
>  obj-$(CONFIG_ARCH_SUNXI)		+= irq-sunxi-nmi.o
>  obj-$(CONFIG_ARCH_SPEAR3XX)		+= spear-shirq.o
>  obj-$(CONFIG_ARM_GIC)			+= irq-gic.o irq-gic-common.o
> diff --git a/drivers/irqchip/irq-sun6i-r.c 
> b/drivers/irqchip/irq-sun6i-r.c
> new file mode 100644
> index 000000000000..37b6e9c60bf8
> --- /dev/null
> +++ b/drivers/irqchip/irq-sun6i-r.c
> @@ -0,0 +1,220 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// Allwinner A31 and newer SoCs R_INTC driver
> +//
> +
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +#define NMI_HWIRQ		0
> +
> +#define SUN6I_R_INTC_NR_IRQS	16
> +
> +#define SUN6I_R_INTC_CTRL	0x0c
> +#define SUN6I_R_INTC_PENDING	0x10
> +#define SUN6I_R_INTC_ENABLE	0x40
> +#define SUN6I_R_INTC_MASK	0x50
> +
> +enum {
> +	SUNXI_SRC_TYPE_LEVEL_LOW = 0,
> +	SUNXI_SRC_TYPE_EDGE_FALLING,
> +	SUNXI_SRC_TYPE_LEVEL_HIGH,
> +	SUNXI_SRC_TYPE_EDGE_RISING,
> +};

It is unusual to use an enum for values that get directly programmed
into the HW.

> +
> +static void __iomem *base;
> +static irq_hw_number_t parent_offset;
> +static u32 parent_type;
> +
> +static void sun6i_r_intc_irq_enable(struct irq_data *data)
> +{
> +	if (data->hwirq == NMI_HWIRQ)
> +		writel(BIT(NMI_HWIRQ), base + SUN6I_R_INTC_ENABLE);

Please consider using _relaxed() accessors.

> +
> +	irq_chip_enable_parent(data);
> +}
> +
> +static void sun6i_r_intc_irq_disable(struct irq_data *data)
> +{
> +	if (data->hwirq == NMI_HWIRQ)
> +		writel(0, base + SUN6I_R_INTC_ENABLE);
> +
> +	irq_chip_disable_parent(data);
> +}
> +
> +static void sun6i_r_intc_irq_mask(struct irq_data *data)
> +{
> +	if (data->hwirq == NMI_HWIRQ)
> +		writel(BIT(NMI_HWIRQ), base + SUN6I_R_INTC_MASK);
> +
> +	irq_chip_mask_parent(data);
> +}
> +
> +static void sun6i_r_intc_irq_unmask(struct irq_data *data)
> +{
> +	if (data->hwirq == NMI_HWIRQ)
> +		writel(0, base + SUN6I_R_INTC_MASK);
> +
> +	irq_chip_unmask_parent(data);
> +}
> +
> +static void sun6i_r_intc_irq_eoi(struct irq_data *data)
> +{
> +	if (data->hwirq == NMI_HWIRQ)
> +		writel(BIT(NMI_HWIRQ), base + SUN6I_R_INTC_PENDING);

Are you sure about this? Clearing the pending bit is not quite an EOI.
It won't hurt a level interrupt, but could be pretty deadly with
edge signaling (you'd loose that interrupt). But does this register
actually latch the input until you clear it? Or does it follow the
level of its input?

> +
> +	irq_chip_eoi_parent(data);
> +}
> +
> +static int sun6i_r_intc_irq_set_type(struct irq_data *data, unsigned 
> int type)
> +{
> +	if (data->hwirq == NMI_HWIRQ) {
> +		u32 src_type;
> +
> +		switch (type) {
> +		case IRQ_TYPE_EDGE_FALLING:
> +			src_type = SUNXI_SRC_TYPE_EDGE_FALLING;
> +			break;
> +		case IRQ_TYPE_EDGE_RISING:
> +			src_type = SUNXI_SRC_TYPE_EDGE_RISING;
> +			break;
> +		case IRQ_TYPE_LEVEL_HIGH:
> +			src_type = SUNXI_SRC_TYPE_LEVEL_HIGH;
> +			break;
> +		case IRQ_TYPE_NONE:

What does "IRQ_TYPE_NONE" mean here?

> +		case IRQ_TYPE_LEVEL_LOW:
> +			src_type = SUNXI_SRC_TYPE_LEVEL_LOW;
> +			break;
> +		default:
> +			pr_err("%pOF: invalid trigger type %d for IRQ %d\n",
> +			       irq_domain_get_of_node(data->domain), type,
> +			       data->irq);
> +			return -EBADR;
> +		}
> +		writel(src_type, base + SUN6I_R_INTC_CTRL);
> +
> +		irqd_set_trigger_type(data, type);

It is odd to update this from a driver. Specially that you change it
before finding out if the parent call has succeeded or not.

> +
> +		/* Send the R_INTC -> GIC trigger type to the GIC driver. */
> +		type = parent_type;
> +	}
> +
> +	return irq_chip_set_type_parent(data, type);

Half of the above signaling modes are invalid for the GIC. Does this
widget actually invert the signalling when the input is either
level low or falling edge?

> +}
> +
> +static struct irq_chip sun6i_r_intc_chip = {
> +	.name			= "sun6i-r-intc",
> +	.irq_enable		= sun6i_r_intc_irq_enable,
> +	.irq_disable		= sun6i_r_intc_irq_disable,
> +	.irq_mask		= sun6i_r_intc_irq_mask,
> +	.irq_unmask		= sun6i_r_intc_irq_unmask,

What is the upshot of having both enable/disable and mask/unmask?
Given that the GIC only supports the latter, I'd expect this driver
to leave everything enabled, and only deal with mask/unmask.

> +	.irq_eoi		= sun6i_r_intc_irq_eoi,
> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
> +	.irq_retrigger		= irq_chip_retrigger_hierarchy,
> +	.irq_set_type		= sun6i_r_intc_irq_set_type,
> +	.irq_set_vcpu_affinity	= irq_chip_set_vcpu_affinity_parent,

Under which circumstances do you expect this to be called?

> +};
> +
> +static int sun6i_r_intc_domain_translate(struct irq_domain *domain,
> +					 struct irq_fwspec *fwspec,
> +					 unsigned long *hwirq,
> +					 unsigned int *type)
> +{
> +	if (!is_of_node(fwspec->fwnode) || fwspec->param_count != 2)
> +		return -EINVAL;
> +
> +	*hwirq = fwspec->param[0];
> +	*type  = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
> +
> +	return 0;
> +}
> +
> +static int sun6i_r_intc_domain_alloc(struct irq_domain *domain,
> +				     unsigned int virq,
> +				     unsigned int nr_irqs, void *arg)
> +{
> +	struct irq_fwspec *fwspec = arg;
> +	struct irq_fwspec gic_fwspec;
> +	irq_hw_number_t hwirq;
> +	unsigned int type;
> +	int i, ret;
> +
> +	ret = sun6i_r_intc_domain_translate(domain, fwspec, &hwirq, &type);
> +	if (ret)
> +		return ret;
> +	if (hwirq + nr_irqs > SUN6I_R_INTC_NR_IRQS)
> +		return -EINVAL;
> +
> +	/* Construct a GIC-compatible fwspec from this fwspec. */
> +	gic_fwspec = (struct irq_fwspec) {
> +		.fwnode      = domain->parent->fwnode,
> +		.param_count = 3,
> +		.param       = { GIC_SPI, parent_offset + hwirq, type },

Same problem here. The GIC only supports level-high and rising-edge
for SPIs.

> +	};
> +
> +	for (i = 0; i < nr_irqs; ++i)
> +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> +					      &sun6i_r_intc_chip, NULL);
> +
> +	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, 
> &gic_fwspec);
> +}
> +
> +static const struct irq_domain_ops sun6i_r_intc_domain_ops = {
> +	.translate	= sun6i_r_intc_domain_translate,
> +	.alloc		= sun6i_r_intc_domain_alloc,
> +	.free		= irq_domain_free_irqs_common,
> +};
> +
> +static int __init sun6i_r_intc_init(struct device_node *node,
> +				    struct device_node *parent)
> +{
> +	struct irq_domain *domain, *parent_domain;
> +	struct of_phandle_args parent_irq;
> +	int ret;
> +
> +	/* Extract the R_INTC -> GIC mapping from the OF node. */
> +	ret = of_irq_parse_one(node, 0, &parent_irq);
> +	if (ret)
> +		return ret;
> +	if (parent_irq.args_count != 3 || parent_irq.args[0] != GIC_SPI)
> +		return -EINVAL;
> +	parent_offset = parent_irq.args[1];
> +	parent_type   = parent_irq.args[2];
> +
> +	parent_domain = irq_find_host(parent);
> +	if (!parent_domain) {
> +		pr_err("%pOF: Failed to obtain parent domain\n", node);
> +		return -ENXIO;
> +	}
> +
> +	base = of_io_request_and_map(node, 0, NULL);
> +	if (IS_ERR(base)) {
> +		pr_err("%pOF: Failed to map MMIO region\n", node);
> +		return PTR_ERR(base);
> +	}
> +
> +	domain = irq_domain_add_hierarchy(parent_domain, 0,
> +					  SUN6I_R_INTC_NR_IRQS, node,
> +					  &sun6i_r_intc_domain_ops, NULL);
> +	if (!domain) {
> +		pr_err("%pOF: Failed to allocate domain\n", node);
> +		iounmap(base);
> +		return -ENOMEM;
> +	}
> +
> +	/* Disable and unmask all interrupts. */
> +	writel(0, base + SUN6I_R_INTC_ENABLE);
> +	writel(0, base + SUN6I_R_INTC_MASK);
> +
> +	/* Clear any pending interrupts. */
> +	writel(~0, base + SUN6I_R_INTC_PENDING);
> +
> +	return 0;
> +}
> +IRQCHIP_DECLARE(sun6i_r_intc, "allwinner,sun6i-a31-r-intc", 
> sun6i_r_intc_init);
> diff --git a/drivers/irqchip/irq-sunxi-nmi.c 
> b/drivers/irqchip/irq-sunxi-nmi.c
> index a412b5d5d0fa..9f2bd0c5d289 100644
> --- a/drivers/irqchip/irq-sunxi-nmi.c
> +++ b/drivers/irqchip/irq-sunxi-nmi.c
> @@ -27,18 +27,12 @@
> 
>  #define SUNXI_NMI_IRQ_BIT	BIT(0)
> 
> -#define SUN6I_R_INTC_CTRL	0x0c
> -#define SUN6I_R_INTC_PENDING	0x10
> -#define SUN6I_R_INTC_ENABLE	0x40
> -
>  /*
>   * For deprecated sun6i-a31-sc-nmi compatible.
> - * Registers are offset by 0x0c.
>   */
> -#define SUN6I_R_INTC_NMI_OFFSET	0x0c
> -#define SUN6I_NMI_CTRL		(SUN6I_R_INTC_CTRL - SUN6I_R_INTC_NMI_OFFSET)
> -#define SUN6I_NMI_PENDING	(SUN6I_R_INTC_PENDING - 
> SUN6I_R_INTC_NMI_OFFSET)
> -#define SUN6I_NMI_ENABLE	(SUN6I_R_INTC_ENABLE - 
> SUN6I_R_INTC_NMI_OFFSET)
> +#define SUN6I_NMI_CTRL		0x00
> +#define SUN6I_NMI_PENDING	0x04
> +#define SUN6I_NMI_ENABLE	0x34
> 
>  #define SUN7I_NMI_CTRL		0x00
>  #define SUN7I_NMI_PENDING	0x04
> @@ -61,12 +55,6 @@ struct sunxi_sc_nmi_reg_offs {
>  	u32 enable;
>  };
> 
> -static const struct sunxi_sc_nmi_reg_offs sun6i_r_intc_reg_offs 
> __initconst = {
> -	.ctrl	= SUN6I_R_INTC_CTRL,
> -	.pend	= SUN6I_R_INTC_PENDING,
> -	.enable	= SUN6I_R_INTC_ENABLE,
> -};
> -
>  static const struct sunxi_sc_nmi_reg_offs sun6i_reg_offs __initconst = 
> {
>  	.ctrl	= SUN6I_NMI_CTRL,
>  	.pend	= SUN6I_NMI_PENDING,
> @@ -232,14 +220,6 @@ static int __init sunxi_sc_nmi_irq_init(struct
> device_node *node,
>  	return ret;
>  }
> 
> -static int __init sun6i_r_intc_irq_init(struct device_node *node,
> -					struct device_node *parent)
> -{
> -	return sunxi_sc_nmi_irq_init(node, &sun6i_r_intc_reg_offs);
> -}
> -IRQCHIP_DECLARE(sun6i_r_intc, "allwinner,sun6i-a31-r-intc",
> -		sun6i_r_intc_irq_init);
> -
>  static int __init sun6i_sc_nmi_irq_init(struct device_node *node,
>  					struct device_node *parent)
>  {

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/9] irqchip/sun6i-r: Switch to a stacked irqchip driver

[Samuel Holland]

Hello, and thanks for the feedback!

I know this is quite the delay in responding; I wanted to make sure my
understanding of the hardware was as clear as possible before sending a v2.

After experimentation, I came up with a diagram describing the hardware
architecture, available here:
https://linux-sunxi.org/images/5/5c/R_INTC.png (PNG)
https://sholland.org/files/R_INTC_v2.svg (SVG)

Based on that, your feedback, and similar examples like the great explanation of
the robustness requirements in 6dd859508336 ("gpio: zynq: Fix IRQ handlers"), I
think v2 will work properly for both edge and level interrupts. I tested both
triggers, albeit with the same source of (level) interrupts connected to the NMI
pin.

On 1/20/20 4:52 AM, Marc Zyngier wrote:
> Hi Samuel,
> 
> On 2020-01-13 05:49, Samuel Holland wrote:
>> The R_INTC in the A31 and newer sun8i/sun50i SoCs is more similar to the
>> original sun4i interrupt controller than the sun7i/sun9i NMI controller.
>> It is used for two distinct purposes:
>>  1) To control the trigger and mask for the NMI input pin
>>  2) To provide the interrupt input for the ARISC coprocessor
>>
>> As this interrupt controller is not documented, information about it
>> comes from reverse-engineering the BSP-provided ARISC firmware.
>>
>> Like the original sun4i interrupt controller, it has:
>>  - A VECTOR_REG at 0x00 (configurable via the BASE_ADDR_REG at 0x04)
>>  - A NMI_CTRL_REG, PENDING_REG, and ENABLE_REG as used by both the
>>    sun4i and sunxi-nmi drivers
>>  - A MASK_REG at 0x50
>>
>> Differences from the sun4i interrupt controller appear to be:
>>  - It is only known to have one register of each kind (max 32 inputs)
>>  - There is no FIQ-related logic
>>  - There is no interrupt priority logic
>>
>> In order to fulfill its two purposes, this hardware block combines two
>> types of IRQs. First, the NMI pin is routed to the "IRQ 0" input on this
>> chip, with a trigger type controlled by the NMI_CTRL_REG. The (masked)
>> "IRQ 0 pending" output from this chip is then routed to a non-maskable
>> SPI IRQ input on the GIC, as IRQ_TYPE_LEVEL_HIGH. In other words, bit 0
> 
> I object to the "non-maskable" wording here. It may be non-maskable
> at this irqchip level (and yet you seem to have code to that effect),
> but the GIC definitely should be able to mask things.

You're 100% correct here. I had thought IRQ 0 was non-maskable because the MASK
register didn't affect the IRQ being sent to the GIC. Disabling the IRQ via
GICD_ICENABLER does indeed work.

>> of ENABLE_REG and MASK_REG *do* affect the IRQs seen at the GIC.
>>
>> The NMI is then followed by a contiguous block of (at least) 15 IRQ
>> inputs that are connected *in parallel* to both R_INTC and the GIC. Or
>> in other words, the other bits of ENABLE_REG and MASK_REG *do not*
>> affect the IRQs seen at the GIC.
>>
>> Finally, the global "IRQ pending" output from R_INTC is connected to the
>> "external interrupt" input of the ARISC CPU (an OR1200).
>>
>> Because of the 1:1 correspondence between R_INTC and GIC inputs, this is
>> a perfect scenario for using a stacked irqchip driver. We want to hook
>> into enabling/disabling/masking IRQs to add more features to the GIC
>> (specifically to allow masking the NMI and setting its trigger type),
>> but we don't need to actually *handle* the IRQ.
>>
>> And since R_INTC is in the always-on power domain, and its output is
>> connected directly in to the power management coprocessor, a stacked
>> irqchip driver provides a simple way to add wakeup support to this set
>> of IRQs. That is a future patch; for now, just the NMI is moved over.
>>
>> This driver keeps the same DT binding as the existing driver. The
>> "interrupt" property of the R_INTC node is used to determine 1) the
>> offset between GIC and R_INTC hwirq numbers and 2) the type of trigger
>> between the R_INTC "IRQ 0 pending" output and the GIC NMI input.
>>
>> This commit mostly reverts commit 173bda53b340 ("irqchip/sunxi-nmi:
>> Support sun6i-a31-r-intc compatible").
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>  arch/arm/mach-sunxi/Kconfig     |   1 +
>>  arch/arm64/Kconfig.platforms    |   1 +
>>  drivers/irqchip/Makefile        |   1 +
>>  drivers/irqchip/irq-sun6i-r.c   | 220 ++++++++++++++++++++++++++++++++
>>  drivers/irqchip/irq-sunxi-nmi.c |  26 +---
>>  5 files changed, 226 insertions(+), 23 deletions(-)
>>  create mode 100644 drivers/irqchip/irq-sun6i-r.c
>>
>> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
>> index eeadb1a4dcfe..ef1cc25902b5 100644
>> --- a/arch/arm/mach-sunxi/Kconfig
>> +++ b/arch/arm/mach-sunxi/Kconfig
>> @@ -6,6 +6,7 @@ menuconfig ARCH_SUNXI
>>      select CLKSRC_MMIO
>>      select GENERIC_IRQ_CHIP
>>      select GPIOLIB
>> +    select IRQ_DOMAIN_HIERARCHY
>>      select PINCTRL
>>      select PM_OPP
>>      select SUN4I_TIMER
>> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
>> index 16d761475a86..d282d0a1d17d 100644
>> --- a/arch/arm64/Kconfig.platforms
>> +++ b/arch/arm64/Kconfig.platforms
>> @@ -17,6 +17,7 @@ config ARCH_SUNXI
>>      bool "Allwinner sunxi 64-bit SoC Family"
>>      select ARCH_HAS_RESET_CONTROLLER
>>      select GENERIC_IRQ_CHIP
>> +    select IRQ_DOMAIN_HIERARCHY
>>      select PINCTRL
>>      select RESET_CONTROLLER
>>      help
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index cc7c43932f16..41996d98c30a 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -24,6 +24,7 @@ obj-$(CONFIG_OR1K_PIC)            += irq-or1k-pic.o
>>  obj-$(CONFIG_ORION_IRQCHIP)        += irq-orion.o
>>  obj-$(CONFIG_OMAP_IRQCHIP)        += irq-omap-intc.o
>>  obj-$(CONFIG_ARCH_SUNXI)        += irq-sun4i.o
>> +obj-$(CONFIG_ARCH_SUNXI)        += irq-sun6i-r.o
>>  obj-$(CONFIG_ARCH_SUNXI)        += irq-sunxi-nmi.o
>>  obj-$(CONFIG_ARCH_SPEAR3XX)        += spear-shirq.o
>>  obj-$(CONFIG_ARM_GIC)            += irq-gic.o irq-gic-common.o
>> diff --git a/drivers/irqchip/irq-sun6i-r.c b/drivers/irqchip/irq-sun6i-r.c
>> new file mode 100644
>> index 000000000000..37b6e9c60bf8
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-sun6i-r.c
>> @@ -0,0 +1,220 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +//
>> +// Allwinner A31 and newer SoCs R_INTC driver
>> +//
>> +
>> +#include <linux/irq.h>
>> +#include <linux/irqchip.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +
>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +#define NMI_HWIRQ        0
>> +
>> +#define SUN6I_R_INTC_NR_IRQS    16
>> +
>> +#define SUN6I_R_INTC_CTRL    0x0c
>> +#define SUN6I_R_INTC_PENDING    0x10
>> +#define SUN6I_R_INTC_ENABLE    0x40
>> +#define SUN6I_R_INTC_MASK    0x50
>> +
>> +enum {
>> +    SUNXI_SRC_TYPE_LEVEL_LOW = 0,
>> +    SUNXI_SRC_TYPE_EDGE_FALLING,
>> +    SUNXI_SRC_TYPE_LEVEL_HIGH,
>> +    SUNXI_SRC_TYPE_EDGE_RISING,
>> +};
> 
> It is unusual to use an enum for values that get directly programmed
> into the HW.

These definitons match the existing driver this was split from. I will inline
these for v2.

>> +
>> +static void __iomem *base;
>> +static irq_hw_number_t parent_offset;
>> +static u32 parent_type;
>> +
>> +static void sun6i_r_intc_irq_enable(struct irq_data *data)
>> +{
>> +    if (data->hwirq == NMI_HWIRQ)
>> +        writel(BIT(NMI_HWIRQ), base + SUN6I_R_INTC_ENABLE);
> 
> Please consider using _relaxed() accessors.

I've done this for v2.

>> +
>> +    irq_chip_enable_parent(data);
>> +}
>> +
>> +static void sun6i_r_intc_irq_disable(struct irq_data *data)
>> +{
>> +    if (data->hwirq == NMI_HWIRQ)
>> +        writel(0, base + SUN6I_R_INTC_ENABLE);
>> +
>> +    irq_chip_disable_parent(data);
>> +}
>> +
>> +static void sun6i_r_intc_irq_mask(struct irq_data *data)
>> +{
>> +    if (data->hwirq == NMI_HWIRQ)
>> +        writel(BIT(NMI_HWIRQ), base + SUN6I_R_INTC_MASK);
>> +
>> +    irq_chip_mask_parent(data);
>> +}
>> +
>> +static void sun6i_r_intc_irq_unmask(struct irq_data *data)
>> +{
>> +    if (data->hwirq == NMI_HWIRQ)
>> +        writel(0, base + SUN6I_R_INTC_MASK);
>> +
>> +    irq_chip_unmask_parent(data);
>> +}
>> +
>> +static void sun6i_r_intc_irq_eoi(struct irq_data *data)
>> +{
>> +    if (data->hwirq == NMI_HWIRQ)
>> +        writel(BIT(NMI_HWIRQ), base + SUN6I_R_INTC_PENDING);
> 
> Are you sure about this? Clearing the pending bit is not quite an EOI.
> It won't hurt a level interrupt, but could be pretty deadly with
> edge signaling (you'd loose that interrupt). But does this register
> actually latch the input until you clear it? Or does it follow the
> level of its input?

For bit 0 (the only one Linux cares about), there is a latch. This latch gets
set whenever the IRQ is triggered (once for edge, continuously for level), and
it gets reset by writing 1 to PENDING.

What I've done for v2 is set this bit in .irq_ack for edge, and .irq_eoi for level.

>> +
>> +    irq_chip_eoi_parent(data);
>> +}
>> +
>> +static int sun6i_r_intc_irq_set_type(struct irq_data *data, unsigned int type)
>> +{
>> +    if (data->hwirq == NMI_HWIRQ) {
>> +        u32 src_type;
>> +
>> +        switch (type) {
>> +        case IRQ_TYPE_EDGE_FALLING:
>> +            src_type = SUNXI_SRC_TYPE_EDGE_FALLING;
>> +            break;
>> +        case IRQ_TYPE_EDGE_RISING:
>> +            src_type = SUNXI_SRC_TYPE_EDGE_RISING;
>> +            break;
>> +        case IRQ_TYPE_LEVEL_HIGH:
>> +            src_type = SUNXI_SRC_TYPE_LEVEL_HIGH;
>> +            break;
>> +        case IRQ_TYPE_NONE:
> 
> What does "IRQ_TYPE_NONE" mean here?

It means that IRQ_TYPE_NONE was put in the specifier in the device tree (this is
copied from the other driver). Since this should never happen, I've removed this
case in v2.

>> +        case IRQ_TYPE_LEVEL_LOW:
>> +            src_type = SUNXI_SRC_TYPE_LEVEL_LOW;
>> +            break;
>> +        default:
>> +            pr_err("%pOF: invalid trigger type %d for IRQ %d\n",
>> +                   irq_domain_get_of_node(data->domain), type,
>> +                   data->irq);
>> +            return -EBADR;
>> +        }
>> +        writel(src_type, base + SUN6I_R_INTC_CTRL);
>> +
>> +        irqd_set_trigger_type(data, type);
> 
> It is odd to update this from a driver. Specially that you change it
> before finding out if the parent call has succeeded or not.

Yes, I'm not sure what I was doing there. I've removed this.

>> +
>> +        /* Send the R_INTC -> GIC trigger type to the GIC driver. */
>> +        type = parent_type;
>> +    }
>> +
>> +    return irq_chip_set_type_parent(data, type);
> 
> Half of the above signaling modes are invalid for the GIC. Does this
> widget actually invert the signalling when the input is either
> level low or falling edge?

Yes. The signal sent to the GIC is effectively bit 0 of the PENDING register. So
it's a "1" when the IRQ is triggered, regardless of the physical pin trigger type.

>> +}
>> +
>> +static struct irq_chip sun6i_r_intc_chip = {
>> +    .name            = "sun6i-r-intc",
>> +    .irq_enable        = sun6i_r_intc_irq_enable,
>> +    .irq_disable        = sun6i_r_intc_irq_disable,
>> +    .irq_mask        = sun6i_r_intc_irq_mask,
>> +    .irq_unmask        = sun6i_r_intc_irq_unmask,
> 
> What is the upshot of having both enable/disable and mask/unmask?
> Given that the GIC only supports the latter, I'd expect this driver
> to leave everything enabled, and only deal with mask/unmask.

This makes sense. I've done this for v2.

>> +    .irq_eoi        = sun6i_r_intc_irq_eoi,
>> +    .irq_set_affinity    = irq_chip_set_affinity_parent,
>> +    .irq_retrigger        = irq_chip_retrigger_hierarchy,
>> +    .irq_set_type        = sun6i_r_intc_irq_set_type,
>> +    .irq_set_vcpu_affinity    = irq_chip_set_vcpu_affinity_parent,
> 
> Under which circumstances do you expect this to be called?

Under the same circumstances as the underlying GIC callback. It's my
understanding that irq_chip hooks always go to the outermost irqdomain in the
hierarchy. So the only way the GIC driver functions get called is if I call them
here with irq_chip_*_parent. Presumably, all of the GIC functions are there for
a reason, so I should expose them.

The same appears to be the case for .flags: flags from the inner irqdomains have
to be duplicated here, because the core interrupt handling code only looks at
the flags of desc->irq_data.chip->flags, which is the outermost irqdomain in the
hierarchy.

>> +};
>> +
>> +static int sun6i_r_intc_domain_translate(struct irq_domain *domain,
>> +                     struct irq_fwspec *fwspec,
>> +                     unsigned long *hwirq,
>> +                     unsigned int *type)
>> +{
>> +    if (!is_of_node(fwspec->fwnode) || fwspec->param_count != 2)
>> +        return -EINVAL;
>> +
>> +    *hwirq = fwspec->param[0];
>> +    *type  = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
>> +
>> +    return 0;
>> +}
>> +
>> +static int sun6i_r_intc_domain_alloc(struct irq_domain *domain,
>> +                     unsigned int virq,
>> +                     unsigned int nr_irqs, void *arg)
>> +{
>> +    struct irq_fwspec *fwspec = arg;
>> +    struct irq_fwspec gic_fwspec;
>> +    irq_hw_number_t hwirq;
>> +    unsigned int type;
>> +    int i, ret;
>> +
>> +    ret = sun6i_r_intc_domain_translate(domain, fwspec, &hwirq, &type);
>> +    if (ret)
>> +        return ret;
>> +    if (hwirq + nr_irqs > SUN6I_R_INTC_NR_IRQS)
>> +        return -EINVAL;
>> +
>> +    /* Construct a GIC-compatible fwspec from this fwspec. */
>> +    gic_fwspec = (struct irq_fwspec) {
>> +        .fwnode      = domain->parent->fwnode,
>> +        .param_count = 3,
>> +        .param       = { GIC_SPI, parent_offset + hwirq, type },
> 
> Same problem here. The GIC only supports level-high and rising-edge
> for SPIs.

This actually doesn't cause any errors. The only check that the GIC does on the
"type" parameter during .translate is verifying that it is not IRQ_TYPE_NONE,
and no additional checks are done during .alloc. In fact, nothing seems to
really care about "type" during .alloc. They just want "hwirq", and it's
convenient to use .translate to get that.

>> +    };
>> +
>> +    for (i = 0; i < nr_irqs; ++i)
>> +        irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
>> +                          &sun6i_r_intc_chip, NULL);
>> +
>> +    return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &gic_fwspec);
>> +}
>> +
>> +static const struct irq_domain_ops sun6i_r_intc_domain_ops = {
>> +    .translate    = sun6i_r_intc_domain_translate,
>> +    .alloc        = sun6i_r_intc_domain_alloc,
>> +    .free        = irq_domain_free_irqs_common,
>> +};
>> +
>> +static int __init sun6i_r_intc_init(struct device_node *node,
>> +                    struct device_node *parent)
>> +{
>> +    struct irq_domain *domain, *parent_domain;
>> +    struct of_phandle_args parent_irq;
>> +    int ret;
>> +
>> +    /* Extract the R_INTC -> GIC mapping from the OF node. */
>> +    ret = of_irq_parse_one(node, 0, &parent_irq);
>> +    if (ret)
>> +        return ret;
>> +    if (parent_irq.args_count != 3 || parent_irq.args[0] != GIC_SPI)
>> +        return -EINVAL;
>> +    parent_offset = parent_irq.args[1];
>> +    parent_type   = parent_irq.args[2];
>> +
>> +    parent_domain = irq_find_host(parent);
>> +    if (!parent_domain) {
>> +        pr_err("%pOF: Failed to obtain parent domain\n", node);
>> +        return -ENXIO;
>> +    }
>> +
>> +    base = of_io_request_and_map(node, 0, NULL);
>> +    if (IS_ERR(base)) {
>> +        pr_err("%pOF: Failed to map MMIO region\n", node);
>> +        return PTR_ERR(base);
>> +    }
>> +
>> +    domain = irq_domain_add_hierarchy(parent_domain, 0,
>> +                      SUN6I_R_INTC_NR_IRQS, node,
>> +                      &sun6i_r_intc_domain_ops, NULL);
>> +    if (!domain) {
>> +        pr_err("%pOF: Failed to allocate domain\n", node);
>> +        iounmap(base);
>> +        return -ENOMEM;
>> +    }
>> +
>> +    /* Disable and unmask all interrupts. */
>> +    writel(0, base + SUN6I_R_INTC_ENABLE);
>> +    writel(0, base + SUN6I_R_INTC_MASK);
>> +
>> +    /* Clear any pending interrupts. */
>> +    writel(~0, base + SUN6I_R_INTC_PENDING);
>> +
>> +    return 0;
>> +}
>> +IRQCHIP_DECLARE(sun6i_r_intc, "allwinner,sun6i-a31-r-intc", sun6i_r_intc_init);
>> diff --git a/drivers/irqchip/irq-sunxi-nmi.c b/drivers/irqchip/irq-sunxi-nmi.c
>> index a412b5d5d0fa..9f2bd0c5d289 100644
>> --- a/drivers/irqchip/irq-sunxi-nmi.c
>> +++ b/drivers/irqchip/irq-sunxi-nmi.c
>> @@ -27,18 +27,12 @@
>>
>>  #define SUNXI_NMI_IRQ_BIT    BIT(0)
>>
>> -#define SUN6I_R_INTC_CTRL    0x0c
>> -#define SUN6I_R_INTC_PENDING    0x10
>> -#define SUN6I_R_INTC_ENABLE    0x40
>> -
>>  /*
>>   * For deprecated sun6i-a31-sc-nmi compatible.
>> - * Registers are offset by 0x0c.
>>   */
>> -#define SUN6I_R_INTC_NMI_OFFSET    0x0c
>> -#define SUN6I_NMI_CTRL        (SUN6I_R_INTC_CTRL - SUN6I_R_INTC_NMI_OFFSET)
>> -#define SUN6I_NMI_PENDING    (SUN6I_R_INTC_PENDING - SUN6I_R_INTC_NMI_OFFSET)
>> -#define SUN6I_NMI_ENABLE    (SUN6I_R_INTC_ENABLE - SUN6I_R_INTC_NMI_OFFSET)
>> +#define SUN6I_NMI_CTRL        0x00
>> +#define SUN6I_NMI_PENDING    0x04
>> +#define SUN6I_NMI_ENABLE    0x34
>>
>>  #define SUN7I_NMI_CTRL        0x00
>>  #define SUN7I_NMI_PENDING    0x04
>> @@ -61,12 +55,6 @@ struct sunxi_sc_nmi_reg_offs {
>>      u32 enable;
>>  };
>>
>> -static const struct sunxi_sc_nmi_reg_offs sun6i_r_intc_reg_offs __initconst = {
>> -    .ctrl    = SUN6I_R_INTC_CTRL,
>> -    .pend    = SUN6I_R_INTC_PENDING,
>> -    .enable    = SUN6I_R_INTC_ENABLE,
>> -};
>> -
>>  static const struct sunxi_sc_nmi_reg_offs sun6i_reg_offs __initconst = {
>>      .ctrl    = SUN6I_NMI_CTRL,
>>      .pend    = SUN6I_NMI_PENDING,
>> @@ -232,14 +220,6 @@ static int __init sunxi_sc_nmi_irq_init(struct
>> device_node *node,
>>      return ret;
>>  }
>>
>> -static int __init sun6i_r_intc_irq_init(struct device_node *node,
>> -                    struct device_node *parent)
>> -{
>> -    return sunxi_sc_nmi_irq_init(node, &sun6i_r_intc_reg_offs);
>> -}
>> -IRQCHIP_DECLARE(sun6i_r_intc, "allwinner,sun6i-a31-r-intc",
>> -        sun6i_r_intc_irq_init);
>> -
>>  static int __init sun6i_sc_nmi_irq_init(struct device_node *node,
>>                      struct device_node *parent)
>>  {
> 
> Thanks,
> 
>         M.

Thanks again for your review,
Samuel