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.ngu...@synopsys.com>
> Sent: 2020年5月21日 9:56
> To: Jun Li <jun...@nxp.com>; Felipe Balbi <ba...@kernel.org>; Jun Li
> <lijun.ker...@gmail.com>
> Cc: John Stultz <john.stu...@linaro.org>; lkml 
> <linux-kernel@vger.kernel.org>; Yu
> Chen <cheny...@huawei.com>; Greg Kroah-Hartman <gre...@linuxfoundation.org>; 
> Rob
> Herring <robh...@kernel.org>; Mark Rutland <mark.rutl...@arm.com>; ShuFan Lee
> <shufan_...@richtek.com>; Heikki Krogerus <heikki.kroge...@linux.intel.com>;
> Suzuki K Poulose <suzuki.poul...@arm.com>; Chunfeng Yun
> <chunfeng....@mediatek.com>; Hans de Goede <hdego...@redhat.com>; Andy 
> Shevchenko
> <andy.shevche...@gmail.com>; Valentin Schneider <valentin.schnei...@arm.com>;
> Jack Pham <ja...@codeaurora.org>; Linux USB List <linux-...@vger.kernel.org>; 
> open
> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS 
> <devicet...@vger.kernel.org>;
> Peter Chen <peter.c...@nxp.com>
> 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.ngu...@synopsys.com>
> >>> Sent: 2020年5月19日 14:46
> >>> To: Jun Li <jun...@nxp.com>; Felipe Balbi <ba...@kernel.org>; Jun Li
> >>> <lijun.ker...@gmail.com>
> >>> Cc: John Stultz <john.stu...@linaro.org>; lkml
> >>> <linux-kernel@vger.kernel.org>; Yu Chen <cheny...@huawei.com>; Greg
> >>> Kroah-Hartman <gre...@linuxfoundation.org>; Rob Herring
> >>> <robh...@kernel.org>; Mark Rutland <mark.rutl...@arm.com>; ShuFan
> >>> Lee <shufan_...@richtek.com>; Heikki Krogerus
> >>> <heikki.kroge...@linux.intel.com>;
> >>> Suzuki K Poulose <suzuki.poul...@arm.com>; Chunfeng Yun
> >>> <chunfeng....@mediatek.com>; Hans de Goede <hdego...@redhat.com>;
> >>> Andy Shevchenko <andy.shevche...@gmail.com>; Valentin Schneider
> >>> <valentin.schnei...@arm.com>; Jack Pham <ja...@codeaurora.org>;
> >>> Linux USB List <linux-...@vger.kernel.org>; open list:OPEN FIRMWARE
> >>> AND FLATTENED DEVICE TREE BINDINGS <devicet...@vger.kernel.org>;
> >>> Peter Chen <peter.c...@nxp.com>
> >>> 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 <bal...@gmail.com> On Behalf Of Felipe Balbi
> >>>>>> Sent: 2020年5月16日 19:57
> >>>>>> To: Jun Li <jun...@nxp.com>; Thinh Nguyen
> >>>>>> <thinh.ngu...@synopsys.com>; Jun Li <lijun.ker...@gmail.com>
> >>>>>> Cc: John Stultz <john.stu...@linaro.org>; lkml
> >>>>>> <linux-kernel@vger.kernel.org>; Yu Chen <cheny...@huawei.com>;
> >>>>>> Greg Kroah-Hartman <gre...@linuxfoundation.org>; Rob Herring
> >>>>>> <robh...@kernel.org>; Mark Rutland <mark.rutl...@arm.com>; ShuFan
> >>>>>> Lee <shufan_...@richtek.com>; Heikki Krogerus
> >>>>>> <heikki.kroge...@linux.intel.com>;
> >>>>>> Suzuki K Poulose <suzuki.poul...@arm.com>; Chunfeng Yun
> >>>>>> <chunfeng....@mediatek.com>; Hans de Goede <hdego...@redhat.com>;
> >>>>>> Andy Shevchenko <andy.shevche...@gmail.com>; Valentin Schneider
> >>>>>> <valentin.schnei...@arm.com>; Jack Pham <ja...@codeaurora.org>;
> >>>>>> Linux USB List <linux-...@vger.kernel.org>; open list:OPEN
> >>>>>> FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
> >>>>>> <devicet...@vger.kernel.org>; Peter Chen <peter.c...@nxp.com>;
> >>>>>> Thinh Nguyen <thinh.ngu...@synopsys.com>
> >>>>>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for
> >>>>>> CmdAct cleared by device controller
> >>>>>>
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> Jun Li <jun...@nxp.com> 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