Correct writes to the UART LCR register (#1347)

Co-authored-by: Luke Wren <wren6991@gmail.com>

src/rp2_common/hardware_uart/include/hardware/uart.h

@@ -131,6 +131,13 @@ typedef enum {
  * Put the UART into a known state, and enable it. Must be called before other
  * functions.
  *
+ * This function always enables the FIFOs, and configures the UART for the
+ * following default line format:
+ *
+ * - 8 data bits
+ * - No parity bit
+ * - One stop bit
+ *
  * \note There is no guarantee that the baudrate requested will be possible, the nearest will be chosen,
  * and this function will return the configured baud rate.
  *
@@ -155,6 +162,17 @@ void uart_deinit(uart_inst_t *uart);
  *
  * Set baud rate as close as possible to requested, and return actual rate selected.
  *
+ * The UART is paused for around two character periods whilst the settings are
+ * changed. Data received during this time may be dropped by the UART.
+ *
+ * Any characters still in the transmit buffer will be sent using the new
+ * updated baud rate. uart_tx_wait_blocking() can be called before this
+ * function to ensure all characters at the old baud rate have been sent
+ * before the rate is changed.
+ *
+ * This function should not be called from an interrupt context, and the UART
+ * interrupt should be disabled before calling this function.
+ *
  * \param uart UART instance. \ref uart0 or \ref uart1
  * \param baudrate Baudrate in Hz
  * \return Actual set baudrate
@@ -177,27 +195,25 @@ static inline void uart_set_hw_flow(uart_inst_t *uart, bool cts, bool rts) {
 /*! \brief Set UART data format
  *  \ingroup hardware_uart
  *
- * Configure the data format (bits etc() for the UART
+ * Configure the data format (bits etc) for the UART.
+ *
+ * The UART is paused for around two character periods whilst the settings are
+ * changed. Data received during this time may be dropped by the UART.
+ *
+ * Any characters still in the transmit buffer will be sent using the new
+ * updated data format. uart_tx_wait_blocking() can be called before this
+ * function to ensure all characters needing the old format have been sent
+ * before the format is changed.
+ *
+ * This function should not be called from an interrupt context, and the UART
+ * interrupt should be disabled before calling this function.
  *
  * \param uart UART instance. \ref uart0 or \ref uart1
  * \param data_bits Number of bits of data. 5..8
  * \param stop_bits Number of stop bits 1..2
  * \param parity Parity option.
  */
-static inline void uart_set_format(uart_inst_t *uart, uint data_bits, uint stop_bits, uart_parity_t parity) {
+void uart_set_format(uart_inst_t *uart, uint data_bits, uint stop_bits, uart_parity_t parity);
-    invalid_params_if(UART, data_bits < 5 || data_bits > 8);
-    invalid_params_if(UART, stop_bits != 1 && stop_bits != 2);
-    invalid_params_if(UART, parity != UART_PARITY_NONE && parity != UART_PARITY_EVEN && parity != UART_PARITY_ODD);
-    hw_write_masked(&uart_get_hw(uart)->lcr_h,
-                   ((data_bits - 5u) << UART_UARTLCR_H_WLEN_LSB) |
-                   ((stop_bits - 1u) << UART_UARTLCR_H_STP2_LSB) |
-                   (bool_to_bit(parity != UART_PARITY_NONE) << UART_UARTLCR_H_PEN_LSB) |
-                   (bool_to_bit(parity == UART_PARITY_EVEN) << UART_UARTLCR_H_EPS_LSB),
-                   UART_UARTLCR_H_WLEN_BITS |
-                   UART_UARTLCR_H_STP2_BITS |
-                   UART_UARTLCR_H_PEN_BITS |
-                   UART_UARTLCR_H_EPS_BITS);
-}
 
 /*! \brief Setup UART interrupts
  *  \ingroup hardware_uart
@@ -242,15 +258,20 @@ static inline bool uart_is_enabled(uart_inst_t *uart) {
 /*! \brief Enable/Disable the FIFOs on specified UART
  *  \ingroup hardware_uart
  *
+ * The UART is paused for around two character periods whilst the settings are
+ * changed. Data received during this time may be dropped by the UART.
+ *
+ * Any characters still in the transmit FIFO will be lost if the FIFO is
+ * disabled. uart_tx_wait_blocking() can be called before this
+ * function to avoid this.
+ *
+ * This function should not be called from an interrupt context, and the UART
+ * interrupt should be disabled when calling this function.
+ *
  * \param uart UART instance. \ref uart0 or \ref uart1
  * \param enabled true to enable FIFO (default), false to disable
  */
-static inline void uart_set_fifo_enabled(uart_inst_t *uart, bool enabled) {
+void uart_set_fifo_enabled(uart_inst_t *uart, bool enabled);
-    hw_write_masked(&uart_get_hw(uart)->lcr_h,
-                   (bool_to_bit(enabled) << UART_UARTLCR_H_FEN_LSB),
-                   UART_UARTLCR_H_FEN_BITS);
-}
-
 
 // ----------------------------------------------------------------------------
 // Generic input/output
@@ -397,12 +418,7 @@ static inline char uart_getc(uart_inst_t *uart) {
  * \param uart UART instance. \ref uart0 or \ref uart1
  * \param en Assert break condition (TX held low) if true. Clear break condition if false.
  */
-static inline void uart_set_break(uart_inst_t *uart, bool en) {
+void uart_set_break(uart_inst_t *uart, bool en);
-    if (en)
-        hw_set_bits(&uart_get_hw(uart)->lcr_h, UART_UARTLCR_H_BRK_BITS);
-    else
-        hw_clear_bits(&uart_get_hw(uart)->lcr_h, UART_UARTLCR_H_BRK_BITS);
-}
 
 /*! \brief Set CR/LF conversion on UART
  *  \ingroup hardware_uart

src/rp2_common/hardware_uart/uart.c

@@ -39,8 +39,9 @@ static inline void uart_unreset(uart_inst_t *uart) {
 uint uart_init(uart_inst_t *uart, uint baudrate) {
     invalid_params_if(UART, uart != uart0 && uart != uart1);
 
-    if (clock_get_hz(clk_peri) == 0)
+    if (clock_get_hz(clk_peri) == 0) {
         return 0;
+    }
 
     uart_reset(uart);
     uart_unreset(uart);
@@ -53,10 +54,10 @@ uint uart_init(uart_inst_t *uart, uint baudrate) {
     uint baud = uart_set_baudrate(uart, baudrate);
     uart_set_format(uart, 8, 1, UART_PARITY_NONE);
 
+    // Enable FIFOs (must be before setting UARTEN, as this is an LCR access)
+    hw_set_bits(&uart_get_hw(uart)->lcr_h, UART_UARTLCR_H_FEN_BITS);
     // Enable the UART, both TX and RX
     uart_get_hw(uart)->cr = UART_UARTCR_UARTEN_BITS | UART_UARTCR_TXE_BITS | UART_UARTCR_RXE_BITS;
-    // Enable FIFOs
-    hw_set_bits(&uart_get_hw(uart)->lcr_h, UART_UARTLCR_H_FEN_BITS);
     // Always enable DREQ signals -- no harm in this if DMA is not listening
     uart_get_hw(uart)->dmacr = UART_UARTDMACR_TXDMAE_BITS | UART_UARTDMACR_RXDMAE_BITS;
 
@@ -69,6 +70,60 @@ void uart_deinit(uart_inst_t *uart) {
     uart_reset(uart);
 }
 
+static uint32_t uart_disable_before_lcr_write(uart_inst_t *uart) {
+    // Notes from PL011 reference manual:
+    //
+    // - Before writing the LCR, if the UART is enabled it needs to be
+    //   disabled and any current TX + RX activity has to be completed
+    //
+    // - There is a BUSY flag which waits for the current TX char, but this is
+    //   OR'd with TX FIFO !FULL, so not usable when FIFOs are enabled and
+    //   potentially nonempty
+    //
+    // - FIFOs can't be set to disabled whilst a character is in progress
+    //   (else "FIFO integrity is not guaranteed")
+    //
+    // Combination of these means there is no general way to halt and poll for
+    // end of TX character, if FIFOs may be enabled. Either way, there is no
+    // way to poll for end of RX character.
+    //
+    // So, insert a 15 Baud period delay before changing the settings.
+    // 15 Baud is comfortably higher than start + max data + parity + stop.
+    // Anything else would require API changes to permit a non-enabled UART
+    // state after init() where settings can be changed safely.
+    uint32_t cr_save = uart_get_hw(uart)->cr;
+
+    if (cr_save & UART_UARTCR_UARTEN_BITS) {
+        hw_clear_bits(&uart_get_hw(uart)->cr,
+            UART_UARTCR_UARTEN_BITS | UART_UARTCR_TXE_BITS | UART_UARTCR_RXE_BITS);
+
+        uint32_t current_ibrd = uart_get_hw(uart)->ibrd;
+        uint32_t current_fbrd = uart_get_hw(uart)->fbrd;
+
+        // Note: Maximise precision here. Show working, the compiler will mop this up.
+        // Create a 16.6 fixed-point fractional division ratio; then scale to 32-bits.
+        uint32_t brdiv_ratio = 64u * current_ibrd + current_fbrd;
+        brdiv_ratio <<= 10;
+        // 3662 is ~(15 * 244.14) where 244.14 is 16e6 / 2^16
+        uint32_t scaled_freq = clock_get_hz(clk_peri) / 3662ul;
+        uint32_t wait_time_us = brdiv_ratio / scaled_freq;
+        busy_wait_us(wait_time_us);
+    }
+
+    return cr_save;
+}
+
+static void uart_write_lcr_bits_masked(uart_inst_t *uart, uint32_t values, uint32_t write_mask) {
+    invalid_params_if(UART, uart != uart0 && uart != uart1);
+
+    // (Potentially) Cleanly handle disabling the UART before touching LCR
+    uint32_t cr_save = uart_disable_before_lcr_write(uart);
+
+    hw_write_masked(&uart_get_hw(uart)->lcr_h, values, write_mask);
+
+    uart_get_hw(uart)->cr = cr_save;
+}
+
 /// \tag::uart_set_baudrate[]
 uint uart_set_baudrate(uart_inst_t *uart, uint baudrate) {
     invalid_params_if(UART, baudrate == 0);
@@ -86,19 +141,56 @@ uint uart_set_baudrate(uart_inst_t *uart, uint baudrate) {
         baud_fbrd = ((baud_rate_div & 0x7f) + 1) / 2;
     }
 
-    // Load PL011's baud divisor registers
     uart_get_hw(uart)->ibrd = baud_ibrd;
     uart_get_hw(uart)->fbrd = baud_fbrd;
 
-    // PL011 needs a (dummy) line control register write to latch in the
+    // PL011 needs a (dummy) LCR_H write to latch in the divisors.
-    // divisors. We don't want to actually change LCR contents here.
+    // We don't want to actually change LCR_H contents here.
-    hw_set_bits(&uart_get_hw(uart)->lcr_h, 0);
+    uart_write_lcr_bits_masked(uart, 0, 0);
 
     // See datasheet
     return (4 * clock_get_hz(clk_peri)) / (64 * baud_ibrd + baud_fbrd);
 }
 /// \end::uart_set_baudrate[]
 
+void uart_set_format(uart_inst_t *uart, uint data_bits, uint stop_bits, uart_parity_t parity) {
+    invalid_params_if(UART, data_bits < 5 || data_bits > 8);
+    invalid_params_if(UART, stop_bits != 1 && stop_bits != 2);
+    invalid_params_if(UART, parity != UART_PARITY_NONE && parity != UART_PARITY_EVEN && parity != UART_PARITY_ODD);
+
+    uart_write_lcr_bits_masked(uart,
+        ((data_bits - 5u) << UART_UARTLCR_H_WLEN_LSB) |
+        ((stop_bits - 1u) << UART_UARTLCR_H_STP2_LSB) |
+        (bool_to_bit(parity != UART_PARITY_NONE) << UART_UARTLCR_H_PEN_LSB) |
+        (bool_to_bit(parity == UART_PARITY_EVEN) << UART_UARTLCR_H_EPS_LSB),
+        UART_UARTLCR_H_WLEN_BITS |
+        UART_UARTLCR_H_STP2_BITS |
+        UART_UARTLCR_H_PEN_BITS |
+        UART_UARTLCR_H_EPS_BITS);
+}
+
+void uart_set_fifo_enabled(uart_inst_t *uart, bool enabled) {
+
+    uint32_t lcr_h_fen_bits = 0;
+
+    if (enabled) {
+        lcr_h_fen_bits = UART_UARTLCR_H_FEN_BITS;
+    }
+
+    uart_write_lcr_bits_masked(uart, lcr_h_fen_bits, UART_UARTLCR_H_FEN_BITS);
+}
+
+void uart_set_break(uart_inst_t *uart, bool en) {
+
+    uint32_t lcr_h_brk_bits = 0;
+
+    if (en) {
+        lcr_h_brk_bits = UART_UARTLCR_H_BRK_BITS;
+    }
+
+    uart_write_lcr_bits_masked(uart, lcr_h_brk_bits, UART_UARTLCR_H_BRK_BITS);
+}
+
 void uart_set_translate_crlf(uart_inst_t *uart, bool crlf) {
 #if PICO_UART_ENABLE_CRLF_SUPPORT
     uart_char_to_line_feed[uart_get_index(uart)] = crlf ? '\n' : 0x100;
@@ -110,7 +202,9 @@ void uart_set_translate_crlf(uart_inst_t *uart, bool crlf) {
 bool uart_is_readable_within_us(uart_inst_t *uart, uint32_t us) {
     uint32_t t = time_us_32();
     do {
-        if (uart_is_readable(uart)) return true;
+        if (uart_is_readable(uart)) {
+             return true;
+        }
     } while ((time_us_32() - t) <= us);
     return false;
 }