[PATCH v2 0/2] Misc OpenRISC fixes for 9.2.0

[PATCH v2 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.

Since v1:
 - Use DIVIDE_ROUND_UP instead of open coding as pointed out by Richard
 - Fix off-by-1 bug in TTCR patch pointed out by Richard
 - Fix commit message and reverse registration order as pointed out by Peter.

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 | 26 ++++++++++++++++++++------
 2 files changed, 35 insertions(+), 17 deletions(-)

-- 
2.47.0

[PATCH v2 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 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".

[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>
---
Since v1:
 - Fix commit message and reverse registration order as pointed out by Peter.

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

[PATCH v2 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>
---
Since v1:
 - Use DIVIDE_ROUND_UP instead of open coding as pointed out by Richard
 - Fix off-by-1 bug in TTCR patch pointed out by Richard

 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..87aa353323 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 +
+        DIV_ROUND_UP(now - or1k_timer->clk_offset, 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 -= (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 v2 0/2] Misc OpenRISC fixes for 9.2.0

[Peter Maydell]

On Sun, 1 Dec 2024 at 07:11, Stafford Horne <shorne@gmail.com> wrote:
>
> This series has 2 fixes for OpenRISC that came in over that past few months.
>
> Since v1:
>  - Use DIVIDE_ROUND_UP instead of open coding as pointed out by Richard
>  - Fix off-by-1 bug in TTCR patch pointed out by Richard
>  - Fix commit message and reverse registration order as pointed out by Peter.
>
> Ahmad Fatoum (1):
>   hw/openrisc/openrisc_sim: keep serial@90000000 as default
>
> Joel Holdsworth (1):
>   hw/openrisc: Fixed undercounting of TTCR in continuous mode

Note that since rc3 is due to be tagged tomorrow, fixes
targeting 9.2 need to be in a pullreq on the list by
Tuesday lunchtime-ish UK time (or if not possible, at least
let me know something is going to be late arriving).

thanks
-- PMM

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

[Peter Maydell]

On Sun, 1 Dec 2024 at 07:11, 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:

three

>  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. Create the UART nodes so they appear in the final DTB in the
     order starting with the lowest address and working upwards.
     (Linux doesn't care about this but some DTB consumers might.)


If you like you could also add something like:

 These changes mean that:
  * 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)

as a summary.

> [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>
> ---
> Since v1:
>  - Fix commit message and reverse registration order as pointed out by Peter.
>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

[Stafford Horne]

On Mon, Dec 02, 2024 at 04:54:11PM +0000, Peter Maydell wrote:
> On Sun, 1 Dec 2024 at 07:11, Stafford Horne <shorne@gmail.com> wrote:
> >
> > This series has 2 fixes for OpenRISC that came in over that past few months.
> >
> > Since v1:
> >  - Use DIVIDE_ROUND_UP instead of open coding as pointed out by Richard
> >  - Fix off-by-1 bug in TTCR patch pointed out by Richard
> >  - Fix commit message and reverse registration order as pointed out by Peter.
> >
> > Ahmad Fatoum (1):
> >   hw/openrisc/openrisc_sim: keep serial@90000000 as default
> >
> > Joel Holdsworth (1):
> >   hw/openrisc: Fixed undercounting of TTCR in continuous mode
> 
> Note that since rc3 is due to be tagged tomorrow, fixes
> targeting 9.2 need to be in a pullreq on the list by
> Tuesday lunchtime-ish UK time (or if not possible, at least
> let me know something is going to be late arriving).

Hi Peter,

Thanks for the heads up.  I don't keep track of the schedule so this is helpful.
I will try to get my v3 series posted and the followup GIT PULL right now.

I am based in UK so there is a bit of a clash with my normal working hours, so
trying to get this taken care of on my coffee break.

-Stafford