| Message ID | 1378913560-2752-2-git-send-email-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Mika Westerberg <mika.westerberg@linux.intel.com> writes: > From: Aaron Lu <aaron.lu@intel.com> > > This patch adds runtime PM support for the I2C bus in a similar way that > has been done for PCI bus already. This means that the I2C bus core > prepares runtime PM for a client device just before a driver is about to be > bound to it. Devices that are not bound to any driver are not prepared for > runtime PM. > > In order to take advantage of this runtime PM support, the client device > driver needs drop the device runtime PM reference count by calling > pm_runtime_put() in its ->probe() callback and possibly implement rest of > the runtime PM callbacks. > > If the driver doesn't support runtime PM (like most of the existing I2C > client drivers), the device in question is regarded as being runtime PM > active and powered on. But for existing drivers which already support runtime PM (at least 7 by a quick grep), they will be stuck runtime enabled and stop hitting low-power states after this patch. > The patch adds also runtime PM support for the adapter device because it is > needed to be able to runtime power manage the I2C controller device. The > adapter device is handled along with the I2C controller device (it uses > pm_runtime_no_callbacks()). > > Signed-off-by: Aaron Lu <aaron.lu@intel.com> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > drivers/i2c/i2c-core.c | 44 +++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 43 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > index f32ca29..44374b4 100644 > --- a/drivers/i2c/i2c-core.c > +++ b/drivers/i2c/i2c-core.c > @@ -248,11 +248,30 @@ static int i2c_device_probe(struct device *dev) > client->flags & I2C_CLIENT_WAKE); > dev_dbg(dev, "probe\n"); > > + /* Make sure the adapter is active */ > + pm_runtime_get_sync(&client->adapter->dev); > + > + /* > + * Enable runtime PM for the client device. If the client wants to > + * participate on runtime PM it should call pm_runtime_put() in its > + * probe() callback. > + */ > + pm_runtime_get_noresume(&client->dev); > + pm_runtime_set_active(&client->dev); Why the set_active here? For hardware that is disabled/powered-off on startup, there will now be a mismatch between the hardware state an the RPM core state. Kevin
On Thu, Sep 12, 2013 at 2:34 PM, Kevin Hilman <khilman@linaro.org> wrote: > Mika Westerberg <mika.westerberg@linux.intel.com> writes: > >> From: Aaron Lu <aaron.lu@intel.com> >> >> This patch adds runtime PM support for the I2C bus in a similar way that >> has been done for PCI bus already. This means that the I2C bus core >> prepares runtime PM for a client device just before a driver is about to be >> bound to it. Devices that are not bound to any driver are not prepared for >> runtime PM. >> >> In order to take advantage of this runtime PM support, the client device >> driver needs drop the device runtime PM reference count by calling >> pm_runtime_put() in its ->probe() callback and possibly implement rest of >> the runtime PM callbacks. >> >> If the driver doesn't support runtime PM (like most of the existing I2C >> client drivers), the device in question is regarded as being runtime PM >> active and powered on. > > But for existing drivers which already support runtime PM (at least 7 by > a quick grep), they will be stuck runtime enabled and stop hitting > low-power states after this patch. Oops, nevermind. I was mixing this up with runtime PM on the i2c adapter but this is for the i2c client. Sorry for the noise. Kevin
On 09/11/2013 05:32 PM, Mika Westerberg wrote: > From: Aaron Lu<aaron.lu@intel.com> > > This patch adds runtime PM support for the I2C bus in a similar way that > has been done for PCI bus already. This means that the I2C bus core > prepares runtime PM for a client device just before a driver is about to be > bound to it. Devices that are not bound to any driver are not prepared for > runtime PM. > > In order to take advantage of this runtime PM support, the client device > driver needs drop the device runtime PM reference count by calling > pm_runtime_put() in its ->probe() callback and possibly implement rest of > the runtime PM callbacks. > > If the driver doesn't support runtime PM (like most of the existing I2C > client drivers), the device in question is regarded as being runtime PM > active and powered on. > > The patch adds also runtime PM support for the adapter device because it is > needed to be able to runtime power manage the I2C controller device. The > adapter device is handled along with the I2C controller device (it uses > pm_runtime_no_callbacks()). > > Signed-off-by: Aaron Lu<aaron.lu@intel.com> > Signed-off-by: Mika Westerberg<mika.westerberg@linux.intel.com> > --- > drivers/i2c/i2c-core.c | 44 +++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 43 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > index f32ca29..44374b4 100644 > --- a/drivers/i2c/i2c-core.c > +++ b/drivers/i2c/i2c-core.c > @@ -248,11 +248,30 @@ static int i2c_device_probe(struct device *dev) > client->flags& I2C_CLIENT_WAKE); > dev_dbg(dev, "probe\n"); > > + /* Make sure the adapter is active */ > + pm_runtime_get_sync(&client->adapter->dev); So there is currently no way to avoid this behaviour, i.e. to have the adapter not activated before any of its client devices is probed, but only later on, after explicit call to pm_runtime_get*(&client->dev) in the client driver ? -- Thanks, Sylwester
On 09/13/2013 06:06 AM, Sylwester Nawrocki wrote: > On 09/11/2013 05:32 PM, Mika Westerberg wrote: >> From: Aaron Lu<aaron.lu@intel.com> >> >> This patch adds runtime PM support for the I2C bus in a similar way that >> has been done for PCI bus already. This means that the I2C bus core >> prepares runtime PM for a client device just before a driver is about to be >> bound to it. Devices that are not bound to any driver are not prepared for >> runtime PM. >> >> In order to take advantage of this runtime PM support, the client device >> driver needs drop the device runtime PM reference count by calling >> pm_runtime_put() in its ->probe() callback and possibly implement rest of >> the runtime PM callbacks. >> >> If the driver doesn't support runtime PM (like most of the existing I2C >> client drivers), the device in question is regarded as being runtime PM >> active and powered on. >> >> The patch adds also runtime PM support for the adapter device because it is >> needed to be able to runtime power manage the I2C controller device. The >> adapter device is handled along with the I2C controller device (it uses >> pm_runtime_no_callbacks()). >> >> Signed-off-by: Aaron Lu<aaron.lu@intel.com> >> Signed-off-by: Mika Westerberg<mika.westerberg@linux.intel.com> >> --- >> drivers/i2c/i2c-core.c | 44 +++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 43 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c >> index f32ca29..44374b4 100644 >> --- a/drivers/i2c/i2c-core.c >> +++ b/drivers/i2c/i2c-core.c >> @@ -248,11 +248,30 @@ static int i2c_device_probe(struct device *dev) >> client->flags& I2C_CLIENT_WAKE); >> dev_dbg(dev, "probe\n"); >> >> + /* Make sure the adapter is active */ >> + pm_runtime_get_sync(&client->adapter->dev); > > So there is currently no way to avoid this behaviour, i.e. to have the > adapter > not activated before any of its client devices is probed, but only later on, > after explicit call to pm_runtime_get*(&client->dev) in the client driver ? The above pm_runtime_get_sync is used to make sure when the client I2C device is going to be probed, its host adapter device is turned on(or we will fail the probe). It doesn't affect the adapter's status before the probe of I2C client device. Thanks, Aaron
On Fri, Sep 13, 2013 at 09:54:34AM +0300, Mika Westerberg wrote: > On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote: > > For hardware that is disabled/powered-off on startup, there will now be > > a mismatch between the hardware state an the RPM core state. > 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. Accessing the bus isn't an issue for I2C outside of ACPI, the power management of the device is totally disassociated from the bus and the controller is responsible for ensuring it is available during transfers.
On Fri, Sep 13, 2013 at 09:14:20AM +0800, Aaron Lu wrote: > On 09/13/2013 06:06 AM, Sylwester Nawrocki wrote: > > So there is currently no way to avoid this behaviour, i.e. to have the > > adapter > > not activated before any of its client devices is probed, but only later on, > > after explicit call to pm_runtime_get*(&client->dev) in the client driver ? > The above pm_runtime_get_sync is used to make sure when the client I2C > device is going to be probed, its host adapter device is turned on(or we > will fail the probe). It doesn't affect the adapter's status before the > probe of I2C client device. The expecation is that if the adaptor needs to do anything to transfer it'll do that when asked to transfer - that way it can sit in a low power state when the bus is idle.
On Fri, Sep 13, 2013 at 01:16:11PM +0300, Mika Westerberg wrote: > On Fri, Sep 13, 2013 at 10:59:50AM +0100, Mark Brown wrote: > > Accessing the bus isn't an issue for I2C outside of ACPI, the power > > management of the device is totally disassociated from the bus and the > > controller is responsible for ensuring it is available during transfers. > Yes, but since we want to support ACPI as well, we must make sure that the > adapter (and the associated controller) is available when client ->probe() > is called. Right, but this probably needs to be highlighted more since it's a very surprising thing for I2C and is causing confusion.
On Fri, Sep 13, 2013 at 02:50:35PM +0300, Mika Westerberg wrote: > On Fri, Sep 13, 2013 at 11:31:52AM +0100, Mark Brown wrote: > > Right, but this probably needs to be highlighted more since it's a very > > surprising thing for I2C and is causing confusion. > By highlighted more, do you mean something like adding a comment in the > code about this or? Perhaps, yes. Or possibly the commit log is going to be enough going forwards.
Mika Westerberg <mika.westerberg@linux.intel.com> writes: > On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote: >> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c >> > index f32ca29..44374b4 100644 >> > --- a/drivers/i2c/i2c-core.c >> > +++ b/drivers/i2c/i2c-core.c >> > @@ -248,11 +248,30 @@ static int i2c_device_probe(struct device *dev) >> > client->flags & I2C_CLIENT_WAKE); >> > dev_dbg(dev, "probe\n"); >> > >> > + /* Make sure the adapter is active */ >> > + pm_runtime_get_sync(&client->adapter->dev); >> > + >> > + /* >> > + * Enable runtime PM for the client device. If the client wants to >> > + * participate on runtime PM it should call pm_runtime_put() in its >> > + * probe() callback. >> > + */ >> > + pm_runtime_get_noresume(&client->dev); >> > + pm_runtime_set_active(&client->dev); >> >> Why the set_active here? >> >> For hardware that is disabled/powered-off on startup, there will now be >> a mismatch between the hardware state an the RPM core state. > > 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. No, after _get_noresume(), nothing happens to the hardware. It simply increments the usecount. From pm_runtime.h: static inline void pm_runtime_get_noresume(struct device *dev) { atomic_inc(&dev->power.usage_count); } So after the _get_noresume() and _set_active() you're very likely to have a disconnect between the hardware state and what state RPM thinks the hardware is in. Kevin
On 09/13/2013 08:54 AM, Mika Westerberg wrote: > On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote: >>> > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c >>> > > index f32ca29..44374b4 100644 >>> > > --- a/drivers/i2c/i2c-core.c >>> > > +++ b/drivers/i2c/i2c-core.c >>> > > @@ -248,11 +248,30 @@ static int i2c_device_probe(struct device *dev) >>> > > client->flags & I2C_CLIENT_WAKE); >>> > > dev_dbg(dev, "probe\n"); >>> > > >>> > > + /* Make sure the adapter is active */ >>> > > + pm_runtime_get_sync(&client->adapter->dev); >>> > > + >>> > > + /* >>> > > + * Enable runtime PM for the client device. If the client wants to >>> > > + * participate on runtime PM it should call pm_runtime_put() in its >>> > > + * probe() callback. >>> > > + */ >>> > > + pm_runtime_get_noresume(&client->dev); >>> > > + pm_runtime_set_active(&client->dev); >> > >> > Why the set_active here? >> > >> > For hardware that is disabled/powered-off on startup, there will now be >> > a mismatch between the hardware state an the RPM core state. > > 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() ? 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. 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(). 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. -- Regards, Sylwester
Mika Westerberg <mika.westerberg@linux.intel.com> writes: > On Fri, Sep 13, 2013 at 05:50:22PM +0300, Mika Westerberg wrote: >> On Fri, Sep 13, 2013 at 07:30:55AM -0700, Kevin Hilman wrote: >> > Mika Westerberg <mika.westerberg@linux.intel.com> writes: >> > >> > > On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote: >> > >> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c >> > >> > index f32ca29..44374b4 100644 >> > >> > --- a/drivers/i2c/i2c-core.c >> > >> > +++ b/drivers/i2c/i2c-core.c >> > >> > @@ -248,11 +248,30 @@ static int i2c_device_probe(struct device *dev) >> > >> > client->flags & I2C_CLIENT_WAKE); >> > >> > dev_dbg(dev, "probe\n"); >> > >> > >> > >> > + /* Make sure the adapter is active */ >> > >> > + pm_runtime_get_sync(&client->adapter->dev); >> > >> > + >> > >> > + /* >> > >> > + * Enable runtime PM for the client device. If the client wants to >> > >> > + * participate on runtime PM it should call pm_runtime_put() in its >> > >> > + * probe() callback. >> > >> > + */ >> > >> > + pm_runtime_get_noresume(&client->dev); >> > >> > + pm_runtime_set_active(&client->dev); >> > >> >> > >> Why the set_active here? >> > >> >> > >> For hardware that is disabled/powered-off on startup, there will now be >> > >> a mismatch between the hardware state an the RPM core state. >> > > >> > > 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. >> > >> > No, after _get_noresume(), nothing happens to the hardware. It simply >> > increments the usecount. From pm_runtime.h: >> > >> > static inline void pm_runtime_get_noresume(struct device *dev) >> > { >> > atomic_inc(&dev->power.usage_count); >> > } >> > >> > So after the _get_noresume() and _set_active() you're very likely to >> > have a disconnect between the hardware state and what state RPM thinks >> > the hardware is in. >> >> Good point. >> >> I suppose calling pm_runtime_get() here would work (and make the state >> active in all case)? I used _noresume() here because at this point the >> driver itself hasn't had change to install it's RPM hooks. > > I take that back. > > [ It has been some time since this code was originally written so I can't > remember all the details :-/ ] > > The pm_runtime_get_noresume() is there just to increment the refcount. It > has nothing to do to "activate" the device in question. Sorry about the > confusion from my part. > > This is what happens in case the device is enumerated from ACPI: > > // This makes sure that the controller itself is powered on > // (adapter device follows its parent which is the controller). The > // controller is attached to the ACPI power domain so it is > // brought to D0 now. > pm_runtime_get_sync(&client->adapter->dev); > > // This binds the client device to the ACPI power domain, and in > // addition to that brings the client device to D0. OK, then here is where the problem is, because you're building ACPI assumptions into the core. For non-ACPI devices, this part is a nop, so the client device is still powered off, which breaks the assumptions below. > if (ACPI_HANDLE(&client->dev)) > acpi_dev_pm_attach(&client->dev, true); > > // Increase the refcount so that client can start runtime PM > // transitions when it calls _put(). > pm_runtime_get_noresume(&client->dev); > // Mark the device being active as > // 1) In ACPI case we know that is true as we just powered the > // device on. > // 2) We treat the device by default to be runtime PM active and > // powered on (that's in the changelog and should follow what > // the PCI bus did). > pm_runtime_set_active(&client->dev); > > // Enable runtime PM but nothing happens yet as long as the client > // driver doesn't call _put(). > pm_runtime_enable(&client->dev); > > So, yes there might be a disconnect between the runtime PM state and the > device HW state now (same is with default to suspended). Yes, but until now, default to suspended has been assumed, so any changes to that will likely require more thorough auditing of other drivers. Kevin > When the driver ->probe() is called, it needs to power on the device (which > it probably needs to do anyway to be able to talk to the device and probe > its registers, etc.). Once the driver calls _put() the device is eventually > moved to suspended state (and its RPM hooks are called). I might be missing > something but is there a case where this is not beneficial?
On Sun, Sep 15, 2013 at 09:41:39AM +0300, Mika Westerberg wrote: > There's also a less intrusive way of fixing the problem we see with ACPI > enabled I2C devices: > 1. In I2C core i2c_device_probe() we power on the I2C controller > and attach the client device to the ACPI power domain. Just like in > this patch but we don't touch the I2C client device runtime PM. > -> This should allow the existing drivers to keep using whatever > runtime PM strategy they have chosen. There should be no explicit need to power on the I2C controller if it's implemented the same way the existing ones are - just have it power itself on when it is doing a transfer. > 2. For ACPI enumerated I2C client devices drivers we need to > implement the runtime PM in order to save power (otherwise the > devices will be left powered on). > and do the same for SPI devices as well. > Then only thing that changes for non-ACPI devices is that the controller > will be powered on during the client device probe (well, and during > remove). > Thoughts? This is definitely less intrusive than the current proposal, there's one ACPI I2C device binding queued for -next after the merge window (rt5640) which will need an update.
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) >> 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. >> 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.
On Sun, Sep 15, 2013 at 04:28:24PM +0300, Mika Westerberg wrote: > On Sun, Sep 15, 2013 at 01:47:44PM +0100, Mark Brown wrote: > > On Sun, Sep 15, 2013 at 09:41:39AM +0300, Mika Westerberg wrote: > > > 1. In I2C core i2c_device_probe() we power on the I2C controller > > > and attach the client device to the ACPI power domain. Just like in > > > this patch but we don't touch the I2C client device runtime PM. > > > -> This should allow the existing drivers to keep using whatever > > > runtime PM strategy they have chosen. > > There should be no explicit need to power on the I2C controller if it's > > implemented the same way the existing ones are - just have it power > > itself on when it is doing a transfer. > The problem is that the ACPI child device can't be in higher power state > than its parent (and this is also what the runtime PM expects). If we don't > power the I2C controller device before we power on the I2C client device > that rule is violated and we get an complaint to the console. That's definitely an ACPI specific (probably x86 specific ACPI?) requirement not a generic one, on some systems it would increase power consumption since the controller will need to sit on while the device is functioning autonomously. Even though the controller power consumption is going to be minimal the power domain it is in may be relatively large. Can't the power domains for ACPI deal with this requirement, for example by making the I2C slave power domains children of the controller power domain?
On Mon, Sep 16, 2013 at 05:38:12PM +0300, Mika Westerberg wrote: > On Mon, Sep 16, 2013 at 11:12:49AM +0100, Mark Brown wrote: > > That's definitely an ACPI specific (probably x86 specific ACPI?) > > requirement not a generic one, on some systems it would increase power > > consumption since the controller will need to sit on while the device is > > functioning autonomously. > > Yes, the ACPI 5.0 spec says that the device cannot be in higher D-state > than its parent. This is not x86 specific, though I'm not sure if this is > implemented elsewhere. > I do not think this stops the OS fine controlling the power of the device though. It is only a mechanism to make sure the tree of D states is vaguely sane from the ACPI point of view. What happens in each D state is never actually defined in the ACPI spec. Graeme
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. > 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). Thanks, Rafael
On Mon, Sep 16, 2013 at 09:07:07PM +0200, Rafael J. Wysocki wrote: > 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. It shouldn't even need to do that, it should just be able to rely on the controller to power itself up when asked to do work. This is how the existing implementations are done - the controller power management is totally transparent to the slave.
On Tuesday, September 17, 2013 12:31:11 AM Mark Brown wrote: > On Mon, Sep 16, 2013 at 09:07:07PM +0200, Rafael J. Wysocki wrote: > > > 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. > > It shouldn't even need to do that, it should just be able to rely on the > controller to power itself up when asked to do work. This is how the > existing implementations are done - the controller power management is > totally transparent to the slave. If both the I2C client and I2C controller have corresponding objects in the ACPI namespace and the client's object is a child of the controller's object, then in order to power up the client we need to power up the controller even if no transactions are going to be carried out. That's what the spec simply requires us to do in that case. Thanks, Rafael
On Tue, Sep 17, 2013 at 03:25:25AM +0200, Rafael J. Wysocki wrote: > On Tuesday, September 17, 2013 12:31:11 AM Mark Brown wrote: > > It shouldn't even need to do that, it should just be able to rely on the > > controller to power itself up when asked to do work. This is how the > > existing implementations are done - the controller power management is > > totally transparent to the slave. > If both the I2C client and I2C controller have corresponding objects in the > ACPI namespace and the client's object is a child of the controller's object, > then in order to power up the client we need to power up the controller even > if no transactions are going to be carried out. That's what the spec simply > requires us to do in that case. Like I said I think this should be handled by the power domains (or otherwise in the ACPI specific code) - we shouldn't be needing to modify individual drivers to work around thoughtlessness in the ACPI spec.
On 09/16/2013 10:47 AM, Mika Westerberg wrote: > 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. > > Do you think that would work better wrt. fimc-isp-i2c driver? That would be no different for this particular driver, as long as the I2C bus controller is activated right before the I2C client's probe(). In general I would expect such additional device activation not to be harmful. For that particular driver I'm going to prepare patches to ensure that the I2C bus controller device and its driver is registered only when a driver it depends on has initialized. This should have been ensured right from the beginning. So don't need to worry about this particular case. I'm just not sure what other devices could be similarly affected.
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
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index f32ca29..44374b4 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -248,11 +248,30 @@ static int i2c_device_probe(struct device *dev) client->flags & I2C_CLIENT_WAKE); dev_dbg(dev, "probe\n"); + /* Make sure the adapter is active */ + pm_runtime_get_sync(&client->adapter->dev); + + /* + * Enable runtime PM for the client device. If the client wants to + * participate on runtime PM it should call pm_runtime_put() in its + * probe() callback. + */ + pm_runtime_get_noresume(&client->dev); + pm_runtime_set_active(&client->dev); + pm_runtime_enable(&client->dev); + status = driver->probe(client, i2c_match_id(driver->id_table, client)); if (status) { client->driver = NULL; i2c_set_clientdata(client, NULL); + + pm_runtime_disable(&client->dev); + pm_runtime_set_suspended(&client->dev); + pm_runtime_put_noidle(&client->dev); } + + pm_runtime_put(&client->adapter->dev); + return status; } @@ -265,6 +284,8 @@ static int i2c_device_remove(struct device *dev) if (!client || !dev->driver) return 0; + pm_runtime_get_sync(&client->adapter->dev); + driver = to_i2c_driver(dev->driver); if (driver->remove) { dev_dbg(dev, "remove\n"); @@ -277,6 +298,13 @@ static int i2c_device_remove(struct device *dev) client->driver = NULL; i2c_set_clientdata(client, NULL); } + + /* Undo the runtime PM done in i2c_probe() */ + pm_runtime_disable(&client->dev); + pm_runtime_set_suspended(&client->dev); + pm_runtime_put_noidle(&client->dev); + + pm_runtime_put(&client->adapter->dev); return status; } @@ -288,8 +316,11 @@ static void i2c_device_shutdown(struct device *dev) if (!client || !dev->driver) return; driver = to_i2c_driver(dev->driver); - if (driver->shutdown) + if (driver->shutdown) { + pm_runtime_get_sync(&client->adapter->dev); driver->shutdown(client); + pm_runtime_put(&client->adapter->dev); + } } #ifdef CONFIG_PM_SLEEP @@ -1066,6 +1097,15 @@ exit_recovery: bus_for_each_drv(&i2c_bus_type, NULL, adap, __process_new_adapter); mutex_unlock(&core_lock); + /* + * Make sure the adapter runtime PM follows the parent device (the + * host controller) so that we can suspend it once there aren't any + * active clients anymore. + */ + pm_runtime_set_active(&adap->dev); + pm_runtime_no_callbacks(&adap->dev); + pm_runtime_enable(&adap->dev); + return 0; out_list: @@ -1230,6 +1270,8 @@ void i2c_del_adapter(struct i2c_adapter *adap) return; } + pm_runtime_disable(&adap->dev); + /* Tell drivers about this removal */ mutex_lock(&core_lock); bus_for_each_drv(&i2c_bus_type, NULL, adap,