Hi Felipe,
On Tue, Apr 02, 2019 at 08:46:16AM +0300, Felipe Balbi wrote:
> Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> writes:
> >>> modphy is the USB PHY integrated in your SoC. There's no control for
> >>> that from OS side, only BIOS unfortunately. There is, however, one thing
> >>> we can try. DWC3 has several quirk flags for known quirky PHYs; perhaps
> >>> CHT needs one of those. Can you try with this patch and let me know
> >>> whether it helps?
> >>
> >> Sure thing, I will try tomorrow. Could you possibly explain what a quirk
> >> is as it relates to the kernel? I see this all over the source tree but
> >> never knew how it was used. Does the dwc3 also know about "quirks" and
> >> these particular flags? or are these flags just specific to the kernel
> >> and its functionality?
> >>
> >>> modified drivers/usb/dwc3/dwc3-pci.c
> >>> @@ -105,6 +105,8 @@ static int dwc3_byt_enable_ulpi_refclock(struct pci_dev *pci)
> >>> static const struct property_entry dwc3_pci_intel_properties[] = {
> >>> PROPERTY_ENTRY_STRING("dr_mode", "peripheral"),
> >>> PROPERTY_ENTRY_BOOL("linux,sysdev_is_parent"),
> >>> + PROPERTY_ENTRY_BOOL("snps,dis_u3_susphy_quirk"),
> >>> + PROPERTY_ENTRY_BOOL("snps,dis_u2_susphy_quirk"),
> >>> {}
> >>> };
> >>>
> >>> These two quirks will PHY suspend. There are other relevant quirk flags
> > ^^^^^^^^
> > will DISABLE phy suspend :-)
> >
> >> What do you mean by PHY suspend? Will it disable U2/U3 for the dwc3? I
> >
> > No, no. DWC3 can still enter U1/U2/U3, but the PHY will not enter the
> > matching P1/P2/P3 state.
> >
> >> see it modified the DWC3_GUSB2PHYCFG_SUSPHY bit in the configuration
> >> register, but I don't have access to the dwc3 databook to dig deeper
> >> into this.
> >
> > The second quirk flag (snps,dis_u2_susphy_quirk) will tell dwc3 to *NOT*
> > request USB2 PHY to enter low power state. While the first flag
> > (snps,dis_u3_susphy_quirk) will tell dwc3 to *NOT* request USB3 PHY to
> > enter low power state.
> >
> > In reality, they are a single block of HW but they _can_ be different
> > and even if they are a single block, they _can_ have separate clock
> > domains and we _may_ be able to control them separately. I haven't read
> > documentation for modphy because the OS doesn't touch that. If
> > necessary, I can try to find out more details about it, but I will
> > probably take some time to find the correct documentation.
> >
> >>> which we can try in case these two don't help. I'd like to figure out
> >>> exactly which quirk flag helps (if any). After that, we would need to
> >>> check if a similar problem happens on any CHT system or just your
> >>> design.
> >>>
> >>> If it happens on any other system, then I can make sure we add a quirk
> >>> flag to all CHTs.
> >>
> >> Sounds good!
> >>
> >> Thanks for taking the time to answer my questions! It's definitely
> >> helpful for my understanding of USB. I'm learning quite a bit of
> >> new information with each email and it's pretty awesome.
> >
> > No problems at all, happy to help.
>
> Any updates here? Hopefully the quirk flags above helped.
Thanks for following up. My team and I were doing a bunch of testing
today as well as this past weekend. We are still seeing mixed results
with the suggestion you provided to us earlier. I unfortunately do not
have any logs, traces, or analyzer captures to back this up yet, but I
hope to gather more concrete data tomorrow.
There were two main issues we observed with the quirk flags above:
* Host mode no longer worked with these quirks applied. Please keep in
mind that we backported part of a patchset [1] to support switching
the internal SS mux from the xHCI controller to the xDCI controller.
At a quick glance, do you see anything in this patchset that you think
might cause a problem with the role switch driver communicating with
the xHCI controller when the quirks are applied?
* We still saw the drop from SS to HS on a Windows laptop. Linux and Mac
seemed to be fine, but there's definitely a chance we did not test
with enough samples to confidently say this quirk is stable with those
hosts.
So tomorrow I hope to gather more concrete data to back up these
findings. Our USB protocol analyzer is also out-of-order at the moment,
so we might not be able to get analyzer data for a day or two either.
Are there some other quirks you would like us to try out?
Over the weekend, one of our team members was hacking in the dwc3 driver
to try and disable LPM at various points in the chain of events. I've
attached a patch that we believe is pretty stable. We have never seen it
drop from SS to HS. USB type A to type C connections from any host to
our device seem pretty stable. I've attached an archive called
"gnarbox-success-a-to-c.tar.xz" That contains the trace events for a
successful run where we connect the device to a Windows host and it
enumerates at SuperSpeed.
This patch isn't entirely stable as I haven't been able to get a
reliable USB C-to-C connection on any host yet. I do not have any logs
or traces for this particular case and I hope to get them to you soon.
Do you remember the "failed to enable ep0out" error I briefly
mentioned earlier? I was able to capture some trace events while this
occurs. This issue occaisonally occurs when modprobing g_mass_storage.
The exact steps are as follows:
1. switch from host mode to device mode (echo device >
/sys/class/usb_role/.../role)
2. modprobe g_mass_storage ...
3. Try and rest device mode with a host computer, but you'll observe
that device mode is not working.
4. modprobe -r g_mass_storage to remove the gadget driver.
5. switch back to host mode (echo host > /sys/class/usb_role/.../role)
6. switch back to device mode
7. modprobe g_mass_storage ...
8. Observe the error
I've attached the traces and dmesg output in an archive titled
"gnarbox-ep0out-failure.tar.xz". Please let me know if you need more
information about this particular issue.
Lastly, I started digging through the errata available for our SoC
series [2] and there are a few that stand out to me, including CHT40,
CHT47, CHT48, and CHT50. CHT47 is the most concerning because I recall
seeing an "Unknown" LFPS status coming from the device as it is trying
to leave U2. I asked the support specialists on our IPS ticket about
these errata and I'm hoping to hear back from them within the day. Do
any of these errata stand out to you as possible culprits?
Thanks for taking a look at all of this again!
Cheers,
Rob Weber
[1] https://lkml.org/lkml/2018/3/20/387
[2] https://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/atom-z8000-spec-update.pdf
Attachment:
gnarbox-success-a-to-c.tar.xz
Description: application/xz
Attachment:
gnarbox-ep0out-failure.tar.xz
Description: application/xz
>From de8b9e63df1e24d1b9402ae13224edbdae80cd5c Mon Sep 17 00:00:00 2001
From: Mike Lemmon <lemmon@xxxxxxxxxxx>
Date: Sun, 31 Mar 2019 13:18:12 -0700
Subject: [PATCH] dwc3: Disable U1 and U2 link states
---
drivers/usb/dwc3/core.c | 10 ++++++++++
drivers/usb/dwc3/debug.h | 28 ++++++++++++++++++++++++++
drivers/usb/dwc3/ep0.c | 10 ++++++++++
drivers/usb/dwc3/gadget.c | 51 ++++++++++++++++++++++++++++++++++++++++++++---
4 files changed, 96 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 53b26e978d90..b869ff53ee86 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -641,6 +641,16 @@ static int dwc3_core_init(struct dwc3 *dwc)
goto err0;
}
+ printk(KERN_INFO "GBX: (dwc3) dwc->revision=%x patch=0038-disable-U1-U2\n", dwc->revision);
+
+ /*
+ // Tried overriding to old revision but it just hung on transition
+ // to mass-storage mode...
+ printk(KERN_INFO "GBX: (dwc3) Overriding dwc3 revision from %x to %x",
+ dwc->revision, DWC3_REVISION_173A);
+ dwc->revision = DWC3_REVISION_173A;
+ */
+
/*
* Write Linux Version Code to our GUID register so it's easy to figure
* out which kernel version a bug was found.
diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
index 33ab2a203c1b..2385c2222123 100644
--- a/drivers/usb/dwc3/debug.h
+++ b/drivers/usb/dwc3/debug.h
@@ -124,6 +124,34 @@ dwc3_gadget_link_string(enum dwc3_link_state link_state)
}
}
+static inline const char *
+dwc3_dsts_speed_string(u8 speed)
+{
+ static char str[256];
+ switch (speed) {
+ case DWC3_DSTS_SUPERSPEED_PLUS: sprintf(str, "Super-Speed-Plus"); break;
+ case DWC3_DSTS_SUPERSPEED: sprintf(str, "Super-Speed"); break;
+ case DWC3_DSTS_HIGHSPEED: sprintf(str, "High-Speed"); break;
+ case DWC3_DSTS_FULLSPEED: sprintf(str, "Full-Speed"); break;
+ case DWC3_DSTS_LOWSPEED: sprintf(str, "Low-Speed"); break;
+ default: sprintf(str, "UNKNOWN");
+ }
+ return str;
+}
+
+static inline const char *
+dwc3_dr_mode_string(enum usb_dr_mode mode)
+{
+ static char str[256];
+ switch (mode) {
+ case USB_DR_MODE_PERIPHERAL: sprintf(str, "Device-Mode"); break;
+ case USB_DR_MODE_HOST: sprintf(str, "Host-Mode"); break;
+ case USB_DR_MODE_OTG: sprintf(str, "OTG-Mode"); break;
+ default: sprintf(str, "Unsupported Mode (%d)", mode);
+ }
+ return str;
+}
+
/**
* dwc3_gadget_event_string - returns event name
* @event: the event code
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 2331469f943d..8c9bdd20ead3 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -420,12 +420,14 @@ static int dwc3_ep0_handle_feature(struct dwc3 *dwc,
switch (wValue) {
case USB_DEVICE_REMOTE_WAKEUP:
+ dwc3_trace(trace_dwc3_gadget, "GBX: USB_RECIP_DEVICE: USB_DEVICE_REMOTE_WAKEUP");
break;
/*
* 9.4.1 says only only for SS, in AddressState only for
* default control pipe
*/
case USB_DEVICE_U1_ENABLE:
+ dwc3_trace(trace_dwc3_gadget, "GBX: USB_RECIP_DEVICE: USB_DEVICE_U1_ENABLE");
if (state != USB_STATE_CONFIGURED)
return -EINVAL;
if ((dwc->speed != DWC3_DSTS_SUPERSPEED) &&
@@ -441,6 +443,7 @@ static int dwc3_ep0_handle_feature(struct dwc3 *dwc,
break;
case USB_DEVICE_U2_ENABLE:
+ dwc3_trace(trace_dwc3_gadget, "GBX: USB_RECIP_DEVICE: USB_DEVICE_U2_ENABLE");
if (state != USB_STATE_CONFIGURED)
return -EINVAL;
if ((dwc->speed != DWC3_DSTS_SUPERSPEED) &&
@@ -456,9 +459,11 @@ static int dwc3_ep0_handle_feature(struct dwc3 *dwc,
break;
case USB_DEVICE_LTM_ENABLE:
+ dwc3_trace(trace_dwc3_gadget, "GBX: USB_RECIP_DEVICE: USB_DEVICE_LTM_ENABLE");
return -EINVAL;
case USB_DEVICE_TEST_MODE:
+ dwc3_trace(trace_dwc3_gadget, "GBX: USB_RECIP_DEVICE: USB_DEVICE_TEST_MODE");
if ((wIndex & 0xff) != 0)
return -EINVAL;
if (!set)
@@ -478,6 +483,7 @@ static int dwc3_ep0_handle_feature(struct dwc3 *dwc,
}
break;
default:
+ dwc3_trace(trace_dwc3_gadget, "GBX: USB_RECIP_DEVICE: UNKNOWN FEATURE");
return -EINVAL;
}
break;
@@ -485,6 +491,7 @@ static int dwc3_ep0_handle_feature(struct dwc3 *dwc,
case USB_RECIP_INTERFACE:
switch (wValue) {
case USB_INTRF_FUNC_SUSPEND:
+ dwc3_trace(trace_dwc3_gadget, "GBX: USB_RECIP_INTERFACE: USB_INTRF_FUNC_SUSPEND");
if (wIndex & USB_INTRF_FUNC_SUSPEND_LP)
/* XXX enable Low power suspend */
;
@@ -493,6 +500,7 @@ static int dwc3_ep0_handle_feature(struct dwc3 *dwc,
;
break;
default:
+ dwc3_trace(trace_dwc3_gadget, "GBX: USB_RECIP_INTERFACE: UNKNOWN");
return -EINVAL;
}
break;
@@ -500,6 +508,7 @@ static int dwc3_ep0_handle_feature(struct dwc3 *dwc,
case USB_RECIP_ENDPOINT:
switch (wValue) {
case USB_ENDPOINT_HALT:
+ dwc3_trace(trace_dwc3_gadget, "GBX: USB_RECIP_ENDPOINT: USB_ENDPOINT_HALT");
dep = dwc3_wIndex_to_dep(dwc, ctrl->wIndex);
if (!dep)
return -EINVAL;
@@ -510,6 +519,7 @@ static int dwc3_ep0_handle_feature(struct dwc3 *dwc,
return -EINVAL;
break;
default:
+ dwc3_trace(trace_dwc3_gadget, "GBX: USB_RECIP_ENDPOINT: UNKNOWN");
return -EINVAL;
}
break;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 26efe8c7535f..baf8967ad015 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -115,6 +115,10 @@ int dwc3_gadget_set_link_state(struct dwc3 *dwc, enum dwc3_link_state state)
}
reg = dwc3_readl(dwc->regs, DWC3_DCTL);
+ dwc3_trace(trace_dwc3_gadget, "GBX: link state change requested: [%s] => [%s]",
+ dwc3_gadget_link_string(DWC3_DSTS_USBLNKST(reg)),
+ dwc3_gadget_link_string(state)
+ );
reg &= ~DWC3_DCTL_ULSTCHNGREQ_MASK;
/* set requested state */
@@ -176,6 +180,8 @@ void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req,
struct dwc3 *dwc = dep->dwc;
unsigned int unmap_after_complete = false;
+ dwc3_trace(trace_dwc3_gadget, "GBX: gadget_giveback: status=%d --> %d", req->request.status, status);
+
req->started = false;
list_del(&req->list);
req->trb = NULL;
@@ -1636,6 +1642,21 @@ static int __dwc3_gadget_start(struct dwc3 *dwc)
reg = dwc3_readl(dwc->regs, DWC3_DCFG);
reg &= ~(DWC3_DCFG_SPEED_MASK);
+ /* // Tried disabling U1 and U2 here but it didn't work :(
+ {
+ u32 u1u2;
+ u32 reg2;
+ dwc3_trace(trace_dwc3_gadget, "GBX: Disabling U1 and U2");
+ reg2 = dwc3_readl(dwc->regs, DWC3_DCTL);
+ u1u2 = reg2 & (DWC3_DCTL_INITU2ENA
+ | DWC3_DCTL_ACCEPTU2ENA
+ | DWC3_DCTL_INITU1ENA
+ | DWC3_DCTL_ACCEPTU1ENA);
+ reg2 &= ~u1u2;
+ dwc3_writel(dwc->regs, DWC3_DCTL, reg2);
+ }
+ */
+
/**
* WORKAROUND: DWC3 revision < 2.20a have an issue
* which would cause metastability state on Run/Stop
@@ -1871,6 +1892,8 @@ static int dwc3_gadget_init_endpoints(struct dwc3 *dwc)
{
int ret;
+ dwc3_trace(trace_dwc3_gadget, "GBX: INIT ENDPOINTS: BEGIN");
+
INIT_LIST_HEAD(&dwc->gadget.ep_list);
ret = dwc3_gadget_init_hw_endpoints(dwc, dwc->num_out_eps, 0);
@@ -1887,6 +1910,8 @@ static int dwc3_gadget_init_endpoints(struct dwc3 *dwc)
return ret;
}
+ dwc3_trace(trace_dwc3_gadget, "GBX: INIT ENDPOINTS: END");
+
return 0;
}
@@ -2113,7 +2138,9 @@ static void dwc3_endpoint_transfer_complete(struct dwc3 *dwc,
* WORKAROUND: This is the 2nd half of U1/U2 -> U0 workaround.
* See dwc3_gadget_linksts_change_interrupt() for 1st half.
*/
+ /* GBX: Only enabling first half of workaround so that we stay out of U1/U2 */
if (dwc->revision < DWC3_REVISION_183A) {
+ dwc3_trace(trace_dwc3_gadget, "GBX: 2nd half of U1/U2 -> U0 workaround");
u32 reg;
int i;
@@ -2376,6 +2403,10 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)
{
int reg;
+ /* // Tried removing these writes to avoid going into U1/U2, but it didn't work :(
+ dwc3_trace(trace_dwc3_gadget,
+ "GBX: disconnect_interrupt: Not entering U1 and U2 even though we should");
+ */
reg = dwc3_readl(dwc->regs, DWC3_DCTL);
reg &= ~DWC3_DCTL_INITU1ENA;
dwc3_writel(dwc->regs, DWC3_DCTL, reg);
@@ -2481,9 +2512,12 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
reg = dwc3_readl(dwc->regs, DWC3_DSTS);
speed = reg & DWC3_DSTS_CONNECTSPD;
dwc->speed = speed;
-
dwc3_update_ram_clk_sel(dwc, speed);
+ dwc3_trace(trace_dwc3_gadget, "GBX: conndone_interrupt speed=%s",
+ dwc3_dsts_speed_string(speed),
+ dwc->revision);
+
switch (speed) {
case DWC3_DSTS_SUPERSPEED_PLUS:
dwc3_gadget_ep0_desc.wMaxPacketSize = cpu_to_le16(512);
@@ -2609,6 +2643,11 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
enum dwc3_link_state next = evtinfo & DWC3_LINK_STATE_MASK;
unsigned int pwropt;
+ dwc3_trace(trace_dwc3_gadget, "GBX: Processing Link Change interrupt: [%s] -> [%s]",
+ dwc3_gadget_link_string(dwc->link_state),
+ dwc3_gadget_link_string(next)
+ );
+
/*
* WORKAROUND: DWC3 < 2.50a have an issue when configured without
* Hibernation mode enabled which would show up when device detects
@@ -2655,8 +2694,10 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
* STAR#9000446952: RTL: Device SS : if U1/U2 ->U0 takes >128us
* core send LGO_Ux entering U0
*/
- if (dwc->revision < DWC3_REVISION_183A) {
+ // if (dwc->revision < DWC3_REVISION_183A) {
+ {
if (next == DWC3_LINK_STATE_U0) {
+ dwc3_trace(trace_dwc3_gadget, "GBX: using workaround for transition to U0");
u32 u1u2;
u32 reg;
@@ -2706,6 +2747,9 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc,
unsigned int evtinfo)
{
+ dwc3_trace(trace_dwc3_gadget, "GBX: suspend_interrupt: speed=%s",
+ dwc3_dsts_speed_string(dwc->speed));
+
enum dwc3_link_state next = evtinfo & DWC3_LINK_STATE_MASK;
if (dwc->link_state != next && next == DWC3_LINK_STATE_U3)
@@ -2787,7 +2831,8 @@ static void dwc3_gadget_interrupt(struct dwc3 *dwc,
dwc3_trace(trace_dwc3_gadget, "Erratic Error");
break;
case DWC3_DEVICE_EVENT_CMD_CMPL:
- dwc3_trace(trace_dwc3_gadget, "Command Complete");
+ dwc3_trace(trace_dwc3_gadget, "GBX: Command Complete. speed=%s",
+ dwc3_dsts_speed_string(dwc->speed));
break;
case DWC3_DEVICE_EVENT_OVERFLOW:
dwc3_trace(trace_dwc3_gadget, "Overflow");
--
2.14.1
