[PATCH v2] TTY: serial, add pm function

[PATCH v2] TTY: serial, add pm function

[Rickard Andersson]

Add power management function to tty driver interface
and add implementation for serial core.

Signed-off-by: Rickard Andersson <rickard.andersson@stericsson.com>
---
ChangeLog v1->v2
- add mutex handling
---
 drivers/tty/serial/serial_core.c | 13 +++++++++++++
 include/linux/tty_driver.h       |  8 ++++++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index b2fc8d7..ec501f5 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1114,6 +1114,18 @@ static int uart_get_icount(struct tty_struct *tty,
 	return 0;
 }
 
+static int uart_pm(struct tty_struct *tty, enum tty_pm_state state)
+{
+	struct uart_state *ustate = tty->driver_data;
+	struct tty_port *port = &ustate->port;
+
+	mutex_lock(&port->mutex);
+	uart_change_pm(ustate, state);
+	mutex_unlock(&port->mutex);
+
+	return 0;
+}
+
 /*
  * Called via sys_ioctl.  We can use spin_lock_irq() here.
  */
@@ -2217,6 +2229,7 @@ static const struct tty_operations uart_ops = {
 	.tiocmget	= uart_tiocmget,
 	.tiocmset	= uart_tiocmset,
 	.get_icount	= uart_get_icount,
+	.pm		= uart_pm,
 #ifdef CONFIG_CONSOLE_POLL
 	.poll_init	= uart_poll_init,
 	.poll_get_char	= uart_poll_get_char,
diff --git a/include/linux/tty_driver.h b/include/linux/tty_driver.h
index d150a6f..ba34b7f 100644
--- a/include/linux/tty_driver.h
+++ b/include/linux/tty_driver.h
@@ -229,6 +229,12 @@
  *	Called when the device receives a TIOCGICOUNT ioctl. Passed a kernel
  *	structure to complete. This method is optional and will only be called
  *	if provided (otherwise EINVAL will be returned).
+ *
+ * int (*pm)(struct tty_struct * tty, enum tty_pm_state state);
+ *
+ *      Perform any power management related activities on the specified
+ *      tty. State indicates the new state.
+ *
  */
 
 #include <linux/export.h>
@@ -249,6 +255,7 @@ struct serial_icounter_struct;
  */
 enum tty_pm_state {
 	TTY_PM_STATE_ON = 0,
+	TTY_PM_STATE_SLEEP,
 	TTY_PM_STATE_OFF = 3, /* number taken from ACPI */
 	TTY_PM_STATE_UNDEFINED,
 };
@@ -290,6 +297,7 @@ struct tty_operations {
 	int (*set_termiox)(struct tty_struct *tty, struct termiox *tnew);
 	int (*get_icount)(struct tty_struct *tty,
 				struct serial_icounter_struct *icount);
+	int (*pm)(struct tty_struct *tty, enum tty_pm_state state);
 #ifdef CONFIG_CONSOLE_POLL
 	int (*poll_init)(struct tty_driver *driver, int line, char *options);
 	int (*poll_get_char)(struct tty_driver *driver, int line);
-- 
1.8.0

Re: [PATCH v2] TTY: serial, add pm function

[Greg KH]

On Wed, Dec 12, 2012 at 01:40:51PM +0100, Rickard Andersson wrote:
> Add power management function to tty driver interface
> and add implementation for serial core.
> 
> Signed-off-by: Rickard Andersson <rickard.andersson@stericsson.com>
> ---
> ChangeLog v1->v2
> - add mutex handling

As no one is using this, why add it?

greg k-h

Re: [PATCH v2] TTY: serial, add pm function

[Rickard Andersson]

On 01/16/2013 08:05 AM, Greg KH wrote:
> On Wed, Dec 12, 2012 at 01:40:51PM +0100, Rickard Andersson wrote:
>> Add power management function to tty driver interface
>> and add implementation for serial core.
>>
>> Signed-off-by: Rickard Andersson<rickard.andersson@stericsson.com>
>> ---
>> ChangeLog v1->v2
>> - add mutex handling
> As no one is using this, why add it?
>
> greg k-h
We want to use this pm function from our bluetooth driver to save 
current when possible. So in our case when the bluetooth driver sees 
that the serial communication to the bluetooth chip will not be used it 
can tell the pl011 serial driver to turn off clocks.

BR
Rickard

Re: [PATCH v2] TTY: serial, add pm function

[Greg KH]

On Wed, Jan 16, 2013 at 08:53:02AM +0100, Rickard Andersson wrote:
> On 01/16/2013 08:05 AM, Greg KH wrote:
> >On Wed, Dec 12, 2012 at 01:40:51PM +0100, Rickard Andersson wrote:
> >>Add power management function to tty driver interface
> >>and add implementation for serial core.
> >>
> >>Signed-off-by: Rickard Andersson<rickard.andersson@stericsson.com>
> >>---
> >>ChangeLog v1->v2
> >>- add mutex handling
> >As no one is using this, why add it?
> >
> >greg k-h
> We want to use this pm function from our bluetooth driver to save
> current when possible. So in our case when the bluetooth driver sees
> that the serial communication to the bluetooth chip will not be used
> it can tell the pl011 serial driver to turn off clocks.

Is that driver in the tree already?

Re: [PATCH v2] TTY: serial, add pm function

[Rickard Andersson]

On 01/16/2013 08:58 AM, Greg KH wrote:
> On Wed, Jan 16, 2013 at 08:53:02AM +0100, Rickard Andersson wrote:
>> On 01/16/2013 08:05 AM, Greg KH wrote:
>>> On Wed, Dec 12, 2012 at 01:40:51PM +0100, Rickard Andersson wrote:
>>>> Add power management function to tty driver interface
>>>> and add implementation for serial core.
>>>>
>>>> Signed-off-by: Rickard Andersson<rickard.andersson@stericsson.com>
>>>> ---
>>>> ChangeLog v1->v2
>>>> - add mutex handling
>>> As no one is using this, why add it?
>>>
>>> greg k-h
>> We want to use this pm function from our bluetooth driver to save
>> current when possible. So in our case when the bluetooth driver sees
>> that the serial communication to the bluetooth chip will not be used
>> it can tell the pl011 serial driver to turn off clocks.
> Is that driver in the tree already?
>
The bluetooth cg2900 driver is on its way towards "staging".

Re: [PATCH v2] TTY: serial, add pm function

[Greg KH]

On Wed, Jan 16, 2013 at 09:33:09AM +0100, Rickard Andersson wrote:
> On 01/16/2013 08:58 AM, Greg KH wrote:
> >On Wed, Jan 16, 2013 at 08:53:02AM +0100, Rickard Andersson wrote:
> >>On 01/16/2013 08:05 AM, Greg KH wrote:
> >>>On Wed, Dec 12, 2012 at 01:40:51PM +0100, Rickard Andersson wrote:
> >>>>Add power management function to tty driver interface
> >>>>and add implementation for serial core.
> >>>>
> >>>>Signed-off-by: Rickard Andersson<rickard.andersson@stericsson.com>
> >>>>---
> >>>>ChangeLog v1->v2
> >>>>- add mutex handling
> >>>As no one is using this, why add it?
> >>>
> >>>greg k-h
> >>We want to use this pm function from our bluetooth driver to save
> >>current when possible. So in our case when the bluetooth driver sees
> >>that the serial communication to the bluetooth chip will not be used
> >>it can tell the pl011 serial driver to turn off clocks.
> >Is that driver in the tree already?
> >
> The bluetooth cg2900 driver is on its way towards "staging".

Really?  I've never heard of it before, nor seen it, so how would I know
this?  And we really never want to change core kernel code for staging
drivers, it's one of the requirements of staging code.

So I'll not take this for now, thanks.

greg k-h

Re: [PATCH v2] TTY: serial, add pm function

[Linus Walleij]

On Wed, Jan 16, 2013 at 4:24 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Jan 16, 2013 at 09:33:09AM +0100, Rickard Andersson wrote:

>> The bluetooth cg2900 driver is on its way towards "staging".
>
> Really?  I've never heard of it before, nor seen it, so how would I know
> this?

It's been posted twice, here is the last time:
http://marc.info/?l=linux-kernel&m=134873373526049&w=2

You commented several times on it, but I know a lot of code
pass by your console.

>  And we really never want to change core kernel code for staging
> drivers, it's one of the requirements of staging code.

Hm OK but it's a quite straight-forward thing for anything
connected on a UART in an embedded system that is not just a
serial cable or something and wants to save power.

Maybe we can augment some other driver for something sitting
on a uart as a proof-of-concept then.

Thanks,
Linus Walleij

Re: [PATCH v2] TTY: serial, add pm function

[Greg KH]

On Thu, Jan 17, 2013 at 01:36:48AM +0100, Linus Walleij wrote:
> On Wed, Jan 16, 2013 at 4:24 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Wed, Jan 16, 2013 at 09:33:09AM +0100, Rickard Andersson wrote:
> 
> >> The bluetooth cg2900 driver is on its way towards "staging".
> >
> > Really?  I've never heard of it before, nor seen it, so how would I know
> > this?
> 
> It's been posted twice, here is the last time:
> http://marc.info/?l=linux-kernel&m=134873373526049&w=2
> 
> You commented several times on it, but I know a lot of code
> pass by your console.

Heh, that's funny, I don't remember that at all, I think I now
officially have no long-term memory about patches I review :)

> >  And we really never want to change core kernel code for staging
> > drivers, it's one of the requirements of staging code.
> 
> Hm OK but it's a quite straight-forward thing for anything
> connected on a UART in an embedded system that is not just a
> serial cable or something and wants to save power.
> 
> Maybe we can augment some other driver for something sitting
> on a uart as a proof-of-concept then.

You know we don't add infrastructure if there is no in-kernel user, and
some random patch that was sent months ago doesn't really count as a
"user" given that it's not even being submitted here, and it wasn't
referenced in the patch itself that added the api.

Also, why does this driver need something that the hundreds of other
serial drivers we have in-kernel today do not?  What makes it special
over everything else?

thanks,

greg "I will not remember writing this email" k-h

Re: [PATCH v2] TTY: serial, add pm function

[Rickard Andersson]

On 01/17/2013 03:32 AM, Greg KH wrote:
> On Thu, Jan 17, 2013 at 01:36:48AM +0100, Linus Walleij wrote:
>> On Wed, Jan 16, 2013 at 4:24 PM, Greg KH<gregkh@linuxfoundation.org>  wrote:
>>> On Wed, Jan 16, 2013 at 09:33:09AM +0100, Rickard Andersson wrote:
>>>> The bluetooth cg2900 driver is on its way towards "staging".
>>> Really?  I've never heard of it before, nor seen it, so how would I know
>>> this?
>> It's been posted twice, here is the last time:
>> http://marc.info/?l=linux-kernel&m=134873373526049&w=2
>>
>> You commented several times on it, but I know a lot of code
>> pass by your console.
> Heh, that's funny, I don't remember that at all, I think I now
> officially have no long-term memory about patches I review :)
>
>>>   And we really never want to change core kernel code for staging
>>> drivers, it's one of the requirements of staging code.
>> Hm OK but it's a quite straight-forward thing for anything
>> connected on a UART in an embedded system that is not just a
>> serial cable or something and wants to save power.
>>
>> Maybe we can augment some other driver for something sitting
>> on a uart as a proof-of-concept then.
> You know we don't add infrastructure if there is no in-kernel user, and
> some random patch that was sent months ago doesn't really count as a
> "user" given that it's not even being submitted here, and it wasn't
> referenced in the patch itself that added the api.
>
> Also, why does this driver need something that the hundreds of other
> serial drivers we have in-kernel today do not?  What makes it special
> over everything else?
>
> thanks,
>
> greg "I will not remember writing this email" k-h
Previously we have used a hack where baud rate was set to 0 using 
set_termios and when the baudrate was 0 clock was turned off to save 
current by the amba-pl011 driver. Now I try to do it in a cleaner way. 
An old discussion can be found here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2010-November/031111.html

Re: [PATCH v2] TTY: serial, add pm function

[Alan Cox]

> Also, why does this driver need something that the hundreds of other
> serial drivers we have in-kernel today do not?  What makes it special
> over everything else?

We certainly will need power management hooks on the tty layer -
probably they belong on the tty_port operations.

Several things have happened that make it more relevant

- power levels have come down to the point that serial ports are no
  longer off the radar

- quite a few embedded and tablet devices have internal serial busses
  where turning off ports and stuff behind them matters.


Alan

Re: [PATCH v2] TTY: serial, add pm function

[Linus Walleij]

On Thu, Jan 17, 2013 at 3:32 AM, Greg KH <gregkh@linuxfoundation.org> wrote:

> Also, why does this driver need something that the hundreds of other
> serial drivers we have in-kernel today do not?  What makes it special
> over everything else?

So (and I think Alan just seconded this) I think several vendors
already have different ugly out-of-tree hacks to solve exactly this.

The reason you haven't seen it before is that there is no AT
command to power save a US Robotics 33k8 modem.
And these are handled from userspace.

The embedded systems have:

- In-kernel driver using the TTY

- Control over both ends of the link (we have the equivalent
  of an AT command setting the 33k8 into retention).

- Critical (as in really critical) power requirements.

Blueetooth chips on megabit serial links seem to be pretty
common actually. Maybe people even have modems on these.

I think the reason you haven't seen it before is that either
people are scared of the TTY layer (whyyyy would they be that)
or that they are in the out-of-tree business. We're trying to be
in the in-tree business, so to be there we need to put the
infrastructure in place to be able to be there.

But I'm looking around for a *simple* UART-connected device
with a low-power/retention state to use as example in
question, so we don't have to conflate the CG2900 upstreaming
issue with the TTY/UART PM issue. Anyone have hints
on a suitable device?

Linus Walleij