Message-Id: <200906270002.32030.rjw@sisk.pl>
Date:	Sat, 27 Jun 2009 00:02:30 +0200
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	Magnus Damm <magnus.damm@...il.com>
Cc:	Alan Stern <stern@...land.harvard.edu>,
	linux-pm@...ts.linux-foundation.org,
	Oliver Neukum <oliver@...kum.org>,
	ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
	Ingo Molnar <mingo@...e.hu>,
	LKML <linux-kernel@...r.kernel.org>, Greg KH <gregkh@...e.de>,
	Arjan van de Ven <arjan@...radead.org>
Subject: Re: [patch update] PM: Introduce core framework for run-time PM of I/O devices (rev. 5)

On Thursday 25 June 2009, Magnus Damm wrote:
> On Thu, Jun 25, 2009 at 4:24 AM, Rafael J. Wysocki<rjw@...k.pl> wrote:
> > From: Rafael J. Wysocki <rjw@...k.pl>
> > Subject: PM: Introduce core framework for run-time PM of I/O devices (rev. 5)
> >
> > Introduce a core framework for run-time power management of I/O
> > devices.  Add device run-time PM fields to 'struct dev_pm_info'
> > and device run-time PM callbacks to 'struct dev_pm_ops'.  Introduce
> > a run-time PM workqueue and define some device run-time PM helper
> > functions at the core level.  Document all these things.
> >
> > Not-yet-signed-off-by: Rafael J. Wysocki <rjw@...k.pl>
> 
> Hi Rafael,
> 
> Thanks for your work on this. I've built some code for SuperH on top
> of this today, and with that behind me I have a few questions and a
> little bit of code feedback.
> 
> Questions:
> 
> 1) Which functions are device drivers supposed to use?
> 
> I simply added pm_runtime_resume() and pm_runtime_suspend() where
> clk_enable() and clk_disable() normally are used. In interrupt
> handlers I used pm_request_suspend() instead of pm_runtime_suspend().
> 
> I'm not sure if the v5 patch does the right thing around
> really_probe() like Alan pointed out.

Yes, Alan was right.  V6 doesn't address that, but V7 will. :-)

> Basically, I'd like to be able to call my bus callback for
> ->runtime_resume() from the driver probe(), but power.resume_count seems
> stuck at 1 which leads to pm_runtime_resume() returning -EAGAIN before
> invoking the bus callback.
> 
> This leads to question number two...
> 
> 2) What's the default state in probe()?

Currently, it's 'active', but runtime_disabled is set.  I'm open to
suggestions, though.

> We touched this subject briefly before. I'd like to compare the
> Runtime PM default state with the clock framework default state. The
> clock framework requires you to use clk_enable() to enable the clock
> to a hardware block before it is allowed to access the hardware
> registers. At least that's how we handle stop bits on SuperH. So
> clocks come up disabled from boot and should be enabled and disabled
> by the device driver to save power.
> 
> I'd like to change our Module Stop Bits code on SuperH (once again)
> from being handled by the clock framework to being managed by the
> Runtime PM framework. Having the clock framework deal with the stop
> bits works fine today, they are off by default after boot, and the
> driver often enables the clock with clk_enable() in probe() or
> hopefully in some more finegrained fashion.
> 
> I'm not sure how the Module Stop Bits should fit with the Runtime PM
> code though. The default state for a device at probe() time seems to
> be RPM_ACTIVE. Should drivers call pm_runtime_enable() to enable
> Runtime PM?

That's the idea.

> One part of me likes the idea that Runtime PM-enabled drivers start in
> RPM_SUSPENDED so they are forced to put pm_runtime_resume() before
> actually using the hardware. This makes the Runtime PM behaviour
> pretty close to the clock framework.
> 
> If you dislike starting from RPM_SUSPENDED (most likely) then I wonder
> how I should set the state to RPM_SUSPENDED in the driver. I'd like to
> make sure that pm_runtime_resume() can invoke the bus callback so the
> hardware can be turned on for the first time somehow. Should I do a
> dummy suspend?

Hmm, good question.

While run-time PM is disabled (power.runtime_disabled is set), which is the
case in the initial state, you can just change power.runtime_status to
RPM_SUSPENDED and nothing wrong happens as long as that reflects the actual
status of the device.

I'll add a helper function for that.

> 3) Should drivers use pm_suspend_ignore_children(dev, true)?
> 
> It turns out that I can't suspend my I2C master driver out of the box
> since it becomes the parent of all slaves on the I2C bus. The I2C
> master driver is just a platform driver, and the children are I2C
> devices (90% sure). I want to do Runtime PM regardless if the child
> devices are suspended or not, I guess I should use
> pm_suspend_ignore_children(dev, true) then?

Yes, that's the idea.

> > +/**
> > + * __pm_runtime_resume - Run a device bus type's runtime_resume() callback.
> > + * @dev: Device to resume.
> > + * @sync: If unset, the funtion has been called via pm_wq.
> > + *
> > + * Check if the device is really suspended and run the ->runtime_resume()
> > + * callback provided by the device's bus type driver.  Update the run-time PM
> > + * flags in the device object to reflect the current status of the device.  If
> > + * runtime suspend is in progress while this function is being run, wait for it
> > + * to finish before resuming the device.  If runtime suspend is scheduled, but
> > + * it hasn't started yet, cancel it and we're done.
> > + */
> > +int __pm_runtime_resume(struct device *dev, bool sync)
> > +{
> [snip]
> > +}
> > +EXPORT_SYMBOL_GPL(pm_runtime_resume);
> 
> You're missing "__" here unless you're aiming for something very exotic. =)

Ah, thanks!

> > +/**
> > + * pm_runtime_work - Run __pm_runtime_resume() for a device.
> > + * @work: Work structure used for scheduling the execution of this function.
> > + *
> > + * Use @work to get the device object the resume has been scheduled for and run
> > + * __pm_runtime_resume() for it.
> > + */
> > +static void pm_runtime_work(struct work_struct *work)
> > +{
> > +       __pm_runtime_resume(work_to_device(work), false);
> > +}
> 
> Anything wrong with the name pm_runtime_resume_work()?

Not at all. :-)  Just switched to that.

> Looking forward to v6, I'll switch task now, will be back to this late Monday.

Well, V6 was already sent, but unfortunately it didn't address your comments.
I'll send V7 during the weekend.

Best,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists