Hi,

2015-01-15 1:02 GMT+08:00 Felipe Balbi [off-list ref]:

Hi,

On Wed, Jan 14, 2015 at 03:08:43PM +0800, Sneeker Yeh wrote:

quoted

Hi Felipe:

thanks for suggestion,

2015-01-13 1:20 GMT+08:00 Felipe Balbi [off-list ref]:

quoted

Hi,

On Sun, Jan 11, 2015 at 11:19:55PM +0800, Sneeker Yeh wrote:

quoted

quoted

quoted

quoted

enable the quirk only for you. Isn't there a better way of

enabling the

quoted

quoted

quoted

quoted

quirk based off of revision detection couple with a look on

GHWPARAMS*

quoted

quoted

quoted

quoted

registers ?

What's tricking me is this claim that only config-free PHYs

would

quoted

quoted

be

quoted

quoted

quoted

quoted

affected, why ?

i'm still struggling now to try to get more information about

this.

quoted

quoted

quoted

quoted

quoted

some security policy inside Fujitsu make me unable to access full
information of this errata today.

Someday after i get enough information,
i shall take your suggestion here that seems better to incur

quirk

quoted

quoted

quoted

quoted

quoted

dynamically via GHWPARAMS,
and then send it here again.

ok, hopefully you'll find a way ;-)

I got some update information here finally~

in case i express unclearly i also put a pdf:
https://drive.google.com/file/d/0B18MmcvvKjNNbDF6eEdHSzZCazA/view

This issue is defined by a two-way race at disconnect, between
1) class driver interrupt endpoint resheduling attempts if the ISR

gave

quoted

quoted

an

quoted

ep error event due to device detach (it would try 3 times)
2) Disconnect interrupt on PORTSC_CSC, which is cleared by hub thread
asynchronously
3) The hardware IP was configured in silicon with
       - DWC_USB3_SUSPEND_ON_DISCONNECT_EN=1 (this is an IP

configuration

quoted

quoted

yeah, aparently this is another configuration which is not exposed on
HWPARAMS registers. Paul, can you confirm for us ? I couldn't find it

on

quoted

quoted

Databook on any of the HWPARAMS registers.

quoted

port whose state cannot be checked from software)
       - Synopsys IP version is < 3.00a

heh, so pretty much everybody :-)

yeah, and I'm really lucky to encounter this @@

quoted

quoted

       The IP will auto-suspend itself on device detach with some
phy-specific interval after CSC is cleared by 2)

If 2) and 3) complete before 1), the interrupts it expects will not

be

quoted

quoted

quoted

generated by the autosuspended IP, leading to a deadlock. Even later
disconnection procedure would detect that corresponding urb is still
in-progress and issue a ep stop command, auto-suspended IP still

won't

quoted

quoted

quoted

respond to that command.

this defect would result in this when device detached:
-------------------
[   99.603544] usb 4-1: USB disconnect, device number 2
[  104.615254] xhci-hcd xhci-hcd.0.auto: xHCI host not responding to

stop

quoted

quoted

quoted

endpoint command.
[  104.623362] xhci-hcd xhci-hcd.0.auto: Assuming host is dying,

halting

quoted

quoted

quoted

host.
[  104.653261] xhci-hcd xhci-hcd.0.auto: Host not halted after 16000
microseconds.
[  104.660584] xhci-hcd xhci-hcd.0.auto: Non-responsive xHCI host is

not

quoted

quoted

quoted

halting.
[  104.667817] xhci-hcd xhci-hcd.0.auto: Completing active URBs

anyway.

quoted

quoted

quoted

[  104.674198] xhci-hcd xhci-hcd.0.auto: HC died; cleaning up
--------------------
As a result, when device detached, we desired to postpone "PORTCSC

clear"

quoted

quoted

quoted

behind "disable slot". it's found that all executed ep command

related to

quoted

quoted

quoted

disconnetion, are executed before "disable slot".

Now this is all great information and they should all be part of your
commit log and probably a big comment should be added to code as well.

I see, thanks.

quoted

Thanks for going after all these details, now let's figure out a way to
pass dwc3 revision to xhci, or maybe we pass just a flag for the quirk,
something like:

        if (dwc->revision < 3.00a && dwc->has_suspend_on_disconnect)
                xhci_pdata.delay_portcsc_clear = true;

or something similar to that.

Sure, how about i write these like this, that i learn from the way dwc3
inject XHCI_LPM_SUPPORT to xhci via platform data:

--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c

@@ -53,6 +53,10 @@ int dwc3_host_init(struct dwc3 *dwc)
        pdata.usb3_lpm_capable = 1;
 #endif

+       if ((dwc->revision < DWC3_REVISION_300A) &&
+           dwc->suspend_on_disconnect_quirk)
+               pdata.delay_portcsc_clear = 1;
+
        ret = platform_device_add_data(xhci, &pdata, sizeof(pdata));
        if (ret) {
                dev_err(dwc->dev, "couldn't add platform data to xHCI

device\n");

--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c

@@ -147,6 +147,9 @@ static int xhci_plat_probe(struct platform_device

*pdev)

quoted

        if ((node && of_property_read_bool(node, "usb3-lpm-capable")) ||
                        (pdata && pdata->usb3_lpm_capable))
                xhci->quirks |= XHCI_LPM_SUPPORT;
+
+       if (pdata && pdata->delay_portcsc_clear)
+               xhci->quirks |= XHCI_DISCONNECT_QUIRK;
        /*
         * Set the xHCI pointer before xhci_plat_setup() (aka
hcd_driver.reset)
         * is called by usb_add_hcd().

but I'm a little confused about how we decide
dwc->suspend_on_disconnect_quirk.
just wondering if pdata & dts would be better than Kconfig to decide

this?

quoted

because using Kconfig means forcing dwc3.ko has this quirk, but platform
might have another dwc3 core doesn't need that quirk.
So I write the following code, is it in a good shape? :

--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c

@@ -838,6 +838,9 @@ static int dwc3_probe(struct platform_device *pdev)
                                "snps,tx_de_emphasis_quirk");
                of_property_read_u8(node, "snps,tx_de_emphasis",
                                &tx_de_emphasis);
+
+               dwc->suspend_on_disconnect_quirk =

of_property_read_bool(node,
+                               "snps,has_suspend_on_disconnect");
        } else if (pdata) {
                dwc->maximum_speed = pdata->maximum_speed;
                dwc->has_lpm_erratum = pdata->has_lpm_erratum;

@@ -864,6 +867,8 @@ static int dwc3_probe(struct platform_device *pdev)
                dwc->tx_de_emphasis_quirk = pdata->tx_de_emphasis_quirk;
                if (pdata->tx_de_emphasis)
                        tx_de_emphasis = pdata->tx_de_emphasis;
+
+               dwc->suspend_on_disconnect_quirk =

pdata->has_suspend_on_disconnect;
        }

        /* default to superspeed if no maximum_speed passed */

this all looks good, but let's split it in tiny little patches:

1) add macros for new revisions
2) add suspend_on_disconnect_quirk (and the DT counterpart, please don't
        forget to document the new binding)
3) pass the quirk to XHCI through pdata
4) Use the quirk on XHCI

That'll make patches real easy to review. As per the commit log, the
more verbose the better, specially when it comes to the quirk. It needs
to be 100% clear that we cannot detect this in runtime.

Super thanks,
I'm separating it and testing it, and would send v2 patch series later
based on all suggestions here.

Much appreciate,
Sneeker

cheers

--
balbi