[PATCH v2 0/3] usb: dwc3: gadget: Improve END_TRANSFER handling

[PATCH v2 0/3] usb: dwc3: gadget: Improve END_TRANSFER handling

[Thinh Nguyen]

This patch series improves the handling of the END_TRANSFER command:

1) Don't issue multiple END_TRANSFER commands while the previous hasn't
   completed
2) Don't issue START_TRANSFER command while END_TRANSFER hasn't completed
3) Remove arbitrary 100us delay

Changes in v2:
 - Remove unused variable


Thinh Nguyen (3):
  usb: dwc3: gadget: Check END_TRANSFER completion
  usb: dwc3: gadget: Delay starting transfer
  usb: dwc3: gadget: Remove END_TRANSFER delay

 drivers/usb/dwc3/core.h   |  2 ++
 drivers/usb/dwc3/ep0.c    |  4 +++-
 drivers/usb/dwc3/gadget.c | 35 +++++++++++++++++++++--------------
 3 files changed, 26 insertions(+), 15 deletions(-)

-- 
2.11.0

[PATCH v2 1/3] usb: dwc3: gadget: Check END_TRANSFER completion

[Thinh Nguyen]

While the END_TRANSFER command is sent but not completed, any request
dequeue during this time will cause the driver to issue the END_TRANSFER
command. The driver needs to submit the command only once to stop the
controller from processing further. The controller may take more time to
process the same command multiple times unnecessarily. Let's add a flag
DWC3_EP_END_TRANSFER_PENDING to check for this condition.

Fixes: 3aec99154db3 ("usb: dwc3: gadget: remove DWC3_EP_END_TRANSFER_PENDING")
Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
Changes in v2:
 - None

 drivers/usb/dwc3/core.h   | 1 +
 drivers/usb/dwc3/ep0.c    | 4 +++-
 drivers/usb/dwc3/gadget.c | 6 +++++-
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 1c8b349379af..da0af11fbc1a 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -688,6 +688,7 @@ struct dwc3_ep {
 #define DWC3_EP_STALL		BIT(1)
 #define DWC3_EP_WEDGE		BIT(2)
 #define DWC3_EP_TRANSFER_STARTED BIT(3)
+#define DWC3_EP_END_TRANSFER_PENDING BIT(4)
 #define DWC3_EP_PENDING_REQUEST	BIT(5)
 
 	/* This last one is specific to EP0 */
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index fd1b100d2927..6dee4dabc0a4 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -1136,8 +1136,10 @@ void dwc3_ep0_interrupt(struct dwc3 *dwc,
 	case DWC3_DEPEVT_EPCMDCMPLT:
 		cmd = DEPEVT_PARAMETER_CMD(event->parameters);
 
-		if (cmd == DWC3_DEPCMD_ENDTRANSFER)
+		if (cmd == DWC3_DEPCMD_ENDTRANSFER) {
+			dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
 			dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
+		}
 		break;
 	}
 }
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index b3f8514d1f27..218022c261bc 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2621,6 +2621,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
 		cmd = DEPEVT_PARAMETER_CMD(event->parameters);
 
 		if (cmd == DWC3_DEPCMD_ENDTRANSFER) {
+			dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
 			dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
 			dwc3_gadget_ep_cleanup_cancelled_requests(dep);
 		}
@@ -2679,7 +2680,8 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 	u32 cmd;
 	int ret;
 
-	if (!(dep->flags & DWC3_EP_TRANSFER_STARTED))
+	if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
+	    (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
 		return;
 
 	/*
@@ -2724,6 +2726,8 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 
 	if (!interrupt)
 		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
+	else
+		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
 
 	if (dwc3_is_usb31(dwc) || dwc->revision < DWC3_REVISION_310A)
 		udelay(100);
-- 
2.11.0

[PATCH v2 2/3] usb: dwc3: gadget: Delay starting transfer

[Thinh Nguyen]

If the END_TRANSFER command hasn't completed yet, then don't send the
START_TRANSFER command. The controller may not be able to start if
that's the case. Some controller revisions depend on this. See
commit 76a638f8ac0d ("usb: dwc3: gadget: wait for End Transfer to
complete"). Let's only send START_TRANSFER command after the
END_TRANSFER command had completed.

Fixes: 3aec99154db3 ("usb: dwc3: gadget: remove DWC3_EP_END_TRANSFER_PENDING")
Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
Changes in v2:
 - None

 drivers/usb/dwc3/core.h   |  1 +
 drivers/usb/dwc3/gadget.c | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index da0af11fbc1a..77c4a9abe365 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -690,6 +690,7 @@ struct dwc3_ep {
 #define DWC3_EP_TRANSFER_STARTED BIT(3)
 #define DWC3_EP_END_TRANSFER_PENDING BIT(4)
 #define DWC3_EP_PENDING_REQUEST	BIT(5)
+#define DWC3_EP_DELAY_START	BIT(6)
 
 	/* This last one is specific to EP0 */
 #define DWC3_EP0_DIR_IN		BIT(31)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 218022c261bc..61b7bef98cf9 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1450,6 +1450,12 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
 	list_add_tail(&req->list, &dep->pending_list);
 	req->status = DWC3_REQUEST_STATUS_QUEUED;
 
+	/* Start the transfer only after the END_TRANSFER is completed */
+	if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) {
+		dep->flags |= DWC3_EP_DELAY_START;
+		return 0;
+	}
+
 	/*
 	 * NOTICE: Isochronous endpoints should NEVER be prestarted. We must
 	 * wait for a XferNotReady event so we will know what's the current
@@ -2624,6 +2630,11 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
 			dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
 			dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
 			dwc3_gadget_ep_cleanup_cancelled_requests(dep);
+			if ((dep->flags & DWC3_EP_DELAY_START) &&
+			    !usb_endpoint_xfer_isoc(dep->endpoint.desc))
+				__dwc3_gadget_kick_transfer(dep);
+
+			dep->flags &= ~DWC3_EP_DELAY_START;
 		}
 		break;
 	case DWC3_DEPEVT_STREAMEVT:
-- 
2.11.0

[PATCH v2 3/3] usb: dwc3: gadget: Remove END_TRANSFER delay

[Thinh Nguyen]

We had a 100us delay to synchronize the END_TRANSFER command completion
before giving back requests to the function drivers. Now, the controller
driver can handle cancelled TRBs with the requests' cancelled_list and
it can also wait until the END_TRANSFER completion before starting new
transfers. Synchronization can simply base on the controller's command
completion interrupt. The 100us delay is no longer needed. Remove this
arbitrary delay.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
Changes in v2:
 - Remove unused variable

 drivers/usb/dwc3/gadget.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 61b7bef98cf9..05dbeae1308a 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2686,7 +2686,6 @@ static void dwc3_reset_gadget(struct dwc3 *dwc)
 static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 	bool interrupt)
 {
-	struct dwc3 *dwc = dep->dwc;
 	struct dwc3_gadget_ep_cmd_params params;
 	u32 cmd;
 	int ret;
@@ -2702,16 +2701,13 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 	 * much trouble synchronizing between us and gadget driver.
 	 *
 	 * We have discussed this with the IP Provider and it was
-	 * suggested to giveback all requests here, but give HW some
-	 * extra time to synchronize with the interconnect. We're using
-	 * an arbitrary 100us delay for that.
+	 * suggested to giveback all requests here.
 	 *
 	 * Note also that a similar handling was tested by Synopsys
 	 * (thanks a lot Paul) and nothing bad has come out of it.
-	 * In short, what we're doing is:
-	 *
-	 * - Issue EndTransfer WITH CMDIOC bit set
-	 * - Wait 100us
+	 * In short, what we're doing is issuing EndTransfer with
+	 * CMDIOC bit set and delay kicking transfer until the
+	 * EndTransfer command had completed.
 	 *
 	 * As of IP version 3.10a of the DWC_usb3 IP, the controller
 	 * supports a mode to work around the above limitation. The
@@ -2720,8 +2716,7 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 	 * by writing GUCTL2[14]. This polling is already done in the
 	 * dwc3_send_gadget_ep_cmd() function so if the mode is
 	 * enabled, the EndTransfer command will have completed upon
-	 * returning from this function and we don't need to delay for
-	 * 100us.
+	 * returning from this function.
 	 *
 	 * This mode is NOT available on the DWC_usb31 IP.
 	 */
@@ -2739,9 +2734,6 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
 	else
 		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
-
-	if (dwc3_is_usb31(dwc) || dwc->revision < DWC3_REVISION_310A)
-		udelay(100);
 }
 
 static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)
-- 
2.11.0

Re: [PATCH v2 0/3] usb: dwc3: gadget: Improve END_TRANSFER handling

[Felipe Balbi]

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


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> This patch series improves the handling of the END_TRANSFER command:
>
> 1) Don't issue multiple END_TRANSFER commands while the previous hasn't
>    completed
> 2) Don't issue START_TRANSFER command while END_TRANSFER hasn't completed
> 3) Remove arbitrary 100us delay
>
> Changes in v2:
>  - Remove unused variable

Tried applying these, but it failed on patch 1.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v2 0/3] usb: dwc3: gadget: Improve END_TRANSFER handling

[Thinh Nguyen]

Hi Felipe,

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> This patch series improves the handling of the END_TRANSFER command:
>>
>> 1) Don't issue multiple END_TRANSFER commands while the previous hasn't
>>     completed
>> 2) Don't issue START_TRANSFER command while END_TRANSFER hasn't completed
>> 3) Remove arbitrary 100us delay
>>
>> Changes in v2:
>>   - Remove unused variable
> Tried applying these, but it failed on patch 1.
>

Your next branch doesn't have these prerequisite patches from mainline:
d3abda5a98a1 ("usb: dwc3: gadget: Clear started flag for non-IOC")
2d7b78f59e02 ("usb: dwc3: ep0: Clear started flag on completion")

Will you rebase on v5.5 rc-x? Let me know how we can proceed to resolve 
the conflict.

Also, have you a chance to take a look at these patches:
https://patchwork.kernel.org/project/linux-usb/list/?series=214669
* usb: dwc3: gadget: Properly set maxpacket limit
* usb: dwc3: Fix GTXFIFOSIZ.TXFDEP macro name


Thanks!
Thinh

Re: [PATCH v2 0/3] usb: dwc3: gadget: Improve END_TRANSFER handling

[Felipe Balbi]

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


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> This patch series improves the handling of the END_TRANSFER command:
>>>
>>> 1) Don't issue multiple END_TRANSFER commands while the previous hasn't
>>>     completed
>>> 2) Don't issue START_TRANSFER command while END_TRANSFER hasn't completed
>>> 3) Remove arbitrary 100us delay
>>>
>>> Changes in v2:
>>>   - Remove unused variable
>> Tried applying these, but it failed on patch 1.
>>
>
> Your next branch doesn't have these prerequisite patches from mainline:
> d3abda5a98a1 ("usb: dwc3: gadget: Clear started flag for non-IOC")
> 2d7b78f59e02 ("usb: dwc3: ep0: Clear started flag on completion")
>
> Will you rebase on v5.5 rc-x? Let me know how we can proceed to resolve 
> the conflict.
>
> Also, have you a chance to take a look at these patches:
> https://patchwork.kernel.org/project/linux-usb/list/?series=214669
> * usb: dwc3: gadget: Properly set maxpacket limit
> * usb: dwc3: Fix GTXFIFOSIZ.TXFDEP macro name

indeed, now they've applied cleanly. Thanks

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]