[PATCH 1/2] serdev: add controller runtime PM support

[PATCH 1/2] serdev: add controller runtime PM support

[Johan Hovold]

Add support for controller runtime power management to serdev core. This
is needed to allow slave drivers to manage the runtime PM state of the
underlying serial controller when its driver, in turn, implements more
aggressive runtime power management (e.g. using autosuspend).

For some applications, for example, where loss off initial data after a
remote-wakeup event is acceptable or where rx is not used at all,
aggressive serial controller runtime PM may be used without further
involvement of the slave driver. But when this is not the case, the
slave driver must be able to indicate when incoming data is expected in
order to avoid data loss.

To facilitate the common case, where the serial controller power state
is active whenever the port is open (which is the case with just about
every serial driver), and where data loss is not acceptable and cannot
even be prevented by explicit controller runtime power management, an
RPM reference is taken in serdev open and put again at close. This
reference can later be balanced by any serdev driver which wants and/or
can handle aggressive controller runtime PM.

Note that the .ignore_children flag is set for the serdev controller to
allow the underlying hardware to idle when no I/O is expected, regardless
of the slave device RPM state.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serdev/core.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index df93b727e984..e5e84303faca 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/serdev.h>
 #include <linux/slab.h>
 
@@ -143,11 +144,28 @@ EXPORT_SYMBOL_GPL(serdev_device_remove);
 int serdev_device_open(struct serdev_device *serdev)
 {
 	struct serdev_controller *ctrl = serdev->ctrl;
+	int ret;
 
 	if (!ctrl || !ctrl->ops->open)
 		return -EINVAL;
 
-	return ctrl->ops->open(ctrl);
+	ret = ctrl->ops->open(ctrl);
+	if (ret)
+		return ret;
+
+	ret = pm_runtime_get_sync(&ctrl->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(&ctrl->dev);
+		goto err_close;
+	}
+
+	return 0;
+
+err_close:
+	if (ctrl->ops->close)
+		ctrl->ops->close(ctrl);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(serdev_device_open);
 
@@ -158,6 +176,8 @@ void serdev_device_close(struct serdev_device *serdev)
 	if (!ctrl || !ctrl->ops->close)
 		return;
 
+	pm_runtime_put(&ctrl->dev);
+
 	ctrl->ops->close(ctrl);
 }
 EXPORT_SYMBOL_GPL(serdev_device_close);
@@ -416,6 +436,9 @@ struct serdev_controller *serdev_controller_alloc(struct device *parent,
 
 	dev_set_name(&ctrl->dev, "serial%d", id);
 
+	pm_runtime_no_callbacks(&ctrl->dev);
+	pm_suspend_ignore_children(&ctrl->dev, true);
+
 	dev_dbg(&ctrl->dev, "allocated controller 0x%p id %d\n", ctrl, id);
 	return ctrl;
 
@@ -547,20 +570,23 @@ int serdev_controller_add(struct serdev_controller *ctrl)
 	if (ret)
 		return ret;
 
+	pm_runtime_enable(&ctrl->dev);
+
 	ret_of = of_serdev_register_devices(ctrl);
 	ret_acpi = acpi_serdev_register_devices(ctrl);
 	if (ret_of && ret_acpi) {
 		dev_dbg(&ctrl->dev, "no devices registered: of:%d acpi:%d\n",
 			ret_of, ret_acpi);
 		ret = -ENODEV;
-		goto out_dev_del;
+		goto err_rpm_disable;
 	}
 
 	dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
 		ctrl->nr, &ctrl->dev);
 	return 0;
 
-out_dev_del:
+err_rpm_disable:
+	pm_runtime_disable(&ctrl->dev);
 	device_del(&ctrl->dev);
 	return ret;
 };
@@ -591,6 +617,7 @@ void serdev_controller_remove(struct serdev_controller *ctrl)
 
 	dummy = device_for_each_child(&ctrl->dev, NULL,
 				      serdev_remove_device);
+	pm_runtime_disable(&ctrl->dev);
 	device_del(&ctrl->dev);
 }
 EXPORT_SYMBOL_GPL(serdev_controller_remove);
-- 
2.17.0

[PATCH EXAMPLE 2/2] dbg: gnss: sirf: allow aggressive controller runtime PM

[Johan Hovold]

This is just an example of how a serdev driver could go about to allow
aggressive controller runtime PM by dropping the RPM reference taken by
serdev core in serdev_device_open().

Note that for most GNSS devices this does not make any sense, as
allowing the controller to suspend this way would cause the
first message of every report burst to be corrupted (and discarded).

This one applies on top of the GNSS series available here:

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

Not-Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gnss/sirf.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
index 497f8eb8467f..31b2cbccd194 100644
--- a/drivers/gnss/sirf.c
+++ b/drivers/gnss/sirf.c
@@ -57,6 +57,9 @@ static int sirf_open(struct gnss_device *gdev)
 		goto err_close;
 	}
 
+	/* Allow aggresive controller runtime PM. */
+	pm_runtime_put(&serdev->ctrl->dev);
+
 	return 0;
 
 err_close:
@@ -70,6 +73,9 @@ static void sirf_close(struct gnss_device *gdev)
 	struct sirf_data *data = gnss_get_drvdata(gdev);
 	struct serdev_device *serdev = data->serdev;
 
+	/* Balance the put in open() */
+	pm_runtime_get(&serdev->ctrl->dev);
+
 	serdev_device_close(serdev);
 
 	pm_runtime_put(&serdev->dev);
-- 
2.17.0

Re: [PATCH 1/2] serdev: add controller runtime PM support

[Tony Lindgren]

* Johan Hovold <johan@kernel.org> [180509 09:46]:
> Add support for controller runtime power management to serdev core. This
> is needed to allow slave drivers to manage the runtime PM state of the
> underlying serial controller when its driver, in turn, implements more
> aggressive runtime power management (e.g. using autosuspend).
> 
> For some applications, for example, where loss off initial data after a
> remote-wakeup event is acceptable or where rx is not used at all,
> aggressive serial controller runtime PM may be used without further
> involvement of the slave driver. But when this is not the case, the
> slave driver must be able to indicate when incoming data is expected in
> order to avoid data loss.
> 
> To facilitate the common case, where the serial controller power state
> is active whenever the port is open (which is the case with just about
> every serial driver), and where data loss is not acceptable and cannot
> even be prevented by explicit controller runtime power management, an
> RPM reference is taken in serdev open and put again at close. This
> reference can later be balanced by any serdev driver which wants and/or
> can handle aggressive controller runtime PM.
> 
> Note that the .ignore_children flag is set for the serdev controller to
> allow the underlying hardware to idle when no I/O is expected, regardless
> of the slave device RPM state.

If this solution works for GPS then this should also work for modems
that might produce data. And as long as the serdev consumer driver
can wake up the UART with pm_runtime_get(&serdev->ctrl->dev) then
the out of band GPIO wake interrupts will work to. And for TX,
the serdev consumer driver can toggle the wake GPIO, and then call
pm_runtime_get(&serdev->ctrl->dev). So yeah, looks nice to me:

Acked-by: Tony Lindgren <tony@atomide.com>

Re: [PATCH 1/2] serdev: add controller runtime PM support

[Johan Hovold]

On Thu, May 10, 2018 at 09:48:31AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [180509 09:46]:
> > Add support for controller runtime power management to serdev core. This
> > is needed to allow slave drivers to manage the runtime PM state of the
> > underlying serial controller when its driver, in turn, implements more
> > aggressive runtime power management (e.g. using autosuspend).
> > 
> > For some applications, for example, where loss off initial data after a
> > remote-wakeup event is acceptable or where rx is not used at all,
> > aggressive serial controller runtime PM may be used without further
> > involvement of the slave driver. But when this is not the case, the
> > slave driver must be able to indicate when incoming data is expected in
> > order to avoid data loss.
> > 
> > To facilitate the common case, where the serial controller power state
> > is active whenever the port is open (which is the case with just about
> > every serial driver), and where data loss is not acceptable and cannot
> > even be prevented by explicit controller runtime power management, an
> > RPM reference is taken in serdev open and put again at close. This
> > reference can later be balanced by any serdev driver which wants and/or
> > can handle aggressive controller runtime PM.
> > 
> > Note that the .ignore_children flag is set for the serdev controller to
> > allow the underlying hardware to idle when no I/O is expected, regardless
> > of the slave device RPM state.
> 
> If this solution works for GPS then this should also work for modems
> that might produce data. And as long as the serdev consumer driver
> can wake up the UART with pm_runtime_get(&serdev->ctrl->dev) then
> the out of band GPIO wake interrupts will work to. And for TX,
> the serdev consumer driver can toggle the wake GPIO, and then call
> pm_runtime_get(&serdev->ctrl->dev).

I don't think any serdev driver action is needed for TX however. The
serial driver itself would know that there's data in the write buffer
and should manage PM itself until the buffer has been drained (which may
never happen due to flow control).

But either way, the mechanism introduced by this patch is general enough
that it could in principle be used also for something like that.

> So yeah, looks nice to me:
> 
> Acked-by: Tony Lindgren <tony@atomide.com>

Thanks,
Johan

Re: [PATCH 1/2] serdev: add controller runtime PM support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 4429 bytes --]

Hi Johan,

On Wed, May 09, 2018 at 11:44:18AM +0200, Johan Hovold wrote:
> Add support for controller runtime power management to serdev core. This
> is needed to allow slave drivers to manage the runtime PM state of the
> underlying serial controller when its driver, in turn, implements more
> aggressive runtime power management (e.g. using autosuspend).
> 
> For some applications, for example, where loss off initial data after a
> remote-wakeup event is acceptable or where rx is not used at all,
> aggressive serial controller runtime PM may be used without further
> involvement of the slave driver. But when this is not the case, the
> slave driver must be able to indicate when incoming data is expected in
> order to avoid data loss.
> 
> To facilitate the common case, where the serial controller power state
> is active whenever the port is open (which is the case with just about
> every serial driver), and where data loss is not acceptable and cannot
> even be prevented by explicit controller runtime power management, an
> RPM reference is taken in serdev open and put again at close. This
> reference can later be balanced by any serdev driver which wants and/or
> can handle aggressive controller runtime PM.
> 
> Note that the .ignore_children flag is set for the serdev controller to
> allow the underlying hardware to idle when no I/O is expected, regardless
> of the slave device RPM state.
> 
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---

Looks good to me.

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>

-- Sebastian

>  drivers/tty/serdev/core.c | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index df93b727e984..e5e84303faca 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -13,6 +13,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/serdev.h>
>  #include <linux/slab.h>
>  
> @@ -143,11 +144,28 @@ EXPORT_SYMBOL_GPL(serdev_device_remove);
>  int serdev_device_open(struct serdev_device *serdev)
>  {
>  	struct serdev_controller *ctrl = serdev->ctrl;
> +	int ret;
>  
>  	if (!ctrl || !ctrl->ops->open)
>  		return -EINVAL;
>  
> -	return ctrl->ops->open(ctrl);
> +	ret = ctrl->ops->open(ctrl);
> +	if (ret)
> +		return ret;
> +
> +	ret = pm_runtime_get_sync(&ctrl->dev);
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(&ctrl->dev);
> +		goto err_close;
> +	}
> +
> +	return 0;
> +
> +err_close:
> +	if (ctrl->ops->close)
> +		ctrl->ops->close(ctrl);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(serdev_device_open);
>  
> @@ -158,6 +176,8 @@ void serdev_device_close(struct serdev_device *serdev)
>  	if (!ctrl || !ctrl->ops->close)
>  		return;
>  
> +	pm_runtime_put(&ctrl->dev);
> +
>  	ctrl->ops->close(ctrl);
>  }
>  EXPORT_SYMBOL_GPL(serdev_device_close);
> @@ -416,6 +436,9 @@ struct serdev_controller *serdev_controller_alloc(struct device *parent,
>  
>  	dev_set_name(&ctrl->dev, "serial%d", id);
>  
> +	pm_runtime_no_callbacks(&ctrl->dev);
> +	pm_suspend_ignore_children(&ctrl->dev, true);
> +
>  	dev_dbg(&ctrl->dev, "allocated controller 0x%p id %d\n", ctrl, id);
>  	return ctrl;
>  
> @@ -547,20 +570,23 @@ int serdev_controller_add(struct serdev_controller *ctrl)
>  	if (ret)
>  		return ret;
>  
> +	pm_runtime_enable(&ctrl->dev);
> +
>  	ret_of = of_serdev_register_devices(ctrl);
>  	ret_acpi = acpi_serdev_register_devices(ctrl);
>  	if (ret_of && ret_acpi) {
>  		dev_dbg(&ctrl->dev, "no devices registered: of:%d acpi:%d\n",
>  			ret_of, ret_acpi);
>  		ret = -ENODEV;
> -		goto out_dev_del;
> +		goto err_rpm_disable;
>  	}
>  
>  	dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
>  		ctrl->nr, &ctrl->dev);
>  	return 0;
>  
> -out_dev_del:
> +err_rpm_disable:
> +	pm_runtime_disable(&ctrl->dev);
>  	device_del(&ctrl->dev);
>  	return ret;
>  };
> @@ -591,6 +617,7 @@ void serdev_controller_remove(struct serdev_controller *ctrl)
>  
>  	dummy = device_for_each_child(&ctrl->dev, NULL,
>  				      serdev_remove_device);
> +	pm_runtime_disable(&ctrl->dev);
>  	device_del(&ctrl->dev);
>  }
>  EXPORT_SYMBOL_GPL(serdev_controller_remove);
> -- 
> 2.17.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] serdev: add controller runtime PM support

[Tony Lindgren]

* Johan Hovold <johan@kernel.org> [180511 08:09]:
> On Thu, May 10, 2018 at 09:48:31AM -0700, Tony Lindgren wrote:
> > If this solution works for GPS then this should also work for modems
> > that might produce data. And as long as the serdev consumer driver
> > can wake up the UART with pm_runtime_get(&serdev->ctrl->dev) then
> > the out of band GPIO wake interrupts will work to. And for TX,
> > the serdev consumer driver can toggle the wake GPIO, and then call
> > pm_runtime_get(&serdev->ctrl->dev).
> 
> I don't think any serdev driver action is needed for TX however. The
> serial driver itself would know that there's data in the write buffer
> and should manage PM itself until the buffer has been drained (which may
> never happen due to flow control).

Sure if the serial driver can manage TX wake directly. However, the
case I'm thinking needs few hundred milliseconds after toggling the
GPIO before we can even attempt to do TX. I guess what I'm saying
let's not try to stuff any "application specific" GPIO handling to
generic UART code :)

> But either way, the mechanism introduced by this patch is general enough
> that it could in principle be used also for something like that.

Yes I agree your patches should work for both cases.

Regards,

Tony

Re: [PATCH 1/2] serdev: add controller runtime PM support

[Johan Hovold]

On Fri, May 11, 2018 at 05:56:27AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [180511 08:09]:
> > On Thu, May 10, 2018 at 09:48:31AM -0700, Tony Lindgren wrote:
> > > If this solution works for GPS then this should also work for modems
> > > that might produce data. And as long as the serdev consumer driver
> > > can wake up the UART with pm_runtime_get(&serdev->ctrl->dev) then
> > > the out of band GPIO wake interrupts will work to. And for TX,
> > > the serdev consumer driver can toggle the wake GPIO, and then call
> > > pm_runtime_get(&serdev->ctrl->dev).
> > 
> > I don't think any serdev driver action is needed for TX however. The
> > serial driver itself would know that there's data in the write buffer
> > and should manage PM itself until the buffer has been drained (which may
> > never happen due to flow control).
> 
> Sure if the serial driver can manage TX wake directly. However, the
> case I'm thinking needs few hundred milliseconds after toggling the
> GPIO before we can even attempt to do TX. I guess what I'm saying
> let's not try to stuff any "application specific" GPIO handling to
> generic UART code :)

Ok, I think understand what you have in mind now, but that sounds like
something which should be handled by the serdev driver which is
responsible for the power state of the serial-attached device (e.g.
through a pm_runtime_get_sync(&serdev->dev)).

And you're absolutely right that that RPM reference cannot be dropped
before any written data has reached the device. But note that the serial
controller responsible for that transfer could potentially runtime
suspend before the write buffer has been drained (e.g. during long
periods without I/O due to flow control).

So I still think that TX is not directly related to this patch and RPM
support for serdev controllers.

Thanks,
Johan

Re: [PATCH 1/2] serdev: add controller runtime PM support

[Tony Lindgren]

* Johan Hovold <johan@kernel.org> [180511 13:14]:
> On Fri, May 11, 2018 at 05:56:27AM -0700, Tony Lindgren wrote:
> > * Johan Hovold <johan@kernel.org> [180511 08:09]:
> > > On Thu, May 10, 2018 at 09:48:31AM -0700, Tony Lindgren wrote:
> > > > If this solution works for GPS then this should also work for modems
> > > > that might produce data. And as long as the serdev consumer driver
> > > > can wake up the UART with pm_runtime_get(&serdev->ctrl->dev) then
> > > > the out of band GPIO wake interrupts will work to. And for TX,
> > > > the serdev consumer driver can toggle the wake GPIO, and then call
> > > > pm_runtime_get(&serdev->ctrl->dev).
> > > 
> > > I don't think any serdev driver action is needed for TX however. The
> > > serial driver itself would know that there's data in the write buffer
> > > and should manage PM itself until the buffer has been drained (which may
> > > never happen due to flow control).
> > 
> > Sure if the serial driver can manage TX wake directly. However, the
> > case I'm thinking needs few hundred milliseconds after toggling the
> > GPIO before we can even attempt to do TX. I guess what I'm saying
> > let's not try to stuff any "application specific" GPIO handling to
> > generic UART code :)
> 
> Ok, I think understand what you have in mind now, but that sounds like
> something which should be handled by the serdev driver which is
> responsible for the power state of the serial-attached device (e.g.
> through a pm_runtime_get_sync(&serdev->dev)).

Yes that makes sense.

> And you're absolutely right that that RPM reference cannot be dropped
> before any written data has reached the device. But note that the serial
> controller responsible for that transfer could potentially runtime
> suspend before the write buffer has been drained (e.g. during long
> periods without I/O due to flow control).
> 
> So I still think that TX is not directly related to this patch and RPM
> support for serdev controllers.

Yes you are right after the device on the serial line has been properly
woken up then TX should just work and at that point the UART driver
runtime PM can take care of things.

Regards,

Tony