Re: [PATCH v2] ALSA: hda/tegra: enable clock during probe

[Dmitry Osipenko]

04.02.2019 14:05, Thierry Reding ÐÐÑÐÑ:
> On Mon, Feb 04, 2019 at 09:53:32AM +0000, Jon Hunter wrote:
>>
>>
>> On 04/02/2019 08:45, Thierry Reding wrote:
>>
>> ...
>>
>>> The idea was, as I was saying below, to reuse dev_pm_ops even if
>>> !CONFIG_PM. So pm_runtime_enable() could be something like this:
>>>
>>> 	pm_runtime_enable(dev)
>>> 	{
>>> 		if (!CONFIG_PM)
>>> 			if (dev->pm_ops->resume)
>>> 				dev->pm_ops->resume(dev);
>>>
>>> 		...
>>> 	}
>>>
>>> But that's admittedly somewhat of a stretch. This could of course be
>>> made somewhat nicer by adding an explicit variant, say:
>>>
>>> 	pm_runtime_enable_foo(dev)
>>> 	{
>>> 		if (!CONFIG_PM && dev->pm_ops->resume)
>>> 			return dev->pm_ops->resume(dev);
>>>
>>> 		return 0;
>>> 	}
>>>
>>> Maybe the fact that I couldn't come up with a good name is a good
>>> indication that this is a bad idea...
>>
>> How about some new APIs called ...
>>
>> pm_runtime_enable_get()
>> pm_runtime_enable_get_sync()
>> pm_runtime_put_disable() (implies a put_sync)
>>
>> ... and in these APIs we add ...
>>
>> pm_runtime_enable_get(dev)
>> {
>> 	if (!CONFIG_PM && dev->pm_ops->resume)
>> 		return dev->pm_ops->resume(dev);
>>
>> 	pm_runtime_enable(dev);
>> 	return pm_runtime_get(dev);
>> }
> 
> Yeah, those sound sensible. I'm still a bit torn between this and just
> enforcing PM. At least on the display side I think we already require PM
> and with all the power domain work that you did we'd be much better off
> if we could just get rid of the !PM workarounds.
> 
>>>>> This would be somewhat tricky because drivers
>>>>> usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and
>>>>> that would result in an empty structure if !CONFIG_PM, but we could
>>>>> probably work around that by adding a __SET_RUNTIME_PM_OPS that would
>>>>> never be compiled out for this kind of case. Or such drivers could even
>>>>> manually set .runtime_suspend and .runtime_resume to make sure they're
>>>>> always populated.
>>>>>
>>>>> Another way out of this would be to make sure we never run into the case
>>>>> where runtime PM is disabled. If we always "select PM" on Tegra, then PM
>>>>> should always be available. But is it guaranteed that runtime PM for the
>>>>> devices is functional in that case? From a cursory look at the code it
>>>>> would seem that way.
>>>>
>>>> If you select PM, then all of the requisite code should be there.
>>>
>>> We do this on 64-bit ARM, but there had been some pushback when we had
>>> proposed to do the same thing on 32-bit ARM. I think there were two
>>> concerns:
>>>
>>> 	1) select PM would force the setting for all platforms on multi-
>>> 	   platforms builds
>>>
>>> 	2) prevents anyone from disabling PM for debugging purposes
>>>
>>> 1) no longer seems to be valid because Rockchip already selects PM
>>> unconditionally. I'm not sure if 2) is valid anymore either. I haven't
>>> run a build with !PM in a very long time and I wouldn't be surprised if
>>> that was completely broken.
>>>
>>> Maybe we need to try this again since a couple of years have elapsed and
>>> runtime PM support on Tegra is much more mature at this point.
>>>
>>>> Alternatively, you can make the driver depend on PM.
>>>
>>> That's probably the easiest way out, but to be honest I think I'd prefer
>>> to just enforce PM and keep things simple.
>>>
>>> Jon, any objections?
>>
>> None, but seems overkill just for this case.
> 
> But that's precisely the point. It's not just about this case. We've
> already got some drivers where we do a similar dance just to be able to
> support the, let's admit it, exotic case where somebody turns off PM. I
> think supporting !PM might have made sense at a point where we had no
> support for power management at all. But I think we've come a long way
> since then, and while we may still have some ways to go in some areas,
> we do fairly decent runtime PM most of the time, to the point where I no
> longer see any benefits in !PM.

I'm requesting PM_DEBUG_ALWAYS_ON option then! Disabling PM is a useful debug feature, it can't just go away.