Hi Yao, On Fri, Dec 19, 2025 at 03:14:03AM +0000, Yao Zi wrote: > On Thu, Dec 18, 2025 at 06:52:50PM +0000, Kuan-Wei Chiu wrote: > > Add support for the Google Goldfish TTY serial device. This virtual > > device is commonly used in QEMU virtual machines (such as the m68k > > virt machine) and Android emulators. > > > > The driver implements basic console output and input polling using the > > Goldfish MMIO interface. > > > > Signed-off-by: Kuan-Wei Chiu <[email protected]> > > --- > > MAINTAINERS | 6 ++ > > drivers/serial/Kconfig | 8 +++ > > drivers/serial/Makefile | 1 + > > drivers/serial/serial_goldfish.c | 112 +++++++++++++++++++++++++++++++ > > include/goldfish_tty.h | 18 +++++ > > 5 files changed, 145 insertions(+) > > create mode 100644 drivers/serial/serial_goldfish.c > > create mode 100644 include/goldfish_tty.h > > ... > > > diff --git a/drivers/serial/serial_goldfish.c > > b/drivers/serial/serial_goldfish.c > > new file mode 100644 > > index 00000000000..85d2a93b6ff > > --- /dev/null > > +++ b/drivers/serial/serial_goldfish.c > > @@ -0,0 +1,112 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > GPL-2.0+ has been deprecated as a SPDX license identifier. > GPL-2.0-or-later should probably be used instead[1].
Thanks for the pointer. I wasn't aware of that. I will update it in all the other files as well in v2. > > > +/* > > + * Copyright (C) 2025, Kuan-Wei Chiu <[email protected]> > > + * Goldfish TTY driver for U-Boot > > + */ > > + > > +#include <dm.h> > > +#include <serial.h> > > +#include <goldfish_tty.h> > > +#include <asm/io.h> > > +#include <linux/types.h> > > It looks better if you sort the headers :) Sure. Will do. :) > > ... > > > +static int goldfish_serial_getc(struct udevice *dev) > > +{ > > + static u8 rx_buf[4]; > > + struct goldfish_tty_priv *priv = dev_get_priv(dev); > > + unsigned long base = (unsigned long)priv->base; > > + unsigned long paddr; > > + u32 count; > > + > > + count = __raw_readl((void *)(base + GOLDFISH_TTY_BYTES_READY)); > > + if (count == 0) > > + return -EAGAIN; > > + > > + if (count > sizeof(rx_buf)) > > + count = sizeof(rx_buf); > > + > > + paddr = virt_to_phys((void *)rx_buf); > > + > > + rx_buf[0] = 0xAA; > > + > > + __raw_writel(0, (void *)(base + GOLDFISH_TTY_DATA_PTR_HIGH)); > > + __raw_writel(paddr, (void *)(base + GOLDFISH_TTY_DATA_PTR)); > > + __raw_writel(count, (void *)(base + GOLDFISH_TTY_DATA_LEN)); > > You send a command that reads count bytes, where count represents the > maximum of bytes available in the buffer and sizeof(rx_buf), so it's > possible to read more than one character... > > > + __raw_writel(CMD_READ_BUFFER, (void *)(base + GOLDFISH_TTY_CMD)); > > + > > + if (rx_buf[0] == 0xAA) > > + return -EAGAIN; > > + > > + return rx_buf[0]; > > But only the first character in the buffer is finally returned. Isn't > the following characters (if present) are incorrectly discarded? You're right. Requesting multiple bytes causes the hardware to drop subsequent characters since getc only consumes one. I will set the read length to 1 in v2. > > rx_buf is declared as static in this case. Are you originally intended > to do some type of buffering here? Originally, I used static to avoid performing DMA on the stack. However, I realize that this introduces re-entrancy issues, which prevents supporting multiple device instances properly. In v2, I will move the buffer into struct goldfish_tty_priv. > > Additionally, I don't get the point of setting rx_buf[0] to 0xaa then > checks whether it changes after sending CMD_READ_BUFFER. Is there a case > that CMD_READ_BUFFER would fail even when reading TTY_BYTES_READY > returns non-zero? I had a brief look at qemu's goldfish tty > implementation, but didn't find such situation. TBH, this is debug code used during development that I should have removed before submitting. I will remove it in v2. For reference, here is the planned update for v2: struct goldfish_tty_priv { void __iomem *base; u8 rx_buf; }; static int goldfish_serial_getc(struct udevice *dev) { struct goldfish_tty_priv *priv = dev_get_priv(dev); unsigned long base = (unsigned long)priv->base; unsigned long paddr; u32 count; count = __raw_readl((void *)(base + GOLDFISH_TTY_BYTES_READY)); if (count == 0) return -EAGAIN; paddr = virt_to_phys((void *)&priv->rx_buf); __raw_writel(0, (void *)(base + GOLDFISH_TTY_DATA_PTR_HIGH)); __raw_writel(paddr, (void *)(base + GOLDFISH_TTY_DATA_PTR)); __raw_writel(1, (void *)(base + GOLDFISH_TTY_DATA_LEN)); __raw_writel(CMD_READ_BUFFER, (void *)(base + GOLDFISH_TTY_CMD)); return priv->rx_buf; } Regards, Kuan-Wei > > > +} > > Best regards, > Yao Zi > > [1]: https://lore.kernel.org/u-boot/20251212142859.GQ303283@bill-the-cat
