| Message ID | 1407527132-3294-1-git-send-email-m-karicheri2@ti.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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? 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? > + > #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(). > + 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 > -- 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
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. > >> + >> #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 >> -- 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
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. >>> + >>> #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 >>> > -- 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
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 >>>> >> -- 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 --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 + #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) { + 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;
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(+)