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

Commit Message

Ahmad Fatoum Aug. 22, 2024, 4:38 p.m. UTC

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 its 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 its
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.

[1]: https://lore.barebox.org/barebox/[e-mail]/T/#m5da26e8a799033301489a938b5d5667b81cef6ad
---
v1 -> RESEND:
  - expand addressees beyond apparently defunct [e-mail]

NOTE: I am not familiar with QEMU internals, so please let me know
if registration in inverse order served a particular purpose.
---
 hw/openrisc/openrisc_sim.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Stafford Horne Aug. 23, 2024, 6:28 a.m. UTC | #1

Note the distribution list you use here: [e-mail]
Is old and we should use [e-mail].  I will get the qemu
maintainer file updated.

On Thu, Aug 22, 2024 at 06:38:38PM +0200, Ahmad Fatoum wrote:
> 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 its 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 its
> output to be visible when the user configured only a single console.

This makes sense but what do you mean here by DEFAULT uart?  I guess you mean
the one connected to qemu's stdout by default?

> 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.

This sounds good to me.  I will test this out and queue to qemu after the small
clarification above.

Also, I will wait to see if Jason has anything to say.

-Stafford

> [1]: https://lore.barebox.org/barebox/[e-mail]/T/#m5da26e8a799033301489a938b5d5667b81cef6ad
> ---
> v1 -> RESEND:
>   - expand addressees beyond apparently defunct [e-mail]
> 
> NOTE: I am not familiar with QEMU internals, so please let me know
> if registration in inverse order served a particular purpose.
> ---
>  hw/openrisc/openrisc_sim.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c
> index bffd6f721f7b..d147b00e4817 100644
> --- a/hw/openrisc/openrisc_sim.c
> +++ b/hw/openrisc/openrisc_sim.c
> @@ -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. */
> -- 
> 2.39.2
>

Ahmad Fatoum Aug. 23, 2024, 7:23 a.m. UTC | #2

Hello Stafford,

On 23.08.24 08:28, Stafford Horne wrote:
> Note the distribution list you use here: [e-mail]
> Is old and we should use [e-mail].  I will get the qemu
> maintainer file updated.

So this list is appropriate for all openrisc-related development and not only
for the kernel?

> On Thu, Aug 22, 2024 at 06:38:38PM +0200, Ahmad Fatoum wrote:
>> 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 its 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 its
>> output to be visible when the user configured only a single console.
> 
> This makes sense but what do you mean here by DEFAULT uart?  I guess you mean
> the one connected to qemu's stdout by default?

Yes. I am not keen on the QEMU terminology, but the first registered UART seems
to have a special place. Besides being connected to QEMU's stdio by default,
it's also used to populate /chosen/stdout-path as can be seen when dumping the dtb:

  qemu-system-or1k -kernel /dev/null -machine or1k-sim,dumpdtb=qemu.dtb -nographic


>> 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.
> 
> This sounds good to me.  I will test this out and queue to qemu after the small
> clarification above.
> 
> Also, I will wait to see if Jason has anything to say.

Sure.

By the way, I botched the RESEND and forgot following two lines:

  Fixes: 777784bda468 ("hw/openrisc: support 4 serial ports in or1ksim")
  Signed-off-by: Ahmad Fatoum <[e-mail]>

Let me know if I should resend (provided there's no code changes warranting a v2).

Thanks,
Ahmad

> 
> -Stafford
> 
>> [1]: https://lore.barebox.org/barebox/[e-mail]/T/#m5da26e8a799033301489a938b5d5667b81cef6ad
>> ---
>> v1 -> RESEND:
>>   - expand addressees beyond apparently defunct [e-mail]
>>
>> NOTE: I am not familiar with QEMU internals, so please let me know
>> if registration in inverse order served a particular purpose.
>> ---
>>  hw/openrisc/openrisc_sim.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c
>> index bffd6f721f7b..d147b00e4817 100644
>> --- a/hw/openrisc/openrisc_sim.c
>> +++ b/hw/openrisc/openrisc_sim.c
>> @@ -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. */
>> -- 
>> 2.39.2
>>
>

Stafford Horne Aug. 25, 2024, 5:49 a.m. UTC | #3

On Fri, Aug 23, 2024 at 09:23:23AM +0200, Ahmad Fatoum wrote:
> Hello Stafford,
> 
> On 23.08.24 08:28, Stafford Horne wrote:
> > Note the distribution list you use here: [e-mail]
> > Is old and we should use [e-mail].  I will get the qemu
> > maintainer file updated.
> 
> So this list is appropriate for all openrisc-related development and not only
> for the kernel?
> 
> > On Thu, Aug 22, 2024 at 06:38:38PM +0200, Ahmad Fatoum wrote:
> >> 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 its 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 its
> >> output to be visible when the user configured only a single console.
> > 
> > This makes sense but what do you mean here by DEFAULT uart?  I guess you mean
> > the one connected to qemu's stdout by default?
> 
> Yes. I am not keen on the QEMU terminology, but the first registered UART seems
> to have a special place. Besides being connected to QEMU's stdio by default,
> it's also used to populate /chosen/stdout-path as can be seen when dumping the dtb:
> 
>   qemu-system-or1k -kernel /dev/null -machine or1k-sim,dumpdtb=qemu.dtb -nographic
> 
> 
> >> 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.
> > 
> > This sounds good to me.  I will test this out and queue to qemu after the small
> > clarification above.
> > 
> > Also, I will wait to see if Jason has anything to say.
> 
> Sure.
> 
> By the way, I botched the RESEND and forgot following two lines:
> 
>   Fixes: 777784bda468 ("hw/openrisc: support 4 serial ports in or1ksim")
>   Signed-off-by: Ahmad Fatoum <[e-mail]>
> 
> Let me know if I should resend (provided there's no code changes warranting a v2).
> 

This should be fine thanks.  I will fixup the commit message and repost after a
bit of testing to ensure this does not affect other environments including
Jason's test suite which uses the 4 UARTs.

-Stafford

Jason A. Donenfeld Aug. 25, 2024, 11:34 a.m. UTC | #4

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.

Jason

Peter Maydell Aug. 25, 2024, 2:09 p.m. UTC | #5

On Sun, 25 Aug 2024 at 12:35, Jason A. Donenfeld <[e-mail]> 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.

-- PMM

Stafford Horne Aug. 27, 2024, 6:53 p.m. UTC | #6

On Sun, Aug 25, 2024 at 03:09:20PM +0100, Peter Maydell wrote:
> On Sun, 25 Aug 2024 at 12:35, Jason A. Donenfeld <[e-mail]> 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

I will amend the patch.

-Stafford

Peter Maydell Aug. 28, 2024, 3:38 p.m. UTC | #7

On Tue, 27 Aug 2024 at 19:53, Stafford Horne <[e-mail]> 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 <[e-mail]> 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.

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.

-- PMM

Stafford Horne Aug. 29, 2024, 3:40 p.m. UTC | #8

On Wed, Aug 28, 2024 at 04:38:49PM +0100, Peter Maydell wrote:
> On Tue, 27 Aug 2024 at 19:53, Stafford Horne <[e-mail]> 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 <[e-mail]> 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);
 }

Patch

diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c
index bffd6f721f7b..d147b00e4817 100644
--- a/hw/openrisc/openrisc_sim.c
+++ b/hw/openrisc/openrisc_sim.c
@@ -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. */