[PATCH LINUX v3 00/10] tty: xuartps: Fix lock ups

[PATCH LINUX v3 00/10] tty: xuartps: Fix lock ups

[Soren Brinkmann]

Hi,

here is v3 of this series. It's largely the same as before, but I
adjusted 'tty: xuartps: Don't consider circular buffer when enabling
transmitter' according to Peter's suggestions.

I also spent some time trying to get Peter's test for flow control and
xchar running. The xchar thing fails and will need some more work, but I
think in general it should be possible to get it to work.
The flow control test passes:

root@Xilinx-ZC1751-DC1:~# ./flow /dev/ttyPS0
Test flow control on /dev/ttyPS0
begin test1
patterns sent: 223  recvd: 208
read distribution: 1   = 0
                   2+  = 0
                   4+  = 0
                   8+  = 0
                  16+  = 0
                  32+  = 643
                  64+  = 0
                 128+  = 0
                 256+  = 0
                 512+  = 0
PASSED

root@Xilinx-ZC1751-DC1:~# ./flow /dev/ttyPS1
Test flow control on /dev/ttyPS1
begin test1
patterns sent: 223  recvd: 208
read distribution: 1   = 0
                   2+  = 0
                   4+  = 0
                   8+  = 0
                  16+  = 0
                  32+  = 643
                  64+  = 0
                 128+  = 0
                 256+  = 0
                 512+  = 0
PASSED

	Sören

Sören Brinkmann (10):
  tty: xuartps: Beautify read-modify writes
  tty: xuartps: Use spinlock to serialize HW access
  tty: xuartps: Don't consider circular buffer when enabling transmitter
  tty: xuartps: Clear interrupt status register in shutdown
  tty: xuartps: Improve startup function
  tty: xuartps: Keep lock for whole ISR
  tty: xuartps: Acquire port lock for shutdown
  tty: xuartps: Move RX path into helper function
  tty: xuartps: Only handle RX IRQs when RX is enabled
  tty: xuartps: Cleanup: Reformat if-else

 drivers/tty/serial/xilinx_uartps.c | 246 +++++++++++++++++++++----------------
 1 file changed, 137 insertions(+), 109 deletions(-)

-- 
2.6.3.3.g9bb996a

[PATCH LINUX v3 01/10] tty: xuartps: Beautify read-modify writes

[Soren Brinkmann]

Non-functional, formatting changes to ease reading the code.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 009e0dbc12d2..50d4082d2354 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -515,12 +515,14 @@ static void cdns_uart_start_tx(struct uart_port *port)
 	if (uart_circ_empty(&port->state->xmit) || uart_tx_stopped(port))
 		return;
 
-	status = readl(port->membase + CDNS_UART_CR_OFFSET);
-	/* Set the TX enable bit and clear the TX disable bit to enable the
+	/*
+	 * Set the TX enable bit and clear the TX disable bit to enable the
 	 * transmitter.
 	 */
-	writel((status & ~CDNS_UART_CR_TX_DIS) | CDNS_UART_CR_TX_EN,
-			port->membase + CDNS_UART_CR_OFFSET);
+	status = readl(port->membase + CDNS_UART_CR_OFFSET);
+	status &= ~CDNS_UART_CR_TX_DIS;
+	status |= CDNS_UART_CR_TX_EN;
+	writel(status, port->membase + CDNS_UART_CR_OFFSET);
 
 	while (numbytes-- && ((readl(port->membase + CDNS_UART_SR_OFFSET) &
 				CDNS_UART_SR_TXFULL)) != CDNS_UART_SR_TXFULL) {
@@ -1123,8 +1125,9 @@ static void cdns_uart_console_write(struct console *co, const char *s,
 	 * clear the TX disable bit to enable the transmitter.
 	 */
 	ctrl = readl(port->membase + CDNS_UART_CR_OFFSET);
-	writel((ctrl & ~CDNS_UART_CR_TX_DIS) | CDNS_UART_CR_TX_EN,
-			port->membase + CDNS_UART_CR_OFFSET);
+	ctrl &= ~CDNS_UART_CR_TX_DIS;
+	ctrl |= CDNS_UART_CR_TX_EN;
+	writel(ctrl, port->membase + CDNS_UART_CR_OFFSET);
 
 	uart_console_write(port, s, count, cdns_uart_console_putchar);
 	cdns_uart_console_wait_tx(port);
-- 
2.6.3.3.g9bb996a

Re: [PATCH LINUX v3 01/10] tty: xuartps: Beautify read-modify writes

[Peter Hurley]

On 11/21/2015 09:59 PM, Soren Brinkmann wrote:
> Non-functional, formatting changes to ease reading the code.

Reviewed-by: Peter Hurley <peter@hurleysoftware.com>

Re: [PATCH LINUX v3 01/10] tty: xuartps: Beautify read-modify writes

[Moritz Fischer]

On Sat, Dec 5, 2015 at 9:04 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 11/21/2015 09:59 PM, Soren Brinkmann wrote:
>> Non-functional, formatting changes to ease reading the code.
>
> Reviewed-by: Peter Hurley <peter@hurleysoftware.com>
Reviewed-by: Moritz Fischer <moritz.fischer@ettus.com

[PATCH LINUX v3 02/10] tty: xuartps: Use spinlock to serialize HW access

[Soren Brinkmann]

Instead of disabling the IRQ, use the spin lock to serialize accesses to
the HW. This protects the driver from interference of non-IRQ callbacks
with each other and makes the driver more consistent in its
serialization method.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 50d4082d2354..2c98c357d9a0 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -945,12 +945,10 @@ static void cdns_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
 #ifdef CONFIG_CONSOLE_POLL
 static int cdns_uart_poll_get_char(struct uart_port *port)
 {
-	u32 imr;
 	int c;
+	unsigned long flags;
 
-	/* Disable all interrupts */
-	imr = readl(port->membase + CDNS_UART_IMR_OFFSET);
-	writel(imr, port->membase + CDNS_UART_IDR_OFFSET);
+	spin_lock_irqsave(&port->lock, flags);
 
 	/* Check if FIFO is empty */
 	if (readl(port->membase + CDNS_UART_SR_OFFSET) & CDNS_UART_SR_RXEMPTY)
@@ -959,19 +957,16 @@ static int cdns_uart_poll_get_char(struct uart_port *port)
 		c = (unsigned char) readl(
 					port->membase + CDNS_UART_FIFO_OFFSET);
 
-	/* Enable interrupts */
-	writel(imr, port->membase + CDNS_UART_IER_OFFSET);
+	spin_unlock_irqrestore(&port->lock, flags);
 
 	return c;
 }
 
 static void cdns_uart_poll_put_char(struct uart_port *port, unsigned char c)
 {
-	u32 imr;
+	unsigned long flags;
 
-	/* Disable all interrupts */
-	imr = readl(port->membase + CDNS_UART_IMR_OFFSET);
-	writel(imr, port->membase + CDNS_UART_IDR_OFFSET);
+	spin_lock_irqsave(&port->lock, flags);
 
 	/* Wait until FIFO is empty */
 	while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
@@ -986,8 +981,7 @@ static void cdns_uart_poll_put_char(struct uart_port *port, unsigned char c)
 				CDNS_UART_SR_TXEMPTY))
 		cpu_relax();
 
-	/* Enable interrupts */
-	writel(imr, port->membase + CDNS_UART_IER_OFFSET);
+	spin_unlock_irqrestore(&port->lock, flags);
 
 	return;
 }
-- 
2.6.3.3.g9bb996a

Re: [PATCH LINUX v3 02/10] tty: xuartps: Use spinlock to serialize HW access

[Peter Hurley]

On 11/21/2015 09:59 PM, Soren Brinkmann wrote:
> Instead of disabling the IRQ, use the spin lock to serialize accesses to
> the HW. This protects the driver from interference of non-IRQ callbacks
> with each other and makes the driver more consistent in its
> serialization method.

Reviewed-by: Peter Hurley <peter@hurleysoftware.com>

[PATCH LINUX v3 03/10] tty: xuartps: Don't consider circular buffer when enabling transmitter

[Soren Brinkmann]

Restarting the transmitter even if the circ buffer is empty may be
necessary to push out remaining data when the port is restarted after
being stopped.

Cc: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
v3:
 - changed this patch to not always enable the transmitter, but keep the
   check for uart_tx_stopped() in place, as suggested by Peter, whose
   explanation I also stole for the new commit message (thx)
---
 drivers/tty/serial/xilinx_uartps.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 2c98c357d9a0..6a7cd4e057ae 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -512,7 +512,7 @@ static void cdns_uart_start_tx(struct uart_port *port)
 {
 	unsigned int status, numbytes = port->fifosize;
 
-	if (uart_circ_empty(&port->state->xmit) || uart_tx_stopped(port))
+	if (uart_tx_stopped(port))
 		return;
 
 	/*
@@ -524,6 +524,9 @@ static void cdns_uart_start_tx(struct uart_port *port)
 	status |= CDNS_UART_CR_TX_EN;
 	writel(status, port->membase + CDNS_UART_CR_OFFSET);
 
+	if (uart_circ_empty(&port->state->xmit))
+		return;
+
 	while (numbytes-- && ((readl(port->membase + CDNS_UART_SR_OFFSET) &
 				CDNS_UART_SR_TXFULL)) != CDNS_UART_SR_TXFULL) {
 		/* Break if no more data available in the UART buffer */
-- 
2.6.3.3.g9bb996a

Re: [PATCH LINUX v3 03/10] tty: xuartps: Don't consider circular buffer when enabling transmitter

[Peter Hurley]

On 11/21/2015 09:59 PM, Soren Brinkmann wrote:
> Restarting the transmitter even if the circ buffer is empty may be
> necessary to push out remaining data when the port is restarted after
> being stopped.

Reviewed-by: Peter Hurley <peter@hurleysoftware.com>

[PATCH LINUX v3 04/10] tty: xuartps: Clear interrupt status register in shutdown

[Soren Brinkmann]

When shutting down the UART, clear the interrupt status register.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 6a7cd4e057ae..ef114d7a0623 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -828,6 +828,7 @@ static void cdns_uart_shutdown(struct uart_port *port)
 	/* Disable interrupts */
 	status = readl(port->membase + CDNS_UART_IMR_OFFSET);
 	writel(status, port->membase + CDNS_UART_IDR_OFFSET);
+	writel(0xffffffff, port->membase + CDNS_UART_ISR_OFFSET);
 
 	/* Disable the TX and RX */
 	writel(CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS,
-- 
2.6.3.3.g9bb996a

Re: [PATCH LINUX v3 04/10] tty: xuartps: Clear interrupt status register in shutdown

[Peter Hurley]

On 11/21/2015 09:59 PM, Soren Brinkmann wrote:
> When shutting down the UART, clear the interrupt status register.

This changelog could have expanded on the way the interrupt status
register worked so I didn't have to look this up myself.

Reviewed-by: Peter Hurley <peter@hurleysoftware.com>

Re: [PATCH LINUX v3 04/10] tty: xuartps: Clear interrupt status register in shutdown

[Moritz Fischer]

On Sat, Dec 5, 2015 at 9:19 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 11/21/2015 09:59 PM, Soren Brinkmann wrote:
>> When shutting down the UART, clear the interrupt status register.
>
> This changelog could have expanded on the way the interrupt status
> register worked so I didn't have to look this up myself.
>
> Reviewed-by: Peter Hurley <peter@hurleysoftware.com>
Reviewed-by: Moritz Fischer <moritz.fischer@ettus.com>

[PATCH LINUX v3 05/10] tty: xuartps: Improve startup function

[Soren Brinkmann]

The startup function is supposed to initialize the UART for receiving.
Hence, don't enable the TX part. Also, protect HW accesses with the port
lock.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index ef114d7a0623..6ffd3bbe3e18 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -758,6 +758,7 @@ static void cdns_uart_set_termios(struct uart_port *port,
  */
 static int cdns_uart_startup(struct uart_port *port)
 {
+	unsigned long flags;
 	unsigned int retval = 0, status = 0;
 
 	retval = request_irq(port->irq, cdns_uart_isr, 0, CDNS_UART_NAME,
@@ -765,6 +766,8 @@ static int cdns_uart_startup(struct uart_port *port)
 	if (retval)
 		return retval;
 
+	spin_lock_irqsave(&port->lock, flags);
+
 	/* Disable the TX and RX */
 	writel(CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS,
 			port->membase + CDNS_UART_CR_OFFSET);
@@ -775,15 +778,14 @@ static int cdns_uart_startup(struct uart_port *port)
 	writel(CDNS_UART_CR_TXRST | CDNS_UART_CR_RXRST,
 			port->membase + CDNS_UART_CR_OFFSET);
 
-	status = readl(port->membase + CDNS_UART_CR_OFFSET);
-
-	/* Clear the RX disable and TX disable bits and then set the TX enable
-	 * bit and RX enable bit to enable the transmitter and receiver.
+	/*
+	 * Clear the RX disable bit and then set the RX enable bit to enable
+	 * the receiver.
 	 */
-	writel((status & ~(CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS))
-			| (CDNS_UART_CR_TX_EN | CDNS_UART_CR_RX_EN |
-			CDNS_UART_CR_STOPBRK),
-			port->membase + CDNS_UART_CR_OFFSET);
+	status = readl(port->membase + CDNS_UART_CR_OFFSET);
+	status &= CDNS_UART_CR_RX_DIS;
+	status |= CDNS_UART_CR_RX_EN;
+	writel(status, port->membase + CDNS_UART_CR_OFFSET);
 
 	/* Set the Mode Register with normal mode,8 data bits,1 stop bit,
 	 * no parity.
@@ -814,6 +816,8 @@ static int cdns_uart_startup(struct uart_port *port)
 		CDNS_UART_IXR_RXTRIG | CDNS_UART_IXR_TOUT,
 		port->membase + CDNS_UART_IER_OFFSET);
 
+	spin_unlock_irqrestore(&port->lock, flags);
+
 	return retval;
 }
 
-- 
2.6.3.3.g9bb996a

Re: [PATCH LINUX v3 05/10] tty: xuartps: Improve startup function

[Peter Hurley]

On 11/21/2015 09:59 PM, Soren Brinkmann wrote:
> The startup function is supposed to initialize the UART for receiving.
> Hence, don't enable the TX part. Also, protect HW accesses with the port
> lock.

Reviewed-by: Peter Hurley <peter@hurleysoftware.com>

NB: the request_irq() should be _after_ h/w programming, otherwise an
interrupt could be triggered and in-progress before the h/w has been
setup.

Re: [PATCH LINUX v3 05/10] tty: xuartps: Improve startup function

[Sören Brinkmann]

On Sat, 2015-12-05 at 12:21PM -0500, Peter Hurley wrote:
> On 11/21/2015 09:59 PM, Soren Brinkmann wrote:
> > The startup function is supposed to initialize the UART for receiving.
> > Hence, don't enable the TX part. Also, protect HW accesses with the port
> > lock.
> 
> Reviewed-by: Peter Hurley <peter@hurleysoftware.com>
> 
> NB: the request_irq() should be _after_ h/w programming, otherwise an
> interrupt could be triggered and in-progress before the h/w has been
> setup.

Thanks for pointing that out. I'll patch that.

	Sören

[PATCH LINUX v3 06/10] tty: xuartps: Keep lock for whole ISR

[Soren Brinkmann]

The RX path in the interrupt handler released a lock unnecessarily.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 6ffd3bbe3e18..ab3995d00973 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -265,9 +265,7 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
 			uart_insert_char(port, isrstatus, CDNS_UART_IXR_OVERRUN,
 					data, status);
 		}
-		spin_unlock(&port->lock);
 		tty_flip_buffer_push(&port->state->port);
-		spin_lock(&port->lock);
 	}
 
 	/* Dispatch an appropriate handler */
-- 
2.6.3.3.g9bb996a

Re: [PATCH LINUX v3 06/10] tty: xuartps: Keep lock for whole ISR

[Peter Hurley]

On 11/21/2015 09:59 PM, Soren Brinkmann wrote:
> The RX path in the interrupt handler released a lock unnecessarily.

Yep. Remnant of old low_latency steering.

Reviewed-by: Peter Hurley <peter@hurleysoftware.com>

[PATCH LINUX v3 07/10] tty: xuartps: Acquire port lock for shutdown

[Soren Brinkmann]

Shutting down the UART port can happen while console operations are in
progress. Holding the port lock serializes these operations and avoids
the UART HW to be disabled in the middle of console prints.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index ab3995d00973..f3ac69387b0a 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -826,6 +826,9 @@ static int cdns_uart_startup(struct uart_port *port)
 static void cdns_uart_shutdown(struct uart_port *port)
 {
 	int status;
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
 
 	/* Disable interrupts */
 	status = readl(port->membase + CDNS_UART_IMR_OFFSET);
@@ -835,6 +838,9 @@ static void cdns_uart_shutdown(struct uart_port *port)
 	/* Disable the TX and RX */
 	writel(CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS,
 			port->membase + CDNS_UART_CR_OFFSET);
+
+	spin_unlock_irqrestore(&port->lock, flags);
+
 	free_irq(port->irq, port);
 }
 
-- 
2.6.3.3.g9bb996a

Re: [PATCH LINUX v3 07/10] tty: xuartps: Acquire port lock for shutdown

[Peter Hurley]

On 11/21/2015 09:59 PM, Soren Brinkmann wrote:
> Shutting down the UART port can happen while console operations are in
> progress. Holding the port lock serializes these operations and avoids
> the UART HW to be disabled in the middle of console prints.

Reviewed-by: Peter Hurley <peter@hurleysoftware.com>

[PATCH LINUX v3 08/10] tty: xuartps: Move RX path into helper function

[Soren Brinkmann]

Move RX-related IRQ handling into a helper function.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 50 +++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 22 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index f3ac69387b0a..db9e23eaf300 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -172,28 +172,8 @@ struct cdns_uart {
 #define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
 		clk_rate_change_nb);
 
-/**
- * cdns_uart_isr - Interrupt handler
- * @irq: Irq number
- * @dev_id: Id of the port
- *
- * Return: IRQHANDLED
- */
-static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
+static void cdns_uart_handle_rx(struct uart_port *port, unsigned int isrstatus)
 {
-	struct uart_port *port = (struct uart_port *)dev_id;
-	unsigned long flags;
-	unsigned int isrstatus, numbytes;
-	unsigned int data;
-	char status = TTY_NORMAL;
-
-	spin_lock_irqsave(&port->lock, flags);
-
-	/* Read the interrupt status register to determine which
-	 * interrupt(s) is/are active.
-	 */
-	isrstatus = readl(port->membase + CDNS_UART_ISR_OFFSET);
-
 	/*
 	 * There is no hardware break detection, so we interpret framing
 	 * error with all-zeros data as a break sequence. Most of the time,
@@ -223,6 +203,9 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
 		/* Receive Timeout Interrupt */
 		while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
 					CDNS_UART_SR_RXEMPTY)) {
+			u32 data;
+			char status = TTY_NORMAL;
+
 			data = readl(port->membase + CDNS_UART_FIFO_OFFSET);
 
 			/* Non-NULL byte after BREAK is garbage (99%) */
@@ -263,10 +246,33 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
 			}
 
 			uart_insert_char(port, isrstatus, CDNS_UART_IXR_OVERRUN,
-					data, status);
+					 data, status);
 		}
 		tty_flip_buffer_push(&port->state->port);
 	}
+}
+
+/**
+ * cdns_uart_isr - Interrupt handler
+ * @irq: Irq number
+ * @dev_id: Id of the port
+ *
+ * Return: IRQHANDLED
+ */
+static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
+{
+	struct uart_port *port = (struct uart_port *)dev_id;
+	unsigned long flags;
+	unsigned int isrstatus, numbytes;
+
+	spin_lock_irqsave(&port->lock, flags);
+
+	/* Read the interrupt status register to determine which
+	 * interrupt(s) is/are active.
+	 */
+	isrstatus = readl(port->membase + CDNS_UART_ISR_OFFSET);
+
+	cdns_uart_handle_rx(port, isrstatus);
 
 	/* Dispatch an appropriate handler */
 	if ((isrstatus & CDNS_UART_IXR_TXEMPTY) == CDNS_UART_IXR_TXEMPTY) {
-- 
2.6.3.3.g9bb996a

Re: [PATCH LINUX v3 08/10] tty: xuartps: Move RX path into helper function

[Peter Hurley]

On 11/21/2015 09:59 PM, Soren Brinkmann wrote:
> Move RX-related IRQ handling into a helper function.
Fixes a problem where every char received after a parity or frame error
in the current isr will also be tagged as a parity or frame error.

Reviewed-by: Peter Hurley <peter@hurleysoftware.com>

NB: the sysrq problem in cdns_uart_isr() with needing to drop the
locks is because cdns_uart_console_write() tries to take the port->lock.
The 8250 driver handles this problem by not trying to take the
port->lock if port->sysrq is non-zero. See serial8250_console_write().


> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> ---
>  drivers/tty/serial/xilinx_uartps.c | 50 +++++++++++++++++++++-----------------
>  1 file changed, 28 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
> index f3ac69387b0a..db9e23eaf300 100644
> --- a/drivers/tty/serial/xilinx_uartps.c
> +++ b/drivers/tty/serial/xilinx_uartps.c
> @@ -172,28 +172,8 @@ struct cdns_uart {
>  #define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
>  		clk_rate_change_nb);
>  
> -/**
> - * cdns_uart_isr - Interrupt handler
> - * @irq: Irq number
> - * @dev_id: Id of the port
> - *
> - * Return: IRQHANDLED
> - */
> -static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
> +static void cdns_uart_handle_rx(struct uart_port *port, unsigned int isrstatus)
>  {
> -	struct uart_port *port = (struct uart_port *)dev_id;
> -	unsigned long flags;
> -	unsigned int isrstatus, numbytes;
> -	unsigned int data;
> -	char status = TTY_NORMAL;

See, the previous code never reset the status back to TTY_NORMAL after any other
status.

> -
> -	spin_lock_irqsave(&port->lock, flags);
> -
> -	/* Read the interrupt status register to determine which
> -	 * interrupt(s) is/are active.
> -	 */
> -	isrstatus = readl(port->membase + CDNS_UART_ISR_OFFSET);
> -
>  	/*
>  	 * There is no hardware break detection, so we interpret framing
>  	 * error with all-zeros data as a break sequence. Most of the time,
> @@ -223,6 +203,9 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
>  		/* Receive Timeout Interrupt */
>  		while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
>  					CDNS_UART_SR_RXEMPTY)) {
> +			u32 data;
> +			char status = TTY_NORMAL;
> +
>  			data = readl(port->membase + CDNS_UART_FIFO_OFFSET);
>  
>  			/* Non-NULL byte after BREAK is garbage (99%) */
> @@ -263,10 +246,33 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
>  			}
>  
>  			uart_insert_char(port, isrstatus, CDNS_UART_IXR_OVERRUN,
> -					data, status);
> +					 data, status);
>  		}
>  		tty_flip_buffer_push(&port->state->port);
>  	}
> +}
> +
> +/**
> + * cdns_uart_isr - Interrupt handler
> + * @irq: Irq number
> + * @dev_id: Id of the port
> + *
> + * Return: IRQHANDLED
> + */
> +static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
> +{
> +	struct uart_port *port = (struct uart_port *)dev_id;
> +	unsigned long flags;
> +	unsigned int isrstatus, numbytes;
> +
> +	spin_lock_irqsave(&port->lock, flags);
> +
> +	/* Read the interrupt status register to determine which
> +	 * interrupt(s) is/are active.
> +	 */
> +	isrstatus = readl(port->membase + CDNS_UART_ISR_OFFSET);
> +
> +	cdns_uart_handle_rx(port, isrstatus);
>  
>  	/* Dispatch an appropriate handler */
>  	if ((isrstatus & CDNS_UART_IXR_TXEMPTY) == CDNS_UART_IXR_TXEMPTY) {
> 

Re: [PATCH LINUX v3 08/10] tty: xuartps: Move RX path into helper function

[Sören Brinkmann]

On Sat, 2015-12-05 at 12:40PM -0500, Peter Hurley wrote:
> On 11/21/2015 09:59 PM, Soren Brinkmann wrote:
> > Move RX-related IRQ handling into a helper function.
> Fixes a problem where every char received after a parity or frame error
> in the current isr will also be tagged as a parity or frame error.
> 
> Reviewed-by: Peter Hurley <peter@hurleysoftware.com>

Thanks. I'll add your text to the commit message.

> 
> NB: the sysrq problem in cdns_uart_isr() with needing to drop the
> locks is because cdns_uart_console_write() tries to take the port->lock.
> The 8250 driver handles this problem by not trying to take the
> port->lock if port->sysrq is non-zero. See serial8250_console_write().

I'll look into this. I'll see if I can include that in the next
iteration of this series or do it later.

	Thanks,
	Sören

[PATCH LINUX v3 09/10] tty: xuartps: Only handle RX IRQs when RX is enabled

[Soren Brinkmann]

Ignore RX-related interrupts if RX is not enabled.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index db9e23eaf300..3a1e508926c1 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -161,6 +161,7 @@ MODULE_PARM_DESC(rx_timeout, "Rx timeout, 1-255");
  * @pclk:		APB clock
  * @baud:		Current baud rate
  * @clk_rate_change_nb:	Notifier block for clock changes
+ * @flags:		Driver flags
  */
 struct cdns_uart {
 	struct uart_port	*port;
@@ -168,7 +169,11 @@ struct cdns_uart {
 	struct clk		*pclk;
 	unsigned int		baud;
 	struct notifier_block	clk_rate_change_nb;
+	u32			flags;
 };
+
+#define CDNS_FLAG_RX_ENABLED	BIT(0)
+
 #define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
 		clk_rate_change_nb);
 
@@ -262,6 +267,7 @@ static void cdns_uart_handle_rx(struct uart_port *port, unsigned int isrstatus)
 static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
 {
 	struct uart_port *port = (struct uart_port *)dev_id;
+	struct cdns_uart *cdns_uart = port->private_data;
 	unsigned long flags;
 	unsigned int isrstatus, numbytes;
 
@@ -272,7 +278,8 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
 	 */
 	isrstatus = readl(port->membase + CDNS_UART_ISR_OFFSET);
 
-	cdns_uart_handle_rx(port, isrstatus);
+	if (cdns_uart->flags & CDNS_FLAG_RX_ENABLED)
+		cdns_uart_handle_rx(port, isrstatus);
 
 	/* Dispatch an appropriate handler */
 	if ((isrstatus & CDNS_UART_IXR_TXEMPTY) == CDNS_UART_IXR_TXEMPTY) {
@@ -579,11 +586,13 @@ static void cdns_uart_stop_tx(struct uart_port *port)
 static void cdns_uart_stop_rx(struct uart_port *port)
 {
 	unsigned int regval;
+	struct cdns_uart *cdns_uart = port->private_data;
 
 	regval = readl(port->membase + CDNS_UART_CR_OFFSET);
 	regval |= CDNS_UART_CR_RX_DIS;
 	/* Disable the receiver */
 	writel(regval, port->membase + CDNS_UART_CR_OFFSET);
+	cdns_uart->flags &= ~CDNS_FLAG_RX_ENABLED;
 }
 
 /**
@@ -764,6 +773,7 @@ static int cdns_uart_startup(struct uart_port *port)
 {
 	unsigned long flags;
 	unsigned int retval = 0, status = 0;
+	struct cdns_uart *cdns_uart = port->private_data;
 
 	retval = request_irq(port->irq, cdns_uart_isr, 0, CDNS_UART_NAME,
 								(void *)port);
@@ -790,6 +800,7 @@ static int cdns_uart_startup(struct uart_port *port)
 	status &= CDNS_UART_CR_RX_DIS;
 	status |= CDNS_UART_CR_RX_EN;
 	writel(status, port->membase + CDNS_UART_CR_OFFSET);
+	cdns_uart->flags |= CDNS_FLAG_RX_ENABLED;
 
 	/* Set the Mode Register with normal mode,8 data bits,1 stop bit,
 	 * no parity.
@@ -833,6 +844,7 @@ static void cdns_uart_shutdown(struct uart_port *port)
 {
 	int status;
 	unsigned long flags;
+	struct cdns_uart *cdns_uart = port->private_data;
 
 	spin_lock_irqsave(&port->lock, flags);
 
@@ -844,6 +856,7 @@ static void cdns_uart_shutdown(struct uart_port *port)
 	/* Disable the TX and RX */
 	writel(CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS,
 			port->membase + CDNS_UART_CR_OFFSET);
+	cdns_uart->flags &= ~CDNS_FLAG_RX_ENABLED;
 
 	spin_unlock_irqrestore(&port->lock, flags);
 
-- 
2.6.3.3.g9bb996a

Re: [PATCH LINUX v3 09/10] tty: xuartps: Only handle RX IRQs when RX is enabled

[Peter Hurley]

On 11/21/2015 09:59 PM, Soren Brinkmann wrote:
> Ignore RX-related interrupts if RX is not enabled.

This doesn't look like a safe approach; fast-forward a couple of years and
some is trying to add some feature but can't figure out why no data is
arriving...

What is this trying to solve?

Regards,
Peter Hurley

> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> ---
>  drivers/tty/serial/xilinx_uartps.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
> index db9e23eaf300..3a1e508926c1 100644
> --- a/drivers/tty/serial/xilinx_uartps.c
> +++ b/drivers/tty/serial/xilinx_uartps.c
> @@ -161,6 +161,7 @@ MODULE_PARM_DESC(rx_timeout, "Rx timeout, 1-255");
>   * @pclk:		APB clock
>   * @baud:		Current baud rate
>   * @clk_rate_change_nb:	Notifier block for clock changes
> + * @flags:		Driver flags
>   */
>  struct cdns_uart {
>  	struct uart_port	*port;
> @@ -168,7 +169,11 @@ struct cdns_uart {
>  	struct clk		*pclk;
>  	unsigned int		baud;
>  	struct notifier_block	clk_rate_change_nb;
> +	u32			flags;
>  };
> +
> +#define CDNS_FLAG_RX_ENABLED	BIT(0)
> +
>  #define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
>  		clk_rate_change_nb);
>  
> @@ -262,6 +267,7 @@ static void cdns_uart_handle_rx(struct uart_port *port, unsigned int isrstatus)
>  static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
>  {
>  	struct uart_port *port = (struct uart_port *)dev_id;
> +	struct cdns_uart *cdns_uart = port->private_data;
>  	unsigned long flags;
>  	unsigned int isrstatus, numbytes;
>  
> @@ -272,7 +278,8 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
>  	 */
>  	isrstatus = readl(port->membase + CDNS_UART_ISR_OFFSET);
>  
> -	cdns_uart_handle_rx(port, isrstatus);
> +	if (cdns_uart->flags & CDNS_FLAG_RX_ENABLED)
> +		cdns_uart_handle_rx(port, isrstatus);
>  
>  	/* Dispatch an appropriate handler */
>  	if ((isrstatus & CDNS_UART_IXR_TXEMPTY) == CDNS_UART_IXR_TXEMPTY) {
> @@ -579,11 +586,13 @@ static void cdns_uart_stop_tx(struct uart_port *port)
>  static void cdns_uart_stop_rx(struct uart_port *port)
>  {
>  	unsigned int regval;
> +	struct cdns_uart *cdns_uart = port->private_data;
>  
>  	regval = readl(port->membase + CDNS_UART_CR_OFFSET);
>  	regval |= CDNS_UART_CR_RX_DIS;
>  	/* Disable the receiver */
>  	writel(regval, port->membase + CDNS_UART_CR_OFFSET);
> +	cdns_uart->flags &= ~CDNS_FLAG_RX_ENABLED;
>  }
>  
>  /**
> @@ -764,6 +773,7 @@ static int cdns_uart_startup(struct uart_port *port)
>  {
>  	unsigned long flags;
>  	unsigned int retval = 0, status = 0;
> +	struct cdns_uart *cdns_uart = port->private_data;
>  
>  	retval = request_irq(port->irq, cdns_uart_isr, 0, CDNS_UART_NAME,
>  								(void *)port);
> @@ -790,6 +800,7 @@ static int cdns_uart_startup(struct uart_port *port)
>  	status &= CDNS_UART_CR_RX_DIS;
>  	status |= CDNS_UART_CR_RX_EN;
>  	writel(status, port->membase + CDNS_UART_CR_OFFSET);
> +	cdns_uart->flags |= CDNS_FLAG_RX_ENABLED;
>  
>  	/* Set the Mode Register with normal mode,8 data bits,1 stop bit,
>  	 * no parity.
> @@ -833,6 +844,7 @@ static void cdns_uart_shutdown(struct uart_port *port)
>  {
>  	int status;
>  	unsigned long flags;
> +	struct cdns_uart *cdns_uart = port->private_data;
>  
>  	spin_lock_irqsave(&port->lock, flags);
>  
> @@ -844,6 +856,7 @@ static void cdns_uart_shutdown(struct uart_port *port)
>  	/* Disable the TX and RX */
>  	writel(CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS,
>  			port->membase + CDNS_UART_CR_OFFSET);
> +	cdns_uart->flags &= ~CDNS_FLAG_RX_ENABLED;
>  
>  	spin_unlock_irqrestore(&port->lock, flags);
>  
> 

Re: [PATCH LINUX v3 09/10] tty: xuartps: Only handle RX IRQs when RX is enabled

[Sören Brinkmann]

On Sat, 2015-12-05 at 12:43PM -0500, Peter Hurley wrote:
> On 11/21/2015 09:59 PM, Soren Brinkmann wrote:
> > Ignore RX-related interrupts if RX is not enabled.
> 
> This doesn't look like a safe approach; fast-forward a couple of years and
> some is trying to add some feature but can't figure out why no data is
> arriving...
> 
> What is this trying to solve?

I definitely saw two kinds of lock ups. One was on the TX side, that the
system tried to print with the transmitter disabled.

The other on the RX side. I don't recall the details, but the system was
stuck in some RX-related function waiting for data while the receiver
was disabled. I think my explanation was that some IRQ condition becomes
true and is dispatched to the RX handler even though the receiver isn't
enabled.
I guess a better way for solving this would be to disable the IRQs that
call the RX handler when the receiver is disabled.

	Sören

[PATCH LINUX v3 10/10] tty: xuartps: Cleanup: Reformat if-else

[Soren Brinkmann]

Convert an if-else into the more common early return on error, reducing
the indent level of the happy path.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 124 ++++++++++++++++++-------------------
 1 file changed, 62 insertions(+), 62 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 3a1e508926c1..7d180a2db67e 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -203,58 +203,55 @@ static void cdns_uart_handle_rx(struct uart_port *port, unsigned int isrstatus)
 	isrstatus &= port->read_status_mask;
 	isrstatus &= ~port->ignore_status_mask;
 
-	if ((isrstatus & CDNS_UART_IXR_TOUT) ||
-		(isrstatus & CDNS_UART_IXR_RXTRIG)) {
-		/* Receive Timeout Interrupt */
-		while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
-					CDNS_UART_SR_RXEMPTY)) {
-			u32 data;
-			char status = TTY_NORMAL;
-
-			data = readl(port->membase + CDNS_UART_FIFO_OFFSET);
-
-			/* Non-NULL byte after BREAK is garbage (99%) */
-			if (data && (port->read_status_mask &
-						CDNS_UART_IXR_BRK)) {
-				port->read_status_mask &= ~CDNS_UART_IXR_BRK;
-				port->icount.brk++;
-				if (uart_handle_break(port))
-					continue;
-			}
+	if (!(isrstatus & (CDNS_UART_IXR_TOUT | CDNS_UART_IXR_RXTRIG)))
+		return;
+
+	while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
+				CDNS_UART_SR_RXEMPTY)) {
+		u32 data;
+		char status = TTY_NORMAL;
+
+		data = readl(port->membase + CDNS_UART_FIFO_OFFSET);
+
+		/* Non-NULL byte after BREAK is garbage (99%) */
+		if (data && (port->read_status_mask & CDNS_UART_IXR_BRK)) {
+			port->read_status_mask &= ~CDNS_UART_IXR_BRK;
+			port->icount.brk++;
+			if (uart_handle_break(port))
+				continue;
+		}
 
 #ifdef SUPPORT_SYSRQ
-			/*
-			 * uart_handle_sysrq_char() doesn't work if
-			 * spinlocked, for some reason
-			 */
-			 if (port->sysrq) {
-				spin_unlock(&port->lock);
-				if (uart_handle_sysrq_char(port,
-							(unsigned char)data)) {
-					spin_lock(&port->lock);
-					continue;
-				}
+		/*
+		 * uart_handle_sysrq_char() doesn't work if
+		 * spinlocked, for some reason
+		 */
+		 if (port->sysrq) {
+			spin_unlock(&port->lock);
+			if (uart_handle_sysrq_char(port, data)) {
 				spin_lock(&port->lock);
+				continue;
 			}
+			spin_lock(&port->lock);
+		}
 #endif
 
-			port->icount.rx++;
-
-			if (isrstatus & CDNS_UART_IXR_PARITY) {
-				port->icount.parity++;
-				status = TTY_PARITY;
-			} else if (isrstatus & CDNS_UART_IXR_FRAMING) {
-				port->icount.frame++;
-				status = TTY_FRAME;
-			} else if (isrstatus & CDNS_UART_IXR_OVERRUN) {
-				port->icount.overrun++;
-			}
+		port->icount.rx++;
 
-			uart_insert_char(port, isrstatus, CDNS_UART_IXR_OVERRUN,
-					 data, status);
+		if (isrstatus & CDNS_UART_IXR_PARITY) {
+			port->icount.parity++;
+			status = TTY_PARITY;
+		} else if (isrstatus & CDNS_UART_IXR_FRAMING) {
+			port->icount.frame++;
+			status = TTY_FRAME;
+		} else if (isrstatus & CDNS_UART_IXR_OVERRUN) {
+			port->icount.overrun++;
 		}
-		tty_flip_buffer_push(&port->state->port);
+
+		uart_insert_char(port, isrstatus, CDNS_UART_IXR_OVERRUN,
+				 data, status);
 	}
+	tty_flip_buffer_push(&port->state->port);
 }
 
 /**
@@ -1434,27 +1431,30 @@ static int cdns_uart_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Cannot get uart_port structure\n");
 		rc = -ENODEV;
 		goto err_out_notif_unreg;
-	} else {
-		/* Register the port.
-		 * This function also registers this device with the tty layer
-		 * and triggers invocation of the config_port() entry point.
-		 */
-		port->mapbase = res->start;
-		port->irq = irq;
-		port->dev = &pdev->dev;
-		port->uartclk = clk_get_rate(cdns_uart_data->uartclk);
-		port->private_data = cdns_uart_data;
-		cdns_uart_data->port = port;
-		platform_set_drvdata(pdev, port);
-		rc = uart_add_one_port(&cdns_uart_uart_driver, port);
-		if (rc) {
-			dev_err(&pdev->dev,
-				"uart_add_one_port() failed; err=%i\n", rc);
-			goto err_out_notif_unreg;
-		}
-		return 0;
 	}
 
+	/*
+	 * Register the port.
+	 * This function also registers this device with the tty layer
+	 * and triggers invocation of the config_port() entry point.
+	 */
+	port->mapbase = res->start;
+	port->irq = irq;
+	port->dev = &pdev->dev;
+	port->uartclk = clk_get_rate(cdns_uart_data->uartclk);
+	port->private_data = cdns_uart_data;
+	cdns_uart_data->port = port;
+	platform_set_drvdata(pdev, port);
+
+	rc = uart_add_one_port(&cdns_uart_uart_driver, port);
+	if (rc) {
+		dev_err(&pdev->dev,
+			"uart_add_one_port() failed; err=%i\n", rc);
+		goto err_out_notif_unreg;
+	}
+
+	return 0;
+
 err_out_notif_unreg:
 #ifdef CONFIG_COMMON_CLK
 	clk_notifier_unregister(cdns_uart_data->uartclk,
-- 
2.6.3.3.g9bb996a

Re: [PATCH LINUX v3 00/10] tty: xuartps: Fix lock ups

[Peter Hurley]

On 11/21/2015 09:59 PM, Soren Brinkmann wrote:
> Hi,
> 
> here is v3 of this series. It's largely the same as before, but I
> adjusted 'tty: xuartps: Don't consider circular buffer when enabling
> transmitter' according to Peter's suggestions.

In reviewing this series, I thought the _OFFSET suffix for every register
address to be excessive. I would refactor all that out; IOW,

-	isrstatus = readl(port->membase + CDNS_UART_ISR_OFFSET);
+	isrstatus = readl(port->membase + CDNS_UART_ISR);


> I also spent some time trying to get Peter's test for flow control and
> xchar running. The xchar thing fails and will need some more work, but I
> think in general it should be possible to get it to work.
> The flow control test passes:
> 
> root@Xilinx-ZC1751-DC1:~# ./flow /dev/ttyPS0
> Test flow control on /dev/ttyPS0
> begin test1
> patterns sent: 223  recvd: 208
> read distribution: 1   = 0
>                    2+  = 0
>                    4+  = 0
>                    8+  = 0
>                   16+  = 0
>                   32+  = 643
>                   64+  = 0
>                  128+  = 0
>                  256+  = 0
>                  512+  = 0
> PASSED

This distribution looks like this exactly because the xchar test is
failing. IOW, this driver isn't performing soft flow control (^S,^Q)
like it should.

Regards,
Peter Hurley

> root@Xilinx-ZC1751-DC1:~# ./flow /dev/ttyPS1
> Test flow control on /dev/ttyPS1
> begin test1
> patterns sent: 223  recvd: 208
> read distribution: 1   = 0
>                    2+  = 0
>                    4+  = 0
>                    8+  = 0
>                   16+  = 0
>                   32+  = 643
>                   64+  = 0
>                  128+  = 0
>                  256+  = 0
>                  512+  = 0
> PASSED
> 
> 	Sören
> 
> Sören Brinkmann (10):
>   tty: xuartps: Beautify read-modify writes
>   tty: xuartps: Use spinlock to serialize HW access
>   tty: xuartps: Don't consider circular buffer when enabling transmitter
>   tty: xuartps: Clear interrupt status register in shutdown
>   tty: xuartps: Improve startup function
>   tty: xuartps: Keep lock for whole ISR
>   tty: xuartps: Acquire port lock for shutdown
>   tty: xuartps: Move RX path into helper function
>   tty: xuartps: Only handle RX IRQs when RX is enabled
>   tty: xuartps: Cleanup: Reformat if-else
> 
>  drivers/tty/serial/xilinx_uartps.c | 246 +++++++++++++++++++++----------------
>  1 file changed, 137 insertions(+), 109 deletions(-)
> 

Re: [PATCH LINUX v3 00/10] tty: xuartps: Fix lock ups

[Sören Brinkmann]

On Sat, 2015-12-05 at 12:49PM -0500, Peter Hurley wrote:
> On 11/21/2015 09:59 PM, Soren Brinkmann wrote:
> > Hi,
> > 
> > here is v3 of this series. It's largely the same as before, but I
> > adjusted 'tty: xuartps: Don't consider circular buffer when enabling
> > transmitter' according to Peter's suggestions.
> 
> In reviewing this series, I thought the _OFFSET suffix for every register
> address to be excessive. I would refactor all that out; IOW,
> 
> -	isrstatus = readl(port->membase + CDNS_UART_ISR_OFFSET);
> +	isrstatus = readl(port->membase + CDNS_UART_ISR);

I'll add that as a patch on top of everything.

> 
> 
> > I also spent some time trying to get Peter's test for flow control and
> > xchar running. The xchar thing fails and will need some more work, but I
> > think in general it should be possible to get it to work.
> > The flow control test passes:
> > 
> > root@Xilinx-ZC1751-DC1:~# ./flow /dev/ttyPS0
> > Test flow control on /dev/ttyPS0
> > begin test1
> > patterns sent: 223  recvd: 208
> > read distribution: 1   = 0
> >                    2+  = 0
> >                    4+  = 0
> >                    8+  = 0
> >                   16+  = 0
> >                   32+  = 643
> >                   64+  = 0
> >                  128+  = 0
> >                  256+  = 0
> >                  512+  = 0
> > PASSED
> 
> This distribution looks like this exactly because the xchar test is
> failing. IOW, this driver isn't performing soft flow control (^S,^Q)
> like it should.

I tried to the your xchar test working, but so far that failed. I'll try
to spend some more time on this.

	Thanks,
	Sören