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:
> > On Monday, September 16, 2013 11:47:08 AM Mika Westerberg wrote:
> > > On Sun, Sep 15, 2013 at 03:48:12PM +0200, Sylwester Nawrocki wrote:
> > > > On 09/13/2013 05:40 PM, Mika Westerberg wrote:
> > > > [...]
> > > > >>>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.
> > >
> > > > >>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.
>
> > > Only when the client driver calls _put() things start to happen and at
> > > that
> > > phase the runtime PM hooks should be usable.
> > >
> > > > >>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).
> > > > >
> > > > >>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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
