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

[Murali Karicheri <m-karicheri2@ti.com>]

On 09/05/2014 04:40 PM, Bjorn Helgaas wrote:
> On Fri, Sep 5, 2014 at 2:30 PM, Murali Karicheri<m-karicheri2@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@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.
Ok. Got it. This is not applicable for Keystone PCI RC.

Regards,

Murali
>
>>>> +
>>>>    #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
>>>>
>>