[edn/dv] Fix failing err vseq #20808

h-filali · 2024-01-11T09:16:56Z

This commit fixes the failing edn_err_vseq.
For further information have a look at the individual commits.

This commit also does part of the clean up mentioned in #18968 because why not :D

rswarbrick · 2024-01-11T10:46:59Z

hw/ip/edn/rtl/edn_pkg.sv

@@ -48,4 +48,13 @@ package edn_pkg;
    Error             = 9'b010010001  // illegal state reached and hang
  } state_e;

+  typedef enum logic [8:0] {


Could this enum go into edn_env_pkg instead of being in the RTL directory? I think the uses are all on the DV side for now, at least.

I thought the same. It would allow us to merge the change immediately. Once the RTL blocker is lifted, we can move the change back into the RTL package file and make the remaining RTL also use that to reduce code duplication.

That's a great idea, thanks to both of you.

rswarbrick · 2024-01-11T10:53:26Z

hw/ip/edn/dv/env/seq_lib/edn_err_vseq.sv

+        if (cfg.boot_req_mode != MuBi4True && cfg.auto_req_mode == MuBi4True
+            && exp_state != AutoLoadIns) begin
+          // Send INS cmd to start the auto/SW mode operation.
+          // Don't use wr_cmd() since we don't want to wait for the ack.


Is there a way to tweak wr_cmd to easily add a "don't wait for the ack" mode? (This is a genuine question: I haven't thought about it hard, and I notice that wr_cmd is already reasonably complicated)

Should be rather easy I think. I'll add a commit that implements this.

rswarbrick · 2024-01-11T10:56:37Z

hw/ip/edn/dv/env/seq_lib/edn_err_vseq.sv

@@ -48,6 +49,7 @@ class edn_err_vseq extends edn_base_vseq;
    end

    `uvm_info(`gfn, $sformatf("Waiting for main_sm to reach state %s", exp_state.name()), UVM_HIGH)
+    transition_to_main_sm_state(exp_state, mode);


I wonder whether we should tweak the name to something more like start_main_sm_state_transition or similar. That might make the fork / join none behaviour a bit more obvious?

Sure, that makes sense.

rswarbrick · 2024-01-11T11:54:38Z

hw/ip/edn/dv/env/seq_lib/edn_err_vseq.sv

    string state_path;
-    logic [8:0] exp_state;
+    state_ack_e exp_state;


I wonder whether it would make sense to extract the translation to using the enum into a separate commit. The resulting history might be a bit easier to follow.

Sure, can do!

rswarbrick · 2024-01-11T11:56:04Z

hw/ip/edn/dv/env/seq_lib/edn_err_vseq.sv


    `uvm_info(`gfn, $sformatf("Waiting for ack_sm[%0d] to reach state %09b",
                              endpoint_port, exp_state), UVM_HIGH)
+    transition_to_ack_state(exp_state, mode);


Same question as above about switching to a name that's more like "start transition" for this task too.

rswarbrick · 2024-01-11T11:57:52Z

hw/ip/edn/dv/env/seq_lib/edn_err_vseq.sv

+          if (cfg.boot_req_mode != MuBi4True) begin
+            // Send INS cmd to start the auto/SW mode operation.
+            wr_cmd(.cmd_type(edn_env_pkg::Sw), .acmd(csrng_pkg::INS),
+                  .clen(0), .flags(MuBi4False), .glen(0), .mode(mode));


Nit: Indentation (and on the following call to wr_cmd too)

rswarbrick · 2024-01-11T11:59:01Z

hw/ip/edn/dv/env/seq_lib/edn_err_vseq.sv

+        // INS and GEN commands are only needed if we are not waiting for EndPointClear or DataWait.
+        if (!exp_state inside {EndPointClear, DataWait}) begin


Shouldn't this "if" surround the fork/join_none? (I realise that it's equivalent, but maybe switching them around would feel a bit cleaner?)

Yes, that would be better, thanks.

rswarbrick · 2024-01-11T12:03:49Z

hw/ip/edn/dv/env/seq_lib/edn_err_vseq.sv

@@ -102,9 +104,53 @@ class edn_err_vseq extends edn_base_vseq;
    )
  endtask

+  task transition_to_ack_state(state_ack_e exp_state, hw_req_mode_e mode);


Nit: I think that the "await" tasks above are ordered main; ack. Could we switch the ordering of the "transition" tasks to match?

I'll swap the await tasks to make it alphabetically ordered. (at least for the two pairs)

This commit adds the enum type for all the ack sm states. This can be used to fix lowRISC#18968 in the following commit. Signed-off-by: Hakim Filali <[email protected]>

vogelpi

LGTM, but I would move the ack_state_e defintion into a DV package file for now. Thanks for working on this @h-filali !

vogelpi · 2024-01-11T12:27:10Z

hw/ip/edn/dv/env/seq_lib/edn_err_vseq.sv

+  endtask
+
+  task transition_to_main_sm_state(state_e exp_state, edn_env_pkg::hw_req_mode_e mode);
+    // Create background thread that transitions the ack sm to the expected state.


Nit: the ack sm -> the main sm

vogelpi · 2024-01-11T12:29:57Z

hw/ip/edn/dv/env/seq_lib/edn_err_vseq.sv

+    // Notify the driver that the EDN was disabled.
+    cfg.edn_vif.drive_edn_disable(1);
+    // In Auto mode, minimize number of requests between reseeds and set the reseed command.
+    if (cfg.auto_req_mode == MuBi4True) begin


Here you could already use the mode variable introduced above, right?

Yes, you're right.

vogelpi · 2024-01-11T12:34:18Z

hw/ip/edn/dv/env/seq_lib/edn_err_vseq.sv

-      gen_cmd_type = edn_env_pkg::Sw;
-      mode = edn_env_pkg::SwMode;
+    if ((cfg.which_err_code == edn_ack_sm_err) || ($urandom_range(10, 1) < 8)) begin
+      // Errors in ack_sm can be used to trigger transitions to the Error state in main_sm.


Maybe want to include the previous comment main_sm is more complex, so the random selection is biased towards it. here to motivate the OR with the urandom-expression?

Makes sense, thanks!

This commit changes the hardcoded ack state values in the edn_err_vseq to the enum type values in the env_pkg.. Signed-off-by: Hakim Filali <[email protected]>

This commit adds the option to skip waiting for a command to be acknowledged in wr_cmd(). This is needed, since if we expect there to be an error in a test sequence we don't want to wait for the command to complete. Signed-off-by: Hakim Filali <[email protected]>

The edn_err_vseq test seems to be failing for multiple reasons. Mainly because of a hanging wr_cmd(). This test sequence used to always write the same couple of commands no matter in what state the error state is induced. This commit reworks the test sequence such that only those commands are issued that are really needed to get the EDN into the wanted ack/main SM state. This way we can avoid hangs because of unnecessary commands. Furthermore, the err sequence now waits for a random ack/main SM state for all types of errors. This used to be the case only for ack_sm, main_sm and counter errors. Signed-off-by: Hakim Filali <[email protected]>

h-filali

Thanks @rswarbrick and @vogelpi for your reviews! Some great inputs.

h-filali · 2024-01-11T12:24:28Z

hw/ip/edn/rtl/edn_pkg.sv

@@ -48,4 +48,13 @@ package edn_pkg;
    Error             = 9'b010010001  // illegal state reached and hang
  } state_e;

+  typedef enum logic [8:0] {


That's a great idea, thanks to both of you.

h-filali · 2024-01-11T12:26:41Z

hw/ip/edn/dv/env/seq_lib/edn_err_vseq.sv

@@ -48,6 +49,7 @@ class edn_err_vseq extends edn_base_vseq;
    end

    `uvm_info(`gfn, $sformatf("Waiting for main_sm to reach state %s", exp_state.name()), UVM_HIGH)
+    transition_to_main_sm_state(exp_state, mode);


Sure, that makes sense.

h-filali · 2024-01-11T12:28:41Z

hw/ip/edn/dv/env/seq_lib/edn_err_vseq.sv

@@ -102,9 +104,53 @@ class edn_err_vseq extends edn_base_vseq;
    )
  endtask

+  task transition_to_ack_state(state_ack_e exp_state, hw_req_mode_e mode);


I'll swap the await tasks to make it alphabetically ordered. (at least for the two pairs)

h-filali · 2024-01-11T12:30:30Z

hw/ip/edn/dv/env/seq_lib/edn_err_vseq.sv

+        // INS and GEN commands are only needed if we are not waiting for EndPointClear or DataWait.
+        if (!exp_state inside {EndPointClear, DataWait}) begin


Yes, that would be better, thanks.

h-filali · 2024-01-11T12:40:19Z

hw/ip/edn/dv/env/seq_lib/edn_err_vseq.sv

-      gen_cmd_type = edn_env_pkg::Sw;
-      mode = edn_env_pkg::SwMode;
+    if ((cfg.which_err_code == edn_ack_sm_err) || ($urandom_range(10, 1) < 8)) begin
+      // Errors in ack_sm can be used to trigger transitions to the Error state in main_sm.


Makes sense, thanks!

h-filali · 2024-01-11T12:41:55Z

hw/ip/edn/dv/env/seq_lib/edn_err_vseq.sv

+    // Notify the driver that the EDN was disabled.
+    cfg.edn_vif.drive_edn_disable(1);
+    // In Auto mode, minimize number of requests between reseeds and set the reseed command.
+    if (cfg.auto_req_mode == MuBi4True) begin


Yes, you're right.

h-filali · 2024-01-11T12:54:20Z

hw/ip/edn/dv/env/seq_lib/edn_err_vseq.sv

+        if (cfg.boot_req_mode != MuBi4True && cfg.auto_req_mode == MuBi4True
+            && exp_state != AutoLoadIns) begin
+          // Send INS cmd to start the auto/SW mode operation.
+          // Don't use wr_cmd() since we don't want to wait for the ack.


Should be rather easy I think. I'll add a commit that implements this.

h-filali · 2024-01-11T12:54:48Z

hw/ip/edn/dv/env/seq_lib/edn_err_vseq.sv

    string state_path;
-    logic [8:0] exp_state;
+    state_ack_e exp_state;


Sure, can do!

h-filali requested a review from a team as a code owner January 11, 2024 09:16

h-filali requested review from rswarbrick and removed request for a team January 11, 2024 09:16

h-filali assigned vogelpi and unassigned vogelpi Jan 11, 2024

h-filali requested a review from vogelpi January 11, 2024 09:17

rswarbrick reviewed Jan 11, 2024

View reviewed changes

[edn/dv] Add enum type for ack sm states

2a0222e

This commit adds the enum type for all the ack sm states. This can be used to fix lowRISC#18968 in the following commit. Signed-off-by: Hakim Filali <[email protected]>

vogelpi approved these changes Jan 11, 2024

View reviewed changes

Hakim Filali added 3 commits January 11, 2024 12:45

[edn/dv] Use the new ack_sm enum type for the edn_err_vseq

5ad1aaa

This commit changes the hardcoded ack state values in the edn_err_vseq to the enum type values in the env_pkg.. Signed-off-by: Hakim Filali <[email protected]>

h-filali force-pushed the edn-fix-err-vseq branch from ed5e216 to a956848 Compare January 11, 2024 13:07

h-filali commented Jan 11, 2024

View reviewed changes

vogelpi merged commit d3f0ae7 into lowRISC:master Jan 11, 2024
32 checks passed

vogelpi mentioned this pull request Mar 29, 2024

[edn] D2S Signoff #20982

Closed

h-filali deleted the edn-fix-err-vseq branch October 7, 2024 14:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[edn/dv] Fix failing err vseq #20808

[edn/dv] Fix failing err vseq #20808

h-filali commented Jan 11, 2024

rswarbrick Jan 11, 2024

vogelpi Jan 11, 2024

h-filali Jan 11, 2024

rswarbrick Jan 11, 2024

h-filali Jan 11, 2024

rswarbrick Jan 11, 2024

h-filali Jan 11, 2024

rswarbrick Jan 11, 2024

h-filali Jan 11, 2024

rswarbrick Jan 11, 2024

rswarbrick Jan 11, 2024

rswarbrick Jan 11, 2024

h-filali Jan 11, 2024

rswarbrick Jan 11, 2024

h-filali Jan 11, 2024

vogelpi left a comment

vogelpi Jan 11, 2024

vogelpi Jan 11, 2024

h-filali Jan 11, 2024

vogelpi Jan 11, 2024

h-filali Jan 11, 2024

h-filali left a comment

h-filali Jan 11, 2024

h-filali Jan 11, 2024

h-filali Jan 11, 2024

h-filali Jan 11, 2024

h-filali Jan 11, 2024

h-filali Jan 11, 2024

h-filali Jan 11, 2024

h-filali Jan 11, 2024

		// INS and GEN commands are only needed if we are not waiting for EndPointClear or DataWait.
		if (!exp_state inside {EndPointClear, DataWait}) begin

[edn/dv] Fix failing err vseq #20808

[edn/dv] Fix failing err vseq #20808

Conversation

h-filali commented Jan 11, 2024

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

vogelpi 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

h-filali 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