Patchwork Fix PCI in Holly device tree

login
register
mail settings
Submitter David Gibson
Date Sept. 25, 2008, 2:39 a.m.
Message ID <20080925023904.GC15169@yookeroo.seuss>
Download mbox | patch
Permalink /patch/1426/
State Accepted
Commit ad611045ce5d059af84a9855b22ca3f7a99d47be
Delegated to: Benjamin Herrenschmidt
Headers show

Comments

David Gibson - Sept. 25, 2008, 2:39 a.m.
The PCI bridge on the Holly board is current incorrectly represented
in the device tree.  The current device tree node for the PCI bridge
sits under the tsi-bridge node.  That's not obviously wrong, but the
PCI bridge translated some PCI spaces into CPU address ranges which
were not translated by the tsi-bridge node.

We used to get away with this problem because the PCI bridge discovery
code was also buggy, assuming incorrectly that PCI host bridge nodes
were always directly under the root bus and treating the translated
addresses as raw CPU addresses, rather than parent bus addresses.
This has since been fixed, breaking Holly.

This could be fixed by adding extra translations to the tsi-bridge
node, but this patch instead moves the Holly PCI bridge out of the
tsi-bridge node to the root bus.  This makes the tsi-bridge node
represent only the built-in IO devices in the bridge, with a
more-or-less contiguous address range.  This is the same convention
used on Freescale SoC chips, where the "soc" node represents only the
IMMR region, and the PCI and other bus bridges are separate nodes
under the root bus.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Josh Boyer - Sept. 25, 2008, 12:11 p.m.
On Thu, 25 Sep 2008 12:39:04 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> The PCI bridge on the Holly board is current incorrectly represented
> in the device tree.  The current device tree node for the PCI bridge
> sits under the tsi-bridge node.  That's not obviously wrong, but the
> PCI bridge translated some PCI spaces into CPU address ranges which
> were not translated by the tsi-bridge node.
> 
> We used to get away with this problem because the PCI bridge discovery
> code was also buggy, assuming incorrectly that PCI host bridge nodes
> were always directly under the root bus and treating the translated
> addresses as raw CPU addresses, rather than parent bus addresses.
> This has since been fixed, breaking Holly.
> 
> This could be fixed by adding extra translations to the tsi-bridge
> node, but this patch instead moves the Holly PCI bridge out of the
> tsi-bridge node to the root bus.  This makes the tsi-bridge node
> represent only the built-in IO devices in the bridge, with a
> more-or-less contiguous address range.  This is the same convention
> used on Freescale SoC chips, where the "soc" node represents only the
> IMMR region, and the PCI and other bus bridges are separate nodes
> under the root bus.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Acked-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>

Paul, I can include this in my 'next' branch if you aren't opposed.
I'll have another set of patches going in there today/tomorrow.

josh
Josh Boyer - Sept. 25, 2008, 12:22 p.m.
On Thu, Sep 25, 2008 at 08:11:29AM -0400, Josh Boyer wrote:
>On Thu, 25 Sep 2008 12:39:04 +1000
>David Gibson <david@gibson.dropbear.id.au> wrote:
>
>> The PCI bridge on the Holly board is current incorrectly represented
>> in the device tree.  The current device tree node for the PCI bridge
>> sits under the tsi-bridge node.  That's not obviously wrong, but the
>> PCI bridge translated some PCI spaces into CPU address ranges which
>> were not translated by the tsi-bridge node.
>> 
>> We used to get away with this problem because the PCI bridge discovery
>> code was also buggy, assuming incorrectly that PCI host bridge nodes
>> were always directly under the root bus and treating the translated
>> addresses as raw CPU addresses, rather than parent bus addresses.
>> This has since been fixed, breaking Holly.
>> 
>> This could be fixed by adding extra translations to the tsi-bridge
>> node, but this patch instead moves the Holly PCI bridge out of the
>> tsi-bridge node to the root bus.  This makes the tsi-bridge node
>> represent only the built-in IO devices in the bridge, with a
>> more-or-less contiguous address range.  This is the same convention
>> used on Freescale SoC chips, where the "soc" node represents only the
>> IMMR region, and the PCI and other bus bridges are separate nodes
>> under the root bus.
>> 
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>
>Acked-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>
>
>Paul, I can include this in my 'next' branch if you aren't opposed.
>I'll have another set of patches going in there today/tomorrow.

Er... on second thought, this actually fixes a regression on Holly.  So
I'll amend my offer to put it in my 'next' branch to be contingent on you
not wanting to get it into 2.6.27 this late.

josh
Benjamin Herrenschmidt - Sept. 29, 2008, 8:53 a.m.
> >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >
> >Acked-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>
> >
> >Paul, I can include this in my 'next' branch if you aren't opposed.
> >I'll have another set of patches going in there today/tomorrow.
> 
> Er... on second thought, this actually fixes a regression on Holly.  So
> I'll amend my offer to put it in my 'next' branch to be contingent on you
> not wanting to get it into 2.6.27 this late.

I've picked it up, I need to check a thing or two and I'll send patches
for .27 to Linus tomorrow (Paulus is on vacation for a week). With a bit
of luck he won't have closed .27 before that :-)

Cheers
Ben.

Patch

Index: working-2.6/arch/powerpc/boot/dts/holly.dts
===================================================================
--- working-2.6.orig/arch/powerpc/boot/dts/holly.dts	2008-09-02 11:50:12.000000000 +1000
+++ working-2.6/arch/powerpc/boot/dts/holly.dts	2008-09-17 11:50:23.000000000 +1000
@@ -133,61 +133,61 @@ 
 			reg = <0x00007400 0x00000400>;
 			big-endian;
 		};
+	};
 
-		pci@1000 {
-			device_type = "pci";
-			compatible = "tsi109-pci", "tsi108-pci";
-			#interrupt-cells = <1>;
-			#size-cells = <2>;
-			#address-cells = <3>;
-			reg = <0x00001000 0x00001000>;
-			bus-range = <0x0 0x0>;
-			/*----------------------------------------------------+
-			| PCI memory range.
-			| 01 denotes I/O space
-			| 02 denotes 32-bit memory space
-			+----------------------------------------------------*/
-			ranges = <0x02000000 0x00000000 0x40000000 0x40000000 0x00000000 0x10000000
-				  0x01000000 0x00000000 0x00000000 0x7e000000 0x00000000 0x00010000>;
-			clock-frequency = <133333332>;
-			interrupt-parent = <&MPIC>;
+	pci@c0001000 {
+		device_type = "pci";
+		compatible = "tsi109-pci", "tsi108-pci";
+		#interrupt-cells = <1>;
+		#size-cells = <2>;
+		#address-cells = <3>;
+		reg = <0xc0001000 0x00001000>;
+		bus-range = <0x0 0x0>;
+		/*----------------------------------------------------+
+		| PCI memory range.
+		| 01 denotes I/O space
+		| 02 denotes 32-bit memory space
+		+----------------------------------------------------*/
+		ranges = <0x02000000 0x00000000 0x40000000 0x40000000 0x00000000 0x10000000
+			  0x01000000 0x00000000 0x00000000 0x7e000000 0x00000000 0x00010000>;
+		clock-frequency = <133333332>;
+		interrupt-parent = <&MPIC>;
+		interrupts = <0x17 0x2>;
+		interrupt-map-mask = <0xf800 0x0 0x0 0x7>;
+		/*----------------------------------------------------+
+		| The INTA, INTB, INTC, INTD are shared.
+		+----------------------------------------------------*/
+		interrupt-map = <
+			0x800 0x0 0x0 0x1 &RT0 0x24 0x0
+			0x800 0x0 0x0 0x2 &RT0 0x25 0x0
+			0x800 0x0 0x0 0x3 &RT0 0x26 0x0
+			0x800 0x0 0x0 0x4 &RT0 0x27 0x0
+
+			0x1000 0x0 0x0 0x1 &RT0 0x25 0x0
+			0x1000 0x0 0x0 0x2 &RT0 0x26 0x0
+			0x1000 0x0 0x0 0x3 &RT0 0x27 0x0
+			0x1000 0x0 0x0 0x4 &RT0 0x24 0x0
+
+			0x1800 0x0 0x0 0x1 &RT0 0x26 0x0
+			0x1800 0x0 0x0 0x2 &RT0 0x27 0x0
+			0x1800 0x0 0x0 0x3 &RT0 0x24 0x0
+			0x1800 0x0 0x0 0x4 &RT0 0x25 0x0
+
+			0x2000 0x0 0x0 0x1 &RT0 0x27 0x0
+			0x2000 0x0 0x0 0x2 &RT0 0x24 0x0
+			0x2000 0x0 0x0 0x3 &RT0 0x25 0x0
+			0x2000 0x0 0x0 0x4 &RT0 0x26 0x0
+			>;
+
+		RT0: router@1180 {
+			device_type = "pic-router";
+			interrupt-controller;
+			big-endian;
+			clock-frequency = <0>;
+			#address-cells = <0>;
+			#interrupt-cells = <2>;
 			interrupts = <0x17 0x2>;
-			interrupt-map-mask = <0xf800 0x0 0x0 0x7>;
-			/*----------------------------------------------------+
-			| The INTA, INTB, INTC, INTD are shared.
-			+----------------------------------------------------*/
-			interrupt-map = <
-				0x800 0x0 0x0 0x1 &RT0 0x24 0x0
-				0x800 0x0 0x0 0x2 &RT0 0x25 0x0
-				0x800 0x0 0x0 0x3 &RT0 0x26 0x0
-				0x800 0x0 0x0 0x4 &RT0 0x27 0x0
-
-				0x1000 0x0 0x0 0x1 &RT0 0x25 0x0
-				0x1000 0x0 0x0 0x2 &RT0 0x26 0x0
-				0x1000 0x0 0x0 0x3 &RT0 0x27 0x0
-				0x1000 0x0 0x0 0x4 &RT0 0x24 0x0
-
-				0x1800 0x0 0x0 0x1 &RT0 0x26 0x0
-				0x1800 0x0 0x0 0x2 &RT0 0x27 0x0
-				0x1800 0x0 0x0 0x3 &RT0 0x24 0x0
-				0x1800 0x0 0x0 0x4 &RT0 0x25 0x0
-
-				0x2000 0x0 0x0 0x1 &RT0 0x27 0x0
-				0x2000 0x0 0x0 0x2 &RT0 0x24 0x0
-				0x2000 0x0 0x0 0x3 &RT0 0x25 0x0
-				0x2000 0x0 0x0 0x4 &RT0 0x26 0x0
-				>;
-
-			RT0: router@1180 {
- 				device_type = "pic-router";
- 				interrupt-controller;
- 				big-endian;
- 				clock-frequency = <0>;
- 				#address-cells = <0>;
- 				#interrupt-cells = <2>;
- 				interrupts = <0x17 0x2>;
-				interrupt-parent = <&MPIC>;
-			};
+			interrupt-parent = <&MPIC>;
 		};
 	};