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

riscv: define mvendorid CSR with macro helpers #255

Merged
merged 3 commits into from
Jan 15, 2025

Conversation

rmsyn
Copy link
Contributor

@rmsyn rmsyn commented Jan 11, 2025

Uses CSR macro helpers to define the mvendorid CSR register.

Related: #229

@rmsyn rmsyn requested a review from a team as a code owner January 11, 2025 10:39
@rmsyn rmsyn force-pushed the riscv/mvendorid-csr-macro branch 2 times, most recently from 0b6d890 to 37a87f3 Compare January 11, 2025 17:38
/// - `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()))
Copy link
Contributor

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

Copy link
Contributor Author

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_manufacturer and leaving bank and offset

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

The 0x80 is the odd parity bit added back to the offset number.

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

@rmsyn rmsyn Jan 14, 2025

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.
@rmsyn rmsyn force-pushed the riscv/mvendorid-csr-macro branch from 37a87f3 to 22cbf3a Compare January 13, 2025 13:00
romancardenas
romancardenas previously approved these changes Jan 13, 2025
Copy link
Contributor

@romancardenas romancardenas left a 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

Copy link
Contributor

@romancardenas romancardenas left a 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.

romancardenas
romancardenas previously approved these changes Jan 14, 2025
Copy link
Contributor

@romancardenas romancardenas left a 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

Comment on lines 40 to 43
let mut done = false;
let mut bank = self.bank();
let offset = self.offset();
let parity = ((1 - (offset.count_ones() % 2)) << 7) as usize;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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();

Comment on lines 46 to 48
0 if done => None,
0 => {
done = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
0 if done => None,
0 => {
done = true;
0 => None,
1 => {
bank -= 1;
let parity = ((1 - (offset.count_ones() % 2)) << 7) as usize;

rmsyn added 2 commits January 15, 2025 13:21
Adds a corrected `Mvendorid::jedec_manufacturer` implementation to
return the decoded JEDEC manufacturer ID.
Adds basic unit tests for the `mvendorid` CSR.
@romancardenas romancardenas added this pull request to the merge queue Jan 15, 2025
Merged via the queue into rust-embedded:master with commit b391a48 Jan 15, 2025
119 checks passed
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.

2 participants