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

Sameer Pujar
2019-01-31T14:21:34Z

On 1/31/2019 5:40 PM, Rafael J. Wysocki wrote:
> On Thu, Jan 31, 2019 at 12:59 PM Takashi Iwai <tiwai@suse.de> wrote:
>> On Thu, 31 Jan 2019 12:46:54 +0100,
>> Rafael J. Wysocki wrote:
>>> On Thu, Jan 31, 2019 at 12:21 PM Takashi Iwai <tiwai@suse.de> wrote:
>>>> On Thu, 31 Jan 2019 12:05:30 +0100,
>>>> Thierry Reding wrote:
>>>>> On Wed, Jan 30, 2019 at 05:40:42PM +0100, Takashi Iwai wrote:
>>> [cut]
>>>
>>>>>> If I understand correctly the code, the pm domain is already activated
>>>>>> at calling driver's probe callback.
>>>>> As far as I can tell, the domain will also be powered off again after
>>>>> probe finished, unless the device grabs a runtime PM reference. This is
>>>>> what happens via the dev->pm_domain->sync() call after successful probe
>>>>> of a driver.
>>>> Ah, a good point. This can be a problem with a probe work like this
>>>> case.

Are you suggesting, whether runtime PM is enabled/disabled, after
successful probe the
domain would be powered off?
For CONFIG_PM enabled case, probe() can call get_sync() and put_sync()
can be in probe_work.
How this needs to be handled for CONFG_PM disabled case? (just calling
clock_enable() may
not be sufficient as per previous comments)

>>>>> It seems to me like it's not a very well defined case what to do when a
>>>>> device needs to be powered up but runtime PM is not enabled.
>>>>>
>>>>> Adding Rafael and linux-pm, maybe they can provide some guidance on what
>>>>> to do in these situations.
>>>>>
>>>>> To summarize, what we're debating here is how to handle powering up a
>>>>> device if the pm_runtime infrastructure doesn't take care of it. Jon's
>>>>> proposal here was, and we use this elsewhere, to do something like this:
>>>>>
>>>>> pm_runtime_enable(dev);
>>>>> if (!pm_runtime_enabled(dev)) {
>>>>> err = foo_runtime_resume(dev);
>>>>> if (err < 0)
>>>>> goto fail;
>>>>> }
>>>>>
>>>>> So basically when runtime PM is not available, we explicitly "resume"
>>>>> the device to power it up.
>>>>>
>>>>> It seems to me like that's a fairly common problem, so I'm wondering if
>>>>> there's something that the runtime PM core could do to help with this.
>>>>> Or perhaps there's already a way to achieve this that we're all
>>>>> overlooking?
>>>>>
>>>>> Rafael, any suggestions?
>>>> If any, a common helper would be appreciated, indeed.
>>> I'm not sure that I understand the problem correctly, so let me
>>> restate it the way I understand it.
>>>
>>> What we're talking about is a driver ->probe() callback. Runtime PM
>>> is disabled initially and the device is off. It needs to be powered
>>> up, but the way to do that depends on some configuration of the board
>>> etc., so ideally
>>>
>>> pm_runtime_enable(dev);
>>> ret = pm_runtime_resume(dev);
>>>
>>> should just work, but the question is what to do if runtime PM doesn't
>>> work as expected. That is, CONFIG_PM_RUNTIME is unset? Or something
>>> else?
>> Yes, the question is how to write the code for both with and without
>> CONFIG_PM (or CONFIG_PM_RUNTIME).
> This basically is about setup, because after that point all should
> just work in both cases.
>
> Personally, I would do
>
> if (IS_ENABLED(CONFIG_PM)) {
> do setup based on pm-runtime
> } else {
> do manual setup
> }

do we really need config check here?
The debate was, whether to call hda_tegra_runtime_resume() or
hda_tegra_enable_clocks() unconditionally here.
It would take care of both CONFIG_PM enabled/disabled cases. Then enable
runtime PM.

>> Right now, we have a code like below, pushing the initialization in an
>> async work and let the probe returning quickly.
>>
>> hda_tegra_probe() {
>> ....
> So why don't you do
>
> if (!IS_ENABLED(CONFIG_PM)) {
> do manual clock setup
> }
>
> here?
>
>> pm_runtime_enable();
>> schedule_work();
>> return;
>> }
>>
>> hda_tegra_probe_work() {
>> pm_runtime_get_sync();
>> ....
>> pm_runtime_put_sync();
>> }
>>
>> Then it truned outhis code lacks of the clock initialization when
>> runtime PM isn't enabled. Normally it's done via runtime resume
>>
>> hda_tegra_runtime_resume() {
>> hda_tegra_enable_clocks();
>> ....
>> }
>>
>> And now the question is what is the standard idiom in such a case.
>>
>> IMO, calling pm_runtime_resume() inside the probe function looks
>> weird, and my preference was to initialize the clocks explicitly, then
>> enable runtime PM. But if using pm_runtime_resume() in the proc
>> should be seen as a standard procedure, I'm fine with that.

I think reference here is, whether calling hda_tegra_runtime_resume() in
probe() is
a standard procedure or not.

> Well, people do pm_runtime_resume() in ->probe() too, but
> pm_runtime_resume() returns 1 for CONFIG_PM unset, so that won't give
> you what you want anyway. :-)
>
> Cheers,
> Rafael