Re: [PATCH 1/3] serial: Add Goldfish TTY driver

[Yao Zi <me@ziyao.cc>]

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 <visitorckw@gmail.com>
> ---
>  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].

> +/*
> + * Copyright (C) 2025, Kuan-Wei Chiu <visitorckw@gmail.com>
> + * 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 :)

...

> +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?

rx_buf is declared as static in this case. Are you originally intended
to do some type of buffering here?

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.

> +}

Best regards,
Yao Zi

[1]: https://lore.kernel.org/u-boot/20251212142859.GQ303283@bill-the-cat