Ferry Toth wrote: > Hi > > Op 17-04-2021 om 16:22 schreef Ferry Toth: >> Hi >> >> Op 17-04-2021 om 04:27 schreef Thinh Nguyen: >>> Ferry Toth wrote: >>>> Hi >>>> >>>> Op 16-04-2021 om 00:23 schreef Thinh Nguyen: >>>>> Thinh Nguyen wrote: >>>>>> From: Yu Chen <chenyu56@xxxxxxxxxx> >>>>>> From: John Stultz <john.stultz@xxxxxxxxxx> >>>>>> >>>>>> According to the programming guide, to switch mode for DRD >>>>>> controller, >>>>>> the driver needs to do the following. >>>>>> >>>>>> To switch from device to host: >>>>>> 1. Reset controller with GCTL.CoreSoftReset >>>>>> 2. Set GCTL.PrtCapDir(host mode) >>>>>> 3. Reset the host with USBCMD.HCRESET >>>>>> 4. Then follow up with the initializing host registers sequence >>>>>> >>>>>> To switch from host to device: >>>>>> 1. Reset controller with GCTL.CoreSoftReset >>>>>> 2. Set GCTL.PrtCapDir(device mode) >>>>>> 3. Reset the device with DCTL.CSftRst >>>>>> 4. Then follow up with the initializing registers sequence >>>>>> >>>>>> Currently we're missing step 1) to do GCTL.CoreSoftReset and step >>>>>> 3) of >>>>>> switching from host to device. John Stult reported a lockup issue >>>>>> seen >>>>>> with HiKey960 platform without these steps[1]. Similar issue is >>>>>> observed >>>>>> with Ferry's testing platform[2]. >>>>>> >>>>>> So, apply the required steps along with some fixes to Yu Chen's >>>>>> and John >>>>>> Stultz's version. The main fixes to their versions are the missing >>>>>> wait >>>>>> for clocks synchronization before clearing GCTL.CoreSoftReset and >>>>>> only >>>>>> apply DCTL.CSftRst when switching from host to device. >>>>>> >>>>>> [1] >>>>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20210108015115.27920-1-john.stultz@xxxxxxxxxx/__;!!A4F2R9G_pg!PW9Jbs4wv4a_zKGgZHN0FYrIpfecPX0Ouq9V3d16Yz-9-GSHqZWsfBAF-WkeqLhzN4i3$ >>>>>> >>>>>> >>>>>> [2] >>>>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/0ba7a6ba-e6a7-9cd4-0695-64fc927e01f1@xxxxxxxxx/__;!!A4F2R9G_pg!PW9Jbs4wv4a_zKGgZHN0FYrIpfecPX0Ouq9V3d16Yz-9-GSHqZWsfBAF-WkeqGeZStt4$ >>>>>> >>>>>> >>>>>> >>>>>> Cc: Andy Shevchenko <andy.shevchenko@xxxxxxxxx> >>>>>> Cc: Ferry Toth <fntoth@xxxxxxxxx> >>>>>> Cc: Wesley Cheng <wcheng@xxxxxxxxxxxxxx> >>>>>> Cc: <stable@xxxxxxxxxxxxxxx> >>>>>> Fixes: 41ce1456e1db ("usb: dwc3: core: make dwc3_set_mode() work >>>>>> properly") >>>>>> Signed-off-by: Yu Chen <chenyu56@xxxxxxxxxx> >>>>>> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx> >>>>>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> >>>>>> --- >>>>>> Changes in v3: >>>>>> - Check if the desired mode is OTG, then keep the old flow >>>>>> - Remove condition for OTG support only since the device can still be >>>>>> configured DRD host/device mode only >>>>>> - Remove redundant hw_mode check since __dwc3_set_mode() only applies >>>>>> when >>>>>> hw_mode is DRD >>>>>> Changes in v2: >>>>>> - Initialize mutex per device and not as global mutex. >>>>>> - Add additional checks for DRD only mode >>>>>> >>>>>> drivers/usb/dwc3/core.c | 27 +++++++++++++++++++++++++++ >>>>>> drivers/usb/dwc3/core.h | 5 +++++ >>>>>> 2 files changed, 32 insertions(+) >>>>>> >>>>> Hi John, >>>>> >>>>> If possible, can you run a test with this version on your platform? >>>>> >>>>> Thanks, >>>>> Thinh >>>>> >>>> I tested this on edison-arduino with this patch on top of usb-next >>>> (5.12-rc7 + "increase BESL baseline to 6" to prevent throttling"). >>>> >>>> On this platform there is a physical switch to switch roles. With this >>>> patch I find: >>>> >>>> - switch to host mode always works fine >>>> >>>> - switch to gadget mode I need to flip the switch 3x >>>> (gadget-host-gadget). >>>> >>>> An error message appears on the gadget side "dwc3 dwc3.0.auto: timed >>>> out >>>> waiting for SETUP phase" appears, but then the device connects to my >>>> PC, >>>> no throttling. >>>> >>>> - alternatively I can switch to gadget 1x and then unplug/replug the >>>> cable. >>>> >>>> No error message and connects fine. >>>> >>>> - if I flip the switch only once, on the PC side I get: >>>> >>>> kernel: usb 1-5: new high-speed USB device number 18 usingxhci_hcd >>>> kernel: usb 1-5: New USB device found, idVendor=1d6b, >>>> idProduct=0104, bcdDevice= 1.00 kernel: usb 1-5: New USB device >>>> strings: Mfr=1, Product=2, SerialNumber=3 kernel: usb 1-5: Product: >>>> USBArmory Gadget kernel: usb 1-5: Manufacturer: USBArmory kernel: >>>> usb 1-5: SerialNumber: 0123456789abcdef kernel: usb 1-5: can't set >>>> config #1, error -110 >>> The device failed at set_configuration() request and timed out. It >>> probably timed out from the status stage looking at the device err >>> print. >>> >>>> Then if I wait long enough on the gadget side I get: >>>> >>>> root@yuna:~# ifconfig >>>> >>>> usb0: flags=-28605<UP,BROADCAST,RUNNING,MULTICAST,DYNAMIC> mtu 1500 >>>> inet 169.254.119.239 netmask 255.255.0.0 broadcast 169.254.255.255 >>>> inet6 fe80::a8bb:ccff:fedd:eef1 prefixlen 64 scopeid 0x20<link> >>>> ether aa:bb:cc:dd:ee:f1 txqueuelen 1000 (Ethernet) RX packets 490424 >>>> bytes 735146578 (701.0 MiB) RX errors 0 dropped 191 overruns 0 frame >>>> 0 TX packets 35279 bytes 2532746 (2.4 MiB) TX errors 0 dropped 0 >>>> overruns 0 carrier 0 collisions 0 >>>> >>>> (correct would be: inet 10.42.0.221 netmask 255.255.255.0 broadcast >>>> 10.42.0.255) >>>> >>>> So much improved now, but it seems I am still missing something on >>>> plug. >>>> >>> That's great! We can look at it further. Can you capture the tracepoints >>> of the issue. Also, can you try with mass_storage gadget to see if the >>> result is the same? >> >> I have already gser, eem, mass_storage and uac2 combo. When eem fails, >> the mass_storage and uac2 don't appear (on KDE you get all kind of >> popups when they appear). >> >> So either all works, or all fails. >> >> I'll trace this later today. > > Trace capturing switch from host-> gadget here > https://urldefense.com/v3/__https://github.com/andy-shev/linux/files/6329600/5.12-rc7*2Busb-next.zip__;JQ!!A4F2R9G_pg!Oa6XGH3IqY3wwG5KK4FwPuNA0m3q5bRj7N6vdP-y4sAY6mya-96J90NJ0tJnXLOiNwGT$ > > (Issue history: > https://urldefense.com/v3/__https://github.com/andy-shev/linux/issues/31__;!!A4F2R9G_pg!Oa6XGH3IqY3wwG5KK4FwPuNA0m3q5bRj7N6vdP-y4sAY6mya-96J90NJ0tJnXNc7KgAw$ > ) > > On the PC side this resulted to: > > apr 17 18:17:44 delfion kernel: usb 1-5: new high-speed USB device > number 12 using xhci_hcd > apr 17 18:17:44 delfion kernel: usb 1-5: New USB device found, > idVendor=1d6b, idProduct=0104, bcdDevice= 1.00 > apr 17 18:17:44 delfion kernel: usb 1-5: New USB device strings: Mfr=1, > Product=2, SerialNumber=3 > apr 17 18:17:44 delfion kernel: usb 1-5: Product: USBArmory Gadget > apr 17 18:17:44 delfion kernel: usb 1-5: Manufacturer: USBArmory > apr 17 18:17:44 delfion kernel: usb 1-5: SerialNumber: 0123456789abcdef > apr 17 18:17:49 delfion kernel: usb 1-5: can't set config #1, error -110 > > > Thanks for all your help! > Looks like it's LPM related again. To confirm, try this: Disable LPM with this property "snps,usb2-gadget-lpm-disable" (Note that it's not the same as "snps,dis_enblslpm_quirk") Make sure that your testing kernel has this patch [1] 475e8be53d04 ("usb: dwc3: gadget: Check for disabled LPM quirk") [1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=475e8be53d0496f9bc6159f4abb3ff5f9b90e8de The failure you saw was probably due the gadget function attempting to start a delayed status stage of the SET_CONFIGURATION request. By this time, the host already put the device in low power. The START_TRANSFER command needs to be executed while the device is on "ON" state (or U0 if eSS). We shouldn't use dwc->link_state to check for link state because we only enable link state change interrupt for some controller versions. Once you confirms disabling LPM works, try this fix: diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 6227641f2d31..06cdec79244e 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -309,10 +309,14 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd, if (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_STARTTRANSFER) { int needs_wakeup; + u8 link_state; - needs_wakeup = (dwc->link_state == DWC3_LINK_STATE_U1 || - dwc->link_state == DWC3_LINK_STATE_U2 || - dwc->link_state == DWC3_LINK_STATE_U3); + reg = dwc3_readl(dwc->regs, DWC3_DSTS); + link_state = DWC3_DSTS_USBLNKST(reg); + + needs_wakeup = (link_state == DWC3_LINK_STATE_U1 || + link_state == DWC3_LINK_STATE_U2 || + link_state == DWC3_LINK_STATE_U3); if (unlikely(needs_wakeup)) { ret = __dwc3_gadget_wakeup(dwc); @@ -1989,6 +1993,8 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc) case DWC3_LINK_STATE_RESET: case DWC3_LINK_STATE_RX_DET: /* in HS, means Early Suspend */ case DWC3_LINK_STATE_U3: /* in HS, means SUSPEND */ + case DWC3_LINK_STATE_U2: /* in HS, means Sleep (L1) */ + case DWC3_LINK_STATE_U1: case DWC3_LINK_STATE_RESUME: break; default: BR, Thinh
