[PATCH v2] PCI: keystone: add a pci quirk to limit mrrs

[Bjorn Helgaas]

On Fri, Sep 5, 2014 at 2:30 PM, Murali Karicheri <m-karicheri2 at ti.com> wrote:
> On 09/05/2014 03:23 PM, Bjorn Helgaas wrote:
>>
>> On Fri, Aug 08, 2014 at 03:45:32PM -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.
>>>
>>> Signed-off-by: Murali Karicheri<m-karicheri2 at ti.com>
>>> ---
>>>   -v2: made the quirk happens after tuning
>>>   -v1: changed printk to indicate PCI bdf
>>>   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(+)
>>>
>>> diff --git a/drivers/pci/host/pci-keystone.c
>>> b/drivers/pci/host/pci-keystone.c
>>> index c1cfaef..a132622 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
>>
>>
>> A Root Complex doesn't appear in config space as a PCI device, so I think
>> these are actually Root Port devices, aren't they?
>
>
> Yes.
>
>>
>> I suppose the spec would allow Root Complex Integrated Endpoints, on which
>> MRRS could also be set, but I think your quirk ignores them because they
>> would appear on the root bus.  Is ignoring them OK, or would you want to
>> set MRRS for those as well?
>
>
> How does the Root Complex integrated endpoints are designed? Our PCI can act
> as a RC or EP and not both at the same time. So I guess this is not
> applicable.

My guess is that you probably don't have any integrated endpoints and
hence don't need to worry about MRRS for them.  The PCIe spec (r3.0,
sec 1.3.2.3) says "A Root Complex Integrated Endpoint is implemented
on internal logic of Root Complexes that contains [sic] the Root
Ports."

For example, my laptop has:

  00:03.0 Audio device: Intel Corporation Haswell-ULT HD Audio Controller
  00:1c.0 PCI bridge: Intel Corporation Lynx Point-LP PCI Express Root Port 1
  00:1c.3 PCI bridge: Intel Corporation Lynx Point-LP PCI Express Root Port 4

the 00:03.0 Audio device is a PCIe device but it is a sibling of the
Root Ports rather than being downstream from them, i.e., it's an
Integrated Endpoint that is part of the Root Complex itself.

>>> +
>>>   #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);
>>
>>
>> I think you should check the Vendor ID here as well.  Otherwise, there's a
>> possibility of applying this quirk to devices under a bridge with the same
>> Device ID but different Vendor ID.
>>
>> And I think you should be able to use the bridge->vendor and
>> bridge->device
>> fields instead of reading them from config space.  Actually, this might be
>> a good place to use pci_match_id().
>
>
> Ok.
>
>
>>
>>> +               if ((id == PCIE_RC_K2HK) || (id == PCIE_RC_K2E) ||
>>> +                   (id == PCIE_RC_K2L)) {
>>> +                       if (pcie_get_readrq(dev)>  256) {
>>> +                               dev_info(&dev->dev, "limiting mrrs to
>>> 256\n");
>>> +                               pcie_set_readrq(dev, 256);
>>> +                       }
>>> +               }
>>> +       }
>>> +}
>>> +DECLARE_PCI_FIXUP_ENABLE(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;
>>> --
>>> 1.7.9.5
>>>
>