drivers: uart: uart_cmsdk_apb: Fix uart_irq_is_pending

Currently CMSDK uart_irq_is_pending does not use RX and TX interrupt
bits found in INTSTATUS register to check for pending interrutps but
rather it checks for pending interrupts indirectly by checking if RX and
TX buffers are, respectively, full and empty, i.e. it checks bits 0 and
1 in STATE register instead of bits meant for interrupt status found in
INTSTATUS register.

That is particularly problematic because although a RX interrupt implies
a RX buffer full and a TX interrupt implies a TX buffer empty, the
converse is not true. For instance, a TX buffer might be empty for all
data was processed (sent to serial line) already and no further data was
pushed into TX buffer so it remained empty, without generating any
additional TX interrupt. In that case the current uart_irq_is_pending
implementation reports that there are pending interrupts because of the
following logic:

/* Return true if rx buffer full or tx buffer empty */
return (UART_STRUCT(dev)->state & (UART_RX_BF | UART_TX_BF))
                                != UART_TX_BF;

which will return 1 (true) if STATE[0] = 0 (TX buffer is empty), since
UART_TX_BF = 1, so STATE[0] != UART_TX_BF, which is true (assuming here
for the sake of simplicity that UART_RX_BF = 0, i.e. RX buffer is empty
too).

One of the common uses of uart_irq_is_pending is in ISR in contructs
like the following:

while (uart_irq_update(dev) && uart_irq_is_pending(dev)) {
  if (uart_irq_rx_ready(dev) == 0) { // RX buffer is empty
    continue;
  }
  // RX buffer is full, process RX data
}

So the ISR can be called due to a RX interrupt. Upon finishing
processing the RX data uart_irq_is_pending is called to check for any
pending IRQs and if it happens that TX buffer is empty (like in the case
that TX interrupt is totally disabled) execution gets stuck in the while
loop because TX buffer will never transition to full again, i.e. it will
never have a chance to have STATE[0] = 1, so STATE[0] != UART_TX_BF is
always true.

This commit fixes that undesirable and problematic behavior by making
uart_irq_is_pending use the proper bits in the interrupt status register
(INTSTATUS) to determine if there is indeed any pending interrupts.

That, on the other hand, requires that the pending interrupt flags are
not clearly automatically when calling the ISR, otherwise
uart_irq_is_pending() will return immediatly false on the first call
without any data being really processed inside the ISR. Thus, because
both RX and TX buffer (FIFO) are only 1 byte long, that commit clears
the proper interrupts flags precisely when data is processed (fifo_read
and fifo_fill) or when the interrupts are disabled (irq_rx_disable and
irq_tx_disable).

Finally, that commits also takes the chance to update some comments,
specially regarding the need to "prime" when enabling the TX interrupts
(in uart_cmsdk_apb_irq_tx_enable()). The need to "prime" can be verified
in the CMSDK UART reference implementation in Verilog mentioned in the
"Arm Cortex-M System Design Kit" [0], on p. 4-8, section 4.3,
in cmsdk_apb_uart.v. In that implementation it's also possible to verify
that the FIFO is only 1 byte long, justifying the semantics that if
buffers are not full (STATE[0] or STATE[1] = 0) they are _completly_
empty, holding no data at all.

[0] https://documentation-service.arm.com/static/5e8f1c777100066a414f770b

Signed-off-by: Gustavo Romero <[email protected]>

Author: gromero

drivers/serial/uart_cmsdk_apb.c

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016 Linaro Limited
+ * Copyright (c) 2021, Linaro Limited.
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -217,8 +217,18 @@ static int uart_cmsdk_apb_fifo_fill(const struct device *dev,
 {
 	volatile struct uart_cmsdk_apb *uart = UART_STRUCT(dev);
 
-	/* No hardware FIFO present */
+	/*
+	 * No hardware FIFO present. Only 1 byte
+	 * to write if TX buffer is empty.
+	 */
 	if (len && !(uart->state & UART_TX_BF)) {
+		/*
+		 * Clear TX int. pending flag before pushing byte to "FIFO".
+		 * If TX interrupt is enabled the UART_TX_IN bit will be set
+		 * again automatically by the UART hardware machinery once
+		 * the "FIFO" becomes empty again.
+		 */
+		uart->intclear = UART_TX_IN;
 		uart->data = *tx_data;
 		return 1;
 	}
@@ -240,8 +250,18 @@ static int uart_cmsdk_apb_fifo_read(const struct device *dev,
 {
 	volatile struct uart_cmsdk_apb *uart = UART_STRUCT(dev);
 
-	/* No hardware FIFO present */
+	/*
+	 * No hardware FIFO present. Only 1 byte
+	 * to read if RX buffer is full.
+	 */
 	if (size && uart->state & UART_RX_BF) {
+		/*
+		 * Clear RX int. pending flag before popping byte from "FIFO".
+		 * If RX interrupt is enabled the UART_RX_IN bit will be set
+		 * again automatically by the UART hardware machinery once
+		 * the "FIFO" becomes full again.
+		 */
+		uart->intclear = UART_RX_IN;
 		*rx_data = (unsigned char)uart->data;
 		return 1;
 	}
@@ -262,10 +282,12 @@ static void uart_cmsdk_apb_irq_tx_enable(const struct device *dev)
 
 	UART_STRUCT(dev)->ctrl |= UART_TX_IN_EN;
 	/* The expectation is that TX is a level interrupt, active for as
-	 * long as TX buffer is empty. But in CMSDK UART, it appears to be
-	 * edge interrupt, firing on a state change of TX buffer. So, we
-	 * need to "prime" it here by calling ISR directly, to get interrupt
-	 * processing going.
+	 * long as TX buffer is empty. But in CMSDK UART it's an edge
+	 * interrupt, firing on a state change of TX buffer from full to
+	 * empty. So, we need to "prime" it here by calling ISR directly,
+	 * to get interrupt processing going, as there is no previous
+	 * full state to allow a transition from full to empty buffer
+	 * that will trigger a TX interrupt.
 	 */
 	key = irq_lock();
 	uart_cmsdk_apb_isr(dev);
@@ -282,6 +304,8 @@ static void uart_cmsdk_apb_irq_tx_enable(const struct device *dev)
 static void uart_cmsdk_apb_irq_tx_disable(const struct device *dev)
 {
 	UART_STRUCT(dev)->ctrl &= ~UART_TX_IN_EN;
+	/* Clear any pending TX interrupt after disabling it */
+	UART_STRUCT(dev)->intclear = UART_TX_IN;
 }
 
 /**
@@ -318,6 +342,8 @@ static void uart_cmsdk_apb_irq_rx_enable(const struct device *dev)
 static void uart_cmsdk_apb_irq_rx_disable(const struct device *dev)
 {
 	UART_STRUCT(dev)->ctrl &= ~UART_RX_IN_EN;
+	/* Clear any pending RX interrupt after disabling it */
+	UART_STRUCT(dev)->intclear = UART_RX_IN;
 }
 
 /**
@@ -377,9 +403,7 @@ static void uart_cmsdk_apb_irq_err_disable(const struct device *dev)
  */
 static int uart_cmsdk_apb_irq_is_pending(const struct device *dev)
 {
-	/* Return true if rx buffer full or tx buffer empty */
-	return (UART_STRUCT(dev)->state & (UART_RX_BF | UART_TX_BF))
-					!= UART_TX_BF;
+	return (UART_STRUCT(dev)->intstatus & (UART_RX_IN | UART_TX_IN));
 }
 
 /**
@@ -421,12 +445,8 @@ static void uart_cmsdk_apb_irq_callback_set(const struct device *dev,
  */
 void uart_cmsdk_apb_isr(const struct device *dev)
 {
-	volatile struct uart_cmsdk_apb *uart = UART_STRUCT(dev);
 	struct uart_cmsdk_apb_dev_data *data = DEV_DATA(dev);
 
-	/* Clear pending interrupts */
-	uart->intclear = UART_RX_IN | UART_TX_IN;
-
 	/* Verify if the callback has been registered */
 	if (data->irq_cb) {
 		data->irq_cb(dev, data->irq_cb_data);