| Message ID | 1362563800-16673-5-git-send-email-peter.chen@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Wed, Mar 06, 2013 at 05:56:35PM +0800, Peter Chen wrote: > - During the connect/disconnect host, we need to pullup > and pulldown dp > - Make sure the dp is not pullup until the vbus is on when > flag CI13XXX_PULLUP_ON_VBUS is set > - Using hw_device_state when set run/stop bit > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > --- > drivers/usb/chipidea/udc.c | 10 ++++++++-- > 1 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > index e82dae4..70f9f2d 100644 > --- a/drivers/usb/chipidea/udc.c > +++ b/drivers/usb/chipidea/udc.c > @@ -91,8 +91,10 @@ static int hw_device_state(struct ci13xxx *ci, u32 dma) > /* interrupt, error, port change, reset, sleep/suspend */ > hw_write(ci, OP_USBINTR, ~0, > USBi_UI|USBi_UEI|USBi_PCI|USBi_URI|USBi_SLI); > + hw_write(ci, OP_USBCMD, USBCMD_RS, USBCMD_RS); > } else { > hw_write(ci, OP_USBINTR, ~0, 0); > + hw_write(ci, OP_USBCMD, USBCMD_RS, 0); this patch doesn't make sense to me. What will happen is that you will be enabling pullups when vbus_session() gets called and this might not be what gadget driver wants. You don't want to fiddle with that yourself since I'm changing the framework so that gadget driver will always request pullups to be enabled. Alex, I don't think this patch is correct
On Wed, Mar 06, 2013 at 01:26:33PM +0200, Felipe Balbi wrote: > Hi, > > On Wed, Mar 06, 2013 at 05:56:35PM +0800, Peter Chen wrote: > > - During the connect/disconnect host, we need to pullup > > and pulldown dp > > - Make sure the dp is not pullup until the vbus is on when > > flag CI13XXX_PULLUP_ON_VBUS is set > > - Using hw_device_state when set run/stop bit > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > > --- > > drivers/usb/chipidea/udc.c | 10 ++++++++-- > > 1 files changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > > index e82dae4..70f9f2d 100644 > > --- a/drivers/usb/chipidea/udc.c > > +++ b/drivers/usb/chipidea/udc.c > > @@ -91,8 +91,10 @@ static int hw_device_state(struct ci13xxx *ci, u32 dma) > > /* interrupt, error, port change, reset, sleep/suspend */ > > hw_write(ci, OP_USBINTR, ~0, > > USBi_UI|USBi_UEI|USBi_PCI|USBi_URI|USBi_SLI); > > + hw_write(ci, OP_USBCMD, USBCMD_RS, USBCMD_RS); > > } else { > > hw_write(ci, OP_USBINTR, ~0, 0); > > + hw_write(ci, OP_USBCMD, USBCMD_RS, 0); > > this patch doesn't make sense to me. What will happen is that you will > be enabling pullups when vbus_session() gets called and this might not > be what gadget driver wants. > > You don't want to fiddle with that yourself since I'm changing the > framework so that gadget driver will always request pullups to be > enabled. Hi Felipe, Do you think pullup dp without vbus is a valid operation? Current udc core code makes that possible. But I think current your udc core code (add pullup after loading gadget) make benefit for udc driver who has not vbus operation. For chipidea driver: - If vbus is there before load gadget module, the pullup dp is done by udc core code. - If vbus is not there before load gadget module, the pullup will be done when the vbus is there. > > Alex, I don't think this patch is correct > > -- > balbi
Hi, On Thu, Mar 07, 2013 at 10:36:13AM +0800, Peter Chen wrote: > > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > > > index e82dae4..70f9f2d 100644 > > > --- a/drivers/usb/chipidea/udc.c > > > +++ b/drivers/usb/chipidea/udc.c > > > @@ -91,8 +91,10 @@ static int hw_device_state(struct ci13xxx *ci, u32 dma) > > > /* interrupt, error, port change, reset, sleep/suspend */ > > > hw_write(ci, OP_USBINTR, ~0, > > > USBi_UI|USBi_UEI|USBi_PCI|USBi_URI|USBi_SLI); > > > + hw_write(ci, OP_USBCMD, USBCMD_RS, USBCMD_RS); > > > } else { > > > hw_write(ci, OP_USBINTR, ~0, 0); > > > + hw_write(ci, OP_USBCMD, USBCMD_RS, 0); > > > > this patch doesn't make sense to me. What will happen is that you will > > be enabling pullups when vbus_session() gets called and this might not > > be what gadget driver wants. > > > > You don't want to fiddle with that yourself since I'm changing the > > framework so that gadget driver will always request pullups to be > > enabled. > > Hi Felipe, > > Do you think pullup dp without vbus is a valid operation? why not ? What I want to connect pullups first and only then issue SRP ? > Current udc core code makes that possible. so ? > But I think current your udc core code (add pullup after loading > gadget) make benefit for udc driver who has not vbus operation. > > For chipidea driver: > > - If vbus is there before load gadget module, the pullup dp is done by > udc core code. > - If vbus is not there before load gadget module, the pullup will be > done when the vbus is there. This isn't legal. If you want to make sure vbus is alive before connecting pullups, then do it generically. Modify udc-core.c to make those checks for you. Bypassing the framework is dangerous because whenever I wanna change something, I might miss your private details and end up causing regressions.
On Thu, Mar 07, 2013 at 11:50:54AM +0200, Felipe Balbi wrote: > Hi, > > On Thu, Mar 07, 2013 at 10:36:13AM +0800, Peter Chen wrote: > > > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > > > > index e82dae4..70f9f2d 100644 > > > > --- a/drivers/usb/chipidea/udc.c > > > > +++ b/drivers/usb/chipidea/udc.c > > > > @@ -91,8 +91,10 @@ static int hw_device_state(struct ci13xxx *ci, u32 dma) > > > > /* interrupt, error, port change, reset, sleep/suspend */ > > > > hw_write(ci, OP_USBINTR, ~0, > > > > USBi_UI|USBi_UEI|USBi_PCI|USBi_URI|USBi_SLI); > > > > + hw_write(ci, OP_USBCMD, USBCMD_RS, USBCMD_RS); > > > > } else { > > > > hw_write(ci, OP_USBINTR, ~0, 0); > > > > + hw_write(ci, OP_USBCMD, USBCMD_RS, 0); > > > > > > this patch doesn't make sense to me. What will happen is that you will > > > be enabling pullups when vbus_session() gets called and this might not > > > be what gadget driver wants. > > > > > > You don't want to fiddle with that yourself since I'm changing the > > > framework so that gadget driver will always request pullups to be > > > enabled. > > > > Hi Felipe, > > > > Do you think pullup dp without vbus is a valid operation? > > why not ? What I want to connect pullups first and only then issue SRP ? I am not familiar with OTG, but it only stands for special case, right? > > > Current udc core code makes that possible. > > so ? Without vbus, but pullup dp, it will cause more power. > > > But I think current your udc core code (add pullup after loading > > gadget) make benefit for udc driver who has not vbus operation. > > > > For chipidea driver: > > > > - If vbus is there before load gadget module, the pullup dp is done by > > udc core code. > > - If vbus is not there before load gadget module, the pullup will be > > done when the vbus is there. > > This isn't legal. If you want to make sure vbus is alive before > connecting pullups, then do it generically. Modify udc-core.c to make > those checks for you. Bypassing the framework is dangerous because > whenever I wanna change something, I might miss your private details and > end up causing regressions. Let thing be generic is a good idea. Then, is it ok I post a patch let udc manage pullup by itself (through a flag) just you did for uvc? UDC core doesn't know VBUS, so the pullup can't be managed by udc core totally. Besides, I looked four udc drivers (fsl_udc_core.c, at91_udc.c, mv_udc_core.c and bcm63xx_udc.c), the first three manage pullup by itself, ony bcm doesn't control it by itself. > -- > balbi
Hi, On Fri, Mar 08, 2013 at 09:28:34AM +0800, Peter Chen wrote: > > > > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > > > > > index e82dae4..70f9f2d 100644 > > > > > --- a/drivers/usb/chipidea/udc.c > > > > > +++ b/drivers/usb/chipidea/udc.c > > > > > @@ -91,8 +91,10 @@ static int hw_device_state(struct ci13xxx *ci, u32 dma) > > > > > /* interrupt, error, port change, reset, sleep/suspend */ > > > > > hw_write(ci, OP_USBINTR, ~0, > > > > > USBi_UI|USBi_UEI|USBi_PCI|USBi_URI|USBi_SLI); > > > > > + hw_write(ci, OP_USBCMD, USBCMD_RS, USBCMD_RS); > > > > > } else { > > > > > hw_write(ci, OP_USBINTR, ~0, 0); > > > > > + hw_write(ci, OP_USBCMD, USBCMD_RS, 0); > > > > > > > > this patch doesn't make sense to me. What will happen is that you will > > > > be enabling pullups when vbus_session() gets called and this might not > > > > be what gadget driver wants. > > > > > > > > You don't want to fiddle with that yourself since I'm changing the > > > > framework so that gadget driver will always request pullups to be > > > > enabled. > > > > > > Hi Felipe, > > > > > > Do you think pullup dp without vbus is a valid operation? > > > > why not ? What I want to connect pullups first and only then issue SRP ? > > I am not familiar with OTG, but it only stands for special case, right? SRP is not OTG-specific. Any host is allowed to support it and any device is allowed to initiate it. > > > Current udc core code makes that possible. > > > > so ? > > Without vbus, but pullup dp, it will cause more power. a fair concern. > > > But I think current your udc core code (add pullup after loading > > > gadget) make benefit for udc driver who has not vbus operation. > > > > > > For chipidea driver: > > > > > > - If vbus is there before load gadget module, the pullup dp is done by > > > udc core code. > > > - If vbus is not there before load gadget module, the pullup will be > > > done when the vbus is there. > > > > This isn't legal. If you want to make sure vbus is alive before > > connecting pullups, then do it generically. Modify udc-core.c to make > > those checks for you. Bypassing the framework is dangerous because > > whenever I wanna change something, I might miss your private details and > > end up causing regressions. > > Let thing be generic is a good idea. Then, is it ok I post a patch let > udc manage pullup by itself (through a flag) just you did for uvc? > UDC core doesn't know VBUS, so the pullup can't be managed by udc core > totally. then teach UDC core about VBUS. Send the patches and we will figure out if they can be applied or not. > Besides, I looked four udc drivers (fsl_udc_core.c, at91_udc.c, mv_udc_core.c > and bcm63xx_udc.c), the first three manage pullup by itself, ony > bcm doesn't control it by itself. those are all bugs that need to be sorted out. I don't have that HW so I can't test any of my changes.
On Fri, Mar 08, 2013 at 09:18:32AM +0200, Felipe Balbi wrote: > > > > > > This isn't legal. If you want to make sure vbus is alive before > > > connecting pullups, then do it generically. Modify udc-core.c to make > > > those checks for you. Bypassing the framework is dangerous because > > > whenever I wanna change something, I might miss your private details and > > > end up causing regressions. > > > > Let thing be generic is a good idea. Then, is it ok I post a patch let > > udc manage pullup by itself (through a flag) just you did for uvc? > > UDC core doesn't know VBUS, so the pullup can't be managed by udc core > > totally. > > then teach UDC core about VBUS. Send the patches and we will figure out > if they can be applied or not. I will do it > > -- > balbi
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c index e82dae4..70f9f2d 100644 --- a/drivers/usb/chipidea/udc.c +++ b/drivers/usb/chipidea/udc.c @@ -91,8 +91,10 @@ static int hw_device_state(struct ci13xxx *ci, u32 dma) /* interrupt, error, port change, reset, sleep/suspend */ hw_write(ci, OP_USBINTR, ~0, USBi_UI|USBi_UEI|USBi_PCI|USBi_URI|USBi_SLI); + hw_write(ci, OP_USBCMD, USBCMD_RS, USBCMD_RS); } else { hw_write(ci, OP_USBINTR, ~0, 0); + hw_write(ci, OP_USBCMD, USBCMD_RS, 0); } return 0; } @@ -1424,10 +1426,14 @@ static int ci13xxx_pullup(struct usb_gadget *_gadget, int is_on) { struct ci13xxx *ci = container_of(_gadget, struct ci13xxx, gadget); + if ((ci->platdata->flags & CI13XXX_PULLUP_ON_VBUS) && + !ci->vbus_active) + return -EPERM; + if (is_on) - hw_write(ci, OP_USBCMD, USBCMD_RS, USBCMD_RS); + hw_device_state(ci, ci->ep0out->qh.dma); else - hw_write(ci, OP_USBCMD, USBCMD_RS, 0); + hw_device_state(ci, 0); return 0; }
- During the connect/disconnect host, we need to pullup and pulldown dp - Make sure the dp is not pullup until the vbus is on when flag CI13XXX_PULLUP_ON_VBUS is set - Using hw_device_state when set run/stop bit Signed-off-by: Peter Chen <peter.chen@freescale.com> --- drivers/usb/chipidea/udc.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)