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

Move scattered CSRs #592

Merged
merged 17 commits into from
Nov 12, 2024
Merged

Move scattered CSRs #592

merged 17 commits into from
Nov 12, 2024

Conversation

jordancarlin
Copy link
Contributor

@jordancarlin jordancarlin commented Oct 14, 2024

This is an initial attempt to relocate the newly scattered CSR functions and mappings.

I attempted to place everything related to a particular CSR together and in a logical place, but the compilation order of some of the files made this tricky. Especially SAIL_SYS_SRCS vs SAIL_REG_SRCS. The sail_sys_control.sail file could probably do with a total restructuring.

This is structured as a series of commits that each move logically grouped CSRs to make it easier to review.

For many of the CSRs, I had to make a judgment call about what seemed to make the most sense, but there are multiple good locations for many of them and we'll probably want to move several of them around before everyone is happy with this.

Copy link

github-actions bot commented Oct 14, 2024

Test Results

396 tests  ±0   396 ✅ ±0   0s ⏱️ ±0s
  4 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 472456e. ± Comparison against base commit 0b9c639.

♻️ This comment has been updated with latest results.

@Timmmm
Copy link
Collaborator

Timmmm commented Oct 15, 2024

Excellent work! I'll take a look when we've excised N.

@Timmmm
Copy link
Collaborator

Timmmm commented Oct 16, 2024

Can you fix the conflicts (presumably due to N?) and then I'll take a look.

@jordancarlin
Copy link
Contributor Author

Can you fix the conflicts (presumably due to N?) and then I'll take a look.

Fixed. Was from N and the addition of menvcfgh.

@Timmmm
Copy link
Collaborator

Timmmm commented Oct 16, 2024

This LGTM. There's definitely still organisational work to do after this (lots of files can be split up more), but we should probably defer that until after the new module system is in place.

The only change I would suggest is very minor: there are a few places where it would improve readability to add blank lines e.g. change:

/* machine information registers */
register mvendorid : bits(32)
register mimpid : xlenbits
register marchid : xlenbits
/* TODO: this should be readonly, and always 0 for now */
register mhartid : xlenbits
register mconfigptr : xlenbits
mapping clause csr_name_map = 0xF11  <-> "mvendorid"
mapping clause csr_name_map = 0xF12  <-> "marchid"
mapping clause csr_name_map = 0xF13  <-> "mimpid"
mapping clause csr_name_map = 0xF14  <-> "mhartid"
mapping clause csr_name_map = 0xF15  <-> "mconfigptr"
function clause is_CSR_defined(0xf11) = true // mvendorid
function clause is_CSR_defined(0xf12) = true // marchdid
function clause is_CSR_defined(0xf13) = true // mimpid
function clause is_CSR_defined(0xf14) = true // mhartid
function clause is_CSR_defined(0xf15) = true // mconfigptr
function clause read_CSR(0xF11) = zero_extend(mvendorid)
function clause read_CSR(0xF12) = marchid
function clause read_CSR(0xF13) = mimpid
function clause read_CSR(0xF14) = mhartid
function clause read_CSR(0xF15) = mconfigptr

to

/* machine information registers */
register mvendorid : bits(32)
register mimpid : xlenbits
register marchid : xlenbits
/* TODO: this should be readonly, and always 0 for now */
register mhartid : xlenbits
register mconfigptr : xlenbits

mapping clause csr_name_map = 0xF11  <-> "mvendorid"
mapping clause csr_name_map = 0xF12  <-> "marchid"
mapping clause csr_name_map = 0xF13  <-> "mimpid"
mapping clause csr_name_map = 0xF14  <-> "mhartid"
mapping clause csr_name_map = 0xF15  <-> "mconfigptr"

function clause is_CSR_defined(0xf11) = true // mvendorid
function clause is_CSR_defined(0xf12) = true // marchdid
function clause is_CSR_defined(0xf13) = true // mimpid
function clause is_CSR_defined(0xf14) = true // mhartid
function clause is_CSR_defined(0xf15) = true // mconfigptr

function clause read_CSR(0xF11) = zero_extend(mvendorid)
function clause read_CSR(0xF12) = marchid
function clause read_CSR(0xF13) = mimpid
function clause read_CSR(0xF14) = mhartid
function clause read_CSR(0xF15) = mconfigptr

@Timmmm Timmmm requested a review from Alasdair October 16, 2024 12:32
@jordancarlin jordancarlin marked this pull request as ready for review October 16, 2024 21:44
@jordancarlin
Copy link
Contributor Author

The only change I would suggest is very minor: there are a few places where it would improve readability to add blank lines e.g. change:

Whitespace updated

@Timmmm
Copy link
Collaborator

Timmmm commented Oct 31, 2024

LGTM; can't see any reason not to merge this - I'll do it in a few days.

@Timmmm
Copy link
Collaborator

Timmmm commented Nov 12, 2024

I had a look through git diff --color-moved master and it all looks good to me.

@Timmmm Timmmm merged commit 1a3d8eb into riscv:master Nov 12, 2024
2 checks passed
@Timmmm
Copy link
Collaborator

Timmmm commented Nov 12, 2024

Merged without squashing since the commits are so neat. It will probably help git blame keep track of things too.

@jordancarlin jordancarlin deleted the move_scattered_csrs branch November 15, 2024 17:33
@jordancarlin
Copy link
Contributor Author

Merged without squashing since the commits are so neat. It will probably help git blame keep track of things too.

That was the idea. I figured it was much easier to keep track of that way.

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