[v4,13/15] usb: dwc3: Add workaround for isoc start transfer failure

* [v4,13/15] usb: dwc3: Add workaround for isoc start transfer failure
@ 2018-03-13  8:45 Felipe Balbi
  0 siblings, 0 replies; 6+ messages in thread
From: Felipe Balbi @ 2018-03-13  8:45 UTC (permalink / raw)
  To: Thinh Nguyen; +Cc: John Youn

Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> 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@synopsys.com>
> ---
>  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, &params);
> +
> +		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.

^ permalink raw reply	[flat|nested] 6+ messages in thread