diff mbox

PCI: keystone: add a pci quirk to limit mrrs

Message ID 1407338300-9146-1-git-send-email-m-karicheri2@ti.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Murali Karicheri Aug. 6, 2014, 3:18 p.m. UTC
Keystone PCI controller has a limitation that memory read request
size must not exceed 256 bytes. This is a hardware limitation and
add a quirk to force this limit on all downstream devices by
updating mrrs.

Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
---
 This applies on top of the Keystone PCI controller patch series
 at http://thread.gmane.org/gmane.linux.kernel.pci/33523
 drivers/pci/host/pci-keystone.c |   40 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

Comments

Jason Gunthorpe Aug. 6, 2014, 4:30 p.m. UTC | #1
On Wed, Aug 06, 2014 at 11:18:20AM -0400, Murali Karicheri wrote:
> Keystone PCI controller has a limitation that memory read request
> size must not exceed 256 bytes. This is a hardware limitation and
> add a quirk to force this limit on all downstream devices by
> updating mrrs.

Does this still work if the tuning is enabled, or does the tuning run
after this?

> +			if (pcie_get_readrq(dev) > 256) {
> +				pr_info("limiting mrrs to 256\n");
> +				pcie_set_readrq(dev, 256);

The pr_info should either go away, or at least print the PCI bdf..

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
Murali Karicheri Aug. 6, 2014, 4:56 p.m. UTC | #2
On 08/06/2014 12:30 PM, Jason Gunthorpe wrote:
> On Wed, Aug 06, 2014 at 11:18:20AM -0400, Murali Karicheri wrote:
>> Keystone PCI controller has a limitation that memory read request
>> size must not exceed 256 bytes. This is a hardware limitation and
>> add a quirk to force this limit on all downstream devices by
>> updating mrrs.
>
> Does this still work if the tuning is enabled, or does the tuning run
> after this?

Yes it works with tuning enabled. Tuning happens afterwards. The 
'limiting mrrs to 256' below is from my patch.


[    2.267670] limiting mrrs to 256
[    2.267698] limiting mrrs to 256
[    2.267808] pcieport 0000:00:00.0: BAR 8: assigned [mem 
0x50000000-0x500fffff]
[    2.267818] pcieport 0000:00:00.0: BAR 9: assigned [mem 
0x50100000-0x501fffff pref]
[    2.267827] pcieport 0000:00:00.0: BAR 7: assigned [io  0x1000-0x1fff]
[    2.267840] pci 0000:01:00.0: BAR 0: assigned [mem 0x50000000-0x5001ffff]
[    2.267855] pci 0000:01:00.0: BAR 1: assigned [mem 0x50020000-0x5003ffff]
[    2.267869] pci 0000:01:00.0: BAR 6: assigned [mem 
0x50100000-0x5011ffff pref]
[    2.267877] pci 0000:01:00.1: BAR 0: assigned [mem 0x50040000-0x5005ffff]
[    2.267891] pci 0000:01:00.1: BAR 1: assigned [mem 0x50060000-0x5007ffff]
[    2.267904] pci 0000:01:00.1: BAR 6: assigned [mem 
0x50120000-0x5013ffff pref]
[    2.267913] pci 0000:01:00.0: BAR 2: assigned [io  0x1000-0x101f]
[    2.267926] pci 0000:01:00.1: BAR 2: assigned [io  0x1020-0x103f]
[    2.267946] pcieport 0000:00:00.0: Max Payload Size set to  256/ 256 
(was  128), Max Read Rq  256
[    2.267980] pci 0000:01:00.0: Max Payload Size set to  256/ 256 (was 
  128), Max Read Rq  256
[    2.268013] pci 0000:01:00.1: Max Payload Size set to  256/ 256 (was 
  128), Max Read Rq  256


>
>> +			if (pcie_get_readrq(dev)>  256) {
>> +				pr_info("limiting mrrs to 256\n");
>> +				pcie_set_readrq(dev, 256);
>
> The pr_info should either go away, or at least print the PCI bdf..

I will change this to a dev_info and print the PCI bdf as part of it.

>
> 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
Jason Gunthorpe Aug. 6, 2014, 4:58 p.m. UTC | #3
On Wed, Aug 06, 2014 at 12:56:04PM -0400, Murali Karicheri wrote:
> On 08/06/2014 12:30 PM, Jason Gunthorpe wrote:
> >On Wed, Aug 06, 2014 at 11:18:20AM -0400, Murali Karicheri wrote:
> >>Keystone PCI controller has a limitation that memory read request
> >>size must not exceed 256 bytes. This is a hardware limitation and
> >>add a quirk to force this limit on all downstream devices by
> >>updating mrrs.
> >
> >Does this still work if the tuning is enabled, or does the tuning run
> >after this?
> 
> Yes it works with tuning enabled. Tuning happens afterwards. The
> 'limiting mrrs to 256' below is from my patch.

That seems backwards to me...

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
Murali Karicheri Aug. 6, 2014, 5:09 p.m. UTC | #4
On 08/06/2014 12:58 PM, Jason Gunthorpe wrote:
> On Wed, Aug 06, 2014 at 12:56:04PM -0400, Murali Karicheri wrote:
>> On 08/06/2014 12:30 PM, Jason Gunthorpe wrote:
>>> On Wed, Aug 06, 2014 at 11:18:20AM -0400, Murali Karicheri wrote:
>>>> Keystone PCI controller has a limitation that memory read request
>>>> size must not exceed 256 bytes. This is a hardware limitation and
>>>> add a quirk to force this limit on all downstream devices by
>>>> updating mrrs.
>>>
>>> Does this still work if the tuning is enabled, or does the tuning run
>>> after this?
>>
>> Yes it works with tuning enabled. Tuning happens afterwards. The
>> 'limiting mrrs to 256' below is from my patch.
>
> That seems backwards to me...
Rational? The tuning is reading mrrs and set mps to less than or equal 
to mrss. So adding this before make sure mrrs used is below keystones's 
limit.

Murali
>
> 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
Jason Gunthorpe Aug. 6, 2014, 5:30 p.m. UTC | #5
On Wed, Aug 06, 2014 at 01:09:41PM -0400, Murali Karicheri wrote:

> >>Yes it works with tuning enabled. Tuning happens afterwards. The
> >>'limiting mrrs to 256' below is from my patch.
> >
> >That seems backwards to me...
> Rational? The tuning is reading mrrs and set mps to less than or
> equal to mrss. So adding this before make sure mrrs used is below
> keystones's limit.

The tuning process adjusts the parameters however it sees fit, todays
algorithm might not increase a BIOS set MRRS, but tomorrow's could.

The quirk should be after the tuning to ensure the MRRS is limited..

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
Murali Karicheri Aug. 6, 2014, 7:05 p.m. UTC | #6
On 08/06/2014 01:30 PM, Jason Gunthorpe wrote:
> On Wed, Aug 06, 2014 at 01:09:41PM -0400, Murali Karicheri wrote:
>
>>>> Yes it works with tuning enabled. Tuning happens afterwards. The
>>>> 'limiting mrrs to 256' below is from my patch.
>>>
>>> That seems backwards to me...
>> Rational? The tuning is reading mrrs and set mps to less than or
>> equal to mrss. So adding this before make sure mrrs used is below
>> keystones's limit.
>
> The tuning process adjusts the parameters however it sees fit, todays
> algorithm might not increase a BIOS set MRRS, but tomorrow's could.
>
> The quirk should be after the tuning to ensure the MRRS is limited..
>
> Jason
Ok. I see it.

The latest possible quirk application point seems to be before enabling
the ep device. A change in macro as below will achieve this and it works.

-DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, quirk_limit_mrrs);
+DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, quirk_limit_mrrs);

If this looks reasonable, I will repost my patch with this change.

regards,

Murali
--
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
diff mbox

Patch

diff --git a/drivers/pci/host/pci-keystone.c b/drivers/pci/host/pci-keystone.c
index c1cfaef..541ab24 100644
--- a/drivers/pci/host/pci-keystone.c
+++ b/drivers/pci/host/pci-keystone.c
@@ -42,8 +42,48 @@ 
 /* DEV_STAT_CTRL */
 #define PCIE_CAP_BASE		0x70
 
+/* PCIE controller device IDs */
+#define PCIE_RC_K2HK		0xb008
+#define PCIE_RC_K2E		0xb009
+#define PCIE_RC_K2L		0xb00a
+
 #define to_keystone_pcie(x)	container_of(x, struct keystone_pcie, pp)
 
+static void quirk_limit_mrrs(struct pci_dev *dev)
+{
+	struct pci_bus *bus = dev->bus;
+	struct pci_dev *bridge = bus->self;
+
+	if (pci_is_root_bus(bus))
+		return;
+
+	/* look for the host bridge */
+	while (!pci_is_root_bus(bus)) {
+		bridge = bus->self;
+		bus = bus->parent;
+	}
+
+	if (bridge) {
+		u16 id;
+
+		/*
+		 * Keystone PCI controller has a h/w limitation of
+		 * 256 bytes maximum read request size. It can't handle
+		 * anything higher than this. So force this limit on
+		 * all downstream devices
+		 */
+		pci_read_config_word(bridge, PCI_DEVICE_ID, &id);
+		if ((id == PCIE_RC_K2HK) || (id == PCIE_RC_K2E) ||
+		    (id == PCIE_RC_K2L)) {
+			if (pcie_get_readrq(dev) > 256) {
+				pr_info("limiting mrrs to 256\n");
+				pcie_set_readrq(dev, 256);
+			}
+		}
+	}
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, quirk_limit_mrrs);
+
 static int ks_pcie_establish_link(struct keystone_pcie *ks_pcie)
 {
 	struct pcie_port *pp = &ks_pcie->pp;