Re: [PATCH 2/2] usb: dwc3: of_simple: don't call pm_runtime_set_active()

[Roger Quadros]

On 31/05/18 10:59, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@xxxxxx> writes:
> 
>> On 30/05/18 15:31, Felipe Balbi wrote:
>>> Roger Quadros <rogerq@xxxxxx> writes:
>>>
>>>> Don't call pm_runtime_set_active() as it will prevent the device
>>>> from being activated in the next pm_runtime_get_sync() call.
>>>>
>>>> Also call pm_runtime_get_sync() before of_platform_populate().
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@xxxxxx>
>>>
>>> This patch is wrong.
>>>
>>>> ---
>>>>  drivers/usb/dwc3/dwc3-of-simple.c | 7 +++----
>>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
>>>> index e98d221..2cbb5c0 100644
>>>> --- a/drivers/usb/dwc3/dwc3-of-simple.c
>>>> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
>>>> @@ -121,6 +121,9 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>>>>  	if (ret)
>>>>  		goto err_resetc_assert;
>>>>  
>>>> +	pm_runtime_enable(dev);
>>>> +	pm_runtime_get_sync(dev);
>>>
>>> No, this is the wrong way to do things. My device should be enabled
>>> already from probe, specially since I have already enabled clocks.
>>
>> As far as I understood just enabling clocks (which might not include bus clock)
>> doesn't ensure device is enabled.
>>
>> Did you mean that I don't need to do a pm_runtime_get_sync() to enable my device in probe?
>> Who is enabling by device for me then? Is device core supposed to do it?
> 
> Not device core, but the bus code. Look at how PCI handles it. IIRC,
> only TI's omap_device behaves peculiarly WRT probe & pm runtime.

PCI does it doesn't mean it is a rule that bus code has to enable the device before probe.
At least platform bus doesn't seem to do it as part of of_platform_populate().

see __device_attach() and driver_probe_device() in drivers/base/dd.c

It does a pm_runtime_get_sync() on dev->parent but not on dev.

Also, from section 5 of Documentation/power/runtime_pm.txt

"In addition to that, the initial runtime PM status of all devices is
'suspended', but it need not reflect the actual physical state of the device.
Thus, if the device is initially active (i.e. it is able to process I/O), its
runtime PM status must be changed to 'active', with the help of
pm_runtime_set_active(), before pm_runtime_enable() is called for the device."

"Note, if the device may execute pm_runtime calls during the probe (such as
if it is registers with a subsystem that may call back in) then the
pm_runtime_get_sync() call paired with a pm_runtime_put() call will be
appropriate to ensure that the device is not put back to sleep during the
probe. This can happen with systems such as the network device layer."

So looks like we can't assume that the device is "active" when probe() is called.
Which means, we need to do

pm_runtime_get_sync();
enable optional clocks;

pm_runtime_forbid(); if we don't yet want runtime suspend/resume.
OR
pm_runtime_put(); if we want runtime suspend/resume support.

I don't think we should be calling pm_runtime_set_active() as we can't be sure of the
initial state of the device for different buses.
> 
>> The issue I was facing is that without this patch my device wasn't being enabled
>> as pm_runtime_set_active() is being done _before_ pm_runtime_get_sync().
>> It could be an issue with the platform's PM domain code as well.
> 
> Could be
> 

-- 
cheers,
-roger

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