Felipe Balbi wrote: > > Hi, > > John Stultz <john.stultz@xxxxxxxxxx> writes: >> On Fri, Apr 16, 2021 at 12:49 PM Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> wrote: >>> >>> John Stultz wrote: >>>> On Fri, Apr 16, 2021 at 3:47 AM Felipe Balbi <balbi@xxxxxxxxxx> wrote: >>>>> >>>>> >>>>> Hi, >>>>> >>>>> Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> writes: >>>>> >>>>>> 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!L4TLb25Nkq0DF2qrCPWW13PUq4idhDn5QSZhgvnVAy7wJiYFOSSouSptwo9nOzIdPD4j$ >>>>>> [2] https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/0ba7a6ba-e6a7-9cd4-0695-64fc927e01f1@xxxxxxxxx/__;!!A4F2R9G_pg!L4TLb25Nkq0DF2qrCPWW13PUq4idhDn5QSZhgvnVAy7wJiYFOSSouSptwo9nO21VT8q7$ >>>>>> >>>>>> 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> >>>>> >>>>> I still have concerns about the soft reset, but I won't block you guys >>>>> from fixing Hikey's problem :-) >>>>> >>>>> The only thing I would like to confirm is that this has been verified >>>>> with hundreds of swaps happening as quickly as possible. DWC3 should >>>>> still be functional after several hundred swaps. >>>>> >>>>> Can someone confirm this is the case? (I'm assuming this can be >>>>> scripted) >>>> >>>> I unfortunately don't have an easy way to automate the switching right >>>> off. But I'll try to hack up the mux switch driver to provide an >>>> interface we can script against. >>>> >>> >>> FYI, you can do the following: >>> >>> 1) Enable "usb-role-switch" DT property if not already done so >>> 2) Add userspace control: >>> >>> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c >>> index e2b68bb770d1..b203e3d87291 100644 >>> --- a/drivers/usb/dwc3/drd.c >>> +++ b/drivers/usb/dwc3/drd.c >>> @@ -555,6 +555,7 @@ static int dwc3_setup_role_switch(struct dwc3 *dwc) >>> mode = DWC3_GCTL_PRTCAP_DEVICE; >>> } >>> >>> + dwc3_role_switch.allow_userspace_control = true; >>> dwc3_role_switch.fwnode = dev_fwnode(dwc->dev); >>> dwc3_role_switch.set = dwc3_usb_role_switch_set; >>> dwc3_role_switch.get = dwc3_usb_role_switch_get; >>> >>> 3) Write a script to do the following: >>> >>> # echo host > /sys/class/usb_role/<UDC>/role >>> >>> and >>> >>> # echo device > /sys/class/usb_role/<UDC>/role >> >> Thanks so much for this. So I ran both of those commands in a while >> loop for awhile and didn't see any trouble. >> >> HiKey960 is interesting as well because we have a mux switch, which is >> sort of an intermediary roll switcher (it gets the role switch signal >> from the tcpci_rt1711h, tweaks some gpios and then signals the dwc3). >> So I also did the above tweaks to the mux-switch and had it switching >> between device/none and validated the onboard hub came up and down >> along with the dwc3 core. >> >> Everything still looks good here. > > Sounds good, happy to see so many platforms supported by Thinh's > change. Thanks for doing this work, Thinh :-) > Thanks for the review Felipe :) Thinh
