On Fri, Feb 16, 2024 at 03:02:10PM +0100, Thomas Huth wrote:
> getchar() can currently only be called once on arm since the implementation
> is a little bit too naïve: After the first character has arrived, the
> data register never gets set to zero again. To properly check whether a
> byte is available, we need to check the "RX fifo empty" on the pl011 UART
> or the "RX data ready" bit on the ns16550a UART instead.
>
> With this proper check in place, we can finally also get rid of the
> ugly assert(count < 16) statement here.
>
> Signed-off-by: Thomas Huth <thuth@xxxxxxxxxx>
> ---
> lib/arm/io.c | 34 ++++++++++++++--------------------
> 1 file changed, 14 insertions(+), 20 deletions(-)
>
> diff --git a/lib/arm/io.c b/lib/arm/io.c
> index c15e57c4..836fa854 100644
> --- a/lib/arm/io.c
> +++ b/lib/arm/io.c
> @@ -28,6 +28,7 @@ static struct spinlock uart_lock;
> */
> #define UART_EARLY_BASE (u8 *)(unsigned long)CONFIG_UART_EARLY_BASE
> static volatile u8 *uart0_base = UART_EARLY_BASE;
> +bool is_pl011_uart;
>
> static void uart0_init_fdt(void)
> {
> @@ -59,7 +60,10 @@ static void uart0_init_fdt(void)
> abort();
> }
>
> + is_pl011_uart = (i == 0);
> } else {
> + is_pl011_uart = !fdt_node_check_compatible(dt_fdt(), ret,
> + "arm,pl011");
> ret = dt_pbus_translate_node(ret, 0, &base);
> assert(ret == 0);
> }
> @@ -111,31 +115,21 @@ void puts(const char *s)
> spin_unlock(&uart_lock);
> }
>
> -static int do_getchar(void)
> +int __getchar(void)
> {
> - int c;
> + int c = -1;
>
> spin_lock(&uart_lock);
> - c = readb(uart0_base);
> - spin_unlock(&uart_lock);
> -
> - return c ?: -1;
> -}
> -
> -/*
> - * Minimalist implementation for migration completion detection.
> - * Without FIFOs enabled on the QEMU UART device we just read
> - * the data register: we cannot read more than 16 characters.
> - */
> -int __getchar(void)
> -{
> - int c = do_getchar();
> - static int count;
>
> - if (c != -1)
> - ++count;
> + if (is_pl011_uart) {
> + if (!(readb(uart0_base + 6 * 4) & 0x10)) /* RX not empty? */
> + c = readb(uart0_base);
> + } else {
> + if (readb(uart0_base + 5) & 0x01) /* RX data ready? */
> + c = readb(uart0_base);
> + }
I think I'd eventually prefer encapsulating these separate implementations
into a console device, but that might be a bit heavy handed for a fix that
we want to pull into the multi-migration series.
Thanks,
drew
