[PATCH 10/10] serial: msm: Use single character mode
Neil Armstrong
neil.armstrong at linaro.org
Tue Jun 24 11:41:56 CEST 2025
On 24/06/2025 10:45, Stephan Gerhold wrote:
> The UART DM controller supports different channel data packing modes,
> either the 4-character packing mode (where 32-bit are read/written at once)
> or the single-character mode (where only a single character is read/written
> at a time). The 4-character mode can be more efficient, but the
> single-character mode is much easier to implement.
>
> At the moment, serial_msm uses the 4-character mode. Since the
> dm_serial_ops operate on one character at the time, the code goes through
> quite some hoops in order to break this down to single characters. This
> code is prone to race conditions (e.g. priv->chars_cnt is read from the
> registers, then a command is issued, what if another char came in
> inbetween?). It also seems to cause another subtle issue with autoboot:
>
> Unlike the previous autoboot failures that happened when UART was
> disconnected, this problem occurs when UART is connected and open in a
> terminal: For EFI boot, the console size is queried in efi_console.c
> query_console_serial() by sending an ANSI escape code via UART. For some
> reason, with the current driver we get yet another 0x00 byte (UART break
> event?) when reading the reply from serial input. Because of that, reading
> the console size fails in efi_console.c, the actual reply remains in the
> UART buffer, and later the boot flow aborts because it detects input after
> printing a prompt.
>
> Rather than trying to fix the issue in the current complicated approach,
> switch the driver to use the single-character mode. This is simple and
> straightforward to implement without race conditions:
>
> - We write one character at a time to UARTDM_TF, as long as the TX FIFO
> has space available (TX_READY). To flush the console before starting
> Linux, we wait for TX_EMPTY.
>
> - We read one character at a time from UARTDM_RF and strip off the
> additional error information (assuming there is something in the
> RX FIFO, as indicated by RX_READY).
>
> In this mode, querying the serial console size works and autoboot is no
> longer interrupted. The overall code is also much shorter.
>
> Signed-off-by: Stephan Gerhold <stephan.gerhold at linaro.org>
> ---
> drivers/serial/serial_msm.c | 102 +++++++-------------------------------------
> 1 file changed, 16 insertions(+), 86 deletions(-)
>
> diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c
> index 2c08a84b02773663f3c3a11eddaaa7a8bab2b4a1..18d15b7b15b3aeb2e8fd1da1aa49967d55c943ab 100644
> --- a/drivers/serial/serial_msm.c
> +++ b/drivers/serial/serial_msm.c
> @@ -21,13 +21,9 @@
>
> /* Serial registers - this driver works in uartdm mode*/
>
> -#define UARTDM_DMRX 0x34 /* Max RX transfer length */
> -#define UARTDM_DMEN 0x3C /* DMA/data-packing mode */
> -#define UARTDM_NCF_TX 0x40 /* Number of chars to TX */
> +#define UARTDM_DMEN 0x3C /* DMA/data-packing mode */
> +#define UARTDM_DMEN_TXRX_SC_ENABLE (BIT(4) | BIT(5))
>
> -#define UARTDM_RXFS 0x50 /* RX channel status register */
> -#define UARTDM_RXFS_BUF_SHIFT 0x7 /* Number of bytes in the packing buffer */
> -#define UARTDM_RXFS_BUF_MASK 0x7
> #define UARTDM_MR1 0x00
> #define UARTDM_MR1_RX_RDY_CTL BIT(7)
> #define UARTDM_MR2 0x04
> @@ -45,118 +41,56 @@
> #define UARTDM_CSR 0xA0
>
> #define UARTDM_SR 0xA4 /* Status register */
> -#define UARTDM_SR_RX_READY (1 << 0) /* Word is the receiver FIFO */
> +#define UARTDM_SR_RX_READY (1 << 0) /* Receiver FIFO has data */
> +#define UARTDM_SR_TX_READY (1 << 2) /* Transmitter FIFO has space */
> #define UARTDM_SR_TX_EMPTY (1 << 3) /* Transmitter underrun */
> -#define UARTDM_SR_UART_OVERRUN (1 << 4) /* Receive overrun */
>
> #define UARTDM_CR 0xA8 /* Command register */
> #define UARTDM_CR_RX_ENABLE (1 << 0) /* Enable receiver */
> #define UARTDM_CR_TX_ENABLE (1 << 2) /* Enable transmitter */
> #define UARTDM_CR_CMD_RESET_RX (1 << 4) /* Reset receiver */
> #define UARTDM_CR_CMD_RESET_TX (2 << 4) /* Reset transmitter */
> -#define UARTDM_CR_CMD_RESET_ERR (3 << 4) /* Clear overrun error */
> -#define UARTDM_CR_CMD_RESET_STALE_INT (8 << 4) /* Clears stale irq */
> -#define UARTDM_CR_CMD_RESET_TX_READY (3 << 8) /* Clears TX Ready irq*/
> -#define UARTDM_CR_CMD_FORCE_STALE (4 << 8) /* Causes stale event */
> -#define UARTDM_CR_CMD_STALE_EVENT_DISABLE (6 << 8) /* Disable stale event */
> -
> -#define UARTDM_IMR 0xB0 /* Interrupt mask register */
> -#define UARTDM_ISR 0xB4 /* Interrupt status register */
> -#define UARTDM_ISR_TX_READY 0x80 /* TX FIFO empty */
>
> #define UARTDM_TF 0x100 /* UART Transmit FIFO register */
> #define UARTDM_RF 0x140 /* UART Receive FIFO register */
> +#define UARTDM_RF_CHAR 0xff /* higher bits contain error information */
>
> DECLARE_GLOBAL_DATA_PTR;
>
> struct msm_serial_data {
> phys_addr_t base;
> - unsigned chars_cnt; /* number of buffered chars */
> - uint32_t chars_buf; /* buffered chars */
> uint32_t clk_rate; /* core clock rate */
> };
>
> -static int msm_serial_fetch(struct udevice *dev)
> -{
> - struct msm_serial_data *priv = dev_get_priv(dev);
> - unsigned sr;
> -
> - if (priv->chars_cnt)
> - return priv->chars_cnt;
> -
> - /* Clear error in case of buffer overrun */
> - if (readl(priv->base + UARTDM_SR) & UARTDM_SR_UART_OVERRUN)
> - writel(UARTDM_CR_CMD_RESET_ERR, priv->base + UARTDM_CR);
> -
> - /* We need to fetch new character */
> - sr = readl(priv->base + UARTDM_SR);
> -
> - if (sr & UARTDM_SR_RX_READY) {
> - /* There are at least 4 bytes in fifo */
> - priv->chars_buf = readl(priv->base + UARTDM_RF);
> - priv->chars_cnt = 4;
> - } else {
> - /* Check if there is anything in fifo */
> - priv->chars_cnt = readl(priv->base + UARTDM_RXFS);
> - /* Extract number of characters in UART packing buffer*/
> - priv->chars_cnt = (priv->chars_cnt >>
> - UARTDM_RXFS_BUF_SHIFT) &
> - UARTDM_RXFS_BUF_MASK;
> - if (!priv->chars_cnt)
> - return 0;
> -
> - /* There is at least one charcter, move it to fifo */
> - writel(UARTDM_CR_CMD_FORCE_STALE,
> - priv->base + UARTDM_CR);
> -
> - priv->chars_buf = readl(priv->base + UARTDM_RF);
> - writel(UARTDM_CR_CMD_RESET_STALE_INT,
> - priv->base + UARTDM_CR);
> - writel(0x7, priv->base + UARTDM_DMRX);
> - }
> -
> - return priv->chars_cnt;
> -}
> -
> static int msm_serial_getc(struct udevice *dev)
> {
> struct msm_serial_data *priv = dev_get_priv(dev);
> - char c;
>
> - if (!msm_serial_fetch(dev))
> + if (!(readl(priv->base + UARTDM_SR) & UARTDM_SR_RX_READY))
> return -EAGAIN;
>
> - c = priv->chars_buf & 0xFF;
> - priv->chars_buf >>= 8;
> - priv->chars_cnt--;
> -
> - return c;
> + return readl(priv->base + UARTDM_RF) & UARTDM_RF_CHAR;
> }
>
> static int msm_serial_putc(struct udevice *dev, const char ch)
> {
> struct msm_serial_data *priv = dev_get_priv(dev);
>
> - if (!(readl(priv->base + UARTDM_SR) & UARTDM_SR_TX_EMPTY) &&
> - !(readl(priv->base + UARTDM_ISR) & UARTDM_ISR_TX_READY))
> + if (!(readl(priv->base + UARTDM_SR) & UARTDM_SR_TX_READY))
> return -EAGAIN;
>
> - writel(UARTDM_CR_CMD_RESET_TX_READY, priv->base + UARTDM_CR);
> -
> - writel(1, priv->base + UARTDM_NCF_TX);
> writel(ch, priv->base + UARTDM_TF);
> -
> return 0;
> }
>
> static int msm_serial_pending(struct udevice *dev, bool input)
> {
> - if (input) {
> - if (msm_serial_fetch(dev))
> - return 1;
> - }
> + struct msm_serial_data *priv = dev_get_priv(dev);
>
> - return 0;
> + if (input)
> + return !!(readl(priv->base + UARTDM_SR) & UARTDM_SR_RX_READY);
> + else
> + return !(readl(priv->base + UARTDM_SR) & UARTDM_SR_TX_EMPTY);
> }
>
> static const struct dm_serial_ops msm_serial_ops = {
> @@ -222,8 +156,8 @@ static void uart_dm_init(struct msm_serial_data *priv)
> writel(UARTDM_MR1_RX_RDY_CTL, priv->base + UARTDM_MR1);
> writel(UARTDM_MR2_8_N_1_MODE, priv->base + UARTDM_MR2);
>
> - /* Make sure BAM/single character mode is disabled */
> - writel(0x0, priv->base + UARTDM_DMEN);
> + /* Enable single character mode */
> + writel(UARTDM_DMEN_TXRX_SC_ENABLE, priv->base + UARTDM_DMEN);
>
> writel(UARTDM_CR_CMD_RESET_RX, priv->base + UARTDM_CR);
> writel(UARTDM_CR_CMD_RESET_TX, priv->base + UARTDM_CR);
> @@ -320,13 +254,9 @@ static inline void _debug_uart_putc(int ch)
> {
> struct msm_serial_data *priv = &init_serial_data;
>
> - while (!(readl(priv->base + UARTDM_SR) & UARTDM_SR_TX_EMPTY) &&
> - !(readl(priv->base + UARTDM_ISR) & UARTDM_ISR_TX_READY))
> + while (!(readl(priv->base + UARTDM_SR) & UARTDM_SR_TX_READY))
> ;
>
> - writel(UARTDM_CR_CMD_RESET_TX_READY, priv->base + UARTDM_CR);
> -
> - writel(1, priv->base + UARTDM_NCF_TX);
> writel(ch, priv->base + UARTDM_TF);
> }
>
>
Reviewed-by: Neil Armstrong <neil.armstrong at linaro.org>
More information about the U-Boot
mailing list