diff mbox series

[PULL,1/2] hw/openrisc/openrisc_sim: keep serial@90000000 as default

Message ID 20241203111513.402641-2-shorne@gmail.com (mailing list archive)
State New, archived
Headers show
Series [PULL,1/2] hw/openrisc/openrisc_sim: keep serial@90000000 as default | expand

Commit Message

Stafford Horne Dec. 3, 2024, 11:15 a.m. UTC 
From: Ahmad Fatoum <a.fatoum@pengutronix.de>

We used to only have a single UART on the platform and it was located at
address 0x90000000. When the number of UARTs was increased to 4, the
first UART remained at it's location, but instead of being the first one
to be registered, it became the last.

This caused QEMU to pick 0x90000300 as the default UART, which broke
software that hardcoded the address of 0x90000000 and expected it's
output to be visible when the user configured only a single console.

This caused regressions[1] in the barebox test suite when updating to a
newer QEMU. As there seems to be no good reason to register the UARTs in
inverse order, let's register them by ascending address, so existing
software can remain oblivious to the additional UART ports.

Changing the order of uart registration alone breaks Linux which
was choosing the UART at 0x90000300 as the default for ttyS0.  To fix
Linux we fix three things in the device tree:

 1. Define stdout-path only one time for the first registered UART
    instead of incorrectly defining for each UART.
 2. Change the UART alias name from 'uart0' to 'serial0' as almost all
    Linux tty drivers look for an alias starting with "serial".
 3. Add the UART nodes so they appear in the final DTB in the
    order starting with the lowest address and working upwards.

In summary these changes mean that the QEMU default UART (serial_hd(0))
is now setup where:

 * serial_hd(0) is the lowest-address UART
 * serial_hd(0) is listed first in the DTB
 * serial_hd(0) is the /chosen/stdout-path one
 * the /aliases/serial0 alias points at serial_hd(0)

[1]: https://lore.barebox.org/barebox/707e7c50-aad1-4459-8796-0cc54bab32e2@pengutronix.de/T/#m5da26e8a799033301489a938b5d5667b81cef6ad

Fixes: 777784bda468 ("hw/openrisc: support 4 serial ports in or1ksim")
Cc: qemu-stable@nongnu.org
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
[stafford: Change to serial0 alias and update change message, reverse
 uart registration order]
Signed-off-by: Stafford Horne <shorne@gmail.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/openrisc/openrisc_sim.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

Comments

Ahmad Fatoum Dec. 5, 2024, 3 p.m. UTC | #1 
Hello Stafford,

On 03.12.24 12:15, Stafford Horne wrote:
> From: Ahmad Fatoum <a.fatoum@pengutronix.de>
> [1]: https://lore.barebox.org/barebox/707e7c50-aad1-4459-8796-0cc54bab32e2@pengutronix.de/T/#m5da26e8a799033301489a938b5d5667b81cef6ad
> 
> Fixes: 777784bda468 ("hw/openrisc: support 4 serial ports in or1ksim")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> [stafford: Change to serial0 alias and update change message, reverse
>  uart registration order]

Thanks a lot for your subsequent iterations on the patch.
The changes look good to me.

Thanks,
Ahmad

> Signed-off-by: Stafford Horne <shorne@gmail.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/openrisc/openrisc_sim.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c
> index 9fb63515ef..42f002985b 100644
> --- a/hw/openrisc/openrisc_sim.c
> +++ b/hw/openrisc/openrisc_sim.c
> @@ -250,7 +250,7 @@ static void openrisc_sim_serial_init(Or1ksimState *state, hwaddr base,
>      void *fdt = state->fdt;
>      char *nodename;
>      qemu_irq serial_irq;
> -    char alias[sizeof("uart0")];
> +    char alias[sizeof("serial0")];
>      int i;
>  
>      if (num_cpus > 1) {
> @@ -265,7 +265,7 @@ static void openrisc_sim_serial_init(Or1ksimState *state, hwaddr base,
>          serial_irq = get_cpu_irq(cpus, 0, irq_pin);
>      }
>      serial_mm_init(get_system_memory(), base, 0, serial_irq, 115200,
> -                   serial_hd(OR1KSIM_UART_COUNT - uart_idx - 1),
> +                   serial_hd(uart_idx),
>                     DEVICE_NATIVE_ENDIAN);
>  
>      /* Add device tree node for serial. */
> @@ -277,10 +277,13 @@ static void openrisc_sim_serial_init(Or1ksimState *state, hwaddr base,
>      qemu_fdt_setprop_cell(fdt, nodename, "clock-frequency", OR1KSIM_CLK_MHZ);
>      qemu_fdt_setprop(fdt, nodename, "big-endian", NULL, 0);
>  
> -    /* The /chosen node is created during fdt creation. */
> -    qemu_fdt_setprop_string(fdt, "/chosen", "stdout-path", nodename);
> -    snprintf(alias, sizeof(alias), "uart%d", uart_idx);
> +    if (uart_idx == 0) {
> +        /* The /chosen node is created during fdt creation. */
> +        qemu_fdt_setprop_string(fdt, "/chosen", "stdout-path", nodename);
> +    }
> +    snprintf(alias, sizeof(alias), "serial%d", uart_idx);
>      qemu_fdt_setprop_string(fdt, "/aliases", alias, nodename);
> +
>      g_free(nodename);
>  }
>  
> @@ -326,11 +329,22 @@ static void openrisc_sim_init(MachineState *machine)
>                                  smp_cpus, cpus, OR1KSIM_OMPIC_IRQ);
>      }
>  
> -    for (n = 0; n < OR1KSIM_UART_COUNT; ++n)
> +    /*
> +     * We create the UART nodes starting with the highest address and
> +     * working downwards, because in QEMU the DTB nodes end up in the
> +     * DTB in reverse order of creation. Correctly-written guest software
> +     * will not care about the node order (it will look at stdout-path
> +     * or the alias nodes), but for the benefit of guest software which
> +     * just looks for the first UART node in the DTB, make sure the
> +     * lowest-address UART (which is QEMU's first serial port) appears
> +     * first in the DTB.
> +     */
> +    for (n = OR1KSIM_UART_COUNT - 1; n >= 0; n--) {
>          openrisc_sim_serial_init(state, or1ksim_memmap[OR1KSIM_UART].base +
>                                          or1ksim_memmap[OR1KSIM_UART].size * n,
>                                   or1ksim_memmap[OR1KSIM_UART].size,
>                                   smp_cpus, cpus, OR1KSIM_UART_IRQ, n);
> +    }
>  
>      load_addr = openrisc_load_kernel(ram_size, kernel_filename,
>                                       &boot_info.bootstrap_pc);
diff mbox series

Patch

diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c
index 9fb63515ef..42f002985b 100644
--- a/hw/openrisc/openrisc_sim.c
+++ b/hw/openrisc/openrisc_sim.c
@@ -250,7 +250,7 @@  static void openrisc_sim_serial_init(Or1ksimState *state, hwaddr base,
     void *fdt = state->fdt;
     char *nodename;
     qemu_irq serial_irq;
-    char alias[sizeof("uart0")];
+    char alias[sizeof("serial0")];
     int i;
 
     if (num_cpus > 1) {
@@ -265,7 +265,7 @@  static void openrisc_sim_serial_init(Or1ksimState *state, hwaddr base,
         serial_irq = get_cpu_irq(cpus, 0, irq_pin);
     }
     serial_mm_init(get_system_memory(), base, 0, serial_irq, 115200,
-                   serial_hd(OR1KSIM_UART_COUNT - uart_idx - 1),
+                   serial_hd(uart_idx),
                    DEVICE_NATIVE_ENDIAN);
 
     /* Add device tree node for serial. */
@@ -277,10 +277,13 @@  static void openrisc_sim_serial_init(Or1ksimState *state, hwaddr base,
     qemu_fdt_setprop_cell(fdt, nodename, "clock-frequency", OR1KSIM_CLK_MHZ);
     qemu_fdt_setprop(fdt, nodename, "big-endian", NULL, 0);
 
-    /* The /chosen node is created during fdt creation. */
-    qemu_fdt_setprop_string(fdt, "/chosen", "stdout-path", nodename);
-    snprintf(alias, sizeof(alias), "uart%d", uart_idx);
+    if (uart_idx == 0) {
+        /* The /chosen node is created during fdt creation. */
+        qemu_fdt_setprop_string(fdt, "/chosen", "stdout-path", nodename);
+    }
+    snprintf(alias, sizeof(alias), "serial%d", uart_idx);
     qemu_fdt_setprop_string(fdt, "/aliases", alias, nodename);
+
     g_free(nodename);
 }
 
@@ -326,11 +329,22 @@  static void openrisc_sim_init(MachineState *machine)
                                 smp_cpus, cpus, OR1KSIM_OMPIC_IRQ);
     }
 
-    for (n = 0; n < OR1KSIM_UART_COUNT; ++n)
+    /*
+     * We create the UART nodes starting with the highest address and
+     * working downwards, because in QEMU the DTB nodes end up in the
+     * DTB in reverse order of creation. Correctly-written guest software
+     * will not care about the node order (it will look at stdout-path
+     * or the alias nodes), but for the benefit of guest software which
+     * just looks for the first UART node in the DTB, make sure the
+     * lowest-address UART (which is QEMU's first serial port) appears
+     * first in the DTB.
+     */
+    for (n = OR1KSIM_UART_COUNT - 1; n >= 0; n--) {
         openrisc_sim_serial_init(state, or1ksim_memmap[OR1KSIM_UART].base +
                                         or1ksim_memmap[OR1KSIM_UART].size * n,
                                  or1ksim_memmap[OR1KSIM_UART].size,
                                  smp_cpus, cpus, OR1KSIM_UART_IRQ, n);
+    }
 
     load_addr = openrisc_load_kernel(ram_size, kernel_filename,
                                      &boot_info.bootstrap_pc);