CSR Id registers implemented #6
Conversation
Register 32 does not exist, replaced it by 31
3 CSR registers to be used
There was a problem hiding this comment.
Great work, thank you!
The code looks great, I only have a few minor stylistic remarks regarding comment styles and commits. It's not that big of a deal, but it's nice to keep some guideline consistent to make the project easier to work with in the long run.
Regarding the commits there are multiple schools, I personally like to group by functionality so that features can easily be cherry-picked or removed with a single commit. It is perfectly fine to work with smaller commits, but its nice to group them by feature when merging.
If you are not familiar with interactive rebase, here is how you can do it:
-
Checkout to this branch
git checkout Week1 -
Rebase on main in interactive mode
git rebase -i main
That will open your editor with:pick 2d09a41 Fixed small typo bug pick 08638f7 Added new CSR registers pick c417ff4 Added new CSR register to decoder pick 26d1552 Wrong upper case in csr name pick cbcda0c Virttualization of new CSR registers pick 1957155 [WIP] New tests for csr ids pick 3a53da1 formatting pick 97cecc5 [WIP] test working for one register pick 736dd65 [WIP] testing using variable csr name pick dc57eff tested read-only csr registers pick 90cf4af justfile test now contains csr_id pick 778d7ce formatting -
Choose the commit to sqash (merge with the previous one) with
sand the commits to reword (change the title and description) withr. For instance this split looks good to me:pick 2d09a41 Fixed small typo bug r 08638f7 Added new CSR registers s c417ff4 Added new CSR register to decoder s 26d1552 Wrong upper case in csr name s cbcda0c Virttualization of new CSR registers r 1957155 [WIP] New tests for csr ids s 3a53da1 formatting s 97cecc5 [WIP] test working for one register s 736dd65 [WIP] testing using variable csr name s dc57eff tested read-only csr registers s 90cf4af justfile test now contains csr_id pick 778d7ce formatting -
Then the editor opens multiple time to give you the change to update and merge the descriptions of your commits.
Here is an example of what you can get:pick 2d09a41 Fixed small typo bug pick 55cea29 Added support for mvendorid, marchid, and mimpid pick 7b11399 Add support for testing mvendorid, marchid, and mimpid pick 308ddd8 formatting
To give you an idea of what the final result can look like, I rebased the branch myself, you can look at the result here main...Week1-rebased.
There are fewer commits but they can all be cherry-picked as a cohesive units (whereas on this branch I would need to cherry-pick multiple commits to get a functionality), that also makes it easier to write descriptive comments without repeating one-selves too much.
If you want to try rebasing yourself feel free to do so on this branch (you will need to force-push once done because this changes the history), otherwise I can merge the other branch, as you prefer!
(As a side note, rebasing preserve code authorship, so you are still the author on the branch I rebased)
| out(reg) res | ||
| ); | ||
| }, | ||
| _ => res = 0x42, //TO FAIL BY DEFAULT IF NO VALID CSR REGISTER IS FOUND |
There was a problem hiding this comment.
Minor stylistic details:
The style we use for comments in the project is // My comment, with a space between // and the text, and capitalizing only the first letter.
| @@ -99,6 +105,9 @@ impl RegisterContext<Csr> for VirtContext { | |||
| } | |||
| Csr::Mtvec => self.csr.mtvec = value, | |||
| Csr::Mscratch => self.csr.mscratch = value, | |||
| Csr::Mvendorid => (), //Read-only | |||
There was a problem hiding this comment.
Same remark for the comment, add a space between // and the text.
|
As discussed, closing in favor of #8. Thanks for the work :) |
Implemented and tested the CSR Id registers. And also fixed the bug related to 31 and 31 being confused.