Re: [PATCH v2 28/35] xen/8250-uart: add missing definitions

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Thursday, December 12th, 2024 at 6:29 AM, Roger Pau Monné 
<roger.pau@xxxxxxxxxx> wrote:

>
>
> On Thu, Dec 05, 2024 at 08:41:58PM -0800, Denis Mukhin via B4 Relay wrote:
>
> > From: Denis Mukhin dmukhin@xxxxxxxx
> >
> > Added missing definitions needed for NS8250 UART emulator.
> >
> > Signed-off-by: Denis Mukhin dmukhin@xxxxxxxx
> > ---
> > xen/include/xen/8250-uart.h | 81 
> > +++++++++++++++++++++++++++++++++------------
> > 1 file changed, 60 insertions(+), 21 deletions(-)
> >
> > diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h
> > index 
> > d13352940c13c50bac17d4cdf2f3bf584380776a..4a69b2b225140efda287bdf96fa0caa4aa70074f
> >  100644
> > --- a/xen/include/xen/8250-uart.h
> > +++ b/xen/include/xen/8250-uart.h
> > @@ -32,16 +32,22 @@
> > #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) /
> > #define UART_XR_EFR 0x09 / Enhanced function register (Exar) */
> >
> > +/* ns8250 emulator: range of emulated registers [0..UART_MAX-1] /
> > +#define UART_MAX (UART_SCR + 1)
> > +
> > / Interrupt Enable Register /
> > #define UART_IER_ERDAI 0x01 / rx data recv'd /
> > #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)
> >
> > /* Interrupt Identification Register /
> > #define UART_IIR_NOINT 0x01 / no interrupt pending /
> > @@ -51,12 +57,21 @@
> > #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_FE0 BIT(6, U) / FIFO enable #0 /
> > +#define UART_IIR_FE1 BIT(7, U) / FIFO enable #1 */
> > +#define UART_IIR_FE_MASK (UART_IIR_FE0 | UART_IIR_FE1)
> >
> > /* 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_ENABLE BIT(0, U) / enable FIFO /
> > +#define UART_FCR_CLRX BIT(1, U) / clear Rx FIFO /
> > +#define UART_FCR_CLTX BIT(2, U) / clear Tx FIFO /
> > +#define UART_FCR_DMA BIT(3, U) / enter DMA mode /
> > +#define UART_FCR_RESERVED0 BIT(4, U) / reserved; always 0 /
> > +#define UART_FCR_RESERVED1 BIT(5, U) / reserved; always 0 */
>
>
> We don't usually define reserved bits I think, as I assume those won't
> be used by the code?

I found examples of reserved bits being defined and not used in the
code base, e.g. PCI_PM_CAP_RESERVED.

IMO marking reserved bits in the code will immediately explain
readers the purpose of those bits.

>
> Maybe a reserved mask for the hole register, to ensure the access to
> the register doesn't attempt to set reserved bits? But even then the
> best we can do when emulating is print a warning message.

Added warning messages in the emulator code (v3).

>
> Thanks, Roger.