[PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices

On Tuesday, September 17, 2013 02:00:22 PM Mika Westerberg wrote:

On Mon, Sep 16, 2013 at 09:07:07PM +0200, Rafael J. Wysocki wrote:

quoted

On Monday, September 16, 2013 11:47:08 AM Mika Westerberg wrote:

quoted

On Sun, Sep 15, 2013 at 03:48:12PM +0200, Sylwester Nawrocki wrote:

quoted

On 09/13/2013 05:40 PM, Mika Westerberg wrote:
[...]

quoted

quoted

quoted

The call to pm_runtime_get_noresume() should make sure that the device is
in active state (at least in state where it can access the bus) if I'm
understanding this right.

I can't see how this would happen. How runtime_resume/runtime_suspend
callbacks would get invoked with this code, if, e.g. originally driver called
pm_runtime_enable(), pm_runtime_get_sync(), pm_runtime_put_sync() in probe() ?

The driver callbacks are not called but if the device has been attached to
a power domain (like we do with ACPI) the power domain callbacks get called
and it brings the "bus" to such state that we are able to access the
device. That also was the reason I used _noresume() but didn't look too
close the implementation.

OK, but if a client driver assumes default inactive power state it
will expect
its callbacks to get called. Otherwise exisiting code might break. So, e.g.
in case of s5p-tv it would rather need to be something like:

	pm_runtime_put()

	pm_runtime_get_sync()
 	sii9234_verify_version()
 	pm_runtime_put(dev)

Yes, or even call directly its runtime_resume callback to bring the device
to the active state.

quoted

quoted

quoted

pm_runtime_get_noresume() merely increments usage counter of a device.
It seems that these changes will break the s5p-tv driver. I might be missing
something though.

You are right and Kevin also mentioned this. It should be pm_runtime_get(),
if I'm not mistaken.

Note that client drivers usually call pm_runtime_enable() only when
it is safe
to call their driver's runtime PM callbacks. By enabling runtime PM
before the
client's driver has completely initialized we may risk that the
callbacks are
executed with uninitialized data, if I understand things correctly.

I think that calling pm_runtime_enable() on behalf of the client driver
shouldn't cause any problems. There is no PM events queued for that device
yet (and we have prevented that from happening when we called
_get_noresume() for the device).

That only is the case if the device in RPM_ACTIVE when we enable runtime PM for
it.  If it is RPM_SUSPENDED at that point, it still is possible that the resume
callback will be executed then.

OK, thanks for the clarification.

quoted

quoted

Only when the client driver calls _put() things start to happen and at that
phase the runtime PM hooks should be usable.

quoted

quoted

quoted

As Mark pointed out this is currently unwanted behaviour to runtime PM
activate a bus controller device manually in the core for when the client's
probe() is executed, since i2c core will activate the bus controller for when
transfer is being carried out.

But I can understand this is needed for ACPI and it shouldn't break existing
drivers, that do runtime PM activate the client device in probe().

Indeed, we don't want to break anything (and we still need something like
this for ACPI).

quoted

Now I'm sure this will break power management of the drivers/media/exynos4-is
driver, due to incorrect power sequence (power domain and clocks handling).
I'll try to take care of it in separate patch, as I have some patches pending,
that move most of code from drivers/media/exynos4-is/fimc-is-sensor.c to
drivers/media/i2c/s5k6a3.c.

I missed that code when I converted existing users to this method. Sorry
about that (I can handle that in the next version).

I quickly looked at it and I don't see anything that could break (once
converted). What it does is this:

pm_runtime_no_callbacks(dev);
pm_runtime_enable(dev);

changing that to:

pm_runtime_no_callbacks(dev);
pm_runtime_put(dev);

shouldn't cause problems AFAICT.

Yes, considering this driver in isolation it should be fine.

However, I observed system suspend issues when the I2C bus controller was
being activated (which would happen in the I2C core after your changes)
before some other driver has initialized.

So to ensure things continue to work the "fimc-isp-i2c" driver would need
to be registered after the "exynos4-fimc-is" driver has initialized. Or the
"exynos4-fimc-is" would need to call of_platform_populate() to instantiate
its all children devices as specified in device tree (see arch/arm/boot/dts/
exynos4x12.dtsi in -next). "simple-bus" would then have to be not listed in
the compatible property of that top level device. So to avoid regressions
some additional changes would be needed, outside of this particular I2C
client driver. I guess this could be avoided by better design of the
exynos4-is driver right from the beginning. But it's all some times tricky
when there is some many IP blocks involved and the hardware behaviour/device
interactions are not always well documented.

OK.

I'm actually thinking that it is probably better now if we don't touch the
client runtime PM at all in the I2C core.

I proposed a less intrusive solution in this same thread where we power the
I2C controller briefly at the client ->probe() (In order to have all the
ACPI power resources etc. and the controller on) and let the client driver
handle their own runtime PM as they do currently.

I'm not sure if the I2C core needs to power up the controller at the probe time.
That may be left to the client driver altogether.  I mean, if the client wants
the controller to be powered up, it should just call
pm_runtime_get_sync(controller device) at a suitable place (and then do the
corresponding _put when the controller is not necessary anu more) from its
->probe() callback.

Or the core can just check if the device is in the ACPI PM domain and power up
the controller if that's the case.  However, that may not match the case when
the I2C client is not a direct descendant of the controller (it may just use
an I2C resource pointing to the controller via a namespace path).

I sure hope we don't have to deal with such devices.

What if we make runtime PM enabled for the I2C adapter device only in case
of ACPI enumerated devices? That way runtime PM itself keeps the adapter
powered on when it has any active kids so we don't violate the ACPI spec,
and still let non-ACPI systems to use I2C as they do today.

Then we could do something like:

static int i2c_device_probe(struct device *dev)
{
	...
	/*
	 * Make sure that the adapter is powered on when the client is
	 * probed.
	 *
	 * Note that this is no-op on non-ACPI systems as runtime PM for
	 * the adapter is not enabled.
	 */
	pm_runtime_get_sync(&client->adapter->dev);
        acpi_dev_pm_attach(&client->dev, true);

I would make the code indicate the ACPI special case, like:

	if (ACPI_HANDLE(&client->dev)) {
		/* Power up the controller, if necessary, to follow the ACPI spec. */
		pm_runtime_get_sync(&client->adapter->dev);
		acpi_dev_pm_attach(&client->dev, true);
	}

        status = driver->probe(client, i2c_match_id(driver->id_table, client));
	if (status) {
		...

and of course you need to do the corresponding pm_runtime_put() for the
controller somewhere.

And because ACPI_HANDLE() is simply false for CONFIG_ACPI unset, that would
cover that case too.

and enable runtime PM only when we find that there are ACPI I2C devices
behind the controller and they are power manageable.

We need to power up the controller regardless of whether or not the child
devices are power manageable.  If a client device we want to access has an
ACPI handle, the controller has to be in D0 at that point.

Thanks,
Rafael