[PATCH 0/5] usb: dwc3: gadget: Handle streams

[PATCH 0/5] usb: dwc3: gadget: Handle streams

[Thinh Nguyen]

The dwc3 driver wasn't implemented to handle streams. This series provide a few
updates to dwc3 and usb request for that:
* Introduce usb_request->is_last field
* Handle dwc3 transfer completion
* Handle dwc3 stream events

To use stream, the dwc3 driver must know which request is the last request of a
transfer. The controller uses this information to reallocate its resources for
different streams as transfers are completed. Function drivers that support
stream must indicate this via usb_request->is_last field.

Please note, I also included the patch
"usb: dwc3: gadget: Continue to process pending requests" in this series to
avoid dependency issue when applying to your git tree.

https://patchwork.kernel.org/patch/11466967/

Prerequisite
------------
This series requires DWC_usb32 patch series:
https://patchwork.kernel.org/project/linux-usb/list/?series=269641

[PATCH 1/2] usb: dwc3: Add support for DWC_usb32 IP
[PATCH 2/2] usb: dwc3: Get MDWIDTH for DWC_usb32



Thinh Nguyen (5):
  usb: gadget: Introduce usb_request->is_last field
  usb: gadget: f_tcm: Inform last transfer request
  usb: dwc3: gadget: Continue to process pending requests
  usb: dwc3: gadget: Handle transfer completion
  usb: dwc3: gadget: Handle stream transfers

 drivers/usb/dwc3/core.h             |  12 ++
 drivers/usb/dwc3/debug.h            |   2 +
 drivers/usb/dwc3/gadget.c           | 225 ++++++++++++++++++++++++++++++------
 drivers/usb/gadget/function/f_tcm.c |   3 +
 include/linux/usb/gadget.h          |   2 +
 5 files changed, 211 insertions(+), 33 deletions(-)

-- 
2.11.0

[PATCH 1/5] usb: gadget: Introduce usb_request->is_last field

[Thinh Nguyen]

A transfer is composed of one or more usb_requests. Currently, only the
function driver knows this based on its implementation and its class
protocol. However, some usb controllers need to know this to update its
resources. For example, the DWC3 controller needs this info to update
its internal resources and initiate different streams.

Introduce a new field is_last to usb_request to inform the controller
driver whether the request is the last of its transfer.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 include/linux/usb/gadget.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index e959c09a97c9..742c52f7e470 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -50,6 +50,7 @@ struct usb_ep;
  * @short_not_ok: When reading data, makes short packets be
  *     treated as errors (queue stops advancing till cleanup).
  * @dma_mapped: Indicates if request has been mapped to DMA (internal)
+ * @is_last: Indicates if this request is the last of a transfer.
  * @complete: Function called when request completes, so this request and
  *	its buffer may be re-used.  The function will always be called with
  *	interrupts disabled, and it must not sleep.
@@ -108,6 +109,7 @@ struct usb_request {
 	unsigned		zero:1;
 	unsigned		short_not_ok:1;
 	unsigned		dma_mapped:1;
+	unsigned		is_last:1;
 
 	void			(*complete)(struct usb_ep *ep,
 					struct usb_request *req);
-- 
2.11.0

Re: [PATCH 1/5] usb: gadget: Introduce usb_request->is_last field

[Peter Chen]

On 20-04-27 15:27:41, Thinh Nguyen wrote:
> A transfer is composed of one or more usb_requests. Currently, only the
> function driver knows this based on its implementation and its class
> protocol. However, some usb controllers need to know this to update its
> resources. For example, the DWC3 controller needs this info to update
> its internal resources and initiate different streams.
> 
> Introduce a new field is_last to usb_request to inform the controller
> driver whether the request is the last of its transfer.
> 
> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> ---
>  include/linux/usb/gadget.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index e959c09a97c9..742c52f7e470 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -50,6 +50,7 @@ struct usb_ep;
>   * @short_not_ok: When reading data, makes short packets be
>   *     treated as errors (queue stops advancing till cleanup).
>   * @dma_mapped: Indicates if request has been mapped to DMA (internal)
> + * @is_last: Indicates if this request is the last of a transfer.

Would you please describe the use case for it, and what's 'transfer'
and 'request' in your use case?

Peter

>   * @complete: Function called when request completes, so this request and
>   *	its buffer may be re-used.  The function will always be called with
>   *	interrupts disabled, and it must not sleep.
> @@ -108,6 +109,7 @@ struct usb_request {
>  	unsigned		zero:1;
>  	unsigned		short_not_ok:1;
>  	unsigned		dma_mapped:1;
> +	unsigned		is_last:1;
>  
>  	void			(*complete)(struct usb_ep *ep,
>  					struct usb_request *req);
> -- 
> 2.11.0
> 

-- 

Thanks,
Peter Chen

Re: [PATCH 1/5] usb: gadget: Introduce usb_request->is_last field

[Thinh Nguyen]

Hi,

Peter Chen wrote:
> On 20-04-27 15:27:41, Thinh Nguyen wrote:
>> A transfer is composed of one or more usb_requests. Currently, only the
>> function driver knows this based on its implementation and its class
>> protocol. However, some usb controllers need to know this to update its
>> resources. For example, the DWC3 controller needs this info to update
>> its internal resources and initiate different streams.
>>
>> Introduce a new field is_last to usb_request to inform the controller
>> driver whether the request is the last of its transfer.
>>
>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>> ---
>>   include/linux/usb/gadget.h | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index e959c09a97c9..742c52f7e470 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -50,6 +50,7 @@ struct usb_ep;
>>    * @short_not_ok: When reading data, makes short packets be
>>    *     treated as errors (queue stops advancing till cleanup).
>>    * @dma_mapped: Indicates if request has been mapped to DMA (internal)
>> + * @is_last: Indicates if this request is the last of a transfer.
> Would you please describe the use case for it, and what's 'transfer'
> and 'request' in your use case?
>

The transfer size is defined by a class protocol. The function driver 
will determine how many usb_requests are needed to fulfill that transfer.

For example, in MSC, a READ/WRITE command can request for a transfer 
size up to (512 * 2^31-1) bytes. It'd be too large for a single 
usb_request, which is why the mass_storage function driver limits each 
request to 16KB max by default. A MSC command itself is a transfer, same 
with its status.

This is a similar case for UASP. However, the f_tcm and the target 
drivers current implementation only use a single request per transfer. 
(This needs to be fixed, along with some other issues in f_tcm).

The use case here is that DWC3x controller needs to update its resources 
whenever a transfer is completed. This is a requirement for streams when 
it needs to free up some resources for different stream transfers. The 
function driver must pass this information to the controller driver for 
streams to work properly.

Potentially, another use case for this is to update the documentation on 
usb_dequeue_request(). By providing the concept of a transfer, we can 
say that dequeuing an in-flight request will cancel the transfer, and 
any incomplete request within the transfer must be given back to the 
function driver. This would make sense for controllers that use TRB ring 
buffer and dequeue/enqueue TRB pointers. But this idea still needs more 
thoughts.

BR,
Thinh

RE: [PATCH 1/5] usb: gadget: Introduce usb_request->is_last field

[Peter Chen]

 
> > On 20-04-27 15:27:41, Thinh Nguyen wrote:
> >> A transfer is composed of one or more usb_requests. Currently, only
> >> the function driver knows this based on its implementation and its
> >> class protocol. However, some usb controllers need to know this to
> >> update its resources. For example, the DWC3 controller needs this
> >> info to update its internal resources and initiate different streams.
> >>
> >> Introduce a new field is_last to usb_request to inform the controller
> >> driver whether the request is the last of its transfer.
> >>
> >> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> >> ---
> >>   include/linux/usb/gadget.h | 2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> >> index e959c09a97c9..742c52f7e470 100644
> >> --- a/include/linux/usb/gadget.h
> >> +++ b/include/linux/usb/gadget.h
> >> @@ -50,6 +50,7 @@ struct usb_ep;
> >>    * @short_not_ok: When reading data, makes short packets be
> >>    *     treated as errors (queue stops advancing till cleanup).
> >>    * @dma_mapped: Indicates if request has been mapped to DMA
> >> (internal)
> >> + * @is_last: Indicates if this request is the last of a transfer.
> > Would you please describe the use case for it, and what's 'transfer'
> > and 'request' in your use case?
> >
> 
> The transfer size is defined by a class protocol. The function driver will determine
> how many usb_requests are needed to fulfill that transfer.
> 

Thanks for your information.

If 'transfer size' here is software concept, why controller needs to know? The general
controller internal logic doesn't care class protocol, it only cares TRB chain software prepares.

> For example, in MSC, a READ/WRITE command can request for a transfer size up
> to (512 * 2^31-1) bytes. It'd be too large for a single usb_request, which is why the
> mass_storage function driver limits each request to 16KB max by default. A MSC
> command itself is a transfer, same with its status.
> 
> This is a similar case for UASP. However, the f_tcm and the target drivers current
> implementation only use a single request per transfer.
> (This needs to be fixed, along with some other issues in f_tcm).
> 
> The use case here is that DWC3x controller needs to update its resources
> whenever a transfer is completed. This is a requirement for streams when it needs
> to free up some resources for different stream transfers. The function driver must
> pass this information to the controller driver for streams to work properly.
> 

Does the controller case about stream information or the transfer information?

> Potentially, another use case for this is to update the documentation on
> usb_dequeue_request(). By providing the concept of a transfer, we can say that
> dequeuing an in-flight request will cancel the transfer, and any incomplete request
> within the transfer must be given back to the function driver. This would make
> sense for controllers that use TRB ring buffer and dequeue/enqueue TRB pointers.
> But this idea still needs more thoughts.
> 
 
I think class driver needs to consider that, the controller driver only needs to handle
request and related TRBs.

Peter

Re: [PATCH 1/5] usb: gadget: Introduce usb_request->is_last field

[Thinh Nguyen]

Peter Chen wrote:
>   
>>> On 20-04-27 15:27:41, Thinh Nguyen wrote:
>>>> A transfer is composed of one or more usb_requests. Currently, only
>>>> the function driver knows this based on its implementation and its
>>>> class protocol. However, some usb controllers need to know this to
>>>> update its resources. For example, the DWC3 controller needs this
>>>> info to update its internal resources and initiate different streams.
>>>>
>>>> Introduce a new field is_last to usb_request to inform the controller
>>>> driver whether the request is the last of its transfer.
>>>>
>>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>>>> ---
>>>>    include/linux/usb/gadget.h | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>>>> index e959c09a97c9..742c52f7e470 100644
>>>> --- a/include/linux/usb/gadget.h
>>>> +++ b/include/linux/usb/gadget.h
>>>> @@ -50,6 +50,7 @@ struct usb_ep;
>>>>     * @short_not_ok: When reading data, makes short packets be
>>>>     *     treated as errors (queue stops advancing till cleanup).
>>>>     * @dma_mapped: Indicates if request has been mapped to DMA
>>>> (internal)
>>>> + * @is_last: Indicates if this request is the last of a transfer.
>>> Would you please describe the use case for it, and what's 'transfer'
>>> and 'request' in your use case?
>>>
>> The transfer size is defined by a class protocol. The function driver will determine
>> how many usb_requests are needed to fulfill that transfer.
>>
> Thanks for your information.
>
> If 'transfer size' here is software concept, why controller needs to know? The general
> controller internal logic doesn't care class protocol, it only cares TRB chain software prepares.

While some controllers don't have the concept of this, DWC_usb3x does. 
It has a notion of starting and ending a transfer. While a transfer is 
started, the endpoint uses a resource. It releases that resource when 
the transfer completes. So far, dwc3 implemented in such a way that bulk 
transfers are always in-progress and don't complete. That's fine so far, 
but it's not the case with streams.

>
>> For example, in MSC, a READ/WRITE command can request for a transfer size up
>> to (512 * 2^31-1) bytes. It'd be too large for a single usb_request, which is why the
>> mass_storage function driver limits each request to 16KB max by default. A MSC
>> command itself is a transfer, same with its status.
>>
>> This is a similar case for UASP. However, the f_tcm and the target drivers current
>> implementation only use a single request per transfer.
>> (This needs to be fixed, along with some other issues in f_tcm).
>>
>> The use case here is that DWC3x controller needs to update its resources
>> whenever a transfer is completed. This is a requirement for streams when it needs
>> to free up some resources for different stream transfers. The function driver must
>> pass this information to the controller driver for streams to work properly.
>>
> Does the controller case about stream information or the transfer information?

For streams, each stransfer has a stream ID, and each transfer uses a 
resource.

>
>> Potentially, another use case for this is to update the documentation on
>> usb_dequeue_request(). By providing the concept of a transfer, we can say that
>> dequeuing an in-flight request will cancel the transfer, and any incomplete request
>> within the transfer must be given back to the function driver. This would make
>> sense for controllers that use TRB ring buffer and dequeue/enqueue TRB pointers.
>> But this idea still needs more thoughts.
>>
>   
> I think class driver needs to consider that, the controller driver only needs to handle
> request and related TRBs.

It maybe true for some controllers, but DWC_usb3x needs this information.

BR,
Thinh

Re: [PATCH 1/5] usb: gadget: Introduce usb_request->is_last field

[Alan Stern]

On Thu, 30 Apr 2020, Thinh Nguyen wrote:

> Peter Chen wrote:

> > If 'transfer size' here is software concept, why controller needs to know? The general
> > controller internal logic doesn't care class protocol, it only cares TRB chain software prepares.
> 
> While some controllers don't have the concept of this, DWC_usb3x does. 
> It has a notion of starting and ending a transfer. While a transfer is 
> started, the endpoint uses a resource. It releases that resource when 
> the transfer completes. So far, dwc3 implemented in such a way that bulk 
> transfers are always in-progress and don't complete. That's fine so far, 
> but it's not the case with streams.

This is peculiar.  I haven't heard of any other controller doing this.

What does the controller use this resource for?  Would anything go 
wrong if you told the controller that each transfer was a single 
usb_request?

Alan Stern

Re: [PATCH 1/5] usb: gadget: Introduce usb_request->is_last field

[Thinh Nguyen]

Hi,

Alan Stern wrote:
> On Thu, 30 Apr 2020, Thinh Nguyen wrote:
>
>> Peter Chen wrote:
>>> If 'transfer size' here is software concept, why controller needs to know? The general
>>> controller internal logic doesn't care class protocol, it only cares TRB chain software prepares.
>> While some controllers don't have the concept of this, DWC_usb3x does.
>> It has a notion of starting and ending a transfer. While a transfer is
>> started, the endpoint uses a resource. It releases that resource when
>> the transfer completes. So far, dwc3 implemented in such a way that bulk
>> transfers are always in-progress and don't complete. That's fine so far,
>> but it's not the case with streams.
> This is peculiar.  I haven't heard of any other controller doing this.
>
> What does the controller use this resource for?  Would anything go
> wrong if you told the controller that each transfer was a single
> usb_request?

It's no problem. Each transfer can be a single request. Just set the 
request->is_last. (Refer to [patch 2/5] for f_tcm).

The issue here is that the controller needs to know when a stream 
completes so it can start on a different stream. In the controller 
driver, this is done by setting a certain control bit in the TRB 
indicating the last TRB of a transfer. This knowledge can only come from 
the function driver, which is why we need this "is_last" field.

BR,
Thinh

Re: [PATCH 1/5] usb: gadget: Introduce usb_request->is_last field

[Alan Stern]

On Thu, 30 Apr 2020, Thinh Nguyen wrote:

> Hi,
> 
> Alan Stern wrote:
> > On Thu, 30 Apr 2020, Thinh Nguyen wrote:
> >
> >> Peter Chen wrote:
> >>> If 'transfer size' here is software concept, why controller needs to know? The general
> >>> controller internal logic doesn't care class protocol, it only cares TRB chain software prepares.
> >> While some controllers don't have the concept of this, DWC_usb3x does.
> >> It has a notion of starting and ending a transfer. While a transfer is
> >> started, the endpoint uses a resource. It releases that resource when
> >> the transfer completes. So far, dwc3 implemented in such a way that bulk
> >> transfers are always in-progress and don't complete. That's fine so far,
> >> but it's not the case with streams.
> > This is peculiar.  I haven't heard of any other controller doing this.
> >
> > What does the controller use this resource for?  Would anything go
> > wrong if you told the controller that each transfer was a single
> > usb_request?
> 
> It's no problem. Each transfer can be a single request. Just set the 
> request->is_last. (Refer to [patch 2/5] for f_tcm).
> 
> The issue here is that the controller needs to know when a stream 
> completes so it can start on a different stream. In the controller 

Why does it need to know this?  Why can't it start on a different 
stream at any time?

> driver, this is done by setting a certain control bit in the TRB 
> indicating the last TRB of a transfer. This knowledge can only come from 
> the function driver, which is why we need this "is_last" field.

What's wrong with just assuming _every_ usb_request is the last one of 
its transfer?  Then you wouldn't have to add a new flag or change the 
existing function drivers.

Alan Stern

Re: [PATCH 1/5] usb: gadget: Introduce usb_request->is_last field

[Thinh Nguyen]

Alan Stern wrote:
> On Thu, 30 Apr 2020, Thinh Nguyen wrote:
>
>> Hi,
>>
>> Alan Stern wrote:
>>> On Thu, 30 Apr 2020, Thinh Nguyen wrote:
>>>
>>>> Peter Chen wrote:
>>>>> If 'transfer size' here is software concept, why controller needs to know? The general
>>>>> controller internal logic doesn't care class protocol, it only cares TRB chain software prepares.
>>>> While some controllers don't have the concept of this, DWC_usb3x does.
>>>> It has a notion of starting and ending a transfer. While a transfer is
>>>> started, the endpoint uses a resource. It releases that resource when
>>>> the transfer completes. So far, dwc3 implemented in such a way that bulk
>>>> transfers are always in-progress and don't complete. That's fine so far,
>>>> but it's not the case with streams.
>>> This is peculiar.  I haven't heard of any other controller doing this.
>>>
>>> What does the controller use this resource for?  Would anything go
>>> wrong if you told the controller that each transfer was a single
>>> usb_request?
>> It's no problem. Each transfer can be a single request. Just set the
>> request->is_last. (Refer to [patch 2/5] for f_tcm).
>>
>> The issue here is that the controller needs to know when a stream
>> completes so it can start on a different stream. In the controller
> Why does it need to know this?  Why can't it start on a different
> stream at any time?

By design, whenever the controller needs to start on a different stream, 
it will issue a START_TRANSFER command along with a stream ID. It cannot 
issue START_TRANSFER command again until the previous transfer completes 
or ended.

>
>> driver, this is done by setting a certain control bit in the TRB
>> indicating the last TRB of a transfer. This knowledge can only come from
>> the function driver, which is why we need this "is_last" field.
> What's wrong with just assuming _every_ usb_request is the last one of
> its transfer?  Then you wouldn't have to add a new flag or change the
> existing function drivers.

That will affect performance. The driver will then need to service 
multiple interrupts and restart the transfer multiple times.

BR,
Thinh

[PATCH 2/5] usb: gadget: f_tcm: Inform last transfer request

[Thinh Nguyen]

Set the request->is_last to each stream request to indicate that the
request is the last stream request of a transfer. The DWC3 controller
needs to know this info to properly allocate resource for different
streams. The current implementation of f_tcm uses a single request per
transfer, so every stream request is the last of its transfer.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 drivers/usb/gadget/function/f_tcm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 36504931b2d1..2979cbe4d95f 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -531,6 +531,7 @@ static int uasp_prepare_r_request(struct usbg_cmd *cmd)
 		stream->req_in->sg = se_cmd->t_data_sg;
 	}
 
+	stream->req_in->is_last = 1;
 	stream->req_in->complete = uasp_status_data_cmpl;
 	stream->req_in->length = se_cmd->data_length;
 	stream->req_in->context = cmd;
@@ -554,6 +555,7 @@ static void uasp_prepare_status(struct usbg_cmd *cmd)
 	 */
 	iu->len = cpu_to_be16(se_cmd->scsi_sense_length);
 	iu->status = se_cmd->scsi_status;
+	stream->req_status->is_last = 1;
 	stream->req_status->context = cmd;
 	stream->req_status->length = se_cmd->scsi_sense_length + 16;
 	stream->req_status->buf = iu;
@@ -991,6 +993,7 @@ static int usbg_prepare_w_request(struct usbg_cmd *cmd, struct usb_request *req)
 		req->sg = se_cmd->t_data_sg;
 	}
 
+	req->is_last = 1;
 	req->complete = usbg_data_write_cmpl;
 	req->length = se_cmd->data_length;
 	req->context = cmd;
-- 
2.11.0

[PATCH 3/5] usb: dwc3: gadget: Continue to process pending requests

[Thinh Nguyen]

If there are still pending requests because no TRB was available,
prepare more when started requests are completed.

Introduce dwc3_gadget_ep_should_continue() to check for incomplete and
pending requests to resume updating new TRBs to the controller's TRB
cache.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 drivers/usb/dwc3/gadget.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 4ca3e197bee4..865e6fbb7360 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2605,10 +2605,8 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
 
 	req->request.actual = req->request.length - req->remaining;
 
-	if (!dwc3_gadget_ep_request_completed(req)) {
-		__dwc3_gadget_kick_transfer(dep);
+	if (!dwc3_gadget_ep_request_completed(req))
 		goto out;
-	}
 
 	dwc3_gadget_giveback(dep, req, status);
 
@@ -2632,6 +2630,24 @@ static void dwc3_gadget_ep_cleanup_completed_requests(struct dwc3_ep *dep,
 	}
 }
 
+static bool dwc3_gadget_ep_should_continue(struct dwc3_ep *dep)
+{
+	struct dwc3_request	*req;
+
+	if (!list_empty(&dep->pending_list))
+		return true;
+
+	/*
+	 * We only need to check the first entry of the started list. We can
+	 * assume the completed requests are removed from the started list.
+	 */
+	req = next_request(&dep->started_list);
+	if (!req)
+		return false;
+
+	return !dwc3_gadget_ep_request_completed(req);
+}
+
 static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep,
 		const struct dwc3_event_depevt *event)
 {
@@ -2661,6 +2677,8 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
 
 	if (stop)
 		dwc3_stop_active_transfer(dep, true, true);
+	else if (dwc3_gadget_ep_should_continue(dep))
+		__dwc3_gadget_kick_transfer(dep);
 
 	/*
 	 * WORKAROUND: This is the 2nd half of U1/U2 -> U0 workaround.
-- 
2.11.0

[PATCH 4/5] usb: dwc3: gadget: Handle transfer completion

[Thinh Nguyen]

With the new usb_request->is_last field, now the function drivers can
inform which request is the end of a transfer, dwc3 can program its TRBs
to let the controller know when to free its resources when a transfer
completes. This is required for stream transfers. The controller needs
to know where one stream starts and ends to properly allocate resources
for different streams.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 drivers/usb/dwc3/core.h   |   1 +
 drivers/usb/dwc3/gadget.c | 107 ++++++++++++++++++++++++++++++++--------------
 2 files changed, 77 insertions(+), 31 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 7204a838ec06..b11183a715a7 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -701,6 +701,7 @@ struct dwc3_ep {
 #define DWC3_EP_END_TRANSFER_PENDING BIT(4)
 #define DWC3_EP_PENDING_REQUEST	BIT(5)
 #define DWC3_EP_DELAY_START	BIT(6)
+#define DWC3_EP_WAIT_TRANSFER_COMPLETE	BIT(7)
 
 	/* This last one is specific to EP0 */
 #define DWC3_EP0_DIR_IN		BIT(31)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 865e6fbb7360..628f9d142876 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -579,6 +579,7 @@ static int dwc3_gadget_set_ep_config(struct dwc3_ep *dep, unsigned int action)
 
 	if (usb_ss_max_streams(comp_desc) && usb_endpoint_xfer_bulk(desc)) {
 		params.param1 |= DWC3_DEPCFG_STREAM_CAPABLE
+			| DWC3_DEPCFG_XFER_COMPLETE_EN
 			| DWC3_DEPCFG_STREAM_EVENT_EN;
 		dep->stream_capable = true;
 	}
@@ -917,8 +918,9 @@ static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep)
 }
 
 static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
-		dma_addr_t dma, unsigned length, unsigned chain, unsigned node,
-		unsigned stream_id, unsigned short_not_ok, unsigned no_interrupt)
+		dma_addr_t dma, unsigned length, unsigned chain,
+		unsigned is_last, unsigned node, unsigned stream_id,
+		unsigned short_not_ok, unsigned no_interrupt)
 {
 	struct dwc3		*dwc = dep->dwc;
 	struct usb_gadget	*gadget = &dwc->gadget;
@@ -1011,6 +1013,8 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
 
 	if (chain)
 		trb->ctrl |= DWC3_TRB_CTRL_CHN;
+	else if (!usb_endpoint_xfer_isoc(dep->endpoint.desc) && is_last)
+		trb->ctrl |= DWC3_TRB_CTRL_LST;
 
 	if (usb_endpoint_xfer_bulk(dep->endpoint.desc) && dep->stream_capable)
 		trb->ctrl |= DWC3_TRB_CTRL_SID_SOFN(stream_id);
@@ -1038,6 +1042,7 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
 	unsigned		stream_id = req->request.stream_id;
 	unsigned		short_not_ok = req->request.short_not_ok;
 	unsigned		no_interrupt = req->request.no_interrupt;
+	unsigned		is_last = req->request.is_last;
 
 	if (req->request.num_sgs > 0) {
 		length = sg_dma_len(req->start_sg);
@@ -1057,7 +1062,7 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
 
 	req->num_trbs++;
 
-	__dwc3_prepare_one_trb(dep, trb, dma, length, chain, node,
+	__dwc3_prepare_one_trb(dep, trb, dma, length, chain, is_last, node,
 			stream_id, short_not_ok, no_interrupt);
 }
 
@@ -1100,7 +1105,8 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
 			trb = &dep->trb_pool[dep->trb_enqueue];
 			req->num_trbs++;
 			__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr,
-					maxp - rem, false, 1,
+					maxp - rem, false,
+					req->request.is_last, 1,
 					req->request.stream_id,
 					req->request.short_not_ok,
 					req->request.no_interrupt);
@@ -1145,7 +1151,8 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep *dep,
 		trb = &dep->trb_pool[dep->trb_enqueue];
 		req->num_trbs++;
 		__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, maxp - rem,
-				false, 1, req->request.stream_id,
+				false, req->request.is_last, 1,
+				req->request.stream_id,
 				req->request.short_not_ok,
 				req->request.no_interrupt);
 	} else if (req->request.zero && req->request.length &&
@@ -1162,7 +1169,8 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep *dep,
 		trb = &dep->trb_pool[dep->trb_enqueue];
 		req->num_trbs++;
 		__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, 0,
-				false, 1, req->request.stream_id,
+				false, req->request.is_last, 1,
+				req->request.stream_id,
 				req->request.short_not_ok,
 				req->request.no_interrupt);
 	} else {
@@ -1223,6 +1231,10 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
 
 		if (!dwc3_calc_trbs_left(dep))
 			return;
+
+		/* Don't prepare ahead. This is not an option for DWC_usb32. */
+		if (req->request.is_last)
+			return;
 	}
 }
 
@@ -1284,6 +1296,9 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
 		return ret;
 	}
 
+	if (req->request.is_last)
+		dep->flags |= DWC3_EP_WAIT_TRANSFER_COMPLETE;
+
 	return 0;
 }
 
@@ -1490,6 +1505,9 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
 	list_add_tail(&req->list, &dep->pending_list);
 	req->status = DWC3_REQUEST_STATUS_QUEUED;
 
+	if (dep->flags & DWC3_EP_WAIT_TRANSFER_COMPLETE)
+		return 0;
+
 	/* Start the transfer only after the END_TRANSFER is completed */
 	if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) {
 		dep->flags |= DWC3_EP_DELAY_START;
@@ -2648,37 +2666,22 @@ static bool dwc3_gadget_ep_should_continue(struct dwc3_ep *dep)
 	return !dwc3_gadget_ep_request_completed(req);
 }
 
-static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep,
-		const struct dwc3_event_depevt *event)
-{
-	dep->frame_number = event->parameters;
-}
-
-static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
-		const struct dwc3_event_depevt *event)
+static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep,
+		const struct dwc3_event_depevt *event, int status)
 {
 	struct dwc3		*dwc = dep->dwc;
-	unsigned		status = 0;
-	bool			stop = false;
-
-	dwc3_gadget_endpoint_frame_from_event(dep, event);
-
-	if (event->status & DEPEVT_STATUS_BUSERR)
-		status = -ECONNRESET;
-
-	if (event->status & DEPEVT_STATUS_MISSED_ISOC) {
-		status = -EXDEV;
-
-		if (list_empty(&dep->started_list))
-			stop = true;
-	}
+	bool			no_started_trb = true;
 
 	dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
 
-	if (stop)
+	if (dep->flags & DWC3_EP_END_TRANSFER_PENDING)
+		return no_started_trb;
+
+	if (status == -EXDEV && list_empty(&dep->started_list))
 		dwc3_stop_active_transfer(dep, true, true);
 	else if (dwc3_gadget_ep_should_continue(dep))
-		__dwc3_gadget_kick_transfer(dep);
+		if (__dwc3_gadget_kick_transfer(dep) == 0)
+			no_started_trb = false;
 
 	/*
 	 * WORKAROUND: This is the 2nd half of U1/U2 -> U0 workaround.
@@ -2695,7 +2698,7 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
 				continue;
 
 			if (!list_empty(&dep->started_list))
-				return;
+				return no_started_trb;
 		}
 
 		reg = dwc3_readl(dwc->regs, DWC3_DCTL);
@@ -2704,6 +2707,45 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
 
 		dwc->u1u2 = 0;
 	}
+
+	return no_started_trb;
+}
+
+static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep,
+		const struct dwc3_event_depevt *event)
+{
+	dep->frame_number = event->parameters;
+}
+
+static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
+		const struct dwc3_event_depevt *event)
+{
+	int status = 0;
+
+	if (usb_endpoint_xfer_isoc(dep->endpoint.desc))
+		dwc3_gadget_endpoint_frame_from_event(dep, event);
+
+	if (event->status & DEPEVT_STATUS_BUSERR)
+		status = -ECONNRESET;
+
+	if (event->status & DEPEVT_STATUS_MISSED_ISOC)
+		status = -EXDEV;
+
+	dwc3_gadget_endpoint_trbs_complete(dep, event, status);
+}
+
+static void dwc3_gadget_endpoint_transfer_complete(struct dwc3_ep *dep,
+		const struct dwc3_event_depevt *event)
+{
+	int status = 0;
+
+	dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
+
+	if (event->status & DEPEVT_STATUS_BUSERR)
+		status = -ECONNRESET;
+
+	if (dwc3_gadget_endpoint_trbs_complete(dep, event, status))
+		dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE;
 }
 
 static void dwc3_gadget_endpoint_transfer_not_ready(struct dwc3_ep *dep,
@@ -2770,7 +2812,10 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
 		}
 		break;
 	case DWC3_DEPEVT_STREAMEVT:
+		break;
 	case DWC3_DEPEVT_XFERCOMPLETE:
+		dwc3_gadget_endpoint_transfer_complete(dep, event);
+		break;
 	case DWC3_DEPEVT_RXTXFIFOEVT:
 		break;
 	}
-- 
2.11.0

Re: [PATCH 4/5] usb: dwc3: gadget: Handle transfer completion

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 4687 bytes --]


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:

> With the new usb_request->is_last field, now the function drivers can
> inform which request is the end of a transfer, dwc3 can program its TRBs
> to let the controller know when to free its resources when a transfer
> completes. This is required for stream transfers. The controller needs
> to know where one stream starts and ends to properly allocate resources
> for different streams.
>
> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>

from a quick read, it looks like this can be broken down into smaller
patches.

> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 7204a838ec06..b11183a715a7 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -701,6 +701,7 @@ struct dwc3_ep {
>  #define DWC3_EP_END_TRANSFER_PENDING BIT(4)
>  #define DWC3_EP_PENDING_REQUEST	BIT(5)
>  #define DWC3_EP_DELAY_START	BIT(6)
> +#define DWC3_EP_WAIT_TRANSFER_COMPLETE	BIT(7)

this could be its own patch with proper description (and usage)

>  
>  	/* This last one is specific to EP0 */
>  #define DWC3_EP0_DIR_IN		BIT(31)
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 865e6fbb7360..628f9d142876 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -579,6 +579,7 @@ static int dwc3_gadget_set_ep_config(struct dwc3_ep *dep, unsigned int action)
>  
>  	if (usb_ss_max_streams(comp_desc) && usb_endpoint_xfer_bulk(desc)) {
>  		params.param1 |= DWC3_DEPCFG_STREAM_CAPABLE
> +			| DWC3_DEPCFG_XFER_COMPLETE_EN

adding this bit for stream endpoints could be a separate commit.

>  			| DWC3_DEPCFG_STREAM_EVENT_EN;
>  		dep->stream_capable = true;
>  	}
> @@ -917,8 +918,9 @@ static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep)
>  }
>  
>  static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
> -		dma_addr_t dma, unsigned length, unsigned chain, unsigned node,
> -		unsigned stream_id, unsigned short_not_ok, unsigned no_interrupt)
> +		dma_addr_t dma, unsigned length, unsigned chain,
> +		unsigned is_last, unsigned node, unsigned stream_id,
> +		unsigned short_not_ok, unsigned no_interrupt)

if you add "is_last" as the last argument, this hunk will look
smaller. Nitpicking, I know.

> @@ -1223,6 +1231,10 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
>  
>  		if (!dwc3_calc_trbs_left(dep))
>  			return;
> +
> +		/* Don't prepare ahead. This is not an option for DWC_usb32. */
> +		if (req->request.is_last)
> +			return;

this requires some better description. Why isn't it an option for dwc_usb32?

> @@ -2648,37 +2666,22 @@ static bool dwc3_gadget_ep_should_continue(struct dwc3_ep *dep)
>  	return !dwc3_gadget_ep_request_completed(req);
>  }
>  
> -static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep,
> -		const struct dwc3_event_depevt *event)
> -{
> -	dep->frame_number = event->parameters;
> -}

moving these functions around could be a separate patch. The way it's
done now takes away from review.

> @@ -2704,6 +2707,45 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
>  
>  		dwc->u1u2 = 0;
>  	}
> +
> +	return no_started_trb;
> +}
> +
> +static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep,
> +		const struct dwc3_event_depevt *event)
> +{
> +	dep->frame_number = event->parameters;
> +}
> +
> +static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
> +		const struct dwc3_event_depevt *event)
> +{
> +	int status = 0;
> +
> +	if (usb_endpoint_xfer_isoc(dep->endpoint.desc))
> +		dwc3_gadget_endpoint_frame_from_event(dep, event);
> +
> +	if (event->status & DEPEVT_STATUS_BUSERR)
> +		status = -ECONNRESET;
> +
> +	if (event->status & DEPEVT_STATUS_MISSED_ISOC)
> +		status = -EXDEV;
> +
> +	dwc3_gadget_endpoint_trbs_complete(dep, event, status);
> +}
> +
> +static void dwc3_gadget_endpoint_transfer_complete(struct dwc3_ep *dep,
> +		const struct dwc3_event_depevt *event)
> +{
> +	int status = 0;
> +
> +	dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
> +
> +	if (event->status & DEPEVT_STATUS_BUSERR)
> +		status = -ECONNRESET;
> +
> +	if (dwc3_gadget_endpoint_trbs_complete(dep, event, status))
> +		dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE;
>  }
>  
>  static void dwc3_gadget_endpoint_transfer_not_ready(struct dwc3_ep *dep,
> @@ -2770,7 +2812,10 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
>  		}
>  		break;
>  	case DWC3_DEPEVT_STREAMEVT:
> +		break;

Swap these around. Keep all the "nothing to do here" cases
together.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 4/5] usb: dwc3: gadget: Handle transfer completion

[Thinh Nguyen]

Hi,

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>
>> With the new usb_request->is_last field, now the function drivers can
>> inform which request is the end of a transfer, dwc3 can program its TRBs
>> to let the controller know when to free its resources when a transfer
>> completes. This is required for stream transfers. The controller needs
>> to know where one stream starts and ends to properly allocate resources
>> for different streams.
>>
>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> from a quick read, it looks like this can be broken down into smaller
> patches.

Ok.

>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 7204a838ec06..b11183a715a7 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -701,6 +701,7 @@ struct dwc3_ep {
>>   #define DWC3_EP_END_TRANSFER_PENDING BIT(4)
>>   #define DWC3_EP_PENDING_REQUEST	BIT(5)
>>   #define DWC3_EP_DELAY_START	BIT(6)
>> +#define DWC3_EP_WAIT_TRANSFER_COMPLETE	BIT(7)
> this could be its own patch with proper description (and usage)

Will do.

>>   
>>   	/* This last one is specific to EP0 */
>>   #define DWC3_EP0_DIR_IN		BIT(31)
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 865e6fbb7360..628f9d142876 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -579,6 +579,7 @@ static int dwc3_gadget_set_ep_config(struct dwc3_ep *dep, unsigned int action)
>>   
>>   	if (usb_ss_max_streams(comp_desc) && usb_endpoint_xfer_bulk(desc)) {
>>   		params.param1 |= DWC3_DEPCFG_STREAM_CAPABLE
>> +			| DWC3_DEPCFG_XFER_COMPLETE_EN
> adding this bit for stream endpoints could be a separate commit.

Will do.

>
>>   			| DWC3_DEPCFG_STREAM_EVENT_EN;
>>   		dep->stream_capable = true;
>>   	}
>> @@ -917,8 +918,9 @@ static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep)
>>   }
>>   
>>   static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
>> -		dma_addr_t dma, unsigned length, unsigned chain, unsigned node,
>> -		unsigned stream_id, unsigned short_not_ok, unsigned no_interrupt)
>> +		dma_addr_t dma, unsigned length, unsigned chain,
>> +		unsigned is_last, unsigned node, unsigned stream_id,
>> +		unsigned short_not_ok, unsigned no_interrupt)
> if you add "is_last" as the last argument, this hunk will look
> smaller. Nitpicking, I know.

No problem :)

>
>> @@ -1223,6 +1231,10 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
>>   
>>   		if (!dwc3_calc_trbs_left(dep))
>>   			return;
>> +
>> +		/* Don't prepare ahead. This is not an option for DWC_usb32. */
>> +		if (req->request.is_last)
>> +			return;
> this requires some better description. Why isn't it an option for dwc_usb32?

Internally, DWC_usb32 does some advance caching and burst that we should 
not prepare more TRB until the transfer is completed.
This doesn't apply for isoc, missed a check here. Need to apply on the 
next version.

>
>> @@ -2648,37 +2666,22 @@ static bool dwc3_gadget_ep_should_continue(struct dwc3_ep *dep)
>>   	return !dwc3_gadget_ep_request_completed(req);
>>   }
>>   
>> -static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep,
>> -		const struct dwc3_event_depevt *event)
>> -{
>> -	dep->frame_number = event->parameters;
>> -}
> moving these functions around could be a separate patch. The way it's
> done now takes away from review.

You're right, it's a bit difficult to review as is.

>
>> @@ -2704,6 +2707,45 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
>>   
>>   		dwc->u1u2 = 0;
>>   	}
>> +
>> +	return no_started_trb;
>> +}
>> +
>> +static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep,
>> +		const struct dwc3_event_depevt *event)
>> +{
>> +	dep->frame_number = event->parameters;
>> +}
>> +
>> +static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
>> +		const struct dwc3_event_depevt *event)
>> +{
>> +	int status = 0;
>> +
>> +	if (usb_endpoint_xfer_isoc(dep->endpoint.desc))
>> +		dwc3_gadget_endpoint_frame_from_event(dep, event);
>> +
>> +	if (event->status & DEPEVT_STATUS_BUSERR)
>> +		status = -ECONNRESET;
>> +
>> +	if (event->status & DEPEVT_STATUS_MISSED_ISOC)
>> +		status = -EXDEV;
>> +
>> +	dwc3_gadget_endpoint_trbs_complete(dep, event, status);
>> +}
>> +
>> +static void dwc3_gadget_endpoint_transfer_complete(struct dwc3_ep *dep,
>> +		const struct dwc3_event_depevt *event)
>> +{
>> +	int status = 0;
>> +
>> +	dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
>> +
>> +	if (event->status & DEPEVT_STATUS_BUSERR)
>> +		status = -ECONNRESET;
>> +
>> +	if (dwc3_gadget_endpoint_trbs_complete(dep, event, status))
>> +		dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE;
>>   }
>>   
>>   static void dwc3_gadget_endpoint_transfer_not_ready(struct dwc3_ep *dep,
>> @@ -2770,7 +2812,10 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
>>   		}
>>   		break;
>>   	case DWC3_DEPEVT_STREAMEVT:
>> +		break;
> Swap these around. Keep all the "nothing to do here" cases
> together.
>

Thanks,
Thinh

Re: [PATCH 4/5] usb: dwc3: gadget: Handle transfer completion

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 725 bytes --]


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> @@ -1223,6 +1231,10 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
>>>   
>>>   		if (!dwc3_calc_trbs_left(dep))
>>>   			return;
>>> +
>>> +		/* Don't prepare ahead. This is not an option for DWC_usb32. */
>>> +		if (req->request.is_last)
>>> +			return;
>> this requires some better description. Why isn't it an option for dwc_usb32?
>
> Internally, DWC_usb32 does some advance caching and burst that we should 
> not prepare more TRB until the transfer is completed.
> This doesn't apply for isoc, missed a check here. Need to apply on the 
> next version.

Do you mind re-wording this statement as a comment in the code?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 4/5] usb: dwc3: gadget: Handle transfer completion

[Thinh Nguyen]

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>> @@ -1223,6 +1231,10 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
>>>>    
>>>>    		if (!dwc3_calc_trbs_left(dep))
>>>>    			return;
>>>> +
>>>> +		/* Don't prepare ahead. This is not an option for DWC_usb32. */
>>>> +		if (req->request.is_last)
>>>> +			return;
>>> this requires some better description. Why isn't it an option for dwc_usb32?
>> Internally, DWC_usb32 does some advance caching and burst that we should
>> not prepare more TRB until the transfer is completed.
>> This doesn't apply for isoc, missed a check here. Need to apply on the
>> next version.
> Do you mind re-wording this statement as a comment in the code?

Yes, I reworded it and added the comment in the code and commit message 
in the v2. You already have it on your testing/next branch.

Thanks,
Thinh

[PATCH 5/5] usb: dwc3: gadget: Handle stream transfers

[Thinh Nguyen]

The dwc3 driver wasn't implemented to handle streams.

This implementation handles the followings:
 * Handles device-initiated streams (e.g. UASP driver)
 * Handles some hosts' quirky behavior where they only prime each
   endpoint once
 * Does not use any timer for NoStream rejection

To use stream, the dwc3 driver must know which request is the last
request of a transfer. The controller uses this information to
reallocate its resources for different streams as transfers are
completed. Function drivers that support stream must indicate this via
usb_request->is_last field.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 drivers/usb/dwc3/core.h   |  11 +++++
 drivers/usb/dwc3/debug.h  |   2 +
 drivers/usb/dwc3/gadget.c | 102 ++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 112 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index b11183a715a7..013f42a2b5dc 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -495,6 +495,7 @@
 #define DWC3_DGCMD_SELECTED_FIFO_FLUSH	0x09
 #define DWC3_DGCMD_ALL_FIFO_FLUSH	0x0a
 #define DWC3_DGCMD_SET_ENDPOINT_NRDY	0x0c
+#define DWC3_DGCMD_SET_ENDPOINT_PRIME	0x0d
 #define DWC3_DGCMD_RUN_SOC_BUS_LOOPBACK	0x10
 
 #define DWC3_DGCMD_STATUS(n)		(((n) >> 12) & 0x0F)
@@ -702,6 +703,9 @@ struct dwc3_ep {
 #define DWC3_EP_PENDING_REQUEST	BIT(5)
 #define DWC3_EP_DELAY_START	BIT(6)
 #define DWC3_EP_WAIT_TRANSFER_COMPLETE	BIT(7)
+#define DWC3_EP_IGNORE_NEXT_NOSTREAM	BIT(8)
+#define DWC3_EP_FORCE_RESTART_STREAM	BIT(9)
+#define DWC3_EP_FIRST_STREAM_PRIMED	BIT(10)
 
 	/* This last one is specific to EP0 */
 #define DWC3_EP0_DIR_IN		BIT(31)
@@ -1157,6 +1161,9 @@ struct dwc3 {
 #define DWC31_REVISION_180A	0x3138302a
 #define DWC31_REVISION_190A	0x3139302a
 
+#define DWC32_REVISION_ANY	0x0
+#define DWC32_REVISION_100A	0x3130302a
+
 	u32			version_type;
 
 #define DWC31_VERSIONTYPE_ANY		0x0
@@ -1301,6 +1308,10 @@ struct dwc3_event_depevt {
 #define DEPEVT_STREAMEVT_FOUND		1
 #define DEPEVT_STREAMEVT_NOTFOUND	2
 
+/* Stream event parameter */
+#define DEPEVT_STREAM_PRIME		0xfffe
+#define DEPEVT_STREAM_NOSTREAM		0x0
+
 /* Control-only Status */
 #define DEPEVT_STATUS_CONTROL_DATA	1
 #define DEPEVT_STATUS_CONTROL_STATUS	2
diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
index 0f95656c9622..d8f600e0e88f 100644
--- a/drivers/usb/dwc3/debug.h
+++ b/drivers/usb/dwc3/debug.h
@@ -68,6 +68,8 @@ dwc3_gadget_generic_cmd_string(u8 cmd)
 		return "All FIFO Flush";
 	case DWC3_DGCMD_SET_ENDPOINT_NRDY:
 		return "Set Endpoint NRDY";
+	case DWC3_DGCMD_SET_ENDPOINT_PRIME:
+		return "Set Endpoint Prime";
 	case DWC3_DGCMD_RUN_SOC_BUS_LOOPBACK:
 		return "Run SoC Bus Loopback Test";
 	default:
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 628f9d142876..5127fbb0521c 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -610,6 +610,9 @@ static int dwc3_gadget_set_ep_config(struct dwc3_ep *dep, unsigned int action)
 	return dwc3_send_gadget_ep_cmd(dep, DWC3_DEPCMD_SETEPCONFIG, &params);
 }
 
+static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
+		bool interrupt);
+
 /**
  * __dwc3_gadget_ep_enable - initializes a hw endpoint
  * @dep: endpoint to be initialized
@@ -670,7 +673,7 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, unsigned int action)
 	 * Issue StartTransfer here with no-op TRB so we can always rely on No
 	 * Response Update Transfer command.
 	 */
-	if ((usb_endpoint_xfer_bulk(desc) && !dep->stream_capable) ||
+	if (usb_endpoint_xfer_bulk(desc) ||
 			usb_endpoint_xfer_int(desc)) {
 		struct dwc3_gadget_ep_cmd_params params;
 		struct dwc3_trb	*trb;
@@ -689,6 +692,29 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, unsigned int action)
 		ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
 		if (ret < 0)
 			return ret;
+
+		if (dep->stream_capable) {
+			/*
+			 * For streams, at start, there maybe a race where the
+			 * host primes the endpoint before the function driver
+			 * queues a request to initiate a stream. In that case,
+			 * the controller will not see the prime to generate the
+			 * ERDY and start stream. To workaround this, issue a
+			 * no-op TRB as normal, but end it immediately. As a
+			 * result, when the function driver queues the request,
+			 * the next START_TRANSFER command will cause the
+			 * controller to generate an ERDY to initiate the
+			 * stream.
+			 */
+			dwc3_stop_active_transfer(dep, true, true);
+
+			/*
+			 * All stream eps will reinitiate stream on NoStream
+			 * rejection until we can determine that the host can
+			 * prime after the first transfer.
+			 */
+			dep->flags |= DWC3_EP_FORCE_RESTART_STREAM;
+		}
 	}
 
 out:
@@ -697,8 +723,6 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, unsigned int action)
 	return 0;
 }
 
-static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
-		bool interrupt);
 static void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep)
 {
 	struct dwc3_request		*req;
@@ -2767,6 +2791,69 @@ static void dwc3_gadget_endpoint_transfer_not_ready(struct dwc3_ep *dep,
 	(void) __dwc3_gadget_start_isoc(dep);
 }
 
+static void dwc3_gadget_endpoint_stream_event(struct dwc3_ep *dep,
+		const struct dwc3_event_depevt *event)
+{
+	struct dwc3 *dwc = dep->dwc;
+
+	if (event->status == DEPEVT_STREAMEVT_FOUND) {
+		dep->flags |= DWC3_EP_FIRST_STREAM_PRIMED;
+		goto out;
+	}
+
+	/* Note: NoStream rejection event param value is 0 and not 0xFFFF */
+	switch (event->parameters) {
+	case DEPEVT_STREAM_PRIME:
+		/*
+		 * If the host can properly transition the endpoint state from
+		 * idle to prime after a NoStream rejection, there's no need to
+		 * force restarting the endpoint to reinitiate the stream. To
+		 * simplify the check, assume the host follows the USB spec if
+		 * it primed the endpoint more than once.
+		 */
+		if (dep->flags & DWC3_EP_FORCE_RESTART_STREAM) {
+			if (dep->flags & DWC3_EP_FIRST_STREAM_PRIMED)
+				dep->flags &= ~DWC3_EP_FORCE_RESTART_STREAM;
+			else
+				dep->flags |= DWC3_EP_FIRST_STREAM_PRIMED;
+		}
+
+		break;
+	case DEPEVT_STREAM_NOSTREAM:
+		if ((dep->flags & DWC3_EP_IGNORE_NEXT_NOSTREAM) ||
+		    !(dep->flags & DWC3_EP_FORCE_RESTART_STREAM))
+			break;
+
+		/*
+		 * If the host rejects a stream due to no active stream, by the
+		 * USB and xHCI spec, the endpoint will be put back to idle
+		 * state. When the host is ready (buffer added/updated), it will
+		 * prime the endpoint to inform the usb device controller. This
+		 * triggers the device controller to issue ERDY to restart the
+		 * stream. However, some hosts don't follow this and keep the
+		 * endpoint in the idle state. No prime will come despite host
+		 * streams are updated, and the device controller will not be
+		 * triggered to generate ERDY to move the next stream data. To
+		 * workaround this and maintain compatibility with various
+		 * hosts, force to reinitate the stream until the host is ready
+		 * instead of waiting for the host to prime the endpoint.
+		 */
+		if (DWC3_VER_IS_WITHIN(DWC32, 100A, ANY)) {
+			unsigned int cmd = DWC3_DGCMD_SET_ENDPOINT_PRIME;
+
+			dwc3_send_gadget_generic_command(dwc, cmd, dep->number);
+		} else {
+			dep->flags |= DWC3_EP_DELAY_START;
+			dwc3_stop_active_transfer(dep, true, true);
+			return;
+		}
+		break;
+	}
+
+out:
+	dep->flags &= ~DWC3_EP_IGNORE_NEXT_NOSTREAM;
+}
+
 static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
 		const struct dwc3_event_depevt *event)
 {
@@ -2812,6 +2899,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
 		}
 		break;
 	case DWC3_DEPEVT_STREAMEVT:
+		dwc3_gadget_endpoint_stream_event(dep, event);
 		break;
 	case DWC3_DEPEVT_XFERCOMPLETE:
 		dwc3_gadget_endpoint_transfer_complete(dep, event);
@@ -2907,6 +2995,14 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 	WARN_ON_ONCE(ret);
 	dep->resource_index = 0;
 
+	/*
+	 * The END_TRANSFER command will cause the controller to generate a
+	 * NoStream Event, and it's not due to the host DP NoStream rejection.
+	 * Ignore the next NoStream event.
+	 */
+	if (dep->stream_capable)
+		dep->flags |= DWC3_EP_IGNORE_NEXT_NOSTREAM;
+
 	if (!interrupt)
 		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
 	else
-- 
2.11.0