OMAP PM patches
Subsystems (6)
cover OMAP PM patches
Hi guys,
this series is actually *REALLY* far from ready, but I wanted to ask if I should continue down this track because it really looks (to me at least) that OMAP's PM layer took a few uneecessary shortcuts.
I'm trying to understand the reasoning behind that, so bear with me for a while.
At least patches 1 and 4 look like they could go upstream, but please give it a very good review. I will continue to work on these if the rest of the community thinks it's valid, otherwise I would like to get some explanation for the way OMAP PM layer is implemented today.
cheers
Felipe Balbi (5):
arm: omap: fix up _od_suspend_noirq and _od_resume_noirq
arm: omap: don't forcefully runtime suspend a device
arm: omap: introduce other PM methods
i2c: omap: don't re-enable IRQs after masking them
i2c: omap: introduce suspend/resume methods
arch/arm/plat-omap/omap_device.c | 162 +++++++++++++++++++++++++++++++++++++-- drivers/i2c/busses/i2c-omap.c | 70 ++++++++++++----- 2 files changed, 207 insertions(+), 25 deletions(-)
Signature · 1 line
1.8.0.rc0
PATCH 1/5 arm: omap: fix up _od_suspend_noirq and _od_resume_noirq
current implementation doesn't take care about drivers which don't provide *_noirq methods and we could fall into a situation where we would suspend/resume devices even though they haven't asked for it.
One such case happened with the I2C driver which was getting suspended during suspend_noirq() just to be resume right after by any other device doing an I2C transfer on its suspend method.
--- arch/arm/plat-omap/omap_device.c | 8 ++++++++ 1 file changed, 8 insertions(+)
arch/arm/plat-omap/omap_device.c +8 -0
@@ -804,8 +804,12 @@ static int _od_suspend_noirq(struct device *dev) {struct platform_device *pdev =to_platform_device (dev);struct omap_device *od =to_omap_device (pdev); +const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :NULL ;int ret; +if (!pm || !pm->suspend_noirq) +return 0 ; +/* Don't attempt late suspend on a driver that is not bound */ if (od->_driver_status !=BUS_NOTIFY_BOUND_DRIVER )return 0 ;
@@ -827,6 +831,10 @@ static int _od_resume_noirq(struct device *dev) {struct platform_device *pdev =to_platform_device (dev);struct omap_device *od =to_omap_device (pdev); +const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :NULL ; + +if (!pm || !pm->resume_noirq) +return 0 ;if ((od->flags &OMAP_DEVICE_SUSPENDED ) && !pm_runtime_status_suspended (dev)) {
Signature · 1 line
1.8.0.rc0
PATCH 1/5 arm: omap: fix up _od_suspend_noirq and _od_resume_noirq
Quoted 2 lines · Felipe Balbi
current implementation doesn't take care about
drivers which don't provide*_noirqmethods
The generic ops handle this. See below.
Quoted 2 lines
and we could fall into a situation where we would suspend/resume devices even though they haven't asked for it.
The following explanation doesn't explain this, so I dont' follow the "even though they haven't asked for it" part.
Quoted 4 lines
One such case happened with the I2C driver which was getting suspended during
suspend_noirq()just to be resume right after by any other device doing an I2C transfer on its suspend method.
In order to be I2C to be runtime resumed after its noirq method has been called, that means the other device is doing an I2C xfer in its noirq method. That is a bug.
Quoted 10 lines
Signed-off-by Felipe Balbi <[email protected]>--- arch/arm/plat-omap/omap_device.c | 8 ++++++++ 1 file changed, 8 insertions(+)arch/arm/plat-omap/omap_device.c +0
-0
arch/arm/plat-omap/omap_device.c static int _od_suspend_noirq(struct device *dev) · line 812
Quoted 9 lines
arch/arm/plat-omap/omap_device.c +4
-0@@ -804,8 +804,12 @@ static int _od_suspend_noirq(struct device *dev) {struct platform_device *pdev =to_platform_device (dev);struct omap_device *od =to_omap_device (pdev); +const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :NULL ;int ret; +if (!pm || !pm->suspend_noirq) +return 0 ; +
you just re implemented pm_generic_suspend_noirq() (c.f. drivers/base/power/generic_ops.c)
arch/arm/plat-omap/omap_device.c static int _od_resume_noirq(struct device *dev) · line 837
Quoted 11 lines
arch/arm/plat-omap/omap_device.c +4
-0/* Don't attempt late suspend on a driver that is not bound */ if (od->_driver_status !=BUS_NOTIFY_BOUND_DRIVER )return 0 ;@@ -827,6 +831,10 @@ static int _od_resume_noirq(struct device *dev) {struct platform_device *pdev =to_platform_device (dev);struct omap_device *od =to_omap_device (pdev); +const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :NULL ; + +if (!pm || !pm->resume_noirq) +return 0 ;
and this is basically pm_generic_resume_noirq()
arch/arm/plat-omap/omap_device.c static int _od_resume_noirq(struct device *dev) · line 840
Quoted 3 lines
arch/arm/plat-omap/omap_device.c +0
-0if ((od->flags &OMAP_DEVICE_SUSPENDED ) && !pm_runtime_status_suspended (dev)) {
Kevin
PATCH 1/5 arm: omap: fix up _od_suspend_noirq and _od_resume_noirq
Hi,
Quoted 44 lines · Kevin Hilman
Quoted 2 lines · Felipe Balbi
current implementation doesn't take care about
drivers which don't provide*_noirqmethodsThe generic ops handle this. See below.
Quoted 2 lines
and we could fall into a situation where we would suspend/resume devices even though they haven't asked for it.
The following explanation doesn't explain this, so I dont' follow the "even though they haven't asked for it" part.
Quoted 4 lines
One such case happened with the I2C driver which was getting suspended during
suspend_noirq()just to be resume right after by any other device doing an I2C transfer on its suspend method.In order to be I2C to be runtime resumed after its noirq method has been called, that means the other device is doing an I2C xfer in its noirq method. That is a bug.
Quoted 10 lines
Signed-off-by Felipe Balbi <[email protected]>--- arch/arm/plat-omap/omap_device.c | 8 ++++++++ 1 file changed, 8 insertions(+)arch/arm/plat-omap/omap_device.c +0
-0arch/arm/plat-omap/omap_device.c static int _od_suspend_noirq(struct device *dev) · line 812
Quoted 9 lines
arch/arm/plat-omap/omap_device.c +4
-0@@ -804,8 +804,12 @@ static int _od_suspend_noirq(struct device *dev) {struct platform_device *pdev =to_platform_device (dev);struct omap_device *od =to_omap_device (pdev); +const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :NULL ;int ret; +if (!pm || !pm->suspend_noirq) +return 0 ; +you just re implemented
pm_generic_suspend_noirq()(c.f.drivers/base/power/generic_ops.c)
Quoted 13 lines
arch/arm/plat-omap/omap_device.c static int _od_resume_noirq(struct device *dev) · line 837
Quoted 11 lines
arch/arm/plat-omap/omap_device.c +4
-0/* Don't attempt late suspend on a driver that is not bound */ if (od->_driver_status !=BUS_NOTIFY_BOUND_DRIVER )return 0 ;@@ -827,6 +831,10 @@ static int _od_resume_noirq(struct device *dev) {struct platform_device *pdev =to_platform_device (dev);struct omap_device *od =to_omap_device (pdev); +const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :NULL ; + +if (!pm || !pm->resume_noirq) +return 0 ;and this is basically
pm_generic_resume_noirq()
not true, there is a slight different. Note that you only call pm_generic_resume_noirq() after calling pm_runtime_resume()ing the device:
Quoted 11 lines
static int
_od_resume_noirq(struct device *dev){ struct platform_device *pdev = to_platform_device(dev); struct omap_device *od = to_omap_device(pdev); if ((od->flags & OMAP_DEVICE_SUSPENDED) && !pm_runtime_status_suspended(dev)) { od->flags &= ~OMAP_DEVICE_SUSPENDED; if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND)) omap_device_enable(pdev); pm_generic_runtime_resume(dev);
right here. IMHO this is a bug, if the define doesn't implement resume_noirq, it's telling you that it has nothing to do at that time, so you shouldn't forcefully resume the device.
If the device dies, then it means that it needs to implement resume_noirq. What you have here, is a "workaround" for badly written device drivers IMHO. Note also, that you could runtime resume devices which don't implement resume_noirq().
the same is valid for suspend_noirq.
Quoted 4 lines
} return pm_generic_resume_noirq(dev); }
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121018/1798106e/attachment.sig>
PATCH 1/5 arm: omap: fix up _od_suspend_noirq and _od_resume_noirq
Quoted 77 lines · Felipe Balbi
Hi,
Quoted 44 lines · Kevin Hilman
Quoted 2 lines · Felipe Balbi
current implementation doesn't take care about
drivers which don't provide*_noirqmethodsThe generic ops handle this. See below.
Quoted 2 lines
and we could fall into a situation where we would suspend/resume devices even though they haven't asked for it.
The following explanation doesn't explain this, so I dont' follow the "even though they haven't asked for it" part.
Quoted 4 lines
One such case happened with the I2C driver which was getting suspended during
suspend_noirq()just to be resume right after by any other device doing an I2C transfer on its suspend method.In order to be I2C to be runtime resumed after its noirq method has been called, that means the other device is doing an I2C xfer in its noirq method. That is a bug.
Quoted 10 lines
Signed-off-by Felipe Balbi <[email protected]>--- arch/arm/plat-omap/omap_device.c | 8 ++++++++ 1 file changed, 8 insertions(+)arch/arm/plat-omap/omap_device.c +0
-0arch/arm/plat-omap/omap_device.c static int _od_suspend_noirq(struct device *dev) · line 812
Quoted 9 lines
arch/arm/plat-omap/omap_device.c +4
-0@@ -804,8 +804,12 @@ static int _od_suspend_noirq(struct device *dev) {struct platform_device *pdev =to_platform_device (dev);struct omap_device *od =to_omap_device (pdev); +const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :NULL ;int ret; +if (!pm || !pm->suspend_noirq) +return 0 ; +you just re implemented
pm_generic_suspend_noirq()(c.f.drivers/base/power/generic_ops.c)Quoted 13 lines
arch/arm/plat-omap/omap_device.c static int _od_resume_noirq(struct device *dev) · line 837
Quoted 11 lines
arch/arm/plat-omap/omap_device.c +4
-0/* Don't attempt late suspend on a driver that is not bound */ if (od->_driver_status !=BUS_NOTIFY_BOUND_DRIVER )return 0 ;@@ -827,6 +831,10 @@ static int _od_resume_noirq(struct device *dev) {struct platform_device *pdev =to_platform_device (dev);struct omap_device *od =to_omap_device (pdev); +const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :NULL ; + +if (!pm || !pm->resume_noirq) +return 0 ;and this is basically
pm_generic_resume_noirq()not true, there is a slight different. Note that you only call
pm_generic_resume_noirq()after calling pm_runtime_resume()ing the device:Quoted 11 lines
static int
_od_resume_noirq(struct device *dev){ struct platform_device *pdev = to_platform_device(dev); struct omap_device *od = to_omap_device(pdev); if ((od->flags & OMAP_DEVICE_SUSPENDED) && !pm_runtime_status_suspended(dev)) { od->flags &= ~OMAP_DEVICE_SUSPENDED; if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND)) omap_device_enable(pdev); pm_generic_runtime_resume(dev);
Quoted 2 lines
right here. IMHO this is a bug, if the define doesn't implement
resume_noirq, it's telling you that it has nothing to do at that time,
This is where the misunderstanding is. I suggest you have a look at commit c03f007a8bf0e092caeb6856a5c8a850df10b974 where this feature was added, but I'll try to summarize:
The goal is simply this:
If, by the time .suspend_noirq has run, the driver is not already runtime suspended, then forcefully runtime suspend it.
That's it.
Again, this is required because system suspend disables runtime PM transitions at a certain point, if the device is used after that point, there is *no other way* for the driver to properly manage the idle transition (or, if there is, please explain it to me.)
Quoted 1 line
so you shouldn't forcefully resume the device.
Note it's only forcefully resumed *if* it was forcefully suspended by suspend_noirq.
Quoted 3 lines
If the device dies, then it means that it needs to implement
resume_noirq. What you have here, is a "workaround" for badly written device drivers IMHO.
That's one way of looking at it. The other way of looking at it is that by handling this at the PM domain level, the drivers can be much simpler, and thus less likely to get wrong.
But even then, with your proposed changes, I don't think the device will be properly idled (including the omap_device_idle) in these important cases:
1) I2C is used by some other device *after* its ->suspend method has been called.
2) I2C runtime PM is disabled from userspace
(echo off > /sys/devices/.../power/control)
but I'll take this up in the I2C driver patch itself.
Quoted 2 lines
Note also, that you could runtime resume devices which don't implement
resume_noirq().
Again, this is by design.
It doesn't matter if the driver has noirq methods. If it is not yet
runtime suspended, it is forceably runtime suspended. The generic
noirq calls are just there in case the driver has noirq callbacks.
Kevin
Quoted 6 lines
the same is valid for
suspend_noirq.Quoted 4 lines
} return pm_generic_resume_noirq(dev); }
PATCH 1/5 arm: omap: fix up _od_suspend_noirq and _od_resume_noirq
Hi,
Quoted 86 lines · Kevin Hilman
Quoted 77 lines · Felipe Balbi
Hi,
Quoted 44 lines · Kevin Hilman
Quoted 2 lines · Felipe Balbi
current implementation doesn't take care about
drivers which don't provide*_noirqmethodsThe generic ops handle this. See below.
Quoted 2 lines
and we could fall into a situation where we would suspend/resume devices even though they haven't asked for it.
The following explanation doesn't explain this, so I dont' follow the "even though they haven't asked for it" part.
Quoted 4 lines
One such case happened with the I2C driver which was getting suspended during
suspend_noirq()just to be resume right after by any other device doing an I2C transfer on its suspend method.In order to be I2C to be runtime resumed after its noirq method has been called, that means the other device is doing an I2C xfer in its noirq method. That is a bug.
Quoted 10 lines
Signed-off-by Felipe Balbi <[email protected]>--- arch/arm/plat-omap/omap_device.c | 8 ++++++++ 1 file changed, 8 insertions(+)arch/arm/plat-omap/omap_device.c +0
-0arch/arm/plat-omap/omap_device.c static int _od_suspend_noirq(struct device *dev) · line 812
Quoted 9 lines
arch/arm/plat-omap/omap_device.c +4
-0@@ -804,8 +804,12 @@ static int _od_suspend_noirq(struct device *dev) {struct platform_device *pdev =to_platform_device (dev);struct omap_device *od =to_omap_device (pdev); +const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :NULL ;int ret; +if (!pm || !pm->suspend_noirq) +return 0 ; +you just re implemented
pm_generic_suspend_noirq()(c.f.drivers/base/power/generic_ops.c)Quoted 13 lines
arch/arm/plat-omap/omap_device.c static int _od_resume_noirq(struct device *dev) · line 837
Quoted 11 lines
arch/arm/plat-omap/omap_device.c +4
-0/* Don't attempt late suspend on a driver that is not bound */ if (od->_driver_status !=BUS_NOTIFY_BOUND_DRIVER )return 0 ;@@ -827,6 +831,10 @@ static int _od_resume_noirq(struct device *dev) {struct platform_device *pdev =to_platform_device (dev);struct omap_device *od =to_omap_device (pdev); +const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :NULL ; + +if (!pm || !pm->resume_noirq) +return 0 ;and this is basically
pm_generic_resume_noirq()not true, there is a slight different. Note that you only call
pm_generic_resume_noirq()after calling pm_runtime_resume()ing the device:Quoted 11 lines
static int
_od_resume_noirq(struct device *dev){ struct platform_device *pdev = to_platform_device(dev); struct omap_device *od = to_omap_device(pdev); if ((od->flags & OMAP_DEVICE_SUSPENDED) && !pm_runtime_status_suspended(dev)) { od->flags &= ~OMAP_DEVICE_SUSPENDED; if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND)) omap_device_enable(pdev); pm_generic_runtime_resume(dev);Quoted 2 lines
right here. IMHO this is a bug, if the define doesn't implement
resume_noirq, it's telling you that it has nothing to do at that time,This is where the misunderstanding is. I suggest you have a look at commit
c03f007a8bf0e092caeb6856a5c8a850df10b974where this feature was added, but I'll try to summarize:
Quoted 6 lines
The goal is simply this:
If, by the time .suspend_noirq has run, the driver is not already runtime suspended, then forcefully runtime suspend it.
That's it.
Yes, I got the intention, but to me it looks wrong.
Quoted 4 lines
Again, this is required because system suspend disables runtime PM transitions at a certain point, if the device is used after that point, there is *no other way* for the driver to properly manage the idle transition (or, if there is, please explain it to me.)
Can you please let me know of any situation where e.g. I2C/SPI would be needed after all its children are suspended ?
Quoted 4 lines
Quoted 1 line
so you shouldn't forcefully resume the device.
Note it's only forcefully resumed *if* it was forcefully suspended by
suspend_noirq.
likewise, you shouldn't forcefully runtime suspend a device. The device driver needs to be required to provide suspend/resume functions if it cares.
Quoted 7 lines
Quoted 3 lines
If the device dies, then it means that it needs to implement
resume_noirq. What you have here, is a "workaround" for badly written device drivers IMHO.That's one way of looking at it. The other way of looking at it is that by handling this at the PM domain level, the drivers can be much simpler, and thus less likely to get wrong.
You're still bypassing what the PM framework wants us to do, no ? What if Rafael decides to drastically change the way system and runtime PM is done and it ends up breaking what we have on OMAP ? If we stick to the standard, the it's less likely to brea.
Quoted 5 lines
But even then, with your proposed changes, I don't think the device will be properly idled (including the
omap_device_idle) in these important cases:1) I2C is used by some other device *after* its ->suspend method has been called.
how can that happen ? I2C's ->suspend method will only be called after all its children are suspended.
Quoted 2 lines
2) I2C runtime PM is disabled from userspace
(echo off >/sys/devices/.../power/control)
that should not make a difference if you omap_device_idle() is written as it should. Maybe what you need is a refactor to provide omap_device_idle() and omap_device_runtime_idle(), the latter taking care of the case where runtime pm can be disabled from userspace while the former idling whenever it's called.
Quoted 1 line
but I'll take this up in the I2C driver patch itself.
sure :-)
Quoted 4 lines
Quoted 2 lines
Note also, that you could runtime resume devices which don't implement
resume_noirq().Again, this is by design.
a very weird design IMHO. Either that or I'm really missing something here.
Quoted 2 lines
It doesn't matter if the driver has noirq methods. If it is not yet
runtime suspended, it is forceably runtime suspended. The generic
if it's not yet runtime suspended, you need to call the driver's ->suspend_* method (depending on the suspend phase you are right now), but "reusing" the runtime_suspend method sounds to me like another "special OMAP requirement".
Quoted 1 line
noirq calls are just there in case the driver has noirq callbacks.
I get that, but why would you suspend a device which doesn't want to be suspended in suspend_noirq(), but using its runtime_suspend method ?
If I2C dies on a suspend/resume transition, it's a bug in the I2C driver, no ?
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121018/687f60a5/attachment.sig>
PATCH 1/5 arm: omap: fix up _od_suspend_noirq and _od_resume_noirq
Quoted 98 lines · Felipe Balbi
Hi,
Quoted 86 lines · Kevin Hilman
Quoted 77 lines · Felipe Balbi
Hi,
Quoted 44 lines · Kevin Hilman
Quoted 2 lines · Felipe Balbi
current implementation doesn't take care about
drivers which don't provide*_noirqmethodsThe generic ops handle this. See below.
Quoted 2 lines
and we could fall into a situation where we would suspend/resume devices even though they haven't asked for it.
The following explanation doesn't explain this, so I dont' follow the "even though they haven't asked for it" part.
Quoted 4 lines
One such case happened with the I2C driver which was getting suspended during
suspend_noirq()just to be resume right after by any other device doing an I2C transfer on its suspend method.In order to be I2C to be runtime resumed after its noirq method has been called, that means the other device is doing an I2C xfer in its noirq method. That is a bug.
Quoted 10 lines
Signed-off-by Felipe Balbi <[email protected]>--- arch/arm/plat-omap/omap_device.c | 8 ++++++++ 1 file changed, 8 insertions(+)arch/arm/plat-omap/omap_device.c +0
-0arch/arm/plat-omap/omap_device.c static int _od_suspend_noirq(struct device *dev) · line 812
Quoted 9 lines
arch/arm/plat-omap/omap_device.c +4
-0@@ -804,8 +804,12 @@ static int _od_suspend_noirq(struct device *dev) {struct platform_device *pdev =to_platform_device (dev);struct omap_device *od =to_omap_device (pdev); +const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :NULL ;int ret; +if (!pm || !pm->suspend_noirq) +return 0 ; +you just re implemented
pm_generic_suspend_noirq()(c.f.drivers/base/power/generic_ops.c)Quoted 13 lines
arch/arm/plat-omap/omap_device.c static int _od_resume_noirq(struct device *dev) · line 837
Quoted 11 lines
arch/arm/plat-omap/omap_device.c +4
-0/* Don't attempt late suspend on a driver that is not bound */ if (od->_driver_status !=BUS_NOTIFY_BOUND_DRIVER )return 0 ;@@ -827,6 +831,10 @@ static int _od_resume_noirq(struct device *dev) {struct platform_device *pdev =to_platform_device (dev);struct omap_device *od =to_omap_device (pdev); +const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :NULL ; + +if (!pm || !pm->resume_noirq) +return 0 ;and this is basically
pm_generic_resume_noirq()not true, there is a slight different. Note that you only call
pm_generic_resume_noirq()after calling pm_runtime_resume()ing the device:Quoted 11 lines
static int
_od_resume_noirq(struct device *dev){ struct platform_device *pdev = to_platform_device(dev); struct omap_device *od = to_omap_device(pdev); if ((od->flags & OMAP_DEVICE_SUSPENDED) && !pm_runtime_status_suspended(dev)) { od->flags &= ~OMAP_DEVICE_SUSPENDED; if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND)) omap_device_enable(pdev); pm_generic_runtime_resume(dev);Quoted 2 lines
right here. IMHO this is a bug, if the define doesn't implement
resume_noirq, it's telling you that it has nothing to do at that time,This is where the misunderstanding is. I suggest you have a look at commit
c03f007a8bf0e092caeb6856a5c8a850df10b974where this feature was added, but I'll try to summarize:Quoted 6 lines
The goal is simply this:
If, by the time .suspend_noirq has run, the driver is not already runtime suspended, then forcefully runtime suspend it.
That's it.
Yes, I got the intention, but to me it looks wrong.
Quoted 7 lines
Quoted 4 lines
Again, this is required because system suspend disables runtime PM transitions at a certain point, if the device is used after that point, there is *no other way* for the driver to properly manage the idle transition (or, if there is, please explain it to me.)
Can you please let me know of any situation where e.g. I2C/SPI would be needed after all its children are suspended ?
There lots of I2C users who are not children of the I2C bus device. Any users of regulators on the I2C-connected PMIC for example (MMC comes to mind, and is not a child of the I2C bus.). Also, enable/disable of GPIO IRQ lines on I2C GPIO expanders. The devices using those GPIO IRQs are not children of the I2C bus.
Stated differently, I2C devices themselves (PMIC, GPIO expanders, etc.) are children of the I2C bus device. But *users* of those devices will be other drivers who have no parent/child relationship with the I2C bus device in the driver model.
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
so you shouldn't forcefully resume the device.
Note it's only forcefully resumed *if* it was forcefully suspended by
suspend_noirq.likewise, you shouldn't forcefully runtime suspend a device. The device driver needs to be required to provide suspend/resume functions if it cares.
I don't think you've fully understood the problem being solved here.
This is for devices who designed for use throughout the suspend process (IOW, *after* their suspend callbacks have been called.)
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
If the device dies, then it means that it needs to implement
resume_noirq. What you have here, is a "workaround" for badly written device drivers IMHO.
Quoted 8 lines
Quoted 3 lines
That's one way of looking at it. The other way of looking at it is that by handling this at the PM domain level, the drivers can be much simpler, and thus less likely to get wrong.
You're still bypassing what the PM framework wants us to do, no ? What if Rafael decides to drastically change the way system and runtime PM is done and it ends up breaking what we have on OMAP ? If we stick to the standard, the it's less likely to brea.
What I have done on OMAP was done in direct collaboration with the PM core mantainers, and designed specifically for limitations in the PM core. If those assumptions change, I will have to change. I have no problems dealing with that if/when the time comes.
Quoted 8 lines
Quoted 5 lines
But even then, with your proposed changes, I don't think the device will be properly idled (including the
omap_device_idle) in these important cases:1) I2C is used by some other device *after* its ->suspend method has been called.
how can that happen ? I2C's ->suspend method will only be called after all its children are suspended.
Unfortunately, drivers which use I2C are not all children of the I2C bus device in the driver model (see above.)
This gets to a much bigger problem with the linux driver model today. There are *many* power relationships/dependencies between devices that are not modeled by driver model device hierarchy.
Quoted 8 lines
Quoted 2 lines
2) I2C runtime PM is disabled from userspace
(echo off >/sys/devices/.../power/control)that should not make a difference if you
omap_device_idle()is written as it should. Maybe what you need is a refactor to provideomap_device_idle()andomap_device_runtime_idle(), the latter taking care of the case where runtime pm can be disabled from userspace while the former idling whenever it's called.
No thanks.
Quoted 11 lines
Quoted 1 line
but I'll take this up in the I2C driver patch itself.
sure :-)
Quoted 4 lines
Quoted 2 lines
Note also, that you could runtime resume devices which don't implement
resume_noirq().Again, this is by design.
a very weird design IMHO. Either that or I'm really missing something here.
I agree it's wierd. Unfortunately, it's necessary.
Quoted 5 lines
Quoted 2 lines
It doesn't matter if the driver has noirq methods. If it is not yet
runtime suspended, it is forceably runtime suspended. The genericif it's not yet runtime suspended, you need to call the driver's ->suspend_* method (depending on the suspend phase you are right now),
You're confusing system suspend and runtime suspend, and forgetting that we might want to use runtime PM *during* system suspend/resume.
Again, the reason a device may not yet runtime suspended is because it was in use during suspend, and runtime PM has been disabled by the PM core during system PM. If the PM core did not disable runtime PM, the drivers runtime PM methods would be called, and we would not need to implement this in the PM domain layer.
Quoted 2 lines
but "reusing" the
runtime_suspendmethod sounds to me like another "special OMAP requirement".
There's nothing special about OMAP here. Any devices that want to use runtime PM *throughout* the suspend path has this requirment, and the implementation used in our PM domain layer was implemented based on the recommendation of Rafael.
Quoted 4 lines
Quoted 1 line
noirq calls are just there in case the driver has noirq callbacks.
I get that, but why would you suspend a device which doesn't want to be suspended in
suspend_noirq(), but using itsruntime_suspendmethod ?
What do you mean "doesn't want to be suspended"? If a device doesn't want to be suspended, it needs to return an error from one of its
suspend methods. Barring that, the system *will* suspend, and in order
to hit low power states, we have to properly idle each device.
The underlying assumption is that by the time the noirq phase runs, the device should already be runtime suspended. If it is not, then one of the following ha happened:
- device was used during suspend, but after runtime PM was disabled by the PM core (during system suspend)
- runtime PM disabled from userspace
In both cases, *somebody* has to properly idle the device and "finish" the runtime suspend late in the suspend phase. As recommended by Rafael, we handle this at the PM domain level.
Quoted 2 lines
If I2C dies on a suspend/resume transition, it's a bug in the I2C driver, no ?
Sure, but this has nothing to do with I2C dying.
Kevin
PATCH 1/5 arm: omap: fix up _od_suspend_noirq and _od_resume_noirq
Hi,
Quoted 5 lines · Kevin Hilman
Quoted 5 lines
Quoted 4 lines
Again, this is required because system suspend disables runtime PM transitions at a certain point, if the device is used after that point, there is *no other way* for the driver to properly manage the idle transition (or, if there is, please explain it to me.)
Quoted 8 lines
Quoted 2 lines
Can you please let me know of any situation where e.g. I2C/SPI would be needed after all its children are suspended ?
There lots of I2C users who are not children of the I2C bus device. Any users of regulators on the I2C-connected PMIC for example (MMC comes to mind, and is not a child of the I2C bus.). Also, enable/disable of GPIO IRQ lines on I2C GPIO expanders. The devices using those GPIO IRQs are not children of the I2C bus.
MMC isn't a child of I2C, indeed, but the regulator is a child of TWL which is a child of the I2C bus.
Quoted 4 lines
Stated differently, I2C devices themselves (PMIC, GPIO expanders, etc.) are children of the I2C bus device. But *users* of those devices will be other drivers who have no parent/child relationship with the I2C bus device in the driver model.
correct, but MMC is not suspended as late as suspend_noirq() is it ? Looks like only I2C can fall into this category because of exactly regulators and I/O expanders. So wouldn't implementing omap_i2c_suspend_noirq() and omap_i2c_resume_noirq() solve the same problem ?
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
Quoted 1 line
so you shouldn't forcefully resume the device.
Note it's only forcefully resumed *if* it was forcefully suspended by
suspend_noirq.
Quoted 8 lines
Quoted 3 lines
likewise, you shouldn't forcefully runtime suspend a device. The device driver needs to be required to provide suspend/resume functions if it cares.
I don't think you've fully understood the problem being solved here.
This is for devices who designed for use throughout the suspend process (IOW, *after* their suspend callbacks have been called.)
is there no way for a device to tell PM core that it doesn't want to be suspended during ->suspend_early or ->suspend but only on ->suspend_noirq ? Isn't the fact that we don't implement ->suspend_early and ->suspend enough to keep the device running ? I think that will be the case with the patch which adds the extra check on _od_suspend_noirq(), no ?
And before you reply, if the device implements none of the suspend methods, then you shouldn't suspend it at all, because you'd be masking a bug in the driver, IMHO.
Quoted 9 lines
Quoted 8 lines
Quoted 5 lines
But even then, with your proposed changes, I don't think the device will be properly idled (including the
omap_device_idle) in these important cases:1) I2C is used by some other device *after* its ->suspend method has been called.
how can that happen ? I2C's ->suspend method will only be called after all its children are suspended.
Quoted 6 lines
Unfortunately, drivers which use I2C are not all children of the I2C bus device in the driver model (see above.)
This gets to a much bigger problem with the linux driver model today. There are *many* power relationships/dependencies between devices that are not modeled by driver model device hierarchy.
Ok. Now I see a good point :-) You're correct, thanks
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
It doesn't matter if the driver has noirq methods. If it is not yet
runtime suspended, it is forceably runtime suspended. The genericif it's not yet runtime suspended, you need to call the driver's ->suspend_* method (depending on the suspend phase you are right now),
You're confusing system suspend and runtime suspend, and forgetting that we might want to use runtime PM *during* system suspend/resume.
Maybe I wasn't clear enough. What I meant to say was that if the device isn't runtime_suspended by the time you should be calling its ->suspend_noirq method, then calling the device's runtime_suspend() method looks very weird to me.
It really seems, to me, that implementing suspend_noirq() on e.g. I2C controller driver should do the trick provided you call ->suspend_noirq() instead of ->runtime_suspend(). I mean, if we do exact the same thing on runtime_suspend() and suspend_noirq() (namely disable IRQs, save context, etc), then it should make no difference which method you call, right ?
Quoted 5 lines
Again, the reason a device may not yet runtime suspended is because it was in use during suspend, and runtime PM has been disabled by the PM core during system PM. If the PM core did not disable runtime PM, the drivers runtime PM methods would be called, and we would not need to implement this in the PM domain layer.
fair enough, but what I still don't get is why you need to call exactly ->runtime_suspend. What's the problem with calling ->suspend_noirq() ?
Again, look at the last patch. Note that I do the same thing on both ->suspend (should be ->suspend_noirq()) and ->runtime_suspend(), with a slight difference:
on ->suspend(_noirq) if the device is already runtime_suspended, I just return 0, otherwise I follow the same procedure and (due to the other patches) omap_device_idle() will be called the same way. The device will be suspended, but (let's assume) PM runtime doesn't know about it. No problem, during resume you will call omap_device_resume() and later call pm_generic_resume(_noirq) which will call omap_i2c_resume(_noirq) which will:
reenable_irqs();restore_context();
pm_runtime_disable(dev);pm_runtime_set_active(dev); /* we have already resumed the device */ pm_runtime_enable(dev);
return 0;
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
noirq calls are just there in case the driver has noirq callbacks.
I get that, but why would you suspend a device which doesn't want to be suspended in
suspend_noirq(), but using itsruntime_suspendmethod ?What do you mean "doesn't want to be suspended"? If a device doesn't
by "doesn't want to be suspended" I mean that it hasn't implemented a noirq method.
Quoted 8 lines
want to be suspended, it needs to return an error from one of its
suspend methods. Barring that, the system *will* suspend, and in order
to hit low power states, we have to properly idle each device.The underlying assumption is that by the time the noirq phase runs, the device should already be runtime suspended. If it is not, then one of the following ha happened:
Quoted 6 lines
- device was used during suspend, but after runtime PM was disabled by the PM core (during system suspend)
- runtime PM disabled from userspace
In both cases, *somebody* has to properly idle the device and "finish"
yes, and that somebody is omap_device_idle() right ? Now my question
remains: why is it so important to call device's ->runtime_suspend method. Why couldn't you call device's ->suspend_noirq method instead ?
Quoted 7 lines
the runtime suspend late in the suspend phase. As recommended by Rafael, we handle this at the PM domain level.
Quoted 2 lines
If I2C dies on a suspend/resume transition, it's a bug in the I2C driver, no ?
Sure, but this has nothing to do with I2C dying.
Imagine that instead of calling ->runtime_suspend we were to call ->suspend_noirq, but the device doesn't implement that. What would happen ? no context save, no context restore, I2C dead.
ps: I'll continue reading the code and further test my series to see if I can understand what you say here.
cheers
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121018/7af66528/attachment.sig>
PATCH 1/5 arm: omap: fix up _od_suspend_noirq and _od_resume_noirq
Felipe Balbi <[email protected]> writes:
[...]
Quoted 3 lines
if the device implements none of the suspend methods, then you shouldn't suspend it at all, because you'd be masking a bug in the driver, IMHO.
OK, let's start over here, because here's the fundamental difference. I don't think missing suspend methods means a bug in the driver at all.
For a moment, let's ignore the limitations/restrictions that exist today betwen system PM and runtime PM and assume runtime PM can be used anytime, including anywhere in the suspend/resume path.
In that ideal world, many drivers would not need system suspend methods at all. They would just runtime PM to enable/disable themselves based on driver/device activity.
Stated differently, since the driver is using runtime PM, when system suspend happens, the device is already idle, and runtime suspended, so there is nothing for the system PM methods to do. Therefore, no system PM callbacks are needed. No bug in driver.
(yes, this is oversimplified to illustrate the goal/point. Some drivers may want to have a ->suspend callback to wait/stop any current processing, but that is just so runtime PM can kick in.)
This is the "runtime PM centric" view of things that we have been driving towards with OMAP drivers (we've been doing this with aggressive clock gating even before runtime PM framework.)
In other words, if drivers are written with this "runtime PM centric" view, there should be almost nothing to do in the suspend method, except quiesce the hardware so runtime PM kicks in.
This runtime PM centric view is the mindset/philosophy behind the PM domain implementation, and driver PM adaptations that we've been doing for some time.
In converting/reviewing/testing *many* drivers that have been PM adapted (for system PM and runtime PM), most folks do not get this right. Therfore, if driver writers can concentrate on runtime PM, and just use system PM as a "quiesce HW, let runtime PM take over" method, that is where I want to go.
Kevin
PATCH 1/5 arm: omap: fix up _od_suspend_noirq and _od_resume_noirq
In reply to Felipe BalbiFelipe Balbi <[email protected]> writes:
[...]
Quoted 2 lines
ps: I'll continue reading the code and further test my series to see if I can understand what you say here.
OK. And please get it working in cases where drivers are using I2C in their suspend/resume (and even late/early) paths, and also where device runtime PM is disabled from userspace. All of that works with today's code and will not work with your proposed code.
Kevin
PATCH 2/5 arm: omap: don't forcefully runtime suspend a device
In reply to Felipe Balbidevice drivers should be smart enough to provide ->suspend/->resume callbacks when needed and they should take care of doing whatever needs to be done in order to allow a device to be suspended.
Calling pm_runtime_* from system suspend isn't the right way to achieve that, as it creates a situation where OMAP's PM has different requirements and semantics than all other architectures.
--- arch/arm/plat-omap/omap_device.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
arch/arm/plat-omap/omap_device.c +3 -6
@@ -817,11 +817,9 @@ static int _od_suspend_noirq(struct device *dev) ret =pm_generic_suspend_noirq (dev);if (!ret && !pm_runtime_status_suspended (dev)) { -if (pm_generic_runtime_suspend (dev) ==0 ) { -if (!(od->flags &OMAP_DEVICE_NO_IDLE_ON_SUSPEND )) -omap_device_idle (pdev); - od->flags |=OMAP_DEVICE_SUSPENDED ; - } +if (!(od->flags &OMAP_DEVICE_NO_IDLE_ON_SUSPEND )) +omap_device_idle (pdev); + od->flags |=OMAP_DEVICE_SUSPENDED ; }return ret;
@@ -841,7 +839,6 @@ static int _od_resume_noirq(struct device *dev) od->flags &= ~OMAP_DEVICE_SUSPENDED ;if (!(od->flags &OMAP_DEVICE_NO_IDLE_ON_SUSPEND ))omap_device_enable (pdev); -pm_generic_runtime_resume (dev); }return pm_generic_resume_noirq (dev);
Signature · 1 line
1.8.0.rc0
Kevin Hilman
NAKPATCH 2/5 arm: omap: don't forcefully runtime suspend a device
Quoted 11 lines · Felipe Balbi
device drivers should be smart enough to provide ->suspend/->resume callbacks when needed and they should take care of doing whatever needs to be done in order to allow a device to be suspended.
Calling pm_runtime_* from system suspend isn't the right way to achieve that, as it creates a situation where OMAP's PM has different requirements and semantics than all other architectures.
Signed-off-by Felipe Balbi <[email protected]>
NAK
This support is required to handle some restrictions placed on runtime
PM and system PM interactions. Basically, runtime PM transitions are
disabled part way through system PM (that itself was a much debated topic last year, but that's how it works today.)
Because of this limitation, drivers that are active during the suspend phase (commonly becasue they are used by [late] suspend methods of other devices) may have multiple runtime PM transitions during static suspend/resume. These drivers have the problem that after runtime PM has been disabled, even when they pm_runtime_put*, they will not actually be transitioned (and their runtime PM callbacks will not be called.)
So these devices are in a "ready to runtime suspend" state, but they will not transition because runtime PM is disabled.
After your patch, they will still be idled (omap_device_idle), but the driver will have no notification that this has happened because you removed the calling of the runtime PM callbacks.
In the changelog, you seem to be implying that anything the driver should be doing should be done in its suspend/resume callbacks instead of the runtime suspend/resume callbacks (but don't give your reasoning.)
Using the current approach (which was actually suggested by Rafael), it means many transiactional drivers (like I2C) can be implemented as runtime PM only, and don't need to provide suspend/resume callbacks at all. It also means they can be used throughout the suspend/resume path (well until noirq methods.)
The approach in $SUBJECT patch would mean that drivers should not be used after their suspend method has been called. That places some severe limitations on drivers like I2C, SPI, HSI, UART etc. that are often used by the suspend/resume methods of other drivers.
Kevin
Quoted 31 lines
--- arch/arm/plat-omap/omap_device.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)arch/arm/plat-omap/omap_device.c +3
-6@@ -817,11 +817,9 @@ static int _od_suspend_noirq(struct device *dev) ret =pm_generic_suspend_noirq (dev);if (!ret && !pm_runtime_status_suspended (dev)) { -if (pm_generic_runtime_suspend (dev) ==0 ) { -if (!(od->flags &OMAP_DEVICE_NO_IDLE_ON_SUSPEND )) -omap_device_idle (pdev); - od->flags |=OMAP_DEVICE_SUSPENDED ; - } +if (!(od->flags &OMAP_DEVICE_NO_IDLE_ON_SUSPEND )) +omap_device_idle (pdev); + od->flags |=OMAP_DEVICE_SUSPENDED ; }return ret;@@ -841,7 +839,6 @@ static int _od_resume_noirq(struct device *dev) od->flags &= ~OMAP_DEVICE_SUSPENDED ;if (!(od->flags &OMAP_DEVICE_NO_IDLE_ON_SUSPEND ))omap_device_enable (pdev); -pm_generic_runtime_resume (dev); }return pm_generic_resume_noirq (dev);
PATCH 2/5 arm: omap: don't forcefully runtime suspend a device
Hi,
Quoted 29 lines · Kevin Hilman
Quoted 11 lines · Felipe Balbi
device drivers should be smart enough to provide ->suspend/->resume callbacks when needed and they should take care of doing whatever needs to be done in order to allow a device to be suspended.
Calling pm_runtime_* from system suspend isn't the right way to achieve that, as it creates a situation where OMAP's PM has different requirements and semantics than all other architectures.
Signed-off-by Felipe Balbi <[email protected]>NAK
This support is required to handle some restrictions placed on runtime
PM and system PM interactions. Basically, runtime PM transitions are
disabled part way through system PM (that itself was a much debated topic last year, but that's how it works today.)Because of this limitation, drivers that are active during the suspend phase (commonly becasue they are used by [late] suspend methods of other devices) may have multiple runtime PM transitions during static suspend/resume. These drivers have the problem that after runtime PM has been disabled, even when they pm_runtime_put*, they will not actually be transitioned (and their runtime PM callbacks will not be called.)
Quoted 6 lines
So these devices are in a "ready to runtime suspend" state, but they will not transition because runtime PM is disabled.
After your patch, they will still be idled (
omap_device_idle), but the driver will have no notification that this has happened because you removed the calling of the runtime PM callbacks.
to me this only seems like the driver misses some static PM callbacks and some pm_runtime_disable; pm_runtime_set_active; pm_runtime_enable; on their system resume methods.
Quoted 3 lines
In the changelog, you seem to be implying that anything the driver should be doing should be done in its suspend/resume callbacks instead of the runtime suspend/resume callbacks (but don't give your reasoning.)
that's not what I implied at all. What I imply is that if driver needs to do something during system suspend/resume, then it needs to implement those methods. Calling runtime_* methods from system PM looks like a workaround for incomplete drivers.
Quoted 3 lines
Using the current approach (which was actually suggested by Rafael), it means many transiactional drivers (like I2C) can be implemented as runtime PM only, and don't need to provide suspend/resume callbacks at
to me that sounds bogus :-s what's the problem of using the same function for system suspend and runtime suspend ? (see the last patch of the series where I implement I2C system suspend and resume)
Quoted 2 lines
all. It also means they can be used throughout the suspend/resume path (well until noirq methods.)
fair enough...
Quoted 4 lines
The approach in $SUBJECT patch would mean that drivers should not be used after their suspend method has been called. That places some severe limitations on drivers like I2C, SPI, HSI, UART etc. that are often used by the suspend/resume methods of other drivers.
I can't see the limitation you talk about. Let's use I2C as an example. In order to suspend RTC we need I2C resume, but driver core guarantees that parent will only suspend after all children are suspended. If that's as far as _noirq, then I2C needs to implement _noirq callbacks.
I don't see the issue because RTC is a child of I2C and will suspend before its parent. No ?
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121018/30dddd42/attachment-0001.sig>
PATCH 3/5 arm: omap: introduce other PM methods
In reply to Felipe Balbicurrent omap_device PM implementation defines
omap-specific *_noirq methods but uses the
generic versions for all other PM methods.
As it turns out, if a device decides to implement
non-runtime PM callbacks, we might fall into a
situation where the hwmod is still idled which
will generate an abort exception when we try
to access device's address space while clocks
are still gated.
In order to solve that, we implement all other
methods taking into account that devices might
not implement those, in which case we return
early.
---
notice here that it would be far better to avoid the code duplication and have another function (e.g. _od_generic_suspend/resume) which would receive pm_message_t and omap_device as arguments so it can decide what to do.
That can be done on a later version, though.
arch/arm/plat-omap/omap_device.c | 145 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 144 insertions(+), 1 deletion(-)
arch/arm/plat-omap/omap_device.c +144 -1
@@ -843,16 +843,159 @@ static int _od_resume_noirq(struct device *dev)return pm_generic_resume_noirq (dev); } + +static int _od_suspend (struct device *dev) +{ +struct platform_device *pdev =to_platform_device (dev); +struct omap_device *od =to_omap_device (pdev); +const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :NULL ; +int ret; + +if (!pm || !pm->suspend) +return 0 ; + +/* Don't attempt suspend on a driver that is not bound */ +if (od->_driver_status !=BUS_NOTIFY_BOUND_DRIVER ) +return 0 ; + + ret =pm_generic_suspend (dev); +if (ret) +return ret; + +if (!(od->flags &OMAP_DEVICE_NO_IDLE_ON_SUSPEND )) +omap_device_idle (pdev); + od->flags |=OMAP_DEVICE_SUSPENDED ; + +return 0 ; +} + +static int _od_resume (struct device *dev) +{ +struct platform_device *pdev =to_platform_device (dev); +struct omap_device *od =to_omap_device (pdev); +const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :NULL ; + +if (!pm || !pm->resume) +return 0 ; + +if (od->flags &OMAP_DEVICE_SUSPENDED ) { + od->flags &= ~OMAP_DEVICE_SUSPENDED ; + +if (!(od->flags &OMAP_DEVICE_NO_IDLE_ON_SUSPEND )) +omap_device_enable (pdev); + } + +return pm_generic_resume (dev); +} + +static int _od_freeze (struct device *dev) +{ +struct platform_device *pdev =to_platform_device (dev); +struct omap_device *od =to_omap_device (pdev); +const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :NULL ; +int ret; + +if (!pm || !pm->freeze) +return 0 ; + +/* Don't attempt late suspend on a driver that is not bound */ +if (od->_driver_status !=BUS_NOTIFY_BOUND_DRIVER ) +return 0 ; + + ret =pm_generic_freeze (dev); +if (ret) +return ret; + +if (!(od->flags &OMAP_DEVICE_NO_IDLE_ON_SUSPEND )) +omap_device_idle (pdev); + od->flags |=OMAP_DEVICE_SUSPENDED ; + +return 0 ; +} + +static int _od_thaw (struct device *dev) +{ +struct platform_device *pdev =to_platform_device (dev); +struct omap_device *od =to_omap_device (pdev); +const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :NULL ; + +if (!pm || !pm->thaw) +return 0 ; + +if (od->flags &OMAP_DEVICE_SUSPENDED ) { + od->flags &= ~OMAP_DEVICE_SUSPENDED ; + +if (!(od->flags &OMAP_DEVICE_NO_IDLE_ON_SUSPEND )) +omap_device_enable (pdev); + } + +return pm_generic_thaw (dev); +} + +static int _od_poweroff (struct device *dev) +{ +struct platform_device *pdev =to_platform_device (dev); +struct omap_device *od =to_omap_device (pdev); +const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :NULL ; +int ret; + +if (!pm || !pm->poweroff) +return 0 ; + +/* Don't attempt late suspend on a driver that is not bound */ +if (od->_driver_status !=BUS_NOTIFY_BOUND_DRIVER ) +return 0 ; + + ret =pm_generic_poweroff (dev); +if (ret) +return ret; + +if (!(od->flags &OMAP_DEVICE_NO_IDLE_ON_SUSPEND )) +omap_device_idle (pdev); + od->flags |=OMAP_DEVICE_SUSPENDED ; + +return 0 ; +} + +static int _od_restore (struct device *dev) +{ +struct platform_device *pdev =to_platform_device (dev); +struct omap_device *od =to_omap_device (pdev); +const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :NULL ; + +if (!pm || !pm->restore) +return 0 ; + +if (od->flags &OMAP_DEVICE_SUSPENDED ) { + od->flags &= ~OMAP_DEVICE_SUSPENDED ; + +if (!(od->flags &OMAP_DEVICE_NO_IDLE_ON_SUSPEND )) +omap_device_enable (pdev); + } + +return pm_generic_restore (dev); +}#else #define _od_suspend_noirq NULL#define _od_resume_noirq NULL +#define _od_suspend NULL +#define _od_resume NULL +#define _od_freeze NULL +#define _od_thaw NULL +#define _od_poweroff NULL +#define _od_restore NULL#endif struct dev_pm_domain omap_device_pm_domain = { .ops = {SET_RUNTIME_PM_OPS (_od_runtime_suspend, _od_runtime_resume, _od_runtime_idle) - USE_PLATFORM_PM_SLEEP_OPS + .suspend = _od_suspend, + .resume = _od_resume, + .freeze = _od_freeze, + .thaw = _od_thaw, + .poweroff = _od_poweroff, + .restore = _od_restore, .suspend_noirq = _od_suspend_noirq, .resume_noirq = _od_resume_noirq, }
Signature · 1 line
1.8.0.rc0
PATCH 3/5 arm: omap: introduce other PM methods
Quoted 10 lines · Felipe Balbi
current
omap_devicePM implementation defines omap-specific*_noirqmethods but uses the generic versions for all other PM methods.As it turns out, if a device decides to implement non-runtime PM callbacks, we might fall into a situation where the hwmod is still idled which will generate an abort exception when we try to access device's address space while clocks are still gated.
Please explain in more detail how this could happen.
Kevin
Quoted 6 lines
In order to solve that, we implement all other methods taking into account that devices might not implement those, in which case we return early.
Signed-off-by Felipe Balbi <[email protected]>
PATCH 3/5 arm: omap: introduce other PM methods
Hi,
Quoted 6 lines · Kevin Hilman
Quoted 4 lines · Felipe Balbi
current
omap_devicePM implementation defines omap-specific*_noirqmethods but uses the generic versions for all other PM methods.
Quoted 8 lines
Quoted 6 lines
As it turns out, if a device decides to implement non-runtime PM callbacks, we might fall into a situation where the hwmod is still idled which will generate an abort exception when we try to access device's address space while clocks are still gated.
Please explain in more detail how this could happen.
just follow the patchset. If I implement system suspend and I'm not focefully calling runtime_* methods, who will be there to call omap_device_enable() ?
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121018/eecb1070/attachment.sig>
PATCH 3/5 arm: omap: introduce other PM methods
Hi,
Quoted 16 lines · Felipe Balbi
Hi,
Quoted 13 lines · Kevin Hilman
Quoted 10 lines · Felipe Balbi
current
omap_devicePM implementation defines omap-specific*_noirqmethods but uses the generic versions for all other PM methods.As it turns out, if a device decides to implement non-runtime PM callbacks, we might fall into a situation where the hwmod is still idled which will generate an abort exception when we try to access device's address space while clocks are still gated.
Quoted 5 lines
Quoted 1 line
Please explain in more detail how this could happen.
just follow the patchset. If I implement system suspend and I'm not focefully calling runtime_* methods, who will be there to call
omap_device_enable()?
let me rephrase this. If we exit _noirq right in the beginning with the extra check I've added, who will call omap_device_enable() ?
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121018/dca978dd/attachment.sig>
PATCH 4/5 i2c: omap: don't re-enable IRQs after masking them
In reply to Felipe BalbiOMAP I2C driver will re-enable IRQs right after
masking them during suspend.
That's not what we want. We want to keep IRQs
masked until our resume method is called.
--- drivers/i2c/busses/i2c-omap.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
drivers/i2c/busses/i2c-omap.c +2 -8
@@ -1247,14 +1247,8 @@ static int omap_i2c_runtime_suspend(struct device *dev)omap_i2c_write_reg (_dev,OMAP_I2C_IE_REG ,0 ); -if (_dev->rev <OMAP_I2C_OMAP1_REV_2 ) { - iv =omap_i2c_read_reg (_dev,OMAP_I2C_IV_REG );/* Read clears */ - }else { -omap_i2c_write_reg (_dev,OMAP_I2C_STAT_REG , _dev->iestate); - -/* Flush posted write */ -omap_i2c_read_reg (_dev,OMAP_I2C_STAT_REG ); - } +/* Flush posted write */ +omap_i2c_read_reg (_dev,OMAP_I2C_STAT_REG );return 0 ; }
Signature · 1 line
1.8.0.rc0
PATCH 4/5 i2c: omap: don't re-enable IRQs after masking them
Quoted 17 lines · Felipe Balbi
OMAP I2C driver will re-enable IRQs right after
masking them during suspend.That's not what we want. We want to keep IRQs
masked until our resume method is called.Signed-off-by Felipe Balbi <[email protected]>--- drivers/i2c/busses/i2c-omap.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)drivers/i2c/busses/i2c-omap.c +0
-0@@ -1247,14 +1247,8 @@ static int omap_i2c_runtime_suspend(struct device *dev)
drivers/i2c/busses/i2c-omap.c static int omap_i2c_runtime_suspend(struct device *dev) · line 1257
Quoted 10 lines
drivers/i2c/busses/i2c-omap.c +0
-8omap_i2c_write_reg (_dev,OMAP_I2C_IE_REG ,0 ); -if (_dev->rev <OMAP_I2C_OMAP1_REV_2 ) { - iv =omap_i2c_read_reg (_dev,OMAP_I2C_IV_REG );/* Read clears */ - }else { -omap_i2c_write_reg (_dev,OMAP_I2C_STAT_REG , _dev->iestate); - -/* Flush posted write */ -omap_i2c_read_reg (_dev,OMAP_I2C_STAT_REG ); - }
Are you sure this re-enables the interrupt? It looks to me like it's meant to clear the interrupt.
drivers/i2c/busses/i2c-omap.c static int omap_i2c_runtime_suspend(struct device *dev) · line 1251
Quoted 2 lines
drivers/i2c/busses/i2c-omap.c +2
-0+/* Flush posted write */ +omap_i2c_read_reg (_dev,OMAP_I2C_STAT_REG );
Assuming the above is correct, should this be IE_REG?
drivers/i2c/busses/i2c-omap.c static int omap_i2c_runtime_suspend(struct device *dev) · line 1254
Quoted 2 lines
drivers/i2c/busses/i2c-omap.c +0
-0return 0 ; }
Kevin
PATCH 4/5 i2c: omap: don't re-enable IRQs after masking them
Quoted 33 lines · Kevin Hilman
Quoted 17 lines · Felipe Balbi
OMAP I2C driver will re-enable IRQs right after
masking them during suspend.That's not what we want. We want to keep IRQs
masked until our resume method is called.Signed-off-by Felipe Balbi <[email protected]>--- drivers/i2c/busses/i2c-omap.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)drivers/i2c/busses/i2c-omap.c +0
-0@@ -1247,14 +1247,8 @@ static int omap_i2c_runtime_suspend(struct device *dev)drivers/i2c/busses/i2c-omap.c static int omap_i2c_runtime_suspend(struct device *dev) · line 1257
Quoted 10 lines
drivers/i2c/busses/i2c-omap.c +0
-8omap_i2c_write_reg (_dev,OMAP_I2C_IE_REG ,0 ); -if (_dev->rev <OMAP_I2C_OMAP1_REV_2 ) { - iv =omap_i2c_read_reg (_dev,OMAP_I2C_IV_REG );/* Read clears */ - }else { -omap_i2c_write_reg (_dev,OMAP_I2C_STAT_REG , _dev->iestate); - -/* Flush posted write */ -omap_i2c_read_reg (_dev,OMAP_I2C_STAT_REG ); - }Are you sure this re-enables the interrupt? It looks to me like it's meant to clear the interrupt.
Quoted 4 lines
drivers/i2c/busses/i2c-omap.c static int omap_i2c_runtime_suspend(struct device *dev) · line 1251
Quoted 2 lines
drivers/i2c/busses/i2c-omap.c +2
-0+/* Flush posted write */ +omap_i2c_read_reg (_dev,OMAP_I2C_STAT_REG );Assuming the above is correct, should this be
IE_REG?
indeed. My eyes failed. This patch can be thrown away. My bad.
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121018/b4a51840/attachment.sig>
PATCH 5/5 i2c: omap: introduce suspend/resume methods
In reply to Felipe Balbiduring system-wide (static) suspend, we also
want to save our context and restore it when
waking up.
Let's introduce new suspend/resume methods so
that we can survive a suspend-to-ram transition.
--- drivers/i2c/busses/i2c-omap.c | 60 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 50 insertions(+), 10 deletions(-)
drivers/i2c/busses/i2c-omap.c +50 -10
@@ -1236,13 +1236,8 @@ static int __devexit omap_i2c_remove(struct platform_device *pdev) }#ifdef CONFIG_PM -#ifdef CONFIG_PM_RUNTIME -static int omap_i2c_runtime_suspend(struct device *dev) +static int omap_i2c_low_level_suspend (struct omap_i2c_dev *_dev) { - struct platform_device *pdev = to_platform_device(dev); - struct omap_i2c_dev *_dev = platform_get_drvdata(pdev); - u16 iv; - _dev->iestate =omap_i2c_read_reg (_dev,OMAP_I2C_IE_REG );omap_i2c_write_reg (_dev,OMAP_I2C_IE_REG ,0 );
@@ -1253,11 +1248,8 @@ static int omap_i2c_runtime_suspend(struct device *dev)return 0 ; } -static int omap_i2c_runtime_resume (struct device *dev) +static int omap_i2c_low_level_resume (struct omap_i2c_dev *_dev) { -struct platform_device *pdev =to_platform_device (dev); -struct omap_i2c_dev *_dev =platform_get_drvdata (pdev); -if (_dev->flags &OMAP_I2C_FLAG_RESET_REGS_POSTIDLE ) {omap_i2c_write_reg (_dev,OMAP_I2C_CON_REG ,0 );omap_i2c_write_reg (_dev,OMAP_I2C_PSC_REG , _dev->pscstate);
@@ -1278,9 +1270,57 @@ static int omap_i2c_runtime_resume(struct device *dev)return 0 ; } + +static int omap_i2c_suspend (struct device *dev) +{ +struct omap_i2c_dev *_dev =dev_get_drvdata (dev); + +if (pm_runtime_suspended (dev)) +return 0 ; + +return omap_i2c_low_level_suspend (_dev); +} + +static int omap_i2c_resume (struct device *dev) +{ +struct omap_i2c_dev *_dev =dev_get_drvdata (dev); +int r; + + r =omap_i2c_low_level_resume (_dev); +if (r) +return r; + +pm_runtime_disable (dev); +pm_runtime_set_active (dev); +pm_runtime_enable (dev); +pm_runtime_mark_last_busy (dev); +pm_runtime_put_autosuspend (dev); + +return 0 ; +} + +#ifdef CONFIG_PM_RUNTIME +static int omap_i2c_runtime_suspend (struct device *dev) +{ +struct omap_i2c_dev *_dev =dev_get_drvdata (dev); + +return omap_i2c_low_level_suspend (_dev); +} + +static int omap_i2c_runtime_resume (struct device *dev) +{ +struct omap_i2c_dev *_dev =dev_get_drvdata (dev); + +return omap_i2c_low_level_resume (_dev); +} +#else +#define omap_i2c_runtime_suspend NULL +#define omap_i2c_runtime_resume NULL#endif /* CONFIG_PM_RUNTIME */ static struct dev_pm_ops omap_i2c_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(omap_i2c_suspend, + omap_i2c_resume)SET_RUNTIME_PM_OPS (omap_i2c_runtime_suspend, omap_i2c_runtime_resume,NULL ) };
Signature · 1 line
1.8.0.rc0