Rework HAL to use the auto-generated register interface by ziuziakowska · Pull Request #299 · lowRISC/mocha

ziuziakowska · 2026-02-17T18:39:09Z

This updates and reworks the HAL for all devices to use the auto-generated register interface in #290. ~~First few commits are from that PR.~~

Changes:

Standardise interrupt handling between all devices. Apart from the timer which only has one interrupt, all devices deal with the enum bit-vectors of interrupts, which allows enabling or disabling sets of interrupts at once, as well as polling for all of a set of interrupts or any of a set of interrupts. The new library interface is as follows:
- device_interrupt_enable_get - get the set of enabled interrupts.
- device_interrupt_enable_set - set the set of enabled interrupts.
- device_interrupt_enable - add interrupts to the set of enabled interrupts (logical or).
- device_interrupt_disable - remove interrupts from the set of enabled interrupts (logical and with negated argument).
- device_interrupt_disable_all - disable all interrupts (equivalent to enable with 0).
- device_interrupt_force - force/trigger a set of interrupts.
- device_interrupt_clear - clear a set of interrupts.
- device_interrupt_all_pending - return true whenever all of a set of interrupts are pending.
- device_interrupt_any_pending - return true whenever any of a set of interrupts is pending.
The system PLIC interrupt map is added to mocha.h, with the IRQs for the SPI Device (interrupt 7) and the UART (interrupt 8).
The timer is configured to use a 1 microsecond standardised tick rate, as that is what all tests were using. The functions timer_schedule_in_us, timer_schedule_in_ms, timer_busy_sleep_us, and timer_busy_sleep_ms are provided to schedule an interrupt in the future and busy wait until some time in the future. The functions us_to_cycles and cycles_to_us are also provided in mocha.h to convert to/from system clock cycles and microseconds.
TODO: Document SPI Device changes.
The DV test status "device" is not present in the SystemRDL yet, so a manually written interface in a similar form to the auto-generator was added to the HAL.
The mocha_system_device functions in mocha.h use the size of the device in memory from the auto-generated output (sizeof(struct device_memory_layout)) to determine the bounds in CHERI mode.
The uint32_t-specific DEV_READ, DEV_WRITE were replaced with more type-generic volatile_read and volatile_write macros.

I've tested locally on all optimisation levels up to -O3 to make sure that MMIO accesses do not get optimised into smaller-sized operations.

Regressions:
- The tests timer_smoketest_cheri_sim_verilator and timer_interrupt_test_cheri_sim_verilator are currently failing due to an assertion being hit in the AXI SRAM. Removing the assertion does not cause any exceptions to occur and the tests continue as normal, and at higher optimisation levels than -O0, the assertions are sometimes not triggered. Edit: No longer failing.

28100: (src/lowrisc_dv_sw_test_status_0/sw_test_status_if.sv:61) [TOP.top_chip_verilator.u_sw_test_status_if] SW test transitioned to SwTestStatusInTest.
ERROR: Partial capability read not supported!
[46052] %Error: axi_sram.sv:112: Assertion failed in TOP.top_chip_verilator.u_top_chip_system.u_axi_sram.NoPartialCapRead: 46052: (src/lowrisc_mocha_top_chip_system_0/rtl/axi_sram.sv:112) [TOP.top_chip_verilator.u_top_chip_system.u_axi_sram.NoPartialCapRead] [ASSERT FAILED] NoPartialCapRead
%Error: src/lowrisc_mocha_top_chip_system_0/rtl/axi_sram.sv:112: Verilog $stop
Aborting...

engdoreis

I gave a first pass, the generated c headers looks good for me. But I left a few comments to be addressed. I will give another pass later.

sw/device/lib/hal/mmio.h

sw/device/examples/hello_world/hello_world.c

sw/device/lib/hal/mocha.c

engdoreis · 2026-02-26T14:51:49Z

sw/device/lib/hal/uart.c

+    uart_ctrl ctrl = volatile_read(uart->ctrl);
+    ctrl.tx = true;
+    ctrl.rx = true;
+    ctrl.nco = (((uint64_t)BAUD_RATE << 20) / SYSCLK_FREQ);


Since this is a new HAL, the baud and the clock should be arguments.

I think that if we are mostly going to be using 1M baud rate on this system, test writers should just be able to call init without having to think about it too much.

The plan is the get the uart to V2, which means that we need to test different bauds, see in OT. It's also possible that as the design grows, we won't be able to keep the same system clock for all the platforms, so these things should not be in the HAL.

I think there should be some higher-level "do what I mean" functions in the HAL, as this will also form the library that all tests are written with. So far all executables have used the same baud rate, so I think it would be nicer if users had a function that provides them with a reasonable default setting for the majority of cases. I can provide another function that takes in a parameter like one of the kBauds for more control when we get there (and there isn't anything stopping users from manually initialising the UART themselves, should they need to).

Based on this discussion, I would maybe suggest an approach where we have a uart_init / uart_configure function that takes the various possible parameters (e.g. baud rate, clock frequency), and then just expose an additional helper function that wraps this with a default configuration?

But I will also point out that for test code specifically, despite the increased verbosity, there is some value in forcing users of the HAL to be explicit with each of their parameters. Default values can more easily lead to missed corner cases and parameters left unconsidered.

I also realize that this code was already just using the constants though, so maybe it is fine to leave it for this PR an open an issue to track improving this interface, rather than addressing everything in this PR?

I'm fine to leave as is in this PR. But I would like to highlight that this HAL is equivalent to the Opentitan DIFs, so, It should be designed for testing allowing tests to configure the HW in all possible ways.

sw/device/lib/hal/uart.c

engdoreis · 2026-02-26T14:55:23Z

sw/device/lib/hal/uart.c

-        DEV_WRITE(uart + UART_INTR_ENABLE_REG,
-                  DEV_READ(uart + UART_INTR_ENABLE_REG) & ~(1 << intr_id));
-    }
+    volatile_write(uart->intr_enable, intrs);


In Opentitan the generator also produces a mask for every register which can be used to check that the value is in range.

This should not be necessary for the flag enums, as you should only construct them by or-ing the variants that are by-definition valid bits to set/get. I would think hardware registers also do not use any reserved bits even if they are written to.

IIRC, generally in C enum is just a suggestion (even for flag enums) and the underlying type is always an integral value. So for example there is nothing stopping some bad code from accidentally writing a value of intrs = (uart_intr)(1u << 20) even though that isn't defined as any combination of the uart_intr enum flags.

And while I generally agree that hardware registers probably won't care, it is good practice from a software standpoint to error early when a problem is detected. That way if a user accidentally passes the wrong value, shifts some interrupts wrong, etc. they get an error rather than hardware silently accepting their malformed instruction.

IIRC, generally in C enum is just a suggestion (even for flag enums) and the underlying type is always an integral value. So for example there is nothing stopping some bad code from accidentally writing a value of intrs = (uart_intr)(1u << 20) even though that isn't defined as any combination of the uart_intr enum flags.

Unfortunately this is the problem with C when trying to design type-safe interfaces - they can just be bypassed with some work ☹️. For what it's worth, without the explicit cast this would be picked up as a warning by clang-tidy and fail CI. I think the best we can do is to model our interfaces in a useful way such that trying to override them like this is an immediate code-smell and subject to more scrutiny during review (and there may be niche valid reasons to override this behaviour, such as very fundamental software tests that check that register fields do not allow writes to read-only fields).

And while I generally agree that hardware registers probably won't care, it is good practice from a software standpoint to error early when a problem is detected. That way if a user accidentally passes the wrong value, shifts some interrupts wrong, etc. they get an error rather than hardware silently accepting their malformed instruction.

At the moment there is no panic-like behaviour or runtime assertions in the library/test framework, so the only thing that could be done is the write could be skipped, which is still a silent failure-mode.

I'm not opposed to this idea in general and I think it could be done for the "safe" HAL interface, but it will need more auto-gen work to generate masks for these registers and that would drag this PR along for longer 😅. I guess if you really do need to read/write raw values, you can use volatile_read/volatile_write.

and there may be niche valid reasons to override this behaviour, such as very fundamental software tests that check that register fields do not allow writes to read-only fields

FWIW in OpenTitan I think historically these kinds of tests have always been done only in DV anyway. I'm not sure if the plan is to do the same in mocha. It would certainly be possible to autogen a large SW test for this based on the register HJSON descriptions but I'm not sure how worth it that would be.

At the moment there is no panic-like behaviour or runtime assertions in the library/test framework, so the only thing that could be done is the write could be skipped, which is still a silent failure-mode.

I'm not opposed to this idea in general and I think it could be done for the "safe" HAL interface, but it will need more auto-gen work to generate masks for these registers and that would drag this PR along for longer 😅.

That makes sense - I wasn't aware that there was no panic/assert in the framework currently! So yes it would be a bit more work. I think that would be too much to require from this PR, so maybe this sounds like a good target to open an issue for instead?

marnovandermaas

As discussed I'm happy with you doing a software workaround on the issue of the capability assertion you mention. Probably good to create an issue to track that activity so we don't forget to fix it eventually.

marnovandermaas

Thank your for your work here. Some of the changes in this PR are quite nice, like using the auto-generated headers and moving the dv test to the HAL. The main problem I see with this PR is that it also causes a lot of code churn based on renaming and reorganising with essentially the same functionality afterwards. I am unsure whether that is a good idea this close to the initial release. Let's wait until Douglas is back to decide on whether this should go in or whether introducing the changes in a way that causes less code churn is a better approach. I'll mark this as "request changes" in the meantime.

sw/device/lib/hal/mmio.h

marnovandermaas · 2026-03-12T13:26:10Z

sw/device/lib/hal/mmio.h

 #define DEV_READ64(addr) (*((volatile uint64_t *)(addr)))
+
+#define volatile_read(reg)       (*((volatile __typeof((reg)) *)&(reg)))
+#define volatile_write(reg, val) (*((volatile __typeof((reg)) *)&(reg)) = (__typeof((reg)))(val))


This commit by itself is hard to review without its use. It's probably good to squash this with where you initially used this definition.

sw/device/lib/hal/mocha.c

sw/device/lib/hal/mocha.h

marnovandermaas · 2026-03-12T13:35:37Z

sw/device/lib/test_framework/main.c

-    /* 
-     * We don't have a hardware system reset yet. So we workaround by jumping back to the bootROM.
-     */
+    /* We don't have a hardware system reset yet. So we workaround by
+     * jumping back to the bootROM. */


Why are you making these changes? I prefer not to do this code churn unless it is automatically checked in the formatter. This also applies to the other comment changes in this file.

marnovandermaas · 2026-03-12T13:37:09Z

sw/device/lib/hal/uart.h


 #pragma once

+#include "autogen/uart.h"


Where is this file? Is this automatically generated by cmake?

These files are generated from the RDL and committed to the repo: 6cd92a7

marnovandermaas · 2026-03-12T13:44:54Z

sw/device/tests/plic/smoketest.c

    plic_interrupt_priority_set(plic, UART_INTR_ID, 3);
    plic_machine_priority_threshold_set(plic, 0);

-    uart_interrupt_disable_all(uart);


Why is it ok to remove the disable all?

marnovandermaas · 2026-03-12T13:45:51Z

sw/device/lib/hal/mocha.c

 {
 #if defined(__riscv_zcherihybrid)
-    return (timer_t)create_mmio_capability(timer_base, 0x120u);
+    return (timer_t)create_mmio_capability(timer_base, sizeof(struct timer_memory_layout));


Where can I find this struct?

The are the auto-generated structures in sw/device/lib/hal/autogen as added in 6cd92a7.

marnovandermaas · 2026-03-12T13:48:34Z

sw/device/bootrom/bootrom.c

-    uprintf(console, "\nJumping to: 0x%0x\n", BootAddress);
+    uprintf(console, "\nJumping to: 0x%x\n", BootAddress);


Why did you remove the prepending zeroes?

I missed this in the original bootrom PR - the printf implementation doesn't support prefixing the format specifier to zero-pad the value, so this would print the literal string Jumping to: 0x%x. Currently the numeric format specifiers always pad with 0s as it made the implementation much simpler.

marnovandermaas · 2026-03-12T13:50:48Z

sw/device/bootrom/bootrom.c

    while (true) {
-        spi_device_cmd_t cmd = spi_device_cmd_get(spid);
-        if (cmd.status != 0) {
+        spi_device_software_command command;


A bit of code churn here to remove the _t and expand from cmd to command. I'm unsure whether this is necessary.

It isn't necessary, but I would prefer we have more descriptive type names. Removing the _t suffix also keeps everything in line with the auto-generated register types (spi_device_intr, spi_device_flash_status, etc.)

AlexJones0

I've only partially reviewed the first couple of commits for now - just leaving my comments in case they are helpful.

sw/device/lib/hal/mmio.h

AlexJones0 · 2026-03-12T12:30:25Z

sw/device/lib/hal/uart.c

+    uart_ctrl ctrl = volatile_read(uart->ctrl);
+    ctrl.tx = true;
+    ctrl.rx = true;
+    ctrl.nco = (((uint64_t)BAUD_RATE << 20) / SYSCLK_FREQ);


Based on this discussion, I would maybe suggest an approach where we have a uart_init / uart_configure function that takes the various possible parameters (e.g. baud rate, clock frequency), and then just expose an additional helper function that wraps this with a default configuration?

But I will also point out that for test code specifically, despite the increased verbosity, there is some value in forcing users of the HAL to be explicit with each of their parameters. Default values can more easily lead to missed corner cases and parameters left unconsidered.

I also realize that this code was already just using the constants though, so maybe it is fine to leave it for this PR an open an issue to track improving this interface, rather than addressing everything in this PR?

sw/device/lib/hal/uart.c

AlexJones0 · 2026-03-12T12:39:05Z

sw/device/lib/hal/uart.c

-        DEV_WRITE(uart + UART_INTR_ENABLE_REG,
-                  DEV_READ(uart + UART_INTR_ENABLE_REG) & ~(1 << intr_id));
-    }
+    volatile_write(uart->intr_enable, intrs);


IIRC, generally in C enum is just a suggestion (even for flag enums) and the underlying type is always an integral value. So for example there is nothing stopping some bad code from accidentally writing a value of intrs = (uart_intr)(1u << 20) even though that isn't defined as any combination of the uart_intr enum flags.

And while I generally agree that hardware registers probably won't care, it is good practice from a software standpoint to error early when a problem is detected. That way if a user accidentally passes the wrong value, shifts some interrupts wrong, etc. they get an error rather than hardware silently accepting their malformed instruction.

sw/device/lib/hal/uart.c

AlexJones0 · 2026-03-12T13:13:03Z

sw/device/lib/hal/uart.c

+    do {
+        status = volatile_read(uart->status);
+    } while (status & uart_status_rxempty);


Note: I'm not too familiar with how the HALs are being used in mocha currently (lacking some context here).

I think it's good to have a blocking uart_in interface, but for test purposes I'd imagine we also want a non-blocking read interface, something like

size_t uart_read(uart_t uart, size_t bytes, uint8_t *data);

that would then read into the buffer and return the number of bytes read, non-blocking (just reading until either the number of requested bytes is filled, or the RX buffer is empty).

At the moment all the usage of the UART is through this blocking interface. Non-blocking interfaces will be really nice and useful in the future, but I think there needs to be some discussion and experimenting on how we use and setup other features of the UART (such as the transmit/receive FIFO functionality) first, before we can implement some more advanced interfaces.

engdoreis · 2026-03-20T16:18:41Z

I think this PR is too ambitious; it attempts to change too many things (including some controversial ones), which makes it hard to review. I suggest creating a separate PR implementing only the HAL basics so we can quickly review and merge it before the release (on Monday). The remaining changes should be split into different PRs to be reviewed over more time.

marnovandermaas · 2026-03-22T11:30:07Z

I've converted this PR to draft so we can have time to split it up into separate PRs.

Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

also change the naming convention to match PLIC Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

the DV test status "device" is currently missing from RDL autogeneration, so this creates the minimal structure manually Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

ziuziakowska mentioned this pull request Feb 17, 2026

Auto-generate device register C headers from SystemRDL description #290

Merged

ziuziakowska force-pushed the hal-rework-autogen branch 6 times, most recently from d9e4de1 to b7a55c7 Compare February 24, 2026 14:04

ziuziakowska requested review from AlexJones0, engdoreis and marnovandermaas February 24, 2026 17:09

ziuziakowska marked this pull request as ready for review February 24, 2026 17:21

marnovandermaas marked this pull request as draft February 25, 2026 11:03

ziuziakowska force-pushed the hal-rework-autogen branch 2 times, most recently from 458332f to 7808bc7 Compare February 25, 2026 15:36

ziuziakowska marked this pull request as ready for review February 25, 2026 16:16

ziuziakowska changed the title ~~[WIP] Rework HAL to use the auto-generated register interface~~ Rework HAL to use the auto-generated register interface Feb 25, 2026

engdoreis reviewed Feb 26, 2026

View reviewed changes

ziuziakowska force-pushed the hal-rework-autogen branch 2 times, most recently from 58fd48f to d425089 Compare February 26, 2026 15:30

marnovandermaas reviewed Feb 27, 2026

View reviewed changes

ziuziakowska force-pushed the hal-rework-autogen branch 8 times, most recently from 3b1c4b6 to afd239c Compare March 10, 2026 18:29

ziuziakowska requested review from engdoreis and raylau1 March 10, 2026 18:30

marnovandermaas requested changes Mar 12, 2026

View reviewed changes

AlexJones0 reviewed Mar 12, 2026

View reviewed changes

ziuziakowska force-pushed the hal-rework-autogen branch 3 times, most recently from a23b128 to b5ba782 Compare March 17, 2026 11:02

ziuziakowska mentioned this pull request Mar 17, 2026

Initial library code/asm wrappers for Hart interrupts #348

Closed

marnovandermaas marked this pull request as draft March 22, 2026 11:29

ziuziakowska added 8 commits March 23, 2026 09:44

sw, mmio.h: add type-safe volatile read/write macros

1693cce

Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

util: clang_tidy.py: lint headers as well as source files

3e14239

Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

sw: update UART HAL to use new autogen interface

481aae2

also change the naming convention to match PLIC Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

sw: update timer HAL to use new autogen interface

9a09674

Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

sw: update PLIC HAL to use new autogen interface

3350c0e

Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

sw: move dv_test_status to dv_test_status.h in HAL

9526100

the DV test status "device" is currently missing from RDL autogeneration, so this creates the minimal structure manually Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

sw: update SPI Device HAL to use new autogen interface

bcecd99

Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

util: fpga_runner: rework to use asyncio

bb1e6fd

Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

ziuziakowska force-pushed the hal-rework-autogen branch from b5ba782 to bb1e6fd Compare March 23, 2026 09:52

ziuziakowska mentioned this pull request Mar 23, 2026

util: FPGA runner: rework to use asyncio #366

Open

ziuziakowska closed this Mar 24, 2026

		uprintf(console, "\nJumping to: 0x%0x\n", BootAddress);
		uprintf(console, "\nJumping to: 0x%x\n", BootAddress);

Conversation

ziuziakowska commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

engdoreis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexJones0 Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marnovandermaas left a comment

Choose a reason for hiding this comment

Uh oh!

marnovandermaas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexJones0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ziuziakowska commented Feb 17, 2026 •

edited

Loading

AlexJones0 Mar 13, 2026 •

edited

Loading