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.
--
balbi
Attachment:
signature.asc
Description: PGP signature
