Hi,
Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> writes:
>> Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> writes:
>>> In DWC_usb31 version 1.70a-ea06 and prior, for highspeed and fullspeed
>>> isochronous IN, BIT[15:14] of the 16-bit microframe number reported by
>>> the XferNotReady event are invalid. The driver uses this number to
>>> schedule the isochronous transfer and passes it to the START TRANSFER
>>> command. Because this number is invalid, the command may fail. If
>>> BIT[15:14] matches the internal 16-bit microframe, the START TRANSFER
>>> command will pass and the transfer will start at the scheduled time, if
>>> it is off by 1, the command will still pass, but the transfer will start
>>> 2 seconds in the future. All other conditions the START TRANSFER command
>>> will fail with bus-expiry.
>>>
>>> In order to workaround this issue, we can issue multiple START TRANSFER
>>> commands with different values of BIT[15:14]: 'b00, 'b01, 'b10, and 'b11
>>> and do an END TRANSFER command. Each combination is 2 seconds apart. 4
>>> seconds into the future will cause a bus-expiry failure. As the result,
>>> within the 4 possible combinations for BIT[15:14], there will be 2
>>> successful and 2 failure START COMMAND status. One of the 2 successful
>>> command status will result in a 2-second delay. The smaller BIT[15:14]
>>> value is the correct one.
>>>
>>> Since there are only 4 outcomes and the results are ordered, we can
>>> simply test 2 START TRANSFER commands with BIT[15:14] combinations 'b00
>>> and 'b01 to deduce the smallest successful combination.
>>>
>>> +---------+---------+
>>> | BIT(15) | BIT(14) |
>>> +=========+=========+
>>> test0 | 0 | 0 |
>>> +---------+---------+
>>> test1 | 0 | 1 |
>>> +---------+---------+
>>>
>>> With test0 and test1 BIT[15:14] combinations, here is the logic:
>>> if test0 passes and test1 passes, BIT[15:14] is 'b00
>>> if test0 passes and test1 fails, BIT[15:14] is 'b11
>>> if test0 fails and test1 fails, BIT[15:14] is 'b10
>>> if test0 fails and test1 passes, BIT[15:14] is 'b01
>>>
>>> Synopsys STAR 9001202023: Wrong microframe number for isochronous IN
>>> endpoints.
>>>
>>> Signed-off-by: Thinh Nguyen <thinhn@xxxxxxxxxxxx>
>>> ---
>>> drivers/usb/dwc3/core.c | 2 +
>>> drivers/usb/dwc3/core.h | 13 ++++
>>> drivers/usb/dwc3/gadget.c | 189 ++++++++++++++++++++++++++++++++++++++++++++--
>>> 3 files changed, 199 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index ddcfa2a60d4b..d27ddfcc3b8a 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -1073,6 +1073,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>> "snps,is-utmi-l1-suspend");
>>> device_property_read_u8(dev, "snps,hird-threshold",
>>> &hird_threshold);
>>> + dwc->dis_start_transfer_quirk = device_property_read_bool(dev,
>>> + "snps,dis-start-transfer-quirk");
>>> dwc->usb3_lpm_capable = device_property_read_bool(dev,
>>> "snps,usb3_lpm_capable");
>>> device_property_read_u8(dev, "snps,rx-thr-num-pkt-prd",
>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>> index 71cf53a06e49..c09cd0c6354e 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -555,6 +555,10 @@ struct dwc3_event_buffer {
>>> * @name: a human readable name e.g. ep1out-bulk
>>> * @direction: true for TX, false for RX
>>> * @stream_capable: true when streams are enabled
>>> + * @frame_number_15_14: BIT[15:14] of the frame number to test isochronous
>>> + * START TRANSFER command failure workaround
>>> + * @test0_status: the result of testing START TRANSFER command with
>>> + * frame_number_15_14 = 'b00 (non-zero means failure)
>>> */
>>> struct dwc3_ep {
>>> struct usb_ep endpoint;
>>> @@ -608,6 +612,10 @@ struct dwc3_ep {
>>>
>>> unsigned direction:1;
>>> unsigned stream_capable:1;
>>> +
>>> + /* Isochronous START TRANSFER workaround for STAR 9001202023 */
>>> + u8 frame_number_15_14;
>>> + int test0_status;
>>> };
>>>
>>> enum dwc3_phy {
>>> @@ -862,6 +870,8 @@ struct dwc3_scratchpad_array {
>>> * @pullups_connected: true when Run/Stop bit is set
>>> * @setup_packet_pending: true when there's a Setup Packet in FIFO. Workaround
>>> * @three_stage_setup: set if we perform a three phase setup
>>> + * @dis_start_transfer_quirk: set if start_transfer failure SW workaround is
>>> + * not needed for DWC_usb31 version 1.70a-ea06 and below
>>> * @usb3_lpm_capable: set if hadrware supports Link Power Management
>>> * @disable_scramble_quirk: set if we enable the disable scramble quirk
>>> * @u2exit_lfps_quirk: set if we enable u2exit lfps quirk
>>> @@ -982,10 +992,12 @@ struct dwc3 {
>>> #define DWC3_REVISION_IS_DWC31 0x80000000
>>> #define DWC3_USB31_REVISION_110A (0x3131302a | DWC3_REVISION_IS_DWC31)
>>> #define DWC3_USB31_REVISION_120A (0x3132302a | DWC3_REVISION_IS_DWC31)
>>> +#define DWC3_USB31_REVISION_170A (0x3137302a | DWC3_REVISION_IS_DWC31)
>>>
>>> u32 ver_type;
>>>
>>> #define DWC31_VERSIONTYPE_EA01 0x65613031
>>> +#define DWC31_VERSIONTYPE_EA06 0x65613036
>>>
>>> enum dwc3_ep0_next ep0_next_event;
>>> enum dwc3_ep0_state ep0state;
>>> @@ -1029,6 +1041,7 @@ struct dwc3 {
>>> unsigned pullups_connected:1;
>>> unsigned setup_packet_pending:1;
>>> unsigned three_stage_setup:1;
>>> + unsigned dis_start_transfer_quirk:1;
>>> unsigned usb3_lpm_capable:1;
>>>
>>> unsigned disable_scramble_quirk:1;
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 24cbd79481e8..f873ebb40ea8 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -1241,6 +1241,161 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
>>> return 0;
>>> }
>>>
>>> +static int __dwc3_cleanup_done_trbs(struct dwc3 *dwc, struct dwc3_ep *dep,
>>> + struct dwc3_request *req,
>>> + struct dwc3_trb *trb,
>>> + const struct dwc3_event_depevt *event,
>>> + int status, int chain);
>>> +
>>> +/**
>>> + * set_isoc_start_frame - set valid wrap-around isoc start frame bits
>>
>> needs a dwc3_ prefix
>>
>>> + * @dwc: pointer to our context structure
>>
>> you don't need to pass dwc here. A reference to it is inside struct
>> dwc3_ep. Also, you never use dwc inside the function apart from an
>> unnecessary dev_info()
>>
>>> + * @dep: isoc endpoint
>>> + * @cur_uf: The current microframe taken from XferNotReady event
>>> + *
>>> + * This function tests for the correct combination of BIT[15:14] from
>>> + * the 16-bit microframe number reported by the XferNotReady event and
>>> + * set it to dep->frame_number.
>>> + *
>>> + * In DWC_usb31 version 1.70a-ea06 and prior, for highspeed and fullspeed
>>> + * isochronous IN, BIT[15:14] of the 16-bit microframe number reported by
>>> + * the XferNotReady event are invalid. The driver uses this number to
>>> + * schedule the isochronous transfer and passes it to the START TRANSFER
>>> + * command. Because this number is invalid, the command may fail. If
>>> + * BIT[15:14] matches the internal 16-bit microframe, the START TRANSFER
>>> + * command will pass and the transfer will start at the scheduled time, if
>>> + * it is off by 1, the command will still pass, but the transfer will start
>>> + * 2 seconds in the future. All other conditions the START TRANSFER command
>>> + * will fail with bus-expiry.
>>> + *
>>> + * In order to workaround this issue, we can issue multiple START TRANSFER
>>> + * commands with different values of BIT[15:14]: 'b00, 'b01, 'b10, and 'b11
>>> + * and do an END TRANSFER command. Each combination is 2 seconds apart. 4
>>> + * seconds into the future will cause a bus-expiry failure. As the result,
>>> + * within the 4 possible combinations for BIT[15:14], there will be 2
>>> + * successful and 2 failure START COMMAND status. One of the 2 successful
>>> + * command status will result in a 2-second delay. The smaller BIT[15:14]
>>> + * value is the correct one.
>>> + *
>>> + * Since there are only 4 outcomes and the results are ordered, we can
>>> + * simply test 2 START TRANSFER commands with BIT[15:14] combinations 'b00
>>> + * and 'b01 to deduce the smallest successful combination.
>>> + *
>>> + * +---------+---------+
>>> + * | BIT(15) | BIT(14) |
>>> + * +=========+=========+
>>> + * test0 | 0 | 0 |
>>> + * +---------+---------+
>>> + * test1 | 0 | 1 |
>>> + * +---------+---------+
>>> + *
>>> + * With test0 and test1 BIT[15:14] combinations, here is the logic:
>>> + * if test0 passes and test1 passes, BIT[15:14] is 'b00
>>> + * if test0 passes and test1 fails, BIT[15:14] is 'b11
>>> + * if test0 fails and test1 fails, BIT[15:14] is 'b10
>>> + * if test0 fails and test1 passes, BIT[15:14] is 'b01
>>> + *
>>> + * Synopsys STAR 9001202023: Wrong microframe number for isochronous IN
>>> + * endpoints.
>>> + *
>>> + * Return 0 if the dep->frame_number is set, return 1 if END TRANSFER
>>> + * command is executed, and return negative errno.
>>> + */
>>> +static int set_isoc_start_frame(struct dwc3 *dwc, struct dwc3_ep *dep,
>>> + u32 cur_uf)
>>> +{
>>> + struct dwc3_request *req;
>>> + int ret = 0;
>>> + int test0, test1;
>>
>> one declaration per line. Also, tabify these
>>
>>> +
>>> + /* Store the 14-bit frame number on the first test */
>>> + if (!dep->frame_number_15_14)
>>> + dep->frame_number = cur_uf & ~(BIT(14) | BIT(15));
>>> +
>>> + while (dep->frame_number_15_14 < 2) {
>>
>> why 2? Why a magic constant?
>>
>>> + u32 cmd;
>>> + u32 frame_number;
>>> + struct dwc3_gadget_ep_cmd_params params;
>>> + struct dwc3_event_depevt event;
>>
>> reverse xmas tree!!
>>
>>> + event.endpoint_number = dep->number;
>>> + event.endpoint_event = DWC3_DEPEVT_EPCMDCMPLT;
>>
>> wait, you're faking an event?!? Sorry, but no. Refactor
>> __dwc3_cleanup_done_trbs().
>>
>>> + req = next_request(&dep->pending_list);
>>> + if (!req) {
>>> + dev_info(dwc->dev, "%s: ran out of requests\n",
>>> + dep->name);
>>
>> unnecessary dev_info()
>>
>>> + dep->flags |= DWC3_EP_PENDING_REQUEST;
>>> + return -EINVAL;
>>> + }
>>> +
>>> + frame_number = dep->frame_number;
>>> + frame_number |= dep->frame_number_15_14 << 14;
>>> + frame_number += max_t(u32, 16, dep->interval);
>>> +
>>> + dwc3_prepare_one_trb(dep, req, false, 0);
>>> +
>>> + params.param0 = upper_32_bits(req->trb_dma);
>>> + params.param1 = lower_32_bits(req->trb_dma);
>>> +
>>> + cmd = DWC3_DEPCMD_STARTTRANSFER;
>>> + cmd |= DWC3_DEPCMD_PARAM(frame_number);
>>> + ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms);
>>> +
>>> + if (dep->frame_number_15_14 == 0)
>>> + dep->test0_status = ret;
>>> +
>>> + if (!ret) {
>>> + dep->resource_index =
>>> + dwc3_gadget_ep_get_transfer_index(dep);
>>> + dwc3_stop_active_transfer(dep->dwc, dep->number, true);
>>> + }
>>> +
>>> + __dwc3_cleanup_done_trbs(dep->dwc, dep, req, req->trb,
>>> + &event, 0, false);
>>> + req->trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
>>> + dwc3_gadget_giveback(dep, req, 0);
>>> +
>>> + dep->frame_number_15_14++;
>>> +
>>> + /*
>>> + * Return early and wait for the next XferNotReady event to come
>>> + * before sending the next START TRANSFER command.
>>> + */
>>> + if (!ret)
>>> + return 1;
>>
>> why 1?
>>
>>> +
>>> + /* End test if not bus-expiry status */
>>> + if (ret != -EAGAIN) {
>>> + dep->test0_status = -EINVAL;
>>> + dep->frame_number_15_14 = 0;
>>> + return ret;
>>
>> this function is quite a mess. It has several possible return values. It
>> seems like "1" means "break out and stop", 0 means "go ahead and kick
>> transfers" and -errno means "also break out and stop". What gives? Why
>> the messed up returns like that?
>>
>>> + }
>>> + }
>>> +
>>> + /* test0 and test1 are both completed at this point */
>>> + test0 = (dep->test0_status == 0);
>>> + test1 = (ret == 0);
>>> +
>>> + if (!test0 && test1)
>>> + dep->frame_number_15_14 = 1;
>>> + else if (!test0 && !test1)
>>> + dep->frame_number_15_14 = 2;
>>> + else if (test0 && !test1)
>>> + dep->frame_number_15_14 = 3;
>>> + else if (test0 && test1)
>>> + dep->frame_number_15_14 = 0;
>>> +
>>> + dep->frame_number |= dep->frame_number_15_14 << 14;
>>> + dep->frame_number += max_t(u32, 16, dep->interval);
>>> +
>>> + /* Reinitialize test variables */
>>> + dep->test0_status = -EINVAL;
>>> + dep->frame_number_15_14 = 0;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>>> {
>>> u32 reg;
>>> @@ -1252,6 +1407,8 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>>> static void __dwc3_gadget_start_isoc(struct dwc3 *dwc,
>>> struct dwc3_ep *dep, u32 cur_uf)
>>> {
>>> + bool need_isoc_start_transfer_workaround = false;
>>
>> doesn't seem like you need this at all...
>>
>>> +
>>> if (list_empty(&dep->pending_list)) {
>>> dev_info(dwc->dev, "%s: ran out of requests\n",
>>> dep->name);
>>> @@ -1259,11 +1416,31 @@ static void __dwc3_gadget_start_isoc(struct dwc3 *dwc,
>>> return;
>>> }
>>>
>>> - /*
>>> - * Schedule the first trb for one interval in the future or at
>>> - * least 4 microframes.
>>> - */
>>> - dep->frame_number = cur_uf + max_t(u32, 4, dep->interval);
>>> + if (dwc3_is_usb31(dwc) && dwc->revision <= DWC3_USB31_REVISION_170A) {
>>> + need_isoc_start_transfer_workaround = true;
>>> +
>>> + /* for 1.70a only ea01 to ea06 are affected */
>>> + if (dwc->revision == DWC3_USB31_REVISION_170A &&
>>> + !(dwc->ver_type >= DWC31_VERSIONTYPE_EA01 &&
>>> + dwc->ver_type <= DWC31_VERSIONTYPE_EA06))
>>> + need_isoc_start_transfer_workaround = false;
>>> + }
>>> +
>>> + if (!dwc->dis_start_transfer_quirk &&
>>> + need_isoc_start_transfer_workaround &&
>>> + dep->direction &&
>>> + (dwc->gadget.speed == USB_SPEED_HIGH ||
>>> + dwc->gadget.speed == USB_SPEED_FULL)) {
>>
>> ... because all these tests could be combined in a simple place and
>> moved inside dwc3_set_isoc_start_frame() so that you could
>> unconditionally call it here.
>>
>> I'll go ahead and drop this series from this merge window, it clearly
>> needs much more work.
>>
>
> Thank you for reviewing the patches. I'll make the change to this patch
> for the next merge window. However, can you cherry-pick the other
> patches in this series for this merge window? If they also need more
> work, please let me know.
I don't think it makes sense to pick the others without this, specially
since you're touching usb core on patch 2 and I can't apply that. I did,
however, take Mass Storage patch setting its max speed to SSP.
--
balbi
Attachment:
signature.asc
Description: PGP signature
