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

From: Sameer Pujar
Date: Mon Feb 04 2019 - 03:16:32 EST


On 2/1/2019 4:54 AM, Rafael J. Wysocki wrote:
On Thursday, January 31, 2019 3:30:24 PM CET Thierry Reding wrote:
--Pk/CTwBz1VvfPIDp
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Jan 31, 2019 at 01:10:01PM +0100, Rafael J. Wysocki wrote:
On Thu, Jan 31, 2019 at 12:59 PM Takashi Iwai <tiwai@xxxxxxx> 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@xxxxxxx> 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 ac=
tivated
at calling driver's probe callback.
As far as I can tell, the domain will also be powered off again a=
fter
probe finished, unless the device grabs a runtime PM reference. T=
his 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.

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 lik=
e this:
pm_runtime_enable(dev);
if (!pm_runtime_enabled(dev)) {
err =3D foo_runtime_resume(dev);
if (err < 0)
goto fail;
}

So basically when runtime PM is not available, we explicitly "res=
ume"
the device to power it up.

It seems to me like that's a fairly common problem, so I'm wonder=
ing 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 =3D 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).
=20
This basically is about setup, because after that point all should
just work in both cases.
=20
Personally, I would do
=20
if (IS_ENABLED(CONFIG_PM)) {
do setup based on pm-runtime
} else {
do manual setup
}
=20
Right now, we have a code like below, pushing the initialization in an
async work and let the probe returning quickly.

hda_tegra_probe() {
....
=20
So why don't you do
=20
if (!IS_ENABLED(CONFIG_PM)) {
do manual clock setup
}
=20
here?
I think that's exactly what Jon and Sameer were proposing, although the
discussion started primarily because of the way it was done.

So basically the idea was to do:

pm_runtime_enable()
if (!pm_runtime_enabled()) /* basically !IS_ENABLED(CONFIG_PM) */
But why is it any better than checking !IS_ENABLED(CONFIG_PM) directly?

hda_runtime_resume()

So we're not calling pm_runtime_resume() but rather the driver's
implementation of it. This is to avoid duplicating the code, which under
some circumstances can be fairly long. Duplicating is also error prone
because both instances may not always be in sync.

My understanding is that Takashi had reservations about using this kind
of construct because, well, frankly, it looks a little weird.
Yes, the way it was originally written above was weird, but is checking
IS_ENABLED(CONFIG_PM) directly really so weird?

We'd also likely want to have a similar construct again in the ->remove()
callback to make sure we properly power off the device when it is no longer
needed.
Sure. Again, why don't you make it conditional on IS_ENABLED(CONFIG_PM)?

I'm just wondering if perhaps there should be a mechanism in the
core to take care of this,
How exactly? How's the core going to know what to do when CONFIG_PM is
disabled?

because this is basically something that we'd need to do for every single
driver.
That is not true. If the device is alwyas "on" to start with, you don't
need to do anything. That's the case on many systems.

For example, if !CONFIG_PM couldn't the pm_runtime_enable() function be
modified to do the above?
But you'd need to pass a pointer to your hda_runtime_resume() to it at least
and how's that simpler than using a simple conditional directly?

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.

Alternatively, you can make the driver depend on PM.
Objective is to have things working with or without CONFIG_PM enabled.
From previous comments and discussions it appears that there is mixed response
for calling hda_tegra_runtime_resume() or runtime PM APIs in probe() call. Need
to have consensus regarding the best practice to be followed, which would eventually
can be used in other drivers too.

Rafael is suggesting to use CONFIG_PM check to do manual setup or runtime PM setup in probe,
which would bring back the earlier above mentioned concern.

if (IS_ENABLED(CONFIG_PM)) {
do setup based on pm-runtime
} else {
ÂÂÂ do manual setup
}
Both if/else might end up doing the same here.
Do we really need CONFIG_PM check here?

Instead does below proposal appear fine?

probe() {
ÂÂÂ hda_tegra_enable_clock();
}

probe_work() {
ÂÂÂ /* hda setup */
ÂÂÂ . . .
ÂÂÂ pm_runtime_set_active(); /* initial state as active */
ÂÂÂ pm_runtime_enable();
ÂÂÂ return;
}

remove() {
ÂÂÂ pm_runtime_disable();
ÂÂÂ if (!pm_runtime_status_suspended())
ÂÂÂ ÂÂÂ hda_tegra_runtime_suspend(); /* takes care of both CONFIG_PM enable/disable case */
}

One of the other concern was, remove() and probe() do not appear to be in sync, because in probe() hda_tegra_enable_clock()
is called and in remove() there is hda_tegra_runtime_suspend() to effectively disable clock.
IMO, this should be ok since it can avoid duplication and proper comment can be added here for clarity.
Alternatively we can call hda_tegra_runtime_resume() in probe() unconditionally to avoid confusion.

Another point Thierry mentioned was, after successful probe() power-domain would be turned OFF. It seems Rafael had a different
view. I am little confused here. Kindly clarify if above proposal seems fine.

Thanks,
Sameer.

Cheers,
Rafael