[patch 0/6] RFC: PCI: consolidate pcibios_enable_resources() implementations, v2

[patch 0/6] RFC: PCI: consolidate pcibios_enable_resources() implementations, v2

[Bjorn Helgaas]

There are many implementations of pcibios_enable_resources() that differ
in minor ways that look more like bugs than architectural differences.
This patch series consolidates most of them to use the x86 version.

Changes between v1 and v2:

  - Moved ARM bridge enable to new platform_pci_enable_device(),
    called by pcibios_enable_device()

  - Moved PA-RISC SERR & PERR enable to new platform_pci_enable_device(),
    called by pcibios_enable_device()

  - In the generic pcibios_enable_resources(), adopted powerpc resource
    collision detection (check for (!r->parent) instead of
    (!r->start && r->end))

  - In the generic pcibios_enable_resources(), added a little more detail to
    the resource collision error message

  - Moved consolidated pcibios_enable_resources() from bios.c to setup-res.c

Thanks for the comments on the first version.  I think this addresses
all of them.

Any other comments would be welcome.

Bjorn
-- 

[patch 1/6] PCI: split pcibios_enable_resources() out of pcibios_enable_device()

[Bjorn Helgaas]

On x86, pcibios_enable_device() is factored into
pcibios_enable_resources() and pcibios_enable_irq().  On several other
architectures, the functional equivalent of pcibios_enable_resources()
is expanded directly inside pcibios_enable_device().

This splits these pcibios_enable_device() implementations to make them
more similar to the x86 implementation.

There should be no functional change from this patch.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

---
 arch/alpha/kernel/pci.c          |    8 +++++++-
 arch/arm/kernel/bios32.c         |    9 +++++++--
 arch/parisc/kernel/pci.c         |    6 +++++-
 arch/powerpc/kernel/pci-common.c |   14 +++++++++-----
 arch/sh/drivers/pci/pci.c        |    7 ++++++-
 arch/sparc64/kernel/pci.c        |    7 ++++++-
 arch/v850/kernel/rte_mb_a_pci.c  |    7 ++++++-
 7 files changed, 46 insertions(+), 12 deletions(-)

Index: work6/arch/alpha/kernel/pci.c
===================================================================
--- work6.orig/arch/alpha/kernel/pci.c	2008-02-18 10:43:50.000000000 -0700
+++ work6/arch/alpha/kernel/pci.c	2008-02-18 10:45:14.000000000 -0700
@@ -370,7 +370,7 @@
 #endif
 
 int
-pcibios_enable_device(struct pci_dev *dev, int mask)
+pcibios_enable_resources(struct pci_dev *dev, int mask)
 {
 	u16 cmd, oldcmd;
 	int i;
@@ -396,6 +396,12 @@
 	return 0;
 }
 
+int
+pcibios_enable_device(struct pci_dev *dev, int mask)
+{
+	return pcibios_enable_resources(dev, mask);
+}
+
 /*
  *  If we set up a device for bus mastering, we need to check the latency
  *  timer as certain firmware forgets to set it properly, as seen
Index: work6/arch/arm/kernel/bios32.c
===================================================================
--- work6.orig/arch/arm/kernel/bios32.c	2008-02-18 10:43:50.000000000 -0700
+++ work6/arch/arm/kernel/bios32.c	2008-02-18 10:45:14.000000000 -0700
@@ -655,10 +655,10 @@
 }
 
 /**
- * pcibios_enable_device - Enable I/O and memory.
+ * pcibios_enable_resources - Enable I/O and memory.
  * @dev: PCI device to be enabled
  */
-int pcibios_enable_device(struct pci_dev *dev, int mask)
+int pcibios_enable_resources(struct pci_dev *dev, int mask)
 {
 	u16 cmd, old_cmd;
 	int idx;
@@ -697,6 +697,11 @@
 	return 0;
 }
 
+int pcibios_enable_device(struct pci_dev *dev, int mask)
+{
+	return pcibios_enable_resources(dev, mask);
+}
+
 int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma,
 			enum pci_mmap_state mmap_state, int write_combine)
 {
Index: work6/arch/parisc/kernel/pci.c
===================================================================
--- work6.orig/arch/parisc/kernel/pci.c	2008-02-18 10:43:50.000000000 -0700
+++ work6/arch/parisc/kernel/pci.c	2008-02-18 10:45:14.000000000 -0700
@@ -285,7 +285,7 @@
  * Drivers that do not need parity (eg graphics and possibly networking)
  * can clear these bits if they want.
  */
-int pcibios_enable_device(struct pci_dev *dev, int mask)
+int pcibios_enable_resources(struct pci_dev *dev, int mask)
 {
 	u16 cmd;
 	int idx;
@@ -317,6 +317,10 @@
 	return 0;
 }
 
+int pcibios_enable_device(struct pci_dev *dev, int mask)
+{
+	return pcibios_enable_resources(dev, mask);
+}
 
 /* PA-RISC specific */
 void pcibios_register_hba(struct pci_hba_data *hba)
Index: work6/arch/powerpc/kernel/pci-common.c
===================================================================
--- work6.orig/arch/powerpc/kernel/pci-common.c	2008-02-18 10:43:50.000000000 -0700
+++ work6/arch/powerpc/kernel/pci-common.c	2008-02-18 10:45:14.000000000 -0700
@@ -1153,16 +1153,12 @@
 EXPORT_SYMBOL_GPL(pcibios_claim_one_bus);
 #endif /* CONFIG_HOTPLUG */
 
-int pcibios_enable_device(struct pci_dev *dev, int mask)
+int pcibios_enable_resources(struct pci_dev *dev, int mask)
 {
 	u16 cmd, old_cmd;
 	int idx;
 	struct resource *r;
 
-	if (ppc_md.pcibios_enable_device_hook)
-		if (ppc_md.pcibios_enable_device_hook(dev))
-			return -EINVAL;
-
 	pci_read_config_word(dev, PCI_COMMAND, &cmd);
 	old_cmd = cmd;
 	for (idx = 0; idx < PCI_NUM_RESOURCES; idx++) {
@@ -1193,3 +1189,11 @@
 	return 0;
 }
 
+int pcibios_enable_device(struct pci_dev *dev, int mask)
+{
+	if (ppc_md.pcibios_enable_device_hook)
+		if (ppc_md.pcibios_enable_device_hook(dev))
+			return -EINVAL;
+
+	return pcibios_enable_resources(dev, mask);
+}
Index: work6/arch/sh/drivers/pci/pci.c
===================================================================
--- work6.orig/arch/sh/drivers/pci/pci.c	2008-02-18 10:43:50.000000000 -0700
+++ work6/arch/sh/drivers/pci/pci.c	2008-02-18 10:45:14.000000000 -0700
@@ -131,7 +131,7 @@
 	}
 }
 
-int pcibios_enable_device(struct pci_dev *dev, int mask)
+int pcibios_enable_resources(struct pci_dev *dev, int mask)
 {
 	u16 cmd, old_cmd;
 	int idx;
@@ -163,6 +163,11 @@
 	return 0;
 }
 
+int pcibios_enable_device(struct pci_dev *dev, int mask)
+{
+	return pcibios_enable_resources(dev, mask);
+}
+
 /*
  *  If we set up a device for bus mastering, we need to check and set
  *  the latency timer as it may not be properly set.
Index: work6/arch/sparc64/kernel/pci.c
===================================================================
--- work6.orig/arch/sparc64/kernel/pci.c	2008-02-18 10:43:50.000000000 -0700
+++ work6/arch/sparc64/kernel/pci.c	2008-02-18 10:45:14.000000000 -0700
@@ -946,7 +946,7 @@
 {
 }
 
-int pcibios_enable_device(struct pci_dev *dev, int mask)
+int pcibios_enable_resources(struct pci_dev *dev, int mask)
 {
 	u16 cmd, oldcmd;
 	int i;
@@ -976,6 +976,11 @@
 	return 0;
 }
 
+int pcibios_enable_device(struct pci_dev *dev, int mask)
+{
+	return pcibios_enable_resources(dev, mask);
+}
+
 void pcibios_resource_to_bus(struct pci_dev *pdev, struct pci_bus_region *region,
 			     struct resource *res)
 {
Index: work6/arch/v850/kernel/rte_mb_a_pci.c
===================================================================
--- work6.orig/arch/v850/kernel/rte_mb_a_pci.c	2008-02-18 10:43:50.000000000 -0700
+++ work6/arch/v850/kernel/rte_mb_a_pci.c	2008-02-18 10:45:14.000000000 -0700
@@ -217,7 +217,7 @@
 }
 
 \f
-int __nomods_init pcibios_enable_device (struct pci_dev *dev, int mask)
+int __nomods_init pcibios_enable_resources (struct pci_dev *dev, int mask)
 {
 	u16 cmd, old_cmd;
 	int idx;
@@ -245,6 +245,11 @@
 	return 0;
 }
 
+int __nomods_init pcibios_enable_device (struct pci_dev *dev, int mask)
+{
+	return pcibios_enable_resources(dev, mask);
+}
+
 \f
 /* Resource allocation.  */
 static void __devinit pcibios_assign_resources (void)

-- 

[patch 2/6] ppc: make pcibios_enable_device() use pcibios_enable_resources()

[Bjorn Helgaas]

pcibios_enable_device() has an almost verbatim copy of
pcibios_enable_resources(), (the only difference is that
pcibios_enable_resources() turns on PCI_COMMAND_MEMORY if
there's a ROM resource).

The duplication might be intentional, but I don't see any callers
of pcibios_enable_resources() on ppc, so I think it's more
likely a historical accident.

This patch removes the duplication, making pcibios_enable_device()
simply call pcibios_enable_resources() as x86 does.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Index: work6/arch/ppc/kernel/pci.c
===================================================================
--- work6.orig/arch/ppc/kernel/pci.c	2008-02-18 10:43:50.000000000 -0700
+++ work6/arch/ppc/kernel/pci.c	2008-02-18 11:31:23.000000000 -0700
@@ -785,33 +785,11 @@
 
 int pcibios_enable_device(struct pci_dev *dev, int mask)
 {
-	u16 cmd, old_cmd;
-	int idx;
-	struct resource *r;
-
 	if (ppc_md.pcibios_enable_device_hook)
 		if (ppc_md.pcibios_enable_device_hook(dev, 0))
 			return -EINVAL;
-		
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-	old_cmd = cmd;
-	for (idx=0; idx<6; idx++) {
-		r = &dev->resource[idx];
-		if (r->flags & IORESOURCE_UNSET) {
-			printk(KERN_ERR "PCI: Device %s not available because of resource collisions\n", pci_name(dev));
-			return -EINVAL;
-		}
-		if (r->flags & IORESOURCE_IO)
-			cmd |= PCI_COMMAND_IO;
-		if (r->flags & IORESOURCE_MEM)
-			cmd |= PCI_COMMAND_MEMORY;
-	}
-	if (cmd != old_cmd) {
-		printk("PCI: Enabling device %s (%04x -> %04x)\n",
-		       pci_name(dev), old_cmd, cmd);
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-	}
-	return 0;
+
+	return pcibios_enable_resources(dev, mask);
 }
 
 struct pci_controller*

-- 

[patch 3/6] xtensa: make pcibios_enable_device() use pcibios_enable_resources()

[Bjorn Helgaas]

pcibios_enable_device() has an almost verbatim copy of
pcibios_enable_resources(), (the only difference is that
pcibios_enable_resources() turns on PCI_COMMAND_MEMORY if
there's a ROM resource).

The duplication might be intentional, but I don't see any callers
of pcibios_enable_resources() on xtensa, so I think it's more
likely a historical accident copied from ppc.

This patch removes the duplication, making pcibios_enable_device()
simply call pcibios_enable_resources() as x86 does.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: work6/arch/xtensa/kernel/pci.c
===================================================================
--- work6.orig/arch/xtensa/kernel/pci.c	2008-02-18 10:43:50.000000000 -0700
+++ work6/arch/xtensa/kernel/pci.c	2008-02-18 11:32:12.000000000 -0700
@@ -238,31 +238,7 @@
 
 int pcibios_enable_device(struct pci_dev *dev, int mask)
 {
-	u16 cmd, old_cmd;
-	int idx;
-	struct resource *r;
-
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-	old_cmd = cmd;
-	for (idx=0; idx<6; idx++) {
-		r = &dev->resource[idx];
-		if (!r->start && r->end) {
-			printk(KERN_ERR "PCI: Device %s not available because "
-			       "of resource collisions\n", pci_name(dev));
-			return -EINVAL;
-		}
-		if (r->flags & IORESOURCE_IO)
-			cmd |= PCI_COMMAND_IO;
-		if (r->flags & IORESOURCE_MEM)
-			cmd |= PCI_COMMAND_MEMORY;
-	}
-	if (cmd != old_cmd) {
-		printk("PCI: Enabling device %s (%04x -> %04x)\n",
-		       pci_name(dev), old_cmd, cmd);
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-	}
-
-	return 0;
+	return pcibios_enable_resources(dev, mask);
 }
 
 #ifdef CONFIG_PROC_FS

-- 

[patch 4/6] ARM: move bridge enable out of pcibios_enable_resources()

[Bjorn Helgaas]

Move bridge enable from pcibios_enable_resources() to
platform_pci_enable_device() so the former matches other
architectures and can be shared.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: work6/arch/arm/kernel/bios32.c
===================================================================
--- work6.orig/arch/arm/kernel/bios32.c	2008-02-27 11:25:29.000000000 -0700
+++ work6/arch/arm/kernel/bios32.c	2008-02-27 11:55:59.000000000 -0700
@@ -683,15 +683,32 @@
 			cmd |= PCI_COMMAND_MEMORY;
 	}
 
+	if (cmd != old_cmd) {
+		printk("PCI: enabling device %s (%04x -> %04x)\n",
+		       pci_name(dev), old_cmd, cmd);
+		pci_write_config_word(dev, PCI_COMMAND, cmd);
+	}
+	return 0;
+}
+
+static int platform_pci_enable_device(struct pci_dev *dev)
+{
+	u16 cmd, old_cmd;
+
+	pci_read_config_word(dev, PCI_COMMAND, &cmd);
+	old_cmd = cmd;
+
 	/*
-	 * Bridges (eg, cardbus bridges) need to be fully enabled
+	 * Bridges (eg, cardbus bridges) need to be fully enabled.
+	 * Most architectures do this in pci_enable_bridges(), not
+	 * in the pci_enable_device() path.
 	 */
 	if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE)
 		cmd |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY;
 
 	if (cmd != old_cmd) {
-		printk("PCI: enabling device %s (%04x -> %04x)\n",
-		       pci_name(dev), old_cmd, cmd);
+		dev_info(&dev->dev, "enabling bridge device (%04x -> %04x)\n",
+		       old_cmd, cmd);
 		pci_write_config_word(dev, PCI_COMMAND, cmd);
 	}
 	return 0;
@@ -699,7 +716,12 @@
 
 int pcibios_enable_device(struct pci_dev *dev, int mask)
 {
-	return pcibios_enable_resources(dev, mask);
+	int err;
+
+	if ((err = pcibios_enable_resources(dev, mask)) < 0)
+		return err;
+
+	return platform_pci_enable_device(dev);
 }
 
 int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma,

-- 

Re: [patch 4/6] ARM: move bridge enable out of pcibios_enable_resources()

[Jesse Barnes]

On Wednesday, February 27, 2008 4:04 pm Bjorn Helgaas wrote:
> Move bridge enable from pcibios_enable_resources() to
> platform_pci_enable_device() so the former matches other
> architectures and can be shared.

I really like the direction of these patches.  Getting PCI resources assigned 
& devices setup correctly for new arches has always been a bit more trouble 
than it should be...

>
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
>
> Index: work6/arch/arm/kernel/bios32.c
> ===================================================================
> --- work6.orig/arch/arm/kernel/bios32.c	2008-02-27 11:25:29.000000000 -0700
> +++ work6/arch/arm/kernel/bios32.c	2008-02-27 11:55:59.000000000 -0700
> @@ -683,15 +683,32 @@
>  			cmd |= PCI_COMMAND_MEMORY;
>  	}
>
> +	if (cmd != old_cmd) {
> +		printk("PCI: enabling device %s (%04x -> %04x)\n",
> +		       pci_name(dev), old_cmd, cmd);

Probably worth giving this printk a prefix at some point (doesn't matter for 
this patchset though since you're just moving it around).

Rest of it looks good.

Jesse

Re: [patch 4/6] ARM: move bridge enable out of pcibios_enable_resources()

[Benjamin Herrenschmidt]

On Mon, 2008-03-03 at 09:59 -0800, Jesse Barnes wrote:
> On Wednesday, February 27, 2008 4:04 pm Bjorn Helgaas wrote:
> > Move bridge enable from pcibios_enable_resources() to
> > platform_pci_enable_device() so the former matches other
> > architectures and can be shared.
> 
> I really like the direction of these patches.  Getting PCI resources assigned 
> & devices setup correctly for new arches has always been a bit more trouble 
> than it should be...

You'll noticed that I recently moved powerpc to something more common to
x86 in the are of resource allocation. Still -slightly- different but I
do believe there is room for somebody with some skills to try to turn
some of that into generic code.

Ben.

Re: [patch 4/6] ARM: move bridge enable out of pcibios_enable_resources()

[Jesse Barnes]

On Monday, March 03, 2008 12:35 pm Benjamin Herrenschmidt wrote:
> On Mon, 2008-03-03 at 09:59 -0800, Jesse Barnes wrote:
> > On Wednesday, February 27, 2008 4:04 pm Bjorn Helgaas wrote:
> > > Move bridge enable from pcibios_enable_resources() to
> > > platform_pci_enable_device() so the former matches other
> > > architectures and can be shared.
> >
> > I really like the direction of these patches.  Getting PCI resources
> > assigned & devices setup correctly for new arches has always been a bit
> > more trouble than it should be...
>
> You'll noticed that I recently moved powerpc to something more common to
> x86 in the are of resource allocation. Still -slightly- different but I
> do believe there is room for somebody with some skills to try to turn
> some of that into generic code.

Yeah, I think that would be a good thing to shoot for.  Even on PCs there are 
times when we need resource allocation to be done (or re-done) by the kernel 
for hotplug or just because the platform is pared down enough that it doesn't 
to it all by itself.

I might be able to find time to look into that myself in the next few weeks; I 
think we even have some open PCI bugs that could be solved by better resource 
allocation.

Jesse

Re: [patch 4/6] ARM: move bridge enable out of pcibios_enable_resources()

[Russell King - ARM Linux]

On Wed, Feb 27, 2008 at 05:04:41PM -0700, Bjorn Helgaas wrote:
> Move bridge enable from pcibios_enable_resources() to
> platform_pci_enable_device() so the former matches other
> architectures and can be shared.
> 
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> 
> Index: work6/arch/arm/kernel/bios32.c
> ===================================================================
> --- work6.orig/arch/arm/kernel/bios32.c	2008-02-27 11:25:29.000000000 -0700
> +++ work6/arch/arm/kernel/bios32.c	2008-02-27 11:55:59.000000000 -0700
> @@ -683,15 +683,32 @@
>  			cmd |= PCI_COMMAND_MEMORY;
>  	}
>  
> +	if (cmd != old_cmd) {
> +		printk("PCI: enabling device %s (%04x -> %04x)\n",
> +		       pci_name(dev), old_cmd, cmd);

Should be dev_info().

Apart from that...

Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>

Re: [patch 4/6] ARM: move bridge enable out of pcibios_enable_resources()

[Bjorn Helgaas]

On Thursday 06 March 2008 08:06:40 am Russell King - ARM Linux wrote:
> On Wed, Feb 27, 2008 at 05:04:41PM -0700, Bjorn Helgaas wrote:
> > Move bridge enable from pcibios_enable_resources() to
> > platform_pci_enable_device() so the former matches other
> > architectures and can be shared.
> > 
> > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> > 
> > Index: work6/arch/arm/kernel/bios32.c
> > ===================================================================
> > --- work6.orig/arch/arm/kernel/bios32.c	2008-02-27 11:25:29.000000000 -0700
> > +++ work6/arch/arm/kernel/bios32.c	2008-02-27 11:55:59.000000000 -0700
> > @@ -683,15 +683,32 @@
> >  			cmd |= PCI_COMMAND_MEMORY;
> >  	}
> >  
> > +	if (cmd != old_cmd) {
> > +		printk("PCI: enabling device %s (%04x -> %04x)\n",
> > +		       pci_name(dev), old_cmd, cmd);
> 
> Should be dev_info().
> 
> Apart from that...
> 
> Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>

I fixed that in the v3 patch that Greg KH put in his tree.

Thanks,
  Bjorn

[patch 5/6] PARISC: move PERR & SERR enables out of pcibios_enable_resources()

[Bjorn Helgaas]

Move PERR and SERR enables from pcibios_enable_resources() to
platform_pci_enable_device() so the former matches other
architectures and can be shared.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: work6/arch/parisc/kernel/pci.c
===================================================================
--- work6.orig/arch/parisc/kernel/pci.c	2008-02-27 11:30:02.000000000 -0700
+++ work6/arch/parisc/kernel/pci.c	2008-02-27 11:38:11.000000000 -0700
@@ -281,9 +281,7 @@
  * A driver is enabling the device.  We make sure that all the appropriate
  * bits are set to allow the device to operate as the driver is expecting.
  * We enable the port IO and memory IO bits if the device has any BARs of
- * that type, and we enable the PERR and SERR bits unconditionally.
- * Drivers that do not need parity (eg graphics and possibly networking)
- * can clear these bits if they want.
+ * that type.
  */
 int pcibios_enable_resources(struct pci_dev *dev, int mask)
 {
@@ -305,8 +303,6 @@
 			cmd |= PCI_COMMAND_MEMORY;
 	}
 
-	cmd |= (PCI_COMMAND_SERR | PCI_COMMAND_PARITY);
-
 #if 0
 	/* If bridge/bus controller has FBB enabled, child must too. */
 	if (dev->bus->bridge_ctl & PCI_BRIDGE_CTL_FAST_BACK)
@@ -317,9 +313,38 @@
 	return 0;
 }
 
+/*
+ * A driver is enabling the device.  We enable the PERR and SERR bits
+ * unconditionally.  Drivers that do not need parity (eg graphics and
+ * possibly networking) can clear these bits if they want.
+ */
+static int platform_pci_enable_device(struct pci_dev *dev)
+{
+	u16 cmd, old_cmd;
+	int idx;
+
+	pci_read_config_word(dev, PCI_COMMAND, &cmd);
+	old_cmd = cmd;
+
+	cmd |= (PCI_COMMAND_SERR | PCI_COMMAND_PARITY);
+
+	if (cmd != old_cmd) {
+		dev_info(&dev->dev, "enabling SERR and PARITY (%04x -> %04x)\n",
+			old_cmd, cmd);
+		pci_write_config_word(dev, PCI_COMMAND, cmd);
+	}
+
+	return 0;
+}
+
 int pcibios_enable_device(struct pci_dev *dev, int mask)
 {
-	return pcibios_enable_resources(dev, mask);
+	int err;
+
+	if ((err = pcibios_enable_resources(dev, mask)) < 0)
+		return err;
+
+	return platform_pci_enable_device(dev);
 }
 
 /* PA-RISC specific */

-- 

Re: [patch 5/6] PARISC: move PERR & SERR enables out of pcibios_enable_resources()

[Grant Grundler]

On Wed, Feb 27, 2008 at 05:04:42PM -0700, Bjorn Helgaas wrote:
> Move PERR and SERR enables from pcibios_enable_resources() to
> platform_pci_enable_device() so the former matches other
> architectures and can be shared.
> 
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Ack-By: Grant Grundler <grundler@parisc-linux.org>

This patch sequence is heading in the right direction.
I've not tested this particular one yet but I'm pretty sure it's ok.
I'll fixup any breakage for parisc.

...
> +/*
> + * A driver is enabling the device.  We enable the PERR and SERR bits
> + * unconditionally.  Drivers that do not need parity (eg graphics and
> + * possibly networking) can clear these bits if they want.
> + */
> +static int platform_pci_enable_device(struct pci_dev *dev)

Thanks for preserving this comment.

In general, I'm wondering if the check for device class would be
sufficient here to NOT enable PERR/SERR for graphics automatically.
While disabling PERR was "the right thing" for older "mostly write"
devices of the 1990's and early 2000, it might not be correct for
current 3-D graphics devices which use host mem to buffer processed
results. I'm thinking of Intel graphics controllers in particular
but I don't know any details of how they actually work.

I'm also a bit concerned about this now becuase (IIRC) AGP didn't
implement parity though it looked like PCI protocol. PCI-e certainly
does but it's possible BIOS/Firmware disable parity generation
on the host bridge when connected to a gfx device.
We wouldn't want to enable parity checking on a PCI-e gfx device in this
case and I hope someone (perhaps at Intel) could double check this.

thanks,
grant

Re: [patch 5/6] PARISC: move PERR & SERR enables out of pcibios_enable_resources()

[Jesse Barnes]

On Thursday, February 28, 2008 9:31 am Grant Grundler wrote:
> In general, I'm wondering if the check for device class would be
> sufficient here to NOT enable PERR/SERR for graphics automatically.
> While disabling PERR was "the right thing" for older "mostly write"
> devices of the 1990's and early 2000, it might not be correct for
> current 3-D graphics devices which use host mem to buffer processed
> results. I'm thinking of Intel graphics controllers in particular
> but I don't know any details of how they actually work.

Well, in general chipset devices aren't required to support parity checking, 
AIUI; Intel gfx devices don't bother (PERR enable is hardwired to 0).

> I'm also a bit concerned about this now becuase (IIRC) AGP didn't
> implement parity though it looked like PCI protocol. PCI-e certainly
> does but it's possible BIOS/Firmware disable parity generation
> on the host bridge when connected to a gfx device.
> We wouldn't want to enable parity checking on a PCI-e gfx device in this
> case and I hope someone (perhaps at Intel) could double check this.

I'd have to ping our BIOS folks to see if that's the case, but I doubt it.  It 
would be a bad idea to disable any PCIe error reporting (including legacy 
error mapping) just because a gfx device was attached.  Apparently the AMD 
PCIe parts include PERR generation, so disabling upstream reporting at boot 
time seems like it would be an outright bug; it should be left up to driver & 
OS software.

Jesse

Re: [patch 5/6] PARISC: move PERR & SERR enables out of pcibios_enable_resources()

[Kyle McMartin]

On Wed, Feb 27, 2008 at 05:04:42PM -0700, Bjorn Helgaas wrote:
> Move PERR and SERR enables from pcibios_enable_resources() to
> platform_pci_enable_device() so the former matches other
> architectures and can be shared.
> 

I don't have any problems with this, but I think the naming needs to
change. pcibios_* namespace should probably remain arch dependent.
Renaming the unified implementation to pci_enable_resources, and adding
a weak function pcibios_enable_resources that can be overridden by
parisc and arm to enable PERR/SERR after calling the generic
pci_enable_resources function. No?

regards, Kyle

[patch 6/6] PCI: consolidate several pcibios_enable_resources() implementations

[Bjorn Helgaas]

There are many implementations of pcibios_enable_resources() that differ
in minor ways that look more like bugs than architectural differences.

This patch consolidates most of them to use the version annotated below.
This is the original x86 version, except that it uses the resource
collision check from powerpc at (5):

    int pcibios_enable_resources(struct pci_dev *dev, int mask)
    {
	u16 cmd, old_cmd;
	int i;
	struct resource *r;

  (0)
	pci_read_config_word(dev, PCI_COMMAND, &cmd);
	old_cmd = cmd;

  (1)	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
  (2)		if (!(mask & (1 << i)))
			continue;

		r = &dev->resource[i];

  (3)		if (!(r->flags & (IORESOURCE_IO | IORESOURCE_MEM)))
			continue;

  (4)		if ((i == PCI_ROM_RESOURCE) &&
				(!(r->flags & IORESOURCE_ROM_ENABLE)))
			continue;

  (5)		if (!r->parent) {
			dev_err(&dev->dev, "device not available because of "
				"BAR %d [%llx:%llx] collisions\n", i,
				(unsigned long long) r->start,
				(unsigned long long) r->end);
			return -EINVAL;
		}

		if (r->flags & IORESOURCE_IO)
			cmd |= PCI_COMMAND_IO;
		if (r->flags & IORESOURCE_MEM)
			cmd |= PCI_COMMAND_MEMORY;
	}

  (6)
  (7)	if (cmd != old_cmd) {
		dev_info(&dev->dev, "enabling device (%04x -> %04x)\n",
			 old_cmd, cmd);
		pci_write_config_word(dev, PCI_COMMAND, cmd);
	}
	return 0;
    }

Compared with the above version, other architectures have the following
functional differences:

    alpha: ignores mask at (2), has no PCI_ROM_RESOURCE check at (4),
	has no collision check at (5)

    arm: checks only 6 resources at (1), has no PCI_ROM_RESOURCE check at (4), 
	always fully enables bridges at (6)

    cris: checks only 6 resources at (1), has a different ROM
	resource check at (4) and (6) that ignores IORESOURCE_ROM_ENABLE

    frv: checks only 6 resources at (1), has a different ROM
	resource check at (4) and (6) that ignores IORESOURCE_ROM_ENABLE

    ia64: checks for NULL dev at (0)

    mips: has no IORESOURCE_{IO,MEM} check at (3), has a different
	ROM resource check at (4) and (6) that ignores IORESOURCE_ROM_ENABLE

    mn10300: checks only 6 resources at (1), has no IORESOURCE_{IO,MEM}
	check at (3), has a different ROM resource check at (4) and (6)
	that ignores IORESOURCE_ROM_ENABLE

    parisc: checks DEVICE_COUNT_RESOURCE (12) instead of PCI_NUM_RESOURCES (11)
	resources at (1), has no IORESOURCE_{IO,MEM} check at (3),
	has no PCI_ROM_RESOURCE check at (4), has no collision check at (5)
	always turns on PCI_COMMAND_SERR | PCI_COMMAND_PARITY at (6),
	writes cmd even if unchanged at (7)

    powerpc: has a different collision check at (5)

    ppc: checks only 6 resources at (1), has no IORESOURCE_{IO,MEM} check
	at (3), has a different ROM resource check at (4) and (6) that
	ignores IORESOURCE_ROM_ENABLE, has a different collision check using
	IORESOURCE_UNSET at (5)

    sh: checks only 6 resources at (1), has no IORESOURCE_{IO,MEM} check
	at (3), has a different ROM resource check at (4) and (6) that
	ignores IORESOURCE_ROM_ENABLE

    sparc64: has no IORESOURCE_{IO,MEM} check at (3), has no PCI_ROM_RESOURCE
	check at (4)

    v850: checks only 6 resources at (1), has no IORESOURCE_{IO,MEM} check
	at (3), has no PCI_ROM_RESOURCE check at (4)

    xtensa: checks only 6 resources at (1), has no IORESOURCE_{IO,MEM} check
	at (3), has a different ROM resource check at (4) and (6) that
	ignores IORESOURCE_ROM_ENABLE

    x86: has a different collision check at (5)

The mips/pmc-sierra implementation of pcibios_enable_resources() is
cluttered with a bunch of titan stuff, so I can't immediately consolidate
it with the others.  So I made the generic version "weak" so pmc-sierra
can override it.

Not-Yet-Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

---
 arch/alpha/kernel/pci.c                 |   27 --------------------
 arch/arm/kernel/bios32.c                |   37 ----------------------------
 arch/cris/arch-v32/drivers/pci/bios.c   |   32 ------------------------
 arch/frv/mb93090-mb00/pci-frv.c         |   32 ------------------------
 arch/ia64/pci/pci.c                     |   42 --------------------------------
 arch/mips/pci/pci.c                     |   32 ------------------------
 arch/mn10300/unit-asb2305/pci-asb2305.c |   39 -----------------------------
 arch/parisc/kernel/pci.c                |   37 ----------------------------
 arch/powerpc/kernel/pci-common.c        |   36 ---------------------------
 arch/ppc/kernel/pci.c                   |   33 -------------------------
 arch/sh/drivers/pci/pci.c               |   32 ------------------------
 arch/sparc64/kernel/pci.c               |   30 ----------------------
 arch/v850/kernel/rte_mb_a_pci.c         |   28 ---------------------
 arch/x86/pci/i386.c                     |   38 ----------------------------
 arch/x86/pci/pci.h                      |    1 
 arch/xtensa/kernel/pci.c                |   31 -----------------------
 drivers/pci/setup-res.c                 |   42 ++++++++++++++++++++++++++++++++
 include/linux/pci.h                     |    1 
 18 files changed, 43 insertions(+), 507 deletions(-)

Index: work6/arch/alpha/kernel/pci.c
===================================================================
--- work6.orig/arch/alpha/kernel/pci.c	2008-02-27 14:37:14.000000000 -0700
+++ work6/arch/alpha/kernel/pci.c	2008-02-27 14:40:34.000000000 -0700
@@ -370,33 +370,6 @@
 #endif
 
 int
-pcibios_enable_resources(struct pci_dev *dev, int mask)
-{
-	u16 cmd, oldcmd;
-	int i;
-
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-	oldcmd = cmd;
-
-	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-		struct resource *res = &dev->resource[i];
-
-		if (res->flags & IORESOURCE_IO)
-			cmd |= PCI_COMMAND_IO;
-		else if (res->flags & IORESOURCE_MEM)
-			cmd |= PCI_COMMAND_MEMORY;
-	}
-
-	if (cmd != oldcmd) {
-		printk(KERN_DEBUG "PCI: Enabling device: (%s), cmd %x\n",
-		       pci_name(dev), cmd);
-		/* Enable the appropriate bits in the PCI command register.  */
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-	}
-	return 0;
-}
-
-int
 pcibios_enable_device(struct pci_dev *dev, int mask)
 {
 	return pcibios_enable_resources(dev, mask);
Index: work6/arch/arm/kernel/bios32.c
===================================================================
--- work6.orig/arch/arm/kernel/bios32.c	2008-02-27 14:37:14.000000000 -0700
+++ work6/arch/arm/kernel/bios32.c	2008-02-27 14:40:34.000000000 -0700
@@ -654,43 +654,6 @@
 	res->start = (start + align - 1) & ~(align - 1);
 }
 
-/**
- * pcibios_enable_resources - Enable I/O and memory.
- * @dev: PCI device to be enabled
- */
-int pcibios_enable_resources(struct pci_dev *dev, int mask)
-{
-	u16 cmd, old_cmd;
-	int idx;
-	struct resource *r;
-
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-	old_cmd = cmd;
-	for (idx = 0; idx < 6; idx++) {
-		/* Only set up the requested stuff */
-		if (!(mask & (1 << idx)))
-			continue;
-
-		r = dev->resource + idx;
-		if (!r->start && r->end) {
-			printk(KERN_ERR "PCI: Device %s not available because"
-			       " of resource collisions\n", pci_name(dev));
-			return -EINVAL;
-		}
-		if (r->flags & IORESOURCE_IO)
-			cmd |= PCI_COMMAND_IO;
-		if (r->flags & IORESOURCE_MEM)
-			cmd |= PCI_COMMAND_MEMORY;
-	}
-
-	if (cmd != old_cmd) {
-		printk("PCI: enabling device %s (%04x -> %04x)\n",
-		       pci_name(dev), old_cmd, cmd);
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-	}
-	return 0;
-}
-
 static int platform_pci_enable_device(struct pci_dev *dev)
 {
 	u16 cmd, old_cmd;
Index: work6/arch/cris/arch-v32/drivers/pci/bios.c
===================================================================
--- work6.orig/arch/cris/arch-v32/drivers/pci/bios.c	2008-02-27 14:37:14.000000000 -0700
+++ work6/arch/cris/arch-v32/drivers/pci/bios.c	2008-02-27 14:40:34.000000000 -0700
@@ -55,38 +55,6 @@
 	}
 }
 
-int pcibios_enable_resources(struct pci_dev *dev, int mask)
-{
-	u16 cmd, old_cmd;
-	int idx;
-	struct resource *r;
-
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-	old_cmd = cmd;
-	for(idx=0; idx<6; idx++) {
-		/* Only set up the requested stuff */
-		if (!(mask & (1<<idx)))
-			continue;
-
-		r = &dev->resource[idx];
-		if (!r->start && r->end) {
-			printk(KERN_ERR "PCI: Device %s not available because of resource collisions\n", pci_name(dev));
-			return -EINVAL;
-		}
-		if (r->flags & IORESOURCE_IO)
-			cmd |= PCI_COMMAND_IO;
-		if (r->flags & IORESOURCE_MEM)
-			cmd |= PCI_COMMAND_MEMORY;
-	}
-	if (dev->resource[PCI_ROM_RESOURCE].start)
-		cmd |= PCI_COMMAND_MEMORY;
-	if (cmd != old_cmd) {
-		printk("PCI: Enabling device %s (%04x -> %04x)\n", pci_name(dev), old_cmd, cmd);
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-	}
-	return 0;
-}
-
 int pcibios_enable_irq(struct pci_dev *dev)
 {
 	dev->irq = EXT_INTR_VECT;
Index: work6/arch/frv/mb93090-mb00/pci-frv.c
===================================================================
--- work6.orig/arch/frv/mb93090-mb00/pci-frv.c	2008-02-27 14:37:14.000000000 -0700
+++ work6/arch/frv/mb93090-mb00/pci-frv.c	2008-02-27 14:40:34.000000000 -0700
@@ -231,38 +231,6 @@
 	pcibios_assign_resources();
 }
 
-int pcibios_enable_resources(struct pci_dev *dev, int mask)
-{
-	u16 cmd, old_cmd;
-	int idx;
-	struct resource *r;
-
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-	old_cmd = cmd;
-	for(idx=0; idx<6; idx++) {
-		/* Only set up the requested stuff */
-		if (!(mask & (1<<idx)))
-			continue;
-
-		r = &dev->resource[idx];
-		if (!r->start && r->end) {
-			printk(KERN_ERR "PCI: Device %s not available because of resource collisions\n", pci_name(dev));
-			return -EINVAL;
-		}
-		if (r->flags & IORESOURCE_IO)
-			cmd |= PCI_COMMAND_IO;
-		if (r->flags & IORESOURCE_MEM)
-			cmd |= PCI_COMMAND_MEMORY;
-	}
-	if (dev->resource[PCI_ROM_RESOURCE].start)
-		cmd |= PCI_COMMAND_MEMORY;
-	if (cmd != old_cmd) {
-		printk("PCI: Enabling device %s (%04x -> %04x)\n", pci_name(dev), old_cmd, cmd);
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-	}
-	return 0;
-}
-
 /*
  *  If we set up a device for bus mastering, we need to check the latency
  *  timer as certain crappy BIOSes forget to set it properly.
Index: work6/arch/ia64/pci/pci.c
===================================================================
--- work6.orig/arch/ia64/pci/pci.c	2008-02-27 14:37:14.000000000 -0700
+++ work6/arch/ia64/pci/pci.c	2008-02-27 14:40:34.000000000 -0700
@@ -499,48 +499,6 @@
 	/* ??? FIXME -- record old value for shutdown.  */
 }
 
-static inline int
-pcibios_enable_resources (struct pci_dev *dev, int mask)
-{
-	u16 cmd, old_cmd;
-	int idx;
-	struct resource *r;
-	unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM;
-
-	if (!dev)
-		return -EINVAL;
-
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-	old_cmd = cmd;
-	for (idx=0; idx<PCI_NUM_RESOURCES; idx++) {
-		/* Only set up the desired resources.  */
-		if (!(mask & (1 << idx)))
-			continue;
-
-		r = &dev->resource[idx];
-		if (!(r->flags & type_mask))
-			continue;
-		if ((idx == PCI_ROM_RESOURCE) &&
-				(!(r->flags & IORESOURCE_ROM_ENABLE)))
-			continue;
-		if (!r->start && r->end) {
-			printk(KERN_ERR
-			       "PCI: Device %s not available because of resource collisions\n",
-			       pci_name(dev));
-			return -EINVAL;
-		}
-		if (r->flags & IORESOURCE_IO)
-			cmd |= PCI_COMMAND_IO;
-		if (r->flags & IORESOURCE_MEM)
-			cmd |= PCI_COMMAND_MEMORY;
-	}
-	if (cmd != old_cmd) {
-		printk("PCI: Enabling device %s (%04x -> %04x)\n", pci_name(dev), old_cmd, cmd);
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-	}
-	return 0;
-}
-
 int
 pcibios_enable_device (struct pci_dev *dev, int mask)
 {
Index: work6/arch/mips/pci/pci.c
===================================================================
--- work6.orig/arch/mips/pci/pci.c	2008-02-27 14:37:14.000000000 -0700
+++ work6/arch/mips/pci/pci.c	2008-02-27 14:40:34.000000000 -0700
@@ -163,38 +163,6 @@
 
 subsys_initcall(pcibios_init);
 
-static int pcibios_enable_resources(struct pci_dev *dev, int mask)
-{
-	u16 cmd, old_cmd;
-	int idx;
-	struct resource *r;
-
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-	old_cmd = cmd;
-	for (idx=0; idx < PCI_NUM_RESOURCES; idx++) {
-		/* Only set up the requested stuff */
-		if (!(mask & (1<<idx)))
-			continue;
-
-		r = &dev->resource[idx];
-		if (!r->start && r->end) {
-			printk(KERN_ERR "PCI: Device %s not available because of resource collisions\n", pci_name(dev));
-			return -EINVAL;
-		}
-		if (r->flags & IORESOURCE_IO)
-			cmd |= PCI_COMMAND_IO;
-		if (r->flags & IORESOURCE_MEM)
-			cmd |= PCI_COMMAND_MEMORY;
-	}
-	if (dev->resource[PCI_ROM_RESOURCE].start)
-		cmd |= PCI_COMMAND_MEMORY;
-	if (cmd != old_cmd) {
-		printk("PCI: Enabling device %s (%04x -> %04x)\n", pci_name(dev), old_cmd, cmd);
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-	}
-	return 0;
-}
-
 /*
  *  If we set up a device for bus mastering, we need to check the latency
  *  timer as certain crappy BIOSes forget to set it properly.
Index: work6/arch/mn10300/unit-asb2305/pci-asb2305.c
===================================================================
--- work6.orig/arch/mn10300/unit-asb2305/pci-asb2305.c	2008-02-27 14:37:14.000000000 -0700
+++ work6/arch/mn10300/unit-asb2305/pci-asb2305.c	2008-02-27 14:40:34.000000000 -0700
@@ -218,45 +218,6 @@
 	pcibios_allocate_resources(1);
 }
 
-int pcibios_enable_resources(struct pci_dev *dev, int mask)
-{
-	u16 cmd, old_cmd;
-	int idx;
-	struct resource *r;
-
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-	old_cmd = cmd;
-
-	for (idx = 0; idx < 6; idx++) {
-		/* Only set up the requested stuff */
-		if (!(mask & (1 << idx)))
-			continue;
-
-		r = &dev->resource[idx];
-
-		if (!r->start && r->end) {
-			printk(KERN_ERR
-			       "PCI: Device %s not available because of"
-			       " resource collisions\n",
-			       pci_name(dev));
-			return -EINVAL;
-		}
-
-		if (r->flags & IORESOURCE_IO)
-			cmd |= PCI_COMMAND_IO;
-		if (r->flags & IORESOURCE_MEM)
-			cmd |= PCI_COMMAND_MEMORY;
-	}
-
-	if (dev->resource[PCI_ROM_RESOURCE].start)
-		cmd |= PCI_COMMAND_MEMORY;
-
-	if (cmd != old_cmd)
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-
-	return 0;
-}
-
 /*
  *  If we set up a device for bus mastering, we need to check the latency
  *  timer as certain crappy BIOSes forget to set it properly.
Index: work6/arch/parisc/kernel/pci.c
===================================================================
--- work6.orig/arch/parisc/kernel/pci.c	2008-02-27 14:37:14.000000000 -0700
+++ work6/arch/parisc/kernel/pci.c	2008-02-27 14:40:34.000000000 -0700
@@ -276,43 +276,6 @@
 	/* The caller updates the end field, we don't.  */
 }
 
-
-/*
- * A driver is enabling the device.  We make sure that all the appropriate
- * bits are set to allow the device to operate as the driver is expecting.
- * We enable the port IO and memory IO bits if the device has any BARs of
- * that type.
- */
-int pcibios_enable_resources(struct pci_dev *dev, int mask)
-{
-	u16 cmd;
-	int idx;
-
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-
-	for (idx = 0; idx < DEVICE_COUNT_RESOURCE; idx++) {
-		struct resource *r = &dev->resource[idx];
-
-		/* only setup requested resources */
-		if (!(mask & (1<<idx)))
-			continue;
-
-		if (r->flags & IORESOURCE_IO)
-			cmd |= PCI_COMMAND_IO;
-		if (r->flags & IORESOURCE_MEM)
-			cmd |= PCI_COMMAND_MEMORY;
-	}
-
-#if 0
-	/* If bridge/bus controller has FBB enabled, child must too. */
-	if (dev->bus->bridge_ctl & PCI_BRIDGE_CTL_FAST_BACK)
-		cmd |= PCI_COMMAND_FAST_BACK;
-#endif
-	DBGC("PCIBIOS: Enabling device %s cmd 0x%04x\n", pci_name(dev), cmd);
-	pci_write_config_word(dev, PCI_COMMAND, cmd);
-	return 0;
-}
-
 /*
  * A driver is enabling the device.  We enable the PERR and SERR bits
  * unconditionally.  Drivers that do not need parity (eg graphics and
Index: work6/arch/powerpc/kernel/pci-common.c
===================================================================
--- work6.orig/arch/powerpc/kernel/pci-common.c	2008-02-27 14:37:14.000000000 -0700
+++ work6/arch/powerpc/kernel/pci-common.c	2008-02-27 14:40:34.000000000 -0700
@@ -1153,42 +1153,6 @@
 EXPORT_SYMBOL_GPL(pcibios_claim_one_bus);
 #endif /* CONFIG_HOTPLUG */
 
-int pcibios_enable_resources(struct pci_dev *dev, int mask)
-{
-	u16 cmd, old_cmd;
-	int idx;
-	struct resource *r;
-
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-	old_cmd = cmd;
-	for (idx = 0; idx < PCI_NUM_RESOURCES; idx++) {
-		/* Only set up the requested stuff */
-		if (!(mask & (1 << idx)))
-			continue;
-		r = &dev->resource[idx];
-		if (!(r->flags & (IORESOURCE_IO | IORESOURCE_MEM)))
-			continue;
-		if ((idx == PCI_ROM_RESOURCE) &&
-				(!(r->flags & IORESOURCE_ROM_ENABLE)))
-			continue;
-		if (r->parent == NULL) {
-			printk(KERN_ERR "PCI: Device %s not available because"
-			       " of resource collisions\n", pci_name(dev));
-			return -EINVAL;
-		}
-		if (r->flags & IORESOURCE_IO)
-			cmd |= PCI_COMMAND_IO;
-		if (r->flags & IORESOURCE_MEM)
-			cmd |= PCI_COMMAND_MEMORY;
-	}
-	if (cmd != old_cmd) {
-		printk("PCI: Enabling device %s (%04x -> %04x)\n",
-		       pci_name(dev), old_cmd, cmd);
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-	}
-	return 0;
-}
-
 int pcibios_enable_device(struct pci_dev *dev, int mask)
 {
 	if (ppc_md.pcibios_enable_device_hook)
Index: work6/arch/ppc/kernel/pci.c
===================================================================
--- work6.orig/arch/ppc/kernel/pci.c	2008-02-27 14:37:14.000000000 -0700
+++ work6/arch/ppc/kernel/pci.c	2008-02-27 14:40:34.000000000 -0700
@@ -578,39 +578,6 @@
 }
 
 
-int
-pcibios_enable_resources(struct pci_dev *dev, int mask)
-{
-	u16 cmd, old_cmd;
-	int idx;
-	struct resource *r;
-
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-	old_cmd = cmd;
-	for (idx=0; idx<6; idx++) {
-		/* Only set up the requested stuff */
-		if (!(mask & (1<<idx)))
-			continue;
-	
-		r = &dev->resource[idx];
-		if (r->flags & IORESOURCE_UNSET) {
-			printk(KERN_ERR "PCI: Device %s not available because of resource collisions\n", pci_name(dev));
-			return -EINVAL;
-		}
-		if (r->flags & IORESOURCE_IO)
-			cmd |= PCI_COMMAND_IO;
-		if (r->flags & IORESOURCE_MEM)
-			cmd |= PCI_COMMAND_MEMORY;
-	}
-	if (dev->resource[PCI_ROM_RESOURCE].start)
-		cmd |= PCI_COMMAND_MEMORY;
-	if (cmd != old_cmd) {
-		printk("PCI: Enabling device %s (%04x -> %04x)\n", pci_name(dev), old_cmd, cmd);
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-	}
-	return 0;
-}
-
 static int next_controller_index;
 
 struct pci_controller * __init
Index: work6/arch/sh/drivers/pci/pci.c
===================================================================
--- work6.orig/arch/sh/drivers/pci/pci.c	2008-02-27 14:37:14.000000000 -0700
+++ work6/arch/sh/drivers/pci/pci.c	2008-02-27 14:40:34.000000000 -0700
@@ -131,38 +131,6 @@
 	}
 }
 
-int pcibios_enable_resources(struct pci_dev *dev, int mask)
-{
-	u16 cmd, old_cmd;
-	int idx;
-	struct resource *r;
-
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-	old_cmd = cmd;
-	for(idx=0; idx<6; idx++) {
-		if (!(mask & (1 << idx)))
-			continue;
-		r = &dev->resource[idx];
-		if (!r->start && r->end) {
-			printk(KERN_ERR "PCI: Device %s not available because "
-			       "of resource collisions\n", pci_name(dev));
-			return -EINVAL;
-		}
-		if (r->flags & IORESOURCE_IO)
-			cmd |= PCI_COMMAND_IO;
-		if (r->flags & IORESOURCE_MEM)
-			cmd |= PCI_COMMAND_MEMORY;
-	}
-	if (dev->resource[PCI_ROM_RESOURCE].start)
-		cmd |= PCI_COMMAND_MEMORY;
-	if (cmd != old_cmd) {
-		printk(KERN_INFO "PCI: Enabling device %s (%04x -> %04x)\n",
-		       pci_name(dev), old_cmd, cmd);
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-	}
-	return 0;
-}
-
 int pcibios_enable_device(struct pci_dev *dev, int mask)
 {
 	return pcibios_enable_resources(dev, mask);
Index: work6/arch/sparc64/kernel/pci.c
===================================================================
--- work6.orig/arch/sparc64/kernel/pci.c	2008-02-27 14:37:14.000000000 -0700
+++ work6/arch/sparc64/kernel/pci.c	2008-02-27 14:40:34.000000000 -0700
@@ -946,36 +946,6 @@
 {
 }
 
-int pcibios_enable_resources(struct pci_dev *dev, int mask)
-{
-	u16 cmd, oldcmd;
-	int i;
-
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-	oldcmd = cmd;
-
-	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-		struct resource *res = &dev->resource[i];
-
-		/* Only set up the requested stuff */
-		if (!(mask & (1<<i)))
-			continue;
-
-		if (res->flags & IORESOURCE_IO)
-			cmd |= PCI_COMMAND_IO;
-		if (res->flags & IORESOURCE_MEM)
-			cmd |= PCI_COMMAND_MEMORY;
-	}
-
-	if (cmd != oldcmd) {
-		printk(KERN_DEBUG "PCI: Enabling device: (%s), cmd %x\n",
-		       pci_name(dev), cmd);
-                /* Enable the appropriate bits in the PCI command register.  */
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-	}
-	return 0;
-}
-
 int pcibios_enable_device(struct pci_dev *dev, int mask)
 {
 	return pcibios_enable_resources(dev, mask);
Index: work6/arch/v850/kernel/rte_mb_a_pci.c
===================================================================
--- work6.orig/arch/v850/kernel/rte_mb_a_pci.c	2008-02-27 14:37:14.000000000 -0700
+++ work6/arch/v850/kernel/rte_mb_a_pci.c	2008-02-27 14:40:34.000000000 -0700
@@ -217,34 +217,6 @@
 }
 
 \f
-int __nomods_init pcibios_enable_resources (struct pci_dev *dev, int mask)
-{
-	u16 cmd, old_cmd;
-	int idx;
-	struct resource *r;
-
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-	old_cmd = cmd;
-	for (idx = 0; idx < 6; idx++) {
-		r = &dev->resource[idx];
-		if (!r->start && r->end) {
-			printk(KERN_ERR "PCI: Device %s not available because "
-			       "of resource collisions\n", pci_name(dev));
-			return -EINVAL;
-		}
-		if (r->flags & IORESOURCE_IO)
-			cmd |= PCI_COMMAND_IO;
-		if (r->flags & IORESOURCE_MEM)
-			cmd |= PCI_COMMAND_MEMORY;
-	}
-	if (cmd != old_cmd) {
-		printk("PCI: Enabling device %s (%04x -> %04x)\n",
-		       pci_name(dev), old_cmd, cmd);
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-	}
-	return 0;
-}
-
 int __nomods_init pcibios_enable_device (struct pci_dev *dev, int mask)
 {
 	return pcibios_enable_resources(dev, mask);
Index: work6/arch/x86/pci/i386.c
===================================================================
--- work6.orig/arch/x86/pci/i386.c	2008-02-27 14:37:14.000000000 -0700
+++ work6/arch/x86/pci/i386.c	2008-02-27 14:40:34.000000000 -0700
@@ -238,44 +238,6 @@
  */
 fs_initcall(pcibios_assign_resources);
 
-int pcibios_enable_resources(struct pci_dev *dev, int mask)
-{
-	u16 cmd, old_cmd;
-	int idx;
-	struct resource *r;
-
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-	old_cmd = cmd;
-	for (idx = 0; idx < PCI_NUM_RESOURCES; idx++) {
-		/* Only set up the requested stuff */
-		if (!(mask & (1 << idx)))
-			continue;
-
-		r = &dev->resource[idx];
-		if (!(r->flags & (IORESOURCE_IO | IORESOURCE_MEM)))
-			continue;
-		if ((idx == PCI_ROM_RESOURCE) &&
-				(!(r->flags & IORESOURCE_ROM_ENABLE)))
-			continue;
-		if (!r->start && r->end) {
-			printk(KERN_ERR "PCI: Device %s not available "
-				"because of resource %d collisions\n",
-				pci_name(dev), idx);
-			return -EINVAL;
-		}
-		if (r->flags & IORESOURCE_IO)
-			cmd |= PCI_COMMAND_IO;
-		if (r->flags & IORESOURCE_MEM)
-			cmd |= PCI_COMMAND_MEMORY;
-	}
-	if (cmd != old_cmd) {
-		printk("PCI: Enabling device %s (%04x -> %04x)\n",
-			pci_name(dev), old_cmd, cmd);
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-	}
-	return 0;
-}
-
 /*
  *  If we set up a device for bus mastering, we need to check the latency
  *  timer as certain crappy BIOSes forget to set it properly.
Index: work6/arch/x86/pci/pci.h
===================================================================
--- work6.orig/arch/x86/pci/pci.h	2008-02-27 14:37:14.000000000 -0700
+++ work6/arch/x86/pci/pci.h	2008-02-27 14:40:34.000000000 -0700
@@ -44,7 +44,6 @@
 extern unsigned int pcibios_max_latency;
 
 void pcibios_resource_survey(void);
-int pcibios_enable_resources(struct pci_dev *, int);
 
 /* pci-pc.c */
 
Index: work6/arch/xtensa/kernel/pci.c
===================================================================
--- work6.orig/arch/xtensa/kernel/pci.c	2008-02-27 14:37:14.000000000 -0700
+++ work6/arch/xtensa/kernel/pci.c	2008-02-27 14:40:34.000000000 -0700
@@ -91,37 +91,6 @@
 	}
 }
 
-int
-pcibios_enable_resources(struct pci_dev *dev, int mask)
-{
-	u16 cmd, old_cmd;
-	int idx;
-	struct resource *r;
-
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-	old_cmd = cmd;
-	for(idx=0; idx<6; idx++) {
-		r = &dev->resource[idx];
-		if (!r->start && r->end) {
-			printk (KERN_ERR "PCI: Device %s not available because "
-				"of resource collisions\n", pci_name(dev));
-			return -EINVAL;
-		}
-		if (r->flags & IORESOURCE_IO)
-			cmd |= PCI_COMMAND_IO;
-		if (r->flags & IORESOURCE_MEM)
-			cmd |= PCI_COMMAND_MEMORY;
-	}
-	if (dev->resource[PCI_ROM_RESOURCE].start)
-		cmd |= PCI_COMMAND_MEMORY;
-	if (cmd != old_cmd) {
-		printk("PCI: Enabling device %s (%04x -> %04x)\n",
-			pci_name(dev), old_cmd, cmd);
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-	}
-	return 0;
-}
-
 struct pci_controller * __init pcibios_alloc_controller(void)
 {
 	struct pci_controller *pci_ctrl;
Index: work6/drivers/pci/setup-res.c
===================================================================
--- work6.orig/drivers/pci/setup-res.c	2008-02-27 14:37:14.000000000 -0700
+++ work6/drivers/pci/setup-res.c	2008-02-27 16:53:59.000000000 -0700
@@ -263,3 +263,47 @@
 		}
 	}
 }
+
+int __attribute__ ((weak)) pcibios_enable_resources(struct pci_dev *dev,
+						    int mask)
+{
+	u16 cmd, old_cmd;
+	int i;
+	struct resource *r;
+
+	pci_read_config_word(dev, PCI_COMMAND, &cmd);
+	old_cmd = cmd;
+
+	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
+		if (!(mask & (1 << i)))
+			continue;
+
+		r = &dev->resource[i];
+
+		if (!(r->flags & (IORESOURCE_IO | IORESOURCE_MEM)))
+			continue;
+		if ((i == PCI_ROM_RESOURCE) &&
+				(!(r->flags & IORESOURCE_ROM_ENABLE)))
+			continue;
+
+		if (!r->parent) {
+			dev_err(&dev->dev, "device not available because of "
+				"BAR %d [%llx:%llx] collisions\n", i,
+				(unsigned long long) r->start,
+				(unsigned long long) r->end);
+			return -EINVAL;
+		}
+
+		if (r->flags & IORESOURCE_IO)
+			cmd |= PCI_COMMAND_IO;
+		if (r->flags & IORESOURCE_MEM)
+			cmd |= PCI_COMMAND_MEMORY;
+	}
+
+	if (cmd != old_cmd) {
+		dev_info(&dev->dev, "enabling device (%04x -> %04x)\n",
+			 old_cmd, cmd);
+		pci_write_config_word(dev, PCI_COMMAND, cmd);
+	}
+	return 0;
+}
Index: work6/include/linux/pci.h
===================================================================
--- work6.orig/include/linux/pci.h	2008-02-27 14:37:14.000000000 -0700
+++ work6/include/linux/pci.h	2008-02-27 14:40:34.000000000 -0700
@@ -616,6 +616,7 @@
 void pci_assign_unassigned_resources(void);
 void pdev_enable_device(struct pci_dev *);
 void pdev_sort_resources(struct pci_dev *, struct resource_list *);
+int pcibios_enable_resources(struct pci_dev *, int mask);
 void pci_fixup_irqs(u8 (*)(struct pci_dev *, u8 *),
 		    int (*)(struct pci_dev *, u8, u8));
 #define HAVE_PCI_REQ_REGIONS	2

-- 

Re: [patch 6/6] PCI: consolidate several pcibios_enable_resources() implementations

[Jesse Barnes]

On Wednesday, February 27, 2008 4:04 pm Bjorn Helgaas wrote:
> There are many implementations of pcibios_enable_resources() that differ
> in minor ways that look more like bugs than architectural differences.
>
> This patch consolidates most of them to use the version annotated below.
> This is the original x86 version, except that it uses the resource
> collision check from powerpc at (5):

Looks really good to me, definitely a step in the right direction.

> The mips/pmc-sierra implementation of pcibios_enable_resources() is
> cluttered with a bunch of titan stuff, so I can't immediately consolidate
> it with the others.  So I made the generic version "weak" so pmc-sierra
> can override it.
>
> Not-Yet-Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

So you'd like to see the MIPS stuff cleaned up a bit more first before actual 
sign-off?  Or just more testing?

>  18 files changed, 43 insertions(+), 507 deletions(-)

Nice diffstat. :)

Jesse

Re: [patch 6/6] PCI: consolidate several pcibios_enable_resources() implementations

[Bjorn Helgaas]

On Monday 03 March 2008 11:45:06 am Jesse Barnes wrote:
> On Wednesday, February 27, 2008 4:04 pm Bjorn Helgaas wrote:
> >
> > Not-Yet-Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> 
> So you'd like to see the MIPS stuff cleaned up a bit more first before actual 
> sign-off?  Or just more testing?

I think it'd be *nice* if that MIPS stuff got cleaned up, but that's
way beyond my scope.  I just want to address Kyle's comments and make
sure I don't screw up ARM and PARISC.

Bjorn

Re: [patch 6/6] PCI: consolidate several pcibios_enable_resources() implementations

[David Howells]

Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:

> 
>     frv: checks only 6 resources at (1), has a different ROM
> 	resource check at (4) and (6) that ignores IORESOURCE_ROM_ENABLE
> ...
>     mn10300: checks only 6 resources at (1), has no IORESOURCE_{IO,MEM}
> 	check at (3), has a different ROM resource check at (4) and (6)
> 	that ignores IORESOURCE_ROM_ENABLE

Both parts:

Acked-by: David Howells <dhowells@redhat.com>

Re: [patch 0/6] RFC: PCI: consolidate pcibios_enable_resources() implementations, v2

[Russell King]

On Wed, Feb 27, 2008 at 05:04:37PM -0700, Bjorn Helgaas wrote:
> There are many implementations of pcibios_enable_resources() that differ
> in minor ways that look more like bugs than architectural differences.
> This patch series consolidates most of them to use the x86 version.
> 
> Changes between v1 and v2:
> 
>   - Moved ARM bridge enable to new platform_pci_enable_device(),
>     called by pcibios_enable_device()

Looks fine.  However, long term I've no idea what to do about this because
I don't remember the reasoning behind it.  So to change it risks breakage
of one sort or another.

It might have been something to do with the Mobility Cardbus docking
station, which adds a pair of P2P bridges onto the PCI chain downstream
of the Cardbus controller, and then a full PCI bus containing USB, VGA,
and other peripherals.

This _once_ used to work with Linux but I suspect as a result of "fixing"
other issues its now utterly broken.

In any case, that docking station isn't ARM specific in any way; merely
a toy Alan Cox sent me.  When I gave up my PCMCIA maintainership, I gave
up trying to keep it supported by Linux.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of: