EvalSoC UART stop-bit and watermark setters cannot set control fields

[KafCoppelia]

SDK version

• Repository: Nuclei-Software/nuclei-sdk
• Tag: 0.9.0
• Affected files:
  • SoC/evalsoc/Common/Source/Drivers/evalsoc_uart.c
  • SoC/evalsoc/Common/Include/evalsoc_uart.h

Problem description

The EvalSoC UART functions that update the stop-bit and TX/RX watermark fields do not correctly write a new nonzero field value.

Stop-bit configuration

uart_config_stopbit() masks the shifted stop-bit value with UART_TXCTRL_TXCNT_MASK instead of UART_TXCTRL_NSTOP_MASK:

stopval = (stopbit << UART_TXCTRL_NSTOP_OFS) & UART_TXCTRL_TXCNT_MASK;
uart->TXCTRL &= stopval | (~UART_TXCTRL_TXCNT_MASK);

The stop-bit and TX watermark fields do not overlap, so stopval becomes zero. The function does not update NSTOP and may clear the existing TX watermark.

TX/RX watermark configuration

Both watermark setters use this form:

uart->TXCTRL &= watermark | (~UART_TXCTRL_TXCNT_MASK);
uart->RXCTRL &= watermark | (~UART_RXCTRL_RXCNT_MASK);

Bitwise AND can preserve or clear existing bits, but it cannot change a field bit from zero to one. For example, setting a reset watermark field from 0 to 1 leaves the field unchanged at 0.

This also means the result depends on the previous field value. Updating a multi-bit field requires clearing the complete old field and OR-ing in the new masked value.

Minimal reproduction

The issue can be reproduced without UART hardware because it is caused by the register bit operations themselves:

#include <stdint.h>
#include <stdio.h>

#define UART_TXCTRL_TXCNT_OFS 16U
#define UART_TXCTRL_TXCNT_MASK (0x1FU << UART_TXCTRL_TXCNT_OFS)
#define UART_TXCTRL_NSTOP_OFS 1U
#define UART_TXCTRL_NSTOP_MASK (0x3U << UART_TXCTRL_NSTOP_OFS)
#define UART_RXCTRL_RXCNT_OFS 16U
#define UART_RXCTRL_RXCNT_MASK (0x1FU << UART_RXCTRL_RXCNT_OFS)
#define UART_TXEN 0x1U
#define UART_RXEN 0x1U

typedef struct {
    uint32_t TXCTRL;
    uint32_t RXCTRL;
} UART_TypeDef;

typedef enum {
    UART_STOP_BIT_1 = 0,
    UART_STOP_BIT_2 = 1,
} UART_STOP_BIT;

static void official_stopbit(UART_TypeDef *uart, UART_STOP_BIT stopbit)
{
    uint32_t stopval = stopbit;
    stopval = ((uint32_t)stopbit << UART_TXCTRL_NSTOP_OFS) &
              UART_TXCTRL_TXCNT_MASK;
    uart->TXCTRL &= stopval | (~UART_TXCTRL_TXCNT_MASK);
}

static void official_tx_watermark(UART_TypeDef *uart, uint32_t watermark)
{
    watermark = (watermark << UART_TXCTRL_TXCNT_OFS) &
                UART_TXCTRL_TXCNT_MASK;
    uart->TXCTRL &= watermark | (~UART_TXCTRL_TXCNT_MASK);
}

static void official_rx_watermark(UART_TypeDef *uart, uint32_t watermark)
{
    watermark = (watermark << UART_RXCTRL_RXCNT_OFS) &
                UART_RXCTRL_RXCNT_MASK;
    uart->RXCTRL &= watermark | (~UART_RXCTRL_RXCNT_MASK);
}

int main(void)
{
    UART_TypeDef uart = {.TXCTRL = UART_TXEN, .RXCTRL = UART_RXEN};
    int failures = 0;

    official_stopbit(&uart, UART_STOP_BIT_2);
    printf("stopbit: observed=0x%08x expected=0x%08x\n", uart.TXCTRL,
           UART_TXEN | (1U << UART_TXCTRL_NSTOP_OFS));
    failures += uart.TXCTRL !=
                (UART_TXEN | (1U << UART_TXCTRL_NSTOP_OFS));

    uart.TXCTRL = UART_TXEN;
    official_tx_watermark(&uart, 1U);
    printf("txwm:    observed=0x%08x expected=0x%08x\n", uart.TXCTRL,
           UART_TXEN | (1U << UART_TXCTRL_TXCNT_OFS));
    failures += uart.TXCTRL !=
                (UART_TXEN | (1U << UART_TXCTRL_TXCNT_OFS));

    uart.RXCTRL = UART_RXEN;
    official_rx_watermark(&uart, 1U);
    printf("rxwm:    observed=0x%08x expected=0x%08x\n", uart.RXCTRL,
           UART_RXEN | (1U << UART_RXCTRL_RXCNT_OFS));
    failures += uart.RXCTRL !=
                (UART_RXEN | (1U << UART_RXCTRL_RXCNT_OFS));

    return failures == 0 ? 0 : 1;
}

Compile and run:

$ cc -std=c11 -Wall -Wextra -Werror repro.c -o repro
$ ./repro
stopbit: observed=0x00000001 expected=0x00000003
txwm:    observed=0x00000001 expected=0x00010001
rxwm:    observed=0x00000001 expected=0x00010001
$ echo $?
1

Proposed solution

Use a standard read-modify-write operation: clear the target field, then OR in the new masked value.

 int32_t uart_config_stopbit(UART_TypeDef *uart, UART_STOP_BIT stopbit)
 {
     if (__RARELY(uart == NULL)) {
         return -1;
     }
-    uint32_t stopval = stopbit;
-    stopval = (stopbit << UART_TXCTRL_NSTOP_OFS) & UART_TXCTRL_TXCNT_MASK;
-    uart->TXCTRL &= stopval | (~UART_TXCTRL_TXCNT_MASK);
+    uint32_t stopval =
+        ((uint32_t)stopbit << UART_TXCTRL_NSTOP_OFS) &
+        UART_TXCTRL_NSTOP_MASK;
+    uart->TXCTRL =
+        (uart->TXCTRL & ~UART_TXCTRL_NSTOP_MASK) | stopval;
     return 0;
 }

 int32_t uart_set_tx_watermark(UART_TypeDef *uart, uint32_t watermark)
 {
     if (__RARELY(uart == NULL)) {
         return -1;
     }
     watermark = (watermark << UART_TXCTRL_TXCNT_OFS) &
                 UART_TXCTRL_TXCNT_MASK;
-    uart->TXCTRL &= watermark | (~UART_TXCTRL_TXCNT_MASK);
+    uart->TXCTRL =
+        (uart->TXCTRL & ~UART_TXCTRL_TXCNT_MASK) | watermark;
     return 0;
 }

 int32_t uart_set_rx_watermark(UART_TypeDef *uart, uint32_t watermark)
 {
     if (__RARELY(uart == NULL)) {
         return -1;
     }
     watermark = (watermark << UART_RXCTRL_RXCNT_OFS) &
                 UART_RXCTRL_RXCNT_MASK;
-    uart->RXCTRL &= watermark | (~UART_RXCTRL_RXCNT_MASK);
+    uart->RXCTRL =
+        (uart->RXCTRL & ~UART_RXCTRL_RXCNT_MASK) | watermark;
     return 0;
 }

The proposed changes preserve unrelated control bits and make the resulting field value independent of its previous register state.

[qiujiandong]

Sorry for the late reply. You're right — this is a valid issue. I'm planning to fix it and will include you as a Co-authored-by in the commit to credit your report.

Could you please share your Git email? (The GitHub noreply alias <username>@users.noreply.github.com> works too.)

Thanks!

[KafCoppelia]

Thank you for your prompt reply and confirmation. I’d be happy to provide my email k740677208@gmail.com