| Message ID | f68c9a54-0fde-4709-9d2f-0d23a049341b@bootlin.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | omap-i2c: runtime pm issue during suspend to ram | expand |
On 12/19/23 18:15, Thomas Richard wrote: > Hello, I add some people in this thread. > > I have a gpio expander (pca953x driver) connected to an i2c controller > managed by the omap-i2c driver. > And I have some issues with pm_runtime_force_suspend/resume during > suspend to ram. > For some reasons, related to hardware design, I need to access to this > gpio expander during suspend_noirq and resume_noirq. So I had to move > the suspend/resume of the pca953x to suspend_noirq/resume_noirq. > > The i2c controller is autosuspended when I start the suspend sequence. > In suspend_noirq, I access to one gpio of the expander, so rpm_resume is > called to resume the i2c controller. > And rpm_resume returns an error because disable_depth > 0 [1]. In > suspend_noirq, runtime pm is disabled (disable_depth is incremented when > runtime pm is disabled [2]). So the expander is not reachable, and the > access fails. > > [1] > https://elixir.bootlin.com/linux/v6.7-rc6/source/drivers/base/power/runtime.c#L773 > [2] > https://elixir.bootlin.com/linux/v6.7-rc6/source/drivers/base/power/runtime.c#L1474 > > The suspend_noirq of the gpio expander don't do i2c access, so no > problem for pca953x suspend. > The pm_runtime_force_suspend (suspend_noirq [3]) of the i2c controller > does nothing as the device is already suspended [4]. > > [3] > https://elixir.bootlin.com/linux/v6.7-rc6/source/drivers/i2c/busses/i2c-omap.c#L1579 > [4] > https://elixir.bootlin.com/linux/v6.7-rc6/source/drivers/base/power/runtime.c#L1878 > > Then during the pm_runtime_force_resume (resume_noirq [3]) the i2c > controller is not resumed because needs_for_resume is equal to 0 [5]. > The needs_for_resume flag is set in pm_runtime_force_suspend [6] but we > don't reach this point, because the device is already suspended [4]. > > [5] > https://elixir.bootlin.com/linux/v6.7-rc6/source/drivers/base/power/runtime.c#L1929 > [6] > https://elixir.bootlin.com/linux/v6.7-rc6/source/drivers/base/power/runtime.c#L1900 > > Then the resume_noirq of the pca953x driver is called, consequently > rpm_resume is called to resume the i2c controller. But it is never > resumed because disable_depth > 0 [7] (runtime pm is still disabled in > resume_noirq). So the resume_noirq fails. > > [7] > https://elixir.bootlin.com/linux/v6.7-rc6/source/drivers/base/power/runtime.c#L773 > > I found a workaround which is to resume the controller and disable > runtime pm during suspend, then runtime pm is enabled during resume. > But there is probably a better solution to fix this issue. > > Best Regards, > > Thomas Richard > > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index 42165ef57946..fe79b27b46fd 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -1575,9 +1575,24 @@ static int __maybe_unused > omap_i2c_runtime_resume(struct device *dev) > return 0; > } > > +static int omap_i2c_suspend(struct device *dev) > +{ > + pm_runtime_get_sync(dev); > + pm_runtime_disable(dev); > + return 0; > +} > + > +static int omap_i2c_resume(struct device *dev) > +{ > + pm_runtime_enable(dev); > + pm_runtime_put_autosuspend(dev); > + return 0; > +} > + > static const struct dev_pm_ops omap_i2c_pm_ops = { > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > pm_runtime_force_resume) > + SET_SYSTEM_SLEEP_PM_OPS(omap_i2c_suspend, omap_i2c_resume) > SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend, > omap_i2c_runtime_resume, NULL) > }; > > >
* Thomas Richard <thomas.richard@bootlin.com> [231220 10:50]: > On 12/19/23 18:15, Thomas Richard wrote: > > Hello, > > I add some people in this thread. > > > > > I have a gpio expander (pca953x driver) connected to an i2c controller > > managed by the omap-i2c driver. > > And I have some issues with pm_runtime_force_suspend/resume during > > suspend to ram. > > For some reasons, related to hardware design, I need to access to this > > gpio expander during suspend_noirq and resume_noirq. So I had to move > > the suspend/resume of the pca953x to suspend_noirq/resume_noirq. Hmm at noirq level you need to do polling on the i2c controller? > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > > index 42165ef57946..fe79b27b46fd 100644 > > --- a/drivers/i2c/busses/i2c-omap.c > > +++ b/drivers/i2c/busses/i2c-omap.c > > @@ -1575,9 +1575,24 @@ static int __maybe_unused > > omap_i2c_runtime_resume(struct device *dev) > > return 0; > > } > > > > +static int omap_i2c_suspend(struct device *dev) > > +{ > > + pm_runtime_get_sync(dev); > > + pm_runtime_disable(dev); > > + return 0; > > +} If you want the i2c controller enabled during suspend, you can leave it enabled above, and as we already have SET_NOIRQ_SYSTEM_SLEEP_PM_OPS doing force_suspend() and force_resume(), you can runtime PM put on resume. So something like below might do the trick: static int omap_i2c_suspend(struct device *dev) { return pm_runtime_resume_and_get(dev); } static int omap_i2c_resume(struct device *dev) { pm_runtime_mark_last_busy(dev); pm_runtime_put_autosuspend(dev); return 0; } > > static const struct dev_pm_ops omap_i2c_pm_ops = { > > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > > pm_runtime_force_resume) > > + SET_SYSTEM_SLEEP_PM_OPS(omap_i2c_suspend, omap_i2c_resume) > > SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend, > > omap_i2c_runtime_resume, NULL) > > }; And with the changes you did to omap_i2c_pm_ops naturally. This way the controller should stay active until noirq ops. Of course it's possible I did not quite understand what you're trying to do, but if so please let me know :) Regards, Tony
On 12/20/23 12:14, Tony Lindgren wrote: > * Thomas Richard <thomas.richard@bootlin.com> [231220 10:50]: >> On 12/19/23 18:15, Thomas Richard wrote: >>> Hello, >> >> I add some people in this thread. >> >>> >>> I have a gpio expander (pca953x driver) connected to an i2c controller >>> managed by the omap-i2c driver. >>> And I have some issues with pm_runtime_force_suspend/resume during >>> suspend to ram. >>> For some reasons, related to hardware design, I need to access to this >>> gpio expander during suspend_noirq and resume_noirq. So I had to move >>> the suspend/resume of the pca953x to suspend_noirq/resume_noirq. > > Hmm at noirq level you need to do polling on the i2c controller? Hello Tony, Thanks for your reply. No, irq is still active in suspend_noirq for this i2c controller due to the flag IRQF_NO_SUSPEND [1]. If this flag is set, the interrupt is still enabled in suspend_noirq [2]. [1] https://elixir.bootlin.com/linux/v6.7-rc6/source/drivers/i2c/busses/i2c-omap.c#L1473 [2] https://www.kernel.org/doc/html/latest/power/suspend-and-interrupts.html#the-irqf-no-suspend-flag > >>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c >>> index 42165ef57946..fe79b27b46fd 100644 >>> --- a/drivers/i2c/busses/i2c-omap.c >>> +++ b/drivers/i2c/busses/i2c-omap.c >>> @@ -1575,9 +1575,24 @@ static int __maybe_unused >>> omap_i2c_runtime_resume(struct device *dev) >>> return 0; >>> } >>> >>> +static int omap_i2c_suspend(struct device *dev) >>> +{ >>> + pm_runtime_get_sync(dev); >>> + pm_runtime_disable(dev); >>> + return 0; >>> +} > > If you want the i2c controller enabled during suspend, you can leave it > enabled above, and as we already have SET_NOIRQ_SYSTEM_SLEEP_PM_OPS > doing force_suspend() and force_resume(), you can runtime PM put on > resume. So something like below might do the trick: Ok I'll test it. Thanks > > static int omap_i2c_suspend(struct device *dev) > { > return pm_runtime_resume_and_get(dev); > } > > static int omap_i2c_resume(struct device *dev) > { > pm_runtime_mark_last_busy(dev); > pm_runtime_put_autosuspend(dev); > > return 0; > } > >>> static const struct dev_pm_ops omap_i2c_pm_ops = { >>> SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, >>> pm_runtime_force_resume) >>> + SET_SYSTEM_SLEEP_PM_OPS(omap_i2c_suspend, omap_i2c_resume) >>> SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend, >>> omap_i2c_runtime_resume, NULL) >>> }; > > And with the changes you did to omap_i2c_pm_ops naturally. This way > the controller should stay active until noirq ops. > > Of course it's possible I did not quite understand what you're trying > to do, but if so please let me know :) The context is: One gpio of this expander is the reset pin of PCIe endpoints. And this reset pin shall be managed during the suspend_noirq and resume_noirq of the PCIe controller. That's why I need to do some i2c accesses during suspend_noirq/resume_noirq.
Hello, On Wed Dec 20, 2023 at 12:36 PM CET, Thomas Richard wrote: > On 12/20/23 12:14, Tony Lindgren wrote: > > * Thomas Richard <thomas.richard@bootlin.com> [231220 10:50]: > >> On 12/19/23 18:15, Thomas Richard wrote: > >>> Hello, > >> > >> I add some people in this thread. > >> > >>> > >>> I have a gpio expander (pca953x driver) connected to an i2c controller > >>> managed by the omap-i2c driver. > >>> And I have some issues with pm_runtime_force_suspend/resume during > >>> suspend to ram. > >>> For some reasons, related to hardware design, I need to access to this > >>> gpio expander during suspend_noirq and resume_noirq. So I had to move > >>> the suspend/resume of the pca953x to suspend_noirq/resume_noirq. > > > > Hmm at noirq level you need to do polling on the i2c controller? > > Hello Tony, > > Thanks for your reply. > > No, irq is still active in suspend_noirq for this i2c controller due to > the flag IRQF_NO_SUSPEND [1]. > If this flag is set, the interrupt is still enabled in suspend_noirq [2]. > > [1] > https://elixir.bootlin.com/linux/v6.7-rc6/source/drivers/i2c/busses/i2c-omap.c#L1473 > [2] > https://www.kernel.org/doc/html/latest/power/suspend-and-interrupts.html#the-irqf-no-suspend-flag > > > > >>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > >>> index 42165ef57946..fe79b27b46fd 100644 > >>> --- a/drivers/i2c/busses/i2c-omap.c > >>> +++ b/drivers/i2c/busses/i2c-omap.c > >>> @@ -1575,9 +1575,24 @@ static int __maybe_unused > >>> omap_i2c_runtime_resume(struct device *dev) > >>> return 0; > >>> } > >>> > >>> +static int omap_i2c_suspend(struct device *dev) > >>> +{ > >>> + pm_runtime_get_sync(dev); > >>> + pm_runtime_disable(dev); > >>> + return 0; > >>> +} > > > > If you want the i2c controller enabled during suspend, you can leave it > > enabled above, and as we already have SET_NOIRQ_SYSTEM_SLEEP_PM_OPS > > doing force_suspend() and force_resume(), you can runtime PM put on > > resume. So something like below might do the trick: > > Ok I'll test it. Thanks The issue with this approach is that it requires knowing at suspend-time if the controller will be used at resume_noirq-time. Ideally the controller's behavior would not be modified until a xfer is done at resume_noirq time. There are many platforms that use this driver that probably don't need the controller woken up. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On 12/20/23 14:46, Théo Lebrun wrote: > Hello, > > On Wed Dec 20, 2023 at 12:36 PM CET, Thomas Richard wrote: >> On 12/20/23 12:14, Tony Lindgren wrote: >>> * Thomas Richard <thomas.richard@bootlin.com> [231220 10:50]: >>>> On 12/19/23 18:15, Thomas Richard wrote: >>>>> Hello, >>>> >>>> I add some people in this thread. >>>> >>>>> >>>>> I have a gpio expander (pca953x driver) connected to an i2c controller >>>>> managed by the omap-i2c driver. >>>>> And I have some issues with pm_runtime_force_suspend/resume during >>>>> suspend to ram. >>>>> For some reasons, related to hardware design, I need to access to this >>>>> gpio expander during suspend_noirq and resume_noirq. So I had to move >>>>> the suspend/resume of the pca953x to suspend_noirq/resume_noirq. >>> >>> Hmm at noirq level you need to do polling on the i2c controller? >> >> Hello Tony, >> >> Thanks for your reply. >> >> No, irq is still active in suspend_noirq for this i2c controller due to >> the flag IRQF_NO_SUSPEND [1]. >> If this flag is set, the interrupt is still enabled in suspend_noirq [2]. >> >> [1] >> https://elixir.bootlin.com/linux/v6.7-rc6/source/drivers/i2c/busses/i2c-omap.c#L1473 >> [2] >> https://www.kernel.org/doc/html/latest/power/suspend-and-interrupts.html#the-irqf-no-suspend-flag >> >>> >>>>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c >>>>> index 42165ef57946..fe79b27b46fd 100644 >>>>> --- a/drivers/i2c/busses/i2c-omap.c >>>>> +++ b/drivers/i2c/busses/i2c-omap.c >>>>> @@ -1575,9 +1575,24 @@ static int __maybe_unused >>>>> omap_i2c_runtime_resume(struct device *dev) >>>>> return 0; >>>>> } >>>>> >>>>> +static int omap_i2c_suspend(struct device *dev) >>>>> +{ >>>>> + pm_runtime_get_sync(dev); >>>>> + pm_runtime_disable(dev); >>>>> + return 0; >>>>> +} >>> >>> If you want the i2c controller enabled during suspend, you can leave it >>> enabled above, and as we already have SET_NOIRQ_SYSTEM_SLEEP_PM_OPS >>> doing force_suspend() and force_resume(), you can runtime PM put on >>> resume. So something like below might do the trick: >> >> Ok I'll test it. Thanks > > The issue with this approach is that it requires knowing at suspend-time > if the controller will be used at resume_noirq-time. Ideally the > controller's behavior would not be modified until a xfer is done at > resume_noirq time. There are many platforms that use this driver that > probably don't need the controller woken up. > @Tony, I tested your patch and it works well. Thanks !! As Théo mentioned it, we wake up the controller even if noboy need it in suspend_noirq/resume_noirq. I don't know if the fact that we cannot wake up a runtime suspended device in suspend_noirq/resume_noirq (yes I know runtime pm is disabled in suspend_noirq/resume_noirq) is a bug or not.
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index 42165ef57946..fe79b27b46fd 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -1575,9 +1575,24 @@ static int __maybe_unused omap_i2c_runtime_resume(struct device *dev) return 0; } +static int omap_i2c_suspend(struct device *dev) +{ + pm_runtime_get_sync(dev); + pm_runtime_disable(dev); + return 0; +} + +static int omap_i2c_resume(struct device *dev) +{ + pm_runtime_enable(dev); + pm_runtime_put_autosuspend(dev); + return 0; +} + static const struct dev_pm_ops omap_i2c_pm_ops = { SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume) + SET_SYSTEM_SLEEP_PM_OPS(omap_i2c_suspend, omap_i2c_resume) SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend, omap_i2c_runtime_resume, NULL)