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

[Roger Quadros <rogerq@ti.com>]

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