Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Tue, Sep 17, 2013 at 01:07:37PM +0200, Sylwester Nawrocki wrote: > 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(). Rafael pointed out that we can use ->ignore_children for the I2C adapter device for everything else than ACPI devices. That should keep the existing behavior. For ACPI devices we don't set that flag and the runtime PM core will activate the adapter whenever we try to power on I2C client device. This will make ACPI happy as well. If there are no objections, I'm going to send a new version based on the above. Thanks everyone for valuable input :) -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Tue, Sep 17, 2013 at 01:07:37PM +0200, Sylwester Nawrocki wrote: 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(). Rafael pointed out that we can use -ignore_children for the I2C adapter device for everything else than ACPI devices. That should keep the existing behavior. For ACPI devices we don't set that flag and the runtime PM core will activate the adapter whenever we try to power on I2C client device. This will make ACPI happy as well. If there are no objections, I'm going to send a new version based on the above. Thanks everyone for valuable input :) -- 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/
Re: [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: > > 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
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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. -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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. signature.asc Description: Digital signature
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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. signature.asc Description: Digital signature
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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. -- 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/
Re: [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: 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
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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 -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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. signature.asc Description: Digital signature
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Mon, Sep 16, 2013 at 03:46:16PM +0100, Graeme Gregory wrote: > 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. I think there's a pretty good definition of the D-states in chapter 2.3 of the ACPI 5.0 spec. In our case the problem is that the I2C controller is in D3Cold (off) and we try to move the I2C client device to D0 (on) it violates the ACPI spec. Anyway we are looking if we can somehow make this work in such way that it doesn't prevent non-ACPI devices from functioning as they expect now. -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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 -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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. > 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? We'll look into this. Thanks for the suggestion. -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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? signature.asc Description: Digital signature
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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). 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
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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). 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. Do you think that would work
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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? signature.asc Description: Digital signature
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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. 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? We'll look into this. Thanks for the suggestion. -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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 -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Mon, Sep 16, 2013 at 03:46:16PM +0100, Graeme Gregory wrote: 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. I think there's a pretty good definition of the D-states in chapter 2.3 of the ACPI 5.0 spec. In our case the problem is that the I2C controller is in D3Cold (off) and we try to move the I2C client device to D0 (on) it violates the ACPI spec. Anyway we are looking if we can somehow make this work in such way that it doesn't prevent non-ACPI devices from functioning as they expect now. -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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. signature.asc Description: Digital signature
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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 -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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. -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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: > > > 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. 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. > > 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. OK, thanks for the heads up. I'll check it. -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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. signature.asc Description: Digital signature
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Fri, Sep 13, 2013 at 02:10:43PM -0700, Kevin Hilman wrote: > > > > // 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(>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. We expect that once the driver ->probe() is called, and it doesn't participate the runtime PM prepared here, the device is regarded as powered on, runtime PM active. If the driver participates in runtime PM, it needs to power on the device and then call pm_runtime_put() to suspend the device. > > if (ACPI_HANDLE(>dev)) > > acpi_dev_pm_attach(>dev, true); > > > > // Increase the refcount so that client can start runtime PM > > // transitions when it calls _put(). > > pm_runtime_get_noresume(>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(>dev); > > > > // Enable runtime PM but nothing happens yet as long as the client > > // driver doesn't call _put(). > > pm_runtime_enable(>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. I agree. And it looks like I missed few existing drivers as well. I'm going to update them in the next version of the series. 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. 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? -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Fri, Sep 13, 2013 at 02:10:43PM -0700, Kevin Hilman wrote: // 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. We expect that once the driver -probe() is called, and it doesn't participate the runtime PM prepared here, the device is regarded as powered on, runtime PM active. If the driver participates in runtime PM, it needs to power on the device and then call pm_runtime_put() to suspend the device. 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. I agree. And it looks like I missed few existing drivers as well. I'm going to update them in the next version of the series. 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. 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? -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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. signature.asc Description: Digital signature
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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: 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. 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. 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. OK, thanks for the heads up. I'll check it. -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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. -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
Mika Westerberg 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 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(>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(>dev);
>> > >> > + pm_runtime_set_active(>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(>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(>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(>dev))
> acpi_dev_pm_attach(>dev, true);
>
> // Increase the refcount so that client can start runtime PM
> // transitions when it calls _put().
> pm_runtime_get_noresume(>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(>dev);
>
> // Enable runtime PM but nothing happens yet as long as the client
> // driver doesn't call _put().
> pm_runtime_enable(>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?
--
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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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 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(>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(>dev);
> > >> > + pm_runtime_set_active(>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(>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(>adapter->dev);
// This binds the client device to the ACPI power domain, and in
// addition to that brings the client device to D0.
if (ACPI_HANDLE(>dev))
acpi_dev_pm_attach(>dev, true);
// Increase the refcount so that client can start runtime PM
// transitions when it calls _put().
pm_runtime_get_noresume(>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(>dev);
// Enable runtime PM but nothing happens yet as long as the client
// driver doesn't call _put().
pm_runtime_enable(>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).
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?
--
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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Fri, Sep 13, 2013 at 05:14:56PM +0200, Sylwester Nawrocki wrote: > 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(>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(>dev); > >>> > > + pm_runtime_set_active(>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() > ? 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. > 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. > 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. -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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(>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(>dev); >>> > > + pm_runtime_set_active(>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 -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
Mika Westerberg 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(>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(>dev);
>> > + pm_runtime_set_active(>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(>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
--
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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Fri, Sep 13, 2013 at 07:30:55AM -0700, Kevin Hilman wrote:
> Mika Westerberg 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(>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(>dev);
> >> > +pm_runtime_set_active(>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(>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.
--
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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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. signature.asc Description: Digital signature
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Fri, Sep 13, 2013 at 11:31:52AM +0100, Mark Brown wrote: > 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. By highlighted more, do you mean something like adding a comment in the code about this or? -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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. signature.asc Description: Digital signature
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Fri, Sep 13, 2013 at 10:59:50AM +0100, Mark Brown wrote: > 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. 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. -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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*(>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. signature.asc Description: Digital signature
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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. signature.asc Description: Digital signature
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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(>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(>dev); > > + pm_runtime_set_active(>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. -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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. -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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. signature.asc Description: Digital signature
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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. signature.asc Description: Digital signature
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Fri, Sep 13, 2013 at 10:59:50AM +0100, Mark Brown wrote: 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. 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. -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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. signature.asc Description: Digital signature
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Fri, Sep 13, 2013 at 11:31:52AM +0100, Mark Brown wrote: 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. By highlighted more, do you mean something like adding a comment in the code about this or? -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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. signature.asc Description: Digital signature
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Fri, Sep 13, 2013 at 07:30:55AM -0700, Kevin Hilman wrote:
Mika Westerberg mika.westerb...@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.
--
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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
Mika Westerberg mika.westerb...@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
--
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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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 -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Fri, Sep 13, 2013 at 05:14:56PM +0200, Sylwester Nawrocki wrote: 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() ? 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. 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. 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. -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
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.westerb...@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.
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).
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?
--
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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
Mika Westerberg mika.westerb...@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.westerb...@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?
--
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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On 09/13/2013 06:06 AM, Sylwester Nawrocki wrote: > On 09/11/2013 05:32 PM, Mika Westerberg wrote: >> From: Aaron Lu >> >> 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 >> Signed-off-by: Mika Westerberg >> --- >> 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(>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*(>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 -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On 09/11/2013 05:32 PM, Mika Westerberg wrote: From: Aaron Lu 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 Signed-off-by: Mika Westerberg --- 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(>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*(>dev) in the client driver ? -- Thanks, Sylwester -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Thu, Sep 12, 2013 at 2:34 PM, Kevin Hilman wrote: > Mika Westerberg writes: > >> From: Aaron Lu >> >> 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 -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
Mika Westerberg writes: > From: Aaron Lu > > 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 > Signed-off-by: Mika Westerberg > --- > 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(>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(>dev); > + pm_runtime_set_active(>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 -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
Mika Westerberg mika.westerb...@linux.intel.com writes: From: Aaron Lu aaron...@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...@intel.com Signed-off-by: Mika Westerberg mika.westerb...@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 -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Thu, Sep 12, 2013 at 2:34 PM, Kevin Hilman khil...@linaro.org wrote: Mika Westerberg mika.westerb...@linux.intel.com writes: From: Aaron Lu aaron...@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 -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On 09/11/2013 05:32 PM, Mika Westerberg wrote: From: Aaron Luaaron...@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 Luaaron...@intel.com Signed-off-by: Mika Westerbergmika.westerb...@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 -- 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/
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On 09/13/2013 06:06 AM, Sylwester Nawrocki wrote: On 09/11/2013 05:32 PM, Mika Westerberg wrote: From: Aaron Luaaron...@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 Luaaron...@intel.com Signed-off-by: Mika Westerbergmika.westerb...@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 -- 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/
[PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
From: Aaron Lu
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
Signed-off-by: Mika Westerberg
---
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(>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(>dev);
+ pm_runtime_set_active(>dev);
+ pm_runtime_enable(>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(>dev);
+ pm_runtime_set_suspended(>dev);
+ pm_runtime_put_noidle(>dev);
}
+
+ pm_runtime_put(>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(>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(>dev);
+ pm_runtime_set_suspended(>dev);
+ pm_runtime_put_noidle(>dev);
+
+ pm_runtime_put(>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(>adapter->dev);
driver->shutdown(client);
+ pm_runtime_put(>adapter->dev);
+ }
}
#ifdef CONFIG_PM_SLEEP
@@ -1066,6 +1097,15 @@ exit_recovery:
bus_for_each_drv(_bus_type, NULL, adap, __process_new_adapter);
mutex_unlock(_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(>dev);
+ pm_runtime_no_callbacks(>dev);
+ pm_runtime_enable(>dev);
+
return 0;
out_list:
@@ -1230,6 +1270,8 @@ void i2c_del_adapter(struct i2c_adapter *adap)
return;
}
+ pm_runtime_disable(>dev);
+
/* Tell drivers about this removal */
mutex_lock(_lock);
bus_for_each_drv(_bus_type, NULL, adap,
--
1.8.4.rc3
--
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/
[PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
From: Aaron Lu aaron...@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...@intel.com
Signed-off-by: Mika Westerberg mika.westerb...@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);
+ 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,
--
1.8.4.rc3
--
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/
