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

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

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 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-VXdhtT5mjnY@public.gmane.org>

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
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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-VXdhtT5mjnY@public.gmane.org>
> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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>

-------------------------------------------------------------------
List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php

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-VXdhtT5mjnY@public.gmane.org>
> > 
> > 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-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>

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

Thanks,
  Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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:
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html