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 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.
> > >
> > > > 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
}
> 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.
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
