-
Notifications
You must be signed in to change notification settings - Fork 185
riscv: define mvendorid CSR with macro helpers #255
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
riscv: define mvendorid CSR with macro helpers #255
Conversation
0b6d890 to
37a87f3
Compare
riscv/src/register/mvendorid.rs
Outdated
| /// - `bank`: the number of continuation bytes (`0x7f`) | ||
| /// - `offset`: specific offset in the JEDEC bank number | ||
| pub fn jedec_manufacturer(&self) -> (usize, usize) { | ||
| (self.bank(), (0x80 | self.offset())) |
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.
Instead of 0x80, I think there goes the corresponding odd parity bit of offset. We can either include this logic or ignore the parity bit, as done in the mvendorid register. Another option is completely removing jedec_manufacturer and leaving bank and offset
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.
Instead of 0x80, I think there goes the corresponding odd parity bit of offset.
I'm not sure I understand what you mean here. The 0x80 is the odd parity bit added back to the offset number.
Another option is completely removing
jedec_manufacturerand leavingbankandoffset
This is my preferred route. I think we should leave it up to users on how they want to encode the JEDEC manufacturer number.
I'll add some documentation to the offset field about the parity bit.
Not sure in practice, but the theoretical max number of continuation bytes is 0x1ff_ffff. We obviously can't output such a number ([0x7f; 0x1ff_ffff]) in an embedded environment (not saying you suggested that, just saying). However, maybe some user wants to return an Iterator of continuation bytes 🤷
I'm not so familiar with the use of JEDEC manufacturer numbers, and how they're usually handled in industry.
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.
The
0x80is the odd parity bit added back to theoffsetnumber.
The odd parity bit is 0x80 only when the number of 1s is even (e.g., 0x0a becomes 0x8a). But it should be 0x00 when the number of 1s is already odd (e.g., 0x0b remains 0x0b).
However, maybe some user wants to return an
Iteratorof continuation bytes
It might be useful for someone to implement an iterator that shows the full JEDEC manufacturer number. If you are in the mood to implement it, then go ahead. Otherwise, we could act lazily and wait for someone who asks for this feature.
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.
It might be useful for someone to implement an iterator that shows the full JEDEC manufacturer number. If you are in the mood to implement it, then go ahead. Otherwise, we could act lazily and wait for someone who asks for this feature.
I agree. I've implemented the Iterator solution, however there is a major Git operations outage on GitHub right now. I'll push my changes as soon as I can.
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.
The odd parity bit is 0x80 only when the number of 1s is even (e.g., 0x0a becomes 0x8a). But it should be 0x00 when the number of 1s is already odd (e.g., 0x0b remains 0x0b).
I've corrected my implementation to set the parity bit based on the 1s count parity in offset.
For example, an odd offset with an even amount of 1s should have the parity bit set (0x03 -> 0x83)? This appears correct, since this is the JEDEC ID for Fairchild.
Uses CSR macro helpers to define the `mvendorid` CSR register.
37a87f3 to
22cbf3a
Compare
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.
We can leave it as is or add an iterator for the JEDEC manufacturer number. Whatever you prefer @rmsyn
1a7fe3c to
978f747
Compare
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!
I suggested a small optimization that leaves computing the parity bit when needed. The idea is to reduce the size of the resulting iterator, as it captures one less value.
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.
I suggested some niche optimizations to reduce even more the size of the resulting iterator
riscv/src/register/mvendorid.rs
Outdated
| let mut done = false; | ||
| let mut bank = self.bank(); | ||
| let offset = self.offset(); | ||
| let parity = ((1 - (offset.count_ones() % 2)) << 7) as usize; |
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.
| let mut done = false; | |
| let mut bank = self.bank(); | |
| let offset = self.offset(); | |
| let parity = ((1 - (offset.count_ones() % 2)) << 7) as usize; | |
| let mut bank = self.bank() + 2; // add 2 for Some(offset) + None | |
| let offset = self.offset(); |
riscv/src/register/mvendorid.rs
Outdated
| 0 if done => None, | ||
| 0 => { | ||
| done = true; |
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.
| 0 if done => None, | |
| 0 => { | |
| done = true; | |
| 0 => None, | |
| 1 => { | |
| bank -= 1; | |
| let parity = ((1 - (offset.count_ones() % 2)) << 7) as usize; |
Adds a corrected `Mvendorid::jedec_manufacturer` implementation to return the decoded JEDEC manufacturer ID.
Adds basic unit tests for the `mvendorid` CSR.
978f747 to
4a0bab7
Compare
Uses CSR macro helpers to define the
mvendoridCSR register.Related: #229