[PATCH] amba-pl011: support hardware flow control

[PATCH] amba-pl011: support hardware flow control

[Rabin Vincent]

Enable/disable automatic hardware flow control as requested by the
termios.  The controller does not allow us to control the RTS line when
auto-RTS is enabled, so we enable auto-RTS only if the kernel has not
disabled RTS.

Acked-by: Linus Walleij <linus.walleij@stericsson.com>
Signed-off-by: Rabin Vincent <rabin.vincent@stericsson.com>
---
 drivers/serial/amba-pl011.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/drivers/serial/amba-pl011.c b/drivers/serial/amba-pl011.c
index ef7adc8..ce6c353 100644
--- a/drivers/serial/amba-pl011.c
+++ b/drivers/serial/amba-pl011.c
@@ -71,6 +71,7 @@ struct uart_amba_port {
 	unsigned int		im;	/* interrupt mask */
 	unsigned int		old_status;
 	unsigned int		ifls;	/* vendor-specific */
+	bool			autorts;
 };
 
 /* There is by now at least one vendor with differing details, so handle it */
@@ -308,6 +309,11 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl)
 	TIOCMBIT(TIOCM_OUT1, UART011_CR_OUT1);
 	TIOCMBIT(TIOCM_OUT2, UART011_CR_OUT2);
 	TIOCMBIT(TIOCM_LOOP, UART011_CR_LBE);
+
+	if (uap->autorts) {
+		/* We need to disable auto-RTS if we want to turn RTS off */
+		TIOCMBIT(TIOCM_RTS, UART011_CR_RTSEN);
+	}
 #undef TIOCMBIT
 
 	writew(cr, uap->port.membase + UART011_CR);
@@ -437,6 +443,7 @@ static void pl011_shutdown(struct uart_port *port)
 	/*
 	 * disable the port
 	 */
+	uap->autorts = false;
 	writew(UART01x_CR_UARTEN | UART011_CR_TXE, uap->port.membase + UART011_CR);
 
 	/*
@@ -456,6 +463,7 @@ static void
 pl011_set_termios(struct uart_port *port, struct ktermios *termios,
 		     struct ktermios *old)
 {
+	struct uart_amba_port *uap = (struct uart_amba_port *)port;
 	unsigned int lcr_h, old_cr;
 	unsigned long flags;
 	unsigned int baud, quot;
@@ -532,6 +540,17 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
 	old_cr = readw(port->membase + UART011_CR);
 	writew(0, port->membase + UART011_CR);
 
+	if (termios->c_cflag & CRTSCTS) {
+		if (old_cr & UART011_CR_RTS)
+			old_cr |= UART011_CR_RTSEN;
+
+		old_cr |= UART011_CR_CTSEN;
+		uap->autorts = true;
+	} else {
+		old_cr &= ~(UART011_CR_CTSEN | UART011_CR_RTSEN);
+		uap->autorts = false;
+	}
+
 	/* Set baud rate */
 	writew(quot & 0x3f, port->membase + UART011_FBRD);
 	writew(quot >> 6, port->membase + UART011_IBRD);
-- 
1.6.5.3

[PATCH] amba-pl011: support hardware flow control

[Rabin Vincent]

Enable/disable hardware flow control as requested by the termios.

Acked-by: Linus Walleij <linus.walleij@stericsson.com>
Signed-off-by: Rabin Vincent <rabin.vincent@stericsson.com>
---
 drivers/serial/amba-pl011.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/serial/amba-pl011.c b/drivers/serial/amba-pl011.c
index ef7adc8..6f37db4 100644
--- a/drivers/serial/amba-pl011.c
+++ b/drivers/serial/amba-pl011.c
@@ -532,6 +532,11 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
 	old_cr = readw(port->membase + UART011_CR);
 	writew(0, port->membase + UART011_CR);
 
+	if (termios->c_cflag & CRTSCTS)
+		old_cr |= UART011_CR_CTSEN | UART011_CR_RTSEN;
+	else
+		old_cr &= ~(UART011_CR_CTSEN | UART011_CR_RTSEN);
+
 	/* Set baud rate */
 	writew(quot & 0x3f, port->membase + UART011_FBRD);
 	writew(quot >> 6, port->membase + UART011_IBRD);
-- 
1.6.5.3

[PATCH] amba-pl011: support hardware flow control

[Russell King - ARM Linux]

On Mon, Feb 08, 2010 at 07:11:50PM +0530, Rabin Vincent wrote:
> Enable/disable hardware flow control as requested by the termios.

[PATCH] amba-pl011: support hardware flow control

[Rabin VINCENT]

On Mon, Feb 08, 2010 at 02:51:24PM +0100, Russell King - ARM Linux wrote:
> On Mon, Feb 08, 2010 at 07:11:50PM +0530, Rabin Vincent wrote:
> > Enable/disable hardware flow control as requested by the termios.
> 
> From what I remember, I didn't implement this because there were
> corner cases to be dealt with.  I don't remember what they were
> off hand though.

We haven't seen any problems with it thus far.  Nothing obvious strikes
me on rechecking the pl011 spec; it would be helpful if you could
elaborate.

Either way, this patch should be safe to apply since existing users of
the driver shouldn't be affected.

Rabin

[PATCH] amba-pl011: support hardware flow control

[Russell King - ARM Linux]

On Tue, Feb 09, 2010 at 08:00:56PM +0530, Rabin VINCENT wrote:
> On Mon, Feb 08, 2010 at 02:51:24PM +0100, Russell King - ARM Linux wrote:
> > On Mon, Feb 08, 2010 at 07:11:50PM +0530, Rabin Vincent wrote:
> > > Enable/disable hardware flow control as requested by the termios.
> > 
> > From what I remember, I didn't implement this because there were
> > corner cases to be dealt with.  I don't remember what they were
> > off hand though.
> 
> We haven't seen any problems with it thus far.  Nothing obvious strikes
> me on rechecking the pl011 spec; it would be helpful if you could
> elaborate.

Can't elaborate at the moment without re-reading the specs.

> Either way, this patch should be safe to apply since existing users of
> the driver shouldn't be affected.

Err, that really doesn't follow.  There are existing users, and existing
users will have CRTSCTS set by default.  Therefore, existing users are
going from having hardware flow control on the UART disabled to having
it enabled.

That's not "shouldn't be affected", that's a "this patch does change
the hardware behaviour for these users".

[PATCH] amba-pl011: support hardware flow control

[Russell King - ARM Linux]

On Tue, Feb 09, 2010 at 08:00:56PM +0530, Rabin VINCENT wrote:
> On Mon, Feb 08, 2010 at 02:51:24PM +0100, Russell King - ARM Linux wrote:
> > On Mon, Feb 08, 2010 at 07:11:50PM +0530, Rabin Vincent wrote:
> > > Enable/disable hardware flow control as requested by the termios.
> > 
> > From what I remember, I didn't implement this because there were
> > corner cases to be dealt with.  I don't remember what they were
> > off hand though.
> 
> We haven't seen any problems with it thus far.  Nothing obvious strikes
> me on rechecking the pl011 spec; it would be helpful if you could
> elaborate.

Right, the problem with RTS flow control is that if you enable it,
the hardware takes full control of the RTS signal.

This means that when the kernel's buffers fill up, the kernel calls
down to the serial core layer to throttle the input.  This then
calls into the set_mctrl function to de-assert RTS.  However, because
the hardware ignores the requested software state, the RTS signal
is not de-asserted, and the remote end continues sending data.

So, enabling hardware auto-RTS is bad news - you will lose data if
the application stops reading data.

Auto-CTS flow control looks safe.

[PATCH] amba-pl011: support hardware flow control

[Jamie Lokier]

Russell King - ARM Linux wrote:
> This means that when the kernel's buffers fill up, the kernel calls
> down to the serial core layer to throttle the input.  This then
> calls into the set_mctrl function to de-assert RTS.  However, because
> the hardware ignores the requested software state, the RTS signal
> is not de-asserted, and the remote end continues sending data.
> 
> So, enabling hardware auto-RTS is bad news - you will lose data if
> the application stops reading data.

Surely the driver can just stop reading from the UART when the
kernel's buffers are full and hardware-RTS is enabled.  Then the
hardware will deassert RTS itself.

The problem with relying on the kernel to deassert RTS is that it's
too slow when something disables interrupts for a few ms, and you get
lost data that way instead.

The ideal combination may be a bit of both:

    1. When kernel deasserts RTS, do that and disable hardware-RTS
       so it really is deasserted.

    2. When kernel asserts RTS with mctrl, enable hardware-RTS
       so what's output depends on the receive FIFO state.

That should solve both types of overrun.

-- Jamie

[PATCH] amba-pl011: support hardware flow control

[Russell King - ARM Linux]

On Wed, Feb 10, 2010 at 01:16:37AM +0000, Jamie Lokier wrote:
> Russell King - ARM Linux wrote:
> > This means that when the kernel's buffers fill up, the kernel calls
> > down to the serial core layer to throttle the input.  This then
> > calls into the set_mctrl function to de-assert RTS.  However, because
> > the hardware ignores the requested software state, the RTS signal
> > is not de-asserted, and the remote end continues sending data.
> > 
> > So, enabling hardware auto-RTS is bad news - you will lose data if
> > the application stops reading data.
> 
> Surely the driver can just stop reading from the UART when the
> kernel's buffers are full and hardware-RTS is enabled.  Then the
> hardware will deassert RTS itself.

I'll let you work out how to implement that.

[PATCH] amba-pl011: support hardware flow control

[Jamie Lokier]

Russell King - ARM Linux wrote:
> On Wed, Feb 10, 2010 at 01:16:37AM +0000, Jamie Lokier wrote:
> > Russell King - ARM Linux wrote:
> > > This means that when the kernel's buffers fill up, the kernel calls
> > > down to the serial core layer to throttle the input.  This then
> > > calls into the set_mctrl function to de-assert RTS.  However, because
> > > the hardware ignores the requested software state, the RTS signal
> > > is not de-asserted, and the remote end continues sending data.
> > > 
> > > So, enabling hardware auto-RTS is bad news - you will lose data if
> > > the application stops reading data.
> > 
> > Surely the driver can just stop reading from the UART when the
> > kernel's buffers are full and hardware-RTS is enabled.  Then the
> > hardware will deassert RTS itself.
> 
> I'll let you work out how to implement that.

You have a point.

What do you think of the other suggestion, switching between
software-RTS+deasserted and hardware-RTS in response to set_mctrl
calls from the generic serial layer - effectively hardware-RTS with
ability for kernel to force it deasserted?

-- Jamie

[PATCH] amba-pl011: support hardware flow control

[Rabin VINCENT]

On Wed, Feb 10, 2010 at 02:16:37AM +0100, Jamie Lokier wrote:
> Russell King - ARM Linux wrote:
> > This means that when the kernel's buffers fill up, the kernel calls
> > down to the serial core layer to throttle the input.  This then
> > calls into the set_mctrl function to de-assert RTS.  However, because
> > the hardware ignores the requested software state, the RTS signal
> > is not de-asserted, and the remote end continues sending data.
> > 
> > So, enabling hardware auto-RTS is bad news - you will lose data if
> > the application stops reading data.
> 
[..]
> The ideal combination may be a bit of both:
> 
>     1. When kernel deasserts RTS, do that and disable hardware-RTS
>        so it really is deasserted.
> 
>     2. When kernel asserts RTS with mctrl, enable hardware-RTS
>        so what's output depends on the receive FIFO state.

I've implemented this in the updated patch below.

[PATCH] amba-pl011: support hardware flow control

[Russell King - ARM Linux]

On Wed, Feb 10, 2010 at 07:15:21PM +0530, Rabin VINCENT wrote:
> I've implemented this in the updated patch below.

I think this is ok.  Has it been tested?

[PATCH] amba-pl011: support hardware flow control

[Rabin VINCENT]

On Wed, Feb 10, 2010 at 09:25:52PM +0100, Russell King - ARM Linux wrote:
> On Wed, Feb 10, 2010 at 07:15:21PM +0530, Rabin VINCENT wrote:
> > I've implemented this in the updated patch below.
> 
> I think this is ok.  Has it been tested?

Yes.  I was able to reproduce the described problem, and this version of
the patch fixes it.

Rabin

[PATCH] amba-pl011: support hardware flow control

[Linus Walleij]

[Russell] 

> On Wed, Feb 10, 2010 at 07:15:21PM +0530, Rabin VINCENT wrote:
> > I've implemented this in the updated patch below.
> 
> I think this is ok.  Has it been tested?

I have also regression tested on the U300 (which has a pure,
unmodified PL011 from ARM) and no obvious problems.
Busybox stty seem to say that -crtscts is on by default
so I believe it's in full use now?

I can do further stress tests, just tell me how...

Linus