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

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

[Johan Hovold]

[ Changing the subject line as this is thread is no longer about DT
  bindings.

  Also adding linux-serial and linux-pm while keeping some context. ]

On Tue, May 08, 2018 at 05:56:08PM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Mon, May 07, 2018 at 06:34:39PM +0200, Johan Hovold wrote:
> > On Mon, May 07, 2018 at 08:45:15AM -0700, Tony Lindgren wrote:
> > > * Johan Hovold <johan@kernel.org> [180507 03:03]:
> > > > On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:
> > > > 
> > > > > Having said all of this, serdev does not yet support runtime PM (at
> > > > > all). Tony is currently looking into it. Fortunately serdev allows
> > > > > us to enable runtime PM by default (once implemented), since we know
> > > > > the remote side and can (hopefully) avoid losing characters (i.e.
> > > > > with sideband wakeup gpios).
> > > > 
> > > > I'm not sure we want generic runtime-pm support for the controllers in
> > > > the sense that the slave device state is always reflected by the serial
> > > > controller. Similar as for i2c and spi, we really only want to keep the
> > > > controller active when we are doing I/O, but we may want to keep a
> > > > client active for longer.
> > > 
> > > Yeah i2c seems to do the right thing where the bus takes care
> > > of runtime PM.
> > 
> > Yeah, but since serial is async in contrast to i2c/spi, we may not be
> > able to push this entirely into core. The serdev drivers may need to
> > indicate when they expect or need to do I/O by opening and closing the
> > port. And this is how I implemented these first couple of gnss drivers.
> >
> > Also note that most serial driver do not do runtime pm while the port is
> > open (as OMAP does), and doing so also has the drawbacks of lost
> > characters etc. as Sebastian mentioned.
> 
> I think using open/close for runtime pm is good enough for GPS,
> since it regularly sends data and draws lots of power anyways.
> But devices, that have an out-of-band wakeup signal can do proper
> runtime PM of the serial port without loosing characters.

Yeah, there may be some applications where this is possible. And this is
not the case for GPS, but not just because of a generally higher power
consumption, but due to the fact that we cannot afford having the first
message in every report burst be dropped.

> Note, that OMAP does not reach deep idle states with active
> serial port. This is not acceptable for low power devices.

Sure, but note that OMAP is the only serial driver which currently
implements this kind of aggressive runtime PM (besides a couple of
usb-serial drivers). This means that a serdev driver can never rely on
this being the case, and therefore needs to be restrictive about how
long the port is kept open if it cares about power at all.

> > > > Take the u-blox driver in this series for example. As I'm using runtime
> > > > PM to manage device power, user-space can chose to prevent the receiver
> > > > from runtime suspending in order to avoid lengthy (re-)acquisition times
> > > > in setups without a backup battery (by means of the power/control
> > > > attribute).
> > > 
> > > Sorry I don't seem to have that one, care to paste the subject
> > > line of that patch?
> > 
> > 	"[PATCH 5/7] gnss: add driver for u-blox receivers"
> > 
> > 	https://lkml.kernel.org/r/20180424163458.11947-6-johan@kernel.org
> > 
> > > > Note that serdev not enabling runtime pm for controllers is roughly
> > > > equivalent to setting the .ignore_children flag, which is what we do for
> > > > i2c and spi controller, and possibly what we want here too.
> 
> For I2C/SPI this works, since receive operations are initiated by
> the controller. Unfortunately its harder to implement for async
> serial. But I agree, that we may want to have runtime PM for the
> serdev client and .ignore_children is the way to go.

It's really about adding runtime PM support to serdev controllers.
Serdev client drivers can already use runtime PM (as mentioned above).

The ignore_children flag would then allow the controller RPM state to be
managed independently of the child/slave device state when more fine
grained RPM control is desired.

> I think the client API should allow two things:
> 
> 1. minimal runtime PM support: The controller is runtime enabled
> on serdev open and disabled on serdev close. This may be enough
> for some clients and useful for writing new drivers.

Right, and we already have something like this today by means of how the
serial driver implement runtime PM (i.e. they are generally active while
the port is open).

> 2. full runtime PM support: The controller is sleeping by default
> even with serdev open. The calls to write/change port settings/...
> automatically enables the device, similar to i2c/spi. But there
> must be additional functions to enable/disable runtime PM based
> on a wakeup gpio or similar out-of-band information. It may be
> enough to just provide:
> 
> int serdev_pm_runtime_get_sync(struct serdev_device *serdev) {
>     pm_runtime_get_sync(&serdev->dev);
> }
> 
> int serdev_pm_runtime_put_autosuspend(struct serdev_device *serdev) {
>     pm_runtime_put_autosuspend(&serdev->dev);
> }

I'm not a big fan of rpm wrappers and prefer using the RPM interface
directly, but that's a side note. In the above case we really want to
manage the controller (&serdev->ctrl->dev) rather than the client
however (.ignore_children should be set, remember).

Also note that a serial driver implementing aggressive RPM (e.g. using
autosuspend) would manage the RPM counts itself when changing terminal
settings etc, so the only thing that would be needed is for the
client/slave device to resume the controller pro-actively when it is
expecting incoming data.

To make this more concrete; an example could be a device with an OOB
wake-up signal, but using a request-response type protocol so that the
client driver knows when it's safe to allow the controller to again
suspend.

We can model this similarly to how we do it for usb-serial, namely that
the core takes an extra RPM reference at open, which a sufficiently
power aware driver can then chose to drop in order to allow for more
aggressive controller runtime PM (should the underlying device and driver
support it).

I've cooked up a patch which I'll be sending as a reply to this mail.

Thanks,
Johan

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

[Johan Hovold]

On Wed, May 09, 2018 at 11:18:31AM +0200, Johan Hovold wrote:

> I've cooked up a patch which I'll be sending as a reply to this mail.

Forgot to add the in-reply-to header of course. For the interested, the
patch can be found here:

https://lkml.kernel.org/r/20180509094419.13470-1-johan@kernel.org

Johan

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: Serdev runtime PM (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding)

[Tony Lindgren]

* Johan Hovold <johan@kernel.org> [180509 09:20]:
> On Tue, May 08, 2018 at 05:56:08PM +0200, Sebastian Reichel wrote:
> > I think using open/close for runtime pm is good enough for GPS,
> > since it regularly sends data and draws lots of power anyways.
> > But devices, that have an out-of-band wakeup signal can do proper
> > runtime PM of the serial port without loosing characters.
> 
> Yeah, there may be some applications where this is possible. And this is
> not the case for GPS, but not just because of a generally higher power
> consumption, but due to the fact that we cannot afford having the first
> message in every report burst be dropped.

Well most of the phone implementations use one or two out of band
GPIOs to first wake the UART before any data is sent. For serdev
this can be called from the serdev consumer write function for TX.
For RX, the serdev consumer needs to implement an interrupt handler
and wake up the parent UART before serdev RX.

> > Note, that OMAP does not reach deep idle states with active
> > serial port. This is not acceptable for low power devices.
> 
> Sure, but note that OMAP is the only serial driver which currently
> implements this kind of aggressive runtime PM (besides a couple of
> usb-serial drivers). This means that a serdev driver can never rely on
> this being the case, and therefore needs to be restrictive about how
> long the port is kept open if it cares about power at all.

Well by default we don't allow lossy UART. It needs to be manually
configured via /sys for the timeout. With serdev, this can all be
done with no /sys configuration needed for the cases with GPIO
wake irqs :)

Regards,

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: Serdev runtime PM (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding)

[Johan Hovold]

On Wed, May 09, 2018 at 07:05:50AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [180509 09:20]:
> > On Tue, May 08, 2018 at 05:56:08PM +0200, Sebastian Reichel wrote:
> > > I think using open/close for runtime pm is good enough for GPS,
> > > since it regularly sends data and draws lots of power anyways.
> > > But devices, that have an out-of-band wakeup signal can do proper
> > > runtime PM of the serial port without loosing characters.
> > 
> > Yeah, there may be some applications where this is possible. And this is
> > not the case for GPS, but not just because of a generally higher power
> > consumption, but due to the fact that we cannot afford having the first
> > message in every report burst be dropped.
> 
> Well most of the phone implementations use one or two out of band
> GPIOs to first wake the UART before any data is sent. For serdev
> this can be called from the serdev consumer write function for TX.
> For RX, the serdev consumer needs to implement an interrupt handler
> and wake up the parent UART before serdev RX.

Right, but the client driver would then wake the parent controller when
expecting RX, while waking its own (serial-attached) device when wanting
to do TX.

Just for the record, we also cleared this up here:

	https://marc.info/?l=linux-kernel&m=152604434504868&w=2

> > > Note, that OMAP does not reach deep idle states with active
> > > serial port. This is not acceptable for low power devices.
> > 
> > Sure, but note that OMAP is the only serial driver which currently
> > implements this kind of aggressive runtime PM (besides a couple of
> > usb-serial drivers). This means that a serdev driver can never rely on
> > this being the case, and therefore needs to be restrictive about how
> > long the port is kept open if it cares about power at all.
> 
> Well by default we don't allow lossy UART. It needs to be manually
> configured via /sys for the timeout. With serdev, this can all be
> done with no /sys configuration needed for the cases with GPIO
> wake irqs :)

Indeed, but serdev client drivers must also work with serial drivers
which do not implement this kind of aggressive runtime PM, and then the
only way to save power is to close the port when not in use.

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