[PATCH 0/2] Misc OpenRISC fixes for 9.2.0

[PATCH 0/2] Misc OpenRISC fixes for 9.2.0

[Stafford Horne]

This series has 2 fixes for OpenRISC that came in over that past few months.

Ahmad Fatoum (1):
  hw/openrisc/openrisc_sim: keep serial@90000000 as default

Joel Holdsworth (1):
  hw/openrisc: Fixed undercounting of TTCR in continuous mode

 hw/openrisc/cputimer.c     | 26 +++++++++++++++-----------
 hw/openrisc/openrisc_sim.c | 13 ++++++++-----
 2 files changed, 23 insertions(+), 16 deletions(-)

-- 
2.47.0

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

[Stafford Horne]

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 two things in the device tree:

 1. Define stdout-path only one time for the first registerd 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".

[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")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
[stafford: Change to serial0 alias and update change message]
Signed-off-by: Stafford Horne <shorne@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")];
     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);
 }
 
-- 
2.47.0

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

[Peter Maydell]

On Sat, 23 Nov 2024 at 10:39, Stafford Horne <shorne@gmail.com> wrote:
>
> 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 two things in the device tree:
>
>  1. Define stdout-path only one time for the first registerd UART

"registered"

>     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);


> [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")
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> [stafford: Change to serial0 alias and update change message]
> Signed-off-by: Stafford Horne <shorne@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.

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

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

[Stafford Horne]

On Mon, Nov 25, 2024 at 02:02:35PM +0000, Peter Maydell wrote:
> On Sat, 23 Nov 2024 at 10:39, Stafford Horne <shorne@gmail.com> wrote:
> >
> > 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 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-0cc54bab32e2@pengutronix.de/T/#m5da26e8a799033301489a938b5d5667b81cef6ad
> >
> > Fixes: 777784bda468 ("hw/openrisc: support 4 serial ports in or1ksim")
> > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> > [stafford: Change to serial0 alias and update change message]
> > Signed-off-by: Stafford Horne <shorne@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

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

[Stafford Horne]

On Sun, Dec 01, 2024 at 06:44:39AM +0000, Stafford Horne wrote:
> On Mon, Nov 25, 2024 at 02:02:35PM +0000, Peter Maydell wrote:
> > On Sat, 23 Nov 2024 at 10:39, Stafford Horne <shorne@gmail.com> wrote:
> > >
> > > 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 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.

Hi Peter,

One comment on this, in the commit message from Ahmad he says.  The registration
order... ''This caused QEMU to pick 0x90000300 as the default UART,''

But you mentioned the lowest address will be used by QEMU.  I was not able to
find the code so easily that confirmed either of these statements.

It seems there is some contradiction to the original commit message from Ahamad,
but I will leave the commit message as is now.

Perhaps it's the value of serial_hd(x) that need to be the lowest.

-Stafford

> > 
> > > [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")
> > > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> > > [stafford: Change to serial0 alias and update change message]
> > > Signed-off-by: Stafford Horne <shorne@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

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

[Peter Maydell]

On Sun, 1 Dec 2024 at 06:57, Stafford Horne <shorne@gmail.com> wrote:
>
> On Sun, Dec 01, 2024 at 06:44:39AM +0000, Stafford Horne wrote:
> > On Mon, Nov 25, 2024 at 02:02:35PM +0000, Peter Maydell wrote:
> > > On Sat, 23 Nov 2024 at 10:39, Stafford Horne <shorne@gmail.com> wrote:
> > > >
> > > > 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 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.
>
> Hi Peter,
>
> One comment on this, in the commit message from Ahmad he says.  The registration
> order... ''This caused QEMU to pick 0x90000300 as the default UART,''

The thing that made QEMU pick 0x90000300 as the first serial
terminal is that the current code
 (1) loops through calling openrisc_sim_serial_init() starting
     lowest address first and
 (2) calls "serial_hd(OR1KSIM_UART_COUNT - uart_idx - 1)"
     to get the QEMU chardev.

If you do that then the uart that gets serial_hd(0) is 0x90000300.
This is completely unrelated to the order of nodes in the dtb,
or to any logic that the guest uses to select a UART: it's what
means that if the user only passes one "-serial foo" that's the
UART they're configuring. It happens that if the guest code
is doing the "pick the first node in the DTB" then the current
code will give them the UART that QEMU thinks of as the "first"
serial port. But if the guest code is doing any of a bunch of
other reasonable things then it will pick a UART that isn't what
QEMU is currently calling the "first" serial port.

 We want all of these to be true:
 * 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)
   (and then up in ascending order for serial1, etc)

(I would probably reword the commit message a bit to make it
clear that this is what we're trying to achieve and what we
think the commit does.)

thanks
-- PMM

[PATCH 2/2] hw/openrisc: Fixed undercounting of TTCR in continuous mode

[Stafford Horne]

From: Joel Holdsworth <jholdsworth@nvidia.com>

In the existing design, TTCR is prone to undercounting when running in
continuous mode. This manifests as a timer interrupt appearing to
trigger a few cycles prior to the deadline set in SPR_TTMR_TP.

When the timer triggers, the virtual time delta in nanoseconds between
the time when the timer was set, and when it triggers is calculated.
This nanoseconds value is then divided by TIMER_PERIOD (50) to compute
an increment of cycles to apply to TTCR.

However, this calculation rounds down the number of cycles causing the
undercounting.

A simplistic solution would be to instead round up the number of cycles,
however this will result in the accumulation of timing error over time.

This patch corrects the issue by calculating the time delta in
nanoseconds between when the timer was last reset and the timer event.
This approach allows the TTCR value to be rounded up, but without
accumulating error over time.

Signed-off-by: Joel Holdsworth <jholdsworth@nvidia.com>
[stafford: Incremented version in vmstate_or1k_timer, checkpatch fixes]
Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 hw/openrisc/cputimer.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/hw/openrisc/cputimer.c b/hw/openrisc/cputimer.c
index 835986c4db..7fb97ce2b0 100644
--- a/hw/openrisc/cputimer.c
+++ b/hw/openrisc/cputimer.c
@@ -29,7 +29,8 @@
 /* Tick Timer global state to allow all cores to be in sync */
 typedef struct OR1KTimerState {
     uint32_t ttcr;
-    uint64_t last_clk;
+    uint32_t ttcr_offset;
+    uint64_t clk_offset;
 } OR1KTimerState;
 
 static OR1KTimerState *or1k_timer;
@@ -37,6 +38,8 @@ static OR1KTimerState *or1k_timer;
 void cpu_openrisc_count_set(OpenRISCCPU *cpu, uint32_t val)
 {
     or1k_timer->ttcr = val;
+    or1k_timer->ttcr_offset = val;
+    or1k_timer->clk_offset = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
 }
 
 uint32_t cpu_openrisc_count_get(OpenRISCCPU *cpu)
@@ -53,9 +56,8 @@ void cpu_openrisc_count_update(OpenRISCCPU *cpu)
         return;
     }
     now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-    or1k_timer->ttcr += (uint32_t)((now - or1k_timer->last_clk)
-                                    / TIMER_PERIOD);
-    or1k_timer->last_clk = now;
+    or1k_timer->ttcr = or1k_timer->ttcr_offset +
+        (now - or1k_timer->clk_offset + TIMER_PERIOD - 1) / TIMER_PERIOD;
 }
 
 /* Update the next timeout time as difference between ttmr and ttcr */
@@ -69,7 +71,7 @@ void cpu_openrisc_timer_update(OpenRISCCPU *cpu)
     }
 
     cpu_openrisc_count_update(cpu);
-    now = or1k_timer->last_clk;
+    now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
 
     if ((cpu->env.ttmr & TTMR_TP) <= (or1k_timer->ttcr & TTMR_TP)) {
         wait = TTMR_TP - (or1k_timer->ttcr & TTMR_TP) + 1;
@@ -110,7 +112,8 @@ static void openrisc_timer_cb(void *opaque)
     case TIMER_NONE:
         break;
     case TIMER_INTR:
-        or1k_timer->ttcr = 0;
+        /* Zero the count by applying a negative offset to the counter */
+        or1k_timer->ttcr_offset += UINT32_MAX - (cpu->env.ttmr & TTMR_TP);
         break;
     case TIMER_SHOT:
         cpu_openrisc_count_stop(cpu);
@@ -137,17 +140,18 @@ static void openrisc_count_reset(void *opaque)
 /* Reset the global timer state. */
 static void openrisc_timer_reset(void *opaque)
 {
-    or1k_timer->ttcr = 0x00000000;
-    or1k_timer->last_clk = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    OpenRISCCPU *cpu = opaque;
+    cpu_openrisc_count_set(cpu, 0);
 }
 
 static const VMStateDescription vmstate_or1k_timer = {
     .name = "or1k_timer",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (const VMStateField[]) {
         VMSTATE_UINT32(ttcr, OR1KTimerState),
-        VMSTATE_UINT64(last_clk, OR1KTimerState),
+        VMSTATE_UINT32(ttcr_offset, OR1KTimerState),
+        VMSTATE_UINT64(clk_offset, OR1KTimerState),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.47.0

Re: [PATCH 2/2] hw/openrisc: Fixed undercounting of TTCR in continuous mode

[Richard Henderson]

On 11/23/24 04:38, Stafford Horne wrote:
> +    or1k_timer->ttcr = or1k_timer->ttcr_offset +
> +        (now - or1k_timer->clk_offset + TIMER_PERIOD - 1) / TIMER_PERIOD;

Better using DIV_ROUND_UP.

> +        /* Zero the count by applying a negative offset to the counter */
> +        or1k_timer->ttcr_offset += UINT32_MAX - (cpu->env.ttmr & TTMR_TP);

Since UINT32_MAX is -1 in this context, this appears to be off-by-one.
I think -(ttmr & mask) alone is correct.


r~

Re: [PATCH 2/2] hw/openrisc: Fixed undercounting of TTCR in continuous mode

[Stafford Horne]

On Sat, Nov 23, 2024 at 07:39:57AM -0600, Richard Henderson wrote:
> On 11/23/24 04:38, Stafford Horne wrote:
> > +    or1k_timer->ttcr = or1k_timer->ttcr_offset +
> > +        (now - or1k_timer->clk_offset + TIMER_PERIOD - 1) / TIMER_PERIOD;
> 
> Better using DIV_ROUND_UP.

Sure, I can change it to that.

> > +        /* Zero the count by applying a negative offset to the counter */
> > +        or1k_timer->ttcr_offset += UINT32_MAX - (cpu->env.ttmr & TTMR_TP);
> 
> Since UINT32_MAX is -1 in this context, this appears to be off-by-one.
> I think -(ttmr & mask) alone is correct.

Thanks, I did send a mail to Joel asking about this bit.  He didn't respond for 2
weeks to I just sent the patch as is as it appears to work.  As I understand,
yes UINT32_MAX is just -1.  But why the -1?  I guess it's because after
ttcr_offset is updated we call cpu_openrisc_timer_update() which calls
cpu_openrisc_count_update() to update ttcr.  Since a few _ns would have passed
and we are rounding up it will update ttcr to 0.

But maybe I am reading too much into it.

If you think that makes sense I could add a comment as such, also I would prefer
to change to UINT32_MAX to -1.

-Stafford

Re: [PATCH 2/2] hw/openrisc: Fixed undercounting of TTCR in continuous mode

[Richard Henderson]

On 11/23/24 11:11, Stafford Horne wrote:
> On Sat, Nov 23, 2024 at 07:39:57AM -0600, Richard Henderson wrote:
>> On 11/23/24 04:38, Stafford Horne wrote:
>>> +    or1k_timer->ttcr = or1k_timer->ttcr_offset +
>>> +        (now - or1k_timer->clk_offset + TIMER_PERIOD - 1) / TIMER_PERIOD;
>>
>> Better using DIV_ROUND_UP.
> 
> Sure, I can change it to that.
> 
>>> +        /* Zero the count by applying a negative offset to the counter */
>>> +        or1k_timer->ttcr_offset += UINT32_MAX - (cpu->env.ttmr & TTMR_TP);
>>
>> Since UINT32_MAX is -1 in this context, this appears to be off-by-one.
>> I think -(ttmr & mask) alone is correct.
> 
> Thanks, I did send a mail to Joel asking about this bit.  He didn't respond for 2
> weeks to I just sent the patch as is as it appears to work.  As I understand,
> yes UINT32_MAX is just -1.  But why the -1?  I guess it's because after
> ttcr_offset is updated we call cpu_openrisc_timer_update() which calls
> cpu_openrisc_count_update() to update ttcr.  Since a few _ns would have passed
> and we are rounding up it will update ttcr to 0.
> 
> But maybe I am reading too much into it.

I think you're reading too much into it -- I just think it's a bug which isn't 
particularly noticeable because the clock is only off by 1ns.


r~

> 
> If you think that makes sense I could add a comment as such, also I would prefer
> to change to UINT32_MAX to -1.
> 
> -Stafford

Re: [PATCH 0/2] Misc OpenRISC fixes for 9.2.0

[Michael Tokarev]

On 23.11.2024 13:38, Stafford Horne wrote:
> This series has 2 fixes for OpenRISC that came in over that past few months.
> 
> Ahmad Fatoum (1):
>    hw/openrisc/openrisc_sim: keep serial@90000000 as default
> 
> Joel Holdsworth (1):
>    hw/openrisc: Fixed undercounting of TTCR in continuous mode

Is there anything in there which is worth picking up for stable?
The first one above seems to be a good candidate.

Thanks,

/mjt

Re: [PATCH 0/2] Misc OpenRISC fixes for 9.2.0

[Stafford Horne]

On Sat, Nov 23, 2024 at 03:12:12PM +0300, Michael Tokarev wrote:
> On 23.11.2024 13:38, Stafford Horne wrote:
> > This series has 2 fixes for OpenRISC that came in over that past few months.
> > 
> > Ahmad Fatoum (1):
> >    hw/openrisc/openrisc_sim: keep serial@90000000 as default
> > 
> > Joel Holdsworth (1):
> >    hw/openrisc: Fixed undercounting of TTCR in continuous mode
> 
> Is there anything in there which is worth picking up for stable?
> The first one above seems to be a good candidate.

Yes, I think so, would you like me to tag it with anything?  It does have a
Fixes.

-Stafford

Re: [PATCH 0/2] Misc OpenRISC fixes for 9.2.0

[Michael Tokarev]

23.11.2024 20:01, Stafford Horne wrote:
> On Sat, Nov 23, 2024 at 03:12:12PM +0300, Michael Tokarev wrote:
>> On 23.11.2024 13:38, Stafford Horne wrote:
>>> This series has 2 fixes for OpenRISC that came in over that past few months.
>>>
>>> Ahmad Fatoum (1):
>>>     hw/openrisc/openrisc_sim: keep serial@90000000 as default
>>>
>>> Joel Holdsworth (1):
>>>     hw/openrisc: Fixed undercounting of TTCR in continuous mode
>>
>> Is there anything in there which is worth picking up for stable?
>> The first one above seems to be a good candidate.
> 
> Yes, I think so, would you like me to tag it with anything?  It does have a
> Fixes.

Well, there are Fixes and Fixes.  Some are much more important to be
in stable series, while others are hardly relevant.  Obviously you're
much better to judge in your area than others.  After all, maybe  you're
the only one in this world to run or1k, and you don't need stable
series at all :)

 From the two fixes in this series, I (with my very limited understanding)
see first one above is maybe good for stable, while for second I don't know.
So I entirely rely on your call, hence I asked.

As for tagging, you might add Cc: qemu-stable@ tag for all changes which
you  want to be back-ported, or just send a note to qemu-stabl@ with the
changes you'd like to be in stable (note: this email has Cc: there already).
The key point is to make qemu-stable@ aware of what do you think is relevant.

Thanks,

/mjt

Re: [PATCH 0/2] Misc OpenRISC fixes for 9.2.0

[Stafford Horne]

On Sun, Nov 24, 2024 at 08:03:53AM +0300, Michael Tokarev wrote:
> 23.11.2024 20:01, Stafford Horne wrote:
> > On Sat, Nov 23, 2024 at 03:12:12PM +0300, Michael Tokarev wrote:
> > > On 23.11.2024 13:38, Stafford Horne wrote:
> > > > This series has 2 fixes for OpenRISC that came in over that past few months.
> > > > 
> > > > Ahmad Fatoum (1):
> > > >     hw/openrisc/openrisc_sim: keep serial@90000000 as default
> > > > 
> > > > Joel Holdsworth (1):
> > > >     hw/openrisc: Fixed undercounting of TTCR in continuous mode
> > > 
> > > Is there anything in there which is worth picking up for stable?
> > > The first one above seems to be a good candidate.
> > 
> > Yes, I think so, would you like me to tag it with anything?  It does have a
> > Fixes.
> 
> Well, there are Fixes and Fixes.  Some are much more important to be
> in stable series, while others are hardly relevant.  Obviously you're
> much better to judge in your area than others.  After all, maybe  you're
> the only one in this world to run or1k, and you don't need stable
> series at all :)

There are a few more people running or1k qemu for itegration testing of various
ports and every once in a while I get someone coming out of the woodwork using
or1k for real world stuff.

But sooner or later it may only be me.

:)

> From the two fixes in this series, I (with my very limited understanding)
> see first one above is maybe good for stable, while for second I don't know.
> So I entirely rely on your call, hence I asked.
> 
> As for tagging, you might add Cc: qemu-stable@ tag for all changes which
> you  want to be back-ported, or just send a note to qemu-stabl@ with the
> changes you'd like to be in stable (note: this email has Cc: there already).
> The key point is to make qemu-stable@ aware of what do you think is relevant.

Thanks for this.  I will add Cc: qemu-stable in the v2 of the series.

-Stafford