[PATCH 0/6] usb: s3c-hsotg: fixes for proper functioning

[PATCH 0/6] usb: s3c-hsotg: fixes for proper functioning

[Robert Baldyga]

These patches add few fixes:
- remove unneeded disconnect from ErlySusp interrupt handling
- remove deprecated driver dependency CONFIG_S3C_DEV_USB_HSOTG
- fix driver unregistration function
- fix "protocol stall" handling, by enqueue new ep0 request when stalled
- add correct interrput handling in dedicated-fifos mode
- fix endpoint halt clearing, when it is not currently halted

Chanho Park (1):
  usb: s3c-hsotg: do not disconnect gadget when receiving ErlySusp intr

Marek Szyprowski (2):
  usb: s3c-hsotg: change dependency to PLAT_SAMSUNG
  usb: s3c-hsotg: fix unregistration function

Robert Baldyga (3):
  USB: gadget: s3c-hsotg: fix "protocol stall" handling
  USB: gadget: s3c-hsotg: fix dedicated fifos handling
  USB: gadget: s3c-hsotg: fix clear feature ENDPOINT_HALT

 drivers/usb/gadget/Kconfig     |    2 +-
 drivers/usb/gadget/s3c-hsotg.c |   81 +++++++++++++++++++++++++++-------------
 2 files changed, 56 insertions(+), 27 deletions(-)

-- 
1.7.9.5

[PATCH 1/6] usb: s3c-hsotg: change dependency to PLAT_SAMSUNG

[Robert Baldyga]

From: Marek Szyprowski <m.szyprowski@samsung.com>

On device tree based Samsung SoC platforms (like Exynos) no platform
devices are defined and CONFIG_S3C_DEV_USB_HSOTG is no longer available,
so change the driver dependency to more generic and approperiate
CONFIG_PLAT_SAMSUNG, as the driver can be used on almost all Samsung
platforms.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/usb/gadget/Kconfig |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 8e93683..08fc9dd 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -282,7 +282,7 @@ config USB_PXA27X
 
 config USB_S3C_HSOTG
 	tristate "S3C HS/OtG USB Device controller"
-	depends on S3C_DEV_USB_HSOTG
+	depends on PLAT_SAMSUNG
 	help
 	  The Samsung S3C64XX USB2.0 high-speed gadget controller
 	  integrated into the S3C64XX series SoC.
-- 
1.7.9.5

[PATCH 2/6] usb: s3c-hsotg: fix unregistration function

[Robert Baldyga]

From: Marek Szyprowski <m.szyprowski@samsung.com>

After driver conversion to udc_start/udc_stop infrastructure (commit
"usb:hsotg:samsung: Use new udc_start and udc_stop callbacks"
f65f0f1098) the gadget unregistration function is almost always called
with 'driver' parameter being NULL, what caused that the unregistration
code has not been executed at all. This is a leftover from the earlier
verison of this function (which used simple start/stop interface), where
driver parameter was obligatory.

This patch removes the NULL check for the 'driver' pointer and removes
all dereferences of it. It also moves disabling voltage regulators out
of the atomic context, because handling regulators (which are usually
i2c devices) might require sleeping.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/usb/gadget/s3c-hsotg.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/s3c-hsotg.c b/drivers/usb/gadget/s3c-hsotg.c
index af22f24..b13a4f9 100644
--- a/drivers/usb/gadget/s3c-hsotg.c
+++ b/drivers/usb/gadget/s3c-hsotg.c
@@ -2961,9 +2961,6 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
 	if (!hsotg)
 		return -ENODEV;
 
-	if (!driver || driver != hsotg->driver || !driver->unbind)
-		return -EINVAL;
-
 	/* all endpoints should be shutdown */
 	for (ep = 0; ep < hsotg->num_of_eps; ep++)
 		s3c_hsotg_ep_disable(&hsotg->eps[ep].ep);
@@ -2971,15 +2968,15 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
 	spin_lock_irqsave(&hsotg->lock, flags);
 
 	s3c_hsotg_phy_disable(hsotg);
-	regulator_bulk_disable(ARRAY_SIZE(hsotg->supplies), hsotg->supplies);
 
-	hsotg->driver = NULL;
+	if (!driver)
+		hsotg->driver = NULL;
+
 	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
 
 	spin_unlock_irqrestore(&hsotg->lock, flags);
 
-	dev_info(hsotg->dev, "unregistered gadget driver '%s'\n",
-		 driver->driver.name);
+	regulator_bulk_disable(ARRAY_SIZE(hsotg->supplies), hsotg->supplies);
 
 	return 0;
 }
-- 
1.7.9.5

[PATCH 3/6] usb: s3c-hsotg: do not disconnect gadget when receiving ErlySusp intr

[Robert Baldyga]

From: Chanho Park <chanho61.park@samsung.com>

DWC2 databook indicates if the core sets "ErlySusp" bit, an idle state has been
detected on the USB for 3 ms. This situation can be occurred when waiting
a request from user daemon. So, we should keep the connection between udc and
gadget even though this interrupt is occurred.

Signed-off-by: Chanho Park <chanho61.park@samsung.com>
Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/usb/gadget/s3c-hsotg.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/usb/gadget/s3c-hsotg.c b/drivers/usb/gadget/s3c-hsotg.c
index b13a4f9..dd5524c 100644
--- a/drivers/usb/gadget/s3c-hsotg.c
+++ b/drivers/usb/gadget/s3c-hsotg.c
@@ -2474,8 +2474,6 @@ irq_retry:
 	if (gintsts & GINTSTS_ErlySusp) {
 		dev_dbg(hsotg->dev, "GINTSTS_ErlySusp\n");
 		writel(GINTSTS_ErlySusp, hsotg->regs + GINTSTS);
-
-		s3c_hsotg_disconnect(hsotg);
 	}
 
 	/*
-- 
1.7.9.5

[PATCH 4/6] USB: gadget: s3c-hsotg: fix "protocol stall" handling

[Robert Baldyga]

After normal handling of SetupDone interrupt, XferCompl interrupt occurs, and
then we enqueue new setup request. But when ep0 is stalled, there is no
XferCompl, so we have to enqueue setup request immediately after stalling ep.
Otherwise incoming control requests won't be processed correctly.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/usb/gadget/s3c-hsotg.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/s3c-hsotg.c b/drivers/usb/gadget/s3c-hsotg.c
index dd5524c..c581cd7 100644
--- a/drivers/usb/gadget/s3c-hsotg.c
+++ b/drivers/usb/gadget/s3c-hsotg.c
@@ -1146,6 +1146,8 @@ static int s3c_hsotg_process_req_feature(struct s3c_hsotg *hsotg,
 	return 1;
 }
 
+static void s3c_hsotg_enqueue_setup(struct s3c_hsotg *hsotg);
+
 /**
  * s3c_hsotg_process_control - process a control request
  * @hsotg: The device state
@@ -1245,11 +1247,12 @@ static void s3c_hsotg_process_control(struct s3c_hsotg *hsotg,
 		 * don't believe we need to anything more to get the EP
 		 * to reply with a STALL packet
 		 */
+
+		 /* complete won't by called, so we enqueue setup request here */
+		 s3c_hsotg_enqueue_setup(hsotg);
 	}
 }
 
-static void s3c_hsotg_enqueue_setup(struct s3c_hsotg *hsotg);
-
 /**
  * s3c_hsotg_complete_setup - completion of a setup transfer
  * @ep: The endpoint the request was on.
-- 
1.7.9.5

Re: [PATCH 4/6] USB: gadget: s3c-hsotg: fix "protocol stall" handling

[Felipe Balbi]

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

On Thu, Sep 12, 2013 at 04:18:50PM +0200, Robert Baldyga wrote:
> After normal handling of SetupDone interrupt, XferCompl interrupt occurs, and
> then we enqueue new setup request. But when ep0 is stalled, there is no
> XferCompl, so we have to enqueue setup request immediately after stalling ep.
> Otherwise incoming control requests won't be processed correctly.
> 
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/usb/gadget/s3c-hsotg.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/s3c-hsotg.c b/drivers/usb/gadget/s3c-hsotg.c
> index dd5524c..c581cd7 100644
> --- a/drivers/usb/gadget/s3c-hsotg.c
> +++ b/drivers/usb/gadget/s3c-hsotg.c
> @@ -1146,6 +1146,8 @@ static int s3c_hsotg_process_req_feature(struct s3c_hsotg *hsotg,
>  	return 1;
>  }
>  
> +static void s3c_hsotg_enqueue_setup(struct s3c_hsotg *hsotg);
> +
>  /**
>   * s3c_hsotg_process_control - process a control request
>   * @hsotg: The device state
> @@ -1245,11 +1247,12 @@ static void s3c_hsotg_process_control(struct s3c_hsotg *hsotg,
>  		 * don't believe we need to anything more to get the EP
>  		 * to reply with a STALL packet
>  		 */
> +
> +		 /* complete won't by called, so we enqueue setup request here */

s/by/be

-- 
balbi

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

[PATCH 5/6] USB: gadget: s3c-hsotg: fix dedicated fifos handling

[Robert Baldyga]

This patch adds few fixes:
- In s3c_hsotg_write_fifo function PTxFEmp/NPTxFEmp interrupts are enabled
  only in shared-fifo mode. In dedicated-fifo mode they should not be used
  (when enabled then cause interrupt storm).
- When s3c_hsotg_trytx is called for ep without enqueued request, interrupts
  for this ep are disabled, to prevent interrupt flooding. Interrupts are
  enabled when new request for this ep is starting.
- In s3c_hsotg_core_init enabled INTknTXFEmpMsk, becouse without this mask
  TxFIFOEmpty interrupt does not occur.
- In OEPInt/IEPInt interrupts handling added bitwise and of DAINT and
  DAINTMSK, because we should handle masked interrupts only.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/usb/gadget/s3c-hsotg.c |   48 ++++++++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/gadget/s3c-hsotg.c b/drivers/usb/gadget/s3c-hsotg.c
index c581cd7..4fb2199 100644
--- a/drivers/usb/gadget/s3c-hsotg.c
+++ b/drivers/usb/gadget/s3c-hsotg.c
@@ -553,9 +553,11 @@ static int s3c_hsotg_write_fifo(struct s3c_hsotg *hsotg,
 	if (to_write > hs_ep->ep.maxpacket) {
 		to_write = hs_ep->ep.maxpacket;
 
-		s3c_hsotg_en_gsint(hsotg,
-				   periodic ? GINTSTS_PTxFEmp :
-				   GINTSTS_NPTxFEmp);
+		/* it's needed only when we do not use dedicated fifos */
+		if (!hsotg->dedicated_fifos)
+			s3c_hsotg_en_gsint(hsotg,
+					   periodic ? GINTSTS_PTxFEmp :
+					   GINTSTS_NPTxFEmp);
 	}
 
 	/* see if we can write data */
@@ -580,9 +582,11 @@ static int s3c_hsotg_write_fifo(struct s3c_hsotg *hsotg,
 		 * is more room left.
 		 */
 
-		s3c_hsotg_en_gsint(hsotg,
-				   periodic ? GINTSTS_PTxFEmp :
-				   GINTSTS_NPTxFEmp);
+		/* it's needed only when we do not use dedicated fifos */
+		if (!hsotg->dedicated_fifos)
+			s3c_hsotg_en_gsint(hsotg,
+					   periodic ? GINTSTS_PTxFEmp :
+					   GINTSTS_NPTxFEmp);
 	}
 
 	dev_dbg(hsotg->dev, "write %d/%d, can_write %d, done %d\n",
@@ -819,6 +823,9 @@ static void s3c_hsotg_start_req(struct s3c_hsotg *hsotg,
 
 	dev_dbg(hsotg->dev, "%s: DxEPCTL=0x%08x\n",
 		__func__, readl(hsotg->regs + epctrl_reg));
+
+	/* enable ep interrupts */
+	s3c_hsotg_ctrl_epint(hsotg, hs_ep->index, hs_ep->dir_in, 1);
 }
 
 /**
@@ -1784,8 +1791,15 @@ static int s3c_hsotg_trytx(struct s3c_hsotg *hsotg,
 {
 	struct s3c_hsotg_req *hs_req = hs_ep->req;
 
-	if (!hs_ep->dir_in || !hs_req)
+	if (!hs_ep->dir_in || !hs_req) {
+		/**
+		 * if request is not enqueued, we disable interrupts for endpoints,
+		 * excepting ep0
+		 */
+		if (hs_ep->index != 0)
+			s3c_hsotg_ctrl_epint(hsotg, hs_ep->index, hs_ep->dir_in, 0);
 		return 0;
+	}
 
 	if (hs_req->req.actual < hs_req->req.length) {
 		dev_dbg(hsotg->dev, "trying to write more for ep%d\n",
@@ -2243,15 +2257,17 @@ static void s3c_hsotg_core_init(struct s3c_hsotg *hsotg)
 		       GAHBCFG_HBstLen_Incr4,
 		       hsotg->regs + GAHBCFG);
 	else
-		writel(GAHBCFG_GlblIntrEn, hsotg->regs + GAHBCFG);
+		writel(GAHBCFG_GlblIntrEn | GAHBCFG_NPTxFEmpLvl,
+		       hsotg->regs + GAHBCFG);
 
 	/*
-	 * Enabling INTknTXFEmpMsk here seems to be a big mistake, we end
-	 * up being flooded with interrupts if the host is polling the
-	 * endpoint to try and read data.
+	 * If INTknTXFEmpMsk is enabled, it's important to disable ep interrupts
+	 * when we have no data to transfer. Otherwise we get being flooded by
+	 * interrupts.
 	 */
 
-	writel(((hsotg->dedicated_fifos) ? DIEPMSK_TxFIFOEmpty : 0) |
+	writel(((hsotg->dedicated_fifos) ? DIEPMSK_TxFIFOEmpty |
+	       DIEPMSK_INTknTXFEmpMsk : 0) |
 	       DIEPMSK_EPDisbldMsk | DIEPMSK_XferComplMsk |
 	       DIEPMSK_TimeOUTMsk | DIEPMSK_AHBErrMsk |
 	       DIEPMSK_INTknEPMisMsk,
@@ -2380,10 +2396,14 @@ irq_retry:
 
 	if (gintsts & (GINTSTS_OEPInt | GINTSTS_IEPInt)) {
 		u32 daint = readl(hsotg->regs + DAINT);
-		u32 daint_out = daint >> DAINT_OutEP_SHIFT;
-		u32 daint_in = daint & ~(daint_out << DAINT_OutEP_SHIFT);
+		u32 daintmsk = readl(hsotg->regs + DAINTMSK);
+		u32 daint_out, daint_in;
 		int ep;
 
+		daint &= daintmsk;
+		daint_out = daint >> DAINT_OutEP_SHIFT;
+		daint_in = daint & ~(daint_out << DAINT_OutEP_SHIFT);
+
 		dev_dbg(hsotg->dev, "%s: daint=%08x\n", __func__, daint);
 
 		for (ep = 0; ep < 15 && daint_out; ep++, daint_out >>= 1) {
-- 
1.7.9.5

Re: [PATCH 5/6] USB: gadget: s3c-hsotg: fix dedicated fifos handling

[Felipe Balbi]

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

On Thu, Sep 12, 2013 at 04:18:51PM +0200, Robert Baldyga wrote:
> This patch adds few fixes:
> - In s3c_hsotg_write_fifo function PTxFEmp/NPTxFEmp interrupts are enabled
>   only in shared-fifo mode. In dedicated-fifo mode they should not be used
>   (when enabled then cause interrupt storm).
> - When s3c_hsotg_trytx is called for ep without enqueued request, interrupts
>   for this ep are disabled, to prevent interrupt flooding. Interrupts are
>   enabled when new request for this ep is starting.
> - In s3c_hsotg_core_init enabled INTknTXFEmpMsk, becouse without this mask
>   TxFIFOEmpty interrupt does not occur.
> - In OEPInt/IEPInt interrupts handling added bitwise and of DAINT and
>   DAINTMSK, because we should handle masked interrupts only.

each fix in its own patch please.

-- 
balbi

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

[PATCH 6/6] USB: gadget: s3c-hsotg: fix clear feature ENDPOINT_HALT

[Robert Baldyga]

This patch adds two fixes:
- Property halted of s3c_hsotg_ep structure is actually changed when halt is
  set/cleared.
- All requests for endpoint are completed when it was halted, and the halt was
  cleared by CLEAR_FEATURE, but not when new state is same as previous.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/usb/gadget/s3c-hsotg.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/s3c-hsotg.c b/drivers/usb/gadget/s3c-hsotg.c
index 4fb2199..db4da58 100644
--- a/drivers/usb/gadget/s3c-hsotg.c
+++ b/drivers/usb/gadget/s3c-hsotg.c
@@ -1097,6 +1097,7 @@ static int s3c_hsotg_process_req_feature(struct s3c_hsotg *hsotg,
 	bool set = (ctrl->bRequest == USB_REQ_SET_FEATURE);
 	struct s3c_hsotg_ep *ep;
 	int ret;
+	bool halted;
 
 	dev_dbg(hsotg->dev, "%s: %s_FEATURE\n",
 		__func__, set ? "SET" : "CLEAR");
@@ -1111,6 +1112,8 @@ static int s3c_hsotg_process_req_feature(struct s3c_hsotg *hsotg,
 
 		switch (le16_to_cpu(ctrl->wValue)) {
 		case USB_ENDPOINT_HALT:
+			halted = ep->halted;
+
 			s3c_hsotg_ep_sethalt(&ep->ep, set);
 
 			ret = s3c_hsotg_send_reply(hsotg, ep0, NULL, 0);
@@ -1120,7 +1123,12 @@ static int s3c_hsotg_process_req_feature(struct s3c_hsotg *hsotg,
 				return ret;
 			}
 
-			if (!set) {
+			/*
+			 * we have to complete all requests for ep if it was halted,
+			 * and the halt was cleared by CLEAR_FEATURE
+			 */
+
+			if (!set || halted) {
 				/*
 				 * If we have request in progress,
 				 * then complete it
@@ -2603,6 +2611,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
 
 	/* default, set to non-periodic */
 	hs_ep->periodic = 0;
+	hs_ep->halted = 0;
 
 	switch (desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) {
 	case USB_ENDPOINT_XFER_ISOC:
@@ -2798,6 +2807,8 @@ static int s3c_hsotg_ep_sethalt(struct usb_ep *ep, int value)
 
 	writel(epctl, hs->regs + epreg);
 
+	hs_ep->halted = value;
+
 	return 0;
 }
 
-- 
1.7.9.5

Re: [PATCH 6/6] USB: gadget: s3c-hsotg: fix clear feature ENDPOINT_HALT

[Felipe Balbi]

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

On Thu, Sep 12, 2013 at 04:18:52PM +0200, Robert Baldyga wrote:
> This patch adds two fixes:
> - Property halted of s3c_hsotg_ep structure is actually changed when halt is
>   set/cleared.
> - All requests for endpoint are completed when it was halted, and the halt was
>   cleared by CLEAR_FEATURE, but not when new state is same as previous.

each fix in its own patch

-- 
balbi

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