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

[Felipe Balbi <balbi@kernel.org>]

Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> 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.
>> 
>
> 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.