Hi,
Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> 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
