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

Flash status synchronisation tb fix #21977

Conversation

antmarzam
Copy link
Contributor

TB has been updated to check for flash_status bits correctness.

SPI-monitor now triggers SV events on each byte sampled on the SPI bus as well as when the CSB is deasserted.

The intercept VSEQ has been updated so reads/writes to flash_status won't occur just sequentially before/after a SPI-txn but also concurrently, as well as sending two flash_status writes in a row (less likely) where the first writes the upper bits of the flash_status register, and the second would clear the busy/wel bits.

The Scoreboard has been updated to be in sync with the latest flash_status behaviour changes in the RTL:

  • SPI reads to FLASH_STATUS (READ_STATUS#1/2/3 commands ) no longer wrap around the register if the CSB was left "active" after the first byte is returned to the host by the RTL
  • Writes to flash_status make their way towards the CDC SPI-side at each of the bytes sent to/from the host.
  • TL-UL view of flash_status updates on CSB being inactive.

@antmarzam antmarzam requested a review from a team as a code owner March 12, 2024 15:06
@antmarzam antmarzam requested review from matutem and removed request for a team March 12, 2024 15:06
@antmarzam
Copy link
Contributor Author

This PR takes care of issue #21111

@antmarzam antmarzam force-pushed the flash_status_and_addr_mode_synchronisation_tb_fix branch from b263980 to dd07f33 Compare March 12, 2024 15:10
@antmarzam antmarzam force-pushed the flash_status_and_addr_mode_synchronisation_tb_fix branch from dd07f33 to acd4bf3 Compare March 18, 2024 10:27
@antmarzam antmarzam force-pushed the flash_status_and_addr_mode_synchronisation_tb_fix branch 3 times, most recently from e4c9841 to 8ef0cef Compare March 22, 2024 16:49
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.

I've left lots of nitty style comments (sorry) on some of the first commit. Sorry that it has taken so long.

I'm a big fan of the extra UVM_DEBUG logs: I've never really looked at code where that turns out to be useful. It's cool to see them in use!

hw/dv/sv/spi_agent/spi_item.sv Outdated Show resolved Hide resolved
hw/dv/sv/spi_agent/spi_monitor.sv Outdated Show resolved Hide resolved
hw/dv/sv/spi_agent/spi_monitor.sv Outdated Show resolved Hide resolved
hw/ip/spi_device/dv/env/spi_device_scoreboard.sv Outdated Show resolved Hide resolved
hw/ip/spi_device/dv/env/spi_device_scoreboard.sv Outdated Show resolved Hide resolved
hw/ip/spi_device/dv/env/spi_device_scoreboard.sv Outdated Show resolved Hide resolved
hw/ip/spi_device/dv/env/spi_device_scoreboard.sv Outdated Show resolved Hide resolved
hw/ip/spi_device/dv/env/spi_device_scoreboard.sv Outdated Show resolved Hide resolved
@antmarzam antmarzam force-pushed the flash_status_and_addr_mode_synchronisation_tb_fix branch 2 times, most recently from 9dda7a7 to 426da15 Compare March 27, 2024 16:20
Flash status checks have been reworked and upgraded (including
READ_STATUS commands no longer wrap around the flash_status value)
There's a new Analysis Port in the monitor which sends the SPI
seq_item on CSB being active, and then whichever component is hooked
to that AP could get notified on each sampled byte going through the SPI-bus
and on CSB going inactive via SV events in the sequence item.
SPI sequence item now was 2 SV events which are triggered by the
monitor each time a byte is sampled and when the CSB goes inactive.

Status bits towards the host-side are committed on each SPI-byte,
whereas the TL-UL side is updated on each CSB assertion (CSB===1)

The TB uses several SV queues to model the view at each side of the
CDC, including fuzzy queues where temporary values are held until
they're known to have travel towards each of the CDC domains

Signed-off-by: Antonio Martinez Zambrana <[email protected]>
@antmarzam antmarzam force-pushed the flash_status_and_addr_mode_synchronisation_tb_fix branch from 426da15 to 48a4443 Compare March 27, 2024 17:07
The intercept sequence now also randomly drives:
- concurrent TL-UL and SPI side transactions
- sequential TL-UL (on CSB inactive) and SPI-TXn
- Double TL-UL flash status write in parallel with SPI txn.
  This double write is sent when the busy bit is set and it comprises
  of the first write setting the upper bits of the status register and
  the second write clearing the busy/wel bits.

The intr_state task now rather than making a read every X random
clk_cycles reads the intr_state every cycle through the backdoor and
if there are interrupts, then it does a front-door read to update the
predicted values in SCB. Otherwise, it may happen the TB doesn't
repopulate either half of the read buffer "early enough" leading to
memory mismatches.

Signed-off-by: Antonio Martinez Zambrana <[email protected]>
@antmarzam antmarzam force-pushed the flash_status_and_addr_mode_synchronisation_tb_fix branch from 48a4443 to ee997eb Compare March 28, 2024 11:53
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.

This looks good, thanks. As a minor nit, I think you addressed some of my style comments in the second commit, so we sometimes get a sequence of "... mess up style; fix style ...". But amending exactly the right commit is a pain!

On the plus side, magit has a reasonably nice way of doing this: either @hcallahan-lowrisc or I can probably show you in person at some point. Another win for Emacs :-)

But it's definitely not worth re-doing everything for. Thank you very much for tidying things up.

@rswarbrick rswarbrick merged commit ec43583 into lowRISC:master Mar 28, 2024
31 checks passed
@a-will
Copy link
Contributor

a-will commented Apr 3, 2024

Was a significant subset of tests run on this? It looks like regression results have taken a big dive between before this change (https://reports.opentitan.org/hw/ip/spi_device_2p/dv/2024.03.29_04.21.20/report.html) and after (https://reports.opentitan.org/hw/ip/spi_device_2p/dv/2024.04.01_04.52.26/report.html).

Are those real failures?

@antmarzam
Copy link
Contributor Author

Was a significant subset of tests run on this? It looks like regression results have taken a big dive between before this change (https://reports.opentitan.org/hw/ip/spi_device_2p/dv/2024.03.29_04.21.20/report.html) and after (https://reports.opentitan.org/hw/ip/spi_device_2p/dv/2024.04.01_04.52.26/report.html).

Are those real failures?

I run quite a bunch of tests for the flash_status changes. I had some spurious failures of tests timing out which passed once I rerun them.
I'll investigate!

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