Re: [PATCH v4 4/8] xen/8250-uart: update definitions

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.07.2025 21:22, dmkhn@xxxxxxxxx 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.
> 
> 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@xxxxxxxx>
> ---
> Changes since v3:
> - feedback addressed
> - made use of new UART_MCR_XXX bits in ns16550 driver
> - Link to v3: 
> https://lore.kernel.org/xen-devel/20250103-vuart-ns8250-v3-v1-19-c5d36b31d66c@xxxxxxxx/
> ---
>  xen/drivers/char/ns16550.c  |  6 ++---
>  xen/include/xen/8250-uart.h | 50 ++++++++++++++++++++++++++++++-------
>  2 files changed, 44 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index df7fff7f81df..a899711e2a8b 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -739,9 +739,9 @@ static int __init check_existence(struct ns16550 *uart)
>       * 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..bc11cdc376c9 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)

At the example of this: It having no users here, how are we to know it'll
gain some (and hence be useful)? Adding missing base definitions is imo
fine without immediate users, but for derived ones it's less clear.

Jan