Hi,
Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> writes:
> Instead of just delaying for 100us, we should
> actually wait for End Transfer Command Complete
> interrupt before moving on. Note that this should
> only be done if we're dealing with one of the core
> revisions that actually require the interrupt before
> moving on.
>
> Reported-by: Baolin Wang <baolin.wang@xxxxxxxxxx>
> Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
I've updated this one in order to fix the comment we had about delaying
100us. No further changes were made, only the comment. Here it is:
8<------------------------------------------------------------------------
From f3fa94f3171709f787a30e3c5ce69a668960b66e Mon Sep 17 00:00:00 2001
From: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
Date: Thu, 13 Oct 2016 14:09:47 +0300
Subject: [PATCH v2] usb: dwc3: gadget: wait for End Transfer to complete
Instead of just delaying for 100us, we should
actually wait for End Transfer Command Complete
interrupt before moving on. Note that this should
only be done if we're dealing with one of the core
revisions that actually require the interrupt before
moving on.
Reported-by: Baolin Wang <baolin.wang@xxxxxxxxxx>
Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
---
. Changes since v1:
- Fix comment
drivers/usb/dwc3/core.h | 10 ++++++-
drivers/usb/dwc3/gadget.c | 71 +++++++++++++++++++++++++++--------------------
2 files changed, 50 insertions(+), 31 deletions(-)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index e878366ead00..2f6f7c4bc8d4 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -26,6 +26,7 @@
#include <linux/dma-mapping.h>
#include <linux/mm.h>
#include <linux/debugfs.h>
+#include <linux/wait.h>
#include <linux/usb/ch9.h>
#include <linux/usb/gadget.h>
@@ -504,6 +505,7 @@ struct dwc3_event_buffer {
* @endpoint: usb endpoint
* @pending_list: list of pending requests for this endpoint
* @started_list: list of started requests on this endpoint
+ * @wait_end_transfer: wait_queue_head_t for waiting on End Transfer complete
* @lock: spinlock for endpoint request queue traversal
* @regs: pointer to first endpoint register
* @trb_pool: array of transaction buffers
@@ -529,6 +531,8 @@ struct dwc3_ep {
struct list_head pending_list;
struct list_head started_list;
+ wait_queue_head_t wait_end_transfer;
+
spinlock_t lock;
void __iomem *regs;
@@ -545,7 +549,8 @@ struct dwc3_ep {
#define DWC3_EP_BUSY (1 << 4)
#define DWC3_EP_PENDING_REQUEST (1 << 5)
#define DWC3_EP_MISSED_ISOC (1 << 6)
-
+#define DWC3_EP_END_TRANSFER_PENDING (1 << 7)
+#define DWC3_EP_KICK_POST_END_TRANSFER (1 << 8)
/* This last one is specific to EP0 */
#define DWC3_EP0_DIR_IN (1 << 31)
@@ -1047,6 +1052,9 @@ struct dwc3_event_depevt {
#define DEPEVT_TRANSFER_BUS_EXPIRY 2
u32 parameters:16;
+
+/* For Command Complete Events */
+#define DEPEVT_PARAMETER_CMD(n) (((n) & (0xf << 8)) >> 8)
} __packed;
/**
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 3ba05b12e49a..05a5b54a001b 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -598,6 +598,8 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
reg |= DWC3_DALEPENA_EP(dep->number);
dwc3_writel(dwc->regs, DWC3_DALEPENA, reg);
+ init_waitqueue_head(&dep->wait_end_transfer);
+
if (usb_endpoint_xfer_control(desc))
return 0;
@@ -1151,6 +1153,11 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
if (!dwc3_calc_trbs_left(dep))
return 0;
+ if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) {
+ dep->flags |= DWC3_EP_KICK_POST_END_TRANSFER;
+ return 0;
+ }
+
ret = __dwc3_gadget_kick_transfer(dep, 0);
if (ret && ret != -EBUSY)
dwc3_trace(trace_dwc3_gadget,
@@ -2156,6 +2163,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
{
struct dwc3_ep *dep;
u8 epnum = event->endpoint_number;
+ u8 cmd;
dep = dwc->eps[epnum];
@@ -2200,8 +2208,15 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
return;
}
break;
- case DWC3_DEPEVT_RXTXFIFOEVT:
case DWC3_DEPEVT_EPCMDCMPLT:
+ cmd = DEPEVT_PARAMETER_CMD(event->parameters);
+
+ if (cmd == DWC3_DEPCMD_ENDTRANSFER) {
+ dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
+ wake_up(&dep->wait_end_transfer);
+ }
+ break;
+ case DWC3_DEPEVT_RXTXFIFOEVT:
break;
}
}
@@ -2246,6 +2261,7 @@ static void dwc3_reset_gadget(struct dwc3 *dwc)
}
static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum, bool force)
+__releases(&dwc->lock) __acquires(&dwc->lock)
{
struct dwc3_ep *dep;
struct dwc3_gadget_ep_cmd_params params;
@@ -2254,36 +2270,20 @@ static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum, bool force)
dep = dwc->eps[epnum];
- if (!dep->resource_index)
+ if ((dep->flags & DWC3_EP_END_TRANSFER_PENDING) ||
+ !dep->resource_index)
return;
/*
- * NOTICE: We are violating what the Databook says about the
- * EndTransfer command. Ideally we would _always_ wait for the
- * EndTransfer Command Completion IRQ, but that's causing too
- * 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.
- *
- * 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
- *
- * As of IP version 3.10a of the DWC_usb3 IP, the controller
- * supports a mode to work around the above limitation. The
- * software can poll the CMDACT bit in the DEPCMD register
- * after issuing a EndTransfer command. This mode is enabled
- * 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.
+ * As of IP version 3.10a of the DWC_usb3 IP, the controller supports a
+ * new mode of operation where we don't need to wait for EndTransfer
+ * Complete Interrupt. Software can poll CMDACT bit in DEPCMD register
+ * after issuing an EndTransfer command. This mode is enabled by writing
+ * GUCTL2[14]. This polling is already done in the
+ * dwc3_send_gadget_ep_cmd() function so if the mode is enabled,
+ * EndTransfer command will have completed upon returning from this
+ * function and we don't need to wait for EndTransfer Command Complete
+ * IRQ.
*
* This mode is NOT available on the DWC_usb31 IP.
*/
@@ -2295,11 +2295,22 @@ static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum, bool force)
memset(¶ms, 0, sizeof(params));
ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms);
WARN_ON_ONCE(ret);
+
+ if (dwc3_is_usb31(dwc) || dwc->revision < DWC3_REVISION_310A) {
+ dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
+ wait_event_lock_irq(dep->wait_end_transfer,
+ !(dep->flags & DWC3_EP_END_TRANSFER_PENDING),
+ dwc->lock);
+ }
+
dep->resource_index = 0;
dep->flags &= ~DWC3_EP_BUSY;
- if (dwc3_is_usb31(dwc) || dwc->revision < DWC3_REVISION_310A)
- udelay(100);
+ if (dep->flags & DWC3_EP_KICK_POST_END_TRANSFER) {
+ dep->flags &= ~DWC3_EP_KICK_POST_END_TRANSFER;
+ ret = __dwc3_gadget_kick_transfer(dep, 0);
+ WARN_ON_ONCE(ret);
+ }
}
static void dwc3_stop_active_transfers(struct dwc3 *dwc)
--
2.10.0.440.g21f862b
--
balbi
Attachment:
signature.asc
Description: PGP signature
