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

[Andreas Kemnade <andreas@kemnade.info>]

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