From: Denis Mukhin <dmukhin@ford.com>
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 <dmukhin@ford.com>
---
Changes since v5:
- fixed commentaries
- Link to v5: https://lore.kernel.org/xen-devel/20250828235409.2835815-3-dmukhin@ford.com/
---
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)
+
#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.0Hi Denis,
On Sat, Sep 6, 2025 at 2:27 AM <dmukhin@xen.org> wrote:
>
> From: Denis Mukhin <dmukhin@ford.com>
>
> 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 <dmukhin@ford.com>
> ---
> Changes since v5:
> - fixed commentaries
> - Link to v5: https://lore.kernel.org/xen-devel/20250828235409.2835815-3-dmukhin@ford.com/
> ---
> 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
On Sat, Sep 06, 2025 at 05:57:02PM +0300, Mykola Kvach wrote: > Hi Denis, > > On Sat, Sep 6, 2025 at 2:27 AM <dmukhin@xen.org> wrote: [..] > > /* 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. I find BIT() notation is more readable than raw bitmasks. But I agree that definitions should be consistently updated. Will update to the raw masks instead of BIT() in v7.
On Fri, 5 Sep 2025, dmukhin@xen.org wrote: > From: Denis Mukhin <dmukhin@ford.com> > > 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 <dmukhin@ford.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
© 2016 - 2025 Red Hat, Inc.