Re: [PATCH v7 02/16] xen/8250-uart: update definitions

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Denis,

Thank you for the updates from the previous version of patch series,

On Tue, Sep 9, 2025 at 1:47 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 08.09.2025 23:11, dmukhin@xxxxxxx wrote:
> > --- 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)
>
> Here, aiui, ..._MASK covers all known bits. No #define-s for reserved
> ones.

I think we can follow the Linux kernel style here [1]. For example:

#define UART_IER_ALL_INTR (UART_IER_MSI | \
        UART_IER_RLSI | \
        UART_IER_THRI | \
        UART_IER_RDI)

This way we avoid using the *_MASK suffix, which could be confusing,
and clearly indicate that these are all valid interrupt bits.

>
> > @@ -51,12 +54,19 @@
> >  #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       */
>
> Question is whether we can actually use the source you indicate as
> reference. TL16C550C may already be too different from what a "standard"
> 16550 is (where admittedly it also looks unclear what "standard" would be,
> as I'm unaware of a "canonical" spec).
>
> The source I'm looking at says something entirely different. Maybe we're
> better off simply omitting this #define?
>
> > +#define UART_FCR_RSRVD0   0x10    /* reserved; always 0   */
> > +#define UART_FCR_RSRVD1   0x20    /* reserved; always 0   */
> > +#define UART_FCR_RTB0     0x40    /* receiver trigger bit #0 */
> > +#define UART_FCR_RTB1     0x80    /* receiver trigger bit #1 */
> > +#define UART_FCR_TRG_MASK (UART_FCR_RTB0 | UART_FCR_RTB1)
>
> Continuing from the top comment - here, with the TRG infix, the scope is
> clear, too.
>
> > @@ -98,9 +108,30 @@
> >  /* 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 */
> > +#define UART_MCR_MASK \
> > +    (UART_MCR_DTR | UART_MCR_RTS | \
> > +     UART_MCR_OUT1 | UART_MCR_OUT2 | \
> > +     UART_MCR_LOOP | UART_MCR_TCRTLR)
>
> Here it's again all non-reserved bits. Yet why do we need #define-s for
> the two reserved ones here? (Same question for FCR, even if there's no
> UART_FCR_MASK.)
>
> > +/* 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
> > +#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)
>
> Here it's properly two subsets.
>
> > @@ -111,6 +142,7 @@
> >  #define UART_LSR_THRE     0x20    /* Xmit hold reg empty  */
> >  #define UART_LSR_TEMT     0x40    /* Xmitter empty        */
> >  #define UART_LSR_ERR      0x80    /* Error                */
> > +#define UART_LSR_MASK     (UART_LSR_OE | UART_LSR_BI)
>
> But what's the deal here? Why would only two of the bits be covered?
>
> Jan
>

[1] https://elixir.bootlin.com/linux/v6.16.5/source/include/linux/serial.h#L15

Best regards,
Mykola