[PATCH] powerpc: make U4 PCIe work

[PATCH] powerpc: make U4 PCIe work

[Benjamin Herrenschmidt]

The Maple support code was missing code for U4/CPC945 PCIe. This adds
it, enabling it to work on tigerwood boards, and possibly also js21
using SLOF.

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

Patch just missed the merge window but should go in anyway. It's been
hanging around for a while, I just forgot about it, and it's fairly
harmless.

Index: linux-work/arch/powerpc/platforms/maple/pci.c
===================================================================
--- linux-work.orig/arch/powerpc/platforms/maple/pci.c	2006-10-05 14:14:28.000000000 +1000
+++ linux-work/arch/powerpc/platforms/maple/pci.c	2006-10-05 14:29:30.000000000 +1000
@@ -8,7 +8,7 @@
  * 2 of the License, or (at your option) any later version.
  */
 
-#define DEBUG
+#undef DEBUG
 
 #include <linux/kernel.h>
 #include <linux/pci.h>
@@ -16,6 +16,7 @@
 #include <linux/string.h>
 #include <linux/init.h>
 #include <linux/bootmem.h>
+#include <linux/irq.h>
 
 #include <asm/sections.h>
 #include <asm/io.h>
@@ -33,7 +34,7 @@
 #define DBG(x...)
 #endif
 
-static struct pci_controller *u3_agp, *u3_ht;
+static struct pci_controller *u3_agp, *u3_ht, *u4_pcie;
 
 static int __init fixup_one_level_bus_range(struct device_node *node, int higher)
 {
@@ -287,6 +288,110 @@ static struct pci_ops u3_ht_pci_ops =
 	u3_ht_write_config
 };
 
+#define U4_PCIE_CFA0(devfn, off)        \
+        ((1 << ((unsigned int)PCI_SLOT(dev_fn)))        \
+         | (((unsigned int)PCI_FUNC(dev_fn)) << 8)      \
+         | ((((unsigned int)(off)) >> 8) << 28) \
+         | (((unsigned int)(off)) & 0xfcU))
+
+#define U4_PCIE_CFA1(bus, devfn, off)   \
+        ((((unsigned int)(bus)) << 16) \
+         |(((unsigned int)(devfn)) << 8)        \
+         | ((((unsigned int)(off)) >> 8) << 28) \
+         |(((unsigned int)(off)) & 0xfcU)       \
+         |1UL)
+
+static volatile void __iomem *u4_pcie_cfg_access(struct pci_controller* hose,
+                                        u8 bus, u8 dev_fn, int offset)
+{
+        unsigned int caddr;
+
+        if (bus == hose->first_busno) {
+                caddr = U4_PCIE_CFA0(dev_fn, offset);
+        } else
+                caddr = U4_PCIE_CFA1(bus, dev_fn, offset);
+
+        /* Uninorth will return garbage if we don't read back the value ! */
+        do {
+                out_le32(hose->cfg_addr, caddr);
+        } while (in_le32(hose->cfg_addr) != caddr);
+
+        offset &= 0x03;
+        return hose->cfg_data + offset;
+}
+
+static int u4_pcie_read_config(struct pci_bus *bus, unsigned int devfn,
+                               int offset, int len, u32 *val)
+{
+        struct pci_controller *hose;
+        volatile void __iomem *addr;
+
+        hose = pci_bus_to_host(bus);
+        if (hose == NULL)
+                return PCIBIOS_DEVICE_NOT_FOUND;
+        if (offset >= 0x1000)
+                return  PCIBIOS_BAD_REGISTER_NUMBER;
+        addr = u4_pcie_cfg_access(hose, bus->number, devfn, offset);
+        if (!addr)
+                return PCIBIOS_DEVICE_NOT_FOUND;
+        /*
+         * Note: the caller has already checked that offset is
+         * suitably aligned and that len is 1, 2 or 4.
+         */
+        switch (len) {
+        case 1:
+                *val = in_8(addr);
+                break;
+        case 2:
+                *val = in_le16(addr);
+                break;
+        default:
+                *val = in_le32(addr);
+                break;
+        }
+        return PCIBIOS_SUCCESSFUL;
+}
+static int u4_pcie_write_config(struct pci_bus *bus, unsigned int devfn,
+                                int offset, int len, u32 val)
+{
+        struct pci_controller *hose;
+        volatile void __iomem *addr;
+
+        hose = pci_bus_to_host(bus);
+        if (hose == NULL)
+                return PCIBIOS_DEVICE_NOT_FOUND;
+        if (offset >= 0x1000)
+                return  PCIBIOS_BAD_REGISTER_NUMBER;
+        addr = u4_pcie_cfg_access(hose, bus->number, devfn, offset);
+        if (!addr)
+                return PCIBIOS_DEVICE_NOT_FOUND;
+        /*
+         * Note: the caller has already checked that offset is
+         * suitably aligned and that len is 1, 2 or 4.
+         */
+        switch (len) {
+        case 1:
+                out_8(addr, val);
+                (void) in_8(addr);
+                break;
+        case 2:
+                out_le16(addr, val);
+                (void) in_le16(addr);
+                break;
+        default:
+                out_le32(addr, val);
+                (void) in_le32(addr);
+                break;
+        }
+        return PCIBIOS_SUCCESSFUL;
+}
+
+static struct pci_ops u4_pcie_pci_ops =
+{
+        u4_pcie_read_config,
+        u4_pcie_write_config
+};
+
 static void __init setup_u3_agp(struct pci_controller* hose)
 {
 	/* On G5, we move AGP up to high bus number so we don't need
@@ -307,6 +412,26 @@ static void __init setup_u3_agp(struct p
 	u3_agp = hose;
 }
 
+static void __init setup_u4_pcie(struct pci_controller* hose)
+{
+        /* We currently only implement the "non-atomic" config space, to
+         * be optimised later.
+         */
+        hose->ops = &u4_pcie_pci_ops;
+        hose->cfg_addr = ioremap(0xf0000000 + 0x800000, 0x1000);
+        hose->cfg_data = ioremap(0xf0000000 + 0xc00000, 0x1000);
+
+        /* The bus contains a bridge from root -> device, we need to
+         * make it visible on bus 0 so that we pick the right type
+         * of config cycles. If we didn't, we would have to force all
+         * config cycles to be type 1. So we override the "bus-range"
+         * property here
+         */
+        hose->first_busno = 0x00;
+        hose->last_busno = 0xff;
+        u4_pcie = hose;
+}
+
 static void __init setup_u3_ht(struct pci_controller* hose)
 {
 	hose->ops = &u3_ht_pci_ops;
@@ -354,6 +479,10 @@ static int __init add_bridge(struct devi
 		setup_u3_ht(hose);
 		disp_name = "U3-HT";
 		primary = 1;
+        } else if (device_is_compatible(dev, "u4-pcie")) {
+                setup_u4_pcie(hose);
+                disp_name = "U4-PCIE";
+                primary = 0;
 	}
 	printk(KERN_INFO "Found %s PCI host bridge. Firmware bus number: %d->%d\n",
 		disp_name, hose->first_busno, hose->last_busno);
@@ -361,7 +490,6 @@ static int __init add_bridge(struct devi
 	/* Interpret the "ranges" property */
 	/* This also maps the I/O region and sets isa_io/mem_base */
 	pci_process_bridge_OF_ranges(hose, dev, primary);
-	pci_setup_phb_io(hose, primary);
 
 	/* Fixup "bus-range" OF property */
 	fixup_bus_range(dev);
@@ -376,8 +504,17 @@ void __init maple_pcibios_fixup(void)
 
 	DBG(" -> maple_pcibios_fixup\n");
 
-	for_each_pci_dev(dev)
-		pci_read_irq_line(dev);
+	for_each_pci_dev(dev) {
+		/* Fixup IRQ for PCIe host */
+		if (u4_pcie != NULL && dev->bus->number == 0 &&
+		    pci_bus_to_host(dev->bus) == u4_pcie) {
+			printk(KERN_DEBUG "Fixup U4 PCIe IRQ\n");
+			dev->irq = irq_create_mapping(NULL, 1);
+			if (dev->irq != NO_IRQ)
+				set_irq_type(dev->irq, IRQ_TYPE_LEVEL_LOW);
+		} else
+			pci_read_irq_line(dev);
+	}
 
 	DBG(" <- maple_pcibios_fixup\n");
 }
@@ -388,8 +525,10 @@ static void __init maple_fixup_phb_resou
 	
 	list_for_each_entry_safe(hose, tmp, &hose_list, list_node) {
 		unsigned long offset = (unsigned long)hose->io_base_virt - pci_io_base;
+
 		hose->io_resource.start += offset;
 		hose->io_resource.end += offset;
+
 		printk(KERN_INFO "PCI Host %d, io start: %llx; io end: %llx\n",
 		       hose->global_number,
 		       (unsigned long long)hose->io_resource.start,
@@ -431,6 +570,19 @@ void __init maple_pci_init(void)
 	if (ht && add_bridge(ht) != 0)
 		of_node_put(ht);
 
+        /*
+         * We need to call pci_setup_phb_io for the HT bridge first
+         * so it gets the I/O port numbers starting at 0, and we
+         * need to call it for the AGP bridge after that so it gets
+         * small positive I/O port numbers.
+         */
+        if (u3_ht)
+                pci_setup_phb_io(u3_ht, 1);
+        if (u3_agp)
+                pci_setup_phb_io(u3_agp, 0);
+        if (u4_pcie)
+                pci_setup_phb_io(u4_pcie, 0);
+
 	/* Fixup the IO resources on our host bridges as the common code
 	 * does it only for childs of the host bridges
 	 */
@@ -479,6 +631,9 @@ int maple_pci_get_legacy_ide_irq(struct 
 /* XXX: To remove once all firmwares are ok */
 static void fixup_maple_ide(struct pci_dev* dev)
 {
+	if (!machine_is(maple))
+		return;
+
 #if 0 /* Enable this to enable IDE port 0 */
 	{
 		u8 v;
@@ -495,7 +650,7 @@ static void fixup_maple_ide(struct pci_d
 	dev->resource[4].start = 0xcc00;
 	dev->resource[4].end = 0xcc10;
 #endif
-#if 1 /* Enable this to fixup IDE sense/polarity of irqs in IO-APICs */
+#if 0 /* Enable this to fixup IDE sense/polarity of irqs in IO-APICs */
 	{
 		struct pci_dev *apicdev;
 		u32 v;

Re: [PATCH] powerpc: make U4 PCIe work

[Nathan Lynch]

Hi-

Benjamin Herrenschmidt wrote:
> The Maple support code was missing code for U4/CPC945 PCIe. This adds
> it, enabling it to work on tigerwood boards, and possibly also js21
> using SLOF.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> 
> Patch just missed the merge window but should go in anyway. It's been
> hanging around for a while, I just forgot about it, and it's fairly
> harmless.
> 
> Index: linux-work/arch/powerpc/platforms/maple/pci.c
> ===================================================================
> --- linux-work.orig/arch/powerpc/platforms/maple/pci.c	2006-10-05 14:14:28.000000000 +1000
> +++ linux-work/arch/powerpc/platforms/maple/pci.c	2006-10-05 14:29:30.000000000 +1000
> @@ -8,7 +8,7 @@
>   * 2 of the License, or (at your option) any later version.
>   */
>  
> -#define DEBUG
> +#undef DEBUG
>  
>  #include <linux/kernel.h>
>  #include <linux/pci.h>
> @@ -16,6 +16,7 @@
>  #include <linux/string.h>
>  #include <linux/init.h>
>  #include <linux/bootmem.h>
> +#include <linux/irq.h>
>  
>  #include <asm/sections.h>
>  #include <asm/io.h>
> @@ -33,7 +34,7 @@
>  #define DBG(x...)
>  #endif
>  
> -static struct pci_controller *u3_agp, *u3_ht;
> +static struct pci_controller *u3_agp, *u3_ht, *u4_pcie;
>  
>  static int __init fixup_one_level_bus_range(struct device_node *node, int higher)
>  {
> @@ -287,6 +288,110 @@ static struct pci_ops u3_ht_pci_ops =
>  	u3_ht_write_config
>  };
>  
> +#define U4_PCIE_CFA0(devfn, off)        \
> +        ((1 << ((unsigned int)PCI_SLOT(dev_fn)))        \
> +         | (((unsigned int)PCI_FUNC(dev_fn)) << 8)      \
> +         | ((((unsigned int)(off)) >> 8) << 28) \
> +         | (((unsigned int)(off)) & 0xfcU))


This macro has the same kind of bug I just fixed for the U3 code in
this file.  The macro parameter is named devfn but the body of the
macro refers to dev_fn.  I see no reason that these shouldn't be
functions.


> +
> +#define U4_PCIE_CFA1(bus, devfn, off)   \
> +        ((((unsigned int)(bus)) << 16) \
> +         |(((unsigned int)(devfn)) << 8)        \
> +         | ((((unsigned int)(off)) >> 8) << 28) \
> +         |(((unsigned int)(off)) & 0xfcU)       \
> +         |1UL)
> +
> +static volatile void __iomem *u4_pcie_cfg_access(struct pci_controller* hose,
> +                                        u8 bus, u8 dev_fn, int offset)
> +{
> +        unsigned int caddr;
> +
> +        if (bus == hose->first_busno) {
> +                caddr = U4_PCIE_CFA0(dev_fn, offset);
> +        } else
> +                caddr = U4_PCIE_CFA1(bus, dev_fn, offset);
> +
> +        /* Uninorth will return garbage if we don't read back the value ! */
> +        do {
> +                out_le32(hose->cfg_addr, caddr);
> +        } while (in_le32(hose->cfg_addr) != caddr);
> +
> +        offset &= 0x03;
> +        return hose->cfg_data + offset;
> +}
> +
> +static int u4_pcie_read_config(struct pci_bus *bus, unsigned int devfn,
> +                               int offset, int len, u32 *val)
> +{
> +        struct pci_controller *hose;
> +        volatile void __iomem *addr;
> +
> +        hose = pci_bus_to_host(bus);
> +        if (hose == NULL)
> +                return PCIBIOS_DEVICE_NOT_FOUND;
> +        if (offset >= 0x1000)
> +                return  PCIBIOS_BAD_REGISTER_NUMBER;
> +        addr = u4_pcie_cfg_access(hose, bus->number, devfn, offset);
> +        if (!addr)
> +                return PCIBIOS_DEVICE_NOT_FOUND;
> +        /*
> +         * Note: the caller has already checked that offset is
> +         * suitably aligned and that len is 1, 2 or 4.
> +         */
> +        switch (len) {
> +        case 1:
> +                *val = in_8(addr);
> +                break;
> +        case 2:
> +                *val = in_le16(addr);
> +                break;
> +        default:
> +                *val = in_le32(addr);
> +                break;
> +        }
> +        return PCIBIOS_SUCCESSFUL;
> +}
> +static int u4_pcie_write_config(struct pci_bus *bus, unsigned int devfn,
> +                                int offset, int len, u32 val)
> +{
> +        struct pci_controller *hose;
> +        volatile void __iomem *addr;
> +
> +        hose = pci_bus_to_host(bus);
> +        if (hose == NULL)
> +                return PCIBIOS_DEVICE_NOT_FOUND;
> +        if (offset >= 0x1000)
> +                return  PCIBIOS_BAD_REGISTER_NUMBER;
> +        addr = u4_pcie_cfg_access(hose, bus->number, devfn, offset);
> +        if (!addr)
> +                return PCIBIOS_DEVICE_NOT_FOUND;
> +        /*
> +         * Note: the caller has already checked that offset is
> +         * suitably aligned and that len is 1, 2 or 4.
> +         */
> +        switch (len) {
> +        case 1:
> +                out_8(addr, val);
> +                (void) in_8(addr);
> +                break;
> +        case 2:
> +                out_le16(addr, val);
> +                (void) in_le16(addr);
> +                break;
> +        default:
> +                out_le32(addr, val);
> +                (void) in_le32(addr);
> +                break;
> +        }
> +        return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static struct pci_ops u4_pcie_pci_ops =
> +{
> +        u4_pcie_read_config,
> +        u4_pcie_write_config
> +};
> +
>  static void __init setup_u3_agp(struct pci_controller* hose)
>  {
>  	/* On G5, we move AGP up to high bus number so we don't need
> @@ -307,6 +412,26 @@ static void __init setup_u3_agp(struct p
>  	u3_agp = hose;
>  }
>  
> +static void __init setup_u4_pcie(struct pci_controller* hose)
> +{
> +        /* We currently only implement the "non-atomic" config space, to
> +         * be optimised later.
> +         */
> +        hose->ops = &u4_pcie_pci_ops;
> +        hose->cfg_addr = ioremap(0xf0000000 + 0x800000, 0x1000);
> +        hose->cfg_data = ioremap(0xf0000000 + 0xc00000, 0x1000);
> +
> +        /* The bus contains a bridge from root -> device, we need to
> +         * make it visible on bus 0 so that we pick the right type
> +         * of config cycles. If we didn't, we would have to force all
> +         * config cycles to be type 1. So we override the "bus-range"
> +         * property here
> +         */
> +        hose->first_busno = 0x00;
> +        hose->last_busno = 0xff;
> +        u4_pcie = hose;
> +}
> +
>  static void __init setup_u3_ht(struct pci_controller* hose)
>  {
>  	hose->ops = &u3_ht_pci_ops;
> @@ -354,6 +479,10 @@ static int __init add_bridge(struct devi
>  		setup_u3_ht(hose);
>  		disp_name = "U3-HT";
>  		primary = 1;
> +        } else if (device_is_compatible(dev, "u4-pcie")) {
> +                setup_u4_pcie(hose);
> +                disp_name = "U4-PCIE";
> +                primary = 0;
>  	}
>  	printk(KERN_INFO "Found %s PCI host bridge. Firmware bus number: %d->%d\n",
>  		disp_name, hose->first_busno, hose->last_busno);
> @@ -361,7 +490,6 @@ static int __init add_bridge(struct devi
>  	/* Interpret the "ranges" property */
>  	/* This also maps the I/O region and sets isa_io/mem_base */
>  	pci_process_bridge_OF_ranges(hose, dev, primary);
> -	pci_setup_phb_io(hose, primary);
>  
>  	/* Fixup "bus-range" OF property */
>  	fixup_bus_range(dev);
> @@ -376,8 +504,17 @@ void __init maple_pcibios_fixup(void)
>  
>  	DBG(" -> maple_pcibios_fixup\n");
>  
> -	for_each_pci_dev(dev)
> -		pci_read_irq_line(dev);
> +	for_each_pci_dev(dev) {
> +		/* Fixup IRQ for PCIe host */
> +		if (u4_pcie != NULL && dev->bus->number == 0 &&
> +		    pci_bus_to_host(dev->bus) == u4_pcie) {
> +			printk(KERN_DEBUG "Fixup U4 PCIe IRQ\n");
> +			dev->irq = irq_create_mapping(NULL, 1);
> +			if (dev->irq != NO_IRQ)
> +				set_irq_type(dev->irq, IRQ_TYPE_LEVEL_LOW);
> +		} else
> +			pci_read_irq_line(dev);
> +	}
>  
>  	DBG(" <- maple_pcibios_fixup\n");
>  }
> @@ -388,8 +525,10 @@ static void __init maple_fixup_phb_resou
>  	
>  	list_for_each_entry_safe(hose, tmp, &hose_list, list_node) {
>  		unsigned long offset = (unsigned long)hose->io_base_virt - pci_io_base;
> +
>  		hose->io_resource.start += offset;
>  		hose->io_resource.end += offset;
> +
>  		printk(KERN_INFO "PCI Host %d, io start: %llx; io end: %llx\n",
>  		       hose->global_number,
>  		       (unsigned long long)hose->io_resource.start,
> @@ -431,6 +570,19 @@ void __init maple_pci_init(void)
>  	if (ht && add_bridge(ht) != 0)
>  		of_node_put(ht);
>  
> +        /*
> +         * We need to call pci_setup_phb_io for the HT bridge first
> +         * so it gets the I/O port numbers starting at 0, and we
> +         * need to call it for the AGP bridge after that so it gets
> +         * small positive I/O port numbers.
> +         */
> +        if (u3_ht)
> +                pci_setup_phb_io(u3_ht, 1);
> +        if (u3_agp)
> +                pci_setup_phb_io(u3_agp, 0);
> +        if (u4_pcie)
> +                pci_setup_phb_io(u4_pcie, 0);
> +
>  	/* Fixup the IO resources on our host bridges as the common code
>  	 * does it only for childs of the host bridges
>  	 */
> @@ -479,6 +631,9 @@ int maple_pci_get_legacy_ide_irq(struct 
>  /* XXX: To remove once all firmwares are ok */
>  static void fixup_maple_ide(struct pci_dev* dev)
>  {
> +	if (!machine_is(maple))
> +		return;
> +
>  #if 0 /* Enable this to enable IDE port 0 */
>  	{
>  		u8 v;
> @@ -495,7 +650,7 @@ static void fixup_maple_ide(struct pci_d
>  	dev->resource[4].start = 0xcc00;
>  	dev->resource[4].end = 0xcc10;
>  #endif
> -#if 1 /* Enable this to fixup IDE sense/polarity of irqs in IO-APICs */
> +#if 0 /* Enable this to fixup IDE sense/polarity of irqs in IO-APICs */
>  	{
>  		struct pci_dev *apicdev;
>  		u32 v;

Do the changes to fixup_maple_ide have anything to do with the rest of
the patch?  It's not clear from your description.  And with that last
hunk the function now appears to do nothing.

Could we consider merging the powermac and maple pci code into a
common location that would be used by both platforms?  There's quite a
bit of code duplication going on here...

Re: [PATCH] powerpc: make U4 PCIe work

[Segher Boessenkool]

> +        if (bus == hose->first_busno) {
> +                caddr = U4_PCIE_CFA0(dev_fn, offset);
> +        } else
> +                caddr = U4_PCIE_CFA1(bus, dev_fn, offset);

Please fold CFA0 and CFA1 into one inline function, much
cleaner.

You also should check (at init time) whether indirect config
access mode is actually enabled -- or just enable it yourself.

> +        /* Uninorth will return garbage if we don't read back the  
> value ! */
> +        do {
> +                out_le32(hose->cfg_addr, caddr);
> +        } while (in_le32(hose->cfg_addr) != caddr);

Not an issue on U4, kill this.

> +static int u4_pcie_read_config(struct pci_bus *bus, unsigned int  
> devfn,
> +                               int offset, int len, u32 *val)
> +{
> +        struct pci_controller *hose;
> +        volatile void __iomem *addr;
> +
> +        hose = pci_bus_to_host(bus);
> +        if (hose == NULL)
> +                return PCIBIOS_DEVICE_NOT_FOUND;

Is this check needed?  Can this code ever be called if hose would
be 0?  The variable isn't actually used anywhere else in this
function either.

> +        struct pci_controller *hose;
> +        volatile void __iomem *addr;

No need for the volatile, the inXX()/outXX() things should handle
this just fine.

> +        /* The bus contains a bridge from root -> device, we need to
> +         * make it visible on bus 0 so that we pick the right type
> +         * of config cycles. If we didn't, we would have to force all
> +         * config cycles to be type 1. So we override the "bus-range"
> +         * property here
> +         */
> +        hose->first_busno = 0x00;
> +        hose->last_busno = 0xff;

I don't understand the comment, nor the need for this code.  Is this
just old garbage ported over from PowerMac?

> +	for_each_pci_dev(dev) {
> +		/* Fixup IRQ for PCIe host */
> +		if (u4_pcie != NULL && dev->bus->number == 0 &&
> +		    pci_bus_to_host(dev->bus) == u4_pcie) {
> +			printk(KERN_DEBUG "Fixup U4 PCIe IRQ\n");
> +			dev->irq = irq_create_mapping(NULL, 1);
> +			if (dev->irq != NO_IRQ)
> +				set_irq_type(dev->irq, IRQ_TYPE_LEVEL_LOW);

Please do this in the platform code for boards that actually need
it, by fixing up the interrupt tree in the device tree.  MPIC IRQ1
is the PCIe error interrupt only btw, I wonder if this code does
the right thing at all?

> +        /*
> +         * We need to call pci_setup_phb_io for the HT bridge first
> +         * so it gets the I/O port numbers starting at 0, and we
> +         * need to call it for the AGP bridge after that so it gets
> +         * small positive I/O port numbers.
> +         */

Comment out of date (doesn't mention PCIe).  Oh and when do we
finally get real PCI domain support ;-)

> +	if (!machine_is(maple))
> +		return;

So the IDE wart fixup is now only executed on "real" Maples?  Or
all 970-based non-Apple boxes?  The naming starts to be confusing,
maybe the maple platform should be en-masse renamed to "ppc970" or
such.


Segher

Re: [PATCH] powerpc: make U4 PCIe work

[Benjamin Herrenschmidt]

> Do the changes to fixup_maple_ide have anything to do with the rest of
> the patch?  It's not clear from your description.  And with that last
> hunk the function now appears to do nothing.

Hrm... I could split that indeed. The last bits to fixup_maple_ide()
should no longer be necessary. They used to work around broken PIBS but
that should have been fixed a long time ago.

> Could we consider merging the powermac and maple pci code into a
> common location that would be used by both platforms?  There's quite a
> bit of code duplication going on here...

A bit but not exactly... I may do it but just not right now.

Ben.

Re: [PATCH] powerpc: make U4 PCIe work

[Benjamin Herrenschmidt]

On Thu, 2006-10-05 at 20:45 +0200, Segher Boessenkool wrote:
> > +        if (bus == hose->first_busno) {
> > +                caddr = U4_PCIE_CFA0(dev_fn, offset);
> > +        } else
> > +                caddr = U4_PCIE_CFA1(bus, dev_fn, offset);
> 
> Please fold CFA0 and CFA1 into one inline function, much
> cleaner.

I like it this way. That's how PowerMac does it too btw.

> You also should check (at init time) whether indirect config
> access mode is actually enabled -- or just enable it yourself.

Care to send a patch ?

> > +        /* Uninorth will return garbage if we don't read back the  
> > value ! */
> > +        do {
> > +                out_le32(hose->cfg_addr, caddr);
> > +        } while (in_le32(hose->cfg_addr) != caddr);
> 
> Not an issue on U4, kill this.

I'd rather keep it for now. A matter of testing. It's harmless anyway.
Not like config space accesses had to be fast...

> > +static int u4_pcie_read_config(struct pci_bus *bus, unsigned int  
> > devfn,
> > +                               int offset, int len, u32 *val)
> > +{
> > +        struct pci_controller *hose;
> > +        volatile void __iomem *addr;
> > +
> > +        hose = pci_bus_to_host(bus);
> > +        if (hose == NULL)
> > +                return PCIBIOS_DEVICE_NOT_FOUND;
> 
> Is this check needed?  Can this code ever be called if hose would
> be 0?  The variable isn't actually used anywhere else in this
> function either.

What are you talking about ? Of course hose is used.

> > +        struct pci_controller *hose;
> > +        volatile void __iomem *addr;
> 
> No need for the volatile, the inXX()/outXX() things should handle
> this just fine.

They can. Are you commenting just for the sake of it or what ? Feel free
to send a patch removing it in all them in all the pci accessors, we'd
hade those for ages.

> > +        /* The bus contains a bridge from root -> device, we need to
> > +         * make it visible on bus 0 so that we pick the right type
> > +         * of config cycles. If we didn't, we would have to force all
> > +         * config cycles to be type 1. So we override the "bus-range"
> > +         * property here
> > +         */
> > +        hose->first_busno = 0x00;
> > +        hose->last_busno = 0xff;
> 
> I don't understand the comment, nor the need for this code.  Is this
> just old garbage ported over from PowerMac?

It comes over from PowerMac indeed, wrong bus-range property there. It
doesn't harm to have it here, I'm not sure what PIBS puts in there.

> > +	for_each_pci_dev(dev) {
> > +		/* Fixup IRQ for PCIe host */
> > +		if (u4_pcie != NULL && dev->bus->number == 0 &&
> > +		    pci_bus_to_host(dev->bus) == u4_pcie) {
> > +			printk(KERN_DEBUG "Fixup U4 PCIe IRQ\n");
> > +			dev->irq = irq_create_mapping(NULL, 1);
> > +			if (dev->irq != NO_IRQ)
> > +				set_irq_type(dev->irq, IRQ_TYPE_LEVEL_LOW);
> 
> Please do this in the platform code for boards that actually need
> it, by fixing up the interrupt tree in the device tree.  MPIC IRQ1
> is the PCIe error interrupt only btw, I wonder if this code does
> the right thing at all?

Well, this is the platform code for boards based on U3/U4, so what ? No
OF tree I've seen so far can provide the irq for the host bridge because
none has a node for it (or rather for the p2p bridge that is visible
below attu). If yours is different, then feel free to send me a
device-tree (that I've asked from you how many monthes ago ?). This is
Maple board support, I'm adding code for Maple/Tigerwood.

It's the right interrupt to use with the PCIe port driver.

> > +        /*
> > +         * We need to call pci_setup_phb_io for the HT bridge first
> > +         * so it gets the I/O port numbers starting at 0, and we
> > +         * need to call it for the AGP bridge after that so it gets
> > +         * small positive I/O port numbers.
> > +         */
> 
> Comment out of date (doesn't mention PCIe).  Oh and when do we
> finally get real PCI domain support ;-)

Comment a bit out of date but the principle is still there, I want PCIe
to get the same non-overlapping bus numbers as AGP did for various
resaons (one is to have X work).
 
> > +	if (!machine_is(maple))
> > +		return;
> 
> So the IDE wart fixup is now only executed on "real" Maples?  Or
> all 970-based non-Apple boxes?  The naming starts to be confusing,
> maybe the maple platform should be en-masse renamed to "ppc970" or
> such.

The whole platform is called maple so what ? We just make sure we don't
call the code below when booting on pseries or whatever else.

At this point, the patch should go in as it is.

Ben.

Re: [PATCH] powerpc: make U4 PCIe work

[Nathan Lynch]

Benjamin Herrenschmidt wrote:
> 
> At this point, the patch should go in as it is.

At the very least, please fix those macros; I already pointed out the
bug to you.

Re: [PATCH] powerpc: make U4 PCIe work

[Benjamin Herrenschmidt]

On Thu, 2006-10-05 at 17:52 -0500, Nathan Lynch wrote:
> Benjamin Herrenschmidt wrote:
> > 
> > At this point, the patch should go in as it is.
> 
> At the very least, please fix those macros; I already pointed out the
> bug to you.

Ah yes, missed that bit of your mail. Interesting.

Ben.