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

CRC Generation Module #54

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open

CRC Generation Module #54

wants to merge 32 commits into from

Conversation

ronitnag04
Copy link
Collaborator

No description provided.

val crc1 = RegInit(Bits(8.W), 0.U)

// Output signal registers
val message_ready = RegInit(Bool(), true.B)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: camelcase?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I was told to use snake case by @rahulk29, but I can switch it to camelcase if that is recommended.

* and crc valid state low. CRC bits will be cleared to 0.
* @group Signals
*/
val reset = Input(Bool())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use implicit reset instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would I just be able to omit the reset declaration in the IO. I also changed it so that when a new message is latched, the crc output is cleared and set invalid, let me know if this is correct though.

src/main/scala/d2dadapter/CRCGenerator.scala Outdated Show resolved Hide resolved
Copy link
Collaborator

@ethanwu10 ethanwu10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of architecture, we will probably need something that can run faster than one byte per cycle in order to keep up with the link's speed, and also it might be good to eventually have some sort of streaming capability so that we can feed the module bytes as soon as they are available, instead of collecting the entire word and summing it at once. This is fine for now though and we should probably wait until we better understand how the module will get used to determine what architecture changes to make.

src/main/scala/d2dadapter/CRC16Lookup8Bit.scala Outdated Show resolved Hide resolved
src/main/scala/d2dadapter/CRC16Lookup8Bit.scala Outdated Show resolved Hide resolved
src/main/scala/d2dadapter/CRCGenerator.scala Outdated Show resolved Hide resolved
src/main/scala/d2dadapter/CRCGenerator.scala Outdated Show resolved Hide resolved
src/main/scala/d2dadapter/CRCGenerator.scala Outdated Show resolved Hide resolved
src/main/scala/d2dadapter/CRCGenerator.scala Outdated Show resolved Hide resolved
src/main/scala/d2dadapter/CRCGenerator.scala Outdated Show resolved Hide resolved
src/main/scala/d2dadapter/CRCGenerator.scala Outdated Show resolved Hide resolved
src/test/scala/d2dadapter/CRCGenerator.scala Outdated Show resolved Hide resolved
Comment on lines 77 to 80
crc1 := crc0 ^ lookup
.table(crc1 ^ message_bits(width - 1, width - 8))(15, 8)
crc0 := lookup.table(crc1 ^ message_bits(width - 1, width - 8))(7, 0)
message_bits := message_bits << 8
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this endianness correct? This implementation treats message as one big-endian integer and CRC's bytes from most-significant-byte to LSB (as documented); not sure if this is what we want though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's a little unconventional compared to what I normally see for CRC calculations, but the results of this CRC generator match the results of the design the UCIe1.0 spec has in the appendix (where they just selectively xor the 1024 bits of the message for each of the 16 crc bits). I ran a simulation of the xor'ing method in python to generate the test cases I listed in the test module.

ethanwu10 and others added 2 commits November 2, 2023 14:48
Since hardware types can only be instantiated inside of a module, the
`table` member is made into a parentheses-less method so that the
`VecInit` is constructed at the use site.
@ronitnag04 ronitnag04 linked an issue Nov 3, 2023 that may be closed by this pull request
3 tasks
@ronitnag04 ronitnag04 removed a link to an issue Nov 3, 2023
3 tasks
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