| Message ID | 51CAEEC4.4030304@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 26 Jun 2013, Roger Quadros wrote: > > Could the mapping be changed so that a different interrupt vector was > > used for wakeups and normal I/O? That would make this a little easier, > > although it wouldn't solve the general problem. > > I'm not sure which IRQ we can map it to, but it could be mapped to some > free IRQ number. Since it doesn't make things easier, I think we can leave > it as it is for now. All right. > > There's still a race problem. Suppose a normal wakeup interrupt occurs > > just before or as the controller gets suspended. By the time the code > > here runs, HCD_HW_ACCESSIBLE may have been cleared by the suspend > > routine. The interrupt would be lost. Depending on the design of the > > controller, the entire wakeup signal could end up getting lost as well. > > But if I call ehci_suspend() in the runtime_suspend handler, this race > won't happen right? That race doesn't apply to your system anyway; it matters only on systems where hcd->has_wakeup_irq isn't set. The only way to fix it involves changing ehci_suspend() somewhat (and making the equivalent change for other HCDs too). Those musings above were just me thinking out loud about the problems involved in implementing reliable wakeups. > > Do you know how the OMAP EHCI controller behaves? Under what > > conditions does it send the wakeup IRQ? How do you tell it to turn off > > the wakeup IRQ? > > Once the controller is suspended, the wakeup IRQ comes out-of-band. i.e. through > pad wakeup and pinctrl subsystem. > The only way to turn that wakeup off is to disable the wakeup enable bit on the pad. > This could be done by not putting the pins in the IDLE_WAKEUP state during > suspend. That's not what I meant. Never mind the pinctrl; I was asking about the EHCI controller itself. Under what circumstances does the controller assert its wakeup signal? And how do you tell it to stop asserting that signal? > Thanks for the review. > > I updated the ehci-omap.c driver to call ehci_suspend/resume during runtime_suspend/resume. > After that, it stopped detecting the port status change event when a device was plugged > to an external HUB. The wakeup irq was coming and the root hub/controller were being resumed, > but after that, no hub_irq. Wait a minute. I'm not clear on what happened. You're starting out with the controller, the root hub, and the external hub all suspended, right? Then you plugged a new device into the external hub. This caused the controller and the root hub to wake up, but not the external hub? > Adding some delay (or printk) somewhere in the resume path fixes the issue. I'm not sure what > is going on and why the delay is needed. Below is the ehci-omap patch. I've put the delay > in the runtime_resume handler. > > e.g. log > > [ 8.674377] usb usb1: usb wakeup-resume > [ 8.678833] ehci-omap 48064800.ehci: omap_ehci_runtime_resume > [ 8.695190] usb usb1: usb auto-resume > [ 8.699066] ehci-omap 48064800.ehci: resume root hub > [ 8.704437] hub 1-0:1.0: hub_resume > [ 8.708312] hub 1-0:1.0: port 2: status 0507 change 0000 > [ 8.714630] hub 1-0:1.0: state 7 ports 3 chg 0000 evt 0000 > > <---- gets stuck here in the failing case----> > > [ 8.723541] hub 1-0:1.0: state 7 ports 3 chg 0000 evt 0004 > [ 8.729400] ehci-omap 48064800.ehci: GetStatus port:2 status 001005 0 ACK POWER sig=se0 PE CONNECT > [ 8.753204] usb 1-2: usb wakeup-resume > [ 8.757293] usb 1-2: finish resume > [ 8.761627] hub 1-2:1.0: hub_resume Yeah, we need more debugging info. In ehci_irq(), right after the "pstatus = ehci_readl(..." line, what is the value of pstatus? And in the GetPortStatus case of ehci_hub_control(), right after the "temp = ehci_readl(..." line, what is the value of temp? > @@ -286,15 +293,70 @@ static const struct of_device_id omap_ehci_dt_ids[] = { > > MODULE_DEVICE_TABLE(of, omap_ehci_dt_ids); > > +static int omap_ehci_suspend(struct device *dev) > +{ > + struct usb_hcd *hcd = dev_get_drvdata(dev); > + bool do_wakeup = device_may_wakeup(dev); > + > + dev_dbg(dev, "%s: do_wakeup: %d\n", __func__, do_wakeup); > + > + return ehci_suspend(hcd, do_wakeup); > +} > + > +static int omap_ehci_resume(struct device *dev) > +{ > + struct usb_hcd *hcd = dev_get_drvdata(dev); > + > + dev_dbg(dev, "%s\n", __func__); > + > + return ehci_resume(hcd, false); > +} Those two routines look okay. > +static int omap_ehci_runtime_suspend(struct device *dev) > +{ > + struct usb_hcd *hcd = dev_get_drvdata(dev); > + struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; > + bool do_wakeup = device_may_wakeup(dev); > + > + dev_dbg(dev, "%s\n", __func__); > + > + if (omap->initialized) > + ehci_suspend(hcd, do_wakeup); Here you should not use do_wakeup. The second argument should always be "true", because wakeup is always enabled during runtime suspend. Also, why do you need omap->initialized? Do you think you might get a wakeup interrupt before the controller has been fully set up? I don't see how you could, given the pm_runtime_get_sync() call in the probe routine. Alan Stern
On 06/27/2013 06:40 PM, Alan Stern wrote: > On Wed, 26 Jun 2013, Roger Quadros wrote: > > > That race doesn't apply to your system anyway; it matters only on > systems where hcd->has_wakeup_irq isn't set. The only way to fix it > involves changing ehci_suspend() somewhat (and making the equivalent > change for other HCDs too). Those musings above were just me thinking > out loud about the problems involved in implementing reliable wakeups. > OK. >>> Do you know how the OMAP EHCI controller behaves? Under what >>> conditions does it send the wakeup IRQ? How do you tell it to turn off >>> the wakeup IRQ? >> >> Once the controller is suspended, the wakeup IRQ comes out-of-band. i.e. through >> pad wakeup and pinctrl subsystem. >> The only way to turn that wakeup off is to disable the wakeup enable bit on the pad. >> This could be done by not putting the pins in the IDLE_WAKEUP state during >> suspend. > > That's not what I meant. Never mind the pinctrl; I was asking about > the EHCI controller itself. Under what circumstances does the > controller assert its wakeup signal? And how do you tell it to stop > asserting that signal? I believe this would be through the EHCI Interrupt enable register (USBINTR). I'm not aware of any other mechanism. >> I updated the ehci-omap.c driver to call ehci_suspend/resume during runtime_suspend/resume. >> After that, it stopped detecting the port status change event when a device was plugged >> to an external HUB. The wakeup irq was coming and the root hub/controller were being resumed, >> but after that, no hub_irq. > > Wait a minute. I'm not clear on what happened. You're starting out > with the controller, the root hub, and the external hub all suspended, > right? Then you plugged a new device into the external hub. This This is right. > caused the controller and the root hub to wake up, but not the external > hub? > Right. It seems the external hub has signaled remote wakeup but the kernel doesn't resume the root hub's port it is connected to. By observing the detailed logs below you can see that the root hub does not generate an INTerrupt transaction to notify the port status change event. I've captured the pstatus and GetPortStatus info as well. Failing case ------------ [ 16.108032] usb usb1: usb auto-resume [ 16.108062] ehci-omap 48064800.ehci: resume root hub [ 16.108154] hub 1-0:1.0: hub_resume [ 16.108398] ehci_hub_control GetPortStatus, port 1 temp = 0x1000 [ 16.108459] ehci_hub_control GetPortStatus, port 2 temp = 0x14c5 [ 16.108551] hub 1-0:1.0: port 2: status 0507 change 0000 [ 16.108612] ehci_hub_control GetPortStatus, port 3 temp = 0x1000 [ 16.108642] hub 1-0:1.0: hub_activate submitting urb [ 16.109222] ehci_irq port 3 pstatus 0x1000 [ 16.109222] ehci_irq port 2 pstatus 0x14c5 [ 16.109252] ehci_irq port 1 pstatus 0x1000 [ 16.109374] hub 1-0:1.0: state 7 ports 3 chg 0000 evt 0000 Passing case ------------ / # [ 19.704589] usb usb1: usb wakeup-resume [ 19.709075] ehci-omap 48064800.ehci: omap_ehci_runtime_resume [ 19.715423] usb usb1: usb auto-resume [ 19.719299] ehci-omap 48064800.ehci: resume root hub [ 19.724670] hub 1-0:1.0: hub_resume [ 19.728424] ehci_hub_control GetPortStatus, port 1 temp = 0x1000 [ 19.734863] ehci_hub_control GetPortStatus, port 2 temp = 0x14c5 [ 19.741271] hub 1-0:1.0: port 2: status 0507 change 0000 [ 19.746948] ehci_hub_control GetPortStatus, port 3 temp = 0x1000 [ 19.753448] hub 1-0:1.0: hub_activate submitting urb [ 19.759216] ehci_irq port 3 pstatus 0x1000 [ 19.763519] ehci_irq port 2 pstatus 0x14c5 [ 19.767822] ehci_irq port 1 pstatus 0x1000 [ 19.772155] hub 1-0:1.0: hub_irq <---This is the Port Status change hub INT which is missing in the failing case---> [ 19.775604] hub 1-0:1.0: hub_irq submitting urb [ 19.780548] hub 1-0:1.0: state 7 ports 3 chg 0000 evt 0004 [ 19.786407] hub 1-0:1.0: hub_irq [ 19.789916] hub 1-0:1.0: hub_irq submitting urb [ 19.794799] ehci_hub_control GetPortStatus, port 2 temp = 0x14c5 [ 19.801147] ehci-omap 48064800.ehci: GetStatus port:2 status 001005 0 ACK POWER sig=se0 PE CONNECT [ 19.822937] usb 1-2: usb wakeup-resume [ 19.826995] ehci_hub_control GetPortStatus, port 2 temp = 0x1005 [ 19.833404] usb 1-2: finish resume [ 19.837738] hub 1-2:1.0: hub_resume [ 19.841613] hub 1-2:1.0: port 1: status 0507 change 0000 [ 19.848358] hub 1-2:1.0: port 4: status 0101 change 0001 [ 19.962890] hub 1-2:1.0: hub_activate submitting urb [ 19.968139] ehci-omap 48064800.ehci: reused qh dd450200 schedule [ 19.974456] usb 1-2: link qh256-0001/dd450200 start 1 [1/0 us] [ 19.980743] hub 1-0:1.0: resume on port 2, status 0 [ 19.985961] hub 1-0:1.0: state 7 ports 3 chg 0000 evt 0000 [ 19.991760] hub 1-2:1.0: state 7 ports 5 chg 0010 evt 0000 [ 19.997741] hub 1-2:1.0: port 4, status 0101, change 0000, 12 Mb/s [ 20.083129] usb 1-2.4: new high-speed USB device number 4 using ehci-omap <snip> One more thing is that the delay didn't help if I reduce printk verbosity to 7. So the debug prints are also adding some delays around the place which is affecting the behaviour. >> +static int omap_ehci_runtime_suspend(struct device *dev) >> +{ >> + struct usb_hcd *hcd = dev_get_drvdata(dev); >> + struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; >> + bool do_wakeup = device_may_wakeup(dev); >> + >> + dev_dbg(dev, "%s\n", __func__); >> + >> + if (omap->initialized) >> + ehci_suspend(hcd, do_wakeup); > > Here you should not use do_wakeup. The second argument should always > be "true", because wakeup is always enabled during runtime suspend. OK. > > Also, why do you need omap->initialized? Do you think you might get a > wakeup interrupt before the controller has been fully set up? I don't > see how you could, given the pm_runtime_get_sync() call in the probe > routine. > During probe we need to runtime_resume the device before usb_add_hcd() since the controller clocks must be enabled before any registers are accessed. However, we cannot call ehci_resume() before usb_add_hcd(). So to prevent this chicken & egg situation, I've used the omap->initialized flag. It only indicates that the ehci structures are initialized and we can call ehci_resume/suspend(). cheers, -roger
On Fri, 28 Jun 2013, Roger Quadros wrote: > > That's not what I meant. Never mind the pinctrl; I was asking about > > the EHCI controller itself. Under what circumstances does the > > controller assert its wakeup signal? And how do you tell it to stop > > asserting that signal? > > I believe this would be through the EHCI Interrupt enable register (USBINTR). > I'm not aware of any other mechanism. That's strange, because ehci_suspend() sets the intr_enable register to 0. So how do you ever get any wakeup interrupts at all? > Right. It seems the external hub has signaled remote wakeup but the kernel doesn't > resume the root hub's port it is connected to. > > By observing the detailed logs below you can see that the root hub does not generate > an INTerrupt transaction to notify the port status change event. I've captured the pstatus > and GetPortStatus info as well. We don't need an interrupt. The driver is supposed to detect the remote wakeup sent by the external hub all by itself. > Failing case > ------------ > > [ 16.108032] usb usb1: usb auto-resume > [ 16.108062] ehci-omap 48064800.ehci: resume root hub > [ 16.108154] hub 1-0:1.0: hub_resume > [ 16.108398] ehci_hub_control GetPortStatus, port 1 temp = 0x1000 > [ 16.108459] ehci_hub_control GetPortStatus, port 2 temp = 0x14c5 Here's where we should detect it. Look at the GetPortStatus case in ehci_hub_control(); the PORT_RESUME bit (0x0040) is set in temp, so the "Remote Wakeup received?" code should run. In particular, these lines should run: /* resume signaling for 20 msec */ ehci->reset_done[wIndex] = jiffies + msecs_to_jiffies(20); usb_hcd_start_port_resume(&hcd->self, wIndex); /* check the port again */ mod_timer(&ehci_to_hcd(ehci)->rh_timer, ehci->reset_done[wIndex]); Therefore 20 ms later, around timestamp 16.128459, ehci_hub_status_data() should have been called. At that time, the root-hub port should have been fully resumed. > [ 16.108551] hub 1-0:1.0: port 2: status 0507 change 0000 > [ 16.108612] ehci_hub_control GetPortStatus, port 3 temp = 0x1000 > [ 16.108642] hub 1-0:1.0: hub_activate submitting urb > [ 16.109222] ehci_irq port 3 pstatus 0x1000 > [ 16.109222] ehci_irq port 2 pstatus 0x14c5 > [ 16.109252] ehci_irq port 1 pstatus 0x1000 > [ 16.109374] hub 1-0:1.0: state 7 ports 3 chg 0000 evt 0000 But apparently nothing happened. Why not? Did the rh_timer get reset? Maybe you can find out what went wrong. (Hmmm, we seem to be missing a set_bit(wIndex, &ehci->resuming_ports); line in there...) > > Also, why do you need omap->initialized? Do you think you might get a > > wakeup interrupt before the controller has been fully set up? I don't > > see how you could, given the pm_runtime_get_sync() call in the probe > > routine. > > > > During probe we need to runtime_resume the device before usb_add_hcd() since the > controller clocks must be enabled before any registers are accessed. > However, we cannot call ehci_resume() before usb_add_hcd(). So to prevent this > chicken & egg situation, I've used the omap->initialized flag. It only indicates that > the ehci structures are initialized and we can call ehci_resume/suspend(). Ah, yes. Other subsystems, such as PCI, face exactly the same problem. You probably shouldn't call it "initialized", though, because the same issue arises in ehci_hcd_omap_remove() -- the pm_runtime_put_sync() there would end up calling ehci_suspend() after usb_remove_hcd(). "bound" or "started" would be better names. Alan Stern
On 06/28/2013 10:06 PM, Alan Stern wrote: > On Fri, 28 Jun 2013, Roger Quadros wrote: > >>> That's not what I meant. Never mind the pinctrl; I was asking about >>> the EHCI controller itself. Under what circumstances does the >>> controller assert its wakeup signal? And how do you tell it to stop >>> asserting that signal? >> >> I believe this would be through the EHCI Interrupt enable register (USBINTR). >> I'm not aware of any other mechanism. > > That's strange, because ehci_suspend() sets the intr_enable register to > 0. So how do you ever get any wakeup interrupts at all? Because after ehci_suspend() for OMAP, we solely rely on the out of band wake up mechanism. i.e. Pad wakeup. > >> Right. It seems the external hub has signaled remote wakeup but the kernel doesn't >> resume the root hub's port it is connected to. >> >> By observing the detailed logs below you can see that the root hub does not generate >> an INTerrupt transaction to notify the port status change event. I've captured the pstatus >> and GetPortStatus info as well. > > We don't need an interrupt. The driver is supposed to detect the > remote wakeup sent by the external hub all by itself. OK. Then it could point to a bug in our stack. > >> Failing case >> ------------ >> >> [ 16.108032] usb usb1: usb auto-resume >> [ 16.108062] ehci-omap 48064800.ehci: resume root hub >> [ 16.108154] hub 1-0:1.0: hub_resume >> [ 16.108398] ehci_hub_control GetPortStatus, port 1 temp = 0x1000 >> [ 16.108459] ehci_hub_control GetPortStatus, port 2 temp = 0x14c5 > > Here's where we should detect it. Look at the GetPortStatus case in > ehci_hub_control(); the PORT_RESUME bit (0x0040) is set in temp, so the > "Remote Wakeup received?" code should run. In particular, these lines > should run: > > /* resume signaling for 20 msec */ > ehci->reset_done[wIndex] = jiffies > + msecs_to_jiffies(20); > usb_hcd_start_port_resume(&hcd->self, wIndex); > /* check the port again */ > mod_timer(&ehci_to_hcd(ehci)->rh_timer, > ehci->reset_done[wIndex]); > > Therefore 20 ms later, around timestamp 16.128459, > ehci_hub_status_data() should have been called. At that time, the > root-hub port should have been fully resumed. OK. right. > >> [ 16.108551] hub 1-0:1.0: port 2: status 0507 change 0000 >> [ 16.108612] ehci_hub_control GetPortStatus, port 3 temp = 0x1000 >> [ 16.108642] hub 1-0:1.0: hub_activate submitting urb >> [ 16.109222] ehci_irq port 3 pstatus 0x1000 >> [ 16.109222] ehci_irq port 2 pstatus 0x14c5 >> [ 16.109252] ehci_irq port 1 pstatus 0x1000 >> [ 16.109374] hub 1-0:1.0: state 7 ports 3 chg 0000 evt 0000 > > But apparently nothing happened. Why not? Did the rh_timer get reset? > Maybe you can find out what went wrong. > Sure. I'll investigate. > (Hmmm, we seem to be missing a > > set_bit(wIndex, &ehci->resuming_ports); > > line in there...) > >>> Also, why do you need omap->initialized? Do you think you might get a >>> wakeup interrupt before the controller has been fully set up? I don't >>> see how you could, given the pm_runtime_get_sync() call in the probe >>> routine. >>> >> >> During probe we need to runtime_resume the device before usb_add_hcd() since the >> controller clocks must be enabled before any registers are accessed. >> However, we cannot call ehci_resume() before usb_add_hcd(). So to prevent this >> chicken & egg situation, I've used the omap->initialized flag. It only indicates that >> the ehci structures are initialized and we can call ehci_resume/suspend(). > > Ah, yes. Other subsystems, such as PCI, face exactly the same problem. > > You probably shouldn't call it "initialized", though, because the same > issue arises in ehci_hcd_omap_remove() -- the pm_runtime_put_sync() > there would end up calling ehci_suspend() after usb_remove_hcd(). > "bound" or "started" would be better names. > OK. Started seems fine. cheers, -roger
On Mon, 1 Jul 2013, Roger Quadros wrote: > On 06/28/2013 10:06 PM, Alan Stern wrote: > > On Fri, 28 Jun 2013, Roger Quadros wrote: > > > >>> That's not what I meant. Never mind the pinctrl; I was asking about > >>> the EHCI controller itself. Under what circumstances does the > >>> controller assert its wakeup signal? And how do you tell it to stop > >>> asserting that signal? > >> > >> I believe this would be through the EHCI Interrupt enable register (USBINTR). > >> I'm not aware of any other mechanism. > > > > That's strange, because ehci_suspend() sets the intr_enable register to > > 0. So how do you ever get any wakeup interrupts at all? > > Because after ehci_suspend() for OMAP, we solely rely on the out of band wake up > mechanism. i.e. Pad wakeup. I don't know what Pad wakeup is. The wakeup signal has to originate from the EHCI controller, doesn't it? If not, how does the Pad know when a wakeup is needed? > We can't enable_irq at the start as the controller will only be resumed > after usb_remote_wakeup(). Hmmm, yes, I had realized that at one point and then forgot it. You don't want an IRQ to arrive before you're prepared to handle it. This suggests that you really want to call enable_irq() call at the end of ehci_resume() instead of the beginning. (Convert the "return 0" to a jump to the end of the routine.) Alan Stern
On Mon, Jul 01, 2013 at 12:24:07PM -0400, Alan Stern wrote: > On Mon, 1 Jul 2013, Roger Quadros wrote: > > > On 06/28/2013 10:06 PM, Alan Stern wrote: > > > On Fri, 28 Jun 2013, Roger Quadros wrote: > > > > > >>> That's not what I meant. Never mind the pinctrl; I was asking about > > >>> the EHCI controller itself. Under what circumstances does the > > >>> controller assert its wakeup signal? And how do you tell it to stop > > >>> asserting that signal? > > >> > > >> I believe this would be through the EHCI Interrupt enable register (USBINTR). > > >> I'm not aware of any other mechanism. > > > > > > That's strange, because ehci_suspend() sets the intr_enable register to > > > 0. So how do you ever get any wakeup interrupts at all? > > > > Because after ehci_suspend() for OMAP, we solely rely on the out of band wake up > > mechanism. i.e. Pad wakeup. > > I don't know what Pad wakeup is. The wakeup signal has to originate > from the EHCI controller, doesn't it? If not, how does the Pad know > when a wakeup is needed? That's really an OMAP thing, I guess. Pad wakeup sits in the PRCM (IIRC) inside and always on power domain. EHCI sits in another power domain which be turned off. When it's turned off (power gated and clock gated) the EHCI block has no means to wakeup whatsoever. That's when pad wakeup comes into play. It is generated when PRCM sees a change in the actual pad of the die. To check who should 'own' the wakeup, it checks the muxing settings to verify whose pad is that.
On Mon, 1 Jul 2013, Felipe Balbi wrote: > > I don't know what Pad wakeup is. The wakeup signal has to originate > > from the EHCI controller, doesn't it? If not, how does the Pad know > > when a wakeup is needed? > > That's really an OMAP thing, I guess. Pad wakeup sits in the PRCM (IIRC) > inside and always on power domain. EHCI sits in another power domain > which be turned off. When it's turned off (power gated and clock gated) > the EHCI block has no means to wakeup whatsoever. That's when pad wakeup > comes into play. It is generated when PRCM sees a change in the actual > pad of the die. To check who should 'own' the wakeup, it checks the > muxing settings to verify whose pad is that. How does the PRCM know which changes should generate wakeup events? In the EHCI controller, this is managed by the settings of the WKOC_E, WKDSCNNT_E, and WKCNNT_E bits in the PORTSC registers. But if EHCI is powered off, those bits can't be used. Also, once the wakeup signal has been turned on, how does it get turned off again? Alan Stern
On 07/02/2013 12:01 AM, Alan Stern wrote: > On Mon, 1 Jul 2013, Felipe Balbi wrote: > >>> I don't know what Pad wakeup is. The wakeup signal has to originate >>> from the EHCI controller, doesn't it? If not, how does the Pad know >>> when a wakeup is needed? >> >> That's really an OMAP thing, I guess. Pad wakeup sits in the PRCM (IIRC) >> inside and always on power domain. EHCI sits in another power domain >> which be turned off. When it's turned off (power gated and clock gated) >> the EHCI block has no means to wakeup whatsoever. That's when pad wakeup >> comes into play. It is generated when PRCM sees a change in the actual >> pad of the die. To check who should 'own' the wakeup, it checks the >> muxing settings to verify whose pad is that. > > How does the PRCM know which changes should generate wakeup events? It doesn't know. It will always generate a wakeup on any change in the monitored pins. We can only configure which pins we want to monitor. So we can't choose which wakeup events we want to monitor. We just can enable or disable all events. > In the EHCI controller, this is managed by the settings of the WKOC_E, > WKDSCNNT_E, and WKCNNT_E bits in the PORTSC registers. But if EHCI is > powered off, those bits can't be used. Is "powered off" same as ehci_suspend? If yes then how does it work on other systems for remote wakeup? > > Also, once the wakeup signal has been turned on, how does it get turned > off again? That is taken care of by the OMAP pinctrl driver. It will always turn off the wakeup signal once the event is detected and forwarded as an IRQ. I know that all this is a bit ugly :(. cheers, -roger
On Tue, 2 Jul 2013, Roger Quadros wrote: > On 07/02/2013 12:01 AM, Alan Stern wrote: > > On Mon, 1 Jul 2013, Felipe Balbi wrote: > > > >>> I don't know what Pad wakeup is. The wakeup signal has to originate > >>> from the EHCI controller, doesn't it? If not, how does the Pad know > >>> when a wakeup is needed? > >> > >> That's really an OMAP thing, I guess. Pad wakeup sits in the PRCM (IIRC) > >> inside and always on power domain. EHCI sits in another power domain > >> which be turned off. When it's turned off (power gated and clock gated) > >> the EHCI block has no means to wakeup whatsoever. That's when pad wakeup > >> comes into play. It is generated when PRCM sees a change in the actual > >> pad of the die. To check who should 'own' the wakeup, it checks the > >> muxing settings to verify whose pad is that. > > > > How does the PRCM know which changes should generate wakeup events? > > It doesn't know. It will always generate a wakeup on any change in the monitored pins. > We can only configure which pins we want to monitor. > So we can't choose which wakeup events we want to monitor. We just can > enable or disable all events. > > > In the EHCI controller, this is managed by the settings of the WKOC_E, > > WKDSCNNT_E, and WKCNNT_E bits in the PORTSC registers. But if EHCI is > > powered off, those bits can't be used. > > Is "powered off" same as ehci_suspend? If yes then how does it work on other systems > for remote wakeup? Above, Felipe wrote that on OMAP, EHCI sits in a power domain which is turned off: power gated and clock gated. ehci_suspend() doesn't do those things, but they are what I was referring to. A PCI-based EHCI controller has two power sources: the core well (which is turned off during suspend) and the auxiliary well (which remains powered). That's how remote wakeup works; it uses the auxiliary well. > > Also, once the wakeup signal has been turned on, how does it get turned > > off again? > > That is taken care of by the OMAP pinctrl driver. It will always turn off the > wakeup signal once the event is detected and forwarded as an IRQ. > > I know that all this is a bit ugly :(. I still a little confused. The wakeup signal turns on. Then the pinctrl driver sees it, forwards it as an IRQ, and turns the wakeup signal off. But what if the IRQ is disabled (as it would be with your patch)? Does the IRQ line remain active until it causes an interrupt? What eventually turns off the IRQ line? Alan Stern
On 07/02/2013 08:17 PM, Alan Stern wrote: > On Tue, 2 Jul 2013, Roger Quadros wrote: > >> On 07/02/2013 12:01 AM, Alan Stern wrote: >>> On Mon, 1 Jul 2013, Felipe Balbi wrote: >>> >>>>> I don't know what Pad wakeup is. The wakeup signal has to originate >>>>> from the EHCI controller, doesn't it? If not, how does the Pad know >>>>> when a wakeup is needed? >>>> >>>> That's really an OMAP thing, I guess. Pad wakeup sits in the PRCM (IIRC) >>>> inside and always on power domain. EHCI sits in another power domain >>>> which be turned off. When it's turned off (power gated and clock gated) >>>> the EHCI block has no means to wakeup whatsoever. That's when pad wakeup >>>> comes into play. It is generated when PRCM sees a change in the actual >>>> pad of the die. To check who should 'own' the wakeup, it checks the >>>> muxing settings to verify whose pad is that. >>> >>> How does the PRCM know which changes should generate wakeup events? >> >> It doesn't know. It will always generate a wakeup on any change in the monitored pins. >> We can only configure which pins we want to monitor. >> So we can't choose which wakeup events we want to monitor. We just can >> enable or disable all events. >> >>> In the EHCI controller, this is managed by the settings of the WKOC_E, >>> WKDSCNNT_E, and WKCNNT_E bits in the PORTSC registers. But if EHCI is >>> powered off, those bits can't be used. >> >> Is "powered off" same as ehci_suspend? If yes then how does it work on other systems >> for remote wakeup? > > Above, Felipe wrote that on OMAP, EHCI sits in a power domain which is > turned off: power gated and clock gated. ehci_suspend() doesn't do > those things, but they are what I was referring to. OK right, those things are done by OMAP platform support code. > > A PCI-based EHCI controller has two power sources: the core well (which > is turned off during suspend) and the auxiliary well (which remains > powered). That's how remote wakeup works; it uses the auxiliary well. > OK. Thanks for the info. >>> Also, once the wakeup signal has been turned on, how does it get turned >>> off again? >> >> That is taken care of by the OMAP pinctrl driver. It will always turn off the >> wakeup signal once the event is detected and forwarded as an IRQ. >> >> I know that all this is a bit ugly :(. > > I still a little confused. The wakeup signal turns on. Then the > pinctrl driver sees it, forwards it as an IRQ, and turns the wakeup > signal off. But what if the IRQ is disabled (as it would be with your > patch)? Does the IRQ line remain active until it causes an interrupt? > What eventually turns off the IRQ line? > In the beagle/panda board, the USB lines of the OMAP are used in ULPI mode. Here we are monitoring DATA0, DATA1 and DATA3 lines which get configured as Linestate and Interrupt of the PHY device whenever the PHY is put into suspend mode. This usually happens when we suspend the EHCI controller. When EHCI is suspended, the pinctrl wakeup mechanism is active. Whenever there is a change in the monitored lines we get the PAD IRQ and hence the EHCI IRQ. We don't really care much about which line changed state. (NOTE: while suspending, we didn't disable the EHCI IRQ). So we will always get the first IRQ and then we disable it and queue a hub_resume_work. Then EHCI resumes, pinctrl wakeup is disabled and EHCI IRQ is enabled. When pinctrl wakeup mechanism is active, it clears the wakeup event flag after a PAD IRQ, but it has no control on disabling the interrupt source. If there is a change in the pad, the PAD IRQ will fire again. cheers, -roger
Hi, On Tue, Jul 02, 2013 at 01:17:58PM -0400, Alan Stern wrote: > A PCI-based EHCI controller has two power sources: the core well (which > is turned off during suspend) and the auxiliary well (which remains > powered). That's how remote wakeup works; it uses the auxiliary well. This, kinda, matches what OMAP tries to do with pad wakeup. Just that pad wakeup sits outside of the device itself. Perhaps we could look into how PCI handles the aux well and take some inspiration from there. Any pointers under drivers/pci/ would be great :-)
On 07/03/2013 03:57 PM, Felipe Balbi wrote: > Hi, > > On Tue, Jul 02, 2013 at 01:17:58PM -0400, Alan Stern wrote: >> A PCI-based EHCI controller has two power sources: the core well (which >> is turned off during suspend) and the auxiliary well (which remains >> powered). That's how remote wakeup works; it uses the auxiliary well. > > This, kinda, matches what OMAP tries to do with pad wakeup. Just that > pad wakeup sits outside of the device itself. Perhaps we could look into > how PCI handles the aux well and take some inspiration from there. > > Any pointers under drivers/pci/ would be great :-) > From what I understood, auxiliary well is just a power source, and it keeps the EHCI controller powered even during suspend. If that is true then it is different from our situation as we power down the EHCI controller completely. cheers, -roger
On Wed, Jul 03, 2013 at 04:06:04PM +0300, Roger Quadros wrote: > On 07/03/2013 03:57 PM, Felipe Balbi wrote: > > Hi, > > > > On Tue, Jul 02, 2013 at 01:17:58PM -0400, Alan Stern wrote: > >> A PCI-based EHCI controller has two power sources: the core well (which > >> is turned off during suspend) and the auxiliary well (which remains > >> powered). That's how remote wakeup works; it uses the auxiliary well. > > > > This, kinda, matches what OMAP tries to do with pad wakeup. Just that > > pad wakeup sits outside of the device itself. Perhaps we could look into > > how PCI handles the aux well and take some inspiration from there. > > > > Any pointers under drivers/pci/ would be great :-) > > > From what I understood, auxiliary well is just a power source, and it keeps > the EHCI controller powered even during suspend. > > If that is true then it is different from our situation as we power down the > EHCI controller completely. right but our "auxiliary well" keeps PRCM powered which can wake EHCI up ;-) What I'm saying is that from ehci-omap's perspective, there's very little difference, specially since we route the wakeup through the same IRQ line anyway. Perhaps we could take some inspiration from the PCI land to make our hwmod/omap_device a little easier from driver perspective. Or maybe it doesn't make sense ;-)
On 06/28/2013 10:06 PM, Alan Stern wrote: > On Fri, 28 Jun 2013, Roger Quadros wrote: > >>> That's not what I meant. Never mind the pinctrl; I was asking about >>> the EHCI controller itself. Under what circumstances does the >>> controller assert its wakeup signal? And how do you tell it to stop >>> asserting that signal? >> >> I believe this would be through the EHCI Interrupt enable register (USBINTR). >> I'm not aware of any other mechanism. > > That's strange, because ehci_suspend() sets the intr_enable register to > 0. So how do you ever get any wakeup interrupts at all? > >> Right. It seems the external hub has signaled remote wakeup but the kernel doesn't >> resume the root hub's port it is connected to. >> >> By observing the detailed logs below you can see that the root hub does not generate >> an INTerrupt transaction to notify the port status change event. I've captured the pstatus >> and GetPortStatus info as well. > > We don't need an interrupt. The driver is supposed to detect the > remote wakeup sent by the external hub all by itself. > >> Failing case >> ------------ >> >> [ 16.108032] usb usb1: usb auto-resume >> [ 16.108062] ehci-omap 48064800.ehci: resume root hub >> [ 16.108154] hub 1-0:1.0: hub_resume >> [ 16.108398] ehci_hub_control GetPortStatus, port 1 temp = 0x1000 >> [ 16.108459] ehci_hub_control GetPortStatus, port 2 temp = 0x14c5 > > Here's where we should detect it. Look at the GetPortStatus case in > ehci_hub_control(); the PORT_RESUME bit (0x0040) is set in temp, so the > "Remote Wakeup received?" code should run. In particular, these lines > should run: > > /* resume signaling for 20 msec */ > ehci->reset_done[wIndex] = jiffies > + msecs_to_jiffies(20); > usb_hcd_start_port_resume(&hcd->self, wIndex); > /* check the port again */ > mod_timer(&ehci_to_hcd(ehci)->rh_timer, > ehci->reset_done[wIndex]); > > Therefore 20 ms later, around timestamp 16.128459, > ehci_hub_status_data() should have been called. At that time, the > root-hub port should have been fully resumed. > >> [ 16.108551] hub 1-0:1.0: port 2: status 0507 change 0000 >> [ 16.108612] ehci_hub_control GetPortStatus, port 3 temp = 0x1000 >> [ 16.108642] hub 1-0:1.0: hub_activate submitting urb >> [ 16.109222] ehci_irq port 3 pstatus 0x1000 >> [ 16.109222] ehci_irq port 2 pstatus 0x14c5 >> [ 16.109252] ehci_irq port 1 pstatus 0x1000 >> [ 16.109374] hub 1-0:1.0: state 7 ports 3 chg 0000 evt 0000 > > But apparently nothing happened. Why not? Did the rh_timer get reset? > Maybe you can find out what went wrong. > > (Hmmm, we seem to be missing a > > set_bit(wIndex, &ehci->resuming_ports); > > line in there...) > Right. This is indeed the problem. I've cooked up a patch for that and will send it separately in a moment. cheers, -roger
diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c index 16d7150..c687610 100644 --- a/drivers/usb/host/ehci-omap.c +++ b/drivers/usb/host/ehci-omap.c @@ -69,6 +69,7 @@ static const char hcd_name[] = "ehci-omap"; struct omap_hcd { struct usb_phy *phy[OMAP3_HS_USB_PORTS]; /* one PHY for each port */ int nports; + bool initialized; }; static inline void ehci_write(void __iomem *base, u32 reg, u32 val) @@ -159,6 +160,7 @@ static int ehci_hcd_omap_probe(struct platform_device *pdev) hcd->rsrc_start = res->start; hcd->rsrc_len = resource_size(res); hcd->regs = regs; + hcd->has_wakeup_irq = true; hcd_to_ehci(hcd)->caps = regs; omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; @@ -210,6 +212,8 @@ static int ehci_hcd_omap_probe(struct platform_device *pdev) goto err_pm_runtime; } + omap->initialized = true; + /* * Bring PHYs out of reset. * Even though HSIC mode is a PHY-less mode, the reset @@ -225,6 +229,8 @@ static int ehci_hcd_omap_probe(struct platform_device *pdev) usb_phy_set_suspend(omap->phy[i], 0); } + pm_runtime_put_sync(dev); + return 0; err_pm_runtime: @@ -257,6 +263,7 @@ static int ehci_hcd_omap_remove(struct platform_device *pdev) struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; int i; + pm_runtime_get_sync(dev); usb_remove_hcd(hcd); for (i = 0; i < omap->nports; i++) { @@ -286,15 +293,70 @@ static const struct of_device_id omap_ehci_dt_ids[] = { MODULE_DEVICE_TABLE(of, omap_ehci_dt_ids); +static int omap_ehci_suspend(struct device *dev) +{ + struct usb_hcd *hcd = dev_get_drvdata(dev); + bool do_wakeup = device_may_wakeup(dev); + + dev_dbg(dev, "%s: do_wakeup: %d\n", __func__, do_wakeup); + + return ehci_suspend(hcd, do_wakeup); +} + +static int omap_ehci_resume(struct device *dev) +{ + struct usb_hcd *hcd = dev_get_drvdata(dev); + + dev_dbg(dev, "%s\n", __func__); + + return ehci_resume(hcd, false); +} + +static int omap_ehci_runtime_suspend(struct device *dev) +{ + struct usb_hcd *hcd = dev_get_drvdata(dev); + struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; + bool do_wakeup = device_may_wakeup(dev); + + dev_dbg(dev, "%s\n", __func__); + + if (omap->initialized) + ehci_suspend(hcd, do_wakeup); + + return 0; +} + +static int omap_ehci_runtime_resume(struct device *dev) +{ + struct usb_hcd *hcd = dev_get_drvdata(dev); + struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; + + dev_dbg(dev, "%s\n", __func__); + + if (omap->initialized) { + ehci_resume(hcd, false); + mdelay(10); + } + + return 0; +} + + +static const struct dev_pm_ops omap_ehci_pm_ops = { + .suspend = omap_ehci_suspend, + .resume = omap_ehci_resume, + .runtime_suspend = omap_ehci_runtime_suspend, + .runtime_resume = omap_ehci_runtime_resume, +}; + static struct platform_driver ehci_hcd_omap_driver = { .probe = ehci_hcd_omap_probe, .remove = ehci_hcd_omap_remove, .shutdown = ehci_hcd_omap_shutdown, - /*.suspend = ehci_hcd_omap_suspend, */ - /*.resume = ehci_hcd_omap_resume, */ .driver = { .name = hcd_name, .of_match_table = of_match_ptr(omap_ehci_dt_ids), + .pm = &omap_ehci_pm_ops, } };