RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller

[Jun Li]

Hi Thinh,
> -----Original Message-----
> From: Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx>
> Sent: 2020å5æ21æ 9:56
> To: Jun Li <jun.li@xxxxxxx>; Felipe Balbi <balbi@xxxxxxxxxx>; Jun Li
> <lijun.kernel@xxxxxxxxx>
> Cc: John Stultz <john.stultz@xxxxxxxxxx>; lkml <linux-kernel@xxxxxxxxxxxxxxx>; Yu
> Chen <chenyu56@xxxxxxxxxx>; Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Rob
> Herring <robh+dt@xxxxxxxxxx>; Mark Rutland <mark.rutland@xxxxxxx>; ShuFan Lee
> <shufan_lee@xxxxxxxxxxx>; Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>;
> Suzuki K Poulose <suzuki.poulose@xxxxxxx>; Chunfeng Yun
> <chunfeng.yun@xxxxxxxxxxxx>; Hans de Goede <hdegoede@xxxxxxxxxx>; Andy Shevchenko
> <andy.shevchenko@xxxxxxxxx>; Valentin Schneider <valentin.schneider@xxxxxxx>;
> Jack Pham <jackp@xxxxxxxxxxxxxx>; Linux USB List <linux-usb@xxxxxxxxxxxxxxx>; open
> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@xxxxxxxxxxxxxxx>;
> Peter Chen <peter.chen@xxxxxxx>
> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device
> controller
> 
> Thinh Nguyen wrote:
> > Jun Li wrote:
> >> Hi
> >>
> >>> -----Original Message-----
> >>> From: Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx>
> >>> Sent: 2020å5æ19æ 14:46
> >>> To: Jun Li <jun.li@xxxxxxx>; Felipe Balbi <balbi@xxxxxxxxxx>; Jun Li
> >>> <lijun.kernel@xxxxxxxxx>
> >>> Cc: John Stultz <john.stultz@xxxxxxxxxx>; lkml
> >>> <linux-kernel@xxxxxxxxxxxxxxx>; Yu Chen <chenyu56@xxxxxxxxxx>; Greg
> >>> Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Rob Herring
> >>> <robh+dt@xxxxxxxxxx>; Mark Rutland <mark.rutland@xxxxxxx>; ShuFan
> >>> Lee <shufan_lee@xxxxxxxxxxx>; Heikki Krogerus
> >>> <heikki.krogerus@xxxxxxxxxxxxxxx>;
> >>> Suzuki K Poulose <suzuki.poulose@xxxxxxx>; Chunfeng Yun
> >>> <chunfeng.yun@xxxxxxxxxxxx>; Hans de Goede <hdegoede@xxxxxxxxxx>;
> >>> Andy Shevchenko <andy.shevchenko@xxxxxxxxx>; Valentin Schneider
> >>> <valentin.schneider@xxxxxxx>; Jack Pham <jackp@xxxxxxxxxxxxxx>;
> >>> Linux USB List <linux-usb@xxxxxxxxxxxxxxx>; open list:OPEN FIRMWARE
> >>> AND FLATTENED DEVICE TREE BINDINGS <devicetree@xxxxxxxxxxxxxxx>;
> >>> Peter Chen <peter.chen@xxxxxxx>
> >>> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct
> >>> cleared by device controller
> >>>
> >>> Thinh Nguyen wrote:
> >>>> Jun Li wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Felipe Balbi <balbif@xxxxxxxxx> On Behalf Of Felipe Balbi
> >>>>>> Sent: 2020å5æ16æ 19:57
> >>>>>> To: Jun Li <jun.li@xxxxxxx>; Thinh Nguyen
> >>>>>> <Thinh.Nguyen@xxxxxxxxxxxx>; Jun Li <lijun.kernel@xxxxxxxxx>
> >>>>>> Cc: John Stultz <john.stultz@xxxxxxxxxx>; lkml
> >>>>>> <linux-kernel@xxxxxxxxxxxxxxx>; Yu Chen <chenyu56@xxxxxxxxxx>;
> >>>>>> Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Rob Herring
> >>>>>> <robh+dt@xxxxxxxxxx>; Mark Rutland <mark.rutland@xxxxxxx>; ShuFan
> >>>>>> Lee <shufan_lee@xxxxxxxxxxx>; Heikki Krogerus
> >>>>>> <heikki.krogerus@xxxxxxxxxxxxxxx>;
> >>>>>> Suzuki K Poulose <suzuki.poulose@xxxxxxx>; Chunfeng Yun
> >>>>>> <chunfeng.yun@xxxxxxxxxxxx>; Hans de Goede <hdegoede@xxxxxxxxxx>;
> >>>>>> Andy Shevchenko <andy.shevchenko@xxxxxxxxx>; Valentin Schneider
> >>>>>> <valentin.schneider@xxxxxxx>; Jack Pham <jackp@xxxxxxxxxxxxxx>;
> >>>>>> Linux USB List <linux-usb@xxxxxxxxxxxxxxx>; open list:OPEN
> >>>>>> FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
> >>>>>> <devicetree@xxxxxxxxxxxxxxx>; Peter Chen <peter.chen@xxxxxxx>;
> >>>>>> Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx>
> >>>>>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for
> >>>>>> CmdAct cleared by device controller
> >>>>>>
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> Jun Li <jun.li@xxxxxxx> writes:
> >>>>>>>>>> Hi Thinh, could you comment this?
> >>>>>>>>> You only need to wake up the usb2 phy when issuing the command
> >>>>>>>>> while running in highspeed or below. If you're running in SS
> >>>>>>>>> or higher, internally the controller does it for you for usb3 phy.
> >>>>>>>>> In Jun's case, it seems like it takes longer for his phy to wake up.
> >>>>>>>>>
> >>>>>>>>> IMO, in this case, I think it's fine to increase the command timeout.
> >>>>>>>> Is there an upper limit to this? Is 32k clock the slowest that
> >>>>>>>> can be fed to the PHY as a suspend clock?
> >>>>>>> Yes, 32K clock is the slowest, Per DWC3 document on Power Down
> >>>>>>> Scale (bits 31:19 of GCTL):
> >>>>>>>
> >>>>>>> "Power Down Scale (PwrDnScale)
> >>>>>>> The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock
> >>>>>>> source to a small part of the USB3 controller that operates when
> >>>>>>> the SS PHY is in its lowest power (P3) state, and therefore does not provide
> a clock.
> >>>>>>> The Power Down Scale field specifies how many suspend_clk
> >>>>>>> periods fit into a 16 kHz clock period. When performing the
> >>>>>>> division, round up the remainder.
> >>>>>>> For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz
> >>>>>>> Suspend clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563
> >>>>>>> (rounder up)
> >>>>>>> Note:
> >>>>>>> - Minimum Suspend clock frequency is 32 kHz
> >>>>>>> - Maximum Suspend clock frequency is 125 MHz"
> >>>>>> Cool, now do we have an upper bound for how many clock cycles it
> >>>>>> takes to wake up the PHY?
> >>>>> My understanding is this ep command does not wake up the SS PHY,
> >>>>> the SS PHY still stays at P3 when execute this ep command. The
> >>>>> time required here is to wait controller complete something for
> >>>>> this ep command with 32K clock.
> >>>> Sorry I made a mistake. You're right. Just checked with one of the
> >>>> RTL engineers, and it doesn't need to wake up the phy. However, if
> >>>> it is eSS speed, it may take longer time as the command may be
> >>>> completing with the suspend clock.
> >>>>
> >>> What's the value for GCTL[7:6]?
> >> 2'b00
> >>
> >> Thanks
> >> Li Jun
> > (Sorry for the delay reply)
> >
> > If it's 0, then the ram clock should be the same as the bus_clk, which
> > is odd since you mentioned that the suspend_clk is used instead while in P3.
> 
> Just checked with the RTL engineer, even if GCTL[7:6] is set to 0, internally it
> can still run with suspend clock during P3.

Thanks for your check.
> 
> > Anyway, I was looking for a way maybe to improve the speed during
> > issuing a command. One way is to set GUSB3PIPECTL[17]=0, and it should
> > wakeup the phy anytime. I think Felipe suggested it. It's odd that it
> > doesn't work for you. I don't have other ideas beside increasing the
> > command timeout.
> >
> 
> In any case, increasing the timeout should be fine with me. It maybe difficult to
> determine the max timeout base on the slowest clock rate and number of cycles.
> Different controller and controller versions behave differently and may have
> different number of clock cycles to complete a command.
> 
> The RTL engineer recommended timeout to be at least 1ms (which maybe more than the
> polling rate of this patch). I'm fine with either the rate provided by this tested
> patch or higher.

OK, I will change the timeout to be 1ms if no object from Felipe.

thanks
Li Jun
> 
> BR,
> Thinh