[PATCH] usb: dwc3: don't reset device side if dwc3 was configured as host-only

[PATCH] usb: dwc3: don't reset device side if dwc3 was configured as host-only

[Jisheng Zhang]

Commit c4a5153e87fd ("usb: dwc3: core: Power-off core/PHYs on
system_suspend in host mode") replaces check for HOST only dr_mode with
current_dr_role. But during booting, the current_dr_role isn't
initialized, thus the device side reset is always issued even if dwc3
was configured as host-only. What's more, on some platforms with host
only dwc3, aways issuing device side reset by accessing device register
block can cause kernel panic.

Fixes: c4a5153e87fd ("usb: dwc3: core: Power-off core/PHYs on system_suspend in host mode")
Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 drivers/usb/dwc3/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 7b2ce013cc5b..16d7a1d1cbfa 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -277,9 +277,9 @@ int dwc3_core_soft_reset(struct dwc3 *dwc)
 	/*
 	 * We're resetting only the device side because, if we're in host mode,
 	 * XHCI driver will reset the host block. If dwc3 was configured for
-	 * host-only mode, then we can return early.
+	 * host-only mode or current role is host, then we can return early.
 	 */
-	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)
+	if (dwc->dr_mode == USB_DR_MODE_HOST || dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)
 		return 0;
 
 	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
-- 
2.40.1

Re: [PATCH] usb: dwc3: don't reset device side if dwc3 was configured as host-only

[Thinh Nguyen]

On Wed, Jun 28, 2023, Jisheng Zhang wrote:
> Commit c4a5153e87fd ("usb: dwc3: core: Power-off core/PHYs on
> system_suspend in host mode") replaces check for HOST only dr_mode with
> current_dr_role. But during booting, the current_dr_role isn't
> initialized, thus the device side reset is always issued even if dwc3
> was configured as host-only. What's more, on some platforms with host
> only dwc3, aways issuing device side reset by accessing device register
> block can cause kernel panic.
> 

Ah... good catch!

This should be backported to stable.

So perhaps you or Greg can add Cc: stable@vger.kernel.org

> Fixes: c4a5153e87fd ("usb: dwc3: core: Power-off core/PHYs on system_suspend in host mode")
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>  drivers/usb/dwc3/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 7b2ce013cc5b..16d7a1d1cbfa 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -277,9 +277,9 @@ int dwc3_core_soft_reset(struct dwc3 *dwc)
>  	/*
>  	 * We're resetting only the device side because, if we're in host mode,
>  	 * XHCI driver will reset the host block. If dwc3 was configured for
> -	 * host-only mode, then we can return early.
> +	 * host-only mode or current role is host, then we can return early.
>  	 */
> -	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)
> +	if (dwc->dr_mode == USB_DR_MODE_HOST || dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)
>  		return 0;
>  
>  	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> -- 
> 2.40.1
> 

Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh

Re: [PATCH] usb: dwc3: don't reset device side if dwc3 was configured as host-only

[Lu jicong]

It seems that this patch causes driver failed to initialize on rockchip rk3399 devices
when DWC3 controller is configured as dual role.

[    2.827119] dwc3 fe900000.usb: error -ETIMEDOUT: failed to initialize core
[    2.827881] dwc3: probe of fe900000.usb failed with error -110

After some tests I am preliminarily certain that this patch caused the failure.

Re: [PATCH] usb: dwc3: don't reset device side if dwc3 was configured as host-only

[Köry Maincent]

Hello,

Similar issue with ZynqMP board related to that patch:

xilinx-psgtr fd400000.phy: lane 3 (type 1, protocol 3): PLL lock timeout
phy phy-fd400000.phy.3: phy poweron failed --> -110
dwc3 fe300000.usb: error -ETIMEDOUT: failed to initialize core

With CONFIG_USB_DWC3_DUAL_ROLE and dr_mode = "host";

It may not be the correct fix.

-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH] usb: dwc3: don't reset device side if dwc3 was configured as host-only

[Köry Maincent]

On Thu, 16 Nov 2023 17:42:06 +0100
Köry Maincent <kory.maincent@bootlin.com> wrote:

> Hello,
> 
> Similar issue with ZynqMP board related to that patch:
> 
> xilinx-psgtr fd400000.phy: lane 3 (type 1, protocol 3): PLL lock timeout
> phy phy-fd400000.phy.3: phy poweron failed --> -110
> dwc3 fe300000.usb: error -ETIMEDOUT: failed to initialize core
> 
> With CONFIG_USB_DWC3_DUAL_ROLE and dr_mode = "host";
> 
> It may not be the correct fix.

Just figured out there was a patch (357191036889 usb: dwc3: Soft reset phy on
probe for host) from Thinh aimed to fix it but the issue is still here on
ZynqMP.

Regards,

-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH] usb: dwc3: don't reset device side if dwc3 was configured as host-only

[Thinh Nguyen]

Hi,

On Thu, Nov 16, 2023, Köry Maincent wrote:
> On Thu, 16 Nov 2023 17:42:06 +0100
> Köry Maincent <kory.maincent@bootlin.com> wrote:
> 
> > Hello,
> > 
> > Similar issue with ZynqMP board related to that patch:
> > 
> > xilinx-psgtr fd400000.phy: lane 3 (type 1, protocol 3): PLL lock timeout
> > phy phy-fd400000.phy.3: phy poweron failed --> -110
> > dwc3 fe300000.usb: error -ETIMEDOUT: failed to initialize core
> > 
> > With CONFIG_USB_DWC3_DUAL_ROLE and dr_mode = "host";
> > 
> > It may not be the correct fix.
> 
> Just figured out there was a patch (357191036889 usb: dwc3: Soft reset phy on
> probe for host) from Thinh aimed to fix it but the issue is still here on
> ZynqMP.
> 

How many ports do you use? Can you try this:

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 0328c86ef806..9921c2737829 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -296,23 +296,28 @@ int dwc3_core_soft_reset(struct dwc3 *dwc)
 	if (dwc->dr_mode == USB_DR_MODE_HOST) {
 		u32 usb3_port;
 		u32 usb2_port;
+		int i;
 
-		usb3_port = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
-		usb3_port |= DWC3_GUSB3PIPECTL_PHYSOFTRST;
-		dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), usb3_port);
+		for (i = 0; i < 16; i++) {
+			usb3_port = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(i));
+			usb3_port |= DWC3_GUSB3PIPECTL_PHYSOFTRST;
+			dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(i), usb3_port);
 
-		usb2_port = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
-		usb2_port |= DWC3_GUSB2PHYCFG_PHYSOFTRST;
-		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), usb2_port);
+			usb2_port = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i));
+			usb2_port |= DWC3_GUSB2PHYCFG_PHYSOFTRST;
+			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), usb2_port);
+		}
 
 		/* Small delay for phy reset assertion */
 		usleep_range(1000, 2000);
 
-		usb3_port &= ~DWC3_GUSB3PIPECTL_PHYSOFTRST;
-		dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), usb3_port);
+		for (i = 0; i < 16; i++) {
+			usb3_port &= ~DWC3_GUSB3PIPECTL_PHYSOFTRST;
+			dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(i), usb3_port);
 
-		usb2_port &= ~DWC3_GUSB2PHYCFG_PHYSOFTRST;
-		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), usb2_port);
+			usb2_port &= ~DWC3_GUSB2PHYCFG_PHYSOFTRST;
+			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), usb2_port);
+		}
 
 		/* Wait for clock synchronization */
 		msleep(50);
---

BR,
Thinh

Re: [PATCH] usb: dwc3: don't reset device side if dwc3 was configured as host-only

[Thinh Nguyen]

Hi,

Sorry, email client issue with your email. Attempt to resend:

On Fri, Nov 17, 2023, Thinh Nguyen wrote:
> Hi,
> 
> On Thu, Nov 16, 2023, Köry Maincent wrote:
> > On Thu, 16 Nov 2023 17:42:06 +0100
> > Köry Maincent <kory.maincent@bootlin.com> wrote:
> > 
> > > Hello,
> > > 
> > > Similar issue with ZynqMP board related to that patch:
> > > 
> > > xilinx-psgtr fd400000.phy: lane 3 (type 1, protocol 3): PLL lock timeout
> > > phy phy-fd400000.phy.3: phy poweron failed --> -110
> > > dwc3 fe300000.usb: error -ETIMEDOUT: failed to initialize core
> > > 
> > > With CONFIG_USB_DWC3_DUAL_ROLE and dr_mode = "host";
> > > 
> > > It may not be the correct fix.
> > 
> > Just figured out there was a patch (357191036889 usb: dwc3: Soft reset phy on
> > probe for host) from Thinh aimed to fix it but the issue is still here on
> > ZynqMP.
> > 
> 
> How many ports do you use? Can you try this:
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 0328c86ef806..9921c2737829 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -296,23 +296,28 @@ int dwc3_core_soft_reset(struct dwc3 *dwc)
>  	if (dwc->dr_mode == USB_DR_MODE_HOST) {
>  		u32 usb3_port;
>  		u32 usb2_port;
> +		int i;
>  
> -		usb3_port = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
> -		usb3_port |= DWC3_GUSB3PIPECTL_PHYSOFTRST;
> -		dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), usb3_port);
> +		for (i = 0; i < 16; i++) {
> +			usb3_port = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(i));
> +			usb3_port |= DWC3_GUSB3PIPECTL_PHYSOFTRST;
> +			dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(i), usb3_port);
>  
> -		usb2_port = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> -		usb2_port |= DWC3_GUSB2PHYCFG_PHYSOFTRST;
> -		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), usb2_port);
> +			usb2_port = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i));
> +			usb2_port |= DWC3_GUSB2PHYCFG_PHYSOFTRST;
> +			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), usb2_port);
> +		}
>  
>  		/* Small delay for phy reset assertion */
>  		usleep_range(1000, 2000);
>  
> -		usb3_port &= ~DWC3_GUSB3PIPECTL_PHYSOFTRST;
> -		dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), usb3_port);
> +		for (i = 0; i < 16; i++) {
> +			usb3_port &= ~DWC3_GUSB3PIPECTL_PHYSOFTRST;
> +			dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(i), usb3_port);
>  
> -		usb2_port &= ~DWC3_GUSB2PHYCFG_PHYSOFTRST;
> -		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), usb2_port);
> +			usb2_port &= ~DWC3_GUSB2PHYCFG_PHYSOFTRST;
> +			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), usb2_port);
> +		}
>  
>  		/* Wait for clock synchronization */
>  		msleep(50);
> ---

Re: [PATCH] usb: dwc3: don't reset device side if dwc3 was configured as host-only

[Köry Maincent]

Hello Thinh,

On Fri, 17 Nov 2023 01:55:30 +0000
Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:

> Hi,
> 
> Sorry, email client issue with your email. Attempt to resend:

Thanks for your quick reply.

> 
> On Fri, Nov 17, 2023, Thinh Nguyen wrote:
> > Hi,
> > 
> > On Thu, Nov 16, 2023, Köry Maincent wrote:  
> > > On Thu, 16 Nov 2023 17:42:06 +0100
> > > Köry Maincent <kory.maincent@bootlin.com> wrote:
> > >   
> > > > Hello,
> > > > 
> > > > Similar issue with ZynqMP board related to that patch:
> > > > 
> > > > xilinx-psgtr fd400000.phy: lane 3 (type 1, protocol 3): PLL lock timeout
> > > > phy phy-fd400000.phy.3: phy poweron failed --> -110
> > > > dwc3 fe300000.usb: error -ETIMEDOUT: failed to initialize core
> > > > 
> > > > With CONFIG_USB_DWC3_DUAL_ROLE and dr_mode = "host";
> > > > 
> > > > It may not be the correct fix.  
> > > 
> > > Just figured out there was a patch (357191036889 usb: dwc3: Soft reset
> > > phy on probe for host) from Thinh aimed to fix it but the issue is still
> > > here on ZynqMP.
> > >   
> > 
> > How many ports do you use? Can you try this:

I am using 2 ports.
I will test it out next week as I don't have access to the board until then.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH] usb: dwc3: don't reset device side if dwc3 was configured as host-only

[Köry Maincent]

Hello Thinh,

On Fri, 17 Nov 2023 01:55:30 +0000
Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:

> > How many ports do you use? Can you try this:
> > 
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 0328c86ef806..9921c2737829 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -296,23 +296,28 @@ int dwc3_core_soft_reset(struct dwc3 *dwc)
> >  	if (dwc->dr_mode == USB_DR_MODE_HOST) {
> >  		u32 usb3_port;
> >  		u32 usb2_port;
> > +		int i;
> >  
> > -		usb3_port = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
> > -		usb3_port |= DWC3_GUSB3PIPECTL_PHYSOFTRST;
> > -		dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), usb3_port);
> > +		for (i = 0; i < 16; i++) {
> > +			usb3_port = dwc3_readl(dwc->regs,
> > DWC3_GUSB3PIPECTL(i));
> > +			usb3_port |= DWC3_GUSB3PIPECTL_PHYSOFTRST;
> > +			dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(i),
> > usb3_port); 
> > -		usb2_port = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> > -		usb2_port |= DWC3_GUSB2PHYCFG_PHYSOFTRST;
> > -		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), usb2_port);
> > +			usb2_port = dwc3_readl(dwc->regs,
> > DWC3_GUSB2PHYCFG(i));
> > +			usb2_port |= DWC3_GUSB2PHYCFG_PHYSOFTRST;
> > +			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i),
> > usb2_port);
> > +		}
> >  
> >  		/* Small delay for phy reset assertion */
> >  		usleep_range(1000, 2000);
> >  
> > -		usb3_port &= ~DWC3_GUSB3PIPECTL_PHYSOFTRST;
> > -		dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), usb3_port);
> > +		for (i = 0; i < 16; i++) {
> > +			usb3_port &= ~DWC3_GUSB3PIPECTL_PHYSOFTRST;
> > +			dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(i),
> > usb3_port); 
> > -		usb2_port &= ~DWC3_GUSB2PHYCFG_PHYSOFTRST;
> > -		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), usb2_port);
> > +			usb2_port &= ~DWC3_GUSB2PHYCFG_PHYSOFTRST;
> > +			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i),
> > usb2_port);
> > +		}
> >  
> >  		/* Wait for clock synchronization */
> >  		msleep(50);
> > --  

Still not working on my side.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH] usb: dwc3: don't reset device side if dwc3 was configured as host-only

[Köry Maincent]

On Tue, 21 Nov 2023 10:49:17 +0100
Köry Maincent <kory.maincent@bootlin.com> wrote:

> Hello Thinh,
> 
> On Fri, 17 Nov 2023 01:55:30 +0000
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:

> Still not working on my side.

Hello,

Just wondering if you have received my email as you said having client mail
issue.

regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH] usb: dwc3: don't reset device side if dwc3 was configured as host-only

[Thinh Nguyen]

Hi,

On Fri, Dec 01, 2023, Köry Maincent wrote:
> On Tue, 21 Nov 2023 10:49:17 +0100
> Köry Maincent <kory.maincent@bootlin.com> wrote:
> 
> > Hello Thinh,
> > 
> > On Fri, 17 Nov 2023 01:55:30 +0000
> > Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> 
> > Still not working on my side.
> 
> Hello,
> 
> Just wondering if you have received my email as you said having client mail
> issue.
> 

Sorry for the delay. Things got busy for me recently.

So your platform has multiple ports. Do you use UTMI or ULPI phy?

I forgot another thing, the phy reset we're doing now doesn't apply to
ULPI phys. We may need to teach HCRST to dwc3, this may not look clean.

Hi Lisheng,

Did you see any reported issue before your change were applied? If not,
perhaps we should revert the changes related to soft-reset for this.

Thanks,
Thinh

Re: [PATCH] usb: dwc3: don't reset device side if dwc3 was configured as host-only

[Thinh Nguyen]

On Sat, Dec 02, 2023, Thinh Nguyen wrote:
> 
> Hi Lisheng,

Typo, I mean Jisheng.

> 
> Did you see any reported issue before your change were applied? If not,
> perhaps we should revert the changes related to soft-reset for this.
> 

BR,
Thinh

Re: [PATCH] usb: dwc3: don't reset device side if dwc3 was configured as host-only

[Jisheng Zhang]

On Tue, Dec 05, 2023 at 01:27:56AM +0000, Thinh Nguyen wrote:
> 
> On Sat, Dec 02, 2023, Thinh Nguyen wrote:
> > 
> > Hi Lisheng,
> 
> Typo, I mean Jisheng.

Hi Thinh

> 
> > 
> > Did you see any reported issue before your change were applied? If not,
> > perhaps we should revert the changes related to soft-reset for this.
> > 

It seems this patch brings more issues than solved, I think reverting it is
better. 

Thanks

Re: [PATCH] usb: dwc3: don't reset device side if dwc3 was configured as host-only

[Köry Maincent]

On Sat, 2 Dec 2023 00:26:34 +0000
Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:

> Hi,
> 
> On Fri, Dec 01, 2023, Köry Maincent wrote:
> > On Tue, 21 Nov 2023 10:49:17 +0100
> > Köry Maincent <kory.maincent@bootlin.com> wrote:
> >   
> > > Hello Thinh,
> > > 
> > > On Fri, 17 Nov 2023 01:55:30 +0000
> > > Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:  
> >   
> > > Still not working on my side.  
> > 
> > Hello,
> > 
> > Just wondering if you have received my email as you said having client mail
> > issue.
> >   
> 
> Sorry for the delay. Things got busy for me recently.
> 
> So your platform has multiple ports. Do you use UTMI or ULPI phy?

It uses USB3220 ULPI phy.

> I forgot another thing, the phy reset we're doing now doesn't apply to
> ULPI phys. We may need to teach HCRST to dwc3, this may not look clean.

So that is the reason it does not apply for my case.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com