Re: [PATCH 4/5] hw/riscv/atlantis: Add machine property to select default uart

[Daniel Henrique Barboza]

On 9/29/2026 12:16 AM, port...@oss.tenstorrent.com wrote:

From: Portia Stephens <port...@oss.tenstorrent.com>

The Atlantis SoC has 5 UARTs. This adds a machine property to set
which uart is on the first serial interface. This is the same pattern
that is used by aspeed. The rest of the uart's will be added in
sequential order starting at serial interface 1.

The default value is uart1 which means the Ascalon console will be on
serial interface 0 by default.

Signed-off-by: Portia Stephens <port...@oss.tenstorrent.com>
---

The code LGTM but I wonder if we can make it simpler.

IIUC there is a new ams->uart_chosen being added that holds the numeric ID of
the first UART.  Then there's a new string property called "console" that needs
to be on the format "uartN".  To do that there's a string parsing code in
tt_atlantis_set_console() to guarantee it.  And there's code to guarantee that
N is also in an acceptable range.  Aside from parsing N the 'console' string is
not being used.

I believe there's a good case here to make "console" an uint32_t property 
instead,
receiving N, getting rid of all the string parsing code.

Also, there's a couple of instances of this:


int uart_chosen = ams->uart_chosen ? ams->uart_chosen : TT_ATL_UART1;


You can avoid this ternary by initializing ams->uart_chosen  = TT_ATL_UART1; 
during
tt_atlantis_soc_init().  Coupled with the parsing code from the "console" 
property
there is a guarantee that ams->uart_chosen will always be an acceptable value.



Thanks,
Daniel

  hw/riscv/tt_atlantis.c         | 77 ++++++++++++++++++++++++++++++----
  include/hw/riscv/tt_atlantis.h |  1 +
  2 files changed, 69 insertions(+), 9 deletions(-)

diff --git a/hw/riscv/tt_atlantis.c b/hw/riscv/tt_atlantis.c
index d567ec1155..32ba755bbf 100644
--- a/hw/riscv/tt_atlantis.c
+++ b/hw/riscv/tt_atlantis.c
@@ -420,6 +420,29 @@ static void load_fdt(TTAtlantisState *ams)
      create_fdt_memory(ms->fdt, &ams->soc);
  }

+static inline int tt_atlantis_uart_index(int uart_dev)

+{
+    return uart_dev - TT_ATL_UART0;
+}
+
+static void tt_atlantis_connect_serial_hds_to_uarts(TTAtlantisState *ams)
+{
+    TTAtlantisSoCState *s = &ams->soc;
+
+    int uart_chosen = ams->uart_chosen ? ams->uart_chosen : TT_ATL_UART1;
+    int uart_index = tt_atlantis_uart_index(uart_chosen);
+
+    qdev_prop_set_chr(DEVICE(&s->uart[uart_index]), "chardev", serial_hd(0));
+
+    for (int i = 1, uart = 0; i < TT_ATL_NUM_UARTS; uart++) {
+        if (uart == uart_index) {
+            continue;
+        }
+        qdev_prop_set_chr(DEVICE(&s->uart[uart]), "chardev", serial_hd(i++));
+    }
+}
+
+
  static void tt_atlantis_machine_done(Notifier *n, void *data)
  {
      TTAtlantisState *ams = container_of(n, TTAtlantisState, machine_done);
@@ -482,11 +505,6 @@ static void tt_atlantis_machine_done(Notifier *n, void 
*data)
                                fdt_load_addr);
  }

-static inline int tt_atlantis_uart_index(int uart_dev)

-{
-    return uart_dev - TT_ATL_UART0;
-}
-
  static void tt_atlantis_soc_init(Object *obj)
  {
      TTAtlantisSoCState *s = TT_ATLANTIS_SOC(obj);
@@ -590,10 +608,6 @@ static void tt_atlantis_soc_realize(DeviceState *dev, 
Error **errp)
      memory_region_add_subregion(s->memory, s->memmap[TT_ATL_BOOTROM].base,
                                  &s->bootrom);

- /* UART1, the soc console (UART0 is for the boot microcontroller */

-    qdev_prop_set_chr(DEVICE(&s->uart[tt_atlantis_uart_index(TT_ATL_UART1)]),
-                      "chardev", serial_hd(0));
-
      for (int i = 0; i < TT_ATL_NUM_UARTS; i++) {
          SysBusDevice *sbd = SYS_BUS_DEVICE(&s->uart[i]);
          if (!sysbus_realize(sbd, errp)) {
@@ -661,6 +675,10 @@ static void tt_atlantis_machine_init(MachineState *machine)
                               OBJECT(&ams->soc_memory), &error_abort);
      object_property_set_link(OBJECT(&ams->soc), "dram", OBJECT(machine->ram),
                               &error_abort);
+
+    /* Map serial interfaces to uart */
+    tt_atlantis_connect_serial_hds_to_uarts(ams);
+
      qdev_realize(DEVICE(&ams->soc), NULL, &error_fatal);

/* I2C peripherals: qemu specific */

@@ -678,6 +696,45 @@ static void tt_atlantis_machine_init(MachineState *machine)
      qemu_add_machine_init_done_notifier(&ams->machine_done);
  }

+static char *tt_atlantis_get_console(Object *obj, Error **errp)

+{
+    TTAtlantisState *ams = TT_ATLANTIS_MACHINE(obj);
+
+    /* Default to Ascalon console */
+    int uart_chosen = ams->uart_chosen ? ams->uart_chosen : TT_ATL_UART1;
+
+    return g_strdup_printf("uart%d", tt_atlantis_uart_index(uart_chosen));
+}
+
+static void tt_atlantis_set_console(Object *obj, const char *value, Error 
**errp)
+{
+    TTAtlantisState *ams = TT_ATLANTIS_MACHINE(obj);
+
+    int val;
+    int uart_first = tt_atlantis_uart_index(TT_ATL_UART0);
+    int uart_last = tt_atlantis_uart_index(TT_ATL_UART4);
+
+    if (sscanf(value, "uart%d", &val) != 1 || val < 0) {
+        error_setg(errp, "Bad value for \"uart\" property");
+        return;
+    }
+
+    if (val < uart_first || val > uart_last) {
+        error_setg(errp, "\"uart\" should be in range [%d - %d]",
+                   uart_first, uart_last);
+        return;
+    }
+    ams->uart_chosen = val + TT_ATL_UART0;
+}
+
+static void tt_atlantis_machine_class_props_init(ObjectClass *oc)
+{
+    object_class_property_add_str(oc, "console", tt_atlantis_get_console,
+                                  tt_atlantis_set_console);
+    object_class_property_set_description(oc, "console",
+                           "Change the default UART to \"uartX\"");
+}
+
  static void tt_atlantis_machine_class_init(ObjectClass *oc, const void *data)
  {
      MachineClass *mc = MACHINE_CLASS(oc);
@@ -691,6 +748,8 @@ static void tt_atlantis_machine_class_init(ObjectClass *oc, 
const void *data)
      mc->block_default_type = IF_VIRTIO;
      mc->no_cdrom = 1;
      mc->default_ram_id = "tt_atlantis.ram";
+
+    tt_atlantis_machine_class_props_init(oc);
  }

static const TypeInfo tt_atlantis_types[] = {

diff --git a/include/hw/riscv/tt_atlantis.h b/include/hw/riscv/tt_atlantis.h
index b81b7c8abd..7bd46e9c9b 100644
--- a/include/hw/riscv/tt_atlantis.h
+++ b/include/hw/riscv/tt_atlantis.h
@@ -56,6 +56,7 @@ struct TTAtlantisState {

MemoryRegion soc_memory;

      TTAtlantisSoCState soc;
+    uint32_t uart_chosen;
  };

enum {