Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754031Ab3IQV1z (ORCPT ); Tue, 17 Sep 2013 17:27:55 -0400 Received: from v094114.home.net.pl ([79.96.170.134]:52407 "HELO v094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1752689Ab3IQV1x (ORCPT ); Tue, 17 Sep 2013 17:27:53 -0400 From: "Rafael J. Wysocki" To: Mika Westerberg Cc: Sylwester Nawrocki , Sylwester Nawrocki , Kevin Hilman , linux-i2c@vger.kernel.org, Wolfram Sang , "Rafael J. Wysocki" , linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org, Lv Zheng , Aaron Lu , linux-arm-kernel@lists.infradead.org, Mark Brown , Dmitry Torokhov , Mauro Carvalho Chehab , Samuel Ortiz , Lee Jones , Arnd Bergmann , Greg Kroah-Hartman , Liam Girdwood , Kyungmin Park Subject: Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices Date: Tue, 17 Sep 2013 23:38:44 +0200 Message-ID: <4065873.t6fMJPXtCW@vostro.rjw.lan> User-Agent: KMail/4.10.5 (Linux/3.11.0+; KDE/4.10.5; x86_64; ; ) In-Reply-To: <20130917110021.GU7393@intel.com> References: <1378913560-2752-1-git-send-email-mika.westerberg@linux.intel.com> <1861747.RtS0ZLgUUN@vostro.rjw.lan> <20130917110021.GU7393@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="utf-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8884 Lines: 193 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/