[chip, gpio, dv] GPIO smoke test by KinzaQamar · Pull Request #382 · lowRISC/mocha

KinzaQamar · 2026-03-26T10:34:41Z

WIP

KinzaQamar · 2026-03-26T10:44:41Z

I forgot that the core files are autogenerated :(( Failing lint

martin-velay

I have some initial comments. It's a very good start though!

hw/top_chip/dv/env/seq_lib/top_chip_dv_gpio_base_vseq.sv

hw/top_chip/dv/env/top_chip_dv_env.sv

hw/top_chip/dv/env/top_chip_dv_env_cfg.sv

hw/top_chip/dv/tb/tb.sv

hw/vendor/lowrisc_ip/ip_templates/gpio/dv/env/gpio_env.core.tpl

sw/device/lib/hal/gpio.c

hw/top_chip/dv/env/seq_lib/top_chip_dv_gpio_base_vseq.sv

martin-velay · 2026-03-31T09:25:25Z

hw/top_chip/dv/env/seq_lib/top_chip_dv_gpio_base_vseq.sv

+endtask : drive_pattern
+
+task top_chip_dv_gpio_base_vseq::wait_for_pattern(logic [NUM_GPIOS-1:0] exp_val);
+  `DV_SPINWAIT(wait(cfg.gpio_vif.pins ==? exp_val);,


Does the expected value can be x or z?

No, because the start of gpio_test() enable the pulldown's. Are you asking this because of case equality?

Yes it's related to this wildcard ==? equality. Is there a reason to have this sign?

For this kind of comparison

for (int i = 0; i < NUM_GPIOS/4; i++) begin wait_for_pattern({8'h?, 1 << i, 16'h????}); end

I want to compare the third quater of pins driven by software and ignore the rest.

But as you know what the rest of the pins should look like, it doesn't harm to make the full check I guess.

martin-velay · 2026-03-31T09:32:19Z

hw/top_chip/dv/env/seq_lib/top_chip_dv_gpio_base_vseq.sv

+  //
+  // Drive 1's in temperature pattern on the lower 8 pins.
+  for (int i = NUM_GPIOS/4; i < (NUM_GPIOS/4) * 2; i++) begin
+    drive_pattern(1, i, 1);


Is it enough to wait 1 clock for the SW to read the register?

So, this is driving a pattern after a clock. From the SW side, the test expect the pattern that's been driven at the end.
In smoketest.c, it is structured as something like that:

for (int i = 0; i < GPIO_NUM_PINS/4; i++) { drive(gpio, GPIO_REG_MASKED_OUT_LOWER, 1 << i); if (i == ((GPIO_NUM_PINS/4)-1)) { wait(gpio, 0xFF80); } }

So, first the C drives walking 1's on the first quater of pins. Meanwhile, vseq check if those quater pads have that pattern. Once that is happened, the vseq drives 1's in temperature sequence manner on the next quater after every cycle. So, the C is only able to see that pattern once all the 1's are written on the second quater. Does that make sense?

Which means that we make some intermediate actions which are not checked? If yes, I guess it's OK as we do checks more in depth at block level. So if we don't check the intermediate values, I'd suggest to either check it, or drop the intermediate driving. WDYT?

If I am dropping the driving part, it means we are not driving anything on the inputs. Okay, I'll tweak it so the SW will wait for every 1's.

Does that look fine now?

hw/top_chip/dv/env/seq_lib/top_chip_dv_gpio_base_vseq.sv

hw/top_chip/dv/env/top_chip_dv_env.sv

hw/top_chip/dv/tb/tb.sv

hw/top_chip/dv/env/seq_lib/top_chip_dv_gpio_base_vseq.sv

hw/top_chip/dv/tb/tb.sv

hw/top_chip/dv/env/seq_lib/top_chip_dv_gpio_base_vseq.sv

hw/vendor/lowrisc_ip/ip_templates/gpio/dv/env/gpio_env.core.tpl

hw/top_chip/dv/env/seq_lib/top_chip_dv_gpio_base_vseq.sv

KinzaQamar · 2026-04-01T08:43:14Z

HI @martin,

Thanks for the detailed review. I realized yesterday that the comments I added in the test and vseq were way more difficult and I spent some time making it a bit easier to understanding. Do you mind take another look? Some of your comments are unresolved because either I asked for a go-ahead or a question in replies.

hw/top_chip/dv/env/seq_lib/top_chip_dv_gpio_base_vseq.sv

martin-velay · 2026-04-01T08:57:18Z

hw/top_chip/dv/env/seq_lib/top_chip_dv_gpio_base_vseq.sv

+endtask : drive_pattern
+
+task top_chip_dv_gpio_base_vseq::wait_for_pattern(logic [NUM_GPIOS-1:0] exp_val);
+  `DV_SPINWAIT(wait(cfg.gpio_vif.pins ==? exp_val);,


Yes it's related to this wildcard ==? equality. Is there a reason to have this sign?

martin-velay · 2026-04-01T09:15:30Z

hw/top_chip/dv/env/seq_lib/top_chip_dv_gpio_base_vseq.sv

+  //
+  // Drive 1's in temperature pattern on the lower 8 pins.
+  for (int i = NUM_GPIOS/4; i < (NUM_GPIOS/4) * 2; i++) begin
+    drive_pattern(1, i, 1);


Which means that we make some intermediate actions which are not checked? If yes, I guess it's OK as we do checks more in depth at block level. So if we don't check the intermediate values, I'd suggest to either check it, or drop the intermediate driving. WDYT?

hw/top_chip/dv/tb/tb.sv

hw/vendor/lowrisc_ip/ip_templates/gpio/dv/env/gpio_env.core.tpl

It is going to be useful in order to import GPIO related parameters in tb.sv Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>

Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>

The smoketest.c and top_chip_dv_gpio_base_vseq works in pairs in a way that C drives W1's and W0's pattern on the outputs and vseq drives T1's and T0's pattern on the inputs. Both C and vseq waits for an expected pattern to arrive in order to drive the next pattern. Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>

martin-velay

I feel that the choice of driving patterns on the quarters from the SW and DV at the same time and by crossing the quarters is hard to follow and I am not sure it brings something for a top-level test. I'd suggest to simplify but still driving and checking from both the SW and the DV sides. If you have a doubt we can have a call.

But we are almost there! 👌

martin-velay · 2026-04-02T06:09:09Z

sw/device/tests/gpio/smoketest.c

+
+// Verifies GPIOs in partially output and input direction. The test distributes GPIOs as four equal
+// quaters. The idea is to drive first quater of GPIOs as outputs and wait for a pattern to appear
+// on the second quater of pins as inputs. Next, drive a pattern on the third quater and waits for a


This nit change should be applied in several places in this PR (not only in this file)

Suggested change

// on the second quater of pins as inputs. Next, drive a pattern on the third quater and waits for a

// on the second quarter of pins as inputs. Next, drive a pattern on the third quarter and waits for a

martin-velay · 2026-04-02T06:19:24Z

hw/top_chip/dv/env/seq_lib/top_chip_dv_gpio_base_vseq.sv

+
+  `uvm_info(`gfn, "Starting GPIOs outputs test", UVM_LOW)
+
+  // The SW first sets the first and third quater of GPIOs to 0's in order to walk 1's on them.


Here we wait for all 0s and not only on the 1st/3rd quarter, your comment is a bit confusing

I think what I meant to say is that the SW writes 0's on the first and third quater but because we haven't drove anything in the pads inputs they must be zero. So we wait for all zeros

Maybe the best answer is to remove the setting to 0's thing from both SW and vseq. I was thinking it is good to drive 0's and wait for 0's then because the function can be used somewhere else in a different context and to do W1's cleanly, we should initialize it with 0's.

martin-velay · 2026-04-02T07:19:54Z

hw/top_chip/dv/env/seq_lib/top_chip_dv_gpio_base_vseq.sv

+
+  // Wait for some cycles so that the pads can transition from the current pin state to the next pin
+  // state
+  cfg.sys_clk_vif.wait_clks(wait_num_clks);


You drive the pins in a for loop and mention it to be a temperature fashion pattern but the wait_clks is outside of the for loop, so the temperature pattern won't be visible at all.

What I meant was that I am driving the quarter pin by pin. But you are right, it is not going to be shown as temperature pattern.

KinzaQamar requested a review from martin-velay March 26, 2026 10:37

KinzaQamar force-pushed the gpio_smoke branch 3 times, most recently from 7d06a9f to 6ddb362 Compare March 26, 2026 13:52

martin-velay reviewed Mar 26, 2026

View reviewed changes

KinzaQamar force-pushed the gpio_smoke branch 8 times, most recently from d83b568 to 32c5f09 Compare March 30, 2026 15:01

KinzaQamar marked this pull request as ready for review March 30, 2026 15:02

KinzaQamar requested review from elliotb-lowrisc and marnovandermaas March 30, 2026 16:16

KinzaQamar force-pushed the gpio_smoke branch from 32c5f09 to 0b3c095 Compare March 30, 2026 16:30

martin-velay reviewed Mar 31, 2026

View reviewed changes

KinzaQamar commented Mar 31, 2026

View reviewed changes

KinzaQamar force-pushed the gpio_smoke branch 4 times, most recently from 8a5da04 to 93f0c4e Compare March 31, 2026 23:23

martin-velay reviewed Apr 1, 2026

View reviewed changes

KinzaQamar force-pushed the gpio_smoke branch 4 times, most recently from 4534ce8 to 5ade54b Compare April 1, 2026 11:44

KinzaQamar force-pushed the gpio_smoke branch 4 times, most recently from 525ccea to e22f462 Compare April 1, 2026 12:40

KinzaQamar added 6 commits April 1, 2026 23:45

[Dependency] Added GPIO related dependency

0cd8f62

It is going to be useful in order to import GPIO related parameters in tb.sv Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>

[chip, gpio, dv] Connected GPIO ports into the tb

de48b69

Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>

[chip, gpio, dv] Feeded the gpio_vif inside the chip_dv_env

36da794

Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>

[chip, gpio, dv] Added gpio_base_vseq

c879624

Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>

[gpio, sw] Added functions to write all output and output_en pins

bf63dfa

Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>

KinzaQamar force-pushed the gpio_smoke branch from e22f462 to 998502d Compare April 1, 2026 22:46

martin-velay reviewed Apr 2, 2026

View reviewed changes

	// on the second quater of pins as inputs. Next, drive a pattern on the third quater and waits for a
	// on the second quarter of pins as inputs. Next, drive a pattern on the third quarter and waits for a


		`uvm_info(`gfn, "Starting GPIOs outputs test", UVM_LOW)

		// The SW first sets the first and third quater of GPIOs to 0's in order to walk 1's on them.

Conversation

KinzaQamar commented Mar 26, 2026

Uh oh!

KinzaQamar commented Mar 26, 2026

Uh oh!

martin-velay 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KinzaQamar Mar 31, 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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KinzaQamar commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

martin-velay 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!

KinzaQamar Mar 31, 2026 •

edited

Loading

KinzaQamar commented Apr 1, 2026 •

edited

Loading