Skip to content

Conversation

@ronitnag04
Copy link

No description provided.

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

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

Choose a reason for hiding this comment

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

nit: camelcase?

Copy link
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
Contributor

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
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.

Copy link
Contributor

@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.

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
Contributor

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
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.

4 participants