* Théo Lebrun <theo.lebrun@xxxxxxxxxxx> [231122 14:47]: > Hi Kevin Hilman, Tony Lindgren & Thomas Richard, > > On Tue Oct 31, 2023 at 6:34 PM CET, Kevin Hilman wrote: > > Tony Lindgren <tony@xxxxxxxxxxx> writes: > > > * Thomas Richard <thomas.richard@xxxxxxxxxxx> [231031 10:15]: > > >> Please find below the logs of the test you asked me. > > > > > > OK great thanks! > > > > > >> I added the call of pm_runtime_get_usage_count at the end of the suspend > > >> function. > > >> The console is attached on 2800000.serial, it has usage_count=4. > > >> Other serial has usage_count=3. > > > > > > So as suspected, it seems the power domain gets force suspended > > > somewhere despite the usage_count. > > > > I think the only way this could happen (excluding any bugs, of course) > > would be for pm_runtime_force_suspend() to be getting called somewhere, > > but I thought the earlier patch made that call conditional, so maybe > > there is another path where that is getting called? > > I'm coming back on this topic as the upstream fix is less than ideal, as > everyone here was agreeing. Thanks for looking into it. > I've had a look at the genpd code & power-domains get powered-off at > suspend_noirq without caring about runtime PM refcounting. See > genpd_suspend_noirq & genpd_finish_suspend. > > Behavior is: > > - In all cases, call suspend_noirq on the underlying device. > - Stop now if device is in wakeup path & PD has the > GENPD_FLAG_ACTIVE_WAKEUP flag. > - If the PD has start & stop callbacks & is not runtime suspended, call > the stop callback on the device. > - Increment the count of suspended devices on this PD. > - If PD is already off or always on, stop. > - If this is the last device on this PD (and some other checks), then > do the PD power off (and maybe parent PDs). > > The current patch sets the PD as always on at suspend. That would not > work if our PD driver registered start/stop callbacks as those would > get called. > > The right solution to avoid getting the PD cut would be to mark the > serials we want to keep alive as on the wakeup path using > device_set_wakeup_path(dev) at suspend. That also requires the PD to be > marked using the GENPD_FLAG_ACTIVE_WAKEUP flag, which is currently not > the case. Not sure if we unconditionally want to set GENPD_FLAG_ACTIVE_WAKEUP as the pm_domain.h describes it with "Instructs genpd to keep the PM domain powered". The power domain should not force suspend automatically if there are active runtime PM users even without that flag.. > That last aspect is what I'm unsure of: should we add this flag to all > power-domains created by ti_sci_pm_domains? Should we pass something > from devicetree? I don't see the reason for not enabling this behavior > by default? To me this still seems like a workaround because of the active runtime PM usage count in the consumer driver :) And GENPD_FLAG_ACTIVE_WAKEUP should not need to be configured separately from the runtime PM usage count in this case. Sure there may be other cases where GENPD_FLAG_ACTIVE_WAKEUP needs to be set dynamically, but we should understand why the power domain code thinks it's OK to shut down the domain if it has active users. Regards, Tony
