| Message ID | f68c9a54-0fde-4709-9d2f-0d23a049341b@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | omap-i2c: runtime pm issue during suspend to ram | expand |
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)