| Message ID | 20191028215919.83697-4-john.stultz@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Prereqs for HiKey960 USB support | expand |
Hi, John Stultz <john.stultz@linaro.org> writes: > From: Yu Chen <chenyu56@huawei.com> > > It needs more time for the device controller to clear the CmdAct of > DEPCMD on Hisilicon Kirin Soc. Why does it need more time? Why is it so that no other platform needs more time, only this one? And which command, specifically, causes problem?
On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote: > John Stultz <john.stultz@linaro.org> writes: > > From: Yu Chen <chenyu56@huawei.com> > > > > It needs more time for the device controller to clear the CmdAct of > > DEPCMD on Hisilicon Kirin Soc. > > Why does it need more time? Why is it so that no other platform needs > more time, only this one? And which command, specifically, causes > problem? Hrm. Sadly I don't have that context (again I'm picking up a semi-abandoned patchset here), which is unfortunate, as I'm sure someone spent a number of hours debugging things to come up with this. :) But alas, I've dropped this for now in my stack, and things seem to be working ok so far. I suspect there's some edge case I'll run into, but hopefully I'll be able to debug and get more details when that happens. I do appreciate the review and pushback here! thanks -john
John Stultz <john.stultz@linaro.org> 于2019年10月30日周三 上午5:18写道： > > On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote: > > John Stultz <john.stultz@linaro.org> writes: > > > From: Yu Chen <chenyu56@huawei.com> > > > > > > It needs more time for the device controller to clear the CmdAct of > > > DEPCMD on Hisilicon Kirin Soc. > > > > Why does it need more time? Why is it so that no other platform needs > > more time, only this one? And which command, specifically, causes > > problem? Sorry for my back to this so late. This change is required on my dwc3 based HW too, I gave a check and the reason is suspend_clk is used in case the PIPE phy is at P3, this slow clock makes my EP command below timeout. dwc3_gadget_ep_cmd: ep0out: cmd 'Set Endpoint Configuration' [401] params 00001000 00000500 00000000 --> status: Timed Out Success case takes about 400us to complete, see below trace(44.286278 - 44.285897 = 0.000381): configfs_acm.sh-822 [000] d..1 44.285896: dwc3_writel: addr 000000006d59aae1 value 00000401 configfs_acm.sh-822 [000] d..1 44.285897: dwc3_readl: addr 000000006d59aae1 value 00000401 ... ... configfs_acm.sh-822 [000] d..1 44.286278: dwc3_readl: addr 000000006d59aae1 value 00000001 configfs_acm.sh-822 [000] d..1 44.286279: dwc3_gadget_ep_cmd: ep0out: cmd 'Set Endpoint Configuration' [401] params 00001000 00000500 00000000 --> status: Successful Hi John, Do you still have this problem? if yes, What's the value of USBLNKST[21:18] when the timeout happens? thanks Li Jun > > Hrm. Sadly I don't have that context (again I'm picking up a > semi-abandoned patchset here), which is unfortunate, as I'm sure > someone spent a number of hours debugging things to come up with this. > :) > > But alas, I've dropped this for now in my stack, and things seem to be > working ok so far. I suspect there's some edge case I'll run into, but > hopefully I'll be able to debug and get more details when that > happens. > > I do appreciate the review and pushback here! > > thanks > -john
On Wed, May 6, 2020 at 2:00 AM Jun Li <lijun.kernel@gmail.com> wrote: > John Stultz <john.stultz@linaro.org> 于2019年10月30日周三 上午5:18写道： > > On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote: > > > John Stultz <john.stultz@linaro.org> writes: > > > > From: Yu Chen <chenyu56@huawei.com> > > > > > > > > It needs more time for the device controller to clear the CmdAct of > > > > DEPCMD on Hisilicon Kirin Soc. > > > > > > Why does it need more time? Why is it so that no other platform needs > > > more time, only this one? And which command, specifically, causes > > > problem? > > Sorry for my back to this so late. > > This change is required on my dwc3 based HW too, I gave a check > and the reason is suspend_clk is used in case the PIPE phy is at P3, > this slow clock makes my EP command below timeout. > > dwc3_gadget_ep_cmd: ep0out: cmd 'Set Endpoint Configuration' [401] > params 00001000 00000500 00000000 --> status: Timed Out > > Success case takes about 400us to complete, see below trace(44.286278 > - 44.285897 = 0.000381): > > configfs_acm.sh-822 [000] d..1 44.285896: dwc3_writel: addr > 000000006d59aae1 value 00000401 > configfs_acm.sh-822 [000] d..1 44.285897: dwc3_readl: addr > 000000006d59aae1 value 00000401 > ... ... > configfs_acm.sh-822 [000] d..1 44.286278: dwc3_readl: addr > 000000006d59aae1 value 00000001 > configfs_acm.sh-822 [000] d..1 44.286279: dwc3_gadget_ep_cmd: > ep0out: cmd 'Set Endpoint Configuration' [401] params 00001000 > 00000500 00000000 --> status: Successful > > Hi John, > > Do you still have this problem? if yes, What's the value of > USBLNKST[21:18] when the timeout happens? Sorry. As I mentioned, I was working to upstream a patchset that I hadn't created, so the context I had was limited. As I couldn't reproduce an issue without the change on the device I had, I figured it would be best to drop it. However, as you have some analysis and rational for why such a change would be needed, I don't have an objection to it. Do you want to resubmit the patch with your explanation and detailed log above in the commit message? thanks -john
John Stultz <john.stultz@linaro.org> 于2020年5月7日周四 上午6:27写道： > > On Wed, May 6, 2020 at 2:00 AM Jun Li <lijun.kernel@gmail.com> wrote: > > John Stultz <john.stultz@linaro.org> 于2019年10月30日周三 上午5:18写道： > > > On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote: > > > > John Stultz <john.stultz@linaro.org> writes: > > > > > From: Yu Chen <chenyu56@huawei.com> > > > > > > > > > > It needs more time for the device controller to clear the CmdAct of > > > > > DEPCMD on Hisilicon Kirin Soc. > > > > > > > > Why does it need more time? Why is it so that no other platform needs > > > > more time, only this one? And which command, specifically, causes > > > > problem? > > > > Sorry for my back to this so late. > > > > This change is required on my dwc3 based HW too, I gave a check > > and the reason is suspend_clk is used in case the PIPE phy is at P3, > > this slow clock makes my EP command below timeout. > > > > dwc3_gadget_ep_cmd: ep0out: cmd 'Set Endpoint Configuration' [401] > > params 00001000 00000500 00000000 --> status: Timed Out > > > > Success case takes about 400us to complete, see below trace(44.286278 > > - 44.285897 = 0.000381): > > > > configfs_acm.sh-822 [000] d..1 44.285896: dwc3_writel: addr > > 000000006d59aae1 value 00000401 > > configfs_acm.sh-822 [000] d..1 44.285897: dwc3_readl: addr > > 000000006d59aae1 value 00000401 > > ... ... > > configfs_acm.sh-822 [000] d..1 44.286278: dwc3_readl: addr > > 000000006d59aae1 value 00000001 > > configfs_acm.sh-822 [000] d..1 44.286279: dwc3_gadget_ep_cmd: > > ep0out: cmd 'Set Endpoint Configuration' [401] params 00001000 > > 00000500 00000000 --> status: Successful > > > > Hi John, > > > > Do you still have this problem? if yes, What's the value of > > USBLNKST[21:18] when the timeout happens? > > Sorry. As I mentioned, I was working to upstream a patchset that I > hadn't created, so the context I had was limited. As I couldn't > reproduce an issue without the change on the device I had, I figured > it would be best to drop it. That was fine. > > However, as you have some analysis and rational for why such a change > would be needed, I don't have an objection to it. Do you want to > resubmit the patch with your explanation and detailed log above in the > commit message? Sure, I will resubmit the patch with my explanation added in commit message. thanks Li Jun > > thanks > -john
Jun Li <lijun.kernel@gmail.com> 于2020年5月7日周四 上午11:08写道： > > John Stultz <john.stultz@linaro.org> 于2020年5月7日周四 上午6:27写道： > > > > On Wed, May 6, 2020 at 2:00 AM Jun Li <lijun.kernel@gmail.com> wrote: > > > John Stultz <john.stultz@linaro.org> 于2019年10月30日周三 上午5:18写道： > > > > On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote: > > > > > John Stultz <john.stultz@linaro.org> writes: > > > > > > From: Yu Chen <chenyu56@huawei.com> > > > > > > > > > > > > It needs more time for the device controller to clear the CmdAct of > > > > > > DEPCMD on Hisilicon Kirin Soc. > > > > > > > > > > Why does it need more time? Why is it so that no other platform needs > > > > > more time, only this one? And which command, specifically, causes > > > > > problem? > > > > > > Sorry for my back to this so late. > > > > > > This change is required on my dwc3 based HW too, I gave a check > > > and the reason is suspend_clk is used in case the PIPE phy is at P3, > > > this slow clock makes my EP command below timeout. > > > > > > dwc3_gadget_ep_cmd: ep0out: cmd 'Set Endpoint Configuration' [401] > > > params 00001000 00000500 00000000 --> status: Timed Out > > > > > > Success case takes about 400us to complete, see below trace(44.286278 > > > - 44.285897 = 0.000381): > > > > > > configfs_acm.sh-822 [000] d..1 44.285896: dwc3_writel: addr > > > 000000006d59aae1 value 00000401 > > > configfs_acm.sh-822 [000] d..1 44.285897: dwc3_readl: addr > > > 000000006d59aae1 value 00000401 > > > ... ... > > > configfs_acm.sh-822 [000] d..1 44.286278: dwc3_readl: addr > > > 000000006d59aae1 value 00000001 > > > configfs_acm.sh-822 [000] d..1 44.286279: dwc3_gadget_ep_cmd: > > > ep0out: cmd 'Set Endpoint Configuration' [401] params 00001000 > > > 00000500 00000000 --> status: Successful > > > > > > Hi John, > > > > > > Do you still have this problem? if yes, What's the value of > > > USBLNKST[21:18] when the timeout happens? > > > > Sorry. As I mentioned, I was working to upstream a patchset that I > > hadn't created, so the context I had was limited. As I couldn't > > reproduce an issue without the change on the device I had, I figured > > it would be best to drop it. > > That was fine. > > > > However, as you have some analysis and rational for why such a change > > would be needed, I don't have an objection to it. Do you want to > > resubmit the patch with your explanation and detailed log above in the > > commit message? > > Sure, I will resubmit the patch with my explanation added in commit message. Hi John A second think of this, I feel use readl_poll_timeout_atomic() to wait by time is more proper here, so I create a new patch to address this also other registers polling, see below patch with you CCed: https://patchwork.kernel.org/patch/11536081/ thanks Li Jun > > thanks > Li Jun > > > > thanks > > -john
Hi, Jun Li <lijun.kernel@gmail.com> writes: > John Stultz <john.stultz@linaro.org> 于2019年10月30日周三 上午5:18写道： >> >> On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote: >> > John Stultz <john.stultz@linaro.org> writes: >> > > From: Yu Chen <chenyu56@huawei.com> >> > > >> > > It needs more time for the device controller to clear the CmdAct of >> > > DEPCMD on Hisilicon Kirin Soc. >> > >> > Why does it need more time? Why is it so that no other platform needs >> > more time, only this one? And which command, specifically, causes >> > problem? > > Sorry for my back to this so late. > > This change is required on my dwc3 based HW too, I gave a check > and the reason is suspend_clk is used in case the PIPE phy is at P3, > this slow clock makes my EP command below timeout. The phy needs to woken up before the command is triggered. Currently we only wake up the HS PHY. Does it help you if we wake up the SS phy as well? Something like below ought to do it: diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index a0555252dee0..ee46c2dacaeb 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -274,7 +274,8 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd, const struct usb_endpoint_descriptor *desc = dep->endpoint.desc; struct dwc3 *dwc = dep->dwc; u32 timeout = 1000; - u32 saved_config = 0; + u32 saved_hs_config = 0; + u32 saved_ss_config = 0; u32 reg; int cmd_status = 0; @@ -293,19 +294,28 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd, if (dwc->gadget.speed <= USB_SPEED_HIGH) { reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) { - saved_config |= DWC3_GUSB2PHYCFG_SUSPHY; + saved_hs_config |= DWC3_GUSB2PHYCFG_SUSPHY; reg &= ~DWC3_GUSB2PHYCFG_SUSPHY; } if (reg & DWC3_GUSB2PHYCFG_ENBLSLPM) { - saved_config |= DWC3_GUSB2PHYCFG_ENBLSLPM; + saved_hs_config |= DWC3_GUSB2PHYCFG_ENBLSLPM; reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM; } - if (saved_config) + if (saved_hs_config) dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); } + reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); + if (unlikely(reg & DWC3_GUSB3PIPECTL_SUSPHY)) { + saved_ss_config |= DWC3_GUSB3PIPECTL_SUSPHY; + reg &= ~DWC3_GUSB3PIPECTL_SUSPHY; + } + + if (saved_ss_config) + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); + if (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_STARTTRANSFER) { int needs_wakeup; @@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd, dwc3_gadget_ep_get_transfer_index(dep); } - if (saved_config) { + if (saved_hs_config) { reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); - reg |= saved_config; + reg |= saved_hs_config; dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); } + if (saved_ss_config) { + reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); + reg |= saved_ss_config; + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); + } + return ret; }
Hi, Jun Li <lijun.kernel@gmail.com> writes: > Jun Li <lijun.kernel@gmail.com> 于2020年5月7日周四 上午11:08写道： >> >> John Stultz <john.stultz@linaro.org> 于2020年5月7日周四 上午6:27写道： >> > >> > On Wed, May 6, 2020 at 2:00 AM Jun Li <lijun.kernel@gmail.com> wrote: >> > > John Stultz <john.stultz@linaro.org> 于2019年10月30日周三 上午5:18写道： >> > > > On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote: >> > > > > John Stultz <john.stultz@linaro.org> writes: >> > > > > > From: Yu Chen <chenyu56@huawei.com> >> > > > > > >> > > > > > It needs more time for the device controller to clear the CmdAct of >> > > > > > DEPCMD on Hisilicon Kirin Soc. >> > > > > >> > > > > Why does it need more time? Why is it so that no other platform needs >> > > > > more time, only this one? And which command, specifically, causes >> > > > > problem? >> > > >> > > Sorry for my back to this so late. >> > > >> > > This change is required on my dwc3 based HW too, I gave a check >> > > and the reason is suspend_clk is used in case the PIPE phy is at P3, >> > > this slow clock makes my EP command below timeout. >> > > >> > > dwc3_gadget_ep_cmd: ep0out: cmd 'Set Endpoint Configuration' [401] >> > > params 00001000 00000500 00000000 --> status: Timed Out >> > > >> > > Success case takes about 400us to complete, see below trace(44.286278 >> > > - 44.285897 = 0.000381): >> > > >> > > configfs_acm.sh-822 [000] d..1 44.285896: dwc3_writel: addr >> > > 000000006d59aae1 value 00000401 >> > > configfs_acm.sh-822 [000] d..1 44.285897: dwc3_readl: addr >> > > 000000006d59aae1 value 00000401 >> > > ... ... >> > > configfs_acm.sh-822 [000] d..1 44.286278: dwc3_readl: addr >> > > 000000006d59aae1 value 00000001 >> > > configfs_acm.sh-822 [000] d..1 44.286279: dwc3_gadget_ep_cmd: >> > > ep0out: cmd 'Set Endpoint Configuration' [401] params 00001000 >> > > 00000500 00000000 --> status: Successful >> > > >> > > Hi John, >> > > >> > > Do you still have this problem? if yes, What's the value of >> > > USBLNKST[21:18] when the timeout happens? >> > >> > Sorry. As I mentioned, I was working to upstream a patchset that I >> > hadn't created, so the context I had was limited. As I couldn't >> > reproduce an issue without the change on the device I had, I figured >> > it would be best to drop it. >> >> That was fine. >> > >> > However, as you have some analysis and rational for why such a change >> > would be needed, I don't have an objection to it. Do you want to >> > resubmit the patch with your explanation and detailed log above in the >> > commit message? >> >> Sure, I will resubmit the patch with my explanation added in commit message. > > Hi John > > A second think of this, I feel use readl_poll_timeout_atomic() to wait by time > is more proper here, so I create a new patch to address this also other > registers polling, see below patch with you CCed: > > https://patchwork.kernel.org/patch/11536081/ Fixing a bug has nothing to do with using readl_poll_timeout_atomic(). Please don't mix things as it just makes review time consuming. Let's find out what the bug is all about, only then should we consider moving over to readl_poll_timeout_atomic().
Hi, Felipe Balbi <balbi@kernel.org> 于2020年5月8日周五 下午8:33写道： > > > Hi, > > Jun Li <lijun.kernel@gmail.com> writes: > > John Stultz <john.stultz@linaro.org> 于2019年10月30日周三 上午5:18写道： > >> > >> On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote: > >> > John Stultz <john.stultz@linaro.org> writes: > >> > > From: Yu Chen <chenyu56@huawei.com> > >> > > > >> > > It needs more time for the device controller to clear the CmdAct of > >> > > DEPCMD on Hisilicon Kirin Soc. > >> > > >> > Why does it need more time? Why is it so that no other platform needs > >> > more time, only this one? And which command, specifically, causes > >> > problem? > > > > Sorry for my back to this so late. > > > > This change is required on my dwc3 based HW too, I gave a check > > and the reason is suspend_clk is used in case the PIPE phy is at P3, > > this slow clock makes my EP command below timeout. > > The phy needs to woken up before the command is triggered. Currently we > only wake up the HS PHY. Does it help you if we wake up the SS phy as > well? > > Something like below ought to do it: > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index a0555252dee0..ee46c2dacaeb 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -274,7 +274,8 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd, > const struct usb_endpoint_descriptor *desc = dep->endpoint.desc; > struct dwc3 *dwc = dep->dwc; > u32 timeout = 1000; > - u32 saved_config = 0; > + u32 saved_hs_config = 0; > + u32 saved_ss_config = 0; > u32 reg; > > int cmd_status = 0; > @@ -293,19 +294,28 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd, > if (dwc->gadget.speed <= USB_SPEED_HIGH) { > reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); > if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) { > - saved_config |= DWC3_GUSB2PHYCFG_SUSPHY; > + saved_hs_config |= DWC3_GUSB2PHYCFG_SUSPHY; > reg &= ~DWC3_GUSB2PHYCFG_SUSPHY; > } > > if (reg & DWC3_GUSB2PHYCFG_ENBLSLPM) { > - saved_config |= DWC3_GUSB2PHYCFG_ENBLSLPM; > + saved_hs_config |= DWC3_GUSB2PHYCFG_ENBLSLPM; > reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM; > } > > - if (saved_config) > + if (saved_hs_config) > dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); > } > > + reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); > + if (unlikely(reg & DWC3_GUSB3PIPECTL_SUSPHY)) { > + saved_ss_config |= DWC3_GUSB3PIPECTL_SUSPHY; > + reg &= ~DWC3_GUSB3PIPECTL_SUSPHY; > + } > + > + if (saved_ss_config) > + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); > + > if (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_STARTTRANSFER) { > int needs_wakeup; > > @@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd, > dwc3_gadget_ep_get_transfer_index(dep); > } > > - if (saved_config) { > + if (saved_hs_config) { > reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); > - reg |= saved_config; > + reg |= saved_hs_config; > dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); > } > > + if (saved_ss_config) { > + reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); > + reg |= saved_ss_config; > + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); > + } > + > return ret; > } Unfortunately this way can't work, once the SS PHY enters P3, disable suspend_en can't force SS PHY exit P3, unless do this at the very beginning to prevent SS PHY entering P3(e.g. add "snps,dis_u3_susphy_quirk" for test). thanks Li Jun > > -- > balbi
Felipe Balbi <balbi@kernel.org> 于2020年5月8日周五 下午8:35写道： > > > Hi, > > Jun Li <lijun.kernel@gmail.com> writes: > > Jun Li <lijun.kernel@gmail.com> 于2020年5月7日周四 上午11:08写道： > >> > >> John Stultz <john.stultz@linaro.org> 于2020年5月7日周四 上午6:27写道： > >> > > >> > On Wed, May 6, 2020 at 2:00 AM Jun Li <lijun.kernel@gmail.com> wrote: > >> > > John Stultz <john.stultz@linaro.org> 于2019年10月30日周三 上午5:18写道： > >> > > > On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote: > >> > > > > John Stultz <john.stultz@linaro.org> writes: > >> > > > > > From: Yu Chen <chenyu56@huawei.com> > >> > > > > > > >> > > > > > It needs more time for the device controller to clear the CmdAct of > >> > > > > > DEPCMD on Hisilicon Kirin Soc. > >> > > > > > >> > > > > Why does it need more time? Why is it so that no other platform needs > >> > > > > more time, only this one? And which command, specifically, causes > >> > > > > problem? > >> > > > >> > > Sorry for my back to this so late. > >> > > > >> > > This change is required on my dwc3 based HW too, I gave a check > >> > > and the reason is suspend_clk is used in case the PIPE phy is at P3, > >> > > this slow clock makes my EP command below timeout. > >> > > > >> > > dwc3_gadget_ep_cmd: ep0out: cmd 'Set Endpoint Configuration' [401] > >> > > params 00001000 00000500 00000000 --> status: Timed Out > >> > > > >> > > Success case takes about 400us to complete, see below trace(44.286278 > >> > > - 44.285897 = 0.000381): > >> > > > >> > > configfs_acm.sh-822 [000] d..1 44.285896: dwc3_writel: addr > >> > > 000000006d59aae1 value 00000401 > >> > > configfs_acm.sh-822 [000] d..1 44.285897: dwc3_readl: addr > >> > > 000000006d59aae1 value 00000401 > >> > > ... ... > >> > > configfs_acm.sh-822 [000] d..1 44.286278: dwc3_readl: addr > >> > > 000000006d59aae1 value 00000001 > >> > > configfs_acm.sh-822 [000] d..1 44.286279: dwc3_gadget_ep_cmd: > >> > > ep0out: cmd 'Set Endpoint Configuration' [401] params 00001000 > >> > > 00000500 00000000 --> status: Successful > >> > > > >> > > Hi John, > >> > > > >> > > Do you still have this problem? if yes, What's the value of > >> > > USBLNKST[21:18] when the timeout happens? > >> > > >> > Sorry. As I mentioned, I was working to upstream a patchset that I > >> > hadn't created, so the context I had was limited. As I couldn't > >> > reproduce an issue without the change on the device I had, I figured > >> > it would be best to drop it. > >> > >> That was fine. > >> > > >> > However, as you have some analysis and rational for why such a change > >> > would be needed, I don't have an objection to it. Do you want to > >> > resubmit the patch with your explanation and detailed log above in the > >> > commit message? > >> > >> Sure, I will resubmit the patch with my explanation added in commit message. > > > > Hi John > > > > A second think of this, I feel use readl_poll_timeout_atomic() to wait by time > > is more proper here, so I create a new patch to address this also other > > registers polling, see below patch with you CCed: > > > > https://patchwork.kernel.org/patch/11536081/ > > Fixing a bug has nothing to do with using > readl_poll_timeout_atomic(). Please don't mix things as it just makes > review time consuming. > > Let's find out what the bug is all about, only then should we consider > moving over to readl_poll_timeout_atomic(). Agreed, sorry about that, I will hold on my readl_poll_timeout_atomic() changes until we have a conclusion on this issue fix. thanks Li Jun > > -- > balbi
Hi, Jun Li <lijun.kernel@gmail.com> writes: >> @@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd, >> dwc3_gadget_ep_get_transfer_index(dep); >> } >> >> - if (saved_config) { >> + if (saved_hs_config) { >> reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); >> - reg |= saved_config; >> + reg |= saved_hs_config; >> dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); >> } >> >> + if (saved_ss_config) { >> + reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); >> + reg |= saved_ss_config; >> + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); >> + } >> + >> return ret; >> } > > Unfortunately this way can't work, once the SS PHY enters P3, disable > suspend_en can't force SS PHY exit P3, unless do this at the very beginning > to prevent SS PHY entering P3(e.g. add "snps,dis_u3_susphy_quirk" for test). It sounds like you have a quirky PHY. If that's the case, then you probably need to use the flag you mentioned above. Please verify with that.
> -----Original Message----- > From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi > Sent: 2020年5月15日 17:31 > To: Jun Li <lijun.kernel@gmail.com> > Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu > Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob > Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee > <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; > Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun > <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko > <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; > Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open > list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; > Peter Chen <peter.chen@nxp.com>; Jun Li <jun.li@nxp.com>; Thinh Nguyen > <Thinh.Nguyen@synopsys.com> > Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device > controller > > > Hi, > > Jun Li <lijun.kernel@gmail.com> writes: > >> @@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned > cmd, > >> dwc3_gadget_ep_get_transfer_index(dep); > >> } > >> > >> - if (saved_config) { > >> + if (saved_hs_config) { > >> reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); > >> - reg |= saved_config; > >> + reg |= saved_hs_config; > >> dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); > >> } > >> > >> + if (saved_ss_config) { > >> + reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); > >> + reg |= saved_ss_config; > >> + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); > >> + } > >> + > >> return ret; > >> } > > > > Unfortunately this way can't work, once the SS PHY enters P3, disable > > suspend_en can't force SS PHY exit P3, unless do this at the very > > beginning to prevent SS PHY entering P3(e.g. add "snps,dis_u3_susphy_quirk" for > test). > > It sounds like you have a quirky PHY. From what I got from the IC design, the behavior of DWC3_GUSB3PIPECTL_SUSPHY bit should be as what I said, not a quirky. Hi Thinh, could you comment this? > If that's the case, then you probably need > to use the flag you mentioned above. Please verify with that. With quirk of "snps,dis_u3_susphy_quirk", I had verified it can resolve the problem, but this will make USB3 Super Speed PHY never enter P3, this is a huge impact on USB power consumption. The timeout increase has no impact on those platforms which have no this problem, but can give chance for platform with very low supspend clk(like my case 32k) to work. Thanks Li Jun > > -- > balbi
Hi, Jun Li <jun.li@nxp.com> writes: >> Jun Li <lijun.kernel@gmail.com> writes: >> >> @@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned >> cmd, >> >> dwc3_gadget_ep_get_transfer_index(dep); >> >> } >> >> >> >> - if (saved_config) { >> >> + if (saved_hs_config) { >> >> reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); >> >> - reg |= saved_config; >> >> + reg |= saved_hs_config; >> >> dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); >> >> } >> >> >> >> + if (saved_ss_config) { >> >> + reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); >> >> + reg |= saved_ss_config; >> >> + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); >> >> + } >> >> + >> >> return ret; >> >> } >> > >> > Unfortunately this way can't work, once the SS PHY enters P3, disable >> > suspend_en can't force SS PHY exit P3, unless do this at the very >> > beginning to prevent SS PHY entering P3(e.g. add "snps,dis_u3_susphy_quirk" for >> test). >> >> It sounds like you have a quirky PHY. > > From what I got from the IC design, the behavior of DWC3_GUSB3PIPECTL_SUSPHY > bit should be as what I said, not a quirky. > > Hi Thinh, could you comment this? > >> If that's the case, then you probably need >> to use the flag you mentioned above. Please verify with that. > > With quirk of "snps,dis_u3_susphy_quirk", I had verified it can > resolve the problem, but this will make USB3 Super Speed PHY > never enter P3, this is a huge impact on USB power consumption. > > The timeout increase has no impact on those platforms which have > no this problem, but can give chance for platform with very low > supspend clk(like my case 32k) to work. I was under the impression that issuing a command would wake the PHY up. I don't have access to DWC3 documentation to verify, but that's as I remember. Is that not the case?
Hi, Jun Li wrote: >> -----Original Message----- >> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi >> Sent: 2020年5月15日 17:31 >> To: Jun Li <lijun.kernel@gmail.com> >> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu >> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob >> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee >> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; >> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun >> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko >> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; >> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open >> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; >> Peter Chen <peter.chen@nxp.com>; Jun Li <jun.li@nxp.com>; Thinh Nguyen >> <Thinh.Nguyen@synopsys.com> >> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device >> controller >> >> >> Hi, >> >> Jun Li <lijun.kernel@gmail.com> writes: >>>> @@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned >> cmd, >>>> dwc3_gadget_ep_get_transfer_index(dep); >>>> } >>>> >>>> - if (saved_config) { >>>> + if (saved_hs_config) { >>>> reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); >>>> - reg |= saved_config; >>>> + reg |= saved_hs_config; >>>> dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); >>>> } >>>> >>>> + if (saved_ss_config) { >>>> + reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); >>>> + reg |= saved_ss_config; >>>> + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); >>>> + } >>>> + >>>> return ret; >>>> } >>> Unfortunately this way can't work, once the SS PHY enters P3, disable >>> suspend_en can't force SS PHY exit P3, unless do this at the very >>> beginning to prevent SS PHY entering P3(e.g. add "snps,dis_u3_susphy_quirk" for >> test). >> >> It sounds like you have a quirky PHY. > From what I got from the IC design, the behavior of DWC3_GUSB3PIPECTL_SUSPHY > bit should be as what I said, not a quirky. > > 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. BR, Thinh
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > Jun Li wrote: >>> -----Original Message----- >>> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi >>> Sent: 2020年5月15日 17:31 >>> To: Jun Li <lijun.kernel@gmail.com> >>> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu >>> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob >>> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee >>> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; >>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun >>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko >>> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; >>> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open >>> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; >>> Peter Chen <peter.chen@nxp.com>; Jun Li <jun.li@nxp.com>; Thinh Nguyen >>> <Thinh.Nguyen@synopsys.com> >>> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device >>> controller >>> >>> >>> Hi, >>> >>> Jun Li <lijun.kernel@gmail.com> writes: >>>>> @@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned >>> cmd, >>>>> dwc3_gadget_ep_get_transfer_index(dep); >>>>> } >>>>> >>>>> - if (saved_config) { >>>>> + if (saved_hs_config) { >>>>> reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); >>>>> - reg |= saved_config; >>>>> + reg |= saved_hs_config; >>>>> dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); >>>>> } >>>>> >>>>> + if (saved_ss_config) { >>>>> + reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); >>>>> + reg |= saved_ss_config; >>>>> + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); >>>>> + } >>>>> + >>>>> return ret; >>>>> } >>>> Unfortunately this way can't work, once the SS PHY enters P3, disable >>>> suspend_en can't force SS PHY exit P3, unless do this at the very >>>> beginning to prevent SS PHY entering P3(e.g. add "snps,dis_u3_susphy_quirk" for >>> test). >>> >>> It sounds like you have a quirky PHY. >> From what I got from the IC design, the behavior of DWC3_GUSB3PIPECTL_SUSPHY >> bit should be as what I said, not a quirky. >> >> 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?
Hi, > -----Original Message----- > From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi > Sent: 2020年5月16日 15:13 > To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>; Jun Li <jun.li@nxp.com>; Jun Li > <lijun.kernel@gmail.com> > Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu > Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob > Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee > <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; > Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun > <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko > <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; > Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open > list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; > Peter Chen <peter.chen@nxp.com>; Thinh Nguyen <Thinh.Nguyen@synopsys.com> > Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device > controller > > > Hi, > > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > > Jun Li wrote: > >>> -----Original Message----- > >>> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi > >>> Sent: 2020年5月15日 17:31 > >>> To: Jun Li <lijun.kernel@gmail.com> > >>> Cc: John Stultz <john.stultz@linaro.org>; lkml > >>> <linux-kernel@vger.kernel.org>; Yu Chen <chenyu56@huawei.com>; Greg > >>> Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring > >>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan > >>> Lee <shufan_lee@richtek.com>; Heikki Krogerus > >>> <heikki.krogerus@linux.intel.com>; > >>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun > >>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; > >>> Andy Shevchenko <andy.shevchenko@gmail.com>; Valentin Schneider > >>> <valentin.schneider@arm.com>; Jack Pham <jackp@codeaurora.org>; > >>> Linux USB List <linux-usb@vger.kernel.org>; open list:OPEN FIRMWARE > >>> AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; > >>> Peter Chen <peter.chen@nxp.com>; Jun Li <jun.li@nxp.com>; Thinh > >>> Nguyen <Thinh.Nguyen@synopsys.com> > >>> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct > >>> cleared by device controller > >>> > >>> > >>> Hi, > >>> > >>> Jun Li <lijun.kernel@gmail.com> writes: > >>>>> @@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep > >>>>> *dep, unsigned > >>> cmd, > >>>>> dwc3_gadget_ep_get_transfer_index(dep); > >>>>> } > >>>>> > >>>>> - if (saved_config) { > >>>>> + if (saved_hs_config) { > >>>>> reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); > >>>>> - reg |= saved_config; > >>>>> + reg |= saved_hs_config; > >>>>> dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); > >>>>> } > >>>>> > >>>>> + if (saved_ss_config) { > >>>>> + reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); > >>>>> + reg |= saved_ss_config; > >>>>> + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); > >>>>> + } > >>>>> + > >>>>> return ret; > >>>>> } > >>>> Unfortunately this way can't work, once the SS PHY enters P3, > >>>> disable suspend_en can't force SS PHY exit P3, unless do this at > >>>> the very beginning to prevent SS PHY entering P3(e.g. add > >>>> "snps,dis_u3_susphy_quirk" for > >>> test). > >>> > >>> It sounds like you have a quirky PHY. > >> From what I got from the IC design, the behavior of > >> DWC3_GUSB3PIPECTL_SUSPHY bit should be as what I said, not a quirky. > >> > >> 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" Li Jun > > -- > balbi
Hi, Jun Li <jun.li@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? Then we can just set the time to that upper bound.
> -----Original Message----- > From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi > Sent: 2020年5月16日 19:57 > To: Jun Li <jun.li@nxp.com>; Thinh Nguyen <Thinh.Nguyen@synopsys.com>; Jun Li > <lijun.kernel@gmail.com> > Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu > Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob > Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee > <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; > Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun > <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko > <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; > Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open > list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; > Peter Chen <peter.chen@nxp.com>; Thinh Nguyen <Thinh.Nguyen@synopsys.com> > Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device > controller > > > Hi, > > Jun Li <jun.li@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. > Then we can just set the time to that upper bound. Per my test with trace, the time is about 400us(~13 cycles). Thanks Li Jun > > -- > balbi
Jun Li wrote: >> -----Original Message----- >> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi >> Sent: 2020年5月16日 19:57 >> To: Jun Li <jun.li@nxp.com>; Thinh Nguyen <Thinh.Nguyen@synopsys.com>; Jun Li >> <lijun.kernel@gmail.com> >> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu >> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob >> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee >> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; >> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun >> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko >> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; >> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open >> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; >> Peter Chen <peter.chen@nxp.com>; Thinh Nguyen <Thinh.Nguyen@synopsys.com> >> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device >> controller >> >> >> Hi, >> >> Jun Li <jun.li@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. BR, Thinh > >> Then we can just set the time to that upper bound. > Per my test with trace, the time is about 400us(~13 cycles). > > Thanks > Li Jun >> -- >> balbi
Thinh Nguyen wrote: > Jun Li wrote: >>> -----Original Message----- >>> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi >>> Sent: 2020年5月16日 19:57 >>> To: Jun Li <jun.li@nxp.com>; Thinh Nguyen <Thinh.Nguyen@synopsys.com>; Jun Li >>> <lijun.kernel@gmail.com> >>> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu >>> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob >>> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee >>> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; >>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun >>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko >>> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; >>> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open >>> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; >>> Peter Chen <peter.chen@nxp.com>; Thinh Nguyen <Thinh.Nguyen@synopsys.com> >>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device >>> controller >>> >>> >>> Hi, >>> >>> Jun Li <jun.li@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]? BR, Thinh
Hi > -----Original Message----- > From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > Sent: 2020年5月19日 14:46 > To: Jun Li <jun.li@nxp.com>; Felipe Balbi <balbi@kernel.org>; Jun Li > <lijun.kernel@gmail.com> > Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu > Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob > Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee > <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; > Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun > <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko > <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; > Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open > list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; > Peter Chen <peter.chen@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 <balbif@gmail.com> On Behalf Of Felipe Balbi > >>> Sent: 2020年5月16日 19:57 > >>> To: Jun Li <jun.li@nxp.com>; Thinh Nguyen > >>> <Thinh.Nguyen@synopsys.com>; Jun Li <lijun.kernel@gmail.com> > >>> Cc: John Stultz <john.stultz@linaro.org>; lkml > >>> <linux-kernel@vger.kernel.org>; Yu Chen <chenyu56@huawei.com>; Greg > >>> Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring > >>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan > >>> Lee <shufan_lee@richtek.com>; Heikki Krogerus > >>> <heikki.krogerus@linux.intel.com>; > >>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun > >>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; > >>> Andy Shevchenko <andy.shevchenko@gmail.com>; Valentin Schneider > >>> <valentin.schneider@arm.com>; Jack Pham <jackp@codeaurora.org>; > >>> Linux USB List <linux-usb@vger.kernel.org>; open list:OPEN FIRMWARE > >>> AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; > >>> Peter Chen <peter.chen@nxp.com>; Thinh Nguyen > >>> <Thinh.Nguyen@synopsys.com> > >>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct > >>> cleared by device controller > >>> > >>> > >>> Hi, > >>> > >>> Jun Li <jun.li@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 > > BR, > Thinh
Jun Li wrote: > Hi > >> -----Original Message----- >> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >> Sent: 2020年5月19日 14:46 >> To: Jun Li <jun.li@nxp.com>; Felipe Balbi <balbi@kernel.org>; Jun Li >> <lijun.kernel@gmail.com> >> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu >> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob >> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee >> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; >> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun >> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko >> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; >> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open >> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; >> Peter Chen <peter.chen@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 <balbif@gmail.com> On Behalf Of Felipe Balbi >>>>> Sent: 2020年5月16日 19:57 >>>>> To: Jun Li <jun.li@nxp.com>; Thinh Nguyen >>>>> <Thinh.Nguyen@synopsys.com>; Jun Li <lijun.kernel@gmail.com> >>>>> Cc: John Stultz <john.stultz@linaro.org>; lkml >>>>> <linux-kernel@vger.kernel.org>; Yu Chen <chenyu56@huawei.com>; Greg >>>>> Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring >>>>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan >>>>> Lee <shufan_lee@richtek.com>; Heikki Krogerus >>>>> <heikki.krogerus@linux.intel.com>; >>>>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun >>>>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; >>>>> Andy Shevchenko <andy.shevchenko@gmail.com>; Valentin Schneider >>>>> <valentin.schneider@arm.com>; Jack Pham <jackp@codeaurora.org>; >>>>> Linux USB List <linux-usb@vger.kernel.org>; open list:OPEN FIRMWARE >>>>> AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; >>>>> Peter Chen <peter.chen@nxp.com>; Thinh Nguyen >>>>> <Thinh.Nguyen@synopsys.com> >>>>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct >>>>> cleared by device controller >>>>> >>>>> >>>>> Hi, >>>>> >>>>> Jun Li <jun.li@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. 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. Thanks, Thinh
Thinh Nguyen wrote: > Jun Li wrote: >> Hi >> >>> -----Original Message----- >>> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >>> Sent: 2020年5月19日 14:46 >>> To: Jun Li <jun.li@nxp.com>; Felipe Balbi <balbi@kernel.org>; Jun Li >>> <lijun.kernel@gmail.com> >>> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu >>> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob >>> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee >>> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; >>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun >>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko >>> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; >>> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open >>> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; >>> Peter Chen <peter.chen@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 <balbif@gmail.com> On Behalf Of Felipe Balbi >>>>>> Sent: 2020年5月16日 19:57 >>>>>> To: Jun Li <jun.li@nxp.com>; Thinh Nguyen >>>>>> <Thinh.Nguyen@synopsys.com>; Jun Li <lijun.kernel@gmail.com> >>>>>> Cc: John Stultz <john.stultz@linaro.org>; lkml >>>>>> <linux-kernel@vger.kernel.org>; Yu Chen <chenyu56@huawei.com>; Greg >>>>>> Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring >>>>>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan >>>>>> Lee <shufan_lee@richtek.com>; Heikki Krogerus >>>>>> <heikki.krogerus@linux.intel.com>; >>>>>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun >>>>>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; >>>>>> Andy Shevchenko <andy.shevchenko@gmail.com>; Valentin Schneider >>>>>> <valentin.schneider@arm.com>; Jack Pham <jackp@codeaurora.org>; >>>>>> Linux USB List <linux-usb@vger.kernel.org>; open list:OPEN FIRMWARE >>>>>> AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; >>>>>> Peter Chen <peter.chen@nxp.com>; Thinh Nguyen >>>>>> <Thinh.Nguyen@synopsys.com> >>>>>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct >>>>>> cleared by device controller >>>>>> >>>>>> >>>>>> Hi, >>>>>> >>>>>> Jun Li <jun.li@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. > 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. BR, Thinh
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>>>>>>> "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. > >> 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. A whole ms waiting for a command to complete? Wow, that's a lot of time blocking the CPU. It looks like, perhaps, we should move to command completion interrupts. The difficulty here is that we issue commands from within the interrupt handler and, as such, can't wait_for_completion(). Meanwhile, we will take the timeout increase I guess, otherwise NXP won't have a working setup.
Hi Jun, Felipe Balbi <balbi@kernel.org> writes: >> 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. > > A whole ms waiting for a command to complete? Wow, that's a lot of time > blocking the CPU. It looks like, perhaps, we should move to command > completion interrupts. The difficulty here is that we issue commands > from within the interrupt handler and, as such, can't > wait_for_completion(). > > Meanwhile, we will take the timeout increase I guess, otherwise NXP > won't have a working setup. patch 1 in this series doesn't apply to testing/next. Care to rebase and resend? Thank you
Hi Thinh, > -----Original Message----- > From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > Sent: 2020年5月21日 9:56 > To: Jun Li <jun.li@nxp.com>; Felipe Balbi <balbi@kernel.org>; Jun Li > <lijun.kernel@gmail.com> > Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu > Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob > Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee > <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; > Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun > <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko > <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; > Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open > list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; > Peter Chen <peter.chen@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.Nguyen@synopsys.com> > >>> Sent: 2020年5月19日 14:46 > >>> To: Jun Li <jun.li@nxp.com>; Felipe Balbi <balbi@kernel.org>; Jun Li > >>> <lijun.kernel@gmail.com> > >>> Cc: John Stultz <john.stultz@linaro.org>; lkml > >>> <linux-kernel@vger.kernel.org>; Yu Chen <chenyu56@huawei.com>; Greg > >>> Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring > >>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan > >>> Lee <shufan_lee@richtek.com>; Heikki Krogerus > >>> <heikki.krogerus@linux.intel.com>; > >>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun > >>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; > >>> Andy Shevchenko <andy.shevchenko@gmail.com>; Valentin Schneider > >>> <valentin.schneider@arm.com>; Jack Pham <jackp@codeaurora.org>; > >>> Linux USB List <linux-usb@vger.kernel.org>; open list:OPEN FIRMWARE > >>> AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; > >>> Peter Chen <peter.chen@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 <balbif@gmail.com> On Behalf Of Felipe Balbi > >>>>>> Sent: 2020年5月16日 19:57 > >>>>>> To: Jun Li <jun.li@nxp.com>; Thinh Nguyen > >>>>>> <Thinh.Nguyen@synopsys.com>; Jun Li <lijun.kernel@gmail.com> > >>>>>> Cc: John Stultz <john.stultz@linaro.org>; lkml > >>>>>> <linux-kernel@vger.kernel.org>; Yu Chen <chenyu56@huawei.com>; > >>>>>> Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring > >>>>>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan > >>>>>> Lee <shufan_lee@richtek.com>; Heikki Krogerus > >>>>>> <heikki.krogerus@linux.intel.com>; > >>>>>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun > >>>>>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; > >>>>>> Andy Shevchenko <andy.shevchenko@gmail.com>; Valentin Schneider > >>>>>> <valentin.schneider@arm.com>; Jack Pham <jackp@codeaurora.org>; > >>>>>> Linux USB List <linux-usb@vger.kernel.org>; open list:OPEN > >>>>>> FIRMWARE AND FLATTENED DEVICE TREE BINDINGS > >>>>>> <devicetree@vger.kernel.org>; Peter Chen <peter.chen@nxp.com>; > >>>>>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> > >>>>>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for > >>>>>> CmdAct cleared by device controller > >>>>>> > >>>>>> > >>>>>> Hi, > >>>>>> > >>>>>> Jun Li <jun.li@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
Hi Felipe, > -----Original Message----- > From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi > Sent: 2020年5月21日 14:23 > To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>; Jun Li <jun.li@nxp.com>; Jun Li > <lijun.kernel@gmail.com> > Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu > Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob > Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee > <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; > Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun > <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko > <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; > Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open > list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; > Peter Chen <peter.chen@nxp.com> > Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device > controller > > > Hi Jun, > > Felipe Balbi <balbi@kernel.org> writes: > >> 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. > > > > A whole ms waiting for a command to complete? Wow, that's a lot of > > time blocking the CPU. It looks like, perhaps, we should move to > > command completion interrupts. The difficulty here is that we issue > > commands from within the interrupt handler and, as such, can't > > wait_for_completion(). > > > > Meanwhile, we will take the timeout increase I guess, otherwise NXP > > won't have a working setup. > > patch 1 in this series doesn't apply to testing/next. Care to rebase and resend? Sure, I will rebase and resend this patch with timeout loop 5000. Thanks Li Jun > > Thank you > > -- > balbi
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 86dc1db788a9..168eb4a0a9b0 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -270,7 +270,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd, { const struct usb_endpoint_descriptor *desc = dep->endpoint.desc; struct dwc3 *dwc = dep->dwc; - u32 timeout = 1000; + u32 timeout = 5000; u32 saved_config = 0; u32 reg;