Please revert: PCI: fix IDE legacy mode resources

Please revert: PCI: fix IDE legacy mode resources

[Benjamin Herrenschmidt]

The commit below that was merged in october looks bogus to me.

At this stage in the PCI probe, the pci_dev->resource's contain RAW bar
values, that is bus values..

A PCI legacy IDE controller that hard decodes 0x1f0 etc...  does such as
bus values as well. That is, the resources should contain 0x1f0...0x1f7
etc... -not- some kind of transformed values, because that's exactly
what a BAR would contain if it had been read from the device by
pci_read_bases() and we haven't performed any fixup yet.

If the platform offsets resources, like powerpc does, it should do so
later on in a fixup pass (on ppc, we use either a header quirk or
fixup_bus depending on the phase of the moon) and that should work
fine. 

I don't understand how his fix can work on MIPS nor why the previous
code didn't, but I don't know how MIPS does its remapping tricks,
however it will definitely -not- work on powerpc (and will break a
couple of machines out there).

So there may be a problem with MIPS but that "fix" is wrong and will
break PowerPC at least. Can it be reverted ? I'll work with Yoichi and
Ralf to understand what mips does and see how it can be fixed.

Cheers,
Ben. 

On Fri, 2007-10-12 at 23:05 +0000, Linux Kernel Mailing List wrote:
> Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=fd6e732186ab522c812ab19c2c5e5befb8ec8115
> Commit:     fd6e732186ab522c812ab19c2c5e5befb8ec8115
> Parent:     cbf5d9e6b9bcf03291cbb51db144b3e2773a8a2d
> Author:     Yoichi Yuasa <yoichi_yuasa@tripeaks.co.jp>
> AuthorDate: Tue Oct 2 14:19:23 2007 -0700
> Committer:  Greg Kroah-Hartman <gregkh@suse.de>
> CommitDate: Fri Oct 12 15:03:17 2007 -0700
> 
>     PCI: fix IDE legacy mode resources
>     
>     I got the following error on MIPS Cobalt.
>     
>     PCI: Unable to reserve I/O region #1:8@f00001f0 for device 0000:00:09.1
>     pata_via 0000:00:09.1: failed to request/iomap BARs for port 0 (errno=-16)
>     PCI: Unable to reserve I/O region #3:8@f0000170 for device 0000:00:09.1
>     pata_via 0000:00:09.1: failed to request/iomap BARs for port 1 (errno=-16)
>     pata_via 0000:00:09.1: no available native port
>     
>     The legacy mode IDE resources set the following order.
>     
>     pci_setup_device()
>         Legacy mode ATA controllers have fixed addresses.
>         IDE resources: 0x1F0-0x1F7, 0x3F6, 0x170-0x177, 0x376
>         |
>         V
>     pcibios_fixup_bus()
>         MIPS Cobalt PCI bus regions have the -0x10000000 offset from PCI resources.
>         pcibios_fixup_bus() fix PCI bus regions.
>         0x1F0 - 0x10000000 = 0xF00001F0
>         |
>         V
>     ata_pci_init_one()
>         PCI: Unable to reserve I/O region #1:8@f00001f0 for device 0000:00:09.1
>     
>     In some architectures, PCI bus regions have the offset from PCI resources.
>     For this reason, pci_setup_device() should set PCI bus regions to
>     dev->resource[].
>     
>     [akpm@linux-foundation.org: use struct initialiser]
>     Signed-off-by: Yoichi Yuasa <yoichi_yuasa@tripeaks.co.jp>
>     Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
>     Cc: Greg KH <greg@kroah.com>
>     Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>     Cc: Ralf Baechle <ralf@linux-mips.org>
>     Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>     Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> ---
>  drivers/pci/probe.c |   48 ++++++++++++++++++++++++++++++++++++------------
>  1 files changed, 36 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 171ca71..40e571d 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -744,22 +744,46 @@ static int pci_setup_device(struct pci_dev * dev)
>  		 */
>  		if (class == PCI_CLASS_STORAGE_IDE) {
>  			u8 progif;
> +			struct pci_bus_region region;
> +
>  			pci_read_config_byte(dev, PCI_CLASS_PROG, &progif);
>  			if ((progif & 1) == 0) {
> -				dev->resource[0].start = 0x1F0;
> -				dev->resource[0].end = 0x1F7;
> -				dev->resource[0].flags = LEGACY_IO_RESOURCE;
> -				dev->resource[1].start = 0x3F6;
> -				dev->resource[1].end = 0x3F6;
> -				dev->resource[1].flags = LEGACY_IO_RESOURCE;
> +				struct resource resource = {
> +					.start = 0x1F0,
> +					.end = 0x1F7,
> +					.flags = LEGACY_IO_RESOURCE,
> +				};
> +
> +				pcibios_resource_to_bus(dev, &region, &resource);
> +				dev->resource[0].start = region.start;
> +				dev->resource[0].end = region.end;
> +				dev->resource[0].flags = resource.flags;
> +				resource.start = 0x3F6;
> +				resource.end = 0x3F6;
> +				resource.flags = LEGACY_IO_RESOURCE;
> +				pcibios_resource_to_bus(dev, &region, &resource);
> +				dev->resource[1].start = region.start;
> +				dev->resource[1].end = region.end;
> +				dev->resource[1].flags = resource.flags;
>  			}
>  			if ((progif & 4) == 0) {
> -				dev->resource[2].start = 0x170;
> -				dev->resource[2].end = 0x177;
> -				dev->resource[2].flags = LEGACY_IO_RESOURCE;
> -				dev->resource[3].start = 0x376;
> -				dev->resource[3].end = 0x376;
> -				dev->resource[3].flags = LEGACY_IO_RESOURCE;
> +				struct resource resource = {
> +					.start = 0x170,
> +					.end = 0x177,
> +					.flags = LEGACY_IO_RESOURCE,
> +				};
> +
> +				pcibios_resource_to_bus(dev, &region, &resource);
> +				dev->resource[2].start = region.start;
> +				dev->resource[2].end = region.end;
> +				dev->resource[2].flags = resource.flags;
> +				resource.start = 0x376;
> +				resource.end = 0x376;
> +				resource.flags = LEGACY_IO_RESOURCE;
> +				pcibios_resource_to_bus(dev, &region, &resource);
> +				dev->resource[3].start = region.start;
> +				dev->resource[3].end = region.end;
> +				dev->resource[3].flags = resource.flags;
>  			}
>  		}
>  		break;
> -
> To unsubscribe from this list: send the line "unsubscribe git-commits-head" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Please revert: PCI: fix IDE legacy mode resources

[Yoichi Yuasa]

On Thu, 06 Dec 2007 11:10:18 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> The commit below that was merged in october looks bogus to me.
> 
> At this stage in the PCI probe, the pci_dev->resource's contain RAW bar
> values, that is bus values..
> 
> A PCI legacy IDE controller that hard decodes 0x1f0 etc...  does such as
> bus values as well. That is, the resources should contain 0x1f0...0x1f7
> etc... -not- some kind of transformed values, because that's exactly
> what a BAR would contain if it had been read from the device by
> pci_read_bases() and we haven't performed any fixup yet.
> 
> If the platform offsets resources, like powerpc does, it should do so
> later on in a fixup pass (on ppc, we use either a header quirk or
> fixup_bus depending on the phase of the moon) and that should work
> fine. 
> 
> I don't understand how his fix can work on MIPS nor why the previous
> code didn't, but I don't know how MIPS does its remapping tricks,
> however it will definitely -not- work on powerpc (and will break a
> couple of machines out there).

MIPS pcibios_fixup_bus() converts RAW BAR values(including offset) to resource values.
How does it fix up on powerpc?

Yoichi 

Re: Please revert: PCI: fix IDE legacy mode resources

[Benjamin Herrenschmidt]

On Thu, 2007-12-06 at 13:34 +0900, Yoichi Yuasa wrote:
> > I don't understand how his fix can work on MIPS nor why the previous
> > code didn't, but I don't know how MIPS does its remapping tricks,
> > however it will definitely -not- work on powerpc (and will break a
> > couple of machines out there).
> 
> MIPS pcibios_fixup_bus() converts RAW BAR values(including offset) to
> resource values. How does it fix up on powerpc?

Same thing. We expect resources to contain raw values before . What I
don't understand is thus why you are calling resource_to_bus on 0x1f0
which is -not- a resource value, but is already a BAR value...

Ben.

Re: Please revert: PCI: fix IDE legacy mode resources

[Yoichi Yuasa]

On Thu, 06 Dec 2007 16:04:07 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> 
> On Thu, 2007-12-06 at 13:34 +0900, Yoichi Yuasa wrote:
> > > I don't understand how his fix can work on MIPS nor why the previous
> > > code didn't, but I don't know how MIPS does its remapping tricks,
> > > however it will definitely -not- work on powerpc (and will break a
> > > couple of machines out there).
> > 
> > MIPS pcibios_fixup_bus() converts RAW BAR values(including offset) to
> > resource values. How does it fix up on powerpc?
> 
> Same thing. We expect resources to contain raw values before .

Same.

> What I don't understand is thus why you are calling resource_to_bus on 0x1f0
> which is -not- a resource value, but is already a BAR value...

0x1f0 is resource value on MIPS Cobalt.
All RAW BAR values contain the offset(0x10000000) on it.

Do the BAR values on your target contain the offset?

Yoichi

Re: Please revert: PCI: fix IDE legacy mode resources

[Benjamin Herrenschmidt]

On Thu, 2007-12-06 at 14:58 +0900, Yoichi Yuasa wrote:
> > What I don't understand is thus why you are calling resource_to_bus
> on 0x1f0
> > which is -not- a resource value, but is already a BAR value...
> 
> 0x1f0 is resource value on MIPS Cobalt.
> All RAW BAR values contain the offset(0x10000000) on it.
> 
> Do the BAR values on your target contain the offset?

No, and I don't understand... raw BAR values don't contain such offset.
The physical address where the PIO is mapped might, but that's not what
we put in struct resource for IO resources and definitely not the BAR
value.

The legacy IDE device will decode cycles at 0x1f0, not cycles at
0x100001f0.

Take for example PowerPC. Imagine that I have a bus whose IO space is
mapped at 0xf0000000 in the processor physical address space (this is a
real example, my powermac does that for the x16 PCI-Express slot though
other slots use other offsets).

Now, the kernel on ppc64 will map that virtually at some allocated
virtual address that we'll call we call bus_io_virt for this
explanation. In addition, inb() and outb() will apply an offset (which
can be different) that we call _IO_BASE to the port numbers. In general,
bus_io_virt of the first bus == _IO_BASE on ppc32 but that's not a
strict rule.

Let's say for the sake of the example, that _IO_BASE is 0xd0000000 and
our bus has been mapped at 0xd0010000 (bus_io_virt). So 0xd0010000 maps
to 0xf0000000 via the MMU.

When we scan the bus, we read the BAR content. So for example, a device
whose IOs have been assigned at 0x1000 will read that as a RAW bar value
and pci_scan_slot() (or whoever does the reading) will put 0x1000 in the
struct resource. In a similar vein, such a legacy controller would thus
be expected to have 0x1f0 in the resource.

Later, when we fixup (in a head quirk on ppc32 and in pcibios_fixup_bus
on ppc64, though that's changing wit 2.6.25 to use the same mechanism),
we see an IO resource, and we fixup by adding to it basically
(bus_io_virt - _IO_BASE).

That is, for our device that has a 0x1000 BAR value, we'll do:

	resource = 0x1000 + (0xd0010000 - 0xd0000000) = 0x11000

And for the legacy IDE:

	resource = 0x1f0 + (0xd0010000 - 0xd0000000) = 0x101f0

Now, if you do an inb or outb to one of these, the inb() and oub()
accessors will add _IO_BASE, which is 0xd0000000 in our example, so
you'll end up doing accesses to respectively:

	access = 0xd0011000 for the example device or
	access = 0xd00101f0 for the IDE controller

That translates via the MMU to

	phys = 0xf0001000 for the example device or
	phys = 0xf00001f0 for the IDE controller

Which translates on to the bus into an IO cycle at the raw BAR
address (which is what is in the BAR content or hard-decoded in the case
of the legacy IDE):

	bus = 0x1000 for the example device
	bus = 0x01f0 for the IDE controller

which ... is what we started with.

Now I don't understand how MIPS does things differently, but I can't see
how it can be legal to call pci_resource_to_bus() on 0x1f0 in
pci_setup_device(), because at this stage, we are putting raw BAR values
in the struct resource (that is PCI bus addresses) and 0x1f0 _is_ such a
value.

Calling pci_resource_to_bus() would somewhat mean that 0x1f0 is not, and
instead is some kind of already fixed up resource value that we want
converted back into a BAR value, which is not the case.

So I suspect your patch works by accident more than by design, or I am
missing something...

Cheers,
Ben.




Ben.

Re: Please revert: PCI: fix IDE legacy mode resources

[Ralf Baechle]

On Thu, Dec 06, 2007 at 05:24:22PM +1100, Benjamin Herrenschmidt wrote:

> On Thu, 2007-12-06 at 14:58 +0900, Yoichi Yuasa wrote:
> > > What I don't understand is thus why you are calling resource_to_bus
> > on 0x1f0
> > > which is -not- a resource value, but is already a BAR value...
> > 
> > 0x1f0 is resource value on MIPS Cobalt.
> > All RAW BAR values contain the offset(0x10000000) on it.
> > 
> > Do the BAR values on your target contain the offset?
> 
> No, and I don't understand... raw BAR values don't contain such offset.
> The physical address where the PIO is mapped might, but that's not what
> we put in struct resource for IO resources and definitely not the BAR
> value.
> 
> The legacy IDE device will decode cycles at 0x1f0, not cycles at
> 0x100001f0.
> 
> Take for example PowerPC. Imagine that I have a bus whose IO space is
> mapped at 0xf0000000 in the processor physical address space (this is a
> real example, my powermac does that for the x16 PCI-Express slot though
> other slots use other offsets).
> 
> Now, the kernel on ppc64 will map that virtually at some allocated
> virtual address that we'll call we call bus_io_virt for this
> explanation. In addition, inb() and outb() will apply an offset (which
> can be different) that we call _IO_BASE to the port numbers. In general,
> bus_io_virt of the first bus == _IO_BASE on ppc32 but that's not a
> strict rule.
> 
> Let's say for the sake of the example, that _IO_BASE is 0xd0000000 and
> our bus has been mapped at 0xd0010000 (bus_io_virt). So 0xd0010000 maps
> to 0xf0000000 via the MMU.
> 
> When we scan the bus, we read the BAR content. So for example, a device
> whose IOs have been assigned at 0x1000 will read that as a RAW bar value
> and pci_scan_slot() (or whoever does the reading) will put 0x1000 in the
> struct resource. In a similar vein, such a legacy controller would thus
> be expected to have 0x1f0 in the resource.
> 
> Later, when we fixup (in a head quirk on ppc32 and in pcibios_fixup_bus
> on ppc64, though that's changing wit 2.6.25 to use the same mechanism),
> we see an IO resource, and we fixup by adding to it basically
> (bus_io_virt - _IO_BASE).
> 
> That is, for our device that has a 0x1000 BAR value, we'll do:
> 
> 	resource = 0x1000 + (0xd0010000 - 0xd0000000) = 0x11000
> 
> And for the legacy IDE:
> 
> 	resource = 0x1f0 + (0xd0010000 - 0xd0000000) = 0x101f0
> 
> Now, if you do an inb or outb to one of these, the inb() and oub()
> accessors will add _IO_BASE, which is 0xd0000000 in our example, so
> you'll end up doing accesses to respectively:
> 
> 	access = 0xd0011000 for the example device or
> 	access = 0xd00101f0 for the IDE controller
> 
> That translates via the MMU to
> 
> 	phys = 0xf0001000 for the example device or
> 	phys = 0xf00001f0 for the IDE controller
> 
> Which translates on to the bus into an IO cycle at the raw BAR
> address (which is what is in the BAR content or hard-decoded in the case
> of the legacy IDE):
> 
> 	bus = 0x1000 for the example device
> 	bus = 0x01f0 for the IDE controller
> 
> which ... is what we started with.
> 
> Now I don't understand how MIPS does things differently, but I can't see
> how it can be legal to call pci_resource_to_bus() on 0x1f0 in
> pci_setup_device(), because at this stage, we are putting raw BAR values
> in the struct resource (that is PCI bus addresses) and 0x1f0 _is_ such a
> value.
> 
> Calling pci_resource_to_bus() would somewhat mean that 0x1f0 is not, and
> instead is some kind of already fixed up resource value that we want
> converted back into a BAR value, which is not the case.
> 
> So I suspect your patch works by accident more than by design, or I am
> missing something...

So where do we stand with this?

As I understand the Cobalt system controller it is not possible to address
ioport addresses below 0x10000000 at all on the PCI bus of the GT-64111.
As such I think the best solution is a GT-64111-specific PCI fixup to
clear out legacy resources.  The IDE controller in the VIA VT82C586 could
then be used only by its normal that is non-legacy address and
commit fd6e732186ab522c812ab19c2c5e5befb8ec8115 could be reverted and PPC
would be happy too?

  Ralf

Re: Please revert: PCI: fix IDE legacy mode resources

[Benjamin Herrenschmidt]

On Sun, 2007-12-09 at 02:12 +0000, Ralf Baechle wrote:
> So where do we stand with this?
> 
> As I understand the Cobalt system controller it is not possible to address
> ioport addresses below 0x10000000 at all on the PCI bus of the GT-64111.
> As such I think the best solution is a GT-64111-specific PCI fixup to
> clear out legacy resources.  The IDE controller in the VIA VT82C586 could
> then be used only by its normal that is non-legacy address and
> commit fd6e732186ab522c812ab19c2c5e5befb8ec8115 could be reverted and PPC
> would be happy too?

If that is the case though (that is it can't issue low ioport cycles),
how would have the fd6e7321...  worked in the first place ? Hrm...
strange. My understanding is that all that patch does is put junk in the
pci_dev resource structures :-) Maybe that's enough to cause the PCI
layer later on to be unhappy about them and reassign the BARs to some
place that works ? In which case, you are right, a better approach is a
quirk on this specific platform, or even better, mark 0...0x10000000
busy in ioport_resources and let the generic code clash & re-assign...

I must admit I'm a bit confused tho...

Anyway, so far, nobody is arguing in favor of keeping this patch in nor
so far trying and explanation on why it wouldn't be totally bogus, so I
suggest we revert it :-)

Cheers,
Ben.

Re: Please revert: PCI: fix IDE legacy mode resources

[Benjamin Herrenschmidt]

> If that is the case though (that is it can't issue low ioport cycles),
> how would have the fd6e7321...  worked in the first place ? Hrm...
> strange. My understanding is that all that patch does is put junk in the
> pci_dev resource structures :-) Maybe that's enough to cause the PCI
> layer later on to be unhappy about them and reassign the BARs to some
> place that works ? In which case, you are right, a better approach is a
> quirk on this specific platform, or even better, mark 0...0x10000000
> busy in ioport_resources and let the generic code clash & re-assign...

  .../...

In fact, I see a deeper problem with Bart's original patch that moved
the "fixup" the PCI probe.

The code now replaces the content of the resource structures with the
hard-decoded legacy addresses for any IDE controller in legacy mode,
just losing whatever was there (the real BAR value).

On some platforms, like PowerPC (and it might be the same problem MIPS
has, I don't know for sure), we have a quirk that puts those controller
back into native mode. But so far, those quirks didn't change the
resources as they were supposed to contain the proper BAR values that
would, from then, be used.

Now that those values have been overriden in the resources, when the
controller is switched to native mode, it will answer to the BAR values
that no longer match the content of the resources and the driver will
fail.

So I think we have a deeper breakage here. I'll dig some machines for
which we do that tomorrow and see what exactly is going on. It's mostly
older machines so if there's a breakage, it has easily gone under the
radar.

I suspect any platform with such a quirk to turn IDE controllers into
native mode will now -also- need to put back the BAR values in the
struct resource, and do so -before- the platform fixup happens, that is
from a header quirk.

Cheers,
Ben.

Re: Please revert: PCI: fix IDE legacy mode resources

[Bartlomiej Zolnierkiewicz]

On Sunday 09 December 2007, Benjamin Herrenschmidt wrote:
> 
> > If that is the case though (that is it can't issue low ioport cycles),
> > how would have the fd6e7321...  worked in the first place ? Hrm...
> > strange. My understanding is that all that patch does is put junk in the
> > pci_dev resource structures :-) Maybe that's enough to cause the PCI
> > layer later on to be unhappy about them and reassign the BARs to some
> > place that works ? In which case, you are right, a better approach is a
> > quirk on this specific platform, or even better, mark 0...0x10000000
> > busy in ioport_resources and let the generic code clash & re-assign...
> 
>   .../...
> 
> In fact, I see a deeper problem with Bart's original patch that moved
> the "fixup" the PCI probe.

I don't remember changing anything there, could you remind me what it was
exactly (commit number or patch name)?

> The code now replaces the content of the resource structures with the
> hard-decoded legacy addresses for any IDE controller in legacy mode,
> just losing whatever was there (the real BAR value).
> 
> On some platforms, like PowerPC (and it might be the same problem MIPS
> has, I don't know for sure), we have a quirk that puts those controller
> back into native mode. But so far, those quirks didn't change the
> resources as they were supposed to contain the proper BAR values that
> would, from then, be used.
> 
> Now that those values have been overriden in the resources, when the
> controller is switched to native mode, it will answer to the BAR values
> that no longer match the content of the resources and the driver will
> fail.
> 
> So I think we have a deeper breakage here. I'll dig some machines for
> which we do that tomorrow and see what exactly is going on. It's mostly
> older machines so if there's a breakage, it has easily gone under the
> radar.
> 
> I suspect any platform with such a quirk to turn IDE controllers into
> native mode will now -also- need to put back the BAR values in the
> struct resource, and do so -before- the platform fixup happens, that is
> from a header quirk.
> 
> Cheers,
> Ben.

Re: Please revert: PCI: fix IDE legacy mode resources

[Alan Cox]

> > In fact, I see a deeper problem with Bart's original patch that moved
> > the "fixup" the PCI probe.
> 
> I don't remember changing anything there, could you remind me what it was
> exactly (commit number or patch name)?

I'm pretty sure he means the code I added a while ago to actually correct
our IDE/ATA handling for legacy devices, fix the resource tree being
wrong and remove tons of special cases from drivers.

Alan

Re: Please revert: PCI: fix IDE legacy mode resources

[Benjamin Herrenschmidt]

On Sun, 2007-12-09 at 13:39 +0000, Alan Cox wrote:
> > > In fact, I see a deeper problem with Bart's original patch that moved
> > > the "fixup" the PCI probe.
> > 
> > I don't remember changing anything there, could you remind me what it was
> > exactly (commit number or patch name)?
> 
> I'm pretty sure he means the code I added a while ago to actually correct
> our IDE/ATA handling for legacy devices, fix the resource tree being
> wrong and remove tons of special cases from drivers.

Yup, that one, sorry Bart, I thought it was from you.

So far, a quirk grep shows such quirks only in powerpc code though, in
the chrp and powermac platforms (mostly the same one copied over). I'll
do a fix that re-allocate all resources on these instead (though I
suppose another option would be to just write the legacy addresses to
the BARs after turning it into native mode... probably even better).

Ben.

Re: Please revert: PCI: fix IDE legacy mode resources

[Alan Cox]

> The code now replaces the content of the resource structures with the
> hard-decoded legacy addresses for any IDE controller in legacy mode,
> just losing whatever was there (the real BAR value).

This is correct behaviour. It checks the port is in legacy mode. If the
port is in legacy mode the BAR value is undefined and in some cases
completely random.

> has, I don't know for sure), we have a quirk that puts those controller
> back into native mode. But so far, those quirks didn't change the
> resources as they were supposed to contain the proper BAR values that
> would, from then, be used.

Then your quirk is faulty (for the general case). The BAR values are
undefined at that point, they may not even be writable.

> I suspect any platform with such a quirk to turn IDE controllers into
> native mode will now -also- need to put back the BAR values in the
> struct resource, and do so -before- the platform fixup happens, that is
> from a header quirk.

Improbable unless its willing to rely on entirely undefined behaviour.

If you kick the device out of legacy mode (itself very very board
dependant) then you must find a suitable new resource allocation for the
controller. 

Alan

Re: Please revert: PCI: fix IDE legacy mode resources

[Benjamin Herrenschmidt]

On Sun, 2007-12-09 at 13:38 +0000, Alan Cox wrote:
> > has, I don't know for sure), we have a quirk that puts those
> controller
> > back into native mode. But so far, those quirks didn't change the
> > resources as they were supposed to contain the proper BAR values
> that
> > would, from then, be used.
> 
> Then your quirk is faulty (for the general case). The BAR values are
> undefined at that point, they may not even be writable.

Possibly, however, the fact is that those quirks "just worked" on all HW
where we used them in the past, so while in "theory" they are incorrect,
in practice, this is a regression and thus needs to be fixed.

> Improbable unless its willing to rely on entirely undefined behaviour.

I would argue to you that the whole legacy PCI thing is mostly
"undefined behaviour" from day 1. We rely on what worked in practice for
us, period. It's broken now, this is a regression. I'm not saying we
should revert the change in the generic code, I'm just raising an alarm
here as I doubt we are the only platform to do that (heh, I didn't even
write those quirks in the first place) and some fixing is needed in that
area. I'm totally fine with changing the quirks to do the "right"
though.

> If you kick the device out of legacy mode (itself very very board
> dependant) then you must find a suitable new resource allocation for
> the controller. 

Quite possibly, though as I said in practice, what we did so far
happened to "just work" on pretty much everything we were faced with
(which iirc is basically winbond and VIA controllers, possibly a few
others).

Anyway, I'll scrub around. Again, I'm not saying the approach is wrong
in the generic code.

Ben.

Re: Please revert: PCI: fix IDE legacy mode resources

[Alan Cox]

> Quite possibly, though as I said in practice, what we did so far
> happened to "just work" on pretty much everything we were faced with
> (which iirc is basically winbond and VIA controllers, possibly a few
> others).
> 
> Anyway, I'll scrub around. Again, I'm not saying the approach is wrong
> in the generic code.

I think the generic code is right, up to the MIPS stuff. What the MIPS
stuff is doing wants looking at further. If it simply can't address
legacy ports them it should set

CONFIG_NO_ATA_LEGACY

and be robust to such resource mappings. The CONFIG option tells libata
to skip over controllers in legacy mode.

For switching quirks you should be fixing the class code in an early
fixup. The early fixups are run before we do the BAR processing. You must
however remember to update dev->class as well as the PCI register if you
do so.

See quirk_svwkrs_csb5ide() for a correct worked example going in the
other direction. If you follow that for the PPC quirks but going native
then you will get desired results, although you want to check/know the BAR
resources are sane, or clear them as part of the process.

So in summary:
	- MIPS resource/bar bit may be wrong - if so lets revert that
	- Rest of the logic is just fine, but you may need to move your
quirks to be 'early' ones.

Alan

Re: Please revert: PCI: fix IDE legacy mode resources

[Benjamin Herrenschmidt]

On Sun, 2007-12-09 at 22:23 +0000, Alan Cox wrote:
> 
> I think the generic code is right, up to the MIPS stuff. What the MIPS
> stuff is doing wants looking at further. If it simply can't address
> legacy ports them it should set

Yes, well, we -do- need to remove the pcibios_resource_to_bus() things
that were added to the generic code. That commit still needs to be
reverted, it is bogus, as I explained in details. But I agree, the rest
of the generic code is probably the best way to deal with those stupid
fixed resources.

> For switching quirks you should be fixing the class code in an early
> fixup. The early fixups are run before we do the BAR processing. You
> must however remember to update dev->class as well as the PCI register
> if you do so.

I would still need to clear out the BARs I suppose when switching from
legacy to native mode, so they get re-assigned later.

> See quirk_svwkrs_csb5ide() for a correct worked example going in the
> other direction. If you follow that for the PPC quirks but going
> native
> then you will get desired results, although you want to check/know the
> BAR
> resources are sane, or clear them as part of the process.

Ok.

> So in summary:
>         - MIPS resource/bar bit may be wrong - if so lets revert that
>         - Rest of the logic is just fine, but you may need to move
> your
> quirks to be 'early' ones.

I'm digging the HW that needs that stuff right now so I can experiment
with various approaches and pick up the best one. Thanks.

Cheers,
Ben.

Re: Please revert: PCI: fix IDE legacy mode resources

[Benjamin Herrenschmidt]

powerpc: Fix IDE legacy vs. native fixups

PowerMac and CHRP/BriQ platforms have quirks to switch some IDE
controllers from legacy mode to fully native mode. Those quirks
however will not work properly anymore due to a change to the
generic code to better handle legacy IDE resources.

This fixes it by moving those quirk to "early" quirks (so they
run before resources are probed for the devices) and clearing
all BARs after the conversion to force a reallocation of sane
values.

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

To be totally correct, we still need to also revert
commit fd6e732186ab522c812ab19c2c5e5befb8ec8115 which
is bogus.

Linus, can you still apply this to 2.6.24 ? I would also like the
above (fd6e...) reverted as so far, nobody have come up with a demonstration
that it's not bogus.

Index: linux-work/arch/powerpc/platforms/chrp/pci.c
===================================================================
--- linux-work.orig/arch/powerpc/platforms/chrp/pci.c	2007-12-10 15:23:21.000000000 +1100
+++ linux-work/arch/powerpc/platforms/chrp/pci.c	2007-12-10 15:23:29.000000000 +1100
@@ -317,8 +317,12 @@ chrp_find_bridges(void)
 /* SL82C105 IDE Control/Status Register */
 #define SL82C105_IDECSR                0x40
 
-/* Fixup for Winbond ATA quirk, required for briq */
-void chrp_pci_fixup_winbond_ata(struct pci_dev *sl82c105)
+/* Fixup for Winbond ATA quirk, required for briq mostly because the
+ * 8259 is configured for level sensitive IRQ 14 and so wants the
+ * ATA controller to be set to fully native mode or bad things
+ * will happen.
+ */
+static void __devinit chrp_pci_fixup_winbond_ata(struct pci_dev *sl82c105)
 {
 	u8 progif;
 
@@ -334,10 +338,15 @@ void chrp_pci_fixup_winbond_ata(struct p
 		sl82c105->class |= 0x05;
 		/* Disable SL82C105 second port */
 		pci_write_config_word(sl82c105, SL82C105_IDECSR, 0x0003);
+		/* Clear IO BARs, they will be reassigned */
+		pci_write_config_dword(sl82c105, PCI_BASE_ADDRESS_0, 0);
+		pci_write_config_dword(sl82c105, PCI_BASE_ADDRESS_1, 0);
+		pci_write_config_dword(sl82c105, PCI_BASE_ADDRESS_2, 0);
+		pci_write_config_dword(sl82c105, PCI_BASE_ADDRESS_3, 0);
 	}
 }
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_WINBOND, PCI_DEVICE_ID_WINBOND_82C105,
-		chrp_pci_fixup_winbond_ata);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_WINBOND, PCI_DEVICE_ID_WINBOND_82C105,
+			chrp_pci_fixup_winbond_ata);
 
 /* Pegasos2 firmware version 20040810 configures the built-in IDE controller
  * in legacy mode, but sets the PCI registers to PCI native mode.
@@ -345,7 +354,7 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_WI
  * mode as well. The same fixup must be done to the class-code property in
  * the IDE node /pci@80000000/ide@C,1
  */
-static void chrp_pci_fixup_vt8231_ata(struct pci_dev *viaide)
+static void __devinit chrp_pci_fixup_vt8231_ata(struct pci_dev *viaide)
 {
 	u8 progif;
 	struct pci_dev *viaisa;
@@ -366,4 +375,4 @@ static void chrp_pci_fixup_vt8231_ata(st
 
 	pci_dev_put(viaisa);
 }
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C586_1, chrp_pci_fixup_vt8231_ata);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C586_1, chrp_pci_fixup_vt8231_ata);
Index: linux-work/arch/powerpc/platforms/powermac/pci.c
===================================================================
--- linux-work.orig/arch/powerpc/platforms/powermac/pci.c	2007-12-10 15:23:21.000000000 +1100
+++ linux-work/arch/powerpc/platforms/powermac/pci.c	2007-12-10 15:23:29.000000000 +1100
@@ -1243,15 +1243,22 @@ void pmac_pci_fixup_pciata(struct pci_de
  good:
 	pci_read_config_byte(dev, PCI_CLASS_PROG, &progif);
 	if ((progif & 5) != 5) {
-		printk(KERN_INFO "Forcing PCI IDE into native mode: %s\n",
+		printk(KERN_INFO "PCI: %s Forcing PCI IDE into native mode\n",
 		       pci_name(dev));
 		(void) pci_write_config_byte(dev, PCI_CLASS_PROG, progif|5);
 		if (pci_read_config_byte(dev, PCI_CLASS_PROG, &progif) ||
 		    (progif & 5) != 5)
 			printk(KERN_ERR "Rewrite of PROGIF failed !\n");
+		else {
+			/* Clear IO BARs, they will be reassigned */
+			pci_write_config_dword(dev, PCI_BASE_ADDRESS_0, 0);
+			pci_write_config_dword(dev, PCI_BASE_ADDRESS_1, 0);
+			pci_write_config_dword(dev, PCI_BASE_ADDRESS_2, 0);
+			pci_write_config_dword(dev, PCI_BASE_ADDRESS_3, 0);
+		}
 	}
 }
-DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, pmac_pci_fixup_pciata);
+DECLARE_PCI_FIXUP_EARLY(PCI_ANY_ID, PCI_ANY_ID, pmac_pci_fixup_pciata);
 #endif
 
 /*

Re: Please revert: PCI: fix IDE legacy mode resources

[Alan Cox]

On Mon, 10 Dec 2007 15:29:22 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> powerpc: Fix IDE legacy vs. native fixups
> 
> PowerMac and CHRP/BriQ platforms have quirks to switch some IDE
> controllers from legacy mode to fully native mode. Those quirks
> however will not work properly anymore due to a change to the
> generic code to better handle legacy IDE resources.
> 
> This fixes it by moving those quirk to "early" quirks (so they
> run before resources are probed for the devices) and clearing
> all BARs after the conversion to force a reallocation of sane
> values.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Acked-by: Alan Cox <alan@redhat.com>

> To be totally correct, we still need to also revert
> commit fd6e732186ab522c812ab19c2c5e5befb8ec8115 which
> is bogus.

Agreed but that would cause an revert on an obscure MIPS platform which
is apparently now a thoughtcrime ;)

> Linus, can you still apply this to 2.6.24 ? I would also like the
> above (fd6e...) reverted as so far, nobody have come up with a demonstration
> that it's not bogus.

Ack the revert too.

The MIPS needs sorting out differently.

Re: Please revert: PCI: fix IDE legacy mode resources

[Ralf Baechle]

On Mon, Dec 10, 2007 at 11:20:50AM +0000, Alan Cox wrote:

> > To be totally correct, we still need to also revert
> > commit fd6e732186ab522c812ab19c2c5e5befb8ec8115 which
> > is bogus.
> 
> Agreed but that would cause an revert on an obscure MIPS platform which
> is apparently now a thoughtcrime ;)
> 
> > Linus, can you still apply this to 2.6.24 ? I would also like the
> > above (fd6e...) reverted as so far, nobody have come up with a demonstration
> > that it's not bogus.
> 
> Ack the revert too.
> 
> The MIPS needs sorting out differently.

Yep.

It seems the whole MIPS resource managment is complicated enough (out of
necessity) that only a few people actually grok it.  Ioports being
actually memory mapped on MIPS only makes the confusion worse, sigh.

  Ralf

Re: Please revert: PCI: fix IDE legacy mode resources

[Alan Cox]

> It seems the whole MIPS resource managment is complicated enough (out of
> necessity) that only a few people actually grok it.  Ioports being
> actually memory mapped on MIPS only makes the confusion worse, sigh.

If the hardware cannot map the low PCI space then set
CONFIG_NO_ATA_LEGACY and the ATA layer will leave legacy ports alone.
Unfortunately its not clear we can make that mode try and force
controllers into native. We could try that if the MIPS and PPC people
want ?

Re: Please revert: PCI: fix IDE legacy mode resources

[Ralf Baechle]

On Mon, Dec 10, 2007 at 03:01:26PM +0000, Alan Cox wrote:

> > It seems the whole MIPS resource managment is complicated enough (out of
> > necessity) that only a few people actually grok it.  Ioports being
> > actually memory mapped on MIPS only makes the confusion worse, sigh.
> 
> If the hardware cannot map the low PCI space then set
> CONFIG_NO_ATA_LEGACY and the ATA layer will leave legacy ports alone.
> Unfortunately its not clear we can make that mode try and force
> controllers into native. We could try that if the MIPS and PPC people
> want ?

It sounds like a reasonable thing to try at this stage.

this btw is the lspci -v output for the IDE controller from a recent Cobalt
kernel:

> 0000:00:09.1 IDE interface: VIA Technologies, Inc.
> VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06)
> (prog-if 8a [Master SecP PriP])
>         Flags: bus master, fast Back2Back, medium devsel, latency 64
>         I/O ports at 1820 [size=16]

And that's lspci -v -b:

> 0000:00:09.1 IDE interface: VIA Technologies, Inc.
> VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06)
> (prog-if 8a [Master SecP PriP])
>         Flags: bus master, fast Back2Back, medium devsel, latency 64
>         I/O ports at 10001820

So the IDE controller already seems to be in native mode?

   Ralf

Re: Please revert: PCI: fix IDE legacy mode resources

[Benjamin Herrenschmidt]

On Mon, 2007-12-10 at 15:47 +0000, Ralf Baechle wrote:
> 
> 
> > 0000:00:09.1 IDE interface: VIA Technologies, Inc.
> > VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06)
> > (prog-if 8a [Master SecP PriP])
> >         Flags: bus master, fast Back2Back, medium devsel, latency 64
> >         I/O ports at 1820 [size=16]
> 
> And that's lspci -v -b:
> 
> > 0000:00:09.1 IDE interface: VIA Technologies, Inc.
> > VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06)
> > (prog-if 8a [Master SecP PriP])
> >         Flags: bus master, fast Back2Back, medium devsel, latency 64
> >         I/O ports at 10001820
> 
> So the IDE controller already seems to be in native mode?
> 

No, native mode is 5 not A in the low 4 bits of progif.

You need to be a bit careful about those VIA, I remember having issues
on Pegasos where we left it in legacy mode. It think the problem is that
even when switched, the IRQ routing might be done based on some other
setting in the chipset, possibly a strap. But that's nothing you can't
deal with an appropriate quirk in the arch code.

Also, double check the level/edge setting of the interrupts as it can be
different between legacy and native (native is level low, legacy is
rising edge).

I'm surprised however that one would use such a legacy southbridge on a
platform that can't issue low IO ports, that doesn't seem to make sense
to me ... there's a whole lot of things on this such as the 8259 PIC
etc.. that can only be addressed via low IOs, unless the ISA space can
be somewhat remapped ?

Ben.

Re: Please revert: PCI: fix IDE legacy mode resources

[Ralf Baechle]

On Tue, Dec 11, 2007 at 07:43:03AM +1100, Benjamin Herrenschmidt wrote:

> > > 0000:00:09.1 IDE interface: VIA Technologies, Inc.
> > > VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06)
> > > (prog-if 8a [Master SecP PriP])
> > >         Flags: bus master, fast Back2Back, medium devsel, latency 64
> > >         I/O ports at 1820 [size=16]
> > 
> > And that's lspci -v -b:
> > 
> > > 0000:00:09.1 IDE interface: VIA Technologies, Inc.
> > > VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06)
> > > (prog-if 8a [Master SecP PriP])
> > >         Flags: bus master, fast Back2Back, medium devsel, latency 64
> > >         I/O ports at 10001820
> > 
> > So the IDE controller already seems to be in native mode?
> > 
> 
> No, native mode is 5 not A in the low 4 bits of progif.
> 
> You need to be a bit careful about those VIA, I remember having issues
> on Pegasos where we left it in legacy mode. It think the problem is that
> even when switched, the IRQ routing might be done based on some other
> setting in the chipset, possibly a strap. But that's nothing you can't
> deal with an appropriate quirk in the arch code.
> 
> Also, double check the level/edge setting of the interrupts as it can be
> different between legacy and native (native is level low, legacy is
> rising edge).
> 
> I'm surprised however that one would use such a legacy southbridge on a
> platform that can't issue low IO ports, that doesn't seem to make sense
> to me ... there's a whole lot of things on this such as the 8259 PIC
> etc.. that can only be addressed via low IOs, unless the ISA space can
> be somewhat remapped ?

The GT-64111 system controller doesn't provide any kind of mapping
functionality that would help here.  So legacy port addressing can only
work by exploiting aliases due to incomplete decoding of legacy ioport
addreses by the VT82C586 - but direct addressing is impossible.

  Ralf

Re: Please revert: PCI: fix IDE legacy mode resources

[Benjamin Herrenschmidt]

> The GT-64111 system controller doesn't provide any kind of mapping
> functionality that would help here.  So legacy port addressing can only
> work by exploiting aliases due to incomplete decoding of legacy ioport
> addreses by the VT82C586 - but direct addressing is impossible.

Ok, that explains how the "fix" that we reverted worked. It caused crap
to be added to the top bits of the address :-)

So here, what you really want to do is not a call to
pcibios_resource_to_bus(), but you actually want to use a different bus
address in the first place, that you know the HW will decode the same
way.

The best way to achieve that imho, is to do a header quirk that is run
just after the generic probe code, which offsets the fixed legacy
resources by 0x10000000 since that's really the bus address you are
going to emit.

Later on, your pcibios_fixup code should take that remove 0x10000000
from all IO resources, since your 0xd0000000 mapping already maps
0x10000000 as you probably already do.

The trick is, you don't want to convert a "resource" into a "bus
address" here, but really issue a different bus address.

Cheers,
Ben.

Re: Please revert: PCI: fix IDE legacy mode resources

[Ralf Baechle]

On Tue, Dec 11, 2007 at 11:27:24AM +1100, Benjamin Herrenschmidt wrote:

> > The GT-64111 system controller doesn't provide any kind of mapping
> > functionality that would help here.  So legacy port addressing can only
> > work by exploiting aliases due to incomplete decoding of legacy ioport
> > addreses by the VT82C586 - but direct addressing is impossible.
> 
> Ok, that explains how the "fix" that we reverted worked. It caused crap
> to be added to the top bits of the address :-)
> 
> So here, what you really want to do is not a call to
> pcibios_resource_to_bus(), but you actually want to use a different bus
> address in the first place, that you know the HW will decode the same
> way.
> 
> The best way to achieve that imho, is to do a header quirk that is run
> just after the generic probe code, which offsets the fixed legacy
> resources by 0x10000000 since that's really the bus address you are
> going to emit.
> 
> Later on, your pcibios_fixup code should take that remove 0x10000000
> from all IO resources, since your 0xd0000000 mapping already maps
> 0x10000000 as you probably already do.
> 
> The trick is, you don't want to convert a "resource" into a "bus
> address" here, but really issue a different bus address.

Oh well, I've cooked up a patch and posted it to linux-mips for testing.

  Ralf

Re: Please revert: PCI: fix IDE legacy mode resources

[Benjamin Herrenschmidt]

On Mon, 2007-12-10 at 15:01 +0000, Alan Cox wrote:
> 
> If the hardware cannot map the low PCI space then set
> CONFIG_NO_ATA_LEGACY and the ATA layer will leave legacy ports alone.
> Unfortunately its not clear we can make that mode try and force
> controllers into native. We could try that if the MIPS and PPC people
> want ?

Forcing controllers into native mode tends to be something that really
only works on -some- controllers. I'm happy to have a hack to try to do
that on all of them on powermacs, because the range of controllers that
might not be in native mode in the first place there is pretty small,
and for CHRP briq, I do it for a specific known controller only.

I think if we start trying to do it for all of them, we'll get into
trouble. Some can't, some might look like they did but still route
interrupts the legacy way, etc...

I think that for that specific problem, a quirk might be the solution
MIPS wants. You can maybe use the same quirk I've posted in my other
patch for CHRP...

Cheers,
Ben.

Re: Please revert: PCI: fix IDE legacy mode resources

[Alan Cox]

> Forcing controllers into native mode tends to be something that really
> only works on -some- controllers. I'm happy to have a hack to try to do
> that on all of them on powermacs, because the range of controllers that
> might not be in native mode in the first place there is pretty small,
> and for CHRP briq, I do it for a specific known controller only.

I'm thinking of doing this solely if the platform has
CONFIG_ATA_NO_LEGACY set. In other words we'd only try this stunt on a
system we *know* cannot address the low PCI space ports.

Re: Please revert: PCI: fix IDE legacy mode resources

[Benjamin Herrenschmidt]

On Mon, 2007-12-10 at 23:07 +0000, Alan Cox wrote:
> > Forcing controllers into native mode tends to be something that really
> > only works on -some- controllers. I'm happy to have a hack to try to do
> > that on all of them on powermacs, because the range of controllers that
> > might not be in native mode in the first place there is pretty small,
> > and for CHRP briq, I do it for a specific known controller only.
> 
> I'm thinking of doing this solely if the platform has
> CONFIG_ATA_NO_LEGACY set. In other words we'd only try this stunt on a
> system we *know* cannot address the low PCI space ports.

Allright. I don't set CONFIG_ATA_NO_LEGACY on powerpc anyway, as I do
support legacy ATA just fine on a range of machines. 

For example, Pegasos does the a quirk the other way around which is to
put it back the VIA IDE into legacy mode as there are issues with the
way that VIA chipset is configured on those machines.

It's mostly a matter of making sure for me that the IRQ routing match
what the platform code is set to deal with or that sort of thing as
unfortunately, anything that involves legacy stuff is still pretty much
full of hacks.

Ben.

Re: Please revert: PCI: fix IDE legacy mode resources

[Ralf Baechle]

On Mon, Dec 10, 2007 at 03:29:22PM +1100, Benjamin Herrenschmidt wrote:

> To: Alan Cox <alan@lxorguk.ukuu.org.uk>,
> 	Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>,
> 	Yoichi Yuasa <yoichi_yuasa@tripeaks.co.jp>,
> 	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
> 	Greg KH <gregkh@suse.de>,
> 	Linus Torvalds <torvalds@linux-foundation.org>,
> 	Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Subject: Re: Please revert: PCI: fix IDE legacy mode resources
> Content-Type: text/plain
> 
> powerpc: Fix IDE legacy vs. native fixups
> 
> PowerMac and CHRP/BriQ platforms have quirks to switch some IDE
> controllers from legacy mode to fully native mode. Those quirks
> however will not work properly anymore due to a change to the
> generic code to better handle legacy IDE resources.
> 
> This fixes it by moving those quirk to "early" quirks (so they
> run before resources are probed for the devices) and clearing
> all BARs after the conversion to force a reallocation of sane
> values.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> 
> To be totally correct, we still need to also revert
> commit fd6e732186ab522c812ab19c2c5e5befb8ec8115 which
> is bogus.
> 
> Linus, can you still apply this to 2.6.24 ? I would also like the
> above (fd6e...) reverted as so far, nobody have come up with a demonstration
> that it's not bogus.

For the revert:

Acked-by: Ralf Baechle <ralf@linux-mips.org>

I'll work with Yoichi to find another solution.

  Ralf

Re: Please revert: PCI: fix IDE legacy mode resources

[Ralf Baechle]

On Sun, Dec 09, 2007 at 06:24:51PM +1100, Benjamin Herrenschmidt wrote:

> If that is the case though (that is it can't issue low ioport cycles),
> how would have the fd6e7321...  worked in the first place ? Hrm...
> strange. My understanding is that all that patch does is put junk in the
> pci_dev resource structures :-) Maybe that's enough to cause the PCI
> layer later on to be unhappy about them and reassign the BARs to some
> place that works ? In which case, you are right, a better approach is a
> quirk on this specific platform, or even better, mark 0...0x10000000
> busy in ioport_resources and let the generic code clash & re-assign...
> 
> I must admit I'm a bit confused tho...
> 
> Anyway, so far, nobody is arguing in favor of keeping this patch in nor
> so far trying and explanation on why it wouldn't be totally bogus, so I
> suggest we revert it :-)

I certainly don't oppose at this point.

  Ralf

Re: Please revert: PCI: fix IDE legacy mode resources

[Ralf Baechle]

On Thu, Dec 06, 2007 at 02:58:13PM +0900, Yoichi Yuasa wrote:

> > What I don't understand is thus why you are calling resource_to_bus on 0x1f0
> > which is -not- a resource value, but is already a BAR value...
> 
> 0x1f0 is resource value on MIPS Cobalt.
> All RAW BAR values contain the offset(0x10000000) on it.

In arch/mips/cobalt/pci.c:

static struct resource cobalt_io_resource = {
        .start  = 0x1000,
        .end    = GT_DEF_PCI0_IO_SIZE - 1,
        .name   = "PCI I/O",
        .flags  = IORESOURCE_IO,
};

static struct pci_controller cobalt_pci_controller = {
	[...]
        .io_resource    = &cobalt_io_resource,
        .io_offset      = 0 - GT_DEF_PCI0_IO_BASE,
};

The .io_offset initialization looks odd; no other platform is trying to
use a negative offset here.  I think there was something odd with the
Galileo GT-64111 which (unlike its later sucessors such as the GT-64120)
passes memory accesses in the range 0x10000000 ... 0x11ffffff to the PCI
bus unmodified.  That is for example a load or store to physical address
0x100003f8 will become an I/O port access to 0x100003f8.

I hope this is just a missconfiguration of the GT-64111, time for manual
reading.

  Ralf

Re: Please revert: PCI: fix IDE legacy mode resources

[Ralf Baechle]

On Thu, Dec 06, 2007 at 12:32:54PM +0000, Ralf Baechle wrote:

> > > What I don't understand is thus why you are calling resource_to_bus on 0x1f0
> > > which is -not- a resource value, but is already a BAR value...
> > 
> > 0x1f0 is resource value on MIPS Cobalt.
> > All RAW BAR values contain the offset(0x10000000) on it.
> 
> In arch/mips/cobalt/pci.c:
> 
> static struct resource cobalt_io_resource = {
>         .start  = 0x1000,
>         .end    = GT_DEF_PCI0_IO_SIZE - 1,
>         .name   = "PCI I/O",
>         .flags  = IORESOURCE_IO,
> };
> 
> static struct pci_controller cobalt_pci_controller = {
> 	[...]
>         .io_resource    = &cobalt_io_resource,
>         .io_offset      = 0 - GT_DEF_PCI0_IO_BASE,
> };
> 
> The .io_offset initialization looks odd; no other platform is trying to
> use a negative offset here.  I think there was something odd with the
> Galileo GT-64111 which (unlike its later sucessors such as the GT-64120)
> passes memory accesses in the range 0x10000000 ... 0x11ffffff to the PCI
> bus unmodified.  That is for example a load or store to physical address
> 0x100003f8 will become an I/O port access to 0x100003f8.
> 
> I hope this is just a missconfiguration of the GT-64111, time for manual
> reading.

The GT-64120 has remapping register which the GT-64111 is lacking.  So
the way things are I don't see how it would be possibly to generate an
I/O port address below GT_DEF_PCI0_IO_BASE (0x10000000) on that system
controller.

Which of course mean any sort of device that requires legacy I/O port
addressing is out of question unless one intentionally (ab-)uses aliases
due to only partial decoding of the I/O port address.  Which would mean
no chance of using legacy I/O port devices with the Galileo.

There must be something I'm missing here, I just can't believe somebody
could possibly design such a lunatic system controller.

  Ralf