[PATCH] usb: musb: omap2430: handle charger OTG xceiver event

[PATCH] usb: musb: omap2430: handle charger OTG xceiver event

[Dima Zavin]

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-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/musb/musb_core.h |    1 +
 drivers/usb/musb/omap2430.c  |   12 ++++++++++++
 2 files changed, 13 insertions(+), 0 deletions(-)

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
-- 
1.7.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: musb: omap2430: handle charger OTG xceiver event

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 799 bytes --]

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.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH] usb: musb: omap2430: handle charger OTG xceiver event

[Dima Zavin]

On Mon, Jul 11, 2011 at 11:01 PM, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> 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-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: musb: omap2430: handle charger OTG xceiver event

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3630 bytes --]

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-l0cyMroinI0@public.gmane.org> 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.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]