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

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Thursday, December 12th, 2024 at 7:07 AM, Jan Beulich <jbeulich@xxxxxxxx> 
wrote:

>
>
> On 06.12.2024 05:41, Denis Mukhin via B4 Relay wrote:
>
> > --- 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)
>
>
> There are two issues here: "max" means "highest within range", yet
> you define it as "first invalid", i.e. something we'd normally call
> "_NR" or "NUM". And then, as the comment says, this is a limit the
> emulation is going to expose, not something generally applicable to
> UARTs of this kind. Hence the UART prefix alone isn't quite correct
> either.

I did renaming and moved declaration to the UART emulator code.

>
> > @@ -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)
>
>
> Much like BSY is a 3-bit field, aiui this is a 2-bit one.

Updated to 2-bit mask.

>
> > /* 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 /
> > +#define UART_FCR_RTB0 BIT(6, U) / receiver trigger bit #0 /
> > +#define UART_FCR_RTB1 BIT(7, U) / receiver trigger bit #1 */
> > +#define UART_FCR_TRG_MASK (UART_FCR_RTB0 | UART_FCR_RTB1)
>
>
> Much like the top two bits here are, and - as Roger has said - the
> reserved bits probably also should be.

I kept reserved bits as is.
I think that will help to avoid confusion whether support for those bits
is missing in the driver code or those are reserved.

There are other examples, e.g. PCI_PM_CAP_RESERVED when reserved bits
declared and not necessarily used in the code base.

>
> > @@ -64,17 +79,17 @@
> >
> > /*
> > * Note: The FIFO trigger levels are chip specific:
> > - * RX:76 = 00 01 10 11 TX:54 = 00 01 10 11
> > - * PC16550D: 1 4 8 14 xx xx xx xx
> > - * TI16C550A: 1 4 8 14 xx xx xx xx
> > - * TI16C550C: 1 4 8 14 xx xx xx xx
> > - * ST16C550: 1 4 8 14 xx xx xx xx
> > - * ST16C650: 8 16 24 28 16 8 24 30 PORT_16650V2
> > - * NS16C552: 1 4 8 14 xx xx xx xx
> > - * ST16C654: 8 16 56 60 8 16 32 56 PORT_16654
> > - * TI16C750: 1 16 32 56 xx xx xx xx PORT_16750
> > - * TI16C752: 8 16 56 60 8 16 32 56
> > - * Tegra: 1 4 8 14 16 8 4 1 PORT_TEGRA
> > + * RX:76 = 00 01 10 11 TX:54 = 00 01 10 11
> > + * PC16550D: 1 4 8 14 xx xx xx xx
> > + * TI16C550A: 1 4 8 14 xx xx xx xx
> > + * TI16C550C: 1 4 8 14 xx xx xx xx
> > + * ST16C550: 1 4 8 14 xx xx xx xx
> > + * ST16C650: 8 16 24 28 16 8 24 30 PORT_16650V2
> > + * NS16C552: 1 4 8 14 xx xx xx xx
> > + * ST16C654: 8 16 56 60 8 16 32 56 PORT_16654
> > + * TI16C750: 1 16 32 56 xx xx xx xx PORT_16750
> > + * TI16C752: 8 16 56 60 8 16 32 56
> > + * Tegra: 1 4 8 14 16 8 4 1 PORT_TEGRA
> > */
>
>
> While perhaps okay, the adjustment of this table still looks unrelated.
> It wants at least mentioning in the description, to clarify it's an
> intentional change (as opposed to e.g. being an effect of how your
> editor is configured).

Thanks, I updated the commit message.

>
> > @@ -96,11 +111,31 @@
> > #define UART_LCR_CONF_MODE_B 0xBF /* Configuration mode B */
> >
> > /* 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_LOOP 0x10 / Enable loopback test mode /
> > -#define UART_MCR_TCRTLR 0x40 / Access TCR/TLR (TI16C752, EFR[4]=1) /
> > +#define UART_MCR_DTR BIT(0, U) / Data Terminal Ready /
> > +#define UART_MCR_RTS BIT(1, U) / Request to Send /
> > +#define UART_MCR_OUT1 BIT(2, U) / OUT1: interrupt mask /
> > +#define UART_MCR_OUT2 BIT(3, U) / OUT2: interrupt mask /
> > +#define UART_MCR_LOOP BIT(4, U) / Enable loopback test mode /
> > +#define UART_MCR_RESERVED0 BIT(5, U) / Reserved #0 /
> > +#define UART_MCR_RESERVED1 BIT(6, U) / Reserved #1 /
> > +#define UART_MCR_TCRTLR BIT(6, U) / Access TCR/TLR (TI16C752, EFR[4]=1) /
> > +#define UART_MCR_RESERVED2 BIT(7, U) / Reserved #2 /
> > +#define UART_MCR_MASK \
> > + (UART_MCR_DTR | UART_MCR_RTS | \
> > + UART_MCR_OUT1 | UART_MCR_OUT2 | \
> > + UART_MCR_LOOP)
> > +
> > +/ Modem Status Register /
> > +#define UART_MSR_DCTS BIT(0, U) / Change in CTS /
> > +#define UART_MSR_DDSR BIT(1, U) / Change in DSR /
> > +#define UART_MSR_TERI BIT(2, U) / Change in RI /
> > +#define UART_MSR_DDCD BIT(3, U) / Change in CTS */
> > +#define UART_MSR_CTS BIT(4, U)
> > +#define UART_MSR_DSR BIT(5, U)
> > +#define UART_MSR_RI BIT(6, U)
> > +#define UART_MSR_DCD BIT(7, U)
>
>
> As you introduce these constants, I think you also want to switch the sole
> MSR read we have to actually use them.

Sure, no problem; fixed.

>
> Jan