[top,sw/dv] Clk pwr rst smoke tests by csabakiss-semify · Pull Request #460 · lowRISC/mocha

csabakiss-semify · 2026-04-22T12:52:24Z

Added power manager related hal files
Extended reset manager hal
Extended clock manager hal
Added smoke tests for the managers

I can see the conflicts, even if I just finished the rebase. However, I am opening this PR to start the review process. I will fix the further conflicts just before the merge.

marnovandermaas

Some initial comments from my end.

marnovandermaas · 2026-04-23T15:04:22Z


-    if (rstmgr_software_reset_info_get(rstmgr)) {
-        return true;
+    if (reason & RSTMGR_RESET_INFO_POR) {


Nice, this is a good change!

marnovandermaas · 2026-04-23T15:05:24Z

    DEV_WRITE(rstmgr + RSTMGR_RESET_REQ_REG, RSTMGR_RESET_REQ_TRUE);
 }

 bool rstmgr_software_reset_info_get(rstmgr_t rstmgr)


I think we can now remove this function. I think it was only used in the software_reset test.

Good point. The test now reads and clears the reset reason directly, so this function is not needed anymore. I will remove it from the HAL source and header.

marnovandermaas · 2026-04-23T15:07:26Z

+static uint32_t clkmgr_read(clkmgr_t clkmgr, uintptr_t reg)
+{
+    return DEV_READ(clkmgr + reg);
+}
+
+static void clkmgr_write(clkmgr_t clkmgr, uintptr_t reg, uint32_t value)
+{
+    DEV_WRITE(clkmgr + reg, value);
+}
+
+static uint32_t clkmgr_bit(size_t clock)
+{
+    return 1u << clock;
+}
+


These don't look like clock manager specific functions. Do they already exist elsewhere in the repo?

I did not find another helper for this, only the DEV_READ and DEV_WRITE macros. I added these local helpers to avoid repeating the same code in this file. I can also remove them and use DEV_READ/DEV_WRITE directly if you prefer.

marnovandermaas · 2026-04-23T15:10:19Z

+    bool toggled = !initial;
+
+    clkmgr_gateable_clock_set_enabled(clkmgr, CLKMGR_GATEABLE_CLOCK_IO_PERI, toggled);
+    if (clkmgr_gateable_clock_get_enabled(clkmgr, CLKMGR_GATEABLE_CLOCK_IO_PERI) != toggled) {


Is there any way that we can confirm that the peripherals are actually stopped? I'm not sure whether reading a register will cause a bus error or just hang the design.

I don't think this software smoke test can safely check that. If we read from a peripheral after stopping its clock, it may hang the design. This test only checks that the clkmgr registers can be written and read back correctly. I think checking that the clock really stops should be done in DV, where we can observe the clock signals directly.

marnovandermaas · 2026-04-23T15:14:04Z

+pwrmgr_t mocha_system_pwrmgr(void)
+{
+#if defined(__riscv_zcherihybrid)
+    return (pwrmgr_t)create_mmio_capability(pwrmgr_base, 0x80u);


I think this length can be 0x44

Agreed. The highest register used here is WAKE_INFO at 0x3c, so 0x44 is enough. I will change it from 0x80 to 0x44.

marnovandermaas · 2026-04-23T15:14:25Z

+static uint32_t pwrmgr_read(pwrmgr_t pwrmgr, uintptr_t reg)
+{
+    return DEV_READ(pwrmgr + reg);
+}
+
+static void pwrmgr_write(pwrmgr_t pwrmgr, uintptr_t reg, uint32_t value)
+{
+    DEV_WRITE(pwrmgr + reg, value);
+}
+
+uint32_t pwrmgr_control_get(pwrmgr_t pwrmgr)
+{
+    return pwrmgr_read(pwrmgr, PWRMGR_CONTROL_REG);
+}


Replicated from clock manager.

Yes, I copied the same style from clkmgr. I can keep both files like this, or I can remove these helpers and use DEV_READ/DEV_WRITE directly in both files.

ziuziakowska

Software side looks good. Just a small nit

ziuziakowska · 2026-05-01T14:50:44Z

The convention for tests seems to be smoketest instead of smoke_test.

(likewise for the clkmgr test).

martin-velay · 2026-05-06T08:39:45Z

  top_chip_system #(
    .SramInitFile(""),
-    .RomInitFile ("")
+    .RomInitFile("")


Could you revert these kind of changes from this file and from hw/top_chip/dv/verilator/top_chip_verilator.sv? It was fine as before IMO (even if Verible linter makes these changes itself)

martin-velay · 2026-05-06T08:44:27Z

-# Can't run this test on FPGA or verilator tops and this test thinks that the GPIO inputs can be
-# driven externally
-mocha_add_test(NAME gpio_smoketest SOURCES gpio/smoketest.c LIBRARIES ${LIBS} SKIP_VERILATOR SKIP_FPGA)
 mocha_add_test(NAME i2c_smoketest SOURCES i2c/smoketest.c LIBRARIES ${LIBS})


Why this test has been removed?

# Can't run this test on FPGA or verilator tops and this test thinks that the GPIO inputs can be # driven externally mocha_add_test(NAME gpio_smoketest SOURCES gpio/smoketest.c LIBRARIES ${LIBS} SKIP_VERILATOR SKIP_FPGA)

martin-velay · 2026-05-06T08:46:21Z

-mocha_add_test(NAME rstmgr_software_reset SOURCES rstmgr/software_reset.c LIBRARIES ${LIBS})
+mocha_add_test(NAME pwrmgr_smoketest SOURCES pwrmgr/smoketest.c LIBRARIES ${LIBS})
+# Cannot currently run this on FPGA because the boot ROM expects the test to be provided on the SPI again.
+mocha_add_test(NAME rstmgr_smoketest SOURCES rstmgr/smoketest.c LIBRARIES ${LIBS})


I think if you don't want this test to run on FPGA you need to add this:

Suggested change

mocha_add_test(NAME rstmgr_smoketest SOURCES rstmgr/smoketest.c LIBRARIES ${LIBS})

mocha_add_test(NAME rstmgr_smoketest SOURCES rstmgr/smoketest.c LIBRARIES ${LIBS} SKIP_FPGA)

The reset test has been enabled for the FPGA in main, bootrom now supports this, please this back.

martin-velay · 2026-05-06T08:56:53Z

+    {
+      name: rstmgr_smoke
+      uvm_test_seq: top_chip_dv_base_vseq
+      sw_images: ["rstmgr_smoketest_vanilla_bare:5"]


I am unsure, I'll let @engdoreis commenting, but I think all these tests should come from the SRAM.

Well noted, the bare suffix does not exist anymore, the sram suffix should be used for now, but it will droped when we are able run tests from DRAM.

These new entries need to be adapted to follow the pattern of the other tests, the rom should included in the sw_images, and the loaded in run_opts

Signed-off-by: Csaba Kiss <csaba.kiss@semify-eda.com>

…ests to top level smoke regression and top level sim config Signed-off-by: Csaba Kiss <csaba.kiss@semify-eda.com>

csabakiss-semify force-pushed the clk_pwr_rst_smoke_test branch 2 times, most recently from 2dbcd22 to 7adf270 Compare April 23, 2026 09:35

martin-velay changed the title ~~Clk pwr rst smoke tests~~ [top,sw/dv] Clk pwr rst smoke tests Apr 23, 2026

martin-velay requested review from engdoreis, marnovandermaas and martin-velay April 23, 2026 13:04

marnovandermaas reviewed Apr 23, 2026

View reviewed changes

marnovandermaas requested a review from ziuziakowska April 23, 2026 15:20

ziuziakowska reviewed May 1, 2026

View reviewed changes

csabakiss-semify force-pushed the clk_pwr_rst_smoke_test branch 5 times, most recently from 5f1c44d to 9b76a67 Compare May 6, 2026 07:54

martin-velay reviewed May 6, 2026

View reviewed changes

csabakiss-semify added 4 commits May 6, 2026 10:09

[sw] Added reset manager smoke test

650059a

Signed-off-by: Csaba Kiss <csaba.kiss@semify-eda.com>

[sw] Added clock manager smoke test

2547886

Signed-off-by: Csaba Kiss <csaba.kiss@semify-eda.com>

[sw] Added power manager smoke test

1e6e566

Signed-off-by: Csaba Kiss <csaba.kiss@semify-eda.com>

[script] Added reset manager, clock manager and power manager smoke t…

ed156ec

…ests to top level smoke regression and top level sim config Signed-off-by: Csaba Kiss <csaba.kiss@semify-eda.com>

csabakiss-semify force-pushed the clk_pwr_rst_smoke_test branch from 9b76a67 to ed156ec Compare May 6, 2026 09:17

	mocha_add_test(NAME rstmgr_smoketest SOURCES rstmgr/smoketest.c LIBRARIES ${LIBS})
	mocha_add_test(NAME rstmgr_smoketest SOURCES rstmgr/smoketest.c LIBRARIES ${LIBS} SKIP_FPGA)

Conversation

csabakiss-semify commented Apr 22, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

engdoreis May 6, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

engdoreis May 6, 2026 •

edited

Loading