Re: [PATCH] serial: 8250_omap: Set the console genpd always on if no console suspend

[Tony Lindgren <tony@atomide.com>]

* Théo Lebrun <theo.lebrun@bootlin.com> [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@atomide.com> writes:
> > > * Thomas Richard <thomas.richard@bootlin.com> [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