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

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

[Roger Quadros]

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

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);
-- 
cheers,
-roger

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

Re: [PATCH] usb: dwc3: core: Don't try to get PHYs during suspend/resume

[Roger Quadros]

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?

> ---
>  drivers/usb/dwc3/core.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> 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);
> 

-- 
cheers,
-roger

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

Re: [PATCH] usb: dwc3: core: Don't try to get PHYs during suspend/resume

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2093 bytes --]


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);
 	if (ret)
 		goto err0;
 
-	ret = dwc3_core_soft_reset(dwc);
+	ret = dwc3_core_get_phy(dwc);
 	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

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] usb: dwc3: core: Don't try to get PHYs during suspend/resume

[Roger Quadros]

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

Re: [PATCH] usb: dwc3: core: Don't try to get PHYs during suspend/resume

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1817 bytes --]


Hi,

Roger Quadros <rogerq@ti.com> writes:
>> 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?

as long as clocks are running, we can do that, yes.

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

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] usb: dwc3: core: Don't try to get PHYs during suspend/resume

[Roger Quadros]

On 10/01/18 16:04, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>>> 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?
> 
> as long as clocks are running, we can do that, yes.
> 
>>> -	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

-- 
cheers,
-roger

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

Re: [PATCH] usb: dwc3: core: Don't try to get PHYs during suspend/resume

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 626 bytes --]


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();

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] usb: dwc3: core: Don't try to get PHYs during suspend/resume

[Roger Quadros]

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

Re: [PATCH] usb: dwc3: core: Don't try to get PHYs during suspend/resume

[Roger Quadros]

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

Re: [PATCH] usb: dwc3: core: Don't try to get PHYs during suspend/resume

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2778 bytes --]


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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] usb: dwc3: core: Don't try to get PHYs during suspend/resume

[Roger Quadros]

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