Re: [PATCH 3/3] usb: dwc3: add a quirk for device disconnection issue in Synopsis dwc3 core

[Felipe Balbi <balbi@ti.com>]

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

Hi,

On Wed, Jan 14, 2015 at 03:08:43PM +0800, Sneeker Yeh wrote:
> Hi Felipe:
> 
> thanks for suggestion,
> 
> 2015-01-13 1:20 GMT+08:00 Felipe Balbi <balbi@ti.com>:
> 
> > Hi,
> >
> > On Sun, Jan 11, 2015 at 11:19:55PM +0800, Sneeker Yeh wrote:
> > > > > > enable the quirk only for you. Isn't there a better way of
> > enabling the
> > > > > > quirk based off of revision detection couple with a look on
> > GHWPARAMS*
> > > > > > registers ?
> > > > > >
> > > > > > What's tricking me is this claim that only config-free PHYs would
> > be
> > > > > > 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
> > > 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.
> >
> > > 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 @@
> 
> 
> >
> > >        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 */

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.

cheers

-- 
balbi

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