Hi Felipe:

thanks for suggestion,

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

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

be

quoted

quoted

quoted

quoted

affected, why ?

i'm still struggling now to try to get more information about this.
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
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

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

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
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

       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
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
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
endpoint command.
[  104.623362] xhci-hcd xhci-hcd.0.auto: Assuming host is dying, halting
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
halting.
[  104.667817] xhci-hcd xhci-hcd.0.auto: Completing active URBs anyway.
[  104.674198] xhci-hcd xhci-hcd.0.auto: HC died; cleaning up
--------------------
As a result, when device detached, we desired to postpone "PORTCSC clear"
behind "disable slot". it's found that all executed ep command related to
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.

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)
        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?
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 */


Much appreciate,
Sneeker

cheers

--
balbi