diff mbox series

usb: dwc3: Soft reset phy on probe for host

Message ID 70aea513215d273669152696cc02b20ddcdb6f1a.1694564261.git.Thinh.Nguyen@synopsys.com (mailing list archive)
State Accepted
Commit 8bea147dfdf823eaa8d3baeccc7aeb041b41944b
Headers show
Series usb: dwc3: Soft reset phy on probe for host | expand

Commit Message

Thinh Nguyen Sept. 13, 2023, 12:52 a.m. UTC 
When there's phy initialization, we need to initiate a soft-reset
sequence. That's done through USBCMD.HCRST in the xHCI driver and its
initialization, However, the dwc3 driver may modify core configs before
the soft-reset. This may result in some connection instability. So,
ensure the phy is ready before the controller updates the GCTL.PRTCAPDIR
or other settings by issuing phy soft-reset.

Note that some host-mode configurations may not expose device registers
to initiate the controller soft-reset (via DCTL.CoreSftRst). So we reset
through GUSB3PIPECTL and GUSB2PHYCFG instead.

Cc: stable@vger.kernel.org
Fixes: e835c0a4e23c ("usb: dwc3: don't reset device side if dwc3 was configured as host-only")
Reported-by: Kenta Sato <tosainu.maple@gmail.com>
Closes: https://lore.kernel.org/linux-usb/ZPUciRLUcjDywMVS@debian.me/
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/dwc3/core.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)


base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d

Comments

Thinh Nguyen Sept. 13, 2023, 12:57 a.m. UTC | #1 
Hi Kenta,

On Wed, Sep 13, 2023, Thinh Nguyen wrote:
> When there's phy initialization, we need to initiate a soft-reset
> sequence. That's done through USBCMD.HCRST in the xHCI driver and its
> initialization, However, the dwc3 driver may modify core configs before
> the soft-reset. This may result in some connection instability. So,
> ensure the phy is ready before the controller updates the GCTL.PRTCAPDIR
> or other settings by issuing phy soft-reset.
> 
> Note that some host-mode configurations may not expose device registers
> to initiate the controller soft-reset (via DCTL.CoreSftRst). So we reset
> through GUSB3PIPECTL and GUSB2PHYCFG instead.
> 
> Cc: stable@vger.kernel.org
> Fixes: e835c0a4e23c ("usb: dwc3: don't reset device side if dwc3 was configured as host-only")
> Reported-by: Kenta Sato <tosainu.maple@gmail.com>
> Closes: https://lore.kernel.org/linux-usb/ZPUciRLUcjDywMVS@debian.me/
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> ---
>  drivers/usb/dwc3/core.c | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 9c6bf054f15d..343d2570189f 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -279,9 +279,46 @@ int dwc3_core_soft_reset(struct dwc3 *dwc)
>  	 * XHCI driver will reset the host block. If dwc3 was configured for
>  	 * host-only mode or current role is host, then we can return early.
>  	 */
> -	if (dwc->dr_mode == USB_DR_MODE_HOST || dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)
> +	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)
>  		return 0;
>  
> +	/*
> +	 * If the dr_mode is host and the dwc->current_dr_role is not the
> +	 * corresponding DWC3_GCTL_PRTCAP_HOST, then the dwc3_core_init_mode
> +	 * isn't executed yet. Ensure the phy is ready before the controller
> +	 * updates the GCTL.PRTCAPDIR or other settings by soft-resetting
> +	 * the phy.
> +	 *
> +	 * Note: GUSB3PIPECTL[n] and GUSB2PHYCFG[n] are port settings where n
> +	 * is port index. If this is a multiport host, then we need to reset
> +	 * all active ports.
> +	 */
> +	if (dwc->dr_mode == USB_DR_MODE_HOST) {
> +		u32 usb3_port;
> +		u32 usb2_port;
> +
> +		usb3_port = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
> +		usb3_port |= DWC3_GUSB3PIPECTL_PHYSOFTRST;
> +		dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), 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);
> +
> +		/* Small delay for phy reset assertion */
> +		usleep_range(1000, 2000);
> +
> +		usb3_port &= ~DWC3_GUSB3PIPECTL_PHYSOFTRST;
> +		dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), usb3_port);
> +
> +		usb2_port &= ~DWC3_GUSB2PHYCFG_PHYSOFTRST;
> +		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), usb2_port);
> +
> +		/* Wait for clock synchronization */
> +		msleep(50);
> +		return 0;
> +	}
> +
>  	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>  	reg |= DWC3_DCTL_CSFTRST;
>  	reg &= ~DWC3_DCTL_RUN_STOP;
> 
> base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
> -- 
> 2.38.1

Can you verify again using this updated patch? If it works, can you add
a Tested-by tag?

Thanks!
Thinh
Kenta Sato Sept. 13, 2023, 5:31 p.m. UTC | #2 
On Wed, Sep 13, 2023 at 9:57 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>
> Hi Kenta,
>
> On Wed, Sep 13, 2023, Thinh Nguyen wrote:
> > When there's phy initialization, we need to initiate a soft-reset
> > sequence. That's done through USBCMD.HCRST in the xHCI driver and its
> > initialization, However, the dwc3 driver may modify core configs before
> > the soft-reset. This may result in some connection instability. So,
> > ensure the phy is ready before the controller updates the GCTL.PRTCAPDIR
> > or other settings by issuing phy soft-reset.
> >
> > Note that some host-mode configurations may not expose device registers
> > to initiate the controller soft-reset (via DCTL.CoreSftRst). So we reset
> > through GUSB3PIPECTL and GUSB2PHYCFG instead.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: e835c0a4e23c ("usb: dwc3: don't reset device side if dwc3 was configured as host-only")
> > Reported-by: Kenta Sato <tosainu.maple@gmail.com>
> > Closes: https://lore.kernel.org/linux-usb/ZPUciRLUcjDywMVS@debian.me/
> > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> > ---
> >  drivers/usb/dwc3/core.c | 39 ++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 9c6bf054f15d..343d2570189f 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -279,9 +279,46 @@ int dwc3_core_soft_reset(struct dwc3 *dwc)
> >        * XHCI driver will reset the host block. If dwc3 was configured for
> >        * host-only mode or current role is host, then we can return early.
> >        */
> > -     if (dwc->dr_mode == USB_DR_MODE_HOST || dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)
> > +     if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)
> >               return 0;
> >
> > +     /*
> > +      * If the dr_mode is host and the dwc->current_dr_role is not the
> > +      * corresponding DWC3_GCTL_PRTCAP_HOST, then the dwc3_core_init_mode
> > +      * isn't executed yet. Ensure the phy is ready before the controller
> > +      * updates the GCTL.PRTCAPDIR or other settings by soft-resetting
> > +      * the phy.
> > +      *
> > +      * Note: GUSB3PIPECTL[n] and GUSB2PHYCFG[n] are port settings where n
> > +      * is port index. If this is a multiport host, then we need to reset
> > +      * all active ports.
> > +      */
> > +     if (dwc->dr_mode == USB_DR_MODE_HOST) {
> > +             u32 usb3_port;
> > +             u32 usb2_port;
> > +
> > +             usb3_port = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
> > +             usb3_port |= DWC3_GUSB3PIPECTL_PHYSOFTRST;
> > +             dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), 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);
> > +
> > +             /* Small delay for phy reset assertion */
> > +             usleep_range(1000, 2000);
> > +
> > +             usb3_port &= ~DWC3_GUSB3PIPECTL_PHYSOFTRST;
> > +             dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), usb3_port);
> > +
> > +             usb2_port &= ~DWC3_GUSB2PHYCFG_PHYSOFTRST;
> > +             dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), usb2_port);
> > +
> > +             /* Wait for clock synchronization */
> > +             msleep(50);
> > +             return 0;
> > +     }
> > +
> >       reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> >       reg |= DWC3_DCTL_CSFTRST;
> >       reg &= ~DWC3_DCTL_RUN_STOP;
> >
> > base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
> > --
> > 2.38.1
>
> Can you verify again using this updated patch? If it works, can you add
> a Tested-by tag?

Hi Thinh,

I tried your updated patch and verified that the kernel detects the
USB devices correctly at every boot while I was testing. I would say
the issue seems to be solved. I attached the boot log for your
information.

Tested-by: Kenta Sato <tosainu.maple@gmail.com>

Thank you so much!
Kenta

>
> Thanks!
> Thinh
Dang Huynh Oct. 3, 2023, 2:49 p.m. UTC | #3 
Hi Thinh,

I can confirm that this patch fixed the USB 3.0 port on the Pinebook Pro. It may 
be too late for a Tested-by but:

Tested-by: Dang Huynh <danct12@riseup.net>

Best regards,
Dang

On Wednesday, September 13, 2023 12:52:15 AM UTC Thinh Nguyen wrote:
> When there's phy initialization, we need to initiate a soft-reset
> sequence. That's done through USBCMD.HCRST in the xHCI driver and its
> initialization, However, the dwc3 driver may modify core configs before
> the soft-reset. This may result in some connection instability. So,
> ensure the phy is ready before the controller updates the GCTL.PRTCAPDIR
> or other settings by issuing phy soft-reset.
> 
> Note that some host-mode configurations may not expose device registers
> to initiate the controller soft-reset (via DCTL.CoreSftRst). So we reset
> through GUSB3PIPECTL and GUSB2PHYCFG instead.
> 
> Cc: stable@vger.kernel.org
> Fixes: e835c0a4e23c ("usb: dwc3: don't reset device side if dwc3 was
> configured as host-only") Reported-by: Kenta Sato <tosainu.maple@gmail.com>
> Closes: https://lore.kernel.org/linux-usb/ZPUciRLUcjDywMVS@debian.me/
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> ---
>  drivers/usb/dwc3/core.c | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 9c6bf054f15d..343d2570189f 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -279,9 +279,46 @@ int dwc3_core_soft_reset(struct dwc3 *dwc)
>  	 * XHCI driver will reset the host block. If dwc3 was configured 
for
>  	 * host-only mode or current role is host, then we can return 
early.
>  	 */
> -	if (dwc->dr_mode == USB_DR_MODE_HOST || dwc->current_dr_role ==
> DWC3_GCTL_PRTCAP_HOST) +	if (dwc->current_dr_role == 
DWC3_GCTL_PRTCAP_HOST)
>  		return 0;
> 
> +	/*
> +	 * If the dr_mode is host and the dwc->current_dr_role is not the
> +	 * corresponding DWC3_GCTL_PRTCAP_HOST, then the 
dwc3_core_init_mode
> +	 * isn't executed yet. Ensure the phy is ready before the 
controller
> +	 * updates the GCTL.PRTCAPDIR or other settings by soft-resetting
> +	 * the phy.
> +	 *
> +	 * Note: GUSB3PIPECTL[n] and GUSB2PHYCFG[n] are port settings where 
n
> +	 * is port index. If this is a multiport host, then we need to 
reset
> +	 * all active ports.
> +	 */
> +	if (dwc->dr_mode == USB_DR_MODE_HOST) {
> +		u32 usb3_port;
> +		u32 usb2_port;
> +
> +		usb3_port = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
> +		usb3_port |= DWC3_GUSB3PIPECTL_PHYSOFTRST;
> +		dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), 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);
> +
> +		/* Small delay for phy reset assertion */
> +		usleep_range(1000, 2000);
> +
> +		usb3_port &= ~DWC3_GUSB3PIPECTL_PHYSOFTRST;
> +		dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), usb3_port);
> +
> +		usb2_port &= ~DWC3_GUSB2PHYCFG_PHYSOFTRST;
> +		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), usb2_port);
> +
> +		/* Wait for clock synchronization */
> +		msleep(50);
> +		return 0;
> +	}
> +
>  	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>  	reg |= DWC3_DCTL_CSFTRST;
>  	reg &= ~DWC3_DCTL_RUN_STOP;
> 
> base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
Thinh Nguyen Oct. 12, 2023, 6:20 p.m. UTC | #4 
On Tue, Oct 03, 2023, Dang Huynh wrote:
> Hi Thinh,
> 
> I can confirm that this patch fixed the USB 3.0 port on the Pinebook Pro. It may 
> be too late for a Tested-by but:
> 
> Tested-by: Dang Huynh <danct12@riseup.net>
> 

Thanks for testing. It's already in Greg's usb-linus branch.

BR,
Thinh
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 9c6bf054f15d..343d2570189f 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -279,9 +279,46 @@  int dwc3_core_soft_reset(struct dwc3 *dwc)
 	 * XHCI driver will reset the host block. If dwc3 was configured for
 	 * host-only mode or current role is host, then we can return early.
 	 */
-	if (dwc->dr_mode == USB_DR_MODE_HOST || dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)
+	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)
 		return 0;
 
+	/*
+	 * If the dr_mode is host and the dwc->current_dr_role is not the
+	 * corresponding DWC3_GCTL_PRTCAP_HOST, then the dwc3_core_init_mode
+	 * isn't executed yet. Ensure the phy is ready before the controller
+	 * updates the GCTL.PRTCAPDIR or other settings by soft-resetting
+	 * the phy.
+	 *
+	 * Note: GUSB3PIPECTL[n] and GUSB2PHYCFG[n] are port settings where n
+	 * is port index. If this is a multiport host, then we need to reset
+	 * all active ports.
+	 */
+	if (dwc->dr_mode == USB_DR_MODE_HOST) {
+		u32 usb3_port;
+		u32 usb2_port;
+
+		usb3_port = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
+		usb3_port |= DWC3_GUSB3PIPECTL_PHYSOFTRST;
+		dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), 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);
+
+		/* Small delay for phy reset assertion */
+		usleep_range(1000, 2000);
+
+		usb3_port &= ~DWC3_GUSB3PIPECTL_PHYSOFTRST;
+		dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), usb3_port);
+
+		usb2_port &= ~DWC3_GUSB2PHYCFG_PHYSOFTRST;
+		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), usb2_port);
+
+		/* Wait for clock synchronization */
+		msleep(50);
+		return 0;
+	}
+
 	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
 	reg |= DWC3_DCTL_CSFTRST;
 	reg &= ~DWC3_DCTL_RUN_STOP;