-
Notifications
You must be signed in to change notification settings - Fork 589
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
SRAM API: Add multiple-clocked port API #3383
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, some style suggestions
private def memInterface_impl[T <: Data]( | ||
size: BigInt, | ||
tpe: T | ||
)(readPortClocks: Seq[Clock], | ||
writePortClocks: Seq[Clock], | ||
readwritePortClocks: Seq[Clock], | ||
memoryFile: Option[MemoryFile] | ||
)(evidenceOpt: Option[T <:< Vec[_]], | ||
sourceInfo: SourceInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a real purpose to having 3 parameter lists here? Since this is a private method I would just defined it with a normal single parameter list.
private def memInterface_impl[T <: Data]( | |
size: BigInt, | |
tpe: T | |
)(readPortClocks: Seq[Clock], | |
writePortClocks: Seq[Clock], | |
readwritePortClocks: Seq[Clock], | |
memoryFile: Option[MemoryFile] | |
)(evidenceOpt: Option[T <:< Vec[_]], | |
sourceInfo: SourceInfo | |
private def memInterface_impl[T <: Data]( | |
size: BigInt, | |
tpe: T, | |
readPortClocks: Seq[Clock], | |
writePortClocks: Seq[Clock], | |
readwritePortClocks: Seq[Clock], | |
memoryFile: Option[MemoryFile], | |
evidenceOpt: Option[T <:< Vec[_]], | |
sourceInfo: SourceInfo |
for ((clock, i) <- readPortClocks.zipWithIndex) { | ||
_out.readPorts(i).data := mem.read(_out.readPorts(i).address, _out.readPorts(i).enable, clock) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for ((clock, i) <- readPortClocks.zipWithIndex) { | |
_out.readPorts(i).data := mem.read(_out.readPorts(i).address, _out.readPorts(i).enable, clock) | |
} | |
for ((clock, port) <- readPortClocks.zip(_out.readPort)) { | |
port.data := mem.read(port.address, port.enable, clock) | |
} |
I should've caught this with the earlier PR, but unless you are writing performance critical code using Arrays, indexing a Seq is code smell. This same change for the write and readwrite ports will make those loops much cleaner.
memInterface_impl(size, tpe)( | ||
Seq.fill(numReadPorts)(Builder.forcedClock), | ||
Seq.fill(numWritePorts)(Builder.forcedClock), | ||
Seq.fill(numReadwritePorts)(Builder.forcedClock), | ||
None | ||
)( | ||
Some(evidence), | ||
sourceInfo | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
memInterface_impl(size, tpe)( | |
Seq.fill(numReadPorts)(Builder.forcedClock), | |
Seq.fill(numWritePorts)(Builder.forcedClock), | |
Seq.fill(numReadwritePorts)(Builder.forcedClock), | |
None | |
)( | |
Some(evidence), | |
sourceInfo | |
) | |
): SRAMInterface[T] = { | |
val clock = Builder.forcedClock | |
memInterface_impl(size, tpe)( | |
Seq.fill(numReadPorts)(clock), | |
Seq.fill(numWritePorts)(clock), | |
Seq.fill(numReadwritePorts)(clock), | |
None | |
)( | |
Some(evidence), | |
sourceInfo | |
) | |
} |
This would be cleaner with just assigning clock to a val
, also that's just generally a good idea because Builder.forcedClock
requires accessing the Chisel runtime in the Dynamic variable which is just unnecessary extra compute to do more than once here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same suggestion for the other places calling Builder.forcedClock
a bunch.
src/test/scala/chiselTests/Mem.scala
Outdated
val (counter, _) = Counter(true.B, 11) | ||
val clock1: Clock = (counter === 1.U).asClock | ||
val clock2: Clock = (counter === 2.U).asClock | ||
val clock3: Clock = (counter === 3.U).asClock | ||
|
||
val readClocks = Seq(clock1, clock2, clock3) | ||
val writeClocks = Seq(clock2, clock3, clock1) | ||
val readwriteClocks = Seq(clock3, clock1, clock2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fun but maybe too clever by half, it would be more straightforward to just have Vecs of clocks (which are Seqs)
val (counter, _) = Counter(true.B, 11) | |
val clock1: Clock = (counter === 1.U).asClock | |
val clock2: Clock = (counter === 2.U).asClock | |
val clock3: Clock = (counter === 3.U).asClock | |
val readClocks = Seq(clock1, clock2, clock3) | |
val writeClocks = Seq(clock2, clock3, clock1) | |
val readwriteClocks = Seq(clock3, clock1, clock2) | |
val readClocks = IO(Input(Vec(3, Clock()))) | |
val writeClocks = IO(Input(Vec(3, Clock()))) | |
val readwriteClocks = IO(Input(Vec(3, Clock()))) |
Then below you just look for the clocks, eg. readClocks[$i]
memInterface_impl( | ||
size, | ||
tpe, | ||
Seq.fill(numReadPorts)(Builder.forcedClock), | ||
Seq.fill(numWritePorts)(Builder.forcedClock), | ||
Seq.fill(numReadwritePorts)(Builder.forcedClock), | ||
Some(memoryFile), | ||
Some(evidence), | ||
sourceInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please assign Builder.forcedClock
to a val here as well.
memInterface_impl( | ||
size, | ||
tpe, | ||
Seq.fill(numReadPorts)(Builder.forcedClock), | ||
Seq.fill(numWritePorts)(Builder.forcedClock), | ||
Seq.fill(numReadwritePorts)(Builder.forcedClock), | ||
None, | ||
Some(evidence), | ||
sourceInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please assign Builder.forcedClock
to a val here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but please address the 2 minor nits
This PR adds new APIs for the SRAM module to instantiate a number of ports connected to individual clocks. This allows the creation of memories driven by multiple clocks, for use cases like clock domain crossover. (cherry picked from commit d79ae71) # Conflicts: # src/test/scala/chiselTests/Mem.scala
This PR adds new APIs for the SRAM module to instantiate a number of ports connected to individual clocks. This allows the creation of memories driven by multiple clocks, for use cases like clock domain crossover. (cherry picked from commit d79ae71) # Conflicts: # src/test/scala/chiselTests/Mem.scala
* SRAM API: Add multiple-clocked port API (#3383) This PR adds new APIs for the SRAM module to instantiate a number of ports connected to individual clocks. This allows the creation of memories driven by multiple clocks, for use cases like clock domain crossover. (cherry picked from commit d79ae71) --------- Co-authored-by: Jared Barocsi <82000041+jared-barocsi@users.noreply.github.com> Co-authored-by: Jared Barocsi <jared.barocsi@sifive.com>
* SRAM API: Add multiple-clocked port API (#3383) This PR adds new APIs for the SRAM module to instantiate a number of ports connected to individual clocks. This allows the creation of memories driven by multiple clocks, for use cases like clock domain crossover. (cherry picked from commit d79ae71) --------- Co-authored-by: Jared Barocsi <82000041+jared-barocsi@users.noreply.github.com> Co-authored-by: Jared Barocsi <jared.barocsi@sifive.com>
It is sometimes desirable to instantiate a memory with ports clocked with different clocks (e.g. clock domain crossing). This PR adds additional APIs to
SRAM
which allow users to drive ports using a sequence of clocks:Contributor Checklist
docs/src
?Type of Improvement
Desired Merge Strategy
Squash
Release Notes
Add new
SRAM
APIs that take threeClock
sequences as parameters instead of the number of read/write/read-write ports. This will sequentially instantiate a memory port for each clock in theClock
sequence and drive them accordingly.Reviewer Checklist (only modified by reviewer)
3.5.x
or3.6.x
depending on impact, API modification or big change:5.0.0
)?Enable auto-merge (squash)
, clean up the commit message, and label withPlease Merge
.Create a merge commit
.