[PATCH] usb: host: xhci-plat: Enable XHCI_SPURIOUS_SUCCESS quirk for xhci-plat

[PATCH] usb: host: xhci-plat: Enable XHCI_SPURIOUS_SUCCESS quirk for xhci-plat

[George Cherian]

Synopsis xhci controllers with hci_version > 0.96 gives spurious success
events on short packet completion. During webcam capture the
"ERROR Transfer event TRB DMA ptr not part of current TD" was observed.
The same application works fine with synopsis controllers hci_version 0.96.

Signed-off-by: George Cherian <george.cherian@ti.com>
---
 drivers/usb/host/xhci-plat.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 93ad67e..e63c6d3 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -25,6 +25,16 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
 	 * dev struct in order to setup MSI
 	 */
 	xhci->quirks |= XHCI_BROKEN_MSI;
+
+	/*
+	 * In some xhci controllers which follows xhci 1.0 spec gives a spurious
+	 * success event after a short transfer. This quirk will ignore such
+	 * spurious event. Hit this issue in synopsis xhci controllers with
+	 * hci_version > 0.96
+	 */
+
+	if (xhci->hci_version > 0x96)
+		xhci->quirks |= XHCI_SPURIOUS_SUCCESS;
 }
 
 /* called during probe() after chip reset completes */
-- 
1.8.1.4

Re: [PATCH] usb: host: xhci-plat: Enable XHCI_SPURIOUS_SUCCESS quirk for xhci-plat

[Felipe Balbi]

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

Hi,

On Wed, Jun 26, 2013 at 02:28:57PM +0530, George Cherian wrote:
> Synopsis xhci controllers with hci_version > 0.96 gives spurious success
> events on short packet completion. During webcam capture the
> "ERROR Transfer event TRB DMA ptr not part of current TD" was observed.
> The same application works fine with synopsis controllers hci_version 0.96.
> 
> Signed-off-by: George Cherian <george.cherian@ti.com>
> ---
>  drivers/usb/host/xhci-plat.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 93ad67e..e63c6d3 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -25,6 +25,16 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
>  	 * dev struct in order to setup MSI
>  	 */
>  	xhci->quirks |= XHCI_BROKEN_MSI;
> +
> +	/*
> +	 * In some xhci controllers which follows xhci 1.0 spec gives a spurious
> +	 * success event after a short transfer. This quirk will ignore such
> +	 * spurious event. Hit this issue in synopsis xhci controllers with
> +	 * hci_version > 0.96
> +	 */
> +
> +	if (xhci->hci_version > 0x96)
> +		xhci->quirks |= XHCI_SPURIOUS_SUCCESS;
>  }

doesn't look like the correct way to do this. What if enabling that
quirk on hosts which don't have the quirk cause problems ?

I would suggest adding a platform_data which (in our case) dwc3 will
pass to xhci-plat. Then you can do proper revision detection of the
synopsys controller and set the quirk only on the failing hosts.

BTW, do you have the STARS number for this errata ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] usb: host: xhci-plat: Enable XHCI_SPURIOUS_SUCCESS quirk for xhci-plat

[George Cherian]

On 6/26/2013 2:32 PM, Felipe Balbi wrote:
> Hi,
>
> On Wed, Jun 26, 2013 at 02:28:57PM +0530, George Cherian wrote:
>> Synopsis xhci controllers with hci_version > 0.96 gives spurious success
>> events on short packet completion. During webcam capture the
>> "ERROR Transfer event TRB DMA ptr not part of current TD" was observed.
>> The same application works fine with synopsis controllers hci_version 0.96.
>>
>> Signed-off-by: George Cherian<george.cherian@ti.com>
>> ---
>>   drivers/usb/host/xhci-plat.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>> index 93ad67e..e63c6d3 100644
>> --- a/drivers/usb/host/xhci-plat.c
>> +++ b/drivers/usb/host/xhci-plat.c
>> @@ -25,6 +25,16 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
>>   	 * dev struct in order to setup MSI
>>   	 */
>>   	xhci->quirks |= XHCI_BROKEN_MSI;
>> +
>> +	/*
>> +	 * In some xhci controllers which follows xhci 1.0 spec gives a spurious
>> +	 * success event after a short transfer. This quirk will ignore such
>> +	 * spurious event. Hit this issue in synopsis xhci controllers with
>> +	 * hci_version > 0.96
>> +	 */
>> +
>> +	if (xhci->hci_version > 0x96)
>> +		xhci->quirks |= XHCI_SPURIOUS_SUCCESS;
>>   }
> doesn't look like the correct way to do this. What if enabling that
> quirk on hosts which don't have the quirk cause problems ?
For a controller which does not have this issue will never get a 
spurious success for short packet ( and that too for only ISOCH).

per this commit ad808333d Intel xhci: Ignore spurious successful event.

This spurious successful event behavior isn't technically disallowed by
the xHCI specification, so make the xHCI driver just ignore the spurious
completion event.
> I would suggest adding a platform_data which (in our case) dwc3 will
> pass to xhci-plat. Then you can do proper revision detection of the
> synopsys controller and set the quirk only on the failing hosts.
>
> BTW, do you have the STARS number for this errata ?
No STARS number yet.


Regards
-George

Re: [PATCH] usb: host: xhci-plat: Enable XHCI_SPURIOUS_SUCCESS quirk for xhci-plat

[Felipe Balbi]

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

Hi,

On Wed, Jun 26, 2013 at 05:46:29PM +0530, George Cherian wrote:
> >>@@ -25,6 +25,16 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
> >>  	 * dev struct in order to setup MSI
> >>  	 */
> >>  	xhci->quirks |= XHCI_BROKEN_MSI;
> >>+
> >>+	/*
> >>+	 * In some xhci controllers which follows xhci 1.0 spec gives a spurious
> >>+	 * success event after a short transfer. This quirk will ignore such
> >>+	 * spurious event. Hit this issue in synopsis xhci controllers with
> >>+	 * hci_version > 0.96
> >>+	 */
> >>+
> >>+	if (xhci->hci_version > 0x96)
> >>+		xhci->quirks |= XHCI_SPURIOUS_SUCCESS;
> >>  }
> >doesn't look like the correct way to do this. What if enabling that
> >quirk on hosts which don't have the quirk cause problems ?
> For a controller which does not have this issue will never get a
> spurious success for short packet ( and that too for only ISOCH).
> 
> per this commit ad808333d Intel xhci: Ignore spurious successful event.
> 
> This spurious successful event behavior isn't technically disallowed by
> the xHCI specification, so make the xHCI driver just ignore the spurious
> completion event.

still we don't want to enable quirks for devices which aren't quirky :-)

Now how Sarah correctly enables the quirk flag only for the known "bad"
Panther Point device.

> >I would suggest adding a platform_data which (in our case) dwc3 will
> >pass to xhci-plat. Then you can do proper revision detection of the
> >synopsys controller and set the quirk only on the failing hosts.
> >
> >BTW, do you have the STARS number for this errata ?
> No STARS number yet.

once we have it, let's make sure to follow what we do on the dwc3 and
list it in a comment. You already that you found the bug with a Synopsys
controller anyway, might as well point to the fact that there is a
ticket with synopsys to track this issue.

cheers

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] usb: host: xhci-plat: Enable XHCI_SPURIOUS_SUCCESS quirk for xhci-plat

[Sarah Sharp]

On Wed, Jun 26, 2013 at 02:28:57PM +0530, George Cherian wrote:
> Synopsis xhci controllers with hci_version > 0.96 gives spurious success
> events on short packet completion. During webcam capture the
> "ERROR Transfer event TRB DMA ptr not part of current TD" was observed.
> The same application works fine with synopsis controllers hci_version 0.96.

It's a known issue.  The xHCI 1.0 spec changed how hardware handles
short packets.  The HW will notify SW of the TRB where the short packet,
and it will also give a successful status for the last TRB in a TD (the
one with the IOC flag set).  On the second successful status, that
warning will be triggered in the driver.

Software is now supposed to not assume the TD as done until it gets that
last successful status.  That means we have a slight race condition,
although it should have little practical impact.  This patch papers over
that issue.

I will take this patch, but will add the note that is on my long-term
to-do list to fix this issue.

Sarah Sharp

> 
> Signed-off-by: George Cherian <george.cherian@ti.com>
> ---
>  drivers/usb/host/xhci-plat.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 93ad67e..e63c6d3 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -25,6 +25,16 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
>  	 * dev struct in order to setup MSI
>  	 */
>  	xhci->quirks |= XHCI_BROKEN_MSI;
> +
> +	/*
> +	 * In some xhci controllers which follows xhci 1.0 spec gives a spurious
> +	 * success event after a short transfer. This quirk will ignore such
> +	 * spurious event. Hit this issue in synopsis xhci controllers with
> +	 * hci_version > 0.96
> +	 */
> +
> +	if (xhci->hci_version > 0x96)
> +		xhci->quirks |= XHCI_SPURIOUS_SUCCESS;
>  }
>  
>  /* called during probe() after chip reset completes */
> -- 
> 1.8.1.4
> 

Re: [PATCH] usb: host: xhci-plat: Enable XHCI_SPURIOUS_SUCCESS quirk for xhci-plat

[Sarah Sharp]

On Wed, Jun 26, 2013 at 09:53:34AM -0700, Sarah Sharp wrote:
> On Wed, Jun 26, 2013 at 02:28:57PM +0530, George Cherian wrote:
> > Synopsis xhci controllers with hci_version > 0.96 gives spurious success
> > events on short packet completion. During webcam capture the
> > "ERROR Transfer event TRB DMA ptr not part of current TD" was observed.
> > The same application works fine with synopsis controllers hci_version 0.96.
> 
> It's a known issue.  The xHCI 1.0 spec changed how hardware handles
> short packets.  The HW will notify SW of the TRB where the short packet,
> and it will also give a successful status for the last TRB in a TD (the
> one with the IOC flag set).  On the second successful status, that
> warning will be triggered in the driver.
> 
> Software is now supposed to not assume the TD as done until it gets that
> last successful status.  That means we have a slight race condition,
> although it should have little practical impact.  This patch papers over
> that issue.
> 
> I will take this patch, but will add the note that is on my long-term
> to-do list to fix this issue.

Actually, since it applies to both xhci-plat and xhci-pci hosts, can you
move setting this quirk into xhci_gen_setup?

Sarah Sharp

> > Signed-off-by: George Cherian <george.cherian@ti.com>
> > ---
> >  drivers/usb/host/xhci-plat.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index 93ad67e..e63c6d3 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -25,6 +25,16 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
> >  	 * dev struct in order to setup MSI
> >  	 */
> >  	xhci->quirks |= XHCI_BROKEN_MSI;
> > +
> > +	/*
> > +	 * In some xhci controllers which follows xhci 1.0 spec gives a spurious
> > +	 * success event after a short transfer. This quirk will ignore such
> > +	 * spurious event. Hit this issue in synopsis xhci controllers with
> > +	 * hci_version > 0.96
> > +	 */
> > +
> > +	if (xhci->hci_version > 0x96)
> > +		xhci->quirks |= XHCI_SPURIOUS_SUCCESS;
> >  }
> >  
> >  /* called during probe() after chip reset completes */
> > -- 
> > 1.8.1.4
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html