[PATCH] pl011: added clock management feature

[PATCH] pl011: added clock management feature

[Linus Walleij]

From: Grzegorz Sygieda <grzegorz.sygieda@tieto.com>

This patch allows to control the pl011 clock using set_termios
callback. Any positive baudrate passed enables clock, otherwise
disables. This saves a lot of power on submicron designs since
we can clock off and disable unused UARTs.

Cc: Lukasz Rymanowski <Lukasz.Rymanowski@tieto.com>
Cc: Par-Gunnar Hjalmdahl <par-gunnar.p.hjalmdahl@stericsson.com>
Signed-off-by: Grzegorz Sygieda <grzegorz.sygieda@tieto.com>
Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
This patch depends on the DMA support, just because it's adjacent
in the file in some functions. If it's desired to have them in the
reverse order, this can be arranged for.

Maybe this shouldn't even be an optional Kconfig option, something
like this should be expected when you set the baud rate to zero
really. Comments welcome.
---
 drivers/serial/Kconfig      |    9 ++
 drivers/serial/amba-pl011.c |  183 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 189 insertions(+), 3 deletions(-)

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index aff9dcd..0328fba 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -323,6 +323,15 @@ config SERIAL_AMBA_PL011_CONSOLE
 	  your boot loader (lilo or loadlin) about how to pass options to the
 	  kernel at boot time.)
 
+config SERIAL_AMBA_PL011_CLOCK_CONTROL
+	bool "Support for clock control on AMBA serial port"
+	depends on SERIAL_AMBA_PL011
+	select CONSOLE_POLL
+	---help---
+	  Say Y here if you wish to use amba set_termios function to control
+	  the pl011 clock. Any positive baudrate passed enables clock,
+	  otherwise disables (baudrate = B0 -> hangup)
+
 config SERIAL_SB1250_DUART
 	tristate "BCM1xxx on-chip DUART serial support"
 	depends on SIBYTE_SB1xxx_SOC=y
diff --git a/drivers/serial/amba-pl011.c b/drivers/serial/amba-pl011.c
index e1b4652..5f4aca7 100644
--- a/drivers/serial/amba-pl011.c
+++ b/drivers/serial/amba-pl011.c
@@ -47,6 +47,7 @@
 #include <linux/serial.h>
 #include <linux/amba/bus.h>
 #include <linux/amba/serial.h>
+#include <linux/workqueue.h>
 #include <linux/clk.h>
 #include <linux/slab.h>
 #include <linux/dmaengine.h>
@@ -89,6 +90,15 @@ struct pl011_dma_tx_transaction {
 	dma_cookie_t cookie;
 };
 
+
+/* Available amba pl011 port clock states */
+enum pl011_clk_states {
+	PL011_CLK_OFF = 0,	/* clock disabled */
+	PL011_CLK_REQUEST_OFF,	/* disable after TX flushed */
+	PL011_CLK_ON,		/* clock enabled */
+	PL011_PORT_OFF,		/* port disabled */
+};
+
 /*
  * We wrap our port structure around the generic uart_port.
  */
@@ -113,6 +123,11 @@ struct uart_amba_port {
 	struct pl011_dma_rx_transaction dmarx;
 	struct pl011_dma_tx_transaction dmatx;
 #endif
+#ifdef CONFIG_SERIAL_AMBA_PL011_CLOCK_CONTROL
+	enum pl011_clk_states	clk_state;	/* actual clock state */
+	struct delayed_work	clk_off_work;	/* work used for clock off */
+	unsigned int		clk_off_delay;	/* clock off delay */
+#endif
 };
 
 /* There is by now at least one vendor with differing details, so handle it */
@@ -869,6 +884,155 @@ static inline int pl011_dma_tx_chars(struct uart_amba_port *uap)
 }
 #endif
 
+#ifdef CONFIG_SERIAL_AMBA_PL011_CLOCK_CONTROL
+/* Turn clock off if TX buffer is empty, otherwise reschedule */
+static void pl011_clock_off(struct work_struct *work)
+{
+	struct uart_amba_port *uap = container_of(work, struct uart_amba_port,
+						clk_off_work.work);
+	struct uart_port *port = &uap->port;
+	struct circ_buf *xmit = &port->state->xmit;
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+
+	if (uap->clk_state == PL011_CLK_REQUEST_OFF) {
+		if (uart_circ_empty(xmit)) {
+			clk_disable(uap->clk);
+			uap->clk_state = PL011_CLK_OFF;
+		} else
+			schedule_delayed_work(&uap->clk_off_work,
+						uap->clk_off_delay);
+	}
+
+	spin_unlock_irqrestore(&port->lock, flags);
+}
+
+/* Request to turn off uart clock once pending TX is flushed */
+static void pl011_clock_request_off(struct uart_port *port)
+{
+	unsigned long flags;
+	struct uart_amba_port *uap = (struct uart_amba_port *)(port);
+
+	spin_lock_irqsave(&port->lock, flags);
+
+	if (uap->clk_state == PL011_CLK_ON) {
+		uap->clk_state = PL011_CLK_REQUEST_OFF;
+		/* Turn off later */
+		schedule_delayed_work(&uap->clk_off_work,
+					uap->clk_off_delay);
+	}
+
+	spin_unlock_irqrestore(&port->lock, flags);
+}
+
+/* Request to immediately turn on uart clock */
+static void pl011_clock_on(struct uart_port *port)
+{
+	unsigned long flags;
+	struct uart_amba_port *uap = (struct uart_amba_port *)(port);
+
+	spin_lock_irqsave(&port->lock, flags);
+
+	switch (uap->clk_state) {
+	case PL011_CLK_OFF:
+		clk_enable(uap->clk);
+		/* fallthrough */
+	case PL011_CLK_REQUEST_OFF:
+		cancel_delayed_work(&uap->clk_off_work);
+		uap->clk_state = PL011_CLK_ON;
+		break;
+	default:
+		break;
+	}
+
+	spin_unlock_irqrestore(&port->lock, flags);
+}
+
+static void pl011_clock_check(struct uart_amba_port *uap)
+{
+	/* Reshedule work during off request  */
+	if (uap->clk_state == PL011_CLK_REQUEST_OFF)
+		/* New TX - restart work */
+		if (cancel_delayed_work(&uap->clk_off_work))
+			schedule_delayed_work(&uap->clk_off_work,
+						uap->clk_off_delay);
+}
+
+static int pl011_clock_startup(struct uart_amba_port *uap)
+{
+	int retval = 0;
+
+	if (uap->clk_state == PL011_PORT_OFF) {
+		retval = clk_enable(uap->clk);
+		if (!retval)
+			uap->clk_state = PL011_CLK_ON;
+		else
+			uap->clk_state = PL011_PORT_OFF;
+	}
+
+	return retval;
+}
+
+static void pl011_clock_shutdown(struct uart_amba_port *uap)
+{
+	spin_lock_irq(&uap->port.lock);
+	if (uap->clk_state == PL011_CLK_ON) {
+		clk_disable(uap->clk);
+		uap->clk_state = PL011_PORT_OFF;
+	}
+	spin_unlock_irq(&uap->port.lock);
+}
+
+static void
+pl011_clock_control(struct uart_port *port, struct ktermios *termios,
+		     struct ktermios *old)
+{
+	if (old) {
+		if ((termios->c_cflag & CBAUD) &&
+		    (termios->c_ispeed != old->c_ispeed ||
+		     termios->c_ospeed != old->c_ospeed)) {
+			if (termios->c_ispeed || termios->c_ospeed)
+				pl011_clock_on(port);
+			else
+				pl011_clock_request_off(port);
+		}
+	}
+}
+
+static void pl011_clock_control_init(struct uart_amba_port *uap)
+{
+	uap->clk_state = PL011_PORT_OFF;
+	INIT_DELAYED_WORK(&uap->clk_off_work, pl011_clock_off);
+	uap->clk_off_delay = HZ / 1000;  /* 1 ms */
+}
+
+#else
+/* Blank functions for clock control */
+static inline void pl011_clock_check(struct uart_amba_port *uap)
+{
+}
+
+static inline int pl011_clock_startup(struct uart_amba_port *uap)
+{
+	return clk_enable(uap->clk);
+}
+
+static inline void pl011_clock_shutdown(struct uart_amba_port *uap)
+{
+	clk_disable(uap->clk);
+}
+
+static inline void
+pl011_clock_control(struct uart_port *port, struct ktermios *termios,
+		     struct ktermios *old)
+{
+}
+
+static inline void pl011_clock_control_init(struct uart_amba_port *uap)
+{
+}
+#endif
 
 static void pl011_stop_tx(struct uart_port *port)
 {
@@ -1024,6 +1188,9 @@ static void pl011_tx_chars(struct uart_amba_port *uap)
 			break;
 	} while (--count > 0);
 
+	if (count)
+		pl011_clock_check(uap);
+
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 		uart_write_wakeup(&uap->port);
 
@@ -1212,7 +1379,7 @@ static int pl011_startup(struct uart_port *port)
 	/*
 	 * Try to enable the clock producer.
 	 */
-	retval = clk_enable(uap->clk);
+	retval = pl011_clock_startup(uap);
 	if (retval)
 		goto out;
 
@@ -1280,7 +1447,7 @@ static int pl011_startup(struct uart_port *port)
 	return 0;
 
  clk_dis:
-	clk_disable(uap->clk);
+	pl011_clock_shutdown(uap);
  out:
 	return retval;
 }
@@ -1331,7 +1498,7 @@ static void pl011_shutdown(struct uart_port *port)
 	/*
 	 * Shut down the clock producer
 	 */
-	clk_disable(uap->clk);
+	pl011_clock_shutdown(uap);
 }
 
 static void
@@ -1344,6 +1511,13 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
 	unsigned int baud, quot;
 
 	/*
+	 * Must be before uart_get_baud_rate() call, because
+	 * this function changes baudrate to default in case of 0
+	 * B0 hangup !!!
+	 */
+	pl011_clock_control(port, termios, old);
+
+	/*
 	 * Ask the core to calculate the divisor for us.
 	 */
 	baud = uart_get_baud_rate(port, termios, old, 0,
@@ -1721,6 +1895,9 @@ static int pl011_probe(struct amba_device *dev, struct amba_id *id)
 	amba_ports[i] = uap;
 
 	amba_set_drvdata(dev, uap);
+
+	pl011_clock_control_init(uap);
+
 	ret = uart_add_one_port(&amba_reg, &uap->port);
 	if (ret) {
 		amba_set_drvdata(dev, NULL);
-- 
1.6.3.3

[PATCH] pl011: added clock management feature

[Linus Walleij]

From: Grzegorz Sygieda <grzegorz.sygieda@tieto.com>

This patch allows to control the pl011 clock using set_termios
callback. Any positive baudrate passed enables clock, otherwise
disables. This saves a lot of power on submicron designs since
we can clock off and disable unused UARTs.

Cc: Lukasz Rymanowski <Lukasz.Rymanowski@tieto.com>
Cc: Par-Gunnar Hjalmdahl <par-gunnar.p.hjalmdahl@stericsson.com>
Signed-off-by: Grzegorz Sygieda <grzegorz.sygieda@tieto.com>
Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
This patch depends on the DMA support, just because it's adjacent
in the file in some functions. If it's desired to have them in the
reverse order, this can be arranged for.

Maybe this shouldn't even be an optional Kconfig option, something
like this should be expected when you set the baud rate to zero
really. Comments welcome.
---
 drivers/serial/Kconfig      |    9 ++
 drivers/serial/amba-pl011.c |  183 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 189 insertions(+), 3 deletions(-)

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index aff9dcd..0328fba 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -323,6 +323,15 @@ config SERIAL_AMBA_PL011_CONSOLE
 	  your boot loader (lilo or loadlin) about how to pass options to the
 	  kernel at boot time.)
 
+config SERIAL_AMBA_PL011_CLOCK_CONTROL
+	bool "Support for clock control on AMBA serial port"
+	depends on SERIAL_AMBA_PL011
+	select CONSOLE_POLL
+	---help---
+	  Say Y here if you wish to use amba set_termios function to control
+	  the pl011 clock. Any positive baudrate passed enables clock,
+	  otherwise disables (baudrate = B0 -> hangup)
+
 config SERIAL_SB1250_DUART
 	tristate "BCM1xxx on-chip DUART serial support"
 	depends on SIBYTE_SB1xxx_SOC=y
diff --git a/drivers/serial/amba-pl011.c b/drivers/serial/amba-pl011.c
index e1b4652..5f4aca7 100644
--- a/drivers/serial/amba-pl011.c
+++ b/drivers/serial/amba-pl011.c
@@ -47,6 +47,7 @@
 #include <linux/serial.h>
 #include <linux/amba/bus.h>
 #include <linux/amba/serial.h>
+#include <linux/workqueue.h>
 #include <linux/clk.h>
 #include <linux/slab.h>
 #include <linux/dmaengine.h>
@@ -89,6 +90,15 @@ struct pl011_dma_tx_transaction {
 	dma_cookie_t cookie;
 };
 
+
+/* Available amba pl011 port clock states */
+enum pl011_clk_states {
+	PL011_CLK_OFF = 0,	/* clock disabled */
+	PL011_CLK_REQUEST_OFF,	/* disable after TX flushed */
+	PL011_CLK_ON,		/* clock enabled */
+	PL011_PORT_OFF,		/* port disabled */
+};
+
 /*
  * We wrap our port structure around the generic uart_port.
  */
@@ -113,6 +123,11 @@ struct uart_amba_port {
 	struct pl011_dma_rx_transaction dmarx;
 	struct pl011_dma_tx_transaction dmatx;
 #endif
+#ifdef CONFIG_SERIAL_AMBA_PL011_CLOCK_CONTROL
+	enum pl011_clk_states	clk_state;	/* actual clock state */
+	struct delayed_work	clk_off_work;	/* work used for clock off */
+	unsigned int		clk_off_delay;	/* clock off delay */
+#endif
 };
 
 /* There is by now at least one vendor with differing details, so handle it */
@@ -869,6 +884,155 @@ static inline int pl011_dma_tx_chars(struct uart_amba_port *uap)
 }
 #endif
 
+#ifdef CONFIG_SERIAL_AMBA_PL011_CLOCK_CONTROL
+/* Turn clock off if TX buffer is empty, otherwise reschedule */
+static void pl011_clock_off(struct work_struct *work)
+{
+	struct uart_amba_port *uap = container_of(work, struct uart_amba_port,
+						clk_off_work.work);
+	struct uart_port *port = &uap->port;
+	struct circ_buf *xmit = &port->state->xmit;
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+
+	if (uap->clk_state == PL011_CLK_REQUEST_OFF) {
+		if (uart_circ_empty(xmit)) {
+			clk_disable(uap->clk);
+			uap->clk_state = PL011_CLK_OFF;
+		} else
+			schedule_delayed_work(&uap->clk_off_work,
+						uap->clk_off_delay);
+	}
+
+	spin_unlock_irqrestore(&port->lock, flags);
+}
+
+/* Request to turn off uart clock once pending TX is flushed */
+static void pl011_clock_request_off(struct uart_port *port)
+{
+	unsigned long flags;
+	struct uart_amba_port *uap = (struct uart_amba_port *)(port);
+
+	spin_lock_irqsave(&port->lock, flags);
+
+	if (uap->clk_state == PL011_CLK_ON) {
+		uap->clk_state = PL011_CLK_REQUEST_OFF;
+		/* Turn off later */
+		schedule_delayed_work(&uap->clk_off_work,
+					uap->clk_off_delay);
+	}
+
+	spin_unlock_irqrestore(&port->lock, flags);
+}
+
+/* Request to immediately turn on uart clock */
+static void pl011_clock_on(struct uart_port *port)
+{
+	unsigned long flags;
+	struct uart_amba_port *uap = (struct uart_amba_port *)(port);
+
+	spin_lock_irqsave(&port->lock, flags);
+
+	switch (uap->clk_state) {
+	case PL011_CLK_OFF:
+		clk_enable(uap->clk);
+		/* fallthrough */
+	case PL011_CLK_REQUEST_OFF:
+		cancel_delayed_work(&uap->clk_off_work);
+		uap->clk_state = PL011_CLK_ON;
+		break;
+	default:
+		break;
+	}
+
+	spin_unlock_irqrestore(&port->lock, flags);
+}
+
+static void pl011_clock_check(struct uart_amba_port *uap)
+{
+	/* Reshedule work during off request  */
+	if (uap->clk_state == PL011_CLK_REQUEST_OFF)
+		/* New TX - restart work */
+		if (cancel_delayed_work(&uap->clk_off_work))
+			schedule_delayed_work(&uap->clk_off_work,
+						uap->clk_off_delay);
+}
+
+static int pl011_clock_startup(struct uart_amba_port *uap)
+{
+	int retval = 0;
+
+	if (uap->clk_state == PL011_PORT_OFF) {
+		retval = clk_enable(uap->clk);
+		if (!retval)
+			uap->clk_state = PL011_CLK_ON;
+		else
+			uap->clk_state = PL011_PORT_OFF;
+	}
+
+	return retval;
+}
+
+static void pl011_clock_shutdown(struct uart_amba_port *uap)
+{
+	spin_lock_irq(&uap->port.lock);
+	if (uap->clk_state == PL011_CLK_ON) {
+		clk_disable(uap->clk);
+		uap->clk_state = PL011_PORT_OFF;
+	}
+	spin_unlock_irq(&uap->port.lock);
+}
+
+static void
+pl011_clock_control(struct uart_port *port, struct ktermios *termios,
+		     struct ktermios *old)
+{
+	if (old) {
+		if ((termios->c_cflag & CBAUD) &&
+		    (termios->c_ispeed != old->c_ispeed ||
+		     termios->c_ospeed != old->c_ospeed)) {
+			if (termios->c_ispeed || termios->c_ospeed)
+				pl011_clock_on(port);
+			else
+				pl011_clock_request_off(port);
+		}
+	}
+}
+
+static void pl011_clock_control_init(struct uart_amba_port *uap)
+{
+	uap->clk_state = PL011_PORT_OFF;
+	INIT_DELAYED_WORK(&uap->clk_off_work, pl011_clock_off);
+	uap->clk_off_delay = HZ / 1000;  /* 1 ms */
+}
+
+#else
+/* Blank functions for clock control */
+static inline void pl011_clock_check(struct uart_amba_port *uap)
+{
+}
+
+static inline int pl011_clock_startup(struct uart_amba_port *uap)
+{
+	return clk_enable(uap->clk);
+}
+
+static inline void pl011_clock_shutdown(struct uart_amba_port *uap)
+{
+	clk_disable(uap->clk);
+}
+
+static inline void
+pl011_clock_control(struct uart_port *port, struct ktermios *termios,
+		     struct ktermios *old)
+{
+}
+
+static inline void pl011_clock_control_init(struct uart_amba_port *uap)
+{
+}
+#endif
 
 static void pl011_stop_tx(struct uart_port *port)
 {
@@ -1024,6 +1188,9 @@ static void pl011_tx_chars(struct uart_amba_port *uap)
 			break;
 	} while (--count > 0);
 
+	if (count)
+		pl011_clock_check(uap);
+
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 		uart_write_wakeup(&uap->port);
 
@@ -1212,7 +1379,7 @@ static int pl011_startup(struct uart_port *port)
 	/*
 	 * Try to enable the clock producer.
 	 */
-	retval = clk_enable(uap->clk);
+	retval = pl011_clock_startup(uap);
 	if (retval)
 		goto out;
 
@@ -1280,7 +1447,7 @@ static int pl011_startup(struct uart_port *port)
 	return 0;
 
  clk_dis:
-	clk_disable(uap->clk);
+	pl011_clock_shutdown(uap);
  out:
 	return retval;
 }
@@ -1331,7 +1498,7 @@ static void pl011_shutdown(struct uart_port *port)
 	/*
 	 * Shut down the clock producer
 	 */
-	clk_disable(uap->clk);
+	pl011_clock_shutdown(uap);
 }
 
 static void
@@ -1344,6 +1511,13 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
 	unsigned int baud, quot;
 
 	/*
+	 * Must be before uart_get_baud_rate() call, because
+	 * this function changes baudrate to default in case of 0
+	 * B0 hangup !!!
+	 */
+	pl011_clock_control(port, termios, old);
+
+	/*
 	 * Ask the core to calculate the divisor for us.
 	 */
 	baud = uart_get_baud_rate(port, termios, old, 0,
@@ -1721,6 +1895,9 @@ static int pl011_probe(struct amba_device *dev, struct amba_id *id)
 	amba_ports[i] = uap;
 
 	amba_set_drvdata(dev, uap);
+
+	pl011_clock_control_init(uap);
+
 	ret = uart_add_one_port(&amba_reg, &uap->port);
 	if (ret) {
 		amba_set_drvdata(dev, NULL);
-- 
1.6.3.3

Re: [PATCH] pl011: added clock management feature

[Russell King - ARM Linux]

On Tue, Nov 09, 2010 at 04:30:37PM +0100, Linus Walleij wrote:
> From: Grzegorz Sygieda <grzegorz.sygieda@tieto.com>
> 
> This patch allows to control the pl011 clock using set_termios
> callback. Any positive baudrate passed enables clock, otherwise
> disables. This saves a lot of power on submicron designs since
> we can clock off and disable unused UARTs.

Why not just close the port to save power - the clock in that case will
automatically be turned off.

It's not like the port can be used without the baud rate correctly set -
or indeed characters transmitted or received without the clock running.
So all the time that the clock is disabled, the port is inoperable.

If you want to maintain the modem control line settings while the port
is closed, why not clear the HUPCL termios flag?

[PATCH] pl011: added clock management feature

[Russell King - ARM Linux]

On Tue, Nov 09, 2010 at 04:30:37PM +0100, Linus Walleij wrote:
> From: Grzegorz Sygieda <grzegorz.sygieda@tieto.com>
> 
> This patch allows to control the pl011 clock using set_termios
> callback. Any positive baudrate passed enables clock, otherwise
> disables. This saves a lot of power on submicron designs since
> we can clock off and disable unused UARTs.

Why not just close the port to save power - the clock in that case will
automatically be turned off.

It's not like the port can be used without the baud rate correctly set -
or indeed characters transmitted or received without the clock running.
So all the time that the clock is disabled, the port is inoperable.

If you want to maintain the modem control line settings while the port
is closed, why not clear the HUPCL termios flag?

Re: [PATCH] pl011: added clock management feature

[Greg KH]

On Tue, Nov 09, 2010 at 04:30:37PM +0100, Linus Walleij wrote:
> From: Grzegorz Sygieda <grzegorz.sygieda@tieto.com>
> 
> This patch allows to control the pl011 clock using set_termios
> callback. Any positive baudrate passed enables clock, otherwise
> disables. This saves a lot of power on submicron designs since
> we can clock off and disable unused UARTs.

That's nice, but it seems like an overload of what people traditionally
think of when it comes to baud rates.  Why not just power down ports
that are not open instead?

thanks,

greg k-h

[PATCH] pl011: added clock management feature

[Greg KH]

On Tue, Nov 09, 2010 at 04:30:37PM +0100, Linus Walleij wrote:
> From: Grzegorz Sygieda <grzegorz.sygieda@tieto.com>
> 
> This patch allows to control the pl011 clock using set_termios
> callback. Any positive baudrate passed enables clock, otherwise
> disables. This saves a lot of power on submicron designs since
> we can clock off and disable unused UARTs.

That's nice, but it seems like an overload of what people traditionally
think of when it comes to baud rates.  Why not just power down ports
that are not open instead?

thanks,

greg k-h

Re: [PATCH] pl011: added clock management feature

[Russell King - ARM Linux]

On Tue, Nov 09, 2010 at 02:40:12PM -0800, Greg KH wrote:
> On Tue, Nov 09, 2010 at 04:30:37PM +0100, Linus Walleij wrote:
> > From: Grzegorz Sygieda <grzegorz.sygieda@tieto.com>
> > 
> > This patch allows to control the pl011 clock using set_termios
> > callback. Any positive baudrate passed enables clock, otherwise
> > disables. This saves a lot of power on submicron designs since
> > we can clock off and disable unused UARTs.
> 
> That's nice, but it seems like an overload of what people traditionally
> think of when it comes to baud rates.  Why not just power down ports
> that are not open instead?

We already do.  My question to Linus (in a previous message) is why this
isn't sufficient.

[PATCH] pl011: added clock management feature

[Russell King - ARM Linux]

On Tue, Nov 09, 2010 at 02:40:12PM -0800, Greg KH wrote:
> On Tue, Nov 09, 2010 at 04:30:37PM +0100, Linus Walleij wrote:
> > From: Grzegorz Sygieda <grzegorz.sygieda@tieto.com>
> > 
> > This patch allows to control the pl011 clock using set_termios
> > callback. Any positive baudrate passed enables clock, otherwise
> > disables. This saves a lot of power on submicron designs since
> > we can clock off and disable unused UARTs.
> 
> That's nice, but it seems like an overload of what people traditionally
> think of when it comes to baud rates.  Why not just power down ports
> that are not open instead?

We already do.  My question to Linus (in a previous message) is why this
isn't sufficient.

RE: [PATCH] pl011: added clock management feature

[Grzegorz.Sygieda]

>On Tue, Nov 09, 2010 at 02:40:12PM -0800, Greg KH wrote:
>> On Tue, Nov 09, 2010 at 04:30:37PM +0100, Linus Walleij wrote:
>> > From: Grzegorz Sygieda <grzegorz.sygieda@tieto.com>
>> > 
>> > This patch allows to control the pl011 clock using set_termios 
>> > callback. Any positive baudrate passed enables clock, otherwise 
>> > disables. This saves a lot of power on submicron designs since we 
>> > can clock off and disable unused UARTs.
>> 
>> That's nice, but it seems like an overload of what people 
>> traditionally think of when it comes to baud rates.  Why not just 
>> power down ports that are not open instead?
>
>We already do.  My question to Linus (in a previous message) is why this isn't sufficient.

The main goal was to disable/enable clock while port open. This is usefull for scenario, where some higher level driver wants to control the power consumption (using set_termios). In the same time a user-space app (eg. hciattach) is still bounded to the specific /dev/tty* device associated with particular uart. From user POV device is always open, and app does not have to respawn, and we can save power.

I hope it's clear explanation. 

[PATCH] pl011: added clock management feature

[Grzegorz.Sygieda at tieto.com]

>On Tue, Nov 09, 2010 at 02:40:12PM -0800, Greg KH wrote:
>> On Tue, Nov 09, 2010 at 04:30:37PM +0100, Linus Walleij wrote:
>> > From: Grzegorz Sygieda <grzegorz.sygieda@tieto.com>
>> > 
>> > This patch allows to control the pl011 clock using set_termios 
>> > callback. Any positive baudrate passed enables clock, otherwise 
>> > disables. This saves a lot of power on submicron designs since we 
>> > can clock off and disable unused UARTs.
>> 
>> That's nice, but it seems like an overload of what people 
>> traditionally think of when it comes to baud rates.  Why not just 
>> power down ports that are not open instead?
>
>We already do.  My question to Linus (in a previous message) is why this isn't sufficient.

The main goal was to disable/enable clock while port open. This is usefull for scenario, where some higher level driver wants to control the power consumption (using set_termios). In the same time a user-space app (eg. hciattach) is still bounded to the specific /dev/tty* device associated with particular uart. From user POV device is always open, and app does not have to respawn, and we can save power.

I hope it's clear explanation. 

RE: [PATCH] pl011: added clock management feature

[Grzegorz.Sygieda]

>On Tue, Nov 09, 2010 at 02:40:12PM -0800, Greg KH wrote:
>> On Tue, Nov 09, 2010 at 04:30:37PM +0100, Linus Walleij wrote:
>> > From: Grzegorz Sygieda <grzegorz.sygieda@tieto.com>
>> > 
>> > This patch allows to control the pl011 clock using set_termios 
>> > callback. Any positive baudrate passed enables clock, otherwise 
>> > disables. This saves a lot of power on submicron designs since we 
>> > can clock off and disable unused UARTs.
>> 
>> That's nice, but it seems like an overload of what people 
>> traditionally think of when it comes to baud rates.  Why not just 
>> power down ports that are not open instead?
>
>We already do.  My question to Linus (in a previous message) is why this isn't sufficient.

The main goal was to disable/enable clock while port open. This is usefull for scenario, where some higher level driver wants to control the power consumption (using set_termios). In the same time a user-space app (eg. hciattach) is still bounded to the specific /dev/tty* device associated with particular uart. From user POV device is always open, and app does not have to respawn, and we can save power.

I hope it's clear explanation. 

[PATCH] pl011: added clock management feature

[Grzegorz.Sygieda at tieto.com]

>On Tue, Nov 09, 2010 at 02:40:12PM -0800, Greg KH wrote:
>> On Tue, Nov 09, 2010 at 04:30:37PM +0100, Linus Walleij wrote:
>> > From: Grzegorz Sygieda <grzegorz.sygieda@tieto.com>
>> > 
>> > This patch allows to control the pl011 clock using set_termios 
>> > callback. Any positive baudrate passed enables clock, otherwise 
>> > disables. This saves a lot of power on submicron designs since we 
>> > can clock off and disable unused UARTs.
>> 
>> That's nice, but it seems like an overload of what people 
>> traditionally think of when it comes to baud rates.  Why not just 
>> power down ports that are not open instead?
>
>We already do.  My question to Linus (in a previous message) is why this isn't sufficient.

The main goal was to disable/enable clock while port open. This is usefull for scenario, where some higher level driver wants to control the power consumption (using set_termios). In the same time a user-space app (eg. hciattach) is still bounded to the specific /dev/tty* device associated with particular uart. From user POV device is always open, and app does not have to respawn, and we can save power.

I hope it's clear explanation. 

Re: [PATCH] pl011: added clock management feature

[Greg KH]

On Wed, Nov 10, 2010 at 10:15:53AM +0200, Grzegorz.Sygieda@tieto.com wrote:
> >On Tue, Nov 09, 2010 at 02:40:12PM -0800, Greg KH wrote:
> >> On Tue, Nov 09, 2010 at 04:30:37PM +0100, Linus Walleij wrote:
> >> > From: Grzegorz Sygieda <grzegorz.sygieda@tieto.com>
> >> > 
> >> > This patch allows to control the pl011 clock using set_termios 
> >> > callback. Any positive baudrate passed enables clock, otherwise 
> >> > disables. This saves a lot of power on submicron designs since we 
> >> > can clock off and disable unused UARTs.
> >> 
> >> That's nice, but it seems like an overload of what people 
> >> traditionally think of when it comes to baud rates.  Why not just 
> >> power down ports that are not open instead?
> >
> >We already do.  My question to Linus (in a previous message) is why this isn't sufficient.
> 
> The main goal was to disable/enable clock while port open. This is
> usefull for scenario, where some higher level driver wants to control
> the power consumption (using set_termios). In the same time a
> user-space app (eg. hciattach) is still bounded to the specific
> /dev/tty* device associated with particular uart. From user POV device
> is always open, and app does not have to respawn, and we can save
> power.

That is nice, but again, you are overloading a common interface (one
defined by POSIX I think) to do something else at the same time.  That
might cause problems with some users that expect you to be able to use a
baud rate of 0 :)

I like the idea, but not the overloading, sorry.

thanks,

greg k-h

[PATCH] pl011: added clock management feature

[Greg KH]

On Wed, Nov 10, 2010 at 10:15:53AM +0200, Grzegorz.Sygieda at tieto.com wrote:
> >On Tue, Nov 09, 2010 at 02:40:12PM -0800, Greg KH wrote:
> >> On Tue, Nov 09, 2010 at 04:30:37PM +0100, Linus Walleij wrote:
> >> > From: Grzegorz Sygieda <grzegorz.sygieda@tieto.com>
> >> > 
> >> > This patch allows to control the pl011 clock using set_termios 
> >> > callback. Any positive baudrate passed enables clock, otherwise 
> >> > disables. This saves a lot of power on submicron designs since we 
> >> > can clock off and disable unused UARTs.
> >> 
> >> That's nice, but it seems like an overload of what people 
> >> traditionally think of when it comes to baud rates.  Why not just 
> >> power down ports that are not open instead?
> >
> >We already do.  My question to Linus (in a previous message) is why this isn't sufficient.
> 
> The main goal was to disable/enable clock while port open. This is
> usefull for scenario, where some higher level driver wants to control
> the power consumption (using set_termios). In the same time a
> user-space app (eg. hciattach) is still bounded to the specific
> /dev/tty* device associated with particular uart. From user POV device
> is always open, and app does not have to respawn, and we can save
> power.

That is nice, but again, you are overloading a common interface (one
defined by POSIX I think) to do something else at the same time.  That
might cause problems with some users that expect you to be able to use a
baud rate of 0 :)

I like the idea, but not the overloading, sorry.

thanks,

greg k-h

Re: [PATCH] pl011: added clock management feature

[Russell King - ARM Linux]

On Wed, Nov 10, 2010 at 10:15:53AM +0200, Grzegorz.Sygieda@tieto.com wrote:
> The main goal was to disable/enable clock while port open. This is usefull
> for scenario, where some higher level driver wants to control the power
> consumption (using set_termios). In the same time a user-space app (eg.
> hciattach) is still bounded to the specific /dev/tty* device associated
> with particular uart. From user POV device is always open, and app does
> not have to respawn, and we can save power.

If a port is open, and bound (eg to a bluetooth hci), how does the higher
level decide whether to place the port in low power mode?  How does it
know that there isn't going to be characters sent to the port?

When the port is in this low power mode, it's as good as closed from the
external world point of view.

I don't like this idea because I really can't see how it could be used
effectively - there isn't really any way for a bound protocol to know
whether there's characters expected to be received or not.  Either the
port is open and in use, or it isn't.

Let's take an example of a modem waiting for an incoming call.  It spends
most of its time waiting for the call.  You could argue that you could
shut the clock off to the port to save power - but then how do you receive
the "RING" message from the modem?  As you've shut the clock off, you're
not going to detect the activity on the receive line.

I'm sure that also applies to bluetooth as well, especially if you're a
'client' device - as long as the port is attached, bluetooth probably
expects to be able to listen to what's going on so the device can be
discovered.

It's all very well adding hooks to the transmit path to ensure that there
clock is on while there's characters to be sent, but this really doesn't
help you on the receive side.

I'm worried...

[PATCH] pl011: added clock management feature

[Russell King - ARM Linux]

On Wed, Nov 10, 2010 at 10:15:53AM +0200, Grzegorz.Sygieda at tieto.com wrote:
> The main goal was to disable/enable clock while port open. This is usefull
> for scenario, where some higher level driver wants to control the power
> consumption (using set_termios). In the same time a user-space app (eg.
> hciattach) is still bounded to the specific /dev/tty* device associated
> with particular uart. From user POV device is always open, and app does
> not have to respawn, and we can save power.

If a port is open, and bound (eg to a bluetooth hci), how does the higher
level decide whether to place the port in low power mode?  How does it
know that there isn't going to be characters sent to the port?

When the port is in this low power mode, it's as good as closed from the
external world point of view.

I don't like this idea because I really can't see how it could be used
effectively - there isn't really any way for a bound protocol to know
whether there's characters expected to be received or not.  Either the
port is open and in use, or it isn't.

Let's take an example of a modem waiting for an incoming call.  It spends
most of its time waiting for the call.  You could argue that you could
shut the clock off to the port to save power - but then how do you receive
the "RING" message from the modem?  As you've shut the clock off, you're
not going to detect the activity on the receive line.

I'm sure that also applies to bluetooth as well, especially if you're a
'client' device - as long as the port is attached, bluetooth probably
expects to be able to listen to what's going on so the device can be
discovered.

It's all very well adding hooks to the transmit path to ensure that there
clock is on while there's characters to be sent, but this really doesn't
help you on the receive side.

I'm worried...

RE: [PATCH] pl011: added clock management feature

[Grzegorz.Sygieda]

> If a port is open, and bound (eg to a bluetooth hci), how does the higher level decide whether to place the port in low power mode?  How does it know that there isn't going to be > >characters sent to the port?
> 
> When the port is in this low power mode, it's as good as closed from the external world point of view.
> 
> I don't like this idea because I really can't see how it could be used effectively - there isn't really any way for a bound protocol to know whether there's characters expected to be received > or not.  Either the port is open and in use, or it isn't.
> 
> Let's take an example of a modem waiting for an incoming call.  It spends most of its time waiting for the call.  You could argue that you could shut the clock off to the port to save > power - but then how do you receive the "RING" message from the modem?  As you've shut the clock off, you're not going to detect the activity on the receive line.
> 
> I'm sure that also applies to bluetooth as well, especially if you're a 'client' device - as long as the port is attached, bluetooth probably expects to be able to listen to what's going on so >the device can be discovered.
>
> It's all very well adding hooks to the transmit path to ensure that there clock is on while there's characters to be sent, but this really doesn't help you on the receive side.
> 
> I'm worried...

We have chip which acts (is registred) as platform_device, and have knowledge about its status through GPIO, (eg. Wake-up line), as well as it can control low power mode. This driver registers to hci discipline, thus gives posibliity to invoke tty functions, including set_termios. And this is what we are actualy doing. We think that this is best way to keep clock control without closing the uart.

We don't think, if it possible to shutdown and then startup the uart port completly, from the chip's driver level. 

Correct us if We're wrong.

[PATCH] pl011: added clock management feature

[Grzegorz.Sygieda at tieto.com]

> If a port is open, and bound (eg to a bluetooth hci), how does the higher level decide whether to place the port in low power mode?  How does it know that there isn't going to be > >characters sent to the port?
> 
> When the port is in this low power mode, it's as good as closed from the external world point of view.
> 
> I don't like this idea because I really can't see how it could be used effectively - there isn't really any way for a bound protocol to know whether there's characters expected to be received > or not.  Either the port is open and in use, or it isn't.
> 
> Let's take an example of a modem waiting for an incoming call.  It spends most of its time waiting for the call.  You could argue that you could shut the clock off to the port to save > power - but then how do you receive the "RING" message from the modem?  As you've shut the clock off, you're not going to detect the activity on the receive line.
> 
> I'm sure that also applies to bluetooth as well, especially if you're a 'client' device - as long as the port is attached, bluetooth probably expects to be able to listen to what's going on so >the device can be discovered.
>
> It's all very well adding hooks to the transmit path to ensure that there clock is on while there's characters to be sent, but this really doesn't help you on the receive side.
> 
> I'm worried...

We have chip which acts (is registred) as platform_device, and have knowledge about its status through GPIO, (eg. Wake-up line), as well as it can control low power mode. This driver registers to hci discipline, thus gives posibliity to invoke tty functions, including set_termios. And this is what we are actualy doing. We think that this is best way to keep clock control without closing the uart.

We don't think, if it possible to shutdown and then startup the uart port completly, from the chip's driver level. 

Correct us if We're wrong.

Re: [PATCH] pl011: added clock management feature

[Vitaly Wool]

On Tue, Nov 9, 2010 at 11:40 PM, Greg KH <greg@kroah.com> wrote:
> On Tue, Nov 09, 2010 at 04:30:37PM +0100, Linus Walleij wrote:
>> From: Grzegorz Sygieda <grzegorz.sygieda@tieto.com>
>>
>> This patch allows to control the pl011 clock using set_termios
>> callback. Any positive baudrate passed enables clock, otherwise
>> disables. This saves a lot of power on submicron designs since
>> we can clock off and disable unused UARTs.
>
> That's nice, but it seems like an overload of what people traditionally
> think of when it comes to baud rates.  Why not just power down ports
> that are not open instead?

I like the idea as well and this is definitely gonna help conserving
the power for Bluetooth UARTs.

But using baud rate 0 is something I don't like. Why don't you stop
the clocks if RTS is cleared? This would have allowed the line
discipline driver to implicitly control the UART clock and there will
be no risk of losing data, as well as no non-standard behavior
involved. In fact, you'll be transparent to the upper layers in this
case.

~Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] pl011: added clock management feature

[Vitaly Wool]

On Tue, Nov 9, 2010 at 11:40 PM, Greg KH <greg@kroah.com> wrote:
> On Tue, Nov 09, 2010 at 04:30:37PM +0100, Linus Walleij wrote:
>> From: Grzegorz Sygieda <grzegorz.sygieda@tieto.com>
>>
>> This patch allows to control the pl011 clock using set_termios
>> callback. Any positive baudrate passed enables clock, otherwise
>> disables. This saves a lot of power on submicron designs since
>> we can clock off and disable unused UARTs.
>
> That's nice, but it seems like an overload of what people traditionally
> think of when it comes to baud rates. ?Why not just power down ports
> that are not open instead?

I like the idea as well and this is definitely gonna help conserving
the power for Bluetooth UARTs.

But using baud rate 0 is something I don't like. Why don't you stop
the clocks if RTS is cleared? This would have allowed the line
discipline driver to implicitly control the UART clock and there will
be no risk of losing data, as well as no non-standard behavior
involved. In fact, you'll be transparent to the upper layers in this
case.

~Vitaly

Re: [PATCH] pl011: added clock management feature

[Russell King - ARM Linux]

On Fri, Dec 03, 2010 at 12:47:14PM +0100, Vitaly Wool wrote:
> On Tue, Nov 9, 2010 at 11:40 PM, Greg KH <greg@kroah.com> wrote:
> > On Tue, Nov 09, 2010 at 04:30:37PM +0100, Linus Walleij wrote:
> >> From: Grzegorz Sygieda <grzegorz.sygieda@tieto.com>
> >>
> >> This patch allows to control the pl011 clock using set_termios
> >> callback. Any positive baudrate passed enables clock, otherwise
> >> disables. This saves a lot of power on submicron designs since
> >> we can clock off and disable unused UARTs.
> >
> > That's nice, but it seems like an overload of what people traditionally
> > think of when it comes to baud rates.  Why not just power down ports
> > that are not open instead?
> 
> I like the idea as well and this is definitely gonna help conserving
> the power for Bluetooth UARTs.
> 
> But using baud rate 0 is something I don't like.

You can't use the baud rate of 0 to power down ports - that already has
a POSIX-defined function (hang up).

> Why don't you stop
> the clocks if RTS is cleared? This would have allowed the line
> discipline driver to implicitly control the UART clock and there will
> be no risk of losing data, as well as no non-standard behavior
> involved. In fact, you'll be transparent to the upper layers in this
> case.

I've no idea what you're thinking, but you can't stop the UART clock
because RTS is deasserted - or DTR for that matter.  Neither of those
two define whether characters will be transmitted or received.

There are specialist protocols and devices out there which require
RTS and DTR to be held at certain non-standard states for them to work.
One range of devices are car OBD-II interfaces, where RTS controls the
L line and sometimes the TX/RX direction on the bidirectional K data
line.

Similar things are done with DTR, and there's protocols which use
DTR as a handshake.
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] pl011: added clock management feature

[Russell King - ARM Linux]

On Fri, Dec 03, 2010 at 12:47:14PM +0100, Vitaly Wool wrote:
> On Tue, Nov 9, 2010 at 11:40 PM, Greg KH <greg@kroah.com> wrote:
> > On Tue, Nov 09, 2010 at 04:30:37PM +0100, Linus Walleij wrote:
> >> From: Grzegorz Sygieda <grzegorz.sygieda@tieto.com>
> >>
> >> This patch allows to control the pl011 clock using set_termios
> >> callback. Any positive baudrate passed enables clock, otherwise
> >> disables. This saves a lot of power on submicron designs since
> >> we can clock off and disable unused UARTs.
> >
> > That's nice, but it seems like an overload of what people traditionally
> > think of when it comes to baud rates. ?Why not just power down ports
> > that are not open instead?
> 
> I like the idea as well and this is definitely gonna help conserving
> the power for Bluetooth UARTs.
> 
> But using baud rate 0 is something I don't like.

You can't use the baud rate of 0 to power down ports - that already has
a POSIX-defined function (hang up).

> Why don't you stop
> the clocks if RTS is cleared? This would have allowed the line
> discipline driver to implicitly control the UART clock and there will
> be no risk of losing data, as well as no non-standard behavior
> involved. In fact, you'll be transparent to the upper layers in this
> case.

I've no idea what you're thinking, but you can't stop the UART clock
because RTS is deasserted - or DTR for that matter.  Neither of those
two define whether characters will be transmitted or received.

There are specialist protocols and devices out there which require
RTS and DTR to be held at certain non-standard states for them to work.
One range of devices are car OBD-II interfaces, where RTS controls the
L line and sometimes the TX/RX direction on the bidirectional K data
line.

Similar things are done with DTR, and there's protocols which use
DTR as a handshake.

Re: [PATCH] pl011: added clock management feature

[Vitaly Wool]

Hi Russell,

On Fri, Dec 3, 2010 at 4:15 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
>> Why don't you stop
>> the clocks if RTS is cleared? This would have allowed the line
>> discipline driver to implicitly control the UART clock and there will
>> be no risk of losing data, as well as no non-standard behavior
>> involved. In fact, you'll be transparent to the upper layers in this
>> case.
>
> I've no idea what you're thinking, but you can't stop the UART clock
> because RTS is deasserted - or DTR for that matter.  Neither of those
> two define whether characters will be transmitted or received.

What I'm mostly up to (and I guess Par also is) is how to conserve
power for the Bluetooth UARTs. For a Bluetooth UART, there usually is
a kind of signalling protocol, either inband or out-of-band, that
allows the host and the chip to negotiate about power conservation
during inactivity periods. These protocols do not belong to UART
implementation and are usually implemented as line discipline drivers.
While BT chip is sleeping, we don't need the UART clock running but we
need to have a decent way of telling the UART driver it can shut those
off. Using RTS for that, even though not being applicable in all the
cases, seems to work pretty well for Bluetooth UARTs. So if we just
add a flag if an UART is used for BT or not and then use RTS for this
kind of communication, will it be okay with you?

Thanks,
   Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] pl011: added clock management feature

[Vitaly Wool]

Hi Russell,

On Fri, Dec 3, 2010 at 4:15 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
>> Why don't you stop
>> the clocks if RTS is cleared? This would have allowed the line
>> discipline driver to implicitly control the UART clock and there will
>> be no risk of losing data, as well as no non-standard behavior
>> involved. In fact, you'll be transparent to the upper layers in this
>> case.
>
> I've no idea what you're thinking, but you can't stop the UART clock
> because RTS is deasserted - or DTR for that matter. ?Neither of those
> two define whether characters will be transmitted or received.

What I'm mostly up to (and I guess Par also is) is how to conserve
power for the Bluetooth UARTs. For a Bluetooth UART, there usually is
a kind of signalling protocol, either inband or out-of-band, that
allows the host and the chip to negotiate about power conservation
during inactivity periods. These protocols do not belong to UART
implementation and are usually implemented as line discipline drivers.
While BT chip is sleeping, we don't need the UART clock running but we
need to have a decent way of telling the UART driver it can shut those
off. Using RTS for that, even though not being applicable in all the
cases, seems to work pretty well for Bluetooth UARTs. So if we just
add a flag if an UART is used for BT or not and then use RTS for this
kind of communication, will it be okay with you?

Thanks,
   Vitaly

Re: [PATCH] pl011: added clock management feature

[Russell King - ARM Linux]

On Mon, Dec 06, 2010 at 10:53:20AM +0100, Vitaly Wool wrote:
> Hi Russell,
> 
> On Fri, Dec 3, 2010 at 4:15 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> >> Why don't you stop
> >> the clocks if RTS is cleared? This would have allowed the line
> >> discipline driver to implicitly control the UART clock and there will
> >> be no risk of losing data, as well as no non-standard behavior
> >> involved. In fact, you'll be transparent to the upper layers in this
> >> case.
> >
> > I've no idea what you're thinking, but you can't stop the UART clock
> > because RTS is deasserted - or DTR for that matter.  Neither of those
> > two define whether characters will be transmitted or received.
> 
> What I'm mostly up to (and I guess Par also is) is how to conserve
> power for the Bluetooth UARTs. For a Bluetooth UART, there usually is
> a kind of signalling protocol, either inband or out-of-band, that
> allows the host and the chip to negotiate about power conservation
> during inactivity periods. These protocols do not belong to UART
> implementation and are usually implemented as line discipline drivers.
> While BT chip is sleeping, we don't need the UART clock running but we
> need to have a decent way of telling the UART driver it can shut those
> off. Using RTS for that, even though not being applicable in all the
> cases, seems to work pretty well for Bluetooth UARTs. So if we just
> add a flag if an UART is used for BT or not and then use RTS for this
> kind of communication, will it be okay with you?

So you want to teach each UART driver a protocol-specific method of
handling power management rather than implementing a sane API to do
this?

Please, come up with a sane API for power management of UARTs rather
than trying to hack protocol specific stuff into UART drivers.
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] pl011: added clock management feature

[Russell King - ARM Linux]

On Mon, Dec 06, 2010 at 10:53:20AM +0100, Vitaly Wool wrote:
> Hi Russell,
> 
> On Fri, Dec 3, 2010 at 4:15 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> >> Why don't you stop
> >> the clocks if RTS is cleared? This would have allowed the line
> >> discipline driver to implicitly control the UART clock and there will
> >> be no risk of losing data, as well as no non-standard behavior
> >> involved. In fact, you'll be transparent to the upper layers in this
> >> case.
> >
> > I've no idea what you're thinking, but you can't stop the UART clock
> > because RTS is deasserted - or DTR for that matter. ?Neither of those
> > two define whether characters will be transmitted or received.
> 
> What I'm mostly up to (and I guess Par also is) is how to conserve
> power for the Bluetooth UARTs. For a Bluetooth UART, there usually is
> a kind of signalling protocol, either inband or out-of-band, that
> allows the host and the chip to negotiate about power conservation
> during inactivity periods. These protocols do not belong to UART
> implementation and are usually implemented as line discipline drivers.
> While BT chip is sleeping, we don't need the UART clock running but we
> need to have a decent way of telling the UART driver it can shut those
> off. Using RTS for that, even though not being applicable in all the
> cases, seems to work pretty well for Bluetooth UARTs. So if we just
> add a flag if an UART is used for BT or not and then use RTS for this
> kind of communication, will it be okay with you?

So you want to teach each UART driver a protocol-specific method of
handling power management rather than implementing a sane API to do
this?

Please, come up with a sane API for power management of UARTs rather
than trying to hack protocol specific stuff into UART drivers.

RE: [PATCH] pl011: added clock management feature

[Par-Gunnar HJALMDAHL]

Hi Russell,

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: den 6 december 2010 11:14
> To: Vitaly Wool
> Cc: Greg KH; Linus WALLEIJ; Par-Gunnar HJALMDAHL; Greg Kroah-Hartman;
> Lukasz Rymanowski; Grzegorz Sygieda; linux-serial@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH] pl011: added clock management feature
> 
> On Mon, Dec 06, 2010 at 10:53:20AM +0100, Vitaly Wool wrote:
> > Hi Russell,
> >
> > On Fri, Dec 3, 2010 at 4:15 PM, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> > >> Why don't you stop
> > >> the clocks if RTS is cleared? This would have allowed the line
> > >> discipline driver to implicitly control the UART clock and there
> will
> > >> be no risk of losing data, as well as no non-standard behavior
> > >> involved. In fact, you'll be transparent to the upper layers in
> this
> > >> case.
> > >
> > > I've no idea what you're thinking, but you can't stop the UART
> clock
> > > because RTS is deasserted - or DTR for that matter.  Neither of
> those
> > > two define whether characters will be transmitted or received.
> >
> > What I'm mostly up to (and I guess Par also is) is how to conserve
> > power for the Bluetooth UARTs. For a Bluetooth UART, there usually is
> > a kind of signalling protocol, either inband or out-of-band, that
> > allows the host and the chip to negotiate about power conservation
> > during inactivity periods. These protocols do not belong to UART
> > implementation and are usually implemented as line discipline
> drivers.
> > While BT chip is sleeping, we don't need the UART clock running but
> we
> > need to have a decent way of telling the UART driver it can shut
> those
> > off. Using RTS for that, even though not being applicable in all the
> > cases, seems to work pretty well for Bluetooth UARTs. So if we just
> > add a flag if an UART is used for BT or not and then use RTS for this
> > kind of communication, will it be okay with you?
> 
> So you want to teach each UART driver a protocol-specific method of
> handling power management rather than implementing a sane API to do
> this?
> 
> Please, come up with a sane API for power management of UARTs rather
> than trying to hack protocol specific stuff into UART drivers.

So I guess the best solution would then be to add function(s) to struct tty_operations in tty_driver.h? Something like
void (*clk_enable)(struct tty_struct *tty);
void (*clk_disable)(struct tty_struct *tty);
and then it is up to each UART driver to implement them? Would such a solution be OK?

/P-G

[PATCH] pl011: added clock management feature

[Par-Gunnar HJALMDAHL]

Hi Russell,

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: den 6 december 2010 11:14
> To: Vitaly Wool
> Cc: Greg KH; Linus WALLEIJ; Par-Gunnar HJALMDAHL; Greg Kroah-Hartman;
> Lukasz Rymanowski; Grzegorz Sygieda; linux-serial at vger.kernel.org;
> linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH] pl011: added clock management feature
> 
> On Mon, Dec 06, 2010 at 10:53:20AM +0100, Vitaly Wool wrote:
> > Hi Russell,
> >
> > On Fri, Dec 3, 2010 at 4:15 PM, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> > >> Why don't you stop
> > >> the clocks if RTS is cleared? This would have allowed the line
> > >> discipline driver to implicitly control the UART clock and there
> will
> > >> be no risk of losing data, as well as no non-standard behavior
> > >> involved. In fact, you'll be transparent to the upper layers in
> this
> > >> case.
> > >
> > > I've no idea what you're thinking, but you can't stop the UART
> clock
> > > because RTS is deasserted - or DTR for that matter. ?Neither of
> those
> > > two define whether characters will be transmitted or received.
> >
> > What I'm mostly up to (and I guess Par also is) is how to conserve
> > power for the Bluetooth UARTs. For a Bluetooth UART, there usually is
> > a kind of signalling protocol, either inband or out-of-band, that
> > allows the host and the chip to negotiate about power conservation
> > during inactivity periods. These protocols do not belong to UART
> > implementation and are usually implemented as line discipline
> drivers.
> > While BT chip is sleeping, we don't need the UART clock running but
> we
> > need to have a decent way of telling the UART driver it can shut
> those
> > off. Using RTS for that, even though not being applicable in all the
> > cases, seems to work pretty well for Bluetooth UARTs. So if we just
> > add a flag if an UART is used for BT or not and then use RTS for this
> > kind of communication, will it be okay with you?
> 
> So you want to teach each UART driver a protocol-specific method of
> handling power management rather than implementing a sane API to do
> this?
> 
> Please, come up with a sane API for power management of UARTs rather
> than trying to hack protocol specific stuff into UART drivers.

So I guess the best solution would then be to add function(s) to struct tty_operations in tty_driver.h? Something like
void (*clk_enable)(struct tty_struct *tty);
void (*clk_disable)(struct tty_struct *tty);
and then it is up to each UART driver to implement them? Would such a solution be OK?

/P-G