Re: [PATCH 1/3] serial: Add Goldfish TTY driver
On Sun, Dec 21, 2025 at 12:28:49AM +0800, Kuan-Wei Chiu wrote:
> Hi Yao,
>
> On Fri, Dec 19, 2025 at 03:14:03AM +, Yao Zi wrote:
> > On Thu, Dec 18, 2025 at 06:52:50PM +, 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
> > > ---
> > > 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
...
> 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;
> }
Thanks for the explanation, this version looks fine to me.
> Regards,
> Kuan-Wei
Regards,
Yao Zi
> > > +}
> >
> > Best regards,
> > Yao Zi
> >
> > [1]: https://lore.kernel.org/u-boot/20251212142859.GQ303283@bill-the-cat
Re: [PATCH 1/3] serial: Add Goldfish TTY driver
Hi Yao,
On Fri, Dec 19, 2025 at 03:14:03AM +, Yao Zi wrote:
> On Thu, Dec 18, 2025 at 06:52:50PM +, 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
> > ---
> > 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 000..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
> > + * Goldfish TTY driver for U-Boot
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
>
> 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
Re: [PATCH 1/3] serial: Add Goldfish TTY driver
On Thu, Dec 18, 2025 at 06:52:50PM +, 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
> ---
> 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 000..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
> + * Goldfish TTY driver for U-Boot
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
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
