Hi Denis,
Thank you for the patch.
On Tue, Sep 9, 2025 at 1:06 AM <dmukhin@xen.org> wrote:
>
> From: Denis Mukhin <dmukhin@ford.com>
>
> 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@ford.com>
> ---
> 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
>
>