Implement a SyncReadMem wrapper with explicit read, write, and read/write ports

[jared-barocsi]

This PR implements a SRAM object, which wraps a SyncReadMem and instantiates a desired number of read, write, and read/write ports.

For example:

val memory = SRAM(UInt(8.W), 1000, 2, 2, 1)

generates a 1R, 2W, 1RW SyncReadMem with a size of 1000, and is controlled like so:

memory.rd(0).enable := timeToRead
memory.rd(0).addr := 3.U
// one cycle later after timeToRead is driven
fooData := memory.rd(0).readValue

memory.wr(1).enable := timeToWrite
memory.rw(1).addr := 100.U
memory.wr(1).writeValue := barData

memory.rw(0).enable := timeToReadWrite
memory.rw(0).isWrite := timeToWrite
memory.rw(0).writeData := bazData
memory.rw(0).addr := 50.U
// one cycle later when timeToReadWrite is driven but not timeToWrite
fizzData := memory.rw(0).readValue

Contributor Checklist

• Did you add Scaladoc to every public function/method?
• Did you add at least one test demonstrating the PR?
• Did you delete any extraneous printlns/debugging code?
• Did you specify the type of improvement?
• Did you add appropriate documentation in docs/src?
• Did you request a desired merge strategy?
• Did you add text to be included in the Release Notes for this change?

Type of Improvement

Desired Merge Strategy

Squash and merge

Release Notes

This SyncReadMem wrapper is instantiated using a new object, SRAM.apply, and invokes .write, .read, and .readWrite to generate a desired number of read, write, and read/write ports. This function returns a new Bundle wire containing the control signals for each requested port.

Reviewer Checklist (only modified by reviewer)

• Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
• Did you mark the proper milestone (Bug fix: 3.5.x or 3.6.x depending on impact, API modification or big change: 5.0.0)?
• Did you review?
• Did you check whether all relevant Contributor checkboxes have been checked?
• Did you do one of the following when ready to merge:
  • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
  • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

[jackkoenig]

The overall code here is really good with a couple of minor issues. My main comments are bikeshedding about names but as this is probably going to become the main SRAM API for a lot of users, I think we should spend the time to get the names right.

[jackkoenig]

This also needs to consider #3278, it should probably be illegal to mix masked and unmasked write ports.

[schoeberl]

Comments on names: simply call it Memory. That it is an interface to SyncReadMem is an implementation detail.

[schoeberl]

As a inspiration for names the Altera/Intel on-chip memory documentation, see around page 4-2
https://www.intel.com/programmable/technical-pdfs/654378.pdf

[mwachs5]

Yeah this should definitely be called SyncReadMem, and if we want we could do something like SyncReadMem.interface(...): MemInterface

One thing I want to agree on is whether we want to distinguish a MemInterface and SyncReadMemInterface in the Bundle subclass types... you could imagine wrapping a combinational mem with the same MemInterface and someone getting confused because the bits are the same but the protocol is different. If we only care about this for now for SyncReadMem let's call the bundles SyncReadMemInterface (or whatever jack suggested) too

[mwachs5]

Yeah this should definitely be called SyncReadMem, and if we want we could do something like SyncReadMem.interface(...): MemInterface

One thing I want to agree on is whether we want to distinguish a MemInterface and SyncReadMemInterface in the Bundle subclass types... you could imagine wrapping a combinational mem with the same MemInterface and someone getting confused because the bits are the same but the protocol is different. If we only care about this for now for SyncReadMem let's call the bundles SyncReadMemInterface (or whatever jack suggested) too

[sequencer]

For the memory wrapper, I have these questions:

• what IR it will be lowered to in the future?(hopefully a memory intrinsic).
• why not use a Module to wrap it like Structural Memory #3131 does.
• for DFT flow, how to meet the MBIST needs.

I personally suggest using the

case class SRAMPortInfo(hasRead: Boolean, hasWrite: Boolean, hasMask: Boolean, portDelay: Int = 1)
class SRAMPort(portInfo: SRAMPortInfo, addressWidth: Int, dataWidth: Int) extends Bundle
class SRAM(width: Int, depth: Int, memoryPorts: Seq[SRAMPortInfo], extraIO: Option[Record] = None, contents: Option[os.Path] = None, macroName: Option[String] = None) extends Module 

from #3131, since I think:

• unlike register/wires, memory are always hard-marco from IP vendors, post-syn flow need it to be module and instance.
• API in this PR(chisel types for data, object user interfaces) is less intuitive than a single Module and Bundle I suggest using that and let users handle Data to UInt conversion and byte-level granularity mask.
• a custom port should be provided in the future PR, it's too much suffer for MBIST design experience with Chisel :(

[jackkoenig]

@sequencer these are all great questions, I think we should discuss at Chisel dev on Monday!

[jackkoenig]

Looks good, would like a couple of changes to the explanation, then let's get this merged!

Concerns have been addressed or are established as future work