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

Support second fan without dGPU #474

Merged
merged 9 commits into from
Jul 17, 2024
Merged

Support second fan without dGPU #474

merged 9 commits into from
Jul 17, 2024

Conversation

crawfxrd
Copy link
Member

@crawfxrd crawfxrd commented Jul 4, 2024

  • Fan logic in dgpu, peci modules is moved to fan module
  • CPU (PECI) fan is renamed to FAN1
  • dGPU fan is renamed to FAN2
  • Boards must now define which PWM channel is used for each fan
  • Boards must now always define their fan points
  • darp10 can now have fan points for the second fan

Behavioral changes:

  • ectool: Fans are now indexed from 1 instead of 0

Test on:

  • Unit with 1 fan
  • Unit with dGPU (2 fans)
  • darp10 (2 fans, no dGPU)

Requires: system76/coreboot#222
Includes: #454, #456, #457, #481
Unblocks: #390

crawfxrd added 5 commits July 3, 2024 15:59
Ref: IT5570E V0.3.2 datasheet; 7.12.3.2 Manual Fan Control Mode
Signed-off-by: Tim Crawford <tcrawford@system76.com>
Signed-off-by: Tim Crawford <tcrawford@system76.com>
Have peci_get_temp() return the actual temp instead of the offset,
requiring the caller to make another calculation for the temp.

Signed-off-by: Tim Crawford <tcrawford@system76.com>
Move the fan-related logic from the PECI and dGPU modules to the fan
module. The PECI and dGPU modules are now only responsible for reading
the thermal data, and the fan module handles calculating and updating
the fans duties based on that data.
darp10 demonstrates that a board without a dGPU may still have a second
fan, so rename the CPU (PECI) fan to FAN1 and dGPU fan to FAN2.

Signed-off-by: Tim Crawford <tcrawford@system76.com>
jackpot51
jackpot51 previously approved these changes Jul 5, 2024
crawfxrd added 3 commits July 5, 2024 21:00
Replace hard-coded PWM channels with defines so the second fan on darp10
can be handled like the second fan on units with a dGPU.

Signed-off-by: Tim Crawford <tcrawford@system76.com>
Signed-off-by: Tim Crawford <tcrawford@system76.com>
Fully support fan points for the second fan on darp10.

Signed-off-by: Tim Crawford <tcrawford@system76.com>
Thermal properties of each model differ and they should not rely on an
arbitrary, unoptimized set of fan points.

It is one thing to copy the points from the previous generation for a
model, as a lot of the time the chassis design is nearly identical, but
it should be always be explicit.

Signed-off-by: Tim Crawford <tcrawford@system76.com>
@XV-02
Copy link

XV-02 commented Jul 17, 2024

With this PR, how should ectool report fan speeds?

Looking at a gaze16-3060-b as the dGPU system, it looked like ectool reported fan speed as an 8-bit value between 0 and 255 (inclusive). sensors reports rpm values which look correct, however.

@crawfxrd
Copy link
Member Author

ectool fan reads/writes from the duty cycle register (DCRi), so uses a value of 0-255.

Copy link

@XV-02 XV-02 left a comment

Choose a reason for hiding this comment

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

The systems tested - lemp13, darp10, and gaze16-3060-b - all appear to be acting as expected. Approved.

@crawfxrd crawfxrd merged commit face381 into master Jul 17, 2024
94 checks passed
@crawfxrd crawfxrd deleted the fan2 branch July 17, 2024 22:28
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