| 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 | expand |
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
* 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
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 >
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
> 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
* 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
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 >
* 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 <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
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), > }, > }; >
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!
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
* 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 <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
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
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
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
* 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
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
* 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
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
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
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
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
* 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
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
* 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
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
* 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
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), }, };
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(-)