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

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

2016-11-01 Thread Baolin Wang

Hi,

On 1 November 2016 at 19:36, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>> Hi,
>>
>> On 1 November 2016 at 19:01, Felipe Balbi  wrote:
>>>
>>> Hi,
>>>
>>> Baolin Wang  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(>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 
> 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.ba...@linux.intel.com: minor improvements ]
>
> Signed-off-by: Baolin Wang 
> Signed-off-by: Felipe Balbi 
> ---
>  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 
>  #include 
>  #include 
> +#include 
>
>  #include 
>  #include 
> @@ -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_headpending_list;
> struct list_headstarted_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(>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

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

2016-11-01 Thread Felipe Balbi


Hi,

Baolin Wang  writes:
> Hi,
>
> On 1 November 2016 at 19:01, Felipe Balbi  wrote:
>>
>> Hi,
>>
>> Baolin Wang  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(>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 :-)

8<--
From 06a204f1c2276ca1ffe68d8d59ef2e2ead337bba Mon Sep 17 00:00:00 2001
From: Baolin Wang 
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.ba...@linux.intel.com: minor improvements ]

Signed-off-by: Baolin Wang 
Signed-off-by: Felipe Balbi 
---
 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 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -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_headpending_list;
struct list_headstarted_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(>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

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

2016-11-01 Thread Baolin Wang

Hi,

On 1 November 2016 at 19:01, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  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(>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'.

>
> 8<--
> From e21260f349271ff1d12e919be20a9ee47e29e4b4 Mon Sep 17 00:00:00 2001
> From: Baolin Wang 
> 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.ba...@linux.intel.com: minor improvements ]
>
> Signed-off-by: Baolin Wang 
> Signed-off-by: Felipe Balbi 
> ---
>  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 
>  #include 
>  #include 
> +#include 
>
>  #include 
>  #include 
> @@ -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_headpending_list;
> struct list_headstarted_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..a375fd28ed96 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(>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,31 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>  {
> struct dwc3 *dwc =

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

2016-11-01 Thread Felipe Balbi


Hi,

Baolin Wang  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(>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.

How about this version, instead?

8<--
From e21260f349271ff1d12e919be20a9ee47e29e4b4 Mon Sep 17 00:00:00 2001
From: Baolin Wang 
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.ba...@linux.intel.com: minor improvements ]

Signed-off-by: Baolin Wang 
Signed-off-by: Felipe Balbi 
---
 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 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -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_headpending_list;
struct list_headstarted_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..a375fd28ed96 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(>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,31 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
 {
struct dwc3 *dwc = gadget_to_dwc(g);
unsigned long   flags;
+   int epnum;
 
spin_lock_irqsave(>lock, flags);
+
+   if (pm_runtime_suspended(dwc->dev))
+   goto out;
+
__dwc3_gadget_stop(dwc);
dwc->gadget_driver  = NULL;
+
+   for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
+   struct dwc3_ep  *dep = dwc->eps[epnum];
+
+   if (!dep)
+   continue;
+
+

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

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

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

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

4 matches

Site Navigation

Mail list logo

Footer information