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
Non-functional, formatting changes to ease reading the code.
Signed-off-by: Soren Brinkmann <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Soren Brinkmann <[email protected]>
---
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
When shutting down the UART, clear the interrupt status register.
Signed-off-by: Soren Brinkmann <[email protected]>
---
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
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 <[email protected]>
---
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
The RX path in the interrupt handler released a lock unnecessarily.
Signed-off-by: Soren Brinkmann <[email protected]>
---
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
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 <[email protected]>
---
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
Move RX-related IRQ handling into a helper function.
Signed-off-by: Soren Brinkmann <[email protected]>
---
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
Ignore RX-related interrupts if RX is not enabled.
Signed-off-by: Soren Brinkmann <[email protected]>
---
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
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 <[email protected]>
---
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
On 11/21/2015 09:59 PM, Soren Brinkmann wrote:
> Non-functional, formatting changes to ease reading the code.
Reviewed-by: Peter Hurley <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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.
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 <[email protected]>
On Sat, Dec 5, 2015 at 9:04 AM, Peter Hurley <[email protected]> wrote:
> On 11/21/2015 09:59 PM, Soren Brinkmann wrote:
>> Non-functional, formatting changes to ease reading the code.
>
> Reviewed-by: Peter Hurley <[email protected]>
Reviewed-by: Moritz Fischer <[email protected]
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 <[email protected]>
On Sat, Dec 5, 2015 at 9:19 AM, Peter Hurley <[email protected]> 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 <[email protected]>
Reviewed-by: Moritz Fischer <[email protected]>
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 <[email protected]>
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 <[email protected]>
> ---
> 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) {
>
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 <[email protected]>
> ---
> 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);
>
>
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(-)
>
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 <[email protected]>
>
> 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
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 <[email protected]>
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
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
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