[PATCH 0/2] ARM: tegra: a couple of irq-related fixes

[PATCH 0/2] ARM: tegra: a couple of irq-related fixes

[Marc Zyngier]

As I've started looking at getting rid of the horrible gic_arch_extn
hack by using stacked irq domains, I couldn't help but notice a couple
of nits, the first one being fairly major:

- The shadow interrupt controller is using the irq field instead of
  hwirq, which leads to fun results...
- We still have some leftovers from non-DT setups, which don't seem to
  exist anymore.

Patches are against v3.18-rc6, and have been tested on a Harmony
board.

Thanks,

	M.

Marc Zyngier (2):
  ARM: tegra: irq: fix buggy usage of irq_data irq field
  ARM: tegra: irq: nuke leftovers from non-DT support

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

-- 
2.1.3

[PATCH 1/2] ARM: tegra: irq: fix buggy usage of irq_data irq field

[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:

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 <marc.zyngier@arm.com>
---
 arch/arm/mach-tegra/irq.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/arm/mach-tegra/irq.c b/arch/arm/mach-tegra/irq.c
index da7be13..ab95f53 100644
--- a/arch/arm/mach-tegra/irq.c
+++ b/arch/arm/mach-tegra/irq.c
@@ -99,42 +99,42 @@ static inline void tegra_irq_write_mask(unsigned int irq, unsigned long reg)
 
 static void tegra_mask(struct irq_data *d)
 {
-	if (d->irq < FIRST_LEGACY_IRQ)
+	if (d->hwirq < FIRST_LEGACY_IRQ)
 		return;
 
-	tegra_irq_write_mask(d->irq, ICTLR_CPU_IER_CLR);
+	tegra_irq_write_mask(d->hwirq, ICTLR_CPU_IER_CLR);
 }
 
 static void tegra_unmask(struct irq_data *d)
 {
-	if (d->irq < FIRST_LEGACY_IRQ)
+	if (d->hwirq < FIRST_LEGACY_IRQ)
 		return;
 
-	tegra_irq_write_mask(d->irq, ICTLR_CPU_IER_SET);
+	tegra_irq_write_mask(d->hwirq, ICTLR_CPU_IER_SET);
 }
 
 static void tegra_ack(struct irq_data *d)
 {
-	if (d->irq < FIRST_LEGACY_IRQ)
+	if (d->hwirq < FIRST_LEGACY_IRQ)
 		return;
 
-	tegra_irq_write_mask(d->irq, ICTLR_CPU_IEP_FIR_CLR);
+	tegra_irq_write_mask(d->hwirq, ICTLR_CPU_IEP_FIR_CLR);
 }
 
 static void tegra_eoi(struct irq_data *d)
 {
-	if (d->irq < FIRST_LEGACY_IRQ)
+	if (d->hwirq < FIRST_LEGACY_IRQ)
 		return;
 
-	tegra_irq_write_mask(d->irq, ICTLR_CPU_IEP_FIR_CLR);
+	tegra_irq_write_mask(d->hwirq, ICTLR_CPU_IEP_FIR_CLR);
 }
 
 static int tegra_retrigger(struct irq_data *d)
 {
-	if (d->irq < FIRST_LEGACY_IRQ)
+	if (d->hwirq < FIRST_LEGACY_IRQ)
 		return 0;
 
-	tegra_irq_write_mask(d->irq, ICTLR_CPU_IEP_FIR_SET);
+	tegra_irq_write_mask(d->hwirq, ICTLR_CPU_IEP_FIR_SET);
 
 	return 1;
 }
@@ -142,7 +142,7 @@ static int tegra_retrigger(struct irq_data *d)
 #ifdef CONFIG_PM_SLEEP
 static int tegra_set_wake(struct irq_data *d, unsigned int enable)
 {
-	u32 irq = d->irq;
+	u32 irq = d->hwirq;
 	u32 index, mask;
 
 	if (irq < FIRST_LEGACY_IRQ ||
-- 
2.1.3

[PATCH 2/2] ARM: tegra: irq: nuke leftovers from non-DT support

[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 <marc.zyngier@arm.com>
---
 arch/arm/mach-tegra/irq.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/arch/arm/mach-tegra/irq.c b/arch/arm/mach-tegra/irq.c
index ab95f53..7f87a50 100644
--- a/arch/arm/mach-tegra/irq.c
+++ b/arch/arm/mach-tegra/irq.c
@@ -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;
 
-	/*
-	 * 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();
 }
-- 
2.1.3

[PATCH 1/2] ARM: tegra: irq: fix buggy usage of irq_data irq field

[Thierry Reding]

On Wed, Nov 26, 2014 at 05:55:31PM +0000, Marc Zyngier wrote:
> 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.

> 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 <marc.zyngier@arm.com>
> ---
>  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.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141127/551a4aa8/attachment.sig>

[PATCH 2/2] ARM: tegra: irq: nuke leftovers from non-DT support

[Thierry Reding]

On Wed, Nov 26, 2014 at 05:55:32PM +0000, Marc Zyngier wrote:
> 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 <marc.zyngier@arm.com>
> ---
>  arch/arm/mach-tegra/irq.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/arch/arm/mach-tegra/irq.c b/arch/arm/mach-tegra/irq.c
> index ab95f53..7f87a50 100644
> --- a/arch/arm/mach-tegra/irq.c
> +++ b/arch/arm/mach-tegra/irq.c
> @@ -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;
>  
> -	/*
> -	 * 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 <treding@nvidia.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141127/db9a7c9a/attachment-0001.sig>

[PATCH 1/2] ARM: tegra: irq: fix buggy usage of irq_data irq field

[Marc Zyngier]

Hi Thierry,

On 27/11/14 08:28, Thierry Reding wrote:
> On Wed, Nov 26, 2014 at 05:55:31PM +0000, Marc Zyngier wrote:
>> 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.

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...

>> 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 <marc.zyngier@arm.com>
>> ---
>>  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.

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.

Thanks,

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

[PATCH 2/2] ARM: tegra: irq: nuke leftovers from non-DT support

[Marc Zyngier]

On 27/11/14 08:31, Thierry Reding wrote:
> On Wed, Nov 26, 2014 at 05:55:32PM +0000, Marc Zyngier wrote:
>> 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 <marc.zyngier@arm.com>
>> ---
>>  arch/arm/mach-tegra/irq.c | 8 --------
>>  1 file changed, 8 deletions(-)
>>
>> diff --git a/arch/arm/mach-tegra/irq.c b/arch/arm/mach-tegra/irq.c
>> index ab95f53..7f87a50 100644
>> --- a/arch/arm/mach-tegra/irq.c
>> +++ b/arch/arm/mach-tegra/irq.c
>> @@ -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;
>>  
>> -	/*
>> -	 * 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 <treding@nvidia.com>

Fine by me, I'll stash it in the rest of the series.

Thanks,

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

[PATCH 1/2] ARM: tegra: irq: fix buggy usage of irq_data irq field

[Thierry Reding]

On Thu, Nov 27, 2014 at 09:08:26AM +0000, Marc Zyngier wrote:
> Hi Thierry,
> 
> On 27/11/14 08:28, Thierry Reding wrote:
> > On Wed, Nov 26, 2014 at 05:55:31PM +0000, Marc Zyngier wrote:
> >> 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.
> 
> 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?

> >> 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 <marc.zyngier@arm.com>
> >> ---
> >>  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.
> 
> 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've been running next-20141126 and nothing seems obviously broken.
Perhaps the numbering doesn't change until we actually enable stacked
domains by wiring this up to the GIC?

> 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 <treding@nvidia.com>

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

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141127/637edd5f/attachment-0001.sig>

[PATCH 1/2] ARM: tegra: irq: fix buggy usage of irq_data irq field

[Arnd Bergmann]

On Thursday 27 November 2014 13:08:28 Thierry Reding wrote:
> 
> > 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 <treding@nvidia.com>
> 
> Arnd, is this anything you could pick up for 3.18?
> 

Sure, no problem. Applied to fixes branch now.

	Arnd

[PATCH 1/2] ARM: tegra: irq: fix buggy usage of irq_data irq field

[Thierry Reding]

On Thu, Nov 27, 2014 at 02:02:48PM +0100, Arnd Bergmann wrote:
> On Thursday 27 November 2014 13:08:28 Thierry Reding wrote:
> > 
> > > 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 <treding@nvidia.com>
> > 
> > Arnd, is this anything you could pick up for 3.18?
> > 
> 
> Sure, no problem. Applied to fixes branch now.

Thanks,
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141127/ae2072c8/attachment.sig>

[PATCH 1/2] ARM: tegra: irq: fix buggy usage of irq_data irq field

[Mark Rutland]

On Thu, Nov 27, 2014 at 12:08:28PM +0000, Thierry Reding wrote:
> On Thu, Nov 27, 2014 at 09:08:26AM +0000, Marc Zyngier wrote:
> > Hi Thierry,
> > 
> > On 27/11/14 08:28, Thierry Reding wrote:
> > > On Wed, Nov 26, 2014 at 05:55:31PM +0000, Marc Zyngier wrote:
> > >> 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.
> > 
> > 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.

For a given node, the interrupt parent is whatever the interrupt is fed
directly into (and hence the interrupts exist within the domain of). Per
the diagram below, the LIC is the interrupt parent of the devices, and
the GIC is the interrupt parent of the LIC. Implicitly the top level
interrupt parent feeds into the CPU(s).

> 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.

If the LIC were the interrupt parent, wouldn't the usual nested irq chip
handling would program the GIC as appropriate for a given LIC interrupt?

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

The other way around would make more sense to me.

Thanks,
Mark.

[PATCH 1/2] ARM: tegra: irq: fix buggy usage of irq_data irq field

[Marc Zyngier]

On 27/11/14 12:08, Thierry Reding wrote:
> On Thu, Nov 27, 2014 at 09:08:26AM +0000, Marc Zyngier wrote:
>> Hi Thierry,
>>
>> On 27/11/14 08:28, Thierry Reding wrote:
>>> On Wed, Nov 26, 2014 at 05:55:31PM +0000, Marc Zyngier wrote:
>>>> 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.
>>
>> 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?

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.

Thanks,

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

[PATCH 1/2] ARM: tegra: irq: fix buggy usage of irq_data irq field

[Thierry Reding]

On Thu, Nov 27, 2014 at 02:19:58PM +0000, Marc Zyngier wrote:
> On 27/11/14 12:08, Thierry Reding wrote:
> > On Thu, Nov 27, 2014 at 09:08:26AM +0000, Marc Zyngier wrote:
> >> Hi Thierry,
> >>
> >> On 27/11/14 08:28, Thierry Reding wrote:
> >>> On Wed, Nov 26, 2014 at 05:55:31PM +0000, Marc Zyngier wrote:
> >>>> 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.
> >>
> >> 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?
> 
> 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.

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?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141127/e2960c55/attachment.sig>

[PATCH 1/2] ARM: tegra: irq: fix buggy usage of irq_data irq field

[Marc Zyngier]

On 27/11/14 14:45, Thierry Reding wrote:
> On Thu, Nov 27, 2014 at 02:19:58PM +0000, Marc Zyngier wrote:
>> On 27/11/14 12:08, Thierry Reding wrote:
>>> On Thu, Nov 27, 2014 at 09:08:26AM +0000, Marc Zyngier wrote:
>>>> Hi Thierry,
>>>>
>>>> On 27/11/14 08:28, Thierry Reding wrote:
>>>>> On Wed, Nov 26, 2014 at 05:55:31PM +0000, Marc Zyngier wrote:
>>>>>> 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.
>>>>
>>>> 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?
>>
>> 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.
> 
> 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?

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?

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

[PATCH 1/2] ARM: tegra: irq: fix buggy usage of irq_data irq field

[Thierry Reding]

On Thu, Nov 27, 2014 at 02:50:27PM +0000, Marc Zyngier wrote:
> On 27/11/14 14:45, Thierry Reding wrote:
[...]
> > 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).

Given your description it sounds like suspend will still work, but the
box won't be able to resume. There must be a way to refuse suspend in
those cases. I think that's something we could live with, but not
resuming sounds pretty bad.

> 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
needs to contain a specific flag to make resume work. Would it be
possible to force that flag if we detect that LIC is not the top-level
interrupt parent?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141127/3ee0cb6e/attachment.sig>