Use device tree to disable U1/U2 in gadget devices based on DWC3

[Rob Weber <rob@gnarbox.com>]

Hi Everyone,

On Wed, Apr 24, 2019 at 02:33:48PM -0400, Alan Stern wrote:
> On Wed, 24 Apr 2019, Claus H. Stovgaard wrote:
> 
> > Hi Balbi and other USB developers.
> > 
> > I am developing camera devices based on Xilinx ZynqMP, using the
> > gadget framework and the build-in dwc3 core of the ZynqMP as USB3
> > controller and the build-in Cirrus SERDES as phy.
> > Testing with a number of hosts and Windows 7, has shown sporadic
> > reconnects when leaving U2/U1, caused by failing link training, where
> > the host resets the bus. Sometime it also means it reconnect via
> > USB2.
> > 
> > So to overcome this, I will like to have the option for disabling
> > U1/U2 on the core when working with those hosts.
> > 
> > Currently I have made a hack in ep0.c where I return EINVAL in
> > dwc3_ep0_handle_u1 and dwc3_ep0_handle_u2 together with not setting
> > DWC3_DCTL_ACCEPTU1ENA and DWC3_DCTL_ACCEPTU2ENA in
> > dwc3_ep0_set_config
> > Will though prefer a solution possible to upstream, so was thinking
> > about adding two devicetree bindings.
> > 
> > * snps,u1_disable_as_gadget: When set the core will not enable U1 if
> > requested from host, nor initiate U1.
> > * snps,u2_disable_as_gadget: When set the core will not enable U2 if
> > requested from host, nor initiate U2.
> > 
> > If you think this might be something which can be upstreamed I will
> > prepare the code and send a patch for discussion.
> > On the other hand, if you think that disabling U1/U2 via device tree
> > as suggested should not be a feature no need for me to try making it
> > a feature.
> 
> Speaking as an interested bystander, I would first wonder why your
> hardware fails during link training.  If it is properly designed, that
> should not happen.  The fact that it does happen suggests your devices
> might also experience problems during normal data transport.

I was recently debugging a similar problem where my device would
improperly transition from U2 to U0. This caused the connection to reset
and the device to be re-enumerated as a USB2 device. Our design uses an
Intel CherryTrail z8550 SoC that has an internal dwc3 USB device
controller. I worked with Felipe a couple of weeks ago [1] to come up with
the same workaround you mentioned above. I disable device-initiated U1/U2
in ep0_set_config and also return -EINVAL when handling SetFeature
requests from the USB host. I've attached my patch for kernel 4.9.115
below for reference.

Although we have applied this workaround, we still have not identified a
root cause of the issue. Intel has no known errata related to the symptoms
we are experiencing. We've done quite a bit of analysis on our design and
are pretty confident we've followed all design guidelines for USB.

Cheers,
Rob Weber

[1] https://marc.info/?t=155349935600001&r=1&w=2
---
drivers/usb/dwc3/ep0.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)

--

diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 2331469f943d..78b75d65b435 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -433,10 +433,13 @@ static int dwc3_ep0_handle_feature(struct dwc3 *dwc,
                return -EINVAL;

            reg = dwc3_readl(dwc->regs, DWC3_DCTL);
-            if (set)
-                reg |= DWC3_DCTL_INITU1ENA;
-            else
+            if (set) {
+                /* reg |= DWC3_DCTL_INITU1ENA; */
+                dwc3_trace(trace_dwc3_ep0, "GBX: Skipping U1 Enable");
+                return -EINVAL;
+            } else {
                reg &= ~DWC3_DCTL_INITU1ENA;
+            }
            dwc3_writel(dwc->regs, DWC3_DCTL, reg);
            break;

@@ -448,10 +451,13 @@ static int dwc3_ep0_handle_feature(struct dwc3 *dwc,
                return -EINVAL;

            reg = dwc3_readl(dwc->regs, DWC3_DCTL);
-            if (set)
-                reg |= DWC3_DCTL_INITU2ENA;
-            else
+            if (set) {
+                /* reg |= DWC3_DCTL_INITU2ENA; */
+                dwc3_trace(trace_dwc3_ep0, "GBX: Skipping U2 Enable");
+                return -EINVAL;
+            } else {
                reg &= ~DWC3_DCTL_INITU2ENA;
+            }
            dwc3_writel(dwc->regs, DWC3_DCTL, reg);
            break;

@@ -567,7 +573,7 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
    enum usb_device_state state = dwc->gadget.state;
    u32 cfg;
    int ret;
-    u32 reg;
+    /* u32 reg; */

    cfg = le16_to_cpu(ctrl->wValue);

@@ -594,9 +600,9 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
             * Enable transition to U1/U2 state when
             * nothing is pending from application.
             */
-            reg = dwc3_readl(dwc->regs, DWC3_DCTL);
-            reg |= (DWC3_DCTL_ACCEPTU1ENA | DWC3_DCTL_ACCEPTU2ENA);
-            dwc3_writel(dwc->regs, DWC3_DCTL, reg);
+            /* reg = dwc3_readl(dwc->regs, DWC3_DCTL); */
+            /* reg |= (DWC3_DCTL_ACCEPTU1ENA | DWC3_DCTL_ACCEPTU2ENA); */
+            /* dwc3_writel(dwc->regs, DWC3_DCTL, reg); */
        }
        break;