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

On Sun, May 19, 2024 at 1:53 PM Jiaxun Yang jiaxun.yang@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@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@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