| Message ID | 1310424197-22909-1-git-send-email-dima@android.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Mon, Jul 11, 2011 at 03:43:17PM -0700, Dima Zavin wrote: > Set a flag on OTG charger event and check it on cable > remove event (i.e. USB_EVENT_NONE). This way we will > not need to power up the PHY when an external charger s/PHY/LINK > is detected by the transceiver itself. ... your logic is inversed. Link shouldn't be powered up to start with. We should only power up the Link after we know it *will* be needed. The way you're doing this is: connect cable -> resume PHY -> resume Link -> if is_charger suspend Link. Where it should be: connect cable -> resume PHY -> if is_charger goto done. Now, to make a generic solution (which is what we want in the long run) we still need a few changes on the OTG support and otg_transceiver on Linux.
On Mon, Jul 11, 2011 at 11:01 PM, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Mon, Jul 11, 2011 at 03:43:17PM -0700, Dima Zavin wrote: >> Set a flag on OTG charger event and check it on cable >> remove event (i.e. USB_EVENT_NONE). This way we will >> not need to power up the PHY when an external charger > > s/PHY/LINK Actually it's both, I should update the commit message. > >> is detected by the transceiver itself. > > ... your logic is inversed. Link shouldn't be powered up to start with. > We should only power up the Link after we know it *will* be needed. The > way you're doing this is: The link *isn't* being powered up by my patch. On an already detected charger you should do nothing at all in musb. That's the whole point of the patch: add the ability to process the USB_EVENT_CHARGER event in musb from the otg_transceiver that has already detected the charger. Today, on USB_EVENT_VBUS, you'd resume your link and then call otg_init, which would powerup the phy on omap4, which we do not need to do for USB_EVENT_CHARGER. > > connect cable -> resume PHY -> resume Link -> if is_charger suspend > Link. Not at all. Have even you looked at the patch? With the patch, 1) "connect cable", detected by otg_transceiver. 2) otg_transceiver does the detection of ID pin and/or VBUS and/or dedicated charger. 3) otg_transceiver sends USB_EVENT_xxx 4) In musb_otg_notification(), check if the transceiver detected a charger (USB_EVENT_CHARGER). 4.1) If yes, then do nothing and leave the LINK *and* the PHY in their off states. > > Where it should be: > > connect cable -> resume PHY -> if is_charger goto done. We only have to resume the PHY if the transceiver didn't do the charger detection. The TWL6030 doesn't do that, but depending on your board, you may have other devices upstream from the OMAP on the USB lines that may have done the detection for you. --Dima > > Now, to make a generic solution (which is what we want in the long run) > we still need a few changes on the OTG support and otg_transceiver on > Linux. > > -- > balbi > -- 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
Hi, On Tue, Jul 12, 2011 at 10:44:19AM -0700, Dima Zavin wrote: > On Mon, Jul 11, 2011 at 11:01 PM, Felipe Balbi <balbi@ti.com> wrote: > > Hi, > > > > On Mon, Jul 11, 2011 at 03:43:17PM -0700, Dima Zavin wrote: > >> Set a flag on OTG charger event and check it on cable > >> remove event (i.e. USB_EVENT_NONE). This way we will > >> not need to power up the PHY when an external charger > > > > s/PHY/LINK > > Actually it's both, I should update the commit message. the PHY has to be powered up during charger detection. You could power it down after charger has been started and power it back up on the rising edge (or high level, don't remember if this is edge of level triggered) of the VBUS IRQ. > >> is detected by the transceiver itself. > > > > ... your logic is inversed. Link shouldn't be powered up to start with. > > We should only power up the Link after we know it *will* be needed. The > > way you're doing this is: > > The link *isn't* being powered up by my patch. On an already detected not by your patch, true. My point is that we're powering it up too early and if you want to change anything regarding Charger Detection you need to change at the right place. > charger you should do nothing at all in musb. That's the whole point > of the patch: add the ability to process the USB_EVENT_CHARGER event > in musb from the otg_transceiver that has already detected the > charger. Today, on USB_EVENT_VBUS, you'd resume your link and then and what I said (or tried to) is that USB_EVENT_VBUS isn't an event to be handled by the link at all. The link should be pm_runtime_suspend()ed right after probe() and should be kept that way until PHY sends USB_EVENT_CHARGER_DETECTION_DONE or something similar. > call otg_init, which would powerup the phy on omap4, which we do not > need to do for USB_EVENT_CHARGER. That is wrong true. Still your patch only adds a flag. It's not implementing what you're describing here. > > connect cable -> resume PHY -> resume Link -> if is_charger suspend > > Link. > > Not at all. Have even you looked at the patch? With the patch, > > 1) "connect cable", detected by otg_transceiver. > 2) otg_transceiver does the detection of ID pin and/or VBUS and/or > dedicated charger. > 3) otg_transceiver sends USB_EVENT_xxx yes, and it will send USB_EVENT_VBUS which is resuming link. > 4) In musb_otg_notification(), check if the transceiver detected a > charger (USB_EVENT_CHARGER). > 4.1) If yes, then do nothing and leave the LINK *and* the PHY in their > off states. this is not what your patch is doing. It only enables/disables a flag which your patch added. > > Where it should be: > > > > connect cable -> resume PHY -> if is_charger goto done. > > We only have to resume the PHY if the transceiver didn't do the > charger detection. The TWL6030 doesn't do that, but depending on your PHY == transceiver. Like I said before, if you want to put any changes regarding charger detection, do it at the right location. First add some events for different USB port types (SDP, CDP, DCP), then notify those, then add handling of those events to MUSB and PHY drivers, then fix up musb to be runtime suspended right after probe, then you only need to wake musb UP in case of SDP and CDP. If you're attached to a DCP you don't even need MUSB to handle that. Also, while you are doing Charger detection, be sure to disable SoftConnect bit on MUSB and put PHY in NONDRIVING mode, this will prevent MUSB from even knowing there's a cable connected.
diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h index 0e053b5..f65269d 100644 --- a/drivers/usb/musb/musb_core.h +++ b/drivers/usb/musb/musb_core.h @@ -497,6 +497,7 @@ struct musb { struct usb_gadget g; /* the gadget */ struct usb_gadget_driver *gadget_driver; /* its driver */ #endif + bool is_ac_charger:1; /* * FIXME: Remove this flag. diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c index c5d4c44..ac4cfbc 100644 --- a/drivers/usb/musb/omap2430.c +++ b/drivers/usb/musb/omap2430.c @@ -257,6 +257,11 @@ static int musb_otg_notifications(struct notifier_block *nb, } break; + case USB_EVENT_CHARGER: + dev_dbg(musb->controller, "Dedicated charger connect\n"); + musb->is_ac_charger = true; + break; + case USB_EVENT_VBUS: dev_dbg(musb->controller, "VBUS Connect\n"); @@ -268,6 +273,13 @@ static int musb_otg_notifications(struct notifier_block *nb, break; case USB_EVENT_NONE: + if (musb->is_ac_charger) { + dev_dbg(musb->controller, + "Dedicated charger disconnect\n"); + musb->is_ac_charger = false; + break; + } + dev_dbg(musb->controller, "VBUS Disconnect\n"); #ifdef CONFIG_USB_GADGET_MUSB_HDRC
Set a flag on OTG charger event and check it on cable remove event (i.e. USB_EVENT_NONE). This way we will not need to power up the PHY when an external charger is detected by the transceiver itself. Signed-off-by: Dima Zavin <dima@android.com> --- drivers/usb/musb/musb_core.h | 1 + drivers/usb/musb/omap2430.c | 12 ++++++++++++ 2 files changed, 13 insertions(+), 0 deletions(-)