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

1. Improvements in Base, System Reset and System Suspend service groups #38

Merged

Conversation

pathakraul
Copy link
Collaborator

No description provided.

@pathakraul pathakraul requested a review from lftan August 8, 2024 14:42
| 0
| SUSPEND_TYPE
| uint32
| Suspend type
Copy link
Collaborator

Choose a reason for hiding this comment

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

either link to line 32 or to Sleep types definitions in SBI spc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

or link to Line 21 (not line 32) in this adoc. Can create a sub-section then reference link to it

| 0 | SUSPEND_TYPE | uint32 | Suspend type value. +
Refer to https://github.com/riscv-non-isa/riscv-sbi-doc/blob/master/src/ext-sys-suspend.adoc#table_susp_sleep_types[*SUSP System Sleep Types*^]
in the RISC-V SBI Specification cite:[SBI] for the SUSPEND_TYPE definition.
| Word
Copy link
Collaborator

Choose a reason for hiding this comment

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

in the new pdf, font size of italic text is look bigger than other text. This can be changed?
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are relying on the format theme coming from the docs_resource submodule.

I think your repo has not the updated docs_resources folder. This is how it currently looks
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

font size looks okay after update doc-resources

| 1
| SUSPEND_TYPE
| uint32
| Suspend type.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, reference link to 'Suspend type'

! Error Code
! Description

! RPMI_ERROR_NOT_SUPPORTED
Copy link
Collaborator

Choose a reason for hiding this comment

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

other services has RPMI_SUCCESS for success, need have this error code for all services?

| 0
| RESET_TYPE
| uint32
| Reset type
Copy link
Collaborator

Choose a reason for hiding this comment

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

add the reference link to line 30

! RPMI_ERROR_NOT_FOUND
! EVENT_ID is not supported or invalid.

! RPMI_ERROR_NOT_SUPPORTED
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adjust the table column width to fix the whole text in one line.

In SYSSUSP_ENABLE_NOTIFICATION table:
image

! Error Code
! Description

! RPMI_ERROR_NOT_SUPPORTED
Copy link
Collaborator

Choose a reason for hiding this comment

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

here also, make it one line.

image

! RPMI_ERROR_NOT_FOUND
! `EVENT_ID` is not supported or invalid.

! RPMI_ERROR_NOT_SUPPORTED
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here, make it display in one line.

@pathakraul pathakraul force-pushed the rpathak_sysreset_syssuspend branch from 9aaa1b5 to b2a1c84 Compare August 12, 2024 16:40
@pathakraul
Copy link
Collaborator Author

Addressed all the comments, hopefully without missing any.
Also pushed another commit in this PR for a incorrect statement in message protocol chapter

@pathakraul pathakraul changed the title Improvements in System Reset and System Suspend service groups Improvements in Base, System Reset and System Suspend service groups Aug 12, 2024
@pathakraul
Copy link
Collaborator Author

I pushed commit for Base service group also

been complete.

==== Suspend Types
RPMI supports suspend states and their values as defined by SBI
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use 'suspend types' instead of suspend states here?

| 0x01 | SYSSUSP_ENABLE_NOTIFICATION | NORMAL_REQUEST
| 0x02 | SYSSUSP_GET_ATTRIBUTES | NORMAL_REQUEST
| 0x03 | SYSSUSP_SUSPEND | NORMAL_REQUEST
| Service ID
Copy link
Collaborator

@lftan lftan Aug 13, 2024

Choose a reason for hiding this comment

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

This table should move to before the "Suspend Types" sub-section. It is under chapter "4.3.1. Suspend Types" now.

| 0x01 | SYSRST_ENABLE_NOTIFICATION | NORMAL_REQUEST
| 0x02 | SYSRST_GET_ATTRIBUTES | NORMAL_REQUEST
| 0x03 | SYSRST_RESET | POSTED_REQUEST
| Service ID
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here, move to before subsection of "Reset Types"

| RESET_TYPE
| uint32
| Reset type +
Refer <<Reset Types>> section for supported reset types.
Copy link
Collaborator

Choose a reason for hiding this comment

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

can remove 'section'here, generated pdf has Section in front.
eg:
Refer Section 4.2.1 section for supported reset types.

| RESET_TYPE
| uint32
| Reset type +
Refer <<Reset Types>> section for supported reset types.
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

A service group is a collection of services which are logically grouped
according to the functionality they provide. For example, all voltage related
services/messages are collectively referred to as the Voltage Service Group.
=== Service Group: BASE (servicegroup_id: 0x0001)
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the Message Format chapter, we use "SERVICEGROUP_ID", should standardize this?
Same for "SERVICE_ID" in each sub-chapter header.

| Description

| 0x001
| REQUEST_HANDLE_ERROR
Copy link
Collaborator

Choose a reason for hiding this comment

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

adjust table width, so that fix whole word in one line

==== Service: BASE_GET_IMPLEMENTATION_ID (service_id: 0x03)
This service is used to get a `32-bit` RPMI implementation ID assigned to the
software which implements the RPMI specification. Every implementation ID is
unique and listed in the table <<table_base_rpmi_impl_id>>
Copy link
Collaborator

Choose a reason for hiding this comment

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

It display like this, fix it.

image

| Platform Identifier field length in bytes.

| 2
| PLATFORM_ID
Copy link
Collaborator

Choose a reason for hiding this comment

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

make them display in one line
image

0: Service group not implemented by platform.
1: Service group implemented by platform.
| 1
| SERVICE_GROUP_STATUS
Copy link
Collaborator

Choose a reason for hiding this comment

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

make this display in one line

@pathakraul pathakraul force-pushed the rpathak_sysreset_syssuspend branch from e72eb72 to a568ffe Compare August 21, 2024 17:39
@pathakraul pathakraul changed the title Improvements in Base, System Reset and System Suspend service groups 1. Improvements in Base, System Reset and System Suspend service groups Aug 30, 2024
@pathakraul
Copy link
Collaborator Author

Merging this since other PRs are dependent on this, we have more review stages coming in

@pathakraul pathakraul merged commit 3326ba6 into riscv-non-isa:main Sep 2, 2024
1 check passed
@pathakraul pathakraul deleted the rpathak_sysreset_syssuspend branch November 9, 2024 14:47
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.

2 participants