diff mbox series

serial: 8250_omap: Set the console genpd always on if no console suspend

Message ID 20231017130540.1149721-1-thomas.richard@bootlin.com
State New
Headers show
Series serial: 8250_omap: Set the console genpd always on if no console suspend | expand

Commit Message

Thomas Richard Oct. 17, 2023, 1:05 p.m. UTC
If the console suspend is disabled, the genpd of the console shall not
be powered-off during suspend.
Set the flag GENPD_FLAG_ALWAYS_ON to the corresponding genpd during
suspend, and restore the original value during the resume.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/tty/serial/8250/8250_omap.c | 33 ++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

Comments

Tony Lindgren Oct. 23, 2023, 7:44 a.m. UTC | #1
Hi,

Adding Kevin and Vignesh too in case they have better ideas on how to
prevent the power domain from suspending for no_console_suspend kernel
parameter.

* Thomas Richard <thomas.richard@bootlin.com> [231017 13:05]:
> If the console suspend is disabled, the genpd of the console shall not
> be powered-off during suspend.
> Set the flag GENPD_FLAG_ALWAYS_ON to the corresponding genpd during
> suspend, and restore the original value during the resume.
> 
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> ---
>  drivers/tty/serial/8250/8250_omap.c | 33 ++++++++++++++++++++++++-----
>  1 file changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index ca972fd37725..91a483dc460c 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -27,6 +27,7 @@
>  #include <linux/pm_wakeirq.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/sys_soc.h>
> +#include <linux/pm_domain.h>
>  
>  #include "8250.h"
>  
> @@ -114,6 +115,12 @@
>  /* RX FIFO occupancy indicator */
>  #define UART_OMAP_RX_LVL		0x19
>  
> +/*
> + * Copy of the genpd flags for the console.
> + * Only used if console suspend is disabled
> + */
> +static unsigned int genpd_flags_console;

This should be priv->genpd_flags_console or something similar as the
uarts in an always-on power domain may have different flags from other
power domains.

Other than that I'm fine with with unless other people have better ideas
on how to handle this.

Regards,

Tony

>  struct omap8250_priv {
>  	void __iomem *membase;
>  	int line;
> @@ -1617,6 +1624,7 @@ static int omap8250_suspend(struct device *dev)
>  {
>  	struct omap8250_priv *priv = dev_get_drvdata(dev);
>  	struct uart_8250_port *up = serial8250_get_port(priv->line);
> +	struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
>  	int err = 0;
>  
>  	serial8250_suspend_port(priv->line);
> @@ -1627,8 +1635,19 @@ 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 {
> +			/*
> +			 * The pd shall not be powered-off (no console suspend).
> +			 * Make copy of genpd flags before to set it always on.
> +			 * The original value is restored during the resume.
> +			 */
> +			genpd_flags_console = genpd->flags;
> +			genpd->flags |= GENPD_FLAG_ALWAYS_ON;
> +		}
> +	}
>  	flush_work(&priv->qos_work);
>  
>  	return err;
> @@ -1638,12 +1657,16 @@ static int omap8250_resume(struct device *dev)
>  {
>  	struct omap8250_priv *priv = dev_get_drvdata(dev);
>  	struct uart_8250_port *up = serial8250_get_port(priv->line);
> +	struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
>  	int err;
>  
>  	if (uart_console(&up->port) && console_suspend_enabled) {
> -		err = pm_runtime_force_resume(dev);
> -		if (err)
> -			return err;
> +		if (console_suspend_enabled) {
> +			err = pm_runtime_force_resume(dev);
> +			if (err)
> +				return err;
> +		} else
> +			genpd->flags = genpd_flags_console;
>  	}
>  
>  	serial8250_resume_port(priv->line);
> -- 
> 2.39.2
>
Greg Kroah-Hartman Oct. 24, 2023, 3:24 p.m. UTC | #2
On Tue, Oct 24, 2023 at 04:53:46PM +0200, Thomas Richard wrote:
> On 10/23/23 09:44, Tony Lindgren wrote:
> > Hi,
> > 
> > Adding Kevin and Vignesh too in case they have better ideas on how to
> > prevent the power domain from suspending for no_console_suspend kernel
> > parameter.
> > 
> > * Thomas Richard <thomas.richard@bootlin.com> [231017 13:05]:
> >> If the console suspend is disabled, the genpd of the console shall not
> >> be powered-off during suspend.
> >> Set the flag GENPD_FLAG_ALWAYS_ON to the corresponding genpd during
> >> suspend, and restore the original value during the resume.
> >>
> >> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> >> ---
> >>  drivers/tty/serial/8250/8250_omap.c | 33 ++++++++++++++++++++++++-----
> >>  1 file changed, 28 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> >> index ca972fd37725..91a483dc460c 100644
> >> --- a/drivers/tty/serial/8250/8250_omap.c
> >> +++ b/drivers/tty/serial/8250/8250_omap.c
> >> @@ -27,6 +27,7 @@
> >>  #include <linux/pm_wakeirq.h>
> >>  #include <linux/dma-mapping.h>
> >>  #include <linux/sys_soc.h>
> >> +#include <linux/pm_domain.h>
> >>  
> >>  #include "8250.h"
> >>  
> >> @@ -114,6 +115,12 @@
> >>  /* RX FIFO occupancy indicator */
> >>  #define UART_OMAP_RX_LVL		0x19
> >>  
> >> +/*
> >> + * Copy of the genpd flags for the console.
> >> + * Only used if console suspend is disabled
> >> + */
> >> +static unsigned int genpd_flags_console;
> > 
> > This should be priv->genpd_flags_console or something similar as the
> > uarts in an always-on power domain may have different flags from other
> > power domains.
> 
> Ok I'll move genpd_flags_console to the priv struct.
> 
> @Greg, as you already added the patch to your tty git tree, do you
> prefer a new version of the patch or a fixup ?

A fixup please.

thanks,

greg k-h
Kevin Hilman Oct. 24, 2023, 6:36 p.m. UTC | #3
Tony Lindgren <tony@atomide.com> writes:

> * Kevin Hilman <khilman@kernel.org> [231023 21:31]:
>> Instead, what should be happening is that when `no_console_suspend` is
>> set, there should be an extra pm_runtime_get() which increases the
>> device usecount such that the device never runtime suspends, and thus
>> the domain will not get powered off.
>
> We already have the runtime PM usage count kept in the driver (unless
> there's a bug somewhere). The issue is that on suspend the power domain
> still gets shut down.
>
> I suspect that some of the SoC power domains get
> force shut down on suspend somewhere?

If setting GENPD_FLAG_ALWAYS_ON works as this patch proposes, then a
force shutdown would override that genpd flag also, so I suspect the 
runtime PM usage count is not correct.

I quick skim of 8250_omap.c, and I don't see any pm_runtime_get() calls
that are conditional on no_console_suspend, which is what I would
suspect for the domain to stay on.

Kevin
Tony Lindgren Oct. 25, 2023, 6:41 a.m. UTC | #4
* Kevin Hilman <khilman@kernel.org> [231024 18:36]:
> Tony Lindgren <tony@atomide.com> writes:
> 
> > * Kevin Hilman <khilman@kernel.org> [231023 21:31]:
> >> Instead, what should be happening is that when `no_console_suspend` is
> >> set, there should be an extra pm_runtime_get() which increases the
> >> device usecount such that the device never runtime suspends, and thus
> >> the domain will not get powered off.
> >
> > We already have the runtime PM usage count kept in the driver (unless
> > there's a bug somewhere). The issue is that on suspend the power domain
> > still gets shut down.
> >
> > I suspect that some of the SoC power domains get
> > force shut down on suspend somewhere?
> 
> If setting GENPD_FLAG_ALWAYS_ON works as this patch proposes, then a
> force shutdown would override that genpd flag also, so I suspect the 
> runtime PM usage count is not correct.

OK good point.

> I quick skim of 8250_omap.c, and I don't see any pm_runtime_get() calls
> that are conditional on no_console_suspend, which is what I would
> suspect for the domain to stay on.

If a serial console is attached, we now have runtime PM usage count
always kept. Users can detach the console via sysfs as needed. See these
two earlier commits from Andy:

a3cb39d258ef ("serial: core: Allow detach and attach serial device for console")
bedb404e91bb ("serial: 8250_port: Don't use power management for kernel console")

Sounds like there's a bug somewhere. It's worth verifying if the runtime
PM usage count is kept for 8250_omap on suspend.

Thomas, care to check your test case with the attached debug hack
and by adding a call for pm_runtime_get_usage_count() on the suspend
path?

Regards,

Tony

8< -------------------------------
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -129,6 +129,11 @@ static inline void pm_runtime_get_noresume(struct device *dev)
 	atomic_inc(&dev->power.usage_count);
 }
 
+static inline int pm_runtime_get_usage_count(struct device *dev)
+{
+	return atomic_read(&dev->power.usage_count);
+}
+
 /**
  * pm_runtime_put_noidle - Drop runtime PM usage counter of a device.
  * @dev: Target device.
Thomas Richard Oct. 31, 2023, 10:15 a.m. UTC | #5
On 10/25/23 08:41, Tony Lindgren wrote:
> * Kevin Hilman <khilman@kernel.org> [231024 18:36]:
>> Tony Lindgren <tony@atomide.com> writes:
>>
>>> * Kevin Hilman <khilman@kernel.org> [231023 21:31]:
>>>> Instead, what should be happening is that when `no_console_suspend` is
>>>> set, there should be an extra pm_runtime_get() which increases the
>>>> device usecount such that the device never runtime suspends, and thus
>>>> the domain will not get powered off.
>>>
>>> We already have the runtime PM usage count kept in the driver (unless
>>> there's a bug somewhere). The issue is that on suspend the power domain
>>> still gets shut down.
>>>
>>> I suspect that some of the SoC power domains get
>>> force shut down on suspend somewhere?
>>
>> If setting GENPD_FLAG_ALWAYS_ON works as this patch proposes, then a
>> force shutdown would override that genpd flag also, so I suspect the 
>> runtime PM usage count is not correct.
> 
> OK good point.
> 
>> I quick skim of 8250_omap.c, and I don't see any pm_runtime_get() calls
>> that are conditional on no_console_suspend, which is what I would
>> suspect for the domain to stay on.
> 
> If a serial console is attached, we now have runtime PM usage count
> always kept. Users can detach the console via sysfs as needed. See these
> two earlier commits from Andy:
> 
> a3cb39d258ef ("serial: core: Allow detach and attach serial device for console")
> bedb404e91bb ("serial: 8250_port: Don't use power management for kernel console")
> 
> Sounds like there's a bug somewhere. It's worth verifying if the runtime
> PM usage count is kept for 8250_omap on suspend.
> 
> Thomas, care to check your test case with the attached debug hack
> and by adding a call for pm_runtime_get_usage_count() on the suspend
> path?

Hi Tony,

Please find below the logs of the test you asked me.
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.

[    4.859058] port 2830000.serial:0.0: PM: calling
pm_runtime_force_suspend+0x0/0x134 @ 114, parent: 2830000.serial:0
[    4.869478] port 2830000.serial:0.0: PM:
pm_runtime_force_suspend+0x0/0x134 returned 0 after 0 usecs
[    4.878602] omap8250 2830000.serial: PM: calling
omap8250_suspend+0x0/0x144 @ 114, parent: bus@100000
[    4.887813] omap8250 2830000.serial: omap8250_suspend: 1634:
usage_count = 3
[    4.894851] omap8250 2830000.serial: PM: omap8250_suspend+0x0/0x144
returned 0 after 7042 usecs
[    4.903538] port 2810000.serial:0.0: PM: calling
pm_runtime_force_suspend+0x0/0x134 @ 114, parent: 2810000.serial:0
[    4.913957] port 2810000.serial:0.0: PM:
pm_runtime_force_suspend+0x0/0x134 returned 0 after 0 usecs
[    4.923080] omap8250 2810000.serial: PM: calling
omap8250_suspend+0x0/0x144 @ 114, parent: bus@100000
[    4.932288] omap8250 2810000.serial: omap8250_suspend: 1634:
usage_count = 3
[    4.939323] omap8250 2810000.serial: PM: omap8250_suspend+0x0/0x144
returned 0 after 7038 usecs
[    4.948010] port 2800000.serial:0.0: PM: calling
pm_runtime_force_suspend+0x0/0x134 @ 114, parent: 2800000.serial:0
[    4.958433] port 2800000.serial:0.0: PM:
pm_runtime_force_suspend+0x0/0x134 returned 0 after 1 usecs
[    4.967557] omap8250 2800000.serial: PM: calling
omap8250_suspend+0x0/0x144 @ 114, parent: bus@100000
[    4.976764] omap8250 2800000.serial: omap8250_suspend: 1634:
usage_count = 4
[    4.983799] omap8250 2800000.serial: PM: omap8250_suspend+0x0/0x144
returned 0 after 7036 usecs
[    4.992488] port 40a00000.serial:0.0: PM: calling
pm_runtime_force_suspend+0x0/0x134 @ 114, parent: 40a00000.serial:0
[    5.003081] port 40a00000.serial:0.0: PM:
pm_runtime_force_suspend+0x0/0x134 returned 0 after 0 usecs
[    5.012291] omap8250 40a00000.serial: PM: calling
omap8250_suspend+0x0/0x144 @ 114, parent: bus@100000:bus@28380000
[    5.022714] omap8250 40a00000.serial: omap8250_suspend: 1634:
usage_count = 3
[    5.029836] omap8250 40a00000.serial: PM: omap8250_suspend+0x0/0x144
returned 0 after 7124 usecs

Regards,

Thomas

8< -------------------------------
diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index ca972fd37725..b978f12fd542 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -1631,6 +1631,9 @@ static int omap8250_suspend(struct device *dev)
                err = pm_runtime_force_suspend(dev);
        flush_work(&priv->qos_work);

+       dev_info(dev, "%s: %d: usage_count = %d\n",
+                __func__, __LINE__, pm_runtime_get_usage_count(dev));
+
        return err;
 }


> 
> Regards,
> 
> Tony
> 
> 8< -------------------------------
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -129,6 +129,11 @@ static inline void pm_runtime_get_noresume(struct device *dev)
>  	atomic_inc(&dev->power.usage_count);
>  }
>  
> +static inline int pm_runtime_get_usage_count(struct device *dev)
> +{
> +	return atomic_read(&dev->power.usage_count);
> +}
> +
>  /**
>   * pm_runtime_put_noidle - Drop runtime PM usage counter of a device.
>   * @dev: Target device.
Kevin Hilman Oct. 31, 2023, 5:34 p.m. UTC | #6
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?

Kevin
Tony Lindgren Nov. 24, 2023, 5:37 a.m. UTC | #7
* 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
Théo Lebrun Nov. 24, 2023, 10:39 a.m. UTC | #8
Hello,

On Fri Nov 24, 2023 at 6:37 AM CET, Tony Lindgren wrote:
> * 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..

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
Tony Lindgren Nov. 24, 2023, 10:54 a.m. UTC | #9
* Théo Lebrun <theo.lebrun@bootlin.com> [231124 10:39]:
> On Fri Nov 24, 2023 at 6:37 AM CET, Tony Lindgren wrote:
> 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.

Setting GENPD_FLAG_ACTIVE_WAKEUP will block deeper idle states for
the SoC most likely.

> 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.

For managing GENPD_FLAG_ACTIVE_WAKEUP dynamically, maybe something like:

device_request_wakeup_path(dev)
...
device_free_wakeup_path(dev)

And those would toggle GENPD_FLAG_ACTIVE_WAKEUP for the power domain
based on some usage count?

Regards,

Tony
diff mbox series

Patch

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index ca972fd37725..91a483dc460c 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -27,6 +27,7 @@ 
 #include <linux/pm_wakeirq.h>
 #include <linux/dma-mapping.h>
 #include <linux/sys_soc.h>
+#include <linux/pm_domain.h>
 
 #include "8250.h"
 
@@ -114,6 +115,12 @@ 
 /* RX FIFO occupancy indicator */
 #define UART_OMAP_RX_LVL		0x19
 
+/*
+ * Copy of the genpd flags for the console.
+ * Only used if console suspend is disabled
+ */
+static unsigned int genpd_flags_console;
+
 struct omap8250_priv {
 	void __iomem *membase;
 	int line;
@@ -1617,6 +1624,7 @@  static int omap8250_suspend(struct device *dev)
 {
 	struct omap8250_priv *priv = dev_get_drvdata(dev);
 	struct uart_8250_port *up = serial8250_get_port(priv->line);
+	struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
 	int err = 0;
 
 	serial8250_suspend_port(priv->line);
@@ -1627,8 +1635,19 @@  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 {
+			/*
+			 * The pd shall not be powered-off (no console suspend).
+			 * Make copy of genpd flags before to set it always on.
+			 * The original value is restored during the resume.
+			 */
+			genpd_flags_console = genpd->flags;
+			genpd->flags |= GENPD_FLAG_ALWAYS_ON;
+		}
+	}
 	flush_work(&priv->qos_work);
 
 	return err;
@@ -1638,12 +1657,16 @@  static int omap8250_resume(struct device *dev)
 {
 	struct omap8250_priv *priv = dev_get_drvdata(dev);
 	struct uart_8250_port *up = serial8250_get_port(priv->line);
+	struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
 	int err;
 
 	if (uart_console(&up->port) && console_suspend_enabled) {
-		err = pm_runtime_force_resume(dev);
-		if (err)
-			return err;
+		if (console_suspend_enabled) {
+			err = pm_runtime_force_resume(dev);
+			if (err)
+				return err;
+		} else
+			genpd->flags = genpd_flags_console;
 	}
 
 	serial8250_resume_port(priv->line);