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. 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. 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? I've tested this approach & it works. See below for diff. Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com 8< ------------------------------- diff --git a/drivers/pmdomain/ti/ti_sci_pm_domains.c b/drivers/pmdomain/ti/ti_sci_pm_domains.c index c091d569ecd5..b0d473776278 100644 --- a/drivers/pmdomain/ti/ti_sci_pm_domains.c +++ b/drivers/pmdomain/ti/ti_sci_pm_domains.c @@ -168,6 +168,7 @@ static int ti_sci_pm_domain_probe(struct platform_device *pdev) pd->pd.power_off = ti_sci_pd_power_off; pd->pd.power_on = ti_sci_pd_power_on; + pd->pd.flags |= GENPD_FLAG_ACTIVE_WAKEUP; pd->idx = args.args[0]; pd->parent = pd_provider; diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c index 4b33f4529aed..ab2f8b7927c3 100644 --- a/drivers/tty/serial/8250/8250_omap.c +++ b/drivers/tty/serial/8250/8250_omap.c @@ -1633,8 +1633,12 @@ static int omap8250_suspend(struct device *dev) if (!device_may_wakeup(dev)) priv->wer = 0; serial_out(up, UART_OMAP_WER, priv->wer); - if (uart_console(&up->port) && console_suspend_enabled) - err = pm_runtime_force_suspend(dev); + if (uart_console(&up->port)) { + if (console_suspend_enabled) + err = pm_runtime_force_suspend(dev); + else + device_set_wakeup_path(dev); + } flush_work(&priv->qos_work); return err;
