Skip to contentSkip to conversation

Minimum set of omap serial patches to fix merge window breakage

PATCH New Russell King - ARM Linux 3 participants 15 messages Oct 16, 2012 TTY layer and serial drivers

lore
Share & tools
lore

Russell King - ARM Linux

author
#0 lore raw

cover Minimum set of omap serial patches to fix merge window breakage

Hi,

During the merge window, a series of patches from various people went in, allegedly fixing various problems with the OMAP serial driver.

Unfortunately, there was not a full understanding of the issues I brought up here back in April, and so the "fixes", while being individually correct, result in a worse situation with the driver than before.

Specifically, this patch:

commit 957ee7270d632245b43f6feb0e70d9a5e9ea6cf6

Author: Vikram Pandita <[email protected]>
Date:   Thu Sep 6 15:45:37 2012 +0300

serial: omap: fix software flow control

Software flow control register bits were not defined correctly.

Also clarify the IXON and IXOFF logic to reflect what userspace wants.

does what it says on the tin - it fixes the register definitions so that we do end up enabling the right two software flow control bits.

The down side is that there are other bugs in this area which have been exposed. For example:

1. the XON/XOFF registers aren't written to; their address is, but we will not be writing to the right registers because their access rules are not respected by the driver. These are by default zero.

This means that with hardware assisted software flow control enabled, the port will now transmit an 0x00 byte for XON and XOFF events.

2. the driver set_termios function is not called for changes in software flow control settings if not accompanied by some other change.

3. the there isn't actually a way for the hardware assisted flow control to be used other than by increasing interrupt latency to cause the receiver hardware FIFO to fill. This will cause 0x00 bytes to be transmitted.

There are two options to resolve this. The first one is to revert this patch to bring the driver back down to the pre-merge window state. The other is to apply this series of patches, which frankly I don't think is -rc material, even with the above regressions.

Given that the above regressions were caused by a lack of due care and correct process (I had declared to TI that I had investigated these issues back in April), I believe that the right answer is to revert at least commit 957ee7270d, which should re-hide these other bugs in the driver.

#1 lore raw

PATCH 1/11 SERIAL: core: use local variable uport in uart_set_termios()

This is to make the following change more clear.

Acked-by Alan Cox <[email protected]>
Signed-off-by Russell King <[email protected]>
---
 drivers/tty/serial/serial_core.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)
drivers/tty/serial/serial_core.c +9 -8
index 0fcfd98..bc2065d 100644
@@ -1210,6 +1210,7 @@ static void uart_set_termios(struct tty_struct *tty,
 						struct ktermios *old_termios)
 {
 	struct uart_state *state = tty->driver_data;
+	struct uart_port *uport = state->uart_port;
 	unsigned long flags;
 	unsigned int cflag = tty->termios.c_cflag;
 

@@ -1232,31 +1233,31 @@ static void uart_set_termios(struct tty_struct *tty,
 
 	/* Handle transition to B0 status */
 	if ((old_termios->c_cflag & CBAUD) && !(cflag & CBAUD))
-		uart_clear_mctrl(state->uart_port, TIOCM_RTS | TIOCM_DTR);
+		uart_clear_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
 	/* Handle transition away from B0 status */
 	else if (!(old_termios->c_cflag & CBAUD) && (cflag & CBAUD)) {
 		unsigned int mask = TIOCM_DTR;
 		if (!(cflag & CRTSCTS) ||
 		    !test_bit(TTY_THROTTLED, &tty->flags))
 			mask |= TIOCM_RTS;
-		uart_set_mctrl(state->uart_port, mask);
+		uart_set_mctrl(uport, mask);
 	}
 
 	/* Handle turning off CRTSCTS */
 	if ((old_termios->c_cflag & CRTSCTS) && !(cflag & CRTSCTS)) {
-		spin_lock_irqsave(&state->uart_port->lock, flags);
+		spin_lock_irqsave(&uport->lock, flags);
 		tty->hw_stopped = 0;
 		__uart_start(tty);
-		spin_unlock_irqrestore(&state->uart_port->lock, flags);
+		spin_unlock_irqrestore(&uport->lock, flags);
 	}
 	/* Handle turning on CRTSCTS */
 	else if (!(old_termios->c_cflag & CRTSCTS) && (cflag & CRTSCTS)) {
-		spin_lock_irqsave(&state->uart_port->lock, flags);
-		if (!(state->uart_port->ops->get_mctrl(state->uart_port) & TIOCM_CTS)) {
+		spin_lock_irqsave(&uport->lock, flags);
+		if (!(uport->ops->get_mctrl(uport) & TIOCM_CTS)) {
 			tty->hw_stopped = 1;
-			state->uart_port->ops->stop_tx(state->uart_port);
+			uport->ops->stop_tx(uport);
 		}
-		spin_unlock_irqrestore(&state->uart_port->lock, flags);
+		spin_unlock_irqrestore(&uport->lock, flags);
 	}
 }
Signature · 1 line
1.7.4.4
#2 lore raw

PATCH 2/11 SERIAL: core: add hardware assisted s/w flow control support

In reply to Russell King - ARM Linux

Ports which are capable of handling s/w flow control in hardware to know when the s/w flow control termios settings are changed. Add a flag to allow the low level serial drivers to indicate that they support this, and these changes should be propagated to them.

Acked-by Alan Cox <[email protected]>
Signed-off-by Russell King <[email protected]>
---
 drivers/tty/serial/serial_core.c |   16 ++++++++++++++--
 include/linux/serial_core.h      |    2 ++
 2 files changed, 16 insertions(+), 2 deletions(-)
drivers/tty/serial/serial_core.c +14 -2
index bc2065d..bd10bbd 100644
@@ -1213,7 +1213,19 @@ static void uart_set_termios(struct tty_struct *tty,
 	struct uart_port *uport = state->uart_port;
 	unsigned long flags;
 	unsigned int cflag = tty->termios.c_cflag;
+	unsigned int iflag_mask = IGNBRK|BRKINT|IGNPAR|PARMRK|INPCK;
+	bool sw_changed = false;
 
+	/*
+	 * Drivers doing software flow control also need to know
+	 * about changes to these input settings.
+	 */
+	if (uport->flags & UPF_SOFT_FLOW) {
+		iflag_mask |= IXANY|IXON|IXOFF;
+		sw_changed =
+		   tty->termios.c_cc[VSTART] != old_termios->c_cc[VSTART] ||
+		   tty->termios.c_cc[VSTOP] != old_termios->c_cc[VSTOP];
+	}
 
 	/*
 	 * These are the bits that are used to setup various

@@ -1221,11 +1233,11 @@ static void uart_set_termios(struct tty_struct *tty,
 	 * bits in c_cflag; c_[io]speed will always be set
 	 * appropriately by set_termios() in tty_ioctl.c
 	 */
-#define RELEVANT_IFLAG(iflag)	((iflag) & (IGNBRK|BRKINT|IGNPAR|PARMRK|INPCK))
 	if ((cflag ^ old_termios->c_cflag) == 0 &&
 	    tty->termios.c_ospeed == old_termios->c_ospeed &&
 	    tty->termios.c_ispeed == old_termios->c_ispeed &&
-	    RELEVANT_IFLAG(tty->termios.c_iflag ^ old_termios->c_iflag) == 0) {
+	    ((tty->termios.c_iflag ^ old_termios->c_iflag) & iflag_mask) == 0 &&
+	    !sw_changed) {
 		return;
 	}
include/linux/serial_core.h +2 -0
index 3c43022..0005138 100644
@@ -163,6 +163,8 @@ struct uart_port {
 #define UPF_BUGGY_UART		((__force upf_t) (1 << 14))
 #define UPF_NO_TXEN_TEST	((__force upf_t) (1 << 15))
 #define UPF_MAGIC_MULTIPLIER	((__force upf_t) (1 << 16))
+/* Port has hardware-assisted s/w flow control */
+#define UPF_SOFT_FLOW		((__force upf_t) (1 << 22))
 #define UPF_CONS_FLOW		((__force upf_t) (1 << 23))
 #define UPF_SHARE_IRQ		((__force upf_t) (1 << 24))
 #define UPF_EXAR_EFR		((__force upf_t) (1 << 25))
Signature · 1 line
1.7.4.4
#3 lore raw

PATCH 3/11 SERIAL: core: add hardware assisted h/w flow control support

In reply to Russell King - ARM Linux

Ports which are handling h/w flow control in hardware must not have their RTS state altered depending on the tty's hardware-stopped state. Avoid this additional logic when setting the termios state.

Acked-by Alan Cox <[email protected]>
Signed-off-by Russell King <[email protected]>
---
 drivers/tty/serial/serial_core.c |    7 +++++++
 include/linux/serial_core.h      |    2 ++
 2 files changed, 9 insertions(+), 0 deletions(-)
drivers/tty/serial/serial_core.c +7 -0
index bd10bbd..9d8796e 100644
@@ -1255,6 +1255,13 @@ static void uart_set_termios(struct tty_struct *tty,
 		uart_set_mctrl(uport, mask);
 	}
 
+	/*
+	 * If the port is doing h/w assisted flow control, do nothing.
+	 * We assume that tty->hw_stopped has never been set.
+	 */
+	if (uport->flags & UPF_HARD_FLOW)
+		return;
+
 	/* Handle turning off CRTSCTS */
 	if ((old_termios->c_cflag & CRTSCTS) && !(cflag & CRTSCTS)) {
 		spin_lock_irqsave(&uport->lock, flags);
include/linux/serial_core.h +2 -0
index 0005138..e2cda5d 100644
@@ -163,6 +163,8 @@ struct uart_port {
 #define UPF_BUGGY_UART		((__force upf_t) (1 << 14))
 #define UPF_NO_TXEN_TEST	((__force upf_t) (1 << 15))
 #define UPF_MAGIC_MULTIPLIER	((__force upf_t) (1 << 16))
+/* Port has hardware-assisted h/w flow control (iow, auto-RTS *not* auto-CTS) */
+#define UPF_HARD_FLOW		((__force upf_t) (1 << 21))
 /* Port has hardware-assisted s/w flow control */
 #define UPF_SOFT_FLOW		((__force upf_t) (1 << 22))
 #define UPF_CONS_FLOW		((__force upf_t) (1 << 23))
Signature · 1 line
1.7.4.4
#4 lore raw

PATCH 4/11 SERIAL: core: add throttle/unthrottle callbacks for hardware assisted flow control

In reply to Russell King - ARM Linux

Add two callbacks for hardware assisted flow control; we need to know when the tty layers want us to stop and restart due to their buffer levels.

Call a driver specific throttle/unthrottle function if and only if the driver indicates that it is using an enabled hardware assisted flow control method, otherwise fall back to the non-hardware assisted methods.

Acked-by Alan Cox <[email protected]>
Signed-off-by Russell King <[email protected]>
---
 drivers/tty/serial/serial_core.c |   31 +++++++++++++++++++++++++++----
 include/linux/serial_core.h      |    2 ++
 2 files changed, 29 insertions(+), 4 deletions(-)
drivers/tty/serial/serial_core.c +27 -4
index 9d8796e..098bb99 100644
@@ -610,27 +610,50 @@ static void uart_send_xchar(struct tty_struct *tty, char ch)
 static void uart_throttle(struct tty_struct *tty)
 {
 	struct uart_state *state = tty->driver_data;
+	struct uart_port *port = state->uart_port;
+	uint32_t mask = 0;
 
 	if (I_IXOFF(tty))
+		mask |= UPF_SOFT_FLOW;
+	if (tty->termios.c_cflag & CRTSCTS)
+		mask |= UPF_HARD_FLOW;
+
+	if (port->flags & mask) {
+		port->ops->throttle(port);
+		mask &= ~port->flags;
+	}
+
+	if (mask & UPF_SOFT_FLOW)
 		uart_send_xchar(tty, STOP_CHAR(tty));
 
-	if (tty->termios.c_cflag & CRTSCTS)
-		uart_clear_mctrl(state->uart_port, TIOCM_RTS);
+	if (mask & UPF_HARD_FLOW)
+		uart_clear_mctrl(port, TIOCM_RTS);
 }
 
 static void uart_unthrottle(struct tty_struct *tty)
 {
 	struct uart_state *state = tty->driver_data;
 	struct uart_port *port = state->uart_port;
+	uint32_t mask = 0;
 
-	if (I_IXOFF(tty)) {
+	if (I_IXOFF(tty))
+		mask |= UPF_SOFT_FLOW;
+	if (tty->termios.c_cflag & CRTSCTS)
+		mask |= UPF_HARD_FLOW;
+
+	if (port->flags & mask) {
+		port->ops->unthrottle(port);
+		mask &= ~port->flags;
+	}
+
+	if (mask & UPF_SOFT_FLOW) {
 		if (port->x_char)
 			port->x_char = 0;
 		else
 			uart_send_xchar(tty, START_CHAR(tty));
 	}
 
-	if (tty->termios.c_cflag & CRTSCTS)
+	if (mask & UPF_HARD_FLOW)
 		uart_set_mctrl(port, TIOCM_RTS);
 }
include/linux/serial_core.h +2 -0
index e2cda5d..c6690a2 100644
@@ -46,6 +46,8 @@ struct uart_ops {
 	unsigned int	(*get_mctrl)(struct uart_port *);
 	void		(*stop_tx)(struct uart_port *);
 	void		(*start_tx)(struct uart_port *);
+	void		(*throttle)(struct uart_port *);
+	void		(*unthrottle)(struct uart_port *);
 	void		(*send_xchar)(struct uart_port *, char ch);
 	void		(*stop_rx)(struct uart_port *);
 	void		(*enable_ms)(struct uart_port *);
Signature · 1 line
1.7.4.4
#5 lore raw

PATCH 5/11 SERIAL: omap: allow hardware assisted rts/cts modes to be disabled

In reply to Russell King - ARM Linux

There is nothing which clears the auto RTS/CTS bits, so once hardware flow control gets enabled, there's no possibility to disable it. So, clear these bits when CRTSCTS is cleared.

Signed-off-by Russell King <[email protected]>
---
 drivers/tty/serial/omap-serial.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
drivers/tty/serial/omap-serial.c +7 -0
index 6ede6fd..c55af63 100644
@@ -921,6 +921,13 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
 		serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
 		serial_out(up, UART_MCR, up->mcr | UART_MCR_RTS);
 		serial_out(up, UART_LCR, cval);
+	} else {
+		/* Disable AUTORTS and AUTOCTS */
+		up->efr &= ~(UART_EFR_CTS | UART_EFR_RTS);
+
+		serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
+		serial_out(up, UART_EFR, up->efr);
+		serial_out(up, UART_LCR, cval);
 	}
 
 	serial_omap_set_mctrl(&up->port, up->port.mctrl);
Signature · 1 line
1.7.4.4
#6 lore raw

PATCH 6/11 SERIAL: omap: allow hardware assisted IXANY mode to be disabled

In reply to Russell King - ARM Linux

Nothing was clearing the UART_MCR_XONANY bit, so once the ixany mode gets set, there's no possibility to disable it. Clear this bit when IXANY mode is cleared.

Signed-off-by Russell King <[email protected]>
---
 drivers/tty/serial/omap-serial.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/tty/serial/omap-serial.c +2 -0
index c55af63..7c313c2 100644
@@ -698,6 +698,8 @@ serial_omap_configure_xonxoff
 	 */
 	if (termios->c_iflag & IXANY)
 		up->mcr |= UART_MCR_XONANY;
+	else
+		up->mcr &= ~UART_MCR_XONANY;
 
 	serial_out(up, UART_MCR, up->mcr | UART_MCR_TCRTLR);
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
Signature · 1 line
1.7.4.4
#7 lore raw

PATCH 7/11 SERIAL: omap: remove setting of EFR SCD bit

In reply to Russell King - ARM Linux

The SCD (special character detect) bit enables comparisons with XOFF2, which we do not program. As the XOFF2 character remains unprogrammed, there's little point enabling this feature along with its associated interrupt. Remove this, and ensure that the SCD bit is cleared.

Signed-off-by Russell King <[email protected]>
---
 drivers/tty/serial/omap-serial.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)
drivers/tty/serial/omap-serial.c +2 -6
index 7c313c2..71db4e6 100644
@@ -704,13 +704,8 @@ serial_omap_configure_xonxoff
 	serial_out(up, UART_MCR, up->mcr | UART_MCR_TCRTLR);
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
 	serial_out(up, UART_TI752_TCR, OMAP_UART_TCR_TRIG);
-	/* Enable special char function UARTi.EFR_REG[5] and
-	 * load the new software flow control mode IXON or IXOFF
-	 * and restore the UARTi.EFR_REG[4] ENHANCED_EN value.
-	 */
-	serial_out(up, UART_EFR, up->efr | UART_EFR_SCD);
+	serial_out(up, UART_EFR, up->efr);
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
-
 	serial_out(up, UART_MCR, up->mcr & ~UART_MCR_TCRTLR);
 	serial_out(up, UART_LCR, up->lcr);
 }

@@ -843,6 +838,7 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
 
 	up->efr = serial_in(up, UART_EFR);
+	up->efr &= ~UART_EFR_SCD;
 	serial_out(up, UART_EFR, up->efr | UART_EFR_ECB);
 
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
Signature · 1 line
1.7.4.4
#8 lore raw

PATCH 8/11 SERIAL: omap: no need to re-read EFR

In reply to Russell King - ARM Linux

There's no need to re-read EFR after we've recently written it; the register is a configuration register which doesn't change its value without us writing to it. The last value which was written to this register was up->efr.

Removing this re-reading avoids the possibility that we end up with up->efr having unintended bits set, which should only be temporarily set when accessing the enhanced features.

Signed-off-by Russell King <[email protected]>
---
 drivers/tty/serial/omap-serial.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)
drivers/tty/serial/omap-serial.c +0 -2
index 71db4e6..c8a0425 100644
@@ -873,8 +873,6 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
 		serial_out(up, UART_OMAP_MDR1, up->mdr1);
 
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
-
-	up->efr = serial_in(up, UART_EFR);
 	serial_out(up, UART_EFR, up->efr | UART_EFR_ECB);
 
 	serial_out(up, UART_LCR, 0);
Signature · 1 line
1.7.4.4
#9 lore raw

PATCH 9/11 SERIAL: omap: fix set_mctrl() breakage

In reply to Russell King - ARM Linux

c538d20c7f (and maybe previous commits) broke set_mctrl() by making it only capable of setting bits in the MCR register. This prevents software controlled flow control and modem control line manipulation via TIOCMSET/TIOCMBIC from working correctly.

Signed-off-by Russell King <[email protected]>
---
 drivers/tty/serial/omap-serial.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)
drivers/tty/serial/omap-serial.c +5 -3
index c8a0425..537829f 100644
@@ -504,7 +504,7 @@ static unsigned int serial_omap_get_mctrl(struct uart_port *port)
 static void serial_omap_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
 	struct uart_omap_port *up = to_uart_omap_port(port);
-	unsigned char mcr = 0;
+	unsigned char mcr = 0, old_mcr;
 
 	dev_dbg(up->port.dev, "serial_omap_set_mctrl+%d\n", up->port.line);
 	if (mctrl & TIOCM_RTS)

@@ -519,8 +519,10 @@ static void serial_omap_set_mctrl(struct uart_port *port, unsigned int mctrl)
 		mcr |= UART_MCR_LOOP;
 
 	pm_runtime_get_sync(up->dev);
-	up->mcr = serial_in(up, UART_MCR);
-	up->mcr |= mcr;
+	old_mcr = serial_in(up, UART_MCR);
+	old_mcr &= ~(UART_MCR_LOOP | UART_MCR_OUT2 | UART_MCR_OUT1 |
+		     UART_MCR_DTR | UART_MCR_RTS);
+	up->mcr = old_mcr | mcr;
 	serial_out(up, UART_MCR, up->mcr);
 	pm_runtime_mark_last_busy(up->dev);
 	pm_runtime_put_autosuspend(up->dev);
Signature · 1 line
1.7.4.4
#10 lore raw

PATCH 10/11 SERIAL: omap: fix MCR TCRTLR bit handling

In reply to Russell King - ARM Linux

The MCR TCRTLR bit can only be changed when ECB is set in the EFR. Unfortunately, several places were trying to alter this bit while ECB was clear:

- serial_omap_configure_xonxoff() was attempting to clear the bit after explicitly clearing the ECB bit.
- serial_omap_set_termios() was trying the same trick after setting the SCR, and when trying to change the TCR register when hardware flow control was enabled.

Fix this by ensuring that we always have ECB set whenever the TCRTLR bit is changed.

Moreover, we start out by reading the EFR and MCR registers, which may have indeterminent bit settings for the ECB and TCRTLR bits. Ensure that these bits always start off in a known state.

Signed-off-by Russell King <[email protected]>
---
 drivers/tty/serial/omap-serial.c |   32 ++++++++++++++++++--------------
 1 files changed, 18 insertions(+), 14 deletions(-)
drivers/tty/serial/omap-serial.c +18 -14
index 537829f..7cc151c 100644
@@ -706,9 +706,10 @@ serial_omap_configure_xonxoff
 	serial_out(up, UART_MCR, up->mcr | UART_MCR_TCRTLR);
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
 	serial_out(up, UART_TI752_TCR, OMAP_UART_TCR_TRIG);
-	serial_out(up, UART_EFR, up->efr);
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
 	serial_out(up, UART_MCR, up->mcr & ~UART_MCR_TCRTLR);
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
+	serial_out(up, UART_EFR, up->efr);
 	serial_out(up, UART_LCR, up->lcr);
 }
 

@@ -729,7 +730,6 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
 {
 	struct uart_omap_port *up = to_uart_omap_port(port);
 	unsigned char cval = 0;
-	unsigned char efr = 0;
 	unsigned long flags = 0;
 	unsigned int baud, quot;
 

@@ -839,12 +839,12 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
 
-	up->efr = serial_in(up, UART_EFR);
+	up->efr = serial_in(up, UART_EFR) & ~UART_EFR_ECB;
 	up->efr &= ~UART_EFR_SCD;
 	serial_out(up, UART_EFR, up->efr | UART_EFR_ECB);
 
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
-	up->mcr = serial_in(up, UART_MCR);
+	up->mcr = serial_in(up, UART_MCR) & ~UART_MCR_TCRTLR;
 	serial_out(up, UART_MCR, up->mcr | UART_MCR_TCRTLR);
 	/* FIFO ENABLE, DMA MODE */
 

@@ -863,9 +863,12 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	serial_out(up, UART_OMAP_SCR, up->scr);
 
-	serial_out(up, UART_EFR, up->efr);
+	/* Reset UART_MCR_TCRTLR: this must be done with the EFR_ECB bit set */
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
 	serial_out(up, UART_MCR, up->mcr);
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
+	serial_out(up, UART_EFR, up->efr);
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
 
 	/* Protocol, Baud Rate, and Interrupt Settings */
 

@@ -904,20 +907,21 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
 	/* Hardware Flow Control Configuration */
 
 	if (termios->c_cflag & CRTSCTS) {
-		efr |= (UART_EFR_CTS | UART_EFR_RTS);
-		serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
-
-		up->mcr = serial_in(up, UART_MCR);
-		serial_out(up, UART_MCR, up->mcr | UART_MCR_TCRTLR);
-
+		/* Enable access to TCR/TLR */
 		serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
-		up->efr = serial_in(up, UART_EFR);
 		serial_out(up, UART_EFR, up->efr | UART_EFR_ECB);
+		serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
+		serial_out(up, UART_MCR, up->mcr | UART_MCR_TCRTLR);
 
 		serial_out(up, UART_TI752_TCR, OMAP_UART_TCR_TRIG);
-		serial_out(up, UART_EFR, efr); /* Enable AUTORTS and AUTOCTS */
-		serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
+
+		/* Enable AUTORTS and AUTOCTS */
+		up->efr |= UART_EFR_CTS | UART_EFR_RTS;
+
+		/* Disable access to TCR/TLR */
 		serial_out(up, UART_MCR, up->mcr | UART_MCR_RTS);
+		serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
+		serial_out(up, UART_EFR, up->efr);
 		serial_out(up, UART_LCR, cval);
 	} else {
 		/* Disable AUTORTS and AUTOCTS */
Signature · 1 line
1.7.4.4
author
#11 lore raw

PATCH 10/11 SERIAL: omap: fix MCR TCRTLR bit handling

Quoted 10 lines · Russell King

The MCR TCRTLR bit can only be changed when ECB is set in the EFR. Unfortunately, several places were trying to alter this bit while ECB was clear:

- serial_omap_configure_xonxoff() was attempting to clear the bit after explicitly clearing the ECB bit.
- serial_omap_set_termios() was trying the same trick after setting the SCR, and when trying to change the TCR register when hardware flow control was enabled.

Quoted 6 lines

Fix this by ensuring that we always have ECB set whenever the TCRTLR bit is changed.

Moreover, we start out by reading the EFR and MCR registers, which may have indeterminent bit settings for the ECB and TCRTLR bits. Ensure that these bits always start off in a known state.

Note - this patch unfortunately exposes some of the bugs in their full glory as we now write to the correct registers and actually end up programming the hardware correctly.

I've reduced the window as far as possible by putting this one next to patch 11 which fixes it, but no amount of reordering can hide some kind of breakage along the way.

Quoted 97 lines
Signed-off-by Russell King <[email protected]>
---
 drivers/tty/serial/omap-serial.c |   32 ++++++++++++++++++--------------
 1 files changed, 18 insertions(+), 14 deletions(-)
drivers/tty/serial/omap-serial.c +18 -14
index 537829f..7cc151c 100644
@@ -706,9 +706,10 @@ serial_omap_configure_xonxoff
 	serial_out(up, UART_MCR, up->mcr | UART_MCR_TCRTLR);
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
 	serial_out(up, UART_TI752_TCR, OMAP_UART_TCR_TRIG);
-	serial_out(up, UART_EFR, up->efr);
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
 	serial_out(up, UART_MCR, up->mcr & ~UART_MCR_TCRTLR);
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
+	serial_out(up, UART_EFR, up->efr);
 	serial_out(up, UART_LCR, up->lcr);
 }
 

@@ -729,7 +730,6 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
 {
 	struct uart_omap_port *up = to_uart_omap_port(port);
 	unsigned char cval = 0;
-	unsigned char efr = 0;
 	unsigned long flags = 0;
 	unsigned int baud, quot;
 

@@ -839,12 +839,12 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
 
-	up->efr = serial_in(up, UART_EFR);
+	up->efr = serial_in(up, UART_EFR) & ~UART_EFR_ECB;
 	up->efr &= ~UART_EFR_SCD;
 	serial_out(up, UART_EFR, up->efr | UART_EFR_ECB);
 
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
-	up->mcr = serial_in(up, UART_MCR);
+	up->mcr = serial_in(up, UART_MCR) & ~UART_MCR_TCRTLR;
 	serial_out(up, UART_MCR, up->mcr | UART_MCR_TCRTLR);
 	/* FIFO ENABLE, DMA MODE */
 

@@ -863,9 +863,12 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	serial_out(up, UART_OMAP_SCR, up->scr);
 
-	serial_out(up, UART_EFR, up->efr);
+	/* Reset UART_MCR_TCRTLR: this must be done with the EFR_ECB bit set */
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
 	serial_out(up, UART_MCR, up->mcr);
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
+	serial_out(up, UART_EFR, up->efr);
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
 
 	/* Protocol, Baud Rate, and Interrupt Settings */
 

@@ -904,20 +907,21 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
 	/* Hardware Flow Control Configuration */
 
 	if (termios->c_cflag & CRTSCTS) {
-		efr |= (UART_EFR_CTS | UART_EFR_RTS);
-		serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
-
-		up->mcr = serial_in(up, UART_MCR);
-		serial_out(up, UART_MCR, up->mcr | UART_MCR_TCRTLR);
-
+		/* Enable access to TCR/TLR */
 		serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
-		up->efr = serial_in(up, UART_EFR);
 		serial_out(up, UART_EFR, up->efr | UART_EFR_ECB);
+		serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
+		serial_out(up, UART_MCR, up->mcr | UART_MCR_TCRTLR);
 
 		serial_out(up, UART_TI752_TCR, OMAP_UART_TCR_TRIG);
-		serial_out(up, UART_EFR, efr); /* Enable AUTORTS and AUTOCTS */
-		serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
+
+		/* Enable AUTORTS and AUTOCTS */
+		up->efr |= UART_EFR_CTS | UART_EFR_RTS;
+
+		/* Disable access to TCR/TLR */
 		serial_out(up, UART_MCR, up->mcr | UART_MCR_RTS);
+		serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
+		serial_out(up, UART_EFR, up->efr);
 		serial_out(up, UART_LCR, cval);
 	} else {
 		/* Disable AUTORTS and AUTOCTS */
Signature · 7 lines
1.7.4.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#12 lore raw

PATCH 11/11 SERIAL: omap: fix hardware assisted flow control

In reply to Russell King - ARM Linux

When the UART device has hardware flow control enabled, it ignores the MCR RTS bit in the MCR register, and keeps RTS asserted as long as we continue to read characters from the UART receiver FIFO. This means that when the TTY buffers become full, the UART doesn't tell the remote end to stop sending, which causes the TTY layer to start dropping characters.

A similar problem exists with software flow control. We need the FIFO register to fill when software flow control is enabled to provoke the UART to send the XOFF character.

Fix this by implementing the throttle/unthrottle callbacks, and use these to disable receiver interrupts. This in turn means that the UART FIFO will fill, which will then cause the UART's hardware to deassert the RTS signal and/or send the XOFF character, stopping the remote end.

Signed-off-by Russell King <[email protected]>
---
 drivers/tty/serial/omap-serial.c |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)
drivers/tty/serial/omap-serial.c +31 -0
index 7cc151c..aabc94d 100644
@@ -303,6 +303,34 @@ static void serial_omap_start_tx(struct uart_port *port)
 	pm_runtime_put_autosuspend(up->dev);
 }
 
+static void serial_omap_throttle(struct uart_port *port)
+{
+	struct uart_omap_port *up = to_uart_omap_port(port);
+	unsigned long flags;
+
+	pm_runtime_get_sync(up->dev);
+	spin_lock_irqsave(&up->port.lock, flags);
+	up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
+	serial_out(up, UART_IER, up->ier);
+	spin_unlock_irqrestore(&up->port.lock, flags);
+	pm_runtime_mark_last_busy(up->dev);
+	pm_runtime_put_autosuspend(up->dev);
+}
+
+static void serial_omap_unthrottle(struct uart_port *port)
+{
+	struct uart_omap_port *up = to_uart_omap_port(port);
+	unsigned long flags;
+
+	pm_runtime_get_sync(up->dev);
+	spin_lock_irqsave(&up->port.lock, flags);
+	up->ier |= UART_IER_RLSI | UART_IER_RDI;
+	serial_out(up, UART_IER, up->ier);
+	spin_unlock_irqrestore(&up->port.lock, flags);
+	pm_runtime_mark_last_busy(up->dev);
+	pm_runtime_put_autosuspend(up->dev);
+}
+
 static unsigned int check_modem_status(struct uart_omap_port *up)
 {
 	unsigned int status;

@@ -1000,6 +1028,7 @@ static void serial_omap_config_port(struct uart_port *port, int flags)
 	dev_dbg(up->port.dev, "serial_omap_config_port+%d\n",
 							up->port.line);
 	up->port.type = PORT_OMAP;
+	up->port.flags |= UPF_SOFT_FLOW | UPF_HARD_FLOW;
 }
 
 static int

@@ -1203,6 +1232,8 @@ static struct uart_ops serial_omap_pops = {
 	.get_mctrl	= serial_omap_get_mctrl,
 	.stop_tx	= serial_omap_stop_tx,
 	.start_tx	= serial_omap_start_tx,
+	.throttle	= serial_omap_throttle,
+	.unthrottle	= serial_omap_unthrottle,
 	.stop_rx	= serial_omap_stop_rx,
 	.enable_ms	= serial_omap_enable_ms,
 	.break_ctl	= serial_omap_break_ctl,
Signature · 1 line
1.7.4.4
author
#13 lore raw

cover Minimum set of omap serial patches to fix merge window breakage

In reply to Russell King - ARM Linux
Quoted 45 lines · Russell King - ARM Linux

During the merge window, a series of patches from various people went in, allegedly fixing various problems with the OMAP serial driver.

Unfortunately, there was not a full understanding of the issues I brought up here back in April, and so the "fixes", while being individually correct, result in a worse situation with the driver than before.

Specifically, this patch:

commit 957ee7270d632245b43f6feb0e70d9a5e9ea6cf6

Author: Vikram Pandita <[email protected]>
Date:   Thu Sep 6 15:45:37 2012 +0300

serial: omap: fix software flow control

Software flow control register bits were not defined correctly.

Also clarify the IXON and IXOFF logic to reflect what userspace wants.

does what it says on the tin - it fixes the register definitions so that we do end up enabling the right two software flow control bits.

The down side is that there are other bugs in this area which have been exposed. For example:

1. the XON/XOFF registers aren't written to; their address is, but we will not be writing to the right registers because their access rules are not respected by the driver. These are by default zero.

This means that with hardware assisted software flow control enabled, the port will now transmit an 0x00 byte for XON and XOFF events.

2. the driver set_termios function is not called for changes in software flow control settings if not accompanied by some other change.

3. the there isn't actually a way for the hardware assisted flow control to be used other than by increasing interrupt latency to cause the receiver hardware FIFO to fill. This will cause 0x00 bytes to be transmitted.

There are two options to resolve this. The first one is to revert this patch to bring the driver back down to the pre-merge window state. The other is to apply this series of patches, which frankly I don't think is -rc material, even with the above regressions.

Quoted 5 lines

Given that the above regressions were caused by a lack of due care and correct process (I had declared to TI that I had investigated these issues back in April), I believe that the right answer is to revert at least commit 957ee7270d, which should re-hide these other bugs in the driver.

Note: these patches are not as well validated as my previous set; I haven't even build-tested just this set, but only my full set. YMMV.

#14 lore raw

cover Minimum set of omap serial patches to fix merge window breakage

In reply to Russell King - ARM Linux
Quoted 5 lines · Russell King - ARM Linux

There are two options to resolve this. The first one is to revert this patch to bring the driver back down to the pre-merge window state. The other is to apply this series of patches, which frankly I don't think is -rc material, even with the above regressions.

Yes reverting it is the way to go as we discussed in some off-list emails. I think everybody agrees that this is too intrusive for the -rc series :)

Regards,

Tony