Hi,
Thinh Nguyen <thinh.ngu...@synopsys.com> writes:
>> Felipe Balbi <felipe.ba...@linux.intel.com> writes:
>>> In the case of interval of 1ms, it will start on the next interval.
>>>> frame_number + max(4, interval) will start at least 4 uframes in the
>>>> future.
>>>>
>>>> In any case, what about immediately retry the START_TRANSFER command
>>>> with a new frame_number + (interval*retry) should it fail with
>>>> bus-expiry? You can set the number of retries to maybe 5 times. This
>>>> should remove the need to do time stamping.
>>> That seems like a good idea. Something like below? (on top of $subject)
>>>
>>> modified drivers/usb/dwc3/core.h
>>> @@ -37,6 +37,7 @@
>>> #define DWC3_EP0_SETUP_SIZE 512
>>> #define DWC3_ENDPOINTS_NUM 32
>>> #define DWC3_XHCI_RESOURCES_NUM 2
>>> +#define DWC3_ISOC_MAX_RETRIES 5
>>>
>>> #define DWC3_SCRATCHBUF_SIZE 4096 /* each buffer is assumed to be
>>> 4KiB */
>>> #define DWC3_EVENT_BUFFERS_SIZE 4096
>>> modified drivers/usb/dwc3/gadget.c
>>> @@ -1271,20 +1271,29 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep
>>> *dep)
>>> u64 current_timestamp;
>>> u64 diff_timestamp;
>>> u32 elapsed_frames;
>>> + int retries;
>>> + int delta = 1
>>> + int ret;
>>>
>>> if (list_empty(&dep->pending_list)) {
>>> dep->flags |= DWC3_EP_PENDING_REQUEST;
>>> return -EAGAIN;
>>> }
>>>
>>> - current_timestamp = ktime_get_ns();
>>> - diff_timestamp = current_timestamp - dep->frame_timestamp;
>>> - elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
>>> + for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
>>> + current_timestamp = ktime_get_ns();
>>> + diff_timestamp = current_timestamp - dep->frame_timestamp;
>>> + elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
>>>
>>> - dep->frame_number += elapsed_frames;
>>> - dep->frame_number = DWC3_ALIGN_FRAME(dep);
>>> + dep->frame_number += elapsed_frames + (delta * i);
>> the other possibility is that we can call DWC3_ALIGN_FRAME() n times
>> since that will put the transfer on the following interval. That would
>> look like so:
>>
>> modified drivers/usb/dwc3/core.h
>> @@ -37,6 +37,7 @@
>> #define DWC3_EP0_SETUP_SIZE 512
>> #define DWC3_ENDPOINTS_NUM 32
>> #define DWC3_XHCI_RESOURCES_NUM 2
>> +#define DWC3_ISOC_MAX_RETRIES 5
>>
>> #define DWC3_SCRATCHBUF_SIZE 4096 /* each buffer is assumed to be
>> 4KiB */
>> #define DWC3_EVENT_BUFFERS_SIZE 4096
>> modified drivers/usb/dwc3/gadget.c
>> @@ -27,7 +27,7 @@
>> #include "gadget.h"
>> #include "io.h"
>>
>> -#define DWC3_ALIGN_FRAME(d) (((d)->frame_number + (d)->interval) \
>> +#define DWC3_ALIGN_FRAME(d, n) (((d)->frame_number + ((d)->interval *
>> (n))) \
>> & ~((d)->interval - 1))
>>
>> /**
>> @@ -1271,20 +1271,28 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep
>> *dep)
>> u64 current_timestamp;
>> u64 diff_timestamp;
>> u32 elapsed_frames;
>> + int retries;
>> + int ret;
>>
>> if (list_empty(&dep->pending_list)) {
>> dep->flags |= DWC3_EP_PENDING_REQUEST;
>> return -EAGAIN;
>> }
>>
>> - current_timestamp = ktime_get_ns();
>> - diff_timestamp = current_timestamp - dep->frame_timestamp;
>> - elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
>> + for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
>> + current_timestamp = ktime_get_ns();
>> + diff_timestamp = current_timestamp - dep->frame_timestamp;
>> + elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
>>
>> - dep->frame_number += elapsed_frames;
>> - dep->frame_number = DWC3_ALIGN_FRAME(dep);
>> + dep->frame_number += elapsed_frames;
>> + dep->frame_number = DWC3_ALIGN_FRAME(dep, i);
>>
>> - return __dwc3_gadget_kick_transfer(dep);
>> + ret = __dwc3_gadget_kick_transfer(dep);
>> + if (ret != -EAGAIN)
>> + break;
>> + }
>> +
>> + return ret;
>> }
>>
>> static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request
>> *req)
>>
>>
> I like the second method. But do we need to keep track of the
> frame_timestamp? I don't think it accurately reflects the timestamp of
no we don't. I've since removed it from my local tree.
> XferNotReady frame number.
> As for the number of retries, we should adjust it according to the
> service interval. For example, for larger service interval such as 16,
> then we don't need to try more than once. To calculate the max number of
> retries, we can do this check (where interval is from 1 to 16):
>
> if (interval >= (16 - (MAX_NUM_RETRIES >> 1))
> num_retries = 1 << (16 - interval);
>
> Please check my math..
>
> If it fails for over 5 times in a row, then we should probably wait for
> the next XferNotReady to get the frame_number again. Also 5 is an
> arbitrary number I came up without any testing, we can probably decide
> on a better number. What do you think?
I have this now:
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 131028501752..3390fa46ea30 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -37,6 +37,7 @@
#define DWC3_EP0_SETUP_SIZE 512
#define DWC3_ENDPOINTS_NUM 32
#define DWC3_XHCI_RESOURCES_NUM 2
+#define DWC3_ISOC_MAX_RETRIES 5
#define DWC3_SCRATCHBUF_SIZE 4096 /* each buffer is assumed to be 4KiB */
#define DWC3_EVENT_BUFFERS_SIZE 4096
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index d8c7ad0c22e8..1590516735cb 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -27,7 +27,7 @@
#include "gadget.h"
#include "io.h"
-#define DWC3_ALIGN_FRAME(d) (((d)->frame_number + (d)->interval) \
+#define DWC3_ALIGN_FRAME(d, n) (((d)->frame_number + ((d)->interval * (n))) \
& ~((d)->interval - 1))
/**
@@ -1268,13 +1268,24 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
{
+ int retries;
+ int ret;
+ int i;
+
if (list_empty(&dep->pending_list)) {
dep->flags |= DWC3_EP_PENDING_REQUEST;
return -EAGAIN;
}
- dep->frame_number = DWC3_ALIGN_FRAME(dep);
- return __dwc3_gadget_kick_transfer(dep);
+ for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
+ dep->frame_number = DWC3_ALIGN_FRAME(dep, i);
+
+ ret = __dwc3_gadget_kick_transfer(dep);
+ if (ret != -EAGAIN)
+ break;
+ }
+
+ return ret;
}
static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request
*req)
This means we will increment in full intervals, up to 5 intervals in the
future. If it still fails, then there's not much we can do.
--
balbi
signature.asc
Description: PGP signature
