| Message ID | 1411445498-20250-3-git-send-email-r65037@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Am Dienstag, den 23.09.2014, 12:11 +0800 schrieb Richard Zhu: > - a while delay is mandatory required after pcie_ref_clk_en > is set. Otherwise, the system would be hang on imx6qdl ard > boards, because that imx6qdl boards don't have the reset_gpio. > - the clocks should be stable already after the > "clk_prepare_enable" is return. So I think it's ok to move the > usleep delay after the pcie_ref_en is set. > You are describing a lot of the conditions around the issue, but not the issue itself, which makes it hard to follow your commit message. After looking at the code I think the problem is this (and should be described accordingly): For boards without a reset gpio we skip the delay between enabling the pcie_ref_clk and touching the RC registers for configuration. Apparently this hangs when the clocks are not yet settled in the DW PCIe core. So we need to make sure that there is always an appropriate delay between those two actions. I have not found this constraint anywhere in the i.MX6 Reference Manual, nor in the DW PCIe documents I have access to, which makes me a bit feel a bit unhappy about this. Richard, do you have better info on why this delay is needed and how long it needs to be? Or is this just empirical? In general I'm ok with this patch, but still want a confirmation from Tim that this doesn't break anything. > Signed-off-by: Richard Zhu <r65037@freescale.com> > --- > drivers/pci/host/pci-imx6.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > index 233fe8a..bc4222b 100644 > --- a/drivers/pci/host/pci-imx6.c > +++ b/drivers/pci/host/pci-imx6.c > @@ -275,15 +275,15 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp) > goto err_pcie; > } > > - /* allow the clocks to stabilize */ > - usleep_range(200, 500); > - > /* power up core phy and enable ref clock */ > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, > IMX6Q_GPR1_PCIE_TEST_PD, 0 << 18); > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, > IMX6Q_GPR1_PCIE_REF_CLK_EN, 1 << 16); > > + /* allow the clocks to stabilize */ > + usleep_range(200, 500); > + > /* Some boards don't have PCIe reset GPIO. */ > if (gpio_is_valid(imx6_pcie->reset_gpio)) { > gpio_set_value(imx6_pcie->reset_gpio, 0);
On Tue, Sep 23, 2014 at 2:56 AM, Lucas Stach <l.stach@pengutronix.de> wrote: > Am Dienstag, den 23.09.2014, 12:11 +0800 schrieb Richard Zhu: >> - a while delay is mandatory required after pcie_ref_clk_en >> is set. Otherwise, the system would be hang on imx6qdl ard >> boards, because that imx6qdl boards don't have the reset_gpio. >> - the clocks should be stable already after the >> "clk_prepare_enable" is return. So I think it's ok to move the >> usleep delay after the pcie_ref_en is set. >> > > You are describing a lot of the conditions around the issue, but not the > issue itself, which makes it hard to follow your commit message. After > looking at the code I think the problem is this (and should be described > accordingly): > > For boards without a reset gpio we skip the delay between enabling the > pcie_ref_clk and touching the RC registers for configuration. Apparently > this hangs when the clocks are not yet settled in the DW PCIe core. So > we need to make sure that there is always an appropriate delay between > those two actions. > > I have not found this constraint anywhere in the i.MX6 Reference Manual, > nor in the DW PCIe documents I have access to, which makes me a bit feel > a bit unhappy about this. Richard, do you have better info on why this > delay is needed and how long it needs to be? Or is this just empirical? > > In general I'm ok with this patch, but still want a confirmation from > Tim that this doesn't break anything. I agree with Lucas' comments and also agree that this can use some testing. Based on my previous findings PCI link is very fragile. It will take me a few days to get a proper test setup in a thermal chamber with a host of boards but I will report back when I have findings. Tim > >> Signed-off-by: Richard Zhu <r65037@freescale.com> >> --- >> drivers/pci/host/pci-imx6.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c >> index 233fe8a..bc4222b 100644 >> --- a/drivers/pci/host/pci-imx6.c >> +++ b/drivers/pci/host/pci-imx6.c >> @@ -275,15 +275,15 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp) >> goto err_pcie; >> } >> >> - /* allow the clocks to stabilize */ >> - usleep_range(200, 500); >> - >> /* power up core phy and enable ref clock */ >> regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, >> IMX6Q_GPR1_PCIE_TEST_PD, 0 << 18); >> regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, >> IMX6Q_GPR1_PCIE_REF_CLK_EN, 1 << 16); >> >> + /* allow the clocks to stabilize */ >> + usleep_range(200, 500); >> + >> /* Some boards don't have PCIe reset GPIO. */ >> if (gpio_is_valid(imx6_pcie->reset_gpio)) { >> gpio_set_value(imx6_pcie->reset_gpio, 0); > > -- > Pengutronix e.K. | Lucas Stach | > Industrial Linux Solutions | http://www.pengutronix.de/ | > > -- > 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 -- 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 Tue, Sep 23, 2014 at 1:11 AM, Richard Zhu <r65037@freescale.com> wrote: > - a while delay is mandatory required after pcie_ref_clk_en > is set. Otherwise, the system would be hang on imx6qdl ard > boards, because that imx6qdl boards don't have the reset_gpio. The mx6qdl sabreauto boards do have PCI reset pins. They come from the I2C MAX7310 expander. Yes, there are boards that do not have PCI reset GPIO, but this commit log need to be rewritten. -- 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
DQo+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IFRpbSBIYXJ2ZXkgW21haWx0 bzp0aGFydmV5QGdhdGV3b3Jrcy5jb21dDQo+IFNlbnQ6IFR1ZXNkYXksIFNlcHRlbWJlciAyMywg MjAxNCA4OjI5IFBNDQo+IFRvOiBMdWNhcyBTdGFjaDsgWmh1IFJpY2hhcmQtUjY1MDM3DQo+IENj OiBsaW51eC1wY2ktb3duZXJAdmdlci5rZXJuZWwub3JnOyBsaW51eC1wY2lAdmdlci5rZXJuZWwu b3JnOyBHdW8gU2hhd24tDQo+IFI2NTA3MzsgRmFiaW8gRXN0ZXZhbQ0KPiBTdWJqZWN0OiBSZTog W1BBVENIIHYyIDIvNV0gUENJOiBpbXg2OiB3YWl0IHRoZSBjbG9ja3MgdG8gc3RhYmlsaXplIGFm dGVyDQo+IHJlZl9lbg0KPiANCj4gT24gVHVlLCBTZXAgMjMsIDIwMTQgYXQgMjo1NiBBTSwgTHVj YXMgU3RhY2ggPGwuc3RhY2hAcGVuZ3V0cm9uaXguZGU+IHdyb3RlOg0KPiA+IEFtIERpZW5zdGFn LCBkZW4gMjMuMDkuMjAxNCwgMTI6MTEgKzA4MDAgc2NocmllYiBSaWNoYXJkIFpodToNCj4gPj4g LSBhIHdoaWxlIGRlbGF5IGlzIG1hbmRhdG9yeSByZXF1aXJlZCBhZnRlciBwY2llX3JlZl9jbGtf ZW4gaXMgc2V0Lg0KPiA+PiBPdGhlcndpc2UsIHRoZSBzeXN0ZW0gd291bGQgYmUgaGFuZyBvbiBp bXg2cWRsIGFyZCBib2FyZHMsIGJlY2F1c2UNCj4gPj4gdGhhdCBpbXg2cWRsIGJvYXJkcyBkb24n dCBoYXZlIHRoZSByZXNldF9ncGlvLg0KPiA+PiAtIHRoZSBjbG9ja3Mgc2hvdWxkIGJlIHN0YWJs ZSBhbHJlYWR5IGFmdGVyIHRoZSAiY2xrX3ByZXBhcmVfZW5hYmxlIg0KPiA+PiBpcyByZXR1cm4u IFNvIEkgdGhpbmsgaXQncyBvayB0byBtb3ZlIHRoZSB1c2xlZXAgZGVsYXkgYWZ0ZXIgdGhlDQo+ ID4+IHBjaWVfcmVmX2VuIGlzIHNldC4NCj4gPj4NCj4gPg0KPiA+IFlvdSBhcmUgZGVzY3JpYmlu ZyBhIGxvdCBvZiB0aGUgY29uZGl0aW9ucyBhcm91bmQgdGhlIGlzc3VlLCBidXQgbm90DQo+ID4g dGhlIGlzc3VlIGl0c2VsZiwgd2hpY2ggbWFrZXMgaXQgaGFyZCB0byBmb2xsb3cgeW91ciBjb21t aXQgbWVzc2FnZS4NCj4gPiBBZnRlciBsb29raW5nIGF0IHRoZSBjb2RlIEkgdGhpbmsgdGhlIHBy b2JsZW0gaXMgdGhpcyAoYW5kIHNob3VsZCBiZQ0KPiA+IGRlc2NyaWJlZA0KPiA+IGFjY29yZGlu Z2x5KToNCj4gPg0KPiA+IEZvciBib2FyZHMgd2l0aG91dCBhIHJlc2V0IGdwaW8gd2Ugc2tpcCB0 aGUgZGVsYXkgYmV0d2VlbiBlbmFibGluZyB0aGUNCj4gPiBwY2llX3JlZl9jbGsgYW5kIHRvdWNo aW5nIHRoZSBSQyByZWdpc3RlcnMgZm9yIGNvbmZpZ3VyYXRpb24uDQo+ID4gQXBwYXJlbnRseSB0 aGlzIGhhbmdzIHdoZW4gdGhlIGNsb2NrcyBhcmUgbm90IHlldCBzZXR0bGVkIGluIHRoZSBEVw0K PiA+IFBDSWUgY29yZS4gU28gd2UgbmVlZCB0byBtYWtlIHN1cmUgdGhhdCB0aGVyZSBpcyBhbHdh eXMgYW4gYXBwcm9wcmlhdGUNCj4gPiBkZWxheSBiZXR3ZWVuIHRob3NlIHR3byBhY3Rpb25zLg0K W1JpY2hhcmRdIFRoYW5rcy4NCj4gPg0KPiA+IEkgaGF2ZSBub3QgZm91bmQgdGhpcyBjb25zdHJh aW50IGFueXdoZXJlIGluIHRoZSBpLk1YNiBSZWZlcmVuY2UNCj4gPiBNYW51YWwsIG5vciBpbiB0 aGUgRFcgUENJZSBkb2N1bWVudHMgSSBoYXZlIGFjY2VzcyB0bywgd2hpY2ggbWFrZXMgbWUNCj4g PiBhIGJpdCBmZWVsIGEgYml0IHVuaGFwcHkgYWJvdXQgdGhpcy4gUmljaGFyZCwgZG8geW91IGhh dmUgYmV0dGVyIGluZm8NCj4gPiBvbiB3aHkgdGhpcyBkZWxheSBpcyBuZWVkZWQgYW5kIGhvdyBs b25nIGl0IG5lZWRzIHRvIGJlPyBPciBpcyB0aGlzIGp1c3QNCj4gZW1waXJpY2FsPw0KW1JpY2hh cmRdIEkgdXNlZCB0byBmaXggb25lIGxlc3MgdGhhbiAwLjMlIHBlcmNlbnRhZ2UgcmFuZG9tbHkg bGluayBkb3duIGlzc3VlDQpkdXJpbmcgdGhlIHdhcm0tcmVzZXQgbG9vcCBzdHJlc3MgdGVzdHMu DQpJIHVzZWQgZ2V0IHNvbWUgaW5mbyBmcm9tIGRlc2lnbiB0ZWFtIHRoYXQgIiB0aGUgYXN5bmMg cmVzZXQgaW5wdXQgbmVlZCByZWYgY2xvY2sgdG8gc3luYyBpbnRlcm5hbGx5LCB3aGVuIHRoZSBy ZWYgY2xvY2sgY29tZXMgYWZ0ZXIgcmVzZXQsIGludGVybmFsIHN5bmNlZCByZXNldCB0aW1lIGlz IHRvbyBzaG9ydCAsIGNhbm5vdCBtZWV0IHRoZSByZXF1aXJlbWVudC4iICJhdCBsZWFzdCA0dXMg ZGVsYXkgaXMgcmVxdWlyZWQgYWZ0ZXIgcmVmIGNsb2NrIHN0YWJsZSBhbmQgYmVmb3JlIHNzcF9l biBpcyBhc3NlcnQiDQpJIHdvdWxkIGFkZCBhYm91dCAxMHVzIGRlbGF5IGp1c3QgYmV0d2VlbiBj bG9ja3MgYXJlIHN0YWJsZSBhbmQgc3NwX2VuIGlzIGFzc2VydCBsYXRlci4NCj4gPg0KPiA+IElu IGdlbmVyYWwgSSdtIG9rIHdpdGggdGhpcyBwYXRjaCwgYnV0IHN0aWxsIHdhbnQgYSBjb25maXJt YXRpb24gZnJvbQ0KPiA+IFRpbSB0aGF0IHRoaXMgZG9lc24ndCBicmVhayBhbnl0aGluZy4NCj4g DQo+IEkgYWdyZWUgd2l0aCBMdWNhcycgY29tbWVudHMgYW5kIGFsc28gYWdyZWUgdGhhdCB0aGlz IGNhbiB1c2Ugc29tZSB0ZXN0aW5nLg0KPiBCYXNlZCBvbiBteSBwcmV2aW91cyBmaW5kaW5ncyBQ Q0kgbGluayBpcyB2ZXJ5IGZyYWdpbGUuIEl0IHdpbGwgdGFrZSBtZSBhIGZldw0KPiBkYXlzIHRv IGdldCBhIHByb3BlciB0ZXN0IHNldHVwIGluIGEgdGhlcm1hbCBjaGFtYmVyIHdpdGggYSBob3N0 IG9mIGJvYXJkcyBidXQNCj4gSSB3aWxsIHJlcG9ydCBiYWNrIHdoZW4gSSBoYXZlIGZpbmRpbmdz Lg0KPiANCj4gVGltDQo+IA0KDQoNCkJlc3QgUmVnYXJkcw0KUmljaGFyZCBaaHUNCg0KPiA+DQo+ ID4+IFNpZ25lZC1vZmYtYnk6IFJpY2hhcmQgWmh1IDxyNjUwMzdAZnJlZXNjYWxlLmNvbT4NCj4g Pj4gLS0tDQo+ID4+ICBkcml2ZXJzL3BjaS9ob3N0L3BjaS1pbXg2LmMgfCA2ICsrKy0tLQ0KPiA+ PiAgMSBmaWxlIGNoYW5nZWQsIDMgaW5zZXJ0aW9ucygrKSwgMyBkZWxldGlvbnMoLSkNCj4gPj4N Cj4gPj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvcGNpL2hvc3QvcGNpLWlteDYuYw0KPiA+PiBiL2Ry aXZlcnMvcGNpL2hvc3QvcGNpLWlteDYuYyBpbmRleCAyMzNmZThhLi5iYzQyMjJiIDEwMDY0NA0K PiA+PiAtLS0gYS9kcml2ZXJzL3BjaS9ob3N0L3BjaS1pbXg2LmMNCj4gPj4gKysrIGIvZHJpdmVy cy9wY2kvaG9zdC9wY2ktaW14Ni5jDQo+ID4+IEBAIC0yNzUsMTUgKzI3NSwxNSBAQCBzdGF0aWMg aW50IGlteDZfcGNpZV9kZWFzc2VydF9jb3JlX3Jlc2V0KHN0cnVjdA0KPiBwY2llX3BvcnQgKnBw KQ0KPiA+PiAgICAgICAgICAgICAgIGdvdG8gZXJyX3BjaWU7DQo+ID4+ICAgICAgIH0NCj4gPj4N Cj4gPj4gLSAgICAgLyogYWxsb3cgdGhlIGNsb2NrcyB0byBzdGFiaWxpemUgKi8NCj4gPj4gLSAg ICAgdXNsZWVwX3JhbmdlKDIwMCwgNTAwKTsNCj4gPj4gLQ0KPiA+PiAgICAgICAvKiBwb3dlciB1 cCBjb3JlIHBoeSBhbmQgZW5hYmxlIHJlZiBjbG9jayAqLw0KPiA+PiAgICAgICByZWdtYXBfdXBk YXRlX2JpdHMoaW14Nl9wY2llLT5pb211eGNfZ3ByLCBJT01VWENfR1BSMSwNCj4gPj4gICAgICAg ICAgICAgICAgICAgICAgIElNWDZRX0dQUjFfUENJRV9URVNUX1BELCAwIDw8IDE4KTsNCj4gPj4g ICAgICAgcmVnbWFwX3VwZGF0ZV9iaXRzKGlteDZfcGNpZS0+aW9tdXhjX2dwciwgSU9NVVhDX0dQ UjEsDQo+ID4+ICAgICAgICAgICAgICAgICAgICAgICBJTVg2UV9HUFIxX1BDSUVfUkVGX0NMS19F TiwgMSA8PCAxNik7DQo+ID4+DQo+ID4+ICsgICAgIC8qIGFsbG93IHRoZSBjbG9ja3MgdG8gc3Rh YmlsaXplICovDQo+ID4+ICsgICAgIHVzbGVlcF9yYW5nZSgyMDAsIDUwMCk7DQo+ID4+ICsNCj4g Pj4gICAgICAgLyogU29tZSBib2FyZHMgZG9uJ3QgaGF2ZSBQQ0llIHJlc2V0IEdQSU8uICovDQo+ ID4+ICAgICAgIGlmIChncGlvX2lzX3ZhbGlkKGlteDZfcGNpZS0+cmVzZXRfZ3BpbykpIHsNCj4g Pj4gICAgICAgICAgICAgICBncGlvX3NldF92YWx1ZShpbXg2X3BjaWUtPnJlc2V0X2dwaW8sIDAp Ow0KPiA+DQo+ID4gLS0NCj4gPiBQZW5ndXRyb25peCBlLksuICAgICAgICAgICAgIHwgTHVjYXMg U3RhY2ggICAgICAgICAgICAgICAgIHwNCj4gPiBJbmR1c3RyaWFsIExpbnV4IFNvbHV0aW9ucyAg IHwgaHR0cDovL3d3dy5wZW5ndXRyb25peC5kZS8gIHwNCj4gPg0KPiA+IC0tDQo+ID4gVG8gdW5z dWJzY3JpYmUgZnJvbSB0aGlzIGxpc3Q6IHNlbmQgdGhlIGxpbmUgInVuc3Vic2NyaWJlIGxpbnV4 LXBjaSINCj4gPiBpbiB0aGUgYm9keSBvZiBhIG1lc3NhZ2UgdG8gbWFqb3Jkb21vQHZnZXIua2Vy bmVsLm9yZyBNb3JlIG1ham9yZG9tbw0KPiA+IGluZm8gYXQgIGh0dHA6Ly92Z2VyLmtlcm5lbC5v cmcvbWFqb3Jkb21vLWluZm8uaHRtbA0K -- 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 Tue, Sep 23, 2014 at 5:28 AM, Tim Harvey <tharvey@gateworks.com> wrote: > On Tue, Sep 23, 2014 at 2:56 AM, Lucas Stach <l.stach@pengutronix.de> wrote: >> Am Dienstag, den 23.09.2014, 12:11 +0800 schrieb Richard Zhu: >>> - a while delay is mandatory required after pcie_ref_clk_en >>> is set. Otherwise, the system would be hang on imx6qdl ard >>> boards, because that imx6qdl boards don't have the reset_gpio. >>> - the clocks should be stable already after the >>> "clk_prepare_enable" is return. So I think it's ok to move the >>> usleep delay after the pcie_ref_en is set. >>> >> >> You are describing a lot of the conditions around the issue, but not the >> issue itself, which makes it hard to follow your commit message. After >> looking at the code I think the problem is this (and should be described >> accordingly): >> >> For boards without a reset gpio we skip the delay between enabling the >> pcie_ref_clk and touching the RC registers for configuration. Apparently >> this hangs when the clocks are not yet settled in the DW PCIe core. So >> we need to make sure that there is always an appropriate delay between >> those two actions. >> >> I have not found this constraint anywhere in the i.MX6 Reference Manual, >> nor in the DW PCIe documents I have access to, which makes me a bit feel >> a bit unhappy about this. Richard, do you have better info on why this >> delay is needed and how long it needs to be? Or is this just empirical? >> >> In general I'm ok with this patch, but still want a confirmation from >> Tim that this doesn't break anything. > > I agree with Lucas' comments and also agree that this can use some > testing. Based on my previous findings PCI link is very fragile. It > will take me a few days to get a proper test setup in a thermal > chamber with a host of boards but I will report back when I have > findings. > > Tim > >> >>> Signed-off-by: Richard Zhu <r65037@freescale.com> >>> --- >>> drivers/pci/host/pci-imx6.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c >>> index 233fe8a..bc4222b 100644 >>> --- a/drivers/pci/host/pci-imx6.c >>> +++ b/drivers/pci/host/pci-imx6.c >>> @@ -275,15 +275,15 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp) >>> goto err_pcie; >>> } >>> >>> - /* allow the clocks to stabilize */ >>> - usleep_range(200, 500); >>> - >>> /* power up core phy and enable ref clock */ >>> regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, >>> IMX6Q_GPR1_PCIE_TEST_PD, 0 << 18); >>> regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, >>> IMX6Q_GPR1_PCIE_REF_CLK_EN, 1 << 16); >>> >>> + /* allow the clocks to stabilize */ >>> + usleep_range(200, 500); >>> + >>> /* Some boards don't have PCIe reset GPIO. */ >>> if (gpio_is_valid(imx6_pcie->reset_gpio)) { >>> gpio_set_value(imx6_pcie->reset_gpio, 0); >> I tested this across temperature over 300+ boots each on several IMX6 based boards with switches and did not encounter any link failures. Tested-by: Tim Harvey <tharvey@gateworks.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
Thanks Tim. Best regards Richard > ? 2014?10?2????2:00?"Tim Harvey" <tharvey@gateworks.com> ??? > >> On Tue, Sep 23, 2014 at 5:28 AM, Tim Harvey <tharvey@gateworks.com> wrote: >>> On Tue, Sep 23, 2014 at 2:56 AM, Lucas Stach <l.stach@pengutronix.de> wrote: >>> Am Dienstag, den 23.09.2014, 12:11 +0800 schrieb Richard Zhu: >>>> - a while delay is mandatory required after pcie_ref_clk_en >>>> is set. Otherwise, the system would be hang on imx6qdl ard >>>> boards, because that imx6qdl boards don't have the reset_gpio. >>>> - the clocks should be stable already after the >>>> "clk_prepare_enable" is return. So I think it's ok to move the >>>> usleep delay after the pcie_ref_en is set. >>> >>> You are describing a lot of the conditions around the issue, but not the >>> issue itself, which makes it hard to follow your commit message. After >>> looking at the code I think the problem is this (and should be described >>> accordingly): >>> >>> For boards without a reset gpio we skip the delay between enabling the >>> pcie_ref_clk and touching the RC registers for configuration. Apparently >>> this hangs when the clocks are not yet settled in the DW PCIe core. So >>> we need to make sure that there is always an appropriate delay between >>> those two actions. >>> >>> I have not found this constraint anywhere in the i.MX6 Reference Manual, >>> nor in the DW PCIe documents I have access to, which makes me a bit feel >>> a bit unhappy about this. Richard, do you have better info on why this >>> delay is needed and how long it needs to be? Or is this just empirical? >>> >>> In general I'm ok with this patch, but still want a confirmation from >>> Tim that this doesn't break anything. >> >> I agree with Lucas' comments and also agree that this can use some >> testing. Based on my previous findings PCI link is very fragile. It >> will take me a few days to get a proper test setup in a thermal >> chamber with a host of boards but I will report back when I have >> findings. >> >> Tim >> >>> >>>> Signed-off-by: Richard Zhu <r65037@freescale.com> >>>> --- >>>> drivers/pci/host/pci-imx6.c | 6 +++--- >>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c >>>> index 233fe8a..bc4222b 100644 >>>> --- a/drivers/pci/host/pci-imx6.c >>>> +++ b/drivers/pci/host/pci-imx6.c >>>> @@ -275,15 +275,15 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp) >>>> goto err_pcie; >>>> } >>>> >>>> - /* allow the clocks to stabilize */ >>>> - usleep_range(200, 500); >>>> - >>>> /* power up core phy and enable ref clock */ >>>> regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, >>>> IMX6Q_GPR1_PCIE_TEST_PD, 0 << 18); >>>> regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, >>>> IMX6Q_GPR1_PCIE_REF_CLK_EN, 1 << 16); >>>> >>>> + /* allow the clocks to stabilize */ >>>> + usleep_range(200, 500); >>>> + >>>> /* Some boards don't have PCIe reset GPIO. */ >>>> if (gpio_is_valid(imx6_pcie->reset_gpio)) { >>>> gpio_set_value(imx6_pcie->reset_gpio, 0); > > I tested this across temperature over 300+ boots each on several IMX6 > based boards with switches and did not encounter any link failures. > > Tested-by: Tim Harvey <tharvey@gateworks.com>
Hi Richard, On Tue, Sep 23, 2014 at 1:11 AM, Richard Zhu <r65037@freescale.com> wrote: > - a while delay is mandatory required after pcie_ref_clk_en > is set. Otherwise, the system would be hang on imx6qdl ard > boards, because that imx6qdl boards don't have the reset_gpio. > - the clocks should be stable already after the > "clk_prepare_enable" is return. So I think it's ok to move the > usleep delay after the pcie_ref_en is set. > > Signed-off-by: Richard Zhu <r65037@freescale.com> Tested-by: Fabio Estevam <fabio.estevam@freescale.com> Without this patch we notice that the kernel does not boot anymore since commit 3fce0e882f61 (PCI: imx6: Delay enabling reference clock for SS until it stabilizes) on a system that does not pass the PCI gpio reset in the dtb. This causes a regression on mx6 nitrogen boards. I would suggest that you resend this patch only so that it could be applied into 3.18 as a bug fix. -- 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
DQo+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IEZhYmlvIEVzdGV2YW0gW21h aWx0bzpmZXN0ZXZhbUBnbWFpbC5jb21dDQo+IFNlbnQ6IEZyaWRheSwgT2N0b2JlciAyNCwgMjAx NCA5OjUxIEFNDQo+IFRvOiBaaHUgUmljaGFyZC1SNjUwMzcNCj4gQ2M6IGxpbnV4LXBjaS1vd25l ckB2Z2VyLmtlcm5lbC5vcmc7IGxpbnV4LXBjaUB2Z2VyLmtlcm5lbC5vcmc7IEd1byBTaGF3bi0N Cj4gUjY1MDczOyBMdWNhcyBTdGFjaDsgVGltIEhhcnZleTsgQmpvcm4gSGVsZ2Fhcw0KPiBTdWJq ZWN0OiBSZTogW1BBVENIIHYyIDIvNV0gUENJOiBpbXg2OiB3YWl0IHRoZSBjbG9ja3MgdG8gc3Rh YmlsaXplIGFmdGVyDQo+IHJlZl9lbg0KPiANCj4gSGkgUmljaGFyZCwNCj4gDQo+IE9uIFR1ZSwg U2VwIDIzLCAyMDE0IGF0IDE6MTEgQU0sIFJpY2hhcmQgWmh1IDxyNjUwMzdAZnJlZXNjYWxlLmNv bT4gd3JvdGU6DQo+ID4gLSBhIHdoaWxlIGRlbGF5IGlzIG1hbmRhdG9yeSByZXF1aXJlZCBhZnRl ciBwY2llX3JlZl9jbGtfZW4gaXMgc2V0Lg0KPiA+IE90aGVyd2lzZSwgdGhlIHN5c3RlbSB3b3Vs ZCBiZSBoYW5nIG9uIGlteDZxZGwgYXJkIGJvYXJkcywgYmVjYXVzZQ0KPiA+IHRoYXQgaW14NnFk bCBib2FyZHMgZG9uJ3QgaGF2ZSB0aGUgcmVzZXRfZ3Bpby4NCj4gPiAtIHRoZSBjbG9ja3Mgc2hv dWxkIGJlIHN0YWJsZSBhbHJlYWR5IGFmdGVyIHRoZSAiY2xrX3ByZXBhcmVfZW5hYmxlIg0KPiA+ IGlzIHJldHVybi4gU28gSSB0aGluayBpdCdzIG9rIHRvIG1vdmUgdGhlIHVzbGVlcCBkZWxheSBh ZnRlciB0aGUNCj4gPiBwY2llX3JlZl9lbiBpcyBzZXQuDQo+ID4NCj4gPiBTaWduZWQtb2ZmLWJ5 OiBSaWNoYXJkIFpodSA8cjY1MDM3QGZyZWVzY2FsZS5jb20+DQo+IA0KPiBUZXN0ZWQtYnk6IEZh YmlvIEVzdGV2YW0gPGZhYmlvLmVzdGV2YW1AZnJlZXNjYWxlLmNvbT4NCj4gDQo+IFdpdGhvdXQg dGhpcyBwYXRjaCB3ZSBub3RpY2UgdGhhdCB0aGUga2VybmVsIGRvZXMgbm90IGJvb3QgYW55bW9y ZSBzaW5jZQ0KPiBjb21taXQgIDNmY2UwZTg4MmY2MSAoUENJOiBpbXg2OiBEZWxheSBlbmFibGlu ZyByZWZlcmVuY2UgY2xvY2sgZm9yIFNTIHVudGlsDQo+IGl0IHN0YWJpbGl6ZXMpIG9uIGEgc3lz dGVtIHRoYXQgZG9lcyBub3QgcGFzcyB0aGUgUENJIGdwaW8gcmVzZXQgaW4gdGhlIGR0Yi4NCj4g VGhpcyBjYXVzZXMgYSByZWdyZXNzaW9uIG9uIG14NiBuaXRyb2dlbiBib2FyZHMuDQo+IA0KPiBJ IHdvdWxkIHN1Z2dlc3QgdGhhdCB5b3UgcmVzZW5kIHRoaXMgcGF0Y2ggb25seSBzbyB0aGF0IGl0 IGNvdWxkIGJlIGFwcGxpZWQNCj4gaW50byAzLjE4IGFzIGEgYnVnIGZpeC4NCg0KDQpPa2F5LCBJ IHdvdWxkIHNlbmQgb3V0IHRoZSBwYXRjaCB0b2RheS4NCg0KQmVzdCBSZWdhcmRzDQpSaWNoYXJk IFpodQ0K -- 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-imx6.c b/drivers/pci/host/pci-imx6.c index 233fe8a..bc4222b 100644 --- a/drivers/pci/host/pci-imx6.c +++ b/drivers/pci/host/pci-imx6.c @@ -275,15 +275,15 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp) goto err_pcie; } - /* allow the clocks to stabilize */ - usleep_range(200, 500); - /* power up core phy and enable ref clock */ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, IMX6Q_GPR1_PCIE_TEST_PD, 0 << 18); regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, IMX6Q_GPR1_PCIE_REF_CLK_EN, 1 << 16); + /* allow the clocks to stabilize */ + usleep_range(200, 500); + /* Some boards don't have PCIe reset GPIO. */ if (gpio_is_valid(imx6_pcie->reset_gpio)) { gpio_set_value(imx6_pcie->reset_gpio, 0);
- a while delay is mandatory required after pcie_ref_clk_en is set. Otherwise, the system would be hang on imx6qdl ard boards, because that imx6qdl boards don't have the reset_gpio. - the clocks should be stable already after the "clk_prepare_enable" is return. So I think it's ok to move the usleep delay after the pcie_ref_en is set. Signed-off-by: Richard Zhu <r65037@freescale.com> --- drivers/pci/host/pci-imx6.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)