[PATCH] powerpc: allow PHBs anywhere in the device tree

[PATCH] powerpc: allow PHBs anywhere in the device tree

[Arnd Bergmann]

The rtas_pci code currently restricts pci host bridges to
locations directly under the device tree root. In order to
correctly model a north bridge that has multiple PCI buses,
that restriction needs to be relaxed.

The new definition is a device node of type "pci" whose
parent is of a different type, so we don't treat pci-to-pci
bridges as host bridges.
It also accepts any device type of "pci", "pcie", "ht" and
"pciex" in order to match anything that is currently in use.

I have added a new helper "of_find_phb_node" to prom.c so
that pci implementations of non-rtas platforms can use this
as well.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Index: linux-2.6/arch/powerpc/kernel/prom.c
===================================================================
--- linux-2.6.orig/arch/powerpc/kernel/prom.c
+++ linux-2.6/arch/powerpc/kernel/prom.c
@@ -1325,6 +1325,72 @@ struct device_node *of_get_next_child(co
 EXPORT_SYMBOL(of_get_next_child);
 
 /**
+ *	of_node_is_pci - Test if a node a pci host
+ *	@node: node to compare
+ *
+ *	returns 1 for PCI hosts and 0 for anything else
+ *
+ *	This function is meant to be called from
+ *	of_find_phb_node as a helper. We compare both
+ *	the compatible and the device_type properties
+ *	to known strings used to indicate PCI hosts.
+ */
+static int of_node_is_pci(struct device_node *node)
+{
+	if (!node->type)
+		return 0;
+
+	if (strcasecmp(node->type, "pci")   == 0 ||
+	    strcasecmp(node->type, "ht")    == 0 ||
+	    strcasecmp(node->type, "pciex") == 0 ||
+	    strcasecmp(node->type, "pcie")  == 0)
+		return 1;
+
+	if (device_is_compatible(node, "pci")	||
+	    device_is_compatible(node, "ht")	||
+	    device_is_compatible(node, "pciex") ||
+	    device_is_compatible(node, "pcie"))
+		return 1;
+
+	return 0;
+}
+
+/**
+ *
+ *	of_find_phb_node - Iterate all PCI host bridge device nodes
+ *
+ *	@from:	The node to start searching from or NULL, the node
+ *		you pass will not be searched, only the next one
+ *		will; typically, you pass what the previous call
+ *		returned. of_node_put() will be called on it
+ *
+ *	Returns a node pointer with refcount incremented, use
+ *	of_node_put() on it when done.
+ *
+ *	since we only want to return host bridges, not pci-pci
+ *	bridges, check if the parent is not also a pci host.
+ */
+struct device_node *of_find_phb_node(struct device_node *from)
+{
+	struct device_node *np;
+
+	read_lock(&devtree_lock);
+	np = from ? from->allnext : allnodes;
+	for (; np; np = np->allnext) {
+		if (of_node_is_pci(np) &&
+		    np->parent &&
+		    !of_node_is_pci(np->parent))
+			break;
+	}
+	if (np)
+		of_node_get(np);
+	if (from)
+		of_node_put(from);
+	read_unlock(&devtree_lock);
+	return np;
+}
+
+/**
  *	of_node_get - Increment refcount of a node
  *	@node:	Node to inc refcount, NULL is supported to
  *		simplify writing of callers
Index: linux-2.6/arch/powerpc/kernel/rtas_pci.c
===================================================================
--- linux-2.6.orig/arch/powerpc/kernel/rtas_pci.c
+++ linux-2.6/arch/powerpc/kernel/rtas_pci.c
@@ -240,13 +240,6 @@ unsigned long __devinit get_phb_buid (st
 
 	if (ibm_read_pci_config == -1) return 0;
 
-	/* PHB's will always be children of the root node,
-	 * or so it is promised by the current firmware. */
-	if (phb->parent == NULL)
-		return 0;
-	if (phb->parent->parent)
-		return 0;
-
 	buid_vals = (unsigned int *) get_property(phb, "reg", &len);
 	if (buid_vals == NULL)
 		return 0;
@@ -297,17 +290,10 @@ unsigned long __init find_and_init_phbs(
 	struct device_node *node;
 	struct pci_controller *phb;
 	unsigned int index;
-	struct device_node *root = of_find_node_by_path("/");
 
+	node = NULL;
 	index = 0;
-	for (node = of_get_next_child(root, NULL);
-	     node != NULL;
-	     node = of_get_next_child(root, node)) {
-
-		if (node->type == NULL || (strcmp(node->type, "pci") != 0 &&
-					   strcmp(node->type, "pciex") != 0))
-			continue;
-
+	while ((node = of_find_phb_node(node))) {
 		phb = pcibios_alloc_controller(node);
 		if (!phb)
 			continue;
@@ -316,8 +302,6 @@ unsigned long __init find_and_init_phbs(
 		pci_setup_phb_io(phb, index == 0);
 		index++;
 	}
-
-	of_node_put(root);
 	pci_devs_phb_init();
 
 	/*
Index: linux-2.6/include/asm-powerpc/prom.h
===================================================================
--- linux-2.6.orig/include/asm-powerpc/prom.h
+++ linux-2.6/include/asm-powerpc/prom.h
@@ -131,6 +131,7 @@ extern struct device_node *of_find_compa
 extern struct device_node *of_find_node_by_path(const char *path);
 extern struct device_node *of_find_node_by_phandle(phandle handle);
 extern struct device_node *of_find_all_nodes(struct device_node *prev);
+extern struct device_node *of_find_phb_node(struct device_node *from);
 extern struct device_node *of_get_parent(const struct device_node *node);
 extern struct device_node *of_get_next_child(const struct device_node *node,
 					     struct device_node *prev);

Re: [PATCH] powerpc: allow PHBs anywhere in the device tree

[Olof Johansson]

Hi,

Looks good. Two very minor nitpicks below.


-Olof

On Tue, 12 Sep 2006 19:52:04 +0200 Arnd Bergmann <arnd.bergmann@de.ibm.com> wrote:

> The rtas_pci code currently restricts pci host bridges to
> locations directly under the device tree root. In order to
> correctly model a north bridge that has multiple PCI buses,
> that restriction needs to be relaxed.
> 
> The new definition is a device node of type "pci" whose
> parent is of a different type, so we don't treat pci-to-pci
> bridges as host bridges.
> It also accepts any device type of "pci", "pcie", "ht" and
> "pciex" in order to match anything that is currently in use.
> 
> I have added a new helper "of_find_phb_node" to prom.c so
> that pci implementations of non-rtas platforms can use this
> as well.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> Index: linux-2.6/arch/powerpc/kernel/prom.c
> ===================================================================
> --- linux-2.6.orig/arch/powerpc/kernel/prom.c
> +++ linux-2.6/arch/powerpc/kernel/prom.c
> @@ -1325,6 +1325,72 @@ struct device_node *of_get_next_child(co
>  EXPORT_SYMBOL(of_get_next_child);
>  
>  /**
> + *	of_node_is_pci - Test if a node a pci host
> + *	@node: node to compare
> + *
> + *	returns 1 for PCI hosts and 0 for anything else
> + *
> + *	This function is meant to be called from
> + *	of_find_phb_node as a helper. We compare both
> + *	the compatible and the device_type properties
> + *	to known strings used to indicate PCI hosts.
> + */
> +static int of_node_is_pci(struct device_node *node)
> +{
> +	if (!node->type)
> +		return 0;
> +
> +	if (strcasecmp(node->type, "pci")   == 0 ||
> +	    strcasecmp(node->type, "ht")    == 0 ||
> +	    strcasecmp(node->type, "pciex") == 0 ||
> +	    strcasecmp(node->type, "pcie")  == 0)

!strcasecmp(...) instead?
Do they ever exist in non-lowercase versions? Old code just did
strcmp().

> +		return 1;
> +
> +	if (device_is_compatible(node, "pci")	||
> +	    device_is_compatible(node, "ht")	||
> +	    device_is_compatible(node, "pciex") ||
> +	    device_is_compatible(node, "pcie"))
> +		return 1;
> +
> +	return 0;
> +}
> +
> +/**
> + *
> + *	of_find_phb_node - Iterate all PCI host bridge device nodes
> + *
> + *	@from:	The node to start searching from or NULL, the node
> + *		you pass will not be searched, only the next one
> + *		will; typically, you pass what the previous call
> + *		returned. of_node_put() will be called on it

Convention seems to be to call this "prev", not "from"?

> + *
> + *	Returns a node pointer with refcount incremented, use
> + *	of_node_put() on it when done.
> + *
> + *	since we only want to return host bridges, not pci-pci
> + *	bridges, check if the parent is not also a pci host.
> + */
> +struct device_node *of_find_phb_node(struct device_node *from)
> +{
> +	struct device_node *np;
> +
> +	read_lock(&devtree_lock);
> +	np = from ? from->allnext : allnodes;
> +	for (; np; np = np->allnext) {
> +		if (of_node_is_pci(np) &&
> +		    np->parent &&
> +		    !of_node_is_pci(np->parent))
> +			break;
> +	}
> +	if (np)
> +		of_node_get(np);
> +	if (from)
> +		of_node_put(from);
> +	read_unlock(&devtree_lock);
> +	return np;
> +}
> +
> +/**
>   *	of_node_get - Increment refcount of a node
>   *	@node:	Node to inc refcount, NULL is supported to
>   *		simplify writing of callers
> Index: linux-2.6/arch/powerpc/kernel/rtas_pci.c
> ===================================================================
> --- linux-2.6.orig/arch/powerpc/kernel/rtas_pci.c
> +++ linux-2.6/arch/powerpc/kernel/rtas_pci.c
> @@ -240,13 +240,6 @@ unsigned long __devinit get_phb_buid (st
>  
>  	if (ibm_read_pci_config == -1) return 0;
>  
> -	/* PHB's will always be children of the root node,
> -	 * or so it is promised by the current firmware. */
> -	if (phb->parent == NULL)
> -		return 0;
> -	if (phb->parent->parent)
> -		return 0;
> -
>  	buid_vals = (unsigned int *) get_property(phb, "reg", &len);
>  	if (buid_vals == NULL)
>  		return 0;
> @@ -297,17 +290,10 @@ unsigned long __init find_and_init_phbs(
>  	struct device_node *node;
>  	struct pci_controller *phb;
>  	unsigned int index;
> -	struct device_node *root = of_find_node_by_path("/");
>  
> +	node = NULL;
>  	index = 0;
> -	for (node = of_get_next_child(root, NULL);
> -	     node != NULL;
> -	     node = of_get_next_child(root, node)) {
> -
> -		if (node->type == NULL || (strcmp(node->type, "pci") != 0 &&
> -					   strcmp(node->type, "pciex") != 0))
> -			continue;
> -
> +	while ((node = of_find_phb_node(node))) {
>  		phb = pcibios_alloc_controller(node);
>  		if (!phb)
>  			continue;
> @@ -316,8 +302,6 @@ unsigned long __init find_and_init_phbs(
>  		pci_setup_phb_io(phb, index == 0);
>  		index++;
>  	}
> -
> -	of_node_put(root);
>  	pci_devs_phb_init();
>  
>  	/*
> Index: linux-2.6/include/asm-powerpc/prom.h
> ===================================================================
> --- linux-2.6.orig/include/asm-powerpc/prom.h
> +++ linux-2.6/include/asm-powerpc/prom.h
> @@ -131,6 +131,7 @@ extern struct device_node *of_find_compa
>  extern struct device_node *of_find_node_by_path(const char *path);
>  extern struct device_node *of_find_node_by_phandle(phandle handle);
>  extern struct device_node *of_find_all_nodes(struct device_node *prev);
> +extern struct device_node *of_find_phb_node(struct device_node *from);
>  extern struct device_node *of_get_parent(const struct device_node *node);
>  extern struct device_node *of_get_next_child(const struct device_node *node,
>  					     struct device_node *prev);
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [Cbe-oss-dev] [PATCH] powerpc: allow PHBs anywhere in the device tree

[Arnd Bergmann]

On Tuesday 12 September 2006 21:54, Olof Johansson wrote:
> !strcasecmp(...) instead?

I do that normally for functions that return either a truth value
or a pointer to an object that may be NULL, whereas strcasecmp
returns a numerical value that I'm checking for a specific result.
This is also consistant with how it is used elsewhere in the file.

> Do they ever exist in non-lowercase versions? Old code just did
> strcmp().

I took that from device_is_compatible(), which also does
strncasecmp, so I assumed that was done for a reason.

> > + *=A0=A0=A0@from:=A0=A0The node to start searching from or NULL, the n=
ode
> > + *=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0you pass will not be searched, only=
 the next one
> > + *=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0will; typically, you pass what the =
previous call
> > + *=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0returned. of_node_put() will be cal=
led on it
>=20
> Convention seems to be to call this "prev", not "from"?

ok

	Arnd <><

Re: [Cbe-oss-dev] [PATCH] powerpc: allow PHBs anywhere in the device tree

[Olof Johansson]

On Tue, 12 Sep 2006 23:16:21 +0200 Arnd Bergmann <arnd@arndb.de> wrote:

> On Tuesday 12 September 2006 21:54, Olof Johansson wrote:
> > !strcasecmp(...) instead?
> 
> I do that normally for functions that return either a truth value
> or a pointer to an object that may be NULL, whereas strcasecmp
> returns a numerical value that I'm checking for a specific result.
> This is also consistant with how it is used elsewhere in the file.

Ok.

> > Do they ever exist in non-lowercase versions? Old code just did
> > strcmp().
> 
> I took that from device_is_compatible(), which also does
> strncasecmp, so I assumed that was done for a reason.

I think it might be because compatible is by tradition less specific. I
was also mostly looking at the patch, not the rest of the file. Either
is fine with me though, I just wanted to bring it up.



-Olof

Re: [PATCH] powerpc: allow PHBs anywhere in the device tree

[Paul Mackerras]

Arnd Bergmann writes:

> The rtas_pci code currently restricts pci host bridges to
> locations directly under the device tree root. In order to
> correctly model a north bridge that has multiple PCI buses,
> that restriction needs to be relaxed.

Ummm, how do you think we've managed on pSeries all this time?

I could understand this if you said you needed to represent multiple
north bridges, though that would be a rather peculiar system
topology.  Having multiple PCI domains hanging off a single north
bridge can be represented perfectly well with the host bridges being
children of the root, because the root node represents the address
space directly accessible to the processor(s), and it is the north
bridge that implements that address space.

What specifically do you want this for?

Paul.

Re: [Cbe-oss-dev] [PATCH] powerpc: allow PHBs anywhere in the device tree

[Arnd Bergmann]

On Wednesday 13 September 2006 00:20, Paul Mackerras wrote:
> Ummm, how do you think we've managed on pSeries all this time?
>=20
> I could understand this if you said you needed to represent multiple
> north bridges, though that would be a rather peculiar system
> topology. =A0Having multiple PCI domains hanging off a single north
> bridge can be represented perfectly well with the host bridges being
> children of the root, because the root node represents the address
> space directly accessible to the processor(s), and it is the north
> bridge that implements that address space.
>=20
> What specifically do you want this for?

=46or the cell blade, we have two bridge chips that are directly
connected to one of the CPUs each and are in separate address spaces.
Besides the PCI host bridges on them (between 1 and 3 per chip,
depending on the model), there are other devices on each bridge chip
that I would like to represent there as well. To make things
worse, they are behind logical bridges on the chip itself, something
like

/bridge@1/interrupt-controller
         /plb5/pcie
              /plb4/pci
                   /ethernet
                   /serial
/bridge@2/plb5/pcie
              /plb4/pci

each of axon, plb5, plb4 and the pci buses has their own ranges
property to map addresses.
While we could probably put all the phbs at the root, i'd much
prefer having the real topology reflected in the device tree.

	Arnd <><

Re: [PATCH] powerpc: allow PHBs anywhere in the device tree

[Segher Boessenkool]

>> The rtas_pci code currently restricts pci host bridges to
>> locations directly under the device tree root. In order to
>> correctly model a north bridge that has multiple PCI buses,
>> that restriction needs to be relaxed.
>
> Ummm, how do you think we've managed on pSeries all this time?
>
> I could understand this if you said you needed to represent multiple
> north bridges, though that would be a rather peculiar system
> topology.

Yes, although it actually already exists.

> Having multiple PCI domains hanging off a single north
> bridge can be represented perfectly well with the host bridges being
> children of the root, because the root node represents the address
> space directly accessible to the processor(s), and it is the north
> bridge that implements that address space.

There are some cases where you *really* want to model the PCI bridges'
parents in the OF tree as well.

And even if it wouldn't be sane -- why would Linux refuse an OF device
tree that makes perfect sense an sich?  If some 970 OF would put the
HyperTransport node under the U4 node, should that really be a problem
for Linux?

> What specifically do you want this for?

Not on a public list ;-)


Segher

Re: [Cbe-oss-dev] [PATCH] powerpc: allow PHBs anywhere in the device tree

[Benjamin Herrenschmidt]

> For the cell blade, we have two bridge chips that are directly
> connected to one of the CPUs each and are in separate address spaces.
> Besides the PCI host bridges on them (between 1 and 3 per chip,
> depending on the model), there are other devices on each bridge chip
> that I would like to represent there as well. To make things
> worse, they are behind logical bridges on the chip itself, something
> like
> 
> /bridge@1/interrupt-controller
>          /plb5/pcie
>               /plb4/pci
>                    /ethernet
>                    /serial
> /bridge@2/plb5/pcie
>               /plb4/pci
> 
> each of axon, plb5, plb4 and the pci buses has their own ranges
> property to map addresses.
> While we could probably put all the phbs at the root, i'd much
> prefer having the real topology reflected in the device tree.

There's one more thing I think your patch isn't fixing and that will
need fixing, is pci_process_OF_bridge_ranges() which currently parses
the PHB's "ranges" property assuming that the addresses it gets for the
"parent" bus are system bus physical addresses. It needs instead to get
those translated all the way up the tree (which isn't hard btw).

As soon as I'm over this TG3 data corruption problem, I'll finally
finish setting up SIMICS here and will look into making that stuff work.

Ben.