On 06.12.2024 05:41, Denis Mukhin via B4 Relay wrote: > --- a/xen/include/xen/8250-uart.h > +++ b/xen/include/xen/8250-uart.h > @@ -32,16 +32,22 @@ > #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) */ > #define UART_XR_EFR 0x09 /* Enhanced function register (Exar) */ > > +/* ns8250 emulator: range of emulated registers [0..UART_MAX-1] */ > +#define UART_MAX (UART_SCR + 1)
There are two issues here: "max" means "highest within range", yet you define it as "first invalid", i.e. something we'd normally call "_NR" or "_NUM". And then, as the comment says, this is a limit the emulation is going to expose, not something generally applicable to UARTs of this kind. Hence the UART_ prefix alone isn't quite correct either. > @@ -51,12 +57,21 @@ > #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_FE0 BIT(6, U) /* FIFO enable #0 */ > +#define UART_IIR_FE1 BIT(7, U) /* FIFO enable #1 */ > +#define UART_IIR_FE_MASK (UART_IIR_FE0 | UART_IIR_FE1) Much like BSY is a 3-bit field, aiui this is a 2-bit one. > /* 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) Much like the top two bits here are, and - as Roger has said - the reserved bits probably also should be. > @@ -64,17 +79,17 @@ > > /* > * Note: The FIFO trigger levels are chip specific: > - * RX:76 = 00 01 10 11 TX:54 = 00 01 10 11 > - * PC16550D: 1 4 8 14 xx xx xx xx > - * TI16C550A: 1 4 8 14 xx xx xx xx > - * TI16C550C: 1 4 8 14 xx xx xx xx > - * ST16C550: 1 4 8 14 xx xx xx xx > - * ST16C650: 8 16 24 28 16 8 24 30 PORT_16650V2 > - * NS16C552: 1 4 8 14 xx xx xx xx > - * ST16C654: 8 16 56 60 8 16 32 56 PORT_16654 > - * TI16C750: 1 16 32 56 xx xx xx xx PORT_16750 > - * TI16C752: 8 16 56 60 8 16 32 56 > - * Tegra: 1 4 8 14 16 8 4 1 PORT_TEGRA > + * RX:76 = 00 01 10 11 TX:54 = 00 01 10 11 > + * PC16550D: 1 4 8 14 xx xx xx xx > + * TI16C550A: 1 4 8 14 xx xx xx xx > + * TI16C550C: 1 4 8 14 xx xx xx xx > + * ST16C550: 1 4 8 14 xx xx xx xx > + * ST16C650: 8 16 24 28 16 8 24 30 PORT_16650V2 > + * NS16C552: 1 4 8 14 xx xx xx xx > + * ST16C654: 8 16 56 60 8 16 32 56 PORT_16654 > + * TI16C750: 1 16 32 56 xx xx xx xx PORT_16750 > + * TI16C752: 8 16 56 60 8 16 32 56 > + * Tegra: 1 4 8 14 16 8 4 1 PORT_TEGRA > */ While perhaps okay, the adjustment of this table still looks unrelated. It wants at least mentioning in the description, to clarify it's an intentional change (as opposed to e.g. being an effect of how your editor is configured). > @@ -96,11 +111,31 @@ > #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) /* OUT1: interrupt mask */ > +#define UART_MCR_OUT2 BIT(3, U) /* OUT2: interrupt mask */ > +#define UART_MCR_LOOP BIT(4, U) /* Enable loopback test mode */ > +#define UART_MCR_RESERVED0 BIT(5, U) /* Reserved #0 */ > +#define UART_MCR_RESERVED1 BIT(6, U) /* Reserved #1 */ > +#define UART_MCR_TCRTLR BIT(6, U) /* Access TCR/TLR (TI16C752, > EFR[4]=1) */ > +#define UART_MCR_RESERVED2 BIT(7, U) /* Reserved #2 */ > +#define UART_MCR_MASK \ > + (UART_MCR_DTR | UART_MCR_RTS | \ > + UART_MCR_OUT1 | UART_MCR_OUT2 | \ > + UART_MCR_LOOP) > + > +/* 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 CTS */ > +#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) As you introduce these constants, I think you also want to switch the sole MSR read we have to actually use them. Jan
