fan: Replace interpolation, smoothing with stepping

src/board/system76/common/acpi.c

@@ -3,6 +3,7 @@
 #include <board/acpi.h>
 #include <board/battery.h>
 #include <board/dgpu.h>
+#include <board/fan.h>
 #include <board/gpio.h>
 #include <board/kbled.h>
 #include <board/lid.h>
@@ -161,14 +162,14 @@ uint8_t acpi_read(uint8_t addr) {
 
         ACPI_8(0xCC, sci_extra);
 
-        ACPI_8(0xCE, FAN1_PWM);
-        ACPI_16(0xD0, pwm_tach0_rpm);
+        ACPI_8(0xCE, fan1_pwm_actual);
+        ACPI_16(0xD0, fan1_rpm);
 #if CONFIG_HAVE_DGPU
         ACPI_8(0xCD, dgpu_temp);
 #endif // CONFIG_HAVE_DGPU
 #ifdef FAN2_PWM
-        ACPI_8(0xCF, FAN2_PWM);
-        ACPI_16(0xD2, pwm_tach1_rpm);
+        ACPI_8(0xCF, fan2_pwm_actual);
+        ACPI_16(0xD2, fan2_rpm);
 #endif // FAN2_PWM
 
 #if HAVE_LED_AIRPLANE_N

src/board/system76/common/fan.c

@@ -10,21 +10,29 @@
 
 bool fan_max = false;
 
-static uint8_t last_fan1_duty = 0;
-static uint8_t last_fan2_duty = 0;
+uint8_t fan1_pwm_actual = 0;
+uint8_t fan1_pwm_target = 0;
+uint16_t fan1_rpm = 0;
+
+uint8_t fan2_pwm_actual = 0;
+uint8_t fan2_pwm_target = 0;
+uint16_t fan2_rpm = 0;
+
+#define TACH_FREQ (CONFIG_CLOCK_FREQ_KHZ * 1000UL)
+
+// Fan Speed (RPM) = 60 / (1/fs sec * {FnTMRR, FnRLRR} * P)
+// - n: 1 or 2
+// - P: the numbers of square pulses per revolution
+// - fs: sample rate (FreqEC / 128)
+// - {FnTMRR, FnTLRR} = 0000h: Fan Speed is zero
+#define TACH_TO_RPM(x) (60UL * TACH_FREQ / 128UL / 2UL / (x))
 
 #define FAN_POINT(T, D) { .temp = (int16_t)(T), .duty = PWM_DUTY(D) }
 
-#if SMOOTH_FANS != 0
-#define MAX_JUMP_UP ((MAX_FAN_SPEED - MIN_FAN_SPEED) / (uint8_t)SMOOTH_FANS_UP)
-#define MAX_JUMP_DOWN ((MAX_FAN_SPEED - MIN_FAN_SPEED) / (uint8_t)SMOOTH_FANS_DOWN)
-#else
-#define MAX_JUMP_UP (MAX_FAN_SPEED - MIN_FAN_SPEED)
-#define MAX_JUMP_DOWN (MAX_FAN_SPEED - MIN_FAN_SPEED)
+#ifndef FAN1_PWM_MIN
+#define FAN1_PWM_MIN 0
 #endif
 
-#define MIN_SPEED_TO_SMOOTH PWM_DUTY(SMOOTH_FANS_MIN)
-
 // Fan speed is the lowest requested over HEATUP seconds
 #ifndef BOARD_FAN1_HEATUP
 #define BOARD_FAN1_HEATUP 4
@@ -55,11 +63,15 @@ static const struct Fan __code FAN1 = {
     .heatup_size = ARRAY_SIZE(FAN1_HEATUP),
     .cooldown = FAN1_COOLDOWN,
     .cooldown_size = ARRAY_SIZE(FAN1_COOLDOWN),
-    .interpolate = SMOOTH_FANS != 0,
+    .pwm_min = PWM_DUTY(FAN1_PWM_MIN),
 };
 
 #ifdef FAN2_PWM
 
+#ifndef FAN2_PWM_MIN
+#define FAN2_PWM_MIN 0
+#endif
+
 // Fan speed is the lowest requested over HEATUP seconds
 #ifndef BOARD_FAN2_HEATUP
 #define BOARD_FAN2_HEATUP 4
@@ -90,7 +102,7 @@ static const struct Fan __code FAN2 = {
     .heatup_size = ARRAY_SIZE(FAN2_HEATUP),
     .cooldown = FAN2_COOLDOWN,
     .cooldown_size = ARRAY_SIZE(FAN2_COOLDOWN),
-    .interpolate = SMOOTH_FANS != 0,
+    .pwm_min = PWM_DUTY(FAN2_PWM_MIN),
 };
 
 #endif // FAN2_PWM
@@ -100,8 +112,6 @@ void fan_reset(void) {
     fan_max = false;
 }
 
-// Get duty cycle based on temperature, adapted from
-// https://github.com/pop-os/system76-power/blob/master/src/fan.rs
 static uint8_t fan_duty(const struct Fan *const fan, int16_t temp) {
     for (uint8_t i = 0; i < fan->points_size; i++) {
         const struct FanPoint *cur = &fan->points[i];
@@ -112,61 +122,16 @@ static uint8_t fan_duty(const struct Fan *const fan, int16_t temp) {
         } else if (temp < cur->temp) {
             // If lower than first temp, return 0%
             if (i == 0) {
-                return MIN_FAN_SPEED;
+                return 0;
             } else {
                 const struct FanPoint *prev = &fan->points[i - 1];
-
-                if (fan->interpolate) {
-                    // If in between current temp and previous temp, interpolate
-                    if (temp > prev->temp) {
-                        int16_t dtemp = (cur->temp - prev->temp);
-                        int16_t dduty = ((int16_t)cur->duty) - ((int16_t)prev->duty);
-                        return (uint8_t)(
-                            ((int16_t)prev->duty) +
-                            ((temp - prev->temp) * dduty) / dtemp
-                        );
-                    }
-                } else {
-                    return prev->duty;
-                }
+                return prev->duty;
             }
         }
     }
 
     // If no point is found, return 100%
-    return MAX_FAN_SPEED;
-}
-
-static uint8_t fan_smooth(uint8_t last_duty, uint8_t duty) {
-    uint8_t next_duty = duty;
-
-    // ramping down
-    if (duty < last_duty) {
-        // out of bounds (lower) safeguard
-        uint8_t smoothed = last_duty < MIN_FAN_SPEED + MAX_JUMP_DOWN
-            ? MIN_FAN_SPEED
-            : last_duty - MAX_JUMP_DOWN;
-
-        // use smoothed value if above min and if smoothed is closer than raw
-        if (last_duty > MIN_SPEED_TO_SMOOTH && smoothed > duty) {
-            next_duty = smoothed;
-        }
-    }
-
-    // ramping up
-    if (duty > last_duty) {
-        // out of bounds (higher) safeguard
-        uint8_t smoothed = last_duty > MAX_FAN_SPEED - MAX_JUMP_UP
-            ? MAX_FAN_SPEED
-            : last_duty + MAX_JUMP_UP;
-
-        // use smoothed value if above min and if smoothed is closer than raw
-        if (duty > MIN_SPEED_TO_SMOOTH && smoothed < duty) {
-            next_duty = smoothed;
-        }
-    }
-
-    return next_duty;
+    return CTR0;
 }
 
 static uint8_t fan_heatup(const struct Fan *const fan, uint8_t duty) {
@@ -204,48 +169,101 @@ static uint8_t fan_cooldown(const struct Fan *const fan, uint8_t duty) {
 static uint8_t fan_get_duty(const struct Fan *const fan, int16_t temp) {
     uint8_t duty;
 
-    if (power_state == POWER_STATE_S0) {
-        duty = fan_duty(fan, temp);
-        if (fan_max) {
-            duty = PWM_DUTY(100);
-        } else {
-            duty = fan_heatup(fan, duty);
-            duty = fan_cooldown(fan, duty);
-        }
-    } else {
-        duty = PWM_DUTY(0);
-    }
+    duty = fan_duty(fan, temp);
+    duty = fan_heatup(fan, duty);
+    duty = fan_cooldown(fan, duty);
 
     return duty;
 }
 
-void fan_update_duty(void) {
+static uint16_t fan_get_tach0_rpm(void) {
+    uint16_t rpm = (F1TMRR << 8) | F1TLRR;
+
+    if (rpm)
+        rpm = TACH_TO_RPM(rpm);
+
+    return rpm;
+}
+
+static uint16_t fan_get_tach1_rpm(void) {
+    uint16_t rpm = (F2TMRR << 8) | F2TLRR;
+
+    if (rpm)
+        rpm = TACH_TO_RPM(rpm);
+
+    return rpm;
+}
+
+void fan_event(void) {
 #if CONFIG_HAVE_DGPU
     int16_t sys_temp = MAX(peci_temp, dgpu_temp);
 #else
     int16_t sys_temp = peci_temp;
 #endif
 
-    uint8_t fan1_duty = fan_get_duty(&FAN1, sys_temp);
-#ifdef FAN2_PWM
-    uint8_t fan2_duty = fan_get_duty(&FAN2, sys_temp);
-#endif // FAN2_PWM
+    // Fan update interval is 100ms (main.c). The event changes PWM duty
+    // by 1 every interval to give a smoothing effect.
+
+    // Enabling fan max toggle and exiting S0 will cause duty to immediately
+    // change instead of stepping to provide the desired effects.
 
-    // set FAN1 duty
-    if (fan1_duty != FAN1_PWM) {
-        TRACE("FAN1 fan_duty_raw=%d\n", fan1_duty);
-        last_fan1_duty = fan1_duty = fan_smooth(last_fan1_duty, fan1_duty);
-        FAN1_PWM = fan_max ? MAX_FAN_SPEED : fan1_duty;
-        TRACE("FAN1 fan_duty_smoothed=%d\n", fan1_duty);
+    // Set FAN1 duty
+    fan1_pwm_target = fan_get_duty(&FAN1, sys_temp);
+    if (fan_max) {
+        fan1_pwm_target = CTR0;
+        fan1_pwm_actual = CTR0;
+    } else if (power_state != POWER_STATE_S0) {
+        fan1_pwm_target = 0;
+        fan1_pwm_actual = 0;
+    } else {
+        if (fan1_pwm_actual < fan1_pwm_target) {
+            if (fan1_pwm_actual < CTR0) {
+                fan1_pwm_actual++;
+                if (fan1_pwm_actual < FAN1.pwm_min) {
+                    fan1_pwm_actual = FAN1.pwm_min;
+                }
+            }
+        } else if (fan1_pwm_actual > fan1_pwm_target) {
+            if (fan1_pwm_actual > 0) {
+                fan1_pwm_actual--;
+                if (fan1_pwm_actual < FAN1.pwm_min) {
+                    fan1_pwm_actual = 0;
+                }
+            }
+        }
     }
+    TRACE("FAN1 duty=%d\n", fan1_pwm_actual);
+    FAN1_PWM = fan1_pwm_actual;
+    fan1_rpm = fan_get_tach0_rpm();
 
 #ifdef FAN2_PWM
     // set FAN2 duty
-    if (fan2_duty != FAN2_PWM) {
-        TRACE("FAN2 fan_duty_raw=%d\n", fan2_duty);
-        last_fan2_duty = fan2_duty = fan_smooth(last_fan2_duty, fan2_duty);
-        FAN2_PWM = fan_max ? MAX_FAN_SPEED : fan2_duty;
-        TRACE("FAN2 fan_duty_smoothed=%d\n", fan2_duty);
+    fan2_pwm_target = fan_get_duty(&FAN2, sys_temp);
+    if (fan_max) {
+        fan2_pwm_target = CTR0;
+        fan2_pwm_actual = CTR0;
+    } else if (power_state != POWER_STATE_S0) {
+        fan2_pwm_target = 0;
+        fan2_pwm_actual = 0;
+    } else {
+        if (fan2_pwm_actual < fan2_pwm_target) {
+            if (fan2_pwm_actual < CTR0) {
+                fan2_pwm_actual++;
+                if (fan2_pwm_actual < FAN2.pwm_min) {
+                    fan2_pwm_actual = FAN2.pwm_min;
+                }
+            }
+        } else if (fan2_pwm_actual > fan2_pwm_target) {
+            if (fan2_pwm_actual > 0) {
+                fan2_pwm_actual--;
+                if (fan2_pwm_actual < FAN2.pwm_min) {
+                    fan2_pwm_actual = 0;
+                }
+            }
+        }
     }
+    TRACE("FAN2 duty=%d\n", fan2_pwm_actual);
+    FAN2_PWM = fan2_pwm_actual;
+    fan2_rpm = fan_get_tach1_rpm();
 #endif
 }

src/board/system76/common/include/board/fan.h

@@ -7,25 +7,6 @@
 #include <stdint.h>
 
 #define PWM_DUTY(X) ((uint8_t)(((((uint16_t)(X)) * 255) + 99) / 100))
-#define MAX_FAN_SPEED PWM_DUTY(100)
-#define MIN_FAN_SPEED PWM_DUTY(0)
-
-#ifndef SMOOTH_FANS
-#define SMOOTH_FANS 1 // default to fan smoothing
-#endif
-
-#if SMOOTH_FANS != 0
-#ifndef SMOOTH_FANS_UP
-#define SMOOTH_FANS_UP 45 // default to ~11 seconds for full ramp-up
-#endif
-#ifndef SMOOTH_FANS_DOWN
-#define SMOOTH_FANS_DOWN 100 // default to ~25 seconds for full ramp-down
-#endif
-#endif
-
-#ifndef SMOOTH_FANS_MIN
-#define SMOOTH_FANS_MIN 0 // default to smoothing all fan speed changes
-#endif
 
 struct FanPoint {
     int16_t temp;
@@ -39,12 +20,19 @@ struct Fan {
     uint8_t heatup_size;
     uint8_t *cooldown;
     uint8_t cooldown_size;
-    bool interpolate;
+    uint8_t pwm_min;
 };
 
 extern bool fan_max;
 
+extern uint8_t fan1_pwm_actual;
+extern uint8_t fan1_pwm_target;
+extern uint16_t fan1_rpm;
+extern uint8_t fan2_pwm_actual;
+extern uint8_t fan2_pwm_target;
+extern uint16_t fan2_rpm;
+
 void fan_reset(void);
-void fan_update_duty(void);
+void fan_event(void);
 
 #endif // _BOARD_FAN_H

src/board/system76/common/include/board/pwm.h

@@ -3,15 +3,6 @@
 #ifndef _BOARD_PWM_H
 #define _BOARD_PWM_H
 
-#include <ec/pwm.h>
-
-// NOTE: These are used instead of the functions directly for ACPI to prevent
-// double reads of the tachometer values.
-extern int16_t pwm_tach0_rpm;
-extern int16_t pwm_tach1_rpm;
-
 void pwm_init(void);
-int16_t pwm_get_tach0_rpm(void);
-int16_t pwm_get_tach1_rpm(void);
 
 #endif // _BOARD_PWM_H