Breaking the single big ISR that has both Rx and Tx
in a single function into smaller ones
Signed-off-by: Nava kishore Manne <[email protected]>
---
Changes for v2:
--Splits up the ISR without any functional changes as suggested
by Peter Hurley
drivers/tty/serial/xilinx_uartps.c | 247 ++++++++++++++++++++-----------------
1 file changed, 135 insertions(+), 112 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 009e0db..2e1b0a8 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -172,6 +172,9 @@ struct cdns_uart {
#define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
clk_rate_change_nb);
+static void cdns_uart_handle_tx(void *dev_id);
+static void cdns_uart_handle_rx(void *dev_id, unsigned int isrstatus);
+
/**
* cdns_uart_isr - Interrupt handler
* @irq: Irq number
@@ -183,9 +186,7 @@ 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;
- unsigned int data;
- char status = TTY_NORMAL;
+ unsigned int isrstatus;
spin_lock_irqsave(&port->lock, flags);
@@ -194,116 +195,12 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
*/
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,
- * there's another non-zero byte at the end of the sequence.
- */
- if (isrstatus & CDNS_UART_IXR_FRAMING) {
- while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
- CDNS_UART_SR_RXEMPTY)) {
- if (!readl(port->membase + CDNS_UART_FIFO_OFFSET)) {
- port->read_status_mask |= CDNS_UART_IXR_BRK;
- isrstatus &= ~CDNS_UART_IXR_FRAMING;
- }
- }
- writel(CDNS_UART_IXR_FRAMING,
- port->membase + CDNS_UART_ISR_OFFSET);
- }
-
- /* drop byte with parity error if IGNPAR specified */
- if (isrstatus & port->ignore_status_mask & CDNS_UART_IXR_PARITY)
- isrstatus &= ~(CDNS_UART_IXR_RXTRIG | CDNS_UART_IXR_TOUT);
-
- 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)) {
- 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;
- }
- 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++;
- }
-
- 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 */
- if ((isrstatus & CDNS_UART_IXR_TXEMPTY) == CDNS_UART_IXR_TXEMPTY) {
- if (uart_circ_empty(&port->state->xmit)) {
- writel(CDNS_UART_IXR_TXEMPTY,
- port->membase + CDNS_UART_IDR_OFFSET);
- } else {
- numbytes = port->fifosize;
- /* Break if no more data available in the UART buffer */
- while (numbytes--) {
- if (uart_circ_empty(&port->state->xmit))
- break;
- /* Get the data from the UART circular buffer
- * and write it to the cdns_uart's TX_FIFO
- * register.
- */
- writel(port->state->xmit.buf[
- port->state->xmit.tail],
- port->membase + CDNS_UART_FIFO_OFFSET);
-
- port->icount.tx++;
-
- /* Adjust the tail of the UART buffer and wrap
- * the buffer if it reaches limit.
- */
- port->state->xmit.tail =
- (port->state->xmit.tail + 1) &
- (UART_XMIT_SIZE - 1);
- }
-
- if (uart_circ_chars_pending(
- &port->state->xmit) < WAKEUP_CHARS)
- uart_write_wakeup(port);
- }
+ if (isrstatus & CDNS_UART_IXR_TXEMPTY) {
+ cdns_uart_handle_tx(dev_id);
+ isrstatus &= ~CDNS_UART_IXR_TXEMPTY;
}
+ if (isrstatus & CDNS_UART_IXR_MASK)
+ cdns_uart_handle_rx(dev_id, isrstatus);
writel(isrstatus, port->membase + CDNS_UART_ISR_OFFSET);
@@ -408,6 +305,132 @@ static unsigned int cdns_uart_set_baud_rate(struct uart_port *port,
return calc_baud;
}
+/**
+ * cdns_uart_handle_tx - Handle the bytes to be Txed.
+ * @dev_id: Id of the UART port
+ * Return: None
+ */
+static void cdns_uart_handle_tx(void *dev_id)
+{
+ struct uart_port *port = (struct uart_port *)dev_id;
+ unsigned int numbytes;
+
+ if (uart_circ_empty(&port->state->xmit)) {
+ writel(CDNS_UART_IXR_TXEMPTY,
+ port->membase + CDNS_UART_IDR_OFFSET);
+ } else {
+ numbytes = port->fifosize;
+ /* Break if no more data available in the UART buffer */
+ while (numbytes--) {
+ if (uart_circ_empty(&port->state->xmit))
+ break;
+ /* Get the data from the UART circular buffer
+ * and write it to the cdns_uart's TX_FIFO
+ * register.
+ */
+ writel(port->state->xmit.buf[port->state->xmit.tail],
+ port->membase + CDNS_UART_FIFO_OFFSET);
+ port->icount.tx++;
+
+ /* Adjust the tail of the UART buffer and wrap
+ * the buffer if it reaches limit.
+ */
+ port->state->xmit.tail = (port->state->xmit.tail + 1) &
+ (UART_XMIT_SIZE - 1);
+ }
+
+ if (uart_circ_chars_pending(&port->state->xmit) < WAKEUP_CHARS)
+ uart_write_wakeup(port);
+ }
+}
+
+/**
+ * cdns_uart_handle_rx - Handle the received bytes along with Rx errors.
+ * @dev_id: Id of the UART port
+ * @isrstatus: The interrupt status register value as read
+ * Return: None
+ */
+static void cdns_uart_handle_rx(void *dev_id, unsigned int isrstatus)
+{
+ struct uart_port *port = (struct uart_port *)dev_id;
+ unsigned int data;
+ char status = TTY_NORMAL;
+
+ /*
+ * There is no hardware break detection, so we interpret framing
+ * error with all-zeros data as a break sequence. Most of the time,
+ * there's another non-zero byte at the end of the sequence.
+ */
+ if (isrstatus & CDNS_UART_IXR_FRAMING) {
+ while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
+ CDNS_UART_SR_RXEMPTY)) {
+ if (!readl(port->membase + CDNS_UART_FIFO_OFFSET)) {
+ port->read_status_mask |= CDNS_UART_IXR_BRK;
+ isrstatus &= ~CDNS_UART_IXR_FRAMING;
+ }
+ }
+ writel(CDNS_UART_IXR_FRAMING,
+ port->membase + CDNS_UART_ISR_OFFSET);
+ }
+
+ /* drop byte with parity error if IGNPAR specified */
+ if (isrstatus & port->ignore_status_mask & CDNS_UART_IXR_PARITY)
+ isrstatus &= ~(CDNS_UART_IXR_RXTRIG | CDNS_UART_IXR_TOUT);
+
+ 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) != CDNS_UART_SR_RXEMPTY) {
+ 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;
+ }
+ 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++;
+ }
+
+ 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);
+}
+}
#ifdef CONFIG_COMMON_CLK
/**
* cdns_uart_clk_notitifer_cb - Clock notifier callback
--
2.1.2
The existing interrupt handling logic has followins issues.
- Upon a parity error with default configuration, the control
never comes out of the ISR thereby hanging Linux.
- The error handling logic around framing and parity error are buggy.
There are chances that the errors will never be captured.
This patch fixes all these concerns.
Signed-off-by: Nava kishore Manne <[email protected]>
---
Changes for v2:
--Add required changes after spliting the ISR as suggested by
Peter Hurley.
drivers/tty/serial/xilinx_uartps.c | 64 ++++++++++++++++++--------------------
1 file changed, 31 insertions(+), 33 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 2e1b0a8..4d71478 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -191,9 +191,10 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
spin_lock_irqsave(&port->lock, flags);
/* Read the interrupt status register to determine which
- * interrupt(s) is/are active.
+ * interrupt(s) is/are active and clear them.
*/
isrstatus = readl(port->membase + CDNS_UART_ISR_OFFSET);
+ writel(isrstatus, port->membase + CDNS_UART_ISR_OFFSET);
if (isrstatus & CDNS_UART_IXR_TXEMPTY) {
cdns_uart_handle_tx(dev_id);
@@ -202,8 +203,6 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
if (isrstatus & CDNS_UART_IXR_MASK)
cdns_uart_handle_rx(dev_id, isrstatus);
- writel(isrstatus, port->membase + CDNS_UART_ISR_OFFSET);
-
/* be sure to release the lock and tty before leaving */
spin_unlock_irqrestore(&port->lock, flags);
@@ -354,37 +353,32 @@ static void cdns_uart_handle_rx(void *dev_id, unsigned int isrstatus)
{
struct uart_port *port = (struct uart_port *)dev_id;
unsigned int data;
+ unsigned int framerrprocessed = 0;
char status = TTY_NORMAL;
- /*
- * There is no hardware break detection, so we interpret framing
- * error with all-zeros data as a break sequence. Most of the time,
- * there's another non-zero byte at the end of the sequence.
- */
- if (isrstatus & CDNS_UART_IXR_FRAMING) {
- while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
- CDNS_UART_SR_RXEMPTY)) {
- if (!readl(port->membase + CDNS_UART_FIFO_OFFSET)) {
+ while ((readl(port->membase + CDNS_UART_SR_OFFSET) &
+ CDNS_UART_SR_RXEMPTY) != CDNS_UART_SR_RXEMPTY) {
+ data = readl(port->membase + CDNS_UART_FIFO_OFFSET);
+ port->icount.rx++;
+
+ /*
+ * There is no hardware break detection, so we interpret framing
+ * error with all-zeros data as a break sequence. Most of the
+ * time, there's another non-zero byte at the end of the
+ * sequence.
+ */
+ if (isrstatus & CDNS_UART_IXR_FRAMING) {
+ if (!data) {
port->read_status_mask |= CDNS_UART_IXR_BRK;
- isrstatus &= ~CDNS_UART_IXR_FRAMING;
+ framerrprocessed = 1;
+ continue;
}
}
- writel(CDNS_UART_IXR_FRAMING,
- port->membase + CDNS_UART_ISR_OFFSET);
- }
- /* drop byte with parity error if IGNPAR specified */
- if (isrstatus & port->ignore_status_mask & CDNS_UART_IXR_PARITY)
- isrstatus &= ~(CDNS_UART_IXR_RXTRIG | CDNS_UART_IXR_TOUT);
-
- 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) != CDNS_UART_SR_RXEMPTY) {
- data = readl(port->membase + CDNS_UART_FIFO_OFFSET);
+ isrstatus &= port->read_status_mask;
+ isrstatus &= ~port->ignore_status_mask;
+ if ((isrstatus & CDNS_UART_IXR_TOUT) ||
+ (isrstatus & CDNS_UART_IXR_RXTRIG)) {
/* Non-NULL byte after BREAK is garbage (99%) */
if (data && (port->read_status_mask
@@ -416,21 +410,25 @@ static void cdns_uart_handle_rx(void *dev_id, unsigned int isrstatus)
if (isrstatus & CDNS_UART_IXR_PARITY) {
port->icount.parity++;
status = TTY_PARITY;
- } else if (isrstatus & CDNS_UART_IXR_FRAMING) {
+ }
+ if (isrstatus & CDNS_UART_IXR_FRAMING) {
port->icount.frame++;
status = TTY_FRAME;
- } else if (isrstatus & CDNS_UART_IXR_OVERRUN) {
+ }
+ if (isrstatus & CDNS_UART_IXR_OVERRUN) {
port->icount.overrun++;
+ tty_insert_flip_char(&port->state->port, 0,
+ TTY_OVERRUN);
}
- uart_insert_char(port, isrstatus, CDNS_UART_IXR_OVERRUN,
- data, status);
+ tty_insert_flip_char(&port->state->port, data, status);
}
+ }
spin_unlock(&port->lock);
tty_flip_buffer_push(&port->state->port);
spin_lock(&port->lock);
}
-}
+
#ifdef CONFIG_COMMON_CLK
/**
* cdns_uart_clk_notitifer_cb - Clock notifier callback
--
2.1.2
The patch makes changes not to enable parity error interrupt.
With the current implementation, each parity error results in
two distinct interrupts (almost always). The first one is normal
parity error interrupt with no data in the fifo and the second one
is a proper Rx interrupt with the received data in the fifo. By
disabling parity error interrupt we still ensure handling of
parity errors as for the Rx fifo interrupt the parity error still
shows up in the interrupt status register. Considering the fact
that the by default INPCK and IGNPAR are not set, this is the
optimal implementation for parity error handling.
Signed-off-by: Nava kishore Manne <[email protected]>
---
Changes for v2:
--None
drivers/tty/serial/xilinx_uartps.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 4d71478..082e7f7 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -824,8 +824,18 @@ static int cdns_uart_startup(struct uart_port *port)
writel(readl(port->membase + CDNS_UART_ISR_OFFSET),
port->membase + CDNS_UART_ISR_OFFSET);
- /* Set the Interrupt Registers with desired interrupts */
- writel(CDNS_UART_IXR_TXEMPTY | CDNS_UART_IXR_PARITY |
+ /*
+ * Set the Interrupt Registers with desired interrupts. Do not
+ * enable parity error interrupt for the following reason:
+ * When parity error interrupt is enabled, each Rx parity error always
+ * results in 2 events. The first one being parity error interrupt
+ * and the second one with a proper Rx interrupt with the incoming data.
+ * Disabling parity error interrupt ensures better handling of parity
+ * error events. With this change, for a parity error case, we get a
+ * Rx interrupt with parity error set in ISR register and we still
+ * handle parity errors in the desired way.
+ */
+ writel(CDNS_UART_IXR_TXEMPTY |
CDNS_UART_IXR_FRAMING | CDNS_UART_IXR_OVERRUN |
CDNS_UART_IXR_RXTRIG | CDNS_UART_IXR_TOUT,
port->membase + CDNS_UART_IER_OFFSET);
--
2.1.2
On Fri, 2015-11-20 at 07:04PM +0530, Nava kishore Manne wrote:
> Breaking the single big ISR that has both Rx and Tx
> in a single function into smaller ones
>
> Signed-off-by: Nava kishore Manne <[email protected]>
> ---
> Changes for v2:
> --Splits up the ISR without any functional changes as suggested
> by Peter Hurley
>
> drivers/tty/serial/xilinx_uartps.c | 247 ++++++++++++++++++++-----------------
> 1 file changed, 135 insertions(+), 112 deletions(-)
>
> diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
> index 009e0db..2e1b0a8 100644
> --- a/drivers/tty/serial/xilinx_uartps.c
> +++ b/drivers/tty/serial/xilinx_uartps.c
> @@ -172,6 +172,9 @@ struct cdns_uart {
> #define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
> clk_rate_change_nb);
>
> +static void cdns_uart_handle_tx(void *dev_id);
> +static void cdns_uart_handle_rx(void *dev_id, unsigned int isrstatus);
Can't you just move these functions here instead of adding prototypes
here? Also, this is likely to conflict with
https://lkml.org/lkml/2015/11/19/622
Sören
> -----Original Message-----
> From: Sören Brinkmann [mailto:[email protected]]
> Sent: Friday, November 20, 2015 8:14 PM
> To: Nava kishore Manne
> Cc: [email protected]; Michal Simek; Anirudha Sarangi; linux-
> [email protected]; [email protected]; linux-
> [email protected]; Nava kishore Manne
> Subject: Re: [PATCH v2 1/3] serial: xuartps: Spliting the ISR into smaller
> routines.
>
> On Fri, 2015-11-20 at 07:04PM +0530, Nava kishore Manne wrote:
> > Breaking the single big ISR that has both Rx and Tx in a single
> > function into smaller ones
> >
> > Signed-off-by: Nava kishore Manne <[email protected]>
> > ---
> > Changes for v2:
> > --Splits up the ISR without any functional changes as suggested
> > by Peter Hurley
> >
> > drivers/tty/serial/xilinx_uartps.c | 247
> > ++++++++++++++++++++-----------------
> > 1 file changed, 135 insertions(+), 112 deletions(-)
> >
> > diff --git a/drivers/tty/serial/xilinx_uartps.c
> > b/drivers/tty/serial/xilinx_uartps.c
> > index 009e0db..2e1b0a8 100644
> > --- a/drivers/tty/serial/xilinx_uartps.c
> > +++ b/drivers/tty/serial/xilinx_uartps.c
> > @@ -172,6 +172,9 @@ struct cdns_uart { #define to_cdns_uart(_nb)
> > container_of(_nb, struct cdns_uart, \
> > clk_rate_change_nb);
> >
> > +static void cdns_uart_handle_tx(void *dev_id); static void
> > +cdns_uart_handle_rx(void *dev_id, unsigned int isrstatus);
>
> Can't you just move these functions here instead of adding prototypes here?
Initially I created the patch without adding prototypes but that patch was unreadable
Due to the common lines of code b/w the functions so created the prototypes to make the
Patch more readable as suggested by Peter.
Will add one more patch in the next series to get rid of the prototypes.
> Also, this is likely to conflict with
> https://lkml.org/lkml/2015/11/19/622
Yes I agree l see that no comments for your series of patches as of now...
Let me know whether I should wait until your patches got applied or you want me to send the next version
Of my patches...
Regards,
Navakishore.
>
> Sören
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
Hi Navakishore,
On 12/02/2015 12:32 AM, Nava kishore Manne wrote:
> Yes I agree l see that no comments for your series of patches as of now...
Reviewing Sören's patches are on my TODO list before the end of
the week.
Regards,
Peter Hurley