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

Wed, 30 Sep 2026 20:15:12 -0700 


On 1/10/2026 12:48 AM, Daniel Henrique Barboza wrote:



On 9/29/2026 12:16 AM, [email protected] wrote:

From: Portia Stephens <[email protected]>

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 <[email protected]>
---


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.




uart_chosen is part of the machine class, so it would need to be 
initialized in the machine_init.  The machine property is set from user 
inputs prior to the machine_init being called.  Initializing uart_chosen 
in the machine_init will just overwrite the user input value. There is a 
object_property_set_default_uint() which allows setting a default value 
for a machine property , I have used this instead in v2 which has the 
same end result.







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 {



























					Reply via email to