Re: [PATCH v7 11/16] emul/ns16x50: implement FCR register (write-only)

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Denis,

Thank you for the patch.

On Tue, Sep 9, 2025 at 1:06 AM <dmukhin@xxxxxxx> wrote:
>
> From: Denis Mukhin <dmukhin@xxxxxxxx>
>
> Add emulation logic for FCR register.
>
> Note, that does not hook FIFO interrupt moderation to the FIFO management
> code for simplicity.
>
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
> Changes since v6:
> - dropped UART_IIR_THR handling from UART_FCR_CLTX case
> ---
>  xen/common/emul/vuart/ns16x50.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/xen/common/emul/vuart/ns16x50.c b/xen/common/emul/vuart/ns16x50.c
> index 137ce08f4e1d..a92df6923aa5 100644
> --- a/xen/common/emul/vuart/ns16x50.c
> +++ b/xen/common/emul/vuart/ns16x50.c
> @@ -374,6 +374,33 @@ static int ns16x50_io_write8(
>              regs[UART_IER] = val & UART_IER_MASK;
>              break;
>
> +        case UART_FCR: /* WO */
> +            if ( val & UART_FCR_RSRVD0 )
> +                ns16x50_warn(vdev, "FCR: attempt to set reserved bit: %x\n",
> +                             UART_FCR_RSRVD0);
> +
> +            if ( val & UART_FCR_RSRVD1 )
> +                ns16x50_warn(vdev, "FCR: attempt to set reserved bit: %x\n",
> +                             UART_FCR_RSRVD1);

Do we really need these checks and prints?

> +
> +            if ( val & UART_FCR_CLRX )
> +            {
> +                ns16x50_fifo_rx_reset(vdev);

Note from the NS16550A datasheet:
Bit 0: This bit must be a 1 when other FCR bits are written to, or they
will not be programmed.

> +                regs[UART_LSR] &= ~UART_LSR_DR;
> +            }
> +
> +            if ( val & UART_FCR_CLTX )
> +                ns16x50_fifo_tx_reset(vdev);
> +
> +            if ( val & UART_FCR_ENABLE )
> +                val &= UART_FCR_ENABLE | UART_FCR_DMA | UART_FCR_TRG_MASK;

Why can’t we just write back val as is, but with CLTX/CLRX cleared when
UART_FCR_ENABLE is set? For example:

    regs[UART_FCR] = val & ~(UART_FCR_CLTX | UART_FCR_CLRX);

> +            else
> +                val = 0;

If we take the above into account, I think we shouldn’t change the
content of FCR in the case where bit 0 is 0.

Also, from the spec:
“Resetting FCR0 will clear all bytes in both FIFOs.”


Best regards,
Mykola



> +
> +            regs[UART_FCR] = val;
> +
> +            break;
> +
>          case UART_LCR:
>              regs[UART_LCR] = val;
>              break;
> --
> 2.51.0
>
>