| Message ID | Pine.LNX.4.44L0.1204261020040.1530-100000@iolanthe.rowland.org |
|---|---|
| State | Not Applicable |
| Headers | show |
On Thu, Apr 26, 2012 at 10:25:41AM -0400, Alan Stern wrote: > On Thu, 26 Apr 2012, Calin Demian wrote: > > > I don't know how, but adding the following line in function > > quirk_usb_handoff_ohci makes the system work fine: > > > > /* reset requires max 10 us delay */ > > + udelay(1); > > for (cnt = 30; cnt > 0; --cnt) { /* ... allow extra time */ > > if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0) > > > > I observed thad the condition in the for loop is allways true, so the > > delay inside the loop is never executed. That implies that something is wrong with the complier here, and it optimized out the loop? Are you sure that really happened? greg k-h -- 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, 26 Apr 2012, Greg KH wrote: > On Thu, Apr 26, 2012 at 10:25:41AM -0400, Alan Stern wrote: > > On Thu, 26 Apr 2012, Calin Demian wrote: > > > > > I don't know how, but adding the following line in function > > > quirk_usb_handoff_ohci makes the system work fine: > > > > > > /* reset requires max 10 us delay */ > > > + udelay(1); > > > for (cnt = 30; cnt > 0; --cnt) { /* ... allow extra time */ > > > if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0) > > > > > > I observed thad the condition in the for loop is allways true, so the > > > delay inside the loop is never executed. > > That implies that something is wrong with the complier here, and it > optimized out the loop? Are you sure that really happened? Here's the complete context: /* reset requires max 10 us delay */ for (cnt = 30; cnt > 0; --cnt) { /* ... allow extra time */ if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0) break; udelay(1); } I think Calim means that the "(readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0" test succeeds on the first pass through the loop, so we break out early and the udelay(1) statement never gets executed. Alan Stern -- 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, Apr 26, 2012 at 11:16:17AM -0400, Alan Stern wrote: > On Thu, 26 Apr 2012, Greg KH wrote: > > > On Thu, Apr 26, 2012 at 10:25:41AM -0400, Alan Stern wrote: > > > On Thu, 26 Apr 2012, Calin Demian wrote: > > > > > > > I don't know how, but adding the following line in function > > > > quirk_usb_handoff_ohci makes the system work fine: > > > > > > > > /* reset requires max 10 us delay */ > > > > + udelay(1); > > > > for (cnt = 30; cnt > 0; --cnt) { /* ... allow extra time */ > > > > if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0) > > > > > > > > I observed thad the condition in the for loop is allways true, so the > > > > delay inside the loop is never executed. > > > > That implies that something is wrong with the complier here, and it > > optimized out the loop? Are you sure that really happened? > > Here's the complete context: > > /* reset requires max 10 us delay */ > for (cnt = 30; cnt > 0; --cnt) { /* ... allow extra time */ > if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0) > break; > udelay(1); > } > > I think Calim means that the "(readl(base + OHCI_CMDSTATUS) & OHCI_HCR) > == 0" test succeeds on the first pass through the loop, so we break out > early and the udelay(1) statement never gets executed. Ok, but if the readl() succeeds, then all should be fine? Ah, unless the hardware is really lying, but no, that would never happen, right? Nevermind, greg k-h -- 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
2012/4/26 Alan Stern <stern@rowland.harvard.edu>: > On Thu, 26 Apr 2012, Greg KH wrote: > >> On Thu, Apr 26, 2012 at 10:25:41AM -0400, Alan Stern wrote: >> > On Thu, 26 Apr 2012, Calin Demian wrote: >> > >> > > I don't know how, but adding the following line in function >> > > quirk_usb_handoff_ohci makes the system work fine: >> > > >> > > /* reset requires max 10 us delay */ >> > > + udelay(1); >> > > for (cnt = 30; cnt > 0; --cnt) { /* ... allow extra time */ >> > > if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0) >> > > >> > > I observed thad the condition in the for loop is allways true, so the >> > > delay inside the loop is never executed. >> >> That implies that something is wrong with the complier here, and it >> optimized out the loop? Are you sure that really happened? > > Here's the complete context: > > /* reset requires max 10 us delay */ > for (cnt = 30; cnt > 0; --cnt) { /* ... allow extra time */ > if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0) > break; > udelay(1); > } > > I think Calim means that the "(readl(base + OHCI_CMDSTATUS) & OHCI_HCR) > == 0" test succeeds on the first pass through the loop, so we break out > early and the udelay(1) statement never gets executed. > > Alan Stern > Yes, that's what I was saying. Storing the readl result in a variable and displaying it later shows that the (readl_result & OHCI_HCR) is 0 at first pass, but that doesn't imply reset is finished, I think. It would be too fast. Maybe the readl doesn't work for some reason... -- 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
I admit it might be a hardware problem. As I said in another mail, I couldn't duplicate the problem on other computers... If that's true than I'm sorry for bothering you. 2012/4/26 Greg KH <greg@kroah.com>: > On Thu, Apr 26, 2012 at 11:16:17AM -0400, Alan Stern wrote: >> On Thu, 26 Apr 2012, Greg KH wrote: >> >> > On Thu, Apr 26, 2012 at 10:25:41AM -0400, Alan Stern wrote: >> > > On Thu, 26 Apr 2012, Calin Demian wrote: >> > > >> > > > I don't know how, but adding the following line in function >> > > > quirk_usb_handoff_ohci makes the system work fine: >> > > > >> > > > /* reset requires max 10 us delay */ >> > > > + udelay(1); >> > > > for (cnt = 30; cnt > 0; --cnt) { /* ... allow extra time */ >> > > > if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0) >> > > > >> > > > I observed thad the condition in the for loop is allways true, so the >> > > > delay inside the loop is never executed. >> > >> > That implies that something is wrong with the complier here, and it >> > optimized out the loop? Are you sure that really happened? >> >> Here's the complete context: >> >> /* reset requires max 10 us delay */ >> for (cnt = 30; cnt > 0; --cnt) { /* ... allow extra time */ >> if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0) >> break; >> udelay(1); >> } >> >> I think Calim means that the "(readl(base + OHCI_CMDSTATUS) & OHCI_HCR) >> == 0" test succeeds on the first pass through the loop, so we break out >> early and the udelay(1) statement never gets executed. > > Ok, but if the readl() succeeds, then all should be fine? > > Ah, unless the hardware is really lying, but no, that would never > happen, right? > > Nevermind, > > greg k-h -- 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
Yes, it's ok. Thank you. Nevertheless, I think I will try to find out exactly what's happening. 2012/4/26 Alan Stern <stern@rowland.harvard.edu>: > On Thu, 26 Apr 2012, Calin Demian wrote: > >> I don't know how, but adding the following line in function >> quirk_usb_handoff_ohci makes the system work fine: >> >> /* reset requires max 10 us delay */ >> + udelay(1); >> for (cnt = 30; cnt > 0; --cnt) { /* ... allow extra time */ >> if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0) >> >> I observed thad the condition in the for loop is allways true, so the >> delay inside the loop is never executed. >> With the unmodified source, the execution doesn't stop at this point >> but at the next readl (I tried inserting printk and readl instructions >> after the writel which restores the fminterval and the execution >> stopped before exiting quirk_usb_handoff_ohci). >> I don't know how it relates to the ACPI message... (see bug 43073 in >> bugzilla.kernel.org). > > I don't know just what the problem is either, but there's nothing wrong > with delaying startup by 1 us. In fact, all we have to do is reorder > the statements within the loop. Is this patch okay for you? > > Alan Stern > > > > Index: usb-3.4/drivers/usb/host/pci-quirks.c > =================================================================== > --- usb-3.4.orig/drivers/usb/host/pci-quirks.c > +++ usb-3.4/drivers/usb/host/pci-quirks.c > @@ -520,9 +520,10 @@ static void __devinit quirk_usb_handoff_ > > /* reset requires max 10 us delay */ > for (cnt = 30; cnt > 0; --cnt) { /* ... allow extra time */ > + /* Some controllers enable HCR too soon, so always delay 1 us */ > + udelay(1); > if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0) > break; > - udelay(1); > } > writel(fminterval, base + OHCI_FMINTERVAL); > > -- 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, 26 Apr 2012, Calin Demian wrote: > Yes, it's ok. Thank you. > Nevertheless, I think I will try to find out exactly what's happening. Okay. It may be that your hardware never turns on the HCR bit in the first place. Remember, there will be no problem merging that patch, if it helps your system. Alan Stern -- 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
Ok, then. In my case it solves the problem. Thank you again. Calin Demian 2012/4/26 Alan Stern <stern@rowland.harvard.edu>: > On Thu, 26 Apr 2012, Calin Demian wrote: > >> Yes, it's ok. Thank you. >> Nevertheless, I think I will try to find out exactly what's happening. > > Okay. It may be that your hardware never turns on the HCR bit in the > first place. > > Remember, there will be no problem merging that patch, if it helps your > system. > > Alan Stern > -- 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
Index: usb-3.4/drivers/usb/host/pci-quirks.c =================================================================== --- usb-3.4.orig/drivers/usb/host/pci-quirks.c +++ usb-3.4/drivers/usb/host/pci-quirks.c @@ -520,9 +520,10 @@ static void __devinit quirk_usb_handoff_ /* reset requires max 10 us delay */ for (cnt = 30; cnt > 0; --cnt) { /* ... allow extra time */ + /* Some controllers enable HCR too soon, so always delay 1 us */ + udelay(1); if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0) break; - udelay(1); } writel(fminterval, base + OHCI_FMINTERVAL);