Hi Denis,
On Sat, Sep 6, 2025 at 2:27 AM <[email protected]> wrote:
>
> From: Denis Mukhin <[email protected]>
>
> Added missing definitions needed for NS16550 UART emulator.
>
> Newly introduced MSR definitions re-used in the existing ns16550 driver.
>
> Also, corrected FCR DMA definition bit#3 (0x08) as per:
> https://www.ti.com/lit/ds/symlink/tl16c550c.pdf
> See "7.7.2 FIFO Control Register (FCR)".
>
> Signed-off-by: Denis Mukhin <[email protected]>
> ---
> Changes since v5:
> - fixed commentaries
> - Link to v5:
> https://lore.kernel.org/xen-devel/[email protected]/
> ---
> xen/drivers/char/ns16550.c | 16 ++++++------
> xen/include/xen/8250-uart.h | 50 ++++++++++++++++++++++++++++++-------
> 2 files changed, 49 insertions(+), 17 deletions(-)
>
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index df7fff7f81df..0e80fadbb894 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -388,7 +388,7 @@ static void __init cf_check ns16550_init_preirq(struct
> serial_port *port)
>
> /* Check this really is a 16550+. Otherwise we have no FIFOs. */
> if ( uart->fifo_size <= 1 &&
> - ((ns_read_reg(uart, UART_IIR) & 0xc0) == 0xc0) &&
> + ((ns_read_reg(uart, UART_IIR) & UART_IIR_FE) == UART_IIR_FE) &&
> ((ns_read_reg(uart, UART_FCR) & UART_FCR_TRG14) == UART_FCR_TRG14) )
> uart->fifo_size = 16;
> }
> @@ -728,20 +728,20 @@ static int __init check_existence(struct ns16550 *uart)
> * Mask out IER[7:4] bits for test as some UARTs (e.g. TL
> * 16C754B) allow only to modify them if an EFR bit is set.
> */
> - scratch2 = ns_read_reg(uart, UART_IER) & 0x0f;
> - ns_write_reg(uart,UART_IER, 0x0F);
> - scratch3 = ns_read_reg(uart, UART_IER) & 0x0f;
> + scratch2 = ns_read_reg(uart, UART_IER) & UART_IER_MASK;
> + ns_write_reg(uart, UART_IER, UART_IER_MASK);
> + scratch3 = ns_read_reg(uart, UART_IER) & UART_IER_MASK;
> ns_write_reg(uart, UART_IER, scratch);
> - if ( (scratch2 != 0) || (scratch3 != 0x0F) )
> + if ( (scratch2 != 0) || (scratch3 != UART_IER_MASK) )
> return 0;
>
> /*
> * Check to see if a UART is really there.
> * Use loopback test mode.
> */
> - ns_write_reg(uart, UART_MCR, UART_MCR_LOOP | 0x0A);
> - status = ns_read_reg(uart, UART_MSR) & 0xF0;
> - return (status == 0x90);
> + ns_write_reg(uart, UART_MCR, UART_MCR_LOOP | UART_MCR_RTS |
> UART_MCR_OUT2);
> + status = ns_read_reg(uart, UART_MSR) & UART_MSR_STATUS;
> + return (status == (UART_MSR_CTS | UART_MSR_DCD));
> }
>
> #ifdef CONFIG_HAS_PCI
> diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h
> index d13352940c13..bbbffb14d320 100644
> --- a/xen/include/xen/8250-uart.h
> +++ b/xen/include/xen/8250-uart.h
> @@ -32,6 +32,7 @@
> #define UART_MCR 0x04 /* Modem control */
> #define UART_LSR 0x05 /* line status */
> #define UART_MSR 0x06 /* Modem status */
> +#define UART_SCR 0x07 /* Scratch pad */
> #define UART_USR 0x1f /* Status register (DW) */
> #define UART_DLL 0x00 /* divisor latch (ls) (DLAB=1) */
> #define UART_DLM 0x01 /* divisor latch (ms) (DLAB=1) */
> @@ -42,6 +43,8 @@
> #define UART_IER_ETHREI 0x02 /* tx reg. empty */
> #define UART_IER_ELSI 0x04 /* rx line status */
> #define UART_IER_EMSI 0x08 /* MODEM status */
> +#define UART_IER_MASK \
> + (UART_IER_ERDAI | UART_IER_ETHREI | UART_IER_ELSI | UART_IER_EMSI)
>
> /* Interrupt Identification Register */
> #define UART_IIR_NOINT 0x01 /* no interrupt pending */
> @@ -51,12 +54,19 @@
> #define UART_IIR_THR 0x02 /* - tx reg. empty */
> #define UART_IIR_MSI 0x00 /* - MODEM status */
> #define UART_IIR_BSY 0x07 /* - busy detect (DW) */
> +#define UART_IIR_FE 0xc0 /* FIFO enabled (2 bits) */
>
> /* FIFO Control Register */
> -#define UART_FCR_ENABLE 0x01 /* enable FIFO */
> -#define UART_FCR_CLRX 0x02 /* clear Rx FIFO */
> -#define UART_FCR_CLTX 0x04 /* clear Tx FIFO */
> -#define UART_FCR_DMA 0x10 /* enter DMA mode */
> +#define UART_FCR_ENABLE BIT(0, U) /* enable FIFO */
> +#define UART_FCR_CLRX BIT(1, U) /* clear Rx FIFO */
> +#define UART_FCR_CLTX BIT(2, U) /* clear Tx FIFO */
> +#define UART_FCR_DMA BIT(3, U) /* enter DMA mode */
> +#define UART_FCR_RESERVED0 BIT(4, U) /* reserved; always 0 */
> +#define UART_FCR_RESERVED1 BIT(5, U) /* reserved; always 0 */
> +#define UART_FCR_RTB0 BIT(6, U) /* receiver trigger bit #0 */
> +#define UART_FCR_RTB1 BIT(7, U) /* receiver trigger bit #1 */
> +#define UART_FCR_TRG_MASK (UART_FCR_RTB0 | UART_FCR_RTB1)
Thanks for the patch. I noticed that in this changeset some bit
definitions (e.g. UART_FCR_*) were rewritten using the BIT(n, U)
macro, while others (e.g. UART_IER_* and rest of UART_FCR_*) are
still left as plain hex values (0x01, 0x02, etc.), even though they
are also powers of two.
Could you clarify the reasoning behind this choice? From a reader’s
perspective it looks inconsistent. Would it make sense to either:
- update all of them to use BIT() for consistency, or
- keep the existing style unchanged in this patch and move a full
conversion to BIT() into a separate cleanup patch?
This would make the codebase easier to follow.
> +
> #define UART_FCR_TRG1 0x00 /* Rx FIFO trig lev 1 */
> #define UART_FCR_TRG4 0x40 /* Rx FIFO trig lev 4 */
> #define UART_FCR_TRG8 0x80 /* Rx FIFO trig lev 8 */
> @@ -96,11 +106,32 @@
> #define UART_LCR_CONF_MODE_B 0xBF /* Configuration mode B */
>
> /* Modem Control Register */
> -#define UART_MCR_DTR 0x01 /* Data Terminal Ready */
> -#define UART_MCR_RTS 0x02 /* Request to Send */
> -#define UART_MCR_OUT2 0x08 /* OUT2: interrupt mask */
> -#define UART_MCR_LOOP 0x10 /* Enable loopback test mode */
> -#define UART_MCR_TCRTLR 0x40 /* Access TCR/TLR (TI16C752, EFR[4]=1) */
> +#define UART_MCR_DTR BIT(0, U) /* Data Terminal Ready */
> +#define UART_MCR_RTS BIT(1, U) /* Request to Send */
> +#define UART_MCR_OUT1 BIT(2, U) /* Output #1 */
> +#define UART_MCR_OUT2 BIT(3, U) /* Output #2 */
> +#define UART_MCR_LOOP BIT(4, U) /* Enable loopback test mode */
> +#define UART_MCR_RESERVED0 BIT(5, U) /* Reserved #0 */
> +#define UART_MCR_TCRTLR BIT(6, U) /* Access TCR/TLR (TI16C752,
> EFR[4]=1) */
> +#define UART_MCR_RESERVED1 BIT(7, U) /* Reserved #1 */
> +#define UART_MCR_MASK \
> + (UART_MCR_DTR | UART_MCR_RTS | \
> + UART_MCR_OUT1 | UART_MCR_OUT2 | \
> + UART_MCR_LOOP | UART_MCR_TCRTLR)
> +
> +/* Modem Status Register */
> +#define UART_MSR_DCTS BIT(0, U) /* Change in CTS */
> +#define UART_MSR_DDSR BIT(1, U) /* Change in DSR */
> +#define UART_MSR_TERI BIT(2, U) /* Change in RI */
> +#define UART_MSR_DDCD BIT(3, U) /* Change in DCD */
> +#define UART_MSR_CTS BIT(4, U)
> +#define UART_MSR_DSR BIT(5, U)
> +#define UART_MSR_RI BIT(6, U)
> +#define UART_MSR_DCD BIT(7, U)
> +#define UART_MSR_CHANGE \
> + (UART_MSR_DCTS | UART_MSR_DDSR | UART_MSR_TERI | UART_MSR_DDCD)
> +#define UART_MSR_STATUS \
> + (UART_MSR_CTS | UART_MSR_DSR | UART_MSR_RI | UART_MSR_DCD)
>
> /* Line Status Register */
> #define UART_LSR_DR 0x01 /* Data ready */
> @@ -111,6 +142,7 @@
> #define UART_LSR_THRE 0x20 /* Xmit hold reg empty */
> #define UART_LSR_TEMT 0x40 /* Xmitter empty */
> #define UART_LSR_ERR 0x80 /* Error */
> +#define UART_LSR_MASK (UART_LSR_OE | UART_LSR_BI)
>
> /* These parity settings can be ORed directly into the LCR. */
> #define UART_PARITY_NONE (0<<3)
> --
> 2.51.0
>
>
Best regards,
Mykola
