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

[assert] Add ASSERT_KNOWN asserts to multiple new outputs #25999

Merged
merged 12 commits into from
Jan 24, 2025

Conversation

Razer6
Copy link
Member

@Razer6 Razer6 commented Jan 24, 2025

This adds ASSERT_KNOWN assertions to many outputs of the newly introduced signals.

It also uncovered a bug. The RACL error may be 'x because the internal address hit depends on the incoming TLUL address, which might be 'x at some cases. To fix this, the a_valid signal is factored in to the RACL error computation.

@Razer6 Razer6 requested review from vogelpi and rswarbrick January 24, 2025 10:01
Copy link
Contributor

@vogelpi vogelpi left a comment

Choose a reason for hiding this comment

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

Thanks @Razer6 this looks good to me!

Nits:

  • Some comments mention that the outputs need to be known after reset. But AFAIK the SVA actually checks they're known all the time.
  • One commit message has nbx in the title, I think it should be mbx.

@vogelpi
Copy link
Contributor

vogelpi commented Jan 24, 2025

CHANGE AUTHORIZED: hw/ip/i2c/rtl/i2c.sv
CHANGE AUTHORIZED: hw/ip/pwm/rtl/pwm.sv
CHANGE AUTHORIZED: hw/ip/spi_host/rtl/spi_host.sv
CHANGE AUTHORIZED: hw/ip/tlul/rtl/tlul_adapter_reg_racl.sv
CHANGE AUTHORIZED: hw/ip/uart/rtl/uart.sv
CHANGE AUTHORIZED: hw/ip/i2c/rtl/i2c_reg_top.sv
CHANGE AUTHORIZED: hw/ip/pwm/rtl/pwm_reg_top.sv
CHANGE AUTHORIZED: hw/ip/uart/rtl/uart_reg_top.sv

This PR only adds missing ASSERT_KNOWN assertions. This doesn't have functional impact.

@Razer6
Copy link
Member Author

Razer6 commented Jan 24, 2025

  • Some comments mention that the outputs need to be known after reset. But AFAIK the SVA actually checks they're known all the time.
  • One commit message has nbx in the title, I think it should be mbx.

I changed the mbx commit message. As discussed offline, the comment about after the reset is correct. SVA's are disabled during reset.

@Razer6 Razer6 requested a review from msfschaffner as a code owner January 24, 2025 11:51
@Razer6 Razer6 removed the request for review from msfschaffner January 24, 2025 11:52
Razer6 added 12 commits January 24, 2025 03:54
The address can have 'x and this would be progated to the RACL
error.

Signed-off-by: Robert Schilling <[email protected]>
@rswarbrick
Copy link
Contributor

CHANGE AUTHORIZED: hw/ip/i2c/rtl/i2c.sv
CHANGE AUTHORIZED: hw/ip/i2c/rtl/i2c_reg_top.sv
CHANGE AUTHORIZED: hw/ip/pwm/rtl/pwm.sv
CHANGE AUTHORIZED: hw/ip/pwm/rtl/pwm_reg_top.sv
CHANGE AUTHORIZED: hw/ip/spi_host/rtl/spi_host.sv
CHANGE AUTHORIZED: hw/ip/tlul/rtl/tlul_adapter_reg_racl.sv
CHANGE AUTHORIZED: hw/ip/uart/rtl/uart.sv
CHANGE AUTHORIZED: hw/ip/uart/rtl/uart_reg_top.sv

This PR is just adding some extra assertions (that we originally missed). No change in functional behaviour.

@rswarbrick rswarbrick merged commit 293f57a into lowRISC:master Jan 24, 2025
37 checks passed
@Razer6 Razer6 deleted the assert-known branch January 24, 2025 13:59
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