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

From: Greg Kroah-Hartman
Date: Fri Dec 08 2017 - 05:21:12 EST

Next message: Eduardo Otubo: "[PATCHv2] tools: hv: hv_set_ifconfig.sh double check before setting ip"
Previous message: Christophe Leroy: "[PATCH v2] watchdog: core: make sure the watchdog worker always works"
Next in thread: Felipe Balbi: "Re: [PATCH v2] usb: host: Implement workaround for Erratum A-009611"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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?

> +
> 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.)

> + */
> + 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

Next message: Eduardo Otubo: "[PATCHv2] tools: hv: hv_set_ifconfig.sh double check before setting ip"
Previous message: Christophe Leroy: "[PATCH v2] watchdog: core: make sure the watchdog worker always works"
Next in thread: Felipe Balbi: "Re: [PATCH v2] usb: host: Implement workaround for Erratum A-009611"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]