[PATCH 1/1] pci-imx6: add speed change timeout message

[PATCH 1/1] pci-imx6: add speed change timeout message

[Troy Kisky]

Currently, the timeout is never detected as count
has a value of -1 if a timeout happens, but the code is checking
for 0. Also, this patch removes the unneeded final wait
if a timeout occurs.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 drivers/pci/host/pci-imx6.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index fdb9536..51be92c 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -398,20 +398,22 @@ static int imx6_pcie_start_link(struct pcie_port *pp)
 	writel(tmp, pp->dbi_base + PCIE_LINK_WIDTH_SPEED_CONTROL);
 
 	count = 200;
-	while (count--) {
+	while (1) {
 		tmp = readl(pp->dbi_base + PCIE_LINK_WIDTH_SPEED_CONTROL);
 		/* Test if the speed change finished. */
-		if (!(tmp & PORT_LOGIC_SPEED_CHANGE))
+		if (!(tmp & PORT_LOGIC_SPEED_CHANGE)) {
+			/* Make sure link training is finished as well! */
+			ret = imx6_pcie_wait_for_link(pp);
+			break;
+		}
+		if (count-- == 0) {
+			dev_err(pp->dev, "Speed change timeout\n");
+			ret = -EINVAL;
 			break;
+		}
 		usleep_range(100, 1000);
 	}
 
-	/* Make sure link training is finished as well! */
-	if (count)
-		ret = imx6_pcie_wait_for_link(pp);
-	else
-		ret = -EINVAL;
-
 	if (ret) {
 		dev_err(pp->dev, "Failed to bring link up!\n");
 	} else {
-- 
1.9.1

Re: [PATCH 1/1] pci-imx6: add speed change timeout message

[Marek Vasut]

On Saturday, June 06, 2015 at 12:37:44 AM, Troy Kisky wrote:
> Currently, the timeout is never detected as count
> has a value of -1 if a timeout happens, but the code is checking
> for 0. Also, this patch removes the unneeded final wait
> if a timeout occurs.
> 
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>

Acked-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

Re: [PATCH 1/1] pci-imx6: add speed change timeout message

[Bjorn Helgaas]

Hi Troy,

On Fri, Jun 05, 2015 at 03:37:44PM -0700, Troy Kisky wrote:
> Currently, the timeout is never detected as count
> has a value of -1 if a timeout happens, but the code is checking
> for 0. Also, this patch removes the unneeded final wait
> if a timeout occurs.
> 
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> ---
>  drivers/pci/host/pci-imx6.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index fdb9536..51be92c 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -398,20 +398,22 @@ static int imx6_pcie_start_link(struct pcie_port *pp)
>  	writel(tmp, pp->dbi_base + PCIE_LINK_WIDTH_SPEED_CONTROL);
>  
>  	count = 200;
> -	while (count--) {
> +	while (1) {
>  		tmp = readl(pp->dbi_base + PCIE_LINK_WIDTH_SPEED_CONTROL);
>  		/* Test if the speed change finished. */
> -		if (!(tmp & PORT_LOGIC_SPEED_CHANGE))
> +		if (!(tmp & PORT_LOGIC_SPEED_CHANGE)) {
> +			/* Make sure link training is finished as well! */
> +			ret = imx6_pcie_wait_for_link(pp);
> +			break;
> +		}
> +		if (count-- == 0) {
> +			dev_err(pp->dev, "Speed change timeout\n");
> +			ret = -EINVAL;
>  			break;
> +		}
>  		usleep_range(100, 1000);
>  	}
>  
> -	/* Make sure link training is finished as well! */
> -	if (count)
> -		ret = imx6_pcie_wait_for_link(pp);
> -	else
> -		ret = -EINVAL;
> -
>  	if (ret) {
>  		dev_err(pp->dev, "Failed to bring link up!\n");
>  	} else {

This looks fine functionally, but I propose the following slight
rearrangement because I have another patch that makes all the
"wait_for_link" timeout loops look similar, and this loop might as
well be similar, too.

Unrelated to your patch, but noticed while doing this: what's the magic
constant 0x80 here?

+	tmp = readl(pp->dbi_base + 0x80);

Is that correct?  Can we add a symbolic name for it?

Bjorn


commit 24533d23e8fa
Author: Troy Kisky <troy.kisky@boundarydevices.com>
Date:   Fri Jun 12 14:30:16 2015 -0500

    PCI: imx6: Add speed change timeout message
    
    Currently, the timeout is never detected as count has a value of -1 if a
    timeout happens, but the code is checking for 0.  Also, this patch removes
    the unneeded final wait if a timeout occurs.
    
    [bhelgaas: reworked starting from http://lkml.kernel.org/r/1433543864-7252-1-git-send-email-troy.kisky@boundarydevices.com]
    Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index fdb9536..2bbd38d 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -352,6 +352,23 @@ static int imx6_pcie_wait_for_link(struct pcie_port *pp)
 	return 0;
 }
 
+static int imx6_pcie_wait_for_speed_change(struct pcie_port *pp)
+{
+	uint32_t tmp;
+	unsigned int retries;
+
+	for (retries = 0; retries < 200; retries++)
+		tmp = readl(pp->dbi_base + PCIE_LINK_WIDTH_SPEED_CONTROL);
+		/* Test if the speed change finished. */
+		if (!(tmp & PORT_LOGIC_SPEED_CHANGE))
+			return 0;
+		usleep_range(100, 1000);
+	}
+
+	dev_err(pp->dev, "Speed change timeout\n");
+	return -EINVAL;
+}
+
 static irqreturn_t imx6_pcie_msi_handler(int irq, void *arg)
 {
 	struct pcie_port *pp = arg;
@@ -363,7 +380,7 @@ static int imx6_pcie_start_link(struct pcie_port *pp)
 {
 	struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp);
 	uint32_t tmp;
-	int ret, count;
+	int ret;
 
 	/*
 	 * Force Gen1 operation when starting the link.  In case the link is
@@ -397,29 +414,22 @@ static int imx6_pcie_start_link(struct pcie_port *pp)
 	tmp |= PORT_LOGIC_SPEED_CHANGE;
 	writel(tmp, pp->dbi_base + PCIE_LINK_WIDTH_SPEED_CONTROL);
 
-	count = 200;
-	while (count--) {
-		tmp = readl(pp->dbi_base + PCIE_LINK_WIDTH_SPEED_CONTROL);
-		/* Test if the speed change finished. */
-		if (!(tmp & PORT_LOGIC_SPEED_CHANGE))
-			break;
-		usleep_range(100, 1000);
+	ret = imx6_pcie_wait_for_speed_change(pp);
+	if (ret) {
+		dev_err(pp->dev, "Failed to bring link up!\n");
+		return ret;
 	}
 
 	/* Make sure link training is finished as well! */
-	if (count)
-		ret = imx6_pcie_wait_for_link(pp);
-	else
-		ret = -EINVAL;
-
+	ret = imx6_pcie_wait_for_link(pp);
 	if (ret) {
 		dev_err(pp->dev, "Failed to bring link up!\n");
-	} else {
-		tmp = readl(pp->dbi_base + 0x80);
-		dev_dbg(pp->dev, "Link up, Gen=%i\n", (tmp >> 16) & 0xf);
+		return ret;
 	}
 
-	return ret;
+	tmp = readl(pp->dbi_base + 0x80);
+	dev_dbg(pp->dev, "Link up, Gen=%i\n", (tmp >> 16) & 0xf);
+	return 0;
 }
 
 static void imx6_pcie_host_init(struct pcie_port *pp)

Re: [PATCH 1/1] pci-imx6: add speed change timeout message

[Troy Kisky]

On 6/12/2015 1:20 PM, Bjorn Helgaas wrote:
> Hi Troy,
> 
> On Fri, Jun 05, 2015 at 03:37:44PM -0700, Troy Kisky wrote:
>> Currently, the timeout is never detected as count
>> has a value of -1 if a timeout happens, but the code is checking
>> for 0. Also, this patch removes the unneeded final wait
>> if a timeout occurs.
>>
>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>> ---
>>  drivers/pci/host/pci-imx6.c | 18 ++++++++++--------
>>  1 file changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
>> index fdb9536..51be92c 100644
>> --- a/drivers/pci/host/pci-imx6.c
>> +++ b/drivers/pci/host/pci-imx6.c
>> @@ -398,20 +398,22 @@ static int imx6_pcie_start_link(struct pcie_port *pp)
>>  	writel(tmp, pp->dbi_base + PCIE_LINK_WIDTH_SPEED_CONTROL);
>>  
>>  	count = 200;
>> -	while (count--) {
>> +	while (1) {
>>  		tmp = readl(pp->dbi_base + PCIE_LINK_WIDTH_SPEED_CONTROL);
>>  		/* Test if the speed change finished. */
>> -		if (!(tmp & PORT_LOGIC_SPEED_CHANGE))
>> +		if (!(tmp & PORT_LOGIC_SPEED_CHANGE)) {
>> +			/* Make sure link training is finished as well! */
>> +			ret = imx6_pcie_wait_for_link(pp);
>> +			break;
>> +		}
>> +		if (count-- == 0) {
>> +			dev_err(pp->dev, "Speed change timeout\n");
>> +			ret = -EINVAL;
>>  			break;
>> +		}
>>  		usleep_range(100, 1000);
>>  	}
>>  
>> -	/* Make sure link training is finished as well! */
>> -	if (count)
>> -		ret = imx6_pcie_wait_for_link(pp);
>> -	else
>> -		ret = -EINVAL;
>> -
>>  	if (ret) {
>>  		dev_err(pp->dev, "Failed to bring link up!\n");
>>  	} else {
> 
> This looks fine functionally, but I propose the following slight
> rearrangement because I have another patch that makes all the
> "wait_for_link" timeout loops look similar, and this loop might as
> well be similar, too.
> 
> Unrelated to your patch, but noticed while doing this: what's the magic
> constant 0x80 here?
> 
> +	tmp = readl(pp->dbi_base + 0x80);
> 
> Is that correct?  Can we add a symbolic name for it?
> 
> Bjorn
> 

The name in the manual for +x80 is
PCIE_RC_LCSR - Link Control and Status Register

Re: [PATCH 1/1] pci-imx6: add speed change timeout message

[Bjorn Helgaas]

On Fri, Jun 12, 2015 at 03:25:57PM -0700, Troy Kisky wrote:
> On 6/12/2015 1:20 PM, Bjorn Helgaas wrote:
> > Unrelated to your patch, but noticed while doing this: what's the magic
> > constant 0x80 here?
> > 
> > +	tmp = readl(pp->dbi_base + 0x80);
> > 
> > Is that correct?  Can we add a symbolic name for it?
> > 
> > Bjorn
> > 
> 
> The name in the manual for +x80 is
> PCIE_RC_LCSR - Link Control and Status Register


commit bc82467358d3793e1291e38cd01883e74e872eeb
Author: Bjorn Helgaas <bhelgaas@google.com>
Date:   Fri Jun 12 17:27:43 2015 -0500

    PCI: imx6: Add #define PCIE_RC_LCSR
    
    Define PCIE_RC_LCSR and use it instead of the bare offset "0x80."
    No functional change.
    
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index a032c01..57db1c1 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -47,6 +47,8 @@ struct imx6_pcie {
 #define PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN2	0x2
 #define PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK	0xf
 
+#define PCIE_RC_LCSR				0x80
+
 /* PCIe Port Logic registers (memory-mapped) */
 #define PL_OFFSET 0x700
 #define PCIE_PL_PFLR (PL_OFFSET + 0x08)
@@ -427,7 +429,7 @@ static int imx6_pcie_start_link(struct pcie_port *pp)
 		return ret;
 	}
 
-	tmp = readl(pp->dbi_base + 0x80);
+	tmp = readl(pp->dbi_base + PCIE_RC_LCSR);
 	dev_dbg(pp->dev, "Link up, Gen=%i\n", (tmp >> 16) & 0xf);
 	return 0;
 }