Re: [PATCH v2] usb: dwc3: gadget: wait for End Transfer to complete

From: Baolin Wang
Date: Tue Nov 01 2016 - 07:46:49 EST

Next message: Robin Murphy: "Re: [PATCH 2/2] swiotlb: Add swiotlb=nobounce debug option"
Previous message: Richard Weinberger: "Re: [PATCH] UBIFS: Remove some dead code"
In reply to: Felipe Balbi: "Re: [PATCH v2] usb: dwc3: gadget: wait for End Transfer to complete"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

On 1 November 2016 at 19:36, Felipe Balbi <balbi@xxxxxxxxxx> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@xxxxxxxxxx> writes:
>> Hi,
>>
>> On 1 November 2016 at 19:01, Felipe Balbi <balbi@xxxxxxxxxx> wrote:
>>>
>>> Hi,
>>>
>>> Baolin Wang <baolin.wang@xxxxxxxxxx> writes:
>>>> Changes since v1:
>>>> - Move the suspend checking to right place to avoid checking twice.
>>>
>>> there is still one problem
>>>
>>>> @@ -1736,12 +1739,38 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>>>> {
>>>> struct dwc3 *dwc = gadget_to_dwc(g);
>>>> unsigned long flags;
>>>> + int epnum;
>>>>
>>>> spin_lock_irqsave(&dwc->lock, flags);
>>>> __dwc3_gadget_stop(dwc);
>>>
>>> this tries to access registers. If __dwc3_gadget_stop() runs while
>>> clocks are gated, we will get Data Abort exception.
>>
>> We have suspend checking in __dwc3_gadget_stop(), thus it will not
>> access registers if clocks are disabled.
>>
>>>
>>> How about this version, instead?
>>
>> I think it is OK except we missed set 'dwc->gadget_driver = NULL'.
>
> argh. Man, next time we meet in a conference, I owe you a beer :-)

Okay:) Now it looks good to me. Thanks.

>
> 8<------------------------------------------------------------------------------
> From 06a204f1c2276ca1ffe68d8d59ef2e2ead337bba Mon Sep 17 00:00:00 2001
> From: Baolin Wang <baolin.wang@xxxxxxxxxx>
> Date: Mon, 31 Oct 2016 19:38:36 +0800
> Subject: [PATCH] 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.
>
> [ felipe.balbi@xxxxxxxxxxxxxxx: minor improvements ]
>
> Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxx>
> Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
> ---
> drivers/usb/dwc3/core.h | 8 ++++++++
> drivers/usb/dwc3/gadget.c | 49 +++++++++++++++++++++++++++++++++++++++--------
> 2 files changed, 49 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 5fc437021ac7..c2b86856e85d 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>
> @@ -505,6 +506,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
> @@ -530,6 +532,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;
>
> @@ -546,6 +550,7 @@ 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)
>
> /* This last one is specific to EP0 */
> #define DWC3_EP0_DIR_IN (1 << 31)
> @@ -1050,6 +1055,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 4743e53cc295..64d01ff8c119 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -593,11 +593,14 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
> dep->comp_desc = comp_desc;
> dep->type = usb_endpoint_type(desc);
> dep->flags |= DWC3_EP_ENABLED;
> + dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
>
> reg = dwc3_readl(dwc->regs, DWC3_DALEPENA);
> 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;
>
> @@ -699,7 +702,7 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
> dep->endpoint.desc = NULL;
> dep->comp_desc = NULL;
> dep->type = 0;
> - dep->flags = 0;
> + dep->flags &= DWC3_EP_END_TRANSFER_PENDING;
>
> return 0;
> }
> @@ -1783,9 +1786,6 @@ static int dwc3_gadget_start(struct usb_gadget *g,
>
> static void __dwc3_gadget_stop(struct dwc3 *dwc)
> {
> - if (pm_runtime_suspended(dwc->dev))
> - return;
> -
> dwc3_gadget_disable_irq(dwc);
> __dwc3_gadget_ep_disable(dwc->eps[0]);
> __dwc3_gadget_ep_disable(dwc->eps[1]);
> @@ -1795,9 +1795,30 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
> {
> struct dwc3 *dwc = gadget_to_dwc(g);
> unsigned long flags;
> + int epnum;
>
> spin_lock_irqsave(&dwc->lock, flags);
> +
> + if (pm_runtime_suspended(dwc->dev))
> + goto out;
> +
> __dwc3_gadget_stop(dwc);
> +
> + for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
> + struct dwc3_ep *dep = dwc->eps[epnum];
> +
> + if (!dep)
> + continue;
> +
> + if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
> + continue;
> +
> + wait_event_lock_irq(dep->wait_end_transfer,
> + !(dep->flags & DWC3_EP_END_TRANSFER_PENDING),
> + dwc->lock);
> + }
> +
> +out:
> dwc->gadget_driver = NULL;
> spin_unlock_irqrestore(&dwc->lock, flags);
>
> @@ -2171,10 +2192,12 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
> {
> struct dwc3_ep *dep;
> u8 epnum = event->endpoint_number;
> + u8 cmd;
>
> dep = dwc->eps[epnum];
>
> - if (!(dep->flags & DWC3_EP_ENABLED))
> + if (!(dep->flags & DWC3_EP_ENABLED) &&
> + !(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
> return;
>
> if (epnum == 0 || epnum == 1) {
> @@ -2215,8 +2238,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;
> }
> }
> @@ -2269,7 +2299,8 @@ 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;
>
> /*
> @@ -2313,8 +2344,10 @@ static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum, bool force)
> dep->resource_index = 0;
> dep->flags &= ~DWC3_EP_BUSY;
>
> - if (dwc3_is_usb31(dwc) || dwc->revision < DWC3_REVISION_310A)
> + if (dwc3_is_usb31(dwc) || dwc->revision < DWC3_REVISION_310A) {
> + dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
> udelay(100);
> + }
> }
>
> static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)
> --
> 2.10.1
>
>
>
> --
> balbi

--
Baolin.wang
Best Regards

Next message: Robin Murphy: "Re: [PATCH 2/2] swiotlb: Add swiotlb=nobounce debug option"
Previous message: Richard Weinberger: "Re: [PATCH] UBIFS: Remove some dead code"
In reply to: Felipe Balbi: "Re: [PATCH v2] usb: dwc3: gadget: wait for End Transfer to complete"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]