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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 11/11/26 8:47 PM, dmukhin@xxxxxxxx wrote:

[...]

> diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h
> index d13352940c13..15cc4be5ff81 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_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   */
These bits are reserved on a 16550, but this header isn't
16550-specific. A few lines below, the same header defines
UART_FCR_T_TRIG_* as bits 5:4 (TX trigger level on 16650/16654/16C752/
Tegra, see the table there in this header). "reserved; always 0" 
contradicts that. Since only the emulator needs to know what *it* treats 
as reserved, I'd keep these masks local to ns16x50.c. Am I missing 
something?
> +#define UART_FCR_RTB0     0x40    /* receiver trigger bit #0 */
> +#define UART_FCR_RTB1     0x80    /* receiver trigger bit #1 */
> +
These duplicate UART_FCR_TRG4/UART_FCR_TRG8 and UART_FCR_R_TRIG_01/
UART_FCR_R_TRIG_10 right below. The emulator only uses them as the mask
UART_FCR_RTB0 | UART_FCR_RTB1, which is UART_FCR_TRG14
(UART_FCR_R_TRIG_11). Could one of the existing names be used, or a
single mask be introduced, instead of a third set of names for the same
two bits?

>   #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   */
> @@ -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 */
Same concern as for FCR: MCR[5] is the auto flow control enable on the
16750 and on DW_apb_uart (which this header covers, see UART_USR and
UART_IIR_BSY), and MCR[7] is the clock prescaler select on the
TI16C752, whose TCRTLR bit is defined right between the two. Local to
the emulator then?

> +
> +/* Modem Status Register */
> +#define UART_MSR_DCTS     0x01    /* Change in CTS */
> +#define UART_MSR_DDSR     0x02    /* Change in DSR */
> +#define UART_MSR_TERI     0x04    /* Change in RI */
> +#define UART_MSR_DDCD     0x08    /* Change in DCD */
> +#define UART_MSR_CTS      0x10
> +#define UART_MSR_DSR      0x20
> +#define UART_MSR_RI       0x40
> +#define UART_MSR_DCD      0x80
Nit: the last four have no comments, unlike everything else in the
file. Also, TERI is "trailing edge of RI" rather than "change in RI",
which matters because the emulator implements exactly that
(0 -> 1 of RI in ns16x50_io_write8()).

Since ns16x50.c already has UART_MCR_MASK built from the header's
definitions, "reserved" from the emulator's point of view is just
~UART_MCR_MASK (and similarly for FCR with a local mask of ENABLE |
CLRX | CLTX | DMA | TRG14), so neither the RSRVD nor the RTB macros are
needed in the common header.

The ns16550.c conversions look equivalent to me: 0x0A == RTS | OUT2,
and 0x90 == CTS | DCD under UART_MSR_STATUS.


> +#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           */
~ Oleksii