irqchip: gic: killing gic_arch_extn and co, slowly

The GIC is now always initialized from DT on tegra, and there is no point in keeping non-DT init code.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm/mach-tegra/irq.c | 8 --------
 1 file changed, 8 deletions(-)

Tegra's LIC (Legacy Interrupt Controller) has been so far only supported as a weird extension of the GIC, which is not exactly pretty.

The stacked irq domain framework fits this pretty well, and allows

the LIC code to be turned into a standalone irqchip. In the process, make the driver DT aware, something that was sorely missing from the mach-tegra implementation.

Signed-off-by Marc Zyngier <[email protected]>

---
 drivers/irqchip/Makefile    |   1 +
 drivers/irqchip/irq-tegra.c | 335 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 336 insertions(+)
 create mode 100644 drivers/irqchip/irq-tegra.c

drivers/irqchip/irq-tegra.c +0 -0

drivers/irqchip/irq-tegra.c +5 -0

+static struct syscore_ops tegra_ictlr_syscore_ops = {
+	.suspend	= tegra_ictlr_suspend,
+	.resume		= tegra_ictlr_resume,
+};
+

+static struct irq_domain_ops tegra_ictlr_domain_ops = {

drivers/irqchip/irq-tegra.c +17 -0

+
+	if (!parent) {
+		pr_err("%s: no parent, giving up\n", node->full_name);
+		return -ENODEV;
+	}
+
+	parent_domain = irq_find_host(parent);
+	if (!parent_domain) {
+		pr_err("%s: unable to obtain parent domain\n", node->full_name);
+		return -ENXIO;
+	}
+
+	tegra_ictlr_info = kzalloc(sizeof(*tegra_ictlr_info), GFP_KERNEL);
+	if (!tegra_ictlr_info)
+		return -ENOMEM;
+
+	for (i = 0; i < TEGRA_MAX_NUM_ICTLRS; i++) {

drivers/irqchip/irq-tegra.c +12 -0

+	if (!domain) {
+		pr_err("%s: failed to allocated domain\n", node->full_name);
+		err = -ENOMEM;
+		goto out_unmap;
+	}
+
+	tegra_ictlr_syscore_init();
+
+	pr_info("%s: %d interrupts forwarded to %s\n",
+		node->full_name, num_ictlrs * 32, parent->full_name);
+
+	return 0;

Tegra's LIC (Legacy Interrupt Controller) has been so far only supported as a weird extension of the GIC, which is not exactly pretty.

The stacked irq domain framework fits this pretty well, and allows the LIC code to be turned into a standalone irqchip. In the process, make the driver DT aware, something that was sorely missing from the mach-tegra implementation.

Signed-off-by Marc Zyngier <[email protected]>

---

Quoted 11 lines · Marc Zyngier

Tegra's LIC (Legacy Interrupt Controller) has been so far only supported as a weird extension of the GIC, which is not exactly pretty.

The stacked irq domain framework fits this pretty well, and allows the LIC code to be turned into a standalone irqchip. In the process, make the driver DT aware, something that was sorely missing from the mach-tegra implementation.

Signed-off-by Marc Zyngier <[email protected]>

---

Saw a few checkpatch warnings as below: all of them seem minors.

(quoted diff) +1 -0

@@ -0,0 +1,35 @@
+WARNING: added, moved or deleted file(s), does MAINTAINERS need 

updating?

(quoted diff) +5 -0

+#36:
+new file mode 100644
+WARNING: line over 80 characters
+#169: FILE: drivers/irqchip/irq-tegra.c:129:
++		tegra_ictlr_info->cpu_ier[i] = readl_relaxed(ictlr + 

ICTLR_CPU_IER);

(quoted diff) +3 -0

+WARNING: line over 80 characters
+#284: FILE: drivers/irqchip/irq-tegra.c:244:
++	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, 

&parent_args);

(quoted diff) +8 -0

+CHECK: Please don't use multiple blank lines
+#287: FILE: drivers/irqchip/irq-tegra.c:247:
++
++
+WARNING: Missing a blank line after declarations
+#296: FILE: drivers/irqchip/irq-tegra.c:256:
++		struct irq_data *d = irq_domain_get_irq_data(domain, virq + i);
++		irq_domain_reset_irq_data(d);

`

If we detect that our DT has a LIC node, don't setup gic_arch_extn, and skip tegra_legacy_irq_syscore_init as well.

This is only a temporary measure until that code is removed for good.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm/mach-tegra/irq.c   | 11 +++++++++++
 arch/arm/mach-tegra/tegra.c |  1 -
 2 files changed, 11 insertions(+), 1 deletion(-)

arch/arm/mach-tegra/irq.c +0 -0

index 7f87a50..b37141d 100644

Describe the legacy interrupt controller in every tegra DTSI files, and make it the parent of most interrupts.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm/boot/dts/tegra114.dtsi | 16 +++++++++++++++-
 arch/arm/boot/dts/tegra124.dtsi | 16 +++++++++++++++-
 arch/arm/boot/dts/tegra20.dtsi  | 15 ++++++++++++++-
 arch/arm/boot/dts/tegra30.dtsi  | 16 +++++++++++++++-
 4 files changed, 59 insertions(+), 4 deletions(-)

arch/arm/boot/dts/tegra114.dtsi +0 -0

index 4296b53..f70bed0 100644

arch/arm/boot/dts/tegra114.dtsi +0 -0

 	};
 };

arch/arm/boot/dts/tegra124.dtsi +0 -0

arch/arm/boot/dts/tegra20.dtsi +2 -0

 	host1x at 50000000 {
 		compatible = "nvidia,tegra20-host1x", "simple-bus";

@@ -142,6 +142,7 @@
 
 	timer at 50004600 {
 		compatible = "arm,cortex-a9-twd-timer";
+		interrupt-parent = <&intc>;
 		reg = <0x50040600 0x20>;
 		interrupts = <GIC_PPI 13
 			(GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_HIGH)>;

@@ -154,6 +155,7 @@
 		       0x50040100 0x0100>;
 		interrupt-controller;
 		#interrupt-cells = <3>;
+		interrupt-parent = <&intc>;
 	};
 
 	cache-controller at 50043000 {

arch/arm/boot/dts/tegra30.dtsi +0 -0

arch/arm/boot/dts/tegra30.dtsi +2 -0

@@ -228,6 +228,7 @@
 	timer at 50004600 {
 		compatible = "arm,cortex-a9-twd-timer";
 		reg = <0x50040600 0x20>;
+		interrupt-parent = <&intc>;
 		interrupts = <GIC_PPI 13
 			(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
 		clocks = <&tegra_car TEGRA30_CLK_TWD>;

@@ -239,6 +240,7 @@
 		       0x50040100 0x0100>;
 		interrupt-controller;
 		#interrupt-cells = <3>;
+		interrupt-parent = <&intc>;
 	};
 
 	cache-controller at 50043000 {

Quoted 3 lines · Marc Zyngier

Describe the legacy interrupt controller in every tegra DTSI files, and make it the parent of most interrupts.

Quoted 8 lines

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm/boot/dts/tegra114.dtsi | 16 +++++++++++++++-
 arch/arm/boot/dts/tegra124.dtsi | 16 +++++++++++++++-
 arch/arm/boot/dts/tegra20.dtsi  | 15 ++++++++++++++-
 arch/arm/boot/dts/tegra30.dtsi  | 16 +++++++++++++++-
 4 files changed, 59 insertions(+), 4 deletions(-)

arch/arm/boot/dts/tegra114.dtsi line 137

Quoted 5 lines

arch/arm/boot/dts/tegra114.dtsi +1 -0

@@ -134,6 +134,19 @@
 		      <0x50046000 0x2000>;
 		interrupts = <GIC_PPI 9
 			(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
+		interrupt-parent = <&gic>;

Is this allowed? It makes the GIC its own parent. I guess we need it to
stop a loop from GIC -> LIC -> GIC, but it doesn't look quite right.

arch/arm/boot/dts/tegra114.dtsi line 782

Quoted 6 lines

arch/arm/boot/dts/tegra114.dtsi +1 -0

 	timer at 60005000 {

@@ -766,5 +779,6 @@
 				(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
 			<GIC_PPI 10
 				(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
+		interrupt-parent = <&gic>;

Why does this get to have a non-default parent?

Quoted 3 lines

arch/arm/boot/dts/tegra20.dtsi +0 -0

 	timer at 60005000 {
 		compatible = "nvidia,tegra20-timer";

reg = <0x60005000 0x60>;

Why doesn't the Tegra timer get to keep the GIC as parent like for Tegra114 and Tegra124? Instead I see that the Cortex-A9 TWD timer gets
to keep the parent instead.

arch/arm/boot/dts/tegra30.dtsi line 269

Quoted 3 lines

arch/arm/boot/dts/tegra30.dtsi +0 -0

 	timer at 60005000 {
 		compatible = "nvidia,tegra30-timer", "nvidia,tegra20-timer";
 		reg = <0x60005000 0x400>;

Like for Tegra20, the Tegra timer is now switched to the LIC as parent.

Signed-off-by Marc Zyngier <[email protected]>

---
 .../interrupt-controller/nvidia,tegra-ictlr.txt    | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/nvidia,tegra-ictlr.txt

Documentation/devicetree/bindings/interrupt-controller/nvidia,tegra-ictlr.txt new file +0 -0

new file mode 100644
index 0000000..44fd873

Signed-off-by Marc Zyngier <[email protected]>

---
 .../interrupt-controller/nvidia,tegra-ictlr.txt    | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/nvidia,tegra-ictlr.txt

Documentation/devicetree/bindings/interrupt-controller/nvidia,tegra-ictlr.txt new file +2 -0

new file mode 100644
index 0000000..44fd873
@@ -0,0 +1,39 @@
+NVIDIA Legacy Interrupt Controller
+

Documentation/devicetree/bindings/interrupt-controller/nvidia,tegra-ictlr.txt new file +27 -0

+- reg : Specifies base physical address and size of the registers.
+  Each frame must be described separately.
+- interrupt-controller : Identifies the node as an interrupt controller.
+- #interrupt-cells : Specifies the number of cells needed to encode an
+  interrupt source. The value must be 3.
+- interrupt-parent : a phandle to the GIC these interrupts are routed
+  to.
+
+Notes:
+
+- Because this HW ultimately routes interrupts to the GIC, the
+  interrupt specifier must be that of the GIC.
+- Only SPIs can use the ictlr as an interrupt parent. SGIs and PPIs
+  are explicitely forbiden.
+
+Example:
+
+	ictlr: interrupt-controller at 60004000 {
+		compatible = "nvidia,tegra20-ictlr", "nvidia,tegra-ictlr";
 
+		reg = <0x60004000 64>,
+		      <0x60004100 64>,
+		      <0x60004200 64>,
+		      <0x60004300 64>;
+		interrupt-controller;
+		#interrupt-cells = <3>;
+		interrupt-parent = <&intc>;
+	};

Signature · 1 line

2.1.4

arch/arm/mach-tegra/irq.c +0 -0

void __init tegra_init_irq(void)
{

arch/arm/mach-tegra/irq.c +0 -22

-	int i;
-	void __iomem *distbase;
-
-	if (of_find_matching_node(NULL, tegra_ictlr_match))
-		goto skip_extn_setup;
-
-	tegra_legacy_irq_syscore_init();
-
-	distbase = IO_ADDRESS(TEGRA_ARM_INT_DIST_BASE);
-	num_ictlrs = readl_relaxed(distbase + GIC_DIST_CTR) & 0x1f;
-
-	if (num_ictlrs > ARRAY_SIZE(ictlr_reg_base)) {
-		WARN(1, "Too many (%d) interrupt controllers found. Maximum is %d.",
-			num_ictlrs, ARRAY_SIZE(ictlr_reg_base));
-		num_ictlrs = ARRAY_SIZE(ictlr_reg_base);
-	}
-
-	for (i = 0; i < num_ictlrs; i++) {
-		void __iomem *ictlr = ictlr_reg_base[i];
-		writel(~0, ictlr + ICTLR_CPU_IER_CLR);
-		writel(0, ictlr + ICTLR_CPU_IEP_CLASS);
-	}

On Wed, Jan 07, 2015 at 05:42:41PM +0000, Marc Zyngier wrote:
[...]

Quoted 1 line

arch/arm/mach-tegra/irq.c +0 -0

[...]

Quoted 34 lines

void __init tegra_init_irq(void)
{

arch/arm/mach-tegra/irq.c +0 -13

-	int i;
-	void __iomem *distbase;
-
-	if (of_find_matching_node(NULL, tegra_ictlr_match))
-		goto skip_extn_setup;
-
-	tegra_legacy_irq_syscore_init();
-
-	distbase = IO_ADDRESS(TEGRA_ARM_INT_DIST_BASE);
-	num_ictlrs = readl_relaxed(distbase + GIC_DIST_CTR) & 0x1f;
-
-	if (num_ictlrs > ARRAY_SIZE(ictlr_reg_base)) {
-		WARN(1, "Too many (%d) interrupt controllers found. Maximum is 

%d.",

arch/arm/mach-tegra/irq.c +1 -17

-			num_ictlrs, ARRAY_SIZE(ictlr_reg_base));
-		num_ictlrs = ARRAY_SIZE(ictlr_reg_base);
-	}
-
-	for (i = 0; i < num_ictlrs; i++) {
-		void __iomem *ictlr = ictlr_reg_base[i];
-		writel(~0, ictlr + ICTLR_CPU_IER_CLR);
-		writel(0, ictlr + ICTLR_CPU_IEP_CLASS);
-	}
-
-	gic_arch_extn.irq_ack = tegra_ack;
-	gic_arch_extn.irq_eoi = tegra_eoi;
-	gic_arch_extn.irq_mask = tegra_mask;
-	gic_arch_extn.irq_unmask = tegra_unmask;
-	gic_arch_extn.irq_retrigger = tegra_retrigger;
-	gic_arch_extn.irq_set_wake = tegra_set_wake;
-	gic_arch_extn.flags = IRQCHIP_MASK_ON_SUSPEND;
+	if (!of_find_matching_node(NULL, tegra_ictlr_match))

arch/arm/mach-tegra/irq.c static const struct of_device_id tegra_ictlr_match[] __initconst = { · line 98

Quoted 1 line

arch/arm/mach-tegra/irq.c +1 -0

+		pr_warn("Outdated DT detected, suspend/resume will NOT work\n");

I'm not very happy about the ABI breakage here, but I also realize that
we need this change to properly describe the hardware. To make it more
obvious that people really should update their DTBs, maybe turn this into a WARN()?

arch/arm/mach-tegra/irq.c static const struct of_device_id tegra_ictlr_match[] __initconst = { · line 100

Quoted 2 lines

arch/arm/mach-tegra/irq.c +0 -1

-skip_extn_setup:
 	tegra114_gic_cpu_pm_registration();

I'm not intimately familiar with the GIC, but is this really SoC specific? Doesn't anybody else need this? Comparing to the GIC spec the
write of 0x1e0 to the GIC_CPU_CTRL register (which I assume corresponds
to GICC_CTLR in the spec), this simply disables the IRQ and FIQ bypass
signals for both group 0 and group 1.

This proves to be usefull with stacked domains, when the current

Quoted 1 line · Marc Zyngier

This proves to be usefull with stacked domains, when the current

			^^ useful ?

minor:

(quoted diff) +6 -0

+WARNING: 'usefull' may be misspelled - perhaps 'useful'?
+#6:
+This proves to be usefull with stacked domains, when the current
+CHECK: extern prototypes should be avoided in .h files
+#23: FILE: include/linux/irq.h:463:
++extern int irq_chip_set_wake_parent(struct irq_data *data, unsigned

int on);

arch/arm/boot/dts/dra7.dtsi +0 -0

On 17:42-20150107, Marc Zyngier wrote:
[..]

arch/arm/boot/dts/dra7-evm.dts line 428

Quoted 13 lines

arch/arm/boot/dts/dra7-evm.dts +1 -1

index 10b725c..048cfeb 100644
@@ -423,7 +423,7 @@
 	status = "okay";
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart1_pins>;
-	interrupts-extended = <&gic GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>,
+	interrupts-extended = <&crossbar_mpu GIC_SPI 67 
 IRQ_TYPE_LEVEL_HIGH>,
 			      <&dra7_pmx_core 0x3e0>;

^^ interrrupt-extended for uart1 here
[..]

Quoted 2 lines

arch/arm/boot/dts/dra7.dtsi +0 -0

[...]

arch/arm/boot/dts/dra7.dtsi line 347

Quoted 6 lines

arch/arm/boot/dts/dra7.dtsi +1 -1

@@ -344,7 +344,7 @@
 		uart1: serial at 4806a000 {
 			compatible = "ti,omap4-uart";
 			reg = <0x4806a000 0x100>;
-			interrupts-extended = <&gic GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>;

^^ implies we will have both interrupts and interrupts-extended properties for uart1 in dra7-evm.dtb

Documentation/devicetree/bindings/interrupt-controller/interrupts.txt does not make it clear as to what the priority will be when both properties are present.

[...]
Also, for 3.19-rc3, Missing the following causing x15 to fail boot.

arch/arm/boot/dts/am57xx-beagle-x15.dts +1 -2

index 49edbda68cd5..c2241c2e5d9d 100644
@@ -335,7 +335,6 @@
 	mcp_rtc: rtc at 6f {
 		compatible = "microchip,mcp7941x";
 		reg = <0x6f>;
-		interrupt-parent = <&gic>;
 		interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_LOW>;  /* IRQ_SYS_1N */
 
 		pinctrl-names = "default";

@@ -358,7 +357,7 @@
 
 &uart3 {
 	status = "okay";
-	interrupts-extended = <&gic GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>,
+	interrupts-extended = <&crossbar_mpu GIC_SPI 69 
 IRQ_TYPE_LEVEL_HIGH>,
 			      <&dra7_pmx_core 0x248>;
 

pinctrl-names = "default";

drivers/irqchip/irq-crossbar.c +0 -0

drivers/irqchip/irq-crossbar.c +7 -2

+static int crossbar_domain_alloc(struct irq_domain *d, unsigned int virq,
+				 unsigned int nr_irqs, void *data)
 {
-	if (needs_crossbar_write(hw))
-		cb->write(hw - GIC_IRQ_START, cb->irq_map[hw - GIC_IRQ_START]);
+	struct of_phandle_args *args = data;
+	irq_hw_number_t hwirq;
+	int i;
+
+	if (args->args_count != 3)

drivers/irqchip/irq-crossbar.c +4 -0

+		if (err)
+			return err;
+
+		irq_domain_set_hwirq_and_chip(d, virq + i, hwirq + i,

- * @irq: virq number
+ * @virq: virq number
+ * @nr_irqs: number of irqs to free
*
* We do not maintain a use count of total number of map/unmap
* calls for a particular irq to find out if a irq can be really

drivers/irqchip/irq-crossbar.c +2 -1

@@ -117,14 +144,19 @@ static int crossbar_domain_map(struct irq_domain *d, unsigned int irq,
  * after which irq is anyways unusable. So an explicit map has to be called
  * after that.
  */
-static void crossbar_domain_unmap(struct irq_domain *d, unsigned int irq)
+static void crossbar_domain_free(struct irq_domain *domain, unsigned int virq,
+				 unsigned int nr_irqs)

The only user of the so called "routable domain" functionnality

OMAP4/5 has been (ab)using the gic_arch_extn to provide wakeup from suspend, and it makes a lot of sense to convert this code to use stacked domains instead.

This patch does just this, updating the DT files to actually reflect what the HW provides.

BIG FAT WARNING: because the DTs were so far lying by not exposing the WUGEN HW block, kernels with this patch applied won't have any suspend-resume facility when booted with old DTs, and old kernels with updated DTs won't even boot.

On a platform with this patch applied, the system looks like this:

root at bacon-fat:~# cat /proc/interrupts
CPU0 CPU1

 16:          0          0     WUGEN  37  gp_timer
 19:     233799     155916       GIC  27  arch_timer
 23:          0          0     WUGEN   9  l3-dbg-irq
 24:          1          0     WUGEN  10  l3-app-irq
 27:        282          0     WUGEN  13  omap-dma-engine
 44:          0          0  4ae10000.gpio  13  DMA
294:          0          0     WUGEN  20  gpmc
297:        506          0     WUGEN  56  48070000.i2c
298:          0          0     WUGEN  57  48072000.i2c
299:          0          0     WUGEN  61  48060000.i2c
300:          0          0     WUGEN  62  4807a000.i2c
301:          8          0     WUGEN  60  4807c000.i2c
308:       2439          0     WUGEN  74  OMAP UART2
312:        362          0     WUGEN  83  mmc2
313:        502          0     WUGEN  86  mmc0
314:         13          0     WUGEN  94  mmc1
350:          0          0      PRCM  pinctrl, pinctrl
406:   35155709          0       GIC 109  ehci_hcd:usb1
407:          0          0     WUGEN   7  palmas
409:          0          0     WUGEN 119  twl6040
410:          0          0   twl6040   5  twl6040_irq_ready
411:          0          0   twl6040   0  twl6040_irq_th
IPI0:          0          1  CPU wakeup interrupts
IPI1:          0          0  Timer broadcast interrupts
IPI2:      95334     902334  Rescheduling interrupts
IPI3:          0          0  Function call interrupts
IPI4:        479        648  Single function call interrupts
IPI5:          0          0  CPU stop interrupts
IPI6:          0          0  IRQ work interrupts
IPI7:          0          0  completion interrupts
Err:          0

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm/boot/dts/am4372.dtsi             |  11 ++-
 arch/arm/boot/dts/dra7.dtsi               |  12 ++-
 arch/arm/boot/dts/dra72x.dtsi             |   2 +-
 arch/arm/boot/dts/dra74x.dtsi             |   2 +-
 arch/arm/boot/dts/omap4-duovero.dtsi      |   2 -
 arch/arm/boot/dts/omap4-panda-common.dtsi |   8 +-
 arch/arm/boot/dts/omap4-sdp.dts           |   8 +-
 arch/arm/boot/dts/omap4-var-som-om44.dtsi |   2 -

 arch/arm/boot/dts/omap4.dtsi              |  18 ++++-
 arch/arm/boot/dts/omap5-cm-t54.dts        |   1 -
 arch/arm/boot/dts/omap5-uevm.dts          |   2 -
 arch/arm/boot/dts/omap5.dtsi              |  24 ++++--
 arch/arm/mach-omap2/omap-wakeupgen.c      | 125 +++++++++++++++++++++++-------
 arch/arm/mach-omap2/omap-wakeupgen.h      |   1 -
 arch/arm/mach-omap2/omap4-common.c        |   1 -
 15 files changed, 154 insertions(+), 65 deletions(-)

Quoted 60 lines · Marc Zyngier

OMAP4/5 has been (ab)using the gic_arch_extn to provide wakeup from suspend, and it makes a lot of sense to convert this code to use stacked domains instead.

This patch does just this, updating the DT files to actually reflect what the HW provides.

BIG FAT WARNING: because the DTs were so far lying by not exposing the WUGEN HW block, kernels with this patch applied won't have any suspend-resume facility when booted with old DTs, and old kernels with updated DTs won't even boot.

On a platform with this patch applied, the system looks like this:

root at bacon-fat:~# cat /proc/interrupts
CPU0 CPU1

 16:          0          0     WUGEN  37  gp_timer
 19:     233799     155916       GIC  27  arch_timer
 23:          0          0     WUGEN   9  l3-dbg-irq
 24:          1          0     WUGEN  10  l3-app-irq
 27:        282          0     WUGEN  13  omap-dma-engine
 44:          0          0  4ae10000.gpio  13  DMA
294:          0          0     WUGEN  20  gpmc
297:        506          0     WUGEN  56  48070000.i2c
298:          0          0     WUGEN  57  48072000.i2c
299:          0          0     WUGEN  61  48060000.i2c
300:          0          0     WUGEN  62  4807a000.i2c
301:          8          0     WUGEN  60  4807c000.i2c
308:       2439          0     WUGEN  74  OMAP UART2
312:        362          0     WUGEN  83  mmc2
313:        502          0     WUGEN  86  mmc0
314:         13          0     WUGEN  94  mmc1
350:          0          0      PRCM  pinctrl, pinctrl
406:   35155709          0       GIC 109  ehci_hcd:usb1
407:          0          0     WUGEN   7  palmas
409:          0          0     WUGEN 119  twl6040
410:          0          0   twl6040   5  twl6040_irq_ready
411:          0          0   twl6040   0  twl6040_irq_th
IPI0:          0          1  CPU wakeup interrupts
IPI1:          0          0  Timer broadcast interrupts
IPI2:      95334     902334  Rescheduling interrupts
IPI3:          0          0  Function call interrupts
IPI4:        479        648  Single function call interrupts
IPI5:          0          0  CPU stop interrupts
IPI6:          0          0  IRQ work interrupts
IPI7:          0          0  completion interrupts
Err:          0

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm/boot/dts/am4372.dtsi             |  11 ++-
 arch/arm/boot/dts/dra7.dtsi               |  12 ++-
 arch/arm/boot/dts/dra72x.dtsi             |   2 +-
 arch/arm/boot/dts/dra74x.dtsi             |   2 +-
 arch/arm/boot/dts/omap4-duovero.dtsi      |   2 -
 arch/arm/boot/dts/omap4-panda-common.dtsi |   8 +-
 arch/arm/boot/dts/omap4-sdp.dts           |   8 +-
 arch/arm/boot/dts/omap4-var-som-om44.dtsi |   2 -
 arch/arm/boot/dts/omap4.dtsi              |  18 ++++-

Quoted 8 lines

 arch/arm/boot/dts/omap5-cm-t54.dts        |   1 -
 arch/arm/boot/dts/omap5-uevm.dts          |   2 -
 arch/arm/boot/dts/omap5.dtsi              |  24 ++++--
 arch/arm/mach-omap2/omap-wakeupgen.c      | 125 
+++++++++++++++++++++++-------
 arch/arm/mach-omap2/omap-wakeupgen.h      |   1 -
 arch/arm/mach-omap2/omap4-common.c        |   1 -
 15 files changed, 154 insertions(+), 65 deletions(-)

we may need the following changes as well:

arch/arm/boot/dts/am437x-gp-evm.dts +0 -1

index 7eaae4cf9f89..69f2313a49d1 100644
@@ -280,7 +280,6 @@
 		reg = <0x24>;
 		compatible = "ti,tps65218";
 		interrupts = <GIC_SPI 7 IRQ_TYPE_NONE>; /* NMIn */
-		interrupt-parent = <&gic>;
 		interrupt-controller;
 		#interrupt-cells = <2>;
 

arch/arm/boot/dts/am437x-sk-evm.dts +0 -1

index 53bbfc90b26a..029badefad3d 100644
@@ -334,7 +334,6 @@
 	tps at 24 {
 		compatible = "ti,tps65218";
 		reg = <0x24>;
-		interrupt-parent = <&gic>;
 		interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
 		interrupt-controller;
 		#interrupt-cells = <2>;

arch/arm/boot/dts/am43x-epos-evm.dts +0 -1

index 662261d6b2ca..2d26bc11bad6 100644
@@ -333,7 +333,6 @@
 		reg = <0x24>;
 		compatible = "ti,tps65218";
 		interrupts = <GIC_SPI 7 IRQ_TYPE_NONE>; /* NMIn */
-		interrupt-parent = <&gic>;
 		interrupt-controller;
 		#interrupt-cells = <2>;
 

arch/arm/boot/dts/am57xx-beagle-x15.dts +1 -2

index 49edbda68cd5..c2241c2e5d9d 100644
@@ -335,7 +335,6 @@
 	mcp_rtc: rtc at 6f {
 		compatible = "microchip,mcp7941x";
 		reg = <0x6f>;
-		interrupt-parent = <&gic>;
 		interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_LOW>;  /* IRQ_SYS_1N */
 
 		pinctrl-names = "default";

@@ -358,7 +357,7 @@
 
 &uart3 {
 	status = "okay";
-	interrupts-extended = <&gic GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>,
+	interrupts-extended = <&crossbar_mpu GIC_SPI 69 
 IRQ_TYPE_LEVEL_HIGH>,
 			      <&dra7_pmx_core 0x248>;
 

arch/arm/boot/dts/am57xx-beagle-x15.dts +0 -0

 	pinctrl-names = "default";

arch/arm/boot/dts/omap5.dtsi +0 -1

index f9c75c782c48..b056156e2a7a 100644
@@ -893,14 +893,12 @@
 			usbhsohci: ohci at 4a064800 {
 				compatible = "ti,ohci-omap3";
 				reg = <0x4a064800 0x400>;
-				interrupt-parent = <&gic>;

Signed-off-by Marc Zyngier <[email protected]>

---
 .../interrupt-controller/ti,omap4-wugen-mpu        | 32 ++++++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ti,omap4-wugen-mpu

Documentation/devicetree/bindings/interrupt-controller/ti,omap4-wugen-mpu new file +0 -0

new file mode 100644
index 0000000..16149d9

Documentation/devicetree/bindings/interrupt-controller/ti,omap4-wugen-mpu new file +11 -0

+  are explicitely forbiden.
+
+Example:
+
+       wakeupgen: interrupt-controller at 48281000 {
+               compatible = "ti,omap5-wugen-mpu", "ti,omap4-wugen-mpu";
+               interrupt-controller;
+               #interrupt-cells = <3>;
+               reg = <0x48281000 0x1000>;
+               interrupt-parent = <&gic>;
+       };

Signature · 1 line

2.1.4

Quoted 17 lines · Marc Zyngier

Signed-off-by Marc Zyngier <[email protected]>

---
 .../interrupt-controller/ti,omap4-wugen-mpu        | 32 
++++++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 

Documentation/devicetree/bindings/interrupt-controller/ti,omap4-wugen-mpu

(quoted diff) +0 -0

a/Documentation/devicetree/bindings/interrupt-controller/ti,omap4-wugen-mpu b/Documentation/devicetree/bindings/interrupt-controller/ti,omap4-wugen-mpu new file mode 100644
index 0000000..16149d9
--- /dev/null
+++
b/Documentation/devicetree/bindings/interrupt-controller/ti,omap4-wugen-mpu

Documentation/devicetree/bindings/interrupt-controller/ti,omap4-wugen-mpu line 3

Quoted 3 lines

Documentation/devicetree/bindings/interrupt-controller/ti,omap4-wugen-mpu new file +3 -0

@@ -0,0 +1,32 @@
+TI OMAP4 Wake-up Generator
+
+All TI OMAP4/5 (and their derivatives) an interrupt controllerthat

controller that

Documentation/devicetree/bindings/interrupt-controller/ti,omap4-wugen-mpu line 9

Quoted 7 lines

(quoted diff) +1 -0

+routes interrupts to the GIC, and also serves as a wakeup source. 

It

Documentation/devicetree/bindings/interrupt-controller/ti,omap4-wugen-mpu new file +5 -0

+is also refered to as "WUGEN-MPU", hence the name of the binding.
+
+Reguired properties:
+
+- compatible : should contain at least "ti,omap4-wugen-mpu"

Could we also document ti,omap5-wugen-mpu. In addition, if you could make this patch prior to patch #12, it helps the checkpatch at the very
least ;)

also saw a few checkpatch warnings:

(quoted diff) +2 -0

+WARNING: 'refered' may be misspelled - perhaps 'referred'?
+#22: FILE:

Documentation/devicetree/bindings/interrupt-controller/ti,omap4-wugen-mpu:5:
++is also refered to as "WUGEN-MPU", hence the name of the binding.

(quoted diff) +2 -0

+WARNING: 'explicitely' may be misspelled - perhaps 'explicitly'?
+#39: FILE:

Documentation/devicetree/bindings/interrupt-controller/ti,omap4-wugen-mpu:22:
++ are explicitely forbiden.

Quoted 9 lines

+- reg : Specifies base physical address and size of the registers.
+- interrupt-controller : Identifies the node as an interrupt 

controller.
+- #interrupt-cells : Specifies the number of cells needed to encode an
+ interrupt source. The value must be 3.
+- interrupt-parent : a phandle to the GIC these interrupts are routed
+ to.

Documentation/devicetree/bindings/interrupt-controller/ti,omap4-wugen-mpu line 21

Quoted 6 lines

Documentation/devicetree/bindings/interrupt-controller/ti,omap4-wugen-mpu new file +6 -0

+
+Notes:
+
+- Because this HW ultimately routes interrupts to the GIC, the
+  interrupt specifier must be that of the GIC.
+- Only SPIs can use the ictlr as an interrupt parent. SGIs and PPIs

I think you mean interrupt controller and not nvidia ictlr here.. :)

IMX6 has been (ab)using the gic_arch_extn to provide wakeup from suspend, and it makes a lot of sense to convert this code to use stacked domains instead.

This patch does just this, updating the DT files to actually reflect what the HW provides.

IMX6 has been (ab)using the gic_arch_extn to provide wakeup from suspend, and it makes a lot of sense to convert this code to use stacked domains instead.

This patch does just this, updating the DT files to actually reflect what the HW provides.

BIG FAT WARNING: because the DTs were so far lying by not exposing the fact that the GPC block is actually the first interrupt controller in the chain, kernels with this patch applied wont have any suspend-resume facility when booted with old DTs, and old kernels with updated DTs won't even boot.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm/boot/dts/imx6qdl.dtsi  |   6 +-
 arch/arm/boot/dts/imx6sl.dtsi   |   5 +-
 arch/arm/boot/dts/imx6sx.dtsi   |   5 +-
 arch/arm/mach-imx/common.h      |   1 -
 arch/arm/mach-imx/gpc.c         | 127 ++++++++++++++++++++++++++++++++--------
 arch/arm/mach-imx/mach-imx6q.c  |   1 -
 arch/arm/mach-imx/mach-imx6sl.c |   1 -
 arch/arm/mach-imx/mach-imx6sx.c |   1 -
 8 files changed, 116 insertions(+), 31 deletions(-)

arch/arm/boot/dts/imx6qdl.dtsi +3 -1

index 4fc03b7..c16d428 100644
@@ -53,6 +53,7 @@
 		interrupt-controller;
 		reg = <0x00a01000 0x1000>,
 		      <0x00a00100 0x100>;
+		interrupt-parent = <&intc>;
 	};
 
 	clocks {

@@ -82,7 +83,7 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 		compatible = "simple-bus";
-		interrupt-parent = <&intc>;
+		interrupt-parent = <&gpc>;
 		ranges;
 
 		dma_apbh: dma-apbh at 00110000 {

@@ -122,6 +123,7 @@
 			compatible = "arm,cortex-a9-twd-timer";
 			reg = <0x00a00600 0x20>;
 			interrupts = <1 13 0xf01>;
+			interrupt-parent = <&intc>;
 			clocks = <&clks IMX6QDL_CLK_TWD>;
 		};
 

arch/arm/boot/dts/imx6qdl.dtsi +1 -0

 				interrupts = <0 89 IRQ_TYPE_LEVEL_HIGH>,
 					     <0 90 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-parent = <&intc>;
 			};
 
 			gpr: iomuxc-gpr at 020e0000 {

arch/arm/boot/dts/imx6sl.dtsi +4 -1

index 36ab8e0..35099b7 100644
@@ -72,6 +72,7 @@
 		interrupt-controller;
 		reg = <0x00a01000 0x1000>,
 		      <0x00a00100 0x100>;
+		interrupt-parent = <&intc>;
 	};
 
 	clocks {

@@ -95,7 +96,7 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 		compatible = "simple-bus";
-		interrupt-parent = <&intc>;
+		interrupt-parent = <&gpc>;
 		ranges;
 
 		ocram: sram at 00900000 {

@@ -603,7 +604,9 @@
 			gpc: gpc at 020dc000 {
 				compatible = "fsl,imx6sl-gpc", "fsl,imx6q-gpc";
 				reg = <0x020dc000 0x4000>;
+				interrupt-controller;
 				interrupts = <0 89 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-parent = <&intc>;
 			};
 
 			gpr: iomuxc-gpr at 020e0000 {

arch/arm/boot/dts/imx6sx.dtsi +4 -1

index 7a24fee..c476e67 100644
@@ -88,6 +88,7 @@
 		interrupt-controller;
 		reg = <0x00a01000 0x1000>,
 		      <0x00a00100 0x100>;
+		interrupt-parent = <&intc>;
 	};
 
 	clocks {

@@ -131,7 +132,7 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 		compatible = "simple-bus";
-		interrupt-parent = <&intc>;
+		interrupt-parent = <&gpc>;
 		ranges;
 
 		pmu {

@@ -700,7 +701,9 @@
 			gpc: gpc at 020dc000 {
 				compatible = "fsl,imx6sx-gpc", "fsl,imx6q-gpc";
 				reg = <0x020dc000 0x4000>;
+				interrupt-controller;
 				interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-parent = <&intc>;
 			};
 
 			iomuxc: iomuxc at 020e0000 {

arch/arm/mach-imx/common.h +0 -1

index cfcdb62..7052302 100644
@@ -102,7 +102,6 @@ static inline void imx_scu_map_io(void) {}
 static inline void imx_smp_prepare(void) {}
 #endif
 void imx_src_init(void);
-void imx_gpc_init(void);
 void imx_gpc_pre_suspend(bool arm_power_off);
 void imx_gpc_post_resume(void);
 void imx_gpc_mask_all(void);

arch/arm/mach-imx/gpc.c +9 -11

index 5f3602e..240109b 100644
@@ -22,6 +22,7 @@
 #define GPC_PGC_CPU_PDN		0x2a0
 
 #define IMR_NUM			4
+#define GPC_MAX_IRQS		(IMR_NUM * 32)
 
 static void __iomem *gpc_base;
 static u32 gpc_wake_irqs[IMR_NUM];

@@ -56,17 +57,17 @@ void imx_gpc_post_resume(void)
 
 static int imx_gpc_irq_set_wake(struct irq_data *d, unsigned int on)
 {
-	unsigned int idx = d->hwirq / 32 - 1;
+	unsigned int idx = d->hwirq / 32;
 	u32 mask;
 
-	/* Sanity check for SPI irq */
-	if (d->hwirq < 32)
-		return -EINVAL;
-
 	mask = 1 << d->hwirq % 32;
 	gpc_wake_irqs[idx] = on ? gpc_wake_irqs[idx] | mask :
 				  gpc_wake_irqs[idx] & ~mask;
 
+	/*
+	 * Do *not* call into the parent, as the GIC doesn't have any
+	 * wake-up facility...
+	 */
 	return 0;
 }
 

@@ -96,7 +97,7 @@ void imx_gpc_hwirq_unmask(unsigned int hwirq)
 	void __iomem *reg;
 	u32 val;
 
-	reg = gpc_base + GPC_IMR1 + (hwirq / 32 - 1) * 4;
+	reg = gpc_base + GPC_IMR1 + hwirq / 32 * 4;
 	val = readl_relaxed(reg);
 	val &= ~(1 << hwirq % 32);
 	writel_relaxed(val, reg);

@@ -107,7 +108,7 @@ void imx_gpc_hwirq_mask(unsigned int hwirq)
 	void __iomem *reg;
 	u32 val;
 
-	reg = gpc_base + GPC_IMR1 + (hwirq / 32 - 1) * 4;
+	reg = gpc_base + GPC_IMR1 + hwirq / 32 * 4;
 	val = readl_relaxed(reg);
 	val |= 1 << (hwirq % 32);
 	writel_relaxed(val, reg);

@@ -115,37 +116,115 @@ void imx_gpc_hwirq_mask(unsigned int hwirq)
 
 static void imx_gpc_irq_unmask(struct irq_data *d)
 {
-	/* Sanity check for SPI irq */
-	if (d->hwirq < 32)
-		return;
-
 	imx_gpc_hwirq_unmask(d->hwirq);
+	irq_chip_unmask_parent(d);
 }

arch/arm/mach-imx/gpc.c +92 -9

+
+static struct irq_chip imx_gpc_chip = {
+	.name		= "GPC",
+	.irq_eoi	= irq_chip_eoi_parent,
+	.irq_mask	= imx_gpc_irq_mask,
+	.irq_unmask	= imx_gpc_irq_unmask,
+	.irq_retrigger	= irq_chip_retrigger_hierarchy,
+	.irq_set_wake	= imx_gpc_irq_set_wake,
+};
+
+static int imx_gpc_domain_xlate(struct irq_domain *domain,
+				struct device_node *controller,
+				const u32 *intspec,
+				unsigned int intsize,
+				unsigned long *out_hwirq,
+				unsigned int *out_type)
+{
+	if (domain->of_node != controller)
+		return -EINVAL;	/* Shouldn't happen, really... */
+	if (intsize != 3)
+		return -EINVAL;	/* Not GIC compliant */
+	if (intspec[0] != 0)
+		return -EINVAL;	/* No PPI should point to this domain */
+
+	*out_hwirq = intspec[1];
+	*out_type = intspec[2];
+	return 0;
+}
+
+static int imx_gpc_domain_alloc(struct irq_domain *domain,
+				  unsigned int virq,
+				  unsigned int nr_irqs, void *data)
+{
+	struct of_phandle_args *args = data;
+	struct of_phandle_args parent_args;
+	irq_hw_number_t hwirq;
+	int i;
+
+	if (args->args_count != 3)
+		return -EINVAL;	/* Not GIC compliant */
+	if (args->args[0] != 0)
+		return -EINVAL;	/* No PPI should point to this domain */
+
+	hwirq = args->args[1];
+	if (hwirq >= GPC_MAX_IRQS)
+		return -EINVAL;	/* Can't deal with this */
+
+	for (i = 0; i < nr_irqs; i++)
+		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
+					      &imx_gpc_chip, NULL);
+
+	parent_args = *args;
+	parent_args.np = domain->parent->of_node;
+	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent_args);
 }
 
-void __init imx_gpc_init(void)
+static struct irq_domain_ops imx_gpc_domain_ops = {
+	.xlate	= imx_gpc_domain_xlate,
+	.alloc	= imx_gpc_domain_alloc,
+	.free	= irq_domain_free_irqs_common,
+};
+
+static int __init imx_gpc_init(struct device_node *node,
+			       struct device_node *parent)
 {
-	struct device_node *np;
+	struct irq_domain *parent_domain, *domain;
 	int i;
 
-	np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-gpc");
-	gpc_base = of_iomap(np, 0);
-	WARN_ON(!gpc_base);
+	if (!parent) {
+		pr_err("%s: no parent, giving up\n", node->full_name);
+		return -ENODEV;
+	}
+
+	parent_domain = irq_find_host(parent);
+	if (!parent_domain) {
+		pr_err("%s: unable to obtain parent domain\n", node->full_name);
+		return -ENXIO;
+	}
+
+	gpc_base = of_iomap(node, 0);
+	if (WARN_ON(!gpc_base))
+	        return -ENOMEM;
+
+	domain = irq_domain_add_hierarchy(parent_domain, 0, GPC_MAX_IRQS,
+					  node, &imx_gpc_domain_ops,
+					  NULL);
+	if (!domain) {
+		iounmap(gpc_base);
+		return -ENOMEM;
+	}
 
 	/* Initially mask all interrupts */
 	for (i = 0; i < IMR_NUM; i++)
 		writel_relaxed(~0, gpc_base + GPC_IMR1 + i * 4);
 
-	/* Register GPC as the secondary interrupt controller behind GIC */
-	gic_arch_extn.irq_mask = imx_gpc_irq_mask;
-	gic_arch_extn.irq_unmask = imx_gpc_irq_unmask;
-	gic_arch_extn.irq_set_wake = imx_gpc_irq_set_wake;
+	return 0;
 }
+
+/*
+ * We cannot use the IRQCHIP_DECLARE macro that lives in
+ * drivers/irqchip, so we're forced to roll our own. Not very nice.
+ */
+OF_DECLARE_2(irqchip, imx_gpc, "fsl,imx6q-gpc", imx_gpc_init);

arch/arm/mach-imx/mach-imx6q.c +0 -1

index 5057d61..bbe6ff8 100644
@@ -390,7 +390,6 @@ static void __init imx6q_init_irq(void)
 	imx_init_revision_from_anatop();
 	imx_init_l2cache();
 	imx_src_init();
-	imx_gpc_init();
 	irqchip_init();
 }
 

arch/arm/mach-imx/mach-imx6sl.c +0 -1

index 24bfaaf..d39c274 100644
@@ -64,7 +64,6 @@ static void __init imx6sl_init_irq(void)
 	imx_init_revision_from_anatop();
 	imx_init_l2cache();
 	imx_src_init();
-	imx_gpc_init();
 	irqchip_init();
 }
 

arch/arm/mach-imx/mach-imx6sx.c +0 -1

index 7a96c65..72fa22d 100644
@@ -84,7 +84,6 @@ static void __init imx6sx_init_irq(void)
 	imx_init_revision_from_anatop();
 	imx_init_l2cache();
 	imx_src_init();
-	imx_gpc_init();
 	irqchip_init();
 }

Hi Marc,

Quoted 56 lines · Marc Zyngier

IMX6 has been (ab)using the gic_arch_extn to provide wakeup from suspend, and it makes a lot of sense to convert this code to use stacked domains instead.

This patch does just this, updating the DT files to actually reflect what the HW provides.

BIG FAT WARNING: because the DTs were so far lying by not exposing the fact that the GPC block is actually the first interrupt controller in the chain, kernels with this patch applied wont have any suspend-resume facility when booted with old DTs, and old kernels with updated DTs won't even boot.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm/boot/dts/imx6qdl.dtsi  |   6 +-
 arch/arm/boot/dts/imx6sl.dtsi   |   5 +-
 arch/arm/boot/dts/imx6sx.dtsi   |   5 +-
 arch/arm/mach-imx/common.h      |   1 -
 arch/arm/mach-imx/gpc.c         | 127 
++++++++++++++++++++++++++++++++--------
 arch/arm/mach-imx/mach-imx6q.c  |   1 -
 arch/arm/mach-imx/mach-imx6sl.c |   1 -
 arch/arm/mach-imx/mach-imx6sx.c |   1 -
 8 files changed, 116 insertions(+), 31 deletions(-)

arch/arm/boot/dts/imx6qdl.dtsi +3 -1

index 4fc03b7..c16d428 100644
@@ -53,6 +53,7 @@
 		interrupt-controller;
 		reg = <0x00a01000 0x1000>,
 		      <0x00a00100 0x100>;
+		interrupt-parent = <&intc>;
 	};
 
 	clocks {

@@ -82,7 +83,7 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 		compatible = "simple-bus";
-		interrupt-parent = <&intc>;
+		interrupt-parent = <&gpc>;
 		ranges;
 
 		dma_apbh: dma-apbh at 00110000 {

@@ -122,6 +123,7 @@
 			compatible = "arm,cortex-a9-twd-timer";
 			reg = <0x00a00600 0x20>;
 			interrupts = <1 13 0xf01>;
+			interrupt-parent = <&intc>;
 			clocks = <&clks IMX6QDL_CLK_TWD>;
 		};
 

arch/arm/boot/dts/imx6qdl.dtsi line 699

Quoted 5 lines

arch/arm/boot/dts/imx6qdl.dtsi +1 -0

@@ -694,8 +696,10 @@
 			gpc: gpc at 020dc000 {
 				compatible = "fsl,imx6q-gpc";
 				reg = <0x020dc000 0x4000>;
+				interrupt-controller;

#interrupt-cells = <3>; is missing here.

I tested the patchset on a Colibri iMX6, but the module stopped booting
at some point. No error, no warn, but it looked like IRQ's are not working:

[    1.623939] platform sound: Driver imx-sgtl5000 requests probe
deferral
[    1.630677] backlight supply power not found, using dummy 
regulator
[    1.637067] pwm-backlight backlight: unable to request PWM, trying
legacy API
[    1.644271] pwm-backlight backlight: unable to request legacy PWM
[    1.650534] platform backlight: Driver pwm-backlight requests 
probe
deferral
[    1.658080] platform 2028000.ssi: Driver fsl-ssi-dai requests 
probe
deferral
[    1.665441] fec 2188000.ethernet eth0: Freescale FEC PHY driver
[Micrel KSZ8041] (mii_bus:phy_addr=2188000.ethernet:00, irq=-1)
[    1.677157] IPv6: ADDRCONF(NETDEV_UP): eth0: link is not ready

[freeze]

I figured out that the GPC code did not get called. After digging through the parsing code, I found the reason: irq_find_host always opted
to intc because this was missing... So, interrupt-cells mandatory for all interrupt-controller? Maybe we could add a warn somewhere..?

With that in place, it worked fine:

Tested-by Stefan Agner <[email protected]>

Quoted 149 lines

arch/arm/boot/dts/imx6qdl.dtsi +1 -0

 				interrupts = <0 89 IRQ_TYPE_LEVEL_HIGH>,
 					     <0 90 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-parent = <&intc>;
 			};
 
 			gpr: iomuxc-gpr at 020e0000 {

arch/arm/boot/dts/imx6sl.dtsi +4 -1

index 36ab8e0..35099b7 100644
@@ -72,6 +72,7 @@
 		interrupt-controller;
 		reg = <0x00a01000 0x1000>,
 		      <0x00a00100 0x100>;
+		interrupt-parent = <&intc>;
 	};
 
 	clocks {

@@ -95,7 +96,7 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 		compatible = "simple-bus";
-		interrupt-parent = <&intc>;
+		interrupt-parent = <&gpc>;
 		ranges;
 
 		ocram: sram at 00900000 {

@@ -603,7 +604,9 @@
 			gpc: gpc at 020dc000 {
 				compatible = "fsl,imx6sl-gpc", "fsl,imx6q-gpc";
 				reg = <0x020dc000 0x4000>;
+				interrupt-controller;
 				interrupts = <0 89 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-parent = <&intc>;
 			};
 
 			gpr: iomuxc-gpr at 020e0000 {

arch/arm/boot/dts/imx6sx.dtsi +4 -1

index 7a24fee..c476e67 100644
@@ -88,6 +88,7 @@
 		interrupt-controller;
 		reg = <0x00a01000 0x1000>,
 		      <0x00a00100 0x100>;
+		interrupt-parent = <&intc>;
 	};
 
 	clocks {

@@ -131,7 +132,7 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 		compatible = "simple-bus";
-		interrupt-parent = <&intc>;
+		interrupt-parent = <&gpc>;
 		ranges;
 
 		pmu {

@@ -700,7 +701,9 @@
 			gpc: gpc at 020dc000 {
 				compatible = "fsl,imx6sx-gpc", "fsl,imx6q-gpc";
 				reg = <0x020dc000 0x4000>;
+				interrupt-controller;
 				interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-parent = <&intc>;
 			};
 
 			iomuxc: iomuxc at 020e0000 {

arch/arm/mach-imx/common.h +0 -1

index cfcdb62..7052302 100644
@@ -102,7 +102,6 @@ static inline void imx_scu_map_io(void) {}
 static inline void imx_smp_prepare(void) {}
 #endif
 void imx_src_init(void);
-void imx_gpc_init(void);
 void imx_gpc_pre_suspend(bool arm_power_off);
 void imx_gpc_post_resume(void);
 void imx_gpc_mask_all(void);

arch/arm/mach-imx/gpc.c +9 -11

index 5f3602e..240109b 100644
@@ -22,6 +22,7 @@
 #define GPC_PGC_CPU_PDN		0x2a0
 
 #define IMR_NUM			4
+#define GPC_MAX_IRQS		(IMR_NUM * 32)
 
 static void __iomem *gpc_base;
 static u32 gpc_wake_irqs[IMR_NUM];

@@ -56,17 +57,17 @@ void imx_gpc_post_resume(void)
 
 static int imx_gpc_irq_set_wake(struct irq_data *d, unsigned int 
 on)
 {
-	unsigned int idx = d->hwirq / 32 - 1;
+	unsigned int idx = d->hwirq / 32;
 	u32 mask;
 
-	/* Sanity check for SPI irq */
-	if (d->hwirq < 32)
-		return -EINVAL;
-
 	mask = 1 << d->hwirq % 32;
 	gpc_wake_irqs[idx] = on ? gpc_wake_irqs[idx] | mask :
 				  gpc_wake_irqs[idx] & ~mask;
 
+	/*
+	 * Do *not* call into the parent, as the GIC doesn't have any
+	 * wake-up facility...
+	 */
 	return 0;
 }

@@ -96,7 +97,7 @@ void imx_gpc_hwirq_unmask(unsigned int hwirq)
 	void __iomem *reg;
 	u32 val;
 
-	reg = gpc_base + GPC_IMR1 + (hwirq / 32 - 1) * 4;
+	reg = gpc_base + GPC_IMR1 + hwirq / 32 * 4;
 	val = readl_relaxed(reg);
 	val &= ~(1 << hwirq % 32);
 	writel_relaxed(val, reg);

@@ -107,7 +108,7 @@ void imx_gpc_hwirq_mask(unsigned int hwirq)
 	void __iomem *reg;
 	u32 val;
 
-	reg = gpc_base + GPC_IMR1 + (hwirq / 32 - 1) * 4;
+	reg = gpc_base + GPC_IMR1 + hwirq / 32 * 4;
 	val = readl_relaxed(reg);
 	val |= 1 << (hwirq % 32);
 	writel_relaxed(val, reg);

@@ -115,37 +116,115 @@ void imx_gpc_hwirq_mask(unsigned int hwirq)
 
 static void imx_gpc_irq_unmask(struct irq_data *d)
 {
-	/* Sanity check for SPI irq */
-	if (d->hwirq < 32)
-		return;
-
 	imx_gpc_hwirq_unmask(d->hwirq);
+	irq_chip_unmask_parent(d);
 }
 

arch/arm/mach-imx/gpc.c void imx_gpc_hwirq_mask(unsigned int hwirq) · line 127

Quoted 9 lines

arch/arm/mach-imx/gpc.c +2 -4

 static void imx_gpc_irq_mask(struct irq_data *d)
 {
-	/* Sanity check for SPI irq */
-	if (d->hwirq < 32)
-		return;
-
 	imx_gpc_hwirq_mask(d->hwirq);
+	irq_chip_mask_parent(d);
+}

This two function end up very small, can't we just alter imx_gpc_hwirq_unmask to use struct irq_data directly?

Code looks fine to me:

Acked-by Stefan Agner <[email protected]>

Quoted 57 lines · Stefan Agner

Hi Marc,

Quoted 54 lines · Marc Zyngier

IMX6 has been (ab)using the gic_arch_extn to provide wakeup from suspend, and it makes a lot of sense to convert this code to use stacked domains instead.

This patch does just this, updating the DT files to actually reflect what the HW provides.

BIG FAT WARNING: because the DTs were so far lying by not exposing the fact that the GPC block is actually the first interrupt controller in the chain, kernels with this patch applied wont have any suspend-resume facility when booted with old DTs, and old kernels with updated DTs won't even boot.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm/boot/dts/imx6qdl.dtsi  |   6 +-
 arch/arm/boot/dts/imx6sl.dtsi   |   5 +-
 arch/arm/boot/dts/imx6sx.dtsi   |   5 +-
 arch/arm/mach-imx/common.h      |   1 -
 arch/arm/mach-imx/gpc.c         | 127 ++++++++++++++++++++++++++++++++--------
 arch/arm/mach-imx/mach-imx6q.c  |   1 -
 arch/arm/mach-imx/mach-imx6sl.c |   1 -
 arch/arm/mach-imx/mach-imx6sx.c |   1 -
 8 files changed, 116 insertions(+), 31 deletions(-)

arch/arm/boot/dts/imx6qdl.dtsi +3 -1

index 4fc03b7..c16d428 100644
@@ -53,6 +53,7 @@
 		interrupt-controller;
 		reg = <0x00a01000 0x1000>,
 		      <0x00a00100 0x100>;
+		interrupt-parent = <&intc>;
 	};
 
 	clocks {

@@ -82,7 +83,7 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 		compatible = "simple-bus";
-		interrupt-parent = <&intc>;
+		interrupt-parent = <&gpc>;
 		ranges;
 
 		dma_apbh: dma-apbh at 00110000 {

@@ -122,6 +123,7 @@
 			compatible = "arm,cortex-a9-twd-timer";
 			reg = <0x00a00600 0x20>;
 			interrupts = <1 13 0xf01>;
+			interrupt-parent = <&intc>;
 			clocks = <&clks IMX6QDL_CLK_TWD>;
 		};
 

Quoted 7 lines

arch/arm/boot/dts/imx6qdl.dtsi line 699

Quoted 5 lines

arch/arm/boot/dts/imx6qdl.dtsi +1 -0

@@ -694,8 +696,10 @@
 			gpc: gpc at 020dc000 {
 				compatible = "fsl,imx6q-gpc";
 				reg = <0x020dc000 0x4000>;
+				interrupt-controller;

#interrupt-cells = <3>; is missing here.

Ah, nice catch!

Quoted 19 lines

I tested the patchset on a Colibri iMX6, but the module stopped booting at some point. No error, no warn, but it looked like IRQ's are not working:

[    1.623939] platform sound: Driver imx-sgtl5000 requests probe
deferral
[    1.630677] backlight supply power not found, using dummy regulator
[    1.637067] pwm-backlight backlight: unable to request PWM, trying
legacy API
[    1.644271] pwm-backlight backlight: unable to request legacy PWM
[    1.650534] platform backlight: Driver pwm-backlight requests probe
deferral
[    1.658080] platform 2028000.ssi: Driver fsl-ssi-dai requests probe
deferral
[    1.665441] fec 2188000.ethernet eth0: Freescale FEC PHY driver
[Micrel KSZ8041] (mii_bus:phy_addr=2188000.ethernet:00, irq=-1)
[    1.677157] IPv6: ADDRCONF(NETDEV_UP): eth0: link is not ready

[freeze]

Quoted 4 lines

I figured out that the GPC code did not get called. After digging through the parsing code, I found the reason: irq_find_host always opted to intc because this was missing... So, interrupt-cells mandatory for all interrupt-controller? Maybe we could add a warn somewhere..?

interrupt-cells has a default of 1, I believe. I suppose I could add a WARN_ON in the xlate/alloc functions...

Quoted 3 lines

With that in place, it worked fine:

Tested-by Stefan Agner <[email protected]>

Thanks!

Quoted 146 lines

Quoted 146 lines

arch/arm/boot/dts/imx6qdl.dtsi +1 -0

 				interrupts = <0 89 IRQ_TYPE_LEVEL_HIGH>,
 					     <0 90 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-parent = <&intc>;
 			};
 
 			gpr: iomuxc-gpr at 020e0000 {

arch/arm/boot/dts/imx6sl.dtsi +4 -1

index 36ab8e0..35099b7 100644
@@ -72,6 +72,7 @@
 		interrupt-controller;
 		reg = <0x00a01000 0x1000>,
 		      <0x00a00100 0x100>;
+		interrupt-parent = <&intc>;
 	};
 
 	clocks {

@@ -95,7 +96,7 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 		compatible = "simple-bus";
-		interrupt-parent = <&intc>;
+		interrupt-parent = <&gpc>;
 		ranges;
 
 		ocram: sram at 00900000 {

@@ -603,7 +604,9 @@
 			gpc: gpc at 020dc000 {
 				compatible = "fsl,imx6sl-gpc", "fsl,imx6q-gpc";
 				reg = <0x020dc000 0x4000>;
+				interrupt-controller;
 				interrupts = <0 89 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-parent = <&intc>;
 			};
 
 			gpr: iomuxc-gpr at 020e0000 {

arch/arm/boot/dts/imx6sx.dtsi +4 -1

index 7a24fee..c476e67 100644
@@ -88,6 +88,7 @@
 		interrupt-controller;
 		reg = <0x00a01000 0x1000>,
 		      <0x00a00100 0x100>;
+		interrupt-parent = <&intc>;
 	};
 
 	clocks {

@@ -131,7 +132,7 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 		compatible = "simple-bus";
-		interrupt-parent = <&intc>;
+		interrupt-parent = <&gpc>;
 		ranges;
 
 		pmu {

@@ -700,7 +701,9 @@
 			gpc: gpc at 020dc000 {
 				compatible = "fsl,imx6sx-gpc", "fsl,imx6q-gpc";
 				reg = <0x020dc000 0x4000>;
+				interrupt-controller;
 				interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-parent = <&intc>;
 			};
 
 			iomuxc: iomuxc at 020e0000 {

arch/arm/mach-imx/common.h +0 -1

index cfcdb62..7052302 100644
@@ -102,7 +102,6 @@ static inline void imx_scu_map_io(void) {}
 static inline void imx_smp_prepare(void) {}
 #endif
 void imx_src_init(void);
-void imx_gpc_init(void);
 void imx_gpc_pre_suspend(bool arm_power_off);
 void imx_gpc_post_resume(void);
 void imx_gpc_mask_all(void);

arch/arm/mach-imx/gpc.c +9 -11

index 5f3602e..240109b 100644
@@ -22,6 +22,7 @@
 #define GPC_PGC_CPU_PDN		0x2a0
 
 #define IMR_NUM			4
+#define GPC_MAX_IRQS		(IMR_NUM * 32)
 
 static void __iomem *gpc_base;
 static u32 gpc_wake_irqs[IMR_NUM];

@@ -56,17 +57,17 @@ void imx_gpc_post_resume(void)
 
 static int imx_gpc_irq_set_wake(struct irq_data *d, unsigned int on)
 {
-	unsigned int idx = d->hwirq / 32 - 1;
+	unsigned int idx = d->hwirq / 32;
 	u32 mask;
 
-	/* Sanity check for SPI irq */
-	if (d->hwirq < 32)
-		return -EINVAL;
-
 	mask = 1 << d->hwirq % 32;
 	gpc_wake_irqs[idx] = on ? gpc_wake_irqs[idx] | mask :
 				  gpc_wake_irqs[idx] & ~mask;
 
+	/*
+	 * Do *not* call into the parent, as the GIC doesn't have any
+	 * wake-up facility...
+	 */
 	return 0;
 }
 

@@ -96,7 +97,7 @@ void imx_gpc_hwirq_unmask(unsigned int hwirq)
 	void __iomem *reg;
 	u32 val;
 
-	reg = gpc_base + GPC_IMR1 + (hwirq / 32 - 1) * 4;
+	reg = gpc_base + GPC_IMR1 + hwirq / 32 * 4;
 	val = readl_relaxed(reg);
 	val &= ~(1 << hwirq % 32);
 	writel_relaxed(val, reg);

@@ -107,7 +108,7 @@ void imx_gpc_hwirq_mask(unsigned int hwirq)
 	void __iomem *reg;
 	u32 val;
 
-	reg = gpc_base + GPC_IMR1 + (hwirq / 32 - 1) * 4;
+	reg = gpc_base + GPC_IMR1 + hwirq / 32 * 4;
 	val = readl_relaxed(reg);
 	val |= 1 << (hwirq % 32);
 	writel_relaxed(val, reg);

@@ -115,37 +116,115 @@ void imx_gpc_hwirq_mask(unsigned int hwirq)
 
 static void imx_gpc_irq_unmask(struct irq_data *d)
 {
-	/* Sanity check for SPI irq */
-	if (d->hwirq < 32)
-		return;
-
 	imx_gpc_hwirq_unmask(d->hwirq);
+	irq_chip_unmask_parent(d);
 }
 

Quoted 12 lines

arch/arm/mach-imx/gpc.c void imx_gpc_hwirq_mask(unsigned int hwirq) · line 127

Quoted 9 lines

arch/arm/mach-imx/gpc.c +2 -4

 static void imx_gpc_irq_mask(struct irq_data *d)
 {
-	/* Sanity check for SPI irq */
-	if (d->hwirq < 32)
-		return;
-
 	imx_gpc_hwirq_mask(d->hwirq);
+	irq_chip_mask_parent(d);
+}

This two function end up very small, can't we just alter imx_gpc_hwirq_unmask to use struct irq_data directly?