[PATCH v2 00/10] more dwc2/gadget fixes

[PATCH v2 00/10] more dwc2/gadget fixes

[Marek Szyprowski]

Hi!

This patchset contains a set of fixes to solve vaious minor issues
related to cable connect/disconnect events, pull-up control,
soft-disconnect mode, proper usb phy operation and restoring gadget
state after suspend/resume cycle.

Changes since v1 (http://www.spinics.net/lists/linux-samsung-soc/msg37842.html):
- fixed issues spotted by Felipe Balbi

Best regards
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Marek Szyprowski (10):
  usb: dwc2/gadget: report disconnect event from 'end session' irq
  usb: dwc2/gadget: fix enumeration issues
  usb: dwc2/gadget: fix gadget unregistration in udc_stop() function
  usb: dwc2/gadget: disable phy before turning off power regulators
  usb: dwc2/gadget: move setting last reset time to s3c_hsotg_core_init
  usb: dwc2/gadget: decouple setting soft-disconnect from
    s3c_hsotg_core_init
  usb: dwc2/gadget: move phy control calls out of pullup() method
  usb: dwc2/gadget: use soft-disconnect udc feature in pullup() method
  usb: dwc2/gadget: fix calls to phy control functions in suspend/resume
    code
  usb: dwc2/gadget: rework suspend/resume code to correctly restore
    gadget state

 drivers/usb/dwc2/core.h   |  4 +-
 drivers/usb/dwc2/gadget.c | 95 ++++++++++++++++++++++++++++++++---------------
 2 files changed, 69 insertions(+), 30 deletions(-)

-- 
1.9.2

[PATCH v2 01/10] usb: dwc2/gadget: report disconnect event from 'end session' irq

[Marek Szyprowski]

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.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/usb/dwc2/gadget.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 7b5856fadd93..119c8a3effc2 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2279,6 +2279,12 @@ irq_retry:
 		dev_info(hsotg->dev, "OTGInt: %08x\n", otgint);
 
 		writel(otgint, hsotg->regs + GOTGINT);
+
+		if (otgint & GOTGINT_SES_END_DET) {
+			if (hsotg->gadget.speed != USB_SPEED_UNKNOWN)
+				s3c_hsotg_disconnect(hsotg);
+			hsotg->gadget.speed = USB_SPEED_UNKNOWN;
+		}
 	}
 
 	if (gintsts & GINTSTS_SESSREQINT) {
-- 
1.9.2

RE: [PATCH v2 01/10] usb: dwc2/gadget: report disconnect event from 'end session' irq

[Paul Zimmerman]

> From: Marek Szyprowski [mailto:m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org]
> Sent: Monday, October 20, 2014 3:46 AM
> 
> 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.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/usb/dwc2/gadget.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 7b5856fadd93..119c8a3effc2 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -2279,6 +2279,12 @@ irq_retry:
>  		dev_info(hsotg->dev, "OTGInt: %08x\n", otgint);
> 
>  		writel(otgint, hsotg->regs + GOTGINT);
> +
> +		if (otgint & GOTGINT_SES_END_DET) {
> +			if (hsotg->gadget.speed != USB_SPEED_UNKNOWN)
> +				s3c_hsotg_disconnect(hsotg);
> +			hsotg->gadget.speed = USB_SPEED_UNKNOWN;
> +		}
>  	}
> 
>  	if (gintsts & GINTSTS_SESSREQINT) {

Does this mean we can get rid of the call to s3c_hsotg_disconnect in
s3c_hsotg_process_control after a SET_ADDRESS is received? If not,
then s3c_hsotg_disconnect will be called twice, once here after the
disconnect, and once again after the reconnect and SET_ADDRESS.

-- 
Paul

--
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 v2 01/10] usb: dwc2/gadget: report disconnect event from 'end session' irq

[Marek Szyprowski]

Hello,

On 2014-10-25 03:23, Paul Zimmerman wrote:
>> From: Marek Szyprowski [mailto:m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org]
>> Sent: Monday, October 20, 2014 3:46 AM
>>
>> 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.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> ---
>>   drivers/usb/dwc2/gadget.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 7b5856fadd93..119c8a3effc2 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -2279,6 +2279,12 @@ irq_retry:
>>   		dev_info(hsotg->dev, "OTGInt: %08x\n", otgint);
>>
>>   		writel(otgint, hsotg->regs + GOTGINT);
>> +
>> +		if (otgint & GOTGINT_SES_END_DET) {
>> +			if (hsotg->gadget.speed != USB_SPEED_UNKNOWN)
>> +				s3c_hsotg_disconnect(hsotg);
>> +			hsotg->gadget.speed = USB_SPEED_UNKNOWN;
>> +		}
>>   	}
>>
>>   	if (gintsts & GINTSTS_SESSREQINT) {
> Does this mean we can get rid of the call to s3c_hsotg_disconnect in
> s3c_hsotg_process_control after a SET_ADDRESS is received? If not,
> then s3c_hsotg_disconnect will be called twice, once here after the
> disconnect, and once again after the reconnect and SET_ADDRESS.

Nope, we cannot get rid of that call, because it is needed got get gadget
operational when device has been re-enumerated. The simplest way to 
reproduce
such case is to connect dwc2 to externally powered hub and then unplug cable
between hub and usb host. In such case the 'end session' interrupt is not
asserted, so the additional s3c_hsotg_disconnect() call before setting new
address is needed. I will rework this patch and add a code to ensures that
the gadget->disconnect() will be called only once.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

[PATCH v3] usb: dwc2/gadget: report disconnect event from 'end session' irq

[Marek Szyprowski]

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) {
-- 
1.9.2

RE: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end session' irq

[Paul Zimmerman]

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

-- 
Paul

Re: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end session' irq

[Marek Szyprowski]

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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

RE: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end session' irq

[Paul Zimmerman]

> 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

Re: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end session' irq

[Marek Szyprowski]

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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end session' irq

[Marek Szyprowski]

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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[PATCH v4] usb: dwc2/gadget: rework disconnect event handling

[Marek Szyprowski]

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. DISCONNINT interrupt cannot be used for this
purpose, because it is asserted only in host mode.

To avoid reporting disconnect event more than once, a disconnect call has
been moved from USB_REQ_SET_ADDRESS handling function to SESSREQINT
interrupt. This way driver ensures that disconnect event is reported
either when usb cable is unplugged or every time the host starts a new
session.

Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/dwc2/core.h   |  1 +
 drivers/usb/dwc2/gadget.c | 13 +++++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 55c90c53f2d6..78b9090ebf71 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -210,6 +210,7 @@ struct s3c_hsotg {
 	u8                      ctrl_buff[8];
 
 	struct usb_gadget       gadget;
+	unsigned int		session:1;
 	unsigned int            setup;
 	unsigned long           last_rst;
 	struct s3c_hsotg_ep     *eps;
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index fcd2bb55ccca..c7f68dc1cf6b 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -1029,7 +1029,6 @@ static int s3c_hsotg_process_req_feature(struct s3c_hsotg *hsotg,
 }
 
 static void s3c_hsotg_enqueue_setup(struct s3c_hsotg *hsotg);
-static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg);
 
 /**
  * s3c_hsotg_stall_ep0 - stall ep0
@@ -1107,7 +1106,6 @@ static void s3c_hsotg_process_control(struct s3c_hsotg *hsotg,
 	if ((ctrl->bRequestType & USB_TYPE_MASK) == USB_TYPE_STANDARD) {
 		switch (ctrl->bRequest) {
 		case USB_REQ_SET_ADDRESS:
-			s3c_hsotg_disconnect(hsotg);
 			dcfg = readl(hsotg->regs + DCFG);
 			dcfg &= ~DCFG_DEVADDR_MASK;
 			dcfg |= (le16_to_cpu(ctrl->wValue) <<
@@ -2031,6 +2029,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg)
 {
 	unsigned ep;
 
+	if (!hsotg->session)
+		return;
+
+	hsotg->session = 0;
 	for (ep = 0; ep < hsotg->num_of_eps; ep++)
 		kill_all_requests(hsotg, &hsotg->eps[ep], -ESHUTDOWN, true);
 
@@ -2290,11 +2292,18 @@ 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) {
 		dev_dbg(hsotg->dev, "%s: SessReqInt\n", __func__);
 		writel(GINTSTS_SESSREQINT, hsotg->regs + GINTSTS);
+		s3c_hsotg_disconnect(hsotg);
+		hsotg->session = 1;
 	}
 
 	if (gintsts & GINTSTS_ENUMDONE) {
-- 
1.9.2

--
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 v4] usb: dwc2/gadget: rework disconnect event handling

[Paul Zimmerman]

> -----Original Message-----
> From: Marek Szyprowski [mailto:m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org]
> Sent: Friday, November 14, 2014 4:20 AM
> 
> 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. DISCONNINT interrupt cannot be used for this
> purpose, because it is asserted only in host mode.
> 
> To avoid reporting disconnect event more than once, a disconnect call has
> been moved from USB_REQ_SET_ADDRESS handling function to SESSREQINT
> interrupt. This way driver ensures that disconnect event is reported
> either when usb cable is unplugged or every time the host starts a new
> session.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/usb/dwc2/core.h   |  1 +
>  drivers/usb/dwc2/gadget.c | 13 +++++++++++--
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 55c90c53f2d6..78b9090ebf71 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -210,6 +210,7 @@ struct s3c_hsotg {
>  	u8                      ctrl_buff[8];
> 
>  	struct usb_gadget       gadget;
> +	unsigned int		session:1;
>  	unsigned int            setup;
>  	unsigned long           last_rst;
>  	struct s3c_hsotg_ep     *eps;
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index fcd2bb55ccca..c7f68dc1cf6b 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -1029,7 +1029,6 @@ static int s3c_hsotg_process_req_feature(struct s3c_hsotg *hsotg,
>  }
> 
>  static void s3c_hsotg_enqueue_setup(struct s3c_hsotg *hsotg);
> -static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg);
> 
>  /**
>   * s3c_hsotg_stall_ep0 - stall ep0
> @@ -1107,7 +1106,6 @@ static void s3c_hsotg_process_control(struct s3c_hsotg *hsotg,
>  	if ((ctrl->bRequestType & USB_TYPE_MASK) == USB_TYPE_STANDARD) {
>  		switch (ctrl->bRequest) {
>  		case USB_REQ_SET_ADDRESS:
> -			s3c_hsotg_disconnect(hsotg);
>  			dcfg = readl(hsotg->regs + DCFG);
>  			dcfg &= ~DCFG_DEVADDR_MASK;
>  			dcfg |= (le16_to_cpu(ctrl->wValue) <<
> @@ -2031,6 +2029,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg)
>  {
>  	unsigned ep;
> 
> +	if (!hsotg->session)
> +		return;
> +
> +	hsotg->session = 0;
>  	for (ep = 0; ep < hsotg->num_of_eps; ep++)
>  		kill_all_requests(hsotg, &hsotg->eps[ep], -ESHUTDOWN, true);
> 
> @@ -2290,11 +2292,18 @@ 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);

I think you should clear hsotg->session here, shouldn't you?
Otherwise I think s3c_hsotg_disconnect() will be called twice, once
here and once when the next GINTSTS_SESSREQINT comes.

-- 
Paul

> +			hsotg->gadget.speed = USB_SPEED_UNKNOWN;
> +		}
>  	}
> 
>  	if (gintsts & GINTSTS_SESSREQINT) {
>  		dev_dbg(hsotg->dev, "%s: SessReqInt\n", __func__);
>  		writel(GINTSTS_SESSREQINT, hsotg->regs + GINTSTS);
> +		s3c_hsotg_disconnect(hsotg);
> +		hsotg->session = 1;
>  	}
> 
>  	if (gintsts & GINTSTS_ENUMDONE) {
> --
> 1.9.2

--
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 v4] usb: dwc2/gadget: rework disconnect event handling

[Felipe Balbi]

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

On Fri, Nov 14, 2014 at 07:01:37PM +0000, Paul Zimmerman wrote:
> > -----Original Message-----
> > From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
> > Sent: Friday, November 14, 2014 4:20 AM
> > 
> > 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. DISCONNINT interrupt cannot be used for this
> > purpose, because it is asserted only in host mode.
> > 
> > To avoid reporting disconnect event more than once, a disconnect call has
> > been moved from USB_REQ_SET_ADDRESS handling function to SESSREQINT
> > interrupt. This way driver ensures that disconnect event is reported
> > either when usb cable is unplugged or every time the host starts a new
> > session.
> > 
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > ---
> >  drivers/usb/dwc2/core.h   |  1 +
> >  drivers/usb/dwc2/gadget.c | 13 +++++++++++--
> >  2 files changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> > index 55c90c53f2d6..78b9090ebf71 100644
> > --- a/drivers/usb/dwc2/core.h
> > +++ b/drivers/usb/dwc2/core.h
> > @@ -210,6 +210,7 @@ struct s3c_hsotg {
> >  	u8                      ctrl_buff[8];
> > 
> >  	struct usb_gadget       gadget;
> > +	unsigned int		session:1;
> >  	unsigned int            setup;
> >  	unsigned long           last_rst;
> >  	struct s3c_hsotg_ep     *eps;
> > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> > index fcd2bb55ccca..c7f68dc1cf6b 100644
> > --- a/drivers/usb/dwc2/gadget.c
> > +++ b/drivers/usb/dwc2/gadget.c
> > @@ -1029,7 +1029,6 @@ static int s3c_hsotg_process_req_feature(struct s3c_hsotg *hsotg,
> >  }
> > 
> >  static void s3c_hsotg_enqueue_setup(struct s3c_hsotg *hsotg);
> > -static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg);
> > 
> >  /**
> >   * s3c_hsotg_stall_ep0 - stall ep0
> > @@ -1107,7 +1106,6 @@ static void s3c_hsotg_process_control(struct s3c_hsotg *hsotg,
> >  	if ((ctrl->bRequestType & USB_TYPE_MASK) == USB_TYPE_STANDARD) {
> >  		switch (ctrl->bRequest) {
> >  		case USB_REQ_SET_ADDRESS:
> > -			s3c_hsotg_disconnect(hsotg);
> >  			dcfg = readl(hsotg->regs + DCFG);
> >  			dcfg &= ~DCFG_DEVADDR_MASK;
> >  			dcfg |= (le16_to_cpu(ctrl->wValue) <<
> > @@ -2031,6 +2029,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg)
> >  {
> >  	unsigned ep;
> > 
> > +	if (!hsotg->session)
> > +		return;
> > +
> > +	hsotg->session = 0;
> >  	for (ep = 0; ep < hsotg->num_of_eps; ep++)
> >  		kill_all_requests(hsotg, &hsotg->eps[ep], -ESHUTDOWN, true);
> > 
> > @@ -2290,11 +2292,18 @@ 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);
> 
> I think you should clear hsotg->session here, shouldn't you?
> Otherwise I think s3c_hsotg_disconnect() will be called twice, once
> here and once when the next GINTSTS_SESSREQINT comes.

the best way to avoid that would be fiddle with hsotg->session inside
s3c_hsotg_disconnect() only.

-- 
balbi

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

RE: [PATCH v4] usb: dwc2/gadget: rework disconnect event handling

[Paul Zimmerman]

> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-owner@vger.kernel.org] On Behalf Of Felipe Balbi
> Sent: Friday, November 14, 2014 11:05 AM
> 
> On Fri, Nov 14, 2014 at 07:01:37PM +0000, Paul Zimmerman wrote:
> > > -----Original Message-----
> > > From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
> > > Sent: Friday, November 14, 2014 4:20 AM
> > >
> > > 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. DISCONNINT interrupt cannot be used for this
> > > purpose, because it is asserted only in host mode.
> > >
> > > To avoid reporting disconnect event more than once, a disconnect call has
> > > been moved from USB_REQ_SET_ADDRESS handling function to SESSREQINT
> > > interrupt. This way driver ensures that disconnect event is reported
> > > either when usb cable is unplugged or every time the host starts a new
> > > session.
> > >
> > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > > ---
> > >  drivers/usb/dwc2/core.h   |  1 +
> > >  drivers/usb/dwc2/gadget.c | 13 +++++++++++--
> > >  2 files changed, 12 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> > > index 55c90c53f2d6..78b9090ebf71 100644
> > > --- a/drivers/usb/dwc2/core.h
> > > +++ b/drivers/usb/dwc2/core.h
> > > @@ -210,6 +210,7 @@ struct s3c_hsotg {
> > >  	u8                      ctrl_buff[8];
> > >
> > >  	struct usb_gadget       gadget;
> > > +	unsigned int		session:1;
> > >  	unsigned int            setup;
> > >  	unsigned long           last_rst;
> > >  	struct s3c_hsotg_ep     *eps;
> > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> > > index fcd2bb55ccca..c7f68dc1cf6b 100644
> > > --- a/drivers/usb/dwc2/gadget.c
> > > +++ b/drivers/usb/dwc2/gadget.c
> > > @@ -1029,7 +1029,6 @@ static int s3c_hsotg_process_req_feature(struct s3c_hsotg *hsotg,
> > >  }
> > >
> > >  static void s3c_hsotg_enqueue_setup(struct s3c_hsotg *hsotg);
> > > -static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg);
> > >
> > >  /**
> > >   * s3c_hsotg_stall_ep0 - stall ep0
> > > @@ -1107,7 +1106,6 @@ static void s3c_hsotg_process_control(struct s3c_hsotg *hsotg,
> > >  	if ((ctrl->bRequestType & USB_TYPE_MASK) == USB_TYPE_STANDARD) {
> > >  		switch (ctrl->bRequest) {
> > >  		case USB_REQ_SET_ADDRESS:
> > > -			s3c_hsotg_disconnect(hsotg);
> > >  			dcfg = readl(hsotg->regs + DCFG);
> > >  			dcfg &= ~DCFG_DEVADDR_MASK;
> > >  			dcfg |= (le16_to_cpu(ctrl->wValue) <<
> > > @@ -2031,6 +2029,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg)
> > >  {
> > >  	unsigned ep;
> > >
> > > +	if (!hsotg->session)
> > > +		return;
> > > +
> > > +	hsotg->session = 0;
> > >  	for (ep = 0; ep < hsotg->num_of_eps; ep++)
> > >  		kill_all_requests(hsotg, &hsotg->eps[ep], -ESHUTDOWN, true);
> > >
> > > @@ -2290,11 +2292,18 @@ 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);
> >
> > I think you should clear hsotg->session here, shouldn't you?
> > Otherwise I think s3c_hsotg_disconnect() will be called twice, once
> > here and once when the next GINTSTS_SESSREQINT comes.
> 
> the best way to avoid that would be fiddle with hsotg->session inside
> s3c_hsotg_disconnect() only.

Whoops, I just noticed that hsotg->session *is* cleared inside of
s3c_hsotg_disconnect(). So I think the patch is good as-is.

Acked-by: Paul Zimmerman <paulz@synopsys.com>

RE: [PATCH v4] usb: dwc2/gadget: rework disconnect event handling

[Paul Zimmerman]

> From: Paul Zimmerman
> Sent: Friday, November 14, 2014 1:21 PM
> 
> > From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-owner@vger.kernel.org] On Behalf Of Felipe Balbi
> > Sent: Friday, November 14, 2014 11:05 AM
> >
> > On Fri, Nov 14, 2014 at 07:01:37PM +0000, Paul Zimmerman wrote:
> > > > -----Original Message-----
> > > > From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
> > > > Sent: Friday, November 14, 2014 4:20 AM
> > > >
> > > > 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. DISCONNINT interrupt cannot be used for this
> > > > purpose, because it is asserted only in host mode.
> > > >
> > > > To avoid reporting disconnect event more than once, a disconnect call has
> > > > been moved from USB_REQ_SET_ADDRESS handling function to SESSREQINT
> > > > interrupt. This way driver ensures that disconnect event is reported
> > > > either when usb cable is unplugged or every time the host starts a new
> > > > session.
> > > >
> > > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > > > ---
> > > >  drivers/usb/dwc2/core.h   |  1 +
> > > >  drivers/usb/dwc2/gadget.c | 13 +++++++++++--
> > > >  2 files changed, 12 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> > > > index 55c90c53f2d6..78b9090ebf71 100644
> > > > --- a/drivers/usb/dwc2/core.h
> > > > +++ b/drivers/usb/dwc2/core.h
> > > > @@ -210,6 +210,7 @@ struct s3c_hsotg {
> > > >  	u8                      ctrl_buff[8];
> > > >
> > > >  	struct usb_gadget       gadget;
> > > > +	unsigned int		session:1;
> > > >  	unsigned int            setup;
> > > >  	unsigned long           last_rst;
> > > >  	struct s3c_hsotg_ep     *eps;
> > > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> > > > index fcd2bb55ccca..c7f68dc1cf6b 100644
> > > > --- a/drivers/usb/dwc2/gadget.c
> > > > +++ b/drivers/usb/dwc2/gadget.c
> > > > @@ -1029,7 +1029,6 @@ static int s3c_hsotg_process_req_feature(struct s3c_hsotg *hsotg,
> > > >  }
> > > >
> > > >  static void s3c_hsotg_enqueue_setup(struct s3c_hsotg *hsotg);
> > > > -static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg);
> > > >
> > > >  /**
> > > >   * s3c_hsotg_stall_ep0 - stall ep0
> > > > @@ -1107,7 +1106,6 @@ static void s3c_hsotg_process_control(struct s3c_hsotg *hsotg,
> > > >  	if ((ctrl->bRequestType & USB_TYPE_MASK) == USB_TYPE_STANDARD) {
> > > >  		switch (ctrl->bRequest) {
> > > >  		case USB_REQ_SET_ADDRESS:
> > > > -			s3c_hsotg_disconnect(hsotg);
> > > >  			dcfg = readl(hsotg->regs + DCFG);
> > > >  			dcfg &= ~DCFG_DEVADDR_MASK;
> > > >  			dcfg |= (le16_to_cpu(ctrl->wValue) <<
> > > > @@ -2031,6 +2029,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg)
> > > >  {
> > > >  	unsigned ep;
> > > >
> > > > +	if (!hsotg->session)
> > > > +		return;
> > > > +
> > > > +	hsotg->session = 0;
> > > >  	for (ep = 0; ep < hsotg->num_of_eps; ep++)
> > > >  		kill_all_requests(hsotg, &hsotg->eps[ep], -ESHUTDOWN, true);
> > > >
> > > > @@ -2290,11 +2292,18 @@ 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);
> > >
> > > I think you should clear hsotg->session here, shouldn't you?
> > > Otherwise I think s3c_hsotg_disconnect() will be called twice, once
> > > here and once when the next GINTSTS_SESSREQINT comes.
> >
> > the best way to avoid that would be fiddle with hsotg->session inside
> > s3c_hsotg_disconnect() only.
> 
> Whoops, I just noticed that hsotg->session *is* cleared inside of
> s3c_hsotg_disconnect(). So I think the patch is good as-is.
> 
> Acked-by: Paul Zimmerman <paulz@synopsys.com>

I'm having second thoughts about this. Currently, the
GINTSTS_SESSREQINT interrupt in the gadget does nothing except print
a debug message. So I'm not sure that all versions of the controller
actually assert this interrupt when a connection is made. If they
don't, then this patch would break the disconnect handling, since
hsotg->session would never get set.

In particular, I'm thinking that a core which is synthesized without
SRP support may not implement the GINTSTS_SESSREQINT interrupt. The
databook seems to imply that:

"SessReqInt: In Device mode, this interrupt is asserted when the
utmisrp_bvalid signal goes high."

And in the section which describes the utmisrp_bvalid signal, there is
a note that says:

"This interface is present only when parameter OTG_MODE specifies an
SRP-capable configuration."

So Marek, can you try moving the line which sets hsotg->session = 1
into s3c_hsotg_irq_enumdone() instead? Then we will be sure it gets set
to one after a connect. Probably it should be renamed from 'session'
to 'connect' in that case.

-- 
Paul

Re: [PATCH v4] usb: dwc2/gadget: rework disconnect event handling

[Marek Szyprowski]

Hello,

On 2014-11-14 23:09, Paul Zimmerman wrote:
>> From: Paul Zimmerman
>> Sent: Friday, November 14, 2014 1:21 PM
>>
>>> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-owner@vger.kernel.org] On Behalf Of Felipe Balbi
>>> Sent: Friday, November 14, 2014 11:05 AM
>>>
>>> On Fri, Nov 14, 2014 at 07:01:37PM +0000, Paul Zimmerman wrote:
>>>>> -----Original Message-----
>>>>> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
>>>>> Sent: Friday, November 14, 2014 4:20 AM
>>>>>
>>>>> 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. DISCONNINT interrupt cannot be used for this
>>>>> purpose, because it is asserted only in host mode.
>>>>>
>>>>> To avoid reporting disconnect event more than once, a disconnect call has
>>>>> been moved from USB_REQ_SET_ADDRESS handling function to SESSREQINT
>>>>> interrupt. This way driver ensures that disconnect event is reported
>>>>> either when usb cable is unplugged or every time the host starts a new
>>>>> session.
>>>>>
>>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>>> ---
>>>>>   drivers/usb/dwc2/core.h   |  1 +
>>>>>   drivers/usb/dwc2/gadget.c | 13 +++++++++++--
>>>>>   2 files changed, 12 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>>>>> index 55c90c53f2d6..78b9090ebf71 100644
>>>>> --- a/drivers/usb/dwc2/core.h
>>>>> +++ b/drivers/usb/dwc2/core.h
>>>>> @@ -210,6 +210,7 @@ struct s3c_hsotg {
>>>>>   	u8                      ctrl_buff[8];
>>>>>
>>>>>   	struct usb_gadget       gadget;
>>>>> +	unsigned int		session:1;
>>>>>   	unsigned int            setup;
>>>>>   	unsigned long           last_rst;
>>>>>   	struct s3c_hsotg_ep     *eps;
>>>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>>>>> index fcd2bb55ccca..c7f68dc1cf6b 100644
>>>>> --- a/drivers/usb/dwc2/gadget.c
>>>>> +++ b/drivers/usb/dwc2/gadget.c
>>>>> @@ -1029,7 +1029,6 @@ static int s3c_hsotg_process_req_feature(struct s3c_hsotg *hsotg,
>>>>>   }
>>>>>
>>>>>   static void s3c_hsotg_enqueue_setup(struct s3c_hsotg *hsotg);
>>>>> -static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg);
>>>>>
>>>>>   /**
>>>>>    * s3c_hsotg_stall_ep0 - stall ep0
>>>>> @@ -1107,7 +1106,6 @@ static void s3c_hsotg_process_control(struct s3c_hsotg *hsotg,
>>>>>   	if ((ctrl->bRequestType & USB_TYPE_MASK) == USB_TYPE_STANDARD) {
>>>>>   		switch (ctrl->bRequest) {
>>>>>   		case USB_REQ_SET_ADDRESS:
>>>>> -			s3c_hsotg_disconnect(hsotg);
>>>>>   			dcfg = readl(hsotg->regs + DCFG);
>>>>>   			dcfg &= ~DCFG_DEVADDR_MASK;
>>>>>   			dcfg |= (le16_to_cpu(ctrl->wValue) <<
>>>>> @@ -2031,6 +2029,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg)
>>>>>   {
>>>>>   	unsigned ep;
>>>>>
>>>>> +	if (!hsotg->session)
>>>>> +		return;
>>>>> +
>>>>> +	hsotg->session = 0;
>>>>>   	for (ep = 0; ep < hsotg->num_of_eps; ep++)
>>>>>   		kill_all_requests(hsotg, &hsotg->eps[ep], -ESHUTDOWN, true);
>>>>>
>>>>> @@ -2290,11 +2292,18 @@ 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);
>>>> I think you should clear hsotg->session here, shouldn't you?
>>>> Otherwise I think s3c_hsotg_disconnect() will be called twice, once
>>>> here and once when the next GINTSTS_SESSREQINT comes.
>>> the best way to avoid that would be fiddle with hsotg->session inside
>>> s3c_hsotg_disconnect() only.
>> Whoops, I just noticed that hsotg->session *is* cleared inside of
>> s3c_hsotg_disconnect(). So I think the patch is good as-is.
>>
>> Acked-by: Paul Zimmerman <paulz@synopsys.com>
> I'm having second thoughts about this. Currently, the
> GINTSTS_SESSREQINT interrupt in the gadget does nothing except print
> a debug message. So I'm not sure that all versions of the controller
> actually assert this interrupt when a connection is made. If they
> don't, then this patch would break the disconnect handling, since
> hsotg->session would never get set.
>
> In particular, I'm thinking that a core which is synthesized without
> SRP support may not implement the GINTSTS_SESSREQINT interrupt. The
> databook seems to imply that:
>
> "SessReqInt: In Device mode, this interrupt is asserted when the
> utmisrp_bvalid signal goes high."
>
> And in the section which describes the utmisrp_bvalid signal, there is
> a note that says:
>
> "This interface is present only when parameter OTG_MODE specifies an
> SRP-capable configuration."
>
> So Marek, can you try moving the line which sets hsotg->session = 1
> into s3c_hsotg_irq_enumdone() instead? Then we will be sure it gets set
> to one after a connect. Probably it should be renamed from 'session'
> to 'connect' in that case.

Well... ok, but this will work exactly the same way as v3, which you were
not keen of. I think the best way will be to keep it set both in SESSREQINT
and enumdone, I will send a patch in a few minutes.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[PATCH v5] usb: dwc2/gadget: rework disconnect event handling

[Marek Szyprowski]

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. DISCONNINT interrupt cannot be used for this
purpose, because it is asserted only in host mode.

To avoid reporting disconnect event more than once, a disconnect call has
been moved from USB_REQ_SET_ADDRESS handling function to SESSREQINT
interrupt. This way driver ensures that disconnect event is reported
either when usb cable is unplugged or every time the host starts a new
session. To handle devices which has been synthesized without
SRP support, connected state is set in ENUMDONE interrupt.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/usb/dwc2/core.h   |  1 +
 drivers/usb/dwc2/gadget.c | 16 ++++++++++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 55c90c53f2d6..e54c3c50cd48 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -210,6 +210,7 @@ struct s3c_hsotg {
 	u8                      ctrl_buff[8];
 
 	struct usb_gadget       gadget;
+	unsigned int		connected:1;
 	unsigned int            setup;
 	unsigned long           last_rst;
 	struct s3c_hsotg_ep     *eps;
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index fcd2bb55ccca..89b1bea50ee3 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -1029,7 +1029,6 @@ static int s3c_hsotg_process_req_feature(struct s3c_hsotg *hsotg,
 }
 
 static void s3c_hsotg_enqueue_setup(struct s3c_hsotg *hsotg);
-static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg);
 
 /**
  * s3c_hsotg_stall_ep0 - stall ep0
@@ -1107,7 +1106,6 @@ static void s3c_hsotg_process_control(struct s3c_hsotg *hsotg,
 	if ((ctrl->bRequestType & USB_TYPE_MASK) == USB_TYPE_STANDARD) {
 		switch (ctrl->bRequest) {
 		case USB_REQ_SET_ADDRESS:
-			s3c_hsotg_disconnect(hsotg);
 			dcfg = readl(hsotg->regs + DCFG);
 			dcfg &= ~DCFG_DEVADDR_MASK;
 			dcfg |= (le16_to_cpu(ctrl->wValue) <<
@@ -2031,6 +2029,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg)
 {
 	unsigned ep;
 
+	if (!hsotg->connected)
+		return;
+
+	hsotg->connected = 0;
 	for (ep = 0; ep < hsotg->num_of_eps; ep++)
 		kill_all_requests(hsotg, &hsotg->eps[ep], -ESHUTDOWN, true);
 
@@ -2290,17 +2292,27 @@ 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) {
 		dev_dbg(hsotg->dev, "%s: SessReqInt\n", __func__);
 		writel(GINTSTS_SESSREQINT, hsotg->regs + GINTSTS);
+		/*
+		 * Report disconnect if there is any previous session established
+		 */
+		s3c_hsotg_disconnect(hsotg);
 	}
 
 	if (gintsts & GINTSTS_ENUMDONE) {
 		writel(GINTSTS_ENUMDONE, hsotg->regs + GINTSTS);
 
 		s3c_hsotg_irq_enumdone(hsotg);
+		hsotg->connected = 1;
 	}
 
 	if (gintsts & GINTSTS_CONIDSTSCHNG) {
-- 
1.9.2

RE: [PATCH v5] usb: dwc2/gadget: rework disconnect event handling

[Paul Zimmerman]

> From: Marek Szyprowski [mailto:m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org]
> Sent: Monday, November 17, 2014 1:00 AM
> 
> 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. DISCONNINT interrupt cannot be used for this
> purpose, because it is asserted only in host mode.
> 
> To avoid reporting disconnect event more than once, a disconnect call has
> been moved from USB_REQ_SET_ADDRESS handling function to SESSREQINT
> interrupt. This way driver ensures that disconnect event is reported
> either when usb cable is unplugged or every time the host starts a new
> session. To handle devices which has been synthesized without
> SRP support, connected state is set in ENUMDONE interrupt.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/usb/dwc2/core.h   |  1 +
>  drivers/usb/dwc2/gadget.c | 16 ++++++++++++++--
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 55c90c53f2d6..e54c3c50cd48 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -210,6 +210,7 @@ struct s3c_hsotg {
>  	u8                      ctrl_buff[8];
> 
>  	struct usb_gadget       gadget;
> +	unsigned int		connected:1;
>  	unsigned int            setup;
>  	unsigned long           last_rst;
>  	struct s3c_hsotg_ep     *eps;
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index fcd2bb55ccca..89b1bea50ee3 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -1029,7 +1029,6 @@ static int s3c_hsotg_process_req_feature(struct s3c_hsotg *hsotg,
>  }
> 
>  static void s3c_hsotg_enqueue_setup(struct s3c_hsotg *hsotg);
> -static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg);
> 
>  /**
>   * s3c_hsotg_stall_ep0 - stall ep0
> @@ -1107,7 +1106,6 @@ static void s3c_hsotg_process_control(struct s3c_hsotg *hsotg,
>  	if ((ctrl->bRequestType & USB_TYPE_MASK) == USB_TYPE_STANDARD) {
>  		switch (ctrl->bRequest) {
>  		case USB_REQ_SET_ADDRESS:
> -			s3c_hsotg_disconnect(hsotg);
>  			dcfg = readl(hsotg->regs + DCFG);
>  			dcfg &= ~DCFG_DEVADDR_MASK;
>  			dcfg |= (le16_to_cpu(ctrl->wValue) <<
> @@ -2031,6 +2029,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg)
>  {
>  	unsigned ep;
> 
> +	if (!hsotg->connected)
> +		return;
> +
> +	hsotg->connected = 0;
>  	for (ep = 0; ep < hsotg->num_of_eps; ep++)
>  		kill_all_requests(hsotg, &hsotg->eps[ep], -ESHUTDOWN, true);
> 
> @@ -2290,17 +2292,27 @@ 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) {
>  		dev_dbg(hsotg->dev, "%s: SessReqInt\n", __func__);
>  		writel(GINTSTS_SESSREQINT, hsotg->regs + GINTSTS);
> +		/*
> +		 * Report disconnect if there is any previous session established
> +		 */
> +		s3c_hsotg_disconnect(hsotg);
>  	}
> 
>  	if (gintsts & GINTSTS_ENUMDONE) {
>  		writel(GINTSTS_ENUMDONE, hsotg->regs + GINTSTS);
> 
>  		s3c_hsotg_irq_enumdone(hsotg);
> +		hsotg->connected = 1;
>  	}
> 
>  	if (gintsts & GINTSTS_CONIDSTSCHNG) {

Acked-by: Paul Zimmerman <paulz-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>

--
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 v5] usb: dwc2/gadget: rework disconnect event handling

[Felipe Balbi]

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

On Mon, Nov 17, 2014 at 09:59:42AM +0100, Marek Szyprowski wrote:
> 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. DISCONNINT interrupt cannot be used for this
> purpose, because it is asserted only in host mode.
> 
> To avoid reporting disconnect event more than once, a disconnect call has
> been moved from USB_REQ_SET_ADDRESS handling function to SESSREQINT
> interrupt. This way driver ensures that disconnect event is reported
> either when usb cable is unplugged or every time the host starts a new
> session. To handle devices which has been synthesized without
> SRP support, connected state is set in ENUMDONE interrupt.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

doesn't apply, please rebase on my testing/next:

checking file drivers/usb/dwc2/core.h
Hunk #1 FAILED at 210.
1 out of 1 hunk FAILED
checking file drivers/usb/dwc2/gadget.c
Hunk #1 FAILED at 1029.
Hunk #2 succeeded at 1108 (offset 1 line).
Hunk #3 succeeded at 2031 (offset 1 line).
Hunk #4 FAILED at 2293.
2 out of 4 hunks FAILED

thanks

-- 
balbi

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

[PATCH v2 05/10] usb: dwc2/gadget: move setting last reset time to s3c_hsotg_core_init

[Marek Szyprowski]

This patch removes duplicated code and sets last_rst variable in the
function which does the hardware reset.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/usb/dwc2/gadget.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index fd52a8b23649..c1dad46bbbdd 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2247,6 +2247,8 @@ static void s3c_hsotg_core_init(struct s3c_hsotg *hsotg)
 	/* must be at-least 3ms to allow bus to see disconnect */
 	mdelay(3);
 
+	hsotg->last_rst = jiffies;
+
 	/* remove the soft-disconnect and let's go */
 	__bic32(hsotg->regs + DCTL, DCTL_SFTDISCON);
 }
@@ -2347,7 +2349,6 @@ irq_retry:
 							  -ECONNRESET, true);
 
 				s3c_hsotg_core_init(hsotg);
-				hsotg->last_rst = jiffies;
 			}
 		}
 	}
@@ -2908,7 +2909,6 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
 		goto err;
 	}
 
-	hsotg->last_rst = jiffies;
 	dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
 	return 0;
 
@@ -3667,7 +3667,6 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
 	}
 
 	spin_lock_irqsave(&hsotg->lock, flags);
-	hsotg->last_rst = jiffies;
 	s3c_hsotg_phy_enable(hsotg);
 	s3c_hsotg_core_init(hsotg);
 	spin_unlock_irqrestore(&hsotg->lock, flags);
-- 
1.9.2

RE: [PATCH v2 05/10] usb: dwc2/gadget: move setting last reset time to s3c_hsotg_core_init

[Paul Zimmerman]

> From: Marek Szyprowski [mailto:m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org]
> Sent: Monday, October 20, 2014 3:46 AM
> 
> This patch removes duplicated code and sets last_rst variable in the
> function which does the hardware reset.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/usb/dwc2/gadget.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index fd52a8b23649..c1dad46bbbdd 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -2247,6 +2247,8 @@ static void s3c_hsotg_core_init(struct s3c_hsotg *hsotg)
>  	/* must be at-least 3ms to allow bus to see disconnect */
>  	mdelay(3);
> 
> +	hsotg->last_rst = jiffies;
> +
>  	/* remove the soft-disconnect and let's go */
>  	__bic32(hsotg->regs + DCTL, DCTL_SFTDISCON);
>  }
> @@ -2347,7 +2349,6 @@ irq_retry:
>  							  -ECONNRESET, true);
> 
>  				s3c_hsotg_core_init(hsotg);
> -				hsotg->last_rst = jiffies;
>  			}
>  		}
>  	}
> @@ -2908,7 +2909,6 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
>  		goto err;
>  	}
> 
> -	hsotg->last_rst = jiffies;
>  	dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
>  	return 0;
> 
> @@ -3667,7 +3667,6 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
>  	}
> 
>  	spin_lock_irqsave(&hsotg->lock, flags);
> -	hsotg->last_rst = jiffies;
>  	s3c_hsotg_phy_enable(hsotg);
>  	s3c_hsotg_core_init(hsotg);
>  	spin_unlock_irqrestore(&hsotg->lock, flags);

Acked-by: Paul Zimmerman <paulz-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>

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

[PATCH v2 02/10] usb: dwc2/gadget: fix enumeration issues

[Marek Szyprowski]

Excessive debug messages might cause timing issues that prevent correct
usb enumeration. This patch hides information about USB bus reset to let
driver enumerate fast enough to avoid making host angry. This fixes
endless enumeration and usb reset loop observed with some Linux hosts.

Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Reviewed-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
---
 drivers/usb/dwc2/gadget.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 119c8a3effc2..8870e38c1d82 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2333,7 +2333,7 @@ irq_retry:
 
 		u32 usb_status = readl(hsotg->regs + GOTGCTL);
 
-		dev_info(hsotg->dev, "%s: USBRst\n", __func__);
+		dev_dbg(hsotg->dev, "%s: USBRst\n", __func__);
 		dev_dbg(hsotg->dev, "GNPTXSTS=%08x\n",
 			readl(hsotg->regs + GNPTXSTS));
 
-- 
1.9.2

--
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 v2 02/10] usb: dwc2/gadget: fix enumeration issues

[Paul Zimmerman]

> -----Original Message-----
> From: Marek Szyprowski [mailto:m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org]
> Sent: Monday, October 20, 2014 3:46 AM
> 
> Excessive debug messages might cause timing issues that prevent correct
> usb enumeration. This patch hides information about USB bus reset to let
> driver enumerate fast enough to avoid making host angry. This fixes
> endless enumeration and usb reset loop observed with some Linux hosts.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Reviewed-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/usb/dwc2/gadget.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 119c8a3effc2..8870e38c1d82 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -2333,7 +2333,7 @@ irq_retry:
> 
>  		u32 usb_status = readl(hsotg->regs + GOTGCTL);
> 
> -		dev_info(hsotg->dev, "%s: USBRst\n", __func__);
> +		dev_dbg(hsotg->dev, "%s: USBRst\n", __func__);
>  		dev_dbg(hsotg->dev, "GNPTXSTS=%08x\n",
>  			readl(hsotg->regs + GNPTXSTS));
> 

Acked-by: Paul Zimmerman <paulz-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>

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

[PATCH v2 03/10] usb: dwc2/gadget: fix gadget unregistration in udc_stop() function

[Marek Szyprowski]

udc_stop() should clear ->driver pointer unconditionally to let the UDC
framework to work correctly with both registering/unregistering gadgets
and enabling/disabling gadgets by writing to
/sys/class/udc/*hsotg/soft_connect interface.

Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/dwc2/gadget.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 8870e38c1d82..a4b4def23afd 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2940,9 +2940,7 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
 
 	spin_lock_irqsave(&hsotg->lock, flags);
 
-	if (!driver)
-		hsotg->driver = NULL;

Re: [PATCH v2 03/10] usb: dwc2/gadget: fix gadget unregistration in udc_stop() function

[Felipe Balbi]

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

On Mon, Oct 20, 2014 at 12:45:33PM +0200, Marek Szyprowski wrote:
> udc_stop() should clear ->driver pointer unconditionally to let the UDC
> framework to work correctly with both registering/unregistering gadgets
> and enabling/disabling gadgets by writing to
> /sys/class/udc/*hsotg/soft_connect interface.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

Also here. In fact, this is much more important :-)

-- 
balbi

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

RE: [PATCH v2 03/10] usb: dwc2/gadget: fix gadget unregistration in udc_stop() function

[Paul Zimmerman]

> From: Felipe Balbi [mailto:balbi@ti.com]
> Sent: Thursday, October 23, 2014 8:02 AM
> 
> On Mon, Oct 20, 2014 at 12:45:33PM +0200, Marek Szyprowski wrote:
> > udc_stop() should clear ->driver pointer unconditionally to let the UDC
> > framework to work correctly with both registering/unregistering gadgets
> > and enabling/disabling gadgets by writing to
> > /sys/class/udc/*hsotg/soft_connect interface.
> >
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> 
> Also here. In fact, this is much more important :-)

Acked-by: Paul Zimmerman <paulz@synopsys.com>

Are there any more patches in this series that need immediate attention?
Otherwise I plan to finish reviewing the series on Friday or so.

-- 
Paul

Re: [PATCH v2 03/10] usb: dwc2/gadget: fix gadget unregistration in udc_stop() function

[Felipe Balbi]

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

On Thu, Oct 23, 2014 at 06:18:51PM +0000, Paul Zimmerman wrote:
> > From: Felipe Balbi [mailto:balbi-l0cyMroinI0@public.gmane.org]
> > Sent: Thursday, October 23, 2014 8:02 AM
> > 
> > On Mon, Oct 20, 2014 at 12:45:33PM +0200, Marek Szyprowski wrote:
> > > udc_stop() should clear ->driver pointer unconditionally to let the UDC
> > > framework to work correctly with both registering/unregistering gadgets
> > > and enabling/disabling gadgets by writing to
> > > /sys/class/udc/*hsotg/soft_connect interface.
> > >
> > > Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > 
> > Also here. In fact, this is much more important :-)
> 
> Acked-by: Paul Zimmerman <paulz-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> 
> Are there any more patches in this series that need immediate attention?
> Otherwise I plan to finish reviewing the series on Friday or so.

no, this is the most important one. Everything else can wait until
Friday, thanks.

-- 
balbi

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

[PATCH v2 04/10] usb: dwc2/gadget: disable phy before turning off power regulators

[Marek Szyprowski]

This patch fixes probe function to match the pattern used elsewhere in
the driver, where power regulators are turned off as the last element in
the device shutdown procedure.

Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/dwc2/gadget.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index a4b4def23afd..fd52a8b23649 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3571,6 +3571,7 @@ static int s3c_hsotg_probe(struct platform_device *pdev)
 		s3c_hsotg_initep(hsotg, &hsotg->eps[epnum], epnum);
 
 	/* disable power and clock */
+	s3c_hsotg_phy_disable(hsotg);
 
 	ret = regulator_bulk_disable(ARRAY_SIZE(hsotg->supplies),
 				    hsotg->supplies);
@@ -3579,8 +3580,6 @@ static int s3c_hsotg_probe(struct platform_device *pdev)
 		goto err_ep_mem;
 	}
 
-	s3c_hsotg_phy_disable(hsotg);

Re: [PATCH v2 04/10] usb: dwc2/gadget: disable phy before turning off power regulators

[Felipe Balbi]

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

On Mon, Oct 20, 2014 at 12:45:34PM +0200, Marek Szyprowski wrote:
> This patch fixes probe function to match the pattern used elsewhere in
> the driver, where power regulators are turned off as the last element in
> the device shutdown procedure.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

Paul, can I have your Acked-by here so I can send this to v3.18-rc1 ?

-- 
balbi

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

RE: [PATCH v2 04/10] usb: dwc2/gadget: disable phy before turning off power regulators

[Paul Zimmerman]

> From: Felipe Balbi [mailto:balbi@ti.com]
> Sent: Thursday, October 23, 2014 8:01 AM
> 
> On Mon, Oct 20, 2014 at 12:45:34PM +0200, Marek Szyprowski wrote:
> > This patch fixes probe function to match the pattern used elsewhere in
> > the driver, where power regulators are turned off as the last element in
> > the device shutdown procedure.
> >
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> 
> Paul, can I have your Acked-by here so I can send this to v3.18-rc1 ?

Sorry, I've been working on a really hot issue at $work and didn't have
time yet to review this series.

Acked-by: Paul Zimmerman <paulz@synopsys.com>

Re: [PATCH v2 04/10] usb: dwc2/gadget: disable phy before turning off power regulators

[Felipe Balbi]

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

On Thu, Oct 23, 2014 at 06:15:57PM +0000, Paul Zimmerman wrote:
> > From: Felipe Balbi [mailto:balbi@ti.com]
> > Sent: Thursday, October 23, 2014 8:01 AM
> > 
> > On Mon, Oct 20, 2014 at 12:45:34PM +0200, Marek Szyprowski wrote:
> > > This patch fixes probe function to match the pattern used elsewhere in
> > > the driver, where power regulators are turned off as the last element in
> > > the device shutdown procedure.
> > >
> > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > 
> > Paul, can I have your Acked-by here so I can send this to v3.18-rc1 ?
> 
> Sorry, I've been working on a really hot issue at $work and didn't have
> time yet to review this series.

don't worry, we've all been there :-)

thanks

-- 
balbi

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

[PATCH v2 06/10] usb: dwc2/gadget: decouple setting soft-disconnect from s3c_hsotg_core_init

[Marek Szyprowski]

This patch changes s3c_hsotg_core_init function to leave hardware in
soft disconnect mode, so the moment of coupling the hardware to the usb
bus can be later controlled by the separate functions for enabling and
disabling soft disconnect mode. This patch is a preparation to rework
pullup() method.

Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/dwc2/gadget.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index c1dad46bbbdd..5eb2473031c4 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2124,7 +2124,7 @@ static int s3c_hsotg_corereset(struct s3c_hsotg *hsotg)
  *
  * Issue a soft reset to the core, and await the core finishing it.
  */
-static void s3c_hsotg_core_init(struct s3c_hsotg *hsotg)
+static void s3c_hsotg_core_init_disconnected(struct s3c_hsotg *hsotg)
 {
 	s3c_hsotg_corereset(hsotg);
 
@@ -2241,14 +2241,23 @@ static void s3c_hsotg_core_init(struct s3c_hsotg *hsotg)
 		readl(hsotg->regs + DOEPCTL0));
 
 	/* clear global NAKs */
-	writel(DCTL_CGOUTNAK | DCTL_CGNPINNAK,
+	writel(DCTL_CGOUTNAK | DCTL_CGNPINNAK | DCTL_SFTDISCON,
 	       hsotg->regs + DCTL);
 
 	/* must be at-least 3ms to allow bus to see disconnect */
 	mdelay(3);
 
 	hsotg->last_rst = jiffies;
+}
+
+static void s3c_hsotg_core_disconnect(struct s3c_hsotg *hsotg)
+{
+	/* set the soft-disconnect bit */
+	__orr32(hsotg->regs + DCTL, DCTL_SFTDISCON);
+}
 
+static void s3c_hsotg_core_connect(struct s3c_hsotg *hsotg)
+{
 	/* remove the soft-disconnect and let's go */
 	__bic32(hsotg->regs + DCTL, DCTL_SFTDISCON);
 }
@@ -2348,7 +2357,8 @@ irq_retry:
 				kill_all_requests(hsotg, &hsotg->eps[0],
 							  -ECONNRESET, true);
 
-				s3c_hsotg_core_init(hsotg);
+				s3c_hsotg_core_init_disconnected(hsotg);
+				s3c_hsotg_core_connect(hsotg);
 			}
 		}
 	}
@@ -2981,7 +2991,8 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
 	if (is_on) {
 		s3c_hsotg_phy_enable(hsotg);
 		clk_enable(hsotg->clk);
-		s3c_hsotg_core_init(hsotg);
+		s3c_hsotg_core_init_disconnected(hsotg);
+		s3c_hsotg_core_connect(hsotg);
 	} else {
 		clk_disable(hsotg->clk);
 		s3c_hsotg_phy_disable(hsotg);
@@ -3668,7 +3679,8 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
 
 	spin_lock_irqsave(&hsotg->lock, flags);
 	s3c_hsotg_phy_enable(hsotg);
-	s3c_hsotg_core_init(hsotg);
+	s3c_hsotg_core_init_disconnected(hsotg);
+	s3c_hsotg_core_connect(hsotg);
 	spin_unlock_irqrestore(&hsotg->lock, flags);
 
 	return ret;
-- 
1.9.2

--
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 v2 06/10] usb: dwc2/gadget: decouple setting soft-disconnect from s3c_hsotg_core_init

[Paul Zimmerman]

> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
> Sent: Monday, October 20, 2014 3:46 AM
> 
> This patch changes s3c_hsotg_core_init function to leave hardware in
> soft disconnect mode, so the moment of coupling the hardware to the usb
> bus can be later controlled by the separate functions for enabling and
> disabling soft disconnect mode. This patch is a preparation to rework
> pullup() method.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/usb/dwc2/gadget.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index c1dad46bbbdd..5eb2473031c4 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -2124,7 +2124,7 @@ static int s3c_hsotg_corereset(struct s3c_hsotg *hsotg)
>   *
>   * Issue a soft reset to the core, and await the core finishing it.
>   */
> -static void s3c_hsotg_core_init(struct s3c_hsotg *hsotg)
> +static void s3c_hsotg_core_init_disconnected(struct s3c_hsotg *hsotg)
>  {
>  	s3c_hsotg_corereset(hsotg);
> 
> @@ -2241,14 +2241,23 @@ static void s3c_hsotg_core_init(struct s3c_hsotg *hsotg)
>  		readl(hsotg->regs + DOEPCTL0));
> 
>  	/* clear global NAKs */
> -	writel(DCTL_CGOUTNAK | DCTL_CGNPINNAK,
> +	writel(DCTL_CGOUTNAK | DCTL_CGNPINNAK | DCTL_SFTDISCON,
>  	       hsotg->regs + DCTL);
> 
>  	/* must be at-least 3ms to allow bus to see disconnect */
>  	mdelay(3);
> 
>  	hsotg->last_rst = jiffies;
> +}
> +
> +static void s3c_hsotg_core_disconnect(struct s3c_hsotg *hsotg)
> +{
> +	/* set the soft-disconnect bit */
> +	__orr32(hsotg->regs + DCTL, DCTL_SFTDISCON);

I'm not really happy with adding more uses of these __orr32, __bic32
functions. When I first saw those, I went 'wtf?", because with the
assembly-sounding names, they look like some special ARM instruction
or something.

But I guess cleaning that up can wait for a future patch series, so OK.

> +}
> 
> +static void s3c_hsotg_core_connect(struct s3c_hsotg *hsotg)
> +{
>  	/* remove the soft-disconnect and let's go */
>  	__bic32(hsotg->regs + DCTL, DCTL_SFTDISCON);
>  }
> @@ -2348,7 +2357,8 @@ irq_retry:
>  				kill_all_requests(hsotg, &hsotg->eps[0],
>  							  -ECONNRESET, true);
> 
> -				s3c_hsotg_core_init(hsotg);
> +				s3c_hsotg_core_init_disconnected(hsotg);
> +				s3c_hsotg_core_connect(hsotg);
>  			}
>  		}
>  	}
> @@ -2981,7 +2991,8 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
>  	if (is_on) {
>  		s3c_hsotg_phy_enable(hsotg);
>  		clk_enable(hsotg->clk);
> -		s3c_hsotg_core_init(hsotg);
> +		s3c_hsotg_core_init_disconnected(hsotg);
> +		s3c_hsotg_core_connect(hsotg);
>  	} else {
>  		clk_disable(hsotg->clk);
>  		s3c_hsotg_phy_disable(hsotg);
> @@ -3668,7 +3679,8 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
> 
>  	spin_lock_irqsave(&hsotg->lock, flags);
>  	s3c_hsotg_phy_enable(hsotg);
> -	s3c_hsotg_core_init(hsotg);
> +	s3c_hsotg_core_init_disconnected(hsotg);
> +	s3c_hsotg_core_connect(hsotg);
>  	spin_unlock_irqrestore(&hsotg->lock, flags);
> 
>  	return ret;

Acked-by: Paul Zimmerman <paulz@synopsys.com>

Re: [PATCH v2 06/10] usb: dwc2/gadget: decouple setting soft-disconnect from s3c_hsotg_core_init

[Marek Szyprowski]

Hello,

On 2014-10-25 02:35, Paul Zimmerman wrote:
>> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
>> Sent: Monday, October 20, 2014 3:46 AM
>>
>> This patch changes s3c_hsotg_core_init function to leave hardware in
>> soft disconnect mode, so the moment of coupling the hardware to the usb
>> bus can be later controlled by the separate functions for enabling and
>> disabling soft disconnect mode. This patch is a preparation to rework
>> pullup() method.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   drivers/usb/dwc2/gadget.c | 22 +++++++++++++++++-----
>>   1 file changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index c1dad46bbbdd..5eb2473031c4 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -2124,7 +2124,7 @@ static int s3c_hsotg_corereset(struct s3c_hsotg *hsotg)
>>    *
>>    * Issue a soft reset to the core, and await the core finishing it.
>>    */
>> -static void s3c_hsotg_core_init(struct s3c_hsotg *hsotg)
>> +static void s3c_hsotg_core_init_disconnected(struct s3c_hsotg *hsotg)
>>   {
>>   	s3c_hsotg_corereset(hsotg);
>>
>> @@ -2241,14 +2241,23 @@ static void s3c_hsotg_core_init(struct s3c_hsotg *hsotg)
>>   		readl(hsotg->regs + DOEPCTL0));
>>
>>   	/* clear global NAKs */
>> -	writel(DCTL_CGOUTNAK | DCTL_CGNPINNAK,
>> +	writel(DCTL_CGOUTNAK | DCTL_CGNPINNAK | DCTL_SFTDISCON,
>>   	       hsotg->regs + DCTL);
>>
>>   	/* must be at-least 3ms to allow bus to see disconnect */
>>   	mdelay(3);
>>
>>   	hsotg->last_rst = jiffies;
>> +}
>> +
>> +static void s3c_hsotg_core_disconnect(struct s3c_hsotg *hsotg)
>> +{
>> +	/* set the soft-disconnect bit */
>> +	__orr32(hsotg->regs + DCTL, DCTL_SFTDISCON);
> I'm not really happy with adding more uses of these __orr32, __bic32
> functions. When I first saw those, I went 'wtf?", because with the
> assembly-sounding names, they look like some special ARM instruction
> or something.
>
> But I guess cleaning that up can wait for a future patch series, so OK.

Those functions are really convenient, but I agree we need a better 
names for
them.

>> +}
>>
>> +static void s3c_hsotg_core_connect(struct s3c_hsotg *hsotg)
>> +{
>>   	/* remove the soft-disconnect and let's go */
>>   	__bic32(hsotg->regs + DCTL, DCTL_SFTDISCON);
>>   }
>> @@ -2348,7 +2357,8 @@ irq_retry:
>>   				kill_all_requests(hsotg, &hsotg->eps[0],
>>   							  -ECONNRESET, true);
>>
>> -				s3c_hsotg_core_init(hsotg);
>> +				s3c_hsotg_core_init_disconnected(hsotg);
>> +				s3c_hsotg_core_connect(hsotg);
>>   			}
>>   		}
>>   	}
>> @@ -2981,7 +2991,8 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
>>   	if (is_on) {
>>   		s3c_hsotg_phy_enable(hsotg);
>>   		clk_enable(hsotg->clk);
>> -		s3c_hsotg_core_init(hsotg);
>> +		s3c_hsotg_core_init_disconnected(hsotg);
>> +		s3c_hsotg_core_connect(hsotg);
>>   	} else {
>>   		clk_disable(hsotg->clk);
>>   		s3c_hsotg_phy_disable(hsotg);
>> @@ -3668,7 +3679,8 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
>>
>>   	spin_lock_irqsave(&hsotg->lock, flags);
>>   	s3c_hsotg_phy_enable(hsotg);
>> -	s3c_hsotg_core_init(hsotg);
>> +	s3c_hsotg_core_init_disconnected(hsotg);
>> +	s3c_hsotg_core_connect(hsotg);
>>   	spin_unlock_irqrestore(&hsotg->lock, flags);
>>
>>   	return ret;
> Acked-by: Paul Zimmerman <paulz@synopsys.com>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[PATCH v2 09/10] usb: dwc2/gadget: fix calls to phy control functions in suspend/resume code

[Marek Szyprowski]

This patch moves calls to phy enable/disable out of spinlock protected
blocks in device suspend/resume to fix incorrect caller context. Phy
related functions must not be called from atomic context. To protect
device internal state from a race during suspend, a call to
s3c_hsotg_core_disconnect() is added under a spinlock, what prevents any
further activity on the usb bus.

Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/dwc2/gadget.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index e8ffc080e6c7..0d34cfc71bfb 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3653,11 +3653,13 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t state)
 			 hsotg->driver->driver.name);
 
 	spin_lock_irqsave(&hsotg->lock, flags);
+	s3c_hsotg_core_disconnect(hsotg);
 	s3c_hsotg_disconnect(hsotg);
-	s3c_hsotg_phy_disable(hsotg);
 	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
 	spin_unlock_irqrestore(&hsotg->lock, flags);
 
+	s3c_hsotg_phy_disable(hsotg);
+
 	if (hsotg->driver) {
 		int ep;
 		for (ep = 0; ep < hsotg->num_of_eps; ep++)
@@ -3686,8 +3688,9 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
 				      hsotg->supplies);
 	}
 
-	spin_lock_irqsave(&hsotg->lock, flags);
 	s3c_hsotg_phy_enable(hsotg);
+
+	spin_lock_irqsave(&hsotg->lock, flags);
 	s3c_hsotg_core_init_disconnected(hsotg);
 	s3c_hsotg_core_connect(hsotg);
 	spin_unlock_irqrestore(&hsotg->lock, flags);
-- 
1.9.2

--
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 v2 09/10] usb: dwc2/gadget: fix calls to phy control functions in suspend/resume code

[Paul Zimmerman]

> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
> Sent: Monday, October 20, 2014 3:46 AM
> 
> This patch moves calls to phy enable/disable out of spinlock protected
> blocks in device suspend/resume to fix incorrect caller context. Phy
> related functions must not be called from atomic context. To protect
> device internal state from a race during suspend, a call to
> s3c_hsotg_core_disconnect() is added under a spinlock, what prevents any
> further activity on the usb bus.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/usb/dwc2/gadget.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index e8ffc080e6c7..0d34cfc71bfb 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -3653,11 +3653,13 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t state)
>  			 hsotg->driver->driver.name);
> 
>  	spin_lock_irqsave(&hsotg->lock, flags);
> +	s3c_hsotg_core_disconnect(hsotg);
>  	s3c_hsotg_disconnect(hsotg);
> -	s3c_hsotg_phy_disable(hsotg);
>  	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
>  	spin_unlock_irqrestore(&hsotg->lock, flags);
> 
> +	s3c_hsotg_phy_disable(hsotg);
> +
>  	if (hsotg->driver) {
>  		int ep;
>  		for (ep = 0; ep < hsotg->num_of_eps; ep++)
> @@ -3686,8 +3688,9 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
>  				      hsotg->supplies);
>  	}
> 
> -	spin_lock_irqsave(&hsotg->lock, flags);
>  	s3c_hsotg_phy_enable(hsotg);
> +
> +	spin_lock_irqsave(&hsotg->lock, flags);
>  	s3c_hsotg_core_init_disconnected(hsotg);
>  	s3c_hsotg_core_connect(hsotg);
>  	spin_unlock_irqrestore(&hsotg->lock, flags);

Acked-by: Paul Zimmerman <paulz@synopsys.com>

[PATCH v2 10/10] usb: dwc2/gadget: rework suspend/resume code to correctly restore gadget state

[Marek Szyprowski]

Suspend/resume code assumed that the gadget was always enabled and
connected to usb bus. This means that the actual state of the gadget
(soft-enabled/disabled or connected/disconnected) was not correctly
preserved on suspend/resume cycle. This patch fixes this issue.

Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/dwc2/core.h   |  4 +++-
 drivers/usb/dwc2/gadget.c | 43 +++++++++++++++++++++++++++----------------
 2 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index bf015ab3b44c..3648b76a18b4 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -210,7 +210,9 @@ struct s3c_hsotg {
 	u8                      ctrl_buff[8];
 
 	struct usb_gadget       gadget;
-	unsigned int            setup;
+	unsigned int            setup:1;
+	unsigned int		connected:1;
+	unsigned int		enabled:1;
 	unsigned long           last_rst;
 	struct s3c_hsotg_ep     *eps;
 };
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 0d34cfc71bfb..c6c6cf982c90 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2925,6 +2925,8 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
 	spin_lock_irqsave(&hsotg->lock, flags);
 	s3c_hsotg_init(hsotg);
 	s3c_hsotg_core_init_disconnected(hsotg);
+	hsotg->enabled = 1;
+	hsotg->connected = 0;
 	spin_unlock_irqrestore(&hsotg->lock, flags);
 
 	dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
@@ -2961,6 +2963,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
 
 	hsotg->driver = NULL;
 	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
+	hsotg->enabled = 0;
+	hsotg->connected = 0;
 
 	spin_unlock_irqrestore(&hsotg->lock, flags);
 
@@ -2999,11 +3003,14 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
 	dev_dbg(hsotg->dev, "%s: is_on: %d\n", __func__, is_on);
 
 	spin_lock_irqsave(&hsotg->lock, flags);
+
 	if (is_on) {
 		clk_enable(hsotg->clk);
+		hsotg->connected = 1;
 		s3c_hsotg_core_connect(hsotg);
 	} else {
 		s3c_hsotg_core_disconnect(hsotg);
+		hsotg->connected = 0;
 		clk_disable(hsotg->clk);
 	}
 
@@ -3652,16 +3659,18 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t state)
 		dev_info(hsotg->dev, "suspending usb gadget %s\n",
 			 hsotg->driver->driver.name);
 
-	spin_lock_irqsave(&hsotg->lock, flags);
-	s3c_hsotg_core_disconnect(hsotg);
-	s3c_hsotg_disconnect(hsotg);
-	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
-	spin_unlock_irqrestore(&hsotg->lock, flags);
+	if (hsotg->enabled) {
+		int ep;
 
-	s3c_hsotg_phy_disable(hsotg);
+		spin_lock_irqsave(&hsotg->lock, flags);
+		if (hsotg->connected)
+			s3c_hsotg_core_disconnect(hsotg);
+		s3c_hsotg_disconnect(hsotg);
+		hsotg->gadget.speed = USB_SPEED_UNKNOWN;
+		spin_unlock_irqrestore(&hsotg->lock, flags);
+
+		s3c_hsotg_phy_disable(hsotg);
 
-	if (hsotg->driver) {
-		int ep;
 		for (ep = 0; ep < hsotg->num_of_eps; ep++)
 			s3c_hsotg_ep_disable(&hsotg->eps[ep].ep);
 
@@ -3679,21 +3688,23 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
 	unsigned long flags;
 	int ret = 0;
 
-	if (hsotg->driver) {
+	if (hsotg->driver)
 		dev_info(hsotg->dev, "resuming usb gadget %s\n",
 			 hsotg->driver->driver.name);
 
+	if (hsotg->enabled) {
 		clk_enable(hsotg->clk);
 		ret = regulator_bulk_enable(ARRAY_SIZE(hsotg->supplies),
-				      hsotg->supplies);
-	}
+					    hsotg->supplies);
 
-	s3c_hsotg_phy_enable(hsotg);
+		s3c_hsotg_phy_enable(hsotg);
 
-	spin_lock_irqsave(&hsotg->lock, flags);
-	s3c_hsotg_core_init_disconnected(hsotg);
-	s3c_hsotg_core_connect(hsotg);
-	spin_unlock_irqrestore(&hsotg->lock, flags);
+		spin_lock_irqsave(&hsotg->lock, flags);
+		s3c_hsotg_core_init_disconnected(hsotg);
+		if (hsotg->connected)
+			s3c_hsotg_core_connect(hsotg);
+		spin_unlock_irqrestore(&hsotg->lock, flags);
+	}
 
 	return ret;
 }
-- 
1.9.2

--
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 v2 10/10] usb: dwc2/gadget: rework suspend/resume code to correctly restore gadget state

[Paul Zimmerman]

> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
> Sent: Monday, October 20, 2014 3:46 AM
> 
> Suspend/resume code assumed that the gadget was always enabled and
> connected to usb bus. This means that the actual state of the gadget
> (soft-enabled/disabled or connected/disconnected) was not correctly
> preserved on suspend/resume cycle. This patch fixes this issue.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/usb/dwc2/core.h   |  4 +++-
>  drivers/usb/dwc2/gadget.c | 43 +++++++++++++++++++++++++++----------------
>  2 files changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index bf015ab3b44c..3648b76a18b4 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -210,7 +210,9 @@ struct s3c_hsotg {
>  	u8                      ctrl_buff[8];
> 
>  	struct usb_gadget       gadget;
> -	unsigned int            setup;
> +	unsigned int            setup:1;
> +	unsigned int		connected:1;
> +	unsigned int		enabled:1;
>  	unsigned long           last_rst;
>  	struct s3c_hsotg_ep     *eps;
>  };
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 0d34cfc71bfb..c6c6cf982c90 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -2925,6 +2925,8 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
>  	spin_lock_irqsave(&hsotg->lock, flags);
>  	s3c_hsotg_init(hsotg);
>  	s3c_hsotg_core_init_disconnected(hsotg);
> +	hsotg->enabled = 1;
> +	hsotg->connected = 0;
>  	spin_unlock_irqrestore(&hsotg->lock, flags);
> 
>  	dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
> @@ -2961,6 +2963,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
> 
>  	hsotg->driver = NULL;
>  	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
> +	hsotg->enabled = 0;
> +	hsotg->connected = 0;
> 
>  	spin_unlock_irqrestore(&hsotg->lock, flags);
> 
> @@ -2999,11 +3003,14 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
>  	dev_dbg(hsotg->dev, "%s: is_on: %d\n", __func__, is_on);
> 
>  	spin_lock_irqsave(&hsotg->lock, flags);
> +
>  	if (is_on) {
>  		clk_enable(hsotg->clk);
> +		hsotg->connected = 1;
>  		s3c_hsotg_core_connect(hsotg);
>  	} else {
>  		s3c_hsotg_core_disconnect(hsotg);
> +		hsotg->connected = 0;
>  		clk_disable(hsotg->clk);
>  	}
> 
> @@ -3652,16 +3659,18 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t state)
>  		dev_info(hsotg->dev, "suspending usb gadget %s\n",
>  			 hsotg->driver->driver.name);
> 
> -	spin_lock_irqsave(&hsotg->lock, flags);
> -	s3c_hsotg_core_disconnect(hsotg);
> -	s3c_hsotg_disconnect(hsotg);
> -	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
> -	spin_unlock_irqrestore(&hsotg->lock, flags);
> +	if (hsotg->enabled) {

Hmm. Are you sure it's safe to check ->enabled outside of the spinlock?
What happens if s3c_hsotg_udc_stop() runs right after this, before the
spinlock is taken, and disables stuff? Sure, it's a tiny window, but
still...

-- 
Paul

> +		int ep;
> 
> -	s3c_hsotg_phy_disable(hsotg);
> +		spin_lock_irqsave(&hsotg->lock, flags);
> +		if (hsotg->connected)
> +			s3c_hsotg_core_disconnect(hsotg);
> +		s3c_hsotg_disconnect(hsotg);
> +		hsotg->gadget.speed = USB_SPEED_UNKNOWN;
> +		spin_unlock_irqrestore(&hsotg->lock, flags);
> +
> +		s3c_hsotg_phy_disable(hsotg);
> 
> -	if (hsotg->driver) {
> -		int ep;
>  		for (ep = 0; ep < hsotg->num_of_eps; ep++)
>  			s3c_hsotg_ep_disable(&hsotg->eps[ep].ep);
> 
> @@ -3679,21 +3688,23 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
>  	unsigned long flags;
>  	int ret = 0;
> 
> -	if (hsotg->driver) {
> +	if (hsotg->driver)
>  		dev_info(hsotg->dev, "resuming usb gadget %s\n",
>  			 hsotg->driver->driver.name);
> 
> +	if (hsotg->enabled) {
>  		clk_enable(hsotg->clk);
>  		ret = regulator_bulk_enable(ARRAY_SIZE(hsotg->supplies),
> -				      hsotg->supplies);
> -	}
> +					    hsotg->supplies);
> 
> -	s3c_hsotg_phy_enable(hsotg);
> +		s3c_hsotg_phy_enable(hsotg);
> 
> -	spin_lock_irqsave(&hsotg->lock, flags);
> -	s3c_hsotg_core_init_disconnected(hsotg);
> -	s3c_hsotg_core_connect(hsotg);
> -	spin_unlock_irqrestore(&hsotg->lock, flags);
> +		spin_lock_irqsave(&hsotg->lock, flags);
> +		s3c_hsotg_core_init_disconnected(hsotg);
> +		if (hsotg->connected)
> +			s3c_hsotg_core_connect(hsotg);
> +		spin_unlock_irqrestore(&hsotg->lock, flags);
> +	}
> 
>  	return ret;
>  }

Re: [PATCH v2 10/10] usb: dwc2/gadget: rework suspend/resume code to correctly restore gadget state

[Marek Szyprowski]

Hello,

On 2014-10-25 03:16, Paul Zimmerman wrote:
>> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
>> Sent: Monday, October 20, 2014 3:46 AM
>>
>> Suspend/resume code assumed that the gadget was always enabled and
>> connected to usb bus. This means that the actual state of the gadget
>> (soft-enabled/disabled or connected/disconnected) was not correctly
>> preserved on suspend/resume cycle. This patch fixes this issue.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   drivers/usb/dwc2/core.h   |  4 +++-
>>   drivers/usb/dwc2/gadget.c | 43 +++++++++++++++++++++++++++----------------
>>   2 files changed, 30 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>> index bf015ab3b44c..3648b76a18b4 100644
>> --- a/drivers/usb/dwc2/core.h
>> +++ b/drivers/usb/dwc2/core.h
>> @@ -210,7 +210,9 @@ struct s3c_hsotg {
>>   	u8                      ctrl_buff[8];
>>
>>   	struct usb_gadget       gadget;
>> -	unsigned int            setup;
>> +	unsigned int            setup:1;
>> +	unsigned int		connected:1;
>> +	unsigned int		enabled:1;
>>   	unsigned long           last_rst;
>>   	struct s3c_hsotg_ep     *eps;
>>   };
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 0d34cfc71bfb..c6c6cf982c90 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -2925,6 +2925,8 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
>>   	spin_lock_irqsave(&hsotg->lock, flags);
>>   	s3c_hsotg_init(hsotg);
>>   	s3c_hsotg_core_init_disconnected(hsotg);
>> +	hsotg->enabled = 1;
>> +	hsotg->connected = 0;
>>   	spin_unlock_irqrestore(&hsotg->lock, flags);
>>
>>   	dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
>> @@ -2961,6 +2963,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
>>
>>   	hsotg->driver = NULL;
>>   	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
>> +	hsotg->enabled = 0;
>> +	hsotg->connected = 0;
>>
>>   	spin_unlock_irqrestore(&hsotg->lock, flags);
>>
>> @@ -2999,11 +3003,14 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
>>   	dev_dbg(hsotg->dev, "%s: is_on: %d\n", __func__, is_on);
>>
>>   	spin_lock_irqsave(&hsotg->lock, flags);
>> +
>>   	if (is_on) {
>>   		clk_enable(hsotg->clk);
>> +		hsotg->connected = 1;
>>   		s3c_hsotg_core_connect(hsotg);
>>   	} else {
>>   		s3c_hsotg_core_disconnect(hsotg);
>> +		hsotg->connected = 0;
>>   		clk_disable(hsotg->clk);
>>   	}
>>
>> @@ -3652,16 +3659,18 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t state)
>>   		dev_info(hsotg->dev, "suspending usb gadget %s\n",
>>   			 hsotg->driver->driver.name);
>>
>> -	spin_lock_irqsave(&hsotg->lock, flags);
>> -	s3c_hsotg_core_disconnect(hsotg);
>> -	s3c_hsotg_disconnect(hsotg);
>> -	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
>> -	spin_unlock_irqrestore(&hsotg->lock, flags);
>> +	if (hsotg->enabled) {
> Hmm. Are you sure it's safe to check ->enabled outside of the spinlock?
> What happens if s3c_hsotg_udc_stop() runs right after this, before the
> spinlock is taken, and disables stuff? Sure, it's a tiny window, but
> still...

This code is called only in system suspend path, when userspace has been 
already frozen.
udc_stop() can be called only as a result of userspace action, so it is 
imho safe to
assume that the above code doesn't need additional locking.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2 10/10] usb: dwc2/gadget: rework suspend/resume code to correctly restore gadget state

[Marek Szyprowski]

Hello,

On 2014-10-27 08:18, Marek Szyprowski wrote:
>
> On 2014-10-25 03:16, Paul Zimmerman wrote:
>>> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
>>> Sent: Monday, October 20, 2014 3:46 AM
>>>
>>> Suspend/resume code assumed that the gadget was always enabled and
>>> connected to usb bus. This means that the actual state of the gadget
>>> (soft-enabled/disabled or connected/disconnected) was not correctly
>>> preserved on suspend/resume cycle. This patch fixes this issue.
>>>
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> ---
>>>   drivers/usb/dwc2/core.h   |  4 +++-
>>>   drivers/usb/dwc2/gadget.c | 43 
>>> +++++++++++++++++++++++++++----------------
>>>   2 files changed, 30 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>>> index bf015ab3b44c..3648b76a18b4 100644
>>> --- a/drivers/usb/dwc2/core.h
>>> +++ b/drivers/usb/dwc2/core.h
>>> @@ -210,7 +210,9 @@ struct s3c_hsotg {
>>>       u8                      ctrl_buff[8];
>>>
>>>       struct usb_gadget       gadget;
>>> -    unsigned int            setup;
>>> +    unsigned int            setup:1;
>>> +    unsigned int        connected:1;
>>> +    unsigned int        enabled:1;
>>>       unsigned long           last_rst;
>>>       struct s3c_hsotg_ep     *eps;
>>>   };
>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>>> index 0d34cfc71bfb..c6c6cf982c90 100644
>>> --- a/drivers/usb/dwc2/gadget.c
>>> +++ b/drivers/usb/dwc2/gadget.c
>>> @@ -2925,6 +2925,8 @@ static int s3c_hsotg_udc_start(struct 
>>> usb_gadget *gadget,
>>>       spin_lock_irqsave(&hsotg->lock, flags);
>>>       s3c_hsotg_init(hsotg);
>>>       s3c_hsotg_core_init_disconnected(hsotg);
>>> +    hsotg->enabled = 1;
>>> +    hsotg->connected = 0;
>>>       spin_unlock_irqrestore(&hsotg->lock, flags);
>>>
>>>       dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
>>> @@ -2961,6 +2963,8 @@ static int s3c_hsotg_udc_stop(struct 
>>> usb_gadget *gadget,
>>>
>>>       hsotg->driver = NULL;
>>>       hsotg->gadget.speed = USB_SPEED_UNKNOWN;
>>> +    hsotg->enabled = 0;
>>> +    hsotg->connected = 0;
>>>
>>>       spin_unlock_irqrestore(&hsotg->lock, flags);
>>>
>>> @@ -2999,11 +3003,14 @@ static int s3c_hsotg_pullup(struct 
>>> usb_gadget *gadget, int is_on)
>>>       dev_dbg(hsotg->dev, "%s: is_on: %d\n", __func__, is_on);
>>>
>>>       spin_lock_irqsave(&hsotg->lock, flags);
>>> +
>>>       if (is_on) {
>>>           clk_enable(hsotg->clk);
>>> +        hsotg->connected = 1;
>>>           s3c_hsotg_core_connect(hsotg);
>>>       } else {
>>>           s3c_hsotg_core_disconnect(hsotg);
>>> +        hsotg->connected = 0;
>>>           clk_disable(hsotg->clk);
>>>       }
>>>
>>> @@ -3652,16 +3659,18 @@ static int s3c_hsotg_suspend(struct 
>>> platform_device *pdev, pm_message_t state)
>>>           dev_info(hsotg->dev, "suspending usb gadget %s\n",
>>>                hsotg->driver->driver.name);
>>>
>>> -    spin_lock_irqsave(&hsotg->lock, flags);
>>> -    s3c_hsotg_core_disconnect(hsotg);
>>> -    s3c_hsotg_disconnect(hsotg);
>>> -    hsotg->gadget.speed = USB_SPEED_UNKNOWN;
>>> -    spin_unlock_irqrestore(&hsotg->lock, flags);
>>> +    if (hsotg->enabled) {
>> Hmm. Are you sure it's safe to check ->enabled outside of the spinlock?
>> What happens if s3c_hsotg_udc_stop() runs right after this, before the
>> spinlock is taken, and disables stuff? Sure, it's a tiny window, but
>> still...
>
> This code is called only in system suspend path, when userspace has 
> been already frozen.
> udc_stop() can be called only as a result of userspace action, so it 
> is imho safe to
> assume that the above code doesn't need additional locking.

However if you prefer the version with explicit locking, I will send v3 
in a few minutes.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls

[Marek Szyprowski]

This patch adds mutex, which protects initialization and
deinitialization procedures against suspend/resume methods.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/usb/dwc2/core.h   |  1 +
 drivers/usb/dwc2/gadget.c | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 9f77b4d1c5ff..58732a9a0019 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -187,6 +187,7 @@ struct s3c_hsotg {
 	struct s3c_hsotg_plat    *plat;
 
 	spinlock_t              lock;
+	struct mutex		init_mutex;
 
 	void __iomem            *regs;
 	int                     irq;
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index d8dda39c9e16..a2e4272a904e 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -21,6 +21,7 @@
 #include <linux/platform_device.h>
 #include <linux/dma-mapping.h>
 #include <linux/debugfs.h>
+#include <linux/mutex.h>
 #include <linux/seq_file.h>
 #include <linux/delay.h>
 #include <linux/io.h>
@@ -2908,6 +2909,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
 		return -EINVAL;
 	}
 
+	mutex_lock(&hsotg->init_mutex);
 	WARN_ON(hsotg->driver);
 
 	driver->driver.bus = NULL;
@@ -2933,9 +2935,12 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
 
 	dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
 
+	mutex_unlock(&hsotg->init_mutex);
+
 	return 0;
 
 err:
+	mutex_unlock(&hsotg->init_mutex);
 	hsotg->driver = NULL;
 	return ret;
 }
@@ -2957,6 +2962,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
 	if (!hsotg)
 		return -ENODEV;
 
+	mutex_lock(&hsotg->init_mutex);
+
 	/* all endpoints should be shutdown */
 	for (ep = 1; ep < hsotg->num_of_eps; ep++)
 		s3c_hsotg_ep_disable(&hsotg->eps[ep].ep);
@@ -2974,6 +2981,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
 
 	clk_disable(hsotg->clk);
 
+	mutex_unlock(&hsotg->init_mutex);
+
 	return 0;
 }
 
@@ -3002,6 +3011,7 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
 
 	dev_dbg(hsotg->dev, "%s: is_on: %d\n", __func__, is_on);
 
+	mutex_lock(&hsotg->init_mutex);
 	spin_lock_irqsave(&hsotg->lock, flags);
 	if (is_on) {
 		clk_enable(hsotg->clk);
@@ -3013,6 +3023,7 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
 
 	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
 	spin_unlock_irqrestore(&hsotg->lock, flags);
+	mutex_unlock(&hsotg->init_mutex);
 
 	return 0;
 }
@@ -3507,6 +3518,7 @@ static int s3c_hsotg_probe(struct platform_device *pdev)
 	}
 
 	spin_lock_init(&hsotg->lock);
+	mutex_init(&hsotg->init_mutex);
 
 	hsotg->irq = ret;
 
@@ -3652,6 +3664,8 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t state)
 	unsigned long flags;
 	int ret = 0;
 
+	mutex_lock(&hsotg->init_mutex);
+
 	if (hsotg->driver)
 		dev_info(hsotg->dev, "suspending usb gadget %s\n",
 			 hsotg->driver->driver.name);
@@ -3674,6 +3688,8 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t state)
 		clk_disable(hsotg->clk);
 	}
 
+	mutex_unlock(&hsotg->init_mutex);
+
 	return ret;
 }
 
@@ -3683,7 +3699,9 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
 	unsigned long flags;
 	int ret = 0;
 
+	mutex_lock(&hsotg->init_mutex);
 	if (hsotg->driver) {
+
 		dev_info(hsotg->dev, "resuming usb gadget %s\n",
 			 hsotg->driver->driver.name);
 
@@ -3699,6 +3717,8 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
 	s3c_hsotg_core_connect(hsotg);
 	spin_unlock_irqrestore(&hsotg->lock, flags);
 
+	mutex_unlock(&hsotg->init_mutex);
+
 	return ret;
 }
 
-- 
1.9.2

[PATCH v3 2/2] usb: dwc2/gadget: rework suspend/resume code to correctly restore gadget state

[Marek Szyprowski]

Suspend/resume code assumed that the gadget was always enabled and
connected to usb bus. This means that the actual state of the gadget
(soft-enabled/disabled or connected/disconnected) was not correctly
preserved on suspend/resume cycle. This patch fixes this issue.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/usb/dwc2/core.h   |  4 +++-
 drivers/usb/dwc2/gadget.c | 41 +++++++++++++++++++++++++----------------
 2 files changed, 28 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 58732a9a0019..a1aa9ecf052e 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -211,7 +211,9 @@ struct s3c_hsotg {
 	u8                      ctrl_buff[8];
 
 	struct usb_gadget       gadget;
-	unsigned int            setup;
+	unsigned int            setup:1;
+	unsigned int		connected:1;
+	unsigned int		enabled:1;
 	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 a2e4272a904e..e61bb1c4275d 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2931,6 +2931,8 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
 	spin_lock_irqsave(&hsotg->lock, flags);
 	s3c_hsotg_init(hsotg);
 	s3c_hsotg_core_init_disconnected(hsotg);
+	hsotg->enabled = 1;
+	hsotg->connected = 0;
 	spin_unlock_irqrestore(&hsotg->lock, flags);
 
 	dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
@@ -2972,6 +2974,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
 
 	hsotg->driver = NULL;
 	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
+	hsotg->enabled = 0;
+	hsotg->connected = 0;
 
 	spin_unlock_irqrestore(&hsotg->lock, flags);
 
@@ -3015,9 +3019,11 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
 	spin_lock_irqsave(&hsotg->lock, flags);
 	if (is_on) {
 		clk_enable(hsotg->clk);
+		hsotg->connected = 1;
 		s3c_hsotg_core_connect(hsotg);
 	} else {
 		s3c_hsotg_core_disconnect(hsotg);
+		hsotg->connected = 0;
 		clk_disable(hsotg->clk);
 	}
 
@@ -3670,16 +3676,18 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t state)
 		dev_info(hsotg->dev, "suspending usb gadget %s\n",
 			 hsotg->driver->driver.name);
 
-	spin_lock_irqsave(&hsotg->lock, flags);
-	s3c_hsotg_core_disconnect(hsotg);
-	s3c_hsotg_disconnect(hsotg);
-	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
-	spin_unlock_irqrestore(&hsotg->lock, flags);
+	if (hsotg->enabled) {
+		int ep;
 
-	s3c_hsotg_phy_disable(hsotg);
+		spin_lock_irqsave(&hsotg->lock, flags);
+		if (hsotg->connected)
+			s3c_hsotg_core_disconnect(hsotg);
+		s3c_hsotg_disconnect(hsotg);
+		hsotg->gadget.speed = USB_SPEED_UNKNOWN;
+		spin_unlock_irqrestore(&hsotg->lock, flags);
+
+		s3c_hsotg_phy_disable(hsotg);
 
-	if (hsotg->driver) {
-		int ep;
 		for (ep = 0; ep < hsotg->num_of_eps; ep++)
 			s3c_hsotg_ep_disable(&hsotg->eps[ep].ep);
 
@@ -3705,18 +3713,19 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
 		dev_info(hsotg->dev, "resuming usb gadget %s\n",
 			 hsotg->driver->driver.name);
 
+	if (hsotg->enabled) {
 		clk_enable(hsotg->clk);
 		ret = regulator_bulk_enable(ARRAY_SIZE(hsotg->supplies),
-				      hsotg->supplies);
-	}
-
-	s3c_hsotg_phy_enable(hsotg);
+					    hsotg->supplies);
 
-	spin_lock_irqsave(&hsotg->lock, flags);
-	s3c_hsotg_core_init_disconnected(hsotg);
-	s3c_hsotg_core_connect(hsotg);
-	spin_unlock_irqrestore(&hsotg->lock, flags);
+		s3c_hsotg_phy_enable(hsotg);
 
+		spin_lock_irqsave(&hsotg->lock, flags);
+		s3c_hsotg_core_init_disconnected(hsotg);
+		if (hsotg->connected)
+			s3c_hsotg_core_connect(hsotg);
+		spin_unlock_irqrestore(&hsotg->lock, flags);
+	}
 	mutex_unlock(&hsotg->init_mutex);
 
 	return ret;
-- 
1.9.2

RE: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls

[Paul Zimmerman]

> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
> Sent: Friday, October 31, 2014 3:13 AM
> 
> This patch adds mutex, which protects initialization and
> deinitialization procedures against suspend/resume methods.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/usb/dwc2/core.h   |  1 +
>  drivers/usb/dwc2/gadget.c | 20 ++++++++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 9f77b4d1c5ff..58732a9a0019 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -187,6 +187,7 @@ struct s3c_hsotg {
>  	struct s3c_hsotg_plat    *plat;
> 
>  	spinlock_t              lock;
> +	struct mutex		init_mutex;
> 
>  	void __iomem            *regs;
>  	int                     irq;
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index d8dda39c9e16..a2e4272a904e 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -21,6 +21,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/debugfs.h>
> +#include <linux/mutex.h>
>  #include <linux/seq_file.h>
>  #include <linux/delay.h>
>  #include <linux/io.h>
> @@ -2908,6 +2909,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
>  		return -EINVAL;
>  	}
> 
> +	mutex_lock(&hsotg->init_mutex);
>  	WARN_ON(hsotg->driver);
> 
>  	driver->driver.bus = NULL;
> @@ -2933,9 +2935,12 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
> 
>  	dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
> 
> +	mutex_unlock(&hsotg->init_mutex);
> +
>  	return 0;
> 
>  err:
> +	mutex_unlock(&hsotg->init_mutex);
>  	hsotg->driver = NULL;
>  	return ret;
>  }
> @@ -2957,6 +2962,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
>  	if (!hsotg)
>  		return -ENODEV;
> 
> +	mutex_lock(&hsotg->init_mutex);
> +
>  	/* all endpoints should be shutdown */
>  	for (ep = 1; ep < hsotg->num_of_eps; ep++)
>  		s3c_hsotg_ep_disable(&hsotg->eps[ep].ep);
> @@ -2974,6 +2981,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
> 
>  	clk_disable(hsotg->clk);
> 
> +	mutex_unlock(&hsotg->init_mutex);
> +
>  	return 0;
>  }
> 
> @@ -3002,6 +3011,7 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
> 
>  	dev_dbg(hsotg->dev, "%s: is_on: %d\n", __func__, is_on);
> 
> +	mutex_lock(&hsotg->init_mutex);
>  	spin_lock_irqsave(&hsotg->lock, flags);
>  	if (is_on) {
>  		clk_enable(hsotg->clk);
> @@ -3013,6 +3023,7 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
> 
>  	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
>  	spin_unlock_irqrestore(&hsotg->lock, flags);
> +	mutex_unlock(&hsotg->init_mutex);
> 
>  	return 0;
>  }
> @@ -3507,6 +3518,7 @@ static int s3c_hsotg_probe(struct platform_device *pdev)
>  	}
> 
>  	spin_lock_init(&hsotg->lock);
> +	mutex_init(&hsotg->init_mutex);
> 
>  	hsotg->irq = ret;
> 
> @@ -3652,6 +3664,8 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t state)
>  	unsigned long flags;
>  	int ret = 0;
> 
> +	mutex_lock(&hsotg->init_mutex);
> +
>  	if (hsotg->driver)
>  		dev_info(hsotg->dev, "suspending usb gadget %s\n",
>  			 hsotg->driver->driver.name);
> @@ -3674,6 +3688,8 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t state)
>  		clk_disable(hsotg->clk);
>  	}
> 
> +	mutex_unlock(&hsotg->init_mutex);
> +
>  	return ret;
>  }
> 
> @@ -3683,7 +3699,9 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
>  	unsigned long flags;
>  	int ret = 0;
> 
> +	mutex_lock(&hsotg->init_mutex);
>  	if (hsotg->driver) {
> +
>  		dev_info(hsotg->dev, "resuming usb gadget %s\n",
>  			 hsotg->driver->driver.name);
> 
> @@ -3699,6 +3717,8 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
>  	s3c_hsotg_core_connect(hsotg);
>  	spin_unlock_irqrestore(&hsotg->lock, flags);
> 
> +	mutex_unlock(&hsotg->init_mutex);
> +
>  	return ret;
>  }
> 

Hmm. I can't find any other UDC driver that uses a mutex in its
suspend/resume functions. Can you explain why this is needed only
for dwc2?

-- 
Paul

Re: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls

[Marek Szyprowski]

Hello,

On 2014-10-31 19:46, Paul Zimmerman wrote:
>> From: Marek Szyprowski [mailto:m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org]
>> Sent: Friday, October 31, 2014 3:13 AM
>>
>> This patch adds mutex, which protects initialization and
>> deinitialization procedures against suspend/resume methods.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> ---
>>   drivers/usb/dwc2/core.h   |  1 +
>>   drivers/usb/dwc2/gadget.c | 20 ++++++++++++++++++++
>>   2 files changed, 21 insertions(+)
>>
>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>> index 9f77b4d1c5ff..58732a9a0019 100644
>> --- a/drivers/usb/dwc2/core.h
>> +++ b/drivers/usb/dwc2/core.h
>> @@ -187,6 +187,7 @@ struct s3c_hsotg {
>>   	struct s3c_hsotg_plat    *plat;
>>
>>   	spinlock_t              lock;
>> +	struct mutex		init_mutex;
>>
>>   	void __iomem            *regs;
>>   	int                     irq;
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index d8dda39c9e16..a2e4272a904e 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -21,6 +21,7 @@
>>   #include <linux/platform_device.h>
>>   #include <linux/dma-mapping.h>
>>   #include <linux/debugfs.h>
>> +#include <linux/mutex.h>
>>   #include <linux/seq_file.h>
>>   #include <linux/delay.h>
>>   #include <linux/io.h>
>> @@ -2908,6 +2909,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
>>   		return -EINVAL;
>>   	}
>>
>> +	mutex_lock(&hsotg->init_mutex);
>>   	WARN_ON(hsotg->driver);
>>
>>   	driver->driver.bus = NULL;
>> @@ -2933,9 +2935,12 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
>>
>>   	dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
>>
>> +	mutex_unlock(&hsotg->init_mutex);
>> +
>>   	return 0;
>>
>>   err:
>> +	mutex_unlock(&hsotg->init_mutex);
>>   	hsotg->driver = NULL;
>>   	return ret;
>>   }
>> @@ -2957,6 +2962,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
>>   	if (!hsotg)
>>   		return -ENODEV;
>>
>> +	mutex_lock(&hsotg->init_mutex);
>> +
>>   	/* all endpoints should be shutdown */
>>   	for (ep = 1; ep < hsotg->num_of_eps; ep++)
>>   		s3c_hsotg_ep_disable(&hsotg->eps[ep].ep);
>> @@ -2974,6 +2981,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
>>
>>   	clk_disable(hsotg->clk);
>>
>> +	mutex_unlock(&hsotg->init_mutex);
>> +
>>   	return 0;
>>   }
>>
>> @@ -3002,6 +3011,7 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
>>
>>   	dev_dbg(hsotg->dev, "%s: is_on: %d\n", __func__, is_on);
>>
>> +	mutex_lock(&hsotg->init_mutex);
>>   	spin_lock_irqsave(&hsotg->lock, flags);
>>   	if (is_on) {
>>   		clk_enable(hsotg->clk);
>> @@ -3013,6 +3023,7 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
>>
>>   	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
>>   	spin_unlock_irqrestore(&hsotg->lock, flags);
>> +	mutex_unlock(&hsotg->init_mutex);
>>
>>   	return 0;
>>   }
>> @@ -3507,6 +3518,7 @@ static int s3c_hsotg_probe(struct platform_device *pdev)
>>   	}
>>
>>   	spin_lock_init(&hsotg->lock);
>> +	mutex_init(&hsotg->init_mutex);
>>
>>   	hsotg->irq = ret;
>>
>> @@ -3652,6 +3664,8 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t state)
>>   	unsigned long flags;
>>   	int ret = 0;
>>
>> +	mutex_lock(&hsotg->init_mutex);
>> +
>>   	if (hsotg->driver)
>>   		dev_info(hsotg->dev, "suspending usb gadget %s\n",
>>   			 hsotg->driver->driver.name);
>> @@ -3674,6 +3688,8 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t state)
>>   		clk_disable(hsotg->clk);
>>   	}
>>
>> +	mutex_unlock(&hsotg->init_mutex);
>> +
>>   	return ret;
>>   }
>>
>> @@ -3683,7 +3699,9 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
>>   	unsigned long flags;
>>   	int ret = 0;
>>
>> +	mutex_lock(&hsotg->init_mutex);
>>   	if (hsotg->driver) {
>> +
>>   		dev_info(hsotg->dev, "resuming usb gadget %s\n",
>>   			 hsotg->driver->driver.name);
>>
>> @@ -3699,6 +3717,8 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
>>   	s3c_hsotg_core_connect(hsotg);
>>   	spin_unlock_irqrestore(&hsotg->lock, flags);
>>
>> +	mutex_unlock(&hsotg->init_mutex);
>> +
>>   	return ret;
>>   }
>>
> Hmm. I can't find any other UDC driver that uses a mutex in its
> suspend/resume functions. Can you explain why this is needed only
> for dwc2?

I've posted this version because I thought you were not convinced that 
the patch
"usb: dwc2/gadget: rework suspend/resume code to correctly restore 
gadget state"
can add code for initialization and deinitialization in suspend/resume 
paths.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

--
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 v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls

[Paul Zimmerman]

> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
> Sent: Thursday, November 13, 2014 7:18 AM
> 
> On 2014-10-31 19:46, Paul Zimmerman wrote:
> >> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
> >> Sent: Friday, October 31, 2014 3:13 AM
> >>
> >> This patch adds mutex, which protects initialization and
> >> deinitialization procedures against suspend/resume methods.
> >>
> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >> ---
> >>   drivers/usb/dwc2/core.h   |  1 +
> >>   drivers/usb/dwc2/gadget.c | 20 ++++++++++++++++++++
> >>   2 files changed, 21 insertions(+)
> >>
> >> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> >> index 9f77b4d1c5ff..58732a9a0019 100644
> >> --- a/drivers/usb/dwc2/core.h
> >> +++ b/drivers/usb/dwc2/core.h
> >> @@ -187,6 +187,7 @@ struct s3c_hsotg {
> >>   	struct s3c_hsotg_plat    *plat;
> >>
> >>   	spinlock_t              lock;
> >> +	struct mutex		init_mutex;
> >>
> >>   	void __iomem            *regs;
> >>   	int                     irq;
> >> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> >> index d8dda39c9e16..a2e4272a904e 100644
> >> --- a/drivers/usb/dwc2/gadget.c
> >> +++ b/drivers/usb/dwc2/gadget.c
> >> @@ -21,6 +21,7 @@
> >>   #include <linux/platform_device.h>
> >>   #include <linux/dma-mapping.h>
> >>   #include <linux/debugfs.h>
> >> +#include <linux/mutex.h>
> >>   #include <linux/seq_file.h>
> >>   #include <linux/delay.h>
> >>   #include <linux/io.h>
> >> @@ -2908,6 +2909,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
> >>   		return -EINVAL;
> >>   	}
> >>
> >> +	mutex_lock(&hsotg->init_mutex);
> >>   	WARN_ON(hsotg->driver);
> >>
> >>   	driver->driver.bus = NULL;
> >> @@ -2933,9 +2935,12 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
> >>
> >>   	dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
> >>
> >> +	mutex_unlock(&hsotg->init_mutex);
> >> +
> >>   	return 0;
> >>
> >>   err:
> >> +	mutex_unlock(&hsotg->init_mutex);
> >>   	hsotg->driver = NULL;
> >>   	return ret;
> >>   }
> >> @@ -2957,6 +2962,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
> >>   	if (!hsotg)
> >>   		return -ENODEV;
> >>
> >> +	mutex_lock(&hsotg->init_mutex);
> >> +
> >>   	/* all endpoints should be shutdown */
> >>   	for (ep = 1; ep < hsotg->num_of_eps; ep++)
> >>   		s3c_hsotg_ep_disable(&hsotg->eps[ep].ep);
> >> @@ -2974,6 +2981,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
> >>
> >>   	clk_disable(hsotg->clk);
> >>
> >> +	mutex_unlock(&hsotg->init_mutex);
> >> +
> >>   	return 0;
> >>   }
> >>
> >> @@ -3002,6 +3011,7 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
> >>
> >>   	dev_dbg(hsotg->dev, "%s: is_on: %d\n", __func__, is_on);
> >>
> >> +	mutex_lock(&hsotg->init_mutex);
> >>   	spin_lock_irqsave(&hsotg->lock, flags);
> >>   	if (is_on) {
> >>   		clk_enable(hsotg->clk);
> >> @@ -3013,6 +3023,7 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
> >>
> >>   	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
> >>   	spin_unlock_irqrestore(&hsotg->lock, flags);
> >> +	mutex_unlock(&hsotg->init_mutex);
> >>
> >>   	return 0;
> >>   }
> >> @@ -3507,6 +3518,7 @@ static int s3c_hsotg_probe(struct platform_device *pdev)
> >>   	}
> >>
> >>   	spin_lock_init(&hsotg->lock);
> >> +	mutex_init(&hsotg->init_mutex);
> >>
> >>   	hsotg->irq = ret;
> >>
> >> @@ -3652,6 +3664,8 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t
> state)
> >>   	unsigned long flags;
> >>   	int ret = 0;
> >>
> >> +	mutex_lock(&hsotg->init_mutex);
> >> +
> >>   	if (hsotg->driver)
> >>   		dev_info(hsotg->dev, "suspending usb gadget %s\n",
> >>   			 hsotg->driver->driver.name);
> >> @@ -3674,6 +3688,8 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t
> state)
> >>   		clk_disable(hsotg->clk);
> >>   	}
> >>
> >> +	mutex_unlock(&hsotg->init_mutex);
> >> +
> >>   	return ret;
> >>   }
> >>
> >> @@ -3683,7 +3699,9 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
> >>   	unsigned long flags;
> >>   	int ret = 0;
> >>
> >> +	mutex_lock(&hsotg->init_mutex);
> >>   	if (hsotg->driver) {
> >> +
> >>   		dev_info(hsotg->dev, "resuming usb gadget %s\n",
> >>   			 hsotg->driver->driver.name);
> >>
> >> @@ -3699,6 +3717,8 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
> >>   	s3c_hsotg_core_connect(hsotg);
> >>   	spin_unlock_irqrestore(&hsotg->lock, flags);
> >>
> >> +	mutex_unlock(&hsotg->init_mutex);
> >> +
> >>   	return ret;
> >>   }
> >>
> > Hmm. I can't find any other UDC driver that uses a mutex in its
> > suspend/resume functions. Can you explain why this is needed only
> > for dwc2?
> 
> I've posted this version because I thought you were not convinced that
> the patch
> "usb: dwc2/gadget: rework suspend/resume code to correctly restore
> gadget state"
> can add code for initialization and deinitialization in suspend/resume
> paths.

My problem with that patch was that you were checking the ->enabled
flag outside of the spinlock. To address that, you only need to move
the check inside of the spinlock. I don't see why a mutex is needed.

-- 
Paul

Re: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls

[Marek Szyprowski]

Hello,

On 2014-11-13 21:55, Paul Zimmerman wrote:
>> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
>> Sent: Thursday, November 13, 2014 7:18 AM
>>
>> On 2014-10-31 19:46, Paul Zimmerman wrote:
>>>> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
>>>> Sent: Friday, October 31, 2014 3:13 AM
>>>>
>>>> This patch adds mutex, which protects initialization and
>>>> deinitialization procedures against suspend/resume methods.
>>>>
>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>> ---
>>>>    drivers/usb/dwc2/core.h   |  1 +
>>>>    drivers/usb/dwc2/gadget.c | 20 ++++++++++++++++++++
>>>>    2 files changed, 21 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>>>> index 9f77b4d1c5ff..58732a9a0019 100644
>>>> --- a/drivers/usb/dwc2/core.h
>>>> +++ b/drivers/usb/dwc2/core.h
>>>> @@ -187,6 +187,7 @@ struct s3c_hsotg {
>>>>    	struct s3c_hsotg_plat    *plat;
>>>>
>>>>    	spinlock_t              lock;
>>>> +	struct mutex		init_mutex;
>>>>
>>>>    	void __iomem            *regs;
>>>>    	int                     irq;
>>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>>>> index d8dda39c9e16..a2e4272a904e 100644
>>>> --- a/drivers/usb/dwc2/gadget.c
>>>> +++ b/drivers/usb/dwc2/gadget.c
>>>> @@ -21,6 +21,7 @@
>>>>    #include <linux/platform_device.h>
>>>>    #include <linux/dma-mapping.h>
>>>>    #include <linux/debugfs.h>
>>>> +#include <linux/mutex.h>
>>>>    #include <linux/seq_file.h>
>>>>    #include <linux/delay.h>
>>>>    #include <linux/io.h>
>>>> @@ -2908,6 +2909,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
>>>>    		return -EINVAL;
>>>>    	}
>>>>
>>>> +	mutex_lock(&hsotg->init_mutex);
>>>>    	WARN_ON(hsotg->driver);
>>>>
>>>>    	driver->driver.bus = NULL;
>>>> @@ -2933,9 +2935,12 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
>>>>
>>>>    	dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
>>>>
>>>> +	mutex_unlock(&hsotg->init_mutex);
>>>> +
>>>>    	return 0;
>>>>
>>>>    err:
>>>> +	mutex_unlock(&hsotg->init_mutex);
>>>>    	hsotg->driver = NULL;
>>>>    	return ret;
>>>>    }
>>>> @@ -2957,6 +2962,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
>>>>    	if (!hsotg)
>>>>    		return -ENODEV;
>>>>
>>>> +	mutex_lock(&hsotg->init_mutex);
>>>> +
>>>>    	/* all endpoints should be shutdown */
>>>>    	for (ep = 1; ep < hsotg->num_of_eps; ep++)
>>>>    		s3c_hsotg_ep_disable(&hsotg->eps[ep].ep);
>>>> @@ -2974,6 +2981,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
>>>>
>>>>    	clk_disable(hsotg->clk);
>>>>
>>>> +	mutex_unlock(&hsotg->init_mutex);
>>>> +
>>>>    	return 0;
>>>>    }
>>>>
>>>> @@ -3002,6 +3011,7 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
>>>>
>>>>    	dev_dbg(hsotg->dev, "%s: is_on: %d\n", __func__, is_on);
>>>>
>>>> +	mutex_lock(&hsotg->init_mutex);
>>>>    	spin_lock_irqsave(&hsotg->lock, flags);
>>>>    	if (is_on) {
>>>>    		clk_enable(hsotg->clk);
>>>> @@ -3013,6 +3023,7 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
>>>>
>>>>    	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
>>>>    	spin_unlock_irqrestore(&hsotg->lock, flags);
>>>> +	mutex_unlock(&hsotg->init_mutex);
>>>>
>>>>    	return 0;
>>>>    }
>>>> @@ -3507,6 +3518,7 @@ static int s3c_hsotg_probe(struct platform_device *pdev)
>>>>    	}
>>>>
>>>>    	spin_lock_init(&hsotg->lock);
>>>> +	mutex_init(&hsotg->init_mutex);
>>>>
>>>>    	hsotg->irq = ret;
>>>>
>>>> @@ -3652,6 +3664,8 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t
>> state)
>>>>    	unsigned long flags;
>>>>    	int ret = 0;
>>>>
>>>> +	mutex_lock(&hsotg->init_mutex);
>>>> +
>>>>    	if (hsotg->driver)
>>>>    		dev_info(hsotg->dev, "suspending usb gadget %s\n",
>>>>    			 hsotg->driver->driver.name);
>>>> @@ -3674,6 +3688,8 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t
>> state)
>>>>    		clk_disable(hsotg->clk);
>>>>    	}
>>>>
>>>> +	mutex_unlock(&hsotg->init_mutex);
>>>> +
>>>>    	return ret;
>>>>    }
>>>>
>>>> @@ -3683,7 +3699,9 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
>>>>    	unsigned long flags;
>>>>    	int ret = 0;
>>>>
>>>> +	mutex_lock(&hsotg->init_mutex);
>>>>    	if (hsotg->driver) {
>>>> +
>>>>    		dev_info(hsotg->dev, "resuming usb gadget %s\n",
>>>>    			 hsotg->driver->driver.name);
>>>>
>>>> @@ -3699,6 +3717,8 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
>>>>    	s3c_hsotg_core_connect(hsotg);
>>>>    	spin_unlock_irqrestore(&hsotg->lock, flags);
>>>>
>>>> +	mutex_unlock(&hsotg->init_mutex);
>>>> +
>>>>    	return ret;
>>>>    }
>>>>
>>> Hmm. I can't find any other UDC driver that uses a mutex in its
>>> suspend/resume functions. Can you explain why this is needed only
>>> for dwc2?
>> I've posted this version because I thought you were not convinced that
>> the patch
>> "usb: dwc2/gadget: rework suspend/resume code to correctly restore
>> gadget state"
>> can add code for initialization and deinitialization in suspend/resume
>> paths.
> My problem with that patch was that you were checking the ->enabled
> flag outside of the spinlock. To address that, you only need to move
> the check inside of the spinlock. I don't see why a mutex is needed.

It is not that simple. I can add spin_lock() before checking enabled, 
but then
I would need to spin_unlock() to call regulator_bulk_enable() and 
phy_enable(),
because both cannot be called from atomic context. This means that the 
spinlock
in such case will not protect anything and is simply useless.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

RE: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls

[Paul Zimmerman]

> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
> Sent: Friday, November 14, 2014 1:19 AM
> 
> On 2014-11-13 21:55, Paul Zimmerman wrote:
> >> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
> >> Sent: Thursday, November 13, 2014 7:18 AM
> >>
> >> On 2014-10-31 19:46, Paul Zimmerman wrote:
> >>>> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
> >>>> Sent: Friday, October 31, 2014 3:13 AM
> >>>>
> >>>> This patch adds mutex, which protects initialization and
> >>>> deinitialization procedures against suspend/resume methods.
> >>>>
> >>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >>>> ---
> >>>>    drivers/usb/dwc2/core.h   |  1 +
> >>>>    drivers/usb/dwc2/gadget.c | 20 ++++++++++++++++++++
> >>>>    2 files changed, 21 insertions(+)
> >>>>
> >>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> >>>> index 9f77b4d1c5ff..58732a9a0019 100644
> >>>> --- a/drivers/usb/dwc2/core.h
> >>>> +++ b/drivers/usb/dwc2/core.h
> >>>> @@ -187,6 +187,7 @@ struct s3c_hsotg {
> >>>>    	struct s3c_hsotg_plat    *plat;
> >>>>
> >>>>    	spinlock_t              lock;
> >>>> +	struct mutex		init_mutex;
> >>>>
> >>>>    	void __iomem            *regs;
> >>>>    	int                     irq;
> >>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> >>>> index d8dda39c9e16..a2e4272a904e 100644
> >>>> --- a/drivers/usb/dwc2/gadget.c
> >>>> +++ b/drivers/usb/dwc2/gadget.c
> >>>> @@ -21,6 +21,7 @@
> >>>>    #include <linux/platform_device.h>
> >>>>    #include <linux/dma-mapping.h>
> >>>>    #include <linux/debugfs.h>
> >>>> +#include <linux/mutex.h>
> >>>>    #include <linux/seq_file.h>
> >>>>    #include <linux/delay.h>
> >>>>    #include <linux/io.h>
> >>>> @@ -2908,6 +2909,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
> >>>>    		return -EINVAL;
> >>>>    	}
> >>>>
> >>>> +	mutex_lock(&hsotg->init_mutex);
> >>>>    	WARN_ON(hsotg->driver);
> >>>>
> >>>>    	driver->driver.bus = NULL;
> >>>> @@ -2933,9 +2935,12 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
> >>>>
> >>>>    	dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
> >>>>
> >>>> +	mutex_unlock(&hsotg->init_mutex);
> >>>> +
> >>>>    	return 0;
> >>>>
> >>>>    err:
> >>>> +	mutex_unlock(&hsotg->init_mutex);
> >>>>    	hsotg->driver = NULL;
> >>>>    	return ret;
> >>>>    }
> >>>> @@ -2957,6 +2962,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
> >>>>    	if (!hsotg)
> >>>>    		return -ENODEV;
> >>>>
> >>>> +	mutex_lock(&hsotg->init_mutex);
> >>>> +
> >>>>    	/* all endpoints should be shutdown */
> >>>>    	for (ep = 1; ep < hsotg->num_of_eps; ep++)
> >>>>    		s3c_hsotg_ep_disable(&hsotg->eps[ep].ep);
> >>>> @@ -2974,6 +2981,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
> >>>>
> >>>>    	clk_disable(hsotg->clk);
> >>>>
> >>>> +	mutex_unlock(&hsotg->init_mutex);
> >>>> +
> >>>>    	return 0;
> >>>>    }
> >>>>
> >>>> @@ -3002,6 +3011,7 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
> >>>>
> >>>>    	dev_dbg(hsotg->dev, "%s: is_on: %d\n", __func__, is_on);
> >>>>
> >>>> +	mutex_lock(&hsotg->init_mutex);
> >>>>    	spin_lock_irqsave(&hsotg->lock, flags);
> >>>>    	if (is_on) {
> >>>>    		clk_enable(hsotg->clk);
> >>>> @@ -3013,6 +3023,7 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
> >>>>
> >>>>    	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
> >>>>    	spin_unlock_irqrestore(&hsotg->lock, flags);
> >>>> +	mutex_unlock(&hsotg->init_mutex);
> >>>>
> >>>>    	return 0;
> >>>>    }
> >>>> @@ -3507,6 +3518,7 @@ static int s3c_hsotg_probe(struct platform_device *pdev)
> >>>>    	}
> >>>>
> >>>>    	spin_lock_init(&hsotg->lock);
> >>>> +	mutex_init(&hsotg->init_mutex);
> >>>>
> >>>>    	hsotg->irq = ret;
> >>>>
> >>>> @@ -3652,6 +3664,8 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t
> >> state)
> >>>>    	unsigned long flags;
> >>>>    	int ret = 0;
> >>>>
> >>>> +	mutex_lock(&hsotg->init_mutex);
> >>>> +
> >>>>    	if (hsotg->driver)
> >>>>    		dev_info(hsotg->dev, "suspending usb gadget %s\n",
> >>>>    			 hsotg->driver->driver.name);
> >>>> @@ -3674,6 +3688,8 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t
> >> state)
> >>>>    		clk_disable(hsotg->clk);
> >>>>    	}
> >>>>
> >>>> +	mutex_unlock(&hsotg->init_mutex);
> >>>> +
> >>>>    	return ret;
> >>>>    }
> >>>>
> >>>> @@ -3683,7 +3699,9 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
> >>>>    	unsigned long flags;
> >>>>    	int ret = 0;
> >>>>
> >>>> +	mutex_lock(&hsotg->init_mutex);
> >>>>    	if (hsotg->driver) {
> >>>> +
> >>>>    		dev_info(hsotg->dev, "resuming usb gadget %s\n",
> >>>>    			 hsotg->driver->driver.name);
> >>>>
> >>>> @@ -3699,6 +3717,8 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
> >>>>    	s3c_hsotg_core_connect(hsotg);
> >>>>    	spin_unlock_irqrestore(&hsotg->lock, flags);
> >>>>
> >>>> +	mutex_unlock(&hsotg->init_mutex);
> >>>> +
> >>>>    	return ret;
> >>>>    }
> >>>>
> >>> Hmm. I can't find any other UDC driver that uses a mutex in its
> >>> suspend/resume functions. Can you explain why this is needed only
> >>> for dwc2?
> >> I've posted this version because I thought you were not convinced that
> >> the patch
> >> "usb: dwc2/gadget: rework suspend/resume code to correctly restore
> >> gadget state"
> >> can add code for initialization and deinitialization in suspend/resume
> >> paths.
> > My problem with that patch was that you were checking the ->enabled
> > flag outside of the spinlock. To address that, you only need to move
> > the check inside of the spinlock. I don't see why a mutex is needed.
> 
> It is not that simple. I can add spin_lock() before checking enabled,
> but then
> I would need to spin_unlock() to call regulator_bulk_enable() and
> phy_enable(),
> because both cannot be called from atomic context. This means that the
> spinlock
> in such case will not protect anything and is simply useless.

Ah, OK. So you're using the mutex instead of the ->enabled flag that you
proposed in the "rework suspend/resume code" patch. So this patch is a
replacement for that one. Somehow I was thinking this patch was on top
of that one.

So I guess this is OK, but I would like to get Felipe's opinion about
it before we apply this.

Felipe?

-- 
Paul

Re: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls

[Felipe Balbi]

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

Hi,

On Fri, Nov 14, 2014 at 07:43:23PM +0000, Paul Zimmerman wrote:
> > >>>> @@ -3699,6 +3717,8 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
> > >>>>    	s3c_hsotg_core_connect(hsotg);
> > >>>>    	spin_unlock_irqrestore(&hsotg->lock, flags);
> > >>>>
> > >>>> +	mutex_unlock(&hsotg->init_mutex);
> > >>>> +
> > >>>>    	return ret;
> > >>>>    }
> > >>>>
> > >>> Hmm. I can't find any other UDC driver that uses a mutex in its
> > >>> suspend/resume functions. Can you explain why this is needed only
> > >>> for dwc2?
> > >> I've posted this version because I thought you were not convinced that
> > >> the patch
> > >> "usb: dwc2/gadget: rework suspend/resume code to correctly restore
> > >> gadget state"
> > >> can add code for initialization and deinitialization in suspend/resume
> > >> paths.
> > > My problem with that patch was that you were checking the ->enabled
> > > flag outside of the spinlock. To address that, you only need to move
> > > the check inside of the spinlock. I don't see why a mutex is needed.
> > 
> > It is not that simple. I can add spin_lock() before checking enabled,
> > but then
> > I would need to spin_unlock() to call regulator_bulk_enable() and
> > phy_enable(),
> > because both cannot be called from atomic context. This means that the
> > spinlock
> > in such case will not protect anything and is simply useless.
> 
> Ah, OK. So you're using the mutex instead of the ->enabled flag that you
> proposed in the "rework suspend/resume code" patch. So this patch is a
> replacement for that one. Somehow I was thinking this patch was on top
> of that one.
> 
> So I guess this is OK, but I would like to get Felipe's opinion about
> it before we apply this.
> 
> Felipe?

I can't think of a better way, I'm afraid :-(

-- 
balbi

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

RE: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls

[Paul Zimmerman]

> From: Marek Szyprowski [mailto:m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org]
> Sent: Friday, October 31, 2014 3:13 AM
> 
> This patch adds mutex, which protects initialization and
> deinitialization procedures against suspend/resume methods.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/usb/dwc2/core.h   |  1 +
>  drivers/usb/dwc2/gadget.c | 20 ++++++++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 9f77b4d1c5ff..58732a9a0019 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -187,6 +187,7 @@ struct s3c_hsotg {
>  	struct s3c_hsotg_plat    *plat;
> 
>  	spinlock_t              lock;
> +	struct mutex		init_mutex;
> 
>  	void __iomem            *regs;
>  	int                     irq;
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index d8dda39c9e16..a2e4272a904e 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -21,6 +21,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/debugfs.h>
> +#include <linux/mutex.h>
>  #include <linux/seq_file.h>
>  #include <linux/delay.h>
>  #include <linux/io.h>
> @@ -2908,6 +2909,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
>  		return -EINVAL;
>  	}
> 
> +	mutex_lock(&hsotg->init_mutex);
>  	WARN_ON(hsotg->driver);
> 
>  	driver->driver.bus = NULL;
> @@ -2933,9 +2935,12 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
> 
>  	dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
> 
> +	mutex_unlock(&hsotg->init_mutex);
> +
>  	return 0;
> 
>  err:
> +	mutex_unlock(&hsotg->init_mutex);
>  	hsotg->driver = NULL;
>  	return ret;
>  }
> @@ -2957,6 +2962,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
>  	if (!hsotg)
>  		return -ENODEV;
> 
> +	mutex_lock(&hsotg->init_mutex);
> +
>  	/* all endpoints should be shutdown */
>  	for (ep = 1; ep < hsotg->num_of_eps; ep++)
>  		s3c_hsotg_ep_disable(&hsotg->eps[ep].ep);
> @@ -2974,6 +2981,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
> 
>  	clk_disable(hsotg->clk);
> 
> +	mutex_unlock(&hsotg->init_mutex);
> +
>  	return 0;
>  }
> 
> @@ -3002,6 +3011,7 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
> 
>  	dev_dbg(hsotg->dev, "%s: is_on: %d\n", __func__, is_on);
> 
> +	mutex_lock(&hsotg->init_mutex);
>  	spin_lock_irqsave(&hsotg->lock, flags);
>  	if (is_on) {
>  		clk_enable(hsotg->clk);
> @@ -3013,6 +3023,7 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
> 
>  	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
>  	spin_unlock_irqrestore(&hsotg->lock, flags);
> +	mutex_unlock(&hsotg->init_mutex);
> 
>  	return 0;
>  }
> @@ -3507,6 +3518,7 @@ static int s3c_hsotg_probe(struct platform_device *pdev)
>  	}
> 
>  	spin_lock_init(&hsotg->lock);
> +	mutex_init(&hsotg->init_mutex);
> 
>  	hsotg->irq = ret;
> 
> @@ -3652,6 +3664,8 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t state)
>  	unsigned long flags;
>  	int ret = 0;
> 
> +	mutex_lock(&hsotg->init_mutex);
> +
>  	if (hsotg->driver)
>  		dev_info(hsotg->dev, "suspending usb gadget %s\n",
>  			 hsotg->driver->driver.name);
> @@ -3674,6 +3688,8 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t state)
>  		clk_disable(hsotg->clk);
>  	}
> 
> +	mutex_unlock(&hsotg->init_mutex);
> +
>  	return ret;
>  }
> 
> @@ -3683,7 +3699,9 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
>  	unsigned long flags;
>  	int ret = 0;
> 
> +	mutex_lock(&hsotg->init_mutex);
>  	if (hsotg->driver) {
> +
>  		dev_info(hsotg->dev, "resuming usb gadget %s\n",
>  			 hsotg->driver->driver.name);
> 
> @@ -3699,6 +3717,8 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
>  	s3c_hsotg_core_connect(hsotg);
>  	spin_unlock_irqrestore(&hsotg->lock, flags);
> 
> +	mutex_unlock(&hsotg->init_mutex);
> +
>  	return ret;
>  }
> 

Acked-by: Paul Zimmerman <paulz-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>

--
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 v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls

[Felipe Balbi]

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

On Fri, Oct 31, 2014 at 11:12:33AM +0100, Marek Szyprowski wrote:
> This patch adds mutex, which protects initialization and
> deinitialization procedures against suspend/resume methods.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

doesn't apply either:

checking file drivers/usb/dwc2/core.h
Hunk #1 FAILED at 187.
1 out of 1 hunk FAILED
checking file drivers/usb/dwc2/gadget.c
Hunk #2 succeeded at 2863 (offset -46 lines).
Hunk #3 succeeded at 2889 (offset -46 lines).
Hunk #4 succeeded at 2915 (offset -47 lines).
Hunk #5 succeeded at 2934 (offset -47 lines).
Hunk #6 succeeded at 2964 (offset -47 lines).
Hunk #7 succeeded at 2976 (offset -47 lines).
Hunk #8 FAILED at 3518.
Hunk #9 succeeded at 3579 (offset -84 lines).
Hunk #10 succeeded at 3603 with fuzz 1 (offset -84 lines).
Hunk #11 succeeded at 3614 (offset -84 lines).
Hunk #12 succeeded at 3632 with fuzz 1 (offset -84 lines).
1 out of 12 hunks FAILED

please rebase on testing/next. Also, add Paul's Ack when doing so ;-)

thanks

-- 
balbi

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

[PATCH v2 07/10] usb: dwc2/gadget: move phy control calls out of pullup() method

[Marek Szyprowski]

This patch moves phy enable/disable calls from pullup() method to
udc_start/stop functions. This solves the issue related to limited caller
context for PHY functions, because they cannot be called from non-sleeping
context. This is also a preparation for using soft-disconnect feature of
udc controller in pullup() method.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/usb/dwc2/gadget.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 5eb2473031c4..98adf8d17493 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2919,6 +2919,8 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
 		goto err;
 	}
 
+	s3c_hsotg_phy_enable(hsotg);
+
 	dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
 	return 0;
 
@@ -2955,6 +2957,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
 
 	spin_unlock_irqrestore(&hsotg->lock, flags);
 
+	s3c_hsotg_phy_disable(hsotg);
+
 	regulator_bulk_disable(ARRAY_SIZE(hsotg->supplies), hsotg->supplies);
 
 	clk_disable(hsotg->clk);
@@ -2989,13 +2993,11 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
 
 	spin_lock_irqsave(&hsotg->lock, flags);
 	if (is_on) {
-		s3c_hsotg_phy_enable(hsotg);
 		clk_enable(hsotg->clk);
 		s3c_hsotg_core_init_disconnected(hsotg);
 		s3c_hsotg_core_connect(hsotg);
 	} else {
 		clk_disable(hsotg->clk);
-		s3c_hsotg_phy_disable(hsotg);
 	}
 
 	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
-- 
1.9.2

RE: [PATCH v2 07/10] usb: dwc2/gadget: move phy control calls out of pullup() method

[Paul Zimmerman]

> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
> Sent: Monday, October 20, 2014 3:46 AM
> 
> This patch moves phy enable/disable calls from pullup() method to
> udc_start/stop functions. This solves the issue related to limited caller
> context for PHY functions, because they cannot be called from non-sleeping
> context. This is also a preparation for using soft-disconnect feature of
> udc controller in pullup() method.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/usb/dwc2/gadget.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 5eb2473031c4..98adf8d17493 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -2919,6 +2919,8 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
>  		goto err;
>  	}
> 
> +	s3c_hsotg_phy_enable(hsotg);
> +
>  	dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
>  	return 0;
> 
> @@ -2955,6 +2957,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
> 
>  	spin_unlock_irqrestore(&hsotg->lock, flags);
> 
> +	s3c_hsotg_phy_disable(hsotg);
> +
>  	regulator_bulk_disable(ARRAY_SIZE(hsotg->supplies), hsotg->supplies);
> 
>  	clk_disable(hsotg->clk);
> @@ -2989,13 +2993,11 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
> 
>  	spin_lock_irqsave(&hsotg->lock, flags);
>  	if (is_on) {
> -		s3c_hsotg_phy_enable(hsotg);
>  		clk_enable(hsotg->clk);
>  		s3c_hsotg_core_init_disconnected(hsotg);
>  		s3c_hsotg_core_connect(hsotg);
>  	} else {
>  		clk_disable(hsotg->clk);
> -		s3c_hsotg_phy_disable(hsotg);
>  	}
> 
>  	hsotg->gadget.speed = USB_SPEED_UNKNOWN;

Acked-by: Paul Zimmerman <paulz@synopsys.com>

[PATCH v2 08/10] usb: dwc2/gadget: use soft-disconnect udc feature in pullup() method

[Marek Szyprowski]

This patch moves udc initialization from pullup() method to
s3c_hsotg_udc_start(), so that method ends with hardware fully
initialized and left in soft-disconnected state. After this change, the
pullup() method simply clears soft-disconnect start() when called with
is_on=1. For completeness, a call to s3c_hsotg_core_disconnect() has
been added when pullup() method is called with is_on=0, what puts the
udc hardware back to soft-disconnected state.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/usb/dwc2/gadget.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 98adf8d17493..e8ffc080e6c7 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2883,6 +2883,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
 			   struct usb_gadget_driver *driver)
 {
 	struct s3c_hsotg *hsotg = to_hsotg(gadget);
+	unsigned long flags;
 	int ret;
 
 	if (!hsotg) {
@@ -2921,7 +2922,13 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
 
 	s3c_hsotg_phy_enable(hsotg);
 
+	spin_lock_irqsave(&hsotg->lock, flags);
+	s3c_hsotg_init(hsotg);
+	s3c_hsotg_core_init_disconnected(hsotg);
+	spin_unlock_irqrestore(&hsotg->lock, flags);
+
 	dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
+
 	return 0;
 
 err:
@@ -2994,9 +3001,9 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
 	spin_lock_irqsave(&hsotg->lock, flags);
 	if (is_on) {
 		clk_enable(hsotg->clk);
-		s3c_hsotg_core_init_disconnected(hsotg);
 		s3c_hsotg_core_connect(hsotg);
 	} else {
+		s3c_hsotg_core_disconnect(hsotg);
 		clk_disable(hsotg->clk);
 	}
 
-- 
1.9.2

RE: [PATCH v2 08/10] usb: dwc2/gadget: use soft-disconnect udc feature in pullup() method

[Paul Zimmerman]

> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
> Sent: Monday, October 20, 2014 3:46 AM
> 
> This patch moves udc initialization from pullup() method to
> s3c_hsotg_udc_start(), so that method ends with hardware fully
> initialized and left in soft-disconnected state. After this change, the
> pullup() method simply clears soft-disconnect start() when called with
> is_on=1. For completeness, a call to s3c_hsotg_core_disconnect() has
> been added when pullup() method is called with is_on=0, what puts the
> udc hardware back to soft-disconnected state.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/usb/dwc2/gadget.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 98adf8d17493..e8ffc080e6c7 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -2883,6 +2883,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
>  			   struct usb_gadget_driver *driver)
>  {
>  	struct s3c_hsotg *hsotg = to_hsotg(gadget);
> +	unsigned long flags;
>  	int ret;
> 
>  	if (!hsotg) {
> @@ -2921,7 +2922,13 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
> 
>  	s3c_hsotg_phy_enable(hsotg);
> 
> +	spin_lock_irqsave(&hsotg->lock, flags);
> +	s3c_hsotg_init(hsotg);
> +	s3c_hsotg_core_init_disconnected(hsotg);
> +	spin_unlock_irqrestore(&hsotg->lock, flags);
> +
>  	dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
> +
>  	return 0;
> 
>  err:
> @@ -2994,9 +3001,9 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
>  	spin_lock_irqsave(&hsotg->lock, flags);
>  	if (is_on) {
>  		clk_enable(hsotg->clk);
> -		s3c_hsotg_core_init_disconnected(hsotg);
>  		s3c_hsotg_core_connect(hsotg);
>  	} else {
> +		s3c_hsotg_core_disconnect(hsotg);
>  		clk_disable(hsotg->clk);
>  	}
> 

Acked-by: Paul Zimmerman <paulz@synopsys.com>