| Message ID | 1363240242-25775-15-git-send-email-peter.chen@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Thu, Mar 14, 2013 at 01:50:42PM +0800, Peter Chen wrote: > - If there is no vbus control to indicate connection. > and disconnect, we can pullup dp when we load gadget module. > - If we have vbus control logic, the dp is better pull up > when there is a vbus session. > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > --- > drivers/usb/gadget/udc-core.c | 10 ++++++++-- > 1 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c > index 4d90bdf..4b56f7c 100644 > --- a/drivers/usb/gadget/udc-core.c > +++ b/drivers/usb/gadget/udc-core.c > @@ -278,7 +278,10 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri > driver->unbind(gadget); > goto err1; > } > - usb_gadget_connect(gadget); > + if (!gadget->ops->vbus_session || > + (gadget->ops->vbus_session > + && gadget->vbus_active)) > + usb_gadget_connect(gadget); > > kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); > return 0; > @@ -384,7 +387,10 @@ static ssize_t usb_udc_softconn_store(struct device *dev, > > if (sysfs_streq(buf, "connect")) { > usb_gadget_udc_start(gadget, udc->driver); > - usb_gadget_connect(gadget); > + if (!gadget->ops->vbus_session || > + (gadget->ops->vbus_session > + && gadget->vbus_active)) > + usb_gadget_connect(gadget); this patch is incomplete. What happens if this test fails ? Who will connect pullup then ?
On Thu, Mar 14, 2013 at 11:00:05AM +0200, Felipe Balbi wrote: > Hi, > > On Thu, Mar 14, 2013 at 01:50:42PM +0800, Peter Chen wrote: > > - If there is no vbus control to indicate connection. > > and disconnect, we can pullup dp when we load gadget module. > > - If we have vbus control logic, the dp is better pull up > > when there is a vbus session. > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > > --- > > drivers/usb/gadget/udc-core.c | 10 ++++++++-- > > 1 files changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c > > index 4d90bdf..4b56f7c 100644 > > --- a/drivers/usb/gadget/udc-core.c > > +++ b/drivers/usb/gadget/udc-core.c > > @@ -278,7 +278,10 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri > > driver->unbind(gadget); > > goto err1; > > } > > - usb_gadget_connect(gadget); > > + if (!gadget->ops->vbus_session || > > + (gadget->ops->vbus_session > > + && gadget->vbus_active)) > > + usb_gadget_connect(gadget); > > > > kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); > > return 0; > > @@ -384,7 +387,10 @@ static ssize_t usb_udc_softconn_store(struct device *dev, > > > > if (sysfs_streq(buf, "connect")) { > > usb_gadget_udc_start(gadget, udc->driver); > > - usb_gadget_connect(gadget); > > + if (!gadget->ops->vbus_session || > > + (gadget->ops->vbus_session > > + && gadget->vbus_active)) > > + usb_gadget_connect(gadget); > > this patch is incomplete. What happens if this test fails ? Who will > connect pullup then ? gadget->ops->vbus_session will handle it when the vbus interrupt comes > > -- > balbi
Hi, On Thu, Mar 14, 2013 at 05:24:39PM +0800, Peter Chen wrote: > > > @@ -278,7 +278,10 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri > > > driver->unbind(gadget); > > > goto err1; > > > } > > > - usb_gadget_connect(gadget); > > > + if (!gadget->ops->vbus_session || > > > + (gadget->ops->vbus_session > > > + && gadget->vbus_active)) > > > + usb_gadget_connect(gadget); > > > > > > kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); > > > return 0; > > > @@ -384,7 +387,10 @@ static ssize_t usb_udc_softconn_store(struct device *dev, > > > > > > if (sysfs_streq(buf, "connect")) { > > > usb_gadget_udc_start(gadget, udc->driver); > > > - usb_gadget_connect(gadget); > > > + if (!gadget->ops->vbus_session || > > > + (gadget->ops->vbus_session > > > + && gadget->vbus_active)) > > > + usb_gadget_connect(gadget); > > > > this patch is incomplete. What happens if this test fails ? Who will > > connect pullup then ? > > gadget->ops->vbus_session will handle it when the vbus interrupt comes for your driver, what about all the others ? Also, we shouldn't allow this ping-pong between who handles pullup and who handles vbus_session. It should all be managed by udc-core with UDC drivers just providing enough hooks. If we allow the UDC driver to connect pullups when VBUS IRQ comes, we could fall into all sorts of traps: a) we could connect cable with no gadget driver loaded b) there will be code duplication among all UDC drivers to call usb_gadge_connect() from vbus_session c) we might screw up the usb_function_activate()/deactivate() counter Need to be very careful with all these details, there are many, many users to udc-core with different requirements. So unless we can come up with a way which wouldn't cause code duplication or regressions, I don't think we can solve the real problem. I guess the best solution to all problems is to start deferring pullup to when gadget driver says it's ok to connect them. It's far more likely that we will already have connection to a host and VBUS will be alive. Also, I'm not sure what does this all mean for SRP. Should we connect pullup before or after SRP ?
On Thu, Mar 14, 2013 at 12:19:04PM +0200, Felipe Balbi wrote: > Hi, > > On Thu, Mar 14, 2013 at 05:24:39PM +0800, Peter Chen wrote: > > > > @@ -278,7 +278,10 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri > > > > driver->unbind(gadget); > > > > goto err1; > > > > } > > > > - usb_gadget_connect(gadget); > > > > + if (!gadget->ops->vbus_session || > > > > + (gadget->ops->vbus_session > > > > + && gadget->vbus_active)) > > > > + usb_gadget_connect(gadget); > > > > > > > > kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); > > > > return 0; > > > > @@ -384,7 +387,10 @@ static ssize_t usb_udc_softconn_store(struct device *dev, > > > > > > > > if (sysfs_streq(buf, "connect")) { > > > > usb_gadget_udc_start(gadget, udc->driver); > > > > - usb_gadget_connect(gadget); > > > > + if (!gadget->ops->vbus_session || > > > > + (gadget->ops->vbus_session > > > > + && gadget->vbus_active)) > > > > + usb_gadget_connect(gadget); > > > > > > this patch is incomplete. What happens if this test fails ? Who will > > > connect pullup then ? > > > > gadget->ops->vbus_session will handle it when the vbus interrupt comes > > for your driver, what about all the others ? All drivers have .vbus_session will try to pullup, but some still check if .pullup (using .softconnect) is called beforehand, it is duplicated with this patch. Sorry, my careless. If you have a look with these drivers, even usb_gadget_connect is called at udc_bind_to_driver, they will NOT pull up if vbus is not there. The most strict condition is : gadget_is_loaded && vbus_session_is_called && pullup_is_called In fact, calling usb_gadget_connect(gadget) unconditionally at udc-core may hang system if low power is enabled as udc will enter low power mode after udc_start if no vbus is there. > Also, we shouldn't allow > this ping-pong between who handles pullup and who handles vbus_session. > > It should all be managed by udc-core with UDC drivers just providing > enough hooks. If we allow the UDC driver to connect pullups when VBUS > IRQ comes, we could fall into all sorts of traps: > > a) we could connect cable with no gadget driver loaded In that case, the pullup will not be called, it will check if gadget module is loaded. > b) there will be code duplication among all UDC drivers to call > usb_gadge_connect() from vbus_session Yes > c) we might screw up the usb_function_activate()/deactivate() counter > why? > Need to be very careful with all these details, there are many, many > users to udc-core with different requirements. So unless we can come up > with a way which wouldn't cause code duplication or regressions, I don't > think we can solve the real problem. Yes, udc has vendor specific, no uniform spec like host. > > I guess the best solution to all problems is to start deferring > pullup to when gadget driver says it's ok to connect them. It's far more > likely that we will already have connection to a host and VBUS will be > alive. I still think (gadget_is_loaded && vbus_is_there) is enough. > > Also, I'm not sure what does this all mean for SRP. Should we connect > pullup before or after SRP ? I am not familiar with SRP, but I think vbus is pre-condition.
Hi, On Fri, Mar 15, 2013 at 02:06:16PM +0800, Peter Chen wrote: > > > > > @@ -278,7 +278,10 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri > > > > > driver->unbind(gadget); > > > > > goto err1; > > > > > } > > > > > - usb_gadget_connect(gadget); > > > > > + if (!gadget->ops->vbus_session || > > > > > + (gadget->ops->vbus_session > > > > > + && gadget->vbus_active)) > > > > > + usb_gadget_connect(gadget); > > > > > > > > > > kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); > > > > > return 0; > > > > > @@ -384,7 +387,10 @@ static ssize_t usb_udc_softconn_store(struct device *dev, > > > > > > > > > > if (sysfs_streq(buf, "connect")) { > > > > > usb_gadget_udc_start(gadget, udc->driver); > > > > > - usb_gadget_connect(gadget); > > > > > + if (!gadget->ops->vbus_session || > > > > > + (gadget->ops->vbus_session > > > > > + && gadget->vbus_active)) > > > > > + usb_gadget_connect(gadget); > > > > > > > > this patch is incomplete. What happens if this test fails ? Who will > > > > connect pullup then ? > > > > > > gadget->ops->vbus_session will handle it when the vbus interrupt comes > > > > for your driver, what about all the others ? > > All drivers have .vbus_session will try to pullup, but some still check > if .pullup (using .softconnect) is called beforehand, it is duplicated > with this patch. Sorry, my careless. > If you have a look with these drivers, even usb_gadget_connect is called > at udc_bind_to_driver, they will NOT pull up if vbus is not there. that's all wrong and needs to be fixed. UDC-core has to be the central point for all these details. We start with the easiest way (call connect unconditionally after gadget driver is loaded) and optimize it as we go (making sure that there is VBUS before connecting pullups); but we can't bypass UDC-core and call usb_gadget_connect() directly from UDC driver. > The most strict condition is : > gadget_is_loaded && vbus_session_is_called && pullup_is_called > > In fact, calling usb_gadget_connect(gadget) unconditionally at udc-core > may hang system if low power is enabled as udc will enter low > power mode after udc_start if no vbus is there. that's a bug in the UDC driver though. It should add proper pm_runtime_get_sync() and pm_runtime_put() (or put_sync()) around register accesses, which means that ->pullup() should take care of that too. > > Also, we shouldn't allow > > this ping-pong between who handles pullup and who handles vbus_session. > > > > It should all be managed by udc-core with UDC drivers just providing > > enough hooks. If we allow the UDC driver to connect pullups when VBUS > > IRQ comes, we could fall into all sorts of traps: > > > > a) we could connect cable with no gadget driver loaded > > In that case, the pullup will not be called, it will check if gadget > module is loaded. I don't see that in your patch. > > b) there will be code duplication among all UDC drivers to call > > usb_gadge_connect() from vbus_session > > Yes yeah, we need to avoid it. > > c) we might screw up the usb_function_activate()/deactivate() counter > > > > why? (USB cable already attached to a host, VBUS alive) gadget driver is loaded, gadget driver calls usb_function_deactive() to prevent enumeration until userland is ready, UDC driver calls usb_gadget_connect() because there is a gadget driver and vbus is alive. > > Need to be very careful with all these details, there are many, many > > users to udc-core with different requirements. So unless we can come up > > with a way which wouldn't cause code duplication or regressions, I don't > > think we can solve the real problem. > > Yes, udc has vendor specific, no uniform spec like host. there are plenty of non-uniform hosts as well. MUSB and dwc2 are just two instances of them. Still usbcore works just fine with those and the standard ones. > > I guess the best solution to all problems is to start deferring > > pullup to when gadget driver says it's ok to connect them. It's far more > > likely that we will already have connection to a host and VBUS will be > > alive. > > I still think (gadget_is_loaded && vbus_is_there) is enough. it's not, see above. > > Also, I'm not sure what does this all mean for SRP. Should we connect > > pullup before or after SRP ? > > I am not familiar with SRP, but I think vbus is pre-condition. hehe, google SRP. Nah, I'll save you the trouble. SRP is a mechanism for the USB peripheral to ask the host to enable VBUS. This means that we can keep the VBUS charge pump disabled, and thus save power, until peripheral requests for a session.
diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c index 4d90bdf..4b56f7c 100644 --- a/drivers/usb/gadget/udc-core.c +++ b/drivers/usb/gadget/udc-core.c @@ -278,7 +278,10 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri driver->unbind(gadget); goto err1; } - usb_gadget_connect(gadget); + if (!gadget->ops->vbus_session || + (gadget->ops->vbus_session + && gadget->vbus_active)) + usb_gadget_connect(gadget); kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); return 0; @@ -384,7 +387,10 @@ static ssize_t usb_udc_softconn_store(struct device *dev, if (sysfs_streq(buf, "connect")) { usb_gadget_udc_start(gadget, udc->driver); - usb_gadget_connect(gadget); + if (!gadget->ops->vbus_session || + (gadget->ops->vbus_session + && gadget->vbus_active)) + usb_gadget_connect(gadget); } else if (sysfs_streq(buf, "disconnect")) { usb_gadget_disconnect(gadget); usb_gadget_udc_stop(gadget, udc->driver);
- If there is no vbus control to indicate connection. and disconnect, we can pullup dp when we load gadget module. - If we have vbus control logic, the dp is better pull up when there is a vbus session. Signed-off-by: Peter Chen <peter.chen@freescale.com> --- drivers/usb/gadget/udc-core.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)