| Message ID | 1399541587-14067-6-git-send-email-george.cherian@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Thu, May 08, 2014 at 03:03:07PM +0530, George Cherian wrote: > Enabling the core interrupts in complete is too late for XHCI, and stops > xhci from proper operation. So remove prepare and complete and disable/enable isn't this a bug in xhci ? I mean the driver should make no assumption as to when IRQs are enabled, why do we need to enable IRQs earlier when the device is only considered "ready for use" after ->complete() finishes executing ? From documentation we have: 107 * @complete: Undo the changes made by @prepare(). This method is executed for 108 * all kinds of resume transitions, following one of the resume callbacks: 109 * @resume(), @thaw(), @restore(). Also called if the state transition 110 * fails before the driver's suspend callback: @suspend(), @freeze() or 111 * @poweroff(), can be executed (e.g. if the suspend callback fails for one 112 * of the other devices that the PM core has unsuccessfully attempted to 113 * suspend earlier). 114 * The PM core executes subsystem-level @complete() after it has executed 115 * the appropriate resume callbacks for all devices. which tells me that using ->complete() to reenable IRQs is ok here. Specially when you consider that the role of ->prepare() is to prevent new children from being created and, for a USB host, that means we should prevent hub port changes. cheers
On 5/13/2014 9:20 PM, Felipe Balbi wrote: > Hi, > > On Thu, May 08, 2014 at 03:03:07PM +0530, George Cherian wrote: >> Enabling the core interrupts in complete is too late for XHCI, and stops >> xhci from proper operation. So remove prepare and complete and disable/enable > isn't this a bug in xhci ? I mean the driver should make no assumption > as to when IRQs are enabled, why do we need to enable IRQs earlier when > the device is only considered "ready for use" after ->complete() > finishes executing ? I dont think its a bug in xhci. In case of xhci-pci driver it actually does an hcd->driver->pci_suspend (xhci_suspend) followed by synchronize_irq() and the does a pci_disable_device(). In resume path it calls pci_enable_device() followed by hcd->driver->pci_resume(xhci_resume). In case of dwc3-omap we do have a wrapper register which can still disable the XHCI IRQs even though the xhci driver enables the interrupts internally. Now dwc3-omap wrapper driver should not actually fiddle with the core Interrupt enable/disable except in probe/remove. > From documentation we have: > > 107 * @complete: Undo the changes made by @prepare(). This method is executed for > 108 * all kinds of resume transitions, following one of the resume callbacks: > 109 * @resume(), @thaw(), @restore(). Also called if the state transition > 110 * fails before the driver's suspend callback: @suspend(), @freeze() or > 111 * @poweroff(), can be executed (e.g. if the suspend callback fails for one > 112 * of the other devices that the PM core has unsuccessfully attempted to > 113 * suspend earlier). > 114 * The PM core executes subsystem-level @complete() after it has executed > 115 * the appropriate resume callbacks for all devices. > > which tells me that using ->complete() to reenable IRQs is ok here. > Specially when you consider that the role of ->prepare() is to prevent > new children from being created and, for a USB host, that means we > should prevent hub port changes. Probably the patch should have been to still keep the complete/prepare in place but not disable the core interrupts, rather enable/disable only the wrapper interrupt. > cheers >
diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c index 82b20d8f..0916c4b 100644 --- a/drivers/usb/dwc3/dwc3-omap.c +++ b/drivers/usb/dwc3/dwc3-omap.c @@ -622,27 +622,12 @@ static const struct of_device_id of_dwc3_match[] = { MODULE_DEVICE_TABLE(of, of_dwc3_match); #ifdef CONFIG_PM_SLEEP -static int dwc3_omap_prepare(struct device *dev) -{ - struct dwc3_omap *omap = dev_get_drvdata(dev); - - dwc3_omap_disable_irqs(omap); - - return 0; -} - -static void dwc3_omap_complete(struct device *dev) -{ - struct dwc3_omap *omap = dev_get_drvdata(dev); - - dwc3_omap_enable_irqs(omap); -} - static int dwc3_omap_suspend(struct device *dev) { struct dwc3_omap *omap = dev_get_drvdata(dev); omap->utmi_otg_status = dwc3_omap_read_utmi_status(omap); + dwc3_omap_disable_irqs(omap); return 0; } @@ -652,6 +637,7 @@ static int dwc3_omap_resume(struct device *dev) struct dwc3_omap *omap = dev_get_drvdata(dev); dwc3_omap_write_utmi_status(omap, omap->utmi_otg_status); + dwc3_omap_enable_irqs(omap); pm_runtime_disable(dev); pm_runtime_set_active(dev); @@ -661,8 +647,6 @@ static int dwc3_omap_resume(struct device *dev) } static const struct dev_pm_ops dwc3_omap_dev_pm_ops = { - .prepare = dwc3_omap_prepare, - .complete = dwc3_omap_complete, SET_SYSTEM_SLEEP_PM_OPS(dwc3_omap_suspend, dwc3_omap_resume) };
Enabling the core interrupts in complete is too late for XHCI, and stops xhci from proper operation. So remove prepare and complete and disable/enable interrupts in suspend/resume Signed-off-by: George Cherian <george.cherian@ti.com> --- drivers/usb/dwc3/dwc3-omap.c | 20 ++------------------ 1 file changed, 2 insertions(+), 18 deletions(-)