OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Johan Hovold]

[ Updating subject and adding linux-serial, linux-pm and linux-omap. ]

On Tue, May 08, 2018 at 09:49:04AM -0700, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [180508 08:54]:
> > * Tony Lindgren <tony@atomide.com> [180508 08:47]:
> > > * Tony Lindgren <tony@atomide.com> [180508 08:22]:
> > > > * Johan Hovold <johan@kernel.org> [180508 07:00]:
> > > > > With the negative autosuspend set in both omap drivers probe functions,
> > > > > this is the expected behaviour. Which I think we must fix.
> > > > 
> > > > Yes indeed. I've been using my script for years now and have
> > > > completely missed the fact that the unused ports are not idled
> > > > at all on start-up when unused.
> > > 
> > > This might be all that's needed, care to try it and if it works
> > > I'll send out two separate proper patches?
> > > 
> > > I'm seeing this now on my bbb after temporarily disabling my
> > > UART idle init script:
> > > 
> > > # rwmem -s32 0x44e004b4           # uart 1 on l4_wkup
> > > 0x44e004b4 = 0x00000002
> > > 
> > > # rwmem -s32 0x44e0006c+0x10      # uart 2 - 5 on l4_per
> > > 0x44e0006c = 0x00030000
> > > 0x44e00070 = 0x00030000
> > > 0x44e00074 = 0x00030000
> > > 0x44e00078 = 0x00030000
> > > 
> > > # rwmem -s32 0x44e00038           # uart 6 on l4_per
> > > 0x44e00038 = 0x00030000
> > 
> > Hmm I forgot to remove status = "disabled" from the other ports,
> > still not idling the unused ports with the patch below it seems.
> 
> No need to enable/disable autosuspend except in startup and shutdown
> I think. The patch below works for me, now includes removal of the
> status = "disabled" flags too. Only tested with 8250_omap on bbb
> so far.
> 
> I wonder if other places still need fixing for autosuspend
> like console support?

While this seems to fix the idling of closed ports here too, I'm not
sure we can move use_autosuspend to startup() like this.

First, this flag should be set before registering the tty so that udev
can be used to update the attributes.

Second, this prevents setting the autosuspend delay through sysfs when
the port is closed (when autosuspend is disabled).

It seems we really should not be using the negative autosuspend to
configure the RPM behaviour the way these drivers do. Perhaps a new
mechanism is needed.

But I'm afraid I don't have time to look at this today.

Thanks,
Johan

> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -605,6 +605,7 @@ static int omap_8250_startup(struct uart_port *port)
>  			return ret;
>  	}
>  
> +	pm_runtime_use_autosuspend(port->dev);
>  	pm_runtime_get_sync(port->dev);
>  
>  	up->mcr = 0;
> @@ -685,8 +686,8 @@ static void omap_8250_shutdown(struct uart_port *port)
>  		serial_out(up, UART_LCR, up->lcr & ~UART_LCR_SBC);
>  	serial_out(up, UART_FCR, UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
>  
> -	pm_runtime_mark_last_busy(port->dev);
> -	pm_runtime_put_autosuspend(port->dev);
> +	pm_runtime_dont_use_autosuspend(port->dev);
> +	pm_runtime_put_sync(port->dev);
>  	free_irq(port->irq, port);
>  	dev_pm_clear_wake_irq(port->dev);
>  }
> @@ -1226,7 +1227,7 @@ static int omap8250_probe(struct platform_device *pdev)
>  	spin_lock_init(&priv->rx_dma_lock);
>  
>  	device_init_wakeup(&pdev->dev, true);
> -	pm_runtime_use_autosuspend(&pdev->dev);
> +	/* See omap_8250_startup and shutdown for autosuspend */
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
>  
>  	pm_runtime_irq_safe(&pdev->dev);
> @@ -1265,8 +1266,8 @@ static int omap8250_probe(struct platform_device *pdev)
>  	}
>  	priv->line = ret;
>  	platform_set_drvdata(pdev, priv);
> -	pm_runtime_mark_last_busy(&pdev->dev);
> -	pm_runtime_put_autosuspend(&pdev->dev);
> +	pm_runtime_put_sync(&pdev->dev);
> +
>  	return 0;
>  err:

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Tony Lindgren]

* Johan Hovold <johan@kernel.org> [180509 13:12]:
> While this seems to fix the idling of closed ports here too, I'm not
> sure we can move use_autosuspend to startup() like this.
> 
> First, this flag should be set before registering the tty so that udev
> can be used to update the attributes.
> 
> Second, this prevents setting the autosuspend delay through sysfs when
> the port is closed (when autosuspend is disabled).

Yeah I noticed that too yesterday.

> It seems we really should not be using the negative autosuspend to
> configure the RPM behaviour the way these drivers do. Perhaps a new
> mechanism is needed.

Hmm well simply defaulting to "on" instead of "auto" and setting the
autosuspend_ms to 3000 by default might be doable. I think that way
we can keep use_autosuspend() in probe. Let's hope there are no
existing use cases that would break with that.

> But I'm afraid I don't have time to look at this today.

Sure np thanks,

Tony

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Johan Hovold]

[ Sorry about the late reply. ]

On Wed, May 09, 2018 at 06:57:06AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [180509 13:12]:

> > It seems we really should not be using the negative autosuspend to
> > configure the RPM behaviour the way these drivers do. Perhaps a new
> > mechanism is needed.
> 
> Hmm well simply defaulting to "on" instead of "auto" and setting the
> autosuspend_ms to 3000 by default might be doable. I think that way
> we can keep use_autosuspend() in probe. Let's hope there are no
> existing use cases that would break with that.

No, defaulting to "on" (i.e. calling pm_runtime_forbid()) wouldn't work
either as that would also prevent the device from runtime suspending
just as the current negative autosuspend delay does.

I fail to see how we can implement this using the current toolbox. What
you're after here is really a mechanism for selecting between two
different runtime PM schemes at runtime:

	1. normal serial RPM, where the controller is active while the
	   port is open (this should be the safe default)

	2. aggressive serial RPM, where the controller is allowed to
	   suspend while the port is open even though this may result in
	   lost characters when waking up on incoming data

For normal ttys, we need a user-space interface for selecting between
the two, and for serdev we may want a way to select the RPM scheme from
within the kernel.

Note that with my serdev controller runtime PM patch, serdev core could
always opt for aggressive PM (as by default serdev core holds an RPM
reference for the controller while the port is open).

Thanks,
Johan

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Tony Lindgren]

* Johan Hovold <johan@kernel.org> [180517 10:12]:
> [ Sorry about the late reply. ]
> 
> On Wed, May 09, 2018 at 06:57:06AM -0700, Tony Lindgren wrote:
> > * Johan Hovold <johan@kernel.org> [180509 13:12]:
> 
> > > It seems we really should not be using the negative autosuspend to
> > > configure the RPM behaviour the way these drivers do. Perhaps a new
> > > mechanism is needed.
> > 
> > Hmm well simply defaulting to "on" instead of "auto" and setting the
> > autosuspend_ms to 3000 by default might be doable. I think that way
> > we can keep use_autosuspend() in probe. Let's hope there are no
> > existing use cases that would break with that.
> 
> No, defaulting to "on" (i.e. calling pm_runtime_forbid()) wouldn't work
> either as that would also prevent the device from runtime suspending
> just as the current negative autosuspend delay does.

Well in that case we should just stick with -1 value for the
autosuspend. And just do pm_runtime_put_sync_suspend() after
probe and on close.

> I fail to see how we can implement this using the current toolbox. What
> you're after here is really a mechanism for selecting between two
> different runtime PM schemes at runtime:
> 
> 	1. normal serial RPM, where the controller is active while the
> 	   port is open (this should be the safe default)

Agreed. And that is the case already.

> 	2. aggressive serial RPM, where the controller is allowed to
> 	   suspend while the port is open even though this may result in
> 	   lost characters when waking up on incoming data

In this case it seems that the only thing needed is to just
configure the autosuspend delay for the parent port. The use of
-1 has been around since the start of runtime PM AFAIK, so maybe
we should just document it. I guess we could also introduce
pm_runtime_block_autoidle_unless_configured() :)

> For normal ttys, we need a user-space interface for selecting between
> the two, and for serdev we may want a way to select the RPM scheme from
> within the kernel.
> 
> Note that with my serdev controller runtime PM patch, serdev core could
> always opt for aggressive PM (as by default serdev core holds an RPM
> reference for the controller while the port is open).

So if your serdev controller was to set the parent autosuspend
delay on open() and set it back on close() this should work?

Regards,

Tony

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Johan Hovold]

On Thu, May 17, 2018 at 10:10:38AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [180517 10:12]:
> > [ Sorry about the late reply. ]
> > 
> > On Wed, May 09, 2018 at 06:57:06AM -0700, Tony Lindgren wrote:
> > > * Johan Hovold <johan@kernel.org> [180509 13:12]:
> > 
> > > > It seems we really should not be using the negative autosuspend to
> > > > configure the RPM behaviour the way these drivers do. Perhaps a new
> > > > mechanism is needed.
> > > 
> > > Hmm well simply defaulting to "on" instead of "auto" and setting the
> > > autosuspend_ms to 3000 by default might be doable. I think that way
> > > we can keep use_autosuspend() in probe. Let's hope there are no
> > > existing use cases that would break with that.
> > 
> > No, defaulting to "on" (i.e. calling pm_runtime_forbid()) wouldn't work
> > either as that would also prevent the device from runtime suspending
> > just as the current negative autosuspend delay does.
> 
> Well in that case we should just stick with -1 value for the
> autosuspend. And just do pm_runtime_put_sync_suspend() after
> probe and on close.

That won't work either as a negative autosuspend delay prevents runtime
suspend completely (by grabbing an extra RPM reference).

> > I fail to see how we can implement this using the current toolbox. What
> > you're after here is really a mechanism for selecting between two
> > different runtime PM schemes at runtime:
> > 
> > 	1. normal serial RPM, where the controller is active while the
> > 	   port is open (this should be the safe default)
> 
> Agreed. And that is the case already.

Yes, but it's not really the case today as since omap-serial (and
8250-omap) sets a negative autosuspend at probe and hence does not
runtime-suspend when the port is closed. So that's the long-standing bug
which needs fixing.

> > 	2. aggressive serial RPM, where the controller is allowed to
> > 	   suspend while the port is open even though this may result in
> > 	   lost characters when waking up on incoming data
> 
> In this case it seems that the only thing needed is to just
> configure the autosuspend delay for the parent port. The use of
> -1 has been around since the start of runtime PM AFAIK, so maybe
> we should just document it. I guess we could also introduce
> pm_runtime_block_autoidle_unless_configured() :)

The implications of a negative autosuspend delay are already documented
(in Documentation/power/runtime_pm.txt); it's just the omap drivers that
gets it wrong when trying to do things which aren't currently supported
(and never have been).

So I still think we need a new mechanism for this.

> > For normal ttys, we need a user-space interface for selecting between
> > the two, and for serdev we may want a way to select the RPM scheme from
> > within the kernel.
> > 
> > Note that with my serdev controller runtime PM patch, serdev core could
> > always opt for aggressive PM (as by default serdev core holds an RPM
> > reference for the controller while the port is open).
> 
> So if your serdev controller was to set the parent autosuspend
> delay on open() and set it back on close() this should work?

Is it really the job of a serdev driver to set the autosuspend delay of
a parent controller? Isn't this somethings which depends on the
characteristics of the controller (possibly configurable by user space)
such as the cost of runtime suspending and resuming?

The patch I posted works with what we have today; if a parent serial
controller driver uses aggressive runtime PM by default or after having
been configured through sysfs to do so.

What I'm getting at here is that the delay should be set by the serial
driver implementing aggressive runtime PM. Then all we need is a
mechanism to determine whether an extra RPM reference should be taken at
tty open or not (configurable by user space, defaulting to yes).

Specifically, the serial drivers themselves would always use
autosuspend and not have to deal with supporting the two RPM schemes
(normal vs aggressive runtime PM).

Thanks,
Johan

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Tony Lindgren]

* Johan Hovold <johan@kernel.org> [180521 13:50]:
> On Thu, May 17, 2018 at 10:10:38AM -0700, Tony Lindgren wrote:
> > * Johan Hovold <johan@kernel.org> [180517 10:12]:
> > > No, defaulting to "on" (i.e. calling pm_runtime_forbid()) wouldn't work
> > > either as that would also prevent the device from runtime suspending
> > > just as the current negative autosuspend delay does.
> > 
> > Well in that case we should just stick with -1 value for the
> > autosuspend. And just do pm_runtime_put_sync_suspend() after
> > probe and on close.
> 
> That won't work either as a negative autosuspend delay prevents runtime
> suspend completely (by grabbing an extra RPM reference).

Well so negative autosuspend delay is working as documented then :)

> > > I fail to see how we can implement this using the current toolbox. What
> > > you're after here is really a mechanism for selecting between two
> > > different runtime PM schemes at runtime:
> > > 
> > > 	1. normal serial RPM, where the controller is active while the
> > > 	   port is open (this should be the safe default)
> > 
> > Agreed. And that is the case already.
> 
> Yes, but it's not really the case today as since omap-serial (and
> 8250-omap) sets a negative autosuspend at probe and hence does not
> runtime-suspend when the port is closed. So that's the long-standing bug
> which needs fixing.

Yes the bug for closed ports needs to be fixed for sure.

> > > 	2. aggressive serial RPM, where the controller is allowed to
> > > 	   suspend while the port is open even though this may result in
> > > 	   lost characters when waking up on incoming data
> > 
> > In this case it seems that the only thing needed is to just
> > configure the autosuspend delay for the parent port. The use of
> > -1 has been around since the start of runtime PM AFAIK, so maybe
> > we should just document it. I guess we could also introduce
> > pm_runtime_block_autoidle_unless_configured() :)
> 
> The implications of a negative autosuspend delay are already documented
> (in Documentation/power/runtime_pm.txt); it's just the omap drivers that
> gets it wrong when trying to do things which aren't currently supported
> (and never have been).
> 
> So I still think we need a new mechanism for this.

Well if you have some better mechanism in mind let's try it out. Short of
sprinkling pm_runtime_force_suspend/resume calls all over, I'm out of ideas
right now.

> > > For normal ttys, we need a user-space interface for selecting between
> > > the two, and for serdev we may want a way to select the RPM scheme from
> > > within the kernel.
> > > 
> > > Note that with my serdev controller runtime PM patch, serdev core could
> > > always opt for aggressive PM (as by default serdev core holds an RPM
> > > reference for the controller while the port is open).
> > 
> > So if your serdev controller was to set the parent autosuspend
> > delay on open() and set it back on close() this should work?
> 
> Is it really the job of a serdev driver to set the autosuspend delay of
> a parent controller? Isn't this somethings which depends on the
> characteristics of the controller (possibly configurable by user space)
> such as the cost of runtime suspending and resuming?

Only in some cases will the serdev driver know it's safe to configure
the parent controller. Configuring the parent controller from userspace
works just fine as we've seen for years now.

> The patch I posted works with what we have today; if a parent serial
> controller driver uses aggressive runtime PM by default or after having
> been configured through sysfs to do so.

Yeah let's stick with configuring the parent controller from userspace
for now at least.

> What I'm getting at here is that the delay should be set by the serial
> driver implementing aggressive runtime PM. Then all we need is a
> mechanism to determine whether an extra RPM reference should be taken at
> tty open or not (configurable by user space, defaulting to yes).

OK yeah some additional on/off switch seems to be missing here.

> Specifically, the serial drivers themselves would always use
> autosuspend and not have to deal with supporting the two RPM schemes
> (normal vs aggressive runtime PM).

OK. So if I understand your idea right, we could have autosuspend timeout
set to 3000ms in the 8250_omap.c but still default to RPM blocked?
Then user can enable aggressive PM via /sys as desired, right?

Regards,

Tony

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Johan Hovold]

On Mon, May 21, 2018 at 08:48:32AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [180521 13:50]:
> > On Thu, May 17, 2018 at 10:10:38AM -0700, Tony Lindgren wrote:
> > > * Johan Hovold <johan@kernel.org> [180517 10:12]:

> > > Well in that case we should just stick with -1 value for the
> > > autosuspend. And just do pm_runtime_put_sync_suspend() after
> > > probe and on close.
> > 
> > That won't work either as a negative autosuspend delay prevents runtime
> > suspend completely (by grabbing an extra RPM reference).
> 
> Well so negative autosuspend delay is working as documented then :)

Yes, indeed. All too well. ;)

> > > > I fail to see how we can implement this using the current toolbox. What
> > > > you're after here is really a mechanism for selecting between two
> > > > different runtime PM schemes at runtime:
> > > > 
> > > > 	1. normal serial RPM, where the controller is active while the
> > > > 	   port is open (this should be the safe default)
> > > 
> > > Agreed. And that is the case already.
> > 
> > Yes, but it's not really the case today as since omap-serial (and
> > 8250-omap) sets a negative autosuspend at probe and hence does not
> > runtime-suspend when the port is closed. So that's the long-standing bug
> > which needs fixing.
> 
> Yes the bug for closed ports needs to be fixed for sure.

I did some forensic on this and it seems this problem has "always" been
there. Specifically, closed ports have never been runtime suspended
unless a non-negative autosuspend delay has been set by user space since
fcdca75728ac ("ARM: OMAP2+: UART: Add runtime pm support for omap-serial
driver") which was merged seven years ago.

So while it would certainly be nice to save some more power by default,
this would really be a new feature rather than a bug or regression fix
(which reduces the urgency for this issue somewhat too).

> > > > 	2. aggressive serial RPM, where the controller is allowed to
> > > > 	   suspend while the port is open even though this may result in
> > > > 	   lost characters when waking up on incoming data
> > > 
> > > In this case it seems that the only thing needed is to just
> > > configure the autosuspend delay for the parent port. The use of
> > > -1 has been around since the start of runtime PM AFAIK, so maybe
> > > we should just document it. I guess we could also introduce
> > > pm_runtime_block_autoidle_unless_configured() :)
> > 
> > The implications of a negative autosuspend delay are already documented
> > (in Documentation/power/runtime_pm.txt); it's just the omap drivers that
> > gets it wrong when trying to do things which aren't currently supported
> > (and never have been).
> > 
> > So I still think we need a new mechanism for this.
> 
> Well if you have some better mechanism in mind let's try it out. Short of
> sprinkling pm_runtime_force_suspend/resume calls all over, I'm out of ideas
> right now.

Yeah, that would be too much of a hack and likely wouldn't work either
(and we really should do away with those _force calls altogether).

I've been thinking a bit too much about this already, but it may be
possible to use the pm QoS framework for this. A resume latency can be
set through sysfs where "n/a" is defined to mean "no latency accepted"
(i.e. controller remains always-on while port is open) and "0" means
"any latency accepted" (i.e. omap aggressive serial RPM is allowed).

Now, implementing this may get a little tricky as we want to be able to
change this setting on the fly (consider consoles) and we need to figure
out the interaction with serdev (user space should probably not be
allowed to request a resume latency for ports used by serdev).

I'd be happy to dig into this some more, but not in my spare time I'm
afraid.

> > > > For normal ttys, we need a user-space interface for selecting between
> > > > the two, and for serdev we may want a way to select the RPM scheme from
> > > > within the kernel.
> > > > 
> > > > Note that with my serdev controller runtime PM patch, serdev core could
> > > > always opt for aggressive PM (as by default serdev core holds an RPM
> > > > reference for the controller while the port is open).
> > > 
> > > So if your serdev controller was to set the parent autosuspend
> > > delay on open() and set it back on close() this should work?
> > 
> > Is it really the job of a serdev driver to set the autosuspend delay of
> > a parent controller? Isn't this somethings which depends on the
> > characteristics of the controller (possibly configurable by user space)
> > such as the cost of runtime suspending and resuming?
> 
> Only in some cases will the serdev driver know it's safe to configure
> the parent controller. Configuring the parent controller from userspace
> works just fine as we've seen for years now.

Yes, user space may override the default settings provided by the serial
driver, but a serdev driver, in contrast, knows nothing about the
underlying serial hardware.

> > The patch I posted works with what we have today; if a parent serial
> > controller driver uses aggressive runtime PM by default or after having
> > been configured through sysfs to do so.
> 
> Yeah let's stick with configuring the parent controller from userspace
> for now at least.

Yep, status quo works for the time being (since this isn't a
regression). 

> > What I'm getting at here is that the delay should be set by the serial
> > driver implementing aggressive runtime PM. Then all we need is a
> > mechanism to determine whether an extra RPM reference should be taken at
> > tty open or not (configurable by user space, defaulting to yes).
> 
> OK yeah some additional on/off switch seems to be missing here.

As mentioned above, PM QoS resume latency may possibly be used, and
otherwise me may able to define a new (generic) QoS flag for this.

> > Specifically, the serial drivers themselves would always use
> > autosuspend and not have to deal with supporting the two RPM schemes
> > (normal vs aggressive runtime PM).
> 
> OK. So if I understand your idea right, we could have autosuspend timeout
> set to 3000ms in the 8250_omap.c but still default to RPM blocked?
> Then user can enable aggressive PM via /sys as desired, right?

Not RPM blocked; the ports must always be able to suspend when the port
is closed. But user space should be able to enable the aggressive
(active) runtime PM via sysfs independently of the autosuspend delay,
yes.

Thanks,
Johan

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Tony Lindgren]

* Johan Hovold <johan@kernel.org> [180524 09:20]:
> On Mon, May 21, 2018 at 08:48:32AM -0700, Tony Lindgren wrote:
> > 
> > Yes the bug for closed ports needs to be fixed for sure.
> 
> I did some forensic on this and it seems this problem has "always" been
> there. Specifically, closed ports have never been runtime suspended
> unless a non-negative autosuspend delay has been set by user space since
> fcdca75728ac ("ARM: OMAP2+: UART: Add runtime pm support for omap-serial
> driver") which was merged seven years ago.
>
> So while it would certainly be nice to save some more power by default,
> this would really be a new feature rather than a bug or regression fix
> (which reduces the urgency for this issue somewhat too).

Yes it's been there since the start.

> > > > > 	2. aggressive serial RPM, where the controller is allowed to
> > > > > 	   suspend while the port is open even though this may result in
> > > > > 	   lost characters when waking up on incoming data
> > > > 
> > > > In this case it seems that the only thing needed is to just
> > > > configure the autosuspend delay for the parent port. The use of
> > > > -1 has been around since the start of runtime PM AFAIK, so maybe
> > > > we should just document it. I guess we could also introduce
> > > > pm_runtime_block_autoidle_unless_configured() :)
> > > 
> > > The implications of a negative autosuspend delay are already documented
> > > (in Documentation/power/runtime_pm.txt); it's just the omap drivers that
> > > gets it wrong when trying to do things which aren't currently supported
> > > (and never have been).
> > > 
> > > So I still think we need a new mechanism for this.
> > 
> > Well if you have some better mechanism in mind let's try it out. Short of
> > sprinkling pm_runtime_force_suspend/resume calls all over, I'm out of ideas
> > right now.
> 
> Yeah, that would be too much of a hack and likely wouldn't work either
> (and we really should do away with those _force calls altogether).
> 
> I've been thinking a bit too much about this already, but it may be
> possible to use the pm QoS framework for this. A resume latency can be
> set through sysfs where "n/a" is defined to mean "no latency accepted"
> (i.e. controller remains always-on while port is open) and "0" means
> "any latency accepted" (i.e. omap aggressive serial RPM is allowed).

Oh yeah, PM QoS might work here!

> Now, implementing this may get a little tricky as we want to be able to
> change this setting on the fly (consider consoles) and we need to figure
> out the interaction with serdev (user space should probably not be
> allowed to request a resume latency for ports used by serdev).

It sounds like Andy Shevchenko has a series of patches that just might
allow us to make this all generic for Linux serial framework. So adding
Andy to Cc, I don't think he has posted all the patches yet.

Andy, see the PM QoS comment above for console idling :)

> I'd be happy to dig into this some more, but not in my spare time I'm
> afraid.

Indeed.

> > > > > For normal ttys, we need a user-space interface for selecting between
> > > > > the two, and for serdev we may want a way to select the RPM scheme from
> > > > > within the kernel.
> > > > > 
> > > > > Note that with my serdev controller runtime PM patch, serdev core could
> > > > > always opt for aggressive PM (as by default serdev core holds an RPM
> > > > > reference for the controller while the port is open).
> > > > 
> > > > So if your serdev controller was to set the parent autosuspend
> > > > delay on open() and set it back on close() this should work?
> > > 
> > > Is it really the job of a serdev driver to set the autosuspend delay of
> > > a parent controller? Isn't this somethings which depends on the
> > > characteristics of the controller (possibly configurable by user space)
> > > such as the cost of runtime suspending and resuming?
> > 
> > Only in some cases will the serdev driver know it's safe to configure
> > the parent controller. Configuring the parent controller from userspace
> > works just fine as we've seen for years now.
> 
> Yes, user space may override the default settings provided by the serial
> driver, but a serdev driver, in contrast, knows nothing about the
> underlying serial hardware.
> 
> > > The patch I posted works with what we have today; if a parent serial
> > > controller driver uses aggressive runtime PM by default or after having
> > > been configured through sysfs to do so.
> > 
> > Yeah let's stick with configuring the parent controller from userspace
> > for now at least.
> 
> Yep, status quo works for the time being (since this isn't a
> regression). 
> 
> > > What I'm getting at here is that the delay should be set by the serial
> > > driver implementing aggressive runtime PM. Then all we need is a
> > > mechanism to determine whether an extra RPM reference should be taken at
> > > tty open or not (configurable by user space, defaulting to yes).
> > 
> > OK yeah some additional on/off switch seems to be missing here.
> 
> As mentioned above, PM QoS resume latency may possibly be used, and
> otherwise me may able to define a new (generic) QoS flag for this.

Good idea.

> > > Specifically, the serial drivers themselves would always use
> > > autosuspend and not have to deal with supporting the two RPM schemes
> > > (normal vs aggressive runtime PM).
> > 
> > OK. So if I understand your idea right, we could have autosuspend timeout
> > set to 3000ms in the 8250_omap.c but still default to RPM blocked?
> > Then user can enable aggressive PM via /sys as desired, right?
> 
> Not RPM blocked; the ports must always be able to suspend when the port
> is closed. But user space should be able to enable the aggressive
> (active) runtime PM via sysfs independently of the autosuspend delay,
> yes.

Yup OK, I like the PM QoS approach.

Regards,

Tony

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Johan Hovold]

On Thu, May 24, 2018 at 06:32:37AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [180524 09:20]:
> > On Mon, May 21, 2018 at 08:48:32AM -0700, Tony Lindgren wrote:

> > > Well if you have some better mechanism in mind let's try it out. Short of
> > > sprinkling pm_runtime_force_suspend/resume calls all over, I'm out of ideas
> > > right now.
> > 
> > Yeah, that would be too much of a hack and likely wouldn't work either
> > (and we really should do away with those _force calls altogether).
> > 
> > I've been thinking a bit too much about this already, but it may be
> > possible to use the pm QoS framework for this. A resume latency can be
> > set through sysfs where "n/a" is defined to mean "no latency accepted"
> > (i.e. controller remains always-on while port is open) and "0" means
> > "any latency accepted" (i.e. omap aggressive serial RPM is allowed).
> 
> Oh yeah, PM QoS might work here!

Actually, after reading a recent QoS related bug report, I realised that
a resume latency request of "n/a" is actually a third way of disabling
runtime PM, which similarly to the negative autosuspend would prevent
also a closed port from suspending.

Using a small positive resume latency for this feels like too much of a
hack, but defining a new QoS flag might still work.

Johan