ARM: tegra: a couple of irq-related fixes

The crazy gic_arch_extn thing that Tegra uses contains multiple references to the irq field in struct irq_data, and uses this to directly poke hardware register.

But irq is the *virtual* irq number, something that has nothing to do with the actual HW irq (stored in the hwirq field). And once we put the stacked domain code in action, the whole thing explodes, as these two values are *very* different:

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

 16:      25801       2075       GIC  29  twd
 17:          0          0       GIC  73  timer0
112:          0          0      GPIO  58  c8000600.sdhci cd
123:          0          0      GPIO  69  c8000200.sdhci cd
279:       1126          0       GIC 122  serial
281:          0          0       GIC  70  7000c000.i2c
282:          0          0       GIC 116  7000c400.i2c
283:          0          0       GIC 124  7000c500.i2c
284:        300          0       GIC  85  7000d000.i2c

[...]

Just replacing all instances of irq with hwirq fixes the issue.

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

---
 arch/arm/mach-tegra/irq.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

Quoted 8 lines · Marc Zyngier

The crazy gic_arch_extn thing that Tegra uses contains multiple references to the irq field in struct irq_data, and uses this to directly poke hardware register.

But irq is the *virtual* irq number, something that has nothing to do with the actual HW irq (stored in the hwirq field). And once we put the stacked domain code in action, the whole thing explodes, as these two values are *very* different:

Do you have follow-up patches to use stacked domains on Tegra? I tried to move this driver out to drivers/irqchip at some point and that caused a bit of pain because of gic_arch_extn and probe order. At the time I was told that work was in progress to provide a more generic solution that could replace gic_arch_extn, which I'm assuming this stacked domain code is.

Quoted 11 lines

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

 16:      25801       2075       GIC  29  twd
 17:          0          0       GIC  73  timer0
112:          0          0      GPIO  58  c8000600.sdhci cd
123:          0          0      GPIO  69  c8000200.sdhci cd
279:       1126          0       GIC 122  serial
281:          0          0       GIC  70  7000c000.i2c
282:          0          0       GIC 116  7000c400.i2c
283:          0          0       GIC 124  7000c500.i2c
284:        300          0       GIC  85  7000d000.i2c

Quoted 8 lines

[...]

Just replacing all instances of irq with hwirq fixes the issue.

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

---
 arch/arm/mach-tegra/irq.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

This looks correct to me. Do you need this to base subsequent patches on or shall I just take those through the Tegra tree? I'm not sure if the ARM SoC maintainers will take a follow-up pull request for 3.19, so let me know if there's a hurry to get this in if it's going to make stacked domain code difficult to merge.

Hi Thierry,

Quoted 10 lines · Thierry Reding

Quoted 8 lines · Marc Zyngier

The crazy gic_arch_extn thing that Tegra uses contains multiple references to the irq field in struct irq_data, and uses this to directly poke hardware register.

But irq is the *virtual* irq number, something that has nothing to do with the actual HW irq (stored in the hwirq field). And once we put the stacked domain code in action, the whole thing explodes, as these two values are *very* different:

Quoted 6 lines

Do you have follow-up patches to use stacked domains on Tegra? I tried to move this driver out to drivers/irqchip at some point and that caused a bit of pain because of gic_arch_extn and probe order. At the time I was told that work was in progress to provide a more generic solution that could replace gic_arch_extn, which I'm assuming this stacked domain code is.

I'm working on that at the moment, and things look pretty good. The only issue I have so far is that this piece of HW needs to become the top-level interrupt-parent for all devices that are currently interrupting on the GIC. So far, the only solution I have is a change in the DT. But arguably, this should have been described in DT too...

Quoted 20 lines

Quoted 11 lines

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

 16:      25801       2075       GIC  29  twd
 17:          0          0       GIC  73  timer0
112:          0          0      GPIO  58  c8000600.sdhci cd
123:          0          0      GPIO  69  c8000200.sdhci cd
279:       1126          0       GIC 122  serial
281:          0          0       GIC  70  7000c000.i2c
282:          0          0       GIC 116  7000c400.i2c
283:          0          0       GIC 124  7000c500.i2c
284:        300          0       GIC  85  7000d000.i2c

Quoted 8 lines

[...]

Just replacing all instances of irq with hwirq fixes the issue.

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

---
 arch/arm/mach-tegra/irq.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

Quoted 5 lines

This looks correct to me. Do you need this to base subsequent patches on or shall I just take those through the Tegra tree? I'm not sure if the ARM SoC maintainers will take a follow-up pull request for 3.19, so let me know if there's a hurry to get this in if it's going to make stacked domain code difficult to merge.

Up to you, really. The only real issue is that when the stacked domain code hits mainline, Tegra will stop working without this patch. -next is probably already broken.

I'd be tempted to consider it as a fix for 3.18, as the current code is obviously wrong. Just let me know how you want to get it in.

Quoted 3 lines

I'd be tempted to consider it as a fix for 3.18, as the current code is obviously wrong. Just let me know how you want to get it in.

That's fine with me:

Acked-by Thierry Reding <[email protected]>

Arnd, is this anything you could pick up for 3.18?

Quoted 4 lines · On Thursday 27 November 2014 13:08:28 Thierry Reding wrote:

Quoted 3 lines

I'd be tempted to consider it as a fix for 3.18, as the current code is obviously wrong. Just let me know how you want to get it in.

Quoted 6 lines

That's fine with me:

Acked-by Thierry Reding <[email protected]>

Arnd, is this anything you could pick up for 3.18?

Sure, no problem. Applied to fixes branch now.

Quoted 20 lines · Marc Zyngier

Hi Thierry,

Quoted 10 lines · Thierry Reding

Quoted 8 lines · Marc Zyngier

The crazy gic_arch_extn thing that Tegra uses contains multiple references to the irq field in struct irq_data, and uses this to directly poke hardware register.

But irq is the *virtual* irq number, something that has nothing to do with the actual HW irq (stored in the hwirq field). And once we put the stacked domain code in action, the whole thing explodes, as these two values are *very* different:

Quoted 6 lines

Do you have follow-up patches to use stacked domains on Tegra? I tried to move this driver out to drivers/irqchip at some point and that caused a bit of pain because of gic_arch_extn and probe order. At the time I was told that work was in progress to provide a more generic solution that could replace gic_arch_extn, which I'm assuming this stacked domain code is.

Quoted 5 lines

I'm working on that at the moment, and things look pretty good. The only issue I have so far is that this piece of HW needs to become the top-level interrupt-parent for all devices that are currently interrupting on the GIC. So far, the only solution I have is a change in the DT. But arguably, this should have been described in DT too...

I think I had discussed this with Arnd (Cc'ed) at some point but I can't find a link to the discussion (perhaps it was on IRC). The outcome I think was that from the CPU's perspective the GIC would still be the interrupt parent of the devices, whereas the LIC would become the interrupt parent of the GIC.

Admittedly, though, everytime I think about this I start feeling dizzy, so perhaps I'm mixing this up again.

Maybe a picture to clarify for my own sake how this works:

	             /-----\     /-----\     /-----\
	   various --|     |     |     |     |     |
	  hardware --| LIC |-----| GIC |-----| CPU |
	interrupts --|     |     |     |  |  |     |
	             \-----/     \-----/  |  \-----/
	                |                 |
	             /-----\              |  /-----\
	             |     |              |  |     |
	             | AVP |              ---| CPU |
	             |     |              .  |     |
	             \-----/              .  \-----/
	                                  .

That is, interrupts are first routed to the LIC, which will primarily be used by the AVP. The LIC is also configured (and that's the part where gic_arch_extn comes into play) to forward interrupts to the GIC which will distribute them to the Cortex-AXs.

Therefore, from the CPU perspective, the interrupt-parent of devices should still be the GIC, since that's where the interrupt numbers will need to come from in order to set up interrupt handlers. For any of these interrupts GIC will need to program LIC so that they are forwarded and can be used to wake up CPUs.

Doesn't that simplify everything to just adding an interrupt-parent property to GIC referencing LIC?

Quoted 20 lines · Marc Zyngier

Hi Thierry,

Quoted 10 lines · Thierry Reding

Quoted 8 lines · Marc Zyngier

The crazy gic_arch_extn thing that Tegra uses contains multiple references to the irq field in struct irq_data, and uses this to directly poke hardware register.

But irq is the *virtual* irq number, something that has nothing to do with the actual HW irq (stored in the hwirq field). And once we put the stacked domain code in action, the whole thing explodes, as these two values are *very* different:

Quoted 6 lines

Do you have follow-up patches to use stacked domains on Tegra? I tried to move this driver out to drivers/irqchip at some point and that caused a bit of pain because of gic_arch_extn and probe order. At the time I was told that work was in progress to provide a more generic solution that could replace gic_arch_extn, which I'm assuming this stacked domain code is.

Quoted 5 lines

I'm working on that at the moment, and things look pretty good. The only issue I have so far is that this piece of HW needs to become the top-level interrupt-parent for all devices that are currently interrupting on the GIC. So far, the only solution I have is a change in the DT. But arguably, this should have been described in DT too...

I think I had discussed this with Arnd (Cc'ed) at some point but I can't find a link to the discussion (perhaps it was on IRC). The outcome I think was that from the CPU's perspective the GIC would still be the interrupt parent of the devices, whereas the LIC would become the interrupt parent of the GIC.

Admittedly, though, everytime I think about this I start feeling dizzy, so perhaps I'm mixing this up again.

Maybe a picture to clarify for my own sake how this works:

	             /-----\     /-----\     /-----\
	   various --|     |     |     |     |     |
	  hardware --| LIC |-----| GIC |-----| CPU |
	interrupts --|     |     |     |  |  |     |
	             \-----/     \-----/  |  \-----/
	                |                 |
	             /-----\              |  /-----\
	             |     |              |  |     |
	             | AVP |              ---| CPU |
	             |     |              .  |     |
	             \-----/              .  \-----/
	                                  .

That is, interrupts are first routed to the LIC, which will primarily be used by the AVP. The LIC is also configured (and that's the part where gic_arch_extn comes into play) to forward interrupts to the GIC which will distribute them to the Cortex-AXs.

Therefore, from the CPU perspective, the interrupt-parent of devices should still be the GIC, since that's where the interrupt numbers will need to come from in order to set up interrupt handlers. For any of these interrupts GIC will need to program LIC so that they are forwarded and can be used to wake up CPUs.

Doesn't that simplify everything to just adding an interrupt-parent property to GIC referencing LIC?

From a DT perspective, all devices (except those using PPIs) are

Quoted 56 lines · Thierry Reding

Quoted 20 lines · Marc Zyngier

Hi Thierry,

Quoted 10 lines · Thierry Reding

Quoted 8 lines · Marc Zyngier

The crazy gic_arch_extn thing that Tegra uses contains multiple references to the irq field in struct irq_data, and uses this to directly poke hardware register.

But irq is the *virtual* irq number, something that has nothing to do with the actual HW irq (stored in the hwirq field). And once we put the stacked domain code in action, the whole thing explodes, as these two values are *very* different:

Quoted 6 lines

Do you have follow-up patches to use stacked domains on Tegra? I tried to move this driver out to drivers/irqchip at some point and that caused a bit of pain because of gic_arch_extn and probe order. At the time I was told that work was in progress to provide a more generic solution that could replace gic_arch_extn, which I'm assuming this stacked domain code is.

Quoted 5 lines

I'm working on that at the moment, and things look pretty good. The only issue I have so far is that this piece of HW needs to become the top-level interrupt-parent for all devices that are currently interrupting on the GIC. So far, the only solution I have is a change in the DT. But arguably, this should have been described in DT too...

I think I had discussed this with Arnd (Cc'ed) at some point but I can't find a link to the discussion (perhaps it was on IRC). The outcome I think was that from the CPU's perspective the GIC would still be the interrupt parent of the devices, whereas the LIC would become the interrupt parent of the GIC.

Admittedly, though, everytime I think about this I start feeling dizzy, so perhaps I'm mixing this up again.

Maybe a picture to clarify for my own sake how this works:

	             /-----\     /-----\     /-----\
	   various --|     |     |     |     |     |
	  hardware --| LIC |-----| GIC |-----| CPU |
	interrupts --|     |     |     |  |  |     |
	             \-----/     \-----/  |  \-----/
	                |                 |
	             /-----\              |  /-----\
	             |     |              |  |     |
	             | AVP |              ---| CPU |
	             |     |              .  |     |
	             \-----/              .  \-----/
	                                  .

That is, interrupts are first routed to the LIC, which will primarily be used by the AVP. The LIC is also configured (and that's the part where gic_arch_extn comes into play) to forward interrupts to the GIC which will distribute them to the Cortex-AXs.

Quoted 8 lines

Therefore, from the CPU perspective, the interrupt-parent of devices should still be the GIC, since that's where the interrupt numbers will need to come from in order to set up interrupt handlers. For any of these interrupts GIC will need to program LIC so that they are forwarded and can be used to wake up CPUs.

Doesn't that simplify everything to just adding an interrupt-parent property to GIC referencing LIC?

Actually, this is exactly the opposite! ;-)

From a DT perspective, all devices (except those using PPIs) are connected to the LIC. Therefore, their interrupt parent has to be the LIC, and I don't think there is a way out of that.

Now, the stacked domains give you a lot of flexibility, and that's what we need to implement this:
- The LIC gets its own domain, and so does the GIC.
- For virq X, you get hwirq Y in the LIC domain, and hwirw Z in the GIC domain. The don't have to be identical.

For example, take the interrupt associated to UARTD, which happens to be SPI90. Its virq is 279, its hwirq in the LIC domain is 90, and 122 in the GIC.

When the interrupt fires, the domain lookup at the GIC level will convert 122 (GIC) to 279 (virq), and process the the interrupt going through the stacked domains, each domain processing its view of the interrupt. Much nicer.

If course, all of this mandates that the device appears to be attached to the LIC, but I believe that it is what both your drawing and the TRM describe.

Quoted 82 lines · Marc Zyngier

Quoted 56 lines · Thierry Reding

Quoted 20 lines · Marc Zyngier

Hi Thierry,

Quoted 10 lines · Thierry Reding

Quoted 8 lines · Marc Zyngier

The crazy gic_arch_extn thing that Tegra uses contains multiple references to the irq field in struct irq_data, and uses this to directly poke hardware register.

But irq is the *virtual* irq number, something that has nothing to do with the actual HW irq (stored in the hwirq field). And once we put the stacked domain code in action, the whole thing explodes, as these two values are *very* different:

Quoted 6 lines

Do you have follow-up patches to use stacked domains on Tegra? I tried to move this driver out to drivers/irqchip at some point and that caused a bit of pain because of gic_arch_extn and probe order. At the time I was told that work was in progress to provide a more generic solution that could replace gic_arch_extn, which I'm assuming this stacked domain code is.

Quoted 5 lines

I'm working on that at the moment, and things look pretty good. The only issue I have so far is that this piece of HW needs to become the top-level interrupt-parent for all devices that are currently interrupting on the GIC. So far, the only solution I have is a change in the DT. But arguably, this should have been described in DT too...

I think I had discussed this with Arnd (Cc'ed) at some point but I can't find a link to the discussion (perhaps it was on IRC). The outcome I think was that from the CPU's perspective the GIC would still be the interrupt parent of the devices, whereas the LIC would become the interrupt parent of the GIC.

Admittedly, though, everytime I think about this I start feeling dizzy, so perhaps I'm mixing this up again.

Maybe a picture to clarify for my own sake how this works:

	             /-----\     /-----\     /-----\
	   various --|     |     |     |     |     |
	  hardware --| LIC |-----| GIC |-----| CPU |
	interrupts --|     |     |     |  |  |     |
	             \-----/     \-----/  |  \-----/
	                |                 |
	             /-----\              |  /-----\
	             |     |              |  |     |
	             | AVP |              ---| CPU |
	             |     |              .  |     |
	             \-----/              .  \-----/
	                                  .

That is, interrupts are first routed to the LIC, which will primarily be used by the AVP. The LIC is also configured (and that's the part where gic_arch_extn comes into play) to forward interrupts to the GIC which will distribute them to the Cortex-AXs.

Quoted 8 lines

Therefore, from the CPU perspective, the interrupt-parent of devices should still be the GIC, since that's where the interrupt numbers will need to come from in order to set up interrupt handlers. For any of these interrupts GIC will need to program LIC so that they are forwarded and can be used to wake up CPUs.

Doesn't that simplify everything to just adding an interrupt-parent property to GIC referencing LIC?

Actually, this is exactly the opposite! ;-)

From a DT perspective, all devices (except those using PPIs) are connected to the LIC. Therefore, their interrupt parent has to be the LIC, and I don't think there is a way out of that.

Now, the stacked domains give you a lot of flexibility, and that's what we need to implement this:
- The LIC gets its own domain, and so does the GIC.
- For virq X, you get hwirq Y in the LIC domain, and hwirw Z in the GIC domain. The don't have to be identical.

For example, take the interrupt associated to UARTD, which happens to be SPI90. Its virq is 279, its hwirq in the LIC domain is 90, and 122 in the GIC.

Quoted 8 lines

When the interrupt fires, the domain lookup at the GIC level will convert 122 (GIC) to 279 (virq), and process the the interrupt going through the stacked domains, each domain processing its view of the interrupt. Much nicer.

If course, all of this mandates that the device appears to be attached to the LIC, but I believe that it is what both your drawing and the TRM describe.

The drawing was derived from my reading of the TRM, so I'm comforted that it matches your interpretation as well. =)

Thanks for explaining this in so much detail. That makes perfect sense. On the other hand it means that we'd be breaking DTs in a backwards- incompatibile way, severely. Would there be provision for some sort of fallback to keep existing DTBs working?

On 27/11/14 14:45, Thierry Reding wrote:

Quoted 4 lines

Thanks for explaining this in so much detail. That makes perfect sense. On the other hand it means that we'd be breaking DTs in a backwards- incompatibile way, severely. Would there be provision for some sort of fallback to keep existing DTBs working?

I've been thinking of this, but that's not very nice. The box will probably still boot, but suspend will be broken (all interrupts would be pointing to the GIC, bypassing the LIC entirely).

I've been looking at how to patch the FDT at runtime (ideally replacing the top-level interrupt-parent), but that doesn't seem to be possible.

Suggestions?

From what I remember the gic_arch_extn structure had a flags field that

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(-)

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

index ab95f53..7f87a50 100644
@@ -283,13 +283,5 @@ void __init tegra_init_irq(void)
 	gic_arch_extn.irq_set_wake = tegra_set_wake;
 	gic_arch_extn.flags = IRQCHIP_MASK_ON_SUSPEND;
 

Quoted 16 lines · Marc Zyngier

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(-)

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

index ab95f53..7f87a50 100644
@@ -283,13 +283,5 @@ void __init tegra_init_irq(void)
 	gic_arch_extn.irq_set_wake = tegra_set_wake;
 	gic_arch_extn.flags = IRQCHIP_MASK_ON_SUSPEND;
 

arch/arm/mach-tegra/irq.c void __init tegra_init_irq(void) · line 287

Quoted 10 lines

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

-	/*
-	 * Check if there is a devicetree present, since the GIC will be
-	 * initialized elsewhere under DT.
-	 */
-	if (!of_have_populated_dt())
-		gic_init(0, 29, distbase,
-			IO_ADDRESS(TEGRA_ARM_PERIF_BASE + 0x100));
-
 	tegra114_gic_cpu_pm_registration();
 }

I've been carrying the same patch in my tree locally for months and was just waiting for the stacked domain code to settle. So if you prefer to merge this as part of other stacked domain code, consider this:

Acked-by Thierry Reding <[email protected]>