[PATCH] usb: musb: Check devctl status again for a spurious session request

[PATCH] usb: musb: Check devctl status again for a spurious session request

[Tony Lindgren]

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(+)

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);
-- 
2.31.1

Re: [PATCH] usb: musb: Check devctl status again for a spurious session request

[Andreas Kemnade]

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

Re: [PATCH] usb: musb: Check devctl status again for a spurious session request

[Tony Lindgren]

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

Re: [PATCH] usb: musb: Check devctl status again for a spurious session request

[Andreas Kemnade]

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

Re: [PATCH] usb: musb: Check devctl status again for a spurious session request

[Tony Lindgren]

* 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

Re: [PATCH] usb: musb: Check devctl status again for a spurious session request

[Andreas Kemnade]

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

Re: [PATCH] usb: musb: Check devctl status again for a spurious session request

[Andreas Kemnade]

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

Re: [PATCH] usb: musb: Check devctl status again for a spurious session request

[Tony Lindgren]

* 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);
 
-- 
2.31.1

Re: [PATCH] usb: musb: Check devctl status again for a spurious session request

[Andreas Kemnade]

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

Re: [PATCH] usb: musb: Check devctl status again for a spurious session request

[Tony Lindgren]

* 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

Re: [PATCH] usb: musb: Check devctl status again for a spurious session request

[Andreas Kemnade]

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

Re: [PATCH] usb: musb: Check devctl status again for a spurious session request

[Andreas Kemnade]

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);
>  

Re: [PATCH] usb: musb: Check devctl status again for a spurious session request

[Andreas Kemnade]

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

Re: [PATCH] usb: musb: Check devctl status again for a spurious session request

[Tony Lindgren]

* 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

Re: [PATCH] usb: musb: Check devctl status again for a spurious session request

[Andreas Kemnade]

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

Re: [PATCH] usb: musb: Check devctl status again for a spurious session request

[Tony Lindgren]

* 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