RE: [PATCH v2] usb: host: Implement workaround for Erratum A-009611

[Yinbo Zhu]

-----Original Message-----
From: Greg Kroah-Hartman [mailto:gregkh@xxxxxxxxxxxxxxxxxxx] 
Sent: Friday, December 08, 2017 6:21 PM
To: Yinbo Zhu <yinbo.zhu@xxxxxxx>
Cc: Felipe Balbi <balbi@xxxxxx>; Mathias Nyman <mathias.nyman@xxxxxxxxx>; open list:DESIGNWARE USB3 DRD IP DRIVER <linux-usb@xxxxxxxxxxxxxxx>; open list:DESIGNWARE USB3 DRD IP DRIVER <linux-omap@xxxxxxxxxxxxxxx>; open list <linux-kernel@xxxxxxxxxxxxxxx>; Xiaobo Xie <xiaobo.xie@xxxxxxx>; Jerry Huang <jerry.huang@xxxxxxx>; Ran Wang <ran.wang_1@xxxxxxx>
Subject: Re: [PATCH v2] usb: host: Implement workaround for Erratum A-009611

On Fri, Dec 08, 2017 at 05:49:41PM +0800, yinbo.zhu@xxxxxxx wrote:
> From: "yinbo.zhu" <yinbo.zhu@xxxxxxx>
> 
> Description: This is a occasional problem where the software

>No need for a "Description:" word.  That's just assumed here, right?

> issues an End Transfer command while a USB transfer is in progress, 
> resulting in the TxFIFO  being flushed when the lower layer is waiting 
> for data,causing the super speed (SS) transmit to get blocked.
> If the End Transfer command is issued on an IN endpoint to flush out 
> the pending transfers when the same IN endpoint is doing transfers on 
> the USB, then depending upon the timing of the End Transfer (and the 
> resulting internal FIFO flush),the lower layer (U3PTL/U3MAC) could get 
> stuck waiting for data indefinitely. This blocks the transmission path 
> on the SS, and no DP/ACK/ERDY/DEVNOTIF packets can be sent from the 
> device.
> Impact: If this issue happens and the transmission gets blocked, then 
> the USB host aborts and resets/re-enumerates the device.
> This unblocks the transmitt engine and the device functions normally.
> 
> Workaround: Software must wait for all existing TRBs to complete 
> before issuing End transfer command.
> 
> Configs Affected:
> LS1088-48A-R1.0, LS2081A-R1.1, LS2088-48A-R1.0, LS2088-48A-R1.1, 
> LX2160-2120-2080A-R1.

What are these Configs?  That doesn't seem to match up with anything that is in the kernel tree that I can see.

> 
> Signed-off-by: yinbo.zhu <yinbo.zhu@xxxxxxx>
> ---
>  drivers/usb/dwc3/core.c      |  3 +++
>  drivers/usb/dwc3/core.h      |  3 +++
>  drivers/usb/dwc3/host.c      |  3 +++
>  drivers/usb/host/xhci-plat.c |  4 ++++
>  drivers/usb/host/xhci.c      | 24 ++++++++++++++++++------
>  drivers/usb/host/xhci.h      |  1 +
>  6 files changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 
> 5cb3f6795b0b..071e7cea8cbb 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1106,6 +1106,9 @@ static void dwc3_get_properties(struct dwc3 
> *dwc)
>  
>  	dwc->quirk_reverse_in_out = device_property_read_bool(dev,
>  				"snps,quirk_reverse_in_out");
> +	dwc->quirk_stop_transfer_in_block = device_property_read_bool(dev,
> +				"snps,quirk_stop_transfer_in_block");

>Have you documented this new DT value somewhere?
I had add some description in drivers/usb/dwc3/core.h.
Is it okay?
"  + * @quirk_stop_transfer_in_block: prevent block transmission from being
   + *                                interrupted."
> +
>  	dwc->needs_fifo_resize = of_property_read_bool(node, 
> "tx-fifo-resize");
>  
>  	dwc->configure_gfladj =
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 
> 6c530cbedf49..b2425799ecb6 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -900,6 +900,8 @@ struct dwc3_scratchpad_array {
>   * 	3	- Reserved
>   * @disable_devinit_u1u2_quirk: disable device-initiated U1/U2 request.
>   * @quirk_reverse_in_out: prevent tx fifo reverse the data direction.
> + * @quirk_stop_transfer_in_block: prevent block transmission from being
> + *                                interrupted.
>   * @imod_interval: set the interrupt moderation interval in 250ns
>   *                 increments or 0 to disable.
>   */
> @@ -1063,6 +1065,7 @@ struct dwc3 {
>  	unsigned		tx_de_emphasis:2;
>  	unsigned		disable_devinit_u1u2_quirk:1;
>  	unsigned                quirk_reverse_in_out:1;
> +	unsigned                quirk_stop_transfer_in_block:1;
>  
>  	u16			imod_interval;
>  };
> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c index 
> 2cd48633d3fa..a9ccbf1b9871 100644
> --- a/drivers/usb/dwc3/host.c
> +++ b/drivers/usb/dwc3/host.c
> @@ -110,6 +110,9 @@ int dwc3_host_init(struct dwc3 *dwc)
>  	if (dwc->quirk_reverse_in_out)
>  		props[prop_idx++].name = "quirk-reverse-in-out";
>  
> +	if (dwc->quirk_stop_transfer_in_block)
> +		props[prop_idx++].name = "quirk-stop-transfer-in-block";
> +
>  	if (dwc->usb3_lpm_capable)
>  		props[prop_idx++].name = "usb3-lpm-capable";
>  
> diff --git a/drivers/usb/host/xhci-plat.c 
> b/drivers/usb/host/xhci-plat.c index d1c1e882e6d7..5721d4ece625 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -272,6 +272,10 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	if (device_property_read_bool(&pdev->dev, "quirk-reverse-in-out"))
>  		xhci->quirks |= XHCI_REVERSE_IN_OUT;
>  
> +	if (device_property_read_bool(&pdev->dev,
> +				"quirk-stop-transfer-in-block"))
> +		xhci->quirks |= XHCI_STOP_TRANSFER_IN_BLOCK;
> +
>  	if (device_property_read_bool(&pdev->dev, "quirk-broken-port-ped"))
>  		xhci->quirks |= XHCI_BROKEN_PORT_PED;
>  
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 
> 21dd1d98508f..925c8d171c0b 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1515,13 +1515,25 @@ static int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
>  			ret = -ENOMEM;
>  			goto done;
>  		}
> -		ep->ep_state |= EP_STOP_CMD_PENDING;
> -		ep->stop_cmd_timer.expires = jiffies +
> +		/*
> +		 *A-009611: Issuing an End Transfer command on an IN endpoint.
> +		 *when a transfer is in progress on USB blocks the transmission
> +		 *Workaround: Software must wait for all existing TRBs to
> +		 *complete before issuing End transfer command.

>What is "A-009611:" mean?

>Also please properly format your comments (look at other ones for examples of how to do it.)
A-009611 is erratum name, about the comments that I will had a change. Thanks.

> +		 */
> +		if ((ep_ring->enqueue == ep_ring->dequeue &&
> +				(xhci->quirks & XHCI_STOP_TRANSFER_IN_BLOCK)) ||
> +				!(xhci->quirks & XHCI_STOP_TRANSFER_IN_BLOCK)) {
> +			ep->ep_state |= EP_STOP_CMD_PENDING;
> +			ep->stop_cmd_timer.expires = jiffies +
>  			XHCI_STOP_EP_CMD_TIMEOUT * HZ;
> -		add_timer(&ep->stop_cmd_timer);
> -		xhci_queue_stop_endpoint(xhci, command, urb->dev->slot_id,
> -					 ep_index, 0);
> -		xhci_ring_cmd_db(xhci);
> +			add_timer(&ep->stop_cmd_timer);
> +			xhci_queue_stop_endpoint(xhci, command,
> +					urb->dev->slot_id,
> +					ep_index, 0);
> +			xhci_ring_cmd_db(xhci);
> +		}
> +
>  	}
>  done:
>  	spin_unlock_irqrestore(&xhci->lock, flags); diff --git 
> a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 
> 78d14ff0b811..bff47d6582a8 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1836,6 +1836,7 @@ struct xhci_hcd {
>  /* Reserved. It was XHCI_U2_DISABLE_WAKE */
>  #define XHCI_ASMEDIA_MODIFY_FLOWCONTROL	(1 << 28)
>  #define XHCI_HW_LPM_DISABLE	(1 << 29)
> +#define XHCI_STOP_TRANSFER_IN_BLOCK    (1 << 30)

>Meta-comment, why are these not using the BIT() macro?

>thanks,

>greg k-h
Okay , I will use the BIT() .

Thanks 
yinbo