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

From: Jun Li <lijun.kernel@gmail.com>
To: Felipe Balbi <balbi@kernel.org>
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@nxp.com, Li Jun <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
Date: Sat, 9 May 2020 16:10:19 +0800	[thread overview]
Message-ID: <CAKgpwJXfWv5=MDqBCADhe2iXf6eiP0GQ13Bwo9fkuU5kGO7dsw@mail.gmail.com> (raw)
In-Reply-To: <87h7wqmwrv.fsf@kernel.org>

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