Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
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 [ 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 evt 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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
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 :-) -- balbi signature.asc Description: Digital signature
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
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 ;-) -- balbi signature.asc Description: Digital signature
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
On Wed, 3 Jul 2013, Felipe Balbi wrote: 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 ;-) This idea probably won't lead anywhere. To the best of my knowledge (and I'm not an expert on PCI), these different power wells are simply part of the PCI bus. The bus controller knows to turn off one of them when the device (or maybe when all the devices on a bus segment) goes into D3. One big difference with respect to OMAP is the way PCI handles wakeup messages. There's a separate bus signal, PME# (which stands for Power Management Event), that a device asserts when it wants to send a wakeup request. When the ACPI driver sees the PME# signal, it searches through the entire PCI tree to find the device or devices which need to be resumed. Or maybe when ACPI sees the signal, it tells the PCI core to do this -- I'm not sure of the details. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
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 [ 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 evt 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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
On 06/28/2013 10:18 PM, Alan Stern wrote: On Fri, 28 Jun 2013, Roger Quadros wrote: Just found the problem. It seems that enabling the ehci_irq _after_ the root hub is resumed is the root cause of the problem. Doing so will miss events from the root hub. This sounds like a bug in the IRQ setup. It's the sort of thing you see when a level-triggered IRQ is treated as though it were edge-triggered. In any case, the wakeup should have worked whether the IRQ was issued or not. OK. I appreciate the symmetry of putting the enable_irq call in ehci-hcd, seeing as how the disable_irq is there too. On the other hand, every HCD using this mechanism is going to have to do the same thing, which argues for putting the enable call in the core. Perhaps at the start OK. of hcd_resume_work() instead of the end. We can't enable_irq at the start as the controller will only be resumed after usb_remote_wakeup(). cheers, -roger -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
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. -- balbi signature.asc Description: Digital signature
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
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 [ 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 evt 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 [ 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 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 [ 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 evt [ 19.991760] hub 1-2:1.0: state 7 ports 5 chg 0010 evt [ 19.997741] hub 1-2:1.0: port 4, status 0101, change , 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
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
On 06/28/2013 03:20 PM, Roger Quadros wrote:
On 06/27/2013 06:40 PM, Alan Stern wrote:
On Wed, 26 Jun 2013, Roger Quadros wrote:
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
[ 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 evt
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
[ 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 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
[ 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 evt
[ 19.991760] hub 1-2:1.0: state 7 ports 5 chg 0010 evt
[ 19.997741] hub 1-2:1.0: port 4, status 0101, change , 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.
Just found the problem. It seems that enabling the ehci_irq _after_ the root
hub is resumed
is the root cause of the problem. Doing so will miss events from the root hub.
The following modification worked for me without any delays.
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 72df4eb..b3af1aa 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2136,11 +2136,6 @@ static void hcd_resume_work(struct work_struct *work)
usb_lock_device(udev);
usb_remote_wakeup(udev);
usb_unlock_device(udev);
- if (HCD_IRQ_DISABLED(hcd)) {
- /* Interrupt was disabled */
- clear_bit(HCD_FLAG_IRQ_DISABLED, hcd-flags);
- enable_irq(hcd-irq);
- }
}
/**
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 593d28d..f2a1a46 100644
--- a/drivers/usb/host/ehci-hcd.c
+++
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
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 [ 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 evt 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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
On Fri, 28 Jun 2013, Roger Quadros wrote:
Just found the problem. It seems that enabling the ehci_irq _after_ the root
hub is resumed
is the root cause of the problem. Doing so will miss events from the root hub.
This sounds like a bug in the IRQ setup. It's the sort of thing you
see when a level-triggered IRQ is treated as though it were
edge-triggered.
In any case, the wakeup should have worked whether the IRQ was issued
or not.
The following modification worked for me without any delays.
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 72df4eb..b3af1aa 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2136,11 +2136,6 @@ static void hcd_resume_work(struct work_struct *work)
usb_lock_device(udev);
usb_remote_wakeup(udev);
usb_unlock_device(udev);
- if (HCD_IRQ_DISABLED(hcd)) {
- /* Interrupt was disabled */
- clear_bit(HCD_FLAG_IRQ_DISABLED, hcd-flags);
- enable_irq(hcd-irq);
- }
}
/**
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 593d28d..f2a1a46 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1110,6 +1110,12 @@ int ehci_resume(struct usb_hcd *hcd, bool hibernated)
{
struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+ if (HCD_IRQ_DISABLED(hcd)) {
+ /* Interrupt was disabled */
+ clear_bit(HCD_FLAG_IRQ_DISABLED, hcd-flags);
+ enable_irq(hcd-irq);
+ }
+
if (time_before(jiffies, ehci-next_statechange))
msleep(100);
I appreciate the symmetry of putting the enable_irq call in ehci-hcd,
seeing as how the disable_irq is there too. On the other hand, every
HCD using this mechanism is going to have to do the same thing, which
argues for putting the enable call in the core. Perhaps at the start
of hcd_resume_work() instead of the end.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
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
[8.714630] hub 1-0:1.0: state 7 ports 3 chg evt
gets stuck here in the failing case
[8.723541] hub 1-0:1.0: state 7 ports 3 chg 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
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
On 06/25/2013 08:38 PM, Alan Stern wrote:
On Tue, 25 Jun 2013, Roger Quadros wrote:
On 06/24/2013 10:34 PM, Alan Stern wrote:
On Mon, 24 Jun 2013, Roger Quadros wrote:
OK I've tried to handle all this in an alternate way. Now the controller
suspend/resume
and runtime suspend/resume is independent of bus suspend.
The controller now runtime suspends when all devices on the bus have
suspended and
the hub auto suspends. NOTE: HW_ACCESSIBLE is still set on runtime_suspend.
The challenge here is to process the interrupt in this state.
The situation is a little peculiar. Does the hardware really use the
same IRQ for reporting wakeup events when the controller is suspended
and for reporting normal I/O events?
No and yes :). Actually the Pad wakeup comes as a separate IRQ from hardware.
The omap pinctrl driver captures that, determines which pad caused the
wakeup and
routes it to the appropriate interrupt based on the mapping provided in the
device tree.
In the ehci-omap case we provide the EHCI IRQ number in the mapping for the
USB host pads.
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.
if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd)))
rc = IRQ_NONE;
+ else if (pm_runtime_status_suspended(hcd-self.controller)) {
+ /*
+ * We can't handle it yet so disable IRQ, make note of it
+ * and resume root hub (i.e. controller as well)
+ */
+ disable_irq_nosync(hcd-irq);
+ set_bit(HCD_FLAG_IRQ_DISABLED, hcd-flags);
+ usb_hcd_resume_root_hub(hcd);
+ rc = IRQ_HANDLED;
+ }
This part will have to be different.
Certainly if HCD_DEAD(hcd) then we want to return IRQ_NONE. Likewise
if (!HCD_HW_ACCESSIBLE(hcd) !hcd-has_wakeup_interrupts). In all
other cases we have to call the HCD's interrupt handler.
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?
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.
ff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index d53547d..24e21a2 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2136,6 +2136,11 @@ static void hcd_resume_work(struct work_struct *work)
usb_lock_device(udev);
usb_remote_wakeup(udev);
usb_unlock_device(udev);
+if (HCD_IRQ_DISABLED(hcd)) {
+/* Interrupt was disabled */
+clear_bit(HCD_FLAG_IRQ_DISABLED, hcd-flags);
+enable_irq(hcd-irq);
+}
}
/**
@@ -2223,7 +2228,9 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd)
*/
local_irq_save(flags);
-if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd)))
+if (unlikely(HCD_DEAD(hcd)))
+rc = IRQ_NONE;
+else if (!HCD_HW_ACCESSIBLE(hcd) !hcd-has_wakeup_irq)
rc = IRQ_NONE;
Add an unlikely() here too.
OK.
else if (hcd-driver-irq(hcd) == IRQ_NONE)
rc = IRQ_NONE;
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 246e124..1844e31 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -689,6 +689,21 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)
spin_lock (ehci-lock);
+if (unlikely(!HCD_HW_ACCESSIBLE(hcd))) {
+/*
+ * We got a wakeup interrupt while the controller was
+ * suspending or suspended. We can't handle it now, so
+ * disable the IRQ and resume the root hub (and hence
+ * the controller too).
+ */
+disable_irq_nosync(hcd-irq);
+set_bit(HCD_FLAG_IRQ_DISABLED, hcd-flags);
To be safe, let's put these two statements inside
if (hcd-has_wakeup_irq) { ... }
I think if the right sort of race occurs, we could end up here even
when hcd-has_wakeup_irq is clear. So this test is needed. We don't
want to disable a
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
On 06/24/2013 10:34 PM, Alan Stern wrote:
On Mon, 24 Jun 2013, Roger Quadros wrote:
OK I've tried to handle all this in an alternate way. Now the controller
suspend/resume
and runtime suspend/resume is independent of bus suspend.
The controller now runtime suspends when all devices on the bus have
suspended and
the hub auto suspends. NOTE: HW_ACCESSIBLE is still set on runtime_suspend.
The challenge here is to process the interrupt in this state.
The situation is a little peculiar. Does the hardware really use the
same IRQ for reporting wakeup events when the controller is suspended
and for reporting normal I/O events?
No and yes :). Actually the Pad wakeup comes as a separate IRQ from hardware.
The omap pinctrl driver captures that, determines which pad caused the wakeup
and
routes it to the appropriate interrupt based on the mapping provided in the
device tree.
In the ehci-omap case we provide the EHCI IRQ number in the mapping for the USB
host pads.
In principle, HW_ACCESSIBLE should not be set when the controller is
suspended, because you can't access the hardware then (since the clocks
are off, right?). But I can see how this would cause a problem if it
leads to wakeup interrupts being ignored.
Also, note that one of the things ehci_suspend() does is turn off the
Interrupt-Enable register, which means the wakeup interrupt would never
be issued in the first place. I guess ehci_hcd_omap_suspend() will
have to turn on the proper enable bit before stopping the clocks.
For us, in runtime_suspend, we do not call ehci_suspend(), we just cut the
clocks.
So HW_ACCESSIBLE is still set, which is kind of wrong, I agree. I'll update
this in
the next patch.
I've tried to handle this state. (i.e. interrupt while controller is runtime
suspended),
by disabling the interrupt till we are ready and calling
usb_hcd_resume_root_hub().
We mark a flag (HW_IRQ_DISABLED) stating that the interrupt was disabled and
based on
that enable the IRQ and clear the flag in hcd_resume_work().
Do let me know what you think of this approach.
This is a very tricky problem. Right now, usbcore assumes that when
HW_ACCESSIBLE is clear, the hardware can't generate interrupt requests
and therefore any interrupt must come from some other device sharing
the same IRQ line. For the systems you're working on, this is wrong in
both respects (the hardware _can_ generate interrupt requests and IRQ
lines aren't shared).
Right.
I think we will have to add a new flag to describe your situation.
Let's call it hcd-has_wakeup_interrupts. Presumably there will never
be a system that uses interrupts for wakeup signals _and_ has shared
IRQ lines? That would be a bad combination...
Sounds good.
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index d53547d..8879cd2 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2136,6 +2136,11 @@ static void hcd_resume_work(struct work_struct *work)
usb_lock_device(udev);
usb_remote_wakeup(udev);
usb_unlock_device(udev);
+if (HCD_IRQ_DISABLED(hcd)) {
+/* Interrupt was disabled */
+clear_bit(HCD_FLAG_IRQ_DISABLED, hcd-flags);
+enable_irq(hcd-irq);
+}
}
This part is okay.
@@ -2225,6 +2230,16 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd)
if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd)))
rc = IRQ_NONE;
+else if (pm_runtime_status_suspended(hcd-self.controller)) {
+/*
+ * We can't handle it yet so disable IRQ, make note of it
+ * and resume root hub (i.e. controller as well)
+ */
+disable_irq_nosync(hcd-irq);
+set_bit(HCD_FLAG_IRQ_DISABLED, hcd-flags);
+usb_hcd_resume_root_hub(hcd);
+rc = IRQ_HANDLED;
+}
This part will have to be different.
Certainly if HCD_DEAD(hcd) then we want to return IRQ_NONE. Likewise
if (!HCD_HW_ACCESSIBLE(hcd) !hcd-has_wakeup_interrupts). In all
other cases we have to call the HCD's interrupt handler.
The rest of the work will have to be done in the HCD, while holding the
private lock. In ehci_irq(), after the spin_lock() call, you'll have
to add something like this:
if (unlikely(!HCD_HW_ACCESSIBLE(hcd))) {
/*
* We got a wakeup interrupt while the controller was
* suspending or suspended. We can't handle it now, so
* disable the IRQ and resume the root hub (and hence
* the controller too).
*/
disable_irq_nosync(hcd-irq);
set_bit(HCD_FLAG_IRQ_DISABLED, hcd-flags);
spin_unlock(ehci-lock);
usb_hcd_resume_root_hub(hcd);
return IRQ_HANDLED;
}
I think this will work. How does it look to you?
Looks good to me. Below is the implementation on these lines.
I've
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
On Tue, 25 Jun 2013, Roger Quadros wrote:
On 06/24/2013 10:34 PM, Alan Stern wrote:
On Mon, 24 Jun 2013, Roger Quadros wrote:
OK I've tried to handle all this in an alternate way. Now the controller
suspend/resume
and runtime suspend/resume is independent of bus suspend.
The controller now runtime suspends when all devices on the bus have
suspended and
the hub auto suspends. NOTE: HW_ACCESSIBLE is still set on runtime_suspend.
The challenge here is to process the interrupt in this state.
The situation is a little peculiar. Does the hardware really use the
same IRQ for reporting wakeup events when the controller is suspended
and for reporting normal I/O events?
No and yes :). Actually the Pad wakeup comes as a separate IRQ from hardware.
The omap pinctrl driver captures that, determines which pad caused the wakeup
and
routes it to the appropriate interrupt based on the mapping provided in the
device tree.
In the ehci-omap case we provide the EHCI IRQ number in the mapping for the
USB host pads.
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.
if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd)))
rc = IRQ_NONE;
+ else if (pm_runtime_status_suspended(hcd-self.controller)) {
+ /*
+ * We can't handle it yet so disable IRQ, make note of it
+ * and resume root hub (i.e. controller as well)
+ */
+ disable_irq_nosync(hcd-irq);
+ set_bit(HCD_FLAG_IRQ_DISABLED, hcd-flags);
+ usb_hcd_resume_root_hub(hcd);
+ rc = IRQ_HANDLED;
+ }
This part will have to be different.
Certainly if HCD_DEAD(hcd) then we want to return IRQ_NONE. Likewise
if (!HCD_HW_ACCESSIBLE(hcd) !hcd-has_wakeup_interrupts). In all
other cases we have to call the HCD's interrupt handler.
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.
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?
Looks good to me. Below is the implementation on these lines.
I've added a flag has_wakeup_irq:1 in struct hcd to indicate the special
case where the interrupt can come even if controller is suspended.
I've modified the ehci-omap driver to call ehci_suspend/resume() on system
suspend/resume. For runtime suspend/resume I just toggle the HCD_HW_ACCESSIBLE
flag to indicate that controller cannot be accessed when runtime suspended.
You're going to change that, right? ehci_suspend/resume should be
called whenever the controller's power state changes, regardless of
whether it is for runtime PM or system PM.
The cutting of clocks is done in the parent driver (i.e.
drivers/mfd/omap-usb-host.c)
Patch is below. If it looks OK. I'll re-post the entire series. Thanks.
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index f5f5c7d..51c2d59 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -110,6 +110,7 @@ struct usb_hcd {
#define HCD_FLAG_WAKEUP_PENDING 4 /* root hub is
resuming? */
#define HCD_FLAG_RH_RUNNING 5 /* root hub is running? */
#define HCD_FLAG_DEAD6 /* controller has died?
*/
+#define HCD_FLAG_IRQ_DISABLED7 /* Interrupt was
disabled */
/* The flags can be tested using these macros; they are likely to
* be slightly faster than test_bit().
@@ -120,6 +121,7 @@ struct usb_hcd {
#define HCD_WAKEUP_PENDING(hcd) ((hcd)-flags (1U
HCD_FLAG_WAKEUP_PENDING))
#define HCD_RH_RUNNING(hcd) ((hcd)-flags (1U HCD_FLAG_RH_RUNNING))
#define HCD_DEAD(hcd)((hcd)-flags (1U HCD_FLAG_DEAD))
+#define HCD_IRQ_DISABLED(hcd)((hcd)-flags (1U
HCD_FLAG_IRQ_DISABLED))
/* Flags that get set only during HCD registration or removal. */
unsignedrh_registered:1;/* is root hub registered? */
@@ -132,6 +134,7 @@ struct usb_hcd {
unsignedwireless:1; /* Wireless USB HCD */
unsignedauthorized_default:1;
unsignedhas_tt:1; /* Integrated TT in root hub */
+ unsignedhas_wakeup_irq:1; /* Can generate IRQ when
suspended */
unsigned intirq;/* irq allocated */
void __iomem*regs; /* device memory/io */
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index d53547d..24e21a2
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
Hi Alan,
On 06/20/2013 08:33 PM, Alan Stern wrote:
On Thu, 20 Jun 2013, Roger Quadros wrote:
runtime_resume(dev)
{
...
if (omap-flags OMAP_EHCI_IRQ_PENDING) {
process_pending_irqs(omap);
OK, thanks.
But I'm not sure if the generic ehci_irq handler is able to
run in a process context. Maybe if we replace spin_lock(ehci-lock);
with spin_lock_irqsave() there, it will work.
Alan is this a doable option?
ehci_irq() will work okay in process context, provided the caller
disables interrupts.
But maybe none of this will be needed after Roger redesigns the
controller suspend to work at the right time. Or if it is, we could
adopt a simpler alternative: the controller's resume routine could
always call usb_hcd_resume_root_hub(). After all, about the only
reason for doing a runtime resume of the controller is because the root
hub needs to do something.
OK I've tried to handle all this in an alternate way. Now the controller
suspend/resume
and runtime suspend/resume is independent of bus suspend.
The controller now runtime suspends when all devices on the bus have suspended
and
the hub auto suspends. NOTE: HW_ACCESSIBLE is still set on runtime_suspend.
The challenge here is to process the interrupt in this state.
I've tried to handle this state. (i.e. interrupt while controller is runtime
suspended),
by disabling the interrupt till we are ready and calling
usb_hcd_resume_root_hub().
We mark a flag (HW_IRQ_DISABLED) stating that the interrupt was disabled and
based on
that enable the IRQ and clear the flag in hcd_resume_work().
Do let me know what you think of this approach.
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index d53547d..8879cd2 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2136,6 +2136,11 @@ static void hcd_resume_work(struct work_struct *work)
usb_lock_device(udev);
usb_remote_wakeup(udev);
usb_unlock_device(udev);
+ if (HCD_IRQ_DISABLED(hcd)) {
+ /* Interrupt was disabled */
+ clear_bit(HCD_FLAG_IRQ_DISABLED, hcd-flags);
+ enable_irq(hcd-irq);
+ }
}
/**
@@ -2225,6 +2230,16 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd)
if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd)))
rc = IRQ_NONE;
+ else if (pm_runtime_status_suspended(hcd-self.controller)) {
+ /*
+* We can't handle it yet so disable IRQ, make note of it
+* and resume root hub (i.e. controller as well)
+*/
+ disable_irq_nosync(hcd-irq);
+ set_bit(HCD_FLAG_IRQ_DISABLED, hcd-flags);
+ usb_hcd_resume_root_hub(hcd);
+ rc = IRQ_HANDLED;
+ }
else if (hcd-driver-irq(hcd) == IRQ_NONE)
rc = IRQ_NONE;
else
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index f5f5c7d..6c6287e 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -110,6 +110,7 @@ struct usb_hcd {
#define HCD_FLAG_WAKEUP_PENDING4 /* root hub is
resuming? */
#define HCD_FLAG_RH_RUNNING5 /* root hub is running? */
#define HCD_FLAG_DEAD 6 /* controller has died? */
+#define HCD_FLAG_IRQ_DISABLED 7 /* Interrupt was disabled */
/* The flags can be tested using these macros; they are likely to
* be slightly faster than test_bit().
@@ -120,6 +121,7 @@ struct usb_hcd {
#define HCD_WAKEUP_PENDING(hcd)((hcd)-flags (1U
HCD_FLAG_WAKEUP_PENDING))
#define HCD_RH_RUNNING(hcd)((hcd)-flags (1U HCD_FLAG_RH_RUNNING))
#define HCD_DEAD(hcd) ((hcd)-flags (1U HCD_FLAG_DEAD))
+#define HCD_IRQ_DISABLED(hcd) ((hcd)-flags (1U HCD_FLAG_IRQ_DISABLED))
/* Flags that get set only during HCD registration or removal. */
unsignedrh_registered:1;/* is root hub registered? */
diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c
index 16d7150..c5320c7 100644
--- a/drivers/usb/host/ehci-omap.c
+++ b/drivers/usb/host/ehci-omap.c
@@ -225,6 +225,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:
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
On Mon, 24 Jun 2013, Roger Quadros wrote:
OK I've tried to handle all this in an alternate way. Now the controller
suspend/resume
and runtime suspend/resume is independent of bus suspend.
The controller now runtime suspends when all devices on the bus have
suspended and
the hub auto suspends. NOTE: HW_ACCESSIBLE is still set on runtime_suspend.
The challenge here is to process the interrupt in this state.
The situation is a little peculiar. Does the hardware really use the
same IRQ for reporting wakeup events when the controller is suspended
and for reporting normal I/O events?
In principle, HW_ACCESSIBLE should not be set when the controller is
suspended, because you can't access the hardware then (since the clocks
are off, right?). But I can see how this would cause a problem if it
leads to wakeup interrupts being ignored.
Also, note that one of the things ehci_suspend() does is turn off the
Interrupt-Enable register, which means the wakeup interrupt would never
be issued in the first place. I guess ehci_hcd_omap_suspend() will
have to turn on the proper enable bit before stopping the clocks.
I've tried to handle this state. (i.e. interrupt while controller is runtime
suspended),
by disabling the interrupt till we are ready and calling
usb_hcd_resume_root_hub().
We mark a flag (HW_IRQ_DISABLED) stating that the interrupt was disabled and
based on
that enable the IRQ and clear the flag in hcd_resume_work().
Do let me know what you think of this approach.
This is a very tricky problem. Right now, usbcore assumes that when
HW_ACCESSIBLE is clear, the hardware can't generate interrupt requests
and therefore any interrupt must come from some other device sharing
the same IRQ line. For the systems you're working on, this is wrong in
both respects (the hardware _can_ generate interrupt requests and IRQ
lines aren't shared).
I think we will have to add a new flag to describe your situation.
Let's call it hcd-has_wakeup_interrupts. Presumably there will never
be a system that uses interrupts for wakeup signals _and_ has shared
IRQ lines? That would be a bad combination...
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index d53547d..8879cd2 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2136,6 +2136,11 @@ static void hcd_resume_work(struct work_struct *work)
usb_lock_device(udev);
usb_remote_wakeup(udev);
usb_unlock_device(udev);
+ if (HCD_IRQ_DISABLED(hcd)) {
+ /* Interrupt was disabled */
+ clear_bit(HCD_FLAG_IRQ_DISABLED, hcd-flags);
+ enable_irq(hcd-irq);
+ }
}
This part is okay.
@@ -2225,6 +2230,16 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd)
if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd)))
rc = IRQ_NONE;
+ else if (pm_runtime_status_suspended(hcd-self.controller)) {
+ /*
+ * We can't handle it yet so disable IRQ, make note of it
+ * and resume root hub (i.e. controller as well)
+ */
+ disable_irq_nosync(hcd-irq);
+ set_bit(HCD_FLAG_IRQ_DISABLED, hcd-flags);
+ usb_hcd_resume_root_hub(hcd);
+ rc = IRQ_HANDLED;
+ }
This part will have to be different.
Certainly if HCD_DEAD(hcd) then we want to return IRQ_NONE. Likewise
if (!HCD_HW_ACCESSIBLE(hcd) !hcd-has_wakeup_interrupts). In all
other cases we have to call the HCD's interrupt handler.
The rest of the work will have to be done in the HCD, while holding the
private lock. In ehci_irq(), after the spin_lock() call, you'll have
to add something like this:
if (unlikely(!HCD_HW_ACCESSIBLE(hcd))) {
/*
* We got a wakeup interrupt while the controller was
* suspending or suspended. We can't handle it now, so
* disable the IRQ and resume the root hub (and hence
* the controller too).
*/
disable_irq_nosync(hcd-irq);
set_bit(HCD_FLAG_IRQ_DISABLED, hcd-flags);
spin_unlock(ehci-lock);
usb_hcd_resume_root_hub(hcd);
return IRQ_HANDLED;
}
I think this will work. How does it look to you?
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
Hi,
On Wed, Jun 19, 2013 at 05:05:51PM +0300, Roger Quadros wrote:
Runtime suspend the controller during bus suspend and resume it
during bus resume. This will ensure that the USB Host power domain
enters lower power state and does not prevent the SoC from
endering deeper sleep states.
Remote wakeup will come up as an interrupt while the controller
is suspended, so tackle it carefully using a workqueue.
Signed-off-by: Roger Quadros rog...@ti.com
---
drivers/usb/host/ehci-omap.c | 82
++
1 files changed, 82 insertions(+), 0 deletions(-)
diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c
index 16d7150..91f14f1 100644
--- a/drivers/usb/host/ehci-omap.c
+++ b/drivers/usb/host/ehci-omap.c
@@ -44,6 +44,8 @@
#include linux/usb/hcd.h
#include linux/of.h
#include linux/dma-mapping.h
+#include linux/workqueue.h
+#include linux/spinlock.h
#include ehci.h
@@ -69,6 +71,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;
+ struct work_struct work;
};
static inline void ehci_write(void __iomem *base, u32 reg, u32 val)
@@ -81,6 +84,76 @@ static inline u32 ehci_read(void __iomem *base, u32 reg)
return __raw_readl(base + reg);
}
+static void omap_ehci_work(struct work_struct *work)
+{
+ struct omap_hcd *omap = container_of(work, struct omap_hcd, work);
+ struct ehci_hcd *ehci = container_of((void *) omap,
+ struct ehci_hcd, priv);
+ struct usb_hcd *hcd = ehci_to_hcd(ehci);
+ struct device *dev = hcd-self.controller;
+
+ pm_runtime_get_sync(dev);
+ enable_irq(hcd-irq);
+ /*
+ * enable_irq() should preempt us with a pending IRQ
+ * so we can be sure that IRQ handler completes before
+ * we call pm_runtime_put_sync()
+ */
+ pm_runtime_put_sync(dev);
+}
+
+static irqreturn_t omap_ehci_irq(struct usb_hcd *hcd)
+{
+ struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)-priv;
+ struct device *dev = hcd-self.controller;
+ irqreturn_t ret;
+
+ if (pm_runtime_suspended(dev)) {
+ schedule_work(omap-work);
+ disable_irq_nosync(hcd-irq);
+ ret = IRQ_HANDLED;
looks like this could be done as:
if (pm_runtime_suspended(dev)) {
pm_runtime_get(dev);
omap-flags |= OMAP_EHCI_IRQ_PENDING;
disable_irq_nosync(hcd-irq);
ret = IRQ_HANDLED;
}
then on your -runtime_resume():
runtime_resume(dev)
{
...
if (omap-flags OMAP_EHCI_IRQ_PENDING) {
process_pending_irqs(omap);
omap-flags = ~OMAP_EHCI_IRQ_PENDING;
}
return 0;
}
or something similar
--
balbi
signature.asc
Description: Digital signature
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
On 06/19/2013 08:39 PM, Kevin Hilman wrote: Hi Roger, Roger Quadros rog...@ti.com writes: Runtime suspend the controller during bus suspend and resume it during bus resume. This will ensure that the USB Host power domain enters lower power state and does not prevent the SoC from endering deeper sleep states. Remote wakeup will come up as an interrupt while the controller is suspended, so tackle it carefully using a workqueue. I don't think you need a special workqueue here. The workqueue of the PM core (pm_wq) will be used if you just use an asynchronous 'get' in the ISR. Then, the driver's own runtime PM callbacks can be used instead of an additional workqueue. Another thing to worry about here when using runtime PM to implement suspend/resume is that runtime PM can be disabled from userspace (e.g. echo disabled /sys/devices/.../power/control.) If that's the case, all the pm_runtime_suspended() checks will always fail becuase that call also checks if runtime PM is disabled. You'll likely want to use the pm_runtime_status_suspended() check instead, which checks only the status, and doesn't matter if runtime PM is currently disabled. Because of the corner issues here, please test system suspend/resume when runtime PM has been disabled. Good points. Thanks. I'll need to think about it some more. cheers, -roger -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
On 06/20/2013 03:11 PM, Felipe Balbi wrote:
Hi,
On Wed, Jun 19, 2013 at 05:05:51PM +0300, Roger Quadros wrote:
Runtime suspend the controller during bus suspend and resume it
during bus resume. This will ensure that the USB Host power domain
enters lower power state and does not prevent the SoC from
endering deeper sleep states.
Remote wakeup will come up as an interrupt while the controller
is suspended, so tackle it carefully using a workqueue.
Signed-off-by: Roger Quadros rog...@ti.com
---
drivers/usb/host/ehci-omap.c | 82
++
1 files changed, 82 insertions(+), 0 deletions(-)
diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c
index 16d7150..91f14f1 100644
--- a/drivers/usb/host/ehci-omap.c
+++ b/drivers/usb/host/ehci-omap.c
@@ -44,6 +44,8 @@
#include linux/usb/hcd.h
#include linux/of.h
#include linux/dma-mapping.h
+#include linux/workqueue.h
+#include linux/spinlock.h
#include ehci.h
@@ -69,6 +71,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;
+struct work_struct work;
};
static inline void ehci_write(void __iomem *base, u32 reg, u32 val)
@@ -81,6 +84,76 @@ static inline u32 ehci_read(void __iomem *base, u32 reg)
return __raw_readl(base + reg);
}
+static void omap_ehci_work(struct work_struct *work)
+{
+struct omap_hcd *omap = container_of(work, struct omap_hcd, work);
+struct ehci_hcd *ehci = container_of((void *) omap,
+struct ehci_hcd, priv);
+struct usb_hcd *hcd = ehci_to_hcd(ehci);
+struct device *dev = hcd-self.controller;
+
+pm_runtime_get_sync(dev);
+enable_irq(hcd-irq);
+/*
+ * enable_irq() should preempt us with a pending IRQ
+ * so we can be sure that IRQ handler completes before
+ * we call pm_runtime_put_sync()
+ */
+pm_runtime_put_sync(dev);
+}
+
+static irqreturn_t omap_ehci_irq(struct usb_hcd *hcd)
+{
+struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)-priv;
+struct device *dev = hcd-self.controller;
+irqreturn_t ret;
+
+if (pm_runtime_suspended(dev)) {
+schedule_work(omap-work);
+disable_irq_nosync(hcd-irq);
+ret = IRQ_HANDLED;
looks like this could be done as:
if (pm_runtime_suspended(dev)) {
pm_runtime_get(dev);
omap-flags |= OMAP_EHCI_IRQ_PENDING;
disable_irq_nosync(hcd-irq);
ret = IRQ_HANDLED;
}
then on your -runtime_resume():
runtime_resume(dev)
{
...
if (omap-flags OMAP_EHCI_IRQ_PENDING) {
process_pending_irqs(omap);
OK, thanks.
But I'm not sure if the generic ehci_irq handler is able to
run in a process context. Maybe if we replace spin_lock(ehci-lock);
with spin_lock_irqsave() there, it will work.
Alan is this a doable option?
omap-flags = ~OMAP_EHCI_IRQ_PENDING;
}
return 0;
}
or something similar
cheers,
-roger
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
On Thu, 20 Jun 2013, Roger Quadros wrote:
runtime_resume(dev)
{
...
if (omap-flags OMAP_EHCI_IRQ_PENDING) {
process_pending_irqs(omap);
OK, thanks.
But I'm not sure if the generic ehci_irq handler is able to
run in a process context. Maybe if we replace spin_lock(ehci-lock);
with spin_lock_irqsave() there, it will work.
Alan is this a doable option?
ehci_irq() will work okay in process context, provided the caller
disables interrupts.
But maybe none of this will be needed after Roger redesigns the
controller suspend to work at the right time. Or if it is, we could
adopt a simpler alternative: the controller's resume routine could
always call usb_hcd_resume_root_hub(). After all, about the only
reason for doing a runtime resume of the controller is because the root
hub needs to do something.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
Hi Roger, Roger Quadros rog...@ti.com writes: Runtime suspend the controller during bus suspend and resume it during bus resume. This will ensure that the USB Host power domain enters lower power state and does not prevent the SoC from endering deeper sleep states. Remote wakeup will come up as an interrupt while the controller is suspended, so tackle it carefully using a workqueue. I don't think you need a special workqueue here. The workqueue of the PM core (pm_wq) will be used if you just use an asynchronous 'get' in the ISR. Then, the driver's own runtime PM callbacks can be used instead of an additional workqueue. Another thing to worry about here when using runtime PM to implement suspend/resume is that runtime PM can be disabled from userspace (e.g. echo disabled /sys/devices/.../power/control.) If that's the case, all the pm_runtime_suspended() checks will always fail becuase that call also checks if runtime PM is disabled. You'll likely want to use the pm_runtime_status_suspended() check instead, which checks only the status, and doesn't matter if runtime PM is currently disabled. Because of the corner issues here, please test system suspend/resume when runtime PM has been disabled. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
