[PATCH 06/10] drivers: serial: Add xtensa semihosting driver

[Max Filippov]

On Sun, May 19, 2024 at 1:53 PM Jiaxun Yang <jiaxun.yang at flygoat.com> wrote:
>
> Add xtensa semihosting driver.
>
> It can't use regular semihosting driver as Xtensa's has it's own
> semihosting ABI.
>
> Note that semihosting supports puts in serial but I never managed to
> get it work, so it's putc only for now.

I wonder, what were the issues that you experienced?

> Signed-off-by: Jiaxun Yang <jiaxun.yang at flygoat.com>
> ---
>  drivers/serial/Kconfig                     | 18 ++++++-
>  drivers/serial/Makefile                    |  1 +
>  drivers/serial/serial_xtensa_semihosting.c | 79 ++++++++++++++++++++++++++++++
>  3 files changed, 97 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> index 1fe4607598eb..3a1e5a6f2877 100644
> --- a/drivers/serial/Kconfig
> +++ b/drivers/serial/Kconfig
> @@ -501,6 +501,15 @@ config DEBUG_UART_MT7620
>           driver will be available until the real driver model serial is
>           running.
>
> +config DEBUG_UART_XTENSA_SEMIHOSTING
> +       bool "Xtensa semihosting"
> +       depends on XTENSA_SEMIHOSTING_SERIAL
> +       help
> +         Select this to enable the debug UART using the Xtensa semihosting driver.
> +         This provides basic serial output from the console without needing to
> +         start up driver model. The driver will be available until the real
> +         driver model serial is running.
> +
>  endchoice
>
>  config DEBUG_UART_BASE
> @@ -936,7 +945,6 @@ config SH_SCIF_CLK_FREQ
>  config SEMIHOSTING_SERIAL
>         bool "Semihosting UART support"
>         depends on SEMIHOSTING && !SERIAL_RX_BUFFER
> -       imply SERIAL_PUTS
>         help
>           Select this to enable a serial UART using semihosting. Special halt
>           instructions will be issued which an external debugger (such as a
> @@ -1115,6 +1123,14 @@ config XEN_SERIAL
>           If built without DM support, then requires Xen
>           to be built with CONFIG_VERBOSE_DEBUG.
>
> +config XTENSA_SEMIHOSTING_SERIAL
> +       bool "Xtensa Semihosting UART support"
> +       depends on DM_SERIAL
> +       depends on XTENSA_SEMIHOSTING
> +       imply SERIAL_PUTS
> +       help
> +         Select this to enable a serial UART using Xtensa semihosting.
> +
>  choice
>         prompt "Console port"
>         default 8xx_CONS_SMC1
> diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
> index dbe598b74064..78810f98367c 100644
> --- a/drivers/serial/Makefile
> +++ b/drivers/serial/Makefile
> @@ -60,6 +60,7 @@ obj-$(CONFIG_MT7620_SERIAL) += serial_mt7620.o
>  obj-$(CONFIG_HTIF_CONSOLE) += serial_htif.o
>  obj-$(CONFIG_SIFIVE_SERIAL) += serial_sifive.o
>  obj-$(CONFIG_XEN_SERIAL) += serial_xen.o
> +obj-$(CONFIG_XTENSA_SEMIHOSTING_SERIAL) += serial_xtensa_semihosting.o
>  obj-$(CONFIG_S5P4418_PL011_SERIAL) += serial_s5p4418_pl011.o
>
>  ifndef CONFIG_SPL_BUILD
> diff --git a/drivers/serial/serial_xtensa_semihosting.c b/drivers/serial/serial_xtensa_semihosting.c
> new file mode 100644
> index 000000000000..1bf3c18537e7
> --- /dev/null
> +++ b/drivers/serial/serial_xtensa_semihosting.c
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2024 Jiaxun Yang <jiaxun.yang at flygoat.com>
> + */
> +
> +#include <dm.h>
> +#include <malloc.h>
> +#include <serial.h>
> +
> +#include <asm/platform/simcall.h>
> +
> +/**
> + * struct simc_serial_priv - Semihosting serial private data
> + * @counter: Counter used to fake pending every other call
> + */
> +struct simc_serial_priv {
> +       unsigned int counter;
> +};
> +
> +static int simc_serial_getc(struct udevice *dev)
> +{
> +       char ch = 0;
> +
> +       simc_read(0, &ch, sizeof(ch));
> +
> +       return ch;
> +}
> +
> +static int simc_serial_putc(struct udevice *dev, const char ch)
> +{
> +       char str[2] = {0};

Why does it need to be an array if only the first element is ever used?

> +
> +       str[0] = ch;
> +       simc_write(1, str, 1);
> +
> +       return 0;
> +}
> +
> +static int simc_serial_pending(struct udevice *dev, bool input)
> +{
> +       struct simc_serial_priv *priv = dev_get_priv(dev);
> +
> +       if (input)
> +               return priv->counter++ & 1;

This can result in blocking the whole CPU thread of the emulator when
used with simcall-based semihosting and there's no input actually available.
Not sure how it would behave with the gdbio. I think something like
the following:

if (input) {
    int res = simc_poll(0);
    return res < 0 ? priv->counter++ & 1 : res;
}

could be used here to avoid that.

> +       return false;
> +}
> +
> +static const struct dm_serial_ops simc_serial_ops = {
> +       .putc = simc_serial_putc,
> +       .getc = simc_serial_getc,
> +       .pending = simc_serial_pending,
> +};
> +
> +U_BOOT_DRIVER(simc_serial) = {
> +       .name   = "serial_xtensa_semihosting",
> +       .id     = UCLASS_SERIAL,
> +       .priv_auto = sizeof(struct simc_serial_priv),
> +       .ops    = &simc_serial_ops,
> +       .flags  = DM_FLAG_PRE_RELOC,
> +};
> +
> +U_BOOT_DRVINFO(simc_serial) = {
> +       .name = "serial_xtensa_semihosting",
> +};
> +
> +#if CONFIG_IS_ENABLED(DEBUG_UART_XTENSA_SEMIHOSTING)
> +#include <debug_uart.h>
> +
> +static inline void _debug_uart_init(void)
> +{
> +}
> +
> +static inline void _debug_uart_putc(int c)
> +{
> +       simc_serial_putc(NULL, c);
> +}
> +
> +DEBUG_UART_FUNCS
> +#endif
>
> --
> 2.43.0
>


-- 
Thanks.
-- Max