diff mbox

[3.11-rc] pci: mvebu: disable prefetchable memory support in PCI-to-PCI bridge

Message ID 1375364659-3228-1-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Thomas Petazzoni Aug. 1, 2013, 1:44 p.m. UTC 
The Marvell PCIe driver uses an emulated PCI-to-PCI bridge to be able
to dynamically set up MBus address decoding windows for PCI I/O and
memory regions depending on the PCI devices enumerated by Linux.

However, this emulated PCI-to-PCI bridge logic makes the Linux PCI
core believe that prefetchable memory regions are supported (because
the registers are read/write), while in fact no adress decoding window
is ever created for such regions. Since the Marvell MBus address
decoding windows do not distinguish memory regions and prefetchable
memory regions, this patch takes a simple approach: change the
PCI-to-PCI bridge emulation to let the Linux PCI core know that we
don't support prefetchable memory regions.

To achieve this, we simply make the prefetchable memory base a
read-only register that always returns 0. Reading/writing all the
other prefetchable memory related registers has no effect.

This problem was originally reported by Finn Hoffmann
<finn@uni-bremen.de>, who couldn't get a RTL8111/8168B PCI NIC working
on the NSA310 Kirkwood platform after updating to 3.11-rc. The problem
was that the PCI-to-PCI bridge emulation was making the Linux PCI core
believe that we support prefetchable memory, so the Linux PCI core was
only filling the prefetchable memory base and limit registers, which
does not lead to a MBus window being created. The below patch has been
confirmed by Finn Hoffmann to fix his problem on Kirkwood, and has
otherwise been successfully tested on the Armada XP GP platform with a
e1000e PCIe NIC and a Marvell SATA PCIe card.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Reported-by: Finn Hoffmann <finn@uni-bremen.de>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
This patch fixes a regression introduced in 3.11-rc, so it should be
applied to 3.11.
---
 drivers/pci/host/pci-mvebu.c | 27 +--------------------------
 1 file changed, 1 insertion(+), 26 deletions(-)

Comments

Bjorn Helgaas Aug. 1, 2013, 4:53 p.m. UTC | #1 
On Thu, Aug 1, 2013 at 7:44 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> The Marvell PCIe driver uses an emulated PCI-to-PCI bridge to be able
> to dynamically set up MBus address decoding windows for PCI I/O and
> memory regions depending on the PCI devices enumerated by Linux.
>
> However, this emulated PCI-to-PCI bridge logic makes the Linux PCI
> core believe that prefetchable memory regions are supported (because
> the registers are read/write), while in fact no adress decoding window
> is ever created for such regions. Since the Marvell MBus address
> decoding windows do not distinguish memory regions and prefetchable
> memory regions, this patch takes a simple approach: change the
> PCI-to-PCI bridge emulation to let the Linux PCI core know that we
> don't support prefetchable memory regions.
>
> To achieve this, we simply make the prefetchable memory base a
> read-only register that always returns 0. Reading/writing all the
> other prefetchable memory related registers has no effect.
>
> This problem was originally reported by Finn Hoffmann
> <finn@uni-bremen.de>, who couldn't get a RTL8111/8168B PCI NIC working
> on the NSA310 Kirkwood platform after updating to 3.11-rc. The problem
> was that the PCI-to-PCI bridge emulation was making the Linux PCI core
> believe that we support prefetchable memory, so the Linux PCI core was
> only filling the prefetchable memory base and limit registers, which
> does not lead to a MBus window being created. The below patch has been
> confirmed by Finn Hoffmann to fix his problem on Kirkwood, and has
> otherwise been successfully tested on the Armada XP GP platform with a
> e1000e PCIe NIC and a Marvell SATA PCIe card.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Reported-by: Finn Hoffmann <finn@uni-bremen.de>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Please merge along with your other pci-mvebu.c changes.

> ---
> This patch fixes a regression introduced in 3.11-rc, so it should be
> applied to 3.11.
> ---
>  drivers/pci/host/pci-mvebu.c | 27 +--------------------------
>  1 file changed, 1 insertion(+), 26 deletions(-)
>
> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> index 13a633b..7bf3926 100644
> --- a/drivers/pci/host/pci-mvebu.c
> +++ b/drivers/pci/host/pci-mvebu.c
> @@ -86,10 +86,6 @@ struct mvebu_sw_pci_bridge {
>         u16 secondary_status;
>         u16 membase;
>         u16 memlimit;
> -       u16 prefmembase;
> -       u16 prefmemlimit;
> -       u32 prefbaseupper;
> -       u32 preflimitupper;
>         u16 iobaseupper;
>         u16 iolimitupper;
>         u8 cappointer;
> @@ -419,15 +415,7 @@ static int mvebu_sw_pci_bridge_read(struct mvebu_pcie_port *port,
>                 break;
>
>         case PCI_PREF_MEMORY_BASE:
> -               *value = (bridge->prefmemlimit << 16 | bridge->prefmembase);
> -               break;
> -
> -       case PCI_PREF_BASE_UPPER32:
> -               *value = bridge->prefbaseupper;
> -               break;
> -
> -       case PCI_PREF_LIMIT_UPPER32:
> -               *value = bridge->preflimitupper;
> +               *value = 0;
>                 break;
>
>         case PCI_IO_BASE_UPPER16:
> @@ -501,19 +489,6 @@ static int mvebu_sw_pci_bridge_write(struct mvebu_pcie_port *port,
>                 mvebu_pcie_handle_membase_change(port);
>                 break;
>
> -       case PCI_PREF_MEMORY_BASE:
> -               bridge->prefmembase = value & 0xffff;
> -               bridge->prefmemlimit = value >> 16;
> -               break;
> -
> -       case PCI_PREF_BASE_UPPER32:
> -               bridge->prefbaseupper = value;
> -               break;
> -
> -       case PCI_PREF_LIMIT_UPPER32:
> -               bridge->preflimitupper = value;
> -               break;
> -
>         case PCI_IO_BASE_UPPER16:
>                 bridge->iobaseupper = value & 0xffff;
>                 bridge->iolimitupper = value >> 16;
> --
> 1.8.1.2
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Petazzoni Aug. 1, 2013, 5:59 p.m. UTC | #2 
Dear Bjorn Helgaas,

On Thu, 1 Aug 2013 10:53:07 -0600, Bjorn Helgaas wrote:
> On Thu, Aug 1, 2013 at 7:44 AM, Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com> wrote:
> > The Marvell PCIe driver uses an emulated PCI-to-PCI bridge to be able
> > to dynamically set up MBus address decoding windows for PCI I/O and
> > memory regions depending on the PCI devices enumerated by Linux.
> >
> > However, this emulated PCI-to-PCI bridge logic makes the Linux PCI
> > core believe that prefetchable memory regions are supported (because
> > the registers are read/write), while in fact no adress decoding window
> > is ever created for such regions. Since the Marvell MBus address
> > decoding windows do not distinguish memory regions and prefetchable
> > memory regions, this patch takes a simple approach: change the
> > PCI-to-PCI bridge emulation to let the Linux PCI core know that we
> > don't support prefetchable memory regions.
> >
> > To achieve this, we simply make the prefetchable memory base a
> > read-only register that always returns 0. Reading/writing all the
> > other prefetchable memory related registers has no effect.
> >
> > This problem was originally reported by Finn Hoffmann
> > <finn@uni-bremen.de>, who couldn't get a RTL8111/8168B PCI NIC working
> > on the NSA310 Kirkwood platform after updating to 3.11-rc. The problem
> > was that the PCI-to-PCI bridge emulation was making the Linux PCI core
> > believe that we support prefetchable memory, so the Linux PCI core was
> > only filling the prefetchable memory base and limit registers, which
> > does not lead to a MBus window being created. The below patch has been
> > confirmed by Finn Hoffmann to fix his problem on Kirkwood, and has
> > otherwise been successfully tested on the Armada XP GP platform with a
> > e1000e PCIe NIC and a Marvell SATA PCIe card.
> >
> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > Reported-by: Finn Hoffmann <finn@uni-bremen.de>
> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> Please merge along with your other pci-mvebu.c changes.

I don't think we have other pci-mvebu.c lined up for 3.11. All the MSI
stuff, mvebu-mbus DT stuff and so on is for 3.12.

So I guess you could take that one through your pci tree. Jason, can
you confirm?

Thanks!

Thomas
Bjorn Helgaas Aug. 1, 2013, 8:51 p.m. UTC | #3 
On Thu, Aug 1, 2013 at 11:59 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Bjorn Helgaas,
>
> On Thu, 1 Aug 2013 10:53:07 -0600, Bjorn Helgaas wrote:
>> On Thu, Aug 1, 2013 at 7:44 AM, Thomas Petazzoni
>> <thomas.petazzoni@free-electrons.com> wrote:
>> > The Marvell PCIe driver uses an emulated PCI-to-PCI bridge to be able
>> > to dynamically set up MBus address decoding windows for PCI I/O and
>> > memory regions depending on the PCI devices enumerated by Linux.
>> >
>> > However, this emulated PCI-to-PCI bridge logic makes the Linux PCI
>> > core believe that prefetchable memory regions are supported (because
>> > the registers are read/write), while in fact no adress decoding window
>> > is ever created for such regions. Since the Marvell MBus address
>> > decoding windows do not distinguish memory regions and prefetchable
>> > memory regions, this patch takes a simple approach: change the
>> > PCI-to-PCI bridge emulation to let the Linux PCI core know that we
>> > don't support prefetchable memory regions.
>> >
>> > To achieve this, we simply make the prefetchable memory base a
>> > read-only register that always returns 0. Reading/writing all the
>> > other prefetchable memory related registers has no effect.
>> >
>> > This problem was originally reported by Finn Hoffmann
>> > <finn@uni-bremen.de>, who couldn't get a RTL8111/8168B PCI NIC working
>> > on the NSA310 Kirkwood platform after updating to 3.11-rc. The problem
>> > was that the PCI-to-PCI bridge emulation was making the Linux PCI core
>> > believe that we support prefetchable memory, so the Linux PCI core was
>> > only filling the prefetchable memory base and limit registers, which
>> > does not lead to a MBus window being created. The below patch has been
>> > confirmed by Finn Hoffmann to fix his problem on Kirkwood, and has
>> > otherwise been successfully tested on the Armada XP GP platform with a
>> > e1000e PCIe NIC and a Marvell SATA PCIe card.
>> >
>> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>> > Reported-by: Finn Hoffmann <finn@uni-bremen.de>
>> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>>
>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>>
>> Please merge along with your other pci-mvebu.c changes.
>
> I don't think we have other pci-mvebu.c lined up for 3.11. All the MSI
> stuff, mvebu-mbus DT stuff and so on is for 3.12.
>
> So I guess you could take that one through your pci tree. Jason, can
> you confirm?

Oh, I missed the fact that this was for v3.11.  I put it in my
for-linus branch.  Thanks!

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Gunthorpe Aug. 2, 2013, 5:43 p.m. UTC | #4 
On Thu, Aug 01, 2013 at 03:44:19PM +0200, Thomas Petazzoni wrote:
> The Marvell PCIe driver uses an emulated PCI-to-PCI bridge to be able
> to dynamically set up MBus address decoding windows for PCI I/O and
> memory regions depending on the PCI devices enumerated by Linux.
> 
> However, this emulated PCI-to-PCI bridge logic makes the Linux PCI
> core believe that prefetchable memory regions are supported (because
> the registers are read/write), while in fact no adress decoding window
> is ever created for such regions. Since the Marvell MBus address
> decoding windows do not distinguish memory regions and prefetchable
> memory regions, this patch takes a simple approach: change the
> PCI-to-PCI bridge emulation to let the Linux PCI core know that we
> don't support prefetchable memory regions.
> 
> To achieve this, we simply make the prefetchable memory base a
> read-only register that always returns 0. Reading/writing all the
> other prefetchable memory related registers has no effect.

Looks good to me. It is very good that you were able to merge the
downstream prefetchable BARs into the normal MMIO window.

Though looking at the original thread I really wonder if something
else is wrong here as well. The ethernet should not have only
prefetchable BARs.

For instance, I found this link:

https://bugzilla.redhat.com/show_bug.cgi?id=448712

Which shows a more resonable arrangement:

02:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168B PCI Express Gigabit Ethernet controller (rev 02)
        Region 0: I/O ports at e800 [size=256]
        Region 2: Memory at dffff000 (64-bit, non-prefetchable) [size=4K]
        Region 4: Memory at deff0000 (64-bit, prefetchable) [size=64K]
        Expansion ROM at dffc0000 [disabled] [size=128K]

vs Finn's:

01:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168B PCI Express Gigabit Ethernet controller (rev)
         Subsystem: Realtek Semiconductor Co., Ltd. TEG-ECTX Gigabit PCI-E Adapter [Trendnet]
         Flags: bus master, fast devsel, latency 0, IRQ 9
         I/O ports at 10000 [size=256]
         Memory at e0014000 (64-bit, prefetchable) [size=4K]
         Memory at e0010000 (64-bit, prefetchable) [size=16K]

Regards,
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Petazzoni Aug. 6, 2013, 10:36 a.m. UTC | #5 
Dear Jason Gunthorpe,

On Fri, 2 Aug 2013 11:43:50 -0600, Jason Gunthorpe wrote:

> > To achieve this, we simply make the prefetchable memory base a
> > read-only register that always returns 0. Reading/writing all the
> > other prefetchable memory related registers has no effect.
> 
> Looks good to me. It is very good that you were able to merge the
> downstream prefetchable BARs into the normal MMIO window.

Thanks.

> Though looking at the original thread I really wonder if something
> else is wrong here as well. The ethernet should not have only
> prefetchable BARs.
> 
> For instance, I found this link:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=448712
> 
> Which shows a more resonable arrangement:
> 
> 02:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168B PCI Express Gigabit Ethernet controller (rev 02)
>         Region 0: I/O ports at e800 [size=256]
>         Region 2: Memory at dffff000 (64-bit, non-prefetchable) [size=4K]
>         Region 4: Memory at deff0000 (64-bit, prefetchable) [size=64K]
>         Expansion ROM at dffc0000 [disabled] [size=128K]
> 
> vs Finn's:
> 
> 01:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168B PCI Express Gigabit Ethernet controller (rev)
>          Subsystem: Realtek Semiconductor Co., Ltd. TEG-ECTX Gigabit PCI-E Adapter [Trendnet]
>          Flags: bus master, fast devsel, latency 0, IRQ 9
>          I/O ports at 10000 [size=256]
>          Memory at e0014000 (64-bit, prefetchable) [size=4K]
>          Memory at e0010000 (64-bit, prefetchable) [size=16K]

Right, but Finn had the same output in 3.10, where the old Kirkwood PCI
driver was used. So it doesn't seem to be related to the driver, but
maybe to how the hardware works?

Thomas
diff mbox

Patch

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 13a633b..7bf3926 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -86,10 +86,6 @@  struct mvebu_sw_pci_bridge {
 	u16 secondary_status;
 	u16 membase;
 	u16 memlimit;
-	u16 prefmembase;
-	u16 prefmemlimit;
-	u32 prefbaseupper;
-	u32 preflimitupper;
 	u16 iobaseupper;
 	u16 iolimitupper;
 	u8 cappointer;
@@ -419,15 +415,7 @@  static int mvebu_sw_pci_bridge_read(struct mvebu_pcie_port *port,
 		break;
 
 	case PCI_PREF_MEMORY_BASE:
-		*value = (bridge->prefmemlimit << 16 | bridge->prefmembase);
-		break;
-
-	case PCI_PREF_BASE_UPPER32:
-		*value = bridge->prefbaseupper;
-		break;
-
-	case PCI_PREF_LIMIT_UPPER32:
-		*value = bridge->preflimitupper;
+		*value = 0;
 		break;
 
 	case PCI_IO_BASE_UPPER16:
@@ -501,19 +489,6 @@  static int mvebu_sw_pci_bridge_write(struct mvebu_pcie_port *port,
 		mvebu_pcie_handle_membase_change(port);
 		break;
 
-	case PCI_PREF_MEMORY_BASE:
-		bridge->prefmembase = value & 0xffff;
-		bridge->prefmemlimit = value >> 16;
-		break;
-
-	case PCI_PREF_BASE_UPPER32:
-		bridge->prefbaseupper = value;
-		break;
-
-	case PCI_PREF_LIMIT_UPPER32:
-		bridge->preflimitupper = value;
-		break;
-
 	case PCI_IO_BASE_UPPER16:
 		bridge->iobaseupper = value & 0xffff;
 		bridge->iolimitupper = value >> 16;