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

rockchip64-6.14: Add missing opp nodes #8048

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SuperKali
Copy link
Member

@SuperKali SuperKali commented Apr 3, 2025

Description

This PR adds missing Operating Performance Point (OPP) nodes for lower frequencies to the RK3588 device tree. by enabling the CPU clusters to scale down to lower frequencies when under light loads.

The changes add OPP nodes for 408MHz, 600MHz, 816MHz, and 1008MHz (for cluster1 and cluster2 only, as cluster0 already had 1008MHz).

How Has This Been Tested?

  • Tested with RK3588-based board (NanoPC T6-LTS) and verified CPU frequency scaling works correctly

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings

Copy link
Contributor

coderabbitai bot commented Apr 3, 2025

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (1)
  • patch/kernel/archive/rockchip64-6.14/rk3588-0025-add-missing-op-nodes.patch is excluded by !patch/**

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added size/medium PR with more then 50 and less then 250 lines Needs review Seeking for review Hardware Hardware related like kernel, U-Boot, ... Patches Patches related to kernel, U-Boot, ... labels Apr 3, 2025
Copy link
Contributor

@amazingfate amazingfate left a comment

Choose a reason for hiding this comment

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

Is it possible to send it upstream?

@SuperKali
Copy link
Member Author

Is it possible to send it upstream?

Definitely yes, but I don't have the technical knowledge to send it upstream, hopefully someone will explain it to me someday, or else I'll have to figure it out myself.

@SuperKali
Copy link
Member Author

@amazingfate i tried to send it on mainline, definitely I was wrong to put two email addresses but it should be no problem, let's see what happens.

@SuperKali SuperKali added the Work in progress Unfinished / work in progress label Apr 3, 2025
@SuperKali
Copy link
Member Author

WIP: I was informed by mainline developers that frequency scaling alone doesn't provide effective power savings without corresponding voltage reduction. I'm currently researching the RK3588 datasheet to determine the minimum safe voltage levels for each frequency step and will propose a revised patch with properly tested voltage scaling.

@SuperKali
Copy link
Member Author

After carefully reviewing the RK3588 datasheet again, I've found that the typical recommended voltage values are always higher than what I had configured in my patch. Since this PR only implements frequency scaling without proper voltage reduction, it doesn't actually save energy.

image

The datasheet's typical values are there for a reason - they ensure stable operation across all silicon variants and operating conditions. Without proper voltage scaling (which would require extensive testing to validate stable operation at lower voltages), simply adding lower frequency steps doesn't provide meaningful power savings.

Given these findings, I'm closing this PR.

@SuperKali SuperKali closed this Apr 3, 2025
@paolosabatino
Copy link
Contributor

After carefully reviewing the RK3588 datasheet again, I've found that the typical recommended voltage values are always higher than what I had configured in my patch. Since this PR only implements frequency scaling without proper voltage reduction, it doesn't actually save energy.

AFAIK this is not entirely true.

Power consumption has a linear dependency against frequency and quadratic dependency against voltage. The assertion of the mainline developer cited above is not formally correct.

This is the answer from chatgpt to the question: how does the power consumption of a chip depends against frequency and against voltage?
immagine

So voltage has the capacity to decrease the power consumption of a wide amount, but also frequency has its own capacity. What the mainline kernel developer was probably talking about is that lowering the frequency too much when the chip is idle, decreases the power consumption in a trascurable manner: that could be indeed be true.

But there is another variable in the game: thermal throttling! If your chip is thermally throrrling, having a lower speed bin may decrease a lot the power consumption in a thermally constrained condition. For example: keeping the voltage fixed, if your chip at full load consumes (and dissipates) 2 watts at 2ghz, will consume (and dissipate) 1 watt at 1ghz and will consume 0.6watts at 600mhz.

Another thing to consider is frequencies that are too low will reduce the promptness of the processor. Usually 408Mhz speed bin is discarded for this reason, for example.

@SuperKali
Copy link
Member Author

SuperKali commented Apr 6, 2025

@paolosabatino So what would you say to do? :)

To make it easier for you, I leave you the link to the patch posted in the mainline: https://lore.kernel.org/all/[email protected]/T/

Be patient it is my first mainline patch, don't criticize me.

@SuperKali SuperKali reopened this Apr 6, 2025
@SuperKali SuperKali removed the Work in progress Unfinished / work in progress label Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hardware Hardware related like kernel, U-Boot, ... Needs review Seeking for review Patches Patches related to kernel, U-Boot, ... size/medium PR with more then 50 and less then 250 lines
Development

Successfully merging this pull request may close these issues.

3 participants