pci-imx6: add speed change timeout message

RFC PATCH New Troy Kisky 3 participants 8 messages May 21, 2015 – May 22, 2015

Subsystems (2)

Arm i.MX and Layerscape PCI

lore

Share & tools

lore

Troy Kisky

author

2015-05-21 18:35 UTC #0 lore raw

RFC PATCH 1/1 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.

Signed-off-by Troy Kisky <[email protected]>

---

This patch breaks pcie for imx6sx as my board always times out. So, if someone could check this on an imx6sx I'd appreciate it.

Signed-off-by Troy Kisky <[email protected]>

---
 drivers/pci/host/pci-imx6.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

drivers/pci/host/pci-imx6.c +10 -8

index fdb9536..51be92c 100644
@@ -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 {

Signature · 1 line

1.9.1

Marek Vasut

2015-05-21 21:19 UTC #1 lore raw

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

Quoted 21 lines · 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.
Signed-off-by Troy Kisky <[email protected]>
---
This patch breaks pcie for imx6sx as my board always times out. So, if someone could check this on an imx6sx I'd appreciate it.
Signed-off-by Troy Kisky <[email protected]>
---
 drivers/pci/host/pci-imx6.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)
drivers/pci/host/pci-imx6.c +0 -0
index fdb9536..51be92c 100644

drivers/pci/host/pci-imx6.c static int imx6_pcie_start_link(struct pcie_port *pp) · line 401

Quoted 4 lines

drivers/pci/host/pci-imx6.c +0 -1

@@ -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--) {

Uh, wouldn't "while (--count)" fix this as well, with a smaller patch?

Best regards,
Marek Vasut

Troy Kisky

author

2015-05-21 23:17 UTC #2 lore raw

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

Quoted 24 lines · Marek Vasut

Quoted 23 lines · 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.
Signed-off-by Troy Kisky <[email protected]>
---
This patch breaks pcie for imx6sx as my board always times out. So, if someone could check this on an imx6sx I'd appreciate it.
Signed-off-by Troy Kisky <[email protected]>
---
 drivers/pci/host/pci-imx6.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)
drivers/pci/host/pci-imx6.c +0 -0
index fdb9536..51be92c 100644
@@ -398,20 +398,22 @@ static int imx6_pcie_start_link(struct pcie_port *pp)
 	writel(tmp, pp->dbi_base + PCIE_LINK_WIDTH_SPEED_CONTROL);
 

Quoted 8 lines

drivers/pci/host/pci-imx6.c static int imx6_pcie_start_link(struct pcie_port *pp) · line 401
Quoted 2 lines
drivers/pci/host/pci-imx6.c +0 -1
 	count = 200;
-	while (count--) {
Uh, wouldn't "while (--count)" fix this as well, with a smaller patch?
Best regards,
Marek Vasut

Yes, but you'd have an unnecessary usleep_range (no check for finished after it) if a timeout happens.

Troy

Marek Vasut

2015-05-21 23:31 UTC #3 lore raw

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

Quoted 34 lines · Troy Kisky

Quoted 33 lines · Marek Vasut
Quoted 22 lines · 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.
Signed-off-by Troy Kisky <[email protected]>
---
This patch breaks pcie for imx6sx as my board always times out. So, if someone could check this on an imx6sx I'd appreciate it.
Signed-off-by Troy Kisky <[email protected]>
---
 drivers/pci/host/pci-imx6.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)
drivers/pci/host/pci-imx6.c +0 -0
index fdb9536..51be92c 100644
drivers/pci/host/pci-imx6.c static int imx6_pcie_start_link(struct pcie_port · line 404
Quoted 7 lines
drivers/pci/host/pci-imx6.c +0 -1
@@ -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--) {
Uh, wouldn't "while (--count)" fix this as well, with a smaller patch?

Quoted 5 lines

Quoted 2 lines
Best regards,
Marek Vasut
Yes, but you'd have an unnecessary usleep_range (no check for finished after it) if a timeout happens.

Oki, then I'll wait for Fabio to ACK this and that'd be it :)

Thanks!

Best regards,
Marek Vasut

Troy Kisky

author

2015-05-22 17:43 UTC #4 lore raw

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

Quoted 41 lines · Marek Vasut

Quoted 34 lines · Troy Kisky
Quoted 33 lines · Marek Vasut
Quoted 22 lines · 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.
Signed-off-by Troy Kisky <[email protected]>
---
This patch breaks pcie for imx6sx as my board always times out. So, if someone could check this on an imx6sx I'd appreciate it.
Signed-off-by Troy Kisky <[email protected]>
---
 drivers/pci/host/pci-imx6.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)
drivers/pci/host/pci-imx6.c +0 -0
index fdb9536..51be92c 100644
drivers/pci/host/pci-imx6.c static int imx6_pcie_start_link(struct pcie_port · line 404
Quoted 7 lines
drivers/pci/host/pci-imx6.c +0 -1
@@ -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--) {
Uh, wouldn't "while (--count)" fix this as well, with a smaller patch?
Quoted 5 lines
Quoted 2 lines
Best regards,
Marek Vasut
Yes, but you'd have an unnecessary usleep_range (no check for finished after it) if a timeout happens.

Quoted 7 lines

Oki, then I'll wait for Fabio to ACK this and that'd be it :)
Thanks!
Best regards,
Marek Vasut

This patch breaks my imx6sx board, so it shouldn't be applied until there is an explanation and work around of some sort.

Thanks
Troy

Fabio Estevam

reviewer

2015-05-22 18:02 UTC #5 lore raw

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

Hi Troy,

Quoted 2 lines · Troy Kisky

This patch breaks my imx6sx board, so it shouldn't be applied until there is an explanation and work around of some sort.

Do you have mx6sx PCI working with mainline or only with FSL 3.14 kernel?

Regards,

Fabio Estevam

Troy Kisky

author

2015-05-22 19:52 UTC #6 lore raw

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

Quoted 5 lines · Fabio Estevam

Hi Troy,
On Fri, May 22, 2015 at 2:43 PM, Troy Kisky
<[email protected]> wrote:

Quoted 9 lines

Quoted 2 lines
This patch breaks my imx6sx board, so it shouldn't be applied until there is an explanation and work around of some sort.
Do you have mx6sx PCI working with mainline or only with FSL 3.14 kernel?
Regards,
Fabio Estevam

Your right, mainline does not support pcie for imx6sx yet, so the patch will not cause a regression. I saw pcie in imx6sx.dtsi and assumed it did. I guess whoever adds imx6sx support can figure it out.

Thanks
Troy

Fabio Estevam

reviewer

2015-05-22 20:18 UTC #7 lore raw

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

Quoted 3 lines · Troy Kisky

Your right, mainline does not support pcie for imx6sx yet, so the patch will not cause a regression. I saw pcie in imx6sx.dtsi and assumed it did. I guess whoever adds imx6sx support can figure it out.

Yes, Richard Zhu has made some attemps to add mx6sx PCI support, but the support is not in place yet.

Maybe you can send your patch without RFC to the linux-pci list as well.

Please add Tim Harvey on Cc as he ran some extensive PCI tests and it would be good to have him look at your patch as well.

Regards,

Fabio Estevam