GPIO straps verif #25868

marcelocarvalhoLowRisc · 2025-01-14T15:59:10Z

New sequence to verify the straps feature and some additional description of the behaviour of this feature in the register document.

martin-velay

I think this PR still requires some changes.

hw/ip/gpio/dv/tb/tb.sv

hw/ip/gpio/data/gpio_testplan.hjson

hw/ip/gpio/dv/tb/tb.sv

hw/ip/gpio/dv/env/gpio_env_cfg.sv

hw/ip/gpio/dv/env/gpio_scoreboard.sv

hw/ip/gpio/dv/env/seq_lib/gpio_rand_straps_vseq.sv

hw/ip/gpio/doc/registers.md

hw/ip/gpio/data/gpio.hjson

hw/ip/gpio/data/gpio_testplan.hjson

hw/ip/gpio/dv/env/gpio_scoreboard.sv

hw/ip/gpio/dv/interfaces/gpio_straps_if.sv

hw/ip/gpio/dv/tb/tb.sv

rswarbrick · 2025-01-23T18:26:55Z

hw/ip/gpio/dv/env/gpio_env_cfg.sv

+    // Used to allow reset operation during a stress all tests and check the CSR after that.
+    can_reset_with_csr_accesses = 1'b1;


To avoid everything getting globbed together, can you pull this change into a separate commit (which can come first)?

That commit could be cherry-picked into a separate PR, that could be reviewed and merged without having to pin down the straps behaviour.

This is merged from the first PR related with stress_all random reset. So I think we don't need to create a new PR only for this line, because it's include in the PR #25657. Or do you mean something else?

I think this suggestion still stands.

I will drop the changes in the line 37, that is part of the PR #25657.
The other things are related with the strap verification updates.

martin-velay · 2025-01-27T08:41:50Z

hw/ip/gpio/dv/env/gpio_scoreboard.sv

+    end
+  endtask : monitor_gpio_straps
+
+  virtual protected task handle_reset();


I am not sure we need this task as this extended from the cip_scoreboard which is itself inherited from dv_base_scoreboard and where do the same: dv_base_scoreboard line#32. What do you think?

Very true, I will remove this and use the dv_base_scoreboard version. Thanks =)

martin-velay · 2025-01-27T08:46:21Z

hw/ip/gpio/dv/interfaces/gpio_straps_if.sv

+    logic          strap_en;       // Signal to enable straps
+    gpio_straps_t  sampled_straps; // Sampled gpio_i snapshot data from GPIO (DUT)
+
+      modport straps_port(input strap_en, output sampled_straps);


Nit: alignment.

And for my knowledge, can you tell me why this modport is required?

I think the idea was that such a port would allow one "actor" to see things in the correct direction.

But (for @marcelocarvalhoLowRisc):

Doesn't the DV code need to sample through the port? I'm thinking about tasks like monitor_gpio_straps.

But don't we need a port for things driving the strap_en state as well? (gpio_rand_straps_vseq)

And the ports should probably be named in a way that relates to who uses them. At the moment, we have a straps port on a straps interface, which is slightly doubled!

Finally: The indentation needs sorting out here.

Very true, I will do this. Makes more sense to create 2 modports, one for each case.

martin-velay · 2025-01-27T08:56:19Z

hw/ip/gpio/dv/tests/gpio_base_test.sv

+  function new(string name, uvm_component parent);
+    super.new(name, parent);
+  endfunction
+  task reset_phase(uvm_phase phase);


Nit: can you insert one line break between each task/function ? And same before virtual function void build_phase(uvm_phase phase);

Yes, indeed is visually not good =D

rswarbrick · 2025-01-31T11:55:26Z

hw/ip/gpio/dv/env/gpio_env_cfg.sv

+    // Used to allow reset operation during a stress all tests and check the CSR after that.
+    can_reset_with_csr_accesses = 1'b1;


I think this suggestion still stands.

rswarbrick · 2025-01-31T11:56:03Z

hw/ip/gpio/data/gpio_testplan.hjson

+    {
+      name: straps_data
+      desc: '''Verify the straps data/valid ouput expected values based on the strap_en and gpio_i inputs:
+      - Drives gpio_i input with random values.


Grammar nit: I think this should probably be "Drive" to be the same as the other lines.

rswarbrick · 2025-01-31T11:58:27Z

hw/ip/gpio/dv/env/gpio_scoreboard.sv

+    `DV_CHECK_FATAL(
+    ral.hw_straps_data_in.predict(.value(gpio_i_driven),


Style nit: The indentation looks a bit odd here. I'd suggest this:

virtual task update_gpio_straps_regs(); // Update data_in ral register value based on result of input `DV_CHECK_FATAL(ral.hw_straps_data_in.predict(.value(gpio_i_driven), .kind(UVM_PREDICT_DIRECT))); // Update data_in valid register value based on result of input `DV_CHECK_FATAL(ral.hw_straps_data_in_valid.predict(.value('b1), .kind(UVM_PREDICT_DIRECT))); endtask : update_gpio_straps_regs

rswarbrick · 2025-01-31T11:59:46Z

hw/ip/gpio/dv/env/gpio_scoreboard.sv

+    // Update data_in ral register value based on result of input
+    `DV_CHECK_FATAL(
+    ral.hw_straps_data_in.predict(.value(gpio_i_driven),
+                                  .kind(UVM_PREDICT_DIRECT)));


Isn't this going to die if it overlaps with a read of one of the two registers? I think this probably needs to be UVM_PREDICT_READ?

Yes, you are correct, this should be UVM_PREDICT_READ, from the documentation:

Predict based on the specified value having been read

rswarbrick · 2025-01-31T12:05:04Z

hw/ip/gpio/dv/env/gpio_scoreboard.sv

+      @(posedge cfg.clk_rst_vif.clk)
+      if(!cfg.under_reset) begin
+        if (|gpio_i_driven === 1'b1) begin
+          @(posedge cfg.straps_vif_inst.port_out.strap_en) begin


Hmm, I don't think this is going to work very nicely: it will block the "every clock" logic that is there otherwise. I think you probably need to rephrase it slightly, updating local variables on every clock edge with the current value of strap_en and then checking each time for a posedge and looking two cycles back (if (cfg.straps_vif_inst.port_out.strap_en && !strap_en_qq) ...).

I implemented the strap_en_qq flop logic inside of the interface, and fixed the statement.

rswarbrick · 2025-01-31T12:08:49Z

hw/ip/gpio/dv/env/seq_lib/gpio_rand_straps_vseq.sv

+  constraint num_trans_c {
+    num_trans inside {[20:200]};
+  }


I think this note still applies?

rswarbrick · 2025-01-31T12:09:49Z

hw/ip/gpio/dv/env/seq_lib/gpio_rand_straps_vseq.sv

+
+    // Wait at least one clock cycle to drive the strap_en
+    // Required because is required one clock cycle to update the gpio_in regsisters.
+    `DV_CHECK_MEMBER_RANDOMIZE_WITH_FATAL(delay, delay >= 1;)


This is reusing delay (a base class variable that's supposed to be gaps between transactions) for something else. I'd just randomise a local variable.

true, fixed

rswarbrick · 2025-01-31T12:13:09Z

hw/ip/gpio/dv/env/seq_lib/gpio_rand_straps_vseq.sv

+//    - Apply reset and make sure the strap registers are clean.
+//    - Read straps registers after reset.
+//    - Iterate again the same flow, with new random values.
+class gpio_rand_straps_vseq extends gpio_base_vseq;


Thinking about this a bit, can't we drop this vseq entirely? Maybe the right approach is:

Update the base vseq so that the strap_en signal gets driven at unexpected times.

Teach the scoreboard when to sample the pins and store the results in the straps registers, updating the RAL.

Then I think tests like the CSR RW test should get all the behaviour you need?

I think having a test case is the right approach. I believe that having a test case for this feature makes it easier to debug in the future if something changes in the RTL and the strap suddenly stops working. Additionally, having a test case for the feature makes it clearer for VPlan and coverage tracking. If the complexity of the feature increases in the future, it will be easier to implement incremental updates, rather than relying on a base class to 'insert' new code. Adding new elements to the base VSeq class could lead to unexpected issues, especially as the complexity of the testbench grows. Eventually, it could become difficult to track what is being driven and what isn’t.

rswarbrick · 2025-01-31T12:14:45Z

hw/ip/gpio/dv/interfaces/gpio_straps_if.sv

+  modport port_in(input strap_en, output sampled_straps);
+  modport port_out(output strap_en, input sampled_straps);


I'm not sure about the naming here: "in" and "out" both have one input and one output! :-)

Maybe name the ports after who is expected to do this sort of access?

yes, that is true, I was not very creative with that names, I changed it, let me know what do you think now.

rswarbrick · 2025-01-31T12:15:49Z

hw/ip/gpio/rtl/gpio.sv

@@ -85,7 +85,6 @@ module gpio
    assign hw2reg.hw_straps_data_in.d        = data_in_d;
    assign sampled_straps_o.data             = reg2hw.hw_straps_data_in.q;
    assign sampled_straps_o.valid            = reg2hw.hw_straps_data_in_valid.q;
-    `ASSUME(StrapSampleOnce_A, ##1 $fell(sample_trigger) |-> always !sample_trigger)


I'd probably pull this 1-line change into a separate commit and push it as a PR in parallel. It's obviously correct.

ok, I will do that.

Merge the PR lowRISC#25868 first. Signed-off-by: Marcelo Carvalho Faleiro de Almeida <[email protected]>

marcelocarvalhoLowRisc

ready to be reviewed again

marcelocarvalhoLowRisc · 2025-02-06T12:13:05Z

hw/ip/gpio/dv/tests/gpio_base_test.sv

+  function new(string name, uvm_component parent);
+    super.new(name, parent);
+  endfunction
+  task reset_phase(uvm_phase phase);


martin-velay · 2025-02-07T12:45:48Z

hw/ip/gpio/dv/interfaces/gpio_straps_if.sv

+  modport tb_if(output strap_en, input sampled_straps);
+
+
+    always_ff @(posedge clk or negedge rst_n) begin


Nit: I think there is still an indentation issue for this whole block.
And by convention, I don't know if usually we do "qq" or "q2" here at lowRISC, @rswarbrick ?

martin-velay

From what I can see most of the comments have been covered and this PR looks better now. If the functionality part is OK, I am keen on having it merged.

marcelocarvalhoLowRisc · 2025-02-07T16:10:31Z

hw/dv/sv/csr_utils/csr_utils_pkg.sv

I created this csr_mirror, based on csr_read but with only the necessary parameters. That is more useful instead of doing this all uvm_reg decode and checks and I kept the increment_outstanding.... task as well, that I think is required if with run this with rnd_reset stress all for example...

- Fix the gpio_stress_all with random reset PR:25657 was taking into this commit. Signed-off-by: Marcelo Carvalho Faleiro de Almeida <[email protected]>

marcelocarvalhoLowRisc requested a review from a team as a code owner January 14, 2025 15:59

marcelocarvalhoLowRisc requested review from rswarbrick and martin-velay and removed request for a team January 14, 2025 15:59

marcelocarvalhoLowRisc force-pushed the gpio_straps_verif branch 3 times, most recently from 74b9fcd to 7921fc9 Compare January 14, 2025 16:20

martin-velay suggested changes Jan 15, 2025

View reviewed changes

marcelocarvalhoLowRisc force-pushed the gpio_straps_verif branch 4 times, most recently from 05e6443 to 2e49144 Compare January 15, 2025 14:21

rswarbrick reviewed Jan 16, 2025

View reviewed changes

marcelocarvalhoLowRisc force-pushed the gpio_straps_verif branch 17 times, most recently from 49e465c to fdfba6b Compare January 21, 2025 11:15

marcelocarvalhoLowRisc force-pushed the gpio_straps_verif branch 5 times, most recently from 33ea687 to 1727a74 Compare January 23, 2025 11:30

rswarbrick mentioned this pull request Jan 23, 2025

Gpio straps stress all #25974

Open

rswarbrick reviewed Jan 23, 2025

View reviewed changes

martin-velay reviewed Jan 27, 2025

View reviewed changes

marcelocarvalhoLowRisc force-pushed the gpio_straps_verif branch 2 times, most recently from f9a778c to 2449eed Compare January 28, 2025 11:36

rswarbrick reviewed Jan 31, 2025

View reviewed changes

marcelocarvalhoLowRisc force-pushed the gpio_straps_verif branch 5 times, most recently from d06c4fb to b2e32ac Compare February 6, 2025 12:14

marcelocarvalhoLowRisc commented Feb 7, 2025

View reviewed changes

martin-velay reviewed Feb 7, 2025

View reviewed changes

martin-velay approved these changes Feb 7, 2025

View reviewed changes

marcelocarvalhoLowRisc force-pushed the gpio_straps_verif branch 3 times, most recently from 841b660 to cca3977 Compare February 7, 2025 16:18

marcelocarvalhoLowRisc commented Feb 7, 2025

View reviewed changes

New testcase for the GPIO feature straps

390be45

- Fix the gpio_stress_all with random reset PR:25657 was taking into this commit. Signed-off-by: Marcelo Carvalho Faleiro de Almeida <[email protected]>

marcelocarvalhoLowRisc force-pushed the gpio_straps_verif branch from cca3977 to 390be45 Compare February 7, 2025 16:23

marcelocarvalhoLowRisc requested a review from rswarbrick February 7, 2025 16:32

		// Used to allow reset operation during a stress all tests and check the CSR after that.
		can_reset_with_csr_accesses = 1'b1;

		`DV_CHECK_FATAL(
		ral.hw_straps_data_in.predict(.value(gpio_i_driven),

		modport port_in(input strap_en, output sampled_straps);
		modport port_out(output strap_en, input sampled_straps);

		modport tb_if(output strap_en, input sampled_straps);


		always_ff @(posedge clk or negedge rst_n) begin

GPIO straps verif #25868

Are you sure you want to change the base?

GPIO straps verif #25868

Conversation

marcelocarvalhoLowRisc commented Jan 14, 2025

martin-velay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelocarvalhoLowRisc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-velay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment