Hi Felipe,
On 14 October 2016 at 19:04, Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> wrote:
>
> 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>
>From my testing, there are still some problems caused by the nested
lock, at lease I found 2 functions will issue the usb_ep_disable()
function with spinlock:
1. In f_fs.c file
static void ffs_func_eps_disable(struct ffs_function *func)
{
struct ffs_ep *ep = func->eps;
struct ffs_epfile *epfile = func->ffs->epfiles;
unsigned count = func->ffs->eps_count;
unsigned long flags;
do {
if (epfile)
mutex_lock(&epfile->mutex);
spin_lock_irqsave(&func->ffs->eps_lock, flags);
/* pending requests get nuked */
if (likely(ep->ep))
usb_ep_disable(ep->ep);
++ep;
spin_unlock_irqrestore(&func->ffs->eps_lock, flags);
if (epfile) {
epfile->ep = NULL;
kfree(epfile->read_buffer);
epfile->read_buffer = NULL;
mutex_unlock(&epfile->mutex);
++epfile;
}
} while (--count);
}
2. In f_phonet.c file
static void __pn_reset(struct usb_function *f)
{
struct f_phonet *fp = func_to_pn(f);
struct net_device *dev = fp->dev;
struct phonet_port *port = netdev_priv(dev);
netif_carrier_off(dev);
port->usb = NULL;
usb_ep_disable(fp->out_ep);
usb_ep_disable(fp->in_ep);
if (fp->rx.skb) {
dev_kfree_skb_irq(fp->rx.skb);
fp->rx.skb = NULL;
}
}
static void pn_disconnect(struct usb_function *f)
{
struct f_phonet *fp = func_to_pn(f);
struct phonet_port *port = netdev_priv(fp->dev);
unsigned long flags;
/* remain disabled until set_alt */
spin_lock_irqsave(&port->lock, flags);
__pn_reset(f);
spin_unlock_irqrestore(&port->lock, flags);
}
> ---
>
> . 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
--
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
