[PATCH] powerpc: Fix irq routing on some PowerMac 32 bits

[PATCH] powerpc: Fix irq routing on some PowerMac 32 bits

[Benjamin Herrenschmidt]

The changes to use pci_read_irq_line() broke interrupt parsing
on some 32 bits powermacs (oops). The reason is a bit obscure.
The code to parse interrupts happens earlier now, during
pcibios_fixup() as the PCI bus is being probed. However, the
current implementation pci_device_to_OF_node() for 32 bits
powerpc relies, on machines like PowerMac who renumber PCI busses,
on a table called pci_OF_bus_map containing a map of bus numbers
between the kernel and the firmware which is setup only later.
Thus, it fails to match the device node. In addition, some of
Apple internal PCI devices lack a proper PCI_INTERRUPT_PIN, thus
preventing the fallback mapping code to work.

This patch fixes it by making pci_device_to_OF_node() 32 bits
implementation use a different algorithm that works without
using the pci_OF_bus_map thing (which I intend to deprecate
anyway). It's a bit slower but that function isn't called in
any hot path hopefully.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Index: linux-work/arch/powerpc/kernel/pci_32.c
===================================================================
--- linux-work.orig/arch/powerpc/kernel/pci_32.c	2006-12-11 13:27:58.000000000 +1100
+++ linux-work/arch/powerpc/kernel/pci_32.c	2006-12-11 13:54:26.000000000 +1100
@@ -736,25 +736,51 @@ scan_OF_pci_childs(struct device_node* n
 	return NULL;
 }
 
-static int
-scan_OF_pci_childs_iterator(struct device_node* node, void* data)
+static struct device_node *scan_OF_for_pci_dev(struct device_node *parent,
+					       unsigned int devfn)
 {
-	const unsigned int *reg;
-	u8* fdata = (u8*)data;
-	
-	reg = get_property(node, "reg", NULL);
-	if (reg && ((reg[0] >> 8) & 0xff) == fdata[1]
-		&& ((reg[0] >> 16) & 0xff) == fdata[0])
-		return 1;
-	return 0;
+	struct device_node *np = NULL;
+	const u32 *reg;
+	unsigned int psize;
+
+	while ((np = of_get_next_child(parent, np)) != NULL) {
+		reg = get_property(np, "reg", &psize);
+		if (reg == NULL || psize < 4)
+			continue;
+		if (((reg[0] >> 8) & 0xff) == devfn)
+			return np;
+	}
+	return NULL;
 }
 
-static struct device_node*
-scan_OF_childs_for_device(struct device_node* node, u8 bus, u8 dev_fn)
+
+static struct device_node *scan_OF_for_pci_bus(struct pci_bus *bus)
 {
-	u8 filter_data[2] = {bus, dev_fn};
+	struct device_node *parent, *np;
+
+	/* Are we a root bus ? */
+	if (bus->self == NULL || bus->parent == NULL) {
+		struct pci_controller *hose = pci_bus_to_hose(bus->number);
+		if (hose == NULL)
+			return NULL;
+		return of_node_get(hose->arch_data);
+	}
+
+	/* not a root bus, we need to get our parent */
+	parent = scan_OF_for_pci_bus(bus->parent);
+	if (parent == NULL)
+		return NULL;
+
+	/* now iterate for children for a match */
+	np = scan_OF_for_pci_dev(parent, bus->self->devfn);
+	of_node_put(parent);
+
+	/* sanity check */
+	if (strcmp(np->type, "pci") != 0)
+		printk(KERN_WARNING "pci: wrong type \"%s\" for bridge %s\n",
+		       np->type, np->full_name);
 
-	return scan_OF_pci_childs(node, scan_OF_pci_childs_iterator, filter_data);
+	return np;
 }
 
 /*
@@ -763,43 +789,25 @@ scan_OF_childs_for_device(struct device_
 struct device_node *
 pci_busdev_to_OF_node(struct pci_bus *bus, int devfn)
 {
-	struct pci_controller *hose;
-	struct device_node *node;
-	int busnr;
+	struct device_node *parent, *np;
 
 	if (!have_of)
 		return NULL;
-	
-	/* Lookup the hose */
-	busnr = bus->number;
-	hose = pci_bus_to_hose(busnr);
-	if (!hose)
-		return NULL;
 
-	/* Check it has an OF node associated */
-	node = (struct device_node *) hose->arch_data;
-	if (!node)
+	DBG("pci_busdev_to_OF_node(%d,0x%x)\n", bus->number, devfn);
+	parent = scan_OF_for_pci_bus(bus);
+	if (parent == NULL)
 		return NULL;
-
-	/* Fixup bus number according to what OF think it is. */
-#ifdef CONFIG_PPC_PMAC
-	/* The G5 need a special case here. Basically, we don't remap all
-	 * busses on it so we don't create the pci-OF-map. However, we do
-	 * remap the AGP bus and so have to deal with it. A future better
-	 * fix has to be done by making the remapping per-host and always
-	 * filling the pci_to_OF map. --BenH
+	DBG(" parent is %s\n", parent ? parent->full_name : "<NULL>");
+	np = scan_OF_for_pci_dev(parent, devfn);
+	of_node_put(parent);
+	DBG(" result is %s\n", np ? np->full_name : "<NULL>");
+
+	/* XXX most callers don't release the returned node
+	 * mostly because ppc64 doesn't increase the refcount,
+	 * we need to fix that.
 	 */
-	if (machine_is(powermac) && busnr >= 0xf0)
-		busnr -= 0xf0;
-	else
-#endif
-	if (pci_to_OF_bus_map)
-		busnr = pci_to_OF_bus_map[busnr];
-	if (busnr == 0xff)
-		return NULL;
-	
-	/* Now, lookup childs of the hose */
-	return scan_OF_childs_for_device(node->child, busnr, devfn);
+	return np;
 }
 EXPORT_SYMBOL(pci_busdev_to_OF_node);
 

Re: [PATCH] powerpc: Fix irq routing on some PowerMac 32 bits

[Paul Collins]

This patch makes my PowerBook work again, which gladdens me.

-- 
Paul Collins
Wellington, New Zealand

Dag vijandelijk luchtschip de huismeester is dood

Re: [PATCH] powerpc: Fix irq routing on some PowerMac 32 bits

[Benjamin Herrenschmidt]

On Mon, 2006-12-11 at 19:22 +1300, Paul Collins wrote:

> This patch makes my PowerBook work again, which gladdens me.

Yup, mine too :-)

Ben.

Re: [PATCH] powerpc: Fix irq routing on some PowerMac 32 bits

[Zang Roy-r61911]

Ben,
	After apply your patch, my 8548CDS board blocked when booting up.
	Should I adjust my flat device tree or bug introduced by this patch?
	Thanks.
Roy


On Mon, 2006-12-11 at 11:09, Benjamin Herrenschmidt wrote:
> The changes to use pci_read_irq_line() broke interrupt parsing
> on some 32 bits powermacs (oops). The reason is a bit obscure.
> The code to parse interrupts happens earlier now, during
> pcibios_fixup() as the PCI bus is being probed. However, the
> current implementation pci_device_to_OF_node() for 32 bits
> powerpc relies, on machines like PowerMac who renumber PCI busses,
> on a table called pci_OF_bus_map containing a map of bus numbers
> between the kernel and the firmware which is setup only later.
> Thus, it fails to match the device node. In addition, some of
> Apple internal PCI devices lack a proper PCI_INTERRUPT_PIN, thus
> preventing the fallback mapping code to work.
> 
> This patch fixes it by making pci_device_to_OF_node() 32 bits
> implementation use a different algorithm that works without
> using the pci_OF_bus_map thing (which I intend to deprecate
> anyway). It's a bit slower but that function isn't called in
> any hot path hopefully.
> 

Re: [PATCH] powerpc: Fix irq routing on some PowerMac 32 bits

[Benjamin Herrenschmidt]

On Wed, 2007-01-10 at 11:00 +0800, Zang Roy-r61911 wrote:
> Ben,
> 	After apply your patch, my 8548CDS board blocked when booting up.
> 	Should I adjust my flat device tree or bug introduced by this patch?
> 	Thanks.
> Roy

Well, it would be useful to track down what's wrong... I suspect it
might be triggering a problem with your device-tree but it's difficult
to say at this point.

Ben.

Re: [PATCH] powerpc: Fix irq routing on some PowerMac 32 bits

[Zang Roy-r61911]

On Wed, 2007-01-10 at 11:16, Benjamin Herrenschmidt wrote:
> On Wed, 2007-01-10 at 11:00 +0800, Zang Roy-r61911 wrote:
> > Ben,
> >       After apply your patch, my 8548CDS board blocked when booting
> up.
> >       Should I adjust my flat device tree or bug introduced by this
> patch?
> >       Thanks.
> > Roy
> 
> Well, it would be useful to track down what's wrong... I suspect it
> might be triggering a problem with your device-tree 

I suppose so!
> but it's difficult
> to say at this point.
> 
I open the "DEBUG" option for pci_32.c, mpic.c and prom.c. The flowing
is the log:
======================================================================
## Booting image at 01000000 ...
   Image Name:   Linux-2.6.20-rc1-g4c2a3b24-dirty
   Image Type:   PowerPC Linux Kernel Image (gzip compressed)
   Data Size:    1375935 Bytes =  1.3 MB
   Load Address: 00000000
   Entry Point:  00000000
   Verifying Checksum ... OK
   Uncompressing Kernel Image ... OK
   Booting using flat device tree at 0x400000
 -> early_init_devtree()
search "chosen", depth: 0, uname:
search "chosen", depth: 1, uname: cpus
search "chosen", depth: 2, uname: PowerPC,8548@0
search "chosen", depth: 1, uname: memory
search "chosen", depth: 1, uname: soc8548@e0000000
search "chosen", depth: 2, uname: i2c@3000
search "chosen", depth: 2, uname: mdio@24520
search "chosen", depth: 3, uname: ethernet-phy@0
search "chosen", depth: 3, uname: ethernet-phy@1
search "chosen", depth: 3, uname: ethernet-phy@2
search "chosen", depth: 3, uname: ethernet-phy@3
search "chosen", depth: 2, uname: ethernet@24000
search "chosen", depth: 2, uname: ethernet@25000
search "chosen", depth: 2, uname: ethernet@26000
search "chosen", depth: 2, uname: serial@4500
search "chosen", depth: 2, uname: serial@4600
search "chosen", depth: 2, uname: pci@8000
search "chosen", depth: 3, uname: i8259@19000
search "chosen", depth: 2, uname: pci@9000
search "chosen", depth: 2, uname: pic@40000
search "chosen", depth: 1, uname: chosen
Command line is: root=/dev/nfs nfsroot=10.193.20.105:/tftpboot/10.193.20.118 ip=
10.193.20.118:10.193.20.105:127.0.0.1:255.255.255.0:mpc8548:eth0:off console=tty
S1,115200
dt_root_size_cells = 1
dt_root_addr_cells = 1
memory scan node memory, reg size 8, data: 0 10000000 2 1,
 - 0 ,  10000000
Phys. mem: 10000000
-> move_device_tree
<- move_device_tree
Scanning CPUs ...
boot cpu: logical 0 physical 0
 <- early_init_devtree()
Using MPC85xx CDS machine description
Memory CAM mapping: CAM0=256Mb, CAM1=0Mb, CAM2=0Mb residual: 0Mb
Linux version 2.6.20-rc1-g4c2a3b24-dirty (roy@bus) (gcc version 3.4.3) #16 Wed J
an 10 11:28:38 CST 2007
 -> unflatten_device_tree()
  size is 1370, allocating...
  unflattening cfffec8c...
fixed up name for  ->
fixed up name for cpus -> cpus
fixed up name for PowerPC,8548@0 -> PowerPC,8548
fixed up name for memory -> memory
fixed up name for soc8548@e0000000 -> soc8548
fixed up name for i2c@3000 -> i2c
fixed up name for mdio@24520 -> mdio
fixed up name for ethernet-phy@0 -> ethernet-phy
fixed up name for ethernet-phy@1 -> ethernet-phy
fixed up name for ethernet-phy@2 -> ethernet-phy
fixed up name for ethernet-phy@3 -> ethernet-phy
fixed up name for ethernet@24000 -> ethernet
fixed up name for ethernet@25000 -> ethernet
fixed up name for ethernet@26000 -> ethernet
fixed up name for serial@4500 -> serial
fixed up name for serial@4600 -> serial
fixed up name for pci@8000 -> pci
fixed up name for i8259@19000 -> i8259
fixed up name for pci@9000 -> pci
fixed up name for pic@40000 -> pic
 <- unflatten_device_tree()
setup_arch: bootmem
mpc85xx_cds_setup_arch()
CDS Version = 0x13 in slot 1

Found MPC85xx PCI host bridge at 0x00000000e0008000. Firmware bus number: 0->2
PCI: MEM[0] 0x80000000 -> 0x9fffffff
PCI: IO 0x0 -> 0xfffff
Found MPC85xx PCI host bridge at 0x00000000e0009000. Firmware bus number: 0->0
PCI: MEM[0] 0xa0000000 -> 0xbfffffff
PCI: IO 0x0 -> 0xfffff
arch: exit
Zone PFN ranges:
  DMA             0 ->    65536
  Normal      65536 ->    65536
early_node_map[1] active PFN ranges
    0:        0 ->    65536
Built 1 zonelists.  Total pages: 65024
Kernel command line: root=/dev/nfs nfsroot=10.193.20.105:/tftpboot/10.193.20.118
 ip=10.193.20.118:10.193.20.105:127.0.0.1:255.255.255.0:mpc8548:eth0:off console
=ttyS1,115200
mpic: Setting up MPIC " OpenPIC  " version 1.2 at e0040000, max 1 CPUs
mpic: ISU size: 4, shift: 2, mask: 3
mpic: Initializing for 60 sources
MPIC flags: 13
PID hash table entries: 1024 (order: 10, 4096 bytes)

============================================================================
Stop here.
I just use the device tree with the git source code. You can find it at
arch/powerpc/boot/dts/mpc8548cds.dts
Any comment?
Thanks a lot!
Roy

Re: [PATCH] powerpc: Fix irq routing on some PowerMac 32 bits

[Benjamin Herrenschmidt]

> Stop here.

Probably the log is just the last console output you got from the early
console before it switched to the main one. Do you use udbg on these
boards for early boot messages ? If yes, have you tried the
udbg-immortal option ? (Or just prevent udbg from unregistering).

Do you have access to a HW debugging tool maybe to trace what's going
on ?

I have no idea what's wrong at this point, though it's likely to be a
problem with your device-tree, I can't verify it without a bit more
debug data. Have you tried adding some debug infos to the new
implementation of pci_device_to_OF_node(), check how it's called and
what it returns ? That might give you a clue about the problem.

Ben.

Re: [PATCH] powerpc: Fix irq routing on some PowerMac 32 bits

[Zang Roy-r61911]

On Wed, 2007-01-10 at 11:58, Benjamin Herrenschmidt wrote:
> > Stop here.
> 
> Probably the log is just the last console output you got from the
> early
> console before it switched to the main one. Do you use udbg on these
> boards for early boot messages ? If yes, have you tried the
> udbg-immortal option ? (Or just prevent udbg from unregistering).
> 
> Do you have access to a HW debugging tool maybe to trace what's going
> on ?
> 
> I have no idea what's wrong at this point, though it's likely to be a
> problem with your device-tree, I can't verify it without a bit more
> debug data. Have you tried adding some debug infos to the new
> implementation of pci_device_to_OF_node(), check how it's called and
> what it returns ? That might give you a clue about the problem.

Thanks for all your suggestions. I had thought you could point out where
was wrong :-). Now, I have to trace it myself. I do not have HW
debugging tools, I only have printk().

I catch the bug. In your patch:

+       /* not a root bus, we need to get our parent */
+       parent = scan_OF_for_pci_bus(bus->parent);
+       if (parent == NULL)
+               return NULL;
+
+       /* now iterate for children for a match */
+       np = scan_OF_for_pci_dev(parent, bus->self->devfn);
+       of_node_put(parent);
+
+       /* sanity check */
+       if (strcmp(np->type, "pci") != 0)
+               printk(KERN_WARNING "pci: wrong type \"%s\" for bridge %s\n",
+                      np->type, np->full_name);

I think you should judge np != NULL before using it.  If np == NULL,
np->type will cause memory access error. On CDS and some other Freescle
board, such as 8641HPCN, there is pci bridge, np = NULL , this will
cause error.
If you agree, you or I will provide a patch to fix it:-).
Thanks.
Roy 

Re: [PATCH] powerpc: Fix irq routing on some PowerMac 32 bits

[Benjamin Herrenschmidt]

> Thanks for all your suggestions. I had thought you could point out where
> was wrong :-). Now, I have to trace it myself. I do not have HW
> debugging tools, I only have printk().
> 
> I catch the bug. In your patch:
> 
> +       /* not a root bus, we need to get our parent */
> +       parent = scan_OF_for_pci_bus(bus->parent);
> +       if (parent == NULL)
> +               return NULL;
> +
> +       /* now iterate for children for a match */
> +       np = scan_OF_for_pci_dev(parent, bus->self->devfn);
> +       of_node_put(parent);
> +
> +       /* sanity check */
> +       if (strcmp(np->type, "pci") != 0)
> +               printk(KERN_WARNING "pci: wrong type \"%s\" for bridge %s\n",
> +                      np->type, np->full_name);
> 
> I think you should judge np != NULL before using it.  If np == NULL,
> np->type will cause memory access error. On CDS and some other Freescle
> board, such as 8641HPCN, there is pci bridge, np = NULL , this will
> cause error.
> If you agree, you or I will provide a patch to fix it:-).

While I agree that we should definitely test for NULL and properly fail
instead of crashing, there is still the question of why do you fail at
this point. Do you have PCI devices that don't have corresponding device
nodes ? That is not a problem per-se, I'm just wondering if you have a
device that is supposed to have a node but it's not matching due to a
problem with the "reg" property...

In any case, a patch is definitely needed to properly check for NULL and
should get into 2.6.20, so if you want to do it, it will be much
welcome !

Cheers,
Ben.

Re: [PATCH] powerpc: Fix irq routing on some PowerMac 32 bits

[Segher Boessenkool]

> While I agree that we should definitely test for NULL and properly fail
> instead of crashing, there is still the question of why do you fail at
> this point. Do you have PCI devices that don't have corresponding 
> device
> nodes ? That is not a problem per-se, I'm just wondering if you have a
> device that is supposed to have a node but it's not matching due to a
> problem with the "reg" property...

Yeah, show us the .dts file if you can, it might have
a problem, or at least it does something the kernel
doesn't expect.


Segher

RE: [PATCH] powerpc: Fix irq routing on some PowerMac 32 bits

[Wang Haiying-r54964]

=20
>While I agree that we should definitely test for NULL and properly fail
>instead of crashing, there is still the question of why do you fail at
>this point. Do you have PCI devices that don't have=20
>corresponding device
>nodes ? That is not a problem per-se, I'm just wondering if you have a
>device that is supposed to have a node but it's not matching due to a
>problem with the "reg" property...

The reason that it failed at this point is that there is a PCI bridge
connected to the PCI controller of the silicon. We defined this PCI
bridge as a pci node, but all the devices behind the bridge are not
defined as PCI node individually, thus they don't have "reg" property
except the bridge itself. Also, they are not on the same bus as which
the pci bridge is on. Please refer the
arch/powerpc/boot/dts/mpc8641_hpcn.dts or mpc8548cds.dts.

So we should definitely test for NULL here.=20

Haiying

Re: [PATCH] powerpc: Fix irq routing on some PowerMac 32 bits

[Zang Roy-r61911]

On Wed, 2007-01-10 at 20:13, Benjamin Herrenschmidt wrote:

> While I agree that we should definitely test for NULL and properly
> fail
> instead of crashing, there is still the question of why do you fail at
> this point. Do you have PCI devices that don't have corresponding
> device
> nodes ? 
Flat device tree does not allocate node for each PCI device on a bus.
> That is not a problem per-se, I'm just wondering if you have a
> device that is supposed to have a node but it's not matching due to a
> problem with the "reg" property...
The problem was induced by PCI bridge just as Haiying explained. Until
now, we do not describe PCI bridge in device tree. Only PCI host has the
"reg" property. We can not identify the PCI device on bus 0 or bus 1 by
node in device tree. The difference between device tree and OF might
introduce potential bugs for PCI bus. We should consider the difference.
> 
> In any case, a patch is definitely needed to properly check for NULL
> and
> should get into 2.6.20, so if you want to do it, it will be much
> welcome !
I will provide it later.
Roy

Re: [PATCH] powerpc: Fix irq routing on some PowerMac 32 bits

[Benjamin Herrenschmidt]

> The problem was induced by PCI bridge just as Haiying explained. Until
> now, we do not describe PCI bridge in device tree. Only PCI host has the
> "reg" property. We can not identify the PCI device on bus 0 or bus 1 by
> node in device tree. The difference between device tree and OF might
> introduce potential bugs for PCI bus. We should consider the difference.

That's fine, there is no problem in doing that, though I do recommend as
a general rule to put PCI-to-PCI bridges that are soldered on the board
in the device-tree, you don't absolutely have to.
 
> > In any case, a patch is definitely needed to properly check for NULL
> > and
> > should get into 2.6.20, so if you want to do it, it will be much
> > welcome !
> I will provide it later.

This urgent, I'll do a patch myself and send it to paulus.

Ben.

Re: [PATCH] powerpc: Fix irq routing on some PowerMac 32 bits

[Zang Roy-r61911]

On Wed, 2007-01-10 at 20:22, Segher Boessenkool wrote:
> > While I agree that we should definitely test for NULL and properly
> fail
> > instead of crashing, there is still the question of why do you fail
> at
> > this point. Do you have PCI devices that don't have corresponding 
> > device
> > nodes ? That is not a problem per-se, I'm just wondering if you have
> a
> > device that is supposed to have a node but it's not matching due to
> a
> > problem with the "reg" property...
> 
> Yeah, show us the .dts file if you can, it might have
> a problem, or at least it does something the kernel
> doesn't expect.
The dts file just locates at arch/powerpc/boot/dts. MPC8641HPCN and
MPC8548CDS can be treated as example.

I believe the mechanism for PCI bridge description in device tree should
be improved to comply with  OF.
Roy

Re: [PATCH] powerpc: Fix irq routing on some PowerMac 32 bits

[Zang Roy-r61911]

On Thu, 2007-01-11 at 13:28, Benjamin Herrenschmidt wrote:
> > The problem was induced by PCI bridge just as Haiying explained.
> Until
> > now, we do not describe PCI bridge in device tree. Only PCI host has
> the
> > "reg" property. We can not identify the PCI device on bus 0 or bus 1
> by
> > node in device tree. The difference between device tree and OF might
> > introduce potential bugs for PCI bus. We should consider the
> difference.
> 
> That's fine, there is no problem in doing that, though I do recommend
> as
> a general rule to put PCI-to-PCI bridges that are soldered on the
> board
> in the device-tree,
I agree! I will try to improve PCI-to-PCI bridges description on my 85xx
CDS board support.
Hope to get your support about OF.
>  you don't absolutely have to.
>  
> > > In any case, a patch is definitely needed to properly check for
> NULL
> > > and
> > > should get into 2.6.20, so if you want to do it, it will be much
> > > welcome !
> > I will provide it later.
> 
> This urgent, I'll do a patch myself and send it to paulus.
urgent for me too. I'll do it now. Let's see ...
Roy

Re: [PATCH] powerpc: Fix irq routing on some PowerMac 32 bits

[Benjamin Herrenschmidt]

> urgent for me too. I'll do it now. Let's see ...

I beat you at it :-) Besides, I think that "sanity check" was bogus for
other reasons anyway. It would break support for the chaos brige on
older Apple powermacs which used the device-type "vci" instead of PCI.

Ben

Re: [PATCH] powerpc: Fix irq routing on some PowerMac 32 bits

[Zang Roy-r61911]

On Thu, 2007-01-11 at 13:40, Benjamin Herrenschmidt wrote:
> > urgent for me too. I'll do it now. Let's see ...
> 
> I beat you at it :-)
You win, then I can see PCI-to-PCI bridge in device tree.  It may be
reasonable to add a node for it.
>  Besides, I think that "sanity check" was bogus for
> other reasons anyway. It would break support for the chaos brige on
> older Apple powermacs which used the device-type "vci" instead of PCI.
Agree to move them!
If I fix the bug, I might test np and then return. So you are the first
is not a bad thing for us.
Roy