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

Update CPPC service group #46

Merged
merged 1 commit into from
Sep 5, 2024
Merged

Conversation

pathakraul
Copy link
Collaborator

No description provided.

the power/performance management software running on the application processor
and the platform to coordinate and manage the group level performance changes.

==== CPPC Fast Channel
Copy link
Collaborator

Choose a reason for hiding this comment

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

In Performance channel, it uses "Fast-Channel" in title, or use "Fast-channel" in the description as a special term.
So, we need to standardize this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Lets use Fast-channel or fast-channel a needed. Not the Fast-Channel

This service group defines the services to control application processor
performance by managing a set of registers per application processor
that are used for performance management and control. The ACPI CPPC
(Collaborative processor performance control) is an abstract and flexible
Copy link
Collaborator

Choose a reason for hiding this comment

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

Collaborative Processor Performance Control

microcontroller to control the performance.

The CPPC extension defined in the RISC-V SBI specification cite:[SBI] defines
the register ids for the standard CPPC registers, together with additional
Copy link
Collaborator

Choose a reason for hiding this comment

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

IDs


The CPPC extension defined in the RISC-V SBI specification cite:[SBI] defines
the register ids for the standard CPPC registers, together with additional
registers also that are also by application processor.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This "also that are also by application processor" sentence sounds like a grammar problem.


For extra debugging, Hart can send a GET_CPPC_PERF_POKE message to exclusively request PuC to process the pending CPPC requests, if any.
| `CPPC2`(Autonomous) Mode
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggest adding a space before '(', same for the rest of this chapter.


PROBE_CPPC_REG service can be used to check if a particular CPPC register is implemented. The service READ_CPPC_REG can be used to read all implemented CPPC register values and WRITE_CPPC_REG can be used to write the registers.
! 0x0
! Desired Performance Level
Copy link
Collaborator

Choose a reason for hiding this comment

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

Desired performance level

The offset can be added into the `base-address` of the shared memory region to
form the address of a application processor fast channel.

===== Fast Channel Doorbell
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need doorbell for fast channel?
Fast channel in the Performance service group doesn't support doorbell. Firmware can update data to the shared memory when has new update. Then AP can read the registers from shared memory anytime (read the last updated value).
Not sure if CPPC use case can use the same method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A interrupt mechanism with a transport completes the design even if not implemented.

I can see Performance service group has doorbell support that is also per domain

| 0x05 | CPPC_GET_FAST_CHANNEL_ATTRIBUTES | NORMAL_REQUEST
| 0x06 | CPPC_POKE_FAST_CHANNEL | NORMAL_REQUEST
| 0x07 | CPPC_GET_HART_LIST | NORMAL_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.

Should move this Services table to before CPPC fast channel sub-chapter. It is part of CPPC fast channel sub-chapter now.
Some other service groups also have similar issue. I will update them.

all the application processors. If a doorbell is available then it must be
supported through a read-modify-write sequence to a memory-mapped register.
The doorbell details and attributes can be discovered by the application processor
through the defined service in this service group.

Below table lists the services in this group:
Copy link
Collaborator

Choose a reason for hiding this comment

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

change to "this service group"

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is better:
"The following table lists the services in the CPPC service group:"

! Description

! RPMI_SUCCESS
! Notifications are subscribed successfully.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This service subscribes to a single EVENT_ID, so it should be a single notification.
So, change to "Notification is subscribed successfully."

I already changed this in CLOCK to RAS service groups, other service groups need to update.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it has to be "Event is subscribed successfully." since subscription is happening for each event

! `EVENT_ID` is invalid.

! RPMI_ERR_NOT_SUPPORTED
! Notifications 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, change to "Notification is not supported."

This service is used to probe a CPPC register implementation status for a
application processor. If the CPPC register `reg_id` is implemented then
the length in bits is returned in `REG_LENGTH` field. If the register is not
supported or invalid then the `REG_LENGTH` will be `0`
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing full stop.

| 1
| REG_LENGTH
| uint32
| Register length(bits)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add space before (

| DATA_HIGH
| uint32
| Upper `32 bits` of data. This will be `0` if the register is of `32-bit`
length
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing full stop

| 3
| DATA_HIGH
| uint32
| Upper `32 bits` of data. This is ignored is the register is of `32-bit` length.
Copy link
Collaborator

Choose a reason for hiding this comment

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

check grammar

| 2
| REGION_ADDR_LOW
| uint32
| Lower `32-bit` of the fast channels shared memory region physical address.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change to 32 bit (no "-"), consistent with others. Same for the rest below.

| 1
| OFFSET_LOW
| uint32
| Lower `32-bit` of a fast channel offset.
Copy link
Collaborator

Choose a reason for hiding this comment

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

change to 32 bit, same for upper offset below.

| 0
| STATUS
| int32
| Return Status Code
Copy link
Collaborator

Choose a reason for hiding this comment

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

Return error code

| 0
| STATUS
| int32
| Return Status Code
Copy link
Collaborator

Choose a reason for hiding this comment

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

Return error code

| 0
| START_INDEX
| uint32
| Starting index of Hart ID
Copy link
Collaborator

@lftan lftan Sep 5, 2024

Choose a reason for hiding this comment

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

Choose to use "Hart ID" or "hart ID" for consistency.

The description below some uses "hart ID". Check the rest of chapter/document.

Signed-off-by: Rahul Pathak <[email protected]>
| 1
| DATA_LOW
| uint32
| Lower `32-bit` of the data.
Copy link
Collaborator

Choose a reason for hiding this comment

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

32 bit

Other service groups use "32 bit", no dash.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I dont think so , I have been using this only places have dash


[#table_cppc_pokefastchan_request_data]
0b00: `8-bit`
Copy link
Collaborator

Choose a reason for hiding this comment

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

8 bit

@lftan lftan merged commit f7cdbdd into riscv-non-isa:main Sep 5, 2024
1 check passed
@pathakraul pathakraul deleted the rpathak_cppc branch November 9, 2024 14:46
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