Re: [PATCH v3 2/2] xen/ns16550: drain TX before reinitializing the UART

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 10/20/26 5:45 AM, Zhang Zheng wrote:
> Drain pending output before resetting FIFOs. Bound the wait to avoid
> hanging boot if the UART never reports empty.
> 
> Wait for TEMT (which already implies THRE on a 16550) to ensure
> transmission has finished before reconfiguration. uart->lsr_mask normally
> checks only THRE, which indicates room for more output.
> 
> Signed-off-by: Zhang Zheng <zhangzheng@iscas.ac.cn>
> ---
>   xen/drivers/char/ns16550.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 1f54155d67..29f784280e 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -410,6 +410,12 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
>   static void __init cf_check ns16550_init_preirq(struct serial_port *port)
>   {
>       struct ns16550 *uart = port->uart;
> +    cycles_t start;
> +    /*
> +     * Too early for proper time calibration, assume a 5GHz CPU and let it wait
> +     * about 1s, with still reasonable timeout on a slower CPU.
> +     */
> +    cycles_t timeout_ticks = 5UL * 1000 * 1000 * 1000;

Shouldn't we use here ULL? 5UL * 1000 * 1000 * 1000 > MAX_unsingned_long 
on 32-bit platforms, at least, for arm33 and riscv32 unsigned long is 
4-byte type?

Also, I think that the comment should be updated as get_cycles() doesn't 
count CPU cycles:
  - on RISC-V it reads the time CSR, which ticks at the timebase
    frequency. E.g. on SpacemiT K1 that's 24MHz, so the "about 1s"
    becomes ~208s; with QEMU virt's 10MHz it's ~500s.
  - on Arm it reads CNTPCT, again typically in the tens of MHz range,
    with similar results.

So it won't be 1s for that arch-s.

Also for Arm and RISC-V preinit_xen_time() runs before 
console_init_preirq(), so time is already usable at this point if I am 
not mistaken something so "Too early for proper time calibration" sounds 
incorrect.

>   
>   #ifdef CONFIG_HAS_IOPORTS
>       /* I/O ports are distinguished by their size (16 bits). */
> @@ -427,6 +433,15 @@ static void __init cf_check ns16550_init_preirq(struct serial_port *port)
>   #endif
>       }
>   
> +    /*
> +     * Drain firmware/early-console output before changing line settings or
> +     * resetting FIFOs. Wait for TEMT so the shift register is empty too.
> +     */
> +    start = get_cycles();
> +    while ( !(ns_read_reg(uart, UART_LSR) & UART_LSR_TEMT) &&

This is done before pci_serial_early_init() (called from
ns16550_setup_preirq()), so for a PCI UART (specified via
<port-bdf> / <bridge-bdf> or found via "pci") whose decoding hasn't
been enabled by firmware, the LSR read may still return all ones.
With TEMT set in that value, the wait is then simply skipped. That's
harmless, but perhaps worth a word in the description or a code
comment.


> +            get_cycles() - start < timeout_ticks )
> +        cpu_relax();
> +
>       ns16550_setup_preirq(uart);
>   
>       /* Check this really is a 16550+. Otherwise we have no FIFOs. */

~ Oleksii