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@xxxxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxx]
Signed-off-by: Troy Kisky <troy.kisky@xxxxxxxxxxxxxxxxxxx>
Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
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)
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
