Re: omap-i2c: runtime pm issue during suspend to ram

[Thomas Richard <thomas.richard@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.

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com