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