[hw] rom_ctrl added to mocha by KolosKoblasz-Semify · Pull Request #389 · lowRISC/mocha

KolosKoblasz-Semify · 2026-03-27T14:03:07Z

existing source files vendored in
rom_ctrl integrated at topl level, AXI and TL busnetwork extended
new smoketest written for reading rom init values
xbar_peri regenerated include new rom_ctrl_regs TL interface

marnovandermaas

Some initial comments from my end. Please also have a look at the CI failures.

marnovandermaas · 2026-03-30T12:27:47Z

hw/top_chip/ip/xbar_peri/xbar_peri.hjson

    },
+    { name:  "rom_ctrl_regs", // Rom_ctrl_regs
+      type:  "device",
+      clock: "clk_io_i",


This should be clk_main_i

marnovandermaas · 2026-03-30T12:27:56Z

hw/top_chip/ip/xbar_peri/xbar_peri.hjson

+    { name:  "rom_ctrl_regs", // Rom_ctrl_regs
+      type:  "device",
+      clock: "clk_io_i",
+      reset: "rst_io_ni",


This should be rst_main_ni

marnovandermaas · 2026-03-30T12:28:01Z

hw/top_chip/ip/xbar_peri/xbar_peri.hjson

Why did you move this file?

The .hjson file is relocated now because I found counter intuitive to store the "source" file for IP generation in the generated folder.

marnovandermaas · 2026-03-30T12:29:10Z

hw/vendor/lowrisc_ip.vendor.hjson

+    {from: "hw/ip/rom_ctrl",     to: "ip/rom_ctrl", patch_dir: "rom_ctrl"}, // ROM Control.
+    {from: "hw/ip/kmac",         to: "ip/kmac", patch_dir: "kmac"}, // kmac.


Adding the patch directories should be in the same commit as where you add them to the vendor.hjson file.

Fixed. IPs and vendoring script committed at the same time

marnovandermaas · 2026-03-30T12:29:44Z

hw/top_chip/dv/tb/tb.sv

-  top_chip_system #() dut (
+  top_chip_system # (
+    .SramInitFile(""),
+    .BootRomInitFile("sw/device/tests/rom_ctrl/mem_init_file.vmem")


Nit: RomInitFile

hw/top_chip/rtl/top_pkg.sv

marnovandermaas · 2026-03-30T12:42:38Z

hw/top_chip/rtl/top_chip_system.sv

+    // Inter-module signals
+    .rom_cfg_i      ('0),   // TO DO: What goes here?
+    .pwrmgr_data_o  (),     // TO DO: What goes here?
+    .keymgr_data_o  (),     // TO DO: What goes here?
+    .kmac_data_o    (),     // TO DO: What goes here?
+    .kmac_data_i    ('0),   // TO DO: What goes here?


Can you make any progress by looking at this file: https://github.com/lowRISC/opentitan/blob/697c06af0222c97734577eae52217d957874277c/hw/top_earlgrey/rtl/autogen/top_earlgrey.sv#L2786

Wired up unconnected ports with signals.

marnovandermaas · 2026-03-30T12:45:07Z

sw/device/lib/hal/mocha.c

+rom_t mocha_system_rom(void)
+{
+#if defined(__riscv_zcherihybrid)
+    return (uart_t)create_mmio_capability(rom_base, 0x48u);


The second argument should be the size of the ROM which is 32 KiB.

marnovandermaas · 2026-03-30T12:46:27Z

sw/device/lib/hal/rom_ctrl.c

+    uint32_t res;
+
+    res = DEV_READ(rom + 4*word_idx);
+
+    return res;


Nit: just return the DEV_READ.

marnovandermaas · 2026-03-30T12:47:52Z

sw/device/tests/rom_ctrl/mem_init_file.vmem

It might be good to put some data in the middle and at the end of ROM as well to test the edges out.

Date put at the beginning, middle and end sections of the ROM. rom_ctrl_smoketest revised to test all relevant address ranges.

* rom_ctrl_regs if added to xbapr_peri cfg

* kmac vendored in * lowrisc_ip.vendor.hjson modified

* axi bus network expanded to handle rom_ctrl data interface * xbar_peri module connected to rom_ctrl register interface * verilator rule exclusions added * Path to ROM init file added to tb.sv * clk and reset of the new axi bus and rom_ctrl fixed on top level

* new testcase implemented among top level sw tests * the new test reads the compile time initialized ROM values * if any comparision fails (expected values differs from read value) test is terminated with an error * sw test files reformated to compplie with clang format

* util/artifacts.py regenrates the xbar_peri files and it needs correct paths pointing at the .hjson cfg file and the output directory

ziuziakowska

Just some nits on the software.

Something to consider might also be modelling the ROM as a const char * instead of a void * typedef, as that could prevent writing through the pointer (although I believe that should cause a fault anyway).

ziuziakowska · 2026-04-02T09:53:20Z

sw/device/tests/rom_ctrl/smoketest.c

+    bool read_test_res;
+
+    uart_init(console);
+
+    read_test_res = rom_read_test(rom, console);
+
+    return read_test_res;
+}


Nit: Just return rom_read_test(...)

ziuziakowska · 2026-04-02T09:59:47Z

sw/device/lib/hal/rom_ctrl.h

+#define ROM_CTRL_FROM_BASE_ADDR(addr) ((rom_ctrl_t)(addr))
+#define ROM_FROM_BASE_ADDR(addr)      ((rom_t)(addr))


Nit: These macros are unused, create_mmio_capability and casting is preferred.

ziuziakowska · 2026-04-02T10:09:57Z

sw/device/lib/hal/CMakeLists.txt

 # SPDX-License-Identifier: Apache-2.0

-set(SRCS clkmgr.c gpio.c i2c.c mailbox.c mocha.c plic.c rstmgr.c spi_device.c timer.c uart.c)
+set(SRCS clkmgr.c gpio.c i2c.c mailbox.c mocha.c plic.c rstmgr.c spi_device.c timer.c uart.c rom_ctrl.c)


Nit: Should be between plic and rstmgr.

ziuziakowska · 2026-04-02T10:12:22Z

sw/device/tests/rom_ctrl/smoketest.c

+    uint8_t data_length = 4;
+
+    // ROM Address values
+    const uint32_t expected_addresses[3] = { 0x00000000, 0x00004000, 0x00007FF0 };


Nit: I would call this just addresses.

ziuziakowska · 2026-04-02T10:15:13Z

sw/device/tests/rom_ctrl/smoketest.c

+    for (uint8_t address_idx = 0; address_idx < address_length; address_idx++) {
+        for (uint8_t word_idx = 0; word_idx < data_length; word_idx++) {


In builtin.h we have an ARRAY_LEN macro to get the lengths of static arrays.

KolosKoblasz-Semify marked this pull request as ready for review March 27, 2026 14:55

KolosKoblasz-Semify force-pushed the kk_rom_ctrl branch from 2affd6c to 499b597 Compare March 30, 2026 08:07

KolosKoblasz-Semify changed the title ~~rom_ctrl added to mocha~~ [hw] rom_ctrl added to mocha Mar 30, 2026

marnovandermaas requested review from marnovandermaas and raylau1 March 30, 2026 12:25

marnovandermaas reviewed Mar 30, 2026

View reviewed changes

marnovandermaas mentioned this pull request Mar 30, 2026

rom_ctrl integration #351

Closed

KolosKoblasz-Semify added 2 commits April 1, 2026 08:36

[xbar_peri, feat] xbar_peri interconnect regenerated

beab1d5

* rom_ctrl_regs if added to xbapr_peri cfg

[rom_ctrl, vendoring] rom_ctrl dependencies vendored in

071058b

* kmac vendored in * lowrisc_ip.vendor.hjson modified

KolosKoblasz-Semify force-pushed the kk_rom_ctrl branch from 499b597 to 212d815 Compare April 1, 2026 08:28

KolosKoblasz-Semify force-pushed the kk_rom_ctrl branch from 212d815 to 71fd63b Compare April 1, 2026 08:57

KolosKoblasz-Semify added 2 commits April 1, 2026 10:22

[rom_ctrl, script] Path fixes for xbar_peri regen

e3eebff

* util/artifacts.py regenrates the xbar_peri files and it needs correct paths pointing at the .hjson cfg file and the output directory

KolosKoblasz-Semify force-pushed the kk_rom_ctrl branch from 71fd63b to e3eebff Compare April 1, 2026 09:23

ziuziakowska reviewed Apr 2, 2026

View reviewed changes

		{from: "hw/ip/rom_ctrl", to: "ip/rom_ctrl", patch_dir: "rom_ctrl"}, // ROM Control.
		{from: "hw/ip/kmac", to: "ip/kmac", patch_dir: "kmac"}, // kmac.

		#define ROM_CTRL_FROM_BASE_ADDR(addr) ((rom_ctrl_t)(addr))
		#define ROM_FROM_BASE_ADDR(addr) ((rom_t)(addr))

		for (uint8_t address_idx = 0; address_idx < address_length; address_idx++) {
		for (uint8_t word_idx = 0; word_idx < data_length; word_idx++) {

Conversation

KolosKoblasz-Semify commented Mar 27, 2026

Uh oh!

marnovandermaas left a comment

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!

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!

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!

ziuziakowska left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants