Re: [PATCH v2 04/21] ARM: tegra: update DTs to expose legacy interrupt controller

2015-01-10 Thread Marc Zyngier 

On 2015-01-08 10:41, Thierry Reding wrote:

On Wed, Jan 07, 2015 at 05:42:39PM +, Marc Zyngier wrote:

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

Signed-off-by: Marc Zyngier marc.zyng...@arm.com
---
 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(-)



I've updated the patch to reflect the requested changes. See below for 
the few contentious points:


[...]


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


This seems to be the expected construct to override a parent interrupt 
controller. Actually, this is already what happens when you have a 
single interrupt-controller and a global interrupt-parent property.


[...]


timer@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?


The per-cpu timers are using PPIs.  As such,. they are not routed 
through the LIC, but are wired to the GIC instead.


[...]


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


The Tegra timer is using SPI (and can probably be used as a wake-up 
source). As such, it is connected to the LIC, not to the GIC.


Per-CPU timers (arch timer for A15 and TWD for A9) are all using PPIs, 
and are directly wired to the GIC.


[...]


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


Which, in my understanding, is the right thing to do. Unless I missed 
something obvious about this?


Thanks,

M.
--
Fast, cheap, reliable. Pick two.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 04/21] ARM: tegra: update DTs to expose legacy interrupt controller

2015-01-08 Thread Thierry Reding 
On Wed, Jan 07, 2015 at 05:42:39PM +, Marc Zyngier wrote:
 Describe the legacy interrupt controller in every tegra DTSI files,
 and make it the parent of most interrupts.
 
 Signed-off-by: Marc Zyngier marc.zyng...@arm.com
 ---
  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(-)
 
 diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
 index 4296b53..f70bed0 100644
 --- a/arch/arm/boot/dts/tegra114.dtsi
 +++ b/arch/arm/boot/dts/tegra114.dtsi
 @@ -8,7 +8,7 @@
  
  / {
   compatible = nvidia,tegra114;
 - interrupt-parent = gic;
 + interrupt-parent = ictlr;

Maybe name the label lic because that's the more common name for this
controller? Same for the other DTSI files.

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

 + };
 +
 + ictlr: interrupt-controller@60004000 {
 + compatible = nvidia,tegra114-ictlr, nvidia,tegra-ictlr;

As previously discussed, I think the second string should be
nvidia,tegra30-ictlr.

 + reg = 0x60004000 64,

I think the first entry should be 256 bytes long since there's another
block of 192 bytes of registers that's part of the interrupt controller,
albeit maybe not related to the functionality of the interrupt chip. But
they're still part of the same hardware block.

 +   0x60004100 64,

According to the TRM there are 4 more registers in this block, so this
should be 80 in size.

 +   0x60004200 64,
 +   0x60004300 64,
 +   0x60004400 64;

I'd prefer all of these to be hexadecimal.

 + interrupt-controller;
 + #interrupt-cells = 3;
 + interrupt-parent = gic;
   };
  
   timer@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?

   };
  };
 diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
[...]
 @@ -190,6 +191,18 @@
   status = disabled;
   };
  
 + ictlr: interrupt-controller@60004000 {
 + compatible = nvidia,tegra124-ictlr, nvidia,tegra-ictlr;

Same as for Tegra114.

 + reg = 0x0 0x60004000 0x0 0x40,
 +   0x0 0x60004100 0x0 0x40,
 +   0x0 0x60004200 0x0 0x40,
 +   0x0 0x60004300 0x0 0x40,
 +   0x0 0x60004400 0x0 0x40;

According to the TRM, entries 1-4 should be 0x100 bytes. Even the first
entry could be 0x100 bytes long since there are additional registers in
there. While they may not be directly relevant to the interrupt chip, I
think it would make sense to include them here because they are part of
the same hardware block.

 diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
 index 8acf5d8..ab2f004 100644
 --- a/arch/arm/boot/dts/tegra20.dtsi
 +++ b/arch/arm/boot/dts/tegra20.dtsi
 @@ -7,7 +7,7 @@
  
  / {
   compatible = nvidia,tegra20;
 - interrupt-parent = intc;
 + interrupt-parent = ictlr;

I wonder if we shouldn't rename the intc label to gic for consistency.
Could be in a follow-up patch though, and something that I can easily do
after this patch set.

  
   host1x@5000 {
   compatible = nvidia,tegra20-host1x, simple-bus;
 @@ -142,6 +142,7 @@
  
   timer@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@50043000 {
 @@ -165,6 +167,17 @@
   cache-level = 2;
   };
  
 + ictlr: interrupt-controller@60004000 {
 + compatible = nvidia,tegra20-ictlr, nvidia,tegra-ictlr;

As discussed previously, I think the second compatible should be
dropped.

 + reg = 0x60004000 64,
 +   0x60004100 64,
 +   0x60004200 64,
 +   0x60004300 64;

Same comments as for Tegra114, except the quinary controller which
doesn't exist on Tegra20.

 +

[PATCH v2 04/21] ARM: tegra: update DTs to expose legacy interrupt controller

2015-01-07 Thread Marc Zyngier 
Describe the legacy interrupt controller in every tegra DTSI files,
and make it the parent of most interrupts.

Signed-off-by: Marc Zyngier marc.zyng...@arm.com
---
 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(-)

diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
index 4296b53..f70bed0 100644
--- a/arch/arm/boot/dts/tegra114.dtsi
+++ b/arch/arm/boot/dts/tegra114.dtsi
@@ -8,7 +8,7 @@
 
 / {
compatible = nvidia,tegra114;
-   interrupt-parent = gic;
+   interrupt-parent = ictlr;
 
host1x@5000 {
compatible = nvidia,tegra114-host1x, simple-bus;
@@ -134,6 +134,19 @@
  0x50046000 0x2000;
interrupts = GIC_PPI 9
(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH);
+   interrupt-parent = gic;
+   };
+
+   ictlr: interrupt-controller@60004000 {
+   compatible = nvidia,tegra114-ictlr, nvidia,tegra-ictlr;
+   reg = 0x60004000 64,
+ 0x60004100 64,
+ 0x60004200 64,
+ 0x60004300 64,
+ 0x60004400 64;
+   interrupt-controller;
+   #interrupt-cells = 3;
+   interrupt-parent = gic;
};
 
timer@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;
};
 };
diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index 4be06c6..6ed6ca0 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -10,7 +10,7 @@
 
 / {
compatible = nvidia,tegra124;
-   interrupt-parent = gic;
+   interrupt-parent = ictlr;
#address-cells = 2;
#size-cells = 2;
 
@@ -173,6 +173,7 @@
  0x0 0x50046000 0x0 0x2000;
interrupts = GIC_PPI 9
(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH);
+   interrupt-parent = gic;
};
 
gpu@0,5700 {
@@ -190,6 +191,18 @@
status = disabled;
};
 
+   ictlr: interrupt-controller@60004000 {
+   compatible = nvidia,tegra124-ictlr, nvidia,tegra-ictlr;
+   reg = 0x0 0x60004000 0x0 0x40,
+ 0x0 0x60004100 0x0 0x40,
+ 0x0 0x60004200 0x0 0x40,
+ 0x0 0x60004300 0x0 0x40,
+ 0x0 0x60004400 0x0 0x40;
+   interrupt-controller;
+   #interrupt-cells = 3;
+   interrupt-parent = gic;
+   };
+
timer@0,60005000 {
compatible = nvidia,tegra124-timer, nvidia,tegra20-timer;
reg = 0x0 0x60005000 0x0 0x400;
@@ -955,5 +968,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;
};
 };
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 8acf5d8..ab2f004 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -7,7 +7,7 @@
 
 / {
compatible = nvidia,tegra20;
-   interrupt-parent = intc;
+   interrupt-parent = ictlr;
 
host1x@5000 {
compatible = nvidia,tegra20-host1x, simple-bus;
@@ -142,6 +142,7 @@
 
timer@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@50043000 {
@@ -165,6 +167,17 @@
cache-level = 2;
};
 
+   ictlr: interrupt-controller@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;
+   };
+
timer@60005000 {
compatible = nvidia,tegra20-timer;
reg = 0x60005000 0x60;
diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index 99475f6..c621e30