| Message ID | 20250131110832.438-1-selvarasu.g@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: dwc3: Fix timeout issue during controller enter/exit from halt state | expand |
On Fri, Jan 31, 2025, Selvarasu Ganesan wrote: > There is a frequent timeout during controller enter/exit from halt state > after toggling the run_stop bit by SW. This timeout occurs when > performing frequent role switches between host and device, causing > device enumeration issues due to the timeout. This issue was not present > when USB2 suspend PHY was disabled by passing the SNPS quirks > (snps,dis_u2_susphy_quirk and snps,dis_enblslpm_quirk) from the DTS. > However, there is a requirement to enable USB2 suspend PHY by setting of > GUSB2PHYCFG.ENBLSLPM and GUSB2PHYCFG.SUSPHY bits when controller starts > in gadget or host mode results in the timeout issue. > > This commit addresses this timeout issue by ensuring that the bits > GUSB2PHYCFG.ENBLSLPM and GUSB2PHYCFG.SUSPHY are cleared before starting > the dwc3_gadget_run_stop sequence and restoring them after the > dwc3_gadget_run_stop sequence is completed. > > Fixes: 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver") > Cc: stable@vger.kernel.org > Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com> > --- > drivers/usb/dwc3/gadget.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index d27af65eb08a..4a158f703d64 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2629,10 +2629,25 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on) > { > u32 reg; > u32 timeout = 2000; > + u32 saved_config = 0; > > if (pm_runtime_suspended(dwc->dev)) > return 0; > Can you add some comments here that this was added through experiment since it is not documented in the programming guide. It would be great to also note which platform you used to test this with. > + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); > + if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) { I know we have "unlikely" in the other check, but we should not need it here or the other place. Please remove this here. > + saved_config |= DWC3_GUSB2PHYCFG_SUSPHY; > + reg &= ~DWC3_GUSB2PHYCFG_SUSPHY; > + } > + > + if (reg & DWC3_GUSB2PHYCFG_ENBLSLPM) { > + saved_config |= DWC3_GUSB2PHYCFG_ENBLSLPM; > + reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM; > + } > + > + if (saved_config) > + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); > + > reg = dwc3_readl(dwc->regs, DWC3_DCTL); > if (is_on) { > if (DWC3_VER_IS_WITHIN(DWC3, ANY, 187A)) { > @@ -2660,6 +2675,12 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on) > reg &= DWC3_DSTS_DEVCTRLHLT; > } while (--timeout && !(!is_on ^ !reg)); > > + if (saved_config) { > + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); > + reg |= saved_config; > + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); > + } > + > if (!timeout) > return -ETIMEDOUT; > > -- > 2.17.1 > Thanks, Thinh
On 2/1/2025 5:45 AM, Thinh Nguyen wrote: > On Fri, Jan 31, 2025, Selvarasu Ganesan wrote: >> There is a frequent timeout during controller enter/exit from halt state >> after toggling the run_stop bit by SW. This timeout occurs when >> performing frequent role switches between host and device, causing >> device enumeration issues due to the timeout. This issue was not present >> when USB2 suspend PHY was disabled by passing the SNPS quirks >> (snps,dis_u2_susphy_quirk and snps,dis_enblslpm_quirk) from the DTS. >> However, there is a requirement to enable USB2 suspend PHY by setting of >> GUSB2PHYCFG.ENBLSLPM and GUSB2PHYCFG.SUSPHY bits when controller starts >> in gadget or host mode results in the timeout issue. >> >> This commit addresses this timeout issue by ensuring that the bits >> GUSB2PHYCFG.ENBLSLPM and GUSB2PHYCFG.SUSPHY are cleared before starting >> the dwc3_gadget_run_stop sequence and restoring them after the >> dwc3_gadget_run_stop sequence is completed. >> >> Fixes: 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver") >> Cc: stable@vger.kernel.org >> Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com> >> --- >> drivers/usb/dwc3/gadget.c | 21 +++++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index d27af65eb08a..4a158f703d64 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -2629,10 +2629,25 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on) >> { >> u32 reg; >> u32 timeout = 2000; >> + u32 saved_config = 0; >> >> if (pm_runtime_suspended(dwc->dev)) >> return 0; >> > Can you add some comments here that this was added through experiment > since it is not documented in the programming guide. It would be great > to also note which platform you used to test this with. > >> + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); >> + if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) { > I know we have "unlikely" in the other check, but we should not need it > here or the other place. Please remove this here. Hi Thinh, Thanks for your review comments. I updated below newer version with addressed your comments. Please review it. https://lore.kernel.org/linux-usb/20250201163903.459-1-selvarasu.g@samsung.com/ Thanks, Selva >> + saved_config |= DWC3_GUSB2PHYCFG_SUSPHY; >> + reg &= ~DWC3_GUSB2PHYCFG_SUSPHY; >> + } >> + >> + if (reg & DWC3_GUSB2PHYCFG_ENBLSLPM) { >> + saved_config |= DWC3_GUSB2PHYCFG_ENBLSLPM; >> + reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM; >> + } >> + >> + if (saved_config) >> + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); >> + >> reg = dwc3_readl(dwc->regs, DWC3_DCTL); >> if (is_on) { >> if (DWC3_VER_IS_WITHIN(DWC3, ANY, 187A)) { >> @@ -2660,6 +2675,12 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on) >> reg &= DWC3_DSTS_DEVCTRLHLT; >> } while (--timeout && !(!is_on ^ !reg)); >> >> + if (saved_config) { >> + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); >> + reg |= saved_config; >> + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); >> + } >> + >> if (!timeout) >> return -ETIMEDOUT; >> >> -- >> 2.17.1 >> > Thanks, > Thinh
On 2/1/2025 5:45 AM, Thinh Nguyen wrote: > On Fri, Jan 31, 2025, Selvarasu Ganesan wrote: >> There is a frequent timeout during controller enter/exit from halt state >> after toggling the run_stop bit by SW. This timeout occurs when >> performing frequent role switches between host and device, causing >> device enumeration issues due to the timeout. This issue was not present >> when USB2 suspend PHY was disabled by passing the SNPS quirks >> (snps,dis_u2_susphy_quirk and snps,dis_enblslpm_quirk) from the DTS. >> However, there is a requirement to enable USB2 suspend PHY by setting of >> GUSB2PHYCFG.ENBLSLPM and GUSB2PHYCFG.SUSPHY bits when controller starts >> in gadget or host mode results in the timeout issue. >> >> This commit addresses this timeout issue by ensuring that the bits >> GUSB2PHYCFG.ENBLSLPM and GUSB2PHYCFG.SUSPHY are cleared before starting >> the dwc3_gadget_run_stop sequence and restoring them after the >> dwc3_gadget_run_stop sequence is completed. >> >> Fixes: 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver") >> Cc: stable@vger.kernel.org >> Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com> >> --- >> drivers/usb/dwc3/gadget.c | 21 +++++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index d27af65eb08a..4a158f703d64 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -2629,10 +2629,25 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on) >> { >> u32 reg; >> u32 timeout = 2000; >> + u32 saved_config = 0; >> >> if (pm_runtime_suspended(dwc->dev)) >> return 0; >> > > Can you add some comments here that this was added through experiment > since it is not documented in the programming guide. It would be great > to also note which platform you used to test this with. > I did see this issue during pullup_exit() in SM6375 and SM8150 targets. The exact code logic worked out for me downstream. Regards, Krishna,
On Sat, Feb 01, 2025, Krishna Kurapati wrote: > > > On 2/1/2025 5:45 AM, Thinh Nguyen wrote: > > On Fri, Jan 31, 2025, Selvarasu Ganesan wrote: > > > There is a frequent timeout during controller enter/exit from halt state > > > after toggling the run_stop bit by SW. This timeout occurs when > > > performing frequent role switches between host and device, causing > > > device enumeration issues due to the timeout. This issue was not present > > > when USB2 suspend PHY was disabled by passing the SNPS quirks > > > (snps,dis_u2_susphy_quirk and snps,dis_enblslpm_quirk) from the DTS. > > > However, there is a requirement to enable USB2 suspend PHY by setting of > > > GUSB2PHYCFG.ENBLSLPM and GUSB2PHYCFG.SUSPHY bits when controller starts > > > in gadget or host mode results in the timeout issue. > > > > > > This commit addresses this timeout issue by ensuring that the bits > > > GUSB2PHYCFG.ENBLSLPM and GUSB2PHYCFG.SUSPHY are cleared before starting > > > the dwc3_gadget_run_stop sequence and restoring them after the > > > dwc3_gadget_run_stop sequence is completed. > > > > > > Fixes: 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver") > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com> > > > --- > > > drivers/usb/dwc3/gadget.c | 21 +++++++++++++++++++++ > > > 1 file changed, 21 insertions(+) > > > > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > > index d27af65eb08a..4a158f703d64 100644 > > > --- a/drivers/usb/dwc3/gadget.c > > > +++ b/drivers/usb/dwc3/gadget.c > > > @@ -2629,10 +2629,25 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on) > > > { > > > u32 reg; > > > u32 timeout = 2000; > > > + u32 saved_config = 0; > > > if (pm_runtime_suspended(dwc->dev)) > > > return 0; > > > > Can you add some comments here that this was added through experiment > > since it is not documented in the programming guide. It would be great > > to also note which platform you used to test this with. > > > > I did see this issue during pullup_exit() in SM6375 and SM8150 targets. The > exact code logic worked out for me downstream. > Noted. This is good info. Thanks, Thinh
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index d27af65eb08a..4a158f703d64 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2629,10 +2629,25 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on) { u32 reg; u32 timeout = 2000; + u32 saved_config = 0; if (pm_runtime_suspended(dwc->dev)) return 0; + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); + if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) { + saved_config |= DWC3_GUSB2PHYCFG_SUSPHY; + reg &= ~DWC3_GUSB2PHYCFG_SUSPHY; + } + + if (reg & DWC3_GUSB2PHYCFG_ENBLSLPM) { + saved_config |= DWC3_GUSB2PHYCFG_ENBLSLPM; + reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM; + } + + if (saved_config) + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); + reg = dwc3_readl(dwc->regs, DWC3_DCTL); if (is_on) { if (DWC3_VER_IS_WITHIN(DWC3, ANY, 187A)) { @@ -2660,6 +2675,12 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on) reg &= DWC3_DSTS_DEVCTRLHLT; } while (--timeout && !(!is_on ^ !reg)); + if (saved_config) { + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); + reg |= saved_config; + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); + } + if (!timeout) return -ETIMEDOUT;
There is a frequent timeout during controller enter/exit from halt state after toggling the run_stop bit by SW. This timeout occurs when performing frequent role switches between host and device, causing device enumeration issues due to the timeout. This issue was not present when USB2 suspend PHY was disabled by passing the SNPS quirks (snps,dis_u2_susphy_quirk and snps,dis_enblslpm_quirk) from the DTS. However, there is a requirement to enable USB2 suspend PHY by setting of GUSB2PHYCFG.ENBLSLPM and GUSB2PHYCFG.SUSPHY bits when controller starts in gadget or host mode results in the timeout issue. This commit addresses this timeout issue by ensuring that the bits GUSB2PHYCFG.ENBLSLPM and GUSB2PHYCFG.SUSPHY are cleared before starting the dwc3_gadget_run_stop sequence and restoring them after the dwc3_gadget_run_stop sequence is completed. Fixes: 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver") Cc: stable@vger.kernel.org Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com> --- drivers/usb/dwc3/gadget.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)