omap-i2c: runtime pm issue during suspend to ram

Hello,

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


 };

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