[PATCH] PCI: rockchip: Fix timeout in rockchip_pcie_host_init_port()

[PATCH] PCI: rockchip: Fix timeout in rockchip_pcie_host_init_port()

[Siyu Jin]

In function rockchip_pcie_host_init_port(), it defines a timeout
value of 500ms to wait for pcie training. However, it is not enough
for samsung PM953 SSD drive and realtek RTL8111F network adapter,
which leads to the following errors:

	[    0.879663] rockchip-pcie f8000000.pcie: PCIe link training gen1 timeout!
	[    0.880284] rockchip-pcie f8000000.pcie: deferred probe failed
	[    0.880932] rockchip-pcie: probe of f8000000.pcie failed with error -110

The pcie spec only defines the min time of training, not the max
one. So set a proper timeout value is important. Change the value
to 1000ms will fix this bug.

Signed-off-by: Siyu Jin <jinsiyu940203@163.com>
---
 drivers/pci/controller/pcie-rockchip-host.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index f1d08a1b1591..aa42e28b49a8 100644
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -329,10 +329,10 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
 
 	gpiod_set_value_cansleep(rockchip->ep_gpio, 1);
 
-	/* 500ms timeout value should be enough for Gen1/2 training */
+	/* 1000ms timeout value should be enough for Gen1/2 training */
 	err = readl_poll_timeout(rockchip->apb_base + PCIE_CLIENT_BASIC_STATUS1,
 				 status, PCIE_LINK_UP(status), 20,
-				 500 * USEC_PER_MSEC);
+				 1000 * USEC_PER_MSEC);
 	if (err) {
 		dev_err(dev, "PCIe link training gen1 timeout!\n");
 		goto err_power_off_phy;
@@ -349,7 +349,7 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
 
 		err = readl_poll_timeout(rockchip->apb_base + PCIE_CORE_CTRL,
 					 status, PCIE_LINK_IS_GEN2(status), 20,
-					 500 * USEC_PER_MSEC);
+					 1000 * USEC_PER_MSEC);
 		if (err)
 			dev_dbg(dev, "PCIe link training gen2 timeout, fall back to gen1!\n");
 	}
-- 
2.17.1

Re: [PATCH] PCI: rockchip: Fix timeout in rockchip_pcie_host_init_port()

[Krzysztof Wilczyński]

Hi,

Thank you for sending the patch over!

By the way, this is v2?  If so, then it would be "[PATCH v2]", but not
a big deal.

A few nitpicks.

> In function rockchip_pcie_host_init_port(), it defines a timeout
> value of 500ms to wait for pcie training. However, it is not enough
> for samsung PM953 SSD drive and realtek RTL8111F network adapter,
> which leads to the following errors:

It would be "PCIe" not "pcie", and "Samsung" and "Realtek" so that these
are properly capitalised.

> 	[    0.879663] rockchip-pcie f8000000.pcie: PCIe link training gen1 timeout!
> 	[    0.880284] rockchip-pcie f8000000.pcie: deferred probe failed
> 	[    0.880932] rockchip-pcie: probe of f8000000.pcie failed with error -110

Normally, the time stamp is better removed as it's not very useful.

> The pcie spec only defines the min time of training, not the max
> one. So set a proper timeout value is important. Change the value
> to 1000ms will fix this bug.
[...]

Again, it would be "PCIe" here.  Also, since you are mentioning the PCI
specification, would you be able to provide either a quote or a location
(standard revision, chapter, page, etc.) from the PCI SIG?

Additionally, if this change fixes a previous commit, then it might be
prudent to include the "Fixes:" field referring to it, as it's customary
to do so - a link to a relevant Bugzilla (https://bugzilla.kernel.org/)
would also be fine, if there is anything.

Other than that,

Reviewed-by: Krzysztof Wilczyński <kw@linux.com>


Krzysztof

Re: [PATCH] PCI: rockchip: Fix timeout in rockchip_pcie_host_init_port()

[Bjorn Helgaas]

On Wed, Apr 21, 2021 at 04:31:15PM +0800, Siyu Jin wrote:
> In function rockchip_pcie_host_init_port(), it defines a timeout
> value of 500ms to wait for pcie training. However, it is not enough
> for samsung PM953 SSD drive and realtek RTL8111F network adapter,
> which leads to the following errors:
> 
> 	[    0.879663] rockchip-pcie f8000000.pcie: PCIe link training gen1 timeout!
> 	[    0.880284] rockchip-pcie f8000000.pcie: deferred probe failed
> 	[    0.880932] rockchip-pcie: probe of f8000000.pcie failed with error -110

s/pcie/PCIe/ (also below)
s/samsung/Samsung/
s/realtek/Realtek/

Remove the timestamps because they're not useful here.

Indent quoted material like the error messages by two spaces.

When you repost, add these recipients (found by
"./scripts/get_maintainer.pl drivers/pci/controller/pcie-rockchip-host.c"):

  Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
  Shawn Lin <shawn.lin@rock-chips.com>
  Rob Herring <robh@kernel.org>
  Krzysztof Wilczyński <kw@linux.com> (since he commented on this post)

> The pcie spec only defines the min time of training, not the max
> one. So set a proper timeout value is important. Change the value
> to 1000ms will fix this bug.

Can you include the spec reference about where it defines the minimum
training time?

I guess this is actually a Rockchip-specific thing, since I assume
these devices work fine on other systems?  So maybe this is not a PCIe
thing but a Rockchip thing?

> Signed-off-by: Siyu Jin <jinsiyu940203@163.com>
> ---
>  drivers/pci/controller/pcie-rockchip-host.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> index f1d08a1b1591..aa42e28b49a8 100644
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -329,10 +329,10 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
>  
>  	gpiod_set_value_cansleep(rockchip->ep_gpio, 1);
>  
> -	/* 500ms timeout value should be enough for Gen1/2 training */
> +	/* 1000ms timeout value should be enough for Gen1/2 training */
>  	err = readl_poll_timeout(rockchip->apb_base + PCIE_CLIENT_BASIC_STATUS1,
>  				 status, PCIE_LINK_UP(status), 20,
> -				 500 * USEC_PER_MSEC);
> +				 1000 * USEC_PER_MSEC);
>  	if (err) {
>  		dev_err(dev, "PCIe link training gen1 timeout!\n");
>  		goto err_power_off_phy;
> @@ -349,7 +349,7 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
>  
>  		err = readl_poll_timeout(rockchip->apb_base + PCIE_CORE_CTRL,
>  					 status, PCIE_LINK_IS_GEN2(status), 20,
> -					 500 * USEC_PER_MSEC);
> +					 1000 * USEC_PER_MSEC);
>  		if (err)
>  			dev_dbg(dev, "PCIe link training gen2 timeout, fall back to gen1!\n");
>  	}
> -- 
> 2.17.1
>