Re: [PATCH v3] usb: dwc3: core: Do core softreset when switch mode

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

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@huawei.com>
>>>>>> From: John Stultz <john.stultz@linaro.org>
>>>>>>
>>>>>> 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@linaro.org/__;!!A4F2R9G_pg!PW9Jbs4wv4a_zKGgZHN0FYrIpfecPX0Ouq9V3d16Yz-9-GSHqZWsfBAF-WkeqLhzN4i3$
>>>>>>
>>>>>>
>>>>>> [2]
>>>>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/0ba7a6ba-e6a7-9cd4-0695-64fc927e01f1@gmail.com/__;!!A4F2R9G_pg!PW9Jbs4wv4a_zKGgZHN0FYrIpfecPX0Ouq9V3d16Yz-9-GSHqZWsfBAF-WkeqGeZStt4$
>>>>>>
>>>>>>
>>>>>>
>>>>>> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
>>>>>> Cc: Ferry Toth <fntoth@gmail.com>
>>>>>> Cc: Wesley Cheng <wcheng@codeaurora.org>
>>>>>> Cc: <stable@vger.kernel.org>
>>>>>> Fixes: 41ce1456e1db ("usb: dwc3: core: make dwc3_set_mode() work
>>>>>> properly")
>>>>>> Signed-off-by: Yu Chen <chenyu56@huawei.com>
>>>>>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>>>>>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>>>>> ---
>>>>>> 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