Re: [PATCH v8 02/26] xen/8250-uart: update definitions

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.11.2026 20:47, dmukhin@xxxxxxxx wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx>
> 
> Added missing definitions needed for NS16550 UART emulator.
> 
> Newly introduced MSR definitions re-used in the existing ns16550 driver.

Why the restriction to MSR here? You also do this for IIR and IER. Simply
drop "MSR" from here?

> 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)".

Hmm, Ralph Brown's (once famous) Interrupt List doesn't describe the bit
like this. At the same time a different doc than yours that I have matches
what you say. So fine perhaps.

> --- 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_ALL_INTR \
> +    (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,18 @@
>  #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_DMA      0x08    /* enter DMA mode       */
> +#define UART_FCR_RSRVD0   0x10    /* reserved; always 0   */
> +#define UART_FCR_RSRVD1   0x20    /* reserved; always 0   */

Do you really need these two and ...

> @@ -98,9 +107,26 @@
>  /* 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_OUT1     0x04    /* Output #1 */
> +#define UART_MCR_OUT2     0x08    /* Output #2 */
>  #define UART_MCR_LOOP     0x10    /* Enable loopback test mode */
> +#define UART_MCR_RSRVD0   0x20    /* Reserved #0 */
>  #define UART_MCR_TCRTLR   0x40    /* Access TCR/TLR (TI16C752, EFR[4]=1) */
> +#define UART_MCR_RSRVD1   0x80    /* Reserved #1 */

... these two reserved bits as separate masks each? The 0/1 nomenclature
won't make obvious (at use sites) which of each (respectively) is meant.

Jan