Re: USB network gadget / DWC3 issue

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

Ferry Toth wrote:
> 
> Op 13-04-2021 om 04:17 schreef Thinh Nguyen:
>> Ferry Toth wrote:
>>> Op 11-04-2021 om 02:04 schreef Thinh Nguyen:
>>>> Ferry Toth wrote:
>>>>> Hi, some corrections below.
>>>>>
>>>>> Op 10-04-2021 om 15:29 schreef Ferry Toth:
>>>>>> Op 09-04-2021 om 15:26 schreef Ferry Toth:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Op 08-04-2021 om 23:12 schreef Thinh Nguyen:
>>>>>>>> Ferry Toth wrote:
>>>>>>>>> Op 07-04-2021 om 15:34 schreef Andy Shevchenko:
>>>>>>>>>> On Wed, Apr 7, 2021 at 3:24 AM Thinh Nguyen
>>>>>>>>>> <Thinh.Nguyen@synopsys.com> wrote:
>>>>>>>>>>> Thinh Nguyen wrote:
>>>>>>>>>> ...
>>>>>>>>>>
>>>>>>>>>>>> I took a look at the "bad" and "normal" tracepoints. There
>>>>>>>>>>>> are a
>>>>>>>>>>>> few
>>>>>>>>>>>> 1-second delays where the host tried to bring the device
>>>>>>>>>>>> back and
>>>>>>>>>>>> resume from low power:
>>>>>>>>>>>>
>>>>>>>>>>>>          ksoftirqd/0-10      [000] d.s.   231.501808:
>>>>>>>>>>>> dwc3_gadget_ep_cmd: ep3in: cmd 'Update Transfer' [60007] params
>>>>>>>>>>>> 00000000 00000000 00000000 --> status: Successful
>>>>>>>>>>>>          ksoftirqd/0-10      [000] d.s.   231.501809:
>>>>>>>>>>>> dwc3_readl:
>>>>>>>>>>>> addr
>>>>>>>>>>>> 00000000d68ecd36 value 0000a610
>>>>>>>>>>>>          ksoftirqd/0-10      [000] d.s.   231.501810:
>>>>>>>>>>>> dwc3_writel:
>>>>>>>>>>>> addr
>>>>>>>>>>>> 00000000d68ecd36 value 0000a710
>>>>>>>>>>>>               <idle>-0       [000] d.h. 232.499418: dwc3_readl:
>>>>>>>>>>>> addr
>>>>>>>>>>>> 00000000a15e0e35 value 00000034
>>>>>>>>>>>>               <idle>-0       [000] d.h. 232.499423: dwc3_readl:
>>>>>>>>>>>> addr
>>>>>>>>>>>> 00000000bb67b585 value 00001000
>>>>>>>>>>>>               <idle>-0       [000] d.h. 232.499425:
>>>>>>>>>>>> dwc3_writel: addr
>>>>>>>>>>>> 00000000bb67b585 value 80001000
>>>>>>>>>>>>               <idle>-0       [000] d.h. 232.499427:
>>>>>>>>>>>> dwc3_writel: addr
>>>>>>>>>>>> 00000000a15e0e35 value 00000034
>>>>>>>>>>>>          irq/15-dwc3-476     [000] d...   232.499480:
>>>>>>>>>>>> dwc3_event:
>>>>>>>>>>>> event
>>>>>>>>>>>> (00000401): WakeUp [U0]
>>>>>>>>>>>>          irq/15-dwc3-476     [000] d...   232.499492:
>>>>>>>>>>>> dwc3_event:
>>>>>>>>>>>> event
>>>>>>>>>>>> (00000401): WakeUp [U0]
>>>>>>>>>>>>          irq/15-dwc3-476     [000] d...   232.499496:
>>>>>>>>>>>> dwc3_event:
>>>>>>>>>>>> event
>>>>>>>>>>>> (00006088): ep2out: Transfer In Progress [0] (SIm)
>>>>>>>>>>>>          irq/15-dwc3-476     [000] d...   232.499501:
>>>>>>>>>>>> dwc3_complete_trb: ep2out: trb 00000000c7ce524e (E179:D170) buf
>>>>>>>>>>>> 0000000008273540 size 1463 ctrl 00000818 (hlcS:sC:normal)
>>>>>>>>>>>>          irq/15-dwc3-476     [000] d...   232.499518:
>>>>>>>>>>>> dwc3_gadget_giveback: ep2out: req 0000000012e296cf length
>>>>>>>>>>>> 73/1536
>>>>>>>>>>>> zsI ==> 0
>>>>>>>>>>>>          irq/15-dwc3-476     [000] d...   232.499562:
>>>>>>>>>>>> dwc3_ep_queue:
>>>>>>>>>>>> ep2out: req 0000000012e296cf length 0/1536 zsI ==> -115
>>>>>>>>>>>>          irq/15-dwc3-476     [000] d...   232.499601:
>>>>>>>>>>>> dwc3_prepare_trb:
>>>>>>>>>>>> ep2out: trb 000000008c083777 (E180:D170) buf 0000000002a7e9c0
>>>>>>>>>>>> size
>>>>>>>>>>>> 1536 ctrl 00000819 (HlcS:sC:normal)
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Your device is operating in highspeed right? Try to turn off
>>>>>>>>>>>> LPM
>>>>>>>>>>>> from
>>>>>>>>>>>> host and see if that helps with the speed throttling issue. (If
>>>>>>>>>>>> you're
>>>>>>>>>>>> using xHCI host, then set XHCI_HW_LPM_DISABLE). It may also
>>>>>>>>>>>> help
>>>>>>>>>>>> with
>>>>>>>>>>>> the connection issue you saw.
>>>>>>>>>>>>
>>>>>>>>>>>> It seems to be an issue from host, but I can't tell for sure
>>>>>>>>>>>> unless we
>>>>>>>>>>>> have some USB traffic analyzer that shows what's going on.
>>>>>>>>>>>> Have you
>>>>>>>>>>>> tried different hosts?
>>>>>>>>>>>>
>>>>>>>>>>> You can also disable LPM from the gadget side by setting
>>>>>>>>>>> dwc->dis_enblslpm_quirk.
>>>>>>>>>> Ferry, it can be done by adding a corresponding property to the
>>>>>>>>>> dwc3-pci.c for Intel Merrifield platform. I'll check also for my
>>>>>>>>>> case
>>>>>>>>>> and perhaps I can collect some traces in my case later on when I
>>>>>>>>>> have
>>>>>>>>>> more time for that.
>>>>>>>>>>
>>>>>>>>> Ok thanks all. Here is what I tried:
>>>>>>>>>
>>>>>>>>> Another computer (Acer 720P brainwashed chromebook), I tried both
>>>>>>>>> full
>>>>>>>>> speed and high speed. Still throttling but less bad.
>>>>>>>>>
>>>>>>>>> Then on desktop, with Edison kernel 5.12-rc5 as above + this
>>>>>>>>> patch:
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/usb/dwc3/dwc3-pci.c
>>>>>>>>> b/drivers/usb/dwc3/dwc3-pci.c
>>>>>>>>>
>>>>>>>>> index 4c5c6972124a..a9268c085840 100644
>>>>>>>>>
>>>>>>>>> --- a/drivers/usb/dwc3/dwc3-pci.c
>>>>>>>>>
>>>>>>>>> +++ b/drivers/usb/dwc3/dwc3-pci.c
>>>>>>>>>
>>>>>>>>> @@ -122,6 +122,7 @@ static const struct property_entry
>>>>>>>>> dwc3_pci_mrfld_properties[] = {
>>>>>>>>>
>>>>>>>>> PROPERTY_ENTRY_STRING("linux,extcon-name", "mrfld_bcove_pwrsrc"),
>>>>>>>>>
>>>>>>>>> PROPERTY_ENTRY_BOOL("snps,dis_u3_susphy_quirk"),
>>>>>>>>>
>>>>>>>>> PROPERTY_ENTRY_BOOL("snps,dis_u2_susphy_quirk"),
>>>>>>>>>
>>>>>>>>> + PROPERTY_ENTRY_BOOL("snps,dis_enblslpm_quirk"),
>>>>>>>>>
>>>>>>>>> PROPERTY_ENTRY_BOOL("linux,sysdev_is_parent"),
>>>>>>>>>
>>>>>>>>> {}
>>>>>>>>>
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>> This fixes the throttling but reveals I had actually at least 2
>>>>>>>>> bugs:
>>>>>>>>>
>>>>>>>>> 1) throttling due to LPM, this seems solved now, thanks to much!
>>>>>>>> Now that we can confirm the speed throttling is related to LPM. We
>>>>>>>> can
>>>>>>>> try to experiment further. (IMO, LPM is an important feature and
>>>>>>>> totally disabling LPM seems like using a sledgehammer to crack a
>>>>>>>> nut)
>>>>>>>>
>>>>>>>> I suspect that your phy/HW has a higher low power exit latency. I
>>>>>>>> don't
>>>>>>>> think you provided any HIRD threshold property in your setup
>>>>>>>> right? So
>>>>>>>> by default, dwc3 sets the base line BESL value to 1 (or 150us).
>>>>>>>> Unless
>>>>>>>> you know what your phy/HW is capable of, try to test and
>>>>>>>> increase the
>>>>>>>> recommended BESL value. The range can be from 0 to 15 where 0 is
>>>>>>>> 150us
>>>>>>>> and 15 is 10ms. Maybe try 6 (i.e. 1ms).
>>>>>>>>
>>>>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>>>>>> index 60e850a395a2..423533df8927 100644
>>>>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>>>>> @@ -2895,7 +2895,7 @@ static void dwc3_gadget_config_params(struct
>>>>>>>> usb_gadget *g,
>>>>>>>>                     * recommended BESL baseline to 1 and clamp the
>>>>>>>> BESL deep to be
>>>>>>>>                     * within 2 to 15.
>>>>>>>>                     */
>>>>>>>> -               params->besl_baseline = 1;
>>>>>>>> +               params->besl_baseline = 6;
>>>>>>>>                    if (dwc->is_utmi_l1_suspend)
>>>>>>>>                           params->besl_deep =
>>>>>>>>                                    clamp_t(u8,
>>>>>>>> dwc->hird_threshold, 2,
>>>>>>>> 15);
>>>>>>>>
>>>>>>> I will try and report back, hopefully this evening.
>>>>>> I tried this and it seems to have the same effect as
>>>>>> dis_enblslpm_quirk
>>>>>>>>> 2) a problem with usb plug detection
>>>>>>>>>
>>>>>>>>> When I unplug/replug the gadget cable I need to do that at least
>>>>>>>>> another
>>>>>>>>> time before gadget is detected. So unplug/replug/unplug/replug
>>>>>>>>> seems to
>>>>>>>>> work.
>>>>>>>>>
>>>>>>>>> Also this platform has a HW switch to select host/device mode,
>>>>>>>>> with
>>>>>>>>> separate connectors for host and device.
>>>>>>>>>
>>>>>>>>> When I flip the switch to host it immediately changes to host.
>>>>>>>>>
>>>>>>>>> Flipping to device leaves the LEDs on my connected usb hub on, so
>>>>>>>>> it's
>>>>>>>>> still powered (but not operational).
>>>>>>>>>
>>>>>>>>> Flipping fast host/device (within 1/2 sec) hub LEDs turns off.
>>>>>>>>> But I
>>>>>>>>> still need to additionally unplug/replug the gadget cable to get
>>>>>>>>> that to
>>>>>>>>> work.
>>>>>>>>>
>>>>>>>> The connection issue can come from different things. Please narrow
>>>>>>>> it down
>>>>>>>> and make sure that you don't use any defective cable or bad hub.
>>>>>>>> Even then,
>>>>>>>> it's difficult to determine whose fault it is from just the dmesg
>>>>>>>> and driver
>>>>>>>> logs alone without looking at the USB traffic at the packet level.
>>>>>>>>
>>>>>>>> Btw, is your setup DRD? If you're switching mode, then I know that
>>>>>>>> dwc3 right
>>>>>>>> now doesn't implement mode switching correctly.
>>>>>>> Yes, we use Extcon driver to support DRD.
>>>>>>>> You can see the discussion we have here:
>>>>>>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20210108015115.27920-1-john.stultz@linaro.org/T/*t__;Iw!!A4F2R9G_pg!MXee1rloMlVeQuXlR60t94lr_6imLoVLTEFXzYWhS27dZFAFtH5AWssCZxlDLGcaKy2f$
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>> I see, that might indeed be related. I will try the patches to
>>>>>>> see if
>>>>>>> that works and report back.
>>>>>> I applied both patches:
>>>>>>
>>>>>> usb: dwc3: Trigger a GCTL soft reset when switching modes in DRD
>>>>>>
>>>>>> usb: dwc3: Fix DRD mode change sequence following programming guide
>>>>>>
>>>>>> It doesn't have an effect on the need to unplug/replug neither on the
>>>>>> problems switch from host/device mode.
>>>>> When I test the correct kernel it does have an effect :-)
>>>>>
>>>>> In most cases the need to unplug/replug is removed, but not always. In
>>>>> the cases when I need to retry the host journal shows "can't set
>>>>> config
>>>>> #1, error -110"
>>>> It's most likely because the driver didn't provide time for the clocks
>>>> synchronization before clearing the GCTL soft reset. I noted that issue
>>>> in the patch in the discussion thread. I can send out a patch next
>>>> week.
>>>>
>>>>> The switch from host->device and device->host mode seems to be
>>>>> resolved.
>>>>>
>>>>> Strangely, iperf3 now reports 130 Mbits/sec (down from 200 Mbits/sec).
>>>>>
>>>> Did this happen with disabling LPM or with increasing BESL baseline?
>>>> Note that increasing the recommended BESL is not the same as disabling
>>>> LPM. With the recommended BESL provided, the host can decide when it
>>>> should put the device in low power so that the device has enough
>>>> time to
>>>> wake up. With LPM enabled, there maybe some minor speed degradation but
>>>> not that much. Anyway, you can experiment with the BESL value to have
>>>> the acceptable speed while still have power saving capability (or
>>>> completely disable LPM if power saving is not an issue for you).
>>> I tried both, the result was exactly the same.
>> That's strange... Also, enabling LPM should not impact the performance
>> that
>> much at all. What's changed to your setup?
>>
>> Anyway, can you try this patch instead of John Stult's. There are a
>> couple
>> of issues from his patches.
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 05e2e54cbbdc..675e861fda1a 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/kernel.h>
>>   #include <linux/slab.h>
>>   #include <linux/spinlock.h>
>> +#include <linux/mutex.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/pm_runtime.h>
>>   #include <linux/interrupt.h>
>> @@ -40,6 +41,8 @@
>>     #define DWC3_DEFAULT_AUTOSUSPEND_DELAY 5000 /* ms */
>>   +static DEFINE_MUTEX(mode_switch_lock);
>> +
>>   /**
>>    * dwc3_get_dr_mode - Validates and sets dr_mode
>>    * @dwc: pointer to our context structure
>> @@ -114,13 +117,20 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode)
>>          dwc->current_dr_role = mode;
>>   }
>>   +static int dwc3_core_soft_reset(struct dwc3 *dwc);
>> +
>>   static void __dwc3_set_mode(struct work_struct *work)
>>   {
>>          struct dwc3 *dwc = work_to_dwc(work);
>>          unsigned long flags;
>> +       unsigned int hw_mode;
>>          int ret;
>>          u32 reg;
>>   +       mutex_lock(&mode_switch_lock);
>> +
>> +       hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
>> +
>>          pm_runtime_get_sync(dwc->dev);
>>            if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_OTG)
>> @@ -154,6 +164,24 @@ static void __dwc3_set_mode(struct work_struct
>> *work)
>>                  break;
>>          }
>>   +       if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD) {
>> +               reg = dwc3_readl(dwc->regs, DWC3_GCTL);
>> +               reg |= DWC3_GCTL_CORESOFTRESET;
>> +               dwc3_writel(dwc->regs, DWC3_GCTL, reg);
>> +
>> +               /*
>> +                * Wait for internal clocks to synchronized. DWC_usb31
>> and
>> +                * DWC_usb32 may need at least 50ms (less for
>> DWC_usb3). To
>> +                * keep it consistent across different IPs, let's wait
>> up to
>> +                * 100ms before clearing GCTL.CORESOFTRESET.
>> +                */
>> +               msleep(100);
>> +
>> +               reg = dwc3_readl(dwc->regs, DWC3_GCTL);
>> +               reg &= ~DWC3_GCTL_CORESOFTRESET;
>> +               dwc3_writel(dwc->regs, DWC3_GCTL, reg);
>> +       }
>> +
>>          spin_lock_irqsave(&dwc->lock, flags);
>>            dwc3_set_prtcap(dwc, dwc->desired_dr_role);
>> @@ -178,6 +206,9 @@ static void __dwc3_set_mode(struct work_struct *work)
>>                  }
>>                  break;
>>          case DWC3_GCTL_PRTCAP_DEVICE:
>> +               if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD)
>> +                       dwc3_core_soft_reset(dwc);
>> +
>>                  dwc3_event_buffers_setup(dwc);
>>                    if (dwc->usb2_phy)
>> @@ -200,6 +231,7 @@ static void __dwc3_set_mode(struct work_struct *work)
>>   out:
>>          pm_runtime_mark_last_busy(dwc->dev);
>>          pm_runtime_put_autosuspend(dwc->dev);
>> +       mutex_unlock(&mode_switch_lock);
>>   }
>>     void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
> This doesn't apply on 5.12-rc5 correct? On which would you like me to
> test it on?

Please test on Greg's "usb-next" branch
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/

Thanks,
Thinh