Hi Felipe,
On 3/13/2018 1:45 AM, Felipe Balbi wrote:
>
> Hi,
>
> 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.
Thanks,
Thinh
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
