Re: [PATCH RESEND] hw/openrisc/openrisc_sim: keep serial@90000000 as default

[Stafford Horne <shorne@gmail.com>]

On Wed, Aug 28, 2024 at 04:38:49PM +0100, Peter Maydell wrote:
> On Tue, 27 Aug 2024 at 19:53, Stafford Horne <shorne@gmail.com> wrote:
> >
> > On Sun, Aug 25, 2024 at 03:09:20PM +0100, Peter Maydell wrote:
> > > On Sun, 25 Aug 2024 at 12:35, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > > >
> > > > On Fri, Aug 23, 2024 at 07:28:43AM +0100, Stafford Horne wrote:
> > > > > Also, I will wait to see if Jason has anything to say.
> > > >
> > > > So long as this doesn't change the assignment of the serial ports to
> > > > device nodes in Linux, I don't think this should interfere with much.
> > > > You might want to try it, though.
> > >
> > > It looks like this board already creates the fdt /aliases/
> > > node and puts uart0, uart1, etc, so that part should be OK.
> > >
> > > However I notice that the openrisc_sim_serial_init() code
> > > will always set the /chosen/stdout-path, so this means
> > > (unless I'm misreading the code -- I haven't tested) that
> > > the last UART we create will be the stdout-path one. Before
> > > this patch, that would be serial_hd(0), but after this it
> > > will not be. So I think we probably need to fix this too
> > > in the same patch, so that we only set stdout-path for uart0,
> > > rather than setting it and then overwriting it on all the
> > > subsequent calls. This patch on its own will change the
> > > stdout-path value I think.
> >
> > Hi Peter,
> >
> > I suspected the same and tested the theory.  Now when running linux with
> > or1k-sim machine we get no stdout output from qemu.  Upon debugging and
> > looking at dmesg via gdb I can see the wrong uart is getting setup in
> > Linux:
> >
> >     [    0.080000] workingset: timestamp_bits=30 max_order=17 bucket_order=0
> >     [    0.100000] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
> >     [    0.110000] printk: legacy console [ttyS0] disabled
> >     [    0.110000] 90000300.serial: ttyS0 at MMIO 0x90000300 (irq = 2, base_baud = 1250000) is a 16550A
> >     [    0.120000] printk: legacy console [ttyS0] enabled
> >     [    0.120000] 90000200.serial: ttyS1 at MMIO 0x90000200 (irq = 2, base_baud = 1250000) is a 16550A
> >     [    0.130000] 90000100.serial: ttyS2 at MMIO 0x90000100 (irq = 2, base_baud = 1250000) is a 16550A
> >     [    0.130000] 90000000.serial: ttyS3 at MMIO 0x90000000 (irq = 2, base_baud = 1250000) is a 16550A
> >     [    0.150000] NET: Registered PF_PACKET protocol family
> >     [    0.160000] clk: Disabling unused clocks
> 
> Interesting, that seems to have assigned ttyS0/1/2/3 in the
> reverse order, which suggests it might be ignoring the /aliases/
> nodes entirely? Though that would surprise me, so perhaps
> something else is going on.

This got me thinking, the /aliases/ defined in OpenRISC are "uart0", "uart1"...
this is different than almost everything else which use "serial0", "serial1"...
I don't know why OpenRISC chose to use "uart0" and I think this is an issue.

I tried the patch below.

After switching to the more standard "serial0", ... everything is working well.

It seems only one driver uses device tree alias stem (prefix) "uart" and that is
drivers/tty/serial/bcm63xx_uart.c.  Which we have no intention of supporting.

All other drivers look for aliases using the serial prefix via call:

  of_alias_get_id(np, "serial");.

> For the Arm virt board we ended up taking a conservative
> approach of making sure the UARTs were listed in the dtb
> in the exact same order we'd traditionally done it, for
> the benefit of any guests which didn't honour /aliases/
> or /chosen/stdout-path. But we were thinking more about
> that being old firmware rather than the kernel.

In this case only the openrisc_sim board has been setup with multiple
UARTs.  I think making sure the first/qemu default serial device
stays the same is the most important for this point, which the original patch
fixes.

-Stafford

diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c
index bffd6f721f..2a15a3a4f0 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);
 }