| Message ID | 1432233345-10160-1-git-send-email-troy.kisky@boundarydevices.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thursday, May 21, 2015 at 08:35:45 PM, 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. > > Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> > > --- > > 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 <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--) { Uh, wouldn't "while (--count)" fix this as well, with a smaller patch? Best regards, Marek Vasut
On 5/21/2015 2:19 PM, Marek Vasut wrote: > On Thursday, May 21, 2015 at 08:35:45 PM, 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. >> >> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> >> >> --- >> >> 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 <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--) { > > 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
On Friday, May 22, 2015 at 01:17:41 AM, Troy Kisky wrote: > On 5/21/2015 2:19 PM, Marek Vasut wrote: > > On Thursday, May 21, 2015 at 08:35:45 PM, 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. > >> > >> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> > >> > >> --- > >> > >> 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 <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--) { > > > > 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. Oki, then I'll wait for Fabio to ACK this and that'd be it :) Thanks! Best regards, Marek Vasut
On 5/21/2015 4:31 PM, Marek Vasut wrote: > On Friday, May 22, 2015 at 01:17:41 AM, Troy Kisky wrote: >> On 5/21/2015 2:19 PM, Marek Vasut wrote: >>> On Thursday, May 21, 2015 at 08:35:45 PM, 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. >>>> >>>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> >>>> >>>> --- >>>> >>>> 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 <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--) { >>> >>> 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. > > 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
Hi Troy, On Fri, May 22, 2015 at 2:43 PM, Troy Kisky <troy.kisky@boundarydevices.com> wrote: > 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
On 5/22/2015 11:02 AM, Fabio Estevam wrote: > Hi Troy, > > On Fri, May 22, 2015 at 2:43 PM, Troy Kisky > <troy.kisky@boundarydevices.com> wrote: > >> 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
On Fri, May 22, 2015 at 4:52 PM, Troy Kisky <troy.kisky@boundarydevices.com> wrote: > 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
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 {