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

[rv_plic] Adjust naming of PRIO regs by using multireg #25701

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

a-will
Copy link
Contributor

@a-will a-will commented Dec 18, 2024

Now that we have non-compact multireg features, use them to eliminate the explicit numbering of CSRs from the template and improve Rust code generation.

Now that we have non-compact multireg features, use them to eliminate
the explicit numbering of CSRs from the template and improve Rust code
generation.

Signed-off-by: Alexander Williams <awill@opentitan.org>
@a-will a-will force-pushed the rv-plic-compact-csrs branch from ca4010e to 5769a84 Compare December 18, 2024 23:15
@a-will a-will marked this pull request as ready for review December 19, 2024 06:32
@a-will a-will requested a review from a team as a code owner December 19, 2024 06:32
@a-will a-will requested review from jadephilipoom, Razer6 and andreaskurth and removed request for a team December 19, 2024 06:32
Copy link
Contributor

@jadephilipoom jadephilipoom left a comment

Choose a reason for hiding this comment

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

Whew! Can't promise I read every line of the diff closely, but I at least opened all the files, read the first occurrence of each code-change pattern, and checked that the following lines looked like 160/185 repetitions of the same pattern. Thanks, Alex!

rv_plic_reg2hw_prio183_reg_t prio183; // [206:205]
rv_plic_reg2hw_prio184_reg_t prio184; // [204:203]
rv_plic_reg2hw_prio185_reg_t prio185; // [202:201]
rv_plic_reg2hw_prio_mreg_t [185:0] prio; // [572:201]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here about the length and bit-length.

rv_plic_reg2hw_prio157_reg_t prio157; // [180:179]
rv_plic_reg2hw_prio158_reg_t prio158; // [178:177]
rv_plic_reg2hw_prio159_reg_t prio159; // [176:175]
rv_plic_reg2hw_prio_mreg_t [159:0] prio; // [494:175]
Copy link
Contributor

Choose a reason for hiding this comment

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

At first glance, it looks to me like the [159:0] and the // [494:201] here disagree, since the comment indicates a 320-bit range. Is this maybe an array of two-bit multireg entries that has length 160 but bit-length 320? But the multireg entry below has [159:0] and // [174:15], which match exactly with length and bit-length 160. I'm not an RTL expert so I'm probably missing something here, but wanted to double-check 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's exactly right. This is a definition of an array of rv_plic_reg2hw_prio_mreg_t, which is defined above as a struct with a single 2-bit member (with the member named q).

@a-will
Copy link
Contributor Author

a-will commented Dec 19, 2024

From a software perspective, the interesting bit here is how the output of //hw/top_earlgrey:rv_plic_rust_regs changes (and same for darjeeling).

With the multireg construction, reggen will generate an array for the Rust struct instead of the flattened variant. Without this, each priority register gets a named member in the IP's register definitions, which is just a little less nice to work with. (since Rust provides iterators and other helpful constructions if the type is a sized aggregate -- in C, we'd just use pointer arithmetic, heh)

I noticed the incongruity with the other CSRs as I was fiddling with topgen for other reasons and figured, "Why not make this little tweak?" ;)

Copy link
Member

@Razer6 Razer6 left a comment

Choose a reason for hiding this comment

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

This looks good to me. Less technical debt. Always good!

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