-
Notifications
You must be signed in to change notification settings - Fork 13
Add DMA data size to DMA read and write control interfaces #23
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.
In effect, this looks great. I have some implementation suggestions on how to do this a bit more safely.
Do you know if there's anything in the simulated DMA that needs to be updated? I don't think it does, but I'm no expert on how that works.
val Size8Word = 5.U; | ||
val Size16Word = 6.U; | ||
val Size32Word = 7.U; | ||
} |
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.
A couple of minor improvements to this. Chisel has an Enum
object which is a method for constructing sequences of UInt
s. It's janky and not type-safe, but it makes what this PR is doing more programmatic. (There's a "strongly typed enum" which will be added in Chisel 3.2, but that isn't released yet and I don't want to point this repo at snapshot version) I'd also prefer to have the width of the size field not hard-coded to 3
.
How about something like the following:
import chisel3.util.Enum // Add this up with the other imports
object DmaSize {
private val enums = Enum(8)
val Seq(bytes, wordHalf, word, wordDouble, wordQuad, word8, word16, word32) = Enum(8)
def gen: UInt = enums.head.chiselCloneType
}
I change the names here for two reasons: (1) bikeshedding from the repetition of size and (2) you can't use uppercase to construct val
s in that way (val Seq(Foo, Bar) = Seq(1, 2)
will error for reasons that I never investigated), and (3) the convention in Scala is that uppercase means "class, object, or trait" while lowercase means "package, member, or function". (2) and (3) matter, but (1) doesn't. Use whatever makes sense to you.
Signed-off-by: Paolo Mantovani <paolo@cs.columbia.edu> Co-Authored-By: Schuyler Eldridge <schuyler.eldridge@gmail.com>
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!
One final change... I screwed up the suggestion and used Enum(8)
twice.
Changes applied: now using enum type and the appropriate Scala naming convention. |
Use predefined alias for Enum(8) Co-Authored-By: Schuyler Eldridge <schuyler.eldridge@gmail.com>
I tested the code on my end. It seems working fine, but the checks get cancelled before completion. |
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!
No clue why Travis cancelled... I manually restarted it. |
Signed-off-by: Paolo Mantovani paolo@cs.columbia.edu