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

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

[Greg Kroah-Hartman]

On Fri, Dec 08, 2017 at 05:49:40PM +0800, yinbo.zhu@nxp.com wrote:
> From: "yinbo.zhu" <yinbo.zhu@nxp.com>

I need a "real name" here, I doubt you sign documents as:
	"yinbo.zhu"
right?  :)

Also, you sent 3 patches, yet no way to know what order to apply them
in.  Please fix that up by sending a patch series, properly numbered.

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Fri, Dec 08, 2017 at 05:49:41PM +0800, yinbo.zhu@nxp.com wrote:
> From: "yinbo.zhu" <yinbo.zhu@nxp.com>
> 
> 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@nxp.com>
> ---
>  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

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2516 bytes --]


Hi,

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> On Fri, Dec 08, 2017 at 05:49:41PM +0800, yinbo.zhu@nxp.com wrote:
>> From: "yinbo.zhu" <yinbo.zhu@nxp.com>
>> 
>> 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@nxp.com>
>> ---
>>  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");

This was generated on vendor tree. This quirk doesn't exist in
dwc3. Also, update your tree and review MAINTAINERS file. It has been
almost 2 years since I left TI :-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Yinbo Zhu]

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

On Fri, Dec 08, 2017 at 05:49:40PM +0800, yinbo.zhu@nxp.com wrote:
> From: "yinbo.zhu" <yinbo.zhu@nxp.com>

>I need a "real name" here, I doubt you sign documents as:
>	"yinbo.zhu"
>right?  :)

>Also, you sent 3 patches, yet no way to know what order to apply them in.  Please fix that up by sending a >patch series, properly numbered.

>thanks,

>greg k-h

Hi Greg Kroah-Hartman,

"Yinbo.zhu" is my email address prefix,and it is automatically generated through the git command.
You can follow the order of patch A-007463 A-009611 A-009668 to apply it.

Thanks
Yinbo.

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

[Yinbo Zhu]

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


>Hi,

>Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> On Fri, Dec 08, 2017 at 05:49:41PM +0800, yinbo.zhu@nxp.com wrote:
>> From: "yinbo.zhu" <yinbo.zhu@nxp.com>
>> 
>> Description: This is a occasional problem where the software
>
> No need for a "Description:" word.  That's just assumed here, right?

I will remove "Description:" thanks.
>> 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.

These configs is soc information, I don't enable it on these platform dts.
Although the erratum issue can't be reproduced.  
>> 
>> Signed-off-by: yinbo.zhu <yinbo.zhu@nxp.com>
>> ---
>>  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");

>This was generated on vendor tree. This quirk doesn't exist in dwc3. Also,
 >update your tree and review MAINTAINERS file. It has been almost 2 years since I left TI :-)

>--
>Balbi

Hi Balbi,

The quirk that I had add it in dwc3. Your meaning is that I can't use quirk to enable or disable the erratum, isn't it? The tree is git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git, I had updated it.

Thanks.
Yinbo

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

[Greg Kroah-Hartman]

On Mon, Dec 11, 2017 at 02:26:02AM +0000, Yinbo Zhu wrote:
> 
> 
> -----Original Message-----
> From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] 
> Sent: Friday, December 08, 2017 6:18 PM
> To: Yinbo Zhu <yinbo.zhu@nxp.com>
> Cc: Felipe Balbi <balbi@ti.com>; Mathias Nyman <mathias.nyman@intel.com>; open list:DESIGNWARE USB3 DRD IP DRIVER <linux-usb@vger.kernel.org>; open list:DESIGNWARE USB3 DRD IP DRIVER <linux-omap@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>; Xiaobo Xie <xiaobo.xie@nxp.com>; Jerry Huang <jerry.huang@nxp.com>; Ran Wang <ran.wang_1@nxp.com>
> Subject: Re: [PATCH v2] usb: host: Implement workaround for Erratum A-007463
> 
> On Fri, Dec 08, 2017 at 05:49:40PM +0800, yinbo.zhu@nxp.com wrote:
> > From: "yinbo.zhu" <yinbo.zhu@nxp.com>
> 
> >I need a "real name" here, I doubt you sign documents as:
> >	"yinbo.zhu"
> >right?  :)
> 
> >Also, you sent 3 patches, yet no way to know what order to apply them in.  Please fix that up by sending a >patch series, properly numbered.
> 
> >thanks,
> 
> >greg k-h
> 
> Hi Greg Kroah-Hartman,
> 
> "Yinbo.zhu" is my email address prefix,and it is automatically generated through the git command.

Then this means you have not correctly configured git, please fix that :)

> You can follow the order of patch A-007463 A-009611 A-009668 to apply it.

That's not how we number patches in the kernel, please do so in the
normal way.  See the mailing lists for lots of examples.  The kernel
documentation also describes the correct format for this.

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Mon, Dec 11, 2017 at 03:15:37AM +0000, Yinbo Zhu wrote:
> 
> 
> -----Original Message-----
> From: Felipe Balbi [mailto:felipe.balbi@linux.intel.com] 
> Sent: Friday, December 08, 2017 6:44 PM
> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Yinbo Zhu <yinbo.zhu@nxp.com>
> Cc: Mathias Nyman <mathias.nyman@intel.com>; open list:DESIGNWARE USB3 DRD IP DRIVER <linux-usb@vger.kernel.org>; open list:DESIGNWARE USB3 DRD IP DRIVER <linux-omap@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>; Xiaobo Xie <xiaobo.xie@nxp.com>; Jerry Huang <jerry.huang@nxp.com>; Ran Wang <ran.wang_1@nxp.com>
> Subject: Re: [PATCH v2] usb: host: Implement workaround for Erratum A-009611
> 
> 
> >Hi,
> 
> >Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> > On Fri, Dec 08, 2017 at 05:49:41PM +0800, yinbo.zhu@nxp.com wrote:
> >> From: "yinbo.zhu" <yinbo.zhu@nxp.com>
> >> 
> >> Description: This is a occasional problem where the software
> >
> > No need for a "Description:" word.  That's just assumed here, right?
> 
> I will remove "Description:" thanks.
> >> 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.
> 
> These configs is soc information, I don't enable it on these platform dts.
> Although the erratum issue can't be reproduced.  

I do not understand what this means, please explain it a bit better.

thanks,

greg k-h

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

[Yinbo Zhu]

-----Original Message-----
From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] 
Sent: Monday, December 11, 2017 3:34 PM
To: Yinbo Zhu <yinbo.zhu@nxp.com>
Cc: Felipe Balbi <balbi@ti.com>; Mathias Nyman <mathias.nyman@intel.com>; open list:DESIGNWARE USB3 DRD IP DRIVER <linux-usb@vger.kernel.org>; open list:DESIGNWARE USB3 DRD IP DRIVER <linux-omap@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>; Xiaobo Xie <xiaobo.xie@nxp.com>; Jerry Huang <jerry.huang@nxp.com>; Ran Wang <ran.wang_1@nxp.com>
Subject: Re: [PATCH v2] usb: host: Implement workaround for Erratum A-007463

On Mon, Dec 11, 2017 at 02:26:02AM +0000, Yinbo Zhu wrote:
> 
> 
> -----Original Message-----
> From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org]
> Sent: Friday, December 08, 2017 6:18 PM
> To: Yinbo Zhu <yinbo.zhu@nxp.com>
> Cc: Felipe Balbi <balbi@ti.com>; Mathias Nyman 
> <mathias.nyman@intel.com>; open list:DESIGNWARE USB3 DRD IP DRIVER 
> <linux-usb@vger.kernel.org>; open list:DESIGNWARE USB3 DRD IP DRIVER 
> <linux-omap@vger.kernel.org>; open list 
> <linux-kernel@vger.kernel.org>; Xiaobo Xie <xiaobo.xie@nxp.com>; Jerry 
> Huang <jerry.huang@nxp.com>; Ran Wang <ran.wang_1@nxp.com>
> Subject: Re: [PATCH v2] usb: host: Implement workaround for Erratum 
> A-007463
> 
> On Fri, Dec 08, 2017 at 05:49:40PM +0800, yinbo.zhu@nxp.com wrote:
> > From: "yinbo.zhu" <yinbo.zhu@nxp.com>
> 
> >I need a "real name" here, I doubt you sign documents as:
> >	"yinbo.zhu"
> >right?  :)
> 
> >Also, you sent 3 patches, yet no way to know what order to apply them in.  Please fix that up by sending a >patch series, properly numbered.
> 
> >thanks,
> 
> >greg k-h
> 
> Hi Greg Kroah-Hartman,
> 
> "Yinbo.zhu" is my email address prefix,and it is automatically generated through the git command.

>Then this means you have not correctly configured git, please fix that :)
I will remove the point in "Yinbo.zhu"
Thanks.
> You can follow the order of patch A-007463 A-009611 A-009668 to apply it.

>That's not how we number patches in the kernel, please do so in the normal way. 
> See the mailing lists for lots of examples.  The kernel documentation also describes the correct format for this.
Okay, I will mark the sign for patch order. About the kernel documentation I will remove the punctuation.
thanks
>thanks,

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

[Yinbo Zhu]

-----Original Message-----
From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] 
Sent: Monday, December 11, 2017 3:35 PM
To: Yinbo Zhu <yinbo.zhu@nxp.com>
Cc: Felipe Balbi <felipe.balbi@linux.intel.com>; Mathias Nyman <mathias.nyman@intel.com>; open list:DESIGNWARE USB3 DRD IP DRIVER <linux-usb@vger.kernel.org>; open list:DESIGNWARE USB3 DRD IP DRIVER <linux-omap@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>; Xiaobo Xie <xiaobo.xie@nxp.com>; Jerry Huang <jerry.huang@nxp.com>; Ran Wang <ran.wang_1@nxp.com>
Subject: Re: [PATCH v2] usb: host: Implement workaround for Erratum A-009611

On Mon, Dec 11, 2017 at 03:15:37AM +0000, Yinbo Zhu wrote:
> 
> 
> -----Original Message-----
> From: Felipe Balbi [mailto:felipe.balbi@linux.intel.com]
> Sent: Friday, December 08, 2017 6:44 PM
> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Yinbo Zhu 
> <yinbo.zhu@nxp.com>
> Cc: Mathias Nyman <mathias.nyman@intel.com>; open list:DESIGNWARE USB3 
> DRD IP DRIVER <linux-usb@vger.kernel.org>; open list:DESIGNWARE USB3 
> DRD IP DRIVER <linux-omap@vger.kernel.org>; open list 
> <linux-kernel@vger.kernel.org>; Xiaobo Xie <xiaobo.xie@nxp.com>; Jerry 
> Huang <jerry.huang@nxp.com>; Ran Wang <ran.wang_1@nxp.com>
> Subject: Re: [PATCH v2] usb: host: Implement workaround for Erratum 
> A-009611
> 
> 
> >Hi,
> 
> >Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> > On Fri, Dec 08, 2017 at 05:49:41PM +0800, yinbo.zhu@nxp.com wrote:
> >> From: "yinbo.zhu" <yinbo.zhu@nxp.com>
> >> 
> >> Description: This is a occasional problem where the software
> >
> > No need for a "Description:" word.  That's just assumed here, right?
> 
> I will remove "Description:" thanks.
> >> 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.
> 
> These configs is soc information, I don't enable it on these platform dts.
> Although the erratum issue can't be reproduced.  

>I do not understand what this means, please explain it a bit better.

>thanks,

>greg k-h

Maybe I have a problem with your words, Your meaning is that you want to ask me why I didn't add an attribute in the device tree to match kernel for every platform, right?

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

[Yinbo Zhu]

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

On Fri, Dec 08, 2017 at 05:49:41PM +0800, yinbo.zhu@nxp.com wrote:
> From: "yinbo.zhu" <yinbo.zhu@nxp.com>
> 
> 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@nxp.com>
> ---
>  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

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1149 bytes --]


Hi,

(please break your lines at 80-characters)

Yinbo Zhu <yinbo.zhu@nxp.com> writes:
>>> 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");
>
>>This was generated on vendor tree. This quirk doesn't exist in dwc3. Also,
>  >update your tree and review MAINTAINERS file. It has been almost 2 years since I left TI :-)
>
>>--
>>Balbi
>
> Hi Balbi,
>
> The quirk that I had add it in dwc3. Your meaning is that I can't use
> quirk to enable or disable the erratum, isn't it? The tree is
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git, I had
> updated it.

-*- mode: grep; default-directory: "~/workspace/linux/" -*-
Grep started at Mon Dec 11 10:50:47

git --no-pager grep --color -nH -e quirk_reverse_in_out

Grep finished with no matches found at Mon Dec 11 10:50:48

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Yinbo Zhu]

-----Original Message-----
From: Felipe Balbi [mailto:felipe.balbi@linux.intel.com] 
Sent: Monday, December 11, 2017 4:52 PM
To: Yinbo Zhu <yinbo.zhu@nxp.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Mathias Nyman <mathias.nyman@intel.com>; open list:DESIGNWARE USB3 DRD IP DRIVER <linux-usb@vger.kernel.org>; open list:DESIGNWARE USB3 DRD IP DRIVER <linux-omap@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>; Xiaobo Xie <xiaobo.xie@nxp.com>; Jerry Huang <jerry.huang@nxp.com>; Ran Wang <ran.wang_1@nxp.com>
Subject: RE: [PATCH v2] usb: host: Implement workaround for Erratum A-009611


>>Hi,

>>(please break your lines at 80-characters)

>>Yinbo Zhu <yinbo.zhu@nxp.com> writes:
I had check it. Every line is less than 80-characters.
>>> 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");
>
>>This was generated on vendor tree. This quirk doesn't exist in dwc3. 
>>Also,
>  >update your tree and review MAINTAINERS file. It has been almost 2 
> years since I left TI :-)
>
>>--
>>Balbi
>
> Hi Balbi,
>
> The quirk that I had add it in dwc3. Your meaning is that I can't use 
> quirk to enable or disable the erratum, isn't it? The tree is 
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git, I had 
> updated it.

>-*- mode: grep; default-directory: "~/workspace/linux/" -*- Grep started at Mon Dec 11 10:50:47

>git --no-pager grep --color -nH -e quirk_reverse_in_out

>Grep finished with no matches found at Mon Dec 11 10:50:48

>--
>balbi
Hi Balbi,

You can't find the quirk that it is normal. There's no one in the previous code.
 The quirk that I added to control the new erratum 
Please you note.

Thanks.
Yinbo.