Skip to contentSkip to conversation

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

PATCH Rob Weber 1 participant 1 message Apr 25, 2019

Subsystems (2)
USB DesignWare USB3 DRD IP driver
lore
Share & tools
lore

Rob Weber

author
#0 lore raw

Hi Everyone,

Quoted 33 lines · Alan Stern
Quoted 25 lines · Claus H. Stovgaard

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.

Quoted 5 lines

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.

Quoted 4 lines

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

--

drivers/usb/dwc3/ep0.c +16 -10
index 2331469f943d..78b75d65b435 100644
@@ -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;