-
Notifications
You must be signed in to change notification settings - Fork 94
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
Few changes before SBI v2 public review #120
Conversation
Just like other RVI specs, the pre-compiled riscv-sbi.pdf should be attached to the release tag instead of having it in spec sources. Signed-off-by: Anup Patel <apatel@ventanamicro.com>
The 'make clean' command should remove the autogenerated directory along with build targets. Signed-off-by: Anup Patel <apatel@ventanamicro.com>
To be consistent with other SBI extensions, let us add flags parameter to sbi_pmu_snapshot_set_shmem() function. Signed-off-by: Anup Patel <apatel@ventanamicro.com>
We update the PMU snapshot documentation to make it consistent with other SBI extensions namely NACL and STA. In the process, we also add mechanism to disable snapshot shared memory (just like other SBI extensions). Signed-off-by: Anup Patel <apatel@ventanamicro.com>
The flags parameter of sbi_steal_time_set_shmem() should be unsigned long for consistency with sbi_nacl_set_shmem(). Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Let us adjust width of CPPC register table columns to make register IDs and names more readable. Signed-off-by: Anup Patel <apatel@ventanamicro.com>
We will be splitting riscv-sbi.adoc into multiple adoc files so the Makefile should have a way to capture dependency. Signed-off-by: Anup Patel <apatel@ventanamicro.com>
riscv-sbi.adoc
Outdated
@@ -1862,8 +1869,9 @@ The possible error codes returned in `sbiret.error` are shown in | |||
[cols="2,3", width=90%, align="center", options="header"] | |||
|=== | |||
| Error code | Description | |||
| SBI_SUCCESS | firmware counter read successfully. | |||
| SBI_ERR_INVALID_PARAM | The `flags` parameter is not zero. | |||
| SBI_SUCCESS | Shared memory was set or cleared successfully. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. Thanks!
Makefile
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about tidying up the root directory by organizing files like the isa manual does
src/*.{adoc,ditaa}
src/images/*.png
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we may now want to provide a diff.orderFile file and instructions on how to use it with git-format-patch/diff/show
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let me add sub-directories for images and sources
e5726ac
to
eb7fadd
Compare
For ease in maintainence, we split the specification into multiple adoc files under a separate src directory. Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Let us move images to a dedicated directory which results in having only text files in root directory. Signed-off-by: Anup Patel <apatel@ventanamicro.com>
eb7fadd
to
84319ee
Compare
The series looks good to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it for easier review while sharing. However, we have a CI which will auto generate the pdf while drafting a release. We can delete this.
riscv-sbi.adoc
Outdated
bitwise then `shmem_phys_lo` specifies the lower XLEN bits and `shmem_phys_hi` | ||
specifies the upper XLEN bits of the snapshot shared memory physical base | ||
address. The `shmem_phys_lo` MUST be 4096 bytes (i.e. page) aligned and | ||
the size of the snapshot shared memory is assumed to be 4096 bytes. The layout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we user a normative text here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks normative from my perspective. If required we can change text as separate patch.
This pull request does few changes and clean-ups before SBI v2 goes to public review.