| Message ID | 1434614214-2085-1-git-send-email-dave.mueller@gmx.ch |
|---|---|
| State | Changes Requested |
| Headers | show |
Am Donnerstag, den 18.06.2015, 09:56 +0200 schrieb David Müller: > This problem has already been reported as > https://bugzilla.kernel.org/show_bug.cgi?id=100031 > > Signed-off-by: David Müller <dave.mueller@gmx.ch> > --- > drivers/pci/host/pci-imx6.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > index fdb9536..c63691c 100644 > --- a/drivers/pci/host/pci-imx6.c > +++ b/drivers/pci/host/pci-imx6.c > @@ -489,7 +489,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp) > * Wait a little bit, then re-check if the link finished > * the training. > */ > - usleep_range(1000, 2000); > + mdelay(20); While switching to mdelay might be the right thing to do here, you are also changing the timeout. This is a change in behavior and so not okay. Especially as you keep the CPU spinning for the duration of that timeout. > } > /* > * From L0, initiate MAC entry to gen2 if EP/RC supports gen2.
On Thu, Jun 18, 2015 at 09:56:54AM +0200, David Müller wrote: > This problem has already been reported as > https://bugzilla.kernel.org/show_bug.cgi?id=100031 > > Signed-off-by: David Müller <dave.mueller@gmx.ch> > --- > drivers/pci/host/pci-imx6.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > index fdb9536..c63691c 100644 > --- a/drivers/pci/host/pci-imx6.c > +++ b/drivers/pci/host/pci-imx6.c > @@ -489,7 +489,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp) > * Wait a little bit, then re-check if the link finished > * the training. > */ > - usleep_range(1000, 2000); > + mdelay(20); > } > /* > * From L0, initiate MAC entry to gen2 if EP/RC supports gen2. I asked in the bugzilla why imx6_pcie_link_up() looks nothing like the other pcie_host_ops.link_up() methods. Lucas responded: > the i.MX6 link up routine is a bit more complex compared to the other > designware drivers because of a hardware bug, where some FIFOs could > get out of sync when changing the link speed. We need to start the > link at Gen1, wait for it to be stable, then start a directed link > speed change if the EP is Gen2 and wait for the link to be stable > again. If the link doesn't come back, we need to start over. > I don't think any of the other DW PCIe implementations need this > workaround. That explanation makes sense, and a comment to that effect should be in the code. But I also wonder whether this workaround could be moved to imx6_pcie_start_link() (just renamed to imx6_pcie_establish_link()). Then imx6_pcie_link_up() could be a simple status check like the others. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 18, 2015 at 5:27 AM, Lucas Stach <l.stach@pengutronix.de> wrote: >> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c >> index fdb9536..c63691c 100644 >> --- a/drivers/pci/host/pci-imx6.c >> +++ b/drivers/pci/host/pci-imx6.c >> @@ -489,7 +489,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp) >> * Wait a little bit, then re-check if the link finished >> * the training. >> */ >> - usleep_range(1000, 2000); >> + mdelay(20); > > While switching to mdelay might be the right thing to do here, you are > also changing the timeout. This is a change in behavior and so not okay. msleep(2) is not recommended according to Documentation/timers/timers-howto.txt. What would be the proper fix then? Regards, Fabio Estevam -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in
Am Montag, den 22.06.2015, 10:30 -0300 schrieb Fabio Estevam: > On Thu, Jun 18, 2015 at 5:27 AM, Lucas Stach <l.stach@pengutronix.de> wrote: > > >> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > >> index fdb9536..c63691c 100644 > >> --- a/drivers/pci/host/pci-imx6.c > >> +++ b/drivers/pci/host/pci-imx6.c > >> @@ -489,7 +489,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp) > >> * Wait a little bit, then re-check if the link finished > >> * the training. > >> */ > >> - usleep_range(1000, 2000); > >> + mdelay(20); > > > > While switching to mdelay might be the right thing to do here, you are > > also changing the timeout. This is a change in behavior and so not okay. > > msleep(2) is not recommended according to Documentation/timers/timers-howto.txt. > > What would be the proper fix then? > mdelay(2) is completely reasonable. Note the difference between delay and sleep. The mid-term correct solution would be to move all the link-up handling out of the atomic path. Regards, Lucas
On Mon, Jun 22, 2015 at 10:37 AM, Lucas Stach <l.stach@pengutronix.de> wrote: > mdelay(2) is completely reasonable. Note the difference between delay > and sleep. Oh, that's right :-) Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in
On Thu, Jun 18, 2015 at 09:56:54AM +0200, David Müller wrote: > This problem has already been reported as > https://bugzilla.kernel.org/show_bug.cgi?id=100031 > > Signed-off-by: David Müller <dave.mueller@gmx.ch> Dropping for now, hoping for an update addressing the review comments. > --- > drivers/pci/host/pci-imx6.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > index fdb9536..c63691c 100644 > --- a/drivers/pci/host/pci-imx6.c > +++ b/drivers/pci/host/pci-imx6.c > @@ -489,7 +489,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp) > * Wait a little bit, then re-check if the link finished > * the training. > */ > - usleep_range(1000, 2000); > + mdelay(20); > } > /* > * From L0, initiate MAC entry to gen2 if EP/RC supports gen2. > -- > 1.8.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c index fdb9536..c63691c 100644 --- a/drivers/pci/host/pci-imx6.c +++ b/drivers/pci/host/pci-imx6.c @@ -489,7 +489,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp) * Wait a little bit, then re-check if the link finished * the training. */ - usleep_range(1000, 2000); + mdelay(20); } /* * From L0, initiate MAC entry to gen2 if EP/RC supports gen2.
This problem has already been reported as https://bugzilla.kernel.org/show_bug.cgi?id=100031 Signed-off-by: David Müller <dave.mueller@gmx.ch> --- drivers/pci/host/pci-imx6.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)