| Message ID | 1313159931-22121-1-git-send-email-vikram.pandita@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 12 Aug 2011, Vikram Pandita wrote: > From: Vikram Pandita <vikram.pandita@ti.com> > > musb pm_runtime_get_sync call happens in intrrupt context on cable attach case > That can result in re-enabling the interrupts and cause side affects. > > So move the code to a work queue. Instead of creating your own, new work queue, just use the pre-existing PM work queue. In other words, replace pm_runtime_get_sync() with pm_runtime_get(). Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 12, 2011 at 8:50 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Fri, 12 Aug 2011, Vikram Pandita wrote: > >> From: Vikram Pandita <vikram.pandita@ti.com> >> >> musb pm_runtime_get_sync call happens in intrrupt context on cable attach case >> That can result in re-enabling the interrupts and cause side affects. >> >> So move the code to a work queue. > > Instead of creating your own, new work queue, just use the pre-existing > PM work queue. In other words, replace pm_runtime_get_sync() with > pm_runtime_get(). In this case it may not work, because immediately after a pm_runtime_get_sync(), we are going to access MUSB registers and xxx_sync() call ensures we will not abort. > > Alan Stern > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 12, 2011 at 10:04 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > > On Fri, 12 Aug 2011, Pandita, Vikram wrote: > > > On Fri, Aug 12, 2011 at 8:50 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > > > On Fri, 12 Aug 2011, Vikram Pandita wrote: > > > > > >> From: Vikram Pandita <vikram.pandita@ti.com> > > >> > > >> musb pm_runtime_get_sync call happens in intrrupt context on cable attach case > > >> That can result in re-enabling the interrupts and cause side affects. > > >> > > >> So move the code to a work queue. > > > > > > Instead of creating your own, new work queue, just use the pre-existing > > > PM work queue. In other words, replace pm_runtime_get_sync() with > > > pm_runtime_get(). > > > > In this case it may not work, because immediately after a > > pm_runtime_get_sync(), > > we are going to access MUSB registers and xxx_sync() call ensures we > > will not abort. > > I don't understand. Why not just access those registers from within > the runtime_resume callback? I am not the runtime_pm expert, but as i understand, in this case: USB cable attach Get Interrupt from Power IC (twl6030) Interrupt handler calls notifier_call_chain - cable attach/detach notification handler in MUSB code Notification_handler does: * enable clocks for musb register access ** setup musb registers to start getting control ep0 interrupts for usb enumeration * is the part were we need to do the clock enabling synchronously. The runtime_resume() call-back today for musb does not have to do anything with USB enumeration. > > Alan Stern > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Felipe On Fri, Aug 12, 2011 at 10:21 AM, Pandita, Vikram <vikram.pandita@ti.com> wrote: > On Fri, Aug 12, 2011 at 10:04 AM, Alan Stern <stern@rowland.harvard.edu> wrote: >> >> On Fri, 12 Aug 2011, Pandita, Vikram wrote: >> >> > On Fri, Aug 12, 2011 at 8:50 AM, Alan Stern <stern@rowland.harvard.edu> wrote: >> > > On Fri, 12 Aug 2011, Vikram Pandita wrote: >> > > >> > >> From: Vikram Pandita <vikram.pandita@ti.com> >> > >> >> > >> musb pm_runtime_get_sync call happens in intrrupt context on cable attach case >> > >> That can result in re-enabling the interrupts and cause side affects. >> > >> >> > >> So move the code to a work queue. Any chance you go to line this up for the merge window? >> > > >> > > Instead of creating your own, new work queue, just use the pre-existing >> > > PM work queue. In other words, replace pm_runtime_get_sync() with >> > > pm_runtime_get(). >> > >> > In this case it may not work, because immediately after a >> > pm_runtime_get_sync(), >> > we are going to access MUSB registers and xxx_sync() call ensures we >> > will not abort. >> >> I don't understand. Why not just access those registers from within >> the runtime_resume callback? > > I am not the runtime_pm expert, but as i understand, in this case: > > USB cable attach > Get Interrupt from Power IC (twl6030) > Interrupt handler calls notifier_call_chain - cable attach/detach > notification handler in MUSB code > Notification_handler does: > * enable clocks for musb register access > ** setup musb registers to start getting control ep0 interrupts > for usb enumeration > > * is the part were we need to do the clock enabling synchronously. > The runtime_resume() call-back today for musb does not have to do > anything with USB enumeration. > >> >> Alan Stern >> > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h index 0e053b5..ff47316 100644 --- a/drivers/usb/musb/musb_core.h +++ b/drivers/usb/musb/musb_core.h @@ -386,6 +386,7 @@ struct musb { irqreturn_t (*isr)(int, void *); struct work_struct irq_work; + struct work_struct otg_notifier_work; u16 hwvers; /* this hub status bit is reserved by USB 2.0 and not seen by usbcore */ @@ -432,6 +433,7 @@ struct musb { u16 int_tx; struct otg_transceiver *xceiv; + u8 xceiv_event; int nIrq; unsigned irq_wake:1; diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c index c5d4c44..3026673 100644 --- a/drivers/usb/musb/omap2430.c +++ b/drivers/usb/musb/omap2430.c @@ -234,11 +234,21 @@ static int musb_otg_notifications(struct notifier_block *nb, unsigned long event, void *unused) { struct musb *musb = container_of(nb, struct musb, nb); + + musb->xceiv_event = event; + schedule_work(&musb->otg_notifier_work); + + return 0; +} + +static void musb_otg_notifier_work(struct work_struct *data_notifier_work) +{ + struct musb *musb = container_of(data_notifier_work, struct musb, otg_notifier_work); struct device *dev = musb->controller; struct musb_hdrc_platform_data *pdata = dev->platform_data; struct omap_musb_board_data *data = pdata->board_data; - switch (event) { + switch (musb->xceiv_event) { case USB_EVENT_ID: dev_dbg(musb->controller, "ID GND\n"); @@ -310,6 +320,8 @@ static int omap2430_musb_init(struct musb *musb) return -ENODEV; } + INIT_WORK(&musb->otg_notifier_work, musb_otg_notifier_work); + status = pm_runtime_get_sync(dev); if (status < 0) { dev_err(dev, "pm_runtime_get_sync FAILED");