Hi,
I picked up all the review tags from Peter and Moritz and addressed
Peter's comments.
Most patches are unchanged but I added the info Peter suggested/provided
to some commit messages.
The patches "tty: xuartps: Move request_irq to after setting up the HW",
"tty: xuartps: Improve sysrq handling" and "tty: xuartps: Remove
'_OFFSET' suffix from #defines" are new, implementing the suggestions
Peter made in the v3 review (https://lkml.org/lkml/2015/11/21/172).
I pretty much rewrote "tty: xuartps: Refactor IRQ handling" (was: "tty:
xuartps: Only handle RX IRQs when RX is enabled"). Instead of adding a
flag, we now disable/enable the RX interrupts together with the
receiver. That should prevent the RX lock ups I saw.
Thanks,
Sören
Sören Brinkmann (13):
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: Move request_irq to after setting up the HW
tty: xuartps: Keep lock for whole ISR
tty: xuartps: Acquire port lock for shutdown
tty: xuartps: Move RX path into helper function
tty: xuartps: Refactor IRQ handling
tty: xuartps: Cleanup: Reformat if-else
tty: xuartps: Improve sysrq handling
tty: xuartps: Remove '_OFFSET' suffix from #defines
drivers/tty/serial/xilinx_uartps.c | 451 ++++++++++++++++++-------------------
1 file changed, 224 insertions(+), 227 deletions(-)
--
2.6.3.3.g9bb996a
Non-functional, formatting changes to ease reading the code.
Signed-off-by: Soren Brinkmann <[email protected]>
Reviewed-by: Peter Hurley <[email protected]>
Reviewed-by: Moritz Fischer <[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]>
Reviewed-by: Peter Hurley <[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]>
Reviewed-by: Peter Hurley <[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. Bits
in the ISR are cleared by writing them as '1'.
Signed-off-by: Soren Brinkmann <[email protected]>
Reviewed-by: Peter Hurley <[email protected]>
Reviewed-by: Moritz Fischer <[email protected]>
---
v4:
- clarify workings of the ISR in the commit message
---
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]>
Reviewed-by: Peter Hurley <[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
Request_irq() should be _after_ h/w programming, otherwise an
interrupt could be triggered and in-progress before the h/w has been
setup.
Reported-by: Peter Hurley <[email protected]>
Signed-off-by: Soren Brinkmann <[email protected]>
---
v4:
- this patch has been added. Thanks to Peter for pointing it out and providing
commit message
---
drivers/tty/serial/xilinx_uartps.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 6ffd3bbe3e18..1e9053656610 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -759,12 +759,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,
- (void *)port);
- if (retval)
- return retval;
+ unsigned int status = 0;
spin_lock_irqsave(&port->lock, flags);
@@ -818,7 +813,7 @@ static int cdns_uart_startup(struct uart_port *port)
spin_unlock_irqrestore(&port->lock, flags);
- return retval;
+ return request_irq(port->irq, cdns_uart_isr, 0, CDNS_UART_NAME, port);
}
/**
--
2.6.3.3.g9bb996a
The RX path in the interrupt handler released a lock unnecessarily.
Signed-off-by: Soren Brinkmann <[email protected]>
Reviewed-by: Peter Hurley <[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 1e9053656610..62224a58056b 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]>
Reviewed-by: Peter Hurley <[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 62224a58056b..80248cb46bf6 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -821,6 +821,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);
@@ -830,6 +833,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.
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.
Signed-off-by: Soren Brinkmann <[email protected]>
Reviewed-by: Peter Hurley <[email protected]>
---
v4:
- added Peter's additional information to the commit message (thx)
---
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 80248cb46bf6..3780d1cf207e 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
The system could deadlock handling RX IRQs when RX-related IRQ
conditions became true while the receiver was disabled. To avoid this,
enable/disable the RX/TX IRQs together with the receiver/transmitter.
Signed-off-by: Soren Brinkmann <[email protected]>
---
drivers/tty/serial/xilinx_uartps.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 3780d1cf207e..1229bb3a275e 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -126,6 +126,10 @@ MODULE_PARM_DESC(rx_timeout, "Rx timeout, 1-255");
#define CDNS_UART_IXR_RXEMPTY 0x00000002 /* RX FIFO empty interrupt. */
#define CDNS_UART_IXR_MASK 0x00001FFF /* Valid bit mask */
+#define CDNS_UART_RX_IRQS (CDNS_UART_IXR_PARITY | CDNS_UART_IXR_FRAMING | \
+ CDNS_UART_IXR_OVERRUN | CDNS_UART_IXR_RXTRIG | \
+ CDNS_UART_IXR_TOUT)
+
/* Goes in read_status_mask for break detection as the HW doesn't do it*/
#define CDNS_UART_IXR_BRK 0x80000000
@@ -169,6 +173,7 @@ struct cdns_uart {
unsigned int baud;
struct notifier_block clk_rate_change_nb;
};
+
#define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
clk_rate_change_nb);
@@ -272,7 +277,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 (isrstatus & CDNS_UART_RX_IRQS)
+ cdns_uart_handle_rx(port, isrstatus);
/* Dispatch an appropriate handler */
if ((isrstatus & CDNS_UART_IXR_TXEMPTY) == CDNS_UART_IXR_TXEMPTY) {
@@ -580,9 +586,12 @@ static void cdns_uart_stop_rx(struct uart_port *port)
{
unsigned int regval;
+ /* Disable RX IRQs */
+ writel(CDNS_UART_RX_IRQS, port->membase + CDNS_UART_IDR_OFFSET);
+
+ /* Disable the receiver */
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);
}
@@ -810,10 +819,7 @@ static int cdns_uart_startup(struct uart_port *port)
port->membase + CDNS_UART_ISR_OFFSET);
/* Set the Interrupt Registers with desired interrupts */
- writel(CDNS_UART_IXR_TXEMPTY | CDNS_UART_IXR_PARITY |
- CDNS_UART_IXR_FRAMING | CDNS_UART_IXR_OVERRUN |
- CDNS_UART_IXR_RXTRIG | CDNS_UART_IXR_TOUT,
- port->membase + CDNS_UART_IER_OFFSET);
+ writel(CDNS_UART_RX_IRQS, port->membase + CDNS_UART_IER_OFFSET);
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 1229bb3a275e..7afab0c36528 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);
}
/**
@@ -1422,27 +1419,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
Handling magic sysrq included dropping a lock to avoid a deadlock that
happened when cdns_uart_console_write tried to acquire a lock in the
from the sysrq code path. By making the acquisition of the lock in
cdns_uart_console_write depending on port->sysrq, cdns_uart_handle_rx can be
simplified to simply call uart_handle_sysrq.
Suggested-by: Peter Hurley <[email protected]>
Signed-off-by: Soren Brinkmann <[email protected]>
---
v4:
- added this patch
---
drivers/tty/serial/xilinx_uartps.c | 20 +++++---------------
1 file changed, 5 insertions(+), 15 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 7afab0c36528..3b8d15fa5959 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -221,20 +221,8 @@ static void cdns_uart_handle_rx(struct uart_port *port, unsigned int isrstatus)
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, data)) {
- spin_lock(&port->lock);
- continue;
- }
- spin_lock(&port->lock);
- }
-#endif
+ if (uart_handle_sysrq_char(port, data))
+ continue;
port->icount.rx++;
@@ -1121,7 +1109,9 @@ static void cdns_uart_console_write(struct console *co, const char *s,
unsigned int imr, ctrl;
int locked = 1;
- if (oops_in_progress)
+ if (port->sysrq)
+ locked = 0;
+ else if (oops_in_progress)
locked = spin_trylock_irqsave(&port->lock, flags);
else
spin_lock_irqsave(&port->lock, flags);
--
2.6.3.3.g9bb996a
Remove the _OFFSET suffix from all register defines which makes code a
little easier to read and avoids a few line breaks.
Suggested-by: Peter Hurley <[email protected]>
Signed-off-by: Soren Brinkmann <[email protected]>
---
v4:
- added this patch
---
drivers/tty/serial/xilinx_uartps.c | 221 ++++++++++++++++++-------------------
1 file changed, 106 insertions(+), 115 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 3b8d15fa5959..91fe0814880e 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -50,24 +50,24 @@ module_param(rx_timeout, uint, S_IRUGO);
MODULE_PARM_DESC(rx_timeout, "Rx timeout, 1-255");
/* Register offsets for the UART. */
-#define CDNS_UART_CR_OFFSET 0x00 /* Control Register */
-#define CDNS_UART_MR_OFFSET 0x04 /* Mode Register */
-#define CDNS_UART_IER_OFFSET 0x08 /* Interrupt Enable */
-#define CDNS_UART_IDR_OFFSET 0x0C /* Interrupt Disable */
-#define CDNS_UART_IMR_OFFSET 0x10 /* Interrupt Mask */
-#define CDNS_UART_ISR_OFFSET 0x14 /* Interrupt Status */
-#define CDNS_UART_BAUDGEN_OFFSET 0x18 /* Baud Rate Generator */
-#define CDNS_UART_RXTOUT_OFFSET 0x1C /* RX Timeout */
-#define CDNS_UART_RXWM_OFFSET 0x20 /* RX FIFO Trigger Level */
-#define CDNS_UART_MODEMCR_OFFSET 0x24 /* Modem Control */
-#define CDNS_UART_MODEMSR_OFFSET 0x28 /* Modem Status */
-#define CDNS_UART_SR_OFFSET 0x2C /* Channel Status */
-#define CDNS_UART_FIFO_OFFSET 0x30 /* FIFO */
-#define CDNS_UART_BAUDDIV_OFFSET 0x34 /* Baud Rate Divider */
-#define CDNS_UART_FLOWDEL_OFFSET 0x38 /* Flow Delay */
-#define CDNS_UART_IRRX_PWIDTH_OFFSET 0x3C /* IR Min Received Pulse Width */
-#define CDNS_UART_IRTX_PWIDTH_OFFSET 0x40 /* IR Transmitted pulse Width */
-#define CDNS_UART_TXWM_OFFSET 0x44 /* TX FIFO Trigger Level */
+#define CDNS_UART_CR 0x00 /* Control Register */
+#define CDNS_UART_MR 0x04 /* Mode Register */
+#define CDNS_UART_IER 0x08 /* Interrupt Enable */
+#define CDNS_UART_IDR 0x0C /* Interrupt Disable */
+#define CDNS_UART_IMR 0x10 /* Interrupt Mask */
+#define CDNS_UART_ISR 0x14 /* Interrupt Status */
+#define CDNS_UART_BAUDGEN 0x18 /* Baud Rate Generator */
+#define CDNS_UART_RXTOUT 0x1C /* RX Timeout */
+#define CDNS_UART_RXWM 0x20 /* RX FIFO Trigger Level */
+#define CDNS_UART_MODEMCR 0x24 /* Modem Control */
+#define CDNS_UART_MODEMSR 0x28 /* Modem Status */
+#define CDNS_UART_SR 0x2C /* Channel Status */
+#define CDNS_UART_FIFO 0x30 /* FIFO */
+#define CDNS_UART_BAUDDIV 0x34 /* Baud Rate Divider */
+#define CDNS_UART_FLOWDEL 0x38 /* Flow Delay */
+#define CDNS_UART_IRRX_PWIDTH 0x3C /* IR Min Received Pulse Width */
+#define CDNS_UART_IRTX_PWIDTH 0x40 /* IR Transmitted pulse Width */
+#define CDNS_UART_TXWM 0x44 /* TX FIFO Trigger Level */
/* Control Register Bit Definitions */
#define CDNS_UART_CR_STOPBRK 0x00000100 /* Stop TX break */
@@ -185,15 +185,14 @@ static void cdns_uart_handle_rx(struct uart_port *port, unsigned int isrstatus)
* 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) &
+ while (!(readl(port->membase + CDNS_UART_SR) &
CDNS_UART_SR_RXEMPTY)) {
- if (!readl(port->membase + CDNS_UART_FIFO_OFFSET)) {
+ if (!readl(port->membase + CDNS_UART_FIFO)) {
port->read_status_mask |= CDNS_UART_IXR_BRK;
isrstatus &= ~CDNS_UART_IXR_FRAMING;
}
}
- writel(CDNS_UART_IXR_FRAMING,
- port->membase + CDNS_UART_ISR_OFFSET);
+ writel(CDNS_UART_IXR_FRAMING, port->membase + CDNS_UART_ISR);
}
/* drop byte with parity error if IGNPAR specified */
@@ -206,12 +205,11 @@ static void cdns_uart_handle_rx(struct uart_port *port, unsigned int isrstatus)
if (!(isrstatus & (CDNS_UART_IXR_TOUT | CDNS_UART_IXR_RXTRIG)))
return;
- while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
- CDNS_UART_SR_RXEMPTY)) {
+ while (!(readl(port->membase + CDNS_UART_SR) & CDNS_UART_SR_RXEMPTY)) {
u32 data;
char status = TTY_NORMAL;
- data = readl(port->membase + CDNS_UART_FIFO_OFFSET);
+ data = readl(port->membase + CDNS_UART_FIFO);
/* Non-NULL byte after BREAK is garbage (99%) */
if (data && (port->read_status_mask & CDNS_UART_IXR_BRK)) {
@@ -260,7 +258,7 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
/* Read the interrupt status register to determine which
* interrupt(s) is/are active.
*/
- isrstatus = readl(port->membase + CDNS_UART_ISR_OFFSET);
+ isrstatus = readl(port->membase + CDNS_UART_ISR);
if (isrstatus & CDNS_UART_RX_IRQS)
cdns_uart_handle_rx(port, isrstatus);
@@ -269,7 +267,7 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
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);
+ port->membase + CDNS_UART_IDR);
} else {
numbytes = port->fifosize;
/* Break if no more data available in the UART buffer */
@@ -282,7 +280,7 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
*/
writel(port->state->xmit.buf[
port->state->xmit.tail],
- port->membase + CDNS_UART_FIFO_OFFSET);
+ port->membase + CDNS_UART_FIFO);
port->icount.tx++;
@@ -300,7 +298,7 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
}
}
- writel(isrstatus, port->membase + CDNS_UART_ISR_OFFSET);
+ writel(isrstatus, port->membase + CDNS_UART_ISR);
/* be sure to release the lock and tty before leaving */
spin_unlock_irqrestore(&port->lock, flags);
@@ -390,14 +388,14 @@ static unsigned int cdns_uart_set_baud_rate(struct uart_port *port,
&div8);
/* Write new divisors to hardware */
- mreg = readl(port->membase + CDNS_UART_MR_OFFSET);
+ mreg = readl(port->membase + CDNS_UART_MR);
if (div8)
mreg |= CDNS_UART_MR_CLKSEL;
else
mreg &= ~CDNS_UART_MR_CLKSEL;
- writel(mreg, port->membase + CDNS_UART_MR_OFFSET);
- writel(cd, port->membase + CDNS_UART_BAUDGEN_OFFSET);
- writel(bdiv, port->membase + CDNS_UART_BAUDDIV_OFFSET);
+ writel(mreg, port->membase + CDNS_UART_MR);
+ writel(cd, port->membase + CDNS_UART_BAUDGEN);
+ writel(bdiv, port->membase + CDNS_UART_BAUDDIV);
cdns_uart->baud = baud;
return calc_baud;
@@ -444,9 +442,9 @@ static int cdns_uart_clk_notifier_cb(struct notifier_block *nb,
spin_lock_irqsave(&cdns_uart->port->lock, flags);
/* Disable the TX and RX to set baud rate */
- ctrl_reg = readl(port->membase + CDNS_UART_CR_OFFSET);
+ ctrl_reg = readl(port->membase + CDNS_UART_CR);
ctrl_reg |= CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS;
- writel(ctrl_reg, port->membase + CDNS_UART_CR_OFFSET);
+ writel(ctrl_reg, port->membase + CDNS_UART_CR);
spin_unlock_irqrestore(&cdns_uart->port->lock, flags);
@@ -471,11 +469,11 @@ static int cdns_uart_clk_notifier_cb(struct notifier_block *nb,
spin_lock_irqsave(&cdns_uart->port->lock, flags);
/* Set TX/RX Reset */
- ctrl_reg = readl(port->membase + CDNS_UART_CR_OFFSET);
+ ctrl_reg = readl(port->membase + CDNS_UART_CR);
ctrl_reg |= CDNS_UART_CR_TXRST | CDNS_UART_CR_RXRST;
- writel(ctrl_reg, port->membase + CDNS_UART_CR_OFFSET);
+ writel(ctrl_reg, port->membase + CDNS_UART_CR);
- while (readl(port->membase + CDNS_UART_CR_OFFSET) &
+ while (readl(port->membase + CDNS_UART_CR) &
(CDNS_UART_CR_TXRST | CDNS_UART_CR_RXRST))
cpu_relax();
@@ -484,11 +482,11 @@ static int cdns_uart_clk_notifier_cb(struct notifier_block *nb,
* enable bit and RX enable bit to enable the transmitter and
* receiver.
*/
- writel(rx_timeout, port->membase + CDNS_UART_RXTOUT_OFFSET);
- ctrl_reg = readl(port->membase + CDNS_UART_CR_OFFSET);
+ writel(rx_timeout, port->membase + CDNS_UART_RXTOUT);
+ ctrl_reg = readl(port->membase + CDNS_UART_CR);
ctrl_reg &= ~(CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS);
ctrl_reg |= CDNS_UART_CR_TX_EN | CDNS_UART_CR_RX_EN;
- writel(ctrl_reg, port->membase + CDNS_UART_CR_OFFSET);
+ writel(ctrl_reg, port->membase + CDNS_UART_CR);
spin_unlock_irqrestore(&cdns_uart->port->lock, flags);
@@ -514,15 +512,15 @@ static void cdns_uart_start_tx(struct uart_port *port)
* Set the TX enable bit and clear the TX disable bit to enable the
* transmitter.
*/
- status = readl(port->membase + CDNS_UART_CR_OFFSET);
+ status = readl(port->membase + CDNS_UART_CR);
status &= ~CDNS_UART_CR_TX_DIS;
status |= CDNS_UART_CR_TX_EN;
- writel(status, port->membase + CDNS_UART_CR_OFFSET);
+ writel(status, port->membase + CDNS_UART_CR);
if (uart_circ_empty(&port->state->xmit))
return;
- while (numbytes-- && ((readl(port->membase + CDNS_UART_SR_OFFSET) &
+ while (numbytes-- && ((readl(port->membase + CDNS_UART_SR) &
CDNS_UART_SR_TXFULL)) != CDNS_UART_SR_TXFULL) {
/* Break if no more data available in the UART buffer */
if (uart_circ_empty(&port->state->xmit))
@@ -532,7 +530,7 @@ static void cdns_uart_start_tx(struct uart_port *port)
* 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->membase + CDNS_UART_FIFO);
port->icount.tx++;
/* Adjust the tail of the UART buffer and wrap
@@ -541,9 +539,9 @@ static void cdns_uart_start_tx(struct uart_port *port)
port->state->xmit.tail = (port->state->xmit.tail + 1) &
(UART_XMIT_SIZE - 1);
}
- writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_ISR_OFFSET);
+ writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_ISR);
/* Enable the TX Empty interrupt */
- writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_IER_OFFSET);
+ writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_IER);
if (uart_circ_chars_pending(&port->state->xmit) < WAKEUP_CHARS)
uart_write_wakeup(port);
@@ -557,10 +555,10 @@ static void cdns_uart_stop_tx(struct uart_port *port)
{
unsigned int regval;
- regval = readl(port->membase + CDNS_UART_CR_OFFSET);
+ regval = readl(port->membase + CDNS_UART_CR);
regval |= CDNS_UART_CR_TX_DIS;
/* Disable the transmitter */
- writel(regval, port->membase + CDNS_UART_CR_OFFSET);
+ writel(regval, port->membase + CDNS_UART_CR);
}
/**
@@ -572,12 +570,12 @@ static void cdns_uart_stop_rx(struct uart_port *port)
unsigned int regval;
/* Disable RX IRQs */
- writel(CDNS_UART_RX_IRQS, port->membase + CDNS_UART_IDR_OFFSET);
+ writel(CDNS_UART_RX_IRQS, port->membase + CDNS_UART_IDR);
/* Disable the receiver */
- regval = readl(port->membase + CDNS_UART_CR_OFFSET);
+ regval = readl(port->membase + CDNS_UART_CR);
regval |= CDNS_UART_CR_RX_DIS;
- writel(regval, port->membase + CDNS_UART_CR_OFFSET);
+ writel(regval, port->membase + CDNS_UART_CR);
}
/**
@@ -590,7 +588,7 @@ static unsigned int cdns_uart_tx_empty(struct uart_port *port)
{
unsigned int status;
- status = readl(port->membase + CDNS_UART_SR_OFFSET) &
+ status = readl(port->membase + CDNS_UART_SR) &
CDNS_UART_SR_TXEMPTY;
return status ? TIOCSER_TEMT : 0;
}
@@ -608,15 +606,15 @@ static void cdns_uart_break_ctl(struct uart_port *port, int ctl)
spin_lock_irqsave(&port->lock, flags);
- status = readl(port->membase + CDNS_UART_CR_OFFSET);
+ status = readl(port->membase + CDNS_UART_CR);
if (ctl == -1)
writel(CDNS_UART_CR_STARTBRK | status,
- port->membase + CDNS_UART_CR_OFFSET);
+ port->membase + CDNS_UART_CR);
else {
if ((status & CDNS_UART_CR_STOPBRK) == 0)
writel(CDNS_UART_CR_STOPBRK | status,
- port->membase + CDNS_UART_CR_OFFSET);
+ port->membase + CDNS_UART_CR);
}
spin_unlock_irqrestore(&port->lock, flags);
}
@@ -639,18 +637,18 @@ static void cdns_uart_set_termios(struct uart_port *port,
spin_lock_irqsave(&port->lock, flags);
/* Wait for the transmit FIFO to empty before making changes */
- if (!(readl(port->membase + CDNS_UART_CR_OFFSET) &
+ if (!(readl(port->membase + CDNS_UART_CR) &
CDNS_UART_CR_TX_DIS)) {
- while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
+ while (!(readl(port->membase + CDNS_UART_SR) &
CDNS_UART_SR_TXEMPTY)) {
cpu_relax();
}
}
/* Disable the TX and RX to set baud rate */
- ctrl_reg = readl(port->membase + CDNS_UART_CR_OFFSET);
+ ctrl_reg = readl(port->membase + CDNS_UART_CR);
ctrl_reg |= CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS;
- writel(ctrl_reg, port->membase + CDNS_UART_CR_OFFSET);
+ writel(ctrl_reg, port->membase + CDNS_UART_CR);
/*
* Min baud rate = 6bps and Max Baud Rate is 10Mbps for 100Mhz clk
@@ -669,20 +667,20 @@ static void cdns_uart_set_termios(struct uart_port *port,
uart_update_timeout(port, termios->c_cflag, baud);
/* Set TX/RX Reset */
- ctrl_reg = readl(port->membase + CDNS_UART_CR_OFFSET);
+ ctrl_reg = readl(port->membase + CDNS_UART_CR);
ctrl_reg |= CDNS_UART_CR_TXRST | CDNS_UART_CR_RXRST;
- writel(ctrl_reg, port->membase + CDNS_UART_CR_OFFSET);
+ writel(ctrl_reg, port->membase + CDNS_UART_CR);
/*
* 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.
*/
- ctrl_reg = readl(port->membase + CDNS_UART_CR_OFFSET);
+ ctrl_reg = readl(port->membase + CDNS_UART_CR);
ctrl_reg &= ~(CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS);
ctrl_reg |= CDNS_UART_CR_TX_EN | CDNS_UART_CR_RX_EN;
- writel(ctrl_reg, port->membase + CDNS_UART_CR_OFFSET);
+ writel(ctrl_reg, port->membase + CDNS_UART_CR);
- writel(rx_timeout, port->membase + CDNS_UART_RXTOUT_OFFSET);
+ writel(rx_timeout, port->membase + CDNS_UART_RXTOUT);
port->read_status_mask = CDNS_UART_IXR_TXEMPTY | CDNS_UART_IXR_RXTRIG |
CDNS_UART_IXR_OVERRUN | CDNS_UART_IXR_TOUT;
@@ -702,7 +700,7 @@ static void cdns_uart_set_termios(struct uart_port *port,
CDNS_UART_IXR_TOUT | CDNS_UART_IXR_PARITY |
CDNS_UART_IXR_FRAMING | CDNS_UART_IXR_OVERRUN;
- mode_reg = readl(port->membase + CDNS_UART_MR_OFFSET);
+ mode_reg = readl(port->membase + CDNS_UART_MR);
/* Handling Data Size */
switch (termios->c_cflag & CSIZE) {
@@ -743,7 +741,7 @@ static void cdns_uart_set_termios(struct uart_port *port,
cval |= CDNS_UART_MR_PARITY_NONE;
}
cval |= mode_reg & 1;
- writel(cval, port->membase + CDNS_UART_MR_OFFSET);
+ writel(cval, port->membase + CDNS_UART_MR);
spin_unlock_irqrestore(&port->lock, flags);
}
@@ -763,48 +761,48 @@ static int cdns_uart_startup(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);
+ port->membase + CDNS_UART_CR);
/* Set the Control Register with TX/RX Enable, TX/RX Reset,
* no break chars.
*/
writel(CDNS_UART_CR_TXRST | CDNS_UART_CR_RXRST,
- port->membase + CDNS_UART_CR_OFFSET);
+ port->membase + CDNS_UART_CR);
/*
* Clear the RX disable bit and then set the RX enable bit to enable
* the receiver.
*/
- status = readl(port->membase + CDNS_UART_CR_OFFSET);
+ status = readl(port->membase + CDNS_UART_CR);
status &= CDNS_UART_CR_RX_DIS;
status |= CDNS_UART_CR_RX_EN;
- writel(status, port->membase + CDNS_UART_CR_OFFSET);
+ writel(status, port->membase + CDNS_UART_CR);
/* Set the Mode Register with normal mode,8 data bits,1 stop bit,
* no parity.
*/
writel(CDNS_UART_MR_CHMODE_NORM | CDNS_UART_MR_STOPMODE_1_BIT
| CDNS_UART_MR_PARITY_NONE | CDNS_UART_MR_CHARLEN_8_BIT,
- port->membase + CDNS_UART_MR_OFFSET);
+ port->membase + CDNS_UART_MR);
/*
* Set the RX FIFO Trigger level to use most of the FIFO, but it
* can be tuned with a module parameter
*/
- writel(rx_trigger_level, port->membase + CDNS_UART_RXWM_OFFSET);
+ writel(rx_trigger_level, port->membase + CDNS_UART_RXWM);
/*
* Receive Timeout register is enabled but it
* can be tuned with a module parameter
*/
- writel(rx_timeout, port->membase + CDNS_UART_RXTOUT_OFFSET);
+ writel(rx_timeout, port->membase + CDNS_UART_RXTOUT);
/* Clear out any pending interrupts before enabling them */
- writel(readl(port->membase + CDNS_UART_ISR_OFFSET),
- port->membase + CDNS_UART_ISR_OFFSET);
+ writel(readl(port->membase + CDNS_UART_ISR),
+ port->membase + CDNS_UART_ISR);
/* Set the Interrupt Registers with desired interrupts */
- writel(CDNS_UART_RX_IRQS, port->membase + CDNS_UART_IER_OFFSET);
+ writel(CDNS_UART_RX_IRQS, port->membase + CDNS_UART_IER);
spin_unlock_irqrestore(&port->lock, flags);
@@ -823,13 +821,13 @@ static void cdns_uart_shutdown(struct uart_port *port)
spin_lock_irqsave(&port->lock, flags);
/* 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);
+ status = readl(port->membase + CDNS_UART_IMR);
+ writel(status, port->membase + CDNS_UART_IDR);
+ writel(0xffffffff, port->membase + CDNS_UART_ISR);
/* Disable the TX and RX */
writel(CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS,
- port->membase + CDNS_UART_CR_OFFSET);
+ port->membase + CDNS_UART_CR);
spin_unlock_irqrestore(&port->lock, flags);
@@ -934,7 +932,7 @@ static void cdns_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
{
u32 val;
- val = readl(port->membase + CDNS_UART_MODEMCR_OFFSET);
+ val = readl(port->membase + CDNS_UART_MODEMCR);
val &= ~(CDNS_UART_MODEMCR_RTS | CDNS_UART_MODEMCR_DTR);
@@ -943,7 +941,7 @@ static void cdns_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
if (mctrl & TIOCM_DTR)
val |= CDNS_UART_MODEMCR_DTR;
- writel(val, port->membase + CDNS_UART_MODEMCR_OFFSET);
+ writel(val, port->membase + CDNS_UART_MODEMCR);
}
#ifdef CONFIG_CONSOLE_POLL
@@ -955,11 +953,10 @@ static int cdns_uart_poll_get_char(struct uart_port *port)
spin_lock_irqsave(&port->lock, flags);
/* Check if FIFO is empty */
- if (readl(port->membase + CDNS_UART_SR_OFFSET) & CDNS_UART_SR_RXEMPTY)
+ if (readl(port->membase + CDNS_UART_SR) & CDNS_UART_SR_RXEMPTY)
c = NO_POLL_CHAR;
else /* Read a character */
- c = (unsigned char) readl(
- port->membase + CDNS_UART_FIFO_OFFSET);
+ c = (unsigned char) readl(port->membase + CDNS_UART_FIFO);
spin_unlock_irqrestore(&port->lock, flags);
@@ -973,16 +970,14 @@ static void cdns_uart_poll_put_char(struct uart_port *port, unsigned char c)
spin_lock_irqsave(&port->lock, flags);
/* Wait until FIFO is empty */
- while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
- CDNS_UART_SR_TXEMPTY))
+ while (!(readl(port->membase + CDNS_UART_SR) & CDNS_UART_SR_TXEMPTY))
cpu_relax();
/* Write a character */
- writel(c, port->membase + CDNS_UART_FIFO_OFFSET);
+ writel(c, port->membase + CDNS_UART_FIFO);
/* Wait until FIFO is empty */
- while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
- CDNS_UART_SR_TXEMPTY))
+ while (!(readl(port->membase + CDNS_UART_SR) & CDNS_UART_SR_TXEMPTY))
cpu_relax();
spin_unlock_irqrestore(&port->lock, flags);
@@ -1059,8 +1054,7 @@ static struct uart_port *cdns_uart_get_port(int id)
*/
static void cdns_uart_console_wait_tx(struct uart_port *port)
{
- while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
- CDNS_UART_SR_TXEMPTY))
+ while (!(readl(port->membase + CDNS_UART_SR) & CDNS_UART_SR_TXEMPTY))
barrier();
}
@@ -1072,7 +1066,7 @@ static void cdns_uart_console_wait_tx(struct uart_port *port)
static void cdns_uart_console_putchar(struct uart_port *port, int ch)
{
cdns_uart_console_wait_tx(port);
- writel(ch, port->membase + CDNS_UART_FIFO_OFFSET);
+ writel(ch, port->membase + CDNS_UART_FIFO);
}
static void __init cdns_early_write(struct console *con, const char *s,
@@ -1117,25 +1111,25 @@ static void cdns_uart_console_write(struct console *co, const char *s,
spin_lock_irqsave(&port->lock, flags);
/* save and disable interrupt */
- imr = readl(port->membase + CDNS_UART_IMR_OFFSET);
- writel(imr, port->membase + CDNS_UART_IDR_OFFSET);
+ imr = readl(port->membase + CDNS_UART_IMR);
+ writel(imr, port->membase + CDNS_UART_IDR);
/*
* Make sure that the tx part is enabled. Set the TX enable bit and
* clear the TX disable bit to enable the transmitter.
*/
- ctrl = readl(port->membase + CDNS_UART_CR_OFFSET);
+ ctrl = readl(port->membase + CDNS_UART_CR);
ctrl &= ~CDNS_UART_CR_TX_DIS;
ctrl |= CDNS_UART_CR_TX_EN;
- writel(ctrl, port->membase + CDNS_UART_CR_OFFSET);
+ writel(ctrl, port->membase + CDNS_UART_CR);
uart_console_write(port, s, count, cdns_uart_console_putchar);
cdns_uart_console_wait_tx(port);
- writel(ctrl, port->membase + CDNS_UART_CR_OFFSET);
+ writel(ctrl, port->membase + CDNS_UART_CR);
/* restore interrupt state */
- writel(imr, port->membase + CDNS_UART_IER_OFFSET);
+ writel(imr, port->membase + CDNS_UART_IER);
if (locked)
spin_unlock_irqrestore(&port->lock, flags);
@@ -1247,14 +1241,13 @@ static int cdns_uart_suspend(struct device *device)
spin_lock_irqsave(&port->lock, flags);
/* Empty the receive FIFO 1st before making changes */
- while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
+ while (!(readl(port->membase + CDNS_UART_SR) &
CDNS_UART_SR_RXEMPTY))
- readl(port->membase + CDNS_UART_FIFO_OFFSET);
+ readl(port->membase + CDNS_UART_FIFO);
/* set RX trigger level to 1 */
- writel(1, port->membase + CDNS_UART_RXWM_OFFSET);
+ writel(1, port->membase + CDNS_UART_RXWM);
/* disable RX timeout interrups */
- writel(CDNS_UART_IXR_TOUT,
- port->membase + CDNS_UART_IDR_OFFSET);
+ writel(CDNS_UART_IXR_TOUT, port->membase + CDNS_UART_IDR);
spin_unlock_irqrestore(&port->lock, flags);
}
@@ -1293,30 +1286,28 @@ static int cdns_uart_resume(struct device *device)
spin_lock_irqsave(&port->lock, flags);
/* Set TX/RX Reset */
- ctrl_reg = readl(port->membase + CDNS_UART_CR_OFFSET);
+ ctrl_reg = readl(port->membase + CDNS_UART_CR);
ctrl_reg |= CDNS_UART_CR_TXRST | CDNS_UART_CR_RXRST;
- writel(ctrl_reg, port->membase + CDNS_UART_CR_OFFSET);
- while (readl(port->membase + CDNS_UART_CR_OFFSET) &
+ writel(ctrl_reg, port->membase + CDNS_UART_CR);
+ while (readl(port->membase + CDNS_UART_CR) &
(CDNS_UART_CR_TXRST | CDNS_UART_CR_RXRST))
cpu_relax();
/* restore rx timeout value */
- writel(rx_timeout, port->membase + CDNS_UART_RXTOUT_OFFSET);
+ writel(rx_timeout, port->membase + CDNS_UART_RXTOUT);
/* Enable Tx/Rx */
- ctrl_reg = readl(port->membase + CDNS_UART_CR_OFFSET);
+ ctrl_reg = readl(port->membase + CDNS_UART_CR);
ctrl_reg &= ~(CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS);
ctrl_reg |= CDNS_UART_CR_TX_EN | CDNS_UART_CR_RX_EN;
- writel(ctrl_reg, port->membase + CDNS_UART_CR_OFFSET);
+ writel(ctrl_reg, port->membase + CDNS_UART_CR);
spin_unlock_irqrestore(&port->lock, flags);
} else {
spin_lock_irqsave(&port->lock, flags);
/* restore original rx trigger level */
- writel(rx_trigger_level,
- port->membase + CDNS_UART_RXWM_OFFSET);
+ writel(rx_trigger_level, port->membase + CDNS_UART_RXWM);
/* enable RX timeout interrupt */
- writel(CDNS_UART_IXR_TOUT,
- port->membase + CDNS_UART_IER_OFFSET);
+ writel(CDNS_UART_IXR_TOUT, port->membase + CDNS_UART_IER);
spin_unlock_irqrestore(&port->lock, flags);
}
--
2.6.3.3.g9bb996a
On 12/05/2015 08:39 PM, Soren Brinkmann wrote:
> Request_irq() should be _after_ h/w programming, otherwise an
> interrupt could be triggered and in-progress before the h/w has been
> setup.
Slight misunderstanding. My fault; I should have been more explicit.
1. Any setup necessary for the isr not to be confused and misdirect spurious
interrupts (or hang) should be before installing the isr with request_irq()
None of this code should trigger an interrupt.
2. Clear pending interrupts
3. Install the isr with request_irq()
4. Enable interrupts
For extra safety, first disable interrupts before starting h/w programming.
I would do the v5 series in the same order as the v3 series only up to
what I reviewed. Then do another series with the remainder plus new changes, ok?
Regards,
Peter Hurley
> Reported-by: Peter Hurley <[email protected]>
> Signed-off-by: Soren Brinkmann <[email protected]>
> ---
> v4:
> - this patch has been added. Thanks to Peter for pointing it out and providing
> commit message
> ---
> drivers/tty/serial/xilinx_uartps.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
> index 6ffd3bbe3e18..1e9053656610 100644
> --- a/drivers/tty/serial/xilinx_uartps.c
> +++ b/drivers/tty/serial/xilinx_uartps.c
> @@ -759,12 +759,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,
> - (void *)port);
> - if (retval)
> - return retval;
> + unsigned int status = 0;
>
> spin_lock_irqsave(&port->lock, flags);
>
> @@ -818,7 +813,7 @@ static int cdns_uart_startup(struct uart_port *port)
>
> spin_unlock_irqrestore(&port->lock, flags);
>
> - return retval;
> + return request_irq(port->irq, cdns_uart_isr, 0, CDNS_UART_NAME, port);
> }
>
> /**
>
On Thu, 2015-12-10 at 01:41PM -0800, Peter Hurley wrote:
> On 12/05/2015 08:39 PM, Soren Brinkmann wrote:
> > Request_irq() should be _after_ h/w programming, otherwise an
> > interrupt could be triggered and in-progress before the h/w has been
> > setup.
>
> Slight misunderstanding. My fault; I should have been more explicit.
>
> 1. Any setup necessary for the isr not to be confused and misdirect spurious
> interrupts (or hang) should be before installing the isr with request_irq()
> None of this code should trigger an interrupt.
> 2. Clear pending interrupts
> 3. Install the isr with request_irq()
> 4. Enable interrupts
Isn't that what the startup function is doing now - more or less. I
think 3 and 4 are swapped to release the lock and then do the
request_irq, but I believe that should be OK.
The startup function configures the HW. Clears the ISR. Enables the
intended IRQs and then does the request_irq call.
>
> For extra safety, first disable interrupts before starting h/w programming.
It's done within spin_lock_irqsave, which gives us at least locally
disabled IRQs. I guess we could add a disabling all IRQs in the UART
core, but it should not really be necessary.
>
> I would do the v5 series in the same order as the v3 series only up to
> what I reviewed. Then do another series with the remainder plus new changes, ok?
Sure.
Sören
>
> Regards,
> Peter Hurley
>
> > Reported-by: Peter Hurley <[email protected]>
> > Signed-off-by: Soren Brinkmann <[email protected]>
> > ---
> > v4:
> > - this patch has been added. Thanks to Peter for pointing it out and providing
> > commit message
> > ---
> > drivers/tty/serial/xilinx_uartps.c | 9 ++-------
> > 1 file changed, 2 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
> > index 6ffd3bbe3e18..1e9053656610 100644
> > --- a/drivers/tty/serial/xilinx_uartps.c
> > +++ b/drivers/tty/serial/xilinx_uartps.c
> > @@ -759,12 +759,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,
> > - (void *)port);
> > - if (retval)
> > - return retval;
> > + unsigned int status = 0;
> >
> > spin_lock_irqsave(&port->lock, flags);
> >
> > @@ -818,7 +813,7 @@ static int cdns_uart_startup(struct uart_port *port)
> >
> > spin_unlock_irqrestore(&port->lock, flags);
> >
> > - return retval;
> > + return request_irq(port->irq, cdns_uart_isr, 0, CDNS_UART_NAME, port);
> > }
> >
> > /**
> >
>
On 12/15/2015 07:41 AM, Sören Brinkmann wrote:
> On Thu, 2015-12-10 at 01:41PM -0800, Peter Hurley wrote:
>> On 12/05/2015 08:39 PM, Soren Brinkmann wrote:
>>> Request_irq() should be _after_ h/w programming, otherwise an
>>> interrupt could be triggered and in-progress before the h/w has been
>>> setup.
>>
>> Slight misunderstanding. My fault; I should have been more explicit.
>>
>> 1. Any setup necessary for the isr not to be confused and misdirect spurious
>> interrupts (or hang) should be before installing the isr with request_irq()
>> None of this code should trigger an interrupt.
>> 2. Clear pending interrupts
>> 3. Install the isr with request_irq()
>> 4. Enable interrupts
>
> Isn't that what the startup function is doing now - more or less. I
> think 3 and 4 are swapped to release the lock and then do the
> request_irq, but I believe that should be OK.
> The startup function configures the HW. Clears the ISR. Enables the
> intended IRQs and then does the request_irq call.
If the driver enables interrupts before installing the isr with request_irq()
and an interrupt occurs there will the no handler to catch it and EOI the
device.
>> For extra safety, first disable interrupts before starting h/w programming.
>
> It's done within spin_lock_irqsave, which gives us at least locally
> disabled IRQs. I guess we could add a disabling all IRQs in the UART
> core, but it should not really be necessary.
Similar issue.
What I mean is to mask interrupts from this device so that h/w programming
doesn't accidentally trigger an interrupt for which no isr is installed.
It's a bit overkill; that's why I said "extra safety".
Regards,
Peter Hurley
>> I would do the v5 series in the same order as the v3 series only up to
>> what I reviewed. Then do another series with the remainder plus new changes, ok?
>
> Sure.
>
> Sören
>
>>
>> Regards,
>> Peter Hurley
>>
>>> Reported-by: Peter Hurley <[email protected]>
>>> Signed-off-by: Soren Brinkmann <[email protected]>
>>> ---
>>> v4:
>>> - this patch has been added. Thanks to Peter for pointing it out and providing
>>> commit message
>>> ---
>>> drivers/tty/serial/xilinx_uartps.c | 9 ++-------
>>> 1 file changed, 2 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
>>> index 6ffd3bbe3e18..1e9053656610 100644
>>> --- a/drivers/tty/serial/xilinx_uartps.c
>>> +++ b/drivers/tty/serial/xilinx_uartps.c
>>> @@ -759,12 +759,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,
>>> - (void *)port);
>>> - if (retval)
>>> - return retval;
>>> + unsigned int status = 0;
>>>
>>> spin_lock_irqsave(&port->lock, flags);
>>>
>>> @@ -818,7 +813,7 @@ static int cdns_uart_startup(struct uart_port *port)
>>>
>>> spin_unlock_irqrestore(&port->lock, flags);
>>>
>>> - return retval;
>>> + return request_irq(port->irq, cdns_uart_isr, 0, CDNS_UART_NAME, port);
>>> }
>>>
>>> /**
>>>
>>
On Tue, 2015-12-15 at 03:26PM -0800, Peter Hurley wrote:
> On 12/15/2015 07:41 AM, Sören Brinkmann wrote:
> > On Thu, 2015-12-10 at 01:41PM -0800, Peter Hurley wrote:
> >> On 12/05/2015 08:39 PM, Soren Brinkmann wrote:
> >>> Request_irq() should be _after_ h/w programming, otherwise an
> >>> interrupt could be triggered and in-progress before the h/w has been
> >>> setup.
> >>
> >> Slight misunderstanding. My fault; I should have been more explicit.
> >>
> >> 1. Any setup necessary for the isr not to be confused and misdirect spurious
> >> interrupts (or hang) should be before installing the isr with request_irq()
> >> None of this code should trigger an interrupt.
> >> 2. Clear pending interrupts
> >> 3. Install the isr with request_irq()
> >> 4. Enable interrupts
> >
> > Isn't that what the startup function is doing now - more or less. I
> > think 3 and 4 are swapped to release the lock and then do the
> > request_irq, but I believe that should be OK.
> > The startup function configures the HW. Clears the ISR. Enables the
> > intended IRQs and then does the request_irq call.
>
> If the driver enables interrupts before installing the isr with request_irq()
> and an interrupt occurs there will the no handler to catch it and EOI the
> device.
Really? Shouldn't the IRQ be masked in the interrupt controller until
everything is in place?
Sören
On 12/16/2015 01:03 AM, Sören Brinkmann wrote:
> On Tue, 2015-12-15 at 03:26PM -0800, Peter Hurley wrote:
>> On 12/15/2015 07:41 AM, Sören Brinkmann wrote:
>>> On Thu, 2015-12-10 at 01:41PM -0800, Peter Hurley wrote:
>>>> On 12/05/2015 08:39 PM, Soren Brinkmann wrote:
>>>>> Request_irq() should be _after_ h/w programming, otherwise an
>>>>> interrupt could be triggered and in-progress before the h/w has been
>>>>> setup.
>>>>
>>>> Slight misunderstanding. My fault; I should have been more explicit.
>>>>
>>>> 1. Any setup necessary for the isr not to be confused and misdirect spurious
>>>> interrupts (or hang) should be before installing the isr with request_irq()
>>>> None of this code should trigger an interrupt.
>>>> 2. Clear pending interrupts
>>>> 3. Install the isr with request_irq()
>>>> 4. Enable interrupts
>>>
>>> Isn't that what the startup function is doing now - more or less. I
>>> think 3 and 4 are swapped to release the lock and then do the
>>> request_irq, but I believe that should be OK.
>>> The startup function configures the HW. Clears the ISR. Enables the
>>> intended IRQs and then does the request_irq call.
>>
>> If the driver enables interrupts before installing the isr with request_irq()
>> and an interrupt occurs there will the no handler to catch it and EOI the
>> device.
>
> Really? Shouldn't the IRQ be masked in the interrupt controller until
> everything is in place?
Sorry, I'm used to shared interrupts, where that isn't the case.
Regards,
Peter Hurley
On Wed, 2015-12-16 at 06:37AM -0800, Peter Hurley wrote:
> On 12/16/2015 01:03 AM, Sören Brinkmann wrote:
> > On Tue, 2015-12-15 at 03:26PM -0800, Peter Hurley wrote:
> >> On 12/15/2015 07:41 AM, Sören Brinkmann wrote:
> >>> On Thu, 2015-12-10 at 01:41PM -0800, Peter Hurley wrote:
> >>>> On 12/05/2015 08:39 PM, Soren Brinkmann wrote:
> >>>>> Request_irq() should be _after_ h/w programming, otherwise an
> >>>>> interrupt could be triggered and in-progress before the h/w has been
> >>>>> setup.
> >>>>
> >>>> Slight misunderstanding. My fault; I should have been more explicit.
> >>>>
> >>>> 1. Any setup necessary for the isr not to be confused and misdirect spurious
> >>>> interrupts (or hang) should be before installing the isr with request_irq()
> >>>> None of this code should trigger an interrupt.
> >>>> 2. Clear pending interrupts
> >>>> 3. Install the isr with request_irq()
> >>>> 4. Enable interrupts
> >>>
> >>> Isn't that what the startup function is doing now - more or less. I
> >>> think 3 and 4 are swapped to release the lock and then do the
> >>> request_irq, but I believe that should be OK.
> >>> The startup function configures the HW. Clears the ISR. Enables the
> >>> intended IRQs and then does the request_irq call.
> >>
> >> If the driver enables interrupts before installing the isr with request_irq()
> >> and an interrupt occurs there will the no handler to catch it and EOI the
> >> device.
> >
> > Really? Shouldn't the IRQ be masked in the interrupt controller until
> > everything is in place?
>
> Sorry, I'm used to shared interrupts, where that isn't the case.
Ahh, I didn't have to deal with such cases yet. Makes sense though.
Thanks,
Sören