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.

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

[rom_ctrl, dv] Exclusion of non-occuring cases for tlul_adapter_sram #25585

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

KinzaQamar
Copy link
Contributor

@KinzaQamar KinzaQamar commented Dec 10, 2024

t is impossible to see !tl_i_int.a_valid & !error_internal. When a_valid is false, it is seen
in u_err inside tlul_adapter_sram.sv. This will make a_config_allowed inside u_err false as it
depends on addr_sz_chk, mask_chk and fulldata_chk. All of them are false if a_valid is false.
This means that the first term inside err_o definition in u_err is false which raised err_o.
Next, err_o become visible to error_det as tlul_error inside tlul_adapter_sram and it will
be true if tlul_error is true. error_det will then be seen as error_i to u_sram_byte inside
tlul_adapter_sram.
Since EnableIntg parameter is 0 for rom_ctrl, error_i comes out as error_o from u_sram_byte.
Then error_o would be seen as error_internal to tlul_adapter_sram and the conditional statement
is not able to cover 00 case for this reason.

It is impossible to get sram_ack & we_o. if sram_ack is true, then we have (req_o & gnt_i) = 1.
For req_o to become true requires error_internal as false. we_o become true when there is an
a_valid and a_opcode as Put. But when a_opcode is a Put, wr_vld_error in the adapter becomes
true as ErrOnWrite is true for the adapter in case of rom_ctrl. Which means that error_internal
is true and req_o is false, which leads to sram_ack being false.

@rswarbrick
Copy link
Contributor

It looks like we diagnosed this wrongly, which is causing CI failures from e.g. the sram_ctrl smoke test. Would you mind taking a look to figure out what we got wrong?

@KinzaQamar KinzaQamar marked this pull request as draft December 11, 2024 15:44
@KinzaQamar KinzaQamar force-pushed the tlul_adapter_sram branch 3 times, most recently from 4d7a1de to 720b25c Compare December 11, 2024 16:04
@KinzaQamar KinzaQamar changed the title [rom_ctrl, rtl] tlul_adapter_sram case 00 for tl_i_int and error_inte… [rom_ctrl, dv] Exclusion of non-occuring cases for tlul_adapter_sram Dec 11, 2024
@KinzaQamar KinzaQamar marked this pull request as ready for review December 11, 2024 19:10
@KinzaQamar KinzaQamar requested a review from a team as a code owner December 11, 2024 19:10
@KinzaQamar KinzaQamar force-pushed the tlul_adapter_sram branch 3 times, most recently from 6bbaaf8 to 4d96421 Compare December 16, 2024 13:21
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.

Tiny nits about the comments, but thank you for the careful explanations. This looks good to me!

hw/ip/rom_ctrl/dv/cov/rom_ctrl_cov_excl.el Outdated Show resolved Hide resolved
hw/ip/rom_ctrl/dv/cov/rom_ctrl_cov_excl.el Outdated Show resolved Hide resolved
It is impossible to see !tl_i_int.a_valid & !error_internal. When a_valid is false, it is seen
in u_err inside tlul_adapter_sram.sv. This will make a_config_allowed inside u_err false as it
depends on addr_sz_chk, mask_chk and fulldata_chk. All of them are false if a_valid is false.
This means that in u_err, the first term of err_o becomes true.
Next, err_o become visible to error_det as tlul_error inside tlul_adapter_sram and it will
be true if tlul_error is true. error_det will then be seen as error_i to u_sram_byte inside
tlul_adapter_sram.
Since EnableIntg parameter is 0 for rom_ctrl, error_i comes out as error_o from u_sram_byte.
Then error_o would be seen as error_internal to tlul_adapter_sram and the conditional statement
is not able to cover 01 case for this reason.

It is impossible to get sram_ack & we_o. we_o becomes true when there is an a_valid and a_opcode
as Put. But when a_opcode is a Put, wr_vld_error in the adapter becomes true as ErrOnWrite is
true for the adapter in case of rom_ctrl. wr_vld_error lets error_det become true. Since
rom_ctrl doesn't enable integrity errors, error_det comes out as error_internal directly from
u_sram_byte. But this makes req_o false and hence sram_ack false.

Signed-off-by: Kinza Qamar <[email protected]>
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.

Thanks for this: looks good to me!

@rswarbrick rswarbrick merged commit c168e77 into lowRISC:master Dec 18, 2024
22 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants