[PATCH 0/2] usb: dwc3: Disable susphy during initialization

[PATCH 0/2] usb: dwc3: Disable susphy during initialization

[Thinh Nguyen]

We notice some platforms set "snps,dis_u3_susphy_quirk" and
"snps,dis_u2_susphy_quirk" when they should not need to. Just make sure that
the GUSB3PIPECTL.SUSPENDENABLE and GUSB2PHYCFG.SUSPHY are clear during
initialization. The host initialization involved xhci. So the dwc3 needs to
implement the xhci_plat_priv->plat_start() for xhci to re-enable the suspend
bits.

Since there's a prerequisite patch to drivers/usb/host/xhci-plat.h that's not a
fix patch, this series should go on Greg's usb-testing branch instead of
usb-linus.



Thinh Nguyen (2):
  usb: xhci-plat: Don't include xhci.h
  usb: dwc3: core: Prevent phy suspend during init

 drivers/usb/dwc3/core.c      | 90 +++++++++++++++---------------------
 drivers/usb/dwc3/core.h      |  1 +
 drivers/usb/dwc3/gadget.c    |  2 +
 drivers/usb/dwc3/host.c      | 27 +++++++++++
 drivers/usb/host/xhci-plat.h |  4 +-
 5 files changed, 71 insertions(+), 53 deletions(-)


base-commit: 3d122e6d27e417a9fa91181922743df26b2cd679
-- 
2.28.0

[PATCH 1/2] usb: xhci-plat: Don't include xhci.h

[Thinh Nguyen]

The xhci_plat.h should not need to include the entire xhci.h header.
This can cause redefinition in dwc3 if it selectively includes some xHCI
definitions. This is a prerequisite change for a fix to disable suspend
during initialization for dwc3.

Cc: stable@vger.kernel.org
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/host/xhci-plat.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h
index 2d15386f2c50..6475130eac4b 100644
--- a/drivers/usb/host/xhci-plat.h
+++ b/drivers/usb/host/xhci-plat.h
@@ -8,7 +8,9 @@
 #ifndef _XHCI_PLAT_H
 #define _XHCI_PLAT_H
 
-#include "xhci.h"	/* for hcd_to_xhci() */
+struct device;
+struct platform_device;
+struct usb_hcd;
 
 struct xhci_plat_priv {
 	const char *firmware_name;
-- 
2.28.0

[PATCH 2/2] usb: dwc3: core: Prevent phy suspend during init

[Thinh Nguyen]

GUSB3PIPECTL.SUSPENDENABLE and GUSB2PHYCFG.SUSPHY should be cleared
during initialization. Suspend during initialization can result in
undefined behavior due to clock synchronization failure, which often
seen as core soft reset timeout.

The programming guide recommended these bits to be cleared during
initialization for DWC_usb3.0 version 1.94 and above (along with
DWC_usb31 and DWC_usb32). The current check in the driver does not
account if it's set by default setting from coreConsultant.

This is especially the case for DRD when switching mode to ensure the
phy clocks are available to change mode. Depending on the
platforms/design, some may be affected more than others. This is noted
in the DWC_usb3x programming guide under the above registers.

Let's just disable them during driver load and mode switching. Restore
them when the controller initialization completes.

Note that some platforms workaround this issue by disabling phy suspend
through "snps,dis_u3_susphy_quirk" and "snps,dis_u2_susphy_quirk" when
they should not need to.

Cc: stable@vger.kernel.org
Fixes: 9ba3aca8fe82 ("usb: dwc3: Disable phy suspend after power-on reset")
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/dwc3/core.c   | 90 +++++++++++++++++----------------------
 drivers/usb/dwc3/core.h   |  1 +
 drivers/usb/dwc3/gadget.c |  2 +
 drivers/usb/dwc3/host.c   | 27 ++++++++++++
 4 files changed, 68 insertions(+), 52 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 31684cdaaae3..100041320e8d 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -104,6 +104,27 @@ static int dwc3_get_dr_mode(struct dwc3 *dwc)
 	return 0;
 }
 
+void dwc3_enable_susphy(struct dwc3 *dwc, bool enable)
+{
+	u32 reg;
+
+	reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
+	if (enable && !dwc->dis_u3_susphy_quirk)
+		reg |= DWC3_GUSB3PIPECTL_SUSPHY;
+	else
+		reg &= ~DWC3_GUSB3PIPECTL_SUSPHY;
+
+	dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
+
+	reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
+	if (enable && !dwc->dis_u2_susphy_quirk)
+		reg |= DWC3_GUSB2PHYCFG_SUSPHY;
+	else
+		reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
+
+	dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+}
+
 void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode)
 {
 	u32 reg;
@@ -585,11 +606,8 @@ static int dwc3_core_ulpi_init(struct dwc3 *dwc)
  */
 static int dwc3_phy_setup(struct dwc3 *dwc)
 {
-	unsigned int hw_mode;
 	u32 reg;
 
-	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
-
 	reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
 
 	/*
@@ -599,21 +617,16 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
 	reg &= ~DWC3_GUSB3PIPECTL_UX_EXIT_PX;
 
 	/*
-	 * Above 1.94a, it is recommended to set DWC3_GUSB3PIPECTL_SUSPHY
-	 * to '0' during coreConsultant configuration. So default value
-	 * will be '0' when the core is reset. Application needs to set it
-	 * to '1' after the core initialization is completed.
-	 */
-	if (!DWC3_VER_IS_WITHIN(DWC3, ANY, 194A))
-		reg |= DWC3_GUSB3PIPECTL_SUSPHY;
-
-	/*
-	 * For DRD controllers, GUSB3PIPECTL.SUSPENDENABLE must be cleared after
-	 * power-on reset, and it can be set after core initialization, which is
-	 * after device soft-reset during initialization.
+	 * Above DWC_usb3.0 1.94a, it is recommended to set
+	 * DWC3_GUSB3PIPECTL_SUSPHY to '0' during coreConsultant configuration.
+	 * So default value will be '0' when the core is reset. Application
+	 * needs to set it to '1' after the core initialization is completed.
+	 *
+	 * Similarly for DRD controllers, GUSB3PIPECTL.SUSPENDENABLE must be
+	 * cleared after power-on reset, and it can be set after core
+	 * initialization.
 	 */
-	if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD)
-		reg &= ~DWC3_GUSB3PIPECTL_SUSPHY;
+	reg &= ~DWC3_GUSB3PIPECTL_SUSPHY;
 
 	if (dwc->u2ss_inp3_quirk)
 		reg |= DWC3_GUSB3PIPECTL_U2SSINP3OK;
@@ -639,9 +652,6 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
 	if (dwc->tx_de_emphasis_quirk)
 		reg |= DWC3_GUSB3PIPECTL_TX_DEEPH(dwc->tx_de_emphasis);
 
-	if (dwc->dis_u3_susphy_quirk)
-		reg &= ~DWC3_GUSB3PIPECTL_SUSPHY;
-
 	if (dwc->dis_del_phy_power_chg_quirk)
 		reg &= ~DWC3_GUSB3PIPECTL_DEPOCHANGE;
 
@@ -689,24 +699,15 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
 	}
 
 	/*
-	 * Above 1.94a, it is recommended to set DWC3_GUSB2PHYCFG_SUSPHY to
-	 * '0' during coreConsultant configuration. So default value will
-	 * be '0' when the core is reset. Application needs to set it to
-	 * '1' after the core initialization is completed.
-	 */
-	if (!DWC3_VER_IS_WITHIN(DWC3, ANY, 194A))
-		reg |= DWC3_GUSB2PHYCFG_SUSPHY;
-
-	/*
-	 * For DRD controllers, GUSB2PHYCFG.SUSPHY must be cleared after
-	 * power-on reset, and it can be set after core initialization, which is
-	 * after device soft-reset during initialization.
+	 * Above DWC_usb3.0 1.94a, it is recommended to set
+	 * DWC3_GUSB2PHYCFG_SUSPHY to '0' during coreConsultant configuration.
+	 * So default value will be '0' when the core is reset. Application
+	 * needs to set it to '1' after the core initialization is completed.
+	 *
+	 * Similarly for DRD controllers, GUSB2PHYCFG.SUSPHY must be cleared
+	 * after power-on reset, and it can be set after core initialization.
 	 */
-	if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD)
-		reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
-
-	if (dwc->dis_u2_susphy_quirk)
-		reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
+	reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
 
 	if (dwc->dis_enblslpm_quirk)
 		reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM;
@@ -1227,21 +1228,6 @@ static int dwc3_core_init(struct dwc3 *dwc)
 	if (ret)
 		goto err_exit_phy;
 
-	if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD &&
-	    !DWC3_VER_IS_WITHIN(DWC3, ANY, 194A)) {
-		if (!dwc->dis_u3_susphy_quirk) {
-			reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
-			reg |= DWC3_GUSB3PIPECTL_SUSPHY;
-			dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
-		}
-
-		if (!dwc->dis_u2_susphy_quirk) {
-			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
-			reg |= DWC3_GUSB2PHYCFG_SUSPHY;
-			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
-		}
-	}
-
 	dwc3_core_setup_global_control(dwc);
 	dwc3_core_num_eps(dwc);
 
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 7e80dd3d466b..180dd8d29287 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1580,6 +1580,7 @@ int dwc3_event_buffers_setup(struct dwc3 *dwc);
 void dwc3_event_buffers_cleanup(struct dwc3 *dwc);
 
 int dwc3_core_soft_reset(struct dwc3 *dwc);
+void dwc3_enable_susphy(struct dwc3 *dwc, bool enable);
 
 #if IS_ENABLED(CONFIG_USB_DWC3_HOST) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
 int dwc3_host_init(struct dwc3 *dwc);
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 4df2661f6675..f94f68f1e7d2 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2924,6 +2924,7 @@ static int __dwc3_gadget_start(struct dwc3 *dwc)
 	dwc3_ep0_out_start(dwc);
 
 	dwc3_gadget_enable_irq(dwc);
+	dwc3_enable_susphy(dwc, true);
 
 	return 0;
 
@@ -4690,6 +4691,7 @@ void dwc3_gadget_exit(struct dwc3 *dwc)
 	if (!dwc->gadget)
 		return;
 
+	dwc3_enable_susphy(dwc, false);
 	usb_del_gadget(dwc->gadget);
 	dwc3_gadget_free_endpoints(dwc);
 	usb_put_gadget(dwc->gadget);
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index 0204787df81d..a171b27a7845 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -10,10 +10,13 @@
 #include <linux/irq.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/usb.h>
+#include <linux/usb/hcd.h>
 
 #include "../host/xhci-port.h"
 #include "../host/xhci-ext-caps.h"
 #include "../host/xhci-caps.h"
+#include "../host/xhci-plat.h"
 #include "core.h"
 
 #define XHCI_HCSPARAMS1		0x4
@@ -57,6 +60,24 @@ static void dwc3_power_off_all_roothub_ports(struct dwc3 *dwc)
 	}
 }
 
+static void dwc3_xhci_plat_start(struct usb_hcd *hcd)
+{
+	struct platform_device *pdev;
+	struct dwc3 *dwc;
+
+	if (!usb_hcd_is_primary_hcd(hcd))
+		return;
+
+	pdev = to_platform_device(hcd->self.controller);
+	dwc = dev_get_drvdata(pdev->dev.parent);
+
+	dwc3_enable_susphy(dwc, true);
+}
+
+static const struct xhci_plat_priv dwc3_xhci_plat_quirk = {
+	.plat_start = dwc3_xhci_plat_start,
+};
+
 static void dwc3_host_fill_xhci_irq_res(struct dwc3 *dwc,
 					int irq, char *name)
 {
@@ -167,6 +188,11 @@ int dwc3_host_init(struct dwc3 *dwc)
 		}
 	}
 
+	ret = platform_device_add_data(xhci, &dwc3_xhci_plat_quirk,
+				       sizeof(struct xhci_plat_priv));
+	if (ret)
+		goto err;
+
 	ret = platform_device_add(xhci);
 	if (ret) {
 		dev_err(dwc->dev, "failed to register xHCI device\n");
@@ -192,6 +218,7 @@ void dwc3_host_exit(struct dwc3 *dwc)
 	if (dwc->sys_wakeup)
 		device_init_wakeup(&dwc->xhci->dev, false);
 
+	dwc3_enable_susphy(dwc, false);
 	platform_device_unregister(dwc->xhci);
 	dwc->xhci = NULL;
 }
-- 
2.28.0

Re: [PATCH 1/2] usb: xhci-plat: Don't include xhci.h

[kernel test robot]

Hi Thinh,

kernel test robot noticed the following build errors:

[auto build test ERROR on 3d122e6d27e417a9fa91181922743df26b2cd679]

url:    https://github.com/intel-lab-lkp/linux/commits/Thinh-Nguyen/usb-xhci-plat-Don-t-include-xhci-h/20240417-074220
base:   3d122e6d27e417a9fa91181922743df26b2cd679
patch link:    https://lore.kernel.org/r/900465dc09f1c8e12c4df98d625b9985965951a8.1713310411.git.Thinh.Nguyen%40synopsys.com
patch subject: [PATCH 1/2] usb: xhci-plat: Don't include xhci.h
config: arc-allmodconfig (https://download.01.org/0day-ci/archive/20240417/202404171847.XPIgGzM6-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240417/202404171847.XPIgGzM6-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404171847.XPIgGzM6-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/usb/host/xhci-rzv2m.c: In function 'xhci_rzv2m_init_quirk':
>> drivers/usb/host/xhci-rzv2m.c:21:33: error: invalid use of undefined type 'struct usb_hcd'
      21 |         struct device *dev = hcd->self.controller;
         |                                 ^~
>> drivers/usb/host/xhci-rzv2m.c:23:32: error: invalid use of undefined type 'struct device'
      23 |         rzv2m_usb3drd_reset(dev->parent, true);
         |                                ^~
   drivers/usb/host/xhci-rzv2m.c: In function 'xhci_rzv2m_start':
   drivers/usb/host/xhci-rzv2m.c:32:16: error: invalid use of undefined type 'struct usb_hcd'
      32 |         if (hcd->regs) {
         |                ^~
>> drivers/usb/host/xhci-rzv2m.c:34:26: error: implicit declaration of function 'readl' [-Werror=implicit-function-declaration]
      34 |                 int_en = readl(hcd->regs + RZV2M_USB3_INTEN);
         |                          ^~~~~
   drivers/usb/host/xhci-rzv2m.c:34:35: error: invalid use of undefined type 'struct usb_hcd'
      34 |                 int_en = readl(hcd->regs + RZV2M_USB3_INTEN);
         |                                   ^~
>> drivers/usb/host/xhci-rzv2m.c:14:33: error: implicit declaration of function 'BIT' [-Werror=implicit-function-declaration]
      14 | #define RZV2M_USB3_INT_XHC_ENA  BIT(0)
         |                                 ^~~
   drivers/usb/host/xhci-rzv2m.c:16:34: note: in expansion of macro 'RZV2M_USB3_INT_XHC_ENA'
      16 | #define RZV2M_USB3_INT_ENA_VAL  (RZV2M_USB3_INT_XHC_ENA \
         |                                  ^~~~~~~~~~~~~~~~~~~~~~
   drivers/usb/host/xhci-rzv2m.c:35:27: note: in expansion of macro 'RZV2M_USB3_INT_ENA_VAL'
      35 |                 int_en |= RZV2M_USB3_INT_ENA_VAL;
         |                           ^~~~~~~~~~~~~~~~~~~~~~
>> drivers/usb/host/xhci-rzv2m.c:36:17: error: implicit declaration of function 'writel' [-Werror=implicit-function-declaration]
      36 |                 writel(int_en, hcd->regs + RZV2M_USB3_INTEN);
         |                 ^~~~~~
   drivers/usb/host/xhci-rzv2m.c:36:35: error: invalid use of undefined type 'struct usb_hcd'
      36 |                 writel(int_en, hcd->regs + RZV2M_USB3_INTEN);
         |                                   ^~
   cc1: some warnings being treated as errors


vim +21 drivers/usb/host/xhci-rzv2m.c

c52c9acc415eb6 Biju Das 2023-01-21  13  
c52c9acc415eb6 Biju Das 2023-01-21 @14  #define RZV2M_USB3_INT_XHC_ENA	BIT(0)
c52c9acc415eb6 Biju Das 2023-01-21  15  #define RZV2M_USB3_INT_HSE_ENA	BIT(2)
c52c9acc415eb6 Biju Das 2023-01-21  16  #define RZV2M_USB3_INT_ENA_VAL	(RZV2M_USB3_INT_XHC_ENA \
c52c9acc415eb6 Biju Das 2023-01-21  17  				 | RZV2M_USB3_INT_HSE_ENA)
c52c9acc415eb6 Biju Das 2023-01-21  18  
c52c9acc415eb6 Biju Das 2023-01-21  19  int xhci_rzv2m_init_quirk(struct usb_hcd *hcd)
c52c9acc415eb6 Biju Das 2023-01-21  20  {
c52c9acc415eb6 Biju Das 2023-01-21 @21  	struct device *dev = hcd->self.controller;
c52c9acc415eb6 Biju Das 2023-01-21  22  
c52c9acc415eb6 Biju Das 2023-01-21 @23  	rzv2m_usb3drd_reset(dev->parent, true);
c52c9acc415eb6 Biju Das 2023-01-21  24  
c52c9acc415eb6 Biju Das 2023-01-21  25  	return 0;
c52c9acc415eb6 Biju Das 2023-01-21  26  }
c52c9acc415eb6 Biju Das 2023-01-21  27  
c52c9acc415eb6 Biju Das 2023-01-21  28  void xhci_rzv2m_start(struct usb_hcd *hcd)
c52c9acc415eb6 Biju Das 2023-01-21  29  {
c52c9acc415eb6 Biju Das 2023-01-21  30  	u32 int_en;
c52c9acc415eb6 Biju Das 2023-01-21  31  
c52c9acc415eb6 Biju Das 2023-01-21  32  	if (hcd->regs) {
c52c9acc415eb6 Biju Das 2023-01-21  33  		/* Interrupt Enable */
c52c9acc415eb6 Biju Das 2023-01-21 @34  		int_en = readl(hcd->regs + RZV2M_USB3_INTEN);
c52c9acc415eb6 Biju Das 2023-01-21  35  		int_en |= RZV2M_USB3_INT_ENA_VAL;
c52c9acc415eb6 Biju Das 2023-01-21 @36  		writel(int_en, hcd->regs + RZV2M_USB3_INTEN);

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] usb: xhci-plat: Don't include xhci.h

[Greg Kroah-Hartman]

On Tue, Apr 16, 2024 at 11:41:36PM +0000, Thinh Nguyen wrote:
> The xhci_plat.h should not need to include the entire xhci.h header.
> This can cause redefinition in dwc3 if it selectively includes some xHCI
> definitions. This is a prerequisite change for a fix to disable suspend
> during initialization for dwc3.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> ---
>  drivers/usb/host/xhci-plat.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h
> index 2d15386f2c50..6475130eac4b 100644
> --- a/drivers/usb/host/xhci-plat.h
> +++ b/drivers/usb/host/xhci-plat.h
> @@ -8,7 +8,9 @@
>  #ifndef _XHCI_PLAT_H
>  #define _XHCI_PLAT_H
>  
> -#include "xhci.h"	/* for hcd_to_xhci() */
> +struct device;
> +struct platform_device;
> +struct usb_hcd;
>  
>  struct xhci_plat_priv {
>  	const char *firmware_name;
> -- 
> 2.28.0
> 

Seems to break the build :(

Re: [PATCH 1/2] usb: xhci-plat: Don't include xhci.h

[Thinh Nguyen]

On Wed, Apr 17, 2024, Greg Kroah-Hartman wrote:
> On Tue, Apr 16, 2024 at 11:41:36PM +0000, Thinh Nguyen wrote:
> > The xhci_plat.h should not need to include the entire xhci.h header.
> > This can cause redefinition in dwc3 if it selectively includes some xHCI
> > definitions. This is a prerequisite change for a fix to disable suspend
> > during initialization for dwc3.
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> > ---
> >  drivers/usb/host/xhci-plat.h | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h
> > index 2d15386f2c50..6475130eac4b 100644
> > --- a/drivers/usb/host/xhci-plat.h
> > +++ b/drivers/usb/host/xhci-plat.h
> > @@ -8,7 +8,9 @@
> >  #ifndef _XHCI_PLAT_H
> >  #define _XHCI_PLAT_H
> >  
> > -#include "xhci.h"	/* for hcd_to_xhci() */
> > +struct device;
> > +struct platform_device;
> > +struct usb_hcd;
> >  
> >  struct xhci_plat_priv {
> >  	const char *firmware_name;
> > -- 
> > 2.28.0
> > 
> 
> Seems to break the build :(

Oops.. I missed checking for xhci-rzv2m build.

Thanks,
Thinh

Re: [PATCH 2/2] usb: dwc3: core: Prevent phy suspend during init

[Roger Quadros]

Hello Thinh,

On 17/04/2024 02:41, Thinh Nguyen wrote:
> GUSB3PIPECTL.SUSPENDENABLE and GUSB2PHYCFG.SUSPHY should be cleared
> during initialization. Suspend during initialization can result in
> undefined behavior due to clock synchronization failure, which often
> seen as core soft reset timeout.
> 
> The programming guide recommended these bits to be cleared during
> initialization for DWC_usb3.0 version 1.94 and above (along with
> DWC_usb31 and DWC_usb32). The current check in the driver does not
> account if it's set by default setting from coreConsultant.
> 
> This is especially the case for DRD when switching mode to ensure the
> phy clocks are available to change mode. Depending on the
> platforms/design, some may be affected more than others. This is noted
> in the DWC_usb3x programming guide under the above registers.
> 
> Let's just disable them during driver load and mode switching. Restore
> them when the controller initialization completes.
> 
> Note that some platforms workaround this issue by disabling phy suspend
> through "snps,dis_u3_susphy_quirk" and "snps,dis_u2_susphy_quirk" when
> they should not need to.
> 
> Cc: stable@vger.kernel.org
> Fixes: 9ba3aca8fe82 ("usb: dwc3: Disable phy suspend after power-on reset")
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

This patch is causing system suspend failures on TI AM62 platforms [1]

I will try to explain why.
Before this patch, both DWC3_GUSB3PIPECTL_SUSPHY and DWC3_GUSB2PHYCFG_SUSPHY
bits (hence forth called 2 SUSPHY bits) were being set during initialization
and even during re-initialization after a system suspend/resume.

These bits are required to be set for system suspend/resume to work correctly
on AM62 platforms.

After this patch, the bits are only set when Host controller starts or
when Gadget driver starts.

On AM62 platform we have 2 USB controllers, one in Host and one in Dual role.
Just after boot, for the Host controller we have the 2 SUSPHY bits set but
for the Dual-Role controller, as no role has started the 2 SUSPHY bits are
not set. Thus system suspend resume will fail.

On the other hand, if we load a gadget driver just after boot then both
controllers have the 2 SUSPHY bits set and system suspend resume works for
the first time.
However, after system resume, the core is re-initialized so the 2 SUSPHY bits
are cleared for both controllers. For host controller it is never set again.
For gadget controller as gadget start is called, the 2 SUSPHY bits are set
again. The second system suspend resume will still fail as one controller
(Host) doesn't have the 2 SUSPHY bits set.

To summarize, the existing solution is not sufficient for us to have a
reliable behavior. We need the 2 SUSPHY bits to be set regardless of what
role we are in or whether the role has started or not.

My suggestion is to move back the SUSPHY enable to end of dwc3_core_init().
Then if SUSPHY needs to be disabled for DRD role switching, it should be
disabled and enabled exactly there.

What do you suggest?

[1] - https://lore.kernel.org/linux-arm-kernel/20240904194229.109886-1-msp@baylibre.com/

-- 
cheers,
-roger

> ---
>  drivers/usb/dwc3/core.c   | 90 +++++++++++++++++----------------------
>  drivers/usb/dwc3/core.h   |  1 +
>  drivers/usb/dwc3/gadget.c |  2 +
>  drivers/usb/dwc3/host.c   | 27 ++++++++++++
>  4 files changed, 68 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 31684cdaaae3..100041320e8d 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -104,6 +104,27 @@ static int dwc3_get_dr_mode(struct dwc3 *dwc)
>  	return 0;
>  }
>  
> +void dwc3_enable_susphy(struct dwc3 *dwc, bool enable)
> +{
> +	u32 reg;
> +
> +	reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
> +	if (enable && !dwc->dis_u3_susphy_quirk)
> +		reg |= DWC3_GUSB3PIPECTL_SUSPHY;
> +	else
> +		reg &= ~DWC3_GUSB3PIPECTL_SUSPHY;
> +
> +	dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
> +
> +	reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> +	if (enable && !dwc->dis_u2_susphy_quirk)
> +		reg |= DWC3_GUSB2PHYCFG_SUSPHY;
> +	else
> +		reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
> +
> +	dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> +}
> +
>  void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode)
>  {
>  	u32 reg;
> @@ -585,11 +606,8 @@ static int dwc3_core_ulpi_init(struct dwc3 *dwc)
>   */
>  static int dwc3_phy_setup(struct dwc3 *dwc)
>  {
> -	unsigned int hw_mode;
>  	u32 reg;
>  
> -	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
> -
>  	reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
>  
>  	/*
> @@ -599,21 +617,16 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
>  	reg &= ~DWC3_GUSB3PIPECTL_UX_EXIT_PX;
>  
>  	/*
> -	 * Above 1.94a, it is recommended to set DWC3_GUSB3PIPECTL_SUSPHY
> -	 * to '0' during coreConsultant configuration. So default value
> -	 * will be '0' when the core is reset. Application needs to set it
> -	 * to '1' after the core initialization is completed.
> -	 */
> -	if (!DWC3_VER_IS_WITHIN(DWC3, ANY, 194A))
> -		reg |= DWC3_GUSB3PIPECTL_SUSPHY;
> -
> -	/*
> -	 * For DRD controllers, GUSB3PIPECTL.SUSPENDENABLE must be cleared after
> -	 * power-on reset, and it can be set after core initialization, which is
> -	 * after device soft-reset during initialization.
> +	 * Above DWC_usb3.0 1.94a, it is recommended to set
> +	 * DWC3_GUSB3PIPECTL_SUSPHY to '0' during coreConsultant configuration.
> +	 * So default value will be '0' when the core is reset. Application
> +	 * needs to set it to '1' after the core initialization is completed.
> +	 *
> +	 * Similarly for DRD controllers, GUSB3PIPECTL.SUSPENDENABLE must be
> +	 * cleared after power-on reset, and it can be set after core
> +	 * initialization.
>  	 */
> -	if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD)
> -		reg &= ~DWC3_GUSB3PIPECTL_SUSPHY;
> +	reg &= ~DWC3_GUSB3PIPECTL_SUSPHY;
>  
>  	if (dwc->u2ss_inp3_quirk)
>  		reg |= DWC3_GUSB3PIPECTL_U2SSINP3OK;
> @@ -639,9 +652,6 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
>  	if (dwc->tx_de_emphasis_quirk)
>  		reg |= DWC3_GUSB3PIPECTL_TX_DEEPH(dwc->tx_de_emphasis);
>  
> -	if (dwc->dis_u3_susphy_quirk)
> -		reg &= ~DWC3_GUSB3PIPECTL_SUSPHY;
> -
>  	if (dwc->dis_del_phy_power_chg_quirk)
>  		reg &= ~DWC3_GUSB3PIPECTL_DEPOCHANGE;
>  
> @@ -689,24 +699,15 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
>  	}
>  
>  	/*
> -	 * Above 1.94a, it is recommended to set DWC3_GUSB2PHYCFG_SUSPHY to
> -	 * '0' during coreConsultant configuration. So default value will
> -	 * be '0' when the core is reset. Application needs to set it to
> -	 * '1' after the core initialization is completed.
> -	 */
> -	if (!DWC3_VER_IS_WITHIN(DWC3, ANY, 194A))
> -		reg |= DWC3_GUSB2PHYCFG_SUSPHY;
> -
> -	/*
> -	 * For DRD controllers, GUSB2PHYCFG.SUSPHY must be cleared after
> -	 * power-on reset, and it can be set after core initialization, which is
> -	 * after device soft-reset during initialization.
> +	 * Above DWC_usb3.0 1.94a, it is recommended to set
> +	 * DWC3_GUSB2PHYCFG_SUSPHY to '0' during coreConsultant configuration.
> +	 * So default value will be '0' when the core is reset. Application
> +	 * needs to set it to '1' after the core initialization is completed.
> +	 *
> +	 * Similarly for DRD controllers, GUSB2PHYCFG.SUSPHY must be cleared
> +	 * after power-on reset, and it can be set after core initialization.
>  	 */
> -	if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD)
> -		reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
> -
> -	if (dwc->dis_u2_susphy_quirk)
> -		reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
> +	reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
>  
>  	if (dwc->dis_enblslpm_quirk)
>  		reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM;
> @@ -1227,21 +1228,6 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  	if (ret)
>  		goto err_exit_phy;
>  
> -	if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD &&
> -	    !DWC3_VER_IS_WITHIN(DWC3, ANY, 194A)) {
> -		if (!dwc->dis_u3_susphy_quirk) {
> -			reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
> -			reg |= DWC3_GUSB3PIPECTL_SUSPHY;
> -			dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
> -		}
> -
> -		if (!dwc->dis_u2_susphy_quirk) {
> -			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> -			reg |= DWC3_GUSB2PHYCFG_SUSPHY;
> -			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> -		}
> -	}
> -
>  	dwc3_core_setup_global_control(dwc);
>  	dwc3_core_num_eps(dwc);
>  
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 7e80dd3d466b..180dd8d29287 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1580,6 +1580,7 @@ int dwc3_event_buffers_setup(struct dwc3 *dwc);
>  void dwc3_event_buffers_cleanup(struct dwc3 *dwc);
>  
>  int dwc3_core_soft_reset(struct dwc3 *dwc);
> +void dwc3_enable_susphy(struct dwc3 *dwc, bool enable);
>  
>  #if IS_ENABLED(CONFIG_USB_DWC3_HOST) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
>  int dwc3_host_init(struct dwc3 *dwc);
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 4df2661f6675..f94f68f1e7d2 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2924,6 +2924,7 @@ static int __dwc3_gadget_start(struct dwc3 *dwc)
>  	dwc3_ep0_out_start(dwc);
>  
>  	dwc3_gadget_enable_irq(dwc);
> +	dwc3_enable_susphy(dwc, true);
>  
>  	return 0;
>  
> @@ -4690,6 +4691,7 @@ void dwc3_gadget_exit(struct dwc3 *dwc)
>  	if (!dwc->gadget)
>  		return;
>  
> +	dwc3_enable_susphy(dwc, false);
>  	usb_del_gadget(dwc->gadget);
>  	dwc3_gadget_free_endpoints(dwc);
>  	usb_put_gadget(dwc->gadget);
> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> index 0204787df81d..a171b27a7845 100644
> --- a/drivers/usb/dwc3/host.c
> +++ b/drivers/usb/dwc3/host.c
> @@ -10,10 +10,13 @@
>  #include <linux/irq.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/usb.h>
> +#include <linux/usb/hcd.h>
>  
>  #include "../host/xhci-port.h"
>  #include "../host/xhci-ext-caps.h"
>  #include "../host/xhci-caps.h"
> +#include "../host/xhci-plat.h"
>  #include "core.h"
>  
>  #define XHCI_HCSPARAMS1		0x4
> @@ -57,6 +60,24 @@ static void dwc3_power_off_all_roothub_ports(struct dwc3 *dwc)
>  	}
>  }
>  
> +static void dwc3_xhci_plat_start(struct usb_hcd *hcd)
> +{
> +	struct platform_device *pdev;
> +	struct dwc3 *dwc;
> +
> +	if (!usb_hcd_is_primary_hcd(hcd))
> +		return;
> +
> +	pdev = to_platform_device(hcd->self.controller);
> +	dwc = dev_get_drvdata(pdev->dev.parent);
> +
> +	dwc3_enable_susphy(dwc, true);
> +}
> +
> +static const struct xhci_plat_priv dwc3_xhci_plat_quirk = {
> +	.plat_start = dwc3_xhci_plat_start,
> +};
> +
>  static void dwc3_host_fill_xhci_irq_res(struct dwc3 *dwc,
>  					int irq, char *name)
>  {
> @@ -167,6 +188,11 @@ int dwc3_host_init(struct dwc3 *dwc)
>  		}
>  	}
>  
> +	ret = platform_device_add_data(xhci, &dwc3_xhci_plat_quirk,
> +				       sizeof(struct xhci_plat_priv));
> +	if (ret)
> +		goto err;
> +
>  	ret = platform_device_add(xhci);
>  	if (ret) {
>  		dev_err(dwc->dev, "failed to register xHCI device\n");
> @@ -192,6 +218,7 @@ void dwc3_host_exit(struct dwc3 *dwc)
>  	if (dwc->sys_wakeup)
>  		device_init_wakeup(&dwc->xhci->dev, false);
>  
> +	dwc3_enable_susphy(dwc, false);
>  	platform_device_unregister(dwc->xhci);
>  	dwc->xhci = NULL;
>  }

Re: [PATCH 2/2] usb: dwc3: core: Prevent phy suspend during init

[Thinh Nguyen]

Hi Roger,

On Wed, Sep 25, 2024, Roger Quadros wrote:
> Hello Thinh,
> 
> On 17/04/2024 02:41, Thinh Nguyen wrote:
> > GUSB3PIPECTL.SUSPENDENABLE and GUSB2PHYCFG.SUSPHY should be cleared
> > during initialization. Suspend during initialization can result in
> > undefined behavior due to clock synchronization failure, which often
> > seen as core soft reset timeout.
> > 
> > The programming guide recommended these bits to be cleared during
> > initialization for DWC_usb3.0 version 1.94 and above (along with
> > DWC_usb31 and DWC_usb32). The current check in the driver does not
> > account if it's set by default setting from coreConsultant.
> > 
> > This is especially the case for DRD when switching mode to ensure the
> > phy clocks are available to change mode. Depending on the
> > platforms/design, some may be affected more than others. This is noted
> > in the DWC_usb3x programming guide under the above registers.
> > 
> > Let's just disable them during driver load and mode switching. Restore
> > them when the controller initialization completes.
> > 
> > Note that some platforms workaround this issue by disabling phy suspend
> > through "snps,dis_u3_susphy_quirk" and "snps,dis_u2_susphy_quirk" when
> > they should not need to.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 9ba3aca8fe82 ("usb: dwc3: Disable phy suspend after power-on reset")
> > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> 
> This patch is causing system suspend failures on TI AM62 platforms [1]
> 
> I will try to explain why.
> Before this patch, both DWC3_GUSB3PIPECTL_SUSPHY and DWC3_GUSB2PHYCFG_SUSPHY
> bits (hence forth called 2 SUSPHY bits) were being set during initialization
> and even during re-initialization after a system suspend/resume.
> 
> These bits are required to be set for system suspend/resume to work correctly
> on AM62 platforms.

Is it only for suspend or both suspend and resume?

> 
> After this patch, the bits are only set when Host controller starts or
> when Gadget driver starts.
> 
> On AM62 platform we have 2 USB controllers, one in Host and one in Dual role.
> Just after boot, for the Host controller we have the 2 SUSPHY bits set but
> for the Dual-Role controller, as no role has started the 2 SUSPHY bits are
> not set. Thus system suspend resume will fail.
> 
> On the other hand, if we load a gadget driver just after boot then both
> controllers have the 2 SUSPHY bits set and system suspend resume works for
> the first time.
> However, after system resume, the core is re-initialized so the 2 SUSPHY bits
> are cleared for both controllers. For host controller it is never set again.
> For gadget controller as gadget start is called, the 2 SUSPHY bits are set
> again. The second system suspend resume will still fail as one controller
> (Host) doesn't have the 2 SUSPHY bits set.
> 
> To summarize, the existing solution is not sufficient for us to have a
> reliable behavior. We need the 2 SUSPHY bits to be set regardless of what
> role we are in or whether the role has started or not.
> 
> My suggestion is to move back the SUSPHY enable to end of dwc3_core_init().
> Then if SUSPHY needs to be disabled for DRD role switching, it should be
> disabled and enabled exactly there.
> 
> What do you suggest?
> 
> [1] - https://urldefense.com/v3/__https://lore.kernel.org/linux-arm-kernel/20240904194229.109886-1-msp@baylibre.com/__;!!A4F2R9G_pg!Y10q3gwCzryOoiXpk6DMGn74iFQIg6GloY10J16kWCbqwgS1Algo5HRg05vm38dMw8n47qmKpqJlyXt9Kqlm$ 
> 

Thanks for reporting the issue.

This is quite an interesting behavior. As you said, we will need to
isolate this change to only during DRD role switch.

We may not necessarily just enable at the end of dwc3_core_init() since
that would keep the SUSPHY bits on during the DRD role switch. If this
issue only occurs before suspend, perhaps we can check and set these
bits during suspend or dwc3_core_exit() instead?

BR,
Thinh

Re: [PATCH 2/2] usb: dwc3: core: Prevent phy suspend during init

[Roger Quadros]

On 27/09/2024 00:51, Thinh Nguyen wrote:
> Hi Roger,
> 
> On Wed, Sep 25, 2024, Roger Quadros wrote:
>> Hello Thinh,
>>
>> On 17/04/2024 02:41, Thinh Nguyen wrote:
>>> GUSB3PIPECTL.SUSPENDENABLE and GUSB2PHYCFG.SUSPHY should be cleared
>>> during initialization. Suspend during initialization can result in
>>> undefined behavior due to clock synchronization failure, which often
>>> seen as core soft reset timeout.
>>>
>>> The programming guide recommended these bits to be cleared during
>>> initialization for DWC_usb3.0 version 1.94 and above (along with
>>> DWC_usb31 and DWC_usb32). The current check in the driver does not
>>> account if it's set by default setting from coreConsultant.
>>>
>>> This is especially the case for DRD when switching mode to ensure the
>>> phy clocks are available to change mode. Depending on the
>>> platforms/design, some may be affected more than others. This is noted
>>> in the DWC_usb3x programming guide under the above registers.
>>>
>>> Let's just disable them during driver load and mode switching. Restore
>>> them when the controller initialization completes.
>>>
>>> Note that some platforms workaround this issue by disabling phy suspend
>>> through "snps,dis_u3_susphy_quirk" and "snps,dis_u2_susphy_quirk" when
>>> they should not need to.
>>>
>>> Cc: stable@vger.kernel.org
>>> Fixes: 9ba3aca8fe82 ("usb: dwc3: Disable phy suspend after power-on reset")
>>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>
>> This patch is causing system suspend failures on TI AM62 platforms [1]
>>
>> I will try to explain why.
>> Before this patch, both DWC3_GUSB3PIPECTL_SUSPHY and DWC3_GUSB2PHYCFG_SUSPHY
>> bits (hence forth called 2 SUSPHY bits) were being set during initialization
>> and even during re-initialization after a system suspend/resume.
>>
>> These bits are required to be set for system suspend/resume to work correctly
>> on AM62 platforms.
> 
> Is it only for suspend or both suspend and resume?

I'm sure about suspend. It is not possible to toggle those bits while in system
suspend so we can't really say if it is required exclusively for system resume or not.

> 
>>
>> After this patch, the bits are only set when Host controller starts or
>> when Gadget driver starts.
>>
>> On AM62 platform we have 2 USB controllers, one in Host and one in Dual role.
>> Just after boot, for the Host controller we have the 2 SUSPHY bits set but
>> for the Dual-Role controller, as no role has started the 2 SUSPHY bits are
>> not set. Thus system suspend resume will fail.
>>
>> On the other hand, if we load a gadget driver just after boot then both
>> controllers have the 2 SUSPHY bits set and system suspend resume works for
>> the first time.
>> However, after system resume, the core is re-initialized so the 2 SUSPHY bits
>> are cleared for both controllers. For host controller it is never set again.
>> For gadget controller as gadget start is called, the 2 SUSPHY bits are set
>> again. The second system suspend resume will still fail as one controller
>> (Host) doesn't have the 2 SUSPHY bits set.
>>
>> To summarize, the existing solution is not sufficient for us to have a
>> reliable behavior. We need the 2 SUSPHY bits to be set regardless of what
>> role we are in or whether the role has started or not.
>>
>> My suggestion is to move back the SUSPHY enable to end of dwc3_core_init().
>> Then if SUSPHY needs to be disabled for DRD role switching, it should be
>> disabled and enabled exactly there.
>>
>> What do you suggest?
>>
>> [1] - https://urldefense.com/v3/__https://lore.kernel.org/linux-arm-kernel/20240904194229.109886-1-msp@baylibre.com/__;!!A4F2R9G_pg!Y10q3gwCzryOoiXpk6DMGn74iFQIg6GloY10J16kWCbqwgS1Algo5HRg05vm38dMw8n47qmKpqJlyXt9Kqlm$ 
>>
> 
> Thanks for reporting the issue.
> 
> This is quite an interesting behavior. As you said, we will need to
> isolate this change to only during DRD role switch.
> 
> We may not necessarily just enable at the end of dwc3_core_init() since
> that would keep the SUSPHY bits on during the DRD role switch. If this
> issue only occurs before suspend, perhaps we can check and set these
> bits during suspend or dwc3_core_exit() instead?

dwc3_core_exit() is not always called in the system suspend path so it
may not be sufficient.

Any issues if we set this these bits at the end of dwc3_suspend_common()
irrespective of runtime suspend or system suspend and operating role?
And should we restore these bits in dwc3_resume_common() to the state they
were before dwc3_suspend_common()?

-- 
cheers,
-roger

Re: [PATCH 2/2] usb: dwc3: core: Prevent phy suspend during init

[Thinh Nguyen]

On Fri, Sep 27, 2024, Roger Quadros wrote:
> 
> 
> On 27/09/2024 00:51, Thinh Nguyen wrote:
> > Hi Roger,
> > 
> > On Wed, Sep 25, 2024, Roger Quadros wrote:
> >> Hello Thinh,
> >>
> >> On 17/04/2024 02:41, Thinh Nguyen wrote:
> >>> GUSB3PIPECTL.SUSPENDENABLE and GUSB2PHYCFG.SUSPHY should be cleared
> >>> during initialization. Suspend during initialization can result in
> >>> undefined behavior due to clock synchronization failure, which often
> >>> seen as core soft reset timeout.
> >>>
> >>> The programming guide recommended these bits to be cleared during
> >>> initialization for DWC_usb3.0 version 1.94 and above (along with
> >>> DWC_usb31 and DWC_usb32). The current check in the driver does not
> >>> account if it's set by default setting from coreConsultant.
> >>>
> >>> This is especially the case for DRD when switching mode to ensure the
> >>> phy clocks are available to change mode. Depending on the
> >>> platforms/design, some may be affected more than others. This is noted
> >>> in the DWC_usb3x programming guide under the above registers.
> >>>
> >>> Let's just disable them during driver load and mode switching. Restore
> >>> them when the controller initialization completes.
> >>>
> >>> Note that some platforms workaround this issue by disabling phy suspend
> >>> through "snps,dis_u3_susphy_quirk" and "snps,dis_u2_susphy_quirk" when
> >>> they should not need to.
> >>>
> >>> Cc: stable@vger.kernel.org
> >>> Fixes: 9ba3aca8fe82 ("usb: dwc3: Disable phy suspend after power-on reset")
> >>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> >>
> >> This patch is causing system suspend failures on TI AM62 platforms [1]
> >>
> >> I will try to explain why.
> >> Before this patch, both DWC3_GUSB3PIPECTL_SUSPHY and DWC3_GUSB2PHYCFG_SUSPHY
> >> bits (hence forth called 2 SUSPHY bits) were being set during initialization
> >> and even during re-initialization after a system suspend/resume.
> >>
> >> These bits are required to be set for system suspend/resume to work correctly
> >> on AM62 platforms.
> > 
> > Is it only for suspend or both suspend and resume?
> 
> I'm sure about suspend. It is not possible to toggle those bits while in system
> suspend so we can't really say if it is required exclusively for system resume or not.
> 
> > 
> >>
> >> After this patch, the bits are only set when Host controller starts or
> >> when Gadget driver starts.
> >>
> >> On AM62 platform we have 2 USB controllers, one in Host and one in Dual role.
> >> Just after boot, for the Host controller we have the 2 SUSPHY bits set but
> >> for the Dual-Role controller, as no role has started the 2 SUSPHY bits are
> >> not set. Thus system suspend resume will fail.
> >>
> >> On the other hand, if we load a gadget driver just after boot then both
> >> controllers have the 2 SUSPHY bits set and system suspend resume works for
> >> the first time.
> >> However, after system resume, the core is re-initialized so the 2 SUSPHY bits
> >> are cleared for both controllers. For host controller it is never set again.
> >> For gadget controller as gadget start is called, the 2 SUSPHY bits are set
> >> again. The second system suspend resume will still fail as one controller
> >> (Host) doesn't have the 2 SUSPHY bits set.
> >>
> >> To summarize, the existing solution is not sufficient for us to have a
> >> reliable behavior. We need the 2 SUSPHY bits to be set regardless of what
> >> role we are in or whether the role has started or not.
> >>
> >> My suggestion is to move back the SUSPHY enable to end of dwc3_core_init().
> >> Then if SUSPHY needs to be disabled for DRD role switching, it should be
> >> disabled and enabled exactly there.
> >>
> >> What do you suggest?
> >>
> >> [1] - https://urldefense.com/v3/__https://lore.kernel.org/linux-arm-kernel/20240904194229.109886-1-msp@baylibre.com/__;!!A4F2R9G_pg!Y10q3gwCzryOoiXpk6DMGn74iFQIg6GloY10J16kWCbqwgS1Algo5HRg05vm38dMw8n47qmKpqJlyXt9Kqlm$ 
> >>
> > 
> > Thanks for reporting the issue.
> > 
> > This is quite an interesting behavior. As you said, we will need to
> > isolate this change to only during DRD role switch.
> > 
> > We may not necessarily just enable at the end of dwc3_core_init() since
> > that would keep the SUSPHY bits on during the DRD role switch. If this
> > issue only occurs before suspend, perhaps we can check and set these
> > bits during suspend or dwc3_core_exit() instead?
> 
> dwc3_core_exit() is not always called in the system suspend path so it
> may not be sufficient.
> 
> Any issues if we set this these bits at the end of dwc3_suspend_common()
> irrespective of runtime suspend or system suspend and operating role?

There should be no issue at this point. The problem occurs during
initialization that involves initializing the usb role.

> And should we restore these bits in dwc3_resume_common() to the state they
> were before dwc3_suspend_common()?
> 

Sounds good to me! Would you mind send a fix patch?

Thanks,
Thinh

Re: [PATCH 2/2] usb: dwc3: core: Prevent phy suspend during init

[Roger Quadros]

On 01/10/2024 04:00, Thinh Nguyen wrote:
> On Fri, Sep 27, 2024, Roger Quadros wrote:
>>
>>
>> On 27/09/2024 00:51, Thinh Nguyen wrote:
>>> Hi Roger,
>>>
>>> On Wed, Sep 25, 2024, Roger Quadros wrote:
>>>> Hello Thinh,
>>>>
>>>> On 17/04/2024 02:41, Thinh Nguyen wrote:
>>>>> GUSB3PIPECTL.SUSPENDENABLE and GUSB2PHYCFG.SUSPHY should be cleared
>>>>> during initialization. Suspend during initialization can result in
>>>>> undefined behavior due to clock synchronization failure, which often
>>>>> seen as core soft reset timeout.
>>>>>
>>>>> The programming guide recommended these bits to be cleared during
>>>>> initialization for DWC_usb3.0 version 1.94 and above (along with
>>>>> DWC_usb31 and DWC_usb32). The current check in the driver does not
>>>>> account if it's set by default setting from coreConsultant.
>>>>>
>>>>> This is especially the case for DRD when switching mode to ensure the
>>>>> phy clocks are available to change mode. Depending on the
>>>>> platforms/design, some may be affected more than others. This is noted
>>>>> in the DWC_usb3x programming guide under the above registers.
>>>>>
>>>>> Let's just disable them during driver load and mode switching. Restore
>>>>> them when the controller initialization completes.
>>>>>
>>>>> Note that some platforms workaround this issue by disabling phy suspend
>>>>> through "snps,dis_u3_susphy_quirk" and "snps,dis_u2_susphy_quirk" when
>>>>> they should not need to.
>>>>>
>>>>> Cc: stable@vger.kernel.org
>>>>> Fixes: 9ba3aca8fe82 ("usb: dwc3: Disable phy suspend after power-on reset")
>>>>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>>>
>>>> This patch is causing system suspend failures on TI AM62 platforms [1]
>>>>
>>>> I will try to explain why.
>>>> Before this patch, both DWC3_GUSB3PIPECTL_SUSPHY and DWC3_GUSB2PHYCFG_SUSPHY
>>>> bits (hence forth called 2 SUSPHY bits) were being set during initialization
>>>> and even during re-initialization after a system suspend/resume.
>>>>
>>>> These bits are required to be set for system suspend/resume to work correctly
>>>> on AM62 platforms.
>>>
>>> Is it only for suspend or both suspend and resume?
>>
>> I'm sure about suspend. It is not possible to toggle those bits while in system
>> suspend so we can't really say if it is required exclusively for system resume or not.
>>
>>>
>>>>
>>>> After this patch, the bits are only set when Host controller starts or
>>>> when Gadget driver starts.
>>>>
>>>> On AM62 platform we have 2 USB controllers, one in Host and one in Dual role.
>>>> Just after boot, for the Host controller we have the 2 SUSPHY bits set but
>>>> for the Dual-Role controller, as no role has started the 2 SUSPHY bits are
>>>> not set. Thus system suspend resume will fail.
>>>>
>>>> On the other hand, if we load a gadget driver just after boot then both
>>>> controllers have the 2 SUSPHY bits set and system suspend resume works for
>>>> the first time.
>>>> However, after system resume, the core is re-initialized so the 2 SUSPHY bits
>>>> are cleared for both controllers. For host controller it is never set again.
>>>> For gadget controller as gadget start is called, the 2 SUSPHY bits are set
>>>> again. The second system suspend resume will still fail as one controller
>>>> (Host) doesn't have the 2 SUSPHY bits set.
>>>>
>>>> To summarize, the existing solution is not sufficient for us to have a
>>>> reliable behavior. We need the 2 SUSPHY bits to be set regardless of what
>>>> role we are in or whether the role has started or not.
>>>>
>>>> My suggestion is to move back the SUSPHY enable to end of dwc3_core_init().
>>>> Then if SUSPHY needs to be disabled for DRD role switching, it should be
>>>> disabled and enabled exactly there.
>>>>
>>>> What do you suggest?
>>>>
>>>> [1] - https://urldefense.com/v3/__https://lore.kernel.org/linux-arm-kernel/20240904194229.109886-1-msp@baylibre.com/__;!!A4F2R9G_pg!Y10q3gwCzryOoiXpk6DMGn74iFQIg6GloY10J16kWCbqwgS1Algo5HRg05vm38dMw8n47qmKpqJlyXt9Kqlm$ 
>>>>
>>>
>>> Thanks for reporting the issue.
>>>
>>> This is quite an interesting behavior. As you said, we will need to
>>> isolate this change to only during DRD role switch.
>>>
>>> We may not necessarily just enable at the end of dwc3_core_init() since
>>> that would keep the SUSPHY bits on during the DRD role switch. If this
>>> issue only occurs before suspend, perhaps we can check and set these
>>> bits during suspend or dwc3_core_exit() instead?
>>
>> dwc3_core_exit() is not always called in the system suspend path so it
>> may not be sufficient.
>>
>> Any issues if we set this these bits at the end of dwc3_suspend_common()
>> irrespective of runtime suspend or system suspend and operating role?
> 
> There should be no issue at this point. The problem occurs during
> initialization that involves initializing the usb role.
> 
>> And should we restore these bits in dwc3_resume_common() to the state they
>> were before dwc3_suspend_common()?
>>
> 
> Sounds good to me! Would you mind send a fix patch?

Thanks for your suggestions. Yes, I will send a fix soon.

-- 
cheers,
-roger

Re: [PATCH 2/2] usb: dwc3: core: Prevent phy suspend during init

[Chris Morgan]

On Wed, Sep 25, 2024 at 10:50:05AM +0300, Roger Quadros wrote:
> Hello Thinh,
> 
> On 17/04/2024 02:41, Thinh Nguyen wrote:
> > GUSB3PIPECTL.SUSPENDENABLE and GUSB2PHYCFG.SUSPHY should be cleared
> > during initialization. Suspend during initialization can result in
> > undefined behavior due to clock synchronization failure, which often
> > seen as core soft reset timeout.
> > 
> > The programming guide recommended these bits to be cleared during
> > initialization for DWC_usb3.0 version 1.94 and above (along with
> > DWC_usb31 and DWC_usb32). The current check in the driver does not
> > account if it's set by default setting from coreConsultant.
> > 
> > This is especially the case for DRD when switching mode to ensure the
> > phy clocks are available to change mode. Depending on the
> > platforms/design, some may be affected more than others. This is noted
> > in the DWC_usb3x programming guide under the above registers.
> > 
> > Let's just disable them during driver load and mode switching. Restore
> > them when the controller initialization completes.
> > 
> > Note that some platforms workaround this issue by disabling phy suspend
> > through "snps,dis_u3_susphy_quirk" and "snps,dis_u2_susphy_quirk" when
> > they should not need to.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 9ba3aca8fe82 ("usb: dwc3: Disable phy suspend after power-on reset")
> > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> 
> This patch is causing system suspend failures on TI AM62 platforms [1]
> 
> I will try to explain why.
> Before this patch, both DWC3_GUSB3PIPECTL_SUSPHY and DWC3_GUSB2PHYCFG_SUSPHY
> bits (hence forth called 2 SUSPHY bits) were being set during initialization
> and even during re-initialization after a system suspend/resume.
> 
> These bits are required to be set for system suspend/resume to work correctly
> on AM62 platforms.
> 
> After this patch, the bits are only set when Host controller starts or
> when Gadget driver starts.
> 
> On AM62 platform we have 2 USB controllers, one in Host and one in Dual role.
> Just after boot, for the Host controller we have the 2 SUSPHY bits set but
> for the Dual-Role controller, as no role has started the 2 SUSPHY bits are
> not set. Thus system suspend resume will fail.
> 
> On the other hand, if we load a gadget driver just after boot then both
> controllers have the 2 SUSPHY bits set and system suspend resume works for
> the first time.
> However, after system resume, the core is re-initialized so the 2 SUSPHY bits
> are cleared for both controllers. For host controller it is never set again.
> For gadget controller as gadget start is called, the 2 SUSPHY bits are set
> again. The second system suspend resume will still fail as one controller
> (Host) doesn't have the 2 SUSPHY bits set.
> 
> To summarize, the existing solution is not sufficient for us to have a
> reliable behavior. We need the 2 SUSPHY bits to be set regardless of what
> role we are in or whether the role has started or not.
> 
> My suggestion is to move back the SUSPHY enable to end of dwc3_core_init().
> Then if SUSPHY needs to be disabled for DRD role switching, it should be
> disabled and enabled exactly there.
> 
> What do you suggest?
> 
> [1] - https://lore.kernel.org/linux-arm-kernel/20240904194229.109886-1-msp@baylibre.com/
> 
> -- 
> cheers,
> -roger
> 
> > ---
> >  drivers/usb/dwc3/core.c   | 90 +++++++++++++++++----------------------
> >  drivers/usb/dwc3/core.h   |  1 +
> >  drivers/usb/dwc3/gadget.c |  2 +
> >  drivers/usb/dwc3/host.c   | 27 ++++++++++++
> >  4 files changed, 68 insertions(+), 52 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 31684cdaaae3..100041320e8d 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -104,6 +104,27 @@ static int dwc3_get_dr_mode(struct dwc3 *dwc)
> >  	return 0;
> >  }
> >  
> > +void dwc3_enable_susphy(struct dwc3 *dwc, bool enable)
> > +{
> > +	u32 reg;
> > +
> > +	reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
> > +	if (enable && !dwc->dis_u3_susphy_quirk)
> > +		reg |= DWC3_GUSB3PIPECTL_SUSPHY;
> > +	else
> > +		reg &= ~DWC3_GUSB3PIPECTL_SUSPHY;
> > +
> > +	dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
> > +
> > +	reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> > +	if (enable && !dwc->dis_u2_susphy_quirk)
> > +		reg |= DWC3_GUSB2PHYCFG_SUSPHY;
> > +	else
> > +		reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
> > +
> > +	dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> > +}
> > +
> >  void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode)
> >  {
> >  	u32 reg;
> > @@ -585,11 +606,8 @@ static int dwc3_core_ulpi_init(struct dwc3 *dwc)
> >   */
> >  static int dwc3_phy_setup(struct dwc3 *dwc)
> >  {
> > -	unsigned int hw_mode;
> >  	u32 reg;
> >  
> > -	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
> > -
> >  	reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
> >  
> >  	/*
> > @@ -599,21 +617,16 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
> >  	reg &= ~DWC3_GUSB3PIPECTL_UX_EXIT_PX;
> >  
> >  	/*
> > -	 * Above 1.94a, it is recommended to set DWC3_GUSB3PIPECTL_SUSPHY
> > -	 * to '0' during coreConsultant configuration. So default value
> > -	 * will be '0' when the core is reset. Application needs to set it
> > -	 * to '1' after the core initialization is completed.
> > -	 */
> > -	if (!DWC3_VER_IS_WITHIN(DWC3, ANY, 194A))
> > -		reg |= DWC3_GUSB3PIPECTL_SUSPHY;
> > -
> > -	/*
> > -	 * For DRD controllers, GUSB3PIPECTL.SUSPENDENABLE must be cleared after
> > -	 * power-on reset, and it can be set after core initialization, which is
> > -	 * after device soft-reset during initialization.
> > +	 * Above DWC_usb3.0 1.94a, it is recommended to set
> > +	 * DWC3_GUSB3PIPECTL_SUSPHY to '0' during coreConsultant configuration.
> > +	 * So default value will be '0' when the core is reset. Application
> > +	 * needs to set it to '1' after the core initialization is completed.
> > +	 *
> > +	 * Similarly for DRD controllers, GUSB3PIPECTL.SUSPENDENABLE must be
> > +	 * cleared after power-on reset, and it can be set after core
> > +	 * initialization.
> >  	 */
> > -	if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD)
> > -		reg &= ~DWC3_GUSB3PIPECTL_SUSPHY;
> > +	reg &= ~DWC3_GUSB3PIPECTL_SUSPHY;
> >  
> >  	if (dwc->u2ss_inp3_quirk)
> >  		reg |= DWC3_GUSB3PIPECTL_U2SSINP3OK;
> > @@ -639,9 +652,6 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
> >  	if (dwc->tx_de_emphasis_quirk)
> >  		reg |= DWC3_GUSB3PIPECTL_TX_DEEPH(dwc->tx_de_emphasis);
> >  
> > -	if (dwc->dis_u3_susphy_quirk)
> > -		reg &= ~DWC3_GUSB3PIPECTL_SUSPHY;
> > -
> >  	if (dwc->dis_del_phy_power_chg_quirk)
> >  		reg &= ~DWC3_GUSB3PIPECTL_DEPOCHANGE;
> >  
> > @@ -689,24 +699,15 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
> >  	}
> >  
> >  	/*
> > -	 * Above 1.94a, it is recommended to set DWC3_GUSB2PHYCFG_SUSPHY to
> > -	 * '0' during coreConsultant configuration. So default value will
> > -	 * be '0' when the core is reset. Application needs to set it to
> > -	 * '1' after the core initialization is completed.
> > -	 */
> > -	if (!DWC3_VER_IS_WITHIN(DWC3, ANY, 194A))
> > -		reg |= DWC3_GUSB2PHYCFG_SUSPHY;
> > -
> > -	/*
> > -	 * For DRD controllers, GUSB2PHYCFG.SUSPHY must be cleared after
> > -	 * power-on reset, and it can be set after core initialization, which is
> > -	 * after device soft-reset during initialization.
> > +	 * Above DWC_usb3.0 1.94a, it is recommended to set
> > +	 * DWC3_GUSB2PHYCFG_SUSPHY to '0' during coreConsultant configuration.
> > +	 * So default value will be '0' when the core is reset. Application
> > +	 * needs to set it to '1' after the core initialization is completed.
> > +	 *
> > +	 * Similarly for DRD controllers, GUSB2PHYCFG.SUSPHY must be cleared
> > +	 * after power-on reset, and it can be set after core initialization.
> >  	 */
> > -	if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD)
> > -		reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
> > -
> > -	if (dwc->dis_u2_susphy_quirk)
> > -		reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
> > +	reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
> >  
> >  	if (dwc->dis_enblslpm_quirk)
> >  		reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM;
> > @@ -1227,21 +1228,6 @@ static int dwc3_core_init(struct dwc3 *dwc)
> >  	if (ret)
> >  		goto err_exit_phy;
> >  
> > -	if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD &&
> > -	    !DWC3_VER_IS_WITHIN(DWC3, ANY, 194A)) {
> > -		if (!dwc->dis_u3_susphy_quirk) {
> > -			reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
> > -			reg |= DWC3_GUSB3PIPECTL_SUSPHY;
> > -			dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
> > -		}
> > -
> > -		if (!dwc->dis_u2_susphy_quirk) {
> > -			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> > -			reg |= DWC3_GUSB2PHYCFG_SUSPHY;
> > -			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> > -		}
> > -	}
> > -
> >  	dwc3_core_setup_global_control(dwc);
> >  	dwc3_core_num_eps(dwc);
> >  
> > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> > index 7e80dd3d466b..180dd8d29287 100644
> > --- a/drivers/usb/dwc3/core.h
> > +++ b/drivers/usb/dwc3/core.h
> > @@ -1580,6 +1580,7 @@ int dwc3_event_buffers_setup(struct dwc3 *dwc);
> >  void dwc3_event_buffers_cleanup(struct dwc3 *dwc);
> >  
> >  int dwc3_core_soft_reset(struct dwc3 *dwc);
> > +void dwc3_enable_susphy(struct dwc3 *dwc, bool enable);
> >  
> >  #if IS_ENABLED(CONFIG_USB_DWC3_HOST) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
> >  int dwc3_host_init(struct dwc3 *dwc);
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 4df2661f6675..f94f68f1e7d2 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -2924,6 +2924,7 @@ static int __dwc3_gadget_start(struct dwc3 *dwc)
> >  	dwc3_ep0_out_start(dwc);
> >  
> >  	dwc3_gadget_enable_irq(dwc);
> > +	dwc3_enable_susphy(dwc, true);
> >  
> >  	return 0;
> >  
> > @@ -4690,6 +4691,7 @@ void dwc3_gadget_exit(struct dwc3 *dwc)
> >  	if (!dwc->gadget)
> >  		return;
> >  
> > +	dwc3_enable_susphy(dwc, false);
> >  	usb_del_gadget(dwc->gadget);
> >  	dwc3_gadget_free_endpoints(dwc);
> >  	usb_put_gadget(dwc->gadget);
> > diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> > index 0204787df81d..a171b27a7845 100644
> > --- a/drivers/usb/dwc3/host.c
> > +++ b/drivers/usb/dwc3/host.c
> > @@ -10,10 +10,13 @@
> >  #include <linux/irq.h>
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/usb.h>
> > +#include <linux/usb/hcd.h>
> >  
> >  #include "../host/xhci-port.h"
> >  #include "../host/xhci-ext-caps.h"
> >  #include "../host/xhci-caps.h"
> > +#include "../host/xhci-plat.h"
> >  #include "core.h"
> >  
> >  #define XHCI_HCSPARAMS1		0x4
> > @@ -57,6 +60,24 @@ static void dwc3_power_off_all_roothub_ports(struct dwc3 *dwc)
> >  	}
> >  }
> >  
> > +static void dwc3_xhci_plat_start(struct usb_hcd *hcd)
> > +{
> > +	struct platform_device *pdev;
> > +	struct dwc3 *dwc;
> > +
> > +	if (!usb_hcd_is_primary_hcd(hcd))
> > +		return;
> > +
> > +	pdev = to_platform_device(hcd->self.controller);
> > +	dwc = dev_get_drvdata(pdev->dev.parent);
> > +
> > +	dwc3_enable_susphy(dwc, true);
> > +}
> > +
> > +static const struct xhci_plat_priv dwc3_xhci_plat_quirk = {
> > +	.plat_start = dwc3_xhci_plat_start,
> > +};
> > +
> >  static void dwc3_host_fill_xhci_irq_res(struct dwc3 *dwc,
> >  					int irq, char *name)
> >  {
> > @@ -167,6 +188,11 @@ int dwc3_host_init(struct dwc3 *dwc)
> >  		}
> >  	}
> >  
> > +	ret = platform_device_add_data(xhci, &dwc3_xhci_plat_quirk,
> > +				       sizeof(struct xhci_plat_priv));
> > +	if (ret)
> > +		goto err;
> > +
> >  	ret = platform_device_add(xhci);
> >  	if (ret) {
> >  		dev_err(dwc->dev, "failed to register xHCI device\n");
> > @@ -192,6 +218,7 @@ void dwc3_host_exit(struct dwc3 *dwc)
> >  	if (dwc->sys_wakeup)
> >  		device_init_wakeup(&dwc->xhci->dev, false);
> >  
> > +	dwc3_enable_susphy(dwc, false);
> >  	platform_device_unregister(dwc->xhci);
> >  	dwc->xhci = NULL;
> >  }
> 

This patch seems to break system suspend on at least the Rockchip
RK3566 platform. I noticed that I was no longer able to suspend
and git bisect led me to this patch.

My kernel message log shows the following, at which point it freezes
and does not allow me to resume from suspend:

[   27.235049] PM: suspend entry (deep)
[   27.871641] Filesystems sync: 0.636 seconds
[   27.885320] Freezing user space processes
[   27.886932] Freezing user space processes completed (elapsed 0.001 seconds)
[   27.887642] OOM killer disabled.
[   27.887981] Freezing remaining freezable tasks
[   27.889655] Freezing remaining freezable tasks completed (elapsed 0.001 seconds)

Thank you,
Chris

Re: [PATCH 2/2] usb: dwc3: core: Prevent phy suspend during init

[Thinh Nguyen]

Hi,

On Fri, Oct 25, 2024, Chris Morgan wrote:
> 
> This patch seems to break system suspend on at least the Rockchip
> RK3566 platform. I noticed that I was no longer able to suspend
> and git bisect led me to this patch.
> 
> My kernel message log shows the following, at which point it freezes
> and does not allow me to resume from suspend:
> 
> [   27.235049] PM: suspend entry (deep)
> [   27.871641] Filesystems sync: 0.636 seconds
> [   27.885320] Freezing user space processes
> [   27.886932] Freezing user space processes completed (elapsed 0.001 seconds)
> [   27.887642] OOM killer disabled.
> [   27.887981] Freezing remaining freezable tasks
> [   27.889655] Freezing remaining freezable tasks completed (elapsed 0.001 seconds)
> 
> Thank you,
> Chris

Did you try out Roger's fix?
705e3ce37bcc ("usb: dwc3: core: Fix system suspend on TI AM62 platforms")

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=705e3ce37bccdf2ed6f848356ff355f480d51a91

BR,
Thinh