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

[dmkhn@xxxxxxxxx]

On Mon, Aug 04, 2025 at 12:23:34PM +0200, Jan Beulich wrote:
> 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.

There're UART_IIR and UART_IER bits which could be re-used.
Will update ns16550 driver.
Thanks.