[PATCHv2] i2c: omap: Use noirq system sleep pm ops to idle device for suspend

Message ID 20190110155916.41266-1-tony@atomide.com
State Accepted
Headers show
Series
  • [PATCHv2] i2c: omap: Use noirq system sleep pm ops to idle device for suspend
Related show

Commit Message

Tony Lindgren Jan. 10, 2019, 3:59 p.m.
We currently get the following error with pixcir_ts driver during a
suspend resume cycle:

omap_i2c 4802a000.i2c: controller timed out
pixcir_ts 1-005c: pixcir_int_enable: can't read reg 0x34 : -110
pixcir_ts 1-005c: Failed to disable interrupt generation: -110
pixcir_ts 1-005c: Failed to stop
dpm_run_callback(): pixcir_i2c_ts_resume+0x0/0x98
[pixcir_i2c_ts] returns -110
PM: Device 1-005c failed to resume: error -110

And at least am437x based devices with pixcir_ts will fail to resume
to a touchscreen that is configured as the wakeup-source in device
tree for these devices.

This is because pixcir_ts tries to reconfigure it's registers for
noirq suspend which fails. This also leaves i2c-omap in enabled state
for suspend.

Let's fix the pixcir_ts issue and make sure i2c-omap is suspended by
adding SET_NOIRQ_SYSTEM_SLEEP_PM_OPS. In the long run the solution
seems to be to move the handling of suspended adapters to the i2c
core, but that still needs some more work.

Let's also get rid of some ifdefs while at it and replace them with
__maybe_unused as SET_RUNTIME_PM_OPS and SET_NOIRQ_SYSTEM_SLEEP_PM_OPS
already deal with the various PM Kconfig options.

Cc: Dave Gerlach <d-gerlach@ti.com>
Cc: Grygorii Strashko <grygorii.strashko@ti.com>
Cc: Keerthy <j-keerthy@ti.com>
Cc: Tero Kristo <t-kristo@ti.com>
Reported-by: Keerthy <j-keerthy@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---

Wolfram, this is still pending, can you please apply this a fix for
the -rc series if no objections?

Changes since v1:

- Updated comments with the pixcir_ts related error messages

---
 drivers/i2c/busses/i2c-omap.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

Comments

Wolfram Sang Jan. 10, 2019, 11:31 p.m. | #1
Hi Tony,

On Thu, Jan 10, 2019 at 07:59:16AM -0800, Tony Lindgren wrote:
> We currently get the following error with pixcir_ts driver during a
> suspend resume cycle:
> 
> omap_i2c 4802a000.i2c: controller timed out
> pixcir_ts 1-005c: pixcir_int_enable: can't read reg 0x34 : -110
> pixcir_ts 1-005c: Failed to disable interrupt generation: -110
> pixcir_ts 1-005c: Failed to stop
> dpm_run_callback(): pixcir_i2c_ts_resume+0x0/0x98
> [pixcir_i2c_ts] returns -110
> PM: Device 1-005c failed to resume: error -110
> 
> And at least am437x based devices with pixcir_ts will fail to resume
> to a touchscreen that is configured as the wakeup-source in device
> tree for these devices.
> 
> This is because pixcir_ts tries to reconfigure it's registers for
> noirq suspend which fails. This also leaves i2c-omap in enabled state
> for suspend.

I didn't fully get this. Does it reconfigure the registers for noirq or
during noirq? If the former, why exactly does it fail?

> Let's fix the pixcir_ts issue and make sure i2c-omap is suspended by
> adding SET_NOIRQ_SYSTEM_SLEEP_PM_OPS. In the long run the solution
> seems to be to move the handling of suspended adapters to the i2c
> core, but that still needs some more work.

I don't know if you followed this, but we failed with a generic handling
within the core. We have helpers now which drivers can apply
individually.

> Let's also get rid of some ifdefs while at it and replace them with
> __maybe_unused as SET_RUNTIME_PM_OPS and SET_NOIRQ_SYSTEM_SLEEP_PM_OPS
> already deal with the various PM Kconfig options.
> 
> Cc: Dave Gerlach <d-gerlach@ti.com>
> Cc: Grygorii Strashko <grygorii.strashko@ti.com>
> Cc: Keerthy <j-keerthy@ti.com>
> Cc: Tero Kristo <t-kristo@ti.com>
> Reported-by: Keerthy <j-keerthy@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
> 
> Wolfram, this is still pending, can you please apply this a fix for
> the -rc series if no objections?

Yes, I can do that (when I have no further questions ;))

Regards,

   Wolfram
Tony Lindgren Jan. 10, 2019, 11:45 p.m. | #2
* Wolfram Sang <wsa@the-dreams.de> [190110 23:31]:
> Hi Tony,
> 
> On Thu, Jan 10, 2019 at 07:59:16AM -0800, Tony Lindgren wrote:
> > We currently get the following error with pixcir_ts driver during a
> > suspend resume cycle:
> > 
> > omap_i2c 4802a000.i2c: controller timed out
> > pixcir_ts 1-005c: pixcir_int_enable: can't read reg 0x34 : -110
> > pixcir_ts 1-005c: Failed to disable interrupt generation: -110
> > pixcir_ts 1-005c: Failed to stop
> > dpm_run_callback(): pixcir_i2c_ts_resume+0x0/0x98
> > [pixcir_i2c_ts] returns -110
> > PM: Device 1-005c failed to resume: error -110
> > 
> > And at least am437x based devices with pixcir_ts will fail to resume
> > to a touchscreen that is configured as the wakeup-source in device
> > tree for these devices.
> > 
> > This is because pixcir_ts tries to reconfigure it's registers for
> > noirq suspend which fails. This also leaves i2c-omap in enabled state
> > for suspend.
> 
> I didn't fully get this. Does it reconfigure the registers for noirq or
> during noirq? If the former, why exactly does it fail?

On suspend and resume, pixcir_ts tries to configure registers
during noirq. At that point we already have PM runtime
rpm_check_suspend_allowed() return -EACCESS for PM runtime
which leads to the pixcir -ETIMEDOUT 110 error from the i2c
controller.

And in general, I don't think we should attempt any i2c during
noirq :)

> > Let's fix the pixcir_ts issue and make sure i2c-omap is suspended by
> > adding SET_NOIRQ_SYSTEM_SLEEP_PM_OPS. In the long run the solution
> > seems to be to move the handling of suspended adapters to the i2c
> > core, but that still needs some more work.
> 
> I don't know if you followed this, but we failed with a generic handling
> within the core. We have helpers now which drivers can apply
> individually.

Hmm I don't follow you you here. Which helpers are you
talking about here?

I'm only aware of your pending patches from thread
"[PATCH v2 0/9] i2c: move handling of suspended adapters
to the core".

Regards,

Tony
Keerthy Jan. 11, 2019, 10:05 a.m. | #3
On Friday 11 January 2019 05:15 AM, Tony Lindgren wrote:
> * Wolfram Sang <wsa@the-dreams.de> [190110 23:31]:
>> Hi Tony,
>>
>> On Thu, Jan 10, 2019 at 07:59:16AM -0800, Tony Lindgren wrote:
>>> We currently get the following error with pixcir_ts driver during a
>>> suspend resume cycle:
>>>
>>> omap_i2c 4802a000.i2c: controller timed out
>>> pixcir_ts 1-005c: pixcir_int_enable: can't read reg 0x34 : -110
>>> pixcir_ts 1-005c: Failed to disable interrupt generation: -110
>>> pixcir_ts 1-005c: Failed to stop
>>> dpm_run_callback(): pixcir_i2c_ts_resume+0x0/0x98
>>> [pixcir_i2c_ts] returns -110
>>> PM: Device 1-005c failed to resume: error -110

- Grygorii

Tony,

I spent some time today bisecting the issue. Unfortunately could not get to a
proper commit but i believe this error got introduced after the ti,sysc
migration series while i started bisecting it am437x stopped booting. May be it
would worth looking at the dt nodes after sysc migration.

This patch surely solves that error but may be if you can look at the migration
patches a bit. So this error starts appearing in 5.0 only. 4.20 has no issues
from my testing on am437x

Regards,
Keerthy

>>>
>>> And at least am437x based devices with pixcir_ts will fail to resume
>>> to a touchscreen that is configured as the wakeup-source in device
>>> tree for these devices.
>>>
>>> This is because pixcir_ts tries to reconfigure it's registers for
>>> noirq suspend which fails. This also leaves i2c-omap in enabled state
>>> for suspend.
>>
>> I didn't fully get this. Does it reconfigure the registers for noirq or
>> during noirq? If the former, why exactly does it fail?
> 
> On suspend and resume, pixcir_ts tries to configure registers
> during noirq. At that point we already have PM runtime
> rpm_check_suspend_allowed() return -EACCESS for PM runtime
> which leads to the pixcir -ETIMEDOUT 110 error from the i2c
> controller.
> 
> And in general, I don't think we should attempt any i2c during
> noirq :)
> 
>>> Let's fix the pixcir_ts issue and make sure i2c-omap is suspended by
>>> adding SET_NOIRQ_SYSTEM_SLEEP_PM_OPS. In the long run the solution
>>> seems to be to move the handling of suspended adapters to the i2c
>>> core, but that still needs some more work.
>>
>> I don't know if you followed this, but we failed with a generic handling
>> within the core. We have helpers now which drivers can apply
>> individually.
> 
> Hmm I don't follow you you here. Which helpers are you
> talking about here?
> 
> I'm only aware of your pending patches from thread
> "[PATCH v2 0/9] i2c: move handling of suspended adapters
> to the core".
> 
> Regards,
> 
> Tony
>
Tony Lindgren Jan. 11, 2019, 3:36 p.m. | #4
Hi,

* Keerthy <j-keerthy@ti.com> [190111 10:05]:
> On Friday 11 January 2019 05:15 AM, Tony Lindgren wrote:
> > * Wolfram Sang <wsa@the-dreams.de> [190110 23:31]:
> >> Hi Tony,
> >>
> >> On Thu, Jan 10, 2019 at 07:59:16AM -0800, Tony Lindgren wrote:
> >>> We currently get the following error with pixcir_ts driver during a
> >>> suspend resume cycle:
> >>>
> >>> omap_i2c 4802a000.i2c: controller timed out
> >>> pixcir_ts 1-005c: pixcir_int_enable: can't read reg 0x34 : -110
> >>> pixcir_ts 1-005c: Failed to disable interrupt generation: -110
> >>> pixcir_ts 1-005c: Failed to stop
> >>> dpm_run_callback(): pixcir_i2c_ts_resume+0x0/0x98
> >>> [pixcir_i2c_ts] returns -110
> >>> PM: Device 1-005c failed to resume: error -110
> 
> I spent some time today bisecting the issue. Unfortunately could not get to a
> proper commit but i believe this error got introduced after the ti,sysc
> migration series while i started bisecting it am437x stopped booting. May be it
> would worth looking at the dt nodes after sysc migration.

Hmm care to post the bisect commit that was not booting for you?
Would be good to know.

> This patch surely solves that error but may be if you can look at the migration
> patches a bit. So this error starts appearing in 5.0 only. 4.20 has no issues
> from my testing on am437x

We have legacy _od_suspend_noirq() do pm_generic_suspend_noirq(),
and on errors call pm_generic_runtime_suspend() anyways to idle
the module. So that explains how i2c-omap gets idled earlier.

With i2c-omap probing with ti-sysc, we can idle i2c-omap simply
with SET_NOIRQ_SYSTEM_SLEEP_PM_OPS in the driver and have the
generic code take care of things for us.

Currently we don't have anything in place to idle i2c-omap for us.
There's no _od_suspend_noirq() or SET_NOIRQ_SYSTEM_SLEEP_PM_OPS to
idle i2-omap. So we start seeing the pixcir errors when it tries
to access i2c-bus but it's too late at that point.

We could go back to _od_suspend_noirq() by moving i2c out of DEBUG in
sysc_revision_quirks and tag it with SYSC_QUIRK_LEGACY_IDLE. But I'd
rather limit SYSC_QUIRK_LEGACY_IDLE usage to the remaining cases of
pm_runtime_irq_safe_use(). And then we can eventually get rid of
SYSC_QUIRK_LEGACY_IDLE too.

Regards,

Tony
Wolfram Sang Jan. 12, 2019, 10:50 a.m. | #5
> On suspend and resume, pixcir_ts tries to configure registers
> during noirq. At that point we already have PM runtime
> rpm_check_suspend_allowed() return -EACCESS for PM runtime
> which leads to the pixcir -ETIMEDOUT 110 error from the i2c
> controller.
> 
> And in general, I don't think we should attempt any i2c during
> noirq :)

I totally agree. But wouldn't the proper solution be to fix the
pixcir_ts driver to not do transfers during noirq in this specific case?

Recalling your original patch from last year, you mentioned RTC devices.
So, another reason for this patch is to enforce suspending the adapter,
even when there is a child device connected which is for good reason not
going to be suspended?

This is to make sure I understand all this correctly.

> > I don't know if you followed this, but we failed with a generic handling
> > within the core. We have helpers now which drivers can apply
> > individually.
> 
> Hmm I don't follow you you here. Which helpers are you
> talking about here?
> 
> I'm only aware of your pending patches from thread
> "[PATCH v2 0/9] i2c: move handling of suspended adapters
> to the core".

Yes, but the v2 approach caused problems, see the discussion there. I
merged v1 (with helpers) a few days ago:

[PATCH 00/10] i2c: move handling of suspended adapters to the core
Tony Lindgren Jan. 13, 2019, 4:23 p.m. | #6
* Wolfram Sang <wsa@the-dreams.de> [190112 10:50]:
> 
> > On suspend and resume, pixcir_ts tries to configure registers
> > during noirq. At that point we already have PM runtime
> > rpm_check_suspend_allowed() return -EACCESS for PM runtime
> > which leads to the pixcir -ETIMEDOUT 110 error from the i2c
> > controller.
> > 
> > And in general, I don't think we should attempt any i2c during
> > noirq :)
> 
> I totally agree. But wouldn't the proper solution be to fix the
> pixcir_ts driver to not do transfers during noirq in this specific case?

Well AFAIK, pixcir_ts is doing the right thing. But because of the
currently missing pm_generic_runtime_suspend() that's been done
earlier in _od_suspend_noirq(), pixcir_ts now ends up trying to
configure things too late.

> Recalling your original patch from last year, you mentioned RTC devices.
> So, another reason for this patch is to enforce suspending the adapter,
> even when there is a child device connected which is for good reason not
> going to be suspended?

Hmm not sure if I've seen this happen with RTC yet. We just do what
pm_generic_runtime_suspend() does, returning -EBUSY is fine too.
Then the controller will just stay powered during the suspend if
necessary.

For a non-i2c example, we have devices where a gpio instance powers
the DDR memory :) In that case the related gpio instance just stays
stays powered during the suspend.

> This is to make sure I understand all this correctly.

Maybe the commit message should mention the currently missing
pm_generic_runtime_suspend() if that might clarify things.

> > > I don't know if you followed this, but we failed with a generic handling
> > > within the core. We have helpers now which drivers can apply
> > > individually.
> > 
> > Hmm I don't follow you you here. Which helpers are you
> > talking about here?
> > 
> > I'm only aware of your pending patches from thread
> > "[PATCH v2 0/9] i2c: move handling of suspended adapters
> > to the core".
> 
> Yes, but the v2 approach caused problems, see the discussion there. I
> merged v1 (with helpers) a few days ago:
> 
> [PATCH 00/10] i2c: move handling of suspended adapters to the core

Oh OK great, I'll take a look.

Regards,

Tony
Keerthy Jan. 14, 2019, 6:30 a.m. | #7
On Friday 11 January 2019 09:06 PM, Tony Lindgren wrote:
> Hi,
> 
> * Keerthy <j-keerthy@ti.com> [190111 10:05]:
>> On Friday 11 January 2019 05:15 AM, Tony Lindgren wrote:
>>> * Wolfram Sang <wsa@the-dreams.de> [190110 23:31]:
>>>> Hi Tony,
>>>>
>>>> On Thu, Jan 10, 2019 at 07:59:16AM -0800, Tony Lindgren wrote:
>>>>> We currently get the following error with pixcir_ts driver during a
>>>>> suspend resume cycle:
>>>>>
>>>>> omap_i2c 4802a000.i2c: controller timed out
>>>>> pixcir_ts 1-005c: pixcir_int_enable: can't read reg 0x34 : -110
>>>>> pixcir_ts 1-005c: Failed to disable interrupt generation: -110
>>>>> pixcir_ts 1-005c: Failed to stop
>>>>> dpm_run_callback(): pixcir_i2c_ts_resume+0x0/0x98
>>>>> [pixcir_i2c_ts] returns -110
>>>>> PM: Device 1-005c failed to resume: error -110
>>
>> I spent some time today bisecting the issue. Unfortunately could not get to a
>> proper commit but i believe this error got introduced after the ti,sysc
>> migration series while i started bisecting it am437x stopped booting. May be it
>> would worth looking at the dt nodes after sysc migration.
> 
> Hmm care to post the bisect commit that was not booting for you?
> Would be good to know.

https://pastebin.ubuntu.com/p/Tc7WGVJ7hT/

commit b5f8ffbb6fad9151634805c2001af4afbb884eca

I wanted to eliminate ARM: dts: am437x: Add l4 interconnect hierarchy and
ti-sysc data

to verify if it started erring after ti,sysc migration i could not boot
to prompt.

- Keerthy
> 
>> This patch surely solves that error but may be if you can look at the migration
>> patches a bit. So this error starts appearing in 5.0 only. 4.20 has no issues
>> from my testing on am437x
> 
> We have legacy _od_suspend_noirq() do pm_generic_suspend_noirq(),
> and on errors call pm_generic_runtime_suspend() anyways to idle
> the module. So that explains how i2c-omap gets idled earlier.

Okay

> 
> With i2c-omap probing with ti-sysc, we can idle i2c-omap simply
> with SET_NOIRQ_SYSTEM_SLEEP_PM_OPS in the driver and have the
> generic code take care of things for us.
> 
> Currently we don't have anything in place to idle i2c-omap for us.
> There's no _od_suspend_noirq() or SET_NOIRQ_SYSTEM_SLEEP_PM_OPS to
> idle i2-omap. So we start seeing the pixcir errors when it tries
> to access i2c-bus but it's too late at that point.

Agreed. So here is the suspend log with some prints added to i2c-omap runtime
ops and Pixcir driver:

[   90.855206] PM: suspend entry (deep)
[   90.860125] PM: Syncing filesystems ... done.
[   90.920632] Freezing user space processes ... (elapsed 0.003 seconds) done.
[   90.932245] OOM killer disabled.
[   90.935668] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
[   90.946227] printk: Suspending console(s) (use no_console_suspend to debug)
[   91.007332] pixcir_ts 1-005c: pixcir_i2c_ts_suspend called
[   91.008492] omap_i2c 4802a000.i2c: omap_i2c_runtime_resume

/* omap_i2c_runtime_resume is called before suspending but no corresponding
omap_i2c_runtime_suspend before suspending */

[   91.080420] cpsw 4a100000.ethernet eth0: Link is Down
[   91.194714] Disabling non-boot CPUs ...
[   91.194892] pm33xx pm33xx: PM: Successfully put all powerdomains to target state
[   91.257081] net eth0: initializing cpsw version 1.15 (0)
[   91.276247] Micrel KSZ9031 Gigabit PHY 4a101000.mdio:00: attached PHY driver
[Micrel KSZ9031 Gigabit PHY] (mii_bus:phy_addr=4a101000.mdio:0)

/* So basically we are trying to resume pixcir and i2c has not even suspended
properly */

[   91.294897] pixcir_ts 1-005c: pixcir_i2c_ts_resume called
[   92.342825] omap_i2c 4802a000.i2c: controller timed out
[   92.372732] pixcir_ts 1-005c: pixcir_int_enable: can't read reg 0x34 : -110
[   92.372779] pixcir_ts 1-005c: Failed to disable interrupt generation: -110
[   92.372815] pixcir_ts 1-005c: Failed to stop
[   92.372900] dpm_run_callback(): pixcir_i2c_ts_resume+0x0/0xc0
[pixcir_i2c_ts] returns -110
[   92.372951] PM: Device 1-005c failed to resume: error -110
[   92.386149] usb usb1: root hub lost power or was reset
[   92.386312] usb usb2: root hub lost power or was reset
[   92.650326] OOM killer enabled.
[   92.653632] Restarting tasks ... done.
[   92.693415] PM: suspend exit


> 
> We could go back to _od_suspend_noirq() by moving i2c out of DEBUG in
> sysc_revision_quirks and tag it with SYSC_QUIRK_LEGACY_IDLE. But I'd
> rather limit SYSC_QUIRK_LEGACY_IDLE usage to the remaining cases of
> pm_runtime_irq_safe_use(). And then we can eventually get rid of
> SYSC_QUIRK_LEGACY_IDLE too.

Yea no need to get back to LEGACY_IDLE mode i believe.

> 
> Regards,
> 
> Tony
>
Tony Lindgren Jan. 15, 2019, 4:17 p.m. | #8
* Keerthy <j-keerthy@ti.com> [190114 06:30]:
> 
> 
> On Friday 11 January 2019 09:06 PM, Tony Lindgren wrote:
> > Hi,
> > 
> > * Keerthy <j-keerthy@ti.com> [190111 10:05]:
> >> On Friday 11 January 2019 05:15 AM, Tony Lindgren wrote:
> >>> * Wolfram Sang <wsa@the-dreams.de> [190110 23:31]:
> >>>> Hi Tony,
> >>>>
> >>>> On Thu, Jan 10, 2019 at 07:59:16AM -0800, Tony Lindgren wrote:
> >>>>> We currently get the following error with pixcir_ts driver during a
> >>>>> suspend resume cycle:
> >>>>>
> >>>>> omap_i2c 4802a000.i2c: controller timed out
> >>>>> pixcir_ts 1-005c: pixcir_int_enable: can't read reg 0x34 : -110
> >>>>> pixcir_ts 1-005c: Failed to disable interrupt generation: -110
> >>>>> pixcir_ts 1-005c: Failed to stop
> >>>>> dpm_run_callback(): pixcir_i2c_ts_resume+0x0/0x98
> >>>>> [pixcir_i2c_ts] returns -110
> >>>>> PM: Device 1-005c failed to resume: error -110
> >>
> >> I spent some time today bisecting the issue. Unfortunately could not get to a
> >> proper commit but i believe this error got introduced after the ti,sysc
> >> migration series while i started bisecting it am437x stopped booting. May be it
> >> would worth looking at the dt nodes after sysc migration.
> > 
> > Hmm care to post the bisect commit that was not booting for you?
> > Would be good to know.
> 
> https://pastebin.ubuntu.com/p/Tc7WGVJ7hT/
> 
> commit b5f8ffbb6fad9151634805c2001af4afbb884eca
> 
> I wanted to eliminate ARM: dts: am437x: Add l4 interconnect hierarchy and
> ti-sysc data
>
> to verify if it started erring after ti,sysc migration i could not boot
> to prompt.

OK seems like some later fix is needed to carry through the bisect
there then. We have all the critical clock and driver fixes merged
since v4.19. But looks like git bisect threw you back into v4.19-rc1
as it took few merge windows to get all this clkctrl and sysc stuff
merged:

$ git show b5f8ffbb6fad9151:Makefile | head -n5
# SPDX-License-Identifier: GPL-2.0
VERSION = 4
PATCHLEVEL = 19
SUBLEVEL = 0
EXTRAVERSION = -rc1

> >> This patch surely solves that error but may be if you can look at the migration
> >> patches a bit. So this error starts appearing in 5.0 only. 4.20 has no issues
> >> from my testing on am437x
> > 
> > We have legacy _od_suspend_noirq() do pm_generic_suspend_noirq(),
> > and on errors call pm_generic_runtime_suspend() anyways to idle
> > the module. So that explains how i2c-omap gets idled earlier.
> 
> Okay
> 
> > 
> > With i2c-omap probing with ti-sysc, we can idle i2c-omap simply
> > with SET_NOIRQ_SYSTEM_SLEEP_PM_OPS in the driver and have the
> > generic code take care of things for us.
> > 
> > Currently we don't have anything in place to idle i2c-omap for us.
> > There's no _od_suspend_noirq() or SET_NOIRQ_SYSTEM_SLEEP_PM_OPS to
> > idle i2-omap. So we start seeing the pixcir errors when it tries
> > to access i2c-bus but it's too late at that point.
> 
> Agreed. So here is the suspend log with some prints added to i2c-omap runtime
> ops and Pixcir driver:
> 
> [   90.855206] PM: suspend entry (deep)
> [   90.860125] PM: Syncing filesystems ... done.
> [   90.920632] Freezing user space processes ... (elapsed 0.003 seconds) done.
> [   90.932245] OOM killer disabled.
> [   90.935668] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
> [   90.946227] printk: Suspending console(s) (use no_console_suspend to debug)
> [   91.007332] pixcir_ts 1-005c: pixcir_i2c_ts_suspend called
> [   91.008492] omap_i2c 4802a000.i2c: omap_i2c_runtime_resume
> 
> /* omap_i2c_runtime_resume is called before suspending but no corresponding
> omap_i2c_runtime_suspend before suspending */
> 
> [   91.080420] cpsw 4a100000.ethernet eth0: Link is Down
> [   91.194714] Disabling non-boot CPUs ...
> [   91.194892] pm33xx pm33xx: PM: Successfully put all powerdomains to target state
> [   91.257081] net eth0: initializing cpsw version 1.15 (0)
> [   91.276247] Micrel KSZ9031 Gigabit PHY 4a101000.mdio:00: attached PHY driver
> [Micrel KSZ9031 Gigabit PHY] (mii_bus:phy_addr=4a101000.mdio:0)
> 
> /* So basically we are trying to resume pixcir and i2c has not even suspended
> properly */
> 
> [   91.294897] pixcir_ts 1-005c: pixcir_i2c_ts_resume called
> [   92.342825] omap_i2c 4802a000.i2c: controller timed out
> [   92.372732] pixcir_ts 1-005c: pixcir_int_enable: can't read reg 0x34 : -110
> [   92.372779] pixcir_ts 1-005c: Failed to disable interrupt generation: -110
> [   92.372815] pixcir_ts 1-005c: Failed to stop
> [   92.372900] dpm_run_callback(): pixcir_i2c_ts_resume+0x0/0xc0
> [pixcir_i2c_ts] returns -110
> [   92.372951] PM: Device 1-005c failed to resume: error -110
> [   92.386149] usb usb1: root hub lost power or was reset
> [   92.386312] usb usb2: root hub lost power or was reset
> [   92.650326] OOM killer enabled.
> [   92.653632] Restarting tasks ... done.
> [   92.693415] PM: suspend exit

Yeah, thanks for confirming it on your hardware. I have
slightly different hardware with a different touchscreen
controller so I've been only able to emulate the issue.

Thanks,

Tony
Tony Lindgren Feb. 4, 2019, 8:29 p.m. | #9
* Tony Lindgren <tony@atomide.com> [190113 16:23]:
> * Wolfram Sang <wsa@the-dreams.de> [190112 10:50]:
> > 
> > > On suspend and resume, pixcir_ts tries to configure registers
> > > during noirq. At that point we already have PM runtime
> > > rpm_check_suspend_allowed() return -EACCESS for PM runtime
> > > which leads to the pixcir -ETIMEDOUT 110 error from the i2c
> > > controller.
> > > 
> > > And in general, I don't think we should attempt any i2c during
> > > noirq :)
> > 
> > I totally agree. But wouldn't the proper solution be to fix the
> > pixcir_ts driver to not do transfers during noirq in this specific case?
> 
> Well AFAIK, pixcir_ts is doing the right thing. But because of the
> currently missing pm_generic_runtime_suspend() that's been done
> earlier in _od_suspend_noirq(), pixcir_ts now ends up trying to
> configure things too late.
> 
> > Recalling your original patch from last year, you mentioned RTC devices.
> > So, another reason for this patch is to enforce suspending the adapter,
> > even when there is a child device connected which is for good reason not
> > going to be suspended?
> 
> Hmm not sure if I've seen this happen with RTC yet. We just do what
> pm_generic_runtime_suspend() does, returning -EBUSY is fine too.
> Then the controller will just stay powered during the suspend if
> necessary.
> 
> For a non-i2c example, we have devices where a gpio instance powers
> the DDR memory :) In that case the related gpio instance just stays
> stays powered during the suspend.
> 
> > This is to make sure I understand all this correctly.
> 
> Maybe the commit message should mention the currently missing
> pm_generic_runtime_suspend() if that might clarify things.
> 
> > > > I don't know if you followed this, but we failed with a generic handling
> > > > within the core. We have helpers now which drivers can apply
> > > > individually.
> > > 
> > > Hmm I don't follow you you here. Which helpers are you
> > > talking about here?
> > > 
> > > I'm only aware of your pending patches from thread
> > > "[PATCH v2 0/9] i2c: move handling of suspended adapters
> > > to the core".
> > 
> > Yes, but the v2 approach caused problems, see the discussion there. I
> > merged v1 (with helpers) a few days ago:
> > 
> > [PATCH 00/10] i2c: move handling of suspended adapters to the core
> 
> Oh OK great, I'll take a look.

So we don't have any suspend state flag for i2c-omap, and
this patch is still needed as a fix. Do you need any changes
done?

BTW, you already have another similar fix queued as 81d696c7c4ff
("i2c: rcar: Fix clients using i2c from suspend callback") so
it's not like it's a unique problem :)

Regards,

Tony
Vignesh Raghavendra Feb. 5, 2019, 6:22 a.m. | #10
On 10/01/19 9:29 PM, Tony Lindgren wrote:
> We currently get the following error with pixcir_ts driver during a
> suspend resume cycle:
> 
> omap_i2c 4802a000.i2c: controller timed out
> pixcir_ts 1-005c: pixcir_int_enable: can't read reg 0x34 : -110
> pixcir_ts 1-005c: Failed to disable interrupt generation: -110
> pixcir_ts 1-005c: Failed to stop
> dpm_run_callback(): pixcir_i2c_ts_resume+0x0/0x98
> [pixcir_i2c_ts] returns -110
> PM: Device 1-005c failed to resume: error -110
> 
> And at least am437x based devices with pixcir_ts will fail to resume
> to a touchscreen that is configured as the wakeup-source in device
> tree for these devices.
> 
> This is because pixcir_ts tries to reconfigure it's registers for
> noirq suspend which fails. This also leaves i2c-omap in enabled state
> for suspend.
> 
> Let's fix the pixcir_ts issue and make sure i2c-omap is suspended by
> adding SET_NOIRQ_SYSTEM_SLEEP_PM_OPS. In the long run the solution
> seems to be to move the handling of suspended adapters to the i2c
> core, but that still needs some more work.
> 
> Let's also get rid of some ifdefs while at it and replace them with
> __maybe_unused as SET_RUNTIME_PM_OPS and SET_NOIRQ_SYSTEM_SLEEP_PM_OPS
> already deal with the various PM Kconfig options.
> 
> Cc: Dave Gerlach <d-gerlach@ti.com>
> Cc: Grygorii Strashko <grygorii.strashko@ti.com>
> Cc: Keerthy <j-keerthy@ti.com>
> Cc: Tero Kristo <t-kristo@ti.com>
> Reported-by: Keerthy <j-keerthy@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
> 

Acked-by: Vignesh R <vigneshr@ti.com>

Regards
Vignesh

> Wolfram, this is still pending, can you please apply this a fix for
> the -rc series if no objections?
> 
> Changes since v1:
> 
> - Updated comments with the pixcir_ts related error messages
> 
> ---
>  drivers/i2c/busses/i2c-omap.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -1500,8 +1500,7 @@ static int omap_i2c_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -#ifdef CONFIG_PM
> -static int omap_i2c_runtime_suspend(struct device *dev)
> +static int __maybe_unused omap_i2c_runtime_suspend(struct device *dev)
>  {
>  	struct omap_i2c_dev *omap = dev_get_drvdata(dev);
>  
> @@ -1527,7 +1526,7 @@ static int omap_i2c_runtime_suspend(struct device *dev)
>  	return 0;
>  }
>  
> -static int omap_i2c_runtime_resume(struct device *dev)
> +static int __maybe_unused omap_i2c_runtime_resume(struct device *dev)
>  {
>  	struct omap_i2c_dev *omap = dev_get_drvdata(dev);
>  
> @@ -1542,20 +1541,18 @@ static int omap_i2c_runtime_resume(struct device *dev)
>  }
>  
>  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_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
>  			   omap_i2c_runtime_resume, NULL)
>  };
> -#define OMAP_I2C_PM_OPS (&omap_i2c_pm_ops)
> -#else
> -#define OMAP_I2C_PM_OPS NULL
> -#endif /* CONFIG_PM */
>  
>  static struct platform_driver omap_i2c_driver = {
>  	.probe		= omap_i2c_probe,
>  	.remove		= omap_i2c_remove,
>  	.driver		= {
>  		.name	= "omap_i2c",
> -		.pm	= OMAP_I2C_PM_OPS,
> +		.pm	= &omap_i2c_pm_ops,
>  		.of_match_table = of_match_ptr(omap_i2c_of_match),
>  	},
>  };
>
Wolfram Sang Feb. 5, 2019, 12:15 p.m. | #11
On Thu, Jan 10, 2019 at 07:59:16AM -0800, Tony Lindgren wrote:
> We currently get the following error with pixcir_ts driver during a
> suspend resume cycle:
> 
> omap_i2c 4802a000.i2c: controller timed out
> pixcir_ts 1-005c: pixcir_int_enable: can't read reg 0x34 : -110
> pixcir_ts 1-005c: Failed to disable interrupt generation: -110
> pixcir_ts 1-005c: Failed to stop
> dpm_run_callback(): pixcir_i2c_ts_resume+0x0/0x98
> [pixcir_i2c_ts] returns -110
> PM: Device 1-005c failed to resume: error -110
> 
> And at least am437x based devices with pixcir_ts will fail to resume
> to a touchscreen that is configured as the wakeup-source in device
> tree for these devices.
> 
> This is because pixcir_ts tries to reconfigure it's registers for
> noirq suspend which fails. This also leaves i2c-omap in enabled state
> for suspend.
> 
> Let's fix the pixcir_ts issue and make sure i2c-omap is suspended by
> adding SET_NOIRQ_SYSTEM_SLEEP_PM_OPS. In the long run the solution
> seems to be to move the handling of suspended adapters to the i2c
> core, but that still needs some more work.
> 
> Let's also get rid of some ifdefs while at it and replace them with
> __maybe_unused as SET_RUNTIME_PM_OPS and SET_NOIRQ_SYSTEM_SLEEP_PM_OPS
> already deal with the various PM Kconfig options.
> 
> Cc: Dave Gerlach <d-gerlach@ti.com>
> Cc: Grygorii Strashko <grygorii.strashko@ti.com>
> Cc: Keerthy <j-keerthy@ti.com>
> Cc: Tero Kristo <t-kristo@ti.com>
> Reported-by: Keerthy <j-keerthy@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Applied to for-current, thanks!
Andreas Kemnade Feb. 21, 2019, 9:04 p.m. | #12
On Tue, 5 Feb 2019 13:15:05 +0100
Wolfram Sang <wsa@the-dreams.de> wrote:

> On Thu, Jan 10, 2019 at 07:59:16AM -0800, Tony Lindgren wrote:
> > We currently get the following error with pixcir_ts driver during a
> > suspend resume cycle:
> > 
> > omap_i2c 4802a000.i2c: controller timed out
> > pixcir_ts 1-005c: pixcir_int_enable: can't read reg 0x34 : -110
> > pixcir_ts 1-005c: Failed to disable interrupt generation: -110
> > pixcir_ts 1-005c: Failed to stop
> > dpm_run_callback(): pixcir_i2c_ts_resume+0x0/0x98
> > [pixcir_i2c_ts] returns -110
> > PM: Device 1-005c failed to resume: error -110
> > 
> > And at least am437x based devices with pixcir_ts will fail to resume
> > to a touchscreen that is configured as the wakeup-source in device
> > tree for these devices.
> > 
> > This is because pixcir_ts tries to reconfigure it's registers for
> > noirq suspend which fails. This also leaves i2c-omap in enabled state
> > for suspend.
> > 
> > Let's fix the pixcir_ts issue and make sure i2c-omap is suspended by
> > adding SET_NOIRQ_SYSTEM_SLEEP_PM_OPS. In the long run the solution
> > seems to be to move the handling of suspended adapters to the i2c
> > core, but that still needs some more work.
> > 
> > Let's also get rid of some ifdefs while at it and replace them with
> > __maybe_unused as SET_RUNTIME_PM_OPS and SET_NOIRQ_SYSTEM_SLEEP_PM_OPS
> > already deal with the various PM Kconfig options.
> > 
> > Cc: Dave Gerlach <d-gerlach@ti.com>
> > Cc: Grygorii Strashko <grygorii.strashko@ti.com>
> > Cc: Keerthy <j-keerthy@ti.com>
> > Cc: Tero Kristo <t-kristo@ti.com>
> > Reported-by: Keerthy <j-keerthy@ti.com>
> > Signed-off-by: Tony Lindgren <tony@atomide.com>  
> 
> Applied to for-current, thanks!
> 
this one breaks my system: gta04 (dm3730 + twl4030)
loaded a minimal set of things:

root@(none):/# rtcwake -s 10 -m mem
rtcwake: wakeup from "mem" using /dev/rtc0 at Sat Jan  1 00:04:01 2000
[   50.857360] PM: suspend entry (deep)
[   50.861480] PM: Syncing filesystems ... done.
[   50.881561] Freezing user space processes ... (elapsed 0.003 seconds) done.
[   50.893493] OOM killer disabled.
[   50.896911] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
[   50.908050] printk: Suspending console(s) (use no_console_suspend to debug)
[   50.978393] Disabling non-boot CPUs ...
[   50.978485] Successfully put all powerdomains to target state
[   50.986816] twl: Read failed (mod 1, reg 0x01 count 1)
[   50.986846] twl4030: I2C error -13 reading PIH ISR
[   50.986907] twl: Read failed (mod 1, reg 0x01 count 1)
[   50.986907] twl4030: I2C error -13 reading PIH ISR
[   50.986968] twl: Read failed (mod 1, reg 0x01 count 1)
[   50.986968] twl4030: I2C error -13 reading PIH ISR
[   50.987030] twl: Read failed (mod 1, reg 0x01 count 1)


Regards,
Andreas
Tony Lindgren Feb. 21, 2019, 9:26 p.m. | #13
* Andreas Kemnade <andreas@kemnade.info> [190221 21:05]:
> this one breaks my system: gta04 (dm3730 + twl4030)
> loaded a minimal set of things:
> 
> root@(none):/# rtcwake -s 10 -m mem
> rtcwake: wakeup from "mem" using /dev/rtc0 at Sat Jan  1 00:04:01 2000
> [   50.857360] PM: suspend entry (deep)
> [   50.861480] PM: Syncing filesystems ... done.
> [   50.881561] Freezing user space processes ... (elapsed 0.003 seconds) done.
> [   50.893493] OOM killer disabled.
> [   50.896911] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
> [   50.908050] printk: Suspending console(s) (use no_console_suspend to debug)
> [   50.978393] Disabling non-boot CPUs ...
> [   50.978485] Successfully put all powerdomains to target state
> [   50.986816] twl: Read failed (mod 1, reg 0x01 count 1)
> [   50.986846] twl4030: I2C error -13 reading PIH ISR
> [   50.986907] twl: Read failed (mod 1, reg 0x01 count 1)
> [   50.986907] twl4030: I2C error -13 reading PIH ISR
> [   50.986968] twl: Read failed (mod 1, reg 0x01 count 1)
> [   50.986968] twl4030: I2C error -13 reading PIH ISR
> [   50.987030] twl: Read failed (mod 1, reg 0x01 count 1)

Interesting, sorry about that. I'm not seeing here with dm3730 + twl4030
logicpd torpedo:

[   79.150573] PM: suspend entry (deep)
[   79.154235] PM: Syncing filesystems ... done.
[   79.161437] Freezing user space processes ... (elapsed 0.002 seconds) done.
[   79.170898] OOM killer disabled.
[   79.174163] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
[   79.183685] printk: Suspending console(s) (use no_console_suspend to debug)
[   79.231964] Disabling non-boot CPUs ...
[   79.231994] Successfully put all powerdomains to target state
[   79.271209] OOM killer enabled.
[   79.274383] Restarting tasks ... done.
[   79.284088] PM: suspend exit

I wonder what code is trying to do the twl i2c reads that late
into suspend?

Regards,

Tony
Tony Lindgren Feb. 21, 2019, 9:41 p.m. | #14
* Tony Lindgren <tony@atomide.com> [190221 21:26]:
> * Andreas Kemnade <andreas@kemnade.info> [190221 21:05]:
> > this one breaks my system: gta04 (dm3730 + twl4030)
> > loaded a minimal set of things:
> > 
> > root@(none):/# rtcwake -s 10 -m mem
> > rtcwake: wakeup from "mem" using /dev/rtc0 at Sat Jan  1 00:04:01 2000
> > [   50.857360] PM: suspend entry (deep)
> > [   50.861480] PM: Syncing filesystems ... done.
> > [   50.881561] Freezing user space processes ... (elapsed 0.003 seconds) done.
> > [   50.893493] OOM killer disabled.
> > [   50.896911] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
> > [   50.908050] printk: Suspending console(s) (use no_console_suspend to debug)
> > [   50.978393] Disabling non-boot CPUs ...
> > [   50.978485] Successfully put all powerdomains to target state
> > [   50.986816] twl: Read failed (mod 1, reg 0x01 count 1)
> > [   50.986846] twl4030: I2C error -13 reading PIH ISR
> > [   50.986907] twl: Read failed (mod 1, reg 0x01 count 1)
> > [   50.986907] twl4030: I2C error -13 reading PIH ISR
> > [   50.986968] twl: Read failed (mod 1, reg 0x01 count 1)
> > [   50.986968] twl4030: I2C error -13 reading PIH ISR
> > [   50.987030] twl: Read failed (mod 1, reg 0x01 count 1)
> 
> Interesting, sorry about that. I'm not seeing here with dm3730 + twl4030
> logicpd torpedo:
> 
> [   79.150573] PM: suspend entry (deep)
> [   79.154235] PM: Syncing filesystems ... done.
> [   79.161437] Freezing user space processes ... (elapsed 0.002 seconds) done.
> [   79.170898] OOM killer disabled.
> [   79.174163] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
> [   79.183685] printk: Suspending console(s) (use no_console_suspend to debug)
> [   79.231964] Disabling non-boot CPUs ...
> [   79.231994] Successfully put all powerdomains to target state
> [   79.271209] OOM killer enabled.
> [   79.274383] Restarting tasks ... done.
> [   79.284088] PM: suspend exit

I'm not seeing the issue here with omap3-evm or n900 either.

> I wonder what code is trying to do the twl i2c reads that late
> into suspend?

I tried with just plain omap2plus_defconfig, do you have maybe
something extra enabled that I don't?

Regards,

Tony
Andreas Kemnade Feb. 21, 2019, 9:44 p.m. | #15
On Thu, 21 Feb 2019 13:26:03 -0800
Tony Lindgren <tony@atomide.com> wrote:

> * Andreas Kemnade <andreas@kemnade.info> [190221 21:05]:
> > this one breaks my system: gta04 (dm3730 + twl4030)
> > loaded a minimal set of things:
> > 
> > root@(none):/# rtcwake -s 10 -m mem
> > rtcwake: wakeup from "mem" using /dev/rtc0 at Sat Jan  1 00:04:01 2000
> > [   50.857360] PM: suspend entry (deep)
> > [   50.861480] PM: Syncing filesystems ... done.
> > [   50.881561] Freezing user space processes ... (elapsed 0.003 seconds) done.
> > [   50.893493] OOM killer disabled.
> > [   50.896911] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
> > [   50.908050] printk: Suspending console(s) (use no_console_suspend to debug)
> > [   50.978393] Disabling non-boot CPUs ...
> > [   50.978485] Successfully put all powerdomains to target state
> > [   50.986816] twl: Read failed (mod 1, reg 0x01 count 1)
> > [   50.986846] twl4030: I2C error -13 reading PIH ISR
> > [   50.986907] twl: Read failed (mod 1, reg 0x01 count 1)
> > [   50.986907] twl4030: I2C error -13 reading PIH ISR
> > [   50.986968] twl: Read failed (mod 1, reg 0x01 count 1)
> > [   50.986968] twl4030: I2C error -13 reading PIH ISR
> > [   50.987030] twl: Read failed (mod 1, reg 0x01 count 1)  
> 
> Interesting, sorry about that. I'm not seeing here with dm3730 + twl4030
> logicpd torpedo:
> 
> [   79.150573] PM: suspend entry (deep)
> [   79.154235] PM: Syncing filesystems ... done.
> [   79.161437] Freezing user space processes ... (elapsed 0.002 seconds) done.
> [   79.170898] OOM killer disabled.
> [   79.174163] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
> [   79.183685] printk: Suspending console(s) (use no_console_suspend to debug)
> [   79.231964] Disabling non-boot CPUs ...
> [   79.231994] Successfully put all powerdomains to target state
> [   79.271209] OOM killer enabled.
> [   79.274383] Restarting tasks ... done.
> [   79.284088] PM: suspend exit
> 
> I wonder what code is trying to do the twl i2c reads that late
> into suspend?
> 


well, I rather guess early after resume. These read failures come
endlessly. My theory is that:
- system is woken up by twl irq (here: rtc alarm)
- twl code tries to handle that by reading the irq registers
- read fails (because omap-i2c is in a non-working state)
- irq stays
- reading is retried but failed again.

Doing more research here:
The key point is to wake up via twl4030 irq (here the rtc alarm).
This does not happen when I am not waking up via twl irq
but via some other pinctl irq of the wkup domain.

Regards,
Andreas
Andreas Kemnade Feb. 21, 2019, 10:13 p.m. | #16
On Thu, 21 Feb 2019 13:41:57 -0800
Tony Lindgren <tony@atomide.com> wrote:

> * Tony Lindgren <tony@atomide.com> [190221 21:26]:
> > * Andreas Kemnade <andreas@kemnade.info> [190221 21:05]:  
> > > this one breaks my system: gta04 (dm3730 + twl4030)
> > > loaded a minimal set of things:
> > > 
> > > root@(none):/# rtcwake -s 10 -m mem
> > > rtcwake: wakeup from "mem" using /dev/rtc0 at Sat Jan  1 00:04:01 2000
> > > [   50.857360] PM: suspend entry (deep)
> > > [   50.861480] PM: Syncing filesystems ... done.
> > > [   50.881561] Freezing user space processes ... (elapsed 0.003 seconds) done.
> > > [   50.893493] OOM killer disabled.
> > > [   50.896911] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
> > > [   50.908050] printk: Suspending console(s) (use no_console_suspend to debug)
> > > [   50.978393] Disabling non-boot CPUs ...
> > > [   50.978485] Successfully put all powerdomains to target state
> > > [   50.986816] twl: Read failed (mod 1, reg 0x01 count 1)
> > > [   50.986846] twl4030: I2C error -13 reading PIH ISR
> > > [   50.986907] twl: Read failed (mod 1, reg 0x01 count 1)
> > > [   50.986907] twl4030: I2C error -13 reading PIH ISR
> > > [   50.986968] twl: Read failed (mod 1, reg 0x01 count 1)
> > > [   50.986968] twl4030: I2C error -13 reading PIH ISR
> > > [   50.987030] twl: Read failed (mod 1, reg 0x01 count 1)  
> > 
> > Interesting, sorry about that. I'm not seeing here with dm3730 + twl4030
> > logicpd torpedo:
> > 
> > [   79.150573] PM: suspend entry (deep)
> > [   79.154235] PM: Syncing filesystems ... done.
> > [   79.161437] Freezing user space processes ... (elapsed 0.002 seconds) done.
> > [   79.170898] OOM killer disabled.
> > [   79.174163] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
> > [   79.183685] printk: Suspending console(s) (use no_console_suspend to debug)
> > [   79.231964] Disabling non-boot CPUs ...
> > [   79.231994] Successfully put all powerdomains to target state
> > [   79.271209] OOM killer enabled.
> > [   79.274383] Restarting tasks ... done.
> > [   79.284088] PM: suspend exit  
> 
> I'm not seeing the issue here with omap3-evm or n900 either.
> 
> > I wonder what code is trying to do the twl i2c reads that late
> > into suspend?  
> 
> I tried with just plain omap2plus_defconfig, do you have maybe
> something extra enabled that I don't?
> 
A quick guess:
CONFIG_PREEMPT=y

my config:
https://misc.andi.de1.cc/mydef

omap2plus_defconfig works here also. Now I need some sleep.

Regards,
Andreas
Andreas Kemnade Feb. 21, 2019, 10:16 p.m. | #17
On Thu, 21 Feb 2019 22:04:45 +0100
Andreas Kemnade <andreas@kemnade.info> wrote:

> On Tue, 5 Feb 2019 13:15:05 +0100
> Wolfram Sang <wsa@the-dreams.de> wrote:
> 
> > On Thu, Jan 10, 2019 at 07:59:16AM -0800, Tony Lindgren wrote:  
> > > We currently get the following error with pixcir_ts driver during a
> > > suspend resume cycle:
> > > 
> > > omap_i2c 4802a000.i2c: controller timed out
> > > pixcir_ts 1-005c: pixcir_int_enable: can't read reg 0x34 : -110
> > > pixcir_ts 1-005c: Failed to disable interrupt generation: -110
> > > pixcir_ts 1-005c: Failed to stop
> > > dpm_run_callback(): pixcir_i2c_ts_resume+0x0/0x98
> > > [pixcir_i2c_ts] returns -110
> > > PM: Device 1-005c failed to resume: error -110
> > > 
> > > And at least am437x based devices with pixcir_ts will fail to resume
> > > to a touchscreen that is configured as the wakeup-source in device
> > > tree for these devices.
> > > 
> > > This is because pixcir_ts tries to reconfigure it's registers for
> > > noirq suspend which fails. This also leaves i2c-omap in enabled state
> > > for suspend.
> > > 
> > > Let's fix the pixcir_ts issue and make sure i2c-omap is suspended by
> > > adding SET_NOIRQ_SYSTEM_SLEEP_PM_OPS. In the long run the solution
> > > seems to be to move the handling of suspended adapters to the i2c
> > > core, but that still needs some more work.
> > > 
> > > Let's also get rid of some ifdefs while at it and replace them with
> > > __maybe_unused as SET_RUNTIME_PM_OPS and SET_NOIRQ_SYSTEM_SLEEP_PM_OPS
> > > already deal with the various PM Kconfig options.
> > > 
> > > Cc: Dave Gerlach <d-gerlach@ti.com>
> > > Cc: Grygorii Strashko <grygorii.strashko@ti.com>
> > > Cc: Keerthy <j-keerthy@ti.com>
> > > Cc: Tero Kristo <t-kristo@ti.com>
> > > Reported-by: Keerthy <j-keerthy@ti.com>
> > > Signed-off-by: Tony Lindgren <tony@atomide.com>    
> > 
> > Applied to for-current, thanks!
> >   
> this one breaks my system: gta04 (dm3730 + twl4030)
> loaded a minimal set of things:
> 
> root@(none):/# rtcwake -s 10 -m mem
> rtcwake: wakeup from "mem" using /dev/rtc0 at Sat Jan  1 00:04:01 2000
> [   50.857360] PM: suspend entry (deep)
> [   50.861480] PM: Syncing filesystems ... done.
> [   50.881561] Freezing user space processes ... (elapsed 0.003 seconds) done.
> [   50.893493] OOM killer disabled.
> [   50.896911] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
> [   50.908050] printk: Suspending console(s) (use no_console_suspend to debug)
> [   50.978393] Disabling non-boot CPUs ...
> [   50.978485] Successfully put all powerdomains to target state
> [   50.986816] twl: Read failed (mod 1, reg 0x01 count 1)
> [   50.986846] twl4030: I2C error -13 reading PIH ISR
> [   50.986907] twl: Read failed (mod 1, reg 0x01 count 1)
> [   50.986907] twl4030: I2C error -13 reading PIH ISR
> [   50.986968] twl: Read failed (mod 1, reg 0x01 count 1)
> [   50.986968] twl4030: I2C error -13 reading PIH ISR
> [   50.987030] twl: Read failed (mod 1, reg 0x01 count 1)
> 
was a bit more patient: after some minutes, resume finishes.

Regards,
Andreas
Tony Lindgren Feb. 21, 2019, 10:25 p.m. | #18
* Andreas Kemnade <andreas@kemnade.info> [190221 22:14]:
> On Thu, 21 Feb 2019 13:41:57 -0800
> Tony Lindgren <tony@atomide.com> wrote:
> > I tried with just plain omap2plus_defconfig, do you have maybe
> > something extra enabled that I don't?
> > 
> A quick guess:
> CONFIG_PREEMPT=y

Hmm I just tried adding that and it still works here.

> my config:
> https://misc.andi.de1.cc/mydef
> 
> omap2plus_defconfig works here also. Now I need some sleep.

Maybe you have a USB cable connected and the USB PHY is
producing interrupts on resume? My test devices are in a
rack so it's a bit hard for me to quickly test that.

Later,

Tony
Andreas Kemnade Feb. 21, 2019, 10:36 p.m. | #19
On Thu, 21 Feb 2019 14:25:32 -0800
Tony Lindgren <tony@atomide.com> wrote:

> * Andreas Kemnade <andreas@kemnade.info> [190221 22:14]:
> > On Thu, 21 Feb 2019 13:41:57 -0800
> > Tony Lindgren <tony@atomide.com> wrote:  
> > > I tried with just plain omap2plus_defconfig, do you have maybe
> > > something extra enabled that I don't?
> > >   
> > A quick guess:
> > CONFIG_PREEMPT=y  
> 
> Hmm I just tried adding that and it still works here.
> 
> > my config:
> > https://misc.andi.de1.cc/mydef
> > 
> > omap2plus_defconfig works here also. Now I need some sleep.  
> 
> Maybe you have a USB cable connected and the USB PHY is
> producing interrupts on resume? My test devices are in a
> rack so it's a bit hard for me to quickly test that.
> 
usb irqs would not explain why waking up via other pinctrl does not
cause these problems. phy modules are not loaded.

So, now really brain sleep time here, hopefully a resume with fresh
ideas.


Regards,
Andreas
Tony Lindgren Feb. 21, 2019, 10:39 p.m. | #20
* Andreas Kemnade <andreas@kemnade.info> [190221 22:37]:
> On Thu, 21 Feb 2019 14:25:32 -0800
> Tony Lindgren <tony@atomide.com> wrote:
> 
> > * Andreas Kemnade <andreas@kemnade.info> [190221 22:14]:
> > > On Thu, 21 Feb 2019 13:41:57 -0800
> > > Tony Lindgren <tony@atomide.com> wrote:  
> > > > I tried with just plain omap2plus_defconfig, do you have maybe
> > > > something extra enabled that I don't?
> > > >   
> > > A quick guess:
> > > CONFIG_PREEMPT=y  
> > 
> > Hmm I just tried adding that and it still works here.
> > 
> > > my config:
> > > https://misc.andi.de1.cc/mydef
> > > 
> > > omap2plus_defconfig works here also. Now I need some sleep.  
> > 
> > Maybe you have a USB cable connected and the USB PHY is
> > producing interrupts on resume? My test devices are in a
> > rack so it's a bit hard for me to quickly test that.
> > 
> usb irqs would not explain why waking up via other pinctrl does not
> cause these problems. phy modules are not loaded.
> 
> So, now really brain sleep time here, hopefully a resume with fresh
> ideas.

OK, good night. I booted your config with 8250 + nfsroot added
and I'm still unable to reproduce your issue here.

Regards,

Tony
Andreas Kemnade Feb. 22, 2019, 12:07 p.m. | #21
On Thu, 21 Feb 2019 14:39:55 -0800
Tony Lindgren <tony@atomide.com> wrote:

> * Andreas Kemnade <andreas@kemnade.info> [190221 22:37]:
> > On Thu, 21 Feb 2019 14:25:32 -0800
> > Tony Lindgren <tony@atomide.com> wrote:
> >   
> > > * Andreas Kemnade <andreas@kemnade.info> [190221 22:14]:  
> > > > On Thu, 21 Feb 2019 13:41:57 -0800
> > > > Tony Lindgren <tony@atomide.com> wrote:    
> > > > > I tried with just plain omap2plus_defconfig, do you have maybe
> > > > > something extra enabled that I don't?
> > > > >     
> > > > A quick guess:
> > > > CONFIG_PREEMPT=y    
> > > 
> > > Hmm I just tried adding that and it still works here.
> > >   
> > > > my config:
> > > > https://misc.andi.de1.cc/mydef
> > > > 
> > > > omap2plus_defconfig works here also. Now I need some sleep.    
> > > 
> > > Maybe you have a USB cable connected and the USB PHY is
> > > producing interrupts on resume? My test devices are in a
> > > rack so it's a bit hard for me to quickly test that.
> > >   
> > usb irqs would not explain why waking up via other pinctrl does not
> > cause these problems. phy modules are not loaded.
> > 
> > So, now really brain sleep time here, hopefully a resume with fresh
> > ideas.  
> 
> OK, good night. I booted your config with 8250 + nfsroot added
> and I'm still unable to reproduce your issue here.
> 
so you have more modules loaded than me when trying that.
Hmm, so what do we know:
- git bisect at least led to something interesting, not some unrelated thing.
    which might just cause timing differences.
- rtcwake + actual wakeup via rtc on my config = problem
- rtcwake + actual wakeup via some other pin in the wkup domain = no problem
- rtcwake + actual wakeup via powerbutton (twl4030-pwrbutton) = problem
- omap2plus_defconfig seems to work here
- no problems at other systems with dm3730 + twl4030 with my config
  and with omap2plus defconfig

I deduce that there happens something strange at resume independant
from what happend at suspend. Probably some timing issue.

Hmm, does "bisecting" omap2_defconfig and my config give some helpful
information? Besides of some key points like CONFIG_PREEMPT, I will probably
just find things which somehow influence timing a little, so I will probably
end up without anything interesting. Hopefully it will not just fix something.

I will try to add printks to find out in which order things happen.
We have
enable_irq_wake(irq_num);
in twl4030-irq.c: Does that has any interersting consequences? sys_nirq seems
to be equal on non-affected and affected systems (pinmux + twl irq config)

Regards,
Andreas
Tony Lindgren Feb. 22, 2019, 2:54 p.m. | #22
Hi,

* Andreas Kemnade <andreas@kemnade.info> [190222 12:08]:
> Hmm, so what do we know:
> - git bisect at least led to something interesting, not some unrelated thing.
>     which might just cause timing differences.

Yes seems it's some twl related driver that gets only enabled for gta04
with your .config.

> - rtcwake + actual wakeup via rtc on my config = problem
> - rtcwake + actual wakeup via some other pin in the wkup domain = no problem
> - rtcwake + actual wakeup via powerbutton (twl4030-pwrbutton) = problem
> - omap2plus_defconfig seems to work here
> - no problems at other systems with dm3730 + twl4030 with my config
>   and with omap2plus defconfig
> 
> I deduce that there happens something strange at resume independant
> from what happend at suspend. Probably some timing issue.

It's probaby some twl related code is (wrongly) trying to read/write
registers at noirq suspend level. Sounds like some driver tagged with
noirq suspend ops while it should not.

> Hmm, does "bisecting" omap2_defconfig and my config give some helpful
> information? Besides of some key points like CONFIG_PREEMPT, I will probably
> just find things which somehow influence timing a little, so I will probably
> end up without anything interesting. Hopefully it will not just fix something.

I doubt it's a register access delay needed somewhere type issue though.
It seems to be related to what happens at suspend/resume noirq level.

> I will try to add printks to find out in which order things happen.

Yes good idea, maybe try with some printks added to twl reads
and writes? If it's too noisy, you could only enable the debugging for
suspend/resume cycle.

> We have
> enable_irq_wake(irq_num);
> in twl4030-irq.c: Does that has any interersting consequences? sys_nirq seems
> to be equal on non-affected and affected systems (pinmux + twl irq config)

The sys_nirq pin needs no remuxing and it's interrupt is always wake-up
capable since it's wired to the wake-up domain.

Regards,

Tony
Sebastian Reichel Feb. 22, 2019, 5:07 p.m. | #23
Hi,

On Fri, Feb 22, 2019 at 06:54:06AM -0800, Tony Lindgren wrote:
> > I will try to add printks to find out in which order things happen.
> 
> Yes good idea, maybe try with some printks added to twl reads
> and writes? If it's too noisy, you could only enable the debugging for
> suspend/resume cycle.

I suggest to also call dump_stack() in the error path.

-- Sebastian
Andreas Kemnade Feb. 22, 2019, 5:08 p.m. | #24
On Fri, 22 Feb 2019 06:54:06 -0800
Tony Lindgren <tony@atomide.com> wrote:

> Hi,
> 
> * Andreas Kemnade <andreas@kemnade.info> [190222 12:08]:
> > Hmm, so what do we know:
> > - git bisect at least led to something interesting, not some unrelated thing.
> >     which might just cause timing differences.  
> 
> Yes seems it's some twl related driver that gets only enabled for gta04
> with your .config.
> 
> > - rtcwake + actual wakeup via rtc on my config = problem
> > - rtcwake + actual wakeup via some other pin in the wkup domain = no problem
> > - rtcwake + actual wakeup via powerbutton (twl4030-pwrbutton) = problem
> > - omap2plus_defconfig seems to work here
> > - no problems at other systems with dm3730 + twl4030 with my config
> >   and with omap2plus defconfig
> > 
> > I deduce that there happens something strange at resume independant
> > from what happend at suspend. Probably some timing issue.  
> 
> It's probaby some twl related code is (wrongly) trying to read/write
> registers at noirq suspend level. Sounds like some driver tagged with
> noirq suspend ops while it should not.
> 
static irqreturn_t handle_twl4030_pih(int irq, void *devid)
in
drivers/mfd/twl4030-irq.c
is doing the failed i2c access.

Regards,
Andreas
Tony Lindgren Feb. 22, 2019, 5:25 p.m. | #25
* Andreas Kemnade <andreas@kemnade.info> [190222 17:09]:
> Tony Lindgren <tony@atomide.com> wrote:
> > It's probaby some twl related code is (wrongly) trying to read/write
> > registers at noirq suspend level. Sounds like some driver tagged with
> > noirq suspend ops while it should not.
> > 
> static irqreturn_t handle_twl4030_pih(int irq, void *devid)
> in
> drivers/mfd/twl4030-irq.c
> is doing the failed i2c access.

I wonder if tagging drivers/mfd/twl4030-irq.c request_threaded_irq
with IRQF_NO_SUSPEND in addition to IRQF_ONESHOT might help if this
triggers right after resume_device_irqs()? Or do we already have
IRQF_NO_SUSPEND set and some interrupt like USB battery charging
is triggering during suspend?

The sys_nirq interrupt can trigger at any point for sure. But we
can't access the i2c registers until the system is back up again.

Regards,

Tony
Andreas Kemnade Feb. 22, 2019, 8:08 p.m. | #26
On Fri, 22 Feb 2019 09:25:04 -0800
Tony Lindgren <tony@atomide.com> wrote:

> * Andreas Kemnade <andreas@kemnade.info> [190222 17:09]:
> > Tony Lindgren <tony@atomide.com> wrote:  
> > > It's probaby some twl related code is (wrongly) trying to read/write
> > > registers at noirq suspend level. Sounds like some driver tagged with
> > > noirq suspend ops while it should not.
> > >   
> > static irqreturn_t handle_twl4030_pih(int irq, void *devid)
> > in
> > drivers/mfd/twl4030-irq.c
> > is doing the failed i2c access.  
> 
> I wonder if tagging drivers/mfd/twl4030-irq.c request_threaded_irq
> with IRQF_NO_SUSPEND in addition to IRQF_ONESHOT might help if this

IRQF_NO_SUSPEND does not help.

> triggers right after resume_device_irqs()? Or do we already have
> IRQF_NO_SUSPEND set and some interrupt like USB battery charging
> is triggering during suspend?

well, if we are doing rtcwake, we could expect to have a twl4030 rtc
interrupt sitting behind the pih.
I would consider usb battery charging irq here a bug because
twl4030_charger and phy-twl4030_usb are modules and not loaded.

Some more research:
what failed is this:
static int
omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
{
        struct omap_i2c_dev *omap = i2c_get_adapdata(adap);
        int i;
        int r;

        r = pm_runtime_get_sync(omap->dev);
        if (r < 0) {
we get -EACCESS there.

It is indeed the case that 
handle_twl4030_pih is called before i2c runtime resume.
Since we have threaded irq here, we can do some sleep in case of
problems. So this dirty hack helps:

diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
index b16c16f194fd..6c729c4ec9b0 100644
--- a/drivers/mfd/twl4030-irq.c
+++ b/drivers/mfd/twl4030-irq.c
@@ -34,6 +34,7 @@
 #include <linux/of.h>
 #include <linux/irqdomain.h>
 #include <linux/mfd/twl.h>
+#include <linux/delay.h>
 
 #include "twl-core.h"
 
@@ -298,7 +299,11 @@ static irqreturn_t handle_twl4030_pih(int irq, void *devid)
                              REG_PIH_ISR_P1);
        if (ret) {
                pr_warn("twl4030: I2C error %d reading PIH ISR\n", ret);
-               return IRQ_NONE;
+               msleep(10);
+               ret = twl_i2c_read_u8(TWL_MODULE_PIH, &pih_isr,
+                                     REG_PIH_ISR_P1);
+               if (ret)
+                       return IRQ_NONE;
        }
 
        while (pih_isr) {


So if for some reason handle_twl4030_pih is called late enough, we will
not have those problems.
So maybe we just need to add a suspend/resume callback pair in
twl-core.c and disable/enable the pih there? So when the pih is run,
runtime_pm is ready, so you are allowed to do pm_runtime_get()

To wake up from suspend, it is sufficient to have

        twl4030_pins: pinmux_twl4030_pins {
                pinctrl-single,pins = <
                        OMAP3_CORE1_IOPAD(0x21e0, PIN_INPUT_PULLUP | PIN_OFF_WAKEUPENABLE | MUX_MODE0) /* sys_nirq.sys_nirq */
                >;
        };

I can even do
@ -752,7 +757,9 @@ int twl4030_init_irq(struct device *dev, int irq_num)
                dev_err(dev, "could not claim irq%d: %d\n", irq_num, status);
                goto fail_rqirq;
        }
+#if 0
        enable_irq_wake(irq_num);
+#endif
 
        return irq_base;
 fail_rqirq:

at least here. So disabling that irq on suspend might be an option.

Regards,
Andreas
Tony Lindgren Feb. 22, 2019, 11:51 p.m. | #27
* Andreas Kemnade <andreas@kemnade.info> [190222 20:08]:
> Some more research:
> what failed is this:
> static int
> omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> {
>         struct omap_i2c_dev *omap = i2c_get_adapdata(adap);
>         int i;
>         int r;
> 
>         r = pm_runtime_get_sync(omap->dev);
>         if (r < 0) {
> we get -EACCESS there.

OK

> It is indeed the case that 
> handle_twl4030_pih is called before i2c runtime resume.
> Since we have threaded irq here, we can do some sleep in case of
> problems. So this dirty hack helps:
> 
> diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
> index b16c16f194fd..6c729c4ec9b0 100644
> --- a/drivers/mfd/twl4030-irq.c
> +++ b/drivers/mfd/twl4030-irq.c
> @@ -34,6 +34,7 @@
>  #include <linux/of.h>
>  #include <linux/irqdomain.h>
>  #include <linux/mfd/twl.h>
> +#include <linux/delay.h>
>  
>  #include "twl-core.h"
>  
> @@ -298,7 +299,11 @@ static irqreturn_t handle_twl4030_pih(int irq, void *devid)
>                               REG_PIH_ISR_P1);
>         if (ret) {
>                 pr_warn("twl4030: I2C error %d reading PIH ISR\n", ret);
> -               return IRQ_NONE;
> +               msleep(10);
> +               ret = twl_i2c_read_u8(TWL_MODULE_PIH, &pih_isr,
> +                                     REG_PIH_ISR_P1);
> +               if (ret)
> +                       return IRQ_NONE;
>         }
>  
>         while (pih_isr) {

How about just check for -EACCESS and return IRQ_NONE without
warning here? And only warn for other errors.

Then in Linux next, we now have new i2c_mark_adapter_suspended(),
so later on if we have something like is_i2c_adapter_suspended()
that could be added. But that's not going to be available for
v5.0 kernels, so for the fix I'd go with just -EACCES + IRQ_NONE.

> So if for some reason handle_twl4030_pih is called late enough, we will
> not have those problems.
> So maybe we just need to add a suspend/resume callback pair in
> twl-core.c and disable/enable the pih there? So when the pih is run,
> runtime_pm is ready, so you are allowed to do pm_runtime_get()
> 
> To wake up from suspend, it is sufficient to have
> 
>         twl4030_pins: pinmux_twl4030_pins {
>                 pinctrl-single,pins = <
>                         OMAP3_CORE1_IOPAD(0x21e0, PIN_INPUT_PULLUP | PIN_OFF_WAKEUPENABLE | MUX_MODE0) /* sys_nirq.sys_nirq */
>                 >;
>         };

Well PIN_OFF_WAKEUPENABLE is nowadays managed with Linux generic
wakeirqs with dev_pm_set_dedicated_wake_irq(). But in this case
you should not even need to set a pad wake up as i2c1 is in
always-on domain.

Because of the RTC on twl, it's best to not add new dependencies
pin muxing and generic wakeirqs that might be needed if we mask
the twl interrupt for suspend.

Regards,

Tony
Andreas Kemnade Feb. 23, 2019, 7:50 a.m. | #28
On Fri, 22 Feb 2019 15:51:32 -0800
Tony Lindgren <tony@atomide.com> wrote:

> * Andreas Kemnade <andreas@kemnade.info> [190222 20:08]:
> > Some more research:
> > what failed is this:
> > static int
> > omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> > {
> >         struct omap_i2c_dev *omap = i2c_get_adapdata(adap);
> >         int i;
> >         int r;
> > 
> >         r = pm_runtime_get_sync(omap->dev);
> >         if (r < 0) {
> > we get -EACCESS there.  
> 
> OK
> 
> > It is indeed the case that 
> > handle_twl4030_pih is called before i2c runtime resume.
> > Since we have threaded irq here, we can do some sleep in case of
> > problems. So this dirty hack helps:
> > 
> > diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
> > index b16c16f194fd..6c729c4ec9b0 100644
> > --- a/drivers/mfd/twl4030-irq.c
> > +++ b/drivers/mfd/twl4030-irq.c
> > @@ -34,6 +34,7 @@
> >  #include <linux/of.h>
> >  #include <linux/irqdomain.h>
> >  #include <linux/mfd/twl.h>
> > +#include <linux/delay.h>
> >  
> >  #include "twl-core.h"
> >  
> > @@ -298,7 +299,11 @@ static irqreturn_t handle_twl4030_pih(int irq, void *devid)
> >                               REG_PIH_ISR_P1);
> >         if (ret) {
> >                 pr_warn("twl4030: I2C error %d reading PIH ISR\n", ret);
> > -               return IRQ_NONE;
> > +               msleep(10);
> > +               ret = twl_i2c_read_u8(TWL_MODULE_PIH, &pih_isr,
> > +                                     REG_PIH_ISR_P1);
> > +               if (ret)
> > +                       return IRQ_NONE;
> >         }
> >  
> >         while (pih_isr) {  
> 
> How about just check for -EACCESS and return IRQ_NONE without
> warning here? And only warn for other errors.
> 
well, then we have one error message less. That does not help much.
The irq is called again and again until:

[   38.590881] twl: Read failed (mod 1, reg 0x01 count 1)
[   38.590942] omap i2c get runtime failed: -13
[   38.590972] twl: Read failed (mod 1, reg 0x01 count 1)
[   38.591735] sched: RT throttling activated
[   38.648101] omap i2c resume
[  282.062530] OOM killer enabled.
[  282.065826] Restarting tasks ... 

so resuming takes 4 minutes. That is not acceptable.

> Then in Linux next, we now have new i2c_mark_adapter_suspended(),
> so later on if we have something like is_i2c_adapter_suspended()
> that could be added. But that's not going to be available for
> v5.0 kernels, so for the fix I'd go with just -EACCES + IRQ_NONE.
> 
> > So if for some reason handle_twl4030_pih is called late enough, we will
> > not have those problems.
> > So maybe we just need to add a suspend/resume callback pair in
> > twl-core.c and disable/enable the pih there? So when the pih is run,
> > runtime_pm is ready, so you are allowed to do pm_runtime_get()
> > 
> > To wake up from suspend, it is sufficient to have
> > 
> >         twl4030_pins: pinmux_twl4030_pins {
> >                 pinctrl-single,pins = <
> >                         OMAP3_CORE1_IOPAD(0x21e0, PIN_INPUT_PULLUP | PIN_OFF_WAKEUPENABLE | MUX_MODE0) /* sys_nirq.sys_nirq */  
> >                 >;  
> >         };  
> 
> Well PIN_OFF_WAKEUPENABLE is nowadays managed with Linux generic
> wakeirqs with dev_pm_set_dedicated_wake_irq(). But in this case
> you should not even need to set a pad wake up as i2c1 is in
> always-on domain.
> 
> Because of the RTC on twl, it's best to not add new dependencies
> pin muxing and generic wakeirqs that might be needed if we mask
> the twl interrupt for suspend.
> 
well, that is the default in twl4030_omap3.dtsi
so it is nothing new.

Regards,
Andreas
Tony Lindgren Feb. 23, 2019, 6:15 p.m. | #29
* Andreas Kemnade <andreas@kemnade.info> [190223 07:50]:
> Tony Lindgren <tony@atomide.com> wrote:
> > How about just check for -EACCESS and return IRQ_NONE without
> > warning here? And only warn for other errors.
> > 
> well, then we have one error message less. That does not help much.
> The irq is called again and again until:
> 
> [   38.590881] twl: Read failed (mod 1, reg 0x01 count 1)
> [   38.590942] omap i2c get runtime failed: -13
> [   38.590972] twl: Read failed (mod 1, reg 0x01 count 1)
> [   38.591735] sched: RT throttling activated
> [   38.648101] omap i2c resume
> [  282.062530] OOM killer enabled.
> [  282.065826] Restarting tasks ... 
> 
> so resuming takes 4 minutes. That is not acceptable.

Oh OK yeah that's not going to work then :)

Regards,

Tony

Patch

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1500,8 +1500,7 @@  static int omap_i2c_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM
-static int omap_i2c_runtime_suspend(struct device *dev)
+static int __maybe_unused omap_i2c_runtime_suspend(struct device *dev)
 {
 	struct omap_i2c_dev *omap = dev_get_drvdata(dev);
 
@@ -1527,7 +1526,7 @@  static int omap_i2c_runtime_suspend(struct device *dev)
 	return 0;
 }
 
-static int omap_i2c_runtime_resume(struct device *dev)
+static int __maybe_unused omap_i2c_runtime_resume(struct device *dev)
 {
 	struct omap_i2c_dev *omap = dev_get_drvdata(dev);
 
@@ -1542,20 +1541,18 @@  static int omap_i2c_runtime_resume(struct device *dev)
 }
 
 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_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
 			   omap_i2c_runtime_resume, NULL)
 };
-#define OMAP_I2C_PM_OPS (&omap_i2c_pm_ops)
-#else
-#define OMAP_I2C_PM_OPS NULL
-#endif /* CONFIG_PM */
 
 static struct platform_driver omap_i2c_driver = {
 	.probe		= omap_i2c_probe,
 	.remove		= omap_i2c_remove,
 	.driver		= {
 		.name	= "omap_i2c",
-		.pm	= OMAP_I2C_PM_OPS,
+		.pm	= &omap_i2c_pm_ops,
 		.of_match_table = of_match_ptr(omap_i2c_of_match),
 	},
 };