WishboneSRAM support for single-port RAM

[kivikakk]

WishboneSRAM can never service a read and a write request at the same time, so it's possible to improve support for SPRAM by disabling the read port when the write-enable is set.

This stops interfering with SPRAM inference where the underlying Memory primitive could otherwise match one.

[kivikakk]

Well, reading these tests have been instructive :) I hadn't imagined we might actually expect to see the pre-write value on dat_r and thus didn't anticipate a test for it!

[kivikakk]

My reading of the Wishbone spec suggests there isn't any 'permission' to do so (and e.g. write cycles don't mention DAT_I at all), so I guess we could consider:

• relax the test and no longer expect read data on a write cycle.
  • I kind of like this option because I imagine there's a reasonable chance the data won't be passed through other layers anyway, when WE is asserted, since there's no provision in the spec for it. That said, I'm very inexperienced here.
• do nothing.
• add an option to WishboneSRAM which controls this behaviour. (I like this the least.)

[whitequark]

My understanding is that DAT_I on a write cycle is don't care, as seen on page 44 of the Wishbone B4 spec:

[whitequark]

Or phrased differently, a test that checks DAT_I during write cycles is clearly invalid in my understanding, and it shouldn't be doing that.

[kivikakk]

Yep, agreed — I've pushed a commit that removes those assertions accordingly.

[jfng]

The current implementation does a simultaneous read and write when we is asserted.

I think WishboneSRAM can be modified to do the write on the next clock cycle. Bus transactions already have a latency of 2 cycles, and adr and w_data are assumed to remain constant for the duration of a transaction.

Otherwise, maybe it is possible to build a custom dual-port RAM out of SPRAM primitives ? Using similar ideas as this blog post.
Then WishboneSRAM can be modified to accept user-provided memories.

[jfng]

My understanding is that DAT_I on a write cycle is don't care, as seen on page 44 of the Wishbone B4 spec:

If this is true, then just removing the test is fine indeed.

[kivikakk]

OK — thank you both! I think this PR is good to go, then.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.56%. Comparing base (9d6bd2c) to head (d30252e).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #100   +/-   ##
=======================================
  Coverage   99.56%   99.56%           
=======================================
  Files          11       11           
  Lines        1601     1602    +1     
  Branches      387      387           
=======================================
+ Hits         1594     1595    +1     
  Misses          4        4           
  Partials        3        3           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jfng]

Thank you!