Re: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume
On Thu, 24 Jun 2010, Paul Walmsley wrote: > Sorry, misread this also. Indeed, something like this is necessary in > your platform bus override code - so please ignore this part of the > comments. By the way, I owe you a more general apology, Kevin, that some of these comments have been erroneous and that you have had to spend time addressing the half-baked ones. They haven't measured up to my personal technical standards for comments... - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume
Paul Walmsley writes: > On Mon, 21 Jun 2010, Kevin Hilman wrote: > >> Paul Walmsley writes: >> >> > I guess the intent of your patch is to avoid having to patch in >> > omap_device_{idle,enable}() into a bunch of driver DPM suspend/resume >> > callbacks? >> >> Partially. Actually, I think many (most?) drivers can get rid of >> static suspend/resume paths all together and just use runtime PM. >> >> There are some cases though (MMC comes to mind, more on that below) >> where static suspend has some extra steps necessary and behaves >> differently than runtime PM. > > It's not just MMC, any driver that can be in the middle of doing something > during DPM ->suspend() will need to have DPM suspend code to stop what > it's doing and quiesce the device before returning from the suspend > callback. I don't think we do a very good job of that today in most drivers, but your point is valid. Probably best to "trick" the runtime PM core by doing a runtime PM put/get in the bus code as you suggest below to avoid this kind of potential conflict. > Either that, or ->suspend() needs to return an error and block > the system suspend process... > [...] >> > Right now the OMAP2+ codebase doesn't use any >> > shared interrupts for on-chip devices, as far as I can see. It >> > looks like you use ->suspend_noirq() to ensure your code runs after >> > the existing driver suspend functions. >> >> No. The primary reason for using _noirq() is that if any driver ever >> did use the _noirq() hooks (for any atomic activity, or late wakeup >> detection, etc.) the device will still be accessible. >> >> > Using ->suspend_noirq() for this purpose seems like a hack. >> > A better place for that code would be in a bus->pm->suspend() >> > callback, which will run after the device's dev_pm_ops->suspend() >> > callback. >> >> I originally put it in bus->pm->suspend, but moved it to >> bus->pm->suspend_noirq() since I didn't do a full audit so see >> if anything was using the _noirq hooks. >> >> If we want to have a requirement that no on-chip devices can use the >> _noirq hooks (or at least they can't touch hardware in those hooks) >> then I can move it back to bus->pm->suspend(). > > That sounds like the best argument for keeping these hooks in > _noirq() -- some driver writer may be likely to use suspend_noirq() > without understanding that they shouldn't... maybe we should add some code > that looks for this and warns. > > But thinking about it further, it also seems possible that a handful of > OMAP drivers might use shared IRQs at some point in the future. DSS comes > to mind -- that really is a shared IRQ. So, _noirq() is fine, then. OK. [...] >> > - It is not safe to call omap_device_{idle,enable}() from DPM >> > callbacks as the patch does, because they will race with runtime PM >> > calls to omap_device_{idle,enable}(). >> >> No, they cannot race. >> >> Runtime PM transitions are prevented by the system PM core during a >> system PM transition. The system suspend path does a pm_runtime_get() >> for each device being suspended, and then does a _put() upon resume. >> (see drivers/base/power/main.c, grep for pm_runtime_) > > Yes, you are correct in terms of my original statement. But the problem > -- and the race -- that I did a poor job of describing is still present. > > What if this pm_bus ->suspend_noirq() calls omap_device_idle() while other > code in the driver is still in the middle of interacting with the device? > Although that would certainly be a driver bug, I certainly wouldn't trust > all of our existing driver DPM suspend* functions to adequately wait for > in-progress operations to complete and block new operations from starting > before returning. Yes, I see the point now, and I agree that this is a problem. > We shouldn't idle (and potentially power off) a device unless we know its > driver is done with it. In an ideal world, this would always be taken > care of by driver ->suspend()/->suspend_noirq() functions. But we know > this isn't always the case -- perhaps it's not even the case for most of > the OMAP drivers. Yeah, I don't think we handle this very well currently in most drivers. > We can use the device's runtime PM state to figure out whether the driver > thinks it's done with the device. Unfortunately, the existing Linux DPM > suspend code makes it difficult to deal with this by calling > pm_runtime_get_noresume() before entering suspend and never calling > pm_runtime_put() until after resume -- no idea why. I gathered it was intended to minimize potential conflicts between system PM and runtime PM, but not sure. I may ask on linux-pm. > That strikes me as a bug. From a semantic perspective it is certainly > confusing: the PM runtime implementation will think the device is > still active after it's been suspended. I wouldn't want the full > Linux system suspend code to enter some low power state while some > driver
Re: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume
One correction here: On Wed, 23 Jun 2010, Paul Walmsley wrote: > On Mon, 21 Jun 2010, Kevin Hilman wrote: > > > Just to clarify. The functions I'm overriding here is the bus > > functions (bus->pm->[suspend|resume]_noirq(), not any driver functions > > OK, I see that now - this code was confusing in the patch's > platform_pm_suspend_noirq(): > > + if (drv->pm) { > + if (drv->pm->suspend_noirq) > + ret = drv->pm->suspend_noirq(dev); > + } > > This is already done by the DPM core in > drivers/base/power/main.c:device_suspend_noirq() and will result in > re-executing the driver's suspend_noirq function, which is potentially > quite bad. Same thing for platform_pm_resume_noirq() in the patch. Sorry, misread this also. Indeed, something like this is necessary in your platform bus override code - so please ignore this part of the comments. - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume
Hi Kevin, On Mon, 21 Jun 2010, Kevin Hilman wrote: > Paul Walmsley writes: > > > I guess the intent of your patch is to avoid having to patch in > > omap_device_{idle,enable}() into a bunch of driver DPM suspend/resume > > callbacks? > > Partially. Actually, I think many (most?) drivers can get rid of > static suspend/resume paths all together and just use runtime PM. > > There are some cases though (MMC comes to mind, more on that below) > where static suspend has some extra steps necessary and behaves > differently than runtime PM. It's not just MMC, any driver that can be in the middle of doing something during DPM ->suspend() will need to have DPM suspend code to stop what it's doing and quiesce the device before returning from the suspend callback. Either that, or ->suspend() needs to return an error and block the system suspend process... > > If so, some comments: > > - dev_pm_ops->suspend_noirq() is intended for use on devices with > > shared interrupts. > > Just to clarify. The functions I'm overriding here is the bus > functions (bus->pm->[suspend|resume]_noirq(), not any driver functions OK, I see that now - this code was confusing in the patch's platform_pm_suspend_noirq(): + if (drv->pm) { + if (drv->pm->suspend_noirq) + ret = drv->pm->suspend_noirq(dev); + } This is already done by the DPM core in drivers/base/power/main.c:device_suspend_noirq() and will result in re-executing the driver's suspend_noirq function, which is potentially quite bad. Same thing for platform_pm_resume_noirq() in the patch. > Indeed, shared IRQs were an intended usage, but I don't see that as a > requirement. Indeed, Documentation/power/devices.txt even seems to > suggest that the _noirq version is the place to turn the device "as > off as possible: > > "For simple drivers, suspend might quiesce the device using class code > and then turn its hardware as "off" as possible during suspend_noirq" Further down in that file, it says: "2. The suspend methods should quiesce the device to stop it from performing I/O. They also may save the device registers and put it into the appropriate low-power state, depending on the bus type the device is on, and they may enable wakeup events." ... and then: "The majority of subsystems and device drivers need not implement [the suspend_noirq] callback. However, bus types allowing devices to share interrupt vectors, like PCI, generally need it; otherwise a driver might encounter an error during the suspend phase by fielding a shared interrupt generated by some other device after its own device had been set to low power." > > Right now the OMAP2+ codebase doesn't use any > > shared interrupts for on-chip devices, as far as I can see. It > > looks like you use ->suspend_noirq() to ensure your code runs after > > the existing driver suspend functions. > > No. The primary reason for using _noirq() is that if any driver ever > did use the _noirq() hooks (for any atomic activity, or late wakeup > detection, etc.) the device will still be accessible. > > > Using ->suspend_noirq() for this purpose seems like a hack. > > A better place for that code would be in a bus->pm->suspend() > > callback, which will run after the device's dev_pm_ops->suspend() > > callback. > > I originally put it in bus->pm->suspend, but moved it to > bus->pm->suspend_noirq() since I didn't do a full audit so see > if anything was using the _noirq hooks. > > If we want to have a requirement that no on-chip devices can use the > _noirq hooks (or at least they can't touch hardware in those hooks) > then I can move it back to bus->pm->suspend(). That sounds like the best argument for keeping these hooks in _noirq() -- some driver writer may be likely to use suspend_noirq() without understanding that they shouldn't... maybe we should add some code that looks for this and warns. But thinking about it further, it also seems possible that a handful of OMAP drivers might use shared IRQs at some point in the future. DSS comes to mind -- that really is a shared IRQ. So, _noirq() is fine, then. > But personally, I would see that as an artificial requirement based on > a very restrictive definiton of the _noirq() methods. It's just the definition from the kernel documentation. > > - It is not safe to call omap_device_{idle,enable}() from DPM > > callbacks as the patch does, because they will race with runtime PM > > calls to omap_device_{idle,enable}(). > > No, they cannot race. > > Runtime PM transitions are prevented by the system PM core during a > system PM transition. The system suspend path does a pm_runtime_get() > for each device being suspended, and then does a _put() upon resume. > (see drivers/base/power/main.c, grep for pm_runtime_) Yes, you are correct in terms of my original statement. But the problem -- and the race -- that I did a poor job of d
Re: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume
Paul Walmsley writes: > On Mon, 21 Jun 2010, Kevin Hilman wrote: > >> Paul Walmsley writes: >> >> > As far as I can tell, it's not safe for upper-layer code to idle a device >> > like this. The driver itself needs to be aware of the device's idle >> > state. >> >> The driver is made aware using the standard dev_pm_ops callback for >> suspend and resume. > > I guess the intent of your patch is to avoid having to patch in > omap_device_{idle,enable}() into a bunch of driver DPM suspend/resume > callbacks? Partially. Actually, I think many (most?) drivers can get rid of static suspend/resume paths all together and just use runtime PM. There are some cases though (MMC comes to mind, more on that below) where static suspend has some extra steps necessary and behaves differently than runtime PM. For those cases, the goal is as you stated. Basically, to avoid having all the drivers having to call omap_device_idle(). > If so, some comments: > - dev_pm_ops->suspend_noirq() is intended for use on devices with > shared interrupts. Just to clarify. The functions I'm overriding here is the bus functions (bus->pm->[suspend|resume]_noirq(), not any driver functions Indeed, shared IRQs were an intended usage, but I don't see that as a requirement. Indeed, Documentation/power/devices.txt even seems to suggest that the _noirq version is the place to turn the device "as off as possible: "For simple drivers, suspend might quiesce the device using class code and then turn its hardware as "off" as possible during suspend_noirq" > Right now the OMAP2+ codebase doesn't use any > shared interrupts for on-chip devices, as far as I can see. It > looks like you use ->suspend_noirq() to ensure your code runs after > the existing driver suspend functions. No. The primary reason for using _noirq() is that if any driver ever did use the _noirq() hooks (for any atomic activity, or late wakeup detection, etc.) the device will still be accessible. > Using ->suspend_noirq() for this purpose seems like a hack. > A better place for that code would be in a bus->pm->suspend() > callback, which will run after the device's dev_pm_ops->suspend() > callback. I originally put it in bus->pm->suspend, but moved it to bus->pm->suspend_noirq() since I didn't do a full audit so see if anything was using the _noirq hooks. If we want to have a requirement that no on-chip devices can use the _noirq hooks (or at least they can't touch hardware in those hooks) then I can move it back to bus->pm->suspend(). But personally, I would see that as an artificial requirement based on a very restrictive definiton of the _noirq() methods. > - It is not safe to call omap_device_{idle,enable}() from DPM > callbacks as the patch does, because they will race with runtime PM > calls to omap_device_{idle,enable}(). No, they cannot race. Runtime PM transitions are prevented by the system PM core during a system PM transition. The system suspend path does a pm_runtime_get() for each device being suspended, and then does a _put() upon resume. (see drivers/base/power/main.c, grep for pm_runtime_) > (This is part of the reason > why it's important to be anal about enforcing the > omap_device_{enable,idle,shutdown}() state transition limitations - > so we get warned early when these types of conflicts appear.) > > omap_device*() calls should either be in runtime PM functions, or > DPM ->suspend/resume() callbacks, but not both. Why not both? I don't follow the reason for this restriction. We certainly did the equivalent clock enable/disable calls in both cases in the past. > Since we've decided > to focus on runtime PM power management as our primary driver power > management approach, and we expect drivers to aggressively idle > their devices, it seems best to keep the omap_device*() functions in > runtime PM code. I agree, mostly. As I mentioned above, I expect most drivers not to even have system PM methods implemented. They should implement everything as runtime PM. However, there will be some exceptions. The use case that brought this patch into existence was the MMC driver where the suspend hook had to do some extra "stuff" before suspending. Even if already runtime suspended, the MMC suspend hook has to re-enable the device do "stuff" and then re-idle the device. Initially, I tried to just use the same runtime PM calls in the suspend hook, but that doesn't work since runtime PM transitions are prevented during system PM. So, I was forced to call omap_device_idle() in the suspend path, which led to the decision to move that into common code. > At that point, the common DPM suspend/idle callbacks that you > propose can simply block until runtime PM indicates that the device > is idle. > > For example, you could use a per-omap_device mutex. > A sketch of a sample implementation follows this message. That is an option, but since there is no potential raci
Re: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume
On Mon, 21 Jun 2010, Kevin Hilman wrote: > Paul Walmsley writes: > > > As far as I can tell, it's not safe for upper-layer code to idle a device > > like this. The driver itself needs to be aware of the device's idle > > state. > > The driver is made aware using the standard dev_pm_ops callback for > suspend and resume. I guess the intent of your patch is to avoid having to patch in omap_device_{idle,enable}() into a bunch of driver DPM suspend/resume callbacks? If so, some comments: - dev_pm_ops->suspend_noirq() is intended for use on devices with shared interrupts. Right now the OMAP2+ codebase doesn't use any shared interrupts for on-chip devices, as far as I can see. It looks like you use ->suspend_noirq() to ensure your code runs after the existing driver suspend functions. Using ->suspend_noirq() for this purpose seems like a hack. A better place for that code would be in a bus->pm->suspend() callback, which will run after the device's dev_pm_ops->suspend() callback. - It is not safe to call omap_device_{idle,enable}() from DPM callbacks as the patch does, because they will race with runtime PM calls to omap_device_{idle,enable}(). (This is part of the reason why it's important to be anal about enforcing the omap_device_{enable,idle,shutdown}() state transition limitations - so we get warned early when these types of conflicts appear.) omap_device*() calls should either be in runtime PM functions, or DPM ->suspend/resume() callbacks, but not both. Since we've decided to focus on runtime PM power management as our primary driver power management approach, and we expect drivers to aggressively idle their devices, it seems best to keep the omap_device*() functions in runtime PM code. At that point, the common DPM suspend/idle callbacks that you propose can simply block until runtime PM indicates that the device is idle. For example, you could use a per-omap_device mutex. A sketch of a sample implementation follows this message. - Paul In omap_device.c, add int omap_device_suspend_mpu_irqs(struct omap_device *od) { /* * For each hwmod, for each MPU IRQ, save current state, * disable_irq(irq) */ } int omap_device_resume_mpu_irqs(struct omap_device *od) { /* * For each hwmod, for each MPU IRQ, enable_irq(irq) if * it was previously */ } In your bus->pm->suspend()/resume() functions: int omap_bus_suspend(struct device *dev, pm_message_t state) { dev_dbg("waiting for device %s to go idle\n", dev_name(dev)); mutex_lock(&od->active_mutex); /* Device guaranteed to be idle at this point */ /* * do anything else necessary to ensure that driver code is not * entered after the unlock() */ omap_device_suspend_mpu_irqs(od); mutex_unlock(&od->active_mutex); return 0; } int omap_bus_resume(struct device *dev, pm_message_t state) { mutex_lock(&od->active_mutex); /* Device guaranteed to be idle at this point */ /* * do anything else necessary to ensure that driver code can * be entered after the unlock() */ omap_device_resume_mpu_irqs(od); mutex_unlock(&od->active_mutex); return 0; } Then in the omap_device code, add a mutex active_mutex in struct omap_device, init the mutex in omap_device_build_ss(), then: int omap_device_enable(struct platform_device *pdev) { struct omap_device *od; od = _find_by_pdev(pdev); ... WARN(!mutex_trylock(&od->active_mutex), "State transition violation - should never happen\n"); mutex_lock(&od->active_mutex); ... } int omap_device_idle(struct platform_device *pdev) { struct omap_device *od; od = _find_by_pdev(pdev); ... mutex_unlock(&od->active_mutex); ... } int omap_device_shutdown(struct platform_device *pdev) { struct omap_device *od; od = _find_by_pdev(pdev); ... mutex_unlock(&od->active_mutex); ... } The driver needs the usual: - add a DPM ->suspend() function that tries to stop whatever the device is doing if it's in the middle of something that can be stopped; - ensure all paths that start new I/O check dev->power.status, and return an error if it is DPM_SUSPENDING - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume
Paul Walmsley writes: > On Tue, 1 Jun 2010, Kevin Hilman wrote: > >> "Nayak, Rajendra" writes: >> >> [...] >> >> >> diff --git a/arch/arm/mach-omap2/pm_bus.c >> >> b/arch/arm/mach-omap2/pm_bus.c >> >> index 69acaa5..3787da8 100644 >> >> --- a/arch/arm/mach-omap2/pm_bus.c >> >> +++ b/arch/arm/mach-omap2/pm_bus.c >> >> @@ -70,3 +70,64 @@ int platform_pm_runtime_idle(struct device *dev) >> >> }; >> >> #endif /* CONFIG_PM_RUNTIME */ >> >> >> >> +#ifdef CONFIG_SUSPEND >> >> +int platform_pm_suspend_noirq(struct device *dev) >> >> +{ >> >> + struct device_driver *drv = dev->driver; >> >> + struct platform_device *pdev = to_platform_device(dev); >> >> + struct omap_device *odev = to_omap_device(pdev); >> >> + int ret = 0; >> >> + >> >> + if (!drv) >> >> + return 0; >> >> + >> >> + if (drv->pm) { >> >> + if (drv->pm->suspend_noirq) >> >> + ret = drv->pm->suspend_noirq(dev); >> >> + } >> >> + >> >> + if (omap_device_is_valid(odev)) { >> >> + if (odev->flags & OMAP_DEVICE_NO_BUS_SUSPEND) { >> >> + dev_dbg(dev, "no automatic bus-level >> >> system resume.\n"); >> >> + return 0; >> >> + } >> >> + >> >> + dev_dbg(dev, "%s\n", __func__); >> >> + omap_device_idle(pdev); >> > >> > Is it expected that a device is always in enabled state at this point? >> > If the device is already in idle a call to omap_device_idle unconditionally >> > throws up warnings from the omap_device api. >> >> Hmm, good point. The device may already be idled (via runtime PM, or >> maybe because it was never enabled.) >> >> There are two options: >> >> 1. fixup the warnings in the omap_device_idle() to allow multiple >>calls to _idle() >> >> 2. Add an omap_device_is_idle() check before calling _idle() >> >> I much prefer (1). >> >> Paul? > > As far as I can tell, it's not safe for upper-layer code to idle a device > like this. The driver itself needs to be aware of the device's idle > state. The driver is made aware using the standard dev_pm_ops callback for suspend and resume. Note that this is not just any upper-layer code that is blindly idling a device. This is only happening at the system-suspend time, and in particular this is happening using the _noirq() versions of the dev_pm_ops hooks. > For example, if the driver has exported some symbols, and some > other code is calling one of those functions, it's the driver code that > needs to be aware of its own idle-state and to return some kind of error > if the device is quiesced. I don't think there's an easy way for > upper-layer code to do that. Drivers already have to handle this today. If they have been suspended and their functions have been called, they have to return errors. This patch doesn't change that. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume
On Tue, 1 Jun 2010, Kevin Hilman wrote: > "Nayak, Rajendra" writes: > > [...] > > >> diff --git a/arch/arm/mach-omap2/pm_bus.c > >> b/arch/arm/mach-omap2/pm_bus.c > >> index 69acaa5..3787da8 100644 > >> --- a/arch/arm/mach-omap2/pm_bus.c > >> +++ b/arch/arm/mach-omap2/pm_bus.c > >> @@ -70,3 +70,64 @@ int platform_pm_runtime_idle(struct device *dev) > >> }; > >> #endif /* CONFIG_PM_RUNTIME */ > >> > >> +#ifdef CONFIG_SUSPEND > >> +int platform_pm_suspend_noirq(struct device *dev) > >> +{ > >> + struct device_driver *drv = dev->driver; > >> + struct platform_device *pdev = to_platform_device(dev); > >> + struct omap_device *odev = to_omap_device(pdev); > >> + int ret = 0; > >> + > >> + if (!drv) > >> + return 0; > >> + > >> + if (drv->pm) { > >> + if (drv->pm->suspend_noirq) > >> + ret = drv->pm->suspend_noirq(dev); > >> + } > >> + > >> + if (omap_device_is_valid(odev)) { > >> + if (odev->flags & OMAP_DEVICE_NO_BUS_SUSPEND) { > >> + dev_dbg(dev, "no automatic bus-level > >> system resume.\n"); > >> + return 0; > >> + } > >> + > >> + dev_dbg(dev, "%s\n", __func__); > >> + omap_device_idle(pdev); > > > > Is it expected that a device is always in enabled state at this point? > > If the device is already in idle a call to omap_device_idle unconditionally > > throws up warnings from the omap_device api. > > Hmm, good point. The device may already be idled (via runtime PM, or > maybe because it was never enabled.) > > There are two options: > > 1. fixup the warnings in the omap_device_idle() to allow multiple >calls to _idle() > > 2. Add an omap_device_is_idle() check before calling _idle() > > I much prefer (1). > > Paul? As far as I can tell, it's not safe for upper-layer code to idle a device like this. The driver itself needs to be aware of the device's idle state. For example, if the driver has exported some symbols, and some other code is calling one of those functions, it's the driver code that needs to be aware of its own idle-state and to return some kind of error if the device is quiesced. I don't think there's an easy way for upper-layer code to do that. The runtime PM-related code, however, should be safe, since it's initiated by the driver itself. - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume
"Nayak, Rajendra" writes: [...] >> diff --git a/arch/arm/mach-omap2/pm_bus.c >> b/arch/arm/mach-omap2/pm_bus.c >> index 69acaa5..3787da8 100644 >> --- a/arch/arm/mach-omap2/pm_bus.c >> +++ b/arch/arm/mach-omap2/pm_bus.c >> @@ -70,3 +70,64 @@ int platform_pm_runtime_idle(struct device *dev) >> }; >> #endif /* CONFIG_PM_RUNTIME */ >> >> +#ifdef CONFIG_SUSPEND >> +int platform_pm_suspend_noirq(struct device *dev) >> +{ >> +struct device_driver *drv = dev->driver; >> +struct platform_device *pdev = to_platform_device(dev); >> +struct omap_device *odev = to_omap_device(pdev); >> +int ret = 0; >> + >> +if (!drv) >> +return 0; >> + >> +if (drv->pm) { >> +if (drv->pm->suspend_noirq) >> +ret = drv->pm->suspend_noirq(dev); >> +} >> + >> +if (omap_device_is_valid(odev)) { >> +if (odev->flags & OMAP_DEVICE_NO_BUS_SUSPEND) { >> +dev_dbg(dev, "no automatic bus-level >> system resume.\n"); >> +return 0; >> +} >> + >> +dev_dbg(dev, "%s\n", __func__); >> +omap_device_idle(pdev); > > Is it expected that a device is always in enabled state at this point? > If the device is already in idle a call to omap_device_idle unconditionally > throws up warnings from the omap_device api. Hmm, good point. The device may already be idled (via runtime PM, or maybe because it was never enabled.) There are two options: 1. fixup the warnings in the omap_device_idle() to allow multiple calls to _idle() 2. Add an omap_device_is_idle() check before calling _idle() I much prefer (1). Paul? Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume
> -Original Message- > From: linux-omap-ow...@vger.kernel.org > [mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of Kevin Hilman > Sent: Friday, May 28, 2010 2:43 AM > To: linux-omap@vger.kernel.org > Subject: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume > > Hook into the platform bus methods for suspend and resume and > use the omap_device API to automatically idle and enable the > device on suspend and resume. > > This allows device drivers to get rid of direct management of their > clocks in their suspend/resume paths, and let omap_device do it for > them. > > We currently use the _noirq (late suspend, early resume) versions of > the suspend/resume methods to ensure that the device is not disabled > too early for any drivers also using the _noirq methods. > > NOTE: only works for devices with omap_hwmod support. > > Signed-off-by: Kevin Hilman > --- > arch/arm/mach-omap2/pm_bus.c | 61 > ++ > 1 files changed, 61 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/mach-omap2/pm_bus.c > b/arch/arm/mach-omap2/pm_bus.c > index 69acaa5..3787da8 100644 > --- a/arch/arm/mach-omap2/pm_bus.c > +++ b/arch/arm/mach-omap2/pm_bus.c > @@ -70,3 +70,64 @@ int platform_pm_runtime_idle(struct device *dev) > }; > #endif /* CONFIG_PM_RUNTIME */ > > +#ifdef CONFIG_SUSPEND > +int platform_pm_suspend_noirq(struct device *dev) > +{ > + struct device_driver *drv = dev->driver; > + struct platform_device *pdev = to_platform_device(dev); > + struct omap_device *odev = to_omap_device(pdev); > + int ret = 0; > + > + if (!drv) > + return 0; > + > + if (drv->pm) { > + if (drv->pm->suspend_noirq) > + ret = drv->pm->suspend_noirq(dev); > + } > + > + if (omap_device_is_valid(odev)) { > + if (odev->flags & OMAP_DEVICE_NO_BUS_SUSPEND) { > + dev_dbg(dev, "no automatic bus-level > system resume.\n"); > + return 0; > + } > + > + dev_dbg(dev, "%s\n", __func__); > + omap_device_idle(pdev); Is it expected that a device is always in enabled state at this point? If the device is already in idle a call to omap_device_idle unconditionally throws up warnings from the omap_device api. > + } else { > + dev_dbg(dev, "not an omap_device\n"); > + } > + > + return ret; > +} > + > +int platform_pm_resume_noirq(struct device *dev) > +{ > + struct device_driver *drv = dev->driver; > + struct platform_device *pdev = to_platform_device(dev); > + struct omap_device *odev = to_omap_device(pdev); > + int ret = 0; > + > + if (omap_device_is_valid(odev)) { > + if (odev->flags & OMAP_DEVICE_NO_BUS_SUSPEND) { > + dev_dbg(dev, "no automatic bus-level > system resume.\n"); > + return 0; > + } > + > + dev_dbg(dev, "%s\n", __func__); > + omap_device_enable(pdev); > + } else { > + dev_dbg(dev, "not an omap_device\n"); > + } > + > + if (!drv) > + return 0; > + > + if (drv->pm) { > + if (drv->pm->resume_noirq) > + ret = drv->pm->resume_noirq(dev); > + } > + > + return ret; > +} > +#endif /* CONFIG_SUSPEND */ > -- > 1.7.0.2 > > -- > To unsubscribe from this list: send the line "unsubscribe > linux-omap" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume
Hook into the platform bus methods for suspend and resume and use the omap_device API to automatically idle and enable the device on suspend and resume. This allows device drivers to get rid of direct management of their clocks in their suspend/resume paths, and let omap_device do it for them. We currently use the _noirq (late suspend, early resume) versions of the suspend/resume methods to ensure that the device is not disabled too early for any drivers also using the _noirq methods. NOTE: only works for devices with omap_hwmod support. Signed-off-by: Kevin Hilman --- arch/arm/mach-omap2/pm_bus.c | 61 ++ 1 files changed, 61 insertions(+), 0 deletions(-) diff --git a/arch/arm/mach-omap2/pm_bus.c b/arch/arm/mach-omap2/pm_bus.c index 69acaa5..3787da8 100644 --- a/arch/arm/mach-omap2/pm_bus.c +++ b/arch/arm/mach-omap2/pm_bus.c @@ -70,3 +70,64 @@ int platform_pm_runtime_idle(struct device *dev) }; #endif /* CONFIG_PM_RUNTIME */ +#ifdef CONFIG_SUSPEND +int platform_pm_suspend_noirq(struct device *dev) +{ + struct device_driver *drv = dev->driver; + struct platform_device *pdev = to_platform_device(dev); + struct omap_device *odev = to_omap_device(pdev); + int ret = 0; + + if (!drv) + return 0; + + if (drv->pm) { + if (drv->pm->suspend_noirq) + ret = drv->pm->suspend_noirq(dev); + } + + if (omap_device_is_valid(odev)) { + if (odev->flags & OMAP_DEVICE_NO_BUS_SUSPEND) { + dev_dbg(dev, "no automatic bus-level system resume.\n"); + return 0; + } + + dev_dbg(dev, "%s\n", __func__); + omap_device_idle(pdev); + } else { + dev_dbg(dev, "not an omap_device\n"); + } + + return ret; +} + +int platform_pm_resume_noirq(struct device *dev) +{ + struct device_driver *drv = dev->driver; + struct platform_device *pdev = to_platform_device(dev); + struct omap_device *odev = to_omap_device(pdev); + int ret = 0; + + if (omap_device_is_valid(odev)) { + if (odev->flags & OMAP_DEVICE_NO_BUS_SUSPEND) { + dev_dbg(dev, "no automatic bus-level system resume.\n"); + return 0; + } + + dev_dbg(dev, "%s\n", __func__); + omap_device_enable(pdev); + } else { + dev_dbg(dev, "not an omap_device\n"); + } + + if (!drv) + return 0; + + if (drv->pm) { + if (drv->pm->resume_noirq) + ret = drv->pm->resume_noirq(dev); + } + + return ret; +} +#endif /* CONFIG_SUSPEND */ -- 1.7.0.2 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html