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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 31 October 2016 at 20:53, Felipe Balbi <balbi@xxxxxxxxxx> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@xxxxxxxxxx> 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.
>>
>> [ felipe.balbi@xxxxxxxxxxxxxxx: minor improvements ]
>>
>> Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxx>
>
> I made one extra modification to prevent us from checking for
> pm_runtime_suspended() twice:
>
> commit 8f48e8d6d3dfe75b5582c8a7b1ee5739a393748c
> Author: Baolin Wang <baolin.wang@xxxxxxxxxx>
> Date:   Mon Oct 31 19:38:36 2016 +0800
>
>     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>
>
> 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 3b53a5714df4..d544e7369776 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,10 +1795,29 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>  {
>         struct dwc3             *dwc = gadget_to_dwc(g);
>         unsigned long           flags;
> +       int                     epnum;
> +
> +       if (pm_runtime_suspended(dwc->dev))
> +               return 0;

If we set the suspend checking at here, we will lose the chance to
free gadget irq when stopping gadget.

>
>         spin_lock_irqsave(&dwc->lock, flags);
>         __dwc3_gadget_stop(dwc);
>         dwc->gadget_driver      = NULL;

Can we set the suspend checking at here? And we should not delete the
suspend checking in __dwc3_gadget_stop() function.

+          if (pm_runtime_suspended(dwc->dev)) {
+                  spin_unlock_irqrestore(&dwc->lock, flags);
+                  goto out;
+          }

> +
> +       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);
> +       }
> +
>         spin_unlock_irqrestore(&dwc->lock, flags);

Add one tag here:

+ out:
>         free_irq(dwc->irq_gadget, dwc->ev_buf);
> @@ -2171,10 +2190,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 +2236,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 +2297,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 +2342,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)
>
> --
> 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



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux