diff mbox series

usb: dwc3: core: Don't try to get PHYs during suspend/resume

Message ID 1515589914-23460-1-git-send-email-rogerq@ti.com
State New
Headers show
Series usb: dwc3: core: Don't try to get PHYs during suspend/resume | expand

Commit Message

Roger Quadros Jan. 10, 2018, 1:11 p.m. UTC 
The USB PHYs should be requested only once during the life cycle of
this driver.

As dwc3_core_init() is called during system suspend/resume
it will result in multiple calls to dwc3_core_get_phy() which is wrong.

To prevent that let's move dwc3_core_get_phy() call
outside dwc3_core_init().

Fixes: 541768b08a4 ("usb: dwc3: core: Call dwc3_core_get_phy() before initializing phys")
Cc: linux-stable <stable@vger.kernel.org> # >= v4.13
Signed-off-by: Roger Quadros <rogerq@ti.com>

---
 drivers/usb/dwc3/core.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

-- 
cheers,
-roger

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

Comments

Roger Quadros Jan. 10, 2018, 1:56 p.m. UTC | #1 
On 10/01/18 15:33, Felipe Balbi wrote:
> 

> Hi,

> 

> Roger Quadros <rogerq@ti.com> writes:

>> Felipe,

>>

>> On 10/01/18 15:11, Roger Quadros wrote:

>>> The USB PHYs should be requested only once during the life cycle of

>>> this driver.

>>>

>>> As dwc3_core_init() is called during system suspend/resume

>>> it will result in multiple calls to dwc3_core_get_phy() which is wrong.

>>>

>>> To prevent that let's move dwc3_core_get_phy() call

>>> outside dwc3_core_init().

>>>

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

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

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

>>

>> FYI. this patch brings the code back to

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

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

>>

>> So looks like this will break ULPI PHY case?

>>

>> Where do we initialize ULPI PHY, in dwc3_phy_setup()?

>>

>> if so then 541768b08a40 breaks the ULPI PHY case as well, right?

> 

> indeed, that commit regressed ULPI PHYs :-(

> 

> Seems like it should be more like below:

> 

> @@ -754,15 +754,15 @@ 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);


But can we do a dwc3_phy_setup() without doing the soft reset of the controller first?


>  	if (ret)

>  		goto err0;

>  

> -	ret = dwc3_core_soft_reset(dwc);

> +	ret = dwc3_core_get_phy(dwc);


we can get_phy in dwc3_core_init() as it will get called on resume().
This was the $subject of this patch.

>  	if (ret)

>  		goto err0;

>  

> -	ret = dwc3_phy_setup(dwc);

> +	ret = dwc3_core_soft_reset(dwc);

>  	if (ret)

>  		goto err0;

>  

> And maybe we rename dwc3_phy_setup() to dwc3_phy_intf_config() just to

> make the name match what the function actually does. Can you check that

> it won't regress the case reported by Carlos? If that works, then we

> would have to move BOTH dwc3_phy_setup() (dwc3_phy_intf_config()) and

> dwc3_core_get_phy() outside of dwc3_core_init(), which would mean

> duplicated code in suspend/resume handlers.

> 

> I'm sure we can sort that out in another way; but the proper order is:

> 

> -> initialize ULPI (if necessary)

> -> get phy

> -> soft reset

> 


-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Roger Quadros Jan. 11, 2018, 9:23 a.m. UTC | #2 
On 11/01/18 11:09, Roger Quadros wrote:
> +Heikki

> 

> On 11/01/18 10:25, Felipe Balbi wrote:

>>

>> Hi,

>>

>> Roger Quadros <rogerq@ti.com> writes:

>>>>>> -	ret = dwc3_core_soft_reset(dwc);

>>>>>> +	ret = dwc3_core_get_phy(dwc);

>>>>>

>>>>> we can get_phy in dwc3_core_init() as it will get called on resume().

>>>>> This was the $subject of this patch.

>>>>

>>>> indeed. thanks :-)

>>>>

>>>

>>> oops sorry. I meant we can't call dwc3_core_get_phy() in dwc3_core_init(). :P

>>

>> bit of a chicken-and-egg problem. We need to setup the PHY interface

>> before getting the PHYs, but can't get PHY during resume. Maybe the best

>> way here would be to check for the pointers being valid. Something like:

>>

>> if (!phy)

>> 	get_phy();

>>

> 

> OK that should take care of not calling get_phy() on suspend.

> However there is one more issue with the approach

> 

>> @@ -754,15 +754,15 @@ 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;

> 

> here we configure PHY related bits and register the ulpi interface.

> 

>>  

>> -	ret = dwc3_core_soft_reset(dwc);

>> +	ret = dwc3_core_get_phy(dwc);

>>  	if (ret)

>>  		goto err0;

>>  

> 

> we got the PHYs. all OK here.

> 

>> -	ret = dwc3_phy_setup(dwc);

>> +	ret = dwc3_core_soft_reset(dwc);

>>  	if (ret)

>>  		goto err0;

> 

> Now we do a soft reset. This means we loose the PHY configuration bits that we did


Actually I was wrong. We're only resetting the device side (DCTL.CSFTRST) which doesn't seem to affect
GUSB2PHYCFGn and GUSB3PIPECTLn registers.

So we're good.

> in dwc3_phy_setup. So we need to call dwc3_phy_setup again but not re-register the ulpi interface.

> I can use a flag there so that dwc3_ulpi_init() is done only once.

> 


-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Felipe Balbi Jan. 11, 2018, 9:31 a.m. UTC | #3 
Hi,

Roger Quadros <rogerq@ti.com> writes:
>> Roger Quadros <rogerq@ti.com> writes:

>>>>>> -	ret = dwc3_core_soft_reset(dwc);

>>>>>> +	ret = dwc3_core_get_phy(dwc);

>>>>>

>>>>> we can get_phy in dwc3_core_init() as it will get called on resume().

>>>>> This was the $subject of this patch.

>>>>

>>>> indeed. thanks :-)

>>>>

>>>

>>> oops sorry. I meant we can't call dwc3_core_get_phy() in dwc3_core_init(). :P

>> 

>> bit of a chicken-and-egg problem. We need to setup the PHY interface

>> before getting the PHYs, but can't get PHY during resume. Maybe the best

>> way here would be to check for the pointers being valid. Something like:

>> 

>> if (!phy)

>> 	get_phy();

>> 

>

> OK that should take care of not calling get_phy() on suspend.

> However there is one more issue with the approach

>

>> @@ -754,15 +754,15 @@ 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;

>

> here we configure PHY related bits and register the ulpi interface.

>

>>  

>> -	ret = dwc3_core_soft_reset(dwc);

>> +	ret = dwc3_core_get_phy(dwc);

>>  	if (ret)

>>  		goto err0;

>>  

>

> we got the PHYs. all OK here.

>

>> -	ret = dwc3_phy_setup(dwc);

>> +	ret = dwc3_core_soft_reset(dwc);

>>  	if (ret)

>>  		goto err0;

>

> Now we do a soft reset. This means we loose the PHY configuration bits that we did

> in dwc3_phy_setup. So we need to call dwc3_phy_setup again but not re-register the ulpi interface.

> I can use a flag there so that dwc3_ulpi_init() is done only once.


sounds like it's better to extract out a smaller function that just
checks if we need ULPI bus and registers it, something akin to:

@@ -482,6 +482,21 @@ static void dwc3_cache_hwparams(struct dwc3 *dwc)
 	parms->hwparams8 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS8);
 }
 
+static int dwc3_ulpi_init(struct dwc3 *dwc)
+{
+	int intf;
+
+	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)))
+		return dwc3_ulpi_init(dwc);
+
+	return 0;
+}
+
 /**
  * dwc3_phy_setup - Configure USB PHY Interface of DWC3 Core
  * @dwc: Pointer to our controller context structure
@@ -563,11 +578,6 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
 				break;
 		}
 		/* FALLTHROUGH */
-	case DWC3_GHWPARAMS3_HSPHY_IFC_ULPI:
-		ret = dwc3_ulpi_init(dwc);
-		if (ret)
-			return ret;
-		/* FALLTHROUGH */
 	default:
 		break;
 	}

Then we just call that outside of any functions that get called during PM.

-- 
balbi
Roger Quadros Jan. 11, 2018, 9:46 a.m. UTC | #4 
On 11/01/18 11:31, Felipe Balbi wrote:
> 

> Hi,

> 

> Roger Quadros <rogerq@ti.com> writes:

>>> Roger Quadros <rogerq@ti.com> writes:

>>>>>>> -	ret = dwc3_core_soft_reset(dwc);

>>>>>>> +	ret = dwc3_core_get_phy(dwc);

>>>>>>

>>>>>> we can get_phy in dwc3_core_init() as it will get called on resume().

>>>>>> This was the $subject of this patch.

>>>>>

>>>>> indeed. thanks :-)

>>>>>

>>>>

>>>> oops sorry. I meant we can't call dwc3_core_get_phy() in dwc3_core_init(). :P

>>>

>>> bit of a chicken-and-egg problem. We need to setup the PHY interface

>>> before getting the PHYs, but can't get PHY during resume. Maybe the best

>>> way here would be to check for the pointers being valid. Something like:

>>>

>>> if (!phy)

>>> 	get_phy();

>>>

>>

>> OK that should take care of not calling get_phy() on suspend.

>> However there is one more issue with the approach

>>

>>> @@ -754,15 +754,15 @@ 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;

>>

>> here we configure PHY related bits and register the ulpi interface.

>>

>>>  

>>> -	ret = dwc3_core_soft_reset(dwc);

>>> +	ret = dwc3_core_get_phy(dwc);

>>>  	if (ret)

>>>  		goto err0;

>>>  

>>

>> we got the PHYs. all OK here.

>>

>>> -	ret = dwc3_phy_setup(dwc);

>>> +	ret = dwc3_core_soft_reset(dwc);

>>>  	if (ret)

>>>  		goto err0;

>>

>> Now we do a soft reset. This means we loose the PHY configuration bits that we did

>> in dwc3_phy_setup. So we need to call dwc3_phy_setup again but not re-register the ulpi interface.

>> I can use a flag there so that dwc3_ulpi_init() is done only once.

> 

> sounds like it's better to extract out a smaller function that just

> checks if we need ULPI bus and registers it, something akin to:

> 

> @@ -482,6 +482,21 @@ static void dwc3_cache_hwparams(struct dwc3 *dwc)

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

>  }

>  

> +static int dwc3_ulpi_init(struct dwc3 *dwc)

> +{

> +	int intf;

> +

> +	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)))

> +		return dwc3_ulpi_init(dwc);

> +

> +	return 0;

> +}

> +

>  /**

>   * dwc3_phy_setup - Configure USB PHY Interface of DWC3 Core

>   * @dwc: Pointer to our controller context structure

> @@ -563,11 +578,6 @@ static int dwc3_phy_setup(struct dwc3 *dwc)

>  				break;

>  		}

>  		/* FALLTHROUGH */

> -	case DWC3_GHWPARAMS3_HSPHY_IFC_ULPI:

> -		ret = dwc3_ulpi_init(dwc);

> -		if (ret)

> -			return ret;

> -		/* FALLTHROUGH */

>  	default:

>  		break;

>  	}

> 

> Then we just call that outside of any functions that get called during PM.

> 


Right. Seems like we've covered everything. I'll send a patch in a while.

-- 
cheers,
-roger

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

Patch

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 0783250..1274251 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -722,8 +722,6 @@  static void dwc3_core_setup_global_control(struct dwc3 *dwc)
 	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
 }
 
-static int dwc3_core_get_phy(struct dwc3 *dwc);
-
 /**
  * dwc3_core_init - Low-level initialization of DWC3 Core
  * @dwc: Pointer to our controller context structure
@@ -754,10 +752,6 @@  static int dwc3_core_init(struct dwc3 *dwc)
 			dwc->maximum_speed = USB_SPEED_HIGH;
 	}
 
-	ret = dwc3_core_get_phy(dwc);
-	if (ret)
-		goto err0;
-
 	ret = dwc3_core_soft_reset(dwc);
 	if (ret)
 		goto err0;
@@ -1177,6 +1171,10 @@  static int dwc3_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, dwc);
 	dwc3_cache_hwparams(dwc);
 
+	ret = dwc3_core_get_phy(dwc);
+	if (ret)
+		goto err0;
+
 	spin_lock_init(&dwc->lock);
 
 	pm_runtime_set_active(dev);