Skip to content

Conversation

@yvantor
Copy link

@yvantor yvantor commented Jul 13, 2025

This PR introduces the OBI_ASSERT_ON define to shut down System Verilog assertions in the apb_2_obi IP. This became useful when simulating code with Verilator since it complains about:

%Error: [...]/apb_to_obi.sv:142:132: Define passed too many arguments: ASSERT
  142 |   ASSERT(penable, obi_phase_q == RESP |-> apb_req_i.penable, clk_i, !rst_ni, "APB PENABLE must be asserted during OBI RESP phase!")

It is not exactly clear to me why it complains for this, at the beginning I was supposing that defaulted values in the assertion arguments (like __desc = "") translated in Verilator hardcoding that argument to the default, not allowing an instance override. This does not seem to be the case, I think with other IPs (like stream_xbar in the common_cells) this problem does not appear because the assertions are all excluded by default.
This might also be useful in synthesis.

@yvantor yvantor requested review from micprog and niwis July 13, 2025 10:39
@yvantor yvantor self-assigned this Jul 13, 2025
Copy link
Member

@micprog micprog left a comment

Choose a reason for hiding this comment

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

Can we change this to add a define disabling the asserts rather than enabling them? I'd much prefer to have these enabled by default to propagate benefits rather than require users who want it to enable this feature. Adding a define to disable is OK IMO.

@micprog
Copy link
Member

micprog commented Aug 5, 2025

After offline discussion, I believe the verilator error you are seeing is caused by a redefinition of the ASSERT macro, not an issue with verilator. Nonetheless agree with adding a disable option as mentioned in the review.

Copy link
Member

@micprog micprog left a comment

Choose a reason for hiding this comment

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

Thanks!

@micprog micprog merged commit 416f806 into main Aug 5, 2025
4 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.

3 participants