All of lore.kernel.org
 help / color / mirror / Atom feed
From: Lucas Stach <l.stach@pengutronix.de>
To: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: linux-pci@vger.kernel.org, yurovsky@gmail.com,
	Bjorn Helgaas <bhelgaas@google.com>,
	Fabio Estevam <fabio.estevam@nxp.com>,
	Dong Aisheng <dongas86@gmail.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 3/4] PCI: imx6: Do not wait for speed change on i.MX7
Date: Tue, 21 Mar 2017 14:50:40 +0100	[thread overview]
Message-ID: <1490104240.2895.22.camel@pengutronix.de> (raw)
In-Reply-To: <20170321134203.26325-4-andrew.smirnov@gmail.com>

Am Dienstag, den 21.03.2017, 06:42 -0700 schrieb Andrey Smirnov:
> As can be seen from [1]:
> 
> "...the different behavior between iMX6Q PCIe and iMX7D PCIe maybe
> caused by the different controller version.
> 
> Regarding to the DOC description, the DIRECT_SPEED_CHANGE should be
> cleared after the speed change from GEN1 to GEN2. Unfortunately, when
> GEN1 device is used, the behavior is not documented.
> 
> So, IC design guys run the simulation and
> find out the following behaviors:
> 
>      1. DIRECT_SPEED_CHANGE will be cleared in 7D after speed change
>      	from GEN1 to GEN2. This matches doc’s description
> 
>      2. set MAX link speed(PCIE_CAP_TARGET_LINK_SPEED=0x01) as GEN1 and
>      	re-run the simulation, DIRECT_SPEED_CHANGE will not be cleared;
>      	remain as 1, this matches your result, but function test is
>      	passed, so this bit should not affect the normal PCIe function.
> ..."
> 
> imx6_pcie_wait_for_speed_change will report false failures for Gen1 ->
> Gen1 speed transition, so avoid doing that check and just rely on
> imx6_pcie_wait_for_link only.
> 
> [1] https://community.nxp.com/message/867943
> 
> Cc: yurovsky@gmail.com
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Dong Aisheng <dongas86@gmail.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  drivers/pci/dwc/pci-imx6.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c
> index 64de9bb..c731e41 100644
> --- a/drivers/pci/dwc/pci-imx6.c
> +++ b/drivers/pci/dwc/pci-imx6.c
> @@ -545,10 +545,21 @@ static int imx6_pcie_establish_link(struct imx6_pcie *imx6_pcie)
>  	tmp |= PORT_LOGIC_SPEED_CHANGE;
>  	dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, tmp);
>  
> -	ret = imx6_pcie_wait_for_speed_change(imx6_pcie);
> -	if (ret) {
> -		dev_err(dev, "Failed to bring link up!\n");
> -		goto err_reset_phy;
> +	if (imx6_pcie->variant != IMX7D) {
> +		/*
> +		 * On i.MX7, DIRECT_SPEED_CHANGE behaves differently
> +		 * from i.MX6 family when no link speed transition
> +		 * occurs and we go Gen1 -> yep, Gen1. The difference
> +		 * is that, in such case, it will not be cleared by HW
> +		 * which will cause the following code to report false
> +		 * failure.
> +		 */
> +
> +		ret = imx6_pcie_wait_for_speed_change(imx6_pcie);
> +		if (ret) {
> +			dev_err(dev, "Failed to bring link up!\n");
> +			goto err_reset_phy;
> +		}
>  	}
>  
>  	/* Make sure link training is finished as well! */

WARNING: multiple messages have this Message-ID (diff)
From: Lucas Stach <l.stach@pengutronix.de>
To: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Dong Aisheng <dongas86@gmail.com>,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	Fabio Estevam <fabio.estevam@nxp.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	linux-arm-kernel@lists.infradead.org, yurovsky@gmail.com
Subject: Re: [PATCH v7 3/4] PCI: imx6: Do not wait for speed change on i.MX7
Date: Tue, 21 Mar 2017 14:50:40 +0100	[thread overview]
Message-ID: <1490104240.2895.22.camel@pengutronix.de> (raw)
In-Reply-To: <20170321134203.26325-4-andrew.smirnov@gmail.com>

QW0gRGllbnN0YWcsIGRlbiAyMS4wMy4yMDE3LCAwNjo0MiAtMDcwMCBzY2hyaWViIEFuZHJleSBT
bWlybm92Ogo+IEFzIGNhbiBiZSBzZWVuIGZyb20gWzFdOgo+IAo+ICIuLi50aGUgZGlmZmVyZW50
IGJlaGF2aW9yIGJldHdlZW4gaU1YNlEgUENJZSBhbmQgaU1YN0QgUENJZSBtYXliZQo+IGNhdXNl
ZCBieSB0aGUgZGlmZmVyZW50IGNvbnRyb2xsZXIgdmVyc2lvbi4KPiAKPiBSZWdhcmRpbmcgdG8g
dGhlIERPQyBkZXNjcmlwdGlvbiwgdGhlIERJUkVDVF9TUEVFRF9DSEFOR0Ugc2hvdWxkIGJlCj4g
Y2xlYXJlZCBhZnRlciB0aGUgc3BlZWQgY2hhbmdlIGZyb20gR0VOMSB0byBHRU4yLiBVbmZvcnR1
bmF0ZWx5LCB3aGVuCj4gR0VOMSBkZXZpY2UgaXMgdXNlZCwgdGhlIGJlaGF2aW9yIGlzIG5vdCBk
b2N1bWVudGVkLgo+IAo+IFNvLCBJQyBkZXNpZ24gZ3V5cyBydW4gdGhlIHNpbXVsYXRpb24gYW5k
Cj4gZmluZCBvdXQgdGhlIGZvbGxvd2luZyBiZWhhdmlvcnM6Cj4gCj4gICAgICAxLiBESVJFQ1Rf
U1BFRURfQ0hBTkdFIHdpbGwgYmUgY2xlYXJlZCBpbiA3RCBhZnRlciBzcGVlZCBjaGFuZ2UKPiAg
ICAgIAlmcm9tIEdFTjEgdG8gR0VOMi4gVGhpcyBtYXRjaGVzIGRvY+KAmXMgZGVzY3JpcHRpb24K
PiAKPiAgICAgIDIuIHNldCBNQVggbGluayBzcGVlZChQQ0lFX0NBUF9UQVJHRVRfTElOS19TUEVF
RD0weDAxKSBhcyBHRU4xIGFuZAo+ICAgICAgCXJlLXJ1biB0aGUgc2ltdWxhdGlvbiwgRElSRUNU
X1NQRUVEX0NIQU5HRSB3aWxsIG5vdCBiZSBjbGVhcmVkOwo+ICAgICAgCXJlbWFpbiBhcyAxLCB0
aGlzIG1hdGNoZXMgeW91ciByZXN1bHQsIGJ1dCBmdW5jdGlvbiB0ZXN0IGlzCj4gICAgICAJcGFz
c2VkLCBzbyB0aGlzIGJpdCBzaG91bGQgbm90IGFmZmVjdCB0aGUgbm9ybWFsIFBDSWUgZnVuY3Rp
b24uCj4gLi4uIgo+IAo+IGlteDZfcGNpZV93YWl0X2Zvcl9zcGVlZF9jaGFuZ2Ugd2lsbCByZXBv
cnQgZmFsc2UgZmFpbHVyZXMgZm9yIEdlbjEgLT4KPiBHZW4xIHNwZWVkIHRyYW5zaXRpb24sIHNv
IGF2b2lkIGRvaW5nIHRoYXQgY2hlY2sgYW5kIGp1c3QgcmVseSBvbgo+IGlteDZfcGNpZV93YWl0
X2Zvcl9saW5rIG9ubHkuCj4gCj4gWzFdIGh0dHBzOi8vY29tbXVuaXR5Lm54cC5jb20vbWVzc2Fn
ZS84Njc5NDMKPiAKPiBDYzogeXVyb3Zza3lAZ21haWwuY29tCj4gQ2M6IEx1Y2FzIFN0YWNoIDxs
LnN0YWNoQHBlbmd1dHJvbml4LmRlPgo+IENjOiBCam9ybiBIZWxnYWFzIDxiaGVsZ2Fhc0Bnb29n
bGUuY29tPgo+IENjOiBGYWJpbyBFc3RldmFtIDxmYWJpby5lc3RldmFtQG54cC5jb20+Cj4gQ2M6
IERvbmcgQWlzaGVuZyA8ZG9uZ2FzODZAZ21haWwuY29tPgo+IENjOiBsaW51eC1hcm0ta2VybmVs
QGxpc3RzLmluZnJhZGVhZC5vcmcKPiBDYzogbGludXgta2VybmVsQHZnZXIua2VybmVsLm9yZwo+
IFNpZ25lZC1vZmYtYnk6IEFuZHJleSBTbWlybm92IDxhbmRyZXcuc21pcm5vdkBnbWFpbC5jb20+
CgpSZXZpZXdlZC1ieTogTHVjYXMgU3RhY2ggPGwuc3RhY2hAcGVuZ3V0cm9uaXguZGU+Cgo+IC0t
LQo+ICBkcml2ZXJzL3BjaS9kd2MvcGNpLWlteDYuYyB8IDE5ICsrKysrKysrKysrKysrKy0tLS0K
PiAgMSBmaWxlIGNoYW5nZWQsIDE1IGluc2VydGlvbnMoKyksIDQgZGVsZXRpb25zKC0pCj4gCj4g
ZGlmZiAtLWdpdCBhL2RyaXZlcnMvcGNpL2R3Yy9wY2ktaW14Ni5jIGIvZHJpdmVycy9wY2kvZHdj
L3BjaS1pbXg2LmMKPiBpbmRleCA2NGRlOWJiLi5jNzMxZTQxIDEwMDY0NAo+IC0tLSBhL2RyaXZl
cnMvcGNpL2R3Yy9wY2ktaW14Ni5jCj4gKysrIGIvZHJpdmVycy9wY2kvZHdjL3BjaS1pbXg2LmMK
PiBAQCAtNTQ1LDEwICs1NDUsMjEgQEAgc3RhdGljIGludCBpbXg2X3BjaWVfZXN0YWJsaXNoX2xp
bmsoc3RydWN0IGlteDZfcGNpZSAqaW14Nl9wY2llKQo+ICAJdG1wIHw9IFBPUlRfTE9HSUNfU1BF
RURfQ0hBTkdFOwo+ICAJZHdfcGNpZV93cml0ZWxfZGJpKHBjaSwgUENJRV9MSU5LX1dJRFRIX1NQ
RUVEX0NPTlRST0wsIHRtcCk7Cj4gIAo+IC0JcmV0ID0gaW14Nl9wY2llX3dhaXRfZm9yX3NwZWVk
X2NoYW5nZShpbXg2X3BjaWUpOwo+IC0JaWYgKHJldCkgewo+IC0JCWRldl9lcnIoZGV2LCAiRmFp
bGVkIHRvIGJyaW5nIGxpbmsgdXAhXG4iKTsKPiAtCQlnb3RvIGVycl9yZXNldF9waHk7Cj4gKwlp
ZiAoaW14Nl9wY2llLT52YXJpYW50ICE9IElNWDdEKSB7Cj4gKwkJLyoKPiArCQkgKiBPbiBpLk1Y
NywgRElSRUNUX1NQRUVEX0NIQU5HRSBiZWhhdmVzIGRpZmZlcmVudGx5Cj4gKwkJICogZnJvbSBp
Lk1YNiBmYW1pbHkgd2hlbiBubyBsaW5rIHNwZWVkIHRyYW5zaXRpb24KPiArCQkgKiBvY2N1cnMg
YW5kIHdlIGdvIEdlbjEgLT4geWVwLCBHZW4xLiBUaGUgZGlmZmVyZW5jZQo+ICsJCSAqIGlzIHRo
YXQsIGluIHN1Y2ggY2FzZSwgaXQgd2lsbCBub3QgYmUgY2xlYXJlZCBieSBIVwo+ICsJCSAqIHdo
aWNoIHdpbGwgY2F1c2UgdGhlIGZvbGxvd2luZyBjb2RlIHRvIHJlcG9ydCBmYWxzZQo+ICsJCSAq
IGZhaWx1cmUuCj4gKwkJICovCj4gKwo+ICsJCXJldCA9IGlteDZfcGNpZV93YWl0X2Zvcl9zcGVl
ZF9jaGFuZ2UoaW14Nl9wY2llKTsKPiArCQlpZiAocmV0KSB7Cj4gKwkJCWRldl9lcnIoZGV2LCAi
RmFpbGVkIHRvIGJyaW5nIGxpbmsgdXAhXG4iKTsKPiArCQkJZ290byBlcnJfcmVzZXRfcGh5Owo+
ICsJCX0KPiAgCX0KPiAgCj4gIAkvKiBNYWtlIHN1cmUgbGluayB0cmFpbmluZyBpcyBmaW5pc2hl
ZCBhcyB3ZWxsISAqLwoKCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f
X19fX19fXwpsaW51eC1hcm0ta2VybmVsIG1haWxpbmcgbGlzdApsaW51eC1hcm0ta2VybmVsQGxp
c3RzLmluZnJhZGVhZC5vcmcKaHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvbWFpbG1hbi9saXN0
aW5mby9saW51eC1hcm0ta2VybmVsCg==

WARNING: multiple messages have this Message-ID (diff)
From: l.stach@pengutronix.de (Lucas Stach)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v7 3/4] PCI: imx6: Do not wait for speed change on i.MX7
Date: Tue, 21 Mar 2017 14:50:40 +0100	[thread overview]
Message-ID: <1490104240.2895.22.camel@pengutronix.de> (raw)
In-Reply-To: <20170321134203.26325-4-andrew.smirnov@gmail.com>

Am Dienstag, den 21.03.2017, 06:42 -0700 schrieb Andrey Smirnov:
> As can be seen from [1]:
> 
> "...the different behavior between iMX6Q PCIe and iMX7D PCIe maybe
> caused by the different controller version.
> 
> Regarding to the DOC description, the DIRECT_SPEED_CHANGE should be
> cleared after the speed change from GEN1 to GEN2. Unfortunately, when
> GEN1 device is used, the behavior is not documented.
> 
> So, IC design guys run the simulation and
> find out the following behaviors:
> 
>      1. DIRECT_SPEED_CHANGE will be cleared in 7D after speed change
>      	from GEN1 to GEN2. This matches doc?s description
> 
>      2. set MAX link speed(PCIE_CAP_TARGET_LINK_SPEED=0x01) as GEN1 and
>      	re-run the simulation, DIRECT_SPEED_CHANGE will not be cleared;
>      	remain as 1, this matches your result, but function test is
>      	passed, so this bit should not affect the normal PCIe function.
> ..."
> 
> imx6_pcie_wait_for_speed_change will report false failures for Gen1 ->
> Gen1 speed transition, so avoid doing that check and just rely on
> imx6_pcie_wait_for_link only.
> 
> [1] https://community.nxp.com/message/867943
> 
> Cc: yurovsky at gmail.com
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Dong Aisheng <dongas86@gmail.com>
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: linux-kernel at vger.kernel.org
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  drivers/pci/dwc/pci-imx6.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c
> index 64de9bb..c731e41 100644
> --- a/drivers/pci/dwc/pci-imx6.c
> +++ b/drivers/pci/dwc/pci-imx6.c
> @@ -545,10 +545,21 @@ static int imx6_pcie_establish_link(struct imx6_pcie *imx6_pcie)
>  	tmp |= PORT_LOGIC_SPEED_CHANGE;
>  	dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, tmp);
>  
> -	ret = imx6_pcie_wait_for_speed_change(imx6_pcie);
> -	if (ret) {
> -		dev_err(dev, "Failed to bring link up!\n");
> -		goto err_reset_phy;
> +	if (imx6_pcie->variant != IMX7D) {
> +		/*
> +		 * On i.MX7, DIRECT_SPEED_CHANGE behaves differently
> +		 * from i.MX6 family when no link speed transition
> +		 * occurs and we go Gen1 -> yep, Gen1. The difference
> +		 * is that, in such case, it will not be cleared by HW
> +		 * which will cause the following code to report false
> +		 * failure.
> +		 */
> +
> +		ret = imx6_pcie_wait_for_speed_change(imx6_pcie);
> +		if (ret) {
> +			dev_err(dev, "Failed to bring link up!\n");
> +			goto err_reset_phy;
> +		}
>  	}
>  
>  	/* Make sure link training is finished as well! */

  reply	other threads:[~2017-03-21 13:52 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-21 13:41 [PATCH v7 0/4] i.MX7 PCI support Andrey Smirnov
2017-03-21 13:41 ` Andrey Smirnov
2017-03-21 13:41 ` Andrey Smirnov
2017-03-21 13:42 ` [PATCH v7 1/4] PCI: imx6: Add code to support i.MX7D Andrey Smirnov
2017-03-21 13:42   ` Andrey Smirnov
2017-03-21 13:42 ` [PATCH v7 2/4] PCI: imx6: Allow probe deferal by reset GPIO Andrey Smirnov
2017-03-21 13:42   ` Andrey Smirnov
2017-03-21 13:42   ` Andrey Smirnov
2017-03-21 13:49   ` Lucas Stach
2017-03-21 13:49     ` Lucas Stach
2017-03-21 13:49     ` Lucas Stach
2017-03-21 13:42 ` [PATCH v7 3/4] PCI: imx6: Do not wait for speed change on i.MX7 Andrey Smirnov
2017-03-21 13:42   ` Andrey Smirnov
2017-03-21 13:50   ` Lucas Stach [this message]
2017-03-21 13:50     ` Lucas Stach
2017-03-21 13:50     ` Lucas Stach
2017-03-21 13:42 ` [PATCH v7 4/4] PCI: imx6: Do not switch speed if Gen2 is disabled Andrey Smirnov
2017-03-21 13:42   ` Andrey Smirnov
2017-03-21 13:51   ` Lucas Stach
2017-03-21 13:51     ` Lucas Stach
2017-03-21 13:51     ` Lucas Stach

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1490104240.2895.22.camel@pengutronix.de \
    --to=l.stach@pengutronix.de \
    --cc=andrew.smirnov@gmail.com \
    --cc=bhelgaas@google.com \
    --cc=dongas86@gmail.com \
    --cc=fabio.estevam@nxp.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=yurovsky@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.