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

[Sameer Pujar <spujar@nvidia.com>]

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