Hello, On Fri Nov 24, 2023 at 6:37 AM CET, Tony Lindgren wrote: > * 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.. Hey don't forgot the important part! > Instructs genpd to keep the PM domain powered on, in case any of its > attached devices is used in the wakeup path to serve system wakeups. Checking the code confirms this behavior. Grep for the macro genpd_is_active_wakeup rather than GENPD_FLAG_ACTIVE_WAKEUP. It gets used twice (suspend & resume), both in the same manner: if (device_wakeup_path(dev) && genpd_is_active_wakeup(genpd)) This means this flag won't have any impact on runtime PM handling of power-domains. By the way, other users of the flag enable it at PD registration & don't touch it afterwards. > > 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. There is currently nothing that links the runtime PM refcount to whether or not the power-domains get powered-off at suspend_noirq. Should that change? Maybe, but it would be a big behavioral change. Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
