| Message ID | 1446735481-27326-1-git-send-email-tharvey@gateworks.com |
|---|---|
| State | Superseded |
| Headers | show |
[Adding Richard Zhu] On Thu, Nov 5, 2015 at 12:58 PM, Tim Harvey <tharvey@gateworks.com> wrote: > Freescale has stated [1] that the LVDS clock source of the IMX6 does not pass > the PCI Gen2 clock jitter test, therefore unless an external Gen2 compliant > external clock source is present and supplied back to the IMX6 PCIe core > via LVDS CLK1/CLK2 you can not claim Gen2 compliance. > > Add a dt property to specify gen1 vs gen2 and check this before allowing > a Gen2 link. > > We default to Gen1 if the property is not present because at this time there > are no IMX6 boards in mainline that 'input' a clock on LVDS CLK1/CLK2. > > In order to be Gen2 compliant on IMX6 you need to: > - have a Gen2 compliant external clock generator and route that clock back > to either LVDS CLK1 or LVDS CLK2 as an input. > (see IMX6SX-SabreSD reference design) > - specify this clock in the pcie node in the dt > (ie IMX6QDL_CLK_LVDS1_IN or IMX6QDL_CLK_LVDS2_IN instead of > IMX6QDL_CLK_LVDS1_GATE which configures it as a CLK output) > > [1] https://community.freescale.com/message/453209 > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > This is an RFC because I'm assuming the decision to default to Gen1 link only > is going to ruffle some feathers. My understanding is that if you do not > use an external Gen2 compliant clockgen for peripepherals 'and' the IMX6 core > you should not claim Gen2 compliance. This was not obvious on original IMX6 > reference designs and I believe the jitter issue was discovered by Freescale > later and future reference designs were modified to state you need an ext > clockgen for Gen2 compliance. Looks good to me: Reviewed-by: Fabio Estevam <fabio.estevam@freescale.com> -- 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
SXQncyBva2F5IGZvciBtZSB0b28uDQpBY2tlZC1ieTogUmljaGFyZCBaaHUgPFJpY2hhcmQuWmh1 QGZyZWVzY2FsZS5jb20+DQpUaGFua3MgVGltLg0KDQpGcmVlc2NhbGUgTGludXggQlNQIHRlYW0N CkJlc3QgUmVnYXJkcw0KUmljaGFyZCBaaHUNCg0KLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0N CkZyb206IEZhYmlvIEVzdGV2YW0gW21haWx0bzpmZXN0ZXZhbUBnbWFpbC5jb21dIA0KU2VudDog VGh1cnNkYXksIE5vdmVtYmVyIDA1LCAyMDE1IDExOjQxIFBNDQpUbzogVGltIEhhcnZleQ0KQ2M6 IEx1Y2FzIFN0YWNoOyBCam9ybiBIZWxnYWFzOyBsaW51eC1wY2lAdmdlci5rZXJuZWwub3JnOyBF c3RldmFtIEZhYmlvLVI0OTQ5NjsgWmh1IFJpY2hhcmQtUjY1MDM3DQpTdWJqZWN0OiBSZTogW1BB VENIIFJGQ10gUENJOiBpbXg2OiBhZGQgZHQgcHJvcCBmb3IgbGluayBnZW4sIGRlZmF1bHQgdG8g Z2VuMQ0KDQpbQWRkaW5nIFJpY2hhcmQgWmh1XQ0KDQpPbiBUaHUsIE5vdiA1LCAyMDE1IGF0IDEy OjU4IFBNLCBUaW0gSGFydmV5IDx0aGFydmV5QGdhdGV3b3Jrcy5jb20+IHdyb3RlOg0KPiBGcmVl c2NhbGUgaGFzIHN0YXRlZCBbMV0gdGhhdCB0aGUgTFZEUyBjbG9jayBzb3VyY2Ugb2YgdGhlIElN WDYgZG9lcyANCj4gbm90IHBhc3MgdGhlIFBDSSBHZW4yIGNsb2NrIGppdHRlciB0ZXN0LCB0aGVy ZWZvcmUgdW5sZXNzIGFuIGV4dGVybmFsIA0KPiBHZW4yIGNvbXBsaWFudCBleHRlcm5hbCBjbG9j ayBzb3VyY2UgaXMgcHJlc2VudCBhbmQgc3VwcGxpZWQgYmFjayB0byANCj4gdGhlIElNWDYgUENJ ZSBjb3JlIHZpYSBMVkRTIENMSzEvQ0xLMiB5b3UgY2FuIG5vdCBjbGFpbSBHZW4yIGNvbXBsaWFu Y2UuDQo+DQo+IEFkZCBhIGR0IHByb3BlcnR5IHRvIHNwZWNpZnkgZ2VuMSB2cyBnZW4yIGFuZCBj aGVjayB0aGlzIGJlZm9yZSANCj4gYWxsb3dpbmcgYSBHZW4yIGxpbmsuDQo+DQo+IFdlIGRlZmF1 bHQgdG8gR2VuMSBpZiB0aGUgcHJvcGVydHkgaXMgbm90IHByZXNlbnQgYmVjYXVzZSBhdCB0aGlz IHRpbWUgDQo+IHRoZXJlIGFyZSBubyBJTVg2IGJvYXJkcyBpbiBtYWlubGluZSB0aGF0ICdpbnB1 dCcgYSBjbG9jayBvbiBMVkRTIENMSzEvQ0xLMi4NCj4NCj4gSW4gb3JkZXIgdG8gYmUgR2VuMiBj b21wbGlhbnQgb24gSU1YNiB5b3UgbmVlZCB0bzoNCj4gIC0gaGF2ZSBhIEdlbjIgY29tcGxpYW50 IGV4dGVybmFsIGNsb2NrIGdlbmVyYXRvciBhbmQgcm91dGUgdGhhdCBjbG9jayBiYWNrDQo+ICAg IHRvIGVpdGhlciBMVkRTIENMSzEgb3IgTFZEUyBDTEsyIGFzIGFuIGlucHV0Lg0KPiAgICAoc2Vl IElNWDZTWC1TYWJyZVNEIHJlZmVyZW5jZSBkZXNpZ24pDQo+ICAtIHNwZWNpZnkgdGhpcyBjbG9j ayBpbiB0aGUgcGNpZSBub2RlIGluIHRoZSBkdA0KPiAgICAoaWUgSU1YNlFETF9DTEtfTFZEUzFf SU4gb3IgSU1YNlFETF9DTEtfTFZEUzJfSU4gaW5zdGVhZCBvZg0KPiAgICAgSU1YNlFETF9DTEtf TFZEUzFfR0FURSB3aGljaCBjb25maWd1cmVzIGl0IGFzIGEgQ0xLIG91dHB1dCkNCj4NCj4gWzFd IGh0dHBzOi8vY29tbXVuaXR5LmZyZWVzY2FsZS5jb20vbWVzc2FnZS80NTMyMDkNCj4NCj4gU2ln bmVkLW9mZi1ieTogVGltIEhhcnZleSA8dGhhcnZleUBnYXRld29ya3MuY29tPg0KPg0KPiBUaGlz IGlzIGFuIFJGQyBiZWNhdXNlIEknbSBhc3N1bWluZyB0aGUgZGVjaXNpb24gdG8gZGVmYXVsdCB0 byBHZW4xIA0KPiBsaW5rIG9ubHkgaXMgZ29pbmcgdG8gcnVmZmxlIHNvbWUgZmVhdGhlcnMuIE15 IHVuZGVyc3RhbmRpbmcgaXMgdGhhdCANCj4gaWYgeW91IGRvIG5vdCB1c2UgYW4gZXh0ZXJuYWwg R2VuMiBjb21wbGlhbnQgY2xvY2tnZW4gZm9yIA0KPiBwZXJpcGVwaGVyYWxzICdhbmQnIHRoZSBJ TVg2IGNvcmUgeW91IHNob3VsZCBub3QgY2xhaW0gR2VuMiANCj4gY29tcGxpYW5jZS4gVGhpcyB3 YXMgbm90IG9idmlvdXMgb24gb3JpZ2luYWwgSU1YNiByZWZlcmVuY2UgZGVzaWducyANCj4gYW5k IEkgYmVsaWV2ZSB0aGUgaml0dGVyIGlzc3VlIHdhcyBkaXNjb3ZlcmVkIGJ5IEZyZWVzY2FsZSBs YXRlciBhbmQgDQo+IGZ1dHVyZSByZWZlcmVuY2UgZGVzaWducyB3ZXJlIG1vZGlmaWVkIHRvIHN0 YXRlIHlvdSBuZWVkIGFuIGV4dCBjbG9ja2dlbiBmb3IgR2VuMiBjb21wbGlhbmNlLg0KDQpMb29r cyBnb29kIHRvIG1lOg0KDQpSZXZpZXdlZC1ieTogRmFiaW8gRXN0ZXZhbSA8ZmFiaW8uZXN0ZXZh bUBmcmVlc2NhbGUuY29tPg0K -- 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
Am Donnerstag, den 05.11.2015, 06:58 -0800 schrieb Tim Harvey: > Freescale has stated [1] that the LVDS clock source of the IMX6 does not pass > the PCI Gen2 clock jitter test, therefore unless an external Gen2 compliant > external clock source is present and supplied back to the IMX6 PCIe core > via LVDS CLK1/CLK2 you can not claim Gen2 compliance. > > Add a dt property to specify gen1 vs gen2 and check this before allowing > a Gen2 link. > I think I already said this in the last round: there is nothing vendor specific in a max-link-speed property. I would really like to have this as a common DW PCIe property right from the beginning, so that we don't burden us with keeping backward compatibility with vendor specific properties when this moves to common code eventually. The only difference between imx6 and all other DW cores is that the absence of the property should mean unconstrained normally and constrained to gen1 for imx. > We default to Gen1 if the property is not present because at this time there > are no IMX6 boards in mainline that 'input' a clock on LVDS CLK1/CLK2. > > In order to be Gen2 compliant on IMX6 you need to: > - have a Gen2 compliant external clock generator and route that clock back > to either LVDS CLK1 or LVDS CLK2 as an input. > (see IMX6SX-SabreSD reference design) > - specify this clock in the pcie node in the dt > (ie IMX6QDL_CLK_LVDS1_IN or IMX6QDL_CLK_LVDS2_IN instead of > IMX6QDL_CLK_LVDS1_GATE which configures it as a CLK output) > > [1] https://community.freescale.com/message/453209 > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > This is an RFC because I'm assuming the decision to default to Gen1 link only > is going to ruffle some feathers. I think everyone is okay with that part. > My understanding is that if you do not > use an external Gen2 compliant clockgen for peripepherals 'and' the IMX6 core > you should not claim Gen2 compliance. This was not obvious on original IMX6 > reference designs and I believe the jitter issue was discovered by Freescale > later and future reference designs were modified to state you need an ext > clockgen for Gen2 compliance. > > --- > .../devicetree/bindings/pci/fsl,imx6q-pcie.txt | 3 +++ > drivers/pci/host/pci-imx6.c | 22 ++++++++++++++++------ > 2 files changed, 19 insertions(+), 6 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > index 6fbba53..7dff332 100644 > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > @@ -12,6 +12,8 @@ Required properties: > - "msi": The interrupt that is asserted when an MSI is received > - clock-names: Must include the following additional entries: > - "pcie_phy" > +- fsl,max-link-speed: Specify PCI gen. Defaults to 1, can be 2 if board has > + an external clock generator fed back to PCIe core. > > Example: > > @@ -37,4 +39,5 @@ Example: > <0 0 0 4 &intc GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&clks 144>, <&clks 206>, <&clks 189>; > clock-names = "pcie", "pcie_bus", "pcie_phy"; > + fsl,max-link-speed = <2>; > }; > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > index 22e8224..16412c4 100644 > --- a/drivers/pci/host/pci-imx6.c > +++ b/drivers/pci/host/pci-imx6.c > @@ -39,6 +39,7 @@ struct imx6_pcie { > struct pcie_port pp; > struct regmap *iomuxc_gpr; > void __iomem *mem_base; > + int link_cap; > }; > > /* PCIe Root Complex registers (memory-mapped) */ > @@ -393,11 +394,15 @@ static int imx6_pcie_establish_link(struct pcie_port *pp) > if (ret) > return ret; > > - /* Allow Gen2 mode after the link is up. */ > - tmp = readl(pp->dbi_base + PCIE_RC_LCR); > - tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; > - tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN2; > - writel(tmp, pp->dbi_base + PCIE_RC_LCR); > + if (imx6_pcie->link_cap == 2) { > + /* Allow Gen2 mode after the link is up. */ > + tmp = readl(pp->dbi_base + PCIE_RC_LCR); > + tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; > + tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN2; > + writel(tmp, pp->dbi_base + PCIE_RC_LCR); > + } else { > + dev_info(pp->dev, "Link: Gen2 disabled\n"); > + } > > /* > * Start Directed Speed Change so the best possible speed both link > @@ -421,7 +426,7 @@ static int imx6_pcie_establish_link(struct pcie_port *pp) > } > > tmp = readl(pp->dbi_base + PCIE_RC_LCSR); > - dev_dbg(pp->dev, "Link up, Gen=%i\n", (tmp >> 16) & 0xf); > + dev_info(pp->dev, "Link up, Gen%i\n", (tmp >> 16) & 0xf); > return 0; > } > > @@ -591,6 +596,11 @@ static int __init imx6_pcie_probe(struct platform_device *pdev) > } > } > > + /* default link capability to gen1 */ > + imx6_pcie->link_cap = 1; > + if (of_property_read_u32(np, "fsl,max-link-speed", &ret) == 0) > + imx6_pcie->link_cap = ret; > + > /* Fetch clocks */ > imx6_pcie->pcie_phy = devm_clk_get(&pdev->dev, "pcie_phy"); > if (IS_ERR(imx6_pcie->pcie_phy)) {
On Fri, Nov 6, 2015 at 1:36 AM, Lucas Stach <l.stach@pengutronix.de> wrote: > Am Donnerstag, den 05.11.2015, 06:58 -0800 schrieb Tim Harvey: >> Freescale has stated [1] that the LVDS clock source of the IMX6 does not pass >> the PCI Gen2 clock jitter test, therefore unless an external Gen2 compliant >> external clock source is present and supplied back to the IMX6 PCIe core >> via LVDS CLK1/CLK2 you can not claim Gen2 compliance. >> >> Add a dt property to specify gen1 vs gen2 and check this before allowing >> a Gen2 link. >> > I think I already said this in the last round: there is nothing vendor > specific in a max-link-speed property. I would really like to have this > as a common DW PCIe property right from the beginning, so that we don't > burden us with keeping backward compatibility with vendor specific > properties when this moves to common code eventually. Hi Lucas, I did discuss this patch with you off-list but I think I misunderstood your meaning then. So you are asking me to simply rename the properly 'fsl,max-link-speed' to 'max-link-speed' and the rest of the patch is good with you? Regards, Tim -- 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
Am Freitag, den 06.11.2015, 11:59 -0800 schrieb Tim Harvey: > On Fri, Nov 6, 2015 at 1:36 AM, Lucas Stach <l.stach@pengutronix.de> wrote: > > Am Donnerstag, den 05.11.2015, 06:58 -0800 schrieb Tim Harvey: > >> Freescale has stated [1] that the LVDS clock source of the IMX6 does not pass > >> the PCI Gen2 clock jitter test, therefore unless an external Gen2 compliant > >> external clock source is present and supplied back to the IMX6 PCIe core > >> via LVDS CLK1/CLK2 you can not claim Gen2 compliance. > >> > >> Add a dt property to specify gen1 vs gen2 and check this before allowing > >> a Gen2 link. > >> > > I think I already said this in the last round: there is nothing vendor > > specific in a max-link-speed property. I would really like to have this > > as a common DW PCIe property right from the beginning, so that we don't > > burden us with keeping backward compatibility with vendor specific > > properties when this moves to common code eventually. > > Hi Lucas, > > I did discuss this patch with you off-list but I think I misunderstood > your meaning then. > > So you are asking me to simply rename the properly > 'fsl,max-link-speed' to 'max-link-speed' and the rest of the patch is > good with you? > Yes, rename to max-link-speed. Move parsing of the property to dw_pcie_host_init() (it is a common property, so must be in common code) before the call to ops->host_init and store value into the pcie_port structure with -1 meaning property in DT is missing. imx6_pcie_establish_link can the treat values of -1 and 1 the same, only allowing Gen2 if the value is 2. Regards, Lucas
diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt index 6fbba53..7dff332 100644 --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt @@ -12,6 +12,8 @@ Required properties: - "msi": The interrupt that is asserted when an MSI is received - clock-names: Must include the following additional entries: - "pcie_phy" +- fsl,max-link-speed: Specify PCI gen. Defaults to 1, can be 2 if board has + an external clock generator fed back to PCIe core. Example: @@ -37,4 +39,5 @@ Example: <0 0 0 4 &intc GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>; clocks = <&clks 144>, <&clks 206>, <&clks 189>; clock-names = "pcie", "pcie_bus", "pcie_phy"; + fsl,max-link-speed = <2>; }; diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c index 22e8224..16412c4 100644 --- a/drivers/pci/host/pci-imx6.c +++ b/drivers/pci/host/pci-imx6.c @@ -39,6 +39,7 @@ struct imx6_pcie { struct pcie_port pp; struct regmap *iomuxc_gpr; void __iomem *mem_base; + int link_cap; }; /* PCIe Root Complex registers (memory-mapped) */ @@ -393,11 +394,15 @@ static int imx6_pcie_establish_link(struct pcie_port *pp) if (ret) return ret; - /* Allow Gen2 mode after the link is up. */ - tmp = readl(pp->dbi_base + PCIE_RC_LCR); - tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; - tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN2; - writel(tmp, pp->dbi_base + PCIE_RC_LCR); + if (imx6_pcie->link_cap == 2) { + /* Allow Gen2 mode after the link is up. */ + tmp = readl(pp->dbi_base + PCIE_RC_LCR); + tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; + tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN2; + writel(tmp, pp->dbi_base + PCIE_RC_LCR); + } else { + dev_info(pp->dev, "Link: Gen2 disabled\n"); + } /* * Start Directed Speed Change so the best possible speed both link @@ -421,7 +426,7 @@ static int imx6_pcie_establish_link(struct pcie_port *pp) } tmp = readl(pp->dbi_base + PCIE_RC_LCSR); - dev_dbg(pp->dev, "Link up, Gen=%i\n", (tmp >> 16) & 0xf); + dev_info(pp->dev, "Link up, Gen%i\n", (tmp >> 16) & 0xf); return 0; } @@ -591,6 +596,11 @@ static int __init imx6_pcie_probe(struct platform_device *pdev) } } + /* default link capability to gen1 */ + imx6_pcie->link_cap = 1; + if (of_property_read_u32(np, "fsl,max-link-speed", &ret) == 0) + imx6_pcie->link_cap = ret; + /* Fetch clocks */ imx6_pcie->pcie_phy = devm_clk_get(&pdev->dev, "pcie_phy"); if (IS_ERR(imx6_pcie->pcie_phy)) {
Freescale has stated [1] that the LVDS clock source of the IMX6 does not pass the PCI Gen2 clock jitter test, therefore unless an external Gen2 compliant external clock source is present and supplied back to the IMX6 PCIe core via LVDS CLK1/CLK2 you can not claim Gen2 compliance. Add a dt property to specify gen1 vs gen2 and check this before allowing a Gen2 link. We default to Gen1 if the property is not present because at this time there are no IMX6 boards in mainline that 'input' a clock on LVDS CLK1/CLK2. In order to be Gen2 compliant on IMX6 you need to: - have a Gen2 compliant external clock generator and route that clock back to either LVDS CLK1 or LVDS CLK2 as an input. (see IMX6SX-SabreSD reference design) - specify this clock in the pcie node in the dt (ie IMX6QDL_CLK_LVDS1_IN or IMX6QDL_CLK_LVDS2_IN instead of IMX6QDL_CLK_LVDS1_GATE which configures it as a CLK output) [1] https://community.freescale.com/message/453209 Signed-off-by: Tim Harvey <tharvey@gateworks.com> This is an RFC because I'm assuming the decision to default to Gen1 link only is going to ruffle some feathers. My understanding is that if you do not use an external Gen2 compliant clockgen for peripepherals 'and' the IMX6 core you should not claim Gen2 compliance. This was not obvious on original IMX6 reference designs and I believe the jitter issue was discovered by Freescale later and future reference designs were modified to state you need an ext clockgen for Gen2 compliance. --- .../devicetree/bindings/pci/fsl,imx6q-pcie.txt | 3 +++ drivers/pci/host/pci-imx6.c | 22 ++++++++++++++++------ 2 files changed, 19 insertions(+), 6 deletions(-)