[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:28:01Z

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.

I think it is better not to have this code churn right now. You can do this in a separate PR after this if you feel strongly about it.

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

+    {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

This is still not fixed. You reference the rom_ctrl and kmac patch directories before they are created.

There are no rom_ctrl and kmac patch files/directories. Shall I remove them from the lowrisc_ip.vendor.hjson file?

If there is no patch needed for kmac and rom_ctrl, you can remove the patch_dir option for them in lowrisc_ip.vendor.hjson

patch_dirs removed

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

raylau1

Thanks for adding this, I have added some comment.

marnovandermaas

Great work! Some more comments from my end before we can merge this.

marnovandermaas · 2026-04-13T10:33:38Z

Can you please change this back? It would be good not to have this code churn.

marnovandermaas · 2026-04-13T10:35:44Z

I think it is better not to have this code churn right now. You can do this in a separate PR after this if you feel strongly about it.

marnovandermaas · 2026-04-13T10:40:07Z

+    {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.


This is still not fixed. You reference the rom_ctrl and kmac patch directories before they are created.

marnovandermaas · 2026-04-13T10:41:34Z

These changes are not currently tested in CI, have you tested these changes?

marnovandermaas · 2026-04-13T10:52:42Z

+    .BootRomInitFile      ( RomInitFile ),
+    .AlertAsyncOn         ( 1'b1 ),
+    .AlertSkewCycles      ( 1 ),
+    .FlopToKmac           ( 1'b0 ),
+    .RndCnstScrNonce      ( '0 ),
+    .RndCnstScrKey        ( '0 ),
+    .SecDisableScrambling ( 1'b1 ),
+    .MemSizeRom           ( 32768 )


Nit: Align the closing parentheses for parameters.

marnovandermaas · 2026-04-13T10:53:39Z

+    .RndCnstScrNonce      ( '0 ),
+    .RndCnstScrKey        ( '0 ),
+    .SecDisableScrambling ( 1'b1 ),
+    .MemSizeRom           ( 32768 )


This should be calculated based on the length defined in top_pkg

fixed: top_pkg::RomCtrlMemLength used

marnovandermaas · 2026-04-13T10:54:36Z

+    .kmac_data_o    (kmac_app_req[0]),
+    .kmac_data_i    (kmac_app_rsp[0]),


I'd rather leave these unconnected until we integrate KMAC

marnovandermaas · 2026-04-13T10:58:28Z

 mocha_add_test(NAME timer_smoketest SOURCES timer/smoketest.c LIBRARIES ${LIBS} FPGA)
 mocha_add_test(NAME timer_interrupt_test SOURCES timer/interrupt.c LIBRARIES ${LIBS})
 mocha_add_test(NAME uart_smoketest SOURCES uart/smoketest.c LIBRARIES ${LIBS})
-


Don't remove this newline.

marnovandermaas · 2026-04-13T11:00:45Z

 timeout 5m $ROOT_DIR/build/lowrisc_mocha_top_chip_verilator_0/sim-verilator/Vtop_chip_verilator \
-  $@ > /dev/null 2>&1
+  --rominit $ROOT_DIR/sw/device/tests/rom_ctrl/mem_init_file.vmem \
+  -E $@ > /dev/null 2>&1


This looks wrong to me. You should make this change in tests.cmake I believe: 9affb1c

marnovandermaas

Some more comments on the software side.

marnovandermaas · 2026-04-13T11:10:45Z

+    //uint8_t address_length = 3;
+    //uint8_t data_length = 4;


Can we remove these comments?

Comented out code removed

marnovandermaas · 2026-04-13T11:11:24Z

+    const uint32_t addresses[3] = { 0x00000000, 0x00004000, 0x00007FF0 };
+
+    // ROM Init values
+    const uint32_t expected_data[3][4] = { { 0xDEADBEEF, 0xCAFEBABE, 0x00000001, 0xEEEEEEEE },


Since the [3] must be the same for both can we make a define for this value?

defines used instead of numbers

* rom_ctrl_regs if added to xbapr_peri cfg

* kmac vendored in * lowrisc_ip.vendor.hjson modified * Apply patch on prim_xilinx_rom.sv. Vivado synth failed due to string type parameter.

* 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 * backdoor access for ROM config added

* 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 complie with clang format * fixed issues highlighted during PR review

* xbar_peri IP regenerated with new paths during CI/CD tests * ROM backdoor init command added to sw regression tests

* FPGA tests need ROM initialization to pass * RomInitFile argument and init file paths added to the nix/fpga.nix script

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 force-pushed the kk_rom_ctrl branch 3 times, most recently from 71fd63b to e3eebff Compare April 1, 2026 09:23

ziuziakowska reviewed Apr 2, 2026

View reviewed changes

Comment thread sw/device/tests/rom_ctrl/smoketest.c Outdated

Comment thread sw/device/lib/hal/rom_ctrl.h Outdated

Comment thread sw/device/lib/hal/CMakeLists.txt Outdated

Comment thread sw/device/tests/rom_ctrl/smoketest.c Outdated

Comment thread sw/device/tests/rom_ctrl/smoketest.c Outdated

KolosKoblasz-Semify force-pushed the kk_rom_ctrl branch from e3eebff to 6f80dee Compare April 7, 2026 08:34

raylau1 reviewed Apr 7, 2026

View reviewed changes

KolosKoblasz-Semify force-pushed the kk_rom_ctrl branch 2 times, most recently from e72eb24 to 555b922 Compare April 7, 2026 13:42

This comment was marked as resolved.

Sign in to view

KolosKoblasz-Semify force-pushed the kk_rom_ctrl branch from 555b922 to 8075c5c Compare April 8, 2026 11:23

marnovandermaas mentioned this pull request Apr 8, 2026

Integrate ROM #135

Open

KolosKoblasz-Semify force-pushed the kk_rom_ctrl branch 2 times, most recently from 33172ca to 4ee0a2b Compare April 9, 2026 12:36

KolosKoblasz-Semify requested review from marnovandermaas, raylau1 and ziuziakowska April 9, 2026 13:36

KolosKoblasz-Semify force-pushed the kk_rom_ctrl branch from 4ee0a2b to 3090824 Compare April 9, 2026 13:37

martin-velay mentioned this pull request Apr 10, 2026

ROM DV - Top-level basic register access #148

Open

marnovandermaas reviewed Apr 13, 2026

View reviewed changes

KolosKoblasz-Semify force-pushed the kk_rom_ctrl branch from 3090824 to 6cf5ade Compare April 14, 2026 11:46

KolosKoblasz-Semify force-pushed the kk_rom_ctrl branch from 6cf5ade to d7fc6d6 Compare April 15, 2026 07:48

KolosKoblasz-Semify added 6 commits April 15, 2026 09:03

[xbar_peri, feat] xbar_peri interconnect regenerated

bf89f97

* rom_ctrl_regs if added to xbapr_peri cfg

[rom_ctrl, vendoring] rom_ctrl dependencies vendored in

4197ec8

* kmac vendored in * lowrisc_ip.vendor.hjson modified * Apply patch on prim_xilinx_rom.sv. Vivado synth failed due to string type parameter.

[rom_ctrl, script] rom_ctrl related script upgrades

8347de4

* xbar_peri IP regenerated with new paths during CI/CD tests * ROM backdoor init command added to sw regression tests

[rom_ctrl, fpga] Add ROM init in fpga bit file

9e41b05

* FPGA tests need ROM initialization to pass * RomInitFile argument and init file paths added to the nix/fpga.nix script

KolosKoblasz-Semify force-pushed the kk_rom_ctrl branch from d7fc6d6 to 9e41b05 Compare April 15, 2026 08:04

		{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.

		.kmac_data_o (kmac_app_req[0]),
		.kmac_data_i (kmac_app_rsp[0]),

Conversation

KolosKoblasz-Semify commented Mar 27, 2026

Uh oh!

marnovandermaas left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ziuziakowska left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

raylau1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

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!

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!