cva6
cva6 copied to clipboard
[DO NOT MERGE] docs: Review of the current cv32a60x documentation
DO NOT MERGE!
Hi @ASintzoff and @MikeOpenHWGroup ,
I analysed the docs under https://cva6-cv32a60x.readthedocs.io/en/latest/07_cv32a60x/, the parts that are specific to the CV32A60X, I would like to point out some questions and suggestions:
- Line 1345 of machine.adoc , on section "3.1.6.7. Extension Context Status in
mstatusRegister", the paragraph "[CV32A60X] When an extension’s status is set to Off, any instruction that attempts to read or write the corresponding state will cause an illegal-instruction exception.". It starts with the "[CV32A60X]" ([{ohg-config}]), but I don't see this paragraph specific to the [CV32A60X]. - Line 1649 of machine.adoc: Mode => MODE, as this is what is used on other parts of the text and on Figure 9.
- Many times an instruction is listed with a `` syntax, others they are formatted as plain text. I am setting them as plain text (without ``). Feel free to completely ignore this though.
- In section 3.1.10. Hardware Performance Monitor, on the part "[CV32A60X] The hardware performance monitor includes 29 additional 64-bit event counters". I see the standard amount of Performance counters is 6, as in
/core/csr_regfile.svline 589:localparam int unsigned MHPMCounterNum = 6;. Should it be mentioned here that the amount of performance counters is configurable, with a maximum amount of 29? - On section "3.1.19. Machine Security Configuration (mseccfg) Register", it is written that "As XLEN=32, mseccfgh is a 32-bit read/write register that aliases bits 63:32 of mseccfg". At the same time, the next paragraph affirms that "[CV32A60X] As Zkr, Smepmp, and Smmpm extensions are not implemented, mseccfg and mseccfgh do not exist.". Only the last one should exist in my opinion.
Thanks for the review.
A comment to explain how the RISC-V ISA manual tailored for CV32A60X is generated. The source files from riscv-isa-manual to be tailored are copied and modified as little as possible. For instance, when I discovered errors I created pull requests on that repository. Therefore, for instance, we do not change how backticks are used or change word capitalization.
3.1.6.7: you are right that the paragraph is not specific to CV32A60X but IMO, as the documentation is only for this configuration, it is not a real issue.
3.1.10: to be compliant with RISC-V ISA, the mphcounter[i] (3<=i<=31) must exist. In CV32A60X, there is no usable performance counter so they are always equal to zero. This is not configurable.
3.1.19: good catch, only the last paragraph should be kept to avoid any confusion.
👋 Hi there!
This pull request seems inactive. Need more help or have updates? Feel free to let us know. If there are no updates within the next few days, we'll go ahead and close this PR. 😊
Hi @cairo-caplan, I am sorry that I have overlooked this PR. Is it still actual? Also, I have moved the review to @ASintzoff who looks more relevant.
Hi @jquevremont, no worries.
Yes, I just checked and can confirm that my last comments would still be relevant to be checked by @ASintzoff.
Thank you, @ASintzoff and @jquevremont.
👋 Hi there!
This pull request seems inactive. Need more help or have updates? Feel free to let us know. If there are no updates within the next few days, we'll go ahead and close this PR. 😊