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

Few changes before SBI v2 public review #120

Merged
merged 9 commits into from
Sep 6, 2023

Conversation

avpatel
Copy link
Contributor

@avpatel avpatel commented Sep 5, 2023

This pull request does few changes and clean-ups before SBI v2 goes to public review.

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops. Thanks!

Makefile Outdated
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor Author

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

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>
@jones-drew
Copy link
Contributor

The series looks good to me.

Copy link
Collaborator

@atishp04 atishp04 left a 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
Copy link
Collaborator

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 ?

Copy link
Contributor Author

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.

@atishp04 atishp04 merged commit d1e5457 into riscv-non-isa:master Sep 6, 2023
@avpatel avpatel deleted the sbi_v2_fixes branch September 6, 2023 07:12
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