[PATCH 0/9] more dwc2/gadget fixes

[PATCH 0/9] 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.

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

Marek Szyprowski (9):
  usb: dwc2/gadget: report disconnect event from 'end session' irq
  usb: dwc2/gadget: fix enumeration issues
  usb: dwc2/gadget: fix support for soft_connect udc framework feature
  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: use soft disconnect mode for implementing pullup
    control
  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 | 93 +++++++++++++++++++++++++++++++++--------------
 2 files changed, 69 insertions(+), 28 deletions(-)

-- 
1.9.2

[PATCH 2/9] 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@samsung.com>
---
 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

Re: [PATCH 2/9] usb: dwc2/gadget: fix enumeration issues

[Felipe Balbi]

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

On Thu, Oct 16, 2014 at 02:57:58PM +0200, Marek Szyprowski wrote:
> 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@samsung.com>
> ---
>  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__);

considering this is inside an IRQ handler, I'd rather use dev_vdbg() but
no strong feelings:

Reviewed-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

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

[PATCH 3/9] usb: dwc2/gadget: fix support for soft_connect udc framework feature

[Marek Szyprowski]

Enabling and disabling usb gadget by writing to
/sys/class/udc/*hsotg/soft_connect results in calling udc_start/udc_stop
functions with the same usb gadget driver, so the driver should not WARN
about such case.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 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 8870e38c1d82..37fda4c03397 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2892,7 +2892,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
 		return -EINVAL;
 	}
 
-	WARN_ON(hsotg->driver);
+	WARN_ON(hsotg->driver && hsotg->driver != driver);
 
 	driver->driver.bus = NULL;
 	hsotg->driver = driver;
-- 
1.9.2

Re: [PATCH 3/9] usb: dwc2/gadget: fix support for soft_connect udc framework feature

[Felipe Balbi]

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

Hi,

On Thu, Oct 16, 2014 at 02:57:59PM +0200, Marek Szyprowski wrote:
> Enabling and disabling usb gadget by writing to
> /sys/class/udc/*hsotg/soft_connect results in calling udc_start/udc_stop
> functions with the same usb gadget driver, so the driver should not WARN
> about such case.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  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 8870e38c1d82..37fda4c03397 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -2892,7 +2892,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
>  		return -EINVAL;
>  	}
>  
> -	WARN_ON(hsotg->driver);
> +	WARN_ON(hsotg->driver && hsotg->driver != driver);

the bug is in your ->udc_stop(). You should clear hsotg->driver to NULL
there.

-- 
balbi

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

Re: [PATCH 3/9] usb: dwc2/gadget: fix support for soft_connect udc framework feature

[Marek Szyprowski]

Hello,

On 2014-10-16 15:36, Felipe Balbi wrote:
> On Thu, Oct 16, 2014 at 02:57:59PM +0200, Marek Szyprowski wrote:
>> Enabling and disabling usb gadget by writing to
>> /sys/class/udc/*hsotg/soft_connect results in calling udc_start/udc_stop
>> functions with the same usb gadget driver, so the driver should not WARN
>> about such case.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   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 8870e38c1d82..37fda4c03397 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -2892,7 +2892,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
>>   		return -EINVAL;
>>   	}
>>   
>> -	WARN_ON(hsotg->driver);
>> +	WARN_ON(hsotg->driver && hsotg->driver != driver);
> the bug is in your ->udc_stop(). You should clear hsotg->driver to NULL
> there.

Ok, I will change udc_stop() to always zero hsotg->driver, like other udc
drivers. I was a bit confused by the fact that udc core passes driver to
udc_stop(), when called from soft_connect and NULL on gadget removal.

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

Re: [PATCH 3/9] usb: dwc2/gadget: fix support for soft_connect udc framework feature

[Felipe Balbi]

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

On Fri, Oct 17, 2014 at 12:43:48PM +0200, Marek Szyprowski wrote:
> Hello,
> 
> On 2014-10-16 15:36, Felipe Balbi wrote:
> >On Thu, Oct 16, 2014 at 02:57:59PM +0200, Marek Szyprowski wrote:
> >>Enabling and disabling usb gadget by writing to
> >>/sys/class/udc/*hsotg/soft_connect results in calling udc_start/udc_stop
> >>functions with the same usb gadget driver, so the driver should not WARN
> >>about such case.
> >>
> >>Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >>---
> >>  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 8870e38c1d82..37fda4c03397 100644
> >>--- a/drivers/usb/dwc2/gadget.c
> >>+++ b/drivers/usb/dwc2/gadget.c
> >>@@ -2892,7 +2892,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
> >>  		return -EINVAL;
> >>  	}
> >>-	WARN_ON(hsotg->driver);
> >>+	WARN_ON(hsotg->driver && hsotg->driver != driver);
> >the bug is in your ->udc_stop(). You should clear hsotg->driver to NULL
> >there.
> 
> Ok, I will change udc_stop() to always zero hsotg->driver, like other udc
> drivers. I was a bit confused by the fact that udc core passes driver to
> udc_stop(), when called from soft_connect and NULL on gadget removal.

That can probably be cleaned up, I'll go have a look on all UDCs and
make sure I won't break anything.

-- 
balbi

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

Re: [PATCH 3/9] usb: dwc2/gadget: fix support for soft_connect udc framework feature

[Felipe Balbi]

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

Hi,

On Fri, Oct 17, 2014 at 10:44:35AM -0500, Felipe Balbi wrote:
> On Fri, Oct 17, 2014 at 12:43:48PM +0200, Marek Szyprowski wrote:
> > Hello,
> > 
> > On 2014-10-16 15:36, Felipe Balbi wrote:
> > >On Thu, Oct 16, 2014 at 02:57:59PM +0200, Marek Szyprowski wrote:
> > >>Enabling and disabling usb gadget by writing to
> > >>/sys/class/udc/*hsotg/soft_connect results in calling udc_start/udc_stop
> > >>functions with the same usb gadget driver, so the driver should not WARN
> > >>about such case.
> > >>
> > >>Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@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 8870e38c1d82..37fda4c03397 100644
> > >>--- a/drivers/usb/dwc2/gadget.c
> > >>+++ b/drivers/usb/dwc2/gadget.c
> > >>@@ -2892,7 +2892,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
> > >>  		return -EINVAL;
> > >>  	}
> > >>-	WARN_ON(hsotg->driver);
> > >>+	WARN_ON(hsotg->driver && hsotg->driver != driver);
> > >the bug is in your ->udc_stop(). You should clear hsotg->driver to NULL
> > >there.
> > 
> > Ok, I will change udc_stop() to always zero hsotg->driver, like other udc
> > drivers. I was a bit confused by the fact that udc core passes driver to
> > udc_stop(), when called from soft_connect and NULL on gadget removal.
> 
> That can probably be cleaned up, I'll go have a look on all UDCs and
> make sure I won't break anything.

looks like chipidea is the only one still using that argument, if you
make your patch look like below:

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 7b5856f..ac14328 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2934,8 +2934,7 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
 
 	spin_lock_irqsave(&hsotg->lock, flags);
 
-	if (!driver)
-		hsotg->driver = NULL;
+	hsotg->driver = NULL;
 
 	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
 

I'll remove the argument.

-- 
balbi

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

Re: [PATCH 3/9] usb: dwc2/gadget: fix support for soft_connect udc framework feature

[Felipe Balbi]

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

On Fri, Oct 17, 2014 at 10:46:30AM -0500, Felipe Balbi wrote:
> Hi,
> 
> On Fri, Oct 17, 2014 at 10:44:35AM -0500, Felipe Balbi wrote:
> > On Fri, Oct 17, 2014 at 12:43:48PM +0200, Marek Szyprowski wrote:
> > > Hello,
> > > 
> > > On 2014-10-16 15:36, Felipe Balbi wrote:
> > > >On Thu, Oct 16, 2014 at 02:57:59PM +0200, Marek Szyprowski wrote:
> > > >>Enabling and disabling usb gadget by writing to
> > > >>/sys/class/udc/*hsotg/soft_connect results in calling udc_start/udc_stop
> > > >>functions with the same usb gadget driver, so the driver should not WARN
> > > >>about such case.
> > > >>
> > > >>Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@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 8870e38c1d82..37fda4c03397 100644
> > > >>--- a/drivers/usb/dwc2/gadget.c
> > > >>+++ b/drivers/usb/dwc2/gadget.c
> > > >>@@ -2892,7 +2892,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
> > > >>  		return -EINVAL;
> > > >>  	}
> > > >>-	WARN_ON(hsotg->driver);
> > > >>+	WARN_ON(hsotg->driver && hsotg->driver != driver);
> > > >the bug is in your ->udc_stop(). You should clear hsotg->driver to NULL
> > > >there.
> > > 
> > > Ok, I will change udc_stop() to always zero hsotg->driver, like other udc
> > > drivers. I was a bit confused by the fact that udc core passes driver to
> > > udc_stop(), when called from soft_connect and NULL on gadget removal.
> > 
> > That can probably be cleaned up, I'll go have a look on all UDCs and
> > make sure I won't break anything.
> 
> looks like chipidea is the only one still using that argument, if you

meant dwc2

-- 
balbi

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

Re: [PATCH 3/9] usb: dwc2/gadget: fix support for soft_connect udc framework feature

[Felipe Balbi]

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

On Fri, Oct 17, 2014 at 10:49:25AM -0500, Felipe Balbi wrote:
> On Fri, Oct 17, 2014 at 10:46:30AM -0500, Felipe Balbi wrote:
> > Hi,
> > 
> > On Fri, Oct 17, 2014 at 10:44:35AM -0500, Felipe Balbi wrote:
> > > On Fri, Oct 17, 2014 at 12:43:48PM +0200, Marek Szyprowski wrote:
> > > > Hello,
> > > > 
> > > > On 2014-10-16 15:36, Felipe Balbi wrote:
> > > > >On Thu, Oct 16, 2014 at 02:57:59PM +0200, Marek Szyprowski wrote:
> > > > >>Enabling and disabling usb gadget by writing to
> > > > >>/sys/class/udc/*hsotg/soft_connect results in calling udc_start/udc_stop
> > > > >>functions with the same usb gadget driver, so the driver should not WARN
> > > > >>about such case.
> > > > >>
> > > > >>Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > > > >>---
> > > > >>  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 8870e38c1d82..37fda4c03397 100644
> > > > >>--- a/drivers/usb/dwc2/gadget.c
> > > > >>+++ b/drivers/usb/dwc2/gadget.c
> > > > >>@@ -2892,7 +2892,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
> > > > >>  		return -EINVAL;
> > > > >>  	}
> > > > >>-	WARN_ON(hsotg->driver);
> > > > >>+	WARN_ON(hsotg->driver && hsotg->driver != driver);
> > > > >the bug is in your ->udc_stop(). You should clear hsotg->driver to NULL
> > > > >there.
> > > > 
> > > > Ok, I will change udc_stop() to always zero hsotg->driver, like other udc
> > > > drivers. I was a bit confused by the fact that udc core passes driver to
> > > > udc_stop(), when called from soft_connect and NULL on gadget removal.
> > > 
> > > That can probably be cleaned up, I'll go have a look on all UDCs and
> > > make sure I won't break anything.
> > 
> > looks like chipidea is the only one still using that argument, if you
> 
> meant dwc2

Hmm, there are still a few other gadgets relying on that argument. It'll
take a little more effort to remove it.

-- 
balbi

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

Re: [PATCH 3/9] usb: dwc2/gadget: fix support for soft_connect udc framework feature

[Felipe Balbi]

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

On Fri, Oct 17, 2014 at 11:02:34AM -0500, Felipe Balbi wrote:
> On Fri, Oct 17, 2014 at 10:49:25AM -0500, Felipe Balbi wrote:
> > On Fri, Oct 17, 2014 at 10:46:30AM -0500, Felipe Balbi wrote:
> > > Hi,
> > > 
> > > On Fri, Oct 17, 2014 at 10:44:35AM -0500, Felipe Balbi wrote:
> > > > On Fri, Oct 17, 2014 at 12:43:48PM +0200, Marek Szyprowski wrote:
> > > > > Hello,
> > > > > 
> > > > > On 2014-10-16 15:36, Felipe Balbi wrote:
> > > > > >On Thu, Oct 16, 2014 at 02:57:59PM +0200, Marek Szyprowski wrote:
> > > > > >>Enabling and disabling usb gadget by writing to
> > > > > >>/sys/class/udc/*hsotg/soft_connect results in calling udc_start/udc_stop
> > > > > >>functions with the same usb gadget driver, so the driver should not WARN
> > > > > >>about such case.
> > > > > >>
> > > > > >>Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@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 8870e38c1d82..37fda4c03397 100644
> > > > > >>--- a/drivers/usb/dwc2/gadget.c
> > > > > >>+++ b/drivers/usb/dwc2/gadget.c
> > > > > >>@@ -2892,7 +2892,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
> > > > > >>  		return -EINVAL;
> > > > > >>  	}
> > > > > >>-	WARN_ON(hsotg->driver);
> > > > > >>+	WARN_ON(hsotg->driver && hsotg->driver != driver);
> > > > > >the bug is in your ->udc_stop(). You should clear hsotg->driver to NULL
> > > > > >there.
> > > > > 
> > > > > Ok, I will change udc_stop() to always zero hsotg->driver, like other udc
> > > > > drivers. I was a bit confused by the fact that udc core passes driver to
> > > > > udc_stop(), when called from soft_connect and NULL on gadget removal.
> > > > 
> > > > That can probably be cleaned up, I'll go have a look on all UDCs and
> > > > make sure I won't break anything.
> > > 
> > > looks like chipidea is the only one still using that argument, if you
> > 
> > meant dwc2
> 
> Hmm, there are still a few other gadgets relying on that argument. It'll
> take a little more effort to remove it.

Alright, just pending your bug fix. I have fixed all other drivers. Will
send out shortly.

-- 
balbi

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

[PATCH 5/9] 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 178a6ea5eef8..1ba0682fb252 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;
 
@@ -3669,7 +3669,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

[PATCH 7/9] usb: dwc2/gadget: use soft disconnect mode for implementing pullup control

[Marek Szyprowski]

This patch moves PHY enable and disable calls from pullup method to
udc_start/stop functions and adds calls to recently introduces soft
disconnect mode in pullup method. This improves dwc2 gadget driver
compatibility with gadget API requirements (now pullup method really
forces soft disconnect mode instead of shutting down the whole hardware)
and as a side effect also solves the issue related to limited caller
context for PHY related functions (they cannot be called from
non-sleeping context).

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

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index d039334967d7..cdf417a7ae63 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) {
@@ -2919,7 +2920,15 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
 		goto err;
 	}
 
+	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:
@@ -2957,6 +2966,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);
@@ -2990,14 +3001,13 @@ 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) {
-		s3c_hsotg_phy_enable(hsotg);
 		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);
-		s3c_hsotg_phy_disable(hsotg);
 	}
 
 	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
-- 
1.9.2

Re: [PATCH 7/9] usb: dwc2/gadget: use soft disconnect mode for implementing pullup control

[Felipe Balbi]

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

Hi,

your subject line is wrong. ->pullup() is already implemented, you're
moving unnecessary code from ->pullup() to other places.

On Thu, Oct 16, 2014 at 02:58:03PM +0200, Marek Szyprowski wrote:
> This patch moves PHY enable and disable calls from pullup method to
> udc_start/stop functions and adds calls to recently introduces soft
> disconnect mode in pullup method. This improves dwc2 gadget driver
> compatibility with gadget API requirements (now pullup method really
> forces soft disconnect mode instead of shutting down the whole hardware)
> and as a side effect also solves the issue related to limited caller
> context for PHY related functions (they cannot be called from
> non-sleeping context).

you're doing two things in one patch. See below

> Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/usb/dwc2/gadget.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index d039334967d7..cdf417a7ae63 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) {
> @@ -2919,7 +2920,15 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
>  		goto err;
>  	}
>  
> +	s3c_hsotg_phy_enable(hsotg);

you moved phy_enable to udc_start/udc_stop (one patch)

> +
> +	spin_lock_irqsave(&hsotg->lock, flags);
> +	s3c_hsotg_init(hsotg);
> +	s3c_hsotg_core_init_disconnected(hsotg);

you moved core init here (another patch).

> +	spin_unlock_irqrestore(&hsotg->lock, flags);
> +
>  	dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
> +
>  	return 0;
>  
>  err:
> @@ -2957,6 +2966,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);
> @@ -2990,14 +3001,13 @@ 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) {
> -		s3c_hsotg_phy_enable(hsotg);
>  		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);
> -		s3c_hsotg_phy_disable(hsotg);
>  	}
>  
>  	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
> -- 
> 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

-- 
balbi

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

[PATCH 1/9] 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 to correctly notify gadget subsystem about unplugged usb
cable.

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) {
-- 
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 1/9] usb: dwc2/gadget: report disconnect event from 'end session' irq

[Felipe Balbi]

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

On Thu, Oct 16, 2014 at 02:57:57PM +0200, Marek Szyprowski wrote:
> This patch adds a call to s3c_hsotg_disconnect() from 'end session'
> interrupt to correctly notify gadget subsystem about unplugged usb
> cable.
> 
> 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) {

looks like this should be done for GINTSTS_DISCONNINT.

-- 
balbi

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

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

[Marek Szyprowski]

Hello,

On 2014-10-16 15:33, Felipe Balbi wrote:
> On Thu, Oct 16, 2014 at 02:57:57PM +0200, Marek Szyprowski wrote:
>> This patch adds a call to s3c_hsotg_disconnect() from 'end session'
>> interrupt to correctly notify gadget subsystem about unplugged usb
>> cable.
>>
>> 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) {
> looks like this should be done for GINTSTS_DISCONNINT.

I also would like to report it from that interrupt, but according to 
DWC2 databook
(version 2.81a) and my observations on Samsung Exynos SoCs, DISCONNINT 
interrupt
is asserted only in host mode, so in device mode we need to use 
something else.

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

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

[Felipe Balbi]

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

On Fri, Oct 17, 2014 at 12:35:39PM +0200, Marek Szyprowski wrote:
> Hello,
> 
> On 2014-10-16 15:33, Felipe Balbi wrote:
> >On Thu, Oct 16, 2014 at 02:57:57PM +0200, Marek Szyprowski wrote:
> >>This patch adds a call to s3c_hsotg_disconnect() from 'end session'
> >>interrupt to correctly notify gadget subsystem about unplugged usb
> >>cable.
> >>
> >>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) {
> >looks like this should be done for GINTSTS_DISCONNINT.
> 
> I also would like to report it from that interrupt, but according to
> DWC2 databook (version 2.81a) and my observations on Samsung Exynos
> SoCs, DISCONNINT interrupt is asserted only in host mode, so in device
> mode we need to use something else.

If that's the case, then I withdraw my comments. I don't have access to
HW or docs about this IP, it just looked suspicious :-)

-- 
balbi

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

[PATCH 4/9] 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 37fda4c03397..178a6ea5eef8 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3573,6 +3573,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);
@@ -3581,8 +3582,6 @@ static int s3c_hsotg_probe(struct platform_device *pdev)
 		goto err_ep_mem;
 	}
 
-	s3c_hsotg_phy_disable(hsotg);

[PATCH 6/9] 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 actual moment of coupling the hardware to
the usb bus can be later controlled by the driver in the more accurate
way. For this purpose, separate functions for enabling and disabling
soft disconnect mode have been added.

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 1ba0682fb252..d039334967d7 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);
 			}
 		}
 	}
@@ -2983,7 +2993,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);
@@ -3670,7 +3681,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_disconnect(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 6/9] usb: dwc2/gadget: decouple setting soft disconnect from s3c_hsotg_core_init

[Felipe Balbi]

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

On Thu, Oct 16, 2014 at 02:58:02PM +0200, Marek Szyprowski wrote:
> This patch changes s3c_hsotg_core_init function to leave hardware in
> soft disconnect mode, so the actual moment of coupling the hardware to
> the usb bus can be later controlled by the driver in the more accurate

what is this "more accurate way" you talk about ? Why is it more
accurate ? Perhaps you have failed some USB Certification test ? Which
test id was that ? Why did it fail ? and why does this patch solve the
issue ?

> way. For this purpose, separate functions for enabling and disabling
> soft disconnect mode have been added.
> 
> 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 1ba0682fb252..d039334967d7 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);
>  			}
>  		}
>  	}
> @@ -2983,7 +2993,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);
> @@ -3670,7 +3681,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_disconnect(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

-- 
balbi

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

Re: [PATCH 6/9] usb: dwc2/gadget: decouple setting soft disconnect from s3c_hsotg_core_init

[Marek Szyprowski]

Hello,

On 2014-10-16 15:38, Felipe Balbi wrote:
> On Thu, Oct 16, 2014 at 02:58:02PM +0200, Marek Szyprowski wrote:
>> This patch changes s3c_hsotg_core_init function to leave hardware in
>> soft disconnect mode, so the actual moment of coupling the hardware to
>> the usb bus can be later controlled by the driver in the more accurate
> what is this "more accurate way" you talk about ? Why is it more
> accurate ? Perhaps you have failed some USB Certification test ? Which
> test id was that ? Why did it fail ? and why does this patch solve the
> issue ?

This patch is just a preparation for the next patches, which introduces 
usage
of soft-disconnect feature in pullup() method.

>> way. For this purpose, separate functions for enabling and disabling
>> soft disconnect mode have been added.
>>
>> 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 1ba0682fb252..d039334967d7 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);
>>   			}
>>   		}
>>   	}
>> @@ -2983,7 +2993,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);
>> @@ -3670,7 +3681,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_disconnect(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@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

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

[PATCH 8/9] 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.

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

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index cdf417a7ae63..052b1a857291 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3656,11 +3656,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++)
@@ -3689,9 +3691,10 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
 				      hsotg->supplies);
 	}
 
-	spin_lock_irqsave(&hsotg->lock, flags);
 	s3c_hsotg_phy_enable(hsotg);
-	s3c_hsotg_core_init_disconnect(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 8/9] usb: dwc2/gadget: fix calls to phy control functions in suspend/resume code

[Felipe Balbi]

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

Hi,

On Thu, Oct 16, 2014 at 02:58:04PM +0200, Marek Szyprowski wrote:
> 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.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/usb/dwc2/gadget.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index cdf417a7ae63..052b1a857291 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -3656,11 +3656,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);

this is aching to have a locked version as well as an unlocked version.
Look at what you do here. There's a minor race when you release that
spinlock. By the time ->suspend() is called, IRQs are not yet disabled.

-- 
balbi

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

Re: [PATCH 8/9] usb: dwc2/gadget: fix calls to phy control functions in suspend/resume code

[Marek Szyprowski]

Hello,

On 2014-10-16 15:42, Felipe Balbi wrote:
> On Thu, Oct 16, 2014 at 02:58:04PM +0200, Marek Szyprowski wrote:
>> 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.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   drivers/usb/dwc2/gadget.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index cdf417a7ae63..052b1a857291 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -3656,11 +3656,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);
> this is aching to have a locked version as well as an unlocked version.
> Look at what you do here. There's a minor race when you release that
> spinlock. By the time ->suspend() is called, IRQs are not yet disabled.

s3c_hsotg_core_disconnect() disconnects the udc hardware from the usb bus, so even
if the irq comes before s3c_hsotg_phy_disable(), nothing wrong happens, because the
driver state is already set to disconnected.

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

[PATCH 9/9] 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 | 42 ++++++++++++++++++++++++++----------------
 2 files changed, 29 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 052b1a857291..83fe123ede05 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);
@@ -2963,6 +2965,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);
 
@@ -3004,9 +3008,11 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
 
 	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);
 	}
 
@@ -3655,16 +3661,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);
 
@@ -3682,21 +3690,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