[v3] usb: dwc3: gadget: Add 1ms delay after end transfer command without IOC

  Previously, there was a 100uS delay inserted after issuing an end transfer
command for specific controller revisions.  This was due to the fact that
there was a GUCTL2 bit field which enabled synchronous completion of the
end transfer command once the CMDACT bit was cleared in the DEPCMD
register.  Since this bit does not exist for all controller revisions, add
the delay back in, and increase the duration to 1ms for the controller to
complete the command.

An issue was seen where the USB request buffer was unmapped while the DWC3
controller was still accessing the TRB.  However, it was confirmed that the
end transfer command was successfully submitted. (no end transfer timeout)
In situations, such as dwc3_gadget_soft_disconnect() and
__dwc3_gadget_ep_disable(), the dwc3_remove_request() is utilized, which
will issue the end transfer command, and follow up with
dwc3_gadget_giveback().  At least for the USB ep disable path, it is
required for any pending and started requests to be completed and returned
to the function driver in the same context of the disable call.  Without
the GUCTL2 bit, it is not ensured that the end transfer is completed before
the buffers are unmapped.

Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
Changes in v3:
- Fixed subject title and modified commit text to reference the new 1ms
delay

Changes in v2:
- Increase delay value to 1ms
- Make this applicable to DWC32 revisions as well

 drivers/usb/dwc3/gadget.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)
  

On Tue, Feb 28, 2023 at 12:24:18PM -0800, Wesley Cheng wrote:
> Previously, there was a 100uS delay inserted after issuing an end transfer
> command for specific controller revisions.  This was due to the fact that
> there was a GUCTL2 bit field which enabled synchronous completion of the
> end transfer command once the CMDACT bit was cleared in the DEPCMD
> register.  Since this bit does not exist for all controller revisions, add
> the delay back in, and increase the duration to 1ms for the controller to
> complete the command.
> 
> An issue was seen where the USB request buffer was unmapped while the DWC3
> controller was still accessing the TRB.  However, it was confirmed that the
> end transfer command was successfully submitted. (no end transfer timeout)
> In situations, such as dwc3_gadget_soft_disconnect() and
> __dwc3_gadget_ep_disable(), the dwc3_remove_request() is utilized, which
> will issue the end transfer command, and follow up with
> dwc3_gadget_giveback().  At least for the USB ep disable path, it is
> required for any pending and started requests to be completed and returned
> to the function driver in the same context of the disable call.  Without
> the GUCTL2 bit, it is not ensured that the end transfer is completed before
> the buffers are unmapped.
> 
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
> Changes in v3:
> - Fixed subject title and modified commit text to reference the new 1ms
> delay
> 
> Changes in v2:
> - Increase delay value to 1ms
> - Make this applicable to DWC32 revisions as well
> 
>  drivers/usb/dwc3/gadget.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 3c63fa97a680..15adf07a4df4 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1699,6 +1699,7 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>   */
>  static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt)
>  {
> +	struct dwc3 *dwc = dep->dwc;
>  	struct dwc3_gadget_ep_cmd_params params;
>  	u32 cmd;
>  	int ret;
> @@ -1722,10 +1723,14 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int
>  	WARN_ON_ONCE(ret);
>  	dep->resource_index = 0;
>  
> -	if (!interrupt)
> +	if (!interrupt) {
> +		if (DWC3_IP_IS(DWC32) || DWC3_IP_IS(DWC31) ||
> +			DWC3_VER_IS_PRIOR(DWC3, 310A))

How about a little more succinctly:

		if (!DWC3_IP_IS(DWC3) || DWC3_VER_IS_PRIOR(DWC3, 310A))
?

Jack
  

On Tue, Feb 28, 2023, Jack Pham wrote:
> On Tue, Feb 28, 2023 at 12:24:18PM -0800, Wesley Cheng wrote:
> > Previously, there was a 100uS delay inserted after issuing an end transfer
> > command for specific controller revisions.  This was due to the fact that
> > there was a GUCTL2 bit field which enabled synchronous completion of the
> > end transfer command once the CMDACT bit was cleared in the DEPCMD
> > register.  Since this bit does not exist for all controller revisions, add
> > the delay back in, and increase the duration to 1ms for the controller to
> > complete the command.
> > 
> > An issue was seen where the USB request buffer was unmapped while the DWC3
> > controller was still accessing the TRB.  However, it was confirmed that the
> > end transfer command was successfully submitted. (no end transfer timeout)
> > In situations, such as dwc3_gadget_soft_disconnect() and
> > __dwc3_gadget_ep_disable(), the dwc3_remove_request() is utilized, which
> > will issue the end transfer command, and follow up with
> > dwc3_gadget_giveback().  At least for the USB ep disable path, it is
> > required for any pending and started requests to be completed and returned
> > to the function driver in the same context of the disable call.  Without
> > the GUCTL2 bit, it is not ensured that the end transfer is completed before
> > the buffers are unmapped.
> > 
> > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> > ---
> > Changes in v3:
> > - Fixed subject title and modified commit text to reference the new 1ms
> > delay
> > 
> > Changes in v2:
> > - Increase delay value to 1ms
> > - Make this applicable to DWC32 revisions as well
> > 
> >  drivers/usb/dwc3/gadget.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 3c63fa97a680..15adf07a4df4 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -1699,6 +1699,7 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
> >   */
> >  static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt)
> >  {
> > +	struct dwc3 *dwc = dep->dwc;
> >  	struct dwc3_gadget_ep_cmd_params params;
> >  	u32 cmd;
> >  	int ret;
> > @@ -1722,10 +1723,14 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int
> >  	WARN_ON_ONCE(ret);
> >  	dep->resource_index = 0;
> >  
> > -	if (!interrupt)
> > +	if (!interrupt) {
> > +		if (DWC3_IP_IS(DWC32) || DWC3_IP_IS(DWC31) ||
> > +			DWC3_VER_IS_PRIOR(DWC3, 310A))
> 
> How about a little more succinctly:
> 
> 		if (!DWC3_IP_IS(DWC3) || DWC3_VER_IS_PRIOR(DWC3, 310A))
> ?
> 

or this:
	if (!DWC3_VER_IS_WITHIN(DWC3, 310A, ANY))


BR,
Thinh
  

On Tue, Feb 28, 2023, Wesley Cheng wrote:
> Previously, there was a 100uS delay inserted after issuing an end transfer
> command for specific controller revisions.  This was due to the fact that
> there was a GUCTL2 bit field which enabled synchronous completion of the
> end transfer command once the CMDACT bit was cleared in the DEPCMD
> register.  Since this bit does not exist for all controller revisions, add
> the delay back in, and increase the duration to 1ms for the controller to
> complete the command.
> 
> An issue was seen where the USB request buffer was unmapped while the DWC3
> controller was still accessing the TRB.  However, it was confirmed that the
> end transfer command was successfully submitted. (no end transfer timeout)
> In situations, such as dwc3_gadget_soft_disconnect() and
> __dwc3_gadget_ep_disable(), the dwc3_remove_request() is utilized, which
> will issue the end transfer command, and follow up with
> dwc3_gadget_giveback().  At least for the USB ep disable path, it is
> required for any pending and started requests to be completed and returned
> to the function driver in the same context of the disable call.  Without
> the GUCTL2 bit, it is not ensured that the end transfer is completed before
> the buffers are unmapped.
> 
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
> Changes in v3:
> - Fixed subject title and modified commit text to reference the new 1ms
> delay
> 
> Changes in v2:
> - Increase delay value to 1ms
> - Make this applicable to DWC32 revisions as well
> 
>  drivers/usb/dwc3/gadget.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 3c63fa97a680..15adf07a4df4 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1699,6 +1699,7 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>   */
>  static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt)
>  {
> +	struct dwc3 *dwc = dep->dwc;
>  	struct dwc3_gadget_ep_cmd_params params;
>  	u32 cmd;
>  	int ret;
> @@ -1722,10 +1723,14 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int
>  	WARN_ON_ONCE(ret);
>  	dep->resource_index = 0;
>  
> -	if (!interrupt)
> +	if (!interrupt) {
> +		if (DWC3_IP_IS(DWC32) || DWC3_IP_IS(DWC31) ||
> +			DWC3_VER_IS_PRIOR(DWC3, 310A))
> +			mdelay(1);
>  		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
> -	else if (!ret)
> +	} else if (!ret) {
>  		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
> +	}
>  
>  	dep->flags &= ~DWC3_EP_DELAY_STOP;
>  	return ret;
> @@ -3774,7 +3779,11 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>  	 * enabled, the EndTransfer command will have completed upon
>  	 * returning from this function.
>  	 *
> -	 * This mode is NOT available on the DWC_usb31 IP.
> +	 * This mode is NOT available on the DWC_usb31 IP.  In this
> +	 * case, if the IOC bit is not set, then delay by 100uS

100us -> 1ms

> +	 * after issuing the EndTransfer command.  This allows for the
> +	 * controller to handle the command completely before DWC3
> +	 * remove requests attempts to unmap USB request buffers.
>  	 */
>  
>  	__dwc3_stop_active_transfer(dep, force, interrupt);

BR,
Thinh