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

From: Jun Li
Date: Sat May 09 2020 - 04:10:34 EST


Hi,

Felipe Balbi <balbi@xxxxxxxxxx> ä2020å5æ8æåä äå8:33åéï
>
>
> Hi,
>
> Jun Li <lijun.kernel@xxxxxxxxx> writes:
> > John Stultz <john.stultz@xxxxxxxxxx> ä2019å10æ30æåä äå5:18åéï
> >>
> >> On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@xxxxxxxxxx> wrote:
> >> > John Stultz <john.stultz@xxxxxxxxxx> writes:
> >> > > From: Yu Chen <chenyu56@xxxxxxxxxx>
> >> > >
> >> > > 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