| Message ID | 1420652576-22309-5-git-send-email-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jan 07, 2015 at 05:42:39PM +0000, 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.zyngier@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@50000000 { > 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. > + interrupt-controller; > + #interrupt-cells = <3>; > + interrupt-parent = <&intc>; > + }; > + > 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. > diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi [...] > @@ -228,6 +228,7 @@ > timer@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@50043000 { > @@ -250,6 +252,18 @@ > cache-level = <2>; > }; > > + ictlr: interrupt-controller@60004000 { > + compatible = "nvidia,tegra30-ictlr", "nvidia,tegra-ictlr"; Should be "nvidia,tegra30-ictlr" only. > + reg = <0x60004000 64>, > + <0x60004100 64>, > + <0x60004200 64>, > + <0x60004300 64>, > + <0x60004400 64>; > + interrupt-controller; > + #interrupt-cells = <3>; > + interrupt-parent = <&intc>; > + }; This should be the same as for Tegra114. > + > 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. Thierry
On 2015-01-08 10:41, Thierry Reding wrote: > On Wed, Jan 07, 2015 at 05:42:39PM +0000, 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.zyngier@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.
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@50000000 { 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,57000000 { @@ -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@50000000 { 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 100644 --- a/arch/arm/boot/dts/tegra30.dtsi +++ b/arch/arm/boot/dts/tegra30.dtsi @@ -8,7 +8,7 @@ / { compatible = "nvidia,tegra30"; - interrupt-parent = <&intc>; + interrupt-parent = <&ictlr>; pcie-controller@00003000 { compatible = "nvidia,tegra30-pcie"; @@ -228,6 +228,7 @@ timer@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@50043000 { @@ -250,6 +252,18 @@ cache-level = <2>; }; + ictlr: interrupt-controller@60004000 { + compatible = "nvidia,tegra30-ictlr", "nvidia,tegra-ictlr"; + reg = <0x60004000 64>, + <0x60004100 64>, + <0x60004200 64>, + <0x60004300 64>, + <0x60004400 64>; + interrupt-controller; + #interrupt-cells = <3>; + interrupt-parent = <&intc>; + }; + timer@60005000 { compatible = "nvidia,tegra30-timer", "nvidia,tegra20-timer"; reg = <0x60005000 0x400>;
Describe the legacy interrupt controller in every tegra DTSI files, and make it the parent of most interrupts. Signed-off-by: Marc Zyngier <marc.zyngier@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(-)