Hi, Thinh Nguyen <[email protected]> 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 <[email protected]> > --- > 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.
--
balbi
signature.asc
Description: PGP signature
