Skip to content

Conversation

@Lemon1151
Copy link
Contributor

@Lemon1151 Lemon1151 commented Dec 7, 2025

-cooling-levels = <0 30 60 90 120 160>;
+cooling-levels = <0 50 100 150 200 255>;

-temperature = <50000>;
+temperature = <55000>;

Summary by CodeRabbit

  • Improvements
    • Adjusted fan cooling levels with updated speed thresholds
    • Modified thermal temperature regulation threshold

✏️ Tip: You can customize this high-level summary in your review settings.

-cooling-levels = <0 30 60 90 120 160>;
+cooling-levels = <0 50 100 150 200 255>;

-temperature = <50000>;
+temperature = <55000>;
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 7, 2025

Walkthrough

Device tree configuration update for the Rockchip RK3399-based fine3399 board, adjusting thermal management parameters. Fan speed levels increased and CPU warm temperature threshold raised.

Changes

Cohort / File(s) Summary
Thermal configuration tuning
patch/kernel/archive/rockchip64-6.12/dt/rk3399-fine3399.dts
Updated fan0 cooling levels from <0 30 60 90 120 160> to <0 50 100 150 200 255> and increased cpu_warm threshold from 50000 to 55000 in cpu_thermal node.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

  • Minimal change scope affecting a single configuration file
  • Simple parameter adjustments with clear numeric modifications
  • Thermal strategy adjustments that may benefit from validation testing on hardware

Suggested reviewers

  • teknoid
  • leggewie
  • krachlatte
  • pyavitz
  • JohnTheCoolingFan

Poem

🐰 Fans now spin with vigor true,
Cooling curves adjusted anew,
Thermal thresholds rise on high,
Keeping temperatures cool and dry,
Fine3399 breathes easier... phew!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'dts: rockchip: rk3399-fine3399: adjust pwm-fan control strategy' directly and specifically describes the main change: adjusting PWM fan control parameters for this specific board in the device tree.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between f9cf9ae and 9e00503.

📒 Files selected for processing (1)
  • patch/kernel/archive/rockchip64-6.12/dt/rk3399-fine3399.dts (2 hunks)
🔇 Additional comments (2)
patch/kernel/archive/rockchip64-6.12/dt/rk3399-fine3399.dts (2)

131-131: LGTM! Improved thermal headroom.

The new cooling levels extend to full PWM duty cycle (255) and provide more aggressive cooling at higher states, which should improve thermal management under sustained load.


201-201: Review comment is factually incorrect.

The shell output from the actual file shows cpu_warm temperature is 50000 (50°C), not 55000 (55°C). Additionally, gpu_warm is also 50000 (50°C), meaning the thresholds are already symmetric—there is no asymmetry to address. The review comment appears to be based on incorrect information about the file state.

Likely an incorrect or invalid review comment.


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

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot added size/small PR with less then 50 lines 02 Milestone: First quarter release Needs review Seeking for review Hardware Hardware related like kernel, U-Boot, ... Patches Patches related to kernel, U-Boot, ... labels Dec 7, 2025
@tabrisnet
Copy link
Collaborator

This feels like something that is quite open to opinion/circumstance. is this just a set of defaults able to be overridden in sysfs [or some other mechanism], or is this the only way to control the setpoints?
@coderabbitai

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 8, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, let's examine the actual device tree file to see the full context
fd -t f "rk3399-fine3399.dts" --exec cat {}

Length of output: 48904


🏁 Script executed:

#!/bin/bash
# Search for documentation about PWM fan control and thermal management
rg -n -C5 "pwm.*fan|cooling-levels" --type=txt --type=md --type=rst -g '!**/test/**' | head -100

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Look for other examples of PWM fan configurations in rockchip device trees
fd -e dts -e dtsi . patch/kernel | xargs rg -n -C3 "cooling-levels" | head -80

Length of output: 6067


🏁 Script executed:

#!/bin/bash
# Search for pwm-fan driver code and sysfs interfaces
rg -n -C5 "pwm.fan|cooling.level" --type=c -g "drivers/hwmon/*" -g "drivers/thermal/*" | head -150

Length of output: 148


🏁 Script executed:

#!/bin/bash
# Look for documentation about pwm-fan and thermal cooling
fd -t f "pwm-fan" | xargs cat 2>/dev/null | head -200

Length of output: 6006


🏁 Script executed:

#!/bin/bash
# Search for sysfs thermal cooling device attributes
rg -n "cur_state|max_state" --type=c -g "drivers/thermal/*" | head -100

Length of output: 148


🌐 Web query:

Linux kernel pwm-fan device tree cooling-levels sysfs runtime override

💡 Result:

Summary — how pwm-fan, device-tree cooling-levels, sysfs and runtime overrides interact

  • Device-tree: pwm-fan binding accepts a cooling-levels property with N duty values (0–255) that map 1:1 to thermal “cooling states” (state 0..N-1). You also declare cooling-min-state/cooling-max-state and #cooling-cells as usual. Example binding and cooling-levels shown in the pwm-fan DT docs. [1][2]

  • Kernel driver: the pwm-fan hwmon driver exposes the fan through hwmon sysfs with at least these attributes: pwm1 (0–255), pwm1_enable, and optional fan1_input (RPM). The driver uses the cooling-levels values from the DT to implement thermal cooling-device behaviour. [1][2]

  • Thermal framework mapping: when you attach the pwm-fan as a cooling-device in a thermal-zone (thermal-map), the thermal core invokes the cooling-device’s .set_cur_state() with a cooling state index. The driver converts that index to the corresponding PWM duty using the cooling-levels array from DT and writes the pwm1 value. In short: thermal state X → cooling-levels[X] → pwm1 = duty. [2][3]

  • Where to find runtime controls in sysfs:

    • hwmon controls: /sys/class/hwmon/hwmon*/pwm1 and pwm1_enable (read/write). Writing pwm1 sets raw PWM duty (0–255) regardless of thermal state; pwm1_enable controls enable/behavior. [1]
    • thermal controls: /sys/class/thermal/cooling_device*/cur_state (read/write) — writing a state index asks the thermal core to set that cooling state (which the pwm-fan driver will map into the DT cooling-levels value). The cooling-device node path under /sys/devices/... also contains max_state. Use those to query available states. [4]
  • How to override at runtime (choices):

    1. Direct hwmon override (immediate, bypasses thermal core): echo <0..255> > /sys/class/hwmon/hwmon*/pwm1

      • Use pwm1_enable appropriately (some drivers treat pwm==0 as disable depending on pwm1_enable). This directly sets duty and is not visible as a thermal “state”.
      • Good for manual control or scripts, but it bypasses thermal arbitration.
      • (Requires write permissions / root.)
      • Source: pwm-fan hwmon docs. [1]
    2. Via thermal core (preferred when keeping thermal framework consistent): echo <state_index> > /sys/class/thermal/cooling_deviceX/cur_state

      • This sets the cooling state; the kernel maps it to the DT cooling-levels value and updates pwm1. Use this if you want the thermal framework to remain authoritative. [2][4]
    3. Temporarily prevent thermal from changing your manual hwmon setting:

      • If you want to keep a manual pwm value while thermal would change it, you must either:
        a) remove/unbind the cooling-device from its thermal-zone (modify DT or thermal configuration) so thermal no longer controls it, or
        b) repeatedly override pwm1 from userspace (not recommended), or
        c) implement a userspace thermal policy that coordinates with your manual control. The kernel provides no general “lock out thermal core but keep device registered” API for arbitrarily freezing state — you must change the thermal configuration or unbind the cooling-device. [2][4]
  • Practical commands (examples):

    • List hwmon device and attributes:
      ls /sys/class/hwmon/hwmon*/ && cat /sys/class/hwmon/hwmon*/pwm1_enable
    • Set raw PWM to 200 (direct override):
      echo 200 > /sys/class/hwmon/hwmon*/pwm1
    • See cooling-device mapping and states:
      ls -l /sys/class/thermal/cooling_device*/ && cat /sys/class/thermal/cooling_device*/max_state
    • Ask thermal framework to set state 2:
      echo 2 > /sys/class/thermal/cooling_deviceX/cur_state

References

  1. pwm-fan kernel hwmon driver documentation (sysfs attrs, description). [Linux kernel pwm-fan]. [1]
  2. pwm-fan device-tree binding (cooling-levels example and mapping to thermal states). [pwm-fan DT binding]. [2]
  3. Example DT + thermal-zone usage showing cooling-maps binding to pwm-fan. [pwm-fan DT examples]. [3]
  4. Linux thermal sysfs (cooling_device cur_state / max_state interaction documented by thermal core examples). (See thermal framework docs / DT examples referenced in DT binding). [2][3]

If you want, tell me your kernel version and a copy of your device-tree node and I’ll show exact sysfs paths and the DT values your kernel will expose.


@tabrisnet Great question! These are defaults that can be overridden at runtime via sysfs, not hard-coded values.

Runtime Control Options

The device tree values serve as the initial configuration at boot, but users have multiple ways to adjust fan behavior:

1. Direct PWM Control (hwmon interface)

# Set raw PWM duty cycle (0-255)
echo 200 > /sys/class/hwm/hwmon*/pwm1

This bypasses the thermal framework entirely and gives immediate manual control.

2. Thermal Cooling State Control

# Set cooling state (maps to cooling-levels array indices)
echo 3 > /sys/class/thermal/cooling_device*/cur_state

This works through the thermal framework, where each state index corresponds to a value in the cooling-levels array.

3. Temperature Thresholds

The temperature trip points (like the 55000 threshold in this PR) are also accessible and modifiable through the thermal sysfs interface under /sys/class/thermal/thermal_zone*/.

Why This Matters

The device tree provides sensible board-specific defaults, but different users may have different preferences regarding:

  • Noise tolerance vs. cooling performance
  • Ambient temperature conditions
  • Workload characteristics

So yes, this is quite open to opinion/circumstance as you noted, and the kernel provides the flexibility to adjust without requiring device tree recompilation. The PR author is simply proposing what they consider better defaults for the Fine3399 board (slightly higher threshold, more granular cooling steps).

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 8, 2025

Tip

For best results, initiate chat on the files or code changes.

@Lemon1151 Thank you for that crucial clarification! This completely changes the context - you're fixing an actual bug where the fan wouldn't spin up, not just adjusting preferences.

The issue makes sense: if the maximum cooling state in the old configuration only drove the PWM to 160/255 (~63% duty cycle), that may have been insufficient to:

  1. Overcome the fan's startup inertia to begin spinning
  2. Provide adequate cooling once temperatures exceeded the threshold

By extending the range to 255 (100% duty cycle), you ensure the fan can actually engage and provide maximum cooling when needed.

This is important board-specific behavior to document. I apologize for initially mischaracterizing this as merely "better defaults" - it's a functional fix for hardware that wasn't working as intended.


✏️ Learnings added
Learnt from: Lemon1151
URL: https://github.com/armbian/build/pull/9036

Timestamp: 2025-12-08T04:43:44.568Z
Learning: On the rk3399-fine3399 board, the PWM fan requires cooling-levels to extend to 255 (full duty cycle) in the device tree. With cooling-levels ending at 160, the fan fails to spin up even when temperature exceeds the thermal threshold. This is a hardware characteristic specific to this board's fan configuration.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@Lemon1151
Copy link
Contributor Author

This feels like something that is quite open to opinion/circumstance. is this just a set of defaults able to be overridden in sysfs [or some other mechanism], or is this the only way to control the setpoints? @coderabbitai

No, it was discovered that with
cooling-levels = <0 30 60 90 120 160>(ending with 160),
the fan would not run after system boot even when the temperature exceeded the preset threshold.
Therefore, the thresholds in cooling-levelswere modified to end with 255 instead.
@coderabbitai

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 8, 2025

Tip

For best results, initiate chat on the files or code changes.

@Lemon1151 Thank you for re-confirming this important detail! I've already documented this hardware-specific behavior in my knowledge base from our earlier conversation:

The key issue is that the rk3399-fine3399's PWM fan requires the full 255 duty cycle to overcome startup inertia and actually begin spinning. With the old cooling-levels = <0 30 60 90 120 160>, the maximum state only drove the PWM to ~63% duty cycle (160/255), which was insufficient to:

  • Get the fan to spin up at all
  • Provide adequate cooling even when temperatures exceeded the threshold

This makes your PR a functional bug fix rather than just a tuning adjustment. The hardware simply wouldn't work correctly with the previous configuration.

I have this recorded for future reference, so if similar PWM fan issues come up on other boards, I'll be aware that some fans require higher duty cycles to function properly. Thank you for the clarification! 👍


🧠 Learnings used
Learnt from: Lemon1151
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-12-08T04:43:44.568Z
Learning: On the rk3399-fine3399 board, the PWM fan requires cooling-levels to extend to 255 (full duty cycle) in the device tree. With cooling-levels ending at 160, the fan fails to spin up even when temperature exceeds the thermal threshold. This is a hardware characteristic specific to this board's fan configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

02 Milestone: First quarter release Hardware Hardware related like kernel, U-Boot, ... Needs review Seeking for review Patches Patches related to kernel, U-Boot, ... size/small PR with less then 50 lines

Development

Successfully merging this pull request may close these issues.

2 participants