On Mon, Nov 25, 2024 at 02:02:35PM +0000, Peter Maydell wrote:
> On Sat, 23 Nov 2024 at 10:39, Stafford Horne <sho...@gmail.com> wrote:
> >
> > From: Ahmad Fatoum <a.fat...@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 two things in the device tree:
> >
> > 1. Define stdout-path only one time for the first registerd UART
>
> "registered"
OK
> > 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".
>
> I would recommend for maximum backwards compatibility also changing
> one other thing. With this patch, the UARTs are listed in the
> device tree starting with the one with the highest address and
> working down. You can see this if you run
> qemu-system-or1k -M or1k-sim -machine dumpdtb=or1k.dtb -kernel /dev/null
> and then
> dtc -I dtb -O dts /or1k.dtb |less
> -- the output shows that "serial@90000300" is first.
>
> This happens because (due to an implementation quirk that I forget
> the details of) nodes we add to the DTB in QEMU end up being listed
> in reverse order of creation. I would recommend making the
> UART-creation loop in openrisc_sim_init() run backwards rather
> than forwards, so that the nodes end up in the DTB in ascending order.
>
> This should not affect any guests that do the "right thing" for
> finding their UART, i.e. look at stdout-path or at the UART alias
> nodes; but for Arm we found that at least some guest code had been
> written to just find the first UART node in the dtb and use that.
>
> (I suspect, incidentally, that this is the reason why 777784bda468
> was using "serial_hd(OR1KSIM_UART_COUNT - uart_idx - 1)" -- it was
> trying to fix this but didn't quite put the change in the right place.)
>
> That would correspond to squashing in this change on top of your patch:
>
> --- a/hw/openrisc/openrisc_sim.c
> +++ b/hw/openrisc/openrisc_sim.c
> @@ -329,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);
OK it makes sense, I will add this as well.
>
> > [1]:
> > https://lore.barebox.org/barebox/707e7c50-aad1-4459-8796-0cc54bab3...@pengutronix.de/T/#m5da26e8a799033301489a938b5d5667b81cef6ad
> >
> > Fixes: 777784bda468 ("hw/openrisc: support 4 serial ports in or1ksim")
> > Signed-off-by: Ahmad Fatoum <a.fat...@pengutronix.de>
> > [stafford: Change to serial0 alias and update change message]
> > Signed-off-by: Stafford Horne <sho...@gmail.com>
> > ---
> > hw/openrisc/openrisc_sim.c | 13 ++++++++-----
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c
> > index 9fb63515ef..5ec9172ccf 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")];
>
> Using g_strdup_printf() (and a g_autofree pointer) is better than
> a fixed-size array; but I guess we don't really need to clean
> that up in this patch.
I will leave this as is.
> > 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);
> > }
>
> thanks
Thanks,
-Stafford
