Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[RV_DM] rv_dm_sba_debug_disabled_vseq #20739

Closed
wants to merge 1 commit into from
Closed

[RV_DM] rv_dm_sba_debug_disabled_vseq #20739

wants to merge 1 commit into from

Conversation

shahid-mehmod
Copy link
Contributor

The test ensures that the SBA interface is disabled when 'lc_hw_debug_en' is not set to true, ensuring no SBA TL accesses occur.

@shahid-mehmod shahid-mehmod requested review from a team and vogelpi as code owners December 28, 2023 07:33
@shahid-mehmod shahid-mehmod requested review from eshapira and removed request for a team December 28, 2023 07:33
@shahid-mehmod shahid-mehmod force-pushed the rv_dm_sba_debug_disabled branch 2 times, most recently from 6d224e7 to dde9d28 Compare December 28, 2023 07:45
@shahid-mehmod
Copy link
Contributor Author

@jdonjdon I have rebased and resolved all the conflicts as requested.
Kindly review it. Thanks

@jdonjdon jdonjdon self-requested a review December 29, 2023 16:23
jdonjdon
jdonjdon reviewed Dec 29, 2023
View reviewed changes
hw/ip/rv_dm/dv/env/rv_dm_scoreboard.sv Outdated
@@ -186,10 +186,14 @@ class rv_dm_scoreboard extends cip_base_scoreboard #(
item.sprint(uvm_default_line_printer)), UVM_HIGH)
if (sba_tl_access_q.size() > 0) begin
compare_sba_access(item, sba_tl_access_q.pop_front());
end else if (cfg.rv_dm_vif.lc_hw_debug_en==lc_ctrl_pkg::Off) begin
`uvm_info(`gfn, $sformatf("Does not receive SBA access item:\n%0s",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 190: use 2 space indentations.

jdonjdon
jdonjdon reviewed Dec 29, 2023
View reviewed changes
hw/ip/rv_dm/dv/env/seq_lib/rv_dm_sba_debug_disabled_vseq.sv Outdated
randomize_req(req);
cfg.debugger.sba_access(req);
cfg.clk_rst_vif.wait_clks($urandom_range(0, 1000));
cfg.rv_dm_vif.lc_hw_debug_en<=lc_ctrl_pkg::Off;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test plan says non-true. So use following syntax to assign random non-true value.

cfg.rv_dm_vif.lc_hw_debug_en = get_rand_lc_tx_val([.t_weight(0), .f_weight(1), .other_weight(4));

jdonjdon
jdonjdon reviewed Dec 29, 2023
View reviewed changes
Copy link
Contributor

@jdonjdon jdonjdon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Shahid-Mehmood-S , please see in lines

@shahid-mehmod shahid-mehmod force-pushed the rv_dm_sba_debug_disabled branch from dde9d28 to 3bf2167 Compare January 18, 2024 04:53
jdonjdon
jdonjdon reviewed Jan 18, 2024
View reviewed changes
hw/ip/rv_dm/dv/env/seq_lib/rv_dm_sba_debug_disabled_vseq.sv Outdated
randomize_req(req);
cfg.debugger.sba_access(req);
cfg.clk_rst_vif.wait_clks($urandom_range(0, 1000));
cfg.rv_dm_vif.lc_hw_debug_en = get_rand_lc_tx_val([.t_weight(0), .f_weight(1), .other_weight(4));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you have a typo in line 24. remove '['.

@rswarbrick
Copy link
Contributor

If I understand correctly, this change is intended to fix issue #15668. Is that correct? If so, it's probably worth marking it in GitHub as such.

rswarbrick
rswarbrick reviewed Jan 18, 2024
View reviewed changes
hw/ip/rv_dm/dv/env/rv_dm_scoreboard.sv Outdated
@@ -186,10 +186,14 @@ class rv_dm_scoreboard extends cip_base_scoreboard #(
item.sprint(uvm_default_line_printer)), UVM_HIGH)
if (sba_tl_access_q.size() > 0) begin
compare_sba_access(item, sba_tl_access_q.pop_front());
end else if (cfg.rv_dm_vif.lc_hw_debug_en==lc_ctrl_pkg::Off) begin
`uvm_info(`gfn, $sformatf("Does not receive SBA access item:\n%0s",
item.sprint(uvm_default_line_printer)), UVM_HIGH)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Indentation is off here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The re-indented version is still incorrect (with one extra space). Once you've got the code correct, please can you indent it correctly? (Most editors can do this for you on a given line)

hw/ip/rv_dm/dv/env/rv_dm_scoreboard.sv Outdated Show resolved Hide resolved
hw/ip/rv_dm/dv/env/rv_dm_scoreboard.sv Outdated Show resolved Hide resolved
hw/ip/rv_dm/dv/env/seq_lib/rv_dm_sba_debug_disabled_vseq.sv Outdated Show resolved Hide resolved
@rswarbrick rswarbrick mentioned this pull request Jan 18, 2024
Closed
@shahid-mehmod shahid-mehmod force-pushed the rv_dm_sba_debug_disabled branch from 3bf2167 to 7860729 Compare January 22, 2024 10:20
@shahid-mehmod
[RV_DM] rv_dm_sba_debug_disabled_vseq
89b8535 
The test ensures that the SBA interface is disabled when 'lc_hw_debug_en' is not set to true, ensuring no SBA TL accesses occur.

Signed-off-by: Shahid Mehmood <[email protected]>
@shahid-mehmod shahid-mehmod force-pushed the rv_dm_sba_debug_disabled branch from 7860729 to 89b8535 Compare January 22, 2024 10:48
rswarbrick
rswarbrick requested changes Jan 22, 2024
View reviewed changes
Copy link
Contributor

@rswarbrick rswarbrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposed change to rv_dm_scoreboard is something we'd need to be a bit careful about. If it needs to happen, it definitely needs to be in a different commit from the change that adds the vseq.

hw/ip/rv_dm/dv/env/rv_dm_scoreboard.sv
@@ -186,10 +186,10 @@ class rv_dm_scoreboard extends cip_base_scoreboard #(
item.sprint(uvm_default_line_printer)), UVM_HIGH)
if (sba_tl_access_q.size() > 0) begin
compare_sba_access(item, sba_tl_access_q.pop_front());
end else begin
`uvm_error(`gfn, $sformatf({"Received predicted SBA access but no transaction was seen on ",
end else if (cfg.rv_dm_vif.lc_hw_debug_en==lc_ctrl_pkg::Off) begin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not completely sure I understand this. I think the previous code was dying with an error if we saw an SBA access that wasn't expected. Your patch demotes this to uvm_info / UVM_HIGH and makes it happen less often (checking lc_hw_debug_en).

We definitely should not make this change without some careful thought and explanatory comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdonjdon jdonjdon jdonjdon left review comments

@rswarbrick rswarbrick rswarbrick requested changes

@vogelpi vogelpi Awaiting requested review from vogelpi vogelpi is a code owner

@eshapira eshapira Awaiting requested review from eshapira eshapira is a code owner automatically assigned from lowRISC/ot-dv-reviewers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shahid-mehmod @rswarbrick @jdonjdon