* [PATCH 1/1] pci-imx6: add speed change timeout message @ 2015-06-05 22:37 Troy Kisky 2015-06-06 0:03 ` Marek Vasut 2015-06-12 20:20 ` Bjorn Helgaas 0 siblings, 2 replies; 5+ messages in thread From: Troy Kisky @ 2015-06-05 22:37 UTC (permalink / raw) To: bhelgaas; +Cc: festevam, marex, linux-pci, r65037, l.stach, tharvey, 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] pci-imx6: add speed change timeout message 2015-06-05 22:37 [PATCH 1/1] pci-imx6: add speed change timeout message Troy Kisky @ 2015-06-06 0:03 ` Marek Vasut 2015-06-12 20:20 ` Bjorn Helgaas 1 sibling, 0 replies; 5+ messages in thread From: Marek Vasut @ 2015-06-06 0:03 UTC (permalink / raw) To: Troy Kisky; +Cc: bhelgaas, festevam, linux-pci, r65037, l.stach, tharvey 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] pci-imx6: add speed change timeout message 2015-06-05 22:37 [PATCH 1/1] pci-imx6: add speed change timeout message Troy Kisky 2015-06-06 0:03 ` Marek Vasut @ 2015-06-12 20:20 ` Bjorn Helgaas 2015-06-12 22:25 ` Troy Kisky 1 sibling, 1 reply; 5+ messages in thread From: Bjorn Helgaas @ 2015-06-12 20:20 UTC (permalink / raw) To: Troy Kisky; +Cc: festevam, marex, linux-pci, r65037, l.stach, tharvey 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) ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] pci-imx6: add speed change timeout message 2015-06-12 20:20 ` Bjorn Helgaas @ 2015-06-12 22:25 ` Troy Kisky 2015-06-12 23:39 ` Bjorn Helgaas 0 siblings, 1 reply; 5+ messages in thread From: Troy Kisky @ 2015-06-12 22:25 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: festevam, marex, linux-pci, r65037, l.stach, tharvey 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] pci-imx6: add speed change timeout message 2015-06-12 22:25 ` Troy Kisky @ 2015-06-12 23:39 ` Bjorn Helgaas 0 siblings, 0 replies; 5+ messages in thread From: Bjorn Helgaas @ 2015-06-12 23:39 UTC (permalink / raw) To: Troy Kisky; +Cc: festevam, marex, linux-pci, r65037, l.stach, tharvey 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; } ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-06-12 23:39 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-06-05 22:37 [PATCH 1/1] pci-imx6: add speed change timeout message Troy Kisky 2015-06-06 0:03 ` Marek Vasut 2015-06-12 20:20 ` Bjorn Helgaas 2015-06-12 22:25 ` Troy Kisky 2015-06-12 23:39 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).