| Message ID | 1503499329-28834-1-git-send-email-ulf.hansson@linaro.org |
|---|---|
| Headers | show |
| Series | PM / ACPI / i2c: Deploy runtime PM centric path for system sleep | expand |
On Thursday, August 24, 2017 10:19:43 AM CEST Ulf Hansson wrote: > On 24 August 2017 at 01:39, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Aug 23, 2017 at 4:42 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > >> In some cases a driver for an ACPI device needs to be able to prevent the > >> ACPI PM domain from using the direct_complete path during system sleep. > >> > >> One typical case is when the driver for the device needs its device to stay > >> runtime enabled, during the __device_suspend phase. This isn't the case > >> when the direct_complete path is being executed by the PM core, as it then > >> disables runtime PM for the device in __device_suspend(). Any following > >> attempts to runtime resume the device after that point, just fails. > > > > OK, that is a problem. > > > >> A workaround to this problem is to let the driver runtime resume its device > >> from its ->prepare() callback, as that would prevent the direct_complete > >> path from being executed. However, that may often be a waste, especially if > >> it turned out that no one really needed the device. > >> > >> For this reason, invent acpi_dev_disable|enable_direct_complete(), to allow > >> drivers to inform the ACPI PM domain to change its default behaviour during > >> system sleep, and thus control whether it may use the direct_complete path > >> or not. > > > > But I'm not sure this is the right place to address it as it very well > > may affect a PCI device too. > > > > Also, this is about the device and not about its ACPI companion > > object, so putting the flag in there is somewhat unclean, so to speak. > > > > It looks like we need a flag in dev_pm_info saying something along the > > lines of "my system suspend callback can deal with runtime PM" that > > will cause the direct_complete update to occur in > > __device_suspend_late() instead of __device_suspend(). > > I realize that in the end this turns out to be a comparison between > the runtime PM centric path and the direct_complete path while > implementing system sleep. In patch 9, there is some more explanation > around this, however if you like I can elaborate even more about > this!? > > Regarding making changes to the PM core and adding more flags to the > dev_pm_info etc, I am not sure we really want that. Isn't it already > complex enough? Maybe it is. > My point is, that I am trying to improve the behavior of the ACPI PM > domain by enabling the runtime PM centric path for it, and even if > there is something similar that could be done for PCI, I don't think > we should need involvement by the PM core. Well, this generally simply doesn't work. The whole "runtime PM centric approach" idea is generally fine by me, but the fact today is that there are drivers not ready for it. Which is why there is the direct_complete thing (it may be regarded as a sort-of workaround for the unreadiness of drivers if you will). Now, buy adding the no_direct_complete flag just to the ACPI PM domain you basically overlook the fact that this potentially affects the parents of the devices in question by preventing direct_complete from being set for them. And those parents may not be in the ACPI PM domain in principle, so the problem needs to be addressed in the core. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, August 24, 2017 11:15:26 AM CEST Ulf Hansson wrote: > [...] > > >> > > >> > It actually should work with the ACPI PM domain code as is except that it > >> > will cause the device to be runtime resumed every time during suspend. > >> > > >> > But acpi_subsys_suspend() can be changed to avoid resuming the device if > >> > power.force_suspend is set. > >> > >> Or better yet, if power.direct_complete is not set, because that means the > >> device needs to be resumed anyway. > >> > >> And if power.direct_complete is set at that point, power.force_suspend has to > >> be set too. > > I believe I understand your goal here. You want to consider if it is > possible to extend the behavior of the direct_complete path, instead > of enabling the runtime PM centric path for the ACPI PM domain, to > accomplish the same optimizations. This isn't really "instead of", but rather "in addition to". > I think the answer to that, is that it simply can't. See more > information about why further down below. > > > > > Overall, like below, and of course drivers that ser power.force_suspend need to > > be able to deal with devices that have been runtime resumed during > > __device_suspend(). > > The a concern I see with this approach, is that is going to be too > complex to understand. That is a valid concern, but then your approach with using an extra flag just for the ACPI PM domain isn't particularly straightforward either IMO. > We have already seen how the i2c-designware-driver was screwed up when > it was trying to take advantage of the optimizations provided by the > current direct_complete path. I think we should be careful when > considering making it even more complex. OK, but this applies to your series as well, as it is adding a new flag too, just not in the core. :-) > In the runtime PM centric path, driver authors can easily deploy > system sleep support. In some cases it may even consists of only two > lines of code. As can be shown in patch9 for the i2c driver. That still requires the new flag to be set if the ACPI PM is in use AFAICS, which then will prevent the entire hierarchy above the device from having direct_complete set. > Just to be clear, that doesn't mean that I think the direct_complete > path isn't useful. Especially it is a good match for the ACPI PM > domain, as it can easily affect all its devices, without having to > care much about the behavior of the drivers. OK [cut] > > The above change would offer an improvement, however there are more > benefits from the runtime PM centric path that it doesn't cover. The idea, though, is not to make it instead of, but in addition to the runtime PM centric path. So a driver using the runtime PM centric (or rather runtime PM aware) callbacks can set power.force_suspend and benefit from having direct_complete set conditional on whether or not the device is runtime resumed before the late suspend. If device_complete remains set in __device_suspend_late(), no system suspend/resume callbacks will be invoked from that point on. If it isn't set, pm_runtime_force_suspend() should work just fine and then pm_runtime_force_resume() should do the right thing on the way back. All boils down to (a) setting a flag and (b) using the right callbacks for system suspend which is the case with your patches either. > The below is pasted from the changelog for patch9 (I will make sure to > fold in this in the changelog for $subject patch next version). > > In case the device is/gets runtime resumed before the device_suspend() > phase is entered, the PM core doesn't run the direct_complete path for > the device during system sleep. During system resume, this lead to > that the device will always be brought back to full power when the > i2c-dw-plat driver's ->resume() callback is invoked. This may not be > necessary, thus increasing the resume time for the device and wasting > power. Well, I guess that depends on what is there in its resume callbacks. > In the runtime PM centric path, the pm_runtime_force_resume() helper > takes better care, as it resumes the device only in case it's really > needed. Normally this means it can be postponed to be dealt with via > regular calls to runtime PM (pm_runtime_get*()) instead. In other > words, the device remains in its low power state until someone request > a new i2c transfer, whenever that may be. > > As far as I can think of, the direct_complete path can't be adjusted > to support this. Or can it? It surely can, it's just software. :-) The question is how complicated that is going to be in the end, however. So the patch I sent didn't address the dependency issue, but it allows the core to deal with the issue which is a core issue, not an ACPI PM domain issue, because the core disables runtime PM for devices with direct_complete set, not the ACPI PM domain or PCI etc. [BTW, it is not entirely clear to me why it ever is necessary to runtime resume a device with direct_complete set after __device_suspend(), because it can only have direct_complete set at that point if all of the hierarchy below it has this flag set too and so runtime PM has to be disabled for all of those devices as well.] It can be made slightly better by adding a piece to avoid clearing the parent's direct_complete if it had the new flag set, so below is a new version (with the new flag renamed to safe_suspend which seemed better to me). Of course, having safe_suspend set will still cause the parent's direct_complete to be cleared unless the parent has safe_suspend set too, but that's better than not allowing the parent to use direct_complete at all. --- drivers/acpi/device_pm.c | 8 ++++++++ drivers/base/power/main.c | 15 ++++++++++++--- include/linux/pm.h | 1 + 3 files changed, 21 insertions(+), 3 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.htmlIndex: linux-pm/drivers/base/power/main.c =================================================================== --- linux-pm.orig/drivers/base/power/main.c +++ linux-pm/drivers/base/power/main.c @@ -1271,9 +1271,16 @@ static int __device_suspend_late(struct goto Complete; } - if (dev->power.syscore || dev->power.direct_complete) + if (dev->power.syscore) goto Complete; + if (dev->power.direct_complete) { + if (pm_runtime_status_suspended(dev)) + goto Complete; + + dev->power.direct_complete = false; + } + if (dev->pm_domain) { info = "late power domain "; callback = pm_late_early_op(&dev->pm_domain->ops, state); @@ -1482,7 +1489,7 @@ static int __device_suspend(struct devic if (dev->power.syscore) goto Complete; - if (dev->power.direct_complete) { + if (dev->power.direct_complete && !dev->power.safe_suspend) { if (pm_runtime_status_suspended(dev)) { pm_runtime_disable(dev); if (pm_runtime_status_suspended(dev)) @@ -1549,7 +1556,9 @@ static int __device_suspend(struct devic if (parent) { spin_lock_irq(&parent->power.lock); - dev->parent->power.direct_complete = false; + if (!dev->parent->power.safe_suspend) + dev->parent->power.direct_complete = false; + if (dev->power.wakeup_path && !dev->parent->power.ignore_children) dev->parent->power.wakeup_path = true; Index: linux-pm/include/linux/pm.h =================================================================== --- linux-pm.orig/include/linux/pm.h +++ linux-pm/include/linux/pm.h @@ -554,6 +554,7 @@ struct dev_pm_info { pm_message_t power_state; unsigned int can_wakeup:1; unsigned int async_suspend:1; + unsigned int safe_suspend:1; bool in_dpm_list:1; /* Owned by the PM core */ bool is_prepared:1; /* Owned by the PM core */ bool is_suspended:1; /* Ditto */ Index: linux-pm/drivers/acpi/device_pm.c =================================================================== --- linux-pm.orig/drivers/acpi/device_pm.c +++ linux-pm/drivers/acpi/device_pm.c @@ -1025,9 +1025,17 @@ EXPORT_SYMBOL_GPL(acpi_subsys_prepare); * * Follow PCI and resume devices suspended at run time before running their * system suspend callbacks. + * + * If the power.direct_complete flag is set for the device, though, skip all + * that, because the device doesn't need to be resumed then and if it is + * resumed via runtime PM, the core will notice that and will carry out the + * late suspend for it. */ int acpi_subsys_suspend(struct device *dev) { + if (dev->power.direct_complete) + return 0; + pm_runtime_resume(dev); return pm_generic_suspend(dev); }
On Friday, August 25, 2017 11:28:25 AM CEST Ulf Hansson wrote: > [...] > > >> > >> The above change would offer an improvement, however there are more > >> benefits from the runtime PM centric path that it doesn't cover. > > > > The idea, though, is not to make it instead of, but in addition to the > > runtime PM centric path. > > > > So a driver using the runtime PM centric (or rather runtime PM aware) > > callbacks can set power.force_suspend and benefit from having direct_complete > > set conditional on whether or not the device is runtime resumed before the > > late suspend. > > > > If device_complete remains set in __device_suspend_late(), no system > > suspend/resume callbacks will be invoked from that point on. If it isn't > > set, pm_runtime_force_suspend() should work just fine and then > > pm_runtime_force_resume() should do the right thing on the way back. > > > > All boils down to (a) setting a flag and (b) using the right callbacks > > for system suspend which is the case with your patches either. > > You make it sound simple. :-) > > I try to get inspired of your ideas and see what I can come up with. > However, I really want us to avoid making it more difficult to > understand the behavior of the core, so I probably look start changing > the ACPI PM domain first, then trying to adopt the behavior of the > core on top of those changes. Does that sound reasonable to you? Well, I don't think we need to change the ACPI PM domain at all. :-) However, let me contiune in a reply to https://marc.info/?l=linux-arm-kernel&m=150361200030186&w=2 for more complete context. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, August 25, 2017 3:42:35 PM CEST Rafael J. Wysocki wrote: > On Thursday, August 24, 2017 11:50:40 PM CEST Rafael J. Wysocki wrote: > > On Thursday, August 24, 2017 6:35:49 PM CEST Rafael J. Wysocki wrote: > > > On Thursday, August 24, 2017 11:15:26 AM CEST Ulf Hansson wrote: > > > > [cut] > > > > > [BTW, it is not entirely clear to me why it ever is necessary to runtime resume > > > a device with direct_complete set after __device_suspend(), because it can only > > > have direct_complete set at that point if all of the hierarchy below it has > > > this flag set too and so runtime PM has to be disabled for all of those > > > devices as well.] > > > > Which makes me realize that we should take a step back and look at what > > problems there are. > > > > First, there are devices (I know about two examples so far and both are PCI) > > that may need to be runtime resumed during system suspend for reasons other > > than the ones checked by the ACPI PM domain (or the PCI bus type). There needs > > to be a way to indicate that from the driver side. > > > > However, it still may be valuable to check the power-related conditions for > > leaving the device in runtime suspend over system suspend/resume in case > > it actually doesn't need to be runtime resumed during system suspend after > > all. That's what the majority of my patch was about. > > > > The second problem is that the ACPI PM domain (and the PCI bus type) > > runtime resumes all devices unconditionally in its ->suspend callback, > > even though that may not be necessary for some devices. Therefore there > > needs to be a way to indicate that too. That still would be good to > > have *regardless* of the direct_complete mechanism, because the direct_complete > > flag may not be set very often due to dependencies and then the > > resume-during-suspend will take place unnecessarily. > > > > Accordingly, it looks like we need a "no need to resume me" flag in the first > > place. That would indicate to interested pieces of code that, from the > > driver perspective, the device doesn't need to be runtime resumed before > > invoking its system suspend callbacks. This should be clear enough to everyone > > IMO. > > > > [Note that if that flag is set for all devices, we may drop it along with > > direct_complete, but before that happens both are needed.] > > I think we are in agreement that direct_complete will not be necessary any > more when all drivers/bus types/PM domains and so on can do the "safe > suspend", but we're not there yet. :-) > > > To address the first issue I would add something like the flag in the patches > > I sent (but without the ACPI PM domain part which should be covered by the > > "no need to resume me" flag above), because that allows the device's ->suspend > > callback to run in principle and the driver may use that callback even to > > runtime resume the device if that's what it wants to do. So something like > > "run my ->suspend callback even though I might stay in runtime suspend". > > > > I would probably add driver_flags to dev_pm_info for that to set at the probe > > time (and I would make the core clear that on driver removal). > > > > The complexity concern is there, but honestly I don't see a better way at > > this point. > > So below is a prototype patch. It still is missing a documentation update, but > other than that it should be complete unless I missed something. > > The way it works is that the SAFE_SUSPEND flag is not looked at by the core > at all. The ACPI PM domain looks at it and the PCI bus type can be modified > to take it into account in the future. That is what causes the "runtime resume > during system suspend" to be skipped. > > In turn, the ALWAYS_SUSPEND flag is only looked at by the core and it causes > the decision on whether or not to use direct_complete to be deferred to the > __device_suspend_late() time. If you set it for a PCI device, the effect is > equivalent to "no direct_complete". If you set it for a device in the ACPI > PM domain, that depends on whether or not SAFE_SUSPEND is set. If it isn't > set, the effect is equivalent to "no direct_complete" too, but if it is set, > the core may still try to use direct_complete for the device, but it will > make the decision on it in __device_suspend_late() and then it will not invoke > the ->suspend_late callback for the device if it is still runtime suspended. > [Note that you cannot runtime resume and runtime suspend again a device during > system suspend, so if it is runtime suspended in __device_suspend_late(), it > has been runtime suspend all the way since device_prepare().] > > So say you point the ->suspend_late and ->resume_early callbacks of > the designware i2c driver to pm_runtime_force_suspend() and > pm_runtime_force_resume(), respectively, and set both the SAFE_SUSPEND > and ALWAYS_SUSPEND flags for the device. > > If system suspend is started and the device is not runtime suspended, > direct_complete is not set for it and everything works as usual, so say > the device is runtime suspended in device_prepare(). Then, the ACPI PM > domain checks the other conditions for leaving it in runtime suspend and > returns either 0 or a positive number from acpi_subsys_prepare(). > > If 0 is returned, direct_complete is not set by the core and > acpi_subsys_suspend() is called. It checks the SAFE_SUSPEND flag and sees > that the device need not be runtime resumed, so it invokes the driver's > ->suspend callback (which is not present, so it doesn't do anything). > Next, in __device_suspend_late(), acpi_subsys_suspend_late() is invoked > and it calls pm_runtime_force_suspend(), which executes the driver's > ->runtime_suspend() callback, and then (if successful) calls > acpi_dev_suspend_late() to put the device into a low-power state. The > resume path is a reverse of the above in this case. So far, so good. Well, not really, because if the device remains runtime suspended, ->runtime_suspend() will not be called by pm_runtime_force_suspend() and acpi_dev_suspend_late() should not be called then. So more changes in the ACPI PM domain are needed after all. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, August 28, 2017 10:31:44 AM CEST Ulf Hansson wrote: > On 28 August 2017 at 03:30, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Friday, August 25, 2017 3:42:35 PM CEST Rafael J. Wysocki wrote: > >> On Thursday, August 24, 2017 11:50:40 PM CEST Rafael J. Wysocki wrote: > >> > On Thursday, August 24, 2017 6:35:49 PM CEST Rafael J. Wysocki wrote: > >> > > On Thursday, August 24, 2017 11:15:26 AM CEST Ulf Hansson wrote: > >> > > >> > [cut] > >> > > >> > > [BTW, it is not entirely clear to me why it ever is necessary to runtime resume > >> > > a device with direct_complete set after __device_suspend(), because it can only > >> > > have direct_complete set at that point if all of the hierarchy below it has > >> > > this flag set too and so runtime PM has to be disabled for all of those > >> > > devices as well.] > >> > > >> > Which makes me realize that we should take a step back and look at what > >> > problems there are. > >> > > >> > First, there are devices (I know about two examples so far and both are PCI) > >> > that may need to be runtime resumed during system suspend for reasons other > >> > than the ones checked by the ACPI PM domain (or the PCI bus type). There needs > >> > to be a way to indicate that from the driver side. > >> > > >> > However, it still may be valuable to check the power-related conditions for > >> > leaving the device in runtime suspend over system suspend/resume in case > >> > it actually doesn't need to be runtime resumed during system suspend after > >> > all. That's what the majority of my patch was about. > >> > > >> > The second problem is that the ACPI PM domain (and the PCI bus type) > >> > runtime resumes all devices unconditionally in its ->suspend callback, > >> > even though that may not be necessary for some devices. Therefore there > >> > needs to be a way to indicate that too. That still would be good to > >> > have *regardless* of the direct_complete mechanism, because the direct_complete > >> > flag may not be set very often due to dependencies and then the > >> > resume-during-suspend will take place unnecessarily. > >> > > >> > Accordingly, it looks like we need a "no need to resume me" flag in the first > >> > place. That would indicate to interested pieces of code that, from the > >> > driver perspective, the device doesn't need to be runtime resumed before > >> > invoking its system suspend callbacks. This should be clear enough to everyone > >> > IMO. > >> > > >> > [Note that if that flag is set for all devices, we may drop it along with > >> > direct_complete, but before that happens both are needed.] > >> > >> I think we are in agreement that direct_complete will not be necessary any > >> more when all drivers/bus types/PM domains and so on can do the "safe > >> suspend", but we're not there yet. :-) > >> > >> > To address the first issue I would add something like the flag in the patches > >> > I sent (but without the ACPI PM domain part which should be covered by the > >> > "no need to resume me" flag above), because that allows the device's ->suspend > >> > callback to run in principle and the driver may use that callback even to > >> > runtime resume the device if that's what it wants to do. So something like > >> > "run my ->suspend callback even though I might stay in runtime suspend". > >> > > >> > I would probably add driver_flags to dev_pm_info for that to set at the probe > >> > time (and I would make the core clear that on driver removal). > >> > > >> > The complexity concern is there, but honestly I don't see a better way at > >> > this point. > >> > >> So below is a prototype patch. It still is missing a documentation update, but > >> other than that it should be complete unless I missed something. > >> > >> The way it works is that the SAFE_SUSPEND flag is not looked at by the core > >> at all. The ACPI PM domain looks at it and the PCI bus type can be modified > >> to take it into account in the future. That is what causes the "runtime resume > >> during system suspend" to be skipped. > >> > >> In turn, the ALWAYS_SUSPEND flag is only looked at by the core and it causes > >> the decision on whether or not to use direct_complete to be deferred to the > >> __device_suspend_late() time. If you set it for a PCI device, the effect is > >> equivalent to "no direct_complete". If you set it for a device in the ACPI > >> PM domain, that depends on whether or not SAFE_SUSPEND is set. If it isn't > >> set, the effect is equivalent to "no direct_complete" too, but if it is set, > >> the core may still try to use direct_complete for the device, but it will > >> make the decision on it in __device_suspend_late() and then it will not invoke > >> the ->suspend_late callback for the device if it is still runtime suspended. > >> [Note that you cannot runtime resume and runtime suspend again a device during > >> system suspend, so if it is runtime suspended in __device_suspend_late(), it > >> has been runtime suspend all the way since device_prepare().] > >> > >> So say you point the ->suspend_late and ->resume_early callbacks of > >> the designware i2c driver to pm_runtime_force_suspend() and > >> pm_runtime_force_resume(), respectively, and set both the SAFE_SUSPEND > >> and ALWAYS_SUSPEND flags for the device. > >> > >> If system suspend is started and the device is not runtime suspended, > >> direct_complete is not set for it and everything works as usual, so say > >> the device is runtime suspended in device_prepare(). Then, the ACPI PM > >> domain checks the other conditions for leaving it in runtime suspend and > >> returns either 0 or a positive number from acpi_subsys_prepare(). > >> > >> If 0 is returned, direct_complete is not set by the core and > >> acpi_subsys_suspend() is called. It checks the SAFE_SUSPEND flag and sees > >> that the device need not be runtime resumed, so it invokes the driver's > >> ->suspend callback (which is not present, so it doesn't do anything). > >> Next, in __device_suspend_late(), acpi_subsys_suspend_late() is invoked > >> and it calls pm_runtime_force_suspend(), which executes the driver's > >> ->runtime_suspend() callback, and then (if successful) calls > >> acpi_dev_suspend_late() to put the device into a low-power state. The > >> resume path is a reverse of the above in this case. So far, so good. > > > > Well, not really, because if the device remains runtime suspended, > > ->runtime_suspend() will not be called by pm_runtime_force_suspend() and > > acpi_dev_suspend_late() should not be called then. > > > > So more changes in the ACPI PM domain are needed after all. > > Yes, that's what I thought as well. > > Anyway, let me cook a new version of the series - trying to address > the first bits you have pointed out. Then we can continue with > fine-tuning on top, addressing further optimizations of the ACPI PM > domain. Actually, please hold on and let me show you what I would like to do first. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, Aug 29, 2017 at 02:18:13AM +0200, Rafael J. Wysocki wrote: > On Wednesday, August 23, 2017 4:42:00 PM CEST Ulf Hansson wrote: > > The i2c designware platform driver, drivers/i2c/busses/i2c-designware-platdrv.c, > > isn't well optimized for system sleep. > > > > What makes this driver particularly interesting is because it's a cross-SoC > > driver, which sometimes means there is an ACPI PM domain attached to the i2c > > device and sometimes not. The driver is being used on both x86 and ARM. ... > Basically, the point is to allow i2c-designware-platdrv to point its late > suspend and early resume callbacks, respectively, to pm_runtime_force_suspend() > and pm_runtime_force_resume() which then will do the right thing regardless of > whether or not the device is runtime suspended when system suspend starts. I'd like to point out a comment added by Hans de Goede in https://bugzilla.kernel.org/show_bug.cgi?id=193891#c99 The D0 / D3 methods of some devices use ACPI OpRegions on the PMIC which is attached to I2C7, these methods get executed by acpi_dev_suspend_late / acpi_dev_resume_early. Since the i2c-designware driver uses regular suspend / resume callbacks it is already suspended at the time those calls happen, leading to a device-suspend error and the system not suspending at all. It's the reason for the Cherrytrail I2C7 special treatment in i2c-designware-platdrv.c and pm_disabled = true in i2c-designware-baytrail.c, however pm_disabled seems to be a problem for S0ix support. To solve it, i2c-designware-platdrv needs to suspend after all devices using ACPI OpRegions for suspend. Johannes -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 29, 2017 at 01:44:11PM +0200, Ulf Hansson wrote: > Did you try out my series (v2) if that could fix this problem in a > more flexible manner? > > In other words, is it fine if the device remains runtime PM enabled > during the entire device_suspend() phase, and also not being suspended > until ->suspend_late()? I tried to try but I also had some test patches applied and it hung up on suspend but I ran out of time to check why... I intend to try again soon. Johannes -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, August 29, 2017 1:44:11 PM CEST Ulf Hansson wrote: > On 29 August 2017 at 12:29, Johannes Stezenbach <js@sig21.net> wrote: > > Hi, > > > > On Tue, Aug 29, 2017 at 02:18:13AM +0200, Rafael J. Wysocki wrote: > >> On Wednesday, August 23, 2017 4:42:00 PM CEST Ulf Hansson wrote: > >> > The i2c designware platform driver, drivers/i2c/busses/i2c-designware-platdrv.c, > >> > isn't well optimized for system sleep. > >> > > >> > What makes this driver particularly interesting is because it's a cross-SoC > >> > driver, which sometimes means there is an ACPI PM domain attached to the i2c > >> > device and sometimes not. The driver is being used on both x86 and ARM. > > ... > >> Basically, the point is to allow i2c-designware-platdrv to point its late > >> suspend and early resume callbacks, respectively, to pm_runtime_force_suspend() > >> and pm_runtime_force_resume() which then will do the right thing regardless of > >> whether or not the device is runtime suspended when system suspend starts. > > > > I'd like to point out a comment added by Hans de Goede in > > https://bugzilla.kernel.org/show_bug.cgi?id=193891#c99 > > > > The D0 / D3 methods of some devices use ACPI OpRegions on the PMIC which is > > attached to I2C7, these methods get executed by acpi_dev_suspend_late / > > acpi_dev_resume_early. Since the i2c-designware driver uses regular suspend / > > resume callbacks it is already suspended at the time those calls happen, > > leading to a device-suspend error and the system not suspending at all. > > Yes, that's why I moved those operation to be managed at the > ->suspend_late() in my series, and at the same time prevent the > direct_complete patch from executed for this device. > > > > > It's the reason for the Cherrytrail I2C7 special treatment in > > i2c-designware-platdrv.c and pm_disabled = true in i2c-designware-baytrail.c, > > however pm_disabled seems to be a problem for S0ix support. > > To solve it, i2c-designware-platdrv needs to suspend after all > > devices using ACPI OpRegions for suspend. > > > > > > Johannes > > Did you try out my series (v2) if that could fix this problem in a > more flexible manner? > > In other words, is it fine if the device remains runtime PM enabled > during the entire device_suspend() phase, and also not being suspended > until ->suspend_late()? Ulf, please, this is a *different* problem. Can we focus on one problem at a time? Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, August 29, 2017 12:29:27 PM CEST Johannes Stezenbach wrote: > Hi, > > On Tue, Aug 29, 2017 at 02:18:13AM +0200, Rafael J. Wysocki wrote: > > On Wednesday, August 23, 2017 4:42:00 PM CEST Ulf Hansson wrote: > > > The i2c designware platform driver, drivers/i2c/busses/i2c-designware-platdrv.c, > > > isn't well optimized for system sleep. > > > > > > What makes this driver particularly interesting is because it's a cross-SoC > > > driver, which sometimes means there is an ACPI PM domain attached to the i2c > > > device and sometimes not. The driver is being used on both x86 and ARM. > ... > > Basically, the point is to allow i2c-designware-platdrv to point its late > > suspend and early resume callbacks, respectively, to pm_runtime_force_suspend() > > and pm_runtime_force_resume() which then will do the right thing regardless of > > whether or not the device is runtime suspended when system suspend starts. > > I'd like to point out a comment added by Hans de Goede in > https://bugzilla.kernel.org/show_bug.cgi?id=193891#c99 > > The D0 / D3 methods of some devices use ACPI OpRegions on the PMIC which is > attached to I2C7, these methods get executed by acpi_dev_suspend_late / > acpi_dev_resume_early. Since the i2c-designware driver uses regular suspend / > resume callbacks it is already suspended at the time those calls happen, > leading to a device-suspend error and the system not suspending at all. > > It's the reason for the Cherrytrail I2C7 special treatment in > i2c-designware-platdrv.c and pm_disabled = true in i2c-designware-baytrail.c, > however pm_disabled seems to be a problem for S0ix support. > To solve it, i2c-designware-platdrv needs to suspend after all > devices using ACPI OpRegions for suspend. That's a totally different issue from the one at hand. It simply means that the devices using I2C for suspend/resume need to be suspended *before* the I2C controller and resumed *after* it. So this is a system suspend/resume *ordering* issue and not a runtime PM vs system suspend issue. It can be addressed, for example, by doing the I2C controller's suspend/resume at the noirq stage. Has anyone tried that? Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29 August 2017 at 02:20, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Add a driver_flags field to struct dev_pm_info for flags that can be > set by device drivers at the probe time to inform the PM core and/or > bus types, PM domains and so on on the capabilities and/or > preferences of device drivers. It is anticipated that more than one > flag of this kind will be necessary going forward. > > Define and document a SAFE_SUSPEND flag to instruct bus types and PM > domains that the system suspend callbacks provided by the driver can > cope with runtime suspended devices, so from the driver's perspective > it should be safe to leave devices in runtime suspend during system > suspend. This changelog is a bit too vague to me. Wouldn't it be more clear if also adding something along the lines that this also means that runtime resuming a device isn't needed by the subsystem/PM domain during system sleep? Because ideally that is what you want to avoid, right? Moreover I am also not convinced that this solution really is the right path. It seems like we might end up adding more bits for the "driver_flag" field and it gets complicated. Do we really need to distinguish between all different cases in such detail? I will continue to review this tomorrow, however in the meantime I have finalized a re-spin of my v3 series so I decided to post it anyway. I am adding only one new flag to the PM core, perhaps I am over-simplifying things, but please have look once more. I think I have addressed all your concerns you have raised so far. Kind regards Uffe > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > Documentation/driver-api/pm/devices.rst | 7 +++++++ > drivers/base/dd.c | 2 ++ > include/linux/pm.h | 16 ++++++++++++++++ > 3 files changed, 25 insertions(+) > > Index: linux-pm/include/linux/pm.h > =================================================================== > --- linux-pm.orig/include/linux/pm.h > +++ linux-pm/include/linux/pm.h > @@ -550,6 +550,21 @@ struct pm_subsys_data { > #endif > }; > > +/* > + * Driver flags to control system suspend/resume behavior. > + * > + * These flags can be set by device drivers at the probe time. They need not be > + * cleared by the drivers as the driver core will take care of that. > + * > + * SAFE_SUSPEND: No need to runtime resume the device during system suspend. > + * > + * Setting SAFE_SUSPEND instructs bus types and PM domains which may want to > + * runtime resume the device upfront during system suspend that doing so is not > + * necessary from the driver's perspective, because the system suspend callbacks > + * provided by it can cope with a runtime suspended device. > + */ > +#define DPM_FLAG_SAFE_SUSPEND BIT(0) > + > struct dev_pm_info { > pm_message_t power_state; > unsigned int can_wakeup:1; > @@ -561,6 +576,7 @@ struct dev_pm_info { > bool is_late_suspended:1; > bool early_init:1; /* Owned by the PM core */ > bool direct_complete:1; /* Owned by the PM core */ > + unsigned int driver_flags; > spinlock_t lock; > #ifdef CONFIG_PM_SLEEP > struct list_head entry; > Index: linux-pm/drivers/base/dd.c > =================================================================== > --- linux-pm.orig/drivers/base/dd.c > +++ linux-pm/drivers/base/dd.c > @@ -436,6 +436,7 @@ pinctrl_bind_failed: > if (dev->pm_domain && dev->pm_domain->dismiss) > dev->pm_domain->dismiss(dev); > pm_runtime_reinit(dev); > + dev->power.driver_flags = 0; > > switch (ret) { > case -EPROBE_DEFER: > @@ -841,6 +842,7 @@ static void __device_release_driver(stru > if (dev->pm_domain && dev->pm_domain->dismiss) > dev->pm_domain->dismiss(dev); > pm_runtime_reinit(dev); > + dev->power.driver_flags = 0; > > klist_remove(&dev->p->knode_driver); > device_pm_check_callbacks(dev); > Index: linux-pm/Documentation/driver-api/pm/devices.rst > =================================================================== > --- linux-pm.orig/Documentation/driver-api/pm/devices.rst > +++ linux-pm/Documentation/driver-api/pm/devices.rst > @@ -729,6 +729,13 @@ state temporarily, for example so that i > disabled. This all depends on the hardware and the design of the subsystem and > device driver in question. > > +Some bus types and PM domains have a policy to runtime resume all > +devices upfront in their ``->suspend`` callbacks, but that may not be really > +necessary if the system suspend-resume callbacks provided by the device's > +driver can cope with a runtime-suspended device. The driver can indicate that > +by setting ``DPM_FLAG_SAFE_SUSPEND`` in :c:member:`power.driver_flags` at the > +probe time. > + > During system-wide resume from a sleep state it's easiest to put devices into > the full-power state, as explained in :file:`Documentation/power/runtime_pm.txt`. > Refer to that document for more information regarding this particular issue as > > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, August 29, 2017 4:57:28 PM CEST Ulf Hansson wrote: > On 29 August 2017 at 02:20, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Add a driver_flags field to struct dev_pm_info for flags that can be > > set by device drivers at the probe time to inform the PM core and/or > > bus types, PM domains and so on on the capabilities and/or > > preferences of device drivers. It is anticipated that more than one > > flag of this kind will be necessary going forward. > > > > Define and document a SAFE_SUSPEND flag to instruct bus types and PM > > domains that the system suspend callbacks provided by the driver can > > cope with runtime suspended devices, so from the driver's perspective > > it should be safe to leave devices in runtime suspend during system > > suspend. > > This changelog is a bit too vague to me. Wouldn't it be more clear if > also adding something along the lines that this also means that > runtime resuming a device isn't needed by the subsystem/PM domain > during system sleep? No. > Because ideally that is what you want to avoid, right? Not really. The driver doesn't know what the needs of the higher level are. It may only say what it can do and the bus type can use this information to make a decision. > Moreover I am also not convinced that this solution really is the > right path. It seems like we might end up adding more bits for the > "driver_flag" field and it gets complicated. Do we really need to > distinguish between all different cases in such detail? Yes, we do. Every time we try to address two different problems with one mechanism, it backfires later. > I will continue to review this tomorrow, however in the meantime I > have finalized a re-spin of my v3 series so I decided to post it > anyway. I am adding only one new flag to the PM core, perhaps I am > over-simplifying things, but please have look once more. I think I > have addressed all your concerns you have raised so far. I'll have a look, but I really don't want to conflate the "I'm fine with not resuming the device" case with the "I don't want to use direct_complete with it" one. To me, they are fundamentally different and I'm not going to apply any patches conflating them. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, August 29, 2017 2:59:49 AM CEST Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Rework the power management part of the i2c-designware-platdrv driver > so that its ->suspend and ->resume callbacks do not point to the > callback routines used by it for runtime PM. Instead, point its late > suspend and early resume callbacks to pm_runtime_force_suspend() and > pm_runtime_force_resume(), respectively, and make it set the > SAFE_SUSPEND driver flag (introduced earlier) to instruct the generic > ACPI PM domain code that the driver can cope with runtime suspended > devices in its system sleep callbacks. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/i2c/busses/i2c-designware-platdrv.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > Index: linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c > =================================================================== > --- linux-pm.orig/drivers/i2c/busses/i2c-designware-platdrv.c > +++ linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c > @@ -358,6 +358,7 @@ static int dw_i2c_plat_probe(struct plat > if (dev->pm_disabled) { > pm_runtime_forbid(&pdev->dev); > } else { > + dev->power.driver_flags = DPM_FLAG_SAFE_SUSPEND; > pm_runtime_set_autosuspend_delay(&pdev->dev, 1000); > pm_runtime_use_autosuspend(&pdev->dev); > pm_runtime_set_active(&pdev->dev); > @@ -455,7 +456,8 @@ static int dw_i2c_plat_resume(struct dev > static const struct dev_pm_ops dw_i2c_dev_pm_ops = { > .prepare = dw_i2c_plat_prepare, > .complete = dw_i2c_plat_complete, > - SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume) > + SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > + pm_runtime_force_resume) > SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, NULL) > }; This isn't going to work, because pm_runtime_force_suspend() will invoke the bus type callback and not the driver's one. So scratch this, please. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, August 29, 2017 6:38:11 PM CEST Rafael J. Wysocki wrote: > On Tuesday, August 29, 2017 2:59:49 AM CEST Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Rework the power management part of the i2c-designware-platdrv driver > > so that its ->suspend and ->resume callbacks do not point to the > > callback routines used by it for runtime PM. Instead, point its late > > suspend and early resume callbacks to pm_runtime_force_suspend() and > > pm_runtime_force_resume(), respectively, and make it set the > > SAFE_SUSPEND driver flag (introduced earlier) to instruct the generic > > ACPI PM domain code that the driver can cope with runtime suspended > > devices in its system sleep callbacks. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/i2c/busses/i2c-designware-platdrv.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > Index: linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c > > =================================================================== > > --- linux-pm.orig/drivers/i2c/busses/i2c-designware-platdrv.c > > +++ linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c > > @@ -358,6 +358,7 @@ static int dw_i2c_plat_probe(struct plat > > if (dev->pm_disabled) { > > pm_runtime_forbid(&pdev->dev); > > } else { > > + dev->power.driver_flags = DPM_FLAG_SAFE_SUSPEND; > > pm_runtime_set_autosuspend_delay(&pdev->dev, 1000); > > pm_runtime_use_autosuspend(&pdev->dev); > > pm_runtime_set_active(&pdev->dev); > > @@ -455,7 +456,8 @@ static int dw_i2c_plat_resume(struct dev > > static const struct dev_pm_ops dw_i2c_dev_pm_ops = { > > .prepare = dw_i2c_plat_prepare, > > .complete = dw_i2c_plat_complete, > > - SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume) > > + SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > > + pm_runtime_force_resume) > > SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, NULL) > > }; > > This isn't going to work, because pm_runtime_force_suspend() will invoke > the bus type callback and not the driver's one. > > So scratch this, please. BTW, I don't think it is OK to mess up with bus type _runtime_suspend and _runtime_resume and try to make them magically handle the system suspend case as well. There has to be a different way ... Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, August 29, 2017 5:05:20 PM CEST Ulf Hansson wrote: > On 29 August 2017 at 16:43, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Tuesday, August 29, 2017 1:44:11 PM CEST Ulf Hansson wrote: > >> On 29 August 2017 at 12:29, Johannes Stezenbach <js@sig21.net> wrote: > >> > Hi, > >> > > >> > On Tue, Aug 29, 2017 at 02:18:13AM +0200, Rafael J. Wysocki wrote: > >> >> On Wednesday, August 23, 2017 4:42:00 PM CEST Ulf Hansson wrote: > >> >> > The i2c designware platform driver, drivers/i2c/busses/i2c-designware-platdrv.c, > >> >> > isn't well optimized for system sleep. > >> >> > > >> >> > What makes this driver particularly interesting is because it's a cross-SoC > >> >> > driver, which sometimes means there is an ACPI PM domain attached to the i2c > >> >> > device and sometimes not. The driver is being used on both x86 and ARM. > >> > ... > >> >> Basically, the point is to allow i2c-designware-platdrv to point its late > >> >> suspend and early resume callbacks, respectively, to pm_runtime_force_suspend() > >> >> and pm_runtime_force_resume() which then will do the right thing regardless of > >> >> whether or not the device is runtime suspended when system suspend starts. > >> > > >> > I'd like to point out a comment added by Hans de Goede in > >> > https://bugzilla.kernel.org/show_bug.cgi?id=193891#c99 > >> > > >> > The D0 / D3 methods of some devices use ACPI OpRegions on the PMIC which is > >> > attached to I2C7, these methods get executed by acpi_dev_suspend_late / > >> > acpi_dev_resume_early. Since the i2c-designware driver uses regular suspend / > >> > resume callbacks it is already suspended at the time those calls happen, > >> > leading to a device-suspend error and the system not suspending at all. > >> > >> Yes, that's why I moved those operation to be managed at the > >> ->suspend_late() in my series, and at the same time prevent the > >> direct_complete patch from executed for this device. > >> > >> > > >> > It's the reason for the Cherrytrail I2C7 special treatment in > >> > i2c-designware-platdrv.c and pm_disabled = true in i2c-designware-baytrail.c, > >> > however pm_disabled seems to be a problem for S0ix support. > >> > To solve it, i2c-designware-platdrv needs to suspend after all > >> > devices using ACPI OpRegions for suspend. > >> > > >> > > >> > Johannes > >> > >> Did you try out my series (v2) if that could fix this problem in a > >> more flexible manner? > >> > >> In other words, is it fine if the device remains runtime PM enabled > >> during the entire device_suspend() phase, and also not being suspended > >> until ->suspend_late()? > > > > Ulf, please, this is a *different* problem. > > Yes, it is! > > Just wanted to point out that if the device remains runtime PM enabled > the entire device_suspend() phase, that *could* solve the problem. > > > > > Can we focus on one problem at a time? > > Yes! > > However, there is lots of things following when we try to enable the > runtime PM centric path for ACPI. :-) Which, I guess, we won't do after all ... So I would prefer to think about addressing problems instead of trying to "enable the runtime PM centric path for ACPI" just for the sake of it. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
The i2c designware platform driver, drivers/i2c/busses/i2c-designware-platdrv.c, isn't well optimized for system sleep. What makes this driver particularly interesting is because it's a cross-SoC driver, which sometimes means there is an ACPI PM domain attached to the i2c device and sometimes not. The driver is being used on both x86 and ARM. In principle, to optimize the system sleep support in i2c driver, this series enables the proven runtime PM centric path for the i2c driver. However, to do that the ACPI PM domain also have to collaborate and understand this behaviour. Therefore a number of changes, patch 1 to patch 7, makes the needed changes to the ACPI PM domain. In patch8 and patch 9, the i2c driver gets optimized and is converted to the runtime PM centric path for system sleep. It shall be noted, the behaviour of the ACPI PM domain should remain intact, still taking benefit of using the direct_complete path during system sleep, except for those drivers that explicitly request it not to (via a new ACPI API added in this series). This series has been tested on an ARM64 Hikey board, which isn't having the i2c device attached to the ACPI PM domain. This means that the ACPI changes needs to be tested on some relevant Intel SoCs and it's greatly appreciated is someone could help out with this, so is of course review comments. Some news in v2: - The v1 contained a fix for the i2c driver, this has been sent separately [1] and picked up for fixes by Wolfram for v4.13-rcs. However the fix has not yet reached Linus' tree. The changes on i2c driver are based upon that change. - To simplify for testers, I have published a branch [2] based upon Rafael's pm tree and linux-next branch, which also includes the above patch. - Rephrased the coverletter to clarify the intent of this series. - Addressed review comments from v1. [1] http://patchwork.ozlabs.org/patch/799803/ [2] git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git acpi_pm_i2c_rpm_path_v2 Kind regards Ulf Hansson Ulf Hansson (9): PM / ACPI: Restore acpi_subsys_complete() PM / Sleep: Remove pm_complete_with_resume_check() PM / ACPI: Split code validating need for runtime resume in ->prepare() PM / ACPI: Split acpi_lpss_suspend_late|resume_early() PM / ACPI: Provide option to disable direct_complete for ACPI devices PM / ACPI: Enable the runtime PM centric approach for system sleep PM / ACPI: Avoid runtime resuming device in acpi_subsys_suspend|freeze() i2c: designware: Don't resume device in the ->complete() callback i2c: designware: Deploy the runtime PM centric approach for system sleep drivers/acpi/acpi_lpss.c | 81 +++++++++---- drivers/acpi/device_pm.c | 169 ++++++++++++++++++++++++---- drivers/base/power/generic_ops.c | 23 ---- drivers/i2c/busses/i2c-designware-platdrv.c | 35 +----- include/acpi/acpi_bus.h | 1 + include/linux/acpi.h | 4 + include/linux/pm.h | 1 - 7 files changed, 218 insertions(+), 96 deletions(-) -- 2.7.4