| Message ID | 1414742668-4107-1-git-send-email-m.szyprowski@samsung.com |
|---|---|
| State | New |
| Headers | show |
> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com] > Sent: Friday, October 31, 2014 1:04 AM > To: linux-usb@vger.kernel.org; linux-samsung-soc@vger.kernel.org > Cc: Marek Szyprowski; Kyungmin Park; Robert Baldyga; Paul Zimmerman; Krzysztof Kozlowski; Felipe Balbi > Subject: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end session' irq > > This patch adds a call to s3c_hsotg_disconnect() from 'end session' > interrupt (GOTGINT_SES_END_DET) to correctly notify gadget subsystem > about unplugged usb cable. 'disconnected' interrupt (DISCONNINT) might > look a bit more suitable for this event, but it is asserted only in > host mode, so in device mode we need to use something else. > > Additional check has been added in s3c_hsotg_disconnect() function > to ensure that the event is reported only once after successful device > enumeration. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/usb/dwc2/core.h | 1 + > drivers/usb/dwc2/gadget.c | 10 ++++++++++ > 2 files changed, 11 insertions(+) > > diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h > index 55c90c53f2d6..b42df32e7737 100644 > --- a/drivers/usb/dwc2/core.h > +++ b/drivers/usb/dwc2/core.h > @@ -212,6 +212,7 @@ struct s3c_hsotg { > struct usb_gadget gadget; > unsigned int setup; > unsigned long last_rst; > + unsigned int address; > struct s3c_hsotg_ep *eps; > }; > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > index fcd2bb55ccca..6304efba11aa 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -1114,6 +1114,7 @@ static void s3c_hsotg_process_control(struct s3c_hsotg *hsotg, > DCFG_DEVADDR_SHIFT) & DCFG_DEVADDR_MASK; > writel(dcfg, hsotg->regs + DCFG); > > + hsotg->address = ctrl->wValue; > dev_info(hsotg->dev, "new address %d\n", ctrl->wValue); > > ret = s3c_hsotg_send_reply(hsotg, ep0, NULL, 0); > @@ -2031,6 +2032,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg) > { > unsigned ep; > > + if (!hsotg->address) > + return; > + > + hsotg->address = 0; > for (ep = 0; ep < hsotg->num_of_eps; ep++) > kill_all_requests(hsotg, &hsotg->eps[ep], -ESHUTDOWN, true); > > @@ -2290,6 +2295,11 @@ irq_retry: > dev_info(hsotg->dev, "OTGInt: %08x\n", otgint); > > writel(otgint, hsotg->regs + GOTGINT); > + > + if (otgint & GOTGINT_SES_END_DET) { > + s3c_hsotg_disconnect(hsotg); > + hsotg->gadget.speed = USB_SPEED_UNKNOWN; > + } > } > > if (gintsts & GINTSTS_SESSREQINT) { I don't think this is right. The host can send control requests to the device before it sends a SetAddress to change from the default address of 0. So if a GOTGINT_SES_END_DET happens before the SetAddress, it will be ignored. Or am I missing something?
Hello, On 2014-10-31 19:15, Paul Zimmerman wrote: >> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com] >> Sent: Friday, October 31, 2014 1:04 AM >> To: linux-usb@vger.kernel.org; linux-samsung-soc@vger.kernel.org >> Cc: Marek Szyprowski; Kyungmin Park; Robert Baldyga; Paul Zimmerman; Krzysztof Kozlowski; Felipe Balbi >> Subject: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end session' irq >> >> This patch adds a call to s3c_hsotg_disconnect() from 'end session' >> interrupt (GOTGINT_SES_END_DET) to correctly notify gadget subsystem >> about unplugged usb cable. 'disconnected' interrupt (DISCONNINT) might >> look a bit more suitable for this event, but it is asserted only in >> host mode, so in device mode we need to use something else. >> >> Additional check has been added in s3c_hsotg_disconnect() function >> to ensure that the event is reported only once after successful device >> enumeration. >> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> --- >> drivers/usb/dwc2/core.h | 1 + >> drivers/usb/dwc2/gadget.c | 10 ++++++++++ >> 2 files changed, 11 insertions(+) >> >> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h >> index 55c90c53f2d6..b42df32e7737 100644 >> --- a/drivers/usb/dwc2/core.h >> +++ b/drivers/usb/dwc2/core.h >> @@ -212,6 +212,7 @@ struct s3c_hsotg { >> struct usb_gadget gadget; >> unsigned int setup; >> unsigned long last_rst; >> + unsigned int address; >> struct s3c_hsotg_ep *eps; >> }; >> >> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >> index fcd2bb55ccca..6304efba11aa 100644 >> --- a/drivers/usb/dwc2/gadget.c >> +++ b/drivers/usb/dwc2/gadget.c >> @@ -1114,6 +1114,7 @@ static void s3c_hsotg_process_control(struct s3c_hsotg *hsotg, >> DCFG_DEVADDR_SHIFT) & DCFG_DEVADDR_MASK; >> writel(dcfg, hsotg->regs + DCFG); >> >> + hsotg->address = ctrl->wValue; >> dev_info(hsotg->dev, "new address %d\n", ctrl->wValue); >> >> ret = s3c_hsotg_send_reply(hsotg, ep0, NULL, 0); >> @@ -2031,6 +2032,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg) >> { >> unsigned ep; >> >> + if (!hsotg->address) >> + return; >> + >> + hsotg->address = 0; >> for (ep = 0; ep < hsotg->num_of_eps; ep++) >> kill_all_requests(hsotg, &hsotg->eps[ep], -ESHUTDOWN, true); >> >> @@ -2290,6 +2295,11 @@ irq_retry: >> dev_info(hsotg->dev, "OTGInt: %08x\n", otgint); >> >> writel(otgint, hsotg->regs + GOTGINT); >> + >> + if (otgint & GOTGINT_SES_END_DET) { >> + s3c_hsotg_disconnect(hsotg); >> + hsotg->gadget.speed = USB_SPEED_UNKNOWN; >> + } >> } >> >> if (gintsts & GINTSTS_SESSREQINT) { > I don't think this is right. The host can send control requests to > the device before it sends a SetAddress to change from the default > address of 0. So if a GOTGINT_SES_END_DET happens before the > SetAddress, it will be ignored. > > Or am I missing something? Well, right. However before finishing enumeration (setting the address) host usually only retrieves some usb descriptors what doesn't change the state of the gadget. Right now we always reported 'disconnected' event before setting the new address, what is a bit overkill (in some cases gadget driver got this even more than once). The above code handles all cases correctly and reports disconnect event only once. Best regards
> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com] > Sent: Thursday, November 13, 2014 5:40 AM > > On 2014-10-31 19:15, Paul Zimmerman wrote: > >> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com] > >> Sent: Friday, October 31, 2014 1:04 AM > >> To: linux-usb@vger.kernel.org; linux-samsung-soc@vger.kernel.org > >> Cc: Marek Szyprowski; Kyungmin Park; Robert Baldyga; Paul Zimmerman; Krzysztof Kozlowski; Felipe > Balbi > >> Subject: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end session' irq > >> > >> This patch adds a call to s3c_hsotg_disconnect() from 'end session' > >> interrupt (GOTGINT_SES_END_DET) to correctly notify gadget subsystem > >> about unplugged usb cable. 'disconnected' interrupt (DISCONNINT) might > >> look a bit more suitable for this event, but it is asserted only in > >> host mode, so in device mode we need to use something else. > >> > >> Additional check has been added in s3c_hsotg_disconnect() function > >> to ensure that the event is reported only once after successful device > >> enumeration. > >> > >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > >> --- > >> drivers/usb/dwc2/core.h | 1 + > >> drivers/usb/dwc2/gadget.c | 10 ++++++++++ > >> 2 files changed, 11 insertions(+) > >> > >> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h > >> index 55c90c53f2d6..b42df32e7737 100644 > >> --- a/drivers/usb/dwc2/core.h > >> +++ b/drivers/usb/dwc2/core.h > >> @@ -212,6 +212,7 @@ struct s3c_hsotg { > >> struct usb_gadget gadget; > >> unsigned int setup; > >> unsigned long last_rst; > >> + unsigned int address; > >> struct s3c_hsotg_ep *eps; > >> }; > >> > >> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > >> index fcd2bb55ccca..6304efba11aa 100644 > >> --- a/drivers/usb/dwc2/gadget.c > >> +++ b/drivers/usb/dwc2/gadget.c > >> @@ -1114,6 +1114,7 @@ static void s3c_hsotg_process_control(struct s3c_hsotg *hsotg, > >> DCFG_DEVADDR_SHIFT) & DCFG_DEVADDR_MASK; > >> writel(dcfg, hsotg->regs + DCFG); > >> > >> + hsotg->address = ctrl->wValue; > >> dev_info(hsotg->dev, "new address %d\n", ctrl->wValue); > >> > >> ret = s3c_hsotg_send_reply(hsotg, ep0, NULL, 0); > >> @@ -2031,6 +2032,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg) > >> { > >> unsigned ep; > >> > >> + if (!hsotg->address) > >> + return; > >> + > >> + hsotg->address = 0; > >> for (ep = 0; ep < hsotg->num_of_eps; ep++) > >> kill_all_requests(hsotg, &hsotg->eps[ep], -ESHUTDOWN, true); > >> > >> @@ -2290,6 +2295,11 @@ irq_retry: > >> dev_info(hsotg->dev, "OTGInt: %08x\n", otgint); > >> > >> writel(otgint, hsotg->regs + GOTGINT); > >> + > >> + if (otgint & GOTGINT_SES_END_DET) { > >> + s3c_hsotg_disconnect(hsotg); > >> + hsotg->gadget.speed = USB_SPEED_UNKNOWN; > >> + } > >> } > >> > >> if (gintsts & GINTSTS_SESSREQINT) { > > I don't think this is right. The host can send control requests to > > the device before it sends a SetAddress to change from the default > > address of 0. So if a GOTGINT_SES_END_DET happens before the > > SetAddress, it will be ignored. > > > > Or am I missing something? > > Well, right. However before finishing enumeration (setting the address) > host usually > only retrieves some usb descriptors what doesn't change the state of the > gadget. > Right now we always reported 'disconnected' event before setting the new > address, > what is a bit overkill (in some cases gadget driver got this even more > than once). > The above code handles all cases correctly and reports disconnect event > only once. Well, if the disconnect happens before the SetAddress, the disconnect won't be reported at all. Unless I'm reading the code wrong. -- Paul
On 2014-11-13 21:50, Paul Zimmerman wrote: >> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com] >> Sent: Thursday, November 13, 2014 5:40 AM >> >> On 2014-10-31 19:15, Paul Zimmerman wrote: >>>> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com] >>>> Sent: Friday, October 31, 2014 1:04 AM >>>> To: linux-usb@vger.kernel.org; linux-samsung-soc@vger.kernel.org >>>> Cc: Marek Szyprowski; Kyungmin Park; Robert Baldyga; Paul Zimmerman; Krzysztof Kozlowski; Felipe >> Balbi >>>> Subject: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end session' irq >>>> >>>> This patch adds a call to s3c_hsotg_disconnect() from 'end session' >>>> interrupt (GOTGINT_SES_END_DET) to correctly notify gadget subsystem >>>> about unplugged usb cable. 'disconnected' interrupt (DISCONNINT) might >>>> look a bit more suitable for this event, but it is asserted only in >>>> host mode, so in device mode we need to use something else. >>>> >>>> Additional check has been added in s3c_hsotg_disconnect() function >>>> to ensure that the event is reported only once after successful device >>>> enumeration. >>>> >>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>>> --- >>>> drivers/usb/dwc2/core.h | 1 + >>>> drivers/usb/dwc2/gadget.c | 10 ++++++++++ >>>> 2 files changed, 11 insertions(+) >>>> >>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h >>>> index 55c90c53f2d6..b42df32e7737 100644 >>>> --- a/drivers/usb/dwc2/core.h >>>> +++ b/drivers/usb/dwc2/core.h >>>> @@ -212,6 +212,7 @@ struct s3c_hsotg { >>>> struct usb_gadget gadget; >>>> unsigned int setup; >>>> unsigned long last_rst; >>>> + unsigned int address; >>>> struct s3c_hsotg_ep *eps; >>>> }; >>>> >>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >>>> index fcd2bb55ccca..6304efba11aa 100644 >>>> --- a/drivers/usb/dwc2/gadget.c >>>> +++ b/drivers/usb/dwc2/gadget.c >>>> @@ -1114,6 +1114,7 @@ static void s3c_hsotg_process_control(struct s3c_hsotg *hsotg, >>>> DCFG_DEVADDR_SHIFT) & DCFG_DEVADDR_MASK; >>>> writel(dcfg, hsotg->regs + DCFG); >>>> >>>> + hsotg->address = ctrl->wValue; >>>> dev_info(hsotg->dev, "new address %d\n", ctrl->wValue); >>>> >>>> ret = s3c_hsotg_send_reply(hsotg, ep0, NULL, 0); >>>> @@ -2031,6 +2032,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg) >>>> { >>>> unsigned ep; >>>> >>>> + if (!hsotg->address) >>>> + return; >>>> + >>>> + hsotg->address = 0; >>>> for (ep = 0; ep < hsotg->num_of_eps; ep++) >>>> kill_all_requests(hsotg, &hsotg->eps[ep], -ESHUTDOWN, true); >>>> >>>> @@ -2290,6 +2295,11 @@ irq_retry: >>>> dev_info(hsotg->dev, "OTGInt: %08x\n", otgint); >>>> >>>> writel(otgint, hsotg->regs + GOTGINT); >>>> + >>>> + if (otgint & GOTGINT_SES_END_DET) { >>>> + s3c_hsotg_disconnect(hsotg); >>>> + hsotg->gadget.speed = USB_SPEED_UNKNOWN; >>>> + } >>>> } >>>> >>>> if (gintsts & GINTSTS_SESSREQINT) { >>> I don't think this is right. The host can send control requests to >>> the device before it sends a SetAddress to change from the default >>> address of 0. So if a GOTGINT_SES_END_DET happens before the >>> SetAddress, it will be ignored. >>> >>> Or am I missing something? >> Well, right. However before finishing enumeration (setting the address) >> host usually >> only retrieves some usb descriptors what doesn't change the state of the >> gadget. >> Right now we always reported 'disconnected' event before setting the new >> address, >> what is a bit overkill (in some cases gadget driver got this even more >> than once). >> The above code handles all cases correctly and reports disconnect event >> only once. > Well, if the disconnect happens before the SetAddress, the disconnect > won't be reported at all. Unless I'm reading the code wrong. Right, although this is not really an issue for any gadget driver. However if you prefer to report disconnect event more than once, I'm okay and I will remove the check based on the device address. Best regards
Hello, On 2014-11-13 21:50, Paul Zimmerman wrote: >> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com] >> Sent: Thursday, November 13, 2014 5:40 AM >> >> On 2014-10-31 19:15, Paul Zimmerman wrote: >>>> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com] >>>> Sent: Friday, October 31, 2014 1:04 AM >>>> To: linux-usb@vger.kernel.org; linux-samsung-soc@vger.kernel.org >>>> Cc: Marek Szyprowski; Kyungmin Park; Robert Baldyga; Paul Zimmerman; Krzysztof Kozlowski; Felipe >> Balbi >>>> Subject: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end session' irq >>>> >>>> This patch adds a call to s3c_hsotg_disconnect() from 'end session' >>>> interrupt (GOTGINT_SES_END_DET) to correctly notify gadget subsystem >>>> about unplugged usb cable. 'disconnected' interrupt (DISCONNINT) might >>>> look a bit more suitable for this event, but it is asserted only in >>>> host mode, so in device mode we need to use something else. >>>> >>>> Additional check has been added in s3c_hsotg_disconnect() function >>>> to ensure that the event is reported only once after successful device >>>> enumeration. >>>> >>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>>> --- >>>> drivers/usb/dwc2/core.h | 1 + >>>> drivers/usb/dwc2/gadget.c | 10 ++++++++++ >>>> 2 files changed, 11 insertions(+) >>>> >>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h >>>> index 55c90c53f2d6..b42df32e7737 100644 >>>> --- a/drivers/usb/dwc2/core.h >>>> +++ b/drivers/usb/dwc2/core.h >>>> @@ -212,6 +212,7 @@ struct s3c_hsotg { >>>> struct usb_gadget gadget; >>>> unsigned int setup; >>>> unsigned long last_rst; >>>> + unsigned int address; >>>> struct s3c_hsotg_ep *eps; >>>> }; >>>> >>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >>>> index fcd2bb55ccca..6304efba11aa 100644 >>>> --- a/drivers/usb/dwc2/gadget.c >>>> +++ b/drivers/usb/dwc2/gadget.c >>>> @@ -1114,6 +1114,7 @@ static void s3c_hsotg_process_control(struct s3c_hsotg *hsotg, >>>> DCFG_DEVADDR_SHIFT) & DCFG_DEVADDR_MASK; >>>> writel(dcfg, hsotg->regs + DCFG); >>>> >>>> + hsotg->address = ctrl->wValue; >>>> dev_info(hsotg->dev, "new address %d\n", ctrl->wValue); >>>> >>>> ret = s3c_hsotg_send_reply(hsotg, ep0, NULL, 0); >>>> @@ -2031,6 +2032,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg) >>>> { >>>> unsigned ep; >>>> >>>> + if (!hsotg->address) >>>> + return; >>>> + >>>> + hsotg->address = 0; >>>> for (ep = 0; ep < hsotg->num_of_eps; ep++) >>>> kill_all_requests(hsotg, &hsotg->eps[ep], -ESHUTDOWN, true); >>>> >>>> @@ -2290,6 +2295,11 @@ irq_retry: >>>> dev_info(hsotg->dev, "OTGInt: %08x\n", otgint); >>>> >>>> writel(otgint, hsotg->regs + GOTGINT); >>>> + >>>> + if (otgint & GOTGINT_SES_END_DET) { >>>> + s3c_hsotg_disconnect(hsotg); >>>> + hsotg->gadget.speed = USB_SPEED_UNKNOWN; >>>> + } >>>> } >>>> >>>> if (gintsts & GINTSTS_SESSREQINT) { >>> I don't think this is right. The host can send control requests to >>> the device before it sends a SetAddress to change from the default >>> address of 0. So if a GOTGINT_SES_END_DET happens before the >>> SetAddress, it will be ignored. >>> >>> Or am I missing something? >> Well, right. However before finishing enumeration (setting the address) >> host usually >> only retrieves some usb descriptors what doesn't change the state of the >> gadget. >> Right now we always reported 'disconnected' event before setting the new >> address, >> what is a bit overkill (in some cases gadget driver got this even more >> than once). >> The above code handles all cases correctly and reports disconnect event >> only once. > Well, if the disconnect happens before the SetAddress, the disconnect > won't be reported at all. Unless I'm reading the code wrong. Ok, I found other way to ensure that disconnect event is reported only once. I will post a patch in a few minutes. Best regards
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index 55c90c53f2d6..b42df32e7737 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -212,6 +212,7 @@ struct s3c_hsotg { struct usb_gadget gadget; unsigned int setup; unsigned long last_rst; + unsigned int address; struct s3c_hsotg_ep *eps; }; diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index fcd2bb55ccca..6304efba11aa 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -1114,6 +1114,7 @@ static void s3c_hsotg_process_control(struct s3c_hsotg *hsotg, DCFG_DEVADDR_SHIFT) & DCFG_DEVADDR_MASK; writel(dcfg, hsotg->regs + DCFG); + hsotg->address = ctrl->wValue; dev_info(hsotg->dev, "new address %d\n", ctrl->wValue); ret = s3c_hsotg_send_reply(hsotg, ep0, NULL, 0); @@ -2031,6 +2032,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg) { unsigned ep; + if (!hsotg->address) + return; + + hsotg->address = 0; for (ep = 0; ep < hsotg->num_of_eps; ep++) kill_all_requests(hsotg, &hsotg->eps[ep], -ESHUTDOWN, true); @@ -2290,6 +2295,11 @@ irq_retry: dev_info(hsotg->dev, "OTGInt: %08x\n", otgint); writel(otgint, hsotg->regs + GOTGINT); + + if (otgint & GOTGINT_SES_END_DET) { + s3c_hsotg_disconnect(hsotg); + hsotg->gadget.speed = USB_SPEED_UNKNOWN; + } } if (gintsts & GINTSTS_SESSREQINT) {
This patch adds a call to s3c_hsotg_disconnect() from 'end session' interrupt (GOTGINT_SES_END_DET) to correctly notify gadget subsystem about unplugged usb cable. 'disconnected' interrupt (DISCONNINT) might look a bit more suitable for this event, but it is asserted only in host mode, so in device mode we need to use something else. Additional check has been added in s3c_hsotg_disconnect() function to ensure that the event is reported only once after successful device enumeration. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/usb/dwc2/core.h | 1 + drivers/usb/dwc2/gadget.c | 10 ++++++++++ 2 files changed, 11 insertions(+)