| Message ID | 20210518150615.53464-1-tony@atomide.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7d076c2f5590e7a2eaba73ded1dbf553fa15a6af |
| Headers | show |
| Series | usb: musb: Check devctl status again for a spurious session request | expand |
Hi, On Tue, 18 May 2021 18:06:15 +0300 Tony Lindgren <tony@atomide.com> wrote: > On start-up, we can get a spurious session request interrupt with nothing > connected. After that the devctl session bit will silently clear, but the > musb hardware is never idled until a cable is plugged in, or the glue > layer module is reloaded. > > Let's just check the session bit again in 3 seconds in peripheral mode > to catch the issue. > Tested this together with the other musb patch you sent on gta04. This has some interesting side effects. Test done: - loading kernel+ramdisk via usb-dfu - disconnecting usb cable - loading omap_hdq (to see battery status) - idling serial ports - checking battery current 1. - loading omap2430, phy-twl4030-usb, g_ether - checking battery current 2 (again with idled serial ports). - rtcwake -s 20 -m mem - checking current during suspend (3) Without your patches: current 2 is current 1 + approx 15 mA, current 3 is near current 1. With your patches: current 2 is near current 1, current 3 is approx 15mA higher. Another strange thing I have hit (I have not done this test before, no idea yet if it is related, but it is also about musb): Connecting a usb cable while serial ports are idle (not in system supend), console serial port does not wake up by input, it reacts again if I unplug usb. If I give usb0 an IP address, I can ping it. No intensive debugging done there yet. Just stumbled across it. Regards, Andreas
Hi, * Andreas Kemnade <andreas@kemnade.info> [210527 19:15]: > Hi, > > On Tue, 18 May 2021 18:06:15 +0300 > Tony Lindgren <tony@atomide.com> wrote: > > > On start-up, we can get a spurious session request interrupt with nothing > > connected. After that the devctl session bit will silently clear, but the > > musb hardware is never idled until a cable is plugged in, or the glue > > layer module is reloaded. > > > > Let's just check the session bit again in 3 seconds in peripheral mode > > to catch the issue. > > > Tested this together with the other musb patch you sent on gta04. > This has some interesting side effects. > > Test done: > - loading kernel+ramdisk via usb-dfu > - disconnecting usb cable > - loading omap_hdq (to see battery status) > - idling serial ports > - checking battery current 1. > - loading omap2430, phy-twl4030-usb, g_ether > - checking battery current 2 (again with idled serial ports). > - rtcwake -s 20 -m mem > - checking current during suspend (3) > > Without your patches: current 2 is current 1 + approx 15 mA, current 3 > is near current 1. > With your patches: current 2 is near current 1, current 3 is approx > 15mA higher. Interesting, so power consumption is now better for runtime with cable disconnected, and after booting, but now somehow is now worse for suspended state. I'll try to reproduce. > Another strange thing I have hit (I have not done this test before, no > idea yet if it is related, but it is also about musb): > Connecting a usb cable while serial ports are idle (not in system > supend), console serial port does not wake up by input, it reacts again > if I unplug usb. If I give usb0 an IP address, I can ping it. No > intensive debugging done there yet. Just stumbled across it. Hmm so if you have a usb cable connected and gta04 is a b-device, and there is vbus, musb should not currently idle at all. Maybe your uart3 wakeirq is not using the uart3_rx.uart3_rx pad but uses some alternative pad and the wakeirq is not working? Or the twl4030 usb phy is somehow interfering with the uart3 pins when usb is in use. Worth checking the schematics again for the uart3 rx pin used? Sorry I don't remember if omap3 has multiple alternative pins for uart3, but I think it does. Regards, Tony
Hi, On Fri, 28 May 2021 08:57:56 +0300 Tony Lindgren <tony@atomide.com> wrote: > Hi, > > * Andreas Kemnade <andreas@kemnade.info> [210527 19:15]: > > Hi, > > > > On Tue, 18 May 2021 18:06:15 +0300 > > Tony Lindgren <tony@atomide.com> wrote: > > > > > On start-up, we can get a spurious session request interrupt with nothing > > > connected. After that the devctl session bit will silently clear, but the > > > musb hardware is never idled until a cable is plugged in, or the glue > > > layer module is reloaded. > > > > > > Let's just check the session bit again in 3 seconds in peripheral mode > > > to catch the issue. > > > > > Tested this together with the other musb patch you sent on gta04. > > This has some interesting side effects. > > > > Test done: > > - loading kernel+ramdisk via usb-dfu > > - disconnecting usb cable > > - loading omap_hdq (to see battery status) > > - idling serial ports > > - checking battery current 1. > > - loading omap2430, phy-twl4030-usb, g_ether > > - checking battery current 2 (again with idled serial ports). > > - rtcwake -s 20 -m mem > > - checking current during suspend (3) > > > > Without your patches: current 2 is current 1 + approx 15 mA, current 3 > > is near current 1. > > With your patches: current 2 is near current 1, current 3 is approx > > 15mA higher. > > Interesting, so power consumption is now better for runtime with cable > disconnected, and after booting, but now somehow is now worse for > suspended state. I'll try to reproduce. dmesg of high power suspend: [ 769.181793] PM: suspend entry (deep) [ 769.181915] Filesystems sync: 0.000 seconds [ 769.183410] Freezing user space processes ... (elapsed 0.001 seconds) done. [ 769.184875] OOM killer disabled. [ 769.184906] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. [ 769.186462] printk: Suspending console(s) (use no_console_suspend to debug) [ 769.514526] Disabling non-boot CPUs ... [ 769.514556] Successfully put all powerdomains to target state [ 770.024322] musb-hdrc musb-hdrc.0.auto: musb_set_peripheral: already in peripheral mode: 80 [ 770.026641] pwrseq_simple wifi_pwrseq: GPIO lookup for consumer reset [ 770.026672] pwrseq_simple wifi_pwrseq: using device tree for GPIO lookup [ 770.026702] pwrseq_simple wifi_pwrseq: No GPIO consumer reset found [ 770.027862] omap_hsmmc 480b4000.mmc: GPIO lookup for consumer cd [ 770.027893] omap_hsmmc 480b4000.mmc: using device tree for GPIO lookup [ 770.027923] of_get_named_gpiod_flags: can't parse 'cd-gpios' property of node '/ocp@68000000/mmc@480b4000[0]' [ 770.027954] of_get_named_gpiod_flags: can't parse 'cd-gpio' property of node '/ocp@68000000/mmc@480b4000[0]' [ 770.028015] omap_hsmmc 480b4000.mmc: using lookup tables for GPIO lookup [ 770.028015] omap_hsmmc 480b4000.mmc: No GPIO consumer cd found [ 770.028045] omap_hsmmc 480b4000.mmc: GPIO lookup for consumer wp [ 770.028045] omap_hsmmc 480b4000.mmc: using device tree for GPIO lookup [ 770.028076] of_get_named_gpiod_flags: can't parse 'wp-gpios' property of node '/ocp@68000000/mmc@480b4000[0]' [ 770.028106] of_get_named_gpiod_flags: can't parse 'wp-gpio' property of node '/ocp@68000000/mmc@480b4000[0]' [ 770.028137] omap_hsmmc 480b4000.mmc: using lookup tables for GPIO lookup [ 770.028137] omap_hsmmc 480b4000.mmc: No GPIO consumer wp found [ 770.028594] OOM killer enabled. [ 770.028625] Restarting tasks ... done. [ 770.142852] PM: suspend exit yes, some gpio driver is not loaded (not included in omap2plus_defconfig). rmmod omap2430 seems to bring back suspend current current to low currents. > > > Another strange thing I have hit (I have not done this test before, no > > idea yet if it is related, but it is also about musb): > > Connecting a usb cable while serial ports are idle (not in system > > supend), console serial port does not wake up by input, it reacts again > > if I unplug usb. If I give usb0 an IP address, I can ping it. No > > intensive debugging done there yet. Just stumbled across it. > > Hmm so if you have a usb cable connected and gta04 is a b-device, and > there is vbus, musb should not currently idle at all. > hmm, if musb and serial idle, and I connect usb, musb should power up. It seems to work. I have no idea about serial state there. I used the non-8250-based driver for that test (not the other one), so I do not need to detach console. Maybe I created some wakeup race when musb/serial wake up at the same time. because musb causes some console messages. Ok, did not hit the send button... retested with the other console driver. > Maybe your uart3 wakeirq is not using the uart3_rx.uart3_rx > pad but uses some alternative pad and the wakeirq is not working? > Console rx is connected to H20, which is uart3_rx_irrx = CONTROL_PADCONF_UART3_RTS_SD[31:16] Nothing strange connected there. Regards, Andreas
* Andreas Kemnade <andreas@kemnade.info> [210528 09:37]: > rmmod omap2430 seems to bring back suspend current current to low > currents. I wonder if the recent musb fix patch helps with this? [PATCH] usb: musb: fix MUSB_QUIRK_B_DISCONNECT_99 handling https://lore.kernel.org/lkml/20210528140446.278076-1-thomas.petazzoni@bootlin.com/ Regards, Tony
Hi Tony, On Wed, 2 Jun 2021 08:41:54 +0300 Tony Lindgren <tony@atomide.com> wrote: > * Andreas Kemnade <andreas@kemnade.info> [210528 09:37]: > > rmmod omap2430 seems to bring back suspend current current to low > > currents. > > I wonder if the recent musb fix patch helps with this? > > [PATCH] usb: musb: fix MUSB_QUIRK_B_DISCONNECT_99 handling > https://lore.kernel.org/lkml/20210528140446.278076-1-thomas.petazzoni@bootlin.com/ > it does not help. I did some more experiments and found out that echo on >/sys/bus/platform/devices/480ab000.usb_otg_hs/power/control sleep 3 before doing an rtcwake -m mem helps 2 seconds are not long enough. All with usb disconnected. some debug printks inserted into omap2430.c (doing it without the echo on thing) [ 355.643768] PM: suspend entry (deep) [ 355.643920] Filesystems sync: 0.000 seconds [ 355.644683] Freezing user space processes ... (elapsed 0.001 seconds) done. [ 355.646392] OOM killer disabled. [ 355.646423] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. [ 355.648010] printk: Suspending console(s) (use no_console_suspend to debug) [ 355.649291] musb-hdrc musb-hdrc.1.auto: omap2430 runtime_resume [ 355.966522] musb-omap2430 480ab000.usb_otg_hs: omap2430 suspend [ 355.966552] musb-hdrc musb-hdrc.1.auto: omap2430 runtime_suspend [ 355.975250] musb-hdrc musb-hdrc.1.auto: omap2430 runtime_suspend [ 355.977294] Disabling non-boot CPUs ... [ 355.977325] Successfully put all powerdomains to target state [ 355.979034] musb-hdrc musb-hdrc.1.auto: omap2430 runtime_resume [ 356.244628] musb-omap2430 480ab000.usb_otg_hs: omap2430 resume [ 356.244659] musb-hdrc musb-hdrc.1.auto: omap2430 runtime_resume [ 356.486145] musb-hdrc musb-hdrc.1.auto: VBUS b_idle, devctl 80 [ 356.486175] musb-hdrc musb-hdrc.1.auto: VBUS Disconnect [ 356.486175] musb-hdrc musb-hdrc.1.auto: musb_set_peripheral: already in peripheral mode: 80 There are two suspends in a row, I tried to fix it but it seems not to be the cause for trouble. Regards, Andreas
On Fri, 28 May 2021 08:57:56 +0300 Tony Lindgren <tony@atomide.com> wrote: > Hi, > > * Andreas Kemnade <andreas@kemnade.info> [210527 19:15]: > > Hi, > > > > On Tue, 18 May 2021 18:06:15 +0300 > > Tony Lindgren <tony@atomide.com> wrote: > > > > > On start-up, we can get a spurious session request interrupt with nothing > > > connected. After that the devctl session bit will silently clear, but the > > > musb hardware is never idled until a cable is plugged in, or the glue > > > layer module is reloaded. > > > > > > Let's just check the session bit again in 3 seconds in peripheral mode > > > to catch the issue. > > > > > Tested this together with the other musb patch you sent on gta04. > > This has some interesting side effects. > > > > Test done: > > - loading kernel+ramdisk via usb-dfu > > - disconnecting usb cable > > - loading omap_hdq (to see battery status) > > - idling serial ports > > - checking battery current 1. > > - loading omap2430, phy-twl4030-usb, g_ether > > - checking battery current 2 (again with idled serial ports). > > - rtcwake -s 20 -m mem > > - checking current during suspend (3) > > > > Without your patches: current 2 is current 1 + approx 15 mA, current 3 > > is near current 1. > > With your patches: current 2 is near current 1, current 3 is approx > > 15mA higher. > > Interesting, so power consumption is now better for runtime with cable > disconnected, and after booting, but now somehow is now worse for > suspended state. I'll try to reproduce. > I inserted some more dev-dbg [ 60.241790] PM: suspend entry (deep) [ 60.245513] Filesystems sync: 0.000 seconds [ 60.251312] Freezing user space processes ... (elapsed 0.001 seconds) done. [ 60.260040] OOM killer disabled. [ 60.263275] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. [ 60.272338] printk: Suspending console(s) (use no_console_suspend to debug) [ 60.281311] musb-omap2430 480ab000.usb_otg_hs: omap2430 runtime_resume -> this is triggered by what? [ 60.281341] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_usb_runtime_resume -> and here something stays on... [ 60.346374] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_phy_power_on [ 60.796630] musb-hdrc musb-hdrc.0.auto: musb_suspend begin [ 60.796722] musb-hdrc musb-hdrc.0.auto: musb_suspend end [ 60.796752] musb-omap2430 480ab000.usb_otg_hs: omap2430 suspend [ 60.796783] musb-omap2430 480ab000.usb_otg_hs: omap2430 runtime_suspend [ 60.796783] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_phy_power_off [ 60.796813] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_usb_suspend [ 60.806549] Disabling non-boot CPUs ... [ 60.806579] Successfully put all powerdomains to target state forcing omap2430 runtime on before suspend: [ 160.467742] musb-omap2430 480ab000.usb_otg_hs: omap2430 runtime_resume [ 165.001495] PM: suspend entry (deep) [ 165.005218] Filesystems sync: 0.000 seconds [ 165.010284] Freezing user space processes ... (elapsed 0.001 seconds) done. [ 165.018981] OOM killer disabled. [ 165.022247] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. [ 165.031311] printk: Suspending console(s) (use no_console_suspend to debug) [ 165.040496] musb-hdrc musb-hdrc.0.auto: musb_suspend begin [ 165.040618] musb-hdrc musb-hdrc.0.auto: musb_suspend end [ 165.040618] musb-omap2430 480ab000.usb_otg_hs: omap2430 suspend [ 165.040649] musb-omap2430 480ab000.usb_otg_hs: omap2430 runtime_suspend [ 165.040679] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_usb_suspend [ 165.050506] Disabling non-boot CPUs ... [ 165.050537] Successfully put all powerdomains to target state Regards, Andreas
* Andreas Kemnade <andreas@kemnade.info> [210604 08:35]: > I inserted some more dev-dbg > [ 60.241790] PM: suspend entry (deep) > [ 60.245513] Filesystems sync: 0.000 seconds > [ 60.251312] Freezing user space processes ... (elapsed 0.001 seconds) done. > [ 60.260040] OOM killer disabled. > [ 60.263275] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. > [ 60.272338] printk: Suspending console(s) (use no_console_suspend to debug) > [ 60.281311] musb-omap2430 480ab000.usb_otg_hs: omap2430 runtime_resume > -> this is triggered by what? I think that comes from the pm_runtime_get_sync() in musb_suspend(). > [ 60.281341] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_usb_runtime_resume > -> and here something stays on... > > [ 60.346374] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_phy_power_on > [ 60.796630] musb-hdrc musb-hdrc.0.auto: musb_suspend begin > [ 60.796722] musb-hdrc musb-hdrc.0.auto: musb_suspend end > [ 60.796752] musb-omap2430 480ab000.usb_otg_hs: omap2430 suspend > [ 60.796783] musb-omap2430 480ab000.usb_otg_hs: omap2430 runtime_suspend > [ 60.796783] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_phy_power_off > [ 60.796813] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_usb_suspend > [ 60.806549] Disabling non-boot CPUs ... > [ 60.806579] Successfully put all powerdomains to target state Well since commit 88d26136a256 ("PM: Prevent runtime suspend during system resume") nothing gets runtime idled during suspend with the extra pm_runtime_get_noresume() call in device_prepare() that does not get released until in device_complete(). > forcing omap2430 runtime on before suspend: > [ 160.467742] musb-omap2430 480ab000.usb_otg_hs: omap2430 runtime_resume > [ 165.001495] PM: suspend entry (deep) > [ 165.005218] Filesystems sync: 0.000 seconds > [ 165.010284] Freezing user space processes ... (elapsed 0.001 seconds) done. > [ 165.018981] OOM killer disabled. > [ 165.022247] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. > [ 165.031311] printk: Suspending console(s) (use no_console_suspend to debug) > [ 165.040496] musb-hdrc musb-hdrc.0.auto: musb_suspend begin > [ 165.040618] musb-hdrc musb-hdrc.0.auto: musb_suspend end > [ 165.040618] musb-omap2430 480ab000.usb_otg_hs: omap2430 suspend > [ 165.040649] musb-omap2430 480ab000.usb_otg_hs: omap2430 runtime_suspend > [ 165.040679] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_usb_suspend > [ 165.050506] Disabling non-boot CPUs ... > [ 165.050537] Successfully put all powerdomains to target state That's interesting. Hmm so we bail out early based on glue->is_runtime_suspended, and omap3 is still probing devices with omap_device.c instead of ti-sysc.c, so sounds like the duplicate calls you noticed might cause the issue. Does the following patch fix things for you or does something else break again? :) Regards, Tony 8< -------------- diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c --- a/drivers/usb/musb/omap2430.c +++ b/drivers/usb/musb/omap2430.c @@ -332,6 +332,7 @@ static int omap2430_probe(struct platform_device *pdev) glue->musb = musb; glue->status = MUSB_UNKNOWN; glue->control_otghs = ERR_PTR(-ENODEV); + glue->is_runtime_suspended = 1; pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); if (!pdata) @@ -453,6 +454,9 @@ static int omap2430_runtime_suspend(struct device *dev) if (!musb) return 0; + if (glue->is_runtime_suspended) + return 0; + musb->context.otg_interfsel = musb_readl(musb->mregs, OTG_INTERFSEL); @@ -474,6 +478,9 @@ static int omap2430_runtime_resume(struct device *dev) if (!musb) return 0; + if (!glue->is_runtime_suspended) + return 0; + phy_init(musb->phy); phy_power_on(musb->phy);
On Fri, 4 Jun 2021 12:39:54 +0300 Tony Lindgren <tony@atomide.com> wrote: > * Andreas Kemnade <andreas@kemnade.info> [210604 08:35]: > > I inserted some more dev-dbg > > [ 60.241790] PM: suspend entry (deep) > > [ 60.245513] Filesystems sync: 0.000 seconds > > [ 60.251312] Freezing user space processes ... (elapsed 0.001 seconds) done. > > [ 60.260040] OOM killer disabled. > > [ 60.263275] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. > > [ 60.272338] printk: Suspending console(s) (use no_console_suspend to debug) > > [ 60.281311] musb-omap2430 480ab000.usb_otg_hs: omap2430 runtime_resume > > -> this is triggered by what? > > I think that comes from the pm_runtime_get_sync() in musb_suspend(). > @@ -2825,6 +2826,7 @@ static int musb_suspend(struct device *dev) struct musb *musb = dev_to_musb(dev); unsigned long flags; int ret; + dev_dbg(musb->controller, "musb_suspend begin\n"); ret = pm_runtime_get_sync(dev); if (ret < 0) { my dev_dbg comes before that. > > [ 60.281341] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_usb_runtime_resume > > -> and here something stays on... > > > > [ 60.346374] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_phy_power_on > > [ 60.796630] musb-hdrc musb-hdrc.0.auto: musb_suspend begin > > [ 60.796722] musb-hdrc musb-hdrc.0.auto: musb_suspend end > > [ 60.796752] musb-omap2430 480ab000.usb_otg_hs: omap2430 suspend > > [ 60.796783] musb-omap2430 480ab000.usb_otg_hs: omap2430 runtime_suspend > > [ 60.796783] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_phy_power_off > > [ 60.796813] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_usb_suspend > > [ 60.806549] Disabling non-boot CPUs ... > > [ 60.806579] Successfully put all powerdomains to target state > > Well since commit 88d26136a256 ("PM: Prevent runtime suspend during system resume") > nothing gets runtime idled during suspend with the extra pm_runtime_get_noresume() > call in device_prepare() that does not get released until in device_complete(). > well, the result is that the phy stays powered on. And that is the problem. static int twl4030_phy_power_off(struct phy *phy) and static int __maybe_unused twl4030_usb_suspend(struct device *dev) do not turn off the phy. I think the solution is to turn the phy off if no cable is connected. > > forcing omap2430 runtime on before suspend: > > [ 160.467742] musb-omap2430 480ab000.usb_otg_hs: omap2430 runtime_resume > > [ 165.001495] PM: suspend entry (deep) > > [ 165.005218] Filesystems sync: 0.000 seconds > > [ 165.010284] Freezing user space processes ... (elapsed 0.001 seconds) done. > > [ 165.018981] OOM killer disabled. > > [ 165.022247] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. > > [ 165.031311] printk: Suspending console(s) (use no_console_suspend to debug) > > [ 165.040496] musb-hdrc musb-hdrc.0.auto: musb_suspend begin > > [ 165.040618] musb-hdrc musb-hdrc.0.auto: musb_suspend end > > [ 165.040618] musb-omap2430 480ab000.usb_otg_hs: omap2430 suspend > > [ 165.040649] musb-omap2430 480ab000.usb_otg_hs: omap2430 runtime_suspend > > [ 165.040679] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_usb_suspend > > [ 165.050506] Disabling non-boot CPUs ... > > [ 165.050537] Successfully put all powerdomains to target state > > That's interesting. Hmm so we bail out early based on glue->is_runtime_suspended, > and omap3 is still probing devices with omap_device.c instead of ti-sysc.c, so > sounds like the duplicate calls you noticed might cause the issue. > > Does the following patch fix things for you or does something else break again? :) > looks like something I have already tried, but I will give it a look again. Regards, Andreas
* Andreas Kemnade <andreas@kemnade.info> [210604 10:00]: > On Fri, 4 Jun 2021 12:39:54 +0300 > Tony Lindgren <tony@atomide.com> wrote: > > > * Andreas Kemnade <andreas@kemnade.info> [210604 08:35]: > > > I inserted some more dev-dbg > > > [ 60.241790] PM: suspend entry (deep) > > > [ 60.245513] Filesystems sync: 0.000 seconds > > > [ 60.251312] Freezing user space processes ... (elapsed 0.001 seconds) done. > > > [ 60.260040] OOM killer disabled. > > > [ 60.263275] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. > > > [ 60.272338] printk: Suspending console(s) (use no_console_suspend to debug) > > > [ 60.281311] musb-omap2430 480ab000.usb_otg_hs: omap2430 runtime_resume > > > -> this is triggered by what? > > > > I think that comes from the pm_runtime_get_sync() in musb_suspend(). > > > @@ -2825,6 +2826,7 @@ static int musb_suspend(struct device *dev) > struct musb *musb = dev_to_musb(dev); > unsigned long flags; > int ret; > + dev_dbg(musb->controller, "musb_suspend begin\n"); > > ret = pm_runtime_get_sync(dev); > if (ret < 0) { > > my dev_dbg comes before that. Hmm maybe try adding dump_stack() call there to see where it comes from? > > > [ 60.281341] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_usb_runtime_resume > > > -> and here something stays on... > > > > > > [ 60.346374] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_phy_power_on > > > [ 60.796630] musb-hdrc musb-hdrc.0.auto: musb_suspend begin > > > [ 60.796722] musb-hdrc musb-hdrc.0.auto: musb_suspend end > > > [ 60.796752] musb-omap2430 480ab000.usb_otg_hs: omap2430 suspend > > > [ 60.796783] musb-omap2430 480ab000.usb_otg_hs: omap2430 runtime_suspend > > > [ 60.796783] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_phy_power_off > > > [ 60.796813] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_usb_suspend > > > [ 60.806549] Disabling non-boot CPUs ... > > > [ 60.806579] Successfully put all powerdomains to target state > > > > Well since commit 88d26136a256 ("PM: Prevent runtime suspend during system resume") > > nothing gets runtime idled during suspend with the extra pm_runtime_get_noresume() > > call in device_prepare() that does not get released until in device_complete(). > > > well, the result is that the phy stays powered on. And that is the > problem. > static int twl4030_phy_power_off(struct phy *phy) > and > static int __maybe_unused twl4030_usb_suspend(struct device *dev) > do not turn off the phy. I think the solution is to turn the phy off if > no cable is connected. I recall there's some errata where the phy needs to be always enabled to avoid PMIC damage when a charger is connected. But if the phy wakes up the glue layer then that should be ignored for the glue layer maybe with .prepare and .complete suspend calls.. > > > forcing omap2430 runtime on before suspend: > > > [ 160.467742] musb-omap2430 480ab000.usb_otg_hs: omap2430 runtime_resume > > > [ 165.001495] PM: suspend entry (deep) > > > [ 165.005218] Filesystems sync: 0.000 seconds > > > [ 165.010284] Freezing user space processes ... (elapsed 0.001 seconds) done. > > > [ 165.018981] OOM killer disabled. > > > [ 165.022247] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. > > > [ 165.031311] printk: Suspending console(s) (use no_console_suspend to debug) > > > [ 165.040496] musb-hdrc musb-hdrc.0.auto: musb_suspend begin > > > [ 165.040618] musb-hdrc musb-hdrc.0.auto: musb_suspend end > > > [ 165.040618] musb-omap2430 480ab000.usb_otg_hs: omap2430 suspend > > > [ 165.040649] musb-omap2430 480ab000.usb_otg_hs: omap2430 runtime_suspend > > > [ 165.040679] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_usb_suspend > > > [ 165.050506] Disabling non-boot CPUs ... > > > [ 165.050537] Successfully put all powerdomains to target state > > > > That's interesting. Hmm so we bail out early based on glue->is_runtime_suspended, > > and omap3 is still probing devices with omap_device.c instead of ti-sysc.c, so > > sounds like the duplicate calls you noticed might cause the issue. > > > > Does the following patch fix things for you or does something else break again? :) > > > looks like something I have already tried, but I will give it a look > again. It seemed to behave for me based on a quick test against Linux next. I saw suspend power consumption go down, this with no cable connected and device in my test rack. But maybe your test is a bit different :) Regards, Tony
On Fri, 4 Jun 2021 13:08:20 +0300 Tony Lindgren <tony@atomide.com> wrote: > * Andreas Kemnade <andreas@kemnade.info> [210604 10:00]: > > On Fri, 4 Jun 2021 12:39:54 +0300 > > Tony Lindgren <tony@atomide.com> wrote: > > > > > * Andreas Kemnade <andreas@kemnade.info> [210604 08:35]: > > > > I inserted some more dev-dbg > > > > [ 60.241790] PM: suspend entry (deep) > > > > [ 60.245513] Filesystems sync: 0.000 seconds > > > > [ 60.251312] Freezing user space processes ... (elapsed 0.001 seconds) done. > > > > [ 60.260040] OOM killer disabled. > > > > [ 60.263275] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. > > > > [ 60.272338] printk: Suspending console(s) (use no_console_suspend to debug) > > > > [ 60.281311] musb-omap2430 480ab000.usb_otg_hs: omap2430 runtime_resume > > > > -> this is triggered by what? > > > > > > I think that comes from the pm_runtime_get_sync() in musb_suspend(). > > > > > @@ -2825,6 +2826,7 @@ static int musb_suspend(struct device *dev) > > struct musb *musb = dev_to_musb(dev); > > unsigned long flags; > > int ret; > > + dev_dbg(musb->controller, "musb_suspend begin\n"); > > > > ret = pm_runtime_get_sync(dev); > > if (ret < 0) { > > > > my dev_dbg comes before that. > > Hmm maybe try adding dump_stack() call there to see where it comes from? > > > > > [ 60.281341] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_usb_runtime_resume > > > > -> and here something stays on... > > > > > > > > [ 60.346374] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_phy_power_on > > > > [ 60.796630] musb-hdrc musb-hdrc.0.auto: musb_suspend begin > > > > [ 60.796722] musb-hdrc musb-hdrc.0.auto: musb_suspend end > > > > [ 60.796752] musb-omap2430 480ab000.usb_otg_hs: omap2430 suspend > > > > [ 60.796783] musb-omap2430 480ab000.usb_otg_hs: omap2430 runtime_suspend > > > > [ 60.796783] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_phy_power_off > > > > [ 60.796813] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_usb_suspend > > > > [ 60.806549] Disabling non-boot CPUs ... > > > > [ 60.806579] Successfully put all powerdomains to target state > > > > > > Well since commit 88d26136a256 ("PM: Prevent runtime suspend during system resume") > > > nothing gets runtime idled during suspend with the extra pm_runtime_get_noresume() > > > call in device_prepare() that does not get released until in device_complete(). > > > > > well, the result is that the phy stays powered on. And that is the > > problem. > > static int twl4030_phy_power_off(struct phy *phy) > > and > > static int __maybe_unused twl4030_usb_suspend(struct device *dev) > > do not turn off the phy. I think the solution is to turn the phy off if > > no cable is connected. > > I recall there's some errata where the phy needs to be always enabled to avoid > PMIC damage when a charger is connected. But if the phy wakes up the glue layer > then that should be ignored for the glue layer maybe with .prepare and .complete > suspend calls.. > yes, phy needs to be enabled when a charger is connected. That was the reason why we do not use phy_power_off() for turning it off. But if no charger is connected, the phy does not need to be turned on. I will test your patch at some time in the afternoon. Regards, Andreas
Hi Tony, On Fri, 4 Jun 2021 12:39:54 +0300 Tony Lindgren <tony@atomide.com> wrote: [...] > Does the following patch fix things for you or does something else break again? :) > It fixes things mostly. If I quickly enter suspend after usb disconnect, current is still high in suspend (limit approx 2-3 seconds). But I think this is the right track and these two patches are clearly an improvement if applied together. Regards, Andreas > Regards, > > Tony > > 8< -------------- > diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c > --- a/drivers/usb/musb/omap2430.c > +++ b/drivers/usb/musb/omap2430.c > @@ -332,6 +332,7 @@ static int omap2430_probe(struct platform_device *pdev) > glue->musb = musb; > glue->status = MUSB_UNKNOWN; > glue->control_otghs = ERR_PTR(-ENODEV); > + glue->is_runtime_suspended = 1; > > pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); > if (!pdata) > @@ -453,6 +454,9 @@ static int omap2430_runtime_suspend(struct device *dev) > if (!musb) > return 0; > > + if (glue->is_runtime_suspended) > + return 0; > + > musb->context.otg_interfsel = musb_readl(musb->mregs, > OTG_INTERFSEL); > > @@ -474,6 +478,9 @@ static int omap2430_runtime_resume(struct device *dev) > if (!musb) > return 0; > > + if (!glue->is_runtime_suspended) > + return 0; > + > phy_init(musb->phy); > phy_power_on(musb->phy); >
On Fri, 4 Jun 2021 12:39:54 +0300 Tony Lindgren <tony@atomide.com> wrote: > * Andreas Kemnade <andreas@kemnade.info> [210604 08:35]: > > I inserted some more dev-dbg > > [ 60.241790] PM: suspend entry (deep) > > [ 60.245513] Filesystems sync: 0.000 seconds > > [ 60.251312] Freezing user space processes ... (elapsed 0.001 seconds) done. > > [ 60.260040] OOM killer disabled. > > [ 60.263275] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. > > [ 60.272338] printk: Suspending console(s) (use no_console_suspend to debug) > > [ 60.281311] musb-omap2430 480ab000.usb_otg_hs: omap2430 runtime_resume > > -> this is triggered by what? > > I think that comes from the pm_runtime_get_sync() in musb_suspend(). > > > [ 60.281341] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_usb_runtime_resume > > -> and here something stays on... > > > > [ 60.346374] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_phy_power_on > > [ 60.796630] musb-hdrc musb-hdrc.0.auto: musb_suspend begin > > [ 60.796722] musb-hdrc musb-hdrc.0.auto: musb_suspend end > > [ 60.796752] musb-omap2430 480ab000.usb_otg_hs: omap2430 suspend > > [ 60.796783] musb-omap2430 480ab000.usb_otg_hs: omap2430 runtime_suspend > > [ 60.796783] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_phy_power_off > > [ 60.796813] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_usb_suspend > > [ 60.806549] Disabling non-boot CPUs ... > > [ 60.806579] Successfully put all powerdomains to target state > > Well since commit 88d26136a256 ("PM: Prevent runtime suspend during system resume") > nothing gets runtime idled during suspend with the extra pm_runtime_get_noresume() > call in device_prepare() that does not get released until in device_complete(). > > > forcing omap2430 runtime on before suspend: > > [ 160.467742] musb-omap2430 480ab000.usb_otg_hs: omap2430 runtime_resume > > [ 165.001495] PM: suspend entry (deep) > > [ 165.005218] Filesystems sync: 0.000 seconds > > [ 165.010284] Freezing user space processes ... (elapsed 0.001 seconds) done. > > [ 165.018981] OOM killer disabled. > > [ 165.022247] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. > > [ 165.031311] printk: Suspending console(s) (use no_console_suspend to debug) > > [ 165.040496] musb-hdrc musb-hdrc.0.auto: musb_suspend begin > > [ 165.040618] musb-hdrc musb-hdrc.0.auto: musb_suspend end > > [ 165.040618] musb-omap2430 480ab000.usb_otg_hs: omap2430 suspend > > [ 165.040649] musb-omap2430 480ab000.usb_otg_hs: omap2430 runtime_suspend > > [ 165.040679] twl4030_usb 48070000.i2c:twl@48:twl4030-usb: twl4030_usb_suspend > > [ 165.050506] Disabling non-boot CPUs ... > > [ 165.050537] Successfully put all powerdomains to target state > > That's interesting. Hmm so we bail out early based on glue->is_runtime_suspended, > and omap3 is still probing devices with omap_device.c instead of ti-sysc.c, so > sounds like the duplicate calls you noticed might cause the issue. > > Does the following patch fix things for you or does something else break again? :) > sigh,.. ok, it breaks something. gadget (at least ecm) only works if musb/phy stuff is loaded, ecm configured via configfs rmmod omap2430 modprube 2430 until the next usb disconnect and another rmmod/modprobe is required. The following musb patches were applied additional to that one you added to this mail on top of 5.13-rc4. usb: musb: fix MUSB_QUIRK_B_DISCONNECT_99 handling usb: musb: Add missing PM suspend and resume functions for 2430 glue usb: musb: Check devctl status again for a spurious session request Regards, Andreas
* Andreas Kemnade <andreas@kemnade.info> [210604 17:00]: > On Fri, 4 Jun 2021 12:39:54 +0300 > Tony Lindgren <tony@atomide.com> wrote: > > Does the following patch fix things for you or does something else break again? :) > > > sigh,.. > ok, it breaks something. gadget (at least ecm) only works if > musb/phy stuff is loaded, ecm configured via configfs > rmmod omap2430 > modprube 2430 > > until the next usb disconnect > and another rmmod/modprobe is required. Hmm I don't follow, do you mean there's now another issue after a system suspend? Or is this issue not related to system suspend and resume? > The following musb patches were applied additional to that one you > added to this mail on top of 5.13-rc4. > > usb: musb: fix MUSB_QUIRK_B_DISCONNECT_99 handling > usb: musb: Add missing PM suspend and resume functions for 2430 glue > usb: musb: Check devctl status again for a spurious session request Does one of the above cause some additional usb gadget issue? Regards, Tony
On Sat, 5 Jun 2021 08:18:14 +0300 Tony Lindgren <tony@atomide.com> wrote: > * Andreas Kemnade <andreas@kemnade.info> [210604 17:00]: > > On Fri, 4 Jun 2021 12:39:54 +0300 > > Tony Lindgren <tony@atomide.com> wrote: > > > Does the following patch fix things for you or does something else break again? :) > > > > > sigh,.. > > ok, it breaks something. gadget (at least ecm) only works if > > musb/phy stuff is loaded, ecm configured via configfs > > rmmod omap2430 > > modprube 2430 > > > > until the next usb disconnect > > and another rmmod/modprobe is required. > > Hmm I don't follow, do you mean there's now another issue after a > system suspend? Or is this issue not related to system suspend and > resume? > independently of suspend. > > The following musb patches were applied additional to that one you > > added to this mail on top of 5.13-rc4. > > > > usb: musb: fix MUSB_QUIRK_B_DISCONNECT_99 handling > > usb: musb: Add missing PM suspend and resume functions for 2430 glue > > usb: musb: Check devctl status again for a spurious session request > > Does one of the above cause some additional usb gadget issue? > I do not see any additional gadget issue just with them. It just starts with the small patch you proposed to fix suspend power consumption in this thread. Regards, Andreas
* Andreas Kemnade <andreas@kemnade.info> [210605 14:21]: > On Sat, 5 Jun 2021 08:18:14 +0300 > Tony Lindgren <tony@atomide.com> wrote: > > > * Andreas Kemnade <andreas@kemnade.info> [210604 17:00]: > > > On Fri, 4 Jun 2021 12:39:54 +0300 > > > Tony Lindgren <tony@atomide.com> wrote: > > > > Does the following patch fix things for you or does something else break again? :) > > > > > > > sigh,.. > > > ok, it breaks something. gadget (at least ecm) only works if > > > musb/phy stuff is loaded, ecm configured via configfs > > > rmmod omap2430 > > > modprube 2430 > > > > > > until the next usb disconnect > > > and another rmmod/modprobe is required. > > > > Hmm I don't follow, do you mean there's now another issue after a > > system suspend? Or is this issue not related to system suspend and > > resume? > > > independently of suspend. OK > > > The following musb patches were applied additional to that one you > > > added to this mail on top of 5.13-rc4. > > > > > > usb: musb: fix MUSB_QUIRK_B_DISCONNECT_99 handling > > > usb: musb: Add missing PM suspend and resume functions for 2430 glue > > > usb: musb: Check devctl status again for a spurious session request > > > > Does one of the above cause some additional usb gadget issue? > > > I do not see any additional gadget issue just with them. It just starts > with the small patch you proposed to fix suspend power consumption in > this thread. Oh OK, so some more mystery hardware debugging still left to do then. Regards, Tony
diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c --- a/drivers/usb/musb/musb_core.c +++ b/drivers/usb/musb/musb_core.c @@ -2055,6 +2055,15 @@ static void musb_pm_runtime_check_session(struct musb *musb) dev_err(musb->controller, "Could not enable: %i\n", error); musb->quirk_retries = 3; + + /* + * We can get a spurious MUSB_INTR_SESSREQ interrupt on start-up + * in B-peripheral mode with nothing connected and the session + * bit clears silently. Check status again in 3 seconds. + */ + if (devctl & MUSB_DEVCTL_BDEVICE) + schedule_delayed_work(&musb->irq_work, + msecs_to_jiffies(3000)); } else { musb_dbg(musb, "Allow PM with no session: %02x", devctl); pm_runtime_mark_last_busy(musb->controller);
On start-up, we can get a spurious session request interrupt with nothing connected. After that the devctl session bit will silently clear, but the musb hardware is never idled until a cable is plugged in, or the glue layer module is reloaded. Let's just check the session bit again in 3 seconds in peripheral mode to catch the issue. Signed-off-by: Tony Lindgren <tony@atomide.com> --- drivers/usb/musb/musb_core.c | 9 +++++++++ 1 file changed, 9 insertions(+)