diff mbox series

[v2,RESEND] usb: dwc3: core: Fix ULPI PHYs and prevent phy_get/ulpi_init during suspend/resume

Message ID ee73347b-e996-4052-6a9d-dd94edd4a61a@ti.com
State Accepted
Commit 98112041bcca164676367e261c8c1073ef70cb51
Headers show
Series [v2,RESEND] usb: dwc3: core: Fix ULPI PHYs and prevent phy_get/ulpi_init during suspend/resume | expand

Commit Message

Roger Quadros Feb. 12, 2018, 1:30 p.m. UTC 
In order for ULPI PHYs to work, dwc3_phy_setup() and dwc3_ulpi_init()
must be doene before dwc3_core_get_phy().

commit 541768b08a40 ("usb: dwc3: core: Call dwc3_core_get_phy() before initializing phys")
broke this.

The other issue is that dwc3_core_get_phy() and dwc3_ulpi_init() should
be called only once during the life cycle of the driver. However,
as dwc3_core_init() is called during system suspend/resume it will
result in multiple calls to dwc3_core_get_phy() and dwc3_ulpi_init()
which is wrong.

Fix this by moving dwc3_ulpi_init() out of dwc3_phy_setup()
into dwc3_core_ulpi_init(). Use a flag 'ulpi_ready' to ensure that
dwc3_core_ulpi_init() is called only once from dwc3_core_init().

Use another flag 'phys_ready' to call dwc3_core_get_phy() only once from
dwc3_core_init().

Fixes: 541768b08a40 ("usb: dwc3: core: Call dwc3_core_get_phy() before initializing phys")
Fixes: f54edb539c11 ("usb: dwc3: core: initialize ULPI before trying to get the PHY")
Cc: linux-stable <stable@vger.kernel.org> # >= v4.13
Signed-off-by: Roger Quadros <rogerq@ti.com>

---
Rebased on Felipe's testing/fixes branch.

 drivers/usb/dwc3/core.c | 47 ++++++++++++++++++++++++++++++++++++-----------
 drivers/usb/dwc3/core.h |  5 +++++
 2 files changed, 41 insertions(+), 11 deletions(-)

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Comments

Todor Tomov Nov. 14, 2018, 4:11 p.m. UTC | #1 
Hello,

After I apply this patch on 4.14 (or receive it with 4.14.70) I see a regression with
the Qualcomm QUSB2 phy driver. I'm testing on a Dragonboard 820c.
In boot log I get:

[    4.525502] phy phy-7412000.phy.6: QUSB2PHY pll lock failed: status reg = 0
[    4.529232] phy phy-7412000.phy.6: phy init failed --> -16
[    4.536170] dwc3 7600000.dwc3: failed to initialize core
[    4.541743] dwc3: probe of 7600000.dwc3 failed with error -16
[    4.549979] phy phy-7411000.phy.5: QUSB2PHY pll lock failed: status reg = 0
[    4.552843] phy phy-7411000.phy.5: phy init failed --> -16
[    4.559606] dwc3 6a00000.dwc3: failed to initialize core
[    4.565181] dwc3: probe of 6a00000.dwc3 failed with error -16

I can provide a full log if needed.

Best regards,
Todor



On 12.02.2018 15:30, Roger Quadros wrote:
> In order for ULPI PHYs to work, dwc3_phy_setup() and dwc3_ulpi_init()

> must be doene before dwc3_core_get_phy().

> 

> commit 541768b08a40 ("usb: dwc3: core: Call dwc3_core_get_phy() before initializing phys")

> broke this.

> 

> The other issue is that dwc3_core_get_phy() and dwc3_ulpi_init() should

> be called only once during the life cycle of the driver. However,

> as dwc3_core_init() is called during system suspend/resume it will

> result in multiple calls to dwc3_core_get_phy() and dwc3_ulpi_init()

> which is wrong.

> 

> Fix this by moving dwc3_ulpi_init() out of dwc3_phy_setup()

> into dwc3_core_ulpi_init(). Use a flag 'ulpi_ready' to ensure that

> dwc3_core_ulpi_init() is called only once from dwc3_core_init().

> 

> Use another flag 'phys_ready' to call dwc3_core_get_phy() only once from

> dwc3_core_init().

> 

> Fixes: 541768b08a40 ("usb: dwc3: core: Call dwc3_core_get_phy() before initializing phys")

> Fixes: f54edb539c11 ("usb: dwc3: core: initialize ULPI before trying to get the PHY")

> Cc: linux-stable <stable@vger.kernel.org> # >= v4.13

> Signed-off-by: Roger Quadros <rogerq@ti.com>

> ---

> Rebased on Felipe's testing/fixes branch.

> 

>  drivers/usb/dwc3/core.c | 47 ++++++++++++++++++++++++++++++++++++-----------

>  drivers/usb/dwc3/core.h |  5 +++++

>  2 files changed, 41 insertions(+), 11 deletions(-)

> 

> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c

> index 59511f2..f1d838a 100644

> --- a/drivers/usb/dwc3/core.c

> +++ b/drivers/usb/dwc3/core.c

> @@ -486,6 +486,22 @@ static void dwc3_cache_hwparams(struct dwc3 *dwc)

>  	parms->hwparams8 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS8);

>  }

>  

> +static int dwc3_core_ulpi_init(struct dwc3 *dwc)

> +{

> +	int intf;

> +	int ret = 0;

> +

> +	intf = DWC3_GHWPARAMS3_HSPHY_IFC(dwc->hwparams.hwparams3);

> +

> +	if (intf == DWC3_GHWPARAMS3_HSPHY_IFC_ULPI ||

> +	    (intf == DWC3_GHWPARAMS3_HSPHY_IFC_UTMI_ULPI &&

> +	     dwc->hsphy_interface &&

> +	     !strncmp(dwc->hsphy_interface, "ulpi", 4)))

> +		ret = dwc3_ulpi_init(dwc);

> +

> +	return ret;

> +}

> +

>  /**

>   * dwc3_phy_setup - Configure USB PHY Interface of DWC3 Core

>   * @dwc: Pointer to our controller context structure

> @@ -497,7 +513,6 @@ static void dwc3_cache_hwparams(struct dwc3 *dwc)

>  static int dwc3_phy_setup(struct dwc3 *dwc)

>  {

>  	u32 reg;

> -	int ret;

>  

>  	reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));

>  

> @@ -568,9 +583,6 @@ static int dwc3_phy_setup(struct dwc3 *dwc)

>  		}

>  		/* FALLTHROUGH */

>  	case DWC3_GHWPARAMS3_HSPHY_IFC_ULPI:

> -		ret = dwc3_ulpi_init(dwc);

> -		if (ret)

> -			return ret;

>  		/* FALLTHROUGH */

>  	default:

>  		break;

> @@ -727,6 +739,7 @@ static void dwc3_core_setup_global_control(struct dwc3 *dwc)

>  }

>  

>  static int dwc3_core_get_phy(struct dwc3 *dwc);

> +static int dwc3_core_ulpi_init(struct dwc3 *dwc);

>  

>  /**

>   * dwc3_core_init - Low-level initialization of DWC3 Core

> @@ -758,17 +771,27 @@ static int dwc3_core_init(struct dwc3 *dwc)

>  			dwc->maximum_speed = USB_SPEED_HIGH;

>  	}

>  

> -	ret = dwc3_core_get_phy(dwc);

> +	ret = dwc3_phy_setup(dwc);

>  	if (ret)

>  		goto err0;

>  

> -	ret = dwc3_core_soft_reset(dwc);

> -	if (ret)

> -		goto err0;

> +	if (!dwc->ulpi_ready) {

> +		ret = dwc3_core_ulpi_init(dwc);

> +		if (ret)

> +			goto err0;

> +		dwc->ulpi_ready = true;

> +	}

>  

> -	ret = dwc3_phy_setup(dwc);

> +	if (!dwc->phys_ready) {

> +		ret = dwc3_core_get_phy(dwc);

> +		if (ret)

> +			goto err0a;

> +		dwc->phys_ready = true;

> +	}

> +

> +	ret = dwc3_core_soft_reset(dwc);

>  	if (ret)

> -		goto err0;

> +		goto err0a;

>  

>  	dwc3_core_setup_global_control(dwc);

>  	dwc3_core_num_eps(dwc);

> @@ -841,6 +864,9 @@ static int dwc3_core_init(struct dwc3 *dwc)

>  	phy_exit(dwc->usb2_generic_phy);

>  	phy_exit(dwc->usb3_generic_phy);

>  

> +err0a:

> +	dwc3_ulpi_exit(dwc);

> +

>  err0:

>  	return ret;

>  }

> @@ -1235,7 +1261,6 @@ static int dwc3_probe(struct platform_device *pdev)

>  

>  err3:

>  	dwc3_free_event_buffers(dwc);

> -	dwc3_ulpi_exit(dwc);

>  

>  err2:

>  	pm_runtime_allow(&pdev->dev);

> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h

> index 185b960..860d2bc 100644

> --- a/drivers/usb/dwc3/core.h

> +++ b/drivers/usb/dwc3/core.h

> @@ -797,7 +797,9 @@ struct dwc3_scratchpad_array {

>   * @usb3_phy: pointer to USB3 PHY

>   * @usb2_generic_phy: pointer to USB2 PHY

>   * @usb3_generic_phy: pointer to USB3 PHY

> + * @phys_ready: flag to indicate that PHYs are ready

>   * @ulpi: pointer to ulpi interface

> + * @ulpi_ready: flag to indicate that ULPI is initialized

>   * @u2sel: parameter from Set SEL request.

>   * @u2pel: parameter from Set SEL request.

>   * @u1sel: parameter from Set SEL request.

> @@ -895,7 +897,10 @@ struct dwc3 {

>  	struct phy		*usb2_generic_phy;

>  	struct phy		*usb3_generic_phy;

>  

> +	bool			phys_ready;

> +

>  	struct ulpi		*ulpi;

> +	bool			ulpi_ready;

>  

>  	void __iomem		*regs;

>  	size_t			regs_size;

>
Todor Tomov Nov. 16, 2018, 5:01 p.m. UTC | #2 
Hi,

On 15.11.2018 10:09, Felipe Balbi wrote:
> 

> Hi,

> 

> Todor Tomov <todor.tomov@linaro.org> writes:

> 

>> Hello,

>>

>> After I apply this patch on 4.14 (or receive it with 4.14.70) I see a regression with

>> the Qualcomm QUSB2 phy driver. I'm testing on a Dragonboard 820c.

>> In boot log I get:

>>

>> [    4.525502] phy phy-7412000.phy.6: QUSB2PHY pll lock failed: status reg = 0

>> [    4.529232] phy phy-7412000.phy.6: phy init failed --> -16

>> [    4.536170] dwc3 7600000.dwc3: failed to initialize core

>> [    4.541743] dwc3: probe of 7600000.dwc3 failed with error -16

>> [    4.549979] phy phy-7411000.phy.5: QUSB2PHY pll lock failed: status reg = 0

>> [    4.552843] phy phy-7411000.phy.5: phy init failed --> -16

>> [    4.559606] dwc3 6a00000.dwc3: failed to initialize core

>> [    4.565181] dwc3: probe of 6a00000.dwc3 failed with error -16

>>

>> I can provide a full log if needed.

> 

> please do. Also, try mainline and let us know if the problem


This is the full log on 4.14.69 + this patch: https://paste.ubuntu.com/p/N5WdHw47QC/
I also managed to get a log from 4.20.0-rc2 and I see the same error: https://paste.ubuntu.com/p/jz6fvYyZh6/

> persists. Why do you get -EBUSY from the PHY driver?


Maybe I could have proposed a fix if I knew but I don't know.

Best regards,
Todor

> 

> - 

> balbi

>
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 59511f2..f1d838a 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -486,6 +486,22 @@  static void dwc3_cache_hwparams(struct dwc3 *dwc)
 	parms->hwparams8 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS8);
 }
 
+static int dwc3_core_ulpi_init(struct dwc3 *dwc)
+{
+	int intf;
+	int ret = 0;
+
+	intf = DWC3_GHWPARAMS3_HSPHY_IFC(dwc->hwparams.hwparams3);
+
+	if (intf == DWC3_GHWPARAMS3_HSPHY_IFC_ULPI ||
+	    (intf == DWC3_GHWPARAMS3_HSPHY_IFC_UTMI_ULPI &&
+	     dwc->hsphy_interface &&
+	     !strncmp(dwc->hsphy_interface, "ulpi", 4)))
+		ret = dwc3_ulpi_init(dwc);
+
+	return ret;
+}
+
 /**
  * dwc3_phy_setup - Configure USB PHY Interface of DWC3 Core
  * @dwc: Pointer to our controller context structure
@@ -497,7 +513,6 @@  static void dwc3_cache_hwparams(struct dwc3 *dwc)
 static int dwc3_phy_setup(struct dwc3 *dwc)
 {
 	u32 reg;
-	int ret;
 
 	reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
 
@@ -568,9 +583,6 @@  static int dwc3_phy_setup(struct dwc3 *dwc)
 		}
 		/* FALLTHROUGH */
 	case DWC3_GHWPARAMS3_HSPHY_IFC_ULPI:
-		ret = dwc3_ulpi_init(dwc);
-		if (ret)
-			return ret;
 		/* FALLTHROUGH */
 	default:
 		break;
@@ -727,6 +739,7 @@  static void dwc3_core_setup_global_control(struct dwc3 *dwc)
 }
 
 static int dwc3_core_get_phy(struct dwc3 *dwc);
+static int dwc3_core_ulpi_init(struct dwc3 *dwc);
 
 /**
  * dwc3_core_init - Low-level initialization of DWC3 Core
@@ -758,17 +771,27 @@  static int dwc3_core_init(struct dwc3 *dwc)
 			dwc->maximum_speed = USB_SPEED_HIGH;
 	}
 
-	ret = dwc3_core_get_phy(dwc);
+	ret = dwc3_phy_setup(dwc);
 	if (ret)
 		goto err0;
 
-	ret = dwc3_core_soft_reset(dwc);
-	if (ret)
-		goto err0;
+	if (!dwc->ulpi_ready) {
+		ret = dwc3_core_ulpi_init(dwc);
+		if (ret)
+			goto err0;
+		dwc->ulpi_ready = true;
+	}
 
-	ret = dwc3_phy_setup(dwc);
+	if (!dwc->phys_ready) {
+		ret = dwc3_core_get_phy(dwc);
+		if (ret)
+			goto err0a;
+		dwc->phys_ready = true;
+	}
+
+	ret = dwc3_core_soft_reset(dwc);
 	if (ret)
-		goto err0;
+		goto err0a;
 
 	dwc3_core_setup_global_control(dwc);
 	dwc3_core_num_eps(dwc);
@@ -841,6 +864,9 @@  static int dwc3_core_init(struct dwc3 *dwc)
 	phy_exit(dwc->usb2_generic_phy);
 	phy_exit(dwc->usb3_generic_phy);
 
+err0a:
+	dwc3_ulpi_exit(dwc);
+
 err0:
 	return ret;
 }
@@ -1235,7 +1261,6 @@  static int dwc3_probe(struct platform_device *pdev)
 
 err3:
 	dwc3_free_event_buffers(dwc);
-	dwc3_ulpi_exit(dwc);
 
 err2:
 	pm_runtime_allow(&pdev->dev);
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 185b960..860d2bc 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -797,7 +797,9 @@  struct dwc3_scratchpad_array {
  * @usb3_phy: pointer to USB3 PHY
  * @usb2_generic_phy: pointer to USB2 PHY
  * @usb3_generic_phy: pointer to USB3 PHY
+ * @phys_ready: flag to indicate that PHYs are ready
  * @ulpi: pointer to ulpi interface
+ * @ulpi_ready: flag to indicate that ULPI is initialized
  * @u2sel: parameter from Set SEL request.
  * @u2pel: parameter from Set SEL request.
  * @u1sel: parameter from Set SEL request.
@@ -895,7 +897,10 @@  struct dwc3 {
 	struct phy		*usb2_generic_phy;
 	struct phy		*usb3_generic_phy;
 
+	bool			phys_ready;
+
 	struct ulpi		*ulpi;
+	bool			ulpi_ready;
 
 	void __iomem		*regs;
 	size_t			regs_size;